Re: [Qemu-devel] [PATCH] target-i386/cpu: Add new EYPC CPU model

[Eduardo Habkost]

Hi,

Thanks for the patch.

On Mon, Aug 14, 2017 at 10:52:17AM -0500, Brijesh Singh wrote:
> Add a new base CPU model called 'EPYC' to model processors from AMD EPYC
> family (which includes EPYC 76xx,75xx,74xx,73xx and 72xx).

I suggest enumerating in the commit message which features were
added to the CPU model in comparison to Opteron_G5.

> 
> Cc: Paolo Bonzini <address@hidden>
> Cc: Richard Henderson <address@hidden>
> Cc: Eduardo Habkost <address@hidden>
> Cc: Tom Lendacky <address@hidden>
> Signed-off-by: Brijesh Singh <address@hidden>
> ---
>  target/i386/cpu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ddc45ab..ed1708b 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1522,6 +1522,50 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .xlevel = 0x8000001A,
>          .model_id = "AMD Opteron 63xx class CPU",
>      },
> +    {
> +        .name = "EPYC",
> +        .level = 0xd,
> +        .vendor = CPUID_VENDOR_AMD,
> +        .family = 23,
> +        .model = 1,
> +        .stepping = 2,
[...]
> +        /* Missing: XSAVES (not supported by some Linux versions,
> +         * including v4.1 to v4.12).
> +         * KVM doesn't yet expose any XSAVES state save component.
> +         */

Do you know which supervisor state components are available in
EPYC CPUs?  Do you have a pointer to public AMD documentation
about XSAVES?


> +        .features[FEAT_XSAVE] =
> +            CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
> +            CPUID_XSAVE_XGETBV1,
> +        .features[FEAT_6_EAX] =
> +            CPUID_6_EAX_ARAT,
> +        .xlevel = 0x8000001F,

All CPUID leaves from 0x8000000B to 0x8000001F return all-zeroes
today.  If we set xlevel to 0x8000001F before we actually
implement those CPUID leaves, we will be forced to add extra
machine-type compat code when we finally implement them.

I suggest setting it to 0x8000000A, and increasing it only after
we actually implement the new CPUID leaves.


> +        .model_id = "AMD EYPC Processor",
> +    },
>  };
>  
>  typedef struct PropValue {
> -- 
> 2.9.4
> 

-- 
Eduardo