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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 14/11/2023 09:12, Juergen Gross wrote:
> On 14.11.23 09:56, Julien Grall wrote:
> > 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.
> The test of XENSTORE_RECONNECT is done for domUs started before or 
> together with
> dom0 in order to give them a signal that they can start to use Xenstore.
>
> Here we are initializing our own connection, so there is no need to test 
> whether
> the other end is waiting for us. We know there is no possible problem 
> sending
> the event, so we can just do it. The main instruction guarded by the 
> test of
> XENSTORE_RECONNECT is the setting of XENSTORE_CONNECTED, which then 
> needs the
> event to be sent to signal that change in the connection state.
>
> In the end we are _not_ diverging. You should just compare the code to 
> the more
> comparable dom0_init() code. There the event is being sent 
> unconditionally, too.
Ok. Can this be documented somewhere?

Cheers,

--
Julien Grall