Re: [PATCH v6 0/2] iommu/arm-smmu-v3: Parse out event records

[Pranjal Shrivastava <praan@google.com>]

On Tue, Dec 10, 2024 at 12:17:29AM +0000, Will Deacon wrote:
> On Tue, 03 Dec 2024 18:49:04 +0000, Pranjal Shrivastava wrote:
> > Enhance the arm-smmu-v3 driver to parse out useful information from
> > event records into a structure for better event handling & logging.
> > 
> > Some sample events, powered by QEMU:
> > 
> > 1. Bad StreamID:
> > 
> > [...]
> 
> Applied to will (for-joerg/arm-smmu/updates), thanks!
> 
> [1/2] iommu/arm-smmu-v3: Introduce struct arm_smmu_event
>       https://git.kernel.org/will/c/43ca55f5555b
> [2/2] iommu/arm-smmu-v3: Log better event records
>       https://git.kernel.org/will/c/d814b70b9b90
> 
> One thing I noticed when playing around with this on Qemu, however, is
> that we print the raw hex dump in reverse compared to the way in which
> the record tables are described in the spec. What do you think about
> doing something like the following on top of your series?
> 
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 143ff6336a95..d33a150b29ef 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1880,8 +1880,8 @@ static void arm_smmu_dump_raw_event(struct arm_smmu_device *smmu, u64 *raw,
>  
>  	dev_err(smmu->dev, "event 0x%02x received:\n", event->id);
>  
> -	for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> -		dev_err(smmu->dev, "\t0x%016llx\n", raw[i]);
> +	for (i = EVTQ_ENT_DWORDS - 1; i >= 0; --i)
> +		dev_err(smmu->dev, "[%u]\t0x%016llx\n", i, raw[i]);
>  }
>  
>  #define ARM_SMMU_EVT_KNOWN(e)	((e)->id < ARRAY_SIZE(event_str) && event_str[(e)->id])
> 
> 
> Then, for example, a permission fault looks like:
> 
> [   32.543815] arm-smmu-v3 arm-smmu-v3.0.auto: event 0x13 received:
> [   32.545115] arm-smmu-v3 arm-smmu-v3.0.auto: [3]	0x0000000000000000
> [   32.546361] arm-smmu-v3 arm-smmu-v3.0.auto: [2]	0x00000000fffff040
> [   32.548672] arm-smmu-v3 arm-smmu-v3.0.auto: [1]	0x0000020000000000
> [   32.550209] arm-smmu-v3 arm-smmu-v3.0.auto: [0]	0x0000002000000013
> [   32.551990] arm-smmu-v3 arm-smmu-v3.0.auto: event: F_PERMISSION client: 0000:00:04.0 sid: 0x20 ssid: 0x0 iova: 0xfffff040 ipa: 0x0
> [   32.553952] arm-smmu-v3 arm-smmu-v3.0.auto: unpriv data write s1 "Input address caused fault" stag: 0x0
> 
> I started going a step further and printing the records 32 bits at a
> time (again, to match the spec), but then I realised that the current
> code doesn't look like it handles big-endian properly...

Ack. I think I can post another series to refactor this to match the
spec by logging 32-bits at a time in Little-endian. I'll give it a go..

> 
> Finally, coccinelle suggests you could use str_read_write() instead of:
> 
> 	evt->read ? "read" : "write",
> 
> which I suppose we could.

Ack. I'll use `str_read_write` do you want me to club it with the above
refactor in a series?

> 
> Cheers,
> -- 
> Will

Thanks,
Praan