Re: [Xen-devel] [PATCH V2] vm_event: Allow subscribing to write events for specific MSR-s

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 04/13/2016 06:05 PM, Tamas K Lengyel wrote:
> 
> 
> On Wed, Apr 13, 2016 at 9:01 AM, Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx <mailto:andrew.cooper3@xxxxxxxxxx>> 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?

I can do that, but it would seem that this is somewhat at odds with
Andrew Cooper's perspective - he has stated that it's within the rules
and the domctl can be changed without there being the need for
XEN_DOMCTL_MONITOR_OP_ENABLE_V2. So this should be clarified, please,
otherwise I'm incurring the risk of changing the code only to have to
revert it later.


Thanks,
Razvan

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