Re: [Qemu-devel] [PATCH 23/31] ich9: implement SMI_LOCK

[Laszlo Ersek <lersek@redhat.com>]

On 05/11/15 15:49, Paolo Bonzini wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/acpi/ich9.c         |  4 +++-
>  hw/isa/lpc_ich9.c      | 19 +++++++++++++++++++
>  include/hw/acpi/ich9.h |  1 +
>  include/hw/i386/ich9.h |  6 ++++++
>  4 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 5352e19..310aa64 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -94,7 +94,8 @@ static void ich9_smi_writel(void *opaque, hwaddr addr, uint64_t val,
>      ICH9LPCPMRegs *pm = opaque;
>      switch (addr) {
>      case 0:
> -        pm->smi_en = val;
> +        pm->smi_en &= ~pm->smi_en_wmask;
> +        pm->smi_en |= (val & pm->smi_en_wmask);
>          break;
>      }
>  }
> @@ -198,6 +199,7 @@ static void pm_reset(void *opaque)
>           * support SMM mode. */
>          pm->smi_en |= ICH9_PMIO_SMI_EN_APMC_EN;
>      }
> +    pm->smi_en_wmask = ~0;
>  
>      acpi_update_sci(&pm->acpi_regs, pm->irq);
>  }
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index dba7585..0269cfe 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -410,12 +410,28 @@ static void ich9_lpc_rcba_update(ICH9LPCState *lpc, uint32_t rbca_old)
>      }
>  }
>  
> +/* config:GEN_PMCON* */
> +static void
> +ich9_lpc_pmcon_update(ICH9LPCState *lpc)
> +{
> +    uint16_t gen_pmcon_1 = pci_get_word(lpc->d.config + ICH9_LPC_GEN_PMCON_1);
> +    uint16_t wmask;
> +
> +    if (gen_pmcon_1 & ICH9_LPC_GEN_PMCON_1_SMI_LOCK) {
> +        wmask = pci_get_word(lpc->d.wmask + ICH9_LPC_GEN_PMCON_1);
> +        wmask &= ~ICH9_LPC_GEN_PMCON_1_SMI_LOCK;
> +        pci_set_word(lpc->d.wmask + ICH9_LPC_GEN_PMCON_1, wmask);
> +        lpc->pm.smi_en_wmask &= ~1;

If I understand correctly, this makes SMI_LOCK lock down the GBL_SMI_EN
bit (and my OVMF patch that relies on that / tests it is satisfied too).

But, it doesn't seem to lock down APMC_EN. According to the ICH9 spec,
it doesn't need to -- however when we discussed this earlier (see
Message-Id: <553F4D23.3060305@redhat.com>), the idea was to lock down
APMC_EN as well. (And I don't understand why the ICH9 spec / hw
implementation doesn't lock APMC_EN; without that, APM_CNT won't
necessarily trigger an SMI.)

Thanks!
Laszlo

> +    }
> +}
> +
>  static int ich9_lpc_post_load(void *opaque, int version_id)
>  {
>      ICH9LPCState *lpc = opaque;
>  
>      ich9_lpc_pmbase_update(lpc);
>      ich9_lpc_rcba_update(lpc, 0 /* disabled ICH9_LPC_RBCA_EN */);
> +    ich9_lpc_pmcon_update(lpc);
>      return 0;
>  }
>  
> @@ -438,6 +454,9 @@ static void ich9_lpc_config_write(PCIDevice *d,
>      if (ranges_overlap(addr, len, ICH9_LPC_PIRQE_ROUT, 4)) {
>          pci_bus_fire_intx_routing_notifier(lpc->d.bus);
>      }
> +    if (ranges_overlap(addr, len, ICH9_LPC_GEN_PMCON_1, 8)) {
> +        ich9_lpc_pmcon_update(lpc);
> +    }
>  }
>  
>  static void ich9_lpc_reset(DeviceState *qdev)
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index c2d3dba..77cc65c 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -39,6 +39,7 @@ typedef struct ICH9LPCPMRegs {
>      MemoryRegion io_smi;
>  
>      uint32_t smi_en;
> +    uint32_t smi_en_wmask;
>      uint32_t smi_sts;
>  
>      qemu_irq irq;      /* SCI */
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index f4e522c..a2cc15c 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -152,6 +152,12 @@ Object *ich9_lpc_find(void);
>  #define ICH9_LPC_PIRQ_ROUT_MASK                 Q35_MASK(8, 3, 0)
>  #define ICH9_LPC_PIRQ_ROUT_DEFAULT              0x80
>  
> +#define ICH9_LPC_GEN_PMCON_1                    0xa0
> +#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK           (1 << 4)
> +#define ICH9_LPC_GEN_PMCON_2                    0xa2
> +#define ICH9_LPC_GEN_PMCON_3                    0xa4
> +#define ICH9_LPC_GEN_PMCON_LOCK                 0xa6
> +
>  #define ICH9_LPC_RCBA                           0xf0
>  #define ICH9_LPC_RCBA_BA_MASK                   Q35_MASK(32, 31, 14)
>  #define ICH9_LPC_RCBA_EN                        0x1
> 

Thanks
Laszlo