[PATCH v3] PCI: vmd: Disable bridge window for domain reset

[PATCH v3] PCI: vmd: Disable bridge window for domain reset

[Nirmal Patel]

During domain reset process vmd_domain_reset() clears PCI
configuration space of VMD root ports. But certain platform
has observed following errors and failed to boot.
  ...
  DMAR: VT-d detected Invalidation Queue Error: Reason f
  DMAR: VT-d detected Invalidation Time-out Error: SID ffff
  DMAR: VT-d detected Invalidation Completion Error: SID ffff
  DMAR: QI HEAD: UNKNOWN qw0 = 0x0, qw1 = 0x0
  DMAR: QI PRIOR: UNKNOWN qw0 = 0x0, qw1 = 0x0
  DMAR: Invalidation Time-out Error (ITE) cleared

The root cause is that memset_io() clears prefetchable memory base/limit
registers and prefetchable base/limit 32 bits registers sequentially.
This seems to be enabling prefetchable memory if the device disabled
prefetchable memory originally.
Here is an example (before memset_io()):

  PCI configuration space for 10000:00:00.0:
  86 80 30 20 06 00 10 00 04 00 04 06 00 00 01 00
  00 00 00 00 00 00 00 00 00 01 01 00 00 00 00 20
  00 00 00 00 01 00 01 00 ff ff ff ff 75 05 00 00
  ...

So, prefetchable memory is ffffffff00000000-575000fffff, which is
disabled. When memset_io() clears prefetchable base 32 bits register,
the prefetchable memory becomes 0000000000000000-575000fffff, which is
enabled.

This is believed to be the reason for the failure and in addition the
sequence of operation in vmd_domain_reset() is not following the PCIe
specs.

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 bridg

Disable the bridge window by executing a sequence of operations
borrowed from pci_disable_bridge_window(), that comply with the
PCI specifications.

Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
---
v3->v3: Add more information to commit description.
v1->v2: Follow same chain of operation as pci_disable_bridge_window
        and update commit log.
---
 drivers/pci/controller/vmd.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 769eedeb8802..ca9081be917d 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -526,8 +526,16 @@ 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);
+				/* MMIO Base/Limit */
+				writel(0x0000fff0, base + PCI_MEMORY_BASE);
+
+				/* Prefetchable MMIO Base/Limit */
+				writel(0, base + PCI_PREF_LIMIT_UPPER32);
+				writel(0x0000fff0, base + PCI_PREF_MEMORY_BASE);
+				writel(0xffffffff, base + PCI_PREF_BASE_UPPER32);
+				writel(0, base + PCI_IO_BASE_UPPER16);
+				writeb(0, base + PCI_CAPABILITY_LIST);
 			}
 		}
 	}
-- 
2.31.1

Re: [PATCH v3] PCI: vmd: Disable bridge window for domain reset

[Bjorn Helgaas]

On Mon, Aug 07, 2023 at 04:45:20PM -0400, Nirmal Patel wrote:
> During domain reset process vmd_domain_reset() clears PCI
> configuration space of VMD root ports. But certain platform
> has observed following errors and failed to boot.
>   ...
>   DMAR: VT-d detected Invalidation Queue Error: Reason f
>   DMAR: VT-d detected Invalidation Time-out Error: SID ffff
>   DMAR: VT-d detected Invalidation Completion Error: SID ffff
>   DMAR: QI HEAD: UNKNOWN qw0 = 0x0, qw1 = 0x0
>   DMAR: QI PRIOR: UNKNOWN qw0 = 0x0, qw1 = 0x0
>   DMAR: Invalidation Time-out Error (ITE) cleared
> 
> The root cause is that memset_io() clears prefetchable memory base/limit
> registers and prefetchable base/limit 32 bits registers sequentially.
> This seems to be enabling prefetchable memory if the device disabled
> prefetchable memory originally.
> Here is an example (before memset_io()):

Paragraph separation or wrapping error.

