[PATCH 0/5] Legacy direct-assign mode

[PATCH 0/5] Legacy direct-assign mode

[Jon Derrick]

This set adds a legacy direct-assign mode. Newer enterprise hardware has
physical addressing hints to assist device passthrough to guests that needs to
correctly program bridge windows with physical addresses. Some customers are
using a legacy method that relies on the VMD subdevice domain's root port
windows to be written with the physical addresses. This method also allows
other hypervisors besides QEMU/KVM to perform guest passthrough.

This patchset adds a host and guest mode to write the physical address
information to the root port registers in the host and read them in the guest,
and restore them in both cases on module unload.

This patchset also folds in the VMD subdevice domain secondary bus reset
patchset [1] to clear the domain prior to guest passthrough.

[1] https://patchwork.kernel.org/project/linux-pci/cover/20200928010557.5324-1-jonathan.derrick@intel.com/

Jon Derrick (5):
  PCI: vmd: Reset the VMD subdevice domain on probe
  PCI: Add a reset quirk for VMD
  PCI: vmd: Add offset translation helper
  PCI: vmd: Pass features to vmd_get_phys_offsets()
  PCI: vmd: Add legacy guest passthrough mode

 drivers/pci/controller/vmd.c | 200 ++++++++++++++++++++++++++++++++++++++-----
 drivers/pci/quirks.c         |  48 +++++++++++
 2 files changed, 227 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] PCI: vmd: Reset the VMD subdevice domain on probe

[Jon Derrick]

The VMD subdevice domain resource requirements may have changed
in-between module loads. Generic PCI resource assignment code may rely
on existing resource configuration rather than the VMD preference of
re-examining the domain. Add a Secondary Bus Reset to the VMD subdevice
domain during driver attachment to clear the PCI config space of the
subdevices.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/controller/vmd.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index c31e4d5..c7b5614 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -14,6 +14,7 @@
 #include <linux/srcu.h>
 #include <linux/rculist.h>
 #include <linux/rcupdate.h>
+#include <linux/delay.h>
 
 #include <asm/irqdomain.h>
 #include <asm/device.h>
@@ -424,6 +425,36 @@ static int vmd_pci_write(struct pci_bus *bus, unsigned int devfn, int reg,
 	.write		= vmd_pci_write,
 };
 
+static void vmd_domain_reset_sbr(struct vmd_dev *vmd)
+{
+	char __iomem *base;
+	int rp;
+	u16 ctl;
+
+	/*
+	 * Subdevice config space is mapped linearly using 4k config space
+	 * increments. Use increments of 0x8000 to locate root port devices.
+	 */
+	for (rp = 0; rp < 4; rp++) {
+		base = vmd->cfgbar + rp * 0x8000;
+		if (readl(base + PCI_COMMAND) == 0xFFFFFFFF)
+			continue;
+
+		/* pci_reset_secondary_bus() */
+		ctl = readw(base + PCI_BRIDGE_CONTROL);
+		ctl |= PCI_BRIDGE_CTL_BUS_RESET;
+		writew(ctl, base + PCI_BRIDGE_CONTROL);
+		readw(base + PCI_BRIDGE_CONTROL);
+		msleep(2);
+
+		ctl &= ~PCI_BRIDGE_CTL_BUS_RESET;
+		writew(ctl, base + PCI_BRIDGE_CONTROL);
+		readw(base + PCI_BRIDGE_CONTROL);
+	}
+
+	ssleep(1);
+}
+
 static void vmd_attach_resources(struct vmd_dev *vmd)
 {
 	vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1];
@@ -707,6 +738,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	if (vmd->irq_domain)
 		dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
 
+	vmd_domain_reset_sbr(vmd);
 	pci_scan_child_bus(vmd->bus);
 	pci_assign_unassigned_bus_resources(vmd->bus);
 
-- 
1.8.3.1

[PATCH 2/5] PCI: Add a reset quirk for VMD

[Jon Derrick]

VMD domains should be reset in-between special attachment such as VFIO
users. VMD does not offer a reset, however the subdevice domain itself
can be reset starting at the Root Bus. Add a Secondary Bus Reset on each
of the individual root port devices immediately downstream of the VMD
root bus.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/quirks.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f70692a..ee58b51 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3744,6 +3744,49 @@ static int reset_ivb_igd(struct pci_dev *dev, int probe)
 	return 0;
 }
 
+/* Issues SBR to VMD domain to clear PCI configuration */
+static int reset_vmd_sbr(struct pci_dev *dev, int probe)
+{
+	char __iomem *cfgbar, *base;
+	int rp;
+	u16 ctl;
+
+	if (probe)
+		return 0;
+
+	if (dev->dev.driver)
+		return 0;
+
+	cfgbar = pci_iomap(dev, 0, 0);
+	if (!cfgbar)
+		return -ENOMEM;
+
+	/*
+	 * Subdevice config space is mapped linearly using 4k config space
+	 * increments. Use increments of 0x8000 to locate root port devices.
+	 */
+	for (rp = 0; rp < 4; rp++) {
+		base = cfgbar + rp * 0x8000;
+		if (readl(base + PCI_COMMAND) == 0xFFFFFFFF)
+			continue;
+
+		/* pci_reset_secondary_bus() */
+		ctl = readw(base + PCI_BRIDGE_CONTROL);
+		ctl |= PCI_BRIDGE_CTL_BUS_RESET;
+		writew(ctl, base + PCI_BRIDGE_CONTROL);
+		readw(base + PCI_BRIDGE_CONTROL);
+		msleep(2);
+
+		ctl &= ~PCI_BRIDGE_CTL_BUS_RESET;
+		writew(ctl, base + PCI_BRIDGE_CONTROL);
+		readw(base + PCI_BRIDGE_CONTROL);
+	}
+
+	ssleep(1);
+	pci_iounmap(dev, cfgbar);
+	return 0;
+}
+
 /* Device-specific reset method for Chelsio T4-based adapters */
 static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
 {
@@ -3919,6 +3962,11 @@ static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
 		reset_ivb_igd },
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IVB_M2_VGA,
 		reset_ivb_igd },
+	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_201D, reset_vmd_sbr },
+	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0, reset_vmd_sbr },
+	{ PCI_VENDOR_ID_INTEL, 0x467f, reset_vmd_sbr },
+	{ PCI_VENDOR_ID_INTEL, 0x4c3d, reset_vmd_sbr },
+	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B, reset_vmd_sbr },
 	{ PCI_VENDOR_ID_SAMSUNG, 0xa804, nvme_disable_and_flr },
 	{ PCI_VENDOR_ID_INTEL, 0x0953, delay_250ms_after_flr },
 	{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
-- 
1.8.3.1

[PATCH 3/5] PCI: vmd: Add offset translation helper

[Jon Derrick]

Adds a helper for translating physical addresses to bridge window
offsets. No functional changes.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/controller/vmd.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index c7b5614..b0504ee 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -483,6 +483,16 @@ static int vmd_find_free_domain(void)
 	return domain + 1;
 }
 
+static void vmd_phys_to_offset(struct vmd_dev *vmd, u64 phys1, u64 phys2,
+				 resource_size_t *offset1,
+				 resource_size_t *offset2)
+{
+	*offset1 = vmd->dev->resource[VMD_MEMBAR1].start -
+			(phys1 & PCI_BASE_ADDRESS_MEM_MASK);
+	*offset2 = vmd->dev->resource[VMD_MEMBAR2].start -
+			(phys2 & PCI_BASE_ADDRESS_MEM_MASK);
+}
+
 static int vmd_get_phys_offsets(struct vmd_dev *vmd, bool native_hint,
 				resource_size_t *offset1,
 				resource_size_t *offset2)
@@ -507,8 +517,8 @@ static int vmd_get_phys_offsets(struct vmd_dev *vmd, bool native_hint,
 			phys1 = readq(membar2 + MB2_SHADOW_OFFSET);
 			phys2 = readq(membar2 + MB2_SHADOW_OFFSET + 8);
 			pci_iounmap(dev, membar2);
-		} else
-			return 0;
+			vmd_phys_to_offset(vmd, phys1, phys2, offset1, offset2);
+		}
 	} else {
 		/* Hypervisor-Emulated Vendor-Specific Capability */
 		int pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
@@ -525,15 +535,10 @@ static int vmd_get_phys_offsets(struct vmd_dev *vmd, bool native_hint,
 			pci_read_config_dword(dev, pos + 16, &reg);
 			pci_read_config_dword(dev, pos + 20, &regu);
 			phys2 = (u64) regu << 32 | reg;
-		} else
-			return 0;
+			vmd_phys_to_offset(vmd, phys1, phys2, offset1, offset2);
+		}
 	}
 
-	*offset1 = dev->resource[VMD_MEMBAR1].start -
-			(phys1 & PCI_BASE_ADDRESS_MEM_MASK);
-	*offset2 = dev->resource[VMD_MEMBAR2].start -
-			(phys2 & PCI_BASE_ADDRESS_MEM_MASK);
-
 	return 0;
 }
 
-- 
1.8.3.1

[PATCH 4/5] PCI: vmd: Pass features to vmd_get_phys_offsets()

[Jon Derrick]

Modifies the physical address offset parser to use the device-specific
features member. No functional changes.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/controller/vmd.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index b0504ee..71aa002 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -493,14 +493,14 @@ static void vmd_phys_to_offset(struct vmd_dev *vmd, u64 phys1, u64 phys2,
 			(phys2 & PCI_BASE_ADDRESS_MEM_MASK);
 }
 
