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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.06.12 at 12:10, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
> On 29/06/2012 09:50, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>>>>> On 26.06.12 at 17:21, Anthony PERARD <anthony.perard@xxxxxxxxxx> 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.

Oh, I see, this is being set by hvm_vcpu_initialize(), and read-only
to any external entity.

>>> +                    {
>>> +                        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?

That's a question to Anthony - I just saw that the same lock is
being used here and a few lines down.

Jan


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