Message-ID: <20240521121945.7f144230.alex.williamson@redhat.com>
Date: Tue, 21 May 2024 12:19:45 -0600
From: Alex Williamson <alex.williamson@...hat.com>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: "Tian, Kevin" <kevin.tian@...el.com>, "Vetter, Daniel"
 <daniel.vetter@...el.com>, "Zhao, Yan Y" <yan.y.zhao@...el.com>,
 "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>,
 "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>, "pbonzini@...hat.com"
 <pbonzini@...hat.com>, "seanjc@...gle.com" <seanjc@...gle.com>,
 "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
 "luto@...nel.org" <luto@...nel.org>, "peterz@...radead.org"
 <peterz@...radead.org>, "tglx@...utronix.de" <tglx@...utronix.de>,
 "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de" <bp@...en8.de>,
 "hpa@...or.com" <hpa@...or.com>, "corbet@....net" <corbet@....net>,
 "joro@...tes.org" <joro@...tes.org>, "will@...nel.org" <will@...nel.org>,
 "robin.murphy@....com" <robin.murphy@....com>, "baolu.lu@...ux.intel.com"
 <baolu.lu@...ux.intel.com>, "Liu, Yi L" <yi.l.liu@...el.com>
Subject: Re: [PATCH 4/5] vfio/type1: Flush CPU caches on DMA pages in
 non-coherent domains

On Tue, 21 May 2024 13:34:00 -0300
Jason Gunthorpe <jgg@...dia.com> wrote:

> On Tue, May 21, 2024 at 10:21:23AM -0600, Alex Williamson wrote:
> 
> > > Intel GPU weirdness should not leak into making other devices
> > > insecure/slow. If necessary Intel GPU only should get some variant
> > > override to keep no snoop working.
> > > 
> > > It would make alot of good sense if VFIO made the default to disable
> > > no-snoop via the config space.  
> > 
> > We can certainly virtualize the config space no-snoop enable bit, but
> > I'm not sure what it actually accomplishes.  We'd then be relying on
> > the device to honor the bit and not have any backdoors to twiddle the
> > bit otherwise (where we know that GPUs often have multiple paths to get
> > to config space).  
> 
> 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?  ie. if the
function isn't permitted to perform no-snoop to an address stored in
cache, there's nothing we need to do here.

> > We also then have the question of does the device function
> > correctly if we disable no-snoop.  
> 
> Other than the GPU BW issue the no-snoop is not a functional behavior.

As with some other config space bits though, I think we're kind of
hoping for sloppy driver behavior to virtualize this.  The spec does
allow the bit to be hardwired to zero:

 "This bit is permitted to be hardwired to 0b if a Function would never
  Set the No Snoop attribute in transactions it initiates."

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.

> > 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.

> 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?

Or maybe we trap on setting the bit to make the flushing less
pessimistic?

Intel folks might be able to comment on the performance hit relative to
iGPU assignment of denying the device the ability to use no-snoop
transactions (assuming the device control bit is actually honored).
The latency of flushing caches on touching no-snoop enable might be
prohibitive in the latter case.  Thanks,

Alex

Powered by blists - more mailing lists