| Message ID | 20210728214639.7204-1-nirmal.patel@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [v3] PCI: vmd: Issue secondary bus reset and vmd domain window reset | expand |
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@linux.intel.com> > Reviewed-by: Jon Derrick <jonathan.derrick@intel.com> 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); > >
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); >> >>
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> #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++) { + base = vmd->cfgbar + + PCIE_ECAM_OFFSET(bus_seq, + PCI_DEVFN(dev_seq, 0), PCI_VENDOR_ID); + + if (readw(base) != PCI_VENDOR_ID_INTEL) + continue; + + 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); + if (readw(addr) != PCI_VENDOR_ID_INTEL) + continue; + + memset_io((vmd->cfgbar + + PCIE_ECAM_OFFSET(0x0,PCI_DEVFN(dev_seq, fn_seq),PCI_IO_BASE)), + 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); + pci_scan_child_bus(vmd->bus); pci_assign_unassigned_bus_resources(vmd->bus);
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. v2->v3: Combining two functions into one, Remove redundant definations and Formatting fixes Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com> --- drivers/pci/controller/vmd.c | 63 ++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)