| Message ID | 20200708224615.114077-4-jusual@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use ACPI PCI hot-plug for q35 | expand |
On Thu, 9 Jul 2020 00:46:13 +0200 Julia Suvorova <jusual@redhat.com> wrote: > Implement notifications and gpe to support q35 ACPI PCI hot-plug. > The addresses specified in [1] remain the same to make fewer changes. > > [1] docs/spec/acpi_pci_hotplug.txt CCing Gerd his opinion on reusing piix4 IO port range for q35 > Signed-off-by: Julia Suvorova <jusual@redhat.com> > --- > hw/i386/acpi-build.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 11c598f955..5c5ad88ad6 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm) > /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */ > pm->fadt.rev = 1; > pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE; > - pm->pcihp_io_base = > - object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL); > - pm->pcihp_io_len = > - object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL); > } > if (lpc) { > struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO, > @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm) > pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP; > pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE; > } > + pm->pcihp_io_base = > + object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL); > + pm->pcihp_io_len = > + object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL); > > /* The above need not be conditional on machine type because the reset port > * happens to be the same on PIIX (pc) and ICH9 (q35). */ > @@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > QLIST_FOREACH(sec, &bus->child, sibling) { > int32_t devfn = sec->parent_dev->devfn; > > - if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) { > + if (pci_bus_is_root(sec)) { > continue; > } > > @@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table) > aml_append(table, scope); > } > > -static Aml *build_q35_osc_method(void) > +static void build_q35_pci_hotplug(Aml *table) > +{ > + build_piix4_pci_hotplug(table); > +} s/build_piix4_pci_hotplug/build_i386_acpi_pci_hotplug/ and reuse it in both cases, instead of adding wrapper? > + > +static Aml *build_q35_osc_method(AcpiPmInfo *pm) > { > Aml *if_ctx; > Aml *if_ctx2; > @@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > build_hpet_aml(dsdt); > build_q35_isa_bridge(dsdt); > build_isa_devices_aml(dsdt); > + build_q35_pci_hotplug(dsdt); > build_q35_pci0_int(dsdt); > if (pcms->smbus && !pcmc->do_not_add_smb_acpi) { > build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC); > @@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > { > aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006"))); > > - if (misc->is_piix4) { > + if (misc->is_piix4 || pm->pcihp_bridge_en) { > method = aml_method("_E01", 0, AML_NOTSERIALIZED); > aml_append(method, > aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote: > On Thu, 9 Jul 2020 00:46:13 +0200 > Julia Suvorova <jusual@redhat.com> wrote: > > > Implement notifications and gpe to support q35 ACPI PCI hot-plug. > > The addresses specified in [1] remain the same to make fewer changes. > > > > [1] docs/spec/acpi_pci_hotplug.txt > > CCing Gerd his opinion on reusing piix4 IO port range for q35 > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com> > > --- > > hw/i386/acpi-build.c | 20 +++++++++++++------- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 11c598f955..5c5ad88ad6 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm) > > /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */ > > pm->fadt.rev = 1; > > pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE; > > - pm->pcihp_io_base = > > - object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL); > > - pm->pcihp_io_len = > > - object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL); > > } > > if (lpc) { > > struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO, > > @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm) > > pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP; > > pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE; > > } > > + pm->pcihp_io_base = > > + object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL); > > + pm->pcihp_io_len = > > + object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL); > > > > /* The above need not be conditional on machine type because the reset port > > * happens to be the same on PIIX (pc) and ICH9 (q35). */ > > @@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > QLIST_FOREACH(sec, &bus->child, sibling) { > > int32_t devfn = sec->parent_dev->devfn; > > > > - if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) { > > + if (pci_bus_is_root(sec)) { > > continue; > > } > > > > @@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table) > > aml_append(table, scope); > > } > > > > -static Aml *build_q35_osc_method(void) > > +static void build_q35_pci_hotplug(Aml *table) > > +{ > > + build_piix4_pci_hotplug(table); > > +} > > s/build_piix4_pci_hotplug/build_i386_acpi_pci_hotplug/ > > and reuse it in both cases, instead of adding wrapper? I'm not sure about that - we have microvm too ... > > + > > +static Aml *build_q35_osc_method(AcpiPmInfo *pm) > > { > > Aml *if_ctx; > > Aml *if_ctx2; > > @@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > build_hpet_aml(dsdt); > > build_q35_isa_bridge(dsdt); > > build_isa_devices_aml(dsdt); > > + build_q35_pci_hotplug(dsdt); > > build_q35_pci0_int(dsdt); > > if (pcms->smbus && !pcmc->do_not_add_smb_acpi) { > > build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC); > > @@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > { > > aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006"))); > > > > - if (misc->is_piix4) { > > + if (misc->is_piix4 || pm->pcihp_bridge_en) { > > method = aml_method("_E01", 0, AML_NOTSERIALIZED); > > aml_append(method, > > aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
On Tue, 14 Jul 2020 05:22:20 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote: > > On Thu, 9 Jul 2020 00:46:13 +0200 > > Julia Suvorova <jusual@redhat.com> wrote: > > > > > Implement notifications and gpe to support q35 ACPI PCI hot-plug. > > > The addresses specified in [1] remain the same to make fewer changes. > > > > > > [1] docs/spec/acpi_pci_hotplug.txt > > > > CCing Gerd his opinion on reusing piix4 IO port range for q35 > > > > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com> > > > --- > > > hw/i386/acpi-build.c | 20 +++++++++++++------- > > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > index 11c598f955..5c5ad88ad6 100644 > > > --- a/hw/i386/acpi-build.c > > > +++ b/hw/i386/acpi-build.c > > > @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm) > > > /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */ > > > pm->fadt.rev = 1; > > > pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE; > > > - pm->pcihp_io_base = > > > - object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL); > > > - pm->pcihp_io_len = > > > - object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL); > > > } > > > if (lpc) { > > > struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO, > > > @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm) > > > pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP; > > > pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE; > > > } > > > + pm->pcihp_io_base = > > > + object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL); > > > + pm->pcihp_io_len = > > > + object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL); > > > > > > /* The above need not be conditional on machine type because the reset port > > > * happens to be the same on PIIX (pc) and ICH9 (q35). */ > > > @@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > > QLIST_FOREACH(sec, &bus->child, sibling) { > > > int32_t devfn = sec->parent_dev->devfn; > > > > > > - if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) { > > > + if (pci_bus_is_root(sec)) { > > > continue; > > > } > > > > > > @@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table) > > > aml_append(table, scope); > > > } > > > > > > -static Aml *build_q35_osc_method(void) > > > +static void build_q35_pci_hotplug(Aml *table) > > > +{ > > > + build_piix4_pci_hotplug(table); > > > +} > > > > s/build_piix4_pci_hotplug/build_i386_acpi_pci_hotplug/ > > > > and reuse it in both cases, instead of adding wrapper? > > I'm not sure about that - we have microvm too ... it doesn't have pci if I'm not mistaken, and when/if it gains one someday, it may or maynot use hotplug it will be up to its specific DSDT to include this call. What doesn't make sense is using board specific naming here for the same code. I don't insist on the variant I've suggested, only on consolidating it instead of adding dummy wrapper > > > + > > > +static Aml *build_q35_osc_method(AcpiPmInfo *pm) > > > { > > > Aml *if_ctx; > > > Aml *if_ctx2; > > > @@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > > build_hpet_aml(dsdt); > > > build_q35_isa_bridge(dsdt); > > > build_isa_devices_aml(dsdt); > > > + build_q35_pci_hotplug(dsdt); > > > build_q35_pci0_int(dsdt); > > > if (pcms->smbus && !pcmc->do_not_add_smb_acpi) { > > > build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC); > > > @@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > > { > > > aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006"))); > > > > > > - if (misc->is_piix4) { > > > + if (misc->is_piix4 || pm->pcihp_bridge_en) { > > > method = aml_method("_E01", 0, AML_NOTSERIALIZED); > > > aml_append(method, > > > aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF)); >
On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote: > On Thu, 9 Jul 2020 00:46:13 +0200 > Julia Suvorova <jusual@redhat.com> wrote: > > > Implement notifications and gpe to support q35 ACPI PCI hot-plug. > > The addresses specified in [1] remain the same to make fewer changes. > > > > [1] docs/spec/acpi_pci_hotplug.txt > > CCing Gerd his opinion on reusing piix4 IO port range for q35 Oh no, please don't. Grabbing random io ports is asking for trouble, especially on q35 where you only need enough pcie devices to have the port range 0xae00 -> 0xae0f actually overlap with a pci bridge window. Ideally we'll find some unused spot in the hidden pci bar of the ich9 pm device. At least the qemu implementation has some room: 0000000000000600-000000000000067f (prio 0, i/o): ich9-pm 0000000000000600-0000000000000603 (prio 0, i/o): acpi-evt 0000000000000604-0000000000000605 (prio 0, i/o): acpi-cnt 0000000000000608-000000000000060b (prio 0, i/o): acpi-tmr 0000000000000620-000000000000062f (prio 0, i/o): acpi-gpe0 0000000000000630-0000000000000637 (prio 0, i/o): acpi-smi 0000000000000660-000000000000067f (prio 0, i/o): sm-tco Offset 0x40 seems to be unused for example (but better check the chipset specs to see if that is really unused or whenever the qemu emulation is incomplete). If that doesn't work out pick an io range which is unlikely to conflict with something. That excludes anything above > 0x1000 (pci bridge windows) and anything below 0x3ff (legacy isa). From the remaining range 0x400 -> 0xfff the 0xc00 -> 0xcff block is the best candidate I think. Because of the pci config registers being there it is unlikely that the firmware places something in that range. Oh, I see the cpu hotplug registers are already there: [ ... ] 0000000000000700-000000000000073f (prio 1, i/o): pm-smbus 0000000000000cd8-0000000000000ce3 (prio 0, i/o): acpi-cpu-hotplug 0000000000000cf8-0000000000000cfb (prio 0, i/o): pci-conf-idx 0000000000000cf9-0000000000000cf9 (prio 1, i/o): lpc-reset-control 0000000000000cfc-0000000000000cff (prio 0, i/o): pci-conf-data [ ... ] So placing the pci hotplug registers next to them (say at 0xcc8) looks like a good pick to me. While being at it: Shouldn't we reserve these port ranges somehow? Using an acpi device for example, simliar to fw_cfg? The guest OS should better know there is something at those ports ... take care, Gerd
On Wed, 15 Jul 2020 08:57:51 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote: > > On Thu, 9 Jul 2020 00:46:13 +0200 > > Julia Suvorova <jusual@redhat.com> wrote: > > > > > Implement notifications and gpe to support q35 ACPI PCI hot-plug. > > > The addresses specified in [1] remain the same to make fewer changes. > > > > > > [1] docs/spec/acpi_pci_hotplug.txt > > > > CCing Gerd his opinion on reusing piix4 IO port range for q35 [...] > While being at it: Shouldn't we reserve these port ranges somehow? > Using an acpi device for example, simliar to fw_cfg? The guest OS > should better know there is something at those ports ... we do it at ACPI level in DSDT, look for comment /* reserve PCIHP resources */ It should make Windows trip over in case of another range overlap with reserved ports. (linux kernel is more tolerant and may silently ignore or print a warning) > take care, > Gerd >
> > While being at it: Shouldn't we reserve these port ranges somehow? > > Using an acpi device for example, simliar to fw_cfg? The guest OS > > should better know there is something at those ports ... > > we do it at ACPI level in DSDT, look for comment > /* reserve PCIHP resources */ Ah, good. > It should make Windows trip over in case of another range overlap with > reserved ports. (linux kernel is more tolerant and may silently ignore > or print a warning) Hmm, linux doesn't list the device, can't see it neither in the kernel log nor in /proc/ioports ... take care, Gerd
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 11c598f955..5c5ad88ad6 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm) /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */ pm->fadt.rev = 1; pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE; - pm->pcihp_io_base = - object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL); - pm->pcihp_io_len = - object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL); } if (lpc) { struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO, @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm) pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP; pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE; } + pm->pcihp_io_base = + object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL); + pm->pcihp_io_len = + object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL); /* The above need not be conditional on machine type because the reset port * happens to be the same on PIIX (pc) and ICH9 (q35). */ @@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, QLIST_FOREACH(sec, &bus->child, sibling) { int32_t devfn = sec->parent_dev->devfn; - if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) { + if (pci_bus_is_root(sec)) { continue; } @@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table) aml_append(table, scope); } -static Aml *build_q35_osc_method(void) +static void build_q35_pci_hotplug(Aml *table) +{ + build_piix4_pci_hotplug(table); +} + +static Aml *build_q35_osc_method(AcpiPmInfo *pm) { Aml *if_ctx; Aml *if_ctx2; @@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, build_hpet_aml(dsdt); build_q35_isa_bridge(dsdt); build_isa_devices_aml(dsdt); + build_q35_pci_hotplug(dsdt); build_q35_pci0_int(dsdt); if (pcms->smbus && !pcmc->do_not_add_smb_acpi) { build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC); @@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, { aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006"))); - if (misc->is_piix4) { + if (misc->is_piix4 || pm->pcihp_bridge_en) { method = aml_method("_E01", 0, AML_NOTSERIALIZED); aml_append(method, aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
Implement notifications and gpe to support q35 ACPI PCI hot-plug. The addresses specified in [1] remain the same to make fewer changes. [1] docs/spec/acpi_pci_hotplug.txt Signed-off-by: Julia Suvorova <jusual@redhat.com> --- hw/i386/acpi-build.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)