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

[Igor Mammedov]

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