Re: [RFC v2 5/7] acpi:arm64: Add support for parsing IORT table

[Julien Grall <julien.grall@linaro.org>]

Hi Sameer,

On 21/09/17 01:37, Sameer Goel wrote:
> Add support for parsing IORT table to initialize SMMU devices.
> * The code for creating an SMMU device has been modified, so that the SMMU
> device can be initialized.
> * The NAMED NODE code has been commented out as this will need DOM0 kernel
> support.
> * ITS code has been included but it has not been tested.
> 
> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
> ---
>   xen/arch/arm/setup.c               |   3 +
>   xen/drivers/acpi/Makefile          |   1 +
>   xen/drivers/acpi/arm/Makefile      |   1 +
>   xen/drivers/acpi/arm/iort.c        | 173 +++++++++++++++++++++----------------
>   xen/drivers/passthrough/arm/smmu.c |   1 +
>   xen/include/acpi/acpi_iort.h       |  17 ++--
>   xen/include/asm-arm/device.h       |   2 +
>   xen/include/xen/acpi.h             |  21 +++++
>   xen/include/xen/pci.h              |   8 ++
>   9 files changed, 146 insertions(+), 81 deletions(-)
>   create mode 100644 xen/drivers/acpi/arm/Makefile
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 92f173b..4ba09b2 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -49,6 +49,7 @@
>   #include <asm/setup.h>
>   #include <xsm/xsm.h>
>   #include <asm/acpi.h>
> +#include <acpi/acpi_iort.h>
>   
>   struct bootinfo __initdata bootinfo;
>   
> @@ -796,6 +797,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>   
>       tasklet_subsys_init();
>   
> +    /* Parse the ACPI iort data */
> +    acpi_iort_init();
>   
>       xsm_dt_init();
>   
> diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
> index 444b11d..e7ffd82 100644
> --- a/xen/drivers/acpi/Makefile
> +++ b/xen/drivers/acpi/Makefile
> @@ -1,5 +1,6 @@
>   subdir-y += tables
>   subdir-y += utilities
> +subdir-$(CONFIG_ARM) += arm
>   subdir-$(CONFIG_X86) += apei
>   
>   obj-bin-y += tables.init.o
> diff --git a/xen/drivers/acpi/arm/Makefile b/xen/drivers/acpi/arm/Makefile
> new file mode 100644
> index 0000000..7c039bb
> --- /dev/null
> +++ b/xen/drivers/acpi/arm/Makefile
> @@ -0,0 +1 @@
> +obj-y += iort.o
> diff --git a/xen/drivers/acpi/arm/iort.c b/xen/drivers/acpi/arm/iort.c
> index 2e368a6..7f54062 100644
> --- a/xen/drivers/acpi/arm/iort.c
> +++ b/xen/drivers/acpi/arm/iort.c
> @@ -14,17 +14,47 @@
>    * This file implements early detection/parsing of I/O mapping
>    * reported to OS through firmware via I/O Remapping Table (IORT)
>    * IORT document number: ARM DEN 0049A
> + *
> + * Based on Linux drivers/acpi/arm64/iort.c
> + * => commit ca78d3173cff3503bcd15723b049757f75762d15
> + *
> + * Xen modification:
> + * Sameer Goel <sgoel@codeaurora.org>
> + * Copyright (C) 2017, The Linux Foundation, All rights reserved.
> + *
>    */
>   
> -#define pr_fmt(fmt)	"ACPI: IORT: " fmt
> -
> -#include <linux/acpi_iort.h>
> -#include <linux/iommu.h>
> -#include <linux/kernel.h>
> -#include <linux/list.h>
> -#include <linux/pci.h>
> -#include <linux/platform_device.h>
> -#include <linux/slab.h>
> +#include <xen/acpi.h>
> +#include <acpi/acpi_iort.h>

Why do you need to include there? Can't this be done after all the <xen/> ?

> +#include <xen/fwnode.h>
> +#include <xen/iommu.h>
> +#include <xen/lib.h>
> +#include <xen/list.h>
> +#include <xen/pci.h>
> +
> +#include <asm/device.h>
> +
> +/* Xen: Define compatibility functions */
> +#define FW_BUG		"[Firmware Bug]: "
> +#define pr_err(fmt, ...) printk(XENLOG_ERR fmt, ## __VA_ARGS__)
> +#define pr_warn(fmt, ...) printk(XENLOG_WARNING fmt, ## __VA_ARGS__)
> +
> +/* Alias to Xen allocation helpers */
> +#define kfree xfree
> +#define kmalloc(size, flags)            _xmalloc(size, sizeof(void *))
> +#define kzalloc(size, flags)            _xzalloc(size, sizeof(void *))

Likely you would need the same macros in the SMMUv3 driver. Could we 
think of a common headers implementing the Linux compat layer?

> +
> +/* Redefine WARN macros */
> +#undef WARN
> +#undef WARN_ON
> +#define WARN(condition, format...) ({					\
> +	int __ret_warn_on = !!(condition);				\
> +	if (unlikely(__ret_warn_on))					\
> +		printk(format);						\
> +	unlikely(__ret_warn_on);					\
> +})

Again, you should at least try to modify the common code version before 
deciding to redefine it here.

> +#define WARN_TAINT(cond, taint, format...) WARN(cond, format)
> +#define WARN_ON(cond)                      (!!cond)
>   
>   #define IORT_TYPE_MASK(type)	(1 << (type))
>   #define IORT_MSI_TYPE		(1 << ACPI_IORT_NODE_ITS_GROUP)
> @@ -256,6 +286,13 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
>   	acpi_status status;
>   
>   	if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT) {
> +		status = AE_NOT_IMPLEMENTED;
> +/*
> + * We need the namespace object name from dsdt to match the iort node, this

Please add a "Xen: TODO:" in front.

> + * will need additions to the kernel xen bus notifiers.
> + * So, disabling the named node code till a proposal is approved.
> + */
> +#if 0
>   		struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
>   		struct acpi_device *adev = to_acpi_device_node(dev->fwnode);
>   		struct acpi_iort_named_component *ncomp;
> @@ -275,11 +312,12 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
>   		status = !strcmp(ncomp->device_name, buf.pointer) ?
>   							AE_OK : AE_NOT_FOUND;
>   		acpi_os_free(buf.pointer);
> +#endif
>   	} else if (node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
>   		struct acpi_iort_root_complex *pci_rc;
> -		struct pci_bus *bus;
> +		struct pci_dev *pci_dev;

Do you really need to modify the code? Wouldn't it be possible to do

#define pci_bus pci_dev

With an explanation why you do that on top.

>   
> -		bus = to_pci_bus(dev);
> +		pci_dev = to_pci_dev(dev);

Same here?

>   		pci_rc = (struct acpi_iort_root_complex *)node->node_data;
>   
>   		/*
> @@ -287,12 +325,11 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
>   		 * with root complexes. Each segment number can represent only
>   		 * one root complex.
>   		 */
> -		status = pci_rc->pci_segment_number == pci_domain_nr(bus) ?
> +		status = pci_rc->pci_segment_number == pci_domain_nr(pci_dev) ?
>   							AE_OK : AE_NOT_FOUND;
>   	} else {
>   		status = AE_NOT_FOUND;
>   	}
> -out:
>   	return status;
>   }
>   
> @@ -320,6 +357,11 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>   	return 0;
>   }
>   
> +/*
> + * Named components are not supported yet so we do not need the
> + * iort_node_get_id function

Missing full stop + TODO.

> + */
> +#if 0
>   static
>   struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>   					u32 *id_out, u8 type_mask,
> @@ -358,6 +400,7 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>   
>   	return NULL;
>   }
> +#endif
>   
>   static struct acpi_iort_node *iort_node_map_rid(struct acpi_iort_node *node,
>   						u32 rid_in, u32 *rid_out,
> @@ -410,6 +453,10 @@ fail_map:
>   	return NULL;
>   }
>   
> +/* Xen: Comment out the NamedComponent and ITS mapping code till the support

+ TODO here please.

> + * is available.
> + */
> +#if 0
>   static struct acpi_iort_node *iort_find_dev_node(struct device *dev)
>   {
>   	struct pci_bus *pbus;
> @@ -481,7 +528,7 @@ static int iort_dev_find_its_id(struct device *dev, u32 req_id,
>   	return 0;
>   }
>   
> -/**
> +/*

Why this change?

>    * iort_get_device_domain() - Find MSI domain related to a device
>    * @dev: The device.
>    * @req_id: Requester ID for the device.
> @@ -510,7 +557,7 @@ static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
>   	*rid = alias;
>   	return 0;
>   }
> -
> +#endif

Please avoid dropping newline.

>   static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>   			       struct fwnode_handle *fwnode,
>   			       const struct iommu_ops *ops)
> @@ -546,6 +593,7 @@ static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
>   	return ret ? NULL : ops;
>   }
>   
> +#if 0 /* Xen: We do not need this function for Xen */
>   /**
>    * iort_set_dma_mask - Set-up dma mask for a device.
>    *
> @@ -567,7 +615,7 @@ void iort_set_dma_mask(struct device *dev)
>   	if (!dev->dma_mask)
>   		dev->dma_mask = &dev->coherent_dma_mask;
>   }
> -
> +#endif

Same here.

>   /**
>    * iort_iommu_configure - Set-up IOMMU configuration for a device.
>    *
> @@ -583,14 +631,13 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
>   	u32 streamid = 0;
>   
>   	if (dev_is_pci(dev)) {
> -		struct pci_bus *bus = to_pci_dev(dev)->bus;
> +		struct pci_dev *pci_device = to_pci_dev(dev);

See above.

>   		u32 rid;
>   
> -		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
> -				       &rid);
> +		rid = PCI_BDF2(pci_device->bus, pci_device->devfn);

I believe we had a discussion on v1 explaining why this is wrong. So I 
don't understand why

>   
>   		node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
> -				      iort_match_node_callback, &bus->dev);
> +				      iort_match_node_callback, dev);
>   		if (!node)
>   			return NULL;
>   
> @@ -600,6 +647,13 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
>   		ops = iort_iommu_xlate(dev, parent, streamid);
>   
>   	} else {
> +		return NULL;
> +/*
> + * We need the namespace object name from dsdt to match the iort node, this

Xen:

> + * will need additions to the kernel xen bus notifiers.
> + * So, disabling the named node code till a proposal is approved.
> + */
> +#if 0
>   		int i = 0;
>   
>   		node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> @@ -616,11 +670,17 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
>   			parent = iort_node_get_id(node, &streamid,
>   						  IORT_IOMMU_TYPE, i++);
>   		}
> +#endif
>   	}
>   
>   	return ops;
>   }
>   
> +/*
> + * Xen: Not using the parsing ops for now. Need to check and see if it will
> + * be useful to use these in some form, or let the driver parse IORT node.
> + */
> +#if 0
>   static void __init acpi_iort_register_irq(int hwirq, const char *name,
>   					  int trigger,
>   					  struct resource *res)
> @@ -807,7 +867,7 @@ const struct iort_iommu_config *iort_get_iommu_cfg(struct acpi_iort_node *node)
>   		return NULL;
>   	}
>   }
> -
> +#endif

