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

[Pranjal Shrivastava <praan@google.com>]

On Tue, Nov 26, 2024 at 07:25:12PM -0800, Daniel Mentz wrote:
> On Tue, Nov 12, 2024 at 12:30 AM Pranjal Shrivastava <praan@google.com> wrote:
> > +       event->stag = FIELD_GET(EVTQ_1_STAG, raw[1]);
> > +       event->stall = raw[1] & EVTQ_1_STALL;
> 
> Any particular reason you're not using FIELD_GET on EVTQ_1_STALL like
> in FIELD_GET(EVTQ_1_STALL, raw[1]); ?
> 

No particular reason as such, just wanted to ensure a bool value as the
current usage in `arm_smmu_handle_evt` fetched the stall field similarly

I'll update this to use a FIELD_GET 

> > +       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.

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).

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.

I think this wasn't discovered until now as we are NOT using the
EVTQ_3_IPA mask anywhere in the `arm-smmu-v3*` source files.


Thanks,
Praan