Re: [edk2-devel] [PATCH v3 00/20] NetworkPkg: CVE-2023-45236 and CVE-2023-45237

[Gerd Hoffmann]

On Thu, May 23, 2024 at 10:44:52PM GMT, Doug Flick via groups.io wrote:
> 
> REF:https://blog.quarkslab.com/pixiefail-nine-vulnerabilities-in-tianocores-edk-ii-ipv6-network-stack.html
> 
> This patch series patches the following CVEs:
> - CVE-2023-45236: Predictable TCP Initial Sequence Numbers
> - CVE-2023-45237: Use of a Weak PseudoRandom Number Generator

Ok, looks like there is some more fallout from this patch series which I
havn't seen in my initial testing.  It does not always happen, didn't
figure yet what exactly triggers the behavior.  But in some cases there
is quite some network stack activity, apparently done by
EVT_SIGNAL_EXIT_BOOT_SERVICES event handlers ...

With the debug patch below applied the tail of the ovmf log looks like
this:

  VirtioRngExitBoot: Context=0x7D73D798
  Hash2ServiceBindingDestroyChild - Invalid handle
  MnpServiceBindingDestroyChild: Failed to uninstall the ManagedNetwork 
protocol, Invalid Parameter.
  Support(): UNDI3.1 found on handle 7D461118
  Support(): supported on 7D461118
  Start(): UNDI3.1 found

  snp->undi.start()  1h:8000h
  InstallProtocolInterface: 7AB33A91-ACE5-4326-B572-E7EE33D39F16 7CE872C0
  InstallProtocolInterface: F44C00EE-1F2C-4A00-AA09-1C9F3E0800A3 7CE7D020
  Failed to generate random data using secure algorithm 0: Unsupported
  Failed to generate random data using secure algorithm 1: Unsupported
  Failed to generate random data using secure algorithm 2: Unsupported
  Failed to generate random data using secure algorithm 3: Unsupported
  VirtioRngGetRNG: not ready
  Failed to generate random data using secure algorithm 4: Device Error

  ASSERT_EFI_ERROR (Status = Device Error)
  ASSERT 
/home/kraxel/projects/edk2/NetworkPkg/Library/DxeNetLib/DxeNetLib.c(965): 
!(((INTN)(RETURN_STATUS)(Status)) < 0)

The VirtioRngDxe EVT_SIGNAL_EXIT_BOOT_SERVICES handler resets the
device, to make sure it will stop any DMA.

Once the reset is done the device can't deliver random numbers any more,
but the network code wants some.  So with the debug patch an assert is
triggered, without the debug patch the system simply hangs because the
virtio-rng device wouldn't answer request sent by the driver.

I'm wondering what the network code is actually doing here in the first
place?  It apparently /installs/ protocols in the
EVT_SIGNAL_EXIT_BOOT_SERVICES handler?  I don't think this is how things
are supposed to work ...

take care,
  Gerd

- cut here -
diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h
index 2da99540a208..3519521d6ab5 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.h
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h
@@ -33,6 +33,7 @@ typedef struct {
   VRING Ring;   // VirtioRingInit   2
   EFI_RNG_PROTOCOL  Rng;// VirtioRngInit1
   VOID  *RingMap;   // VirtioRingMap2
+  BOOLEAN   Ready;
 } VIRTIO_RNG_DEV;
 
 #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
index 75a9644f749c..36e3eed4617c 100644
--- a/OvmfPkg/VirtioNetDxe/Events.c
+++ b/OvmfPkg/VirtioNetDxe/Events.c
@@ -77,7 +77,7 @@ VirtioNetExitBoot (
   //
   VNET_DEV  *Dev;
 
-  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __func__, Context));
+  DEBUG ((DEBUG_INFO, "%a: Context=0x%p\n", __func__, Context));
   Dev = Context;
   if (Dev->Snm.State == EfiSimpleNetworkInitialized) {
 Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
index 069aed148af1..370c9ac8f1de 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
@@ -156,6 +156,10 @@ VirtioRngGetRNG (
   }
 
   Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
+  if (!Dev->Ready) {
+DEBUG ((DEBUG_INFO, "%a: not ready\n", __func__));
+return EFI_DEVICE_ERROR;
+  }
   //
   // Map Buffer's system physical address to device address
   //
@@ -382,6 +386,7 @@ VirtioRngInit (
   //
   Dev->Rng.GetInfo = VirtioRngGetInfo;
   Dev->Rng.GetRNG  = VirtioRngGetRNG;
+  Dev->Ready = TRUE;
 
   return EFI_SUCCESS;
 
@@ -414,8 +419,8 @@ VirtioRngUninit (
   // VIRTIO_CFG_WRITE() returns, the host will have learned to stay away from
   // the old comms area.
   //
+  Dev->Ready = FALSE;
   Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
-
   Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
 
   VirtioRingUninit (Dev->VirtIo, >Ring);
@@ -435,7 +440,7 @@ VirtioRngExitBoot (
 {
   VIRTIO_RNG_DEV  *Dev;
 
-  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __func__, Context));
+  DEBUG ((DEBUG_INFO, "%a: Context=0x%p\n", __func__, Context));
   //
   // Reset the device. This causes the hypervisor to forget about the virtio
   // ring.
@@ -444,6 +449,7 @@ VirtioRngExitBoot (
   // executing after ExitBootServices() is permitted to overwrite it.
   //
   Dev = Context;
+  Dev->Ready = FALSE;
   

Re: [edk2-devel] [PATCH v1 0/2] Add a new FdtNorFalshQemuLib and enable it in

[Gerd Hoffmann]

On Fri, May 24, 2024 at 04:38:26PM GMT, Chao Li wrote:
> Hi Ard and other maintainers,
> 
> Could you help to review this patch set?

Looks good to me and survived a quick smoke test.

Tested-by: Gerd Hoffmann 
Acked-by: Gerd Hoffmann 

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119336): https://edk2.groups.io/g/devel/message/119336
Mute This Topic: https://groups.io/mt/106149594/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] GitHub PR Code Review process now active

[Gerd Hoffmann]

> The GitHub PR code review process is now active.  Please 
> use the new PR based code review process for all new 
> submissions starting today.
> 
> * The Wiki has been updated with the process changes.
> 
>   
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
> 
>   Big thanks to Michael Kubacki for writing up all the 
>   changes based on the RFC proposal and community discussions.
> 
>   We will learn by using, so if you see anything missing or 
>   incorrect or clarifications needed, please send feedback 
>   here so the Wiki pages can be updated quickly for everyone.

$ gh pr edit kraxel:devel/emu-buildfix --add-reviewer niruiyu,ajfish
GraphQL: kraxel does not have the correct permissions to execute 
`RequestReviews` (requestReviews)

Looks like the permissions need some tweaks ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119314): https://edk2.groups.io/g/devel/message/119314
Mute This Topic: https://groups.io/mt/106355103/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/1] EmulatorPkg: fix build error.

[Gerd Hoffmann]

GasketSecSetTime is EMU_SET_TIME and returns EFI_STATUS.  Fix the
declaration accordingly.  Fixes build error with gcc 14.

/home/kraxel/projects/edk2/EmulatorPkg/Unix/Host/EmuThunk.c:429:3: error: 
initialization of ‘EFI_STATUS (__attribute__((ms_abi)) *)(EFI_TIME *)’ {aka 
‘long long unsigned int (__attribute__((ms_abi)) *)(EFI_TIME *)’} from 
incompatible pointer type ‘void (__attribute__((ms_abi)) *)(EFI_TIME *)’ 
[-Wincompatible-pointer-types]
  429 |   GasketSecSetTime,
  |   ^~~~

Cc: Andrew Fish 
Cc: Ray Ni 
Signed-off-by: Gerd Hoffmann 
---
 EmulatorPkg/Unix/Host/Gasket.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/EmulatorPkg/Unix/Host/Gasket.h b/EmulatorPkg/Unix/Host/Gasket.h
index 6dafc903cfce..71b459ddd8c7 100644
--- a/EmulatorPkg/Unix/Host/Gasket.h
+++ b/EmulatorPkg/Unix/Host/Gasket.h
@@ -140,7 +140,7 @@ GasketSecGetTime (
   OUT EFI_TIME_CAPABILITIES  *Capabilities OPTIONAL
   );
 
-VOID
+EFI_STATUS
 EFIAPI
 GasketSecSetTime (
   IN  EFI_TIME  *Time
-- 
2.45.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119309): https://edk2.groups.io/g/devel/message/119309
Mute This Topic: https://groups.io/mt/106350150/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/2] OvmfPkg: Add no hardcode version of FtdNorFlashQemuLib

[Gerd Hoffmann]

On Fri, May 24, 2024 at 11:11:41AM GMT, Marcin Juszkiewicz wrote:
> W dniu 17.05.2024 o 09:17, Chao Li via groups.io pisze:
> > This library is copied from ArmVirtPkg, in the Arm version, the value of
> > PcdFlashNvStorageVariableBase, PcdFlashNvStorageFtwWorkingBase and
> > PcdFlashNvStorageFtwSpareBase are hardcoded in INC file.
> > 
> > This version will calculate them from FDT resource and using the set PCD
> > to store when the NorFlashInitialise is called. By default, the first
> > available flash(not used for storage UEFI code) as NV variable storage
> > medium.
> > 
> > In this way, UEFI can better handle the change of flash base address,
> > which is suitable for different cpu architecture board implementation.
> > 
> > BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=4770
> > 
> > Cc: Ard Biesheuvel
> > Cc: Leif Lindholm
> > Cc: Sami Mujawar
> > Cc: Gerd Hoffmann
> > Cc: Jiewen Yao
> > Signed-off-by: Chao Li
> > Signed-off-by: Xianglai Li
> 
> Can you split it into driver itself and part which uses DT data to setup
> parameters?

Hmm?  This split exists already.  This is a library for flash detection,
which is used by the flash driver (VirtNorFlashDxe) to find flash.  If
using fdt doesn't work for sbsa-ref you can implement an alternative
without rewriting the driver itself.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119275): https://edk2.groups.io/g/devel/message/119275
Mute This Topic: https://groups.io/mt/106149595/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 00/20] NetworkPkg: CVE-2023-45236 and CVE-2023-45237

[Gerd Hoffmann]

On Fri, May 24, 2024 at 11:41:04AM GMT, Ard Biesheuvel wrote:
> On Fri, 24 May 2024 at 11:12, gaoliming via groups.io
>  wrote:
> >
> > Ard:
> >   Here is Doug PR https://github.com/tianocore/edk2/pull/5582 that includes 
> > 20 commits. You can check them.
> >
> 
> This looks fine to me in principle.
> 
> Reviewed-by: Ard Biesheuvel 
> 
> However, IIUC, the impact of this series is that all out-of-tree
> platforms that lack the right implementation of the EFI_RNG_PROTOCOL
> (i.e., using a GUID that appears in the allowlist) will lose the
> ability to do network boot. If that is a tolerable result, I am fine
> with that too, but I think it needs to be made very clear in the
> stable tag release notes.

Tested the v3 series with OVMF, results are as expected:  Without
virtio-rng-pci network boot does not work.  With virtio-rng-pci
everything is fine.

Tested-by: Gerd Hoffmann 
Acked-by: Gerd Hoffmann 

Agree that this must be noted in the release notes.

Related: I'm working on patch series adding RngDxe to OVMF with
runtime rdrand detection:
https://github.com/kraxel/edk2/commits/devel/ovmf-rdrand/

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119254): https://edk2.groups.io/g/devel/message/119254
Mute This Topic: https://groups.io/mt/106278600/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] 回复: [edk2-devel] [PATCH v2 03/13] OvmfPkg:PlatformCI: Support virtio-rng-pci

[Gerd Hoffmann]

On Fri, May 17, 2024 at 09:27:53AM GMT, Ard Biesheuvel wrote:
> On Fri, 17 May 2024 at 05:27, Doug Flick via groups.io
>  wrote:
> >
> > On ARM, we can actually do better than this: I have taken Doug's v2 and 
> > applied some changes on top to make it work with ArmVirtQemu.
> >
> > https://github.com/ardbiesheuvel/edk2/tree/doug-v2
> >
> > Ard, would you be comfortable with this patch series if I take the commits 
> > you're suggesting? I'm being asked to see what it would take to get these 
> > commits in for this release.
> 
> I won't object to that, but I'd like Gerd's take as well, given that a
> similar concern appears to apply to OVMF/x86 IIUC.

I think including RngDxe in OvmfPkg is not an option.  That would
be a silent regression on the random number quality delivered by
EFI_RNG_PROTOCOL because OvmfPkg uses BaseRngLibTimerLib.

Switching to BaseRngLib is an easy way out for physical platforms
with a sufficient recent processor.  OVMF can not assume the rdrand
instruction is available, so that is not possible.

So short-term (i.e. 2024-05 stable tag) the only option I see is
depending on virtio-rng.  Which is a regression too (network booting
without '-device virtio-rng-pci' breaks), but it is an obvious failure
with an easy fix.  Not an ideal solution, but much better than a
regression which can easily go unnoticed.

Longer term it probably makes sense to have a EFI_RNG_PROTOCOL driver
using the rdrand instruction and runtime detection whenever the
instruction is available or not.  Either by adapting RngDxe accordingly,
or by having an OVMF-specific driver handling the runtime detection.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119016): https://edk2.groups.io/g/devel/message/119016
Mute This Topic: https://groups.io/mt/106013302/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 10/18] UefiCpuPkg:Relocate AP to new safe buffer in PeiMpLib

[Gerd Hoffmann]

On Tue, May 14, 2024 at 05:17:51AM GMT, Ni, Ray wrote:
> Gerd,
> I agree that the logic might be duplicated in multi places.
> 
> But even CPU supports 1G paging, caller can decide whether to use 1G paging 
> or 2M paging, or 4K paging.
> Using a single API to encapsulate the entire logic may not seem flexible.

Sure, I don't want take away that flexibility.  A caller might also
prepare page tables for a paging mode not matching the current CPU
paging mode (i.e. 32-bit PEI preparing page tables for 64-bit DXE).

> Maybe, a lib API to detect 1G paging capability can be added to CpuLib.

Yep, that is exactly what I think would be useful.  Add a

PAGING_MODE PageTableBestMode(VOID);

function with this code to CpuPageTableLib, so callers have the option
to use

PagingMode = PageTableBestMode();

instead of duplicating the code block.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118881): https://edk2.groups.io/g/devel/message/118881
Mute This Topic: https://groups.io/mt/106018135/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 10/18] UefiCpuPkg:Relocate AP to new safe buffer in PeiMpLib

[Gerd Hoffmann]

  Hi,

> +  if (sizeof (UINTN) == sizeof (UINT64)) {
> +//
> +// Check Page5Level Support or not.
> +//
> +Cr4.UintN = AsmReadCr4 ();
> +Page5LevelSupport = (Cr4.Bits.LA57 ? TRUE : FALSE);
> +
> +//
> +// Check Page1G Support or not.
> +//
> +Page1GSupport = FALSE;
> +AsmCpuid (CPUID_EXTENDED_FUNCTION, , NULL, NULL, NULL);
> +if (RegEax >= CPUID_EXTENDED_CPU_SIG) {
> +  AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, );
> +  if (RegEdx.Bits.Page1GB != 0) {
> +Page1GSupport = TRUE;
> +  }
> +}
> +
> +//
> +// Decide Paging Mode according Page5LevelSupport & Page1GSupport.
> +//
> +if (Page5LevelSupport) {
> +  PagingMode = Page1GSupport ? Paging5Level1GB : Paging5Level;
> +} else {
> +  PagingMode = Page1GSupport ? Paging4Level1GB : Paging4Level;
> +}
> +  } else {
> +PagingMode = PagingPae;
> +  }

I'm wondering whenever CpuPageTableLib should get a function for this?
I suspect there a multiple places in edk2 which will need this
functionality ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118862): https://edk2.groups.io/g/devel/message/118862
Mute This Topic: https://groups.io/mt/106018135/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] 回复: [edk2-devel] [PATCH v2 03/13] OvmfPkg:PlatformCI: Support virtio-rng-pci

[Gerd Hoffmann]

On Sat, May 11, 2024 at 10:40:23AM GMT, Ard Biesheuvel wrote:
> As I pointed out before, on the ARM side there are a few intersecting
> issues with these changes. (On x86, this is mostly avoided due to the
> fact that RDRAND is universally supported)

Well, it's not that easy on x86 either.

Current state of affairs is that the time based LibRng is used, all
OvmfPkg / ArmVirtPkg have this:

  RngLib|MdeModulePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf

So, this is what will be used if something uses LibRng.

On x64 RngDxe will just use call LibRng too.  So adding RngDxe will
effectively make BaseRngLibTimerLib available via EFI_RNG_PROTOCOL.

In case '-device virtio-rng-pci' is present we now have *two* drivers
providing EFI_RNG_PROTOCOL.  What will happen in this case?  What we
surely not want is RngDxe being used in case we have a virtio-rng
device ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118861): https://edk2.groups.io/g/devel/message/118861
Mute This Topic: https://groups.io/mt/106013302/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Update VMM Hob list check to support new resource attributes

[Gerd Hoffmann]

On Thu, May 09, 2024 at 01:27:07PM GMT, Du Lin wrote:
> Encrypted and Special Purpose resource attributes are introduced in
> PI 1.8 Specification. This patch is to update VMM Hob list integrity
> check to recognise these resource attributes.
> 
> Cc: Ard Biesheuvel 
> Cc: Gerd Hoffmann 
> Cc: Jiewen Yao 
> Signed-off-by: Du Lin 

> +
> EFI_RESOURCE_ATTRIBUTE_ENCRYPTED|
> +
> EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE |

Acked-by: Gerd Hoffmann 

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118859): https://edk2.groups.io/g/devel/message/118859
Mute This Topic: https://groups.io/mt/105996363/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Assistance Needed: ArmVirtPkg

[Gerd Hoffmann]

On Mon, May 06, 2024 at 10:22:07PM GMT, Doug Flick wrote:
> All,
> 
> In order to patch Tianocore Bugzilla issues and CVEs:
>  4541 – Bug 08 - edk2/NetworkPkg: Predictable TCP ISNs 
> (tianocore.org)
> and
> 4542 – Bug 09 - edk2/NetworkPkg: Use of a Weak PseudoRandom Number Generator 
> (tianocore.org)
> 
> I've added as a dependency Hash2CryptoDxe and RngDxe lib to NetworkPkg. I've 
> been able to add the relevant libraries to the DSCs of OvmfPkg and 
> EmulatorPkg however I'm seeing odd behavior with ArmVirtPkg.
> 
> Would someone more knowledgeable with ArmVirtPkg take a look this PR.

Both OVMF and ArmVirt use the virtio random number device as
source for random numbers.

Driver: OvmfPkg/VirtioRngDxe
Some Background: https://wiki.qemu.org/Features/VirtIORNG

Typically the virtio rng device is present in virtual machine
configurations.  It might be missing though.

I'd recommend:
  (1) Do *not* add RngDxe to OvmfPkg and ArmVirtPkg dsc files, instead
  continue to depend on VirtioRngDxe.
  (2) Keep the time-based not-really-random RNG generator as fallback in
  case EFI_RNG_PROTOCOL is not present (possibly requiring a PCD
  being set so the fallback option can be disabled at build time).

HTH & take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118642): https://edk2.groups.io/g/devel/message/118642
Mute This Topic: https://groups.io/mt/105949609/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4 0/3] OvmfPkg: Don't make APIC MMIO accesses with encryption bit set

[Gerd Hoffmann]

On Wed, May 01, 2024 at 02:03:37PM GMT, Michael Roth wrote:
> For the most part, OVMF will clear the encryption bit for MMIO regions,
> but there is currently one known exception during SEC when the APIC
> base address is accessed via MMIO with the encryption bit set for
> SEV-ES/SEV-SNP guests. In the case of SEV-SNP, this requires special
> handling on the hypervisor side which may not be available in the
> future[1], so make the necessary changes in the SEC-configured page
> table to clear the encryption bit for 4K region containing the APIC
> base address.
> 
> Since CpuPageTableLib is used to handle the splitting, some additional
> care must be taken to clear the C-bit in all non-leaf PTEs since the
> library expects that to be the case. Add handling for that when setting
> up the SEC page table.
> 
> While here, drop special handling for the APIC base address in the
> SEV-ES/SNP #VC handler.

Series:
Reviewed-by: Gerd Hoffmann 

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118502): https://edk2.groups.io/g/devel/message/118502
Mute This Topic: https://groups.io/mt/105849106/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH ovmf v2 0/5] Enable AMD SEV-ES DebugSwap

[Gerd Hoffmann]

  Hi,

> How do I proceed from here? Repost patches here or that pull request will
> do? I did not change anything besides spaces and CCs. Thanks,

Patch review happens on the mailing list, so please post v3 series.

thanks,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118501): https://edk2.groups.io/g/devel/message/118501
Mute This Topic: https://groups.io/mt/105668050/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3] OvmfPkg: Don't make APIC MMIO accesses with encryption bit set

[Gerd Hoffmann]

On Fri, Apr 26, 2024 at 02:50:07PM GMT, Roth, Michael via groups.io wrote:
> For the most part, OVMF will clear the encryption bit for MMIO regions,
> but there is currently one known exception during SEC when the APIC
> base address is accessed via MMIO with the encryption bit set for
> SEV-ES/SEV-SNP guests. In the case of SEV-SNP, this requires special
> handling on the hypervisor side which may not be available in the
> future[1], so make the necessary changes in the SEC-configured page
> table to clear the encryption bit for 4K region containing the APIC
> base address.
> 
> Since CpuPageTableLib is used to handle the splitting, some additional
> care must be taken to clear the C-bit in all non-leaf PTEs since the
> library expects that to be the case. Add handling for that when setting
> up the SEC page table.
> 
> While here, drop special handling for the APIC base address in the
> SEV-ES/SNP #VC handler.

Please split this into smaller patches (one for each commit message
paragraph).

> -0x011000|0x00F000
> +0x011000|0x001000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableSize

Hmm, I think we need to figure how we'll organize page table allocation
best.  This becomes a bit messy, even more so once we enable 5-level
paging for sev because one page isn't enough then ...

Is it possible to move the GHCB page setup to SecCoreStartupWithStack()
too?  If so it would be nice to setup both GHCB and APIC page in C code,
and it should also allow to cleanup the page table memory reservation
and simplify the reset vector.

Note: I'm fine with this as-is for the next stable tag, the comments are
more for the next patch series, when enabling 5-level paging for sev,
where this must be touched again.

> +  ApicAddress = (UINT64)GetLocalApicBaseAddress ();
> +  Buffer  = (VOID *)(UINTN)FixedPcdGet32 (PcdOvmfSecApicPageTableBase);
> +  Cr3 = AsmReadCr3 ();
> +
> +  MapAttribute.Uint64 = ApicAddress;
> +  MapAttribute.Bits.Present   = 1;
> +  MapAttribute.Bits.ReadWrite = 1;
> +  MapMask.Uint64  = MAX_UINT64;
> +  BufferSize  = SIZE_4KB;
> +
> +  Status = PageTableMap (
> + (UINTN *),
> + Paging4Level,
> + Buffer,
> + ,
> + ApicAddress,
> + SIZE_4KB,
> + ,
> + ,
> + NULL
> + );

Very nice.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118436): https://edk2.groups.io/g/devel/message/118436
Mute This Topic: https://groups.io/mt/105757219/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4 00/14] Add SmmRelocationLib

[Gerd Hoffmann]

On Fri, Apr 26, 2024 at 08:17:06PM GMT, Jiaxin Wu wrote:
> PR: https://github.com/tianocore/edk2/pull/5546
> 
> Intel plans to separate the smbase relocation logic from
> PiSmmCpuDxeSmm driver, and the related behavior will be
> moved to the new interface defined by the SmmRelocationLib
> class.
> 
> The SmmRelocationLib class provides the SmmRelocationInit()
> interface for platform to do the smbase relocation, which
> shall provide below 2 functionalities:
> 1. Relocate smbases for each processor.
> 2. Create the gSmmBaseHobGuid HOB.
> 
> With SmmRelocationLib, PiSmmCpuDxeSmm driver (which runs at
> a later phase) can be simplfied as below for SMM init:
> 1. Consume the gSmmBaseHobGuid HOB for the relocated smbases
> for each Processor.
> 2. Execute the early SMM Init.

Series:
Tested-by: Gerd Hoffmann 
Acked-by: Gerd Hoffmann 

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118433): https://edk2.groups.io/g/devel/message/118433
Mute This Topic: https://groups.io/mt/105748911/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Set PcdCpuMaxLogicalProcessorNumber in OvmfXen

[Gerd Hoffmann]

  Hi,
 
> It's a bit more complicated than setting it at build time, but we can
> always ask Xen how many vcpu we have and set the PCD accordingly. This
> is something that can happen in OvmfPkg/XenPlatformPei module.

Exactly.

> But to be honest, I don't know if it's worth it, because I don't know the
> downside of having a higher value for PcdCpuMaxLogicalProcessorNumber.

Well, the downside is that you have to keep ovmf and xen in sync
manually.  Your call.  If you prefer to do it that way I have no
objections, just wanted point out the alternative approach.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118301): https://edk2.groups.io/g/devel/message/118301
Mute This Topic: https://groups.io/mt/105721898/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 3/7] OvmfPkg: Add the QemuFwCfgMmioLib PEI stage version

[Gerd Hoffmann]

On Thu, Apr 25, 2024 at 04:06:13PM +0800, Chao Li wrote:
> Hi Gerd,
> 
> 
> Thanks,
> Chao
> On 2024/4/25 15:53, Gerd Hoffmann wrote:
> >Hi,
> > 
> > > +UINTN  mFwCfgSelectorAddress;
> > > +UINTN  mFwCfgDataAddress;
> > > +UINTN  mFwCfgDmaAddress;
> > Hmm, global variables for PEI?  I think the point of storing these in
> > the HOB is to avoid the need for global variables?  Also does that work
> > when running PEI in-place from flash?
> I think it would be useful if some platforms(not LoongArch) could use the
> global variables in PEI, because the global variables are faster.

Performance isn't my main concern here, I very much prefer code which is
easy to maintain.  Taking the same code path on all platforms is good
for that.  It's less code and it also makes testing easier.  The risk of
breaking loongarch when changing something for riscv or arm is much
lower if all platforms work the same way.

I'd suggest to first refactor the existing DXE code to use a HOB instead
of global variables.  Have a helper function which looks up the HOB and
returns a pointer to the configuration struct.  That helper function can
be slightly different for DXE/PEI, the DXE variant can cache the pointer
to the struct in a global variable so it needs to do the lookup only
once.

> > > +  UINT64FwCfgDataSize;
> > > +  UINT64FwCfgDmaAddress;
> > > +  UINT64FwCfgDmaSize;
> > First thing this function should do is check whenever the HOB already
> > exists.  Should that be the case there is no need to parse the device
> > tree.
> This is a constructor in PEI, that has to parse the device tree and then
> build the HOBs.

This is a library, so it can be linked into multiple PEI and DXE
modules.  So it must be prepared to run multiple times.  On the
second and all following runs the HOB will already exist.

The DXE variant will need the check for sure.  I'd strongly suggest to
add it to the PEI variant too, even though it might not be needed right
now because PlatformPei is the only PEI module using the library.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118282): https://edk2.groups.io/g/devel/message/118282
Mute This Topic: https://groups.io/mt/105724970/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 3/7] OvmfPkg: Add the QemuFwCfgMmioLib PEI stage version

[Gerd Hoffmann]

  Hi,

> +UINTN  mFwCfgSelectorAddress;
> +UINTN  mFwCfgDataAddress;
> +UINTN  mFwCfgDmaAddress;

Hmm, global variables for PEI?  I think the point of storing these in
the HOB is to avoid the need for global variables?  Also does that work
when running PEI in-place from flash?

> +RETURN_STATUS
> +EFIAPI
> +QemuFwCfgInitialize (
> +  VOID
> +  )
> +{
> +  VOID  *DeviceTreeBase;
> +  INT32 Node;
> +  INT32 Prev;
> +  CONST CHAR8   *Type;
> +  INT32 Len;
> +  CONST UINT64  *Reg;
> +  UINT64FwCfgSelectorAddress;
> +  UINT64FwCfgSelectorSize;
> +  UINT64FwCfgDataAddress;
> +  UINT64FwCfgDataSize;
> +  UINT64FwCfgDmaAddress;
> +  UINT64FwCfgDmaSize;

First thing this function should do is check whenever the HOB already
exists.  Should that be the case there is no need to parse the device
tree.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118277): https://edk2.groups.io/g/devel/message/118277
Mute This Topic: https://groups.io/mt/105724970/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/7] OvmfPkg: Add the way of HOBs in QemuFwCfgLibMmio

[Gerd Hoffmann]

  Hi,

> +EFI_GUID  mFwCfgSelectorAddressGuid = FW_CONFIG_SELECTOR_ADDRESS_HOB_GUID;
> +EFI_GUID  mFwCfgDataAddressGuid = FW_CONFIG_DATA_ADDRESS_HOB_GUID;
> +EFI_GUID  mFwCfgDmaAddressGuid  = FW_CONFIG_DMA_ADDRESS_HOB_GUID;

Oh.  I assumed that would be obvious (because it's common practice for
HOBs), but I was thinking about a single HOB containing a struct with
all three values instead of a separate HOB for each value.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118275): https://edk2.groups.io/g/devel/message/118275
Mute This Topic: https://groups.io/mt/105724969/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Set PcdCpuMaxLogicalProcessorNumber in OvmfXen

[Gerd Hoffmann]

On Wed, Apr 24, 2024 at 02:36:32PM +0100, Alejandro Vallejo wrote:
> Bump the compile-time constant for maximum processor count from 64 to 128
> in order to allow that many vCPUs to be brought online on Xen guests with
> the default OVMF configuration.

> +  # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|128

Note that this is a dynamic PCD, so you can set it at runtime to the
number of vcpus present in the VM.  See MaxCpuCountInitialization() in
OvmfPkg/PlatformPei/Platform.c for example.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118274): https://edk2.groups.io/g/devel/message/118274
Mute This Topic: https://groups.io/mt/105721898/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 08/13] OvmfPkg/PlatformInitLib: Create gEfiSmmSmramMemoryGuid

[Gerd Hoffmann]

  Hi,

> Let me explain more why need this change:
> 
> 1. The EFI_SMM_SMRAM_MEMORY_GUID HOB, as defined in the PI specification, is 
> used to describe the SMRAM memory regions supported by the platform. This HOB 
> should be produced during the memory detection phase to align with the PI 
> spec.
> 
> 2. In addition to the memory reserved for ACPI S3 resume, an increasing 
> number of features require reserving SMRAM for specific purposes, such as 
> SmmRelocation. Other advanced features in Intel platforms also necessitate 
> this. The implementation of these features varies and is entirely dependent 
> on the platform. This is why an increasing number of platforms are adopting 
> the EFI_SMM_SMRAM_MEMORY_GUID HOB for SMRAM description.
> 
> 3. It is crucial that the SMRAM information remains consistent when retrieved 
> from the platform, whether through the SMM ACCESS PPI/Protocol or the 
> EFI_SMM_SMRAM_MEMORY_GUID HOB. Inconsistencies can lead to unexpected issues, 
> most commonly memory region conflicts.
> 
> 4. The SMM ACCESS PPI/Protocol can be naturally implemented for general use. 
> The common approach is to utilize the EFI_SMM_SMRAM_MEMORY_GUID HOB. For 
> reference, see the existing implementation in the EDK2 repository at 
> edk2/UefiPayloadPkg/SmmAccessDxe/SmmAccessDxe.inf and 
> edk2-platforms/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.inf.
>  
> 
> For the reasons mentioned, we are moving the SMRAM memory regions to HOBs and 
> allowing SMM access to consume these HOBs.
> 
> I will add the above info into commit message.

Thanks.

Creating the EFI_SMM_SMRAM_MEMORY_GUID HOB should be moved to its own
function.  Also move over the comments from SmmAccess describing the
regions please.

Adding a reference to the PI spec section describing this would be good
too.

> > Storing anything SMM related outside SMRAM makes me nervous.
> > I'd strongly suggest to avoid that.
> > 
> > It might be that in this specific case it is not a problem.  But it
> > needs very careful review of the implications (which I have not done)
> > and you have to hope you don't miss a possible attack vector, such as
> > someone modifying the HOB and the firmware then storing SMM data + code
> > outside SMRAM.
> 
> Understand, but here is the case we can record the info in non-smram
> since PI spec exposes that, there is no difference the info retrieved
> from PPI/ non-smm Protocol or the non-smram.

Good point.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118273): https://edk2.groups.io/g/devel/message/118273
Mute This Topic: https://groups.io/mt/105593577/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 00/13] Add SmmRelocationLib

[Gerd Hoffmann]

  Hi,

> That means the SMMRevId is 0_xx64h for AMD64 processor. But I am not
> sure what the value is for AMD32 processor. Maybe 0 according to the
> OVMF logic.

The smm emulation in the linux kernel uses 0 and 0x64.

> But, I am very suspicious about the logic in AMD's version as below:
> --- AMD's version
>   SmmSaveStateRegisterLma = (UINT8)EFI_MM_SAVE_STATE_REGISTER_LMA_32BIT;
> 
>   LMAValue = (UINT32)AsmReadMsr64 (EFER_ADDRESS) & LMA;
>   if (LMAValue) {
> SmmSaveStateRegisterLma = (UINT8)EFI_MM_SAVE_STATE_REGISTER_LMA_64BIT;
>   }
> ---
> The above logic detects the current CPU mode and 64bit save state area layout 
> is used if it's running in 64bit.

> But if a AMD64 CPU runs in 32bit mode, the above logic causes the
> 32bit save state area layout is used. It's not right!  The save state
> area layout does not depend on the CPU running mode, but whether it's
> a legacy CPU or a 64-capable CPU.

Well, that is not entirely clear to me.  Could it be 64-bit processors
support both 32-bit and 64-bit format, for backward compatibility
reasons?

So OvmfPkgIa32 builds could use the 32-bit format, OvmfPkgX64 builds use
the 64-bit format, everything works fine?

The tricky corner case is OvmfPkgIa32X64, where (after applying this
series) 32-bit PEI should setup things for 64-bit SMM/DXE, and checking
the current processor mode will not give use the result we need.

> Jiaxin, I agree that the confusion should be cleaned up by AMD
> experts. Let's not change any existing behavior.

Agree.  Tom?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118272): https://edk2.groups.io/g/devel/message/118272
Mute This Topic: https://groups.io/mt/105593568/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Don't make APIC MMIO accesses with encryption bit set

[Gerd Hoffmann]

  Hi,

> > That is incompatible with 5-level paging.  The current reset vector will
> > never turn on 5-level paging in case SEV is active because we have more
> > incompatibilities elsewhere (BaseMemEncryptSevLib IIRC).  But still,
> > it's moving things into the wrong direction ...
> 
> Tom had mentioned this eventuality and we discussed it to an extent. AIUI
> once we make that switch then most of this function could be replaced with
> a call into the library to handle the splitting, and similar re-work would
> need to be done for handling splitting the area for the GHCB page which is
> also currently done with direct page table manipulation. So while it
> does sort of move in the wrong direction, I don't think it would
> significantly complicate things as far as making that transition.

GHCB page is handled with asm code in the reset vector and I'm not
sure it can be moved out there as the page will be needed quite early
in firmware boot.

> > Ideally CpuPageTableLib should be used for this.
> 
> What's the outlook for moving CpuPageTableLib before the next OVMF release?
> My concern is that once SNP KVM support goes upstream (which is currently
> looking to be within kernel 6.10 timeframe), SNP guest support in OVMF will
> be completely broken without a fix like this for APIC MMIO accesses.

Fixing this surely should be done before the may stable tag.  If
CpuPageTableLib changes are needed, then going the CpuPageTableLib
route is a bit risky indeed.

I don't think we need CpuPageTableLib changes though.  At least not for
the reason (page table memory allocation) mentioned by Tom.  The patch
reserves a page in MEMFD, and simply giving that page to CpuPageTableLib
should work just fine.

> One thing to maybe get ahead of is the fact that splitting pages with
> 5-level paging will require having 2 pages reserved for GHCB instead of
> the 1 we have currently, and 2 pages reserved for APIC range instead of
> the 1 proposed by this patch (since we'd need to not only split a 2MB PTE
> to 4KB, but the upper 1GB PTE to 2MB).

The first GB is covered by 2M pages even with 5-level paging, exactly to
simplify the GHCB setup.

For APIC + 5-level paging we'll indeed need a second page table page.

> Do we know enough about what that sort of allocation/reserve logic would
> look to start modifying PcdOvmfSecPageTablesBase,
> PcdOvmfSecGhcbPageTableBase, and PcdOvmfSecApicPageTableBase to start
> preping for such a change?

Well, CpuPageTableLib simply expects getting a buffer passed in with
page table memory.  So allocation is fully in the hands of the caller.
It's also possible to call the library without buffer and get back the
number of pages which will be needed to apply the changes, so the caller
can allocate exactly what will be needed.  That would not be needed here
given we need a big enough pool of pages in MEMFD anyway.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118218): https://edk2.groups.io/g/devel/message/118218
Mute This Topic: https://groups.io/mt/105698125/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Don't make APIC MMIO accesses with encryption bit set

[Gerd Hoffmann]

  Hi,

> > Ideally CpuPageTableLib should be used for this.
> 
> CpuPageTableLib will need to be modified in order for it to be used at this
> (Sec) stage. In order to work in Sec - either the caller will have to supply
> a list of pages that can be used if pagetable entries need to be allocated
> for splits

I don't think so.  PageTableMap() has the 'Buffer' parameter for passing
in page table memory.  And the patch reserves a page in MEMFD.  Handing
that page over to PageTableMap() should work just fine, no?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118216): https://edk2.groups.io/g/devel/message/118216
Mute This Topic: https://groups.io/mt/105698125/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 08/13] OvmfPkg/PlatformInitLib: Create gEfiSmmSmramMemoryGuid

[Gerd Hoffmann]

  Hi,

> Transfer to 16bit OS waking vector - 991F0 > hang here!!!

That is the last ovmf message of a successful S3 resume, after that the
OS should have back control.  Looks fine to me.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118213): https://edk2.groups.io/g/devel/message/118213
Mute This Topic: https://groups.io/mt/105593577/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 08/13] OvmfPkg/PlatformInitLib: Create gEfiSmmSmramMemoryGuid

[Gerd Hoffmann]

  Hi,

> > First, smram allocation doesn't work that way.  Have a look at
> > OvmfPkg/SmmAccess.  I guess that easily explains why this series
> > breaks S3 suspend.
> 
> Oh? Could you explain a bit more for 1) how smram allocation works? 2) what's 
> the possible reason break the S3? I haven't check yet. 

SmramInternal.c handles that.  It creates two regions, one is a page at
the start of SMRAM where S3 state is stored (and marked as allocated),
one is all the rest.

So, if you need some smram to initialize SMM in PEI I'd suggest to
either add a third region, or make the first region larger.

It's not clear to me why you put the logic upside down and introduce
that HOB in the first place.

> > Second, storing these descriptors in a HOB (which is PEI memory)
> > is questionable from a security point of view.
> 
> HOB is only to expose the SMRAM address and size, not the contents in smram, 
> what's the security concern?

Storing anything SMM related outside SMRAM makes me nervous.
I'd strongly suggest to avoid that.

It might be that in this specific case it is not a problem.  But it
needs very careful review of the implications (which I have not done)
and you have to hope you don't miss a possible attack vector, such as
someone modifying the HOB and the firmware then storing SMM data + code
outside SMRAM.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118212): https://edk2.groups.io/g/devel/message/118212
Mute This Topic: https://groups.io/mt/105593577/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Don't make APIC MMIO accesses with encryption bit set

[Gerd Hoffmann]

On Tue, Apr 23, 2024 at 03:59:58PM -0500, Michael Roth wrote:
> For the most part, OVMF will clear the encryption bit for MMIO regions,
> but there is currently one known exception during SEC when the APIC
> base address is accessed via MMIO with the encryption bit set for
> SEV-ES/SEV-SNP guests.

what exactly accesses the lapic that early?

> +/**
> +  Map known MMIO regions unencrypted if SEV-ES is active.
> +
> +  During early booting, page table entries default to having the encryption 
> bit
> +  set for SEV-ES/SEV-SNP guests. In cases where there is MMIO to an address, 
> the
> +  encryption bit should be cleared. Clear it here for any known MMIO accesses
> +  during SEC, which is currently just the APIC base address.
> +
> +**/
> +VOID
> +SecMapApicBaseUnencrypted (
> +  VOID
> +  )
> +{
> +  PAGE_MAP_AND_DIRECTORY_POINTER  *Level4Entry;
> +  PAGE_MAP_AND_DIRECTORY_POINTER  *Level3Entry;
> +  PAGE_MAP_AND_DIRECTORY_POINTER  *Level2Entry;
> +  PAGE_TABLE_4K_ENTRY *Level1Entry;
> +  SEC_SEV_ES_WORK_AREA*SevEsWorkArea;
> +  PHYSICAL_ADDRESSCr3;
> +  UINT64  ApicAddress;
> +  UINT64  PgTableMask;
> +  UINT32  Level1Page;
> +  UINT64  Level1Address;
> +  UINT64  Level1Flags;
> +  UINTN   PteIndex;
> +
> +  if (!SevEsIsEnabled ()) {
> +return;
> +  }

That is incompatible with 5-level paging.  The current reset vector will
never turn on 5-level paging in case SEV is active because we have more
incompatibilities elsewhere (BaseMemEncryptSevLib IIRC).  But still,
it's moving things into the wrong direction ...

Ideally CpuPageTableLib should be used for this.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118211): https://edk2.groups.io/g/devel/message/118211
Mute This Topic: https://groups.io/mt/105698125/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 00/13] Add SmmRelocationLib

[Gerd Hoffmann]

On Wed, Apr 24, 2024 at 03:56:56AM +, Wu, Jiaxin wrote:
> Hi Gerd,
> 
> AMD version is not work for IA32X64 ovmf.
> 
> I checked the detailed: CpuSaveState->x64 is always used for OVMF no matter 
> IA32 or X64, while AMD is not, which is decided by the MSR EFER_ADDRESS LMA 
> bit check.

Hmm, probably because only PEI runs in 32-bit mode whereas DXE and SMM
run in 64-bit mode, so 32-bit PEI has to prepare things for the 64-bit
SMM.

> There is a potential issue/open in OVMF why need use the X64
> CpuSaveState for IA32. Before this open resolved, I still prefer to
> keep use the ovmf specific lib instance.

Yes, lets stick to the ovmf version for now, and maybe remove it later
when fixing the ia32 ovmf builds.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118204): https://edk2.groups.io/g/devel/message/118204
Mute This Topic: https://groups.io/mt/105593568/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Adjust the QemuFwCfgLibMmio and add PEI stage

[Gerd Hoffmann]

On Wed, Apr 24, 2024 at 09:57:50AM +0800, Chao Li wrote:
> Hi Gerd and Ard,
> 
> Can I submit the V2 this week? I want all OvmfPkg changes to be meged before
> the 202405 feature freeze.

Yea, go ahead, lets stick to the PCD approach, given that Ard seems to
not have objections to that ;)

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118203): https://edk2.groups.io/g/devel/message/118203
Mute This Topic: https://groups.io/mt/105573103/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 1/1] OvmfPkg/VirtHstiDxe: do not load driver in confidential guests

[Gerd Hoffmann]

The VirtHstiDxe does not work in confidential guests.  There also isn't
anything we can reasonably test, neither flash storage nor SMM mode will
be used in that case.  So just skip driver load when running in a
confidential guest.

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Fixes: 506740982bba ("OvmfPkg/VirtHstiDxe: add code flash check")
Signed-off-by: Gerd Hoffmann 
Tested-by: Srikanth Aithal 
---
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf | 1 +
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c   | 6 ++
 2 files changed, 7 insertions(+)

diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
index 9514933011e8..b5c237288766 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
@@ -49,6 +49,7 @@ [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
 
 [Pcd]
+  gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr
   gUefiOvmfPkgTokenSpaceGuid.PcdBfvBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
 
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
index b6e53a1219d1..efaff0d1f3cb 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
@@ -17,6 +17,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -140,6 +141,11 @@ VirtHstiDxeEntrypoint (
   EFI_STATUS   Status;
   EFI_EVENTEvent;
 
+  if (PcdGet64 (PcdConfidentialComputingGuestAttr)) {
+DEBUG ((DEBUG_INFO, "%a: confidential guest\n", __func__));
+return EFI_UNSUPPORTED;
+  }
+
   DevId = VirtHstiGetHostBridgeDevId ();
   switch (DevId) {
 case INTEL_82441_DEVICE_ID:
-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118181): https://edk2.groups.io/g/devel/message/118181
Mute This Topic: https://groups.io/mt/105705705/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 4/5] OvmfPkg/VirtHstiDxe: add code flash check

[Gerd Hoffmann]

On Tue, Apr 23, 2024 at 07:14:04PM +0530, Aithal, Srikanth wrote:
> Correcting.
> 
> On 4/23/2024 7:09 PM, Aithal, Srikanth wrote:
> > Hello,
> > 
> > Todays OVMF/edk2 master branch is breaking AMD SEV-ES guest boot with
> > OvmfX64 package, where as sev-es guest boots fine with AmdSev package.
> > 
> > Git bisect pointed to below commit as bad, going back to previous commit
> > i.e ddc43e7a SEV-ES guest boots fine with OvmfX64 package:
> Git bisect pointed to below commit as bad, going back to previous commit i.e
> ddc43e7a SEV-ES guest boots fine. With OVMF/edk2 master branch SEV-ES guest
> boots fine with *AmdSev *package:

The tests don't make much sense in confidential guests (both sev and
tdx).  Which why the driver is not included in the AmdSevPkg builds.

Not activating the driver in confidential guests should fix that, test
patch below.

take care,
  Gerd

diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
index 9514933011e8..b5c237288766 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
@@ -49,6 +49,7 @@ [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
 
 [Pcd]
+  gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr
   gUefiOvmfPkgTokenSpaceGuid.PcdBfvBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
 
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
index b6e53a1219d1..efaff0d1f3cb 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
@@ -17,6 +17,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -140,6 +141,11 @@ VirtHstiDxeEntrypoint (
   EFI_STATUS   Status;
   EFI_EVENTEvent;
 
+  if (PcdGet64 (PcdConfidentialComputingGuestAttr)) {
+DEBUG ((DEBUG_INFO, "%a: confidential guest\n", __func__));
+return EFI_UNSUPPORTED;
+  }
+
   DevId = VirtHstiGetHostBridgeDevId ();
   switch (DevId) {
 case INTEL_82441_DEVICE_ID:



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118142): https://edk2.groups.io/g/devel/message/118142
Mute This Topic: https://groups.io/mt/105667072/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms] SbsaQemu: move code outside of methods in DSDT

[Gerd Hoffmann]

  Hi,

> +Name (RBUF, ResourceTemplate() {
> +Memory32Fixed (ReadWrite,
> +   FixedPcdGet32 (PcdPlatformXhciBase),
> +   FixedPcdGet32 (PcdPlatformXhciSize))
> +Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 43 }
> +})
>  Method (_CRS, 0x0, Serialized) {
> -Name (RBUF, ResourceTemplate() {

If the resources never change _CRS doesn't need to be a method, you can
go for "Name (_CRS, ResourceTemplate () ..." instead.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118134): https://edk2.groups.io/g/devel/message/118134
Mute This Topic: https://groups.io/mt/105666235/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Gerd Hoffmann]

On Fri, Apr 19, 2024 at 11:21:46AM -0700, Adam Dunlap wrote:
> Ensure that when a #VC exception happens, the instruction at the
> instruction pointer matches the instruction that is expected given the
> error code. This is to mitigate the ahoi WeSee attack [1] that could
> allow hypervisors to breach integrity and confidentiality of the
> firmware by maliciously injecting interrupts. This change is a
> translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
> instruction emulation somewhat")
> 
> [1] https://ahoi-attacks.github.io/wesee/
> 
> Cc: Borislav Petkov (AMD) 
> Cc: Tom Lendacky 
> Signed-off-by: Adam Dunlap 

Reviewed-by: Gerd Hoffmann 

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118132): https://edk2.groups.io/g/devel/message/118132
Mute This Topic: https://groups.io/mt/105623545/21656
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 08/13] OvmfPkg/PlatformInitLib: Create gEfiSmmSmramMemoryGuid

[Gerd Hoffmann]

  Hi,

> +Hob.Raw = BuildGuidHob (
> +,
> +BufferSize
> +);

> +SmramHobDescriptorBlock = 
> (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *)(Hob.Raw);

> +SmramHobDescriptorBlock->Descriptor[0].PhysicalStart = 
> PlatformInfoHob->LowMemory - TsegSize;
> +SmramHobDescriptorBlock->Descriptor[0].CpuStart  = 
> PlatformInfoHob->LowMemory - TsegSize;
> +SmramHobDescriptorBlock->Descriptor[0].PhysicalSize  = EFI_PAGE_SIZE;
> +SmramHobDescriptorBlock->Descriptor[0].RegionState   = EFI_SMRAM_CLOSED 
> | EFI_CACHEABLE | EFI_ALLOCATED;

> +SmramHobDescriptorBlock->Descriptor[1].PhysicalStart = 
> SmramHobDescriptorBlock->Descriptor[0].PhysicalStart + EFI_PAGE_SIZE;
> +SmramHobDescriptorBlock->Descriptor[1].CpuStart  = 
> SmramHobDescriptorBlock->Descriptor[0].CpuStart + EFI_PAGE_SIZE;
> +SmramHobDescriptorBlock->Descriptor[1].PhysicalSize  = TsegSize - 
> EFI_PAGE_SIZE;
> +SmramHobDescriptorBlock->Descriptor[1].RegionState   = EFI_SMRAM_CLOSED 
> | EFI_CACHEABLE;

This is not going to fly.

First, smram allocation doesn't work that way.  Have a look at
OvmfPkg/SmmAccess.  I guess that easily explains why this series
breaks S3 suspend.

Second, storing these descriptors in a HOB (which is PEI memory)
is questionable from a security point of view.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118131): https://edk2.groups.io/g/devel/message/118131
Mute This Topic: https://groups.io/mt/105593577/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 00/13] Add SmmRelocationLib

[Gerd Hoffmann]

On Tue, Apr 23, 2024 at 07:31:18AM +, Wu, Jiaxin wrote:
> Thanks Gerd, I will try the S3 on OVMF.
> 
> And for AmdSmmRelocationLib usage in OVMF, do you prefer:
> 1. use the AmdSmmRelocationLib directly in this patch set? Or
> 2. still keep the original to create the OvmfPkg/SmmRelocationLib, and clean 
> the code in the future patch?

Clear preference for (1), why introduce OvmfPkg/SmmRelocationLib only to
delete it shortly thereafter?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118130): https://edk2.groups.io/g/devel/message/118130
Mute This Topic: https://groups.io/mt/105593568/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 2/5] OvmfPkg: Add VirtHstiDxe to OVMF firmware build

[Gerd Hoffmann]

From: Konstantin Kostiuk 

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Signed-off-by: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
Reviewed-by: Jiewen Yao 
---
 OvmfPkg/OvmfPkgIa32.dsc| 2 ++
 OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
 OvmfPkg/OvmfPkgX64.dsc | 2 ++
 OvmfPkg/OvmfPkgIa32.fdf| 1 +
 OvmfPkg/OvmfPkgIa32X64.fdf | 1 +
 OvmfPkg/OvmfPkgX64.fdf | 1 +
 6 files changed, 9 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 15fadc2fdc6e..9db3ebd0e722 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -188,6 +188,7 @@ [LibraryClasses]
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
+  HstiLib|MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
 !endif
@@ -829,6 +830,7 @@ [Components]
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
   #
   # ISA Support
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 6e55b50a9641..43378122925b 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -193,6 +193,7 @@ [LibraryClasses]
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
+  HstiLib|MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
 !endif
@@ -843,6 +844,7 @@ [Components.X64]
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
   #
   # ISA Support
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index f2edd3bbc05a..157ae6c0e4b0 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -205,6 +205,7 @@ [LibraryClasses]
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
+  HstiLib|MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
 
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
@@ -911,6 +912,7 @@ [Components]
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
   #
   # ISA Support
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 6c56c5e53f21..6eb26f7d4613 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -316,6 +316,7 @@ [FV.DXEFV]
 INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
 INF  OvmfPkg/PlatformDxe/Platform.inf
 INF  OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+INF  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
 !if $(SMM_REQUIRE) == TRUE
 INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index ee8068ad55dc..080784f722a7 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -323,6 +323,7 @@ [FV.DXEFV]
 INF  OvmfPkg/PlatformDxe/Platform.inf
 INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
 INF  OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+INF  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
 !if $(SMM_REQUIRE) == TRUE
 INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index fecb1fcfda4d..c2d3cc901e94 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -353,6 +353,7 @@ [FV.DXEFV]
 INF  OvmfPkg/PlatformDxe/Platform.inf
 INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
 INF  OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+INF  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
 !if $(SMM_REQUIRE) == TRUE
 INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118085): https://edk2.groups.io/g/devel/message/118085
Mute This Topic: https://groups.io/mt/105667074/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 5/5] OvmfPkg/VirtHstiDxe: add README.md

[Gerd Hoffmann]

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
Reviewed-by: Jiewen Yao 
---
 OvmfPkg/VirtHstiDxe/README.md | 48 +++
 1 file changed, 48 insertions(+)
 create mode 100644 OvmfPkg/VirtHstiDxe/README.md

diff --git a/OvmfPkg/VirtHstiDxe/README.md b/OvmfPkg/VirtHstiDxe/README.md
new file mode 100644
index ..c3975b854715
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/README.md
@@ -0,0 +1,48 @@
+
+# virtual machine platform hsti driver
+
+This driver supports three tests.
+
+## VIRT_HSTI_BYTE0_SMM_SMRAM_LOCK
+
+Verify the SMM memory is properly locked down.
+
+Supported platforms:
+ * Qemu Q35 (SMM_REQUIRE=TRUE builds).
+
+## VIRT_HSTI_BYTE0_SMM_SECURE_VARS_FLASH
+
+Verify the variable store is not writable for normal (not SMM) code.
+
+Supported platforms:
+ * Qemu Q35 (SMM_REQUIRE=TRUE builds).
+
+## VIRT_HSTI_BYTE0_READONLY_CODE_FLASH
+
+Verify the firmware code is not writable for the guest.
+
+Supported platforms:
+ * Qemu Q35
+ * Qemu PC
+
+# qemu flash configuration
+
+With qemu being configured properly flash behavior should be this:
+
+configuration  |  OVMF_CODE.fd  |  OVMF_VARS.fd
+---||---
+SMM_REQUIRE=TRUE, SMM mode |  read-only |  writable
+SMM_REQUIRE=TRUE, normal mode  |  read-only (1) |  read-only (2)
+SMM_REQUIRE=FALSE  |  read-only (3) |  writable
+
+VIRT_HSTI_BYTE0_READONLY_CODE_FLASH will verify (1) + (3).
+VIRT_HSTI_BYTE0_SMM_SECURE_VARS_FLASH will verify (2).
+
+## qemu command line for SMM_REQUIRE=TRUE builds
+```
+qemu-system-x86-64 -M q35,smm=on,pflash0=code,pflash1=vars \
+  -blockdev node-name=code,driver=file,filename=OVMF_CODE.fd,read-only=on \
+  -blockdev node-name=vars,driver=file,filename=OVMF_VARS.fd \
+  -global driver=cfi.pflash01,property=secure,value=on \
+  [ ... more options here ... ]
+```
-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118086): https://edk2.groups.io/g/devel/message/118086
Mute This Topic: https://groups.io/mt/105667075/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 4/5] OvmfPkg/VirtHstiDxe: add code flash check

[Gerd Hoffmann]

Detects qemu config issue: code pflash is writable.
Checked for both PC and Q35.

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
Reviewed-by: Jiewen Yao 
---
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  2 ++
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   | 13 +++
 OvmfPkg/VirtHstiDxe/QemuCommon.c| 36 +
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c   |  4 
 4 files changed, 55 insertions(+)
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuCommon.c

diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
index b6bdd1f22e83..9514933011e8 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
@@ -22,6 +22,7 @@ [Sources]
   VirtHstiDxe.c
   QemuPC.c
   QemuQ35.c
+  QemuCommon.c
   Flash.c
 
 [Packages]
@@ -48,6 +49,7 @@ [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
 
 [Pcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdBfvBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
 
 [Depex]
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
index ceff41c03711..f8bdcfe8f219 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
@@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #define VIRT_HSTI_BYTE0_SMM_SMRAM_LOCK BIT0
 #define VIRT_HSTI_BYTE0_SMM_SECURE_VARS_FLASH  BIT1
+#define VIRT_HSTI_BYTE0_READONLY_CODE_FLASHBIT2
 
 typedef struct {
   // ADAPTER_INFO_PLATFORM_SECURITY
@@ -67,6 +68,18 @@ VirtHstiQemuPCVerify (
   VOID
   );
 
+/* QemuCommon.c */
+
+VOID
+VirtHstiQemuCommonInit (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti
+  );
+
+VOID
+VirtHstiQemuCommonVerify (
+  VOID
+  );
+
 /* Flash.c */
 
 #define QEMU_FIRMWARE_FLASH_UNKNOWN0
diff --git a/OvmfPkg/VirtHstiDxe/QemuCommon.c b/OvmfPkg/VirtHstiDxe/QemuCommon.c
new file mode 100644
index ..4ab3fe2d6e63
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/QemuCommon.c
@@ -0,0 +1,36 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 
+#include 
+
+#include "VirtHstiDxe.h"
+
+VOID
+VirtHstiQemuCommonInit (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti
+  )
+{
+  VirtHstiSetSupported (VirtHsti, 0, VIRT_HSTI_BYTE0_READONLY_CODE_FLASH);
+}
+
+VOID
+VirtHstiQemuCommonVerify (
+  VOID
+  )
+{
+  CHAR16  *ErrorMsg;
+
+  switch (VirtHstiQemuFirmwareFlashCheck (PcdGet32 (PcdBfvBase))) {
+case QEMU_FIRMWARE_FLASH_WRITABLE:
+  ErrorMsg = L"qemu code pflash is writable";
+  break;
+default:
+  ErrorMsg = NULL;
+  }
+
+  VirtHstiTestResult (ErrorMsg, 0, VIRT_HSTI_BYTE0_READONLY_CODE_FLASH);
+}
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
index 74e5e6bd9d4f..b6e53a1219d1 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
@@ -104,9 +104,11 @@ VirtHstiOnReadyToBoot (
   switch (VirtHstiGetHostBridgeDevId ()) {
 case INTEL_82441_DEVICE_ID:
   VirtHstiQemuPCVerify ();
+  VirtHstiQemuCommonVerify ();
   break;
 case INTEL_Q35_MCH_DEVICE_ID:
   VirtHstiQemuQ35Verify ();
+  VirtHstiQemuCommonVerify ();
   break;
 default:
   ASSERT (FALSE);
@@ -142,9 +144,11 @@ VirtHstiDxeEntrypoint (
   switch (DevId) {
 case INTEL_82441_DEVICE_ID:
   VirtHsti = VirtHstiQemuPCInit ();
+  VirtHstiQemuCommonInit (VirtHsti);
   break;
 case INTEL_Q35_MCH_DEVICE_ID:
   VirtHsti = VirtHstiQemuQ35Init ();
+  VirtHstiQemuCommonInit (VirtHsti);
   break;
 default:
   DEBUG ((DEBUG_INFO, "%a: unknown platform (0x%x)\n", __func__, DevId));
-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118083): https://edk2.groups.io/g/devel/message/118083
Mute This Topic: https://groups.io/mt/105667072/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 3/5] OvmfPkg/VirtHstiDxe: add varstore flash check

[Gerd Hoffmann]

Detects qemu config issue: vars pflash is not in secure mode (write
access restricted to smm).  Applies to Q35 with SMM only.

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
Reviewed-by: Jiewen Yao 
---
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  4 ++
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   | 16 -
 OvmfPkg/VirtHstiDxe/Flash.c | 90 +
 OvmfPkg/VirtHstiDxe/QemuQ35.c   | 13 +
 4 files changed, 122 insertions(+), 1 deletion(-)
 create mode 100644 OvmfPkg/VirtHstiDxe/Flash.c

diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
index 8c63ff6a8953..b6bdd1f22e83 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
@@ -22,6 +22,7 @@ [Sources]
   VirtHstiDxe.c
   QemuPC.c
   QemuQ35.c
+  Flash.c
 
 [Packages]
   MdePkg/MdePkg.dec
@@ -46,5 +47,8 @@ [Guids]
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
 
+[Pcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
+
 [Depex]
   TRUE
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
index cf0d77fc3af9..ceff41c03711 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
@@ -6,7 +6,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #define VIRT_HSTI_SECURITY_FEATURE_SIZE  2
 
-#define VIRT_HSTI_BYTE0_SMM_SMRAM_LOCK  BIT0
+#define VIRT_HSTI_BYTE0_SMM_SMRAM_LOCK BIT0
+#define VIRT_HSTI_BYTE0_SMM_SECURE_VARS_FLASH  BIT1
 
 typedef struct {
   // ADAPTER_INFO_PLATFORM_SECURITY
@@ -65,3 +66,16 @@ VOID
 VirtHstiQemuPCVerify (
   VOID
   );
+
+/* Flash.c */
+
+#define QEMU_FIRMWARE_FLASH_UNKNOWN0
+#define QEMU_FIRMWARE_FLASH_IS_ROM 1
+#define QEMU_FIRMWARE_FLASH_IS_RAM 2
+#define QEMU_FIRMWARE_FLASH_READ_ONLY  3
+#define QEMU_FIRMWARE_FLASH_WRITABLE   4
+
+UINT32
+VirtHstiQemuFirmwareFlashCheck (
+  UINT32  Address
+  );
diff --git a/OvmfPkg/VirtHstiDxe/Flash.c b/OvmfPkg/VirtHstiDxe/Flash.c
new file mode 100644
index ..e93356793f8c
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/Flash.c
@@ -0,0 +1,90 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 
+#include 
+
+#include "VirtHstiDxe.h"
+
+#define WRITE_BYTE_CMD   0x10
+#define BLOCK_ERASE_CMD  0x20
+#define CLEAR_STATUS_CMD 0x50
+#define READ_STATUS_CMD  0x70
+#define READ_DEVID_CMD   0x90
+#define BLOCK_ERASE_CONFIRM_CMD  0xd0
+#define READ_ARRAY_CMD   0xff
+#define CLEARED_ARRAY_STATUS 0x00
+
+/* based on QemuFlashDetected (QemuFlashFvbServicesRuntimeDxe) */
+UINT32
+VirtHstiQemuFirmwareFlashCheck (
+  UINT32  Address
+  )
+{
+  volatile UINT8  *Ptr;
+
+  UINTN  Offset;
+  UINT8  OriginalUint8;
+  UINT8  ProbeUint8;
+
+  for (Offset = 0; Offset < EFI_PAGE_SIZE; Offset++) {
+Ptr= (UINT8 *)(UINTN)(Address + Offset);
+ProbeUint8 = *Ptr;
+if ((ProbeUint8 != CLEAR_STATUS_CMD) &&
+(ProbeUint8 != READ_STATUS_CMD) &&
+(ProbeUint8 != CLEARED_ARRAY_STATUS))
+{
+  break;
+}
+  }
+
+  if (Offset >= EFI_PAGE_SIZE) {
+DEBUG ((DEBUG_INFO, "%a: check failed\n", __func__));
+return QEMU_FIRMWARE_FLASH_UNKNOWN;
+  }
+
+  OriginalUint8 = *Ptr;
+  *Ptr  = CLEAR_STATUS_CMD;
+  ProbeUint8= *Ptr;
+  if ((OriginalUint8 != CLEAR_STATUS_CMD) &&
+  (ProbeUint8 == CLEAR_STATUS_CMD))
+  {
+*Ptr = OriginalUint8;
+DEBUG ((DEBUG_INFO, "%a: %p behaves as RAM\n", __func__, Ptr));
+return QEMU_FIRMWARE_FLASH_IS_RAM;
+  }
+
+  *Ptr   = READ_STATUS_CMD;
+  ProbeUint8 = *Ptr;
+  if (ProbeUint8 == OriginalUint8) {
+DEBUG ((DEBUG_INFO, "%a: %p behaves as ROM\n", __func__, Ptr));
+return QEMU_FIRMWARE_FLASH_IS_ROM;
+  }
+
+  if (ProbeUint8 == READ_STATUS_CMD) {
+*Ptr = OriginalUint8;
+DEBUG ((DEBUG_INFO, "%a: %p behaves as RAM\n", __func__, Ptr));
+return QEMU_FIRMWARE_FLASH_IS_RAM;
+  }
+
+  if (ProbeUint8 == CLEARED_ARRAY_STATUS) {
+*Ptr   = WRITE_BYTE_CMD;
+*Ptr   = OriginalUint8;
+*Ptr   = READ_STATUS_CMD;
+ProbeUint8 = *Ptr;
+*Ptr   = READ_ARRAY_CMD;
+if (ProbeUint8 & 0x10 /* programming error */) {
+  DEBUG ((DEBUG_INFO, "%a: %p behaves as FLASH, write-protected\n", 
__func__, Ptr));
+  return QEMU_FIRMWARE_FLASH_READ_ONLY;
+} else {
+  DEBUG ((DEBUG_INFO, "%a: %p behaves as FLASH, writable\n", __func__, 
Ptr));
+  return QEMU_FIRMWARE_FLASH_WRITABLE;
+}
+  }
+
+  DEBUG ((DEBUG_INFO, "%a: check failed\n", __func__));
+  return QEMU_FIRMWARE_FLASH_UNKNOWN;
+}
diff --git a/OvmfPkg/VirtHstiDxe/QemuQ35.c b/OvmfPkg/VirtHstiDxe/QemuQ35.c
index 5eab4aab29d1..2dcfa5239cbf 100644
--- a/OvmfPkg/VirtHstiDxe/QemuQ35.c
+++ b/OvmfPkg/VirtHstiDxe/QemuQ35.c
@@ -29,6 +29,7 @@ VirtHstiQemuQ35Init (
 {
   if (FeaturePcdGet (PcdS

[edk2-devel] [PATCH v3 1/5] OvmfPkg: Add VirtHstiDxe driver

[Gerd Hoffmann]

From: Konstantin Kostiuk 

The driver supports qemu machine types 'pc' and 'q35'.

This patch adds some helper functions to manage the bitmasks.
The implemented features depend on both OVMF build configuration
and qemu VM configuration.

For q35 a single security feature is supported and checked: In
SMM-enabled builds the driver will verify smram is properly locked.
That test should never fail.

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Konstantin Kostiuk 
Initial-patch-by: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
Reviewed-by: Jiewen Yao 
---
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  50 
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   |  67 +++
 OvmfPkg/VirtHstiDxe/QemuPC.c|  38 +++
 OvmfPkg/VirtHstiDxe/QemuQ35.c   |  58 ++
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c   | 169 
 5 files changed, 382 insertions(+)
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuPC.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuQ35.c
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c

diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
new file mode 100644
index ..8c63ff6a8953
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
@@ -0,0 +1,50 @@
+## @file
+#  Component description file for Virt Hsti Driver
+#
+# Copyright (c) 2017, Intel Corporation. All rights reserved.
+# Copyright (c) Microsoft Corporation.
+# Copyright (c) 2024, Red Hat. Inc
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION= 0x00010005
+  BASE_NAME  = VirtHstiDxe
+  FILE_GUID  = 60740CF3-D428-4500-80E6-04A5798241ED
+  MODULE_TYPE= DXE_DRIVER
+  VERSION_STRING = 1.0
+  ENTRY_POINT= VirtHstiDxeEntrypoint
+
+[Sources]
+  VirtHstiDxe.h
+  VirtHstiDxe.c
+  QemuPC.c
+  QemuQ35.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  UefiDriverEntryPoint
+  UefiLib
+  BaseLib
+  BaseMemoryLib
+  MemoryAllocationLib
+  DebugLib
+  HobLib
+  HstiLib
+  PcdLib
+  PciLib
+  UefiBootServicesTableLib
+
+[Guids]
+  gUefiOvmfPkgPlatformInfoGuid
+
+[FeaturePcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
+
+[Depex]
+  TRUE
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
new file mode 100644
index ..cf0d77fc3af9
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
@@ -0,0 +1,67 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#define VIRT_HSTI_SECURITY_FEATURE_SIZE  2
+
+#define VIRT_HSTI_BYTE0_SMM_SMRAM_LOCK  BIT0
+
+typedef struct {
+  // ADAPTER_INFO_PLATFORM_SECURITY
+  UINT32Version;
+  UINT32Role;
+  CHAR16ImplementationID[256];
+  UINT32SecurityFeaturesSize;
+  // bitfields
+  UINT8 SecurityFeaturesRequired[VIRT_HSTI_SECURITY_FEATURE_SIZE];
+  UINT8 SecurityFeaturesImplemented[VIRT_HSTI_SECURITY_FEATURE_SIZE];
+  UINT8 SecurityFeaturesVerified[VIRT_HSTI_SECURITY_FEATURE_SIZE];
+  CHAR16ErrorString[1];
+} VIRT_ADAPTER_INFO_PLATFORM_SECURITY;
+
+VOID
+VirtHstiSetSupported (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti,
+  IN UINT32ByteIndex,
+  IN UINT8 BitMask
+  );
+
+BOOLEAN
+VirtHstiIsSupported (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti,
+  IN UINT32ByteIndex,
+  IN UINT8 BitMask
+  );
+
+VOID
+VirtHstiTestResult (
+  CHAR16 *ErrorMsg,
+  IN UINT32  ByteIndex,
+  IN UINT8   BitMask
+  );
+
+/* QemuQ35.c */
+
+VIRT_ADAPTER_INFO_PLATFORM_SECURITY *
+VirtHstiQemuQ35Init (
+  VOID
+  );
+
+VOID
+VirtHstiQemuQ35Verify (
+  VOID
+  );
+
+/* QemuPC.c */
+
+VIRT_ADAPTER_INFO_PLATFORM_SECURITY *
+VirtHstiQemuPCInit (
+  VOID
+  );
+
+VOID
+VirtHstiQemuPCVerify (
+  VOID
+  );
diff --git a/OvmfPkg/VirtHstiDxe/QemuPC.c b/OvmfPkg/VirtHstiDxe/QemuPC.c
new file mode 100644
index ..aa0459e8b6c6
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/QemuPC.c
@@ -0,0 +1,38 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#include "VirtHstiDxe.h"
+
+STATIC VIRT_ADAPTER_INFO_PLATFORM_SECURITY  mHstiPC = {
+  PLATFORM_SECURITY_VERSION_VNEXTCS,
+  PLATFORM_SECURITY_ROLE_PLATFORM_REFERENCE,
+  { L"OVMF (Qemu PC)" },
+  VIRT_HSTI_SECURITY_FEATURE_SIZE,
+};
+
+VIRT_ADAPTER_INFO_PLATFORM_SECURITY *
+VirtHstiQemuPCInit (
+  VOID
+  )
+{
+  return 
+}
+
+VOID
+VirtHstiQemuPCVerify (
+  VOID
+  )
+{
+}
diff --git a/OvmfPkg/VirtHstiDxe/QemuQ35.c b/OvmfPkg/VirtHstiDxe/QemuQ35.c
new file mode 100644
index ..5eab4aab29d1
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/QemuQ35.c
@@ -0,0 +1,58 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Paten

[edk2-devel] [PATCH v3 0/5] OvmfPkg: Add VirtHstiDxe driver

[Gerd Hoffmann]

v3:
 - use PcdOvmfFlashNvStorageVariableBase
 - add reviewed-by tags
v2:
 - remove 'Q35' from test bits
 - add patch with a README.md

Gerd Hoffmann (3):
  OvmfPkg/VirtHstiDxe: add varstore flash check
  OvmfPkg/VirtHstiDxe: add code flash check
  OvmfPkg/VirtHstiDxe: add README.md

Konstantin Kostiuk (2):
  OvmfPkg: Add VirtHstiDxe driver
  OvmfPkg: Add VirtHstiDxe to OVMF firmware build

 OvmfPkg/OvmfPkgIa32.dsc |   2 +
 OvmfPkg/OvmfPkgIa32X64.dsc  |   2 +
 OvmfPkg/OvmfPkgX64.dsc  |   2 +
 OvmfPkg/OvmfPkgIa32.fdf |   1 +
 OvmfPkg/OvmfPkgIa32X64.fdf  |   1 +
 OvmfPkg/OvmfPkgX64.fdf  |   1 +
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  56 +
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   |  94 +++
 OvmfPkg/VirtHstiDxe/Flash.c |  90 +++
 OvmfPkg/VirtHstiDxe/QemuCommon.c|  36 ++
 OvmfPkg/VirtHstiDxe/QemuPC.c|  38 ++
 OvmfPkg/VirtHstiDxe/QemuQ35.c   |  71 
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c   | 173 
 OvmfPkg/VirtHstiDxe/README.md   |  48 
 14 files changed, 615 insertions(+)
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
 create mode 100644 OvmfPkg/VirtHstiDxe/Flash.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuCommon.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuPC.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuQ35.c
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
 create mode 100644 OvmfPkg/VirtHstiDxe/README.md

-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118081): https://edk2.groups.io/g/devel/message/118081
Mute This Topic: https://groups.io/mt/105667070/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 00/13] Add SmmRelocationLib

[Gerd Hoffmann]

On Thu, Apr 18, 2024 at 08:02:43AM +, Wu, Jiaxin wrote:
> Hi Gerd,
> 
> Could you help review & check below OVMF related patches?
> 
> >   OvmfPkg/SmmRelocationLib: Add library instance for OVMF
> >   OvmfPkg/PlatformInitLib: Create gEfiSmmSmramMemoryGuid
> >   OvmfPkg: Refine SmmAccess implementation
> >   OvmfPkg/SmmCpuFeaturesLib: Check Smbase Relocation is done or not
> >   OvmfPkg/PlatformPei: Relocate SmBases in PEI phase

Patch series breaks S3 suspend support in OVMF.

On a quick check (OvmfPkgX64 only) using AmdSmmRelocationLib.inf for
OVMF seems to work fine (S3 is broken too though).

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118074): https://edk2.groups.io/g/devel/message/118074
Mute This Topic: https://groups.io/mt/105593568/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 5/5] OvmfPkg/VirtHstiDxe: add README.md

[Gerd Hoffmann]

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
---
 OvmfPkg/VirtHstiDxe/README.md | 48 +++
 1 file changed, 48 insertions(+)
 create mode 100644 OvmfPkg/VirtHstiDxe/README.md

diff --git a/OvmfPkg/VirtHstiDxe/README.md b/OvmfPkg/VirtHstiDxe/README.md
new file mode 100644
index ..c3975b854715
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/README.md
@@ -0,0 +1,48 @@
+
+# virtual machine platform hsti driver
+
+This driver supports three tests.
+
+## VIRT_HSTI_BYTE0_SMM_SMRAM_LOCK
+
+Verify the SMM memory is properly locked down.
+
+Supported platforms:
+ * Qemu Q35 (SMM_REQUIRE=TRUE builds).
+
+## VIRT_HSTI_BYTE0_SMM_SECURE_VARS_FLASH
+
+Verify the variable store is not writable for normal (not SMM) code.
+
+Supported platforms:
+ * Qemu Q35 (SMM_REQUIRE=TRUE builds).
+
+## VIRT_HSTI_BYTE0_READONLY_CODE_FLASH
+
+Verify the firmware code is not writable for the guest.
+
+Supported platforms:
+ * Qemu Q35
+ * Qemu PC
+
+# qemu flash configuration
+
+With qemu being configured properly flash behavior should be this:
+
+configuration  |  OVMF_CODE.fd  |  OVMF_VARS.fd
+---||---
+SMM_REQUIRE=TRUE, SMM mode |  read-only |  writable
+SMM_REQUIRE=TRUE, normal mode  |  read-only (1) |  read-only (2)
+SMM_REQUIRE=FALSE  |  read-only (3) |  writable
+
+VIRT_HSTI_BYTE0_READONLY_CODE_FLASH will verify (1) + (3).
+VIRT_HSTI_BYTE0_SMM_SECURE_VARS_FLASH will verify (2).
+
+## qemu command line for SMM_REQUIRE=TRUE builds
+```
+qemu-system-x86-64 -M q35,smm=on,pflash0=code,pflash1=vars \
+  -blockdev node-name=code,driver=file,filename=OVMF_CODE.fd,read-only=on \
+  -blockdev node-name=vars,driver=file,filename=OVMF_VARS.fd \
+  -global driver=cfi.pflash01,property=secure,value=on \
+  [ ... more options here ... ]
+```
-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118027): https://edk2.groups.io/g/devel/message/118027
Mute This Topic: https://groups.io/mt/105616663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 4/5] OvmfPkg/VirtHstiDxe: add code flash check

[Gerd Hoffmann]

Detects qemu config issue: code pflash is writable.
Checked for both PC and Q35.

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
---
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  2 ++
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   | 13 +++
 OvmfPkg/VirtHstiDxe/QemuCommon.c| 36 +
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c   |  4 
 4 files changed, 55 insertions(+)
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuCommon.c

diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
index 9cb2ed1f0c64..376cd28aeb7e 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
@@ -22,6 +22,7 @@ [Sources]
   VirtHstiDxe.c
   QemuPC.c
   QemuQ35.c
+  QemuCommon.c
   Flash.c
 
 [Packages]
@@ -49,6 +50,7 @@ [FeaturePcd]
 
 [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+  gUefiOvmfPkgTokenSpaceGuid.PcdBfvBase
 
 [Depex]
   TRUE
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
index ceff41c03711..f8bdcfe8f219 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
@@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #define VIRT_HSTI_BYTE0_SMM_SMRAM_LOCK BIT0
 #define VIRT_HSTI_BYTE0_SMM_SECURE_VARS_FLASH  BIT1
+#define VIRT_HSTI_BYTE0_READONLY_CODE_FLASHBIT2
 
 typedef struct {
   // ADAPTER_INFO_PLATFORM_SECURITY
@@ -67,6 +68,18 @@ VirtHstiQemuPCVerify (
   VOID
   );
 
+/* QemuCommon.c */
+
+VOID
+VirtHstiQemuCommonInit (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti
+  );
+
+VOID
+VirtHstiQemuCommonVerify (
+  VOID
+  );
+
 /* Flash.c */
 
 #define QEMU_FIRMWARE_FLASH_UNKNOWN0
diff --git a/OvmfPkg/VirtHstiDxe/QemuCommon.c b/OvmfPkg/VirtHstiDxe/QemuCommon.c
new file mode 100644
index ..4ab3fe2d6e63
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/QemuCommon.c
@@ -0,0 +1,36 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 
+#include 
+
+#include "VirtHstiDxe.h"
+
+VOID
+VirtHstiQemuCommonInit (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti
+  )
+{
+  VirtHstiSetSupported (VirtHsti, 0, VIRT_HSTI_BYTE0_READONLY_CODE_FLASH);
+}
+
+VOID
+VirtHstiQemuCommonVerify (
+  VOID
+  )
+{
+  CHAR16  *ErrorMsg;
+
+  switch (VirtHstiQemuFirmwareFlashCheck (PcdGet32 (PcdBfvBase))) {
+case QEMU_FIRMWARE_FLASH_WRITABLE:
+  ErrorMsg = L"qemu code pflash is writable";
+  break;
+default:
+  ErrorMsg = NULL;
+  }
+
+  VirtHstiTestResult (ErrorMsg, 0, VIRT_HSTI_BYTE0_READONLY_CODE_FLASH);
+}
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
index 74e5e6bd9d4f..b6e53a1219d1 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
@@ -104,9 +104,11 @@ VirtHstiOnReadyToBoot (
   switch (VirtHstiGetHostBridgeDevId ()) {
 case INTEL_82441_DEVICE_ID:
   VirtHstiQemuPCVerify ();
+  VirtHstiQemuCommonVerify ();
   break;
 case INTEL_Q35_MCH_DEVICE_ID:
   VirtHstiQemuQ35Verify ();
+  VirtHstiQemuCommonVerify ();
   break;
 default:
   ASSERT (FALSE);
@@ -142,9 +144,11 @@ VirtHstiDxeEntrypoint (
   switch (DevId) {
 case INTEL_82441_DEVICE_ID:
   VirtHsti = VirtHstiQemuPCInit ();
+  VirtHstiQemuCommonInit (VirtHsti);
   break;
 case INTEL_Q35_MCH_DEVICE_ID:
   VirtHsti = VirtHstiQemuQ35Init ();
+  VirtHstiQemuCommonInit (VirtHsti);
   break;
 default:
   DEBUG ((DEBUG_INFO, "%a: unknown platform (0x%x)\n", __func__, DevId));
-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118026): https://edk2.groups.io/g/devel/message/118026
Mute This Topic: https://groups.io/mt/105616662/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/5] OvmfPkg: Add VirtHstiDxe driver

[Gerd Hoffmann]

From: Konstantin Kostiuk 

The driver supports qemu machine types 'pc' and 'q35'.

This patch adds some helper functions to manage the bitmasks.
The implemented features depend on both OVMF build configuration
and qemu VM configuration.

For q35 a single security feature is supported and checked: In
SMM-enabled builds the driver will verify smram is properly locked.
That test should never fail.

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Konstantin Kostiuk 
Initial-patch-by: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
---
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  50 
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   |  67 +++
 OvmfPkg/VirtHstiDxe/QemuPC.c|  38 +++
 OvmfPkg/VirtHstiDxe/QemuQ35.c   |  58 ++
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c   | 169 
 5 files changed, 382 insertions(+)
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuPC.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuQ35.c
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c

diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
new file mode 100644
index ..8c63ff6a8953
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
@@ -0,0 +1,50 @@
+## @file
+#  Component description file for Virt Hsti Driver
+#
+# Copyright (c) 2017, Intel Corporation. All rights reserved.
+# Copyright (c) Microsoft Corporation.
+# Copyright (c) 2024, Red Hat. Inc
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION= 0x00010005
+  BASE_NAME  = VirtHstiDxe
+  FILE_GUID  = 60740CF3-D428-4500-80E6-04A5798241ED
+  MODULE_TYPE= DXE_DRIVER
+  VERSION_STRING = 1.0
+  ENTRY_POINT= VirtHstiDxeEntrypoint
+
+[Sources]
+  VirtHstiDxe.h
+  VirtHstiDxe.c
+  QemuPC.c
+  QemuQ35.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  UefiDriverEntryPoint
+  UefiLib
+  BaseLib
+  BaseMemoryLib
+  MemoryAllocationLib
+  DebugLib
+  HobLib
+  HstiLib
+  PcdLib
+  PciLib
+  UefiBootServicesTableLib
+
+[Guids]
+  gUefiOvmfPkgPlatformInfoGuid
+
+[FeaturePcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
+
+[Depex]
+  TRUE
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
new file mode 100644
index ..cf0d77fc3af9
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
@@ -0,0 +1,67 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#define VIRT_HSTI_SECURITY_FEATURE_SIZE  2
+
+#define VIRT_HSTI_BYTE0_SMM_SMRAM_LOCK  BIT0
+
+typedef struct {
+  // ADAPTER_INFO_PLATFORM_SECURITY
+  UINT32Version;
+  UINT32Role;
+  CHAR16ImplementationID[256];
+  UINT32SecurityFeaturesSize;
+  // bitfields
+  UINT8 SecurityFeaturesRequired[VIRT_HSTI_SECURITY_FEATURE_SIZE];
+  UINT8 SecurityFeaturesImplemented[VIRT_HSTI_SECURITY_FEATURE_SIZE];
+  UINT8 SecurityFeaturesVerified[VIRT_HSTI_SECURITY_FEATURE_SIZE];
+  CHAR16ErrorString[1];
+} VIRT_ADAPTER_INFO_PLATFORM_SECURITY;
+
+VOID
+VirtHstiSetSupported (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti,
+  IN UINT32ByteIndex,
+  IN UINT8 BitMask
+  );
+
+BOOLEAN
+VirtHstiIsSupported (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti,
+  IN UINT32ByteIndex,
+  IN UINT8 BitMask
+  );
+
+VOID
+VirtHstiTestResult (
+  CHAR16 *ErrorMsg,
+  IN UINT32  ByteIndex,
+  IN UINT8   BitMask
+  );
+
+/* QemuQ35.c */
+
+VIRT_ADAPTER_INFO_PLATFORM_SECURITY *
+VirtHstiQemuQ35Init (
+  VOID
+  );
+
+VOID
+VirtHstiQemuQ35Verify (
+  VOID
+  );
+
+/* QemuPC.c */
+
+VIRT_ADAPTER_INFO_PLATFORM_SECURITY *
+VirtHstiQemuPCInit (
+  VOID
+  );
+
+VOID
+VirtHstiQemuPCVerify (
+  VOID
+  );
diff --git a/OvmfPkg/VirtHstiDxe/QemuPC.c b/OvmfPkg/VirtHstiDxe/QemuPC.c
new file mode 100644
index ..aa0459e8b6c6
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/QemuPC.c
@@ -0,0 +1,38 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#include "VirtHstiDxe.h"
+
+STATIC VIRT_ADAPTER_INFO_PLATFORM_SECURITY  mHstiPC = {
+  PLATFORM_SECURITY_VERSION_VNEXTCS,
+  PLATFORM_SECURITY_ROLE_PLATFORM_REFERENCE,
+  { L"OVMF (Qemu PC)" },
+  VIRT_HSTI_SECURITY_FEATURE_SIZE,
+};
+
+VIRT_ADAPTER_INFO_PLATFORM_SECURITY *
+VirtHstiQemuPCInit (
+  VOID
+  )
+{
+  return 
+}
+
+VOID
+VirtHstiQemuPCVerify (
+  VOID
+  )
+{
+}
diff --git a/OvmfPkg/VirtHstiDxe/QemuQ35.c b/OvmfPkg/VirtHstiDxe/QemuQ35.c
new file mode 100644
index ..5eab4aab29d1
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/QemuQ35.c
@@ -0,0 +1,58 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 

[edk2-devel] [PATCH v2 3/5] OvmfPkg/VirtHstiDxe: add varstore flash check

[Gerd Hoffmann]

Detects qemu config issue: vars pflash is not in secure mode (write
access restricted to smm).  Applies to Q35 with SMM only.

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
---
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  4 ++
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   | 16 -
 OvmfPkg/VirtHstiDxe/Flash.c | 90 +
 OvmfPkg/VirtHstiDxe/QemuQ35.c   | 13 +
 4 files changed, 122 insertions(+), 1 deletion(-)
 create mode 100644 OvmfPkg/VirtHstiDxe/Flash.c

diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
index 8c63ff6a8953..9cb2ed1f0c64 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
@@ -22,6 +22,7 @@ [Sources]
   VirtHstiDxe.c
   QemuPC.c
   QemuQ35.c
+  Flash.c
 
 [Packages]
   MdePkg/MdePkg.dec
@@ -46,5 +47,8 @@ [Guids]
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
 
+[Pcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+
 [Depex]
   TRUE
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
index cf0d77fc3af9..ceff41c03711 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
@@ -6,7 +6,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #define VIRT_HSTI_SECURITY_FEATURE_SIZE  2
 
-#define VIRT_HSTI_BYTE0_SMM_SMRAM_LOCK  BIT0
+#define VIRT_HSTI_BYTE0_SMM_SMRAM_LOCK BIT0
+#define VIRT_HSTI_BYTE0_SMM_SECURE_VARS_FLASH  BIT1
 
 typedef struct {
   // ADAPTER_INFO_PLATFORM_SECURITY
@@ -65,3 +66,16 @@ VOID
 VirtHstiQemuPCVerify (
   VOID
   );
+
+/* Flash.c */
+
+#define QEMU_FIRMWARE_FLASH_UNKNOWN0
+#define QEMU_FIRMWARE_FLASH_IS_ROM 1
+#define QEMU_FIRMWARE_FLASH_IS_RAM 2
+#define QEMU_FIRMWARE_FLASH_READ_ONLY  3
+#define QEMU_FIRMWARE_FLASH_WRITABLE   4
+
+UINT32
+VirtHstiQemuFirmwareFlashCheck (
+  UINT32  Address
+  );
diff --git a/OvmfPkg/VirtHstiDxe/Flash.c b/OvmfPkg/VirtHstiDxe/Flash.c
new file mode 100644
index ..e93356793f8c
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/Flash.c
@@ -0,0 +1,90 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 
+#include 
+
+#include "VirtHstiDxe.h"
+
+#define WRITE_BYTE_CMD   0x10
+#define BLOCK_ERASE_CMD  0x20
+#define CLEAR_STATUS_CMD 0x50
+#define READ_STATUS_CMD  0x70
+#define READ_DEVID_CMD   0x90
+#define BLOCK_ERASE_CONFIRM_CMD  0xd0
+#define READ_ARRAY_CMD   0xff
+#define CLEARED_ARRAY_STATUS 0x00
+
+/* based on QemuFlashDetected (QemuFlashFvbServicesRuntimeDxe) */
+UINT32
+VirtHstiQemuFirmwareFlashCheck (
+  UINT32  Address
+  )
+{
+  volatile UINT8  *Ptr;
+
+  UINTN  Offset;
+  UINT8  OriginalUint8;
+  UINT8  ProbeUint8;
+
+  for (Offset = 0; Offset < EFI_PAGE_SIZE; Offset++) {
+Ptr= (UINT8 *)(UINTN)(Address + Offset);
+ProbeUint8 = *Ptr;
+if ((ProbeUint8 != CLEAR_STATUS_CMD) &&
+(ProbeUint8 != READ_STATUS_CMD) &&
+(ProbeUint8 != CLEARED_ARRAY_STATUS))
+{
+  break;
+}
+  }
+
+  if (Offset >= EFI_PAGE_SIZE) {
+DEBUG ((DEBUG_INFO, "%a: check failed\n", __func__));
+return QEMU_FIRMWARE_FLASH_UNKNOWN;
+  }
+
+  OriginalUint8 = *Ptr;
+  *Ptr  = CLEAR_STATUS_CMD;
+  ProbeUint8= *Ptr;
+  if ((OriginalUint8 != CLEAR_STATUS_CMD) &&
+  (ProbeUint8 == CLEAR_STATUS_CMD))
+  {
+*Ptr = OriginalUint8;
+DEBUG ((DEBUG_INFO, "%a: %p behaves as RAM\n", __func__, Ptr));
+return QEMU_FIRMWARE_FLASH_IS_RAM;
+  }
+
+  *Ptr   = READ_STATUS_CMD;
+  ProbeUint8 = *Ptr;
+  if (ProbeUint8 == OriginalUint8) {
+DEBUG ((DEBUG_INFO, "%a: %p behaves as ROM\n", __func__, Ptr));
+return QEMU_FIRMWARE_FLASH_IS_ROM;
+  }
+
+  if (ProbeUint8 == READ_STATUS_CMD) {
+*Ptr = OriginalUint8;
+DEBUG ((DEBUG_INFO, "%a: %p behaves as RAM\n", __func__, Ptr));
+return QEMU_FIRMWARE_FLASH_IS_RAM;
+  }
+
+  if (ProbeUint8 == CLEARED_ARRAY_STATUS) {
+*Ptr   = WRITE_BYTE_CMD;
+*Ptr   = OriginalUint8;
+*Ptr   = READ_STATUS_CMD;
+ProbeUint8 = *Ptr;
+*Ptr   = READ_ARRAY_CMD;
+if (ProbeUint8 & 0x10 /* programming error */) {
+  DEBUG ((DEBUG_INFO, "%a: %p behaves as FLASH, write-protected\n", 
__func__, Ptr));
+  return QEMU_FIRMWARE_FLASH_READ_ONLY;
+} else {
+  DEBUG ((DEBUG_INFO, "%a: %p behaves as FLASH, writable\n", __func__, 
Ptr));
+  return QEMU_FIRMWARE_FLASH_WRITABLE;
+}
+  }
+
+  DEBUG ((DEBUG_INFO, "%a: check failed\n", __func__));
+  return QEMU_FIRMWARE_FLASH_UNKNOWN;
+}
diff --git a/OvmfPkg/VirtHstiDxe/QemuQ35.c b/OvmfPkg/VirtHstiDxe/QemuQ35.c
index 5eab4aab29d1..33753027060b 100644
--- a/OvmfPkg/VirtHstiDxe/QemuQ35.c
+++ b/OvmfPkg/VirtHstiDxe/QemuQ35.c
@@ -29,6 +29,7 @@ VirtHstiQemuQ35Init (
 {
   if (FeaturePcdGet (PcdSmmSmramRequire)) {
 VirtHstiSetSupp

[edk2-devel] [PATCH v2 2/5] OvmfPkg: Add VirtHstiDxe to OVMF firmware build

[Gerd Hoffmann]

From: Konstantin Kostiuk 

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Signed-off-by: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
---
 OvmfPkg/OvmfPkgIa32.dsc| 2 ++
 OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
 OvmfPkg/OvmfPkgX64.dsc | 2 ++
 OvmfPkg/OvmfPkgIa32.fdf| 1 +
 OvmfPkg/OvmfPkgIa32X64.fdf | 1 +
 OvmfPkg/OvmfPkgX64.fdf | 1 +
 6 files changed, 9 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 15fadc2fdc6e..9db3ebd0e722 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -188,6 +188,7 @@ [LibraryClasses]
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
+  HstiLib|MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
 !endif
@@ -829,6 +830,7 @@ [Components]
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
   #
   # ISA Support
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 6e55b50a9641..43378122925b 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -193,6 +193,7 @@ [LibraryClasses]
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
+  HstiLib|MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
 !endif
@@ -843,6 +844,7 @@ [Components.X64]
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
   #
   # ISA Support
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index f2edd3bbc05a..157ae6c0e4b0 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -205,6 +205,7 @@ [LibraryClasses]
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
+  HstiLib|MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
 
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
@@ -911,6 +912,7 @@ [Components]
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
   #
   # ISA Support
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 6c56c5e53f21..6eb26f7d4613 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -316,6 +316,7 @@ [FV.DXEFV]
 INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
 INF  OvmfPkg/PlatformDxe/Platform.inf
 INF  OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+INF  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
 !if $(SMM_REQUIRE) == TRUE
 INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index ee8068ad55dc..080784f722a7 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -323,6 +323,7 @@ [FV.DXEFV]
 INF  OvmfPkg/PlatformDxe/Platform.inf
 INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
 INF  OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+INF  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
 !if $(SMM_REQUIRE) == TRUE
 INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index fecb1fcfda4d..c2d3cc901e94 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -353,6 +353,7 @@ [FV.DXEFV]
 INF  OvmfPkg/PlatformDxe/Platform.inf
 INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
 INF  OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+INF  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
 !if $(SMM_REQUIRE) == TRUE
 INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118024): https://edk2.groups.io/g/devel/message/118024
Mute This Topic: https://groups.io/mt/105616659/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 0/5] OvmfPkg: Add VirtHstiDxe driver

[Gerd Hoffmann]

v2:
 - remove 'Q35' from test bits
 - add patch with a README.md

Gerd Hoffmann (3):
  OvmfPkg/VirtHstiDxe: add varstore flash check
  OvmfPkg/VirtHstiDxe: add code flash check
  OvmfPkg/VirtHstiDxe: add README.md

Konstantin Kostiuk (2):
  OvmfPkg: Add VirtHstiDxe driver
  OvmfPkg: Add VirtHstiDxe to OVMF firmware build

 OvmfPkg/OvmfPkgIa32.dsc |   2 +
 OvmfPkg/OvmfPkgIa32X64.dsc  |   2 +
 OvmfPkg/OvmfPkgX64.dsc  |   2 +
 OvmfPkg/OvmfPkgIa32.fdf |   1 +
 OvmfPkg/OvmfPkgIa32X64.fdf  |   1 +
 OvmfPkg/OvmfPkgX64.fdf  |   1 +
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  56 +
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   |  94 +++
 OvmfPkg/VirtHstiDxe/Flash.c |  90 +++
 OvmfPkg/VirtHstiDxe/QemuCommon.c|  36 ++
 OvmfPkg/VirtHstiDxe/QemuPC.c|  38 ++
 OvmfPkg/VirtHstiDxe/QemuQ35.c   |  71 
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c   | 173 
 OvmfPkg/VirtHstiDxe/README.md   |  48 
 14 files changed, 615 insertions(+)
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
 create mode 100644 OvmfPkg/VirtHstiDxe/Flash.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuCommon.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuPC.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuQ35.c
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
 create mode 100644 OvmfPkg/VirtHstiDxe/README.md

-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118023): https://edk2.groups.io/g/devel/message/118023
Mute This Topic: https://groups.io/mt/105616658/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2 1/1] OvmfPkg/IntelTdx: Update TDVF README

[Gerd Hoffmann]

On Fri, Apr 19, 2024 at 08:11:27AM +0800, Min Xu wrote:
> From: Min M Xu 
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4756
> 
> There are below updates in this patch:
> 1. Rename README to README.md so that it can be show as markdown
>document.
> 2. Update some information about TDVF.
> 2. Fix some typo.

Acked-by: Gerd Hoffmann 

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118021): https://edk2.groups.io/g/devel/message/118021
Mute This Topic: https://groups.io/mt/105609363/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

[Gerd Hoffmann]

  Hi,

> Gerd, any ideas?  Maybe I needs something subtly different in my
> edk2 build?  I've not looked at this bit of the qemu infrastructure
> before - is there a document on how that image is built?

There is roms/Makefile for that.

make -C roms help
make -C roms efi

So easiest would be to just update the edk2 submodule to what you
need, then rebuild.

The build is handled by the roms/edk2-build.py script,
with the build configuration being in roms/edk2-build.config.
That is usable outside the qemu source tree too, i.e. like this:

  python3 /path/to/qemu.git/roms/edk2-build.py \
--config /path/to/qemu.git/roms/edk2-build.config \
--core /path/to/edk2.git \
--match armvirt \
--silent --no-logs

That'll try to place the images build in "../pc-bios", so maybe better
work with a copy of the config file where you adjust this.

HTH,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118020): https://edk2.groups.io/g/devel/message/118020
Mute This Topic: https://groups.io/mt/105602816/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Gerd Hoffmann]

On Thu, Apr 18, 2024 at 08:39:20AM -0700, Adam Dunlap wrote:
> On Thu, Apr 18, 2024 at 5:15 AM Gerd Hoffmann  wrote:
> >
> > On Wed, Apr 17, 2024 at 09:54:00AM -0700, Adam Dunlap via groups.io wrote:
> > > +  UINT8  OpCode;
> >
> > The linux kernel patch uses "unsigned int opcode" and apparently
> > checks more than just the first byte for multi-byte opcodes.  Why
> > do it differently here?
> 
> Good question. This patch does check for two-byte opcodes with this snippet:
> 
> +  OpCode = *(InstructionData->OpCodes);
> +  if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
> +OpCode = *(InstructionData->OpCodes + 1);
> +  }
> 
> This works because the first byte of two-byte opcodes is always 0x0f in the
> cases that we're checking for.

Ok, missed that little detail.  Thanks for explaining.

Reviewed-by: Gerd Hoffmann 

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118018): https://edk2.groups.io/g/devel/message/118018
Mute This Topic: https://groups.io/mt/105581633/21656
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Gerd Hoffmann]

On Wed, Apr 17, 2024 at 09:54:00AM -0700, Adam Dunlap via groups.io wrote:
> Ensure that when a #VC exception happens, the instruction at the
> instruction pointer matches the instruction that is expected given the
> error code. This is to mitigate the ahoi WeSee attack [1] that could
> allow hypervisors to breach integrity and confidentiality of the
> firmware by maliciously injecting interrupts. This change is a
> translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
> instruction emulation somewhat")

> +**/
> +STATIC
> +UINT64
> +VcCheckOpcodeBytes (
> +  IN OUT GHCB*Ghcb,
> +  IN OUT EFI_SYSTEM_CONTEXT_X64  *Regs,
> +  IN OUT CC_INSTRUCTION_DATA *InstructionData,
> +  IN UINT64  ExitCode
> +  )
> +{
> +  UINT8  OpCode;

The linux kernel patch uses "unsigned int opcode" and apparently
checks more than just the first byte for multi-byte opcodes.  Why
do it differently here?

On the bigger picture:  I'm wondering why SNP allows external #VC
injections in the first place?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117985): https://edk2.groups.io/g/devel/message/117985
Mute This Topic: https://groups.io/mt/105581633/21656
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/IntelTdx: Update TDVF README

[Gerd Hoffmann]

  Hi,

> -The Intel? TDX Virtual Firmware Design Guide is at
> +The Intel TDX Virtual Firmware Design Guide is at

'' looks more like HTML than markdown.

text updates look fine to me.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117984): https://edk2.groups.io/g/devel/message/117984
Mute This Topic: https://groups.io/mt/105573458/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/4] OvmfPkg: Add VirtHstiDxe driver

[Gerd Hoffmann]

On Wed, Apr 17, 2024 at 01:20:57PM +, Yao, Jiewen wrote:
> That is good start. The SMRAM lock and Flash lock seem good to me.
> 
> Comment:
> 1) Do we really need to add "Q35" for the policy?
> #define VIRT_HSTI_BYTE0_Q35_SMM_SMRAM_LOCK BIT0
> #define VIRT_HSTI_BYTE0_Q35_SMM_SECURE_VARS_FLASH  BIT1
> 
> I feel we had better remove it, since SMM_SMRAM_LOCK and 
> SMM_SECURE_VARS_FLASH are common features for almost all X86 platforms.

Well, SMM mode is supported for the qemu 'q35' machine type only, the
'pc' machine type doesn't provide enough memory for SMM.  Which why I've
added 'Q35' to the name.

The SMM_SMRAM_LOCK test actually is q35-specific because the control
registers are chipset specific.  But, yes, the concept is not q35
specific.

I can drop 'Q35' if you prefer it that way.

> 2) Would you please let me know what "READONLY_CODE_FLASH" really means?
> 
> #define VIRT_HSTI_BYTE0_Q35_SMM_SECURE_VARS_FLASH  BIT1
> #define VIRT_HSTI_BYTE0_READONLY_CODE_FLASHBIT2
> 
> Does READONLY_CODE_FLASH mean NO write to flash even in SMM mode?
> Or does it just mean NO write in normal operation mode, but still writable in 
> SMM mode?

With qemu being configured properly flash behavior should be this:

   |  OVMF_CODE.fd  |  OVMF_VARS.fd
---++
SMM_REQUIRE=TRUE, SMM mode |  read-only |  writable
SMM_REQUIRE=TRUE, normal mode  |  read-only (1) |  read-only (2)
SMM_REQUIRE=FALSE  |  read-only (3) |  writable

VIRT_HSTI_BYTE0_READONLY_CODE_FLASH will verify (1) + (3).
VIRT_HSTI_BYTE0_Q35_SMM_SECURE_VARS_FLASH will verify (2).

(probably a good idea to add that as comment to the patches).

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117983): https://edk2.groups.io/g/devel/message/117983
Mute This Topic: https://groups.io/mt/105086174/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/4] OvmfPkg: Add VirtHstiDxe driver

[Gerd Hoffmann]

On Wed, Apr 17, 2024 at 01:38:20PM +0200, Ard Biesheuvel wrote:
> On Wed, 17 Apr 2024 at 10:18, Gerd Hoffmann  wrote:
> >
> > On Fri, Mar 22, 2024 at 03:27:31PM +0100, Gerd Hoffmann wrote:
> > >
> > >
> > > Gerd Hoffmann (2):
> > >   OvmfPkg/VirtHstiDxe: add varstore flash check
> > >   OvmfPkg/VirtHstiDxe: add code flash check
> > >
> > > Konstantin Kostiuk (2):
> > >   OvmfPkg: Add VirtHstiDxe driver
> > >   OvmfPkg: Add VirtHstiDxe to OVMF firmware build
> >
> > Ping.  Any comments on this series?
> >
> 
> Dunno. What does it do?

It implements 
https://learn.microsoft.com/en-us/windows-hardware/test/hlk/testref/hardware-security-testability-specification
for OVMF.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117982): https://edk2.groups.io/g/devel/message/117982
Mute This Topic: https://groups.io/mt/105086174/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Adjust the QemuFwCfgLibMmio and add PEI stage

[Gerd Hoffmann]

On Wed, Apr 17, 2024 at 04:12:56PM +0800, Chao Li wrote:
> Patch1: Added three PCDs for QemuFwCfgLibMmio
> Patch2: Sparate QemuFwCfgLibMmio.c into two files and default as DXE
> stage library.
> Patch3: Added QemuFwCfgMmiLib PEI version
> Patch4: Rename QemuFwCfgLibMmio.inf to QemuFwCfgMmioDxeLib.inf and
> enable it in AARCH64 and RISCV64.

Ok, I see, you are using the PCDs because global variables don't work
in PEI.

Alternative approach would be to create a HOB for that (see
EFI_HOB_PLATFORM_INFO used by X64).  Not sure this is a good idea
though given that we have three different architectures using that code.
Ard, any advise?

>   OvmfPkg: Add three PCDs for QemuFwCfgLib
>   OvmfPkg: Separate QemuFwCfgLibMmio.c into two files

This patch should be splitted into two, one doing the code split without
functional change, and one which switches from global variables to PCDs
(or HOB).

Otherwise this looks good to me (and I'd suggest to keep and merge this
as separate patch series).

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117912): https://edk2.groups.io/g/devel/message/117912
Mute This Topic: https://groups.io/mt/105573103/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/4] OvmfPkg: Add VirtHstiDxe driver

[Gerd Hoffmann]

On Fri, Mar 22, 2024 at 03:27:31PM +0100, Gerd Hoffmann wrote:
> 
> 
> Gerd Hoffmann (2):
>   OvmfPkg/VirtHstiDxe: add varstore flash check
>   OvmfPkg/VirtHstiDxe: add code flash check
> 
> Konstantin Kostiuk (2):
>   OvmfPkg: Add VirtHstiDxe driver
>   OvmfPkg: Add VirtHstiDxe to OVMF firmware build

Ping.  Any comments on this series?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117909): https://edk2.groups.io/g/devel/message/117909
Mute This Topic: https://groups.io/mt/105086174/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 21/26] OvmfPkg/LoongArchVirt: Add FdtQemuFwCfgLib

[Gerd Hoffmann]

On Wed, Apr 17, 2024 at 03:43:30PM +0800, Chao Li wrote:
> Hi Gerd,
> 
> 
> Thanks,
> Chao
> On 2024/4/17 14:59, Gerd Hoffmann wrote:
> > On Wed, Apr 17, 2024 at 10:53:21AM +0800, Chao Li wrote:
> > > Hi Gerd,
> > > 
> > > Part 2 has been be merged, I'm separating  this Lib into two serve the PEI
> > > stage and DXE stage.
> > > 
> > > Currently, This DXE library uses three global variables, and when I 
> > > simulate
> > > the no-mmio version: MmioLib.c + Dxe.c + Pei.c, I can abstract some helper
> > > functions as the public functions in Mmio version.
> > > 
> > > Do you mind if I replace these three vaiables with three dynamically typed
> > > PCDs? If so, the PEI and DXE stage libraries can using  some of the same
> > > APIs.
> > What is your idea?  Let PEI discover fw_cfg, store results in PCDs, and
> > DXE will read the PCDs instead of using FdtClientProtocol to find the
> > fw_cfg?
> > 
> > Note that risc-v and arm don't need access to fw_cfg in PEI, so this
> > approach would not work for them.
> 
> No no no, I mean these two stages have their own way to get the FDT
> resouces, for example: PEI stage uses fdtlib to find the fw_cfg and the DXE
> stage uses FdtClientProtocol to find the fw_cfg, DXE will look for fw_cfg
> anyway.

Ok.  I'm wondering what the point of using PCDs is then.

> Can I submit a patch set for quick review by you? It content only four
> patches.

Sure, maybe that is the easiest way to explain ;)

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117903): https://edk2.groups.io/g/devel/message/117903
Mute This Topic: https://groups.io/mt/104859897/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 0/5] Move Tdx specific lib from SecurityPkg to OvmfPkg

[Gerd Hoffmann]

On Tue, Apr 16, 2024 at 03:40:08PM +, Yao, Jiewen wrote:
> Yeah, I also considered that before. But after look at current code 
> structure, I give up.
> 
> Since following SEV component are NOT in AmdSev directory (especially the TCG 
> one), I do not see a strong reason to put them to IntelTdx dir.
> https://github.com/tianocore/edk2/tree/master/OvmfPkg/AmdSevDxe
> https://github.com/tianocore/edk2/tree/master/OvmfPkg/Tcg/TpmMmioSevDecryptPei
> https://github.com/tianocore/edk2/tree/master/OvmfPkg/Library/BaseMemEncryptSevLib

Yes, existing placement is inconsistent.  Some code is in
AmdSev / IntelTdx subdirs, some is not.

There are also some Tdx libraries in OvmfPkg/Library instead
of OvmfPkg/IntelTdx

> I think we can follow the existing code structure in this patch set.

OK

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117900): https://edk2.groups.io/g/devel/message/117900
Mute This Topic: https://groups.io/mt/105531957/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF

[Gerd Hoffmann]

On Tue, Apr 16, 2024 at 11:34:00AM +, Wu, Jiaxin wrote:
> Hi Gerd,
> 
> > Is the SmmRelocationLib approach supposed to work with mixed mode
> > firmware where PEI is running in ia32 mode and dxe/smm is running
> > in x64 mode (i.e. OvmfPkg/OvmfPkgIa32X64.dsc)?
> 
> Yes, I passed the test on the both OvmfPkgIa32X64 & OvmfPkgX64 for SMM 
> support.

The OVMF library I assume?
Did you try the AMD library too?

> It does has the problem on OvmfPkgIa32 for smm support (same as
> master). I did quick check, it's not only the CpuSaveState->x86 or
> CpuSaveState->x64 structure issue, but also has some problem to
> handler the smi hook return. We can handle this problem in another
> topic.

Yes, fixing ia32 can be done separately.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117897): https://edk2.groups.io/g/devel/message/117897
Mute This Topic: https://groups.io/mt/105441992/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 21/26] OvmfPkg/LoongArchVirt: Add FdtQemuFwCfgLib

[Gerd Hoffmann]

On Wed, Apr 17, 2024 at 10:53:21AM +0800, Chao Li wrote:
> Hi Gerd,
> 
> Part 2 has been be merged, I'm separating  this Lib into two serve the PEI
> stage and DXE stage.
> 
> Currently, This DXE library uses three global variables, and when I simulate
> the no-mmio version: MmioLib.c + Dxe.c + Pei.c, I can abstract some helper
> functions as the public functions in Mmio version.
> 
> Do you mind if I replace these three vaiables with three dynamically typed
> PCDs? If so, the PEI and DXE stage libraries can using  some of the same
> APIs.

What is your idea?  Let PEI discover fw_cfg, store results in PCDs, and
DXE will read the PCDs instead of using FdtClientProtocol to find the
fw_cfg?

Note that risc-v and arm don't need access to fw_cfg in PEI, so this
approach would not work for them.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117896): https://edk2.groups.io/g/devel/message/117896
Mute This Topic: https://groups.io/mt/104859897/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF

[Gerd Hoffmann]

  Hi,

> > > 2) Existing SmBase configuration is different between the AMD & OVMF.
> > > OVMF:
> > >  AmdCpuState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> > >
> > > AMD:
> > >  if ((CpuSaveState->x86.SMMRevId & 0x) == 0) {
> > > CpuSaveState->x86.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> > >   } else {
> > > CpuSaveState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> > >   }

> I don't the background why AMD and OVMF has such difference. Maybe OVFM 
> doesn't not support the MSR "EFER_ADDRESS".

It surely does, it's a rather essential MSR for x64 CPUs.

> > The SmBase configuration for OVMF looks suspicious to me.  I'm wondering
> > whenever the OVMF code actually works in Ia32 builds ...

Tested OvmfPkg/OvmfPkgIa32.dsc with SMM_REQUIRE == TRUE (master branch).
Doesn't boot.  The difference above (where the OVMF code does not
consider the 32bit case) could very well explain why 32bit support is
broken.  Switching to the AMD code might actually fix that.

Is the SmmRelocationLib approach supposed to work with mixed mode
firmware where PEI is running in ia32 mode and dxe/smm is running
in x64 mode (i.e. OvmfPkg/OvmfPkgIa32X64.dsc)?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117877): https://edk2.groups.io/g/devel/message/117877
Mute This Topic: https://groups.io/mt/105441992/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 0/5] Move Tdx specific lib from SecurityPkg to OvmfPkg

[Gerd Hoffmann]

On Mon, Apr 15, 2024 at 03:55:49PM +0800, Min Xu wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4752
> 
> HashLibTdx and TdTcg2Dxe are designed for Intel TDX enlightened OVMF.
> They're more reasonable to be put in OvmfPkg than in SecurityPkg.
> 
> SecTpmMeasurementLibTdx is not used anymore. So it is deleted in this
> patch-set.
> 

>  rename {SecurityPkg => OvmfPkg}/Library/HashLibTdx/HashLibTdx.c (100%)
>  rename {SecurityPkg => OvmfPkg}/Library/HashLibTdx/HashLibTdx.inf (100%)
>  rename {SecurityPkg => OvmfPkg}/Tcg/TdTcg2Dxe/MeasureBootPeCoff.c (100%)
>  rename {SecurityPkg => OvmfPkg}/Tcg/TdTcg2Dxe/TdTcg2Dxe.c (100%)
>  rename {SecurityPkg => OvmfPkg}/Tcg/TdTcg2Dxe/TdTcg2Dxe.inf (100%)

Better place them in OvmfPkg/IntelTdx ?

Otherwise looks fine to me.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117875): https://edk2.groups.io/g/devel/message/117875
Mute This Topic: https://groups.io/mt/105531957/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: Warn if out of space when writing variables

[Gerd Hoffmann]

On Mon, Apr 15, 2024 at 09:46:37PM +0200, Oliver Steffen wrote:
> Emit a DEBUG_ERROR message if there is not enough flash variable left to
> write/update a variable. This condition is currently not logged
> appropriately in all cases, given that full variable store can easily
> render the system unbootable.
> This new message helps identifying this condition.
> 
> Cc: Bob Feng 
> Cc: Gerd Hoffmann 
> Cc: Jiewen Yao 
> Cc: Laszlo Ersek 
> Cc: Liming Gao 
> Cc: Rahul Kumar 
> Cc: Rebecca Cran 
> Cc: Yuwei Chen 
> 
> Signed-off-by: Oliver Steffen 
> Reviewed-by: Laszlo Ersek 

Reviewed-by: Gerd Hoffmann 

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117854): https://edk2.groups.io/g/devel/message/117854
Mute This Topic: https://groups.io/mt/105543779/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF

[Gerd Hoffmann]

On Mon, Apr 15, 2024 at 01:04:58PM +, Wu, Jiaxin wrote:
> Hi Gred,
> 
> Because:
> 1) The mode of the CPU check is different between the AMD & OVMF.
> OVMF: 
> CpuSaveState->x86.SMMRevId & 0X
> 
> AMD:
>  LMAValue = (UINT32)AsmReadMsr64 (EFER_ADDRESS) & LMA
> 
> 2) Existing SmBase configuration is different between the AMD & OVMF.
> OVMF:
>  AmdCpuState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
> 
> AMD:   
>  if ((CpuSaveState->x86.SMMRevId & 0x) == 0) {
> CpuSaveState->x86.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
>   } else {
> CpuSaveState->x64.SMBASE = (UINT32)mSmBaseForAllCpus[CpuIndex];
>   }
> 
> This series patch won't change the existing implementation code logic, so, we 
> need override one version for OVMF.

The real question is why do these differences exist and are they
actually needed.

I'd expect the CPU mode check return identical results.

The SmBase configuration for OVMF looks suspicious to me.  I'm wondering
whenever the OVMF code actually works in Ia32 builds ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117853): https://edk2.groups.io/g/devel/message/117853
Mute This Topic: https://groups.io/mt/105441992/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 00/10] Add SmmRelocationLib

[Gerd Hoffmann]

On Mon, Apr 15, 2024 at 09:30:11PM +0800, Wu, Jiaxin wrote:
> Intel plans to separate the smbase relocation logic from
> PiSmmCpuDxeSmm driver, and the related behavior will be
> moved to the new interface defined by the SmmRelocationLib
> class.
> 
> The SmmRelocationLib class provides the SmmRelocationInit()
> interface for platform to do the smbase relocation, which
> shall provide below 2 functionalities:
> 1. Relocate smbases for each processor.
> 2. Create the gSmmBaseHobGuid HOB.
> 
> With SmmRelocationLib, PiSmmCpuDxeSmm driver (which runs at
> a later phase) can be simplfied as below for SMM init:
> 1. Consume the gSmmBaseHobGuid HOB for the relocated smbases
> for each Processor.
> 2. Execute the early SMM Init.

How this was tested?
I can't even build this (without -D SMM_REQUIRE=TRUE).

/home/kraxel/projects/edk2/OvmfPkg/OvmfPkgX64.dsc(...): error 4000: Instance of 
library class [SmmRelocationLib] is not found
in [/home/kraxel/projects/edk2/OvmfPkg/PlatformPei/PlatformPei.inf] 
[X64]
consumed by module 
[/home/kraxel/projects/edk2/OvmfPkg/PlatformPei/PlatformPei.inf]

I doubt it passes CI.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117852): https://edk2.groups.io/g/devel/message/117852
Mute This Topic: https://groups.io/mt/105535804/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] ACPI table generators and ConfigurationManagerProtocol

[Gerd Hoffmann]

  Hi,

> > And tell which of platforms is a good example of using those?
> 
> Juno, FVP, Morello, N1SDP, one NXP platform and ArmVirt use them. Probably
> the last one would be best to look at but who knows...

Probably not ArmVirt.  At least not the qemu variant, maybe the kvmtool
version.

On qemu the usual workflow is that qemu generates the acpi tables,
matching the virtual machine configuration, and the firmware just
downloads and installs them.

> > From first look it seems like using ACPI table generators may allow to
> > simplify code by not creating tables by hand (or in ASL). I would
> > like to do some changes around SBSA Reference Platform without rewriting
> > ASL into C again.
> 
> In meantime I rewrote some ASL into C. Again. Now need a way to generate
> DSDT for PCIe buses. Can write something in C again. But do I really need
> to?

Is there a non-qemu implementation of the SBSA Reference Platform?
If not it might be easiest to offload acpi table generation to qemu.

If the acpi tables are not changing you might have a look at
OvmfPkg/Bhyve/AcpiTables instead of going for DynamicTablesPkg.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117668): https://edk2.groups.io/g/devel/message/117668
Mute This Topic: https://groups.io/mt/105481311/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH] OvmfPkg/SecurityPkg: Add build option for coexistance of vTPM and RTMR.

[Gerd Hoffmann]

On Thu, Apr 11, 2024 at 09:56:48AM +, Yao, Jiewen wrote:
> Please allow me to clarify what you are proposing:
> Do you mean in vTPM case, we extend both, but we only need TCG event log, NOT 
> CC event log?

Elsewhere in this thread it was mentioned that writing both vTPM and
RTMR events to the event log is problematic because the event log format
has no field to specify whenever a given event was measured to vTPM or
RTMR.

If the firmware can make sure all events are measured to both vTPM and
RTMR the need to trace them separately goes away.

So, yes, in case a vTPM is present the firmware would:
  (a) expose EFI_TCG2_PROTOCOL, measure to both vTPM + RTMR
  (b) not expose EFI_CC_MEASUREMENT_PROTOCOL
  (c) log measurements to TCG event log

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117629): https://edk2.groups.io/g/devel/message/117629
Mute This Topic: https://groups.io/mt/105070442/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/4] OvmfPkg/Sec: Setup MTRR early in the boot process.

[Gerd Hoffmann]

  Hi,

> > > @Gerd, what's the qemu command and test environment your QE
> > > run the case? We'd like run it in our side.
> > 
> > 
> > 
> > Tested edk2-ovmf-20231122-1.el9.rhel21704.20240202.1130.noarch with
> > TDX guest, no issue found
> > 
> > Version:
> > 
> > edk2-ovmf-20231122-1.el9.rhel21704.20240202.1130.noarch
> > 
> > guest kernel: 5.14.0-415.el9.x86_64
> > 
> > qemu-kvm-8.0.0-15.el9s.x86_64
> > host kernel-5.14.0-411.test.el9s.x86_64
> > 
> > Steps:
> > 
> > $ sudo /usr/libexec/qemu-kvm  -accel kvm   -drive
> > file=/home/zixchen/rhel94_tdx.qcow2,if=none,id=virtio-disk0   -device
> > virtio-blk-pci,drive=virtio-disk0   -cpu host -smp 16 -m 10240 -
> > object tdx-guest,id=tdx,debug=on   -machine
> > q35,hpet=off,kernel_irqchip=split,memory-encryption=tdx,confidential-
> > guest-support=tdx,memory-backend=ram1   -object memory-backend-
> > ram,id=ram1,size=10240M,private=on  -nographic -vga none   -
> > nodefaults -bios /usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd  -
> > serial stdio  -netdev user,id=user.0 -device e1000,netdev=user.0
> > 
> > $ dmesg|grep -i tdx
> > [    0.00] tdx: Guest detected
> > [    0.719122] TECH PREVIEW: Intel Trusted Domain Extensions (TDX)
> > may not be fully supported.
> > [    0.719122]  Intel TDX
> > [    0.719122] process: using TDX aware idle routine
> > 
> > 
> > 
> > Host configuration with the tdx test packages:
> > https://sigs.centos.org/virt/tdx/host/
> > 
> > Latest edk2 build (stable202311 + patches) has the patch series
> > included:
> > 
> > https://kojihub.stream.centos.org/koji/buildinfo?buildID=56985
> > 
> > take care,
> >   Gerd
> 
> Hi,
> 
> any progress on that patch?
> 
> I'm currently trying to passthrough the integrated GPU of an Intel
> CPUs. When I add the GPU to the qemu command, I'm faced with the
> descripted issue. This patch solves the issue.

On my end the state of affairs is unchanged.  Our builds have the patch
included and there are zero problems so far, the issue reported by Min
doesn't reproduce and it is still unclear what is going on.

Min, any update?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117620): https://edk2.groups.io/g/devel/message/117620
Mute This Topic: https://groups.io/mt/104052591/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH] OvmfPkg/SecurityPkg: Add build option for coexistance of vTPM and RTMR.

[Gerd Hoffmann]

  Hi,
 
> Given that RTMR is a proper subset of vTPM (modulo the PCR/RTMR index
> conversion), I feel that it should be the CoCo firmware's
> responsibility to either:
> - expose RTMR and not vTPM
> - expose vTPM, and duplicate each measurement into RTMR as they are taken

That approach looks good to me.  It will make sure vTPM and RTMR
measurements are consistent and it also solves the event log issue
(we don't need separate vTPM and RTMR entries then).

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117619): https://edk2.groups.io/g/devel/message/117619
Mute This Topic: https://groups.io/mt/105070442/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 03/13] UefiCpuPkg/SmmRelocationLib: Add library instance for OVMF

[Gerd Hoffmann]

On Wed, Apr 10, 2024 at 09:57:14PM +0800, Jiaxin Wu wrote:
> Due to the definition difference of SMRAM Save State,
> SmmBase config in SMRAM Save State for OVMF is also different.
> 
> This patch provides the OvmfSmmRelocationLib library instance
> to handle the SMRAM Save State difference.

Why ovmf needs its own version?  Patch #4 adds an AMD version, and given
that KVM uses the AMD smram layout that library should work for OVMF
too, no?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117618): https://edk2.groups.io/g/devel/message/117618
Mute This Topic: https://groups.io/mt/105441992/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg: OVMF supports USB mouses

[Gerd Hoffmann]

On Tue, Apr 09, 2024 at 04:51:20PM +0100, Pedro Falcato wrote:
> On Tue, Apr 9, 2024 at 12:56 PM Gerd Hoffmann  wrote:
> >
> > On Mon, Apr 08, 2024 at 08:53:10AM +0100, Phillip Tennen wrote:
> > > Hi, thank you for taking a look at the patch!
> > >
> > > This patch can be verified to be working with this app (which was the
> > > motivation for submitting this):
> > > https://github.com/codyd51/uefirc/releases/tag/1.0.1.
> >
> > Quoting https://github.com/codyd51/uefirc:
> >
> > Q: Should I use this?
> > A: This should not exist.
> >
> > Well.  This certainly one of the more interesting ways to have some fun
> > and improve your rust coding skills.  But a justification to include a
> > mouse driver by default which is not used by anything else?  IMHO it
> > isn't.
> 
> Maybe some better reasons:
> 
> 1) It has been conspicuously missing from OVMF. I've heard N questions
> over the years (on the #osdev IRC, etc) regarding their mouse code not
> working on OVMF, whereas you'd see that protocol in other normal
> platforms

Those other platforms often have a funky setup application which (unlike
UiApp) actually support using the mouse.

So, I'm still wondering what applications people want use the mouse for?

> For sure, UsbMouseDxe isn't #1 on my most desired EFI modules list
> (e.g I'd love to eventually be able to consume Ext4Dxe from OVMF,
> where it'd actually be useful, if I can ever ditch edk2-platforms),
> but I don't really see the harm in doing it.

UsbMouseDxe is IMHO the least useful driver.

We have:
  - Ps2MouseDxe, exposing EFI_SIMPLE_POINTER_PROTOCOL, and
  - UsbMouseDxe, exposing EFI_SIMPLE_POINTER_PROTOCOL too, and
  - UsbMouseAbsolutePointerDxe, exposing EFI_ABSOLUTE_POINTER_PROTOCOL.

On the qemu side a ps/2 mouse is always present.  Working with a
relative pointer device in a virtual machine isn't very smooth though,
which why qemu offers absolute pointer devices as alternative
(usb-tablet, virtio-tablet).

So a typical configuration (on x64, aa64 is a different story) is to
have a ps/2 mouse and an usb tablet.  qemu will start in relative
pointer mode and send events to the mouse.  As soon as the guest
activates the tablet (typically when the linux kernel loads the usb hid
drivers) qemu switches into absolute pointer mode and sends events to
the tablet.

Linux applications don't have to worry about relative vs. absolute
pointer mode.  The display server (x11/wayland) will deal with that for
them, they get the pointer position already translated to screen
coordinates.

That is not the case for efi applications though.  If they want work
with both relative and absolute pointing devices they have to implement
both protocols.

Now the tricky part here is that efi applications implementing only
EFI_SIMPLE_POINTER_PROTOCOL will *not* work in case
UsbMouseAbsolutePointerDxe included in the firmware image.  Loading the
driver will activate absolute pointer mode and qemu will route events
to the tablet not the mouse ...

> There's an argument in giving people a full-fledged UEFI
> implementation of most protocols. OVMF is *the* platform in mainline
> edk2 after all :)

If we do that we IMHO should
 (a) add a config option for that (similar to the rarely used scsi
 drivers),
 (b) update all ovmf variants consistently, and
 (c) include both ps/2 and usb mouse drivers.

Not sure whenever it is a good idea to include
UsbMouseAbsolutePointerDxe.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117584): https://edk2.groups.io/g/devel/message/117584
Mute This Topic: https://groups.io/mt/105365480/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg: OVMF supports USB mouses

[Gerd Hoffmann]

On Mon, Apr 08, 2024 at 08:53:10AM +0100, Phillip Tennen wrote:
> Hi, thank you for taking a look at the patch!
> 
> This patch can be verified to be working with this app (which was the
> motivation for submitting this):
> https://github.com/codyd51/uefirc/releases/tag/1.0.1.

Quoting https://github.com/codyd51/uefirc:

Q: Should I use this?
A: This should not exist.

Well.  This certainly one of the more interesting ways to have some fun
and improve your rust coding skills.  But a justification to include a
mouse driver by default which is not used by anything else?  IMHO it
isn't.

Note that you can load drivers from efi shell with the 'load' command,
so there is no need to have a ovmf firmware image with the mouse drivers
included.  You can boot into efi shell and use a startup.nsh script to
automatically load drivers needed and start the irc app.

HTH & take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117544): https://edk2.groups.io/g/devel/message/117544
Mute This Topic: https://groups.io/mt/105365480/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] OVMF SMM Support

[Gerd Hoffmann]

On Mon, Apr 08, 2024 at 08:33:30AM +, Wu, Jiaxin wrote:
> Hi Gerd,
> 
> With below OVMF build and QEMU command, OVMF hangs after SendSmiIpi
> (mBspApicId) during SmmRelocateBases(), is there any issues with
> latest code to support SMM on OVMF or my local command/configuration
> issue?

Have not noticed any problems.  I've stopped using OvmfPkgIa32X64.dsc
though and switched to OvmfPkgX64.dsc.  Also running on linux not
windows host, which might make a difference too.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117495): https://edk2.groups.io/g/devel/message/117495
Mute This Topic: https://groups.io/mt/105397533/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg: OVMF supports USB mouses

[Gerd Hoffmann]

On Sat, Apr 06, 2024 at 02:41:54PM +0200, Heinrich Schuchardt wrote:
> From: Phillip Tennen 
> 
> From: Phillip Tennen 
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4747
> 
> UsbMouseDxe was missing from the OVMF build description, so=20
> the Simple Pointer Protocol wasn't usable from within QEMU.

What is the use case?

How can this be tested?  As far I know neither the edk2 setup utility
(aka UiApp) nor typical OS boot loaders have mouse support ...

Also note that virtual machines typically do *not* have a mouse but a
tablet device, so UsbMouseAbsolutePointerDxe looks like the more
sensible choice to me.

>  OvmfPkg/OvmfPkgX64.dsc | 1 +
>  OvmfPkg/OvmfPkgX64.fdf | 1 +

What about the other ovmf variants?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117492): https://edk2.groups.io/g/devel/message/117492
Mute This Topic: https://groups.io/mt/105365480/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[Gerd Hoffmann]

On Tue, Mar 26, 2024 at 09:08:59AM +, Sun, CepingX wrote:
> On Friday, March 22, 2024 5:06 PM Gerd Hoffmann wrote:
> > 
> > No, we only need to update QemuFwCfgSelectItem + QemuFwCfgReadBytes to
> > support reading from the cache.
> Do you mean the existing API (QemuFwCfgSelectItem + QemuFwCfgReadBytes) need 
> to be changed to support reading from the cache?
> 
> If that is the case,  there are some concerns as below:
> 1:  One or more new parameters (of QemuFwCfgReadBytes())  need to be added to 
> search 
> the item in cache, which is equivalent to adding a new API.

No.

Yes, you need to maintain some extra state, so you know which item was
selected most recently and how many bytes have been read already.

It's not needed to task the caller with that though.  Alternatively you
can add fields to EFI_HOB_PLATFORM_INFO, or create a new HOB for that,
and store the state there.  That way QemuFwCfgReadBytes works properly
with the cache without the caller passing in the state.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117129): https://edk2.groups.io/g/devel/message/117129
Mute This Topic: https://groups.io/mt/104880546/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 4/4] OvmfPkg/VirtHstiDxe: add code flash check

[Gerd Hoffmann]

Detects qemu config issue: code pflash is writable.
Checked for both PC and Q35.

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
---
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  2 ++
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   | 13 +++
 OvmfPkg/VirtHstiDxe/QemuCommon.c| 36 +
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c   |  4 
 4 files changed, 55 insertions(+)
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuCommon.c

diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
index 9cb2ed1f0c64..376cd28aeb7e 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
@@ -22,6 +22,7 @@ [Sources]
   VirtHstiDxe.c
   QemuPC.c
   QemuQ35.c
+  QemuCommon.c
   Flash.c
 
 [Packages]
@@ -49,6 +50,7 @@ [FeaturePcd]
 
 [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+  gUefiOvmfPkgTokenSpaceGuid.PcdBfvBase
 
 [Depex]
   TRUE
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
index ca4e376582ad..b915f5cdf5ac 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
@@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #define VIRT_HSTI_BYTE0_Q35_SMM_SMRAM_LOCK BIT0
 #define VIRT_HSTI_BYTE0_Q35_SMM_SECURE_VARS_FLASH  BIT1
+#define VIRT_HSTI_BYTE0_READONLY_CODE_FLASHBIT2
 
 typedef struct {
   // ADAPTER_INFO_PLATFORM_SECURITY
@@ -67,6 +68,18 @@ VirtHstiQemuPCVerify (
   VOID
   );
 
+/* QemuCommon.c */
+
+VOID
+VirtHstiQemuCommonInit (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti
+  );
+
+VOID
+VirtHstiQemuCommonVerify (
+  VOID
+  );
+
 /* Flash.c */
 
 #define QEMU_FIRMWARE_FLASH_UNKNOWN0
diff --git a/OvmfPkg/VirtHstiDxe/QemuCommon.c b/OvmfPkg/VirtHstiDxe/QemuCommon.c
new file mode 100644
index ..4ab3fe2d6e63
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/QemuCommon.c
@@ -0,0 +1,36 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 
+#include 
+
+#include "VirtHstiDxe.h"
+
+VOID
+VirtHstiQemuCommonInit (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti
+  )
+{
+  VirtHstiSetSupported (VirtHsti, 0, VIRT_HSTI_BYTE0_READONLY_CODE_FLASH);
+}
+
+VOID
+VirtHstiQemuCommonVerify (
+  VOID
+  )
+{
+  CHAR16  *ErrorMsg;
+
+  switch (VirtHstiQemuFirmwareFlashCheck (PcdGet32 (PcdBfvBase))) {
+case QEMU_FIRMWARE_FLASH_WRITABLE:
+  ErrorMsg = L"qemu code pflash is writable";
+  break;
+default:
+  ErrorMsg = NULL;
+  }
+
+  VirtHstiTestResult (ErrorMsg, 0, VIRT_HSTI_BYTE0_READONLY_CODE_FLASH);
+}
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
index 74e5e6bd9d4f..b6e53a1219d1 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.c
@@ -104,9 +104,11 @@ VirtHstiOnReadyToBoot (
   switch (VirtHstiGetHostBridgeDevId ()) {
 case INTEL_82441_DEVICE_ID:
   VirtHstiQemuPCVerify ();
+  VirtHstiQemuCommonVerify ();
   break;
 case INTEL_Q35_MCH_DEVICE_ID:
   VirtHstiQemuQ35Verify ();
+  VirtHstiQemuCommonVerify ();
   break;
 default:
   ASSERT (FALSE);
@@ -142,9 +144,11 @@ VirtHstiDxeEntrypoint (
   switch (DevId) {
 case INTEL_82441_DEVICE_ID:
   VirtHsti = VirtHstiQemuPCInit ();
+  VirtHstiQemuCommonInit (VirtHsti);
   break;
 case INTEL_Q35_MCH_DEVICE_ID:
   VirtHsti = VirtHstiQemuQ35Init ();
+  VirtHstiQemuCommonInit (VirtHsti);
   break;
 default:
   DEBUG ((DEBUG_INFO, "%a: unknown platform (0x%x)\n", __func__, DevId));
-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117051): https://edk2.groups.io/g/devel/message/117051
Mute This Topic: https://groups.io/mt/105086175/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 0/4] OvmfPkg: Add VirtHstiDxe driver

[Gerd Hoffmann]

Gerd Hoffmann (2):
  OvmfPkg/VirtHstiDxe: add varstore flash check
  OvmfPkg/VirtHstiDxe: add code flash check

Konstantin Kostiuk (2):
  OvmfPkg: Add VirtHstiDxe driver
  OvmfPkg: Add VirtHstiDxe to OVMF firmware build

 OvmfPkg/OvmfPkgIa32.dsc |   2 +
 OvmfPkg/OvmfPkgIa32X64.dsc  |   2 +
 OvmfPkg/OvmfPkgX64.dsc  |   2 +
 OvmfPkg/OvmfPkgIa32.fdf |   1 +
 OvmfPkg/OvmfPkgIa32X64.fdf  |   1 +
 OvmfPkg/OvmfPkgX64.fdf  |   1 +
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  56 +
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   |  94 +++
 OvmfPkg/VirtHstiDxe/Flash.c |  90 +++
 OvmfPkg/VirtHstiDxe/QemuCommon.c|  36 ++
 OvmfPkg/VirtHstiDxe/QemuPC.c|  38 ++
 OvmfPkg/VirtHstiDxe/QemuQ35.c   |  71 
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c   | 173 
 13 files changed, 567 insertions(+)
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
 create mode 100644 OvmfPkg/VirtHstiDxe/Flash.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuCommon.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuPC.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuQ35.c
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c

-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117050): https://edk2.groups.io/g/devel/message/117050
Mute This Topic: https://groups.io/mt/105086174/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 3/4] OvmfPkg/VirtHstiDxe: add varstore flash check

[Gerd Hoffmann]

Detects qemu config issue: vars pflash is not in secure mode (write
access restricted to smm).  Applies to Q35 with SMM only.

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
---
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  4 ++
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   | 16 -
 OvmfPkg/VirtHstiDxe/Flash.c | 90 +
 OvmfPkg/VirtHstiDxe/QemuQ35.c   | 13 +
 4 files changed, 122 insertions(+), 1 deletion(-)
 create mode 100644 OvmfPkg/VirtHstiDxe/Flash.c

diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
index 8c63ff6a8953..9cb2ed1f0c64 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
@@ -22,6 +22,7 @@ [Sources]
   VirtHstiDxe.c
   QemuPC.c
   QemuQ35.c
+  Flash.c
 
 [Packages]
   MdePkg/MdePkg.dec
@@ -46,5 +47,8 @@ [Guids]
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
 
+[Pcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+
 [Depex]
   TRUE
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
index 26109bf322e9..ca4e376582ad 100644
--- a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
@@ -6,7 +6,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #define VIRT_HSTI_SECURITY_FEATURE_SIZE  2
 
-#define VIRT_HSTI_BYTE0_Q35_SMM_SMRAM_LOCK  BIT0
+#define VIRT_HSTI_BYTE0_Q35_SMM_SMRAM_LOCK BIT0
+#define VIRT_HSTI_BYTE0_Q35_SMM_SECURE_VARS_FLASH  BIT1
 
 typedef struct {
   // ADAPTER_INFO_PLATFORM_SECURITY
@@ -65,3 +66,16 @@ VOID
 VirtHstiQemuPCVerify (
   VOID
   );
+
+/* Flash.c */
+
+#define QEMU_FIRMWARE_FLASH_UNKNOWN0
+#define QEMU_FIRMWARE_FLASH_IS_ROM 1
+#define QEMU_FIRMWARE_FLASH_IS_RAM 2
+#define QEMU_FIRMWARE_FLASH_READ_ONLY  3
+#define QEMU_FIRMWARE_FLASH_WRITABLE   4
+
+UINT32
+VirtHstiQemuFirmwareFlashCheck (
+  UINT32  Address
+  );
diff --git a/OvmfPkg/VirtHstiDxe/Flash.c b/OvmfPkg/VirtHstiDxe/Flash.c
new file mode 100644
index ..e93356793f8c
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/Flash.c
@@ -0,0 +1,90 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 
+#include 
+
+#include "VirtHstiDxe.h"
+
+#define WRITE_BYTE_CMD   0x10
+#define BLOCK_ERASE_CMD  0x20
+#define CLEAR_STATUS_CMD 0x50
+#define READ_STATUS_CMD  0x70
+#define READ_DEVID_CMD   0x90
+#define BLOCK_ERASE_CONFIRM_CMD  0xd0
+#define READ_ARRAY_CMD   0xff
+#define CLEARED_ARRAY_STATUS 0x00
+
+/* based on QemuFlashDetected (QemuFlashFvbServicesRuntimeDxe) */
+UINT32
+VirtHstiQemuFirmwareFlashCheck (
+  UINT32  Address
+  )
+{
+  volatile UINT8  *Ptr;
+
+  UINTN  Offset;
+  UINT8  OriginalUint8;
+  UINT8  ProbeUint8;
+
+  for (Offset = 0; Offset < EFI_PAGE_SIZE; Offset++) {
+Ptr= (UINT8 *)(UINTN)(Address + Offset);
+ProbeUint8 = *Ptr;
+if ((ProbeUint8 != CLEAR_STATUS_CMD) &&
+(ProbeUint8 != READ_STATUS_CMD) &&
+(ProbeUint8 != CLEARED_ARRAY_STATUS))
+{
+  break;
+}
+  }
+
+  if (Offset >= EFI_PAGE_SIZE) {
+DEBUG ((DEBUG_INFO, "%a: check failed\n", __func__));
+return QEMU_FIRMWARE_FLASH_UNKNOWN;
+  }
+
+  OriginalUint8 = *Ptr;
+  *Ptr  = CLEAR_STATUS_CMD;
+  ProbeUint8= *Ptr;
+  if ((OriginalUint8 != CLEAR_STATUS_CMD) &&
+  (ProbeUint8 == CLEAR_STATUS_CMD))
+  {
+*Ptr = OriginalUint8;
+DEBUG ((DEBUG_INFO, "%a: %p behaves as RAM\n", __func__, Ptr));
+return QEMU_FIRMWARE_FLASH_IS_RAM;
+  }
+
+  *Ptr   = READ_STATUS_CMD;
+  ProbeUint8 = *Ptr;
+  if (ProbeUint8 == OriginalUint8) {
+DEBUG ((DEBUG_INFO, "%a: %p behaves as ROM\n", __func__, Ptr));
+return QEMU_FIRMWARE_FLASH_IS_ROM;
+  }
+
+  if (ProbeUint8 == READ_STATUS_CMD) {
+*Ptr = OriginalUint8;
+DEBUG ((DEBUG_INFO, "%a: %p behaves as RAM\n", __func__, Ptr));
+return QEMU_FIRMWARE_FLASH_IS_RAM;
+  }
+
+  if (ProbeUint8 == CLEARED_ARRAY_STATUS) {
+*Ptr   = WRITE_BYTE_CMD;
+*Ptr   = OriginalUint8;
+*Ptr   = READ_STATUS_CMD;
+ProbeUint8 = *Ptr;
+*Ptr   = READ_ARRAY_CMD;
+if (ProbeUint8 & 0x10 /* programming error */) {
+  DEBUG ((DEBUG_INFO, "%a: %p behaves as FLASH, write-protected\n", 
__func__, Ptr));
+  return QEMU_FIRMWARE_FLASH_READ_ONLY;
+} else {
+  DEBUG ((DEBUG_INFO, "%a: %p behaves as FLASH, writable\n", __func__, 
Ptr));
+  return QEMU_FIRMWARE_FLASH_WRITABLE;
+}
+  }
+
+  DEBUG ((DEBUG_INFO, "%a: check failed\n", __func__));
+  return QEMU_FIRMWARE_FLASH_UNKNOWN;
+}
diff --git a/OvmfPkg/VirtHstiDxe/QemuQ35.c b/OvmfPkg/VirtHstiDxe/QemuQ35.c
index 75e9731b4a52..203122627d2d 100644
--- a/OvmfPkg/VirtHstiDxe/QemuQ35.c
+++ b/OvmfPkg/VirtHstiDxe/QemuQ35.c
@@ -29,6 +29,7 @@ VirtHstiQemuQ35Init (
 {
   if (FeaturePcdGet (PcdSmmSm

[edk2-devel] [PATCH 2/4] OvmfPkg: Add VirtHstiDxe to OVMF firmware build

[Gerd Hoffmann]

From: Konstantin Kostiuk 

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Signed-off-by: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
---
 OvmfPkg/OvmfPkgIa32.dsc| 2 ++
 OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
 OvmfPkg/OvmfPkgX64.dsc | 2 ++
 OvmfPkg/OvmfPkgIa32.fdf| 1 +
 OvmfPkg/OvmfPkgIa32X64.fdf | 1 +
 OvmfPkg/OvmfPkgX64.fdf | 1 +
 6 files changed, 9 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 713f08764b07..52b05cb373fb 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -188,6 +188,7 @@ [LibraryClasses]
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
+  HstiLib|MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
 !endif
@@ -828,6 +829,7 @@ [Components]
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
   #
   # ISA Support
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 90b15dc27097..be386914b047 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -193,6 +193,7 @@ [LibraryClasses]
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
+  HstiLib|MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
 !endif
@@ -842,6 +843,7 @@ [Components.X64]
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
   #
   # ISA Support
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 56c920168d25..ae8c7e2fbdcb 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -205,6 +205,7 @@ [LibraryClasses]
   PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf
   DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf
   
ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
+  HstiLib|MdePkg/Library/DxeHstiLib/DxeHstiLib.inf
 
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
@@ -910,6 +911,7 @@ [Components]
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
+  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
   #
   # ISA Support
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 6c56c5e53f21..6eb26f7d4613 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -316,6 +316,7 @@ [FV.DXEFV]
 INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
 INF  OvmfPkg/PlatformDxe/Platform.inf
 INF  OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+INF  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
 !if $(SMM_REQUIRE) == TRUE
 INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index ee8068ad55dc..080784f722a7 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -323,6 +323,7 @@ [FV.DXEFV]
 INF  OvmfPkg/PlatformDxe/Platform.inf
 INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
 INF  OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+INF  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
 !if $(SMM_REQUIRE) == TRUE
 INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index eb3fb90cb8b6..5bc979bc91b7 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -350,6 +350,7 @@ [FV.DXEFV]
 INF  OvmfPkg/PlatformDxe/Platform.inf
 INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
 INF  OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+INF  OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 
 !if $(SMM_REQUIRE) == TRUE
 INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
-- 
2.44.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117048): https://edk2.groups.io/g/devel/message/117048
Mute This Topic: https://groups.io/mt/105086166/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/4] OvmfPkg: Add VirtHstiDxe driver

[Gerd Hoffmann]

From: Konstantin Kostiuk 

The driver supports qemu machine types 'pc' and 'q35'.

This patch adds some helper functions to manage the bitmasks.
The implemented features depend on both OVMF build configuration
and qemu VM configuration.

For q35 a single security feature is supported and checked: In
SMM-enabled builds the driver will verify smram is properly locked.
That test should never fail.

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Konstantin Kostiuk 
Initial-patch-by: Konstantin Kostiuk 
Signed-off-by: Gerd Hoffmann 
---
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf |  50 
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h   |  67 +++
 OvmfPkg/VirtHstiDxe/QemuPC.c|  38 +++
 OvmfPkg/VirtHstiDxe/QemuQ35.c   |  58 ++
 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c   | 169 
 5 files changed, 382 insertions(+)
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuPC.c
 create mode 100644 OvmfPkg/VirtHstiDxe/QemuQ35.c
 create mode 100644 OvmfPkg/VirtHstiDxe/VirtHstiDxe.c

diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
new file mode 100644
index ..8c63ff6a8953
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.inf
@@ -0,0 +1,50 @@
+## @file
+#  Component description file for Virt Hsti Driver
+#
+# Copyright (c) 2017, Intel Corporation. All rights reserved.
+# Copyright (c) Microsoft Corporation.
+# Copyright (c) 2024, Red Hat. Inc
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION= 0x00010005
+  BASE_NAME  = VirtHstiDxe
+  FILE_GUID  = 60740CF3-D428-4500-80E6-04A5798241ED
+  MODULE_TYPE= DXE_DRIVER
+  VERSION_STRING = 1.0
+  ENTRY_POINT= VirtHstiDxeEntrypoint
+
+[Sources]
+  VirtHstiDxe.h
+  VirtHstiDxe.c
+  QemuPC.c
+  QemuQ35.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  UefiDriverEntryPoint
+  UefiLib
+  BaseLib
+  BaseMemoryLib
+  MemoryAllocationLib
+  DebugLib
+  HobLib
+  HstiLib
+  PcdLib
+  PciLib
+  UefiBootServicesTableLib
+
+[Guids]
+  gUefiOvmfPkgPlatformInfoGuid
+
+[FeaturePcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
+
+[Depex]
+  TRUE
diff --git a/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h 
b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
new file mode 100644
index ..26109bf322e9
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/VirtHstiDxe.h
@@ -0,0 +1,67 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#define VIRT_HSTI_SECURITY_FEATURE_SIZE  2
+
+#define VIRT_HSTI_BYTE0_Q35_SMM_SMRAM_LOCK  BIT0
+
+typedef struct {
+  // ADAPTER_INFO_PLATFORM_SECURITY
+  UINT32Version;
+  UINT32Role;
+  CHAR16ImplementationID[256];
+  UINT32SecurityFeaturesSize;
+  // bitfields
+  UINT8 SecurityFeaturesRequired[VIRT_HSTI_SECURITY_FEATURE_SIZE];
+  UINT8 SecurityFeaturesImplemented[VIRT_HSTI_SECURITY_FEATURE_SIZE];
+  UINT8 SecurityFeaturesVerified[VIRT_HSTI_SECURITY_FEATURE_SIZE];
+  CHAR16ErrorString[1];
+} VIRT_ADAPTER_INFO_PLATFORM_SECURITY;
+
+VOID
+VirtHstiSetSupported (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti,
+  IN UINT32ByteIndex,
+  IN UINT8 BitMask
+  );
+
+BOOLEAN
+VirtHstiIsSupported (
+  VIRT_ADAPTER_INFO_PLATFORM_SECURITY  *VirtHsti,
+  IN UINT32ByteIndex,
+  IN UINT8 BitMask
+  );
+
+VOID
+VirtHstiTestResult (
+  CHAR16 *ErrorMsg,
+  IN UINT32  ByteIndex,
+  IN UINT8   BitMask
+  );
+
+/* QemuQ35.c */
+
+VIRT_ADAPTER_INFO_PLATFORM_SECURITY *
+VirtHstiQemuQ35Init (
+  VOID
+  );
+
+VOID
+VirtHstiQemuQ35Verify (
+  VOID
+  );
+
+/* QemuPC.c */
+
+VIRT_ADAPTER_INFO_PLATFORM_SECURITY *
+VirtHstiQemuPCInit (
+  VOID
+  );
+
+VOID
+VirtHstiQemuPCVerify (
+  VOID
+  );
diff --git a/OvmfPkg/VirtHstiDxe/QemuPC.c b/OvmfPkg/VirtHstiDxe/QemuPC.c
new file mode 100644
index ..aa0459e8b6c6
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/QemuPC.c
@@ -0,0 +1,38 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#include "VirtHstiDxe.h"
+
+STATIC VIRT_ADAPTER_INFO_PLATFORM_SECURITY  mHstiPC = {
+  PLATFORM_SECURITY_VERSION_VNEXTCS,
+  PLATFORM_SECURITY_ROLE_PLATFORM_REFERENCE,
+  { L"OVMF (Qemu PC)" },
+  VIRT_HSTI_SECURITY_FEATURE_SIZE,
+};
+
+VIRT_ADAPTER_INFO_PLATFORM_SECURITY *
+VirtHstiQemuPCInit (
+  VOID
+  )
+{
+  return 
+}
+
+VOID
+VirtHstiQemuPCVerify (
+  VOID
+  )
+{
+}
diff --git a/OvmfPkg/VirtHstiDxe/QemuQ35.c b/OvmfPkg/VirtHstiDxe/QemuQ35.c
new file mode 100644
index ..75e9731b4a52
--- /dev/null
+++ b/OvmfPkg/VirtHstiDxe/QemuQ35.c
@@ -0,0 +1,58 @@
+/** @file
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 

Re: [edk2-devel] [PATCH v2 00/13] Part 2 patch set to add LoongArch support into UefiCpuPkg

[Gerd Hoffmann]

On Wed, Mar 20, 2024 at 04:41:52PM +0800, Chao Li wrote:
> This patch set adjusted some order in UefiCpuPig alphabetically, added
> LoongArch libraries and drivers into UefiCpuPkg, it is a continuation of
> the first patch series v8 submitted at
> https://edk2.groups.io/g/devel/message/114526.
> 
> And also separated from https://edk2.groups.io/g/devel/message/116583.
> 
> This series only contents the changes for UefiCpuPkg.
> 
> Patch1-Patch4: Reorder some INF files located in UefiCpuPkg
> alphabetically.
> 
> Patch5-Patch13: Added Timer, CpuMmuLib, CpuMmuInitLib, MpInitLib, CpuDxe
> for LoongArch, and added some PCD and header files requested by the
> above libraries and drivers.
> 
> Modfied modules: UefiCpuPkg
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4726
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4734
> 
> PR: https://github.com/tianocore/edk2/pull/5483
> 
> V1 -> V2:
> 1. Removed PcdCpuMmuIsEnabled.
> 2. Removed API GetMemoryRegionAttributes API as it is no longer needed.
> 3. Patch3, added two empty line in DXE and PEI INF files.
> 4. Patch5, added the Status check in GetTimeInnanoSecond function.
> 5. Separated into two series, this is series one, and the second one is
> OvmfPkg.

While I can't comment on the loongarch architecture details the code
and the integration into build system looks overall sane to me.

Series:
Acked-by: Gerd Hoffmann 

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117046): https://edk2.groups.io/g/devel/message/117046
Mute This Topic: https://groups.io/mt/105041080/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[Gerd Hoffmann]

On Fri, Mar 22, 2024 at 08:29:28AM +, Sun, CepingX wrote:
> On Thursday, March 21, 2024 8:25 PM Gerd Hoffmann wrote:
> > Well, just try to read them.  If present they can just be measured.
> > If not present we can either skip them, or measure with an empty data 
> > field to indicate it is not present.
> My understanding :
> If the fw_cfg is present,  it must be measured and consumed later.
> Is this correct?

I think that would be the best strategy.  In SEC or early PEI read the
items, cache them in HOBs, optionally measure them.

When consumed just fetch them from the cache.  For entries we are
measuring caching is mandatory (to make sure we are actually using
the same data we have measured).

> > But then you have to find and update all callsites (or at least the 
> > ones where we care about measurement).
> In your solution,  if we cache all items that need to be measured,
> we would have to add a new API (example: QemuFwCfgGetDataFromCache ())  to 
> get the data from cache.

No, we only need to update QemuFwCfgSelectItem + QemuFwCfgReadBytes to
support reading from the cache.

QemuFwCfgGetDataFromCache() can be added as additional API, and
callsites have the option to either switch over, or continue to
use the existing API.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117032): https://edk2.groups.io/g/devel/message/117032
Mute This Topic: https://groups.io/mt/104880546/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH] OvmfPkg/SecurityPkg: Add build option for coexistance of vTPM and RTMR.

[Gerd Hoffmann]

On Fri, Mar 22, 2024 at 02:39:20AM +, Yao, Jiewen wrote:
> Please aware that this option will cause potential security risk.
> 
> In case that any the guest component only knows one of vTPM or RTMR,
> and only extends one of vTPM or RTMR, but the other one only verifies
> the other, then the chain of trust is broken.  This solution is secure
> if and only if all guest components aware of coexistence, and can
> ensure all measurements are extended to both vTPM and RTMR.  But I am
> not sure if all guest components are ready today.

As far I know (it's been a while I looked at those patches) shim.efi and
grub.efi have support for EFI_CC_MEASUREMENT_PROTOCOL, but use the same
logic we have in DxeTpm2MeasureBootLib, i.e. they will not measure to
both RTMR and vTPM.

Looking at systemd-boot I see it will likewise not measure to both RTMR
and vTPM, but with reversed priority (use vTPM not RTMR in case both are
present).

Linux kernel appears to not have EFI_CC_MEASUREMENT_PROTOCOL support.

> Since this option caused a potential risk / misuse breaking the chain
> of trust, I recommend we have at least one more company to endorse the
> runtime co-existence of vTPM and RTMR.  Also, I would like to hear the
> opinions from other companies.

Rumors say intel is working on coconut-svsm support for tdx.  That will
most likely allow to use a vTPM with tdx even without depending on the
virtualization host or cloud hyperscaler providing one.  We will see VMs
with both RTMR and vTPM and surely need a strategy how guests should
deal with that situation, consistent across the whole boot stack and
not every component doing something different.

Given that the vTPM might be provided by the hypervisor and thus not be
part of the TCB I can see that guests might want use both vTPM and RTMR.
So, yes, for that case coexistance makes sense.  I'm not convinced it is
a good idea to make that a compile time option though.  That will not
help to promote a consistent story ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117029): https://edk2.groups.io/g/devel/message/117029
Mute This Topic: https://groups.io/mt/105070442/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[Gerd Hoffmann]

On Thu, Mar 21, 2024 at 08:39:13AM +, sunceping wrote:
> On Wednesday, March 20, 2024 6:05 PM Gerd Hoffmann wrote:
> > 
> > We don't need to read + cache all fw_cfg data.  We only need to cache the
> > entries which (a) must be measured, and (b) will not be measured in some
> > other way.
> > 
> I am afraid that it is difficult to determine which fw_cfg items 
> are need to be cached and measured, since there are some fw_cfg items are
> optional with special qemu cmd.
> Example : 
> fw_cfg item: opt/ovmf/X-PciMmio64Mb  
> depends on qemu command: "-fw_cfg name=opt/ovmf/X-PciMmio64Mb,string=N"

Well, just try to read them.  If present they can just be measured.
If not present we can either skip them, or measure with an empty data
field to indicate it is not present.

> > > Further more this is not a lazy mode which means some fw_cfg data may
> > > be read but it is not to be consumed later.
> > 
> > The problem with lazy mode is that the measurement order is not fixed.
> We'd better define what's the "fixed measurement order".
> Think about such scenario:
> There are 5 fw_cfg items ("A-B-C-D-E"), and C is an optional one which 
> depends on special qemu cmd
>  (example : etc/boot-menu-wait depends on qemu command: " -boot 
> menu=on,splash-time=N").
>   
> So, there are two measurement orders in different qemu command:
> 1, "A-B-C-D-E" (with qemu command "-boot menu=on,splash-time=N ")
> 2, "A-B-D-E" (without qemu command "-boot menu=on,splash-time=N ") 
> 
> Is the "A-B-C-D-E" or "A-B-D-E"  fixed order ? 
> 
> What's your thoughts ?

See above for optional items.

I'm more concerned about reordering.  We have a number of DXE modules
reading fw_cfg entries.  The order the modules are scheduled depends
multiple factors (order in the firmware volume, depex), so that order
may change.

With lazy loading and measurement changes in the initialization order
would also change the measurements even if the content of the fw_cfg
items files is identical, because the items are measured in B-A instead
of A-B order.

> > > We propose below solution :
> > >
> > > Add a new API in QemuFwCfgLib,
> > > RETURN_STATUS QemuFwCfgGetData(fw_cfg_name, *size, *value,
> > FW_CFG_GET_DATA_FLAG flag).
> > 
> > I'd suggest to *not* touch the existing interfaces for reading entries.
> > Instead change the existing functions to first check the cache, in case 
> > there is
> > no cache entry go read from fw_cfg.
> Actually we don't touch the existing interface for reading entries.
> The API is newly added.

But then you have to find and update all callsites (or at least the
ones where we care about measurement).

> > Add a new interface for adding fw_cfg entries to the cache, with optional
> > measurement.
> Do you mean to add a new API (example : QemuFwCfgReadBytes2(*size, *buffer, 
> Flag))
>  with optional measurement to cache fw_cfg data?

More like this:

QemuFwCfgCacheAdd(int Item, int Size, bool Measure);

> We can update it like below:
>   QemuFwCfgGetData("etc/e820", ,  Buffer, Flag);
>   for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { 
> EFI_E820_ENTRY64 *E820Entry = (EFI_E820_ENTRY64 *)(Buffer + Processed);
> Callback (E820Entry, PlatformInfoHob);
>   }

Well, not that easy.  Allocating memory is not always possible in early
firmware phases, which is the reason why the code works with the stack
and chunkwise reads.

We might offer an QemuFwCfgCacheGet() API though.  When storing cached
items in HOBs we can hand out a pointer to the data in the HOB, and
callers access the complete fw_cfg item then without having to allocate
memory.

> > Also note that not all fw_cfg entries are (pseudo-)files.
> I am not sure what you mean about (pseudo-)files.

Well, even though fw_cfg uses names which look like file paths for
entries it's not a real filesystem, it's just a naming convention,
that's why named them pseudo files.

Some older fw_cfg items don't use this file-naming scheme, they have a
fixed number instead (see QemuFwCfgItem*).

> Base on your comments,  does this mean there are other fw_cfg data 
> which are read from QEMU via different method (not 
> QemuFwCfgFindFile/QemuFwCfgSelectItem/QemuFwCfgReadBytes) ?

It all comes down to QemuFwCfgSelectItem + QemuFwCfgReadBytes.  There
are some convenience functions to read (for example) integers, they
internally call QemuFwCfgReadBytes though.  QemuFwCfgFindFile also
simply calls QemuFwCfgSelectItem + QemuFwCfgReadBytes.

So making QemuFwCfgSelectItem + QemuFwCfgReadBytes aware of the cache
should be all you need.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116959): https://edk2.groups.io/g/devel/message/116959
Mute This Topic: https://groups.io/mt/104880546/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 21/26] OvmfPkg/LoongArchVirt: Add FdtQemuFwCfgLib

[Gerd Hoffmann]

  Hi,

> QemuFwCfgLibMmio.inf is looks like a DXE stage library, while this patch is
> the PEI stage library we are dicussing.
> 
> I have tow plans:
> 
> *Plan A:* Keep this library under LoongArchQemuVirt.
> 
> *Plan  B:* Create a new INF named QemuFwCfgPeiLibMmio.inf under
> OvmfPkg/Library/QemuFwCfgLib/, which will obtain the resources from FDT, and
> store them in the HOB or dynamic PCD.
> 
> Which one do you like? I'm leaning toward B because more people will be
> served if it's under OvmfPkg/Library.

Yes, Plan (b) is better.  Also try avoid code duplication.  The existing
code can be splitted into two files.  Move the code which works in DXE
only (i.e. the bits using FdtClientProtocol to find the fw_cfg mmio
address, maybe more) to QemuFwCfgLibMmioDxe.c, keep the code which can
work for both PEI and DXE in QemuFwCfgLibMmio.c.  Add
QemuFwCfgLibMmioPei.c for the PEI-specific code.

The ioport version of the library uses the same approach with
QemuFwCfgLib.c + QemuFwCfgDxe.c + QemuFwCfgPei.c

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116958): https://edk2.groups.io/g/devel/message/116958
Mute This Topic: https://groups.io/mt/104859897/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 21/26] OvmfPkg/LoongArchVirt: Add FdtQemuFwCfgLib

[Gerd Hoffmann]

On Mon, Mar 18, 2024 at 04:28:17PM +0100, Gerd Hoffmann wrote:
> On Sat, Mar 16, 2024 at 10:17:00AM +0800, lixianglai wrote:
> > Hi Gerd:
> > > On Mon, Mar 11, 2024 at 02:39:31AM -0700, Chao Li wrote:
> > >> This library for PEI phase, and obtains the QemuFwCfg base address by
> > >> directly parsing the FDT, reads and writes the data in QemuFwCfg by
> > >> operating on the QemuFwCfg base address.
> > >>  create mode 100644 
> > >> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/FdtQemuFwCfgPeiLib.c
> > >>  create mode 100644 
> > >> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/FdtQemuFwCfgPeiLib.inf
> > >>  create mode 100644 
> > >> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/QemuFwCfgLibInternal.h
> > >>  create mode 100644 
> > >> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/QemuFwCfgPei.c
> > > Is there anything LoongArch-specific in there?
> > No,The main function of this lib library is to obtain the fwcfg base 
> > address by parsing fdt in the pei stage,
> >  and provide access to fwcfg through mmio mode,
> >  the difference between it and the existing library is that the fwcfg base 
> > address is not hard-coded in the compilation stage,
> >  and is accessed through mmio rather than io port.
> 
> That would be the case for risc-v and aarch64 too, although I think they
> don't need fw_cfg right now (they get all info needed via fdt).

Oops, I was wrong, we have OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.inf

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116933): https://edk2.groups.io/g/devel/message/116933
Mute This Topic: https://groups.io/mt/104859897/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/QemuBootOrderLib: Measure the etc/boot-menu-wait

[Gerd Hoffmann]

On Wed, Mar 20, 2024 at 08:41:41AM +, Sun, CepingX wrote:
> 
> On Thursday, March 14, 2024 5:31 PM Gerd Hoffmann wrote:
> > Load, measure and cache all fw_cfg entries we care about early in the PEI 
> > phase
> > (or SEC phase for pei-less builds), so we can
> >  (a) easily have a fixed order, and
> >  (b) store them all in HOBs?
> > 
> > Which implies SEC/PEI must read all relevant fw_cfg entries, even in case 
> > they
> > are used only later in the DXE phase.
> > 
> > Advantage is we have a single cache which can be used in all firmware 
> > phases.
> > When using global variables in DXE we still can end up reading entries 
> > multiple
> > times, either because entries are needed by both PEI and DXE, or because
> > multiple DXE modules need them (global variables are per module).
> > 
> We have some concerns that in above solution the size of a single cache is 
> large
> ( because all fw_cfg data need to be read from qemu). 

We don't need to read + cache all fw_cfg data.  We only need to cache
the entries which (a) must be measured, and (b) will not be measured in
some other way.

The big entries I'm aware of are:
 (1) kernel for direct boot (no fw_cfg measurement needed, binary
 will be measured before launch).
 (2) ACPI tables (no fw_cfg measurement needed, acpi tables are
 measured when installed).
 (3) smbios tables (to be decided, could be handled similar to ACPI).

Anything else where you have size concerns?

> Further more this is not a lazy mode which means some fw_cfg data 
> may be read but it is not to be consumed later. 

The problem with lazy mode is that the measurement order is not fixed.

> We propose below solution :
> 
> Add a new API in QemuFwCfgLib,
> RETURN_STATUS QemuFwCfgGetData(fw_cfg_name, *size, *value, 
> FW_CFG_GET_DATA_FLAG flag).

I'd suggest to *not* touch the existing interfaces for reading entries.
Instead change the existing functions to first check the cache, in case
there is no cache entry go read from fw_cfg.

Add a new interface for adding fw_cfg entries to the cache, with
optional measurement.

Populate the cache with all entries which need fw_cfg measurement.

Additionally cache entries which are used frequently
(QemuFwCfgItemSignature + QemuFwCfgItemInterfaceVersion +
QemuFwCfgItemFileDir are candidates).

Note that QemuFwCfgItemFileDir must be cached for security reasons (so
the hypervisor can't present different versions of the file list) even
though I don't think we need to measure it.

> Pros:
> Caller can decide how to get the fw_cfg data from QEMU (cache? Measurement?)

Cache and measurement are not independent options.  Measurement without
caching doesn't make sense.

>  And it can reduce the code complexity because it pack 3 fw_cfg call 
> (QemuFwCfgFindFile/QemuFwCfgSelectItem/QemuFwCfgReadBytes) into one call. 

There are a few places in the code which expect they can read fw_cfg
files in multiple chunks (i.e. call QemuFwCfgReadBytes multiple times).

Also note that not all fw_cfg entries are (pseudo-)files.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116932): https://edk2.groups.io/g/devel/message/116932
Mute This Topic: https://groups.io/mt/104880546/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 20/26] OvmfPkg/LoongArchVirt: Add NorFlashQemuLib

[Gerd Hoffmann]

On Tue, Mar 19, 2024 at 05:10:39PM +0800, Chao Li wrote:
> He Gerd,
> 
> > Speaking of this series: maybe split it into two?  The first part
> > of this series with the Mde*Pkg + UefiPkg changes looks almost ready
> > to merge to me, so maybe we can get that in while still sorting out
> > the remaining issues in the OvmfPkg patches ...
> 
> This series does not include changes to Mde*Pkg , so if the patches are
> separated,

Oh, right, Mde*Pkg was splitted before and is merged already.

> I think it might be two series, one is the UefiCpuPkg series, and
> other is OvmfPkg.

Agree.

> I tend to split patches into two series.



take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116889): https://edk2.groups.io/g/devel/message/116889
Mute This Topic: https://groups.io/mt/104859896/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 20/26] OvmfPkg/LoongArchVirt: Add NorFlashQemuLib

[Gerd Hoffmann]

  Hi,

> > > I can't tell the implementation scheme of the current lib and existing
> > > lib implementation scheme which one is better, Could you give we some
> > > advice?
> > I'd suggest to merge your code as OvmfPkg/Library/FdtNorFlashQemuLib as
> > it is not really loongarch-specific.
> > 
> > If you want try switch aarch64 to use the same code that'll be great,
> > but sorting that out later is also fine with me.
> 
> If you think this design is looks better, then I'm prepare to commit this
> change under the OvmfPkg/Library as a public library. And I will enable it
> in aarch64 after merging this change, because I think it may be tweaked and
> validated in aarch64 for many platforms. Do you think that is good?

The VirtNorFlashDxe is optimized for qemu-emulated pflash.  It tries to
avoid switching between read and write mode much, because that operation
has a significant overhead in virtualization.  So it's really only used
by ArmVirtPkg and not lots of other arm platforms.

Doing it separate from this patch series makes sense nevertheless.

Speaking of this series: maybe split it into two?  The first part
of this series with the Mde*Pkg + UefiPkg changes looks almost ready
to merge to me, so maybe we can get that in while still sorting out
the remaining issues in the OvmfPkg patches ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116879): https://edk2.groups.io/g/devel/message/116879
Mute This Topic: https://groups.io/mt/104859896/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 21/26] OvmfPkg/LoongArchVirt: Add FdtQemuFwCfgLib

[Gerd Hoffmann]

On Sat, Mar 16, 2024 at 10:17:00AM +0800, lixianglai wrote:
> Hi Gerd:
> > On Mon, Mar 11, 2024 at 02:39:31AM -0700, Chao Li wrote:
> >> This library for PEI phase, and obtains the QemuFwCfg base address by
> >> directly parsing the FDT, reads and writes the data in QemuFwCfg by
> >> operating on the QemuFwCfg base address.
> >>  create mode 100644 
> >> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/FdtQemuFwCfgPeiLib.c
> >>  create mode 100644 
> >> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/FdtQemuFwCfgPeiLib.inf
> >>  create mode 100644 
> >> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/QemuFwCfgLibInternal.h
> >>  create mode 100644 
> >> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/QemuFwCfgPei.c
> > Is there anything LoongArch-specific in there?
> No,The main function of this lib library is to obtain the fwcfg base address 
> by parsing fdt in the pei stage,
>  and provide access to fwcfg through mmio mode,
>  the difference between it and the existing library is that the fwcfg base 
> address is not hard-coded in the compilation stage,
>  and is accessed through mmio rather than io port.

That would be the case for risc-v and aarch64 too, although I think they
don't need fw_cfg right now (they get all info needed via fdt).

I think we should add this as OvmfPkg/Library/FdtQemuFwCfgLib.

> Another point that needs to be explained is that because loongarch virtual 
> machine runs on flash in pei phase,
> it cannot assign the pcd global variable, so we use Hob as the global 
> variable to store the fwcfg base address.

I think the dynamic PCD database is stored in a HOB and you should be
able to set PCDs them even when running from (read-only) flash.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116843): https://edk2.groups.io/g/devel/message/116843
Mute This Topic: https://groups.io/mt/104859897/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 20/26] OvmfPkg/LoongArchVirt: Add NorFlashQemuLib

[Gerd Hoffmann]

On Sat, Mar 16, 2024 at 06:19:00PM +0800, lixianglai wrote:
> Hi Gerd:
> > On Mon, Mar 11, 2024 at 02:39:24AM -0700, Chao Li wrote:
> >> Add NorFlashQemuLib for LoongArch, it is referenced from ArmVirtPkg.
> > What are the differences to the ArmVirtPkg version?
> In this lib we have assigned the following three pcd variables:
> PcdFlashNvStorageVariableBase
> PcdFlashNvStorageFtwWorkingBase
> PcdFlashNvStorageFtwSpareBase
> Instead of hardcoding these three variables in the VarStore.fdf.inc file as 
> arm does,
> the benefit is that when the flash base address changes in the qemu 
> implementation,
> there is no need to re-adapt and compile UEFI.

The flash memory layout (address + size) for the aarch64 virt machine
has never changed.  So while it sounds nice in theory to have that
option it could very well be that this will never ever needed in
practice.

Having sayed that I'd also note that I think it should also be possible
to switch the aarch64 builds to set the PCDs at runtime instead of
compile time.

> When I tried to implement the current patch scheme on aarch64,
> I found that the FaultTolerantWriteDxe driver loaded earlier than 
> VirtNorFlashDxe.
> And It requires the PcdFlashNvStorageFtwWorkingSize and 
> PcdFlashNvStorageFtwSpareSize variables for initialization,
> However the initialization of these two variables is completed in 
> VirtNorFlashDxe,
> The fdf file specifies that VirtNorFlashDxe is loaded first and then 
> FaultTolerantWriteDxe is loaded in loongarch64.
> So this is going to be a problem if we want to apply the current solution to 
> aarch64 or risc-v.

There is a non-obvious twist:

VirtNorFlashDxe registers the gEdkiiNvVarStoreFormattedGuid protocol.

There is the
EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
library, which only purpose is to add a dependency to
gEdkiiNvVarStoreFormattedGuid to depex.

NvVarStoreFormattedLib.inf is used this way ...

  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
A
  [ ... ]
  NULL|EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
  [ ... ]
  }

... to make sure VariableRuntimeDxe is scheduled after VirtNorFlashDxe.

I think you can apply the same idea to FaultTolerantWriteDxe.

> I can't tell the implementation scheme of the current lib and existing
> lib implementation scheme which one is better, Could you give we some
> advice?

I'd suggest to merge your code as OvmfPkg/Library/FdtNorFlashQemuLib as
it is not really loongarch-specific.

If you want try switch aarch64 to use the same code that'll be great,
but sorting that out later is also fine with me.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116842): https://edk2.groups.io/g/devel/message/116842
Mute This Topic: https://groups.io/mt/104859896/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] OvmfPkg: Add VirtHstiDxe driver

[Gerd Hoffmann]

On Thu, Mar 14, 2024 at 12:05:28PM +, Yao, Jiewen wrote:
> I agree that not all bits make sense to virtual machine.
> However, I do see some bits should be there if we really want to add HSTI to 
> report security propery.

Setting the bits which are obviously correct makes sense indeed.

> Please take a look at the HSTI spec - 
> https://learn.microsoft.com/en-us/windows-hardware/test/hlk/testref/hardware-security-testability-specification
> For example:
> Do you use RSA 2048 and SHA256 only (or higher but not lower than this)

Hmm.  That single line (and the spec doesn't have more) is not very
helpful.  Consider this corner case:

The virtual TPM supported by qemu has banks for sha1, sha256, sha384 and
sha512.  The default configuration created by libvirt enables only the
sha256 bank.  But it's possible to go into the firmware setup and turn
on the sha1 bank too.

How should the HSTI driver handle that?

> Compatibility Support Modules (CSM)

That one is easy, CSM support is gone, we can set it.

> Firmware Code must be present in protected storage

Typically this is the case (ROM or read-only flash), although qemu
does not enforce that the code flash is actually read-only, it can
be configured in writable mode.

Hmm.

> Secure firmware update process

IMHO doesn't apply to virtual machines.  Firmware updates are usually
handled by updating the images on the host machine, that is very
different from a physical machine.  All the questions about key handling
do not make any sense.

> Do you have backdoors to override SecureBoot

No (you can only turn it off altogether).  I think we can set this (in
secure boot enabled builds).

Use "FeaturePcdGet (PcdSecureBootSupported)" to figure whenever a given
build supports secure boot or not.

> Protection from internal and external DMA

I don't think qemu supports DMA access to NV (aka flash) storage.
Is that good enough to set that bit?

> Another question: I notice you report platform as “Intel(R) 9-Series v1”.
> Is that right configuration for current OVMF?

Probably refers to q35 (aka INTEL_Q35_MCH_DEVICE_ID).

> I think there is some configuration detection, such as 
> https://github.com/tianocore/edk2/blob/master/OvmfPkg/PlatformPei/Platform.c.

Looking at PlatformInfoHob->HostBridgeDevId and setting the name
accordingly makes sense indeed.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116813): https://edk2.groups.io/g/devel/message/116813
Mute This Topic: https://groups.io/mt/104923813/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 18/26] OvmfPkg/LoongArchVirt: Add the early serial port output library

[Gerd Hoffmann]

On Fri, Mar 15, 2024 at 06:19:12PM +0800, Chao Li wrote:
> Hi Gerd,
> 
> 
> Thanks,
> Chao
> On 2024/3/15 17:36, Gerd Hoffmann wrote:
> > On Mon, Mar 11, 2024 at 05:39:08PM +0800, Chao Li wrote:
> > > Add a early serial port output library into LoongArchVirt that named
> > > EarlyFdtSerialPortLib16550, this library is referenced from
> > > MdeModulePkg.
> > Why create your own copy?  What are the differences to
> > MdeModulePkg/Library/BaseSerialPortLib16550?
> This library is used in the PEI stage, same as the Hook Library, LoongArch
> QEMU is runs on flash in PEI stage, MdeModule version does not support to
> get the SerialRegisterBase via registers or HOB or FDT,

It uses PcdSerialRegisterBase.

Having a quick look at the PCD code it looks like the PCD are stored in
in a HOB (gPcdDataBaseHobGuid), so setting dynamic PCDs should work even
when running directly from flash.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116807): https://edk2.groups.io/g/devel/message/116807
Mute This Topic: https://groups.io/mt/104859894/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 09/26] UefiCpuPkg: Added a new PCD named PcdCpuMmuIsEnabled

[Gerd Hoffmann]

  Hi,

> > > +  ## This dynamic PCD indicates whether CPU MMU is enabled.
> > > +  #  TRUE  - CPU MMU is enabled.
> > > +  #  FASLE - CPU MMU have not enabled.
> > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMmuIsEnabled|FALSE|BOOLEAN|0x6023
> > Why create a PCD for this?  Can't you simply read the control register
> > to figure whenever paging is enabled or not?

> This is a good question. When I designed the API of
> SetMemoryRegionAttributes, it only contained 4 parameters, namely Start,
> Size, Attribut and Mask(refer to ARM version), and the MmuInitLib needs call
> this API to configure or initialize the MMU. SetMemoryRegionAttributes will
> call a private function called UpdateRegionMapping, it needs to know if the
> MMU is started to do different logic(such as wether should be invalidated
> TLB when filling the tables for the first time, etc), therefore, there
> create a new PCD to let it know this. BTW, ARM and RISC-V have the same
> logic.

Yes, UpdateRegionMapping needs to know this, no doubt.  But do we need a
PCD for this?  Looks like a pointless indirection to me.

Arm has a ArmMmuEnabled() functions which goes read the control
registers directly.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116801): https://edk2.groups.io/g/devel/message/116801
Mute This Topic: https://groups.io/mt/104859880/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 21/26] OvmfPkg/LoongArchVirt: Add FdtQemuFwCfgLib

[Gerd Hoffmann]

On Mon, Mar 11, 2024 at 02:39:31AM -0700, Chao Li wrote:
> This library for PEI phase, and obtains the QemuFwCfg base address by
> directly parsing the FDT, reads and writes the data in QemuFwCfg by
> operating on the QemuFwCfg base address.

>  create mode 100644 
> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/FdtQemuFwCfgPeiLib.c
>  create mode 100644 
> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/FdtQemuFwCfgPeiLib.inf
>  create mode 100644 
> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/QemuFwCfgLibInternal.h
>  create mode 100644 
> OvmfPkg/LoongArchVirt/Library/FdtQemuFwCfgLib/QemuFwCfgPei.c

Is there anything LoongArch-specific in there?

If not this can go to OvmfPkg/Library (and possibly used by arm and
risc-v too).

> +GuidHob  = GetFirstGuidHob ();

This hob is not mentioned in the commit message.  Please describe the
changes in more detail.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116798): https://edk2.groups.io/g/devel/message/116798
Mute This Topic: https://groups.io/mt/104859897/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 20/26] OvmfPkg/LoongArchVirt: Add NorFlashQemuLib

[Gerd Hoffmann]

On Mon, Mar 11, 2024 at 02:39:24AM -0700, Chao Li wrote:
> Add NorFlashQemuLib for LoongArch, it is referenced from ArmVirtPkg.

What are the differences to the ArmVirtPkg version?

Is it possible to have a FdtNorFlashQemuLib which is shared
between arm and loongarch?  And maybe risc-v too?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116795): https://edk2.groups.io/g/devel/message/116795
Mute This Topic: https://groups.io/mt/104859896/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 18/26] OvmfPkg/LoongArchVirt: Add the early serial port output library

[Gerd Hoffmann]

On Mon, Mar 11, 2024 at 05:39:08PM +0800, Chao Li wrote:
> Add a early serial port output library into LoongArchVirt that named
> EarlyFdtSerialPortLib16550, this library is referenced from
> MdeModulePkg.

Why create your own copy?  What are the differences to 
MdeModulePkg/Library/BaseSerialPortLib16550?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116794): https://edk2.groups.io/g/devel/message/116794
Mute This Topic: https://groups.io/mt/104859894/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 17/26] OvmfPkg/LoongArchVirt: Add serial port hook library

[Gerd Hoffmann]

On Mon, Mar 11, 2024 at 05:39:02PM +0800, Chao Li wrote:
> Add a serial port hook library in LoongArchVirt named
> Fdt16550SerialProtHookLib, this library is referenced from ArmVirtPkg.
> 
> LoongArch QEMU virtual machine uses register of LOONGARCH_CSR_KS1 to
> transfer serial port base addres from the PEI phase to the DXE phase.

Why use LOONGARCH_CSR_KS1?  Can't you simply set PcdSerialRegisterBase?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116792): https://edk2.groups.io/g/devel/message/116792
Mute This Topic: https://groups.io/mt/104859892/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-