Xen devel - Jan 2013 - [PATCH] arch/x86/hvm: Bind xen-created event channels to building domain

Instead of using a hardcoded domain 0 as the endpoint for the event
channels created in hvm_vcpu_initialise, use the domain ID of the
building domain so that a domain builder in a domain other than dom0 has
the expected access to the event channels.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/hvm/hvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 40c1ab2..682d934 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1071,7 +1071,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
         goto fail3;
 
     /* Create ioreq event channel. */
-    rc = alloc_unbound_xen_event_channel(v, 0, NULL);
+    rc = alloc_unbound_xen_event_channel(v, current->domain->domain_id,
NULL);
     if ( rc < 0 )
         goto fail4;
 
@@ -1081,7 +1081,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
     if ( v->vcpu_id == 0 )
     {
         /* Create bufioreq event channel. */
-        rc = alloc_unbound_xen_event_channel(v, 0, NULL);
+        rc = alloc_unbound_xen_event_channel(v,
current->domain->domain_id, NULL);
         if ( rc < 0 )
             goto fail2;
         v->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN] =
rc;
-- 
1.7.11.7

On 03/01/2013 18:28, "Daniel De Graaf" <dgdegra@tycho.nsa.gov>
wrote:
> Instead of using a hardcoded domain 0 as the endpoint for the event
> channels created in hvm_vcpu_initialise, use the domain ID of the
> building domain so that a domain builder in a domain other than dom0 has
> the expected access to the event channels.
This should be done by setting HVM_PARAM_DM_DOMAIN appropriately after the
guest is created. As it is, HVM_PARAM_DM_DOMAIN can be out of sync with the
event-channel bindings after this patch.

I see this patch is already applied, and I propose to revert it. Is that
okay with you, Jan?

 -- Keir
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 40c1ab2..682d934 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1071,7 +1071,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
>          goto fail3;
>  
>      /* Create ioreq event channel. */
> -    rc = alloc_unbound_xen_event_channel(v, 0, NULL);
> +    rc = alloc_unbound_xen_event_channel(v,
current->domain->domain_id,
> NULL);
>      if ( rc < 0 )
>          goto fail4;
>  
> @@ -1081,7 +1081,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
>      if ( v->vcpu_id == 0 )
>      {
>          /* Create bufioreq event channel. */
> -        rc = alloc_unbound_xen_event_channel(v, 0, NULL);
> +        rc = alloc_unbound_xen_event_channel(v,
current->domain->domain_id,
> NULL);
>          if ( rc < 0 )
>              goto fail2;
>          v->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN]
= rc;

>>> On 09.01.13 at 09:51, Keir Fraser <keir@xen.org> wrote:
> On 03/01/2013 18:28, "Daniel De Graaf"
<dgdegra@tycho.nsa.gov> wrote:
> 
>> Instead of using a hardcoded domain 0 as the endpoint for the event
>> channels created in hvm_vcpu_initialise, use the domain ID of the
>> building domain so that a domain builder in a domain other than dom0
has
>> the expected access to the event channels.
> 
> This should be done by setting HVM_PARAM_DM_DOMAIN appropriately after the
> guest is created. As it is, HVM_PARAM_DM_DOMAIN can be out of sync with the
> event-channel bindings after this patch.
> 
> I see this patch is already applied, and I propose to revert it. Is that
> okay with you, Jan?
Sure. Am I right in understanding that this, apart from a possible
tools side change to set the parameter earlier, then means to use
v->domain->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN]
instead in the two places that Daniel''s patch changed? If so rather
than reverting, we could as well adjust it to that right away...

Looks like I didn''t wait long enough for feedback on the
(apparently trivial) patch here...

Jan

On Thu, 2013-01-03 at 18:28 +0000, Daniel De Graaf
wrote:
> Instead of using a hardcoded domain 0 as the endpoint for the event
> channels created in hvm_vcpu_initialise, use the domain ID of the
> building domain so that a domain builder in a domain other than dom0 has
> the expected access to the event channels.
OOI what is the builder (I assume it''s not specific to being a separate
domain) doing that requires it to access to the IOEMU event channels?

