On Tue, Sep 10, 2024 at 05:22:51PM -0300, Jason Gunthorpe wrote:

On Tue, Sep 10, 2024 at 10:55:51AM +0000, Mostafa Saleh wrote:

quoted

On Tue, Sep 03, 2024 at 08:33:40PM -0300, Jason Gunthorpe wrote:

quoted

On Tue, Sep 03, 2024 at 07:57:01AM +0000, Mostafa Saleh wrote:

quoted

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?

Well, I'd argue that it provides more certainty for the kernel that
the DMA API behavior is matched by HW behavior. But I don't feel strongly.

I adjusted the patch to only enable it for nesting parents.

quoted

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.

Not quite, the purpose of FWB is to allow the hypervisor to avoid
costly cache flushing. It is specifically to protect the hypervisor
against a VM causing the caches to go incoherent.

Caches that are unexpectedly incoherent are a security problem for the
hypervisor.

I see, thanks for explaining, I got confused about the device emulation case,
it’s also about corruption because of a mismatch of memory attributes,
something like:
https://bugzilla.redhat.com/show_bug.cgi?id=1679680

At the moment, I see KVM doesn’t really touch guest memory, but it does CMO for
guest map(in case memslot had already some data) and on unmap, which I
believe has significant performance improvement.

quoted

quoted

quoted

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?

Because iommufd/vfio doesn't have cache flushing.
 

I see.

quoted

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.

When KVM turns on S2FWB it stops doing cache flushing. As I understand
it S2FWB is significantly a performance optimization.

On the VFIO side we don't have cache flushing at all. So enforcing
cache consistency is mandatory for security.

For native VFIO we set IOMMU_CACHE and expect that the contract with
the IOMMU is that no cache flushing is required.

For nested we set S2FWB/CANWBS to prevent the VM from disabling VFIO's
IOMMU_CACHE and again the contract with the HW is that no cache
flushing is required.

Thus VFIO is security correct even though it doesn't cache flush.

None of this has anything to do with device coherence capability. It
is why I keep saying incoherent devices must be blocked from VFIO
because it cannot operate them securely/correctly.

Fixing that is a whole other topic, Yi has a series for it on x86 at
least..

I see, that makes sense to only support it for nested domains on
the assumption they are only used for VFIO/IOMMUFD till we figure out
non-coherent devices, I guess you are referring to:
https://lore.kernel.org/all/ZltQ3PyHKiQmN9SU@nvidia.com/t/#me702dd242782393eb7769000c96702a0fed7f6ca (local)

Thanks,
Mostafa

Jason