From: Jason Gunthorpe <jgg@nvidia.com>
To: Mostafa Saleh <smostafa@google.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>, Eric Auger <eric.auger@redhat.com>,
Jean-Philippe Brucker <jean-philippe@linaro.org>,
Moritz Fischer <mdf@kernel.org>,
Michael Shavit <mshavit@google.com>,
Nicolin Chen <nicolinc@nvidia.com>,
patches@lists.linux.dev,
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Subject: Re: [PATCH v5 05/27] iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry()
Date: Wed, 27 Mar 2024 13:42:51 -0300 [thread overview]
Message-ID: <20240327164251.GL946323@nvidia.com> (raw)
In-Reply-To: <ZgPqnzA_a4ZdYGXM@google.com>
On Wed, Mar 27, 2024 at 09:45:03AM +0000, Mostafa Saleh wrote:
> On Tue, Mar 26, 2024 at 07:27:58PM -0300, Jason Gunthorpe wrote:
> > On Tue, Mar 26, 2024 at 07:12:53PM +0000, Mostafa Saleh wrote:
> > > On Tue, Mar 26, 2024 at 03:30:55PM -0300, Jason Gunthorpe wrote:
> > > > On Sat, Mar 23, 2024 at 01:02:15PM +0000, Mostafa Saleh wrote:
> > > > > > +static void arm_smmu_get_cd_used(const __le64 *ent, __le64 *used_bits)
> > > > > > +{
> > > > > > + used_bits[0] = cpu_to_le64(CTXDESC_CD_0_V);
> > > > > > + if (!(ent[0] & cpu_to_le64(CTXDESC_CD_0_V)))
> > > > > > + return;
> > > > > > + memset(used_bits, 0xFF, sizeof(struct arm_smmu_cd));
> > > > >
> > > > > This is a slightly different approach than what the driver does for STEs,
> > > > > where it explicitly sets the used bits. Is there a reason for that?
> > > >
> > > > It is just more compact this way
> > >
> > > IMHO, it seems too much to have this mechanism for CDs for just one
> > > SVA case, but I'll need to go through the whole seires first to make
> > > sure I am not missing anything.
> >
> > It is pretty ugly if you try to do it that way. You still need to
> > create some ops because the entry_set should be re-used (I mean I
> > guess you could copy it as well). Then you have to open code the
> > logic. And then the EPD0 path is somewhat fragile. Something sort of
> > like this:
> >
> > void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
> > struct arm_smmu_cd *cdptr,
> > const struct arm_smmu_cd *target)
> > {
> > bool target_valid = target->data[0] & cpu_to_le64(CTXDESC_CD_0_V);
> > bool cur_valid = cdptr->data[0] & cpu_to_le64(CTXDESC_CD_0_V);
> > struct arm_smmu_cd_writer cd_writer = {
> > .writer = {
> > .ops = &arm_smmu_cd_writer_ops,
> > .master = master,
> > },
> > .ssid = ssid,
> > };
> >
> > if (ssid != IOMMU_NO_PASID && cur_valid != target_valid) {
> > if (cur_valid)
> > master->cd_table.used_ssids--;
> > else
> > master->cd_table.used_ssids++;
> > }
> >
> > /* Force a V=0/V=1 update*/
> > __le64 update = target[0] & ~cpu_to_le64(CTXDESC_CD_0_V);
> > entry_set(&cd_writer.writer, cdptr->data, &update, 0, 1);
> > entry_set(&cd_writer.writer, cdptr->data, target->data, 1, NUM_ENTRY_QWORDS - 1);
> > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, 1);
> > }
> >
> > void arm_smmu_write_cd_entry_epd0(struct arm_smmu_master *master, int ssid,
> > struct arm_smmu_cd *cdptr,
> > const struct arm_smmu_cd *target)
> > {
> > struct arm_smmu_cd_writer cd_writer = {
> > .writer = {
> > .ops = &arm_smmu_cd_writer_ops,
> > .master = master,
> > },
> > .ssid = ssid,
> > };
> >
> > /*
> > * Target must the EPD0 = 1 version of the existing CD entry, caller
> > * must enforce it. Assume used_ssids doesn't need updating
> > * for this reason.
> > */
> > /* Update EPD0 */
> > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, 1);
> > /* Update everthing else */
> > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, NUM_ENTRY_QWORDS - 1);
> > }
> >
> > IMOH, at this point it is saner to have just implemented the used
> > function and use the mechanism robustly. Less special cases, less
> > fragility, less duplication.
> >
>
> But that adds extra cost of adding ops, indirection, modifying STE
> code..., for a case that is not common, so I think special casing it
> is actually better for readability and maintainability.
The above is what the special case would have to look like. This
programming work is not trivial. The programmer code was always
intended to be re-used for the CD and STE together so there is only
one logic, not two or three copies.
Reducing duplicated logic in such a tricky area is worth it, IMHO.
Jason
next prev parent reply other threads:[~2024-03-27 16:42 UTC|newest]
Thread overview: 116+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-04 23:43 [PATCH v5 00/27] Update SMMUv3 to the modern iommu API (part 2/3) Jason Gunthorpe
2024-03-04 23:43 ` [PATCH v5 01/27] iommu/arm-smmu-v3: Do not allow a SVA domain to be set on the wrong PASID Jason Gunthorpe
2024-03-15 3:38 ` Nicolin Chen
2024-03-18 18:16 ` Jason Gunthorpe
2024-03-22 17:48 ` Mostafa Saleh
2024-03-26 18:30 ` Jason Gunthorpe
2024-03-26 19:06 ` Mostafa Saleh
2024-03-26 22:10 ` Jason Gunthorpe
2024-03-04 23:43 ` [PATCH v5 02/27] iommu/arm-smmu-v3: Do not ATC invalidate the entire domain Jason Gunthorpe
2024-03-13 9:18 ` Michael Shavit
2024-03-15 2:24 ` Nicolin Chen
2024-03-16 18:09 ` Moritz Fischer
2024-03-22 17:51 ` Mostafa Saleh
2024-03-04 23:43 ` [PATCH v5 03/27] iommu/arm-smmu-v3: Add a type for the CD entry Jason Gunthorpe
2024-03-13 9:44 ` Michael Shavit
2024-03-16 18:10 ` Moritz Fischer
2024-03-18 18:02 ` Jason Gunthorpe
2024-03-15 3:12 ` Nicolin Chen
2024-03-22 17:52 ` Mostafa Saleh
2024-03-04 23:43 ` [PATCH v5 04/27] iommu/arm-smmu-v3: Add an ops indirection to the STE code Jason Gunthorpe
2024-03-13 11:30 ` Michael Shavit
2024-03-15 4:22 ` Nicolin Chen
2024-03-15 5:20 ` Nicolin Chen
2024-03-18 18:06 ` Jason Gunthorpe
2024-03-22 18:14 ` Mostafa Saleh
2024-03-25 14:11 ` Jason Gunthorpe
2024-03-25 21:01 ` Mostafa Saleh
2024-03-04 23:43 ` [PATCH v5 05/27] iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry() Jason Gunthorpe
2024-03-15 7:52 ` Nicolin Chen
2024-03-20 12:46 ` Jason Gunthorpe
2024-03-16 18:14 ` Moritz Fischer
2024-03-23 13:02 ` Mostafa Saleh
2024-03-25 14:25 ` Jason Gunthorpe
2024-03-26 18:30 ` Jason Gunthorpe
2024-03-26 19:12 ` Mostafa Saleh
2024-03-26 22:27 ` Jason Gunthorpe
2024-03-27 9:45 ` Mostafa Saleh
2024-03-27 16:42 ` Jason Gunthorpe [this message]
2024-03-04 23:43 ` [PATCH v5 06/27] iommu/arm-smmu-v3: Consolidate clearing a CD table entry Jason Gunthorpe
2024-03-13 11:57 ` Michael Shavit
2024-03-15 6:17 ` Nicolin Chen
2024-03-16 18:15 ` Moritz Fischer
2024-03-22 18:36 ` Mostafa Saleh
2024-03-25 14:14 ` Jason Gunthorpe
2024-03-25 21:02 ` Mostafa Saleh
2024-03-04 23:43 ` [PATCH v5 07/27] iommu/arm-smmu-v3: Move the CD generation for S1 domains into a function Jason Gunthorpe
2024-03-13 12:13 ` Michael Shavit
2024-03-18 18:11 ` Jason Gunthorpe
2024-03-23 13:11 ` Mostafa Saleh
2024-03-25 14:30 ` Jason Gunthorpe
2024-03-04 23:43 ` [PATCH v5 08/27] iommu/arm-smmu-v3: Move allocation of the cdtable into arm_smmu_get_cd_ptr() Jason Gunthorpe
2024-03-13 12:15 ` Michael Shavit
2024-03-16 3:31 ` Nicolin Chen
2024-03-22 19:07 ` Mostafa Saleh
2024-03-25 14:21 ` Jason Gunthorpe
2024-03-25 21:03 ` Mostafa Saleh
2024-03-04 23:43 ` [PATCH v5 09/27] iommu/arm-smmu-v3: Allocate the CD table entry in advance Jason Gunthorpe
2024-03-13 12:17 ` Michael Shavit
2024-03-16 4:16 ` Nicolin Chen
2024-03-18 18:14 ` Jason Gunthorpe
2024-03-22 19:15 ` Mostafa Saleh
2024-03-04 23:43 ` [PATCH v5 10/27] iommu/arm-smmu-v3: Move the CD generation for SVA into a function Jason Gunthorpe
2024-03-16 5:19 ` Nicolin Chen
2024-03-20 13:09 ` Jason Gunthorpe
2024-03-04 23:43 ` [PATCH v5 11/27] iommu/arm-smmu-v3: Build the whole CD in arm_smmu_make_s1_cd() Jason Gunthorpe
2024-03-15 10:04 ` Michael Shavit
2024-03-20 12:50 ` Jason Gunthorpe
2024-03-23 13:20 ` Mostafa Saleh
2024-03-04 23:44 ` [PATCH v5 12/27] iommu/arm-smmu-v3: Start building a generic PASID layer Jason Gunthorpe
2024-03-19 16:11 ` Michael Shavit
2024-03-20 18:32 ` Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 13/27] iommu/arm-smmu-v3: Make smmu_domain->devices into an allocated list Jason Gunthorpe
2024-03-19 13:09 ` Michael Shavit
2024-03-04 23:44 ` [PATCH v5 14/27] iommu/arm-smmu-v3: Make changing domains be hitless for ATS Jason Gunthorpe
2024-03-21 12:26 ` Michael Shavit
2024-03-21 13:28 ` Jason Gunthorpe
2024-03-21 14:53 ` Michael Shavit
2024-03-21 14:57 ` Michael Shavit
2024-03-21 17:32 ` Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 15/27] iommu/arm-smmu-v3: Add ssid to struct arm_smmu_master_domain Jason Gunthorpe
2024-03-19 13:31 ` Michael Shavit
2024-03-20 12:53 ` Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 16/27] iommu/arm-smmu-v3: Keep track of valid CD entries in the cd_table Jason Gunthorpe
2024-03-19 13:55 ` Michael Shavit
2024-03-20 18:21 ` Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 17/27] iommu/arm-smmu-v3: Thread SSID through the arm_smmu_attach_*() interface Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 18/27] iommu/arm-smmu-v3: Make SVA allocate a normal arm_smmu_domain Jason Gunthorpe
2024-03-19 14:52 ` Michael Shavit
2024-03-20 23:20 ` Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 19/27] iommu/arm-smmu-v3: Keep track of arm_smmu_master_domain for SVA Jason Gunthorpe
2024-03-21 10:47 ` Michael Shavit
2024-03-21 13:55 ` Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 20/27] iommu: Add ops->domain_alloc_sva() Jason Gunthorpe
2024-03-19 15:09 ` Michael Shavit
2024-03-04 23:44 ` [PATCH v5 21/27] iommu/arm-smmu-v3: Put the SVA mmu notifier in the smmu_domain Jason Gunthorpe
2024-03-19 16:23 ` Michael Shavit
2024-03-20 18:35 ` Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 22/27] iommu/arm-smmu-v3: Consolidate freeing the ASID/VMID Jason Gunthorpe
2024-03-19 16:44 ` Michael Shavit
2024-03-19 18:37 ` Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 23/27] iommu/arm-smmu-v3: Move the arm_smmu_asid_xa to per-smmu like vmid Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 24/27] iommu/arm-smmu-v3: Bring back SVA BTM support Jason Gunthorpe
2024-03-19 17:07 ` Michael Shavit
2024-03-20 13:05 ` Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 25/27] iommu/arm-smmu-v3: Allow IDENTITY/BLOCKED to be set while PASID is used Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 26/27] iommu/arm-smmu-v3: Allow a PASID to be set when RID is IDENTITY/BLOCKED Jason Gunthorpe
2024-03-04 23:44 ` [PATCH v5 27/27] iommu/arm-smmu-v3: Allow setting a S1 domain to a PASID Jason Gunthorpe
2024-03-15 10:40 ` [PATCH v5 00/27] Update SMMUv3 to the modern iommu API (part 2/3) Shameerali Kolothum Thodi
2024-03-23 13:38 ` Mostafa Saleh
2024-03-25 14:35 ` Jason Gunthorpe
2024-03-25 21:06 ` Mostafa Saleh
2024-03-25 22:44 ` Jason Gunthorpe
2024-03-25 10:22 ` Mostafa Saleh
2024-03-25 10:44 ` Shameerali Kolothum Thodi
2024-03-25 11:22 ` Mostafa Saleh
2024-03-25 16:47 ` Jason Gunthorpe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240327164251.GL946323@nvidia.com \
--to=jgg@nvidia.com \
--cc=eric.auger@redhat.com \
--cc=iommu@lists.linux.dev \
--cc=jean-philippe@linaro.org \
--cc=joro@8bytes.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=mdf@kernel.org \
--cc=mshavit@google.com \
--cc=nicolinc@nvidia.com \
--cc=patches@lists.linux.dev \
--cc=robin.murphy@arm.com \
--cc=shameerali.kolothum.thodi@huawei.com \
--cc=smostafa@google.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).