[Qemu-devel] [PATCH 1/2] q35: implement SMRAM.D_LCK

[Qemu-devel] [PATCH 1/2] q35: implement SMRAM.D_LCK

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/q35.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 79bab15..9227489 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -268,6 +268,20 @@ 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_D_OPEN;
+        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_D_LCK;
+        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_G_SMRAME;
+        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_C_BASE_SEG_MASK;
+
+        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME;
+        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK;
+        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_T_EN;
+    }
+
     memory_region_transaction_begin();
 
     if (pd->config[MCH_HOST_BRIDGE_SMRAM] & SMRAM_D_OPEN) {
@@ -297,7 +311,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,
@@ -351,6 +364,8 @@ static void mch_reset(DeviceState *qdev)
                  MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT);
 
     d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
+    d->wmask[MCH_HOST_BRIDGE_SMRAM] = 0xff;
+    d->wmask[MCH_HOST_BRIDGE_ESMRAMC] = 0xff;
 
     mch_update(mch);
 }
-- 
1.8.3.1

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

[Gerd Hoffmann]

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

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

Re: [Qemu-devel] [PATCH 1/2] q35: implement SMRAM.D_LCK

[Paolo Bonzini]

On 14/04/2015 15:12, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci-host/q35.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 79bab15..9227489 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -268,6 +268,20 @@ 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_D_OPEN;
> +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_D_LCK;
> +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_G_SMRAME;
> +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_C_BASE_SEG_MASK;
> +
> +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME;
> +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK;
> +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_T_EN;
> +    }
> +
>      memory_region_transaction_begin();
>  
>      if (pd->config[MCH_HOST_BRIDGE_SMRAM] & SMRAM_D_OPEN) {
> @@ -297,7 +311,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,
> @@ -351,6 +364,8 @@ static void mch_reset(DeviceState *qdev)
>                   MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT);
>  
>      d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
> +    d->wmask[MCH_HOST_BRIDGE_SMRAM] = 0xff;
> +    d->wmask[MCH_HOST_BRIDGE_ESMRAMC] = 0xff;

S3, if I remember correctly, should not be able to reset D_LCK.  Does
this do the right thing?

Paolo

>  
>      mch_update(mch);
>  }
> 

Re: [Qemu-devel] [PATCH 1/2] q35: implement SMRAM.D_LCK

[Gerd Hoffmann]

On Di, 2015-04-14 at 16:35 +0200, Paolo Bonzini wrote:
> 
> On 14/04/2015 15:12, Gerd Hoffmann wrote:
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  hw/pci-host/q35.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index 79bab15..9227489 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -268,6 +268,20 @@ 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_D_OPEN;
> > +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_D_LCK;
> > +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_G_SMRAME;
> > +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_C_BASE_SEG_MASK;
> > +
> > +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME;
> > +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK;
> > +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_T_EN;
> > +    }
> > +
> >      memory_region_transaction_begin();
> >  
> >      if (pd->config[MCH_HOST_BRIDGE_SMRAM] & SMRAM_D_OPEN) {
> > @@ -297,7 +311,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,
> > @@ -351,6 +364,8 @@ static void mch_reset(DeviceState *qdev)
> >                   MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT);
> >  
> >      d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
> > +    d->wmask[MCH_HOST_BRIDGE_SMRAM] = 0xff;
> > +    d->wmask[MCH_HOST_BRIDGE_ESMRAMC] = 0xff;
> 
> S3, if I remember correctly, should not be able to reset D_LCK.

The "A Tour Beyond BIOS Implementing S3 Resume with EDKII" white paper
lists "Lock SMM. This must be done to maintain SMM integrity." as todo
list item for the edk2 resume code path (page 18).

So it seems to me it is the job of the firmware to re-lock smm after S3
(and before handing control back to the OS, obviously).

> Does
> this do the right thing?

I think so ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] q35: implement SMRAM.D_LCK

[Paolo Bonzini]

On 15/04/2015 15:58, Gerd Hoffmann wrote:
> The "A Tour Beyond BIOS Implementing S3 Resume with EDKII" white paper
> lists "Lock SMM. This must be done to maintain SMM integrity." as todo
> list item for the edk2 resume code path (page 18).
> 
> So it seems to me it is the job of the firmware to re-lock smm after S3
> (and before handing control back to the OS, obviously).

Right, however "D_LCK can be set to 1 via a normal configuration space
write but can only be cleared by a Full Reset."  A Full Reset is the
PLTRST# pin, which is asserted by the south bridge during power-up and
during CF9h reset.  S3 doesn't seem to be included.

My reading of the EDK2 whitepaper is that this may vary for other chipsets.

Paolo

Re: [Qemu-devel] [PATCH 1/2] q35: implement SMRAM.D_LCK

[Laszlo Ersek]

On 04/16/15 10:12, Paolo Bonzini wrote:
>
>
> On 15/04/2015 15:58, Gerd Hoffmann wrote:
>> The "A Tour Beyond BIOS Implementing S3 Resume with EDKII" white
>> paper lists "Lock SMM. This must be done to maintain SMM integrity."
>> as todo list item for the edk2 resume code path (page 18).

let's mark this with (*) for the end of my email

>> So it seems to me it is the job of the firmware to re-lock smm after
>> S3 (and before handing control back to the OS, obviously).
>
> Right, however "D_LCK can be set to 1 via a normal configuration space
> write but can only be cleared by a Full Reset."  A Full Reset is the
> PLTRST# pin, which is asserted by the south bridge during power-up and
> during CF9h reset.  S3 doesn't seem to be included.
>
> My reading of the EDK2 whitepaper is that this may vary for other
> chipsets.

Page 25, "PEI Instance", option "2)" applies: the S3 resume PEI module
needs to be able to open SMRAM.

>From the OVMF whitepaper
<http://people.redhat.com/~lersek/ovmf-whitepaper-c770f8c.txt>:

>     (b) In PEI, the S3 Resume PEIM (UefiCpuPkg/Universal/Acpi/S3Resume2Pei)
>         retrieves data from the LockBox.
>
>         Presumably, S3Resume2Pei should be considered an "unprivileged PEIM",
>         and the SMRAM access should be layered as seen in DXE. Unfortunately,
>         edk2 does not implement all of the layers in PEI -- the code either
>         doesn't exist, or it is not open source:
>
>   role         | DXE: protocol/module           | PEI: PPI/module
>   -------------+--------------------------------+------------------------------
>   unprivileged | any                            | S3Resume2Pei.inf
>   driver       |                                |
>   -------------+--------------------------------+------------------------------
>   command      | LIBRARY_CLASS = LockBoxLib     | LIBRARY_CLASS = LockBoxLib
>   formatting   |                                |
>   and response | SmmLockBoxDxeLib.inf           | SmmLockBoxPeiLib.inf
>   parsing      |                                |
>   -------------+--------------------------------+------------------------------
>   privilege    | EFI_SMM_COMMUNICATION_PROTOCOL | EFI_PEI_SMM_COMMUNICATION_PPI
>   separation   |                                |
>                | PiSmmCore.inf                  | missing!
>   -------------+--------------------------------+------------------------------
>   platform SMM | EFI_SMM_CONTROL2_PROTOCOL      | PEI_SMM_CONTROL_PPI
>   and SMRAM    | EFI_SMM_ACCESS2_PROTOCOL       | PEI_SMM_ACCESS_PPI
>   access       |                                |
>                | to be done in OVMF             | to be done in OVMF
>   -------------+--------------------------------+------------------------------
>   command      | LIBRARY_CLASS = LockBoxLib     | LIBRARY_CLASS = LockBoxLib
>   parsing and  |                                |
>   response     | SmmLockBoxSmmLib.inf           | missing!
>   formatting   |                                |
>   -------------+--------------------------------+------------------------------
>   privileged   | SmmLockBox.inf                 | missing!
>   LockBox      |                                |
>   driver       |                                |
>
>         Alternatively, in the future OVMF might be able to provide a LockBoxLib
>         instance (an SmmLockBoxPeiLib substitute) for S3Resume2Pei that
>         accesses SMRAM directly, eliminating the need for deeper layers in the
>         stack (that is, EFI_PEI_SMM_COMMUNICATION_PPI and deeper).
>
>         In fact, a "thin" EFI_PEI_SMM_COMMUNICATION_PPI implementation whose
>         sole Communicate() member invariably returns EFI_NOT_STARTED would
>         cause the current SmmLockBoxPeiLib library instance to directly perform
>         full-depth SMRAM access and LockBox search, obviating the "missing"
>         cells. (With reference to A Tour Beyond BIOS: Implementing S3 Resume
>         with EDK2, by Jiewen Yao and Vincent Zimmer, October 2014.)

The code flow in edk2 corresponding to the last paragraph above is:

S3RestoreConfig2()                     [UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c]
  RestoreLockBox()                     [MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c]
    SmmCommunicationPpi->Communicate() [1]
    InternalRestoreLockBoxFromSmram()  [MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c]
      SmmAccess->Open()                [2]

[1] EFI_PEI_SMM_COMMUNICATION_PPI, to be implemented in OVMF; returns
    EFI_NOT_STARTED (see the reasoning above, in both whitepapers)
[2] PEI_SMM_ACCESS_PPI, to be implemented in OVMF

The PEI_SMM_ACCESS_PPI.Open() member, called on the last line above,
runs on S3 resume, and it needs to be able to open up SMRAM. If the lock
is still in place, that won't work.


Additionally, the following language from page 18 in the Intel
whitepaper, marked above as (*):

  o Lock SMM. This must be done to maintain SMM integrity.

is not a TODO list item; it is a responsibility that the S3Resume2Pei
module already covers. See

  S3ResumeExecuteBootScript() [UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c]
    SmmAccess->Close()        [2]
    SmmAccess->Lock()         [2]


So, the hardware should clear the lock on S3 resume, then edk2 will open
and close the SMRAM regions as appropriate, and it will also re-lock
SMRAM, before
(a) it executes the S3 boot script (which might contain opcodes from
    UEFI (ie. third party) drivers),
(b) it transfers control to the OS's wakeup vector.

(In our case the S3 boot script has only one entry (an informational
no-op) anyway; the main assurance is that the OS's wakeup vector is
reached with the SMRAM re-locked.)

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 1/2] q35: implement SMRAM.D_LCK

[Michael S. Tsirkin]

On Tue, Apr 14, 2015 at 03:12:39PM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci-host/q35.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 79bab15..9227489 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -268,6 +268,20 @@ 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_D_OPEN;
> +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_D_LCK;
> +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_G_SMRAME;
> +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_C_BASE_SEG_MASK;
> +
> +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME;
> +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK;
> +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_T_EN;


I'd prefer a single statement:
	pd->wmask[MCH_HOST_BRIDGE_SMRAM] &=
		~(MCH_HOST_BRIDGE_SMRAM_D_OPEN | MCH_HOST_BRIDGE_SMRAM_D_LCK | ... )

> +    }
> +
>      memory_region_transaction_begin();
>  
>      if (pd->config[MCH_HOST_BRIDGE_SMRAM] & SMRAM_D_OPEN) {
> @@ -297,7 +311,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,
> @@ -351,6 +364,8 @@ static void mch_reset(DeviceState *qdev)
>                   MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT);
>  
>      d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
> +    d->wmask[MCH_HOST_BRIDGE_SMRAM] = 0xff;

Is this right? I see a bunch of reserved bits etc there.


> +    d->wmask[MCH_HOST_BRIDGE_ESMRAMC] = 0xff;

Doesn't this mean we need to reset this register now?

>  
>      mch_update(mch);
>  }

Don't you also need to clear D_LCK?

> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] q35: implement SMRAM.D_LCK

[Michael S. Tsirkin]

On Tue, Apr 14, 2015 at 05:41:14PM +0200, Michael S. Tsirkin wrote:
> On Tue, Apr 14, 2015 at 03:12:39PM +0200, Gerd Hoffmann wrote:
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  hw/pci-host/q35.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index 79bab15..9227489 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -268,6 +268,20 @@ 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_D_OPEN;
> > +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_D_LCK;
> > +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_G_SMRAME;
> > +        pd->wmask[MCH_HOST_BRIDGE_SMRAM] &= ~MCH_HOST_BRIDGE_SMRAM_C_BASE_SEG_MASK;
> > +
> > +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME;
> > +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK;
> > +        pd->wmask[MCH_HOST_BRIDGE_ESMRAMC] &= ~MCH_HOST_BRIDGE_ESMRAMC_T_EN;
> 
> 
> I'd prefer a single statement:
> 	pd->wmask[MCH_HOST_BRIDGE_SMRAM] &=
> 		~(MCH_HOST_BRIDGE_SMRAM_D_OPEN | MCH_HOST_BRIDGE_SMRAM_D_LCK | ... )
> 
> > +    }
> > +
> >      memory_region_transaction_begin();
> >  
> >      if (pd->config[MCH_HOST_BRIDGE_SMRAM] & SMRAM_D_OPEN) {
> > @@ -297,7 +311,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,
> > @@ -351,6 +364,8 @@ static void mch_reset(DeviceState *qdev)
> >                   MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT);
> >  
> >      d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
> > +    d->wmask[MCH_HOST_BRIDGE_SMRAM] = 0xff;
> 
> Is this right? I see a bunch of reserved bits etc there.
> 
> 
> > +    d->wmask[MCH_HOST_BRIDGE_ESMRAMC] = 0xff;

And this mask seems not to match the spec, either.

> Doesn't this mean we need to reset this register now?
> 
> >  
> >      mch_update(mch);
> >  }
> 
> Don't you also need to clear D_LCK?
> 
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] q35: implement SMRAM.D_LCK

[Gerd Hoffmann]

  Hi,

> >      d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
> > +    d->wmask[MCH_HOST_BRIDGE_SMRAM] = 0xff;
> 
> Is this right? I see a bunch of reserved bits etc there.

Restores the state we had before the guest flipped the lock bit.

Entriely possible that we should have a non-0xff wmask in the first
place, I'll look into that, but it's unrelated to lock bit handling and
thus something for another patch.

> > +    d->wmask[MCH_HOST_BRIDGE_ESMRAMC] = 0xff;
> 
> Doesn't this mean we need to reset this register now?

Again, this is something not related to the lock bit implementation,
probably the patch adding esramc support should have added this too.

I'll have a look, probably will cook up a incremental fix paolo can
squash in then.

> >  
> >      mch_update(mch);
> >  }
> 
> Don't you also need to clear D_LCK?

Setting MCH_HOST_BRIDGE_SMRAM to MCH_HOST_BRIDGE_SMRAM_DEFAULT does
that.

Also see 2/2 with the test case which shows lock+unlock works correctly.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] q35: implement SMRAM.D_LCK

[Michael S. Tsirkin]

On Wed, Apr 15, 2015 at 04:12:00PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > >      d->config[MCH_HOST_BRIDGE_SMRAM] = MCH_HOST_BRIDGE_SMRAM_DEFAULT;
> > > +    d->wmask[MCH_HOST_BRIDGE_SMRAM] = 0xff;
> > 
> > Is this right? I see a bunch of reserved bits etc there.
> 
> Restores the state we had before the guest flipped the lock bit.
> 
> Entriely possible that we should have a non-0xff wmask in the first
> place, I'll look into that, but it's unrelated to lock bit handling and
> thus something for another patch.
> > > +    d->wmask[MCH_HOST_BRIDGE_ESMRAMC] = 0xff;
> > 
> > Doesn't this mean we need to reset this register now?
> 
> Again, this is something not related to the lock bit implementation,
> probably the patch adding esramc support should have added this too.
> 
> I'll have a look, probably will cook up a incremental fix paolo can
> squash in then.

I'd prefer a complete patch to review.
Let's just set the correct wmask values directly.

> 
> > >  
> > >      mch_update(mch);
> > >  }
> > 
> > Don't you also need to clear D_LCK?
> 
> Setting MCH_HOST_BRIDGE_SMRAM to MCH_HOST_BRIDGE_SMRAM_DEFAULT does
> that.
> 
> Also see 2/2 with the test case which shows lock+unlock works correctly.
> 
> cheers,
>   Gerd
>