diff mbox series

[XEN,6/7] xenstored: do_introduce: handle the late_init case

Message ID 20220108004912.3820176-6-sstabellini@kernel.org (mailing list archive)
State Superseded
Headers show
Series dom0less PV drivers | expand

Commit Message

Stefano Stabellini Jan. 8, 2022, 12:49 a.m. UTC 
From: Luca Miccio <lucmiccio@gmail.com>

If the function is called with late_init set then also notify the domain
using the xenstore event channel.

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: wl@xen.org
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: julien@xen.org
---
 tools/xenstore/xenstored_domain.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Marek Marczykowski-Górecki Jan. 8, 2022, 2:39 a.m. UTC | #1 
On Fri, Jan 07, 2022 at 04:49:11PM -0800, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> If the function is called with late_init set then also notify the domain
> using the xenstore event channel.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: wl@xen.org
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: julien@xen.org
> ---
>  tools/xenstore/xenstored_domain.c | 15 ++++++++++-----

Isn't the same necessary in oxenstored too? Otherwise, I think it needs
some explicit documentation, that late PV with dom0less requires
cxenstored.
Julien Grall Jan. 8, 2022, 3:54 a.m. UTC | #2 
Hi Stefano,

On 08/01/2022 00:49, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> If the function is called with late_init set then also notify the domain
> using the xenstore event channel.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: wl@xen.org
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: julien@xen.org
> ---
>   tools/xenstore/xenstored_domain.c | 15 ++++++++++-----

All the changes to the protocol should be reflected in 
docs/misc/xenstore.txt. However...

