linux-kernel - Re: [PATCH v1] PCI: qcom: Avoid DBI and ATU register space mirror to BAR/MMIO region

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 24 Jun 2024 10:04:54 -0700
From: Mayank Rana <quic_mrana@...cinc.com>
To: Bjorn Helgaas <helgaas@...nel.org>,
        Manivannan Sadhasivam
	<manivannan.sadhasivam@...aro.org>
CC: Prudhvi Yarlagadda <quic_pyarlaga@...cinc.com>, <lpieralisi@...nel.org>,
        <kw@...ux.com>, <bhelgaas@...gle.com>, <robh@...nel.org>,
        <linux-pci@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] PCI: qcom: Avoid DBI and ATU register space mirror to
 BAR/MMIO region



On 6/22/2024 10:41 AM, Bjorn Helgaas wrote:
> On Sat, Jun 22, 2024 at 09:24:44AM +0530, Manivannan Sadhasivam wrote:
>> On Thu, Jun 20, 2024 at 02:34:05PM -0700, Prudhvi Yarlagadda wrote:
>>> PARF hardware block which is a wrapper on top of DWC PCIe controller
>>> mirrors the DBI and ATU register space. It uses PARF_SLV_ADDR_SPACE_SIZE
>>> register to get the size of the memory block to be mirrored and uses
>>> PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR registers to determine the base
>>> address of DBI and ATU space inside the memory block that is being
>>> mirrored.
>>
>> This PARF_SLV_ADDR_SPACE register is a mystery to me. I tried getting to the
>> bottom of it, but nobody could explain it to me clearly. Looks like you know
>> more about it...
>>
>>  From your description, it seems like this register specifies the size of the
>> mirroring region (ATU + DBI), but the response from your colleague indicates
>> something different [1].
>>
>> [1] https://lore.kernel.org/linux-pci/f42559f5-9d4c-4667-bf0e-7abfd9983c36@quicinc.com/
>>
>>> When a memory region which is located above the SLV_ADDR_SPACE_SIZE
>>> boundary is used for BAR region then there could be an overlap of DBI and
>>> ATU address space that is getting mirrored and the BAR region. This
>>> results in DBI and ATU address space contents getting updated when a PCIe
>>> function driver tries updating the BAR/MMIO memory region. Reference
>>> memory map of the PCIe memory region with DBI and ATU address space
>>> overlapping BAR region is as below.
>>>
>>> 			|---------------|
>>> 			|		|
>>> 			|		|
>>> 	-------	--------|---------------|
>>> 	   |	   |	|---------------|
>>> 	   |	   |	|	DBI	|
>>> 	   |	   |	|---------------|---->DBI_BASE_ADDR
>>> 	   |	   |	|		|
>>> 	   |	   |	|		|
>>> 	   |	PCIe	|		|---->2*SLV_ADDR_SPACE_SIZE
>>> 	   |	BAR/MMIO|---------------|
>>> 	   |	Region	|	ATU	|
>>> 	   |	   |	|---------------|---->ATU_BASE_ADDR
>>> 	   |	   |	|		|
>>> 	PCIe	   |	|---------------|
>>> 	Memory	   |	|	DBI	|
>>> 	Region	   |	|---------------|---->DBI_BASE_ADDR
>>> 	   |	   |	|		|
>>> 	   |	--------|		|
>>> 	   |		|		|---->SLV_ADDR_SPACE_SIZE
>>> 	   |		|---------------|
>>> 	   |		|	ATU	|
>>> 	   |		|---------------|---->ATU_BASE_ADDR
>>> 	   |		|		|
>>> 	   |		|---------------|
>>> 	   |		|	DBI	|
>>> 	   |		|---------------|---->DBI_BASE_ADDR
>>> 	   |		|		|
>>> 	   |		|		|
>>> 	----------------|---------------|
>>> 			|		|
>>> 			|		|
>>> 			|		|
>>> 			|---------------|
>>>
>>> Currently memory region beyond the SLV_ADDR_SPACE_SIZE boundary is
>>> not used for BAR region which is why the above mentioned issue is
>>> not encountered. This issue is discovered as part of internal
>>> testing when we tried moving the BAR region beyond the
>>> SLV_ADDR_SPACE_SIZE boundary. Hence we are trying to fix this.
>>
>> I don't quite understand this. PoR value of SLV_ADDR_SPACE_SIZE is
>> 16MB and most of the platforms have the size of whole PCIe region
>> defined in DT as 512MB (registers + I/O + MEM). So the range is
>> already crossing the SLV_ADDR_SPACE_SIZE boundary.
>>
>> Ironically, the patch I pointed out above changes the value of this
>> register as 128MB, and the PCIe region size of that platform is also
>> 128MB. The author of that patch pointed out that if the
>> SLV_ADDR_SPACE_SIZE is set to 256MB, then they are seeing
>> enumeration failures. If we go by your description of that register,
>> the SLV_ADDR_SPACE_SIZE of 256MB should be > PCIe region size of
>> 128MB. So they should not see any issues, right?
>>
>>> As PARF hardware block mirrors DBI and ATU register space after
>>> every PARF_SLV_ADDR_SPACE_SIZE (default 0x1000000) boundary
>>> multiple, write U64_MAX to PARF_SLV_ADDR_SPACE_SIZE register to
>>> avoid mirroring DBI and ATU to BAR/MMIO region.
>>
>> Looks like you are trying to avoid this mirroring on a whole. First
>> of all, what is the reasoning behind this mirroring?
> 
> This sounds like what we usually call "aliasing" that happens when
> some upper address bits are ignored.
Yes, here we are trying to disable altogether aliasing (mirroring of 
DBI/ATU) address space).

Regards,
Mayank