* Re: [PATCH] PCI: vmd: Fix domain reset operation
2023-05-30 21:47 [PATCH] PCI: vmd: Fix domain reset operation Nirmal Patel
@ 2023-06-20 17:14 ` Patel, Nirmal
2023-06-20 17:36 ` Lorenzo Pieralisi
2023-06-20 17:58 ` Bjorn Helgaas
2023-06-20 18:07 ` Bjorn Helgaas
2 siblings, 1 reply; 7+ messages in thread
From: Patel, Nirmal @ 2023-06-20 17:14 UTC (permalink / raw)
To: Lorenzo Pieralisi; +Cc: linux-pci@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PCI: vmd: Fix domain reset operation
2023-06-20 17:14 ` Patel, Nirmal
@ 2023-06-20 17:36 ` Lorenzo Pieralisi
0 siblings, 0 replies; 7+ messages in thread
From: Lorenzo Pieralisi @ 2023-06-20 17:36 UTC (permalink / raw)
To: Patel, Nirmal; +Cc: Lorenzo Pieralisi, linux-pci@vger.kernel.org
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.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PCI: vmd: Fix domain reset operation
2023-05-30 21:47 [PATCH] PCI: vmd: Fix domain reset operation Nirmal Patel
2023-06-20 17:14 ` Patel, Nirmal
@ 2023-06-20 17:58 ` Bjorn Helgaas
2023-06-20 18:07 ` Bjorn Helgaas
2 siblings, 0 replies; 7+ messages in thread
From: Bjorn Helgaas @ 2023-06-20 17:58 UTC (permalink / raw)
To: Nirmal Patel; +Cc: linux-pci
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PCI: vmd: Fix domain reset operation
2023-05-30 21:47 [PATCH] PCI: vmd: Fix domain reset operation Nirmal Patel
2023-06-20 17:14 ` Patel, Nirmal
2023-06-20 17:58 ` Bjorn Helgaas
@ 2023-06-20 18:07 ` Bjorn Helgaas
2023-06-21 23:02 ` Patel, Nirmal
2023-07-05 17:39 ` Patel, Nirmal
2 siblings, 2 replies; 7+ messages in thread
From: Bjorn Helgaas @ 2023-06-20 18:07 UTC (permalink / raw)
To: Nirmal Patel; +Cc: linux-pci
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] PCI: vmd: Fix domain reset operation
2023-06-20 18:07 ` Bjorn Helgaas
@ 2023-06-21 23:02 ` Patel, Nirmal
2023-07-05 17:39 ` Patel, Nirmal
1 sibling, 0 replies; 7+ messages in thread
From: Patel, Nirmal @ 2023-06-21 23:02 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci
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
>>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] PCI: vmd: Fix domain reset operation
2023-06-20 18:07 ` Bjorn Helgaas
2023-06-21 23:02 ` Patel, Nirmal
@ 2023-07-05 17:39 ` Patel, Nirmal
1 sibling, 0 replies; 7+ messages in thread
From: Patel, Nirmal @ 2023-07-05 17:39 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci
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
>>
^ permalink raw reply [flat|nested] 7+ messages in thread