[PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3974

Ovmf work-area (PcdOvmfWorkArea) was designed to store the Confidential
Computing guest information, including the CC guest type. This
information will be probed by CcProbeLib so that the CC guest type can
be determined in run-time. But the Ovmf work-area was reserved as
BT_Data so that it cannot be accessed after ExitBootService. Please see
the detailed analysis in BZ#3974.

RH also reports a similar bug. Please see:
https://bugzilla.redhat.com/show_bug.cgi?id=2114858

This patch reserves the work-area as RT_Data to fix this bug.

Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/Library/PlatformInitLib/IntelTdx.c  | 2 +-
 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
index c6d7c8bb6e0e..286f447fea03 100644
--- a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
+++ b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
@@ -557,7 +557,7 @@ PlatformTdxPublishRamRegions (
     BuildMemoryAllocationHob (
       (EFI_PHYSICAL_ADDRESS)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaBase),
       (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaSize),
-      EfiBootServicesData
+      EfiRuntimeServicesData
       );
   }
 }
diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 942eaf89cfcf..83fc061fcbac 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -1022,7 +1022,7 @@ PlatformQemuInitializeRamForS3 (
       BuildMemoryAllocationHob (
         (EFI_PHYSICAL_ADDRESS)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaBase),
         (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaSize),
-        PlatformInfoHob->S3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
+        PlatformInfoHob->S3Supported ? EfiACPIMemoryNVS : EfiRuntimeServicesData
         );
     }
 
-- 
2.29.2.windows.2

Re: [PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Gerd Hoffmann]

On Mon, Aug 22, 2022 at 10:23:01AM +0800, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3974
> 
> Ovmf work-area (PcdOvmfWorkArea) was designed to store the Confidential
> Computing guest information, including the CC guest type. This
> information will be probed by CcProbeLib so that the CC guest type can
> be determined in run-time. But the Ovmf work-area was reserved as
> BT_Data so that it cannot be accessed after ExitBootService. Please see
> the detailed analysis in BZ#3974.
> 
> RH also reports a similar bug. Please see:
> https://bugzilla.redhat.com/show_bug.cgi?id=2114858
> 
> This patch reserves the work-area as RT_Data to fix this bug.

Seems to not be enough, I still see the page fault.
Maybe EfiConvertPointer() is needed at runtime?

take care,
  Gerd

Re: [PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Lendacky, Thomas]

On 8/21/22 21:23, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3974
> 
> Ovmf work-area (PcdOvmfWorkArea) was designed to store the Confidential
> Computing guest information, including the CC guest type. This
> information will be probed by CcProbeLib so that the CC guest type can
> be determined in run-time. But the Ovmf work-area was reserved as
> BT_Data so that it cannot be accessed after ExitBootService. Please see
> the detailed analysis in BZ#3974.
> 
> RH also reports a similar bug. Please see:
> https://bugzilla.redhat.com/show_bug.cgi?id=2114858
> 
> This patch reserves the work-area as RT_Data to fix this bug.

The work area was never meant to be kept around. When first introduced, 
Laszlo had said it could be used early, but that global structures should 
be represented by PCDs. So the code is correct. It seems that the 
CcProbeLib should be setting some PCDs during the start of DXE or similar 
for use during run time services.

Thanks,
Tom

> 
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>   OvmfPkg/Library/PlatformInitLib/IntelTdx.c  | 2 +-
>   OvmfPkg/Library/PlatformInitLib/MemDetect.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
> index c6d7c8bb6e0e..286f447fea03 100644
> --- a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
> +++ b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
> @@ -557,7 +557,7 @@ PlatformTdxPublishRamRegions (
>       BuildMemoryAllocationHob (
>         (EFI_PHYSICAL_ADDRESS)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaBase),
>         (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaSize),
> -      EfiBootServicesData
> +      EfiRuntimeServicesData
>         );
>     }
>   }
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index 942eaf89cfcf..83fc061fcbac 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -1022,7 +1022,7 @@ PlatformQemuInitializeRamForS3 (
>         BuildMemoryAllocationHob (
>           (EFI_PHYSICAL_ADDRESS)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaBase),
>           (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaSize),
> -        PlatformInfoHob->S3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
> +        PlatformInfoHob->S3Supported ? EfiACPIMemoryNVS : EfiRuntimeServicesData
>           );
>       }
>   

