| Message ID | 20190311180216.18811-3-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Xen stubdom support | expand |
> -----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
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
> -----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
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;
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(-)