mbox series

[v6,00/11] x86: fix cpu hotplug with secure boot

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

Message

Igor Mammedov Sept. 23, 2020, 9:46 a.m. UTC
v6:
  - [9/10] Add comment explaining why while_ctx2 restarts from the last processed CPU.
  - rebase on top of current master, due to non trivial conflict
    caused by microvm series, which moved/renamed pc_cpu_pre_plug()
v5:
  - fix hotplug on Windows when there is more than 256 possible CPUs
    (Windows isn't able to handle VarPackage over 255 elements
     so process CPUs in batches)
  - fix off-by-one in package length (Laszlo)
  - fix not selecting CPU before clearing insert event (Laszlo)
  - use aml_lgreater() instead of aml_lnot(aml_equal(num_added_cpus, zero) (Laszlo)
  - split 'x68: acpi: trigger SMI before sending hotplug Notify event to OSPM'
    in samller chunks (Laszlo)
  - fix comment to match spec (Laszlo)
  - reorder aml_lor() and aml_land() in header (Laszlo)
v4:
  - fix 5.2 machine types so they won't apply pc_compat_5_1 (Laszlo)
v3:
  - rebase on top of "[PATCH v2] hw: add compat machines for 5.2"
    so apply that before this patch
v2:
  - AML: clean is_inserted flag only after SMI callback
  - make x-smi-cpu-hotunplug false by default
  - massage error hint on not supported unplug
v1:
  - fix typos and some phrases (Laszlo)
  - add unplug check (Laszlo)
  - redo AML scan logic to avoid race when adding multiple CPUs

CPU hotplug with Secure Boot was not really supported and firmware wasn't aware
of hotplugged CPUs (which might lead to guest crashes). During 4.2 we introduced
locked SMI handler RAM arrea to make sure that guest OS wasn't able to inject
its own SMI handler and OVMF added initial CPU hotplug support.

This series is QEMU part of that support which lets QMVF tell QEMU that
CPU hotplug with SMI broadcast enabled is supported so that QEMU would be able
to prevent hotplug in case it's not supported and trigger SMI on hotplug when
it's necessary.

Igor Mammedov (11):
  x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features
  x86: cpuhp: prevent guest crash on CPU hotplug when broadcast SMI is
    in use
  x86: cpuhp: refuse cpu hot-unplug request earlier if not supported
  acpi: add aml_land() and aml_break() primitives
  tests: acpi: mark to be changed tables in
    bios-tables-test-allowed-diff
  x86: ich9: expose "smi_negotiated_features" as a QOM property
  x86: acpi: introduce AcpiPmInfo::smi_on_cpuhp
  x86: acpi: introduce the PCI0.SMI0 ACPI device
  x68: acpi: trigger SMI before sending hotplug Notify event to OSPM
  tests: acpi: update acpi blobs with new AML
  smp: drop support for deprecated (invalid topologies)

 include/hw/acpi/aml-build.h       |   2 +
 include/hw/acpi/cpu.h             |   1 +
 include/hw/i386/ich9.h            |   4 +
 docs/system/deprecated.rst        |  26 ++---
 hw/acpi/aml-build.c               |  16 +++
 hw/acpi/cpu.c                     | 165 +++++++++++++++++++++++++-----
 hw/acpi/ich9.c                    |  24 ++++-
 hw/core/machine.c                 |  16 +--
 hw/i386/acpi-build.c              |  35 ++++++-
 hw/i386/pc.c                      |  20 ++--
 hw/i386/x86.c                     |  11 ++
 hw/isa/lpc_ich9.c                 |  16 +++
 tests/data/acpi/pc/DSDT           | Bin 4934 -> 5060 bytes
 tests/data/acpi/pc/DSDT.acpihmat  | Bin 6258 -> 6385 bytes
 tests/data/acpi/pc/DSDT.bridge    | Bin 6793 -> 6919 bytes
 tests/data/acpi/pc/DSDT.cphp      | Bin 5397 -> 5524 bytes
 tests/data/acpi/pc/DSDT.dimmpxm   | Bin 6587 -> 6714 bytes
 tests/data/acpi/pc/DSDT.ipmikcs   | Bin 5006 -> 5132 bytes
 tests/data/acpi/pc/DSDT.memhp     | Bin 6293 -> 6419 bytes
 tests/data/acpi/pc/DSDT.numamem   | Bin 4940 -> 5066 bytes
 tests/data/acpi/q35/DSDT          | Bin 7678 -> 7804 bytes
 tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9129 bytes
 tests/data/acpi/q35/DSDT.bridge   | Bin 7695 -> 7821 bytes
 tests/data/acpi/q35/DSDT.cphp     | Bin 8141 -> 8268 bytes
 tests/data/acpi/q35/DSDT.dimmpxm  | Bin 9331 -> 9458 bytes
 tests/data/acpi/q35/DSDT.ipmibt   | Bin 7753 -> 7879 bytes
 tests/data/acpi/q35/DSDT.memhp    | Bin 9037 -> 9163 bytes
 tests/data/acpi/q35/DSDT.mmio64   | Bin 8808 -> 8934 bytes
 tests/data/acpi/q35/DSDT.numamem  | Bin 7684 -> 7810 bytes
 tests/data/acpi/q35/DSDT.tis      | Bin 8283 -> 8409 bytes
 30 files changed, 269 insertions(+), 67 deletions(-)

Comments

Igor Mammedov Sept. 23, 2020, 9:55 a.m. UTC | #1
On Wed, 23 Sep 2020 05:46:50 -0400
Igor Mammedov <imammedo@redhat.com> wrote:

> it's was deprecated since 3.1
> 
> Support for invalid topologies is removed, the user must ensure
> that topologies described with -smp include all possible cpus,
> i.e. (sockets * cores * threads) == maxcpus or QEMU will
> exit with error.

ignore this stowaway, it's not part of the series 


> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  docs/system/deprecated.rst | 26 +++++++++++++-------------
>  hw/core/machine.c          | 16 ++++------------
>  hw/i386/pc.c               | 16 ++++------------
>  3 files changed, 21 insertions(+), 37 deletions(-)
> 
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 808c334fe7..fb95d2ecc4 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -47,19 +47,6 @@ The 'file' driver for drives is no longer appropriate for character or host
>  devices and will only accept regular files (S_IFREG). The correct driver
>  for these file types is 'host_cdrom' or 'host_device' as appropriate.
>  
> -``-smp`` (invalid topologies) (since 3.1)
> -'''''''''''''''''''''''''''''''''''''''''
> -
> -CPU topology properties should describe whole machine topology including
> -possible CPUs.
> -
> -However, historically it was possible to start QEMU with an incorrect topology
> -where *n* <= *sockets* * *cores* * *threads* < *maxcpus*,
> -which could lead to an incorrect topology enumeration by the guest.
> -Support for invalid topologies will be removed, the user must ensure
> -topologies described with -smp include all possible cpus, i.e.
> -*sockets* * *cores* * *threads* = *maxcpus*.
> -
>  ``-vnc acl`` (since 4.0.0)
>  ''''''''''''''''''''''''''
>  
> @@ -642,6 +629,19 @@ New machine versions (since 5.1) will not accept the option but it will still
>  work with old machine types. User can check the QAPI schema to see if the legacy
>  option is supported by looking at MachineInfo::numa-mem-supported property.
>  
> +``-smp`` (invalid topologies) (removed 5.2)
> +'''''''''''''''''''''''''''''''''''''''''''
> +
> +CPU topology properties should describe whole machine topology including
> +possible CPUs.
> +
> +However, historically it was possible to start QEMU with an incorrect topology
> +where *n* <= *sockets* * *cores* * *threads* < *maxcpus*,
> +which could lead to an incorrect topology enumeration by the guest.
> +Support for invalid topologies is removed, the user must ensure
> +topologies described with -smp include all possible cpus, i.e.
> +*sockets* * *cores* * *threads* = *maxcpus*.
> +
>  Block devices
>  -------------
>  
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ea26d61237..09aee4ea52 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -754,23 +754,15 @@ static void smp_parse(MachineState *ms, QemuOpts *opts)
>              exit(1);
>          }
>  
> -        if (sockets * cores * threads > ms->smp.max_cpus) {
> -            error_report("cpu topology: "
> -                         "sockets (%u) * cores (%u) * threads (%u) > "
> -                         "maxcpus (%u)",
> +        if (sockets * cores * threads != ms->smp.max_cpus) {
> +            error_report("Invalid CPU topology: "
> +                         "sockets (%u) * cores (%u) * threads (%u) "
> +                         "!= maxcpus (%u)",
>                           sockets, cores, threads,
>                           ms->smp.max_cpus);
>              exit(1);
>          }
>  
> -        if (sockets * cores * threads != ms->smp.max_cpus) {
> -            warn_report("Invalid CPU topology deprecated: "
> -                        "sockets (%u) * cores (%u) * threads (%u) "
> -                        "!= maxcpus (%u)",
> -                        sockets, cores, threads,
> -                        ms->smp.max_cpus);
> -        }
> -
>          ms->smp.cpus = cpus;
>          ms->smp.cores = cores;
>          ms->smp.threads = threads;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2af660c55e..53e21a186d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -748,23 +748,15 @@ void pc_smp_parse(MachineState *ms, QemuOpts *opts)
>              exit(1);
>          }
>  
> -        if (sockets * dies * cores * threads > ms->smp.max_cpus) {
> -            error_report("cpu topology: "
> -                         "sockets (%u) * dies (%u) * cores (%u) * threads (%u) > "
> -                         "maxcpus (%u)",
> +        if (sockets * dies * cores * threads != ms->smp.max_cpus) {
> +            error_report("Invalid CPU topology deprecated: "
> +                         "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
> +                         "!= maxcpus (%u)",
>                           sockets, dies, cores, threads,
>                           ms->smp.max_cpus);
>              exit(1);
>          }
>  
> -        if (sockets * dies * cores * threads != ms->smp.max_cpus) {
> -            warn_report("Invalid CPU topology deprecated: "
> -                        "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
> -                        "!= maxcpus (%u)",
> -                        sockets, dies, cores, threads,
> -                        ms->smp.max_cpus);
> -        }
> -
>          ms->smp.cpus = cpus;
>          ms->smp.cores = cores;
>          ms->smp.threads = threads;
Laszlo Ersek Sept. 23, 2020, 4:44 p.m. UTC | #2
On 09/23/20 11:46, Igor Mammedov wrote:
> v6:
>   - [9/10] Add comment explaining why while_ctx2 restarts from the last processed CPU.
>   - rebase on top of current master, due to non trivial conflict
>     caused by microvm series, which moved/renamed pc_cpu_pre_plug()

So, I went back to my local branch where I had applied your v5, *plus*
the comment fixup ("[PATCH v5 9/10] fixup! x68: acpi: trigger SMI before
sending hotplug Notify event to OSPM") on top. I rebased that branch to
its *same* base commit, only squashing the comment fixup into patch#9.

Then I applied your v6 series on top of current master, using a
different (new) local branch.

Then I ran git-range-diff on these two local branches.

In patches 6, 7, 8, and 9, you've picked up my feedback tags from the v5
review session; that's good, there was nothing else to do.

There is a trivial difference in patch 2 -- trivial to review, that is;
I'm not saying that it's so trivial that git-rebase should have coped
with it automatically on your end. Here's the git-range-diff output:

>  2:  e606a75432a8 !  2:  94702d2e3125 x86: cpuhp: prevent guest crash on CPU hotplug when broadcast SMI is in use
>     @@ -12,7 +12,7 @@
>          Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>          Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>          Tested-by: Laszlo Ersek <lersek@redhat.com>
>     -    Message-Id: <20200907112348.530921-3-imammedo@redhat.com>
>     +    Message-Id: <20200923094650.1301166-3-imammedo@redhat.com>
>
>      diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>      --- a/hw/acpi/ich9.c
>     @@ -40,17 +40,17 @@
>
>       void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>
>     -diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>     ---- a/hw/i386/pc.c
>     -+++ b/hw/i386/pc.c
>     +diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>     +--- a/hw/i386/x86.c
>     ++++ b/hw/i386/x86.c
>      @@
>               return;
>           }
>
>     -+    if (pcms->acpi_dev) {
>     ++    if (x86ms->acpi_dev) {
>      +        Error *local_err = NULL;
>      +
>     -+        hotplug_handler_pre_plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev,
>     ++        hotplug_handler_pre_plug(HOTPLUG_HANDLER(x86ms->acpi_dev), dev,
>      +                                 &local_err);
>      +        if (local_err) {
>      +            error_propagate(errp, local_err);

Meaning that, in v6, you had to refer to "x86ms", rather than to "pcms",
and that the code had to be introduced in a different file / function.

The need for that originates from 0cca1a918b85 ("x86: move cpu hotplug
from pc to x86", 2020-09-17).

It looks innocent enough, but I should still retest patch#2. I'll report
back under that patch in this series.

Thanks
Laszlo
Igor Mammedov Sept. 24, 2020, 7:12 a.m. UTC | #3
On Wed, 23 Sep 2020 18:44:50 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 09/23/20 11:46, Igor Mammedov wrote:

> > v6:

> >   - [9/10] Add comment explaining why while_ctx2 restarts from the last processed CPU.

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

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

> 

> So, I went back to my local branch where I had applied your v5, *plus*

> the comment fixup ("[PATCH v5 9/10] fixup! x68: acpi: trigger SMI before

> sending hotplug Notify event to OSPM") on top. I rebased that branch to

> its *same* base commit, only squashing the comment fixup into patch#9.

> 

> Then I applied your v6 series on top of current master, using a

> different (new) local branch.

> 

> Then I ran git-range-diff on these two local branches.

> 

> In patches 6, 7, 8, and 9, you've picked up my feedback tags from the v5

> review session; that's good, there was nothing else to do.

> 

> There is a trivial difference in patch 2 -- trivial to review, that is;

> I'm not saying that it's so trivial that git-rebase should have coped

> with it automatically on your end. Here's the git-range-diff output:

> 

> >  2:  e606a75432a8 !  2:  94702d2e3125 x86: cpuhp: prevent guest crash on CPU hotplug when broadcast SMI is in use

> >     @@ -12,7 +12,7 @@

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

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

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

> >     -    Message-Id: <20200907112348.530921-3-imammedo@redhat.com>

> >     +    Message-Id: <20200923094650.1301166-3-imammedo@redhat.com>

> >

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

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

> >     @@ -40,17 +40,17 @@

> >

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

> >

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

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

> >     -+++ b/hw/i386/pc.c

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

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

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

> >      @@

> >               return;

> >           }

> >

> >     -+    if (pcms->acpi_dev) {

> >     ++    if (x86ms->acpi_dev) {

> >      +        Error *local_err = NULL;

> >      +

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

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

> >      +                                 &local_err);

> >      +        if (local_err) {

> >      +            error_propagate(errp, local_err);  

> 

> Meaning that, in v6, you had to refer to "x86ms", rather than to "pcms",

> and that the code had to be introduced in a different file / function.

> 

> The need for that originates from 0cca1a918b85 ("x86: move cpu hotplug

> from pc to x86", 2020-09-17).

I should have added this commit to change log to spare you
trouble figuring out what exactly has changed.

> 

> It looks innocent enough, but I should still retest patch#2. I'll report

> back under that patch in this series.

> 

> Thanks

> Laszlo