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

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

[Laszlo Ersek]

This is the second (ICH9-LPC) half of the v5 iteration of the series
posted here:
<http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00129.html>.

In this version, the ICH9-LPC patches have been separated into a
standalone "wave", for helping review / maintenance. More importantly,
I've addressed Igor's v4 feedback (which happens to address Eduardo's
feedback as well). See the individual patches for the details.

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   |  5 +++
 hw/isa/lpc_ich9.c      | 91 +++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 108 insertions(+), 1 deletion(-)

-- 
2.9.3

[Qemu-devel] [PATCH v5 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>
---

Notes:
    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 v5 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>
---

Notes:
    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 v5 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>
---

Notes:
    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 | 5 +++++
 hw/isa/lpc_ich9.c    | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 230e9e70c504..fb8ca7c907f6 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -376,6 +376,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
 #define PC_COMPAT_2_8 \
     HW_COMPAT_2_8 \
+    {\
+        .driver   = "ICH9-LPC",\
+        .property = "smi_broadcast",\
+        .value    = "off",\
+    },\
 
 
 #define PC_COMPAT_2_7 \
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index ced6f803a4f2..27fae5144744 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("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 v5 wave 2 3/3] hw/isa/lpc_ich9: negotiate SMI broadcast on pc-q35-2.9+ machine types

[Eduardo Habkost]

On Wed, Jan 11, 2017 at 06:35:28PM +0100, Laszlo Ersek wrote:
> 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>
> ---
> 
> Notes:
>     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 | 5 +++++
>  hw/isa/lpc_ich9.c    | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 230e9e70c504..fb8ca7c907f6 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -376,6 +376,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
>  #define PC_COMPAT_2_8 \
>      HW_COMPAT_2_8 \
> +    {\
> +        .driver   = "ICH9-LPC",\
> +        .property = "smi_broadcast",\
> +        .value    = "off",\
> +    },\
>  
>  
>  #define PC_COMPAT_2_7 \
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index ced6f803a4f2..27fae5144744 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("smi_broadcast", ICH9LPCState, smi_host_features,
> +                      ICH9_LPC_SMI_F_BROADCAST_BIT, true),

Please use hyphens instead of underscores on QOM property names.

Also, if this is not supposed to be configured directly by the
user, please use a "x-" prefix.

With that fixed:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

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

[Michael S. Tsirkin]

On Wed, Jan 11, 2017 at 06:35:25PM +0100, Laszlo Ersek wrote:
> This is the second (ICH9-LPC) half of the v5 iteration of the series
> posted here:
> <http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00129.html>.
> 
> In this version, the ICH9-LPC patches have been separated into a
> standalone "wave", for helping review / maintenance. More importantly,
> I've addressed Igor's v4 feedback (which happens to address Eduardo's
> feedback as well). See the individual patches for the details.
> 
> 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

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> 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   |  5 +++
>  hw/isa/lpc_ich9.c      | 91 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 108 insertions(+), 1 deletion(-)
> 
> -- 
> 2.9.3

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

[Laszlo Ersek]

On 01/12/17 16:57, Michael S. Tsirkin wrote:
> On Wed, Jan 11, 2017 at 06:35:25PM +0100, Laszlo Ersek wrote:
>> This is the second (ICH9-LPC) half of the v5 iteration of the series
>> posted here:
>> <http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00129.html>.
>>
>> In this version, the ICH9-LPC patches have been separated into a
>> standalone "wave", for helping review / maintenance. More importantly,
>> I've addressed Igor's v4 feedback (which happens to address Eduardo's
>> feedback as well). See the individual patches for the details.
>>
>> 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
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Awesome, thanks!

I'll send out v6 soon, addressing Eduardo's feedback.

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   |  5 +++
>>  hw/isa/lpc_ich9.c      | 91 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 108 insertions(+), 1 deletion(-)
>>
>> -- 
>> 2.9.3

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

[Laszlo Ersek]

On 01/12/17 13:40, Eduardo Habkost wrote:
> On Wed, Jan 11, 2017 at 06:35:28PM +0100, Laszlo Ersek wrote:
>> 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>
>> ---
>>
>> Notes:
>>     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 | 5 +++++
>>  hw/isa/lpc_ich9.c    | 2 ++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 230e9e70c504..fb8ca7c907f6 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -376,6 +376,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>>  
>>  #define PC_COMPAT_2_8 \
>>      HW_COMPAT_2_8 \
>> +    {\
>> +        .driver   = "ICH9-LPC",\
>> +        .property = "smi_broadcast",\
>> +        .value    = "off",\
>> +    },\
>>  
>>  
>>  #define PC_COMPAT_2_7 \
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index ced6f803a4f2..27fae5144744 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("smi_broadcast", ICH9LPCState, smi_host_features,
>> +                      ICH9_LPC_SMI_F_BROADCAST_BIT, true),
> 
> Please use hyphens instead of underscores on QOM property names.

Will do, thank you.

> 
> Also, if this is not supposed to be configured directly by the
> user, please use a "x-" prefix.

Hmm, for "normal use", the user is not supposed to mess with it,
correct. However, at least for testing OVMF's behavior when the
negotiation is available vs. unavailable, the flag is useful on the
command line too (although the same effect can be achieved with
selecting pc-q35-2.8 vs. pc-q35-2.9). What does this imply?...

Okay, I think I convinced myself that this should be called
x-smi-broadcast. Ultimately I don't think libvirt should ever try to set
this property, for example.

> With that fixed:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 

Many thanks!
Laszlo

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

[Eduardo Habkost]

On Thu, Jan 12, 2017 at 05:40:40PM +0100, Laszlo Ersek wrote:
> On 01/12/17 13:40, Eduardo Habkost wrote:
> > On Wed, Jan 11, 2017 at 06:35:28PM +0100, Laszlo Ersek wrote:
> >> 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>
> >> ---
> >>
> >> Notes:
> >>     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 | 5 +++++
> >>  hw/isa/lpc_ich9.c    | 2 ++
> >>  2 files changed, 7 insertions(+)
> >>
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index 230e9e70c504..fb8ca7c907f6 100644
> >> --- a/include/hw/i386/pc.h
> >> +++ b/include/hw/i386/pc.h
> >> @@ -376,6 +376,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> >>  
> >>  #define PC_COMPAT_2_8 \
> >>      HW_COMPAT_2_8 \
> >> +    {\
> >> +        .driver   = "ICH9-LPC",\
> >> +        .property = "smi_broadcast",\
> >> +        .value    = "off",\
> >> +    },\
> >>  
> >>  
> >>  #define PC_COMPAT_2_7 \
> >> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> >> index ced6f803a4f2..27fae5144744 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("smi_broadcast", ICH9LPCState, smi_host_features,
> >> +                      ICH9_LPC_SMI_F_BROADCAST_BIT, true),
> > 
> > Please use hyphens instead of underscores on QOM property names.
> 
> Will do, thank you.
> 
> > 
> > Also, if this is not supposed to be configured directly by the
> > user, please use a "x-" prefix.
> 
> Hmm, for "normal use", the user is not supposed to mess with it,
> correct. However, at least for testing OVMF's behavior when the
> negotiation is available vs. unavailable, the flag is useful on the
> command line too (although the same effect can be achieved with
> selecting pc-q35-2.8 vs. pc-q35-2.9). What does this imply?...

It will be still available for testing, the difference is that we
won't guarantee it will be available in future QEMU versions.

> 
> Okay, I think I convinced myself that this should be called
> x-smi-broadcast. Ultimately I don't think libvirt should ever try to set
> this property, for example.

Exactly.

-- 
Eduardo