Re: [Xen-devel] [PATCH 1/4] x86/domctl: Implement XEN_DOMCTL_{get, set}_vcpu_msrs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.06.14 at 16:53, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 05/06/14 14:33, Jan Beulich wrote:
>> <snip>
>>> The sole use of this hypercall needs to ensure that all MSRs are gotten,
>>> otherwise VM corruption will occur.  Permitting a partial get will make
>>> the return value ambiguous for making this hypercall a single time and
>>> guessing at the size to use, although I suspect we are less interested
>>> in this problem.
>> Why would the return value ambiguous? You'd get -ENOBUFS if you
>> provided too few slots, and you'd get to know the maximum number
>> at that point at once.
>>
>> Jan
>>
> 
> Having tried to implement these improvements, I hit problems so would
> like to decide upon an interface before hacking futher.
> 
> Currently behaviour for get:
> * Null guest handle returns msr_count set to maximum number of msrs Xen
> might write
> * msr_count < max_msrs fails with -ENOBUFS
> * if msrs are written, msr_count reflects the number written (likely
> less than max_msrs)
> 
> Current behaviour for set:
> * msr_count > max_msrs fails with -EINVAL
> * problems with individual msrs fail with -EINVAL
> 
> Suggestions:
> * for get, msr_count < max_msrs should perform a partial write,
> returning -ENOBUFS if Xen needs to write more than msr_count msrs.
> 
> This reduces the amount of code added to xc_domain_save() to fail
> migrations actually using PV msrs.  I am not too concerned about this
> code, as it will be rm'd in the migration-v2 series which implements PV
> MSR migration properly.  I am a little bit hesitant about supporting
> partial writes, although I suppose it is plausible to want to know "how
> many MSRs is the vcpu currently using", and doing that with a single
> hypercall is preferable to requiring two.

Yes. I didn't see above what problem you found with this.

> * for set, in the case of a bad msr, identify it back to the caller to
> aid with debugging.
> 
> This is useful to help debugging, but needs disambiguating against the
> other cases which fail with -EINVAL, including the paths which would
> fail before having a chance to set msr_count to the index of the bad
> msr.  Therefore, msr_count *can't* be overloaded for this purpose.

Actually it can - the caller will know the number it put there, and if it's
unchanged then the failure was not associated with a particular array
entry (all possible values on error would be smaller than the value
originally there).

> I see one solution to these problems.  Using:
> 
> struct xen_domctl_vcpu_msrs {
>     u32 vcpu;
>     union { u16 max_msrs, /* OUT from get */
>             u16 err_idx}; /* Possibly OUT from set */
>     u16 msr_count;
>     XEN_GUEST_HANDLE_64(xen_domctl_vcpu_msr_t) msrs;
> };
> 
> max_msrs and current msrs can be reported at the same time (both on a
> NULL guest handle).  If the caller of set sets err_idx to ~0 before the
> call, it can unambiguously determine the offending MSR, without
> confusing other -EINVAL failure cases.
> 
> Does this look plausible? Can we get away with anonymous unions in the
> public header files?

No, nothing that isn't C89 is permitted in the public headers.

Jan


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