Re: [PATCH 2/3] dp8393x: fix dp8393x_receive()

From: Laurent Vivier <laurent@vivier.eu>
To: "Hervé Poussineau" <hpoussin@reactos.org>, qemu-devel@nongnu.org
Cc: Jason Wang <jasowang@redhat.com>
Subject: Re: [PATCH 2/3] dp8393x: fix dp8393x_receive()
Date: Tue, 5 Nov 2019 22:53:58 +0100	[thread overview]
Message-ID: <74333772-6574-2f56-df2c-e7ab48773996@vivier.eu> (raw)
In-Reply-To: <b357fb5d-bae2-5ab0-7c63-4f7106fb8c4e@reactos.org>

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

