[Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default

[Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default

[Gerd Hoffmann]

---
 hw/pci-host/q35.c         | 1 +
 include/hw/pci-host/q35.h | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 79bab15..9735db2 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -351,6 +351,7 @@ static void mch_reset(DeviceState *qdev)
                  MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT);
 
     d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
+    d->config[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_DEFAULT;
 
     mch_update(mch);
 }
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index e5d5a22..9704ccd 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -129,7 +129,6 @@ typedef struct Q35PCIHost {
 
 #define MCH_HOST_BRIDGE_SMRAM                  0x9d
 #define MCH_HOST_BRIDGE_SMRAM_SIZE             2
-#define MCH_HOST_BRIDGE_SMRAM_DEFAULT          ((uint8_t)0x2)
 #define MCH_HOST_BRIDGE_SMRAM_D_OPEN           ((uint8_t)(1 << 6))
 #define MCH_HOST_BRIDGE_SMRAM_D_CLS            ((uint8_t)(1 << 5))
 #define MCH_HOST_BRIDGE_SMRAM_D_LCK            ((uint8_t)(1 << 4))
@@ -140,6 +139,8 @@ typedef struct Q35PCIHost {
 #define MCH_HOST_BRIDGE_SMRAM_C_END            0xc0000
 #define MCH_HOST_BRIDGE_SMRAM_C_SIZE           0x20000
 #define MCH_HOST_BRIDGE_UPPER_SYSTEM_BIOS_END  0x100000
+#define MCH_HOST_BRIDGE_SMRAM_DEFAULT           \
+    MCH_HOST_BRIDGE_SMRAM_C_BASE_SEG
 
 #define MCH_HOST_BRIDGE_ESMRAMC                0x9e
 #define MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME       ((uint8_t)(1 << 7))
@@ -152,6 +153,10 @@ typedef struct Q35PCIHost {
 #define MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_2MB    ((uint8_t)(0x1 << 1))
 #define MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_8MB    ((uint8_t)(0x2 << 1))
 #define MCH_HOST_BRIDGE_ESMRAMC_T_EN           ((uint8_t)1)
+#define MCH_HOST_BRIDGE_ESMRAMC_DEFAULT \
+    (MCH_HOST_BRIDGE_ESMRAMC_SM_CACHE | \
+     MCH_HOST_BRIDGE_ESMRAMC_SM_L1 |    \
+     MCH_HOST_BRIDGE_ESMRAMC_SM_L2)
 
 /* D1:F0 PCIE* port*/
 #define MCH_PCIE_DEV                           1
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/6] add SMRAM+ESMRAMC wmask

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/q35.c         | 2 ++
 include/hw/pci-host/q35.h | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 9735db2..7093cc3 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -352,6 +352,8 @@ static void mch_reset(DeviceState *qdev)
 
     d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
     d->config[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_DEFAULT;
+    d->wmask[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_WMASK;
+    d->wmask[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_WMASK;
 
     mch_update(mch);
 }
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 9704ccd..82452c5 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -141,6 +141,11 @@ typedef struct Q35PCIHost {
 #define MCH_HOST_BRIDGE_UPPER_SYSTEM_BIOS_END  0x100000
 #define MCH_HOST_BRIDGE_SMRAM_DEFAULT           \
     MCH_HOST_BRIDGE_SMRAM_C_BASE_SEG
+#define MCH_HOST_BRIDGE_SMRAM_WMASK             \
+    (MCH_HOST_BRIDGE_SMRAM_D_OPEN |             \
+     MCH_HOST_BRIDGE_SMRAM_D_CLS |              \
+     MCH_HOST_BRIDGE_SMRAM_D_LCK |              \
+     MCH_HOST_BRIDGE_SMRAM_G_SMRAME)
 
 #define MCH_HOST_BRIDGE_ESMRAMC                0x9e
 #define MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME       ((uint8_t)(1 << 7))
@@ -157,6 +162,10 @@ typedef struct Q35PCIHost {
     (MCH_HOST_BRIDGE_ESMRAMC_SM_CACHE | \
      MCH_HOST_BRIDGE_ESMRAMC_SM_L1 |    \
      MCH_HOST_BRIDGE_ESMRAMC_SM_L2)
+#define MCH_HOST_BRIDGE_ESMRAMC_WMASK               \
+    (MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME |             \
+     MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK |         \
+     MCH_HOST_BRIDGE_ESMRAMC_T_EN)
 
 /* D1:F0 PCIE* port*/
 #define MCH_PCIE_DEV                           1
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/6] q35: implement SMRAM.D_LCK

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/q35.c         | 8 +++++++-
 include/hw/pci-host/q35.h | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 7093cc3..f0d840c 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -268,6 +268,13 @@ static void mch_update_smram(MCHPCIState *mch)
     PCIDevice *pd = PCI_DEVICE(mch);
     bool h_smrame = (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME);
 
+    /* implement SMRAM.D_LCK */
+    if (pd->config[MCH_HOST_BRIDGE_SMRAM] & MCH_HOST_BRIDGE_SMRAM_D_LCK) {
+        pd->config[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_D_OPEN;
+        pd->wmask[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_WMASK_LCK;
+        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_WMASK_LCK;
+    }
+
     memory_region_transaction_begin();
 
     if (pd->config[MCH_HOST_BRIDGE_SMRAM] & SMRAM_D_OPEN) {
@@ -297,7 +304,6 @@ static void mch_write_config(PCIDevice *d,
 {
     MCHPCIState *mch = MCH_PCI_DEVICE(d);
 
-    /* XXX: implement SMRAM.D_LOCK */
     pci_default_write_config(d, address, val, len);
 
     if (ranges_overlap(address, len, MCH_HOST_BRIDGE_PAM0,
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 82452c5..61bfe7e 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -146,6 +146,8 @@ typedef struct Q35PCIHost {
      MCH_HOST_BRIDGE_SMRAM_D_CLS |              \
      MCH_HOST_BRIDGE_SMRAM_D_LCK |              \
      MCH_HOST_BRIDGE_SMRAM_G_SMRAME)
+#define MCH_HOST_BRIDGE_SMRAM_WMASK_LCK         \
+    MCH_HOST_BRIDGE_SMRAM_D_CLS
 
 #define MCH_HOST_BRIDGE_ESMRAMC                0x9e
 #define MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME       ((uint8_t)(1 << 7))
@@ -166,6 +168,7 @@ typedef struct Q35PCIHost {
     (MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME |             \
      MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK |         \
      MCH_HOST_BRIDGE_ESMRAMC_T_EN)
+#define MCH_HOST_BRIDGE_ESMRAMC_WMASK_LCK     0
 
 /* D1:F0 PCIE* port*/
 #define MCH_PCIE_DEV                           1
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/6] q35: add test for SMRAM.D_LCK

[Gerd Hoffmann]

---
 tests/Makefile     |  2 ++
 tests/smram-test.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+)
 create mode 100644 tests/smram-test.c

diff --git a/tests/Makefile b/tests/Makefile
index 55aa745..cf2bd87 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -174,6 +174,7 @@ gcov-files-i386-y += hw/usb/dev-storage.c
 check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
 gcov-files-i386-y += hw/usb/hcd-xhci.c
 check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
+check-qtest-i386-y += tests/smram-test$(EXESUF)
 check-qtest-i386-$(CONFIG_LINUX) += tests/vhost-user-test$(EXESUF)
 check-qtest-x86_64-y = $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
@@ -365,6 +366,7 @@ tests/usb-hcd-uhci-test$(EXESUF): tests/usb-hcd-uhci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
 tests/pc-cpu-test$(EXESUF): tests/pc-cpu-test.o
+tests/smram-test$(EXESUF): tests/smram-test.o $(libqos-pc-obj-y)
 tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y)
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o libqemuutil.a libqemustub.a
diff --git a/tests/smram-test.c b/tests/smram-test.c
new file mode 100644
index 0000000..339d5d1
--- /dev/null
+++ b/tests/smram-test.c
@@ -0,0 +1,80 @@
+#include <glib.h>
+#include <string.h>
+#include "libqtest.h"
+#include "libqos/pci.h"
+#include "libqos/pci-pc.h"
+#include "qemu/osdep.h"
+#include "hw/pci-host/q35.h"
+
+static void smram_set_bit(QPCIDevice *pcidev, uint8_t mask, bool enabled)
+{
+    uint8_t smram;
+
+    smram = qpci_config_readb(pcidev, MCH_HOST_BRIDGE_SMRAM);
+    if (enabled) {
+        smram |= mask;
+    } else {
+        smram &= ~mask;
+    }
+    qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_SMRAM, smram);
+}
+
+static bool smram_test_bit(QPCIDevice *pcidev, uint8_t mask)
+{
+    uint8_t smram;
+
+    smram = qpci_config_readb(pcidev, MCH_HOST_BRIDGE_SMRAM);
+    return smram & mask;
+}
+
+static void test_smram_lock(void)
+{
+    QPCIBus *pcibus;
+    QPCIDevice *pcidev;
+    QDict *response;
+
+    pcibus = qpci_init_pc();
+    g_assert(pcibus != NULL);
+
+    pcidev = qpci_device_find(pcibus, 0);
+    g_assert(pcidev != NULL);
+
+    /* check open is settable */
+    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, false);
+    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == false);
+    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, true);
+    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == true);
+
+    /* lock, check open is cleared & not settable */
+    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_LCK, true);
+    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == false);
+    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, true);
+    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == false);
+
+    /* reset */
+    response = qmp("{'execute': 'system_reset', 'arguments': {} }");
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    /* check open is settable again */
+    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, false);
+    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == false);
+    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, true);
+    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == true);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("/smram/lock", test_smram_lock);
+
+    qtest_start("-M q35");
+    ret = g_test_run();
+    qtest_end();
+
+    return ret;
+}
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Gerd Hoffmann]

route access to tseg into nowhere when enabled,
for both cpus and busmaster dma.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/q35.c         | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci-host/q35.h |  1 +
 2 files changed, 58 insertions(+)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index f0d840c..412ff0a 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -198,6 +198,28 @@ static const TypeInfo q35_host_info = {
  * MCH D0:F0
  */
 
+static uint64_t tseg_blackhole_read(void *ptr, hwaddr reg, unsigned size)
+{
+    return 0xffffffff;
+}
+
+static void tseg_blackhole_write(void *opaque, hwaddr addr, uint64_t val,
+                                 unsigned width)
+{
+    /* nothing */
+}
+
+static const MemoryRegionOps tseg_blackhole_ops = {
+    .read = tseg_blackhole_read,
+    .write = tseg_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,
+};
+
 /* PCIe MMCFG */
 static void mch_update_pciexbar(MCHPCIState *mch)
 {
@@ -267,6 +289,7 @@ static void mch_update_smram(MCHPCIState *mch)
 {
     PCIDevice *pd = PCI_DEVICE(mch);
     bool h_smrame = (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME);
+    uint32_t tseg_size;
 
     /* implement SMRAM.D_LCK */
     if (pd->config[MCH_HOST_BRIDGE_SMRAM] & MCH_HOST_BRIDGE_SMRAM_D_LCK) {
@@ -296,6 +319,32 @@ static void mch_update_smram(MCHPCIState *mch)
         memory_region_set_enabled(&mch->high_smram, false);
     }
 
+    if (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_T_EN) {
+        switch (pd->config[MCH_HOST_BRIDGE_ESMRAMC] &
+                MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK) {
+        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_1MB:
+            tseg_size = 1024 * 1024;
+            break;
+        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_2MB:
+            tseg_size = 1024 * 1024 * 2;
+            break;
+        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_8MB:
+            tseg_size = 1024 * 1024 * 8;
+            break;
+        default:
+            tseg_size = 0;
+            break;
+        }
+    } else {
+        tseg_size = 0;
+    }
+    memory_region_del_subregion(mch->system_memory, &mch->tseg_blackhole);
+    memory_region_set_enabled(&mch->tseg_blackhole, tseg_size);
+    memory_region_set_size(&mch->tseg_blackhole, tseg_size);
+    memory_region_add_subregion_overlap(mch->system_memory,
+                                        mch->below_4g_mem_size - tseg_size,
+                                        &mch->tseg_blackhole, 1);
+
     memory_region_transaction_commit();
 }
 
@@ -443,6 +492,14 @@ static void mch_realize(PCIDevice *d, Error **errp)
     object_property_add_alias(qdev_get_machine(), "smram",
                               OBJECT(&mch->smram), ".", NULL);
 
+    memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch),
+                          &tseg_blackhole_ops, NULL,
+                          "tseg-blackhole", 0);
+    memory_region_set_enabled(&mch->tseg_blackhole, false);
+    memory_region_add_subregion_overlap(mch->system_memory,
+                                        mch->below_4g_mem_size,
+                                        &mch->tseg_blackhole, 1);
+
     init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
              mch->pci_address_space, &mch->pam_regions[0],
              PAM_BIOS_BASE, PAM_BIOS_SIZE);
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 61bfe7e..ba64c70 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -55,6 +55,7 @@ typedef struct MCHPCIState {
     PAMMemoryRegion pam_regions[13];
     MemoryRegion smram_region, open_high_smram;
     MemoryRegion smram, low_smram, high_smram;
+    MemoryRegion tseg_blackhole;
     PcPciInfo pci_info;
     ram_addr_t below_4g_mem_size;
     ram_addr_t above_4g_mem_size;
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested

[Gerd Hoffmann]

add tseg window to smram region, so cpus can access it in smm mode.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/q35.c         | 13 +++++++++++++
 include/hw/pci-host/q35.h |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 412ff0a..7d21399 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -345,6 +345,13 @@ static void mch_update_smram(MCHPCIState *mch)
                                         mch->below_4g_mem_size - tseg_size,
                                         &mch->tseg_blackhole, 1);
 
+    memory_region_set_enabled(&mch->tseg_window, tseg_size);
+    memory_region_set_size(&mch->tseg_window, tseg_size);
+    memory_region_set_address(&mch->tseg_window,
+                              mch->below_4g_mem_size - tseg_size);
+    memory_region_set_alias_offset(&mch->tseg_window,
+                                   mch->below_4g_mem_size - tseg_size);
+
     memory_region_transaction_commit();
 }
 
@@ -500,6 +507,12 @@ static void mch_realize(PCIDevice *d, Error **errp)
                                         mch->below_4g_mem_size,
                                         &mch->tseg_blackhole, 1);
 
+    memory_region_init_alias(&mch->tseg_window, OBJECT(mch), "tseg-window",
+                             mch->ram_memory, mch->below_4g_mem_size, 0);
+    memory_region_set_enabled(&mch->tseg_window, false);
+    memory_region_add_subregion(&mch->smram, mch->below_4g_mem_size,
+                                &mch->tseg_window);
+
     init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
              mch->pci_address_space, &mch->pam_regions[0],
              PAM_BIOS_BASE, PAM_BIOS_SIZE);
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index ba64c70..23b7700 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -55,7 +55,7 @@ typedef struct MCHPCIState {
     PAMMemoryRegion pam_regions[13];
     MemoryRegion smram_region, open_high_smram;
     MemoryRegion smram, low_smram, high_smram;
-    MemoryRegion tseg_blackhole;
+    MemoryRegion tseg_blackhole, tseg_window;
     PcPciInfo pci_info;
     ram_addr_t below_4g_mem_size;
     ram_addr_t above_4g_mem_size;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Paolo Bonzini]

On 20/04/2015 11:19, Gerd Hoffmann wrote:
> +    memory_region_del_subregion(mch->system_memory, &mch->tseg_blackhole);
> +    memory_region_set_enabled(&mch->tseg_blackhole, tseg_size);

Please use "tseg_size > 0" here.

Paolo

> +    memory_region_set_size(&mch->tseg_blackhole, tseg_size);
> +    memory_region_add_subregion_overlap(mch->system_memory,
> +                                        mch->below_4g_mem_size - tseg_size,
> +                                        &mch->tseg_blackhole, 1);

Re: [Qemu-devel] [PATCH 2/6] add SMRAM+ESMRAMC wmask

[Michael S. Tsirkin]

On Mon, Apr 20, 2015 at 11:19:16AM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

I would probably squash 2 and 3, to reduce the
chance of bisect related issues.

Otherwise

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


> ---
>  hw/pci-host/q35.c         | 2 ++
>  include/hw/pci-host/q35.h | 9 +++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 9735db2..7093cc3 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -352,6 +352,8 @@ static void mch_reset(DeviceState *qdev)
>  
>      d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
>      d->config[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_DEFAULT;
> +    d->wmask[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_WMASK;
> +    d->wmask[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_WMASK;
>  
>      mch_update(mch);
>  }
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index 9704ccd..82452c5 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -141,6 +141,11 @@ typedef struct Q35PCIHost {
>  #define MCH_HOST_BRIDGE_UPPER_SYSTEM_BIOS_END  0x100000
>  #define MCH_HOST_BRIDGE_SMRAM_DEFAULT           \
>      MCH_HOST_BRIDGE_SMRAM_C_BASE_SEG
> +#define MCH_HOST_BRIDGE_SMRAM_WMASK             \
> +    (MCH_HOST_BRIDGE_SMRAM_D_OPEN |             \
> +     MCH_HOST_BRIDGE_SMRAM_D_CLS |              \
> +     MCH_HOST_BRIDGE_SMRAM_D_LCK |              \
> +     MCH_HOST_BRIDGE_SMRAM_G_SMRAME)
>  
>  #define MCH_HOST_BRIDGE_ESMRAMC                0x9e
>  #define MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME       ((uint8_t)(1 << 7))
> @@ -157,6 +162,10 @@ typedef struct Q35PCIHost {
>      (MCH_HOST_BRIDGE_ESMRAMC_SM_CACHE | \
>       MCH_HOST_BRIDGE_ESMRAMC_SM_L1 |    \
>       MCH_HOST_BRIDGE_ESMRAMC_SM_L2)
> +#define MCH_HOST_BRIDGE_ESMRAMC_WMASK               \
> +    (MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME |             \
> +     MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK |         \
> +     MCH_HOST_BRIDGE_ESMRAMC_T_EN)
>  
>  /* D1:F0 PCIE* port*/
>  #define MCH_PCIE_DEV                           1
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 4/6] q35: add test for SMRAM.D_LCK

[Michael S. Tsirkin]

On Mon, Apr 20, 2015 at 11:19:18AM +0200, Gerd Hoffmann wrote:
> ---


signature is missing.

Besides that

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

>  tests/Makefile     |  2 ++
>  tests/smram-test.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+)
>  create mode 100644 tests/smram-test.c
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index 55aa745..cf2bd87 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -174,6 +174,7 @@ gcov-files-i386-y += hw/usb/dev-storage.c
>  check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
>  gcov-files-i386-y += hw/usb/hcd-xhci.c
>  check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
> +check-qtest-i386-y += tests/smram-test$(EXESUF)
>  check-qtest-i386-$(CONFIG_LINUX) += tests/vhost-user-test$(EXESUF)
>  check-qtest-x86_64-y = $(check-qtest-i386-y)
>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
> @@ -365,6 +366,7 @@ tests/usb-hcd-uhci-test$(EXESUF): tests/usb-hcd-uhci-test.o $(libqos-usb-obj-y)
>  tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
>  tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
>  tests/pc-cpu-test$(EXESUF): tests/pc-cpu-test.o
> +tests/smram-test$(EXESUF): tests/smram-test.o $(libqos-pc-obj-y)
>  tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y)
>  tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
>  tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o libqemuutil.a libqemustub.a
> diff --git a/tests/smram-test.c b/tests/smram-test.c
> new file mode 100644
> index 0000000..339d5d1
> --- /dev/null
> +++ b/tests/smram-test.c
> @@ -0,0 +1,80 @@
> +#include <glib.h>
> +#include <string.h>
> +#include "libqtest.h"
> +#include "libqos/pci.h"
> +#include "libqos/pci-pc.h"
> +#include "qemu/osdep.h"
> +#include "hw/pci-host/q35.h"
> +
> +static void smram_set_bit(QPCIDevice *pcidev, uint8_t mask, bool enabled)
> +{
> +    uint8_t smram;
> +
> +    smram = qpci_config_readb(pcidev, MCH_HOST_BRIDGE_SMRAM);
> +    if (enabled) {
> +        smram |= mask;
> +    } else {
> +        smram &= ~mask;
> +    }
> +    qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_SMRAM, smram);
> +}
> +
> +static bool smram_test_bit(QPCIDevice *pcidev, uint8_t mask)
> +{
> +    uint8_t smram;
> +
> +    smram = qpci_config_readb(pcidev, MCH_HOST_BRIDGE_SMRAM);
> +    return smram & mask;
> +}
> +
> +static void test_smram_lock(void)
> +{
> +    QPCIBus *pcibus;
> +    QPCIDevice *pcidev;
> +    QDict *response;
> +
> +    pcibus = qpci_init_pc();
> +    g_assert(pcibus != NULL);
> +
> +    pcidev = qpci_device_find(pcibus, 0);
> +    g_assert(pcidev != NULL);
> +
> +    /* check open is settable */
> +    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, false);
> +    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == false);
> +    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, true);
> +    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == true);
> +
> +    /* lock, check open is cleared & not settable */
> +    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_LCK, true);
> +    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == false);
> +    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, true);
> +    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == false);
> +
> +    /* reset */
> +    response = qmp("{'execute': 'system_reset', 'arguments': {} }");
> +    g_assert(response);
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +
> +    /* check open is settable again */
> +    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, false);
> +    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == false);
> +    smram_set_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN, true);
> +    g_assert(smram_test_bit(pcidev, MCH_HOST_BRIDGE_SMRAM_D_OPEN) == true);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    int ret;
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    qtest_add_func("/smram/lock", test_smram_lock);
> +
> +    qtest_start("-M q35");
> +    ret = g_test_run();
> +    qtest_end();
> +
> +    return ret;
> +}
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 3/6] q35: implement SMRAM.D_LCK

[Michael S. Tsirkin]

On Mon, Apr 20, 2015 at 11:19:17AM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci-host/q35.c         | 8 +++++++-
>  include/hw/pci-host/q35.h | 3 +++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 7093cc3..f0d840c 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -268,6 +268,13 @@ static void mch_update_smram(MCHPCIState *mch)
>      PCIDevice *pd = PCI_DEVICE(mch);
>      bool h_smrame = (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME);
>  
> +    /* implement SMRAM.D_LCK */
> +    if (pd->config[MCH_HOST_BRIDGE_SMRAM] & MCH_HOST_BRIDGE_SMRAM_D_LCK) {
> +        pd->config[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_D_OPEN;
> +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_WMASK_LCK;
> +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_WMASK_LCK;
> +    }
> +
>      memory_region_transaction_begin();
>  
>      if (pd->config[MCH_HOST_BRIDGE_SMRAM] & SMRAM_D_OPEN) {
> @@ -297,7 +304,6 @@ static void mch_write_config(PCIDevice *d,
>  {
>      MCHPCIState *mch = MCH_PCI_DEVICE(d);
>  
> -    /* XXX: implement SMRAM.D_LOCK */
>      pci_default_write_config(d, address, val, len);
>  
>      if (ranges_overlap(address, len, MCH_HOST_BRIDGE_PAM0,
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index 82452c5..61bfe7e 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -146,6 +146,8 @@ typedef struct Q35PCIHost {
>       MCH_HOST_BRIDGE_SMRAM_D_CLS |              \
>       MCH_HOST_BRIDGE_SMRAM_D_LCK |              \
>       MCH_HOST_BRIDGE_SMRAM_G_SMRAME)
> +#define MCH_HOST_BRIDGE_SMRAM_WMASK_LCK         \
> +    MCH_HOST_BRIDGE_SMRAM_D_CLS
>  
>  #define MCH_HOST_BRIDGE_ESMRAMC                0x9e
>  #define MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME       ((uint8_t)(1 << 7))
> @@ -166,6 +168,7 @@ typedef struct Q35PCIHost {
>      (MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME |             \
>       MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK |         \
>       MCH_HOST_BRIDGE_ESMRAMC_T_EN)
> +#define MCH_HOST_BRIDGE_ESMRAMC_WMASK_LCK     0
>  
>  /* D1:F0 PCIE* port*/
>  #define MCH_PCIE_DEV                           1
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default

[Michael S. Tsirkin]

On Mon, Apr 20, 2015 at 11:19:15AM +0200, Gerd Hoffmann wrote:
> ---

signature is missing.
And it might be a good idea to add a cover letter,
stick q35: in subject for patches, and add a
bit of description in the commit log.

Besides that

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>




>  hw/pci-host/q35.c         | 1 +
>  include/hw/pci-host/q35.h | 7 ++++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 79bab15..9735db2 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -351,6 +351,7 @@ static void mch_reset(DeviceState *qdev)
>                   MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT);
>  
>      d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
> +    d->config[MCH_HOST_BRIDGE_ESMRAMC] = MCH_HOST_BRIDGE_ESMRAMC_DEFAULT;
>  
>      mch_update(mch);
>  }
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index e5d5a22..9704ccd 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -129,7 +129,6 @@ typedef struct Q35PCIHost {
>  
>  #define MCH_HOST_BRIDGE_SMRAM                  0x9d
>  #define MCH_HOST_BRIDGE_SMRAM_SIZE             2
> -#define MCH_HOST_BRIDGE_SMRAM_DEFAULT          ((uint8_t)0x2)
>  #define MCH_HOST_BRIDGE_SMRAM_D_OPEN           ((uint8_t)(1 << 6))
>  #define MCH_HOST_BRIDGE_SMRAM_D_CLS            ((uint8_t)(1 << 5))
>  #define MCH_HOST_BRIDGE_SMRAM_D_LCK            ((uint8_t)(1 << 4))
> @@ -140,6 +139,8 @@ typedef struct Q35PCIHost {
>  #define MCH_HOST_BRIDGE_SMRAM_C_END            0xc0000
>  #define MCH_HOST_BRIDGE_SMRAM_C_SIZE           0x20000
>  #define MCH_HOST_BRIDGE_UPPER_SYSTEM_BIOS_END  0x100000
> +#define MCH_HOST_BRIDGE_SMRAM_DEFAULT           \
> +    MCH_HOST_BRIDGE_SMRAM_C_BASE_SEG
>  
>  #define MCH_HOST_BRIDGE_ESMRAMC                0x9e
>  #define MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME       ((uint8_t)(1 << 7))
> @@ -152,6 +153,10 @@ typedef struct Q35PCIHost {
>  #define MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_2MB    ((uint8_t)(0x1 << 1))
>  #define MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_8MB    ((uint8_t)(0x2 << 1))
>  #define MCH_HOST_BRIDGE_ESMRAMC_T_EN           ((uint8_t)1)
> +#define MCH_HOST_BRIDGE_ESMRAMC_DEFAULT \
> +    (MCH_HOST_BRIDGE_ESMRAMC_SM_CACHE | \
> +     MCH_HOST_BRIDGE_ESMRAMC_SM_L1 |    \
> +     MCH_HOST_BRIDGE_ESMRAMC_SM_L2)
>  
>  /* D1:F0 PCIE* port*/
>  #define MCH_PCIE_DEV                           1
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default

[Paolo Bonzini]

On 20/04/2015 14:07, Michael S. Tsirkin wrote:
> signature is missing.
> And it might be a good idea to add a cover letter,
> stick q35: in subject for patches, and add a
> bit of description in the commit log.

The patches as they are do not apply to master.  I expect Gerd to send
the final versions to me privately, and then I can post the joint work
to the mailing list for review.

Paolo

Re: [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default

[Gerd Hoffmann]

On Mo, 2015-04-20 at 14:27 +0200, Paolo Bonzini wrote:
> 
> On 20/04/2015 14:07, Michael S. Tsirkin wrote:
> > signature is missing.
> > And it might be a good idea to add a cover letter,
> > stick q35: in subject for patches, and add a
> > bit of description in the commit log.
> 
> The patches as they are do not apply to master.  I expect Gerd to send
> the final versions to me privately, and then I can post the joint work
> to the mailing list for review.

Yes, thats why I'm a bit sloppy with the commit messages atm, they build
on top of paolos smm branch and I've mostly sent them out for early
review (some memory api details are new to me) and for laszlo to play
with.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Laszlo Ersek]

In general, commit messages for the series would be appreciated by the
uninitiated :)

Then,

On 04/20/15 11:19, Gerd Hoffmann wrote:
> route access to tseg into nowhere when enabled,
> for both cpus and busmaster dma.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci-host/q35.c         | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci-host/q35.h |  1 +
>  2 files changed, 58 insertions(+)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index f0d840c..412ff0a 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -198,6 +198,28 @@ static const TypeInfo q35_host_info = {
>   * MCH D0:F0
>   */
>  
> +static uint64_t tseg_blackhole_read(void *ptr, hwaddr reg, unsigned size)
> +{
> +    return 0xffffffff;
> +}
> +
> +static void tseg_blackhole_write(void *opaque, hwaddr addr, uint64_t val,
> +                                 unsigned width)
> +{
> +    /* nothing */
> +}
> +
> +static const MemoryRegionOps tseg_blackhole_ops = {
> +    .read = tseg_blackhole_read,
> +    .write = tseg_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,
> +};

(a) you've specified .endianness twice (with inconsistent initializers
at that, although the value returned by the accessor happens to be
unaffected).

(b) Why are 8-byte accesses forbidden? ... Hm, actually, the above
doesn't forbid it, it just causes qemu to split up the access. Should be
fine.

> +
>  /* PCIe MMCFG */
>  static void mch_update_pciexbar(MCHPCIState *mch)
>  {
> @@ -267,6 +289,7 @@ static void mch_update_smram(MCHPCIState *mch)
>  {
>      PCIDevice *pd = PCI_DEVICE(mch);
>      bool h_smrame = (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME);
> +    uint32_t tseg_size;
>  
>      /* implement SMRAM.D_LCK */
>      if (pd->config[MCH_HOST_BRIDGE_SMRAM] & MCH_HOST_BRIDGE_SMRAM_D_LCK) {
> @@ -296,6 +319,32 @@ static void mch_update_smram(MCHPCIState *mch)
>          memory_region_set_enabled(&mch->high_smram, false);
>      }
>  
> +    if (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_T_EN) {
> +        switch (pd->config[MCH_HOST_BRIDGE_ESMRAMC] &
> +                MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK) {
> +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_1MB:
> +            tseg_size = 1024 * 1024;
> +            break;
> +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_2MB:
> +            tseg_size = 1024 * 1024 * 2;
> +            break;
> +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_8MB:
> +            tseg_size = 1024 * 1024 * 8;
> +            break;
> +        default:
> +            tseg_size = 0;
> +            break;
> +        }
> +    } else {
> +        tseg_size = 0;
> +    }

- so, is the guest supposed to select the tseg size by writing this
register?

- I assume this register is not reprogrammable once SMRAM is locked --
is that correct?

- can the guest somehow use this facility to detect, dynamically, the
presence of this feature? (For now I'm thinking about a static build
flag for OVMF that would enable SMM support, but I'm 99% sure Jordan
will object and ask for a dynamic feature detection.)

> +    memory_region_del_subregion(mch->system_memory, &mch->tseg_blackhole);

Hmm. I think this API is for something else. It is not for hole punching.

... Ah, okay, you *are* removing the previously configured blackhole
region (ie. you're not hole punching), I see. But, does this work if
tseg_blackhole is still all zeroes? (Ie. this is the first invocation.)

> +    memory_region_set_enabled(&mch->tseg_blackhole, tseg_size);
> +    memory_region_set_size(&mch->tseg_blackhole, tseg_size);
> +    memory_region_add_subregion_overlap(mch->system_memory,
> +                                        mch->below_4g_mem_size - tseg_size,
> +                                        &mch->tseg_blackhole, 1);

So, does tseg overlay the last few MB of the RAM below 4GB? (Ie. right
before the PCI hole starts?)

> +
>      memory_region_transaction_commit();
>  }
>  
> @@ -443,6 +492,14 @@ static void mch_realize(PCIDevice *d, Error **errp)
>      object_property_add_alias(qdev_get_machine(), "smram",
>                                OBJECT(&mch->smram), ".", NULL);
>  
> +    memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch),
> +                          &tseg_blackhole_ops, NULL,
> +                          "tseg-blackhole", 0);

Okay, this answers one of the above, tseg_blackhole is never uninitialized.

> +    memory_region_set_enabled(&mch->tseg_blackhole, false);
> +    memory_region_add_subregion_overlap(mch->system_memory,
> +                                        mch->below_4g_mem_size,
> +                                        &mch->tseg_blackhole, 1);

The address (2nd argument) is inconsistent with the one in
mch_update_smram() -- this function call places the tseg black hole at
the beginning of the 32-bit PCI hole, not just below it.

(OTOH it doesn't really matter, because the region is disabled. Still,
it would be easier to understand.)

... I'm thinking loud about what this means for the OVMF memory space
map... The central function we use in PlatformPei is
GetSystemMemorySizeBelow4gb(), which reads the CMOS (0x34/0x35). That
gives us "below_4g_mem_size" in OVMF.

Currently that value is used for three purposes:
(a) place the permanent PEI RAM so that it ends at the top of
"below_4g_mem_size"; PublishPeiMemory()
(b) create one of the memory HOBs, QemuInitializeRam(),
(c) create the MMIO HOB that describes the 32-bit PCI hole,
    MemMapInitialization()

If the last 1 / 2 / 8 MB of the low RAM can't be used as general purpose
RAM, then (a) is affected (it should simply be sunk by the tseg size of
choice); (b) is affected similarly (just decrease the top of the memory
HOB); (c) is not affected (the start of the low PCI hole remains
unchanged). Okay...

> +
>      init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
>               mch->pci_address_space, &mch->pam_regions[0],
>               PAM_BIOS_BASE, PAM_BIOS_SIZE);
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index 61bfe7e..ba64c70 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -55,6 +55,7 @@ typedef struct MCHPCIState {
>      PAMMemoryRegion pam_regions[13];
>      MemoryRegion smram_region, open_high_smram;
>      MemoryRegion smram, low_smram, high_smram;
> +    MemoryRegion tseg_blackhole;
>      PcPciInfo pci_info;
>      ram_addr_t below_4g_mem_size;
>      ram_addr_t above_4g_mem_size;
> 

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested

[Laszlo Ersek]

On 04/20/15 11:19, Gerd Hoffmann wrote:
> add tseg window to smram region, so cpus can access it in smm mode.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci-host/q35.c         | 13 +++++++++++++
>  include/hw/pci-host/q35.h |  2 +-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 412ff0a..7d21399 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -345,6 +345,13 @@ static void mch_update_smram(MCHPCIState *mch)
>                                          mch->below_4g_mem_size - tseg_size,
>                                          &mch->tseg_blackhole, 1);
>  
> +    memory_region_set_enabled(&mch->tseg_window, tseg_size);
> +    memory_region_set_size(&mch->tseg_window, tseg_size);
> +    memory_region_set_address(&mch->tseg_window,
> +                              mch->below_4g_mem_size - tseg_size);
> +    memory_region_set_alias_offset(&mch->tseg_window,
> +                                   mch->below_4g_mem_size - tseg_size);
> +
>      memory_region_transaction_commit();
>  }
>  
> @@ -500,6 +507,12 @@ static void mch_realize(PCIDevice *d, Error **errp)
>                                          mch->below_4g_mem_size,
>                                          &mch->tseg_blackhole, 1);
>  
> +    memory_region_init_alias(&mch->tseg_window, OBJECT(mch), "tseg-window",
> +                             mch->ram_memory, mch->below_4g_mem_size, 0);
> +    memory_region_set_enabled(&mch->tseg_window, false);
> +    memory_region_add_subregion(&mch->smram, mch->below_4g_mem_size,
> +                                &mch->tseg_window);
> +
>      init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
>               mch->pci_address_space, &mch->pam_regions[0],
>               PAM_BIOS_BASE, PAM_BIOS_SIZE);
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index ba64c70..23b7700 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -55,7 +55,7 @@ typedef struct MCHPCIState {
>      PAMMemoryRegion pam_regions[13];
>      MemoryRegion smram_region, open_high_smram;
>      MemoryRegion smram, low_smram, high_smram;
> -    MemoryRegion tseg_blackhole;
> +    MemoryRegion tseg_blackhole, tseg_window;
>      PcPciInfo pci_info;
>      ram_addr_t below_4g_mem_size;
>      ram_addr_t above_4g_mem_size;
> 

Why is this necessary? If you disable the black hole overlay, the access
will go to the RAM. (Or can't that be done per-CPU?)

I'm thinking, the last 1 / 2 / 8 megabytes should behave as RAM in all
of the following cases:
- no SMRAM programmed (tseg size = 0)
- SMRAM programmed (tseg size > 0), and it is open
- SMRAM programmed (tseg size > 0) and closed, but CPU in SMM

Does any of the above require anything else than simply disabling the
black hole overlay? (Sorry if I'm missing something obvious!) Assuming
that a lockdown prevents the reprogramming of tseg size, I think the
above could all be unified.

... Another question, related to SMM (but not related to SMRAM): Paolo,
am I right to think that we'll be keying off at least two independent
things of SMM-or-not: one is access to SMRAM (tseg), for LockBox and SMM
driver purposes, the other is pflash access (with the MemTxAttrs thing),
for the varstore?

(BTW in the meantime I found out about
EFI_SMM_FIRMWARE_VOLUME_BLOCK_PROTOCOL too, so at least in *theory* it
is clear what has to be done with / for the flash driver.)

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested

[Paolo Bonzini]

On 21/04/2015 16:30, Laszlo Ersek wrote:
>> > -    MemoryRegion tseg_blackhole;
>> > +    MemoryRegion tseg_blackhole, tseg_window;
>> >      PcPciInfo pci_info;
>> >      ram_addr_t below_4g_mem_size;
>> >      ram_addr_t above_4g_mem_size;
>> > 
> Why is this necessary? If you disable the black hole overlay, the access
> will go to the RAM. (Or can't that be done per-CPU?)

The reason to have two separate MemoryRegions is exactly to allow
per-CPU access.

tseg_blackhole is added on top of address_space_memory to hide TSEG;
tseg_window is included in /machine/smram and TCG adds it to the private
per-CPU address space when it enters system management mode.

> I'm thinking, the last 1 / 2 / 8 megabytes should behave as RAM in all
> of the following cases:
> - no SMRAM programmed (tseg size = 0)
> - SMRAM programmed (tseg size > 0), and it is open
> - SMRAM programmed (tseg size > 0) and closed, but CPU in SMM

Correct.  However, you can have one CPU in SMM and another executing
"normal" code.  It would be a hole to allow that CPU to read (or worse,
write) the TSEG or legacy SMRAM areas.

> ... Another question, related to SMM (but not related to SMRAM): Paolo,
> am I right to think that we'll be keying off at least two independent
> things of SMM-or-not: one is access to SMRAM (tseg), for LockBox and SMM
> driver purposes, the other is pflash access (with the MemTxAttrs thing),
> for the varstore?

Yes.

Paolo

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Gerd Hoffmann]

> > +static const MemoryRegionOps tseg_blackhole_ops = {
> > +    .read = tseg_blackhole_read,
> > +    .write = tseg_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,
> > +};
> 
> (a) you've specified .endianness twice (with inconsistent initializers
> at that, although the value returned by the accessor happens to be
> unaffected).

Oops, cut+paste bug, will fix (/me wonders why gcc didn't throw an error
on that one).

> (b) Why are 8-byte accesses forbidden? ...

just a matter of setting .valid.max_access_size = 8, can do that of
course.

> > +
> >  /* PCIe MMCFG */
> >  static void mch_update_pciexbar(MCHPCIState *mch)
> >  {
> > @@ -267,6 +289,7 @@ static void mch_update_smram(MCHPCIState *mch)
> >  {
> >      PCIDevice *pd = PCI_DEVICE(mch);
> >      bool h_smrame = (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME);
> > +    uint32_t tseg_size;
> >  
> >      /* implement SMRAM.D_LCK */
> >      if (pd->config[MCH_HOST_BRIDGE_SMRAM] & MCH_HOST_BRIDGE_SMRAM_D_LCK) {
> > @@ -296,6 +319,32 @@ static void mch_update_smram(MCHPCIState *mch)
> >          memory_region_set_enabled(&mch->high_smram, false);
> >      }
> >  
> > +    if (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_T_EN) {
> > +        switch (pd->config[MCH_HOST_BRIDGE_ESMRAMC] &
> > +                MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK) {
> > +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_1MB:
> > +            tseg_size = 1024 * 1024;
> > +            break;
> > +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_2MB:
> > +            tseg_size = 1024 * 1024 * 2;
> > +            break;
> > +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_8MB:
> > +            tseg_size = 1024 * 1024 * 8;
> > +            break;
> > +        default:
> > +            tseg_size = 0;
> > +            break;
> > +        }
> > +    } else {
> > +        tseg_size = 0;
> > +    }
> 
> - so, is the guest supposed to select the tseg size by writing this
> register?

Correct.

> - I assume this register is not reprogrammable once SMRAM is locked --
> is that correct?

Correct.

> - can the guest somehow use this facility to detect, dynamically, the
> presence of this feature? (For now I'm thinking about a static build
> flag for OVMF that would enable SMM support, but I'm 99% sure Jordan
> will object and ask for a dynamic feature detection.)

Hmm.  I think if we merge all the smm / smram / tseg patches in one go
this should work.  Without patches reading the ESMRAMC register returns
zero.  With the patches the three cache-disable bits are hardcoded to
'1'.  This should work for detecting tseg support.

You also have to test for smm support.  The current protocol for this is
that seabios checks whenever smm is already initialized (see
*_apmc_smm_setup() in seabios/src/fw/smm.c) and if so it skips smm
initialization.  Right now qemu sets the bit on reset when running on
kvm, so seabios doesn't try to use smm.  On tcg the bit is clear after
reset and seabios actually uses smm mode.

> > +    memory_region_set_enabled(&mch->tseg_blackhole, tseg_size);
> > +    memory_region_set_size(&mch->tseg_blackhole, tseg_size);
> > +    memory_region_add_subregion_overlap(mch->system_memory,
> > +                                        mch->below_4g_mem_size - tseg_size,
> > +                                        &mch->tseg_blackhole, 1);
> 
> So, does tseg overlay the last few MB of the RAM below 4GB? (Ie. right
> before the PCI hole starts?)

tseg is just normal ram (yes, located at the end of memory), but (once
tseg is enabled) only cpus in smm mode are allowed to access it.
Likewise busmaster dma access is rejected, so non-smm code can't use the
sata controller to access this indirectly.

> > +    memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch),
> > +                          &tseg_blackhole_ops, NULL,
> > +                          "tseg-blackhole", 0);
> 
> Okay, this answers one of the above, tseg_blackhole is never uninitialized.
> 
> > +    memory_region_set_enabled(&mch->tseg_blackhole, false);
> > +    memory_region_add_subregion_overlap(mch->system_memory,
> > +                                        mch->below_4g_mem_size,
> > +                                        &mch->tseg_blackhole, 1);
> 
> The address (2nd argument) is inconsistent with the one in
> mch_update_smram() -- this function call places the tseg black hole at
> the beginning of the 32-bit PCI hole, not just below it.

It's a zero-length hole here, therefore it actually is the same.  But I
didn't bother to use "mch->below_4g_mem_size - 0" as start address to
make that more clear ;)

> (a) place the permanent PEI RAM so that it ends at the top of
> "below_4g_mem_size"; PublishPeiMemory()
> (b) create one of the memory HOBs, QemuInitializeRam(),
> (c) create the MMIO HOB that describes the 32-bit PCI hole,
>     MemMapInitialization()
> 
> If the last 1 / 2 / 8 MB of the low RAM can't be used as general purpose
> RAM, then (a) is affected (it should simply be sunk by the tseg size of
> choice); (b) is affected similarly (just decrease the top of the memory
> HOB);

Yes.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested

[Laszlo Ersek]

On 04/21/15 16:38, Paolo Bonzini wrote:
> 
> 
> On 21/04/2015 16:30, Laszlo Ersek wrote:
>>>> -    MemoryRegion tseg_blackhole;
>>>> +    MemoryRegion tseg_blackhole, tseg_window;
>>>>      PcPciInfo pci_info;
>>>>      ram_addr_t below_4g_mem_size;
>>>>      ram_addr_t above_4g_mem_size;
>>>>
>> Why is this necessary? If you disable the black hole overlay, the access
>> will go to the RAM. (Or can't that be done per-CPU?)
> 
> The reason to have two separate MemoryRegions is exactly to allow
> per-CPU access.
> 
> tseg_blackhole is added on top of address_space_memory to hide TSEG;
> tseg_window is included in /machine/smram and TCG adds it to the private
> per-CPU address space when it enters system management mode.

Hm, I must have missed this (or not seen it at all) -- should I have
noticed it in Gerd's series somewhere (or in yours)? Or is that by
virtue of mapping mch->tseg_window as a subregion of mch->smram?

(These overlays are pretty confusing, without a graphical visualization :))

>> I'm thinking, the last 1 / 2 / 8 megabytes should behave as RAM in all
>> of the following cases:
>> - no SMRAM programmed (tseg size = 0)
>> - SMRAM programmed (tseg size > 0), and it is open
>> - SMRAM programmed (tseg size > 0) and closed, but CPU in SMM
> 
> Correct.  However, you can have one CPU in SMM and another executing
> "normal" code.  It would be a hole to allow that CPU to read (or worse,
> write) the TSEG or legacy SMRAM areas.
> 
>> ... Another question, related to SMM (but not related to SMRAM): Paolo,
>> am I right to think that we'll be keying off at least two independent
>> things of SMM-or-not: one is access to SMRAM (tseg), for LockBox and SMM
>> driver purposes, the other is pflash access (with the MemTxAttrs thing),
>> for the varstore?
> 
> Yes.

Great, thank you.

Yet another question -- as far as I understand, I should have enough
info (with my pending questions of course) for EFI_SMM_ACCESS2_PROTOCOL.
I've now reviewed EFI_SMM_CONTROL2_PROTOCOL too, and AFAICS the only
thing I need to know for it is "how to raise an SMI, synchronously".
What are the plans for that? An ioport write perhaps? (I skimmed the
ICH9 spec, but whatever I found seemed to be quite inappropriate.)

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Paolo Bonzini]

On 21/04/2015 17:04, Gerd Hoffmann wrote:
> Hmm.  I think if we merge all the smm / smram / tseg patches in one go
> this should work.  Without patches reading the ESMRAMC register returns
> zero.  With the patches the three cache-disable bits are hardcoded to
> '1'.  This should work for detecting tseg support.
> 
> You also have to test for smm support.  The current protocol for this is
> that seabios checks whenever smm is already initialized (see
> *_apmc_smm_setup() in seabios/src/fw/smm.c) and if so it skips smm
> initialization.  Right now qemu sets the bit on reset when running on
> kvm, so seabios doesn't try to use smm.  On tcg the bit is clear after
> reset and seabios actually uses smm mode.

BTW, I plan to add "-machine smm=yes/no/auto".

Gerd, are you going to implement SMI_EN locking as well?

Paolo

Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested

[Gerd Hoffmann]

  Hi,

> > I'm thinking, the last 1 / 2 / 8 megabytes should behave as RAM in all
> > of the following cases:
> > - no SMRAM programmed (tseg size = 0)
> > - SMRAM programmed (tseg size > 0), and it is open
> > - SMRAM programmed (tseg size > 0) and closed, but CPU in SMM
> 
> Correct.

Almost.  I think the smram open bit doesn't affect tseg at all.  As tseg
doesn't have the funky overlay properties with the vga window you can
simply initialize tseg memory outside smm mode without any special
tricks, you just need to do it before flipping the tseg enable bit.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested

[Gerd Hoffmann]

> > tseg_blackhole is added on top of address_space_memory to hide TSEG;
> > tseg_window is included in /machine/smram and TCG adds it to the private
> > per-CPU address space when it enters system management mode.
> 
> Hm, I must have missed this (or not seen it at all) -- should I have
> noticed it in Gerd's series somewhere (or in yours)? Or is that by
> virtue of mapping mch->tseg_window as a subregion of mch->smram?

Yes.  mch->smram holds the memory regions accessible in smm mode only.

> (These overlays are pretty confusing, without a graphical visualization :))

Try 'info mtree'.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Gerd Hoffmann]

On Di, 2015-04-21 at 17:08 +0200, Paolo Bonzini wrote:
> 
> On 21/04/2015 17:04, Gerd Hoffmann wrote:
> > Hmm.  I think if we merge all the smm / smram / tseg patches in one go
> > this should work.  Without patches reading the ESMRAMC register returns
> > zero.  With the patches the three cache-disable bits are hardcoded to
> > '1'.  This should work for detecting tseg support.
> > 
> > You also have to test for smm support.  The current protocol for this is
> > that seabios checks whenever smm is already initialized (see
> > *_apmc_smm_setup() in seabios/src/fw/smm.c) and if so it skips smm
> > initialization.  Right now qemu sets the bit on reset when running on
> > kvm, so seabios doesn't try to use smm.  On tcg the bit is clear after
> > reset and seabios actually uses smm mode.
> 
> BTW, I plan to add "-machine smm=yes/no/auto".
> 
> Gerd, are you going to implement SMI_EN locking as well?

Sure, can do that while being at it.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested

[Paolo Bonzini]

On 21/04/2015 17:05, Laszlo Ersek wrote:
> 
> Yet another question -- as far as I understand, I should have enough
> info (with my pending questions of course) for EFI_SMM_ACCESS2_PROTOCOL.
> I've now reviewed EFI_SMM_CONTROL2_PROTOCOL too, and AFAICS the only
> thing I need to know for it is "how to raise an SMI, synchronously".
> What are the plans for that? An ioport write perhaps? (I skimmed the
> ICH9 spec, but whatever I found seemed to be quite inappropriate.)

You can write to ioport 0xb2 in order to raise the SMI, or you can also
write to the APIC ICR register with LOCAL_APIC_DELIVERY_MODE_SMI.  I am
not sure if the latter is synchronous.

If you use the former, it probably should be protected by some kind of
spinlock (I don't know the details of UEFI multi tasking) and you also
have to set the APMC_EN and GBL_SMI_EN bits of the SMI_EN register.

Paolo

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Laszlo Ersek]

On 04/21/15 17:04, Gerd Hoffmann wrote:
>>> +static const MemoryRegionOps tseg_blackhole_ops = {
>>> +    .read = tseg_blackhole_read,
>>> +    .write = tseg_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,
>>> +};
>>
>> (a) you've specified .endianness twice (with inconsistent initializers
>> at that, although the value returned by the accessor happens to be
>> unaffected).
> 
> Oops, cut+paste bug, will fix (/me wonders why gcc didn't throw an error
> on that one).

