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

[PATCH v4 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 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 set 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         | 26 +++++++++++++++++++++++++-
 hw/i386/pc_piix.c    | 14 +++++---------
 hw/i386/pc_q35.c     | 15 ++++++---------
 include/hw/i386/pc.h |  2 ++
 4 files changed, 38 insertions(+), 19 deletions(-)

-- 
2.34.1

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

[Suravee Suthikulpanit]

In preparation for subsequent code to upgrade default SMBIOS
entry point type. There is no functional change.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 hw/i386/pc.c      | 12 ++++++++++++
 hw/i386/pc_piix.c |  9 ---------
 hw/i386/pc_q35.c  |  8 --------
 3 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bb62c994fa..8fc34f5454 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -781,6 +781,18 @@ void pc_machine_done(Notifier *notifier, void *data)
 
     acpi_setup();
     if (x86ms->fw_cfg) {
+        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+
+        if (pcmc->smbios_defaults) {
+            MachineClass *mc = MACHINE_GET_CLASS(pcms);
+
+            /* 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);
+        }
+
         fw_cfg_build_smbios(MACHINE(pcms), x86ms->fw_cfg);
         fw_cfg_build_feature_control(MACHINE(pcms), x86ms->fw_cfg);
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
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 v4 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"

Note that user can still override the entry point tyme w/ QEMU option
"-M ..., smbios-entry-point-type=[32|64].

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8fc34f5454..5a87b82185 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -786,6 +786,14 @@ void pc_machine_done(Notifier *notifier, void *data)
         if (pcmc->smbios_defaults) {
             MachineClass *mc = MACHINE_GET_CLASS(pcms);
 
+            /*
+             * Check if user has specified a command line option
+             * to override the 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,
@@ -1782,7 +1790,10 @@ static void pc_machine_set_smbios_ep(Object *obj, Visitor *v, const char *name,
 {
     PCMachineState *pcms = PC_MACHINE(obj);
 
-    visit_type_SmbiosEntryPointType(v, name, &pcms->smbios_entry_point_type, errp);
+    pcms->smbios_use_cmdline_ep_type =
+        visit_type_SmbiosEntryPointType(v, name,
+                                        &pcms->smbios_entry_point_type,
+                                        errp);
 }
 
 static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
@@ -1992,6 +2003,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..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;
-- 
2.34.1

[PATCH v4 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

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 371cca7484..bd862add94 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -365,7 +365,7 @@ static void pc_q35_machine_options(MachineClass *m)
     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;
+    m->max_cpus = 1024;
 }
 
 static void pc_q35_8_1_machine_options(MachineClass *m)
-- 
2.34.1

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

[Igor Mammedov]

On Mon, 5 Jun 2023 16:39:05 -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"
> 
> Note that user can still override the entry point tyme w/ QEMU option
> "-M ..., smbios-entry-point-type=[32|64].
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  hw/i386/pc.c         | 14 +++++++++++++-
>  hw/i386/pc_piix.c    |  5 +++++
>  hw/i386/pc_q35.c     |  5 +++++
>  include/hw/i386/pc.h |  2 ++
>  4 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8fc34f5454..5a87b82185 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -786,6 +786,14 @@ void pc_machine_done(Notifier *notifier, void *data)
>          if (pcmc->smbios_defaults) {
>              MachineClass *mc = MACHINE_GET_CLASS(pcms);
>  
> +            /*
> +             * Check if user has specified a command line option
> +             * to override the SMBIOS default entry point type.
> +             */
> +            if (!pcms->smbios_use_cmdline_ep_type) {
> +                pcms->smbios_entry_point_type = pcmc->default_smbios_ep_type;
> +            }

why pcms->smbios_entry_point_type that we already have is not enough
we need to add extra pcms->smbios_use_cmdline_ep_type field?

> +
>              /* These values are guest ABI, do not change */
>              smbios_set_defaults("QEMU", mc->desc,
>                                  mc->name, pcmc->smbios_legacy_mode,
> @@ -1782,7 +1790,10 @@ static void pc_machine_set_smbios_ep(Object *obj, Visitor *v, const char *name,
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
>  
> -    visit_type_SmbiosEntryPointType(v, name, &pcms->smbios_entry_point_type, errp);
> +    pcms->smbios_use_cmdline_ep_type =
> +        visit_type_SmbiosEntryPointType(v, name,
> +                                        &pcms->smbios_entry_point_type,
> +                                        errp);
>  }
>  
>  static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
> @@ -1992,6 +2003,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..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;

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

[Igor Mammedov]

On Mon, 5 Jun 2023 16:39:04 -0500
Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:

> In preparation for subsequent code to upgrade default SMBIOS
> entry point type. There is no functional change.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  hw/i386/pc.c      | 12 ++++++++++++
>  hw/i386/pc_piix.c |  9 ---------
>  hw/i386/pc_q35.c  |  8 --------
>  3 files changed, 12 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index bb62c994fa..8fc34f5454 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -781,6 +781,18 @@ void pc_machine_done(Notifier *notifier, void *data)
>  
>      acpi_setup();
>      if (x86ms->fw_cfg) {
> +        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +
> +        if (pcmc->smbios_defaults) {
> +            MachineClass *mc = MACHINE_GET_CLASS(pcms);
> +
> +            /* 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);
> +        }
> 

well, pc_machine_done() is the hack for the last minute changes to
board that can't done earlier otherwise (during machine_initfn time).
So I'd prefer not adding anything there unless we have to.

>          fw_cfg_build_smbios(MACHINE(pcms), x86ms->fw_cfg);
>          fw_cfg_build_feature_control(MACHINE(pcms), x86ms->fw_cfg);
>          /* update FW_CFG_NB_CPUS to account for -device added CPUs */
> 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 v4 3/3] pc: q35: Bump max_cpus to 1024

[Igor Mammedov]

On Mon, 5 Jun 2023 16:39:06 -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
> 
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 371cca7484..bd862add94 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -365,7 +365,7 @@ static void pc_q35_machine_options(MachineClass *m)
>      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;
> +    m->max_cpus = 1024;

did you forgot to preserve value for older machine types?
use commit 00d0f9fd6602a2 as reference 

>  }
>
>  static void pc_q35_8_1_machine_options(MachineClass *m)

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

[Igor Mammedov]

On Tue, 6 Jun 2023 09:35:41 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 5 Jun 2023 16:39:05 -0500
> Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:
[...]
> > +    /* For pc-i44fx-8.0 and older, use SMBIOS 2.8 by default */
> > +    pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_32;
> >  }
[...]
> >  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;
> >  }
[...]

what Michael referred to as duplication is probably these lines,
however "smbios-entry-point-type: is machine property so we can't
use device compat machinery here (pc_compat_8_0).
What we can do is override "default_machine_opts" value, but then
again it has to be opencoded in  piix4/q35_machine_options, so
don't see a point in doing so.
As you wrote here is what typically do for machine compat knobs,
so it should be fine.

I just doubt that we need extra default_smbios_ep_type field.

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

[Suthikulpanit, Suravee]

Igor,

On 6/6/2023 2:55 PM, Igor Mammedov wrote:
> On Mon, 5 Jun 2023 16:39:06 -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
>>
>> 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 | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 371cca7484..bd862add94 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -365,7 +365,7 @@ static void pc_q35_machine_options(MachineClass *m)
>>       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;
>> +    m->max_cpus = 1024;
> 
> did you forgot to preserve value for older machine types?
> use commit 00d0f9fd6602a2 as reference

Thanks for suggestion. I'll fix this in v5.

Suravee

>>   }
>>
>>   static void pc_q35_8_1_machine_options(MachineClass *m)
> 

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

[Suthikulpanit, Suravee]

Igore,

On 6/6/2023 2:45 PM, Igor Mammedov wrote:
> On Mon, 5 Jun 2023 16:39:04 -0500
> Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>  wrote:
> 
>> In preparation for subsequent code to upgrade default SMBIOS
>> entry point type. There is no functional change.
>>
>> Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>> ---
>>   hw/i386/pc.c      | 12 ++++++++++++
>>   hw/i386/pc_piix.c |  9 ---------
>>   hw/i386/pc_q35.c  |  8 --------
>>   3 files changed, 12 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index bb62c994fa..8fc34f5454 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -781,6 +781,18 @@ void pc_machine_done(Notifier *notifier, void *data)
>>   
>>       acpi_setup();
>>       if (x86ms->fw_cfg) {
>> +        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> +
>> +        if (pcmc->smbios_defaults) {
>> +            MachineClass *mc = MACHINE_GET_CLASS(pcms);
>> +
>> +            /* 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);
>> +        }
>>
> well, pc_machine_done() is the hack for the last minute changes to
> board that can't done earlier otherwise (during machine_initfn time).
> So I'd prefer not adding anything there unless we have to.

Originally, I put it here because pc_machine_set_smbios_ep() is called 
between the pc_machine_init_fn() and pc_machine_done().


In this case, I'll move this code to the end of pc_machine_init_fn(). 
Then, I can call smbios_set_defaults() from pc_machine_set_smbios_ep() 
to override the previously set defaults.

Thanks,
Suravee

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

[Suthikulpanit, Suravee]

On 6/6/2023 2:35 PM, Igor Mammedov wrote:
> On Mon, 5 Jun 2023 16:39:05 -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"
>>
>> Note that user can still override the entry point tyme w/ QEMU option
>> "-M ..., smbios-entry-point-type=[32|64].
>>
>> Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>> ---
>>   hw/i386/pc.c         | 14 +++++++++++++-
>>   hw/i386/pc_piix.c    |  5 +++++
>>   hw/i386/pc_q35.c     |  5 +++++
>>   include/hw/i386/pc.h |  2 ++
>>   4 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 8fc34f5454..5a87b82185 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -786,6 +786,14 @@ void pc_machine_done(Notifier *notifier, void *data)
>>           if (pcmc->smbios_defaults) {
>>               MachineClass *mc = MACHINE_GET_CLASS(pcms);
>>   
>> +            /*
>> +             * Check if user has specified a command line option
>> +             * to override the SMBIOS default entry point type.
>> +             */
>> +            if (!pcms->smbios_use_cmdline_ep_type) {
>> +                pcms->smbios_entry_point_type = pcmc->default_smbios_ep_type;
>> +            }
> why pcms->smbios_entry_point_type that we already have is not enough
> we need to add extra pcms->smbios_use_cmdline_ep_type field?

I'll get rid of this in V5.

Suravee

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

[Suthikulpanit, Suravee]

Igor,

On 6/6/2023 3:11 PM, Igor Mammedov wrote:
> On Tue, 6 Jun 2023 09:35:41 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
>> On Mon, 5 Jun 2023 16:39:05 -0500
>> Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:
> [...]
>>> +    /* For pc-i44fx-8.0 and older, use SMBIOS 2.8 by default */
>>> +    pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_32;
>>>   }
> [...]
>>>   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;
>>>   }
> [...]
> 
> what Michael referred to as duplication is probably these lines,
> however "smbios-entry-point-type: is machine property so we can't
> use device compat machinery here (pc_compat_8_0).
> What we can do is override "default_machine_opts" value, but then
> again it has to be opencoded in  piix4/q35_machine_options, so
> don't see a point in doing so.
> As you wrote here is what typically do for machine compat knobs,
> so it should be fine.

Ok

> I just doubt that we need extra default_smbios_ep_type field.

I introduce pcmc->default_smbios_ep_type because I could not get to 
pcms->smbios_entry_point_type from these functions. Any suggestions?

Anyhow, I'll send out v5 with some more clean up.

Thanks,
Suravee