[v5,00/15] KVM: SVM: Misc SEV cleanups
mbox series

Message ID 20210422021125.3417167-1-seanjc@google.com
Headers show
Series
  • KVM: SVM: Misc SEV cleanups
Related show

Message

Sean Christopherson April 22, 2021, 2:11 a.m. UTC
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 kvm/queue-ish, commit 0e91d1992235 ("KVM: SVM: Allocate SEV command
structures on local stack"), to avoid the conflicting CPUID.0x8000_001F
patch sitting in kvm/queue.

v5:
 - Use Paolo's version of the CPUID.0x8000_001F patch, with some of my
   goo on top.  Paolo gets credit by introducing fewer bugs; v4 missed
   the SEV/SEV-ES module params and used the wrong reverse-CPUID index...
 - Add a patch to disable SEV/SEV-ES if NPT is disabled.
 - Rebased, as above.
v4:
 - Reinstate the patch to override CPUID.0x8000_001F.
 - Properly configure the CPUID.0x8000_001F override. [Paolo]
 - Rebase to v5.12-rc1-dontuse.
v3:
 - Drop two patches: add a dedicated feature word for CPUID_0x8000001F,
   and use the new word to mask host CPUID in KVM.  I'll send these as a
   separate mini-series so that Boris can take them through tip.
 - Add a patch to remove dependency on
   CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT. [Boris / Paolo]
 - Use kcalloc() instead of an open-coded equivalent. [Tom]
 - Nullify sev_asid_bitmap when freeing it during setup. [Tom]
 - Add a comment in sev_hardware_teardown() to document why it's safe to
   query the ASID bitmap without taking the lock. [Tom]
 - Collect reviews. [Tom and Brijesh]
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/20210109004714.1341275-1-seanjc@google.com

Paolo Bonzini (1):
  KVM: SEV: Mask CPUID[0x8000001F].eax according to supported features

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: Disable SEV/SEV-ES if NPT is disabled
  KVM: SVM: Move SEV module params/variables to sev.c
  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: Enable SEV/SEV-ES functionality by default (when supported)
  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/mem_encrypt.h |  1 -
 arch/x86/kvm/cpuid.c               |  8 ++-
 arch/x86/kvm/cpuid.h               |  1 +
 arch/x86/kvm/svm/sev.c             | 80 ++++++++++++++++++++++--------
 arch/x86/kvm/svm/svm.c             | 57 +++++++++------------
 arch/x86/kvm/svm/svm.h             |  9 +---
 arch/x86/mm/mem_encrypt.c          | 12 ++---
 arch/x86/mm/mem_encrypt_identity.c |  1 -
 8 files changed, 97 insertions(+), 72 deletions(-)

Comments

Paolo Bonzini April 22, 2021, 7:30 a.m. UTC | #1
On 22/04/21 04:11, Sean Christopherson wrote:
> 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 kvm/queue-ish, commit 0e91d1992235 ("KVM: SVM: Allocate SEV command
> structures on local stack"), to avoid the conflicting CPUID.0x8000_001F
> patch sitting in kvm/queue.
> 
> v5:
>   - Use Paolo's version of the CPUID.0x8000_001F patch, with some of my
>     goo on top.  Paolo gets credit by introducing fewer bugs; v4 missed
>     the SEV/SEV-ES module params and used the wrong reverse-CPUID index...
>   - Add a patch to disable SEV/SEV-ES if NPT is disabled.
>   - Rebased, as above.
> v4:
>   - Reinstate the patch to override CPUID.0x8000_001F.
>   - Properly configure the CPUID.0x8000_001F override. [Paolo]
>   - Rebase to v5.12-rc1-dontuse.
> v3:
>   - Drop two patches: add a dedicated feature word for CPUID_0x8000001F,
>     and use the new word to mask host CPUID in KVM.  I'll send these as a
>     separate mini-series so that Boris can take them through tip.
>   - Add a patch to remove dependency on
>     CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT. [Boris / Paolo]
>   - Use kcalloc() instead of an open-coded equivalent. [Tom]
>   - Nullify sev_asid_bitmap when freeing it during setup. [Tom]
>   - Add a comment in sev_hardware_teardown() to document why it's safe to
>     query the ASID bitmap without taking the lock. [Tom]
>   - Collect reviews. [Tom and Brijesh]
> 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/20210109004714.1341275-1-seanjc@google.com
> 
> Paolo Bonzini (1):
>    KVM: SEV: Mask CPUID[0x8000001F].eax according to supported features
> 
> 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: Disable SEV/SEV-ES if NPT is disabled
>    KVM: SVM: Move SEV module params/variables to sev.c
>    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: Enable SEV/SEV-ES functionality by default (when supported)
>    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/mem_encrypt.h |  1 -
>   arch/x86/kvm/cpuid.c               |  8 ++-
>   arch/x86/kvm/cpuid.h               |  1 +
>   arch/x86/kvm/svm/sev.c             | 80 ++++++++++++++++++++++--------
>   arch/x86/kvm/svm/svm.c             | 57 +++++++++------------
>   arch/x86/kvm/svm/svm.h             |  9 +---
>   arch/x86/mm/mem_encrypt.c          | 12 ++---
>   arch/x86/mm/mem_encrypt_identity.c |  1 -
>   8 files changed, 97 insertions(+), 72 deletions(-)
> 

Queued except for patch 6, send that separately since it's purely x86 
and maintainers will likely not notice it inside this thread.  You 
probably want to avoid conflicts by waiting for the migration patches, 
though.

Paolo
Sean Christopherson April 22, 2021, 4:02 p.m. UTC | #2
On Thu, Apr 22, 2021, Paolo Bonzini wrote:
> > Paolo Bonzini (1):
> >    KVM: SEV: Mask CPUID[0x8000001F].eax according to supported features
> > 
> > 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: Disable SEV/SEV-ES if NPT is disabled
> >    KVM: SVM: Move SEV module params/variables to sev.c
> >    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: Enable SEV/SEV-ES functionality by default (when supported)
> >    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/mem_encrypt.h |  1 -
> >   arch/x86/kvm/cpuid.c               |  8 ++-
> >   arch/x86/kvm/cpuid.h               |  1 +
> >   arch/x86/kvm/svm/sev.c             | 80 ++++++++++++++++++++++--------
> >   arch/x86/kvm/svm/svm.c             | 57 +++++++++------------
> >   arch/x86/kvm/svm/svm.h             |  9 +---
> >   arch/x86/mm/mem_encrypt.c          | 12 ++---
> >   arch/x86/mm/mem_encrypt_identity.c |  1 -
> >   8 files changed, 97 insertions(+), 72 deletions(-)
> > 
> 
> Queued except for patch 6, send that separately since it's purely x86 and
> maintainers will likely not notice it inside this thread.  You probably want
> to avoid conflicts by waiting for the migration patches, though.

It can't be sent separately, having both the kernel's sev_enabled and KVM's
sev_enabled doesn't build with CONFIG_AMD_MEM_ENCRYPT=y:

arch/x86/kvm/svm/sev.c:33:13: error: static declaration of ‘sev_enabled’ follows non-static declaration
   33 | static bool sev_enabled = true;
      |             ^~~~~~~~~~~
In file included from include/linux/mem_encrypt.h:17,
                 from arch/x86/include/asm/page_types.h:7,
                 from arch/x86/include/asm/page.h:9,
                 from arch/x86/include/asm/thread_info.h:12,
                 from include/linux/thread_info.h:58,
                 from arch/x86/include/asm/preempt.h:7,
                 from include/linux/preempt.h:78,
                 from include/linux/percpu.h:6,
                 from include/linux/context_tracking_state.h:5,
                 from include/linux/hardirq.h:5,
                 from include/linux/kvm_host.h:7,
                 from arch/x86/kvm/svm/sev.c:11:
arch/x86/include/asm/mem_encrypt.h:23:13: note: previous declaration of ‘sev_enabled’ was here
   23 | extern bool sev_enabled;
      |             ^~~~~~~~~~~
make[3]: *** [scripts/Makefile.build:271: arch/x86/kvm/svm/sev.o] Error 1
Paolo Bonzini April 22, 2021, 5:08 p.m. UTC | #3
On 22/04/21 18:02, Sean Christopherson wrote:
> On Thu, Apr 22, 2021, Paolo Bonzini wrote:
>>> Paolo Bonzini (1):
>>>     KVM: SEV: Mask CPUID[0x8000001F].eax according to supported features
>>>
>>> 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: Disable SEV/SEV-ES if NPT is disabled
>>>     KVM: SVM: Move SEV module params/variables to sev.c
>>>     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: Enable SEV/SEV-ES functionality by default (when supported)
>>>     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/mem_encrypt.h |  1 -
>>>    arch/x86/kvm/cpuid.c               |  8 ++-
>>>    arch/x86/kvm/cpuid.h               |  1 +
>>>    arch/x86/kvm/svm/sev.c             | 80 ++++++++++++++++++++++--------
>>>    arch/x86/kvm/svm/svm.c             | 57 +++++++++------------
>>>    arch/x86/kvm/svm/svm.h             |  9 +---
>>>    arch/x86/mm/mem_encrypt.c          | 12 ++---
>>>    arch/x86/mm/mem_encrypt_identity.c |  1 -
>>>    8 files changed, 97 insertions(+), 72 deletions(-)
>>>
>>
>> Queued except for patch 6, send that separately since it's purely x86 and
>> maintainers will likely not notice it inside this thread.  You probably want
>> to avoid conflicts by waiting for the migration patches, though.
> 
> It can't be sent separately, having both the kernel's sev_enabled and KVM's
> sev_enabled doesn't build with CONFIG_AMD_MEM_ENCRYPT=y:

I discovered just as much a few hours later, but Boris has acked it 
already so it's all set.

Paolo

> arch/x86/kvm/svm/sev.c:33:13: error: static declaration of ‘sev_enabled’ follows non-static declaration
>     33 | static bool sev_enabled = true;
>        |             ^~~~~~~~~~~
> In file included from include/linux/mem_encrypt.h:17,
>                   from arch/x86/include/asm/page_types.h:7,
>                   from arch/x86/include/asm/page.h:9,
>                   from arch/x86/include/asm/thread_info.h:12,
>                   from include/linux/thread_info.h:58,
>                   from arch/x86/include/asm/preempt.h:7,
>                   from include/linux/preempt.h:78,
>                   from include/linux/percpu.h:6,
>                   from include/linux/context_tracking_state.h:5,
>                   from include/linux/hardirq.h:5,
>                   from include/linux/kvm_host.h:7,
>                   from arch/x86/kvm/svm/sev.c:11:
> arch/x86/include/asm/mem_encrypt.h:23:13: note: previous declaration of ‘sev_enabled’ was here
>     23 | extern bool sev_enabled;
>        |             ^~~~~~~~~~~
> make[3]: *** [scripts/Makefile.build:271: arch/x86/kvm/svm/sev.o] Error 1
>