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

[K V P, Satyanarayana]

> From: Tian, Kevin <kevin.tian@xxxxxxxxx>
> Sent: Wednesday, March 8, 2023 1:21 PM
> To: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxx>; K V P, Satyanarayana
> <satyanarayana.k.v.p@xxxxxxxxx>; kvm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; cohuck@xxxxxxxxxx
> Subject: RE: [PATCH] vfio/pci: Add DVSEC PCI Extended Config Capability to
> user visible list.
> 
> > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Sent: Wednesday, March 8, 2023 7:29 AM
> >
> > 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.
> 
> from the description in v2 sounds like it's a telemetry/crashlog/etc.
> capability on a PCI device, though it's confusingly called 'platform'.
> 
For Intel's new Max series devices, telemetry/crashlog/watchers are retrieved using a PCI device
with DVSEC capability. In case of Pass-through virtualization, complete GPU is Passed to VM and 
user can get telemetry data inside VM only if PCI card with DVSEC capability is available.
Otherwise, no telemetry data can be retrieved inside VM.

The name 'platform' might have come from Integrated graphics and the same is being used for 
discrete graphics as well.
> >
> > > > 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.
> 
> Yep, this is weird. Even when a telemetry/crashlog feature is more for
> reading data from the device, there should be certain control knobs to
> enable/disable it then write access is also required.
Acknowledged. Will be adding rw support in V2 patch.
> 
> >
> > 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
> 
> oh, I was unaware of it.
> 
Acknowledged. Will be adding rw support in V2 patch.
> > something dangerous, at some point we need to rely on platform
> > isolation.
> 
> Probably it's easier for HW vendors to make security mistake in config
> space other than in MMIO bar. 😊 but I agree if nobody complains
> on how VSEC is handle today there is no reason why we should not do
> the same thing for DVSEC.
>
Thanks for feedback. Will submit V2 patch where DVSEC follows same as VSEC. 
> >
> > 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

Satya