Re: [PATCH v6 5/7] xenstored: send an evtchn notification on introduce_domain

[Julien Grall <julien@xxxxxxx>]

Hi,

On 05/05/2022 01:16, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@xxxxxxxxx>
>
> When xs_introduce_domain is called, send out a notification on the
> xenstore event channel so that any (dom0less) domain waiting for the
> xenstore interface to be ready can continue with the initialization.
> Before sending the notification, clear XENSTORE_RECONNECTING.
>
> The extra notification is harmless for domains that don't require it.
>
> In xs_wire.h update the commment on top of XENSTORE_RECONNECTING to
> generalize its meaning to suit the dom0less use-case better.
>
> Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> CC: jgross@xxxxxxxx
> CC: anthony.perard@xxxxxxxxxx
> CC: wl@xxxxxxx
> ---
> If you have better suggestions for the wording in xs_wire.h please
> suggest!
>
>
> Changes in v6:
> - use XENSTORE_CONNECTED instead of 0x0
> - update xs_wire.h
>
> Changes in v5:
> - reset XS_CONNECTION_STATE_RECONNECTING before notifying the domU
>
> Changes in v2:
> - drop the new late_init parameter
> ---
>   tools/xenstore/xenstored_domain.c | 4 ++++
>   xen/include/public/io/xs_wire.h   | 2 +-
I am not entirely sure this is the right place to mention it. But I 
couldn't find a better one.
The documentation (docs/misc/xenstore-misc.txt) states that the field is 
valid when the server advertised ``Connection State``.
Is there any guarantee the field will be 0 for any previous {C, 
O}xenstored implementation? If not, then I think we need to set the 
feature flag so Linux knows the field can be used.
If yes, then the documentation should be relaxed so an OS knows it can 
safely use the field without checking the feature flag.
>   2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tools/xenstore/xenstored_domain.c 
> b/tools/xenstore/xenstored_domain.c
> index ae065fcbee..6f34af225c 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -493,6 +493,10 @@ static struct domain *introduce_domain(const void *ctx,
>                 /* Now domain belongs to its connection. */
>                 talloc_steal(domain->conn, domain);
>   
> +		/* Notify the domain that xenstore is available */
> +               interface->connection = XENSTORE_CONNECTED;
> +               xenevtchn_notify(xce_handle, domain->port);
> +
>                 if (!is_master_domain && !restore)
>                         fire_watches(NULL, ctx, "@introduceDomain", NULL,
>                                      false, NULL);
> diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
> index 953a0050a3..c1ec7c73e3 100644
> --- a/xen/include/public/io/xs_wire.h
> +++ b/xen/include/public/io/xs_wire.h
> @@ -141,7 +141,7 @@ struct xenstore_domain_interface {
>   
>   /* Valid values for the connection field */
>   #define XENSTORE_CONNECTED 0 /* the steady-state */
> -#define XENSTORE_RECONNECT 1 /* guest has initiated a reconnect */
> +#define XENSTORE_RECONNECT 1 /* reconnect in progress */
The definition in the docs needs to be updated.

>   
>   /* Valid values for the error field */
>   #define XENSTORE_ERROR_NONE    0 /* No error */
Cheers,

--
Julien Grall