diff mbox series

[v6,02/11] x86: cpuhp: prevent guest crash on CPU hotplug when broadcast SMI is in use

Message ID 20200923094650.1301166-3-imammedo@redhat.com
State New
Headers show
Series x86: fix cpu hotplug with secure boot | expand

Commit Message

Igor Mammedov Sept. 23, 2020, 9:46 a.m. UTC
There were reports of guest crash on CPU hotplug, when using q35 machine
type and OVMF with SMM, due to hotplugged CPU trying to process SMI at
default SMI handler location without it being relocated by firmware first.

Fix it by refusing hotplug if firmware hasn't negotiated CPU hotplug with
SMI support while SMI broadcast is in use.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
---
v6:
  - rebase on top of current master, due to non trivial conflicts
    caused by microvm series, which moved/renamed pc_cpu_pre_plug()
    (Ankur Arora <ankur.a.arora@oracle.com>)
v1:
   fix typos an use suggested wording in commit and error msg
   s/secure boot/smm/; s/hotplug SMI/hotplug with SMI/
      (Laszlo Ersek <lersek@redhat.com>)
---
 hw/acpi/ich9.c | 12 +++++++++++-
 hw/i386/x86.c  | 11 +++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

Comments

Laszlo Ersek Sept. 23, 2020, 5:16 p.m. UTC | #1
On 09/23/20 11:46, Igor Mammedov wrote:
> There were reports of guest crash on CPU hotplug, when using q35 machine

> type and OVMF with SMM, due to hotplugged CPU trying to process SMI at

> default SMI handler location without it being relocated by firmware first.

> 

> Fix it by refusing hotplug if firmware hasn't negotiated CPU hotplug with

> SMI support while SMI broadcast is in use.

> 

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

> Tested-by: Laszlo Ersek <lersek@redhat.com>

> ---

> v6:

>   - rebase on top of current master, due to non trivial conflicts

>     caused by microvm series, which moved/renamed pc_cpu_pre_plug()

>     (Ankur Arora <ankur.a.arora@oracle.com>)


Tested-by: Laszlo Ersek <lersek@redhat.com>


Thanks
Laszlo

> v1:

>    fix typos an use suggested wording in commit and error msg

>    s/secure boot/smm/; s/hotplug SMI/hotplug with SMI/

>       (Laszlo Ersek <lersek@redhat.com>)

> ---

>  hw/acpi/ich9.c | 12 +++++++++++-

>  hw/i386/x86.c  | 11 +++++++++++

>  2 files changed, 22 insertions(+), 1 deletion(-)

> 

> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c

> index 6a19070cec..0acc9a3107 100644

> --- a/hw/acpi/ich9.c

> +++ b/hw/acpi/ich9.c

> @@ -408,10 +408,20 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,

>      ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);

>  

>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&

> -        !lpc->pm.acpi_memory_hotplug.is_enabled)

> +        !lpc->pm.acpi_memory_hotplug.is_enabled) {

>          error_setg(errp,

>                     "memory hotplug is not enabled: %s.memory-hotplug-support "

>                     "is not set", object_get_typename(OBJECT(lpc)));

> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {

> +        uint64_t negotiated = lpc->smi_negotiated_features;

> +

> +        if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&

> +            !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {

> +            error_setg(errp, "cpu hotplug with SMI wasn't enabled by firmware");

> +            error_append_hint(errp, "update machine type to newer than 5.1 "

> +                "and firmware that suppors CPU hotplug with SMM");

> +        }

> +    }

>  }

>  

>  void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,

> diff --git a/hw/i386/x86.c b/hw/i386/x86.c

> index c2ea989579..403c2b1dad 100644

> --- a/hw/i386/x86.c

> +++ b/hw/i386/x86.c

> @@ -279,6 +279,17 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,

>          return;

>      }

>  

> +    if (x86ms->acpi_dev) {

> +        Error *local_err = NULL;

> +

> +        hotplug_handler_pre_plug(HOTPLUG_HANDLER(x86ms->acpi_dev), dev,

> +                                 &local_err);

> +        if (local_err) {

> +            error_propagate(errp, local_err);

> +            return;

> +        }

> +    }

> +

>      init_topo_info(&topo_info, x86ms);

>  

>      env->nr_dies = x86ms->smp_dies;

>
diff mbox series

Patch

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 6a19070cec..0acc9a3107 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -408,10 +408,20 @@  void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
     ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
-        !lpc->pm.acpi_memory_hotplug.is_enabled)
+        !lpc->pm.acpi_memory_hotplug.is_enabled) {
         error_setg(errp,
                    "memory hotplug is not enabled: %s.memory-hotplug-support "
                    "is not set", object_get_typename(OBJECT(lpc)));
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        uint64_t negotiated = lpc->smi_negotiated_features;
+
+        if (negotiated & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT) &&
+            !(negotiated & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {
+            error_setg(errp, "cpu hotplug with SMI wasn't enabled by firmware");
+            error_append_hint(errp, "update machine type to newer than 5.1 "
+                "and firmware that suppors CPU hotplug with SMM");
+        }
+    }
 }
 
 void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index c2ea989579..403c2b1dad 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -279,6 +279,17 @@  void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
         return;
     }
 
+    if (x86ms->acpi_dev) {
+        Error *local_err = NULL;
+
+        hotplug_handler_pre_plug(HOTPLUG_HANDLER(x86ms->acpi_dev), dev,
+                                 &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
     init_topo_info(&topo_info, x86ms);
 
     env->nr_dies = x86ms->smp_dies;