| Message ID | 20191102171511.31881-3-laurent@vivier.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dp8393x: fix problems detected with Quadra 800 machine | expand |
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); >
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."
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é
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
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>
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);
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(-)