I think because it's valid C99. The "current object" concept is just
moved by the designation, and you can initialize the "current object".

>> - can the guest somehow use this facility to detect, dynamically, the
>> presence of this feature? (For now I'm thinking about a static build
>> flag for OVMF that would enable SMM support, but I'm 99% sure Jordan
>> will object and ask for a dynamic feature detection.)
> 
> Hmm.  I think if we merge all the smm / smram / tseg patches in one go
> this should work.  Without patches reading the ESMRAMC register returns
> zero.  With the patches the three cache-disable bits are hardcoded to
> '1'.  This should work for detecting tseg support.
> 
> You also have to test for smm support.  The current protocol for this is
> that seabios checks whenever smm is already initialized (see
> *_apmc_smm_setup() in seabios/src/fw/smm.c) and if so it skips smm
> initialization.  Right now qemu sets the bit on reset when running on
> kvm, so seabios doesn't try to use smm.  On tcg the bit is clear after
> reset and seabios actually uses smm mode.

Thanks. I'll stash the rest of this thread for later.

A "unified" patchset (not necessarily posted, just pushed somewhere)
would be helpful down the road -- IIRC this one was claimed unapplicable
to current master. (Which is also why I didn't try the "info mtree"
command, see elsewhere in the thread -- I didn't try to build the series.)

Thanks!
Laszlo

[Qemu-devel] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested)

[Laszlo Ersek]

adding edk2-devel

On 04/21/15 17:21, Paolo Bonzini wrote:
> 
> 
> On 21/04/2015 17:05, Laszlo Ersek wrote:
>>
>> Yet another question -- as far as I understand, I should have enough
>> info (with my pending questions of course) for EFI_SMM_ACCESS2_PROTOCOL.
>> I've now reviewed EFI_SMM_CONTROL2_PROTOCOL too, and AFAICS the only
>> thing I need to know for it is "how to raise an SMI, synchronously".
>> What are the plans for that? An ioport write perhaps? (I skimmed the
>> ICH9 spec, but whatever I found seemed to be quite inappropriate.)
> 
> You can write to ioport 0xb2 in order to raise the SMI, or you can also
> write to the APIC ICR register with LOCAL_APIC_DELIVERY_MODE_SMI.  I am
> not sure if the latter is synchronous.
> 
> If you use the former, it probably should be protected by some kind of
> spinlock (I don't know the details of UEFI multi tasking) and you also
> have to set the APMC_EN and GBL_SMI_EN bits of the SMI_EN register.

This raises a number of questions. I'm hoping to get help for the below
from edk2-devel subscribers. :)

(1) About locking. "MdePkg/Library/UefiLib/UefiLib.c" in edk2 has a pair
of functions called EfiAcquireLock() and EfiReleaseLock() -- and some
variation too.

I'm not really sure why these functions (and the underlying data
structure, EFI_LOCK) exist.

------------------------------
typedef enum {
  EfiLockUninitialized = 0,
  EfiLockReleased      = 1,
  EfiLockAcquired      = 2
} EFI_LOCK_STATE;

typedef struct {
  EFI_TPL         Tpl;
  EFI_TPL         OwnerTpl;
  EFI_LOCK_STATE  Lock;
} EFI_LOCK;

VOID
EFIAPI
EfiAcquireLock (
  IN EFI_LOCK  *Lock
  )
{
  ASSERT (Lock != NULL);
  ASSERT (Lock->Lock == EfiLockReleased);

  Lock->OwnerTpl = gBS->RaiseTPL (Lock->Tpl);
  Lock->Lock     = EfiLockAcquired;
}

VOID
EFIAPI
EfiReleaseLock (
  IN EFI_LOCK  *Lock
  )
{
  EFI_TPL Tpl;

  ASSERT (Lock != NULL);
  ASSERT (Lock->Lock == EfiLockAcquired);

  Tpl = Lock->OwnerTpl;

  Lock->Lock = EfiLockReleased;

  gBS->RestoreTPL (Tpl);
}
------------------------------

I understand the stashing of the old TPL (task priority level) for
restoration. I understand that the lock object itself carries new (ie.
masked) TPL as well. The lock state is apparently only used for
enforcing symmetry between locking and unlocking.

But the prototypes of these functions are very misleading. They imply
that you can take separate locks, and that locking one will not prevent
locking another. This is false. There's only one task priority level,
and raising the TPL ensures that various registered callbacks
(TPL_CALLBACK) and asynch notifications (TPL_NOTIFY: in practice always
running in the context of the timer interrupt handler) are masked.

So, if you have two EFI_LOCK objects, Lock1 and Lock2, and Lock1.Tpl ==
TPL_CALLBACK, and Lock2.Tpl == TPL_NOTIFY, and you take Lock2 first,
then an attempt to lock the seemingly independent Lock1 (whose TPL is
TPL_CALLBACK, which is lower than the current TPL, TPL_NOTIFY) will lead
to indeterminate system state. See the description of the RaiseTPL()
boot service:

    If NewTpl is below the current TPL level, then the system behavior
    is indeterminate.

So, I really don't understand why these UefiLib functions exist at all;
to me they seem more confusing than useful.

(2) Again, about locking. Assuming someone explains the above functions
to me, or I just use naked gBS->RaiseTPL() inside
EFI_SMM_CONTROL2_PROTOCOL.Trigger(), I wonder if that's a good idea.

Namely, the hardware should assert the SMI while inside Trigger(),
edk2's SMI handler / dispatcher will run, and call into the appropriate
SMM DXE driver. That driver might do any kind of stuff, like update the
varstore in the pflash, which could take long.

I observe two things:
(a) "taking long" may not be allowed on whatever task priority level we
are at the moment. For example, if we raised the TPL to TPL_NOTIFY in
the "locking", then:

    Blocking is not allowed at this level. Code executes to completion
    and returns. If code requires more processing, it needs to signal
    an event to wait to obtain control again at whatever level it
    requires. This level is typically used to process low level IO to
    or from a device.

I guess it's akin to the Linux kernel's "hardirq" (top half) context. So
I'm not sure all SMM DXE drivers satisfy this, ie. if Trigger() can just
bump the TPL to TPL_NOTIFY indiscriminately.

(b) This is actually the opposite argument. Since SMM is security
sensitive, *and* UEFI is single-processor / single-threaded (well you
can use multiple processors, you just can't run any UEFI code on
anything but the BP), the timer interrupt should be blocked / masked
*anyway* while in SMM, no? Otherwise edk2 could start running a plain
timer event callback in the middle of an SMM handler. Which makes me
think that this locking / interrupt masking must already be in place,
and it's not the responsibility of the individual
EFI_SMM_CONTROL2_PROTOCOL.Trigger() implementation. (PI 1.3 vol4 doesn't
mention this responsibility in any case.)

I've learned about the "EDK II SMM Call Topology" document from a piwg
message:

http://sourceforge.net/projects/edk2/files/General%20Documentation/EDK%20II%20SMM%20call%20topology.pdf/download

It doesn't speak about masking the timer.

Does SMI mask other interrupts "architecturally" perhaps?

...

(3) Oh, this is sad. Well, I am sad. Turns out there's a third UEFI
protocol that OVMF needs to implement: EFI_SMM_CONFIGURATION_PROTOCOL.
Its only interesting member is RegisterSmmEntry(), and that function is
supposed to bind the central entry point (which is already available in
the edk2 tree) to the processor-level SMI handler.

(I'm kind of confused, because last time I experimented with faking
SMRAM / SMM in OVMF, my fake Trigger() function just returned success,
and there was no EFI_SMM_CONFIGURATION_PROTOCOL at all, and things
seemed to work. In retrospect I can't imagine how control was
transferred at all, without actual SMM / SMI support in both QEMU and
OVMF. Hm... looking at occurrences of "SmmEntryPointRegistered", this
may have been intentional in edk2.)

EFI_SMM_CONFIGURATION_PROTOCOL discussed in the "EDK II SMM Call
Topology" document, on the "SmmDriverDispatcher" and especially the
"SMBASE Relocation" pages. It takes a separate CPU driver, and
(obviously) assembly code.

The "SMBASE Relocation" page references "IA32FamilyCpuPkg", which is not
open source. Nothing in edk2 produces gEfiSmmConfigurationProtocol, and
no source file contains the RSM instruction.

The Vlv2TbltDevicePkg package (ValleyView2 Tablet?) makes several
references to IA32FamilyCpuPkg, but those are only references.

I guess IA32FamilyCpuPkg is exactly the kind of chipset code that Intel
has not opened up.

Our "SMM in OVMF" effort just got set back by months. :(

Laszlo

Re: [Qemu-devel] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger()

[Paolo Bonzini]

On 21/04/2015 22:31, Laszlo Ersek wrote:
> typedef enum {
>   EfiLockUninitialized = 0,
>   EfiLockReleased      = 1,
>   EfiLockAcquired      = 2
> } EFI_LOCK_STATE;
> 
> typedef struct {
>   EFI_TPL         Tpl;
>   EFI_TPL         OwnerTpl;
>   EFI_LOCK_STATE  Lock;
> } EFI_LOCK;
> 
> VOID
> EFIAPI
> EfiAcquireLock (
>   IN EFI_LOCK  *Lock
>   )
> {
>   ASSERT (Lock != NULL);
>   ASSERT (Lock->Lock == EfiLockReleased);
> 
>   Lock->OwnerTpl = gBS->RaiseTPL (Lock->Tpl);
>   Lock->Lock     = EfiLockAcquired;
> }
> 
> VOID
> EFIAPI
> EfiReleaseLock (
>   IN EFI_LOCK  *Lock
>   )
> {
>   EFI_TPL Tpl;
> 
>   ASSERT (Lock != NULL);
>   ASSERT (Lock->Lock == EfiLockAcquired);
> 
>   Tpl = Lock->OwnerTpl;
> 
>   Lock->Lock = EfiLockReleased;
> 
>   gBS->RestoreTPL (Tpl);
> }

Okay, first of all this is obviously not a spinlock that works on a
multiprocessor system.  But it is actually relatively close.

> But the prototypes of these functions are very misleading. They imply
> that you can take separate locks, and that locking one will not prevent
> locking another. This is false.

No, I think it can work.  You just have to be aware of the TPL of the
locks.   If you assume that you're being called with no lock taken, it's
easy to track that.

The TPL of the lock should be the highest TPL of "things that can
interrupt you".  I guess you can just use TPL_NOTIFY and possibly then
look into relaxing it.

> Does SMI mask other interrupts "architecturally" perhaps?

It masks interrupts just because on entry to SMM the interrupt flag is
cleared.  NMIs are also inhibited on entry to SMM.

> EFI_SMM_CONFIGURATION_PROTOCOL discussed in the "EDK II SMM Call
> Topology" document, on the "SmmDriverDispatcher" and especially the
> "SMBASE Relocation" pages. It takes a separate CPU driver, and
> (obviously) assembly code.

Oh, that's unfortunate.  It's not really bad, as the SMM "trampoline"
code can be really small (SeaBIOS does just "mov %cs, %ax" followed by a
jump to its usual protected mode entry routine, IIRC) and the exit is
really as simple as "rsm".  But it sucks. :(

Oh well, at least it's well documented.

Paolo

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Gerd Hoffmann]

  Hi,

> A "unified" patchset (not necessarily posted, just pushed somewhere)
> would be helpful down the road -- IIRC this one was claimed unapplicable
> to current master. (Which is also why I didn't try the "info mtree"
> command, see elsewhere in the thread -- I didn't try to build the series.)

https://www.kraxel.org/cgit/qemu/log/?h=rebase/smm-wip

HTH,
  Gerd

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Gerd Hoffmann]

  Hi,

> tseg is just normal ram (yes, located at the end of memory), but (once
> tseg is enabled) only cpus in smm mode are allowed to access it.
> Likewise busmaster dma access is rejected, so non-smm code can't use the
> sata controller to access this indirectly.

Update:  Seems tseg can be anywhere, there is a "tseg memory base"
register @ 0xac in pci config space.

Placing it at the end of memory is just what the bios is supposed to do
by default.  And it makes sense to place it there.

<quote>
This register contains the base address of TSEG DRAM memory. BIOS
determines the base of TSEG memory by subtracting the TSEG size (PCI
Device 0, offset 9Eh, bits 2:1) from graphics GTT stolen base (PCI
Device 0, offset A8h, bits 31:20).

Once D_LCK has been set, these bits becomes read only.
</quote>

"GTT stolen base" equals "top of below-4g memory" for us because we
emulate the chipset variant without graphics in qemu.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Laszlo Ersek]

On 04/22/15 10:09, Gerd Hoffmann wrote:
>   Hi,
> 
>> tseg is just normal ram (yes, located at the end of memory), but (once
>> tseg is enabled) only cpus in smm mode are allowed to access it.
>> Likewise busmaster dma access is rejected, so non-smm code can't use the
>> sata controller to access this indirectly.
> 
> Update:  Seems tseg can be anywhere, there is a "tseg memory base"
> register @ 0xac in pci config space.
> 
> Placing it at the end of memory is just what the bios is supposed to do
> by default.  And it makes sense to place it there.
> 
> <quote>
> This register contains the base address of TSEG DRAM memory. BIOS
> determines the base of TSEG memory by subtracting the TSEG size (PCI
> Device 0, offset 9Eh, bits 2:1) from graphics GTT stolen base (PCI
> Device 0, offset A8h, bits 31:20).
> 
> Once D_LCK has been set, these bits becomes read only.
> </quote>
> 
> "GTT stolen base" equals "top of below-4g memory" for us because we
> emulate the chipset variant without graphics in qemu.

Thanks, that sounds good. So, as far as I understand, no changes to what
we've discussed thus far.

But, I have another question -- am I allowed to use "top of below-4g
memory" directly, as discussed earlier, or should I use the above PCI
registers? The tseg size will actually come from me (because I'll select
it), but the top I could take from "top of below-4g memory" (preferably,
see earlier), or reading the 0xA8 register.

Unless, of course 0xA8 won't be implemented at all, *because* "we
emulate the chipset variant without graphics in qemu." In other words,
if the 0xA8 register is dependent on the integrated graphics, then I
don't have a question. :)

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Gerd Hoffmann]

  Hi,

> Thanks, that sounds good. So, as far as I understand, no changes to what
> we've discussed thus far.
> 
> But, I have another question -- am I allowed to use "top of below-4g
> memory" directly, as discussed earlier, or should I use the above PCI
> registers? The tseg size will actually come from me (because I'll select
> it), but the top I could take from "top of below-4g memory" (preferably,
> see earlier), or reading the 0xA8 register.

I've figured there is one more register, with the funky name "Top of Low
Usable DRAM".  Which should hold our "top of below-4g memory" value.  I
guess I should read the whole mch spec from start to end once ...

But as I read the specs it is the job of the firmware to set all these
registers.  On physical hardware you can go into the firmware setup and
configure all sorts of stuff there, like the pci io window size,
graphics memory size etc.  And probably the firmware then goes setup all
those chipset registers for low memory size, gfx regions size, tseg etc.
according to the setup configuration.

I'd recommend to likewise just write the correct values into those
registers.  Right now it has no effect (other than the guest os seeing
realistic values when reading those registers).  But if the tseg memory
base register is implemented some day it surely is helpful if ovmf sets
it correctly ;)

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Laszlo Ersek]

On 04/21/15 17:04, Gerd Hoffmann wrote:
>>> +static const MemoryRegionOps tseg_blackhole_ops = {
>>> +    .read = tseg_blackhole_read,
>>> +    .write = tseg_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,
>>> +};
>>
>> (a) you've specified .endianness twice (with inconsistent initializers
>> at that, although the value returned by the accessor happens to be
>> unaffected).
> 
> Oops, cut+paste bug, will fix (/me wonders why gcc didn't throw an error
> on that one).
> 
>> (b) Why are 8-byte accesses forbidden? ...
> 
> just a matter of setting .valid.max_access_size = 8, can do that of
> course.
> 
>>> +
>>>  /* PCIe MMCFG */
>>>  static void mch_update_pciexbar(MCHPCIState *mch)
>>>  {
>>> @@ -267,6 +289,7 @@ static void mch_update_smram(MCHPCIState *mch)
>>>  {
>>>      PCIDevice *pd = PCI_DEVICE(mch);
>>>      bool h_smrame = (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME);
>>> +    uint32_t tseg_size;
>>>  
>>>      /* implement SMRAM.D_LCK */
>>>      if (pd->config[MCH_HOST_BRIDGE_SMRAM] & MCH_HOST_BRIDGE_SMRAM_D_LCK) {
>>> @@ -296,6 +319,32 @@ static void mch_update_smram(MCHPCIState *mch)
>>>          memory_region_set_enabled(&mch->high_smram, false);
>>>      }
>>>  
>>> +    if (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_T_EN) {
>>> +        switch (pd->config[MCH_HOST_BRIDGE_ESMRAMC] &
>>> +                MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK) {
>>> +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_1MB:
>>> +            tseg_size = 1024 * 1024;
>>> +            break;
>>> +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_2MB:
>>> +            tseg_size = 1024 * 1024 * 2;
>>> +            break;
>>> +        case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_8MB:
>>> +            tseg_size = 1024 * 1024 * 8;
>>> +            break;
>>> +        default:
>>> +            tseg_size = 0;
>>> +            break;
>>> +        }
>>> +    } else {
>>> +        tseg_size = 0;
>>> +    }
>>
>> - so, is the guest supposed to select the tseg size by writing this
>> register?
> 
> Correct.
> 
>> - I assume this register is not reprogrammable once SMRAM is locked --
>> is that correct?
> 
> Correct.
> 
>> - can the guest somehow use this facility to detect, dynamically, the
>> presence of this feature? (For now I'm thinking about a static build
>> flag for OVMF that would enable SMM support, but I'm 99% sure Jordan
>> will object and ask for a dynamic feature detection.)
> 
> Hmm.  I think if we merge all the smm / smram / tseg patches in one go
> this should work.  Without patches reading the ESMRAMC register returns
> zero.  With the patches the three cache-disable bits are hardcoded to
> '1'.  This should work for detecting tseg support.
> 
> You also have to test for smm support.  The current protocol for this is
> that seabios checks whenever smm is already initialized (see
> *_apmc_smm_setup() in seabios/src/fw/smm.c) and if so it skips smm
> initialization.  Right now qemu sets the bit on reset when running on
> kvm, so seabios doesn't try to use smm.  On tcg the bit is clear after
> reset and seabios actually uses smm mode.

I started looking into this. Basically the condition for SMM/SMRAM
support is:

  Q35 && SMRAM support && SMM support           (1)

The first subcondition can be checked via the host bridge devid (and
OVMF already does, for other purposes).

The second one relies on the ESMRAMC, which is in PCI config space.

The third one is messy. It relies on SMI_EN, which is an ioport at
PMBA+0x30. It requires a configured PMBA.

The problem for OVMF is the following: this condition is too complex /
too intrusive to evaluate in order to see whether Q35+SMM+SMRAM are
available.

For that reason, I'd like to ask if it would be possible to introduce a
new fw_cfg file that would simply communicate the end result of the
above expression.

Long version:

If the above condition evaluates to TRUE, we assume that the user wants
"security" -- ie. it wants to prevent a "malicious" runtime guest OS
form messing with OVMF's Secure Boot feature. Good.

However, at the moment, OVMF has a large number of holes that a
malicious runtime guest OS can exploit. The S3 LockBox and the varstore
pflash chip are most frequently mentioned, but there are more; in
particular I'm referring to the other special memory ranges OVMF uses.

(Please refer to the

  A comprehensive memory map of OVMF

section of the OVMF whitepaper
<http://people.redhat.com/~lersek/ovmf-whitepaper-c770f8c.txt> for the
following.  It describes in detail what special memory ranges OVMF has,
and the life cycle of each.)

These ranges are adequately protected against a *benign* guest runtime
OS; the UEFI memory map makes sure that such a runtime OS doesn't
accidentally overwrite stuff that OVMF will either *need* for S3 resume
in pristine form from the first cold boot, or will *overwrite* during S3
resume as scratch space (thereby potentially destroying OS data).

But a malicious guest OS can nonetheless overwrite things in these
special areas that OVMF will run (or use) during S3, before it locks
down SMRAM again.

Some of those holes / inappropriately protected memory ranges are tied
to the SEC phase, which is the earliest and least capable UEFI phase.
For example, refer to:

+--------------------------+ 9216 KB       PcdOvmfDxeMemFvBase
|                          |
|PEIFV from FVMAIN_COMPACT | size: 896 KB  PcdOvmfPeiMemFvSize
|  decompressed firmware   |
| volume with PEI modules  |
|                          |
+--------------------------+ 8320 KB       PcdOvmfPeiMemFvBase

"(6) PEIFV -- decompressed firmware volume with PEI modules".

OVMF is an unconventional UEFI firmware in the sense that it runs *only*
the SEC phase from (unmodifiable, read-only) flash. The PEI and DXE
firmware volumes are decompressed from flash to RAM by SEC, and then PEI
and DXE modules run from RAM.

(It is more conventional to run PEI modules from the flash as well, not
just SEC. The way OVMF does it allows it to save space in the pflash,
because PEI modules are thus allowed to exist only in compressed (as
opposed to directly executable) form in the pflash.)

In order to save some cycles, OVMF's SEC decompresses the PEI and DXE
firmware volumes (in one go) to RAM after a real reset only, and then
"protects" the decompressed PEI firmware volume in RAM via the UEFI
memory map (see above). Then, at S3 resume, SEC simply reuses the
already decompressed (and "protected") PEI firmware volume when handing
off to PEI.

Obviously, if a guest OS pokes some foreign code into this decompressed
PEI firmware volume before S3 suspend, then OVMF will run that foreign
code at S3 resume, with the SMRAM open.

Therefore OVMF has to forego such optimizations even as early as in SEC
when condition (1) evaluates to false -- it must decompress the firmware
volumes from pristine pflash over the possibly breached RAM even during
S3 resume. And for that SEC needs to evaluate (1).

This is where the complexity / intrusiveness of (1) becomes a problem.
Programming the PMBA in SEC just so I can read SMI_EN (for the sake of
evaluation (1)) is something I'd like to avoid. We already have a nice
fw_cfg client library that is usable in SEC; that would be my choice for
learning about condition (1).

I'll audit the rest of the special memory regions too, of course. In any
case, exposing this QEMU capability / configuration (see also Paolo's
-machine smm=on/off idea) via fw_cfg seems both appropriate for fw_cfg
(after all it does configure the firmware) and the least intrusive for OVMF.

What do you think?

Thanks!
Laszlo


>>> +    memory_region_set_enabled(&mch->tseg_blackhole, tseg_size);
>>> +    memory_region_set_size(&mch->tseg_blackhole, tseg_size);
>>> +    memory_region_add_subregion_overlap(mch->system_memory,
>>> +                                        mch->below_4g_mem_size - tseg_size,
>>> +                                        &mch->tseg_blackhole, 1);
>>
>> So, does tseg overlay the last few MB of the RAM below 4GB? (Ie. right
>> before the PCI hole starts?)
> 
> tseg is just normal ram (yes, located at the end of memory), but (once
> tseg is enabled) only cpus in smm mode are allowed to access it.
> Likewise busmaster dma access is rejected, so non-smm code can't use the
> sata controller to access this indirectly.
> 
>>> +    memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch),
>>> +                          &tseg_blackhole_ops, NULL,
>>> +                          "tseg-blackhole", 0);
>>
>> Okay, this answers one of the above, tseg_blackhole is never uninitialized.
>>
>>> +    memory_region_set_enabled(&mch->tseg_blackhole, false);
>>> +    memory_region_add_subregion_overlap(mch->system_memory,
>>> +                                        mch->below_4g_mem_size,
>>> +                                        &mch->tseg_blackhole, 1);
>>
>> The address (2nd argument) is inconsistent with the one in
>> mch_update_smram() -- this function call places the tseg black hole at
>> the beginning of the 32-bit PCI hole, not just below it.
> 
> It's a zero-length hole here, therefore it actually is the same.  But I
> didn't bother to use "mch->below_4g_mem_size - 0" as start address to
> make that more clear ;)
> 
>> (a) place the permanent PEI RAM so that it ends at the top of
>> "below_4g_mem_size"; PublishPeiMemory()
>> (b) create one of the memory HOBs, QemuInitializeRam(),
>> (c) create the MMIO HOB that describes the 32-bit PCI hole,
>>     MemMapInitialization()
>>
>> If the last 1 / 2 / 8 MB of the low RAM can't be used as general purpose
>> RAM, then (a) is affected (it should simply be sunk by the tseg size of
>> choice); (b) is affected similarly (just decrease the top of the memory
>> HOB);
> 
> Yes.
> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Laszlo Ersek]

another point:

On 04/22/15 23:41, Laszlo Ersek wrote:
> On 04/21/15 17:04, Gerd Hoffmann wrote:

>>> - can the guest somehow use this facility to detect, dynamically, the
>>> presence of this feature? (For now I'm thinking about a static build
>>> flag for OVMF that would enable SMM support, but I'm 99% sure Jordan
>>> will object and ask for a dynamic feature detection.)
>>
>> Hmm.  I think if we merge all the smm / smram / tseg patches in one go
>> this should work.  Without patches reading the ESMRAMC register returns
>> zero.  With the patches the three cache-disable bits are hardcoded to
>> '1'.  This should work for detecting tseg support.
>>
>> You also have to test for smm support.  The current protocol for this is
>> that seabios checks whenever smm is already initialized (see
>> *_apmc_smm_setup() in seabios/src/fw/smm.c) and if so it skips smm
>> initialization.  Right now qemu sets the bit on reset when running on
>> kvm, so seabios doesn't try to use smm.  On tcg the bit is clear after
>> reset and seabios actually uses smm mode.
> 
> I started looking into this. Basically the condition for SMM/SMRAM
> support is:
> 
>   Q35 && SMRAM support && SMM support           (1)
> 
> The first subcondition can be checked via the host bridge devid (and
> OVMF already does, for other purposes).
> 
> The second one relies on the ESMRAMC, which is in PCI config space.
> 
> The third one is messy. It relies on SMI_EN, which is an ioport at
> PMBA+0x30. It requires a configured PMBA.
> 
> The problem for OVMF is the following: this condition is too complex /
> too intrusive to evaluate in order to see whether Q35+SMM+SMRAM are
> available.
> 
> For that reason, I'd like to ask if it would be possible to introduce a
> new fw_cfg file that would simply communicate the end result of the
> above expression.

There's another problem with basing this decision in OVMF on
SMI_EN.APMC_EN: it is not an idempotent check. At some point the
firmware itself has to set SMI_EN.APMC_EN.

This is probably no issue for SeaBIOS, but in OVMF there's a bunch of
more-or-less independently dispatched modules that depend on condition
(1), and if I set SMI_EN.APMC_EN in some of those modules myself, then
code that ends up running later cannot reuse SMI_EN.APMC_EN for
determining (1). So I'd have to store the original result in some PCD,
but that itself doesn't guarantee ordering either, so it just becomes a
huge mess.

fw_cfg on the other hand is always available, and this fw_cfg file would
never change.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Gerd Hoffmann]

  Hi,

> > The third one is messy. It relies on SMI_EN, which is an ioport at
> > PMBA+0x30. It requires a configured PMBA.

Isn't that initialized early anyway, because the pm timer lives there?
It's not a regular pci bar exactly to allow early init, so the full pci
bus scan + bar allocation done later will not mess up things.

[ just a question, could very well be that even when initialized early
  it isn't early enough because you need to know the memory layout
  before uncompressing the firmware modules ]

> There's another problem with basing this decision in OVMF on
> SMI_EN.APMC_EN: it is not an idempotent check. At some point the
> firmware itself has to set SMI_EN.APMC_EN.

Yep, you'll need some variable saying whenever smm is there or not and
check that.  Because of this.  Or (in case we are going for the fw_cfg
file) because you don't want dig into fw_cfg each time you need to know
this.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Laszlo Ersek]

On 04/23/15 09:02, Gerd Hoffmann wrote:
>   Hi,
> 
>>> The third one is messy. It relies on SMI_EN, which is an ioport at
>>> PMBA+0x30. It requires a configured PMBA.
> 
> Isn't that initialized early anyway, because the pm timer lives there?

TimerLib (which is based in OVMF's case on the PM timer) is not needed /
used before PEI in the default case. It is used in SEC only when -D
SOURCE_DEBUG_ENABLE is passed for the build (which is "never" in practice).

> It's not a regular pci bar exactly to allow early init, so the full pci
> bus scan + bar allocation done later will not mess up things.
> 
> [ just a question, could very well be that even when initialized early
>   it isn't early enough because you need to know the memory layout
>   before uncompressing the firmware modules ]

I'm trying to avoid that; the decompression happens to a low fixed range.

>> There's another problem with basing this decision in OVMF on
>> SMI_EN.APMC_EN: it is not an idempotent check. At some point the
>> firmware itself has to set SMI_EN.APMC_EN.
> 
> Yep, you'll need some variable saying whenever smm is there or not and
> check that.  Because of this.  Or (in case we are going for the fw_cfg
> file) because you don't want dig into fw_cfg each time you need to know
> this.

The annoying limitation with SEC is that it cannot use normal C static
variables, nor PCDs. It is okay if SEC is a bit wasteful on fw_cfg (and
even in SEC I might be able to cache the result in a local variable and
pass it around). In PEI I can already use static variables (because OVMF
is unorthodox and runs PEI modules from RAM, not flash), plus I can set
PCDs (because in PEI the PCDs live in a dedicated HOB, and that HOB is
allocated from RAM (independently of OVMF)).

I'll ask Jordan too about the dynamic feature detection, because there's
another big hurdle with it (selecting a LockBox library instance at
runtime, dependent on the presence of SMM). Deciding about SMM support
at build time would make things hugely easier (because that's how
firmware for physical platforms is built as well).

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Laszlo Ersek]

On 04/23/15 09:41, Laszlo Ersek wrote:
> On 04/23/15 09:02, Gerd Hoffmann wrote:
>>   Hi,
>>
>>>> The third one is messy. It relies on SMI_EN, which is an ioport at
>>>> PMBA+0x30. It requires a configured PMBA.
>>
>> Isn't that initialized early anyway, because the pm timer lives there?
> 
> TimerLib (which is based in OVMF's case on the PM timer) is not needed /
> used before PEI in the default case. It is used in SEC only when -D
> SOURCE_DEBUG_ENABLE is passed for the build (which is "never" in practice).
> 
>> It's not a regular pci bar exactly to allow early init, so the full pci
>> bus scan + bar allocation done later will not mess up things.
>>
>> [ just a question, could very well be that even when initialized early
>>   it isn't early enough because you need to know the memory layout
>>   before uncompressing the firmware modules ]
> 
> I'm trying to avoid that; the decompression happens to a low fixed range.
> 
>>> There's another problem with basing this decision in OVMF on
>>> SMI_EN.APMC_EN: it is not an idempotent check. At some point the
>>> firmware itself has to set SMI_EN.APMC_EN.
>>
>> Yep, you'll need some variable saying whenever smm is there or not and
>> check that.  Because of this.  Or (in case we are going for the fw_cfg
>> file) because you don't want dig into fw_cfg each time you need to know
>> this.
> 
> The annoying limitation with SEC is that it cannot use normal C static
> variables, nor PCDs. It is okay if SEC is a bit wasteful on fw_cfg (and
> even in SEC I might be able to cache the result in a local variable and
> pass it around). In PEI I can already use static variables (because OVMF
> is unorthodox and runs PEI modules from RAM, not flash), plus I can set
> PCDs (because in PEI the PCDs live in a dedicated HOB, and that HOB is
> allocated from RAM (independently of OVMF)).
> 
> I'll ask Jordan too about the dynamic feature detection, because there's
> another big hurdle with it (selecting a LockBox library instance at
> runtime, dependent on the presence of SMM). Deciding about SMM support
> at build time would make things hugely easier (because that's how
> firmware for physical platforms is built as well).

Okay, looks like Jordan agrees with using a static build option.

This makes things much easier, and I think I won't need a new fw_cfg file.

I will certainly do a sanity check (based on the condition we discussed
eariler in this thread, using the PCI config and iospace registers).

It's going to be "somewhere" in the firmware (still thinking about the
best place), but this way I won't have to handle everything (fall back
to non-SMM behavior) if the sanity check fails; in that case I can just
stop.

Public IRC transcript follows.

Thanks!
Laszlo

* Loaded log from Wed Apr 22 21:45:19 2015

* Now talking on #edk2
* Topic for #edk2 is: EDK II / OVMF / UEFI development discussions |
  http://www.tianocore.org/edk2
* Topic for #edk2 set by
  jljusten!~jljusten@static-50-43-41-168.bvtn.or.frontiernet.net at Thu
  Feb  5 03:36:47 2015

<lersek>   jljusten, are you still online?
<lersek>   (I guess you are, from your recent email)
<jljusten> lersek: yeah. it's getting a bit late though. :)
<lersek>   jljusten, okay, just briefly then
<lersek>   I can send an email too if you prefer that
<lersek>   the question is dynamic detection of SMM/SMRAM
           support
<lersek>   it is quite messy
<lersek>   and has a large number of ties into things
<lersek>   several modules / module types are affected
<lersek>   but the worst question is how we'd select a
           LockBox library instance dynamically
<lersek>   dependent on the presence of the SMM/SMRAM
           capability in QEMU
<lersek>   one idea I had
<lersek>   is quite ugly
<lersek>   but could work
<jljusten> ah, yikes
<lersek>   (1) introduce a new PPI and a new protocol
<lersek>   with some GUID we generate
<lersek>   this PPI and protocol would abstract the LockBox
           library interface
<lersek>   the PPI for PEIMs
<lersek>   the protocol for DXE modules
<lersek>   (2) we'd build two such drivers
<lersek>   for each of the phases
<lersek>   one pair would back the PPI / protocol with the
           SMM-based (unprivileged) lockbox library instance
<lersek>   another pair would back the PPI / protocol with
           our current (insecure) lockbox library instance
<lersek>   (3) we'd build both pairs of drivers into OVMF
<lersek>   and their entry points would check for SMM/SMRAM
<lersek>   only one pair would remain active and install the
           PPI / protocol
<lersek>   (4) we'd introduce another LockBox library
           instance
<lersek>   that would depend on the PPI / protocol
<lersek>   on the depex level
<lersek>   (PPI depexes can also be stated for PEIMs)
<jljusten> wow. any chance this could be a build time
           option? :)
<lersek>   then all modules using this library instance
           would be delayed until after the corect driver
<lersek>   exactly!
<lersek>   so you can see where this is going
<lersek>   a huge mess
<lersek>   I actually *want* to make this a build time option
<lersek>   but I was worried you'd ask me to do it
           dynamically
<lersek>   basing it on -D SMM_ENABLE, statically
<lersek>   would make things hugely easier
<lersek>   not just for the LockBox lib instance
<lersek>   but a number of other things as well
<lersek>   so if you agree to make SMM/SMRAM support a
           static build time config option
<lersek>   that would be awesome
<jljusten> Whenever I think of SMM, I think, yeah that'd be
           cool to have a sample platform in EDK II, but it never
           sounds like something we'd want to enable by default.
<jljusten> Of course, that could change over time...
<lersek>   right
<lersek>   so let's agree on the -D SMM_ENABLE for now
<lersek>   thank you very much
<lersek>   another question quickly while we're at it
<lersek>   how would you prefer checks for this in the C
           code?
<lersek>   feature PCD?
<lersek>   controlled by the build flag?
<lersek>   I think that's usually the best method for stuff
           like this
<lersek>   it's better than conditional inclusion (ie.
           #ifdef etc)
<lersek>   because all code gets verified by the compiler
<lersek>   I'll also add some sanity checks of course
<jljusten> Yeah, I think so.
<lersek>   so that a -D SMM_ENABLE build rejects running
           early, if qemu doesn't actually provide the feature
<lersek>   jljusten, thank you
<jljusten> Any chance to get video running before then?
           Probably not critical. If they are using the SMM build, they
           should know they need a newer QEMU...
<lersek>   jljusten, hm, I think video is initialized quite
           late; it depends on a UEFI driver and is connected in BDS --
           presence of SMM/SMRAM communicates that the user wants
           things to be secure, so it affects even SEC and PEI.
<lersek>   If those phases expect SMM/SMRAM and it's not
           there, the best would be just to stop right there (after
           logging an error message)
<lersek>   anyway we can perhaps refine this in one of the
           versions of the patchset-to-be
<lersek>   the important thing is that I can now start out
           with a static config flag, knowing that I won't have to
           rewrite *that*. :)
<lersek>   thanks!
<jljusten> I think SMM is not initialized until DXE, but
           yeah, I somehow doubt video would work in that case.
<lersek>   jljusten, indeed, SMM *drivers* only come into
           the picture in DXE
<lersek>   but until then the SMRAM area (TSEG on Q35/MCH)
           needs to be removed from the system memory HOB we install in
           PlatformPei
<lersek>   so that no memory allocations are served from
           there
<lersek>   the TSEG area is the last 1, 2, or 8 MB just
           below the end of the low RAM (ie. top of RAM under 4GB)
<lersek>   we need to set that aside while in PEI
<lersek>   even the permanent PEI ram cannot be installed
           the way it is now
<lersek>   (last 64 MB under top of RAM below 4GB)
<lersek>   so this affects a number of things
<lersek>   I'll also have to audit all of the special memory
           ranges that OVMF uses (see the "comprehensive memory map"
           section in the whitepaper)
<lersek>   because those ranges are sensitive (SMRAM is open
           while SEC and PEI run) and we must not allow the OS to poke
           data directly into some of them
<lersek>   so for example
<lersek>   we can't let SEC just reuse the pre-decompressed
           PEIFV on S3 resume
<lersek>   if SMM/SMRAM is selected
<lersek>   because a malicious OS may have overwritten the
           PEIFV
<lersek>   and then if it suspends, then the user resumes
<lersek>   the firmware will run the OS-injected code as
           part of PEI, with the SMRAM open
<lersek>   etc
<lersek>   it's going to be hugely intrusive
<jljusten> Right. I think most real platforms run code from
           flash in that case.
<lersek>   exactly
<lersek>   so that's one area where our smartness (ie.
           running PEI from RAM, and only SEC from flash) is hurting us
           wrt. SMM a little bit
<jljusten> You ought to be able to run from the SMRAM region
           though.
<lersek>   considering the above
<lersek>   the first idea I've come up with
<lersek>   is to just decompress the firmware volumes again
<lersek>   on S3 resume
<lersek>   overwriting whatever a potentially malicious
           runtime guest OS may have poked into there
<lersek>   but
<lersek>   of course
<lersek>   given the way we compress things, for the sake of
           good compression ratio
<lersek>   that means we'd decompress not just PEIFV but
           DXEFV too
<lersek>   and for that we'll need to set aside much more RAM
<lersek>   even from a non-malicious OS
<lersek>   because decompressing DXEFV too during S3 resume
           must not overwrite OS data
<lersek>   so this would be an argument for reverting to a
           more traditional layout
<lersek>   where PEI is uncompressed and runs from flash
<lersek>   and DXEFV is separately decompressed
<lersek>   in any case,
<lersek>   let me just run with this "decompress them both
           on S3" initial idea
<lersek>   we can discuss things better when there are
           patches for illustration
<lersek>   it doesn't have to be perfect in v1
<jljusten> We could trust code in SMRAM right?
<lersek>   see the original S3 series :)
<lersek>   that's one idea as well
<lersek>   I did think of it
<lersek>   we could place PEI in SMRAM
<lersek>   there are at least two problems with it though
<lersek>   (1) it would require making SEC too smart
<lersek>   configuring the SMRAM control registers and stuff
<lersek>   but that's the smaller issue
<lersek>   the big issue is the S3 PEIM itself
<lersek>   (2) that one has builtin knowledge about SMRAM
<lersek>   and it explicitly uses the SMM ACCESS2 PPI
<lersek>   to open and close smram
<lersek>   obviously
<lersek>   if it is running from SMRAM
<lersek>   it doesn't help if it closes down SMRAM
           underneath itself. :)
<lersek>   anyway what we know now should be enough for
           prototyping
<lersek>   without committing to directions that would be
           certainly wrong in the future
<lersek>   I'll send out a transcript of this discussion in
           the qemu-devel thread
<lersek>   I'll CC you too, and then from the message ID
           you'll be able to find the entire thread on gmane

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Gerd Hoffmann]

  Hi,

> I'll ask Jordan too about the dynamic feature detection, because there's
> another big hurdle with it (selecting a LockBox library instance at
> runtime, dependent on the presence of SMM). Deciding about SMM support
> at build time would make things hugely easier (because that's how
> firmware for physical platforms is built as well).

piix/q35 falls into the same category I guess.  We are just lucky that
(a) alot of the chipset-specific initialization is not needed on qemu
and (b) we can hide the remaining differences in ovmf-specific modules.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Laszlo Ersek]

On 04/23/15 10:34, Gerd Hoffmann wrote:
>   Hi,
> 
>> I'll ask Jordan too about the dynamic feature detection, because there's
>> another big hurdle with it (selecting a LockBox library instance at
>> runtime, dependent on the presence of SMM). Deciding about SMM support
>> at build time would make things hugely easier (because that's how
>> firmware for physical platforms is built as well).
> 
> piix/q35 falls into the same category I guess.  We are just lucky that
> (a) alot of the chipset-specific initialization is not needed on qemu
> and (b) we can hide the remaining differences in ovmf-specific modules.

That's precisely it.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Paolo Bonzini]

On 23/04/2015 09:02, Gerd Hoffmann wrote:
>>> > > The third one is messy. It relies on SMI_EN, which is an ioport at
>>> > > PMBA+0x30. It requires a configured PMBA.
> Isn't that initialized early anyway, because the pm timer lives there?
> It's not a regular pci bar exactly to allow early init, so the full pci
> bus scan + bar allocation done later will not mess up things.
> 
> [ just a question, could very well be that even when initialized early
>   it isn't early enough because you need to know the memory layout
>   before uncompressing the firmware modules ]

The nice thing about TSEG is that it's just normal RAM until you lock
it.  So you can add a HOB very early that reserves that memory, and only
later decide whether to use "real" SMM, or just not lock TSEG and jump
into the SMM handler.  (This would be the "experimentation" setting you
were talking about earlier; it would also be used if TSEG is not available).

The fake path would also be used on PIIX.  This means you would _always_
use the SMM stack, even if it's not running in SMM, and you could e.g.
get rid of the PEI/DXE drivers for flash access.

>> > There's another problem with basing this decision in OVMF on
>> > SMI_EN.APMC_EN: it is not an idempotent check. At some point the
>> > firmware itself has to set SMI_EN.APMC_EN.
> Yep, you'll need some variable saying whenever smm is there or not and
> check that.  Because of this.  Or (in case we are going for the fw_cfg
> file) because you don't want dig into fw_cfg each time you need to know
> this.

You can assume that either all of TSEG/SMM/outb(0xb2)/GLB_SMI_EN are
available, or none is.  Detection can go like this:

1) if APMC_EN = 0, SMM is available but other features might not be.
Set APMC_EN and GLB_SMI_EN, set SMI_LOCK(*), then clear GLB_SMI_EN.  Now
APMC_EN is always 1, but a new QEMU won't let you clear GLB_SMI_EN.

	(*) SMI_LOCK is in the configuration space of
	    device 31 function 0 (byte 0xa0, bit 4)

2) is GLB_SMI_EN = 1?  Then SMM is available.

3) else, QEMU has set APMC_EN to communicate that SMIs are not
available, or SMIs are available (e.g. old QEMU with TCG) but you don't
have TSEG.  Proceed with fake SMM.

