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

This is a missing part from the previous patch that add the BUFIOREQ_EVTCHN
parameter. This patch changes the ownership of the buifioreq event channel to
the stubdom (when HVM_PARAM_DM_DOMAIN is set within the stubdom).

This patch introduces an helper to replace a xen port.

This fix the initialization of QEMU inside the stubdomain.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Change:
  - an helper
  - the replacement of the buferioreq evtchn is inside iorp->lock now.

 xen/arch/x86/hvm/hvm.c |   37 ++++++++++++++++++++++++++++---------
 1 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e0d495d..e8e86c0 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3664,6 +3664,21 @@ static int hvmop_flush_tlb_all(void)
     return 0;
 }
 
+static int hvm_replace_event_channel(struct vcpu *v, uint64_t remote_domid,
+                                     int *port)
+{
+    int old_port, new_port;
+
+    new_port = alloc_unbound_xen_event_channel(v, remote_domid, NULL);
+    if ( new_port < 0 )
+        return new_port;
+
+    /* xchg() ensures that only we free_xen_event_channel() */
+    old_port = xchg(port, new_port);
+    free_xen_event_channel(v, old_port);
+    return 0;
+}
+
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void) arg)
 
 {
@@ -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 )
+                    {
+                        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;
                     }
-                    /* 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;
-- 
Anthony PERARD

On Tue, 2012-06-26 at 16:21 +0100, Anthony PERARD wrote:
> This is a missing part from the previous patch that add the BUFIOREQ_EVTCHN
> parameter. This patch changes the ownership of the buifioreq event channel
to
> the stubdom (when HVM_PARAM_DM_DOMAIN is set within the stubdom).
> 
> This patch introduces an helper to replace a xen port.
> 
> This fix the initialization of QEMU inside the stubdomain.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Thought I''d already acked this but apparently not. This fixes stub
domains for me.

Acked-by: Ian Campbell <ian.campbell@citrix.com>
Tested-by: Ian Campbell <ian.campbell@citrix.com>

CCing Hypervisor maintainers...
> ---
> Change:
>   - an helper
>   - the replacement of the buferioreq evtchn is inside iorp->lock now.
> 
>  xen/arch/x86/hvm/hvm.c |   37 ++++++++++++++++++++++++++++---------
>  1 files changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e0d495d..e8e86c0 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3664,6 +3664,21 @@ static int hvmop_flush_tlb_all(void)
>      return 0;
>  }
>  
> +static int hvm_replace_event_channel(struct vcpu *v, uint64_t
remote_domid,
> +                                     int *port)
> +{
> +    int old_port, new_port;
> +
> +    new_port = alloc_unbound_xen_event_channel(v, remote_domid, NULL);
> +    if ( new_port < 0 )
> +        return new_port;
> +
> +    /* xchg() ensures that only we free_xen_event_channel() */
> +    old_port = xchg(port, new_port);
> +    free_xen_event_channel(v, old_port);
> +    return 0;
> +}
> +
>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void) arg)
>  
>  {
> @@ -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 )
> +                    {
> +                        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;
>                      }
> -                    /* 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;

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

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;

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

On 29/06/2012 09:26, "Ian Campbell" <Ian.Campbell@citrix.com>
wrote:
>> +static int hvm_replace_event_channel(struct vcpu *v, uint64_t
remote_domid,
>> +                                     int *port)
int *p_port?...
>> +{
>> +    int old_port, new_port;
>> +
>> +    new_port = alloc_unbound_xen_event_channel(v, remote_domid, NULL);
>> +    if ( new_port < 0 )
>> +        return new_port;
>> +
>> +    /* xchg() ensures that only we free_xen_event_channel() */
>> +    old_port = xchg(port, new_port);
...Would make this line a little bit clearer imo.
>> +    free_xen_event_channel(v, old_port);
>> +    return 0;
>> +}
>> +

On Fri, 2012-06-29 at 11:10 +0100, Keir Fraser wrote:
> 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?
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@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
>

>>> On 29.06.12 at 12:10, Keir Fraser <keir.xen@gmail.com> wrote:
> 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.
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

On 29/06/2012 11:14, "Ian Campbell" <Ian.Campbell@citrix.com>
wrote:
>> 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.
That lock is really protecting access to the shared bufioreq page. The
evtchn notify could equally well be moved outside the 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.
A. This would be equally true for the per-vcpu hvm_vcpu.xen_port; but
B. We avoid such races by domain_pause()ing when changing
HVM_PARAM_DM_DOMAIN; and
C. In practice we only set HVM_PARAM_DM_DOMAIN before the guest starts to
run anyway.

 -- Keir

On Fri, 2012-06-29 at 11:37 +0100, Keir Fraser wrote:
> On 29/06/2012 11:14, "Ian Campbell"
<Ian.Campbell@citrix.com> wrote:
> 
> >> 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.
> 
> That lock is really protecting access to the shared bufioreq page. The
> evtchn notify could equally well be moved outside the lock.
OK.
> > 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.
> 
> A. This would be equally true for the per-vcpu hvm_vcpu.xen_port; but
> B. We avoid such races by domain_pause()ing when changing
> HVM_PARAM_DM_DOMAIN; and
> C. In practice we only set HVM_PARAM_DM_DOMAIN before the guest starts to
> run anyway.
I thought we locked the update of xen_port too -- but actually that''s
only the update of get_ioreq(v)->vp_eport, which is locked against
iorp->va changing.

Ian.

On 29/06/2012 11:25, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> +                    {
>>>> +                        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.
Ah, I see. It is not necessary to take the lock in the above code fragment.

 -- Keir
> Jan
>