Re: [RESEND RFC PATCH 3/3] samples: devsec: Add support for PCI_DOMAINS_GENERIC

[Suzuki K Poulose <suzuki.poulose@arm.com>]

Hi Dan

On 20/04/2025 19:29, Dan Williams wrote:
> Suzuki K Poulose wrote:
>> Allocate/free a domain at runtime for the sample devsec TSM.
>>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   samples/Kconfig         |  1 -
>>   samples/devsec/bus.c    | 32 +++++++++++++++++++++-----------
>>   samples/devsec/common.c |  2 +-
>>   samples/devsec/devsec.h |  2 +-
>>   4 files changed, 23 insertions(+), 14 deletions(-)
>>
>> diff --git a/samples/Kconfig b/samples/Kconfig
>> index 6bd64fc54ac1..f23be5088b9e 100644
>> --- a/samples/Kconfig
>> +++ b/samples/Kconfig
>> @@ -308,7 +308,6 @@ config SAMPLE_DEVSEC
>>   	tristate "Build a sample TEE Security Manager with an emulated PCI endpoint"
>>   	depends on PCI
>>   	depends on VIRT_DRIVERS
>> -	depends on X86 # TODO: PCI_DOMAINS_GENERIC support
>>   	select PCI_BRIDGE_EMUL
>>   	select PCI_TSM
>>   	select TSM
>> diff --git a/samples/devsec/bus.c b/samples/devsec/bus.c
>> index b78c04b21eb9..8ec04b3549f0 100644
>> --- a/samples/devsec/bus.c
>> +++ b/samples/devsec/bus.c
>> @@ -21,7 +21,7 @@
>>   #define NR_DEVSEC_DEVS 1
>>   
>>   struct devsec {
>> -	struct pci_sysdata sysdata;
>> +	int domain;
>>   	struct gen_pool *iomem_pool;
>>   	struct resource resource[2];
>>   	struct pci_bus *bus;
>> @@ -70,7 +70,7 @@ struct devsec {
>>   
>>   static struct devsec *bus_to_devsec(struct pci_bus *bus)
>>   {
>> -	return container_of(bus->sysdata, struct devsec, sysdata);
>> +	return container_of(bus->sysdata, struct devsec, domain);
>>   }
>>   
>>   static int devsec_dev_config_read(struct devsec *devsec, struct pci_bus *bus,
>> @@ -309,6 +309,17 @@ static struct pci_ops devsec_ops = {
>>   };
>>   
>>   /* borrowed from vmd_find_free_domain() */
>> +#ifdef CONFIG_PCI_GENERIC_DOMAINS
> 
> The config symbol name is PCI_DOMAINS_GENERIC, so the only way I can see
> this patch working is if you have ACPI=n in your build? See below.

You're right. But I did have ACPI=y, but built it for only arm64 (but 
only booted with DT).

>> @@ -633,15 +645,13 @@ static int __init devsec_bus_probe(struct platform_device *pdev)
>>   	};
>>   	pci_add_resource(&resources, &devsec->resource[1]);
>>   
>> -	sd = &devsec->sysdata;
>> -	devsec_sysdata = sd;
>> -	sd->domain = find_free_domain();
>> -	if (sd->domain < 0)
>> -		return sd->domain;
>> -
>> +	devsec_sysdata = &devsec->domain;
>> +	devsec->domain = find_free_domain();
>> +	if (devsec->domain < 0)
>> +		return devsec->domain;
>>   
>>   	devsec->bus = pci_create_root_bus(dev, 0, &devsec_ops,
>> -					  &devsec->sysdata, &resources);
>> +					  &devsec->domain, &resources);
> 
> See pci_register_host_bridge()... I don't think this works unless you
> have CONFIG_ACPI=n to avoid trying to lookup the non-existent ACPI
> device associated with this fake bridge.
> 
> I think the path here is to make emulated host bridges a first-class
> citizen of the PCI core.
> 
> The below seems to work. It adds a new pci_bus_find_emul_domain_nr() to
> unify a few cases where PCI domain host-bridges are being emulated, and
> it allows to the emulated domain_nr to be established between
> pci_alloc_host_bridge() and pci_register_host_bridge(). It still needs
> to play pci_sysdata hiding games for now:

