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@xxxxxxx> 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@xxxxxxx>
>>> ---
>>>  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.
>