| Message ID | 20241028024512.156724-5-tao1.su@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add AVX10.1 CPUID support and GraniteRapids-v2 model | expand |
On 10/28/24 03:45, Tao Su wrote: > Since the highest supported vector length for a processor implies that > all lesser vector lengths are also supported, add the dependencies of > the supported vector lengths. If all vector lengths aren't supported, > clear AVX10 enable bit as well. > > Note that the order of AVX10 related dependencies should be kept as: > CPUID_24_0_EBX_AVX10_128 -> CPUID_24_0_EBX_AVX10_256, > CPUID_24_0_EBX_AVX10_256 -> CPUID_24_0_EBX_AVX10_512, > CPUID_24_0_EBX_AVX10_VL_MASK -> CPUID_7_1_EDX_AVX10, I think you need to add a set of dependencies so that avx10 cannot be set, unless all the older AVX features that it's composed of are available. From the manual these are AVX512F, AVX512CD, AVX512VW, AVX512DQ, AVX512_VBMI, AVX512_IFMA, AVX512_VNNI, AVX512_BF16, AVX512_VPOPCNTDQ, AVX512_VBMI2, VAES, GFNI, VPCLMULQDQ, AVX512_BITALG, AVX512_FP16. Otherwise, the changes look good. Thanks, Paolo
On Mon, Oct 28, 2024 at 09:45:39AM +0100, Paolo Bonzini wrote: > On 10/28/24 03:45, Tao Su wrote: > > Since the highest supported vector length for a processor implies that > > all lesser vector lengths are also supported, add the dependencies of > > the supported vector lengths. If all vector lengths aren't supported, > > clear AVX10 enable bit as well. > > > > Note that the order of AVX10 related dependencies should be kept as: > > CPUID_24_0_EBX_AVX10_128 -> CPUID_24_0_EBX_AVX10_256, > > CPUID_24_0_EBX_AVX10_256 -> CPUID_24_0_EBX_AVX10_512, > > CPUID_24_0_EBX_AVX10_VL_MASK -> CPUID_7_1_EDX_AVX10, > > I think you need to add a set of dependencies so that avx10 cannot be set, > unless all the older AVX features that it's composed of are available. From > the manual these are > > AVX512F, AVX512CD, AVX512VW, AVX512DQ, AVX512_VBMI, AVX512_IFMA, > AVX512_VNNI, AVX512_BF16, AVX512_VPOPCNTDQ, AVX512_VBMI2, VAES, GFNI, > VPCLMULQDQ, AVX512_BITALG, AVX512_FP16. Thanks for such a quick review!! AVX10.1 spec said: Intel AVX-512 will continue to be supported on P-core-only processors for the foreseeable future to support legacy applications. However, new vector ISA features will only be added to the Intel AVX10 ISA moving forward. While Intel AVX10/512 includes all Intel AVX-512 instructions, it important to note that applications compiled to Intel AVX-512 with vector length limited to 256 bits are not guaranteed to be compatible on an Intel AVX10/256 processor. I.e. AVX10/256 processors will support old AVX-512 instructions (limited to 256 bits and enumerated by AVX10) but not set AVX-512 related CPUIDs. So, I think we can't add these dependencies…
On 10/28/24 11:02, Tao Su wrote: > On Mon, Oct 28, 2024 at 09:45:39AM +0100, Paolo Bonzini wrote: >> On 10/28/24 03:45, Tao Su wrote: >>> Since the highest supported vector length for a processor implies that >>> all lesser vector lengths are also supported, add the dependencies of >>> the supported vector lengths. If all vector lengths aren't supported, >>> clear AVX10 enable bit as well. >>> >>> Note that the order of AVX10 related dependencies should be kept as: >>> CPUID_24_0_EBX_AVX10_128 -> CPUID_24_0_EBX_AVX10_256, >>> CPUID_24_0_EBX_AVX10_256 -> CPUID_24_0_EBX_AVX10_512, >>> CPUID_24_0_EBX_AVX10_VL_MASK -> CPUID_7_1_EDX_AVX10, >> >> I think you need to add a set of dependencies so that avx10 cannot be set, >> unless all the older AVX features that it's composed of are available. From >> the manual these are >> >> AVX512F, AVX512CD, AVX512VW, AVX512DQ, AVX512_VBMI, AVX512_IFMA, >> AVX512_VNNI, AVX512_BF16, AVX512_VPOPCNTDQ, AVX512_VBMI2, VAES, GFNI, >> VPCLMULQDQ, AVX512_BITALG, AVX512_FP16. > > Thanks for such a quick review!! > > AVX10.1 spec said: > Intel AVX-512 will continue to be supported on P-core-only processors for > the foreseeable future to support legacy applications. However, new vector > ISA features will only be added to the Intel AVX10 ISA moving forward. > While Intel AVX10/512 includes all Intel AVX-512 instructions, it > important to note that applications compiled to Intel AVX-512 with vector > length limited to 256 bits are not guaranteed to be compatible on an Intel > AVX10/256 processor. > > I.e. AVX10/256 processors will support old AVX-512 instructions > (limited to 256 bits and enumerated by AVX10) but not set AVX-512 related > CPUIDs. So, I think we can't add these dependencies… Of course you're right about AVX10 in general, you still need to add the dependency but only for CPUID_24_0_EBX_AVX10_512. Paolo
On Mon, Oct 28, 2024 at 11:45:25AM +0100, Paolo Bonzini wrote: > On 10/28/24 11:02, Tao Su wrote: > > On Mon, Oct 28, 2024 at 09:45:39AM +0100, Paolo Bonzini wrote: > > > On 10/28/24 03:45, Tao Su wrote: > > > > Since the highest supported vector length for a processor implies that > > > > all lesser vector lengths are also supported, add the dependencies of > > > > the supported vector lengths. If all vector lengths aren't supported, > > > > clear AVX10 enable bit as well. > > > > > > > > Note that the order of AVX10 related dependencies should be kept as: > > > > CPUID_24_0_EBX_AVX10_128 -> CPUID_24_0_EBX_AVX10_256, > > > > CPUID_24_0_EBX_AVX10_256 -> CPUID_24_0_EBX_AVX10_512, > > > > CPUID_24_0_EBX_AVX10_VL_MASK -> CPUID_7_1_EDX_AVX10, > > > > > > I think you need to add a set of dependencies so that avx10 cannot be set, > > > unless all the older AVX features that it's composed of are available. From > > > the manual these are > > > > > > AVX512F, AVX512CD, AVX512VW, AVX512DQ, AVX512_VBMI, AVX512_IFMA, > > > AVX512_VNNI, AVX512_BF16, AVX512_VPOPCNTDQ, AVX512_VBMI2, VAES, GFNI, > > > VPCLMULQDQ, AVX512_BITALG, AVX512_FP16. > > > > Thanks for such a quick review!! > > > > AVX10.1 spec said: > > Intel AVX-512 will continue to be supported on P-core-only processors for > > the foreseeable future to support legacy applications. However, new vector > > ISA features will only be added to the Intel AVX10 ISA moving forward. > > While Intel AVX10/512 includes all Intel AVX-512 instructions, it > > important to note that applications compiled to Intel AVX-512 with vector > > length limited to 256 bits are not guaranteed to be compatible on an Intel > > AVX10/256 processor. > > > > I.e. AVX10/256 processors will support old AVX-512 instructions > > (limited to 256 bits and enumerated by AVX10) but not set AVX-512 related > > CPUIDs. So, I think we can't add these dependencies… > > Of course you're right about AVX10 in general, you still need to add the > dependency but only for CPUID_24_0_EBX_AVX10_512. > I agree, will add in v2, thanks!
On 10/28/2024 6:45 PM, Paolo Bonzini wrote: > On 10/28/24 11:02, Tao Su wrote: >> On Mon, Oct 28, 2024 at 09:45:39AM +0100, Paolo Bonzini wrote: >>> On 10/28/24 03:45, Tao Su wrote: >>>> Since the highest supported vector length for a processor implies that >>>> all lesser vector lengths are also supported, add the dependencies of >>>> the supported vector lengths. If all vector lengths aren't supported, >>>> clear AVX10 enable bit as well. >>>> >>>> Note that the order of AVX10 related dependencies should be kept as: >>>> CPUID_24_0_EBX_AVX10_128 -> CPUID_24_0_EBX_AVX10_256, >>>> CPUID_24_0_EBX_AVX10_256 -> CPUID_24_0_EBX_AVX10_512, >>>> CPUID_24_0_EBX_AVX10_VL_MASK -> CPUID_7_1_EDX_AVX10, >>> >>> I think you need to add a set of dependencies so that avx10 cannot be >>> set, >>> unless all the older AVX features that it's composed of are >>> available. From >>> the manual these are >>> >>> AVX512F, AVX512CD, AVX512VW, AVX512DQ, AVX512_VBMI, AVX512_IFMA, >>> AVX512_VNNI, AVX512_BF16, AVX512_VPOPCNTDQ, AVX512_VBMI2, VAES, GFNI, >>> VPCLMULQDQ, AVX512_BITALG, AVX512_FP16. >> >> Thanks for such a quick review!! >> >> AVX10.1 spec said: >> Intel AVX-512 will continue to be supported on P-core-only processors for >> the foreseeable future to support legacy applications. However, new >> vector >> ISA features will only be added to the Intel AVX10 ISA moving forward. >> While Intel AVX10/512 includes all Intel AVX-512 instructions, it >> important to note that applications compiled to Intel AVX-512 with vector >> length limited to 256 bits are not guaranteed to be compatible on an >> Intel >> AVX10/256 processor. >> >> I.e. AVX10/256 processors will support old AVX-512 instructions >> (limited to 256 bits and enumerated by AVX10) but not set AVX-512 related >> CPUIDs. So, I think we can't add these dependencies… > > Of course you're right about AVX10 in general, you still need to add the > dependency but only for CPUID_24_0_EBX_AVX10_512. What if future E-core processor starts to support AVX10/512 but not enumerating any individual AVX512 bit? (AVX10.1 spec only states the compatibility behavior for P-core-only processors) > Paolo >
On Mon, Oct 28, 2024 at 3:48 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote: > > On 10/28/2024 6:45 PM, Paolo Bonzini wrote: > > On 10/28/24 11:02, Tao Su wrote: > >> On Mon, Oct 28, 2024 at 09:45:39AM +0100, Paolo Bonzini wrote: > >>> On 10/28/24 03:45, Tao Su wrote: > >>>> Since the highest supported vector length for a processor implies that > >>>> all lesser vector lengths are also supported, add the dependencies of > >>>> the supported vector lengths. If all vector lengths aren't supported, > >>>> clear AVX10 enable bit as well. > >>>> > >>>> Note that the order of AVX10 related dependencies should be kept as: > >>>> CPUID_24_0_EBX_AVX10_128 -> CPUID_24_0_EBX_AVX10_256, > >>>> CPUID_24_0_EBX_AVX10_256 -> CPUID_24_0_EBX_AVX10_512, > >>>> CPUID_24_0_EBX_AVX10_VL_MASK -> CPUID_7_1_EDX_AVX10, > >>> > >>> I think you need to add a set of dependencies so that avx10 cannot be > >>> set, > >>> unless all the older AVX features that it's composed of are > >>> available. From > >>> the manual these are > >>> > >>> AVX512F, AVX512CD, AVX512VW, AVX512DQ, AVX512_VBMI, AVX512_IFMA, > >>> AVX512_VNNI, AVX512_BF16, AVX512_VPOPCNTDQ, AVX512_VBMI2, VAES, GFNI, > >>> VPCLMULQDQ, AVX512_BITALG, AVX512_FP16. > >> > >> Thanks for such a quick review!! > >> > >> AVX10.1 spec said: > >> Intel AVX-512 will continue to be supported on P-core-only processors for > >> the foreseeable future to support legacy applications. However, new > >> vector > >> ISA features will only be added to the Intel AVX10 ISA moving forward. > >> While Intel AVX10/512 includes all Intel AVX-512 instructions, it > >> important to note that applications compiled to Intel AVX-512 with vector > >> length limited to 256 bits are not guaranteed to be compatible on an > >> Intel > >> AVX10/256 processor. > >> > >> I.e. AVX10/256 processors will support old AVX-512 instructions > >> (limited to 256 bits and enumerated by AVX10) but not set AVX-512 related > >> CPUIDs. So, I think we can't add these dependencies… > > > > Of course you're right about AVX10 in general, you still need to add the > > dependency but only for CPUID_24_0_EBX_AVX10_512. > > What if future E-core processor starts to support AVX10/512 but not > enumerating any individual AVX512 bit? (AVX10.1 spec only states the > compatibility behavior for P-core-only processors) KVM and QEMU could always specify the bits. If you want to ask around if this is possible then go ahead. In the meanwhile I actually can apply Tao Su's patches, since the dependencies are merely a safety feature. Paolo
On 10/28/2024 10:50 PM, Paolo Bonzini wrote: > On Mon, Oct 28, 2024 at 3:48 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote: >> >> On 10/28/2024 6:45 PM, Paolo Bonzini wrote: >>> On 10/28/24 11:02, Tao Su wrote: >>>> On Mon, Oct 28, 2024 at 09:45:39AM +0100, Paolo Bonzini wrote: >>>>> On 10/28/24 03:45, Tao Su wrote: >>>>>> Since the highest supported vector length for a processor implies that >>>>>> all lesser vector lengths are also supported, add the dependencies of >>>>>> the supported vector lengths. If all vector lengths aren't supported, >>>>>> clear AVX10 enable bit as well. >>>>>> >>>>>> Note that the order of AVX10 related dependencies should be kept as: >>>>>> CPUID_24_0_EBX_AVX10_128 -> CPUID_24_0_EBX_AVX10_256, >>>>>> CPUID_24_0_EBX_AVX10_256 -> CPUID_24_0_EBX_AVX10_512, >>>>>> CPUID_24_0_EBX_AVX10_VL_MASK -> CPUID_7_1_EDX_AVX10, >>>>> >>>>> I think you need to add a set of dependencies so that avx10 cannot be >>>>> set, >>>>> unless all the older AVX features that it's composed of are >>>>> available. From >>>>> the manual these are >>>>> >>>>> AVX512F, AVX512CD, AVX512VW, AVX512DQ, AVX512_VBMI, AVX512_IFMA, >>>>> AVX512_VNNI, AVX512_BF16, AVX512_VPOPCNTDQ, AVX512_VBMI2, VAES, GFNI, >>>>> VPCLMULQDQ, AVX512_BITALG, AVX512_FP16. >>>> >>>> Thanks for such a quick review!! >>>> >>>> AVX10.1 spec said: >>>> Intel AVX-512 will continue to be supported on P-core-only processors for >>>> the foreseeable future to support legacy applications. However, new >>>> vector >>>> ISA features will only be added to the Intel AVX10 ISA moving forward. >>>> While Intel AVX10/512 includes all Intel AVX-512 instructions, it >>>> important to note that applications compiled to Intel AVX-512 with vector >>>> length limited to 256 bits are not guaranteed to be compatible on an >>>> Intel >>>> AVX10/256 processor. >>>> >>>> I.e. AVX10/256 processors will support old AVX-512 instructions >>>> (limited to 256 bits and enumerated by AVX10) but not set AVX-512 related >>>> CPUIDs. So, I think we can't add these dependencies… >>> >>> Of course you're right about AVX10 in general, you still need to add the >>> dependency but only for CPUID_24_0_EBX_AVX10_512. >> >> What if future E-core processor starts to support AVX10/512 but not >> enumerating any individual AVX512 bit? (AVX10.1 spec only states the >> compatibility behavior for P-core-only processors) > > KVM and QEMU could always specify the bits. That will need KVM to report individual AVX512 bits in KVM_GET_SUPPORTED_CPUID to not break QEMU. > If you want to ask around > if this is possible then go ahead. We will ask internal architects and get back with the answer. > In the meanwhile I actually can apply Tao Su's patches, since the > dependencies are merely a safety feature. Sound reasonable. > Paolo >
On Tue, Oct 29, 2024 at 10:47:04PM +0800, Zhao Liu wrote: > Hi Tao, > > On Mon, Oct 28, 2024 at 10:45:10AM +0800, Tao Su wrote: > > Date: Mon, 28 Oct 2024 10:45:10 +0800 > > From: Tao Su <tao1.su@linux.intel.com> > > Subject: [PATCH 4/6] target/i386: Add feature dependencies for AVX10 > > X-Mailer: git-send-email 2.34.1 > > > > Since the highest supported vector length for a processor implies that > > all lesser vector lengths are also supported, add the dependencies of > > the supported vector lengths. If all vector lengths aren't supported, > > clear AVX10 enable bit as well. > > > > Note that the order of AVX10 related dependencies should be kept as: > > CPUID_24_0_EBX_AVX10_128 -> CPUID_24_0_EBX_AVX10_256, > > CPUID_24_0_EBX_AVX10_256 -> CPUID_24_0_EBX_AVX10_512, > > CPUID_24_0_EBX_AVX10_VL_MASK -> CPUID_7_1_EDX_AVX10, > > CPUID_7_1_EDX_AVX10 -> CPUID_24_0_EBX, > > so that prevent user from setting weird CPUID combinations, e.g. 256-bits > > and 512-bits are supported but 128-bits is not, no vector lengths are > > supported but AVX10 enable bit is still set. > > > > Since AVX10_128 will be reserved as 1, > > Does this means AVX10_128 bit is reserved and it is always 1? > > From the spec you linked in cover letter (Table 1-1. CPUID Enumeration > of Intel® AVX10), it seems AVX10_128 bit is marked as reserved. > > It's worth describing its behavior. Yes, AVX10_128 will be reserved as 1. SDM(Volume 1, Ch16) and AVX10.2 have already described this. I believe AVX10.1 spec (linked in cover letter) will change the description.
Hi Tao, On Mon, Oct 28, 2024 at 10:45:10AM +0800, Tao Su wrote: > Date: Mon, 28 Oct 2024 10:45:10 +0800 > From: Tao Su <tao1.su@linux.intel.com> > Subject: [PATCH 4/6] target/i386: Add feature dependencies for AVX10 > X-Mailer: git-send-email 2.34.1 > > Since the highest supported vector length for a processor implies that > all lesser vector lengths are also supported, add the dependencies of > the supported vector lengths. If all vector lengths aren't supported, > clear AVX10 enable bit as well. > > Note that the order of AVX10 related dependencies should be kept as: > CPUID_24_0_EBX_AVX10_128 -> CPUID_24_0_EBX_AVX10_256, > CPUID_24_0_EBX_AVX10_256 -> CPUID_24_0_EBX_AVX10_512, > CPUID_24_0_EBX_AVX10_VL_MASK -> CPUID_7_1_EDX_AVX10, > CPUID_7_1_EDX_AVX10 -> CPUID_24_0_EBX, > so that prevent user from setting weird CPUID combinations, e.g. 256-bits > and 512-bits are supported but 128-bits is not, no vector lengths are > supported but AVX10 enable bit is still set. > > Since AVX10_128 will be reserved as 1, Does this means AVX10_128 bit is reserved and it is always 1? From the spec you linked in cover letter (Table 1-1. CPUID Enumeration of Intel® AVX10), it seems AVX10_128 bit is marked as reserved. It's worth describing its behavior. > adding these dependencies has the > bonus that when user sets -cpu host,-avx10-128, CPUID_7_1_EDX_AVX10 and > CPUID_24_0_EBX will be disabled automatically. > > Tested-by: Xuelian Guo <xuelian.guo@intel.com> > Signed-off-by: Tao Su <tao1.su@linux.intel.com> > --- > target/i386/cpu.c | 16 ++++++++++++++++ > target/i386/cpu.h | 4 ++++ > 2 files changed, 20 insertions(+) Otherwise, Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 91fae0dcb7..9666dbf29c 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -1760,6 +1760,22 @@ static FeatureDep feature_dependencies[] = { .from = { FEAT_7_0_EBX, CPUID_7_0_EBX_SGX }, .to = { FEAT_SGX_12_1_EAX, ~0ull }, }, + { + .from = { FEAT_24_0_EBX, CPUID_24_0_EBX_AVX10_128 }, + .to = { FEAT_24_0_EBX, CPUID_24_0_EBX_AVX10_256 }, + }, + { + .from = { FEAT_24_0_EBX, CPUID_24_0_EBX_AVX10_256 }, + .to = { FEAT_24_0_EBX, CPUID_24_0_EBX_AVX10_512 }, + }, + { + .from = { FEAT_24_0_EBX, CPUID_24_0_EBX_AVX10_VL_MASK }, + .to = { FEAT_7_1_EDX, CPUID_7_1_EDX_AVX10 }, + }, + { + .from = { FEAT_7_1_EDX, CPUID_7_1_EDX_AVX10 }, + .to = { FEAT_24_0_EBX, ~0ull }, + }, }; typedef struct X86RegisterInfo32 { diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 5566a13f4f..e4c947b478 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -997,6 +997,10 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w); #define CPUID_24_0_EBX_AVX10_256 (1U << 17) /* AVX10 512-bit vector support is present */ #define CPUID_24_0_EBX_AVX10_512 (1U << 18) +/* AVX10 vector length support mask */ +#define CPUID_24_0_EBX_AVX10_VL_MASK (CPUID_24_0_EBX_AVX10_128 | \ + CPUID_24_0_EBX_AVX10_256 | \ + CPUID_24_0_EBX_AVX10_512) /* RAS Features */ #define CPUID_8000_0007_EBX_OVERFLOW_RECOV (1U << 0)
Since the highest supported vector length for a processor implies that all lesser vector lengths are also supported, add the dependencies of the supported vector lengths. If all vector lengths aren't supported, clear AVX10 enable bit as well. Note that the order of AVX10 related dependencies should be kept as: CPUID_24_0_EBX_AVX10_128 -> CPUID_24_0_EBX_AVX10_256, CPUID_24_0_EBX_AVX10_256 -> CPUID_24_0_EBX_AVX10_512, CPUID_24_0_EBX_AVX10_VL_MASK -> CPUID_7_1_EDX_AVX10, CPUID_7_1_EDX_AVX10 -> CPUID_24_0_EBX, so that prevent user from setting weird CPUID combinations, e.g. 256-bits and 512-bits are supported but 128-bits is not, no vector lengths are supported but AVX10 enable bit is still set. Since AVX10_128 will be reserved as 1, adding these dependencies has the bonus that when user sets -cpu host,-avx10-128, CPUID_7_1_EDX_AVX10 and CPUID_24_0_EBX will be disabled automatically. Tested-by: Xuelian Guo <xuelian.guo@intel.com> Signed-off-by: Tao Su <tao1.su@linux.intel.com> --- target/i386/cpu.c | 16 ++++++++++++++++ target/i386/cpu.h | 4 ++++ 2 files changed, 20 insertions(+)