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

["Lengyel, Tamas" <tlengyel@xxxxxxxxxxx>]

On Mon, Jul 6, 2015 at 11:54 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 06.07.15 at 17:35, <tlengyel@xxxxxxxxxxx> wrote:
> On Mon, Jul 6, 2015 at 11:26 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
>> >>> On 30.06.15 at 16:40, <tlengyel@xxxxxxxxxxx> wrote:
>> > On Tue, Jun 30, 2015 at 10:18 AM, Andrew Cooper <
>> andrew.cooper3@xxxxxxxxxx>
>> > 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.

Thanks,

Tamas

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