On Mon, 9 Sep 2019 21:15:44 +0200
Laszlo Ersek <ler...@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 <imamm...@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(),
> > };
> >
> >
>
