| Message ID | 1383137800-2990-3-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: > From: Markus Armbruster <armbru@redhat.com> > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, > no version. Best SeaBIOS can do, but we can provide better defaults: > manufacturer QEMU, product & version taken from QEMUMachine desc and > name. > > Take care to do this only for new machine types, of course. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: > From: Markus Armbruster <armbru@redhat.com> > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, > no version. Best SeaBIOS can do, but we can provide better defaults: > manufacturer QEMU, product & version taken from QEMUMachine desc and > name. > > Take care to do this only for new machine types, of course. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> I feel applying this one would be a mistake. Machine desc is for human readers. For example, it currently says "Standard PC (Q35 + ICH9, 2009)" but if we add a variant with IDE compatibility mode we will likely want to tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" and add another one saying ""Standard PC (Q35 + ICH9/compat mode, 2009)". In other words we want the ability to tweak description retroactively, and exposing it to guest will break this ability. So we really need a new field not tied to the human description. > --- > hw/i386/pc_piix.c | 9 +++++++++ > hw/i386/pc_q35.c | 7 +++++++ > hw/i386/smbios.c | 14 ++++++++++++++ > include/hw/i386/smbios.h | 2 ++ > 4 files changed, 32 insertions(+) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index c6042c7..417ad33 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -28,6 +28,7 @@ > #include "hw/loader.h" > #include "hw/i386/pc.h" > #include "hw/i386/apic.h" > +#include "hw/i386/smbios.h" > #include "hw/pci/pci.h" > #include "hw/pci/pci_ids.h" > #include "hw/usb.h" > @@ -59,6 +60,7 @@ static const int ide_irq[MAX_IDE_BUS] = { 14, 15 }; > > static bool has_pvpanic; > static bool has_pci_info = true; > +static bool smbios_type1_defaults = true; > > /* PC hardware initialisation */ > static void pc_init1(QEMUMachineInitArgs *args, > @@ -124,6 +126,10 @@ static void pc_init1(QEMUMachineInitArgs *args, > guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size); > guest_info->has_pci_info = has_pci_info; > guest_info->isapc_ram_fw = !pci_enabled; > + if (smbios_type1_defaults) { > + smbios_set_type1_defaults("QEMU", args->machine->desc, > + args->machine->name); > + } > > /* allocate ram and load rom/bios */ > if (!xen_enabled()) { > @@ -240,6 +246,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args) > { > has_pci_info = false; > rom_file_in_ram = false; > + smbios_type1_defaults = false; > } > > static void pc_compat_1_5(QEMUMachineInitArgs *args) > @@ -304,6 +311,7 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args) > static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) > { > has_pci_info = false; > + smbios_type1_defaults = false; > disable_kvm_pv_eoi(); > enable_compat_apic_id_mode(); > pc_init1(args, 1, 0); > @@ -312,6 +320,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) > static void pc_init_isa(QEMUMachineInitArgs *args) > { > has_pci_info = false; > + smbios_type1_defaults = false; > if (!args->cpu_model) { > args->cpu_model = "486"; > } > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index ca84e1c..9e3213f 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -39,6 +39,7 @@ > #include "hw/pci-host/q35.h" > #include "exec/address-spaces.h" > #include "hw/i386/ich9.h" > +#include "hw/i386/smbios.h" > #include "hw/ide/pci.h" > #include "hw/ide/ahci.h" > #include "hw/usb.h" > @@ -49,6 +50,7 @@ > > static bool has_pvpanic; > static bool has_pci_info = true; > +static bool smbios_type1_defaults = true; > > /* PC hardware initialisation */ > static void pc_q35_init(QEMUMachineInitArgs *args) > @@ -111,6 +113,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args) > guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size); > guest_info->has_pci_info = has_pci_info; > guest_info->isapc_ram_fw = false; > + if (smbios_type1_defaults) { > + smbios_set_type1_defaults("QEMU", args->machine->desc, > + args->machine->name); > + } > > /* allocate ram and load rom/bios */ > if (!xen_enabled()) { > @@ -224,6 +230,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args) > { > has_pci_info = false; > rom_file_in_ram = false; > + smbios_type1_defaults = false; > } > > static void pc_compat_1_5(QEMUMachineInitArgs *args) > diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c > index d3f1ee6..e8f41ad 100644 > --- a/hw/i386/smbios.c > +++ b/hw/i386/smbios.c > @@ -256,6 +256,20 @@ static void smbios_build_type_1_fields(void) > } > } > > +void smbios_set_type1_defaults(const char *manufacturer, > + const char *product, const char *version) > +{ > + if (!type1.manufacturer) { > + type1.manufacturer = manufacturer; > + } > + if (!type1.product) { > + type1.product = product; > + } > + if (!type1.version) { > + type1.version = version; > + } > +} > + > uint8_t *smbios_get_table(size_t *length) > { > if (!smbios_immutable) { > diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h > index b08ec71..18fb970 100644 > --- a/include/hw/i386/smbios.h > +++ b/include/hw/i386/smbios.h > @@ -16,6 +16,8 @@ > #include "qemu/option.h" > > void smbios_entry_add(QemuOpts *opts); > +void smbios_set_type1_defaults(const char *manufacturer, > + const char *product, const char *version); > uint8_t *smbios_get_table(size_t *length); > > /* > -- > 1.8.1.4
On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote: > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: > > From: Markus Armbruster <armbru@redhat.com> > > > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, > > no version. Best SeaBIOS can do, but we can provide better defaults: > > manufacturer QEMU, product & version taken from QEMUMachine desc and > > name. > > > > Take care to do this only for new machine types, of course. > > > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > > I feel applying this one would be a mistake. > > Machine desc is for human readers. > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" > but if we add a variant with IDE compatibility mode we will likely want to > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, > 2009)". > > In other words we want the ability to tweak > description retroactively, and exposing it to guest will > break this ability. > > So we really need a new field not tied to the human description. > You have a point, but if we do that one day, then we can add a new smbios-specific field and set it for each of the existing machine-types so they keep the same ABI. This patch doesn't make us unable to do that in the future. As we are past hard freeze, I think this simple patch is better than a more complex solution for a problem we still don't have (that can still be implemented in 1.8).
On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote: > On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote: > > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: > > > From: Markus Armbruster <armbru@redhat.com> > > > > > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, > > > no version. Best SeaBIOS can do, but we can provide better defaults: > > > manufacturer QEMU, product & version taken from QEMUMachine desc and > > > name. > > > > > > Take care to do this only for new machine types, of course. > > > > > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > > > > I feel applying this one would be a mistake. > > > > Machine desc is for human readers. > > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" > > but if we add a variant with IDE compatibility mode we will likely want to > > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" > > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, > > 2009)". > > > > In other words we want the ability to tweak > > description retroactively, and exposing it to guest will > > break this ability. > > > > So we really need a new field not tied to the human description. > > > > You have a point, but if we do that one day, then we can add a new > smbios-specific field and set it for each of the existing machine-types > so they keep the same ABI. This patch doesn't make us unable to do that > in the future. We'll likely forget and just break guest ABI. So we really need a unit test for this, too. > As we are past hard freeze, I think this simple patch is better than a > more complex solution for a problem we still don't have (that can still > be implemented in 1.8). I don't see why we need to rush this into 1.7. Downstreams want their info in smbios for support, branding etc, but I don't see a burning need for this in upstream QEMU. It's kind of nice to have it say "QEMU", but we can afford to delay and do it properly for 1.8. > -- > Eduardo
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: >> From: Markus Armbruster <armbru@redhat.com> >> >> Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, >> no version. Best SeaBIOS can do, but we can provide better defaults: >> manufacturer QEMU, product & version taken from QEMUMachine desc and >> name. >> >> Take care to do this only for new machine types, of course. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > I feel applying this one would be a mistake. > > Machine desc is for human readers. > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" > but if we add a variant with IDE compatibility mode we will likely want to > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, > 2009)". > > In other words we want the ability to tweak > description retroactively, and exposing it to guest will > break this ability. These will be new machine types, won't they? > So we really need a new field not tied to the human description. The SMBIOS string is *also* for human readers. I can't see why we should jump through hoops *now* to separate the two. The values are the same. I don't expect us to change them for old machine types. But if we ever feel the need to change them in one place but not the other, nothing will prevent us from splitting them up then. Pay as you go, not pay as you fear you might have to go some day.
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote: >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote: >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: >> > > From: Markus Armbruster <armbru@redhat.com> >> > > >> > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, >> > > no version. Best SeaBIOS can do, but we can provide better defaults: >> > > manufacturer QEMU, product & version taken from QEMUMachine desc and >> > > name. >> > > >> > > Take care to do this only for new machine types, of course. >> > > >> > > Signed-off-by: Markus Armbruster <armbru@redhat.com> >> > >> > I feel applying this one would be a mistake. >> > >> > Machine desc is for human readers. >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" >> > but if we add a variant with IDE compatibility mode we will likely want to >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, >> > 2009)". >> > >> > In other words we want the ability to tweak >> > description retroactively, and exposing it to guest will >> > break this ability. >> > >> > So we really need a new field not tied to the human description. >> > >> >> You have a point, but if we do that one day, then we can add a new >> smbios-specific field and set it for each of the existing machine-types >> so they keep the same ABI. This patch doesn't make us unable to do that >> in the future. > > We'll likely forget and just break guest ABI. > So we really need a unit test for this, too. More tests are good, but we I think we got bigger fish to fry than writing tests to catch changes that are so obviously foolish as messing with old machine type's QEMUMachine. >> As we are past hard freeze, I think this simple patch is better than a >> more complex solution for a problem we still don't have (that can still >> be implemented in 1.8). > > I don't see why we need to rush this into 1.7. > Downstreams want their info in smbios for support, branding etc, > but I don't see a burning need for this in upstream QEMU. > It's kind of nice to have it say "QEMU", but we can afford to > delay and do it properly for 1.8. Define "properly". I don't see what I'd like to do differently for 1.8.
On Wed, Oct 30, 2013 at 03:48:55PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: > >> From: Markus Armbruster <armbru@redhat.com> > >> > >> Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, > >> no version. Best SeaBIOS can do, but we can provide better defaults: > >> manufacturer QEMU, product & version taken from QEMUMachine desc and > >> name. > >> > >> Take care to do this only for new machine types, of course. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > > > I feel applying this one would be a mistake. > > > > Machine desc is for human readers. > > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" > > but if we add a variant with IDE compatibility mode we will likely want to > > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" > > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, > > 2009)". > > > > In other words we want the ability to tweak > > description retroactively, and exposing it to guest will > > break this ability. > > These will be new machine types, won't they? Description is there for humans, so we should be free to change it to make it more readable for old types, too. > > So we really need a new field not tied to the human description. > > The SMBIOS string is *also* for human readers. > > I can't see why we should jump through hoops *now* to separate the two. > The values are the same. I don't expect us to change them for old > machine types. But if we ever feel the need to change them in one place > but not the other, nothing will prevent us from splitting them up then. > > Pay as you go, not pay as you fear you might have to go some day.
On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote: > >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote: > >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: > >> > > From: Markus Armbruster <armbru@redhat.com> > >> > > > >> > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, > >> > > no version. Best SeaBIOS can do, but we can provide better defaults: > >> > > manufacturer QEMU, product & version taken from QEMUMachine desc and > >> > > name. > >> > > > >> > > Take care to do this only for new machine types, of course. > >> > > > >> > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> > > >> > I feel applying this one would be a mistake. > >> > > >> > Machine desc is for human readers. > >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" > >> > but if we add a variant with IDE compatibility mode we will likely want to > >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" > >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, > >> > 2009)". > >> > > >> > In other words we want the ability to tweak > >> > description retroactively, and exposing it to guest will > >> > break this ability. > >> > > >> > So we really need a new field not tied to the human description. > >> > > >> > >> You have a point, but if we do that one day, then we can add a new > >> smbios-specific field and set it for each of the existing machine-types > >> so they keep the same ABI. This patch doesn't make us unable to do that > >> in the future. > > > > We'll likely forget and just break guest ABI. > > So we really need a unit test for this, too. > > More tests are good, but we I think we got bigger fish to fry than > writing tests to catch changes that are so obviously foolish as messing > with old machine type's QEMUMachine. You "messed with" old machine type's QEMUMachine in your cleanup patches too, didn't you? > >> As we are past hard freeze, I think this simple patch is better than a > >> more complex solution for a problem we still don't have (that can still > >> be implemented in 1.8). > > > > I don't see why we need to rush this into 1.7. > > Downstreams want their info in smbios for support, branding etc, > > but I don't see a burning need for this in upstream QEMU. > > It's kind of nice to have it say "QEMU", but we can afford to > > delay and do it properly for 1.8. > > Define "properly". I don't see what I'd like to do differently for 1.8. With proper tests going in first before we start changing things. Ideally with better separation between user visible and guest visible interfaces - though if there was a test to catch guest visible changes, I would be less worried about this lack of separation.
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >> > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote: >> >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote: >> >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: >> >> > > From: Markus Armbruster <armbru@redhat.com> >> >> > > >> >> > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, >> >> > > no version. Best SeaBIOS can do, but we can provide better defaults: >> >> > > manufacturer QEMU, product & version taken from QEMUMachine desc and >> >> > > name. >> >> > > >> >> > > Take care to do this only for new machine types, of course. >> >> > > >> >> > > Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> > >> >> > I feel applying this one would be a mistake. >> >> > >> >> > Machine desc is for human readers. >> >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" >> >> > but if we add a variant with IDE compatibility mode we will >> >> > likely want to >> >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" >> >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, >> >> > 2009)". >> >> > >> >> > In other words we want the ability to tweak >> >> > description retroactively, and exposing it to guest will >> >> > break this ability. >> >> > >> >> > So we really need a new field not tied to the human description. >> >> > >> >> >> >> You have a point, but if we do that one day, then we can add a new >> >> smbios-specific field and set it for each of the existing machine-types >> >> so they keep the same ABI. This patch doesn't make us unable to do that >> >> in the future. >> > >> > We'll likely forget and just break guest ABI. >> > So we really need a unit test for this, too. >> >> More tests are good, but we I think we got bigger fish to fry than >> writing tests to catch changes that are so obviously foolish as messing >> with old machine type's QEMUMachine. > > You "messed with" old machine type's QEMUMachine in your cleanup > patches too, didn't you? I've occasionally touched QEMUMachine initializers in cleanup series, but nothing as frivolous as changing strings. And I can't find anything as frivolous as that in git. We *are* careful and conservative there. >> >> As we are past hard freeze, I think this simple patch is better than a >> >> more complex solution for a problem we still don't have (that can still >> >> be implemented in 1.8). >> > >> > I don't see why we need to rush this into 1.7. >> > Downstreams want their info in smbios for support, branding etc, >> > but I don't see a burning need for this in upstream QEMU. >> > It's kind of nice to have it say "QEMU", but we can afford to >> > delay and do it properly for 1.8. >> >> Define "properly". I don't see what I'd like to do differently for 1.8. > > With proper tests going in first before we start changing things. > Ideally with better separation between user visible and guest visible > interfaces - though if there was a test to catch guest visible changes, > I would be less worried about this lack of separation. You want me to test for unlikely developer mistakes that are far easier to catch in review than most other guest ABI changes, and far less harmful than pretty much any other guest ABI change. This would multiply the size of this mini-series by a significant factor. I can't justify this in good conscience to my (and your) employer. So this isn't going to happen. If the maintainers agree with you, then I wasted my time. Sad, but I'd rather write off the work I've already done than do much more work of no particular value just to save it.
On Wed, Oct 30, 2013 at 09:22:38PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> writes: > >> > >> > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote: > >> >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote: > >> >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: > >> >> > > From: Markus Armbruster <armbru@redhat.com> > >> >> > > > >> >> > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, > >> >> > > no version. Best SeaBIOS can do, but we can provide better defaults: > >> >> > > manufacturer QEMU, product & version taken from QEMUMachine desc and > >> >> > > name. > >> >> > > > >> >> > > Take care to do this only for new machine types, of course. > >> >> > > > >> >> > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> >> > > >> >> > I feel applying this one would be a mistake. > >> >> > > >> >> > Machine desc is for human readers. > >> >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" > >> >> > but if we add a variant with IDE compatibility mode we will > >> >> > likely want to > >> >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" > >> >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, > >> >> > 2009)". > >> >> > > >> >> > In other words we want the ability to tweak > >> >> > description retroactively, and exposing it to guest will > >> >> > break this ability. > >> >> > > >> >> > So we really need a new field not tied to the human description. > >> >> > > >> >> > >> >> You have a point, but if we do that one day, then we can add a new > >> >> smbios-specific field and set it for each of the existing machine-types > >> >> so they keep the same ABI. This patch doesn't make us unable to do that > >> >> in the future. > >> > > >> > We'll likely forget and just break guest ABI. > >> > So we really need a unit test for this, too. > >> > >> More tests are good, but we I think we got bigger fish to fry than > >> writing tests to catch changes that are so obviously foolish as messing > >> with old machine type's QEMUMachine. > > > > You "messed with" old machine type's QEMUMachine in your cleanup > > patches too, didn't you? > > I've occasionally touched QEMUMachine initializers in cleanup series, > but nothing as frivolous as changing strings. And I can't find anything > as frivolous as that in git. We *are* careful and conservative there. > > >> >> As we are past hard freeze, I think this simple patch is better than a > >> >> more complex solution for a problem we still don't have (that can still > >> >> be implemented in 1.8). > >> > > >> > I don't see why we need to rush this into 1.7. > >> > Downstreams want their info in smbios for support, branding etc, > >> > but I don't see a burning need for this in upstream QEMU. > >> > It's kind of nice to have it say "QEMU", but we can afford to > >> > delay and do it properly for 1.8. > >> > >> Define "properly". I don't see what I'd like to do differently for 1.8. > > > > With proper tests going in first before we start changing things. > > Ideally with better separation between user visible and guest visible > > interfaces - though if there was a test to catch guest visible changes, > > I would be less worried about this lack of separation. > > You want me to test for unlikely developer mistakes that are far easier > to catch in review than most other guest ABI changes, and far less > harmful than pretty much any other guest ABI change. This would > multiply the size of this mini-series by a significant factor. I can't > justify this in good conscience to my (and your) employer. So this > isn't going to happen. > > If the maintainers agree with you, then I wasted my time. Sad, but I'd > rather write off the work I've already done than do much more work of no > particular value just to save it. It would be of no particular value *if we only test these strings*. But testing smbios generally has a lot of value IMHO.
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Oct 30, 2013 at 09:22:38PM +0100, Markus Armbruster wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >> > On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote: >> >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >> >> >> > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote: >> >> >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote: >> >> >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: >> >> >> > > From: Markus Armbruster <armbru@redhat.com> >> >> >> > > >> >> >> > > Currently, we get SeaBIOS defaults: manufacturer Bochs, >> >> >> > > product Bochs, >> >> >> > > no version. Best SeaBIOS can do, but we can provide >> >> >> > > better defaults: >> >> >> > > manufacturer QEMU, product & version taken from QEMUMachine desc and >> >> >> > > name. >> >> >> > > >> >> >> > > Take care to do this only for new machine types, of course. >> >> >> > > >> >> >> > > Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> >> > >> >> >> > I feel applying this one would be a mistake. >> >> >> > >> >> >> > Machine desc is for human readers. >> >> >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" >> >> >> > but if we add a variant with IDE compatibility mode we will >> >> >> > likely want to >> >> >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" >> >> >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, >> >> >> > 2009)". >> >> >> > >> >> >> > In other words we want the ability to tweak >> >> >> > description retroactively, and exposing it to guest will >> >> >> > break this ability. >> >> >> > >> >> >> > So we really need a new field not tied to the human description. >> >> >> > >> >> >> >> >> >> You have a point, but if we do that one day, then we can add a new >> >> >> smbios-specific field and set it for each of the existing machine-types >> >> >> so they keep the same ABI. This patch doesn't make us unable to do that >> >> >> in the future. >> >> > >> >> > We'll likely forget and just break guest ABI. >> >> > So we really need a unit test for this, too. >> >> >> >> More tests are good, but we I think we got bigger fish to fry than >> >> writing tests to catch changes that are so obviously foolish as messing >> >> with old machine type's QEMUMachine. >> > >> > You "messed with" old machine type's QEMUMachine in your cleanup >> > patches too, didn't you? >> >> I've occasionally touched QEMUMachine initializers in cleanup series, >> but nothing as frivolous as changing strings. And I can't find anything >> as frivolous as that in git. We *are* careful and conservative there. >> >> >> >> As we are past hard freeze, I think this simple patch is better than a >> >> >> more complex solution for a problem we still don't have (that can still >> >> >> be implemented in 1.8). >> >> > >> >> > I don't see why we need to rush this into 1.7. >> >> > Downstreams want their info in smbios for support, branding etc, >> >> > but I don't see a burning need for this in upstream QEMU. >> >> > It's kind of nice to have it say "QEMU", but we can afford to >> >> > delay and do it properly for 1.8. >> >> >> >> Define "properly". I don't see what I'd like to do differently for 1.8. >> > >> > With proper tests going in first before we start changing things. >> > Ideally with better separation between user visible and guest visible >> > interfaces - though if there was a test to catch guest visible changes, >> > I would be less worried about this lack of separation. >> >> You want me to test for unlikely developer mistakes that are far easier >> to catch in review than most other guest ABI changes, and far less >> harmful than pretty much any other guest ABI change. This would >> multiply the size of this mini-series by a significant factor. I can't >> justify this in good conscience to my (and your) employer. So this >> isn't going to happen. >> >> If the maintainers agree with you, then I wasted my time. Sad, but I'd >> rather write off the work I've already done than do much more work of no >> particular value just to save it. > > It would be of no particular value *if we only test these strings*. > But testing smbios generally has a lot of value IMHO. Yes, it has value, but you're not going to succeed into blackmailing me to do that work now.
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: >> From: Markus Armbruster <armbru@redhat.com> >> >> Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, >> no version. Best SeaBIOS can do, but we can provide better defaults: >> manufacturer QEMU, product & version taken from QEMUMachine desc and >> name. >> >> Take care to do this only for new machine types, of course. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > I feel applying this one would be a mistake. > > Machine desc is for human readers. > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" > but if we add a variant with IDE compatibility mode we will likely want to > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, > 2009)". > > In other words we want the ability to tweak > description retroactively, and exposing it to guest will > break this ability. > > So we really need a new field not tied to the human description. Can you live with the separation I just posted as PATCH 3/2?
On Thu, Oct 31, 2013 at 06:54:37AM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: > >> From: Markus Armbruster <armbru@redhat.com> > >> > >> Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, > >> no version. Best SeaBIOS can do, but we can provide better defaults: > >> manufacturer QEMU, product & version taken from QEMUMachine desc and > >> name. > >> > >> Take care to do this only for new machine types, of course. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > > > I feel applying this one would be a mistake. > > > > Machine desc is for human readers. > > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" > > but if we add a variant with IDE compatibility mode we will likely want to > > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" > > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, > > 2009)". > > > > In other words we want the ability to tweak > > description retroactively, and exposing it to guest will > > break this ability. > > > > So we really need a new field not tied to the human description. > > Can you live with the separation I just posted as PATCH 3/2? Yes, I think that's a good way to do this.
On Wed, Oct 30, 2013 at 09:22:38PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> writes: > >> > >> > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote: > >> >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote: > >> >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, armbru@redhat.com wrote: > >> >> > > From: Markus Armbruster <armbru@redhat.com> > >> >> > > > >> >> > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs, > >> >> > > no version. Best SeaBIOS can do, but we can provide better defaults: > >> >> > > manufacturer QEMU, product & version taken from QEMUMachine desc and > >> >> > > name. > >> >> > > > >> >> > > Take care to do this only for new machine types, of course. > >> >> > > > >> >> > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> >> > > >> >> > I feel applying this one would be a mistake. > >> >> > > >> >> > Machine desc is for human readers. > >> >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)" > >> >> > but if we add a variant with IDE compatibility mode we will > >> >> > likely want to > >> >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)" > >> >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode, > >> >> > 2009)". > >> >> > > >> >> > In other words we want the ability to tweak > >> >> > description retroactively, and exposing it to guest will > >> >> > break this ability. > >> >> > > >> >> > So we really need a new field not tied to the human description. > >> >> > > >> >> > >> >> You have a point, but if we do that one day, then we can add a new > >> >> smbios-specific field and set it for each of the existing machine-types > >> >> so they keep the same ABI. This patch doesn't make us unable to do that > >> >> in the future. > >> > > >> > We'll likely forget and just break guest ABI. > >> > So we really need a unit test for this, too. > >> > >> More tests are good, but we I think we got bigger fish to fry than > >> writing tests to catch changes that are so obviously foolish as messing > >> with old machine type's QEMUMachine. > > > > You "messed with" old machine type's QEMUMachine in your cleanup > > patches too, didn't you? > > I've occasionally touched QEMUMachine initializers in cleanup series, > but nothing as frivolous as changing strings. And I can't find anything > as frivolous as that in git. We *are* careful and conservative there. Actually, we changed .desc for old machine types before. See commit a0dba644c139907ccf6735c505fbd254010d6938.
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index c6042c7..417ad33 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -28,6 +28,7 @@ #include "hw/loader.h" #include "hw/i386/pc.h" #include "hw/i386/apic.h" +#include "hw/i386/smbios.h" #include "hw/pci/pci.h" #include "hw/pci/pci_ids.h" #include "hw/usb.h" @@ -59,6 +60,7 @@ static const int ide_irq[MAX_IDE_BUS] = { 14, 15 }; static bool has_pvpanic; static bool has_pci_info = true; +static bool smbios_type1_defaults = true; /* PC hardware initialisation */ static void pc_init1(QEMUMachineInitArgs *args, @@ -124,6 +126,10 @@ static void pc_init1(QEMUMachineInitArgs *args, guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size); guest_info->has_pci_info = has_pci_info; guest_info->isapc_ram_fw = !pci_enabled; + if (smbios_type1_defaults) { + smbios_set_type1_defaults("QEMU", args->machine->desc, + args->machine->name); + } /* allocate ram and load rom/bios */ if (!xen_enabled()) { @@ -240,6 +246,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args) { has_pci_info = false; rom_file_in_ram = false; + smbios_type1_defaults = false; } static void pc_compat_1_5(QEMUMachineInitArgs *args) @@ -304,6 +311,7 @@ static void pc_init_pci_1_2(QEMUMachineInitArgs *args) static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) { has_pci_info = false; + smbios_type1_defaults = false; disable_kvm_pv_eoi(); enable_compat_apic_id_mode(); pc_init1(args, 1, 0); @@ -312,6 +320,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) static void pc_init_isa(QEMUMachineInitArgs *args) { has_pci_info = false; + smbios_type1_defaults = false; if (!args->cpu_model) { args->cpu_model = "486"; } diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index ca84e1c..9e3213f 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -39,6 +39,7 @@ #include "hw/pci-host/q35.h" #include "exec/address-spaces.h" #include "hw/i386/ich9.h" +#include "hw/i386/smbios.h" #include "hw/ide/pci.h" #include "hw/ide/ahci.h" #include "hw/usb.h" @@ -49,6 +50,7 @@ static bool has_pvpanic; static bool has_pci_info = true; +static bool smbios_type1_defaults = true; /* PC hardware initialisation */ static void pc_q35_init(QEMUMachineInitArgs *args) @@ -111,6 +113,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args) guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size); guest_info->has_pci_info = has_pci_info; guest_info->isapc_ram_fw = false; + if (smbios_type1_defaults) { + smbios_set_type1_defaults("QEMU", args->machine->desc, + args->machine->name); + } /* allocate ram and load rom/bios */ if (!xen_enabled()) { @@ -224,6 +230,7 @@ static void pc_compat_1_6(QEMUMachineInitArgs *args) { has_pci_info = false; rom_file_in_ram = false; + smbios_type1_defaults = false; } static void pc_compat_1_5(QEMUMachineInitArgs *args) diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c index d3f1ee6..e8f41ad 100644 --- a/hw/i386/smbios.c +++ b/hw/i386/smbios.c @@ -256,6 +256,20 @@ static void smbios_build_type_1_fields(void) } } +void smbios_set_type1_defaults(const char *manufacturer, + const char *product, const char *version) +{ + if (!type1.manufacturer) { + type1.manufacturer = manufacturer; + } + if (!type1.product) { + type1.product = product; + } + if (!type1.version) { + type1.version = version; + } +} + uint8_t *smbios_get_table(size_t *length) { if (!smbios_immutable) { diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h index b08ec71..18fb970 100644 --- a/include/hw/i386/smbios.h +++ b/include/hw/i386/smbios.h @@ -16,6 +16,8 @@ #include "qemu/option.h" void smbios_entry_add(QemuOpts *opts); +void smbios_set_type1_defaults(const char *manufacturer, + const char *product, const char *version); uint8_t *smbios_get_table(size_t *length); /*