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

[Juergen Gross <jgross@xxxxxxxx>]

On 14.11.23 21:48, Julien Grall wrote:
> 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?
I'll add something to the commit message.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature