diff mbox series

[RFC,3/5] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35

Message ID 20200708224615.114077-4-jusual@redhat.com (mailing list archive)
State New, archived
Headers show
Series Use ACPI PCI hot-plug for q35 | expand

Commit Message

Julia Suvorova July 8, 2020, 10:46 p.m. UTC
Implement notifications and gpe to support q35 ACPI PCI hot-plug.
The addresses specified in [1] remain the same to make fewer changes.

[1] docs/spec/acpi_pci_hotplug.txt

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 hw/i386/acpi-build.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

Igor Mammedov July 13, 2020, 2:39 p.m. UTC | #1
On Thu,  9 Jul 2020 00:46:13 +0200
Julia Suvorova <jusual@redhat.com> wrote:

> Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> The addresses specified in [1] remain the same to make fewer changes.
> 
> [1] docs/spec/acpi_pci_hotplug.txt

CCing Gerd his opinion on reusing piix4 IO port range for q35

 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
>  hw/i386/acpi-build.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 11c598f955..5c5ad88ad6 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
>          /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
>          pm->fadt.rev = 1;
>          pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> -        pm->pcihp_io_base =
> -            object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> -        pm->pcihp_io_len =
> -            object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
>      }
>      if (lpc) {
>          struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
>          pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
>          pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
>      }
> +    pm->pcihp_io_base =
> +        object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> +    pm->pcihp_io_len =
> +        object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
>  
>      /* The above need not be conditional on machine type because the reset port
>       * happens to be the same on PIIX (pc) and ICH9 (q35). */
> @@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>          QLIST_FOREACH(sec, &bus->child, sibling) {
>              int32_t devfn = sec->parent_dev->devfn;
>  
> -            if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
> +            if (pci_bus_is_root(sec)) {
>                  continue;
>              }
>  
> @@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table)
>      aml_append(table, scope);
>  }
>  
> -static Aml *build_q35_osc_method(void)
> +static void build_q35_pci_hotplug(Aml *table)
> +{
> +    build_piix4_pci_hotplug(table);
> +}

s/build_piix4_pci_hotplug/build_i386_acpi_pci_hotplug/

and reuse it in both cases, instead of adding wrapper?

