Xen devel - Jul 2012 - [PATCH V3] 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 v3:
  For the bufioreq replacement:
  - the code is now out of the vcpu loop
  - the code does not take a lock anymore
  - rename int *port to int *p_port
Change v2:
  - an helper
  - the replacement of the buferioreq evtchn is inside iorp->lock now.

 xen/arch/x86/hvm/hvm.c |   32 ++++++++++++++++++++++----------
 1 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e0d495d..c2dfa73 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 *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(p_port, new_port);
+    free_xen_event_channel(v, old_port);
+    return 0;
+}
+
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void) arg)
 
 {
@@ -3775,19 +3790,16 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void)
arg)
                 rc = 0;
                 domain_pause(d); /* safe to change per-vcpu xen_port */
                 iorp = &d->arch.hvm_domain.ioreq;
+                if (d->vcpu[0])
+                    hvm_replace_event_channel(d->vcpu[0], a.value,
+                       
(int*)&d->vcpu[0]->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN]);
                 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;
-                    }
-                    /* 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 02.07.12 at 17:41, Anthony PERARD
<anthony.perard@citrix.com> 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>
> ---
> Change v3:
>   For the bufioreq replacement:
>   - the code is now out of the vcpu loop
>   - the code does not take a lock anymore
>   - rename int *port to int *p_port
> Change v2:
>   - an helper
>   - the replacement of the buferioreq evtchn is inside iorp->lock now.
> 
>  xen/arch/x86/hvm/hvm.c |   32 ++++++++++++++++++++++----------
>  1 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e0d495d..c2dfa73 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 *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(p_port, new_port);
> +    free_xen_event_channel(v, old_port);
> +    return 0;
> +}
> +
>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void) arg)
>  
>  {
> @@ -3775,19 +3790,16 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE(void) arg)
>                  rc = 0;
>                  domain_pause(d); /* safe to change per-vcpu xen_port */
>                  iorp = &d->arch.hvm_domain.ioreq;
> +                if (d->vcpu[0])
> +                    hvm_replace_event_channel(d->vcpu[0], a.value,
> +                       
(int*)&d->vcpu[0]->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN]);
Did I overlook this in v2? You clearly need to handle the error
case here (it is being handled, albeit - but that''s not your
patch''s
fault - only in a rudimentary way, inside the loop).

Jan
>                  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;
> -                    }
> -                    /* 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 02.07.12 at 17:58, "Jan Beulich"
<JBeulich@suse.com> wrote:
>> @@ -3775,19 +3790,16 @@ long do_hvm_op(unsigned long op, 
>> XEN_GUEST_HANDLE(void) arg)
>>                  rc = 0;
>>                  domain_pause(d); /* safe to change per-vcpu xen_port
*/
>>                  iorp = &d->arch.hvm_domain.ioreq;
>> +                if (d->vcpu[0])
>> +                    hvm_replace_event_channel(d->vcpu[0], a.value,
>> +                       
(int*)&d->vcpu[0]->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN]);
> 
> Did I overlook this in v2? You clearly need to handle the error
> case here (it is being handled, albeit - but that''s not your
patch''s
> fault - only in a rudimentary way, inside the loop).
Probably it''ll be easiest if I - or Keir if he''s faster - add
this while
committing, to save you from posting another version.

Jan

On 02/07/12 16:58, Jan Beulich wrote:
>>>> On 02.07.12 at 17:41, Anthony PERARD
<anthony.perard@citrix.com> 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>
>> ---
>> Change v3:
>>    For the bufioreq replacement:
>>    - the code is now out of the vcpu loop
>>    - the code does not take a lock anymore
>>    - rename int *port to int *p_port
>> Change v2:
>>    - an helper
>>    - the replacement of the buferioreq evtchn is inside iorp->lock
now.
>>
>>   xen/arch/x86/hvm/hvm.c |   32 ++++++++++++++++++++++----------
>>   1 files changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index e0d495d..c2dfa73 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 *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(p_port, new_port);
>> +    free_xen_event_channel(v, old_port);
>> +    return 0;
>> +}
>> +
>>   long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void) arg)
>>
>>   {
>> @@ -3775,19 +3790,16 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE(void) arg)
>>                   rc = 0;
>>                   domain_pause(d); /* safe to change per-vcpu xen_port
*/
>>                   iorp = &d->arch.hvm_domain.ioreq;
>> +                if (d->vcpu[0])
>> +                    hvm_replace_event_channel(d->vcpu[0], a.value,
>> +                       
(int*)&d->vcpu[0]->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN]);
>
> Did I overlook this in v2? You clearly need to handle the error
> case here (it is being handled, albeit - but that''s not your
patch''s
> fault - only in a rudimentary way, inside the loop).
Well, if there is an error with the replace, it just break the for_each 
loop and do domain_unpause. So my guest was to leave it fail a second 
time :).

But here, how should I handle the error case? If there is an error, 
should I not go into the for_each and go strait to domain_unpause (with 
the rc value set)?
>>                   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;
>> -                    }
>> -                    /* 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
>
>
>

-- 
Anthony PERARD

On 02/07/2012 17:01, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 02.07.12 at 17:58, "Jan Beulich"
<JBeulich@suse.com> wrote:
>>> @@ -3775,19 +3790,16 @@ long do_hvm_op(unsigned long op,
>>> XEN_GUEST_HANDLE(void) arg)
>>>                  rc = 0;
>>>                  domain_pause(d); /* safe to change per-vcpu
xen_port */
>>>                  iorp = &d->arch.hvm_domain.ioreq;
>>> +                if (d->vcpu[0])
>>> +                    hvm_replace_event_channel(d->vcpu[0],
a.value,
>>> +              
>>>
(int*)&d->vcpu[0]->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN]
>>> );
>> 
>> Did I overlook this in v2? You clearly need to handle the error
>> case here (it is being handled, albeit - but that''s not your
patch''s
>> fault - only in a rudimentary way, inside the loop).
> 
> Probably it''ll be easiest if I - or Keir if he''s faster -
add this while
> committing, to save you from posting another version.
I''m travelling so please just go ahead, Jan.

 K.
> Jan
>