| Message ID | 20200911180755.28409-9-ani@anisinha.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] tests/acpi: document addition of table DSDT.roothp for unit testing root pci hotplug on/off | expand |
On Fri, 11 Sep 2020 23:37:55 +0530 Ani Sinha <ani@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) { } > > Signed-off-by: Ani Sinha <ani@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,
On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > On Fri, 11 Sep 2020 23:37:55 +0530 > Ani Sinha <ani@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? > > > > Signed-off-by: Ani Sinha <ani@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, >
On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > Ani Sinha <ani@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 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. > > > > > > > Signed-off-by: Ani Sinha <ani@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, > >
On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > On Fri, 11 Sep 2020 23:37:55 +0530 > Ani Sinha <ani@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 about this comment? " Add unconditionally for root since DSDT expects it." Can this method then be removed when bsel is absent for the root bus? > > > > Signed-off-by: Ani Sinha <ani@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, >
On Mon, 14 Sep 2020 18:58:19 +0530 Ani Sinha <ani@anisinha.ca> wrote: > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > Ani Sinha <ani@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? > 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 <ani@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, > > > >
On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > On Mon, 14 Sep 2020 18:58:19 +0530 > Ani Sinha <ani@anisinha.ca> wrote: > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > Ani Sinha <ani@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. > > > 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 <ani@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, > > > > > > >
On Mon, Sep 14, 2020 at 8:20 PM Ani Sinha <ani@anisinha.ca> wrote: > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > On Mon, 14 Sep 2020 18:58:19 +0530 > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > > Ani Sinha <ani@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? > > > > > > 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 <ani@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, > > > > > > > > > >
On Mon, Sep 14, 2020 at 8:51 PM Ani Sinha <ani@anisinha.ca> wrote: > > On Mon, Sep 14, 2020 at 8:20 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > On Mon, 14 Sep 2020 18:58:19 +0530 > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > > > Ani Sinha <ani@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. > > > > > > > > > > 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 <ani@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, > > > > > > > > > > > > >
On Mon, Sep 14, 2020 at 9:11 PM Ani Sinha <ani@anisinha.ca> wrote: > > On Mon, Sep 14, 2020 at 8:51 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > On Mon, Sep 14, 2020 at 8:20 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > On Mon, 14 Sep 2020 18:58:19 +0530 > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > > > > Ani Sinha <ani@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 <ani@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, > > > > > > > > > > > > > > > >
On Mon, 14 Sep 2020 20:20:21 +0530 Ani Sinha <ani@anisinha.ca> wrote: > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > On Mon, 14 Sep 2020 18:58:19 +0530 > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > > Ani Sinha <ani@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); and this also introduces mem leak > > > 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. well I'm not sure it works using ACPI hotplug, with -global PIIX4_PM.acpi-root-pci-hotplug=off -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on DSDT ins't what I'd expect when bridge hp is on: Scope (\_SB) { Scope (PCI0) { Device (S00) { Name (_ADR, Zero) // _ADR: Address } Device (S08) { 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) } } } } > > > > > > 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 <ani@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, > > > > > > > > > > >
On Tue, Sep 15, 2020 at 5:21 PM Igor Mammedov <imammedo@redhat.com> wrote: > > On Mon, 14 Sep 2020 20:20:21 +0530 > Ani Sinha <ani@anisinha.ca> wrote: > > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > On Mon, 14 Sep 2020 18:58:19 +0530 > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > > > Ani Sinha <ani@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); > > and this also introduces mem leak > > > > > > 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. > > well I'm not sure it works using ACPI hotplug, > > with > -global PIIX4_PM.acpi-root-pci-hotplug=off > -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on > > DSDT ins't what I'd expect when bridge hp is on: > > Scope (\_SB) > { > Scope (PCI0) > { > Device (S00) > { > Name (_ADR, Zero) // _ADR: Address > } > > Device (S08) I think you should look for AMLs for Device (S18) because the bridge goes into slot 3. > { > 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) > } > } > } > } > > > > > > > > > > > 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 <ani@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, > > > > > > > > > > > > > > > >
On Tue, 15 Sep 2020 17:40:25 +0530 Ani Sinha <ani@anisinha.ca> wrote: > On Tue, Sep 15, 2020 at 5:21 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > On Mon, 14 Sep 2020 20:20:21 +0530 > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > On Mon, 14 Sep 2020 18:58:19 +0530 > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > > > > Ani Sinha <ani@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); > > > > and this also introduces mem leak > > > > > > > > > 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. > > > > well I'm not sure it works using ACPI hotplug, > > > > with > > -global PIIX4_PM.acpi-root-pci-hotplug=off > > -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on > > > > DSDT ins't what I'd expect when bridge hp is on: > > > > Scope (\_SB) > > { > > Scope (PCI0) > > { > > Device (S00) > > { > > Name (_ADR, Zero) // _ADR: Address > > } > > > > Device (S08) > > I think you should look for AMLs for Device (S18) because the bridge > goes into slot 3. as you see, there weren't any here. so maybe something else gone wrong in earlier patches > > > { > > 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) > > } > > } > > } > > } > > > > > > > > > > > > > > > > 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 <ani@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, > > > > > > > > > > > > > > > > > > > > > >
On Tue, Sep 15, 2020 at 6:19 PM Igor Mammedov <imammedo@redhat.com> wrote: > > On Tue, 15 Sep 2020 17:40:25 +0530 > Ani Sinha <ani@anisinha.ca> wrote: > > > On Tue, Sep 15, 2020 at 5:21 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > On Mon, 14 Sep 2020 20:20:21 +0530 > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > On Mon, 14 Sep 2020 18:58:19 +0530 > > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > > > > > Ani Sinha <ani@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); > > > > > > and this also introduces mem leak > > > > > > > > > > > > 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. > > > > > > well I'm not sure it works using ACPI hotplug, > > > > > > with > > > -global PIIX4_PM.acpi-root-pci-hotplug=off > > > -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on > > > > > > DSDT ins't what I'd expect when bridge hp is on: > > > > > > Scope (\_SB) > > > { > > > Scope (PCI0) > > > { > > > Device (S00) > > > { > > > Name (_ADR, Zero) // _ADR: Address > > > } > > > > > > Device (S08) > > > > I think you should look for AMLs for Device (S18) because the bridge > > goes into slot 3. > > as you see, there weren't any here. so maybe something else gone wrong > in earlier patches Can you send the full command line you are passing? You need to attach a bridge, then the function will recurse into it. > > > > > > { > > > 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) > > > } > > > } > > > } > > > } > > > > > > > > > > > > > > > > > > > > > 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 <ani@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, > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Tue, Sep 15, 2020 at 6:25 PM Ani Sinha <ani@anisinha.ca> wrote: > > On Tue, Sep 15, 2020 at 6:19 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > On Tue, 15 Sep 2020 17:40:25 +0530 > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > On Tue, Sep 15, 2020 at 5:21 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > On Mon, 14 Sep 2020 20:20:21 +0530 > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > On Mon, 14 Sep 2020 18:58:19 +0530 > > > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > > > > > > Ani Sinha <ani@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); > > > > > > > > and this also introduces mem leak > > > > > > > > > > > > > > > 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. > > > > > > > > well I'm not sure it works using ACPI hotplug, > > > > > > > > with > > > > -global PIIX4_PM.acpi-root-pci-hotplug=off > > > > -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on > > > > > > > > DSDT ins't what I'd expect when bridge hp is on: > > > > > > > > Scope (\_SB) > > > > { > > > > Scope (PCI0) > > > > { > > > > Device (S00) > > > > { > > > > Name (_ADR, Zero) // _ADR: Address > > > > } > > > > > > > > Device (S08) > > > > > > I think you should look for AMLs for Device (S18) because the bridge > > > goes into slot 3. > > > > as you see, there weren't any here. so maybe something else gone wrong > > in earlier patches > > Can you send the full command line you are passing? You need to attach > a bridge, then the function will recurse into it. Just to be clear, the AMLs will be added for every slot on the bridge. So you need to attach a bridge to see them. > > > > > > > > > > { > > > > 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) > > > > } > > > > } > > > > } > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > 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 <ani@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, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Tue, 15 Sep 2020 18:25:55 +0530 Ani Sinha <ani@anisinha.ca> wrote: > On Tue, Sep 15, 2020 at 6:19 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > On Tue, 15 Sep 2020 17:40:25 +0530 > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > On Tue, Sep 15, 2020 at 5:21 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > On Mon, 14 Sep 2020 20:20:21 +0530 > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > On Mon, 14 Sep 2020 18:58:19 +0530 > > > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > > > > > > Ani Sinha <ani@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); > > > > > > > > and this also introduces mem leak > > > > > > > > > > > > > > > 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. > > > > > > > > well I'm not sure it works using ACPI hotplug, > > > > > > > > with > > > > -global PIIX4_PM.acpi-root-pci-hotplug=off > > > > -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on > > > > > > > > DSDT ins't what I'd expect when bridge hp is on: > > > > > > > > Scope (\_SB) > > > > { > > > > Scope (PCI0) > > > > { > > > > Device (S00) > > > > { > > > > Name (_ADR, Zero) // _ADR: Address > > > > } > > > > > > > > Device (S08) > > > > > > I think you should look for AMLs for Device (S18) because the bridge > > > goes into slot 3. > > > > as you see, there weren't any here. so maybe something else gone wrong > > in earlier patches > > Can you send the full command line you are passing? You need to attach > a bridge, then the function will recurse into it. all your 9 pathes with this on top: diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 7e3cf3b57b..8339a3538a 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); } diff --git a/slirp b/slirp --- a/slirp +++ b/slirp @@ -1 +1 @@ -Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275 +Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275-dirty diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index 7632cfe1be..fb56c1eeb0 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -750,7 +750,7 @@ static void test_acpi_piix4_root_hotplug(void) data.required_struct_types = base_required_struct_types; data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); test_acpi_one("-global PIIX4_PM.acpi-root-pci-hotplug=off " - "-device pci-bridge,chassis_nr=1", &data); + "-device pci-bridge,chassis_nr=1 -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on", &data); free_test_data(&data); } and run test with: make -j32 && make tests/qtest/bios-tables-test && V=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img tests/qtest/bios-tables-test > > > > > > > > > > { > > > > 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) > > > > } > > > > } > > > > } > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > 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 <ani@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, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Tue, Sep 15, 2020 at 7:12 PM Igor Mammedov <imammedo@redhat.com> wrote: > > On Tue, 15 Sep 2020 18:25:55 +0530 > Ani Sinha <ani@anisinha.ca> wrote: > > > On Tue, Sep 15, 2020 at 6:19 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > On Tue, 15 Sep 2020 17:40:25 +0530 > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > On Tue, Sep 15, 2020 at 5:21 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > On Mon, 14 Sep 2020 20:20:21 +0530 > > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > > > On Mon, 14 Sep 2020 18:58:19 +0530 > > > > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > > > > > > > Ani Sinha <ani@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); > > > > > > > > > > and this also introduces mem leak > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > well I'm not sure it works using ACPI hotplug, > > > > > > > > > > with > > > > > -global PIIX4_PM.acpi-root-pci-hotplug=off > > > > > -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on > > > > > > > > > > DSDT ins't what I'd expect when bridge hp is on: > > > > > > > > > > Scope (\_SB) > > > > > { > > > > > Scope (PCI0) > > > > > { > > > > > Device (S00) > > > > > { > > > > > Name (_ADR, Zero) // _ADR: Address > > > > > } > > > > > > > > > > Device (S08) > > > > > > > > I think you should look for AMLs for Device (S18) because the bridge > > > > goes into slot 3. > > > > > > as you see, there weren't any here. so maybe something else gone wrong > > > in earlier patches > > > > Can you send the full command line you are passing? You need to attach > > a bridge, then the function will recurse into it. > > all your 9 pathes with this on top: > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 7e3cf3b57b..8339a3538a 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); > } > > diff --git a/slirp b/slirp > --- a/slirp > +++ b/slirp > @@ -1 +1 @@ > -Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275 > +Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275-dirty > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > index 7632cfe1be..fb56c1eeb0 100644 > --- a/tests/qtest/bios-tables-test.c > +++ b/tests/qtest/bios-tables-test.c > @@ -750,7 +750,7 @@ static void test_acpi_piix4_root_hotplug(void) > data.required_struct_types = base_required_struct_types; > data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); > test_acpi_one("-global PIIX4_PM.acpi-root-pci-hotplug=off " > - "-device pci-bridge,chassis_nr=1", &data); > + "-device pci-bridge,chassis_nr=1 -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on", &data); That's a noop since by default its ON. I ran the test and it passed. The disassembly of tests/data/acpi/pc/DSDT.roothp is here: https://pastebin.ubuntu.com/p/52VS6HRPx8/ Seems OK to me. The table for the bridge slots starts from line 871. > free_test_data(&data); > } > > > and run test with: > > make -j32 && make tests/qtest/bios-tables-test && V=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img tests/qtest/bios-tables-test > > > > > > > > > > > > > > > { > > > > > 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) > > > > > } > > > > > } > > > > > } > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 <ani@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, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Tue, Sep 15, 2020 at 7:33 PM Ani Sinha <ani@anisinha.ca> wrote: > > On Tue, Sep 15, 2020 at 7:12 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > On Tue, 15 Sep 2020 18:25:55 +0530 > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > On Tue, Sep 15, 2020 at 6:19 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > On Tue, 15 Sep 2020 17:40:25 +0530 > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > On Tue, Sep 15, 2020 at 5:21 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > On Mon, 14 Sep 2020 20:20:21 +0530 > > > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > > > > > On Mon, 14 Sep 2020 18:58:19 +0530 > > > > > > > > Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > > > > > > > > > > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > > > > > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530 > > > > > > > > > > > Ani Sinha <ani@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); > > > > > > > > > > > > and this also introduces mem leak > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > well I'm not sure it works using ACPI hotplug, > > > > > > > > > > > > with > > > > > > -global PIIX4_PM.acpi-root-pci-hotplug=off > > > > > > -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on > > > > > > > > > > > > DSDT ins't what I'd expect when bridge hp is on: > > > > > > > > > > > > Scope (\_SB) > > > > > > { > > > > > > Scope (PCI0) > > > > > > { > > > > > > Device (S00) > > > > > > { > > > > > > Name (_ADR, Zero) // _ADR: Address > > > > > > } > > > > > > > > > > > > Device (S08) > > > > > > > > > > I think you should look for AMLs for Device (S18) because the bridge > > > > > goes into slot 3. > > > > > > > > as you see, there weren't any here. so maybe something else gone wrong > > > > in earlier patches > > > > > > Can you send the full command line you are passing? You need to attach > > > a bridge, then the function will recurse into it. > > > > all your 9 pathes with this on top: > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 7e3cf3b57b..8339a3538a 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); > > } > > > > diff --git a/slirp b/slirp > > --- a/slirp > > +++ b/slirp > > @@ -1 +1 @@ > > -Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275 > > +Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275-dirty > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > > index 7632cfe1be..fb56c1eeb0 100644 > > --- a/tests/qtest/bios-tables-test.c > > +++ b/tests/qtest/bios-tables-test.c > > @@ -750,7 +750,7 @@ static void test_acpi_piix4_root_hotplug(void) > > data.required_struct_types = base_required_struct_types; > > data.required_struct_types_len = ARRAY_SIZE(base_required_struct_types); > > test_acpi_one("-global PIIX4_PM.acpi-root-pci-hotplug=off " > > - "-device pci-bridge,chassis_nr=1", &data); > > + "-device pci-bridge,chassis_nr=1 -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on", &data); > > That's a noop since by default its ON. > > I ran the test and it passed. The disassembly of > tests/data/acpi/pc/DSDT.roothp is here: > > https://pastebin.ubuntu.com/p/52VS6HRPx8/ > > Seems OK to me. The table for the bridge slots starts from line 871. I just sent v2 for the entire patch set. I have updated patch #7 and patch #9. Can you try again with the updated patch #9? I think it's OK now. Maybe the previous patch had an issue. > > > free_test_data(&data); > > } > > > > > > and run test with: > > > > make -j32 && make tests/qtest/bios-tables-test && V=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img tests/qtest/bios-tables-test > > > > > > > > > > > > > > > > > > > > { > > > > > > 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) > > > > > > } > > > > > > } > > > > > > } > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 <ani@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, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
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,
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. Signed-off-by: Ani Sinha <ani@anisinha.ca> --- hw/acpi/piix4.c | 6 ++++-- hw/i386/acpi-build.c | 10 ++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-)