Re: [PATCH] iommu/arm-smmu-v3: Fix event queue overflow acknowledgment

From: Tomas Krcka <krckatom@amazon.de>
To: <robin.murphy@arm.com>
Cc: <baolu.lu@linux.intel.com>, <iommu@lists.linux.dev>,
	<joro@8bytes.org>, <krckatom@amazon.de>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>,
	<shameerali.kolothum.thodi@huawei.com>, <will@kernel.org>
Subject: Re: [PATCH] iommu/arm-smmu-v3: Fix event queue overflow acknowledgment
Date: Wed, 8 Mar 2023 14:02:04 +0000	[thread overview]
Message-ID: <20230308140204.83249-1-krckatom@amazon.de> (raw)
In-Reply-To: <8291b66d-b9b8-47c9-f5ed-a4e951c92154@arm.com>

>> When an overflow occurs in the event queue, the SMMU toggles overflow
>> flag OVFLG in the PROD register.
>> The evtq thread is supposed to acknowledge the overflow flag by toggling
>> flag OVACKFLG in the CONS register, otherwise the overflow condition is
>> still active (OVFLG != OVACKFLG).
>>
>> Currently the acknowledge register is toggled after clearing the event
>> queue but is never propagated to the hardware. It would be done next
>> time when executing evtq thread.
>>
>> The SMMU still adds elements to the queue when the overflow condition is
>> active but any subsequent overflow information after clearing the event
>> queue will be lost.
>>
>> This change keeps the SMMU in sync as it's expected by design.
>
> If I've understood correctly, the upshot of this is that if the queue
> has overflowed once, become empty, then somehow goes from empty to full
> before we manage to consume a single event, we won't print the "events
> lost" message a second time.
>
> Have you seen this happen in practice? TBH if the event queue ever
> overflows even once it's indicative that the system is hosed anyway, so
> it's not clear to me that there's any great loss of value in sometimes
> failing to repeat a warning for a chronic ongoing operational failure.
>

Yes, I did see in practice. And it’s not just about loosing subsequence warning.
The way how it’s done now keeps inconsistent CONS register value between SMMU and the kernel
until any new event happens. The kernel doesn’t inform SMMU that we know about the overflow
and consuming events as fast as we can.

> It could be argued that we have a subtle inconsistency between
> arm_smmu_evtq_thread() and arm_smmu_priq_thread() here, but the fact is
> that the Event queue and PRI queue *do* have different overflow
> behaviours, so it could equally be argued that inconsistency in the code
> helps reflect that. FWIW I can't say I have a strong preference either way.

For the argument that the code can reflect the difference.
Then the comment 'Sync our overflow flag, as we believe we're up to speed’ is
already misleading.

Thanks.
BR,
Tomas

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel