Xen stubdom support

[Qemu-devel] [PATCH 2/6] xen: Move xenstore initialization to common location

Posted by Jason Andryuk 6 years, 11 months ago

For the xen stubdom case, we'll want xenstore initialized, but we'll
want to skip the rest of xen_be_init.  Move the initialization to
xen_hvm_init so we can conditionalize calling xen_be_init.

xs_domain_open() is deprecated for xs_open(0), so make the replacement
as well.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 hw/i386/xen/xen-hvm.c       | 8 ++++++++
 hw/xen/xen-legacy-backend.c | 8 --------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 2939122e7c..c20c4b27f6 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1487,6 +1487,14 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 
     xen_bus_init();
 
+    xenstore = xs_open(0);
+    if (!xenstore) {
+        error_report("Can't connect to xenstored");
+        goto err;
+    }
+
+    qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
+
     /* Initialize backend core & drivers */
     if (xen_be_init() != 0) {
         error_report("xen backend core setup failed");
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 36fd1e9b09..bdf2fa917f 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -683,14 +683,6 @@ int xen_be_init(void)
 {
     xengnttab_handle *gnttabdev;
 
-    xenstore = xs_daemon_open();
-    if (!xenstore) {
-        xen_pv_printf(NULL, 0, "can't connect to xenstored\n");
-        return -1;
-    }
-
-    qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
-
     if (xen_xc == NULL || xen_fmem == NULL) {
         /* Check if xen_init() have been called */
         goto err;
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/6] xen: Move xenstore initialization to common location

Posted by Paul Durrant 6 years, 11 months ago

> -----Original Message-----
> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> Sent: 11 March 2019 18:02
> To: qemu-devel@nongnu.org
> Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Jason Andryuk
> <jandryuk@gmail.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> <anthony.perard@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>;
> Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Subject: [PATCH 2/6] xen: Move xenstore initialization to common location
> 
> For the xen stubdom case, we'll want xenstore initialized, but we'll
> want to skip the rest of xen_be_init.  Move the initialization to
> xen_hvm_init so we can conditionalize calling xen_be_init.
> 
> xs_domain_open() is deprecated for xs_open(0), so make the replacement
> as well.

Can you elaborate as to why you need to do this when the code at the top of xen_hvm_init() already opens xenstore for its own purposes, and AFAICT xenstore_update() is only needed if QEMU is implementing a PV backend?

  Paul

> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
>  hw/i386/xen/xen-hvm.c       | 8 ++++++++
>  hw/xen/xen-legacy-backend.c | 8 --------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index 2939122e7c..c20c4b27f6 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -1487,6 +1487,14 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
> 
>      xen_bus_init();
> 
> +    xenstore = xs_open(0);
> +    if (!xenstore) {
> +        error_report("Can't connect to xenstored");
> +        goto err;
> +    }
> +
> +    qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
> +
>      /* Initialize backend core & drivers */
>      if (xen_be_init() != 0) {
>          error_report("xen backend core setup failed");
> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> index 36fd1e9b09..bdf2fa917f 100644
> --- a/hw/xen/xen-legacy-backend.c
> +++ b/hw/xen/xen-legacy-backend.c
> @@ -683,14 +683,6 @@ int xen_be_init(void)
>  {
>      xengnttab_handle *gnttabdev;
> 
> -    xenstore = xs_daemon_open();
> -    if (!xenstore) {
> -        xen_pv_printf(NULL, 0, "can't connect to xenstored\n");
> -        return -1;
> -    }
> -
> -    qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
> -
>      if (xen_xc == NULL || xen_fmem == NULL) {
>          /* Check if xen_init() have been called */
>          goto err;
> --
> 2.20.1

Re: [Qemu-devel] [PATCH 2/6] xen: Move xenstore initialization to common location

Posted by Jason Andryuk 6 years, 11 months ago

On Wed, Mar 13, 2019 at 11:01 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
>
> > -----Original Message-----
> > From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > Sent: 11 March 2019 18:02
> > To: qemu-devel@nongnu.org
> > Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Jason Andryuk
> > <jandryuk@gmail.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > <anthony.perard@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Paolo Bonzini
> > <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>;
> > Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > Subject: [PATCH 2/6] xen: Move xenstore initialization to common location
> >
> > For the xen stubdom case, we'll want xenstore initialized, but we'll
> > want to skip the rest of xen_be_init.  Move the initialization to
> > xen_hvm_init so we can conditionalize calling xen_be_init.
> >
> > xs_domain_open() is deprecated for xs_open(0), so make the replacement
> > as well.
>
> Can you elaborate as to why you need to do this when the code at the top of xen_hvm_init() already opens xenstore for its own purposes, and AFAICT xenstore_update() is only needed if QEMU is implementing a PV backend?
>
>

Hi, Paul.  Thanks for reviewing.

I think you are right, that this basically shouldn't be needed if PV
backends are disabled.  This patch came out of OpenXT, where it is
needed for some out-of-tree patches.  But that doesn't make it
suitable for upstreaming.

However, while reviewing, it looks like the xen accelerator in
hw/xen/xen-common.c:xen_init() registers xen_change_state_handler().
xen_change_state_handler() uses the global xenstore handle and will
exit(1) if NULL.  I'm not sure how to get the XenIOState xenstore
handle over to the accelerator's xen_init.  Outside of that, I think
you are correct that xenstore_update doesn't need to be run when PV
backends are disabled.

Thanks,
Jason

Re: [Qemu-devel] [PATCH 2/6] xen: Move xenstore initialization to common location

Posted by Paul Durrant 6 years, 11 months ago

> -----Original Message-----
> From: Jason Andryuk [mailto:jandryuk@gmail.com]
> Sent: 13 March 2019 18:12
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Stefano
> Stabellini <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>;
> Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Subject: Re: [PATCH 2/6] xen: Move xenstore initialization to common location
> 
> On Wed, Mar 13, 2019 at 11:01 AM Paul Durrant <Paul.Durrant@citrix.com> wrote:
> >
> > > -----Original Message-----
> > > From: Jason Andryuk [mailto:jandryuk@gmail.com]
> > > Sent: 11 March 2019 18:02
> > > To: qemu-devel@nongnu.org
> > > Cc: xen-devel@lists.xenproject.org; marmarek@invisiblethingslab.com; Jason Andryuk
> > > <jandryuk@gmail.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard
> > > <anthony.perard@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>; Richard Henderson <rth@twiddle.net>; Eduardo Habkost <ehabkost@redhat.com>;
> > > Michael S. Tsirkin <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > Subject: [PATCH 2/6] xen: Move xenstore initialization to common location
> > >
> > > For the xen stubdom case, we'll want xenstore initialized, but we'll
> > > want to skip the rest of xen_be_init.  Move the initialization to
> > > xen_hvm_init so we can conditionalize calling xen_be_init.
> > >
> > > xs_domain_open() is deprecated for xs_open(0), so make the replacement
> > > as well.
> >
> > Can you elaborate as to why you need to do this when the code at the top of xen_hvm_init() already
> opens xenstore for its own purposes, and AFAICT xenstore_update() is only needed if QEMU is
> implementing a PV backend?
> >
> >
> 
> Hi, Paul.  Thanks for reviewing.
> 
> I think you are right, that this basically shouldn't be needed if PV
> backends are disabled.  This patch came out of OpenXT, where it is
> needed for some out-of-tree patches.  But that doesn't make it
> suitable for upstreaming.
> 
> However, while reviewing, it looks like the xen accelerator in
> hw/xen/xen-common.c:xen_init() registers xen_change_state_handler().
> xen_change_state_handler() uses the global xenstore handle and will
> exit(1) if NULL.

I see it yes. TBH signalling state via xenstore should go away as it is incompatible with deprivileging, and I think Anthony might have some patches for that? In the meantime I suggest just doing a local xs_open(0) in that function.

> I'm not sure how to get the XenIOState xenstore
> handle over to the accelerator's xen_init.

That would not be appropriate as the machine type may not be xenfv and hence xen_hvm_init() may not have been called.

  Paul

>  Outside of that, I think
> you are correct that xenstore_update doesn't need to be run when PV
> backends are disabled.
> 
> Thanks,
> Jason