Re: [PATCH] xen/arm: smmuv3: Add cache maintenance for non-coherent SMMU queues

[Dmytro Firsov <Dmytro_Firsov@xxxxxxxx>]

Hi Julien, Michal

On 28.08.25 00:42, Julien Grall wrote:
> Hi Michal,
>
> On 26/08/2025 12:44, Orzel, Michal wrote:
>>
>>
>> On 26/08/2025 12:48, Julien Grall wrote:
>>> Hi,
>>>
>>> On 26/08/2025 10:47, Dmytro Firsov wrote:
>>>> On 22.08.25 11:12, Orzel, Michal wrote:
>>>>
>>>> There are already a few places advertising the SMMU coherency:
>>>> 1) smmu->features
>>>> 2) d->iommu->features
>>>> 3) platform_features
>>>>
>>>> All of them are better places than queue struct (that as you 
>>>> pointed out is not
>>>> specific to coherency). I'd suggest maybe to use 3) and removing 
>>>> ro_after_init
>>>> if you don't have access to 1) and 2). All in all, providing yet 
>>>> another place
>>>> for coherency flag seems a bit too much.
>>>   > >
>>>> First of all, thank you very much for review! I will consider using
>>>> "platform_features" in next patch version, it looks more 
>>>> appropriate and
>>>> should be available within the whole driver. Also, I believe 
>>>> "ro_after_init"
>>>> is also OK, since I have no need to change it (only check if cache
>>>> maintenance should be performed).
>>>
>>> I have to disagree with using "platform_features". Flushing the 
>>> queue is
>>> a per-SMMU decision. But looking at the code, I think passing the SMMU
>>> to the caller would look wrong (what if you mistakenly pass the wrong
>>> SMMU?). So I think a boolean per queue is the right appraoch.
>> For my own understanding: don't we consider SMMU coherency globally, 
>> not per
>> SMMU (hence why I suggested re-using the global flag)?
> > We set the coherency> feature in the flags if all SMMUs support it. 
> Do we want to diverge now and do
>> the flushing per SMMU?
>
> The two contexts are quite different.
>
> We need the global flag when cleaning the stage-2 page-tables because 
> we don't always know at boot which SMMUs will be used for the devices 
> attached (think PCI hotplug).
>
> In the context of this patch, we know the queue is only used by a 
> given SMMU. So i tis better to take this decision per-SMMU/queue to 
> reduce the number of cache flush. This will be particularly important 
> for the 2-stage SMMU work because there will be a lot of commands sent 
> on behalf of the guest (every TLB flushes will be command).
>
> Cheers,
Thank you for the review and comments.

I wanted to follow up on the discussion. Michal, I still need to 
clarify: do you agree with the per-queue flag approach?
If so, I will simplify the header comment to "Is memory access 
coherent?", add Julien's RB tag, and post v2.


Also, thank you, Mykola, for the testing and TB tag.

---

BR,

Dmytro.