Re: [RFC PATCH 4/5] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC

[Igor Mammedov]

On Thu,  9 Jul 2020 00:46:14 +0200
Julia Suvorova <jusual@redhat.com> wrote:

> Other methods may be used if the system is capable of this and the _OSC bit
> is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
> versions will still use PCIe native.
> 
> 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 5c5ad88ad6..0e2891c3ea 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1599,6 +1599,7 @@ static Aml *build_q35_osc_method(AcpiPmInfo *pm)
>      Aml *method;
>      Aml *a_cwd1 = aml_name("CDW1");
>      Aml *a_ctrl = aml_local(0);
> +    unsigned osc_ctrl;
>  
>      method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
>      aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), 
> "CDW1"));
> @@ -1612,9 +1613,12 @@ static Aml *build_q35_osc_method(AcpiPmInfo *pm)
>  
>      /*
>       * Always allow native PME, AER (no dependencies)
> -     * Allow SHPC (PCI bridges can have SHPC controller)
> +     * Need to disable native and SHPC hot-plug to use acpihp
> +     *
> +     * PCI Firmware Specification, Revision 3.2
seems incomplete, were you going to point to a concrete chapter that requires 
this change?

>       */
> -    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> +    osc_ctrl = pm->pcihp_bridge_en ? 0x1C : 0x1F;
Since you are touching this, how about converting this magic number to
something more readable?
i.e.
            set_bit(ACPI_OSC_SHPC_EN,  osc_ctrl)
          or
            osc_ctrl |= BIT(SOME_FEATURE)

> +    aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
>  
>      if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
>      /* Unknown revision */
> @@ -1696,7 +1700,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(1)));
> -        aml_append(dev, build_q35_osc_method());
> +        aml_append(dev, build_q35_osc_method(pm));
>          aml_append(sb_scope, dev);
>          aml_append(dsdt, sb_scope);
>  
> @@ -1771,7 +1775,7 @@ 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());
> +                aml_append(dev, build_q35_osc_method(pm));
>              } else {
>                  aml_append(dev, aml_name_decl("_HID", 
> aml_eisaid("PNP0A03")));
>              }