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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant
> Sent: 06 May 2014 14:40
> To: 'Jan Beulich'
> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org)
> Subject: RE: [PATCH v5 4/9] ioreq-server: create basic ioreq server
> abstraction.
> 
> > -----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.

... and of course, I mean longs there and not unsigned longs!

> 
>   Paul
> 
> > Jan


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