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.
thanks
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78604): https://edk2.groups.io/g/devel/message/78604
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]
-=-=-=-=-=-=-=-=-=-=-=-
