[PATCH v2 0/2] Issue secondary bus reset and domain window reset

[PATCH v2 0/2] Issue secondary bus reset and domain window reset

[Nirmal Patel]

During VT-d pass-through, the VMD driver occasionally fails to
enumerate underlying NVMe devices when repetitive reboots are
performed in the guest OS. The issue seems to be resolved by
performing secondary bus resets and reinitializing the root
port's bridge windows.

Nirmal Patel (2):
  PCI: vmd: Trigger secondary bus reset
  PCI: vmd: Issue vmd domain window reset

 drivers/pci/controller/vmd.c | 81 ++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

-- 
2.27.0

[PATCH v2 1/2] PCI: vmd: Trigger secondary bus reset

[Nirmal Patel]

During VT-d passthrough repetitive reboot tests, it was determined that the VMD
domain needed to be reset in order to allow downstream devices to reinitialize
properly. This is done using a secondary bus reset at each of the VMD root
ports and any bridges in the domain.

Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/controller/vmd.c | 46 ++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index e3fcdfec58b3..6e1c60048774 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -15,6 +15,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>
@@ -447,6 +448,49 @@ static struct pci_ops vmd_ops = {
 	.write		= vmd_pci_write,
 };
 
+#define PCI_HEADER_TYPE_MASK 0x7f
+#define PCI_CLASS_BRIDGE_PCI 0x0604
+#define DEVICE_SPACE (8 * 4096)
+#define VMD_DEVICE_BASE(vmd, device) ((vmd)->cfgbar + (device) * DEVICE_SPACE)
+#define VMD_FUNCTION_BASE(vmd, device, fn) ((vmd)->cfgbar + (device) * (DEVICE_SPACE + (fn*4096)))
+static void vmd_domain_sbr(struct vmd_dev *vmd)
+{
+	char __iomem *base;
+	u16 ctl;
+	int dev_seq;
+	int max_devs = resource_size(&vmd->resources[0]) * 32;
+
+	/*
+	* Subdevice config space may or many not be mapped linearly using 4k config
+	* space.
+	*/
+	for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
+		base = VMD_DEVICE_BASE(vmd, dev_seq);
+		if (readw(base + PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL)
+			continue;
+
+		if ((readb(base + PCI_HEADER_TYPE) & PCI_HEADER_TYPE_MASK) !=
+		    PCI_HEADER_TYPE_BRIDGE)
+			continue;
+
+		if (readw(base + PCI_CLASS_DEVICE) != PCI_CLASS_BRIDGE_PCI)
+			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];
@@ -747,6 +791,8 @@ 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_sbr(vmd);
+
 	pci_scan_child_bus(vmd->bus);
 	pci_assign_unassigned_bus_resources(vmd->bus);
 
-- 
2.27.0

[PATCH v2 2/2] PCI: vmd: Issue vmd domain window reset

[Nirmal Patel]

In order to properly re-initialize the VMD domain during repetitive driver
attachment or reboot tests, ensure that the VMD root ports are re-initialized
to a blank state that can be re-enumerated appropriately by the PCI core.
This is performed by re-initializing all of the bridge windows to ensure
that PCI core enumeration does not detect potentially invalid bridge windows
and misinterpret them as firmware-assigned windows, when they simply may be
invalid bridge window information from a previous boot.

Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/controller/vmd.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 6e1c60048774..e52bdb95218e 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -651,6 +651,39 @@ static int vmd_alloc_irqs(struct vmd_dev *vmd)
 	return 0;
 }
 
