Re: [Xen-devel] [PATCH v2 for-next v2 5/8] x86/mm: split PV guest supporting code to pv/mm.c

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Apr 19, 2017 at 09:31:12AM -0600, Jan Beulich wrote:
> >>> On 03.04.17 at 13:22, <wei.liu2@xxxxxxxxxx> wrote:
> > -static s8 __read_mostly opt_mmio_relax;
> > +s8 __read_mostly opt_mmio_relax;
> >  static void __init parse_mmio_relax(const char *s)
> 
> The variable has exactly one use site outside of the parsing function,
> which means both variable and parsing function should be moved too.
> However, this sole user is get_page_from_l1e(), and that in turn is
> also being used from shadow code. It's not immediately clear to me
> whether moving it under pv/ (and hence eventually compiling it out
> of Xen when PV is de-selected) is the right thing.
> 

Yes, you're right. I read the shadow code but couldn't immediately
figure out whether the call site of get_page_from_l1e is PV specific.

I plan to leave get_page_from_l1e in mm.c and make adjustments to the
series where necessary.

> > -static int get_page_from_pagenr(unsigned long page_nr, struct domain *d)
> > +int get_page_from_pagenr(unsigned long page_nr, struct domain *d)
> >  {
> 
> I don't think this should be made non-static without first correcting its
> return type - either int (and returning -E... values) or bool.
> 

Another patch it is.

> > @@ -834,3414 +524,489 @@ static int update_xen_mappings(unsigned long mfn, 
> > unsigned int cacheattr)
> >      return err;
> >  }
> >  
> > -#ifndef NDEBUG
> > -struct mmio_emul_range_ctxt {
> > -    const struct domain *d;
> > -    unsigned long mfn;
> > -};
> > -
> > -static int print_mmio_emul_range(unsigned long s, unsigned long e, void 
> > *arg)
> > +bool_t fill_ro_mpt(unsigned long mfn)
> >  {
> > -    const struct mmio_emul_range_ctxt *ctxt = arg;
> > -
> > -    if ( ctxt->mfn > e )
> > -        return 0;
> > +    l4_pgentry_t *l4tab = map_domain_page(_mfn(mfn));
> > +    bool_t ret = 0;
> 
> From here on the patch becomes basically unreviewable, because
> plain removals (which I'd expect is all that is happening here) aren't
> represented as such anymore. Please see whether you can
> arrange for the diff to become more legible. If you can't talk git
> into doing so, maybe an option would be to split this patch by
> temporarily #include-ing pv/mm.c here until the move is complete.
> 

I will see what I can do here. I will probably end up splitting it to
more patches.

Wei.

> Jan
> 

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