Re: [PATCH v2 02/25] hw/arm/virt: Introduce machine state acpi pcihp flags and props

[Eric Auger <eric.auger@redhat.com>]

Hi Igor,
On 6/11/25 10:45 AM, Igor Mammedov wrote:
> On Wed, 11 Jun 2025 08:53:28 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> Hi Gustavo, Alex,
>>
>> On 5/28/25 12:33 PM, Igor Mammedov wrote:
>>> On Tue, 27 May 2025 15:54:15 +0200
>>> Eric Auger <eric.auger@redhat.com> wrote:
>>>  
>>>> Hi Igor,
>>>>
>>>> On 5/27/25 1:58 PM, Igor Mammedov wrote:  
>>>>> On Tue, 27 May 2025 09:40:04 +0200
>>>>> Eric Auger <eric.auger@redhat.com> wrote:
>>>>>    
>>>>>> acpi_pcihp VirtMachineClass state flag will allow
>>>>>> to opt in for acpi pci hotplug. This is guarded by a
>>>>>> class no_acpi_pcihp flag to manage compats (<= 10.0
>>>>>> machine types will not support ACPI PCI hotplug).    
>>>>> there is no reason to put an effort in force disabling it
>>>>> on old machines, as long as code works when explicitly
>>>>> enabled property on CLI.
>>>>>
>>>>> See comment below on how to deal with it 
>>>>>    
>>>>>> Machine state acpi_pcihp flag must be set before the creation
>>>>>> of the GED device which will use it.
>>>>>>
>>>>>> Currently the ACPI PCI HP is turned off by default. This will
>>>>>> change later on for 10.1 machine type.    
>>>>> one thing to note, is that turning it on by default might
>>>>> cause change of NIC naming in guest as this brings in
>>>>> new "_Sxx" slot naming. /so configs tied to nic  go down the drain/
>>>>>
>>>>> Naming, we have, also happens to be broken wrt spec
>>>>> (it should be unique system wide, there was a gitlab issue for that,
>>>>> there is no easy fix that though)
>>>>>
>>>>> So I'd leave it disabled by default and let users to turn
>>>>> it on explicitly when needed.     
>>>> what is the status on q35, isn't it enabled by default? If so why
>>>> wouldn't we want the same setting on ARM? Is that because of the known
>>>> issue you report above?  
>>> Above issue is not a blocker (for thae lack of a good way to fix it)
>>>
>>> on q35 we have had a few complains and fixes, after pcihp was promoted
>>> to default (so hopefully that won't happen on with ARM). Also given
>>> that ARM VM is less popular like hood breaking someone setup is even less.
>>>
>>> That said I'd be cautions keep native hotplug as default,
>>> and only ones who need ACPI one, could turn it on explicitly.
>>>
>>> But well it's policies, so it's up to you ARM folks to decide what
>>> virt board should look like.  
>> What is your preference? Do you prefer enabling ACPI PCI HP by default
>> or the opposite.
> I'd prefer native PCIe hotplug being default,
> that way we have less chance of causing regressions not to mention
> less complexity (as acpi pcihp adds up quite a bit of it).
>
> And ones who want/need acpi-pcihp/acpi-index can enable it explicitly,
> to play with.

OK I will follow your suggestion. You have definitively more expertise
than me here ! ;-)

Thanks!

