[Qemu-devel] [PATCH v7 wave 2 0/3] q35: add negotiable broadcast SMI

[Qemu-devel] [PATCH v7 wave 2 0/3] q35: add negotiable broadcast SMI

[Laszlo Ersek]

Hi Paolo,

I'm sending v7 with no functional changes; I've only picked up Igor's
R-b's from v6 so you don't have to, and while rebasing to current
master, I resolved a conflict in patch #3 against recent commit
abc62c89f319 ("pc.h: move x-mach-use-reliable-get-clock compat entry to
PC_COMPAT_2_8", 2017-01-18).

The series is fully reviewed and is mergeable as-is.

v6 wave 2 was at
<http://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg02424.html>.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>

Thanks
Laszlo

Laszlo Ersek (3):
  hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg
  hw/isa/lpc_ich9: add broadcast SMI feature
  hw/isa/lpc_ich9: negotiate SMI broadcast on pc-q35-2.9+ machine types

 include/hw/i386/ich9.h | 13 ++++++++
 include/hw/i386/pc.h   |  6 ++++
 hw/isa/lpc_ich9.c      | 91 +++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 109 insertions(+), 1 deletion(-)

-- 
2.9.3

[Qemu-devel] [PATCH v7 wave 2 1/3] hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg

[Laszlo Ersek]

Introduce the following fw_cfg files:

- "etc/smi/supported-features": a little endian uint64_t feature bitmap,
  presenting the features known by the host to the guest. Read-only for
  the guest.

  The content of this file will be determined via bit-granularity ICH9-LPC
  device properties, to be introduced later. For now, the bitmask is left
  zeroed. The bits will be set from machine type compat properties and on
  the QEMU command line, hence this file is not migrated.

- "etc/smi/requested-features": a little endian uint64_t feature bitmap,
  representing the features the guest would like to request. Read-write
  for the guest.

  The guest can freely (re)write this file, it has no direct consequence.
  Initial value is zero. A nonzero value causes the SMI-related fw_cfg
  files and fields that are under guest influence to be migrated.

- "etc/smi/features-ok": contains a uint8_t value, and it is read-only for
  the guest. When the guest selects the associated fw_cfg key, the guest
  features are validated against the host features. In case of error, the
  negotiation doesn't proceed, and the "features-ok" file remains zero. In
  case of success, the "features-ok" file becomes (uint8_t)1, and the
  negotiated features are locked down internally (to which no further
  changes are possible until reset).

  The initial value is zero.  A nonzero value causes the SMI-related
  fw_cfg files and fields that are under guest influence to be migrated.

The C-language fields backing the "supported-features" and
"requested-features" files are uint8_t arrays. This is because they carry
guest-side representation (our choice is little endian), while
VMSTATE_UINT64() assumes / implies host-side endianness for any uint64_t
fields. If we migrate a guest between hosts with different endiannesses
(which is possible with TCG), then the host-side value is preserved, and
the host-side representation is translated. This would be visible to the
guest through fw_cfg, unless we used plain byte arrays. So we do.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---

Notes:
    v7:
    - no changes, pick up Igor's R-b
    
    v6:
    - no changes, pick up Michael's R-b
    
    v5:
    - rename the "etc/smi/host-features" fw_cfg file to
      "etc/smi/supported-features" [Igor]
    
    - rename the "etc/smi/guest-features" fw_cfg file to
      "etc/smi/requested-features" [Igor]
    
    - suffix the names of the "ICH9LPCState.smi_host_features" and
      "ICH9LPCState.smi_guest_features" array fields with "_le" for
      representing their guest-visible encoding [Igor]
    
    - Replace the "smi_host_features" parameter of ich9_lpc_pm_init() --
      which was meant in v4 to be set by  board code -- with a new
      "ICH9LPCState.smi_host_features" field, of type uint64_t.
      Bit-granularity ICH9-LPC device properties will be carved out of this
      field. [Igor]
    
    - Given the "ICH9LPCState.smi_host_features" uint64_t field, we can now
      use that directly for feature validation in
      smi_features_ok_callback(). Converting the (otherwise guest-read-only)
      "ICH9LPCState.smi_host_features_le" array back to CPU endianness just
      for this is no longer necessary.

 include/hw/i386/ich9.h | 10 +++++++
 hw/isa/lpc_ich9.c      | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index 5fd7e97d2347..da1118727146 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -64,6 +64,16 @@ typedef struct ICH9LPCState {
     uint8_t rst_cnt;
     MemoryRegion rst_cnt_mem;
 
+    /* SMI feature negotiation via fw_cfg */
+    uint64_t smi_host_features;       /* guest-invisible, host endian */
+    uint8_t smi_host_features_le[8];  /* guest-visible, read-only, little
+                                       * endian uint64_t */
+    uint8_t smi_guest_features_le[8]; /* guest-visible, read-write, little
+                                       * endian uint64_t */
+    uint8_t smi_features_ok;          /* guest-visible, read-only; selecting it
+                                       * triggers feature lockdown */
+    uint64_t smi_negotiated_features; /* guest-invisible, host endian */
+
     /* isa bus */
     ISABus *isa_bus;
     MemoryRegion rcrb_mem; /* root complex register block */
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 10d1ee8b9310..376b7801a42c 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -48,6 +48,8 @@
 #include "exec/address-spaces.h"
 #include "sysemu/sysemu.h"
 #include "qom/cpu.h"
+#include "hw/nvram/fw_cfg.h"
+#include "qemu/cutils.h"
 
 /*****************************************************************************/
 /* ICH9 LPC PCI to ISA bridge */
@@ -360,13 +362,62 @@ static void ich9_set_sci(void *opaque, int irq_num, int level)
     }
 }
 
