Re: [PATCH 4/5] hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC

[Michael S. Tsirkin]

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 <jusual@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...


> @@ -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));
>              } else {
>                  aml_append(dev, aml_name_decl("_HID", 
> aml_eisaid("PNP0A03")));
>              }
> -- 
> 2.31.1