Re: [PATCH v2 2/8] iommu/arm-smmu-v3: Use S2FWB when available

[Mostafa Saleh <smostafa@google.com>]

On Tue, Sep 03, 2024 at 08:33:40PM -0300, Jason Gunthorpe wrote:
> On Tue, Sep 03, 2024 at 07:57:01AM +0000, Mostafa Saleh wrote:
> 
> > Basically, I believe we shouldn’t set FWB blindly just because it’s supported,
> > I don’t see how it’s useful for stage-2 only domains.
> 
> And the only problem we can see is some niche scenario where incoming
> memory attributes that are already requesting cachable combine to a
> different kind of cachable?

No, it’s not about the niche scenario, as I mentioned I don’t think
we should enable FWB because it just exists. One can argue the opposite,
if S2FWB is no different why enable it?

AFAIU, FWB would be useful in cases where the hypervisor(or VMM) knows
better than the VM, for example some devices MMIO space are emulated so
they are normal memory and it’s more efficient to use memory attributes.

Taking into consideration all the hassle that can happen if non-coherent
devices use the wrong attribute, I’d suggest either set FWB only for
coherent devices (I know it’s not easy to define, but maybe be should?)
or we have a new CAP where the caller is aware of that. But I don’t think
the driver should decide that on behalf of the caller.

> 
> > And I believe making assumptions about VFIO (which actually is not correctly
> > enforced at the moment) is fragile.
> 
> VFIO requiring cachable is definately not fragile, and it also sets
> the IOMMU_CACHE flag to indicate this. Revising VFIO to allow
> non-cachable would be a signficant change and would also change what
> IOMMU_CACHE flag it sets.
> 

I meant the driver shouldn't assume the caller behaviour, if it's VFIO
or something new.

> > and we should only set FWB for coherent
> > devices in nested setup only where the VMM(or hypervisor) knows better than
> > the VM.
> 
> I don't want to touch the 'only coherent devices' question. Last time
> I tried to do that I got told every option was wrong.
> 
> I would be fine to only enable for nesting parent domains. It is
> mandatory here and we definitely don't support non-cachable nesting
> today.  Can we agree on that?
> 
Why is it mandatory?

I think a supporting point for this, is that KVM does the same for
the CPU, where it enables FWB for VMs if supported. I have this on
my list to study if that can be improved. But may be if we are out
of options that would be a start.

> Keep in mind SMMU S2FWB is really new and probably very little HW
> supports it right now. So we are not breaking anything existing
> here. IMHO it is better to always enable the stricter features going
> forward, and then evaluate an in-kernel opt-out if someone comes with
> a concrete use case.
> 

I agree, it’s unlikely that this breaks existing hardware, but I’d
be concerned if FWB is enabled unconditionally it breaks devices in
the future and we end up restricting it more.

Thanks,
Mostafa
> Jason