[PATCH] NTB: Register and offset values fix for memory window

[PATCH] NTB: Register and offset values fix for memory window

[Shyam Sundar S K]

Due to incorrect limit and translation register values, NTB link was
going down when the memory window translation was setup. Made appropriate
changes as per spec.

Also, fixed the limit register values for BAR1, which was overlapping
with the BAR23 address.

Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
Acked-by: Xiangliang Yu <Xiangliang.Yu@amd.com>
Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
---
 drivers/ntb/hw/amd/ntb_hw_amd.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
index 6ccba0d..9bbe3e0 100644
--- a/drivers/ntb/hw/amd/ntb_hw_amd.c
+++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
@@ -138,11 +138,11 @@ static int amd_ntb_mw_set_trans(struct ntb_dev *ntb, int idx,
 	base_addr = pci_resource_start(ndev->ntb.pdev, bar);

 	if (bar != 1) {
-		xlat_reg = AMD_BAR23XLAT_OFFSET + ((bar - 2) << 3);
-		limit_reg = AMD_BAR23LMT_OFFSET + ((bar - 2) << 3);
+		xlat_reg = AMD_BAR23XLAT_OFFSET + ((bar - 2) << 2);
+		limit_reg = AMD_BAR23LMT_OFFSET + ((bar - 2) << 2);

 		/* Set the limit if supported */
-		limit = base_addr + size;
+		limit = size;

 		/* set and verify setting the translation address */
 		write64(addr, peer_mmio + xlat_reg);
@@ -164,14 +164,9 @@ static int amd_ntb_mw_set_trans(struct ntb_dev *ntb, int idx,
 		xlat_reg = AMD_BAR1XLAT_OFFSET;
 		limit_reg = AMD_BAR1LMT_OFFSET;

-		/* split bar addr range must all be 32 bit */
-		if (addr & (~0ull << 32))
-			return -EINVAL;
-		if ((addr + size) & (~0ull << 32))
-			return -EINVAL;

 		/* Set the limit if supported */
-		limit = base_addr + size;
+		limit = size;

 		/* set and verify setting the translation address */
 		write64(addr, peer_mmio + xlat_reg);
@@ -376,13 +371,11 @@ static u32 amd_ntb_spad_read(struct ntb_dev *ntb, int idx)
 {
 	struct amd_ntb_dev *ndev = ntb_ndev(ntb);
 	void __iomem *mmio = ndev->self_mmio;
-	u32 offset;

-	if (idx < 0 || idx >= ndev->spad_count)
+	if (idx < 0 || idx >= (ndev->spad_count + 4))
 		return 0;

-	offset = ndev->self_spad + (idx << 2);
-	return readl(mmio + AMD_SPAD_OFFSET + offset);
+	return readl(mmio + AMD_SPAD_OFFSET + (idx << 2));
 }

 static int amd_ntb_spad_write(struct ntb_dev *ntb,
@@ -392,11 +385,10 @@ static int amd_ntb_spad_write(struct ntb_dev *ntb,
 	void __iomem *mmio = ndev->self_mmio;
 	u32 offset;

-	if (idx < 0 || idx >= ndev->spad_count)
+	if (idx < 0 || idx >= (ndev->spad_count + 4))
 		return -EINVAL;

-	offset = ndev->self_spad + (idx << 2);
-	writel(val, mmio + AMD_SPAD_OFFSET + offset);
+	writel(val, mmio + AMD_SPAD_OFFSET + (idx << 2));

 	return 0;
 }
@@ -405,13 +397,11 @@ static u32 amd_ntb_peer_spad_read(struct ntb_dev *ntb, int idx)
 {
 	struct amd_ntb_dev *ndev = ntb_ndev(ntb);
 	void __iomem *mmio = ndev->self_mmio;
-	u32 offset;

-	if (idx < 0 || idx >= ndev->spad_count)
+	if (idx < 0 || idx >= (ndev->spad_count + 4))
 		return -EINVAL;

-	offset = ndev->peer_spad + (idx << 2);
-	return readl(mmio + AMD_SPAD_OFFSET + offset);
+	return readl(mmio + AMD_SPAD_OFFSET + (idx << 2));
 }

 static int amd_ntb_peer_spad_write(struct ntb_dev *ntb,
@@ -421,11 +411,10 @@ static int amd_ntb_peer_spad_write(struct ntb_dev *ntb,
 	void __iomem *mmio = ndev->self_mmio;
 	u32 offset;

-	if (idx < 0 || idx >= ndev->spad_count)
+	if (idx < 0 || idx >= (ndev->spad_count + 4))
 		return -EINVAL;

-	offset = ndev->peer_spad + (idx << 2);
-	writel(val, mmio + AMD_SPAD_OFFSET + offset);
+	writel(val, mmio + AMD_SPAD_OFFSET + (idx << 2));

 	return 0;
 }
-- 
2.7.4

RE: [PATCH] NTB: Register and offset values fix for memory window

[Allen Hubbe]

From: Shyam Sundar S K
> Due to incorrect limit and translation register values, NTB link was
> going down when the memory window translation was setup. Made appropriate
> changes as per spec.
> 
> Also, fixed the limit register values for BAR1, which was overlapping
> with the BAR23 address.
> 
> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
> Acked-by: Xiangliang Yu <Xiangliang.Yu@amd.com>
> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
> ---

> @@ -376,13 +371,11 @@ static u32 amd_ntb_spad_read(struct ntb_dev *ntb, int idx)
>  {
>  	struct amd_ntb_dev *ndev = ntb_ndev(ntb);
>  	void __iomem *mmio = ndev->self_mmio;
> -	u32 offset;
> 
> -	if (idx < 0 || idx >= ndev->spad_count)
> +	if (idx < 0 || idx >= (ndev->spad_count + 4))

Why does this change add four to the upper end of the range check?  Does spad_count have the wrong number of spads?

>  		return 0;
> 
> -	offset = ndev->self_spad + (idx << 2);
> -	return readl(mmio + AMD_SPAD_OFFSET + offset);
> +	return readl(mmio + AMD_SPAD_OFFSET + (idx << 2));

The self_spad is used for sharing the spads of a single ntb.  It is the offset of the first or second half of the spads, and the peer self_spad is the other half.  From this change, can we assume that a single-ntb topology will not be supported?

Re: [PATCH] NTB: Register and offset values fix for memory window

[Shyam Sundar S K]

On 11/14/2016 8:05 PM, Allen Hubbe wrote:
> From: Shyam Sundar S K
>> Due to incorrect limit and translation register values, NTB link was
>> going down when the memory window translation was setup. Made appropriate
>> changes as per spec.
>>
>> Also, fixed the limit register values for BAR1, which was overlapping
>> with the BAR23 address.
>>
>> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
>> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>> Acked-by: Xiangliang Yu <Xiangliang.Yu@amd.com>
>> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>> ---
> 
>> @@ -376,13 +371,11 @@ static u32 amd_ntb_spad_read(struct ntb_dev *ntb, int idx)
>>  {
>>  	struct amd_ntb_dev *ndev = ntb_ndev(ntb);
>>  	void __iomem *mmio = ndev->self_mmio;
>> -	u32 offset;
>>
>> -	if (idx < 0 || idx >= ndev->spad_count)
>> +	if (idx < 0 || idx >= (ndev->spad_count + 4))
> 
> Why does this change add four to the upper end of the range check?  Does spad_count have the wrong number of spads?
> 
>>  		return 0;
>>
>> -	offset = ndev->self_spad + (idx << 2);
>> -	return readl(mmio + AMD_SPAD_OFFSET + offset);
>> +	return readl(mmio + AMD_SPAD_OFFSET + (idx << 2));
> 
> The self_spad is used for sharing the spads of a single ntb.  It is the offset of the first or second half of the spads, and the peer self_spad is the other half.  From this change, can we assume that a single-ntb topology will not be supported?
> 
This patch has more to with the NTB driver becoming functional on AMD platforms.
We were reading from the incorrect offsets than the one mentioned in the AMD NTB HW spec. So changed it accordingly.

Re: [PATCH] NTB: Register and offset values fix for memory window

[Jon Mason]

On Tue, Nov 15, 2016 at 2:04 AM, Shyam Sundar S K <ssundark@amd.com> wrote:
>
>
> On 11/14/2016 8:05 PM, Allen Hubbe wrote:
>> From: Shyam Sundar S K
>>> Due to incorrect limit and translation register values, NTB link was
>>> going down when the memory window translation was setup. Made appropriate
>>> changes as per spec.
>>>
>>> Also, fixed the limit register values for BAR1, which was overlapping
>>> with the BAR23 address.
>>>
>>> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
>>> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>> Acked-by: Xiangliang Yu <Xiangliang.Yu@amd.com>
>>> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>>> ---
>>
>>> @@ -376,13 +371,11 @@ static u32 amd_ntb_spad_read(struct ntb_dev *ntb, int idx)
>>>  {
>>>      struct amd_ntb_dev *ndev = ntb_ndev(ntb);
>>>      void __iomem *mmio = ndev->self_mmio;
>>> -    u32 offset;
>>>
>>> -    if (idx < 0 || idx >= ndev->spad_count)
>>> +    if (idx < 0 || idx >= (ndev->spad_count + 4))
>>
>> Why does this change add four to the upper end of the range check?  Does spad_count have the wrong number of spads?
>>
>>>              return 0;
>>>
>>> -    offset = ndev->self_spad + (idx << 2);
>>> -    return readl(mmio + AMD_SPAD_OFFSET + offset);
>>> +    return readl(mmio + AMD_SPAD_OFFSET + (idx << 2));
>>
>> The self_spad is used for sharing the spads of a single ntb.  It is the offset of the first or second half of the spads, and the peer self_spad is the other half.  From this change, can we assume that a single-ntb topology will not be supported?
>>
> This patch has more to with the NTB driver becoming functional on AMD platforms.
> We were reading from the incorrect offsets than the one mentioned in the AMD NTB HW spec. So changed it accordingly.

I believe the point Alan was making was that if the SPAD count is +4,
then perhaps the value in spad_count should be +4.  Are there 16 or 20
SPADs?

Thanks,
Jon


>
> --
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com.
> To post to this group, send email to linux-ntb@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/ca26d2ef-f3ab-3713-eacb-cb2d05d61547%40amd.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] NTB: Register and offset values fix for memory window

[Shyam Sundar S K]

On 11/17/2016 9:09 PM, Jon Mason wrote:
> On Tue, Nov 15, 2016 at 2:04 AM, Shyam Sundar S K <ssundark@amd.com> wrote:
>>
>>
>> On 11/14/2016 8:05 PM, Allen Hubbe wrote:
>>> From: Shyam Sundar S K
>>>> Due to incorrect limit and translation register values, NTB link was
>>>> going down when the memory window translation was setup. Made appropriate
>>>> changes as per spec.
>>>>
>>>> Also, fixed the limit register values for BAR1, which was overlapping
>>>> with the BAR23 address.
>>>>
>>>> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
>>>> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>>> Acked-by: Xiangliang Yu <Xiangliang.Yu@amd.com>
>>>> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>>>> ---
>>>
>>>> @@ -376,13 +371,11 @@ static u32 amd_ntb_spad_read(struct ntb_dev *ntb, int idx)
>>>>  {
>>>>      struct amd_ntb_dev *ndev = ntb_ndev(ntb);
>>>>      void __iomem *mmio = ndev->self_mmio;
>>>> -    u32 offset;
>>>>
>>>> -    if (idx < 0 || idx >= ndev->spad_count)
>>>> +    if (idx < 0 || idx >= (ndev->spad_count + 4))
>>>
>>> Why does this change add four to the upper end of the range check?  Does spad_count have the wrong number of spads?
>>>
>>>>              return 0;
>>>>
>>>> -    offset = ndev->self_spad + (idx << 2);
>>>> -    return readl(mmio + AMD_SPAD_OFFSET + offset);
>>>> +    return readl(mmio + AMD_SPAD_OFFSET + (idx << 2));
>>>
>>> The self_spad is used for sharing the spads of a single ntb.  It is the offset of the first or second half of the spads, and the peer self_spad is the other half.  From this change, can we assume that a single-ntb topology will not be supported?
>>>
>> This patch has more to with the NTB driver becoming functional on AMD platforms.
>> We were reading from the incorrect offsets than the one mentioned in the AMD NTB HW spec. So changed it accordingly.
> 
> I believe the point Alan was making was that if the SPAD count is +4,
> then perhaps the value in spad_count should be +4.  Are there 16 or 20
> SPADs?
> 

There are 16 SPAD in our case, if I don't add that hack here eth link does not become ready.

meaning, in ntb_transport: this never gets set "nt->link_is_up = true;" reason being ntb_spad_read(ndev, MW0_SZ_HIGH + (i * 2)); and ntb_spad_read(ndev, MW0_SZ_LOW + (i * 2));
so, calls to ntb_spad_read() will have idx more than the ndev->spad_count.

Any suggestions ?

Thanks,
Shyam

> 
>>
>> --
>> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com.
>> To post to this group, send email to linux-ntb@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/ca26d2ef-f3ab-3713-eacb-cb2d05d61547%40amd.com.
>> For more options, visit https://groups.google.com/d/optout.

RE: [PATCH] NTB: Register and offset values fix for memory window

[Allen Hubbe]

From: Shyam Sundar S K
> On 11/17/2016 9:09 PM, Jon Mason wrote:
> > On Tue, Nov 15, 2016 at 2:04 AM, Shyam Sundar S K <ssundark@amd.com> wrote:
> >> On 11/14/2016 8:05 PM, Allen Hubbe wrote:
> >>> From: Shyam Sundar S K
> >>>> Due to incorrect limit and translation register values, NTB link was
> >>>> going down when the memory window translation was setup. Made appropriate
> >>>> changes as per spec.
> >>>>
> >>>> Also, fixed the limit register values for BAR1, which was overlapping
> >>>> with the BAR23 address.
> >>>>
> >>>> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
> >>>> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
> >>>> Acked-by: Xiangliang Yu <Xiangliang.Yu@amd.com>
> >>>> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
> >>>> ---
> >>>
> >>>> @@ -376,13 +371,11 @@ static u32 amd_ntb_spad_read(struct ntb_dev *ntb, int idx)
> >>>>  {
> >>>>      struct amd_ntb_dev *ndev = ntb_ndev(ntb);
> >>>>      void __iomem *mmio = ndev->self_mmio;
> >>>> -    u32 offset;
> >>>>
> >>>> -    if (idx < 0 || idx >= ndev->spad_count)
> >>>> +    if (idx < 0 || idx >= (ndev->spad_count + 4))
> >>>
> >>> Why does this change add four to the upper end of the range check?  Does spad_count
> have the wrong number of spads?
> >>>
> >>>>              return 0;
> >>>>
> >>>> -    offset = ndev->self_spad + (idx << 2);
> >>>> -    return readl(mmio + AMD_SPAD_OFFSET + offset);
> >>>> +    return readl(mmio + AMD_SPAD_OFFSET + (idx << 2));
> >>>
> >>> The self_spad is used for sharing the spads of a single ntb.  It is the offset of the
> first or second half of the spads, and the peer self_spad is the other half.  From this
> change, can we assume that a single-ntb topology will not be supported?
> >>>
> >> This patch has more to with the NTB driver becoming functional on AMD platforms.
> >> We were reading from the incorrect offsets than the one mentioned in the AMD NTB HW
> spec. So changed it accordingly.
> >
> > I believe the point Alan was making was that if the SPAD count is +4,
> > then perhaps the value in spad_count should be +4.  Are there 16 or 20
> > SPADs?
> >
> 
> There are 16 SPAD in our case, if I don't add that hack here eth link does not become
> ready.
> 
> meaning, in ntb_transport: this never gets set "nt->link_is_up = true;" reason being
> ntb_spad_read(ndev, MW0_SZ_HIGH + (i * 2)); and ntb_spad_read(ndev, MW0_SZ_LOW + (i * 2));
> so, calls to ntb_spad_read() will have idx more than the ndev->spad_count.
> 

In MW0_SZ_LOW + (i * 2), 'i' is in the range of the number of memory windows, which is greater on AMD.  The ntb_transport driver as currently written will require more scratchpads for initialization on AMD, because the AMD driver advertises more memory windows.

The changes here in ntb_hw_amd are incorrect, because they do allow read and write access out of bounds of the hardware scratchpad registers.  This only appears to work, because ntb_netdev only creates one queue in ntb_transport.  Because ntb_netdev only creates one queue, we don't observe any trouble with the misconfigured third memory window, which is configured with bogus values from non-scratchpad hardware registers.  If a driver allocated a third queue, I expect there would be trouble.

> Any suggestions ?
>

Instead, limit the number of memory windows actually used by ntb_transport, such that ntb_transport can configure those memory windows using only the available scratchpads.  If there are not enough scratchpads to exchange values for all of the memory windows, then ntb_transport should limit itself to use fewer memory windows.

Re: [PATCH] NTB: Register and offset values fix for memory window

[Shyam Sundar S K]

On 11/22/2016 7:49 PM, Allen Hubbe wrote:
> From: Shyam Sundar S K
>> On 11/17/2016 9:09 PM, Jon Mason wrote:
>>> On Tue, Nov 15, 2016 at 2:04 AM, Shyam Sundar S K <ssundark@amd.com> wrote:
>>>> On 11/14/2016 8:05 PM, Allen Hubbe wrote:
>>>>> From: Shyam Sundar S K
>>>>>> Due to incorrect limit and translation register values, NTB link was
>>>>>> going down when the memory window translation was setup. Made appropriate
>>>>>> changes as per spec.
>>>>>>
>>>>>> Also, fixed the limit register values for BAR1, which was overlapping
>>>>>> with the BAR23 address.
>>>>>>
>>>>>> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
>>>>>> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>>>>> Acked-by: Xiangliang Yu <Xiangliang.Yu@amd.com>
>>>>>> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>>>>>> ---
>>>>>
>>>>>> @@ -376,13 +371,11 @@ static u32 amd_ntb_spad_read(struct ntb_dev *ntb, int idx)
>>>>>>  {
>>>>>>      struct amd_ntb_dev *ndev = ntb_ndev(ntb);
>>>>>>      void __iomem *mmio = ndev->self_mmio;
>>>>>> -    u32 offset;
>>>>>>
>>>>>> -    if (idx < 0 || idx >= ndev->spad_count)
>>>>>> +    if (idx < 0 || idx >= (ndev->spad_count + 4))
>>>>>
>>>>> Why does this change add four to the upper end of the range check?  Does spad_count
>> have the wrong number of spads?
>>>>>
>>>>>>              return 0;
>>>>>>
>>>>>> -    offset = ndev->self_spad + (idx << 2);
>>>>>> -    return readl(mmio + AMD_SPAD_OFFSET + offset);
>>>>>> +    return readl(mmio + AMD_SPAD_OFFSET + (idx << 2));
>>>>>
>>>>> The self_spad is used for sharing the spads of a single ntb.  It is the offset of the
>> first or second half of the spads, and the peer self_spad is the other half.  From this
>> change, can we assume that a single-ntb topology will not be supported?
>>>>>
>>>> This patch has more to with the NTB driver becoming functional on AMD platforms.
>>>> We were reading from the incorrect offsets than the one mentioned in the AMD NTB HW
>> spec. So changed it accordingly.
>>>
>>> I believe the point Alan was making was that if the SPAD count is +4,
>>> then perhaps the value in spad_count should be +4.  Are there 16 or 20
>>> SPADs?
>>>
>>
>> There are 16 SPAD in our case, if I don't add that hack here eth link does not become
>> ready.
>>
>> meaning, in ntb_transport: this never gets set "nt->link_is_up = true;" reason being
>> ntb_spad_read(ndev, MW0_SZ_HIGH + (i * 2)); and ntb_spad_read(ndev, MW0_SZ_LOW + (i * 2));
>> so, calls to ntb_spad_read() will have idx more than the ndev->spad_count.
>>
> 
> In MW0_SZ_LOW + (i * 2), 'i' is in the range of the number of memory windows, which is greater on AMD.  The ntb_transport driver as currently written will require more scratchpads for initialization on AMD, because the AMD driver advertises more memory windows.
> 
> The changes here in ntb_hw_amd are incorrect, because they do allow read and write access out of bounds of the hardware scratchpad registers.  This only appears to work, because ntb_netdev only creates one queue in ntb_transport.  Because ntb_netdev only creates one queue, we don't observe any trouble with the misconfigured third memory window, which is configured with bogus values from non-scratchpad hardware registers.  If a driver allocated a third queue, I expect there would be trouble.
> 
>> Any suggestions ?
>>
> 
> Instead, limit the number of memory windows actually used by ntb_transport, such that ntb_transport can configure those memory windows using only the available scratchpads.  

Thank you for your valuable inputs. Can you please elaborate more on this point ? How to limit memory windows in ntb_transport ? We have 16 scratchpad, 8 shared across each NTB's and 3 memory windows.

If there are not enough scratchpads to exchange values for all of the memory windows, then ntb_transport should limit itself to use fewer memory windows.
> 

Re: [PATCH] NTB: Register and offset values fix for memory window

[Shyam Sundar S K]

On 11/22/2016 7:49 PM, Allen Hubbe wrote:
> From: Shyam Sundar S K
>> On 11/17/2016 9:09 PM, Jon Mason wrote:
>>> On Tue, Nov 15, 2016 at 2:04 AM, Shyam Sundar S K <ssundark@amd.com> wrote:
>>>> On 11/14/2016 8:05 PM, Allen Hubbe wrote:
>>>>> From: Shyam Sundar S K
>>>>>> Due to incorrect limit and translation register values, NTB link was
>>>>>> going down when the memory window translation was setup. Made appropriate
>>>>>> changes as per spec.
>>>>>>
>>>>>> Also, fixed the limit register values for BAR1, which was overlapping
>>>>>> with the BAR23 address.
>>>>>>
>>>>>> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
>>>>>> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>>>>> Acked-by: Xiangliang Yu <Xiangliang.Yu@amd.com>
>>>>>> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>>>>>> ---
>>>>>
>>>>>> @@ -376,13 +371,11 @@ static u32 amd_ntb_spad_read(struct ntb_dev *ntb, int idx)
>>>>>>  {
>>>>>>      struct amd_ntb_dev *ndev = ntb_ndev(ntb);
>>>>>>      void __iomem *mmio = ndev->self_mmio;
>>>>>> -    u32 offset;
>>>>>>
>>>>>> -    if (idx < 0 || idx >= ndev->spad_count)
>>>>>> +    if (idx < 0 || idx >= (ndev->spad_count + 4))
>>>>>
>>>>> Why does this change add four to the upper end of the range check?  Does spad_count
>> have the wrong number of spads?
>>>>>
>>>>>>              return 0;
>>>>>>
>>>>>> -    offset = ndev->self_spad + (idx << 2);
>>>>>> -    return readl(mmio + AMD_SPAD_OFFSET + offset);
>>>>>> +    return readl(mmio + AMD_SPAD_OFFSET + (idx << 2));
>>>>>
>>>>> The self_spad is used for sharing the spads of a single ntb.  It is the offset of the
>> first or second half of the spads, and the peer self_spad is the other half.  From this
>> change, can we assume that a single-ntb topology will not be supported?
>>>>>
>>>> This patch has more to with the NTB driver becoming functional on AMD platforms.
>>>> We were reading from the incorrect offsets than the one mentioned in the AMD NTB HW
>> spec. So changed it accordingly.
>>>
>>> I believe the point Alan was making was that if the SPAD count is +4,
>>> then perhaps the value in spad_count should be +4.  Are there 16 or 20
>>> SPADs?
>>>
>>
>> There are 16 SPAD in our case, if I don't add that hack here eth link does not become
>> ready.
>>
>> meaning, in ntb_transport: this never gets set "nt->link_is_up = true;" reason being
>> ntb_spad_read(ndev, MW0_SZ_HIGH + (i * 2)); and ntb_spad_read(ndev, MW0_SZ_LOW + (i * 2));
>> so, calls to ntb_spad_read() will have idx more than the ndev->spad_count.
>>
> 
> In MW0_SZ_LOW + (i * 2), 'i' is in the range of the number of memory windows, which is greater on AMD.  The ntb_transport driver as currently written will require more scratchpads for initialization on AMD, because the AMD driver advertises more memory windows.
> 
> The changes here in ntb_hw_amd are incorrect, because they do allow read and write access out of bounds of the hardware scratchpad registers.  This only appears to work, because ntb_netdev only creates one queue in ntb_transport.  Because ntb_netdev only creates one queue, we don't observe any trouble with the misconfigured third memory window, which is configured with bogus values from non-scratchpad hardware registers.  If a driver allocated a third queue, I expect there would be trouble.
> 
>> Any suggestions ?
>>
> 
> Instead, limit the number of memory windows actually used by ntb_transport, such that ntb_transport can configure those memory windows using only the available scratchpads.  If there are not enough scratchpads to exchange values for all of the memory windows, then ntb_transport should limit itself to use fewer memory windows.
> 

Allen, I get your point. So, you are asking to limit the number of memory windows we advertise in our driver since there are less number of scratchpads available when we have to configure the 3rd
window. I believe you are asking me to do

-       ndev->mw_count = AMD_MW_CNT;
+       ndev->mw_count = AMD_MW_CNT - 1;

in amd_init_ntb(). Is this understanding correct ?

RE: [PATCH] NTB: Register and offset values fix for memory window

[Allen Hubbe]

From: Shyam Sundar S K
> On 11/22/2016 7:49 PM, Allen Hubbe wrote:
> > Instead, limit the number of memory windows actually used by ntb_transport, such that
> ntb_transport can configure those memory windows using only the available scratchpads.  If
> there are not enough scratchpads to exchange values for all of the memory windows, then
> ntb_transport should limit itself to use fewer memory windows.
> >
> 
> Allen, I get your point. So, you are asking to limit the number of memory windows we
> advertise in our driver since there are less number of scratchpads available when we have
> to configure the 3rd
> window. I believe you are asking me to do
> 
> -       ndev->mw_count = AMD_MW_CNT;
> +       ndev->mw_count = AMD_MW_CNT - 1;
> 
> in amd_init_ntb(). Is this understanding correct ?

That would work, but it would limit other transport drivers from using all the memory windows of your device.
 
This is the change I meant to describe, in ntb_transport:

static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev)
{
...
	mw_count = ntb_mw_count(ndev);

Instead of this:

	nt->mw_count = mw_count;

Limit the mw count like this:

	spad_count = ntb_spad_count(ndev);

	max_mw_count_for_spads = some_arithmetic(spad_count);

	nt->mw_count = min(mw_count, max_mw_count_for_spads);

Re: [PATCH] NTB: Register and offset values fix for memory window

[Shyam Sundar S K]

On 11/29/2016 9:33 PM, Allen Hubbe wrote:
> From: Shyam Sundar S K
>> On 11/22/2016 7:49 PM, Allen Hubbe wrote:
>>> Instead, limit the number of memory windows actually used by ntb_transport, such that
>> ntb_transport can configure those memory windows using only the available scratchpads.  If
>> there are not enough scratchpads to exchange values for all of the memory windows, then
>> ntb_transport should limit itself to use fewer memory windows.
>>>
>>
>> Allen, I get your point. So, you are asking to limit the number of memory windows we
>> advertise in our driver since there are less number of scratchpads available when we have
>> to configure the 3rd
>> window. I believe you are asking me to do
>>
>> -       ndev->mw_count = AMD_MW_CNT;
>> +       ndev->mw_count = AMD_MW_CNT - 1;
>>
>> in amd_init_ntb(). Is this understanding correct ?
> 
> That would work, but it would limit other transport drivers from using all the memory windows of your device.
>  
> This is the change I meant to describe, in ntb_transport:
> 
> static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev)
> {
> ...
> 	mw_count = ntb_mw_count(ndev);
> 
> Instead of this:
> 
> 	nt->mw_count = mw_count;
> 
> Limit the mw count like this:
> 
> 	spad_count = ntb_spad_count(ndev);
> 
> 	max_mw_count_for_spads = some_arithmetic(spad_count);
> 
> 	nt->mw_count = min(mw_count, max_mw_count_for_spads);
> 
> 
Ok. Got it. Submitting two separate patches now. One has the change in AMD and another in transport driver.