Re: [PATCH v2 28/31] coco/tdx-host: Parse ACPI KEYP table to init IDE for PCI host bridges

[Ackerley Tng]

Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx> writes:

> Parse the KEYP Key Configuration Units (KCU), to decide the max IDE
> streams supported for each host bridge.
>
> The KEYP table points to a number of KCU structures that each associates
> with a list of root ports (RP) via segment, bus, and devfn. Sanity check
> the KEYP table, ensure all RPs listed for each KCU are included in one
> host bridge. Then extact the max IDE streams supported to
> pci_host_bridge via pci_ide_set_nr_streams().
>
> Co-developed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
> Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> ---
>  drivers/virt/coco/tdx-host/tdx-host.c | 111 ++++++++++++++++++++++++++
>  1 file changed, 111 insertions(+)
>
> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index 4d127b7c2591..d5072a68b81a 100644
> --- a/drivers/virt/coco/tdx-host/tdx-host.c
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2025 Intel Corporation
>   */
>
> +#include <linux/acpi.h>
>  #include <linux/bitfield.h>
>  #include <linux/device/faux.h>
>  #include <linux/dmar.h>
> @@ -12,6 +13,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/pci.h>
>  #include <linux/pci-doe.h>
> +#include <linux/pci-ide.h>
>  #include <linux/pci-tsm.h>
>  #include <linux/tsm.h>
>  #include <linux/vmalloc.h>
> @@ -474,6 +476,111 @@ static void unregister_link_tsm(void *link)
>  	tsm_unregister(link);
>  }
>
> +#define KCU_STR_CAP_NUM_STREAMS		GENMASK(8, 0)
> +
> +/* The bus_end is inclusive */
> +struct keyp_hb_info {
> +	/* input */
> +	u16 segment;
> +	u8 bus_start;
> +	u8 bus_end;

Would you consider using bus_last instead of bus_end to indicate that
bus_last is inclusive? Or would it be better to say nr_buses to avoid
the inclusive/exclusive problem?

> +	/* output */
> +	u8 nr_ide_streams;
> +};
> +
> +static bool keyp_info_match(struct acpi_keyp_rp_info *rp,
> +			    struct keyp_hb_info *hb)
> +{
> +	return rp->segment == hb->segment && rp->bus >= hb->bus_start &&
> +	       rp->bus <= hb->bus_end;
> +}
> +
> +static int keyp_config_unit_handler(union acpi_subtable_headers *header,
> +				    void *arg, const unsigned long end)
> +{
> +	struct acpi_keyp_config_unit *acpi_cu =
> +		(struct acpi_keyp_config_unit *)&header->keyp;
> +	struct keyp_hb_info *hb_info = arg;
> +	int rp_size, rp_count, i;
> +	void __iomem *addr;
> +	bool match = false;
> +	u32 cap;
> +
> +	rp_size = acpi_cu->header.length - sizeof(*acpi_cu);
> +	if (rp_size % sizeof(struct acpi_keyp_rp_info))
> +		return -EINVAL;
> +
> +	rp_count = rp_size / sizeof(struct acpi_keyp_rp_info);
> +	if (!rp_count || rp_count != acpi_cu->root_port_count)
> +		return -EINVAL;
> +
> +	for (i = 0; i < rp_count; i++) {
> +		struct acpi_keyp_rp_info *rp_info = &acpi_cu->rp_info[i];
> +
> +		if (i == 0) {
> +			match = keyp_info_match(rp_info, hb_info);
> +			/* The host bridge already matches another KCU */
> +			if (match && hb_info->nr_ide_streams)
> +				return -EINVAL;
> +
> +			continue;
> +		}
> +
> +		if (match ^ keyp_info_match(rp_info, hb_info))
> +			return -EINVAL;
> +	}
> +
> +	if (!match)
> +		return 0;
> +
> +	addr = ioremap(acpi_cu->register_base_address, sizeof(cap));
> +	if (!addr)
> +		return -ENOMEM;
> +	cap = ioread32(addr);
> +	iounmap(addr);
> +
> +	hb_info->nr_ide_streams = FIELD_GET(KCU_STR_CAP_NUM_STREAMS, cap) + 1;
> +
> +	return 0;
> +}
> +
> +static u8 keyp_find_nr_ide_stream(u16 segment, u8 bus_start, u8 bus_end)
> +{
> +	struct keyp_hb_info hb_info = {
> +		.segment = segment,
> +		.bus_start = bus_start,
> +		.bus_end = bus_end,
> +	};
> +	int rc;
> +
> +	rc = acpi_table_parse_keyp(ACPI_KEYP_TYPE_CONFIG_UNIT,
> +				   keyp_config_unit_handler, &hb_info);
> +	if (rc < 0)
> +		return 0;
> +
> +	return hb_info.nr_ide_streams;
> +}
> +
> +static void keyp_setup_nr_ide_stream(struct pci_bus *bus)
> +{
> +	struct pci_host_bridge *hb = pci_find_host_bridge(bus);
> +	u8 nr_ide_streams;
> +
> +	nr_ide_streams = keyp_find_nr_ide_stream(pci_domain_nr(bus),
> +						 bus->busn_res.start,
> +						 bus->busn_res.end);
> +
> +	pci_ide_set_nr_streams(hb, nr_ide_streams);
> +}
> +
> +static void tdx_setup_nr_ide_stream(void)

Was wondering, is nr in the name meant as an abbreviation for number?
Why not just tdx_setup_ide_streams(), since this doesn't really set up a
specific number of streams.

Same for the naming of keyp_setup_nr_ide_stream().

> +{
> +	struct pci_bus *bus = NULL;
> +
> +	while ((bus = pci_find_next_bus(bus)))
> +		keyp_setup_nr_ide_stream(bus);
> +}
> +
>  static DEFINE_XARRAY(tlink_iommu_xa);
>
>  static void tdx_iommu_clear(u64 iommu_id, struct tdx_page_array *iommu_mt)
> @@ -569,6 +676,8 @@ static int __maybe_unused tdx_connect_init(struct device *dev)
>  	if (ret)
>  		return ret;
>
> +	tdx_setup_nr_ide_stream();
> +
>  	link = tsm_register(dev, &tdx_tsm_link_ops);
>  	if (IS_ERR(link))
>  		return dev_err_probe(dev, PTR_ERR(link),
> @@ -612,5 +721,7 @@ static void __exit tdx_host_exit(void)
>  }
>  module_exit(tdx_host_exit);
>
> +MODULE_IMPORT_NS("ACPI");
> +MODULE_IMPORT_NS("PCI_IDE");
>  MODULE_DESCRIPTION("TDX Host Services");
>  MODULE_LICENSE("GPL");
> --
> 2.25.1