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

From: Tamas K Lengyel <tamas@tklengyel.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	"wei.liu2@citrix.com" <wei.liu2@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Xen-devel <xen-devel@lists.xen.org>,
	Jun Nakajima <jun.nakajima@intel.com>, Keir Fraser <keir@xen.org>
Subject: Re: [PATCH V2] vm_event: Allow subscribing to write events for specific MSR-s
Date: Wed, 13 Apr 2016 09:05:18 -0600	[thread overview]
Message-ID: <CABfawh=XJLeysnZnq5WuF28Fn90QU5bZffo7dxLYdvrNnvtMfA@mail.gmail.com> (raw)
In-Reply-To: <570E5F4F.7070506@citrix.com>

[-- Attachment #1.1: Type: text/plain, Size: 2363 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 3354 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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