Re: [PATCH v3 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Pranjal Shrivastava <praan@google.com>]

On Tue, Oct 15, 2024 at 09:03:54PM +0100, Robin Murphy wrote:
> On 2024-10-15 7:34 pm, Pranjal Shrivastava wrote:
> > Just a few things before I respin,
> > On Fri, Oct 11, 2024 at 01:59:36PM +0100, Robin Murphy wrote:
> > > On 2024-10-11 12:06 pm, Pranjal Shrivastava wrote:
> > > > On Fri, Oct 11, 2024 at 10:45:52AM +0000, Pranjal Shrivastava wrote:
> > > > > On Fri, Oct 11, 2024 at 11:21:48AM +0100, Robin Murphy wrote:
> > > > > > On 2024-10-11 8:55 am, Pranjal Shrivastava wrote:
> > > > > > > On Thu, Oct 03, 2024 at 03:53:15PM -0700, Nicolin Chen wrote:
> > > > > > > > On Thu, Oct 03, 2024 at 09:34:35PM +0000, Pranjal Shrivastava wrote:
> > > > > > > > > > The master and master_name are not that necessary to be in the
> > > > > > > > > > arm_smmu_event. I would keep both of them as local variables in
> > > > > > > > > > arm_smmu_handle_evt and pass master_name in to dump().
> > > > > > > > > 
> > > > > > > > > Hmm, right or maybe ONLY have the master_name in arm_smmu_event?
> > > > > > > > 
> > > > > > > > Just move both out of arm_smmu_event. Keep the master_name as a
> > > > > > > > local variable like the "master". You'd just need to pass it in
> > > > > > > > to the dump(). They both are in that same handle_evt().
> > > > > > > > 
> > > > > > > 
> > > > > > > Ack.
> > > > > > > 
> > > > > > > > > > > Also, shall we rename it to `arm_smmu_read_evt_info` ?
> > > > > > > > > > 
> > > > > > > > > > I'd probably use arm_smmu_event_get_from_raw()? Trying to high-
> > > > > > > > > > light struct arm_smmu_event v.s. u64 evt[EVTQ_ENT_DWORDS]. Yet,
> > > > > > > > > > no strong feeling about that.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Ack. How about `read_arm_smmu_event` :)
> > > > > > > > > Unless, we wanna follow the arm_smmu_<func> convention?
> > > > > > > > 
> > > > > > > > I think it'd be nicer if we do.
> > > > > > > > 
> > > > > > > 
> > > > > > > Ack. `arm_smmu_get_evt_from_raw` it is!
> > > > > > 
> > > > > > FWIW that sounds needlessly overcomplicated to me - the "raw" event array
> > > > > > should be a member of arm_smmu_event itself, since it seems silly to have
> > > > > > them separate with one pointing to the other when they have the exact same
> > > > > > scope and lifetime anyway. There still shouldn't need to be more than a
> > > > > > single logical step to process an evtq record into a finished
> > > > > > arm_smmu_event, just that that processing is now going to go a bit further
> > > > > > than simply le64_to_cpu().
> > > > > 
> > > > > Hmm, are you suggesting something like queue_remove_raw(q, event->raw)?
> > > > > OR do you mean we should have our own queue_parse_evt(q, &event) that
> > > > > parses out the raw event into an `arm_smmu_event` record eliminating the
> > > > > need for the `arm_smmu_read_evt_info` altogether?
> > > > > 
> > > > > Because we'd still need to store the raw event anyway since we're still
> > > > > logging the raw event.
> > > > > 
> > > > > I think we can go with the former, i.e. queue_remove_raw(q, event->raw)
> > > > > and then use ``arm_smmu_read_evt_info` to populate other `event` fields
> > > > 
> > > > Based on your comment, I was thinking like the attached sample code.
> > > > LMK if you were referring to something like that?
> > > 
> > > Ah, apologies, it had slipped my mind that queue_read() is already buried
> > > two layers deep in an abstraction shared with the priq... so yeah, rather
> > > than start trying to turn that inside-out, I guess the neatest approach is
> > > to effectively flip this series instead - i.e. start with the minor shift
> > > to:
> > > 
> > > 	struct arm_smmu_event evt;
> > > 
> > > 	while (!queue_remove_raw(q, evt.raw)) {
> > > 		ret = arm_smmu_handle_evt(smmu, &evt);
> > > 		...
> > > 		dev_info(smmu->dev, "event 0x%02x received:\n", evt.id);
> > > 		//etc.
> > > 
> > > and leave arm_smmu_handle_evt() decoding the event as both it and
> > > arm_smmu_handle_ppr() already do, just now using the new structure instead
> > > of locals. Then we can come in to factor out and enhance the unhandled event
> > > report, and anything else specific to that reporting beyond the actual SMMU
> > > event record, like device names and ratelimiting, can remain private to
> > > arm_smmu_dump_event().
> > 
> > Quick clarification, the ratelimiting here was ONLY for logs or to limit
> > the number of thread / loop executions?
> 
> Yes, the ratelimit is purely for the printks, so once those are factored out
> into a dedicated function it can move with them. The cond_resched() is what
> keeps the loop itself well-behaved if there's a giant flood of events; we
> never want to skip trying to handle any of them.
> 
> > > 
> > > AFAICS the locking concern seems avoidable as well - we don't actually need
> > > the whole arm_smmu_master in either case, just the device, so then end of
> > > arm_smmu_handle_evt() could easily be a little simpler:
> > > 
> > > 	...
> > > 	mutex_lock(&smmu->streams_mutex);
> > > 	master = arm_smmu_find_master(smmu, sid);
> > > 	if (master)
> > > 		event->dev = get_device(master->dev);
> > > 	mutex_unlock(&smmu->streams_mutex);
> > > 
> > > 	if (!event->dev)
> > > 		return -EINVAL;
> > 
> > Thinking about this a little more, I guess there are a few paths that
> > may return before we even get to the `mutex_lock(&smmu->streams_mutex)`
> > For example: if (!event->stall) return -ENOTSUPP; and the default case
> > return from the switch case. Which has the following implications:
> > 
> > 1. We might return from `arm_smmu_handle_evt` before logging the event
> > 2. Even if we move the logging out of `arm_smmu_handle_evt` as presented
> >     by Robin's snippet above, event->dev might still be NULL.
> 
> The point is that it *can* be NULL in general, and the logging must expect
> that. Just move the arm_smmu_find_master() stanza to the top of
> arm_smmu_handle_evt() to grab the device reference before any of the actual
> handling may start wanting to bail out - same as you'll want to do for all
> the other decoding as well - then we know it's either valid (and stable) or
> NULL for the lifetime of the event through all subsequent processing. AFAICS
> calling iommu_report_device_fault() under streams_mutex isn't meaningful in
> itself, right now it just seems to be about protecting the inline
> "master->dev" dereference against "master" going away.

Ahh, alright. Let me refactor that too then.

> 
> We can still have multiple returns as appropriate in the actual handling
> *after* collecting all the arm_smmu_event data, which is a big part of why
> I've suggested keeping the same effective structure as currently - we have
> one function to decode and try to handle the event, and based on the final
> outcome of that function, we may then want to report if it wasn't handled
> (subject to other constraints like ratelimiting). There's absolutely no need
> to tie the control flow in knots attempting to nano-optimise that into a
> tail call - using the returned value in the returned-to scope is natural,
> simple and obvious.
> 

Agreed.

> Thanks,
> Robin.

Thanks,
Pranjal