On October 12, 2021 4:22 PM, Gerd Hoffmann wrote:
> > +// PageSize is mapped to PageLevel like below:
> > +// 4KB - 0, 2MB - 1
> > +UINT64 mTdxAcceptPageLevelMap[2] = {
> > + SIZE_4KB,
> > + SIZE_2MB
>
> No 1G pages?
TDX:
https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf
Theoretically there are 3 AcceptPageSize (4KB/2MB/1GB). But currently only 4KB
and 2MB are supported. See section 22.3.2.
>
> > +UINTN
> > +GetGpaPageLevel (
> > + UINT64 PageSize
>
> Uh, UINT32 is not enough? Keep the door open for 512G pages?
Thanks for reminder. UINT32 is enough. It will be updated in next version.
>
> > +{
> > + UINTN Index;
> > +
> > + for (Index = 0; Index < sizeof (mTdxAcceptPageLevelMap) / sizeof
> > + (mTdxAcceptPageLevelMap[0]); Index++) {
>
> There is an ARRAY_SIZE() macro, no need to open code the sizeof() trick.
Thanks for reminder. It will be updated in next version.
>
> > + if (mTdxAcceptPageLevelMap[Index] == PageSize) {
> > + break;
> > + }
> > + }
> > +
> > + return Index;
> > +}
>
> No error handling (invalid PageSize) here?
>
> > +/**
> > + This function accept a pending private page, and initialize the
> > +page to
> > + all-0 using the TD ephemeral private key.
> > +
> > + Sometimes TDCALL [TDG.MEM.PAGE.ACCEPT] may return
> > + TDX_EXIT_REASON_PAGE_SIZE_MISMATCH. It indicates the input PageLevel
> > + is not workable. In this case we need to try to fallback to a
> > + smaller PageLevel if possible.
> > +
> > + @param[in] StartAddress Guest physical address of the private
> > + page to accept.
> > + @param[in] NumberOfPages Number of the pages to be accepted.
> > + @param[in] PageSize GPA page size. Only accept 1G/2M/4K size.
> > +
> > + @return EFI_SUCCESS Accept successfully
> > + @return others Indicate other errors
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +TdAcceptPages (
> > + IN UINT64 StartAddress,
> > + IN UINT64 NumberOfPages,
> > + IN UINT64 PageSize
> > + )
> > +{
> > + EFI_STATUS Status;
> > + UINT64 Address;
> > + UINT64 TdxStatus;
> > + UINT64 Index;
> > + UINT64 GpaPageLevel;
> > + UINT64 PageSize2;
> > +
> > + Address = StartAddress;
> > +
> > + GpaPageLevel = (UINT64) GetGpaPageLevel (PageSize);
>
> Why cast?
Thanks for reminder. The cast is not needed. It will be updated in next version.
>
> > + if (GpaPageLevel > sizeof (mTdxAcceptPageLevelMap) / sizeof
> (mTdxAcceptPageLevelMap[0])) {
> > + DEBUG ((DEBUG_ERROR, "Accept page size must be 4K/2M. Invalid
> > + page size - 0x%llx\n", PageSize));
>
> Ah. Errors are catched here. Well, no. The check is wrong, it should be
> ">=" not
> ">".
>
> Better would be GetGpaPageLevel explicitly returning a specific value (for
> example -1) on error.
You're right. A dumb error ... It will be fixed in the next version.
>
> > + Status = EFI_SUCCESS;
> > + for (Index = 0; Index < NumberOfPages; Index++) {
> > + TdxStatus = TdCall (TDCALL_TDACCEPTPAGE, Address | GpaPageLevel, 0, 0,
> 0);
> > + if (TdxStatus != TDX_EXIT_REASON_SUCCESS) {
> > + if ((TdxStatus & ~0xFFFFULL) ==
> TDX_EXIT_REASON_PAGE_ALREADY_ACCEPTED) {
> > + //
> > + // Already accepted
> > + //
> > + mNumberOfDuplicatedAcceptedPages++;
>
> Hmm. When this happens we have a bug somewhere, right?
> So should this be an assert()?
> Or should we at least log the address?
ASSERT() is not necessary because the code can still run. But we should log the
address which has been already accepted.
It will be added in the next version.
>
> > +#define RTMR_COUNT 4
> > +#define TD_EXTEND_BUFFER_LEN (64 + 64)
> > +#define EXTEND_BUFFER_ADDRESS_MASK 0x3f
> > +
> > +
> > +#pragma pack(16)
> > +typedef struct {
> > + UINT8 Buffer[TD_EXTEND_BUFFER_LEN];
> > +} TDX_EXTEND_BUFFER;
> > +#pragma pack()
> > +
> > +UINT8 *mExtendBufferAddress = NULL;
> > +TDX_EXTEND_BUFFER mExtendBuffer;
> > +
> > +/**
> > + TD.RTMR.EXTEND requires 64B-aligned guest physical address of
> > + 48B-extension data. In runtime we walk thru the Buffer to find
> > + out a 64B-aligned start address.
>
> Can't you just use __attribute__((aligned(64))) for that?
>
In the PoC of TDVF I had thought about it. But at last I gave up such solution.
The reasons are:
1) OVMF/TDVF supports both GCC and VS2019 tool chain.
__attribute__((aligned(64))) is for GCC. Its counterpart of VS2019 Tool chain
is __declspec(align(x)).
2) There is the limitation of /ALIGN:32 in the build scripts which means
aligned 64 exceeds the /ALIGN 32, unless /ALIGN is updated to 64.
That's why the current solution is used.
Thanks!
Min
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81883): https://edk2.groups.io/g/devel/message/81883
Mute This Topic: https://groups.io/mt/86085729/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
