[PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[He Chen]

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;
 		} else {
 			entry->ebx = 0;
 			entry->ecx = 0;
+            entry->edx = 0;
 		}
 		entry->eax = 0;
-		entry->edx = 0;
 		break;
 	}
 	case 9:
-- 
2.7.4

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Paolo Bonzini]

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:
> 

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[He Chen]

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

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Paolo Bonzini]

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

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[He Chen]

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 :-)

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Luc, Piotr]

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.

>  		} else {
>  			entry->ebx = 0;
>  			entry->ecx = 0;
> +            entry->edx = 0;
>  		}
>  		entry->eax = 0;
> -		entry->edx = 0;
>  		break;
>  	}
>  	case 9:

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Paolo Bonzini]

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:

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Borislav Petkov]

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...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Paolo Bonzini]

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

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Borislav Petkov]

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?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Paolo Bonzini]

----- 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

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Borislav Petkov]

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

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Paolo Bonzini]

----- 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

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Paolo Bonzini]

----- 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

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Luc, Piotr]

On Sat, 2016-10-29 at 08:21 -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 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. 

Thx
Piotr

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Luc, Piotr]

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

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Paolo Bonzini]

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

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Paolo Bonzini]

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

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[He Chen]

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

Re: [PATCH] x86/cpuid: expose AVX512_4VNNIW and AVX512_4FMAPS features to kvm guest

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.