Re: [PATCH v2 2/2] x86/mm: merge ptwr and mmio_ro page fault handlers

[Wei Liu <wei.liu2@citrix.com>]

On Fri, Sep 01, 2017 at 03:38:49AM -0600, Jan Beulich wrote:
> >  /*************************
> >   * fault handling for read-only MMIO pages
> >   */
> 
> Note how you move the remains of the function above below this
> comment, which isn't really correct.

I can place the new function where the old was.
> 
[..]
> > +int pv_ro_page_fault(struct vcpu *v, unsigned long addr,
> 
> Since you alter this and the caller anyway, the first parameter
> could as well become const struct domain *, simplifying ...
> 
> > +                     struct cpu_user_regs *regs)
> > +{
> > +    l1_pgentry_t pte;
> > +    struct domain *d = v->domain;
> > +    unsigned int addr_size = is_pv_32bit_vcpu(v) ? 32 : BITS_PER_LONG;
> > +    struct x86_emulate_ctxt ctxt = {
> > +        .regs      = regs,
> > +        .vendor    = d->arch.cpuid->x86_vendor,
> > +        .addr_size = addr_size,
> > +        .sp_size   = addr_size,
> > +        .lma       = !is_pv_32bit_vcpu(v),
> 
> ... both is_pv_32bit_vcpu() accesses here (which internally use
> v->domain). In fact I wonder whether it wouldn't yield more
> consistent code if you didn't pass in the domain at all, as this
> must strictly be current->domain, or invoking x86_emulate() and
> various other functions is bogus. You'd then latch this into currd
> here.
> 
> Furthermore I think this second use could become "addr_size > 32".
> 

Sorry, second use of what?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel