2024-03-07 11:05:41

by Manivannan Sadhasivam

[permalink] [raw]
Subject: [PATCH] PCI: qcom: Enable BDF to SID translation properly

Qcom SoCs making use of ARM SMMU require BDF to SID translation table in
the driver to properly map the SID for the PCIe devices based on their BDF
identifier. This is currently achieved with the help of
qcom_pcie_config_sid_1_9_0() function for SoCs supporting the 1_9_0 config.

But With newer Qcom SoCs starting from SM8450, BDF to SID translation is
set to bypass mode by default in hardware. Due to this, the translation
table that is set in the qcom_pcie_config_sid_1_9_0() is essentially
unused and the default SID is used for all endpoints in SoCs starting from
SM8450.

This is a security concern and also warrants swapping the DeviceID in DT
while using the GIC ITS to handle MSIs from endpoints. The swapping is
currently done like below in DT when using GIC ITS:

/*
* MSIs for BDF (1:0.0) only works with Device ID 0x5980.
* Hence, the IDs are swapped.
*/
msi-map = <0x0 &gic_its 0x5981 0x1>,
<0x100 &gic_its 0x5980 0x1>;

Here, swapping of the DeviceIDs ensure that the endpoint with BDF (1:0.0)
gets the DeviceID 0x5980 which is associated with the default SID as per
the iommu mapping in DT. So MSIs were delivered with IDs swapped so far.
But this also means the Root Port (0:0.0) won't receive any MSIs (for PME,
AER etc...)

So let's fix these issues by clearing the BDF to SID bypass mode for all
SoCs making use of the 1_9_0 config. This allows the PCIe devices to use
the correct SID, thus avoiding the DeviceID swapping hack in DT and also
achieving the isolation between devices.

Cc: <[email protected]> # 5.11
Fixes: 4c9398822106 ("PCI: qcom: Add support for configuring BDF to SID mapping for SM8250")
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
I will send the DT patches to fix the msi-map entries once this patch gets
merged.
---
drivers/pci/controller/dwc/pcie-qcom.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 10f2d0bb86be..84e47c6f95fe 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -53,6 +53,7 @@
#define PARF_SLV_ADDR_SPACE_SIZE 0x358
#define PARF_DEVICE_TYPE 0x1000
#define PARF_BDF_TO_SID_TABLE_N 0x2000
+#define PARF_BDF_TO_SID_CFG 0x2c00

/* ELBI registers */
#define ELBI_SYS_CTRL 0x04
@@ -120,6 +121,9 @@
/* PARF_DEVICE_TYPE register fields */
#define DEVICE_TYPE_RC 0x4

+/* PARF_BDF_TO_SID_CFG fields */
+#define BDF_TO_SID_BYPASS BIT(0)
+
/* ELBI_SYS_CTRL register fields */
#define ELBI_SYS_CTRL_LT_ENABLE BIT(0)

@@ -1008,11 +1012,17 @@ static int qcom_pcie_config_sid_1_9_0(struct qcom_pcie *pcie)
u8 qcom_pcie_crc8_table[CRC8_TABLE_SIZE];
int i, nr_map, size = 0;
u32 smmu_sid_base;
+ u32 val;

of_get_property(dev->of_node, "iommu-map", &size);
if (!size)
return 0;

+ /* Enable BDF to SID translation by disabling bypass mode (default) */
+ val = readl(pcie->parf + PARF_BDF_TO_SID_CFG);
+ val &= ~BDF_TO_SID_BYPASS;
+ writel(val, pcie->parf + PARF_BDF_TO_SID_CFG);
+
map = kzalloc(size, GFP_KERNEL);
if (!map)
return -ENOMEM;

---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20240307-pci-bdf-sid-fix-c9cd8c0023d0

Best regards,
--
Manivannan Sadhasivam <[email protected]>



2024-03-07 11:21:14

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH] PCI: qcom: Enable BDF to SID translation properly



On 3/7/24 12:05, Manivannan Sadhasivam wrote:
> Qcom SoCs making use of ARM SMMU require BDF to SID translation table in
> the driver to properly map the SID for the PCIe devices based on their BDF
> identifier. This is currently achieved with the help of
> qcom_pcie_config_sid_1_9_0() function for SoCs supporting the 1_9_0 config.
>
> But With newer Qcom SoCs starting from SM8450, BDF to SID translation is
> set to bypass mode by default in hardware. Due to this, the translation
> table that is set in the qcom_pcie_config_sid_1_9_0() is essentially
> unused and the default SID is used for all endpoints in SoCs starting from
> SM8450.
>
> This is a security concern and also warrants swapping the DeviceID in DT
> while using the GIC ITS to handle MSIs from endpoints. The swapping is
> currently done like below in DT when using GIC ITS:
>
> /*
> * MSIs for BDF (1:0.0) only works with Device ID 0x5980.
> * Hence, the IDs are swapped.
> */
> msi-map = <0x0 &gic_its 0x5981 0x1>,
> <0x100 &gic_its 0x5980 0x1>;
>
> Here, swapping of the DeviceIDs ensure that the endpoint with BDF (1:0.0)
> gets the DeviceID 0x5980 which is associated with the default SID as per
> the iommu mapping in DT. So MSIs were delivered with IDs swapped so far.
> But this also means the Root Port (0:0.0) won't receive any MSIs (for PME,
> AER etc...)
>
> So let's fix these issues by clearing the BDF to SID bypass mode for all
> SoCs making use of the 1_9_0 config. This allows the PCIe devices to use
> the correct SID, thus avoiding the DeviceID swapping hack in DT and also
> achieving the isolation between devices.
>
> Cc: <[email protected]> # 5.11
> Fixes: 4c9398822106 ("PCI: qcom: Add support for configuring BDF to SID mapping for SM8250")
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---

