[+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@xxxxxxxxx> > --- > 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 >