[PATCH 0/3] RISC-V: ACPI: Namespace updates

[PATCH 0/3] RISC-V: ACPI: Namespace updates

[Sunil V L]

This series adds few updates to RISC-V ACPI namespace for virt platform.

1) PCI Link devices need to be created outside the scope of the PCI root
complex to ensure correct probe ordering by the OS. This matches the
example given in ACPI spec as well.

2) Add PLIC and APLIC as platform devices as well to ensure probing
order as per BRS spec [1] requirement.

3) BRS spec requires RISC-V to use new ACPI ID for the generic UART. So,
update the HID of the UART.

[1] - https://github.com/riscv-non-isa/riscv-brs

Sunil V L (3):
  gpex-acpi: Support PCI link devices outside the host bridge
  hw/riscv/virt-acpi-build.c: Add namespace devices for PLIC and APLIC
  hw/riscv/virt-acpi-build.c: Update the HID of RISC-V UART

 hw/pci-host/gpex-acpi.c    | 29 +++++++++++++++----
 hw/riscv/virt-acpi-build.c | 57 +++++++++++++++++++++++++++++++++++---
 include/hw/pci-host/gpex.h |  5 +++-
 3 files changed, 81 insertions(+), 10 deletions(-)

-- 
2.40.1

[PATCH 1/3] gpex-acpi: Support PCI link devices outside the host bridge

[Sunil V L]

Currently, PCI link devices (PNP0C0F) are always created within the
scope of the PCI root complex. However, RISC-V needs PCI link devices to
be outside the scope of the PCI host bridge to properly enable the probe
order. This matches the example given in the ACPI specification section
6.2.13.1 as well.

Enable creating link devices outside the scope of PCI root complex based
on the flag which gets set currently only for RISC-V.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 hw/pci-host/gpex-acpi.c    | 29 ++++++++++++++++++++++++-----
 hw/riscv/virt-acpi-build.c |  8 +++++---
 include/hw/pci-host/gpex.h |  5 ++++-
 3 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
index f69413ea2c..cea89a3ed8 100644
--- a/hw/pci-host/gpex-acpi.c
+++ b/hw/pci-host/gpex-acpi.c
@@ -7,7 +7,7 @@
 #include "hw/pci/pcie_host.h"
 #include "hw/acpi/cxl.h"
 
-static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq)
+static void acpi_dsdt_add_pci_route_table(Aml *scope, Aml *dev, uint32_t irq)
 {
     Aml *method, *crs;
     int i, slot_no;
@@ -45,7 +45,17 @@ static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq)
         aml_append(dev_gsi, aml_name_decl("_CRS", crs));
         method = aml_method("_SRS", 1, AML_NOTSERIALIZED);
         aml_append(dev_gsi, method);
-        aml_append(dev, dev_gsi);
+
+        /*
+         * Some architectures like RISC-V need PCI link devices created
+         * outside the scope of the PCI host bridge similar to the example
+         * given in the section 6.2.13.1 of ACPI spec 6.5.
+         */
+         if (scope) {
+            aml_append(scope, dev_gsi);
+        } else {
+            aml_append(dev, dev_gsi);
+        }
     }
 }
 
@@ -174,7 +184,11 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
                 aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node)));
             }
 
