On Wed, 10 Nov 2021 02:21:34 -0500 "Michael S. Tsirkin" <m...@redhat.com> wrote:
> On Wed, Nov 10, 2021 at 06:30:13AM +0100, Julia Suvorova wrote: > > There are two ways to enable ACPI PCI Hot-plug: > > > > * Disable the Hot-plug Capable bit on PCIe slots. > > > > This was the first approach which led to regression [1-2], as > > I/O space for a port is allocated only when it is hot-pluggable, > > which is determined by HPC bit. > > > > * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC > > method. > > > > This removes the (future) ability of hot-plugging switches with PCIe > > Native hotplug since ACPI PCI Hot-plug only works with cold-plugged > > bridges. If the user wants to explicitely use this feature, they can > > disable ACPI PCI Hot-plug with: > > --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off > > > > Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug > > instead of PCIe Native. > > > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641 > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 > > > > Signed-off-by: Julia Suvorova <jus...@redhat.com> > > --- > > hw/i386/acpi-build.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index a3ad6abd33..a2f92ab32b 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -1337,7 +1337,7 @@ static void build_x86_acpi_pci_hotplug(Aml *table, > > uint64_t pcihp_addr) > > aml_append(table, scope); > > } > > > > -static Aml *build_q35_osc_method(void) > > +static Aml *build_q35_osc_method(bool acpi_pcihp) > > { > > Aml *if_ctx; > > Aml *if_ctx2; > > @@ -1345,6 +1345,7 @@ static Aml *build_q35_osc_method(void) > > Aml *method; > > Aml *a_cwd1 = aml_name("CDW1"); > > Aml *a_ctrl = aml_local(0); > > + const uint64_t hotplug = acpi_pcihp ? 0x1E : 0x1F; > > drop this variable and open-code at use point below. > As it is the comment is misplaced. > Also a slightly better way to write this: > 0x1E | (acpi_pcihp ? 0x0 : 0x1) > > > > > > > method = aml_method("_OSC", 4, AML_NOTSERIALIZED); > > aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), > > "CDW1")); > > So what acpi_pcihp does is enable/disable native pcie hotplug. > How about we call the option exactly that? > > > > @@ -1359,8 +1360,9 @@ static Aml *build_q35_osc_method(void) > > /* > > * Always allow native PME, AER (no dependencies) > > * Allow SHPC (PCI bridges can have SHPC controller) > > + * Disable PCIe Native Hot-plug if ACPI PCI Hot-plug is enabled. > > */ > > - aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl)); > > + aml_append(if_ctx, aml_and(a_ctrl, aml_int(hotplug), a_ctrl)); > > > > using the variable hotplug just made things confusing, > open-coding will be clearer. > > > > if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1)))); > > /* Unknown revision */ > > @@ -1449,7 +1451,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03"))); > > aml_append(dev, aml_name_decl("_ADR", aml_int(0))); > > aml_append(dev, aml_name_decl("_UID", > > aml_int(pcmc->pci_root_uid))); > > - aml_append(dev, build_q35_osc_method()); > > + aml_append(dev, build_q35_osc_method(pm->pcihp_bridge_en)); > > aml_append(sb_scope, dev); > > if (mcfg_valid) { > > aml_append(sb_scope, build_q35_dram_controller(&mcfg)); > > One of the complaints of libvirt people was that the > name is confusing. It seems to say whether to describe bridges > in ACPI but it also disables native hotplug, but only for PCIe. > > Maybe we should address this with flags saying whether to enable each of > acpi,pcie,shpc hotplug separately... yep, mask with field defines would be better, but I'd hold off such change to post 6.2 time. > > > @@ -1565,7 +1567,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > if (pci_bus_is_express(bus)) { > > aml_append(dev, aml_name_decl("_HID", > > aml_eisaid("PNP0A08"))); > > aml_append(dev, aml_name_decl("_CID", > > aml_eisaid("PNP0A03"))); > > - aml_append(dev, build_q35_osc_method()); > > + /* Expander bridges do not have ACPI PCI Hot-plug enabled > > */ > > + aml_append(dev, build_q35_osc_method(false)); this's it not obvious, why PXBs aren't capable of ACPI PCI Hot-plug? > > } else { > > aml_append(dev, aml_name_decl("_HID", > > aml_eisaid("PNP0A03"))); > > } > > -- > > 2.31.1 >
