From mboxrd@z Thu Jan  1 00:00:00 1970
From: Keir Fraser <keir.xen@gmail.com>
Subject: Re: [PATCH V2] xen: Fix BUFIOREQ evtchn init for a
 stubdom.
Date: Fri, 29 Jun 2012 11:10:48 +0100
Message-ID: <CC133DB8.37278%keir.xen@gmail.com>
References: <4FED8886020000780008CC7B@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <4FED8886020000780008CC7B@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>, Anthony PERARD <anthony.perard@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Ian Campbell <ian.campbell@citrix.com>, Xen Devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

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