RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug

[Salil Mehta via <qemu-devel@nongnu.org>]

>  From: Zhao Liu <zhao1.liu@intel.com>
>  Sent: Monday, May 6, 2024 10:06 AM
>  
>  Hi Salil,
>  
>  On Fri, May 03, 2024 at 07:59:32PM +0000, Salil Mehta wrote:
>  > Date: Fri, 3 May 2024 19:59:32 +0000
>  > From: Salil Mehta <salil.mehta@huawei.com>
>  > Subject: RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to
>  > support  vCPU Hotplug
>  >
>  > Hello,
>  >
>  > Sorry, I missed this earlier.
>  >
>  > >  From: Zhao Liu <zhao1.liu@intel.com>
>  > >  Sent: Wednesday, March 13, 2024 6:14 AM
>  > >  To: Salil Mehta <salil.mehta@huawei.com>
>  > >
>  > >  Hi Salil,
>  > >
>  > >  It seems my comment [1] in v7 was missed, but I still hit the same
>  > > issue. Pls  let me paste the previous comment here again.
>  > >
>  > >  [1]: https://lore.kernel.org/qemu- devel/ZXCqp32ggIFvUweu@intel.com/
>  >
>  > Yes, I have this in my mind.
>  >
>  > >
>  > >  [snip]
>  > >
>  > >  > @@ -400,6 +411,12 @@ static void acpi_ged_initfn(Object *obj)
>  > >  >      memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops,  ged_st,
>  > >  >                            TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
>  > >  >      sysbus_init_mmio(sbd, &ged_st->regs);
>  > >  > +
>  > >  > +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp
>  > >  container",
>  > >  > +                       ACPI_CPU_HOTPLUG_REG_LEN);
>  > >  > +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
>  > >  > +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
>  > >  > +                        &s->cpuhp_state, 0);
>  > >  >  }
>  > >  >
>  > >
>  > >  I find this cpu_hotplug_hw_init() can still cause qtest errors (for
>  > > v8) on x86  platforms as you mentioned in v6:
>  > >  https://lore.kernel.org/qemu-devel/15e70616-6abb-63a4-17d0-820f4a254607@opnsrc.net/T/#m108f102b2fe92b7dd7218f2f942f7b233a9d6af3
>  > >
>  > >  IIUC, microvm machine has its own 'possible_cpus_arch_ids' and that
>  > > is  inherited from its parent x86 machine.
>  > >
>  > >  The above error is because device-introspect-test sets the none-machine:
>  > >
>  > >  # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-
>  > > 3094820.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-
>  > >  3094820.qmp,id=char0 -mon chardev=char0,mode=control -display none
>  > > -  audio none -nodefaults -machine none -accel qtest
>  > >
>  > >  So what about just checking mc->possible_cpu_arch_ids instead of an
>  > > assert in cpu_hotplug_hw_init()?
>  > >
>  > >  diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index
>  > > 4b24a2500361..303f1f1f57bc 100644
>  > >  --- a/hw/acpi/cpu.c
>  > >  +++ b/hw/acpi/cpu.c
>  > >  @@ -221,7 +221,10 @@ void cpu_hotplug_hw_init(MemoryRegion *as,
>  > > Object *owner,
>  > >       const CPUArchIdList *id_list;
>  > >       int i;
>  > >
>  > >  -    assert(mc->possible_cpu_arch_ids);
>  > >  +    if (!mc->possible_cpu_arch_ids) {
>  > >  +        return;
>  > >  +    }
>  > >  +
>  >
>  >
>  > Yes, we can do this with some debug print or trace maybe.
>  
>  Here it is just to return early without touching mc->possible_cpu_arch_ids().
>  If you adopt this workaround, then in the meantime I suggest adding a
>  comment to this "if" to clarify that it is for compatibility with certain
>  machines that do not implement mc->possible_cpu_arch_ids().


sure.


>  
>  > >       id_list = mc->possible_cpu_arch_ids(machine);
>  > >       state->dev_count = id_list->len;
>  > >       state->devs = g_new0(typeof(*state->devs), state->dev_count);
>  > >
>  > >  This check seems to be acceptable in the general code path? Not all
>  > > machines have possible_cpu_arch_ids, after all.
>  >
>  > True. BTW, have you tested this with Qtest?
>  
>  Yes, by "make check" on x86 platform. This workaround can help us pass the
>  x86 tests.

thanks.

Best
Salil.