Re: [PATCH 4/5] vfio/type1: Flush CPU caches on DMA pages in non-coherent domains

[Jason Gunthorpe]

On Tue, May 21, 2024 at 12:19:45PM -0600, Alex Williamson wrote:
> > I'm OK with this. If devices are insecure then they need quirks in
> > vfio to disclose their problems, we shouldn't punish everyone who
> > followed the spec because of some bad actors.
> > 
> > But more broadly in a security engineered environment we can trust the
> > no-snoop bit to work properly.
> 
>  The spec has an interesting requirement on devices sending no-snoop
>  transactions anyway (regarding PCI_EXP_DEVCTL_NOSNOOP_EN):
> 
>  "Even when this bit is Set, a Function is only permitted to Set the No
>   Snoop attribute on a transaction when it can guarantee that the
>   address of the transaction is not stored in any cache in the system."
> 
> I wouldn't think the function itself has such visibility and it would
> leave the problem of reestablishing coherency to the driver, but am I
> overlooking something that implicitly makes this safe?  

I think it is just bad spec language! People are clearly using
no-snoop on cachable memory today. The authors must have had some
other usage in mind than what the industry actually did.

> But there's no capability bit that allows us to report whether the
> device supports no-snoop, we're just hoping that a driver writing to
> the bit doesn't generate a fault if the bit doesn't stick.  For example
> the no-snoop bit in the TLP itself may only be a bandwidth issue, but
> if the driver thinks no-snoop support is enabled it may request the
> device use the attribute for a specific transaction and the device
> could fault if it cannot comply.

It could, but that is another wierdo quirk IMHO. We already see things
in config space under hypervisor control because the VF don't have the
bits :\

> > > The more secure approach might be that we need to do these cache
> > > flushes for any IOMMU that doesn't maintain coherency, even for
> > > no-snoop transactions.  Thanks,  
> > 
> > Did you mean 'even for snoop transactions'?
> 
> I was referring to IOMMUs that maintain coherency regardless of
> no-snoop transactions, ie domain->enforce_cache_coherency (ex. snoop
> control/SNP on Intel), so I meant as typed, the IOMMU maintaining
> coherency even for no-snoop transactions.
> 
> That's essentially the case we expect and we don't need to virtualize
> no-snoop enable on the device.

It is the most robust case to be sure, and then we don't need
flushing.

My point was we could extend the cases where we don't need to flush if
we pay attention to, or virtualize, the PCI_EXP_DEVCTL_NOSNOOP_EN.

> > That is where this series is, it assumes a no-snoop transaction took
> > place even if that is impossible, because of config space, and then
> > does pessimistic flushes.
> 
> So are you proposing that we can trust devices to honor the
> PCI_EXP_DEVCTL_NOSNOOP_EN bit and virtualize it to be hardwired to zero
> on IOMMUs that do not enforce coherency as the entire solution?

Maybe not entire, but as an additional step to reduce the cost of
this. ARM would like this for instance.
 
> Or maybe we trap on setting the bit to make the flushing less
> pessimistic?

Also a good idea. The VMM could then decide on policy.

Jason