On Mon, Sep 14, 2020 at 9:11 PM Ani Sinha <a...@anisinha.ca> wrote:
>
> On Mon, Sep 14, 2020 at 8:51 PM Ani Sinha <a...@anisinha.ca> wrote:
> >
> > On Mon, Sep 14, 2020 at 8:20 PM Ani Sinha <a...@anisinha.ca> wrote:
> > >
> > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imamm...@redhat.com> wrote:
> > > >
> > > > On Mon, 14 Sep 2020 18:58:19 +0530
> > > > Ani Sinha <a...@anisinha.ca> wrote:
> > > >
> > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <a...@anisinha.ca> wrote:
> > > > > >
> > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imamm...@redhat.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530
> > > > > > > Ani Sinha <a...@anisinha.ca> wrote:
> > > > > > >
> > > > > > > > When acpi hotplug is turned off for both root pci bus as well
> > > > > > > > as for pci
> > > > > > > > bridges, we should not generate the related amls for DSDT table
> > > > > > > > or initialize
> > > > > > > > related hw ports or reserve hw resources. This change makes
> > > > > > > > sure all those
> > > > > > > > operations are turned off in the case acpi pci hotplug is off
> > > > > > > > globally.
> > > > > > >
> > > > > > > it still leaves around pure PCI hotplug ACPI code:
> > > > > > >
> > > > > > > Method (PCNT, 0, NotSerialized)
> > > > > > > {
> > > > > > > }
> > > > > >
> > > > > > How do you suggest we fix this?
> > > > >
> > > > > One way to fix this would be to do this:
> > > > >
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index 7e3cf3b57b..2bd0c37143 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -483,7 +483,8 @@ static void build_append_pci_bus_devices(Aml
> > > > > *parent_scope, PCIBus *bus,
> > > > > aml_append(method, aml_name("^S%.02X.PCNT", devfn));
> > > > > }
> > > > > }
> > > > > - aml_append(parent_scope, method);
> > > > > + if (bsel)
> > > > > + aml_append(parent_scope, method);
> > > > > qobject_unref(bsel);
> > > > > }
> > > > >
> > > > > This means that if the hotplug on the root bus is disabled, we also
> > > > > disable PCNT. This will also need the unit test update as the unit
> > > > Does bridge hot-plug still work if we disable it only on root bus?
> > >
> > > Just did a quick test. Windows seems to eject devices on the bridge
> > > even when the root bus hot plug is off.
> >
> > OK I am really confused! With the above change, the unit tests are
> > failing because of differences in tests/data/acpi/q35/DSDT !! Why is
> > q35 affected?
>
> Following is the list of golden master tables which need to be
> refreshed in order to make the change:
>
> > > > > - aml_append(parent_scope, method);
> > > > > + if (bsel)
> > > > > + aml_append(parent_scope, method);
> > > > > qobject_unref(bsel);
>
> "tests/data/acpi/pc/DSDT.roothp",
> "tests/data/acpi/q35/DSDT",
> "tests/data/acpi/q35/DSDT.tis",
> "tests/data/acpi/q35/DSDT.bridge",
> "tests/data/acpi/q35/DSDT.mmio64",
> "tests/data/acpi/q35/DSDT.ipmibt",
> "tests/data/acpi/q35/DSDT.cphp",
> "tests/data/acpi/q35/DSDT.memhp",
> "tests/data/acpi/q35/DSDT.numamem",
> "tests/data/acpi/q35/DSDT.dimmpxm",
> "tests/data/acpi/q35/DSDT.acpihmat",
>
> I am not sure if this is the right thing to do. The diff in every case is:
>
> @@ -3413,24 +3413,20 @@
> Name (_ADR, 0x00010000) // _ADR: Address
> Method (_S1D, 0, NotSerialized) // _S1D: S1 Device State
> {
> Return (Zero)
> }
>
> Method (_S2D, 0, NotSerialized) // _S2D: S2 Device State
> {
> Return (Zero)
> }
>
> Method (_S3D, 0, NotSerialized) // _S3D: S3 Device State
> {
> Return (Zero)
> }
> }
> -
> - Method (PCNT, 0, NotSerialized)
> - {
> - }
> }
> }
> }
>
> @Igor Mammedov Please advice.
I could be wrong but it seems bsel is not set for pci buses for q35.
>
> >
> > >
> > > >
> > > > > test
> > > > > will fail with :
> > > > >
> > > > > @@ -3113,24 +3113,20 @@
> > > > > Name (_ADR, 0x00010000) // _ADR: Address
> > > > > Method (_S1D, 0, NotSerialized) // _S1D: S1 Device
> > > > > State
> > > > > {
> > > > > Return (Zero)
> > > > > }
> > > > >
> > > > > Method (_S2D, 0, NotSerialized) // _S2D: S2 Device
> > > > > State
> > > > > {
> > > > > Return (Zero)
> > > > > }
> > > > >
> > > > > Method (_S3D, 0, NotSerialized) // _S3D: S3 Device
> > > > > State
> > > > > {
> > > > > Return (Zero)
> > > > > }
> > > > > }
> > > > > -
> > > > > - Method (PCNT, 0, NotSerialized)
> > > > > - {
> > > > > - }
> > > > > }
> > > > > }
> > > > > }
> > > > >
> > > > > Let's fix this as a separate patch.
> > > >
> > > > I'd rather fix up this patch, so it would do what it claims.
> > > >
> > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Ani Sinha <a...@anisinha.ca>
> > > > > > > > ---
> > > > > > > > hw/acpi/piix4.c | 6 ++++--
> > > > > > > > hw/i386/acpi-build.c | 10 ++++++++--
> > > > > > > > 2 files changed, 12 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > > > > > index e6163bb6ce..b70b1f98af 100644
> > > > > > > > --- a/hw/acpi/piix4.c
> > > > > > > > +++ b/hw/acpi/piix4.c
> > > > > > > > @@ -596,8 +596,10 @@ static void
> > > > > > > > piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> > > > > > > > "acpi-gpe0", GPE_LEN);
> > > > > > > > memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> > > > > > > >
> > > > > > > > - acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus,
> > > > > > > > parent,
> > > > > > > > - s->use_acpi_hotplug_bridge);
> > > > > > > > + if (s->use_acpi_hotplug_bridge ||
> > > > > > > > s->use_acpi_root_pci_hotplug) {
> > > > > > > > + acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus,
> > > > > > > > parent,
> > > > > > > > + s->use_acpi_hotplug_bridge);
> > > > > > > > + }
> > > > > > > >
> > > > > > > > s->cpu_hotplug_legacy = true;
> > > > > > > > object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > > > index e079b686f5..7e3cf3b57b 100644
> > > > > > > > --- a/hw/i386/acpi-build.c
> > > > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > > > @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
> > > > > > > > bool s3_disabled;
> > > > > > > > bool s4_disabled;
> > > > > > > > bool pcihp_bridge_en;
> > > > > > > > + bool pcihp_root_en;
> > > > > > > > uint8_t s4_val;
> > > > > > > > AcpiFadtData fadt;
> > > > > > > > uint16_t cpu_hp_io_base;
> > > > > > > > @@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState
> > > > > > > > *machine, AcpiPmInfo *pm)
> > > > > > > > pm->pcihp_bridge_en =
> > > > > > > > object_property_get_bool(obj,
> > > > > > > > "acpi-pci-hotplug-with-bridge-support",
> > > > > > > > NULL);
> > > > > > > > + pm->pcihp_root_en =
> > > > > > > > + object_property_get_bool(obj, "acpi-root-pci-hotplug",
> > > > > > > > + NULL);
> > > > > > > > }
> > > > > > > >
> > > > > > > > static void acpi_get_misc_info(AcpiMiscInfo *info)
> > > > > > > > @@ -1504,7 +1508,9 @@ build_dsdt(GArray *table_data, BIOSLinker
> > > > > > > > *linker,
> > > > > > > > build_hpet_aml(dsdt);
> > > > > > > > build_piix4_isa_bridge(dsdt);
> > > > > > > > build_isa_devices_aml(dsdt);
> > > > > > > > - build_piix4_pci_hotplug(dsdt);
> > > > > > > > + if (pm->pcihp_bridge_en || pm->pcihp_root_en) {
> > > > > > > > + build_piix4_pci_hotplug(dsdt);
> > > > > > > > + }
> > > > > > > > build_piix4_pci0_int(dsdt);
> > > > > > > > } else {
> > > > > > > > sb_scope = aml_scope("_SB");
> > > > > > > > @@ -1698,7 +1704,7 @@ build_dsdt(GArray *table_data, BIOSLinker
> > > > > > > > *linker,
> > > > > > > > crs_range_set_free(&crs_range_set);
> > > > > > > >
> > > > > > > > /* reserve PCIHP resources */
> > > > > > > > - if (pm->pcihp_io_len) {
> > > > > > > > + if (pm->pcihp_io_len && (pm->pcihp_bridge_en ||
> > > > > > > > pm->pcihp_root_en)) {
> > > > > > > > dev = aml_device("PHPR");
> > > > > > > > aml_append(dev, aml_name_decl("_HID",
> > > > > > > > aml_string("PNP0A06")));
> > > > > > > > aml_append(dev,
> > > > > > >
> > > > >
> > > >