>   PCI configuration space for 10000:00:00.0:
>   86 80 30 20 06 00 10 00 04 00 04 06 00 00 01 00
>   00 00 00 00 00 00 00 00 00 01 01 00 00 00 00 20
>   00 00 00 00 01 00 01 00 ff ff ff ff 75 05 00 00
>   ...
> 
> So, prefetchable memory is ffffffff00000000-575000fffff, which is
> disabled. When memset_io() clears prefetchable base 32 bits register,
> the prefetchable memory becomes 0000000000000000-575000fffff, which is
> enabled.
> 
> This is believed to be the reason for the failure and in addition the
> sequence of operation in vmd_domain_reset() is not following the PCIe
> specs.
> 
> 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 bridg

s/bridg/bridge/

The [mem 0x0-0x575000fffff] state is transient, right?  After the
memset_io() completes, the window is [mem 0x0-0x000fffff], which still
not what you want, since it's enabled, while you want to *disable* the
window.

I don't know what the connection between this and the DMAR
invalidation queue errors is.  Maybe those can happen with either the
transient [mem 0x0-0x575000fffff] state or the [mem 0x0-0x000fffff]
end state?

IIUC there are two problems with the memset_io():

  1) The memset_io() writes 0 to all the base and limit registers.
     For address decoding purposes, the low bits of the base are
     implicitly clear while the low bits of the limit are implicitly
     set, so setting the base to zero always makes the windows
     *enabled*, e.g., [io 0x0000-0x0fff].

  2) The I/O and prefetchable base/limit can't be configured with a
     single config write, so we have to write them in a specific order
     to avoid transient enabled windows that could cause conflicts.

> Disable the bridge window by executing a sequence of operations
> borrowed from pci_disable_bridge_window(), that comply with the
> PCI specifications.
> 
> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> ---
> v3->v3: Add more information to commit description.
> v1->v2: Follow same chain of operation as pci_disable_bridge_window
>         and update commit log.
> ---
>  drivers/pci/controller/vmd.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 769eedeb8802..ca9081be917d 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -526,8 +526,16 @@ 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);

This is a 32-bit write, so it writes zero to PCI_IO_BASE,
PCI_IO_LIMIT, and PCI_SEC_STATUS.  Writing zero to PCI_SEC_STATUS is
probably harmless since everything there is RO or RW1C, but is
unnecessary and seems a little sloppy.

Writing zero to PCI_IO_BASE and PCI_IO_LIMIT enables it as
[io 0x0000-0x0fff].  I think the code in pci_setup_bridge_io() is more
like what you want.

> +				/* MMIO Base/Limit */
> +				writel(0x0000fff0, base + PCI_MEMORY_BASE);
> +
> +				/* Prefetchable MMIO Base/Limit */
> +				writel(0, base + PCI_PREF_LIMIT_UPPER32);
> +				writel(0x0000fff0, base + PCI_PREF_MEMORY_BASE);
> +				writel(0xffffffff, base + PCI_PREF_BASE_UPPER32);
> +				writel(0, base + PCI_IO_BASE_UPPER16);
kkkk
> +				writeb(0, base + PCI_CAPABILITY_LIST);
>  			}
>  		}
>  	}
> -- 
> 2.31.1
> 

Re: [PATCH v3] PCI: vmd: Disable bridge window for domain reset

[Patel, Nirmal]

On 8/7/2023 3:23 PM, Bjorn Helgaas wrote:
> On Mon, Aug 07, 2023 at 04:45:20PM -0400, Nirmal Patel wrote:
>> During domain reset process vmd_domain_reset() clears PCI
>> configuration space of VMD root ports. But certain platform
>> has observed following errors and failed to boot.
>>   ...
>>   DMAR: VT-d detected Invalidation Queue Error: Reason f
>>   DMAR: VT-d detected Invalidation Time-out Error: SID ffff
>>   DMAR: VT-d detected Invalidation Completion Error: SID ffff
>>   DMAR: QI HEAD: UNKNOWN qw0 = 0x0, qw1 = 0x0
>>   DMAR: QI PRIOR: UNKNOWN qw0 = 0x0, qw1 = 0x0
>>   DMAR: Invalidation Time-out Error (ITE) cleared
>>
>> The root cause is that memset_io() clears prefetchable memory base/limit
>> registers and prefetchable base/limit 32 bits registers sequentially.
>> This seems to be enabling prefetchable memory if the device disabled
>> prefetchable memory originally.
>> Here is an example (before memset_io()):
> Paragraph separation or wrapping error.
I will fix it.
>
>>   PCI configuration space for 10000:00:00.0:
>>   86 80 30 20 06 00 10 00 04 00 04 06 00 00 01 00
>>   00 00 00 00 00 00 00 00 00 01 01 00 00 00 00 20
>>   00 00 00 00 01 00 01 00 ff ff ff ff 75 05 00 00
>>   ...
>>
>> So, prefetchable memory is ffffffff00000000-575000fffff, which is
>> disabled. When memset_io() clears prefetchable base 32 bits register,
>> the prefetchable memory becomes 0000000000000000-575000fffff, which is
>> enabled.
>>
>> This is believed to be the reason for the failure and in addition the
>> sequence of operation in vmd_domain_reset() is not following the PCIe
>> specs.
>>
>> 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 bridg
> s/bridg/bridge/
>
> The [mem 0x0-0x575000fffff] state is transient, right?  After the
> memset_io() completes, the window is [mem 0x0-0x000fffff], which still
> not what you want, since it's enabled, while you want to *disable* the
> window.
>
> I don't know what the connection between this and the DMAR
> invalidation queue errors is.  Maybe those can happen with either the
> transient [mem 0x0-0x575000fffff] state or the [mem 0x0-0x000fffff]
> end state?
>
> IIUC there are two problems with the memset_io():
>
>   1) The memset_io() writes 0 to all the base and limit registers.
>      For address decoding purposes, the low bits of the base are
>      implicitly clear while the low bits of the limit are implicitly
>      set, so setting the base to zero always makes the windows
>      *enabled*, e.g., [io 0x0000-0x0fff].
>
>   2) The I/O and prefetchable base/limit can't be configured with a
>      single config write, so we have to write them in a specific order
>      to avoid transient enabled windows that could cause conflicts.
Yes. Your explanation is very accurate.
>
>> Disable the bridge window by executing a sequence of operations
>> borrowed from pci_disable_bridge_window(), that comply with the
>> PCI specifications.
>>
>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
>> ---
>> v3->v3: Add more information to commit description.
>> v1->v2: Follow same chain of operation as pci_disable_bridge_window
>>         and update commit log.
>> ---
>>  drivers/pci/controller/vmd.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index 769eedeb8802..ca9081be917d 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -526,8 +526,16 @@ 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);
> This is a 32-bit write, so it writes zero to PCI_IO_BASE,
> PCI_IO_LIMIT, and PCI_SEC_STATUS.  Writing zero to PCI_SEC_STATUS is
> probably harmless since everything there is RO or RW1C, but is
> unnecessary and seems a little sloppy.
>
> Writing zero to PCI_IO_BASE and PCI_IO_LIMIT enables it as
> [io 0x0000-0x0fff].  I think the code in pci_setup_bridge_io() is more
> like what you want.
I will make changes according to pci_setup_bridge_io() ASAP.

Thanks.

>
>> +				/* MMIO Base/Limit */
>> +				writel(0x0000fff0, base + PCI_MEMORY_BASE);
>> +
>> +				/* Prefetchable MMIO Base/Limit */
>> +				writel(0, base + PCI_PREF_LIMIT_UPPER32);
>> +				writel(0x0000fff0, base + PCI_PREF_MEMORY_BASE);
>> +				writel(0xffffffff, base + PCI_PREF_BASE_UPPER32);
>> +				writel(0, base + PCI_IO_BASE_UPPER16);
> kkkk
>> +				writeb(0, base + PCI_CAPABILITY_LIST);
>>  			}
>>  		}
>>  	}
>> -- 
>> 2.31.1
>>