| Message ID | 20230607024939.703991-2-suravee.suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/i386/pc: Update max_cpus and default to SMBIOS | expand |
On 7/6/23 04:49, Suravee Suthikulpanit wrote: "Refactor logic to set SMBIOS defaults" > Into a helper function pc_machine_init_smbios() in preparation for > subsequent code to upgrade default SMBIOS entry point type. > > Then, call the helper function from the pc_machine_initfn() to eliminate > duplicate code in pc_q35.c and pc_pixx.c. However, this changes the > ordering of when the smbios_set_defaults() is called to before > pc_machine_set_smbios_ep() (i.e. before handling the user specified > QEMU option "-M ...,smbios-entry-point-type=[32|64]" to override > the default type.) > > Therefore, also call the helper function in pc_machine_set_smbios_ep() > to update the defaults. > > There is no functional change. > > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > --- > hw/i386/pc.c | 24 +++++++++++++++++++++++- > hw/i386/pc_piix.c | 9 --------- > hw/i386/pc_q35.c | 8 -------- > 3 files changed, 23 insertions(+), 18 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Tue, Jun 06, 2023 at 09:49:37PM -0500, Suravee Suthikulpanit wrote: > Into a helper function pc_machine_init_smbios() in preparation for > subsequent code to upgrade default SMBIOS entry point type. > > Then, call the helper function from the pc_machine_initfn() to eliminate > duplicate code in pc_q35.c and pc_pixx.c. However, this changes the > ordering of when the smbios_set_defaults() is called to before > pc_machine_set_smbios_ep() (i.e. before handling the user specified > QEMU option "-M ...,smbios-entry-point-type=[32|64]" to override > the default type.) > > Therefore, also call the helper function in pc_machine_set_smbios_ep() > to update the defaults. This is unsafe - smbios_set_defaults is only intended to be called once. Calling it twice leads to a SEGV due to double-free $ ./build/qemu-system-x86_64 -machine pc,smbios-entry-point-type=64 -smbios file=/tmp/smbios_entry_point Segmentation fault (core dumped) IMHO we should just not do this refactoring. The existing duplicated code is not a significant burden, and thus is better than having to workaround calling pc_machine_set_smbios_ep too early in startup. > > There is no functional change. > > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > --- > hw/i386/pc.c | 24 +++++++++++++++++++++++- > hw/i386/pc_piix.c | 9 --------- > hw/i386/pc_q35.c | 8 -------- > 3 files changed, 23 insertions(+), 18 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index bb62c994fa..b720dc67b6 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1756,6 +1756,22 @@ static void pc_machine_set_default_bus_bypass_iommu(Object *obj, bool value, > pcms->default_bus_bypass_iommu = value; > } > > +static void pc_machine_init_smbios(PCMachineState *pcms) > +{ > + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > + MachineClass *mc = MACHINE_GET_CLASS(pcms); > + > + if (!pcmc->smbios_defaults) { > + return; > + } > + > + /* These values are guest ABI, do not change */ > + smbios_set_defaults("QEMU", mc->desc, > + mc->name, pcmc->smbios_legacy_mode, > + pcmc->smbios_uuid_encoded, > + pcms->smbios_entry_point_type); > +} > + > static void pc_machine_get_smbios_ep(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > @@ -1768,9 +1784,14 @@ static void pc_machine_get_smbios_ep(Object *obj, Visitor *v, const char *name, > static void pc_machine_set_smbios_ep(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > + SmbiosEntryPointType ep_type; > PCMachineState *pcms = PC_MACHINE(obj); > > - visit_type_SmbiosEntryPointType(v, name, &pcms->smbios_entry_point_type, errp); > + if (!visit_type_SmbiosEntryPointType(v, name, &ep_type, errp)) { > + return; > + } > + pcms->smbios_entry_point_type = ep_type; > + pc_machine_init_smbios(pcms); > } > > static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v, > @@ -1878,6 +1899,7 @@ static void pc_machine_initfn(Object *obj) > object_property_add_alias(OBJECT(pcms), "pcspk-audiodev", > OBJECT(pcms->pcspk), "audiodev"); > cxl_machine_init(obj, &pcms->cxl_devices_state); > + pc_machine_init_smbios(pcms); > } > > int pc_machine_kvm_type(MachineState *machine, const char *kvm_type) > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index d5b0dcd1fe..da6ba4eeb4 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -198,15 +198,6 @@ static void pc_init1(MachineState *machine, > > pc_guest_info_init(pcms); > > - if (pcmc->smbios_defaults) { > - MachineClass *mc = MACHINE_GET_CLASS(machine); > - /* These values are guest ABI, do not change */ > - smbios_set_defaults("QEMU", mc->desc, > - mc->name, pcmc->smbios_legacy_mode, > - pcmc->smbios_uuid_encoded, > - pcms->smbios_entry_point_type); > - } > - > /* allocate ram and load rom/bios */ > if (!xen_enabled()) { > pc_memory_init(pcms, system_memory, rom_memory, hole64_size); > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 6155427e48..a58cd1d3ea 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -198,14 +198,6 @@ static void pc_q35_init(MachineState *machine) > > pc_guest_info_init(pcms); > > - if (pcmc->smbios_defaults) { > - /* These values are guest ABI, do not change */ > - smbios_set_defaults("QEMU", mc->desc, > - mc->name, pcmc->smbios_legacy_mode, > - pcmc->smbios_uuid_encoded, > - pcms->smbios_entry_point_type); > - } > - > /* create pci host bus */ > q35_host = Q35_HOST_DEVICE(qdev_new(TYPE_Q35_HOST_DEVICE)); > > -- > 2.34.1 > With regards, Daniel
On 7/6/23 10:11, Daniel P. Berrangé wrote: > On Tue, Jun 06, 2023 at 09:49:37PM -0500, Suravee Suthikulpanit wrote: >> Into a helper function pc_machine_init_smbios() in preparation for >> subsequent code to upgrade default SMBIOS entry point type. >> >> Then, call the helper function from the pc_machine_initfn() to eliminate >> duplicate code in pc_q35.c and pc_pixx.c. However, this changes the >> ordering of when the smbios_set_defaults() is called to before >> pc_machine_set_smbios_ep() (i.e. before handling the user specified >> QEMU option "-M ...,smbios-entry-point-type=[32|64]" to override >> the default type.) >> >> Therefore, also call the helper function in pc_machine_set_smbios_ep() >> to update the defaults. > > This is unsafe - smbios_set_defaults is only intended to be called > once. Calling it twice leads to a SEGV due to double-free > > $ ./build/qemu-system-x86_64 -machine pc,smbios-entry-point-type=64 -smbios file=/tmp/smbios_entry_point > Segmentation fault (core dumped) Doh, good catch. > IMHO we should just not do this refactoring. The existing duplicated > code is not a significant burden, and thus is better than having to > workaround calling pc_machine_set_smbios_ep too early in startup. > >> >> There is no functional change. I was too confident because of this line ... >> >> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> >> --- >> hw/i386/pc.c | 24 +++++++++++++++++++++++- >> hw/i386/pc_piix.c | 9 --------- >> hw/i386/pc_q35.c | 8 -------- >> 3 files changed, 23 insertions(+), 18 deletions(-)
On Tue, 6 Jun 2023 21:49:37 -0500 Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote: > Into a helper function pc_machine_init_smbios() in preparation for > subsequent code to upgrade default SMBIOS entry point type. > > Then, call the helper function from the pc_machine_initfn() to eliminate > duplicate code in pc_q35.c and pc_pixx.c. However, this changes the > ordering of when the smbios_set_defaults() is called to before > pc_machine_set_smbios_ep() (i.e. before handling the user specified > QEMU option "-M ...,smbios-entry-point-type=[32|64]" to override > the default type.) > > Therefore, also call the helper function in pc_machine_set_smbios_ep() > to update the defaults. > > There is no functional change. with 2/3 amended as suggested, this patch is not necessary and 2/3 and 3/3 would do the job just fine > > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > --- > hw/i386/pc.c | 24 +++++++++++++++++++++++- > hw/i386/pc_piix.c | 9 --------- > hw/i386/pc_q35.c | 8 -------- > 3 files changed, 23 insertions(+), 18 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index bb62c994fa..b720dc67b6 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1756,6 +1756,22 @@ static void pc_machine_set_default_bus_bypass_iommu(Object *obj, bool value, > pcms->default_bus_bypass_iommu = value; > } > > +static void pc_machine_init_smbios(PCMachineState *pcms) > +{ > + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > + MachineClass *mc = MACHINE_GET_CLASS(pcms); > + > + if (!pcmc->smbios_defaults) { > + return; > + } > + > + /* These values are guest ABI, do not change */ > + smbios_set_defaults("QEMU", mc->desc, > + mc->name, pcmc->smbios_legacy_mode, > + pcmc->smbios_uuid_encoded, > + pcms->smbios_entry_point_type); > +} > + > static void pc_machine_get_smbios_ep(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > @@ -1768,9 +1784,14 @@ static void pc_machine_get_smbios_ep(Object *obj, Visitor *v, const char *name, > static void pc_machine_set_smbios_ep(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > + SmbiosEntryPointType ep_type; > PCMachineState *pcms = PC_MACHINE(obj); > > - visit_type_SmbiosEntryPointType(v, name, &pcms->smbios_entry_point_type, errp); > + if (!visit_type_SmbiosEntryPointType(v, name, &ep_type, errp)) { > + return; > + } > + pcms->smbios_entry_point_type = ep_type; > + pc_machine_init_smbios(pcms); > } > > static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v, > @@ -1878,6 +1899,7 @@ static void pc_machine_initfn(Object *obj) > object_property_add_alias(OBJECT(pcms), "pcspk-audiodev", > OBJECT(pcms->pcspk), "audiodev"); > cxl_machine_init(obj, &pcms->cxl_devices_state); > + pc_machine_init_smbios(pcms); > } > > int pc_machine_kvm_type(MachineState *machine, const char *kvm_type) > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index d5b0dcd1fe..da6ba4eeb4 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -198,15 +198,6 @@ static void pc_init1(MachineState *machine, > > pc_guest_info_init(pcms); > > - if (pcmc->smbios_defaults) { > - MachineClass *mc = MACHINE_GET_CLASS(machine); > - /* These values are guest ABI, do not change */ > - smbios_set_defaults("QEMU", mc->desc, > - mc->name, pcmc->smbios_legacy_mode, > - pcmc->smbios_uuid_encoded, > - pcms->smbios_entry_point_type); > - } > - > /* allocate ram and load rom/bios */ > if (!xen_enabled()) { > pc_memory_init(pcms, system_memory, rom_memory, hole64_size); > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 6155427e48..a58cd1d3ea 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -198,14 +198,6 @@ static void pc_q35_init(MachineState *machine) > > pc_guest_info_init(pcms); > > - if (pcmc->smbios_defaults) { > - /* These values are guest ABI, do not change */ > - smbios_set_defaults("QEMU", mc->desc, > - mc->name, pcmc->smbios_legacy_mode, > - pcmc->smbios_uuid_encoded, > - pcms->smbios_entry_point_type); > - } > - > /* create pci host bus */ > q35_host = Q35_HOST_DEVICE(qdev_new(TYPE_Q35_HOST_DEVICE)); >
On 6/7/2023 3:11 PM, Daniel P. Berrangé wrote: > On Tue, Jun 06, 2023 at 09:49:37PM -0500, Suravee Suthikulpanit wrote: >> Into a helper function pc_machine_init_smbios() in preparation for >> subsequent code to upgrade default SMBIOS entry point type. >> >> Then, call the helper function from the pc_machine_initfn() to eliminate >> duplicate code in pc_q35.c and pc_pixx.c. However, this changes the >> ordering of when the smbios_set_defaults() is called to before >> pc_machine_set_smbios_ep() (i.e. before handling the user specified >> QEMU option "-M ...,smbios-entry-point-type=[32|64]" to override >> the default type.) >> >> Therefore, also call the helper function in pc_machine_set_smbios_ep() >> to update the defaults. > > This is unsafe - smbios_set_defaults is only intended to be called > once. Calling it twice leads to a SEGV due to double-free > > $ ./build/qemu-system-x86_64 -machine pc,smbios-entry-point-type=64 -smbios file=/tmp/smbios_entry_point > Segmentation fault (core dumped) Thanks for pointing this out. I missed this > IMHO we should just not do this refactoring. The existing duplicated > code is not a significant burden, and thus is better than having to > workaround calling pc_machine_set_smbios_ep too early in startup. Ok Thanks, Suravee
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index bb62c994fa..b720dc67b6 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1756,6 +1756,22 @@ static void pc_machine_set_default_bus_bypass_iommu(Object *obj, bool value, pcms->default_bus_bypass_iommu = value; } +static void pc_machine_init_smbios(PCMachineState *pcms) +{ + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); + MachineClass *mc = MACHINE_GET_CLASS(pcms); + + if (!pcmc->smbios_defaults) { + return; + } + + /* These values are guest ABI, do not change */ + smbios_set_defaults("QEMU", mc->desc, + mc->name, pcmc->smbios_legacy_mode, + pcmc->smbios_uuid_encoded, + pcms->smbios_entry_point_type); +} + static void pc_machine_get_smbios_ep(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { @@ -1768,9 +1784,14 @@ static void pc_machine_get_smbios_ep(Object *obj, Visitor *v, const char *name, static void pc_machine_set_smbios_ep(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { + SmbiosEntryPointType ep_type; PCMachineState *pcms = PC_MACHINE(obj); - visit_type_SmbiosEntryPointType(v, name, &pcms->smbios_entry_point_type, errp); + if (!visit_type_SmbiosEntryPointType(v, name, &ep_type, errp)) { + return; + } + pcms->smbios_entry_point_type = ep_type; + pc_machine_init_smbios(pcms); } static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v, @@ -1878,6 +1899,7 @@ static void pc_machine_initfn(Object *obj) object_property_add_alias(OBJECT(pcms), "pcspk-audiodev", OBJECT(pcms->pcspk), "audiodev"); cxl_machine_init(obj, &pcms->cxl_devices_state); + pc_machine_init_smbios(pcms); } int pc_machine_kvm_type(MachineState *machine, const char *kvm_type) diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index d5b0dcd1fe..da6ba4eeb4 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -198,15 +198,6 @@ static void pc_init1(MachineState *machine, pc_guest_info_init(pcms); - if (pcmc->smbios_defaults) { - MachineClass *mc = MACHINE_GET_CLASS(machine); - /* These values are guest ABI, do not change */ - smbios_set_defaults("QEMU", mc->desc, - mc->name, pcmc->smbios_legacy_mode, - pcmc->smbios_uuid_encoded, - pcms->smbios_entry_point_type); - } - /* allocate ram and load rom/bios */ if (!xen_enabled()) { pc_memory_init(pcms, system_memory, rom_memory, hole64_size); diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 6155427e48..a58cd1d3ea 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -198,14 +198,6 @@ static void pc_q35_init(MachineState *machine) pc_guest_info_init(pcms); - if (pcmc->smbios_defaults) { - /* These values are guest ABI, do not change */ - smbios_set_defaults("QEMU", mc->desc, - mc->name, pcmc->smbios_legacy_mode, - pcmc->smbios_uuid_encoded, - pcms->smbios_entry_point_type); - } - /* create pci host bus */ q35_host = Q35_HOST_DEVICE(qdev_new(TYPE_Q35_HOST_DEVICE));
Into a helper function pc_machine_init_smbios() in preparation for subsequent code to upgrade default SMBIOS entry point type. Then, call the helper function from the pc_machine_initfn() to eliminate duplicate code in pc_q35.c and pc_pixx.c. However, this changes the ordering of when the smbios_set_defaults() is called to before pc_machine_set_smbios_ep() (i.e. before handling the user specified QEMU option "-M ...,smbios-entry-point-type=[32|64]" to override the default type.) Therefore, also call the helper function in pc_machine_set_smbios_ep() to update the defaults. There is no functional change. Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> --- hw/i386/pc.c | 24 +++++++++++++++++++++++- hw/i386/pc_piix.c | 9 --------- hw/i386/pc_q35.c | 8 -------- 3 files changed, 23 insertions(+), 18 deletions(-)