[PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[Janakarajan Natarajan]

Use Kconfig imply 'option' when specifying SEV CRYPTO dependencies.

Example configuration:
.
.
CONFIG_CRYPTO_DEV_CCP=y
CONFIG_CRYPTO_DEV_CCP_DD=m
CONFIG_CRYPTO_DEV_SP_CCP=y
CONFIG_CRYPTO_DEV_CCP_CRYPTO=m
CONFIG_CRYPTO_DEV_SP_PSP=y
.
.
CONFIG_KVM_AMD=y
CONFIG_KVM_AMD_SEV=y
.
.

When the CRYPTO_DEV_SP_DD is m, KVM_AMD set to y produces compile time
errors.

Since KVM_AMD_SEV depends on KVM_AMD and CRYPTO_DEV_CCP_DD, the
issue can be prevented by using 'imply' Kconfig option when specifying
the CRYPTO dependencies.

Fixes: 505c9e94d832 ("KVM: x86: prefer "depends on" to "select" for SEV")

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 arch/x86/kvm/Kconfig | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index 92fd433..d9b16b7 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -85,7 +85,9 @@ config KVM_AMD_SEV
 	def_bool y
 	bool "AMD Secure Encrypted Virtualization (SEV) support"
 	depends on KVM_AMD && X86_64
-	depends on CRYPTO_DEV_CCP && CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP
+	imply CRYPTO_DEV_CCP
+	imply CRYPTO_DEV_CCP_DD
+	imply CRYPTO_DEV_SP_PSP
 	---help---
 	Provides support for launching Encrypted VMs on AMD processors.
 
-- 
2.7.4

Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[Borislav Petkov]

+ Tom and Brijesh.

On Mon, May 21, 2018 at 10:12:53AM -0500, Janakarajan Natarajan wrote:
> Use Kconfig imply 'option' when specifying SEV CRYPTO dependencies.
> 
> Example configuration:
> .
> .
> CONFIG_CRYPTO_DEV_CCP=y
> CONFIG_CRYPTO_DEV_CCP_DD=m
> CONFIG_CRYPTO_DEV_SP_CCP=y
> CONFIG_CRYPTO_DEV_CCP_CRYPTO=m
> CONFIG_CRYPTO_DEV_SP_PSP=y
> .
> .
> CONFIG_KVM_AMD=y
> CONFIG_KVM_AMD_SEV=y
> .
> .
> 
> When the CRYPTO_DEV_SP_DD is m, KVM_AMD set to y produces compile time
> errors.
> 
> Since KVM_AMD_SEV depends on KVM_AMD and CRYPTO_DEV_CCP_DD, the
> issue can be prevented by using 'imply' Kconfig option when specifying
> the CRYPTO dependencies.
> 
> Fixes: 505c9e94d832 ("KVM: x86: prefer "depends on" to "select" for SEV")
> 
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
> ---
>  arch/x86/kvm/Kconfig | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index 92fd433..d9b16b7 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -85,7 +85,9 @@ config KVM_AMD_SEV
>  	def_bool y
>  	bool "AMD Secure Encrypted Virtualization (SEV) support"
>  	depends on KVM_AMD && X86_64
> -	depends on CRYPTO_DEV_CCP && CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP
> +	imply CRYPTO_DEV_CCP
> +	imply CRYPTO_DEV_CCP_DD
> +	imply CRYPTO_DEV_SP_PSP
>  	---help---
>  	Provides support for launching Encrypted VMs on AMD processors.

Well, this solves the build issue but I just created a config:

$ grep -E "(KVM|PSP)" .config | grep -v '#'
CONFIG_HAVE_KVM=y
CONFIG_HAVE_KVM_IRQCHIP=y
CONFIG_HAVE_KVM_IRQFD=y
CONFIG_HAVE_KVM_IRQ_ROUTING=y
CONFIG_HAVE_KVM_EVENTFD=y
CONFIG_KVM_MMIO=y
CONFIG_KVM_ASYNC_PF=y
CONFIG_HAVE_KVM_MSI=y
CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT=y
CONFIG_KVM_VFIO=y
CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT=y
CONFIG_KVM_COMPAT=y
CONFIG_HAVE_KVM_IRQ_BYPASS=y
CONFIG_KVM=y
CONFIG_KVM_AMD=m

which builds but the PSP functionality is not there. And I don't think
this is serving the user: she should be able to select what she wants
and get the required functionality added and not have build errors with
any possible configuration.

Now, looking at the security processor Kconfig stuff, it is somewhat
confusing but maybe I didn't look at it long enough. A couple of points:

config CRYPTO_DEV_CCP_DD
        tristate "Secure Processor device driver"

If this is going to be the top-level menu item for the SP, call that

 CRYPTO_DEV_SP

to mean, this is the security processor. CCP_DD is confusing because you
have CRYPTO_DEV_SP_CCP which is the crypto coprocessor support.

And "DD" for device driver is a pure tautology - most of the Kconfig
items are device drivers :)