Looks sensible..

Does switching away from bypass show any performance degradation?

Konrad

2024-03-07 11:52:27

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH] PCI: qcom: Enable BDF to SID translation properly

On Thu, Mar 07, 2024 at 12:19:56PM +0100, Konrad Dybcio wrote:
>
>
> On 3/7/24 12:05, Manivannan Sadhasivam wrote:
> > Qcom SoCs making use of ARM SMMU require BDF to SID translation table in
> > the driver to properly map the SID for the PCIe devices based on their BDF
> > identifier. This is currently achieved with the help of
> > qcom_pcie_config_sid_1_9_0() function for SoCs supporting the 1_9_0 config.
> >
> > But With newer Qcom SoCs starting from SM8450, BDF to SID translation is
> > set to bypass mode by default in hardware. Due to this, the translation
> > table that is set in the qcom_pcie_config_sid_1_9_0() is essentially
> > unused and the default SID is used for all endpoints in SoCs starting from
> > SM8450.
> >
> > This is a security concern and also warrants swapping the DeviceID in DT
> > while using the GIC ITS to handle MSIs from endpoints. The swapping is
> > currently done like below in DT when using GIC ITS:
> >
> > /*
> > * MSIs for BDF (1:0.0) only works with Device ID 0x5980.
> > * Hence, the IDs are swapped.
> > */
> > msi-map = <0x0 &gic_its 0x5981 0x1>,
> > <0x100 &gic_its 0x5980 0x1>;
> >
> > Here, swapping of the DeviceIDs ensure that the endpoint with BDF (1:0.0)
> > gets the DeviceID 0x5980 which is associated with the default SID as per
> > the iommu mapping in DT. So MSIs were delivered with IDs swapped so far.
> > But this also means the Root Port (0:0.0) won't receive any MSIs (for PME,
> > AER etc...)
> >
> > So let's fix these issues by clearing the BDF to SID bypass mode for all
> > SoCs making use of the 1_9_0 config. This allows the PCIe devices to use
> > the correct SID, thus avoiding the DeviceID swapping hack in DT and also
> > achieving the isolation between devices.
> >
> > Cc: <[email protected]> # 5.11
> > Fixes: 4c9398822106 ("PCI: qcom: Add support for configuring BDF to SID mapping for SM8250")
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > ---
>
> Looks sensible..
>
> Does switching away from bypass show any performance degradation?
>

Not at all with my throughput test on SM8450 dev board. But there shouldn't be
any performance related issue since we are just forcing the hw to use a
different SID (correct one) than the default one.

- Mani

--
மணிவண்ணன் சதாசிவம்

2024-03-10 17:45:58

by Krzysztof Wilczyński

[permalink] [raw]
Subject: Re: [PATCH] PCI: qcom: Enable BDF to SID translation properly

Hello,

> Qcom SoCs making use of ARM SMMU require BDF to SID translation table in
> the driver to properly map the SID for the PCIe devices based on their BDF
> identifier. This is currently achieved with the help of
> qcom_pcie_config_sid_1_9_0() function for SoCs supporting the 1_9_0 config.
>
> But With newer Qcom SoCs starting from SM8450, BDF to SID translation is
> set to bypass mode by default in hardware. Due to this, the translation
> table that is set in the qcom_pcie_config_sid_1_9_0() is essentially
> unused and the default SID is used for all endpoints in SoCs starting from
> SM8450.
>
> This is a security concern and also warrants swapping the DeviceID in DT
> while using the GIC ITS to handle MSIs from endpoints. The swapping is
> currently done like below in DT when using GIC ITS:
>
> /*
> * MSIs for BDF (1:0.0) only works with Device ID 0x5980.
> * Hence, the IDs are swapped.
> */
> msi-map = <0x0 &gic_its 0x5981 0x1>,
> <0x100 &gic_its 0x5980 0x1>;
>
> Here, swapping of the DeviceIDs ensure that the endpoint with BDF (1:0.0)
> gets the DeviceID 0x5980 which is associated with the default SID as per
> the iommu mapping in DT. So MSIs were delivered with IDs swapped so far.
> But this also means the Root Port (0:0.0) won't receive any MSIs (for PME,
> AER etc...)
>
> So let's fix these issues by clearing the BDF to SID bypass mode for all
> SoCs making use of the 1_9_0 config. This allows the PCIe devices to use
> the correct SID, thus avoiding the DeviceID swapping hack in DT and also
> achieving the isolation between devices.

Applied to controller/qcom, thank you!

[1/1] PCI: qcom: Enable BDF to SID translation properly
https://git.kernel.org/pci/pci/c/b9bc750e1193

Krzysztof