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

[Igor Mammedov]

On Tue, 14 Jul 2020 04:39:53 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 13, 2020 at 04:56:54PM +0200, Igor Mammedov wrote:
> > 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.  
> 
> Do we need that later part btw?
> 
> > > 
> > > 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  
> 
> I don't think you have to add a reference as part of this patchset.
> The spec in question documents _OSC so it's not a bad idea to
> add it, but it's a bit more work, e.g. we generally try to list
> the earliest spec that documents the given feature, since
> So I suspect this is 3.0 actually.
> 
> 
> > seems incomplete, were you going to point to a concrete chapter that 
> > requires this change?  
> 
> 
> It doesn't as such. A better description would be:
> 
> / * Guests seem to generally prefer native hotplug control. As we want them to
>   * use ACPI, don't enable it.
>   */
> 
> 
> 
> > >       */
> > > -    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)
> >   
> 
> ... if there is such a macro. If not I suspect it's better as a comment:
> 
>       0x10 /* PCI Express Capability Structure control */ |
>       0x80 /* PCI Express Advanced Error Reporting control */ |
>       0x40 /* PCI Express Native Power Management Events control */

if that is a spec quoted nearby than, I like comments idea as it
follows the same style which we use with ACPI numbers.
We just need split single magic number onto separate features bits
so it would be readable.

> 
> 
> 
> > > +    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")));
> > >              }  
> 
>