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
