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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 08 May 2014 15:47
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH v6 3/9] ioreq-server: create basic ioreq server
> abstraction.
> 
> >>> On 08.05.14 at 15:23, <paul.durrant@xxxxxxxxxx> wrote:
> > @@ -446,16 +443,11 @@ void destroy_ring_for_helper(
> >      }
> >  }
> >
> > -static void hvm_unmap_ioreq_page(
> > -    struct domain *d, struct hvm_ioreq_page *iorp)
> > +static void hvm_unmap_ioreq_page(struct hvm_ioreq_server *s, bool_t
> buf)
> >  {
> > -    spin_lock(&iorp->lock);
> > -
> > -    ASSERT(d->is_dying);
> > +    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> >
> >      destroy_ring_for_helper(&iorp->va, iorp->page);
> > -
> > -    spin_unlock(&iorp->lock);
> >  }
> 
> Are you sure you can do without any locking (i.e. neither here nor in
> the caller)? For the respective map function, afaict the locking simply
> got moved to the caller. Remember that you need to also protect
> yourself against hostile control domains...
> 

I think it's safe as the unmap is only done during domain destruction. Could 
this race? I would have though other stuff would break.

> > +static int hvm_set_ioreq_pfn(struct domain *d, bool_t buf,
> > +                             unsigned long pfn)
> > +{
> > +    struct hvm_ioreq_server *s = d->arch.hvm_domain.ioreq_server;
> > +    int rc;
> > +
> > +    spin_lock(&s->lock);
> > +
> > +    rc = hvm_map_ioreq_page(s, buf, pfn);
> > +    if ( rc )
> > +        goto fail;
> > +
> > +    if (!buf)
> 
> Coding style still...
> 

Sorry, I'll fix. Without a style checker it's really hard to notice when 
reviewing your own patches.

  Paul

> Jan


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