On Tue, 21 Jun 2016 14:47:24 +0300
Marcel Apfelbaum <mar...@redhat.com> wrote:
> On 06/21/2016 02:28 PM, Igor Mammedov wrote:
> > On Tue, 21 Jun 2016 12:05:12 +0300
> > Marcel Apfelbaum <mar...@redhat.com> wrote:
> >
> >> On 06/20/2016 05:11 PM, Igor Mammedov wrote:
> >>> On Tue, 31 May 2016 20:48:37 +0300
> >>> Marcel Apfelbaum <mar...@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 <mar...@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.
ok,
just make sure patch affects only Q35 and doesn't regress piix4
(where PCI0 always has hotplug AML)
I'd write it a little bit different though:
if (pm->pcihp_bridge_en && has_pci_bus) {
Aml *scope = aml_scope("PCI0");
build_append_pci_bus_devices(scope, root_bus,...
aml_append(sb_scope, scope);
}
if (misc->tpm_version != TPM_VERSION_UNSPEC) {
dev = aml_device("PCI0.ISA.TPM");
...
aml_append(sb_scope, dev);
}
which looks somewhat cleaner than tracking if scope was used.
>
>
> 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));
> >>>
> >>
> >
>
