Re: [RFC PATCH 15/25] acpi/pxb/cxl: Reserve host bridge MMIO

[Jonathan Cameron]

On Tue, 10 Nov 2020 21:47:14 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> For all host bridges, reserve MMIO space with _CRS. The MMIO for the
> host bridge lives in a magically hard coded space in the system's
> physical address space. The standard mechanism to tell the OS about
> regions which can't be used for host bridges is _CRS.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  hw/i386/acpi-build.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index aaed7da7dc..fae4fa28e1 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -28,6 +28,7 @@
>  #include "qemu/bitmap.h"
>  #include "qemu/error-report.h"
>  #include "hw/pci/pci.h"
> +#include "hw/cxl/cxl.h"
>  #include "hw/core/cpu.h"
>  #include "target/i386/cpu.h"
>  #include "hw/misc/pvpanic.h"
> @@ -1486,7 +1487,7 @@ static void build_smb0(Aml *table, I2CBus *smbus, int 
> devnr, int func)
>      aml_append(table, scope);
>  }
>  
> -enum { PCI, PCIE };
> +enum { PCI, PCIE, CXL };
>  static void init_pci_acpi(Aml *dev, int uid, int type)
>  {
>      if (type == PCI) {
> @@ -1635,20 +1636,28 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              uint8_t bus_num = pci_bus_num(bus);
>              uint8_t numa_node = pci_bus_numa_node(bus);
>              int32_t uid = pci_bus_uid(bus);
> +            int type;
>  
>              /* look only for expander root buses */
>              if (!pci_bus_is_root(bus)) {
>                  continue;
>              }
>  
> +            type = pci_bus_is_cxl(bus) ? CXL :
> +                                         pci_bus_is_express(bus) ? PCIE : 
> PCI;
> +
>              if (bus_num < root_bus_limit) {
>                  root_bus_limit = bus_num - 1;
>              }
>  
>              scope = aml_scope("\\_SB");
> -            dev = aml_device("PC%.02X", bus_num);
> +            if (type == CXL) {
> +                dev = aml_device("CXL%.01X", pci_bus_uid(bus));
> +            } else {
> +                dev = aml_device("PC%.02X", bus_num);
> +            }
>              aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num)));
> -            init_pci_acpi(dev, uid, pci_bus_is_express(bus) ? PCIE : PCI);
> +            init_pci_acpi(dev, uid, type);

Ah, so you are relying on the fact you didn't do a switch in init_pci_acpi.
I'd rather see that called out explicitly there though as then obvious what
subset of bus types share the same init. It isn't trivial to follow through
this code right now adn work out what ends up in the AML for each type.
It might even be worth just allowing some repetition to make that easier
to see.

>  
>              if (numa_node != NUMA_NODE_UNASSIGNED) {
>                  aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node)));
> @@ -1659,6 +1668,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              aml_append(dev, aml_name_decl("_CRS", crs));
>              aml_append(scope, dev);
>              aml_append(dsdt, scope);
> +
> +            /* Handle the ranges for the PXB expanders */
> +            if (type == CXL) {
> +                uint64_t base = CXL_HOST_BASE + uid * 0x10000;
> +                crs_range_insert(crs_range_set.mem_ranges, base,
> +                                 base + 0x10000 - 1);
> +            }
>          }
>      }
>