-static int vmd_get_phys_offsets(struct vmd_dev *vmd, bool native_hint,
+static int vmd_get_phys_offsets(struct vmd_dev *vmd, unsigned long features,
 				resource_size_t *offset1,
 				resource_size_t *offset2)
 {
 	struct pci_dev *dev = vmd->dev;
 	u64 phys1, phys2;
 
-	if (native_hint) {
+	if (features & VMD_FEAT_HAS_MEMBAR_SHADOW) {
 		u32 vmlock;
 		int ret;
 
@@ -519,7 +519,7 @@ static int vmd_get_phys_offsets(struct vmd_dev *vmd, bool native_hint,
 			pci_iounmap(dev, membar2);
 			vmd_phys_to_offset(vmd, phys1, phys2, offset1, offset2);
 		}
-	} else {
+	} else if (features & VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP) {
 		/* Hypervisor-Emulated Vendor-Specific Capability */
 		int pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
 		u32 reg, regu;
@@ -638,16 +638,12 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	 * and child devices. These registers will either return the host value
 	 * or 0, depending on an enable bit in the VMD device.
 	 */
-	if (features & VMD_FEAT_HAS_MEMBAR_SHADOW) {
+	if (features & VMD_FEAT_HAS_MEMBAR_SHADOW)
 		membar2_offset = MB2_SHADOW_OFFSET + MB2_SHADOW_SIZE;
-		ret = vmd_get_phys_offsets(vmd, true, &offset[0], &offset[1]);
-		if (ret)
-			return ret;
-	} else if (features & VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP) {
-		ret = vmd_get_phys_offsets(vmd, false, &offset[0], &offset[1]);
-		if (ret)
-			return ret;
-	}
+
+	ret = vmd_get_phys_offsets(vmd, features, &offset[0], &offset[1]);
+	if (ret)
+		return ret;
 
 	/*
 	 * Certain VMD devices may have a root port configuration option which
-- 
1.8.3.1

[PATCH 5/5] PCI: vmd: Add legacy guest passthrough mode

[Jon Derrick]

Some hypervisors allow passthrough of VMD to guests, but don't supply
the emulated vendor-specific capability. VMD users currently
passing-through VMD rely on a preconfiguration of the VMD Root Ports to
inform the guest of the physical addresses for offset mapping the bridge
windows.

This patch adds a non-visible module parameter to activate host or guest
passthrough mode. In host mode, this patch will write out the VMD MEMBAR
information into the root ports on module unload. Guest mode will use
the direct-assign hints, saving the host-supplied root port information
on VMD module load and restore on exit. It uses this information in the
offset calculation for bridge windows.

This is enabled by non-visible module parameter because it is
non-standard use case for certain users for a legacy behavior.

Link: https://lore.kernel.org/linux-pci/20200706091625.GA26377@e121166-lin.cambridge.arm.com/
Signed-off-by: Sushma Kalakota <sushmax.kalakota@intel.com>
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/controller/vmd.c | 127 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 71aa002..711bbee 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -35,6 +35,19 @@
 #define MB2_SHADOW_OFFSET	0x2000
 #define MB2_SHADOW_SIZE		16
 
+enum legacy_da_mode {
+	VMD_DA_NONE,
+	VMD_DA_HOST,
+	VMD_DA_GUEST,
+};
+
+static int legacy_da_mode;
+static char legacy_da_mode_str[sizeof("guest")];
+module_param_string(legacy_da_mode, legacy_da_mode_str,
+		    sizeof(legacy_da_mode_str), 0);
+MODULE_PARM_DESC(legacy_da_mode,
+	"use legacy host-provided addressing hints in Root Ports to assist guest passthrough (off, host, guest)");
+
 enum vmd_features {
 	/*
 	 * Device may contain registers which hint the physical location of the
@@ -97,6 +110,12 @@ struct vmd_irq_list {
 	unsigned int		count;
 };
 
+struct root_port_addr {
+	int port;
+	u64 membase;
+	u64 pref_membase;
+};
+
 struct vmd_dev {
 	struct pci_dev		*dev;
 
@@ -112,6 +131,7 @@ struct vmd_dev {
 	struct pci_bus		*bus;
 	u8			busn_start;
 	u8			first_vec;
+	struct root_port_addr	rp_addr;
 };
 
 static inline struct vmd_dev *vmd_from_bus(struct pci_bus *bus)
@@ -483,6 +503,97 @@ static int vmd_find_free_domain(void)
 	return domain + 1;
 }
 
+#define VMD_RP_BASE(vmd, port) ((vmd)->cfgbar + (port) * 8 * 4096)
+static void vmd_save_root_port_info(struct vmd_dev *vmd)
+{
+	resource_size_t physical = 0;
+	char __iomem *addr;
+	int port;
+
+	if (upper_32_bits(pci_resource_start(vmd->dev, VMD_MEMBAR1)))
+		return;
+
+	for (port = 0; port < 4; port++) {
+		u32 membase;
+
+		addr = VMD_RP_BASE(vmd, port) + PCI_MEMORY_BASE;
+		membase = readl(addr);
+
+		/* Break on first found root port */
+		if ((membase != 0xffffffff) && (membase != 0) &&
+		    (membase != 0x0000fff0))
+			break;
+	}
+
+	if (port >= 4)
+		return;
+
+	vmd->rp_addr.port = port;
+
+	/* Only save the first root port index in host mode */
+	if (legacy_da_mode == VMD_DA_HOST)
+		return;
+
+	addr = VMD_RP_BASE(vmd, port) + PCI_MEMORY_BASE;
+	physical = ((u64)readw(addr) & 0xfff0) << 16;
+	vmd->rp_addr.membase = physical;
+
+	addr = VMD_RP_BASE(vmd, port) + PCI_PREF_BASE_UPPER32;
+	physical = ((u64)readl(addr)) << 32;
+	vmd->rp_addr.pref_membase = physical;
+
+	addr = VMD_RP_BASE(vmd, port) + PCI_PREF_MEMORY_BASE;
+	physical |= ((u64)readw(addr) & 0xfff0) << 16;
+	vmd->rp_addr.pref_membase |= physical;
+
+	writel(0, VMD_RP_BASE(vmd, port) + PCI_MEMORY_BASE);
+	writel(0, VMD_RP_BASE(vmd, port) + PCI_PREF_BASE_UPPER32);
+	writel(0, VMD_RP_BASE(vmd, port) + PCI_PREF_MEMORY_BASE);
+	writel(0, VMD_RP_BASE(vmd, port) + PCI_PREF_MEMORY_LIMIT);
+}
+
+static void vmd_restore_root_port_info(struct vmd_dev *vmd)
+{
+	resource_size_t	phyaddr;
+	char __iomem *addr;
+	u32 val;
+	int port;
+
+	port = vmd->rp_addr.port;
+	if (legacy_da_mode == VMD_DA_HOST) {
+		/* Write the MEMBAR information to prepare the guest */
+		phyaddr = pci_resource_start(vmd->dev, VMD_MEMBAR1);
+		if (upper_32_bits(phyaddr))
+			return;
+
+		addr = VMD_RP_BASE(vmd, port) + PCI_MEMORY_BASE;
+		val = (phyaddr >> 16) & 0xfff0;
+		writew(val, addr);
+
+		phyaddr = pci_resource_start(vmd->dev, VMD_MEMBAR2);
+		addr = VMD_RP_BASE(vmd, port) + PCI_PREF_BASE_UPPER32;
+		val = phyaddr >> 32;
+		writel(val, addr);
+
+		addr = VMD_RP_BASE(vmd, port) + PCI_PREF_MEMORY_BASE;
+		val = (phyaddr >> 16) & 0xfff0;
+		writew(val, addr);
+	} else if (legacy_da_mode == VMD_DA_GUEST) {
+		/* Restore information provided by Host */
+		addr = VMD_RP_BASE(vmd, port) + PCI_MEMORY_BASE;
+		val = (vmd->rp_addr.membase >> 16) & 0xfff0;
+		writew(val, addr);
+
+		addr = VMD_RP_BASE(vmd, port) + PCI_PREF_BASE_UPPER32;
+		val = vmd->rp_addr.pref_membase >> 32;
+		writel(val, addr);
+
+		addr = VMD_RP_BASE(vmd, port) + PCI_PREF_MEMORY_BASE;
+		val = (vmd->rp_addr.pref_membase >> 16) & 0xfff0;
+		writew(val, addr);
+	}
+}
+
 static void vmd_phys_to_offset(struct vmd_dev *vmd, u64 phys1, u64 phys2,
 				 resource_size_t *offset1,
 				 resource_size_t *offset2)
@@ -500,7 +611,19 @@ static int vmd_get_phys_offsets(struct vmd_dev *vmd, unsigned long features,
 	struct pci_dev *dev = vmd->dev;
 	u64 phys1, phys2;
 
-	if (features & VMD_FEAT_HAS_MEMBAR_SHADOW) {
+	if (!strncmp(legacy_da_mode_str, "host", 4))
+		legacy_da_mode = VMD_DA_HOST;
+	else if (!strncmp(legacy_da_mode_str, "guest", 5))
+		legacy_da_mode = VMD_DA_GUEST;
+
+	if (legacy_da_mode != VMD_DA_NONE) {
+		vmd_save_root_port_info(vmd);
+		if (legacy_da_mode == VMD_DA_GUEST) {
+			vmd_phys_to_offset(vmd, vmd->rp_addr.membase,
+					   vmd->rp_addr.pref_membase,
+					   offset1, offset2);
+		}
+	} else if (features & VMD_FEAT_HAS_MEMBAR_SHADOW) {
 		u32 vmlock;
 		int ret;
 
@@ -732,6 +855,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	if (!vmd->bus) {
 		pci_free_resource_list(&resources);
 		vmd_remove_irq_domain(vmd);
+		vmd_restore_root_port_info(vmd);
 		return -ENODEV;
 	}
 
@@ -821,6 +945,7 @@ static void vmd_remove(struct pci_dev *dev)
 	vmd_cleanup_srcu(vmd);
 	vmd_detach_resources(vmd);
 	vmd_remove_irq_domain(vmd);
+	vmd_restore_root_port_info(vmd);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
1.8.3.1

Re: [PATCH 2/5] PCI: Add a reset quirk for VMD

[Bjorn Helgaas]

[+cc Alex]

On Fri, Nov 20, 2020 at 03:51:41PM -0700, Jon Derrick wrote:
> VMD domains should be reset in-between special attachment such as VFIO
> users. VMD does not offer a reset, however the subdevice domain itself
> can be reset starting at the Root Bus. Add a Secondary Bus Reset on each
> of the individual root port devices immediately downstream of the VMD
> root bus.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/quirks.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index f70692a..ee58b51 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3744,6 +3744,49 @@ static int reset_ivb_igd(struct pci_dev *dev, int probe)
>  	return 0;
>  }
>  
> +/* Issues SBR to VMD domain to clear PCI configuration */
> +static int reset_vmd_sbr(struct pci_dev *dev, int probe)
> +{
> +	char __iomem *cfgbar, *base;
> +	int rp;
> +	u16 ctl;
> +
> +	if (probe)
> +		return 0;
> +
> +	if (dev->dev.driver)
> +		return 0;

I guess "dev" here is the VMD endpoint?  And if the vmd.c driver is
bound to it, you return success without doing anything?

If there's no driver for the VMD device, who is trying to reset it?

I guess I don't quite understand how VMD works.  I would have thought
that if vmd.c isn't bound to the VMD device, the devices behind the
VMD would be inaccessible and there'd be no point in a reset.

> +	cfgbar = pci_iomap(dev, 0, 0);
> +	if (!cfgbar)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Subdevice config space is mapped linearly using 4k config space
> +	 * increments. Use increments of 0x8000 to locate root port devices.
> +	 */
> +	for (rp = 0; rp < 4; rp++) {
> +		base = cfgbar + rp * 0x8000;

I really don't like this part -- iomapping BAR 0 (apparently
VMD_CFGBAR), and making up the ECAM-ish addresses and basically
open-coding ECAM accesses below.  I guess this assumes Root Ports are
only on functions .0, .2, .4, .6?

Is it all open-coded here because this reset path is only of interest
when vmd.c is NOT bound to the the VMD device, so you can't use
vmd->cfgbar, etc?

What about the case when vmd.c IS bound?  We don't do anything here,
so does that mean we instead use the usual case of asserting SBR on
the Root Ports behind the VMD?

> +		if (readl(base + PCI_COMMAND) == 0xFFFFFFFF)
> +			continue;
> +
> +		/* pci_reset_secondary_bus() */
> +		ctl = readw(base + PCI_BRIDGE_CONTROL);
> +		ctl |= PCI_BRIDGE_CTL_BUS_RESET;
> +		writew(ctl, base + PCI_BRIDGE_CONTROL);
> +		readw(base + PCI_BRIDGE_CONTROL);
> +		msleep(2);
> +
> +		ctl &= ~PCI_BRIDGE_CTL_BUS_RESET;
> +		writew(ctl, base + PCI_BRIDGE_CONTROL);
> +		readw(base + PCI_BRIDGE_CONTROL);
> +	}
> +
> +	ssleep(1);
> +	pci_iounmap(dev, cfgbar);
> +	return 0;
> +}
> +
>  /* Device-specific reset method for Chelsio T4-based adapters */
>  static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
>  {
> @@ -3919,6 +3962,11 @@ static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
>  		reset_ivb_igd },
>  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IVB_M2_VGA,
>  		reset_ivb_igd },
> +	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_201D, reset_vmd_sbr },
> +	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0, reset_vmd_sbr },
> +	{ PCI_VENDOR_ID_INTEL, 0x467f, reset_vmd_sbr },
> +	{ PCI_VENDOR_ID_INTEL, 0x4c3d, reset_vmd_sbr },
> +	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B, reset_vmd_sbr },
>  	{ PCI_VENDOR_ID_SAMSUNG, 0xa804, nvme_disable_and_flr },
>  	{ PCI_VENDOR_ID_INTEL, 0x0953, delay_250ms_after_flr },
>  	{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
> -- 
> 1.8.3.1
> 

Re: [PATCH 2/5] PCI: Add a reset quirk for VMD

[Derrick, Jonathan]

Hi Bjorn,

On Tue, 2020-11-24 at 15:40 -0600, Bjorn Helgaas wrote:
> [+cc Alex]
> 
> On Fri, Nov 20, 2020 at 03:51:41PM -0700, Jon Derrick wrote:
> > VMD domains should be reset in-between special attachment such as VFIO
> > users. VMD does not offer a reset, however the subdevice domain itself
> > can be reset starting at the Root Bus. Add a Secondary Bus Reset on each
> > of the individual root port devices immediately downstream of the VMD
> > root bus.
> > 
> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> >  drivers/pci/quirks.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index f70692a..ee58b51 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3744,6 +3744,49 @@ static int reset_ivb_igd(struct pci_dev *dev, int probe)
> >  	return 0;
> >  }
> >  
> > +/* Issues SBR to VMD domain to clear PCI configuration */
> > +static int reset_vmd_sbr(struct pci_dev *dev, int probe)
> > +{
> > +	char __iomem *cfgbar, *base;
> > +	int rp;
> > +	u16 ctl;
> > +
> > +	if (probe)
> > +		return 0;
> > +
> > +	if (dev->dev.driver)
> > +		return 0;
> 
> I guess "dev" here is the VMD endpoint?  And if the vmd.c driver is
> bound to it, you return success without doing anything?
> 
> If there's no driver for the VMD device, who is trying to reset it?
> 
> I guess I don't quite understand how VMD works.  I would have thought
> that if vmd.c isn't bound to the VMD device, the devices behind the
> VMD would be inaccessible and there'd be no point in a reset.

This is basically the idea behind this reset - allow the user to reset
VMD if there is no driver bound to it, but prevent the reset from
deenumerating the domain if there is a driver.

If this is an unusual/unexpected use case, we can drop it.


> 
> > +	cfgbar = pci_iomap(dev, 0, 0);
> > +	if (!cfgbar)
> > +		return -ENOMEM;
> > +
> > +	/*
> > +	 * Subdevice config space is mapped linearly using 4k config space
> > +	 * increments. Use increments of 0x8000 to locate root port devices.
> > +	 */
> > +	for (rp = 0; rp < 4; rp++) {
> > +		base = cfgbar + rp * 0x8000;
> 
> I really don't like this part -- iomapping BAR 0 (apparently
> VMD_CFGBAR), and making up the ECAM-ish addresses and basically
> open-coding ECAM accesses below.  I guess this assumes Root Ports are
> only on functions .0, .2, .4, .6?

The Root Ports are Devices xx:00.0, xx:01.0, xx:02.0, and xx:03.0
(corresponding to PCIE_EXT_SLOT_SHIFT = 15)


> 
> Is it all open-coded here because this reset path is only of interest
> when vmd.c is NOT bound to the the VMD device, so you can't use
> vmd->cfgbar, etc?

That's correct, but as mentioned above it might be an unusual code path
so is not as important as the reset within the driver in patch 1/5.

> 
> What about the case when vmd.c IS bound?  We don't do anything here,
> so does that mean we instead use the usual case of asserting SBR on
> the Root Ports behind the VMD?

It uses the standard Linux reset code paths for Root Port devices

> 
> > +		if (readl(base + PCI_COMMAND) == 0xFFFFFFFF)
> > +			continue;
> > +
> > +		/* pci_reset_secondary_bus() */
> > +		ctl = readw(base + PCI_BRIDGE_CONTROL);
> > +		ctl |= PCI_BRIDGE_CTL_BUS_RESET;
> > +		writew(ctl, base + PCI_BRIDGE_CONTROL);
> > +		readw(base + PCI_BRIDGE_CONTROL);
> > +		msleep(2);
> > +
> > +		ctl &= ~PCI_BRIDGE_CTL_BUS_RESET;
> > +		writew(ctl, base + PCI_BRIDGE_CONTROL);
> > +		readw(base + PCI_BRIDGE_CONTROL);
> > +	}
> > +
> > +	ssleep(1);
> > +	pci_iounmap(dev, cfgbar);
> > +	return 0;
> > +}
> > +
> >  /* Device-specific reset method for Chelsio T4-based adapters */
> >  static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
> >  {
> > @@ -3919,6 +3962,11 @@ static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
> >  		reset_ivb_igd },
> >  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IVB_M2_VGA,
> >  		reset_ivb_igd },
> > +	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_201D, reset_vmd_sbr },
> > +	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0, reset_vmd_sbr },
> > +	{ PCI_VENDOR_ID_INTEL, 0x467f, reset_vmd_sbr },
> > +	{ PCI_VENDOR_ID_INTEL, 0x4c3d, reset_vmd_sbr },
> > +	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B, reset_vmd_sbr },
> >  	{ PCI_VENDOR_ID_SAMSUNG, 0xa804, nvme_disable_and_flr },
> >  	{ PCI_VENDOR_ID_INTEL, 0x0953, delay_250ms_after_flr },
> >  	{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
> > -- 
> > 1.8.3.1
> > 

Re: [PATCH 2/5] PCI: Add a reset quirk for VMD

[Alex Williamson]

On Wed, 25 Nov 2020 17:22:05 +0000
"Derrick, Jonathan" <jonathan.derrick@intel.com> wrote:

> Hi Bjorn,
> 
> On Tue, 2020-11-24 at 15:40 -0600, Bjorn Helgaas wrote:
> > [+cc Alex]
> > 
> > On Fri, Nov 20, 2020 at 03:51:41PM -0700, Jon Derrick wrote:  
> > > VMD domains should be reset in-between special attachment such as VFIO
> > > users. VMD does not offer a reset, however the subdevice domain itself
> > > can be reset starting at the Root Bus. Add a Secondary Bus Reset on each
> > > of the individual root port devices immediately downstream of the VMD
> > > root bus.
> > > 
> > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > > ---
> > >  drivers/pci/quirks.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 48 insertions(+)
> > > 
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index f70692a..ee58b51 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -3744,6 +3744,49 @@ static int reset_ivb_igd(struct pci_dev *dev, int probe)
> > >  	return 0;
> > >  }
> > >  
> > > +/* Issues SBR to VMD domain to clear PCI configuration */
> > > +static int reset_vmd_sbr(struct pci_dev *dev, int probe)
> > > +{
> > > +	char __iomem *cfgbar, *base;
> > > +	int rp;
> > > +	u16 ctl;
> > > +
> > > +	if (probe)
> > > +		return 0;
> > > +
> > > +	if (dev->dev.driver)
> > > +		return 0;  
> > 
> > I guess "dev" here is the VMD endpoint?  And if the vmd.c driver is
> > bound to it, you return success without doing anything?
> > 
> > If there's no driver for the VMD device, who is trying to reset it?
> > 
> > I guess I don't quite understand how VMD works.  I would have thought
> > that if vmd.c isn't bound to the VMD device, the devices behind the
> > VMD would be inaccessible and there'd be no point in a reset.  
> 
> This is basically the idea behind this reset - allow the user to reset
> VMD if there is no driver bound to it, but prevent the reset from
> deenumerating the domain if there is a driver.
> 
> If this is an unusual/unexpected use case, we can drop it.

I don't understand how this improves the vfio use case as claimed in
the commit log, are you expecting the device to be unbound from all
drivers and reset via pci-sysfs between uses?  vfio would not be able
to perform the reset itself with this behavior, including between
resets of a VM or between separate users without external manual
unbinding and reset.

   
> > > +	cfgbar = pci_iomap(dev, 0, 0);
> > > +	if (!cfgbar)
> > > +		return -ENOMEM;
> > > +
> > > +	/*
> > > +	 * Subdevice config space is mapped linearly using 4k config space
> > > +	 * increments. Use increments of 0x8000 to locate root port devices.
> > > +	 */
> > > +	for (rp = 0; rp < 4; rp++) {
> > > +		base = cfgbar + rp * 0x8000;  
> > 
> > I really don't like this part -- iomapping BAR 0 (apparently
> > VMD_CFGBAR), and making up the ECAM-ish addresses and basically
> > open-coding ECAM accesses below.  I guess this assumes Root Ports are
> > only on functions .0, .2, .4, .6?  
> 
> The Root Ports are Devices xx:00.0, xx:01.0, xx:02.0, and xx:03.0
> (corresponding to PCIE_EXT_SLOT_SHIFT = 15)
> 
> 
> > 
> > Is it all open-coded here because this reset path is only of interest
> > when vmd.c is NOT bound to the the VMD device, so you can't use
> > vmd->cfgbar, etc?  
> 
> That's correct, but as mentioned above it might be an unusual code path
> so is not as important as the reset within the driver in patch 1/5.
> 
> > 
> > What about the case when vmd.c IS bound?  We don't do anything here,
> > so does that mean we instead use the usual case of asserting SBR on
> > the Root Ports behind the VMD?  
> 
> It uses the standard Linux reset code paths for Root Port devices
> 
> >   
> > > +		if (readl(base + PCI_COMMAND) == 0xFFFFFFFF)
> > > +			continue;
> > > +
> > > +		/* pci_reset_secondary_bus() */
> > > +		ctl = readw(base + PCI_BRIDGE_CONTROL);
> > > +		ctl |= PCI_BRIDGE_CTL_BUS_RESET;
> > > +		writew(ctl, base + PCI_BRIDGE_CONTROL);
> > > +		readw(base + PCI_BRIDGE_CONTROL);
> > > +		msleep(2);
> > > +
> > > +		ctl &= ~PCI_BRIDGE_CTL_BUS_RESET;
> > > +		writew(ctl, base + PCI_BRIDGE_CONTROL);
> > > +		readw(base + PCI_BRIDGE_CONTROL);

We're performing an SBR of the internal root ports here, is the config
space of the affected endpoints handled via save+restore of the code
that calls this?  I'm a little rusty on VMD again.  Thanks,

Alex


> > > +	}
> > > +
> > > +	ssleep(1);
> > > +	pci_iounmap(dev, cfgbar);
> > > +	return 0;
> > > +}
> > > +
> > >  /* Device-specific reset method for Chelsio T4-based adapters */
> > >  static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
> > >  {
> > > @@ -3919,6 +3962,11 @@ static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
> > >  		reset_ivb_igd },
> > >  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IVB_M2_VGA,
> > >  		reset_ivb_igd },
> > > +	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_201D, reset_vmd_sbr },
> > > +	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0, reset_vmd_sbr },
> > > +	{ PCI_VENDOR_ID_INTEL, 0x467f, reset_vmd_sbr },
> > > +	{ PCI_VENDOR_ID_INTEL, 0x4c3d, reset_vmd_sbr },
> > > +	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B, reset_vmd_sbr },
> > >  	{ PCI_VENDOR_ID_SAMSUNG, 0xa804, nvme_disable_and_flr },
> > >  	{ PCI_VENDOR_ID_INTEL, 0x0953, delay_250ms_after_flr },
> > >  	{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
> > > -- 
> > > 1.8.3.1
> > >   

Re: [PATCH 0/5] Legacy direct-assign mode

[Lorenzo Pieralisi]

On Fri, Nov 20, 2020 at 03:51:39PM -0700, Jon Derrick wrote:
> This set adds a legacy direct-assign mode. Newer enterprise hardware has
> physical addressing hints to assist device passthrough to guests that needs to
> correctly program bridge windows with physical addresses. Some customers are
> using a legacy method that relies on the VMD subdevice domain's root port
> windows to be written with the physical addresses. This method also allows
> other hypervisors besides QEMU/KVM to perform guest passthrough.
> 
> This patchset adds a host and guest mode to write the physical address
> information to the root port registers in the host and read them in the guest,
> and restore them in both cases on module unload.
> 
> This patchset also folds in the VMD subdevice domain secondary bus reset
> patchset [1] to clear the domain prior to guest passthrough.
> 
> [1] https://patchwork.kernel.org/project/linux-pci/cover/20200928010557.5324-1-jonathan.derrick@intel.com/
> 
> Jon Derrick (5):
>   PCI: vmd: Reset the VMD subdevice domain on probe
>   PCI: Add a reset quirk for VMD
>   PCI: vmd: Add offset translation helper
>   PCI: vmd: Pass features to vmd_get_phys_offsets()
>   PCI: vmd: Add legacy guest passthrough mode
> 
>  drivers/pci/controller/vmd.c | 200 ++++++++++++++++++++++++++++++++++++++-----
>  drivers/pci/quirks.c         |  48 +++++++++++
>  2 files changed, 227 insertions(+), 21 deletions(-)

Hi Jon,

it is unclear to me where we are with this series, please let me know.

Thanks,
Lorenzo

Re: [PATCH 0/5] Legacy direct-assign mode

[Derrick, Jonathan]

On Mon, 2021-03-22 at 12:28 +0000, Lorenzo Pieralisi wrote:
> On Fri, Nov 20, 2020 at 03:51:39PM -0700, Jon Derrick wrote:
> > This set adds a legacy direct-assign mode. Newer enterprise hardware has
> > physical addressing hints to assist device passthrough to guests that needs to
> > correctly program bridge windows with physical addresses. Some customers are
> > using a legacy method that relies on the VMD subdevice domain's root port
> > windows to be written with the physical addresses. This method also allows
> > other hypervisors besides QEMU/KVM to perform guest passthrough.
> > 
> > This patchset adds a host and guest mode to write the physical address
> > information to the root port registers in the host and read them in the guest,
> > and restore them in both cases on module unload.
> > 
> > This patchset also folds in the VMD subdevice domain secondary bus reset
> > patchset [1] to clear the domain prior to guest passthrough.
> > 
> > [1] https://patchwork.kernel.org/project/linux-pci/cover/20200928010557.5324-1-jonathan.derrick@intel.com/
> > 
> > Jon Derrick (5):
> >   PCI: vmd: Reset the VMD subdevice domain on probe
> >   PCI: Add a reset quirk for VMD
> >   PCI: vmd: Add offset translation helper
> >   PCI: vmd: Pass features to vmd_get_phys_offsets()
> >   PCI: vmd: Add legacy guest passthrough mode
> > 
> >  drivers/pci/controller/vmd.c | 200 ++++++++++++++++++++++++++++++++++++++-----
> >  drivers/pci/quirks.c         |  48 +++++++++++
> >  2 files changed, 227 insertions(+), 21 deletions(-)
> 
> Hi Jon,
> 
> it is unclear to me where we are with this series, please let me know.
> 
> Thanks,
> Lorenzo

Hi Lorenzo,

We can drop this series for now. We may revive it in the future with
modifications to the reset sequence.

Best,
Jon

Re: [PATCH 0/5] Legacy direct-assign mode

[Christoph Hellwig]

On Mon, Mar 22, 2021 at 12:28:37PM +0000, Lorenzo Pieralisi wrote:
> > correctly program bridge windows with physical addresses. Some customers are
> > using a legacy method that relies on the VMD subdevice domain's root port
> > windows to be written with the physical addresses. This method also allows
> > other hypervisors besides QEMU/KVM to perform guest passthrough.

This seems like a bad idea.  What are these other hypervisors?  AFAIK
there are no purely userspace hypervisors, so in other words what you
propose here is only for unsupported external modules.

I don't think we shoud merge something like this.

Re: [PATCH 0/5] Legacy direct-assign mode

[Derrick, Jonathan]

On Mon, 2021-03-22 at 19:48 +0000, Christoph Hellwig wrote:
> On Mon, Mar 22, 2021 at 12:28:37PM +0000, Lorenzo Pieralisi wrote:
> > > correctly program bridge windows with physical addresses. Some customers are
> > > using a legacy method that relies on the VMD subdevice domain's root port
> > > windows to be written with the physical addresses. This method also allows
> > > other hypervisors besides QEMU/KVM to perform guest passthrough.
> 
> This seems like a bad idea.  What are these other hypervisors?  AFAIK
> there are no purely userspace hypervisors, so in other words what you
> propose here is only for unsupported external modules.
Any of the type 1 hypervisors here:
https://en.wikipedia.org/wiki/Hypervisor

> 
> I don't think we shoud merge something like this.