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

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Fri, 2014-03-28 at 14:43 +0000, Jan Beulich wrote:
> >>> On 28.03.14 at 14:36, <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> > On Fri, 2014-03-28 at 13:27 +0000, Jan Beulich wrote:
> >> >>> On 28.03.14 at 14:19, <JBeulich@xxxxxxxx> wrote:
> >> > - 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)
> >> > - 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.
> >> 
> >> Actually I think I should point you guys more explicitly at the above
> >> (forgot to Cc you when sending the patch).
> > 
> > This is about XEN_DOMCTL_pin_mem_cacheattr?
> > 
> > Is it the case that we are failing to set the cacheattrs of the last
> > page in the range?
> 
> No, the attributes get set for one page too many.

Just as bad I guess ;-)

> > I think it is generally better if libxc remains a pretty thin layer over
> > the hypercall, so adjustments to the inclusiveness of the arguments
> > don't really belong there IMHO.
> > 
> > What is considered more normal for our hypercall interfaces, in- or
> > exclusive?
> 
> I think ranges specified via two GFNs should generally be inclusive, in
> order to make sure they can extend to the end of address space.

This is a strong argument IMHO and would seem to suggest that qemu is at
fault and is the one which should be fixed.

> > For the cacheflush thing I added recently you suggested to use a nrpages
> > instead of end which nicely sidestepped the issue. Is that an option
> > here while you are changing things anyway?
> 
> I considered that, but didn't want to propose it because such a
> change to the interface would go un-noticeable to the caller (at
> build time). But of course it is an option.

Right that is a real downside.

In the domctl struct itself you would naturally change the field name,
so that would catch that.

For libxc, which is how all the real callers end up calling it, I guess
you would have to change the function name, which would also allow the
existing one to remain for compatibility/transitional purposes (if we
wanted that).

xc_domain_pin_memory_cacheattr_range isn't a very good name, but it is
not terrible I suppose.

(I'm not necessarily advocating this approach, just thinking it through)

Ian.


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