Hi Laszlo, I am sorry to hear that it sounded like we are dictating a certain approach. Although I can see why it sounded that way, it certainly was not my intention. We want to work with the EDK2 community to have a solution that is beneficial for everyone and we appreciate the inputs that we got from you and Paolo. Code quality is always a high priority for us. Therefore, if, at some point, things get too hacky to impact the quality/maintainability of the upstream code, we will consider making adjustments on our side. With the current discussion, I was just trying to describe our use case and the importance of having a single binary where there is no absolute need for architectural differences. As far as I know, the only problematic area is modifying the reset vector to be compatible with TDX and it seems like Intel has a solution for it without impacting the overall quality of the upstream code. I just want to reiterate that we are open for discussion and what we ask should not be considered "at all cost" and please let us know that if edk2 community/maintainers are still thinking that what Intel is proposing is not feasible. >>Can Google at least propose a designated reviewer ("R") for the >>"OvmfPkg: Confidential Computing" section of "Maintainers.txt", in a patch? Sure I would be happy too. -Erdem On Wed, Apr 21, 2021 at 3:44 AM Laszlo Ersek wrote: > On 04/21/21 02:38, Yao, Jiewen wrote: > > Hello > > Do we have some conclusion on this topic? > > > > Do we agree the one-binary solution in OVMF or we need more discussion? > > Well it's not technically impossible to do, just very ugly and brittle. > And I'm doubtful that this is a unique problem ("just fix the reset > vector") the likes of which will supposedly never return during the > integration of SEV and TDX. Once we make this promise ("one firmware > binary at all costs"), the hacks we accept for its sake will only > accumulate over time, and we'll have more and more precedent to justify > the next hack. Technical debt is not exactly what we don't have enough > of, in edk2. > > I won't make a secret out of the fact that I'm slightly annoyed that > this approach is being dictated by Google (as far as I understand, at > this point, anyway). I don't see or recall a lot of Google contributions > in the edk2 history or the bug tracker. I'm not enthusiastic about > complexity without explicit commitment / investment on the beneficiary's > side. > > I won't nack the approach personally, but I'm quite unhappy about it. > Can Google at least propose a designated reviewer ("R") for the > "OvmfPkg: Confidential Computing" section of "Maintainers.txt", in a patch? > > Thanks > Laszlo > > > > > > > Thank you > > Yao Jiewen > > > >> -----Original Message----- > >> From: Erdem Aktas > >> Sent: Friday, April 16, 2021 3:43 AM > >> To: Paolo Bonzini > >> Cc: devel@edk2.groups.io; jejb@linux.ibm.com; Yao, Jiewen > >> ; dgilbert@redhat.com; Laszlo Ersek > >> ; Xu, Min M ; > >> thomas.lendacky@amd.com; Brijesh Singh ; Justen, > >> Jordan L ; Ard Biesheuvel > >> ; Nathaniel McCallum > >> ; Ning Yang > >> Subject: Re: [edk2-devel] separate OVMF binary for TDX? [was: OvmfPkg: > >> Reserve the Secrets and Cpuid page for the SEV-SNP guest] > >> > >> Thanks Paolo. > >> > >> On Thu, Apr 15, 2021 at 12:59 AM Paolo Bonzini > >> wrote: > >>> > >>> On 15/04/21 01:34, Erdem Aktas wrote: > >>>> We do not want to generate different binaries for AMD, Intel, Intel > >>>> with TDX, AMD with SEV/SNP etc > >>> > >>> My question is why the user would want a single binary for VMs with and > >>> without TDX/SNP. I know there is attestation, but why would you even > >>> want the _possibility_ that your guest starts running without TDX or > SNP > >>> protection, and only find out later via attestation? > >> > >> There might be multiple reasons why customers want it but we need this > >> requirement for a couple of other reasons too. > >> > >> We do not only have hardware based confidential VMs. We might have > >> some other solutions which measure the initial image before boot. > >> Ultimately we might want to use a common attestation interface where > >> customers might be running different kinds of VMs. Using a single > >> binary will make it easier to manage/verify measurements for both of > >> us and the customers. I am not a PM so I cannot give more context on > >> customer use cases. > >> > >> Another reason is how we deploy and manage guest firmware. We have a > >> lot of optimization and customization to speed up firmware loading > >> time and also reduce the time to deploy new builds on the whole fleet > >> uniformly. Adding a new firmware binary is a big challenge for us to > >> enable these features. On the top of integration challenges, it will > >> create maintainability issues in the long run for us when we provide > >> tools to verify/reproduce the hashes in the attestation report. > >> > >>> want the _possibility_ that your guest starts running without TDX or > SNP > >>> protection, and only find out later via attestation? > >> > >> I am missing the point here. Customers should rely on only the > >> attestation report to establish the trust. > >> -If firmware does not support TDX and TDX is enabled, that firmware > >> will crash at some point. > >> -If firmware is generic firmware that supports TDX and SNP and others, > >> and TDX is enabled or not, still the customer needs to verify the TDX > >> enablement through attestation. > >> -If firmware is a customized binary compiled to support TDX, > >> irrelevant of TDX being enabled or not, still the customer needs to > >> verify the TDX enablement through attestation. > >> > >> > >>> For a similar reason, OVMF already supports shipping a binary that > fails > >>> to boot if SMM is not available to the firmware, because then secure > >>> boot would be trivially circumvented. > >>> > >>> I can understand having a single binary for both TDX or SNP. That's > not > >>> a problem since you can set up the SEV startup VMSA to 32-bit protected > >>> mode just like TDX wants. > >> > >> I agree that this is doable but I am not sure if we need to also > >> modify the reset vector for AMD SNP in that case. Also it will not > >> solve our problem. If we start to generate a new firmware for every > >> feature , it will not end well for us, I think. Both TDX and SNP are > >> still new features in the same architecture, and it seems to me that > >> they are sharing a lot of common/similar code. AMD has already made > >> some of their patches in (SEV and SEV-ES) which works very nicely for > >> our use case and integration. Looks like Intel just has an issue on > >> how to fix their reset vector problem. Once they solve it and upstream > >> accepts the changes, I do not see any other big blocker. OVMF was > >> doing a great job on abstracting differences and providing a common > >> interface without creating multiple binaries. I do not see why it > >> should not do the same thing here. > >> > >>>> therefore we were expecting the TDX > >>>> changes to be part of the upstream code. > >>> > >>> Having 1 or more binaries should be unrelated to the changes being > >>> upstream (or more likely, I am misunderstanding you). > >> > >> You are right, it is my bad for not clarifying it. What I mean is we > >> want it to be part of the upstream so it can be easier for us to pull > >> the changes and they are compatible with the changes that SNP is doing > >> but we also do not want to use different configuration files to > >> generate different binaries for each use case. > >> > >> > >>> Thanks, > >>> > >>> Paolo > >>> > > > > > > >