Re: [PATCH 2/2] misc: pci_endpoint_test: Set reserved BARs for each SoCs

[Kunihiko Hayashi]

Hi Niklas,

Thank you for your comment.

On 2024/12/17 17:19, Niklas Cassel wrote:
> Hello Hayashisan,
>
> On Mon, Dec 16, 2024 at 04:39:41PM +0900, Kunihiko Hayashi wrote:
> > There are bar numbers that cannot be used on the endpoint.
> > So instead of SoC-specific conditions, add "reserved_bar" bar number
> > bitmap to the SoC data.
>
> I think that it was mistake to put is_am654_pci_dev() checks in
> pci_endpoint_test.c in the first place. However, let's not make the
> situation worse by introducing a reserved_bar bitmap on the host side as
> well.
>
> IMO, we should not have any logic for this the host side at all.

I also think that is_am654_pci_dev() isn't good solution, and I agree 
with you.

> Just like for am654, rk3588 has a BAR (BAR4) that should not be written by
> pci_endpoint_test.c (as it exposes the ATU registers in BAR4, so if the
> host writes this BAR, all address translation will be broken).
>
> An EPC driver can mark a BAR as reserved and that is exactly was rk3588
> does for BAR4:
> https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L300
>
> Marking a BAR as reserved means that an EPF driver should not touch that
> BAR at all.
>
> However, this by itself is not enough if the BAR is enabled by default,
> in that case we also need to disable the BAR for the host side to not
> be able to write to it:
> https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L248-L249
>
>
> If we look at am654, we can see that it does set BAR0 and BAR1 as reserved:
> https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pci-keystone.c#L967-L968
>
> The problem is that am654 does not also disable these BARs by default.
>
>
> If you look at most DWC based EPC drivers:
> drivers/pci/controller/dwc/pci-dra7xx.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pci-imx6.c:          dw_pcie_ep_reset_bar(pci,
> bar);
> drivers/pci/controller/dwc/pci-layerscape-ep.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-artpec6.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-designware-plat.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-dw-rockchip.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-qcom-ep.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-rcar-gen4.c:
> dw_pcie_ep_reset_bar(pci, bar);
> drivers/pci/controller/dwc/pcie-uniphier-ep.c:
> dw_pcie_ep_reset_bar(pci, bar);
>
> They call dw_pcie_ep_reset_bar() in EP init for all BARs.
> (An EPF driver will be able to re-enable all BARs that are not marked as
> reserved.)
>
> am654 seems to be the exception here.

The endpoint driver including am654 should surely call 
dw_pcie_ep_reset_bar() to initialize BARs at first.

> If you simply add code that disables all BARs by default in am654, you
> should be able to remove these ugly is_am654_pci_dev() checks in the host
> driver, and the host driver should not be able to write to these reserved
> BARs, as they will never get enabled by pci-epf-test.c.

However, dw_pcie_ep_reset_bar() only clears BAR registers to 0x0. BAR 
doesn't have any "disabled" field, so I think that it means "32-bit, 
memory, non-prefetchable".


https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L47-L52

And even if each endpoint driver marks "BAR_RESERVED" to the features, 
it is only referred to as excluded BARs when searching for free BARs. So 
the host will recognize this "reserved" BAR.


https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/endpoint/pci-epc-core.c#L123-L124

I've not actually used am654, but for example, when I tried to execute 
BAR test on the other SoC that has inaccessible BAR (marked as 
"reserved"), AER reported an uncorrectable bus error.
This behavior depends on the bus connection to the BAR.

It seems that access to the BAR is unavoidable even if calling 
dw_pci_ep_reset_bar() and marking as BAR_RESERVED.

But I don't have any other idea to avoid access to reserved BARs,
so this patch [2/2] will be withdrawn.

Thank you,

---
Best Regards
Kunihiko Hayashi