Minor bug fixes and refactorings of SEV related code, mainly to clean up
the KVM code for tracking whether or not SEV and SEV-ES are enabled. E.g.
KVM has both sev_es and svm_sev_enabled(), and a global 'sev' flag while
also using 'sev' as a local variable in several places.
Based on kvm/master, commit 872f36eb0b0f ("KVM: x86: __kvm_vcpu_halt can
be static").
v2:
- Remove the kernel's sev_enabled instead of renaming it to sev_guest.
- Fix various build issues. [Tom]
- Remove stable tag from the patch to free sev_asid_bitmap. Keeping the
bitmap on failure is truly only a leak once svm_sev_enabled() is
dropped later in the series. It's still arguably a fix since KVM will
unnecessarily keep memory, but it's not stable material. [Tom]
- Collect one Ack. [Tom]
v1:
- https://lkml.kernel.org/r/[email protected]
Sean Christopherson (14):
KVM: SVM: Zero out the VMCB array used to track SEV ASID association
KVM: SVM: Free sev_asid_bitmap during init if SEV setup fails
KVM: SVM: Move SEV module params/variables to sev.c
x86/cpufeatures: Assign dedicated feature word for AMD mem encryption
KVM: x86: Override reported SME/SEV feature flags with host mask
x86/sev: Drop redundant and potentially misleading 'sev_enabled'
KVM: SVM: Append "_enabled" to module-scoped SEV/SEV-ES control
variables
KVM: SVM: Condition sev_enabled and sev_es_enabled on
CONFIG_KVM_AMD_SEV=y
KVM: SVM: Unconditionally invoke sev_hardware_teardown()
KVM: SVM: Explicitly check max SEV ASID during sev_hardware_setup()
KVM: SVM: Move SEV VMCB tracking allocation to sev.c
KVM: SVM: Drop redundant svm_sev_enabled() helper
KVM: SVM: Remove an unnecessary prototype declaration of
sev_flush_asids()
KVM: SVM: Skip SEV cache flush if no ASIDs have been used
arch/x86/include/asm/cpufeature.h | 7 +-
arch/x86/include/asm/cpufeatures.h | 17 +++--
arch/x86/include/asm/disabled-features.h | 3 +-
arch/x86/include/asm/mem_encrypt.h | 1 -
arch/x86/include/asm/required-features.h | 3 +-
arch/x86/kernel/cpu/common.c | 3 +
arch/x86/kernel/cpu/scattered.c | 5 --
arch/x86/kvm/cpuid.c | 2 +
arch/x86/kvm/cpuid.h | 1 +
arch/x86/kvm/svm/sev.c | 71 +++++++++++++------
arch/x86/kvm/svm/svm.c | 35 +++------
arch/x86/kvm/svm/svm.h | 8 +--
arch/x86/mm/mem_encrypt.c | 12 ++--
arch/x86/mm/mem_encrypt_identity.c | 1 -
.../arch/x86/include/asm/disabled-features.h | 3 +-
.../arch/x86/include/asm/required-features.h | 3 +-
16 files changed, 96 insertions(+), 79 deletions(-)
--
2.30.0.284.gd98b1dd5eaa7-goog
Free sev_asid_bitmap if the reclaim bitmap allocation fails, othwerise
KVM will unnecessarily keep the bitmap when SEV is not fully enabled.
Freeing the page is also necessary to avoid introducing a bug when a
future patch eliminates svm_sev_enabled() in favor of using the global
'sev' flag directly. While sev_hardware_enabled() checks max_sev_asid,
which is true even if KVM setup fails, 'sev' will be true if and only
if KVM setup fully succeeds.
Fixes: 33af3a7ef9e6 ("KVM: SVM: Reduce WBINVD/DF_FLUSH invocations")
Cc: Tom Lendacky <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/svm/sev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index c8ffdbc81709..0eeb6e1b803d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1274,8 +1274,10 @@ void __init sev_hardware_setup(void)
goto out;
sev_reclaim_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
- if (!sev_reclaim_asid_bitmap)
+ if (!sev_reclaim_asid_bitmap) {
+ bitmap_free(sev_asid_bitmap);
goto out;
+ }
pr_info("SEV supported: %u ASIDs\n", max_sev_asid - min_sev_asid + 1);
sev_supported = true;
--
2.30.0.284.gd98b1dd5eaa7-goog
Zero out the array of VMCB pointers so that pre_sev_run() won't see
garbage when querying the array to detect when an SEV ASID is being
associated with a new VMCB. In practice, reading random values is all
but guaranteed to be benign as a false negative (which is extremely
unlikely on its own) can only happen on CPU0 on the first VMRUN and would
only cause KVM to skip the ASID flush. For anything bad to happen, a
previous instance of KVM would have to exit without flushing the ASID,
_and_ KVM would have to not flush the ASID at any time while building the
new SEV guest.
Cc: Borislav Petkov <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Brijesh Singh <[email protected]>
Fixes: 70cd94e60c73 ("KVM: SVM: VMRUN should use associated ASID when SEV is enabled")
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/svm/svm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7ef171790d02..ccf52c5531fb 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -573,7 +573,7 @@ static int svm_cpu_init(int cpu)
if (svm_sev_enabled()) {
sd->sev_vmcbs = kmalloc_array(max_sev_asid + 1,
sizeof(void *),
- GFP_KERNEL);
+ GFP_KERNEL | __GFP_ZERO);
if (!sd->sev_vmcbs)
goto free_save_area;
}
--
2.30.0.284.gd98b1dd5eaa7-goog
On 1/13/21 6:36 PM, Sean Christopherson wrote:
> Free sev_asid_bitmap if the reclaim bitmap allocation fails, othwerise
> KVM will unnecessarily keep the bitmap when SEV is not fully enabled.
>
> Freeing the page is also necessary to avoid introducing a bug when a
> future patch eliminates svm_sev_enabled() in favor of using the global
> 'sev' flag directly. While sev_hardware_enabled() checks max_sev_asid,
> which is true even if KVM setup fails, 'sev' will be true if and only
> if KVM setup fully succeeds.
>
> Fixes: 33af3a7ef9e6 ("KVM: SVM: Reduce WBINVD/DF_FLUSH invocations")
> Cc: Tom Lendacky <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/svm/sev.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c8ffdbc81709..0eeb6e1b803d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1274,8 +1274,10 @@ void __init sev_hardware_setup(void)
> goto out;
>
> sev_reclaim_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
> - if (!sev_reclaim_asid_bitmap)
> + if (!sev_reclaim_asid_bitmap) {
> + bitmap_free(sev_asid_bitmap);
Until that future change, you probably need to do sev_asid_bitmap = NULL
here to avoid an issue in sev_hardware_teardown() when it tries to free it
again.
Thanks,
Tom
> goto out;
> + }
>
> pr_info("SEV supported: %u ASIDs\n", max_sev_asid - min_sev_asid + 1);
> sev_supported = true;
>
On 1/13/21 6:36 PM, Sean Christopherson wrote:
> Zero out the array of VMCB pointers so that pre_sev_run() won't see
> garbage when querying the array to detect when an SEV ASID is being
> associated with a new VMCB. In practice, reading random values is all
> but guaranteed to be benign as a false negative (which is extremely
> unlikely on its own) can only happen on CPU0 on the first VMRUN and would
> only cause KVM to skip the ASID flush. For anything bad to happen, a
> previous instance of KVM would have to exit without flushing the ASID,
> _and_ KVM would have to not flush the ASID at any time while building the
> new SEV guest.
>
> Cc: Borislav Petkov <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: Brijesh Singh <[email protected]>
> Fixes: 70cd94e60c73 ("KVM: SVM: VMRUN should use associated ASID when SEV is enabled")
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/svm/svm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7ef171790d02..ccf52c5531fb 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -573,7 +573,7 @@ static int svm_cpu_init(int cpu)
> if (svm_sev_enabled()) {
> sd->sev_vmcbs = kmalloc_array(max_sev_asid + 1,
> sizeof(void *),
> - GFP_KERNEL);
> + GFP_KERNEL | __GFP_ZERO);
Alternatively, this call could just be changed to kcalloc().
Either way,
Reviewed-by: Tom Lendacky <[email protected]>
> if (!sd->sev_vmcbs)
> goto free_save_area;
> }
>
On Thu, Jan 14, 2021, Tom Lendacky wrote:
> On 1/13/21 6:36 PM, Sean Christopherson wrote:
> > Free sev_asid_bitmap if the reclaim bitmap allocation fails, othwerise
> > KVM will unnecessarily keep the bitmap when SEV is not fully enabled.
> >
> > Freeing the page is also necessary to avoid introducing a bug when a
> > future patch eliminates svm_sev_enabled() in favor of using the global
> > 'sev' flag directly. While sev_hardware_enabled() checks max_sev_asid,
> > which is true even if KVM setup fails, 'sev' will be true if and only
> > if KVM setup fully succeeds.
> >
> > Fixes: 33af3a7ef9e6 ("KVM: SVM: Reduce WBINVD/DF_FLUSH invocations")
> > Cc: Tom Lendacky <[email protected]>
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > arch/x86/kvm/svm/sev.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index c8ffdbc81709..0eeb6e1b803d 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -1274,8 +1274,10 @@ void __init sev_hardware_setup(void)
> > goto out;
> > sev_reclaim_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
> > - if (!sev_reclaim_asid_bitmap)
> > + if (!sev_reclaim_asid_bitmap) {
> > + bitmap_free(sev_asid_bitmap);
>
> Until that future change, you probably need to do sev_asid_bitmap = NULL
> here to avoid an issue in sev_hardware_teardown() when it tries to free it
> again.
Argh, you're right. Thanks!
On Thu, Jan 14, 2021, Tom Lendacky wrote:
> On 1/13/21 6:36 PM, Sean Christopherson wrote:
> > Zero out the array of VMCB pointers so that pre_sev_run() won't see
> > garbage when querying the array to detect when an SEV ASID is being
> > associated with a new VMCB. In practice, reading random values is all
> > but guaranteed to be benign as a false negative (which is extremely
> > unlikely on its own) can only happen on CPU0 on the first VMRUN and would
> > only cause KVM to skip the ASID flush. For anything bad to happen, a
> > previous instance of KVM would have to exit without flushing the ASID,
> > _and_ KVM would have to not flush the ASID at any time while building the
> > new SEV guest.
> >
> > Cc: Borislav Petkov <[email protected]>
> > Cc: Tom Lendacky <[email protected]>
> > Cc: Brijesh Singh <[email protected]>
> > Fixes: 70cd94e60c73 ("KVM: SVM: VMRUN should use associated ASID when SEV is enabled")
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > arch/x86/kvm/svm/svm.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 7ef171790d02..ccf52c5531fb 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -573,7 +573,7 @@ static int svm_cpu_init(int cpu)
> > if (svm_sev_enabled()) {
> > sd->sev_vmcbs = kmalloc_array(max_sev_asid + 1,
> > sizeof(void *),
> > - GFP_KERNEL);
> > + GFP_KERNEL | __GFP_ZERO);
>
> Alternatively, this call could just be changed to kcalloc().
Agreed, kcalloc() is a better option. I'll do that in v3.
On 1/14/21 11:12 AM, Sean Christopherson wrote:
> On Thu, Jan 14, 2021, Tom Lendacky wrote:
>> On 1/13/21 6:36 PM, Sean Christopherson wrote:
>>> Free sev_asid_bitmap if the reclaim bitmap allocation fails, othwerise
>>> KVM will unnecessarily keep the bitmap when SEV is not fully enabled.
>>>
>>> Freeing the page is also necessary to avoid introducing a bug when a
>>> future patch eliminates svm_sev_enabled() in favor of using the global
>>> 'sev' flag directly. While sev_hardware_enabled() checks max_sev_asid,
>>> which is true even if KVM setup fails, 'sev' will be true if and only
>>> if KVM setup fully succeeds.
>>>
>>> Fixes: 33af3a7ef9e6 ("KVM: SVM: Reduce WBINVD/DF_FLUSH invocations")
Oops, missed this last time... I don't think the Fixes: tag is needed
anymore unless you don't want the memory consumption of the first bitmap,
should the allocation of the second bitmap fail, until kvm_amd is
rmmod'ed. Up to you.
Thanks,
Tom
>>> Cc: Tom Lendacky <[email protected]>
>>> Signed-off-by: Sean Christopherson <[email protected]>
>>> ---
>>> arch/x86/kvm/svm/sev.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index c8ffdbc81709..0eeb6e1b803d 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -1274,8 +1274,10 @@ void __init sev_hardware_setup(void)
>>> goto out;
>>> sev_reclaim_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
>>> - if (!sev_reclaim_asid_bitmap)
>>> + if (!sev_reclaim_asid_bitmap) {
>>> + bitmap_free(sev_asid_bitmap);
>>
>> Until that future change, you probably need to do sev_asid_bitmap = NULL
>> here to avoid an issue in sev_hardware_teardown() when it tries to free it
>> again.
>
> Argh, you're right. Thanks!
>
On Thu, Jan 14, 2021, Tom Lendacky wrote:
> On 1/14/21 11:12 AM, Sean Christopherson wrote:
> > On Thu, Jan 14, 2021, Tom Lendacky wrote:
> > > On 1/13/21 6:36 PM, Sean Christopherson wrote:
> > > > Free sev_asid_bitmap if the reclaim bitmap allocation fails, othwerise
> > > > KVM will unnecessarily keep the bitmap when SEV is not fully enabled.
> > > >
> > > > Freeing the page is also necessary to avoid introducing a bug when a
> > > > future patch eliminates svm_sev_enabled() in favor of using the global
> > > > 'sev' flag directly. While sev_hardware_enabled() checks max_sev_asid,
> > > > which is true even if KVM setup fails, 'sev' will be true if and only
> > > > if KVM setup fully succeeds.
> > > >
> > > > Fixes: 33af3a7ef9e6 ("KVM: SVM: Reduce WBINVD/DF_FLUSH invocations")
>
> Oops, missed this last time... I don't think the Fixes: tag is needed
> anymore unless you don't want the memory consumption of the first bitmap,
If Fixes is viewed as purely a "this needs to be backported", then yes, it
should be dropped. But, since KVM policy is to backport only patches that are
explicitly tagged with stable@, I like to use to Fixes to create a paper trail
for bug fixes even if the bug is essentially benign.
That being said, I have no objection to dropping it if anyone feels strongly
about not playing fast and loose with Fixes.
> should the allocation of the second bitmap fail, until kvm_amd is rmmod'ed.
> Up to you.
On 1/13/21 6:36 PM, Sean Christopherson wrote:
> Zero out the array of VMCB pointers so that pre_sev_run() won't see
> garbage when querying the array to detect when an SEV ASID is being
> associated with a new VMCB. In practice, reading random values is all
> but guaranteed to be benign as a false negative (which is extremely
> unlikely on its own) can only happen on CPU0 on the first VMRUN and would
> only cause KVM to skip the ASID flush. For anything bad to happen, a
> previous instance of KVM would have to exit without flushing the ASID,
> _and_ KVM would have to not flush the ASID at any time while building the
> new SEV guest.
>
> Cc: Borislav Petkov <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: Brijesh Singh <[email protected]>
> Fixes: 70cd94e60c73 ("KVM: SVM: VMRUN should use associated ASID when SEV is enabled")
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/svm/svm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7ef171790d02..ccf52c5531fb 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -573,7 +573,7 @@ static int svm_cpu_init(int cpu)
> if (svm_sev_enabled()) {
> sd->sev_vmcbs = kmalloc_array(max_sev_asid + 1,
> sizeof(void *),
> - GFP_KERNEL);
> + GFP_KERNEL | __GFP_ZERO);
> if (!sd->sev_vmcbs)
> goto free_save_area;
> }
Reviewed-by: Brijesh Singh <[email protected]>