Re: [PATCH v6 1/2] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models
On 6/8/2023 3:40 PM, Igor Mammedov wrote:
On Wed, 7 Jun 2023 15:57:16 -0500
Suravee Suthikulpanit 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
Looks good to me (see comment below for extra cleanup possibility):
Reviewed-by: Igor Mammedov
---
hw/i386/pc.c | 4 +++-
hw/i386/pc_piix.c| 5 +
hw/i386/pc_q35.c | 5 +
include/hw/i386/pc.h | 1 +
4 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bb62c994fa..33ffb03a32 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1853,6 +1853,7 @@ static void pc_machine_set_max_fw_size(Object *obj,
Visitor *v,
static void pc_machine_initfn(Object *obj)
{
PCMachineState *pcms = PC_MACHINE(obj);
+PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
since you introduce this local,
I suggest you to post an extra clean up patch on top of this series
here is a line to cleanup with 'pcmc'
/* acpi build is enabled by default if machine supports it */
pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
Sure just sent out the "[PATCH] hw/i386/pc: Clean up pc_machine_initfn".
Thanks,
Suravee
Re: [PATCH v5 1/3] hw/i386/pc: Refactor logic to set SMBIOS defaults
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
Re: [PATCH v5 2/3] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models
On 6/7/2023 8:49 PM, Igor Mammedov wrote: On Tue, 6 Jun 2023 21:49:38 -0500 Suravee Suthikulpanit 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 v4 2/3] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models
Igor,
On 6/6/2023 3:11 PM, Igor Mammedov wrote:
On Tue, 6 Jun 2023 09:35:41 +0200
Igor Mammedov wrote:
On Mon, 5 Jun 2023 16:39:05 -0500
Suravee Suthikulpanit 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
Re: [PATCH v4 2/3] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models
On 6/6/2023 2:35 PM, Igor Mammedov wrote:
On Mon, 5 Jun 2023 16:39:05 -0500
Suravee Suthikulpanit 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
---
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 3/3] pc: q35: Bump max_cpus to 1024
Igor, On 6/6/2023 2:55 PM, Igor Mammedov wrote: On Mon, 5 Jun 2023 16:39:06 -0500 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 Cc: Igor Mammedov Cc: Daniel P. Berrangé Cc: Michael S. Tsirkin Cc: Julia Suvorova Signed-off-by: Suravee Suthikulpanit --- 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
Igore,
On 6/6/2023 2:45 PM, Igor Mammedov wrote:
On Mon, 5 Jun 2023 16:39:04 -0500
Suravee Suthikulpanit wrote:
In preparation for subsequent code to upgrade default SMBIOS
entry point type. There is no functional change.
Signed-off-by: Suravee Suthikulpanit
---
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 v3 1/2] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models
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
Re: [PATCH v2] pc: q35: Bump max_cpus to 1024
On 6/1/2023 6:09 PM, Michael S. Tsirkin wrote: On Thu, Jun 01, 2023 at 11:17:30AM +0100, Daniel P. Berrangé wrote: On Thu, Jun 01, 2023 at 11:09:45AM +0100, Joao Martins wrote: On 31/05/2023 23:51, 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 Signed-off-by: Suravee Suthikulpanit --- Changes from V1: (https://lore.kernel.org/all/ynkdgsii1vfvx...@redhat.com/T/) * Bump from 512 to KVM_MAX_VCPUS (per Igor's suggestion) Note: From the last discussion, Daniel mentioned that SMBIO 2.1 tables might cause overflow at approx 720 CPUs, and it might require using the SMBIO 3.0 entry point. Also, we might need to change the default for the x86 machine type to SMBIO 3.0. However, I do not know the status of this. I suspect smbios 3.0 (64-bit entry point) is already supported. With current qemu and all the smbios fixes in the last cycle, perhaps this is mainly just setting smbios_entry_point_type to SMBIOS_ENTRY_POINT_TYPE_64 if MachineState::smp::max_cpus is bigger than 720 (e.g. in pc_q35_init()?) >> The need for the 64-bit entry point depends on a combination of RAM config and CPU count. IMHO we need to unconditionally switch the latest machine types to use the 64-bit entry point by default, rather than trying to infer some special condition to dynamically change on the fly. Yes, makes sense. Thanks all for the feedback. So, IIUC, here is how the SMBIOS entry point types would be affected by the QEMU options: - pc-q35-8.1 and later, default to SMBIOS EP type 64. - pc-q35-8.0 and older, default to SMBIOS EP type 32. - User can override the type w/ QEMU option "-M .., smbios-entry-point-type=[32|64]" Please let me know if I am missing anything. If this is accurate, I'll send out v3 with this change. Thanks, Suravee
[RFC] KVM / QEMU: Introduce Interface for Querying APICv Info
Hi All, Currently, we don't have a good way to check whether APICV is active on a VM. Normally, For AMD SVM AVIC, users either have to check for trace point, or using "perf kvm stat live" to catch AVIC-related #VMEXIT. For KVM, I would like to propose introducing a new IOCTL interface (i.e. KVM_GET_APICV_INFO), where user-space tools (e.g. QEMU monitor) can query run-time information of APICv for VM and vCPUs such as APICv inhibit reason flags. For QEMU, we can leverage the "info lapic" command, and append the APICV information after all LAPIC register information: For example: - Begin Snippet - (qemu) info lapic 0 dumping local APIC state for CPU 0 LVT0 0x00010700 active-hi edge masked ExtINT (vec 0) LVT1 0x0400 active-hi edge NMI LVTPC0x0001 active-hi edge masked Fixed (vec 0) LVTERR 0x00fe active-hi edge Fixed (vec 254) LVTTHMR 0x0001 active-hi edge masked Fixed (vec 0) LVTT 0x000400ee active-hi edge tsc-deadline Fixed (vec 238) TimerDCR=0x0 (divide by 2) initial_count = 0 current_count = 0 SPIV 0x01ff APIC enabled, focus=off, spurious vec 255 ICR 0x00fd physical edge de-assert no-shorthand ICR2 0x0005 cpu 5 (X2APIC ID) ESR 0x ISR (none) IRR (none) APR 0x00 TPR 0x00 DFR 0x0f LDR 0x00PPR 0x00 APICV vm inhibit: 0x10 <-- HERE APICV vcpu inhibit: 0 <-- HERE -- End Snippet -- Otherwise, we can have APICv-specific info command (e.g. info apicv). Any suggestions are much appreciated. Best Regards, Suravee
Re: [PATCH] pc: q35: Bump max_cpus to 512
Igor, On 5/9/2022 2:12 PM, Igor Mammedov wrote: On Wed, 4 May 2022 08:16:39 -0500 Suravee Suthikulpanit wrote: This is the maximum number of vCPU supported by the AMD x2APIC virtualization. Signed-off-by: Suravee Suthikulpanit --- 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 302288342a..e82b1c690d 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -357,7 +357,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 = 512; Maybe we should bump it to KVM VCPU maximum, Grepping through the Linux kernel source, the KVM_MAX_VCPUS macro is defined per architecture. AFAICT, the absolute maximum is for x86, which is 1024. Does that sound about right? and make sure we error out if asked for combination of hardware/irqchip is not usable. Could you please elaborate on this part? Thanks, Suravee
