Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature

["Mikołaj Lenczewski" <miko.lenczewski@arm.com>]

Hi Both,

Thanks for review and for checking other implementations for this
discrepancy.

On Mon, Mar 03, 2025 at 08:49:02AM +0000, Shameerali Kolothum Thodi wrote:
> 
> 
> > -----Original Message-----
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Friday, February 28, 2025 7:32 PM
> > To: Mikołaj Lenczewski <miko.lenczewski@arm.com>; Shameerali Kolothum
> > Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: ryan.roberts@arm.com; suzuki.poulose@arm.com;
> > yang@os.amperecomputing.com; catalin.marinas@arm.com;
> > will@kernel.org; joro@8bytes.org; jean-philippe@linaro.org;
> > mark.rutland@arm.com; joey.gouly@arm.com; oliver.upton@linux.dev;
> > james.morse@arm.com; broonie@kernel.org; maz@kernel.org;
> > david@redhat.com; akpm@linux-foundation.org; nicolinc@nvidia.com;
> > mshavit@google.com; jsnitsel@redhat.com; smostafa@google.com; linux-
> > arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > iommu@lists.linux.dev
> > Subject: Re: [PATCH v2 4/4] iommu/arm: Add BBM Level 2 smmu feature
> > 
> > On Fri, Feb 28, 2025 at 06:24:04PM +0000, Mikołaj Lenczewski wrote:
> > > For supporting BBM Level 2 for userspace mappings, we want to ensure
> > > that the smmu also supports its own version of BBM Level 2. Luckily, the
> > > smmu spec (IHI 0070G 3.21.1.3) is stricter than the aarch64 spec (DDI
> > > 0487K.a D8.16.2), so already guarantees that no aborts are raised when
> > > BBM level 2 is claimed.
> > >
> > > Add the feature and testing for it under arm_smmu_sva_supported().
> > >
> > > Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com>
> > > ---
> > >  arch/arm64/kernel/cpufeature.c                  | 7 +++----
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +++
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 3 +++
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     | 4 ++++
> > >  4 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > This patch looks good, for what it does. However for bisection safety
> > it should be earlier, before the patches that change the page table
> > algorithms to be unsafe for the SMMU.

Right, I should have noticed this earlier. Will reorder the patch.

> > However, I've heard people talking about shipping chips that have CPUs
> > with BBML2 but SMMUs without.
> > 
> > On such a system it seems like your series would break previously
> > working SVA support because this patch will end up disabling it?

Perhaps my understanding is flawed here, but I was under the impression
that with SVA both the core and smmu MUST support BBML2 to use it safely
for core translations? Otherwise the smmu might experience page faults
when it touches pages from the core that use BBML2, if it does not
support BBML2 itself? Again, I could very well be wrong, will double
check with the reference manuals.

> > Though I see your MIDR_REV list is limited, so perhaps that worry
> > doesn't effect any real chips made with those families? I am trying to
> > check some NVIDIA products against this list..

Hopefully, as you say, the MIDR list restricts the breakage to a limited
(ideally, zero-size) set of implementations which advertise BBML2
without conflict aborts, but which do not support BBML2 on the smmu.

However, if my understanding of the BBML2 feature and how it interacts
with SVA is flawed, this will obviously be something for me to fix.

> We do have implementations that support CPUs with BBLM2 with TLB
> conflict aborts and SMMUv3 with BBML2.  So don't think those platforms
> be affected by this.  Will check with our hardware folks if there is
> anything that will be affected by this.
> 
> Also we  have plans to try to use SMMUv3 BBML2 during VM live migration
> to split block pages to 4K. I guess, in that case we can enable SMMU BBML2
> independent of CPU side.
> 
> Thanks,
> Shameer

On independently enabling BBML2 on the smmu but not the CPU, this should
be possible. My check was intended to catch the case where the CPU is on
the MIDR allowlist, but the smmu does not support the feature. Whilst
this might still be bad logic, if it is correct, then as long as the CPU
is not in the allowlist sva should not be affected. And bbml2 itself on
the smmu should be safe regardless, as it is stricted than the
corresponding cpu-side feature.

-- 
Kind regards,
Mikołaj Lenczewski