+static void smi_features_ok_callback(void *opaque)
+{
+    ICH9LPCState *lpc = opaque;
+    uint64_t guest_features;
+
+    if (lpc->smi_features_ok) {
+        /* negotiation already complete, features locked */
+        return;
+    }
+
+    memcpy(&guest_features, lpc->smi_guest_features_le, sizeof guest_features);
+    le64_to_cpus(&guest_features);
+    if (guest_features & ~lpc->smi_host_features) {
+        /* guest requests invalid features, leave @features_ok at zero */
+        return;
+    }
+
+    /* valid feature subset requested, lock it down, report success */
+    lpc->smi_negotiated_features = guest_features;
+    lpc->smi_features_ok = 1;
+}
+
 void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled)
 {
     ICH9LPCState *lpc = ICH9_LPC_DEVICE(lpc_pci);
     qemu_irq sci_irq;
+    FWCfgState *fw_cfg = fw_cfg_find();
 
     sci_irq = qemu_allocate_irq(ich9_set_sci, lpc, 0);
     ich9_pm_init(lpc_pci, &lpc->pm, smm_enabled, sci_irq);
+
+    if (lpc->smi_host_features && fw_cfg) {
+        uint64_t host_features_le;
+
+        host_features_le = cpu_to_le64(lpc->smi_host_features);
+        memcpy(lpc->smi_host_features_le, &host_features_le,
+               sizeof host_features_le);
+        fw_cfg_add_file(fw_cfg, "etc/smi/supported-features",
+                        lpc->smi_host_features_le,
+                        sizeof lpc->smi_host_features_le);
+
+        /* The other two guest-visible fields are cleared on device reset, we
+         * just link them into fw_cfg here.
+         */
+        fw_cfg_add_file_callback(fw_cfg, "etc/smi/requested-features",
+                                 NULL, NULL,
+                                 lpc->smi_guest_features_le,
+                                 sizeof lpc->smi_guest_features_le,
+                                 false);
+        fw_cfg_add_file_callback(fw_cfg, "etc/smi/features-ok",
+                                 smi_features_ok_callback, lpc,
+                                 &lpc->smi_features_ok,
+                                 sizeof lpc->smi_features_ok,
+                                 true);
+    }
+
     ich9_lpc_reset(&lpc->d.qdev);
 }
 
