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

Re: [Xen-devel] [PATCH v5 12/14] xen/arm: p2m: Clean cache PT when the IOMMU doesn't support coherent walk

Hi Ian,

On 14/05/14 08:18, Jan Beulich wrote:
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -344,6 +344,17 @@ void iommu_crash_shutdown(void)
      iommu_enabled = iommu_intremap = 0;

+bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
+    const struct iommu_ops *ops = domain_hvm_iommu(d)->platform_ops;
+    uint32_t features = 0;

Please here and further down - don't use fixed width type unless
you really need to.

--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -67,6 +67,14 @@ int iommu_map_page(struct domain *d, unsigned long gfn, 
unsigned long mfn,
                     unsigned int flags);
  int iommu_unmap_page(struct domain *d, unsigned long gfn);

+enum iommu_feature

Why 1? Enumerations are defined to start at zero, and starting at
zero is what you really want here. Don't specify a value at all.

It's a mistake when I create the enum. I will drop the 1.

@@ -139,6 +147,7 @@ struct iommu_ops {
      void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int 
      void (*iotlb_flush_all)(struct domain *d);
      void (*dump_p2m_table)(struct domain *d);
+    uint32_t (*features)(struct domain *d);

I think I said this on an earlier round already - for future extensibility
this should return "const unsigned long *", and get accessed by the
wrapper function using test_bit(). Or even better without an accessor
function at all, just directly having a "const unsigned long *" field here.
Unless of course the backend implementation - which isn't part of this
patch - would have difficulty setting up a suitable bitfield during (early)

The SMMU drivers handle multiple SMMUs. Each SMMU can have different specifications (e.g coherent walk support or not).

As each domain doesn't use all SMMUs, we might be able to avoid flushing PT on some of them. That's why I've choose to use a callback with the domain in parameter.

I don't like the solution which return "unsigned long *" because we are assuming the driver will always a valid pointer (for instance with 2 unsigned long), even if he doesn't need it.


Julien Grall

Xen-devel mailing list



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