netdev - Re: [PATCH] [net-next] net: hns3: add IOMMU

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: <Zyi3kb_Q-0Fs9Ycw@google.com>
Date: Mon, 4 Nov 2024 12:01:21 +0000
From: Pranjal Shrivastava <praan@...gle.com>
To: Robin Murphy <robin.murphy@....com>
Cc: Arnd Bergmann <arnd@...nel.org>, Jian Shen <shenjian15@...wei.com>,
	Salil Mehta <salil.mehta@...wei.com>, Arnd Bergmann <arnd@...db.de>,
	Will Deacon <will@...nel.org>, Joerg Roedel <jroedel@...e.de>,
	iommu@...ts.linux.dev, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jijie Shao <shaojijie@...wei.com>,
	Peiyang Wang <wangpeiyang1@...wei.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [net-next] net: hns3: add IOMMU_SUPPORT dependency

On Mon, Nov 04, 2024 at 10:29:28AM +0000, Robin Murphy wrote:
> On 2024-11-04 8:21 am, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@...db.de>
> > 
> > The hns3 driver started filling iommu_iotlb_gather structures itself,
> > which requires CONFIG_IOMMU_SUPPORT is enabled:
> > 
> > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c: In function 'hns3_dma_map_sync':
> > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:395:14: error: 'struct iommu_iotlb_gather' has no member named 'start'
> >    395 |  iotlb_gather.start = iova;
> >        |              ^
> > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:396:14: error: 'struct iommu_iotlb_gather' has no member named 'end'
> >    396 |  iotlb_gather.end = iova + granule - 1;
> >        |              ^
> > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:397:14: error: 'struct iommu_iotlb_gather' has no member named 'pgsize'
> >    397 |  iotlb_gather.pgsize = granule;
> >        |              ^
> > 
> > Add a Kconfig dependency to make it build in random configurations.
> > 
> > Cc: Will Deacon <will@...nel.org>
> > Cc: Joerg Roedel <jroedel@...e.de>
> > Cc: Robin Murphy <robin.murphy@....com>
> > Cc: iommu@...ts.linux.dev
> > Fixes: f2c14899caba ("net: hns3: add sync command to sync io-pgtable")
> > Signed-off-by: Arnd Bergmann <arnd@...db.de>
> > ---
> > I noticed that no other driver does this, so it would be good to
> > have a confirmation from the iommu maintainers that this is how
> > the interface and the dependency is intended to be used.
> 
> WTF is that patch doing!? No, random device drivers should absolutely not be
> poking into IOMMU driver internals, this is egregiously wrong and the
> correct action is to drop it entirely.

+1. Device drivers shouldn't poke into IOMMU internals.

> 
> Thanks,
> Robin.

I see that the requirement in [1] is to somehow flush / sync the iotlb
right after dma mapping an address as a work around for a HW bug, right?

I'd like to point out that the dma_map* API take care of this if the
underlying iommu implements the `iotlb_sync_map` op (check iommu_map).

Thus, in case you require it and your iommu driver does NOT support
`iotlb_sync_map`, the right way would be to add this functionality to
*that* iommu driver instead of letting the device driver poke into IOMMU

Another thing worth noting is, we do have some quirks to address broken
hardware in the iommu drivers. For example, a broken pre-fetch command
for hisilicon HI161x in arm-smmu-v3 driver[2]. Such options can be added
to iommu drivers after discussion with the relevant stakeholders.

Thanks,
Praan

[1]
 https://lore.kernel.org/netdev/20241025092938.2912958-2-shaojijie@huawei.com/

[2]
 https://elixir.bootlin.com/linux/v6.11.6/source/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c#L81

> 
> > ---
> >   drivers/net/ethernet/hisilicon/Kconfig | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/net/ethernet/hisilicon/Kconfig b/drivers/net/ethernet/hisilicon/Kconfig
> > index 65302c41bfb1..790efc8d2de6 100644
> > --- a/drivers/net/ethernet/hisilicon/Kconfig
> > +++ b/drivers/net/ethernet/hisilicon/Kconfig
> > @@ -91,6 +91,7 @@ config HNS_ENET
> >   config HNS3
> >   	tristate "Hisilicon Network Subsystem Support HNS3 (Framework)"
> >   	depends on PCI
> > +	depends on IOMMU_SUPPORT
> >   	select NET_DEVLINK
> >   	select PAGE_POOL
> >   	help
> 
>