>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index d03c7d93a9..17b8021ca8 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -429,7 +429,7 @@ static void domain_conn_reset(struct domain *domain)
>   
>   static struct domain *introduce_domain(const void *ctx,
>   				       unsigned int domid,
> -				       evtchn_port_t port, bool restore)
> +				       evtchn_port_t port, bool restore, bool late_init)
>   {
>   	struct domain *domain;
>   	int rc;
> @@ -461,6 +461,9 @@ static struct domain *introduce_domain(const void *ctx,
>   		/* Now domain belongs to its connection. */
>   		talloc_steal(domain->conn, domain);
>   
> +		if (late_init)
> +			xenevtchn_notify(xce_handle, domain->port);

... I am not convinced the parameter late_init is necessary. I believe 
it would be safe to always raise an event channel because a domain 
should be resilient (event channel are just to say "Please check the 
status", there are no data carried).

If you really need late_init, then it should be made optional to avoid 
breaking existing user of Xenstore (IHMO the protocol is stable and 
should be backward compatible).

Cheers,
Stefano Stabellini Jan. 10, 2022, 10:48 p.m. UTC | #3 
On Sat, 8 Jan 2022, Julien Grall wrote:
> Hi Stefano,
> 
> On 08/01/2022 00:49, Stefano Stabellini wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> > 
> > If the function is called with late_init set then also notify the domain
> > using the xenstore event channel.
> > 
> > Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > CC: wl@xen.org
> > CC: Anthony PERARD <anthony.perard@citrix.com>
> > CC: Juergen Gross <jgross@suse.com>
> > CC: julien@xen.org
> > ---
> >   tools/xenstore/xenstored_domain.c | 15 ++++++++++-----
> 
> All the changes to the protocol should be reflected in docs/misc/xenstore.txt.
> However...
> 
> >   1 file changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/xenstore/xenstored_domain.c
> > b/tools/xenstore/xenstored_domain.c
> > index d03c7d93a9..17b8021ca8 100644
> > --- a/tools/xenstore/xenstored_domain.c
> > +++ b/tools/xenstore/xenstored_domain.c
> > @@ -429,7 +429,7 @@ static void domain_conn_reset(struct domain *domain)
> >     static struct domain *introduce_domain(const void *ctx,
> >   				       unsigned int domid,
> > -				       evtchn_port_t port, bool restore)
> > +				       evtchn_port_t port, bool restore, bool
> > late_init)
> >   {
> >   	struct domain *domain;
> >   	int rc;
> > @@ -461,6 +461,9 @@ static struct domain *introduce_domain(const void *ctx,
> >   		/* Now domain belongs to its connection. */
> >   		talloc_steal(domain->conn, domain);
> >   +		if (late_init)
> > +			xenevtchn_notify(xce_handle, domain->port);
> 
> ... I am not convinced the parameter late_init is necessary. I believe it
> would be safe to always raise an event channel because a domain should be
> resilient (event channel are just to say "Please check the status", there are
> no data carried).

This is a fantastic idea. I gave it a quick try and it seems to work
fine. If everything checks out I'll make the change in the next version
and drop late_init (the new parameter to xs_introduce_domain) completely.


> If you really need late_init, then it should be made optional to avoid
> breaking existing user of Xenstore (IHMO the protocol is stable and should be
> backward compatible).
Stefano Stabellini Jan. 13, 2022, 12:51 a.m. UTC | #4 
On Sat, 8 Jan 2022, Marek Marczykowski-Górecki wrote:
> On Fri, Jan 07, 2022 at 04:49:11PM -0800, Stefano Stabellini wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> > 
> > If the function is called with late_init set then also notify the domain
> > using the xenstore event channel.
> > 
> > Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > CC: wl@xen.org
> > CC: Anthony PERARD <anthony.perard@citrix.com>
> > CC: Juergen Gross <jgross@suse.com>
> > CC: julien@xen.org
> > ---
> >  tools/xenstore/xenstored_domain.c | 15 ++++++++++-----
> 
> Isn't the same necessary in oxenstored too? Otherwise, I think it needs
> some explicit documentation, that late PV with dom0less requires
> cxenstored.

You have a point here, thanks for the heads up. Given my lack of OCaml
skills, I'll re-submit without the oxenstored part. Once we are settled
on the cxenstored changes, I'll attempt to change oxenstored too.
diff mbox series

Patch

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index d03c7d93a9..17b8021ca8 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -429,7 +429,7 @@  static void domain_conn_reset(struct domain *domain)
 
 static struct domain *introduce_domain(const void *ctx,
 				       unsigned int domid,
-				       evtchn_port_t port, bool restore)
+				       evtchn_port_t port, bool restore, bool late_init)
 {
 	struct domain *domain;
 	int rc;
@@ -461,6 +461,9 @@  static struct domain *introduce_domain(const void *ctx,
 		/* Now domain belongs to its connection. */
 		talloc_steal(domain->conn, domain);
 
+		if (late_init)
+			xenevtchn_notify(xce_handle, domain->port);
+
 		if (!is_master_domain && !restore)
 			fire_watches(NULL, ctx, "@introduceDomain", NULL,
 				     false, NULL);
@@ -479,9 +482,10 @@  static struct domain *introduce_domain(const void *ctx,
 int do_introduce(struct connection *conn, struct buffered_data *in)
 {
 	struct domain *domain;
-	char *vec[3];
+	char *vec[4];
 	unsigned int domid;
 	evtchn_port_t port;
+	bool late_init;
 
 	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec))
 		return EINVAL;
@@ -489,12 +493,13 @@  int do_introduce(struct connection *conn, struct buffered_data *in)
 	domid = atoi(vec[0]);
 	/* Ignore the gfn, we don't need it. */
 	port = atoi(vec[2]);
+	late_init = atoi(vec[3]);
 
 	/* Sanity check args. */
 	if (port <= 0)
 		return EINVAL;
 
-	domain = introduce_domain(in, domid, port, false);
+	domain = introduce_domain(in, domid, port, false, late_init);
 	if (!domain)
 		return errno;
 
@@ -723,7 +728,7 @@  void dom0_init(void)
 	if (port == -1)
 		barf_perror("Failed to initialize dom0 port");
 
-	dom0 = introduce_domain(NULL, xenbus_master_domid(), port, false);
+	dom0 = introduce_domain(NULL, xenbus_master_domid(), port, false, false);
 	if (!dom0)
 		barf_perror("Failed to initialize dom0");
 
@@ -1292,7 +1297,7 @@  void read_state_connection(const void *ctx, const void *state)
 #endif
 	} else {
 		domain = introduce_domain(ctx, sc->spec.ring.domid,
-					  sc->spec.ring.evtchn, true);
+					  sc->spec.ring.evtchn, true, false);
 		if (!domain)
 			barf("domain allocation error");