+
+static void vmd_domain_reset_windows(struct vmd_dev *vmd)
+{
+	u8 hdr_type;
+	char __iomem *addr;
+	int dev_seq;
+	u8 functions;
+	u8 fn_seq;
+	int max_devs = resource_size(&vmd->resources[0]) * 32;
+
+	for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
+		addr = VMD_DEVICE_BASE(vmd, dev_seq) + PCI_VENDOR_ID;
+		if (readw(addr) != PCI_VENDOR_ID_INTEL)
+			continue;
+
+		addr = VMD_DEVICE_BASE(vmd, dev_seq) + PCI_HEADER_TYPE;
+		hdr_type = readb(addr) & PCI_HEADER_TYPE_MASK;
+		if (hdr_type != PCI_HEADER_TYPE_BRIDGE)
+			continue;
+
+		functions = !!(hdr_type & 0x80) ? 8 : 1;
+		for (fn_seq = 0; fn_seq < functions; fn_seq++)
+		{
+			addr = VMD_FUNCTION_BASE(vmd, dev_seq, fn_seq) + PCI_VENDOR_ID;
+			if (readw(addr) != PCI_VENDOR_ID_INTEL)
+				continue;
+
+			memset_io((VMD_FUNCTION_BASE(vmd, dev_seq, fn_seq) + PCI_IO_BASE),
+			 0, PCI_ROM_ADDRESS1 - PCI_IO_BASE);
+		}
+	}
+}
+
 static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
@@ -741,6 +774,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 		.parent = res,
 	};
 
+	vmd_domain_reset_windows(vmd);
+
 	sd->vmd_dev = vmd->dev;
 	sd->domain = vmd_find_free_domain();
 	if (sd->domain < 0)
-- 
2.27.0

Re: [PATCH v2 0/2] Issue secondary bus reset and domain window reset

[Patel, Nirmal]

On 7/20/2021 1:50 PM, Nirmal Patel wrote:
> During VT-d pass-through, the VMD driver occasionally fails to
> enumerate underlying NVMe devices when repetitive reboots are
> performed in the guest OS. The issue seems to be resolved by
> performing secondary bus resets and reinitializing the root
> port's bridge windows.
>
> Nirmal Patel (2):
>   PCI: vmd: Trigger secondary bus reset
>   PCI: vmd: Issue vmd domain window reset
>
>  drivers/pci/controller/vmd.c | 81 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>
There is no v1 of this patch. I made a newbie mistake and didn't remove v2 after internal review. Please let me know if you have alternate suggestion to fix this issue.

Re: [PATCH v2 1/2] PCI: vmd: Trigger secondary bus reset

[Bjorn Helgaas]

On Tue, Jul 20, 2021 at 01:50:08PM -0700, Nirmal Patel wrote:
> During VT-d passthrough repetitive reboot tests, it was determined that the VMD
> domain needed to be reset in order to allow downstream devices to reinitialize
> properly. This is done using a secondary bus reset at each of the VMD root
> ports and any bridges in the domain.

I don't understand the "any bridges in the domain" part.  Clearly you
only reset Intel bridges, and only those on a single "bus".  So can
there be both VMD root ports and another kind of bridge on the same
bus?

Rewrap this to fit in 75 columns or so, so it doesn't overflow 80
column lines when git log indents it by 4.

> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/controller/vmd.c | 46 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index e3fcdfec58b3..6e1c60048774 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -15,6 +15,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>
> @@ -447,6 +448,49 @@ static struct pci_ops vmd_ops = {
>  	.write		= vmd_pci_write,
>  };
>  
> +#define PCI_HEADER_TYPE_MASK 0x7f

Already defined in include/uapi/linux/pci_regs.h.

> +#define PCI_CLASS_BRIDGE_PCI 0x0604

Already defined in include/linux/pci_ids.h.  What am I missing?  Seems
like you should see duplicate definition warnings.

> +#define DEVICE_SPACE (8 * 4096)
> +#define VMD_DEVICE_BASE(vmd, device) ((vmd)->cfgbar + (device) * DEVICE_SPACE)
> +#define VMD_FUNCTION_BASE(vmd, device, fn) ((vmd)->cfgbar + (device) * (DEVICE_SPACE + (fn*4096)))

Add blank line here.

