[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 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@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®.