Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1345902

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

Show all headers | View raw

Hi Thomas,
On 02/26/2016 07:19 PM, Thomas Gleixner wrote:
> 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.
OK I will rewrite this.
> 
>> +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
> 
>> +}
>> +
>> +#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
> 
>> +{
>> +
>> +	struct msi_desc *desc;
>> +	struct device *dev;
>> +	struct iommu_domain *d;
>> +	int ret;
> 
> Please order variables by descending length
ok
> 
>> +	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.
> 
>> +}
>> +#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
> 

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[RFC v4 00/14] KVM PCIe/MSI passthrough on ARM/ARM64 Eric Auger <eric.auger@linaro.org> - 2016-02-26 18:40 +0100
  [RFC v4 09/14] msi: IOMMU map the doorbell address when needed Eric Auger <eric.auger@linaro.org> - 2016-02-26 18:40 +0100
    Re: [RFC v4 09/14] msi: IOMMU map the doorbell address when needed Thomas Gleixner <tglx@linutronix.de> - 2016-02-26 19:30 +0100
      Re: [RFC v4 09/14] msi: IOMMU map the doorbell address when needed Eric Auger <eric.auger@linaro.org> - 2016-02-29 16:30 +0100
  [RFC v4 12/14] vfio/type1: also check IRQ remapping capability at msi domain Eric Auger <eric.auger@linaro.org> - 2016-02-26 18:40 +0100
  [RFC v4 07/14] msi: Add a new MSI_FLAG_IRQ_REMAPPING flag Eric Auger <eric.auger@linaro.org> - 2016-02-26 18:40 +0100
    Re: [RFC v4 07/14] msi: Add a new MSI_FLAG_IRQ_REMAPPING flag Thomas Gleixner <tglx@linutronix.de> - 2016-02-26 19:10 +0100
      Re: [RFC v4 07/14] msi: Add a new MSI_FLAG_IRQ_REMAPPING flag Eric Auger <eric.auger@linaro.org> - 2016-02-29 16:30 +0100
  [RFC v4 02/14] iommu: introduce a reserved iova cookie Eric Auger <eric.auger@linaro.org> - 2016-02-26 18:40 +0100
  [RFC v4 01/14] iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute Eric Auger <eric.auger@linaro.org> - 2016-02-26 18:40 +0100
  [RFC v4 10/14] vfio: introduce VFIO_IOVA_RESERVED vfio_dma type Eric Auger <eric.auger@linaro.org> - 2016-02-26 18:40 +0100
  [RFC v4 08/14] msi: export msi_get_domain_info Eric Auger <eric.auger@linaro.org> - 2016-02-26 18:40 +0100
  [RFC v4 03/14] dma-reserved-iommu: alloc/free_reserved_iova_domain Eric Auger <eric.auger@linaro.org> - 2016-02-26 18:40 +0100
  [RFC v4 06/14] dma-reserved-iommu: iommu_unmap_reserved Eric Auger <eric.auger@linaro.org> - 2016-02-26 18:50 +0100
  [RFC v4 05/14] dma-reserved-iommu: iommu_get/put_single_reserved Eric Auger <eric.auger@linaro.org> - 2016-02-26 18:50 +0100

csiph-web