Re: [PATCH V2] xen: Fix BUFIOREQ evtchn init for a stubdom.

[Keir Fraser <keir.xen@gmail.com>]

On 29/06/2012 09:50, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 26.06.12 at 17:21, Anthony PERARD <anthony.perard@citrix.com> wrote:
>> @@ -3777,17 +3792,21 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE(void) arg)
>>                  iorp = &d->arch.hvm_domain.ioreq;
>>                  for_each_vcpu ( d, v )
>>                  {
>> -                    int old_port, new_port;
>> -                    new_port = alloc_unbound_xen_event_channel(
>> -                        v, a.value, NULL);
>> -                    if ( new_port < 0 )
>> -                    {
>> -                        rc = new_port;
>> +                    rc = hvm_replace_event_channel(v, a.value,
>> +               
>> &v->arch.hvm_vcpu.xen_port);
>> +                    if ( rc )
>>                          break;
>> +
>> +                    if ( v->vcpu_id == 0 )
> 
> Don't you also have to check params[HVM_PARAM_BUFIOREQ_EVTCHN]
> is valid (as otherwise free_xen_event_channel() will BUG_ON() on
> it)?

No, params[HVM_PARAM_BUFIOREQ_EVTCHN] is guaranteed valid.

>> +                    {
>> +                        spin_lock(&iorp->lock);
>> +                        rc = hvm_replace_event_channel(v, a.value,
>> +               
>> (int*)&v->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN]);
>> +                        spin_unlock(&iorp->lock);
>> +                        if ( rc )
>> +                            break;
>>                      }
> 
> My first preference would be for this to be moved out of the
> loop. If it has to remain in the loop for some reason, then the
> next best option would be to move this into the iorp->lock
> protected region immediately below.

Agree on moving it out of the loop. But why would you want it protected by
iorp->lock?

 -- Keir

> Jan
> 
>> -                    /* xchg() ensures that only we free_xen_event_channel()
>> */
>> -                    old_port = xchg(&v->arch.hvm_vcpu.xen_port, new_port);
>> -                    free_xen_event_channel(v, old_port);
>> +
>>                      spin_lock(&iorp->lock);
>>                      if ( iorp->va != NULL )
>>                          get_ioreq(v)->vp_eport = v->arch.hvm_vcpu.xen_port;
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel