Message-ID: <SN6PR12MB27671B7E6C6CA09B9B74502A8EAA9@SN6PR12MB2767.namprd12.prod.outlook.com>
Date:   Tue, 14 Jun 2022 20:39:44 +0000
From:   "Kalra, Ashish" <Ashish.Kalra@....com>
To:     Peter Gonda <pgonda@...gle.com>
CC:     Alper Gun <alpergun@...gle.com>,
        the arch/x86 maintainers <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        kvm list <kvm@...r.kernel.org>,
        "linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
        "Lendacky, Thomas" <Thomas.Lendacky@....com>,
        "H. Peter Anvin" <hpa@...or.com>, Ard Biesheuvel <ardb@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Sergio Lopez <slp@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        David Rientjes <rientjes@...gle.com>,
        Dov Murik <dovmurik@...ux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@....com>,
        Borislav Petkov <bp@...en8.de>,
        "Roth, Michael" <Michael.Roth@....com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Andi Kleen <ak@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>, Marc Orr <marcorr@...gle.com>,
        Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
Subject: RE: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command

[AMD Official Use Only - General]

Hello Peter,

>> Here is the feedback from the SEV/SNP firmware team:
>>
>> The SNP spec has this line in SNP_INIT_EX:
>>
>> “The firmware marks all encryption capable ASIDs as unusable for encrypted virtualization.”
>>
>> This is a back-handed way of saying that after SNP_INIT, all of the ASIDs are considered “dirty”. None are in use, but the FW can’t know if there’s any data in the caches left over from some prior guest. So after doing an SNP_INIT (or SNP_INIT_EX), a WBINVD (on all threads) and DF_FLUSH sequence is required. It doesn’t matter whether it’s an SEV DF_FLUSH or an SNP_DF_FLUSH… they both do EXACTLY the same thing.
>>
>> I don’t understand off hand why SNP_LAUNCH_START would require a 
>> DF_FLUSH… Usually, it’s only an “activate” command that requires the 
>> DF_FLUSH. ACTIVATE/ACTIVATE_EX/SNP_ACTIVATE/SNP_ACTIVATE_EX
>>
>> [Ashish] That's why I asked if you are getting the DLFLUSH_REQUIRED error after the SNP activate command ?
>>
>> It’s when you attempt to (re-)use >an ASID that’s “dirty” that you should get the DF_FLUSH_REQUIRED error.
>>
>> [Ashish] And we do SNP_DF_FLUSH/SEV_DF_FLUSH whenever ASIDs are reused/re-cycled.
>>
>> If the host only wants to run SNP guests, there’s no need to do an SEV INIT or any other SEV operations. If the host DOES want to run SEV AND SNP guests, then the required sequence is to do the SNP_INIT before the SEV INIT. Doing the >>WBINVD/DF_FLUSH any time between the SNP_INIT and any ACTIVATE command should be sufficient.

>Thanks for the follow up Ashish! I was assuming there was some issue with the ASIDs here but didn't have time to experiment yet.

Yes, it looks to be some issue with ASIDs.

> Is there any downside to enabling SEV? If not these patches make sense but if there is some cost should we make KVM capable of running SNP VMs without enabling SEV?

As mentioned above we only need to do this if the host wants to run both SEV and SNP guests. I assume the cost should not be considered as this is only a init or guest launch time addition.

Thanks,
Ashish

> -----Original Message-----
> From: Alper Gun <alpergun@...gle.com>
> Sent: Tuesday, June 14, 2022 1:58 PM
> To: Kalra, Ashish <Ashish.Kalra@....com>
> Cc: Peter Gonda <pgonda@...gle.com>; the arch/x86 maintainers 
> <x86@...nel.org>; LKML <linux-kernel@...r.kernel.org>; kvm list 
> <kvm@...r.kernel.org>; linux-coco@...ts.linux.dev; linux-mm@...ck.org; 
> Linux Crypto Mailing List <linux-crypto@...r.kernel.org>; Thomas 
> Gleixner <tglx@...utronix.de>; Ingo Molnar <mingo@...hat.com>; Joerg 
> Roedel <jroedel@...e.de>; Lendacky, Thomas <Thomas.Lendacky@....com>; 
> H. Peter Anvin <hpa@...or.com>; Ard Biesheuvel <ardb@...nel.org>; 
> Paolo Bonzini <pbonzini@...hat.com>; Sean Christopherson 
> <seanjc@...gle.com>; Vitaly Kuznetsov <vkuznets@...hat.com>; Wanpeng 
> Li <wanpengli@...cent.com>; Jim Mattson <jmattson@...gle.com>; Andy 
> Lutomirski <luto@...nel.org>; Dave Hansen 
> <dave.hansen@...ux.intel.com>; Sergio Lopez <slp@...hat.com>; Peter 
> Zijlstra <peterz@...radead.org>; Srinivas Pandruvada 
> <srinivas.pandruvada@...ux.intel.com>; David Rientjes 
> <rientjes@...gle.com>; Dov Murik <dovmurik@...ux.ibm.com>; Tobin 
> Feldman-Fitzthum <tobin@....com>; Borislav Petkov <bp@...en8.de>; 
> Roth, Michael <Michael.Roth@....com>; Vlastimil Babka 
> <vbabka@...e.cz>; Kirill A . Shutemov <kirill@...temov.name>; Andi 
> Kleen <ak@...ux.intel.com>; Tony Luck <tony.luck@...el.com>; Marc Orr 
> <marcorr@...gle.com>; Sathyanarayanan Kuppuswamy 
> <sathyanarayanan.kuppuswamy@...ux.intel.com>
> Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command
>
> Let me summarize what I tried.
>
> 1- when using psp_init_probe false, the SNP VM fails in SNP_LAUNCH_START step with error SEV_RET_DFFLUSH_REQUIRED(15).
> 2- added SEV_DF_FLUSH just after SNP platform init and it didn't fail 
> in launch start but failed later during SNP_LAUNCH_UPDATE with
> SEV_RET_INVALID_PARAM(22)
> 3- added SNP_DF_FLUSH just after SNP platform init and it failed again 
> during SNP_LAUNCH_UPDATE with SEV_RET_INVALID_PARAM(22)
> 4- added sev_platform_init for SNP VMs and it worked.
>
> For me DF_FLUSH alone didn' help boot a VM. I don't know yet why sev platform status impacts the SNP VM, but sev_platform_init fixes the problem.
>
>
> On Tue, Jun 14, 2022 at 10:16 AM Kalra, Ashish <Ashish.Kalra@....com> wrote:
> >
> > [AMD Official Use Only - General]
> >
> > Hello Alper, Peter,
> >
> > -----Original Message-----
> > From: Peter Gonda <pgonda@...gle.com>
> > Sent: Tuesday, June 14, 2022 11:30 AM
> > To: Kalra, Ashish <Ashish.Kalra@....com>
> > Cc: Alper Gun <alpergun@...gle.com>; Brijesh Singh 
> > <brijesh.singh@....com>; the arch/x86 maintainers <x86@...nel.org>; 
> > LKML <linux-kernel@...r.kernel.org>; kvm list <kvm@...r.kernel.org>; 
> > linux-coco@...ts.linux.dev; linux-mm@...ck.org; Linux Crypto Mailing 
> > List <linux-crypto@...r.kernel.org>; Thomas Gleixner 
> > <tglx@...utronix.de>; Ingo Molnar <mingo@...hat.com>; Joerg Roedel 
> > <jroedel@...e.de>; Lendacky, Thomas <Thomas.Lendacky@....com>; H.
> > Peter Anvin <hpa@...or.com>; Ard Biesheuvel <ardb@...nel.org>; Paolo 
> > Bonzini <pbonzini@...hat.com>; Sean Christopherson 
> > <seanjc@...gle.com>; Vitaly Kuznetsov <vkuznets@...hat.com>; Wanpeng 
> > Li <wanpengli@...cent.com>; Jim Mattson <jmattson@...gle.com>; Andy 
> > Lutomirski <luto@...nel.org>; Dave Hansen 
> > <dave.hansen@...ux.intel.com>; Sergio Lopez <slp@...hat.com>; Peter 
> > Zijlstra <peterz@...radead.org>; Srinivas Pandruvada 
> > <srinivas.pandruvada@...ux.intel.com>; David Rientjes 
> > <rientjes@...gle.com>; Dov Murik <dovmurik@...ux.ibm.com>; Tobin 
> > Feldman-Fitzthum <tobin@....com>; Borislav Petkov <bp@...en8.de>; 
> > Roth, Michael <Michael.Roth@....com>; Vlastimil Babka 
> > <vbabka@...e.cz>; Kirill A . Shutemov <kirill@...temov.name>; Andi 
> > Kleen <ak@...ux.intel.com>; Tony Luck <tony.luck@...el.com>; Marc 
> > Orr <marcorr@...gle.com>; Sathyanarayanan Kuppuswamy 
> > <sathyanarayanan.kuppuswamy@...ux.intel.com>; Pavan Kumar Paluri 
> > <papaluri@....com>
> > Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT 
> > command
> >
> > On Tue, Jun 14, 2022 at 10:11 AM Kalra, Ashish <Ashish.Kalra@....com> wrote:
> > >
> > > [AMD Official Use Only - General]
> > >
> > >
> > > -----Original Message-----
> > > From: Peter Gonda <pgonda@...gle.com>
> > > Sent: Tuesday, June 14, 2022 10:38 AM
> > > To: Kalra, Ashish <Ashish.Kalra@....com>
> > > Cc: Alper Gun <alpergun@...gle.com>; Brijesh Singh 
> > > <brijesh.singh@....com>; Kalra, Ashish <Ashish.Kalra@....com>; the
> > > arch/x86 maintainers <x86@...nel.org>; LKML 
> > > <linux-kernel@...r.kernel.org>; kvm list <kvm@...r.kernel.org>; 
> > > linux-coco@...ts.linux.dev; linux-mm@...ck.org; Linux Crypto 
> > > Mailing List <linux-crypto@...r.kernel.org>; Thomas Gleixner 
> > > <tglx@...utronix.de>; Ingo Molnar <mingo@...hat.com>; Joerg Roedel 
> > > <jroedel@...e.de>; Lendacky, Thomas <Thomas.Lendacky@....com>; H.
> > > Peter Anvin <hpa@...or.com>; Ard Biesheuvel <ardb@...nel.org>; 
> > > Paolo Bonzini <pbonzini@...hat.com>; Sean Christopherson 
> > > <seanjc@...gle.com>; Vitaly Kuznetsov <vkuznets@...hat.com>; 
> > > Wanpeng Li <wanpengli@...cent.com>; Jim Mattson 
> > > <jmattson@...gle.com>; Andy Lutomirski <luto@...nel.org>; Dave 
> > > Hansen <dave.hansen@...ux.intel.com>; Sergio Lopez 
> > > <slp@...hat.com>; Peter Zijlstra <peterz@...radead.org>; Srinivas 
> > > Pandruvada <srinivas.pandruvada@...ux.intel.com>; David Rientjes 
> > > <rientjes@...gle.com>; Dov Murik <dovmurik@...ux.ibm.com>; Tobin 
> > > Feldman-Fitzthum <tobin@....com>; Borislav Petkov <bp@...en8.de>; 
> > > Roth, Michael <Michael.Roth@....com>; Vlastimil Babka 
> > > <vbabka@...e.cz>; Kirill A . Shutemov <kirill@...temov.name>; Andi 
> > > Kleen <ak@...ux.intel.com>; Tony Luck <tony.luck@...el.com>; Marc 
> > > Orr <marcorr@...gle.com>; Sathyanarayanan Kuppuswamy 
> > > <sathyanarayanan.kuppuswamy@...ux.intel.com>; Pavan Kumar Paluri 
> > > <papaluri@....com>
> > > Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT 
> > > command
> > >
> > > On Mon, Jun 13, 2022 at 6:21 PM Ashish Kalra <ashkalra@....com> wrote:
> > > >
> > > >
> > > > On 6/13/22 23:33, Alper Gun wrote:
> > > > > On Mon, Jun 13, 2022 at 4:15 PM Ashish Kalra <ashkalra@....com> wrote:
> > > > >> Hello Alper,
> > > > >>
> > > > >> On 6/13/22 20:58, Alper Gun wrote:
> > > > >>> static int sev_guest_init(struct kvm *kvm, struct 
> > > > >>> kvm_sev_cmd
> > > > >>> *argp)
> > > > >>>>    {
> > > > >>>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT ||
> > > > >>>> + argp->id == KVM_SEV_SNP_INIT);
> > > > >>>>           struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > > > >>>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> > > > >>>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
> > > > >>>>           int asid, ret;
> > > > >>>>
> > > > >>>>           if (kvm->created_vcpus) @@ -249,12 +269,22 @@ 
> > > > >>>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > > > >>>>                   return ret;
> > > > >>>>
> > > > >>>>           sev->es_active = es_active;
> > > > >>>> +       sev->snp_active = snp_active;
> > > > >>>>           asid = sev_asid_new(sev);
> > > > >>>>           if (asid < 0)
> > > > >>>>                   goto e_no_asid;
> > > > >>>>           sev->asid = asid;
> > > > >>>>
> > > > >>>> -       ret = sev_platform_init(&argp->error);
> > > > >>>> +       if (snp_active) {
> > > > >>>> +               ret = verify_snp_init_flags(kvm, argp);
> > > > >>>> +               if (ret)
> > > > >>>> +                       goto e_free;
> > > > >>>> +
> > > > >>>> +               ret = sev_snp_init(&argp->error);
> > > > >>>> +       } else {
> > > > >>>> +               ret = sev_platform_init(&argp->error);
> > > > >>> After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > > > >>> In my tests, if SEV has not been initialized in the platform 
> > > > >>> yet, SNP VMs fail with SEV_DF_FLUSH required error. I tried 
> > > > >>> calling SEV_DF_FLUSH right after the SNP platform init but 
> > > > >>> this time it failed later on the SNP launch update command 
> > > > >>> with SEV_RET_INVALID_PARAM error. Looks like there is 
> > > > >>> another dependency on SEV platform initialization.
> > > > >>>
> > > > >>> Calling sev_platform_init for SNP VMs fixes the problem in our tests.
> > > > >> Trying to get some more context for this issue.
> > > > >>
> > > > >> When you say after SEV_INIT_EX support patches, SEV may be 
> > > > >> initialized in the platform late, do you mean sev_pci_init()->sev_snp_init() ...
> > > > >> sev_platform_init() code path has still not executed on the host BSP ?
> > > > >>
> > > > > Correct, INIT_EX requires the file system to be ready and 
> > > > > there is a ccp module param to call it only when needed.
> > > > >
> > > > > MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be 
> > > > > initialized on module init. Else the PSP will be initialized 
> > > > > on the first command requiring it");
> > > > >
> > > > > If this module param is false, it won't initialize SEV on the 
> > > > > platform until the first SEV VM.
> > > > >
> > > > Ok, that makes sense.
> > > >
> > > > So the fix will be to call sev_platform_init() unconditionally 
> > > > here in sev_guest_init(), and both sev_snp_init() and
> > > > sev_platform_init() are protected from being called again, so 
> > > > there won't be any issues if these functions are invoked again 
> > > > at SNP/SEV VM launch if they have been invoked earlier during module init.
> > >
> > > >That's one solution. I don't know if there is a downside to the system for enabling SEV if SNP is being enabled but another solution could be to just directly place a DF_FLUSH command instead of calling sev_platform_init().
> > >
> > > Actually sev_platform_init() is already called on module init if psp_init_on_probe is not false. Only need to ensure that SNP firmware is initialized first with SNP_INIT command.
> >
> > > But if psp_init_on_probe is false, sev_platform_init() isn't called down this path. Alper has suggested we always call sev_platform_init() but we could just place an SEV_DF_FLUSH command instead. Or am I still missing something?
> >
> > >After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > > In my tests, if SEV has not been initialized in the platform  yet, 
> > >SNP VMs fail with SEV_DF_FLUSH required error. I tried  calling 
> > >SEV_DF_FLUSH right after the SNP platform init.
> >
> > Are you getting the DLFLUSH_REQUIRED error after the SNP activate command ?
> >
> > Also did you use the SEV_DF_FLUSH command or the SNP_DF_FLUSH command ?
> >
> > With SNP you need to use SNP_DF_FLUSH command.
> >
> > Thanks,
> > Ashish

Powered by blists - more mailing lists