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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