Re: [PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Min Xu]

On August 22, 2022 4:52 PM, Gerd Hoffmann wrote:
> On Mon, Aug 22, 2022 at 10:23:01AM +0800, Min Xu wrote:
> > From: Min M Xu <min.m.xu@intel.com>
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3974
> >
> > Ovmf work-area (PcdOvmfWorkArea) was designed to store the
> > Confidential Computing guest information, including the CC guest type.
> > This information will be probed by CcProbeLib so that the CC guest
> > type can be determined in run-time. But the Ovmf work-area was
> > reserved as BT_Data so that it cannot be accessed after
> > ExitBootService. Please see the detailed analysis in BZ#3974.
> >
> > RH also reports a similar bug. Please see:
> > https://bugzilla.redhat.com/show_bug.cgi?id=2114858
> >
> > This patch reserves the work-area as RT_Data to fix this bug.
> 
> Seems to not be enough, I still see the page fault.
> Maybe EfiConvertPointer() is needed at runtime?
> 
Ah, I forget to reserve the work-area as RT_Data in below code:
      BuildMemoryAllocationHob (
        (EFI_PHYSICAL_ADDRESS)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaBase),
        (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaSize),
        PlatformInfoHob->S3Supported ? EfiACPIMemoryNVS : EfiRuntimeServicesData  <-- ACPI_NVS is not accessible either in OS-Runtime.
        );
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformInitLib/MemDetect.c#L1022-L1026

Gerd, do you think we can reserve Ovmf WorkArea as RT_Data even when S3 is supported?

Thanks
Min

Re: [PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Min Xu]

On August 22, 2022 9:19 PM, Tom Lendacky wrote:
> On 8/21/22 21:23, Min Xu wrote:
> > From: Min M Xu <min.m.xu@intel.com>
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3974
> >
> > Ovmf work-area (PcdOvmfWorkArea) was designed to store the
> > Confidential Computing guest information, including the CC guest type.
> > This information will be probed by CcProbeLib so that the CC guest
> > type can be determined in run-time. But the Ovmf work-area was
> > reserved as BT_Data so that it cannot be accessed after
> > ExitBootService. Please see the detailed analysis in BZ#3974.
> >
> > RH also reports a similar bug. Please see:
> > https://bugzilla.redhat.com/show_bug.cgi?id=2114858
> >
> > This patch reserves the work-area as RT_Data to fix this bug.
> 
> The work area was never meant to be kept around. When first introduced,
> Laszlo had said it could be used early, but that global structures should be
> represented by PCDs. So the code is correct. It seems that the CcProbeLib
> should be setting some PCDs during the start of DXE or similar for use during
> run time services.
> 
Hi, Tom
The dynamic PCD is not thread-safe. Will IoLib be used in multi-thread scenario?

Thanks
Min

Re: [PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Gerd Hoffmann]

  Hi,

> Ah, I forget to reserve the work-area as RT_Data in below code:
>       BuildMemoryAllocationHob (
>         (EFI_PHYSICAL_ADDRESS)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaBase),
>         (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaSize),
>         PlatformInfoHob->S3Supported ? EfiACPIMemoryNVS : EfiRuntimeServicesData  <-- ACPI_NVS is not accessible either in OS-Runtime.
>         );
> https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformInitLib/MemDetect.c#L1022-L1026

With that changed to use EfiRuntimeServicesData unconditionally the page
fault is gone.

> Gerd, do you think we can reserve Ovmf WorkArea as RT_Data even when S3 is supported?

Hmm, not fully sure how the various memory types are handled when the
VM is suspended.

take care,
  Gerd

Re: [PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Min Xu]

On August 23, 2022 3:38 PM, Gerd Hoffmann wrote:
>   Hi,
> 
> > Ah, I forget to reserve the work-area as RT_Data in below code:
> >       BuildMemoryAllocationHob (
> >         (EFI_PHYSICAL_ADDRESS)(UINTN)FixedPcdGet32
> (PcdOvmfWorkAreaBase),
> >         (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaSize),
> >         PlatformInfoHob->S3Supported ? EfiACPIMemoryNVS :
> EfiRuntimeServicesData  <-- ACPI_NVS is not accessible either in OS-Runtime.
> >         );
> >
> https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/Platform
> > InitLib/MemDetect.c#L1022-L1026
> 
> With that changed to use EfiRuntimeServicesData unconditionally the page
> fault is gone.
> 
> > Gerd, do you think we can reserve Ovmf WorkArea as RT_Data even when
> S3 is supported?
> 
> Hmm, not fully sure how the various memory types are handled when the
> VM is suspended.
> 
Tom suggested that the work area should not  be kept around. In stead a PCD should be used.
https://edk2.groups.io/g/devel/message/92617

