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

["Igor Mammedov" <imammedo@redhat.com>]

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(),
> >  };
> >
> >  
>