diff mbox series

[v5,1/6] target/i386: Update EPYC CPU model for Cache property, RAS, SVM feature bits

Message ID c777bf763a636c8922164a174685b4f03864452f.1738869208.git.babu.moger@amd.com (mailing list archive)
State New
Headers show
Series target/i386: Update EPYC CPU models for Cache property, RAS, SVM feature and add EPYC-Turin CPU model | expand

Commit Message

Babu Moger Feb. 6, 2025, 7:28 p.m. UTC 
Found that some of the cache properties are not set correctly for EPYC models.

l1d_cache.no_invd_sharing should not be true.
l1i_cache.no_invd_sharing should not be true.

L2.self_init should be true.
L2.inclusive should be true.

L3.inclusive should not be true.
L3.no_invd_sharing should be true.

Fix the cache properties.

Also add the missing RAS and SVM features bits on AMD
EPYC CPU models. The SVM feature bits are used in nested guests.

succor		: Software uncorrectable error containment and recovery capability.
overflow-recov	: MCA overflow recovery support.
lbrv		: LBR virtualization
tsc-scale	: MSR based TSC rate control
vmcb-clean	: VMCB clean bits
flushbyasid	: Flush by ASID
pause-filter	: Pause intercept filter
pfthreshold	: PAUSE filter threshold
v-vmsave-vmload	: Virtualized VMLOAD and VMSAVE
vgif		: Virtualized GIF

Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Maksim Davydov <davydov-max@yandex-team.ru>
---
 target/i386/cpu.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

Comments

Zhao Liu Feb. 20, 2025, 10:59 a.m. UTC | #1 
> +static CPUCaches epyc_v5_cache_info = {
> +    .l1d_cache = &(CPUCacheInfo) {
> +        .type = DATA_CACHE,
> +        .level = 1,
> +        .size = 32 * KiB,
> +        .line_size = 64,
> +        .associativity = 8,
> +        .partitions = 1,
> +        .sets = 64,
> +        .lines_per_tag = 1,
> +        .self_init = 1,

For consistency as the below parts, it's better to code `true` for all
boolean types.

> +        .share_level = CPU_TOPOLOGY_LEVEL_CORE,
> +    },
> +    .l1i_cache = &(CPUCacheInfo) {
> +        .type = INSTRUCTION_CACHE,
> +        .level = 1,
> +        .size = 64 * KiB,
> +        .line_size = 64,
> +        .associativity = 4,
> +        .partitions = 1,
> +        .sets = 256,
> +        .lines_per_tag = 1,
> +        .self_init = 1,

ditto.

Others are fine for me, so,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>


And one more thing :-) ...

>  static const CPUCaches epyc_rome_cache_info = {
>      .l1d_cache = &(CPUCacheInfo) {
>          .type = DATA_CACHE,
> @@ -5207,6 +5261,25 @@ static const X86CPUDefinition builtin_x86_defs[] = {
>                  },
>                  .cache_info = &epyc_v4_cache_info
>              },
> +            {
> +                .version = 5,
> +                .props = (PropValue[]) {
> +                    { "overflow-recov", "on" },
> +                    { "succor", "on" },

When I checks the "overflow-recov" and "succor" enabling, I find these 2
bits are set unconditionally.

I'm not sure if all AMD platforms support both bits, do you think it's
necessary to check the host support?

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 6c749d4ee812..03e463076632 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -555,7 +555,10 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
         cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
         ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES;
     } else if (function == 0x80000007 && reg == R_EBX) {
-        ret |= CPUID_8000_0007_EBX_OVERFLOW_RECOV | CPUID_8000_0007_EBX_SUCCOR;
+        uint32_t ebx;
+        host_cpuid(0x80000007, 0, &unused, &ebx, &unused, &unused);
+
+        ret |= ebx & (CPUID_8000_0007_EBX_OVERFLOW_RECOV | CPUID_8000_0007_EBX_SUCCOR);
     } else if (function == KVM_CPUID_FEATURES && reg == R_EAX) {
         /* kvm_pv_unhalt is reported by GET_SUPPORTED_CPUID, but it can't
          * be enabled without the in-kernel irqchip

Thanks,
Zhao
John Allen Feb. 25, 2025, 5:01 p.m. UTC | #2 
On Thu, Feb 20, 2025 at 06:59:34PM +0800, Zhao Liu wrote:
> And one more thing :-) ...
> 
> >  static const CPUCaches epyc_rome_cache_info = {
> >      .l1d_cache = &(CPUCacheInfo) {
> >          .type = DATA_CACHE,
> > @@ -5207,6 +5261,25 @@ static const X86CPUDefinition builtin_x86_defs[] = {
> >                  },
> >                  .cache_info = &epyc_v4_cache_info
> >              },
> > +            {
> > +                .version = 5,
> > +                .props = (PropValue[]) {
> > +                    { "overflow-recov", "on" },
> > +                    { "succor", "on" },
> 
> When I checks the "overflow-recov" and "succor" enabling, I find these 2
> bits are set unconditionally.
> 
> I'm not sure if all AMD platforms support both bits, do you think it's
> necessary to check the host support?

Hi Zhao,

IIRC, we intentionally set these unconditionally since there is no
specific support needed from the host side for guests to use these bits
to handle MCEs. See the original discussion and rationale in this
thread:

https://lore.kernel.org/all/20230706194022.2485195-2-john.allen@amd.com/

However, this discussion only applied to the SUCCOR feature and not the
OVERFLOW_RECOV feature and now that you bring it up, I'm second guessing
whether we can apply the same thinking to OVERFLOW_RECOV. I think we may
want to keep setting the SUCCOR bit unconditionally, but we may want to
handle OVERFLOW_RECOV normally. I'll have to track down some old
hardware to see how this behaves when the hardware doesn't support it.

Thanks,
John

> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 6c749d4ee812..03e463076632 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -555,7 +555,10 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
>          cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
>          ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES;
>      } else if (function == 0x80000007 && reg == R_EBX) {
> -        ret |= CPUID_8000_0007_EBX_OVERFLOW_RECOV | CPUID_8000_0007_EBX_SUCCOR;
> +        uint32_t ebx;
> +        host_cpuid(0x80000007, 0, &unused, &ebx, &unused, &unused);
> +
> +        ret |= ebx & (CPUID_8000_0007_EBX_OVERFLOW_RECOV | CPUID_8000_0007_EBX_SUCCOR);
>      } else if (function == KVM_CPUID_FEATURES && reg == R_EAX) {
>          /* kvm_pv_unhalt is reported by GET_SUPPORTED_CPUID, but it can't
>           * be enabled without the in-kernel irqchip
> 
> Thanks,
> Zhao
> 
>
Babu Moger Feb. 26, 2025, 8:28 p.m. UTC | #3 
Hi John,

On 2/25/25 11:01, John Allen wrote:
> On Thu, Feb 20, 2025 at 06:59:34PM +0800, Zhao Liu wrote:
>> And one more thing :-) ...
>>
>>>  static const CPUCaches epyc_rome_cache_info = {
>>>      .l1d_cache = &(CPUCacheInfo) {
>>>          .type = DATA_CACHE,
>>> @@ -5207,6 +5261,25 @@ static const X86CPUDefinition builtin_x86_defs[] = {
>>>                  },
>>>                  .cache_info = &epyc_v4_cache_info
>>>              },
>>> +            {
>>> +                .version = 5,
>>> +                .props = (PropValue[]) {
>>> +                    { "overflow-recov", "on" },
>>> +                    { "succor", "on" },
>>
>> When I checks the "overflow-recov" and "succor" enabling, I find these 2
>> bits are set unconditionally.
>>
>> I'm not sure if all AMD platforms support both bits, do you think it's
>> necessary to check the host support?
> 
> Hi Zhao,
> 
> IIRC, we intentionally set these unconditionally since there is no
> specific support needed from the host side for guests to use these bits
> to handle MCEs. See the original discussion and rationale in this
> thread:
> 
> https://lore.kernel.org/all/20230706194022.2485195-2-john.allen@amd.com/
> 
> However, this discussion only applied to the SUCCOR feature and not the
> OVERFLOW_RECOV feature and now that you bring it up, I'm second guessing
> whether we can apply the same thinking to OVERFLOW_RECOV. I think we may
> want to keep setting the SUCCOR bit unconditionally, but we may want to
> handle OVERFLOW_RECOV normally. I'll have to track down some old
> hardware to see how this behaves when the hardware doesn't support it.

Yes. We need to verify it on pre-EPYC hardware. Please let us know how it
goes.

But, this series updates only the EPYC based CPU models. It should not be
a concern here. Right?


> 
> Thanks,
> John
> 
>>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index 6c749d4ee812..03e463076632 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -555,7 +555,10 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
>>          cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
>>          ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES;
>>      } else if (function == 0x80000007 && reg == R_EBX) {
>> -        ret |= CPUID_8000_0007_EBX_OVERFLOW_RECOV | CPUID_8000_0007_EBX_SUCCOR;
>> +        uint32_t ebx;
>> +        host_cpuid(0x80000007, 0, &unused, &ebx, &unused, &unused);
>> +
>> +        ret |= ebx & (CPUID_8000_0007_EBX_OVERFLOW_RECOV | CPUID_8000_0007_EBX_SUCCOR);
>>      } else if (function == KVM_CPUID_FEATURES && reg == R_EAX) {
>>          /* kvm_pv_unhalt is reported by GET_SUPPORTED_CPUID, but it can't
>>           * be enabled without the in-kernel irqchip
>>
>> Thanks,
>> Zhao
>>
>>
>
Zhao Liu Feb. 27, 2025, 6:42 a.m. UTC | #4 
On Wed, Feb 26, 2025 at 02:28:35PM -0600, Moger, Babu wrote:
> Date: Wed, 26 Feb 2025 14:28:35 -0600
> From: "Moger, Babu" <babu.moger@amd.com>
> Subject: Re: [PATCH v5 1/6] target/i386: Update EPYC CPU model for Cache
>  property, RAS, SVM feature bits
> 
> Hi John,
> 
> On 2/25/25 11:01, John Allen wrote:
> > On Thu, Feb 20, 2025 at 06:59:34PM +0800, Zhao Liu wrote:
> >> And one more thing :-) ...
> >>
> >>>  static const CPUCaches epyc_rome_cache_info = {
> >>>      .l1d_cache = &(CPUCacheInfo) {
> >>>          .type = DATA_CACHE,
> >>> @@ -5207,6 +5261,25 @@ static const X86CPUDefinition builtin_x86_defs[] = {
> >>>                  },
> >>>                  .cache_info = &epyc_v4_cache_info
> >>>              },
> >>> +            {
> >>> +                .version = 5,
> >>> +                .props = (PropValue[]) {
> >>> +                    { "overflow-recov", "on" },
> >>> +                    { "succor", "on" },
> >>
> >> When I checks the "overflow-recov" and "succor" enabling, I find these 2
> >> bits are set unconditionally.
> >>
> >> I'm not sure if all AMD platforms support both bits, do you think it's
> >> necessary to check the host support?
> > 
> > Hi Zhao,
> > 
> > IIRC, we intentionally set these unconditionally since there is no
> > specific support needed from the host side for guests to use these bits
> > to handle MCEs. See the original discussion and rationale in this
> > thread:
> > 
> > https://lore.kernel.org/all/20230706194022.2485195-2-john.allen@amd.com/
> > 
> > However, this discussion only applied to the SUCCOR feature and not the
> > OVERFLOW_RECOV feature and now that you bring it up, I'm second guessing
> > whether we can apply the same thinking to OVERFLOW_RECOV. I think we may
> > want to keep setting the SUCCOR bit unconditionally, but we may want to
> > handle OVERFLOW_RECOV normally. I'll have to track down some old
> > hardware to see how this behaves when the hardware doesn't support it.

Yes, thanks!

> Yes. We need to verify it on pre-EPYC hardware. Please let us know how it
> goes.
> 
> But, this series updates only the EPYC based CPU models. It should not be
> a concern here. Right?

Yes, it doesn't block this series. :-)

Thank you both,
Zhao
diff mbox series

Patch

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b5dd60d281..94292bfaa2 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2180,6 +2180,60 @@  static CPUCaches epyc_v4_cache_info = {
     },
 };
 
+static CPUCaches epyc_v5_cache_info = {
+    .l1d_cache = &(CPUCacheInfo) {
+        .type = DATA_CACHE,
+        .level = 1,
+        .size = 32 * KiB,
+        .line_size = 64,
+        .associativity = 8,
+        .partitions = 1,
+        .sets = 64,
+        .lines_per_tag = 1,
+        .self_init = 1,
+        .share_level = CPU_TOPOLOGY_LEVEL_CORE,
+    },
+    .l1i_cache = &(CPUCacheInfo) {
+        .type = INSTRUCTION_CACHE,
+        .level = 1,
+        .size = 64 * KiB,
+        .line_size = 64,
+        .associativity = 4,
+        .partitions = 1,
+        .sets = 256,
+        .lines_per_tag = 1,
+        .self_init = 1,
+        .share_level = CPU_TOPOLOGY_LEVEL_CORE,
+    },
+    .l2_cache = &(CPUCacheInfo) {
+        .type = UNIFIED_CACHE,
+        .level = 2,
+        .size = 512 * KiB,
+        .line_size = 64,
+        .associativity = 8,
+        .partitions = 1,
+        .sets = 1024,
+        .lines_per_tag = 1,
+        .self_init = true,
+        .inclusive = true,
+        .share_level = CPU_TOPOLOGY_LEVEL_CORE,
+    },
+    .l3_cache = &(CPUCacheInfo) {
+        .type = UNIFIED_CACHE,
+        .level = 3,
+        .size = 8 * MiB,
+        .line_size = 64,
+        .associativity = 16,
+        .partitions = 1,
+        .sets = 8192,
+        .lines_per_tag = 1,
+        .self_init = true,
+        .no_invd_sharing = true,
+        .complex_indexing = false,
+        .share_level = CPU_TOPOLOGY_LEVEL_DIE,
+    },
+};
+
 static const CPUCaches epyc_rome_cache_info = {
     .l1d_cache = &(CPUCacheInfo) {
         .type = DATA_CACHE,
@@ -5207,6 +5261,25 @@  static const X86CPUDefinition builtin_x86_defs[] = {
                 },
                 .cache_info = &epyc_v4_cache_info
             },
+            {
+                .version = 5,
+                .props = (PropValue[]) {
+                    { "overflow-recov", "on" },
+                    { "succor", "on" },
+                    { "lbrv", "on" },
+                    { "tsc-scale", "on" },
+                    { "vmcb-clean", "on" },
+                    { "flushbyasid", "on" },
+                    { "pause-filter", "on" },
+                    { "pfthreshold", "on" },
+                    { "v-vmsave-vmload", "on" },
+                    { "vgif", "on" },
+                    { "model-id",
+                      "AMD EPYC-v5 Processor" },
+                    { /* end of list */ }
+                },
+                .cache_info = &epyc_v5_cache_info
+            },
             { /* end of list */ }
         }
     },