[PATCH v2 1/1] PCI: endpoint: pci-epf-vntb: Fix transfer fail when BAR1 is fixed size

[PATCH v2 1/1] PCI: endpoint: pci-epf-vntb: Fix transfer fail when BAR1 is fixed size

[Frank Li]

ntb_netdev transfer is failing when epc controller's BAR1 is fix size, such
as 64K. Certain controller(like dwc) require memory address must be align
with the fixed bar size.

For example:
	If BAR1's fix size is 64K, and other size programmable BAR's
alignment is 4K.
	vntb call pci_epf_alloc_space() get 4K aligned address, like
0x104E31000. But root complex actually write to address 0x104E30000 when
write BAR1.

Adds bar_fixed_size check and sets correct alignment for fixed-size BAR.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---

Notes:
    Change from v1 to v2
    - Remove unnessary set align when fix_bar_size.

 drivers/pci/endpoint/functions/pci-epf-vntb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 3f60128560ed0..ec3922f404efe 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -550,6 +550,14 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 
 	barno = ntb->epf_ntb_bar[BAR_DB];
 
+	if (epc_features->bar_fixed_size[barno]) {
+		if (size > epc_features->bar_fixed_size[barno]) {
+			dev_err(dev, "Fixed BAR%d is too small for doorbell\n", barno);
+			return -EINVAL;
+		}
+		size = epc_features->bar_fixed_size[barno];
+	}
+
 	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
 	if (!mw_addr) {
 		dev_err(dev, "Failed to allocate OB address\n");
-- 
2.34.1

Re: [PATCH v2 1/1] PCI: endpoint: pci-epf-vntb: Fix transfer fail when BAR1 is fixed size

[Manivannan Sadhasivam]

On Tue, Dec 19, 2023 at 09:24:03AM -0500, Frank Li wrote:
> ntb_netdev transfer is failing when epc controller's BAR1 is fix size, such
> as 64K. Certain controller(like dwc) require memory address must be align
> with the fixed bar size.
> 
> For example:
> 	If BAR1's fix size is 64K, and other size programmable BAR's
> alignment is 4K.
> 	vntb call pci_epf_alloc_space() get 4K aligned address, like
> 0x104E31000. But root complex actually write to address 0x104E30000 when
> write BAR1.
> 
> Adds bar_fixed_size check and sets correct alignment for fixed-size BAR.
> 

Change looks fine by me, but I have a hard time understanding this commit
message.

The change just checks the size of the doorbell BAR if a fixed size BAR is used
by the controller and uses the fixed size. In the commit message you are talking
about alignment and root complex writing to the BAR which are just confusing.

Please reword this commit message to make it understandable.

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> 
> Notes:
>     Change from v1 to v2
>     - Remove unnessary set align when fix_bar_size.
> 
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 3f60128560ed0..ec3922f404efe 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -550,6 +550,14 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
>  
>  	barno = ntb->epf_ntb_bar[BAR_DB];
>  
> +	if (epc_features->bar_fixed_size[barno]) {
> +		if (size > epc_features->bar_fixed_size[barno]) {
> +			dev_err(dev, "Fixed BAR%d is too small for doorbell\n", barno);
> +			return -EINVAL;

-ENOMEM?

- Mani

> +		}
> +		size = epc_features->bar_fixed_size[barno];
> +	}
> +
>  	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
>  	if (!mw_addr) {
>  		dev_err(dev, "Failed to allocate OB address\n");
> -- 
> 2.34.1
> 

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

Re: [PATCH v2 1/1] PCI: endpoint: pci-epf-vntb: Fix transfer fail when BAR1 is fixed size

[Frank Li]

On Wed, Dec 20, 2023 at 07:57:36PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Dec 19, 2023 at 09:24:03AM -0500, Frank Li wrote:
> > ntb_netdev transfer is failing when epc controller's BAR1 is fix size, such
> > as 64K. Certain controller(like dwc) require memory address must be align
> > with the fixed bar size.
> > 
> > For example:
> > 	If BAR1's fix size is 64K, and other size programmable BAR's
> > alignment is 4K.
> > 	vntb call pci_epf_alloc_space() get 4K aligned address, like
> > 0x104E31000. But root complex actually write to address 0x104E30000 when
> > write BAR1.
> > 
> > Adds bar_fixed_size check and sets correct alignment for fixed-size BAR.
> > 
> 
> Change looks fine by me, but I have a hard time understanding this commit
> message.
> 
> The change just checks the size of the doorbell BAR if a fixed size BAR is used
> by the controller and uses the fixed size. In the commit message you are talking
> about alignment and root complex writing to the BAR which are just confusing.
> 
> Please reword this commit message to make it understandable.

Maybe I talk about too much about it. Actually, supposed it should  work if
use fixed-size bar(64K), which actually only mapped 4k, if RC only use 4k
also.

"copy from dwc spec"

But dwc iATU IATU_LWR_TARGET_ADDR_OFF_INBOUND_0
11-0
LWR_TARGET_HW
Forms the LSB's of the Lower Target part of the new address of the translated region.
Forms the LSB's of the Lower Target part of the new address of the translated region. The start address must be aligned to a CX_ATU_MIN_REGION_SIZE kB boundary (in address match mode); and to the Bar size boundary (in BAR match mode) so that these bits are always '0'. If the BAR is smaller than the iATU region size, then the iATU target address must align to the iATU region size; otherwise it must align to the BAR size.
A write to this location is ignored by the PCIe controller.
- Field size depends on log2(CX_ATU_MIN_REGION_SIZE) in address match mode.
- Field size depends on log2(BAR_MASK+1) in BAR match mode.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

For fixed size bar, BAR_MASK is fixed value. If pass down a 4k aligned
address to iATU for 64K fixed sized bar, some address bits will be
truncated. 

Hidden some hardware detail, do you think if it is enough?

"ntb_netdev transfer is failing when epc controller's BAR1 is fix size.
Adds bar_fixed_size check. If require memory bigger than BAR1's fixed size,
return -ENOMEM. If smaller than BAR1's fixed size, use BAR1's fixed size."

Frank

> 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > 
> > Notes:
> >     Change from v1 to v2
> >     - Remove unnessary set align when fix_bar_size.
> > 
> >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > index 3f60128560ed0..ec3922f404efe 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > @@ -550,6 +550,14 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
> >  
> >  	barno = ntb->epf_ntb_bar[BAR_DB];
> >  
> > +	if (epc_features->bar_fixed_size[barno]) {
> > +		if (size > epc_features->bar_fixed_size[barno]) {
> > +			dev_err(dev, "Fixed BAR%d is too small for doorbell\n", barno);
> > +			return -EINVAL;
> 
> -ENOMEM?
> 
> - Mani
> 
> > +		}
> > +		size = epc_features->bar_fixed_size[barno];
> > +	}
> > +
> >  	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
> >  	if (!mw_addr) {
> >  		dev_err(dev, "Failed to allocate OB address\n");
> > -- 
> > 2.34.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 1/1] PCI: endpoint: pci-epf-vntb: Fix transfer fail when BAR1 is fixed size

[Manivannan Sadhasivam]

On Wed, Dec 20, 2023 at 11:24:18AM -0500, Frank Li wrote:
> On Wed, Dec 20, 2023 at 07:57:36PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Dec 19, 2023 at 09:24:03AM -0500, Frank Li wrote:
> > > ntb_netdev transfer is failing when epc controller's BAR1 is fix size, such
> > > as 64K. Certain controller(like dwc) require memory address must be align
> > > with the fixed bar size.
> > > 
> > > For example:
> > > 	If BAR1's fix size is 64K, and other size programmable BAR's
> > > alignment is 4K.
> > > 	vntb call pci_epf_alloc_space() get 4K aligned address, like
> > > 0x104E31000. But root complex actually write to address 0x104E30000 when
> > > write BAR1.
> > > 
> > > Adds bar_fixed_size check and sets correct alignment for fixed-size BAR.
> > > 
> > 
> > Change looks fine by me, but I have a hard time understanding this commit
> > message.
> > 
> > The change just checks the size of the doorbell BAR if a fixed size BAR is used
> > by the controller and uses the fixed size. In the commit message you are talking
> > about alignment and root complex writing to the BAR which are just confusing.
> > 
> > Please reword this commit message to make it understandable.
> 
> Maybe I talk about too much about it. Actually, supposed it should  work if
> use fixed-size bar(64K), which actually only mapped 4k, if RC only use 4k
> also.
> 
> "copy from dwc spec"
> 
> But dwc iATU IATU_LWR_TARGET_ADDR_OFF_INBOUND_0
> 11-0
> LWR_TARGET_HW
> Forms the LSB's of the Lower Target part of the new address of the translated region.
> Forms the LSB's of the Lower Target part of the new address of the translated region. The start address must be aligned to a CX_ATU_MIN_REGION_SIZE kB boundary (in address match mode); and to the Bar size boundary (in BAR match mode) so that these bits are always '0'. If the BAR is smaller than the iATU region size, then the iATU target address must align to the iATU region size; otherwise it must align to the BAR size.
> A write to this location is ignored by the PCIe controller.
> - Field size depends on log2(CX_ATU_MIN_REGION_SIZE) in address match mode.
> - Field size depends on log2(BAR_MASK+1) in BAR match mode.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> For fixed size bar, BAR_MASK is fixed value. If pass down a 4k aligned
> address to iATU for 64K fixed sized bar, some address bits will be
> truncated. 
> 

Okay, now I understood the issue, thanks.

But this issue can happen for other BARs also, right. Since you encountered the
issue with DB BAR, it doesn't mean that the issue won't happen with other BARs.
So you need to have this check in place for all the used BARs.

> Hidden some hardware detail, do you think if it is enough?
> 
> "ntb_netdev transfer is failing when epc controller's BAR1 is fix size.
> Adds bar_fixed_size check. If require memory bigger than BAR1's fixed size,
> return -ENOMEM. If smaller than BAR1's fixed size, use BAR1's fixed size."
> 

How about,

"PCI: endpoint: pci-epf-vntb: Fix transfer failure for fixed size BARs

For the inbound MEM/IO TLPs, iATU on the endpoint expects the target address to
be aligned to the size of the BAR. For configurable BARs, there is no issue
because both host and endpoint will know the exact size of the BAR region. But
for fixed size BARs available in some controllers, if the BAR size advertised by
the endpoint is not same as of the actual BAR size used in the controller, then
the MEM/IO TLPs generated by the host will not be translated properly by the
endpoint iATU.

So if the fixed size BARs are available in endpoint controllers, always use the
actual BAR size."

Also, add the Fixes tag and CC stable list since this is a bug.

- Mani

> Frank
> 
> > 
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > > 
> > > Notes:
> > >     Change from v1 to v2
> > >     - Remove unnessary set align when fix_bar_size.
> > > 
> > >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > index 3f60128560ed0..ec3922f404efe 100644
> > > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > @@ -550,6 +550,14 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
> > >  
> > >  	barno = ntb->epf_ntb_bar[BAR_DB];
> > >  
> > > +	if (epc_features->bar_fixed_size[barno]) {
> > > +		if (size > epc_features->bar_fixed_size[barno]) {
> > > +			dev_err(dev, "Fixed BAR%d is too small for doorbell\n", barno);
> > > +			return -EINVAL;
> > 
> > -ENOMEM?
> > 
> > - Mani
> > 
> > > +		}
> > > +		size = epc_features->bar_fixed_size[barno];
> > > +	}
> > > +
> > >  	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
> > >  	if (!mw_addr) {
> > >  		dev_err(dev, "Failed to allocate OB address\n");
> > > -- 
> > > 2.34.1
> > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v2 1/1] PCI: endpoint: pci-epf-vntb: Fix transfer fail when BAR1 is fixed size

[Frank Li]

On Wed, Dec 20, 2023 at 11:11:13PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Dec 20, 2023 at 11:24:18AM -0500, Frank Li wrote:
> > On Wed, Dec 20, 2023 at 07:57:36PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Dec 19, 2023 at 09:24:03AM -0500, Frank Li wrote:
> > > > ntb_netdev transfer is failing when epc controller's BAR1 is fix size, such
> > > > as 64K. Certain controller(like dwc) require memory address must be align
> > > > with the fixed bar size.
> > > > 
> > > > For example:
> > > > 	If BAR1's fix size is 64K, and other size programmable BAR's
> > > > alignment is 4K.
> > > > 	vntb call pci_epf_alloc_space() get 4K aligned address, like
> > > > 0x104E31000. But root complex actually write to address 0x104E30000 when
> > > > write BAR1.
> > > > 
> > > > Adds bar_fixed_size check and sets correct alignment for fixed-size BAR.
> > > > 
> > > 
> > > Change looks fine by me, but I have a hard time understanding this commit
> > > message.
> > > 
> > > The change just checks the size of the doorbell BAR if a fixed size BAR is used
> > > by the controller and uses the fixed size. In the commit message you are talking
> > > about alignment and root complex writing to the BAR which are just confusing.
> > > 
> > > Please reword this commit message to make it understandable.
> > 
> > Maybe I talk about too much about it. Actually, supposed it should  work if
> > use fixed-size bar(64K), which actually only mapped 4k, if RC only use 4k
> > also.
> > 
> > "copy from dwc spec"
> > 
> > But dwc iATU IATU_LWR_TARGET_ADDR_OFF_INBOUND_0
> > 11-0
> > LWR_TARGET_HW
> > Forms the LSB's of the Lower Target part of the new address of the translated region.
> > Forms the LSB's of the Lower Target part of the new address of the translated region. The start address must be aligned to a CX_ATU_MIN_REGION_SIZE kB boundary (in address match mode); and to the Bar size boundary (in BAR match mode) so that these bits are always '0'. If the BAR is smaller than the iATU region size, then the iATU target address must align to the iATU region size; otherwise it must align to the BAR size.
> > A write to this location is ignored by the PCIe controller.
> > - Field size depends on log2(CX_ATU_MIN_REGION_SIZE) in address match mode.
> > - Field size depends on log2(BAR_MASK+1) in BAR match mode.
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > For fixed size bar, BAR_MASK is fixed value. If pass down a 4k aligned
> > address to iATU for 64K fixed sized bar, some address bits will be
> > truncated. 
> > 
> 
> Okay, now I understood the issue, thanks.
> 
> But this issue can happen for other BARs also, right. Since you encountered the
> issue with DB BAR, it doesn't mean that the issue won't happen with other BARs.
> So you need to have this check in place for all the used BARs.

Config BAR already consider it. Memory windows bar is little complex. I
need do some tests, let me finish 

https://lore.kernel.org/imx/20231211215842.134823-1-Frank.Li@nxp.com/T/#t

Then I will can add more SOC, which all bars is fixed size.

Frank

> 
> > Hidden some hardware detail, do you think if it is enough?
> > 
> > "ntb_netdev transfer is failing when epc controller's BAR1 is fix size.
> > Adds bar_fixed_size check. If require memory bigger than BAR1's fixed size,
> > return -ENOMEM. If smaller than BAR1's fixed size, use BAR1's fixed size."
> > 
> 
> How about,
> 
> "PCI: endpoint: pci-epf-vntb: Fix transfer failure for fixed size BARs
> 
> For the inbound MEM/IO TLPs, iATU on the endpoint expects the target address to
> be aligned to the size of the BAR. For configurable BARs, there is no issue
> because both host and endpoint will know the exact size of the BAR region. But
> for fixed size BARs available in some controllers, if the BAR size advertised by
> the endpoint is not same as of the actual BAR size used in the controller, then
> the MEM/IO TLPs generated by the host will not be translated properly by the
> endpoint iATU.
> 
> So if the fixed size BARs are available in endpoint controllers, always use the
> actual BAR size."
> 
> Also, add the Fixes tag and CC stable list since this is a bug.
> 
> - Mani
> 
> > Frank
> > 
> > > 
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > > 
> > > > Notes:
> > > >     Change from v1 to v2
> > > >     - Remove unnessary set align when fix_bar_size.
> > > > 
> > > >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > index 3f60128560ed0..ec3922f404efe 100644
> > > > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > @@ -550,6 +550,14 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
> > > >  
> > > >  	barno = ntb->epf_ntb_bar[BAR_DB];
> > > >  
> > > > +	if (epc_features->bar_fixed_size[barno]) {
> > > > +		if (size > epc_features->bar_fixed_size[barno]) {
> > > > +			dev_err(dev, "Fixed BAR%d is too small for doorbell\n", barno);
> > > > +			return -EINVAL;
> > > 
> > > -ENOMEM?
> > > 
> > > - Mani
> > > 
> > > > +		}
> > > > +		size = epc_features->bar_fixed_size[barno];
> > > > +	}
> > > > +
> > > >  	mw_addr = pci_epf_alloc_space(ntb->epf, size, barno, align, 0);
> > > >  	if (!mw_addr) {
> > > >  		dev_err(dev, "Failed to allocate OB address\n");
> > > > -- 
> > > > 2.34.1
> > > > 
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்