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