1. Patchew
  2. QEMU
  3. View series

[PATCH v2] target/i386: Fix CPUID encoding of Fn8000001E_ECX

Babu Moger posted 1 patch 10 months, 4 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240102231738.46553-1-babu.moger@amd.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>
There is a newer version of this series
hw/i386/pc.c      |  4 +++-
target/i386/cpu.c | 18 ++++++++++--------
target/i386/cpu.h |  6 ++++++
3 files changed, 19 insertions(+), 9 deletions(-)
[PATCH v2] target/i386: Fix CPUID encoding of Fn8000001E_ECX
Posted by Babu Moger 10 months, 4 weeks ago 
Observed the following failure while booting the SEV-SNP guest and the
guest fails to boot with the smp parameters:
"-smp 192,sockets=1,dies=12,cores=8,threads=2".

qemu-system-x86_64: sev_snp_launch_update: SNP_LAUNCH_UPDATE ret=-5 fw_error=22 'Invalid parameter'
qemu-system-x86_64: SEV-SNP: CPUID validation failed for function 0x8000001e, index: 0x0.
provided: eax:0x00000000, ebx: 0x00000100, ecx: 0x00000b00, edx: 0x00000000
expected: eax:0x00000000, ebx: 0x00000100, ecx: 0x00000300, edx: 0x00000000
qemu-system-x86_64: SEV-SNP: failed update CPUID page

Reason for the failure is due to overflowing of bits used for "Node per
processor" in CPUID Fn8000001E_ECX. This field's width is 3 bits wide and
can hold maximum value 0x7. With dies=12 (0xB), it overflows and spills
over into the reserved bits. In the case of SEV-SNP, this causes CPUID
enforcement failure and guest fails to boot.

The PPR documentation for CPUID_Fn8000001E_ECX [Node Identifiers]
=================================================================
Bits    Description
31:11   Reserved.

10:8    NodesPerProcessor: Node per processor. Read-only.
        ValidValues:
        Value   Description
        0h      1 node per processor.
        7h-1h   Reserved.

7:0     NodeId: Node ID. Read-only. Reset: Fixed,XXh.
=================================================================

As in the spec, the valid value for "node per processor" is 0 and rest
are reserved.

Looking back at the history of decoding of CPUID_Fn8000001E_ECX, noticed
that there were cases where "node per processor" can be more than 1. It
is valid only for pre-F17h (pre-EPYC) architectures. For EPYC or later
CPUs, the linux kernel does not use this information to build the L3
topology.

Also noted that the CPUID Function 0x8000001E_ECX is available only when
TOPOEXT feature is enabled. This feature is enabled only for EPYC(F17h)
or later processors. So, previous generation of processors do not not
enumerate 0x8000001E_ECX leaf.

There could be some corner cases where the older guests could enable the
TOPOEXT feature by running with -cpu host, in which case legacy guests
might notice the topology change. To address those cases introduced a
new CPU property "legacy-multi-node". It will be true for older machine
types to maintain compatibility. By default, it will be false, so new
decoding will be used going forward.

The documentation is taken from Preliminary Processor Programming
Reference (PPR) for AMD Family 19h Model 11h, Revision B1 Processors 55901
Rev 0.25 - Oct 6, 2022.

Cc: qemu-stable@nongnu.org
Fixes: 31ada106d891 ("Simplify CPUID_8000_001E for AMD")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
---
v2: Rebased to the latest tree.
    Updated the pc_compat_8_2 for the new flag.
    Added the comment for new property legacy_multi_node.
    Added Reviwed-by from Zhao.
---
 hw/i386/pc.c      |  4 +++-
 target/i386/cpu.c | 18 ++++++++++--------
 target/i386/cpu.h |  6 ++++++
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 496498df3a..a504e05e62 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -78,7 +78,9 @@
     { "qemu64-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },\
     { "athlon-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },
 
