| Message ID | 20241216073941.2572407-2-hayashi.kunihiko@socionext.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | [1/2] misc: pci_endpoint_test: Fix irq_type to convey the correct type | expand |
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
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
On Thu, Dec 19, 2024 at 08:17:50PM +0900, Kunihiko Hayashi wrote: > On 2024/12/17 17:19, Niklas Cassel wrote: > > > 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". From the DWC databook, 4.60a, section "6.1.2 BAR Details", heading "Disabling a BAR". "To disable a BAR (in any of the three schemes), your application can write ‘0’ to the LSB of the BAR mask register." dw_pcie_ep_reset_bar() calls __dw_pcie_ep_reset_bar(), which will write a zero to the LSB of the BAR mask register: https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L50 > > > 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. A BAR that has been disabled on the EP side, will not have a size/ be visible on host side. Like I said, rk3588 calls dw_pcie_ep_reset_bar() on all BARs in EP init, like most DWC based EPC drivers, and marks BAR4 as reserved. This is how it looks on the host side during enumeration: [ 25.496645] pci 0000:01:00.0: [1d87:3588] type 00 class 0xff0000 PCIe Endpoint [ 25.497322] pci 0000:01:00.0: BAR 0 [mem 0x00000000-0x000fffff] [ 25.497863] pci 0000:01:00.0: BAR 1 [mem 0x00000000-0x000fffff] [ 25.498400] pci 0000:01:00.0: BAR 2 [mem 0x00000000-0x000fffff] [ 25.498937] pci 0000:01:00.0: BAR 3 [mem 0x00000000-0x000fffff] [ 25.499498] pci 0000:01:00.0: BAR 5 [mem 0x00000000-0x000fffff] [ 25.500036] pci 0000:01:00.0: ROM [mem 0x00000000-0x0000ffff pref] [ 25.500861] pci 0000:01:00.0: supports D1 D2 [ 25.501240] pci 0000:01:00.0: PME# supported from D0 D1 D3hot Likewise the looping in pci_endpoint_test.c will skip disabled BARs, e.g.: https://github.com/torvalds/linux/blob/v6.13-rc3/drivers/misc/pci_endpoint_test.c#L940-L943 Since test->bar[bar] will be NULL for BARs that are disabled. Kind regards, Niklas
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 854480921470..e6a1a2916425 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -76,9 +76,6 @@ #define PCI_DEVICE_ID_LS1088A 0x80c0 #define PCI_DEVICE_ID_IMX8 0x0808 -#define is_am654_pci_dev(pdev) \ - ((pdev)->device == PCI_DEVICE_ID_TI_AM654) - #define PCI_DEVICE_ID_RENESAS_R8A774A1 0x0028 #define PCI_DEVICE_ID_RENESAS_R8A774B1 0x002b #define PCI_DEVICE_ID_RENESAS_R8A774C0 0x002d @@ -123,6 +120,7 @@ struct pci_endpoint_test { struct miscdevice miscdev; enum pci_barno test_reg_bar; size_t alignment; + u32 reserved_bar; const char *name; }; @@ -130,6 +128,7 @@ struct pci_endpoint_test_data { enum pci_barno test_reg_bar; size_t alignment; int irq_type; + u32 reserved_bar; }; static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test, @@ -753,7 +752,6 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, int ret = -EINVAL; enum pci_barno bar; struct pci_endpoint_test *test = to_endpoint_test(file->private_data); - struct pci_dev *pdev = test->pdev; mutex_lock(&test->mutex); @@ -765,7 +763,7 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, bar = arg; if (bar > BAR_5) goto ret; - if (is_am654_pci_dev(pdev) && bar == BAR_0) + if (BIT(bar) & test->reserved_bar) goto ret; ret = pci_endpoint_test_bar(test, bar); break; @@ -840,6 +838,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, test_reg_bar = data->test_reg_bar; test->test_reg_bar = test_reg_bar; test->alignment = data->alignment; + test->reserved_bar = data->reserved_bar; irq_type = data->irq_type; } @@ -991,6 +990,7 @@ static const struct pci_endpoint_test_data am654_data = { .test_reg_bar = BAR_2, .alignment = SZ_64K, .irq_type = IRQ_TYPE_MSI, + .reserved_bar = BIT(BAR_0), }; static const struct pci_endpoint_test_data j721e_data = {
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. Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> --- drivers/misc/pci_endpoint_test.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)