Then,

 config KVM_AMD_SEV
        def_bool y
        bool "AMD Secure Encrypted Virtualization (SEV) support"
        depends on KVM_AMD && X86_64
        depends on CRYPTO_DEV_CCP && CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP

that last line is pulling the required functionality for SEV but - and
I *think* we have talked about this before - having a hierarchical
dependency should make this a lot clearer and fix the build issues along
the way.

Because, IMHO, KVM_AMD_SEV should depend only on CRYPTO_DEV_SP_PSP -
i.e., the PSP device because SEV needs the PSP, right?

Now, the PSP device *itself* should depend on whatever it needs to
function properly, CRYPTO_DEV_CCP_DD I guess.

But SEV should not depend on CRYPTO_DEV_CCP - which is the top-level
Kconfig item - that should be expressed implicitly through the
dependency chain where PSP ends up depending on it.

So that you have one-hop deps:

KVM_SEV -> PSP -> CCP -> ...

IOW, a config item, say PSP - if enabled - should make sure it
selects/depends on everything it needs to function. The upper level
item KVM_SEV - which selects/depends on that config item shouldn't
be responsible for making sure the correct items for PSP's proper
functioning are enabled - that's PSP's item's job.

Makes sense?

Maybe I'm missing something but applying this simple logic would prevent
such Kconfig build issues and make the whole dependency chain almost
trivial.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[Richard Weinberger]

On Wed, May 23, 2018 at 4:46 PM, Borislav Petkov <bp@suse.de> wrote:
> + Tom and Brijesh.
>
> On Mon, May 21, 2018 at 10:12:53AM -0500, Janakarajan Natarajan wrote:
>> Use Kconfig imply 'option' when specifying SEV CRYPTO dependencies.
>>
>> Example configuration:
>> .
>> .
>> CONFIG_CRYPTO_DEV_CCP=y
>> CONFIG_CRYPTO_DEV_CCP_DD=m
>> CONFIG_CRYPTO_DEV_SP_CCP=y
>> CONFIG_CRYPTO_DEV_CCP_CRYPTO=m
>> CONFIG_CRYPTO_DEV_SP_PSP=y
>> .
>> .
>> CONFIG_KVM_AMD=y
>> CONFIG_KVM_AMD_SEV=y
>> .
>> .
>>
>> When the CRYPTO_DEV_SP_DD is m, KVM_AMD set to y produces compile time
>> errors.
>>
>> Since KVM_AMD_SEV depends on KVM_AMD and CRYPTO_DEV_CCP_DD, the
>> issue can be prevented by using 'imply' Kconfig option when specifying
>> the CRYPTO dependencies.
>>
>> Fixes: 505c9e94d832 ("KVM: x86: prefer "depends on" to "select" for SEV")
>>
>> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
>> ---
>>  arch/x86/kvm/Kconfig | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
>> index 92fd433..d9b16b7 100644
>> --- a/arch/x86/kvm/Kconfig
>> +++ b/arch/x86/kvm/Kconfig
>> @@ -85,7 +85,9 @@ config KVM_AMD_SEV
>>       def_bool y
>>       bool "AMD Secure Encrypted Virtualization (SEV) support"
>>       depends on KVM_AMD && X86_64
>> -     depends on CRYPTO_DEV_CCP && CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP
>> +     imply CRYPTO_DEV_CCP
>> +     imply CRYPTO_DEV_CCP_DD
>> +     imply CRYPTO_DEV_SP_PSP
>>       ---help---
>>       Provides support for launching Encrypted VMs on AMD processors.
>
> Well, this solves the build issue but I just created a config:
>
> $ grep -E "(KVM|PSP)" .config | grep -v '#'
> CONFIG_HAVE_KVM=y
> CONFIG_HAVE_KVM_IRQCHIP=y
> CONFIG_HAVE_KVM_IRQFD=y
> CONFIG_HAVE_KVM_IRQ_ROUTING=y
> CONFIG_HAVE_KVM_EVENTFD=y
> CONFIG_KVM_MMIO=y
> CONFIG_KVM_ASYNC_PF=y
> CONFIG_HAVE_KVM_MSI=y
> CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT=y
> CONFIG_KVM_VFIO=y
> CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT=y
> CONFIG_KVM_COMPAT=y
> CONFIG_HAVE_KVM_IRQ_BYPASS=y
> CONFIG_KVM=y
> CONFIG_KVM_AMD=m
>
> which builds but the PSP functionality is not there. And I don't think
> this is serving the user: she should be able to select what she wants
> and get the required functionality added and not have build errors with
> any possible configuration.
>
> Now, looking at the security processor Kconfig stuff, it is somewhat
> confusing but maybe I didn't look at it long enough. A couple of points:
>
> config CRYPTO_DEV_CCP_DD
>         tristate "Secure Processor device driver"
>
> If this is going to be the top-level menu item for the SP, call that
>
>  CRYPTO_DEV_SP
>
> to mean, this is the security processor. CCP_DD is confusing because you
> have CRYPTO_DEV_SP_CCP which is the crypto coprocessor support.
>
> And "DD" for device driver is a pure tautology - most of the Kconfig
> items are device drivers :)
>
> Then,
>
>  config KVM_AMD_SEV
>         def_bool y
>         bool "AMD Secure Encrypted Virtualization (SEV) support"
>         depends on KVM_AMD && X86_64
>         depends on CRYPTO_DEV_CCP && CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP
>
> that last line is pulling the required functionality for SEV but - and
> I *think* we have talked about this before - having a hierarchical
> dependency should make this a lot clearer and fix the build issues along
> the way.
>
> Because, IMHO, KVM_AMD_SEV should depend only on CRYPTO_DEV_SP_PSP -
> i.e., the PSP device because SEV needs the PSP, right?
>
> Now, the PSP device *itself* should depend on whatever it needs to
> function properly, CRYPTO_DEV_CCP_DD I guess.
>
> But SEV should not depend on CRYPTO_DEV_CCP - which is the top-level
> Kconfig item - that should be expressed implicitly through the
> dependency chain where PSP ends up depending on it.
>
> So that you have one-hop deps:
>
> KVM_SEV -> PSP -> CCP -> ...
>
> IOW, a config item, say PSP - if enabled - should make sure it
> selects/depends on everything it needs to function. The upper level
> item KVM_SEV - which selects/depends on that config item shouldn't
> be responsible for making sure the correct items for PSP's proper
> functioning are enabled - that's PSP's item's job.
>
> Makes sense?
>
> Maybe I'm missing something but applying this simple logic would prevent
> such Kconfig build issues and make the whole dependency chain almost
> trivial.
>
> Thx.

