Re: [PATCH v5 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava <praan@google.com>]

On Thu, Nov 28, 2024 at 03:40:51PM -0800, Daniel Mentz wrote:
> On Wed, Nov 27, 2024 at 12:53 PM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Wed, Nov 27, 2024 at 11:42:11AM -0800, Daniel Mentz wrote:
> > > On Wed, Nov 27, 2024 at 1:25 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > > > > +       event->class = FIELD_GET(EVTQ_1_CLASS, raw[1]);
> > > > > > +       event->iova = FIELD_GET(EVTQ_2_ADDR, raw[2]);
> > > > > > +       event->ipa = raw[3];
> > > > >
> > > > > Shouldn't this be
> > > > >
> > > > > event->ipa =FIELD_GET(EVTQ_3_IPA, raw[3]);
> > > >
> > > > No. For this, there's a reason for not using the
> > > > `FIELD_GET(EVTQ_3_IPA, raw[3]);`. As mentioned in the cover letter, this
> > > > was the root cause of the IPA truncation bug, i.e. FIELD_GET only
> > > > fetches the IPA field without trailing zeroes, truncating the address.
> > > > For e.g. IPA = 0xfffff instead of 0xfffff000 which is faulting address.
> > >
> > > I disagree with the term truncation bug, because the information was
> > > never there in the first place. Truncating the IPA was presumably a
> > > conscious decision made by the architect. It is not a bug.
> > >
> >
> > I'm NOT claiming that the HW or architecture has a bug.
> > I meant that SW can't ONLY look at (and log) bits 51:12 only which the
> > `FIELD_GET(EVTQ_3_IPA, raw[3]);` does (the values I've shared in the
> > cover letter are actual values printed on QEMU). If we'd wanna use the
> > `FIELD_GET(EVTQ_3_IPA, raw[3]);` then we'd also have to shift it by 12.
> 
> I believe that using FIELD_GET(EVTQ_3_IPA, raw[3]) and then left
> shifting by 12 is the correct approach. As per the Arm ARM, software
> shouldn't rely on RES0 bits being 0.
> 

Ack. Looking at the G.a version[1] of the spec, I agree with this.
Apologies for the confusion!

I'm planning to take the raw[3] dword, mask it appropriately and store
it in 2 different members, ipa & fetch_addr: 

	event->ipa = raw[3] & EVTQ_3_IPA_MASK;
	event->fetch_addr = raw[3] & EVTQ_3_FETCH_ADDR_MASK;

This way, we can use either field as required and not clobber the decode
func with if (event->id == 'a' || event->id == 'b' || event->id == 'c').
Allowing the user to use the appropriate member  as necessary.

For example, in the `arm_smmu_dump_event` we'll use event->ipa or
event->fetch_addr as per the `event->id`

I'm slightly against the `event_id` based checks because it may not
scale well if (in the future) more fetch faults are added.

> > > >
> > > > Since the SMMUv3 spec ensures that the bitfields NOT representing an
> > > > address, i.e. fields other than GENMASK_ULL(51,12) or GENMASK_ULL(51,3),
> > > > are zeros. It's safe to read the raw[3] dword directly.
> > >
> > > Somewhere in the Arm ARM, I found the following language:
> > >
> > > "The RES0 description can apply to bits or fields that are read-only,
> > > or are write-only: For a read-only bit, RES0 indicates that the bit
> > > reads as 0, but software must treat the bit as UNKNOWN."
> > > "A bit that is RES0 in a context is reserved for possible future use
> > > in that context. To preserve forward compatibility, software: Must not
> > > rely on the bit reading as 0."
> > >
> > > As it stands, I believe the code relies on RES0 fields reading 0 which
> > > would violate this rule.
> >
> > The fields I'm talking about aren't marked as `RES0` in the spec. For
> > example, if we look at Section 7.3.4 in the spec, F_STE_FETCH
> > *explicitly* marks non-address fields as 0 not RES0. The same can be
> > noticed in sections 7.3.10, 7.3.12 - 7.3.17 & 7.3.20. Unless I missed
> > something that mentions that the "0" fields in event records are RES0?
> 
> It's RES0 in version G.a of the spec. It appears as if Arm changed
> that between versions D.a and D.b.

Ahh alright. I was referring to D.a version. Thanks for catching this!
The G.a version clearly marks everything as RES0 which can cause trouble

Thanks,
Praan

[1] https://developer.arm.com/documentation/ihi0070/latest/