[PATCH v2 14/17] tools/xenstored: Auto-introduce domains

Jason Andryuk posted 17 patches 3 months, 2 weeks ago
[PATCH v2 14/17] tools/xenstored: Auto-introduce domains
Posted by Jason Andryuk 3 months, 2 weeks ago
Replace dom0_init() with init_domains() which uses libxenmanage to
iterate through all existing domains and introduce them.

dom0_domid is updated with the xenstore domain, since it really
indicates the local domain.

priv_domid is set to the control domain.  This makes it limited to a
single domain.

These features let xenstore automatically connect any existing domains,
which means it doesn't need to be done manually from init-dom0less.

For a legacy dom0, the result should be unchanged.

For a late xenstore stubdom it should also be the same, but priv_domid
would be set automatically to control domain (which default to 0
normally).

Always signal the event channel for initial domains.  This gets dom0 (a
local xenstored domain) to connect.

Also always write XENSTORE_CONNECTED since we know we are connected at
this point.

Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
 tools/xenstored/core.c   |  2 +-
 tools/xenstored/domain.c | 45 +++++++++++++++++++++++++++++++---------
 tools/xenstored/domain.h |  2 +-
 3 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
index 550e879a00..835402af81 100644
--- a/tools/xenstored/core.c
+++ b/tools/xenstored/core.c
@@ -2762,7 +2762,7 @@ int main(int argc, char *argv[])
 	/* Listen to hypervisor. */
 	if (!live_update) {
 		domain_init(-1);
-		dom0_init();
+		init_domains();
 	}
 
 	/* redirect to /dev/null now we're ready to accept connections */
diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
index 5443b4e608..44e997cee4 100644
--- a/tools/xenstored/domain.c
+++ b/tools/xenstored/domain.c
@@ -1257,20 +1257,45 @@ const char *get_implicit_path(const struct connection *conn)
 	return conn->domain->path;
 }
 
-void dom0_init(void)
+void init_domains(void)
 {
-	evtchn_port_t port;
-	struct domain *dom0;
+	unsigned int domid;
+	unsigned int state;
+	unsigned int caps;
+	uint64_t unique_id;
+
+	while (!xenmanage_poll_changed_domain(xm_handle, &domid, &state, &caps,
+					      &unique_id)) {
+		evtchn_port_t port = 0;
+		struct domain *domain;
+
+		if (caps & XENMANAGE_GETDOMSTATE_CAP_XENSTORE)
+			dom0_domid = domid;
+
+		if (caps & XENMANAGE_GETDOMSTATE_CAP_CONTROL)
+			priv_domid = domid;
 
-	port = get_xenbus_evtchn();
-	if (port == -1)
-		barf_perror("Failed to initialize dom0 port");
+		if (domid == dom0_domid) {
+			port = get_xenbus_evtchn();
+			if (port == -1)
+				barf_perror("Failed to initialize dom%u port",
+					    domid);
+		}
+
+		domain = introduce_domain(NULL, domid, port, false);
+		if (!domain) {
+			xprintf("Could not initialize dom%u", domid);
+			continue;
+		}
 
-	dom0 = introduce_domain(NULL, xenbus_master_domid(), port, false);
-	if (!dom0)
-		barf_perror("Failed to initialize dom0");
+		domain_conn_reset(domain);
 
-	xenevtchn_notify(xce_handle, dom0->port);
+		if (domain->interface)
+			domain->interface->connection = XENSTORE_CONNECTED;
+
+		/* Notify the domain that xenstore is available */
+		xenevtchn_notify(xce_handle, domain->port);
+	}
 }
 
 void stubdom_init(void)
