linux-kernel - Re: [PATCH v5 2/6] iommu/arm-smmu-v3: Keep track of attached ssids

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]

Date:   Fri, 4 Aug 2023 00:32:08 +0800
From:   Michael Shavit <mshavit@...gle.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     iommu@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, baolu.lu@...ux.intel.com,
        will@...nel.org, jean-philippe@...aro.org, robin.murphy@....com,
        nicolinc@...dia.com
Subject: Re: [PATCH v5 2/6] iommu/arm-smmu-v3: Keep track of attached ssids

On Thu, Aug 3, 2023 at 11:42 PM Jason Gunthorpe <jgg@...dia.com> wrote:
>
> On Thu, Aug 03, 2023 at 06:12:22PM +0800, Michael Shavit wrote:
> > The arm-smmu-v3 driver keeps track of all masters that a domain is
> > attached to so that it can re-write their STEs when the domain's ASID is
> > upated by SVA.
>
> Wah?
>
> A domain's ASID shouldn't change, why does it change for SVA? Doesn't
> SVA use CDTE's only? Why would it ever change a STE? I'm confused what
> you are trying to explain here.

Urh right, I mixed up CD entry and STE here. Before this patch, SVA
keeps tracks of all the masters attached to a CD domain, and updates
the CD entry 0 in their CD table. Now that a CD domain can be attached
on non-zero SSIDs, SVA can't simply update slot 0 in the table; it
must know which slot(s) this domain is attached to.

>
> What is a "primary domain"? Why can't we fix SVA first so it doesn't
> have this weird "piggybacks" or:
>
...
>
> This patch is not making sense to me, the goal in the commit message
> seems logical, but why is tracking CD entries introducing this concept
> of a primary domain and doing special stuff for SSID=0?

I'd argue this patch isn't introducing anything the driver isn't
already doing. It's just making the status-quo explicit since it has
to handle it. I do have a patch series in the works to properly fix
SVA, but it's growing quite large and I was trying to get this feature
out first. At a high level, the subsequent series:
1. Nests the list of attached masters in a list of SMMUs the domain is
installed in. Updates SMMU-level operations (such as invalidations) to
iterate over said list.
2. Checks the compatibility of a domain when attaching to a new SMMU
instead of outright rejecting, to allow attaching a domain to multiple
SMMUs.
3. Thus allowing SVA to alloc a single domain for the MM struct (which
the series maps from multiple SVA domains internally, pending support
at the iommu core layer)
4. And allowing SVA to use the same set_dev_pasid implementation used
here on that domain.

Now having said that, it might be possible to get rid of piggybacking
sooner if we create an MM per master instead of per "primary-domain",
but I'm not too sure about performance implications. AFAICT, the only
downside might be for invalidate_range commands that could no longer
be sent as a batched command to the SMMU (since each mmu notifier
would be called independently).