Re: [PATCH v2] x86/vm_event: toggle singlestep from vm_event response

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/06/2015 07:26 PM, Lengyel, Tamas wrote:
> 
> 
> On Mon, Jul 6, 2015 at 11:54 AM, Jan Beulich <JBeulich@suse.com
> <mailto:JBeulich@suse.com>> wrote:
> 
>     >>> On 06.07.15 at 17:35, <tlengyel@novetta.com
>     <mailto:tlengyel@novetta.com>> wrote:
>     > On Mon, Jul 6, 2015 at 11:26 AM, Jan Beulich <JBeulich@suse.com
>     <mailto:JBeulich@suse.com>> wrote:
>     >
>     >> >>> On 30.06.15 at 16:40, <tlengyel@novetta.com
>     <mailto:tlengyel@novetta.com>> wrote:
>     >> > On Tue, Jun 30, 2015 at 10:18 AM, Andrew Cooper <
>     >> andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>>
>     >> > wrote:
>     >> >
>     >> >> On 30/06/15 15:11, Tamas K Lengyel wrote:
>     >> >> > diff --git a/xen/include/public/vm_event.h
>     >> >> b/xen/include/public/vm_event.h
>     >> >> > index 577e971..b8c3dde 100644
>     >> >> > --- a/xen/include/public/vm_event.h
>     >> >> > +++ b/xen/include/public/vm_event.h
>     >> >> > @@ -44,9 +44,15 @@
>     >> >> >   *  paused
>     >> >> >   * VCPU_PAUSED in a response signals to unpause the vCPU
>     >> >> >   */
>     >> >> > -#define VM_EVENT_FLAG_VCPU_PAUSED     (1 << 0)
>     >> >> > -/* Flags to aid debugging mem_event */
>     >> >> > -#define VM_EVENT_FLAG_FOREIGN         (1 << 1)
>     >> >> > +#define VM_EVENT_FLAG_VCPU_PAUSED       (1 << 0)
>     >> >> > +/* Flag to aid debugging mem_event */
>     >> >> > +#define VM_EVENT_FLAG_FOREIGN           (1 << 1)
>     >> >> > +/*
>     >> >> > + * Toggle singlestepping on vm_event response.
>     >> >> > + * Requires the vCPU to be paused already (synchronous
>     events only).
>     >> >> > + * Only supported on Intel CPUs with MTF capability.
>     >> >>
>     >> >> This sentence shouldn't be in the public API.  It is a
>     limitation of the
>     >> >> current implementation, not of the API, and could be removed with
>     >> >> further development.
>     >> >>
>     >> >
>     >> > I disagree because there is no error condition returned if a
>     user tries
>     >> to
>     >> > use it on non-Intel hw, so the only option a user would have to
>     figure
>     >> out
>     >> > why it's not working is reading the Xen source. IMHO the public API
>     >> should
>     >> > describe the limitations as that's what potential users will
>     read first.
>     >> > When we have hardware other then Intel that supports something
>     like this,
>     >> > we can remove the comment.
>     >>
>     >> FWIW I agree with Andrew, and if on non-Intel hardware there's
>     >> no error (or other indication) being returned, that's actually an
>     >> issue to be fixed imo.
>     >
>     > There is no opportunity for that, the current API does not provide a
>     > mechanism to signal failure on things that were requested on the vm_event
>     > response. Creating such a mechanism is beyond the scope of this patch and I
>     > don't think it's necessary. IMHO the comment makes it clear that this will
>     > only work on Intel hardware which suffices for now.
> 
>     You're the maintainer of the code in question, so I won't (and
>     can't) enforce Andrew's and my view.
> 
>     Jan
> 
> 
> Unless Razvan have a different opinion on the matter (although he
> already Acked it), I think it's good as is.

I don't mind just having the comment for now, so for what it's worth I
stand by my ack.

Having said that (and with the understading that it is beyond the scope
of this patch), a way to validate things like these is a good idea. I
wonder if, in a future patch, we could not have ./configure detect these
things and simply disable the relevant VM_EVENT_FLAG constants with
#if(n)defs, for example. That way, you wouldn't be able to compile code
that wouldn't work silently on platforms where that is the case.


Regards,
Razvan