Hi Gustavo,
On 6/13/25 5:01 AM, Gustavo Romero wrote:
> Hi Eric,
>
> On 6/12/25 09:55, Igor Mammedov wrote:
>> On Wed, 11 Jun 2025 10:50:04 +0200
>> Eric Auger <eric.au...@redhat.com> wrote:
>>
>>> Hi Igor,
>>> On 6/11/25 10:45 AM, Igor Mammedov wrote:
>>>> On Wed, 11 Jun 2025 08:53:28 +0200
>>>> Eric Auger <eric.au...@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.au...@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.au...@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 ! ;-)
>>
>> So far what I suggest looks like better option compared to multiple
>> machine knobs
>> fiddling. But I can easily change my mind once I see respin, if
>> experiment
>> with compat props is not coming well together.
>
> For now, I think it's okay to let ACPI PCI hotplug stabilize (while
> not being the
> default) for at least one release cycle. So I'm fine with keeping
> acpi-pcihp=off as
> the default.
>
> As I mentioned elsewhere, I don't consider native PCIe hotplug to be
> legacy.
>
> We can make acpi-pcihp=on the default in a future release once it's
> been more
> widely exercised.
>
> I'll update the bios-tables-test.c test accordingly, then you can
> either put them
> in the v3 (if you happen to send v3 next week) or add them to a v4.
OK thank you for the confirmation. So following Igor's suggestion I
indeed kept the current default value (legacy PCIe hotplug) and I don't
use a machine option anymore. Instead I use the x86 trick, ie.
-global acpi-ged.acpi-pci-hotplug-with-bridge-support=on
I can easily update your tests with that option, don't bother
respinning. I should be able to send the v3 by beginning of next week.
Thanks!
Eric
>
>
> Cheers,
> Gustavo
>
>>> 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.au...@redhat.com>
>>>>>>>>> Reviewed-by: Gustavo Romero <gustavo.rom...@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)
>>>>>>>>>
>>>
>>
>