Would this work?  It is idempotent at least.  Even if the OS tries to
maliciously set APMC_EN to 0 (SMI_LOCK doesn't lock APMC_EN), the result
after step 1 is always going to be the same: APMC_EN=GLB_SMI_EN=1 on new
QEMU, APMC_EN=1/GLB_SMI_EN=0 on old QEMU.

Paolo

Re: [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested)

[Yao, Jiewen]

Hi Laszlo
Below is my thought.

2) You are right. According to IA32 manual - "Maskable hardware interrupts, exceptions, NMI interrupts, SMI interrupts, A20M interrupts, single-step traps, breakpoint traps, and INIT operations are inhibited when the processor enters SMM." You can also find more detail in "34.6 EXCEPTIONS AND INTERRUPTS WITHIN SMM"

In below doc, the "SMM Timer is initialized" means the timer used by BSP to check if APs are all pulled into SMM for CPU Rendez-vous. A typical implementation is ACPI timer (PCH), or APIC timer (CPU). BSP just check timer count, but the timer does not generate any interrupt.

3) Yes. SmmCoreEntry is useful function as bridge between a real CPU_SMM driver and SMM_CORE.
In real world, when SMI happen, CPU will jump to SM_BASE in real mode. Then it will jump to X64 mode immediately.
The all CPUs will be in election phase. Only one will be elected as BSP.
Just in case, some APIs are not in SMM yet, the BSP will send IPI to try pull those APs into SMM mode. (SMM timer is used at this moment).
Above process is called CPU rendez-vous.

Then BSP will do enter SMM_CORE entry point (registered by SmmCoreEntry), while APs are in loop state.
After BSP returns from SMM_CORE, it will let APs leave loop state, and all CPUs run RSM instruction to return back.

Hope that helps.

BTW: I am not sure how QEMU emulate SMI. Does SMI can be trigger by 0xB2 port? And CPU will run to SMBASE in real mode?


Thank you
Yao Jiewen


-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, April 22, 2015 4:32 AM
To: Paolo Bonzini; Gerd Hoffmann
Cc: edk2-devel list; qemu-devel@nongnu.org; mst@redhat.com
Subject: [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested)


 (b) This is actually the opposite argument. Since SMM is security sensitive, *and* UEFI is single-processor / single-threaded (well you can use multiple processors, you just can't run any UEFI code on anything but the BP), the timer interrupt should be blocked / masked
*anyway* while in SMM, no? Otherwise edk2 could start running a plain timer event callback in the middle of an SMM handler. Which makes me think that this locking / interrupt masking must already be in place, and it's not the responsibility of the individual
EFI_SMM_CONTROL2_PROTOCOL.Trigger() implementation. (PI 1.3 vol4 doesn't mention this responsibility in any case.)



I've learned about the "EDK II SMM Call Topology" document from a piwg
message:

http://sourceforge.net/projects/edk2/files/General%20Documentation/EDK%20II%20SMM%20call%20topology.pdf/download

It doesn't speak about masking the timer.

Does SMI mask other interrupts "architecturally" perhaps?

...

(3) Oh, this is sad. Well, I am sad. Turns out there's a third UEFI protocol that OVMF needs to implement: EFI_SMM_CONFIGURATION_PROTOCOL.
Its only interesting member is RegisterSmmEntry(), and that function is supposed to bind the central entry point (which is already available in the edk2 tree) to the processor-level SMI handler.

(I'm kind of confused, because last time I experimented with faking SMRAM / SMM in OVMF, my fake Trigger() function just returned success, and there was no EFI_SMM_CONFIGURATION_PROTOCOL at all, and things seemed to work. In retrospect I can't imagine how control was transferred at all, without actual SMM / SMI support in both QEMU and OVMF. Hm... looking at occurrences of "SmmEntryPointRegistered", this may have been intentional in edk2.)

EFI_SMM_CONFIGURATION_PROTOCOL discussed in the "EDK II SMM Call Topology" document, on the "SmmDriverDispatcher" and especially the "SMBASE Relocation" pages. It takes a separate CPU driver, and
(obviously) assembly code.

The "SMBASE Relocation" page references "IA32FamilyCpuPkg", which is not open source. Nothing in edk2 produces gEfiSmmConfigurationProtocol, and no source file contains the RSM instruction.

The Vlv2TbltDevicePkg package (ValleyView2 Tablet?) makes several references to IA32FamilyCpuPkg, but those are only references.

I guess IA32FamilyCpuPkg is exactly the kind of chipset code that Intel has not opened up.

Our "SMM in OVMF" effort just got set back by months. :(

Laszlo

------------------------------------------------------------------------------
BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT Develop your own process in accordance with the BPMN 2 standard Learn Process modeling best practices with Bonita BPM through live exercises
http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_ source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Re: [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger()

[Paolo Bonzini]

On 24/04/2015 13:56, Yao, Jiewen wrote:
> BTW: I am not sure how QEMU emulate SMI. Does SMI can be trigger by
> 0xB2 port? And CPU will run to SMBASE in real mode?

Yes, operation is the same.

Paolo

Re: [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger()

[Yao, Jiewen]

Got it. Thanks!

-----Original Message-----
From: Paolo Bonzini [mailto:pbonzini@redhat.com] 
Sent: Friday, April 24, 2015 9:01 PM
To: Yao, Jiewen; edk2-devel@lists.sourceforge.net; Gerd Hoffmann
Cc: qemu-devel@nongnu.org; mst@redhat.com
Subject: Re: [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger()



On 24/04/2015 13:56, Yao, Jiewen wrote:
> BTW: I am not sure how QEMU emulate SMI. Does SMI can be trigger by
> 0xB2 port? And CPU will run to SMBASE in real mode?

Yes, operation is the same.

Paolo

Re: [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested)

[Yao, Jiewen]

Hi Laszlo

I think there is good resource for your reference - Intel Quark.
https://downloadcenter.intel.com/download/23197

You may download "Board_Support_Package_Sources_for_Intel_Quark_v1.1.0.7z", and find "Quark_EDKII_v1.1.0"

IA32FamilyCpuBasePkg\PiSmmCpuDxeSmm - it is CPUSMM driver.
IA32FamilyCpuBasePkg\PiSmmCommunication - it is CommunicationPeim.

PiSmmCpuDxeSmm works for Quark, but I think it should be easy to port to QEMU platform.
PiSmmCommunication should be generic, it might be able to put to UefiCpuPkg later.

Thank you
Yao Jiewen

-----Original Message-----
From: Yao, Jiewen [mailto:jiewen.yao@intel.com] 
Sent: Friday, April 24, 2015 7:56 PM
To: edk2-devel@lists.sourceforge.net; Paolo Bonzini; Gerd Hoffmann
Cc: qemu-devel@nongnu.org; mst@redhat.com
Subject: Re: [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested)

Hi Laszlo
Below is my thought.

2) You are right. According to IA32 manual - "Maskable hardware interrupts, exceptions, NMI interrupts, SMI interrupts, A20M interrupts, single-step traps, breakpoint traps, and INIT operations are inhibited when the processor enters SMM." You can also find more detail in "34.6 EXCEPTIONS AND INTERRUPTS WITHIN SMM"

In below doc, the "SMM Timer is initialized" means the timer used by BSP to check if APs are all pulled into SMM for CPU Rendez-vous. A typical implementation is ACPI timer (PCH), or APIC timer (CPU). BSP just check timer count, but the timer does not generate any interrupt.

3) Yes. SmmCoreEntry is useful function as bridge between a real CPU_SMM driver and SMM_CORE.
In real world, when SMI happen, CPU will jump to SM_BASE in real mode. Then it will jump to X64 mode immediately.
The all CPUs will be in election phase. Only one will be elected as BSP.
Just in case, some APIs are not in SMM yet, the BSP will send IPI to try pull those APs into SMM mode. (SMM timer is used at this moment).
Above process is called CPU rendez-vous.

Then BSP will do enter SMM_CORE entry point (registered by SmmCoreEntry), while APs are in loop state.
After BSP returns from SMM_CORE, it will let APs leave loop state, and all CPUs run RSM instruction to return back.

Hope that helps.

BTW: I am not sure how QEMU emulate SMI. Does SMI can be trigger by 0xB2 port? And CPU will run to SMBASE in real mode?


Thank you
Yao Jiewen


-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, April 22, 2015 4:32 AM
To: Paolo Bonzini; Gerd Hoffmann
Cc: edk2-devel list; qemu-devel@nongnu.org; mst@redhat.com
Subject: [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested)


 (b) This is actually the opposite argument. Since SMM is security sensitive, *and* UEFI is single-processor / single-threaded (well you can use multiple processors, you just can't run any UEFI code on anything but the BP), the timer interrupt should be blocked / masked
*anyway* while in SMM, no? Otherwise edk2 could start running a plain timer event callback in the middle of an SMM handler. Which makes me think that this locking / interrupt masking must already be in place, and it's not the responsibility of the individual
EFI_SMM_CONTROL2_PROTOCOL.Trigger() implementation. (PI 1.3 vol4 doesn't mention this responsibility in any case.)



I've learned about the "EDK II SMM Call Topology" document from a piwg
message:

http://sourceforge.net/projects/edk2/files/General%20Documentation/EDK%20II%20SMM%20call%20topology.pdf/download

It doesn't speak about masking the timer.

Does SMI mask other interrupts "architecturally" perhaps?

...

(3) Oh, this is sad. Well, I am sad. Turns out there's a third UEFI protocol that OVMF needs to implement: EFI_SMM_CONFIGURATION_PROTOCOL.
Its only interesting member is RegisterSmmEntry(), and that function is supposed to bind the central entry point (which is already available in the edk2 tree) to the processor-level SMI handler.

(I'm kind of confused, because last time I experimented with faking SMRAM / SMM in OVMF, my fake Trigger() function just returned success, and there was no EFI_SMM_CONFIGURATION_PROTOCOL at all, and things seemed to work. In retrospect I can't imagine how control was transferred at all, without actual SMM / SMI support in both QEMU and OVMF. Hm... looking at occurrences of "SmmEntryPointRegistered", this may have been intentional in edk2.)

EFI_SMM_CONFIGURATION_PROTOCOL discussed in the "EDK II SMM Call Topology" document, on the "SmmDriverDispatcher" and especially the "SMBASE Relocation" pages. It takes a separate CPU driver, and
(obviously) assembly code.

The "SMBASE Relocation" page references "IA32FamilyCpuPkg", which is not open source. Nothing in edk2 produces gEfiSmmConfigurationProtocol, and no source file contains the RSM instruction.

The Vlv2TbltDevicePkg package (ValleyView2 Tablet?) makes several references to IA32FamilyCpuPkg, but those are only references.

I guess IA32FamilyCpuPkg is exactly the kind of chipset code that Intel has not opened up.

Our "SMM in OVMF" effort just got set back by months. :(

Laszlo

------------------------------------------------------------------------------
BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT Develop your own process in accordance with the BPMN 2 standard Learn Process modeling best practices with Bonita BPM through live exercises
http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_ source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Re: [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger()

[Laszlo Ersek]

On 04/24/15 16:50, Yao, Jiewen wrote:
> Hi Laszlo
> 
> I think there is good resource for your reference - Intel Quark.
> https://downloadcenter.intel.com/download/23197
> 
> You may download "Board_Support_Package_Sources_for_Intel_Quark_v1.1.0.7z", and find "Quark_EDKII_v1.1.0"
> 
> IA32FamilyCpuBasePkg\PiSmmCpuDxeSmm - it is CPUSMM driver.
> IA32FamilyCpuBasePkg\PiSmmCommunication - it is CommunicationPeim.
> 
> PiSmmCpuDxeSmm works for Quark, but I think it should be easy to port to QEMU platform.
> PiSmmCommunication should be generic, it might be able to put to UefiCpuPkg later.

Jiewen, thank you so much -- this is perfect. Exactly what I needed. It provides EFI_SMM_CONFIGURATION_PROTOCOL and SMM_S3_RESUME_STATE:

-rw-------. 1 13909 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/CpuS3.c
-rw-------. 1 16290 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/CpuService.c
-rw-------. 1  8100 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/CpuService.h
-rw-------. 1  7221 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/MpFuncs.S
-rw-------. 1  7072 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/MpFuncs.asm
-rw-------. 1  3304 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
-rw-------. 1  3013 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/Semaphore.c
-rw-------. 1  5470 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S
-rw-------. 1  5574 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm
-rw-------. 1 38464 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/SmiException.S
-rw-------. 1 26268 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm
-rw-------. 1  4115 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/SmmInit.S
-rw-------. 1  4331 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/SmmInit.asm
-rw-------. 1  2801 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c
-rw-------. 1  3553 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.h
-rw-------. 1 54847 Apr 24 18:32 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/MpService.c
-rwx------. 1 59571 Apr 24 18:34 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
-rw-------. 1 25929 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
-rw-------. 1  5318 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
-rw-------. 1  8619 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/SmmFeatures.c
-rw-------. 1  5062 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/SmmFeatures.h
-rw-------. 1 40893 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/SmmProfile.c
-rw-------. 1  2290 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/SmmProfile.h
-rw-------. 1  5357 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
-rw-------. 1  3391 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCpuDxeSmm/SyncTimer.c

(~360K)

and even EFI_PEI_SMM_COMMUNICATION_PPI:

-rw-------. 1 15508 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCommunication/PiSmmCommunicationPei.c
-rw-------. 1  2706 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCommunication/PiSmmCommunicationPei.inf
-rw-------. 1  1981 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCommunication/PiSmmCommunicationPrivate.h
-rw-------. 1 13522 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCommunication/PiSmmCommunicationSmm.c
-rw-------. 1  3004 Feb 16 10:55 Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/PiSmmCommunication/PiSmmCommunicationSmm.inf

and "Quark_EDKII_v1.1.0/LICENSE" is the 3-clause BSDL. Awesome.

Thanks!
Laszlo