[Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject SMI on all VCPUs if APM_STS == 'Q'

[Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject SMI on all VCPUs if APM_STS == 'Q'

[Laszlo Ersek]

The generic edk2 SMM infrastructure prefers
EFI_SMM_CONTROL2_PROTOCOL.Trigger() to inject an SMI on each processor. If
Trigger() only brings the current processor into SMM, then edk2 handles it
in the following ways:

(1) If Trigger() is executed by the BSP (which is guaranteed before
    ExitBootServices(), but is not necessarily true at runtime), then:

    (a) If edk2 has been configured for "traditional" SMM synchronization,
        then the BSP sends directed SMIs to the APs with APIC delivery,
        bringing them into SMM individually. Then the BSP runs the SMI
        handler / dispatcher.

    (b) If edk2 has been configured for "relaxed" SMM synchronization,
        then the APs that are not already in SMM are not brought in, and
        the BSP runs the SMI handler / dispatcher.

(2) If Trigger() is executed by an AP (which is possible after
    ExitBootServices(), and can be forced e.g. by "taskset -c 1
    efibootmgr"), then the AP in question brings in the BSP with a
    directed SMI, and the BSP runs the SMI handler / dispatcher.

The problem with (1a) and (2) is that the BSP and AP synchronization is
slow. The above taskset + efibootmgr command takes more than 30 seconds to
complete on TCG, for example, because efibootmgr accesses non-volatile
UEFI variables intensively.

Therefore introduce a special APM_STS value (0x51) that causes QEMU to
inject the SMI on all VCPUs. OVMF's EFI_SMM_CONTROL2_PROTOCOL.Trigger()
can utilize this to accommodate edk2's preference about "broadcast" SMI.

SeaBIOS uses values 0x00 and 0x01 for APM_STS (called PORT_SMI_STATUS in
the SeaBIOS code), so this change should be transparent to it.

While commit 3c23402d4032 targeted correctness, this one aims at better
performance only.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: "Kevin O'Connor" <kevin@koconnor.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/isa/lpc_ich9.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 1ffc803..117baff 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -380,6 +380,8 @@ void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled, bool enable_tco)
 
 /* APM */
 
+#define QEMU_ICH9_APM_STS_BROADCAST_SMI 'Q'
+
 static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
 {
     ICH9LPCState *lpc = arg;
@@ -394,7 +396,15 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
 
     /* SMI_EN = PMBASE + 30. SMI control and enable register */
     if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN) {
-        cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+        if (lpc->apm.apms == QEMU_ICH9_APM_STS_BROADCAST_SMI) {
+            CPUState *cs;
+
+            CPU_FOREACH(cs) {
+                cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+            }
+        } else {
+            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+        }
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject SMI on all VCPUs if APM_STS == 'Q'

[Laszlo Ersek]

On 10/23/15 18:05, Laszlo Ersek wrote:
> The generic edk2 SMM infrastructure prefers
> EFI_SMM_CONTROL2_PROTOCOL.Trigger() to inject an SMI on each processor. If
> Trigger() only brings the current processor into SMM, then edk2 handles it
> in the following ways:
> 
> (1) If Trigger() is executed by the BSP (which is guaranteed before
>     ExitBootServices(), but is not necessarily true at runtime), then:
> 
>     (a) If edk2 has been configured for "traditional" SMM synchronization,
>         then the BSP sends directed SMIs to the APs with APIC delivery,
>         bringing them into SMM individually. Then the BSP runs the SMI
>         handler / dispatcher.
> 
>     (b) If edk2 has been configured for "relaxed" SMM synchronization,
>         then the APs that are not already in SMM are not brought in, and
>         the BSP runs the SMI handler / dispatcher.
> 
> (2) If Trigger() is executed by an AP (which is possible after
>     ExitBootServices(), and can be forced e.g. by "taskset -c 1
>     efibootmgr"), then the AP in question brings in the BSP with a
>     directed SMI, and the BSP runs the SMI handler / dispatcher.
> 
> The problem with (1a) and (2) is that the BSP and AP synchronization is
> slow. The above taskset + efibootmgr command takes more than 30 seconds to
> complete on TCG, for example, because efibootmgr accesses non-volatile
> UEFI variables intensively.
> 
> Therefore introduce a special APM_STS value (0x51) that causes QEMU to
> inject the SMI on all VCPUs. OVMF's EFI_SMM_CONTROL2_PROTOCOL.Trigger()
> can utilize this to accommodate edk2's preference about "broadcast" SMI.
> 
> SeaBIOS uses values 0x00 and 0x01 for APM_STS (called PORT_SMI_STATUS in
> the SeaBIOS code), so this change should be transparent to it.
> 
> While commit 3c23402d4032 targeted correctness, this one aims at better
> performance only.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: "Kevin O'Connor" <kevin@koconnor.net>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/isa/lpc_ich9.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 1ffc803..117baff 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -380,6 +380,8 @@ void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled, bool enable_tco)
>  
>  /* APM */
>  
> +#define QEMU_ICH9_APM_STS_BROADCAST_SMI 'Q'
> +
>  static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>  {
>      ICH9LPCState *lpc = arg;
> @@ -394,7 +396,15 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>  
>      /* SMI_EN = PMBASE + 30. SMI control and enable register */
>      if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN) {
> -        cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> +        if (lpc->apm.apms == QEMU_ICH9_APM_STS_BROADCAST_SMI) {
> +            CPUState *cs;
> +
> +            CPU_FOREACH(cs) {
> +                cpu_interrupt(cs, CPU_INTERRUPT_SMI);
> +            }
> +        } else {
> +            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> +        }
>      }
>  }
>  
> 

Can we please continue the discussion on this? Here's my mental image of
the most recent opinions:
- Paolo doesn't like the idea that QEMU raise the interrupt on all
  VCPUs. He suggests to do it in the firmware, somewhere.
- Mike proposed SmmCpuFeaturesRendezvousEntry() in SmmCpuFeaturesLib
  wrt. "somewhere".
- Jordan likes the idea that QEMU raise the interrupt on all VCPUs, but
  he doesn't like this exact method of requesting the broadcast.

I like the idea that QEMU raise the interrupt on all VCPUs. I'd be happy
to work on a knob different from APM_STS to enable the broadcast.

But I'm also open to implementing it in the firmware, if we can find a
reliable and fast way.

Given that I don't have a crystallized preference for either approach,
can you guys please work out an agreement, and tell me what to
implement? I'd just like to move forward with this.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject SMI on all VCPUs if APM_STS == 'Q'

[Laszlo Ersek]

On 10/23/15 18:05, Laszlo Ersek wrote:
> The generic edk2 SMM infrastructure prefers
> EFI_SMM_CONTROL2_PROTOCOL.Trigger() to inject an SMI on each processor. If
> Trigger() only brings the current processor into SMM, then edk2 handles it
> in the following ways:
> 
> (1) If Trigger() is executed by the BSP (which is guaranteed before
>     ExitBootServices(), but is not necessarily true at runtime), then:
> 
>     (a) If edk2 has been configured for "traditional" SMM synchronization,
>         then the BSP sends directed SMIs to the APs with APIC delivery,
>         bringing them into SMM individually. Then the BSP runs the SMI
>         handler / dispatcher.
> 
>     (b) If edk2 has been configured for "relaxed" SMM synchronization,
>         then the APs that are not already in SMM are not brought in, and
>         the BSP runs the SMI handler / dispatcher.
> 
> (2) If Trigger() is executed by an AP (which is possible after
>     ExitBootServices(), and can be forced e.g. by "taskset -c 1
>     efibootmgr"), then the AP in question brings in the BSP with a
>     directed SMI, and the BSP runs the SMI handler / dispatcher.
> 
> The problem with (1a) and (2) is that the BSP and AP synchronization is
> slow. The above taskset + efibootmgr command takes more than 30 seconds to
> complete on TCG, for example, because efibootmgr accesses non-volatile
> UEFI variables intensively.
> 
> Therefore introduce a special APM_STS value (0x51) that causes QEMU to
> inject the SMI on all VCPUs. OVMF's EFI_SMM_CONTROL2_PROTOCOL.Trigger()
> can utilize this to accommodate edk2's preference about "broadcast" SMI.
> 
> SeaBIOS uses values 0x00 and 0x01 for APM_STS (called PORT_SMI_STATUS in
> the SeaBIOS code), so this change should be transparent to it.
> 
> While commit 3c23402d4032 targeted correctness, this one aims at better
> performance only.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: "Kevin O'Connor" <kevin@koconnor.net>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/isa/lpc_ich9.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 1ffc803..117baff 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -380,6 +380,8 @@ void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled, bool enable_tco)
>  
>  /* APM */
>  
> +#define QEMU_ICH9_APM_STS_BROADCAST_SMI 'Q'
> +
>  static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>  {
>      ICH9LPCState *lpc = arg;
> @@ -394,7 +396,15 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>  
>      /* SMI_EN = PMBASE + 30. SMI control and enable register */
>      if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN) {
> -        cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> +        if (lpc->apm.apms == QEMU_ICH9_APM_STS_BROADCAST_SMI) {
> +            CPUState *cs;
> +
> +            CPU_FOREACH(cs) {
> +                cpu_interrupt(cs, CPU_INTERRUPT_SMI);
> +            }
> +        } else {
> +            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> +        }
>      }
>  }
>  
> 

I'm withdrawing this patch for now.

Thanks
Laszlo