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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 08 April 2014 10:47
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org)
> Subject: RE: [PATCH v4 3/8] ioreq-server: create basic ioreq server
> abstraction.
> 
> >>> On 08.04.14 at 11:32, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >>> On 02.04.14 at 17:11, <paul.durrant@xxxxxxxxxx> wrote:
> >> > +
> >> > +    rc = alloc_unbound_xen_event_channel(v, s->domid, NULL);
> >> > +    if ( rc < 0 )
> >> > +        goto fail2;
> >> > +
> >> > +    sv->ioreq_evtchn = rc;
> >> > +
> >> > +    if ( v->vcpu_id == 0 )
> >> > +    {
> >>
> >> I generally dislike needless dependencies on vCPU 0 being the first
> >> one to make it into any specific function. Can't you check emptiness
> >> of s->ioreq_vcpu_list instead?
> >>
> >
> > That will need a bit more code. I could create the buffered channel on first
> > cpu addition but, I'd then need to track which cpu that  was and re-plumb
> the
> > event channel if that cpu disappears. Also, the default server has always
> > bound the buffered channel to cpu 0 so would I not risk a compatibility
> issue
> > by changing this?
> 
> Hmm, good point. Albeit I still wonder what would happen if vCPU 0
> went away.
> 
> But yes, considering that this is code effectively getting moved here
> (i.e. having special cased vCPU 0 already before), I guess I withdraw
> my change request for now.
> 
> >> > +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;
> >> > +    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> >> > +    int rc;
> >> > +
> >> > +    spin_lock(&s->lock);
> >> > +
> >> > +    rc = hvm_map_ioreq_page(d, iorp, pfn);
> >>
> >> While I realize that at this point there's only one server per domain,
> >> the locking and operation still look to be out of sync at the first
> >> glance: As this can't remain that way anyway till the end of the series,
> >> can't this be brought back in sync here right away (whether that's by
> >> passing s instead of d into the function or acquiring the lock only after
> >> the call I don't know offhand)?
> >
> > I don't follow what you mean by 'out of sync' here: The lock is taken, the
> > pfn is mapped, the lock is dropped. What am I missing?
> 
> You lock "s" but operate on "d".

Ah, I see what you mean. I'll mod. the function to take s rather than d then.

> 
> >> > @@ -1339,30 +1549,10 @@ int hvm_vcpu_initialise(struct vcpu *v)
> >> >           && (rc = nestedhvm_vcpu_initialise(v)) < 0 ) /* teardown:
> >> > nestedhvm_vcpu_destroy */
> >> >          goto fail5;
> >> >
> >> > -    dm_domid = d-
> >> >arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
> >> > -
> >> > -    /* Create ioreq event channel. */
> >> > -    rc = alloc_unbound_xen_event_channel(v, dm_domid, NULL); /*
> >> teardown: none */
> >> > -    if ( rc < 0 )
> >> > +    rc = hvm_ioreq_server_add_vcpu(s, v);
> >> > +    if ( rc != 0 )
> >>
> >> Can this really be > 0 now, and if so is this being handled correctly in
> >> the caller(s)?
> >
> > It's not the same function being called. hvm_ioreq_server_add_vcpu () can
> > only return 0 or a -ve errno.
> 
> Oh, sorry, didn't pay close enough attention.
> 
> > I can test for < 0 but IIRC you objected to doing
> > that in a review of a previous patch if the function never returns > 0. 
> > Would
> > you prefer 'if ( rc )'?
> 
> Yes, "if ( rc )" would seem better, but with the called function changing
> it doesn't really matter all that much.

Ok. I'll check for consistency with what I did elsewhere.

  Paul

> 
> Jan


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