[2/3] dp8393x: fix dp8393x_receive()
diff mbox series

Message ID 20191102171511.31881-3-laurent@vivier.eu
State New
Headers show
Series
  • dp8393x: fix problems detected with Quadra 800 machine
Related show

Commit Message

Laurent Vivier Nov. 2, 2019, 5:15 p.m. UTC
address_space_rw() access size must be multiplied by the width.

This fixes DHCP for Q800 guest.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé Nov. 2, 2019, 7:41 p.m. UTC | #1
On 11/2/19 6:15 PM, Laurent Vivier wrote:
> address_space_rw() access size must be multiplied by the width.
> 
> This fixes DHCP for Q800 guest.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 85d3f3788e..b8c4473f99 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       } else {
>           dp8393x_put(s, width, 0, 0); /* in_use */
>           address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 6 * width,
> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, sizeof(uint16_t), 1);
> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);

Which is following:

     if (s->regs[SONIC_LLFA] & 0x1) {
         size = sizeof(uint16_t) * 1 * width;

So OK (you describe 'width' but use 'size').

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>           s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>           s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
>
Laurent Vivier Nov. 2, 2019, 7:58 p.m. UTC | #2
Le 02/11/2019 à 20:41, Philippe Mathieu-Daudé a écrit :
> On 11/2/19 6:15 PM, Laurent Vivier wrote:
>> address_space_rw() access size must be multiplied by the width.
>>
>> This fixes DHCP for Q800 guest.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   hw/net/dp8393x.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index 85d3f3788e..b8c4473f99 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
>> const uint8_t * buf,
>>       } else {
>>           dp8393x_put(s, width, 0, 0); /* in_use */
>>           address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t)
>> * 6 * width,
>> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
>> sizeof(uint16_t), 1);
>> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
> 
> Which is following:
> 
>     if (s->regs[SONIC_LLFA] & 0x1) {
>         size = sizeof(uint16_t) * 1 * width;

but we have "size = sizeof(uint16_t) * width;" later.

This file needs cleanup...

> So OK (you describe 'width' but use 'size').
>

I meant the access size "sizeof(uint16_t)" must be adjusted by the width
of the bus (defined by (s->regs[SONIC_DCR] & SONIC_DCR_DW). [1]

And this is exactly what contains "size".

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

Thanks,
Laurent

[1] DP83932C-20/25/33 MHz SONIC (tm) Systems-Oriented Network Interface
Controller

"1.3 DATA WIDTH AND BYTE ORDERING

The SONIC can be programmed to operate with either
32-bit or 16-bit wide memory. The data width is configured
during initialization by programming the DW bit in the Data
Configuration Register (DCR, Section 4.3.2). If the 16-bit
data path is selected, data is driven on pins D15– D0."
Hervé Poussineau Nov. 5, 2019, 9:06 p.m. UTC | #3
Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
> address_space_rw() access size must be multiplied by the width.
> 
> This fixes DHCP for Q800 guest.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 85d3f3788e..b8c4473f99 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>       } else {
>           dp8393x_put(s, width, 0, 0); /* in_use */
>           address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 6 * width,
> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, sizeof(uint16_t), 1);
> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>           s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>           s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
> 

This patch is problematic.
The code was initially created with "size".
It was changed in 409b52bfe199d8106dadf7c5ff3d88d2228e89b5 to fix networking in NetBSD 5.1.

To test with NetBSD 5.1
- boot the installer (arccd-5.1.iso)
- choose (S)hell option
- "ifconfig sn0 10.0.2.15 netmask 255.255.255.0"
- "route add default 10.0.2.2"
- networking should work (I test with "ftp 212.27.63.3")

Without this patch, I get the FTP banner.
With this patch, connection can't be established.

In datasheet page 17, you can see the "Receive Descriptor Format", which contains the in_use field.
It is clearly said that RXpkt.in_use is 16 bit wide, and that the bits 16-31 are not used in 32-bit mode.

So, I don't see why you need to clear 32 bits in 32-bit mode. Maybe you need to clear only the other
16 bits ? Maybe it depends of endianness ?

Regards,

Hervé
Laurent Vivier Nov. 5, 2019, 9:53 p.m. UTC | #4
Le 05/11/2019 à 22:06, Hervé Poussineau a écrit :
> Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
>> address_space_rw() access size must be multiplied by the width.
>>
>> This fixes DHCP for Q800 guest.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   hw/net/dp8393x.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index 85d3f3788e..b8c4473f99 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
>> const uint8_t * buf,
>>       } else {
>>           dp8393x_put(s, width, 0, 0); /* in_use */
>>           address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t)
>> * 6 * width,
>> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
>> sizeof(uint16_t), 1);
>> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
>>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>>           s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>>           s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
>> (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
>>
> 
> This patch is problematic.
> The code was initially created with "size".
> It was changed in 409b52bfe199d8106dadf7c5ff3d88d2228e89b5 to fix
> networking in NetBSD 5.1.
> 
> To test with NetBSD 5.1
> - boot the installer (arccd-5.1.iso)
> - choose (S)hell option
> - "ifconfig sn0 10.0.2.15 netmask 255.255.255.0"
> - "route add default 10.0.2.2"
> - networking should work (I test with "ftp 212.27.63.3")

I've the firmware from
http://hpoussineau.free.fr/qemu/firmware/magnum-4000/setup.zip
Which file to use? NTPROM.RAW?

> Without this patch, I get the FTP banner.
> With this patch, connection can't be established.
> 
> In datasheet page 17, you can see the "Receive Descriptor Format", which
> contains the in_use field.
> It is clearly said that RXpkt.in_use is 16 bit wide, and that the bits
> 16-31 are not used in 32-bit mode.
> 
> So, I don't see why you need to clear 32 bits in 32-bit mode. Maybe you
> need to clear only the other
> 16 bits ? Maybe it depends of endianness ?

Thank you for the details. I think the problem should likely come from
the endianness.

The offset must be adjusted according to the access mode (endianness and
size).

The following patch fixes the problem for me, and should not break other
targets:

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 85d3f3788e..3d991af163 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -831,9 +831,15 @@ static ssize_t dp8393x_receive(NetClientState *nc,
const uint8_t * buf,
         /* EOL detected */
         s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
     } else {
-        dp8393x_put(s, width, 0, 0); /* in_use */
-        address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 6
* width,
-            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
sizeof(uint16_t), 1);
+        /* Clear in_use, but it is always 16bit wide */
+        int offset = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
+        if (s->big_endian && width == 2) {
+            /* we need to adjust the offset of the 16bit field */
+            offset += sizeof(uint16_t);
+        }
+        s->data[0] = 0;
+        address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
+                         (uint8_t *)s->data, sizeof(uint16_t), 1);
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
         s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
         s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
(((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);

What is your opinion?

Thanks,
Laurent
Hervé Poussineau Nov. 6, 2019, 6:13 a.m. UTC | #5
Le 05/11/2019 à 22:53, Laurent Vivier a écrit :
> Le 05/11/2019 à 22:06, Hervé Poussineau a écrit :
>> Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
>>> address_space_rw() access size must be multiplied by the width.
>>>
>>> This fixes DHCP for Q800 guest.
>>>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>>    hw/net/dp8393x.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>>> index 85d3f3788e..b8c4473f99 100644
>>> --- a/hw/net/dp8393x.c
>>> +++ b/hw/net/dp8393x.c
>>> @@ -833,7 +833,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
>>> const uint8_t * buf,
>>>        } else {
>>>            dp8393x_put(s, width, 0, 0); /* in_use */
>>>            address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t)
>>> * 6 * width,
>>> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
>>> sizeof(uint16_t), 1);
>>> +            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
>>>            s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>>>            s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>>>            s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
>>> (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
>>>
>>
>> This patch is problematic.
>> The code was initially created with "size".
>> It was changed in 409b52bfe199d8106dadf7c5ff3d88d2228e89b5 to fix
>> networking in NetBSD 5.1.
>>
>> To test with NetBSD 5.1
>> - boot the installer (arccd-5.1.iso)
>> - choose (S)hell option
>> - "ifconfig sn0 10.0.2.15 netmask 255.255.255.0"
>> - "route add default 10.0.2.2"
>> - networking should work (I test with "ftp 212.27.63.3")
> 
> I've the firmware from
> http://hpoussineau.free.fr/qemu/firmware/magnum-4000/setup.zip
> Which file to use? NTPROM.RAW?
> 
>> Without this patch, I get the FTP banner.
>> With this patch, connection can't be established.
>>
>> In datasheet page 17, you can see the "Receive Descriptor Format", which
>> contains the in_use field.
>> It is clearly said that RXpkt.in_use is 16 bit wide, and that the bits
>> 16-31 are not used in 32-bit mode.
>>
>> So, I don't see why you need to clear 32 bits in 32-bit mode. Maybe you
>> need to clear only the other
>> 16 bits ? Maybe it depends of endianness ?
> 
> Thank you for the details. I think the problem should likely come from
> the endianness.
> 
> The offset must be adjusted according to the access mode (endianness and
> size).
> 
> The following patch fixes the problem for me, and should not break other
> targets:
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 85d3f3788e..3d991af163 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -831,9 +831,15 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> const uint8_t * buf,
>           /* EOL detected */
>           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>       } else {
> -        dp8393x_put(s, width, 0, 0); /* in_use */
> -        address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 6
> * width,
> -            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data,
> sizeof(uint16_t), 1);
> +        /* Clear in_use, but it is always 16bit wide */
> +        int offset = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
> +        if (s->big_endian && width == 2) {
> +            /* we need to adjust the offset of the 16bit field */
> +            offset += sizeof(uint16_t);
> +        }
> +        s->data[0] = 0;
> +        address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
> +                         (uint8_t *)s->data, sizeof(uint16_t), 1);
>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>           s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
>           s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
> (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
> 
> What is your opinion?

This one works for NetBSD.
Tested-by: Hervé Poussineau <hpoussin@reactos.org>

Patch
diff mbox series

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 85d3f3788e..b8c4473f99 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -833,7 +833,7 @@  static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     } else {
         dp8393x_put(s, width, 0, 0); /* in_use */
         address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 6 * width,
-            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, sizeof(uint16_t), 1);
+            MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
         s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
         s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) | (((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);