2019-12-02 10:50:05

by Bao Xiaowei

Subject: [PATCH] PCI: layerscape: Add the SRIOV support in host side

GIC get the map relations of devid and stream id from the msi-map
property of DTS, our platform add this property in u-boot base on
the PCIe device in the bus, but if enable the vf device in kernel,
the vf device msi-map will not set, so the vf device can't work,
this patch purpose is that manage the stream id and device id map
relations dynamically in kernel, and make the new PCIe device work
in kernel.

Signed-off-by: Xiaowei Bao <[email protected]>
---
drivers/of/irq.c | 9 +++
drivers/pci/controller/dwc/pci-layerscape.c | 94 +++++++++++++++++++++++++++++
drivers/pci/probe.c | 6 ++
drivers/pci/remove.c | 6 ++
4 files changed, 115 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index a296eaf..791e609 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -576,6 +576,11 @@ void __init of_irq_init(const struct of_device_id *matches)
}
}

+u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid)
+{
+ return rid;
+}
+
static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
u32 rid_in)
{
@@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
"msi-map-mask", np, &rid_out))
break;
+
+ if (rid_out == rid_in)
+ rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
+
return rid_out;
}

diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
index f24f79a..c1b3675 100644
--- a/drivers/pci/controller/dwc/pci-layerscape.c
+++ b/drivers/pci/controller/dwc/pci-layerscape.c
@@ -22,6 +22,8 @@

#include "pcie-designware.h"

+#define FSL_PEX_STREAM_ID_START 7
+#define FSL_PEX_STREAM_ID_END 22
/* PEX1/2 Misc Ports Status Register */
#define SCFG_PEXMSCPORTSR(pex_idx) (0x94 + (pex_idx) * 4)
#define LTSSM_STATE_SHIFT 20
@@ -33,8 +35,16 @@
#define PCIE_ABSERR 0x8d0 /* Bridge Slave Error Response Register */
#define PCIE_ABSERR_SETTING 0x9401 /* Forward error of non-posted request */

+/* LUT registers */
+#define PCIE_LUT_UDR(n) (0x800 + (n) * 8)
+#define PCIE_LUT_LDR(n) (0x804 + (n) * 8)
+#define PCIE_LUT_ENABLE (1 << 31)
+#define PCIE_LUT_ENTRY_COUNT 32
+
#define PCIE_IATU_NUM 6

+unsigned long *stream_id_map;
+
struct ls_pcie_drvdata {
u32 lut_offset;
u32 ltssm_shift;
@@ -49,6 +59,7 @@ struct ls_pcie {
struct regmap *scfg;
const struct ls_pcie_drvdata *drvdata;
int index;
+ unsigned long *lut_reg_map;
};

#define to_ls_pcie(x) dev_get_drvdata((x)->dev)
@@ -291,6 +302,77 @@ static int __init ls_add_pcie_port(struct ls_pcie *pcie)
return 0;
}

+u32 ls_pcie_streamid_fix(struct device *dev, u32 rid)
+{
+ u32 lut_idx, streamid, val;
+ struct platform_device *pdev = to_platform_device(dev);
+ struct ls_pcie *pcie = platform_get_drvdata(pdev);
+
+ for (lut_idx = 0; lut_idx < PCIE_LUT_ENTRY_COUNT; lut_idx++) {
+ val = ioread32(pcie->lut + PCIE_LUT_UDR(lut_idx)) >> 16;
+ if (val == rid) {
+ streamid = ioread32(pcie->lut + PCIE_LUT_LDR(lut_idx)) &
+ (~PCIE_LUT_ENABLE);
+ break;
+ }
+ }
+
+ return streamid;
+}
+
+void ls_pcie_remove_streamid(struct pci_bus *bus, struct pci_dev *pdev)
+{
+ struct pcie_port *pp = bus->sysdata;
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct ls_pcie *pcie = to_ls_pcie(pci);
+ u32 lut_idx, streamid, rid, val;
+
+ rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
+
+ for (lut_idx = 0; lut_idx < PCIE_LUT_ENTRY_COUNT; lut_idx++) {
+ val = ioread32(pcie->lut + PCIE_LUT_UDR(lut_idx)) >> 16;
+ if (val == rid) {
+ streamid = ioread32(pcie->lut + PCIE_LUT_LDR(lut_idx)) &
+ (~PCIE_LUT_ENABLE);
+ break;
+ }
+ }
+
+ if (lut_idx >= PCIE_LUT_ENTRY_COUNT) {
+ pr_err("Don't find the streamid relate to the rid !\n");
+ return;
+ }
+
+ iowrite32(0, pcie->lut + PCIE_LUT_UDR(lut_idx));
+ iowrite32(0, pcie->lut + PCIE_LUT_LDR(lut_idx));
+
+ clear_bit(streamid, stream_id_map);
+ clear_bit(lut_idx, pcie->lut_reg_map);
+}
+
+void ls_pcie_add_streamid(struct pci_bus *bus, struct pci_dev *pdev)
+{
+ u32 free_lut, free_streamid, rid;
+ struct pcie_port *pp = bus->sysdata;
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct ls_pcie *pcie = to_ls_pcie(pci);
+
+ rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
+
+ free_streamid = find_first_zero_bit(stream_id_map,
+ FSL_PEX_STREAM_ID_END);
+
+ free_lut = find_first_zero_bit(pcie->lut_reg_map,
+ PCIE_LUT_ENTRY_COUNT);
+
+ iowrite32(rid << 16, pcie->lut + PCIE_LUT_UDR(free_lut));
+ iowrite32(free_streamid | PCIE_LUT_ENABLE,
+ pcie->lut + PCIE_LUT_LDR(free_lut));
+
+ set_bit(free_streamid, stream_id_map);
+ set_bit(free_lut, pcie->lut_reg_map);
+}
+
static int __init ls_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -298,6 +380,7 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
struct ls_pcie *pcie;
struct resource *dbi_base;
int ret;
+ u32 id;

pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
if (!pcie)
@@ -324,6 +407,17 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
if (!ls_pcie_is_bridge(pcie))
return -ENODEV;

+ stream_id_map = devm_kcalloc(dev,
+ BITS_TO_LONGS(FSL_PEX_STREAM_ID_END),
+ sizeof(long), GFP_KERNEL);
+
+ for (id = 0; id < FSL_PEX_STREAM_ID_START; id++)
+ set_bit(id, stream_id_map);
+
+ pcie->lut_reg_map = devm_kcalloc(dev,
+ BITS_TO_LONGS(PCIE_LUT_ENTRY_COUNT),
+ sizeof(long), GFP_KERNEL);
+
platform_set_drvdata(pdev, pcie);

ret = ls_add_pcie_port(pcie);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 512cb43..d4729b4 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2380,6 +2380,10 @@ static void pci_set_msi_domain(struct pci_dev *dev)
dev_set_msi_domain(&dev->dev, d);
}

+void __weak ls_pcie_add_streamid(struct pci_bus *bus, struct pci_dev *pdev)
+{
+}
+
void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
{
int ret;
@@ -2417,6 +2421,8 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
ret = pcibios_add_device(dev);
WARN_ON(ret < 0);

+ ls_pcie_add_streamid(bus, dev);
+
/* Set up MSI IRQ domain */
pci_set_msi_domain(dev);

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index e9c6b12..af6cc7f 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -62,11 +62,17 @@ void pci_remove_bus(struct pci_bus *bus)
}
EXPORT_SYMBOL(pci_remove_bus);

+void __weak ls_pcie_remove_streamid(struct pci_bus *bus, struct pci_dev *pdev)
+{
+}
+
static void pci_stop_bus_device(struct pci_dev *dev)
{
struct pci_bus *bus = dev->subordinate;
struct pci_dev *child, *tmp;

+ ls_pcie_remove_streamid(dev->bus, dev);
+
/*
* Stopping an SR-IOV PF device removes all the associated VFs,
* which will update the bus->devices list and confuse the
--
2.9.5

2019-12-02 11:06:26

by Lorenzo Pieralisi

Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side

On Mon, Dec 02, 2019 at 06:45:06PM +0800, Xiaowei Bao wrote:
> GIC get the map relations of devid and stream id from the msi-map
> property of DTS, our platform add this property in u-boot base on
> the PCIe device in the bus, but if enable the vf device in kernel,
> the vf device msi-map will not set, so the vf device can't work,
> this patch purpose is that manage the stream id and device id map
> relations dynamically in kernel, and make the new PCIe device work
> in kernel.
>
> Signed-off-by: Xiaowei Bao <[email protected]>
> ---
> drivers/of/irq.c | 9 +++
> drivers/pci/controller/dwc/pci-layerscape.c | 94 +++++++++++++++++++++++++++++
> drivers/pci/probe.c | 6 ++
> drivers/pci/remove.c | 6 ++
> 4 files changed, 115 insertions(+)

NAK

If u-boot is broken it should be fixed, we won't apply core PCI/OF
changes to paper over it.

Thanks,
Lorenzo

> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index a296eaf..791e609 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -576,6 +576,11 @@ void __init of_irq_init(const struct of_device_id *matches)
> }
> }
>
> +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid)
> +{
> + return rid;
> +}
> +
> static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
> u32 rid_in)
> {
> @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
> if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
> "msi-map-mask", np, &rid_out))
> break;
> +
> + if (rid_out == rid_in)
> + rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
> +
> return rid_out;
> }
>
> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c b/drivers/pci/controller/dwc/pci-layerscape.c
> index f24f79a..c1b3675 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> @@ -22,6 +22,8 @@
>
> #include "pcie-designware.h"
>
> +#define FSL_PEX_STREAM_ID_START 7
> +#define FSL_PEX_STREAM_ID_END 22
> /* PEX1/2 Misc Ports Status Register */
> #define SCFG_PEXMSCPORTSR(pex_idx) (0x94 + (pex_idx) * 4)
> #define LTSSM_STATE_SHIFT 20
> @@ -33,8 +35,16 @@
> #define PCIE_ABSERR 0x8d0 /* Bridge Slave Error Response Register */
> #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of non-posted request */
>
> +/* LUT registers */
> +#define PCIE_LUT_UDR(n) (0x800 + (n) * 8)
> +#define PCIE_LUT_LDR(n) (0x804 + (n) * 8)
> +#define PCIE_LUT_ENABLE (1 << 31)
> +#define PCIE_LUT_ENTRY_COUNT 32
> +
> #define PCIE_IATU_NUM 6
>
> +unsigned long *stream_id_map;
> +
> struct ls_pcie_drvdata {
> u32 lut_offset;
> u32 ltssm_shift;
> @@ -49,6 +59,7 @@ struct ls_pcie {
> struct regmap *scfg;
> const struct ls_pcie_drvdata *drvdata;
> int index;
> + unsigned long *lut_reg_map;
> };
>
> #define to_ls_pcie(x) dev_get_drvdata((x)->dev)
> @@ -291,6 +302,77 @@ static int __init ls_add_pcie_port(struct ls_pcie *pcie)
> return 0;
> }
>
> +u32 ls_pcie_streamid_fix(struct device *dev, u32 rid)
> +{
> + u32 lut_idx, streamid, val;
> + struct platform_device *pdev = to_platform_device(dev);
> + struct ls_pcie *pcie = platform_get_drvdata(pdev);
> +
> + for (lut_idx = 0; lut_idx < PCIE_LUT_ENTRY_COUNT; lut_idx++) {
> + val = ioread32(pcie->lut + PCIE_LUT_UDR(lut_idx)) >> 16;
> + if (val == rid) {
> + streamid = ioread32(pcie->lut + PCIE_LUT_LDR(lut_idx)) &
> + (~PCIE_LUT_ENABLE);
> + break;
> + }
> + }
> +
> + return streamid;
> +}
> +
> +void ls_pcie_remove_streamid(struct pci_bus *bus, struct pci_dev *pdev)
> +{
> + struct pcie_port *pp = bus->sysdata;
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct ls_pcie *pcie = to_ls_pcie(pci);
> + u32 lut_idx, streamid, rid, val;
> +
> + rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
> +
> + for (lut_idx = 0; lut_idx < PCIE_LUT_ENTRY_COUNT; lut_idx++) {
> + val = ioread32(pcie->lut + PCIE_LUT_UDR(lut_idx)) >> 16;
> + if (val == rid) {
> + streamid = ioread32(pcie->lut + PCIE_LUT_LDR(lut_idx)) &
> + (~PCIE_LUT_ENABLE);
> + break;
> + }
> + }
> +
> + if (lut_idx >= PCIE_LUT_ENTRY_COUNT) {
> + pr_err("Don't find the streamid relate to the rid !\n");
> + return;
> + }
> +
> + iowrite32(0, pcie->lut + PCIE_LUT_UDR(lut_idx));
> + iowrite32(0, pcie->lut + PCIE_LUT_LDR(lut_idx));
> +
> + clear_bit(streamid, stream_id_map);
> + clear_bit(lut_idx, pcie->lut_reg_map);
> +}
> +
> +void ls_pcie_add_streamid(struct pci_bus *bus, struct pci_dev *pdev)
> +{
> + u32 free_lut, free_streamid, rid;
> + struct pcie_port *pp = bus->sysdata;
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct ls_pcie *pcie = to_ls_pcie(pci);
> +
> + rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
> +
> + free_streamid = find_first_zero_bit(stream_id_map,
> + FSL_PEX_STREAM_ID_END);
> +
> + free_lut = find_first_zero_bit(pcie->lut_reg_map,
> + PCIE_LUT_ENTRY_COUNT);
> +
> + iowrite32(rid << 16, pcie->lut + PCIE_LUT_UDR(free_lut));
> + iowrite32(free_streamid | PCIE_LUT_ENABLE,
> + pcie->lut + PCIE_LUT_LDR(free_lut));
> +
> + set_bit(free_streamid, stream_id_map);
> + set_bit(free_lut, pcie->lut_reg_map);
> +}
> +
> static int __init ls_pcie_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -298,6 +380,7 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
> struct ls_pcie *pcie;
> struct resource *dbi_base;
> int ret;
> + u32 id;
>
> pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> if (!pcie)
> @@ -324,6 +407,17 @@ static int __init ls_pcie_probe(struct platform_device *pdev)
> if (!ls_pcie_is_bridge(pcie))
> return -ENODEV;
>
> + stream_id_map = devm_kcalloc(dev,
> + BITS_TO_LONGS(FSL_PEX_STREAM_ID_END),
> + sizeof(long), GFP_KERNEL);
> +
> + for (id = 0; id < FSL_PEX_STREAM_ID_START; id++)
> + set_bit(id, stream_id_map);
> +
> + pcie->lut_reg_map = devm_kcalloc(dev,
> + BITS_TO_LONGS(PCIE_LUT_ENTRY_COUNT),
> + sizeof(long), GFP_KERNEL);
> +
> platform_set_drvdata(pdev, pcie);
>
> ret = ls_add_pcie_port(pcie);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 512cb43..d4729b4 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2380,6 +2380,10 @@ static void pci_set_msi_domain(struct pci_dev *dev)
> dev_set_msi_domain(&dev->dev, d);
> }
>
> +void __weak ls_pcie_add_streamid(struct pci_bus *bus, struct pci_dev *pdev)
> +{
> +}
> +
> void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> {
> int ret;
> @@ -2417,6 +2421,8 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> ret = pcibios_add_device(dev);
> WARN_ON(ret < 0);
>
> + ls_pcie_add_streamid(bus, dev);
> +
> /* Set up MSI IRQ domain */
> pci_set_msi_domain(dev);
>
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index e9c6b12..af6cc7f 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -62,11 +62,17 @@ void pci_remove_bus(struct pci_bus *bus)
> }
> EXPORT_SYMBOL(pci_remove_bus);
>
> +void __weak ls_pcie_remove_streamid(struct pci_bus *bus, struct pci_dev *pdev)
> +{
> +}
> +
> static void pci_stop_bus_device(struct pci_dev *dev)
> {
> struct pci_bus *bus = dev->subordinate;
> struct pci_dev *child, *tmp;
>
> + ls_pcie_remove_streamid(dev->bus, dev);
> +
> /*
> * Stopping an SR-IOV PF device removes all the associated VFs,
> * which will update the bus->devices list and confuse the
> --
> 2.9.5
>

2019-12-02 12:49:20

by Marc Zyngier

Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side

On 2019-12-02 10:45, Xiaowei Bao wrote:
> GIC get the map relations of devid and stream id from the msi-map
> property of DTS, our platform add this property in u-boot base on
> the PCIe device in the bus, but if enable the vf device in kernel,
> the vf device msi-map will not set, so the vf device can't work,
> this patch purpose is that manage the stream id and device id map
> relations dynamically in kernel, and make the new PCIe device work
> in kernel.
>
> Signed-off-by: Xiaowei Bao <[email protected]>
> ---
> drivers/of/irq.c | 9 +++
> drivers/pci/controller/dwc/pci-layerscape.c | 94
> +++++++++++++++++++++++++++++
> drivers/pci/probe.c | 6 ++
> drivers/pci/remove.c | 6 ++
> 4 files changed, 115 insertions(+)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index a296eaf..791e609 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -576,6 +576,11 @@ void __init of_irq_init(const struct
> of_device_id *matches)
> }
> }
>
> +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid)
> +{
> + return rid;
> +}
> +
> static u32 __of_msi_map_rid(struct device *dev, struct device_node
> **np,
> u32 rid_in)
> {
> @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device *dev,
> struct device_node **np,
> if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
> "msi-map-mask", np, &rid_out))
> break;
> +
> + if (rid_out == rid_in)
> + rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);

Over my dead body. Get your firmware to properly program the LUT
so that it presents the ITS with a reasonable topology. There is
absolutely no way this kind of change makes it into the kernel.

Thanks,

M.
--
Who you jivin' with that Cosmik Debris?

2019-12-03 01:29:29

by Bao Xiaowei

Subject: RE: [PATCH] PCI: layerscape: Add the SRIOV support in host side

> -----Original Message-----
> From: Lorenzo Pieralisi <[email protected]>
> Sent: 2019??12??2?? 19:02
> To: Xiaowei Bao <[email protected]>
> Cc: [email protected]; [email protected]; M.h. Lian
> <[email protected]>; Mingkai Hu <[email protected]>; Roy Zang
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Z.q. Hou
> <[email protected]>
> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side
>
> On Mon, Dec 02, 2019 at 06:45:06PM +0800, Xiaowei Bao wrote:
> > GIC get the map relations of devid and stream id from the msi-map
> > property of DTS, our platform add this property in u-boot base on the
> > PCIe device in the bus, but if enable the vf device in kernel, the vf
> > device msi-map will not set, so the vf device can't work, this patch
> > purpose is that manage the stream id and device id map relations
> > dynamically in kernel, and make the new PCIe device work in kernel.
> >
> > Signed-off-by: Xiaowei Bao <[email protected]>
> > ---
> > drivers/of/irq.c | 9 +++
> > drivers/pci/controller/dwc/pci-layerscape.c | 94
> +++++++++++++++++++++++++++++
> > drivers/pci/probe.c | 6 ++
> > drivers/pci/remove.c | 6 ++
> > 4 files changed, 115 insertions(+)
>
> NAK
>
> If u-boot is broken it should be fixed, we won't apply core PCI/OF changes to
> paper over it.

Thanks for your comments, I know this patch is not reasonable, but I have no other way,
I think all ARM platform have this issue if it get the stream ID from msi-map property of
DTS, do you have the best advice to make it reasonable, or do you know how other arm
vendors implement the mapping of stream id to request id? If all arm vendors use the
msi-map method, I do not think they can support the SRIOV VF function on the host side.
e.g. x710 enable VF:
echo 1 > /sys/class/net/eth0/device/sriov_numvfs
echo 1 > /sys/class/net/eth1/device/sriov_numvfs
Running this command is equivalent to dynamically adding a new PCIe device. The msi-map
will definitely not exist in DTS, so the VF device of x710 cannot work.

Thanks
Xiaowei

>
> Thanks,
> Lorenzo
>
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
> > a296eaf..791e609 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -576,6 +576,11 @@ void __init of_irq_init(const struct of_device_id
> *matches)
> > }
> > }
> >
> > +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid) {
> > + return rid;
> > +}
> > +
> > static u32 __of_msi_map_rid(struct device *dev, struct device_node
> **np,
> > u32 rid_in)
> > {
> > @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device *dev,
> struct device_node **np,
> > if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
> > "msi-map-mask", np, &rid_out))
> > break;
> > +
> > + if (rid_out == rid_in)
> > + rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
> > +
> > return rid_out;
> > }
> >
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
> > b/drivers/pci/controller/dwc/pci-layerscape.c
> > index f24f79a..c1b3675 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> > @@ -22,6 +22,8 @@
> >
> > #include "pcie-designware.h"
> >
> > +#define FSL_PEX_STREAM_ID_START 7
> > +#define FSL_PEX_STREAM_ID_END 22
> > /* PEX1/2 Misc Ports Status Register */
> > #define SCFG_PEXMSCPORTSR(pex_idx) (0x94 + (pex_idx) * 4)
> > #define LTSSM_STATE_SHIFT 20
> > @@ -33,8 +35,16 @@
> > #define PCIE_ABSERR 0x8d0 /* Bridge Slave Error Response
> Register */
> > #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of non-posted
> request */
> >
> > +/* LUT registers */
> > +#define PCIE_LUT_UDR(n) (0x800 + (n) * 8)
> > +#define PCIE_LUT_LDR(n) (0x804 + (n) * 8)
> > +#define PCIE_LUT_ENABLE (1 << 31)
> > +#define PCIE_LUT_ENTRY_COUNT 32
> > +
> > #define PCIE_IATU_NUM 6
> >
> > +unsigned long *stream_id_map;
> > +
> > struct ls_pcie_drvdata {
> > u32 lut_offset;
> > u32 ltssm_shift;
> > @@ -49,6 +59,7 @@ struct ls_pcie {
> > struct regmap *scfg;
> > const struct ls_pcie_drvdata *drvdata;
> > int index;
> > + unsigned long *lut_reg_map;
> > };
> >
> > #define to_ls_pcie(x) dev_get_drvdata((x)->dev)
> > @@ -291,6 +302,77 @@ static int __init ls_add_pcie_port(struct ls_pcie
> *pcie)
> > return 0;
> > }
> >
> > +u32 ls_pcie_streamid_fix(struct device *dev, u32 rid) {
> > + u32 lut_idx, streamid, val;
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct ls_pcie *pcie = platform_get_drvdata(pdev);
> > +
> > + for (lut_idx = 0; lut_idx < PCIE_LUT_ENTRY_COUNT; lut_idx++) {
> > + val = ioread32(pcie->lut + PCIE_LUT_UDR(lut_idx)) >> 16;
> > + if (val == rid) {
> > + streamid = ioread32(pcie->lut + PCIE_LUT_LDR(lut_idx)) &
> > + (~PCIE_LUT_ENABLE);
> > + break;
> > + }
> > + }
> > +
> > + return streamid;
> > +}
> > +
> > +void ls_pcie_remove_streamid(struct pci_bus *bus, struct pci_dev
> > +*pdev) {
> > + struct pcie_port *pp = bus->sysdata;
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct ls_pcie *pcie = to_ls_pcie(pci);
> > + u32 lut_idx, streamid, rid, val;
> > +
> > + rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
> > +
> > + for (lut_idx = 0; lut_idx < PCIE_LUT_ENTRY_COUNT; lut_idx++) {
> > + val = ioread32(pcie->lut + PCIE_LUT_UDR(lut_idx)) >> 16;
> > + if (val == rid) {
> > + streamid = ioread32(pcie->lut + PCIE_LUT_LDR(lut_idx)) &
> > + (~PCIE_LUT_ENABLE);
> > + break;
> > + }
> > + }
> > +
> > + if (lut_idx >= PCIE_LUT_ENTRY_COUNT) {
> > + pr_err("Don't find the streamid relate to the rid !\n");
> > + return;
> > + }
> > +
> > + iowrite32(0, pcie->lut + PCIE_LUT_UDR(lut_idx));
> > + iowrite32(0, pcie->lut + PCIE_LUT_LDR(lut_idx));
> > +
> > + clear_bit(streamid, stream_id_map);
> > + clear_bit(lut_idx, pcie->lut_reg_map); }
> > +
> > +void ls_pcie_add_streamid(struct pci_bus *bus, struct pci_dev *pdev)
> > +{
> > + u32 free_lut, free_streamid, rid;
> > + struct pcie_port *pp = bus->sysdata;
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct ls_pcie *pcie = to_ls_pcie(pci);
> > +
> > + rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
> > +
> > + free_streamid = find_first_zero_bit(stream_id_map,
> > + FSL_PEX_STREAM_ID_END);
> > +
> > + free_lut = find_first_zero_bit(pcie->lut_reg_map,
> > + PCIE_LUT_ENTRY_COUNT);
> > +
> > + iowrite32(rid << 16, pcie->lut + PCIE_LUT_UDR(free_lut));
> > + iowrite32(free_streamid | PCIE_LUT_ENABLE,
> > + pcie->lut + PCIE_LUT_LDR(free_lut));
> > +
> > + set_bit(free_streamid, stream_id_map);
> > + set_bit(free_lut, pcie->lut_reg_map); }
> > +
> > static int __init ls_pcie_probe(struct platform_device *pdev) {
> > struct device *dev = &pdev->dev;
> > @@ -298,6 +380,7 @@ static int __init ls_pcie_probe(struct
> platform_device *pdev)
> > struct ls_pcie *pcie;
> > struct resource *dbi_base;
> > int ret;
> > + u32 id;
> >
> > pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > if (!pcie)
> > @@ -324,6 +407,17 @@ static int __init ls_pcie_probe(struct
> platform_device *pdev)
> > if (!ls_pcie_is_bridge(pcie))
> > return -ENODEV;
> >
> > + stream_id_map = devm_kcalloc(dev,
> > + BITS_TO_LONGS(FSL_PEX_STREAM_ID_END),
> > + sizeof(long), GFP_KERNEL);
> > +
> > + for (id = 0; id < FSL_PEX_STREAM_ID_START; id++)
> > + set_bit(id, stream_id_map);
> > +
> > + pcie->lut_reg_map = devm_kcalloc(dev,
> > + BITS_TO_LONGS(PCIE_LUT_ENTRY_COUNT),
> > + sizeof(long), GFP_KERNEL);
> > +
> > platform_set_drvdata(pdev, pcie);
> >
> > ret = ls_add_pcie_port(pcie);
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index
> > 512cb43..d4729b4 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2380,6 +2380,10 @@ static void pci_set_msi_domain(struct pci_dev
> *dev)
> > dev_set_msi_domain(&dev->dev, d);
> > }
> >
> > +void __weak ls_pcie_add_streamid(struct pci_bus *bus, struct pci_dev
> > +*pdev) { }
> > +
> > void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) {
> > int ret;
> > @@ -2417,6 +2421,8 @@ void pci_device_add(struct pci_dev *dev, struct
> pci_bus *bus)
> > ret = pcibios_add_device(dev);
> > WARN_ON(ret < 0);
> >
> > + ls_pcie_add_streamid(bus, dev);
> > +
> > /* Set up MSI IRQ domain */
> > pci_set_msi_domain(dev);
> >
> > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index
> > e9c6b12..af6cc7f 100644
> > --- a/drivers/pci/remove.c
> > +++ b/drivers/pci/remove.c
> > @@ -62,11 +62,17 @@ void pci_remove_bus(struct pci_bus *bus) }
> > EXPORT_SYMBOL(pci_remove_bus);
> >
> > +void __weak ls_pcie_remove_streamid(struct pci_bus *bus, struct
> > +pci_dev *pdev) { }
> > +
> > static void pci_stop_bus_device(struct pci_dev *dev) {
> > struct pci_bus *bus = dev->subordinate;
> > struct pci_dev *child, *tmp;
> >
> > + ls_pcie_remove_streamid(dev->bus, dev);
> > +
> > /*
> > * Stopping an SR-IOV PF device removes all the associated VFs,
> > * which will update the bus->devices list and confuse the
> > --
> > 2.9.5
> >

2019-12-03 01:43:21

by Bao Xiaowei

Subject: RE: [PATCH] PCI: layerscape: Add the SRIOV support in host side

> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: 2019年12月2日 20:48
> To: Xiaowei Bao <[email protected]>
> Cc: [email protected]; [email protected]; M.h. Lian
> <[email protected]>; Mingkai Hu <[email protected]>; Roy Zang
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Z.q. Hou <[email protected]>
> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side
>
> On 2019-12-02 10:45, Xiaowei Bao wrote:
> > GIC get the map relations of devid and stream id from the msi-map
> > property of DTS, our platform add this property in u-boot base on the
> > PCIe device in the bus, but if enable the vf device in kernel, the vf
> > device msi-map will not set, so the vf device can't work, this patch
> > purpose is that manage the stream id and device id map relations
> > dynamically in kernel, and make the new PCIe device work in kernel.
> >
> > Signed-off-by: Xiaowei Bao <[email protected]>
> > ---
> > drivers/of/irq.c | 9 +++
> > drivers/pci/controller/dwc/pci-layerscape.c | 94
> > +++++++++++++++++++++++++++++
> > drivers/pci/probe.c | 6 ++
> > drivers/pci/remove.c | 6 ++
> > 4 files changed, 115 insertions(+)
> >
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
> > a296eaf..791e609 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -576,6 +576,11 @@ void __init of_irq_init(const struct of_device_id
> > *matches)
> > }
> > }
> >
> > +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid) {
> > + return rid;
> > +}
> > +
> > static u32 __of_msi_map_rid(struct device *dev, struct device_node
> > **np,
> > u32 rid_in)
> > {
> > @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device *dev,
> > struct device_node **np,
> > if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
> > "msi-map-mask", np, &rid_out))
> > break;
> > +
> > + if (rid_out == rid_in)
> > + rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
>
> Over my dead body. Get your firmware to properly program the LUT so that it
> presents the ITS with a reasonable topology. There is absolutely no way this
> kind of change makes it into the kernel.

Sorry for this, I know it is not reasonable, but I have no other way, as I know, ARM
get the mapping of stream ID to request ID from the msi-map property of DTS, if
add a new device which need the stream ID and try to get it from the msi-map of DTS,
it will failed and not work, yes? So could you give me a better advice to fix this issue,
I would really appreciate any comments or suggestions, thanks a lot.

Thanks
Xiaowei

>
> Thanks,
>
> M.
> --
> Who you jivin' with that Cosmik Debris?

2019-12-03 11:51:52

by Marc Zyngier

Subject: RE: [PATCH] PCI: layerscape: Add the SRIOV support in host side

On 2019-12-03 01:42, Xiaowei Bao wrote:
>> -----Original Message-----
>> From: Marc Zyngier <[email protected]>
>> Sent: 2019年12月2日 20:48
>> To: Xiaowei Bao <[email protected]>
>> Cc: [email protected]; [email protected]; M.h. Lian
>> <[email protected]>; Mingkai Hu <[email protected]>; Roy Zang
>> <[email protected]>; [email protected];
>> [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; Z.q. Hou
>> <[email protected]>
>> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host
>> side
>>
>> On 2019-12-02 10:45, Xiaowei Bao wrote:
>> > GIC get the map relations of devid and stream id from the msi-map
>> > property of DTS, our platform add this property in u-boot base on
>> the
>> > PCIe device in the bus, but if enable the vf device in kernel, the
>> vf
>> > device msi-map will not set, so the vf device can't work, this
>> patch
>> > purpose is that manage the stream id and device id map relations
>> > dynamically in kernel, and make the new PCIe device work in
>> kernel.
>> >
>> > Signed-off-by: Xiaowei Bao <[email protected]>
>> > ---
>> > drivers/of/irq.c | 9 +++
>> > drivers/pci/controller/dwc/pci-layerscape.c | 94
>> > +++++++++++++++++++++++++++++
>> > drivers/pci/probe.c | 6 ++
>> > drivers/pci/remove.c | 6 ++
>> > 4 files changed, 115 insertions(+)
>> >
>> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
>> > a296eaf..791e609 100644
>> > --- a/drivers/of/irq.c
>> > +++ b/drivers/of/irq.c
>> > @@ -576,6 +576,11 @@ void __init of_irq_init(const struct
>> of_device_id
>> > *matches)
>> > }
>> > }
>> >
>> > +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid) {
>> > + return rid;
>> > +}
>> > +
>> > static u32 __of_msi_map_rid(struct device *dev, struct
>> device_node
>> > **np,
>> > u32 rid_in)
>> > {
>> > @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device
>> *dev,
>> > struct device_node **np,
>> > if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
>> > "msi-map-mask", np, &rid_out))
>> > break;
>> > +
>> > + if (rid_out == rid_in)
>> > + rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
>>
>> Over my dead body. Get your firmware to properly program the LUT so
>> that it
>> presents the ITS with a reasonable topology. There is absolutely no
>> way this
>> kind of change makes it into the kernel.
>
> Sorry for this, I know it is not reasonable, but I have no other way,
> as I know, ARM
> get the mapping of stream ID to request ID from the msi-map property
> of DTS, if
> add a new device which need the stream ID and try to get it from the
> msi-map of DTS,
> it will failed and not work, yes? So could you give me a better
> advice to fix this issue,
> I would really appreciate any comments or suggestions, thanks a lot.

Why can't firmware expose an msi-map/msi-map-mask that has a large
enough range to ensure mapping of VFs? What are the limitations of
the LUT that would prevent this from being configured before the
kernel boots?

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2019-12-03 15:21:27

by Robin Murphy

Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side

On 03/12/2019 11:51 am, Marc Zyngier wrote:
> On 2019-12-03 01:42, Xiaowei Bao wrote:
>>> -----Original Message-----
>>> From: Marc Zyngier <[email protected]>
>>> Sent: 2019年12月2日 20:48
>>> To: Xiaowei Bao <[email protected]>
>>> Cc: [email protected]; [email protected]; M.h. Lian
>>> <[email protected]>; Mingkai Hu <[email protected]>; Roy Zang
>>> <[email protected]>; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; Z.q. Hou <[email protected]>
>>> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side
>>>
>>> On 2019-12-02 10:45, Xiaowei Bao wrote:
>>> > GIC get the map relations of devid and stream id from the msi-map
>>> > property of DTS, our platform add this property in u-boot base on the
>>> > PCIe device in the bus, but if enable the vf device in kernel, the vf
>>> > device msi-map will not set, so the vf device can't work, this patch
>>> > purpose is that manage the stream id and device id map relations
>>> > dynamically in kernel, and make the new PCIe device work in kernel.
>>> >
>>> > Signed-off-by: Xiaowei Bao <[email protected]>
>>> > ---
>>> >  drivers/of/irq.c                            |  9 +++
>>> >  drivers/pci/controller/dwc/pci-layerscape.c | 94
>>> > +++++++++++++++++++++++++++++
>>> >  drivers/pci/probe.c                         |  6 ++
>>> >  drivers/pci/remove.c                        |  6 ++
>>> >  4 files changed, 115 insertions(+)
>>> >
>>> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
>>> > a296eaf..791e609 100644
>>> > --- a/drivers/of/irq.c
>>> > +++ b/drivers/of/irq.c
>>> > @@ -576,6 +576,11 @@ void __init of_irq_init(const struct of_device_id
>>> > *matches)
>>> >      }
>>> >  }
>>> >
>>> > +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid) {
>>> > +    return rid;
>>> > +}
>>> > +
>>> >  static u32 __of_msi_map_rid(struct device *dev, struct device_node
>>> > **np,
>>> >                  u32 rid_in)
>>> >  {
>>> > @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device *dev,
>>> > struct device_node **np,
>>> >          if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
>>> >                  "msi-map-mask", np, &rid_out))
>>> >              break;
>>> > +
>>> > +    if (rid_out == rid_in)
>>> > +        rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
>>>
>>> Over my dead body. Get your firmware to properly program the LUT so
>>> that it
>>> presents the ITS with a reasonable topology. There is absolutely no
>>> way this
>>> kind of change makes it into the kernel.
>>
>> Sorry for this, I know it is not reasonable, but I have no other way,
>> as I know, ARM
>> get the mapping of stream ID to request ID from the msi-map property
>> of DTS, if
>> add a new device which need the stream ID and try to get it from the
>> msi-map of DTS,
>> it will failed and not work, yes? So could you give me a better
>> advice to fix this issue,
>> I would really appreciate any comments or suggestions, thanks a lot.
>
> Why can't firmware expose an msi-map/msi-map-mask that has a large
> enough range to ensure mapping of VFs? What are the limitations of
> the LUT that would prevent this from being configured before the
> kernel boots?

Furthermore, note that this attempt isn't doing anything for the SMMU
Stream IDs, so the moment anyone tries to assign those VFs they're still
going to go bang anyway. Any firmware-based fixup for ID mappings,
config space addresses, etc. needs to be SR-IOV-aware and account for
all *possible* BDFs.

On LS2085 at least, IIRC you can configure a single LUT entry to just
translate the Bus:Device identifier and pass some or all of the Function
bits straight through as the LSBs of the Stream ID, so I don't believe
the relatively limited number of LUT registers should be too much of an
issue. For example, last time I hacked on that I apparently had it set
up statically like this:

&pcie3 {
/* Squash 8:5:3 BDF down to 2:2:3 */
msi-map-mask = <0x031f>;
msi-map = <0x000 &its 0x00 0x20>,
<0x100 &its 0x20 0x20>,
<0x200 &its 0x40 0x20>,
<0x300 &its 0x60 0x20>;
};

Robin.

2019-12-04 04:35:48

by Bao Xiaowei

Subject: RE: [PATCH] PCI: layerscape: Add the SRIOV support in host side

> -----Original Message-----
> From: Robin Murphy <[email protected]>
> Sent: 2019年12月3日 23:20
> To: Marc Zyngier <[email protected]>; Xiaowei Bao <[email protected]>
> Cc: Roy Zang <[email protected]>; [email protected];
> [email protected]; [email protected]; Z.q. Hou
> <[email protected]>; [email protected]; M.h. Lian
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Mingkai Hu
> <[email protected]>
> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side
>
> On 03/12/2019 11:51 am, Marc Zyngier wrote:
> > On 2019-12-03 01:42, Xiaowei Bao wrote:
> >>> -----Original Message-----
> >>> From: Marc Zyngier <[email protected]>
> >>> Sent: 2019年12月2日 20:48
> >>> To: Xiaowei Bao <[email protected]>
> >>> Cc: [email protected]; [email protected]; M.h. Lian
> >>> <[email protected]>; Mingkai Hu <[email protected]>; Roy
> Zang
> >>> <[email protected]>; [email protected];
> >>> [email protected]; [email protected];
> >>> [email protected]; [email protected];
> >>> [email protected]; [email protected];
> >>> Z.q. Hou <[email protected]>
> >>> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host
> >>> side
> >>>
> >>> On 2019-12-02 10:45, Xiaowei Bao wrote:
> >>> > GIC get the map relations of devid and stream id from the msi-map
> >>> > property of DTS, our platform add this property in u-boot base on
> >>> > the PCIe device in the bus, but if enable the vf device in kernel,
> >>> > the vf device msi-map will not set, so the vf device can't work,
> >>> > this patch purpose is that manage the stream id and device id map
> >>> > relations dynamically in kernel, and make the new PCIe device work in
> kernel.
> >>> >
> >>> > Signed-off-by: Xiaowei Bao <[email protected]>
> >>> > ---
> >>> >  drivers/of/irq.c                            |  9 +++
> >>> >  drivers/pci/controller/dwc/pci-layerscape.c | 94
> >>> > +++++++++++++++++++++++++++++
> >>> >  drivers/pci/probe.c                         |  6 ++
> >>> >  drivers/pci/remove.c                        |  6 ++
> >>> >  4 files changed, 115 insertions(+)
> >>> >
> >>> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
> >>> > a296eaf..791e609 100644
> >>> > --- a/drivers/of/irq.c
> >>> > +++ b/drivers/of/irq.c
> >>> > @@ -576,6 +576,11 @@ void __init of_irq_init(const struct
> >>> >of_device_id
> >>> > *matches)
> >>> >      }
> >>> >  }
> >>> >
> >>> > +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid) {
> >>> > +    return rid;
> >>> > +}
> >>> > +
> >>> >  static u32 __of_msi_map_rid(struct device *dev, struct
> >>> >device_node **np,
> >>> >                  u32 rid_in)
> >>> >  {
> >>> > @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device
> >>> >*dev, struct device_node **np,
> >>> >          if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
> >>> >                  "msi-map-mask", np, &rid_out))
> >>> >              break;
> >>> > +
> >>> > +    if (rid_out == rid_in)
> >>> > +        rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
> >>>
> >>> Over my dead body. Get your firmware to properly program the LUT so
> >>> that it presents the ITS with a reasonable topology. There is
> >>> absolutely no way this kind of change makes it into the kernel.
> >>
> >> Sorry for this, I know it is not reasonable, but I have no other way,
> >> as I know, ARM get the mapping of stream ID to request ID from the
> >> msi-map property of DTS, if add a new device which need the stream ID
> >> and try to get it from the msi-map of DTS, it will failed and not
> >> work, yes? So could you give me a better advice to fix this issue, I
> >> would really appreciate any comments or suggestions, thanks a lot.
> >
> > Why can't firmware expose an msi-map/msi-map-mask that has a large
> > enough range to ensure mapping of VFs? What are the limitations of the
> > LUT that would prevent this from being configured before the kernel
> > boots?

Thanks for your comments, yes, this is the root cause, we only have 16 stream
IDs for PCIe domain, this is the hardware limitation, if there have enough stream
IDs, we can expose an msi-map/msi-map-mask for all PCIe devices in system,
unfortunately, the stream IDs is not enough, I think other ARM vendor have same
issue that they don't have enough stream IDs.

Thanks
Xiaowei

>
> Furthermore, note that this attempt isn't doing anything for the SMMU
> Stream IDs, so the moment anyone tries to assign those VFs they're still going
> to go bang anyway. Any firmware-based fixup for ID mappings, config space
> addresses, etc. needs to be SR-IOV-aware and account for all *possible*
> BDFs.
>
> On LS2085 at least, IIRC you can configure a single LUT entry to just translate
> the Bus:Device identifier and pass some or all of the Function bits straight
> through as the LSBs of the Stream ID, so I don't believe the relatively limited
> number of LUT registers should be too much of an issue. For example, last
> time I hacked on that I apparently had it set up statically like this:
>
> &pcie3 {
> /* Squash 8:5:3 BDF down to 2:2:3 */
> msi-map-mask = <0x031f>;
> msi-map = <0x000 &its 0x00 0x20>,
> <0x100 &its 0x20 0x20>,
> <0x200 &its 0x40 0x20>,
> <0x300 &its 0x60 0x20>;
> };

Thanks Robin, this is a effective way, but we only have total 16 stream IDs for PCIe domain,
and only assign 4 stream IDs for each PCIe controller if the board have 4 PCIe controllers,
this is the root cause, I submitted this patch to dynamically manage these stream IDs,
so that it looks like each PCIe controller has 16 stream IDs. I can dynamically allocate and
release these stream IDs based on the PCIe devices in the current system. If use your method,
we support up to 4 PCIe devices(2 PFs and 2 VFs), it will not achieve our purpose.

Thanks
Xiaowei

>
> Robin.

2019-12-04 08:15:10

by Marc Zyngier

Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side

On Wed, 04 Dec 2019 04:34:32 +0000,
Xiaowei Bao <[email protected]> wrote:
>
>
>
> > -----Original Message-----
> > From: Robin Murphy <[email protected]>
> > Sent: 2019年12月3日 23:20
> > To: Marc Zyngier <[email protected]>; Xiaowei Bao <[email protected]>
> > Cc: Roy Zang <[email protected]>; [email protected];
> > [email protected]; [email protected]; Z.q. Hou
> > <[email protected]>; [email protected]; M.h. Lian
> > <[email protected]>; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Mingkai Hu
> > <[email protected]>
> > Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side
> >
> > On 03/12/2019 11:51 am, Marc Zyngier wrote:
> > > On 2019-12-03 01:42, Xiaowei Bao wrote:
> > >>> -----Original Message-----
> > >>> From: Marc Zyngier <[email protected]>
> > >>> Sent: 2019年12月2日 20:48
> > >>> To: Xiaowei Bao <[email protected]>
> > >>> Cc: [email protected]; [email protected]; M.h. Lian
> > >>> <[email protected]>; Mingkai Hu <[email protected]>; Roy
> > Zang
> > >>> <[email protected]>; [email protected];
> > >>> [email protected]; [email protected];
> > >>> [email protected]; [email protected];
> > >>> [email protected]; [email protected];
> > >>> Z.q. Hou <[email protected]>
> > >>> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host
> > >>> side
> > >>>
> > >>> On 2019-12-02 10:45, Xiaowei Bao wrote:
> > >>> > GIC get the map relations of devid and stream id from the msi-map
> > >>> > property of DTS, our platform add this property in u-boot base on
> > >>> > the PCIe device in the bus, but if enable the vf device in kernel,
> > >>> > the vf device msi-map will not set, so the vf device can't work,
> > >>> > this patch purpose is that manage the stream id and device id map
> > >>> > relations dynamically in kernel, and make the new PCIe device work in
> > kernel.
> > >>> >
> > >>> > Signed-off-by: Xiaowei Bao <[email protected]>
> > >>> > ---
> > >>> >  drivers/of/irq.c                            |  9 +++
> > >>> >  drivers/pci/controller/dwc/pci-layerscape.c | 94
> > >>> > +++++++++++++++++++++++++++++
> > >>> >  drivers/pci/probe.c                         |  6 ++
> > >>> >  drivers/pci/remove.c                        |  6 ++
> > >>> >  4 files changed, 115 insertions(+)
> > >>> >
> > >>> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
> > >>> > a296eaf..791e609 100644
> > >>> > --- a/drivers/of/irq.c
> > >>> > +++ b/drivers/of/irq.c
> > >>> > @@ -576,6 +576,11 @@ void __init of_irq_init(const struct
> > >>> >of_device_id
> > >>> > *matches)
> > >>> >      }
> > >>> >  }
> > >>> >
> > >>> > +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid) {
> > >>> > +    return rid;
> > >>> > +}
> > >>> > +
> > >>> >  static u32 __of_msi_map_rid(struct device *dev, struct
> > >>> >device_node **np,
> > >>> >                  u32 rid_in)
> > >>> >  {
> > >>> > @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device
> > >>> >*dev, struct device_node **np,
> > >>> >          if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
> > >>> >                  "msi-map-mask", np, &rid_out))
> > >>> >              break;
> > >>> > +
> > >>> > +    if (rid_out == rid_in)
> > >>> > +        rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
> > >>>
> > >>> Over my dead body. Get your firmware to properly program the LUT so
> > >>> that it presents the ITS with a reasonable topology. There is
> > >>> absolutely no way this kind of change makes it into the kernel.
> > >>
> > >> Sorry for this, I know it is not reasonable, but I have no other way,
> > >> as I know, ARM get the mapping of stream ID to request ID from the
> > >> msi-map property of DTS, if add a new device which need the stream ID
> > >> and try to get it from the msi-map of DTS, it will failed and not
> > >> work, yes? So could you give me a better advice to fix this issue, I
> > >> would really appreciate any comments or suggestions, thanks a lot.
> > >
> > > Why can't firmware expose an msi-map/msi-map-mask that has a large
> > > enough range to ensure mapping of VFs? What are the limitations of the
> > > LUT that would prevent this from being configured before the kernel
> > > boots?
>
> Thanks for your comments, yes, this is the root cause, we only have
> 16 stream IDs for PCIe domain, this is the hardware limitation, if
> there have enough stream IDs, we can expose an msi-map/msi-map-mask
> for all PCIe devices in system, unfortunately, the stream IDs is not
> enough, I think other ARM vendor have same issue that they don't
> have enough stream IDs.

Not that I know off.

I'm using a number of ARM-based, SMMU-equipped HW that works just
fine. SR-IOV is perfectly functional on these platforms, and it seems
that only FSL/NXP HW requires hacks of this sort.

M.

--
Jazz is not dead, it just smells funny.

2019-12-04 12:00:20

by Robin Murphy

Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side

On 2019-12-04 4:34 am, Xiaowei Bao wrote:
>
>
>> -----Original Message-----
>> From: Robin Murphy <[email protected]>
>> Sent: 2019年12月3日 23:20
>> To: Marc Zyngier <[email protected]>; Xiaowei Bao <[email protected]>
>> Cc: Roy Zang <[email protected]>; [email protected];
>> [email protected]; [email protected]; Z.q. Hou
>> <[email protected]>; [email protected]; M.h. Lian
>> <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; Mingkai Hu
>> <[email protected]>
>> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side
>>
>> On 03/12/2019 11:51 am, Marc Zyngier wrote:
>>> On 2019-12-03 01:42, Xiaowei Bao wrote:
>>>>> -----Original Message-----
>>>>> From: Marc Zyngier <[email protected]>
>>>>> Sent: 2019年12月2日 20:48
>>>>> To: Xiaowei Bao <[email protected]>
>>>>> Cc: [email protected]; [email protected]; M.h. Lian
>>>>> <[email protected]>; Mingkai Hu <[email protected]>; Roy
>> Zang
>>>>> <[email protected]>; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> [email protected]; [email protected];
>>>>> Z.q. Hou <[email protected]>
>>>>> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host
>>>>> side
>>>>>
>>>>> On 2019-12-02 10:45, Xiaowei Bao wrote:
>>>>>> GIC get the map relations of devid and stream id from the msi-map
>>>>>> property of DTS, our platform add this property in u-boot base on
>>>>>> the PCIe device in the bus, but if enable the vf device in kernel,
>>>>>> the vf device msi-map will not set, so the vf device can't work,
>>>>>> this patch purpose is that manage the stream id and device id map
>>>>>> relations dynamically in kernel, and make the new PCIe device work in
>> kernel.
>>>>>>
>>>>>> Signed-off-by: Xiaowei Bao <[email protected]>
>>>>>> ---
>>>>>>   drivers/of/irq.c                            |  9 +++
>>>>>>   drivers/pci/controller/dwc/pci-layerscape.c | 94
>>>>>> +++++++++++++++++++++++++++++
>>>>>>   drivers/pci/probe.c                         |  6 ++
>>>>>>   drivers/pci/remove.c                        |  6 ++
>>>>>>   4 files changed, 115 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
>>>>>> a296eaf..791e609 100644
>>>>>> --- a/drivers/of/irq.c
>>>>>> +++ b/drivers/of/irq.c
>>>>>> @@ -576,6 +576,11 @@ void __init of_irq_init(const struct
>>>>>> of_device_id
>>>>>> *matches)
>>>>>>       }
>>>>>>   }
>>>>>>
>>>>>> +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid) {
>>>>>> +    return rid;
>>>>>> +}
>>>>>> +
>>>>>>   static u32 __of_msi_map_rid(struct device *dev, struct
>>>>>> device_node **np,
>>>>>>                   u32 rid_in)
>>>>>>   {
>>>>>> @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device
>>>>>> *dev, struct device_node **np,
>>>>>>           if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
>>>>>>                   "msi-map-mask", np, &rid_out))
>>>>>>               break;
>>>>>> +
>>>>>> +    if (rid_out == rid_in)
>>>>>> +        rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
>>>>>
>>>>> Over my dead body. Get your firmware to properly program the LUT so
>>>>> that it presents the ITS with a reasonable topology. There is
>>>>> absolutely no way this kind of change makes it into the kernel.
>>>>
>>>> Sorry for this, I know it is not reasonable, but I have no other way,
>>>> as I know, ARM get the mapping of stream ID to request ID from the
>>>> msi-map property of DTS, if add a new device which need the stream ID
>>>> and try to get it from the msi-map of DTS, it will failed and not
>>>> work, yes? So could you give me a better advice to fix this issue, I
>>>> would really appreciate any comments or suggestions, thanks a lot.
>>>
>>> Why can't firmware expose an msi-map/msi-map-mask that has a large
>>> enough range to ensure mapping of VFs? What are the limitations of the
>>> LUT that would prevent this from being configured before the kernel
>>> boots?
>
> Thanks for your comments, yes, this is the root cause, we only have 16 stream
> IDs for PCIe domain, this is the hardware limitation, if there have enough stream
> IDs, we can expose an msi-map/msi-map-mask for all PCIe devices in system,
> unfortunately, the stream IDs is not enough, I think other ARM vendor have same
> issue that they don't have enough stream IDs.

Some SMMUv2 configurations may have an uncomfortably limited number of
context banks, but they almost always have more than enough stream ID
bits. Your ICID allocation policy is most certainly an issue unique to
Layerscape platforms.

Furthermore, that argument doesn't make a whole lot of sense anyway - if
you don't have enough stream IDs for all possible VFs at boot time, then
you still won't have enough later, so pretending to support SR-IOV, only
for things to start subtly going wrong if the user has too many
endpoints active at once, isn't going to cut it.

>> Furthermore, note that this attempt isn't doing anything for the SMMU
>> Stream IDs, so the moment anyone tries to assign those VFs they're still going
>> to go bang anyway. Any firmware-based fixup for ID mappings, config space
>> addresses, etc. needs to be SR-IOV-aware and account for all *possible*
>> BDFs.
>>
>> On LS2085 at least, IIRC you can configure a single LUT entry to just translate
>> the Bus:Device identifier and pass some or all of the Function bits straight
>> through as the LSBs of the Stream ID, so I don't believe the relatively limited
>> number of LUT registers should be too much of an issue. For example, last
>> time I hacked on that I apparently had it set up statically like this:
>>
>> &pcie3 {
>> /* Squash 8:5:3 BDF down to 2:2:3 */
>> msi-map-mask = <0x031f>;
>> msi-map = <0x000 &its 0x00 0x20>,
>> <0x100 &its 0x20 0x20>,
>> <0x200 &its 0x40 0x20>,
>> <0x300 &its 0x60 0x20>;
>> };
>
> Thanks Robin, this is a effective way, but we only have total 16 stream IDs for PCIe domain,
> and only assign 4 stream IDs for each PCIe controller if the board have 4 PCIe controllers,
> this is the root cause, I submitted this patch to dynamically manage these stream IDs,
> so that it looks like each PCIe controller has 16 stream IDs. I can dynamically allocate and
> release these stream IDs based on the PCIe devices in the current system. If use your method,
> we support up to 4 PCIe devices(2 PFs and 2 VFs), it will not achieve our purpose.

Sure, that was just an example to illustrate that you don't need a
separate msi-map entry (and corresponding LUT entry) for each individual
PCI RID - that dates from before U-Boot had ICID support, so I had hacks
all over various kernel drivers to set them arbitrarily when I was
playing with the SMMU.

Realistically, at this point your options are a) reserve more ICIDs for
PCIe and allocate them in a way that accounts for the present endpoints'
SR-IOV capabilities, or b) don't expose SR-IOV functionality at all on
the root complex if it can't be guaranteed to work properly.

Robin.

2019-12-05 02:57:35

by Bao Xiaowei

Subject: RE: [PATCH] PCI: layerscape: Add the SRIOV support in host side

> -----Original Message-----
> From: Robin Murphy <[email protected]>
> Sent: 2019年12月4日 19:59
> To: Xiaowei Bao <[email protected]>; Marc Zyngier <[email protected]>
> Cc: Roy Zang <[email protected]>; [email protected];
> [email protected]; [email protected]; Z.q. Hou
> <[email protected]>; [email protected]; M.h. Lian
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Mingkai Hu
> <[email protected]>
> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side
>
> On 2019-12-04 4:34 am, Xiaowei Bao wrote:
> >
> >
> >> -----Original Message-----
> >> From: Robin Murphy <[email protected]>
> >> Sent: 2019年12月3日 23:20
> >> To: Marc Zyngier <[email protected]>; Xiaowei Bao
> <[email protected]>
> >> Cc: Roy Zang <[email protected]>; [email protected];
> >> [email protected]; [email protected]; Z.q. Hou
> >> <[email protected]>; [email protected]; M.h. Lian
> >> <[email protected]>; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; Mingkai Hu
> >> <[email protected]>
> >> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host
> >> side
> >>
> >> On 03/12/2019 11:51 am, Marc Zyngier wrote:
> >>> On 2019-12-03 01:42, Xiaowei Bao wrote:
> >>>>> -----Original Message-----
> >>>>> From: Marc Zyngier <[email protected]>
> >>>>> Sent: 2019年12月2日 20:48
> >>>>> To: Xiaowei Bao <[email protected]>
> >>>>> Cc: [email protected]; [email protected]; M.h. Lian
> >>>>> <[email protected]>; Mingkai Hu <[email protected]>; Roy
> >> Zang
> >>>>> <[email protected]>; [email protected];
> >>>>> [email protected]; [email protected];
> >>>>> [email protected]; [email protected];
> >>>>> [email protected]; [email protected];
> >>>>> Z.q. Hou <[email protected]>
> >>>>> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in
> >>>>> host side
> >>>>>
> >>>>> On 2019-12-02 10:45, Xiaowei Bao wrote:
> >>>>>> GIC get the map relations of devid and stream id from the msi-map
> >>>>>> property of DTS, our platform add this property in u-boot base on
> >>>>>> the PCIe device in the bus, but if enable the vf device in
> >>>>>> kernel, the vf device msi-map will not set, so the vf device
> >>>>>> can't work, this patch purpose is that manage the stream id and
> >>>>>> device id map relations dynamically in kernel, and make the new
> >>>>>> PCIe device work in
> >> kernel.
> >>>>>>
> >>>>>> Signed-off-by: Xiaowei Bao <[email protected]>
> >>>>>> ---
> >>>>>>   drivers/of/irq.c                            |  9
> +++
> >>>>>>   drivers/pci/controller/dwc/pci-layerscape.c | 94
> >>>>>> +++++++++++++++++++++++++++++
> >>>>>>   drivers/pci/probe.c                         |  6
> ++
> >>>>>>   drivers/pci/remove.c                        |  6
> ++
> >>>>>>   4 files changed, 115 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
> >>>>>> a296eaf..791e609 100644
> >>>>>> --- a/drivers/of/irq.c
> >>>>>> +++ b/drivers/of/irq.c
> >>>>>> @@ -576,6 +576,11 @@ void __init of_irq_init(const struct
> >>>>>> of_device_id
> >>>>>> *matches)
> >>>>>>       }
> >>>>>>   }
> >>>>>>
> >>>>>> +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid) {
> >>>>>> +    return rid;
> >>>>>> +}
> >>>>>> +
> >>>>>>   static u32 __of_msi_map_rid(struct device *dev, struct
> >>>>>> device_node **np,
> >>>>>>                   u32 rid_in)
> >>>>>>   {
> >>>>>> @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device
> >>>>>> *dev, struct device_node **np,
> >>>>>>           if (!of_map_rid(parent_dev->of_node, rid_in,
> >>>>>> "msi-map",
> >>>>>>                   "msi-map-mask", np, &rid_out))
> >>>>>>               break;
> >>>>>> +
> >>>>>> +    if (rid_out == rid_in)
> >>>>>> +        rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
> >>>>>
> >>>>> Over my dead body. Get your firmware to properly program the LUT
> >>>>> so that it presents the ITS with a reasonable topology. There is
> >>>>> absolutely no way this kind of change makes it into the kernel.
> >>>>
> >>>> Sorry for this, I know it is not reasonable, but I have no other
> >>>> way, as I know, ARM get the mapping of stream ID to request ID from
> >>>> the msi-map property of DTS, if add a new device which need the
> >>>> stream ID and try to get it from the msi-map of DTS, it will failed
> >>>> and not work, yes? So could you give me a better advice to fix this
> >>>> issue, I would really appreciate any comments or suggestions, thanks a
> lot.
> >>>
> >>> Why can't firmware expose an msi-map/msi-map-mask that has a large
> >>> enough range to ensure mapping of VFs? What are the limitations of
> >>> the LUT that would prevent this from being configured before the
> >>> kernel boots?
> >
> > Thanks for your comments, yes, this is the root cause, we only have 16
> > stream IDs for PCIe domain, this is the hardware limitation, if there
> > have enough stream IDs, we can expose an msi-map/msi-map-mask for all
> > PCIe devices in system, unfortunately, the stream IDs is not enough, I
> > think other ARM vendor have same issue that they don't have enough
> stream IDs.
>
> Some SMMUv2 configurations may have an uncomfortably limited number of
> context banks, but they almost always have more than enough stream ID bits.
> Your ICID allocation policy is most certainly an issue unique to Layerscape
> platforms.

I am not sure which version of SMMU used in our Layerscape platform, I think the
SMMU version is 1 in our early board. So there is not enough stream ID for PCIe or
other peripherals.

>
> Furthermore, that argument doesn't make a whole lot of sense anyway - if
> you don't have enough stream IDs for all possible VFs at boot time, then you
> still won't have enough later, so pretending to support SR-IOV, only for things
> to start subtly going wrong if the user has too many endpoints active at once,
> isn't going to cut it.

Thanks Robin, yes, agree your point, I think I have to drop this patch, the purpose
of submitting this patch to opensource is that to know whether have a best way
to fix this issue, but due to the limitation of hardware, it looks like have no better
way, in a word, thanks a lot.

>
> >> Furthermore, note that this attempt isn't doing anything for the SMMU
> >> Stream IDs, so the moment anyone tries to assign those VFs they're
> >> still going to go bang anyway. Any firmware-based fixup for ID
> >> mappings, config space addresses, etc. needs to be SR-IOV-aware and
> >> account for all *possible* BDFs.
> >>
> >> On LS2085 at least, IIRC you can configure a single LUT entry to just
> >> translate the Bus:Device identifier and pass some or all of the
> >> Function bits straight through as the LSBs of the Stream ID, so I
> >> don't believe the relatively limited number of LUT registers should
> >> be too much of an issue. For example, last time I hacked on that I
> apparently had it set up statically like this:
> >>
> >> &pcie3 {
> >> /* Squash 8:5:3 BDF down to 2:2:3 */
> >> msi-map-mask = <0x031f>;
> >> msi-map = <0x000 &its 0x00 0x20>,
> >> <0x100 &its 0x20 0x20>,
> >> <0x200 &its 0x40 0x20>,
> >> <0x300 &its 0x60 0x20>;
> >> };
> >
> > Thanks Robin, this is a effective way, but we only have total 16
> > stream IDs for PCIe domain, and only assign 4 stream IDs for each PCIe
> > controller if the board have 4 PCIe controllers, this is the root
> > cause, I submitted this patch to dynamically manage these stream IDs,
> > so that it looks like each PCIe controller has 16 stream IDs. I can
> > dynamically allocate and release these stream IDs based on the PCIe
> devices in the current system. If use your method, we support up to 4 PCIe
> devices(2 PFs and 2 VFs), it will not achieve our purpose.
>
> Sure, that was just an example to illustrate that you don't need a separate
> msi-map entry (and corresponding LUT entry) for each individual PCI RID -
> that dates from before U-Boot had ICID support, so I had hacks all over
> various kernel drivers to set them arbitrarily when I was playing with the
> SMMU.
>
> Realistically, at this point your options are a) reserve more ICIDs for PCIe and
> allocate them in a way that accounts for the present endpoints'
> SR-IOV capabilities, or b) don't expose SR-IOV functionality at all on the root
> complex if it can't be guaranteed to work properly.

Agree, if enable more than 16 PCIe devices, the stream IDs for PCIe are not enough,
the root cause is the limitation of the hardware, I think I have to drop it, thanks a lot.

>
> Robin.

2019-12-05 10:45:18

by Laurentiu Tudor

Subject: RE: [PATCH] PCI: layerscape: Add the SRIOV support in host side

Hi Xiaowei,

