| Message ID | 20130827070110.GA16338@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 27/08/2013 09:01, Michael S. Tsirkin ha scritto: > We have a lot of code duplication between machine types, > this increases with each new machine type > and each new field. > > This has already introduced a minor bug: description > for pc-1.3 says "Standard PC" while description for > pc-1.4 is "Standard PC (i440FX + PIIX, 1996)" > which makes you think 1.3 is somehow more standard, > or newer, while in fact it's a revision of the same PC. > > This patch addresses this issue by using macros, along > the lines used by PC_COMPAT_X_X - only for > non-property options. > > Note: this conflicts (trivially) with patch > [PATCH v3 6/6] hw: Clean up bogus default boot order > by Markus. Sent separately for ease of review. If people like this > approach, I can rebase on top of that patch. > > The approach can extend to non-PC machine types. > > Cc: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > static QEMUMachine isapc_machine = { > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 198c785..084ecac 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -253,39 +253,41 @@ static void pc_q35_init_1_4(QEMUMachineInitArgs *args) > pc_q35_init(args); > } > > +#define PC_Q35_MACHINE_OPTIONS \ > + PC_DEFAULT_MACHINE_OPTIONS, \ > + .hot_add_cpu = pc_hot_add_cpu .desc is missing, right? Otherwise looks good. Paolo > + > +#define PC_Q35_1_6_MACHINE_OPTIONS PC_Q35_MACHINE_OPTIONS > static QEMUMachine pc_q35_machine_v1_6 = { > + PC_Q35_1_6_MACHINE_OPTIONS, > .name = "pc-q35-1.6", > .alias = "q35", > - .desc = "Standard PC (Q35 + ICH9, 2009)", > .init = pc_q35_init_1_6, > - .hot_add_cpu = pc_hot_add_cpu, > - .max_cpus = 255, > - DEFAULT_MACHINE_OPTIONS, > }; > > static QEMUMachine pc_q35_machine_v1_5 = { > + PC_Q35_1_6_MACHINE_OPTIONS, > .name = "pc-q35-1.5", > - .desc = "Standard PC (Q35 + ICH9, 2009)", > .init = pc_q35_init_1_5, > - .hot_add_cpu = pc_hot_add_cpu, > - .max_cpus = 255, > .compat_props = (GlobalProperty[]) { > PC_COMPAT_1_5, > { /* end of list */ } > }, > - DEFAULT_MACHINE_OPTIONS, > }; > > +#define PC_Q35_1_4_MACHINE_OPTIONS \ > + PC_Q35_1_6_MACHINE_OPTIONS, \ > + .hot_add_cpu = NULL > + > static QEMUMachine pc_q35_machine_v1_4 = { > + PC_Q35_1_4_MACHINE_OPTIONS, > .name = "pc-q35-1.4", > - .desc = "Standard PC (Q35 + ICH9, 2009)", > .init = pc_q35_init_1_4, > - .max_cpus = 255, > .compat_props = (GlobalProperty[]) { > PC_COMPAT_1_4, > { /* end of list */ } > }, > - DEFAULT_MACHINE_OPTIONS, > }; > > static void pc_q35_machine_init(void) > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 475ba9e..4a38fad 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -325,4 +325,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t); > .value = stringify(0),\ > } > > +#define PC_DEFAULT_MACHINE_OPTIONS \ > + DEFAULT_MACHINE_OPTIONS, \ > + .hot_add_cpu = pc_hot_add_cpu, \ > + .max_cpus = 255 > + > #endif >
"Michael S. Tsirkin" <mst@redhat.com> writes: > We have a lot of code duplication between machine types, > this increases with each new machine type > and each new field. > > This has already introduced a minor bug: description > for pc-1.3 says "Standard PC" while description for > pc-1.4 is "Standard PC (i440FX + PIIX, 1996)" > which makes you think 1.3 is somehow more standard, > or newer, while in fact it's a revision of the same PC. I wouldn't call it a bug. We're in the habit of never changing old machine types, so when we created pc-i440fx-1.4 and pc-q35-1.4 with a .desc that clearly explains the difference, we didn't change the older versions the PC machine types as well. That may have been overly cautious. As long as .desc is not exposed to the guest, it's not ABI, thus can be changed. > This patch addresses this issue by using macros, along > the lines used by PC_COMPAT_X_X - only for > non-property options. > > Note: this conflicts (trivially) with patch > [PATCH v3 6/6] hw: Clean up bogus default boot order > by Markus. Sent separately for ease of review. If people like this > approach, I can rebase on top of that patch. Appreciated. I'd very much prefer this patch to be rebased on top of mine rather than the other way round, because mine got a review from Laszlo. He wrote my patch "wasn't easy to verify", so I don't want to ask him for reviewing v5 without a really good reason. > The approach can extend to non-PC machine types. Yes, but since these types aren't versioned, there's less food for de-duplication. Interested parties are of course quite welcome to de-duplicate whatever is there. > Cc: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/i386/pc_piix.c | 82 ++++++++++++++++++++-------------------------------- > hw/i386/pc_q35.c | 24 ++++++++------- > include/hw/i386/pc.h | 5 ++++ > 3 files changed, 50 insertions(+), 61 deletions(-) The patch trades a daisy-chain of macros for a few duplicated lines of data. Matter of taste. Not really my thing, but I don't really mind either. If it was my thing, I'd probably change the .desc in 1/2, then introduce the macro chain in 2/2, so that each commit does one thing only.
On Tue, Aug 27, 2013 at 09:29:33AM +0200, Paolo Bonzini wrote: > Il 27/08/2013 09:01, Michael S. Tsirkin ha scritto: > > We have a lot of code duplication between machine types, > > this increases with each new machine type > > and each new field. > > > > This has already introduced a minor bug: description > > for pc-1.3 says "Standard PC" while description for > > pc-1.4 is "Standard PC (i440FX + PIIX, 1996)" > > which makes you think 1.3 is somehow more standard, > > or newer, while in fact it's a revision of the same PC. > > > > This patch addresses this issue by using macros, along > > the lines used by PC_COMPAT_X_X - only for > > non-property options. > > > > Note: this conflicts (trivially) with patch > > [PATCH v3 6/6] hw: Clean up bogus default boot order > > by Markus. Sent separately for ease of review. If people like this > > approach, I can rebase on top of that patch. > > > > The approach can extend to non-PC machine types. > > > > Cc: Markus Armbruster <armbru@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > static QEMUMachine isapc_machine = { > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > > index 198c785..084ecac 100644 > > --- a/hw/i386/pc_q35.c > > +++ b/hw/i386/pc_q35.c > > @@ -253,39 +253,41 @@ static void pc_q35_init_1_4(QEMUMachineInitArgs *args) > > pc_q35_init(args); > > } > > > > +#define PC_Q35_MACHINE_OPTIONS \ > > + PC_DEFAULT_MACHINE_OPTIONS, \ > > + .hot_add_cpu = pc_hot_add_cpu > > .desc is missing, right? > > Otherwise looks good. > > Paolo Good catch, I'll fix that up. > > + > > +#define PC_Q35_1_6_MACHINE_OPTIONS PC_Q35_MACHINE_OPTIONS > > static QEMUMachine pc_q35_machine_v1_6 = { > > + PC_Q35_1_6_MACHINE_OPTIONS, > > .name = "pc-q35-1.6", > > .alias = "q35", > > - .desc = "Standard PC (Q35 + ICH9, 2009)", > > .init = pc_q35_init_1_6, > > - .hot_add_cpu = pc_hot_add_cpu, > > - .max_cpus = 255, > > - DEFAULT_MACHINE_OPTIONS, > > }; > > > > static QEMUMachine pc_q35_machine_v1_5 = { > > + PC_Q35_1_6_MACHINE_OPTIONS, > > .name = "pc-q35-1.5", > > - .desc = "Standard PC (Q35 + ICH9, 2009)", > > .init = pc_q35_init_1_5, > > - .hot_add_cpu = pc_hot_add_cpu, > > - .max_cpus = 255, > > .compat_props = (GlobalProperty[]) { > > PC_COMPAT_1_5, > > { /* end of list */ } > > }, > > - DEFAULT_MACHINE_OPTIONS, > > }; > > > > +#define PC_Q35_1_4_MACHINE_OPTIONS \ > > + PC_Q35_1_6_MACHINE_OPTIONS, \ > > + .hot_add_cpu = NULL > > + > > static QEMUMachine pc_q35_machine_v1_4 = { > > + PC_Q35_1_4_MACHINE_OPTIONS, > > .name = "pc-q35-1.4", > > - .desc = "Standard PC (Q35 + ICH9, 2009)", > > .init = pc_q35_init_1_4, > > - .max_cpus = 255, > > .compat_props = (GlobalProperty[]) { > > PC_COMPAT_1_4, > > { /* end of list */ } > > }, > > - DEFAULT_MACHINE_OPTIONS, > > }; > > > > static void pc_q35_machine_init(void) > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > > index 475ba9e..4a38fad 100644 > > --- a/include/hw/i386/pc.h > > +++ b/include/hw/i386/pc.h > > @@ -325,4 +325,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t); > > .value = stringify(0),\ > > } > > > > +#define PC_DEFAULT_MACHINE_OPTIONS \ > > + DEFAULT_MACHINE_OPTIONS, \ > > + .hot_add_cpu = pc_hot_add_cpu, \ > > + .max_cpus = 255 > > + > > #endif > >
On Tue, Aug 27, 2013 at 09:51:22AM +0200, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > We have a lot of code duplication between machine types, > > this increases with each new machine type > > and each new field. > > > > This has already introduced a minor bug: description > > for pc-1.3 says "Standard PC" while description for > > pc-1.4 is "Standard PC (i440FX + PIIX, 1996)" > > which makes you think 1.3 is somehow more standard, > > or newer, while in fact it's a revision of the same PC. > > I wouldn't call it a bug. We're in the habit of never changing old > machine types, so when we created pc-i440fx-1.4 and pc-q35-1.4 with a > .desc that clearly explains the difference, we didn't change the older > versions the PC machine types as well. > > That may have been overly cautious. As long as .desc is not exposed to > the guest, it's not ABI, thus can be changed. I just confirmed that on qemu-system-x86_64 the only place where .desc is used is on the machine_parse() help/error code at vl.c. I don't know about other architectures. I suggest we explicitly document the field as a human-readable machine-type description that should be never exposed to the guest (just in case somebody decides to use .desc on some guest-visible table one day).
On Tue, Aug 27, 2013 at 01:09:23PM -0300, Eduardo Habkost wrote: > On Tue, Aug 27, 2013 at 09:51:22AM +0200, Markus Armbruster wrote: > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > > > We have a lot of code duplication between machine types, > > > this increases with each new machine type > > > and each new field. > > > > > > This has already introduced a minor bug: description > > > for pc-1.3 says "Standard PC" while description for > > > pc-1.4 is "Standard PC (i440FX + PIIX, 1996)" > > > which makes you think 1.3 is somehow more standard, > > > or newer, while in fact it's a revision of the same PC. > > > > I wouldn't call it a bug. We're in the habit of never changing old > > machine types, so when we created pc-i440fx-1.4 and pc-q35-1.4 with a > > .desc that clearly explains the difference, we didn't change the older > > versions the PC machine types as well. > > > > That may have been overly cautious. As long as .desc is not exposed to > > the guest, it's not ABI, thus can be changed. > > I just confirmed that on qemu-system-x86_64 the only place where .desc > is used is on the machine_parse() help/error code at vl.c. I don't know > about other architectures. Generally, one would hope we won't expose random internal stuff to guests :). > I suggest we explicitly document the field as a human-readable > machine-type description that should be never exposed to the guest (just > in case somebody decides to use .desc on some guest-visible table one > day). Where would you document this? > -- > Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes: > On Tue, Aug 27, 2013 at 09:51:22AM +0200, Markus Armbruster wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >> > We have a lot of code duplication between machine types, >> > this increases with each new machine type >> > and each new field. >> > >> > This has already introduced a minor bug: description >> > for pc-1.3 says "Standard PC" while description for >> > pc-1.4 is "Standard PC (i440FX + PIIX, 1996)" >> > which makes you think 1.3 is somehow more standard, >> > or newer, while in fact it's a revision of the same PC. >> >> I wouldn't call it a bug. We're in the habit of never changing old >> machine types, so when we created pc-i440fx-1.4 and pc-q35-1.4 with a >> .desc that clearly explains the difference, we didn't change the older >> versions the PC machine types as well. >> >> That may have been overly cautious. As long as .desc is not exposed to >> the guest, it's not ABI, thus can be changed. > > I just confirmed that on qemu-system-x86_64 the only place where .desc > is used is on the machine_parse() help/error code at vl.c. I don't know > about other architectures. > > I suggest we explicitly document the field as a human-readable > machine-type description that should be never exposed to the guest (just > in case somebody decides to use .desc on some guest-visible table one > day). My "[PATCH v2 7/7] smbios: Set system manufacturer, product & version by default"[*] puts .desc in SMBIOS type 1 entry product name. Makes more sense than our current "Bochs". I'm fine with reserving .desc for QEMU help, I'll simply create another member that may be exposed to the guest then. [*] On list since mid July, Reviewed-by: Eric, Ignored-by: all maintainers.
On Tue, Aug 27, 2013 at 07:13:49PM +0300, Michael S. Tsirkin wrote: > On Tue, Aug 27, 2013 at 01:09:23PM -0300, Eduardo Habkost wrote: > > On Tue, Aug 27, 2013 at 09:51:22AM +0200, Markus Armbruster wrote: > > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > > > > > We have a lot of code duplication between machine types, > > > > this increases with each new machine type > > > > and each new field. > > > > > > > > This has already introduced a minor bug: description > > > > for pc-1.3 says "Standard PC" while description for > > > > pc-1.4 is "Standard PC (i440FX + PIIX, 1996)" > > > > which makes you think 1.3 is somehow more standard, > > > > or newer, while in fact it's a revision of the same PC. > > > > > > I wouldn't call it a bug. We're in the habit of never changing old > > > machine types, so when we created pc-i440fx-1.4 and pc-q35-1.4 with a > > > .desc that clearly explains the difference, we didn't change the older > > > versions the PC machine types as well. > > > > > > That may have been overly cautious. As long as .desc is not exposed to > > > the guest, it's not ABI, thus can be changed. > > > > I just confirmed that on qemu-system-x86_64 the only place where .desc > > is used is on the machine_parse() help/error code at vl.c. I don't know > > about other architectures. > > Generally, one would hope we won't expose random > internal stuff to guests :). Exactly: we won't expose random internal stuff to guests, but there was nothing about the field that made it clear that it was internal. In this specific case, somebody could one day suggest adding .desc to some SMBIOS field, and the suggestion would even make sense to me. > > > I suggest we explicitly document the field as a human-readable > > machine-type description that should be never exposed to the guest (just > > in case somebody decides to use .desc on some guest-visible table one > > day). > > Where would you document this? Just above the field declaration on struct QEMUMachine at hw/boards.h.
On Tue, Aug 27, 2013 at 04:19:39PM -0300, Eduardo Habkost wrote: > On Tue, Aug 27, 2013 at 07:13:49PM +0300, Michael S. Tsirkin wrote: > > On Tue, Aug 27, 2013 at 01:09:23PM -0300, Eduardo Habkost wrote: > > > On Tue, Aug 27, 2013 at 09:51:22AM +0200, Markus Armbruster wrote: > > > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > > > > > > > We have a lot of code duplication between machine types, > > > > > this increases with each new machine type > > > > > and each new field. > > > > > > > > > > This has already introduced a minor bug: description > > > > > for pc-1.3 says "Standard PC" while description for > > > > > pc-1.4 is "Standard PC (i440FX + PIIX, 1996)" > > > > > which makes you think 1.3 is somehow more standard, > > > > > or newer, while in fact it's a revision of the same PC. > > > > > > > > I wouldn't call it a bug. We're in the habit of never changing old > > > > machine types, so when we created pc-i440fx-1.4 and pc-q35-1.4 with a > > > > .desc that clearly explains the difference, we didn't change the older > > > > versions the PC machine types as well. > > > > > > > > That may have been overly cautious. As long as .desc is not exposed to > > > > the guest, it's not ABI, thus can be changed. > > > > > > I just confirmed that on qemu-system-x86_64 the only place where .desc > > > is used is on the machine_parse() help/error code at vl.c. I don't know > > > about other architectures. > > > > Generally, one would hope we won't expose random > > internal stuff to guests :). > > Exactly: we won't expose random internal stuff to guests, but there was > nothing about the field that made it clear that it was internal. > > In this specific case, somebody could one day suggest adding .desc to > some SMBIOS field, and the suggestion would even make sense to me. I'm not sure why, but assuming we do this one day, we'd need to do this in some way that only affects new machine types, so we really can't expose .desc (which old machine types have) we'd need to expose something new. > > > > > I suggest we explicitly document the field as a human-readable > > > machine-type description that should be never exposed to the guest (just > > > in case somebody decides to use .desc on some guest-visible table one > > > day). > > > > Where would you document this? > > Just above the field declaration on struct QEMUMachine at hw/boards.h. So it's human readable and I see no reason to expose it to guests, but I'm still not sure what would be wrong if some future machine version did exactly that assuming a compelling reason. > -- > Eduardo
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 3c36a2a..ae2a805 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -334,40 +334,43 @@ static void pc_xen_hvm_init(QEMUMachineInitArgs *args) } #endif +#define PC_I440FX_MACHINE_OPTIONS \ + PC_DEFAULT_MACHINE_OPTIONS, \ + .desc = "Standard PC (i440FX + PIIX, 1996)", \ + .hot_add_cpu = pc_hot_add_cpu + +#define PC_I440FX_1_6_MACHINE_OPTIONS PC_I440FX_MACHINE_OPTIONS + static QEMUMachine pc_i440fx_machine_v1_6 = { + PC_I440FX_1_6_MACHINE_OPTIONS, .name = "pc-i440fx-1.6", .alias = "pc", - .desc = "Standard PC (i440FX + PIIX, 1996)", .init = pc_init_pci_1_6, - .hot_add_cpu = pc_hot_add_cpu, - .max_cpus = 255, .is_default = 1, - DEFAULT_MACHINE_OPTIONS, }; static QEMUMachine pc_i440fx_machine_v1_5 = { + PC_I440FX_1_6_MACHINE_OPTIONS, .name = "pc-i440fx-1.5", - .desc = "Standard PC (i440FX + PIIX, 1996)", .init = pc_init_pci_1_5, - .hot_add_cpu = pc_hot_add_cpu, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_1_5, { /* end of list */ } }, - DEFAULT_MACHINE_OPTIONS, }; +#define PC_I440FX_1_4_MACHINE_OPTIONS \ + PC_I440FX_1_6_MACHINE_OPTIONS, \ + .hot_add_cpu = NULL + static QEMUMachine pc_i440fx_machine_v1_4 = { + PC_I440FX_1_4_MACHINE_OPTIONS, .name = "pc-i440fx-1.4", - .desc = "Standard PC (i440FX + PIIX, 1996)", .init = pc_init_pci_1_4, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_1_4, { /* end of list */ } }, - DEFAULT_MACHINE_OPTIONS, }; #define PC_COMPAT_1_3 \ @@ -391,15 +394,13 @@ static QEMUMachine pc_i440fx_machine_v1_4 = { } static QEMUMachine pc_machine_v1_3 = { + PC_I440FX_1_4_MACHINE_OPTIONS, .name = "pc-1.3", - .desc = "Standard PC", .init = pc_init_pci_1_3, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_1_3, { /* end of list */ } }, - DEFAULT_MACHINE_OPTIONS, }; #define PC_COMPAT_1_2 \ @@ -430,16 +431,17 @@ static QEMUMachine pc_machine_v1_3 = { .value = "off",\ } +#define PC_I440FX_1_2_MACHINE_OPTIONS \ + PC_I440FX_1_4_MACHINE_OPTIONS, \ + .init = pc_init_pci_1_2 + static QEMUMachine pc_machine_v1_2 = { + PC_I440FX_1_2_MACHINE_OPTIONS, .name = "pc-1.2", - .desc = "Standard PC", - .init = pc_init_pci_1_2, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_1_2, { /* end of list */ } }, - DEFAULT_MACHINE_OPTIONS, }; #define PC_COMPAT_1_1 \ @@ -475,15 +477,12 @@ static QEMUMachine pc_machine_v1_2 = { } static QEMUMachine pc_machine_v1_1 = { + PC_I440FX_1_2_MACHINE_OPTIONS, .name = "pc-1.1", - .desc = "Standard PC", - .init = pc_init_pci_1_2, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_1_1, { /* end of list */ } }, - DEFAULT_MACHINE_OPTIONS, }; #define PC_COMPAT_1_0 \ @@ -507,32 +506,26 @@ static QEMUMachine pc_machine_v1_1 = { } static QEMUMachine pc_machine_v1_0 = { + PC_I440FX_1_2_MACHINE_OPTIONS, .name = "pc-1.0", - .desc = "Standard PC", - .init = pc_init_pci_1_2, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_1_0, { /* end of list */ } }, .hw_version = "1.0", - DEFAULT_MACHINE_OPTIONS, }; #define PC_COMPAT_0_15 \ PC_COMPAT_1_0 static QEMUMachine pc_machine_v0_15 = { + PC_I440FX_1_2_MACHINE_OPTIONS, .name = "pc-0.15", - .desc = "Standard PC", - .init = pc_init_pci_1_2, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_0_15, { /* end of list */ } }, .hw_version = "0.15", - DEFAULT_MACHINE_OPTIONS, }; #define PC_COMPAT_0_14 \ @@ -556,10 +549,8 @@ static QEMUMachine pc_machine_v0_15 = { } static QEMUMachine pc_machine_v0_14 = { + PC_I440FX_1_2_MACHINE_OPTIONS, .name = "pc-0.14", - .desc = "Standard PC", - .init = pc_init_pci_1_2, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_0_14, { @@ -574,7 +565,6 @@ static QEMUMachine pc_machine_v0_14 = { { /* end of list */ } }, .hw_version = "0.14", - DEFAULT_MACHINE_OPTIONS, }; #define PC_COMPAT_0_13 \ @@ -589,11 +579,13 @@ static QEMUMachine pc_machine_v0_14 = { .value = stringify(1),\ } +#define PC_I440FX_0_13_MACHINE_OPTIONS \ + PC_I440FX_1_2_MACHINE_OPTIONS, \ + .init = pc_init_pci_no_kvmclock + static QEMUMachine pc_machine_v0_13 = { + PC_I440FX_0_13_MACHINE_OPTIONS, .name = "pc-0.13", - .desc = "Standard PC", - .init = pc_init_pci_no_kvmclock, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_0_13, { @@ -612,7 +604,6 @@ static QEMUMachine pc_machine_v0_13 = { { /* end of list */ } }, .hw_version = "0.13", - DEFAULT_MACHINE_OPTIONS, }; #define PC_COMPAT_0_12 \ @@ -640,10 +631,8 @@ static QEMUMachine pc_machine_v0_13 = { } static QEMUMachine pc_machine_v0_12 = { + PC_I440FX_0_13_MACHINE_OPTIONS, .name = "pc-0.12", - .desc = "Standard PC", - .init = pc_init_pci_no_kvmclock, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_0_12, { @@ -658,7 +647,6 @@ static QEMUMachine pc_machine_v0_12 = { { /* end of list */ } }, .hw_version = "0.12", - DEFAULT_MACHINE_OPTIONS, }; #define PC_COMPAT_0_11 \ @@ -674,10 +662,8 @@ static QEMUMachine pc_machine_v0_12 = { } static QEMUMachine pc_machine_v0_11 = { + PC_I440FX_0_13_MACHINE_OPTIONS, .name = "pc-0.11", - .desc = "Standard PC, qemu 0.11", - .init = pc_init_pci_no_kvmclock, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_0_11, { @@ -692,14 +678,11 @@ static QEMUMachine pc_machine_v0_11 = { { /* end of list */ } }, .hw_version = "0.11", - DEFAULT_MACHINE_OPTIONS, }; static QEMUMachine pc_machine_v0_10 = { + PC_I440FX_0_13_MACHINE_OPTIONS, .name = "pc-0.10", - .desc = "Standard PC, qemu 0.10", - .init = pc_init_pci_no_kvmclock, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_0_11, { @@ -726,7 +709,6 @@ static QEMUMachine pc_machine_v0_10 = { { /* end of list */ } }, .hw_version = "0.10", - DEFAULT_MACHINE_OPTIONS, }; static QEMUMachine isapc_machine = { diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 198c785..084ecac 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -253,39 +253,41 @@ static void pc_q35_init_1_4(QEMUMachineInitArgs *args) pc_q35_init(args); } +#define PC_Q35_MACHINE_OPTIONS \ + PC_DEFAULT_MACHINE_OPTIONS, \ + .hot_add_cpu = pc_hot_add_cpu + +#define PC_Q35_1_6_MACHINE_OPTIONS PC_Q35_MACHINE_OPTIONS + static QEMUMachine pc_q35_machine_v1_6 = { + PC_Q35_1_6_MACHINE_OPTIONS, .name = "pc-q35-1.6", .alias = "q35", - .desc = "Standard PC (Q35 + ICH9, 2009)", .init = pc_q35_init_1_6, - .hot_add_cpu = pc_hot_add_cpu, - .max_cpus = 255, - DEFAULT_MACHINE_OPTIONS, }; static QEMUMachine pc_q35_machine_v1_5 = { + PC_Q35_1_6_MACHINE_OPTIONS, .name = "pc-q35-1.5", - .desc = "Standard PC (Q35 + ICH9, 2009)", .init = pc_q35_init_1_5, - .hot_add_cpu = pc_hot_add_cpu, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_1_5, { /* end of list */ } }, - DEFAULT_MACHINE_OPTIONS, }; +#define PC_Q35_1_4_MACHINE_OPTIONS \ + PC_Q35_1_6_MACHINE_OPTIONS, \ + .hot_add_cpu = NULL + static QEMUMachine pc_q35_machine_v1_4 = { + PC_Q35_1_4_MACHINE_OPTIONS, .name = "pc-q35-1.4", - .desc = "Standard PC (Q35 + ICH9, 2009)", .init = pc_q35_init_1_4, - .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_1_4, { /* end of list */ } }, - DEFAULT_MACHINE_OPTIONS, }; static void pc_q35_machine_init(void) diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 475ba9e..4a38fad 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -325,4 +325,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t); .value = stringify(0),\ } +#define PC_DEFAULT_MACHINE_OPTIONS \ + DEFAULT_MACHINE_OPTIONS, \ + .hot_add_cpu = pc_hot_add_cpu, \ + .max_cpus = 255 + #endif
We have a lot of code duplication between machine types, this increases with each new machine type and each new field. This has already introduced a minor bug: description for pc-1.3 says "Standard PC" while description for pc-1.4 is "Standard PC (i440FX + PIIX, 1996)" which makes you think 1.3 is somehow more standard, or newer, while in fact it's a revision of the same PC. This patch addresses this issue by using macros, along the lines used by PC_COMPAT_X_X - only for non-property options. Note: this conflicts (trivially) with patch [PATCH v3 6/6] hw: Clean up bogus default boot order by Markus. Sent separately for ease of review. If people like this approach, I can rebase on top of that patch. The approach can extend to non-PC machine types. Cc: Markus Armbruster <armbru@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/i386/pc_piix.c | 82 ++++++++++++++++++++-------------------------------- hw/i386/pc_q35.c | 24 ++++++++------- include/hw/i386/pc.h | 5 ++++ 3 files changed, 50 insertions(+), 61 deletions(-)