On Wed, Apr 13, 2016 at 9:01 AM, Andrew Cooper <andrew.coop...@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
