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

[Zhao Liu <zhao1.liu@xxxxxxxxx>]

> +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@xxxxxxxxx>


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