Re: [Xen-devel] [PATCH v3 4/5] x86/HVM: fix pinned cache attribute handling

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

forgot the dumb questions. some problem on my mail rules, so only [4/5]
reaches my inbox, while others are redirected to other folders. will review
them together. :-)

> -----Original Message-----
> From: Tian, Kevin
> Sent: Thursday, April 10, 2014 4:31 PM
> To: 'Jan Beulich'; xen-devel
> Cc: Dong, Eddie; Nakajima, Jun; Keir Fraser; Tim Deegan
> Subject: RE: [PATCH v3 4/5] x86/HVM: fix pinned cache attribute handling
> 
> > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > Sent: Monday, April 07, 2014 6:10 PM
> >
> > - make sure UC- is only used for PAT purposes (MTRRs and hence EPT
> >   don't have this type)
> > - add order input to "get", and properly handle conflict case (forcing
> >   an EPT page split)
> 
> could you elaborate a bit how an EPT page split is forced with this change?
> 
> > - properly detect (and refuse) overlaps during "set"
> > - properly use RCU constructs
> > - support deleting ranges through a special type input to "set"
> > - set ignore-PAT flag in epte_get_entry_emt() when "get" succeeds
> > - set "get" output to ~0 (invalid) rather than 0 (UC) on error (the
> >   caller shouldn't be looking at it anyway)
> > - move struct hvm_mem_pinned_cacheattr_range from header to C file
> >   (used only there)
> >
> > Note that the code (before and after this change) implies the GFN
> > ranges passed to the hypercall to be inclusive, which is in contrast
> > to the sole current user in qemu (all variants). It is not clear to me
> > at which layer (qemu, libxc, hypervisor) this would best be fixed.
> >
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > Reviewed-by: Tim Deegan <tim@xxxxxxx>
> >
> > @@ -732,8 +801,14 @@ int epte_get_entry_emt(struct domain *d,
> >      if ( !mfn_valid(mfn_x(mfn)) )
> >          return MTRR_TYPE_UNCACHABLE;
> >
> > -    if ( hvm_get_mem_pinned_cacheattr(d, gfn, &type) )
> > -        return type;
> > +    switch ( hvm_get_mem_pinned_cacheattr(d, gfn, order, &type) )
> > +    {
> > +    case 1:
> > +        *ipat = 1;
> > +        return type != PAT_TYPE_UC_MINUS ? type :
> > PAT_TYPE_UNCACHABLE;
> > +    case -1:
> > +        return -1;
> > +    }
> 
> what does '-1' mean for a cache type? I didn't see callers check this
> value, e.g:
> 
>     /* Construct the new entry, and then write it once */
>     new_entry.emt = epte_get_entry_emt(p2m->domain, gfn, mfn, &ipat,
>                                     direct_mmio);
> 
> Thanks
> Kevin

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