On Wed, 11 Jun 2025 08:47:36 +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.
> >
> >> 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
> What I fail to understand is whether you want me to attach this property
> to the GPEX host bridge device or to the GED device. Comment on patch
I'd say GED.
> 6/25 seems to indicate you expect it to be attached to the GPEX. I ask
> here because also the GED device will need to be configured depending on
> the hp setting. Maybe we can retrieve the info from the gpex at that
> time. on x86 it is attached to piix4 or ich9 I/O controller hub which do
> not have direct equivalent on ARM.
for ARM, equivalent would be GED device which hosts our paravirt acpi registers.
>
> 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)
> >>
>
