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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

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


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?

~Andrew