-GlobalProperty pc_compat_8_2[] = {};
+GlobalProperty pc_compat_8_2[] = {
+    { TYPE_X86_CPU, "legacy-multi-node", "on" },
+};
 const size_t pc_compat_8_2_len = G_N_ELEMENTS(pc_compat_8_2);
 
 GlobalProperty pc_compat_8_1[] = {};
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 95d5f16cd5..2cc84e8500 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -398,12 +398,9 @@ static void encode_topo_cpuid8000001e(X86CPU *cpu, X86CPUTopoInfo *topo_info,
      * 31:11 Reserved.
      * 10:8 NodesPerProcessor: Node per processor. Read-only. Reset: XXXb.
      *      ValidValues:
-     *      Value Description
-     *      000b  1 node per processor.
-     *      001b  2 nodes per processor.
-     *      010b Reserved.
-     *      011b 4 nodes per processor.
-     *      111b-100b Reserved.
+     *      Value   Description
+     *      0h      1 node per processor.
+     *      7h-1h   Reserved.
      *  7:0 NodeId: Node ID. Read-only. Reset: XXh.
      *
      * NOTE: Hardware reserves 3 bits for number of nodes per processor.
@@ -412,8 +409,12 @@ static void encode_topo_cpuid8000001e(X86CPU *cpu, X86CPUTopoInfo *topo_info,
      * NodeId is combination of node and socket_id which is already decoded
      * in apic_id. Just use it by shifting.
      */
-    *ecx = ((topo_info->dies_per_pkg - 1) << 8) |
-           ((cpu->apic_id >> apicid_die_offset(topo_info)) & 0xFF);
+    if (cpu->legacy_multi_node) {
+        *ecx = ((topo_info->dies_per_pkg - 1) << 8) |
+               ((cpu->apic_id >> apicid_die_offset(topo_info)) & 0xFF);
+    } else {
+        *ecx = (cpu->apic_id >> apicid_pkg_offset(topo_info)) & 0xFF;
+    }
 
     *edx = 0;
 }
@@ -7895,6 +7896,7 @@ static Property x86_cpu_properties[] = {
      * own cache information (see x86_cpu_load_def()).
      */
     DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
+    DEFINE_PROP_BOOL("legacy-multi-node", X86CPU, legacy_multi_node, false),
     DEFINE_PROP_BOOL("xen-vapic", X86CPU, xen_vapic, false),
 
     /*
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index ef987f344c..6ef4396fc5 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1989,6 +1989,12 @@ struct ArchCPU {
      */
     bool legacy_cache;
 
+    /* Compatibility bits for old machine types.
+     * If true decode the CPUID Function 0x8000001E_ECX to support multiple
+     * nodes per processor
+     */
+    bool legacy_multi_node;
+
     /* Compatibility bits for old machine types: */
     bool enable_cpuid_0xb;
 
-- 
2.34.1
Re: [PATCH v2] target/i386: Fix CPUID encoding of Fn8000001E_ECX
Posted by Moger, Babu 8 months, 1 week ago 
Any feedback or concerns with this patch? Otherwise can this be merged?
Thanks
Babu

On 1/2/24 17:17, Babu Moger wrote:
> Observed the following failure while booting the SEV-SNP guest and the
> guest fails to boot with the smp parameters:
> "-smp 192,sockets=1,dies=12,cores=8,threads=2".
> 
> qemu-system-x86_64: sev_snp_launch_update: SNP_LAUNCH_UPDATE ret=-5 fw_error=22 'Invalid parameter'
> qemu-system-x86_64: SEV-SNP: CPUID validation failed for function 0x8000001e, index: 0x0.
> provided: eax:0x00000000, ebx: 0x00000100, ecx: 0x00000b00, edx: 0x00000000
> expected: eax:0x00000000, ebx: 0x00000100, ecx: 0x00000300, edx: 0x00000000
> qemu-system-x86_64: SEV-SNP: failed update CPUID page
> 
> Reason for the failure is due to overflowing of bits used for "Node per
> processor" in CPUID Fn8000001E_ECX. This field's width is 3 bits wide and
> can hold maximum value 0x7. With dies=12 (0xB), it overflows and spills
> over into the reserved bits. In the case of SEV-SNP, this causes CPUID
> enforcement failure and guest fails to boot.
> 
> The PPR documentation for CPUID_Fn8000001E_ECX [Node Identifiers]
> =================================================================
> Bits    Description
> 31:11   Reserved.
> 
> 10:8    NodesPerProcessor: Node per processor. Read-only.
>         ValidValues:
>         Value   Description
>         0h      1 node per processor.
>         7h-1h   Reserved.
> 
> 7:0     NodeId: Node ID. Read-only. Reset: Fixed,XXh.
> =================================================================
> 
> As in the spec, the valid value for "node per processor" is 0 and rest
> are reserved.
> 
> Looking back at the history of decoding of CPUID_Fn8000001E_ECX, noticed
> that there were cases where "node per processor" can be more than 1. It
> is valid only for pre-F17h (pre-EPYC) architectures. For EPYC or later
> CPUs, the linux kernel does not use this information to build the L3
> topology.
> 
> Also noted that the CPUID Function 0x8000001E_ECX is available only when
> TOPOEXT feature is enabled. This feature is enabled only for EPYC(F17h)
> or later processors. So, previous generation of processors do not not
> enumerate 0x8000001E_ECX leaf.
> 
> There could be some corner cases where the older guests could enable the
> TOPOEXT feature by running with -cpu host, in which case legacy guests
> might notice the topology change. To address those cases introduced a
> new CPU property "legacy-multi-node". It will be true for older machine
> types to maintain compatibility. By default, it will be false, so new
> decoding will be used going forward.
> 
> The documentation is taken from Preliminary Processor Programming
> Reference (PPR) for AMD Family 19h Model 11h, Revision B1 Processors 55901
> Rev 0.25 - Oct 6, 2022.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 31ada106d891 ("Simplify CPUID_8000_001E for AMD")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> v2: Rebased to the latest tree.
>     Updated the pc_compat_8_2 for the new flag.
>     Added the comment for new property legacy_multi_node.
>     Added Reviwed-by from Zhao.
> ---
>  hw/i386/pc.c      |  4 +++-
>  target/i386/cpu.c | 18 ++++++++++--------
>  target/i386/cpu.h |  6 ++++++
>  3 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 496498df3a..a504e05e62 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -78,7 +78,9 @@
>      { "qemu64-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },\
>      { "athlon-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },
>  
> -GlobalProperty pc_compat_8_2[] = {};
> +GlobalProperty pc_compat_8_2[] = {
> +    { TYPE_X86_CPU, "legacy-multi-node", "on" },
> +};
>  const size_t pc_compat_8_2_len = G_N_ELEMENTS(pc_compat_8_2);
>  
>  GlobalProperty pc_compat_8_1[] = {};
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 95d5f16cd5..2cc84e8500 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -398,12 +398,9 @@ static void encode_topo_cpuid8000001e(X86CPU *cpu, X86CPUTopoInfo *topo_info,
>       * 31:11 Reserved.
>       * 10:8 NodesPerProcessor: Node per processor. Read-only. Reset: XXXb.
>       *      ValidValues:
> -     *      Value Description
> -     *      000b  1 node per processor.
> -     *      001b  2 nodes per processor.
> -     *      010b Reserved.
> -     *      011b 4 nodes per processor.
> -     *      111b-100b Reserved.
> +     *      Value   Description
> +     *      0h      1 node per processor.
> +     *      7h-1h   Reserved.
>       *  7:0 NodeId: Node ID. Read-only. Reset: XXh.
>       *
>       * NOTE: Hardware reserves 3 bits for number of nodes per processor.
> @@ -412,8 +409,12 @@ static void encode_topo_cpuid8000001e(X86CPU *cpu, X86CPUTopoInfo *topo_info,
>       * NodeId is combination of node and socket_id which is already decoded
>       * in apic_id. Just use it by shifting.
>       */
> -    *ecx = ((topo_info->dies_per_pkg - 1) << 8) |
> -           ((cpu->apic_id >> apicid_die_offset(topo_info)) & 0xFF);
> +    if (cpu->legacy_multi_node) {
> +        *ecx = ((topo_info->dies_per_pkg - 1) << 8) |
> +               ((cpu->apic_id >> apicid_die_offset(topo_info)) & 0xFF);
> +    } else {
> +        *ecx = (cpu->apic_id >> apicid_pkg_offset(topo_info)) & 0xFF;
> +    }
>  
>      *edx = 0;
>  }
> @@ -7895,6 +7896,7 @@ static Property x86_cpu_properties[] = {
>       * own cache information (see x86_cpu_load_def()).
>       */
>      DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
> +    DEFINE_PROP_BOOL("legacy-multi-node", X86CPU, legacy_multi_node, false),
>      DEFINE_PROP_BOOL("xen-vapic", X86CPU, xen_vapic, false),
>  
>      /*
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index ef987f344c..6ef4396fc5 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1989,6 +1989,12 @@ struct ArchCPU {
>       */
>      bool legacy_cache;
>  
> +    /* Compatibility bits for old machine types.
> +     * If true decode the CPUID Function 0x8000001E_ECX to support multiple
> +     * nodes per processor
> +     */
> +    bool legacy_multi_node;
> +
>      /* Compatibility bits for old machine types: */
>      bool enable_cpuid_0xb;
>  

-- 
Thanks
Babu Moger
[PATCH v3] target/i386: Fix CPUID encoding of Fn8000001E_ECX
Posted by Babu Moger 6 months, 3 weeks ago 
Observed the following failure while booting the SEV-SNP guest and the
guest fails to boot with the smp parameters:
"-smp 192,sockets=1,dies=12,cores=8,threads=2".

qemu-system-x86_64: sev_snp_launch_update: SNP_LAUNCH_UPDATE ret=-5 fw_error=22 'Invalid parameter'
qemu-system-x86_64: SEV-SNP: CPUID validation failed for function 0x8000001e, index: 0x0.
provided: eax:0x00000000, ebx: 0x00000100, ecx: 0x00000b00, edx: 0x00000000
expected: eax:0x00000000, ebx: 0x00000100, ecx: 0x00000300, edx: 0x00000000
qemu-system-x86_64: SEV-SNP: failed update CPUID page

Reason for the failure is due to overflowing of bits used for "Node per
processor" in CPUID Fn8000001E_ECX. This field's width is 3 bits wide and
can hold maximum value 0x7. With dies=12 (0xB), it overflows and spills
over into the reserved bits. In the case of SEV-SNP, this causes CPUID
enforcement failure and guest fails to boot.

The PPR documentation for CPUID_Fn8000001E_ECX [Node Identifiers]
=================================================================
Bits    Description
31:11   Reserved.

10:8    NodesPerProcessor: Node per processor. Read-only.
        ValidValues:
        Value   Description
        0h      1 node per processor.
        7h-1h   Reserved.

7:0     NodeId: Node ID. Read-only. Reset: Fixed,XXh.
=================================================================

As in the spec, the valid value for "node per processor" is 0 and rest
are reserved.

Looking back at the history of decoding of CPUID_Fn8000001E_ECX, noticed
that there were cases where "node per processor" can be more than 1. It
is valid only for pre-F17h (pre-EPYC) architectures. For EPYC or later
CPUs, the linux kernel does not use this information to build the L3
topology.

Also noted that the CPUID Function 0x8000001E_ECX is available only when
TOPOEXT feature is enabled. This feature is enabled only for EPYC(F17h)
or later processors. So, previous generation of processors do not not
enumerate 0x8000001E_ECX leaf.

There could be some corner cases where the older guests could enable the
TOPOEXT feature by running with -cpu host, in which case legacy guests
might notice the topology change. To address those cases introduced a
new CPU property "legacy-multi-node". It will be true for older machine
types to maintain compatibility. By default, it will be false, so new
decoding will be used going forward.

The documentation is taken from Preliminary Processor Programming
Reference (PPR) for AMD Family 19h Model 11h, Revision B1 Processors 55901
Rev 0.25 - Oct 6, 2022.

Cc: qemu-stable@nongnu.org
Fixes: 31ada106d891 ("Simplify CPUID_8000_001E for AMD")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v3:
  Rebased to the latest tree.
  Updated the pc_compat_9_0 for the new flag.

v2:
   https://lore.kernel.org/kvm/20240102231738.46553-1-babu.moger@amd.com/
   Rebased to the latest tree.
   Updated the pc_compat_8_2 for the new flag.
   Added the comment for new property legacy_multi_node.
   Added Reviwed-by from Zhao.

v1:
   https://lore.kernel.org/kvm/20231110170806.70962-1-babu.moger@amd.com/
---
 hw/i386/pc.c      |  1 +
 target/i386/cpu.c | 18 ++++++++++--------
 target/i386/cpu.h |  6 ++++++
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 08c7de416f..46235466d7 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -81,6 +81,7 @@
 GlobalProperty pc_compat_9_0[] = {
     { TYPE_X86_CPU, "guest-phys-bits", "0" },
     { "sev-guest", "legacy-vm-type", "true" },
+    { TYPE_X86_CPU, "legacy-multi-node", "on" },
 };
 const size_t pc_compat_9_0_len = G_N_ELEMENTS(pc_compat_9_0);
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index aa3b2d8391..ceb068027d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -398,12 +398,9 @@ static void encode_topo_cpuid8000001e(X86CPU *cpu, X86CPUTopoInfo *topo_info,
      * 31:11 Reserved.
      * 10:8 NodesPerProcessor: Node per processor. Read-only. Reset: XXXb.
      *      ValidValues:
-     *      Value Description
-     *      000b  1 node per processor.
-     *      001b  2 nodes per processor.
-     *      010b Reserved.
-     *      011b 4 nodes per processor.
-     *      111b-100b Reserved.
+     *      Value   Description
+     *      0h      1 node per processor.
+     *      7h-1h   Reserved.
      *  7:0 NodeId: Node ID. Read-only. Reset: XXh.
      *
      * NOTE: Hardware reserves 3 bits for number of nodes per processor.
@@ -412,8 +409,12 @@ static void encode_topo_cpuid8000001e(X86CPU *cpu, X86CPUTopoInfo *topo_info,
      * NodeId is combination of node and socket_id which is already decoded
      * in apic_id. Just use it by shifting.
      */
-    *ecx = ((topo_info->dies_per_pkg - 1) << 8) |
-           ((cpu->apic_id >> apicid_die_offset(topo_info)) & 0xFF);
+    if (cpu->legacy_multi_node) {
+        *ecx = ((topo_info->dies_per_pkg - 1) << 8) |
+               ((cpu->apic_id >> apicid_die_offset(topo_info)) & 0xFF);
+    } else {
+        *ecx = (cpu->apic_id >> apicid_pkg_offset(topo_info)) & 0xFF;
+    }
 
     *edx = 0;
 }
@@ -8073,6 +8074,7 @@ static Property x86_cpu_properties[] = {
      * own cache information (see x86_cpu_load_def()).
      */
     DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
+    DEFINE_PROP_BOOL("legacy-multi-node", X86CPU, legacy_multi_node, false),
     DEFINE_PROP_BOOL("xen-vapic", X86CPU, xen_vapic, false),
 
     /*
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 565c7a98c3..1e0d2c915f 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1994,6 +1994,12 @@ struct ArchCPU {
      */
     bool legacy_cache;
 
+    /* Compatibility bits for old machine types.
+     * If true decode the CPUID Function 0x8000001E_ECX to support multiple
+     * nodes per processor
+     */
+    bool legacy_multi_node;
+
     /* Compatibility bits for old machine types: */
     bool enable_cpuid_0xb;
 
-- 
2.34.1
Re: [PATCH v3] target/i386: Fix CPUID encoding of Fn8000001E_ECX
Posted by Michael Tokarev 6 months, 3 weeks ago 
03.05.2024 20:46, Babu Moger wrote:
> Observed the following failure while booting the SEV-SNP guest and the
> guest fails to boot with the smp parameters:
> "-smp 192,sockets=1,dies=12,cores=8,threads=2".
> 
> qemu-system-x86_64: sev_snp_launch_update: SNP_LAUNCH_UPDATE ret=-5 fw_error=22 'Invalid parameter'
> qemu-system-x86_64: SEV-SNP: CPUID validation failed for function 0x8000001e, index: 0x0.
> provided: eax:0x00000000, ebx: 0x00000100, ecx: 0x00000b00, edx: 0x00000000
> expected: eax:0x00000000, ebx: 0x00000100, ecx: 0x00000300, edx: 0x00000000
> qemu-system-x86_64: SEV-SNP: failed update CPUID page
...
> Cc: qemu-stable@nongnu.org
> Fixes: 31ada106d891 ("Simplify CPUID_8000_001E for AMD")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v3:
>    Rebased to the latest tree.
>    Updated the pc_compat_9_0 for the new flag.

> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 08c7de416f..46235466d7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -81,6 +81,7 @@
>   GlobalProperty pc_compat_9_0[] = {
>       { TYPE_X86_CPU, "guest-phys-bits", "0" },
>       { "sev-guest", "legacy-vm-type", "true" },
> +    { TYPE_X86_CPU, "legacy-multi-node", "on" },
>   };

Should this legacy-multi-node property be added to previous
machine types when applying to stable?  How about stable-8.2
and stable-7.2?

Thanks,

/mjt
Re: [PATCH v3] target/i386: Fix CPUID encoding of Fn8000001E_ECX
Posted by Daniel P. Berrangé 6 months, 3 weeks ago 
On Thu, May 09, 2024 at 04:54:16PM +0300, Michael Tokarev wrote:
> 03.05.2024 20:46, Babu Moger wrote:
> > Observed the following failure while booting the SEV-SNP guest and the
> > guest fails to boot with the smp parameters:
> > "-smp 192,sockets=1,dies=12,cores=8,threads=2".
> > 
> > qemu-system-x86_64: sev_snp_launch_update: SNP_LAUNCH_UPDATE ret=-5 fw_error=22 'Invalid parameter'
> > qemu-system-x86_64: SEV-SNP: CPUID validation failed for function 0x8000001e, index: 0x0.
> > provided: eax:0x00000000, ebx: 0x00000100, ecx: 0x00000b00, edx: 0x00000000
> > expected: eax:0x00000000, ebx: 0x00000100, ecx: 0x00000300, edx: 0x00000000
> > qemu-system-x86_64: SEV-SNP: failed update CPUID page
> ...
> > Cc: qemu-stable@nongnu.org
> > Fixes: 31ada106d891 ("Simplify CPUID_8000_001E for AMD")
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> > Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> > v3:
> >    Rebased to the latest tree.
> >    Updated the pc_compat_9_0 for the new flag.
> 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 08c7de416f..46235466d7 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -81,6 +81,7 @@
> >   GlobalProperty pc_compat_9_0[] = {
> >       { TYPE_X86_CPU, "guest-phys-bits", "0" },
> >       { "sev-guest", "legacy-vm-type", "true" },
> > +    { TYPE_X86_CPU, "legacy-multi-node", "on" },
> >   };
> 
> Should this legacy-multi-node property be added to previous
> machine types when applying to stable?  How about stable-8.2
> and stable-7.2?

machine types are considered to express a fixed guest ABI
once part of a QEMU release. Given that we should not be
changing existing machine types in stable branches.

In theory we could create new "bug fix" machine types in stable
branches. To support live migration, we would then need to also
add those same stable branch "bug fix" machine type versions in
all future QEMU versions. This is generally not worth the hassle
of exploding the number of machine types.

If you backport the patch, minus the machine type, then users
can still get the fix but they'll need to manually set the
property to enable it.

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 v3] target/i386: Fix CPUID encoding of Fn8000001E_ECX
Posted by Michael Tokarev 6 months, 2 weeks ago 
09.05.2024 17:11, Daniel P. Berrangé wrote:
> On Thu, May 09, 2024 at 04:54:16PM +0300, Michael Tokarev wrote:
>> 03.05.2024 20:46, Babu Moger wrote:

>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 08c7de416f..46235466d7 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -81,6 +81,7 @@
>>>    GlobalProperty pc_compat_9_0[] = {
>>>        { TYPE_X86_CPU, "guest-phys-bits", "0" },
>>>        { "sev-guest", "legacy-vm-type", "true" },
>>> +    { TYPE_X86_CPU, "legacy-multi-node", "on" },
>>>    };
>>
>> Should this legacy-multi-node property be added to previous
>> machine types when applying to stable?  How about stable-8.2
>> and stable-7.2?
> 
> machine types are considered to express a fixed guest ABI
> once part of a QEMU release. Given that we should not be
> changing existing machine types in stable branches.

Yes, I understand this, and this is exactly why I asked.
The change in question has been Cc'ed to stable.  And I'm
trying to understand what should I do with it :)

> In theory we could create new "bug fix" machine types in stable
> branches. To support live migration, we would then need to also
> add those same stable branch "bug fix" machine type versions in
> all future QEMU versions. This is generally not worth the hassle
> of exploding the number of machine types.
> 
> If you backport the patch, minus the machine type, then users
> can still get the fix but they'll need to manually set the
> property to enable it.

I don't think this makes big sense.  But maybe for someone who
actually hits this issue such backport will let to fix it.
Hence, again, I'm asking if it really a good idea to pick this
up for stable (any version of, - currently there are 2 active
series, 7.2, 8.2 and 9.0).

Also, the parameter has to be compatible with 9.1+ (maybe having
different default).

Thanks,

/mjt

-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt
Re: [PATCH v3] target/i386: Fix CPUID encoding of Fn8000001E_ECX
Posted by Daniel P. Berrangé 6 months, 2 weeks ago 
On Fri, May 10, 2024 at 11:05:44AM +0300, Michael Tokarev wrote:
> 09.05.2024 17:11, Daniel P. Berrangé wrote:
> > On Thu, May 09, 2024 at 04:54:16PM +0300, Michael Tokarev wrote:
> > > 03.05.2024 20:46, Babu Moger wrote:
> 
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index 08c7de416f..46235466d7 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -81,6 +81,7 @@
> > > >    GlobalProperty pc_compat_9_0[] = {
> > > >        { TYPE_X86_CPU, "guest-phys-bits", "0" },
> > > >        { "sev-guest", "legacy-vm-type", "true" },
> > > > +    { TYPE_X86_CPU, "legacy-multi-node", "on" },
> > > >    };
> > > 
> > > Should this legacy-multi-node property be added to previous
> > > machine types when applying to stable?  How about stable-8.2
> > > and stable-7.2?
> > 
> > machine types are considered to express a fixed guest ABI
> > once part of a QEMU release. Given that we should not be
> > changing existing machine types in stable branches.
> 
> Yes, I understand this, and this is exactly why I asked.
> The change in question has been Cc'ed to stable.  And I'm
> trying to understand what should I do with it :)
> 
> > In theory we could create new "bug fix" machine types in stable
> > branches. To support live migration, we would then need to also
> > add those same stable branch "bug fix" machine type versions in
> > all future QEMU versions. This is generally not worth the hassle
> > of exploding the number of machine types.
> > 
> > If you backport the patch, minus the machine type, then users
> > can still get the fix but they'll need to manually set the
> > property to enable it.
> 
> I don't think this makes big sense.  But maybe for someone who
> actually hits this issue such backport will let to fix it.
> Hence, again, I'm asking if it really a good idea to pick this
> up for stable (any version of, - currently there are 2 active
> series, 7.2, 8.2 and 9.0).

Hmm, the description says

  "Observed the following failure while booting the SEV-SNP guest"

and yet the patches for SEV-SNP are *not* merged in QEMU yet. So this
does not look relevant for stable unless I'm missing something.

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 v3] target/i386: Fix CPUID encoding of Fn8000001E_ECX
Posted by Moger, Babu 6 months, 2 weeks ago 
Hi Daniel,

On 5/10/2024 3:10 AM, Daniel P. Berrangé wrote:
> On Fri, May 10, 2024 at 11:05:44AM +0300, Michael Tokarev wrote:
>> 09.05.2024 17:11, Daniel P. Berrangé wrote:
>>> On Thu, May 09, 2024 at 04:54:16PM +0300, Michael Tokarev wrote:
>>>> 03.05.2024 20:46, Babu Moger wrote:
>>
>>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>>> index 08c7de416f..46235466d7 100644
>>>>> --- a/hw/i386/pc.c
>>>>> +++ b/hw/i386/pc.c
>>>>> @@ -81,6 +81,7 @@
>>>>>     GlobalProperty pc_compat_9_0[] = {
>>>>>         { TYPE_X86_CPU, "guest-phys-bits", "0" },
>>>>>         { "sev-guest", "legacy-vm-type", "true" },
>>>>> +    { TYPE_X86_CPU, "legacy-multi-node", "on" },
>>>>>     };
>>>>
>>>> Should this legacy-multi-node property be added to previous
>>>> machine types when applying to stable?  How about stable-8.2
>>>> and stable-7.2?
>>>
>>> machine types are considered to express a fixed guest ABI
>>> once part of a QEMU release. Given that we should not be
>>> changing existing machine types in stable branches.
>>
>> Yes, I understand this, and this is exactly why I asked.
>> The change in question has been Cc'ed to stable.  And I'm
>> trying to understand what should I do with it :)
>>
>>> In theory we could create new "bug fix" machine types in stable
>>> branches. To support live migration, we would then need to also
>>> add those same stable branch "bug fix" machine type versions in
>>> all future QEMU versions. This is generally not worth the hassle
>>> of exploding the number of machine types.
>>>
>>> If you backport the patch, minus the machine type, then users
>>> can still get the fix but they'll need to manually set the
>>> property to enable it.
>>
>> I don't think this makes big sense.  But maybe for someone who
>> actually hits this issue such backport will let to fix it.
>> Hence, again, I'm asking if it really a good idea to pick this
>> up for stable (any version of, - currently there are 2 active
>> series, 7.2, 8.2 and 9.0).
> 
> Hmm, the description says
> 
>    "Observed the following failure while booting the SEV-SNP guest"
> 
> and yet the patches for SEV-SNP are *not* merged in QEMU yet. So this
> does not look relevant for stable unless I'm missing something.

I have not thought thru about stable tag. This is not critical for 
stable release.

If required I will send a separate patch for stable later. It is not 
required right now. Sorry about the noise.

-- 
- Babu Moger
Re: [PATCH v3] target/i386: Fix CPUID encoding of Fn8000001E_ECX
Posted by Paolo Bonzini 6 months, 3 weeks ago 
Queued, thanks.

Paolo

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.