> +static void vmd_domain_sbr(struct vmd_dev *vmd)
> +{
> +	char __iomem *base;
> +	u16 ctl;
> +	int dev_seq;
> +	int max_devs = resource_size(&vmd->resources[0]) * 32;
> +
> +	/*
> +	* Subdevice config space may or many not be mapped linearly using 4k config
> +	* space.

Fix comment indentation.

s/many/may/

I don't really understand the comment anyway.  Is the point that
subdevice config space may not be physically contiguous?  Certainly
VMD_DEVICE_BASE() computes virtual addresses that are linear.

> +	*/
> +	for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
> +		base = VMD_DEVICE_BASE(vmd, dev_seq);
> +		if (readw(base + PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL)
> +			continue;
> +
> +		if ((readb(base + PCI_HEADER_TYPE) & PCI_HEADER_TYPE_MASK) !=
> +		    PCI_HEADER_TYPE_BRIDGE)
> +			continue;
> +
> +		if (readw(base + PCI_CLASS_DEVICE) != PCI_CLASS_BRIDGE_PCI)
> +			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);

It's a shame we can't do this with pci_reset_secondary_bus().  Makes
me wonder if there's an opportunity for special pci_ops.

> +		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];
> @@ -747,6 +791,8 @@ 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_sbr(vmd);
> +
>  	pci_scan_child_bus(vmd->bus);
>  	pci_assign_unassigned_bus_resources(vmd->bus);
>  
> -- 
> 2.27.0
> 

Re: [PATCH v2 2/2] PCI: vmd: Issue vmd domain window reset

[Bjorn Helgaas]

On Tue, Jul 20, 2021 at 01:50:09PM -0700, Nirmal Patel wrote:
> In order to properly re-initialize the VMD domain during repetitive driver
> attachment or reboot tests, ensure that the VMD root ports are re-initialized
> to a blank state that can be re-enumerated appropriately by the PCI core.
> This is performed by re-initializing all of the bridge windows to ensure
> that PCI core enumeration does not detect potentially invalid bridge windows
> and misinterpret them as firmware-assigned windows, when they simply may be
> invalid bridge window information from a previous boot.

Rewrap commit log to fit in 75 columns.  No problem about v2 vs v1.

> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/controller/vmd.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 6e1c60048774..e52bdb95218e 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -651,6 +651,39 @@ static int vmd_alloc_irqs(struct vmd_dev *vmd)
>  	return 0;
>  }
>  
> +

Remove spurious blank line here.

> +static void vmd_domain_reset_windows(struct vmd_dev *vmd)
> +{
> +	u8 hdr_type;
> +	char __iomem *addr;
> +	int dev_seq;
> +	u8 functions;
> +	u8 fn_seq;
> +	int max_devs = resource_size(&vmd->resources[0]) * 32;
> +
> +	for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
> +		addr = VMD_DEVICE_BASE(vmd, dev_seq) + PCI_VENDOR_ID;
> +		if (readw(addr) != PCI_VENDOR_ID_INTEL)
> +			continue;
> +
> +		addr = VMD_DEVICE_BASE(vmd, dev_seq) + PCI_HEADER_TYPE;
> +		hdr_type = readb(addr) & PCI_HEADER_TYPE_MASK;
> +		if (hdr_type != PCI_HEADER_TYPE_BRIDGE)
> +			continue;
> +
> +		functions = !!(hdr_type & 0x80) ? 8 : 1;
> +		for (fn_seq = 0; fn_seq < functions; fn_seq++)
> +		{

Put "{" on previous line.

Looks quite parallel to vmd_domain_sbr(), except that here we iterate
through functions as well.  Why does vmd_domain_sbr() not need to
iterate through functions?

> +			addr = VMD_FUNCTION_BASE(vmd, dev_seq, fn_seq) + PCI_VENDOR_ID;
> +			if (readw(addr) != PCI_VENDOR_ID_INTEL)
> +				continue;
> +
> +			memset_io((VMD_FUNCTION_BASE(vmd, dev_seq, fn_seq) + PCI_IO_BASE),
> +			 0, PCI_ROM_ADDRESS1 - PCI_IO_BASE);

Make the lines above fit in 80 columns.

> +		}
> +	}
> +}
> +
>  static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  {
>  	struct pci_sysdata *sd = &vmd->sysdata;
> @@ -741,6 +774,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  		.parent = res,
>  	};
>  
> +	vmd_domain_reset_windows(vmd);
> +
>  	sd->vmd_dev = vmd->dev;
>  	sd->domain = vmd_find_free_domain();
>  	if (sd->domain < 0)
> -- 
> 2.27.0
> 

Re: [PATCH v2 1/2] PCI: vmd: Trigger secondary bus reset

[Christoph Hellwig]

On Tue, Jul 20, 2021 at 01:50:08PM -0700, Nirmal Patel wrote:
> +#define PCI_HEADER_TYPE_MASK 0x7f
> +#define PCI_CLASS_BRIDGE_PCI 0x0604

Please use the existing definitions from pci_regs.h / pci_ids.h.

>
> +#define DEVICE_SPACE (8 * 4096)
> +#define VMD_DEVICE_BASE(vmd, device) ((vmd)->cfgbar + (device) * DEVICE_SPACE)
> +#define VMD_FUNCTION_BASE(vmd, device, fn) ((vmd)->cfgbar + (device) * (DEVICE_SPACE + (fn*4096)))

Plase turn thos into readable inline functions and avoid the overly long
lines.

> +	/*
> +	* Subdevice config space may or many not be mapped linearly using 4k config
> +	* space.
> +	*/

Please avoid the overly long line, especially in a comment.

Re: [PATCH v2 1/2] PCI: vmd: Trigger secondary bus reset

[Pali Rohár]

On Tuesday 20 July 2021 13:50:08 Nirmal Patel wrote:
> During VT-d passthrough repetitive reboot tests, it was determined that the VMD
> domain needed to be reset in order to allow downstream devices to reinitialize
> properly. This is done using a secondary bus reset at each of the VMD root
> ports and any bridges in the domain.
> 
> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/controller/vmd.c | 46 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index e3fcdfec58b3..6e1c60048774 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -15,6 +15,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>
> @@ -447,6 +448,49 @@ static struct pci_ops vmd_ops = {
>  	.write		= vmd_pci_write,
>  };
>  
> +#define PCI_HEADER_TYPE_MASK 0x7f
> +#define PCI_CLASS_BRIDGE_PCI 0x0604
> +#define DEVICE_SPACE (8 * 4096)
> +#define VMD_DEVICE_BASE(vmd, device) ((vmd)->cfgbar + (device) * DEVICE_SPACE)
> +#define VMD_FUNCTION_BASE(vmd, device, fn) ((vmd)->cfgbar + (device) * (DEVICE_SPACE + (fn*4096)))
> +static void vmd_domain_sbr(struct vmd_dev *vmd)
> +{
> +	char __iomem *base;
> +	u16 ctl;
> +	int dev_seq;
> +	int max_devs = resource_size(&vmd->resources[0]) * 32;
> +
> +	/*
> +	* Subdevice config space may or many not be mapped linearly using 4k config
> +	* space.
> +	*/
> +	for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
> +		base = VMD_DEVICE_BASE(vmd, dev_seq);
> +		if (readw(base + PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL)
> +			continue;
> +
> +		if ((readb(base + PCI_HEADER_TYPE) & PCI_HEADER_TYPE_MASK) !=
> +		    PCI_HEADER_TYPE_BRIDGE)
> +			continue;
> +
> +		if (readw(base + PCI_CLASS_DEVICE) != PCI_CLASS_BRIDGE_PCI)
> +			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);

Hello!

You cannot unconditionally call secondary bus reset for arbitrary PCIe
Bridge. Calling it breaks more PCIe devices behind bridge and
pci_reset_secondary_bus() already handles it and skip reset if reset is
causing issues.

I would suggest to use pci_reset_secondary_bus() and extend it
so you can call it also from your driver.

> +	}
> +	ssleep(1);
> +}
> +
>  static void vmd_attach_resources(struct vmd_dev *vmd)
>  {
>  	vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1];
> @@ -747,6 +791,8 @@ 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_sbr(vmd);
> +
>  	pci_scan_child_bus(vmd->bus);
>  	pci_assign_unassigned_bus_resources(vmd->bus);
>  
> -- 
> 2.27.0
> 

Re: [PATCH v2 1/2] PCI: vmd: Trigger secondary bus reset

[Patel, Nirmal]

On 7/20/2021 3:33 PM, Bjorn Helgaas wrote:
> On Tue, Jul 20, 2021 at 01:50:08PM -0700, Nirmal Patel wrote:
>> During VT-d passthrough repetitive reboot tests, it was determined that the VMD
>> domain needed to be reset in order to allow downstream devices to reinitialize
>> properly. This is done using a secondary bus reset at each of the VMD root
>> ports and any bridges in the domain.
> I don't understand the "any bridges in the domain" part.  Clearly you
> only reset Intel bridges, and only those on a single "bus".  So can
> there be both VMD root ports and another kind of bridge on the same
> bus?
>
> Rewrap this to fit in 75 columns or so, so it doesn't overflow 80
> column lines when git log indents it by 4.

You are right on resetting only Intel bridges. I think I can reword the message
like "This is done using setting secondary bus reset bit of each of the VMD
root port and will propagate reset through downstream bridges."

>
>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
>> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
>> ---
>>  drivers/pci/controller/vmd.c | 46 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index e3fcdfec58b3..6e1c60048774 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -15,6 +15,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>
>> @@ -447,6 +448,49 @@ static struct pci_ops vmd_ops = {
>>  	.write		= vmd_pci_write,
>>  };
>>  
>> +#define PCI_HEADER_TYPE_MASK 0x7f
> Already defined in include/uapi/linux/pci_regs.h.

I will make the changes.

>
>> +#define PCI_CLASS_BRIDGE_PCI 0x0604
> Already defined in include/linux/pci_ids.h.  What am I missing?  Seems
> like you should see duplicate definition warnings.

I will make the changes.

>
>> +#define DEVICE_SPACE (8 * 4096)
>> +#define VMD_DEVICE_BASE(vmd, device) ((vmd)->cfgbar + (device) * DEVICE_SPACE)
>> +#define VMD_FUNCTION_BASE(vmd, device, fn) ((vmd)->cfgbar + (device) * (DEVICE_SPACE + (fn*4096)))
> Add blank line here.
Sure.
>
>> +static void vmd_domain_sbr(struct vmd_dev *vmd)
>> +{
>> +	char __iomem *base;
>> +	u16 ctl;
>> +	int dev_seq;
>> +	int max_devs = resource_size(&vmd->resources[0]) * 32;
>> +
>> +	/*
>> +	* Subdevice config space may or many not be mapped linearly using 4k config
>> +	* space.
> Fix comment indentation.
>
> s/many/may/
>
> I don't really understand the comment anyway.  Is the point that
> subdevice config space may not be physically contiguous?  Certainly
> VMD_DEVICE_BASE() computes virtual addresses that are linear.

I will remove this confusing comment for the sack of simplicity.

>
>> +	*/
>> +	for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
>> +		base = VMD_DEVICE_BASE(vmd, dev_seq);
>> +		if (readw(base + PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL)
>> +			continue;
>> +
>> +		if ((readb(base + PCI_HEADER_TYPE) & PCI_HEADER_TYPE_MASK) !=
>> +		    PCI_HEADER_TYPE_BRIDGE)
>> +			continue;
>> +
>> +		if (readw(base + PCI_CLASS_DEVICE) != PCI_CLASS_BRIDGE_PCI)
>> +			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);
> It's a shame we can't do this with pci_reset_secondary_bus().  Makes
> me wonder if there's an opportunity for special pci_ops.
>
>> +		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];
>> @@ -747,6 +791,8 @@ 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_sbr(vmd);
>> +
>>  	pci_scan_child_bus(vmd->bus);
>>  	pci_assign_unassigned_bus_resources(vmd->bus);
>>  
>> -- 
>> 2.27.0
>>

Re: [PATCH v2 1/2] PCI: vmd: Trigger secondary bus reset

[Patel, Nirmal]

