[Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model

Pu Wen posted 1 patch 5 years ago
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1555124080-27089-1-git-send-email-puwen@hygon.cn
Maintainers: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Richard Henderson <rth@twiddle.net>, Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
hw/i386/pc.c      |  3 +++
target/i386/cpu.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
target/i386/cpu.h |  2 ++
3 files changed, 55 insertions(+)
[Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
Posted by Pu Wen 5 years ago
Add a new base CPU model called 'Dhyana' to model processors from Hygon
Dhyana(family 18h), which derived from AMD EPYC(family 17h).

The following features bits have been removed compare to AMD EPYC:
aes, pclmulqdq, sha_ni

The Hygon Dhyana support to KVM in Linux is already accepted upstream[1].
So add Hygon Dhyana support to Qemu is necessary to create Hygon's own
CPU model.

Reference:
[1] https://git.kernel.org/tip/fec98069fb72fb656304a3e52265e0c2fc9adf87

Signed-off-by: Pu Wen <puwen@hygon.cn>
---
v1->v2:
  - Remove CPU model 'Dhyana' and rename the CPU model 'Dhyana-IBPB' to
    'Dhyana' because Dhyana CPUs already have the IBPB feature.

 hw/i386/pc.c      |  3 +++
 target/i386/cpu.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 target/i386/cpu.h |  2 ++
 3 files changed, 55 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f2c15bf..551bec9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -128,6 +128,8 @@ GlobalProperty pc_compat_3_1[] = {
     { "EPYC" "-" TYPE_X86_CPU, "nrip-save", "off" },
     { "EPYC-IBPB" "-" TYPE_X86_CPU, "npt", "off" },
     { "EPYC-IBPB" "-" TYPE_X86_CPU, "nrip-save", "off" },
+    { "Dhyana" "-" TYPE_X86_CPU, "npt", "off" },
+    { "Dhyana" "-" TYPE_X86_CPU, "nrip-save", "off" },
     { "Skylake-Client" "-" TYPE_X86_CPU,      "mpx", "on" },
     { "Skylake-Client-IBRS" "-" TYPE_X86_CPU, "mpx", "on" },
     { "Skylake-Server" "-" TYPE_X86_CPU,      "mpx", "on" },
@@ -152,6 +154,7 @@ GlobalProperty pc_compat_2_12[] = {
     { TYPE_X86_CPU, "topoext", "off" },
     { "EPYC-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
     { "EPYC-IBPB-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
+    { "Dhyana-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
 };
 const size_t pc_compat_2_12_len = G_N_ELEMENTS(pc_compat_2_12);
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d6bb57d..d314c5f 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2934,6 +2934,56 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .model_id = "AMD EPYC Processor (with IBPB)",
         .cache_info = &epyc_cache_info,
     },
+    {
+        .name = "Dhyana",
+        .level = 0xd,
+        .vendor = CPUID_VENDOR_HYGON,
+        .family = 24,
+        .model = 0,
+        .stepping = 1,
+        .features[FEAT_1_EDX] =
+            CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX | CPUID_CLFLUSH |
+            CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA | CPUID_PGE |
+            CPUID_MTRR | CPUID_SEP | CPUID_APIC | CPUID_CX8 | CPUID_MCE |
+            CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE | CPUID_DE |
+            CPUID_VME | CPUID_FP87,
+        .features[FEAT_1_ECX] =
+            CPUID_EXT_RDRAND | CPUID_EXT_F16C | CPUID_EXT_AVX |
+            CPUID_EXT_XSAVE | CPUID_EXT_POPCNT |
+            CPUID_EXT_MOVBE | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 |
+            CPUID_EXT_CX16 | CPUID_EXT_FMA | CPUID_EXT_SSSE3 |
+            CPUID_EXT_MONITOR | CPUID_EXT_SSE3,
+        .features[FEAT_8000_0001_EDX] =
+            CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_PDPE1GB |
+            CPUID_EXT2_FFXSR | CPUID_EXT2_MMXEXT | CPUID_EXT2_NX |
+            CPUID_EXT2_SYSCALL,
+        .features[FEAT_8000_0001_ECX] =
+            CPUID_EXT3_OSVW | CPUID_EXT3_3DNOWPREFETCH |
+            CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM |
+            CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM |
+            CPUID_EXT3_TOPOEXT,
+        .features[FEAT_8000_0008_EBX] =
+            CPUID_8000_0008_EBX_IBPB,
+        .features[FEAT_7_0_EBX] =
+            CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_AVX2 |
+            CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_RDSEED |
+            CPUID_7_0_EBX_ADX | CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_CLFLUSHOPT,
+        /*
+         * Missing: XSAVES (not supported by some Linux versions,
+         * including v4.1 to v4.12).
+         * KVM doesn't yet expose any XSAVES state save component.
+         */
+        .features[FEAT_XSAVE] =
+            CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
+            CPUID_XSAVE_XGETBV1,
+        .features[FEAT_6_EAX] =
+            CPUID_6_EAX_ARAT,
+        .features[FEAT_SVM] =
+            CPUID_SVM_NPT | CPUID_SVM_NRIPSAVE,
+        .xlevel = 0x8000001E,
+        .model_id = "Hygon Dhyana Processor",
+        .cache_info = &epyc_cache_info,
+    },
 };
 
 typedef struct PropValue {
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 83fb522..553dbe7 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -726,6 +726,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 
 #define CPUID_VENDOR_VIA   "CentaurHauls"
 
+#define CPUID_VENDOR_HYGON    "HygonGenuine"
+
 #define CPUID_MWAIT_IBE     (1U << 1) /* Interrupts can exit capability */
 #define CPUID_MWAIT_EMX     (1U << 0) /* enumeration supported */
 
-- 
2.7.4


Re: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
Posted by Daniel P. Berrangé 5 years ago
On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote:
> Add a new base CPU model called 'Dhyana' to model processors from Hygon
> Dhyana(family 18h), which derived from AMD EPYC(family 17h).
> 
> The following features bits have been removed compare to AMD EPYC:
> aes, pclmulqdq, sha_ni
> 
> The Hygon Dhyana support to KVM in Linux is already accepted upstream[1].
> So add Hygon Dhyana support to Qemu is necessary to create Hygon's own
> CPU model.
> 
> Reference:
> [1] https://git.kernel.org/tip/fec98069fb72fb656304a3e52265e0c2fc9adf87
> 
> Signed-off-by: Pu Wen <puwen@hygon.cn>
> ---
> v1->v2:
>   - Remove CPU model 'Dhyana' and rename the CPU model 'Dhyana-IBPB' to
>     'Dhyana' because Dhyana CPUs already have the IBPB feature.
> 
>  hw/i386/pc.c      |  3 +++
>  target/i386/cpu.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  target/i386/cpu.h |  2 ++
>  3 files changed, 55 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f2c15bf..551bec9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -128,6 +128,8 @@ GlobalProperty pc_compat_3_1[] = {
>      { "EPYC" "-" TYPE_X86_CPU, "nrip-save", "off" },
>      { "EPYC-IBPB" "-" TYPE_X86_CPU, "npt", "off" },
>      { "EPYC-IBPB" "-" TYPE_X86_CPU, "nrip-save", "off" },
> +    { "Dhyana" "-" TYPE_X86_CPU, "npt", "off" },
> +    { "Dhyana" "-" TYPE_X86_CPU, "nrip-save", "off" },
>      { "Skylake-Client" "-" TYPE_X86_CPU,      "mpx", "on" },
>      { "Skylake-Client-IBRS" "-" TYPE_X86_CPU, "mpx", "on" },
>      { "Skylake-Server" "-" TYPE_X86_CPU,      "mpx", "on" },
> @@ -152,6 +154,7 @@ GlobalProperty pc_compat_2_12[] = {
>      { TYPE_X86_CPU, "topoext", "off" },
>      { "EPYC-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
>      { "EPYC-IBPB-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
> +    { "Dhyana-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
>  };
>  const size_t pc_compat_2_12_len = G_N_ELEMENTS(pc_compat_2_12);

You can drop the changes in this file. This CPU model didn't exist
in any older QEMU releases, so there's no machine type backcompat
required, at least from upstream QEMU POV.


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: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
Posted by Pu Wen 5 years ago
On 2019/4/15 17:25, Daniel P. Berrangé wrote:
> On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote:
...
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index f2c15bf..551bec9 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -128,6 +128,8 @@ GlobalProperty pc_compat_3_1[] = {
>>       { "EPYC" "-" TYPE_X86_CPU, "nrip-save", "off" },
>>       { "EPYC-IBPB" "-" TYPE_X86_CPU, "npt", "off" },
>>       { "EPYC-IBPB" "-" TYPE_X86_CPU, "nrip-save", "off" },
>> +    { "Dhyana" "-" TYPE_X86_CPU, "npt", "off" },
>> +    { "Dhyana" "-" TYPE_X86_CPU, "nrip-save", "off" },
>>       { "Skylake-Client" "-" TYPE_X86_CPU,      "mpx", "on" },
>>       { "Skylake-Client-IBRS" "-" TYPE_X86_CPU, "mpx", "on" },
>>       { "Skylake-Server" "-" TYPE_X86_CPU,      "mpx", "on" },
>> @@ -152,6 +154,7 @@ GlobalProperty pc_compat_2_12[] = {
>>       { TYPE_X86_CPU, "topoext", "off" },
>>       { "EPYC-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
>>       { "EPYC-IBPB-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
>> +    { "Dhyana-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
>>   };
>>   const size_t pc_compat_2_12_len = G_N_ELEMENTS(pc_compat_2_12);
> 
> You can drop the changes in this file. This CPU model didn't exist
> in any older QEMU releases, so there's no machine type backcompat
> required, at least from upstream QEMU POV.

Then how about running QEMU with the parameter like "-M pc-i440fx-2.12"? 
Although the default machine is newer than that.

-- 
Regards,
Pu Wen

Re: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
Posted by Daniel P. Berrangé 5 years ago
On Tue, Apr 16, 2019 at 02:56:12PM +0800, Pu Wen wrote:
> On 2019/4/15 17:25, Daniel P. Berrangé wrote:
> > On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote:
> ...
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index f2c15bf..551bec9 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -128,6 +128,8 @@ GlobalProperty pc_compat_3_1[] = {
> > >       { "EPYC" "-" TYPE_X86_CPU, "nrip-save", "off" },
> > >       { "EPYC-IBPB" "-" TYPE_X86_CPU, "npt", "off" },
> > >       { "EPYC-IBPB" "-" TYPE_X86_CPU, "nrip-save", "off" },
> > > +    { "Dhyana" "-" TYPE_X86_CPU, "npt", "off" },
> > > +    { "Dhyana" "-" TYPE_X86_CPU, "nrip-save", "off" },
> > >       { "Skylake-Client" "-" TYPE_X86_CPU,      "mpx", "on" },
> > >       { "Skylake-Client-IBRS" "-" TYPE_X86_CPU, "mpx", "on" },
> > >       { "Skylake-Server" "-" TYPE_X86_CPU,      "mpx", "on" },
> > > @@ -152,6 +154,7 @@ GlobalProperty pc_compat_2_12[] = {
> > >       { TYPE_X86_CPU, "topoext", "off" },
> > >       { "EPYC-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
> > >       { "EPYC-IBPB-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
> > > +    { "Dhyana-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
> > >   };
> > >   const size_t pc_compat_2_12_len = G_N_ELEMENTS(pc_compat_2_12);
> > 
> > You can drop the changes in this file. This CPU model didn't exist
> > in any older QEMU releases, so there's no machine type backcompat
> > required, at least from upstream QEMU POV.
> 
> Then how about running QEMU with the parameter like "-M pc-i440fx-2.12"?
> Although the default machine is newer than that.

That doesnt matter.  This back-compat property table is about ensuring
that "-cpu Skylake-Client -M pc-i440fx-2.12" run on QEMU 4.0.0 release
works the same way as when run on QEMU 2.12.0 release.

Since there is no Dhyana CPU model in QEMU 2.12.0 release, there's no
back compat issue to fix.

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: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
Posted by Pu Wen 5 years ago
On 2019/4/16 16:19, Daniel P. Berrangé wrote:
> On Tue, Apr 16, 2019 at 02:56:12PM +0800, Pu Wen wrote:
>> On 2019/4/15 17:25, Daniel P. Berrangé wrote:
>>> On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote:
>>>> @@ -152,6 +154,7 @@ GlobalProperty pc_compat_2_12[] = {
>>>>        { TYPE_X86_CPU, "topoext", "off" },
>>>>        { "EPYC-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
>>>>        { "EPYC-IBPB-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
>>>> +    { "Dhyana-" TYPE_X86_CPU, "xlevel", "0x8000000a" },
>>>>    };
>>>>    const size_t pc_compat_2_12_len = G_N_ELEMENTS(pc_compat_2_12);
>>>
>>> You can drop the changes in this file. This CPU model didn't exist
>>> in any older QEMU releases, so there's no machine type backcompat
>>> required, at least from upstream QEMU POV.
>>
>> Then how about running QEMU with the parameter like "-M pc-i440fx-2.12"?
>> Although the default machine is newer than that.
> 
> That doesnt matter.  This back-compat property table is about ensuring
> that "-cpu Skylake-Client -M pc-i440fx-2.12" run on QEMU 4.0.0 release
> works the same way as when run on QEMU 2.12.0 release.
> 
> Since there is no Dhyana CPU model in QEMU 2.12.0 release, there's no
> back compat issue to fix.

Thanks for the explanation. Will drop the changes in next version patch.

-- 
Regards,
Pu Wen

Re: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
Posted by Eduardo Habkost 5 years ago
On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote:
> Add a new base CPU model called 'Dhyana' to model processors from Hygon
> Dhyana(family 18h), which derived from AMD EPYC(family 17h).
> 
> The following features bits have been removed compare to AMD EPYC:
> aes, pclmulqdq, sha_ni
> 
> The Hygon Dhyana support to KVM in Linux is already accepted upstream[1].
> So add Hygon Dhyana support to Qemu is necessary to create Hygon's own
> CPU model.
> 
> Reference:
> [1] https://git.kernel.org/tip/fec98069fb72fb656304a3e52265e0c2fc9adf87
> 
> Signed-off-by: Pu Wen <puwen@hygon.cn>

Thanks for the patch.

I'm wondering if we should let the CPU model be used only on
Hygon hosts, to avoid confusion.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
Posted by Daniel P. Berrangé 5 years ago
On Mon, Apr 15, 2019 at 05:39:17PM -0300, Eduardo Habkost wrote:
> On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote:
> > Add a new base CPU model called 'Dhyana' to model processors from Hygon
> > Dhyana(family 18h), which derived from AMD EPYC(family 17h).
> > 
> > The following features bits have been removed compare to AMD EPYC:
> > aes, pclmulqdq, sha_ni
> > 
> > The Hygon Dhyana support to KVM in Linux is already accepted upstream[1].
> > So add Hygon Dhyana support to Qemu is necessary to create Hygon's own
> > CPU model.
> > 
> > Reference:
> > [1] https://git.kernel.org/tip/fec98069fb72fb656304a3e52265e0c2fc9adf87
> > 
> > Signed-off-by: Pu Wen <puwen@hygon.cn>
> 
> Thanks for the patch.
> 
> I'm wondering if we should let the CPU model be used only on
> Hygon hosts, to avoid confusion.

Why should we artificially restrict it ?  All the other CPUs are able to
be used on any host that is able to support the feature list required by
the CPU model. If some other host has sufficient features to run Dhyana
the CPU model we shouldn't block it.

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: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
Posted by Eduardo Habkost 5 years ago
On Tue, Apr 16, 2019 at 09:16:05AM +0100, Daniel P. Berrangé wrote:
> On Mon, Apr 15, 2019 at 05:39:17PM -0300, Eduardo Habkost wrote:
> > On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote:
> > > Add a new base CPU model called 'Dhyana' to model processors from Hygon
> > > Dhyana(family 18h), which derived from AMD EPYC(family 17h).
> > > 
> > > The following features bits have been removed compare to AMD EPYC:
> > > aes, pclmulqdq, sha_ni
> > > 
> > > The Hygon Dhyana support to KVM in Linux is already accepted upstream[1].
> > > So add Hygon Dhyana support to Qemu is necessary to create Hygon's own
> > > CPU model.
> > > 
> > > Reference:
> > > [1] https://git.kernel.org/tip/fec98069fb72fb656304a3e52265e0c2fc9adf87
> > > 
> > > Signed-off-by: Pu Wen <puwen@hygon.cn>
> > 
> > Thanks for the patch.
> > 
> > I'm wondering if we should let the CPU model be used only on
> > Hygon hosts, to avoid confusion.
> 
> Why should we artificially restrict it ?  All the other CPUs are able to
> be used on any host that is able to support the feature list required by
> the CPU model. If some other host has sufficient features to run Dhyana
> the CPU model we shouldn't block it.

Running it on Intel or AMD hosts will create a frankenstein CPU
with vendor=AuthenticAMD|GenuineIntel but with
family/model/stepping/model_id values that make sense only on
Hygon CPUs.  I don't see why this is preferable to simply telling
the user that the CPU model is unavailable.

If somebody really needs that specific set of features and know
they are runnable on their AMD host, they can easily run
"-cpu EPYC,+aes,+pclmulqdq,+sha-ni".

We have the same issue with Intel & AMD CPUs.  The only reason we
don't prevent this with AMD or Intel CPU models is our huge fear
of breaking backwards compatibility.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
Posted by Daniel P. Berrangé 5 years ago
On Tue, Apr 16, 2019 at 11:23:13AM -0300, Eduardo Habkost wrote:
> On Tue, Apr 16, 2019 at 09:16:05AM +0100, Daniel P. Berrangé wrote:
> > On Mon, Apr 15, 2019 at 05:39:17PM -0300, Eduardo Habkost wrote:
> > > On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote:
> > > > Add a new base CPU model called 'Dhyana' to model processors from Hygon
> > > > Dhyana(family 18h), which derived from AMD EPYC(family 17h).
> > > > 
> > > > The following features bits have been removed compare to AMD EPYC:
> > > > aes, pclmulqdq, sha_ni
> > > > 
> > > > The Hygon Dhyana support to KVM in Linux is already accepted upstream[1].
> > > > So add Hygon Dhyana support to Qemu is necessary to create Hygon's own
> > > > CPU model.
> > > > 
> > > > Reference:
> > > > [1] https://git.kernel.org/tip/fec98069fb72fb656304a3e52265e0c2fc9adf87
> > > > 
> > > > Signed-off-by: Pu Wen <puwen@hygon.cn>
> > > 
> > > Thanks for the patch.
> > > 
> > > I'm wondering if we should let the CPU model be used only on
> > > Hygon hosts, to avoid confusion.
> > 
> > Why should we artificially restrict it ?  All the other CPUs are able to
> > be used on any host that is able to support the feature list required by
> > the CPU model. If some other host has sufficient features to run Dhyana
> > the CPU model we shouldn't block it.
> 
> Running it on Intel or AMD hosts will create a frankenstein CPU
> with vendor=AuthenticAMD|GenuineIntel but with
> family/model/stepping/model_id values that make sense only on
> Hygon CPUs.  I don't see why this is preferable to simply telling
> the user that the CPU model is unavailable.

IIUC, you're saying that we don't (can't?) honour the "vendor" field
QEMU has listed against the CPU model, so the guest sees the vendor
of the real physical host ?

If so that's news to me, and does indeed make it interesting to
disable the mismatched combination.

> If somebody really needs that specific set of features and know
> they are runnable on their AMD host, they can easily run
> "-cpu EPYC,+aes,+pclmulqdq,+sha-ni".
> 
> We have the same issue with Intel & AMD CPUs.  The only reason we
> don't prevent this with AMD or Intel CPU models is our huge fear
> of breaking backwards compatibility.

We could put a deprecation warning that we intended to stop allowing
this mismatch Intel/AMD guest vs host models and tie it to machine
type.

ie, if we deprecate in 4.1, then in 5.0 we can make pc-i440fx-5.0
machine type refuse to allow this combination.

That way existing deployed guests keep working, and users get some
warning that we're going to stop future guests doing this.

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: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
Posted by Eduardo Habkost 5 years ago
On Tue, Apr 16, 2019 at 03:27:45PM +0100, Daniel P. Berrangé wrote:
> On Tue, Apr 16, 2019 at 11:23:13AM -0300, Eduardo Habkost wrote:
> > On Tue, Apr 16, 2019 at 09:16:05AM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Apr 15, 2019 at 05:39:17PM -0300, Eduardo Habkost wrote:
> > > > On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote:
> > > > > Add a new base CPU model called 'Dhyana' to model processors from Hygon
> > > > > Dhyana(family 18h), which derived from AMD EPYC(family 17h).
> > > > > 
> > > > > The following features bits have been removed compare to AMD EPYC:
> > > > > aes, pclmulqdq, sha_ni
> > > > > 
> > > > > The Hygon Dhyana support to KVM in Linux is already accepted upstream[1].
> > > > > So add Hygon Dhyana support to Qemu is necessary to create Hygon's own
> > > > > CPU model.
> > > > > 
> > > > > Reference:
> > > > > [1] https://git.kernel.org/tip/fec98069fb72fb656304a3e52265e0c2fc9adf87
> > > > > 
> > > > > Signed-off-by: Pu Wen <puwen@hygon.cn>
> > > > 
> > > > Thanks for the patch.
> > > > 
> > > > I'm wondering if we should let the CPU model be used only on
> > > > Hygon hosts, to avoid confusion.
> > > 
> > > Why should we artificially restrict it ?  All the other CPUs are able to
> > > be used on any host that is able to support the feature list required by
> > > the CPU model. If some other host has sufficient features to run Dhyana
> > > the CPU model we shouldn't block it.
> > 
> > Running it on Intel or AMD hosts will create a frankenstein CPU
> > with vendor=AuthenticAMD|GenuineIntel but with
> > family/model/stepping/model_id values that make sense only on
> > Hygon CPUs.  I don't see why this is preferable to simply telling
> > the user that the CPU model is unavailable.
> 
> IIUC, you're saying that we don't (can't?) honour the "vendor" field
> QEMU has listed against the CPU model, so the guest sees the vendor
> of the real physical host ?
> 
> If so that's news to me, and does indeed make it interesting to
> disable the mismatched combination.
> 
> > If somebody really needs that specific set of features and know
> > they are runnable on their AMD host, they can easily run
> > "-cpu EPYC,+aes,+pclmulqdq,+sha-ni".
> > 
> > We have the same issue with Intel & AMD CPUs.  The only reason we
> > don't prevent this with AMD or Intel CPU models is our huge fear
> > of breaking backwards compatibility.
> 
> We could put a deprecation warning that we intended to stop allowing
> this mismatch Intel/AMD guest vs host models and tie it to machine
> type.
> 
> ie, if we deprecate in 4.1, then in 5.0 we can make pc-i440fx-5.0
> machine type refuse to allow this combination.
> 
> That way existing deployed guests keep working, and users get some
> warning that we're going to stop future guests doing this.

Yes, it makes sense.

I would prefer to make pc-4.2 refuse to allow this combination,
but this would require making libvirt versions released before
QEMU 4.2 not translate "pc" to "pc-4.2".

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] i386: Add new Hygon 'Dhyana' CPU model
Posted by Pu Wen 5 years ago
On 2019/4/16 4:40, Eduardo Habkost wrote:
> On Sat, Apr 13, 2019 at 10:54:40AM +0800, Pu Wen wrote:
>> Add a new base CPU model called 'Dhyana' to model processors from Hygon
>> Dhyana(family 18h), which derived from AMD EPYC(family 17h).
>>
>> The following features bits have been removed compare to AMD EPYC:
>> aes, pclmulqdq, sha_ni
>>
>> The Hygon Dhyana support to KVM in Linux is already accepted upstream[1].
>> So add Hygon Dhyana support to Qemu is necessary to create Hygon's own
>> CPU model.
>>
>> Reference:
>> [1] https://git.kernel.org/tip/fec98069fb72fb656304a3e52265e0c2fc9adf87
>>
>> Signed-off-by: Pu Wen <puwen@hygon.cn>
> 
> Thanks for the patch.
> 
> I'm wondering if we should let the CPU model be used only on
> Hygon hosts, to avoid confusion.

Yes, it should be. But how to achieve this?

-- 
Regards,
Pu Wen