[PATCH] KVM: X86: Add missing KVM_AMD dependency

[PATCH] KVM: X86: Add missing KVM_AMD dependency

[Guenter Roeck]

Building an image with KVM_AMD=y, CRYPTO_DEV_SP_PSP=y, and
CRYPTO_DEV_CCP_DD=m fails with the following error messages.

arch/x86/kvm/svm.c:6287: undefined reference to `sev_issue_cmd_external_user'
arch/x86/kvm/svm.o: In function `sev_unbind_asid':
arch/x86/kvm/svm.c:1747: undefined reference to `sev_guest_deactivate'
arch/x86/kvm/svm.c:1750: undefined reference to `sev_guest_df_flush'
arch/x86/kvm/svm.c:1759: undefined reference to `sev_guest_decommission'

Analysis shows that commit 59414c9892208 ("KVM: SVM: Add support for
KVM_SEV_LAUNCH_START command") added a dependency of KVM_AMD on
CRYPTO_DEV_CCP_DD if CRYPTO_DEV_SP_PSP is enabled: If CRYPTO_DEV_CCP_DD
is built as module, KVM_AMD must be built as module as well.

Fixes: 59414c9892208 ("KVM: SVM: Add support for KVM_SEV_LAUNCH_START command")
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Borislav Petkov <bp@suse.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/x86/kvm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index 1bbec387d289..a8eff6910ea3 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -74,6 +74,7 @@ config KVM_INTEL
 config KVM_AMD
 	tristate "KVM for AMD processors support"
 	depends on KVM
+	depends on !CRYPTO_DEV_SP_PSP || (CRYPTO_DEV_SP_PSP && CRYPTO_DEV_CCP_DD)
 	---help---
 	  Provides support for KVM on AMD processors equipped with the AMD-V
 	  (SVM) extensions.
-- 
2.7.4

Re: [PATCH] KVM: X86: Add missing KVM_AMD dependency

[Paolo Bonzini]

On 05/10/2018 20:46, Guenter Roeck wrote:
> Analysis shows that commit 59414c9892208 ("KVM: SVM: Add support for
> KVM_SEV_LAUNCH_START command") added a dependency of KVM_AMD on
> CRYPTO_DEV_CCP_DD if CRYPTO_DEV_SP_PSP is enabled: If CRYPTO_DEV_CCP_DD
> is built as module, KVM_AMD must be built as module as well.
> 
> Fixes: 59414c9892208 ("KVM: SVM: Add support for KVM_SEV_LAUNCH_START command")
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Borislav Petkov <bp@suse.de>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

This should be handled by

config KVM_AMD_SEV
        def_bool y
        bool "AMD Secure Encrypted Virtualization (SEV) support"
        depends on KVM_AMD && X86_64
        depends on CRYPTO_DEV_SP_PSP && !(KVM_AMD=y && CRYPTO_DEV_CCP_DD=m)
        ---help---
        Provides support for launching Encrypted VMs on AMD processors.

Maybe this works as well?  I haven't tested it yet:

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 89c4c5aa15f1..55f10b17d044 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -441,9 +441,13 @@ static inline bool svm_sev_enabled(void)

 static inline bool sev_guest(struct kvm *kvm)
 {
+#ifdef CONFIG_KVM_AMD_SEV
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;

 	return sev->active;
+#else
+	return false;
+#endif
 }

 static inline int sev_get_asid(struct kvm *kvm)

Thanks,

Paolo

Re: [PATCH] KVM: X86: Add missing KVM_AMD dependency

[Guenter Roeck]

On Fri, Oct 05, 2018 at 10:41:55PM +0200, Paolo Bonzini wrote:
> On 05/10/2018 20:46, Guenter Roeck wrote:
> > Analysis shows that commit 59414c9892208 ("KVM: SVM: Add support for
> > KVM_SEV_LAUNCH_START command") added a dependency of KVM_AMD on
> > CRYPTO_DEV_CCP_DD if CRYPTO_DEV_SP_PSP is enabled: If CRYPTO_DEV_CCP_DD
> > is built as module, KVM_AMD must be built as module as well.
> > 
> > Fixes: 59414c9892208 ("KVM: SVM: Add support for KVM_SEV_LAUNCH_START command")
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> This should be handled by
> 
> config KVM_AMD_SEV
>         def_bool y
>         bool "AMD Secure Encrypted Virtualization (SEV) support"
>         depends on KVM_AMD && X86_64
>         depends on CRYPTO_DEV_SP_PSP && !(KVM_AMD=y && CRYPTO_DEV_CCP_DD=m)
>         ---help---
>         Provides support for launching Encrypted VMs on AMD processors.
> 

Unfortunately it doesn't. It disables KVM_AMD_SEV, but that doesn't prevent
the calls.

> Maybe this works as well?  I haven't tested it yet:
> 

I am sure there are many possible solutions. I would personally prefer one
that enforces KVM_AMD=m with CRYPTO_DEV_CCP_DD=m, but that is just me.

Thanks,
Guenter

> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 89c4c5aa15f1..55f10b17d044 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -441,9 +441,13 @@ static inline bool svm_sev_enabled(void)
> 
>  static inline bool sev_guest(struct kvm *kvm)
>  {
> +#ifdef CONFIG_KVM_AMD_SEV
>  	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> 
>  	return sev->active;
> +#else
> +	return false;
> +#endif
>  }
> 
>  static inline int sev_get_asid(struct kvm *kvm)
> 
> Thanks,
> 
> Paolo

Re: [PATCH] KVM: X86: Add missing KVM_AMD dependency

[Paolo Bonzini]

On 06/10/2018 00:03, Guenter Roeck wrote:
>> This should be handled by
>>
>> config KVM_AMD_SEV
>>         def_bool y
>>         bool "AMD Secure Encrypted Virtualization (SEV) support"
>>         depends on KVM_AMD && X86_64
>>         depends on CRYPTO_DEV_SP_PSP && !(KVM_AMD=y && CRYPTO_DEV_CCP_DD=m)
>>         ---help---
>>         Provides support for launching Encrypted VMs on AMD processors.
>>
> Unfortunately it doesn't. It disables KVM_AMD_SEV, but that doesn't prevent
> the calls.

Yes, exactly - that's why I mentioned the sev_guest patch that should
cull all the SEV code from a !KVM_AMD_SEV build.

>> Maybe this works as well?  I haven't tested it yet:
>>
> I am sure there are many possible solutions. I would personally prefer one
> that enforces KVM_AMD=m with CRYPTO_DEV_CCP_DD=m, but that is just me.

Well, KVM_AMD=y is a relatively unusual choice to begin with.  The
question is whether then you want to disable this choice completely when
CRYPTO_DEV_CCP_DD=m, or just disable SEV.

My patch is a good idea anyway, if I may say so :), because it culls a
lot of code from a !KVM_AMD_SEV build.  But if it is not enough, we
certainly have to do something else about the failure you're reporting.

Paolo

Re: [PATCH] KVM: X86: Add missing KVM_AMD dependency

[Guenter Roeck]

Hi Paolo,

On 10/05/2018 03:18 PM, Paolo Bonzini wrote:
> On 06/10/2018 00:03, Guenter Roeck wrote:
>>> This should be handled by
>>>
>>> config KVM_AMD_SEV
>>>          def_bool y
>>>          bool "AMD Secure Encrypted Virtualization (SEV) support"
>>>          depends on KVM_AMD && X86_64
>>>          depends on CRYPTO_DEV_SP_PSP && !(KVM_AMD=y && CRYPTO_DEV_CCP_DD=m)
>>>          ---help---
>>>          Provides support for launching Encrypted VMs on AMD processors.
>>>
>> Unfortunately it doesn't. It disables KVM_AMD_SEV, but that doesn't prevent
>> the calls.
> 
> Yes, exactly - that's why I mentioned the sev_guest patch that should
> cull all the SEV code from a !KVM_AMD_SEV build.
> 
>>> Maybe this works as well?  I haven't tested it yet:
>>>
>> I am sure there are many possible solutions. I would personally prefer one
>> that enforces KVM_AMD=m with CRYPTO_DEV_CCP_DD=m, but that is just me.
> 
> Well, KVM_AMD=y is a relatively unusual choice to begin with.  The

It is common enough that we are not the only ones affected. Also, even a
"relatively unusual choice" should, in my opinion, not result in a build
error. Never mind, I'll just apply the suggested workaround and configure
CRYPTO_DEV_CCP_DD=y. I need to do that anyway, after all, if I want to keep
KVM_AMD=y.

Thanks,
Guenter

Re: [PATCH] KVM: X86: Add missing KVM_AMD dependency

[Paolo Bonzini]

On 06/10/2018 22:43, Guenter Roeck wrote:
>>
>>>> Maybe this works as well?  I haven't tested it yet:
>>>>
>>> I am sure there are many possible solutions. I would personally
>>> prefer one
>>> that enforces KVM_AMD=m with CRYPTO_DEV_CCP_DD=m, but that is just me.
>>
>> Well, KVM_AMD=y is a relatively unusual choice to begin with.  The
> 
> It is common enough that we are not the only ones affected. Also, even a
> "relatively unusual choice" should, in my opinion, not result in a build
> error.

Of course not!  The question is whether to solve it by disabling
KVM_AMD_SEV (which is what the current code attempts to do, and my patch
should fix that) or KVM_AMD (your patch).

Paolo

> Never mind, I'll just apply the suggested workaround and configure
> CRYPTO_DEV_CCP_DD=y. I need to do that anyway, after all, if I want to keep
> KVM_AMD=y.

Re: [PATCH] KVM: X86: Add missing KVM_AMD dependency

[Singh, Brijesh]

On 10/08/2018 06:27 AM, Paolo Bonzini wrote:
> On 06/10/2018 22:43, Guenter Roeck wrote:
>>>
>>>>> Maybe this works as well?  I haven't tested it yet:
>>>>>
>>>> I am sure there are many possible solutions. I would personally
>>>> prefer one
>>>> that enforces KVM_AMD=m with CRYPTO_DEV_CCP_DD=m, but that is just me.
>>>
>>> Well, KVM_AMD=y is a relatively unusual choice to begin with.  The
>>
>> It is common enough that we are not the only ones affected. Also, even a
>> "relatively unusual choice" should, in my opinion, not result in a build
>> error.
> 
> Of course not!  The question is whether to solve it by disabling
> KVM_AMD_SEV (which is what the current code attempts to do, and my patch
> should fix that) or KVM_AMD (your patch).


IMHO, Paolo's patch make sense; it removes all the SEV specific code
when KVM_AMD_SEV=n. It saves ~4K in text section.

Paolo,

Does it make sense to move all the SEV specific code in svm-sev.c ?
I am looking to add SEV migration support very soon, and can see
myself adding more SEV command handling which will grow svm.c further.

-Brijesh

Re: [PATCH] KVM: X86: Add missing KVM_AMD dependency

[Borislav Petkov]

On Mon, Oct 08, 2018 at 02:52:46PM +0000, Singh, Brijesh wrote:
> Does it make sense to move all the SEV specific code in svm-sev.c ?
> I am looking to add SEV migration support very soon, and can see
> myself adding more SEV command handling which will grow svm.c further.

Amen to that - svm.c is pretty huge already. And if you end up moving
facilities, you can simply call it sev.c

Just my 2¢.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH] KVM: X86: Add missing KVM_AMD dependency

[Paolo Bonzini]

On 08/10/2018 19:32, Borislav Petkov wrote:
> On Mon, Oct 08, 2018 at 02:52:46PM +0000, Singh, Brijesh wrote:
>> Does it make sense to move all the SEV specific code in svm-sev.c ?
>> I am looking to add SEV migration support very soon, and can see
>> myself adding more SEV command handling which will grow svm.c further.
> 
> Amen to that - svm.c is pretty huge already. And if you end up moving
> facilities, you can simply call it sev.c

Creating arch/x86/kvm/{mmu,vmx,svm}/ has been on my todo list for a
while.  If you would like to split the SEV bits, that would be a good start.

Paolo