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>, Eric Auger <eric.auger@redhat.com>,
Moritz Fischer <mdf@kernel.org>,
Moritz Fischer <moritzf@google.com>,
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 v7 2/9] iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry()
Date: Mon, 29 Apr 2024 15:30:53 +0000 [thread overview]
Message-ID: <Zi-9LZGgT-PdhFWM@google.com> (raw)
In-Reply-To: <20240429142905.GF941030@nvidia.com>
On Mon, Apr 29, 2024 at 11:29:05AM -0300, Jason Gunthorpe wrote:
> On Sat, Apr 27, 2024 at 10:08:57PM +0000, Mostafa Saleh wrote:
> > > The issue is the old logic constructs the new CD by manipulating the
> > > existing CD in various ways "in place" that ends up creating CDs that
> > > don't meet the requirements for the new programmer. For instance EPD0
> > > will be set and the TTB0 will also be left programmed.
> > >
> >
> > I see, but what I don’t understand is why doesn't the function construct
> > the CD correctly, as from
>
> Why? Because it never had to before. It made minimal edits to minimize
> the code.
I understand, my point was why don’t we introduce a new logic to construct it
correctly, instead of hacking the old one, as it is much easier to reason
about (at least from my point of view)
>
> > } else if (cd == &quiet_cd) { /* (4) */
> > if (!(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
> > val &= ~(CTXDESC_CD_0_S | CTXDESC_CD_0_R);
> > val |= CTXDESC_CD_0_TCR_EPD0;
> > // populate the rest of the CD correctly here.
> > }
>
> What you are asking for is this:
>
> cd_live = !!(val & CTXDESC_CD_0_V);
>
> if (!cd) { /* (5) */
> + memset(cdptr, 0, sizeof(*cdptr));
> val = 0;
> } else if (cd == &quiet_cd) { /* (4) */
> + val &= ~(CTXDESC_CD_0_TCR_T0SZ | CTXDESC_CD_0_TCR_TG0 |
> + CTXDESC_CD_0_TCR_IRGN0 | CTXDESC_CD_0_TCR_ORGN0 |
> + CTXDESC_CD_0_TCR_SH0);
> if (!(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
> val &= ~(CTXDESC_CD_0_S | CTXDESC_CD_0_R);
> val |= CTXDESC_CD_0_TCR_EPD0;
> + cdptr->data[1] &= ~cpu_to_le64(CTXDESC_CD_1_TTB0_MASK);
> } else if (cd_live) { /* (3) */
> val &= ~CTXDESC_CD_0_ASID;
> val |= FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid);
>
> I think.. I've been staring at this a while now and I *think* it
> covers all the cases and we won't hit the WARN_ON?
>
That’s similar to how I imagined it.
> So sure, lets do it that way, the code is all deleted anyhow ..
>
I agree, if it's deleted anyway we shouldn't put much time, I haven't
looked at the SVA patch yet.
> > As I don’t think the right approach is to populate the CD incorrectly
> > and then clear the parts not needed for EPD0.
>
> It is very easy to see that such a simple algorithm will not trigger
> the WARN_ON. The above is somewhat trickier.
>
> > Also, TTB0 is ignored anyway in that case, no?
>
> Only by HW, there is a protective WARN_ON that will trigger in the
> programmer, that is what this is trying to avoid. For bisection.
Makes sense.
Thanks,
Mostafa
> Jason
next prev parent reply other threads:[~2024-04-29 15:31 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-16 19:28 [PATCH v7 0/9] Make the SMMUv3 CD logic match the new STE design (part 2a/3) Jason Gunthorpe
2024-04-16 19:28 ` [PATCH v7 1/9] iommu/arm-smmu-v3: Add an ops indirection to the STE code Jason Gunthorpe
2024-04-16 20:18 ` Nicolin Chen
2024-04-19 21:02 ` Mostafa Saleh
2024-04-22 13:09 ` Jason Gunthorpe
2024-04-16 19:28 ` [PATCH v7 2/9] iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry() Jason Gunthorpe
2024-04-16 20:48 ` Nicolin Chen
2024-04-18 13:01 ` Robin Murphy
2024-04-18 16:08 ` Jason Gunthorpe
2024-04-19 21:07 ` Mostafa Saleh
2024-04-22 13:29 ` Jason Gunthorpe
2024-04-27 22:08 ` Mostafa Saleh
2024-04-29 14:29 ` Jason Gunthorpe
2024-04-29 15:30 ` Mostafa Saleh [this message]
2024-04-16 19:28 ` [PATCH v7 3/9] iommu/arm-smmu-v3: Move the CD generation for S1 domains into a function Jason Gunthorpe
2024-04-16 21:22 ` Nicolin Chen
2024-04-19 21:10 ` Mostafa Saleh
2024-04-22 13:52 ` Jason Gunthorpe
2024-04-16 19:28 ` [PATCH v7 4/9] iommu/arm-smmu-v3: Consolidate clearing a CD table entry Jason Gunthorpe
2024-04-16 19:28 ` [PATCH v7 5/9] iommu/arm-smmu-v3: Make arm_smmu_alloc_cd_ptr() Jason Gunthorpe
2024-04-16 22:19 ` Nicolin Chen
2024-04-19 21:14 ` Mostafa Saleh
2024-04-22 14:20 ` Jason Gunthorpe
2024-04-27 22:19 ` Mostafa Saleh
2024-04-29 14:01 ` Jason Gunthorpe
2024-04-29 14:47 ` Mostafa Saleh
2024-04-29 14:55 ` Jason Gunthorpe
2024-04-16 19:28 ` [PATCH v7 6/9] iommu/arm-smmu-v3: Allocate the CD table entry in advance Jason Gunthorpe
2024-04-16 19:28 ` [PATCH v7 7/9] iommu/arm-smmu-v3: Move the CD generation for SVA into a function Jason Gunthorpe
2024-04-17 7:37 ` Nicolin Chen
2024-04-17 13:17 ` Jason Gunthorpe
2024-04-17 16:25 ` Nicolin Chen
2024-04-17 16:26 ` Nicolin Chen
2024-04-18 4:40 ` Michael Shavit
2024-04-18 14:28 ` Jason Gunthorpe
2024-04-16 19:28 ` [PATCH v7 8/9] iommu/arm-smmu-v3: Build the whole CD in arm_smmu_make_s1_cd() Jason Gunthorpe
2024-04-17 7:43 ` Nicolin Chen
2024-04-16 19:28 ` [PATCH v7 9/9] iommu/arm-smmu-v3: Add unit tests for arm_smmu_write_entry Jason Gunthorpe
2024-04-17 8:09 ` Nicolin Chen
2024-04-17 14:16 ` Jason Gunthorpe
2024-04-17 16:13 ` Nicolin Chen
2024-04-18 4:39 ` Michael Shavit
2024-04-18 12:48 ` Jason Gunthorpe
2024-04-18 14:34 ` Michael Shavit
2024-04-19 21:24 ` Mostafa Saleh
2024-04-22 14:24 ` Jason Gunthorpe
2024-04-27 22:33 ` Mostafa Saleh
2024-04-16 19:40 ` [PATCH v7 0/9] Make the SMMUv3 CD logic match the new STE design (part 2a/3) Nicolin Chen
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=Zi-9LZGgT-PdhFWM@google.com \
--to=smostafa@google.com \
--cc=eric.auger@redhat.com \
--cc=iommu@lists.linux.dev \
--cc=jgg@nvidia.com \
--cc=joro@8bytes.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=mdf@kernel.org \
--cc=moritzf@google.com \
--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=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).