Ian.

On 01/09/2013 06:12 AM, Ian Campbell wrote:
> On Thu, 2013-01-03 at 18:28 +0000, Daniel De Graaf wrote:
>> Instead of using a hardcoded domain 0 as the endpoint for the event
>> channels created in hvm_vcpu_initialise, use the domain ID of the
>> building domain so that a domain builder in a domain other than dom0
has
>> the expected access to the event channels.
> 
> OOI what is the builder (I assume it''s not specific to being a
separate
> domain) doing that requires it to access to the IOEMU event channels?
> 
> Ian.
> 
I believe this caused problems when the device model was running in the 
same domain as the domain builder (where that was not dom0), not during
the domain build process.  After seeing Keir and Jan''s comments, I
think
the best solution is for the domain builder to set HVM_PARAM_DM_DOMAIN 
to DOMID_SELF (or the actual device model domain ID) earlier in the 
build process and use that parameter in the event channel creation
instead of 0 or current->domain->domain_id.

-- 
Daniel De Graaf
National Security Agency

On Wed, 2013-01-09 at 14:43 +0000, Daniel De Graaf
wrote:
> On 01/09/2013 06:12 AM, Ian Campbell wrote:
> > On Thu, 2013-01-03 at 18:28 +0000, Daniel De Graaf wrote:
> >> Instead of using a hardcoded domain 0 as the endpoint for the
event
> >> channels created in hvm_vcpu_initialise, use the domain ID of the
> >> building domain so that a domain builder in a domain other than
dom0 has
> >> the expected access to the event channels.
> > 
> > OOI what is the builder (I assume it''s not specific to being
a separate
> > domain) doing that requires it to access to the IOEMU event channels?
> > 
> > Ian.
> > 
> 
> I believe this caused problems when the device model was running in the 
> same domain as the domain builder (where that was not dom0), not during
> the domain build process.
I thought the DM set HVM_PARAM_DM_DOMAIN itself?
> After seeing Keir and Jan''s comments, I think
> the best solution is for the domain builder to set HVM_PARAM_DM_DOMAIN 
> to DOMID_SELF (or the actual device model domain ID)
 
libxc probably doesn''t know this, FWIW.
>  earlier in the 
> build process and use that parameter in the event channel creation
> instead of 0 or current->domain->domain_id.

On 09/01/2013 14:43, "Daniel De Graaf" <dgdegra@tycho.nsa.gov>
wrote:
>> OOI what is the builder (I assume it''s not specific to being a
separate
>> domain) doing that requires it to access to the IOEMU event channels?
>> 
>> Ian.
>> 
> 
> I believe this caused problems when the device model was running in the
> same domain as the domain builder (where that was not dom0), not during
> the domain build process.  After seeing Keir and Jan''s comments, I
think
> the best solution is for the domain builder to set HVM_PARAM_DM_DOMAIN
> to DOMID_SELF (or the actual device model domain ID) earlier in the
> build process and use that parameter in the event channel creation
> instead of 0 or current->domain->domain_id.
This is the fix I have just applied as xen-unstable:26339. See what you
think. It will require you to set HVM_PARAM_DM_DOMAIN from the toolstack at
some point during domain creation, any time before the device model starts
running.

 -- Keir