On 7/21/2021 1:50 AM, Pali Rohár wrote:
> On Tuesday 20 July 2021 13:50:08 Nirmal Patel wrote:
>> During VT-d passthrough repetitive reboot tests, it was determined that the VMD
>> domain needed to be reset in order to allow downstream devices to reinitialize
>> properly. This is done using a secondary bus reset at each of the VMD root
>> ports and any bridges in the domain.
>>
>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
>> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
>> ---
>>  drivers/pci/controller/vmd.c | 46 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index e3fcdfec58b3..6e1c60048774 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -15,6 +15,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>
>> @@ -447,6 +448,49 @@ static struct pci_ops vmd_ops = {
>>  	.write		= vmd_pci_write,
>>  };
>>  
>> +#define PCI_HEADER_TYPE_MASK 0x7f
>> +#define PCI_CLASS_BRIDGE_PCI 0x0604
>> +#define DEVICE_SPACE (8 * 4096)
>> +#define VMD_DEVICE_BASE(vmd, device) ((vmd)->cfgbar + (device) * DEVICE_SPACE)
>> +#define VMD_FUNCTION_BASE(vmd, device, fn) ((vmd)->cfgbar + (device) * (DEVICE_SPACE + (fn*4096)))
>> +static void vmd_domain_sbr(struct vmd_dev *vmd)
>> +{
>> +	char __iomem *base;
>> +	u16 ctl;
>> +	int dev_seq;
>> +	int max_devs = resource_size(&vmd->resources[0]) * 32;
>> +
>> +	/*
>> +	* Subdevice config space may or many not be mapped linearly using 4k config
>> +	* space.
>> +	*/
>> +	for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
>> +		base = VMD_DEVICE_BASE(vmd, dev_seq);
>> +		if (readw(base + PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL)
>> +			continue;
>> +
>> +		if ((readb(base + PCI_HEADER_TYPE) & PCI_HEADER_TYPE_MASK) !=
>> +		    PCI_HEADER_TYPE_BRIDGE)
>> +			continue;
>> +
>> +		if (readw(base + PCI_CLASS_DEVICE) != PCI_CLASS_BRIDGE_PCI)
>> +			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);
> Hello!
>
> You cannot unconditionally call secondary bus reset for arbitrary PCIe
> Bridge. Calling it breaks more PCIe devices behind bridge and
> pci_reset_secondary_bus() already handles it and skip reset if reset is
> causing issues.
>
> I would suggest to use pci_reset_secondary_bus() and extend it
> so you can call it also from your driver.
Secondary bus reset is only performed on Intel VMD root ports prior to the
VMD domain PCI enumerations.
>
>> +	}
>> +	ssleep(1);
>> +}
>> +
>>  static void vmd_attach_resources(struct vmd_dev *vmd)
>>  {
>>  	vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1];
>> @@ -747,6 +791,8 @@ 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_sbr(vmd);
>> +
>>  	pci_scan_child_bus(vmd->bus);
>>  	pci_assign_unassigned_bus_resources(vmd->bus);
>>  
>> -- 
>> 2.27.0
>>

Re: [PATCH v2 1/2] PCI: vmd: Trigger secondary bus reset

[Patel, Nirmal]

On 7/20/2021 10:45 PM, Christoph Hellwig wrote:
> On Tue, Jul 20, 2021 at 01:50:08PM -0700, Nirmal Patel wrote:
>> +#define PCI_HEADER_TYPE_MASK 0x7f
>> +#define PCI_CLASS_BRIDGE_PCI 0x0604
> Please use the existing definitions from pci_regs.h / pci_ids.h.
Sure.
>
>> +#define DEVICE_SPACE (8 * 4096)
>> +#define VMD_DEVICE_BASE(vmd, device) ((vmd)->cfgbar + (device) * DEVICE_SPACE)
>> +#define VMD_FUNCTION_BASE(vmd, device, fn) ((vmd)->cfgbar + (device) * (DEVICE_SPACE + (fn*4096)))
> Plase turn thos into readable inline functions and avoid the overly long
> lines.
Sure.
>
>> +	/*
>> +	* Subdevice config space may or many not be mapped linearly using 4k config
>> +	* space.
>> +	*/
> Please avoid the overly long line, especially in a comment.
It is better to remove this confusing comment.

Re: [PATCH v2 2/2] PCI: vmd: Issue vmd domain window reset

[Patel, Nirmal]

On 7/20/2021 3:42 PM, Bjorn Helgaas wrote:
> On Tue, Jul 20, 2021 at 01:50:09PM -0700, Nirmal Patel wrote:
>> In order to properly re-initialize the VMD domain during repetitive driver
>> attachment or reboot tests, ensure that the VMD root ports are re-initialized
>> to a blank state that can be re-enumerated appropriately by the PCI core.
>> This is performed by re-initializing all of the bridge windows to ensure
>> that PCI core enumeration does not detect potentially invalid bridge windows
>> and misinterpret them as firmware-assigned windows, when they simply may be
>> invalid bridge window information from a previous boot.
> Rewrap commit log to fit in 75 columns.  No problem about v2 vs v1.
I will take care of it.
>
>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
>> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
>> ---
>>  drivers/pci/controller/vmd.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index 6e1c60048774..e52bdb95218e 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -651,6 +651,39 @@ static int vmd_alloc_irqs(struct vmd_dev *vmd)
>>  	return 0;
>>  }
>>  
>> +
> Remove spurious blank line here.
Sure.
>
>> +static void vmd_domain_reset_windows(struct vmd_dev *vmd)
>> +{
>> +	u8 hdr_type;
>> +	char __iomem *addr;
>> +	int dev_seq;
>> +	u8 functions;
>> +	u8 fn_seq;
>> +	int max_devs = resource_size(&vmd->resources[0]) * 32;
>> +
>> +	for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
>> +		addr = VMD_DEVICE_BASE(vmd, dev_seq) + PCI_VENDOR_ID;
>> +		if (readw(addr) != PCI_VENDOR_ID_INTEL)
>> +			continue;
>> +
>> +		addr = VMD_DEVICE_BASE(vmd, dev_seq) + PCI_HEADER_TYPE;
>> +		hdr_type = readb(addr) & PCI_HEADER_TYPE_MASK;
>> +		if (hdr_type != PCI_HEADER_TYPE_BRIDGE)
>> +			continue;
>> +
>> +		functions = !!(hdr_type & 0x80) ? 8 : 1;
>> +		for (fn_seq = 0; fn_seq < functions; fn_seq++)
>> +		{
> Put "{" on previous line.
>
> Looks quite parallel to vmd_domain_sbr(), except that here we iterate
> through functions as well.  Why does vmd_domain_sbr() not need to
> iterate through functions?
I am not sure if there is VMD hardware with non zero functions.
>
>> +			addr = VMD_FUNCTION_BASE(vmd, dev_seq, fn_seq) + PCI_VENDOR_ID;
>> +			if (readw(addr) != PCI_VENDOR_ID_INTEL)
>> +				continue;
>> +
>> +			memset_io((VMD_FUNCTION_BASE(vmd, dev_seq, fn_seq) + PCI_IO_BASE),
>> +			 0, PCI_ROM_ADDRESS1 - PCI_IO_BASE);
> Make the lines above fit in 80 columns.
Sure.
>
>> +		}
>> +	}
>> +}
>> +
>>  static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>>  {
>>  	struct pci_sysdata *sd = &vmd->sysdata;
>> @@ -741,6 +774,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>>  		.parent = res,
>>  	};
>>  
>> +	vmd_domain_reset_windows(vmd);
>> +
>>  	sd->vmd_dev = vmd->dev;
>>  	sd->domain = vmd_find_free_domain();
>>  	if (sd->domain < 0)
>> -- 
>> 2.27.0
>>

Re: [PATCH v2 2/2] PCI: vmd: Issue vmd domain window reset

[Bjorn Helgaas]

On Thu, Jul 22, 2021 at 11:47:06AM -0700, Patel, Nirmal wrote:
> On 7/20/2021 3:42 PM, Bjorn Helgaas wrote:
> > On Tue, Jul 20, 2021 at 01:50:09PM -0700, Nirmal Patel wrote:
> >> In order to properly re-initialize the VMD domain during repetitive driver
> >> attachment or reboot tests, ensure that the VMD root ports are re-initialized
> >> to a blank state that can be re-enumerated appropriately by the PCI core.
> >> This is performed by re-initializing all of the bridge windows to ensure
> >> that PCI core enumeration does not detect potentially invalid bridge windows
> >> and misinterpret them as firmware-assigned windows, when they simply may be
> >> invalid bridge window information from a previous boot.

> >> +static void vmd_domain_reset_windows(struct vmd_dev *vmd)
> >> +{
> >> +	u8 hdr_type;
> >> +	char __iomem *addr;
> >> +	int dev_seq;
> >> +	u8 functions;
> >> +	u8 fn_seq;
> >> +	int max_devs = resource_size(&vmd->resources[0]) * 32;
> >> +
> >> +	for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
> >> +		addr = VMD_DEVICE_BASE(vmd, dev_seq) + PCI_VENDOR_ID;
> >> +		if (readw(addr) != PCI_VENDOR_ID_INTEL)
> >> +			continue;
> >> +
> >> +		addr = VMD_DEVICE_BASE(vmd, dev_seq) + PCI_HEADER_TYPE;
> >> +		hdr_type = readb(addr) & PCI_HEADER_TYPE_MASK;
> >> +		if (hdr_type != PCI_HEADER_TYPE_BRIDGE)
> >> +			continue;
> >> +
> >> +		functions = !!(hdr_type & 0x80) ? 8 : 1;
> >> +		for (fn_seq = 0; fn_seq < functions; fn_seq++)
> >> +		{
> >
> > Looks quite parallel to vmd_domain_sbr(), except that here we iterate
> > through functions as well.  Why does vmd_domain_sbr() not need to
> > iterate through functions?
>
> I am not sure if there is VMD hardware with non zero functions.

I'm not sure either ;)  Hopefully you can resolve this one way or the
other.  It would be good to either make them the same or add a comment
about why they are different.  Otherwise it just looks like a possible
bug.

Re: [PATCH v2 1/2] PCI: vmd: Trigger secondary bus reset

[Bjorn Helgaas]

On Wed, Jul 21, 2021 at 10:50:26AM +0200, Pali Rohár wrote:
> On Tuesday 20 July 2021 13:50:08 Nirmal Patel wrote:
> > During VT-d passthrough repetitive reboot tests, it was determined that the VMD
> > domain needed to be reset in order to allow downstream devices to reinitialize
> > properly. This is done using a secondary bus reset at each of the VMD root
> > ports and any bridges in the domain.
> > 
> > Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> > Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> >  drivers/pci/controller/vmd.c | 46 ++++++++++++++++++++++++++++++++++++

> > +static void vmd_domain_sbr(struct vmd_dev *vmd)
> > +{
> > +	char __iomem *base;
> > +	u16 ctl;
> > +	int dev_seq;
> > +	int max_devs = resource_size(&vmd->resources[0]) * 32;
> > +
> > +	/*
> > +	* Subdevice config space may or many not be mapped linearly using 4k config
> > +	* space.
> > +	*/
> > +	for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
> > +		base = VMD_DEVICE_BASE(vmd, dev_seq);
> > +		if (readw(base + PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL)
> > +			continue;
> > +
> > +		if ((readb(base + PCI_HEADER_TYPE) & PCI_HEADER_TYPE_MASK) !=
> > +		    PCI_HEADER_TYPE_BRIDGE)
> > +			continue;
> > +
> > +		if (readw(base + PCI_CLASS_DEVICE) != PCI_CLASS_BRIDGE_PCI)
> > +			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);
> 
> You cannot unconditionally call secondary bus reset for arbitrary PCIe
> Bridge. Calling it breaks more PCIe devices behind bridge and
> pci_reset_secondary_bus() already handles it and skip reset if reset is
> causing issues.
> 
> I would suggest to use pci_reset_secondary_bus() and extend it
> so you can call it also from your driver.

Are you referring to PCI_DEV_FLAGS_NO_BUS_RESET?  That's handled in
pci_parent_bus_reset(), not pci_reset_secondary_bus().

I would probably agree that PCI_DEV_FLAGS_NO_BUS_RESET *should* be
checked in pci_reset_secondary_bus(), since there are several paths to
get there without going through pci_parent_bus_reset().

> > +	}
> > +	ssleep(1);
> > +}