See my feedback below.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
> Singh via groups.io
> Sent: Tuesday, August 3, 2021 11:01 PM
> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io
> Cc: brijesh.si...@amd.com; James Bottomley <j...@linux.ibm.com>; Xu, Min M
> <min.m...@intel.com>; Tom Lendacky <thomas.lenda...@amd.com>; Justen,
> Jordan L <jordan.l.jus...@intel.com>; Ard Biesheuvel
> <ardb+tianoc...@kernel.org>; Laszlo Ersek <ler...@redhat.com>; Erdem Aktas
> <erdemak...@google.com>; Dong, Eric <eric.d...@intel.com>; Ni, Ray
> <ray...@intel.com>; Kumar, Rahul1 <rahul1.ku...@intel.com>; Kinney, Michael
> D <michael.d.kin...@intel.com>; Liming Gao <gaolim...@byosoft.com.cn>; Liu,
> Zhiguang <zhiguang....@intel.com>; Michael Roth <michael.r...@amd.com>
> Subject: Re: [edk2-devel] [RFC PATCH v4 00/27] Add AMD Secure Nested Paging
> (SEV-SNP) support
>
>
>
> On 7/28/21 9:22 PM, Yao, Jiewen wrote:
> > Hi Brijesh
> > Thanks for the patient.
> > Most of my comment focus on the *common* part, and *interface* between
> SEV and common code.
> > I will leave you to decide the detailed SEV specific implementation.
> >
>
> Thank you Jiewen for your feedback. I will try to address the comments
> in next version.
>
> >
> > Patch-04:
> > Can we use consistent naming conversion?
> > We have PcdOvmfSecGhcbPageTableBase, PcdOvmfSecGhcbBase,
> PcdSevLaunchSecretBase. Now we are adding PcdOvmfSnpSecretsBase.
> > Can we change PcdOvmfSnpSecretsBase to PcdSevSnpSecretsBase?
> > Or we change PcdSevLaunchSecretBase to PcdOvmfSevLaunchSecretBase?
>
> I don't know why we choose "Ovmf" from the LaunchSecretsBase PCD. I
> thought PCD's specific the Uefi typically contains the Ovmf name. Maybe
> we can fix the LaunchSecretsBase to match with the name. I will do that
> as a separate patch.
>
> >
> > Patch-05:
> > Ditto. Naming convention.
> >
> > Patch-06:
> > I have recommendation to Min, to separate SEV stuff to a standalone file
> > from
> ResetVectorVtf0.asm.
> > Intel can add TDX stuff to a standalone file, and make it included by
> ResetVectorVtf0.asm.
> >
> > I am not sure if you want to do it, or you leave Min to do it.
> >
>
> For the SEV stuff, I will do it myself so that I can test it as well :)
>
> > Patch-07:
> > Same naming convention issue. See #04 and #05.
> >
> > Patch-08:
> > I hope we can move all below code to AmdSev.asm, such as
> PostPageTableHookSev().
> > Then the PageTable64.asm can be SEV/TDX agnostic.
> >
> > I am not sure if you want to do it, or you leave Min to do it.
> >
> > ==============
> > ;
> > ; Clear the encryption bit from the GHCB entry
> > ;
> > mov ecx, (GHCB_BASE & 0x1F_FFFF) >> 12
> > mov [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0
> >
> > mov ecx, GHCB_SIZE / 4
> > xor eax, eax
> > clearGhcbMemoryLoop:
> > mov dword[ecx * 4 + GHCB_BASE - 4], eax
> > loop clearGhcbMemoryLoop
> >
> > ;
> > ; The page table built above cleared the memory encryption mask from
> > the
> > ; GHCB_BASE (aka made it shared). When SEV-SNP is enabled, to maintain
> > ; the security guarantees, the page state transition from private to
> > ; shared must go through the page invalidation steps. Invalidate the
> > ; memory range before loading the page table below.
> > ;
> > ; NOTE: the invalidation must happen after zeroing the GHCB memory.
> > This
> > ; is because, in the 32-bit mode all the access are considered
> > private.
> > ; The invalidation before the zero'ing will cause a #VC.
> > ;
> > OneTimeCall InvalidateGHCBPage
> > ==============
> >
>
> I will try to see if I can move that out as well.
>
> > Patch-10:
> > I am not UEFI CPU package maintainer. But I do have a little concern to add
> more PcdXxxIsEnable style PCD, especially when they are mutual exclusive (like
> TDX v.s SEV).
> > If we follow this pattern, we will have PcdSevEsIsEnabled,
> > PcdSevSnpIsEnabled,
> PcdSevFutureIsEnabled, PcdTdxIsEnabled, PcdTdxFutureIsEnabled, ... that will
> be
> an endless list.
> >
> > If possible, I suggest define one PcdConfidentialComputingType - indicate
> Legacy, SEV, TDX.
> >
>
> There are certain things which are applicable to SEV-ES and not for the
> SEV-SNP and vice versa. I am not oppose to create a generic helper e.g
>
> enum {
> AmdSev,
> AmdSevEs,
> AmdSevSnp,
> IntelTdx,
> IntelSgx,
> ..
> ..
> };
>
> bool EncryptedGuestFeatureEnabled(enum type);
>
> But I think some of this can be done later as well.
>
> > Patch-12:
> > Can we move all SEV stuff to a standalone file, such as AmdSev.c?
> >
> > I am not sure if you want to do it, or you leave Min to do it.
> >
>
> Yes, I can do it.
>
> > Patch-18:
> > If we have a standalone AmdSev.c (#12), then we can move the function to
> that file, and only leave a hook call to SEV.
> >
>
> I will try to consolidate it in AmdSev.c
>
> > Patch-23:
> > This is UEFI CPU package update. I am thinking if we can follow same patter
> > to
> move all SEV stuff to a standalone file, such as AmdSev.c, AmdSev.asm.
> > In the future, we may add TDX stuff as well.
> >
> > Patch-26:
> > Same comment as #23.
> >
> > Patch-27:
> > Can we move that function to a standalone AmdSev.c ?
> >
> > Patch-28:
> > Would you please describe more on what is ConfidentialComputingBlob ?
>
> While launching the SEV-SNP guests, the hypervisor may need to provide
> some additional information during the guest boot. When booting under
> the EFI based BIOS, the EFI configuration table contains an entry for
> the confidential computing blob that contains the required information.
> The Linux kernel will lookup for this EFI table during the boot to
> locate the secrets and cpuid page.
>
>
> > Is that generic concept? Or SEV specific thing?
>
> Its designed as a generic and the current only SEV-SNP provides it.
> > Who is consumer?
>
> Any guest kernel (window or Linux)
>
> > What is difference between ConfidentialComputingSecret and
> ConfidentialComputingBlob ? When to use which?
> >
>
> The confidentialComputingSecrets contains the secrets keys where the
> CCBlob contains the information which maybe used during the boot.
>
> You can see some more about it on my kernel patches:
>
> https://lore.kernel.org/lkml/20210707181506.30489-26-
> brijesh.si...@amd.com/
>
> > I can understand how TDX use ConfidentialComputingSecret, but how do you
> expect TDX use ConfidentialComputingBlob (if it is a generic concept) ?
>
> I think in the case of TDX , the information needed during the boot is
> provided through the ACPI tables but in SEV-SNP those are provided
> throught the CCBlob. In the contianer environement there will be no EFI
> so in that case the Blob will be passed to the boot loader setup data.
> If required then TDX can use it to pass the boot information.
[Jiewen] Got it. I treat it as a generic way to pass information from Guest FW
to guest OS.
If so, I have concern on having a generic name - CC_BLOB, but only includes SEV
specific data structure there.
I offer two possible alternatives, and I open on other options.
A) define it as SEV_BLOB. Don’t use generic name. As such, the consume knows
this blob is for SEV.
B) define it TYPE-LENGTH-VALUE list in CC_BLOB. As such, the consume can pass
the TYPE to know this blob is for SEV.
If possible, I prefer to split this big patch series to smaller one, especially
the CC_BLOB.
I think we may have more discussion on how to support that.
But I don’t want to block your other work such as creating standalone SEV file
and add SEV-SNP stuff there.
>
> thanks
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78646): https://edk2.groups.io/g/devel/message/78646
Mute This Topic: https://groups.io/mt/83850692/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
