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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Apr 13, 2016 at 9:01 AM, Andrew Cooper <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?

Tamas

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