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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Sep 01, 2017 at 02:46:27PM +0100, Wei Liu wrote:
> 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?

Oh, you mean

   .lma = addr_size > 32

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