> +
> +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
>  {
>      Aml *if_ctx;
>      Aml *if_ctx2;
> @@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          build_hpet_aml(dsdt);
>          build_q35_isa_bridge(dsdt);
>          build_isa_devices_aml(dsdt);
> +        build_q35_pci_hotplug(dsdt);
>          build_q35_pci0_int(dsdt);
>          if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
>              build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
> @@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      {
>          aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
>  
> -        if (misc->is_piix4) {
> +        if (misc->is_piix4 || pm->pcihp_bridge_en) {
>              method = aml_method("_E01", 0, AML_NOTSERIALIZED);
>              aml_append(method,
>                  aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
Michael S. Tsirkin July 14, 2020, 9:22 a.m. UTC | #2
On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote:
> On Thu,  9 Jul 2020 00:46:13 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
> 
> > Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> > The addresses specified in [1] remain the same to make fewer changes.
> > 
> > [1] docs/spec/acpi_pci_hotplug.txt
> 
> CCing Gerd his opinion on reusing piix4 IO port range for q35
> 
>  
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> >  hw/i386/acpi-build.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 11c598f955..5c5ad88ad6 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> >          /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
> >          pm->fadt.rev = 1;
> >          pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> > -        pm->pcihp_io_base =
> > -            object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > -        pm->pcihp_io_len =
> > -            object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> >      }
> >      if (lpc) {
> >          struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> > @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> >          pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
> >          pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
> >      }
> > +    pm->pcihp_io_base =
> > +        object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > +    pm->pcihp_io_len =
> > +        object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> >  
> >      /* The above need not be conditional on machine type because the reset port
> >       * happens to be the same on PIIX (pc) and ICH9 (q35). */
> > @@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> >          QLIST_FOREACH(sec, &bus->child, sibling) {
> >              int32_t devfn = sec->parent_dev->devfn;
> >  
> > -            if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
> > +            if (pci_bus_is_root(sec)) {
> >                  continue;
> >              }
> >  
> > @@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table)
> >      aml_append(table, scope);
> >  }
> >  
> > -static Aml *build_q35_osc_method(void)
> > +static void build_q35_pci_hotplug(Aml *table)
> > +{
> > +    build_piix4_pci_hotplug(table);
> > +}
> 
> s/build_piix4_pci_hotplug/build_i386_acpi_pci_hotplug/
> 
> and reuse it in both cases, instead of adding wrapper?

I'm not sure about that - we have microvm too ...

> > +
> > +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> >  {
> >      Aml *if_ctx;
> >      Aml *if_ctx2;
> > @@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          build_hpet_aml(dsdt);
> >          build_q35_isa_bridge(dsdt);
> >          build_isa_devices_aml(dsdt);
> > +        build_q35_pci_hotplug(dsdt);
> >          build_q35_pci0_int(dsdt);
> >          if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
> >              build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
> > @@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >      {
> >          aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
> >  
> > -        if (misc->is_piix4) {
> > +        if (misc->is_piix4 || pm->pcihp_bridge_en) {
> >              method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> >              aml_append(method,
> >                  aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
Igor Mammedov July 14, 2020, 2:57 p.m. UTC | #3
On Tue, 14 Jul 2020 05:22:20 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote:
> > On Thu,  9 Jul 2020 00:46:13 +0200
> > Julia Suvorova <jusual@redhat.com> wrote:
> >   
> > > Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> > > The addresses specified in [1] remain the same to make fewer changes.
> > > 
> > > [1] docs/spec/acpi_pci_hotplug.txt  
> > 
> > CCing Gerd his opinion on reusing piix4 IO port range for q35
> > 
> >    
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > ---
> > >  hw/i386/acpi-build.c | 20 +++++++++++++-------
> > >  1 file changed, 13 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 11c598f955..5c5ad88ad6 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> > >          /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
> > >          pm->fadt.rev = 1;
> > >          pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> > > -        pm->pcihp_io_base =
> > > -            object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > > -        pm->pcihp_io_len =
> > > -            object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> > >      }
> > >      if (lpc) {
> > >          struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> > > @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> > >          pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
> > >          pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
> > >      }
> > > +    pm->pcihp_io_base =
> > > +        object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > > +    pm->pcihp_io_len =
> > > +        object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> > >  
> > >      /* The above need not be conditional on machine type because the reset port
> > >       * happens to be the same on PIIX (pc) and ICH9 (q35). */
> > > @@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> > >          QLIST_FOREACH(sec, &bus->child, sibling) {
> > >              int32_t devfn = sec->parent_dev->devfn;
> > >  
> > > -            if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
> > > +            if (pci_bus_is_root(sec)) {
> > >                  continue;
> > >              }
> > >  
> > > @@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table)
> > >      aml_append(table, scope);
> > >  }
> > >  
> > > -static Aml *build_q35_osc_method(void)
> > > +static void build_q35_pci_hotplug(Aml *table)
> > > +{
> > > +    build_piix4_pci_hotplug(table);
> > > +}  
> > 
> > s/build_piix4_pci_hotplug/build_i386_acpi_pci_hotplug/
> > 
> > and reuse it in both cases, instead of adding wrapper?  
> 
> I'm not sure about that - we have microvm too ...
it doesn't have pci if I'm not mistaken,
and when/if it gains one someday, it may or maynot use hotplug
it will be up to its specific DSDT to include this call.

What doesn't make sense is using board specific naming
here for the same code. I don't insist on the variant I've suggested,
only on consolidating it instead of adding dummy wrapper


> > > +
> > > +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> > >  {
> > >      Aml *if_ctx;
> > >      Aml *if_ctx2;
> > > @@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >          build_hpet_aml(dsdt);
> > >          build_q35_isa_bridge(dsdt);
> > >          build_isa_devices_aml(dsdt);
> > > +        build_q35_pci_hotplug(dsdt);
> > >          build_q35_pci0_int(dsdt);
> > >          if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
> > >              build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
> > > @@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >      {
> > >          aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
> > >  
> > > -        if (misc->is_piix4) {
> > > +        if (misc->is_piix4 || pm->pcihp_bridge_en) {
> > >              method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> > >              aml_append(method,
> > >                  aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));  
>
Gerd Hoffmann July 15, 2020, 6:57 a.m. UTC | #4
On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote:
> On Thu,  9 Jul 2020 00:46:13 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
> 
> > Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> > The addresses specified in [1] remain the same to make fewer changes.
> > 
> > [1] docs/spec/acpi_pci_hotplug.txt
> 
> CCing Gerd his opinion on reusing piix4 IO port range for q35

Oh no, please don't.  Grabbing random io ports is asking for trouble,
especially on q35 where you only need enough pcie devices to have the
port range 0xae00 -> 0xae0f actually overlap with a pci bridge window.


Ideally we'll find some unused spot in the hidden pci bar of the ich9
pm device.  At least the qemu implementation has some room:

    0000000000000600-000000000000067f (prio 0, i/o): ich9-pm
      0000000000000600-0000000000000603 (prio 0, i/o): acpi-evt
      0000000000000604-0000000000000605 (prio 0, i/o): acpi-cnt
      0000000000000608-000000000000060b (prio 0, i/o): acpi-tmr
      0000000000000620-000000000000062f (prio 0, i/o): acpi-gpe0
      0000000000000630-0000000000000637 (prio 0, i/o): acpi-smi
      0000000000000660-000000000000067f (prio 0, i/o): sm-tco

Offset 0x40 seems to be unused for example (but better check the chipset
specs to see if that is really unused or whenever the qemu emulation is
incomplete).


If that doesn't work out pick an io range which is unlikely to conflict
with something.  That excludes anything above > 0x1000 (pci bridge
windows) and anything below 0x3ff (legacy isa).  From the remaining
range 0x400 -> 0xfff the 0xc00 -> 0xcff block is the best candidate
I think.  Because of the pci config registers being there it is unlikely
that the firmware places something in that range.

Oh, I see the cpu hotplug registers are already there:

    [ ... ]
    0000000000000700-000000000000073f (prio 1, i/o): pm-smbus
    0000000000000cd8-0000000000000ce3 (prio 0, i/o): acpi-cpu-hotplug
    0000000000000cf8-0000000000000cfb (prio 0, i/o): pci-conf-idx
    0000000000000cf9-0000000000000cf9 (prio 1, i/o): lpc-reset-control
    0000000000000cfc-0000000000000cff (prio 0, i/o): pci-conf-data
    [ ... ]

So placing the pci hotplug registers next to them (say at 0xcc8) looks
like a good pick to me.


While being at it:  Shouldn't we reserve these port ranges somehow?
Using an acpi device for example, simliar to fw_cfg?  The guest OS
should better know there is something at those ports ...

take care,
  Gerd
Igor Mammedov July 15, 2020, 1:17 p.m. UTC | #5
On Wed, 15 Jul 2020 08:57:51 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote:
> > On Thu,  9 Jul 2020 00:46:13 +0200
> > Julia Suvorova <jusual@redhat.com> wrote:
> >   
> > > Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> > > The addresses specified in [1] remain the same to make fewer changes.
> > > 
> > > [1] docs/spec/acpi_pci_hotplug.txt  
> > 
> > CCing Gerd his opinion on reusing piix4 IO port range for q35  

[...]

> While being at it:  Shouldn't we reserve these port ranges somehow?
> Using an acpi device for example, simliar to fw_cfg?  The guest OS
> should better know there is something at those ports ...

we do it at ACPI level in DSDT, look for comment
/* reserve PCIHP resources */

It should make Windows trip over in case of another range overlap with
reserved ports. (linux kernel is more tolerant and may silently ignore
or print a warning) 

> take care,
>   Gerd
>
Gerd Hoffmann July 15, 2020, 2:02 p.m. UTC | #6
> > While being at it:  Shouldn't we reserve these port ranges somehow?
> > Using an acpi device for example, simliar to fw_cfg?  The guest OS
> > should better know there is something at those ports ...
> 
> we do it at ACPI level in DSDT, look for comment
> /* reserve PCIHP resources */

Ah, good.

> It should make Windows trip over in case of another range overlap with
> reserved ports. (linux kernel is more tolerant and may silently ignore
> or print a warning) 

Hmm, linux doesn't list the device, can't see it neither in the kernel
log nor in /proc/ioports ...

take care,
  Gerd
diff mbox series

Patch

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 11c598f955..5c5ad88ad6 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -201,10 +201,6 @@  static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
         /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
         pm->fadt.rev = 1;
         pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
-        pm->pcihp_io_base =
-            object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
-        pm->pcihp_io_len =
-            object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
     }
     if (lpc) {
         struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
@@ -214,6 +210,10 @@  static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
         pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
         pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
     }
+    pm->pcihp_io_base =
+        object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
+    pm->pcihp_io_len =
+        object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
 
     /* The above need not be conditional on machine type because the reset port
      * happens to be the same on PIIX (pc) and ICH9 (q35). */
@@ -472,7 +472,7 @@  static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
         QLIST_FOREACH(sec, &bus->child, sibling) {
             int32_t devfn = sec->parent_dev->devfn;
 
-            if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
+            if (pci_bus_is_root(sec)) {
                 continue;
             }
 
@@ -1586,7 +1586,12 @@  static void build_piix4_pci_hotplug(Aml *table)
     aml_append(table, scope);
 }
 
-static Aml *build_q35_osc_method(void)
+static void build_q35_pci_hotplug(Aml *table)
+{
+    build_piix4_pci_hotplug(table);
+}
+
+static Aml *build_q35_osc_method(AcpiPmInfo *pm)
 {
     Aml *if_ctx;
     Aml *if_ctx2;
@@ -1698,6 +1703,7 @@  build_dsdt(GArray *table_data, BIOSLinker *linker,
         build_hpet_aml(dsdt);
         build_q35_isa_bridge(dsdt);
         build_isa_devices_aml(dsdt);
+        build_q35_pci_hotplug(dsdt);
         build_q35_pci0_int(dsdt);
         if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
             build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
@@ -1724,7 +1730,7 @@  build_dsdt(GArray *table_data, BIOSLinker *linker,
     {
         aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
 
-        if (misc->is_piix4) {
+        if (misc->is_piix4 || pm->pcihp_bridge_en) {
             method = aml_method("_E01", 0, AML_NOTSERIALIZED);
             aml_append(method,
                 aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));