But the dynamic PCD is not thread-safe and it cannot be accessed by APs. 

Now we have 2 solutions to fix this issue.
1. Reserve the Ovmf work area as RT_DATA.
2. Split the CcProbeLib into 2 instances. See https://edk2.groups.io/g/devel/message/91132

Gerd & Tom, what's your thought?

Thanks
Min

Re: [PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Lendacky, Thomas]

On 8/23/22 03:40, Xu, Min M wrote:
> On August 23, 2022 3:38 PM, Gerd Hoffmann wrote:
>>    Hi,
>>
>>> Ah, I forget to reserve the work-area as RT_Data in below code:
>>>        BuildMemoryAllocationHob (
>>>          (EFI_PHYSICAL_ADDRESS)(UINTN)FixedPcdGet32
>> (PcdOvmfWorkAreaBase),
>>>          (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaSize),
>>>          PlatformInfoHob->S3Supported ? EfiACPIMemoryNVS :
>> EfiRuntimeServicesData  <-- ACPI_NVS is not accessible either in OS-Runtime.
>>>          );
>>>
>> https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/Platform
>>> InitLib/MemDetect.c#L1022-L1026
>>
>> With that changed to use EfiRuntimeServicesData unconditionally the page
>> fault is gone.
>>
>>> Gerd, do you think we can reserve Ovmf WorkArea as RT_Data even when
>> S3 is supported?
>>
>> Hmm, not fully sure how the various memory types are handled when the
>> VM is suspended.
>>
> Tom suggested that the work area should not  be kept around. In stead a PCD should be used.
> https://edk2.groups.io/g/devel/message/92617
> 
> But the dynamic PCD is not thread-safe and it cannot be accessed by APs.
> 
> Now we have 2 solutions to fix this issue.
> 1. Reserve the Ovmf work area as RT_DATA.
> 2. Split the CcProbeLib into 2 instances. See https://edk2.groups.io/g/devel/message/91132
> 
> Gerd & Tom, what's your thought?

Runtime service call are restricted so that you don't have concurrent 
threads executing (see section 8.1 of the specification). Without that you 
would have problems with runtime services today.

Thanks,
Tom

> 
> Thanks
> Min
> 

Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Min Xu]

On August 23, 2022 9:07 PM, Lendacky,Thomas wrote:
> On 8/23/22 03:40, Xu, Min M wrote:
> > On August 23, 2022 3:38 PM, Gerd Hoffmann wrote:
> >>    Hi,
> >>
> >>> Ah, I forget to reserve the work-area as RT_Data in below code:
> >>>        BuildMemoryAllocationHob (
> >>>          (EFI_PHYSICAL_ADDRESS)(UINTN)FixedPcdGet32
> >> (PcdOvmfWorkAreaBase),
> >>>          (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaSize),
> >>>          PlatformInfoHob->S3Supported ? EfiACPIMemoryNVS :
> >> EfiRuntimeServicesData  <-- ACPI_NVS is not accessible either in OS-
> Runtime.
> >>>          );
> >>>
> >> https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/Platfor
> >> m
> >>> InitLib/MemDetect.c#L1022-L1026
> >>
> >> With that changed to use EfiRuntimeServicesData unconditionally the
> >> page fault is gone.
> >>
> >>> Gerd, do you think we can reserve Ovmf WorkArea as RT_Data even when
> >> S3 is supported?
> >>
> >> Hmm, not fully sure how the various memory types are handled when the
> >> VM is suspended.
> >>
> > Tom suggested that the work area should not  be kept around. In stead a
> PCD should be used.
> > https://edk2.groups.io/g/devel/message/92617
> >
> > But the dynamic PCD is not thread-safe and it cannot be accessed by APs.
> >
> > Now we have 2 solutions to fix this issue.
> > 1. Reserve the Ovmf work area as RT_DATA.
> > 2. Split the CcProbeLib into 2 instances. See
> > https://edk2.groups.io/g/devel/message/91132
> >
> > Gerd & Tom, what's your thought?
> 
> Runtime service call are restricted so that you don't have concurrent threads
> executing (see section 8.1 of the specification). Without that you would have
> problems with runtime services today.
>
One of the situation of CcProbe used is in BaseIoLib.
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdx.c#L35-L40

