* [Qemu-devel] [PATCH 2/6] add SMRAM+ESMRAMC wmask
2015-04-20 9:19 [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default Gerd Hoffmann
@ 2015-04-20 9:19 ` Gerd Hoffmann
2015-04-20 12:05 ` Michael S. Tsirkin
2015-04-20 9:19 ` [Qemu-devel] [PATCH 3/6] q35: implement SMRAM.D_LCK Gerd Hoffmann
` (4 subsequent siblings)
5 siblings, 1 reply; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-20 9:19 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, lersek, Gerd Hoffmann, mst
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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] add SMRAM+ESMRAMC wmask
2015-04-20 9:19 ` [Qemu-devel] [PATCH 2/6] add SMRAM+ESMRAMC wmask Gerd Hoffmann
@ 2015-04-20 12:05 ` Michael S. Tsirkin
0 siblings, 0 replies; 43+ messages in thread
From: Michael S. Tsirkin @ 2015-04-20 12:05 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, lersek, qemu-devel
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 3/6] q35: implement SMRAM.D_LCK
2015-04-20 9:19 [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default Gerd Hoffmann
2015-04-20 9:19 ` [Qemu-devel] [PATCH 2/6] add SMRAM+ESMRAMC wmask Gerd Hoffmann
@ 2015-04-20 9:19 ` Gerd Hoffmann
2015-04-20 12:06 ` Michael S. Tsirkin
2015-04-20 9:19 ` [Qemu-devel] [PATCH 4/6] q35: add test for SMRAM.D_LCK Gerd Hoffmann
` (3 subsequent siblings)
5 siblings, 1 reply; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-20 9:19 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, lersek, Gerd Hoffmann, mst
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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 3/6] q35: implement SMRAM.D_LCK
2015-04-20 9:19 ` [Qemu-devel] [PATCH 3/6] q35: implement SMRAM.D_LCK Gerd Hoffmann
@ 2015-04-20 12:06 ` Michael S. Tsirkin
0 siblings, 0 replies; 43+ messages in thread
From: Michael S. Tsirkin @ 2015-04-20 12:06 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, lersek, qemu-devel
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 4/6] q35: add test for SMRAM.D_LCK
2015-04-20 9:19 [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default Gerd Hoffmann
2015-04-20 9:19 ` [Qemu-devel] [PATCH 2/6] add SMRAM+ESMRAMC wmask Gerd Hoffmann
2015-04-20 9:19 ` [Qemu-devel] [PATCH 3/6] q35: implement SMRAM.D_LCK Gerd Hoffmann
@ 2015-04-20 9:19 ` Gerd Hoffmann
2015-04-20 12:06 ` Michael S. Tsirkin
2015-04-20 9:19 ` [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested Gerd Hoffmann
` (2 subsequent siblings)
5 siblings, 1 reply; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-20 9:19 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, lersek, Gerd Hoffmann, mst
---
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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 4/6] q35: add test for SMRAM.D_LCK
2015-04-20 9:19 ` [Qemu-devel] [PATCH 4/6] q35: add test for SMRAM.D_LCK Gerd Hoffmann
@ 2015-04-20 12:06 ` Michael S. Tsirkin
0 siblings, 0 replies; 43+ messages in thread
From: Michael S. Tsirkin @ 2015-04-20 12:06 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, lersek, qemu-devel
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-20 9:19 [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default Gerd Hoffmann
` (2 preceding siblings ...)
2015-04-20 9:19 ` [Qemu-devel] [PATCH 4/6] q35: add test for SMRAM.D_LCK Gerd Hoffmann
@ 2015-04-20 9:19 ` Gerd Hoffmann
2015-04-20 11:45 ` Paolo Bonzini
2015-04-21 14:18 ` Laszlo Ersek
2015-04-20 9:19 ` [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, " Gerd Hoffmann
2015-04-20 12:07 ` [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default Michael S. Tsirkin
5 siblings, 2 replies; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-20 9:19 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, lersek, Gerd Hoffmann, mst
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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-20 9:19 ` [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested Gerd Hoffmann
@ 2015-04-20 11:45 ` Paolo Bonzini
2015-04-21 14:18 ` Laszlo Ersek
1 sibling, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2015-04-20 11:45 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel; +Cc: lersek, mst
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);
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-20 9:19 ` [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested Gerd Hoffmann
2015-04-20 11:45 ` Paolo Bonzini
@ 2015-04-21 14:18 ` Laszlo Ersek
2015-04-21 15:04 ` Gerd Hoffmann
1 sibling, 1 reply; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-21 14:18 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-21 14:18 ` Laszlo Ersek
@ 2015-04-21 15:04 ` Gerd Hoffmann
2015-04-21 15:08 ` Paolo Bonzini
` (3 more replies)
0 siblings, 4 replies; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-21 15:04 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: pbonzini, qemu-devel, mst
> > +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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-21 15:04 ` Gerd Hoffmann
@ 2015-04-21 15:08 ` Paolo Bonzini
2015-04-21 15:16 ` Gerd Hoffmann
2015-04-21 18:46 ` Laszlo Ersek
` (2 subsequent siblings)
3 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2015-04-21 15:08 UTC (permalink / raw)
To: Gerd Hoffmann, Laszlo Ersek; +Cc: qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-21 15:08 ` Paolo Bonzini
@ 2015-04-21 15:16 ` Gerd Hoffmann
0 siblings, 0 replies; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-21 15:16 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Laszlo Ersek, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-21 15:04 ` Gerd Hoffmann
2015-04-21 15:08 ` Paolo Bonzini
@ 2015-04-21 18:46 ` Laszlo Ersek
2015-04-22 6:07 ` Gerd Hoffmann
2015-04-22 8:09 ` Gerd Hoffmann
2015-04-22 21:41 ` Laszlo Ersek
3 siblings, 1 reply; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-21 18:46 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-21 15:04 ` Gerd Hoffmann
2015-04-21 15:08 ` Paolo Bonzini
2015-04-21 18:46 ` Laszlo Ersek
@ 2015-04-22 8:09 ` Gerd Hoffmann
2015-04-22 8:52 ` Laszlo Ersek
2015-04-22 21:41 ` Laszlo Ersek
3 siblings, 1 reply; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-22 8:09 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: pbonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-22 8:09 ` Gerd Hoffmann
@ 2015-04-22 8:52 ` Laszlo Ersek
2015-04-22 9:33 ` Gerd Hoffmann
0 siblings, 1 reply; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-22 8:52 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-22 8:52 ` Laszlo Ersek
@ 2015-04-22 9:33 ` Gerd Hoffmann
0 siblings, 0 replies; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-22 9:33 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: pbonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-21 15:04 ` Gerd Hoffmann
` (2 preceding siblings ...)
2015-04-22 8:09 ` Gerd Hoffmann
@ 2015-04-22 21:41 ` Laszlo Ersek
2015-04-22 21:51 ` Laszlo Ersek
3 siblings, 1 reply; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-22 21:41 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, qemu-devel, mst
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
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-22 21:41 ` Laszlo Ersek
@ 2015-04-22 21:51 ` Laszlo Ersek
2015-04-23 7:02 ` Gerd Hoffmann
0 siblings, 1 reply; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-22 21:51 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-22 21:51 ` Laszlo Ersek
@ 2015-04-23 7:02 ` Gerd Hoffmann
2015-04-23 7:41 ` Laszlo Ersek
2015-04-23 10:27 ` Paolo Bonzini
0 siblings, 2 replies; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-23 7:02 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: pbonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-23 7:02 ` Gerd Hoffmann
@ 2015-04-23 7:41 ` Laszlo Ersek
2015-04-23 8:33 ` Laszlo Ersek
2015-04-23 8:34 ` Gerd Hoffmann
2015-04-23 10:27 ` Paolo Bonzini
1 sibling, 2 replies; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-23 7:41 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-23 7:41 ` Laszlo Ersek
@ 2015-04-23 8:33 ` Laszlo Ersek
2015-04-23 8:34 ` Gerd Hoffmann
1 sibling, 0 replies; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-23 8:33 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, qemu-devel, Jordan Justen (Intel address), mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-23 7:41 ` Laszlo Ersek
2015-04-23 8:33 ` Laszlo Ersek
@ 2015-04-23 8:34 ` Gerd Hoffmann
2015-04-23 8:42 ` Laszlo Ersek
1 sibling, 1 reply; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-23 8:34 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: pbonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-23 8:34 ` Gerd Hoffmann
@ 2015-04-23 8:42 ` Laszlo Ersek
0 siblings, 0 replies; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-23 8:42 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested
2015-04-23 7:02 ` Gerd Hoffmann
2015-04-23 7:41 ` Laszlo Ersek
@ 2015-04-23 10:27 ` Paolo Bonzini
1 sibling, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2015-04-23 10:27 UTC (permalink / raw)
To: Gerd Hoffmann, Laszlo Ersek; +Cc: qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested
2015-04-20 9:19 [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default Gerd Hoffmann
` (3 preceding siblings ...)
2015-04-20 9:19 ` [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested Gerd Hoffmann
@ 2015-04-20 9:19 ` Gerd Hoffmann
2015-04-21 14:30 ` Laszlo Ersek
2015-04-20 12:07 ` [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default Michael S. Tsirkin
5 siblings, 1 reply; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-20 9:19 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, lersek, Gerd Hoffmann, mst
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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested
2015-04-20 9:19 ` [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, " Gerd Hoffmann
@ 2015-04-21 14:30 ` Laszlo Ersek
2015-04-21 14:38 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-21 14:30 UTC (permalink / raw)
To: Gerd Hoffmann, pbonzini; +Cc: qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested
2015-04-21 14:30 ` Laszlo Ersek
@ 2015-04-21 14:38 ` Paolo Bonzini
2015-04-21 15:05 ` Laszlo Ersek
2015-04-21 15:12 ` [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested Gerd Hoffmann
0 siblings, 2 replies; 43+ messages in thread
From: Paolo Bonzini @ 2015-04-21 14:38 UTC (permalink / raw)
To: Laszlo Ersek, Gerd Hoffmann; +Cc: qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested
2015-04-21 14:38 ` Paolo Bonzini
@ 2015-04-21 15:05 ` Laszlo Ersek
2015-04-21 15:14 ` Gerd Hoffmann
2015-04-21 15:21 ` Paolo Bonzini
2015-04-21 15:12 ` [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested Gerd Hoffmann
1 sibling, 2 replies; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-21 15:05 UTC (permalink / raw)
To: Paolo Bonzini, Gerd Hoffmann; +Cc: qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested
2015-04-21 15:05 ` Laszlo Ersek
@ 2015-04-21 15:14 ` Gerd Hoffmann
2015-04-21 15:21 ` Paolo Bonzini
1 sibling, 0 replies; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-21 15:14 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Paolo Bonzini, qemu-devel, mst
> > 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested
2015-04-21 15:05 ` Laszlo Ersek
2015-04-21 15:14 ` Gerd Hoffmann
@ 2015-04-21 15:21 ` Paolo Bonzini
2015-04-21 20:31 ` [Qemu-devel] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested) Laszlo Ersek
1 sibling, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2015-04-21 15:21 UTC (permalink / raw)
To: Laszlo Ersek, Gerd Hoffmann; +Cc: qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested)
2015-04-21 15:21 ` Paolo Bonzini
@ 2015-04-21 20:31 ` Laszlo Ersek
2015-04-21 20:58 ` [Qemu-devel] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() Paolo Bonzini
2015-04-24 11:56 ` [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested) Yao, Jiewen
0 siblings, 2 replies; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-21 20:31 UTC (permalink / raw)
To: Paolo Bonzini, Gerd Hoffmann; +Cc: edk2-devel list, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger()
2015-04-21 20:31 ` [Qemu-devel] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested) Laszlo Ersek
@ 2015-04-21 20:58 ` Paolo Bonzini
2015-04-24 11:56 ` [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested) Yao, Jiewen
1 sibling, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2015-04-21 20:58 UTC (permalink / raw)
To: Laszlo Ersek, Gerd Hoffmann; +Cc: edk2-devel list, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested)
2015-04-21 20:31 ` [Qemu-devel] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested) Laszlo Ersek
2015-04-21 20:58 ` [Qemu-devel] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() Paolo Bonzini
@ 2015-04-24 11:56 ` Yao, Jiewen
2015-04-24 13:00 ` [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() Paolo Bonzini
2015-04-24 14:50 ` [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested) Yao, Jiewen
1 sibling, 2 replies; 43+ messages in thread
From: Yao, Jiewen @ 2015-04-24 11:56 UTC (permalink / raw)
To: edk2-devel, Paolo Bonzini, Gerd Hoffmann; +Cc: qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger()
2015-04-24 11:56 ` [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested) Yao, Jiewen
@ 2015-04-24 13:00 ` Paolo Bonzini
2015-04-24 13:16 ` Yao, Jiewen
2015-04-24 14:50 ` [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested) Yao, Jiewen
1 sibling, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2015-04-24 13:00 UTC (permalink / raw)
To: Yao, Jiewen, edk2-devel, Gerd Hoffmann; +Cc: qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger()
2015-04-24 13:00 ` [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() Paolo Bonzini
@ 2015-04-24 13:16 ` Yao, Jiewen
0 siblings, 0 replies; 43+ messages in thread
From: Yao, Jiewen @ 2015-04-24 13:16 UTC (permalink / raw)
To: Paolo Bonzini, edk2-devel, Gerd Hoffmann; +Cc: qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested)
2015-04-24 11:56 ` [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested) Yao, Jiewen
2015-04-24 13:00 ` [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() Paolo Bonzini
@ 2015-04-24 14:50 ` Yao, Jiewen
2015-04-24 16:46 ` [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() Laszlo Ersek
1 sibling, 1 reply; 43+ messages in thread
From: Yao, Jiewen @ 2015-04-24 14:50 UTC (permalink / raw)
To: edk2-devel, Paolo Bonzini, Gerd Hoffmann; +Cc: qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger()
2015-04-24 14:50 ` [Qemu-devel] [edk2] implementing EFI_SMM_CONTROL2_PROTOCOL.Trigger() (was: [PATCH 6/6] [wip] tseg, part2, not (yet) tested) Yao, Jiewen
@ 2015-04-24 16:46 ` Laszlo Ersek
0 siblings, 0 replies; 43+ messages in thread
From: Laszlo Ersek @ 2015-04-24 16:46 UTC (permalink / raw)
To: Yao Jiewen; +Cc: Paolo Bonzini, edk2-devel, mst, Gerd Hoffmann, qemu-devel
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, not (yet) tested
2015-04-21 14:38 ` Paolo Bonzini
2015-04-21 15:05 ` Laszlo Ersek
@ 2015-04-21 15:12 ` Gerd Hoffmann
1 sibling, 0 replies; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-21 15:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Laszlo Ersek, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default
2015-04-20 9:19 [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default Gerd Hoffmann
` (4 preceding siblings ...)
2015-04-20 9:19 ` [Qemu-devel] [PATCH 6/6] [wip] tseg, part2, " Gerd Hoffmann
@ 2015-04-20 12:07 ` Michael S. Tsirkin
2015-04-20 12:27 ` Paolo Bonzini
5 siblings, 1 reply; 43+ messages in thread
From: Michael S. Tsirkin @ 2015-04-20 12:07 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, lersek, qemu-devel
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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default
2015-04-20 12:07 ` [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default Michael S. Tsirkin
@ 2015-04-20 12:27 ` Paolo Bonzini
2015-04-20 13:23 ` Gerd Hoffmann
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2015-04-20 12:27 UTC (permalink / raw)
To: Michael S. Tsirkin, Gerd Hoffmann; +Cc: lersek, qemu-devel
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] [fixup] add ESMRAMC default
2015-04-20 12:27 ` Paolo Bonzini
@ 2015-04-20 13:23 ` Gerd Hoffmann
0 siblings, 0 replies; 43+ messages in thread
From: Gerd Hoffmann @ 2015-04-20 13:23 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: lersek, qemu-devel, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 43+ messages in thread