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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 06 May 2014 14:24
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org)
> Subject: RE: [PATCH v5 4/9] ioreq-server: create basic ioreq server
> abstraction.
> 
> >>> On 06.05.14 at 15:12, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >>> On 01.05.14 at 14:08, <paul.durrant@xxxxxxxxxx> wrote:
> >> > @@ -426,14 +431,6 @@ void hvm_do_resume(struct vcpu *v)
> >> >      }
> >> >  }
> >> >
> >> > -static void hvm_init_ioreq_page(
> >> > -    struct domain *d, struct hvm_ioreq_page *iorp)
> >> > -{
> >> > -    memset(iorp, 0, sizeof(*iorp));
> >> > -    spin_lock_init(&iorp->lock);
> >> > -    domain_pause(d);
> >>
> >> So where is this ...
> >
> > Nowhere. As I said in the checkin comment, the lock has gone and the
> > domain_pause() and subsequent domain_unpause() were always
> unnecessary
> > AFAICT. I think the intention was that the domain was not unpaused until
> both
> > the IOREQ PFNs were set, but since the PFNs are set in the domain build
> code
> > in the toolstack I can't see why this was needed.
> 
> So with a disaggregated, hostile tool stack this would still be
> unnecessary? It can go away only if the answer to this is "yes".
>

Ok. I'll add some belt and braces code but I was under the impression that a 
domain building domain currently had to be trusted.
 
> >> > +static int hvm_replace_event_channel(struct vcpu *v, domid_t
> >> remote_domid,
> >> > +                                     evtchn_port_t *p_port)
> >> > +{
> >> > +    evtchn_port_t old_port, new_port;
> >> > +
> >> > +    new_port = alloc_unbound_xen_event_channel(v, remote_domid,
> NULL);
> >> > +    if ( new_port < 0 )
> >> > +        return new_port;
> >>
> >> I'm pretty sure I commented on this too in a previous version:
> >> evtchn_port_t is an unsigned type, hence checking it to be negative
> >> is pointless.
> >
> > Yes, but as I'm pretty sure I responded,
> alloc_unbound_xen_event_channel()
> > doesn't return an evtchn_port_t!
> 
> Which doesn't matter here at all: Once you store the function result
> in a variable of type evtchn_port_t, its original signedness is lost.
> 

...which is probably why I had these coded as unsigned longs originally. I'll 
change them back.

  Paul

> Jan


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