2021-01-14 01:51:11

by Sean Christopherson

Subject: [PATCH v2 05/14] KVM: x86: Override reported SME/SEV feature flags with host mask

Add a reverse-CPUID entry for the memory encryption word, 0x8000001F.EAX,
and use it to override the supported CPUID flags reported to userspace.
Masking the reported CPUID flags avoids over-reporting KVM support, e.g.
without the mask a SEV-SNP capable CPU may incorrectly advertise SNP
support to userspace.

Cc: Brijesh Singh <[email protected]>
Cc: Tom Lendacky <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/cpuid.c | 2 ++
arch/x86/kvm/cpuid.h | 1 +
2 files changed, 3 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 13036cf0b912..b7618cdd06b5 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -855,6 +855,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
case 0x8000001F:
if (!boot_cpu_has(X86_FEATURE_SEV))
entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+ else
+ cpuid_entry_override(entry, CPUID_8000_001F_EAX);
break;
/*Add support for Centaur's CPUID instruction*/
case 0xC0000000:
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index dc921d76e42e..8b6fc9bde248 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -63,6 +63,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
[CPUID_8000_0007_EBX] = {0x80000007, 0, CPUID_EBX},
[CPUID_7_EDX] = { 7, 0, CPUID_EDX},
[CPUID_7_1_EAX] = { 7, 1, CPUID_EAX},
+ [CPUID_8000_001F_EAX] = {0x8000001f, 1, CPUID_EAX},
};

