Re: [PATCH] tools/xenstore: don't store domU's mfn of ring page in xensotred

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

Hi Juergen,

On Tue, 28 Apr 2020 at 16:53, Juergen Gross <jgross@xxxxxxxx> wrote:
>
> The XS_INTRODUCE command has two parameters: the mfn (or better: gfn)
> of the domain's xenstore ring page and the event channel of the
> domain for communicating with Xenstore.
>
> The gfn is not really needed. It is stored in the per-domain struct
> in xenstored and in case of another XS_INTRODUCE for the domain it
> is tested to match the original value. If it doesn't match the
> command is aborted via EINVAL.
>
> Today there shouldn't be multiple XS_INTRODUCE requests for the same
> domain issued, so the mfn/gfn can just be ignored and multiple
> XS_INTRODUCE commands can be rejected without testing the mfn/gfn.

So there is a comment in the else part:

/* Use XS_INTRODUCE for recreating the xenbus event-channel. */

>From the commit message this is not entirely clear why we want to
prevent recreating the event-channel. Can you expand it?

>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>  tools/xenstore/xenstored_domain.c | 47 
> ++++++++++++++++-----------------------
>  1 file changed, 19 insertions(+), 28 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_domain.c 
> b/tools/xenstore/xenstored_domain.c
> index 5858185211..17328f9fc9 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -369,7 +369,6 @@ int do_introduce(struct connection *conn, struct 
> buffered_data *in)
>         struct domain *domain;
>         char *vec[3];
>         unsigned int domid;
> -       unsigned long mfn;
>         evtchn_port_t port;
>         int rc;
>         struct xenstore_domain_interface *interface;
> @@ -381,7 +380,7 @@ int do_introduce(struct connection *conn, struct 
> buffered_data *in)
>                 return EACCES;
>
>         domid = atoi(vec[0]);
> -       mfn = atol(vec[1]);
> +       /* Ignore the mfn, we don't need it. */

s/mfn/GFN/

>         port = atoi(vec[2]);
>
>         /* Sanity check args. */
> @@ -390,34 +389,26 @@ int do_introduce(struct connection *conn, struct 
> buffered_data *in)
>
>         domain = find_domain_by_domid(domid);
>
> -       if (domain == NULL) {
> -               interface = map_interface(domid);
> -               if (!interface)
> -                       return errno;
> -               /* Hang domain off "in" until we're finished. */
> -               domain = new_domain(in, domid, port);
> -               if (!domain) {
> -                       rc = errno;
> -                       unmap_interface(interface);
> -                       return rc;
> -               }
> -               domain->interface = interface;
> -               domain->mfn = mfn;

AFAICT domain->mfn is not used anymore, so could we remove the field?

Cheers,