On Wed, 23 Sep 2020 18:44:50 +0200
Laszlo Ersek <ler...@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 <imamm...@redhat.com>
> > Reviewed-by: Laszlo Ersek <ler...@redhat.com>
> > Tested-by: Laszlo Ersek <ler...@redhat.com>
> > - Message-Id: <20200907112348.530921-3-imamm...@redhat.com>
> > + Message-Id: <20200923094650.1301166-3-imamm...@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
