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

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

[K V P, Satyanarayana]

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@intel.com>
---
 drivers/vfio/pci/vfio_pci_config.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index 523e0144c86f..d984c0dd6cca 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -96,6 +96,7 @@ static const u16 pci_ext_cap_length[PCI_EXT_CAP_ID_MAX + 1] = {
 	[PCI_EXT_CAP_ID_SECPCI]	=	0,	/* not yet */
 	[PCI_EXT_CAP_ID_PMUX]	=	0,	/* not yet */
 	[PCI_EXT_CAP_ID_PASID]	=	0,	/* not yet */
+	[PCI_EXT_CAP_ID_DVSEC]	=	0xFF,
 };
 
 /*
@@ -1440,6 +1441,13 @@ static int vfio_ext_cap_len(struct vfio_pci_core_device *vdev, u16 ecap, u16 epo
 			return PCI_TPH_BASE_SIZEOF + (sts * 2) + 2;
 		}
 		return PCI_TPH_BASE_SIZEOF;
+	case PCI_EXT_CAP_ID_DVSEC:
+		ret = pci_read_config_dword(pdev, epos + PCI_DVSEC_HEADER1, &dword);
+		if (ret)
+			return pcibios_err_to_errno(ret);
+
+		return PCI_DVSEC_HEADER1_LEN(dword);
+
 	default:
 		pci_warn(pdev, "%s: unknown length for PCI ecap %#x@%#x\n",
 			 __func__, ecap, epos);
-- 
2.34.1

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

[Jason Gunthorpe]

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@intel.com>
> ---
>  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.

Why would you use a "Platform Monitoring Technology" device with VFIO
anyhow?

Honestly I'm a bit reluctant to allow arbitary config space, some of
the stuff people put there can be dangerous.

Jason

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

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> 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@intel.com>
> > ---
> >  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. 

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

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

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@intel.com> wrote:

> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > 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@intel.com>
> > > ---
> > >  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

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

[Tian, Kevin]

> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Wednesday, March 8, 2023 7:29 AM
> 
> On Tue, 7 Mar 2023 05:54:46 +0000
> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
> > > From: Jason Gunthorpe <jgg@ziepe.ca>
> > > 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@intel.com>
> > > > ---
> > > >  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'.

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

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

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

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

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

[K V P, Satyanarayana]

> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Wednesday, March 8, 2023 1:21 PM
> To: Alex Williamson <alex.williamson@redhat.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>; K V P, Satyanarayana
> <satyanarayana.k.v.p@intel.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; cohuck@redhat.com
> Subject: RE: [PATCH] vfio/pci: Add DVSEC PCI Extended Config Capability to
> user visible list.
> 
> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Wednesday, March 8, 2023 7:29 AM
> >
> > On Tue, 7 Mar 2023 05:54:46 +0000
> > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >
> > > > From: Jason Gunthorpe <jgg@ziepe.ca>
> > > > 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@intel.com>
> > > > > ---
> > > > >  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