Re: [Qemu-devel] [PATCH RFC 6/7] hw/acpi: extend acpi pci hotplug support for pci express

[Marcel Apfelbaum <marcel@redhat.com>]

On 06/21/2016 02:28 PM, Igor Mammedov wrote:
> On Tue, 21 Jun 2016 12:05:12 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 06/20/2016 05:11 PM, Igor Mammedov wrote:
>>> On Tue, 31 May 2016 20:48:37 +0300
>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>
>>> subj doesn't match patch, it does opposite i.e. makes sure that
>>> pcie isn't counted in when building acpi hotplug aml
>>>
>>
>> Maybe I wasn't clear enough. Until now q35 had no acpi hotplug code.
>> This patch adds acpi hotplug code for all Q35 PCI buses, but no for
>> pcie buses That means for pci-pci bridge and dmi-pci bridge.
>>
>>
>> Maybe I should rename it to hw/acpi: extend acpi pci hotplug support
>> for Q35 pci buses.
> this looks better to me
>
>>
>>>> Emit acpi code for pci hotplug on all PC machines:
>>>>    - if legacy pci hotpug is enabled (pcihp_bridge_en)
>>>>    - if there is at least one legacy pci bridge in current system
>>>>    - for PCI buses only (skip PCIe buses)
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>> ---
>>>>    hw/i386/acpi-build.c | 49
>>>> ++++++++++++++++++++++++++++++++----------------- 1 file changed,
>>>> 32 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>> index 8ae3e53..cc51f9e 100644
>>>> --- a/hw/i386/acpi-build.c
>>>> +++ b/hw/i386/acpi-build.c
>>>> @@ -250,6 +250,11 @@ static void acpi_get_pci_info(PcPciInfo *info)
>>>>                                                NULL);
>>>>    }
>>>>
>>>> +static void acpi_test_pci_bus(PCIBus *bus, void *opaque)
>>>> +{
>>>> +    *(bool *)opaque |= !pci_bus_is_express(bus);
>>>> +}
>>>> +
>>>>    #define ACPI_PORT_SMI_CMD           0x00b2 /* TODO: this is
>>>> APM_CNT_IOPORT */
>>>>    static void acpi_align_size(GArray *blob, unsigned align)
>>>> @@ -422,7 +427,7 @@ static void *acpi_set_bsel(PCIBus *bus, void
>>>> *opaque) unsigned *bsel_alloc = opaque;
>>>>        unsigned *bus_bsel;
>>>>
>>>> -    if (qbus_is_hotpluggable(BUS(bus))) {
>>>> +    if (qbus_is_hotpluggable(BUS(bus))
>>>> && !pci_bus_is_express(bus)) { bus_bsel = g_malloc(sizeof
>>>> *bus_bsel);
>>>>
>>>>            *bus_bsel = (*bsel_alloc)++;
>>>> @@ -433,15 +438,12 @@ static void *acpi_set_bsel(PCIBus *bus, void
>>>> *opaque) return bsel_alloc;
>>>>    }
>>>>
>>>> -static void acpi_set_pci_info(void)
>>>> +static void acpi_set_pci_info(PCIBus *bus)
>>>>    {
>>>> -    PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
>>>>        unsigned bsel_alloc = 0;
>>>>
>>>> -    if (bus) {
>>>> -        /* Scan all PCI buses. Set property to enable acpi based
>>>> hotplug. */
>>>> -        pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
>>>> &bsel_alloc);
>>>> -    }
>>>> +    /* Scan all PCI buses. Set property to enable acpi based
>>>> hotplug. */
>>>> +    pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
>>>> &bsel_alloc); }
>>>>
>>>>    static void build_append_pcihp_notify_entry(Aml *method, int
>>>> slot) @@ -503,7 +505,7 @@ static void
>>>> build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>>>>             * In this case they aren't themselves hot-pluggable.
>>>>             * Hotplugged bridges *are* hot-pluggable.
>>>>             */
>>>> -        bridge_in_acpi = pc->is_bridge && pcihp_bridge_en &&
>>>> +        bridge_in_acpi = pc->is_bridge && !pc->is_express &&
>>>> pcihp_bridge_en && !DEVICE(pdev)->hotplugged;
>>>>
>>>>            hotplug_enabled_dev = bsel && dc->hotpluggable
>>>> && !bridge_in_acpi; @@ -1892,7 +1894,7 @@ static void
>>>> build_piix4_isa_bridge(Aml *table) aml_append(table, scope);
>>>>    }
>>>>
>>>> -static void build_piix4_pci_hotplug(Aml *table)
>>>> +static void build_acpi_pci_hotplug(Aml *table)
>>>>    {
>>>>        Aml *scope;
>>>>        Aml *field;
>>>> @@ -1995,6 +1997,7 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker, uint32_t nr_mem = machine->ram_slots;
>>>>        int root_bus_limit = 0xFF;
>>>>        PCIBus *root_bus = NULL;
>>>> +    bool has_pci_bus = false;
>>>>        int i;
>>>>
>>>>        dsdt = init_aml_allocator();
>>>> @@ -2016,7 +2019,6 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker, build_piix4_pm(dsdt);
>>>>            build_piix4_isa_bridge(dsdt);
>>>>            build_isa_devices_aml(dsdt);
>>>> -        build_piix4_pci_hotplug(dsdt);
>>>>            build_piix4_pci0_int(dsdt);
>>>>        } else {
>>>>            sb_scope = aml_scope("_SB");
>>>> @@ -2040,6 +2042,11 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker, root_bus = PC_MACHINE(machine)->bus;
>>>>        assert(root_bus);
>>>>
>>>> +    pci_for_each_bus(root_bus, acpi_test_pci_bus, &has_pci_bus);
>>>> +    if (pm->pcihp_bridge_en && has_pci_bus) {
>>>              ^^^^^ wouldn't that forbid acpi hotplug on PCI0 is it's
>>> not set?
>>
>> Yes, and this is by design. 'pcihp_bridge_en' flag matches 'acpi
>> hotplug'. I followed the initialization phase, am I wrong? Can you
>> have acpi hotplug if the bit is off?
> my understanding was that pcihp_bridge_en governs only hotplug
> AML for bridges, while with it disabled hotplug AML would be
> generated only for PCI0.
>
>>>
>>>> +        build_acpi_pci_hotplug(dsdt);
>>>> +    }
>>>> +
>>>>        build_cpu_hotplug_aml(dsdt);
>>>>        build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base,
>>>>                                 pm->mem_hp_io_len);
>>>> @@ -2050,7 +2057,7 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker,
>>>>
>>>>            aml_append(scope, aml_method("_L00", 0,
>>>> AML_NOTSERIALIZED));
>>>>
>>>> -        if (misc->is_piix4) {
>>>> +        if (pm->pcihp_bridge_en && has_pci_bus) {
>>> same question as above
>>>
>>>>                method = aml_method("_E01", 0, AML_NOTSERIALIZED);
>>>>                aml_append(method,
>>>>                    aml_acquire(aml_name("\\_SB.PCI0.BLCK"),
>>>> 0xFFFF)); @@ -2188,7 +2195,7 @@ build_dsdt(GArray *table_data,
>>>> GArray *linker, g_ptr_array_free(mem_ranges, true);
>>>>
>>>>        /* reserve PCIHP resources */
>>>> -    if (pm->pcihp_io_len) {
>>>> +    if (pm->pcihp_bridge_en && has_pci_bus && pm->pcihp_io_len) {
>>> same question as above
>>>
>>>>            dev = aml_device("PHPR");
>>>>            aml_append(dev, aml_name_decl("_HID",
>>>> aml_string("PNP0A06"))); aml_append(dev,
>>>> @@ -2326,10 +2333,14 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker,
>>>>
>>>>            {
>>>>                Aml *scope = aml_scope("PCI0");
>>>> +            bool scope_used = false;
>>> Could you explain why ^^^ is needed?
>>>
>>
>> Sure. If none ACPI code will be generated by the below 'if' statements
>> we will have an empty PCI0 scope. I wanted to get rid of it.
> considering that hotplug AML for PCI0 is always generated, that seems
> useless.
>

Not in Q35. pcie.0 is not hot-pluggable.
Run Q35 without a dmi-pci bridge or pci-bridge and you'll have an empty scope.


Thanks,
Marcel

>>
>>
>> Thanks,
>> Marcel
>>
>>>>
>>>> -            /* Scan all PCI buses. Generate tables to support
>>>> hotplug. */
>>>> -            build_append_pci_bus_devices(scope, root_bus,
>>>> -                                         pm->pcihp_bridge_en);
>>>> +            if (pm->pcihp_bridge_en && has_pci_bus) {
>>>> +                /* Scan all PCI buses. Generate tables to support
>>>> hotplug. */
>>>> +                build_append_pci_bus_devices(scope, root_bus,
>>>> +                                             pm->pcihp_bridge_en);
>>>> +                scope_used = true;
>>>> +            }
>>>>
>>>>                if (misc->tpm_version != TPM_VERSION_UNSPEC) {
>>>>                    dev = aml_device("ISA.TPM");
>>>> @@ -2346,8 +2357,12 @@ build_dsdt(GArray *table_data, GArray
>>>> *linker, /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
>>>>                    aml_append(dev, aml_name_decl("_CRS", crs));
>>>>                    aml_append(scope, dev);
>>>> +                scope_used = true;
>>>> +            }
>>>> +
>>>> +            if (scope_used) {
>>>> +                aml_append(sb_scope, scope);
>>>>                }
>>>> -            aml_append(sb_scope, scope);
>>>>            }
>>>>            aml_append(dsdt, sb_scope);
>>>>        }
>>>> @@ -2870,7 +2885,7 @@ void acpi_setup(void)
>>>>
>>>>        build_state = g_malloc0(sizeof *build_state);
>>>>
>>>> -    acpi_set_pci_info();
>>>> +    acpi_set_pci_info(pcms->bus);
>>>>
>>>>        acpi_build_tables_init(&tables);
>>>>        acpi_build(&tables, MACHINE(pcms));
>>>
>>
>