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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 04/03/2014 04:43 PM, Paul Durrant wrote:
> > -----Original Message-----
> > From: dunlapg@xxxxxxxxx [mailto:dunlapg@xxxxxxxxx] On Behalf Of
> > George Dunlap
> > Sent: 03 April 2014 15:50
> > To: Paul Durrant
> > Cc: xen-devel@xxxxxxxxxxxxx; 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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > > Cc: Keir Fraser <keir@xxxxxxx>
> > > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > > ---
> > >   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

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