Mailing List Archive

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;



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

>>> 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


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

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.



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

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

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

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.



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

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



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

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. */


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

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



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

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.


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