Date:   Thu, 14 Jun 2018 14:37:42 +0200
From:   Felix Niederwanger <felix@...dspaten.org>
To:     Brijesh Singh <brijesh.singh@....com>,
        Richard Weinberger <richard.weinberger@...il.com>,
        Borislav Petkov <bp@...e.de>
Cc:     Janakarajan Natarajan <Janakarajan.Natarajan@....com>,
        Tom Lendacky <thomas.lendacky@....com>, x86@...nel.org,
        kvm <kvm@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krcmar <rkrcmar@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>
Subject: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

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





Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists