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

[Gerd Hoffmann <kraxel@redhat.com>]

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