/*
--
2.30.0.284.gd98b1dd5eaa7-goog

2021-01-14 21:23:08

by Brijesh Singh

Subject: Re: [PATCH v2 05/14] KVM: x86: Override reported SME/SEV feature flags with host mask

On 1/13/21 6:36 PM, Sean Christopherson wrote:
> Add a reverse-CPUID entry for the memory encryption word, 0x8000001F.EAX,
> and use it to override the supported CPUID flags reported to userspace.
> Masking the reported CPUID flags avoids over-reporting KVM support, e.g.
> without the mask a SEV-SNP capable CPU may incorrectly advertise SNP
> support to userspace.
>
> Cc: Brijesh Singh <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/cpuid.c | 2 ++
> arch/x86/kvm/cpuid.h | 1 +
> 2 files changed, 3 insertions(+)

thanks

Reviewed-by: Brijesh Singh <[email protected]>

>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 13036cf0b912..b7618cdd06b5 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -855,6 +855,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> case 0x8000001F:
> if (!boot_cpu_has(X86_FEATURE_SEV))
> entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> + else
> + cpuid_entry_override(entry, CPUID_8000_001F_EAX);
> break;
> /*Add support for Centaur's CPUID instruction*/
> case 0xC0000000:
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index dc921d76e42e..8b6fc9bde248 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -63,6 +63,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
> [CPUID_8000_0007_EBX] = {0x80000007, 0, CPUID_EBX},
> [CPUID_7_EDX] = { 7, 0, CPUID_EDX},
> [CPUID_7_1_EAX] = { 7, 1, CPUID_EAX},
> + [CPUID_8000_001F_EAX] = {0x8000001f, 1, CPUID_EAX},
> };
>
> /*

2021-01-28 15:11:55

by Paolo Bonzini

Subject: Re: [PATCH v2 05/14] KVM: x86: Override reported SME/SEV feature flags with host mask

On 14/01/21 01:36, Sean Christopherson wrote:
> Add a reverse-CPUID entry for the memory encryption word, 0x8000001F.EAX,
> and use it to override the supported CPUID flags reported to userspace.
> Masking the reported CPUID flags avoids over-reporting KVM support, e.g.
> without the mask a SEV-SNP capable CPU may incorrectly advertise SNP
> support to userspace.
>
> Cc: Brijesh Singh <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/cpuid.c | 2 ++
> arch/x86/kvm/cpuid.h | 1 +
> 2 files changed, 3 insertions(+)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 13036cf0b912..b7618cdd06b5 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -855,6 +855,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> case 0x8000001F:
> if (!boot_cpu_has(X86_FEATURE_SEV))
> entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> + else
> + cpuid_entry_override(entry, CPUID_8000_001F_EAX);
> break;
> /*Add support for Centaur's CPUID instruction*/
> case 0xC0000000:
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index dc921d76e42e..8b6fc9bde248 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -63,6 +63,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
> [CPUID_8000_0007_EBX] = {0x80000007, 0, CPUID_EBX},
> [CPUID_7_EDX] = { 7, 0, CPUID_EDX},
> [CPUID_7_1_EAX] = { 7, 1, CPUID_EAX},
> + [CPUID_8000_001F_EAX] = {0x8000001f, 1, CPUID_EAX},
> };
>
> /*
>

I don't understand, wouldn't this also need a kvm_cpu_cap_mask call
somewhere else? As it is, it doesn't do anything.

Paolo

2021-01-28 17:14:11

by Sean Christopherson

Subject: Re: [PATCH v2 05/14] KVM: x86: Override reported SME/SEV feature flags with host mask

On Thu, Jan 28, 2021, Paolo Bonzini wrote:
> On 14/01/21 01:36, Sean Christopherson wrote:
> > Add a reverse-CPUID entry for the memory encryption word, 0x8000001F.EAX,
> > and use it to override the supported CPUID flags reported to userspace.
> > Masking the reported CPUID flags avoids over-reporting KVM support, e.g.
> > without the mask a SEV-SNP capable CPU may incorrectly advertise SNP
> > support to userspace.
> >
> > Cc: Brijesh Singh <[email protected]>
> > Cc: Tom Lendacky <[email protected]>
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > arch/x86/kvm/cpuid.c | 2 ++
> > arch/x86/kvm/cpuid.h | 1 +
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 13036cf0b912..b7618cdd06b5 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -855,6 +855,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> > case 0x8000001F:
> > if (!boot_cpu_has(X86_FEATURE_SEV))
> > entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> > + else
> > + cpuid_entry_override(entry, CPUID_8000_001F_EAX);
> > break;
> > /*Add support for Centaur's CPUID instruction*/
> > case 0xC0000000:
> > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> > index dc921d76e42e..8b6fc9bde248 100644
> > --- a/arch/x86/kvm/cpuid.h
> > +++ b/arch/x86/kvm/cpuid.h
> > @@ -63,6 +63,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
> > [CPUID_8000_0007_EBX] = {0x80000007, 0, CPUID_EBX},
> > [CPUID_7_EDX] = { 7, 0, CPUID_EDX},
> > [CPUID_7_1_EAX] = { 7, 1, CPUID_EAX},
> > + [CPUID_8000_001F_EAX] = {0x8000001f, 1, CPUID_EAX},
> > };
> > /*
> >
>
> I don't understand, wouldn't this also need a kvm_cpu_cap_mask call
> somewhere else? As it is, it doesn't do anything.

Ugh, yes, apparently I thought the kernel would magically clear bits it doesn't
care about.

Looking at this again, I think the kvm_cpu_cap_mask() invocation should always
mask off X86_FEATURE_SME. SME cannot be virtualized, and AFAIK it's not
emulated by KVM. This would fix an oddity where SME would be advertised if SEV
is also supported.

Boris has queue the kernel change to tip/x86/cpu, I'll spin v4 against that.

2021-01-28 17:33:24

by Paolo Bonzini

Subject: Re: [PATCH v2 05/14] KVM: x86: Override reported SME/SEV feature flags with host mask

On 28/01/21 18:09, Sean Christopherson wrote:
> On Thu, Jan 28, 2021, Paolo Bonzini wrote:
>> On 14/01/21 01:36, Sean Christopherson wrote:
>>> Add a reverse-CPUID entry for the memory encryption word, 0x8000001F.EAX,
>>> and use it to override the supported CPUID flags reported to userspace.
>>> Masking the reported CPUID flags avoids over-reporting KVM support, e.g.
>>> without the mask a SEV-SNP capable CPU may incorrectly advertise SNP
>>> support to userspace.
>>>
>>> Cc: Brijesh Singh <[email protected]>
>>> Cc: Tom Lendacky <[email protected]>
>>> Signed-off-by: Sean Christopherson <[email protected]>
>>> ---
>>> arch/x86/kvm/cpuid.c | 2 ++
>>> arch/x86/kvm/cpuid.h | 1 +
>>> 2 files changed, 3 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>> index 13036cf0b912..b7618cdd06b5 100644
>>> --- a/arch/x86/kvm/cpuid.c
>>> +++ b/arch/x86/kvm/cpuid.c
>>> @@ -855,6 +855,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>>> case 0x8000001F:
>>> if (!boot_cpu_has(X86_FEATURE_SEV))
>>> entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
>>> + else
>>> + cpuid_entry_override(entry, CPUID_8000_001F_EAX);
>>> break;
>>> /*Add support for Centaur's CPUID instruction*/
>>> case 0xC0000000:
>>> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
>>> index dc921d76e42e..8b6fc9bde248 100644
>>> --- a/arch/x86/kvm/cpuid.h
>>> +++ b/arch/x86/kvm/cpuid.h
>>> @@ -63,6 +63,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
>>> [CPUID_8000_0007_EBX] = {0x80000007, 0, CPUID_EBX},
>>> [CPUID_7_EDX] = { 7, 0, CPUID_EDX},
>>> [CPUID_7_1_EAX] = { 7, 1, CPUID_EAX},
>>> + [CPUID_8000_001F_EAX] = {0x8000001f, 1, CPUID_EAX},
>>> };
>>> /*
>>>
>>
>> I don't understand, wouldn't this also need a kvm_cpu_cap_mask call
>> somewhere else? As it is, it doesn't do anything.
>
> Ugh, yes, apparently I thought the kernel would magically clear bits it doesn't
> care about.
>
> Looking at this again, I think the kvm_cpu_cap_mask() invocation should always
> mask off X86_FEATURE_SME. SME cannot be virtualized, and AFAIK it's not
> emulated by KVM. This would fix an oddity where SME would be advertised if SEV
> is also supported.
>
> Boris has queue the kernel change to tip/x86/cpu, I'll spin v4 against that.

You can send it after the 5.12 merge window.

Paolo