diff --git a/tools/xenstored/domain.h b/tools/xenstored/domain.h
index 844ac11510..6a78f06935 100644
--- a/tools/xenstored/domain.h
+++ b/tools/xenstored/domain.h
@@ -84,7 +84,7 @@ int do_reset_watches(const void *ctx, struct connection *conn,
 
 void domain_early_init(void);
 void domain_init(int evtfd);
-void dom0_init(void);
+void init_domains(void);
 void stubdom_init(void);
 void domain_deinit(void);
 void ignore_connection(struct connection *conn, unsigned int err);
-- 
2.50.0
Re: [PATCH v2 14/17] tools/xenstored: Auto-introduce domains
Posted by Juergen Gross 3 months, 2 weeks ago
On 16.07.25 23:15, Jason Andryuk wrote:
> Replace dom0_init() with init_domains() which uses libxenmanage to
> iterate through all existing domains and introduce them.
> 
> dom0_domid is updated with the xenstore domain, since it really
> indicates the local domain.

I agree with that explanation, but I wonder whether this explanation
doesn't indicate that a rename of the dom0_domid variable is wanted,
e.g. to "store_domid".

> priv_domid is set to the control domain.  This makes it limited to a
> single domain.

Maybe another candidate for renaming (ctrl_domid?).

> These features let xenstore automatically connect any existing domains,
> which means it doesn't need to be done manually from init-dom0less.
> 
> For a legacy dom0, the result should be unchanged.
> 
> For a late xenstore stubdom it should also be the same, but priv_domid
> would be set automatically to control domain (which default to 0
> normally).
> 
> Always signal the event channel for initial domains.  This gets dom0 (a
> local xenstored domain) to connect.
> 
> Also always write XENSTORE_CONNECTED since we know we are connected at
> this point.
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
>   tools/xenstored/core.c   |  2 +-
>   tools/xenstored/domain.c | 45 +++++++++++++++++++++++++++++++---------
>   tools/xenstored/domain.h |  2 +-
>   3 files changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
> index 550e879a00..835402af81 100644
> --- a/tools/xenstored/core.c
> +++ b/tools/xenstored/core.c
> @@ -2762,7 +2762,7 @@ int main(int argc, char *argv[])
>   	/* Listen to hypervisor. */
>   	if (!live_update) {
>   		domain_init(-1);
> -		dom0_init();
> +		init_domains();
>   	}
>   
>   	/* redirect to /dev/null now we're ready to accept connections */
> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index 5443b4e608..44e997cee4 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c
> @@ -1257,20 +1257,45 @@ const char *get_implicit_path(const struct connection *conn)
>   	return conn->domain->path;
>   }
>   
> -void dom0_init(void)
> +void init_domains(void)
>   {
> -	evtchn_port_t port;
> -	struct domain *dom0;
> +	unsigned int domid;
> +	unsigned int state;
> +	unsigned int caps;
> +	uint64_t unique_id;
> +
> +	while (!xenmanage_poll_changed_domain(xm_handle, &domid, &state, &caps,
> +					      &unique_id)) {
> +		evtchn_port_t port = 0;
> +		struct domain *domain;
> +
> +		if (caps & XENMANAGE_GETDOMSTATE_CAP_XENSTORE)
> +			dom0_domid = domid;
> +
> +		if (caps & XENMANAGE_GETDOMSTATE_CAP_CONTROL)
> +			priv_domid = domid;
>   
> -	port = get_xenbus_evtchn();
> -	if (port == -1)
> -		barf_perror("Failed to initialize dom0 port");
> +		if (domid == dom0_domid) {
> +			port = get_xenbus_evtchn();
> +			if (port == -1)
> +				barf_perror("Failed to initialize dom%u port",
> +					    domid);
> +		}

I don't think this is correct for a xenstore-stubdom. See stubdom_init().

> +
> +		domain = introduce_domain(NULL, domid, port, false);
> +		if (!domain) {
> +			xprintf("Could not initialize dom%u", domid);
> +			continue;
> +		}
>   
> -	dom0 = introduce_domain(NULL, xenbus_master_domid(), port, false);
> -	if (!dom0)
> -		barf_perror("Failed to initialize dom0");
> +		domain_conn_reset(domain);
>   
> -	xenevtchn_notify(xce_handle, dom0->port);
> +		if (domain->interface)
> +			domain->interface->connection = XENSTORE_CONNECTED;
> +
> +		/* Notify the domain that xenstore is available */
> +		xenevtchn_notify(xce_handle, domain->port);
> +	}
>   }
>   
>   void stubdom_init(void)
> diff --git a/tools/xenstored/domain.h b/tools/xenstored/domain.h
> index 844ac11510..6a78f06935 100644
> --- a/tools/xenstored/domain.h
> +++ b/tools/xenstored/domain.h
> @@ -84,7 +84,7 @@ int do_reset_watches(const void *ctx, struct connection *conn,
>   
>   void domain_early_init(void);
>   void domain_init(int evtfd);
> -void dom0_init(void);
> +void init_domains(void);
>   void stubdom_init(void);
>   void domain_deinit(void);
>   void ignore_connection(struct connection *conn, unsigned int err);


Juergen
Re: [PATCH v2 14/17] tools/xenstored: Auto-introduce domains
Posted by Jason Andryuk 3 months, 2 weeks ago
On 2025-07-17 04:50, Juergen Gross wrote:
> On 16.07.25 23:15, Jason Andryuk wrote:
>> Replace dom0_init() with init_domains() which uses libxenmanage to
>> iterate through all existing domains and introduce them.
>>
>> dom0_domid is updated with the xenstore domain, since it really
>> indicates the local domain.
> 
> I agree with that explanation, but I wonder whether this explanation
> doesn't indicate that a rename of the dom0_domid variable is wanted,
> e.g. to "store_domid".

I've written a patch using "local_domid", though "store_domid" would be 
fine.  I used "local" since I thought that would be better for 
indicating when we need to use /proc/xen/xsd_* for a "local" access. 
And for xenstore-stubdom, local_domain would use the special case code.

>> priv_domid is set to the control domain.  This makes it limited to a
>> single domain.
> 
> Maybe another candidate for renaming (ctrl_domid?).

I've further experimented with replacing priv_domid checks with a 
per-connection is_priv flag.  Though now that I've written that down, 
maybe we don't want to support multiple domains having Xenstore privilege?

>> These features let xenstore automatically connect any existing domains,
>> which means it doesn't need to be done manually from init-dom0less.
>>
>> For a legacy dom0, the result should be unchanged.
>>
>> For a late xenstore stubdom it should also be the same, but priv_domid
>> would be set automatically to control domain (which default to 0
>> normally).
>>
>> Always signal the event channel for initial domains.  This gets dom0 (a
>> local xenstored domain) to connect.
>>
>> Also always write XENSTORE_CONNECTED since we know we are connected at
>> this point.
>>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> ---
>>   tools/xenstored/core.c   |  2 +-
>>   tools/xenstored/domain.c | 45 +++++++++++++++++++++++++++++++---------
>>   tools/xenstored/domain.h |  2 +-
>>   3 files changed, 37 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
>> index 550e879a00..835402af81 100644
>> --- a/tools/xenstored/core.c
>> +++ b/tools/xenstored/core.c
>> @@ -2762,7 +2762,7 @@ int main(int argc, char *argv[])
>>       /* Listen to hypervisor. */
>>       if (!live_update) {
>>           domain_init(-1);
>> -        dom0_init();
>> +        init_domains();
>>       }
>>       /* redirect to /dev/null now we're ready to accept connections */
>> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
>> index 5443b4e608..44e997cee4 100644
>> --- a/tools/xenstored/domain.c
>> +++ b/tools/xenstored/domain.c
>> @@ -1257,20 +1257,45 @@ const char *get_implicit_path(const struct 
>> connection *conn)
>>       return conn->domain->path;
>>   }
>> -void dom0_init(void)
>> +void init_domains(void)
>>   {
>> -    evtchn_port_t port;
>> -    struct domain *dom0;
>> +    unsigned int domid;
>> +    unsigned int state;
>> +    unsigned int caps;
>> +    uint64_t unique_id;
>> +
>> +    while (!xenmanage_poll_changed_domain(xm_handle, &domid, &state, 
>> &caps,
>> +                          &unique_id)) {
>> +        evtchn_port_t port = 0;
>> +        struct domain *domain;
>> +
>> +        if (caps & XENMANAGE_GETDOMSTATE_CAP_XENSTORE)
>> +            dom0_domid = domid;
>> +
>> +        if (caps & XENMANAGE_GETDOMSTATE_CAP_CONTROL)
>> +            priv_domid = domid;
>> -    port = get_xenbus_evtchn();
>> -    if (port == -1)
>> -        barf_perror("Failed to initialize dom0 port");
>> +        if (domid == dom0_domid) {
>> +            port = get_xenbus_evtchn();
>> +            if (port == -1)
>> +                barf_perror("Failed to initialize dom%u port",
>> +                        domid);
>> +        }
> 
> I don't think this is correct for a xenstore-stubdom. See stubdom_init().

Yes, you are right.  There is a mismatch where for xenstored, dom0_domid 
means the local domain, but for stubdom, dom0_domid is dom0.  I have 
some further changes I need to revisit that work to address that. 
Basically, make minios.c and posix.c only handle the "local" case. 
Anything that is not local is just a grant map.  With that in place, 
stubdom_init() does not need an introduce_domain() call.

I think I hadn't figured out exactly how to handle the dom0 event 
channel passed on the command line to the stubdom.

>> +
>> +        domain = introduce_domain(NULL, domid, port, false);
>> +        if (!domain) {
>> +            xprintf("Could not initialize dom%u", domid);
>> +            continue;

I need to expand the commit message to mention this change.  Here I made 
failure non-fatal since for ARM there can be domains without xenstore 
access.  They can be identified by an hvm_param, but xenstore doesn't 
have permission to read those.  Here introduce_domain() is called on 
them, and the grant mapping fails.  So we note the error here and 
continue on.

Thanks,
Jason

Re: [PATCH v2 14/17] tools/xenstored: Auto-introduce domains
Posted by Jürgen Groß 3 months, 2 weeks ago
On 17.07.25 23:39, Jason Andryuk wrote:
> On 2025-07-17 04:50, Juergen Gross wrote:
>> On 16.07.25 23:15, Jason Andryuk wrote:
>>> Replace dom0_init() with init_domains() which uses libxenmanage to
>>> iterate through all existing domains and introduce them.
>>>
>>> dom0_domid is updated with the xenstore domain, since it really
>>> indicates the local domain.
>>
>> I agree with that explanation, but I wonder whether this explanation
>> doesn't indicate that a rename of the dom0_domid variable is wanted,
>> e.g. to "store_domid".
> 
> I've written a patch using "local_domid", though "store_domid" would be fine.  I 
> used "local" since I thought that would be better for indicating when we need to 
> use /proc/xen/xsd_* for a "local" access. And for xenstore-stubdom, local_domain 
> would use the special case code.

I'd prefer store_domid.

> 
>>> priv_domid is set to the control domain.  This makes it limited to a
>>> single domain.
>>
>> Maybe another candidate for renaming (ctrl_domid?).
> 
> I've further experimented with replacing priv_domid checks with a per-connection 
> is_priv flag.  Though now that I've written that down, maybe we don't want to 
> support multiple domains having Xenstore privilege?

At least not per default. When needed it might be possible to add
a new privileged XS_ command to grant another domain that status.

> 
>>> These features let xenstore automatically connect any existing domains,
>>> which means it doesn't need to be done manually from init-dom0less.
>>>
>>> For a legacy dom0, the result should be unchanged.
>>>
>>> For a late xenstore stubdom it should also be the same, but priv_domid
>>> would be set automatically to control domain (which default to 0
>>> normally).
>>>
>>> Always signal the event channel for initial domains.  This gets dom0 (a
>>> local xenstored domain) to connect.
>>>
>>> Also always write XENSTORE_CONNECTED since we know we are connected at
>>> this point.
>>>
>>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>>> ---
>>>   tools/xenstored/core.c   |  2 +-
>>>   tools/xenstored/domain.c | 45 +++++++++++++++++++++++++++++++---------
>>>   tools/xenstored/domain.h |  2 +-
>>>   3 files changed, 37 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
>>> index 550e879a00..835402af81 100644
>>> --- a/tools/xenstored/core.c
>>> +++ b/tools/xenstored/core.c
>>> @@ -2762,7 +2762,7 @@ int main(int argc, char *argv[])
>>>       /* Listen to hypervisor. */
>>>       if (!live_update) {
>>>           domain_init(-1);
>>> -        dom0_init();
>>> +        init_domains();
>>>       }
>>>       /* redirect to /dev/null now we're ready to accept connections */
>>> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
>>> index 5443b4e608..44e997cee4 100644
>>> --- a/tools/xenstored/domain.c
>>> +++ b/tools/xenstored/domain.c
>>> @@ -1257,20 +1257,45 @@ const char *get_implicit_path(const struct connection 
>>> *conn)
>>>       return conn->domain->path;
>>>   }
>>> -void dom0_init(void)
>>> +void init_domains(void)
>>>   {
>>> -    evtchn_port_t port;
>>> -    struct domain *dom0;
>>> +    unsigned int domid;
>>> +    unsigned int state;
>>> +    unsigned int caps;
>>> +    uint64_t unique_id;
>>> +
>>> +    while (!xenmanage_poll_changed_domain(xm_handle, &domid, &state, &caps,
>>> +                          &unique_id)) {
>>> +        evtchn_port_t port = 0;
>>> +        struct domain *domain;
>>> +
>>> +        if (caps & XENMANAGE_GETDOMSTATE_CAP_XENSTORE)
>>> +            dom0_domid = domid;
>>> +
>>> +        if (caps & XENMANAGE_GETDOMSTATE_CAP_CONTROL)
>>> +            priv_domid = domid;
>>> -    port = get_xenbus_evtchn();
>>> -    if (port == -1)
>>> -        barf_perror("Failed to initialize dom0 port");
>>> +        if (domid == dom0_domid) {
>>> +            port = get_xenbus_evtchn();
>>> +            if (port == -1)
>>> +                barf_perror("Failed to initialize dom%u port",
>>> +                        domid);
>>> +        }
>>
>> I don't think this is correct for a xenstore-stubdom. See stubdom_init().
> 
> Yes, you are right.  There is a mismatch where for xenstored, dom0_domid means 
> the local domain, but for stubdom, dom0_domid is dom0.  I have some further 
> changes I need to revisit that work to address that. Basically, make minios.c 
> and posix.c only handle the "local" case. Anything that is not local is just a 
> grant map.  With that in place, stubdom_init() does not need an 
> introduce_domain() call.

I agree with that approach.

> 
> I think I hadn't figured out exactly how to handle the dom0 event channel passed 
> on the command line to the stubdom.
> 
>>> +
>>> +        domain = introduce_domain(NULL, domid, port, false);
>>> +        if (!domain) {
>>> +            xprintf("Could not initialize dom%u", domid);
>>> +            continue;
> 
> I need to expand the commit message to mention this change.  Here I made failure 
> non-fatal since for ARM there can be domains without xenstore access.  They can 
> be identified by an hvm_param, but xenstore doesn't have permission to read 
> those.  Here introduce_domain() is called on them, and the grant mapping fails.  
> So we note the error here and continue on.

Right, but I think you should print errno as well, which means that you
need to make sure that errno is set to a specific value in case the
mapping fails.

This might be needed to distinguish this case from other errors, like
ENOMEM (which would be rather strange at this early phase, though).


Juergen