Please avoid dropping newline.

>   /**
>    * iort_add_smmu_platform_device() - Allocate a platform device for SMMU
>    * @node: Pointer to SMMU ACPI IORT node
> @@ -817,78 +877,42 @@ const struct iort_iommu_config *iort_get_iommu_cfg(struct acpi_iort_node *node)
>   static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)

Looking at the changes in this function. You basically rewrite 
everything. I would prefer if you comment the current one and implement 
from scratch the Xen version.

>   {
>   	struct fwnode_handle *fwnode;
> -	struct platform_device *pdev;
> -	struct resource *r;
> -	enum dev_dma_attr attr;
> -	int ret, count;
> -	const struct iort_iommu_config *ops = iort_get_iommu_cfg(node);
> -
> -	if (!ops)
> -		return -ENODEV;
> -
> -	pdev = platform_device_alloc(ops->name, PLATFORM_DEVID_AUTO);
> -	if (!pdev)
> -		return -ENOMEM;
> -
> -	count = ops->iommu_count_resources(node);
> -
> -	r = kcalloc(count, sizeof(*r), GFP_KERNEL);
> -	if (!r) {
> -		ret = -ENOMEM;
> -		goto dev_put;
> -	}
> -
> -	ops->iommu_init_resources(r, node);
> +	struct device *dev;
> +	int ret;
>   
> -	ret = platform_device_add_resources(pdev, r, count);
>   	/*
> -	 * Resources are duplicated in platform_device_add_resources,
> -	 * free their allocated memory
> +	 * Not enabling the parsing ops for now. The corresponding driver
> +	 * can parse this information as needed, so deleting relevant code as
> +	 * compared to base revision.
>   	 */
> -	kfree(r);
>   
> -	if (ret)
> -		goto dev_put;
> +	dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
>   
>   	/*
>   	 * Add a copy of IORT node pointer to platform_data to
>   	 * be used to retrieve IORT data information.
>   	 */
> -	ret = platform_device_add_data(pdev, &node, sizeof(node));
> -	if (ret)
> -		goto dev_put;
> -
> -	/*
> -	 * We expect the dma masks to be equivalent for
> -	 * all SMMUs set-ups
> -	 */
> -	pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> +	dev->type = DEV_ACPI;
> +	dev->acpi_node = node;
>   
>   	fwnode = iort_get_fwnode(node);
>   
>   	if (!fwnode) {
>   		ret = -ENODEV;
> -		goto dev_put;
> +		goto error;
>   	}
>   
> -	pdev->dev.fwnode = fwnode;
> -
> -	attr = ops->iommu_is_coherent(node) ?
> -			     DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT;
> -
> -	/* Configure DMA for the page table walker */
> -	acpi_dma_configure(&pdev->dev, attr);
> +	dev->fwnode = fwnode;
>   
> -	ret = platform_device_add(pdev);
> -	if (ret)
> -		goto dma_deconfigure;
> +	/* Call the acpi init functions for IOMMU devices */
> +	ret = acpi_device_init(DEVICE_IOMMU, (void *)dev, node->type);
>   
>   	return 0;
>   
> -dma_deconfigure:
> -	acpi_dma_deconfigure(&pdev->dev);
> -dev_put:
> -	platform_device_put(pdev);
> +error:
> +	kfree(dev);
>   
>   	return ret;
>   }
> @@ -957,5 +981,6 @@ void __init acpi_iort_init(void)
>   
>   	iort_init_platform_devices();
>   
> -	acpi_probe_device_table(iort);
> +	/* Xen; Do not need a device table probe */
> +	/* acpi_probe_device_table(iort);*/

