On 04/21/21 19:07, Erdem Aktas wrote:

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.

OK.

It's not lost on me that we're talking about ~3 instructions.

Let's keep a close eye on further "polymorphisms" that would require hacks.

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.

Yes, please do that. It can be included in the TDX patch set from Min Xu
that modifies the beginning of reset vector as discussed above.

Thanks!
Laszlo

-Erdem

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