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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-06-29 at 11:10 +0100, Keir Fraser 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.
> 
> >> +                    {
> >> +                        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?

I suggested it because the user of the field locks with that lock.

I think that even with the xchg there is still scope for the old event
channel to be in use in hvm_buffered_io_send after it has been replaced.
The xchg just protects against concurrent freeing.

Ian.

> 
>  -- 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@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel
> 
> 



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