Re: [PATCH] cxenstored: wait until after reset to notify dom0less domains

[Juergen Gross <jgross@xxxxxxxx>]

On 14.10.23 01:06, Stefano Stabellini wrote:
> From: George Dunlap <george.dunlap@xxxxxxxxx>
>
> Commit fc2b57c9a ("xenstored: send an evtchn notification on
> introduce_domain") introduced the sending of an event channel to the
> guest when first introduced, so that dom0less domains waiting for the
> connection would know that xenstore was ready to use.
>
> Unfortunately, it was introduced in introduce_domain(), which 1) is
> called by other functions, where such functionality is unneeded, and
> 2) after the main XS_INTRODUCE call, calls domain_conn_reset().  This
> introduces a race condition, whereby if xenstored is delayed, a domain
> can wake up, send messages to the buffer, only to have them deleted by
> xenstore before finishing its processing of the XS_INTRODUCE message.
>
> Move the connect-and-notfy call into do_introduce() instead, after the
> domain_conn_rest(); predicated on the state being in the
> XENSTORE_RECONNECT state.
>
> (We don't need to check for "restoring", since that value is always
> passed as "false" from do_domain_introduce()).
>
> Also take the opportunity to add a missing wmb barrier after resetting
> the indexes of the ring in domain_conn_reset.
>
> This change will also remove an extra event channel notification for
> dom0 (because the notification is now done by do_introduce which is not
> called for dom0.) The extra dom0 event channel notification was only
> introduced by fc2b57c9a and was never present before. It is not needed
> because dom0 is the one to tell xenstored the connection parameters, so
> dom0 has to know that the ring page is setup correctly by the time
> xenstored starts looking at it. It is dom0 that performs the ring page
> init.
>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> CC: jgross@xxxxxxxx
> CC: julien@xxxxxxx
> CC: wl@xxxxxxx
Two nits below, with those fixed:

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>

> ---
>   tools/xenstored/domain.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index a6cd199fdc..f6ef37856c 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c
> @@ -923,6 +923,7 @@ static void domain_conn_reset(struct domain *domain)
>   
>   	domain->interface->req_cons = domain->interface->req_prod = 0;
>         domain->interface->rsp_cons = domain->interface->rsp_prod = 0;
> +       xen_wmb();
>   }
>   
>   /*
> @@ -988,12 +989,6 @@ static struct domain *introduce_domain(const void *ctx,
>                 /* Now domain belongs to its connection. */
>                 talloc_steal(domain->conn, domain);
>   
> -		if (!restore) {
> -                       /* Notify the domain that xenstore is available */
> -                       interface->connection = XENSTORE_CONNECTED;
> -                       xenevtchn_notify(xce_handle, domain->port);
> -               }
> -
>                 if (!is_master_domain && !restore)
>                         fire_special_watches("@introduceDomain");
>         } else {
> @@ -1033,6 +1028,13 @@ int do_introduce(const void *ctx, struct connection 
> *conn,
>   
>   	domain_conn_reset(domain);
>   
> +	if (domain->interface != NULL &&
> +               domain->interface->connection == XENSTORE_RECONNECT) {
Identation should be fixed (both tested conditions at the same position).

> +               /* Notify the domain that xenstore is available */
Please add a "." at the end of the sentence while you are moving this line.

> +               domain->interface->connection = XENSTORE_CONNECTED;
> +               xenevtchn_notify(xce_handle, domain->port);
> +       }
> +
>         send_ack(conn, XS_INTRODUCE);
>   
>   	return 0;

Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature