linux-kernel - Re: [PATCH 1/2] iommufd/selftest: Use a fwnode to distinguish devices

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: <20231128190758.GA1312390@ziepe.ca>
Date:   Tue, 28 Nov 2023 15:07:58 -0400
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Robin Murphy <robin.murphy@....com>
Cc:     joro@...tes.org, kevin.tian@...el.com, will@...nel.org,
        iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] iommufd/selftest: Use a fwnode to distinguish devices

On Tue, Nov 28, 2023 at 05:36:33PM +0000, Robin Murphy wrote:

> You see this is exactly the kind of complexity I *don't* want, since the
> only thing it would foreseeably benefit is the one special case of the
> IOMMUFD selftest, which can far more trivially just adopt the other of the
> two "standard" usage models we have. I've been trying to get *away* from
> having to have boilerplate checks in all the drivers, and this would require
> bringing back a load of the ones I've just removed :(

I don't think we need to bring back the fwspec checks you removed, the
loop just needs to keep the NULL check:

 +	list_for_each_entry(iommu, &iommu_device_list, list) {
 +		if (iommu->ops != ops && !iommu->fwnode) {
 +			ops = iommu->ops;
 +			ret = iommu_init_device(pinf, iommu->ops);
 +			if (ret != -ENODEV)
 +				return ret;
 +		}
 +	}

Iterate over all the global driver ops only. Drivers with a fwnode
will never be called without a fwspec.

Also, does omap have problems now too? omap seems to set fwnode but
does some slightly different open coded non-fwspec parsing that worked
at bus time? Is it still OK? Does fwspec even find ops in omap's FW
description (ie it looks like it make iommu-cells optional or
something)?

> As I said before, I really want to avoid the perf_event_init model of
> calling round every driver saying "hey, do you want this?" since it's also
> error-prone if any of those drivers doesn't get the boilerplate exactly
> right and inadvertently fails to reject something it should have. 

The core missed an API that every driver needs: give me the struct
iommu_driver* the FW has referenced.

Instead every driver open codes something like
arm_smmu_get_by_fwnode(), or much worse.

If we force the drivers to say

 iommu_driver = iommu_fw_give_me_my_driver(dev, ops)

Then we automatically have a place to do all the rejection checks we
need, and driver's can't inadvertently skip this because they really
can't work without the iommu_driver at all.

Anyhow, I completed the series I talked about yesterday. It turned out
really nice I think, especially the driver facing API is much
cleaner. I'm just going through the last bits before I share it.

Jason