[staging/branch]: CdePkg - C Development Environment Package

[staging/branch]: CdePkg - C Development Environment Package

[KILIAN_KEGEL]

[-- Attachment #1: Type: text/plain, Size: 4853 bytes --]

Hi UEFI community,

I’d like to introduce the CdePkg to edk2-staging.


Some time ago I decided to write my own ANSI C Library for UEFI Shell and POST.



The UEFI Shell library (“Torito C Library”) has been production-ready for more than one year.

The POST version of the library (“CdeLib”) is not yet fully tested.



I will be demonstrating my verification procedure in the upcoming weeks on EDK2 STAGING  https://github.com/tianocore/edk2-staging/tree/CdePkg



Currently there are 3 examples implemented:

  1.  argvc: https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/HOSTED_ENV/argcv/main.c#L57

argc/argv handling according to https://msdn.microsoft.com/en-us/library/a1y7w461.aspx

  1.  systeminterfacePEI: https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/SYSTEM_IF/systeminterfacePEI/main.c#L57

demonstration, how PeiServices and FileHandle are passed into main()

  1.  systeminterfaceDXE: https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/SYSTEM_IF/systeminterfaceDXE/main.c#L57

demonstration, how SystemTable and ImageHandle are passed into main()



Upcoming next demonstration will be the clock() function end of this week



The idea is to bring the ANSI C Library interface into POST drivers.

This will:

  1.  ease porting tasks
  2.  allow cross development
  3.  allow developers to focus on their aims, because they aren’t forced to keep in mind a lot of additional info (e.g. RShiftU64)
  4.  provide all intrisics to allow the compiler to be a “C compiler”

(e.g. char buffer[256] = { 1 };)



What is CdePkg and Torito C Library?

  *   CdePkg and Torito C Library are a one man show / after work party, that is owned and written solely by myself
  *   CdePkg is a reference implementation only for Microsoft C compiler
  *   CdePkg is a feasibility study
  *   CdePkg is the successor of Torito C, based on the same source code
  *   CdePkg C Development Environment is similar to  MdePkg Module Development Environment

but guarantees  that the C compiler is always fully usable (all intrinsics available) and the C90/C95 standard library is always available



What are the design goals?

  *   to rewrite the whole thing from scratch, without using any public source code from GNU, BSD, Watcom
  *   completeness: full blown C90 + C95 support, as lowest common denominator
  *   tailored for UEFI: small code size, for UEFI-POST-driver uses a C-Library-Driver, that contains core/worker functions for realloc() ==  malloc() and free(),

entire printf()-family, entire scanf()-family.

UEFI-POST-driver just uses small wrapper functions to run the C-Library-Driver code.

  *   stable, exact, chipset independent TSC based clock() with CLOCKS_PER_SEC == 1000
  *   complete set of the Microsoft C-compiler intrinsic functions
  *   ROM-able! Runs with stack but w/o any static storage duration in .data segment, e.g. for rand(), strtok(), tmpfile()

This is required for early PEI before memory sizing, when PEI-images run directly out of flash

  *   Microsoft (bug) compatible (as far as possible)

     *   use original Microsoft header files for UEFI Shell Apps created in VS2019
     *   allow expensive debugging tasks of ANSI C .EFI applications in Visual Studio in its Windows NT counter part
     *   to save my lifetime writing a documentation https://github.com/tianocore/edk2-staging/tree/CdePkg/implemented.md#validation-status

  *   all the above in one single C-Library CdeLib.lib


CdePkg shall be adjusted to other compilers/tool chains too, once it is feature-complete and accepted by the UEFI community.
As long as it is for Microsoft VS2019 only.

CdePkg README.md is here:  <https://github.com/MinnowWare/CdePkg#cdepkg>  https://github.com/tianocore/edk2-staging/tree/CdePkg#cdepkg
CdePkg HOWTO is here: https://github.com/tianocore/edk2-staging/blob/CdePkg/README.md#howto
CdeValidationPkg README.md is here: https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/README.md<https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/README.md#cdevalidationpkg>
CdeValidationPkg HOWTO is here: https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/README.md<https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/README.md#howto>


HOWTO:

  1.  clone the edk2-staging repository
  2.  checkout CdePkg
  3.  run LAUNCH.BAT
  4.  run build -p EmulatorPkg\EmulatorPkg.dsc -t VS2015x86 -a IA32
  5.  run DBGEMU.BAT to start emulation (EmulatorPkg)
  6.  run build -a IA32 -a X64 -n 5 -t VS2015x86 -b DEBUG -p Vlv2TbltDevicePkg\PlatformPkgX64.dsc
  7.  update MinnowBoard with Build/Vlv2TbltDevicePkgX64\DEBUG_VS2015x86\FV\VLV.fd



Best regards,

Kilian Kegel



[-- Attachment #2: Type: text/html, Size: 24283 bytes --]

Re: [staging/branch]: CdePkg - C Development Environment Package

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 6029 bytes --]

Hi Kilian,

Thanks for the contribution.  I have a couple quick comments.


  *   Some files do not have a file header at all.  Please update these with a proper file header
  *   Some files have a BSD 2-Clause license.  The preferred license for EDK II is BSD+Patent and we prefer SPDX identifiers.  Will you be able to update to this license and header format?
  *   There are binaries (.lib) files in this branch.  Source repos should not have binaries.  The preferred locations for binaries are GitHub release pages for binaries build from a source tag or the edk2-non-osi repo.
  *   Can you please summarize the differences between the copies of the EmulatorPkg and Vlv2TbltDevicePkg in your branch and the versions of those packages in the edk2 and edk2-platforms repos?  Do you need help from those package maintainers to resolve the differences?

Thanks,

Mike


From: Kilian Kegel <kilian_kegel@outlook.com>
Sent: Wednesday, October 23, 2019 1:02 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Richardson, Brian <brian.richardson@intel.com>
Subject: [staging/branch]: CdePkg - C Development Environment Package

Hi UEFI community,

I'd like to introduce the CdePkg to edk2-staging.


Some time ago I decided to write my own ANSI C Library for UEFI Shell and POST.



The UEFI Shell library ("Torito C Library") has been production-ready for more than one year.

The POST version of the library ("CdeLib") is not yet fully tested.



I will be demonstrating my verification procedure in the upcoming weeks on EDK2 STAGING  https://github.com/tianocore/edk2-staging/tree/CdePkg



Currently there are 3 examples implemented:

  1.  argvc: https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/HOSTED_ENV/argcv/main.c#L57

argc/argv handling according to https://msdn.microsoft.com/en-us/library/a1y7w461.aspx

  1.  systeminterfacePEI: https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/SYSTEM_IF/systeminterfacePEI/main.c#L57

demonstration, how PeiServices and FileHandle are passed into main()

  1.  systeminterfaceDXE: https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/SYSTEM_IF/systeminterfaceDXE/main.c#L57

demonstration, how SystemTable and ImageHandle are passed into main()



Upcoming next demonstration will be the clock() function end of this week



The idea is to bring the ANSI C Library interface into POST drivers.

This will:

  1.  ease porting tasks
  2.  allow cross development
  3.  allow developers to focus on their aims, because they aren't forced to keep in mind a lot of additional info (e.g. RShiftU64)
  4.  provide all intrisics to allow the compiler to be a "C compiler"

(e.g. char buffer[256] = { 1 };)



What is CdePkg and Torito C Library?

  *   CdePkg and Torito C Library are a one man show / after work party, that is owned and written solely by myself
  *   CdePkg is a reference implementation only for Microsoft C compiler
  *   CdePkg is a feasibility study
  *   CdePkg is the successor of Torito C, based on the same source code
  *   CdePkg C Development Environment is similar to  MdePkg Module Development Environment

but guarantees  that the C compiler is always fully usable (all intrinsics available) and the C90/C95 standard library is always available



What are the design goals?

  *   to rewrite the whole thing from scratch, without using any public source code from GNU, BSD, Watcom
  *   completeness: full blown C90 + C95 support, as lowest common denominator
  *   tailored for UEFI: small code size, for UEFI-POST-driver uses a C-Library-Driver, that contains core/worker functions for realloc() ==  malloc() and free(),

entire printf()-family, entire scanf()-family.

UEFI-POST-driver just uses small wrapper functions to run the C-Library-Driver code.

  *   stable, exact, chipset independent TSC based clock() with CLOCKS_PER_SEC == 1000
  *   complete set of the Microsoft C-compiler intrinsic functions
  *   ROM-able! Runs with stack but w/o any static storage duration in .data segment, e.g. for rand(), strtok(), tmpfile()

This is required for early PEI before memory sizing, when PEI-images run directly out of flash

  *   Microsoft (bug) compatible (as far as possible)

     *   use original Microsoft header files for UEFI Shell Apps created in VS2019
     *   allow expensive debugging tasks of ANSI C .EFI applications in Visual Studio in its Windows NT counter part
     *   to save my lifetime writing a documentation https://github.com/tianocore/edk2-staging/tree/CdePkg/implemented.md#validation-status

  *   all the above in one single C-Library CdeLib.lib


CdePkg shall be adjusted to other compilers/tool chains too, once it is feature-complete and accepted by the UEFI community.
As long as it is for Microsoft VS2019 only.

CdePkg README.md is here:  <https://github.com/MinnowWare/CdePkg#cdepkg>  https://github.com/tianocore/edk2-staging/tree/CdePkg#cdepkg
CdePkg HOWTO is here: https://github.com/tianocore/edk2-staging/blob/CdePkg/README.md#howto
CdeValidationPkg README.md is here: https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/README.md<https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/README.md#cdevalidationpkg>
CdeValidationPkg HOWTO is here: https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/README.md<https://github.com/tianocore/edk2-staging/blob/CdePkg/CdeValidationPkg/README.md#howto>


HOWTO:

  1.  clone the edk2-staging repository
  2.  checkout CdePkg
  3.  run LAUNCH.BAT
  4.  run build -p EmulatorPkg\EmulatorPkg.dsc -t VS2015x86 -a IA32
  5.  run DBGEMU.BAT to start emulation (EmulatorPkg)
  6.  run build -a IA32 -a X64 -n 5 -t VS2015x86 -b DEBUG -p Vlv2TbltDevicePkg\PlatformPkgX64.dsc
  7.  update MinnowBoard with Build/Vlv2TbltDevicePkgX64\DEBUG_VS2015x86\FV\VLV.fd



Best regards,

Kilian Kegel



[-- Attachment #2: Type: text/html, Size: 70992 bytes --]

Re: [Qemu-devel] [edk2-rfc] [edk2-devel] CPU hotplug using SMM with QEMU+OVMF

[Laszlo Ersek]

On 09/04/19 11:52, Igor Mammedov wrote:

>  it could be stolen RAM + black hole like TSEG, assuming fw can live without RAM(0x30000+128K) range
>   (in this case fwcfg interface would only work for locking down the range)
> 
>  or
> 
>  we can actually have a dedicated SMRAM (like in my earlier RFC),
>  in this case FW can use RAM(0x30000+128K) when SMRAM isn't mapped into RAM address space
>  (in this case fwcfg would be used to temporarily map SMRAM into normal RAM and unmap/lock
>   after SMI relocation handler was initialized).
> 
> If possible I'd prefer a simpler TSEG like variant.

I think TSEG-like behavior is between these two. That is, I believe we
should have explicit open/close/lock operations. And, when the range is
closed (meaning, closed+unlocked, or closed+locked), then the black hole
should take effect for code that's not running in SMM.

Put differently, its like the second choice, except the range never
appears as normal RAM. "When SMRAM isn't mapped into RAM address space",
then the address range shows "nothing" (black hole).


Regarding "fw can live without RAM(0x30000+128K) range" -- do you mean
whether the firmware could use another RAM area for fw_cfg DMA?

If that's the question, then I wouldn't worry about it. I'd remove the
0x30000+128K range from the memory map, so the fw_cfg stuff (or anything
else) would never allocate memory from the range. It's much more
concerning to me however how the SMM infrastructure would deal with a
hole in the memory map right there.

Thanks
Laszlo

[PATCH] q35: lpc: allow to lock down 128K RAM at default SMBASE address

[Igor Mammedov]

lpc already has SMI negotiation feature, extend it by adding
optin ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT to supported features.

Writing this bit into "etc/smi/requested-features" fw_cfg file,
tells QEMU to alias 0x30000,128K RAM range into SMRAM address
space and mask this region from normal RAM address space
(reads return 0xff and writes are ignored, i.e. guest code
should be able to deal with not usable 0x30000,128K RAM range
once ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT is activated).

To make negotiated change effective, guest should read
"etc/smi/features-ok" fw_cfg file, which activates negotiated
features and locks down negotiating capabilities until hard reset.

Flow for initializing SMI handler on guest side:
 1. set SMI handler entry point at default SMBASE location
 2. check that host supports ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT
    in "etc/smi/supported-features" and set if supported set
    it in "etc/smi/requested-features"
 3. read "etc/smi/features-ok", if returned value is 1
    negotiated at step 2 features are activated successfully.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/i386/ich9.h | 11 ++++++--
 hw/i386/pc.c           |  4 ++-
 hw/i386/pc_q35.c       |  3 ++-
 hw/isa/lpc_ich9.c      | 58 +++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index 72e803f6e2..c28685b753 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -12,11 +12,14 @@
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/ich9.h"
 #include "hw/pci/pci_bus.h"
+#include "qemu/units.h"
 
 void ich9_lpc_set_irq(void *opaque, int irq_num, int level);
 int ich9_lpc_map_irq(PCIDevice *pci_dev, int intx);
 PCIINTxRoute ich9_route_intx_pin_to_irq(void *opaque, int pirq_pin);
-void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled);
+void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled,
+                      MemoryRegion *system_memory, MemoryRegion *ram,
+                      MemoryRegion *smram);
 I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base);
 
 void ich9_generate_smi(void);
