Re: [PATCH v1 4/5] OvmfPkg/SmmCpuFeaturesLib: Abstact processor features

[Laszlo Ersek <lersek@redhat.com>]

Adding Paolo.

Paolo, here's the root of the thread:

https://lists.01.org/pipermail/edk2-devel/2016-September/001500.html

On 09/08/16 06:30, Joseph Shifflett wrote:
> Create new functions to abstract how XD/NX is detected, enabled, and
> disabled.  Also, create a new function to determine if Branch Trace
> Storage is supported.  Existing code is specific to Intel processors.
> 
> This provides NULL implementations of the new functions.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Joseph Shifflett <joseph.shifflett@hpe.com>
> ---
>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 44 ++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index a307f64c9c61..4c8c7fb7b25f 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -2,6 +2,7 @@
>  The CPU specific programming for PiSmmCpuDxeSmm module.
>  
>  Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
> +(C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
>  which accompanies this distribution.  The full text of the license may be found at
> @@ -832,3 +833,46 @@ SmmCpuFeaturesAllocatePageTableMemory (
>    return NULL;
>  }
>  
> +/**
> +  This API provides a method to determine if XD/NX support has been forced off in
> +  the non-SMM execution environment.  It will enable XD/NX support while in SMM
> +
> +  @retval TRUE   XD/NX was disabled when runningn in the non-SMM execution environment
> +  @retval FALSE  XD/NX was enabled when runningn in the non-SMM execution environment
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +SmmCpuFeaturesCheckAndEnableXdSupport (
> +  VOID
> +  )
> +{
> +  return FALSE;
> +}
> +
> +/**
> +  This API provides a method to disable XD/NX support before exiting SMM
> +**/
> +VOID
> +EFIAPI
> +SmmCpuFeaturesDisableXdSupport (
> +  VOID
> +  )
> +{
> +}
> +
> +/**
> +  This API determines if Branch Trace Storage Support is currently available
> +
> +  @retval TRUE   BTS is available
> +  @retval FALSE  BTS is disabled
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +SmmCpuFeaturesConfirmBranchTraceStorageSupport (
> +  VOID
> +  )
> +{
> +  return FALSE;
> +}
> 

I think it would be best to check these with Paolo, as he authored
OvmfPkg's SmmCpuFeaturesLib.

* The XD manipulation (wrt. the first two functions) is definitely
supported by KVM, so at least for those functions, I think the same
functionality as in the UefiCpuPkg library instance would be
appropriate. (See some additional pondering at [1] below.)

* For the last function, I think KVM does not implement the branch
recording MSRs. I also don't know if it even implements the "feature
query" MSR for the branch recording feature. Here too, the UefiCpuPkg
variant might be more appropriate for OvmfPkg than a constant FALSE --
please consult with Paolo.


Most importantly (I should have asked this first): what are you going to
use these functions for? I assume you are abstracting PiSmmCpuDxeSmm
from Ia32/X64, bringing it closer to ARM/AARCH64. It would be nice to
see the exact actions in PiSmmCpuDxeSmm that you plan to rebase on these
new library APIs.

If you post a version 2 of the patch set,
- please spell out the intended *concrete* use of each function in the
commit message of patch #1 (that is, where you modify the library class
header),
- please CC Paolo on the entire set.

(

[1] I recall that there was some nastiness wrt. the AMD SMM save state
area *not* saving and restoring XD state when entering and leaving SMM
*on Ia32*, but correctly saving and restoring XD state when doing the
same on X64.

Therefore our current recommendation for running a 32-bit OVMF binary
(that was built with -D SMM_REQUIRE) on qemu-system-i386 is to pass

  -cpu XXXX,-nx

because if XD is presented to the guest, then SMM will break. IOW,
currently we ask the QEMU user to hide the XD processor feature on
32-bit processors.

I don't know if we could get rid of the user-side tweak with the help of
this library. I guess you might be able to add MDE_CPU_IA32 vs.
MDE_CPU_X64 specific code to the first two functions in OvmfPkg;
reporting "XD invariably absent" in the MDE_CPU_IA32 case. However I'm
worried that some other component in the firmware would still query
MSR_IA32_MISC_ENABLE, and then be inconsistent with the constant retval
from here.

Thus we'll likely have to keep the -nx tweak in OvmfPkg/README anyway,
and in turn these functions should not be specific to Ia32/X64 in OvmfPkg.

I believe the first two functions should just copy the UefiCpuPkg
implementation, and possibly even the third one. But, check with Paolo
please.

)

Thanks!
Laszlo