@@ -507,6 +558,10 @@ static void ich9_lpc_reset(DeviceState *qdev)
 
     lpc->sci_level = 0;
     lpc->rst_cnt = 0;
+
+    memset(lpc->smi_guest_features_le, 0, sizeof lpc->smi_guest_features_le);
+    lpc->smi_features_ok = 0;
+    lpc->smi_negotiated_features = 0;
 }
 
 /* root complex register block is mapped into memory space */
@@ -668,6 +723,29 @@ static const VMStateDescription vmstate_ich9_rst_cnt = {
     }
 };
 
+static bool ich9_smi_feat_needed(void *opaque)
+{
+    ICH9LPCState *lpc = opaque;
+
+    return !buffer_is_zero(lpc->smi_guest_features_le,
+                           sizeof lpc->smi_guest_features_le) ||
+           lpc->smi_features_ok;
+}
+
+static const VMStateDescription vmstate_ich9_smi_feat = {
+    .name = "ICH9LPC/smi_feat",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = ich9_smi_feat_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8_ARRAY(smi_guest_features_le, ICH9LPCState,
+                            sizeof(uint64_t)),
+        VMSTATE_UINT8(smi_features_ok, ICH9LPCState),
+        VMSTATE_UINT64(smi_negotiated_features, ICH9LPCState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_ich9_lpc = {
     .name = "ICH9LPC",
     .version_id = 1,
@@ -683,6 +761,7 @@ static const VMStateDescription vmstate_ich9_lpc = {
     },
     .subsections = (const VMStateDescription*[]) {
         &vmstate_ich9_rst_cnt,
+        &vmstate_ich9_smi_feat,
         NULL
     }
 };
-- 
2.9.3

[Qemu-devel] [PATCH v7 wave 2 2/3] hw/isa/lpc_ich9: add broadcast SMI feature

[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 smaller problem with (1a) and (2) is that the BSP and AP
synchronization is slow. For example, the "taskset -c 1 efibootmgr"
command from (2) can take more than 3 seconds to complete, because
efibootmgr accesses non-volatile UEFI variables intensively.

The larger problem is that QEMU's current behavior diverges from the
behavior usually seen on physical hardware, and that keeps exposing
obscure corner cases, race conditions and other instabilities in edk2,
which generally expects / prefers a software SMI to affect all CPUs at
once.

Therefore introduce the "broadcast SMI" feature that causes QEMU to inject
the SMI on all VCPUs.

While the original posting of this patch
<http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05658.html>
only intended to speed up (2), based on our recent "stress testing" of SMM
this patch actually provides functional improvements.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---

Notes:
    v7:
    - no changes, pick up Igor's R-b
    
    v6:
    - no changes, pick up Michael's R-b
    
    v5:
    - replace the ICH9_LPC_SMI_F_BROADCAST bit value with the
      ICH9_LPC_SMI_F_BROADCAST_BIT bit position (necessary for
      DEFINE_PROP_BIT() in the next patch)

 include/hw/i386/ich9.h |  3 +++
 hw/isa/lpc_ich9.c      | 10 +++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index da1118727146..18dcca7ebcbf 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -250,4 +250,7 @@ Object *ich9_lpc_find(void);
 #define ICH9_SMB_HST_D1                         0x06
 #define ICH9_SMB_HOST_BLOCK_DB                  0x07
 
+/* bit positions used in fw_cfg SMI feature negotiation */
+#define ICH9_LPC_SMI_F_BROADCAST_BIT            0
+
 #endif /* HW_ICH9_H */
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 376b7801a42c..ced6f803a4f2 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -437,7 +437,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->smi_negotiated_features &
+            (UINT64_C(1) << ICH9_LPC_SMI_F_BROADCAST_BIT)) {
+            CPUState *cs;
+            CPU_FOREACH(cs) {
+                cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+            }
+        } else {
+            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+        }
     }
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH v7 wave 2 3/3] hw/isa/lpc_ich9: negotiate SMI broadcast on pc-q35-2.9+ machine types

[Laszlo Ersek]

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---

Notes:
    v7:
    - no functional changes, pick up Igor's R-b
    - rebase to master, resolve conflict with commit abc62c89f319 ("pc.h:
      move x-mach-use-reliable-get-clock compat entry to PC_COMPAT_2_8",
      2017-01-18)
    
    v6:
    - rename "smi_broadcast" to "x-smi-broadcast" [Eduardo]
    - pick up Eduardo's R-b
    - pick up Michael's R-b
    
    v5:
    - replace the v4 patch "hw/i386/pc_q35: advertise broadcast SMI if VCPU
      hotplug is turned off" with a simple device property and compat
      setting [Igor]

 include/hw/i386/pc.h | 6 ++++++
 hw/isa/lpc_ich9.c    | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 5a20c5e38ef5..b89441d2c49f 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -381,6 +381,12 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .property = "x-mach-use-reliable-get-clock",\
         .value    = "off",\
     },\
+    {\
+        .driver   = "ICH9-LPC",\
+        .property = "x-smi-broadcast",\
+        .value    = "off",\
+    },\
+
 
 #define PC_COMPAT_2_7 \
     HW_COMPAT_2_7 \
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index ced6f803a4f2..59930dd9d09d 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -776,6 +776,8 @@ static const VMStateDescription vmstate_ich9_lpc = {
 
 static Property ich9_lpc_properties[] = {
     DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
+    DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
+                      ICH9_LPC_SMI_F_BROADCAST_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH v7 wave 2 0/3] q35: add negotiable broadcast SMI

[Paolo Bonzini]

On 26/01/2017 02:44, Laszlo Ersek wrote:
> Hi Paolo,
> 
> I'm sending v7 with no functional changes; I've only picked up Igor's
> R-b's from v6 so you don't have to, and while rebasing to current
> master, I resolved a conflict in patch #3 against recent commit
> abc62c89f319 ("pc.h: move x-mach-use-reliable-get-clock compat entry to
> PC_COMPAT_2_8", 2017-01-18).
> 
> The series is fully reviewed and is mergeable as-is.

Much appreciated, thanks.  I hope to send the pull request already tomorrow.

Paolo

> v6 wave 2 was at
> <http://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg02424.html>.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (3):
>   hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg
>   hw/isa/lpc_ich9: add broadcast SMI feature
>   hw/isa/lpc_ich9: negotiate SMI broadcast on pc-q35-2.9+ machine types
> 
>  include/hw/i386/ich9.h | 13 ++++++++
>  include/hw/i386/pc.h   |  6 ++++
>  hw/isa/lpc_ich9.c      | 91 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 109 insertions(+), 1 deletion(-)
> 

Re: [PATCH v7 2/3] hw/isa/lpc_ich9: add broadcast SMI feature

[Philippe Mathieu-Daudé]

Hi Laszlo, Igor, Gerd,

(old patch, now commit 5ce45c7a2b)

On 26/1/17 02:44, 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 smaller problem with (1a) and (2) is that the BSP and AP
> synchronization is slow. For example, the "taskset -c 1 efibootmgr"
> command from (2) can take more than 3 seconds to complete, because
> efibootmgr accesses non-volatile UEFI variables intensively.
> 
> The larger problem is that QEMU's current behavior diverges from the
> behavior usually seen on physical hardware, and that keeps exposing
> obscure corner cases, race conditions and other instabilities in edk2,
> which generally expects / prefers a software SMI to affect all CPUs at
> once.
> 
> Therefore introduce the "broadcast SMI" feature that causes QEMU to inject
> the SMI on all VCPUs.

I'm trying to remove cpu_interrupt() API from hw/ and found this odd
case.

IIUC, the code you added is closer to what real HW is doing:

   CPU_FOREACH(cs) { cpu_interrupt(cs, CPU_INTERRUPT_SMI); }

and previous implementation was bogus:

   cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);

but to avoid breaking older VMs ready to deal with bogus impl,
you have to add a virtual (non-HW) ICH9_LPC_SMI_F_BROADCAST bit
so new VMs can detect (negotiating) it and use normal expected
HW behavior.

If so, and since this change was almost 7 years ago, can we
expect that most of today's VMs use ICH9_LPC_SMI_F_BROADCAST_BIT,
and would it be possible to deprecate it, so it become the only
possibility, allowing us to remove this bogus call?

   cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);

