Message-ID: <BN9PR11MB527612F299A6436DDFE4DAB28C739@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Sat, 6 May 2023 02:58:32 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     "Torvalds, Linus" <torvalds@...ux-foundation.org>,
        Jason Gunthorpe <jgg@...pe.ca>,
        "jean-philippe@...aro.org" <jean-philippe@...aro.org>
CC:     Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>
Subject: RE: [git pull] IOMMU Updates for Linux v6.4

> From: Linus Torvalds <torvalds@...ux-foundation.org>
> Sent: Saturday, May 6, 2023 4:11 AM
> 
> On Fri, May 5, 2023 at 12:57 PM Jason Gunthorpe <jgg@...pe.ca> wrote:
> >
> > At least with the current situation CONFIG_INTEL_ENQCMD might be an
> > appropriate name, split out from the IOMMU kconfig and put in arch
> > kconfig?
> 
> That would at least somewhat clarify the use. I find IOMMU_SVA to be a
> particularly opaque name.
> 
> Admittedly I probably find it opaque because I come at it as somebody
> much more familiar with the MM side, not from the IOMMU side (and the
> _other_ conditional fields there make sense from that standpoint), but
> even so I think it would be good to clarify.
> 
> Of course, making clear it's architecture-specific would also be an
> argument for having an actual architecture-specific part of 'struct
> mm_struct' (the same way we have 'struct thread_info' in task_struct),
> but with only one single field I suspect that's just not worth it.
> 
> And who knows - maybe other architectures will pick something like this up?

Looks ARM has ST64BV/ST64BV0 similar to ENQCMD according to [1].

Looking at ARM spec [2]:

  Single-copy Atomic 64-byte EL0 Store with Return stores eight 64-bit
  doublewords from consecutive registers, Xt to X(t+7), to a memory
  location, with the bottom 32 bits taken from ACCDATA_EL1, and writes
  the status result of the store to a register.

ACCDATA_EL1 sounds like the PASID_MSR used in ENQCMD, though it's
not explicitly stated so...

With that CONFIG_CPU_PASID might be more future-proof based on
the semantics of those ENQCMD-like instructions?

But...

> 
> But yes, it might be good to really pin down what the rules are. Right
> now IOMMU_SVA has *no* real help-text (there's a comment saying "#
> Shared Virtual Addressing"), and is also enabled by ARM_SMMU_V3_SVA,
> which apparently doesn't actually want pasid support at all.

... ARM_SMMU_V3_SVA does require pasid support. Just no ENQCMD.

SVA allows the device to access CPU virtual addresses with pasid to
identify a specific CPU page table when the device is shared by many
processes. e.g. each work queue in the device can be associated with
a unique pasid pointing to the CPU page table of the owning process.

w/o ENQCMD mm->pasid is not strictly necessary. IOMMU driver can
store the pasid information in its own structure when a CPU page
table is associated to a device.

ENQCMD further allows a single work queue shared by many processes,
by storing pasid in a MSR and sending it in the wire so the work
queue can be stateless. This needs to store the pasid in mm to handle
#GP, fork(), etc.

But after mm->pasid was introduced in 52ad9bc64c74 ("mm: Add a
pasid member to struct mm_struct "), it then became the common
storage for SVA beyond ENQCMD and common sva helpers are built
around mm->pasid, e.g. iommu_sva_bind_device().

From this angle using IOMMU_SVA to guard mm->pasid still makes
sense to me. But we do need to add the missing help-text for it.

> 
> I dunno. I don't think this is a huge deal, but it did cause some
> confusion during the merge.
> 
>                  Linus

[1] https://stackoverflow.com/questions/70561491/temporality-of-st64b-and-movdir64b
[2] https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/ST64BV0--Single-copy-Atomic-64-byte-EL0-Store-with-Return-

Powered by blists - more mailing lists