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@xxxxxxxxxxxxxxx>
>> Reviewed-by: Jon Derrick <jonathan.derrick@xxxxxxxxx>
> 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);
>>
>>
