| Message ID | 1445441038-25903-10-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 21 Oct 2015, Ian Campbell wrote: > Until the previous patch this relied on xc_fd(), which was only > implemented for Xen 4.0 and earlier. > > Given this wasn't working since Xen 4.0 I have marked this as disabled > by default. > > Removing this support drops the use of a bunch of symbols from > libxenctrl, specifically: > > - xc_domain_create > - xc_domain_destroy > - xc_domain_getinfo > - xc_domain_max_vcpus > - xc_domain_setmaxmem > - xc_domain_unpause > - xc_evtchn_alloc_unbound > - xc_linux_build > > This is another step towards only using Xen libraries which provide a > stable inteface. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > v4: Fixed all checkpatch errors. > Disabled by default. > --- > configure | 17 +++++++++++++++++ > hw/xenpv/Makefile.objs | 4 +++- > hw/xenpv/xen_machine_pv.c | 14 ++++++++++---- > 3 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/configure b/configure > index fe0a39d..9eab587 100755 > --- a/configure > +++ b/configure > @@ -910,6 +910,10 @@ for opt do > ;; > --enable-xen-pci-passthrough) xen_pci_passthrough="yes" > ;; > + --disable-xen-pv-domain-build) xen_pv_domain_build="no" > + ;; > + --enable-xen-pv-domain-build) xen_pv_domain_build="yes" > + ;; > --disable-brlapi) brlapi="no" > ;; > --enable-brlapi) brlapi="yes" > @@ -2113,6 +2117,15 @@ if test "$xen_pci_passthrough" != "no"; then > fi > fi > > +if test "$xen_pv_domain_build" != "no"; then > + if test "$xen_pv_domain_build" = "yes" && > + test "$xen" != "yes"; then > + error_exit "User requested Xen PV domain builder support" \ > + "which requires Xen support." > + fi > + xen_pv_domain_build=no > +fi Can we simplify this to: if test "$xen_pv_domain_build" = "yes" && test "$xen" != "yes"; then error_exit "User requested Xen PV domain builder support" \ "which requires Xen support." fi fi and move xen_pv_domain_build=no at the beginning of the file? > ########################################## > # libtool probe > > @@ -4393,6 +4406,7 @@ fi > echo "xen support $xen" > if test "$xen" = "yes" ; then > echo "xen ctrl version $xen_ctrl_version" > + echo "pv dom build $xen_pv_domain_build" > fi > echo "brlapi support $brlapi" > echo "bluez support $bluez" > @@ -4725,6 +4739,9 @@ fi > if test "$xen" = "yes" ; then > echo "CONFIG_XEN_BACKEND=y" >> $config_host_mak > echo "CONFIG_XEN_CTRL_INTERFACE_VERSION=$xen_ctrl_version" >> $config_host_mak > + if test "$xen_pv_domain_build" = "yes" ; then > + echo "CONFIG_XEN_PV_DOMAIN_BUILD=y" >> $config_host_mak > + fi > fi > if test "$linux_aio" = "yes" ; then > echo "CONFIG_LINUX_AIO=y" >> $config_host_mak > diff --git a/hw/xenpv/Makefile.objs b/hw/xenpv/Makefile.objs > index 49f6e9e..bbf5873 100644 > --- a/hw/xenpv/Makefile.objs > +++ b/hw/xenpv/Makefile.objs > @@ -1,2 +1,4 @@ > # Xen PV machine support > -obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o > +obj-$(CONFIG_XEN) += xen_machine_pv.o > +# Xen PV machine builder support > +obj-$(CONFIG_XEN_PV_DOMAIN_BUILD) += xen_domainbuild.o > diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c > index 2e545d2..e5b3698 100644 > --- a/hw/xenpv/xen_machine_pv.c > +++ b/hw/xenpv/xen_machine_pv.c > @@ -30,9 +30,6 @@ > > static void xen_init_pv(MachineState *machine) > { > - const char *kernel_filename = machine->kernel_filename; > - const char *kernel_cmdline = machine->kernel_cmdline; > - const char *initrd_filename = machine->initrd_filename; > DriveInfo *dinfo; > int i; > > @@ -46,13 +43,22 @@ static void xen_init_pv(MachineState *machine) > case XEN_ATTACH: > /* nothing to do, xend handles everything */ > break; > - case XEN_CREATE: > + case XEN_CREATE: { > +#ifdef CONFIG_XEN_PV_DOMAIN_BUILD > + const char *kernel_filename = machine->kernel_filename; > + const char *kernel_cmdline = machine->kernel_cmdline; > + const char *initrd_filename = machine->initrd_filename; > if (xen_domain_build_pv(kernel_filename, initrd_filename, > kernel_cmdline) < 0) { > fprintf(stderr, "xen pv domain creation failed\n"); > exit(1); > } > +#else > + fprintf(stderr, "xen pv domain creation not supported\n"); > + exit(1); > +#endif > break; > + } > case XEN_EMULATE: > fprintf(stderr, "xen emulation not implemented (yet)\n"); > exit(1); Please add a default case with an error and place the XEN_CREATE entirely within the ifdef CONFIG_XEN_PV_DOMAIN_BUILD.
On Fri, 2015-10-23 at 12:12 +0100, Stefano Stabellini wrote: > @@ -2113,6 +2117,15 @@ if test "$xen_pci_passthrough" != "no"; then > > fi > > fi > > > > +if test "$xen_pv_domain_build" != "no"; then > > + if test "$xen_pv_domain_build" = "yes" && > > + test "$xen" != "yes"; then > > + error_exit "User requested Xen PV domain builder support" \ > > + "which requires Xen support." > > + fi > > + xen_pv_domain_build=no > > +fi > > Can we simplify this to: > > if test "$xen_pv_domain_build" = "yes" && > test "$xen" != "yes"; then > error_exit "User requested Xen PV domain builder support" \ > "which requires Xen support." > fi > fi > > and move xen_pv_domain_build=no at the beginning of the file? I think so, I hadn't noticed the precedent for doing so further up in the file. Just to check, is this still your preference after my earlier reply-to-self explaining why the code above is utter rubbish and proposing a different simplified version? @@ -46,13 +43,22 @@ static void xen_init_pv(MachineState *machine) > > case XEN_ATTACH: > > /* nothing to do, xend handles everything */ > > break; > > - case XEN_CREATE: > > + case XEN_CREATE: { > > +#ifdef CONFIG_XEN_PV_DOMAIN_BUILD > > + const char *kernel_filename = machine->kernel_filename; > > + const char *kernel_cmdline = machine->kernel_cmdline; > > + const char *initrd_filename = machine->initrd_filename; > > if (xen_domain_build_pv(kernel_filename, initrd_filename, > > kernel_cmdline) < 0) { > > fprintf(stderr, "xen pv domain creation failed\n"); > > exit(1); > > } > > +#else > > + fprintf(stderr, "xen pv domain creation not supported\n"); > > + exit(1); > > +#endif > > break; > > + } > > case XEN_EMULATE: > > fprintf(stderr, "xen emulation not implemented (yet)\n"); > > exit(1); > > Please add a default case with an error and place the XEN_CREATE > entirely within the ifdef CONFIG_XEN_PV_DOMAIN_BUILD. Will do.
On Fri, 23 Oct 2015, Ian Campbell wrote: > On Fri, 2015-10-23 at 12:12 +0100, Stefano Stabellini wrote: > > @@ -2113,6 +2117,15 @@ if test "$xen_pci_passthrough" != "no"; then > > > fi > > > fi > > > > > > +if test "$xen_pv_domain_build" != "no"; then > > > + if test "$xen_pv_domain_build" = "yes" && > > > + test "$xen" != "yes"; then > > > + error_exit "User requested Xen PV domain builder support" \ > > > + "which requires Xen support." > > > + fi > > > + xen_pv_domain_build=no > > > +fi > > > > Can we simplify this to: > > > > if test "$xen_pv_domain_build" = "yes" && > > test "$xen" != "yes"; then > > error_exit "User requested Xen PV domain builder support" \ > > "which requires Xen support." > > fi > > fi > > > > and move xen_pv_domain_build=no at the beginning of the file? > > I think so, I hadn't noticed the precedent for doing so further up in the > file. > > Just to check, is this still your preference after my earlier reply-to-self > explaining why the code above is utter rubbish and proposing a different > simplified version? The simplified check is OK, but I would still prefer if you moved xen_pv_domain_build=no at the beginning of the file. > @@ -46,13 +43,22 @@ static void xen_init_pv(MachineState *machine) > > > case XEN_ATTACH: > > > /* nothing to do, xend handles everything */ > > > break; > > > - case XEN_CREATE: > > > + case XEN_CREATE: { > > > +#ifdef CONFIG_XEN_PV_DOMAIN_BUILD > > > + const char *kernel_filename = machine->kernel_filename; > > > + const char *kernel_cmdline = machine->kernel_cmdline; > > > + const char *initrd_filename = machine->initrd_filename; > > > if (xen_domain_build_pv(kernel_filename, initrd_filename, > > > kernel_cmdline) < 0) { > > > fprintf(stderr, "xen pv domain creation failed\n"); > > > exit(1); > > > } > > > +#else > > > + fprintf(stderr, "xen pv domain creation not supported\n"); > > > + exit(1); > > > +#endif > > > break; > > > + } > > > case XEN_EMULATE: > > > fprintf(stderr, "xen emulation not implemented (yet)\n"); > > > exit(1); > > > > Please add a default case with an error and place the XEN_CREATE > > entirely within the ifdef CONFIG_XEN_PV_DOMAIN_BUILD. > > Will do. >
On Fri, 2015-10-23 at 12:35 +0100, Stefano Stabellini wrote: > On Fri, 23 Oct 2015, Ian Campbell wrote: > > On Fri, 2015-10-23 at 12:12 +0100, Stefano Stabellini wrote: > > > @@ -2113,6 +2117,15 @@ if test "$xen_pci_passthrough" != "no"; then > > > > fi > > > > fi > > > > > > > > +if test "$xen_pv_domain_build" != "no"; then > > > > + if test "$xen_pv_domain_build" = "yes" && > > > > + test "$xen" != "yes"; then > > > > + error_exit "User requested Xen PV domain builder support" \ > > > > + "which requires Xen support." > > > > + fi > > > > + xen_pv_domain_build=no > > > > +fi > > > > > > Can we simplify this to: > > > > > > if test "$xen_pv_domain_build" = "yes" && > > > test "$xen" != "yes"; then > > > error_exit "User requested Xen PV domain builder support" \ > > > "which requires Xen support." > > > fi > > > fi > > > > > > and move xen_pv_domain_build=no at the beginning of the file? > > > > I think so, I hadn't noticed the precedent for doing so further up in > > the > > file. > > > > Just to check, is this still your preference after my earlier reply-to > > -self > > explaining why the code above is utter rubbish and proposing a > > different > > simplified version? > > The simplified check is OK, but I would still prefer if you moved > xen_pv_domain_build=no at the beginning of the file. Ack, I'll do that then. > > > > @@ -46,13 +43,22 @@ static void xen_init_pv(MachineState *machine) > > > > case XEN_ATTACH: > > > > /* nothing to do, xend handles everything */ > > > > break; > > > > - case XEN_CREATE: > > > > + case XEN_CREATE: { > > > > +#ifdef CONFIG_XEN_PV_DOMAIN_BUILD > > > > + const char *kernel_filename = machine->kernel_filename; > > > > + const char *kernel_cmdline = machine->kernel_cmdline; > > > > + const char *initrd_filename = machine->initrd_filename; > > > > if (xen_domain_build_pv(kernel_filename, initrd_filename, > > > > kernel_cmdline) < 0) { > > > > fprintf(stderr, "xen pv domain creation failed\n"); > > > > exit(1); > > > > } > > > > +#else > > > > + fprintf(stderr, "xen pv domain creation not supported\n"); > > > > + exit(1); > > > > +#endif > > > > break; > > > > + } > > > > case XEN_EMULATE: > > > > fprintf(stderr, "xen emulation not implemented (yet)\n"); > > > > exit(1); > > > > > > Please add a default case with an error and place the XEN_CREATE > > > entirely within the ifdef CONFIG_XEN_PV_DOMAIN_BUILD. > > > > Will do. > >
diff --git a/configure b/configure index fe0a39d..9eab587 100755 --- a/configure +++ b/configure @@ -910,6 +910,10 @@ for opt do ;; --enable-xen-pci-passthrough) xen_pci_passthrough="yes" ;; + --disable-xen-pv-domain-build) xen_pv_domain_build="no" + ;; + --enable-xen-pv-domain-build) xen_pv_domain_build="yes" + ;; --disable-brlapi) brlapi="no" ;; --enable-brlapi) brlapi="yes" @@ -2113,6 +2117,15 @@ if test "$xen_pci_passthrough" != "no"; then fi fi +if test "$xen_pv_domain_build" != "no"; then + if test "$xen_pv_domain_build" = "yes" && + test "$xen" != "yes"; then + error_exit "User requested Xen PV domain builder support" \ + "which requires Xen support." + fi + xen_pv_domain_build=no +fi + ########################################## # libtool probe @@ -4393,6 +4406,7 @@ fi echo "xen support $xen" if test "$xen" = "yes" ; then echo "xen ctrl version $xen_ctrl_version" + echo "pv dom build $xen_pv_domain_build" fi echo "brlapi support $brlapi" echo "bluez support $bluez" @@ -4725,6 +4739,9 @@ fi if test "$xen" = "yes" ; then echo "CONFIG_XEN_BACKEND=y" >> $config_host_mak echo "CONFIG_XEN_CTRL_INTERFACE_VERSION=$xen_ctrl_version" >> $config_host_mak + if test "$xen_pv_domain_build" = "yes" ; then + echo "CONFIG_XEN_PV_DOMAIN_BUILD=y" >> $config_host_mak + fi fi if test "$linux_aio" = "yes" ; then echo "CONFIG_LINUX_AIO=y" >> $config_host_mak diff --git a/hw/xenpv/Makefile.objs b/hw/xenpv/Makefile.objs index 49f6e9e..bbf5873 100644 --- a/hw/xenpv/Makefile.objs +++ b/hw/xenpv/Makefile.objs @@ -1,2 +1,4 @@ # Xen PV machine support -obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o +obj-$(CONFIG_XEN) += xen_machine_pv.o +# Xen PV machine builder support +obj-$(CONFIG_XEN_PV_DOMAIN_BUILD) += xen_domainbuild.o diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c index 2e545d2..e5b3698 100644 --- a/hw/xenpv/xen_machine_pv.c +++ b/hw/xenpv/xen_machine_pv.c @@ -30,9 +30,6 @@ static void xen_init_pv(MachineState *machine) { - const char *kernel_filename = machine->kernel_filename; - const char *kernel_cmdline = machine->kernel_cmdline; - const char *initrd_filename = machine->initrd_filename; DriveInfo *dinfo; int i; @@ -46,13 +43,22 @@ static void xen_init_pv(MachineState *machine) case XEN_ATTACH: /* nothing to do, xend handles everything */ break; - case XEN_CREATE: + case XEN_CREATE: { +#ifdef CONFIG_XEN_PV_DOMAIN_BUILD + const char *kernel_filename = machine->kernel_filename; + const char *kernel_cmdline = machine->kernel_cmdline; + const char *initrd_filename = machine->initrd_filename; if (xen_domain_build_pv(kernel_filename, initrd_filename, kernel_cmdline) < 0) { fprintf(stderr, "xen pv domain creation failed\n"); exit(1); } +#else + fprintf(stderr, "xen pv domain creation not supported\n"); + exit(1); +#endif break; + } case XEN_EMULATE: fprintf(stderr, "xen emulation not implemented (yet)\n"); exit(1);
Until the previous patch this relied on xc_fd(), which was only implemented for Xen 4.0 and earlier. Given this wasn't working since Xen 4.0 I have marked this as disabled by default. Removing this support drops the use of a bunch of symbols from libxenctrl, specifically: - xc_domain_create - xc_domain_destroy - xc_domain_getinfo - xc_domain_max_vcpus - xc_domain_setmaxmem - xc_domain_unpause - xc_evtchn_alloc_unbound - xc_linux_build This is another step towards only using Xen libraries which provide a stable inteface. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- v4: Fixed all checkpatch errors. Disabled by default. --- configure | 17 +++++++++++++++++ hw/xenpv/Makefile.objs | 4 +++- hw/xenpv/xen_machine_pv.c | 14 ++++++++++---- 3 files changed, 30 insertions(+), 5 deletions(-)