Re: [PATCH] vfio/pci: Add DVSEC PCI Extended Config Capability to user visible list.

[Alex Williamson]

On Tue, 7 Mar 2023 05:54:46 +0000
"Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:

> > From: Jason Gunthorpe <jgg@xxxxxxxx>
> > Sent: Monday, March 6, 2023 9:58 PM
> > 
> > On Fri, Mar 03, 2023 at 05:54:26AM +0000, K V P, Satyanarayana wrote:  
> > > Intel Platform Monitoring Technology (PMT) support is indicated by  
> > presence  
> > > of an Intel defined PCIe Designated Vendor Specific Extended Capabilities
> > > (DVSEC) structure with a PMT specific ID.However DVSEC structures may  
> > also  
> > > be used by Intel to indicate support for other features. The Out Of Band  
> > Management  
> > > Services Module (OOBMSM) uses DVSEC to enumerate several features,  
> > including PMT.  
> > >
> > > The current VFIO driver does not pass DVSEC capabilities to virtual machine  
> > (VM)  
> > > which makes intel_vsec driver not to work in the VM. This series adds  
> > DVSEC  
> > > capability to user visible list to allow its use with VFIO.
> > >
> > > Signed-off-by: K V P Satyanarayana <satyanarayana.k.v.p@xxxxxxxxx>
> > > ---
> > >  drivers/vfio/pci/vfio_pci_config.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)  
> > 
> > Wasn't the IDXD/SIOV team proposing to use the fact that DVSEC doesn't
> > propogate to indicate that IMS doesn't work?
> > 
> > Did this plan get abandoned? It seems at odds with this patch.  
> 
> No. Guest IMS will be indicated via hypercall/vIR as planned. 

Thank goodness, basing a feature on the absence of a capability that's
subject to change would have really put us, or IMS, in a corner.

> > Why would you use a "Platform Monitoring Technology" device with VFIO
> > anyhow?  
> 
> Ack. I guess it's a monitoring capability per PCI device to form a
> platform-level monitoring technology. But w/o all those background
> and usage description it's really strange to pass a 'platform' capability
> into a guest.

Is this perhaps for validation of the device, because yes, assigning
platform devices to a VM doesn't seem like something a system vendor
would tend to promote.

> > Honestly I'm a bit reluctant to allow arbitary config space, some of
> > the stuff people put there can be dangerous.
> >   
> 
> Probably an allowed list to manage which DVSEC ID can be exposed
> to userspace via vfio-pci, e.g. if the PMT ID in this patch is proved
> to be safe for a meaningful usage?

Well, let me take this a different direction because the support
proposed here only allows read-only access to the DVSEC capability.  Is
that actually useful?  Drivers making use of write access to DVSEC are
going to fail in unpredictable ways if their writes are dropped.  That
seems worse than our current state of hiding it.

We already provide raw write access to both the standard and extended
vendor specific capabilities, why wouldn't we by default do the same
for DVSEC?  Devices aren't limited to config space if they want to do
something dangerous, at some point we need to rely on platform
isolation.

If there are underlying concerns here, then we probably need some sort
of opt-in policy which restricts vfio-pci from binding to anything but
VFs.  Thanks,

Alex