On Wed, Apr 13, 2016 at 9:01 AM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> On 13/04/16 15:56, Razvan Cojocaru wrote:
> > On 04/13/2016 05:52 PM, Tamas K Lengyel wrote:
> >>     >> diff --git a/xen/include/public/domctl.h
> >>     b/xen/include/public/domctl.h
> >>     >> index 2457698..875c09a 100644
> >>     >> --- a/xen/include/public/domctl.h
> >>     >> +++ b/xen/include/public/domctl.h
> >>     >> @@ -1107,8 +1107,7 @@ struct xen_domctl_monitor_op {
> >>     >>          } mov_to_cr;
> >>     >>
> >>     >>          struct {
> >>     >> -            /* Enable the capture of an extended set of MSRs */
> >>     >> -            uint8_t extended_capture;
> >>     >> +            uint32_t msr;
> >>     >
> >>     > Whoa there. Isn't it expanding the structure? Will this be
> backwards
> >>     > compatible? What if somebody is using an older version of
> xen-access
> >>     > against this hypervisor? Will they work?
> >>     >
> >>     > Perhaps this should have a new struct / sub-ops? And the old
> >>     > 'mov_to_msr' will just re-use this new fangled code?
> >>
> >>     In addition to Andrew's comments, I think simply changing
> >>     VM_EVENT_INTERFACE_VERSION should be enough for xen-access-like
> clients
> >>     to figure out the incompatibility.
> >>
> >>
> >>
> >> This is an independent system from VM_EVENT, so IMHO the two shouldn't
> >> be mixed. The union size right now is 24-bits so if a uint16_t is enough
> >> for the bitmask that should be used instead. That way we don't end up
> >> growing the struct size.
> > Right. Well, MSR-s seem to be passed around as 32-bit unsigned integers
> > everywhere in the Xen source code, so unless that also needs correcting
> > then unfortunately it'll have to grow.
>
> MSR indices are always 32bits wide, as they live specifically in %ecx
> when encoded for instructions.
>
> Only 2K MSRs are currently specified in hardware, with some extra ones
> in the hypervisor range, but this doesn't mean that list won't grow in
> the future.
>

Yea, well then we need to introduce a new struct with a new subop to pass
the bitmask. I guess its a lesson in ABI design to leave some wiggle room
for future-proofing it (my bad). So I guess we can introduce
XEN_DOMCTL_MONITOR_OP_ENABLE_V2 and struct xen_domctl_monitor_op_v2 where
say expand the union to uint64_t just in case?

Tamas