Re: [PATCH v2 25/29] tools/xenstored: map stubdom interface

[Julien Grall <julien@xxxxxxx>]

Hi,

On 14/11/2023 06:33, Juergen Gross wrote:
> On 13.11.23 23:04, Julien Grall wrote:
> > Hi Juergen,
> >
> > On 10/11/2023 16:08, Juergen Gross wrote:
> > > When running as stubdom, map the stubdom's Xenstore ring page in order
> > > to support using the 9pfs frontend.
> > >
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > ---
> > >   tools/xenstored/core.c   |  2 ++
> > >   tools/xenstored/domain.c | 27 ++++++++++++++++++++++++++-
> > >   tools/xenstored/domain.h |  1 +
> > >   3 files changed, 29 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
> > > index b9ec50b34c..4a9d874f17 100644
> > > --- a/tools/xenstored/core.c
> > > +++ b/tools/xenstored/core.c
> > > @@ -2991,6 +2991,8 @@ int main(int argc, char *argv[])
> > >           lu_read_state();
> > >   #endif
> > > +    stubdom_init();
> > > +
> > >       check_store();
> > >       /* Get ready to listen to the tools. */
> > > diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> > > index fa17f68618..162b87b460 100644
> > > --- a/tools/xenstored/domain.c
> > > +++ b/tools/xenstored/domain.c
> > > @@ -37,6 +37,10 @@
> > >   #include <xenctrl.h>
> > >   #include <xen/grant_table.h>
> > > +#ifdef __MINIOS__
> > > +#include <mini-os/xenbus.h>
> > > +#endif
> > > +
> > >   static xc_interface **xc_handle;
> > >   xengnttab_handle **xgt_handle;
> > >   static evtchn_port_t virq_port;
> > > @@ -500,6 +504,11 @@ static void *map_interface(domid_t domid)
> > >       if (domid == xenbus_master_domid())
> > >           return xenbus_map();
> > > +#ifdef __MINIOS__
> > > +    if (domid == stub_domid)
> > > +        return xenstore_buf;
> > > +#endif
> > > +
> > >       return xengnttab_map_grant_ref(*xgt_handle, domid,
> > >                          GNTTAB_RESERVED_XENSTORE,
> > >                          PROT_READ|PROT_WRITE);
> > > @@ -509,7 +518,7 @@ static void unmap_interface(domid_t domid, void 
> > > *interface)
> > >   {
> > >       if (domid == xenbus_master_domid())
> > >           unmap_xenbus(interface);
> > > -    else
> > > +    else if (domid != stub_domid)
> > >           xengnttab_unmap(*xgt_handle, interface, 1);
> > >   }
> > > @@ -1214,6 +1223,22 @@ void dom0_init(void)
> > >       xenevtchn_notify(xce_handle, dom0->port);
> > >   }
> > > +void stubdom_init(void)
> > > +{
> > > +#ifdef __MINIOS__
> > > +    struct domain *stubdom;
> > > +
> > > +    if (stub_domid < 0)
> > > +        return;
> > > +
> > > +    stubdom = introduce_domain(NULL, stub_domid, xenbus_evtchn, false);
> > > +    if (!stubdom)
> > > +        barf_perror("Failed to initialize stubdom");
> > > +
> > > +    xenevtchn_notify(xce_handle, stubdom->port);
> > If I compare to introduce_domain(), it is not entirely clear to me why 
> > the notification is done unconditionally here. Can you clarify?
> There is no reason to do it conditionally, as we can be sure the event 
> channel
> is existing and the ring page is accessible.
Hmmm... But there is a second part in the condition:

domain->interface->connection == XENSTORE_RECONNECT

Why isn't it necessary here? What I am looking for particularly is some 
in-code comment (or at least in the commit message) because this is not 
100% clear why we are diverging.
Cheers,

--
Julien Grall