| Message ID | 1477645960-6898-1-git-send-email-he.chen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/10/2016 11:12, He Chen wrote: > The spec can be found in Intel Software Developer Manual or in > Instruction Set Extensions Programming Reference. > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > Signed-off-by: He Chen <he.chen@linux.intel.com> > --- > arch/x86/kvm/cpuid.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index afa7bbb..328b169 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -376,6 +376,10 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > /* cpuid 7.0.ecx*/ > const u32 kvm_cpuid_7_0_ecx_x86_features = F(PKU) | 0 /*OSPKE*/; > > + /* cpuid 7.0.edx*/ > + const u32 kvm_cpuid_7_0_edx_x86_features = > + 0x4 /* AVX512-4VNNIW */ | 0x8 /* AVX512-4FMAPS */; Please define the new features in cpufeature.h first. Thanks, Paolo > /* all calls to cpuid_count() should be made on the same cpu */ > get_cpu(); > > @@ -458,12 +462,13 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > /* PKU is not yet implemented for shadow paging. */ > if (!tdp_enabled) > entry->ecx &= ~F(PKU); > + entry->edx &= kvm_cpuid_7_0_edx_x86_features; > } else { > entry->ebx = 0; > entry->ecx = 0; > + entry->edx = 0; > } > entry->eax = 0; > - entry->edx = 0; > break; > } > case 9: > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 28, 2016 at 11:31:05AM +0200, Paolo Bonzini wrote: > > > On 28/10/2016 11:12, He Chen wrote: > > The spec can be found in Intel Software Developer Manual or in > > Instruction Set Extensions Programming Reference. > > > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > > Signed-off-by: He Chen <he.chen@linux.intel.com> > > --- > > arch/x86/kvm/cpuid.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > index afa7bbb..328b169 100644 > > --- a/arch/x86/kvm/cpuid.c > > +++ b/arch/x86/kvm/cpuid.c > > @@ -376,6 +376,10 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > > /* cpuid 7.0.ecx*/ > > const u32 kvm_cpuid_7_0_ecx_x86_features = F(PKU) | 0 /*OSPKE*/; > > > > + /* cpuid 7.0.edx*/ > > + const u32 kvm_cpuid_7_0_edx_x86_features = > > + 0x4 /* AVX512-4VNNIW */ | 0x8 /* AVX512-4FMAPS */; > > Please define the new features in cpufeature.h first. > These 2 new features defined as scattered features in kernel. In cpufeature.h, there are: #define X86_FEATURE_AVX512_4VNNIW (7*32+16) #define X86_FEATURE_AVX512_4FMAPS (7*32+17) Please see disscusion here: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1250183.html Thanks, -He -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/10/2016 11:46, He Chen wrote: > On Fri, Oct 28, 2016 at 11:31:05AM +0200, Paolo Bonzini wrote: >> >> >> On 28/10/2016 11:12, He Chen wrote: >>> The spec can be found in Intel Software Developer Manual or in >>> Instruction Set Extensions Programming Reference. >>> >>> Signed-off-by: Luwei Kang <luwei.kang@intel.com> >>> Signed-off-by: He Chen <he.chen@linux.intel.com> >>> --- >>> arch/x86/kvm/cpuid.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >>> index afa7bbb..328b169 100644 >>> --- a/arch/x86/kvm/cpuid.c >>> +++ b/arch/x86/kvm/cpuid.c >>> @@ -376,6 +376,10 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, >>> /* cpuid 7.0.ecx*/ >>> const u32 kvm_cpuid_7_0_ecx_x86_features = F(PKU) | 0 /*OSPKE*/; >>> >>> + /* cpuid 7.0.edx*/ >>> + const u32 kvm_cpuid_7_0_edx_x86_features = >>> + 0x4 /* AVX512-4VNNIW */ | 0x8 /* AVX512-4FMAPS */; >> >> Please define the new features in cpufeature.h first. >> > These 2 new features defined as scattered features in kernel. > In cpufeature.h, there are: > #define X86_FEATURE_AVX512_4VNNIW (7*32+16) > #define X86_FEATURE_AVX512_4FMAPS (7*32+17) > > Please see disscusion here: > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1250183.html Uff, that sucks. :( I'd agree with hpa's position in that thread. Please do something like /* These are scattered features in cpufeature.h. */ #define KVM_CPUID_BIT_AVX512_4VNNIW 2 #define KVM_CPUID_BIT_AVX512_4FMAPS 3 #define KF(x) bit(KVM_CPUID_BIT_##x) and then const u32 kvm_cpuid_7_0_edx_x86_features = KF(AVX512_4VNNIW) | KF(AVX512_4FMAPS) I'll think of a trick to avoid using F for scattered features... Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 28, 2016 at 11:54:13AM +0200, Paolo Bonzini wrote: > > > On 28/10/2016 11:46, He Chen wrote: > > On Fri, Oct 28, 2016 at 11:31:05AM +0200, Paolo Bonzini wrote: > >> > >> > >> On 28/10/2016 11:12, He Chen wrote: > >>> The spec can be found in Intel Software Developer Manual or in > >>> Instruction Set Extensions Programming Reference. > >>> > >>> Signed-off-by: Luwei Kang <luwei.kang@intel.com> > >>> Signed-off-by: He Chen <he.chen@linux.intel.com> > >>> --- > >>> arch/x86/kvm/cpuid.c | 7 ++++++- > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > >>> index afa7bbb..328b169 100644 > >>> --- a/arch/x86/kvm/cpuid.c > >>> +++ b/arch/x86/kvm/cpuid.c > >>> @@ -376,6 +376,10 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > >>> /* cpuid 7.0.ecx*/ > >>> const u32 kvm_cpuid_7_0_ecx_x86_features = F(PKU) | 0 /*OSPKE*/; > >>> > >>> + /* cpuid 7.0.edx*/ > >>> + const u32 kvm_cpuid_7_0_edx_x86_features = > >>> + 0x4 /* AVX512-4VNNIW */ | 0x8 /* AVX512-4FMAPS */; > >> > >> Please define the new features in cpufeature.h first. > >> > > These 2 new features defined as scattered features in kernel. > > In cpufeature.h, there are: > > #define X86_FEATURE_AVX512_4VNNIW (7*32+16) > > #define X86_FEATURE_AVX512_4FMAPS (7*32+17) > > > > Please see disscusion here: > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1250183.html > > Uff, that sucks. :( I'd agree with hpa's position in that thread. > > Please do something like > > /* These are scattered features in cpufeature.h. */ > #define KVM_CPUID_BIT_AVX512_4VNNIW 2 > #define KVM_CPUID_BIT_AVX512_4FMAPS 3 > #define KF(x) bit(KVM_CPUID_BIT_##x) > > and then > > const u32 kvm_cpuid_7_0_edx_x86_features = > KF(AVX512_4VNNIW) | KF(AVX512_4FMAPS) > > I'll think of a trick to avoid using F for scattered features... > Appreciate it :-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gRnJpLCAyMDE2LTEwLTI4IGF0IDE3OjEyICswODAwLCBIZSBDaGVuIHdyb3RlOg0KPiBUaGUg c3BlYyBjYW4gYmUgZm91bmQgaW4gSW50ZWwgU29mdHdhcmUgRGV2ZWxvcGVyIE1hbnVhbCBvciBp bg0KPiBJbnN0cnVjdGlvbiBTZXQgRXh0ZW5zaW9ucyBQcm9ncmFtbWluZyBSZWZlcmVuY2UuDQo+ IA0KPiBTaWduZWQtb2ZmLWJ5OiBMdXdlaSBLYW5nIDxsdXdlaS5rYW5nQGludGVsLmNvbT4NCj4g U2lnbmVkLW9mZi1ieTogSGUgQ2hlbiA8aGUuY2hlbkBsaW51eC5pbnRlbC5jb20+DQo+IC0tLQ0K PiDCoGFyY2gveDg2L2t2bS9jcHVpZC5jIHwgNyArKysrKystDQo+IMKgMSBmaWxlIGNoYW5nZWQs IDYgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQ0KPiANCj4gZGlmZiAtLWdpdCBhL2FyY2gv eDg2L2t2bS9jcHVpZC5jIGIvYXJjaC94ODYva3ZtL2NwdWlkLmMNCj4gaW5kZXggYWZhN2JiYi4u MzI4YjE2OSAxMDA2NDQNCj4gLS0tIGEvYXJjaC94ODYva3ZtL2NwdWlkLmMNCj4gKysrIGIvYXJj aC94ODYva3ZtL2NwdWlkLmMNCj4gQEAgLTM3Niw2ICszNzYsMTAgQEAgc3RhdGljIGlubGluZSBp bnQgX19kb19jcHVpZF9lbnQoc3RydWN0DQo+IGt2bV9jcHVpZF9lbnRyeTIgKmVudHJ5LCB1MzIg ZnVuY3Rpb24sDQo+IMKgCS8qIGNwdWlkIDcuMC5lY3gqLw0KPiDCoAljb25zdCB1MzIga3ZtX2Nw dWlkXzdfMF9lY3hfeDg2X2ZlYXR1cmVzID0gRihQS1UpIHwgMA0KPiAvKk9TUEtFKi87DQo+IMKg DQo+ICsJLyogY3B1aWQgNy4wLmVkeCovDQo+ICsJY29uc3QgdTMyIGt2bV9jcHVpZF83XzBfZWR4 X3g4Nl9mZWF0dXJlcyA9DQo+ICvCoMKgwqDCoMKgwqDCoMKgMHg0IC8qIEFWWDUxMi00Vk5OSVcg Ki8gfCAweDggLyogQVZYNTEyLTRGTUFQUyAqLzsNCj4gKw0KPiDCoAkvKiBhbGwgY2FsbHMgdG8g Y3B1aWRfY291bnQoKSBzaG91bGQgYmUgbWFkZSBvbiB0aGUgc2FtZSBjcHUNCj4gKi8NCj4gwqAJ Z2V0X2NwdSgpOw0KPiDCoA0KPiBAQCAtNDU4LDEyICs0NjIsMTMgQEAgc3RhdGljIGlubGluZSBp bnQgX19kb19jcHVpZF9lbnQoc3RydWN0DQo+IGt2bV9jcHVpZF9lbnRyeTIgKmVudHJ5LCB1MzIg ZnVuY3Rpb24sDQo+IMKgCQkJLyogUEtVIGlzIG5vdCB5ZXQgaW1wbGVtZW50ZWQgZm9yIHNoYWRv dw0KPiBwYWdpbmcuICovDQo+IMKgCQkJaWYgKCF0ZHBfZW5hYmxlZCkNCj4gwqAJCQkJZW50cnkt PmVjeCAmPSB+RihQS1UpOw0KPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgZW50cnktPmVkeCAm PSBrdm1fY3B1aWRfN18wX2VkeF94ODZfZmVhdHVyZXM7DQoNClRoZSBjcHVfbWFzaygpIGlzIG1p c3NlZCBoZXJlLg0KSSB1bmRlcnN0YW5kIHRoYXQgaXQgZG9lc24ndCB3b3JrIGZvciB0aGlzIHNj YXR0ZXJlZCBmZWF0dXJlcyBidXQgdGhlDQpiaXRzIGluIGVkeCBtdXN0IGJlIHplcm9lZCBpZiBj b3JyZXNwb25kaW5nIGZsYWdzIHdlcmUgY2xlYXJlZCBpbg0KZnB1X194c3RhdGVfY2xlYXJfYWxs X2NwdV9jYXBzLg0KU28gdGhpcyBpbXBsaWVzIG1vcmUgd29yayB1bmZvcnR1bmF0ZWx5Lg0KDQo+ IMKgCQl9IGVsc2Ugew0KPiDCoAkJCWVudHJ5LT5lYnggPSAwOw0KPiDCoAkJCWVudHJ5LT5lY3gg PSAwOw0KPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgZW50cnktPmVkeCA9IDA7DQo+IMKgCQl9 DQo+IMKgCQllbnRyeS0+ZWF4ID0gMDsNCj4gLQkJZW50cnktPmVkeCA9IDA7DQo+IMKgCQlicmVh azsNCj4gwqAJfQ0KPiDCoAljYXNlIDk6 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/10/2016 12:13, Luc, Piotr wrote: > On Fri, 2016-10-28 at 17:12 +0800, He Chen wrote: >> The spec can be found in Intel Software Developer Manual or in >> Instruction Set Extensions Programming Reference. >> >> Signed-off-by: Luwei Kang <luwei.kang@intel.com> >> Signed-off-by: He Chen <he.chen@linux.intel.com> >> --- >> arch/x86/kvm/cpuid.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >> index afa7bbb..328b169 100644 >> --- a/arch/x86/kvm/cpuid.c >> +++ b/arch/x86/kvm/cpuid.c >> @@ -376,6 +376,10 @@ static inline int __do_cpuid_ent(struct >> kvm_cpuid_entry2 *entry, u32 function, >> /* cpuid 7.0.ecx*/ >> const u32 kvm_cpuid_7_0_ecx_x86_features = F(PKU) | 0 >> /*OSPKE*/; >> >> + /* cpuid 7.0.edx*/ >> + const u32 kvm_cpuid_7_0_edx_x86_features = >> + 0x4 /* AVX512-4VNNIW */ | 0x8 /* AVX512-4FMAPS */; >> + >> /* all calls to cpuid_count() should be made on the same cpu >> */ >> get_cpu(); >> >> @@ -458,12 +462,13 @@ static inline int __do_cpuid_ent(struct >> kvm_cpuid_entry2 *entry, u32 function, >> /* PKU is not yet implemented for shadow >> paging. */ >> if (!tdp_enabled) >> entry->ecx &= ~F(PKU); >> + entry->edx &= kvm_cpuid_7_0_edx_x86_features; > > The cpu_mask() is missed here. > I understand that it doesn't work for this scattered features but the > bits in edx must be zeroed if corresponding flags were cleared in > fpu__xstate_clear_all_cpu_caps. > So this implies more work unfortunately. So if the x86 folks would retract their objection and accept a new cpufeature array element it would be nice, because KVM could just do cpuid_mask(&entry->edx, CPUID_7_0_EDX); Otherwise, if you add a cpuid_count_edx function to processor.h then one can do: entry_>edx &= cpuid_count_edx(7, 0); which is decent too. Thanks, Paolo >> } else { >> entry->ebx = 0; >> entry->ecx = 0; >> + entry->edx = 0; >> } >> entry->eax = 0; >> - entry->edx = 0; >> break; >> } >> case 9: -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 28, 2016 at 12:17:02PM +0200, Paolo Bonzini wrote: > Otherwise, if you add a cpuid_count_edx function to processor.h then one > can do: > > entry_>edx &= cpuid_count_edx(7, 0); > > which is decent too. If you think of iterating over the cpuid_bits[] array and recreating the CPUID leaf for KVM, sure, why not...
On 28/10/2016 13:08, Borislav Petkov wrote: > On Fri, Oct 28, 2016 at 12:17:02PM +0200, Paolo Bonzini wrote: >> Otherwise, if you add a cpuid_count_edx function to processor.h then one >> can do: >> >> entry_>edx &= cpuid_count_edx(7, 0); >> >> which is decent too. > > If you think of iterating over the cpuid_bits[] array and recreating the > CPUID leaf for KVM, sure, why not... > cpuid_count_edx would be just static inline unsigned int cpuid_count_edx(unsigned op, unsigned count) { unsigned int eax, ebx, ecx, edx; cpuid_count(op, count, &eax, &ebx, &ecx, &edx); return edx; } Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 28, 2016 at 02:07:21PM +0200, Paolo Bonzini wrote: > cpuid_count_edx would be just > > static inline unsigned int cpuid_count_edx(unsigned op, unsigned count) > { > unsigned int eax, ebx, ecx, edx; > > cpuid_count(op, count, &eax, &ebx, &ecx, &edx); > > return edx; > } Even better. But shouldn't this be hiding unimplemented CPUID bits from the guest?
----- Original Message ----- > From: "Borislav Petkov" <bp@alien8.de> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: "Piotr Luc" <Piotr.Luc@intel.com>, kvm@vger.kernel.org, "he chen" <he.chen@linux.intel.com>, > linux-kernel@vger.kernel.org, tglx@linutronix.de, x86@kernel.org, hpa@zytor.com, mingo@redhat.com, "Luwei Kang" > <luwei.kang@intel.com>, rkrcmar@redhat.com > Sent: Friday, October 28, 2016 2:21:23 PM > Subject: Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest > > On Fri, Oct 28, 2016 at 02:07:21PM +0200, Paolo Bonzini wrote: > > cpuid_count_edx would be just > > > > static inline unsigned int cpuid_count_edx(unsigned op, unsigned count) > > { > > unsigned int eax, ebx, ecx, edx; > > > > cpuid_count(op, count, &eax, &ebx, &ecx, &edx); > > > > return edx; > > } > > Even better. > > But shouldn't this be hiding unimplemented CPUID bits from the guest? Currently none of the bits in CPUID[7,0].edx is ever masked by the host, so this would be enough. If we ever need to do some masking, I guess I'll practice my puss-in-boots look and submit a patch to add CPUID[7,0] back as a separate cpufeature entr. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Oct 29, 2016 at 08:21:17AM -0400, Paolo Bonzini wrote: > Currently none of the bits in CPUID[7,0].edx is ever masked by the host, so > this would be enough. If we ever need to do some masking, I guess I'll > practice my puss-in-boots look and submit a patch to add CPUID[7,0] back > as a separate cpufeature entr. I don't understand - why can't it be filtered here if needed? I.e., return edx & KVM_CPUID_EDX_7_MASK; or so? Btw, we already have a cpuid_edx() helper in arch/x86/include/asm/processor.h
----- Original Message ----- > From: "Borislav Petkov" <bp@alien8.de> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: "Piotr Luc" <Piotr.Luc@intel.com>, kvm@vger.kernel.org, "he chen" <he.chen@linux.intel.com>, > linux-kernel@vger.kernel.org, tglx@linutronix.de, x86@kernel.org, hpa@zytor.com, mingo@redhat.com, "Luwei Kang" > <luwei.kang@intel.com>, rkrcmar@redhat.com > Sent: Saturday, October 29, 2016 2:25:48 PM > Subject: Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest > > On Sat, Oct 29, 2016 at 08:21:17AM -0400, Paolo Bonzini wrote: > > Currently none of the bits in CPUID[7,0].edx is ever masked by the host, so > > this would be enough. If we ever need to do some masking, I guess I'll > > practice my puss-in-boots look and submit a patch to add CPUID[7,0] back > > as a separate cpufeature entr. > > I don't understand - why can't it be filtered here if needed? I.e., > > return edx & KVM_CPUID_EDX_7_MASK; > > or so? Because then it wouldn't be in processor.h. > Btw, we already have a cpuid_edx() helper in arch/x86/include/asm/processor.h Yes, but it doesn't take an ecx. Anyhow this is not an issue for now. It will depend on which other bits are added to CPUID[7,0].edx, but in general it's relatively rare to blacklist bits from cpufeature. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Oct 29, 2016 at 08:36:11AM -0400, Paolo Bonzini wrote: > Because then it wouldn't be in processor.h. Easy: return cpuid_edx(…) & KVM_CPUID_EDX_7_MASK; at the call site. > Yes, but it doesn't take an ecx. Looks like we need another set of macros :-) > Anyhow this is not an issue for now. It will depend on which other bits > are added to CPUID[7,0].edx, but in general it's relatively rare to blacklist > bits from cpufeature. Right. Thanks.
----- Original Message ----- > From: "Borislav Petkov" <bp@alien8.de> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: "Piotr Luc" <Piotr.Luc@intel.com>, kvm@vger.kernel.org, "he chen" <he.chen@linux.intel.com>, > linux-kernel@vger.kernel.org, tglx@linutronix.de, x86@kernel.org, hpa@zytor.com, mingo@redhat.com, "Luwei Kang" > <luwei.kang@intel.com>, rkrcmar@redhat.com > Sent: Saturday, October 29, 2016 2:53:10 PM > Subject: Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest > > On Sat, Oct 29, 2016 at 08:36:11AM -0400, Paolo Bonzini wrote: > > Because then it wouldn't be in processor.h. > > Easy: > > return cpuid_edx(…) & KVM_CPUID_EDX_7_MASK; > > at the call site. Yup. That's what I said before (entry->edx &= cpuid_count_edx(7, 0)). Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gU2F0LCAyMDE2LTEwLTI5IGF0IDA4OjIxIC0wNDAwLCBQYW9sbyBCb256aW5pIHdyb3RlOg0K PiANCj4gQ3VycmVudGx5IG5vbmUgb2YgdGhlIGJpdHMgaW4gQ1BVSURbNywwXS5lZHggaXMgZXZl ciBtYXNrZWQgYnkgdGhlDQo+IGhvc3QsIHNvDQo+IHRoaXMgd291bGQgYmUgZW5vdWdoLsKgwqBJ ZiB3ZSBldmVyIG5lZWQgdG8gZG8gc29tZSBtYXNraW5nLCBJIGd1ZXNzDQo+IEknbGwNCj4gcHJh Y3RpY2UgbXkgcHVzcy1pbi1ib290cyBsb29rIGFuZCBzdWJtaXQgYSBwYXRjaCB0byBhZGQgQ1BV SURbNywwXQ0KPiBiYWNrDQo+IGFzIGEgc2VwYXJhdGUgY3B1ZmVhdHVyZSBlbnRyLg0KDQpJIHRo aW5rIHRoYXQgaW4gdjQuOS1yYzIgdGhlwqBDUFVJRFs3LDBdLmVkeCBiaXRzIGNhbiBiZSBtYXNr ZWQgb3V0IGJ5DQphcHBseWluZyBub3hzYXZlIHRvIGNtZGxpbmUuIFVzaW5nIGRpcmVjdGx5IGNw dV9jb3VudCB3aWxsIHJlc3VsdCBpbg0KcGFzc2luZyB0aGUgYml0cyBpbiBlZHggdG8gYSBndWVz dCBkaXJlY3RseSB3aGlsZSB0aGUgeHNhdmVvcHQgYW5kIHJlc3QNCm9mIEFWWDUxMiBmZWF0dXJl cyBiaXRzIHdpbGwgYmUgY2xlYXJlZC7CoA0KDQpUaHgNClBpb3Ry -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 31, 2016 at 09:15:43AM +0000, Luc, Piotr wrote: > I think that in v4.9-rc2 the CPUID[7,0].edx bits can be masked out by > applying noxsave to cmdline. Using directly cpu_count will result in > passing the bits in edx to a guest directly while the xsaveopt and rest > of AVX512 features bits will be cleared. Errr, I can't parse that reading it backwards and forwards. Please elaborate.
On Mon, 2016-10-31 at 10:53 +0100, Borislav Petkov wrote: > > I think that in v4.9-rc2 the CPUID[7,0].edx bits can be masked out > by > > applying noxsave to cmdline. Using directly cpu_count will result > in > > passing the bits in edx to a guest directly while the xsaveopt and > rest > > of AVX512 features bits will be cleared. > > Errr, I can't parse that reading it backwards and forwards. Please > elaborate. The patch that introduces AVX512_4VNNIW and AVX512_4FMAPS features was merged to kernel 4.9-rc2 so we have possibility to mask the feature bits using 'noxsave' option in kernel cmdline. This option clears all AVX512 feature bits in boot_cpu_data.x86_capability. The cpuid_mask function, which usually used in kvm, read bit from this x86_capabity and mask out. This prevents passing disabled features to guest. If we use cpu_count instead, which reports bits directly from CPU, then the bits of features that are disabled in host are passed to guest as enabled. This seems be inconsistent. Thanks, Piotr
On Mon, Oct 31, 2016 at 10:18:41AM +0000, Luc, Piotr wrote: > The cpuid_mask function, which usually used in kvm, read bit from this > x86_capabity and mask out. This prevents passing disabled features to > guest. If we use cpu_count instead, which reports bits directly from Ah, you mean cpuid_count(). > CPU, then the bits of features that are disabled in host are passed to > guest as enabled. This seems be inconsistent. Ok, I see what you mean. So I guess we'll have to iterate over the cpuid_bits[] array and recreate the CPUID leaf for KVM instead, as I suggested earlier.
On 31/10/2016 11:30, Borislav Petkov wrote: > On Mon, Oct 31, 2016 at 10:18:41AM +0000, Luc, Piotr wrote: >> The cpuid_mask function, which usually used in kvm, read bit from this >> x86_capabity and mask out. This prevents passing disabled features to >> guest. If we use cpu_count instead, which reports bits directly from > > Ah, you mean cpuid_count(). > >> CPU, then the bits of features that are disabled in host are passed to >> guest as enabled. This seems be inconsistent. > > Ok, I see what you mean. > > So I guess we'll have to iterate over the cpuid_bits[] array and > recreate the CPUID leaf for KVM instead, as I suggested earlier. Yes, indeed. :( The information is all in arch/x86/kernel/cpu/scattered.c's cpuid_bits array. Borislav, would it be okay to export the cpuid_regs enum? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 31, 2016 at 11:47:48AM +0100, Paolo Bonzini wrote: > The information is all in arch/x86/kernel/cpu/scattered.c's cpuid_bits > array. Borislav, would it be okay to export the cpuid_regs enum? Yeah, and kill the duplicated one in arch/x86/events/intel/pt.c too please, while at it. I'd still put it all in arch/x86/kernel/cpu/scattered.c so that it is close-by and call it from outside. Thanks.
On 31/10/2016 12:05, Borislav Petkov wrote: > On Mon, Oct 31, 2016 at 11:47:48AM +0100, Paolo Bonzini wrote: >> The information is all in arch/x86/kernel/cpu/scattered.c's cpuid_bits >> array. Borislav, would it be okay to export the cpuid_regs enum? > > Yeah, and kill the duplicated one in arch/x86/events/intel/pt.c too > please, while at it. > > I'd still put it all in arch/x86/kernel/cpu/scattered.c so that it is > close-by and call it from outside. Good. Chen, are you going to do this? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 31, 2016 at 12:41:32PM +0100, Paolo Bonzini wrote: > > > On 31/10/2016 12:05, Borislav Petkov wrote: > > On Mon, Oct 31, 2016 at 11:47:48AM +0100, Paolo Bonzini wrote: > >> The information is all in arch/x86/kernel/cpu/scattered.c's cpuid_bits > >> array. Borislav, would it be okay to export the cpuid_regs enum? > > > > Yeah, and kill the duplicated one in arch/x86/events/intel/pt.c too > > please, while at it. > > > > I'd still put it all in arch/x86/kernel/cpu/scattered.c so that it is > > close-by and call it from outside. > > Good. Chen, are you going to do this? > Sure. Before sending a patch, let me check if my understanding is right... I will add a helper in scattered.c like: unsigned int get_scattered_cpuid_features(unsigned int level, unsigned int sub_leaf, enum cpuid_regs reg) { u32 val = 0; const struct cpuid_bit *cb; for (cb = cpuid_bits; cb->feature; cb++) { if (reg == cb->reg && level == cb->level && sub_leaf == cb->sub_leaf && boot_cpu_has(cb->feature)) val |= cb->bit; } return val; } And, when KVM wants to mask out features, it can be called outside like: entry->edx &= kvm_cpuid_7_0_edx_x86_features; entry->edx &= get_scatterd_cpuid_features(7, 0, CR_EDX); Thanks, -He -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 01, 2016 at 03:48:50PM +0800, He Chen wrote: > Before sending a patch, let me check if my understanding is right... > I will add a helper in scattered.c like: > > unsigned int get_scattered_cpuid_features(unsigned int level, > unsigned int sub_leaf, enum cpuid_regs reg) > { > u32 val = 0; > const struct cpuid_bit *cb; > > for (cb = cpuid_bits; cb->feature; cb++) { Right, we can improve that by keeping cpuid_bit.level sorted so that you can break out early if the level is exceeded. For that please sort it and add a comment stating that the leaf should be kept sorted ontop of it. And then you do something like this: u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf, enum cpuid_regs reg) { u32 cpuid_val = 0; for (cb = cpuid_bits; cb->feature; cb++) { if (level < cb->level) continue; if (level > cb->level) break; if (cb->reg == reg && sub_leaf == cb->sub_leaf) { if (cpu_has(cb->feature)) cpuid_val |= BIT(cb->bit); } } return cpuid_val; } Completely untested, of course.
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index afa7bbb..328b169 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -376,6 +376,10 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, /* cpuid 7.0.ecx*/ const u32 kvm_cpuid_7_0_ecx_x86_features = F(PKU) | 0 /*OSPKE*/; + /* cpuid 7.0.edx*/ + const u32 kvm_cpuid_7_0_edx_x86_features = + 0x4 /* AVX512-4VNNIW */ | 0x8 /* AVX512-4FMAPS */; + /* all calls to cpuid_count() should be made on the same cpu */ get_cpu(); @@ -458,12 +462,13 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, /* PKU is not yet implemented for shadow paging. */ if (!tdp_enabled) entry->ecx &= ~F(PKU); + entry->edx &= kvm_cpuid_7_0_edx_x86_features; } else { entry->ebx = 0; entry->ecx = 0; + entry->edx = 0; } entry->eax = 0; - entry->edx = 0; break; } case 9: