[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.