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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 06.05.14 at 15:12, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >>> On 01.05.14 at 14:08, <paul.durrant@citrix.com> 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".

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

Jan