Re: [edk2-devel] [RFC PATCH v4 00/27] Add AMD Secure Nested Paging (SEV-SNP) support

["Brijesh Singh" <brijesh.singh@amd.com>]

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

thanks