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

[Juergen Gross <jgross@xxxxxxxx>]

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.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature