linux-kernel - Re: [PATCH v5 19/19] iommu/dma: Add support for mapping MSIs

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.20.1608240950500.5714@nanos>
Date:   Wed, 24 Aug 2016 10:16:26 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Robin Murphy <robin.murphy@....com>
cc:     joro@...tes.org, will.deacon@....com,
        iommu@...ts.linux-foundation.org, devicetree@...r.kernel.org,
        lorenzo.pieralisi@....com, jean-philippe.brucker@....com,
        punit.agrawal@....com, thunder.leizhen@...wei.com,
        eric.auger@...hat.com, Jason Cooper <jason@...edaemon.net>,
        Marc Zyngier <marc.zyngier@....com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 19/19] iommu/dma: Add support for mapping MSIs

On Tue, 23 Aug 2016, Robin Murphy wrote:
> +	cookie = domain->iova_cookie;
> +	iovad = &cookie->iovad;
> +
> +	spin_lock(&cookie->msi_lock);
> +	list_for_each_entry(msi_page, &cookie->msi_page_list, list)
> +		if (msi_page->phys_hi == msg->address_hi &&
> +		    msi_page->phys_lo - msg->address_lo < iovad->granule)
> +			goto unlock;
> +
> +	ret = __iommu_dma_map_msi_page(dev, msg, domain, &msi_page);
> +unlock:
> +	spin_unlock(&cookie->msi_lock);
> +
> +	if (!ret) {
> +		msg->address_hi = upper_32_bits(msi_page->iova);
> +		msg->address_lo &= iova_mask(iovad);
> +		msg->address_lo += lower_32_bits(msi_page->iova);
> +	} else {
> +		/*
> +		 * We're called from a void callback, so the best we can do is
> +		 * 'fail' by filling the message with obviously bogus values.
> +		 * Since we got this far due to an IOMMU being present, it's
> +		 * not like the existing address would have worked anyway...
> +		 */
> +		msg->address_hi = ~0U;
> +		msg->address_lo = ~0U;
> +		msg->data = ~0U;
> +	}

The above is really horrible to parse. I had to read it five times to
understand the logic.

static struct iommu_dma_msi_page *
find_or_map_msi_page(struct iommu_dma_cookie *cookie, struct msi_msg *msg)
{
	struct iova_domain *iovad = &cookie->iovad;
       	struct iommu_dma_msi_page *page;

	list_for_each_entry(*page, &cookie->msi_page_list, list) {
		if (page->phys_hi == msg->address_hi &&
		    page->phys_lo - msg->address_lo < iovad->granule)
		    	return page;
	}

	/*
	 * FIXME: __iommu_dma_map_msi_page() should return a page or NULL.
	 * The integer return value is pretty pointless. If seperate error
	 * codes are required that's what ERR_PTR() is for ....
	 */
	ret = __iommu_dma_map_msi_page(dev, msg, domain, &page);
	return ret ? ERR_PTR(ret) : page;
}

So now the code in iommu_dma_map_msi_msg() becomes:

	spin_lock(&cookie->msi_lock);
	msi_page = find_or_map_msi_page(cookie, msg);
	spin_unlock(&cookie->msi_lock);

	if (!IS_ERR_OR_NULL(msi_page)) {
		msg->address_hi = upper_32_bits(msi_page->iova);
		msg->address_lo &= iova_mask(iovad);
		msg->address_lo += lower_32_bits(msi_page->iova);
	} else {
		/*
		 * We're called from a void callback, so the best we can do is
		 * 'fail' by filling the message with obviously bogus values.
		 * Since we got this far due to an IOMMU being present, it's
		 * not like the existing address would have worked anyway...
		 */
		msg->address_hi = ~0U;
		msg->address_lo = ~0U;
		msg->data = ~0U;
	}

Hmm? 

Thanks,

	tglx