Re: [PATCH v4 07/12] tools/xenstored: Auto-introduce domains

[Jürgen Groß <jgross@xxxxxxxx>]

On 25.07.25 04:28, Jason Andryuk wrote:
> Replace dom0_init() with init_domains() which uses libxenmanage to
> iterate through all existing domains, storing them in a list.  The xenstore
> domain is introduced first, and then all the other domains are
> introduced.  The xenstore domain needs to be introduced first to setup
> structures needed for firing watches.
>
> 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.
>
> To support ARM dom0less domains with xen,enhanced = "no-xenstore" a
> failed introduce_domain() becomes non-fatal.  Normally,
> HVM_PARAM_STORE_EVTHCN is used to identify .
HVM_PARAM_STORE_EVTCHN

> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> ---
> I noticed domain_conn_reset() isn't called for the stubdom, so I added
> the ifdef to special case it.  I haven't tested with a stubdom yet, and
> I wanted to be conservative.  Ideally it would be dropped - the issue
> would be if the stubdom queues requests before xenstore is in service.
> ---
>   tools/xenstored/core.c   |  2 +-
>   tools/xenstored/domain.c | 83 ++++++++++++++++++++++++++++------------
>   tools/xenstored/domain.h |  2 +-
>   3 files changed, 61 insertions(+), 26 deletions(-)
>
> diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
> index 37e4dd5a5b..c2f8d20211 100644
> --- a/tools/xenstored/core.c
> +++ b/tools/xenstored/core.c
> @@ -2757,7 +2757,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 1c52254ba8..e9e45ed8e8 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c
> @@ -1280,41 +1280,76 @@ evtchn_port_t get_domain_evtchn(domid_t domid)
>         return 0;
>   }
>   
> -void dom0_init(void)
> +void init_domains(void)
>   {
> -       evtchn_port_t port;
> -       struct domain *dom0;
> +       unsigned int *domids = NULL;
> +       unsigned int nr_domids = 0;
> +       unsigned int domid;
> +       unsigned int state;
> +       unsigned int caps;
> +       uint64_t unique_id;
> +       int introduce_count = 0;
> +
> +       while (!xenmanage_poll_changed_domain(xm_handle, &domid, &state, &caps,
> +                                             &unique_id)) {
> +               nr_domids++;
> +               domids = realloc(domids, nr_domids * sizeof(*domids));
Please use the talloc framework for memory allocations.

> +               if (!domids)
> +                       barf_perror("Failed to reallocate domids");
> +
> +               domids[nr_domids - 1] = domid;
> +
> +               if (caps & XENMANAGE_GETDOMSTATE_CAP_XENSTORE) {
> +                       memmove(&domids[1], domids, (nr_domids - 1) * 
> sizeof(*domids));
> +                       /*
> +                        * Local domid must be first to setup structures for
> +                        * firing the special watches.
> +                        */
> +                       domids[0] = domid;
> +                       dom0_domid = domid;
> +               }
You could move the loop body below in a helper function and call that
for the Xenstore domain just here without adding the domid to domids[].
This would require to do the XENMANAGE_GETDOMSTATE_CAP_XENSTORE test
after all other caps tests.

>   
> -	port = get_domain_evtchn(xenbus_master_domid());
> -       if (port == -1)
> -               barf_perror("Failed to initialize dom0 port");
> +               if (caps & XENMANAGE_GETDOMSTATE_CAP_CONTROL)
> +                       priv_domid = domid;
> +       }
Is it okay to overwrite priv_domid in case multiple domains have
XENMANAGE_GETDOMSTATE_CAP_CONTROL active? Same question regarding
XENMANAGE_GETDOMSTATE_CAP_XENSTORE above.

>   
> -	dom0 = introduce_domain(NULL, xenbus_master_domid(), port, false);
> -       if (!dom0)
> -               barf_perror("Failed to initialize dom0");
> +       for (unsigned int i = 0; i < nr_domids; i++) {
> +               evtchn_port_t port;
> +               struct domain *domain;
> +               domid = domids[i];
>   
> -	xenevtchn_notify(xce_handle, dom0->port);
> -}
> +               port = get_domain_evtchn(domid);
> +               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;
> +               }
> +               introduce_count++;
>   
> -void stubdom_init(void)
> -{
>   #ifdef __MINIOS__
> -       struct domain *stubdom;
> -       evtchn_port_t port;
> +               if (domid != stub_domid)
> +#endif
> +                       domain_conn_reset(domain);
You definitely don't want to call domain_conn_reset() here.

It is NOT called for dom0 today and for normal domUs it is called before
the domain gets started.

>   
> -	if (stub_domid < 0)
> -               return;
> +               if (domain->interface)
> +                       domain->interface->connection = XENSTORE_CONNECTED;
>   
> -	port = get_domain_evtchn(stub_domid);
> -       if (port == -1)
> -               barf_perror("Failed to initialize dom0 port");
> +               /* Notify the domain that xenstore is available */
> +               xenevtchn_notify(xce_handle, domain->port);
> +       }
>   
> -	stubdom = introduce_domain(NULL, stub_domid, port, false);
> -       if (!stubdom)
> -               barf_perror("Failed to initialize stubdom");
> +       free(domids);
>   
> -	xenevtchn_notify(xce_handle, stubdom->port);
> +       if (introduce_count == 0)
> +               barf("Did not initialize any domains");
> +}
>   
> +void stubdom_init(void)
> +{
> +#ifdef __MINIOS__
>         mount_9pfs();
>   #endif
>   }
> 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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature