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?
>
> v2->v3: Combining two functions into one, Remove redundant definations
> and Formatting fixes
Below the dashes please
>
> Signed-off-by: Nirmal Patel <nirmal.patel@xxxxxxxxxxxxxxx>
> Reviewed-by: Jon Derrick <jonathan.derrick@xxxxxxxxx>
Not yet :)
> ---
> 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?
>
> #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'
> + 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);
> +
> + 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?
> +
> + 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?
> + if (readw(addr) != PCI_VENDOR_ID_INTEL)
> + continue;
Is this necessary?
> +
> + 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.
> + 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
> pci_scan_child_bus(vmd->bus);
> pci_assign_unassigned_bus_resources(vmd->bus);
>
>
