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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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)...

>> 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.

> 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.

Jan


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