[PATCH v5 0/3] hw/i386/pc: Update max_cpus and default to SMBIOS

[PATCH v5 0/3] hw/i386/pc: Update max_cpus and default to SMBIOS

[Suravee Suthikulpanit]

In order to support large number of vcpus, a newer 64-bit SMBIOS
entry point type is needed. Therefore, upgrade the default SMBIOS version
for PC machines to SMBIOS 3.0 for newer systems. Then increase the maximum
number of vCPUs for Q35 models to 1024, which is the limit for KVM.

Changes from V4:
(https://lore.kernel.org/qemu-devel/20230605213906.644883-1-suravee.suthikulpanit@amd.com/)
 * Patch 1: Introduce a helper function pc_machine_init_smbios() and
   move the callsite from pc_machine_done() to pc_machine_init_fn().

 * Patch 2: Remove stuct PCMachineState::smbios_use_cmdline_ep_type.

 * Patch 3: Keep max_cpus for pc machine version 8.0 and older as 288.

Changes from V3:
(https://lore.kernel.org/qemu-devel/20230603032255.517970-1-suravee.suthikulpanit@amd.com/T/#t )
 * Patch 1: Refactor the code to setup SMBIOS defaults to pc_machine_done().
 * Patch 2: Minor typo fixes in comment.

Changes from V2:
(https://lore.kernel.org/qemu-devel/20230531225127.331998-1-suravee.suthikulpanit@amd.com/)
 * Add patch 1.

Changes from V1:
(https://lore.kernel.org/all/YnkDGsIi1vFvXmiP@redhat.com/T/)
 * Bump from 512 to KVM_MAX_VCPUS (per Igor's suggestion)

Thank you,
Suravee

Suravee Suthikulpanit (3):
  hw/i386/pc: Refactor logic to set SMBIOS defaults
  hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models
  pc: q35: Bump max_cpus to 1024

 hw/i386/pc.c         | 28 +++++++++++++++++++++++++++-
 hw/i386/pc_piix.c    | 14 +++++---------
 hw/i386/pc_q35.c     | 16 +++++++---------
 include/hw/i386/pc.h |  1 +
 4 files changed, 40 insertions(+), 19 deletions(-)

-- 
2.34.1

[PATCH v5 1/3] hw/i386/pc: Refactor logic to set SMBIOS defaults

[Suravee Suthikulpanit]

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(-)

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

[PATCH v5 2/3] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models

[Suravee Suthikulpanit]

Currently, pc-q35 and pc-i44fx machine models are default to use SMBIOS 2.8
(32-bit entry point). Since SMBIOS 3.0 (64-bit entry point) is now fully
supported since QEMU 7.0, default to use SMBIOS 3.0 for newer machine
models. This is necessary to avoid the following message when launching
a VM with large number of vcpus.

   "SMBIOS 2.1 table length 66822 exceeds 65535"

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 hw/i386/pc.c         | 6 +++++-
 hw/i386/pc_piix.c    | 5 +++++
 hw/i386/pc_q35.c     | 5 +++++
 include/hw/i386/pc.h | 1 +
 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b720dc67b6..b5c585579a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1765,6 +1765,8 @@ static void pc_machine_init_smbios(PCMachineState *pcms)
         return;
     }
 
+    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,
@@ -1786,11 +1788,12 @@ static void pc_machine_set_smbios_ep(Object *obj, Visitor *v, const char *name,
 {
     SmbiosEntryPointType ep_type;
     PCMachineState *pcms = PC_MACHINE(obj);
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
 
     if (!visit_type_SmbiosEntryPointType(v, name, &ep_type, errp)) {
         return;
     }
-    pcms->smbios_entry_point_type = ep_type;
+    pcmc->default_smbios_ep_type = ep_type;
     pc_machine_init_smbios(pcms);
 }
 
@@ -2002,6 +2005,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->nvdimm_supported = true;
     mc->smp_props.dies_supported = true;
     mc->default_ram_id = "pc.ram";
+    pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_64;
 
     object_class_property_add(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "size",
         pc_machine_get_max_ram_below_4g, pc_machine_set_max_ram_below_4g,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index da6ba4eeb4..1a2bb25c75 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -467,11 +467,16 @@ DEFINE_I440FX_MACHINE(v8_1, "pc-i440fx-8.1", NULL,
 
 static void pc_i440fx_8_0_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+
     pc_i440fx_8_1_machine_options(m);
     m->alias = NULL;
     m->is_default = false;
     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-i44fx-8.0 and older, use SMBIOS 2.8 by default */
+    pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_32;
 }
 
 DEFINE_I440FX_MACHINE(v8_0, "pc-i440fx-8.0", NULL,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a58cd1d3ea..371cca7484 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -379,10 +379,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..6eec0fc51d 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -110,6 +110,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;
-- 
2.34.1

[PATCH v5 3/3] pc: q35: Bump max_cpus to 1024

[Suravee Suthikulpanit]

Since KVM_MAX_VCPUS is currently defined to 1024 for x86 as shown in
arch/x86/include/asm/kvm_host.h, update QEMU limits to the same number.

In case KVM could not support the specified number of vcpus, QEMU would
return the following error message:

  qemu-system-x86_64: kvm_init_vcpu: kvm_get_vcpu failed (xxx): Invalid argument

Also, keep max_cpus at 288 for machine version 8.0 and older.

Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Julia Suvorova <jusual@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 hw/i386/pc_q35.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 371cca7484..63866a16e0 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -360,12 +360,12 @@ static void pc_q35_machine_options(MachineClass *m)
     m->default_nic = "e1000e";
     m->default_kernel_irqchip_split = false;
     m->no_floppy = 1;
+    m->max_cpus = 1024;
     m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE);
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
-    m->max_cpus = 288;
 }
 
 static void pc_q35_8_1_machine_options(MachineClass *m)
@@ -388,6 +388,7 @@ static void pc_q35_8_0_machine_options(MachineClass *m)
 
     /* For pc-q35-8.0 and older, use SMBIOS 2.8 by default */
     pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_32;
+    m->max_cpus = 288;
 }
 
 DEFINE_Q35_MACHINE(v8_0, "pc-q35-8.0", NULL,
-- 
2.34.1

Re: [PATCH v5 1/3] hw/i386/pc: Refactor logic to set SMBIOS defaults

[Philippe Mathieu-Daudé]

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>

Re: [PATCH v5 1/3] hw/i386/pc: Refactor logic to set SMBIOS defaults

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v5 1/3] hw/i386/pc: Refactor logic to set SMBIOS defaults

[Philippe Mathieu-Daudé]

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(-)

Re: [PATCH v5 3/3] pc: q35: Bump max_cpus to 1024

[Philippe Mathieu-Daudé]

On 7/6/23 04:49, Suravee Suthikulpanit wrote:
> Since KVM_MAX_VCPUS is currently defined to 1024 for x86 as shown in
> arch/x86/include/asm/kvm_host.h, update QEMU limits to the same number.
> 
> In case KVM could not support the specified number of vcpus, QEMU would
> return the following error message:
> 
>    qemu-system-x86_64: kvm_init_vcpu: kvm_get_vcpu failed (xxx): Invalid argument

Odd, we already check the upper limit with KVM_CAP_NR_VCPUS.
See in kvm_init():

     /* check the vcpu limits */
     soft_vcpus_limit = kvm_recommended_vcpus(s);
     hard_vcpus_limit = kvm_max_vcpus(s);

When testing your series I get:

qemu-system-x86_64: -accel kvm: warning: Number of SMP cpus requested 
(1024) exceeds the recommended cpus supported by KVM (710)

$ uname -srvp
Linux 5.15.0-71-generic #78-Ubuntu SMP Tue Apr 18 09:00:29 UTC 2023 x86_64

> Also, keep max_cpus at 288 for machine version 8.0 and older.
> 
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Julia Suvorova <jusual@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   hw/i386/pc_q35.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Re: [PATCH v5 3/3] pc: q35: Bump max_cpus to 1024

[Daniel P. Berrangé]

On Wed, Jun 07, 2023 at 10:26:59AM +0200, Philippe Mathieu-Daudé wrote:
> On 7/6/23 04:49, Suravee Suthikulpanit wrote:
> > Since KVM_MAX_VCPUS is currently defined to 1024 for x86 as shown in
> > arch/x86/include/asm/kvm_host.h, update QEMU limits to the same number.
> > 
> > In case KVM could not support the specified number of vcpus, QEMU would
> > return the following error message:
> > 
> >    qemu-system-x86_64: kvm_init_vcpu: kvm_get_vcpu failed (xxx): Invalid argument
> 
> Odd, we already check the upper limit with KVM_CAP_NR_VCPUS.
> See in kvm_init():
> 
>     /* check the vcpu limits */
>     soft_vcpus_limit = kvm_recommended_vcpus(s);
>     hard_vcpus_limit = kvm_max_vcpus(s);
> 
> When testing your series I get:
> 
> qemu-system-x86_64: -accel kvm: warning: Number of SMP cpus requested (1024)
> exceeds the recommended cpus supported by KVM (710)
> 
> $ uname -srvp
> Linux 5.15.0-71-generic #78-Ubuntu SMP Tue Apr 18 09:00:29 UTC 2023 x86_64

That's a relatively old kernel. With latest kernels, the 'recommended cpus'
limit reported will match the number of CPUs in your host, so for me it
always complains for smp > 12.



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Igor Mammedov]

On Tue, 6 Jun 2023 21:49:38 -0500
Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:

> Currently, pc-q35 and pc-i44fx machine models are default to use SMBIOS 2.8
> (32-bit entry point). Since SMBIOS 3.0 (64-bit entry point) is now fully
> supported since QEMU 7.0, default to use SMBIOS 3.0 for newer machine
> models. This is necessary to avoid the following message when launching
> a VM with large number of vcpus.
> 
>    "SMBIOS 2.1 table length 66822 exceeds 65535"
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  hw/i386/pc.c         | 6 +++++-
>  hw/i386/pc_piix.c    | 5 +++++
>  hw/i386/pc_q35.c     | 5 +++++
>  include/hw/i386/pc.h | 1 +
>  4 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b720dc67b6..b5c585579a 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1765,6 +1765,8 @@ static void pc_machine_init_smbios(PCMachineState *pcms)
>          return;
>      }
>  
> +    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,
> @@ -1786,11 +1788,12 @@ static void pc_machine_set_smbios_ep(Object *obj, Visitor *v, const char *name,
>  {
>      SmbiosEntryPointType ep_type;
>      PCMachineState *pcms = PC_MACHINE(obj);
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>  
>      if (!visit_type_SmbiosEntryPointType(v, name, &ep_type, errp)) {
>          return;
>      }
> -    pcms->smbios_entry_point_type = ep_type;
> +    pcmc->default_smbios_ep_type = ep_type;

class field is supposed to be constant, not something modified by property
drop all of above hunks


>      pc_machine_init_smbios(pcms);
>  }

and use this with the rest of your patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b3d826a83a..c5bab28e9c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1859,7 +1859,7 @@ static void pc_machine_initfn(Object *obj)
     pcms->vmport = ON_OFF_AUTO_OFF;
 #endif /* CONFIG_VMPORT */
     pcms->max_ram_below_4g = 0; /* use default */
-    pcms->smbios_entry_point_type = SMBIOS_ENTRY_POINT_TYPE_32;
+    pcms->smbios_entry_point_type = pcmc->default_smbios_ep_type;
 
     /* acpi build is enabled by default if machine supports it */
     pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
@@ -1979,6 +1979,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->nvdimm_supported = true;
     mc->smp_props.dies_supported = true;
     mc->default_ram_id = "pc.ram";
+    mc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_64;
 
     object_class_property_add(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "size",
         pc_machine_get_max_ram_below_4g, pc_machine_set_max_ram_below_4g,


>  
> @@ -2002,6 +2005,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      mc->nvdimm_supported = true;
>      mc->smp_props.dies_supported = true;
>      mc->default_ram_id = "pc.ram";
> +    pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_64;
>  
>      object_class_property_add(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "size",
>          pc_machine_get_max_ram_below_4g, pc_machine_set_max_ram_below_4g,
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index da6ba4eeb4..1a2bb25c75 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -467,11 +467,16 @@ DEFINE_I440FX_MACHINE(v8_1, "pc-i440fx-8.1", NULL,
>  
>  static void pc_i440fx_8_0_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> +
>      pc_i440fx_8_1_machine_options(m);
>      m->alias = NULL;
>      m->is_default = false;
>      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-i44fx-8.0 and older, use SMBIOS 2.8 by default */
> +    pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_32;
>  }
>  
>  DEFINE_I440FX_MACHINE(v8_0, "pc-i440fx-8.0", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index a58cd1d3ea..371cca7484 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -379,10 +379,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..6eec0fc51d 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -110,6 +110,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;

Re: [PATCH v5 3/3] pc: q35: Bump max_cpus to 1024

[Igor Mammedov]

On Wed, 7 Jun 2023 10:26:59 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 7/6/23 04:49, Suravee Suthikulpanit wrote:
> > Since KVM_MAX_VCPUS is currently defined to 1024 for x86 as shown in
> > arch/x86/include/asm/kvm_host.h, update QEMU limits to the same number.
> > 
> > In case KVM could not support the specified number of vcpus, QEMU would
> > return the following error message:
> > 
> >    qemu-system-x86_64: kvm_init_vcpu: kvm_get_vcpu failed (xxx): Invalid argument  
> 
> Odd, we already check the upper limit with KVM_CAP_NR_VCPUS.
> See in kvm_init():
> 
>      /* check the vcpu limits */
>      soft_vcpus_limit = kvm_recommended_vcpus(s);
>      hard_vcpus_limit = kvm_max_vcpus(s);
> 
> When testing your series I get:
> 
> qemu-system-x86_64: -accel kvm: warning: Number of SMP cpus requested 
> (1024) exceeds the recommended cpus supported by KVM (710)

Also do not forget about TCG where KVM accel is not even in the picture.

> $ uname -srvp
> Linux 5.15.0-71-generic #78-Ubuntu SMP Tue Apr 18 09:00:29 UTC 2023 x86_64
> 
> > Also, keep max_cpus at 288 for machine version 8.0 and older.
> > 
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Julia Suvorova <jusual@redhat.com>
> > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> > ---
> >   hw/i386/pc_q35.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)  
> 

Re: [PATCH v5 3/3] pc: q35: Bump max_cpus to 1024

[Igor Mammedov]

On Tue, 6 Jun 2023 21:49:39 -0500
Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:

> Since KVM_MAX_VCPUS is currently defined to 1024 for x86 as shown in
> arch/x86/include/asm/kvm_host.h, update QEMU limits to the same number.
> 
> In case KVM could not support the specified number of vcpus, QEMU would
> return the following error message:
> 
>   qemu-system-x86_64: kvm_init_vcpu: kvm_get_vcpu failed (xxx): Invalid argument
> 
> Also, keep max_cpus at 288 for machine version 8.0 and older.
> 
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Julia Suvorova <jusual@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc_q35.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 371cca7484..63866a16e0 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -360,12 +360,12 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->default_nic = "e1000e";
>      m->default_kernel_irqchip_split = false;
>      m->no_floppy = 1;
> +    m->max_cpus = 1024;
>      m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE);
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
> -    m->max_cpus = 288;
>  }
>  
>  static void pc_q35_8_1_machine_options(MachineClass *m)
> @@ -388,6 +388,7 @@ static void pc_q35_8_0_machine_options(MachineClass *m)
>  
>      /* For pc-q35-8.0 and older, use SMBIOS 2.8 by default */
>      pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_32;
> +    m->max_cpus = 288;
>  }
>  
>  DEFINE_Q35_MACHINE(v8_0, "pc-q35-8.0", NULL,

Re: [PATCH v5 1/3] hw/i386/pc: Refactor logic to set SMBIOS defaults

[Igor Mammedov]

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));
>  

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

[Suthikulpanit, Suravee]

On 6/7/2023 8:49 PM, Igor Mammedov wrote:
> On Tue, 6 Jun 2023 21:49:38 -0500
> Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>  wrote:
>
> ....
>
> and use this with the rest of your patch
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b3d826a83a..c5bab28e9c 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1859,7 +1859,7 @@ static void pc_machine_initfn(Object *obj)
>       pcms->vmport = ON_OFF_AUTO_OFF;
>   #endif /* CONFIG_VMPORT */
>       pcms->max_ram_below_4g = 0; /* use default */
> -    pcms->smbios_entry_point_type = SMBIOS_ENTRY_POINT_TYPE_32;
> +    pcms->smbios_entry_point_type = pcmc->default_smbios_ep_type;

Ah, I missed this part. Thanks for suggestions. I'll send out v6.

Suravee

>       /* acpi build is enabled by default if machine supports it */
>       pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
> @@ -1979,6 +1979,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>       mc->nvdimm_supported = true;
>       mc->smp_props.dies_supported = true;
>       mc->default_ram_id = "pc.ram";
> +    mc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_64;
>   
>       object_class_property_add(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "size",
>           pc_machine_get_max_ram_below_4g, pc_machine_set_max_ram_below_4g,
> 
> 

Re: [PATCH v5 1/3] hw/i386/pc: Refactor logic to set SMBIOS defaults

[Suthikulpanit, Suravee]

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