Re: [Xen-devel] [PATCH v6 3/9] x86/mm: add disallow_mask parameter to get_page_from_l1e

[Tim Deegan <tim@xxxxxxx>]

At 09:54 -0600 on 13 Mar (1520934871), Jan Beulich wrote:
> >>> On 13.02.18 at 21:04, <wei.liu2@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/mm/shadow/multi.c
> > +++ b/xen/arch/x86/mm/shadow/multi.c
> > @@ -858,13 +858,21 @@ shadow_get_page_from_l1e(shadow_l1e_t sl1e, struct 
> > domain *d, p2m_type_t type)
> >      int res;
> >      mfn_t mfn;
> >      struct domain *owner;
> > +    /* The disallow mask is taken from arch/x86/mm.c for HVM guest */
> > +    uint32_t disallow_mask =
> > +        ~(_PAGE_PRESENT | _PAGE_RW | _PAGE_USER | _PAGE_ACCESSED |
> > +          _PAGE_DIRTY | _PAGE_AVAIL | _PAGE_AVAIL_HIGH | _PAGE_NX);
> >  
> > +    disallow_mask = (disallow_mask | _PAGE_GNTTAB) & ~_PAGE_GLOBAL;
> > +    disallow_mask &= ~PAGE_CACHE_ATTRS;
> 
> If any of this is needed in the first place (see below), at least this
> last line could be folded into the variable's initializer as it looks.

Building it piecewise is fine by me, but since this is a constant it
should probably be declared once in some header file rather than here.
In any case it needs a comment that describes what it is, and not that
it used to live in mm.c. :)

Like Jan, I'm not convinced that having this be an argument to
get_page_from_l1e is super-useful -- it seems like an opportunity for
one of the callers to get it wrong and fail to enforce our invariants.

> > +    ASSERT(is_hvm_domain(d));

This assertion isn't particularly helpful IMO (because it's
shadow_mode_refcounts that's relevant here, not is_hvm) but I don't
object to it.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel