subject:"\[Qemu\-devel\] \[PATCH 23\/31\] ich9\: implement SMI

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

2015-05-12 Thread Gerd Hoffmann

On Mo, 2015-05-11 at 15:49 +0200, Paolo Bonzini wrote:
 From: Gerd Hoffmann kra...@redhat.com

[ more verbose commit message for squashing in ]

Add write mask for the smi enable register, so we can disable write
access to certain bits.  Open all bits on reset.  Disable write access
to GBL_SMI_EN when SMI_LOCK (in ich9 lpc pci config space) is set.
Write access to SMI_LOCK itself is disabled too.

 Signed-off-by: Gerd Hoffmann kra...@redhat.com
 Signed-off-by: Paolo Bonzini pbonz...@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;
 +}
 +}
 +
  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_10xa0
 +#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK   (1  4)
 +#define ICH9_LPC_GEN_PMCON_20xa2
 +#define ICH9_LPC_GEN_PMCON_30xa4
 +#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_EN0x1

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

2015-05-11 Thread Paolo Bonzini

From: Gerd Hoffmann kra...@redhat.com

Signed-off-by: Gerd Hoffmann kra...@redhat.com
Signed-off-by: Paolo Bonzini pbonz...@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;
+}
+}
+
 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_10xa0
+#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK   (1  4)
+#define ICH9_LPC_GEN_PMCON_20xa2
+#define ICH9_LPC_GEN_PMCON_30xa4
+#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_EN0x1
-- 
1.8.3.1

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

2015-05-11 Thread Laszlo Ersek

On 05/11/15 15:49, Paolo Bonzini wrote:
 From: Gerd Hoffmann kra...@redhat.com
 
 Signed-off-by: Gerd Hoffmann kra...@redhat.com
 Signed-off-by: Paolo Bonzini pbonz...@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.3060...@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_10xa0
 +#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK   (1  4)
 +#define ICH9_LPC_GEN_PMCON_20xa2
 +#define ICH9_LPC_GEN_PMCON_30xa4
 +#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_EN0x1
 

Thanks
Laszlo

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

2015-05-11 Thread Paolo Bonzini



On 11/05/2015 17:17, Laszlo Ersek wrote:
 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.3060...@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.)

I don't think it should.  See here
https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02758.html
where I wrote explicitly Even if the OS tries to maliciously set
APMC_EN to 0 (SMI_LOCK doesn't lock APMC_EN)

Paolo

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

2015-05-11 Thread Laszlo Ersek

On 05/11/15 17:21, Paolo Bonzini wrote:
 
 
 On 11/05/2015 17:17, Laszlo Ersek wrote:
 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.3060...@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.)
 
 I don't think it should.  See here
 https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02758.html
 where I wrote explicitly Even if the OS tries to maliciously set
 APMC_EN to 0 (SMI_LOCK doesn't lock APMC_EN)

It's not about feature detection -- the question (from
553f4d23.3060...@redhat.com) is whether I should set APMC_EN myself
*every time* before writing to APM_CNT, in the
EFI_SMM_CONTROL2_PROTOCOL.Trigger() method. That protocol is provided by
a runtime DXE driver and would be exercised by eg. the non-privileged
half of the runtime variable service driver.

It's no problem to set it, I have the code ready, I was just wondering
if I should keep that hunk. (In fact it might not even matter: if the OS
interferes and clears APMC_EN before the non-privileged half mentioned
above manages to raise the SMI, then the call / transition to SMM will
simply not happen, which is bad for the OS, and probably irrelevant for
the firmware (... the security thereof).)

I guess I'll keep re-setting APMC_EN in the Trigger() method; it won't hurt.

Thanks
Laszlo

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

2015-05-11 Thread Paolo Bonzini



On 11/05/2015 17:36, Laszlo Ersek wrote:
 It's not about feature detection -- the question (from
 553f4d23.3060...@redhat.com) is whether I should set APMC_EN myself
 *every time* before writing to APM_CNT, in the
 EFI_SMM_CONTROL2_PROTOCOL.Trigger() method. That protocol is provided by
 a runtime DXE driver and would be exercised by eg. the non-privileged
 half of the runtime variable service driver.

Oh sorry, I couldn't find that message ID.

 It's no problem to set it, I have the code ready, I was just wondering
 if I should keep that hunk. (In fact it might not even matter: if the OS
 interferes and clears APMC_EN before the non-privileged half mentioned
 above manages to raise the SMI, then the call / transition to SMM will
 simply not happen, which is bad for the OS, and probably irrelevant for
 the firmware (... the security thereof).)

The OS can also race against you and clear APMC_EN, so it's even
unnecessary to reset it.

Paolo

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

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

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

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

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

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

6 matches

Site Navigation

Mail list logo

Footer information