@@ -71,6 +74,8 @@ typedef struct ICH9LPCState {
     uint8_t smi_features_ok;          /* guest-visible, read-only; selecting it
                                        * triggers feature lockdown */
     uint64_t smi_negotiated_features; /* guest-invisible, host endian */
+    MemoryRegion smbase_blackhole;
+    MemoryRegion smbase_window;
 
     /* isa bus */
     ISABus *isa_bus;
@@ -248,5 +253,7 @@ typedef struct ICH9LPCState {
 
 /* bit positions used in fw_cfg SMI feature negotiation */
 #define ICH9_LPC_SMI_F_BROADCAST_BIT            0
-
+#define ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT        1
+#define ICH9_LPC_SMBASE_ADDR                    0x30000
+#define ICH9_LPC_SMBASE_RAM_SIZE                (128 * KiB)
 #endif /* HW_ICH9_H */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c14ed86439..99a98303eb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -119,7 +119,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 /* Physical Address of PVH entry point read from kernel ELF NOTE */
 static size_t pvh_start_addr;
 
-GlobalProperty pc_compat_4_1[] = {};
+GlobalProperty pc_compat_4_1[] = {
+    { "ICH9-LPC", "x-smi-locked-smbase", "off" },
+};
 const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
 
 GlobalProperty pc_compat_4_0[] = {};
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d4e8a1cb9f..50462686a0 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -292,7 +292,8 @@ static void pc_q35_init(MachineState *machine)
                          0xff0104);
 
     /* connect pm stuff to lpc */
-    ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms));
+    ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms), get_system_memory(),
+        ram_memory, MEMORY_REGION(object_resolve_path("/machine/smram", NULL)));
 
     if (pcms->sata_enabled) {
         /* ahci and SATA device, for q35 1 ahci controller is built-in */
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 17c292e306..17a8cd1b51 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -359,6 +359,38 @@ static void ich9_set_sci(void *opaque, int irq_num, int level)
     }
 }
 
+static uint64_t smbase_blackhole_read(void *ptr, hwaddr reg, unsigned size)
+{
+    return 0xffffffff;
+}
+
+static void smbase_blackhole_write(void *opaque, hwaddr addr, uint64_t val,
+                                   unsigned width)
+{
+    /* nothing */
+}
+
+static const MemoryRegionOps smbase_blackhole_ops = {
+    .read = smbase_blackhole_read,
+    .write = smbase_blackhole_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .impl.min_access_size = 4,
+    .impl.max_access_size = 4,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void ich9_lpc_smbase_locked_update(ICH9LPCState *lpc)
+{
+    bool en = lpc->smi_negotiated_features & ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT;
+
+    memory_region_transaction_begin();
+    memory_region_set_enabled(&lpc->smbase_blackhole, en);
+    memory_region_set_enabled(&lpc->smbase_window, en);
+    memory_region_transaction_commit();
+}
+
 static void smi_features_ok_callback(void *opaque)
 {
     ICH9LPCState *lpc = opaque;
@@ -379,9 +411,13 @@ static void smi_features_ok_callback(void *opaque)
     /* valid feature subset requested, lock it down, report success */
     lpc->smi_negotiated_features = guest_features;
     lpc->smi_features_ok = 1;
+
+    ich9_lpc_smbase_locked_update(lpc);
 }
 
-void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled)
+void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled,
+                      MemoryRegion *system_memory,  MemoryRegion *ram,
+                      MemoryRegion *smram)
 {
     ICH9LPCState *lpc = ICH9_LPC_DEVICE(lpc_pci);
     qemu_irq sci_irq;
@@ -413,6 +449,20 @@ void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled)
                                  &lpc->smi_features_ok,
                                  sizeof lpc->smi_features_ok,
                                  true);
+
+        memory_region_init_io(&lpc->smbase_blackhole, OBJECT(lpc),
+                              &smbase_blackhole_ops, NULL,
+                              "smbase-blackhole", ICH9_LPC_SMBASE_RAM_SIZE);
+        memory_region_set_enabled(&lpc->smbase_blackhole, false);
+        memory_region_add_subregion_overlap(system_memory, ICH9_LPC_SMBASE_ADDR,
+                                            &lpc->smbase_blackhole, 1);
+
+
+        memory_region_init_alias(&lpc->smbase_window, OBJECT(lpc),
+            "smbase-window", ram,
+             ICH9_LPC_SMBASE_ADDR, ICH9_LPC_SMBASE_RAM_SIZE);
+        memory_region_set_enabled(&lpc->smbase_window, false);
+        memory_region_add_subregion(smram, 0x30000, &lpc->smbase_window);
     }
 
     ich9_lpc_reset(DEVICE(lpc));
@@ -508,6 +558,7 @@ static int ich9_lpc_post_load(void *opaque, int version_id)
     ich9_lpc_pmbase_sci_update(lpc);
     ich9_lpc_rcba_update(lpc, 0 /* disabled ICH9_LPC_RCBA_EN */);
     ich9_lpc_pmcon_update(lpc);
+    ich9_lpc_smbase_locked_update(lpc);
     return 0;
 }
 
@@ -567,6 +618,8 @@ static void ich9_lpc_reset(DeviceState *qdev)
     memset(lpc->smi_guest_features_le, 0, sizeof lpc->smi_guest_features_le);
     lpc->smi_features_ok = 0;
     lpc->smi_negotiated_features = 0;
+
+    ich9_lpc_smbase_locked_update(lpc);
 }
 
 /* root complex register block is mapped into memory space */
@@ -697,6 +750,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
     qdev_init_gpio_out_named(dev, lpc->gsi, ICH9_GPIO_GSI, GSI_NUM_PINS);
 
     isa_bus_irqs(isa_bus, lpc->gsi);
+
 }
 
 static bool ich9_rst_cnt_needed(void *opaque)
@@ -764,6 +818,8 @@ static Property ich9_lpc_properties[] = {
     DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
     DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
                       ICH9_LPC_SMI_F_BROADCAST_BIT, true),