> While the original posting of this patch
> <http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05658.html>
> only intended to speed up (2), based on our recent "stress testing" of SMM
> this patch actually provides functional improvements.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> 
> Notes:
>      v7:
>      - no changes, pick up Igor's R-b
>      
>      v6:
>      - no changes, pick up Michael's R-b
>      
>      v5:
>      - replace the ICH9_LPC_SMI_F_BROADCAST bit value with the
>        ICH9_LPC_SMI_F_BROADCAST_BIT bit position (necessary for
>        DEFINE_PROP_BIT() in the next patch)
> 
>   include/hw/i386/ich9.h |  3 +++
>   hw/isa/lpc_ich9.c      | 10 +++++++++-
>   2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index da1118727146..18dcca7ebcbf 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -250,4 +250,7 @@ Object *ich9_lpc_find(void);
>   #define ICH9_SMB_HST_D1                         0x06
>   #define ICH9_SMB_HOST_BLOCK_DB                  0x07
>   
> +/* bit positions used in fw_cfg SMI feature negotiation */
> +#define ICH9_LPC_SMI_F_BROADCAST_BIT            0
> +
>   #endif /* HW_ICH9_H */
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 376b7801a42c..ced6f803a4f2 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -437,7 +437,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->smi_negotiated_features &
> +            (UINT64_C(1) << ICH9_LPC_SMI_F_BROADCAST_BIT)) {
> +            CPUState *cs;
> +            CPU_FOREACH(cs) {
> +                cpu_interrupt(cs, CPU_INTERRUPT_SMI);
> +            }
> +        } else {
> +            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> +        }
>       }
>   }
>   

Re: [PATCH v7 2/3] hw/isa/lpc_ich9: add broadcast SMI feature

[Laszlo Ersek]

On 2/20/24 08:58, Philippe Mathieu-Daudé wrote:
> Hi Laszlo, Igor, Gerd,
> 
> (old patch, now commit 5ce45c7a2b)
> 
> On 26/1/17 02:44, 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 smaller problem with (1a) and (2) is that the BSP and AP
>> synchronization is slow. For example, the "taskset -c 1 efibootmgr"
>> command from (2) can take more than 3 seconds to complete, because
>> efibootmgr accesses non-volatile UEFI variables intensively.
>>
>> The larger problem is that QEMU's current behavior diverges from the
>> behavior usually seen on physical hardware, and that keeps exposing
>> obscure corner cases, race conditions and other instabilities in edk2,
>> which generally expects / prefers a software SMI to affect all CPUs at
>> once.
>>
>> Therefore introduce the "broadcast SMI" feature that causes QEMU to
>> inject
>> the SMI on all VCPUs.
> 
> I'm trying to remove cpu_interrupt() API from hw/ and found this odd
> case.
> 
> IIUC, the code you added is closer to what real HW is doing:
> 
>   CPU_FOREACH(cs) { cpu_interrupt(cs, CPU_INTERRUPT_SMI); }
> 
> and previous implementation was bogus:
> 
>   cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> 
> but to avoid breaking older VMs ready to deal with bogus impl,
> you have to add a virtual (non-HW) ICH9_LPC_SMI_F_BROADCAST bit
> so new VMs can detect (negotiating) it and use normal expected
> HW behavior.
> 
> If so, and since this change was almost 7 years ago, can we
> expect that most of today's VMs use ICH9_LPC_SMI_F_BROADCAST_BIT,
> and would it be possible to deprecate it, so it become the only
> possibility, allowing us to remove this bogus call?
> 
>   cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);

For OVMF guests: yes, said deprecation should be safe.

Note however that the "current_cpu" case (the original case) had been in
place minimally for SeaBIOS. I don't know how exactly the deprecation /
removal in QEMU would work, but if you build SeaBIOS with
CONFIG_USE_SMM, it might still depend on the "current_cpu" branch.

FWIW, "roms/config.seabios-128k" and "roms/config.seabios-microvm" both
contain CONFIG_USE_SMM=n, so the deprecation likely wouldn't matter for
those SeaBIOS binaries (bundled with QEMU). But it could matter for
SeaBIOS binaries from other sources; plus "roms/config.seabios-256k"
does *not* contain a setting like that (and the SeaBIOS default is "y",
when building for QEMU).

For another data point: as far as I remember, we had disabled
CONFIG_USE_SMM in RHEL; there had been stability issues.

... I can't describe *all* the uses that SeaBIOS has for SMM. *One* use
is from commit 55215cd425d3 ("Implement call32 mechanism using SMIs.",
2014-10-15) -- "this allows SeaBIOS to transition to 32bit mode even
when called in vm86 mode". Because that commit modifies "stacks.c", I
think it must be related to the "cooperative multi-tasking system"
described here: <https://www.seabios.org/Execution_and_code_flow#Threads>.

I'm really rusty on this [*], but here's one potential symptom I can
theorize about: assuming you silently make broadcast SMI the default in
QEMU, and SeaBIOS raises an SMI (expecting it to only affect the BSP),
the SMI could become pending on all the APs (which would all be in RESET
state at that point [**]). And when Linux booted those APs with
INIT-SIPI-SIPI sequences, the pending SMIs could be delivered
immediately, and the APs would launch immediately into SMM. That would
likely not be expected.

[*] Using earlier edk2 commit cbccf995920a ("OvmfPkg/CpuHotplugSmm: fix
CPU hotplug race just after SMI broadcast", 2020-08-27) as cheat-sheet.
IIRC, I consulted the Intel SDM extensively back when I was working on
that patch.

[**] Hm... wait. It seems SeaBIOS does boot all APs in the system. The
APs apparently end up penned in a HLT-loop; see at the end of
"entry_smp" in "romlayout.S". In that case, the problem could be worse I
guess? The broadcast SMI could wake all APs from their halted states.
SeaBIOS doesn't seem to relocate SMBASE for the APs, so if multiple
processors start executing in SMM at the same time, the results are not
going to be stellar.

Laszlo

> 
>> While the original posting of this patch
>> <http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05658.html>
>> only intended to speed up (2), based on our recent "stress testing" of
>> SMM
>> this patch actually provides functional improvements.
>>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>
>> Notes:
>>      v7:
>>      - no changes, pick up Igor's R-b
>>           v6:
>>      - no changes, pick up Michael's R-b
>>           v5:
>>      - replace the ICH9_LPC_SMI_F_BROADCAST bit value with the
>>        ICH9_LPC_SMI_F_BROADCAST_BIT bit position (necessary for
>>        DEFINE_PROP_BIT() in the next patch)
>>
>>   include/hw/i386/ich9.h |  3 +++
>>   hw/isa/lpc_ich9.c      | 10 +++++++++-
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
>> index da1118727146..18dcca7ebcbf 100644
>> --- a/include/hw/i386/ich9.h
>> +++ b/include/hw/i386/ich9.h
>> @@ -250,4 +250,7 @@ Object *ich9_lpc_find(void);
>>   #define ICH9_SMB_HST_D1                         0x06
>>   #define ICH9_SMB_HOST_BLOCK_DB                  0x07
>>   +/* bit positions used in fw_cfg SMI feature negotiation */
>> +#define ICH9_LPC_SMI_F_BROADCAST_BIT            0
>> +
>>   #endif /* HW_ICH9_H */
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 376b7801a42c..ced6f803a4f2 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -437,7 +437,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->smi_negotiated_features &
>> +            (UINT64_C(1) << ICH9_LPC_SMI_F_BROADCAST_BIT)) {
>> +            CPUState *cs;
>> +            CPU_FOREACH(cs) {
>> +                cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>> +            }
>> +        } else {
>> +            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
>> +        }
>>       }
>>   }
>>   
> 

