Re: [edk2-devel] [PATCH RFC v3 05/22] OvmfPkg: reserve Secrets page in MEMFD

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

On 6/8/21 1:01 PM, Laszlo Ersek via groups.io wrote:
>
>> Now I think about it maybe we should leave the driver where it is
>> because OvmfPkgX64.dsc does not need to deal with the attestation etc.
>> But we need to create a driver that can install the EFI configuration
>> table for the SNP secrets page. Is that okay ?
> Well I'm sure I'm day-dreaming, but here's what I'd like, all at the
> same time of course:
>
> - to-the-point PCD names and explanations in the DEC file,
>
> - minimize the new pages carved out of MEMFD,
>
> - make sure that this new SEV-SNP secret page is reusable (at least
> conceptually) in the "AmdSevX64.dsc" platform if necessary -- it may
> exist at a different base address in that platform, but the mechanism
> should be the same,
>
> - I wouldn't like a new DXE driver for installing this new config table,
> and I also wouldn't like to move and/or reuse SecretDxe. Instead, I
> might like some new code in the existent AmdSevDxe driver, which is
> already the SEV-specific platform driver (also included by the
> "AmdSevX64.dsc" platform). If the config table cannot be installed in
> the entry point function of the driver, can we invent a suitable
> protocol notify, or other event notification callback?

Yes, I will try that approach.


>
>>
>>>> Later, the AmdSev.dsc can include a library to perform the
>>>> SEV-SNP-specific attestation. The library can use the SNP secrets page
>>>> to get the key and message counter use for constructing the guest
>>>> message to query the attestation report.
>>>>
>>>> I hope it clarifies it.
>>>>
>>>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F56860.pdf&data=04%7C01%7Cbrijesh.singh%40amd.com%7C8db4f78aad5148606b6608d92aa789fe%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637587721344269131%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=roXIS4GuBJg3ftf5lQyFwItfDXQ8oHukiN0wolvd8Wg%3D&reserved=0
>>>>
>>>>
>>>>> Honestly I'm getting a *rushed* vibe on this whole series. Why is that?
>>>> I am not sure why you are getting this feel, please let me know where I
>>>> can help to clarify but the series is *rushed* at all. Its building on
>>>> existing support. It's possible that we are getting mixed with the
>>>> fundamental difference between the SEV and SEV-SNP attestation flow and
>>>> recent patches from Dov to expand the attestation to cover other aspects
>>>> of the boot flow.
>>>>
>>>> In case of SEV-SNP, some folks may prefer to do all the attestation in
>>>> the OVMF and others may prefer to do the attestation in the guest OS. We
>>>> should try to not restrict one approach over the other.
>>>>
>>>>
>>>>> Assume that I'm dumb. You won't be far from the truth. Then hold my hand
>>>>> through all this?
>>>> Please let me know if the above explanation helps or I should expand more.
>>> You should please (a) expand your *commit messages*, (b) add a *wall* of
>>> text in the "OvmfPkg.dec" file, where the PCDs in questions are
>>> declared. When I grep the OvmfPkg subdirectory in two years for
>>> "PcdSevLaunchSecretBase", I'd like to find the DEC file's comments to be
>>> consistent with the actual uses of the PCD, and I'd like git-blame to
>>> tell me something useful about those lines, too.
>>>
>> I will add more comments in the patch to clarify certain things.
>>
>>
>>> One problem is that I'm supposed to internalize about 50 pages from yet
>>> from another technical specification, in order to get the basics of a
>>> single patch. I can't even follow the *set* of AMD documents I should
>>> have a local copy of. How am I supposed to interleave all that with, for
>>> example, reviewing a 57 slide TDX design presentation?
>> As you may have seen that myself and Tom try not to put the exact link
>> or  document number in the comment
> Huh, so that's been intentional? I didn't expect it.

At least I have been asked by it in the kernel ML folks to avoid using
the moving URL. So, I am doing similar thing for OVMF commit messages.


>
>> is because we have seen that our docs
>> folks change the link or they replace the old document with the new
>> copy.
> Changing links is tolerable, as long as previously released versions
> (identified by document# and revision#) remain accessible forever.
>
>> We have similar issue in kernel. The kernel maintainer now have a
>> bugzilla where they want us to upload the document so that they can keep
>> a copy and in the commits we refer to that BZ link instead of AMD URL.
> This is a fantastic idea from those kernel developers, and I'm scolding
> myself for not thinking of the same.
>
> If you do upload such documents to the kernel bugzilla, please feel free
> to refer to the exact same (kernel bugziila) URLs in the edk2 commit
> messages, and (perhaps) code comments. There's no need to upload the
> (large) PDFs to the tianocore bugzilla separately; I trust the kernel
> bugzilla instance to stick around.
>
>> I
>> myself gets so mixed up with various version of documents. I don't like
>> that we replace the old docs with a new without archiving it.
> Hm, so the old docs do disappear for good (not just the URLs but the
> docs themselves). In that case, please let us go with kernel bugzilla
> attachments.

Yes, it simply disappears hence my frustration. I have ran into cases
that during the review the doc gets updated and my code gets out of sync
with the posted spec :(. I think some of this has to do with the
feedbacks from other hypervisor folks on SNP mailing list. I am hoping
this gets stable soon. SEV-SNP FW is still in 0.9 and I hope it moves to
1.0 sooner for the stable APIs. Anyway the good part is that none of
these affect OVMF. Since early SEV days we have tried best to avoid
expanding the blobs in the guest firmware and OS. When its comes to
attestation then we have no choice but to define the Secrets page layout
to access all those keys etc. In future I am sure someone will work to
extend AmdSev package to provide attestation for the SEV-SNP and at time
they will have much stable layout to work it.

Thanks