Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Tamas Lengyel]

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

Hi everyone,
commit 6ae2df93c277b4093b3e54c9606387
d1ba6d10fe into xen-staging includes a new function in xenctrl.h,
xc_mem_event_enable. This function name however has been used previously in
xenctrl.h up till at least Xen 4.1.2 for a different purpose. We have been
using autoconf to check which version of the mem_access API is available in
Xen by checking if xc_mem_event_enable is available, signaling that the
mem_access API is Xen 4.1 style, and for xc_mem_access_enable signaling
4.2+ style API. See
https://github.com/bdpayne/libvmi/blob/master/configure.ac#L140 for more
details.

Now with this function being reintroduced, it becomes more complicated to
determine which version of the mem_access API does Xen actually provide. A
#define indicating mem_access API version would nicely overcome this issue,
or  naming xc_mem_event_enable something else.

Furthermore, the new xc_mem_event_enable function unconditionally unpauses
the VM. This may not be a desired behavior in all cases, especially if the
VM was in a paused state when the function was called.

Best,
Tamas

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

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

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

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Ian Campbell]

On Mon, 2014-06-23 at 18:31 +0200, Tamas Lengyel wrote:
> Hi everyone,
> commit 6ae2df93c277b4093b3e54c9606387
> d1ba6d10fe into xen-staging includes a new function in xenctrl.h,
> xc_mem_event_enable. This function name however has been used
> previously in xenctrl.h up till at least Xen 4.1.2 for a different
> purpose. We have been using autoconf to check which version of the
> mem_access API is available in Xen by checking if xc_mem_event_enable
> is available, signaling that the mem_access API is Xen 4.1 style, and
> for xc_mem_access_enable signaling 4.2+ style API. See
> https://github.com/bdpayne/libvmi/blob/master/configure.ac#L140 for
> more details.
> 
> 
> Now with this function being reintroduced, it becomes more complicated
> to determine which version of the mem_access API does Xen actually
> provide. A #define indicating mem_access API version would nicely
> overcome this issue, or  naming xc_mem_event_enable something else.

Doesn't configure support checking for functions with a given prototype?

> Furthermore, the new xc_mem_event_enable function unconditionally
> unpauses the VM. This may not be a desired behavior in all cases,
> especially if the VM was in a paused state when the function was
> called.

domain pauses are referenced counted on the hypervisor side.

Ian.

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Tamas Lengyel]

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

> > Now with this function being reintroduced, it becomes more complicated
> > to determine which version of the mem_access API does Xen actually
> > provide. A #define indicating mem_access API version would nicely
> > overcome this issue, or  naming xc_mem_event_enable something else.
>
> Doesn't configure support checking for functions with a given prototype?
>

It does but in a very hacky way, essentially trying to compile code where
the function is being called with different prototypes. We can work around
it but a clean solution would be preferred at some point.



> > Furthermore, the new xc_mem_event_enable function unconditionally
> > unpauses the VM. This may not be a desired behavior in all cases,
> > especially if the VM was in a paused state when the function was
> > called.
>
> domain pauses are referenced counted on the hypervisor side.
>

> Ian.
>
>
I did a quick test with xc_domain_pause being called twice, then
xc_domain_unpause once and the domain was in unpaused state at the end.
Same with the xen-access example, a paused domain gets automatically
unpaused by libxc, either though the xen-access code doesn't specifically
say it should be. This is now a rather opaque behavior of libxc. As it is
not merged into master I guess I will just submit a patch for it..

Tamas

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

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

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

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Ian Jackson]

Tamas Lengyel writes ("Re: [Xen-devel] Issues regarding "mem_access: Add helper API to setup ring and enable mem_access""):
> 
>     > Now with this function being reintroduced, it becomes more complicated
>     > to determine which version of the mem_access API does Xen actually
>     > provide. A #define indicating mem_access API version would nicely
>     > overcome this issue, or  naming xc_mem_event_enable something else.
> 
>     Doesn't configure support checking for functions with a given prototype?
> 
> It does but in a very hacky way, essentially trying to compile code where the
> function is being called with different prototypes. We can work around it but a
> clean solution would be preferred at some point.