I have tested this on arm64 (Juno board) with ACPI and DT and it works*

I have spotted a crash with an unrelated bug in the GICv2M driver, with
ACPI. I will send out a fix for that.

Thanks for reworking the patch !

FWIW:

Tested-by: Suzuki K Poulose <suzuki.poulose@arm.com> # on Arm64

Suzuki


> 
> -- 8< --
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 8df064b62a2f..efef329c359a 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -565,22 +565,6 @@ static void vmd_detach_resources(struct vmd_dev *vmd)
>   	vmd->dev->resource[VMD_MEMBAR2].child = NULL;
>   }
>   
> -/*
> - * VMD domains start at 0x10000 to not clash with ACPI _SEG domains.
> - * Per ACPI r6.0, sec 6.5.6,  _SEG returns an integer, of which the lower
> - * 16 bits are the PCI Segment Group (domain) number.  Other bits are
> - * currently reserved.
> - */
> -static int vmd_find_free_domain(void)
> -{
> -	int domain = 0xffff;
> -	struct pci_bus *bus = NULL;
> -
> -	while ((bus = pci_find_next_bus(bus)) != NULL)
> -		domain = max_t(int, domain, pci_domain_nr(bus));
> -	return domain + 1;
> -}
> -
>   static int vmd_get_phys_offsets(struct vmd_dev *vmd, bool native_hint,
>   				resource_size_t *offset1,
>   				resource_size_t *offset2)
> @@ -866,7 +850,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>   	};
>   
>   	sd->vmd_dev = vmd->dev;
> -	sd->domain = vmd_find_free_domain();
> +	sd->domain = pci_bus_find_emul_domain_nr();
>   	if (sd->domain < 0)
>   		return sd->domain;
>   
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 4d7c9f64ea24..0d5a08583197 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6774,7 +6774,7 @@ static void of_pci_bus_release_domain_nr(struct device *parent, int domain_nr)
>   		return;
>   
>   	/* Release domain from IDA where it was allocated. */
> -	if (of_get_pci_domain_nr(parent->of_node) == domain_nr)
> +	if (parent && of_get_pci_domain_nr(parent->of_node) == domain_nr)
>   		ida_free(&pci_domain_nr_static_ida, domain_nr);
>   	else
>   		ida_free(&pci_domain_nr_dynamic_ida, domain_nr);
> @@ -6794,6 +6794,42 @@ void pci_bus_release_domain_nr(struct device *parent, int domain_nr)
>   }
>   #endif
>   
> +/*
> + * Find a free domain_nr either allocated by of_pci_bus_find_domain_nr()
> + * or fallback to the first free domain number above the last ACPI
> + * segment number, do not ask acpi_pci_bus_find_domain_nr() it has no
> + * information about emulated host bridges
> + */
> +int pci_bus_find_emul_domain_nr(void)
> +{
> +	/*
> +	 * Emulated domains start at 0x10000 to not clash with ACPI _SEG
> +	 * domains.  Per ACPI r6.0, sec 6.5.6,  _SEG returns an integer, of
> +	 * which the lower 16 bits are the PCI Segment Group (domain) number.
> +	 * Other bits are currently reserved.
> +	 */
> +	int domain = 0xffff;
> +	struct pci_bus *bus = NULL;
> +
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +	if (acpi_disabled)
> +		return of_pci_bus_find_domain_nr(NULL);
> +#endif
> +
> +	while ((bus = pci_find_next_bus(bus)) != NULL)
> +		domain = max_t(int, domain, pci_domain_nr(bus));
> +	return domain + 1;
> +}
> +EXPORT_SYMBOL_GPL(pci_bus_find_emul_domain_nr);
> +
> +void pci_bus_release_emul_domain_nr(int domain_nr)
> +{
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +	/* Note that in the in the ACPI emulation case this is a nop */
> +	pci_bus_release_domain_nr(NULL, domain_nr);
> +#endif
> +}
> +
>   /**
>    * pci_ext_cfg_avail - can we access extended PCI config space?
>    *
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index c090289b70be..1e65692efc0a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -632,6 +632,11 @@ static void pci_release_host_bridge_dev(struct device *dev)
>   
>   	pci_free_resource_list(&bridge->windows);
>   	pci_free_resource_list(&bridge->dma_ranges);
> +
> +	/* Host bridges only have domain_nr set in the emulation case */
> +	if (bridge->domain_nr != PCI_DOMAIN_NR_NOT_SET)
> +		pci_bus_release_emul_domain_nr(bridge->domain_nr);
> +
>   	kfree(bridge);
>   }
>   
> @@ -949,7 +954,7 @@ static bool pci_preserve_config(struct pci_host_bridge *host_bridge)
>   	return false;
>   }
>   
> -static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> +int pci_register_host_bridge(struct pci_host_bridge *bridge)
>   {
>   	struct device *parent = bridge->dev.parent;
>   	struct resource_entry *window, *next, *n;
> @@ -1112,7 +1117,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>   	device_del(&bridge->dev);
>   free:
>   #ifdef CONFIG_PCI_DOMAINS_GENERIC
> -	pci_bus_release_domain_nr(parent, bus->domain_nr);
> +	if (bridge->domain_nr == PCI_DOMAIN_NR_NOT_SET)
> +		pci_bus_release_domain_nr(parent, bus->domain_nr);
>   #endif
>   	if (bus_registered)
>   		put_device(&bus->dev);
> @@ -1121,6 +1127,7 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>   
>   	return err;
>   }
> +EXPORT_SYMBOL_GPL(pci_register_host_bridge);
>   
>   static bool pci_bridge_child_ext_cfg_accessible(struct pci_dev *bridge)
>   {
> @@ -3176,6 +3183,22 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
>   {
>   }
>   
> +void pci_setup_host_bridge(struct pci_host_bridge *bridge,
> +			   struct device *parent, int bus, struct pci_ops *ops,
> +			   void *sysdata, struct list_head *resources,
> +			   int domain_nr)
> +{
> +	WARN_ON(device_is_registered(&bridge->dev));
> +	bridge->dev.parent = parent;
> +
> +	list_splice_init(resources, &bridge->windows);
> +	bridge->sysdata = sysdata;
> +	bridge->busnr = bus;
> +	bridge->ops = ops;
> +	bridge->domain_nr = domain_nr;
> +}
> +EXPORT_SYMBOL_GPL(pci_setup_host_bridge);
> +
>   struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>   		struct pci_ops *ops, void *sysdata, struct list_head *resources)
>   {
> @@ -3186,12 +3209,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>   	if (!bridge)
>   		return NULL;
>   
> -	bridge->dev.parent = parent;
> -
> -	list_splice_init(resources, &bridge->windows);
> -	bridge->sysdata = sysdata;
> -	bridge->busnr = bus;
> -	bridge->ops = ops;
> +	pci_setup_host_bridge(bridge, parent, bus, ops, sysdata, resources,
> +			      PCI_DOMAIN_NR_NOT_SET);
>   
>   	error = pci_register_host_bridge(bridge);
>   	if (error < 0)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 72d07ad994fa..f3bed5462279 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1165,6 +1165,11 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
>   struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>   				    struct pci_ops *ops, void *sysdata,
>   				    struct list_head *resources);
> +void pci_setup_host_bridge(struct pci_host_bridge *bridge,
> +			   struct device *parent, int bus, struct pci_ops *ops,
> +			   void *sysdata, struct list_head *resources,
> +			   int domain_nr);
> +int pci_register_host_bridge(struct pci_host_bridge *bridge);
>   int pci_host_probe(struct pci_host_bridge *bridge);
>   int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
>   int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
> @@ -1919,6 +1924,8 @@ static inline int acpi_pci_bus_find_domain_nr(struct pci_bus *bus)
>   int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent);
>   void pci_bus_release_domain_nr(struct device *parent, int domain_nr);
>   #endif
> +int pci_bus_find_emul_domain_nr(void);
> +void pci_bus_release_emul_domain_nr(int domain_nr);
>   
>   /* Some architectures require additional setup to direct VGA traffic */
>   typedef int (*arch_set_vga_state_t)(struct pci_dev *pdev, bool decode,
> diff --git a/samples/Kconfig b/samples/Kconfig
> index 6d6aeb736c8f..523a7129aed3 100644
> --- a/samples/Kconfig
> +++ b/samples/Kconfig
> @@ -317,7 +317,7 @@ config SAMPLE_DEVSEC
>   	tristate "Build a sample TEE Security Manager with an emulated PCI endpoint"
>   	depends on PCI
>   	depends on VIRT_DRIVERS
> -	depends on X86 # TODO: PCI_DOMAINS_GENERIC support
> +	depends on PCI_DOMAINS_GENERIC || X86
>   	select PCI_BRIDGE_EMUL
>   	select PCI_TSM
>   	select TSM
> diff --git a/samples/devsec/bus.c b/samples/devsec/bus.c
> index 69117db10897..0ca8acaf2204 100644
> --- a/samples/devsec/bus.c
> +++ b/samples/devsec/bus.c
> @@ -20,7 +20,7 @@
>   #define NR_DEVSEC_DEVS 1
>   
>   struct devsec {
> -	struct pci_sysdata sysdata;
> +	struct devsec_sysdata sysdata;
>   	struct gen_pool *iomem_pool;
>   	struct resource resource[2];
>   	struct pci_bus *bus;
> @@ -307,17 +307,6 @@ static struct pci_ops devsec_ops = {
>   	.write = devsec_pci_write,
>   };
>   
> -/* borrowed from vmd_find_free_domain() */
> -static int find_free_domain(void)
> -{
> -	int domain = 0xffff;
> -	struct pci_bus *bus = NULL;
> -
> -	while ((bus = pci_find_next_bus(bus)) != NULL)
> -		domain = max_t(int, domain, pci_domain_nr(bus));
> -	return domain + 1;
> -}
> -
>   static void destroy_bus(void *data)
>   {
>   	struct devsec *devsec = data;
> @@ -582,12 +571,60 @@ static int alloc_ports(struct devsec *devsec)
>   	return 0;
>   }
>   
> +static struct list_head *devsec_add_resources(struct devsec *devsec,
> +					      struct list_head *resources,
> +					      u64 mmio_start, u64 mmio_size)
> +{
> +	devsec->resource[0] = (struct resource) {
> +		.name = "DEVSEC BUSES",
> +		.start = 0,
> +		.end = NR_DEVSEC_BUSES + NR_DEVSEC_ROOT_PORTS - 1,
> +		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
> +	};
> +	pci_add_resource(resources, &devsec->resource[0]);
> +
> +	devsec->resource[1] = (struct resource) {
> +		.name = "DEVSEC MMIO",
> +		.start = mmio_start,
> +		.end = mmio_start + mmio_size - 1,
> +		.flags = IORESOURCE_MEM | IORESOURCE_MEM_64,
> +	};
> +	pci_add_resource(resources, &devsec->resource[1]);
> +
> +	return resources;
> +}
> +
> +static int devsec_autofree_bus(struct devsec *devsec, struct pci_bus *bus,
> +			       struct list_head *resources)
> +{
> +	devsec->bus = bus;
> +	return devm_add_action_or_reset(devsec->dev, destroy_bus, devsec);
> +}
> +
> +static int *devsec_alloc_domain_nr(int *domain_nr)
> +{
> +	*domain_nr = pci_bus_find_emul_domain_nr();
> +	if (*domain_nr < 0)
> +		return NULL;
> +	return domain_nr;
> +}
> +
> +static void devsec_free_domain_nr(int *domain_nr)
> +{
> +	if (domain_nr)
> +		pci_bus_release_emul_domain_nr(*domain_nr);
> +}
> +
> +DEFINE_FREE(free_resource_list, struct list_head *, if (_T) pci_free_resource_list(_T))
> +DEFINE_FREE(free_bridge, struct pci_host_bridge *, if (_T) put_device(&_T->dev));
> +DEFINE_FREE(free_domain_nr, int *, if (_T) devsec_free_domain_nr(_T))
> +
>   static int __init devsec_bus_probe(struct platform_device *pdev)
>   {
> -	int rc;
> -	LIST_HEAD(resources);
> +	int rc, __domain_nr;
> +	LIST_HEAD(__resources);
>   	struct devsec *devsec;
> -	struct pci_sysdata *sd;
> +	struct devsec_sysdata *sd;
>   	u64 mmio_size = SZ_64G;
>   	struct device *dev = &pdev->dev;
>   	u64 mmio_start = iomem_resource.end + 1 - SZ_64G;
> @@ -615,37 +652,34 @@ static int __init devsec_bus_probe(struct platform_device *pdev)
>   	if (rc)
>   		return rc;
>   
> -	devsec->resource[0] = (struct resource) {
> -		.name = "DEVSEC BUSES",
> -		.start = 0,
> -		.end = NR_DEVSEC_BUSES + NR_DEVSEC_ROOT_PORTS - 1,
> -		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
> -	};
> -	pci_add_resource(&resources, &devsec->resource[0]);
> +	struct list_head *resources __free(free_resource_list) =
> +		devsec_add_resources(devsec, &__resources, mmio_start,
> +				     mmio_size);
>   
> -	devsec->resource[1] = (struct resource) {
> -		.name = "DEVSEC MMIO",
> -		.start = mmio_start,
> -		.end = mmio_start + mmio_size - 1,
> -		.flags = IORESOURCE_MEM | IORESOURCE_MEM_64,
> -	};
> -	pci_add_resource(&resources, &devsec->resource[1]);
> +	struct pci_host_bridge *hb __free(free_bridge) = pci_alloc_host_bridge(0);
> +	if (!hb)
> +		return -ENOMEM;
>   
>   	sd = &devsec->sysdata;
>   	devsec_sysdata = sd;
> -	sd->domain = find_free_domain();
> -	if (sd->domain < 0)
> -		return sd->domain;
> -
> -
> -	devsec->bus = pci_create_root_bus(dev, 0, &devsec_ops,
> -					  &devsec->sysdata, &resources);
> -	if (!devsec->bus) {
> -		pci_free_resource_list(&resources);
> -		return -ENOMEM;
> -	}
> +	int *domain_nr __free(free_domain_nr) = devsec_alloc_domain_nr(&__domain_nr);
> +	if (!domain_nr)
> +		return __domain_nr;
> +
> +	/*
> +	 * Note, domain_nr is set in devsec_sysdata for
> +	 * !CONFIG_PCI_DOMAINS_GENERIC platforms
> +	 */
> +	devsec_set_domain_nr(sd, *no_free_ptr(domain_nr));
> +	pci_setup_host_bridge(hb, dev, 0, &devsec_ops, &devsec->sysdata,
> +			      resources, devsec_get_domain_nr(sd));
> +
> +	rc = pci_register_host_bridge(hb);
> +	if (rc)
> +		return rc;
>   
> -	rc = devm_add_action_or_reset(dev, destroy_bus, devsec);
> +	rc = devsec_autofree_bus(devsec, no_free_ptr(hb)->bus,
> +				 no_free_ptr(resources));
>   	if (rc)
>   		return rc;
>   
> diff --git a/samples/devsec/devsec.h b/samples/devsec/devsec.h
> index 794a9898ee2d..64e35731325b 100644
> --- a/samples/devsec/devsec.h
> +++ b/samples/devsec/devsec.h
> @@ -3,5 +3,38 @@
>   
>   #ifndef __DEVSEC_H__
>   #define __DEVSEC_H__
> -extern struct pci_sysdata *devsec_sysdata;
> +struct devsec_sysdata {
> +#ifdef CONFIG_X86
> +	/*
> +	 * Must be first member to that x86::pci_domain_nr() can type
> +	 * pun devsec_sysdata and pci_sysdata.
> +	 */
> +	struct pci_sysdata sd;
> +#else
> +	int domain_nr;
> +#endif
> +};
> +
> +#ifdef CONFIG_X86
> +static inline void devsec_set_domain_nr(struct devsec_sysdata *sd,
> +					int domain_nr)
> +{
> +	sd->sd.domain = domain_nr;
> +}
> +static inline int devsec_get_domain_nr(struct devsec_sysdata *sd)
> +{
> +	return sd->sd.domain;
> +}
> +#else
> +static inline void devsec_set_domain_nr(struct devsec_sysdata *sd,
> +					int domain_nr)
> +{
> +	sd->domain_nr = domain_nr;
> +}
> +static inline int devsec_get_domain_nr(struct devsec_sysdata *sd)
> +{
> +	return sd->domain_nr;
> +}
> +#endif
> +extern struct devsec_sysdata *devsec_sysdata;
>   #endif /* __DEVSEC_H__ */