On 01/09/2013 09:56 AM, Ian Campbell wrote:
> On Wed, 2013-01-09 at 14:43 +0000, Daniel De Graaf wrote:
>> On 01/09/2013 06:12 AM, Ian Campbell wrote:
>>> On Thu, 2013-01-03 at 18:28 +0000, Daniel De Graaf wrote:
>>>> Instead of using a hardcoded domain 0 as the endpoint for the
event
>>>> channels created in hvm_vcpu_initialise, use the domain ID of
the
>>>> building domain so that a domain builder in a domain other than
dom0 has
>>>> the expected access to the event channels.
>>>
>>> OOI what is the builder (I assume it''s not specific to
being a separate
>>> domain) doing that requires it to access to the IOEMU event
channels?
>>>
>>> Ian.
>>>
>>
>> I believe this caused problems when the device model was running in the
>> same domain as the domain builder (where that was not dom0), not during
>> the domain build process.
> 
> I thought the DM set HVM_PARAM_DM_DOMAIN itself?
It does, but only qemu-xen-traditional and only inside a stubdom due to:
#ifdef CONFIG_STUBDOM /* the hvmop is not supported on older hypervisors */
    xc_set_hvm_param(xc_handle, domid, HVM_PARAM_DM_DOMAIN, DOMID_SELF);
#endif


I was considering making HVM_PARAM_DM_DOMAIN effectively default to 
DOMID_SELF for the domain builder with the below patch, since the most
logical place to set the parameter in libxl (hvm_build_set_params called
by libxl__build_hvm) is called after vcpus are initialized in
libxl__build_pre, which means the event channels will be assigned to
domain 0 for a short time and then reassigned later.  It looks like
waiting for the device model domain to be started will also be
significantly later in the build process, since it''s done in the
callbacks after adding disks.  While this is probably harmless, it just 
seems cleaner to initialize it to the domain ID from the start.

------------------------------------->8---------------------------

arch/x86/hvm: initialize HVM_PARAM_DM_DOMAIN to the building domain

Instead of assuming domain 0 runs the device model unless otherwise set,
initialize the parameter to the domain ID of the building domain.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 123a147..a1fb363 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -529,6 +529,7 @@ int hvm_domain_initialise(struct domain *d)
     hvm_init_guest_time(d);
 
     d->arch.hvm_domain.params[HVM_PARAM_HPET_ENABLED] = 1;
+    d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN] =
current->domain->domain_id;
 
     hvm_init_cacheattr_region_list(d);
 
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 6b05f61..6348ab0 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -92,7 +92,7 @@
 /* Identity-map page directory used by Intel EPT when CR0.PG=0. */
 #define HVM_PARAM_IDENT_PT     12
 
-/* Device Model domain, defaults to 0. */
+/* Device Model domain, defaults to the building domain. */
 #define HVM_PARAM_DM_DOMAIN    13
 
 /* ACPI S state: currently support S0 and S3 on x86. */

On 09/01/2013 15:30, "Daniel De Graaf" <dgdegra@tycho.nsa.gov>
wrote:
> I was considering making HVM_PARAM_DM_DOMAIN effectively default to
> DOMID_SELF for the domain builder with the below patch, since the most
> logical place to set the parameter in libxl (hvm_build_set_params called
> by libxl__build_hvm) is called after vcpus are initialized in
> libxl__build_pre, which means the event channels will be assigned to
> domain 0 for a short time and then reassigned later.  It looks like
> waiting for the device model domain to be started will also be
> significantly later in the build process, since it''s done in the
> callbacks after adding disks.  While this is probably harmless, it just
> seems cleaner to initialize it to the domain ID from the start.
Xen handles doing it later just fine. I don''t think any further patches
to
Xen are required -- just do an hvm_set_param() call at any convenient time
from the toolstack.

 -- Keir

On Wed, 2013-01-09 at 15:30 +0000, Daniel De Graaf
wrote:
> > I thought the DM set HVM_PARAM_DM_DOMAIN itself?
> 
> It does, but only qemu-xen-traditional and only inside a stubdom due to:
> #ifdef CONFIG_STUBDOM /* the hvmop is not supported on older hypervisors */
>     xc_set_hvm_param(xc_handle, domid, HVM_PARAM_DM_DOMAIN, DOMID_SELF);
> #endif
Ah, my grep didn''t show me the ifdef!

Ian.