[RFC 0/6] IORT support and introduce fwspec

[RFC 0/6] IORT support and introduce fwspec

[Sameer Goel]

This changelist is in preparation for porting the latest SMMUv3 driver from
Linux kernel  4.11 release.

Scope of the changes:
- Introduce the iommu_fwspec implementation
    * This implementation is a direct port from Linux. The code that is not
      needed for Xen is removed.
- IORT port from Linux. The differences are as under:
    * The DMA ops are removed.
    * Modified the code for creating the SMMU devices. This also initializes
      the discoverd SMMU devices.
    * MSI code is commented out till the MSI framework API usage is clearer.
    * IORT node data parsing is delegated to the driver. Looking for comments
      on enabling the code in IORT driver. This will need a standard resource
      object. (Direct port from Linux or a new define for Xen?)
    * Assumptions on PCI IORT SMMU interaction. PCI assign device will call
      iort_iommu_configure to setup the streamids.Then it will call SMMU
      assign device with the right struct device argument.

Sameer Goel (6):
  passthrough/arm: Modify SMMU driver to use generic device definition
  arm64: Add definitions for fwnode_handle
  Introduce _xrealloc
  xen/passthrough/arm: Introduce iommu_fwspec
  ACPI: arm: Support for IORT
  acpi:arm64: Add support for parsing IORT table

 xen/arch/arm/setup.c                |   3 +
 xen/common/xmalloc_tlsf.c           |  13 +
 xen/drivers/acpi/Makefile           |   1 +
 xen/drivers/acpi/arm/Makefile       |   1 +
 xen/drivers/acpi/arm/iort.c         | 977 ++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/arm/iommu.c |  58 +++
 xen/drivers/passthrough/arm/smmu.c  |  12 +-
 xen/include/acpi/acpi.h             |   1 +
 xen/include/acpi/acpi_iort.h        |  65 +++
 xen/include/asm-arm/device.h        |   5 +
 xen/include/xen/acpi.h              |  21 +
 xen/include/xen/fwnode.h            |  35 ++
 xen/include/xen/iommu.h             |  28 ++
 xen/include/xen/lib.h               |   7 +-
 xen/include/xen/pci.h               |   1 +
 xen/include/xen/xmalloc.h           |   1 +
 16 files changed, 1222 insertions(+), 7 deletions(-)
 create mode 100644 xen/drivers/acpi/arm/Makefile
 create mode 100644 xen/drivers/acpi/arm/iort.c
 create mode 100644 xen/include/acpi/acpi_iort.h
 create mode 100644 xen/include/xen/fwnode.h

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[RFC 1/6] passthrough/arm: Modify SMMU driver to use generic device definition

[Sameer Goel]

Modify the SMMU code to use generic device instead of dt_device_node for
functions that can be used for ACPI based systems too.

Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
---
 xen/drivers/passthrough/arm/smmu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 1082fcf..a298661 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -76,7 +76,7 @@ struct resource
 
 #define resource_size(res) (res)->size;
 
-#define platform_device dt_device_node
+#define platform_device device
 
 #define IORESOURCE_MEM 0
 #define IORESOURCE_IRQ 1
@@ -97,12 +97,12 @@ static struct resource *platform_get_resource(struct platform_device *pdev,
 
 	switch (type) {
 	case IORESOURCE_MEM:
-		ret = dt_device_get_address(pdev, num, &res.addr, &res.size);
+		ret = dt_device_get_address(dev_to_dt(pdev), num, &res.addr, &res.size);
 
 		return ((ret) ? NULL : &res);
 
 	case IORESOURCE_IRQ:
-		ret = platform_get_irq(pdev, num);
+		ret = platform_get_irq(dev_to_dt(pdev), num);
 		if (ret < 0)
 			return NULL;
 
@@ -2285,7 +2285,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	const struct of_device_id *of_id;
 	struct resource *res;
 	struct arm_smmu_device *smmu;
-	struct device *dev = &pdev->dev;
+	struct device *dev = pdev;
 	struct rb_node *node;
 	struct of_phandle_args masterspec;
 	int num_irqs, i, err;
@@ -2338,7 +2338,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < num_irqs; ++i) {
-		int irq = platform_get_irq(pdev, i);
+		int irq = platform_get_irq(dev_to_dt(pdev), i);
 
 		if (irq < 0) {
 			dev_err(dev, "failed to get irq index %d\n", i);
@@ -2821,7 +2821,7 @@ static __init int arm_smmu_dt_init(struct dt_device_node *dev,
 	 */
 	dt_device_set_used_by(dev, DOMID_XEN);
 
-	rc = arm_smmu_device_dt_probe(dev);
+	rc = arm_smmu_device_dt_probe(dt_to_dev(dev));
 	if (rc)
 		return rc;
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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

Re: [RFC 1/6] passthrough/arm: Modify SMMU driver to use generic device definition

[Julien Grall]

Hi Sameer,

On 08/06/17 20:30, Sameer Goel wrote:
> Modify the SMMU code to use generic device instead of dt_device_node for
> functions that can be used for ACPI based systems too.

I don't think this patch series is sufficient to get ACPI support in the 
SMMU. I would prefer to wait and see the full support before giving an 
ack on this patch.

Cheers,

-- 
Julien Grall

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

[RFC 2/6] arm64: Add definitions for fwnode_handle

[Sameer Goel]

This will be used as a device property to match the DMA capable devices
with the associated SMMU. The header file is a port from linux.

Linux ChangeId:ce793486e23e: driver core / ACPI: Represent ACPI
companions using fwnode_handle

Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
---
 xen/include/asm-arm/device.h |  2 ++
 xen/include/xen/fwnode.h     | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 xen/include/xen/fwnode.h

diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 6734ae8..78c38fe 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -2,6 +2,7 @@
 #define __ASM_ARM_DEVICE_H
 
 #include <xen/init.h>
+#include <xen/fwnode.h>
 
 enum device_type
 {
@@ -19,6 +20,7 @@ struct device
 #ifdef CONFIG_HAS_DEVICE_TREE
     struct dt_device_node *of_node; /* Used by drivers imported from Linux */
 #endif
+    struct fwnode_handle *fwnode; /*fw device node identifier */
     struct dev_archdata archdata;
 };
 
diff --git a/xen/include/xen/fwnode.h b/xen/include/xen/fwnode.h
new file mode 100644
index 0000000..db65b15
--- /dev/null
+++ b/xen/include/xen/fwnode.h
@@ -0,0 +1,35 @@
+/*
+ * fwnode.h - Firmware device node object handle type definition.
+ *
+ * Copyright (C) 2015, Intel Corporation
+ * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Ported from Linux include/linux/fwnode.h
+ *  => commit ce793486e23e0162a732c605189c8028e0910e86
+ *
+ * No functional Xen modifications.
+ */
+
+#ifndef __XEN_FWNODE_H_
+#define __XEN_FWNODE_H_
+
+enum fwnode_type {
+	FWNODE_INVALID = 0,
+	FWNODE_OF,
+	FWNODE_ACPI,
+	FWNODE_ACPI_DATA,
+	FWNODE_ACPI_STATIC,
+	FWNODE_PDATA,
+	FWNODE_IRQCHIP
+};
+
+struct fwnode_handle {
+	enum fwnode_type type;
+	struct fwnode_handle *secondary;
+};
+
+#endif
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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

Re: [RFC 2/6] arm64: Add definitions for fwnode_handle

[Julien Grall]

Hi,

Please CC all the relevant maintainers on this patch.

On 08/06/2017 20:30, Sameer Goel wrote:
> This will be used as a device property to match the DMA capable devices
> with the associated SMMU. The header file is a port from linux.
>
> Linux ChangeId:ce793486e23e: driver core / ACPI: Represent ACPI
> companions using fwnode_handle
>
> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
> ---
>  xen/include/asm-arm/device.h |  2 ++
>  xen/include/xen/fwnode.h     | 35 +++++++++++++++++++++++++++++++++++

I am not totally convinced this is the right place for this header. But 
I will leave the REST maintainers deciding.

>  2 files changed, 37 insertions(+)
>  create mode 100644 xen/include/xen/fwnode.h
>
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 6734ae8..78c38fe 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -2,6 +2,7 @@
>  #define __ASM_ARM_DEVICE_H
>
>  #include <xen/init.h>
> +#include <xen/fwnode.h>
>
>  enum device_type
>  {
> @@ -19,6 +20,7 @@ struct device
>  #ifdef CONFIG_HAS_DEVICE_TREE
>      struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>  #endif
> +    struct fwnode_handle *fwnode; /*fw device node identifier */
>      struct dev_archdata archdata;
>  };
>
> diff --git a/xen/include/xen/fwnode.h b/xen/include/xen/fwnode.h
> new file mode 100644
> index 0000000..db65b15
> --- /dev/null
> +++ b/xen/include/xen/fwnode.h
> @@ -0,0 +1,35 @@
> +/*
> + * fwnode.h - Firmware device node object handle type definition.
> + *
> + * Copyright (C) 2015, Intel Corporation
> + * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Ported from Linux include/linux/fwnode.h
> + *  => commit ce793486e23e0162a732c605189c8028e0910e86
> + *
> + * No functional Xen modifications.
> + */
> +
> +#ifndef __XEN_FWNODE_H_
> +#define __XEN_FWNODE_H_
> +
> +enum fwnode_type {
> +	FWNODE_INVALID = 0,
> +	FWNODE_OF,
> +	FWNODE_ACPI,
> +	FWNODE_ACPI_DATA,
> +	FWNODE_ACPI_STATIC,
> +	FWNODE_PDATA,
> +	FWNODE_IRQCHIP
> +};
> +
> +struct fwnode_handle {
> +	enum fwnode_type type;
> +	struct fwnode_handle *secondary;
> +};
> +
> +#endif
>

-- 
Julien Grall

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

Re: [RFC 2/6] arm64: Add definitions for fwnode_handle

[Julien Grall]

On 08/06/2017 20:30, Sameer Goel wrote:
> This will be used as a device property to match the DMA capable devices
> with the associated SMMU. The header file is a port from linux.
>
> Linux ChangeId:ce793486e23e: driver core / ACPI: Represent ACPI
> companions using fwnode_handle
>
> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
> ---
>  xen/include/asm-arm/device.h |  2 ++
>  xen/include/xen/fwnode.h     | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
>  create mode 100644 xen/include/xen/fwnode.h
>
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 6734ae8..78c38fe 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -2,6 +2,7 @@
>  #define __ASM_ARM_DEVICE_H
>
>  #include <xen/init.h>
> +#include <xen/fwnode.h>
>
>  enum device_type
>  {
> @@ -19,6 +20,7 @@ struct device
>  #ifdef CONFIG_HAS_DEVICE_TREE
>      struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>  #endif
> +    struct fwnode_handle *fwnode; /*fw device node identifier */

I am a bit surprised you don't rework struct dev. As of_node is now 
redundant with fwnode.

>      struct dev_archdata archdata;
>  };
>
> diff --git a/xen/include/xen/fwnode.h b/xen/include/xen/fwnode.h
> new file mode 100644
> index 0000000..db65b15
> --- /dev/null
> +++ b/xen/include/xen/fwnode.h
> @@ -0,0 +1,35 @@
> +/*
> + * fwnode.h - Firmware device node object handle type definition.
> + *
> + * Copyright (C) 2015, Intel Corporation
> + * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Ported from Linux include/linux/fwnode.h
> + *  => commit ce793486e23e0162a732c605189c8028e0910e86
> + *
> + * No functional Xen modifications.
> + */
> +
> +#ifndef __XEN_FWNODE_H_
> +#define __XEN_FWNODE_H_
> +
> +enum fwnode_type {
> +	FWNODE_INVALID = 0,
> +	FWNODE_OF,
> +	FWNODE_ACPI,
> +	FWNODE_ACPI_DATA,
> +	FWNODE_ACPI_STATIC,
> +	FWNODE_PDATA,
> +	FWNODE_IRQCHIP

Do you really need to introduce all of them?

> +};
> +
> +struct fwnode_handle {
> +	enum fwnode_type type;
> +	struct fwnode_handle *secondary;
> +};
> +
> +#endif
>

Cheers,

-- 
Julien Grall

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

Re: [RFC 2/6] arm64: Add definitions for fwnode_handle

[Goel, Sameer]

On 6/8/2017 1:59 PM, Julien Grall wrote:
> 
> 
> On 08/06/2017 20:30, Sameer Goel wrote:
>> This will be used as a device property to match the DMA capable devices
>> with the associated SMMU. The header file is a port from linux.
>>
>> Linux ChangeId:ce793486e23e: driver core / ACPI: Represent ACPI
>> companions using fwnode_handle
>>
>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>> ---
>>  xen/include/asm-arm/device.h |  2 ++
>>  xen/include/xen/fwnode.h     | 35 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 37 insertions(+)
>>  create mode 100644 xen/include/xen/fwnode.h
>>
>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>> index 6734ae8..78c38fe 100644
>> --- a/xen/include/asm-arm/device.h
>> +++ b/xen/include/asm-arm/device.h
>> @@ -2,6 +2,7 @@
>>  #define __ASM_ARM_DEVICE_H
>>
>>  #include <xen/init.h>
>> +#include <xen/fwnode.h>
>>
>>  enum device_type
>>  {
>> @@ -19,6 +20,7 @@ struct device
>>  #ifdef CONFIG_HAS_DEVICE_TREE
>>      struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>>  #endif
>> +    struct fwnode_handle *fwnode; /*fw device node identifier */
> 
> I am a bit surprised you don't rework struct dev. As of_node is now redundant with fwnode.

I agree that this will eventually be removed. I have kept this in now just to maintain compatibility
(compilation and otherwise) with smmuv2 driver. I will add a comment to indicate this. So that it can 
be easily identified and remove when we do a final cleanup. Can I prefix the comment with with XEN:TODO:? 

> 
>>      struct dev_archdata archdata;
>>  };
>>
>> diff --git a/xen/include/xen/fwnode.h b/xen/include/xen/fwnode.h
>> new file mode 100644
>> index 0000000..db65b15
>> --- /dev/null
>> +++ b/xen/include/xen/fwnode.h
>> @@ -0,0 +1,35 @@
>> +/*
>> + * fwnode.h - Firmware device node object handle type definition.
>> + *
>> + * Copyright (C) 2015, Intel Corporation
>> + * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * Ported from Linux include/linux/fwnode.h
>> + *  => commit ce793486e23e0162a732c605189c8028e0910e86
>> + *
>> + * No functional Xen modifications.
>> + */
>> +
>> +#ifndef __XEN_FWNODE_H_
>> +#define __XEN_FWNODE_H_
>> +
>> +enum fwnode_type {
>> +    FWNODE_INVALID = 0,
>> +    FWNODE_OF,
>> +    FWNODE_ACPI,
>> +    FWNODE_ACPI_DATA,
>> +    FWNODE_ACPI_STATIC,
>> +    FWNODE_PDATA,
>> +    FWNODE_IRQCHIP
> 
> Do you really need to introduce all of them?
> 
Not really. We are interested in OF and ACPI_STATIC for now. Since the verbatim file from Linux applied ok, I did not remove the other entries.
What's your recommendation?

>> +};
>> +
>> +struct fwnode_handle {
>> +    enum fwnode_type type;
>> +    struct fwnode_handle *secondary;
>> +};
>> +
>> +#endif
>>
> 
> Cheers,
> 

Thanks,
Sameer
-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

Re: [RFC 2/6] arm64: Add definitions for fwnode_handle

[Stefano Stabellini]

On Thu, 8 Jun 2017, Goel, Sameer wrote:
> >> diff --git a/xen/include/xen/fwnode.h b/xen/include/xen/fwnode.h
> >> new file mode 100644
> >> index 0000000..db65b15
> >> --- /dev/null
> >> +++ b/xen/include/xen/fwnode.h
> >> @@ -0,0 +1,35 @@
> >> +/*
> >> + * fwnode.h - Firmware device node object handle type definition.
> >> + *
> >> + * Copyright (C) 2015, Intel Corporation
> >> + * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * Ported from Linux include/linux/fwnode.h
> >> + *  => commit ce793486e23e0162a732c605189c8028e0910e86
> >> + *
> >> + * No functional Xen modifications.
> >> + */
> >> +
> >> +#ifndef __XEN_FWNODE_H_
> >> +#define __XEN_FWNODE_H_
> >> +
> >> +enum fwnode_type {
> >> +    FWNODE_INVALID = 0,
> >> +    FWNODE_OF,
> >> +    FWNODE_ACPI,
> >> +    FWNODE_ACPI_DATA,
> >> +    FWNODE_ACPI_STATIC,
> >> +    FWNODE_PDATA,
> >> +    FWNODE_IRQCHIP
> > 
> > Do you really need to introduce all of them?
> > 
> Not really. We are interested in OF and ACPI_STATIC for now. Since the verbatim file from Linux applied ok, I did not remove the other entries.
> What's your recommendation?

Usually we keep the imported Linux definitions as-is.


> >> +};
> >> +
> >> +struct fwnode_handle {
> >> +    enum fwnode_type type;
> >> +    struct fwnode_handle *secondary;
> >> +};
> >> +
> >> +#endif
> >>

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

Re: [RFC 2/6] arm64: Add definitions for fwnode_handle

[Julien Grall]

Hi,

On 08/06/17 22:57, Stefano Stabellini wrote:
> On Thu, 8 Jun 2017, Goel, Sameer wrote:
>>>> diff --git a/xen/include/xen/fwnode.h b/xen/include/xen/fwnode.h
>>>> new file mode 100644
>>>> index 0000000..db65b15
>>>> --- /dev/null
>>>> +++ b/xen/include/xen/fwnode.h
>>>> @@ -0,0 +1,35 @@
>>>> +/*
>>>> + * fwnode.h - Firmware device node object handle type definition.
>>>> + *
>>>> + * Copyright (C) 2015, Intel Corporation
>>>> + * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + *
>>>> + * Ported from Linux include/linux/fwnode.h
>>>> + *  => commit ce793486e23e0162a732c605189c8028e0910e86
>>>> + *
>>>> + * No functional Xen modifications.
>>>> + */
>>>> +
>>>> +#ifndef __XEN_FWNODE_H_
>>>> +#define __XEN_FWNODE_H_
>>>> +
>>>> +enum fwnode_type {
>>>> +    FWNODE_INVALID = 0,
>>>> +    FWNODE_OF,
>>>> +    FWNODE_ACPI,
>>>> +    FWNODE_ACPI_DATA,
>>>> +    FWNODE_ACPI_STATIC,
>>>> +    FWNODE_PDATA,
>>>> +    FWNODE_IRQCHIP
>>>
>>> Do you really need to introduce all of them?
>>>
>> Not really. We are interested in OF and ACPI_STATIC for now. Since the verbatim file from Linux applied ok, I did not remove the other entries.
>> What's your recommendation?
>
> Usually we keep the imported Linux definitions as-is.

If we keep as-is, the coding style should stay exactly the same. Even

Otherwise there is no point to import a verbatim copy of Linux as it 
would be difficult to keep track of the changes.

In this case, I am not convinced we have a benefits to keep this code 
close to Linux. It is small enough and we don't need 80% of it.

Cheers,

-- 
Julien Grall

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

Re: [RFC 2/6] arm64: Add definitions for fwnode_handle

[Goel, Sameer]

On 6/12/2017 6:40 AM, Julien Grall wrote:
> Hi,
> 
> On 08/06/17 22:57, Stefano Stabellini wrote:
>> On Thu, 8 Jun 2017, Goel, Sameer wrote:
>>>>> diff --git a/xen/include/xen/fwnode.h b/xen/include/xen/fwnode.h
>>>>> new file mode 100644
>>>>> index 0000000..db65b15
>>>>> --- /dev/null
>>>>> +++ b/xen/include/xen/fwnode.h
>>>>> @@ -0,0 +1,35 @@
>>>>> +/*
>>>>> + * fwnode.h - Firmware device node object handle type definition.
>>>>> + *
>>>>> + * Copyright (C) 2015, Intel Corporation
>>>>> + * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License version 2 as
>>>>> + * published by the Free Software Foundation.
>>>>> + *
>>>>> + * Ported from Linux include/linux/fwnode.h
>>>>> + *  => commit ce793486e23e0162a732c605189c8028e0910e86
>>>>> + *
>>>>> + * No functional Xen modifications.
>>>>> + */
>>>>> +
>>>>> +#ifndef __XEN_FWNODE_H_
>>>>> +#define __XEN_FWNODE_H_
>>>>> +
>>>>> +enum fwnode_type {
>>>>> +    FWNODE_INVALID = 0,
>>>>> +    FWNODE_OF,
>>>>> +    FWNODE_ACPI,
>>>>> +    FWNODE_ACPI_DATA,
>>>>> +    FWNODE_ACPI_STATIC,
>>>>> +    FWNODE_PDATA,
>>>>> +    FWNODE_IRQCHIP
>>>>
>>>> Do you really need to introduce all of them?
>>>>
>>> Not really. We are interested in OF and ACPI_STATIC for now. Since the verbatim file from Linux applied ok, I did not remove the other entries.
>>> What's your recommendation?
>>
>> Usually we keep the imported Linux definitions as-is.
> 
> If we keep as-is, the coding style should stay exactly the same. Even
> 
> Otherwise there is no point to import a verbatim copy of Linux as it would be difficult to keep track of the changes.
> 
> In this case, I am not convinced we have a benefits to keep this code close to Linux. It is small enough and we don't need 80% of it.
Ok. I will go ahead and remove whatever is not needed.
> 
> Cheers,
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

Re: [RFC 2/6] arm64: Add definitions for fwnode_handle

[Julien Grall]

Hi Sameer,

On 08/06/17 22:42, Goel, Sameer wrote:
> On 6/8/2017 1:59 PM, Julien Grall wrote:
>>
>>
>> On 08/06/2017 20:30, Sameer Goel wrote:
>>> This will be used as a device property to match the DMA capable devices
>>> with the associated SMMU. The header file is a port from linux.
>>>
>>> Linux ChangeId:ce793486e23e: driver core / ACPI: Represent ACPI
>>> companions using fwnode_handle
>>>
>>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>>> ---
>>>  xen/include/asm-arm/device.h |  2 ++
>>>  xen/include/xen/fwnode.h     | 35 +++++++++++++++++++++++++++++++++++
>>>  2 files changed, 37 insertions(+)
>>>  create mode 100644 xen/include/xen/fwnode.h
>>>
>>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>>> index 6734ae8..78c38fe 100644
>>> --- a/xen/include/asm-arm/device.h
>>> +++ b/xen/include/asm-arm/device.h
>>> @@ -2,6 +2,7 @@
>>>  #define __ASM_ARM_DEVICE_H
>>>
>>>  #include <xen/init.h>
>>> +#include <xen/fwnode.h>
>>>
>>>  enum device_type
>>>  {
>>> @@ -19,6 +20,7 @@ struct device
>>>  #ifdef CONFIG_HAS_DEVICE_TREE
>>>      struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>>>  #endif
>>> +    struct fwnode_handle *fwnode; /*fw device node identifier */
>>
>> I am a bit surprised you don't rework struct dev. As of_node is now redundant with fwnode.
>
> I agree that this will eventually be removed. I have kept this in now just to maintain compatibility
> (compilation and otherwise) with smmuv2 driver. I will add a comment to indicate this. So that it can
> be easily identified and remove when we do a final cleanup. Can I prefix the comment with with XEN:TODO:?

A TODO would be nice, but who is going to do the rework?

Cheers,

-- 
Julien Grall

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

Re: [RFC 2/6] arm64: Add definitions for fwnode_handle

[Goel, Sameer]

On 6/12/2017 6:51 AM, Julien Grall wrote:
> Hi Sameer,
> 
> On 08/06/17 22:42, Goel, Sameer wrote:
>> On 6/8/2017 1:59 PM, Julien Grall wrote:
>>>
>>>
>>> On 08/06/2017 20:30, Sameer Goel wrote:
>>>> This will be used as a device property to match the DMA capable devices
>>>> with the associated SMMU. The header file is a port from linux.
>>>>
>>>> Linux ChangeId:ce793486e23e: driver core / ACPI: Represent ACPI
>>>> companions using fwnode_handle
>>>>
>>>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>>>> ---
>>>>  xen/include/asm-arm/device.h |  2 ++
>>>>  xen/include/xen/fwnode.h     | 35 +++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 37 insertions(+)
>>>>  create mode 100644 xen/include/xen/fwnode.h
>>>>
>>>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>>>> index 6734ae8..78c38fe 100644
>>>> --- a/xen/include/asm-arm/device.h
>>>> +++ b/xen/include/asm-arm/device.h
>>>> @@ -2,6 +2,7 @@
>>>>  #define __ASM_ARM_DEVICE_H
>>>>
>>>>  #include <xen/init.h>
>>>> +#include <xen/fwnode.h>
>>>>
>>>>  enum device_type
>>>>  {
>>>> @@ -19,6 +20,7 @@ struct device
>>>>  #ifdef CONFIG_HAS_DEVICE_TREE
>>>>      struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>>>>  #endif
>>>> +    struct fwnode_handle *fwnode; /*fw device node identifier */
>>>
>>> I am a bit surprised you don't rework struct dev. As of_node is now redundant with fwnode.
>>
>> I agree that this will eventually be removed. I have kept this in now just to maintain compatibility
>> (compilation and otherwise) with smmuv2 driver. I will add a comment to indicate this. So that it can
>> be easily identified and remove when we do a final cleanup. Can I prefix the comment with with XEN:TODO:?
> 
> A TODO would be nice, but who is going to do the rework?
I will still be on the hook to complete this cleanup. I was hoping to get the basic code in place and then 
start the rework on older drivers.
> 
> Cheers,
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[RFC 3/6] Introduce _xrealloc

[Sameer Goel]

Introduce a memory realloc function.

Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
---
 xen/common/xmalloc_tlsf.c | 13 +++++++++++++
 xen/include/xen/xmalloc.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
index b256dc5..52385a8 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -612,6 +612,19 @@ void *_xzalloc(unsigned long size, unsigned long align)
     return p ? memset(p, 0, size) : p;
 }
 
+void *_xrealloc(void *p, unsigned long new_size, unsigned long align)
+{
+    void *new_p = _xmalloc(new_size, align);
+
+    if(new_p && p)
+    {
+        memcpy(new_p, p, new_size);
+        xfree(p);
+    }
+
+    return new_p;
+}
+
 void xfree(void *p)
 {
     struct bhdr *b;
diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
index 24a99ac..41a9b2f 100644
--- a/xen/include/xen/xmalloc.h
+++ b/xen/include/xen/xmalloc.h
@@ -29,6 +29,7 @@ extern void xfree(void *);
 /* Underlying functions */
 extern void *_xmalloc(unsigned long size, unsigned long align);
 extern void *_xzalloc(unsigned long size, unsigned long align);
+extern void *_xrealloc(void *p, unsigned long new_size, unsigned long align);
 
 static inline void *_xmalloc_array(
     unsigned long size, unsigned long align, unsigned long num)
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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

Re: [RFC 3/6] Introduce _xrealloc

[Julien Grall]

CC the REST maintainers

On 08/06/2017 20:30, Sameer Goel wrote:
> Introduce a memory realloc function.
>
> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
> ---
>  xen/common/xmalloc_tlsf.c | 13 +++++++++++++
>  xen/include/xen/xmalloc.h |  1 +
>  2 files changed, 14 insertions(+)
>
> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> index b256dc5..52385a8 100644
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -612,6 +612,19 @@ void *_xzalloc(unsigned long size, unsigned long align)
>      return p ? memset(p, 0, size) : p;
>  }
>
> +void *_xrealloc(void *p, unsigned long new_size, unsigned long align)
> +{
> +    void *new_p = _xmalloc(new_size, align);
> +
> +    if(new_p && p)

Coding style: if ( ... )

> +    {
> +        memcpy(new_p, p, new_size);
> +        xfree(p);
> +    }
> +
> +    return new_p;
> +}
> +
>  void xfree(void *p)
>  {
>      struct bhdr *b;
> diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
> index 24a99ac..41a9b2f 100644
> --- a/xen/include/xen/xmalloc.h
> +++ b/xen/include/xen/xmalloc.h
> @@ -29,6 +29,7 @@ extern void xfree(void *);
>  /* Underlying functions */
>  extern void *_xmalloc(unsigned long size, unsigned long align);
>  extern void *_xzalloc(unsigned long size, unsigned long align);
> +extern void *_xrealloc(void *p, unsigned long new_size, unsigned long align);
>
>  static inline void *_xmalloc_array(
>      unsigned long size, unsigned long align, unsigned long num)
>

-- 
Julien Grall

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

Re: [RFC 3/6] Introduce _xrealloc

[Wei Liu]

On Thu, Jun 08, 2017 at 08:49:01PM +0100, Julien Grall wrote:
> CC the REST maintainers
> 
> On 08/06/2017 20:30, Sameer Goel wrote:
> > Introduce a memory realloc function.
> > 
> > Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
> > ---
> >  xen/common/xmalloc_tlsf.c | 13 +++++++++++++
> >  xen/include/xen/xmalloc.h |  1 +
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> > index b256dc5..52385a8 100644
> > --- a/xen/common/xmalloc_tlsf.c
> > +++ b/xen/common/xmalloc_tlsf.c
> > @@ -612,6 +612,19 @@ void *_xzalloc(unsigned long size, unsigned long align)
> >      return p ? memset(p, 0, size) : p;
> >  }
> > 
> > +void *_xrealloc(void *p, unsigned long new_size, unsigned long align)
> > +{
> > +    void *new_p = _xmalloc(new_size, align);
> > +
> > +    if(new_p && p)
> 
> Coding style: if ( ... )
> 
> > +    {
> > +        memcpy(new_p, p, new_size);

This is wrong. How can you know if the area pointed to by p is at least
new_size bytes long?

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

Re: [RFC 3/6] Introduce _xrealloc

[Goel, Sameer]

On 6/9/2017 3:44 AM, Wei Liu wrote:
> On Thu, Jun 08, 2017 at 08:49:01PM +0100, Julien Grall wrote:
>> CC the REST maintainers
>>
>> On 08/06/2017 20:30, Sameer Goel wrote:
>>> Introduce a memory realloc function.
>>>
>>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>>> ---
>>>  xen/common/xmalloc_tlsf.c | 13 +++++++++++++
>>>  xen/include/xen/xmalloc.h |  1 +
>>>  2 files changed, 14 insertions(+)
>>>
>>> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
>>> index b256dc5..52385a8 100644
>>> --- a/xen/common/xmalloc_tlsf.c
>>> +++ b/xen/common/xmalloc_tlsf.c
>>> @@ -612,6 +612,19 @@ void *_xzalloc(unsigned long size, unsigned long align)
>>>      return p ? memset(p, 0, size) : p;
>>>  }
>>>
>>> +void *_xrealloc(void *p, unsigned long new_size, unsigned long align)
>>> +{
>>> +    void *new_p = _xmalloc(new_size, align);
>>> +
>>> +    if(new_p && p)
>>
>> Coding style: if ( ... )
>>
>>> +    {
>>> +        memcpy(new_p, p, new_size);
> 
> This is wrong. How can you know if the area pointed to by p is at least
> new_size bytes long?
> 
Agreed, I revisited the code and will remove _xrealloc and use xfree and _xmalloc instead.

Thanks,
Sameer
-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

Re: [RFC 3/6] Introduce _xrealloc

[Julien Grall]

Hi,

Bringing back this discussion.

On 28/08/17 22:39, Goel, Sameer wrote:
> 
> 
> On 6/9/2017 3:44 AM, Wei Liu wrote:
>> On Thu, Jun 08, 2017 at 08:49:01PM +0100, Julien Grall wrote:
>>> CC the REST maintainers
>>>
>>> On 08/06/2017 20:30, Sameer Goel wrote:
>>>> Introduce a memory realloc function.
>>>>
>>>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>>>> ---
>>>>   xen/common/xmalloc_tlsf.c | 13 +++++++++++++
>>>>   xen/include/xen/xmalloc.h |  1 +
>>>>   2 files changed, 14 insertions(+)
>>>>
>>>> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
>>>> index b256dc5..52385a8 100644
>>>> --- a/xen/common/xmalloc_tlsf.c
>>>> +++ b/xen/common/xmalloc_tlsf.c
>>>> @@ -612,6 +612,19 @@ void *_xzalloc(unsigned long size, unsigned long align)
>>>>       return p ? memset(p, 0, size) : p;
>>>>   }
>>>>
>>>> +void *_xrealloc(void *p, unsigned long new_size, unsigned long align)
>>>> +{
>>>> +    void *new_p = _xmalloc(new_size, align);
>>>> +
>>>> +    if(new_p && p)
>>>
>>> Coding style: if ( ... )
>>>
>>>> +    {
>>>> +        memcpy(new_p, p, new_size);
>>
>> This is wrong. How can you know if the area pointed to by p is at least
>> new_size bytes long?
>>
> Agreed, I revisited the code and will remove _xrealloc and use xfree and _xmalloc instead.

I am not sure why you choose to drop it completely. xfree is able to 
know the size of the buffer to free.

So you could find out the size and copy the right amount of data.

Note that having _xrealloc would be my preference over an open-coded 
version in the code.

Cheers,

-- 
Julien Grall

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

Re: [RFC 3/6] Introduce _xrealloc

[Wei Liu]

On Thu, Oct 12, 2017 at 02:33:04PM +0100, Julien Grall wrote:
> Hi,
> 
> Bringing back this discussion.
> 
> On 28/08/17 22:39, Goel, Sameer wrote:
> > 
> > 
> > On 6/9/2017 3:44 AM, Wei Liu wrote:
> > > On Thu, Jun 08, 2017 at 08:49:01PM +0100, Julien Grall wrote:
> > > > CC the REST maintainers
> > > > 
> > > > On 08/06/2017 20:30, Sameer Goel wrote:
> > > > > Introduce a memory realloc function.
> > > > > 
> > > > > Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
> > > > > ---
> > > > >   xen/common/xmalloc_tlsf.c | 13 +++++++++++++
> > > > >   xen/include/xen/xmalloc.h |  1 +
> > > > >   2 files changed, 14 insertions(+)
> > > > > 
> > > > > diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> > > > > index b256dc5..52385a8 100644
> > > > > --- a/xen/common/xmalloc_tlsf.c
> > > > > +++ b/xen/common/xmalloc_tlsf.c
> > > > > @@ -612,6 +612,19 @@ void *_xzalloc(unsigned long size, unsigned long align)
> > > > >       return p ? memset(p, 0, size) : p;
> > > > >   }
> > > > > 
> > > > > +void *_xrealloc(void *p, unsigned long new_size, unsigned long align)
> > > > > +{
> > > > > +    void *new_p = _xmalloc(new_size, align);
> > > > > +
> > > > > +    if(new_p && p)
> > > > 
> > > > Coding style: if ( ... )
> > > > 
> > > > > +    {
> > > > > +        memcpy(new_p, p, new_size);
> > > 
> > > This is wrong. How can you know if the area pointed to by p is at least
> > > new_size bytes long?
> > > 
> > Agreed, I revisited the code and will remove _xrealloc and use xfree and _xmalloc instead.
> 
> I am not sure why you choose to drop it completely. xfree is able to know
> the size of the buffer to free.
> 
> So you could find out the size and copy the right amount of data.
> 
> Note that having _xrealloc would be my preference over an open-coded version
> in the code.

I would vouch for a properly implemented _xrealloc. :-)

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

Re: [RFC 3/6] Introduce _xrealloc

[Stefano Stabellini]

On Thu, 8 Jun 2017, Sameer Goel wrote:
> Introduce a memory realloc function.
> 
> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
> ---
>  xen/common/xmalloc_tlsf.c | 13 +++++++++++++
>  xen/include/xen/xmalloc.h |  1 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> index b256dc5..52385a8 100644
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -612,6 +612,19 @@ void *_xzalloc(unsigned long size, unsigned long align)
>      return p ? memset(p, 0, size) : p;
>  }
>  
> +void *_xrealloc(void *p, unsigned long new_size, unsigned long align)
> +{
> +    void *new_p = _xmalloc(new_size, align);

It might be best to handle the case where new_size is 0 explicitly, and
only free p.


> +    if(new_p && p)
> +    {
> +        memcpy(new_p, p, new_size);
> +        xfree(p);
> +    }
> +
> +    return new_p;
> +}
> +
>  void xfree(void *p)
>  {
>      struct bhdr *b;
> diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
> index 24a99ac..41a9b2f 100644
> --- a/xen/include/xen/xmalloc.h
> +++ b/xen/include/xen/xmalloc.h
> @@ -29,6 +29,7 @@ extern void xfree(void *);
>  /* Underlying functions */
>  extern void *_xmalloc(unsigned long size, unsigned long align);
>  extern void *_xzalloc(unsigned long size, unsigned long align);
> +extern void *_xrealloc(void *p, unsigned long new_size, unsigned long align);
>  
>  static inline void *_xmalloc_array(
>      unsigned long size, unsigned long align, unsigned long num)
> -- 
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

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

[RFC 4/6] xen/passthrough/arm: Introduce iommu_fwspec

[Sameer Goel]

Introduce a common structure to hold the fw (ACPI or DT) defined
configuration for SMMU hw. The current use case is for arm SMMUs. So,
making this architecture specific.

Based on Linux kernel commit 57f98d2f61e1: iommu: Introduce iommu_fwspec
Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
---
 xen/drivers/passthrough/arm/iommu.c | 57 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/device.h        |  1 +
 xen/include/xen/iommu.h             | 28 ++++++++++++++++++
 3 files changed, 86 insertions(+)

diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 95b1abb..edf70c2 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -73,3 +73,60 @@ int arch_iommu_populate_page_table(struct domain *d)
     /* The IOMMU shares the p2m with the CPU */
     return -ENOSYS;
 }
+
+int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
+        const struct iommu_ops *ops)
+{
+    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+    if (fwspec)
+        return ops == fwspec->ops ? 0 : -EINVAL;
+
+    fwspec = xzalloc(struct iommu_fwspec);
+    if (!fwspec)
+        return -ENOMEM;
+
+    /* Ref counting for the dt device node is not needed */
+
+    /*of_node_get(to_of_node(iommu_fwnode));*/
+
+    fwspec->iommu_fwnode = iommu_fwnode;
+    fwspec->ops = ops;
+    dev->iommu_fwspec = fwspec;
+    return 0;
+}
+
+void iommu_fwspec_free(struct device *dev)
+{
+    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+    if (fwspec) {
+        /*fwnode_handle_put(fwspec->iommu_fwnode);*/
+        xfree(fwspec);
+        dev->iommu_fwspec = NULL;
+    }
+}
+
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
+{
+    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+    size_t size;
+    int i;
+
+    if (!fwspec)
+        return -EINVAL;
+
+    size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
+    if (size > sizeof(*fwspec)) {
+        fwspec = _xrealloc(dev->iommu_fwspec, size, sizeof(void *));
+        if (!fwspec)
+            return -ENOMEM;
+    }
+
+    for (i = 0; i < num_ids; i++)
+        fwspec->ids[fwspec->num_ids + i] = ids[i];
+
+    fwspec->num_ids += num_ids;
+    dev->iommu_fwspec = fwspec;
+    return 0;
+}
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 78c38fe..5027c87 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -21,6 +21,7 @@ struct device
     struct dt_device_node *of_node; /* Used by drivers imported from Linux */
 #endif
     struct fwnode_handle *fwnode; /*fw device node identifier */
+    struct iommu_fwspec *iommu_fwspec;
     struct dev_archdata archdata;
 };
 
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5803e3f..7ef9b93 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -224,4 +224,32 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
 extern struct spinlock iommu_pt_cleanup_lock;
 extern struct page_list_head iommu_pt_cleanup_list;
 
+/**
+ * Following block was ported from Linux to help with the implementation of
+ * arm64 iommu devices. Hence the architecture specific compile
+ */
+
+#if defined(CONFIG_ARM_64) || defined(CONFIG_ARM)
+/**
+ * struct iommu_fwspec - per-device IOMMU instance data
+ * @ops: ops for this device's IOMMU
+ * @iommu_fwnode: firmware handle for this device's IOMMU
+ * @iommu_priv: IOMMU driver private data for this device
+ * @num_ids: number of associated device IDs
+ * @ids: IDs which this device may present to the IOMMU
+ */
+struct iommu_fwspec {
+	const struct iommu_ops	*ops;
+	struct fwnode_handle	*iommu_fwnode;
+	void			*iommu_priv;
+	unsigned int		num_ids;
+	u32			ids[1];
+};
+
+int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
+		      const struct iommu_ops *ops);
+void iommu_fwspec_free(struct device *dev);
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
+
+#endif
 #endif /* _IOMMU_H_ */
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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

Re: [RFC 4/6] xen/passthrough/arm: Introduce iommu_fwspec

[Julien Grall]

Hi,

On 08/06/2017 20:30, Sameer Goel wrote:
> Introduce a common structure to hold the fw (ACPI or DT) defined
> configuration for SMMU hw. The current use case is for arm SMMUs. So,
> making this architecture specific.
>
> Based on Linux kernel commit 57f98d2f61e1: iommu: Introduce iommu_fwspec
> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
> ---
>  xen/drivers/passthrough/arm/iommu.c | 57 +++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/device.h        |  1 +
>  xen/include/xen/iommu.h             | 28 ++++++++++++++++++
>  3 files changed, 86 insertions(+)
>
> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
> index 95b1abb..edf70c2 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -73,3 +73,60 @@ int arch_iommu_populate_page_table(struct domain *d)
>      /* The IOMMU shares the p2m with the CPU */
>      return -ENOSYS;
>  }
> +
> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> +        const struct iommu_ops *ops)

If you put code in iommu.c then you need to respect the coding style of 
the file. So this should be correctly indented.

> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> +    if (fwspec)

Coding style.

> +        return ops == fwspec->ops ? 0 : -EINVAL;
> +
> +    fwspec = xzalloc(struct iommu_fwspec);
> +    if (!fwspec)

Ditto.

> +        return -ENOMEM;
> +
> +    /* Ref counting for the dt device node is not needed */
> +
> +    /*of_node_get(to_of_node(iommu_fwnode));*/

I am a bit confused. You put this in a comment here and drop it in patch 
#5. So you probably want to drop here.

> +
> +    fwspec->iommu_fwnode = iommu_fwnode;
> +    fwspec->ops = ops;
> +    dev->iommu_fwspec = fwspec;

Newline here please.

> +    return 0;
> +}
> +
> +void iommu_fwspec_free(struct device *dev)
> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> +    if (fwspec) {

Coding style.

> +        /*fwnode_handle_put(fwspec->iommu_fwnode);*/

And ditto for the comment.

> +        xfree(fwspec);
> +        dev->iommu_fwspec = NULL;
> +    }
> +}
> +
> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)

As you don't respect the Linux coding style (you are using soft-tab). I 
see little point to stay close to Linux for this code as it would be 
difficult to port anyway.

So s/u32/uint32_t/

> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +    size_t size;
> +    int i;
> +
> +    if (!fwspec)

Coding style.

> +        return -EINVAL;
> +
> +    size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
> +    if (size > sizeof(*fwspec)) {

Coding style.

> +        fwspec = _xrealloc(dev->iommu_fwspec, size, sizeof(void *));

If you introduce _xrealloc (I will leave the REST maintainers decide 
whether they want it), then please introduce xrealloc macro.

> +        if (!fwspec)

Coding style.

> +            return -ENOMEM;
> +    }
> +
> +    for (i = 0; i < num_ids; i++)

Coding style.

> +        fwspec->ids[fwspec->num_ids + i] = ids[i];
> +
> +    fwspec->num_ids += num_ids;
> +    dev->iommu_fwspec = fwspec;

newline.

> +    return 0;
> +}
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 78c38fe..5027c87 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -21,6 +21,7 @@ struct device
>      struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>  #endif
>      struct fwnode_handle *fwnode; /*fw device node identifier */
> +    struct iommu_fwspec *iommu_fwspec;
>      struct dev_archdata archdata;
>  };
>
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 5803e3f..7ef9b93 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -224,4 +224,32 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>  extern struct spinlock iommu_pt_cleanup_lock;
>  extern struct page_list_head iommu_pt_cleanup_list;
>
> +/**
> + * Following block was ported from Linux to help with the implementation of
> + * arm64 iommu devices. Hence the architecture specific compile
> + */
> +
> +#if defined(CONFIG_ARM_64) || defined(CONFIG_ARM)