-            acpi_dsdt_add_pci_route_table(dev, cfg->irq);
+            if (cfg->flags & GPEX_FLAGS_EXT_GSI_LINK) {
+                acpi_dsdt_add_pci_route_table(scope, dev, cfg->irq);
+            } else {
+                acpi_dsdt_add_pci_route_table(NULL, dev, cfg->irq);
+            }
 
             /*
              * Resources defined for PXBs are composed of the following parts:
@@ -205,7 +219,11 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
     aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
     aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
 
-    acpi_dsdt_add_pci_route_table(dev, cfg->irq);
+    if (cfg->flags & GPEX_FLAGS_EXT_GSI_LINK) {
+        acpi_dsdt_add_pci_route_table(scope, dev, cfg->irq);
+    } else {
+        acpi_dsdt_add_pci_route_table(NULL, dev, cfg->irq);
+    }
 
     method = aml_method("_CBA", 0, AML_NOTSERIALIZED);
     aml_append(method, aml_return(aml_int(cfg->ecam.base)));
@@ -282,7 +300,7 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
     crs_range_set_free(&crs_range_set);
 }
 
-void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq)
+void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq, uint32_t flags)
 {
     bool ambig;
     Object *obj = object_resolve_path_type("", TYPE_GPEX_HOST, &ambig);
@@ -292,5 +310,6 @@ void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq)
     }
 
     GPEX_HOST(obj)->gpex_cfg.irq = irq;
+    GPEX_HOST(obj)->gpex_cfg.flags = flags;
     acpi_dsdt_add_gpex(scope, &GPEX_HOST(obj)->gpex_cfg);
 }
diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
index 0925528160..832a3acb8d 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -417,19 +417,21 @@ static void build_dsdt(GArray *table_data,
         virtio_acpi_dsdt_add(scope, memmap[VIRT_VIRTIO].base,
                              memmap[VIRT_VIRTIO].size,
                              VIRTIO_IRQ, 0, VIRTIO_COUNT);
-        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ);
+        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ, GPEX_FLAGS_EXT_GSI_LINK);
     } else if (socket_count == 2) {
         virtio_acpi_dsdt_add(scope, memmap[VIRT_VIRTIO].base,
                              memmap[VIRT_VIRTIO].size,
                              VIRTIO_IRQ + VIRT_IRQCHIP_NUM_SOURCES, 0,
                              VIRTIO_COUNT);
-        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES);
+        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES,
+                                GPEX_FLAGS_EXT_GSI_LINK);
     } else {
         virtio_acpi_dsdt_add(scope, memmap[VIRT_VIRTIO].base,
                              memmap[VIRT_VIRTIO].size,
                              VIRTIO_IRQ + VIRT_IRQCHIP_NUM_SOURCES, 0,
                              VIRTIO_COUNT);
-        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES * 2);
+        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES * 2,
+                                GPEX_FLAGS_EXT_GSI_LINK);
     }
 
     aml_append(dsdt, scope);
diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
index dce883573b..bee17d62c5 100644
--- a/include/hw/pci-host/gpex.h
+++ b/include/hw/pci-host/gpex.h
@@ -47,8 +47,11 @@ struct GPEXConfig {
     MemMapEntry pio;
     int         irq;
     PCIBus      *bus;
+    uint32_t    flags;
 };
 
+#define GPEX_FLAGS_EXT_GSI_LINK BIT(0)
+
 struct GPEXHost {
     /*< private >*/
     PCIExpressHost parent_obj;
@@ -71,7 +74,7 @@ struct GPEXHost {
 int gpex_set_irq_num(GPEXHost *s, int index, int gsi);
 
 void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg);
-void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq);
+void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq, uint32_t flags);
 
 #define PCI_HOST_PIO_BASE               "x-pio-base"
 #define PCI_HOST_PIO_SIZE               "x-pio-size"
-- 
2.40.1

Re: [PATCH 1/3] gpex-acpi: Support PCI link devices outside the host bridge

[Alistair Francis]

On Tue, May 28, 2024 at 5:32 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> Currently, PCI link devices (PNP0C0F) are always created within the
> scope of the PCI root complex. However, RISC-V needs PCI link devices to
> be outside the scope of the PCI host bridge to properly enable the probe
> order. This matches the example given in the ACPI specification section
> 6.2.13.1 as well.
>
> Enable creating link devices outside the scope of PCI root complex based
> on the flag which gets set currently only for RISC-V.
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/pci-host/gpex-acpi.c    | 29 ++++++++++++++++++++++++-----
>  hw/riscv/virt-acpi-build.c |  8 +++++---
>  include/hw/pci-host/gpex.h |  5 ++++-
>  3 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
> index f69413ea2c..cea89a3ed8 100644
> --- a/hw/pci-host/gpex-acpi.c
> +++ b/hw/pci-host/gpex-acpi.c
> @@ -7,7 +7,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "hw/acpi/cxl.h"
>
> -static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq)
> +static void acpi_dsdt_add_pci_route_table(Aml *scope, Aml *dev, uint32_t irq)
>  {
>      Aml *method, *crs;
>      int i, slot_no;
> @@ -45,7 +45,17 @@ static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq)
>          aml_append(dev_gsi, aml_name_decl("_CRS", crs));
>          method = aml_method("_SRS", 1, AML_NOTSERIALIZED);
>          aml_append(dev_gsi, method);
> -        aml_append(dev, dev_gsi);
> +
> +        /*
> +         * Some architectures like RISC-V need PCI link devices created
> +         * outside the scope of the PCI host bridge similar to the example
> +         * given in the section 6.2.13.1 of ACPI spec 6.5.
> +         */
> +         if (scope) {
> +            aml_append(scope, dev_gsi);
> +        } else {
> +            aml_append(dev, dev_gsi);
> +        }
>      }
>  }
>
> @@ -174,7 +184,11 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>                  aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node)));
>              }
>
> -            acpi_dsdt_add_pci_route_table(dev, cfg->irq);
> +            if (cfg->flags & GPEX_FLAGS_EXT_GSI_LINK) {
> +                acpi_dsdt_add_pci_route_table(scope, dev, cfg->irq);
> +            } else {
> +                acpi_dsdt_add_pci_route_table(NULL, dev, cfg->irq);
> +            }
>
>              /*
>               * Resources defined for PXBs are composed of the following parts:
> @@ -205,7 +219,11 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>      aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
>      aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
>
> -    acpi_dsdt_add_pci_route_table(dev, cfg->irq);
> +    if (cfg->flags & GPEX_FLAGS_EXT_GSI_LINK) {
> +        acpi_dsdt_add_pci_route_table(scope, dev, cfg->irq);
> +    } else {
> +        acpi_dsdt_add_pci_route_table(NULL, dev, cfg->irq);
> +    }
>
>      method = aml_method("_CBA", 0, AML_NOTSERIALIZED);
>      aml_append(method, aml_return(aml_int(cfg->ecam.base)));
> @@ -282,7 +300,7 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>      crs_range_set_free(&crs_range_set);
>  }
>
> -void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq)
> +void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq, uint32_t flags)
>  {
>      bool ambig;
>      Object *obj = object_resolve_path_type("", TYPE_GPEX_HOST, &ambig);
> @@ -292,5 +310,6 @@ void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq)
>      }
>
>      GPEX_HOST(obj)->gpex_cfg.irq = irq;
> +    GPEX_HOST(obj)->gpex_cfg.flags = flags;
>      acpi_dsdt_add_gpex(scope, &GPEX_HOST(obj)->gpex_cfg);
>  }
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 0925528160..832a3acb8d 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -417,19 +417,21 @@ static void build_dsdt(GArray *table_data,
>          virtio_acpi_dsdt_add(scope, memmap[VIRT_VIRTIO].base,
>                               memmap[VIRT_VIRTIO].size,
>                               VIRTIO_IRQ, 0, VIRTIO_COUNT);
> -        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ);
> +        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ, GPEX_FLAGS_EXT_GSI_LINK);
>      } else if (socket_count == 2) {
>          virtio_acpi_dsdt_add(scope, memmap[VIRT_VIRTIO].base,
>                               memmap[VIRT_VIRTIO].size,
>                               VIRTIO_IRQ + VIRT_IRQCHIP_NUM_SOURCES, 0,
>                               VIRTIO_COUNT);
> -        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES);
> +        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES,
> +                                GPEX_FLAGS_EXT_GSI_LINK);
>      } else {
>          virtio_acpi_dsdt_add(scope, memmap[VIRT_VIRTIO].base,
>                               memmap[VIRT_VIRTIO].size,
>                               VIRTIO_IRQ + VIRT_IRQCHIP_NUM_SOURCES, 0,
>                               VIRTIO_COUNT);
> -        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES * 2);
> +        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES * 2,
> +                                GPEX_FLAGS_EXT_GSI_LINK);
>      }
>
>      aml_append(dsdt, scope);
> diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
> index dce883573b..bee17d62c5 100644
> --- a/include/hw/pci-host/gpex.h
> +++ b/include/hw/pci-host/gpex.h
> @@ -47,8 +47,11 @@ struct GPEXConfig {
>      MemMapEntry pio;
>      int         irq;
>      PCIBus      *bus;
> +    uint32_t    flags;
>  };
>
> +#define GPEX_FLAGS_EXT_GSI_LINK BIT(0)
> +
>  struct GPEXHost {
>      /*< private >*/
>      PCIExpressHost parent_obj;
> @@ -71,7 +74,7 @@ struct GPEXHost {
>  int gpex_set_irq_num(GPEXHost *s, int index, int gsi);
>
>  void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg);
> -void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq);
> +void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq, uint32_t flags);
>
>  #define PCI_HOST_PIO_BASE               "x-pio-base"
>  #define PCI_HOST_PIO_SIZE               "x-pio-size"
> --
> 2.40.1
>
>

Re: [PATCH 1/3] gpex-acpi: Support PCI link devices outside the host bridge

[Michael S. Tsirkin]

On Tue, May 28, 2024 at 01:01:01PM +0530, Sunil V L wrote:
> Currently, PCI link devices (PNP0C0F) are always created within the
> scope of the PCI root complex. However, RISC-V needs PCI link devices to
> be outside the scope of the PCI host bridge to properly enable the probe
> order. This matches the example given in the ACPI specification section
> 6.2.13.1 as well.

Given that, what happens if we do this for all architectures?

> 
> Enable creating link devices outside the scope of PCI root complex based
> on the flag which gets set currently only for RISC-V.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/pci-host/gpex-acpi.c    | 29 ++++++++++++++++++++++++-----
>  hw/riscv/virt-acpi-build.c |  8 +++++---
>  include/hw/pci-host/gpex.h |  5 ++++-
>  3 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
> index f69413ea2c..cea89a3ed8 100644
> --- a/hw/pci-host/gpex-acpi.c
> +++ b/hw/pci-host/gpex-acpi.c
> @@ -7,7 +7,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "hw/acpi/cxl.h"
>  
> -static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq)
> +static void acpi_dsdt_add_pci_route_table(Aml *scope, Aml *dev, uint32_t irq)
>  {
>      Aml *method, *crs;
>      int i, slot_no;
> @@ -45,7 +45,17 @@ static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq)
>          aml_append(dev_gsi, aml_name_decl("_CRS", crs));
>          method = aml_method("_SRS", 1, AML_NOTSERIALIZED);
>          aml_append(dev_gsi, method);
> -        aml_append(dev, dev_gsi);
> +
> +        /*
> +         * Some architectures like RISC-V


Just risc-v for now right?
> need PCI link devices created
> +         * outside the scope of the PCI host bridge

.. in order to load the drivers in the correct order.
Others .... .

> similar to the example
> +         * given in the section 6.2.13.1 of ACPI spec 6.5.


This is not how we quote ACPI spec.

First you find the earliest spec version which has it.
Then you mention that, section/table # and title.
For example:

	ACPI 6.1: 18.3.2.8 Generic Hardware Error Source


> +         */
> +         if (scope) {
> +            aml_append(scope, dev_gsi);
> +        } else {
> +            aml_append(dev, dev_gsi);
> +        }
>      }
>  }
>  
> @@ -174,7 +184,11 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>                  aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node)));
>              }
>  
> -            acpi_dsdt_add_pci_route_table(dev, cfg->irq);
> +            if (cfg->flags & GPEX_FLAGS_EXT_GSI_LINK) {
> +                acpi_dsdt_add_pci_route_table(scope, dev, cfg->irq);
> +            } else {
> +                acpi_dsdt_add_pci_route_table(NULL, dev, cfg->irq);
> +            }
>  
>              /*
>               * Resources defined for PXBs are composed of the following parts:
> @@ -205,7 +219,11 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>      aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
>      aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
>  
> -    acpi_dsdt_add_pci_route_table(dev, cfg->irq);
> +    if (cfg->flags & GPEX_FLAGS_EXT_GSI_LINK) {
> +        acpi_dsdt_add_pci_route_table(scope, dev, cfg->irq);
> +    } else {
> +        acpi_dsdt_add_pci_route_table(NULL, dev, cfg->irq);
> +    }
>  
>      method = aml_method("_CBA", 0, AML_NOTSERIALIZED);
>      aml_append(method, aml_return(aml_int(cfg->ecam.base)));
> @@ -282,7 +300,7 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>      crs_range_set_free(&crs_range_set);
>  }
>  
> -void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq)
> +void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq, uint32_t flags)
>  {
>      bool ambig;
>      Object *obj = object_resolve_path_type("", TYPE_GPEX_HOST, &ambig);
> @@ -292,5 +310,6 @@ void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq)
>      }
>  
>      GPEX_HOST(obj)->gpex_cfg.irq = irq;
> +    GPEX_HOST(obj)->gpex_cfg.flags = flags;
>      acpi_dsdt_add_gpex(scope, &GPEX_HOST(obj)->gpex_cfg);
>  }
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 0925528160..832a3acb8d 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -417,19 +417,21 @@ static void build_dsdt(GArray *table_data,
>          virtio_acpi_dsdt_add(scope, memmap[VIRT_VIRTIO].base,
>                               memmap[VIRT_VIRTIO].size,
>                               VIRTIO_IRQ, 0, VIRTIO_COUNT);
> -        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ);
> +        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ, GPEX_FLAGS_EXT_GSI_LINK);
>      } else if (socket_count == 2) {
>          virtio_acpi_dsdt_add(scope, memmap[VIRT_VIRTIO].base,
>                               memmap[VIRT_VIRTIO].size,
>                               VIRTIO_IRQ + VIRT_IRQCHIP_NUM_SOURCES, 0,
>                               VIRTIO_COUNT);
> -        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES);
> +        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES,
> +                                GPEX_FLAGS_EXT_GSI_LINK);
>      } else {
>          virtio_acpi_dsdt_add(scope, memmap[VIRT_VIRTIO].base,
>                               memmap[VIRT_VIRTIO].size,
>                               VIRTIO_IRQ + VIRT_IRQCHIP_NUM_SOURCES, 0,
>                               VIRTIO_COUNT);
> -        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES * 2);
> +        acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES * 2,
> +                                GPEX_FLAGS_EXT_GSI_LINK);
>      }
>  
>      aml_append(dsdt, scope);
> diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
> index dce883573b..bee17d62c5 100644
> --- a/include/hw/pci-host/gpex.h
> +++ b/include/hw/pci-host/gpex.h
> @@ -47,8 +47,11 @@ struct GPEXConfig {
>      MemMapEntry pio;
>      int         irq;
>      PCIBus      *bus;
> +    uint32_t    flags;
>  };
>  
> +#define GPEX_FLAGS_EXT_GSI_LINK BIT(0)
> +
>  struct GPEXHost {
>      /*< private >*/
>      PCIExpressHost parent_obj;
> @@ -71,7 +74,7 @@ struct GPEXHost {
>  int gpex_set_irq_num(GPEXHost *s, int index, int gsi);
>  
>  void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg);
> -void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq);
> +void acpi_dsdt_add_gpex_host(Aml *scope, uint32_t irq, uint32_t flags);
>  
>  #define PCI_HOST_PIO_BASE               "x-pio-base"
>  #define PCI_HOST_PIO_SIZE               "x-pio-size"
> -- 
> 2.40.1
> 
> 

Re: [PATCH 1/3] gpex-acpi: Support PCI link devices outside the host bridge

[Sunil V L]

Hi Michael,

Thank you very much for the review!

On Wed, Jun 05, 2024 at 04:23:36AM -0400, Michael S. Tsirkin wrote:
> On Tue, May 28, 2024 at 01:01:01PM +0530, Sunil V L wrote:
> > Currently, PCI link devices (PNP0C0F) are always created within the
> > scope of the PCI root complex. However, RISC-V needs PCI link devices to
> > be outside the scope of the PCI host bridge to properly enable the probe
> > order. This matches the example given in the ACPI specification section
> > 6.2.13.1 as well.
> 
> Given that, what happens if we do this for all architectures?
> 
In theory and from my observation of linux on arm64, this should not
have any impact. However, I was bit hesitant to change for other
architectures since I was not sure how namespace changes like this will
affect different OS/architecture combination.

It looks like there is no real concern to make this change generic. That
should simplify the patch as well. Let me update in next version.
It would also warrant updating ACPI table blob for bios-table-test.

> > 
> > Enable creating link devices outside the scope of PCI root complex based
> > on the flag which gets set currently only for RISC-V.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  hw/pci-host/gpex-acpi.c    | 29 ++++++++++++++++++++++++-----
> >  hw/riscv/virt-acpi-build.c |  8 +++++---
> >  include/hw/pci-host/gpex.h |  5 ++++-
> >  3 files changed, 33 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
> > index f69413ea2c..cea89a3ed8 100644
> > --- a/hw/pci-host/gpex-acpi.c
> > +++ b/hw/pci-host/gpex-acpi.c
> > @@ -7,7 +7,7 @@
> >  #include "hw/pci/pcie_host.h"
> >  #include "hw/acpi/cxl.h"
> >  
> > -static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq)
> > +static void acpi_dsdt_add_pci_route_table(Aml *scope, Aml *dev, uint32_t irq)
> >  {
> >      Aml *method, *crs;
> >      int i, slot_no;
> > @@ -45,7 +45,17 @@ static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq)
> >          aml_append(dev_gsi, aml_name_decl("_CRS", crs));
> >          method = aml_method("_SRS", 1, AML_NOTSERIALIZED);
> >          aml_append(dev_gsi, method);
> > -        aml_append(dev, dev_gsi);
> > +
> > +        /*
> > +         * Some architectures like RISC-V
> 
> 
> Just risc-v for now right?
> > need PCI link devices created
> > +         * outside the scope of the PCI host bridge
> 
> .. in order to load the drivers in the correct order.
> Others .... .
> 
Okay.

> > similar to the example
> > +         * given in the section 6.2.13.1 of ACPI spec 6.5.
> 
> 
> This is not how we quote ACPI spec.
> 
> First you find the earliest spec version which has it.
> Then you mention that, section/table # and title.
> For example:
> 
> 	ACPI 6.1: 18.3.2.8 Generic Hardware Error Source
> 
>
Thanks!. Let me update as you suggested.

Thanks,
Sunil 

[PATCH 2/3] hw/riscv/virt-acpi-build.c: Add namespace devices for PLIC and APLIC

[Sunil V L]

PLIC and APLIC should be in namespace as well. So, add them using the
defined HID.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 hw/riscv/virt-acpi-build.c | 47 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
index 832a3acb8d..47ec78e432 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -141,6 +141,52 @@ static void acpi_dsdt_add_cpus(Aml *scope, RISCVVirtState *s)
     }
 }
 
+static void acpi_dsdt_add_plic_aplic(Aml *scope, RISCVVirtState *s)
+{
+    MachineState *ms = MACHINE(s);
+    uint64_t plic_aplic_addr;
+    uint32_t gsi_base;
+    uint8_t  socket;
+
+    if (s->aia_type == VIRT_AIA_TYPE_NONE) {
+        /* PLICs */
+        for (socket = 0; socket < riscv_socket_count(ms); socket++) {
+            plic_aplic_addr = s->memmap[VIRT_PLIC].base +
+                         s->memmap[VIRT_PLIC].size * socket;
+            gsi_base = VIRT_IRQCHIP_NUM_SOURCES * socket;
+            Aml *dev = aml_device("IC%.02X", socket);
+            aml_append(dev, aml_name_decl("_HID", aml_string("RSCV0001")));
+            aml_append(dev, aml_name_decl("_UID", aml_int(socket)));
+            aml_append(dev, aml_name_decl("_GSB", aml_int(gsi_base)));
+
+            Aml *crs = aml_resource_template();
+            aml_append(crs, aml_memory32_fixed(plic_aplic_addr,
+                                               s->memmap[VIRT_PLIC].size,
+                                               AML_READ_WRITE));
+            aml_append(dev, aml_name_decl("_CRS", crs));
+            aml_append(scope, dev);
+        }
+    } else {
+        /* APLICs */
+        for (socket = 0; socket < riscv_socket_count(ms); socket++) {
+            plic_aplic_addr = s->memmap[VIRT_APLIC_S].base +
+                             s->memmap[VIRT_APLIC_S].size * socket;
+            gsi_base = VIRT_IRQCHIP_NUM_SOURCES * socket;
+            Aml *dev = aml_device("IC%.02X", socket);
+            aml_append(dev, aml_name_decl("_HID", aml_string("RSCV0002")));
+            aml_append(dev, aml_name_decl("_UID", aml_int(socket)));
+            aml_append(dev, aml_name_decl("_GSB", aml_int(gsi_base)));
+
+            Aml *crs = aml_resource_template();
+            aml_append(crs, aml_memory32_fixed(plic_aplic_addr,
+                                               s->memmap[VIRT_APLIC_S].size,
+                                               AML_READ_WRITE));
+            aml_append(dev, aml_name_decl("_CRS", crs));
+            aml_append(scope, dev);
+        }
+    }
+}
+
 static void
 acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
                     uint32_t uart_irq)
@@ -411,6 +457,7 @@ static void build_dsdt(GArray *table_data,
 
     socket_count = riscv_socket_count(ms);
 
+    acpi_dsdt_add_plic_aplic(scope, s);
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART0], UART0_IRQ);
 
     if (socket_count == 1) {
-- 
2.40.1

Re: [PATCH 2/3] hw/riscv/virt-acpi-build.c: Add namespace devices for PLIC and APLIC

[Alistair Francis]

On Tue, May 28, 2024 at 5:32 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> PLIC and APLIC should be in namespace as well. So, add them using the
> defined HID.
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt-acpi-build.c | 47 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 832a3acb8d..47ec78e432 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -141,6 +141,52 @@ static void acpi_dsdt_add_cpus(Aml *scope, RISCVVirtState *s)
>      }
>  }
>
> +static void acpi_dsdt_add_plic_aplic(Aml *scope, RISCVVirtState *s)
> +{
> +    MachineState *ms = MACHINE(s);
> +    uint64_t plic_aplic_addr;
> +    uint32_t gsi_base;
> +    uint8_t  socket;
> +
> +    if (s->aia_type == VIRT_AIA_TYPE_NONE) {
> +        /* PLICs */
> +        for (socket = 0; socket < riscv_socket_count(ms); socket++) {
> +            plic_aplic_addr = s->memmap[VIRT_PLIC].base +
> +                         s->memmap[VIRT_PLIC].size * socket;
> +            gsi_base = VIRT_IRQCHIP_NUM_SOURCES * socket;
> +            Aml *dev = aml_device("IC%.02X", socket);
> +            aml_append(dev, aml_name_decl("_HID", aml_string("RSCV0001")));
> +            aml_append(dev, aml_name_decl("_UID", aml_int(socket)));
> +            aml_append(dev, aml_name_decl("_GSB", aml_int(gsi_base)));
> +
> +            Aml *crs = aml_resource_template();
> +            aml_append(crs, aml_memory32_fixed(plic_aplic_addr,
> +                                               s->memmap[VIRT_PLIC].size,
> +                                               AML_READ_WRITE));
> +            aml_append(dev, aml_name_decl("_CRS", crs));
> +            aml_append(scope, dev);
> +        }
> +    } else {
> +        /* APLICs */
> +        for (socket = 0; socket < riscv_socket_count(ms); socket++) {
> +            plic_aplic_addr = s->memmap[VIRT_APLIC_S].base +
> +                             s->memmap[VIRT_APLIC_S].size * socket;
> +            gsi_base = VIRT_IRQCHIP_NUM_SOURCES * socket;
> +            Aml *dev = aml_device("IC%.02X", socket);
> +            aml_append(dev, aml_name_decl("_HID", aml_string("RSCV0002")));
> +            aml_append(dev, aml_name_decl("_UID", aml_int(socket)));
> +            aml_append(dev, aml_name_decl("_GSB", aml_int(gsi_base)));
> +
> +            Aml *crs = aml_resource_template();
> +            aml_append(crs, aml_memory32_fixed(plic_aplic_addr,
> +                                               s->memmap[VIRT_APLIC_S].size,
> +                                               AML_READ_WRITE));
> +            aml_append(dev, aml_name_decl("_CRS", crs));
> +            aml_append(scope, dev);
> +        }
> +    }
> +}
> +
>  static void
>  acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
>                      uint32_t uart_irq)
> @@ -411,6 +457,7 @@ static void build_dsdt(GArray *table_data,
>
>      socket_count = riscv_socket_count(ms);
>
> +    acpi_dsdt_add_plic_aplic(scope, s);
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART0], UART0_IRQ);
>
>      if (socket_count == 1) {
> --
> 2.40.1
>
>

[PATCH 3/3] hw/riscv/virt-acpi-build.c: Update the HID of RISC-V UART

[Sunil V L]

RISC-V is going to use new HID RSCV0003 for generi UART. So, update the
HID.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 hw/riscv/virt-acpi-build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
index 47ec78e432..7f80368415 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -192,7 +192,7 @@ acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
                     uint32_t uart_irq)
 {
     Aml *dev = aml_device("COM0");
-    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0501")));
+    aml_append(dev, aml_name_decl("_HID", aml_string("RSCV0003")));
     aml_append(dev, aml_name_decl("_UID", aml_int(0)));
 
     Aml *crs = aml_resource_template();
-- 
2.40.1

Re: [PATCH 3/3] hw/riscv/virt-acpi-build.c: Update the HID of RISC-V UART

[Alistair Francis]

On Tue, May 28, 2024 at 5:32 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> RISC-V is going to use new HID RSCV0003 for generi UART. So, update the
> HID.
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt-acpi-build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 47ec78e432..7f80368415 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -192,7 +192,7 @@ acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
>                      uint32_t uart_irq)
>  {
>      Aml *dev = aml_device("COM0");
> -    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0501")));
> +    aml_append(dev, aml_name_decl("_HID", aml_string("RSCV0003")));
>      aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>
>      Aml *crs = aml_resource_template();
> --
> 2.40.1
>
>

Re: [PATCH 3/3] hw/riscv/virt-acpi-build.c: Update the HID of RISC-V UART

[Igor Mammedov]

On Tue, 28 May 2024 13:01:03 +0530
Sunil V L <sunilvl@ventanamicro.com> wrote:

> RISC-V is going to use new HID RSCV0003 for generi UART. So, update the
> HID.

where does it come from?

> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/riscv/virt-acpi-build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 47ec78e432..7f80368415 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -192,7 +192,7 @@ acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
>                      uint32_t uart_irq)
>  {
>      Aml *dev = aml_device("COM0");
> -    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0501")));
> +    aml_append(dev, aml_name_decl("_HID", aml_string("RSCV0003")));

the only place I've found (that could serve as justification)
  https://github.com/riscv-non-isa/riscv-brs/blame/main/acpi.adoc

which mentions _CID and not _HID as it is in this patch

>      aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>  
>      Aml *crs = aml_resource_template();

Re: [PATCH 3/3] hw/riscv/virt-acpi-build.c: Update the HID of RISC-V UART

[Sunil V L]

Hi Igor,
On Wed, Jun 05, 2024 at 04:48:14PM +0200, Igor Mammedov wrote:
> On Tue, 28 May 2024 13:01:03 +0530
> Sunil V L <sunilvl@ventanamicro.com> wrote:
> 
> > RISC-V is going to use new HID RSCV0003 for generi UART. So, update the
> > HID.
> 
> where does it come from?
> 
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  hw/riscv/virt-acpi-build.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> > index 47ec78e432..7f80368415 100644
> > --- a/hw/riscv/virt-acpi-build.c
> > +++ b/hw/riscv/virt-acpi-build.c
> > @@ -192,7 +192,7 @@ acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
> >                      uint32_t uart_irq)
> >  {
> >      Aml *dev = aml_device("COM0");
> > -    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0501")));
> > +    aml_append(dev, aml_name_decl("_HID", aml_string("RSCV0003")));
> 
> the only place I've found (that could serve as justification)
>   https://github.com/riscv-non-isa/riscv-brs/blame/main/acpi.adoc
> 
> which mentions _CID and not _HID as it is in this patch
> 
Right, this is the requirement.

Yes, _CID says device is compatible. But different vendors may have
different _HID. For qemu, _CID value is used as _HID since it is not
vendor specific. This is something similar to how ARMH0011 is used (qemu
vs RPi) in ARM world.

I am checking with Andrei to see if we can relax to make it either _HID
or _CID.

Thanks,
Sunil