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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.11.1602261907050.3638@nanos>
Date:	Fri, 26 Feb 2016 19:19:45 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Eric Auger <eric.auger@...aro.org>
cc:	eric.auger@...com, robin.murphy@....com,
	alex.williamson@...hat.com, will.deacon@....com, joro@...tes.org,
	jason@...edaemon.net, marc.zyngier@....com,
	christoffer.dall@...aro.org, linux-arm-kernel@...ts.infradead.org,
	kvmarm@...ts.cs.columbia.edu, kvm@...r.kernel.org,
	suravee.suthikulpanit@....com, patches@...aro.org,
	linux-kernel@...r.kernel.org, Manish.Jaggi@...iumnetworks.com,
	Bharat.Bhushan@...escale.com, pranav.sawargaonkar@...il.com,
	p.fedin@...sung.com, iommu@...ts.linux-foundation.org
Subject: Re: [RFC v4 09/14] msi: IOMMU map the doorbell address when needed

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