| Message ID | 20230530214706.75700-1-nirmal.patel@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: vmd: Fix domain reset operation | expand |
On 5/30/2023 2:47 PM, Nirmal Patel wrote: > During domain reset process we are accidentally enabling > the prefetchable memory by writing 0x0 to Prefetchable Memory > Base and Prefetchable Memory Limit registers. As a result certain > platforms failed to boot up. > > Here is the quote from section 7.5.1.3.9 of PCI Express Base 6.0 spec: > > The Prefetchable Memory Limit register must be programmed to a smaller > value than the Prefetchable Memory Base register if there is no > prefetchable memory on the secondary side of the bridge. > > When clearing Prefetchable Memory Base, Prefetchable Memory > Limit and Prefetchable Base Upper 32 bits, the prefetchable > memory range becomes 0x0-0x575000fffff. As a result the > prefetchable memory is enabled accidentally. > > Implementing correct operation by writing a value to Prefetchable > Base Memory larger than the value of Prefetchable Memory Limit. > > Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com> > --- > drivers/pci/controller/vmd.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > index 769eedeb8802..f3eb740e3028 100644 > --- a/drivers/pci/controller/vmd.c > +++ b/drivers/pci/controller/vmd.c > @@ -526,8 +526,18 @@ static void vmd_domain_reset(struct vmd_dev *vmd) > PCI_CLASS_BRIDGE_PCI)) > continue; > > - memset_io(base + PCI_IO_BASE, 0, > - PCI_ROM_ADDRESS1 - PCI_IO_BASE); > + writel(0, base + PCI_IO_BASE); > + writew(0xFFF0, base + PCI_MEMORY_BASE); > + writew(0, base + PCI_MEMORY_LIMIT); > + > + writew(0xFFF1, base + PCI_PREF_MEMORY_BASE); > + writew(0, base + PCI_PREF_MEMORY_LIMIT); > + > + writel(0xFFFFFFFF, base + PCI_PREF_BASE_UPPER32); > + writel(0, base + PCI_PREF_LIMIT_UPPER32); > + > + writel(0, base + PCI_IO_BASE_UPPER16); > + writeb(0, base + PCI_CAPABILITY_LIST); > } > } > } Gentle reminder! Thanks.
On Tue, Jun 20, 2023 at 10:14:10AM -0700, Patel, Nirmal wrote: > On 5/30/2023 2:47 PM, Nirmal Patel wrote: > > > During domain reset process we are accidentally enabling > > the prefetchable memory by writing 0x0 to Prefetchable Memory > > Base and Prefetchable Memory Limit registers. As a result certain > > platforms failed to boot up. > > > > Here is the quote from section 7.5.1.3.9 of PCI Express Base 6.0 spec: > > > > The Prefetchable Memory Limit register must be programmed to a smaller > > value than the Prefetchable Memory Base register if there is no > > prefetchable memory on the secondary side of the bridge. > > > > When clearing Prefetchable Memory Base, Prefetchable Memory > > Limit and Prefetchable Base Upper 32 bits, the prefetchable > > memory range becomes 0x0-0x575000fffff. As a result the I don't get why the top 32 bits aren't cleared. The patch is fine to me. Lorenzo > > prefetchable memory is enabled accidentally. > > > > Implementing correct operation by writing a value to Prefetchable > > Base Memory larger than the value of Prefetchable Memory Limit. > > > > Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com> > > --- > > drivers/pci/controller/vmd.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > > index 769eedeb8802..f3eb740e3028 100644 > > --- a/drivers/pci/controller/vmd.c > > +++ b/drivers/pci/controller/vmd.c > > @@ -526,8 +526,18 @@ static void vmd_domain_reset(struct vmd_dev *vmd) > > PCI_CLASS_BRIDGE_PCI)) > > continue; > > > > - memset_io(base + PCI_IO_BASE, 0, > > - PCI_ROM_ADDRESS1 - PCI_IO_BASE); > > + writel(0, base + PCI_IO_BASE); > > + writew(0xFFF0, base + PCI_MEMORY_BASE); > > + writew(0, base + PCI_MEMORY_LIMIT); > > + > > + writew(0xFFF1, base + PCI_PREF_MEMORY_BASE); > > + writew(0, base + PCI_PREF_MEMORY_LIMIT); > > + > > + writel(0xFFFFFFFF, base + PCI_PREF_BASE_UPPER32); > > + writel(0, base + PCI_PREF_LIMIT_UPPER32); > > + > > + writel(0, base + PCI_IO_BASE_UPPER16); > > + writeb(0, base + PCI_CAPABILITY_LIST); > > } > > } > > } > > Gentle reminder! > Thanks. >
On Tue, May 30, 2023 at 02:47:06PM -0700, Nirmal Patel wrote: > During domain reset process we are accidentally enabling > the prefetchable memory by writing 0x0 to Prefetchable Memory > Base and Prefetchable Memory Limit registers. As a result certain > platforms failed to boot up. > > Here is the quote from section 7.5.1.3.9 of PCI Express Base 6.0 spec: > > The Prefetchable Memory Limit register must be programmed to a smaller > value than the Prefetchable Memory Base register if there is no > prefetchable memory on the secondary side of the bridge. > > When clearing Prefetchable Memory Base, Prefetchable Memory > Limit and Prefetchable Base Upper 32 bits, the prefetchable > memory range becomes 0x0-0x575000fffff. As a result the > prefetchable memory is enabled accidentally. > > Implementing correct operation by writing a value to Prefetchable > Base Memory larger than the value of Prefetchable Memory Limit. Can you make the subject line more descriptive than "Fix X"? There are a zillion ways X might be broken, so it's useful to have a hint about which one it is. Actually, vmd_domain_reset() is sort of weirdly named, because I don't see any *reset* there. It looks like it's actually disabling all bridge windows (not just the prefetchable one, as the log suggests). The fact that this makes a difference suggests that the bridges had PCI_COMMAND_MEMORY set, which sounds like something you may not have wanted. > Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com> > --- > drivers/pci/controller/vmd.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > index 769eedeb8802..f3eb740e3028 100644 > --- a/drivers/pci/controller/vmd.c > +++ b/drivers/pci/controller/vmd.c > @@ -526,8 +526,18 @@ static void vmd_domain_reset(struct vmd_dev *vmd) > PCI_CLASS_BRIDGE_PCI)) > continue; > > - memset_io(base + PCI_IO_BASE, 0, > - PCI_ROM_ADDRESS1 - PCI_IO_BASE); > + writel(0, base + PCI_IO_BASE); > + writew(0xFFF0, base + PCI_MEMORY_BASE); > + writew(0, base + PCI_MEMORY_LIMIT); > + > + writew(0xFFF1, base + PCI_PREF_MEMORY_BASE); > + writew(0, base + PCI_PREF_MEMORY_LIMIT); > + > + writel(0xFFFFFFFF, base + PCI_PREF_BASE_UPPER32); > + writel(0, base + PCI_PREF_LIMIT_UPPER32); > + > + writel(0, base + PCI_IO_BASE_UPPER16); > + writeb(0, base + PCI_CAPABILITY_LIST); > } > } > } > -- > 2.27.0 >
On Tue, May 30, 2023 at 02:47:06PM -0700, Nirmal Patel wrote: > During domain reset process we are accidentally enabling > the prefetchable memory by writing 0x0 to Prefetchable Memory > Base and Prefetchable Memory Limit registers. As a result certain > platforms failed to boot up. > > Here is the quote from section 7.5.1.3.9 of PCI Express Base 6.0 spec: > > The Prefetchable Memory Limit register must be programmed to a smaller > value than the Prefetchable Memory Base register if there is no > prefetchable memory on the secondary side of the bridge. > > When clearing Prefetchable Memory Base, Prefetchable Memory > Limit and Prefetchable Base Upper 32 bits, the prefetchable > memory range becomes 0x0-0x575000fffff. As a result the > prefetchable memory is enabled accidentally. > > Implementing correct operation by writing a value to Prefetchable > Base Memory larger than the value of Prefetchable Memory Limit. Oh, I forgot: better to use imperative mood here instead of present participle ("-ing" form), e.g., "Write ... so that ..." This should probably use the same form as pci_disable_bridge_window(), since I think it's doing the same thing. > Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com> > --- > drivers/pci/controller/vmd.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > index 769eedeb8802..f3eb740e3028 100644 > --- a/drivers/pci/controller/vmd.c > +++ b/drivers/pci/controller/vmd.c > @@ -526,8 +526,18 @@ static void vmd_domain_reset(struct vmd_dev *vmd) > PCI_CLASS_BRIDGE_PCI)) > continue; > > - memset_io(base + PCI_IO_BASE, 0, > - PCI_ROM_ADDRESS1 - PCI_IO_BASE); > + writel(0, base + PCI_IO_BASE); > + writew(0xFFF0, base + PCI_MEMORY_BASE); > + writew(0, base + PCI_MEMORY_LIMIT); > + > + writew(0xFFF1, base + PCI_PREF_MEMORY_BASE); > + writew(0, base + PCI_PREF_MEMORY_LIMIT); > + > + writel(0xFFFFFFFF, base + PCI_PREF_BASE_UPPER32); > + writel(0, base + PCI_PREF_LIMIT_UPPER32); > + > + writel(0, base + PCI_IO_BASE_UPPER16); > + writeb(0, base + PCI_CAPABILITY_LIST); > } > } > } > -- > 2.27.0 >
On 6/20/2023 11:07 AM, Bjorn Helgaas wrote: > On Tue, May 30, 2023 at 02:47:06PM -0700, Nirmal Patel wrote: >> During domain reset process we are accidentally enabling >> the prefetchable memory by writing 0x0 to Prefetchable Memory >> Base and Prefetchable Memory Limit registers. As a result certain >> platforms failed to boot up. >> >> Here is the quote from section 7.5.1.3.9 of PCI Express Base 6.0 spec: >> >> The Prefetchable Memory Limit register must be programmed to a smaller >> value than the Prefetchable Memory Base register if there is no >> prefetchable memory on the secondary side of the bridge. >> >> When clearing Prefetchable Memory Base, Prefetchable Memory >> Limit and Prefetchable Base Upper 32 bits, the prefetchable >> memory range becomes 0x0-0x575000fffff. As a result the >> prefetchable memory is enabled accidentally. >> >> Implementing correct operation by writing a value to Prefetchable >> Base Memory larger than the value of Prefetchable Memory Limit. > Oh, I forgot: better to use imperative mood here instead of present > participle ("-ing" form), e.g., "Write ... so that ..." > > This should probably use the same form as pci_disable_bridge_window(), > since I think it's doing the same thing. I will make the adjustment. > >> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com> >> --- >> drivers/pci/controller/vmd.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c >> index 769eedeb8802..f3eb740e3028 100644 >> --- a/drivers/pci/controller/vmd.c >> +++ b/drivers/pci/controller/vmd.c >> @@ -526,8 +526,18 @@ static void vmd_domain_reset(struct vmd_dev *vmd) >> PCI_CLASS_BRIDGE_PCI)) >> continue; >> >> - memset_io(base + PCI_IO_BASE, 0, >> - PCI_ROM_ADDRESS1 - PCI_IO_BASE); >> + writel(0, base + PCI_IO_BASE); >> + writew(0xFFF0, base + PCI_MEMORY_BASE); >> + writew(0, base + PCI_MEMORY_LIMIT); >> + >> + writew(0xFFF1, base + PCI_PREF_MEMORY_BASE); >> + writew(0, base + PCI_PREF_MEMORY_LIMIT); >> + >> + writel(0xFFFFFFFF, base + PCI_PREF_BASE_UPPER32); >> + writel(0, base + PCI_PREF_LIMIT_UPPER32); >> + >> + writel(0, base + PCI_IO_BASE_UPPER16); >> + writeb(0, base + PCI_CAPABILITY_LIST); >> } >> } >> } >> -- >> 2.27.0 >>
On 6/20/2023 11:07 AM, Bjorn Helgaas wrote: > On Tue, May 30, 2023 at 02:47:06PM -0700, Nirmal Patel wrote: >> During domain reset process we are accidentally enabling >> the prefetchable memory by writing 0x0 to Prefetchable Memory >> Base and Prefetchable Memory Limit registers. As a result certain >> platforms failed to boot up. >> >> Here is the quote from section 7.5.1.3.9 of PCI Express Base 6.0 spec: >> >> The Prefetchable Memory Limit register must be programmed to a smaller >> value than the Prefetchable Memory Base register if there is no >> prefetchable memory on the secondary side of the bridge. >> >> When clearing Prefetchable Memory Base, Prefetchable Memory >> Limit and Prefetchable Base Upper 32 bits, the prefetchable >> memory range becomes 0x0-0x575000fffff. As a result the >> prefetchable memory is enabled accidentally. >> >> Implementing correct operation by writing a value to Prefetchable >> Base Memory larger than the value of Prefetchable Memory Limit. > Oh, I forgot: better to use imperative mood here instead of present > participle ("-ing" form), e.g., "Write ... so that ..." > > This should probably use the same form as pci_disable_bridge_window(), > since I think it's doing the same thing. will do that. Thanks. > >> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com> >> --- >> drivers/pci/controller/vmd.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c >> index 769eedeb8802..f3eb740e3028 100644 >> --- a/drivers/pci/controller/vmd.c >> +++ b/drivers/pci/controller/vmd.c >> @@ -526,8 +526,18 @@ static void vmd_domain_reset(struct vmd_dev *vmd) >> PCI_CLASS_BRIDGE_PCI)) >> continue; >> >> - memset_io(base + PCI_IO_BASE, 0, >> - PCI_ROM_ADDRESS1 - PCI_IO_BASE); >> + writel(0, base + PCI_IO_BASE); >> + writew(0xFFF0, base + PCI_MEMORY_BASE); >> + writew(0, base + PCI_MEMORY_LIMIT); >> + >> + writew(0xFFF1, base + PCI_PREF_MEMORY_BASE); >> + writew(0, base + PCI_PREF_MEMORY_LIMIT); >> + >> + writel(0xFFFFFFFF, base + PCI_PREF_BASE_UPPER32); >> + writel(0, base + PCI_PREF_LIMIT_UPPER32); >> + >> + writel(0, base + PCI_IO_BASE_UPPER16); >> + writeb(0, base + PCI_CAPABILITY_LIST); >> } >> } >> } >> -- >> 2.27.0 >>
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c index 769eedeb8802..f3eb740e3028 100644 --- a/drivers/pci/controller/vmd.c +++ b/drivers/pci/controller/vmd.c @@ -526,8 +526,18 @@ static void vmd_domain_reset(struct vmd_dev *vmd) PCI_CLASS_BRIDGE_PCI)) continue; - memset_io(base + PCI_IO_BASE, 0, - PCI_ROM_ADDRESS1 - PCI_IO_BASE); + writel(0, base + PCI_IO_BASE); + writew(0xFFF0, base + PCI_MEMORY_BASE); + writew(0, base + PCI_MEMORY_LIMIT); + + writew(0xFFF1, base + PCI_PREF_MEMORY_BASE); + writew(0, base + PCI_PREF_MEMORY_LIMIT); + + writel(0xFFFFFFFF, base + PCI_PREF_BASE_UPPER32); + writel(0, base + PCI_PREF_LIMIT_UPPER32); + + writel(0, base + PCI_IO_BASE_UPPER16); + writeb(0, base + PCI_CAPABILITY_LIST); } } }
During domain reset process we are accidentally enabling the prefetchable memory by writing 0x0 to Prefetchable Memory Base and Prefetchable Memory Limit registers. As a result certain platforms failed to boot up. Here is the quote from section 7.5.1.3.9 of PCI Express Base 6.0 spec: The Prefetchable Memory Limit register must be programmed to a smaller value than the Prefetchable Memory Base register if there is no prefetchable memory on the secondary side of the bridge. When clearing Prefetchable Memory Base, Prefetchable Memory Limit and Prefetchable Base Upper 32 bits, the prefetchable memory range becomes 0x0-0x575000fffff. As a result the prefetchable memory is enabled accidentally. Implementing correct operation by writing a value to Prefetchable Base Memory larger than the value of Prefetchable Memory Limit. Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com> --- drivers/pci/controller/vmd.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)