Re: [RFC v4 09/14] msi: IOMMU map the doorbell address when needed

[Thomas Gleixner]

On Fri, 26 Feb 2016, Eric Auger wrote:
> +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.

> +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.

> +}
> +
> +#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.

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

Please order variables by descending length

> +	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.

> +}
> +#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.

Thanks,

	tglx