Eric
>
>> Anybody else?
>>
>> On my end I think I would prefer to have the same default setting than
>> on x86 (ie. ACPI PCI hotplug set by default) but I have no strong
>> opinion either.
>>
>> Thanks
>>
>> Eric
>>>  
>>>> The no_foo compat stuff was especially introduced to avoid breaking the
>>>> guest ABI for old machine types (like the NIC naming alternation you evoke).  
>>> no_foo is just another way to handle compat stuff,
>>> and when it's more than one knob per feature it gets ugly really fast.
>>> Hence, I'd prefer pcihp done in x86 way aka:
>>>    hw_compat_OLD(ged.use_acpi_hotplug_bridge, false|true)
>>> to manage presence of ACPI hotplug on desired machine version.
>>> Side benefit it's consistent with how pcihp works on x86
>>>  
>>>>>    
>>>>>> We also introduce properties to allow disabling it.
>>>>>>
>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
>>>>>> ---
>>>>>>  include/hw/arm/virt.h |  2 ++
>>>>>>  hw/arm/virt.c         | 27 +++++++++++++++++++++++++++
>>>>>>  2 files changed, 29 insertions(+)
>>>>>>
>>>>>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>>>>>> index 9a1b0f53d2..10ea581f06 100644
>>>>>> --- a/include/hw/arm/virt.h
>>>>>> +++ b/include/hw/arm/virt.h
>>>>>> @@ -129,6 +129,7 @@ struct VirtMachineClass {
>>>>>>      bool no_tcg_lpa2;
>>>>>>      bool no_ns_el2_virt_timer_irq;
>>>>>>      bool no_nested_smmu;
>>>>>> +    bool no_acpi_pcihp;
>>>>>>  };
>>>>>>  
>>>>>>  struct VirtMachineState {
>>>>>> @@ -150,6 +151,7 @@ struct VirtMachineState {
>>>>>>      bool mte;
>>>>>>      bool dtb_randomness;
>>>>>>      bool second_ns_uart_present;
>>>>>> +    bool acpi_pcihp;
>>>>>>      OnOffAuto acpi;
>>>>>>      VirtGICType gic_version;
>>>>>>      VirtIOMMUType iommu;
>>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>>>> index 9a6cd085a3..a0deeaf2b3 100644
>>>>>> --- a/hw/arm/virt.c
>>>>>> +++ b/hw/arm/virt.c
>>>>>> @@ -2397,8 +2397,10 @@ static void machvirt_init(MachineState *machine)
>>>>>>      create_pcie(vms);
>>>>>>  
>>>>>>      if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
>>>>>> +        vms->acpi_pcihp &= !vmc->no_acpi_pcihp;    
>>>>> I don't particularly like no_foo naming as it makes code harder to read
>>>>> and combined with 'duplicated' field in machine state it make even things worse.
>>>>> (if I recall right Philippe was cleaning mess similar flags usage
>>>>> have introduced with ITS)
>>>>>
>>>>> instead of adding machine property (both class and state),
>>>>> I'd suggest adding the only property to GPE device (akin to what we have in x86 world)
>>>>> And then one can meddle with defaults using hw_compat_xxx    
>>>> no_foo still is a largely used pattern in arm virt: no_ged,
>>>> kvm_no_adjvtime, no_kvm_steal_time, no_tcg_lpa2, ../.. There are plenty
>>>> of them and I am not under the impression this is going to be changed.
>>>>
>>>> If you refer to 8d23b1df7212 ("hw/arm/virt: Remove
>>>> VirtMachineClass::no_its field") I think the no_its was removed because
>>>> the machine it applied was removed.
>>>>
>>>> If I understand correctly you would like the prop to be attached to the
>>>> GED device. However the GED device is internally created by the virt
>>>> machine code and not passed through a "-device" CLI option. So how would
>>>> you pass the option on the cmd line if you don't want it to be set by
>>>> default per machine type?
>>>>
>>>> Thanks
>>>>
>>>> Eric  
>>>>>    
>>>>>>          vms->acpi_dev = create_acpi_ged(vms);
>>>>>>      } else {
>>>>>> +        vms->acpi_pcihp = false;
>>>>>>          create_gpio_devices(vms, VIRT_GPIO, sysmem);
>>>>>>      }
>>>>>>  
>>>>>> @@ -2593,6 +2595,20 @@ static void virt_set_its(Object *obj, bool value, Error **errp)
>>>>>>      vms->its = value;
>>>>>>  }
>>>>>>  
>>>>>> +static bool virt_get_acpi_pcihp(Object *obj, Error **errp)
>>>>>> +{
>>>>>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>>>>>> +
>>>>>> +    return vms->acpi_pcihp;
>>>>>> +}
>>>>>> +
>>>>>> +static void virt_set_acpi_pcihp(Object *obj, bool value, Error **errp)
>>>>>> +{
>>>>>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>>>>>> +
>>>>>> +    vms->acpi_pcihp = value;
>>>>>> +}
>>>>>> +
>>>>>>  static bool virt_get_dtb_randomness(Object *obj, Error **errp)
>>>>>>  {
>>>>>>      VirtMachineState *vms = VIRT_MACHINE(obj);
>>>>>> @@ -3310,6 +3326,10 @@ static void virt_machine_class_init(ObjectClass *oc, const void *data)
>>>>>>                                            "in ACPI table header."
>>>>>>                                            "The string may be up to 8 bytes in size");
>>>>>>  
>>>>>> +    object_class_property_add_bool(oc, "acpi-pcihp",
>>>>>> +                                   virt_get_acpi_pcihp, virt_set_acpi_pcihp);
>>>>>> +    object_class_property_set_description(oc, "acpi-pcihp",
>>>>>> +                                          "Force ACPI PCI hotplug");
>>>>>>  }
>>>>>>  
>>>>>>  static void virt_instance_init(Object *obj)
>>>>>> @@ -3344,6 +3364,9 @@ static void virt_instance_init(Object *obj)
>>>>>>          vms->tcg_its = true;
>>>>>>      }
>>>>>>  
>>>>>> +    /* default disallows ACPI PCI hotplug */
>>>>>> +    vms->acpi_pcihp = false;
>>>>>> +
>>>>>>      /* Default disallows iommu instantiation */
>>>>>>      vms->iommu = VIRT_IOMMU_NONE;
>>>>>>  
>>>>>> @@ -3394,8 +3417,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(10, 1)
>>>>>>  
>>>>>>  static void virt_machine_10_0_options(MachineClass *mc)
>>>>>>  {
>>>>>> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
>>>>>> +
>>>>>>      virt_machine_10_1_options(mc);
>>>>>>      compat_props_add(mc->compat_props, hw_compat_10_0, hw_compat_10_0_len);
>>>>>> +    /* 10.0 and earlier do not support ACPI PCI hotplug */
>>>>>> +    vmc->no_acpi_pcihp = true;
>>>>>>  }
>>>>>>  DEFINE_VIRT_MACHINE(10, 0)
>>>>>>