Re: [edk2-devel] separate OVMF binary for TDX? [was: OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest]

["Erdem Aktas" <erdemaktas@google.com>]

[-- Attachment #1: Type: text/plain, Size: 7911 bytes --]

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 <lersek@redhat.com> 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 <erdemaktas@google.com>
> >> Sent: Friday, April 16, 2021 3:43 AM
> >> To: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: devel@edk2.groups.io; jejb@linux.ibm.com; Yao, Jiewen
> >> <jiewen.yao@intel.com>; dgilbert@redhat.com; Laszlo Ersek
> >> <lersek@redhat.com>; Xu, Min M <min.m.xu@intel.com>;
> >> thomas.lendacky@amd.com; Brijesh Singh <brijesh.singh@amd.com>; Justen,
> >> Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> >> <ardb+tianocore@kernel.org>; Nathaniel McCallum
> >> <npmccallum@redhat.com>; Ning Yang <ningyang@google.com>
> >> 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 <pbonzini@redhat.com>
> >> 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
> >>>
>
>
>
> 
>
>
>

[-- Attachment #2: Type: text/html, Size: 10576 bytes --]