Re: [edk2-devel] [PATCH v3 0/3] SEV Page Encryption Bitmap support for OVMF.

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

On 12/7/20 8:44 PM, Laszlo Ersek wrote:
> On 12/04/20 09:10, Ashish Kalra wrote:
>> On Fri, Dec 04, 2020 at 04:50:05AM +0100, Laszlo Ersek wrote:
>>> On 12/04/20 01:03, Ashish Kalra wrote:
>>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>>
>>>> By default all the SEV guest memory regions are considered encrypted,
>>>> if a guest changes the encryption attribute of the page (e.g mark a
>>>> page as decrypted) then notify hypervisor. Hypervisor will need to
>>>> track the unencrypted pages. The information will be used during
>>>> guest live migration, guest page migration and guest debugging.
>>>>
>>>> The patch-set also adds a new SEV and SEV-ES hypercall abstraction
>>>> library to support SEV Page encryption/decryption status hypercalls
>>>> for SEV and SEV-ES guests.
>>>>
>>>> BaseMemEncryptSevLib invokes hypercalls via this new hypercall library.
>>>>
>>>> A branch containing these patches is available here:
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fashkalra%2Fedk2%2Ftree%2Fsev_page_encryption_bitmap_v3&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C13b084db30e246f25b3f08d89b233f99%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637429922982198583%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=wuYFXFyBtwZWSWOCb3OYK8I7MDFAxId%2BC63fsa0XcjQ%3D&amp;reserved=0
>>>>
>>>> Changes since v2:
>>>>  - GHCB_BASE setup during reset-vector as decrypted is marked explicitly
>>>>    in the hypervisor page encryption bitmap after setting the 
>>>>    PcdSevEsIsEnabled PCD.
>>>>
>>>> Changes since v1:
>>>>  - Mark GHCB_BASE setup during reset-vector as decrypted explicitly in
>>>>    the hypervisor page encryption bitmap.
>>>>  - Resending the series with correct shallow threading.
>>>>
>>>> Ashish Kalra (2):
>>>>   OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls.
>>>>   OvmfPkg/PlatformPei: Mark SEC GHCB page in the page encrpytion bitmap.
>>>>
>>>> Brijesh Singh (1):
>>>>   OvmfPkg/BaseMemEncryptLib: Support to issue unencrypted hypercall
>>>>
>>>>  .../Include/Library/MemEncryptHypercallLib.h  |  37 ++++++
>>>>  .../BaseMemEncryptSevLib.inf                  |   1 +
>>>>  .../BaseMemEncryptSevLib/X64/VirtualMemory.c  |  18 +++
>>>>  .../MemEncryptHypercallLib.c                  | 105 ++++++++++++++++++
>>>>  .../MemEncryptHypercallLib.inf                |  39 +++++++
>>>>  .../X64/AsmHelperStub.nasm                    |  39 +++++++
>>>>  OvmfPkg/OvmfPkgX64.dsc                        |   1 +
>>>>  OvmfPkg/PlatformPei/AmdSev.c                  |  10 ++
>>>>  8 files changed, 250 insertions(+)
>>>>  create mode 100644 OvmfPkg/Include/Library/MemEncryptHypercallLib.h
>>>>  create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.c
>>>>  create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf
>>>>  create mode 100644 OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm
>>>>
>>> I'll need some time to get to this series.
>>>
>>> I'm fairly certain though, from a quick skim, that this series breaks
>>> all DSC files under OvmfPkg except X64. Please fix that.
>>>
>>>
>> Ok thanks Laszlo, i will fix this.
> Thanks.
>
> I can see a new comment for the series from Dov Murik, and I think
> that's awesome. I'd welcome if there were lively exchanges around OVMF
> patch sets. I'm selfish of course: I'd like to delegate reviews.
>
> So, on this patch set, I notice it does not add the new
> (MemEncryptHypercallLib-related) files to Maintainers.txt, namely
> section "OvmfPkg: SEV-related modules".
>
> Please include such a patch in v4 -- if Tom and Brijesh agree, I'd like
> to put the new lib explicitly under their reviewership.


I am okay with the ownership.


> Also, I plan to review this series (v4, at this point) only for
> formalities. I'd like to receive an R-b from Tom or Brijesh [*], and
> another from Dov or a colleague at IBM, for this series; those together
> should suffice for merging the library.


Since this patch has dependency on HV feature, so I was going to review
this patch after I see some confirmation coming from KVM upstream on the
hypervcall approach. It appears that Sean may have some other ideas, so
lets wait to hear those before we consider this patch.


>
> [*] Brijesh seems to be the original author of patch#2, so maybe Tom is
> a better-poised reviewer for this.
>
> Thanks
> Laszlo
>