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

[Jürgen Groß <jgross@xxxxxxxx>]

On 28.04.20 22:55, Julien Grall wrote:
> 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?
Recreating the event channel would be fine, but I don't see why it
would ever be needed. And XS_INTRODUCE is called only at domain creation
time today, so there is obviously no need for repeating this call.

Maybe the idea was to do this after sending a XS_RESUME command, which
isn't used anywhere in Xen and is another part of Xenstore which doesn't
make any sense today.

> > 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/
Okay, then I should probably change the parameter description, too.

> >          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?
Oh, yes, I missed that.


Juergen