I agree with your criticism, TBH.  Aravindh/Ian, can we rename this
function ?

> This is now a rather opaque behavior of libxc. As it is not merged
> into master I guess I will just submit a patch for it..

Yes.  I think this would be the right approach to addressing both of
these problems.  (Please send two patches, one for each.)

Thanks,
Ian.

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Ian Campbell]

On Mon, 2014-06-30 at 13:42 +0100, Ian Jackson wrote:
> Tamas Lengyel writes ("Re: [Xen-devel] Issues regarding "mem_access: Add helper API to setup ring and enable mem_access""):
> > 
> >     > Now with this function being reintroduced, it becomes more complicated
> >     > to determine which version of the mem_access API does Xen actually
> >     > provide. A #define indicating mem_access API version would nicely
> >     > overcome this issue, or  naming xc_mem_event_enable something else.
> > 
> >     Doesn't configure support checking for functions with a given prototype?
> > 
> > It does but in a very hacky way, essentially trying to compile code where the
> > function is being called with different prototypes. We can work around it but a
> > clean solution would be preferred at some point.
> 
> I agree with your criticism, TBH.  Aravindh/Ian, can we rename this
> function ?

I have no objection to some other name.

> > This is now a rather opaque behavior of libxc. As it is not merged
> > into master I guess I will just submit a patch for it..
> 
> Yes.  I think this would be the right approach to addressing both of
> these problems.  (Please send two patches, one for each.)
> 
> Thanks,
> Ian.

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Andrew Cooper]

On 30/06/14 13:44, Ian Campbell wrote:
> On Mon, 2014-06-30 at 13:42 +0100, Ian Jackson wrote:
>> Tamas Lengyel writes ("Re: [Xen-devel] Issues regarding "mem_access: Add helper API to setup ring and enable mem_access""):
>>>     > Now with this function being reintroduced, it becomes more complicated
>>>     > to determine which version of the mem_access API does Xen actually
>>>     > provide. A #define indicating mem_access API version would nicely
>>>     > overcome this issue, or  naming xc_mem_event_enable something else.
>>>
>>>     Doesn't configure support checking for functions with a given prototype?
>>>
>>> It does but in a very hacky way, essentially trying to compile code where the
>>> function is being called with different prototypes. We can work around it but a
>>> clean solution would be preferred at some point.
>> I agree with your criticism, TBH.  Aravindh/Ian, can we rename this
>> function ?
> I have no objection to some other name.
>
>>> This is now a rather opaque behavior of libxc. As it is not merged
>>> into master I guess I will just submit a patch for it..
>> Yes.  I think this would be the right approach to addressing both of
>> these problems.  (Please send two patches, one for each.)
>>
>> Thanks,
>> Ian.

There is a bug in Xen.  domctl pause and unpause hypercalls are not
properly refcounted.  Patch on its way.

~Andrew

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Ian Campbell]

On Mon, 2014-06-30 at 14:07 +0100, Andrew Cooper wrote:
> On 30/06/14 13:44, Ian Campbell wrote:
> > On Mon, 2014-06-30 at 13:42 +0100, Ian Jackson wrote:
> >> Tamas Lengyel writes ("Re: [Xen-devel] Issues regarding "mem_access: Add helper API to setup ring and enable mem_access""):
> >>>     > Now with this function being reintroduced, it becomes more complicated
> >>>     > to determine which version of the mem_access API does Xen actually
> >>>     > provide. A #define indicating mem_access API version would nicely
> >>>     > overcome this issue, or  naming xc_mem_event_enable something else.
> >>>
> >>>     Doesn't configure support checking for functions with a given prototype?
> >>>
> >>> It does but in a very hacky way, essentially trying to compile code where the
> >>> function is being called with different prototypes. We can work around it but a
> >>> clean solution would be preferred at some point.
> >> I agree with your criticism, TBH.  Aravindh/Ian, can we rename this
> >> function ?
> > I have no objection to some other name.
> >
> >>> This is now a rather opaque behavior of libxc. As it is not merged
> >>> into master I guess I will just submit a patch for it..
> >> Yes.  I think this would be the right approach to addressing both of
> >> these problems.  (Please send two patches, one for each.)
> >>
> >> Thanks,
> >> Ian.
> 
> There is a bug in Xen.  domctl pause and unpause hypercalls are not
> properly refcounted.  Patch on its way.

I had convinced myself by reading that they were, but Tamas'
experimental evidence obviously counters that ;-)

Ian.

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Andrew Cooper]

On 30/06/14 14:09, Ian Campbell wrote:
> On Mon, 2014-06-30 at 14:07 +0100, Andrew Cooper wrote:
>> On 30/06/14 13:44, Ian Campbell wrote:
>>> On Mon, 2014-06-30 at 13:42 +0100, Ian Jackson wrote:
>>>> Tamas Lengyel writes ("Re: [Xen-devel] Issues regarding "mem_access: Add helper API to setup ring and enable mem_access""):
>>>>>     > Now with this function being reintroduced, it becomes more complicated
>>>>>     > to determine which version of the mem_access API does Xen actually
>>>>>     > provide. A #define indicating mem_access API version would nicely
>>>>>     > overcome this issue, or  naming xc_mem_event_enable something else.
>>>>>
>>>>>     Doesn't configure support checking for functions with a given prototype?
>>>>>
>>>>> It does but in a very hacky way, essentially trying to compile code where the
>>>>> function is being called with different prototypes. We can work around it but a
>>>>> clean solution would be preferred at some point.
>>>> I agree with your criticism, TBH.  Aravindh/Ian, can we rename this
>>>> function ?
>>> I have no objection to some other name.
>>>
>>>>> This is now a rather opaque behavior of libxc. As it is not merged
>>>>> into master I guess I will just submit a patch for it..
>>>> Yes.  I think this would be the right approach to addressing both of
>>>> these problems.  (Please send two patches, one for each.)
>>>>
>>>> Thanks,
>>>> Ian.
>> There is a bug in Xen.  domctl pause and unpause hypercalls are not
>> properly refcounted.  Patch on its way.
> I had convinced myself by reading that they were, but Tamas'
> experimental evidence obviously counters that ;-)
>
> Ian.
>

The "d->is_paused_by_controller" is a boolean, not an atomic count.

The second pause doesn't add an extra pause ref to the domain, and the
first unpause resumes the domain.

~Andrew

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Jan Beulich]

>>> On 30.06.14 at 15:10, <andrew.cooper3@citrix.com> wrote:
> On 30/06/14 14:09, Ian Campbell wrote:
>> On Mon, 2014-06-30 at 14:07 +0100, Andrew Cooper wrote:
>>> On 30/06/14 13:44, Ian Campbell wrote:
>>>> On Mon, 2014-06-30 at 13:42 +0100, Ian Jackson wrote:
>>>>> Tamas Lengyel writes ("Re: [Xen-devel] Issues regarding "mem_access: Add 
> helper API to setup ring and enable mem_access""):
>>>>>>     > Now with this function being reintroduced, it becomes more complicated
>>>>>>     > to determine which version of the mem_access API does Xen actually
>>>>>>     > provide. A #define indicating mem_access API version would nicely
>>>>>>     > overcome this issue, or  naming xc_mem_event_enable something else.
>>>>>>
>>>>>>     Doesn't configure support checking for functions with a given prototype?
>>>>>>
>>>>>> It does but in a very hacky way, essentially trying to compile code where 
> the
>>>>>> function is being called with different prototypes. We can work around it 
> but a
>>>>>> clean solution would be preferred at some point.
>>>>> I agree with your criticism, TBH.  Aravindh/Ian, can we rename this
>>>>> function ?
>>>> I have no objection to some other name.
>>>>
>>>>>> This is now a rather opaque behavior of libxc. As it is not merged
>>>>>> into master I guess I will just submit a patch for it..
>>>>> Yes.  I think this would be the right approach to addressing both of
>>>>> these problems.  (Please send two patches, one for each.)
>>>>>
>>>>> Thanks,
>>>>> Ian.
>>> There is a bug in Xen.  domctl pause and unpause hypercalls are not
>>> properly refcounted.  Patch on its way.
>> I had convinced myself by reading that they were, but Tamas'
>> experimental evidence obviously counters that ;-)
> 
> The "d->is_paused_by_controller" is a boolean, not an atomic count.
> 
> The second pause doesn't add an extra pause ref to the domain, and the
> first unpause resumes the domain.

