| Message ID | 1400743250-2315-1-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, May 22, 2014 at 09:20:50AM +0200, Gerd Hoffmann wrote: > Patch hooks up the xen platform device to the default device code we > have in qemu. Two effects: > > (1) The device will not be created in case -nodefaults is specified > on the command line. > (2) Autocreating the device is also turned off in case xen-platform > is added manually via -device. > > With the patch applied you can move the xen-platform device to some > other place with a simple 'qemu -device xen-platform,addr=$slot'. > > Tested-by: Tiejun Chen <tiejun.chen@intel.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Applied, thanks! > --- > hw/i386/pc_piix.c | 2 +- > include/hw/xen/xen.h | 1 + > vl.c | 3 +++ > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index eaf3e61..f987d03 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -385,7 +385,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs *args) > pc_init_pci(args); > > bus = pci_find_primary_bus(); > - if (bus != NULL) { > + if (bus != NULL && default_xenplatform) { > pci_create_simple(bus, -1, "xen-platform"); > } > } > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h > index 85fda3d..b350413 100644 > --- a/include/hw/xen/xen.h > +++ b/include/hw/xen/xen.h > @@ -20,6 +20,7 @@ enum xen_mode { > > extern uint32_t xen_domid; > extern enum xen_mode xen_mode; > +extern int default_xenplatform; > > extern bool xen_allowed; > > diff --git a/vl.c b/vl.c > index 709d8cd..673148e 100644 > --- a/vl.c > +++ b/vl.c > @@ -226,6 +226,7 @@ static int default_floppy = 1; > static int default_cdrom = 1; > static int default_sdcard = 1; > static int default_vga = 1; > +int default_xenplatform = 1; > > static struct { > const char *driver; > @@ -247,6 +248,7 @@ static struct { > { .driver = "isa-cirrus-vga", .flag = &default_vga }, > { .driver = "vmware-svga", .flag = &default_vga }, > { .driver = "qxl-vga", .flag = &default_vga }, > + { .driver = "xen-platform", .flag = &default_xenplatform }, > }; > > static QemuOptsList qemu_rtc_opts = { > @@ -4101,6 +4103,7 @@ int main(int argc, char **argv, char **envp) > default_monitor = 0; > default_net = 0; > default_vga = 0; > + default_xenplatform = 0; > } > > if (is_daemonized()) { > -- > 1.8.3.1
> -----Original Message----- > From: qemu-devel-bounces+tiejun.chen=intel.com@nongnu.org > [mailto:qemu-devel-bounces+tiejun.chen=intel.com@nongnu.org] On Behalf Of > Michael S. Tsirkin > Sent: Thursday, May 22, 2014 3:22 PM > To: Gerd Hoffmann > Cc: qemu-devel@nongnu.org; Anthony Liguori > Subject: Re: [Qemu-devel] [PATCH] xen: make xen-platform a default device > > On Thu, May 22, 2014 at 09:20:50AM +0200, Gerd Hoffmann wrote: > > Patch hooks up the xen platform device to the default device code we > > have in qemu. Two effects: > > > > (1) The device will not be created in case -nodefaults is specified > > on the command line. > > (2) Autocreating the device is also turned off in case xen-platform > > is added manually via -device. > > > > With the patch applied you can move the xen-platform device to some > > other place with a simple 'qemu -device xen-platform,addr=$slot'. > > > > Tested-by: Tiejun Chen <tiejun.chen@intel.com> Sorry, I'm not sure if this works well as Gerd expect. I already reply something online, please take a look at that before apply. Thanks Tiejun > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > Applied, thanks! > > > --- > > hw/i386/pc_piix.c | 2 +- > > include/hw/xen/xen.h | 1 + > > vl.c | 3 +++ > > 3 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index > > eaf3e61..f987d03 100644 > > --- a/hw/i386/pc_piix.c > > +++ b/hw/i386/pc_piix.c > > @@ -385,7 +385,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs > *args) > > pc_init_pci(args); > > > > bus = pci_find_primary_bus(); > > - if (bus != NULL) { > > + if (bus != NULL && default_xenplatform) { > > pci_create_simple(bus, -1, "xen-platform"); > > } > > } > > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h index > > 85fda3d..b350413 100644 > > --- a/include/hw/xen/xen.h > > +++ b/include/hw/xen/xen.h > > @@ -20,6 +20,7 @@ enum xen_mode { > > > > extern uint32_t xen_domid; > > extern enum xen_mode xen_mode; > > +extern int default_xenplatform; > > > > extern bool xen_allowed; > > > > diff --git a/vl.c b/vl.c > > index 709d8cd..673148e 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -226,6 +226,7 @@ static int default_floppy = 1; static int > > default_cdrom = 1; static int default_sdcard = 1; static int > > default_vga = 1; > > +int default_xenplatform = 1; > > > > static struct { > > const char *driver; > > @@ -247,6 +248,7 @@ static struct { > > { .driver = "isa-cirrus-vga", .flag = &default_vga }, > > { .driver = "vmware-svga", .flag = &default_vga }, > > { .driver = "qxl-vga", .flag = &default_vga }, > > + { .driver = "xen-platform", .flag = &default_xenplatform }, > > }; > > > > static QemuOptsList qemu_rtc_opts = { @@ -4101,6 +4103,7 @@ int > > main(int argc, char **argv, char **envp) > > default_monitor = 0; > > default_net = 0; > > default_vga = 0; > > + default_xenplatform = 0; > > } > > > > if (is_daemonized()) { > > -- > > 1.8.3.1
On Thu, May 22, 2014 at 10:57:54AM +0000, Chen, Tiejun wrote: > > -----Original Message----- > > From: qemu-devel-bounces+tiejun.chen=intel.com@nongnu.org > > [mailto:qemu-devel-bounces+tiejun.chen=intel.com@nongnu.org] On Behalf Of > > Michael S. Tsirkin > > Sent: Thursday, May 22, 2014 3:22 PM > > To: Gerd Hoffmann > > Cc: qemu-devel@nongnu.org; Anthony Liguori > > Subject: Re: [Qemu-devel] [PATCH] xen: make xen-platform a default device > > > > On Thu, May 22, 2014 at 09:20:50AM +0200, Gerd Hoffmann wrote: > > > Patch hooks up the xen platform device to the default device code we > > > have in qemu. Two effects: > > > > > > (1) The device will not be created in case -nodefaults is specified > > > on the command line. > > > (2) Autocreating the device is also turned off in case xen-platform > > > is added manually via -device. > > > > > > With the patch applied you can move the xen-platform device to some > > > other place with a simple 'qemu -device xen-platform,addr=$slot'. > > > > > > Tested-by: Tiejun Chen <tiejun.chen@intel.com> > > Sorry, I'm not sure if this works well as Gerd expect. > > I already reply something online, please take a look at that before apply. > > Thanks > Tiejun No worries, I'll wait. > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > > Applied, thanks! > > > > > --- > > > hw/i386/pc_piix.c | 2 +- > > > include/hw/xen/xen.h | 1 + > > > vl.c | 3 +++ > > > 3 files changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index > > > eaf3e61..f987d03 100644 > > > --- a/hw/i386/pc_piix.c > > > +++ b/hw/i386/pc_piix.c > > > @@ -385,7 +385,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs > > *args) > > > pc_init_pci(args); > > > > > > bus = pci_find_primary_bus(); > > > - if (bus != NULL) { > > > + if (bus != NULL && default_xenplatform) { > > > pci_create_simple(bus, -1, "xen-platform"); > > > } > > > } > > > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h index > > > 85fda3d..b350413 100644 > > > --- a/include/hw/xen/xen.h > > > +++ b/include/hw/xen/xen.h > > > @@ -20,6 +20,7 @@ enum xen_mode { > > > > > > extern uint32_t xen_domid; > > > extern enum xen_mode xen_mode; > > > +extern int default_xenplatform; > > > > > > extern bool xen_allowed; > > > > > > diff --git a/vl.c b/vl.c > > > index 709d8cd..673148e 100644 > > > --- a/vl.c > > > +++ b/vl.c > > > @@ -226,6 +226,7 @@ static int default_floppy = 1; static int > > > default_cdrom = 1; static int default_sdcard = 1; static int > > > default_vga = 1; > > > +int default_xenplatform = 1; > > > > > > static struct { > > > const char *driver; > > > @@ -247,6 +248,7 @@ static struct { > > > { .driver = "isa-cirrus-vga", .flag = &default_vga }, > > > { .driver = "vmware-svga", .flag = &default_vga }, > > > { .driver = "qxl-vga", .flag = &default_vga }, > > > + { .driver = "xen-platform", .flag = &default_xenplatform }, > > > }; > > > > > > static QemuOptsList qemu_rtc_opts = { @@ -4101,6 +4103,7 @@ int > > > main(int argc, char **argv, char **envp) > > > default_monitor = 0; > > > default_net = 0; > > > default_vga = 0; > > > + default_xenplatform = 0; > > > } > > > > > > if (is_daemonized()) { > > > -- > > > 1.8.3.1
On Thu, 22 May 2014, Gerd Hoffmann wrote: > Patch hooks up the xen platform device to the default device code we > have in qemu. Two effects: > > (1) The device will not be created in case -nodefaults is specified > on the command line. > (2) Autocreating the device is also turned off in case xen-platform > is added manually via -device. > > With the patch applied you can move the xen-platform device to some > other place with a simple 'qemu -device xen-platform,addr=$slot'. > > Tested-by: Tiejun Chen <tiejun.chen@intel.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Given that libxl always passes -nodefaults to QEMU, this patch is going to effectively disable xen_platform_pci for all Xen users. It is not a good idea. With the patch applied a Xen user would have no way to enable xen_platform_pci except for passing some magic command line runes via device_model_args_hvm. > hw/i386/pc_piix.c | 2 +- > include/hw/xen/xen.h | 1 + > vl.c | 3 +++ > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index eaf3e61..f987d03 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -385,7 +385,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs *args) > pc_init_pci(args); > > bus = pci_find_primary_bus(); > - if (bus != NULL) { > + if (bus != NULL && default_xenplatform) { > pci_create_simple(bus, -1, "xen-platform"); > } > } > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h > index 85fda3d..b350413 100644 > --- a/include/hw/xen/xen.h > +++ b/include/hw/xen/xen.h > @@ -20,6 +20,7 @@ enum xen_mode { > > extern uint32_t xen_domid; > extern enum xen_mode xen_mode; > +extern int default_xenplatform; > > extern bool xen_allowed; > > diff --git a/vl.c b/vl.c > index 709d8cd..673148e 100644 > --- a/vl.c > +++ b/vl.c > @@ -226,6 +226,7 @@ static int default_floppy = 1; > static int default_cdrom = 1; > static int default_sdcard = 1; > static int default_vga = 1; > +int default_xenplatform = 1; > > static struct { > const char *driver; > @@ -247,6 +248,7 @@ static struct { > { .driver = "isa-cirrus-vga", .flag = &default_vga }, > { .driver = "vmware-svga", .flag = &default_vga }, > { .driver = "qxl-vga", .flag = &default_vga }, > + { .driver = "xen-platform", .flag = &default_xenplatform }, > }; > > static QemuOptsList qemu_rtc_opts = { > @@ -4101,6 +4103,7 @@ int main(int argc, char **argv, char **envp) > default_monitor = 0; > default_net = 0; > default_vga = 0; > + default_xenplatform = 0; > } > > if (is_daemonized()) { > -- > 1.8.3.1 > >
On Thu, May 22, 2014 at 01:11:28PM +0100, Stefano Stabellini wrote: > On Thu, 22 May 2014, Gerd Hoffmann wrote: > > Patch hooks up the xen platform device to the default device code we > > have in qemu. Two effects: > > > > (1) The device will not be created in case -nodefaults is specified > > on the command line. > > (2) Autocreating the device is also turned off in case xen-platform > > is added manually via -device. > > > > With the patch applied you can move the xen-platform device to some > > other place with a simple 'qemu -device xen-platform,addr=$slot'. > > > > Tested-by: Tiejun Chen <tiejun.chen@intel.com> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > Given that libxl always passes -nodefaults to QEMU, this patch is going > to effectively disable xen_platform_pci for all Xen users. It is not a > good idea. With the patch applied a Xen user would have no way to enable > xen_platform_pci except for passing some magic command line runes via > device_model_args_hvm. > Yes, it's an unfortunate use of the interface. How about a new machine type for xenfv - that's the only one that's affected, right? It's time xen started versioning qemu hardware anyway. > > hw/i386/pc_piix.c | 2 +- > > include/hw/xen/xen.h | 1 + > > vl.c | 3 +++ > > 3 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > > index eaf3e61..f987d03 100644 > > --- a/hw/i386/pc_piix.c > > +++ b/hw/i386/pc_piix.c > > @@ -385,7 +385,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs *args) > > pc_init_pci(args); > > > > bus = pci_find_primary_bus(); > > - if (bus != NULL) { > > + if (bus != NULL && default_xenplatform) { > > pci_create_simple(bus, -1, "xen-platform"); > > } > > } > > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h > > index 85fda3d..b350413 100644 > > --- a/include/hw/xen/xen.h > > +++ b/include/hw/xen/xen.h > > @@ -20,6 +20,7 @@ enum xen_mode { > > > > extern uint32_t xen_domid; > > extern enum xen_mode xen_mode; > > +extern int default_xenplatform; > > > > extern bool xen_allowed; > > > > diff --git a/vl.c b/vl.c > > index 709d8cd..673148e 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -226,6 +226,7 @@ static int default_floppy = 1; > > static int default_cdrom = 1; > > static int default_sdcard = 1; > > static int default_vga = 1; > > +int default_xenplatform = 1; > > > > static struct { > > const char *driver; > > @@ -247,6 +248,7 @@ static struct { > > { .driver = "isa-cirrus-vga", .flag = &default_vga }, > > { .driver = "vmware-svga", .flag = &default_vga }, > > { .driver = "qxl-vga", .flag = &default_vga }, > > + { .driver = "xen-platform", .flag = &default_xenplatform }, > > }; > > > > static QemuOptsList qemu_rtc_opts = { > > @@ -4101,6 +4103,7 @@ int main(int argc, char **argv, char **envp) > > default_monitor = 0; > > default_net = 0; > > default_vga = 0; > > + default_xenplatform = 0; > > } > > > > if (is_daemonized()) { > > -- > > 1.8.3.1 > > > >
Hi, > Given that libxl always passes -nodefaults to QEMU, this patch is going > to effectively disable xen_platform_pci for all Xen users. It is not a > good idea. With the patch applied a Xen user would have no way to enable > xen_platform_pci except for passing some magic command line runes via > device_model_args_hvm. Indeed. > > @@ -4101,6 +4103,7 @@ int main(int argc, char **argv, char **envp) > > default_monitor = 0; > > default_net = 0; > > default_vga = 0; > > + default_xenplatform = 0; > > } With that chunk removed -nodefaults will have no effect for the xen platform device, but explicitly moving it somewhere else via -device xen-platform,addr=$slot should still work. cheers, Gerd
Il 22/05/2014 14:11, Stefano Stabellini ha scritto: > On Thu, 22 May 2014, Gerd Hoffmann wrote: >> Patch hooks up the xen platform device to the default device code we >> have in qemu. Two effects: >> >> (1) The device will not be created in case -nodefaults is specified >> on the command line. >> (2) Autocreating the device is also turned off in case xen-platform >> is added manually via -device. >> >> With the patch applied you can move the xen-platform device to some >> other place with a simple 'qemu -device xen-platform,addr=$slot'. >> >> Tested-by: Tiejun Chen <tiejun.chen@intel.com> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > Given that libxl always passes -nodefaults to QEMU, this patch is going > to effectively disable xen_platform_pci for all Xen users. It is not a > good idea. With the patch applied a Xen user would have no way to enable > xen_platform_pci except for passing some magic command line runes via > device_model_args_hvm. In fact this code only runs for "-M xenfv". If you use "-M pc", the xen-platform device has to be added manually. Perhaps it would be worthwhile to do the opposite, i.e. add the xen-platform device to "-M pc" if not using -nodefaults. Paolo > >> hw/i386/pc_piix.c | 2 +- >> include/hw/xen/xen.h | 1 + >> vl.c | 3 +++ >> 3 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index eaf3e61..f987d03 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -385,7 +385,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs *args) >> pc_init_pci(args); >> >> bus = pci_find_primary_bus(); >> - if (bus != NULL) { >> + if (bus != NULL && default_xenplatform) { >> pci_create_simple(bus, -1, "xen-platform"); >> } >> } >> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h >> index 85fda3d..b350413 100644 >> --- a/include/hw/xen/xen.h >> +++ b/include/hw/xen/xen.h >> @@ -20,6 +20,7 @@ enum xen_mode { >> >> extern uint32_t xen_domid; >> extern enum xen_mode xen_mode; >> +extern int default_xenplatform; >> >> extern bool xen_allowed; >> >> diff --git a/vl.c b/vl.c >> index 709d8cd..673148e 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -226,6 +226,7 @@ static int default_floppy = 1; >> static int default_cdrom = 1; >> static int default_sdcard = 1; >> static int default_vga = 1; >> +int default_xenplatform = 1; >> >> static struct { >> const char *driver; >> @@ -247,6 +248,7 @@ static struct { >> { .driver = "isa-cirrus-vga", .flag = &default_vga }, >> { .driver = "vmware-svga", .flag = &default_vga }, >> { .driver = "qxl-vga", .flag = &default_vga }, >> + { .driver = "xen-platform", .flag = &default_xenplatform }, >> }; >> >> static QemuOptsList qemu_rtc_opts = { >> @@ -4101,6 +4103,7 @@ int main(int argc, char **argv, char **envp) >> default_monitor = 0; >> default_net = 0; >> default_vga = 0; >> + default_xenplatform = 0; >> } >> >> if (is_daemonized()) { >> -- >> 1.8.3.1 >> >> > >
On Do, 2014-05-22 at 15:35 +0300, Michael S. Tsirkin wrote: > On Thu, May 22, 2014 at 01:11:28PM +0100, Stefano Stabellini wrote: > > On Thu, 22 May 2014, Gerd Hoffmann wrote: > > > Patch hooks up the xen platform device to the default device code we > > > have in qemu. Two effects: > > > > > > (1) The device will not be created in case -nodefaults is specified > > > on the command line. > > > (2) Autocreating the device is also turned off in case xen-platform > > > is added manually via -device. > > > > > > With the patch applied you can move the xen-platform device to some > > > other place with a simple 'qemu -device xen-platform,addr=$slot'. > > > > > > Tested-by: Tiejun Chen <tiejun.chen@intel.com> > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > > Given that libxl always passes -nodefaults to QEMU, this patch is going > > to effectively disable xen_platform_pci for all Xen users. It is not a > > good idea. With the patch applied a Xen user would have no way to enable > > xen_platform_pci except for passing some magic command line runes via > > device_model_args_hvm. > > > > Yes, it's an unfortunate use of the interface. > How about a new machine type for xenfv - that's the only one > that's affected, right? > It's time xen started versioning qemu hardware anyway. That would do the trick for sure. Question is what versioning scheme. Might be useful for xen machine types to version after xen releases, i.e. xenfv-4.4 would would be supposed to be compatible with the xen-4.4 toolstack etc. cheers, Gerd
> > Given that libxl always passes -nodefaults to QEMU, this patch is going > > to effectively disable xen_platform_pci for all Xen users. It is not a > > good idea. With the patch applied a Xen user would have no way to enable > > xen_platform_pci except for passing some magic command line runes via > > device_model_args_hvm. > > In fact this code only runs for "-M xenfv". If you use "-M pc", the > xen-platform device has to be added manually. Perhaps it would be > worthwhile to do the opposite, i.e. add the xen-platform device to "-M > pc" if not using -nodefaults. /me looks at the code. Yes, all the differences between xenfv and pc machine types are guarded by if (xen_enabled()) these days, except for adding the platform device. So using the pc machine type should just work on xen, and give you a machine without the platform device. So it can be added via -device, at any slot, if needed. No need to patch qemu at all. Adding or not adding xen-platform can easily handled by libxl then, depending on the xen_platform_pci switch in the config file. cheers, Gerd
On Thu, 22 May 2014, Gerd Hoffmann wrote: > Hi, > > > Given that libxl always passes -nodefaults to QEMU, this patch is going > > to effectively disable xen_platform_pci for all Xen users. It is not a > > good idea. With the patch applied a Xen user would have no way to enable > > xen_platform_pci except for passing some magic command line runes via > > device_model_args_hvm. > > Indeed. > > > > @@ -4101,6 +4103,7 @@ int main(int argc, char **argv, char **envp) > > > default_monitor = 0; > > > default_net = 0; > > > default_vga = 0; > > > + default_xenplatform = 0; > > > } > > With that chunk removed -nodefaults will have no effect for the xen > platform device, but explicitly moving it somewhere else via -device > xen-platform,addr=$slot should still work. That could work.
On Thu, 22 May 2014, Paolo Bonzini wrote: > Il 22/05/2014 14:11, Stefano Stabellini ha scritto: > > On Thu, 22 May 2014, Gerd Hoffmann wrote: > > > Patch hooks up the xen platform device to the default device code we > > > have in qemu. Two effects: > > > > > > (1) The device will not be created in case -nodefaults is specified > > > on the command line. > > > (2) Autocreating the device is also turned off in case xen-platform > > > is added manually via -device. > > > > > > With the patch applied you can move the xen-platform device to some > > > other place with a simple 'qemu -device xen-platform,addr=$slot'. > > > > > > Tested-by: Tiejun Chen <tiejun.chen@intel.com> > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > > Given that libxl always passes -nodefaults to QEMU, this patch is going > > to effectively disable xen_platform_pci for all Xen users. It is not a > > good idea. With the patch applied a Xen user would have no way to enable > > xen_platform_pci except for passing some magic command line runes via > > device_model_args_hvm. > > In fact this code only runs for "-M xenfv". If you use "-M pc", the > xen-platform device has to be added manually. Perhaps it would be worthwhile > to do the opposite, i.e. add the xen-platform device to "-M pc" if not using > -nodefaults. Unfortunately that would not work too. libxl exploits the fact that -M pc does not have a xen-platform device by default: case LIBXL_DOMAIN_TYPE_HVM: if (!libxl_defbool_val(b_info->u.hvm.xen_platform_pci)) { /* Switching here to the machine "pc" which does not add * the xen-platform device instead of the default "xenfv" machine. */ flexarray_append(dm_args, "pc,accel=xen"); } else { flexarray_append(dm_args, "xenfv"); } We could/should change libxl to always use -M pc but I would still rather avoid changing the existing interface between libxl and QEMU. Versioning xenfv or using a new pc machine is OK though. > > > hw/i386/pc_piix.c | 2 +- > > > include/hw/xen/xen.h | 1 + > > > vl.c | 3 +++ > > > 3 files changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > > > index eaf3e61..f987d03 100644 > > > --- a/hw/i386/pc_piix.c > > > +++ b/hw/i386/pc_piix.c > > > @@ -385,7 +385,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs *args) > > > pc_init_pci(args); > > > > > > bus = pci_find_primary_bus(); > > > - if (bus != NULL) { > > > + if (bus != NULL && default_xenplatform) { > > > pci_create_simple(bus, -1, "xen-platform"); > > > } > > > } > > > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h > > > index 85fda3d..b350413 100644 > > > --- a/include/hw/xen/xen.h > > > +++ b/include/hw/xen/xen.h > > > @@ -20,6 +20,7 @@ enum xen_mode { > > > > > > extern uint32_t xen_domid; > > > extern enum xen_mode xen_mode; > > > +extern int default_xenplatform; > > > > > > extern bool xen_allowed; > > > > > > diff --git a/vl.c b/vl.c > > > index 709d8cd..673148e 100644 > > > --- a/vl.c > > > +++ b/vl.c > > > @@ -226,6 +226,7 @@ static int default_floppy = 1; > > > static int default_cdrom = 1; > > > static int default_sdcard = 1; > > > static int default_vga = 1; > > > +int default_xenplatform = 1; > > > > > > static struct { > > > const char *driver; > > > @@ -247,6 +248,7 @@ static struct { > > > { .driver = "isa-cirrus-vga", .flag = &default_vga }, > > > { .driver = "vmware-svga", .flag = &default_vga }, > > > { .driver = "qxl-vga", .flag = &default_vga }, > > > + { .driver = "xen-platform", .flag = &default_xenplatform }, > > > }; > > > > > > static QemuOptsList qemu_rtc_opts = { > > > @@ -4101,6 +4103,7 @@ int main(int argc, char **argv, char **envp) > > > default_monitor = 0; > > > default_net = 0; > > > default_vga = 0; > > > + default_xenplatform = 0; > > > } > > > > > > if (is_daemonized()) { > > > -- > > > 1.8.3.1 > > > > > > > > > > >
On Thu, 22 May 2014, Gerd Hoffmann wrote: > > > Given that libxl always passes -nodefaults to QEMU, this patch is going > > > to effectively disable xen_platform_pci for all Xen users. It is not a > > > good idea. With the patch applied a Xen user would have no way to enable > > > xen_platform_pci except for passing some magic command line runes via > > > device_model_args_hvm. > > > > In fact this code only runs for "-M xenfv". If you use "-M pc", the > > xen-platform device has to be added manually. Perhaps it would be > > worthwhile to do the opposite, i.e. add the xen-platform device to "-M > > pc" if not using -nodefaults. > > /me looks at the code. Yes, all the differences between xenfv and pc > machine types are guarded by if (xen_enabled()) these days, except for > adding the platform device. > > So using the pc machine type should just work on xen, and give you a > machine without the platform device. So it can be added via -device, at > any slot, if needed. No need to patch qemu at all. Adding or not > adding xen-platform can easily handled by libxl then, depending on the > xen_platform_pci switch in the config file. I agree. Changing libxl to always use -M pc and using -device to add xen-platform when needed sounds like the best option.
On Thu, May 22, 2014 at 02:53:19PM +0200, Gerd Hoffmann wrote: > On Do, 2014-05-22 at 15:35 +0300, Michael S. Tsirkin wrote: > > On Thu, May 22, 2014 at 01:11:28PM +0100, Stefano Stabellini wrote: > > > On Thu, 22 May 2014, Gerd Hoffmann wrote: > > > > Patch hooks up the xen platform device to the default device code we > > > > have in qemu. Two effects: > > > > > > > > (1) The device will not be created in case -nodefaults is specified > > > > on the command line. > > > > (2) Autocreating the device is also turned off in case xen-platform > > > > is added manually via -device. > > > > > > > > With the patch applied you can move the xen-platform device to some > > > > other place with a simple 'qemu -device xen-platform,addr=$slot'. > > > > > > > > Tested-by: Tiejun Chen <tiejun.chen@intel.com> > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > > > > Given that libxl always passes -nodefaults to QEMU, this patch is going > > > to effectively disable xen_platform_pci for all Xen users. It is not a > > > good idea. With the patch applied a Xen user would have no way to enable > > > xen_platform_pci except for passing some magic command line runes via > > > device_model_args_hvm. > > > > > > > Yes, it's an unfortunate use of the interface. > > How about a new machine type for xenfv - that's the only one > > that's affected, right? > > It's time xen started versioning qemu hardware anyway. > > That would do the trick for sure. Question is what versioning scheme. > Might be useful for xen machine types to version after xen releases, > i.e. xenfv-4.4 would would be supposed to be compatible with the xen-4.4 > toolstack etc. > > cheers, > Gerd > It's much easier for us to version it with qemu machine version, like we do for pc, if that can work at all.
Il 22/05/2014 15:55, Stefano Stabellini ha scritto: >> > In fact this code only runs for "-M xenfv". If you use "-M pc", the >> > xen-platform device has to be added manually. Perhaps it would be worthwhile >> > to do the opposite, i.e. add the xen-platform device to "-M pc" if not using >> > -nodefaults. > Unfortunately that would not work too. libxl exploits the fact that -M > pc does not have a xen-platform device by default: It wouldn't affect libxl, since it uses -nodefaults too (thus the xen-platform device would not be auto-added for -M pc). Paolo
> -----Original Message----- > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > Sent: 22 May 2014 14:57 > To: Gerd Hoffmann > Cc: Paolo Bonzini; Stefano Stabellini; Paul Durrant; qemu-devel@nongnu.org; > Anthony Liguori; mst@redhat.com > Subject: Re: [PATCH] xen: make xen-platform a default device > > On Thu, 22 May 2014, Gerd Hoffmann wrote: > > > > Given that libxl always passes -nodefaults to QEMU, this patch is going > > > > to effectively disable xen_platform_pci for all Xen users. It is not a > > > > good idea. With the patch applied a Xen user would have no way to > enable > > > > xen_platform_pci except for passing some magic command line runes > via > > > > device_model_args_hvm. > > > > > > In fact this code only runs for "-M xenfv". If you use "-M pc", the > > > xen-platform device has to be added manually. Perhaps it would be > > > worthwhile to do the opposite, i.e. add the xen-platform device to "-M > > > pc" if not using -nodefaults. > > > > /me looks at the code. Yes, all the differences between xenfv and pc > > machine types are guarded by if (xen_enabled()) these days, except for > > adding the platform device. > > > > So using the pc machine type should just work on xen, and give you a > > machine without the platform device. So it can be added via -device, at > > any slot, if needed. No need to patch qemu at all. Adding or not > > adding xen-platform can easily handled by libxl then, depending on the > > xen_platform_pci switch in the config file. > > I agree. Changing libxl to always use -M pc and using -device to add > xen-platform when needed sounds like the best option. Really? You opposed this before: http://lists.xen.org/archives/html/xen-devel/2013-06/msg01946.html stating that you wanted a mechanism to query the running version of QEMU before choosing the machine type. Paul
On Fri, 23 May 2014, Paul Durrant wrote: > > -----Original Message----- > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > Sent: 22 May 2014 14:57 > > To: Gerd Hoffmann > > Cc: Paolo Bonzini; Stefano Stabellini; Paul Durrant; qemu-devel@nongnu.org; > > Anthony Liguori; mst@redhat.com > > Subject: Re: [PATCH] xen: make xen-platform a default device > > > > On Thu, 22 May 2014, Gerd Hoffmann wrote: > > > > > Given that libxl always passes -nodefaults to QEMU, this patch is going > > > > > to effectively disable xen_platform_pci for all Xen users. It is not a > > > > > good idea. With the patch applied a Xen user would have no way to > > enable > > > > > xen_platform_pci except for passing some magic command line runes > > via > > > > > device_model_args_hvm. > > > > > > > > In fact this code only runs for "-M xenfv". If you use "-M pc", the > > > > xen-platform device has to be added manually. Perhaps it would be > > > > worthwhile to do the opposite, i.e. add the xen-platform device to "-M > > > > pc" if not using -nodefaults. > > > > > > /me looks at the code. Yes, all the differences between xenfv and pc > > > machine types are guarded by if (xen_enabled()) these days, except for > > > adding the platform device. > > > > > > So using the pc machine type should just work on xen, and give you a > > > machine without the platform device. So it can be added via -device, at > > > any slot, if needed. No need to patch qemu at all. Adding or not > > > adding xen-platform can easily handled by libxl then, depending on the > > > xen_platform_pci switch in the config file. > > > > I agree. Changing libxl to always use -M pc and using -device to add > > xen-platform when needed sounds like the best option. > > Really? You opposed this before: > > http://lists.xen.org/archives/html/xen-devel/2013-06/msg01946.html > > stating that you wanted a mechanism to query the running version of QEMU before choosing the machine type. Many QEMU releases have been made since then. I feel more comfortable with using -M pc, especially given the alternatives that have been proposed. Of course having a mechanism to query the running version of QEMU would still be the safest solution.
> -----Original Message----- > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > Sent: 23 May 2014 11:11 > To: Paul Durrant > Cc: Stefano Stabellini; Gerd Hoffmann; Paolo Bonzini; qemu- > devel@nongnu.org; Anthony Liguori; mst@redhat.com > Subject: RE: [PATCH] xen: make xen-platform a default device > > On Fri, 23 May 2014, Paul Durrant wrote: > > > -----Original Message----- > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > > Sent: 22 May 2014 14:57 > > > To: Gerd Hoffmann > > > Cc: Paolo Bonzini; Stefano Stabellini; Paul Durrant; qemu- > devel@nongnu.org; > > > Anthony Liguori; mst@redhat.com > > > Subject: Re: [PATCH] xen: make xen-platform a default device > > > > > > On Thu, 22 May 2014, Gerd Hoffmann wrote: > > > > > > Given that libxl always passes -nodefaults to QEMU, this patch is > going > > > > > > to effectively disable xen_platform_pci for all Xen users. It is not a > > > > > > good idea. With the patch applied a Xen user would have no way to > > > enable > > > > > > xen_platform_pci except for passing some magic command line > runes > > > via > > > > > > device_model_args_hvm. > > > > > > > > > > In fact this code only runs for "-M xenfv". If you use "-M pc", the > > > > > xen-platform device has to be added manually. Perhaps it would be > > > > > worthwhile to do the opposite, i.e. add the xen-platform device to "- > M > > > > > pc" if not using -nodefaults. > > > > > > > > /me looks at the code. Yes, all the differences between xenfv and pc > > > > machine types are guarded by if (xen_enabled()) these days, except > for > > > > adding the platform device. > > > > > > > > So using the pc machine type should just work on xen, and give you a > > > > machine without the platform device. So it can be added via -device, at > > > > any slot, if needed. No need to patch qemu at all. Adding or not > > > > adding xen-platform can easily handled by libxl then, depending on the > > > > xen_platform_pci switch in the config file. > > > > > > I agree. Changing libxl to always use -M pc and using -device to add > > > xen-platform when needed sounds like the best option. > > > > Really? You opposed this before: > > > > http://lists.xen.org/archives/html/xen-devel/2013-06/msg01946.html > > > > stating that you wanted a mechanism to query the running version of > QEMU before choosing the machine type. > > Many QEMU releases have been made since then. I feel more comfortable > with using -M pc, especially given the alternatives that have been > proposed. Of course having a mechanism to query the running version of > QEMU would still be the safest solution. But I thought the problem was that, should someone change what 'pc' means, then libxl would invoke the 'wrong' machine type on a domain restore. I know that xenfv is essentially 'pc' at the moment but would could fix on a particular version if compatibility becomes an issue. So, rather than choosing 'pc' now, should we not choose 'pc-i440fx-1.6' which is the current default? Paul
On Fri, 23 May 2014, Paul Durrant wrote: > > -----Original Message----- > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > Sent: 23 May 2014 11:11 > > To: Paul Durrant > > Cc: Stefano Stabellini; Gerd Hoffmann; Paolo Bonzini; qemu- > > devel@nongnu.org; Anthony Liguori; mst@redhat.com > > Subject: RE: [PATCH] xen: make xen-platform a default device > > > > On Fri, 23 May 2014, Paul Durrant wrote: > > > > -----Original Message----- > > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > > > Sent: 22 May 2014 14:57 > > > > To: Gerd Hoffmann > > > > Cc: Paolo Bonzini; Stefano Stabellini; Paul Durrant; qemu- > > devel@nongnu.org; > > > > Anthony Liguori; mst@redhat.com > > > > Subject: Re: [PATCH] xen: make xen-platform a default device > > > > > > > > On Thu, 22 May 2014, Gerd Hoffmann wrote: > > > > > > > Given that libxl always passes -nodefaults to QEMU, this patch is > > going > > > > > > > to effectively disable xen_platform_pci for all Xen users. It is not a > > > > > > > good idea. With the patch applied a Xen user would have no way to > > > > enable > > > > > > > xen_platform_pci except for passing some magic command line > > runes > > > > via > > > > > > > device_model_args_hvm. > > > > > > > > > > > > In fact this code only runs for "-M xenfv". If you use "-M pc", the > > > > > > xen-platform device has to be added manually. Perhaps it would be > > > > > > worthwhile to do the opposite, i.e. add the xen-platform device to "- > > M > > > > > > pc" if not using -nodefaults. > > > > > > > > > > /me looks at the code. Yes, all the differences between xenfv and pc > > > > > machine types are guarded by if (xen_enabled()) these days, except > > for > > > > > adding the platform device. > > > > > > > > > > So using the pc machine type should just work on xen, and give you a > > > > > machine without the platform device. So it can be added via -device, at > > > > > any slot, if needed. No need to patch qemu at all. Adding or not > > > > > adding xen-platform can easily handled by libxl then, depending on the > > > > > xen_platform_pci switch in the config file. > > > > > > > > I agree. Changing libxl to always use -M pc and using -device to add > > > > xen-platform when needed sounds like the best option. > > > > > > Really? You opposed this before: > > > > > > http://lists.xen.org/archives/html/xen-devel/2013-06/msg01946.html > > > > > > stating that you wanted a mechanism to query the running version of > > QEMU before choosing the machine type. > > > > Many QEMU releases have been made since then. I feel more comfortable > > with using -M pc, especially given the alternatives that have been > > proposed. Of course having a mechanism to query the running version of > > QEMU would still be the safest solution. > > But I thought the problem was that, should someone change what 'pc' means, then libxl would invoke the 'wrong' machine type on a domain restore. I know that xenfv is essentially 'pc' at the moment but would could fix on a particular version if compatibility becomes an issue. So, rather than choosing 'pc' now, should we not choose 'pc-i440fx-1.6' which is the current default? One issue is that -M pc didn't always work with Xen. Now it does and we are already relying on it in libxl since 2bc047635b51abd41c917aa2b813211ee0de2c38. It is safe because all QEMU releases from 1.6 onward work well with Xen and -M pc. Older QEMU releases are considered ancient and unmaintained. This is what I was referring to in my last reply. I really meant "we should move away from xenfv and use a pc.* machine that does not create xen-platform by default". As you say the other issue is the version of the pc machine, especially in relation to save/restore. However since: commit 2bc047635b51abd41c917aa2b813211ee0de2c38 Author: Anthony PERARD <anthony.perard@citrix.com> Date: Wed Nov 27 18:21:34 2013 +0000 libxl: Handle xen_platform_pci=0 case with qemu-xen. we are simply calling QEMU with -M pc if xen_platform_pci=0. I think we should change that too and backport the patch to 4.4. pc-i440fx-1.6 seems like a good choice to me.
> -----Original Message----- > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > Sent: 23 May 2014 12:37 > To: Paul Durrant > Cc: Stefano Stabellini; Gerd Hoffmann; Paolo Bonzini; qemu- > devel@nongnu.org; Anthony Liguori; mst@redhat.com; Anthony Perard > Subject: RE: [PATCH] xen: make xen-platform a default device > > On Fri, 23 May 2014, Paul Durrant wrote: > > > -----Original Message----- > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > > Sent: 23 May 2014 11:11 > > > To: Paul Durrant > > > Cc: Stefano Stabellini; Gerd Hoffmann; Paolo Bonzini; qemu- > > > devel@nongnu.org; Anthony Liguori; mst@redhat.com > > > Subject: RE: [PATCH] xen: make xen-platform a default device > > > > > > On Fri, 23 May 2014, Paul Durrant wrote: > > > > > -----Original Message----- > > > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > > > > > Sent: 22 May 2014 14:57 > > > > > To: Gerd Hoffmann > > > > > Cc: Paolo Bonzini; Stefano Stabellini; Paul Durrant; qemu- > > > devel@nongnu.org; > > > > > Anthony Liguori; mst@redhat.com > > > > > Subject: Re: [PATCH] xen: make xen-platform a default device > > > > > > > > > > On Thu, 22 May 2014, Gerd Hoffmann wrote: > > > > > > > > Given that libxl always passes -nodefaults to QEMU, this patch is > > > going > > > > > > > > to effectively disable xen_platform_pci for all Xen users. It is not > a > > > > > > > > good idea. With the patch applied a Xen user would have no way > to > > > > > enable > > > > > > > > xen_platform_pci except for passing some magic command line > > > runes > > > > > via > > > > > > > > device_model_args_hvm. > > > > > > > > > > > > > > In fact this code only runs for "-M xenfv". If you use "-M pc", the > > > > > > > xen-platform device has to be added manually. Perhaps it would > be > > > > > > > worthwhile to do the opposite, i.e. add the xen-platform device > to "- > > > M > > > > > > > pc" if not using -nodefaults. > > > > > > > > > > > > /me looks at the code. Yes, all the differences between xenfv and > pc > > > > > > machine types are guarded by if (xen_enabled()) these days, > except > > > for > > > > > > adding the platform device. > > > > > > > > > > > > So using the pc machine type should just work on xen, and give you > a > > > > > > machine without the platform device. So it can be added via - > device, at > > > > > > any slot, if needed. No need to patch qemu at all. Adding or not > > > > > > adding xen-platform can easily handled by libxl then, depending on > the > > > > > > xen_platform_pci switch in the config file. > > > > > > > > > > I agree. Changing libxl to always use -M pc and using -device to add > > > > > xen-platform when needed sounds like the best option. > > > > > > > > Really? You opposed this before: > > > > > > > > http://lists.xen.org/archives/html/xen-devel/2013-06/msg01946.html > > > > > > > > stating that you wanted a mechanism to query the running version of > > > QEMU before choosing the machine type. > > > > > > Many QEMU releases have been made since then. I feel more > comfortable > > > with using -M pc, especially given the alternatives that have been > > > proposed. Of course having a mechanism to query the running version of > > > QEMU would still be the safest solution. > > > > But I thought the problem was that, should someone change what 'pc' > means, then libxl would invoke the 'wrong' machine type on a domain > restore. I know that xenfv is essentially 'pc' at the moment but would could > fix on a particular version if compatibility becomes an issue. So, rather than > choosing 'pc' now, should we not choose 'pc-i440fx-1.6' which is the current > default? > > One issue is that -M pc didn't always work with Xen. Now it does and we > are already relying on it in libxl since > 2bc047635b51abd41c917aa2b813211ee0de2c38. It is safe because all QEMU > releases from 1.6 onward work well with Xen and -M pc. Older QEMU > releases are considered ancient and unmaintained. This is what I was > referring to in my last reply. I really meant "we should move away from > xenfv and use a pc.* machine that does not create xen-platform by > default". > > As you say the other issue is the version of the pc machine, especially > in relation to save/restore. However since: > > commit 2bc047635b51abd41c917aa2b813211ee0de2c38 > Author: Anthony PERARD <anthony.perard@citrix.com> > Date: Wed Nov 27 18:21:34 2013 +0000 > > libxl: Handle xen_platform_pci=0 case with qemu-xen. > > we are simply calling QEMU with -M pc if xen_platform_pci=0. I think we > should change that too and backport the patch to 4.4. pc-i440fx-1.6 > seems like a good choice to me. Yep - that sounds good. Paul
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index eaf3e61..f987d03 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -385,7 +385,7 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs *args) pc_init_pci(args); bus = pci_find_primary_bus(); - if (bus != NULL) { + if (bus != NULL && default_xenplatform) { pci_create_simple(bus, -1, "xen-platform"); } } diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h index 85fda3d..b350413 100644 --- a/include/hw/xen/xen.h +++ b/include/hw/xen/xen.h @@ -20,6 +20,7 @@ enum xen_mode { extern uint32_t xen_domid; extern enum xen_mode xen_mode; +extern int default_xenplatform; extern bool xen_allowed; diff --git a/vl.c b/vl.c index 709d8cd..673148e 100644 --- a/vl.c +++ b/vl.c @@ -226,6 +226,7 @@ static int default_floppy = 1; static int default_cdrom = 1; static int default_sdcard = 1; static int default_vga = 1; +int default_xenplatform = 1; static struct { const char *driver; @@ -247,6 +248,7 @@ static struct { { .driver = "isa-cirrus-vga", .flag = &default_vga }, { .driver = "vmware-svga", .flag = &default_vga }, { .driver = "qxl-vga", .flag = &default_vga }, + { .driver = "xen-platform", .flag = &default_xenplatform }, }; static QemuOptsList qemu_rtc_opts = { @@ -4101,6 +4103,7 @@ int main(int argc, char **argv, char **envp) default_monitor = 0; default_net = 0; default_vga = 0; + default_xenplatform = 0; } if (is_daemonized()) {
Patch hooks up the xen platform device to the default device code we have in qemu. Two effects: (1) The device will not be created in case -nodefaults is specified on the command line. (2) Autocreating the device is also turned off in case xen-platform is added manually via -device. With the patch applied you can move the xen-platform device to some other place with a simple 'qemu -device xen-platform,addr=$slot'. Tested-by: Tiejun Chen <tiejun.chen@intel.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/i386/pc_piix.c | 2 +- include/hw/xen/xen.h | 1 + vl.c | 3 +++ 3 files changed, 5 insertions(+), 1 deletion(-)