Re: [PATCH v7 2/3] hw/isa/lpc_ich9: add broadcast SMI feature

[Philippe Mathieu-Daudé]

On 20/2/24 11:09, Laszlo Ersek wrote:

(Cc'ing Kevin too)

> On 2/20/24 08:58, Philippe Mathieu-Daudé wrote:
>> Hi Laszlo, Igor, Gerd,
>>
>> (old patch, now commit 5ce45c7a2b)
>>
>> On 26/1/17 02:44, 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 smaller problem with (1a) and (2) is that the BSP and AP
>>> synchronization is slow. For example, the "taskset -c 1 efibootmgr"
>>> command from (2) can take more than 3 seconds to complete, because
>>> efibootmgr accesses non-volatile UEFI variables intensively.
>>>
>>> The larger problem is that QEMU's current behavior diverges from the
>>> behavior usually seen on physical hardware, and that keeps exposing
>>> obscure corner cases, race conditions and other instabilities in edk2,
>>> which generally expects / prefers a software SMI to affect all CPUs at
>>> once.
>>>
>>> Therefore introduce the "broadcast SMI" feature that causes QEMU to
>>> inject
>>> the SMI on all VCPUs.
>>
>> I'm trying to remove cpu_interrupt() API from hw/ and found this odd
>> case.
>>
>> IIUC, the code you added is closer to what real HW is doing:
>>
>>    CPU_FOREACH(cs) { cpu_interrupt(cs, CPU_INTERRUPT_SMI); }
>>
>> and previous implementation was bogus:
>>
>>    cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
>>
>> but to avoid breaking older VMs ready to deal with bogus impl,
>> you have to add a virtual (non-HW) ICH9_LPC_SMI_F_BROADCAST bit
>> so new VMs can detect (negotiating) it and use normal expected
>> HW behavior.
>>
>> If so, and since this change was almost 7 years ago, can we
>> expect that most of today's VMs use ICH9_LPC_SMI_F_BROADCAST_BIT,
>> and would it be possible to deprecate it, so it become the only
>> possibility, allowing us to remove this bogus call?
>>
>>    cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> 
> For OVMF guests: yes, said deprecation should be safe.
> 
> Note however that the "current_cpu" case (the original case) had been in
> place minimally for SeaBIOS. I don't know how exactly the deprecation /
> removal in QEMU would work, but if you build SeaBIOS with
> CONFIG_USE_SMM, it might still depend on the "current_cpu" branch.
> 
> FWIW, "roms/config.seabios-128k" and "roms/config.seabios-microvm" both
> contain CONFIG_USE_SMM=n, so the deprecation likely wouldn't matter for
> those SeaBIOS binaries (bundled with QEMU). But it could matter for
> SeaBIOS binaries from other sources; plus "roms/config.seabios-256k"
> does *not* contain a setting like that (and the SeaBIOS default is "y",
> when building for QEMU).
> 
> For another data point: as far as I remember, we had disabled
> CONFIG_USE_SMM in RHEL; there had been stability issues.
> 
> ... I can't describe *all* the uses that SeaBIOS has for SMM. *One* use
> is from commit 55215cd425d3 ("Implement call32 mechanism using SMIs.",
> 2014-10-15) -- "this allows SeaBIOS to transition to 32bit mode even
> when called in vm86 mode". Because that commit modifies "stacks.c", I
> think it must be related to the "cooperative multi-tasking system"
> described here: <https://www.seabios.org/Execution_and_code_flow#Threads>.
> 
> I'm really rusty on this [*], but here's one potential symptom I can
> theorize about: assuming you silently make broadcast SMI the default in
> QEMU, and SeaBIOS raises an SMI (expecting it to only affect the BSP),
> the SMI could become pending on all the APs (which would all be in RESET
> state at that point [**]). And when Linux booted those APs with
> INIT-SIPI-SIPI sequences, the pending SMIs could be delivered
> immediately, and the APs would launch immediately into SMM. That would
> likely not be expected.
> 
> [*] Using earlier edk2 commit cbccf995920a ("OvmfPkg/CpuHotplugSmm: fix
> CPU hotplug race just after SMI broadcast", 2020-08-27) as cheat-sheet.
> IIRC, I consulted the Intel SDM extensively back when I was working on
> that patch.
> 
> [**] Hm... wait. It seems SeaBIOS does boot all APs in the system. The
> APs apparently end up penned in a HLT-loop; see at the end of
> "entry_smp" in "romlayout.S". In that case, the problem could be worse I
> guess? The broadcast SMI could wake all APs from their halted states.
> SeaBIOS doesn't seem to relocate SMBASE for the APs, so if multiple
> processors start executing in SMM at the same time, the results are not
> going to be stellar.

Thanks a lot Laszlo for this detailed historical information, it is
very helpful. I'll preserve current behavior as a kludge, because it
helps demonstrate part of my heterogeneous prototype, and see later
if we can deprecate that to eventually remove the kludge.

Regards,

Phil.

> Laszlo
> 
>>
>>> While the original posting of this patch
>>> <http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05658.html>
>>> only intended to speed up (2), based on our recent "stress testing" of
>>> SMM
>>> this patch actually provides functional improvements.
>>>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>>
>>> Notes:
>>>       v7:
>>>       - no changes, pick up Igor's R-b
>>>            v6:
>>>       - no changes, pick up Michael's R-b
>>>            v5:
>>>       - replace the ICH9_LPC_SMI_F_BROADCAST bit value with the
>>>         ICH9_LPC_SMI_F_BROADCAST_BIT bit position (necessary for
>>>         DEFINE_PROP_BIT() in the next patch)
>>>
>>>    include/hw/i386/ich9.h |  3 +++
>>>    hw/isa/lpc_ich9.c      | 10 +++++++++-
>>>    2 files changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
>>> index da1118727146..18dcca7ebcbf 100644
>>> --- a/include/hw/i386/ich9.h
>>> +++ b/include/hw/i386/ich9.h
>>> @@ -250,4 +250,7 @@ Object *ich9_lpc_find(void);
>>>    #define ICH9_SMB_HST_D1                         0x06
>>>    #define ICH9_SMB_HOST_BLOCK_DB                  0x07
>>>    +/* bit positions used in fw_cfg SMI feature negotiation */
>>> +#define ICH9_LPC_SMI_F_BROADCAST_BIT            0
>>> +
>>>    #endif /* HW_ICH9_H */
>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>> index 376b7801a42c..ced6f803a4f2 100644
>>> --- a/hw/isa/lpc_ich9.c
>>> +++ b/hw/isa/lpc_ich9.c
>>> @@ -437,7 +437,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->smi_negotiated_features &
>>> +            (UINT64_C(1) << ICH9_LPC_SMI_F_BROADCAST_BIT)) {
>>> +            CPUState *cs;
>>> +            CPU_FOREACH(cs) {
>>> +                cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>>> +            }
>>> +        } else {
>>> +            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
>>> +        }
>>>        }
>>>    }
>>>    
>>
>