Hi Igor,
On 6/12/25 3:00 PM, Igor Mammedov wrote:
> On Wed, 11 Jun 2025 10:56:36 +0200
> Eric Auger <eric.au...@redhat.com> wrote:
>
>> On 6/11/25 10:49 AM, Igor Mammedov wrote:
>>> 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.
>> OK thank you for the clarification. I will add a property to the GED device.
>>
>> As the GED device is directly instantiated by the virt machine code (not
>> exposed to the end user CLI) I guess we still want a virt machine option
>> that would allow to set the property explicitly from CLI?
> Instead of machine option,
> I'd go pc/q35 way (aka -global ged.acpi-pci-hotplug-with-bridges=on/off).
> it's not as pretty as machine specific option but that would work just as well
> and as bonus could be used for microvm in the same form.
Ah OK. Then I need to rework it.
Thanks!
Eric
>
>> Thanks
>>
>> Eric
>>>
>>>> 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)
>>>>>>
