Re: [PATCH v5 03/17] iommu/arm-smmu-v3: Move arm_smmu_rmr_install_bypass_ste()

From: Mostafa Saleh <smostafa@google.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Joerg Roedel <jroedel@suse.de>, Moritz Fischer <mdf@kernel.org>,
	Moritz Fischer <moritzf@google.com>,
	Michael Shavit <mshavit@google.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	patches@lists.linux.dev,
	Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>,
	Zhangfei Gao <zhangfei.gao@linaro.org>
Subject: Re: [PATCH v5 03/17] iommu/arm-smmu-v3: Move arm_smmu_rmr_install_bypass_ste()
Date: Tue, 13 Feb 2024 16:46:42 +0000	[thread overview]
Message-ID: <Zcuc8g-G4dCxieUl@google.com> (raw)
In-Reply-To: <20240213161601.GB1088888@nvidia.com>

On Tue, Feb 13, 2024 at 12:16:01PM -0400, Jason Gunthorpe wrote:
> On Tue, Feb 13, 2024 at 03:37:43PM +0000, Mostafa Saleh wrote:
> > Hi Jason,
> > 
> > On Tue, Feb 06, 2024 at 11:12:40AM -0400, Jason Gunthorpe wrote:
> > > Logically arm_smmu_init_strtab() is the function that allocates and
> > > populates the stream table with the initial value of the STEs. After this
> > > function returns the stream table should be fully ready.
> > > 
> > > arm_smmu_rmr_install_bypass_ste() adjusts the initial stream table to force
> > > any SIDs that the FW says have IOMMU_RESV_DIRECT to use bypass. This
> > > ensures there is no disruption to the identity mapping during boot.
> > > 
> > > Put arm_smmu_rmr_install_bypass_ste() into arm_smmu_init_strtab(), it
> > > already executes immediately after arm_smmu_init_strtab().
> > > 
> > > No functional change intended.
> > 
> > I think arm_smmu_init_strtab is quite low level to abstract FW configuration in it.
> > For example in KVM[1] we'd re-use a big part of this driver and rely on similar
> > low-level functions. But no strong opinion.
> 
> I'm happy to drop this patch, if no strong opinion I will leave it
> 
> > [1] https://lore.kernel.org/kvmarm/20230201125328.2186498-1-jean-philippe@linaro.org/
> 
> I saw this but I didn't try to figure out too much what it is
> doing.. It looks like a new iommu driver that re-uses alot of the
> datastructures of SMMU but has a different HW facing API based on
> hypercalls?

Yes, this is an implementation for the SMMUv3 driver in EL2 in KVM that is needed
for DMA isolation for Protected KVM(pKVM) as the host is untrusted in this case and
can attack the hypervisor through DMA.

This is similar to the kernel driver but with a bunch of tricks such as page table
allocation, power management and more.

Also we have some plans to extend it to KVM guests, (Initially I was considering
KVM-VFIO device but now with the new iommufd stuff, it might fit in there)

> It seems interesting, but my knee jerk reaction would be that a new
> iommu driver proposal needs to implement the new iommu core APIs, not
> the old stuff. IMHO, when this progresses past a RFC it needs to come
> as a proper submission of just the iommu driver, in the normal way.
> 
> I also wonder about the wisdom of sharing so much code. Code sharing
> is not unconditionally good if it doesn't have a robust abstraction..
>

The idea is that as the hardware is common and most of the dt binding also,
we shouldn’t really reinvent the wheel, so this series moves the common code
for the page-table library to be shared with EL1/EL2 and for HW/FW probe and
init to be shared between the kernel drivers and adds a lot of the infrastructure
for IOMMUs in the hypervisor.

Thanks,
Mostafa

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel