Hi Thomas,
On 02/26/2016 07:19 PM, Thomas Gleixner wrote:

On Fri, 26 Feb 2016, Eric Auger wrote:

quoted

+static int msi_map_doorbell(struct iommu_domain *d, struct msi_msg *msg)
+{
+#ifdef CONFIG_IOMMU_DMA_RESERVED
+	dma_addr_t iova;
+	phys_addr_t addr;
+	int ret;
+
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+	addr = ((phys_addr_t)(msg->address_hi) << 32) | msg->address_lo;
+#else
+	addr = msg->address_lo;
+#endif
+
+	ret = iommu_get_single_reserved(d, addr, IOMMU_WRITE, &iova);
+	if (!ret) {
+		msg->address_lo = lower_32_bits(iova);
+		msg->address_hi = upper_32_bits(iova);
+	}
+	return ret;
+#else
+	return -ENODEV;
+#endif

This is completely unreadable. Please make this in a way which is parseable.
A few small inline functions do the trick.

OK I will rewrite this.

quoted

+static void msi_unmap_doorbell(struct iommu_domain *d, struct msi_msg *msg)
+{
+#ifdef CONFIG_IOMMU_DMA_RESERVED
+	dma_addr_t iova;
+
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+	iova = ((dma_addr_t)(msg->address_hi) << 32) | msg->address_lo;
+#else
+	iova = msg->address_lo;
+#endif
+
+	iommu_put_single_reserved(d, iova);
+#endif

Ditto.

ok

quoted

+}
+
+#ifdef CONFIG_IOMMU_API
+static struct iommu_domain *
+	irq_data_to_msi_mapping_domain(struct irq_data *irq_data)

If you split lines, then the function name starts at the beginning of the line
and not at some randome place.

ok

quoted

+{
+
+	struct msi_desc *desc;
+	struct device *dev;
+	struct iommu_domain *d;
+	int ret;

Please order variables by descending length

ok

quoted

+	desc = irq_data_get_msi_desc(irq_data);
+	if (!desc)
+		return NULL;
+
+	dev = msi_desc_to_dev(desc);
+
+	d = iommu_get_domain_for_dev(dev);
+	if (!d)
+		return NULL;
+
+	ret = iommu_domain_get_attr(d, DOMAIN_ATTR_MSI_MAPPING, NULL);
+	if (!ret)
+		return d;
+	else
+		return NULL;

Does anyone except you understand the purpose of the function? Comments have
been invented for a reason.

ok I will comment on the role of those functions.

quoted

+}
+#else
+static inline iommu_domain *
+		irq_data_to_msi_mapping_domain(struct irq_data *irq_data)
+{
+	return NULL;
+}
+#endif /* CONFIG_IOMMU_API */
+
+static int msi_compose(struct irq_data *irq_data,
+		       struct msi_msg *msg, bool erase)
+{
+	struct msi_msg old_msg;
+	struct iommu_domain *d;
+	int ret = 0;
+
+	d = irq_data_to_msi_mapping_domain(irq_data);
+	if (unlikely(d))
+		get_cached_msi_msg(irq_data->irq, &old_msg);
+
+	if (erase)
+		memset(msg, 0, sizeof(*msg));
+	else
+		ret = irq_chip_compose_msi_msg(irq_data, msg);
+
+	if (!d)
+		goto out;
+
+	/* handle (re-)mapping of MSI doorbell */
+	if ((old_msg.address_lo != msg->address_lo) ||
+	    (old_msg.address_hi != msg->address_hi))
+		msi_unmap_doorbell(d, &old_msg);
+
+	if (!erase)
+		WARN_ON(msi_map_doorbell(d, msg));
+
+out:
+	return ret;
+}

No, this is not the way we do this. You replace existing functionality by some
new fangled thing. which behaves differently.

This wants to be seperate patches, which first create a wrapper for
irq_chip_compose_msi_msg() and then adds the new functionality to it including
a proper explanation.

I have no idea how the above is supposed to be the same as the existing code
for the non iommu case.

Sure I will decompose things and provide more explanation.

Thank you for your time.

Best Regards

Eric

Thanks,

	tglx