No ifdef ARCH in xen/* headers. If this is only used by ARM, then it 
should go in asm-arm/iommu.h.

> +/**
> + * struct iommu_fwspec - per-device IOMMU instance data
> + * @ops: ops for this device's IOMMU
> + * @iommu_fwnode: firmware handle for this device's IOMMU
> + * @iommu_priv: IOMMU driver private data for this device
> + * @num_ids: number of associated device IDs
> + * @ids: IDs which this device may present to the IOMMU
> + */
> +struct iommu_fwspec {
> +	const struct iommu_ops	*ops;
> +	struct fwnode_handle	*iommu_fwnode;
> +	void			*iommu_priv;
> +	unsigned int		num_ids;
> +	u32			ids[1];
> +};
> +
> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> +		      const struct iommu_ops *ops);
> +void iommu_fwspec_free(struct device *dev);
> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> +
> +#endif
>  #endif /* _IOMMU_H_ */
>

Cheers,

-- 
Julien Grall

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

[RFC 5/6] ACPI: arm: Support for IORT

[Sameer Goel]

Verbatim files from Linux kernel.
iort.c: commit ca78d3173cff:Merge tag 'arm64-upstream'
acpi_iort.h: commit 18b709beb503:ACPI/IORT: Make dma masks set-up IORT
specific

Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
---
 xen/drivers/acpi/arm/iort.c  | 961 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/acpi/acpi_iort.h |  58 +++
 2 files changed, 1019 insertions(+)
 create mode 100644 xen/drivers/acpi/arm/iort.c
 create mode 100644 xen/include/acpi/acpi_iort.h

diff --git a/xen/drivers/acpi/arm/iort.c b/xen/drivers/acpi/arm/iort.c
new file mode 100644
index 0000000..4a5bb96
--- /dev/null
+++ b/xen/drivers/acpi/arm/iort.c
@@ -0,0 +1,961 @@
+/*
+ * Copyright (C) 2016, Semihalf
+ *	Author: Tomasz Nowicki <tn@semihalf.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * 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
+ */
+
+#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>
+
+#define IORT_TYPE_MASK(type)	(1 << (type))
+#define IORT_MSI_TYPE		(1 << ACPI_IORT_NODE_ITS_GROUP)
+#define IORT_IOMMU_TYPE		((1 << ACPI_IORT_NODE_SMMU) |	\
+				(1 << ACPI_IORT_NODE_SMMU_V3))
+
+struct iort_its_msi_chip {
+	struct list_head	list;
+	struct fwnode_handle	*fw_node;
+	u32			translation_id;
+};
+
+struct iort_fwnode {
+	struct list_head list;
+	struct acpi_iort_node *iort_node;
+	struct fwnode_handle *fwnode;
+};
+static LIST_HEAD(iort_fwnode_list);
+static DEFINE_SPINLOCK(iort_fwnode_lock);
+
+/**
+ * iort_set_fwnode() - Create iort_fwnode and use it to register
+ *		       iommu data in the iort_fwnode_list
+ *
+ * @node: IORT table node associated with the IOMMU
+ * @fwnode: fwnode associated with the IORT node
+ *
+ * Returns: 0 on success
+ *          <0 on failure
+ */
+static inline int iort_set_fwnode(struct acpi_iort_node *iort_node,
+				  struct fwnode_handle *fwnode)
+{
+	struct iort_fwnode *np;
+
+	np = kzalloc(sizeof(struct iort_fwnode), GFP_ATOMIC);
+
+	if (WARN_ON(!np))
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&np->list);
+	np->iort_node = iort_node;
+	np->fwnode = fwnode;
+
+	spin_lock(&iort_fwnode_lock);
+	list_add_tail(&np->list, &iort_fwnode_list);
+	spin_unlock(&iort_fwnode_lock);
+
+	return 0;
+}
+
+/**
+ * iort_get_fwnode() - Retrieve fwnode associated with an IORT node
+ *
+ * @node: IORT table node to be looked-up
+ *
+ * Returns: fwnode_handle pointer on success, NULL on failure
+ */
+static inline
+struct fwnode_handle *iort_get_fwnode(struct acpi_iort_node *node)
+{
+	struct iort_fwnode *curr;
+	struct fwnode_handle *fwnode = NULL;
+
+	spin_lock(&iort_fwnode_lock);
+	list_for_each_entry(curr, &iort_fwnode_list, list) {
+		if (curr->iort_node == node) {
+			fwnode = curr->fwnode;
+			break;
+		}
+	}
+	spin_unlock(&iort_fwnode_lock);
+
+	return fwnode;
+}
+
+/**
+ * iort_delete_fwnode() - Delete fwnode associated with an IORT node
+ *
+ * @node: IORT table node associated with fwnode to delete
+ */
+static inline void iort_delete_fwnode(struct acpi_iort_node *node)
+{
+	struct iort_fwnode *curr, *tmp;
+
+	spin_lock(&iort_fwnode_lock);
+	list_for_each_entry_safe(curr, tmp, &iort_fwnode_list, list) {
+		if (curr->iort_node == node) {
+			list_del(&curr->list);
+			kfree(curr);
+			break;
+		}
+	}
+	spin_unlock(&iort_fwnode_lock);
+}
+
+typedef acpi_status (*iort_find_node_callback)
+	(struct acpi_iort_node *node, void *context);
+
+/* Root pointer to the mapped IORT table */
+static struct acpi_table_header *iort_table;
+
+static LIST_HEAD(iort_msi_chip_list);
+static DEFINE_SPINLOCK(iort_msi_chip_lock);
+
+/**
+ * iort_register_domain_token() - register domain token and related ITS ID
+ * to the list from where we can get it back later on.
+ * @trans_id: ITS ID.
+ * @fw_node: Domain token.
+ *
+ * Returns: 0 on success, -ENOMEM if no memory when allocating list element
+ */
+int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node)
+{
+	struct iort_its_msi_chip *its_msi_chip;
+
+	its_msi_chip = kzalloc(sizeof(*its_msi_chip), GFP_KERNEL);
+	if (!its_msi_chip)
+		return -ENOMEM;
+
+	its_msi_chip->fw_node = fw_node;
+	its_msi_chip->translation_id = trans_id;
+
+	spin_lock(&iort_msi_chip_lock);
+	list_add(&its_msi_chip->list, &iort_msi_chip_list);
+	spin_unlock(&iort_msi_chip_lock);
+
+	return 0;
+}
+
+/**
+ * iort_deregister_domain_token() - Deregister domain token based on ITS ID
+ * @trans_id: ITS ID.
+ *
+ * Returns: none.
+ */
+void iort_deregister_domain_token(int trans_id)
+{
+	struct iort_its_msi_chip *its_msi_chip, *t;
+
+	spin_lock(&iort_msi_chip_lock);
+	list_for_each_entry_safe(its_msi_chip, t, &iort_msi_chip_list, list) {
+		if (its_msi_chip->translation_id == trans_id) {
+			list_del(&its_msi_chip->list);
+			kfree(its_msi_chip);
+			break;
+		}
+	}
+	spin_unlock(&iort_msi_chip_lock);
+}
+
+/**
+ * iort_find_domain_token() - Find domain token based on given ITS ID
+ * @trans_id: ITS ID.
+ *
+ * Returns: domain token when find on the list, NULL otherwise
+ */
+struct fwnode_handle *iort_find_domain_token(int trans_id)
+{
+	struct fwnode_handle *fw_node = NULL;
+	struct iort_its_msi_chip *its_msi_chip;
+
+	spin_lock(&iort_msi_chip_lock);
+	list_for_each_entry(its_msi_chip, &iort_msi_chip_list, list) {
+		if (its_msi_chip->translation_id == trans_id) {
+			fw_node = its_msi_chip->fw_node;
+			break;
+		}
+	}
+	spin_unlock(&iort_msi_chip_lock);
+
+	return fw_node;
+}
+
+static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
+					     iort_find_node_callback callback,
+					     void *context)
+{
+	struct acpi_iort_node *iort_node, *iort_end;
+	struct acpi_table_iort *iort;
+	int i;
+
+	if (!iort_table)
+		return NULL;
+
+	/* Get the first IORT node */
+	iort = (struct acpi_table_iort *)iort_table;
+	iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
+				 iort->node_offset);
+	iort_end = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
+				iort_table->length);
+
+	for (i = 0; i < iort->node_count; i++) {
+		if (WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND,
+			       "IORT node pointer overflows, bad table!\n"))
+			return NULL;
+
+		if (iort_node->type == type &&
+		    ACPI_SUCCESS(callback(iort_node, context)))
+				return iort_node;
+
+		iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort_node,
+					 iort_node->length);
+	}
+
+	return NULL;
+}
+
+static acpi_status
+iort_match_type_callback(struct acpi_iort_node *node, void *context)
+{
+	return AE_OK;
+}
+
+bool iort_node_match(u8 type)
+{
+	struct acpi_iort_node *node;
+
+	node = iort_scan_node(type, iort_match_type_callback, NULL);
+
+	return node != NULL;
+}
+
+static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
+					    void *context)
+{
+	struct device *dev = context;
+	acpi_status status;
+
+	if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT) {
+		struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
+		struct acpi_device *adev = to_acpi_device_node(dev->fwnode);
+		struct acpi_iort_named_component *ncomp;
+
+		if (!adev) {
+			status = AE_NOT_FOUND;
+			goto out;
+		}
+
+		status = acpi_get_name(adev->handle, ACPI_FULL_PATHNAME, &buf);
+		if (ACPI_FAILURE(status)) {
+			dev_warn(dev, "Can't get device full path name\n");
+			goto out;
+		}
+
+		ncomp = (struct acpi_iort_named_component *)node->node_data;
+		status = !strcmp(ncomp->device_name, buf.pointer) ?
+							AE_OK : AE_NOT_FOUND;
+		acpi_os_free(buf.pointer);
+	} else if (node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
+		struct acpi_iort_root_complex *pci_rc;
+		struct pci_bus *bus;
+
+		bus = to_pci_bus(dev);
+		pci_rc = (struct acpi_iort_root_complex *)node->node_data;
+
+		/*
+		 * It is assumed that PCI segment numbers maps one-to-one
+		 * with root complexes. Each segment number can represent only
+		 * one root complex.
+		 */
+		status = pci_rc->pci_segment_number == pci_domain_nr(bus) ?
+							AE_OK : AE_NOT_FOUND;
+	} else {
+		status = AE_NOT_FOUND;
+	}
+out:
+	return status;
+}
+
+static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
+		       u32 *rid_out)
+{
+	/* Single mapping does not care for input id */
+	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
+		if (type == ACPI_IORT_NODE_NAMED_COMPONENT ||
+		    type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
+			*rid_out = map->output_base;
+			return 0;
+		}
+
+		pr_warn(FW_BUG "[map %p] SINGLE MAPPING flag not allowed for node type %d, skipping ID map\n",
+			map, type);
+		return -ENXIO;
+	}
+
+	if (rid_in < map->input_base ||
+	    (rid_in >= map->input_base + map->id_count))
+		return -ENXIO;
+
+	*rid_out = map->output_base + (rid_in - map->input_base);
+	return 0;
+}
+
+static
+struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
+					u32 *id_out, u8 type_mask,
+					int index)
+{
+	struct acpi_iort_node *parent;
+	struct acpi_iort_id_mapping *map;
+
+	if (!node->mapping_offset || !node->mapping_count ||
+				     index >= node->mapping_count)
+		return NULL;
+
+	map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
+			   node->mapping_offset + index * sizeof(*map));
+
+	/* Firmware bug! */
+	if (!map->output_reference) {
+		pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
+		       node, node->type);
+		return NULL;
+	}
+
+	parent = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
+			       map->output_reference);
+
+	if (!(IORT_TYPE_MASK(parent->type) & type_mask))
+		return NULL;
+
+	if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
+		if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
+		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
+			*id_out = map->output_base;
+			return parent;
+		}
+	}
+
+	return NULL;
+}
+
+static struct acpi_iort_node *iort_node_map_rid(struct acpi_iort_node *node,
+						u32 rid_in, u32 *rid_out,
+						u8 type_mask)
+{
+	u32 rid = rid_in;
+
+	/* Parse the ID mapping tree to find specified node type */
+	while (node) {
+		struct acpi_iort_id_mapping *map;
+		int i;
+
+		if (IORT_TYPE_MASK(node->type) & type_mask) {
+			if (rid_out)
+				*rid_out = rid;
+			return node;
+		}
+
+		if (!node->mapping_offset || !node->mapping_count)
+			goto fail_map;
+
+		map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
+				   node->mapping_offset);
+
+		/* Firmware bug! */
+		if (!map->output_reference) {
+			pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
+			       node, node->type);
+			goto fail_map;
+		}
+
+		/* Do the RID translation */
+		for (i = 0; i < node->mapping_count; i++, map++) {
+			if (!iort_id_map(map, node->type, rid, &rid))
+				break;
+		}
+
+		if (i == node->mapping_count)
+			goto fail_map;
+
+		node = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
+				    map->output_reference);
+	}
+
+fail_map:
+	/* Map input RID to output RID unchanged on mapping failure*/
+	if (rid_out)
+		*rid_out = rid_in;
+
+	return NULL;
+}
+
+static struct acpi_iort_node *iort_find_dev_node(struct device *dev)
+{
+	struct pci_bus *pbus;
+
+	if (!dev_is_pci(dev))
+		return iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
+				      iort_match_node_callback, dev);
+
+	/* Find a PCI root bus */
+	pbus = to_pci_dev(dev)->bus;
+	while (!pci_is_root_bus(pbus))
+		pbus = pbus->parent;
+
+	return iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
+			      iort_match_node_callback, &pbus->dev);
+}
+
+/**
+ * iort_msi_map_rid() - Map a MSI requester ID for a device
+ * @dev: The device for which the mapping is to be done.
+ * @req_id: The device requester ID.
+ *
+ * Returns: mapped MSI RID on success, input requester ID otherwise
+ */
+u32 iort_msi_map_rid(struct device *dev, u32 req_id)
+{
+	struct acpi_iort_node *node;
+	u32 dev_id;
+
+	node = iort_find_dev_node(dev);
+	if (!node)
+		return req_id;
+
+	iort_node_map_rid(node, req_id, &dev_id, IORT_MSI_TYPE);
+	return dev_id;
+}
+
+/**
+ * iort_dev_find_its_id() - Find the ITS identifier for a device
+ * @dev: The device.
+ * @idx: Index of the ITS identifier list.
+ * @its_id: ITS identifier.
+ *
+ * Returns: 0 on success, appropriate error value otherwise
+ */
+static int iort_dev_find_its_id(struct device *dev, u32 req_id,
+				unsigned int idx, int *its_id)
+{
+	struct acpi_iort_its_group *its;
+	struct acpi_iort_node *node;
+
+	node = iort_find_dev_node(dev);
+	if (!node)
+		return -ENXIO;
+
+	node = iort_node_map_rid(node, req_id, NULL, IORT_MSI_TYPE);
+	if (!node)
+		return -ENXIO;
+
+	/* Move to ITS specific data */
+	its = (struct acpi_iort_its_group *)node->node_data;
+	if (idx > its->its_count) {
+		dev_err(dev, "requested ITS ID index [%d] is greater than available [%d]\n",
+			idx, its->its_count);
+		return -ENXIO;
+	}
+
+	*its_id = its->identifiers[idx];
+	return 0;
+}
+
+/**
+ * iort_get_device_domain() - Find MSI domain related to a device
+ * @dev: The device.
+ * @req_id: Requester ID for the device.
+ *
+ * Returns: the MSI domain for this device, NULL otherwise
+ */
+struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id)
+{
+	struct fwnode_handle *handle;
+	int its_id;
+
+	if (iort_dev_find_its_id(dev, req_id, 0, &its_id))
+		return NULL;
+
+	handle = iort_find_domain_token(its_id);
+	if (!handle)
+		return NULL;
+
+	return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
+}
+
+static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
+{
+	u32 *rid = data;
+
+	*rid = alias;
+	return 0;
+}
+
+static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
+			       struct fwnode_handle *fwnode,
+			       const struct iommu_ops *ops)
+{
+	int ret = iommu_fwspec_init(dev, fwnode, ops);
+
+	if (!ret)
+		ret = iommu_fwspec_add_ids(dev, &streamid, 1);
+
+	return ret;
+}
+
+static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
+					struct acpi_iort_node *node,
+					u32 streamid)
+{
+	const struct iommu_ops *ops = NULL;
+	int ret = -ENODEV;
+	struct fwnode_handle *iort_fwnode;
+
+	if (node) {
+		iort_fwnode = iort_get_fwnode(node);
+		if (!iort_fwnode)
+			return NULL;
+
+		ops = iommu_ops_from_fwnode(iort_fwnode);
+		if (!ops)
+			return NULL;
+
+		ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
+	}
+
+	return ret ? NULL : ops;
+}
+
+/**
+ * iort_set_dma_mask - Set-up dma mask for a device.
+ *
+ * @dev: device to configure
+ */
+void iort_set_dma_mask(struct device *dev)
+{
+	/*
+	 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
+	 * setup the correct supported mask.
+	 */
+	if (!dev->coherent_dma_mask)
+		dev->coherent_dma_mask = DMA_BIT_MASK(32);
+
+	/*
+	 * Set it to coherent_dma_mask by default if the architecture
+	 * code has not set it.
+	 */
+	if (!dev->dma_mask)
+		dev->dma_mask = &dev->coherent_dma_mask;
+}
+
+/**
+ * iort_iommu_configure - Set-up IOMMU configuration for a device.
+ *
+ * @dev: device to configure
+ *
+ * Returns: iommu_ops pointer on configuration success
+ *          NULL on configuration failure
+ */
+const struct iommu_ops *iort_iommu_configure(struct device *dev)
+{
+	struct acpi_iort_node *node, *parent;
+	const struct iommu_ops *ops = NULL;
+	u32 streamid = 0;
+
+	if (dev_is_pci(dev)) {
+		struct pci_bus *bus = to_pci_dev(dev)->bus;
+		u32 rid;
+
+		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
+				       &rid);
+
+		node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
+				      iort_match_node_callback, &bus->dev);
+		if (!node)
+			return NULL;
+
+		parent = iort_node_map_rid(node, rid, &streamid,
+					   IORT_IOMMU_TYPE);
+
+		ops = iort_iommu_xlate(dev, parent, streamid);
+
+	} else {
+		int i = 0;
+
+		node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
+				      iort_match_node_callback, dev);
+		if (!node)
+			return NULL;
+
+		parent = iort_node_get_id(node, &streamid,
+					  IORT_IOMMU_TYPE, i++);
+
+		while (parent) {
+			ops = iort_iommu_xlate(dev, parent, streamid);
+
+			parent = iort_node_get_id(node, &streamid,
+						  IORT_IOMMU_TYPE, i++);
+		}
+	}
+
+	return ops;
+}
+
+static void __init acpi_iort_register_irq(int hwirq, const char *name,
+					  int trigger,
+					  struct resource *res)
+{
+	int irq = acpi_register_gsi(NULL, hwirq, trigger,
+				    ACPI_ACTIVE_HIGH);
+
+	if (irq <= 0) {
+		pr_err("could not register gsi hwirq %d name [%s]\n", hwirq,
+								      name);
+		return;
+	}
+
+	res->start = irq;
+	res->end = irq;
+	res->flags = IORESOURCE_IRQ;
+	res->name = name;
+}
+
+static int __init arm_smmu_v3_count_resources(struct acpi_iort_node *node)
+{
+	struct acpi_iort_smmu_v3 *smmu;
+	/* Always present mem resource */
+	int num_res = 1;
+
+	/* Retrieve SMMUv3 specific data */
+	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
+
+	if (smmu->event_gsiv)
+		num_res++;
+
+	if (smmu->pri_gsiv)
+		num_res++;
+
+	if (smmu->gerr_gsiv)
+		num_res++;
+
+	if (smmu->sync_gsiv)
+		num_res++;
+
+	return num_res;
+}
+
+static void __init arm_smmu_v3_init_resources(struct resource *res,
+					      struct acpi_iort_node *node)
+{
+	struct acpi_iort_smmu_v3 *smmu;
+	int num_res = 0;
+
+	/* Retrieve SMMUv3 specific data */
+	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
+
+	res[num_res].start = smmu->base_address;
+	res[num_res].end = smmu->base_address + SZ_128K - 1;
+	res[num_res].flags = IORESOURCE_MEM;
+
+	num_res++;
+
+	if (smmu->event_gsiv)
+		acpi_iort_register_irq(smmu->event_gsiv, "eventq",
+				       ACPI_EDGE_SENSITIVE,
+				       &res[num_res++]);
+
+	if (smmu->pri_gsiv)
+		acpi_iort_register_irq(smmu->pri_gsiv, "priq",
+				       ACPI_EDGE_SENSITIVE,
+				       &res[num_res++]);
+
+	if (smmu->gerr_gsiv)
+		acpi_iort_register_irq(smmu->gerr_gsiv, "gerror",
+				       ACPI_EDGE_SENSITIVE,
+				       &res[num_res++]);
+
+	if (smmu->sync_gsiv)
+		acpi_iort_register_irq(smmu->sync_gsiv, "cmdq-sync",
+				       ACPI_EDGE_SENSITIVE,
+				       &res[num_res++]);
+}
+
+static bool __init arm_smmu_v3_is_coherent(struct acpi_iort_node *node)
+{
+	struct acpi_iort_smmu_v3 *smmu;
+
+	/* Retrieve SMMUv3 specific data */
+	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
+
+	return smmu->flags & ACPI_IORT_SMMU_V3_COHACC_OVERRIDE;
+}
+
+static int __init arm_smmu_count_resources(struct acpi_iort_node *node)
+{
+	struct acpi_iort_smmu *smmu;
+
+	/* Retrieve SMMU specific data */
+	smmu = (struct acpi_iort_smmu *)node->node_data;
+
+	/*
+	 * Only consider the global fault interrupt and ignore the
+	 * configuration access interrupt.
+	 *
+	 * MMIO address and global fault interrupt resources are always
+	 * present so add them to the context interrupt count as a static
+	 * value.
+	 */
+	return smmu->context_interrupt_count + 2;
+}
+
+static void __init arm_smmu_init_resources(struct resource *res,
+					   struct acpi_iort_node *node)
+{
+	struct acpi_iort_smmu *smmu;
+	int i, hw_irq, trigger, num_res = 0;
+	u64 *ctx_irq, *glb_irq;
+
+	/* Retrieve SMMU specific data */
+	smmu = (struct acpi_iort_smmu *)node->node_data;
+
+	res[num_res].start = smmu->base_address;
+	res[num_res].end = smmu->base_address + smmu->span - 1;
+	res[num_res].flags = IORESOURCE_MEM;
+	num_res++;
+
+	glb_irq = ACPI_ADD_PTR(u64, node, smmu->global_interrupt_offset);
+	/* Global IRQs */
+	hw_irq = IORT_IRQ_MASK(glb_irq[0]);
+	trigger = IORT_IRQ_TRIGGER_MASK(glb_irq[0]);
+
+	acpi_iort_register_irq(hw_irq, "arm-smmu-global", trigger,
+				     &res[num_res++]);
+
+	/* Context IRQs */
+	ctx_irq = ACPI_ADD_PTR(u64, node, smmu->context_interrupt_offset);
+	for (i = 0; i < smmu->context_interrupt_count; i++) {
+		hw_irq = IORT_IRQ_MASK(ctx_irq[i]);
+		trigger = IORT_IRQ_TRIGGER_MASK(ctx_irq[i]);
+
+		acpi_iort_register_irq(hw_irq, "arm-smmu-context", trigger,
+				       &res[num_res++]);
+	}
+}
+
+static bool __init arm_smmu_is_coherent(struct acpi_iort_node *node)
+{
+	struct acpi_iort_smmu *smmu;
+
+	/* Retrieve SMMU specific data */
+	smmu = (struct acpi_iort_smmu *)node->node_data;
+
+	return smmu->flags & ACPI_IORT_SMMU_COHERENT_WALK;
+}
+
+struct iort_iommu_config {
+	const char *name;
+	int (*iommu_init)(struct acpi_iort_node *node);
+	bool (*iommu_is_coherent)(struct acpi_iort_node *node);
+	int (*iommu_count_resources)(struct acpi_iort_node *node);
+	void (*iommu_init_resources)(struct resource *res,
+				     struct acpi_iort_node *node);
+};
+
+static const struct iort_iommu_config iort_arm_smmu_v3_cfg __initconst = {
+	.name = "arm-smmu-v3",
+	.iommu_is_coherent = arm_smmu_v3_is_coherent,
+	.iommu_count_resources = arm_smmu_v3_count_resources,
+	.iommu_init_resources = arm_smmu_v3_init_resources
+};
+
+static const struct iort_iommu_config iort_arm_smmu_cfg __initconst = {
+	.name = "arm-smmu",
+	.iommu_is_coherent = arm_smmu_is_coherent,
+	.iommu_count_resources = arm_smmu_count_resources,
+	.iommu_init_resources = arm_smmu_init_resources
+};
+
+static __init
+const struct iort_iommu_config *iort_get_iommu_cfg(struct acpi_iort_node *node)
+{
+	switch (node->type) {
+	case ACPI_IORT_NODE_SMMU_V3:
+		return &iort_arm_smmu_v3_cfg;
+	case ACPI_IORT_NODE_SMMU:
+		return &iort_arm_smmu_cfg;
+	default:
+		return NULL;
+	}
+}
+
+/**
+ * iort_add_smmu_platform_device() - Allocate a platform device for SMMU
+ * @node: Pointer to SMMU ACPI IORT node
+ *
+ * Returns: 0 on success, <0 failure
+ */
+static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
+{
+	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);
+
+	ret = platform_device_add_resources(pdev, r, count);
+	/*
+	 * Resources are duplicated in platform_device_add_resources,
+	 * free their allocated memory
+	 */
+	kfree(r);
+
+	if (ret)
+		goto dev_put;
+
+	/*
+	 * 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;
+
+	fwnode = iort_get_fwnode(node);
+
+	if (!fwnode) {
+		ret = -ENODEV;
+		goto dev_put;
+	}
+
+	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);
+
+	ret = platform_device_add(pdev);
+	if (ret)
+		goto dma_deconfigure;
+
+	return 0;
+
+dma_deconfigure:
+	acpi_dma_deconfigure(&pdev->dev);
+dev_put:
+	platform_device_put(pdev);
+
+	return ret;
+}
+
+static void __init iort_init_platform_devices(void)
+{
+	struct acpi_iort_node *iort_node, *iort_end;
+	struct acpi_table_iort *iort;
+	struct fwnode_handle *fwnode;
+	int i, ret;
+
+	/*
+	 * iort_table and iort both point to the start of IORT table, but
+	 * have different struct types
+	 */
+	iort = (struct acpi_table_iort *)iort_table;
+
+	/* Get the first IORT node */
+	iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
+				 iort->node_offset);
+	iort_end = ACPI_ADD_PTR(struct acpi_iort_node, iort,
+				iort_table->length);
+
+	for (i = 0; i < iort->node_count; i++) {
+		if (iort_node >= iort_end) {
+			pr_err("iort node pointer overflows, bad table\n");
+			return;
+		}
+
+		if ((iort_node->type == ACPI_IORT_NODE_SMMU) ||
+			(iort_node->type == ACPI_IORT_NODE_SMMU_V3)) {
+
+			fwnode = acpi_alloc_fwnode_static();
+			if (!fwnode)
+				return;
+
+			iort_set_fwnode(iort_node, fwnode);
+
+			ret = iort_add_smmu_platform_device(iort_node);
+			if (ret) {
+				iort_delete_fwnode(iort_node);
+				acpi_free_fwnode_static(fwnode);
+				return;
+			}
+		}
+
+		iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort_node,
+					 iort_node->length);
+	}
+}
+
+void __init acpi_iort_init(void)
+{
+	acpi_status status;
+
+	status = acpi_get_table(ACPI_SIG_IORT, 0, &iort_table);
+	if (ACPI_FAILURE(status)) {
+		if (status != AE_NOT_FOUND) {
+			const char *msg = acpi_format_exception(status);
+
+			pr_err("Failed to get table, %s\n", msg);
+		}
+
+		return;
+	}
+
+	iort_init_platform_devices();
+
+	acpi_probe_device_table(iort);
+}
diff --git a/xen/include/acpi/acpi_iort.h b/xen/include/acpi/acpi_iort.h
new file mode 100644
index 0000000..77e0809
--- /dev/null
+++ b/xen/include/acpi/acpi_iort.h
@@ -0,0 +1,58 @@
+/*
+ * Copyright (C) 2016, Semihalf
+ *	Author: Tomasz Nowicki <tn@semihalf.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ */
+
+#ifndef __ACPI_IORT_H__
+#define __ACPI_IORT_H__
+
+#include <linux/acpi.h>
+#include <linux/fwnode.h>
+#include <linux/irqdomain.h>
+
+#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
+void acpi_iort_init(void);
+bool iort_node_match(u8 type);
+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);
+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; }
+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,
+							u32 req_id)
+{ return NULL; }
+/* IOMMU interface */
+static inline void iort_set_dma_mask(struct device *dev) { }
+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__ */
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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

Re: [RFC 5/6] ACPI: arm: Support for IORT

[Jan Beulich]

>>> On 08.06.17 at 21:30, <sgoel@codeaurora.org> wrote:
> Verbatim files from Linux kernel.

This is pretty odd - they won't even come close to compile in that
shape. But if the ARM folks are happy with it to be done this way,
so be it.

> iort.c: commit ca78d3173cff:Merge tag 'arm64-upstream'

Please don't name merge commits - there must be a real one (or
perhaps it's more than one, but anyway).

Jan


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

[RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Sameer Goel]

Add limited support for parsing IORT table to initialize SMMU devices.

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         | 232 +++++++++++++++++++-----------------
 xen/drivers/passthrough/arm/iommu.c |  15 +--
 xen/include/acpi/acpi.h             |   1 +
 xen/include/acpi/acpi_iort.h        |  25 ++--
 xen/include/asm-arm/device.h        |   2 +
 xen/include/xen/acpi.h              |  21 ++++
 xen/include/xen/lib.h               |   7 +-
 xen/include/xen/pci.h               |   1 +
 11 files changed, 184 insertions(+), 125 deletions(-)
 create mode 100644 xen/drivers/acpi/arm/Makefile

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 92a2de6..5dc93ff 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -753,6 +753,9 @@ void __init start_xen(unsigned long boot_phys_offset,
     /* Parse the ACPI tables for possible boot-time configuration */
     acpi_boot_table_init();
 
+    /* Initialize the IORT tables */
+    acpi_iort_init();
+
     if ( acpi_disabled )
         printk("Booting using Device Tree\n");
     else
diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
index 444b11d..4165318 100644
--- a/xen/drivers/acpi/Makefile
+++ b/xen/drivers/acpi/Makefile
@@ -1,5 +1,6 @@
 subdir-y += tables
 subdir-y += utilities
+subdir-$(CONFIG_ARM_64) += 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 4a5bb96..c22ec31 100644
--- a/xen/drivers/acpi/arm/iort.c
+++ b/xen/drivers/acpi/arm/iort.c
@@ -14,29 +14,40 @@
  * 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 <xen/acpi.h>
+#include <xen/fwnode.h>
+#include <xen/iommu.h>
+#include <xen/lib.h>
+#include <xen/list.h>
+#include <xen/pci.h>
+
+#include <asm/device.h>
 
-#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>
 
 #define IORT_TYPE_MASK(type)	(1 << (type))
 #define IORT_MSI_TYPE		(1 << ACPI_IORT_NODE_ITS_GROUP)
 #define IORT_IOMMU_TYPE		((1 << ACPI_IORT_NODE_SMMU) |	\
 				(1 << ACPI_IORT_NODE_SMMU_V3))
 
+#if 0
 struct iort_its_msi_chip {
 	struct list_head	list;
 	struct fwnode_handle	*fw_node;
 	u32			translation_id;
 };
 
+#endif
+
 struct iort_fwnode {
 	struct list_head list;
 	struct acpi_iort_node *iort_node;
@@ -60,7 +71,7 @@ static inline int iort_set_fwnode(struct acpi_iort_node *iort_node,
 {
 	struct iort_fwnode *np;
 
-	np = kzalloc(sizeof(struct iort_fwnode), GFP_ATOMIC);
+	np = xzalloc(struct iort_fwnode);
 
 	if (WARN_ON(!np))
 		return -ENOMEM;
@@ -114,7 +125,7 @@ static inline void iort_delete_fwnode(struct acpi_iort_node *node)
 	list_for_each_entry_safe(curr, tmp, &iort_fwnode_list, list) {
 		if (curr->iort_node == node) {
 			list_del(&curr->list);
-			kfree(curr);
+			xfree(curr);
 			break;
 		}
 	}
@@ -127,6 +138,7 @@ typedef acpi_status (*iort_find_node_callback)
 /* Root pointer to the mapped IORT table */
 static struct acpi_table_header *iort_table;
 
+#if 0
 static LIST_HEAD(iort_msi_chip_list);
 static DEFINE_SPINLOCK(iort_msi_chip_lock);
 
@@ -199,7 +211,7 @@ struct fwnode_handle *iort_find_domain_token(int trans_id)
 
 	return fw_node;
 }
-
+#endif
 static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
 					     iort_find_node_callback callback,
 					     void *context)
@@ -219,9 +231,10 @@ static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
 				iort_table->length);
 
 	for (i = 0; i < iort->node_count; i++) {
-		if (WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND,
-			       "IORT node pointer overflows, bad table!\n"))
+		if (iort_node >= iort_end) {
+			printk(XENLOG_ERR "IORT node pointer overflows, bad table!\n");
 			return NULL;
+		}
 
 		if (iort_node->type == type &&
 		    ACPI_SUCCESS(callback(iort_node, context)))
@@ -249,6 +262,14 @@ bool iort_node_match(u8 type)
 	return node != NULL;
 }
 
+/*
+ * Following 2 definies should come from the PCI passthrough implementation.
+ * Based on the current pci_dev define the bus number and seg number come
+ * from pci_dev so making an API assumption
+ */
+#define to_pci_dev(p) container_of(p, struct pci_dev,dev)
+#define pci_domain_nr(dev) dev->seg
+
 static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
 					    void *context)
 {
@@ -256,6 +277,11 @@ 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;
+/*
+ * Named components not supported yet.
+ */
+#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 +301,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;
 
-		bus = to_pci_bus(dev);
+		pci_dev = to_pci_dev(dev);
 		pci_rc = (struct acpi_iort_root_complex *)node->node_data;
 
 		/*
@@ -287,12 +314,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;
 }
 
@@ -307,7 +333,8 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
 			return 0;
 		}
 
-		pr_warn(FW_BUG "[map %p] SINGLE MAPPING flag not allowed for node type %d, skipping ID map\n",
+		printk(XENLOG_WARNING "[map %p] SINGLE MAPPING flag not \
+		       allowed for node type %d, skipping ID map\n",
 			map, type);
 		return -ENXIO;
 	}
@@ -320,6 +347,11 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
 	return 0;
 }
 
+/*
+ * Name components are not supported yet so we do not need the
+ * iort_node_get_id function
+ */
+#if 0
 static
 struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
 					u32 *id_out, u8 type_mask,
@@ -337,8 +369,8 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
 
 	/* Firmware bug! */
 	if (!map->output_reference) {
-		pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
-		       node, node->type);
+		printk(XENLOG_ERR "[node %p type %d] ID map has NULL parent \
+		       reference\n", node, node->type);
 		return NULL;
 	}
 
@@ -358,6 +390,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,
@@ -384,8 +417,8 @@ static struct acpi_iort_node *iort_node_map_rid(struct acpi_iort_node *node,
 
 		/* Firmware bug! */
 		if (!map->output_reference) {
-			pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
-			       node, node->type);
+			printk(XENLOG_ERR "[node %p type %d] ID map has NULL \
+			       parent reference\n", node, node->type);
 			goto fail_map;
 		}
 
@@ -410,6 +443,10 @@ fail_map:
 	return NULL;
 }
 
+/* Xen: Comment out the NamedComponent and ITS mapping code till the support
+ * is available.
+ * */
+#if 0
 static struct acpi_iort_node *iort_find_dev_node(struct device *dev)
 {
 	struct pci_bus *pbus;
@@ -503,6 +540,9 @@ struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id)
 	return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
 }
 
+/*
+ * RID is the same as PCI_DEVID(BDF) for QDF2400
+ */
 static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
 {
 	u32 *rid = data;
@@ -510,7 +550,7 @@ static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
 	*rid = alias;
 	return 0;
 }
-
+#endif
 static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
 			       struct fwnode_handle *fwnode,
 			       const struct iommu_ops *ops)
@@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
 	return ret;
 }
 
-static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
-					struct acpi_iort_node *node,
-					u32 streamid)
+static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
+			    u32 streamid)
 {
-	const struct iommu_ops *ops = NULL;
 	int ret = -ENODEV;
 	struct fwnode_handle *iort_fwnode;
 
 	if (node) {
 		iort_fwnode = iort_get_fwnode(node);
 		if (!iort_fwnode)
-			return NULL;
-
-		ops = iommu_ops_from_fwnode(iort_fwnode);
-		if (!ops)
-			return NULL;
+			return ret;
 
-		ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
+		ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, NULL);
 	}
 
-	return ret ? NULL : ops;
+	return ret;
 }
 
+#if 0 /* Xen: We do not need this function for Xen */
 /**
  * iort_set_dma_mask - Set-up dma mask for a device.
  *
@@ -567,39 +602,43 @@ void iort_set_dma_mask(struct device *dev)
 	if (!dev->dma_mask)
 		dev->dma_mask = &dev->coherent_dma_mask;
 }
-
+#endif
 /**
- * iort_iommu_configure - Set-up IOMMU configuration for a device.
+ * iort_iommu_configure - Set-up IOMMU configuration for a device. This
+ * function sets up the fwspec as needed for a given device. Only PCI
+ * devices are supported for now.
  *
  * @dev: device to configure
  *
- * Returns: iommu_ops pointer on configuration success
- *          NULL on configuration failure
+ * Returns: Appropriate acpi_status
  */
-const struct iommu_ops *iort_iommu_configure(struct device *dev)
+acpi_status iort_iommu_configure(struct device *dev)
 {
 	struct acpi_iort_node *node, *parent;
-	const struct iommu_ops *ops = NULL;
 	u32 streamid = 0;
+	acpi_status status = AE_OK;
 
 	if (dev_is_pci(dev)) {
-		struct pci_bus *bus = to_pci_dev(dev)->bus;
+		struct pci_dev *pci_device = to_pci_dev(dev);
 		u32 rid;
 
-		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
-				       &rid);
+		rid = PCI_BDF2(pci_device->bus,pci_device->devfn);
 
 		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;
+			return AE_NOT_FOUND;
 
 		parent = iort_node_map_rid(node, rid, &streamid,
 					   IORT_IOMMU_TYPE);
 
-		ops = iort_iommu_xlate(dev, parent, streamid);
+		status = iort_iommu_xlate(dev, parent, streamid);
+
+		status = status ? AE_ERROR : AE_OK;
 
 	} else {
+		status = AE_NOT_IMPLEMENTED;
+#if 0
 		int i = 0;
 
 		node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
@@ -616,11 +655,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;
+	return status;
 }
 
+/*
+ * Xen: Not using the parsin 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,93 +852,63 @@ const struct iort_iommu_config *iort_get_iommu_cfg(struct acpi_iort_node *node)
 		return NULL;
 	}
 }
-
+#endif
 /**
- * iort_add_smmu_platform_device() - Allocate a platform device for SMMU
+ * Xen: rename the function to iort_add_smmu_device
+ * iort_add_smmu_device() - Allocate a device for SMMU
  * @node: Pointer to SMMU ACPI IORT node
  *
  * Returns: 0 on success, <0 failure
  */
-static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
+static int __init iort_add_smmu_device(struct acpi_iort_node *node)
 {
 	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);
+	struct device *dev;
+	int ret;
 
-	r = kcalloc(count, sizeof(*r), GFP_KERNEL);
-	if (!r) {
-		ret = -ENOMEM;
-		goto dev_put;
-	}
-
-	ops->iommu_init_resources(r, node);
-
-	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 relevent code as
+	 * compared to base revision.
 	 */
-	kfree(r);
 
-	if (ret)
-		goto dev_put;
+	dev = xzalloc(struct device);
+	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; /* This should not be needed and we can check
+				 fwnode. Defining for legacy support */
+	dev->acpi_node = node;/* A copy of the node does not seem necessary */ 
 
 	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:
+	xfree(dev);
 
 	return ret;
 }
 
-static void __init iort_init_platform_devices(void)
+/*
+ * Xen: Rename the function to iort_init_devices as this function will
+ * populate the device object for SMMU devices.
+ */
+static void __init iort_init_devices(void)
 {
 	struct acpi_iort_node *iort_node, *iort_end;
 	struct acpi_table_iort *iort;
@@ -914,7 +929,7 @@ static void __init iort_init_platform_devices(void)
 
 	for (i = 0; i < iort->node_count; i++) {
 		if (iort_node >= iort_end) {
-			pr_err("iort node pointer overflows, bad table\n");
+			printk(XENLOG_ERR "iort node pointer overflows, bad table\n");
 			return;
 		}
 
@@ -927,7 +942,7 @@ static void __init iort_init_platform_devices(void)
 
 			iort_set_fwnode(iort_node, fwnode);
 
-			ret = iort_add_smmu_platform_device(iort_node);
+			ret = iort_add_smmu_device(iort_node);
 			if (ret) {
 				iort_delete_fwnode(iort_node);
 				acpi_free_fwnode_static(fwnode);
@@ -949,13 +964,14 @@ void __init acpi_iort_init(void)
 		if (status != AE_NOT_FOUND) {
 			const char *msg = acpi_format_exception(status);
 
-			pr_err("Failed to get table, %s\n", msg);
+			printk(XENLOG_ERR "Failed to get table, %s\n", msg);
 		}
 
 		return;
 	}
 
-	iort_init_platform_devices();
+	iort_init_devices();
 
-	acpi_probe_device_table(iort);
+        /* Not used for now */
+	//acpi_probe_device_table(iort);
 }
diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index edf70c2..1397da5 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -74,24 +74,26 @@ int arch_iommu_populate_page_table(struct domain *d)
     return -ENOSYS;
 }
 
+/*
+ * The ops parameter in this function will always be NULL for Xen,
+ * as the ops are set per domain.
+ */
 int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
         const struct iommu_ops *ops)
 {
     struct iommu_fwspec *fwspec = dev->iommu_fwspec;
 
+    /*
+     * fwspec is already allocated for this device.
+     */
     if (fwspec)
-        return ops == fwspec->ops ? 0 : -EINVAL;
+        return 0;
 
     fwspec = xzalloc(struct iommu_fwspec);
     if (!fwspec)
         return -ENOMEM;
 
-    /* Ref counting for the dt device node is not needed */
-
-    /*of_node_get(to_of_node(iommu_fwnode));*/
-
     fwspec->iommu_fwnode = iommu_fwnode;
-    fwspec->ops = ops;
     dev->iommu_fwspec = fwspec;
     return 0;
 }
@@ -101,7 +103,6 @@ void iommu_fwspec_free(struct device *dev)
     struct iommu_fwspec *fwspec = dev->iommu_fwspec;
 
     if (fwspec) {
-        /*fwnode_handle_put(fwspec->iommu_fwnode);*/
         xfree(fwspec);
         dev->iommu_fwspec = NULL;
     }
diff --git a/xen/include/acpi/acpi.h b/xen/include/acpi/acpi.h
index c852701..1ac92b2 100644
--- a/xen/include/acpi/acpi.h
+++ b/xen/include/acpi/acpi.h
@@ -60,6 +60,7 @@
 #include "actbl.h"		/* ACPI table definitions */
 #include "aclocal.h"		/* Internal data types */
 #include "acoutput.h"		/* Error output and Debug macros */
+#include "acpi_iort.h"          /* Utility defines for IORT */
 #include "acpiosxf.h"		/* Interfaces to the ACPI-to-OS layer */
 #include "acpixf.h"		/* ACPI core subsystem external interfaces */
 #include "acglobal.h"		/* All global variables */
diff --git a/xen/include/acpi/acpi_iort.h b/xen/include/acpi/acpi_iort.h
index 77e0809..c0b5b8d 100644
--- a/xen/include/acpi/acpi_iort.h
+++ b/xen/include/acpi/acpi_iort.h
@@ -19,27 +19,35 @@
 #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>
 
+/*
+ * We are not using IORT IRQ bindings for this change set
+ */
+#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
 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);
-const struct iommu_ops *iort_iommu_configure(struct device *dev);
+#endif
+acpi_status 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 +55,11 @@ 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; }
+acpi_status iommu_ops iort_iommu_configure(struct device *dev)
+{ return AE_NOT_IMPLEMENTED; }
 #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 */
     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 30ec0ee..2106ba9 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>
 
 #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 (WARN_ON(!fwnode || fwnode->type != FWNODE_ACPI_STATIC))
+		return;
+
+	xfree(fwnode);
+}
 #ifdef CONFIG_ACPI
 
 enum acpi_interrupt_id {
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 995a85a..3785fae 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -9,7 +9,12 @@
 #include <asm/bug.h>
 
 #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
-#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
+#define WARN_ON(p) ({                                      \
+    int __ret_warn_on = !!(p);                             \
+    if (unlikely(__ret_warn_on))                           \
+        WARN();                                            \
+    unlikely(__ret_warn_on);                               \
+})
 
 #if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
 /* Force a compilation error if condition is true */
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 59b6e8a..c518569 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -88,6 +88,7 @@ struct pci_dev {
 #define PT_FAULT_THRESHOLD 10
     } fault;
     u64 vf_rlen[6];
+    struct device dev;
 };
 
 #define for_each_pdev(domain, pdev) \
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 26229 bytes --]

CCing Jan

On Thu, 8 Jun 2017, Sameer Goel wrote:
> Add limited support for parsing IORT table to initialize SMMU devices.
> 
> 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         | 232 +++++++++++++++++++-----------------
>  xen/drivers/passthrough/arm/iommu.c |  15 +--
>  xen/include/acpi/acpi.h             |   1 +
>  xen/include/acpi/acpi_iort.h        |  25 ++--
>  xen/include/asm-arm/device.h        |   2 +
>  xen/include/xen/acpi.h              |  21 ++++
>  xen/include/xen/lib.h               |   7 +-
>  xen/include/xen/pci.h               |   1 +
>  11 files changed, 184 insertions(+), 125 deletions(-)
>  create mode 100644 xen/drivers/acpi/arm/Makefile

This patch doesn't apply with "git am" and doesn't build on x86 with:

In file included from /local/repos/xen-upstream/xen/include/xen/iommu.h:24:0,
                 from /local/repos/xen-upstream/xen/include/asm/hvm/domain.h:23,
                 from /local/repos/xen-upstream/xen/include/asm/domain.h:7,
                 from /local/repos/xen-upstream/xen/include/xen/domain.h:8,
                 from /local/repos/xen-upstream/xen/include/xen/sched.h:11,
                 from x86_64/asm-offsets.c:9:
/local/repos/xen-upstream/xen/include/xen/pci.h:91:19: error: field ‘dev’ has incomplete type
     struct device dev;
                   ^
In file included from /local/repos/xen-upstream/xen/include/acpi/acpi.h:63:0,
                 from /local/repos/xen-upstream/xen/include/xen/acpi.h:33,
                 from /local/repos/xen-upstream/xen/include/asm/fixmap.h:21,
                 from x86_64/asm-offsets.c:12:
/local/repos/xen-upstream/xen/include/acpi/acpi_iort.h:60:23: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘iort_iommu_configure’
 acpi_status iommu_ops iort_iommu_configure(struct device *dev)
                       ^
make[3]: *** [asm-offsets.s] Error 1


And it doesn't build on arm64 with:

/local/repos/xen-upstream/xen/arch/arm/setup.c:765: undefined reference to `acpi_iort_init'
/local/repos/gcc-linaro-4.9-2014.05-aarch64-linux-gnu-x86_64-linux-gnu/bin/aarch64-linux-gnu-ld: /local/repos/xen-upstream/xen/.xen-syms.0: hidden symbol `acpi_iort_init' isn't defined
/local/repos/gcc-linaro-4.9-2014.05-aarch64-linux-gnu-x86_64-linux-gnu/bin/aarch64-linux-gnu-ld: final link failed: Bad value
make[3]: *** [/local/repos/xen-upstream/xen/xen-syms] Error 1


> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 92a2de6..5dc93ff 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -753,6 +753,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>      /* Parse the ACPI tables for possible boot-time configuration */
>      acpi_boot_table_init();
>  
> +    /* Initialize the IORT tables */
> +    acpi_iort_init();
> +
>      if ( acpi_disabled )
>          printk("Booting using Device Tree\n");
>      else
> diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
> index 444b11d..4165318 100644
> --- a/xen/drivers/acpi/Makefile
> +++ b/xen/drivers/acpi/Makefile
> @@ -1,5 +1,6 @@
>  subdir-y += tables
>  subdir-y += utilities
> +subdir-$(CONFIG_ARM_64) += 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 4a5bb96..c22ec31 100644
> --- a/xen/drivers/acpi/arm/iort.c
> +++ b/xen/drivers/acpi/arm/iort.c
> @@ -14,29 +14,40 @@
>   * 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 <xen/acpi.h>
> +#include <xen/fwnode.h>
> +#include <xen/iommu.h>
> +#include <xen/lib.h>
> +#include <xen/list.h>
> +#include <xen/pci.h>
> +
> +#include <asm/device.h>
>  
> -#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>
>  
>  #define IORT_TYPE_MASK(type)	(1 << (type))
>  #define IORT_MSI_TYPE		(1 << ACPI_IORT_NODE_ITS_GROUP)
>  #define IORT_IOMMU_TYPE		((1 << ACPI_IORT_NODE_SMMU) |	\
>  				(1 << ACPI_IORT_NODE_SMMU_V3))
>  
> +#if 0
>  struct iort_its_msi_chip {
>  	struct list_head	list;
>  	struct fwnode_handle	*fw_node;
>  	u32			translation_id;
>  };
>  
> +#endif
> +
>  struct iort_fwnode {
>  	struct list_head list;
>  	struct acpi_iort_node *iort_node;
> @@ -60,7 +71,7 @@ static inline int iort_set_fwnode(struct acpi_iort_node *iort_node,
>  {
>  	struct iort_fwnode *np;
>  
> -	np = kzalloc(sizeof(struct iort_fwnode), GFP_ATOMIC);
> +	np = xzalloc(struct iort_fwnode);
>  
>  	if (WARN_ON(!np))
>  		return -ENOMEM;
> @@ -114,7 +125,7 @@ static inline void iort_delete_fwnode(struct acpi_iort_node *node)
>  	list_for_each_entry_safe(curr, tmp, &iort_fwnode_list, list) {
>  		if (curr->iort_node == node) {
>  			list_del(&curr->list);
> -			kfree(curr);
> +			xfree(curr);
>  			break;
>  		}
>  	}
> @@ -127,6 +138,7 @@ typedef acpi_status (*iort_find_node_callback)
>  /* Root pointer to the mapped IORT table */
>  static struct acpi_table_header *iort_table;
>  
> +#if 0
>  static LIST_HEAD(iort_msi_chip_list);
>  static DEFINE_SPINLOCK(iort_msi_chip_lock);
>  
> @@ -199,7 +211,7 @@ struct fwnode_handle *iort_find_domain_token(int trans_id)
>  
>  	return fw_node;
>  }
> -
> +#endif
>  static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
>  					     iort_find_node_callback callback,
>  					     void *context)
> @@ -219,9 +231,10 @@ static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
>  				iort_table->length);
>  
>  	for (i = 0; i < iort->node_count; i++) {
> -		if (WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND,
> -			       "IORT node pointer overflows, bad table!\n"))
> +		if (iort_node >= iort_end) {
> +			printk(XENLOG_ERR "IORT node pointer overflows, bad table!\n");
>  			return NULL;
> +		}
>  
>  		if (iort_node->type == type &&
>  		    ACPI_SUCCESS(callback(iort_node, context)))
> @@ -249,6 +262,14 @@ bool iort_node_match(u8 type)
>  	return node != NULL;
>  }
>  
> +/*
> + * Following 2 definies should come from the PCI passthrough implementation.
> + * Based on the current pci_dev define the bus number and seg number come
> + * from pci_dev so making an API assumption
> + */
> +#define to_pci_dev(p) container_of(p, struct pci_dev,dev)
> +#define pci_domain_nr(dev) dev->seg
> +
>  static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
>  					    void *context)
>  {
> @@ -256,6 +277,11 @@ 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;
> +/*
> + * Named components not supported yet.
> + */
> +#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 +301,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;
>  
> -		bus = to_pci_bus(dev);
> +		pci_dev = to_pci_dev(dev);
>  		pci_rc = (struct acpi_iort_root_complex *)node->node_data;
>  
>  		/*
> @@ -287,12 +314,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;
>  }
>  
> @@ -307,7 +333,8 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>  			return 0;
>  		}
>  
> -		pr_warn(FW_BUG "[map %p] SINGLE MAPPING flag not allowed for node type %d, skipping ID map\n",
> +		printk(XENLOG_WARNING "[map %p] SINGLE MAPPING flag not \
> +		       allowed for node type %d, skipping ID map\n",
>  			map, type);
>  		return -ENXIO;
>  	}
> @@ -320,6 +347,11 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>  	return 0;
>  }
>  
> +/*
> + * Name components are not supported yet so we do not need the
> + * iort_node_get_id function
> + */
> +#if 0
>  static
>  struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>  					u32 *id_out, u8 type_mask,
> @@ -337,8 +369,8 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>  
>  	/* Firmware bug! */
>  	if (!map->output_reference) {
> -		pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
> -		       node, node->type);
> +		printk(XENLOG_ERR "[node %p type %d] ID map has NULL parent \
> +		       reference\n", node, node->type);
>  		return NULL;
>  	}
>  
> @@ -358,6 +390,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,
> @@ -384,8 +417,8 @@ static struct acpi_iort_node *iort_node_map_rid(struct acpi_iort_node *node,
>  
>  		/* Firmware bug! */
>  		if (!map->output_reference) {
> -			pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
> -			       node, node->type);
> +			printk(XENLOG_ERR "[node %p type %d] ID map has NULL \
> +			       parent reference\n", node, node->type);
>  			goto fail_map;
>  		}
>  
> @@ -410,6 +443,10 @@ fail_map:
>  	return NULL;
>  }
>  
> +/* Xen: Comment out the NamedComponent and ITS mapping code till the support
> + * is available.
> + * */
> +#if 0
>  static struct acpi_iort_node *iort_find_dev_node(struct device *dev)
>  {
>  	struct pci_bus *pbus;
> @@ -503,6 +540,9 @@ struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id)
>  	return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
>  }
>  
> +/*
> + * RID is the same as PCI_DEVID(BDF) for QDF2400
> + */
>  static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
>  {
>  	u32 *rid = data;
> @@ -510,7 +550,7 @@ static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
>  	*rid = alias;
>  	return 0;
>  }
> -
> +#endif
>  static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>  			       struct fwnode_handle *fwnode,
>  			       const struct iommu_ops *ops)
> @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>  	return ret;
>  }
>  
> -static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
> -					struct acpi_iort_node *node,
> -					u32 streamid)
> +static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
> +			    u32 streamid)
>  {
> -	const struct iommu_ops *ops = NULL;
>  	int ret = -ENODEV;
>  	struct fwnode_handle *iort_fwnode;
>  
>  	if (node) {
>  		iort_fwnode = iort_get_fwnode(node);
>  		if (!iort_fwnode)
> -			return NULL;
> -
> -		ops = iommu_ops_from_fwnode(iort_fwnode);
> -		if (!ops)
> -			return NULL;
> +			return ret;
>  
> -		ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
> +		ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, NULL);
>  	}
>  
> -	return ret ? NULL : ops;
> +	return ret;
>  }
>  
> +#if 0 /* Xen: We do not need this function for Xen */
>  /**
>   * iort_set_dma_mask - Set-up dma mask for a device.
>   *
> @@ -567,39 +602,43 @@ void iort_set_dma_mask(struct device *dev)
>  	if (!dev->dma_mask)
>  		dev->dma_mask = &dev->coherent_dma_mask;
>  }
> -
> +#endif
>  /**
> - * iort_iommu_configure - Set-up IOMMU configuration for a device.
> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
> + * function sets up the fwspec as needed for a given device. Only PCI
> + * devices are supported for now.
>   *
>   * @dev: device to configure
>   *
> - * Returns: iommu_ops pointer on configuration success
> - *          NULL on configuration failure
> + * Returns: Appropriate acpi_status
>   */
> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
> +acpi_status iort_iommu_configure(struct device *dev)
>  {
>  	struct acpi_iort_node *node, *parent;
> -	const struct iommu_ops *ops = NULL;
>  	u32 streamid = 0;
> +	acpi_status status = AE_OK;
>  
>  	if (dev_is_pci(dev)) {
> -		struct pci_bus *bus = to_pci_dev(dev)->bus;
> +		struct pci_dev *pci_device = to_pci_dev(dev);
>  		u32 rid;
>  
> -		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
> -				       &rid);
> +		rid = PCI_BDF2(pci_device->bus,pci_device->devfn);
>  
>  		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;
> +			return AE_NOT_FOUND;
>  
>  		parent = iort_node_map_rid(node, rid, &streamid,
>  					   IORT_IOMMU_TYPE);
>  
> -		ops = iort_iommu_xlate(dev, parent, streamid);
> +		status = iort_iommu_xlate(dev, parent, streamid);
> +
> +		status = status ? AE_ERROR : AE_OK;
>  
>  	} else {
> +		status = AE_NOT_IMPLEMENTED;
> +#if 0
>  		int i = 0;
>  
>  		node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> @@ -616,11 +655,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;
> +	return status;
>  }
>  
> +/*
> + * Xen: Not using the parsin 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,93 +852,63 @@ const struct iort_iommu_config *iort_get_iommu_cfg(struct acpi_iort_node *node)
>  		return NULL;
>  	}
>  }
> -
> +#endif
>  /**
> - * iort_add_smmu_platform_device() - Allocate a platform device for SMMU
> + * Xen: rename the function to iort_add_smmu_device
> + * iort_add_smmu_device() - Allocate a device for SMMU
>   * @node: Pointer to SMMU ACPI IORT node
>   *
>   * Returns: 0 on success, <0 failure
>   */
> -static int __init iort_add_smmu_platform_device(struct acpi_iort_node *node)
> +static int __init iort_add_smmu_device(struct acpi_iort_node *node)
>  {
>  	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);
> +	struct device *dev;
> +	int ret;
>  
> -	r = kcalloc(count, sizeof(*r), GFP_KERNEL);
> -	if (!r) {
> -		ret = -ENOMEM;
> -		goto dev_put;
> -	}
> -
> -	ops->iommu_init_resources(r, node);
> -
> -	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 relevent code as
> +	 * compared to base revision.
>  	 */
> -	kfree(r);
>  
> -	if (ret)
> -		goto dev_put;
> +	dev = xzalloc(struct device);
> +	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; /* This should not be needed and we can check
> +				 fwnode. Defining for legacy support */
> +	dev->acpi_node = node;/* A copy of the node does not seem necessary */ 
>  
>  	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:
> +	xfree(dev);
>  
>  	return ret;
>  }
>  
> -static void __init iort_init_platform_devices(void)
> +/*
> + * Xen: Rename the function to iort_init_devices as this function will
> + * populate the device object for SMMU devices.
> + */
> +static void __init iort_init_devices(void)
>  {
>  	struct acpi_iort_node *iort_node, *iort_end;
>  	struct acpi_table_iort *iort;
> @@ -914,7 +929,7 @@ static void __init iort_init_platform_devices(void)
>  
>  	for (i = 0; i < iort->node_count; i++) {
>  		if (iort_node >= iort_end) {
> -			pr_err("iort node pointer overflows, bad table\n");
> +			printk(XENLOG_ERR "iort node pointer overflows, bad table\n");
>  			return;
>  		}
>  
> @@ -927,7 +942,7 @@ static void __init iort_init_platform_devices(void)
>  
>  			iort_set_fwnode(iort_node, fwnode);
>  
> -			ret = iort_add_smmu_platform_device(iort_node);
> +			ret = iort_add_smmu_device(iort_node);
>  			if (ret) {
>  				iort_delete_fwnode(iort_node);
>  				acpi_free_fwnode_static(fwnode);
> @@ -949,13 +964,14 @@ void __init acpi_iort_init(void)
>  		if (status != AE_NOT_FOUND) {
>  			const char *msg = acpi_format_exception(status);
>  
> -			pr_err("Failed to get table, %s\n", msg);
> +			printk(XENLOG_ERR "Failed to get table, %s\n", msg);
>  		}
>  
>  		return;
>  	}
>  
> -	iort_init_platform_devices();
> +	iort_init_devices();
>  
> -	acpi_probe_device_table(iort);
> +        /* Not used for now */
> +	//acpi_probe_device_table(iort);
>  }
> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
> index edf70c2..1397da5 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -74,24 +74,26 @@ int arch_iommu_populate_page_table(struct domain *d)
>      return -ENOSYS;
>  }
>  
> +/*
> + * The ops parameter in this function will always be NULL for Xen,
> + * as the ops are set per domain.
> + */
>  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>          const struct iommu_ops *ops)
>  {
>      struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>  
> +    /*
> +     * fwspec is already allocated for this device.
> +     */
>      if (fwspec)
> -        return ops == fwspec->ops ? 0 : -EINVAL;
> +        return 0;
>  
>      fwspec = xzalloc(struct iommu_fwspec);
>      if (!fwspec)
>          return -ENOMEM;
>  
> -    /* Ref counting for the dt device node is not needed */
> -
> -    /*of_node_get(to_of_node(iommu_fwnode));*/
> -
>      fwspec->iommu_fwnode = iommu_fwnode;
> -    fwspec->ops = ops;
>      dev->iommu_fwspec = fwspec;
>      return 0;
>  }
> @@ -101,7 +103,6 @@ void iommu_fwspec_free(struct device *dev)
>      struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>  
>      if (fwspec) {
> -        /*fwnode_handle_put(fwspec->iommu_fwnode);*/
>          xfree(fwspec);
>          dev->iommu_fwspec = NULL;
>      }
> diff --git a/xen/include/acpi/acpi.h b/xen/include/acpi/acpi.h
> index c852701..1ac92b2 100644
> --- a/xen/include/acpi/acpi.h
> +++ b/xen/include/acpi/acpi.h
> @@ -60,6 +60,7 @@
>  #include "actbl.h"		/* ACPI table definitions */
>  #include "aclocal.h"		/* Internal data types */
>  #include "acoutput.h"		/* Error output and Debug macros */
> +#include "acpi_iort.h"          /* Utility defines for IORT */
>  #include "acpiosxf.h"		/* Interfaces to the ACPI-to-OS layer */
>  #include "acpixf.h"		/* ACPI core subsystem external interfaces */
>  #include "acglobal.h"		/* All global variables */
> diff --git a/xen/include/acpi/acpi_iort.h b/xen/include/acpi/acpi_iort.h
> index 77e0809..c0b5b8d 100644
> --- a/xen/include/acpi/acpi_iort.h
> +++ b/xen/include/acpi/acpi_iort.h
> @@ -19,27 +19,35 @@
>  #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>
>  
> +/*
> + * We are not using IORT IRQ bindings for this change set
> + */
> +#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
>  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);
> -const struct iommu_ops *iort_iommu_configure(struct device *dev);
> +#endif
> +acpi_status 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 +55,11 @@ 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; }
> +acpi_status iommu_ops iort_iommu_configure(struct device *dev)
> +{ return AE_NOT_IMPLEMENTED; }
>  #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 */
>      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 30ec0ee..2106ba9 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>
>  
>  #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 (WARN_ON(!fwnode || fwnode->type != FWNODE_ACPI_STATIC))
> +		return;
> +
> +	xfree(fwnode);
> +}
>  #ifdef CONFIG_ACPI
>  
>  enum acpi_interrupt_id {
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 995a85a..3785fae 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -9,7 +9,12 @@
>  #include <asm/bug.h>
>  
>  #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
> +#define WARN_ON(p) ({                                      \
> +    int __ret_warn_on = !!(p);                             \
> +    if (unlikely(__ret_warn_on))                           \
> +        WARN();                                            \
> +    unlikely(__ret_warn_on);                               \
> +})

Why this change?


>  #if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
>  /* Force a compilation error if condition is true */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 59b6e8a..c518569 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -88,6 +88,7 @@ struct pci_dev {
>  #define PT_FAULT_THRESHOLD 10
>      } fault;
>      u64 vf_rlen[6];
> +    struct device dev;
>  };
>  
>  #define for_each_pdev(domain, pdev) \

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Robin Murphy]

On 08/06/17 20:30, Sameer Goel wrote:
[...]
>  /**
> - * iort_iommu_configure - Set-up IOMMU configuration for a device.
> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
> + * function sets up the fwspec as needed for a given device. Only PCI
> + * devices are supported for now.
>   *
>   * @dev: device to configure
>   *
> - * Returns: iommu_ops pointer on configuration success
> - *          NULL on configuration failure
> + * Returns: Appropriate acpi_status
>   */
> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
> +acpi_status iort_iommu_configure(struct device *dev)
>  {
>  	struct acpi_iort_node *node, *parent;
> -	const struct iommu_ops *ops = NULL;
>  	u32 streamid = 0;
> +	acpi_status status = AE_OK;
>  
>  	if (dev_is_pci(dev)) {
> -		struct pci_bus *bus = to_pci_dev(dev)->bus;
> +		struct pci_dev *pci_device = to_pci_dev(dev);
>  		u32 rid;
>  
> -		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
> -				       &rid);
> +		rid = PCI_BDF2(pci_device->bus,pci_device->devfn);

Beware that the Linux code isn't actually correct to begin with[1]. I
don't know how much Xen deals with PCI bridges and quirks, but as it
stands you should be able to trivially expose the flaw here by plugging
in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs
(or even both) kick up stream table faults.

Robin.

[1]:http://www.spinics.net/lists/linux-acpi/msg74844.html

>  
>  		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;
> +			return AE_NOT_FOUND;
>  
>  		parent = iort_node_map_rid(node, rid, &streamid,
>  					   IORT_IOMMU_TYPE);
>  
> -		ops = iort_iommu_xlate(dev, parent, streamid);
> +		status = iort_iommu_xlate(dev, parent, streamid);
> +
> +		status = status ? AE_ERROR : AE_OK;
>  
>  	} else {
> +		status = AE_NOT_IMPLEMENTED;
> +#if 0
>  		int i = 0;
>  
>  		node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> @@ -616,11 +655,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;
> +	return status;
>  }

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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Julien Grall]

(CC x86 folks)

Hi,

On 09/06/17 12:15, Robin Murphy wrote:
> On 08/06/17 20:30, Sameer Goel wrote:
> [...]
>>  /**
>> - * iort_iommu_configure - Set-up IOMMU configuration for a device.
>> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
>> + * function sets up the fwspec as needed for a given device. Only PCI
>> + * devices are supported for now.
>>   *
>>   * @dev: device to configure
>>   *
>> - * Returns: iommu_ops pointer on configuration success
>> - *          NULL on configuration failure
>> + * Returns: Appropriate acpi_status
>>   */
>> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
>> +acpi_status iort_iommu_configure(struct device *dev)
>>  {
>>  	struct acpi_iort_node *node, *parent;
>> -	const struct iommu_ops *ops = NULL;
>>  	u32 streamid = 0;
>> +	acpi_status status = AE_OK;
>>
>>  	if (dev_is_pci(dev)) {
>> -		struct pci_bus *bus = to_pci_dev(dev)->bus;
>> +		struct pci_dev *pci_device = to_pci_dev(dev);
>>  		u32 rid;
>>
>> -		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
>> -				       &rid);
>> +		rid = PCI_BDF2(pci_device->bus,pci_device->devfn);
>
> Beware that the Linux code isn't actually correct to begin with[1]. I
> don't know how much Xen deals with PCI bridges and quirks, but as it
> stands you should be able to trivially expose the flaw here by plugging
> in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs
> (or even both) kick up stream table faults.
>
> Robin.
>
> [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html

I am not sure how x86 is handling that. The closest I can find would be 
domain_context_mapping. I have CCed x86 folks to get more feedback here.

Cheers,

-- 
Julien Grall

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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Jan Beulich]

>>> On 12.06.17 at 15:36, <julien.grall@arm.com> wrote:
> On 09/06/17 12:15, Robin Murphy wrote:
>> On 08/06/17 20:30, Sameer Goel wrote:
>> [...]
>>>  /**
>>> - * iort_iommu_configure - Set-up IOMMU configuration for a device.
>>> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
>>> + * function sets up the fwspec as needed for a given device. Only PCI
>>> + * devices are supported for now.
>>>   *
>>>   * @dev: device to configure
>>>   *
>>> - * Returns: iommu_ops pointer on configuration success
>>> - *          NULL on configuration failure
>>> + * Returns: Appropriate acpi_status
>>>   */
>>> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
>>> +acpi_status iort_iommu_configure(struct device *dev)
>>>  {
>>>  	struct acpi_iort_node *node, *parent;
>>> -	const struct iommu_ops *ops = NULL;
>>>  	u32 streamid = 0;
>>> +	acpi_status status = AE_OK;
>>>
>>>  	if (dev_is_pci(dev)) {
>>> -		struct pci_bus *bus = to_pci_dev(dev)->bus;
>>> +		struct pci_dev *pci_device = to_pci_dev(dev);
>>>  		u32 rid;
>>>
>>> -		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
>>> -				       &rid);
>>> +		rid = PCI_BDF2(pci_device->bus,pci_device->devfn);
>>
>> Beware that the Linux code isn't actually correct to begin with[1]. I
>> don't know how much Xen deals with PCI bridges and quirks, but as it
>> stands you should be able to trivially expose the flaw here by plugging
>> in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs
>> (or even both) kick up stream table faults.
>>
>> Robin.
>>
>> [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html 
> 
> I am not sure how x86 is handling that. The closest I can find would be 
> domain_context_mapping. I have CCed x86 folks to get more feedback here.

I'm lacking enough context to know whether this is the issue
with what we call phantom devices, or something else.

Jan


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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Robin Murphy]

On 12/06/17 14:44, Jan Beulich wrote:
>>>> On 12.06.17 at 15:36, <julien.grall@arm.com> wrote:
>> On 09/06/17 12:15, Robin Murphy wrote:
>>> On 08/06/17 20:30, Sameer Goel wrote:
>>> [...]
>>>>  /**
>>>> - * iort_iommu_configure - Set-up IOMMU configuration for a device.
>>>> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
>>>> + * function sets up the fwspec as needed for a given device. Only PCI
>>>> + * devices are supported for now.
>>>>   *
>>>>   * @dev: device to configure
>>>>   *
>>>> - * Returns: iommu_ops pointer on configuration success
>>>> - *          NULL on configuration failure
>>>> + * Returns: Appropriate acpi_status
>>>>   */
>>>> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
>>>> +acpi_status iort_iommu_configure(struct device *dev)
>>>>  {
>>>>  	struct acpi_iort_node *node, *parent;
>>>> -	const struct iommu_ops *ops = NULL;
>>>>  	u32 streamid = 0;
>>>> +	acpi_status status = AE_OK;
>>>>
>>>>  	if (dev_is_pci(dev)) {
>>>> -		struct pci_bus *bus = to_pci_dev(dev)->bus;
>>>> +		struct pci_dev *pci_device = to_pci_dev(dev);
>>>>  		u32 rid;
>>>>
>>>> -		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
>>>> -				       &rid);
>>>> +		rid = PCI_BDF2(pci_device->bus,pci_device->devfn);
>>>
>>> Beware that the Linux code isn't actually correct to begin with[1]. I
>>> don't know how much Xen deals with PCI bridges and quirks, but as it
>>> stands you should be able to trivially expose the flaw here by plugging
>>> in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs
>>> (or even both) kick up stream table faults.
>>>
>>> Robin.
>>>
>>> [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html 
>>
>> I am not sure how x86 is handling that. The closest I can find would be 
>> domain_context_mapping. I have CCed x86 folks to get more feedback here.
> 
> I'm lacking enough context to know whether this is the issue
> with what we call phantom devices, or something else.

Phantom functions, PCIe-PCI bridges, basically anything that can result
in traffic from a single endpoint appearing on multiple RIDs, or a RID
other than the device's BDF, at the root complex (and IOMMUs/MSI
doorbells beyond).

Robin.

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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Goel, Sameer]

On 6/9/2017 5:15 AM, Robin Murphy wrote:
> On 08/06/17 20:30, Sameer Goel wrote:
> [...]
>>  /**
>> - * iort_iommu_configure - Set-up IOMMU configuration for a device.
>> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
>> + * function sets up the fwspec as needed for a given device. Only PCI
>> + * devices are supported for now.
>>   *
>>   * @dev: device to configure
>>   *
>> - * Returns: iommu_ops pointer on configuration success
>> - *          NULL on configuration failure
>> + * Returns: Appropriate acpi_status
>>   */
>> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
>> +acpi_status iort_iommu_configure(struct device *dev)
>>  {
>>  	struct acpi_iort_node *node, *parent;
>> -	const struct iommu_ops *ops = NULL;
>>  	u32 streamid = 0;
>> +	acpi_status status = AE_OK;
>>  
>>  	if (dev_is_pci(dev)) {
>> -		struct pci_bus *bus = to_pci_dev(dev)->bus;
>> +		struct pci_dev *pci_device = to_pci_dev(dev);
>>  		u32 rid;
>>  
>> -		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
>> -				       &rid);
>> +		rid = PCI_BDF2(pci_device->bus,pci_device->devfn);
> 
> Beware that the Linux code isn't actually correct to begin with[1]. I
> don't know how much Xen deals with PCI bridges and quirks, but as it
> stands you should be able to trivially expose the flaw here by plugging
> in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs
> (or even both) kick up stream table faults.
> 
Appreciate the feedback Robin. I will try to incorporate the new IORT changes 
when I release the first patch set.  
> Robin.
> 
> [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html
> 
>>  
>>  		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;
>> +			return AE_NOT_FOUND;
>>  
>>  		parent = iort_node_map_rid(node, rid, &streamid,
>>  					   IORT_IOMMU_TYPE);
>>  
>> -		ops = iort_iommu_xlate(dev, parent, streamid);
>> +		status = iort_iommu_xlate(dev, parent, streamid);
>> +
>> +		status = status ? AE_ERROR : AE_OK;
>>  
>>  	} else {
>> +		status = AE_NOT_IMPLEMENTED;
>> +#if 0
>>  		int i = 0;
>>  
>>  		node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
>> @@ -616,11 +655,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;
>> +	return status;
>>  }
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Julien Grall]

Hi Sameer,

On 08/06/17 20:30, Sameer Goel wrote:
> Add limited support for parsing IORT table to initialize SMMU devices.

It would be nice to explain what you actually support in the commit message.

[...]

>
>  #define IORT_TYPE_MASK(type)	(1 << (type))
>  #define IORT_MSI_TYPE		(1 << ACPI_IORT_NODE_ITS_GROUP)
>  #define IORT_IOMMU_TYPE		((1 << ACPI_IORT_NODE_SMMU) |	\
>  				(1 << ACPI_IORT_NODE_SMMU_V3))
>
> +#if 0
>  struct iort_its_msi_chip {
>  	struct list_head	list;
>  	struct fwnode_handle	*fw_node;
>  	u32			translation_id;
>  };
>
> +#endif
> +

Why cannot you enable MSI now? Actually this would allow us to know 
whether we should import the code from Linux or reimplement or own.

>  struct iort_fwnode {
>  	struct list_head list;
>  	struct acpi_iort_node *iort_node;
> @@ -60,7 +71,7 @@ static inline int iort_set_fwnode(struct acpi_iort_node *iort_node,
>  {
>  	struct iort_fwnode *np;
>
> -	np = kzalloc(sizeof(struct iort_fwnode), GFP_ATOMIC);
> +	np = xzalloc(struct iort_fwnode);

If we decide to keep this code close to Linux you need to:
	- avoid replacing Linux name by Xen name as much as possible. Instead 
use macro
	- explain above each change why you need then

See what I did for the ARM SMMU.

>
>  	if (WARN_ON(!np))
>  		return -ENOMEM;
> @@ -114,7 +125,7 @@ static inline void iort_delete_fwnode(struct acpi_iort_node *node)
>  	list_for_each_entry_safe(curr, tmp, &iort_fwnode_list, list) {
>  		if (curr->iort_node == node) {
>  			list_del(&curr->list);
> -			kfree(curr);
> +			xfree(curr);
>  			break;
>  		}
>  	}
> @@ -127,6 +138,7 @@ typedef acpi_status (*iort_find_node_callback)
>  /* Root pointer to the mapped IORT table */
>  static struct acpi_table_header *iort_table;
>
> +#if 0
>  static LIST_HEAD(iort_msi_chip_list);
>  static DEFINE_SPINLOCK(iort_msi_chip_lock);
>
> @@ -199,7 +211,7 @@ struct fwnode_handle *iort_find_domain_token(int trans_id)
>
>  	return fw_node;
>  }
> -
> +#endif

Please avoid dropping newline.

>  static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
>  					     iort_find_node_callback callback,
>  					     void *context)
> @@ -219,9 +231,10 @@ static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
>  				iort_table->length);
>
>  	for (i = 0; i < iort->node_count; i++) {
> -		if (WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND,
> -			       "IORT node pointer overflows, bad table!\n"))
> +		if (iort_node >= iort_end) {
> +			printk(XENLOG_ERR "IORT node pointer overflows, bad table!\n");
>  			return NULL;
> +		}
>
>  		if (iort_node->type == type &&
>  		    ACPI_SUCCESS(callback(iort_node, context)))
> @@ -249,6 +262,14 @@ bool iort_node_match(u8 type)
>  	return node != NULL;
>  }
>
> +/*
> + * Following 2 definies should come from the PCI passthrough implementation.
> + * Based on the current pci_dev define the bus number and seg number come
> + * from pci_dev so making an API assumption
> + */
> +#define to_pci_dev(p) container_of(p, struct pci_dev,dev)
> +#define pci_domain_nr(dev) dev->seg

This should go in pci.h.

> +
>  static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
>  					    void *context)
>  {
> @@ -256,6 +277,11 @@ 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;
> +/*
> + * Named components not supported yet.

Can you expand?

[...]

> +/*
> + * RID is the same as PCI_DEVID(BDF) for QDF2400

Xen is meant to be agnostic to the platform. Rather than making 
assumption, we should discuss on way to get the RID. I will answer on 
Robin's mail about it.

> + */
>  static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
>  {
>  	u32 *rid = data;
> @@ -510,7 +550,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)
> @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>  	return ret;
>  }
>
> -static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
> -					struct acpi_iort_node *node,
> -					u32 streamid)
> +static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
> +			    u32 streamid)
>  {
> -	const struct iommu_ops *ops = NULL;
>  	int ret = -ENODEV;
>  	struct fwnode_handle *iort_fwnode;
>
>  	if (node) {
>  		iort_fwnode = iort_get_fwnode(node);
>  		if (!iort_fwnode)
> -			return NULL;
> -
> -		ops = iommu_ops_from_fwnode(iort_fwnode);
> -		if (!ops)
> -			return NULL;
> +			return ret;
>
> -		ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
> +		ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, NULL);

Why don't you get the IOMMU ops here? This would avoid unecessary change.

>  	}
>
> -	return ret ? NULL : ops;
> +	return ret;
>  }
>
> +#if 0 /* Xen: We do not need this function for Xen */
>  /**
>   * iort_set_dma_mask - Set-up dma mask for a device.
>   *
> @@ -567,39 +602,43 @@ void iort_set_dma_mask(struct device *dev)
>  	if (!dev->dma_mask)
>  		dev->dma_mask = &dev->coherent_dma_mask;
>  }
> -
> +#endif

Please avoid dropping newline

>  /**
> - * iort_iommu_configure - Set-up IOMMU configuration for a device.
> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
> + * function sets up the fwspec as needed for a given device. Only PCI
> + * devices are supported for now.
>   *
>   * @dev: device to configure
>   *
> - * Returns: iommu_ops pointer on configuration success
> - *          NULL on configuration failure
> + * Returns: Appropriate acpi_status
>   */
> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
> +acpi_status iort_iommu_configure(struct device *dev)

I don't think this change is necessary. Returning the iommu_ops here 
would be the best.

[...]

> +/*
> + * Xen: Not using the parsin 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,93 +852,63 @@ 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
> + * Xen: rename the function to iort_add_smmu_device

Renaming the function make more difficult to backport change. So why 
renaming it?

[...]

> -static void __init iort_init_platform_devices(void)
> +/*
> + * Xen: Rename the function to iort_init_devices as this function will
> + * populate the device object for SMMU devices.

Ditto.

> + */
> +static void __init iort_init_devices(void)

[...]

> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
> index edf70c2..1397da5 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -74,24 +74,26 @@ int arch_iommu_populate_page_table(struct domain *d)
>      return -ENOSYS;
>  }
>
> +/*
> + * The ops parameter in this function will always be NULL for Xen,
> + * as the ops are set per domain.
> + */
>  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>          const struct iommu_ops *ops)
>  {
>      struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>
> +    /*
> +     * fwspec is already allocated for this device.
> +     */
>      if (fwspec)
> -        return ops == fwspec->ops ? 0 : -EINVAL;
> +        return 0;
>
>      fwspec = xzalloc(struct iommu_fwspec);
>      if (!fwspec)
>          return -ENOMEM;
>
> -    /* Ref counting for the dt device node is not needed */
> -
> -    /*of_node_get(to_of_node(iommu_fwnode));*/

This could should have neever been commented at first hand.

> -
>      fwspec->iommu_fwnode = iommu_fwnode;
> -    fwspec->ops = ops;
>      dev->iommu_fwspec = fwspec;
>      return 0;
>  }
> @@ -101,7 +103,6 @@ void iommu_fwspec_free(struct device *dev)
>      struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>
>      if (fwspec) {
> -        /*fwnode_handle_put(fwspec->iommu_fwnode);*/

Ditto.

>          xfree(fwspec);
>          dev->iommu_fwspec = NULL;
>      }
> diff --git a/xen/include/acpi/acpi.h b/xen/include/acpi/acpi.h
> index c852701..1ac92b2 100644
> --- a/xen/include/acpi/acpi.h
> +++ b/xen/include/acpi/acpi.h
> @@ -60,6 +60,7 @@
>  #include "actbl.h"		/* ACPI table definitions */
>  #include "aclocal.h"		/* Internal data types */
>  #include "acoutput.h"		/* Error output and Debug macros */
> +#include "acpi_iort.h"          /* Utility defines for IORT */

I think this is a pretty bad idea. You don't need to include acpi_iort.h 
everywhere.

>  #include "acpiosxf.h"		/* Interfaces to the ACPI-to-OS layer */
>  #include "acpixf.h"		/* ACPI core subsystem external interfaces */
>  #include "acglobal.h"		/* All global variables */
> diff --git a/xen/include/acpi/acpi_iort.h b/xen/include/acpi/acpi_iort.h
> index 77e0809..c0b5b8d 100644
> --- a/xen/include/acpi/acpi_iort.h
> +++ b/xen/include/acpi/acpi_iort.h
> @@ -19,27 +19,35 @@
>  #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>
>
> +/*
> + * We are not using IORT IRQ bindings for this change set
> + */
> +#if 0

Whilst I can understand why we want to ifdef in the *.c. I think it less 
warrant in the header. I would prefer them to either kept or dropped. 
But not #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

Why #ifdef CONFIG_ARM_64?

>  void acpi_iort_init(void);
>  bool iort_node_match(u8 type);
>  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);
> -const struct iommu_ops *iort_iommu_configure(struct device *dev);
> +#endif
> +acpi_status 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 +55,11 @@ 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; }
> +acpi_status iommu_ops iort_iommu_configure(struct device *dev)
> +{ return AE_NOT_IMPLEMENTED; }
>  #endif
>
> +#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);
> -const struct iommu_ops *iort_iommu_configure(struct device *dev);
> +#endif
> +acpi_status 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 +55,11 @@ 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; }
> +acpi_status iommu_ops iort_iommu_configure(struct device *dev)
> +{ return AE_NOT_IMPLEMENTED; }
>  #endif
>
> -#define IORT_ACPI_DECLARE(name, table_id, fn)		\
> -	ACPI_DECLARE_PROBE_ENTRY(iort, name, table_id, 0, NULL, 0, fn)

I think we whould need something similar for Xen.

[...]

> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 995a85a..3785fae 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -9,7 +9,12 @@
>  #include <asm/bug.h>
>
>  #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
> +#define WARN_ON(p) ({                                      \
> +    int __ret_warn_on = !!(p);                             \
> +    if (unlikely(__ret_warn_on))                           \
> +        WARN();                                            \
> +    unlikely(__ret_warn_on);                               \
> +})

hmmmm. Why this change?

>
>  #if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
>  /* Force a compilation error if condition is true */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 59b6e8a..c518569 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -88,6 +88,7 @@ struct pci_dev {
>  #define PT_FAULT_THRESHOLD 10
>      } fault;
>      u64 vf_rlen[6];
> +    struct device dev;

struct device does not exist on x86. If you still want to add it, then 
you should do it in a separate patch and CC relevant maintainers.

>  };
>
>  #define for_each_pdev(domain, pdev) \
>

Cheers,

-- 
Julien Grall

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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Goel, Sameer]

