| Message ID | 20221227183713.29140-2-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA | expand |
On Tue, Dec 27, 2022 at 10:38 AM Aaron Lewis <aaronlewis@google.com> wrote: > > Be good citizens by clearing both > CPUID.(EAX=0DH,ECX=0):EAX.XTILECFG[bit-17] and > CPUID.(EAX=0DH,ECX=0):EAX.XTILEDATA[bit-18] if they are both not set. > That way userspace or a guest doesn't fail if it attempts to set XCR0 > with the user xfeature bits, i.e. EDX:EAX of CPUID.(EAX=0DH,ECX=0). > > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > --- > arch/x86/kvm/cpuid.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 0b5bf013fcb8e..2d9910847786a 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -977,6 +977,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) > u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm(); > u64 permitted_xss = kvm_caps.supported_xss; > > + if (!(permitted_xcr0 & XFEATURE_MASK_XTILE_CFG) || > + !(permitted_xcr0 & XFEATURE_MASK_XTILE_DATA)) > + permitted_xcr0 &= ~XFEATURE_MASK_XTILE; > + > entry->eax &= permitted_xcr0; > entry->ebx = xstate_required_size(permitted_xcr0, false); > entry->ecx = entry->ebx; > -- > 2.39.0.314.g84b9a713c41-goog > Two questions: 1) Under what circumstances would this happen? 2) Shouldn't we also clear XFEATURE_MASK_CFG if both bits are not set?
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > index 0b5bf013fcb8e..2d9910847786a 100644 > > --- a/arch/x86/kvm/cpuid.c > > +++ b/arch/x86/kvm/cpuid.c > > @@ -977,6 +977,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) > > u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm(); > > u64 permitted_xss = kvm_caps.supported_xss; > > > > + if (!(permitted_xcr0 & XFEATURE_MASK_XTILE_CFG) || > > + !(permitted_xcr0 & XFEATURE_MASK_XTILE_DATA)) > > + permitted_xcr0 &= ~XFEATURE_MASK_XTILE; > > + > > entry->eax &= permitted_xcr0; > > entry->ebx = xstate_required_size(permitted_xcr0, false); > > entry->ecx = entry->ebx; > > -- > > 2.39.0.314.g84b9a713c41-goog > > > > Two questions: > > 1) Under what circumstances would this happen? This would happen if userspace hasn't opted in to using AMX via arch_prctl(). > 2) Shouldn't we also clear XFEATURE_MASK_CFG if both bits are not set? Both CFG and DATA are cleared with XFEATURE_MASK_XTILE. It defines both.
On Tue, Dec 27, 2022 at 12:44 PM Aaron Lewis <aaronlewis@google.com> wrote: > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > > index 0b5bf013fcb8e..2d9910847786a 100644 > > > --- a/arch/x86/kvm/cpuid.c > > > +++ b/arch/x86/kvm/cpuid.c > > > @@ -977,6 +977,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) > > > u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm(); > > > u64 permitted_xss = kvm_caps.supported_xss; > > > > > > + if (!(permitted_xcr0 & XFEATURE_MASK_XTILE_CFG) || > > > + !(permitted_xcr0 & XFEATURE_MASK_XTILE_DATA)) > > > + permitted_xcr0 &= ~XFEATURE_MASK_XTILE; > > > + > > > entry->eax &= permitted_xcr0; > > > entry->ebx = xstate_required_size(permitted_xcr0, false); > > > entry->ecx = entry->ebx; > > > -- > > > 2.39.0.314.g84b9a713c41-goog > > > > > > > Two questions: > > > > 1) Under what circumstances would this happen? > This would happen if userspace hasn't opted in to using AMX via arch_prctl(). > > > 2) Shouldn't we also clear XFEATURE_MASK_CFG if both bits are not set? > Both CFG and DATA are cleared with XFEATURE_MASK_XTILE. It defines both. Doh! Reviewed-by: Jim Mattson <jmattson@google.com>
On Tue, Dec 27, 2022 at 1:14 PM Jim Mattson <jmattson@google.com> wrote:
> Reviewed-by: Jim Mattson <jmattson@google.com>
Could you change the shortlog to clarify that both feature bits are
cleared if either one is clear?
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 0b5bf013fcb8e..2d9910847786a 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -977,6 +977,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm(); u64 permitted_xss = kvm_caps.supported_xss; + if (!(permitted_xcr0 & XFEATURE_MASK_XTILE_CFG) || + !(permitted_xcr0 & XFEATURE_MASK_XTILE_DATA)) + permitted_xcr0 &= ~XFEATURE_MASK_XTILE; + entry->eax &= permitted_xcr0; entry->ebx = xstate_required_size(permitted_xcr0, false); entry->ecx = entry->ebx;
Be good citizens by clearing both CPUID.(EAX=0DH,ECX=0):EAX.XTILECFG[bit-17] and CPUID.(EAX=0DH,ECX=0):EAX.XTILEDATA[bit-18] if they are both not set. That way userspace or a guest doesn't fail if it attempts to set XCR0 with the user xfeature bits, i.e. EDX:EAX of CPUID.(EAX=0DH,ECX=0). Suggested-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Aaron Lewis <aaronlewis@google.com> --- arch/x86/kvm/cpuid.c | 4 ++++ 1 file changed, 4 insertions(+)