linux-kernel - Re: [PATCH 06/11] iommu: Add iommu_device_set

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]

Date:   Fri, 10 Feb 2017 16:59:43 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Joerg Roedel <jroedel@...e.de>
Cc:     Joerg Roedel <joro@...tes.org>, Will Deacon <will.deacon@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Alex Williamson <alex.williamson@...hat.com>,
        David Woodhouse <dwmw2@...radead.org>,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 06/11] iommu: Add iommu_device_set_fwnode() interface

On 10/02/17 16:11, Joerg Roedel wrote:
> On Fri, Feb 10, 2017 at 04:03:07PM +0000, Robin Murphy wrote:
>> Yeah, on reflection explicit initialisation is certainly easier to read
>> than a bunch of arguments handled implicitly by register(), but then
>> from that angle, even more clear would be to simply have the drivers
>> write the relevant struct members directly - I'd be quite happy with
>> that, and we then don't have to add another setter to iommu.h for every
>> new struct member (and risk it looking like Java code...)
> 
> Yeah, that was my first approach. But there is the Intel VT-d anomaly,
> where a part of the driver can be built-in (dmar.c) with
> CONFIG_IOMMU_API=N. In this case 'struct iommu_device' is empty, and
> trying to access the members directly doesn't compile anymore.
> 
> I have to look if this anomaly could be removed, then it is probably the
> best to set the struct members directly without wrapper functions.

Ah, I hadn't managed to spot that - I assume there probably is some
valid edge case for wanting x2APIC functionality without DMA remapping
which prevents us from just adding the dependency. Looking at the code,
though, that situation does seem to rely on the call never actually
executing at runtime - not only is it conditional on a static variable
which is only ever set by non-present code, it would fail the probe if
it were called - so I think it would be perfectly reasonable to just
address that particular problem as below (untested, but if it lets us
get rid of the dummy !IOMMU_API definitions of the registration
functions I'd say we've done the right thing).

Robin.

----->8-----
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 8ccbd7023194..161641caff79 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1077,6 +1077,7 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)

        raw_spin_lock_init(&iommu->register_lock);

+#ifdef CONFIG_IOMMU_API
        if (intel_iommu_enabled) {
                iommu->iommu_dev = iommu_device_create(NULL, iommu,
                                                       intel_iommu_groups,
@@ -1087,6 +1088,7 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
                        goto err_unmap;
                }
        }
+#endif

        drhd->iommu = iommu;