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