Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/07/14 05:29, Xu, Dongxiao wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, July 04, 2014 6:44 PM
>> To: Xu, Dongxiao
>> Cc: andrew.cooper3@citrix.com; Ian.Campbell@citrix.com;
>> George.Dunlap@eu.citrix.com; Ian.Jackson@eu.citrix.com;
>> stefano.stabellini@eu.citrix.com; xen-devel@lists.xen.org;
>> konrad.wilk@oracle.com; dgdegra@tycho.nsa.gov; keir@xen.org
>> Subject: Re: [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access
>> hypercall
>>
>>>>> On 04.07.14 at 10:34, <dongxiao.xu@intel.com> wrote:
>>> Add a generic resource access hypercall for tool stack or other
>>> components, e.g., accessing MSR, port I/O, etc.
>> Sigh - you're still allocating an unbounded amount of memory for
>> passing around the input arguments, despite it being possible (and
>> having been suggested) to read these from the original buffer on
>> each iteration. You're still not properly checking for preemption
>> between iterations. And you're still not making use of
>> continue_hypercall_on_cpu(). Plus you now silently ignore the
>> upper 32-bits of the passing in "idx" value as well as not
>> understood XEN_RESOURCE_OP_* values.
> continue_hypercall_on_cpu() is asynchronized, which requires the "data" field always points to the right place before the hypercall returns.
> However in our function, we have a "for" loop to cover multiple operations, so the "data" field will be modified in each iteration, which cannot meet the continue_hypercall_on_cpu() requirements...

This is because you are still copying all resource data at once from the
hypercall.

As Jan points out, this is an unbounded allocation in Xen which must be
addresses.  If instead you were to copy each element one at a time, you
would avoid this allocation entirely and be able to correctly use
continue_hypercall_on_cpu().


>
> For the preemption check, what about the following? Here the preemption is checked within each resource_access_one() function.

None of this preemption works.

In the case a hypercall gets preempted, you need to increment the guest
handle along to the next element to process, and decrement the count by
the number of elements processed in *the guest context*.

That way, when the hypercall continues in Xen, it shall pick up with the
next action to perform rather than restarting from the beginning.

~Andrew