Re: [PATCH v3] PCI: vmd: Issue secondary bus reset and vmd domain window reset

["Patel, Nirmal" <nirmal.patel@linux.intel.com>]

On 7/28/2021 4:21 PM, Derrick, Jonathan wrote:
> Hey Nirmal
>
> On 7/28/2021 3:46 PM, 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.
>>
>> 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 setting secondary bus
>> reset bit of each of the VMD root port and will propagate reset through
>> downstream bridges.
> Can we better combine these two paragraphs?
I will try to create better summary.
>
>
>> v2->v3: Combining two functions into one, Remove redundant definations
>>         and Formatting fixes
> Below the dashes please
Ack
>
>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
>> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
> Not yet :)
Sorry about that. will fix it.
>
>> ---
>>  drivers/pci/controller/vmd.c | 63 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 63 insertions(+)
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index e3fcdfec58b3..e2c0de700e61 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -15,6 +15,9 @@
>>  #include <linux/srcu.h>
>>  #include <linux/rculist.h>
>>  #include <linux/rcupdate.h>
>> +#include <linux/delay.h>
>> +#include <linux/pci_regs.h>
>> +#include <linux/pci_ids.h>
> Do you need to include pci_regs.h and pci_ids.h?
Works without including header files too.
>
>
>>  
>>  #include <asm/irqdomain.h>
>>  #include <asm/device.h>
>> @@ -447,6 +450,64 @@ static struct pci_ops vmd_ops = {
>>  	.write		= vmd_pci_write,
>>  };
>>  
>> +static void vmd_domain_reset(struct vmd_dev *vmd)
>> +{
>> +	char __iomem *base;
>> +	char __iomem *addr;
>> +	u16 ctl;
>> +	int dev_seq;
>> +	int max_devs = 32;
>> +	int max_buses = resource_size(&vmd->resources[0]);
>> +	int bus_seq;
>> +	u8 functions;
>> +	u8 fn_seq;
>> +	u8 hdr_type;
>> +
>> +	for(bus_seq = 0; bus_seq < max_buses; bus_seq++) {
>> +		for (dev_seq = 0; dev_seq < max_devs; dev_seq++) {
> No need for max_devs - just open-code '32'
Ack.
>
>
>> +			base = vmd->cfgbar
>> +					+ PCIE_ECAM_OFFSET(bus_seq,
>> +					   PCI_DEVFN(dev_seq, 0), PCI_VENDOR_ID);
> How about:
> 			base = vmd->cfgbar + PCIE_ECAM_OFFSET(bus_seq,
> 				 PCI_DEVFN(dev_seq, 0), PCI_VENDOR_ID);
Ack.
>
>
>> +
>> +			if (readw(base) != PCI_VENDOR_ID_INTEL)
>> +				continue;
> Now that it's iterating all of the bridges in all of the buses, should
> it be limited to Intel devices?
Ack. I will remove it. It shouldn't have significant performance hit.
>
>
>> +
>> +			hdr_type = readb(base + PCI_HEADER_TYPE) & 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->cfgbar
>> +						+ PCIE_ECAM_OFFSET(0x0,
>> +						   PCI_DEVFN(dev_seq, fn_seq), PCI_VENDOR_ID);
> Can you do the same as above here? Putting PCIE_ECAM_OFFSET on the same
> line as vmd->cfgbar? Also could you change bus from 0x0 to 0?
Yes.
>
>
>> +				if (readw(addr) != PCI_VENDOR_ID_INTEL)
>> +					continue;
> Is this necessary?
Ack.
>
>
>> +
>> +				memset_io((vmd->cfgbar +
>> +				 PCIE_ECAM_OFFSET(0x0,PCI_DEVFN(dev_seq, fn_seq),PCI_IO_BASE)),
> Needs a space after the commas, and please use 0 instead of 0x0.
Ack.
>
>
>> +				 0, PCI_ROM_ADDRESS1 - PCI_IO_BASE);
>> +			}
>> +
>> +			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 +808,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_reset(vmd);
>> +
> I'd remove this blank line
Ack.
>
>>  	pci_scan_child_bus(vmd->bus);
>>  	pci_assign_unassigned_bus_resources(vmd->bus);
>>  
>>