BaseIoLib is a basic library and it may be called by APs. While dynamic PCD access is not allowed in APs. Of course we can cache the PCD in a variable but I think it is still not safe.

Reserving the OVMF work area as RT_DATA breaks the original intention of the design.

Then how about this solution?
https://edk2.groups.io/g/devel/message/91132
We can design 2 instances of CcProbe. One is to read the OvmfWorkArea. The other is to call Cc guest specific way to determine the type.

Thanks
Min

Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Gerd Hoffmann]

  Hi,

> > Runtime service call are restricted so that you don't have concurrent threads
> > executing (see section 8.1 of the specification). Without that you would have
> > problems with runtime services today.
> >
> One of the situation of CcProbe used is in BaseIoLib.
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdx.c#L35-L40
> 
> BaseIoLib is a basic library and it may be called by APs. While dynamic PCD access is not allowed in APs. Of course we can cache the PCD in a variable but I think it is still not safe.
> 
> Reserving the OVMF work area as RT_DATA breaks the original intention of the design.
> 
> Then how about this solution?
> https://edk2.groups.io/g/devel/message/91132
> We can design 2 instances of CcProbe. One is to read the OvmfWorkArea. The other is to call Cc guest specific way to determine the type.

When using RT_DATA is out of question there is no way around two
instances I think.

I don't see the point in reinventing the wheel though.  We know what the
guest type is, why determine it *again*?  We only need to copy the guest
type from the work area to another place before it is gone.  I think
the dxe library instance could copy it to a variable in the init function.

take care,
  Gerd

Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Min Xu]

On August 25, 2022 3:42 PM, Gerd Hoffmann wrote:
>   Hi,
> 
> > > Runtime service call are restricted so that you don't have
> > > concurrent threads executing (see section 8.1 of the specification).
> > > Without that you would have problems with runtime services today.
> > >
> > One of the situation of CcProbe used is in BaseIoLib.
> > https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseIoLib
> > Intrinsic/IoLibInternalTdx.c#L35-L40
> >
> > BaseIoLib is a basic library and it may be called by APs. While dynamic PCD
> access is not allowed in APs. Of course we can cache the PCD in a variable but
> I think it is still not safe.
> >
> > Reserving the OVMF work area as RT_DATA breaks the original intention of
> the design.
> >
> > Then how about this solution?
> > https://edk2.groups.io/g/devel/message/91132
> > We can design 2 instances of CcProbe. One is to read the OvmfWorkArea.
> The other is to call Cc guest specific way to determine the type.
> 
> When using RT_DATA is out of question there is no way around two instances
> I think.
> 
> I don't see the point in reinventing the wheel though.  We know what the
> guest type is, why determine it *again*?  We only need to copy the guest type
> from the work area to another place before it is gone.  I think the dxe library
> instance could copy it to a variable in the init function.
> 
As I mentioned CcProbe is used in BaseIoLib which is a basic library and it is used in SEC/PEI/DXE phases. Global variable cannot be used in SEC phase. Unless BaseIoLib is split into 2 instance, one for SEC/PEI phase, the other for DXE phase. 
Gerd, do you mean this solution?

Or there is another solution based on https://edk2.groups.io/g/devel/message/91132
We can design 2 instances of CcProbe. One is to read the OvmfWorkArea. This is for SEC/PEI phase.
The other is to save the guest type in Ovmf work area to a global variable in its init function. This is for DXE phase.

Thanks
Min

Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Reserve the Ovmf work area as RT_DATA

[Gerd Hoffmann]

  Hi,

> We can design 2 instances of CcProbe. One is to read the OvmfWorkArea. This is for SEC/PEI phase.
> The other is to save the guest type in Ovmf work area to a global variable in its init function. This is for DXE phase.

I mean this one.

take care,
  Gerd