*kind ping*

Felix just reported me that build failure too.

-- 
Thanks,
//richard

Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[Brijesh Singh]

On 6/14/18 2:58 AM, Richard Weinberger wrote:
> On Wed, May 23, 2018 at 4:46 PM, Borislav Petkov <bp@suse.de> wrote:
>> + Tom and Brijesh.
>>
>> On Mon, May 21, 2018 at 10:12:53AM -0500, Janakarajan Natarajan wrote:
>>> Use Kconfig imply 'option' when specifying SEV CRYPTO dependencies.
>>>
>>> Example configuration:
>>> .
>>> .
>>> CONFIG_CRYPTO_DEV_CCP=y
>>> CONFIG_CRYPTO_DEV_CCP_DD=m
>>> CONFIG_CRYPTO_DEV_SP_CCP=y
>>> CONFIG_CRYPTO_DEV_CCP_CRYPTO=m
>>> CONFIG_CRYPTO_DEV_SP_PSP=y
>>> .
>>> .
>>> CONFIG_KVM_AMD=y
>>> CONFIG_KVM_AMD_SEV=y
>>> .
>>> .
>>>
>>> When the CRYPTO_DEV_SP_DD is m, KVM_AMD set to y produces compile time
>>> errors.
>>>
>>> Since KVM_AMD_SEV depends on KVM_AMD and CRYPTO_DEV_CCP_DD, the
>>> issue can be prevented by using 'imply' Kconfig option when specifying
>>> the CRYPTO dependencies.
>>>
>>> Fixes: 505c9e94d832 ("KVM: x86: prefer "depends on" to "select" for SEV")
>>>
>>> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
>>> ---
>>>  arch/x86/kvm/Kconfig | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
>>> index 92fd433..d9b16b7 100644
>>> --- a/arch/x86/kvm/Kconfig
>>> +++ b/arch/x86/kvm/Kconfig
>>> @@ -85,7 +85,9 @@ config KVM_AMD_SEV
>>>       def_bool y
>>>       bool "AMD Secure Encrypted Virtualization (SEV) support"
>>>       depends on KVM_AMD && X86_64
>>> -     depends on CRYPTO_DEV_CCP && CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP
>>> +     imply CRYPTO_DEV_CCP
>>> +     imply CRYPTO_DEV_CCP_DD
>>> +     imply CRYPTO_DEV_SP_PSP
>>>       ---help---
>>>       Provides support for launching Encrypted VMs on AMD processors.
>> Well, this solves the build issue but I just created a config:
>>
>> $ grep -E "(KVM|PSP)" .config | grep -v '#'
>> CONFIG_HAVE_KVM=y
>> CONFIG_HAVE_KVM_IRQCHIP=y
>> CONFIG_HAVE_KVM_IRQFD=y
>> CONFIG_HAVE_KVM_IRQ_ROUTING=y
>> CONFIG_HAVE_KVM_EVENTFD=y
>> CONFIG_KVM_MMIO=y
>> CONFIG_KVM_ASYNC_PF=y
>> CONFIG_HAVE_KVM_MSI=y
>> CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT=y
>> CONFIG_KVM_VFIO=y
>> CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT=y
>> CONFIG_KVM_COMPAT=y
>> CONFIG_HAVE_KVM_IRQ_BYPASS=y
>> CONFIG_KVM=y
>> CONFIG_KVM_AMD=m
>>
>> which builds but the PSP functionality is not there. And I don't think
>> this is serving the user: she should be able to select what she wants
>> and get the required functionality added and not have build errors with
>> any possible configuration.