Please use either

#if 0

#endif

or introduce a dummy acpi_probe_device_table(...) at the start.

>   }
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 362d578..ad956d5 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -181,6 +181,7 @@ static void __iomem *devm_ioremap_resource(struct device *dev,
>    * Xen: PCI functions
>    * TODO: It should be implemented when PCI will be supported
>    */
> +#undef to_pci_dev

Why this change?

>   #define to_pci_dev(dev)	(NULL)
>   static inline int pci_for_each_dma_alias(struct pci_dev *pdev,
>   					 int (*fn) (struct pci_dev *pdev,
> diff --git a/xen/include/acpi/acpi_iort.h b/xen/include/acpi/acpi_iort.h
> index 77e0809..d4315a4 100644
> --- a/xen/include/acpi/acpi_iort.h
> +++ b/xen/include/acpi/acpi_iort.h

You probably want to re-sync this headers as it changed quite a bit and 
would avoid some specific #if 0 for Xen.

> @@ -19,27 +19,32 @@
>   #ifndef __ACPI_IORT_H__
>   #define __ACPI_IORT_H__
>   
> -#include <linux/acpi.h>
> -#include <linux/fwnode.h>
> -#include <linux/irqdomain.h>
> +#include <xen/acpi.h>
> +#include <asm/device.h>
>   
> +/* Xen: Not using IORT IRQ bindings */
> +#if 0
>   #define IORT_IRQ_MASK(irq)		(irq & 0xffffffffULL)
>   #define IORT_IRQ_TRIGGER_MASK(irq)	((irq >> 32) & 0xffffffffULL)
>   
>   int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node);
>   void iort_deregister_domain_token(int trans_id);
>   struct fwnode_handle *iort_find_domain_token(int trans_id);
> -#ifdef CONFIG_ACPI_IORT
> +#endif
> +#ifdef CONFIG_ARM_64

As said in the first version, I see no point of replacing 
CONFIG_ACPI_IORT with CONFIG_ARM_64. You should instead take advantage 
of the Kconfig to add a new config ACPI_IORT and select on Arm64 with ACPI.

>   void acpi_iort_init(void);
>   bool iort_node_match(u8 type);
> +#if 0
>   u32 iort_msi_map_rid(struct device *dev, u32 req_id);
>   struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
>   /* IOMMU interface */
>   void iort_set_dma_mask(struct device *dev);
> +#endif
>   const struct iommu_ops *iort_iommu_configure(struct device *dev);
>   #else
>   static inline void acpi_iort_init(void) { }
>   static inline bool iort_node_match(u8 type) { return false; }
> +#if 0
>   static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
>   { return req_id; }
>   static inline struct irq_domain *iort_get_device_domain(struct device *dev,
> @@ -47,12 +52,10 @@ static inline struct irq_domain *iort_get_device_domain(struct device *dev,
>   { return NULL; }
>   /* IOMMU interface */
>   static inline void iort_set_dma_mask(struct device *dev) { }
> +#endif
>   static inline
>   const struct iommu_ops *iort_iommu_configure(struct device *dev)
>   { return NULL; }
>   #endif
>   
> -#define IORT_ACPI_DECLARE(name, table_id, fn)		\
> -	ACPI_DECLARE_PROBE_ENTRY(iort, name, table_id, 0, NULL, 0, fn)
> -
>   #endif /* __ACPI_IORT_H__ */
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 5027c87..4eef9ce 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -7,6 +7,7 @@
>   enum device_type
>   {
>       DEV_DT,
> +    DEV_ACPI,
>   };
>   
>   struct dev_archdata {
> @@ -20,6 +21,7 @@ struct device
>   #ifdef CONFIG_HAS_DEVICE_TREE
>       struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>   #endif
> +    void *acpi_node; /*Current use case is acpi_iort_node */

Can you explain why you need that? After the creation of fwnode, I was 
expecting of_node to disappear. So I don't really fancy see acpi_node 
here more it does not exist in Linux.

>       struct fwnode_handle *fwnode; /*fw device node identifier */
>       struct iommu_fwspec *iommu_fwspec;
>       struct dev_archdata archdata;
> diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
> index 9409350..2f6aae1 100644
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -32,6 +32,7 @@
>   
>   #include <acpi/acpi.h>
>   #include <asm/acpi.h>
> +#include <xen/fwnode.h>

I think this and ...

>   
>   #define ACPI_MADT_GET_(fld, x) (((x) & ACPI_MADT_##fld##_MASK) / \
>   	(ACPI_MADT_##fld##_MASK & -ACPI_MADT_##fld##_MASK))
> @@ -49,6 +50,26 @@
>                   (!(entry)) || (unsigned long)(entry) + sizeof(*(entry)) > (end) ||  \
>                   (entry)->header.length < sizeof(*(entry)))
>   
> +static inline struct fwnode_handle *acpi_alloc_fwnode_static(void)
> +{
> +	struct fwnode_handle *fwnode;
> +
> +	fwnode = xzalloc(struct fwnode_handle);
> +	if (!fwnode)
> +		return NULL;
> +
> +	fwnode->type = FWNODE_ACPI_STATIC;
> +
> +	return fwnode;
> +}
> +
> +static inline void acpi_free_fwnode_static(struct fwnode_handle *fwnode)
> +{
> +	if (!fwnode || fwnode->type != FWNODE_ACPI_STATIC)
> +		return;
> +
> +	xfree(fwnode);
> +}

... those 2 helpers should go in asm/acpi.h.

>   #ifdef CONFIG_ACPI
>   
>   enum acpi_interrupt_id {
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 43f2125..182b1a5 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -92,8 +92,16 @@ struct pci_dev {
>   #define PT_FAULT_THRESHOLD 10
>       } fault;
>       u64 vf_rlen[6];
> +#ifdef CONFIG_ARM
> +    struct device dev;
> +#endif

There are a part of PCI that is already per-arch. See arch_pci_dev in 
asm-arm/pci.h.

Please define the field dev in it rather than here.

>   };
>   
> +#ifdef CONFIG_ARM
> +#define to_pci_dev(p) container_of(p, struct pci_dev,dev)
> +#define pci_domain_nr(dev) dev->seg
> +#endif

Similarly, this should be moved in asm-arm/pci.h.

> +
>   #define for_each_pdev(domain, pdev) \
>       list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
>   
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel