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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-07-25 03:04, Jürgen Groß wrote:
> 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
Thanks.

> > 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.
> > 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.
Yes, of course.

> > +        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.
Yes, good idea.  Actually, given the "last set" xenstore domid issue I 
mention below, I'll move it between the two loops.
> > -    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?
Probably not, no.  First domain, being domain 0, getting priv_domid is 
probably what we want by default.  And we should respect the command 
line value if set.
if (priv_domid != DOMID_INVALID)
    priv_domid = domid;

> Same question regarding
> XENMANAGE_GETDOMSTATE_CAP_XENSTORE above.
dom0less only allows creating 1 xenstore domain... but the "late" 
stubdom case is different.  dom0 and the stubdom will both have 
XENMANAGE_GETDOMSTATE_CAP_XENSTORE.  Since in that case, we only have 
dom0 and dom1, letting the second one take does what we want.  dom0 
assigning VIRQ_DOM_EXC to another domain could transfer the capability, 
and then there would only be one.  Similarly, passing --master-domid to 
minios would make it explicit.  (I'm not a fan of passing the domain's 
own domid).
A more explicit way to convey the information would be better.  Directly 
exposing the domid and the capabilities to the domain would let 
xenstored know exactly what it should configure.
Until then, the cases we have are either a single xenstore domain, or 
dom0 and dom1.  Selecting the last one will do the right thing.
> > -    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.
Ok, thanks.  I didn't look back far enough, so I didn't realize I added 
it myself.  I did that to make this loop as close as possible to 
do_introduce() for the domUs.  Since there shouldn't be anything to 
reset, I'll drop it.
Thanks,
Jason