+    DEFINE_PROP_BIT64("x-smi-locked-smbase", ICH9LPCState, smi_host_features,
+                      ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.18.1

Re: [PATCH] q35: lpc: allow to lock down 128K RAM at default SMBASE address

[Laszlo Ersek]

Hi Igor,

On 09/05/19 17:49, Igor Mammedov wrote:
> lpc already has SMI negotiation feature, extend it by adding
> optin ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT to supported features.
>
> Writing this bit into "etc/smi/requested-features" fw_cfg file,
> tells QEMU to alias 0x30000,128K RAM range into SMRAM address
> space and mask this region from normal RAM address space
> (reads return 0xff and writes are ignored, i.e. guest code
> should be able to deal with not usable 0x30000,128K RAM range
> once ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT is activated).
>
> To make negotiated change effective, guest should read
> "etc/smi/features-ok" fw_cfg file, which activates negotiated
> features and locks down negotiating capabilities until hard reset.
>
> Flow for initializing SMI handler on guest side:
>  1. set SMI handler entry point at default SMBASE location
>  2. check that host supports ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT
>     in "etc/smi/supported-features" and set if supported set
>     it in "etc/smi/requested-features"
>  3. read "etc/smi/features-ok", if returned value is 1
>     negotiated at step 2 features are activated successfully.

Tying the [0x30000+128K) lockdown to the broadcast SMI negotiation is a
simplification for QEMU, but it is a complication for OVMF.

(This QEMU patch ties those things together in effect because
"etc/smi/features-ok" can be selected for lockdown only once.)

In OVMF, at least 6 modules are involved in SMM setup. Here I'm only
going to list some steps for 4 modules (skipping
"OvmfPkg/SmmAccess/SmmAccess2Dxe.inf" and
"UefiCpuPkg/CpuIo2Smm/CpuIo2Smm.inf").


(1) The "OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf" driver is launched,
and it produces the EFI_SMM_CONTROL2_PROTOCOL.

EFI_SMM_CONTROL2_PROTOCOL.Trigger() is the standard / abstract method
for synchronously raising an SMI. The OVMF implementation writes to IO
port 0xB2.

Because OVMF exposes this protocol to the rest of the firmware, it first
negotiates SMI broadcast, if QEMU offers it. The idea is that, without
negotiating SMI broadcast (if it's available), EFI_SMM_CONTROL2_PROTOCOL
is not fully configured, and should not be exposed. (Because, Trigger()
wouldn't work properly). Incomplete / halfway functional protocols are
not to be published.

That is, we have

(1a) negotiate SMI broadcast
(1b) install EFI_SMM_CONTROL2_PROTOCOL.


(2) Dependent on EFI_SMM_CONTROL2_PROTOCOL, the SMM IPL (Initial Program
Load -- "MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf") is launched.

This module
(2a) registers a callback for EFI_SMM_CONFIGURATION_PROTOCOL,
(2b) loads the SMM Core ("MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf")
     into SMRAM and starts it.


(3) The SMM Core launches the SMM processor driver
("UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf").

The SMM processor driver
(3a) performs the initial SMBASE relocation,
(3b) and then installs EFI_SMM_CONFIGURATION_PROTOCOL.

(Side remark: the SMM processor driver does not use IO port 0xB2 (it
does not call Trigger()); it uses LAPIC accesses. This is by design (PI
spec); Trigger() is supposed to be called  after the relocation is done,
and not for starting the relocation.)


(4) The SMM IPL's callback fires. It uses EFI_SMM_CONFIGURATION_PROTOCOL
to connect the platform-independent SMM entry point (= central
high-level SMI handler), which is in the SMM Core, into the low-level
(CPU-specific) SMI handler in the SMM processor driver.

At this point, SMIs are considered fully functional. General drivers
that are split into privileged (SMM) and unprivileged (runtime DXE)
halves, such as the variable service driver, can use
EFI_SMM_COMMUNICATION_PROTOCOL to submit messages to the privileged
(SMM) halves. And that boils down to EFI_SMM_CONTROL2_PROTOCOL.Trigger()
calls, which depends on SMI broadcast.

--*--

The present QEMU patch requires the firmware to (i) negotiate SMI
broadcast and to (ii) lock down [0x30000+128K) at the same time.

If OVMF does both in step (1a) -- i.e. where it currently negotiates the
broadcast --, then step (3a) breaks: because the initial SMBASE
relocation depends on RAM at [0x30000+128K).

In a theoretical ordering perspective, we could perhaps move the logic
from step (1a) between steps (3a) and (3b). There are two problems with
that:

- The platform logic from step (1a) doesn't belong in the SMM processor
driver (even if we managed to hook it in).

- In step (1b), we'd be installing a protocol
(EFI_SMM_CONTROL2_PROTOCOL) that is simply not set up correctly -- it's
incomplete.


Can QEMU offer this new "[0x30000+128K) lockdown" hardware feature in a
separate platform device? (Such as a PCI device with fixed
(QEMU-specified) B/D/F, and config space register(s).)

It would be less difficult to lock such hardware down in isolation: I
wouldn't even attempt to inject that action between steps (3a) and (3b),
but write it as a new, independent End-of-DXE handler, in
"OvmfPkg/SmmAccess/SmmAccess2Dxe.inf". (That driver already offers SMRAM
open/close/lock services.) I would also reserve the memory away at that
time -- I don't expect the firmware to keep anything that low.
(Allocations are generally served top-down.)

--*--

... I've done some testing too. Applying the QEMU patch on top of
89ea03a7dc83, my plan was:

- do not change OVMF, just see if it continues booting with the QEMU
patch

- then negotiate bit#1 too, in step (1a) -- this is when I'd expect (3a)
to break.

Unfortunately, the result is worse than that; even without negotiating
bit#1 (i.e. in the baseline test), the firmware crashes (reboots) in
step (3a). I've checked "info mtree", and all occurences of
"smbase-blackhole" and "smbase-blackhole" are marked [disabled]. I'm not
sure what's wrong with the baseline test (i.e. without negotiating
bit#1). If I drop the patch (build QEMU at 89ea03a7dc83), then things
work fine.

Thank you!
Laszlo

>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/i386/ich9.h | 11 ++++++--
>  hw/i386/pc.c           |  4 ++-
>  hw/i386/pc_q35.c       |  3 ++-
>  hw/isa/lpc_ich9.c      | 58 +++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 71 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index 72e803f6e2..c28685b753 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -12,11 +12,14 @@
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/ich9.h"
>  #include "hw/pci/pci_bus.h"
> +#include "qemu/units.h"
>
>  void ich9_lpc_set_irq(void *opaque, int irq_num, int level);
>  int ich9_lpc_map_irq(PCIDevice *pci_dev, int intx);
>  PCIINTxRoute ich9_route_intx_pin_to_irq(void *opaque, int pirq_pin);
> -void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled);
> +void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled,
> +                      MemoryRegion *system_memory, MemoryRegion *ram,
> +                      MemoryRegion *smram);
>  I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base);
>
>  void ich9_generate_smi(void);
> @@ -71,6 +74,8 @@ typedef struct ICH9LPCState {
>      uint8_t smi_features_ok;          /* guest-visible, read-only; selecting it
>                                         * triggers feature lockdown */
>      uint64_t smi_negotiated_features; /* guest-invisible, host endian */
> +    MemoryRegion smbase_blackhole;
> +    MemoryRegion smbase_window;
>
>      /* isa bus */
>      ISABus *isa_bus;
> @@ -248,5 +253,7 @@ typedef struct ICH9LPCState {
>
>  /* bit positions used in fw_cfg SMI feature negotiation */
>  #define ICH9_LPC_SMI_F_BROADCAST_BIT            0
> -
> +#define ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT        1
> +#define ICH9_LPC_SMBASE_ADDR                    0x30000
> +#define ICH9_LPC_SMBASE_RAM_SIZE                (128 * KiB)
>  #endif /* HW_ICH9_H */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c14ed86439..99a98303eb 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -119,7 +119,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>  /* Physical Address of PVH entry point read from kernel ELF NOTE */
>  static size_t pvh_start_addr;
>
> -GlobalProperty pc_compat_4_1[] = {};
> +GlobalProperty pc_compat_4_1[] = {
> +    { "ICH9-LPC", "x-smi-locked-smbase", "off" },
> +};
>  const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
>
>  GlobalProperty pc_compat_4_0[] = {};
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d4e8a1cb9f..50462686a0 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -292,7 +292,8 @@ static void pc_q35_init(MachineState *machine)
>                           0xff0104);
>
>      /* connect pm stuff to lpc */
> -    ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms));
> +    ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms), get_system_memory(),
> +        ram_memory, MEMORY_REGION(object_resolve_path("/machine/smram", NULL)));
>
>      if (pcms->sata_enabled) {
>          /* ahci and SATA device, for q35 1 ahci controller is built-in */
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 17c292e306..17a8cd1b51 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -359,6 +359,38 @@ static void ich9_set_sci(void *opaque, int irq_num, int level)
>      }
>  }
>
> +static uint64_t smbase_blackhole_read(void *ptr, hwaddr reg, unsigned size)
> +{
> +    return 0xffffffff;
> +}
> +
> +static void smbase_blackhole_write(void *opaque, hwaddr addr, uint64_t val,
> +                                   unsigned width)
> +{
> +    /* nothing */
> +}
> +
> +static const MemoryRegionOps smbase_blackhole_ops = {
> +    .read = smbase_blackhole_read,
> +    .write = smbase_blackhole_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .impl.min_access_size = 4,
> +    .impl.max_access_size = 4,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void ich9_lpc_smbase_locked_update(ICH9LPCState *lpc)
> +{
> +    bool en = lpc->smi_negotiated_features & ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT;
> +
> +    memory_region_transaction_begin();
> +    memory_region_set_enabled(&lpc->smbase_blackhole, en);
> +    memory_region_set_enabled(&lpc->smbase_window, en);
> +    memory_region_transaction_commit();
> +}
> +
>  static void smi_features_ok_callback(void *opaque)
>  {
>      ICH9LPCState *lpc = opaque;
> @@ -379,9 +411,13 @@ static void smi_features_ok_callback(void *opaque)
>      /* valid feature subset requested, lock it down, report success */
>      lpc->smi_negotiated_features = guest_features;
>      lpc->smi_features_ok = 1;
> +
> +    ich9_lpc_smbase_locked_update(lpc);
>  }
>
> -void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled)
> +void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled,
> +                      MemoryRegion *system_memory,  MemoryRegion *ram,
> +                      MemoryRegion *smram)
>  {
>      ICH9LPCState *lpc = ICH9_LPC_DEVICE(lpc_pci);
>      qemu_irq sci_irq;
> @@ -413,6 +449,20 @@ void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled)
>                                   &lpc->smi_features_ok,
>                                   sizeof lpc->smi_features_ok,
>                                   true);
> +
> +        memory_region_init_io(&lpc->smbase_blackhole, OBJECT(lpc),
> +                              &smbase_blackhole_ops, NULL,
> +                              "smbase-blackhole", ICH9_LPC_SMBASE_RAM_SIZE);
> +        memory_region_set_enabled(&lpc->smbase_blackhole, false);
> +        memory_region_add_subregion_overlap(system_memory, ICH9_LPC_SMBASE_ADDR,
> +                                            &lpc->smbase_blackhole, 1);
> +
> +
> +        memory_region_init_alias(&lpc->smbase_window, OBJECT(lpc),
> +            "smbase-window", ram,
> +             ICH9_LPC_SMBASE_ADDR, ICH9_LPC_SMBASE_RAM_SIZE);
> +        memory_region_set_enabled(&lpc->smbase_window, false);
> +        memory_region_add_subregion(smram, 0x30000, &lpc->smbase_window);
>      }
>
>      ich9_lpc_reset(DEVICE(lpc));
> @@ -508,6 +558,7 @@ static int ich9_lpc_post_load(void *opaque, int version_id)
>      ich9_lpc_pmbase_sci_update(lpc);
>      ich9_lpc_rcba_update(lpc, 0 /* disabled ICH9_LPC_RCBA_EN */);
>      ich9_lpc_pmcon_update(lpc);
> +    ich9_lpc_smbase_locked_update(lpc);
>      return 0;
>  }
>
> @@ -567,6 +618,8 @@ static void ich9_lpc_reset(DeviceState *qdev)
>      memset(lpc->smi_guest_features_le, 0, sizeof lpc->smi_guest_features_le);
>      lpc->smi_features_ok = 0;
>      lpc->smi_negotiated_features = 0;
> +
> +    ich9_lpc_smbase_locked_update(lpc);
>  }
>
>  /* root complex register block is mapped into memory space */
> @@ -697,6 +750,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
>      qdev_init_gpio_out_named(dev, lpc->gsi, ICH9_GPIO_GSI, GSI_NUM_PINS);
>
>      isa_bus_irqs(isa_bus, lpc->gsi);
> +
>  }
>
>  static bool ich9_rst_cnt_needed(void *opaque)
> @@ -764,6 +818,8 @@ static Property ich9_lpc_properties[] = {
>      DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
>      DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
>                        ICH9_LPC_SMI_F_BROADCAST_BIT, true),
> +    DEFINE_PROP_BIT64("x-smi-locked-smbase", ICH9LPCState, smi_host_features,
> +                      ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
>

Re: [PATCH] q35: lpc: allow to lock down 128K RAM at default SMBASE address

[Igor Mammedov]

On Mon, 9 Sep 2019 21:15:44 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> Hi Igor,
> 
> On 09/05/19 17:49, Igor Mammedov wrote:
> > lpc already has SMI negotiation feature, extend it by adding
> > optin ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT to supported features.
> >
> > Writing this bit into "etc/smi/requested-features" fw_cfg file,
> > tells QEMU to alias 0x30000,128K RAM range into SMRAM address
> > space and mask this region from normal RAM address space
> > (reads return 0xff and writes are ignored, i.e. guest code
> > should be able to deal with not usable 0x30000,128K RAM range
> > once ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT is activated).
> >
> > To make negotiated change effective, guest should read
> > "etc/smi/features-ok" fw_cfg file, which activates negotiated
> > features and locks down negotiating capabilities until hard reset.
> >
> > Flow for initializing SMI handler on guest side:
> >  1. set SMI handler entry point at default SMBASE location
> >  2. check that host supports ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT
> >     in "etc/smi/supported-features" and set if supported set
> >     it in "etc/smi/requested-features"
> >  3. read "etc/smi/features-ok", if returned value is 1
> >     negotiated at step 2 features are activated successfully.  
> 
> Tying the [0x30000+128K) lockdown to the broadcast SMI negotiation is a
> simplification for QEMU, but it is a complication for OVMF.
> 
> (This QEMU patch ties those things together in effect because
> "etc/smi/features-ok" can be selected for lockdown only once.)
> 
> In OVMF, at least 6 modules are involved in SMM setup. Here I'm only
> going to list some steps for 4 modules (skipping
> "OvmfPkg/SmmAccess/SmmAccess2Dxe.inf" and
> "UefiCpuPkg/CpuIo2Smm/CpuIo2Smm.inf").
> 
> 
> (1) The "OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf" driver is launched,
> and it produces the EFI_SMM_CONTROL2_PROTOCOL.
> 
> EFI_SMM_CONTROL2_PROTOCOL.Trigger() is the standard / abstract method
> for synchronously raising an SMI. The OVMF implementation writes to IO
> port 0xB2.
> 
> Because OVMF exposes this protocol to the rest of the firmware, it first
> negotiates SMI broadcast, if QEMU offers it. The idea is that, without
> negotiating SMI broadcast (if it's available), EFI_SMM_CONTROL2_PROTOCOL
> is not fully configured, and should not be exposed. (Because, Trigger()
> wouldn't work properly). Incomplete / halfway functional protocols are
> not to be published.
> 
> That is, we have
> 
> (1a) negotiate SMI broadcast
> (1b) install EFI_SMM_CONTROL2_PROTOCOL.
> 
> 
> (2) Dependent on EFI_SMM_CONTROL2_PROTOCOL, the SMM IPL (Initial Program
> Load -- "MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf") is launched.
> 
> This module
> (2a) registers a callback for EFI_SMM_CONFIGURATION_PROTOCOL,
> (2b) loads the SMM Core ("MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf")
>      into SMRAM and starts it.
> 
> 
> (3) The SMM Core launches the SMM processor driver
> ("UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf").
> 
> The SMM processor driver
> (3a) performs the initial SMBASE relocation,
> (3b) and then installs EFI_SMM_CONFIGURATION_PROTOCOL.
> 
> (Side remark: the SMM processor driver does not use IO port 0xB2 (it
> does not call Trigger()); it uses LAPIC accesses. This is by design (PI
> spec); Trigger() is supposed to be called  after the relocation is done,
> and not for starting the relocation.)
> 
> 
> (4) The SMM IPL's callback fires. It uses EFI_SMM_CONFIGURATION_PROTOCOL
> to connect the platform-independent SMM entry point (= central
> high-level SMI handler), which is in the SMM Core, into the low-level
> (CPU-specific) SMI handler in the SMM processor driver.
> 
> At this point, SMIs are considered fully functional. General drivers
> that are split into privileged (SMM) and unprivileged (runtime DXE)
> halves, such as the variable service driver, can use
> EFI_SMM_COMMUNICATION_PROTOCOL to submit messages to the privileged
> (SMM) halves. And that boils down to EFI_SMM_CONTROL2_PROTOCOL.Trigger()
> calls, which depends on SMI broadcast.
> 
> --*--
> 
> The present QEMU patch requires the firmware to (i) negotiate SMI
> broadcast and to (ii) lock down [0x30000+128K) at the same time.
> 
> If OVMF does both in step (1a) -- i.e. where it currently negotiates the
> broadcast --, then step (3a) breaks: because the initial SMBASE
> relocation depends on RAM at [0x30000+128K).
> 
> In a theoretical ordering perspective, we could perhaps move the logic
> from step (1a) between steps (3a) and (3b). There are two problems with
> that:
> 
> - The platform logic from step (1a) doesn't belong in the SMM processor
> driver (even if we managed to hook it in).
> 
> - In step (1b), we'd be installing a protocol
> (EFI_SMM_CONTROL2_PROTOCOL) that is simply not set up correctly -- it's
> incomplete.
> 
> 
> Can QEMU offer this new "[0x30000+128K) lockdown" hardware feature in a
> separate platform device? (Such as a PCI device with fixed
> (QEMU-specified) B/D/F, and config space register(s).)
It looks like fwcfg smi feature negotiation isn't reusable in this case.
I'd prefer not to add another device just for another SMI feature
negotiation/activation.
How about stealing reserved register from pci-host similar to your
extended TSEG commit (2f295167 q35/mch: implement extended TSEG sizes)?
(Looking into spec can (ab)use 0x58 or 0x59 register)

> It would be less difficult to lock such hardware down in isolation: I
> wouldn't even attempt to inject that action between steps (3a) and (3b),
> but write it as a new, independent End-of-DXE handler, in
> "OvmfPkg/SmmAccess/SmmAccess2Dxe.inf". (That driver already offers SMRAM
> open/close/lock services.) I would also reserve the memory away at that
> time -- I don't expect the firmware to keep anything that low.
> (Allocations are generally served top-down.)

> 
> --*--
> 
> ... I've done some testing too. Applying the QEMU patch on top of
> 89ea03a7dc83, my plan was:
> 
> - do not change OVMF, just see if it continues booting with the QEMU
> patch
> 
> - then negotiate bit#1 too, in step (1a) -- this is when I'd expect (3a)
> to break.
> 
> Unfortunately, the result is worse than that; even without negotiating
> bit#1 (i.e. in the baseline test), the firmware crashes (reboots) in
> step (3a). I've checked "info mtree", and all occurences of
> "smbase-blackhole" and "smbase-blackhole" are marked [disabled]. I'm not
> sure what's wrong with the baseline test (i.e. without negotiating
> bit#1). If I drop the patch (build QEMU at 89ea03a7dc83), then things
> work fine.

that was a bug in my code, which always made lock down effective on
feature_ok selection, which breaks relocation for reasons you've
explained above.

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 17a8cd1b51..32ddf54fc2 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -383,7 +383,7 @@ static const MemoryRegionOps smbase_blackhole_ops = {
 
 static void ich9_lpc_smbase_locked_update(ICH9LPCState *lpc)
 {
-    bool en = lpc->smi_negotiated_features & ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT;
+    bool en = lpc->smi_negotiated_features & (UINT64_C(1) << ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT);
 
     memory_region_transaction_begin();
     memory_region_set_enabled(&lpc->smbase_blackhole, en);


> 
> Thank you!
> Laszlo
> 
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/hw/i386/ich9.h | 11 ++++++--
> >  hw/i386/pc.c           |  4 ++-
> >  hw/i386/pc_q35.c       |  3 ++-
> >  hw/isa/lpc_ich9.c      | 58 +++++++++++++++++++++++++++++++++++++++++-
> >  4 files changed, 71 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> > index 72e803f6e2..c28685b753 100644
> > --- a/include/hw/i386/ich9.h
> > +++ b/include/hw/i386/ich9.h
> > @@ -12,11 +12,14 @@
> >  #include "hw/acpi/acpi.h"
> >  #include "hw/acpi/ich9.h"
> >  #include "hw/pci/pci_bus.h"
> > +#include "qemu/units.h"
> >
> >  void ich9_lpc_set_irq(void *opaque, int irq_num, int level);
> >  int ich9_lpc_map_irq(PCIDevice *pci_dev, int intx);
> >  PCIINTxRoute ich9_route_intx_pin_to_irq(void *opaque, int pirq_pin);
> > -void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled);
> > +void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled,
> > +                      MemoryRegion *system_memory, MemoryRegion *ram,
> > +                      MemoryRegion *smram);
> >  I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base);
> >
> >  void ich9_generate_smi(void);
> > @@ -71,6 +74,8 @@ typedef struct ICH9LPCState {
> >      uint8_t smi_features_ok;          /* guest-visible, read-only; selecting it
> >                                         * triggers feature lockdown */
> >      uint64_t smi_negotiated_features; /* guest-invisible, host endian */
> > +    MemoryRegion smbase_blackhole;
> > +    MemoryRegion smbase_window;
> >
> >      /* isa bus */
> >      ISABus *isa_bus;
> > @@ -248,5 +253,7 @@ typedef struct ICH9LPCState {
> >
> >  /* bit positions used in fw_cfg SMI feature negotiation */
> >  #define ICH9_LPC_SMI_F_BROADCAST_BIT            0
> > -
> > +#define ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT        1
> > +#define ICH9_LPC_SMBASE_ADDR                    0x30000
> > +#define ICH9_LPC_SMBASE_RAM_SIZE                (128 * KiB)
> >  #endif /* HW_ICH9_H */
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index c14ed86439..99a98303eb 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -119,7 +119,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
> >  /* Physical Address of PVH entry point read from kernel ELF NOTE */
> >  static size_t pvh_start_addr;
> >
> > -GlobalProperty pc_compat_4_1[] = {};
> > +GlobalProperty pc_compat_4_1[] = {
> > +    { "ICH9-LPC", "x-smi-locked-smbase", "off" },
> > +};
> >  const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
> >
> >  GlobalProperty pc_compat_4_0[] = {};
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index d4e8a1cb9f..50462686a0 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -292,7 +292,8 @@ static void pc_q35_init(MachineState *machine)
> >                           0xff0104);
> >
> >      /* connect pm stuff to lpc */
> > -    ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms));
> > +    ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms), get_system_memory(),
> > +        ram_memory, MEMORY_REGION(object_resolve_path("/machine/smram", NULL)));
> >
> >      if (pcms->sata_enabled) {
> >          /* ahci and SATA device, for q35 1 ahci controller is built-in */
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index 17c292e306..17a8cd1b51 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -359,6 +359,38 @@ static void ich9_set_sci(void *opaque, int irq_num, int level)
> >      }
> >  }
> >
> > +static uint64_t smbase_blackhole_read(void *ptr, hwaddr reg, unsigned size)
> > +{
> > +    return 0xffffffff;
> > +}
> > +
> > +static void smbase_blackhole_write(void *opaque, hwaddr addr, uint64_t val,
> > +                                   unsigned width)
> > +{
> > +    /* nothing */
> > +}
> > +
> > +static const MemoryRegionOps smbase_blackhole_ops = {
> > +    .read = smbase_blackhole_read,
> > +    .write = smbase_blackhole_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +    .valid.min_access_size = 1,
> > +    .valid.max_access_size = 4,
> > +    .impl.min_access_size = 4,
> > +    .impl.max_access_size = 4,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +};
> > +
> > +static void ich9_lpc_smbase_locked_update(ICH9LPCState *lpc)
> > +{
> > +    bool en = lpc->smi_negotiated_features & ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT;
> > +
> > +    memory_region_transaction_begin();
> > +    memory_region_set_enabled(&lpc->smbase_blackhole, en);
> > +    memory_region_set_enabled(&lpc->smbase_window, en);
> > +    memory_region_transaction_commit();
> > +}
> > +
> >  static void smi_features_ok_callback(void *opaque)
> >  {
> >      ICH9LPCState *lpc = opaque;
> > @@ -379,9 +411,13 @@ static void smi_features_ok_callback(void *opaque)
> >      /* valid feature subset requested, lock it down, report success */
> >      lpc->smi_negotiated_features = guest_features;
> >      lpc->smi_features_ok = 1;
> > +
> > +    ich9_lpc_smbase_locked_update(lpc);
> >  }
> >
> > -void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled)
> > +void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled,
> > +                      MemoryRegion *system_memory,  MemoryRegion *ram,
> > +                      MemoryRegion *smram)
> >  {
> >      ICH9LPCState *lpc = ICH9_LPC_DEVICE(lpc_pci);
> >      qemu_irq sci_irq;
> > @@ -413,6 +449,20 @@ void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled)
> >                                   &lpc->smi_features_ok,
> >                                   sizeof lpc->smi_features_ok,
> >                                   true);
> > +
> > +        memory_region_init_io(&lpc->smbase_blackhole, OBJECT(lpc),
> > +                              &smbase_blackhole_ops, NULL,
> > +                              "smbase-blackhole", ICH9_LPC_SMBASE_RAM_SIZE);
> > +        memory_region_set_enabled(&lpc->smbase_blackhole, false);
> > +        memory_region_add_subregion_overlap(system_memory, ICH9_LPC_SMBASE_ADDR,
> > +                                            &lpc->smbase_blackhole, 1);
> > +
> > +
> > +        memory_region_init_alias(&lpc->smbase_window, OBJECT(lpc),
> > +            "smbase-window", ram,
> > +             ICH9_LPC_SMBASE_ADDR, ICH9_LPC_SMBASE_RAM_SIZE);
> > +        memory_region_set_enabled(&lpc->smbase_window, false);
> > +        memory_region_add_subregion(smram, 0x30000, &lpc->smbase_window);
> >      }
> >
> >      ich9_lpc_reset(DEVICE(lpc));
> > @@ -508,6 +558,7 @@ static int ich9_lpc_post_load(void *opaque, int version_id)
> >      ich9_lpc_pmbase_sci_update(lpc);
> >      ich9_lpc_rcba_update(lpc, 0 /* disabled ICH9_LPC_RCBA_EN */);
> >      ich9_lpc_pmcon_update(lpc);
> > +    ich9_lpc_smbase_locked_update(lpc);
> >      return 0;
> >  }
> >
> > @@ -567,6 +618,8 @@ static void ich9_lpc_reset(DeviceState *qdev)
> >      memset(lpc->smi_guest_features_le, 0, sizeof lpc->smi_guest_features_le);
> >      lpc->smi_features_ok = 0;
> >      lpc->smi_negotiated_features = 0;
> > +
> > +    ich9_lpc_smbase_locked_update(lpc);
> >  }
> >
> >  /* root complex register block is mapped into memory space */
> > @@ -697,6 +750,7 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
> >      qdev_init_gpio_out_named(dev, lpc->gsi, ICH9_GPIO_GSI, GSI_NUM_PINS);
> >
> >      isa_bus_irqs(isa_bus, lpc->gsi);
> > +
> >  }
> >
> >  static bool ich9_rst_cnt_needed(void *opaque)
> > @@ -764,6 +818,8 @@ static Property ich9_lpc_properties[] = {
> >      DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
> >      DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
> >                        ICH9_LPC_SMI_F_BROADCAST_BIT, true),
> > +    DEFINE_PROP_BIT64("x-smi-locked-smbase", ICH9LPCState, smi_host_features,
> > +                      ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT, true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> >  
> 

Re: [PATCH] q35: lpc: allow to lock down 128K RAM at default SMBASE address

[Laszlo Ersek]

On 09/10/19 17:58, Igor Mammedov wrote:
> On Mon, 9 Sep 2019 21:15:44 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:

[...]

> It looks like fwcfg smi feature negotiation isn't reusable in this case.
> I'd prefer not to add another device just for another SMI feature
> negotiation/activation.

I thought it could be a register on the new CPU hotplug controller that
we're going to need anyway (if I understand correctly, at least).

But:

> How about stealing reserved register from pci-host similar to your
> extended TSEG commit (2f295167 q35/mch: implement extended TSEG sizes)?
> (Looking into spec can (ab)use 0x58 or 0x59 register)

Yes, that should work.

In fact, I had considered 0x58 / 0x59 when looking for unused registers
for extended TSEG configuration:

http://mid.mail-archive.com/d8802612-0b11-776f-b209-53bbdaf67515@redhat.com

So I'm OK with this, thank you.

More below:

>> ... I've done some testing too. Applying the QEMU patch on top of
>> 89ea03a7dc83, my plan was:
>>
>> - do not change OVMF, just see if it continues booting with the QEMU
>> patch
>>
>> - then negotiate bit#1 too, in step (1a) -- this is when I'd expect (3a)
>> to break.
>>
>> Unfortunately, the result is worse than that; even without negotiating
>> bit#1 (i.e. in the baseline test), the firmware crashes (reboots) in
>> step (3a). I've checked "info mtree", and all occurences of
>> "smbase-blackhole" and "smbase-blackhole" are marked [disabled]. I'm not
>> sure what's wrong with the baseline test (i.e. without negotiating
>> bit#1). If I drop the patch (build QEMU at 89ea03a7dc83), then things
>> work fine.
> 
> that was a bug in my code, which always made lock down effective on
> feature_ok selection, which breaks relocation for reasons you've
> explained above.
> 
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 17a8cd1b51..32ddf54fc2 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -383,7 +383,7 @@ static const MemoryRegionOps smbase_blackhole_ops = {
>  
>  static void ich9_lpc_smbase_locked_update(ICH9LPCState *lpc)
>  {
> -    bool en = lpc->smi_negotiated_features & ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT;
> +    bool en = lpc->smi_negotiated_features & (UINT64_C(1) << ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT);
>  
>      memory_region_transaction_begin();
>      memory_region_set_enabled(&lpc->smbase_blackhole, en);

I see.

ICH9_LPC_SMI_F_LOCKED_SMBASE_BIT is 1, with the intended value for
bitmask checkin) being 1LLU<<1 == 2LLU.

Due to the bug, the function would check value 1 in the bitmask -- which
in fact corresponds to bit#0. Bit#0 happens to be
ICH9_LPC_SMI_F_BROADCAST_BIT.

And because OVMF would negotiate that feature (= broadcast SMI) even in
the baseline test, it ended up enabling the "locked smbase" feature too.

So ultimately I think we can consider this a valid test (= with
meaningful result); the result is that we can't reuse these fw_cfg files
for "locked smbase", as discussed above.

Thanks!
Laszlo

Re: [edk2-devel] [PATCH] q35: lpc: allow to lock down 128K RAM at default SMBASE address

[Igor Mammedov]

On Wed, 11 Sep 2019 19:30:46 +0200
"Laszlo Ersek" <lersek@redhat.com> wrote:

> On 09/10/19 17:58, Igor Mammedov wrote:
> > On Mon, 9 Sep 2019 21:15:44 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:  
> 
> [...]
> 
> > It looks like fwcfg smi feature negotiation isn't reusable in this case.
> > I'd prefer not to add another device just for another SMI feature
> > negotiation/activation.  
> 
> I thought it could be a register on the new CPU hotplug controller that
> we're going to need anyway (if I understand correctly, at least).
If we don't have to 'park' hotplugged CPUs, then I don't see a need for
an extra controller.


> But:
> 
> > How about stealing reserved register from pci-host similar to your
> > extended TSEG commit (2f295167 q35/mch: implement extended TSEG sizes)?
> > (Looking into spec can (ab)use 0x58 or 0x59 register)  
> 
> Yes, that should work.
> 
> In fact, I had considered 0x58 / 0x59 when looking for unused registers
> for extended TSEG configuration:
> 
> http://mid.mail-archive.com/d8802612-0b11-776f-b209-53bbdaf67515@redhat.com
> 
> So I'm OK with this, thank you.
Thanks for the tip!
... patches with a stolen register are on the way to mail-list.

[staging/branch]: CdePkg - C Development Environment Package

[Minnow Ware]

[-- Attachment #1: Type: text/plain, Size: 2611 bytes --]

Hi UEFI community,

I’d like to introduce the CdePkg to edk2-staging.

The package is not yet completed but ready to demonstrate it’s power, probably also for modernFW.


A couple of years ago, after an UEFI BIOS project on AMD platform I decided to write my own ANSI C Library for UEFI Shell and POST.



My design goals were:

  1.  to rewrite the whole thing from scratch, without using any public source code from GNU, BSD, Watcom or Intel EDK2 / tiano core
  2.  completeness: full blown C90 + C95 support, no C99, no non-specified extensions at all , e.g. itoa(), stricmp()...
  3.  small code size, for UEFI-POST-driver uses a C-Library-Driver, that contains core/worker functions for realloc() ==  malloc() and free(),

entire printf-family, entire scanf-family.

UEFI-POST-driver just uses small wrapper functions to run the C-Library-Driver code.

  1.  stable, exact, chipset independent (w/o ACPI timer) "clock()” with CLOCKS_PER_SEC == 1000
  2.  complete set of the Microsoft C-compiler intrinsic functions
  3.  ROM-able! Runs with stack but w/o any static storage duration in .data segment, e.g. for rand(), strtok(), tmpfile()

This is required for early PEI before memory sizing, when PEI-images run directly out of flash.

  1.  Microsoft bug compatible (as far as possible)

     *   to save my lifetime writing a documentation  https://github.com/JoaquinConoBolillo/torito-C-Library/blob/master/implemented.md
     *   use original Microsoft header files for UEFI Shell Apps created in VS2017/19
     *   “debug”-mode for UEFI Shell executable in VS2017/19, that truly runs on Windows (that works

when using library functions only, no HW access, not UEFI-API use) to debug the library

itself – but this just links the same .OBJ module with the WinNT-EntryPoint instead of UEFI-EntryPoint

(The entry point module pulls in the appropriate OS-interface branch dispatcher)

  1.  all that in one single C-Library CdeLib.lib


The Readme.MD is here: https://github.com/MinnowWare/CdePkg#cdepkg

CdePkg shall be adjusted to other compilers/tool chains too, once it is feature complete and accepted by the UEFI community,
as long as it is for Microsoft VS2017/19 only.

The CdePkg is integrated into the “vUDK2018”-EDK2, which in turn runs in a MinnowBoard build.
It can be emulated in the Nt32Pkg, since EmulatorPkg in “vUDK2018” doesn’t support Windows…

I would like to move the “vUDK2018”-EDK2 to the edk2-staging branch CdePkg, but need to have access granted.

Can anyone kindly grant access rights to me?

Best Regards,
Kilian



[-- Attachment #2: Type: text/html, Size: 12701 bytes --]