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

[Niklas Cassel <cassel@kernel.org>]

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.


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.


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.


Kind regards,
Niklas