RE: [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability

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

Hi Igor,

>  From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org <qemu-
>  arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Igor
>  Mammedov
>  Sent: Wednesday, November 6, 2024 1:57 PM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  On Wed, 6 Nov 2024 13:03:30 +0000
>  Salil Mehta <salil.mehta@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 <imammedo@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
and physical CPU hot plug can co-exists or entirely as sole features.  This is
a requirement.


>  
>  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.


>       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

>  
>  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.


Thanks
Salil.