Yes, I agree.

>>
>> Now, looking at the security processor Kconfig stuff, it is somewhat
>> confusing but maybe I didn't look at it long enough. A couple of points:
>>
>> config CRYPTO_DEV_CCP_DD
>>         tristate "Secure Processor device driver"
>>
>> If this is going to be the top-level menu item for the SP, call that
>>
>>  CRYPTO_DEV_SP
>>
>> to mean, this is the security processor. CCP_DD is confusing because you
>> have CRYPTO_DEV_SP_CCP which is the crypto coprocessor support.

IIRC, the patch series which added this support started with naming it
to CRYPTO_DEV_SP but somewhere during review process we discussed that
since the module name is ccp.ko hence we kept the config with same name.
We can submit a follow-up patch to correct it to avoid any further
confusion.


>> And "DD" for device driver is a pure tautology - most of the Kconfig
>> items are device drivers :)
>>
>> Then,
>>
>>  config KVM_AMD_SEV
>>         def_bool y
>>         bool "AMD Secure Encrypted Virtualization (SEV) support"
>>         depends on KVM_AMD && X86_64
>>         depends on CRYPTO_DEV_CCP && CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP
>>
>> that last line is pulling the required functionality for SEV but - and
>> I *think* we have talked about this before - having a hierarchical
>> dependency should make this a lot clearer and fix the build issues along
>> the way.

The first set of SEV patches accepted in upstream was using select
instead of depends on. I used select mainly to ensure that all the
drivers needed to run SEV is either builtin or module.  A follow up
patch was submitted by Paolo to use 'depends on' so that we don't create
a circular dependency - I agree with that patch.

>> Because, IMHO, KVM_AMD_SEV should depend only on CRYPTO_DEV_SP_PSP -
>> i.e., the PSP device because SEV needs the PSP, right?

I think depends should look like this:

config KVM_AMD_SEV
    def_bool y
    bool "AMD Secure Encrypted Virtualization (SEV) support"
    depends KVM_AMD && X86_64
    depends CRYPTO_DEV_SP_PSP && !(KVM_AMD=y && CRYPTO_DEV_CCP_DD=m)


Like you said,  the KVM_AMD_SEV should depends only on
"CRYPTO_DEV_SP_PSP".  But when SEV support is enabled in KVM, we need a
CCP driver at the runtime, hence we should ensure that if user selects
KVM_AMD=y then CCP driver is also builtin otherwise she will not get SEV
feature.


>> Now, the PSP device *itself* should depend on whatever it needs to
>> function properly, CRYPTO_DEV_CCP_DD I guess.
>>
>> But SEV should not depend on CRYPTO_DEV_CCP - which is the top-level
>> Kconfig item - that should be expressed implicitly through the
>> dependency chain where PSP ends up depending on it.
>>
>> So that you have one-hop deps:
>>
>> KVM_SEV -> PSP -> CCP -> ...
>>
>> IOW, a config item, say PSP - if enabled - should make sure it
>> selects/depends on everything it needs to function. The upper level
>> item KVM_SEV - which selects/depends on that config item shouldn't
>> be responsible for making sure the correct items for PSP's proper
>> functioning are enabled - that's PSP's item's job.
>>
>> Makes sense?
>>
>> Maybe I'm missing something but applying this simple logic would prevent
>> such Kconfig build issues and make the whole dependency chain almost
>> trivial.
>>
>> Thx.
> *kind ping*

Thanks for ping, sorry I was meaning to reply to this it but somehow got
dropped. Can you please try above recommendation and see if its builds
and runs?


> Felix just reported me that build failure too.
>

[PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[Felix Niederwanger]

[-- Attachment #1.1: Type: text/plain, Size: 6768 bytes --]

On 14/06/18 14:08, Brijesh Singh wrote:
> 
> 
> On 6/14/18 2:58 AM, Richard Weinberger wrote:
>> On Wed, May 23, 2018 at 4:46 PM, Borislav Petkov <bp@suse.de> wrote:
>>> + Tom and Brijesh.
>>>
>>> On Mon, May 21, 2018 at 10:12:53AM -0500, Janakarajan Natarajan wrote:
>>>> Use Kconfig imply 'option' when specifying SEV CRYPTO dependencies.
>>>>
>>>> Example configuration:
>>>> .
>>>> .
>>>> CONFIG_CRYPTO_DEV_CCP=y
>>>> CONFIG_CRYPTO_DEV_CCP_DD=m
>>>> CONFIG_CRYPTO_DEV_SP_CCP=y
>>>> CONFIG_CRYPTO_DEV_CCP_CRYPTO=m
>>>> CONFIG_CRYPTO_DEV_SP_PSP=y
>>>> .
>>>> .
>>>> CONFIG_KVM_AMD=y
>>>> CONFIG_KVM_AMD_SEV=y
>>>> .
>>>> .
>>>>
>>>> When the CRYPTO_DEV_SP_DD is m, KVM_AMD set to y produces compile time
>>>> errors.
>>>>
>>>> Since KVM_AMD_SEV depends on KVM_AMD and CRYPTO_DEV_CCP_DD, the
>>>> issue can be prevented by using 'imply' Kconfig option when specifying
>>>> the CRYPTO dependencies.
>>>>
>>>> Fixes: 505c9e94d832 ("KVM: x86: prefer "depends on" to "select" for SEV")
>>>>
>>>> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
>>>> ---
>>>>  arch/x86/kvm/Kconfig | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
>>>> index 92fd433..d9b16b7 100644
>>>> --- a/arch/x86/kvm/Kconfig
>>>> +++ b/arch/x86/kvm/Kconfig
>>>> @@ -85,7 +85,9 @@ config KVM_AMD_SEV
>>>>       def_bool y
>>>>       bool "AMD Secure Encrypted Virtualization (SEV) support"
>>>>       depends on KVM_AMD && X86_64
>>>> -     depends on CRYPTO_DEV_CCP && CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP
>>>> +     imply CRYPTO_DEV_CCP
>>>> +     imply CRYPTO_DEV_CCP_DD
>>>> +     imply CRYPTO_DEV_SP_PSP
>>>>       ---help---
>>>>       Provides support for launching Encrypted VMs on AMD processors.
>>> Well, this solves the build issue but I just created a config:
>>>
>>> $ grep -E "(KVM|PSP)" .config | grep -v '#'
>>> CONFIG_HAVE_KVM=y
>>> CONFIG_HAVE_KVM_IRQCHIP=y
>>> CONFIG_HAVE_KVM_IRQFD=y
>>> CONFIG_HAVE_KVM_IRQ_ROUTING=y
>>> CONFIG_HAVE_KVM_EVENTFD=y
>>> CONFIG_KVM_MMIO=y
>>> CONFIG_KVM_ASYNC_PF=y
>>> CONFIG_HAVE_KVM_MSI=y
>>> CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT=y
>>> CONFIG_KVM_VFIO=y
>>> CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT=y
>>> CONFIG_KVM_COMPAT=y
>>> CONFIG_HAVE_KVM_IRQ_BYPASS=y
>>> CONFIG_KVM=y
>>> CONFIG_KVM_AMD=m
>>>
>>> which builds but the PSP functionality is not there. And I don't think
>>> this is serving the user: she should be able to select what she wants
>>> and get the required functionality added and not have build errors with
>>> any possible configuration.
> 
> Yes, I agree.
> 
>>>
>>> Now, looking at the security processor Kconfig stuff, it is somewhat
>>> confusing but maybe I didn't look at it long enough. A couple of points:
>>>
>>> config CRYPTO_DEV_CCP_DD
>>>         tristate "Secure Processor device driver"
>>>
>>> If this is going to be the top-level menu item for the SP, call that
>>>
>>>  CRYPTO_DEV_SP
>>>
>>> to mean, this is the security processor. CCP_DD is confusing because you
>>> have CRYPTO_DEV_SP_CCP which is the crypto coprocessor support.
> 
> IIRC, the patch series which added this support started with naming it
> to CRYPTO_DEV_SP but somewhere during review process we discussed that
> since the module name is ccp.ko hence we kept the config with same name.
> We can submit a follow-up patch to correct it to avoid any further
> confusion.
> 
> 
>>> And "DD" for device driver is a pure tautology - most of the Kconfig
>>> items are device drivers :)
>>>
>>> Then,
>>>
>>>  config KVM_AMD_SEV
>>>         def_bool y
>>>         bool "AMD Secure Encrypted Virtualization (SEV) support"
>>>         depends on KVM_AMD && X86_64
>>>         depends on CRYPTO_DEV_CCP && CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP
>>>
>>> that last line is pulling the required functionality for SEV but - and
>>> I *think* we have talked about this before - having a hierarchical
>>> dependency should make this a lot clearer and fix the build issues along
>>> the way.
> 
> The first set of SEV patches accepted in upstream was using select
> instead of depends on. I used select mainly to ensure that all the
> drivers needed to run SEV is either builtin or module.  A follow up
> patch was submitted by Paolo to use 'depends on' so that we don't create
> a circular dependency - I agree with that patch.
> 
>>> Because, IMHO, KVM_AMD_SEV should depend only on CRYPTO_DEV_SP_PSP -
>>> i.e., the PSP device because SEV needs the PSP, right?
> 
> I think depends should look like this:
> 
> config KVM_AMD_SEV
>     def_bool y
>     bool "AMD Secure Encrypted Virtualization (SEV) support"
>     depends KVM_AMD && X86_64
>     depends CRYPTO_DEV_SP_PSP && !(KVM_AMD=y && CRYPTO_DEV_CCP_DD=m)
> 
> 
> Like you said,  the KVM_AMD_SEV should depends only on
> "CRYPTO_DEV_SP_PSP".  But when SEV support is enabled in KVM, we need a
> CCP driver at the runtime, hence we should ensure that if user selects
> KVM_AMD=y then CCP driver is also builtin otherwise she will not get SEV
> feature.
> 
> 
>>> Now, the PSP device *itself* should depend on whatever it needs to
>>> function properly, CRYPTO_DEV_CCP_DD I guess.
>>>
>>> But SEV should not depend on CRYPTO_DEV_CCP - which is the top-level
>>> Kconfig item - that should be expressed implicitly through the
>>> dependency chain where PSP ends up depending on it.
>>>
>>> So that you have one-hop deps:
>>>
>>> KVM_SEV -> PSP -> CCP -> ...
>>>
>>> IOW, a config item, say PSP - if enabled - should make sure it
>>> selects/depends on everything it needs to function. The upper level
>>> item KVM_SEV - which selects/depends on that config item shouldn't
>>> be responsible for making sure the correct items for PSP's proper
>>> functioning are enabled - that's PSP's item's job.
>>>
>>> Makes sense?
>>>
>>> Maybe I'm missing something but applying this simple logic would prevent
>>> such Kconfig build issues and make the whole dependency chain almost
>>> trivial.
>>>
>>> Thx.
>> *kind ping*
> 
> Thanks for ping, sorry I was meaning to reply to this it but somehow got
> dropped. Can you please try above recommendation and see if its builds
> and runs?
> 

Hi Brijesh and thank you for your reply,

yes, I could solve the issue by setting CONFIG_CRYPTO_DEV_CCP=y

Because at some point I got quiet desperate for not finding anything
useful on google, I wrote a small blog post - hopefully others with
similar problems can use the workaround as well.

Thank for your advice and have a nice day,
Greets,
Felix

> 
>> Felix just reported me that build failure too.
>>
> 




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[Borislav Petkov]

On Thu, Jun 14, 2018 at 07:08:26AM -0500, Brijesh Singh wrote:
> I think depends should look like this:
> 
> config KVM_AMD_SEV
>     def_bool y
>     bool "AMD Secure Encrypted Virtualization (SEV) support"
>     depends KVM_AMD && X86_64
>     depends CRYPTO_DEV_SP_PSP && !(KVM_AMD=y && CRYPTO_DEV_CCP_DD=m)

CRYPTO_DEV_CCP_DD - this still doesn't belong here. KVM_AMD_SEV doesn't
care about what kind of functionality the PSP needs to function and
select it - it should only depend on a single symbol.

Now, you can add a separate CRYPTO .config item which collects all
required functionality for SEV guests and have KVM_AMD_SEV depend
on that. Or a similar solution. But not have KVM_AMD_SEV poke into
CRYPTO...PSP dependencies because it doesn't need to know about it.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[Brijesh Singh]

Hi Boris,


On 06/19/2018 04:46 AM, Borislav Petkov wrote:
> On Thu, Jun 14, 2018 at 07:08:26AM -0500, Brijesh Singh wrote:
>> I think depends should look like this:
>>
>> config KVM_AMD_SEV
>>      def_bool y
>>      bool "AMD Secure Encrypted Virtualization (SEV) support"
>>      depends KVM_AMD && X86_64
>>      depends CRYPTO_DEV_SP_PSP && !(KVM_AMD=y && CRYPTO_DEV_CCP_DD=m)
> 
> CRYPTO_DEV_CCP_DD - this still doesn't belong here. KVM_AMD_SEV doesn't
> care about what kind of functionality the PSP needs to function and
> select it - it should only depend on a single symbol.
> 


It would be nice to have a single depends. But the main issue is, PSP 
support is provided through the ccp driver (aka CRYPTO_DEV_CCP_DD). 
Hence KVM_AMD_SEV need to have some level of dependency with ccp driver. 
This is to ensure that the ccp was 'y' when kvm-amd=y for SEV to work.


> Now, you can add a separate CRYPTO .config item which collects all
> required functionality for SEV guests and have KVM_AMD_SEV depend
> on that. Or a similar solution. But not have KVM_AMD_SEV poke into
> CRYPTO...PSP dependencies because it doesn't need to know about it.
> 


I am sorry but I am not able to follow you on how creating a separate 
CRYPTO .config item will solve this problem. Creating a separate config 
will be useful if we are okay with calling 'select' from kvm (i.e if 
kvm-amd is 'y' then all the symbols from crypto will be 'y').

I looked at other drivers where they have similar situation and it seems 
like solution is same as what I have used above.


-Brijesh

Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[Borislav Petkov]

On Tue, Jun 19, 2018 at 02:22:53PM -0500, Brijesh Singh wrote:
> It would be nice to have a single depends. But the main issue is, PSP
> support is provided through the ccp driver (aka CRYPTO_DEV_CCP_DD).

And this sentence states also your dependency:

KVM_AMD_SEV <- PSP driver <- CCP driver

but what you do is jump over one layer

KVM_AMD_SEV <- PSP driver
KVM_AMD_SEV <- CCP driver
PSP driver <- CCP driver

Do you see the difference and how the former is much easier to express
as a dependency vs the latter? And thus a lot more robust when it comes to
random configs.

> Hence KVM_AMD_SEV need to have some level of dependency with ccp
> driver. This is to ensure that the ccp was 'y' when kvm-amd=y for SEV
> to work.

If that is so, then your dependencies are wrong. KVM doesn't care about
CCP driver - it only cares about the PSP driver which gives the SEV API
it can call. That's it. If you have to make it depend on CCP, then the
design of what depends on what is wrong.

> I am sorry but I am not able to follow you on how creating a separate CRYPTO
> .config item will solve this problem. Creating a separate config will be
> useful if we are okay with calling 'select' from kvm (i.e if kvm-amd is 'y'
> then all the symbols from crypto will be 'y').

This is just an example of what you could do.

> I looked at other drivers where they have similar situation and it seems
> like solution is same as what I have used above.

Copying how the others do it is just not good enough - especially since
people can create broken configs currently.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[Brijesh Singh]

On 06/19/2018 02:43 PM, Borislav Petkov wrote:
> On Tue, Jun 19, 2018 at 02:22:53PM -0500, Brijesh Singh wrote:
>> It would be nice to have a single depends. But the main issue is, PSP
>> support is provided through the ccp driver (aka CRYPTO_DEV_CCP_DD).
> 
> And this sentence states also your dependency:
> 
> KVM_AMD_SEV <- PSP driver <- CCP driver
> 
> but what you do is jump over one layer
> 
> KVM_AMD_SEV <- PSP driver
> KVM_AMD_SEV <- CCP driver
> PSP driver <- CCP driver
> 
> Do you see the difference and how the former is much easier to express
> as a dependency vs the latter? And thus a lot more robust when it comes to
> random configs.
> 


In case if it was not clear, we don't have a standalone PSP driver.
The PSP support is provided by the CCP driver. If you look at config
changes I proposed then it says if PSP is available then we can support
SEV. But since PSP support is provided by the CCP driver hence we
need to have module dependency with CCP. So, we are using your former 
expression in the dependency but have to extend it a bit more.


>> Hence KVM_AMD_SEV need to have some level of dependency with ccp
>> driver. This is to ensure that the ccp was 'y' when kvm-amd=y for SEV
>> to work.
> 
> If that is so, then your dependencies are wrong. KVM doesn't care about
> CCP driver - it only cares about the PSP driver which gives the SEV API
> it can call. That's it. If you have to make it depend on CCP, then the
> design of what depends on what is wrong.
> 


We had discussion about this during our patch review process but lets
revisit again. CCP driver manages CCP and PSP devices. Ideally the
driver should have been called SP driver but ccp name existed well
before we added high level SP interface. IIRC, during SP patch review it
was recommended not to rename the driver from ccp->sp because it may
break folks who are already using with ccp name.

Here is how the config looks:

                   +------ CRYPTO_DEV_SP_PSP
                   |
CRYPTO_DEV_CCP_DD *
  (ccp.ko)         |                            +-- ccpv3
                   +------ CRYPTO_DEV_SP_CCP  --|
                                                +-- ccpv5
                                                ....




>> I am sorry but I am not able to follow you on how creating a separate CRYPTO
>> .config item will solve this problem. Creating a separate config will be
>> useful if we are okay with calling 'select' from kvm (i.e if kvm-amd is 'y'
>> then all the symbols from crypto will be 'y').
> 
> This is just an example of what you could do.
> 
>> I looked at other drivers where they have similar situation and it seems
>> like solution is same as what I have used above.
> 
> Copying how the others do it is just not good enough - especially since
> people can create broken configs currently.
> 

Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[Borislav Petkov]

On Tue, Jun 19, 2018 at 03:46:40PM -0500, Brijesh Singh wrote:
> In case if it was not clear, we don't have a standalone PSP driver.
> The PSP support is provided by the CCP driver. If you look at config
> changes I proposed then it says if PSP is available then we can support
> SEV. But since PSP support is provided by the CCP driver hence we
> need to have module dependency with CCP. So, we are using your former
> expression in the dependency but have to extend it a bit more.

It doesn't matter whether the driver is standalong or not as long as you
specifically have to express explicit dependency on:

depends CRYPTO_DEV_SP_PSP

Now that's the PSP component or whatever but as long as it has a
separate Kconfig symbol, it is the only thing that is important here.

Now, KVM_AMD_SEV needs the functionality behind CRYPTO_DEV_SP_PSP. So
far so good?

> We had discussion about this during our patch review process but lets
> revisit again. CCP driver manages CCP and PSP devices. Ideally the
> driver should have been called SP driver but ccp name existed well
> before we added high level SP interface. IIRC, during SP patch review it
> was recommended not to rename the driver from ccp->sp because it may
> break folks who are already using with ccp name.
> 
> Here is how the config looks:
> 
>                   +------ CRYPTO_DEV_SP_PSP
>                   |
> CRYPTO_DEV_CCP_DD *
>  (ccp.ko)         |                            +-- ccpv3
>                   +------ CRYPTO_DEV_SP_CCP  --|
>                                                +-- ccpv5
>                                                ....

Let me repeat my point more detailed again because it seems like it is
still not clear:

KVM needs SEV functionality which is behind CRYPTO_DEV_SP_PSP. Which
means, KVM_AMD_SEV should depend *only* on that. This is the symbol
which gives that functionality.

Now, you say "CCP driver manages CCP and PSP devices". So
CRYPTO_DEV_SP_PSP should select/depend on *everything* it needs in order
to function properly in order to provide that functionalty to KVM.

BUT! - and this is the key point - KVM should *not* care what PSP needs
to depend on in order to provide that functionality to KVM. PSP should
provide all that functionality itself and not rely on KVM_AMD_SEV to
select it for the PSP functionality.

IOW, you should have this:

config KVM_AMD_SEV
	bool "AMD Secure Encrypted Virtualization (SEV) support"
	depends KVM_AMD && X86_64 && CRYPTO_DEV_SP_PSP

No more symbols.

CRYPTO_DEV_SP_PSP should then do the proper selection so that
CRYPTO_DEV_CCP_DD has the proper setting wrt whether KVM is a module
or builtin. When you add CRYPTO_DEV_CCP_DD to the dependencies list,
you "fix" the design issue that ccp.ko is the driver which provides PSP
functionality.

I'm not sure if you could do something with a separate symbol as I
suggested earlier:

bool CRYPTO_PSP_FOR_KVM
	depends on CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP

or so and have KVM_AMD_SEV depend on that so that you have one
indirection more which wraps the CCP and PSP dependency.

But that seems unnecessary especially since we already have

config CRYPTO_DEV_SP_PSP
        bool "Platform Security Processor (PSP) device"
        default y
        depends on CRYPTO_DEV_CCP_DD && X86_64

so CRYPTO_DEV_SP_PSP already depends on CRYPTO_DEV_CCP_DD!

So why should KVM_AMD_SEV depend on CRYPTO_DEV_CCP_DD too?!? Do you
catch my drift?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

[Borislav Petkov]

On Tue, Jun 19, 2018 at 11:32:56PM +0200, Borislav Petkov wrote:
> config CRYPTO_DEV_SP_PSP
>         bool "Platform Security Processor (PSP) device"
>         default y
>         depends on CRYPTO_DEV_CCP_DD && X86_64
> 
> so CRYPTO_DEV_SP_PSP already depends on CRYPTO_DEV_CCP_DD!
> 
> So why should KVM_AMD_SEV depend on CRYPTO_DEV_CCP_DD too?!? Do you
> catch my drift?

And then in addition to that, KVM_AMD_SEV needs to "synchronize"
selection states so that KVM_AMD *and* CRYPTO_DEV_SP_PSP both have the
same setting - either =m or =y - so that the build works.

I can't think of a proper way to fix that right now, though, need to
sleep on it.

-- 
Regards/Gruss,
    Boris.

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