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

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

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature