Xen devel - Jun 2012 - [PATCH] 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 fix the initialization of QEMU inside the stubdomain.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

---
 xen/arch/x86/hvm/hvm.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e0d495d..9ff1ded 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3788,6 +3788,22 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void)
arg)
                     /* 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);
+
+                    if ( v->vcpu_id == 0 )
+                    {
+                        new_port = alloc_unbound_xen_event_channel(
+                            v, a.value, NULL);
+                        if ( new_port < 0 )
+                        {
+                            rc = new_port;
+                            break;
+                        }
+                        old_port = xchg(
+                           
&v->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN],
+                            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 11:45 +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 fix the initialization of QEMU inside the stubdomain.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
This fixes stub domains for me, thanks.

The user of this field (hvm_buffered_io_send) takes iorp->lock so we
probably want to put the update inside that too, just like for the
non-buffered case?

It would be nice to extract the alloc&replace functionality into a
helper.

Ian.

> ---
>  xen/arch/x86/hvm/hvm.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e0d495d..9ff1ded 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3788,6 +3788,22 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE(void) arg)
>                      /* 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);
> +
> +                    if ( v->vcpu_id == 0 )
> +                    {
> +                        new_port = alloc_unbound_xen_event_channel(
> +                            v, a.value, NULL);
> +                        if ( new_port < 0 )
> +                        {
> +                            rc = new_port;
> +                            break;
> +                        }
> +                        old_port = xchg(
> +                           
&v->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN],
> +                            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 Tue, Jun 26, 2012 at 11:54 AM, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Tue, 2012-06-26 at 11:45 +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 fix the initialization of QEMU inside the stubdomain.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>
> This fixes stub domains for me, thanks.
>
> The user of this field (hvm_buffered_io_send) takes iorp->lock so we
> probably want to put the update inside that too, just like for the
> non-buffered case?
Will do.
> It would be nice to extract the alloc&replace functionality into a
> helper.
I tried but it does not work well.
xen_port is an it (non-buffer case) and params[bufioreq...] is an
uint64_t. Both should be argument of the helper, as a pointer to be
pass to xchg().

The prototype of the not working helper:
static int hvm_replace_event_channel(struct vcpu *v, uint64_t
remote_domid, int *port)

So unless there is another way to create this helper, there won''t be
any.

-- 
Anthony PERARD

On Tue, 2012-06-26 at 15:13 +0100, Anthony PERARD wrote:
> On Tue, Jun 26, 2012 at 11:54 AM, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
> > On Tue, 2012-06-26 at 11:45 +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 fix the initialization of QEMU inside the stubdomain.
> >>
> >> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> >
> > This fixes stub domains for me, thanks.
> >
> > The user of this field (hvm_buffered_io_send) takes iorp->lock so
we
> > probably want to put the update inside that too, just like for the
> > non-buffered case?
> 
> Will do.
> 
> > It would be nice to extract the alloc&replace functionality into a
> > helper.
> 
> I tried but it does not work well.
> xen_port is an it (non-buffer case) and params[bufioreq...] is an
> uint64_t. Both should be argument of the helper, as a pointer to be
> pass to xchg().
> 
> The prototype of the not working helper:
> static int hvm_replace_event_channel(struct vcpu *v, uint64_t
> remote_domid, int *port)
> 
> So unless there is another way to create this helper, there won''t
be any.
Although params[...] are generally uint64_t the params[BUFIOREQ_EVTCHN]
is really an int (or more properly an evtchn_port_t, although this is
only used in include/public). Perhaps a cast is appropriate in this
case?

Ian.

On Tue, Jun 26, 2012 at 3:21 PM, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Tue, 2012-06-26 at 15:13 +0100, Anthony PERARD wrote:
>> On Tue, Jun 26, 2012 at 11:54 AM, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
>> > On Tue, 2012-06-26 at 11:45 +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 fix the initialization of QEMU inside the stubdomain.
>> >>
>> >> Signed-off-by: Anthony PERARD
<anthony.perard@citrix.com>
>> >
>> > This fixes stub domains for me, thanks.
>> >
>> > The user of this field (hvm_buffered_io_send) takes iorp->lock
so we
>> > probably want to put the update inside that too, just like for the
>> > non-buffered case?
>>
>> Will do.
>>
>> > It would be nice to extract the alloc&replace functionality
into a
>> > helper.
>>
>> I tried but it does not work well.
>> xen_port is an it (non-buffer case) and params[bufioreq...] is an
>> uint64_t. Both should be argument of the helper, as a pointer to be
>> pass to xchg().
>>
>> The prototype of the not working helper:
>> static int hvm_replace_event_channel(struct vcpu *v, uint64_t
>> remote_domid, int *port)
>>
>> So unless there is another way to create this helper, there
won''t be any.
>
> Although params[...] are generally uint64_t the params[BUFIOREQ_EVTCHN]
> is really an int (or more properly an evtchn_port_t, although this is
> only used in include/public). Perhaps a cast is appropriate in this
> case?
All right, I will cast.

-- 
Anthony PERARD