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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.05.14 at 15:40, <Paul.Durrant@xxxxxxxxxx> wrote:
>>  -----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.

That's the defacto state, but with moving towards disaggregation
we're not intending to grow the number of things that need fixing
for this purpose. See XSA-77.

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

No matter which unsigned type you pick, it won't help.

Jan


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