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

[Pranjal Shrivastava <praan@google.com>]

On Fri, Nov 01, 2024 at 02:41:07PM +0000, Robin Murphy wrote:
> On 2024-10-18 7:00 pm, Pranjal Shrivastava wrote:
> > Introduce `struct arm_smmu_event` to represent event records.
> > Parse out relevant fields from raw event records for ease and
> > use the new `struct arm_smmu_event` instead.
> > 
> > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 59 ++++++++++++++-------
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 20 +++++++
> >   2 files changed, 59 insertions(+), 20 deletions(-)
> > 
> > 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 737c5b882355..2f1108e5de51 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -1757,17 +1757,16 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> >   }
> >   /* IRQ and event handlers */
> > -static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> > +static int arm_smmu_handle_evt(struct arm_smmu_event *event)
> >   {
> >   	int ret = 0;
> >   	u32 perm = 0;
> >   	struct arm_smmu_master *master;
> > -	bool ssid_valid = evt[0] & EVTQ_0_SSV;
> > -	u32 sid = FIELD_GET(EVTQ_0_SID, evt[0]);
> >   	struct iopf_fault fault_evt = { };
> > +	struct arm_smmu_device *smmu = event->smmu;
> >   	struct iommu_fault *flt = &fault_evt.fault;
> > -	switch (FIELD_GET(EVTQ_0_ID, evt[0])) {
> > +	switch (event->id) {
> >   	case EVT_ID_TRANSLATION_FAULT:
> >   	case EVT_ID_ADDR_SIZE_FAULT:
> >   	case EVT_ID_ACCESS_FAULT:
> > @@ -1777,35 +1776,35 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> >   		return -EOPNOTSUPP;
> >   	}
> > -	if (!(evt[1] & EVTQ_1_STALL))
> > +	if (!event->stall)
> >   		return -EOPNOTSUPP;
> > -	if (evt[1] & EVTQ_1_RnW)
> > +	if (event->read)
> >   		perm |= IOMMU_FAULT_PERM_READ;
> >   	else
> >   		perm |= IOMMU_FAULT_PERM_WRITE;
> > -	if (evt[1] & EVTQ_1_InD)
> > +	if (event->instruction)
> >   		perm |= IOMMU_FAULT_PERM_EXEC;
> > -	if (evt[1] & EVTQ_1_PnU)
> > +	if (event->privileged)
> >   		perm |= IOMMU_FAULT_PERM_PRIV;
> >   	flt->type = IOMMU_FAULT_PAGE_REQ;
> >   	flt->prm = (struct iommu_fault_page_request) {
> >   		.flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
> > -		.grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
> > +		.grpid = event->stag,
> >   		.perm = perm,
> > -		.addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
> > +		.addr = event->iova,
> >   	};
> > -	if (ssid_valid) {
> > +	if (event->ssid_valid) {
> >   		flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
> > -		flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
> > +		flt->prm.pasid = event->ssid;
> >   	}
> >   	mutex_lock(&smmu->streams_mutex);
> > -	master = arm_smmu_find_master(smmu, sid);
> > +	master = arm_smmu_find_master(smmu, event->sid);
> >   	if (!master) {
> >   		ret = -EINVAL;
> >   		goto out_unlock;
> > @@ -1817,28 +1816,48 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> >   	return ret;
> >   }
> > +static void arm_smmu_get_event_from_raw(struct arm_smmu_device *smmu,
> > +					struct arm_smmu_event *event)
> 
> One would kind of expect "get event from raw" to take a raw thing and return
> an event... personally I'd still just inline this in arm_smmu_handle_evt()

Hmm.. I think we can do both of those things.

We can kzalloc struct arm_smmu_event and return it, that should also
reduce the stack size. However, I'm unsure if that'd slow things down
because kzalloc may be a little more expensive than a local variable..

For the inlining, just to ensure I understand, we're looking to keep the
old arm_smmu_handle_evt(smmu, raw_evt) as is and then decode the event
within arm_smmu_handle_evt before we start handling it, right?

I'm fine with both of the suggestions above, let me know your vote?

> itself, but otherwise something like arm_smmu_decode_evt() would seem a more
> logical and obvious name at this point.
> 
> > +{
> > +	/* Pick out the good stuff */
> > +	event->id = FIELD_GET(EVTQ_0_ID, event->raw[0]);
> > +	event->sid = FIELD_GET(EVTQ_0_SID, event->raw[0]);
> > +	event->ssid_valid = event->raw[0] & EVTQ_0_SSV;
> > +	event->ssid = event->ssid_valid ? FIELD_GET(EVTQ_0_SSID, event->raw[0]) : IOMMU_NO_PASID;
> > +	event->privileged = FIELD_GET(EVTQ_1_PnU, event->raw[1]);
> > +	event->instruction = FIELD_GET(EVTQ_1_InD, event->raw[1]);
> > +	event->s2 = FIELD_GET(EVTQ_1_S2, event->raw[1]);
> > +	event->read = FIELD_GET(EVTQ_1_RnW, event->raw[1]);
> > +	event->stag = FIELD_GET(EVTQ_1_STAG, event->raw[1]);
> > +	event->stall = event->raw[1] & EVTQ_1_STALL;
> > +	event->class = FIELD_GET(EVTQ_1_CLASS, event->raw[1]);
> > +	event->iova = FIELD_GET(EVTQ_2_ADDR, event->raw[2]);
> > +	event->ipa = FIELD_GET(EVTQ_3_IPA, event->raw[3]);
> > +	event->smmu = smmu;
> 
> The SMMU pointer isn't part of the raw event record... TBH I'd be inclined
> to leave it entirely separate, but if you really do want to hide it in the
> arm_smmu_event, at least keep things simple and initialise it outside the
> loop - it's not like it's ever going to change between events.

Yea.. honestly, just init-ing one member at a different time makes it
look a little weird if we allocate `arm_smmu_event` dynamically.
If we go ahead with allocating the `arm_smmu_event` using k*alloc, I
think it's best to remove the `smmu` member from the struct and pass it
around in functions.

> 
> Thanks,
> Robin.
> 

Thanks,
Praan

> > +}
> > +
> >   static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
> >   {
> >   	int i, ret;
> > +	struct arm_smmu_event evt;
> >   	struct arm_smmu_device *smmu = dev;
> >   	struct arm_smmu_queue *q = &smmu->evtq.q;
> >   	struct arm_smmu_ll_queue *llq = &q->llq;
> >   	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
> >   				      DEFAULT_RATELIMIT_BURST);
> > -	u64 evt[EVTQ_ENT_DWORDS];
> >   	do {
> > -		while (!queue_remove_raw(q, evt)) {
> > -			u8 id = FIELD_GET(EVTQ_0_ID, evt[0]);
> > +		while (!queue_remove_raw(q, evt.raw)) {
> > -			ret = arm_smmu_handle_evt(smmu, evt);
> > +			arm_smmu_get_event_from_raw(smmu, &evt);
> > +			ret = arm_smmu_handle_evt(&evt);
> >   			if (!ret || !__ratelimit(&rs))
> >   				continue;
> > -			dev_info(smmu->dev, "event 0x%02x received:\n", id);
> > -			for (i = 0; i < ARRAY_SIZE(evt); ++i)
> > +			dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> > +			for (i = 0; i < EVTQ_ENT_DWORDS; ++i)
> >   				dev_info(smmu->dev, "\t0x%016llx\n",
> > -					 (unsigned long long)evt[i]);
> > +					(unsigned long long)evt.raw[i]);
> >   			cond_resched();
> >   		}
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > index 1e9952ca989f..8a42d7b701fb 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -437,6 +437,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> >   #define EVTQ_0_ID			GENMASK_ULL(7, 0)
> > +/* Events */
> >   #define EVT_ID_TRANSLATION_FAULT	0x10
> >   #define EVT_ID_ADDR_SIZE_FAULT		0x11
> >   #define EVT_ID_ACCESS_FAULT		0x12
> > @@ -452,6 +453,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> >   #define EVTQ_1_RnW			(1UL << 35)
> >   #define EVTQ_1_S2			(1UL << 39)
> >   #define EVTQ_1_CLASS			GENMASK_ULL(41, 40)
> > +#define EVTQ_1_CLASS_TT			0x1
> >   #define EVTQ_1_TT_READ			(1UL << 44)
> >   #define EVTQ_2_ADDR			GENMASK_ULL(63, 0)
> >   #define EVTQ_3_IPA			GENMASK_ULL(51, 12)
> > @@ -771,6 +773,24 @@ struct arm_smmu_stream {
> >   	struct rb_node			node;
> >   };
> > +struct arm_smmu_event {
> > +	struct arm_smmu_device		*smmu;
> > +	u8				id;
> > +	u8				class;
> > +	u16				stag;
> > +	u32				sid;
> > +	u32				ssid;
> > +	u64				iova;
> > +	u64				ipa;
> > +	u64				raw[EVTQ_ENT_DWORDS];
> > +	bool				stall;
> > +	bool				ssid_valid;
> > +	bool				privileged;
> > +	bool				instruction;
> > +	bool				s2;
> > +	bool				read;
> > +};
> > +
> >   /* SMMU private data for each master */
> >   struct arm_smmu_master {
> >   	struct arm_smmu_device		*smmu;
>