On 6/12/2017 7:24 AM, Julien Grall wrote:
> Hi Sameer,
> 
> On 08/06/17 20:30, Sameer Goel wrote:
>> Add limited support for parsing IORT table to initialize SMMU devices.
> 
> It would be nice to explain what you actually support in the commit message.
> 
> [...]
> 
>>
>>  #define IORT_TYPE_MASK(type)    (1 << (type))
>>  #define IORT_MSI_TYPE        (1 << ACPI_IORT_NODE_ITS_GROUP)
>>  #define IORT_IOMMU_TYPE        ((1 << ACPI_IORT_NODE_SMMU) |    \
>>                  (1 << ACPI_IORT_NODE_SMMU_V3))
>>
>> +#if 0
>>  struct iort_its_msi_chip {
>>      struct list_head    list;
>>      struct fwnode_handle    *fw_node;
>>      u32            translation_id;
>>  };
>>
>> +#endif
>> +
> 
> Why cannot you enable MSI now? Actually this would allow us to know whether we should import the code from Linux or reimplement or own.
Well was not too sure about how this will fit in, so was leaving this for next iteration. I will try to enable this.
> 
>>  struct iort_fwnode {
>>      struct list_head list;
>>      struct acpi_iort_node *iort_node;
>> @@ -60,7 +71,7 @@ static inline int iort_set_fwnode(struct acpi_iort_node *iort_node,
>>  {
>>      struct iort_fwnode *np;
>>
>> -    np = kzalloc(sizeof(struct iort_fwnode), GFP_ATOMIC);
>> +    np = xzalloc(struct iort_fwnode);
> 
> If we decide to keep this code close to Linux you need to:
>     - avoid replacing Linux name by Xen name as much as possible. Instead use macro
>     - explain above each change why you need then
> 
> See what I did for the ARM SMMU.
Agreed
> 
>>
>>      if (WARN_ON(!np))
>>          return -ENOMEM;
>> @@ -114,7 +125,7 @@ static inline void iort_delete_fwnode(struct acpi_iort_node *node)
>>      list_for_each_entry_safe(curr, tmp, &iort_fwnode_list, list) {
>>          if (curr->iort_node == node) {
>>              list_del(&curr->list);
>> -            kfree(curr);
>> +            xfree(curr);
>>              break;
>>          }
>>      }
>> @@ -127,6 +138,7 @@ typedef acpi_status (*iort_find_node_callback)
>>  /* Root pointer to the mapped IORT table */
>>  static struct acpi_table_header *iort_table;
>>
>> +#if 0
>>  static LIST_HEAD(iort_msi_chip_list);
>>  static DEFINE_SPINLOCK(iort_msi_chip_lock);
>>
>> @@ -199,7 +211,7 @@ struct fwnode_handle *iort_find_domain_token(int trans_id)
>>
>>      return fw_node;
>>  }
>> -
>> +#endif
> 
> Please avoid dropping newline.
> 
>>  static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
>>                           iort_find_node_callback callback,
>>                           void *context)
>> @@ -219,9 +231,10 @@ static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
>>                  iort_table->length);
>>
>>      for (i = 0; i < iort->node_count; i++) {
>> -        if (WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND,
>> -                   "IORT node pointer overflows, bad table!\n"))
>> +        if (iort_node >= iort_end) {
>> +            printk(XENLOG_ERR "IORT node pointer overflows, bad table!\n");
>>              return NULL;
>> +        }
>>
>>          if (iort_node->type == type &&
>>              ACPI_SUCCESS(callback(iort_node, context)))
>> @@ -249,6 +262,14 @@ bool iort_node_match(u8 type)
>>      return node != NULL;
>>  }
>>
>> +/*
>> + * Following 2 definies should come from the PCI passthrough implementation.
>> + * Based on the current pci_dev define the bus number and seg number come
>> + * from pci_dev so making an API assumption
>> + */
>> +#define to_pci_dev(p) container_of(p, struct pci_dev,dev)
>> +#define pci_domain_nr(dev) dev->seg
> 
> This should go in pci.h.
> 
>> +
>>  static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
>>                          void *context)
>>  {
>> @@ -256,6 +277,11 @@ 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;
>> +/*
>> + * Named components not supported yet.
> 
> Can you expand?
Were not needed for now, but I agree we can enable the code.
> 
> [...]
> 
>> +/*
>> + * RID is the same as PCI_DEVID(BDF) for QDF2400
> 
> Xen is meant to be agnostic to the platform. Rather than making assumption, we should discuss on way to get the RID. I will answer on Robin's mail about it.
> 
>> + */
>>  static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
>>  {
>>      u32 *rid = data;
>> @@ -510,7 +550,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)
>> @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>>      return ret;
>>  }
>>
>> -static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
>> -                    struct acpi_iort_node *node,
>> -                    u32 streamid)
>> +static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
>> +                u32 streamid)
>>  {
>> -    const struct iommu_ops *ops = NULL;
>>      int ret = -ENODEV;
>>      struct fwnode_handle *iort_fwnode;
>>
>>      if (node) {
>>          iort_fwnode = iort_get_fwnode(node);
>>          if (!iort_fwnode)
>> -            return NULL;
>> -
>> -        ops = iommu_ops_from_fwnode(iort_fwnode);
>> -        if (!ops)
>> -            return NULL;
>> +            return ret;
>>
>> -        ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
>> +        ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, NULL);
> 
> Why don't you get the IOMMU ops here? This would avoid unecessary change.
From the linux definition it seems that there will be eventually used to override the ops
set by the bus. I did not find a use for this here, so removed it to simplify code. I can 
add these back, but see this as dead code.

> 
>>      }
>>
>> -    return ret ? NULL : ops;
>> +    return ret;
>>  }
>>
>> +#if 0 /* Xen: We do not need this function for Xen */
>>  /**
>>   * iort_set_dma_mask - Set-up dma mask for a device.
>>   *
>> @@ -567,39 +602,43 @@ void iort_set_dma_mask(struct device *dev)
>>      if (!dev->dma_mask)
>>          dev->dma_mask = &dev->coherent_dma_mask;
>>  }
>> -
>> +#endif
> 
> Please avoid dropping newline
> 
>>  /**
>> - * iort_iommu_configure - Set-up IOMMU configuration for a device.
>> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
>> + * function sets up the fwspec as needed for a given device. Only PCI
>> + * devices are supported for now.
>>   *
>>   * @dev: device to configure
>>   *
>> - * Returns: iommu_ops pointer on configuration success
>> - *          NULL on configuration failure
>> + * Returns: Appropriate acpi_status
>>   */
>> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
>> +acpi_status iort_iommu_configure(struct device *dev)
> 
> I don't think this change is necessary. Returning the iommu_ops here would be the best.
> 
> [...]
> 
>> +/*
>> + * Xen: Not using the parsin 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,93 +852,63 @@ 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
>> + * Xen: rename the function to iort_add_smmu_device
> 
> Renaming the function make more difficult to backport change. So why renaming it?
> 
> [...]
> 
>> -static void __init iort_init_platform_devices(void)
>> +/*
>> + * Xen: Rename the function to iort_init_devices as this function will
>> + * populate the device object for SMMU devices.
> 
> Ditto.
> 
>> + */
>> +static void __init iort_init_devices(void)
> 
> [...]
> 
>> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
>> index edf70c2..1397da5 100644
>> --- a/xen/drivers/passthrough/arm/iommu.c
>> +++ b/xen/drivers/passthrough/arm/iommu.c
>> @@ -74,24 +74,26 @@ int arch_iommu_populate_page_table(struct domain *d)
>>      return -ENOSYS;
>>  }
>>
>> +/*
>> + * The ops parameter in this function will always be NULL for Xen,
>> + * as the ops are set per domain.
>> + */
>>  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>>          const struct iommu_ops *ops)
>>  {
>>      struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>
>> +    /*
>> +     * fwspec is already allocated for this device.
>> +     */
>>      if (fwspec)
>> -        return ops == fwspec->ops ? 0 : -EINVAL;
>> +        return 0;
>>
>>      fwspec = xzalloc(struct iommu_fwspec);
>>      if (!fwspec)
>>          return -ENOMEM;
>>
>> -    /* Ref counting for the dt device node is not needed */
>> -
>> -    /*of_node_get(to_of_node(iommu_fwnode));*/
> 
> This could should have neever been commented at first hand.
Needed some help on enabling the dt related code. I will try to enable this.
> 
>> -
>>      fwspec->iommu_fwnode = iommu_fwnode;
>> -    fwspec->ops = ops;
>>      dev->iommu_fwspec = fwspec;
>>      return 0;
>>  }
>> @@ -101,7 +103,6 @@ void iommu_fwspec_free(struct device *dev)
>>      struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>
>>      if (fwspec) {
>> -        /*fwnode_handle_put(fwspec->iommu_fwnode);*/
> 
> Ditto.
> 
>>          xfree(fwspec);
>>          dev->iommu_fwspec = NULL;
>>      }
>> diff --git a/xen/include/acpi/acpi.h b/xen/include/acpi/acpi.h
>> index c852701..1ac92b2 100644
>> --- a/xen/include/acpi/acpi.h
>> +++ b/xen/include/acpi/acpi.h
>> @@ -60,6 +60,7 @@
>>  #include "actbl.h"        /* ACPI table definitions */
>>  #include "aclocal.h"        /* Internal data types */
>>  #include "acoutput.h"        /* Error output and Debug macros */
>> +#include "acpi_iort.h"          /* Utility defines for IORT */
> 
> I think this is a pretty bad idea. You don't need to include acpi_iort.h everywhere.
True this is needed for arm only. I will include this in arm specific code.
> 
>>  #include "acpiosxf.h"        /* Interfaces to the ACPI-to-OS layer */
>>  #include "acpixf.h"        /* ACPI core subsystem external interfaces */
>>  #include "acglobal.h"        /* All global variables */
>> diff --git a/xen/include/acpi/acpi_iort.h b/xen/include/acpi/acpi_iort.h
>> index 77e0809..c0b5b8d 100644
>> --- a/xen/include/acpi/acpi_iort.h
>> +++ b/xen/include/acpi/acpi_iort.h
>> @@ -19,27 +19,35 @@
>>  #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>
>>
>> +/*
>> + * We are not using IORT IRQ bindings for this change set
>> + */
>> +#if 0
> 
> Whilst I can understand why we want to ifdef in the *.c. I think it less warrant in the header. I would prefer them to either kept or dropped. But not #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
> 
> Why #ifdef CONFIG_ARM_64?
Was trying to keep the impact low for this RFC iteration (My use-case is arm64 only). Looking for the right recommendation?
> 
>>  void acpi_iort_init(void);
>>  bool iort_node_match(u8 type);
>>  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);
>> -const struct iommu_ops *iort_iommu_configure(struct device *dev);
>> +#endif
>> +acpi_status 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 +55,11 @@ 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; }
>> +acpi_status iommu_ops iort_iommu_configure(struct device *dev)
>> +{ return AE_NOT_IMPLEMENTED; }
>>  #endif
>>
>> +#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);
>> -const struct iommu_ops *iort_iommu_configure(struct device *dev);
>> +#endif
>> +acpi_status 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 +55,11 @@ 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; }
>> +acpi_status iommu_ops iort_iommu_configure(struct device *dev)
>> +{ return AE_NOT_IMPLEMENTED; }
>>  #endif
>>
>> -#define IORT_ACPI_DECLARE(name, table_id, fn)        \
>> -    ACPI_DECLARE_PROBE_ENTRY(iort, name, table_id, 0, NULL, 0, fn)
> 
> I think we whould need something similar for Xen.
Yes. I have defined something similar to what you have defined for DT in smmu 
driver.
> 
> [...]
> 
>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>> index 995a85a..3785fae 100644
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -9,7 +9,12 @@
>>  #include <asm/bug.h>
>>
>>  #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>> +#define WARN_ON(p) ({                                      \
>> +    int __ret_warn_on = !!(p);                             \
>> +    if (unlikely(__ret_warn_on))                           \
>> +        WARN();                                            \
>> +    unlikely(__ret_warn_on);                               \
>> +})
> 
> hmmmm. Why this change?
Was getting a compilation error when I was using WARN_ON as a conditional
in an if statement regarding the return value. So removed the loop. This
looks similar to Linux now.
> 
>>
>>  #if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
>>  /* Force a compilation error if condition is true */
>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
>> index 59b6e8a..c518569 100644
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -88,6 +88,7 @@ struct pci_dev {
>>  #define PT_FAULT_THRESHOLD 10
>>      } fault;
>>      u64 vf_rlen[6];
>> +    struct device dev;
> 
> struct device does not exist on x86. If you still want to add it, then you should do it in a separate patch and CC relevant maintainers.
Ok, I will go ahead and do that.
> 
>>  };
>>
>>  #define for_each_pdev(domain, pdev) \
>>
> 
> Cheers,
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Julien Grall]

Hi Sameer,

On 28/08/17 23:21, Goel, Sameer wrote:
> On 6/12/2017 7:24 AM, Julien Grall wrote:
>>>   static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>>>                      struct fwnode_handle *fwnode,
>>>                      const struct iommu_ops *ops)
>>> @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>>>       return ret;
>>>   }
>>>
>>> -static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
>>> -                    struct acpi_iort_node *node,
>>> -                    u32 streamid)
>>> +static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
>>> +                u32 streamid)
>>>   {
>>> -    const struct iommu_ops *ops = NULL;
>>>       int ret = -ENODEV;
>>>       struct fwnode_handle *iort_fwnode;
>>>
>>>       if (node) {
>>>           iort_fwnode = iort_get_fwnode(node);
>>>           if (!iort_fwnode)
>>> -            return NULL;
>>> -
>>> -        ops = iommu_ops_from_fwnode(iort_fwnode);
>>> -        if (!ops)
>>> -            return NULL;
>>> +            return ret;
>>>
>>> -        ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
>>> +        ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, NULL);
>>
>> Why don't you get the IOMMU ops here? This would avoid unecessary change.
>  From the linux definition it seems that there will be eventually used to override the ops
> set by the bus. I did not find a use for this here, so removed it to simplify code. I can
> add these back, but see this as dead code.

You will always have dead code if you import code as it is from Linux. 
This is the price to pay to help rebase the code in the future.

In that specific case, I think return the ops is the right thing to do. 
Potentially it will allow us to support different IOMMU at the same time.

[...]

>>>   #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
>>
>> Why #ifdef CONFIG_ARM_64?
> Was trying to keep the impact low for this RFC iteration (My use-case is arm64 only). Looking for the right recommendation?

IORT is not specific to ARM64. Even though ACPI is only supported for 
ARM64 on Xen today, we try to keep the code as generic as possible.

In that case, you could turn CONFIG_ACPI_IORT on in the Kconfig when 
ACPI is enabled.

[...]

>>
>> [...]
>>
>>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>>> index 995a85a..3785fae 100644
>>> --- a/xen/include/xen/lib.h
>>> +++ b/xen/include/xen/lib.h
>>> @@ -9,7 +9,12 @@
>>>   #include <asm/bug.h>
>>>
>>>   #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
>>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>>> +#define WARN_ON(p) ({                                      \
>>> +    int __ret_warn_on = !!(p);                             \
>>> +    if (unlikely(__ret_warn_on))                           \
>>> +        WARN();                                            \
>>> +    unlikely(__ret_warn_on);                               \
>>> +})
>>
>> hmmmm. Why this change?
> Was getting a compilation error when I was using WARN_ON as a conditional
> in an if statement regarding the return value. So removed the loop. This
> looks similar to Linux now.

This should belong to a separate patch with a commit message explaining 
why the change.

Cheers,

-- 
Julien Grall

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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Goel, Sameer]

On 9/12/2017 5:25 AM, Julien Grall wrote:
> Hi Sameer,
> 
> On 28/08/17 23:21, Goel, Sameer wrote:
>> On 6/12/2017 7:24 AM, Julien Grall wrote:
>>>>   static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>>>>                      struct fwnode_handle *fwnode,
>>>>                      const struct iommu_ops *ops)
>>>> @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>>>>       return ret;
>>>>   }
>>>>
>>>> -static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
>>>> -                    struct acpi_iort_node *node,
>>>> -                    u32 streamid)
>>>> +static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
>>>> +                u32 streamid)
>>>>   {
>>>> -    const struct iommu_ops *ops = NULL;
>>>>       int ret = -ENODEV;
>>>>       struct fwnode_handle *iort_fwnode;
>>>>
>>>>       if (node) {
>>>>           iort_fwnode = iort_get_fwnode(node);
>>>>           if (!iort_fwnode)
>>>> -            return NULL;
>>>> -
>>>> -        ops = iommu_ops_from_fwnode(iort_fwnode);
>>>> -        if (!ops)
>>>> -            return NULL;
>>>> +            return ret;
>>>>
>>>> -        ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
>>>> +        ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, NULL);
>>>
>>> Why don't you get the IOMMU ops here? This would avoid unecessary change.
>>  From the linux definition it seems that there will be eventually used to override the ops
>> set by the bus. I did not find a use for this here, so removed it to simplify code. I can
>> add these back, but see this as dead code.
> 
> You will always have dead code if you import code as it is from Linux. This is the price to pay to help rebase the code in the future.
> 
> In that specific case, I think return the ops is the right thing to do. Potentially it will allow us to support different IOMMU at the same time.
>
Agreed.
 
> [...]
> 
>>>>   #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
>>>
>>> Why #ifdef CONFIG_ARM_64?
>> Was trying to keep the impact low for this RFC iteration (My use-case is arm64 only). Looking for the right recommendation?
> 
> IORT is not specific to ARM64. Even though ACPI is only supported for ARM64 on Xen today, we try to keep the code as generic as possible.
> 
> In that case, you could turn CONFIG_ACPI_IORT on in the Kconfig when ACPI is enabled.
Agreed. I will add the new config in the next patch set.
> 
> [...]
> 
>>>
>>> [...]
>>>
>>>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>>>> index 995a85a..3785fae 100644
>>>> --- a/xen/include/xen/lib.h
>>>> +++ b/xen/include/xen/lib.h
>>>> @@ -9,7 +9,12 @@
>>>>   #include <asm/bug.h>
>>>>
>>>>   #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
>>>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>>>> +#define WARN_ON(p) ({                                      \
>>>> +    int __ret_warn_on = !!(p);                             \
>>>> +    if (unlikely(__ret_warn_on))                           \
>>>> +        WARN();                                            \
>>>> +    unlikely(__ret_warn_on);                               \
>>>> +})
>>>
>>> hmmmm. Why this change?
>> Was getting a compilation error when I was using WARN_ON as a conditional
>> in an if statement regarding the return value. So removed the loop. This
>> looks similar to Linux now.
> 
> This should belong to a separate patch with a commit message explaining why the change.

Agreed. Since this is more of a Linux compat function, I do not want to change the system-wide behavior. I will move this in the IORT code.
> 
> Cheers,
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Julien Grall]

Hi Sameer,

On 21/09/17 01:37, Goel, Sameer wrote:
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
>>>>> index 995a85a..3785fae 100644
>>>>> --- a/xen/include/xen/lib.h
>>>>> +++ b/xen/include/xen/lib.h
>>>>> @@ -9,7 +9,12 @@
>>>>>    #include <asm/bug.h>
>>>>>
>>>>>    #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
>>>>> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>>>>> +#define WARN_ON(p) ({                                      \
>>>>> +    int __ret_warn_on = !!(p);                             \
>>>>> +    if (unlikely(__ret_warn_on))                           \
>>>>> +        WARN();                                            \
>>>>> +    unlikely(__ret_warn_on);                               \
>>>>> +})
>>>>
>>>> hmmmm. Why this change?
>>> Was getting a compilation error when I was using WARN_ON as a conditional
>>> in an if statement regarding the return value. So removed the loop. This
>>> looks similar to Linux now.
>>
>> This should belong to a separate patch with a commit message explaining why the change.
> 
> Agreed. Since this is more of a Linux compat function, I do not want to change the system-wide behavior. I will move this in the IORT code.

I don't see any reason to keep this implementation only for IORT. It 
would be possible to have similar construction in Xen.

So I would first try to see if maintainers would be willing to accept a 
system-wide change before thinking to re-implement WARN_ON in IORT.

Cheers,

-- 
Julien Grall

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

Re: [RFC 6/6] acpi:arm64: Add support for parsing IORT table

[Jan Beulich]

>>> On 08.06.17 at 21:30, <sgoel@codeaurora.org> wrote:
> Add limited support for parsing IORT table to initialize SMMU devices.
> 
> 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         | 232 +++++++++++++++++++-----------------

With the amount of changes done to this file I question even more
the value of first pulling in the plain Linux commits.

> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -9,7 +9,12 @@
>  #include <asm/bug.h>
>  
>  #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
> +#define WARN_ON(p) ({                                      \
> +    int __ret_warn_on = !!(p);                             \
> +    if (unlikely(__ret_warn_on))                           \
> +        WARN();                                            \
> +    unlikely(__ret_warn_on);                               \
> +})

This has nothing to do with the intention of the patch. If you want
WARN_ON()s behavior to change, please submit a separate patch
doing just that.

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -88,6 +88,7 @@ struct pci_dev {
>  #define PT_FAULT_THRESHOLD 10
>      } fault;
>      u64 vf_rlen[6];
> +    struct device dev;

Why? Please rationalize your changes in the patch description (and
perhaps split them).

Jan


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