Which, as you see, is (in the hypervisor) intended behavior. If the
tool stack wants this to be ref-counted, it needs to do so by itself.

Jan

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Andrew Cooper]

On 30/06/14 15:12, Jan Beulich wrote:
>>>> On 30.06.14 at 15:10, <andrew.cooper3@citrix.com> wrote:
>> On 30/06/14 14:09, Ian Campbell wrote:
>>> On Mon, 2014-06-30 at 14:07 +0100, Andrew Cooper wrote:
>>>> On 30/06/14 13:44, Ian Campbell wrote:
>>>>> On Mon, 2014-06-30 at 13:42 +0100, Ian Jackson wrote:
>>>>>> Tamas Lengyel writes ("Re: [Xen-devel] Issues regarding "mem_access: Add 
>> helper API to setup ring and enable mem_access""):
>>>>>>>     > Now with this function being reintroduced, it becomes more complicated
>>>>>>>     > to determine which version of the mem_access API does Xen actually
>>>>>>>     > provide. A #define indicating mem_access API version would nicely
>>>>>>>     > overcome this issue, or  naming xc_mem_event_enable something else.
>>>>>>>
>>>>>>>     Doesn't configure support checking for functions with a given prototype?
>>>>>>>
>>>>>>> It does but in a very hacky way, essentially trying to compile code where 
>> the
>>>>>>> function is being called with different prototypes. We can work around it 
>> but a
>>>>>>> clean solution would be preferred at some point.
>>>>>> I agree with your criticism, TBH.  Aravindh/Ian, can we rename this
>>>>>> function ?
>>>>> I have no objection to some other name.
>>>>>
>>>>>>> This is now a rather opaque behavior of libxc. As it is not merged
>>>>>>> into master I guess I will just submit a patch for it..
>>>>>> Yes.  I think this would be the right approach to addressing both of
>>>>>> these problems.  (Please send two patches, one for each.)
>>>>>>
>>>>>> Thanks,
>>>>>> Ian.
>>>> There is a bug in Xen.  domctl pause and unpause hypercalls are not
>>>> properly refcounted.  Patch on its way.
>>> I had convinced myself by reading that they were, but Tamas'
>>> experimental evidence obviously counters that ;-)
>> The "d->is_paused_by_controller" is a boolean, not an atomic count.
>>
>> The second pause doesn't add an extra pause ref to the domain, and the
>> first unpause resumes the domain.
> Which, as you see, is (in the hypervisor) intended behavior. If the
> tool stack wants this to be ref-counted, it needs to do so by itself.
>
> Jan

I can see that it is not the current behaviour, but no indication as to
why it is intended not to be properly recounted.

With memaccess setup like this it is perfectly possible to have multiple
toolstack userspace processes needing to pause/unpause the domain over
small periods to ensure safety.


Refcounting in Xen is the only plausible way of maintaining consistency;
there is no safe way to refcount this in the toolstack.  The patch from
Tanas introduces a TOCTOU race which won't solve the problem in the
general case.

~Andrew

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Tamas Lengyel]

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

On Mon, Jun 30, 2014 at 2:44 PM, Ian Campbell <Ian.Campbell@citrix.com>
wrote:

> On Mon, 2014-06-30 at 13:42 +0100, Ian Jackson wrote:
> > Tamas Lengyel writes ("Re: [Xen-devel] Issues regarding "mem_access: Add
> helper API to setup ring and enable mem_access""):
> > >
> > >     > Now with this function being reintroduced, it becomes more
> complicated
> > >     > to determine which version of the mem_access API does Xen
> actually
> > >     > provide. A #define indicating mem_access API version would nicely
> > >     > overcome this issue, or  naming xc_mem_event_enable something
> else.
> > >
> > >     Doesn't configure support checking for functions with a given
> prototype?
> > >
> > > It does but in a very hacky way, essentially trying to compile code
> where the
> > > function is being called with different prototypes. We can work around
> it but a
> > > clean solution would be preferred at some point.
> >
> > I agree with your criticism, TBH.  Aravindh/Ian, can we rename this
> > function ?
>
> I have no objection to some other name.
>

A question regarding renaming the xc_mem_event_enable function. The public
xenctrl.h clearly says

/**
 * mem_event operations. Internal use only.
 */

There are only three of these, xc_mem_event_control, xc_mem_event_memop and
xc_mem_event_enable. Wouldn't it make more sense to just exclude these
functions from the public header and move them to xc_private.h? Why have
internal functions in the public header?

Tamas

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

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

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

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Aravindh Puthiyaparambil (aravindp)]

>	> I agree with your criticism, TBH.  Aravindh/Ian, can we rename this
>	> function ?
>
>	I have no objection to some other name.

How about xc_enable_mem_event()? If that is fine, I will submit a patch.

>A question regarding renaming the xc_mem_event_enable function. The
>public xenctrl.h clearly says
>
>/**
> * mem_event operations. Internal use only.
> */
>
>There are only three of these, xc_mem_event_control,
>xc_mem_event_memop and xc_mem_event_enable. Wouldn't it make more
>sense to just exclude these functions from the public header and move them
>to xc_private.h? Why have internal functions in the public header?

I too think these can be moved to the xc_private.h. IanC / IanJ, what are your thoughts on doing this?

Thanks,
Aravindh

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Aravindh Puthiyaparambil (aravindp)]

>>	> I agree with your criticism, TBH.  Aravindh/Ian, can we rename this
>>	> function ?
>>
>>	I have no objection to some other name.
>
>How about xc_enable_mem_event()? If that is fine, I will submit a patch.
>
>>A question regarding renaming the xc_mem_event_enable function. The
>>public xenctrl.h clearly says
>>
>>/**
>> * mem_event operations. Internal use only.
>> */
>>
>>There are only three of these, xc_mem_event_control,
>>xc_mem_event_memop and xc_mem_event_enable. Wouldn't it make
>more
>>sense to just exclude these functions from the public header and move
>them
>>to xc_private.h? Why have internal functions in the public header?
>
>I too think these can be moved to the xc_private.h. IanC / IanJ, what are your
>thoughts on doing this?

Forgot to add that if this move is done then I am assuming the rename is not required. Correct?

Thanks,
Aravindh

Re: Issues regarding "mem_access: Add helper API to setup ring and enable mem_access"

[Tamas Lengyel]

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

On Tue, Jul 1, 2014 at 12:31 AM, Aravindh Puthiyaparambil (aravindp) <
aravindp@cisco.com> wrote:

> >>      > I agree with your criticism, TBH.  Aravindh/Ian, can we rename
> this
> >>      > function ?
> >>
> >>      I have no objection to some other name.
> >
> >How about xc_enable_mem_event()? If that is fine, I will submit a patch.
> >
> >>A question regarding renaming the xc_mem_event_enable function. The
> >>public xenctrl.h clearly says
> >>
> >>/**
> >> * mem_event operations. Internal use only.
> >> */
> >>
> >>There are only three of these, xc_mem_event_control,
> >>xc_mem_event_memop and xc_mem_event_enable. Wouldn't it make
> >more
> >>sense to just exclude these functions from the public header and move
> >them
> >>to xc_private.h? Why have internal functions in the public header?
> >
> >I too think these can be moved to the xc_private.h. IanC / IanJ, what are
> your
> >thoughts on doing this?
>
> Forgot to add that if this move is done then I am assuming the rename is
> not required. Correct?
>
> Thanks,
> Aravindh
>

With relocating these functions to xc_private.h the issue I had would be
solved so no renaming would be required. My patch for doing that is on its
way momentarily.

Tamas

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

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

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