Regression: 100x I/O performance slowdown in SEC phase caused by TDX

Regression: 100x I/O performance slowdown in SEC phase caused by TDX

[James Bottomley]

I'm using a SEC phase which has a TPM driver to experiment with sorting
out measured boot, which is how I noticed (usually SEC doesn't do MMIO)
.  What I'm seeing is after commit b6b2de884864 ("MdePkg: Support mmio
for Tdx guest in BaseIoLibIntrinsic") we get a massive slowdown of
about 100x in TPM performance.  The reason seems to be this addition to
the mmioreadX/mmiowriteX code:

     MemoryFence ();
-    *(volatile UINT16 *)Address = Value;
+
+    if (IsTdxGuest ()) {
+      TdMmioWrite16 (Address, Value);
+    } else {
+      *(volatile UINT16 *)Address = Value;
+    }
+
     MemoryFence ();


The problem is that IsTdxGuest () has this structure:

BOOLEAN
EFIAPI
IsTdxGuest (
  VOID
  )
{
  if (mTdxProbed) {
    return mTdxEnabled;
  }

  mTdxEnabled = TdIsEnabled ();
  mTdxProbed  = TRUE;

  return mTdxEnabled;
}

Which is trying to cache the result of the probe in the efi data
segment.  However, that doesn't work in SEC, because the data segment
is read only (so the write seems to succeed but a read will always
return the original value), leading to us calling TdIsEnabled() check
for every mmio we do, which is causing the slowdown because it's very
expensive.

James

Re: [edk2-devel] Regression: 100x I/O performance slowdown in SEC phase caused by TDX

[Min Xu]

On April 13, 2022 5:22 AM, James Bottom wrote:
> 
> I'm using a SEC phase which has a TPM driver to experiment with sorting out
> measured boot, which is how I noticed (usually SEC doesn't do MMIO) .
> What I'm seeing is after commit b6b2de884864 ("MdePkg: Support mmio for
> Tdx guest in BaseIoLibIntrinsic") we get a massive slowdown of about 100x in
> TPM performance.  The reason seems to be this addition to the
> mmioreadX/mmiowriteX code:
> 
>      MemoryFence ();
> -    *(volatile UINT16 *)Address = Value;
> +
> +    if (IsTdxGuest ()) {
> +      TdMmioWrite16 (Address, Value);
> +    } else {
> +      *(volatile UINT16 *)Address = Value;
> +    }
> +
>      MemoryFence ();
> 
> 
> The problem is that IsTdxGuest () has this structure:
> 
> BOOLEAN
> EFIAPI
> IsTdxGuest (
>   VOID
>   )
> {
>   if (mTdxProbed) {
>     return mTdxEnabled;
>   }
> 
>   mTdxEnabled = TdIsEnabled ();
>   mTdxProbed  = TRUE;
> 
>   return mTdxEnabled;
> }
> 
> Which is trying to cache the result of the probe in the efi data segment.
> However, that doesn't work in SEC, because the data segment is read only
> (so the write seems to succeed but a read will always return the original
> value), leading to us calling TdIsEnabled() check for every mmio we do,
> which is causing the slowdown because it's very expensive.
> 
The analysis is right. I hadn't thought the basic IO would be heavily called in SEC phase. I will fix it as soon as possible.

My thought is that a TdProbeLib will be introduced in MdePkg. In MdePkg this lib is of null instance. It just simply return TD_PROBE_NON which means it is not Td guest. This lib will be added in MdePkg/MdeLibs.dsc.inc (like RegisterFilterLibNull). 
In OvmfPkg TdProbeLib will be implemented. It checks the OvmfWorkArea to determine the guest type.

Thanks
Min