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).
It looks innocent enough, but I should still retest patch#2. I'll report
back under that patch in this series.
Thanks
Laszlo
