Re: [PATCH v3 1/2] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models

[Suthikulpanit, Suravee]

Michael,

On 6/4/2023 7:55 PM, Michael S. Tsirkin wrote:
> On Fri, Jun 02, 2023 at 10:22:54PM -0500, Suravee Suthikulpanit wrote:
> > ....
> >
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -199,6 +199,14 @@ static void pc_q35_init(MachineState *machine)
> >       pc_guest_info_init(pcms);
> >   
> >       if (pcmc->smbios_defaults) {
> > +        /*
> > +         * Check if user has specified command line option to override
> > +         * the default SMBIOS default entry point type.
> > +         */
> > +        if (!pcms->smbios_use_cmdline_ep_type) {
> > +            pcms->smbios_entry_point_type = pcmc->default_smbios_ep_type;
> > +        }
> > +
> >           /* These values are guest ABI, do not change */
> >           smbios_set_defaults("QEMU", mc->desc,
> >                               mc->name, pcmc->smbios_legacy_mode,
> > @@ -359,6 +367,7 @@ static void pc_q35_machine_options(MachineClass *m)
> >       PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> >       pcmc->pci_root_uid = 0;
> >       pcmc->default_cpu_version = 1;
> > +    pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_64;
> >   
> >       m->family = "pc_q35";
> >       m->desc = "Standard PC (Q35 + ICH9, 2009)";
> > @@ -387,10 +396,15 @@ DEFINE_Q35_MACHINE(v8_1, "pc-q35-8.1", NULL,
> >   
> >   static void pc_q35_8_0_machine_options(MachineClass *m)
> >   {
> > +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> > +
> >       pc_q35_8_1_machine_options(m);
> >       m->alias = NULL;
> >       compat_props_add(m->compat_props, hw_compat_8_0, hw_compat_8_0_len);
> >       compat_props_add(m->compat_props, pc_compat_8_0, pc_compat_8_0_len);
> > +
> > +    /* For pc-q35-8.0 and older, use SMBIOS 2.8 by default */
> > +    pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_32;
> >   }
> >   
> >   DEFINE_Q35_MACHINE(v8_0, "pc-q35-8.0", NULL,
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index c661e9cc80..f754da5a38 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -50,6 +50,7 @@ typedef struct PCMachineState {
> >       bool i8042_enabled;
> >       bool default_bus_bypass_iommu;
> >       uint64_t max_fw_size;
> > +    bool smbios_use_cmdline_ep_type;
> >   
> >       /* ACPI Memory hotplug IO base address */
> >       hwaddr memhp_io_base;
> > @@ -110,6 +111,7 @@ struct PCMachineClass {
> >       bool smbios_defaults;
> >       bool smbios_legacy_mode;
> >       bool smbios_uuid_encoded;
> > +    SmbiosEntryPointType default_smbios_ep_type;
> >   
> >       /* RAM / address space compat: */
> >       bool gigabyte_align;
>
>
> Can't we avoid this code duplication?
>
> E.g. can't we use the pc_compat_8_0 machinery?

I think we can. I have just submitted v4 with some code refactoring to 
avoid duplication when set up SMBIOS defaults.

Thanks,
Suravee