[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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 
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. */


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.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.