Re: [PATCH v4 3/8] ioreq-server: create basic ioreq server abstraction.

[George Dunlap <george.dunlap@eu.citrix.com>]

On 04/03/2014 04:43 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: dunlapg@gmail.com [mailto:dunlapg@gmail.com] On Behalf Of
>> George Dunlap
>> Sent: 03 April 2014 15:50
>> To: Paul Durrant
>> Cc: xen-devel@lists.xen.org; Keir (Xen.org); Jan Beulich
>> Subject: Re: [Xen-devel] [PATCH v4 3/8] ioreq-server: create basic ioreq
>> server abstraction.
>>
>> On Wed, Apr 2, 2014 at 4:11 PM, Paul Durrant <paul.durrant@citrix.com>
>> wrote:
>>> Collect together data structures concerning device emulation together into
>>> a new struct hvm_ioreq_server.
>>>
>>> Code that deals with the shared and buffered ioreq pages is extracted from
>>> functions such as hvm_domain_initialise, hvm_vcpu_initialise and
>> do_hvm_op
>>> and consolidated into a set of hvm_ioreq_server manipulation functions.
>> The
>>> lock in the hvm_ioreq_page served two different purposes and has been
>>> replaced by separate locks in the hvm_ioreq_server.
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>> Cc: Keir Fraser <keir@xen.org>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> ---
>>>   xen/arch/x86/hvm/hvm.c           |  406 ++++++++++++++++++++++++++----
>> --------
>>>   xen/include/asm-x86/hvm/domain.h |   35 +++-
>>>   xen/include/asm-x86/hvm/vcpu.h   |   12 +-
>>>   3 files changed, 322 insertions(+), 131 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index 573f845..5f131c4 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -352,39 +352,49 @@ void hvm_migrate_pirqs(struct vcpu *v)
>>>       spin_unlock(&d->event_lock);
>>>   }
>>>
>>> -static ioreq_t *get_ioreq(struct vcpu *v)
>>> +static ioreq_t *get_ioreq(struct hvm_ioreq_server *s, struct vcpu *v)
>>>   {
>>> -    struct domain *d = v->domain;
>>> -    shared_iopage_t *p = d->arch.hvm_domain.ioreq.va;
>>> +    shared_iopage_t *p = s->ioreq.va;
>>>
>>> -    ASSERT((v == current) || spin_is_locked(&d-
>>> arch.hvm_domain.ioreq.lock));
>>> +    /*
>>> +     * Manipulation of the shared ioreq structure (to update the event
>>> +     * channel) is protected by a domain_pause(). So this function should
>>> +     * only ever be executed for the current vcpu or one that is paused.
>>> +     */
>> What on earth is "manipulation of the shared ioreq structure is
>> protected by domain_pause()" supposed to mean?  Do you mean that the
>> only time there may be a race is between something in the emulation
>> code writing to it, and something in the resume path reading it?  That
>> there are never any other races to access the structure?  And that
>> since the resume path won't be taken when the domain is paused, there
>> can be no races, and therefore we do not need to be locked?
>>
> The sentiment I'm trying to express is that the shared structure can never be in use in the emulation path whilst it is being modified as the code that modifies always pauses the domain before doing so, so the assertion is that either v == current (in which case the domain is clearly not paused and we're in the emulation path) or !vcpu_runnable(v) (in which case the domain is paused and we're making a change).

Sure, but is there a risk of two different invocations of the "code that 
modifies" happening at the same time?  (Perhaps, for instance, because 
of a buggy toolstack that makes two calls on the same ioreq server?)

  -George