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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----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).

  Paul

>  -George

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