> -----Original Message-----
> From: linux-arm-kernel <[email protected]> On
> Behalf Of Xiaowei Bao
>
> > -----Original Message-----
> > From: Marc Zyngier <[email protected]>
> > Sent: 2019年12月2日 20:48
> > To: Xiaowei Bao <[email protected]>
> > Cc: [email protected]; [email protected]; M.h. Lian
> > <[email protected]>; Mingkai Hu <[email protected]>; Roy Zang
> > <[email protected]>; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; Z.q. Hou <[email protected]>
> > Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side
> >
> > On 2019-12-02 10:45, Xiaowei Bao wrote:
> > > GIC get the map relations of devid and stream id from the msi-map
> > > property of DTS, our platform add this property in u-boot base on the
> > > PCIe device in the bus, but if enable the vf device in kernel, the vf
> > > device msi-map will not set, so the vf device can't work, this patch
> > > purpose is that manage the stream id and device id map relations
> > > dynamically in kernel, and make the new PCIe device work in kernel.
> > >
> > > Signed-off-by: Xiaowei Bao <[email protected]>
> > > ---
> > > drivers/of/irq.c | 9 +++
> > > drivers/pci/controller/dwc/pci-layerscape.c | 94
> > > +++++++++++++++++++++++++++++
> > > drivers/pci/probe.c | 6 ++
> > > drivers/pci/remove.c | 6 ++
> > > 4 files changed, 115 insertions(+)
> > >
> > > diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
> > > a296eaf..791e609 100644
> > > --- a/drivers/of/irq.c
> > > +++ b/drivers/of/irq.c
> > > @@ -576,6 +576,11 @@ void __init of_irq_init(const struct of_device_id
> > > *matches)
> > > }
> > > }
> > >
> > > +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid) {
> > > + return rid;
> > > +}
> > > +
> > > static u32 __of_msi_map_rid(struct device *dev, struct device_node
> > > **np,
> > > u32 rid_in)
> > > {
> > > @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device *dev,
> > > struct device_node **np,
> > > if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
> > > "msi-map-mask", np, &rid_out))
> > > break;
> > > +
> > > + if (rid_out == rid_in)
> > > + rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
> >
> > Over my dead body. Get your firmware to properly program the LUT so that
> it
> > presents the ITS with a reasonable topology. There is absolutely no way
> this
> > kind of change makes it into the kernel.
>
> Sorry for this, I know it is not reasonable, but I have no other way, as I
> know, ARM
> get the mapping of stream ID to request ID from the msi-map property of
> DTS, if
> add a new device which need the stream ID and try to get it from the msi-
> map of DTS,
> it will failed and not work, yes? So could you give me a better advice to
> fix this issue,
> I would really appreciate any comments or suggestions, thanks a lot.
>

I agree with the community that this should be tackled in firmware. I actually submitted (by mistake, but let's disregard that :-)) a simple proposal in u-boot [1] that should take care of it. We can discuss further on it, if you wish.

[1] https://patchwork.ozlabs.org/patch/1033466/

---
Best Regards, Laurentiu

2019-12-05 11:12:03

by Laurentiu Tudor

Subject: RE: [PATCH] PCI: layerscape: Add the SRIOV support in host side

Hi Xiaowei,

> -----Original Message-----
> From: linux-arm-kernel <[email protected]> On
> Behalf Of Xiaowei Bao
>
> > -----Original Message-----
> > From: Robin Murphy <[email protected]>
> > Sent: 2019年12月3日 23:20
> > To: Marc Zyngier <[email protected]>; Xiaowei Bao <[email protected]>
> > Cc: Roy Zang <[email protected]>; [email protected];
> > [email protected]; [email protected]; Z.q. Hou
> > <[email protected]>; [email protected]; M.h. Lian
> > <[email protected]>; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Mingkai Hu
> > <[email protected]>
> > Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host side
> >
> > On 03/12/2019 11:51 am, Marc Zyngier wrote:
> > > On 2019-12-03 01:42, Xiaowei Bao wrote:
> > >>> -----Original Message-----
> > >>> From: Marc Zyngier <[email protected]>
> > >>> Sent: 2019年12月2日 20:48
> > >>> To: Xiaowei Bao <[email protected]>
> > >>> Cc: [email protected]; [email protected]; M.h. Lian
> > >>> <[email protected]>; Mingkai Hu <[email protected]>; Roy
> > Zang
> > >>> <[email protected]>; [email protected];
> > >>> [email protected]; [email protected];
> > >>> [email protected]; [email protected];
> > >>> [email protected]; [email protected];
> > >>> Z.q. Hou <[email protected]>
> > >>> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host
> > >>> side
> > >>>
> > >>> On 2019-12-02 10:45, Xiaowei Bao wrote:
> > >>> > GIC get the map relations of devid and stream id from the msi-map
> > >>> > property of DTS, our platform add this property in u-boot base on
> > >>> > the PCIe device in the bus, but if enable the vf device in kernel,
> > >>> > the vf device msi-map will not set, so the vf device can't work,
> > >>> > this patch purpose is that manage the stream id and device id map
> > >>> > relations dynamically in kernel, and make the new PCIe device work
> in
> > kernel.
> > >>> >
> > >>> > Signed-off-by: Xiaowei Bao <[email protected]>
> > >>> > ---
> > >>> >  drivers/of/irq.c                            |  9 +++
> > >>> >  drivers/pci/controller/dwc/pci-layerscape.c | 94
> > >>> > +++++++++++++++++++++++++++++
> > >>> >  drivers/pci/probe.c                         |  6 ++
> > >>> >  drivers/pci/remove.c                        |  6 ++
> > >>> >  4 files changed, 115 insertions(+)
> > >>> >
> > >>> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
> > >>> > a296eaf..791e609 100644
> > >>> > --- a/drivers/of/irq.c
> > >>> > +++ b/drivers/of/irq.c
> > >>> > @@ -576,6 +576,11 @@ void __init of_irq_init(const struct
> > >>> >of_device_id
> > >>> > *matches)
> > >>> >      }
> > >>> >  }
> > >>> >
> > >>> > +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid) {
> > >>> > +    return rid;
> > >>> > +}
> > >>> > +
> > >>> >  static u32 __of_msi_map_rid(struct device *dev, struct
> > >>> >device_node **np,
> > >>> >                  u32 rid_in)
> > >>> >  {
> > >>> > @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device
> > >>> >*dev, struct device_node **np,
> > >>> >          if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
> > >>> >                  "msi-map-mask", np, &rid_out))
> > >>> >              break;
> > >>> > +
> > >>> > +    if (rid_out == rid_in)
> > >>> > +        rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
> > >>>
> > >>> Over my dead body. Get your firmware to properly program the LUT so
> > >>> that it presents the ITS with a reasonable topology. There is
> > >>> absolutely no way this kind of change makes it into the kernel.
> > >>
> > >> Sorry for this, I know it is not reasonable, but I have no other way,
> > >> as I know, ARM get the mapping of stream ID to request ID from the
> > >> msi-map property of DTS, if add a new device which need the stream ID
> > >> and try to get it from the msi-map of DTS, it will failed and not
> > >> work, yes? So could you give me a better advice to fix this issue, I
> > >> would really appreciate any comments or suggestions, thanks a lot.
> > >
> > > Why can't firmware expose an msi-map/msi-map-mask that has a large
> > > enough range to ensure mapping of VFs? What are the limitations of the
> > > LUT that would prevent this from being configured before the kernel
> > > boots?
>
> Thanks for your comments, yes, this is the root cause, we only have 16
> stream
> IDs for PCIe domain, this is the hardware limitation, if there have enough
> stream
> IDs, we can expose an msi-map/msi-map-mask for all PCIe devices in system,
> unfortunately, the stream IDs is not enough, I think other ARM vendor have
> same
> issue that they don't have enough stream IDs.
>
> Thanks
> Xiaowei
>
> >
> > Furthermore, note that this attempt isn't doing anything for the SMMU
> > Stream IDs, so the moment anyone tries to assign those VFs they're still
> going
> > to go bang anyway. Any firmware-based fixup for ID mappings, config
> space
> > addresses, etc. needs to be SR-IOV-aware and account for all *possible*
> > BDFs.
> >
> > On LS2085 at least, IIRC you can configure a single LUT entry to just
> translate
> > the Bus:Device identifier and pass some or all of the Function bits
> straight
> > through as the LSBs of the Stream ID, so I don't believe the relatively
> limited
> > number of LUT registers should be too much of an issue. For example,
> last
> > time I hacked on that I apparently had it set up statically like this:
> >
> > &pcie3 {
> > /* Squash 8:5:3 BDF down to 2:2:3 */
> > msi-map-mask = <0x031f>;
> > msi-map = <0x000 &its 0x00 0x20>,
> > <0x100 &its 0x20 0x20>,
> > <0x200 &its 0x40 0x20>,
> > <0x300 &its 0x60 0x20>;
> > };
>
> Thanks Robin, this is a effective way, but we only have total 16 stream
> IDs for PCIe domain,
> and only assign 4 stream IDs for each PCIe controller if the board have 4
> PCIe controllers,
> this is the root cause, I submitted this patch to dynamically manage these
> stream IDs,
> so that it looks like each PCIe controller has 16 stream IDs. I can
> dynamically allocate and
> release these stream IDs based on the PCIe devices in the current system.
> If use your method,
> we support up to 4 PCIe devices(2 PFs and 2 VFs), it will not achieve our
> purpose.
>

We allocate the Stream_IDs in a static fashion in u-boot, see [1], so if a user would need a larger range for PCI {s}he could adjust that in there. On most of our Layerscape chips the SMMU is configured to 5 bits for TBU_ID plus 10 bits for StreamID. Out of these 10 controllable bits we can effectively use 7 bits giving us a max range of 127 Stream_IDs.

[1] https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/include/asm/arch-fsl-layerscape/stream_id_lsch3.h

---
Best Regards, Laurentiu

2019-12-09 07:01:11

by Bao Xiaowei

Subject: RE: [PATCH] PCI: layerscape: Add the SRIOV support in host side

> -----Original Message-----
> From: Laurentiu Tudor <[email protected]>
> Sent: 2019年12月5日 19:11
> To: Xiaowei Bao <[email protected]>; Robin Murphy
> <[email protected]>; Marc Zyngier <[email protected]>
> Cc: Roy Zang <[email protected]>; [email protected];
> [email protected]; [email protected]; Z.q. Hou
> <[email protected]>; [email protected]; M.h. Lian
> <[email protected]>; [email protected]; Mingkai Hu
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; Diana
> Madalina Craciun <[email protected]>
> Subject: RE: [PATCH] PCI: layerscape: Add the SRIOV support in host side
>
> Hi Xiaowei,
>
> > -----Original Message-----
> > From: linux-arm-kernel <[email protected]>
> > On Behalf Of Xiaowei Bao
> >
> > > -----Original Message-----
> > > From: Robin Murphy <[email protected]>
> > > Sent: 2019年12月3日 23:20
> > > To: Marc Zyngier <[email protected]>; Xiaowei Bao
> <[email protected]>
> > > Cc: Roy Zang <[email protected]>; [email protected];
> > > [email protected]; [email protected]; Z.q. Hou
> > > <[email protected]>; [email protected]; M.h. Lian
> > > <[email protected]>; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; Mingkai Hu
> > > <[email protected]>
> > > Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host
> > > side
> > >
> > > On 03/12/2019 11:51 am, Marc Zyngier wrote:
> > > > On 2019-12-03 01:42, Xiaowei Bao wrote:
> > > >>> -----Original Message-----
> > > >>> From: Marc Zyngier <[email protected]>
> > > >>> Sent: 2019年12月2日 20:48
> > > >>> To: Xiaowei Bao <[email protected]>
> > > >>> Cc: [email protected]; [email protected]; M.h. Lian
> > > >>> <[email protected]>; Mingkai Hu <[email protected]>; Roy
> > > Zang
> > > >>> <[email protected]>; [email protected];
> > > >>> [email protected]; [email protected];
> > > >>> [email protected]; [email protected];
> > > >>> [email protected]; [email protected];
> > > >>> Z.q. Hou <[email protected]>
> > > >>> Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in
> > > >>> host side
> > > >>>
> > > >>> On 2019-12-02 10:45, Xiaowei Bao wrote:
> > > >>> > GIC get the map relations of devid and stream id from the
> > > >>> > msi-map property of DTS, our platform add this property in
> > > >>> > u-boot base on the PCIe device in the bus, but if enable the
> > > >>> > vf device in kernel, the vf device msi-map will not set, so
> > > >>> > the vf device can't work, this patch purpose is that manage
> > > >>> > the stream id and device id map relations dynamically in
> > > >>> > kernel, and make the new PCIe device work
> > in
> > > kernel.
> > > >>> >
> > > >>> > Signed-off-by: Xiaowei Bao <[email protected]>
> > > >>> > ---
> > > >>> >  drivers/of/irq.c                            |  9
> +++
> > > >>> >  drivers/pci/controller/dwc/pci-layerscape.c | 94
> > > >>> > +++++++++++++++++++++++++++++
> > > >>> >  drivers/pci/probe.c                         |  6 ++
> > > >>> >  drivers/pci/remove.c                        |  6 ++
> > > >>> >  4 files changed, 115 insertions(+)
> > > >>> >
> > > >>> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
> > > >>> > a296eaf..791e609 100644
> > > >>> > --- a/drivers/of/irq.c
> > > >>> > +++ b/drivers/of/irq.c
> > > >>> > @@ -576,6 +576,11 @@ void __init of_irq_init(const struct
> > > >>> >of_device_id
> > > >>> > *matches)
> > > >>> >      }
> > > >>> >  }
> > > >>> >
> > > >>> > +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid)
> > > >>> > +{
> > > >>> > +    return rid;
> > > >>> > +}
> > > >>> > +
> > > >>> >  static u32 __of_msi_map_rid(struct device *dev, struct
> > > >>> >device_node **np,
> > > >>> >                  u32 rid_in)
> > > >>> >  {
> > > >>> > @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device
> > > >>> >*dev, struct device_node **np,
> > > >>> >          if (!of_map_rid(parent_dev->of_node, rid_in,
> > > >>> >"msi-map",
> > > >>> >                  "msi-map-mask", np, &rid_out))
> > > >>> >              break;
> > > >>> > +
> > > >>> > +    if (rid_out == rid_in)
> > > >>> > +        rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
> > > >>>
> > > >>> Over my dead body. Get your firmware to properly program the LUT
> > > >>> so that it presents the ITS with a reasonable topology. There is
> > > >>> absolutely no way this kind of change makes it into the kernel.
> > > >>
> > > >> Sorry for this, I know it is not reasonable, but I have no other
> > > >> way, as I know, ARM get the mapping of stream ID to request ID
> > > >> from the msi-map property of DTS, if add a new device which need
> > > >> the stream ID and try to get it from the msi-map of DTS, it will
> > > >> failed and not work, yes? So could you give me a better advice to
> > > >> fix this issue, I would really appreciate any comments or suggestions,
> thanks a lot.
> > > >
> > > > Why can't firmware expose an msi-map/msi-map-mask that has a large
> > > > enough range to ensure mapping of VFs? What are the limitations of
> > > > the LUT that would prevent this from being configured before the
> > > > kernel boots?
> >
> > Thanks for your comments, yes, this is the root cause, we only have 16
> > stream IDs for PCIe domain, this is the hardware limitation, if there
> > have enough stream IDs, we can expose an msi-map/msi-map-mask for all
> > PCIe devices in system, unfortunately, the stream IDs is not enough, I
> > think other ARM vendor have same issue that they don't have enough
> > stream IDs.
> >
> > Thanks
> > Xiaowei
> >
> > >
> > > Furthermore, note that this attempt isn't doing anything for the
> > > SMMU Stream IDs, so the moment anyone tries to assign those VFs
> > > they're still
> > going
> > > to go bang anyway. Any firmware-based fixup for ID mappings, config
> > space
> > > addresses, etc. needs to be SR-IOV-aware and account for all
> > > *possible* BDFs.
> > >
> > > On LS2085 at least, IIRC you can configure a single LUT entry to
> > > just
> > translate
> > > the Bus:Device identifier and pass some or all of the Function bits
> > straight
> > > through as the LSBs of the Stream ID, so I don't believe the
> > > relatively
> > limited
> > > number of LUT registers should be too much of an issue. For example,
> > last
> > > time I hacked on that I apparently had it set up statically like this:
> > >
> > > &pcie3 {
> > > /* Squash 8:5:3 BDF down to 2:2:3 */
> > > msi-map-mask = <0x031f>;
> > > msi-map = <0x000 &its 0x00 0x20>,
> > > <0x100 &its 0x20 0x20>,
> > > <0x200 &its 0x40 0x20>,
> > > <0x300 &its 0x60 0x20>;
> > > };
> >
> > Thanks Robin, this is a effective way, but we only have total 16
> > stream IDs for PCIe domain, and only assign 4 stream IDs for each PCIe
> > controller if the board have 4 PCIe controllers, this is the root
> > cause, I submitted this patch to dynamically manage these stream IDs,
> > so that it looks like each PCIe controller has 16 stream IDs. I can
> > dynamically allocate and release these stream IDs based on the PCIe
> > devices in the current system.
> > If use your method,
> > we support up to 4 PCIe devices(2 PFs and 2 VFs), it will not achieve
> > our purpose.
> >
>
> We allocate the Stream_IDs in a static fashion in u-boot, see [1], so if a user
> would need a larger range for PCI {s}he could adjust that in there. On most of
> our Layerscape chips the SMMU is configured to 5 bits for TBU_ID plus 10 bits
> for StreamID. Out of these 10 controllable bits we can effectively use 7 bits
> giving us a max range of 127 Stream_IDs.
>
> [1]
> https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/include/asm/ar
> ch-fsl-layerscape/stream_id_lsch3.h

Thanks for your information, it is not enough even there are 127 Stream_IDs, if a
PCIe device which support SRIOV, but the VFs offset is 128, the VFs will not work.

Thanks
Xiaowei

>
> ---
> Best Regards, Laurentiu

2019-12-09 07:05:38

by Bao Xiaowei

Subject: RE: [PATCH] PCI: layerscape: Add the SRIOV support in host side

> -----Original Message-----
> From: Laurentiu Tudor <[email protected]>
> Sent: 2019年12月5日 18:45
> To: Xiaowei Bao <[email protected]>; Marc Zyngier
> <[email protected]>
> Cc: Roy Zang <[email protected]>; [email protected];
> [email protected]; [email protected]; Z.q. Hou
> <[email protected]>; [email protected]; M.h. Lian
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Mingkai Hu
> <[email protected]>; Diana Madalina Craciun <[email protected]>
> Subject: RE: [PATCH] PCI: layerscape: Add the SRIOV support in host side
>
> Hi Xiaowei,
>
> > -----Original Message-----
> > From: linux-arm-kernel <[email protected]>
> > On Behalf Of Xiaowei Bao
> >
> > > -----Original Message-----
> > > From: Marc Zyngier <[email protected]>
> > > Sent: 2019年12月2日 20:48
> > > To: Xiaowei Bao <[email protected]>
> > > Cc: [email protected]; [email protected]; M.h. Lian
> > > <[email protected]>; Mingkai Hu <[email protected]>; Roy
> Zang
> > > <[email protected]>; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > Z.q. Hou <[email protected]>
> > > Subject: Re: [PATCH] PCI: layerscape: Add the SRIOV support in host
> > > side
> > >
> > > On 2019-12-02 10:45, Xiaowei Bao wrote:
> > > > GIC get the map relations of devid and stream id from the msi-map
> > > > property of DTS, our platform add this property in u-boot base on
> > > > the PCIe device in the bus, but if enable the vf device in kernel,
> > > > the vf device msi-map will not set, so the vf device can't work,
> > > > this patch purpose is that manage the stream id and device id map
> > > > relations dynamically in kernel, and make the new PCIe device work in
> kernel.
> > > >
> > > > Signed-off-by: Xiaowei Bao <[email protected]>
> > > > ---
> > > > drivers/of/irq.c | 9 +++
> > > > drivers/pci/controller/dwc/pci-layerscape.c | 94
> > > > +++++++++++++++++++++++++++++
> > > > drivers/pci/probe.c | 6 ++
> > > > drivers/pci/remove.c | 6 ++
> > > > 4 files changed, 115 insertions(+)
> > > >
> > > > diff --git a/drivers/of/irq.c b/drivers/of/irq.c index
> > > > a296eaf..791e609 100644
> > > > --- a/drivers/of/irq.c
> > > > +++ b/drivers/of/irq.c
> > > > @@ -576,6 +576,11 @@ void __init of_irq_init(const struct
> > > > of_device_id
> > > > *matches)
> > > > }
> > > > }
> > > >
> > > > +u32 __weak ls_pcie_streamid_fix(struct device *dev, u32 rid) {
> > > > + return rid;
> > > > +}
> > > > +
> > > > static u32 __of_msi_map_rid(struct device *dev, struct
> > > > device_node **np,
> > > > u32 rid_in)
> > > > {
> > > > @@ -590,6 +595,10 @@ static u32 __of_msi_map_rid(struct device
> > > > *dev, struct device_node **np,
> > > > if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
> > > > "msi-map-mask", np, &rid_out))
> > > > break;
> > > > +
> > > > + if (rid_out == rid_in)
> > > > + rid_out = ls_pcie_streamid_fix(parent_dev, rid_in);
> > >
> > > Over my dead body. Get your firmware to properly program the LUT so
> > > that
> > it
> > > presents the ITS with a reasonable topology. There is absolutely no
> > > way
> > this
> > > kind of change makes it into the kernel.
> >
> > Sorry for this, I know it is not reasonable, but I have no other way,
> > as I know, ARM get the mapping of stream ID to request ID from the
> > msi-map property of DTS, if add a new device which need the stream ID
> > and try to get it from the msi- map of DTS, it will failed and not
> > work, yes? So could you give me a better advice to fix this issue, I
> > would really appreciate any comments or suggestions, thanks a lot.
> >
>
> I agree with the community that this should be tackled in firmware. I actually
> submitted (by mistake, but let's disregard that :-)) a simple proposal in u-boot
> [1] that should take care of it. We can discuss further on it, if you wish.
>
> [1] https://patchwork.ozlabs.org/patch/1033466/


I will do a experiment to verify whether all PCIe request ID map one stream ID,
and I will show the result when I complete the experiment.

Thanks
Xiaowei

>
> ---
> Best Regards, Laurentiu