| 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
| ||
|
Date: Tue, 28 Jun 2016 09:03:09 +0530
From: Pratyush Anand <pratyush.anand@...il.com>
To: "dongbo (E)" <dongbo4@...wei.com>
Cc: "jingoohan1@...il.com" <jingoohan1@...il.com>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Linuxarm <linuxarm@...wei.com>,
Zhanweitao <zhanweitao@...ilicon.com>
Subject: Re: [PATCH] Decouple CFG and IO in Designware PCIe Driver
On Mon, Jun 27, 2016 at 6:38 PM, dongbo (E) <dongbo4@...wei.com> wrote:
> Hi, all.
>
> How about exchanging the assignments of `MEMORYs' and `CFGs/IOs'?
> In other words, assign MEMEORYs to iatu0, CFGs and IOs to iatu1.
>
> Once the iatu0 is initialized to MEMORY accesses, its BASE_ADDR,
> LIMIT and TYPE is fixed. MEMORYs match with iatu0 at first, so
> they will never being treated as IOs or CFGs by mistake.
This should be acceptable, so you can send it as a formal patch.
However, other corner point for failure is still there. Suppose,
another thread tries to write on IO space when iatu1 was programmed
for CFG.
~Pratyush
>
> The number of viewpoints used remains two, it would be OK for
> platforms only have 2 viewpoints.
>
> Here is the patch:
>
> Signed-off-by: Dong Bo <dongbo4@...wei.com>
> ---
> drivers/pci/host/pcie-designware.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index aafd766..1bd88d9 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -599,11 +599,11 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> va_cfg_base = pp->va_cfg1_base;
> }
>
> - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
> + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> type, cpu_addr,
> busdev, cfg_size);
> ret = dw_pcie_cfg_read(va_cfg_base + where, size, val);
> - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
> + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> PCIE_ATU_TYPE_IO, pp->io_base,
> pp->io_bus_addr, pp->io_size);
>
> @@ -636,11 +636,11 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> va_cfg_base = pp->va_cfg1_base;
> }
>
> - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
> + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> type, cpu_addr,
> busdev, cfg_size);
> ret = dw_pcie_cfg_write(va_cfg_base + where, size, val);
> - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
> + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> PCIE_ATU_TYPE_IO, pp->io_base,
> pp->io_bus_addr, pp->io_size);
>
> @@ -779,7 +779,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> * we should not program the ATU here.
> */
> if (!pp->ops->rd_other_conf)
> - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
> PCIE_ATU_TYPE_MEM, pp->mem_base,
> pp->mem_bus_addr, pp->mem_size);
>
> --
> 1.9.1
>
> On 2016/6/27 12:37, Pratyush Anand wrote:
>> On Wed, Jun 22, 2016 at 1:54 PM, dongbo (E) <dongbo4@...wei.com> wrote:
>>> From: Dong Bo <dongbo4@...wei.com>
>>>
>>> In designware PCIe driver, the iatu0 is used for both CFG and IO accesses.
>>> When PCIe sends CFGs to peripherals (e.g. lspci),
>>> iatu0 frequently switches between CFG and IO alternatively.
>>>
>>> If the LIMIT of MEMORY is a value between CFGs BASE_ADDR and IOs LIMIT,
>>> this probably results in a MEMORY beging matched to be an IO by mistake.
>>>
>>> Considering the following configurations:
>>> MEMORY -> BASE_ADDR: 0xb4100000, LIMIT: 0xb4100FFF, TYPE=mem
>>> CFG -> BASE_ADDR: 0xb4000000, LIMIT: 0xb4000FFF, TYPE=cfg
>>> IO -> BASE_ADDR: 0xFFFFFFFF, LIMIT: 0xFFFFFFFE, TYPE=io
>>> Suppose PCIe has just completed a CFG access, to switch back to IO, it set the BASE_ADDR to 0xFFFFFFFF, LIMIT 0xFFFFFFFE and TYPE to io.
>>> When another CFG access come,
>>> PCIe first set BASE_ADDR to 0xb4000000 to switch to CFG.
>>> At this moment, a MEMORY access shows up, due to `0xb4000000 <= MEMORY BASE_ADDR <= MEMORY LIMIE <= 0xFFFFFFF, it matches with iatu0.
>>> And it is treated as an IO access by mistake, then sent to perpheral.
>>>
>>
>> Hummm...This portion of driver has always been buggy.
>>
>>> This patch fixes the problem by decoupling CFG and IO, reassigning iatu2 to IO.
>>
>> But, we can not just assign IOs to iatu2.
>> IIRC then, there are atleast two platforms which have only 2
>> viewports, therefore they can not program iatu2.
>>
>> Jingoo,Bjorn: IMHO, we should modify this portion of code, since more
>> number of platforms has 4+ viewports. Probably, we can take following
>> approach:
>>
>> (1) Pass number of viewports through DT. If we have *atleast* 3
>> viewports then assign separate viewports to memory and IO, and share
>> one with CFG0 and CFG1.
>> (2) If we can have *atleast* 4 then, we may have separate for CFG0
>> and CFG1 as well.
>>
>> (3) If we have *only* 2 then, either we let them work as they work
>> today with bug, or may be we restrict them from using IO transactions.
>> So assign one to memory and share other for CFG0 and CFG1.
>>
>> Please let me know your opnion.
>>
>> ~Pratyush
>>
>>>
>>> Signed-off-by: Dong Bo <dongbo4@...wei.com>
>>> ---
>>> drivers/pci/host/pcie-designware.c | 14 +++++++-------
>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>>> index aafd766..1a40305 100644
>>> --- a/drivers/pci/host/pcie-designware.c
>>> +++ b/drivers/pci/host/pcie-designware.c
>>> @@ -51,6 +51,7 @@
>>> #define PCIE_ATU_VIEWPORT 0x900
>>> #define PCIE_ATU_REGION_INBOUND (0x1 << 31)
>>> #define PCIE_ATU_REGION_OUTBOUND (0x0 << 31)
>>> +#define PCIE_ATU_REGION_INDEX2 (0x2 << 0)
>>> #define PCIE_ATU_REGION_INDEX1 (0x1 << 0)
>>> #define PCIE_ATU_REGION_INDEX0 (0x0 << 0)
>>> #define PCIE_ATU_CR1 0x904
>>> @@ -603,9 +604,6 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>>> type, cpu_addr,
>>> busdev, cfg_size);
>>> ret = dw_pcie_cfg_read(va_cfg_base + where, size, val);
>>> - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
>>> - PCIE_ATU_TYPE_IO, pp->io_base,
>>> - pp->io_bus_addr, pp->io_size);
>>>
>>> return ret;
>>> }
>>> @@ -640,9 +638,6 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>>> type, cpu_addr,
>>> busdev, cfg_size);
>>> ret = dw_pcie_cfg_write(va_cfg_base + where, size, val);
>>> - dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
>>> - PCIE_ATU_TYPE_IO, pp->io_base,
>>> - pp->io_bus_addr, pp->io_size);
>>>
>>> return ret;
>>> }
>>> @@ -778,10 +773,15 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>>> * uses its own address translation component rather than ATU, so
>>> * we should not program the ATU here.
>>> */
>>> - if (!pp->ops->rd_other_conf)
>>> + if (!pp->ops->rd_other_conf) {
>>> dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
>>> PCIE_ATU_TYPE_MEM, pp->mem_base,
>>> pp->mem_bus_addr, pp->mem_size);
>>> + dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX2,
>>> + PCIE_ATU_TYPE_IO, pp->io_base,
>>> + pp->io_bus_addr, pp->io_size);
>>> +
>>> + }
>>>
>>> dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
>>>
>>> --
>>> 1.9.1
>>>
>>
>> .
>>
>
Powered by blists - more mailing lists