Re: [Xen-devel] [PATCH v4 2/2] x86/PV: support data breakpoint extension registers

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Wed, 2014-04-23 at 13:35 +0100, Jan Beulich wrote:
> >>> On 23.04.14 at 14:23, <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> > On Wed, 2014-04-23 at 12:52 +0100, Jan Beulich wrote:
> >> >>> On 23.04.14 at 12:23, <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> >> > On Wed, 2014-04-16 at 15:34 +0100, Jan Beulich wrote:
> >> >> @@ -583,6 +593,7 @@ struct xen_domctl_ext_vcpucontext {
> >> >>      uint16_t         sysenter_callback_cs;
> >> >>      uint8_t          syscall32_disables_events;
> >> >>      uint8_t          sysenter_disables_events;
> >> >> +    uint16_t         msr_count;
> >> >>  #if defined(__GNUC__)
> >> >>      union {
> >> >>          uint64_aligned_t mcg_cap;
> >> >> @@ -591,6 +602,7 @@ struct xen_domctl_ext_vcpucontext {
> >> >>  #else
> >> >>      struct hvm_vmce_vcpu vmce;
> >> >>  #endif
> >> >> +    XEN_GUEST_HANDLE_64(xen_domctl_ext_vcpu_msr_t) msrs;
> >> > 
> >> > I must be missing something because I can't see where the tools are
> >> > initialising msrs, nor does the hypervisor appear to check it is valid
> >> > before trying to save stuff to it (although that would be caught by the
> >> > copy_to_user I expect).
> >> > 
> >> > Also how does one go about determining the correct msr_count before
> >> > retrieving this state?
> >> 
> >> When msr_count is zero and MSRs are there that need storing, the
> >> call will return -ENOBUFS and set msr_count to the required (minimum)
> >> value. Furthermore the field is only being looked at if the size stored
> >> inside the structure covers the entire msrs field. And yes, if
> >> msr_count is non-zero but msrs doesn't point to a valid memory block,
> >> copy_to_guest() will catch this (as usual).
> > 
> > All makes sense. Worth a comment though?
> 
> Not sure - it's no more subtle than other code in the handling of that
> specific sub-hypercall. But yes, by my own argumentation elsewhere
> maybe I shouldn't be extending badness - if only I saw ways of
> describing things like this without just converting C to human language
> (which doesn't seem all that helpful)...

Nothing the behaviour of msr_count you describe doesn't sound like just
converting the C to human readable to me, unless you expect readers of
this interface header to go digging into the implementation to figure
this out, which shouldn't be needed (that's the point of docs after all!
)

> >> So as is the tools are fine not explicitly setting msr_count (it's being
> >> implicitly set to zero) - state save will fail in that case. It's the 3rd
> >> (unfinished and now delayed until after Andrew's re-write) patch
> >> that would be dealing with that error, re-issuing the call after
> >> allocating a suitable array.
> > 
> > That would be a bisection hazard wouldn't it?
> 
> Only with guests using the extension and wanting to be migrated.

I hadn't realised there was this condition as well (since hte feature
looks more generic at the level I'm looking at it).

So any such guests would fail to migrate today anyway, right?

> > The tools side needs to be
> > part of this patch I think. In any case I suppose this series should
> > wait until the 3rd patch is available?
> 
> That might mean I'd have to carry this for months. I'd rather want
> to get this in as is, and live with save/migration not working for
> guests using the extension. Even more so that I'm hoping to get
> someone more fluent in libxc speak to take care of that.

Yes, so long as it isn't a regression I think that is fine.

Ian.


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