Hi Igor,
Many thanks for taking time to reply.
> From: qemu-arm-bounces+salil.mehta=huawei....@nongnu.org <qemu-
> arm-bounces+salil.mehta=huawei....@nongnu.org> On Behalf Of Igor
> Mammedov
> Sent: Thursday, November 7, 2024 4:57 PM
> To: Salil Mehta <salil.me...@huawei.com>
>
> On Wed, 6 Nov 2024 19:05:15 +0000
> Salil Mehta <salil.me...@huawei.com> wrote:
>
> > Hi Igor,
> >
> > Thanks for replying back and the reviews. Please find my replies
> > inline.
> >
> > > From: Igor Mammedov <imamm...@redhat.com>
> > > Sent: Wednesday, November 6, 2024 4:08 PM
> > > To: Salil Mehta <salil.me...@huawei.com>
> > >
> > > On Wed, 6 Nov 2024 14:45:42 +0000
> > > Salil Mehta <salil.me...@huawei.com> wrote:
> > >
> > > > Hi Igor,
> > > >
> > > > > From: qemu-arm-bounces+salil.mehta=huawei....@nongnu.org
> > > <qemu-
> > > > > arm-bounces+salil.mehta=huawei....@nongnu.org> On Behalf Of
> > > Igor > > Mammedov > > Sent: Wednesday, November 6, 2024 1:57 PM
> > > > > To: Salil Mehta <salil.me...@huawei.com> > > > > On Wed, 6
> > > Nov 2024 13:03:30 +0000 > > Salil Mehta <salil.me...@huawei.com>
> > > wrote:
> > > > >
> > > > > > Checking `is_present` first can break x86 migration from new
> > > Qemu > > > version to old Qemu. This is because CPRS Bit is not
> > > defined in > > the > older Qemu register block and will always be
> > > 0 resulting in > > check > always failing. Reversing the logic to
> > > first check > > `is_enabled` can > alleviate below problem:
> > > > > >
> > > > > > - If ((\_SB.PCI0.PRES.CPEN == One))
> > > > > > - {
> > > > > > - Local0 = 0x0F
> > > > > > + If ((\_SB.PCI0.PRES.CPRS == One))
> > > > > > + {
> > > > > > + If ((\_SB.PCI0.PRES.CPEN == One))
> > > > > > + {
> > > > > > + Local0 = 0x0F
> > > > > > + }
> > > > > > + Else
> > > > > > + {
> > > > > > + Local0 = 0x0D
> > > > > > + }
> > > > > > }
> > > > > >
> > > > > > Suggested-by: Igor Mammedov <imamm...@redhat.com>
> > > 'Reported-by'
> > > > > maybe, but certainly not suggested.
> > > >
> > > >
> > > > No issues. I can change.
> > > >
> > > >
> > > > >
> > > > > After more thinking and given presence is system wide that
> > > doesn't > > change at runtime, I don't see any reason for
> > > introducing presence > > bit as ABI (and undocumented on top of that).
> > > >
> > > >
> > > > This is a wrong assumption. Presence bit can change in future. We
> > > have > taken into account this aspect by design in the kernel code as
> well.
> > > > Both virtual
> > >
> > > Above does imply that support for really hotpluggable CPUs might be
> > > implemented in the future.
> > > Can you point to relevant kernel code/patches?
> >
> >
> > Let me make it clear so that there is no confusion, there is no
> > support of physical "CPU" hot-plug on ARM platforms right now and nor
> > will there be any in future as it does not makes sense to have. The
> > point I made in the patches is about hot-plug was at different
> > granularity which has not been denied by ARM.
>
> _STA and ACPI cphp registers are per logical CPU and can't be anything else
> per spec.
>
> how different granularity is relevant here?
It is because hot-plug at socket level (or similar level) has not been denied.
> > > > and physical CPU hot plug can co-exists or entirely as sole features.
> > > > This is a requirement.
> > >
> > >
> > > I don't see any _must_ requirements here whatsoever. If/when ARM
> > > reaches point where standby and hotplug cpus are mixed within VM
> > > instance, we might have to expose presence bit to guest dynamically
> > > but it is totally not needed within observable future and premature.
> >
> >
> > > Your cpu class presence check also works target-wise just with more
> > > boiler code + not needed presence bit ABI for guest side, The
> > > same as my suggestion only from other side.
> > >
> > > But regardless of that as long as machine has doesn't mix always
> > > present CPUs with hotpluggable ones within the same instance,
> > > changing AML side as I've just suggested works.
> >
> >
> > Sure, I'm not disagreeing. It will work by adding the flag you've
> > suggested but it will work even by not adding any flag and not
> > defining a `persistent` hook for any architecture.
>
> hook is more complicated and hidden way, than directly passing down
> configuration data to routine that builds AML where it is needed, but that's
> cosmetics in the end.
It is indeed an abstraction and an intentional one because decision whether
some or all CPUs should remain present even after unplug action is
specific to an architecture. Hence, should be left with that part.
>
> However there is more to your hook approach, it exposes is_present bit in
> cphp flag register which is ABI to guest which we will have to maintain
> forever and guest will do not necessary round-trip to QEMU to query it,
> while alternative is much simpler AML change without any ABI changes to
> care about.
I understand your predicament about ABI but I've to respectfully disagree on
the assumption that guest will never round trip and check for the presence bit.
>
> The point is we shouldn't add new ABI unless we have to.
Ok sure. point taken.
> In this case new ABI (is_presence flag) is not _must_, and much simpler
> static AML change is sufficient.
Agreed, there can be multiple ways to get rid of it . But I'm not sure if its
simpler or easier to maintain because CPUs AML code has become even
more obscure by that compact logic. It is not easy to understand at the
first look. This is not a performance leg and maintainability should take
over the precedence.
But I understand your primary concern now (i.e. to get rid of CPU_PRESENT
Bit) and allow me to propose in the lines of what you want but maybe in a
slightly different way. I would still like to retain hooks though.
> > > If ARM ever gets real cpu hotplug as your comment above hints, my
> > > suggestion also works fine. With only difference that board code
> > > would turn off always_present_cpus if hotpluggable CPUs are used
> instead of standby.
> >
> >
> > Sure, but is it necessary to define a new flag when you can do even
> without it?
> > Having few lines of extra code (literally 2-3 only) should not be a
> > major cause of worry, especially, if it makes design more clear and
> > easy to understand. This is not a performance code either.
> >
> > Again, I appreciate your compact logic. I'm not disagreeing with it.
> >
> >
> > > > > Instead changing AML code to account for it would be better,
> > > > > something like > > this:
> > > > >
> > > > > diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> > > index > > 32654dc274..4a3e591120 100644 > > ---
> > > a/include/hw/acpi/cpu.h > > +++ b/include/hw/acpi/cpu.h > > @@
> > > -55,6 +55,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object
> >
> > > > *owner, typedef struct CPUHotplugFeatures {
> > > > > bool acpi_1_compatible;
> > > > > bool has_legacy_cphp;
> > > > > + bool always_present_cpus;
> > > >
> > > >
> > > > This has to be fetched from architecture code. Please see other
> > > > changes in the V3 patch-set.
> > >
> > > I've seen, that and it doesn't have to be fetched dynamically.
> >
> >
> > Agreed, it is not necessary to be fetched. Hence, if you do not define
> > the hook. In later case, it assumes the existing logic, which x86 has been
> following.
> > It will work.
>
> It is better to get rid of not necessary hook altogether, and replace it
> with a
> simple AML change.
We still need a way to initialize flags in the architecture specific way. Hooks
are
clean way to fetch that architecture specific state rather fiddling with flags
at multiple levels (initialization, migration etc) and you will be duplicating
CPU states at ACPI level and QOM vCPU object level . To be frank that's not
a very good design.
That said, I believe your end goal of getting rid of the change in the ABI
can still be achieved.
> > > In my opinion the series was not ready to be merged (Michael
> > > probably picked it by mistake).
> > >
> > > We don't really need this in 9.2 as it is only ARM cpu 'hotplug'
> > > related, and the later is not ready for 9.2.
> > > I'd prefer the series being reverted and we continue improving
> > > series, instead of rushing it and fixing broken thing up.
> > >
> > >
> > > >
> > > >
> > > > > bool fw_unplugs_cpu;
> > > > > const char *smi_path;
> > > > > } CPUHotplugFeatures;
> > > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index > >
> > > 5cb60ca8bc..2bcce2b31c > > 100644 > > --- a/hw/acpi/cpu.c > >
> > > +++ b/hw/acpi/cpu.c > > @@ -452,15 +452,16 @@ void
> > > build_cpus_aml(Aml *table, MachineState > > *machine,
> > > CPUHotplugFeatures opts, > >
> > > > > method = aml_method(CPU_STS_METHOD, 1,
> AML_SERIALIZED);
> > > > > {
> > > > > + uint8_t default_sta = opts.always_present_cpus?0xd:0;
> > > > > Aml *idx = aml_arg(0);
> > > > > - Aml *sta = aml_local(0);
> > > > > + Aml *sta = aml_local(default_sta);
> > > > >
> > > > > aml_append(method, aml_acquire(ctrl_lock, 0xFFFF));
> > > > > aml_append(method, aml_store(idx, cpu_selector));
> > > > > aml_append(method, aml_store(zero, sta));
> > > > > ifctx = aml_if(aml_equal(is_enabled, one));
> > > > > {
> > > > > - aml_append(ifctx, aml_store(aml_int(0xF), sta));
> > > > > + aml_append(ifctx, aml_or(aml_int(0xF), sta, sta));
> > > > > }
> > > > > aml_append(method, ifctx);
> > > > > aml_append(method, aml_release(ctrl_lock))
> > > > >
> > > > > then for ARM set
> > > > > CPUHotplugFeatures::always_present_cpus = true to get present
> flag
> > > > > always enabled
> > > >
> > > >
> > > > We MUST fetch this from architecture code as this can dynamically
> > > > change in future and hence, we need to keep that flexibility
> > >
> > > CPUHotplugFeatures::always_present_cpus can be set dynamically per
> > > VM instance or per Machine type.
> >
> >
> > Yes, but you need a code for that and I'm not sure what is being saved
> > by introducing an extra flag then?
>
> beside snippet, I've suggested. You need to delete all is_present callback
> related logic, than it save quite a bit, and not only on lines of code.
>
> I guess, I need to send a patch to get point across.
There is some misunderstanding. We are not maintaining any `is_present`
state at the ACPI level. Both flags `is_present` and `is_enabled` were
removed in V3 patch-set.
> > > > > After that revert _all_ other presence bit related changes
> > > that > > were just merged.
> > > > > (I did ask to get rid of that in previous reviews but it came
> > > back > > again for no good reason).
> > > >
> > > >
> > > > The CPUs AML in the V2 patch-set would have broken the x86
> > > functionality.
> > >
> > > Frankly speaking, I don't see much progress. All that happens on
> > > respins is flipping between what I asked to remove before to some
> earlier concept.
> >
> >
> > I think then you've missed the code which addressed one of your key
> > concerns in the V1 patch-set that would have broken x86 migration i.e.
> > presence of the `is_enabled` state in the CPU Hot-plug VMSD. That has
> > been removed. Maybe you would like to go through the change log of the
> > V3 patch-set
> >
> > https://lore.kernel.org/qemu-
> devel/20241018163118.0ae01...@imammedo.us
> > ers.ipa.redhat.com/
> >
> > Below is the relevant excerpt from your many comments:
> >
> > [...]
> > > .fields = (const VMStateField[]) {
> > > VMSTATE_BOOL(is_inserting, AcpiCpuStatus),
> > > VMSTATE_BOOL(is_removing, AcpiCpuStatus),
> > > + VMSTATE_BOOL(is_present, AcpiCpuStatus),
> > > + VMSTATE_BOOL(is_enabled, AcpiCpuStatus),
> >
> > that's likely will break x86 migration, but before bothering peterx,
> > maybe we won't need this hunk if is_enabled is migrated as part of
> > vCPU state.
> >
>
> what has been done in v3 is moving is_present/is_enabled, elsewhere
> (callbacks in this case). While argument was that both are not necessary to
> begin with.
We need to have a way to check whether CPU is _STA.Enabled or not and
the meaning of the `disabled` is best decided by the architecture specific
code not the ACPI code. It could mean different across architectures and
the implementations. We need to keep that flexibility in design.
The thin Qemu ACPI code is just an interfacing logic with the guest. It is
best not to duplicate the state of the QOM objects inside the ACPI. That's
a bad design practice because we will have to worry about keeping those
states consistent at both the places all the time whether it is during
initialization or migration.
>
> > [...]
> >
> >
> > > And all of that for the purpose to workaround/accommodate fake cpu
> > > hotplug hacks.
> >
> >
> > I've to respectfully disagree on this. This is your assumption which
> > is far from reality. The accompanying main series of this will never
> > have vCPUs which are
> > *not* present.
>
> Reality of your last posted main series (v5), disagrees with what you are
> saying here
>
> [PATCH RFC V5 12/30] arm/virt: Release objects for *disabled* possible
> vCPUs after init
> https://patchew.org/QEMU/20241015100012.254223-1-
> salil.me...@huawei.com/20241015100012.254223-13-
> salil.me...@huawei.com/
I think you missed what I mentioned above " The accompanying main series...".
To get more information about that series, please check the details mentioned
in the cover-letter of the V3 patch-set (link below) which clearly says,
https://lore.kernel.org/qemu-devel/20241103102419.202225-1-salil.me...@huawei.com/
Excerpt from the cover letter:
========================
[...]
This patch-set has been tested alongside ARM-specific
vCPU hotplug changes (included in the upcoming RFC V6 [4]), and
hot(un)plug functionalities performed as expected which concerns this
patch-set. Please have a look.
[...]
References
==========
[...]
[4] Upcoming RFC V6, Support of Virtual CPU Hotplug for ARMv8 Arch
Link:
https://github.com/salil-mehta/qemu/commits/virt-cpuhp-armv8/rfc-v6-rc5
[...]
> after this patch, new is_present hook would let you bury the lie about CPU
> being present inside ARM specific code.
I would request you to check the facts again! (please see previous pointers)
ACPI logic should not worry about what is happening inside architecture
code. It is beyond its realms and should be by design. It is an interfacing
upper
layer gelling the Guest with the VMM.
> > BTW, these changes have been tested by Oracle folks with that series
> > and are known to work.
> >
> > https://lore.kernel.org/qemu-devel/C4FEC9E7-69DB-428A-A85F-
> 30170F98814
> > b...@oracle.com/
>
> One can write anything and it can even work somehow, that however
> doesn't mean it's something merge-able, maintainable, ...whatever else
> come to mind...
It would be very helpful if you can `objectively` point the problems in the
main series so that there is a context and we can have a productive
discussion there. There is always a scope of improvement.
Many thanks!
Best regards
Salil.
