On Wed, Jan 11, 2023 at 11:58 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> s/acceidental/accidental/ in subject
Thanks. Sorry for the fat-finger error.
> > When memset_io() clears prefetchable base 32 bits register, the
> > prefetchable memory becomes 0000000000000000-575000fffff, which is enabled.
> > This behavior (accidental enablement of window) causes that config accesses
> > get routed to the wrong place, and the access content of PCI configuration
> > space of VMD root ports is 0xff after invoking memset_io() in
> > vmd_domain_reset():
>
> I was thinking the problem was only between clearing
> PCI_PREF_MEMORY_BASE and PCI_PREF_BASE_UPPER32, but that would be a
> pretty small window, and you're seeing a lot of config accesses going
> wrong. Why is that? Is there enumeration that races with this domain
> reset?
Well, I didn't see the races. The problem is that: memset_io() uses
enhanced REP STOSB, fast-string operation or legacy method (see
arch/x86/lib/memset_64.S) to *sequentially* clear the memory location
from lower memory location to higher one. When clearing at
PCI_PREF_BASE_UPPER32, the prefetchable memory window is accidentally
enabled. The subsequent accesses (each read returns 0xff, and each
write does not take any effect) cannot be made correctly. In this
case, clearing at PCI_PREF_LIMIT_UPPER32 cannot take any effect. So,
we're unable to configure VMD devices anymore for subsequent writes.
Here are the test codes for emulating memset_io(), and they are 4-byte
writes. The issue can be reproduced:
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 769eedeb8802..e27e2a0f68e0 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -526,8 +526,15 @@ 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);
+ writel(0, base + PCI_MEMORY_BASE);
+ writel(0, base + PCI_PREF_MEMORY_BASE);
+
+ writel(0, base + PCI_PREF_BASE_UPPER32);
+ writel(0, base + PCI_PREF_LIMIT_UPPER32);
+
+ writel(0, base + PCI_IO_BASE_UPPER16);
+ writel(0, base + PCI_CAPABILITY_LIST);
}
}
}
The following test codes can fix the issue (clear
PCI_PREF_LIMIT_UPPER32 firstly):
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 769eedeb8802..b9140e081793 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -526,8 +526,15 @@ 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);
+ writel(0, base + PCI_MEMORY_BASE);
+ writel(0, base + PCI_PREF_MEMORY_BASE);
+
+ writel(0, base + PCI_PREF_LIMIT_UPPER32);
+ writel(0, base + PCI_PREF_BASE_UPPER32);
+
+ writel(0, base + PCI_IO_BASE_UPPER16);
+ writel(0, base + PCI_CAPABILITY_LIST);
> I guess the same problem occurs with PCI_IO_BASE/PCI_IO_BASE_UPPER16,
> but maybe there's no concurrent I/O port access?
For the general case, how about the following code snippets?
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 769eedeb8802..e29e33f7b70e 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -499,6 +499,19 @@ static inline void vmd_acpi_begin(void) { }
static inline void vmd_acpi_end(void) { }
#endif /* CONFIG_ACPI */
+static inline void vmd_clear_cfg_space(char __iomem *base, int start, int end)
+{
+ int i;
+
+ /*
+ * Clear PCI configuration space from higher memory location
+ * to lower one. This prevents from enabling IO window or
+ * prefetchable memory window if it is disabled initially.
+ */
+ for (i = end - 1; i >= start; i--)
+ writeb(0, base + i);
+}
+
static void vmd_domain_reset(struct vmd_dev *vmd)
{
u16 bus, max_buses = resource_size(&vmd->resources[0]);
@@ -526,8 +539,8 @@ 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);
+ vmd_clear_cfg_space(base, PCI_IO_BASE,
+ PCI_ROM_ADDRESS1);
}
}
}
-- Adrian
