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

[Pranjal Shrivastava <praan@google.com>]

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.

The architects provided bits 51:12 as IPA since the min Translation
Granule supported by SMMUv3 is 4KB, hence the last 12-bits will be 0s.
Thus, an "IPA" or "PA" (in S1 only) *has* to have the last 12-bits as 0s
hence, I'd believe IPA = 0xFFFFF would not make sense as min TG is 4KB
and 0xFFFFF is not 4K aligned.

> >
> > Also, since the definition of the field is GENMASK_ULL(51,12) it doesn't
> > work for events like `F_STE_FETCH` where the Fetch address field is:
> > GENMASK_ULL(51,3).
> 
> Using the ipa member for FetchAddr appears to be a bit hacky. Can you
> use a union in the struct that combines ipa and fetchaddr (or
> fetch_addr), and then decode either field depending on the event id? I
> understand that no event type has both FetchAddr and IPA fields.
> 

Ack. We can use unions, but I'd still say that we can simply read raw[3]
for all addresses as the spec marks them as 0 (not RES0).

> >
> > 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?

Additionally, I don't think we can generalize this a lot because the
SMMUv3 spec also says stuff like: "In SMMUv3.0, FetchAddr bits [51:48]
are RES0." Now in this case, we'll have to read the IDR, figure out the
SMMU version and then accordingly mask out RES0 bits because they can be
UNKNOWN, which in my opinion would be a bit too much.

Thanks,
Praan