Housekeeping while reviewing Mark's "fixes for MacOS toolbox ROM" series v2. RFC because totally untested =) Just compiled. Mark Cave-Ayland (2): dp8393x: fix CAM descriptor entry index dp8393x: don't force 32-bit register access Philippe Mathieu-Daudé (4): dp8393x: Restrict bus access to 16/32-bit operations dp8393x: Store CAM registers as 16-bit dp8393x: Replace address_space_rw(is_write=1) by address_space_write() dp8393x: Rewrite dp8393x_get() / dp8393x_put() hw/net/dp8393x.c | 191 +++++++++++++++++++---------------------------- 1 file changed, 76 insertions(+), 115 deletions(-) -- 2.31.1
From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Currently when a LOAD CAM command is executed the entries are loaded into the CAM from memory in order which is incorrect. According to the datasheet the first entry in the CAM descriptor is the entry index which means that each descriptor may update any single entry in the CAM rather than the Nth entry. Decode the CAM entry index and use it store the descriptor in the appropriate slot in the CAM. This fixes the issue where the MacOS toolbox loads a single CAM descriptor into the final slot in order to perform a loopback test which must succeed before the Ethernet port is enabled. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Tested-by: Finn Thain <fthain@linux-m68k.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-Id: <20210625065401.30170-10-mark.cave-ayland@ilande.co.uk> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/net/dp8393x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 252c0a26641..11810c9b600 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -270,7 +270,7 @@ static void dp8393x_update_irq(dp8393xState *s) static void dp8393x_do_load_cam(dp8393xState *s) { int width, size; - uint16_t index = 0; + uint16_t index; width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1; size = sizeof(uint16_t) * 4 * width; @@ -279,6 +279,7 @@ static void dp8393x_do_load_cam(dp8393xState *s) /* Fill current entry */ address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED, s->data, size); + index = dp8393x_get(s, width, 0) & 0xf; s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff; s->cam[index][1] = dp8393x_get(s, width, 1) >> 8; s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff; @@ -291,7 +292,6 @@ static void dp8393x_do_load_cam(dp8393xState *s) /* Move to next entry */ s->regs[SONIC_CDC]--; s->regs[SONIC_CDP] += size; - index++; } /* Read CAM enable */ -- 2.31.1
From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that all accesses to the registers were 32-bit but this is actually not the case. The access size is determined by the CPU instruction used and not the number of physical address lines. The big_endian workaround applied to the register read/writes was actually caused by forcing the access size to 32-bit when the guest OS was using a 16-bit access. Since the registers are 16-bit then we can simply set .impl.min_access to 2 and then the memory API will automatically do the right thing for both 16-bit accesses used by Linux and 32-bit accesses used by the MacOS toolbox ROM. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses") Tested-by: Finn Thain <fthain@linux-m68k.org> Message-Id: <20210625065401.30170-9-mark.cave-ayland@ilande.co.uk> [PMD: dp8393x_ops.impl.max_access_size 4 -> 2] Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/net/dp8393x.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 11810c9b600..d16ade2b198 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -602,15 +602,14 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size) trace_dp8393x_read(reg, reg_names[reg], val, size); - return s->big_endian ? val << 16 : val; + return val; } -static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data, +static void dp8393x_write(void *opaque, hwaddr addr, uint64_t val, unsigned int size) { dp8393xState *s = opaque; int reg = addr >> s->it_shift; - uint32_t val = s->big_endian ? data >> 16 : data; trace_dp8393x_write(reg, reg_names[reg], val, size); @@ -694,8 +693,8 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data, static const MemoryRegionOps dp8393x_ops = { .read = dp8393x_read, .write = dp8393x_write, - .impl.min_access_size = 4, - .impl.max_access_size = 4, + .impl.min_access_size = 2, + .impl.max_access_size = 2, .endianness = DEVICE_NATIVE_ENDIAN, }; -- 2.31.1
Per the DP83932C datasheet from July 1995: 1. Functional Description 1.3 DATA WIDTH AND BYTE ORDERING The SONIC can be programmed to operate with either 32-bit or 16-bit wide memory. Restrict the memory bus to reject 8/64-bit accesses. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/net/dp8393x.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index d16ade2b198..c9b478c127c 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -695,6 +695,8 @@ static const MemoryRegionOps dp8393x_ops = { .write = dp8393x_write, .impl.min_access_size = 2, .impl.max_access_size = 2, + .valid.min_access_size = 2, + .valid.max_access_size = 4, .endianness = DEVICE_NATIVE_ENDIAN, }; -- 2.31.1
Per the DP83932C datasheet from July 1995: 4.0 SONIC Registers 4.1 THE CAM UNIT The Content Addressable Memory (CAM) consists of sixteen 48-bit entries for complete address filtering of network packets. Each entry corresponds to a 48-bit destination address that is user programmable and can contain any combination of Multicast or Physical addresses. Each entry is partitioned into three 16-bit CAM cells accessible through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with CAP0 corresponding to the least significant 16 bits of the Destination Address and CAP2 corresponding to the most significant bits. Store the CAM registers as 16-bit as it simplifies the code. There is no change in the migration stream. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/net/dp8393x.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index c9b478c127c..e0055b178b1 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -157,7 +157,7 @@ struct dp8393xState { MemoryRegion mmio; /* Registers */ - uint8_t cam[16][6]; + uint16_t cam[16][3]; uint16_t regs[0x40]; /* Temporaries */ @@ -280,15 +280,13 @@ static void dp8393x_do_load_cam(dp8393xState *s) address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED, s->data, size); index = dp8393x_get(s, width, 0) & 0xf; - s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff; - s->cam[index][1] = dp8393x_get(s, width, 1) >> 8; - s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff; - s->cam[index][3] = dp8393x_get(s, width, 2) >> 8; - s->cam[index][4] = dp8393x_get(s, width, 3) & 0xff; - s->cam[index][5] = dp8393x_get(s, width, 3) >> 8; - trace_dp8393x_load_cam(index, s->cam[index][0], s->cam[index][1], - s->cam[index][2], s->cam[index][3], - s->cam[index][4], s->cam[index][5]); + s->cam[index][0] = dp8393x_get(s, width, 1); + s->cam[index][1] = dp8393x_get(s, width, 2); + s->cam[index][2] = dp8393x_get(s, width, 3); + trace_dp8393x_load_cam(index, + s->cam[index][0] >> 8, s->cam[index][0] & 0xff, + s->cam[index][1] >> 8, s->cam[index][1] & 0xff, + s->cam[index][2] >> 8, s->cam[index][2] & 0xff); /* Move to next entry */ s->regs[SONIC_CDC]--; s->regs[SONIC_CDP] += size; @@ -591,8 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size) case SONIC_CAP1: case SONIC_CAP0: if (s->regs[SONIC_CR] & SONIC_CR_RST) { - val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg) + 1] << 8; - val |= s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)]; + val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)]; } break; /* All other registers have no special contraints */ @@ -987,7 +984,7 @@ static const VMStateDescription vmstate_dp8393x = { .version_id = 0, .minimum_version_id = 0, .fields = (VMStateField []) { - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6), + VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2), VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40), VMSTATE_END_OF_LIST() } -- 2.31.1
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/net/dp8393x.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index e0055b178b1..bbe241ef9db 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -814,8 +814,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, size = sizeof(uint16_t) * width; address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; dp8393x_put(s, width, 0, 0); - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED, - (uint8_t *)s->data, size, 1); + address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, + (uint8_t *)s->data, size); /* Move to next descriptor */ s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; @@ -844,8 +844,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, /* Pad short packets to keep pointers aligned */ if (rx_len < padded_len) { size = padded_len - rx_len; - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED, - (uint8_t *)"\xFF\xFF\xFF", size, 1); + address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, + (uint8_t *)"\xFF\xFF\xFF", size); address += size; } -- 2.31.1
Instead of accessing N registers via a single address_space API call using a temporary buffer (stored in the device state) and updating each register, move the address_space call in the register put/get. The load/store and word size checks are moved to put/get too. This simplifies a bit, making the code easier to read. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/net/dp8393x.c | 157 ++++++++++++++++++----------------------------- 1 file changed, 60 insertions(+), 97 deletions(-) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index bbe241ef9db..db9cfd786f5 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -162,7 +162,6 @@ struct dp8393xState { /* Temporaries */ uint8_t tx_buffer[0x10000]; - uint16_t data[12]; int loopback_packet; /* Memory access */ @@ -219,34 +218,48 @@ static uint32_t dp8393x_wt(dp8393xState *s) return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0]; } -static uint16_t dp8393x_get(dp8393xState *s, int width, int offset) +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned ofs16) { + const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; uint16_t val; - if (s->big_endian) { - val = be16_to_cpu(s->data[offset * width + width - 1]); + if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { + addr += 2 * ofs16; + if (s->big_endian) { + val = address_space_ldl_be(&s->as, addr, attrs, NULL); + } else { + val = address_space_ldl_le(&s->as, addr, attrs, NULL); + } } else { - val = le16_to_cpu(s->data[offset * width]); + addr += 1 * ofs16; + if (s->big_endian) { + val = address_space_lduw_be(&s->as, addr, attrs, NULL); + } else { + val = address_space_lduw_le(&s->as, addr, attrs, NULL); + } } + return val; } -static void dp8393x_put(dp8393xState *s, int width, int offset, - uint16_t val) +static void dp8393x_put(dp8393xState *s, + hwaddr addr, unsigned ofs16, uint16_t val) { - if (s->big_endian) { - if (width == 2) { - s->data[offset * 2] = 0; - s->data[offset * 2 + 1] = cpu_to_be16(val); + const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; + + if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { + addr += 2 * ofs16; + if (s->big_endian) { + address_space_stl_be(&s->as, addr, val, attrs, NULL); } else { - s->data[offset] = cpu_to_be16(val); + address_space_stl_le(&s->as, addr, val, attrs, NULL); } } else { - if (width == 2) { - s->data[offset * 2] = cpu_to_le16(val); - s->data[offset * 2 + 1] = 0; + addr += 1 * ofs16; + if (s->big_endian) { + address_space_stw_be(&s->as, addr, val, attrs, NULL); } else { - s->data[offset] = cpu_to_le16(val); + address_space_stw_le(&s->as, addr, val, attrs, NULL); } } } @@ -277,12 +290,10 @@ static void dp8393x_do_load_cam(dp8393xState *s) while (s->regs[SONIC_CDC] & 0x1f) { /* Fill current entry */ - address_space_read(&s->as, dp8393x_cdp(s), - MEMTXATTRS_UNSPECIFIED, s->data, size); - index = dp8393x_get(s, width, 0) & 0xf; - s->cam[index][0] = dp8393x_get(s, width, 1); - s->cam[index][1] = dp8393x_get(s, width, 2); - s->cam[index][2] = dp8393x_get(s, width, 3); + index = dp8393x_get(s, dp8393x_cdp(s), 0) & 0xf; + s->cam[index][0] = dp8393x_get(s, dp8393x_cdp(s), 1); + s->cam[index][1] = dp8393x_get(s, dp8393x_cdp(s), 2); + s->cam[index][2] = dp8393x_get(s, dp8393x_cdp(s), 3); trace_dp8393x_load_cam(index, s->cam[index][0] >> 8, s->cam[index][0] & 0xff, s->cam[index][1] >> 8, s->cam[index][1] & 0xff, @@ -293,9 +304,7 @@ static void dp8393x_do_load_cam(dp8393xState *s) } /* Read CAM enable */ - address_space_read(&s->as, dp8393x_cdp(s), - MEMTXATTRS_UNSPECIFIED, s->data, size); - s->regs[SONIC_CE] = dp8393x_get(s, width, 0); + s->regs[SONIC_CE] = dp8393x_get(s, dp8393x_cdp(s), 0); trace_dp8393x_load_cam_done(s->regs[SONIC_CE]); /* Done */ @@ -311,14 +320,12 @@ static void dp8393x_do_read_rra(dp8393xState *s) /* Read memory */ width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1; size = sizeof(uint16_t) * 4 * width; - address_space_read(&s->as, dp8393x_rrp(s), - MEMTXATTRS_UNSPECIFIED, s->data, size); /* Update SONIC registers */ - s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0); - s->regs[SONIC_CRBA1] = dp8393x_get(s, width, 1); - s->regs[SONIC_RBWC0] = dp8393x_get(s, width, 2); - s->regs[SONIC_RBWC1] = dp8393x_get(s, width, 3); + s->regs[SONIC_CRBA0] = dp8393x_get(s, dp8393x_rrp(s), 0); + s->regs[SONIC_CRBA1] = dp8393x_get(s, dp8393x_rrp(s), 1); + s->regs[SONIC_RBWC0] = dp8393x_get(s, dp8393x_rrp(s), 2); + s->regs[SONIC_RBWC1] = dp8393x_get(s, dp8393x_rrp(s), 3); trace_dp8393x_read_rra_regs(s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1], s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]); @@ -414,28 +421,22 @@ static void dp8393x_do_receiver_disable(dp8393xState *s) static void dp8393x_do_transmit_packets(dp8393xState *s) { NetClientState *nc = qemu_get_queue(s->nic); - int width, size; int tx_len, len; uint16_t i; - width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1; - while (1) { /* Read memory */ - size = sizeof(uint16_t) * 6 * width; s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA]; trace_dp8393x_transmit_packet(dp8393x_ttda(s)); - address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width, - MEMTXATTRS_UNSPECIFIED, s->data, size); tx_len = 0; /* Update registers */ - s->regs[SONIC_TCR] = dp8393x_get(s, width, 0) & 0xf000; - s->regs[SONIC_TPS] = dp8393x_get(s, width, 1); - s->regs[SONIC_TFC] = dp8393x_get(s, width, 2); - s->regs[SONIC_TSA0] = dp8393x_get(s, width, 3); - s->regs[SONIC_TSA1] = dp8393x_get(s, width, 4); - s->regs[SONIC_TFS] = dp8393x_get(s, width, 5); + s->regs[SONIC_TCR] = dp8393x_get(s, dp8393x_ttda(s), 0) & 0xf000; + s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 1); + s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 2); + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 3); + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 4); + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 5); /* Handle programmable interrupt */ if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) { @@ -457,15 +458,9 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) i++; if (i != s->regs[SONIC_TFC]) { /* Read next fragment details */ - size = sizeof(uint16_t) * 3 * width; - address_space_read(&s->as, - dp8393x_ttda(s) - + sizeof(uint16_t) * width * (4 + 3 * i), - MEMTXATTRS_UNSPECIFIED, s->data, - size); - s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0); - s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1); - s->regs[SONIC_TFS] = dp8393x_get(s, width, 2); + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 0); + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 1); + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 2); } } @@ -498,22 +493,12 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) s->regs[SONIC_TCR] |= SONIC_TCR_PTX; /* Write status */ - dp8393x_put(s, width, 0, - s->regs[SONIC_TCR] & 0x0fff); /* status */ - size = sizeof(uint16_t) * width; - address_space_write(&s->as, dp8393x_ttda(s), - MEMTXATTRS_UNSPECIFIED, s->data, size); + dp8393x_put(s, dp8393x_ttda(s), 0, s->regs[SONIC_TCR] & 0x0fff); if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) { /* Read footer of packet */ - size = sizeof(uint16_t) * width; - address_space_read(&s->as, - dp8393x_ttda(s) - + sizeof(uint16_t) * width - * (4 + 3 * s->regs[SONIC_TFC]), - MEMTXATTRS_UNSPECIFIED, s->data, - size); - s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0); + s->regs[SONIC_CTDA] = dp8393x_get(s, dp8393x_ttda(s), + 4 + 3 * s->regs[SONIC_TFC]); if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) { /* EOL detected */ break; @@ -762,7 +747,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, dp8393xState *s = qemu_get_nic_opaque(nc); int packet_type; uint32_t available, address; - int width, rx_len, padded_len; + int rx_len, padded_len; uint32_t checksum; int size; @@ -775,10 +760,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, rx_len = pkt_size + sizeof(checksum); if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { - width = 2; padded_len = ((rx_len - 1) | 3) + 1; } else { - width = 1; padded_len = ((rx_len - 1) | 1) + 1; } @@ -799,11 +782,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, /* Check for EOL */ if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { /* Are we still in resource exhaustion? */ - size = sizeof(uint16_t) * 1 * width; - address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width; - address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED, - s->data, size); - s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0); + s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5); if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { /* Still EOL ; stop reception */ return -1; @@ -811,11 +790,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, /* Link has been updated by host */ /* Clear in_use */ - size = sizeof(uint16_t) * width; - address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; - dp8393x_put(s, width, 0, 0); - address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, - (uint8_t *)s->data, size); + dp8393x_put(s, dp8393x_crda(s), 6, 0x0000); /* Move to next descriptor */ s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; @@ -869,32 +844,20 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, /* Write status to memory */ trace_dp8393x_receive_write_status(dp8393x_crda(s)); - dp8393x_put(s, width, 0, s->regs[SONIC_RCR]); /* status */ - dp8393x_put(s, width, 1, rx_len); /* byte count */ - dp8393x_put(s, width, 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */ - dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */ - dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */ - size = sizeof(uint16_t) * 5 * width; - address_space_write(&s->as, dp8393x_crda(s), - MEMTXATTRS_UNSPECIFIED, - s->data, size); + dp8393x_put(s, dp8393x_crda(s), 0, s->regs[SONIC_RCR]); /* status */ + dp8393x_put(s, dp8393x_crda(s), 1, rx_len); /* byte count */ + dp8393x_put(s, dp8393x_crda(s), 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */ + dp8393x_put(s, dp8393x_crda(s), 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */ + dp8393x_put(s, dp8393x_crda(s), 4, s->regs[SONIC_RSC]); /* seq_no */ /* Check link field */ - size = sizeof(uint16_t) * width; - address_space_read(&s->as, - dp8393x_crda(s) + sizeof(uint16_t) * 5 * width, - MEMTXATTRS_UNSPECIFIED, s->data, size); - s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0); + s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5); if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { /* EOL detected */ s->regs[SONIC_ISR] |= SONIC_ISR_RDE; } else { /* Clear in_use */ - size = sizeof(uint16_t) * width; - address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; - dp8393x_put(s, width, 0, 0); - address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, - s->data, size); + dp8393x_put(s, dp8393x_crda(s), 6, 0x0000); /* Move to next descriptor */ s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; -- 2.31.1
On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote: > From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that all accesses > to the registers were 32-bit but this is actually not the case. The access size is > determined by the CPU instruction used and not the number of physical address lines. > > The big_endian workaround applied to the register read/writes was actually caused > by forcing the access size to 32-bit when the guest OS was using a 16-bit access. > Since the registers are 16-bit then we can simply set .impl.min_access to 2 and > then the memory API will automatically do the right thing for both 16-bit accesses > used by Linux and 32-bit accesses used by the MacOS toolbox ROM. The change should work, but the commit message above needs a slight tweak - maybe something like this? Since the registers are 16-bit then we can simply set both .impl.min_access and .impl.max_access to 2 and then the memory API will automatically do the right thing for both 16-bit accesses used by Linux and 32-bit accesses used by the MacOS toolbox ROM. > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses") > Tested-by: Finn Thain <fthain@linux-m68k.org> > Message-Id: <20210625065401.30170-9-mark.cave-ayland@ilande.co.uk> > [PMD: dp8393x_ops.impl.max_access_size 4 -> 2] > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/net/dp8393x.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c > index 11810c9b600..d16ade2b198 100644 > --- a/hw/net/dp8393x.c > +++ b/hw/net/dp8393x.c > @@ -602,15 +602,14 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size) > > trace_dp8393x_read(reg, reg_names[reg], val, size); > > - return s->big_endian ? val << 16 : val; > + return val; > } > > -static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data, > +static void dp8393x_write(void *opaque, hwaddr addr, uint64_t val, > unsigned int size) > { > dp8393xState *s = opaque; > int reg = addr >> s->it_shift; > - uint32_t val = s->big_endian ? data >> 16 : data; > > trace_dp8393x_write(reg, reg_names[reg], val, size); > > @@ -694,8 +693,8 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data, > static const MemoryRegionOps dp8393x_ops = { > .read = dp8393x_read, > .write = dp8393x_write, > - .impl.min_access_size = 4, > - .impl.max_access_size = 4, > + .impl.min_access_size = 2, > + .impl.max_access_size = 2, > .endianness = DEVICE_NATIVE_ENDIAN, > }; ATB, Mark.
On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
> Per the DP83932C datasheet from July 1995:
>
> 1. Functional Description
> 1.3 DATA WIDTH AND BYTE ORDERING
>
> The SONIC can be programmed to operate with
> either 32-bit or 16-bit wide memory.
>
> Restrict the memory bus to reject 8/64-bit accesses.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/net/dp8393x.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index d16ade2b198..c9b478c127c 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -695,6 +695,8 @@ static const MemoryRegionOps dp8393x_ops = {
> .write = dp8393x_write,
> .impl.min_access_size = 2,
> .impl.max_access_size = 2,
> + .valid.min_access_size = 2,
> + .valid.max_access_size = 4,
> .endianness = DEVICE_NATIVE_ENDIAN,
> };
The changes to .valid are correct, but I don't think the above reference to the
datasheet is. My reading of the sentence in section 1.3 "The SONIC can be programmed
to operate with either 32-bit or 16-bit wide memory" (along with subsequent
references to the DW bit in the DCR) is that this section is describing bus master
transfers as per figure 1-5 instead of register accesses which is what this patch
changes.
I think you could simply squash this patch into the previous one since there will
already be a behaviour change there.
ATB,
Mark.
On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
> Per the DP83932C datasheet from July 1995:
>
> 4.0 SONIC Registers
> 4.1 THE CAM UNIT
>
> The Content Addressable Memory (CAM) consists of sixteen
> 48-bit entries for complete address filtering of network
> packets. Each entry corresponds to a 48-bit destination
> address that is user programmable and can contain any
> combination of Multicast or Physical addresses. Each entry
> is partitioned into three 16-bit CAM cells accessible
> through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with
> CAP0 corresponding to the least significant 16 bits of
> the Destination Address and CAP2 corresponding to the
> most significant bits.
>
> Store the CAM registers as 16-bit as it simplifies the code.
> There is no change in the migration stream.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/net/dp8393x.c | 23 ++++++++++-------------
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index c9b478c127c..e0055b178b1 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -157,7 +157,7 @@ struct dp8393xState {
> MemoryRegion mmio;
>
> /* Registers */
> - uint8_t cam[16][6];
> + uint16_t cam[16][3];
> uint16_t regs[0x40];
>
> /* Temporaries */
> @@ -280,15 +280,13 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> address_space_read(&s->as, dp8393x_cdp(s),
> MEMTXATTRS_UNSPECIFIED, s->data, size);
> index = dp8393x_get(s, width, 0) & 0xf;
> - s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
> - s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
> - s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
> - s->cam[index][3] = dp8393x_get(s, width, 2) >> 8;
> - s->cam[index][4] = dp8393x_get(s, width, 3) & 0xff;
> - s->cam[index][5] = dp8393x_get(s, width, 3) >> 8;
> - trace_dp8393x_load_cam(index, s->cam[index][0], s->cam[index][1],
> - s->cam[index][2], s->cam[index][3],
> - s->cam[index][4], s->cam[index][5]);
> + s->cam[index][0] = dp8393x_get(s, width, 1);
> + s->cam[index][1] = dp8393x_get(s, width, 2);
> + s->cam[index][2] = dp8393x_get(s, width, 3);
> + trace_dp8393x_load_cam(index,
> + s->cam[index][0] >> 8, s->cam[index][0] & 0xff,
> + s->cam[index][1] >> 8, s->cam[index][1] & 0xff,
> + s->cam[index][2] >> 8, s->cam[index][2] & 0xff);
> /* Move to next entry */
> s->regs[SONIC_CDC]--;
> s->regs[SONIC_CDP] += size;
> @@ -591,8 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
> case SONIC_CAP1:
> case SONIC_CAP0:
> if (s->regs[SONIC_CR] & SONIC_CR_RST) {
> - val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg) + 1] << 8;
> - val |= s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)];
> + val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)];
> }
> break;
> /* All other registers have no special contraints */
> @@ -987,7 +984,7 @@ static const VMStateDescription vmstate_dp8393x = {
> .version_id = 0,
> .minimum_version_id = 0,
> .fields = (VMStateField []) {
> - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6),
> + VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2),
> VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
> VMSTATE_END_OF_LIST()
> }
I need to do some testing first to make sure dp8393x_get() and dp8393x_put() are
doing the right thing here, but it looks correct.
Also given that the migration stream for MIPS machines has been broken for many years
until your latest PR, I would have no objection if you wanted to change
VMSTATE_BUFFER_UNSAFE to VMSTATE_UINT16_ARRAY.
ATB,
Mark.
On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/net/dp8393x.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index e0055b178b1..bbe241ef9db 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -814,8 +814,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> size = sizeof(uint16_t) * width;
> address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
> dp8393x_put(s, width, 0, 0);
> - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Move to next descriptor */
> s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> @@ -844,8 +844,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Pad short packets to keep pointers aligned */
> if (rx_len < padded_len) {
> size = padded_len - rx_len;
> - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)"\xFF\xFF\xFF", size, 1);
> + address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)"\xFF\xFF\xFF", size);
> address += size;
> }
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
> Instead of accessing N registers via a single address_space API
> call using a temporary buffer (stored in the device state) and
> updating each register, move the address_space call in the
> register put/get. The load/store and word size checks are moved
> to put/get too. This simplifies a bit, making the code easier
> to read.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/net/dp8393x.c | 157 ++++++++++++++++++-----------------------------
> 1 file changed, 60 insertions(+), 97 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index bbe241ef9db..db9cfd786f5 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -162,7 +162,6 @@ struct dp8393xState {
>
> /* Temporaries */
> uint8_t tx_buffer[0x10000];
> - uint16_t data[12];
> int loopback_packet;
>
> /* Memory access */
> @@ -219,34 +218,48 @@ static uint32_t dp8393x_wt(dp8393xState *s)
> return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
> }
>
> -static uint16_t dp8393x_get(dp8393xState *s, int width, int offset)
> +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned ofs16)
> {
> + const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> uint16_t val;
>
> - if (s->big_endian) {
> - val = be16_to_cpu(s->data[offset * width + width - 1]);
> + if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> + addr += 2 * ofs16;
> + if (s->big_endian) {
> + val = address_space_ldl_be(&s->as, addr, attrs, NULL);
> + } else {
> + val = address_space_ldl_le(&s->as, addr, attrs, NULL);
> + }
> } else {
> - val = le16_to_cpu(s->data[offset * width]);
> + addr += 1 * ofs16;
> + if (s->big_endian) {
> + val = address_space_lduw_be(&s->as, addr, attrs, NULL);
> + } else {
> + val = address_space_lduw_le(&s->as, addr, attrs, NULL);
> + }
> }
> +
> return val;
> }
>
> -static void dp8393x_put(dp8393xState *s, int width, int offset,
> - uint16_t val)
> +static void dp8393x_put(dp8393xState *s,
> + hwaddr addr, unsigned ofs16, uint16_t val)
> {
> - if (s->big_endian) {
> - if (width == 2) {
> - s->data[offset * 2] = 0;
> - s->data[offset * 2 + 1] = cpu_to_be16(val);
> + const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> +
> + if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> + addr += 2 * ofs16;
> + if (s->big_endian) {
> + address_space_stl_be(&s->as, addr, val, attrs, NULL);
> } else {
> - s->data[offset] = cpu_to_be16(val);
> + address_space_stl_le(&s->as, addr, val, attrs, NULL);
> }
> } else {
> - if (width == 2) {
> - s->data[offset * 2] = cpu_to_le16(val);
> - s->data[offset * 2 + 1] = 0;
> + addr += 1 * ofs16;
> + if (s->big_endian) {
> + address_space_stw_be(&s->as, addr, val, attrs, NULL);
> } else {
> - s->data[offset] = cpu_to_le16(val);
> + address_space_stw_le(&s->as, addr, val, attrs, NULL);
> }
> }
> }
> @@ -277,12 +290,10 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>
> while (s->regs[SONIC_CDC] & 0x1f) {
> /* Fill current entry */
> - address_space_read(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, s->data, size);
> - index = dp8393x_get(s, width, 0) & 0xf;
> - s->cam[index][0] = dp8393x_get(s, width, 1);
> - s->cam[index][1] = dp8393x_get(s, width, 2);
> - s->cam[index][2] = dp8393x_get(s, width, 3);
> + index = dp8393x_get(s, dp8393x_cdp(s), 0) & 0xf;
> + s->cam[index][0] = dp8393x_get(s, dp8393x_cdp(s), 1);
> + s->cam[index][1] = dp8393x_get(s, dp8393x_cdp(s), 2);
> + s->cam[index][2] = dp8393x_get(s, dp8393x_cdp(s), 3);
> trace_dp8393x_load_cam(index,
> s->cam[index][0] >> 8, s->cam[index][0] & 0xff,
> s->cam[index][1] >> 8, s->cam[index][1] & 0xff,
> @@ -293,9 +304,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> }
>
> /* Read CAM enable */
> - address_space_read(&s->as, dp8393x_cdp(s),
> - MEMTXATTRS_UNSPECIFIED, s->data, size);
> - s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
> + s->regs[SONIC_CE] = dp8393x_get(s, dp8393x_cdp(s), 0);
> trace_dp8393x_load_cam_done(s->regs[SONIC_CE]);
>
> /* Done */
> @@ -311,14 +320,12 @@ static void dp8393x_do_read_rra(dp8393xState *s)
> /* Read memory */
> width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
> size = sizeof(uint16_t) * 4 * width;
> - address_space_read(&s->as, dp8393x_rrp(s),
> - MEMTXATTRS_UNSPECIFIED, s->data, size);
>
> /* Update SONIC registers */
> - s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0);
> - s->regs[SONIC_CRBA1] = dp8393x_get(s, width, 1);
> - s->regs[SONIC_RBWC0] = dp8393x_get(s, width, 2);
> - s->regs[SONIC_RBWC1] = dp8393x_get(s, width, 3);
> + s->regs[SONIC_CRBA0] = dp8393x_get(s, dp8393x_rrp(s), 0);
> + s->regs[SONIC_CRBA1] = dp8393x_get(s, dp8393x_rrp(s), 1);
> + s->regs[SONIC_RBWC0] = dp8393x_get(s, dp8393x_rrp(s), 2);
> + s->regs[SONIC_RBWC1] = dp8393x_get(s, dp8393x_rrp(s), 3);
> trace_dp8393x_read_rra_regs(s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
> s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]);
>
> @@ -414,28 +421,22 @@ static void dp8393x_do_receiver_disable(dp8393xState *s)
> static void dp8393x_do_transmit_packets(dp8393xState *s)
> {
> NetClientState *nc = qemu_get_queue(s->nic);
> - int width, size;
> int tx_len, len;
> uint16_t i;
>
> - width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
> -
> while (1) {
> /* Read memory */
> - size = sizeof(uint16_t) * 6 * width;
> s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
> trace_dp8393x_transmit_packet(dp8393x_ttda(s));
> - address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width,
> - MEMTXATTRS_UNSPECIFIED, s->data, size);
> tx_len = 0;
>
> /* Update registers */
> - s->regs[SONIC_TCR] = dp8393x_get(s, width, 0) & 0xf000;
> - s->regs[SONIC_TPS] = dp8393x_get(s, width, 1);
> - s->regs[SONIC_TFC] = dp8393x_get(s, width, 2);
> - s->regs[SONIC_TSA0] = dp8393x_get(s, width, 3);
> - s->regs[SONIC_TSA1] = dp8393x_get(s, width, 4);
> - s->regs[SONIC_TFS] = dp8393x_get(s, width, 5);
> + s->regs[SONIC_TCR] = dp8393x_get(s, dp8393x_ttda(s), 0) & 0xf000;
> + s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 1);
> + s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 2);
> + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 3);
> + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 4);
> + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 5);
>
> /* Handle programmable interrupt */
> if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) {
> @@ -457,15 +458,9 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> i++;
> if (i != s->regs[SONIC_TFC]) {
> /* Read next fragment details */
> - size = sizeof(uint16_t) * 3 * width;
> - address_space_read(&s->as,
> - dp8393x_ttda(s)
> - + sizeof(uint16_t) * width * (4 + 3 * i),
> - MEMTXATTRS_UNSPECIFIED, s->data,
> - size);
> - s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0);
> - s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1);
> - s->regs[SONIC_TFS] = dp8393x_get(s, width, 2);
> + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 0);
> + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 1);
> + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 2);
> }
> }
>
> @@ -498,22 +493,12 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> s->regs[SONIC_TCR] |= SONIC_TCR_PTX;
>
> /* Write status */
> - dp8393x_put(s, width, 0,
> - s->regs[SONIC_TCR] & 0x0fff); /* status */
> - size = sizeof(uint16_t) * width;
> - address_space_write(&s->as, dp8393x_ttda(s),
> - MEMTXATTRS_UNSPECIFIED, s->data, size);
> + dp8393x_put(s, dp8393x_ttda(s), 0, s->regs[SONIC_TCR] & 0x0fff);
>
> if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
> /* Read footer of packet */
> - size = sizeof(uint16_t) * width;
> - address_space_read(&s->as,
> - dp8393x_ttda(s)
> - + sizeof(uint16_t) * width
> - * (4 + 3 * s->regs[SONIC_TFC]),
> - MEMTXATTRS_UNSPECIFIED, s->data,
> - size);
> - s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
> + s->regs[SONIC_CTDA] = dp8393x_get(s, dp8393x_ttda(s),
> + 4 + 3 * s->regs[SONIC_TFC]);
> if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
> /* EOL detected */
> break;
> @@ -762,7 +747,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> dp8393xState *s = qemu_get_nic_opaque(nc);
> int packet_type;
> uint32_t available, address;
> - int width, rx_len, padded_len;
> + int rx_len, padded_len;
> uint32_t checksum;
> int size;
>
> @@ -775,10 +760,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>
> rx_len = pkt_size + sizeof(checksum);
> if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> - width = 2;
> padded_len = ((rx_len - 1) | 3) + 1;
> } else {
> - width = 1;
> padded_len = ((rx_len - 1) | 1) + 1;
> }
>
> @@ -799,11 +782,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Check for EOL */
> if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> /* Are we still in resource exhaustion? */
> - size = sizeof(uint16_t) * 1 * width;
> - address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
> - address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - s->data, size);
> - s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> + s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5);
> if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> /* Still EOL ; stop reception */
> return -1;
> @@ -811,11 +790,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Link has been updated by host */
>
> /* Clear in_use */
> - size = sizeof(uint16_t) * width;
> - address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
> - dp8393x_put(s, width, 0, 0);
> - address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, size);
> + dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);
>
> /* Move to next descriptor */
> s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> @@ -869,32 +844,20 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>
> /* Write status to memory */
> trace_dp8393x_receive_write_status(dp8393x_crda(s));
> - dp8393x_put(s, width, 0, s->regs[SONIC_RCR]); /* status */
> - dp8393x_put(s, width, 1, rx_len); /* byte count */
> - dp8393x_put(s, width, 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */
> - dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
> - dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */
> - size = sizeof(uint16_t) * 5 * width;
> - address_space_write(&s->as, dp8393x_crda(s),
> - MEMTXATTRS_UNSPECIFIED,
> - s->data, size);
> + dp8393x_put(s, dp8393x_crda(s), 0, s->regs[SONIC_RCR]); /* status */
> + dp8393x_put(s, dp8393x_crda(s), 1, rx_len); /* byte count */
> + dp8393x_put(s, dp8393x_crda(s), 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */
> + dp8393x_put(s, dp8393x_crda(s), 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */
> + dp8393x_put(s, dp8393x_crda(s), 4, s->regs[SONIC_RSC]); /* seq_no */
>
> /* Check link field */
> - size = sizeof(uint16_t) * width;
> - address_space_read(&s->as,
> - dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> - MEMTXATTRS_UNSPECIFIED, s->data, size);
> - s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> + s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5);
> if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> /* EOL detected */
> s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
> } else {
> /* Clear in_use */
> - size = sizeof(uint16_t) * width;
> - address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
> - dp8393x_put(s, width, 0, 0);
> - address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - s->data, size);
> + dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);
>
> /* Move to next descriptor */
> s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
This seems like a nice tidy-up. I'll need a bit of time to give some more R-b and T-b
tags since MacOS is running on a very diverged branch, so I'll need to pick through
and update the changes to run across all my test images accordingly.
With this patch is it now possible to remove s->data completely?
ATB,
Mark.
On 7/3/21 5:00 PM, Mark Cave-Ayland wrote: > On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote: > >> Instead of accessing N registers via a single address_space API >> call using a temporary buffer (stored in the device state) and >> updating each register, move the address_space call in the >> register put/get. The load/store and word size checks are moved >> to put/get too. This simplifies a bit, making the code easier >> to read. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/net/dp8393x.c | 157 ++++++++++++++++++----------------------------- >> 1 file changed, 60 insertions(+), 97 deletions(-) >> >> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c >> index bbe241ef9db..db9cfd786f5 100644 >> --- a/hw/net/dp8393x.c >> +++ b/hw/net/dp8393x.c >> @@ -162,7 +162,6 @@ struct dp8393xState { >> /* Temporaries */ >> uint8_t tx_buffer[0x10000]; >> - uint16_t data[12]; >> int loopback_packet; >> /* Memory access */ > This seems like a nice tidy-up. I'll need a bit of time to give some > more R-b and T-b tags since MacOS is running on a very diverged branch, > so I'll need to pick through and update the changes to run across all my > test images accordingly. No hurry. > With this patch is it now possible to remove s->data completely? First line remove it ;) (It was not part of the migration stream).
On 7/3/21 4:39 PM, Mark Cave-Ayland wrote: > On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote: > >> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> >> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that >> all accesses >> to the registers were 32-bit but this is actually not the case. The >> access size is >> determined by the CPU instruction used and not the number of physical >> address lines. >> >> The big_endian workaround applied to the register read/writes was >> actually caused >> by forcing the access size to 32-bit when the guest OS was using a >> 16-bit access. >> Since the registers are 16-bit then we can simply set .impl.min_access >> to 2 and >> then the memory API will automatically do the right thing for both >> 16-bit accesses >> used by Linux and 32-bit accesses used by the MacOS toolbox ROM. > > The change should work, but the commit message above needs a slight > tweak - maybe something like this? > > Since the registers are 16-bit then we can simply set both > .impl.min_access and .impl.max_access to 2 and then the memory API will > automatically do the right thing for both 16-bit accesses used by Linux > and 32-bit accesses used by the MacOS toolbox ROM. Do you mind sending v3 of this patch reworded (and including the .valid fields)? >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses") >> Tested-by: Finn Thain <fthain@linux-m68k.org> >> Message-Id: <20210625065401.30170-9-mark.cave-ayland@ilande.co.uk> >> [PMD: dp8393x_ops.impl.max_access_size 4 -> 2] >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/net/dp8393x.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-)
[-- Attachment #1: Type: text/plain, Size: 802 bytes --] On Sat, 3 Jul 2021, Philippe Mathieu-Daudé wrote: > Instead of accessing N registers via a single address_space API > call using a temporary buffer (stored in the device state) and > updating each register, move the address_space call in the > register put/get. The load/store and word size checks are moved > to put/get too. This simplifies a bit, making the code easier > to read. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> I tried this series with a Linux/m68k guest but network activity hanged the emulator. The cause of the problem is somewhere in this patch. BTW, I've become suspicious of the word "rewrite". In this case I think it describes a commit that is attempting to do too much and needs to be split up to make it easier to review (and debug).
[-- Attachment #1: Type: text/plain, Size: 1326 bytes --] On Sat, 3 Jul 2021, Philippe Mathieu-Daudé wrote: > From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that all accesses > to the registers were 32-bit but this is actually not the case. The access size is > determined by the CPU instruction used and not the number of physical address lines. > > The big_endian workaround applied to the register read/writes was actually caused > by forcing the access size to 32-bit when the guest OS was using a 16-bit access. > Since the registers are 16-bit then we can simply set .impl.min_access to 2 and > then the memory API will automatically do the right thing for both 16-bit accesses > used by Linux and 32-bit accesses used by the MacOS toolbox ROM. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses") > Tested-by: Finn Thain <fthain@linux-m68k.org> I have to retract that tested-by tag for this new version. It breaks my Linux/mipsel guest. The jazzsonic driver now says, SONIC ethernet controller not found (0x40004) > Message-Id: <20210625065401.30170-9-mark.cave-ayland@ilande.co.uk> > [PMD: dp8393x_ops.impl.max_access_size 4 -> 2] > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
> Per the DP83932C datasheet from July 1995:
>
> 1. Functional Description
> 1.3 DATA WIDTH AND BYTE ORDERING
>
> The SONIC can be programmed to operate with
> either 32-bit or 16-bit wide memory.
>
> Restrict the memory bus to reject 8/64-bit accesses.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/net/dp8393x.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index d16ade2b198..c9b478c127c 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -695,6 +695,8 @@ static const MemoryRegionOps dp8393x_ops = {
> .write = dp8393x_write,
> .impl.min_access_size = 2,
> .impl.max_access_size = 2,
> + .valid.min_access_size = 2,
> + .valid.max_access_size = 4,
> .endianness = DEVICE_NATIVE_ENDIAN,
> };
Unfortunately this patch breaks MacOS - it seems very early in startup the MacOS
toolbox probes for the presence of the network adapter using single byte accesses
which are rejected by this change :(
I'd suggest that we simply drop this patch.
ATB,
Mark.
On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
> Per the DP83932C datasheet from July 1995:
>
> 4.0 SONIC Registers
> 4.1 THE CAM UNIT
>
> The Content Addressable Memory (CAM) consists of sixteen
> 48-bit entries for complete address filtering of network
> packets. Each entry corresponds to a 48-bit destination
> address that is user programmable and can contain any
> combination of Multicast or Physical addresses. Each entry
> is partitioned into three 16-bit CAM cells accessible
> through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with
> CAP0 corresponding to the least significant 16 bits of
> the Destination Address and CAP2 corresponding to the
> most significant bits.
>
> Store the CAM registers as 16-bit as it simplifies the code.
> There is no change in the migration stream.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/net/dp8393x.c | 23 ++++++++++-------------
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index c9b478c127c..e0055b178b1 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -157,7 +157,7 @@ struct dp8393xState {
> MemoryRegion mmio;
>
> /* Registers */
> - uint8_t cam[16][6];
> + uint16_t cam[16][3];
> uint16_t regs[0x40];
>
> /* Temporaries */
> @@ -280,15 +280,13 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> address_space_read(&s->as, dp8393x_cdp(s),
> MEMTXATTRS_UNSPECIFIED, s->data, size);
> index = dp8393x_get(s, width, 0) & 0xf;
> - s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
> - s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
> - s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
> - s->cam[index][3] = dp8393x_get(s, width, 2) >> 8;
> - s->cam[index][4] = dp8393x_get(s, width, 3) & 0xff;
> - s->cam[index][5] = dp8393x_get(s, width, 3) >> 8;
> - trace_dp8393x_load_cam(index, s->cam[index][0], s->cam[index][1],
> - s->cam[index][2], s->cam[index][3],
> - s->cam[index][4], s->cam[index][5]);
> + s->cam[index][0] = dp8393x_get(s, width, 1);
> + s->cam[index][1] = dp8393x_get(s, width, 2);
> + s->cam[index][2] = dp8393x_get(s, width, 3);
> + trace_dp8393x_load_cam(index,
> + s->cam[index][0] >> 8, s->cam[index][0] & 0xff,
> + s->cam[index][1] >> 8, s->cam[index][1] & 0xff,
> + s->cam[index][2] >> 8, s->cam[index][2] & 0xff);
> /* Move to next entry */
> s->regs[SONIC_CDC]--;
> s->regs[SONIC_CDP] += size;
> @@ -591,8 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
> case SONIC_CAP1:
> case SONIC_CAP0:
> if (s->regs[SONIC_CR] & SONIC_CR_RST) {
> - val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg) + 1] << 8;
> - val |= s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)];
> + val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)];
> }
> break;
> /* All other registers have no special contraints */
> @@ -987,7 +984,7 @@ static const VMStateDescription vmstate_dp8393x = {
> .version_id = 0,
> .minimum_version_id = 0,
> .fields = (VMStateField []) {
> - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6),
> + VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2),
> VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
> VMSTATE_END_OF_LIST()
> }
I'd still be inclined to change VMSTATE_BUFFER_UNSAFE for VMSTATE_UINT16_ARRAY whilst
you can do it without having to worry about the migration stream being already
broken, but anyhow:
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/net/dp8393x.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index e0055b178b1..bbe241ef9db 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -814,8 +814,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> size = sizeof(uint16_t) * width;
> address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
> dp8393x_put(s, width, 0, 0);
> - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)s->data, size, 1);
> + address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->data, size);
>
> /* Move to next descriptor */
> s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
> @@ -844,8 +844,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
> /* Pad short packets to keep pointers aligned */
> if (rx_len < padded_len) {
> size = padded_len - rx_len;
> - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> - (uint8_t *)"\xFF\xFF\xFF", size, 1);
> + address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)"\xFF\xFF\xFF", size);
> address += size;
> }
I've also verified that this works so it can also have:
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote: > Instead of accessing N registers via a single address_space API > call using a temporary buffer (stored in the device state) and > updating each register, move the address_space call in the > register put/get. The load/store and word size checks are moved > to put/get too. This simplifies a bit, making the code easier > to read. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/net/dp8393x.c | 157 ++++++++++++++++++----------------------------- > 1 file changed, 60 insertions(+), 97 deletions(-) > > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c > index bbe241ef9db..db9cfd786f5 100644 > --- a/hw/net/dp8393x.c > +++ b/hw/net/dp8393x.c > @@ -162,7 +162,6 @@ struct dp8393xState { > > /* Temporaries */ > uint8_t tx_buffer[0x10000]; > - uint16_t data[12]; > int loopback_packet; > > /* Memory access */ > @@ -219,34 +218,48 @@ static uint32_t dp8393x_wt(dp8393xState *s) > return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0]; > } > > -static uint16_t dp8393x_get(dp8393xState *s, int width, int offset) > +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned ofs16) > { > + const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; > uint16_t val; > > - if (s->big_endian) { > - val = be16_to_cpu(s->data[offset * width + width - 1]); > + if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { > + addr += 2 * ofs16; > + if (s->big_endian) { > + val = address_space_ldl_be(&s->as, addr, attrs, NULL); > + } else { > + val = address_space_ldl_le(&s->as, addr, attrs, NULL); > + } > } else { > - val = le16_to_cpu(s->data[offset * width]); > + addr += 1 * ofs16; > + if (s->big_endian) { > + val = address_space_lduw_be(&s->as, addr, attrs, NULL); > + } else { > + val = address_space_lduw_le(&s->as, addr, attrs, NULL); > + } > } > + > return val; > } > > -static void dp8393x_put(dp8393xState *s, int width, int offset, > - uint16_t val) > +static void dp8393x_put(dp8393xState *s, > + hwaddr addr, unsigned ofs16, uint16_t val) > { > - if (s->big_endian) { > - if (width == 2) { > - s->data[offset * 2] = 0; > - s->data[offset * 2 + 1] = cpu_to_be16(val); > + const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; > + > + if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { > + addr += 2 * ofs16; > + if (s->big_endian) { > + address_space_stl_be(&s->as, addr, val, attrs, NULL); > } else { > - s->data[offset] = cpu_to_be16(val); > + address_space_stl_le(&s->as, addr, val, attrs, NULL); > } > } else { > - if (width == 2) { > - s->data[offset * 2] = cpu_to_le16(val); > - s->data[offset * 2 + 1] = 0; > + addr += 1 * ofs16; > + if (s->big_endian) { > + address_space_stw_be(&s->as, addr, val, attrs, NULL); > } else { > - s->data[offset] = cpu_to_le16(val); > + address_space_stw_le(&s->as, addr, val, attrs, NULL); > } > } > } > @@ -277,12 +290,10 @@ static void dp8393x_do_load_cam(dp8393xState *s) > > while (s->regs[SONIC_CDC] & 0x1f) { > /* Fill current entry */ > - address_space_read(&s->as, dp8393x_cdp(s), > - MEMTXATTRS_UNSPECIFIED, s->data, size); > - index = dp8393x_get(s, width, 0) & 0xf; > - s->cam[index][0] = dp8393x_get(s, width, 1); > - s->cam[index][1] = dp8393x_get(s, width, 2); > - s->cam[index][2] = dp8393x_get(s, width, 3); > + index = dp8393x_get(s, dp8393x_cdp(s), 0) & 0xf; > + s->cam[index][0] = dp8393x_get(s, dp8393x_cdp(s), 1); > + s->cam[index][1] = dp8393x_get(s, dp8393x_cdp(s), 2); > + s->cam[index][2] = dp8393x_get(s, dp8393x_cdp(s), 3); > trace_dp8393x_load_cam(index, > s->cam[index][0] >> 8, s->cam[index][0] & 0xff, > s->cam[index][1] >> 8, s->cam[index][1] & 0xff, > @@ -293,9 +304,7 @@ static void dp8393x_do_load_cam(dp8393xState *s) > } > > /* Read CAM enable */ > - address_space_read(&s->as, dp8393x_cdp(s), > - MEMTXATTRS_UNSPECIFIED, s->data, size); > - s->regs[SONIC_CE] = dp8393x_get(s, width, 0); > + s->regs[SONIC_CE] = dp8393x_get(s, dp8393x_cdp(s), 0); > trace_dp8393x_load_cam_done(s->regs[SONIC_CE]); > > /* Done */ > @@ -311,14 +320,12 @@ static void dp8393x_do_read_rra(dp8393xState *s) > /* Read memory */ > width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1; > size = sizeof(uint16_t) * 4 * width; > - address_space_read(&s->as, dp8393x_rrp(s), > - MEMTXATTRS_UNSPECIFIED, s->data, size); > > /* Update SONIC registers */ > - s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0); > - s->regs[SONIC_CRBA1] = dp8393x_get(s, width, 1); > - s->regs[SONIC_RBWC0] = dp8393x_get(s, width, 2); > - s->regs[SONIC_RBWC1] = dp8393x_get(s, width, 3); > + s->regs[SONIC_CRBA0] = dp8393x_get(s, dp8393x_rrp(s), 0); > + s->regs[SONIC_CRBA1] = dp8393x_get(s, dp8393x_rrp(s), 1); > + s->regs[SONIC_RBWC0] = dp8393x_get(s, dp8393x_rrp(s), 2); > + s->regs[SONIC_RBWC1] = dp8393x_get(s, dp8393x_rrp(s), 3); > trace_dp8393x_read_rra_regs(s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1], > s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]); > > @@ -414,28 +421,22 @@ static void dp8393x_do_receiver_disable(dp8393xState *s) > static void dp8393x_do_transmit_packets(dp8393xState *s) > { > NetClientState *nc = qemu_get_queue(s->nic); > - int width, size; > int tx_len, len; > uint16_t i; > > - width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1; > - > while (1) { > /* Read memory */ > - size = sizeof(uint16_t) * 6 * width; > s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA]; > trace_dp8393x_transmit_packet(dp8393x_ttda(s)); > - address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width, > - MEMTXATTRS_UNSPECIFIED, s->data, size); > tx_len = 0; > > /* Update registers */ > - s->regs[SONIC_TCR] = dp8393x_get(s, width, 0) & 0xf000; > - s->regs[SONIC_TPS] = dp8393x_get(s, width, 1); > - s->regs[SONIC_TFC] = dp8393x_get(s, width, 2); > - s->regs[SONIC_TSA0] = dp8393x_get(s, width, 3); > - s->regs[SONIC_TSA1] = dp8393x_get(s, width, 4); > - s->regs[SONIC_TFS] = dp8393x_get(s, width, 5); > + s->regs[SONIC_TCR] = dp8393x_get(s, dp8393x_ttda(s), 0) & 0xf000; > + s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 1); > + s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 2); > + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 3); > + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 4); > + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 5); > > /* Handle programmable interrupt */ > if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) { > @@ -457,15 +458,9 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) > i++; > if (i != s->regs[SONIC_TFC]) { > /* Read next fragment details */ > - size = sizeof(uint16_t) * 3 * width; > - address_space_read(&s->as, > - dp8393x_ttda(s) > - + sizeof(uint16_t) * width * (4 + 3 * i), > - MEMTXATTRS_UNSPECIFIED, s->data, > - size); > - s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0); > - s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1); > - s->regs[SONIC_TFS] = dp8393x_get(s, width, 2); > + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 0); > + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 1); > + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 2); > } > } > > @@ -498,22 +493,12 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) > s->regs[SONIC_TCR] |= SONIC_TCR_PTX; > > /* Write status */ > - dp8393x_put(s, width, 0, > - s->regs[SONIC_TCR] & 0x0fff); /* status */ > - size = sizeof(uint16_t) * width; > - address_space_write(&s->as, dp8393x_ttda(s), > - MEMTXATTRS_UNSPECIFIED, s->data, size); > + dp8393x_put(s, dp8393x_ttda(s), 0, s->regs[SONIC_TCR] & 0x0fff); > > if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) { > /* Read footer of packet */ > - size = sizeof(uint16_t) * width; > - address_space_read(&s->as, > - dp8393x_ttda(s) > - + sizeof(uint16_t) * width > - * (4 + 3 * s->regs[SONIC_TFC]), > - MEMTXATTRS_UNSPECIFIED, s->data, > - size); > - s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0); > + s->regs[SONIC_CTDA] = dp8393x_get(s, dp8393x_ttda(s), > + 4 + 3 * s->regs[SONIC_TFC]); > if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) { > /* EOL detected */ > break; > @@ -762,7 +747,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, > dp8393xState *s = qemu_get_nic_opaque(nc); > int packet_type; > uint32_t available, address; > - int width, rx_len, padded_len; > + int rx_len, padded_len; > uint32_t checksum; > int size; > > @@ -775,10 +760,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, > > rx_len = pkt_size + sizeof(checksum); > if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { > - width = 2; > padded_len = ((rx_len - 1) | 3) + 1; > } else { > - width = 1; > padded_len = ((rx_len - 1) | 1) + 1; > } > > @@ -799,11 +782,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, > /* Check for EOL */ > if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { > /* Are we still in resource exhaustion? */ > - size = sizeof(uint16_t) * 1 * width; > - address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width; > - address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED, > - s->data, size); > - s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0); > + s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5); > if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { > /* Still EOL ; stop reception */ > return -1; > @@ -811,11 +790,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, > /* Link has been updated by host */ > > /* Clear in_use */ > - size = sizeof(uint16_t) * width; > - address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; > - dp8393x_put(s, width, 0, 0); > - address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, > - (uint8_t *)s->data, size); > + dp8393x_put(s, dp8393x_crda(s), 6, 0x0000); > > /* Move to next descriptor */ > s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; > @@ -869,32 +844,20 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, > > /* Write status to memory */ > trace_dp8393x_receive_write_status(dp8393x_crda(s)); > - dp8393x_put(s, width, 0, s->regs[SONIC_RCR]); /* status */ > - dp8393x_put(s, width, 1, rx_len); /* byte count */ > - dp8393x_put(s, width, 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */ > - dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */ > - dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */ > - size = sizeof(uint16_t) * 5 * width; > - address_space_write(&s->as, dp8393x_crda(s), > - MEMTXATTRS_UNSPECIFIED, > - s->data, size); > + dp8393x_put(s, dp8393x_crda(s), 0, s->regs[SONIC_RCR]); /* status */ > + dp8393x_put(s, dp8393x_crda(s), 1, rx_len); /* byte count */ > + dp8393x_put(s, dp8393x_crda(s), 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */ > + dp8393x_put(s, dp8393x_crda(s), 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */ > + dp8393x_put(s, dp8393x_crda(s), 4, s->regs[SONIC_RSC]); /* seq_no */ > > /* Check link field */ > - size = sizeof(uint16_t) * width; > - address_space_read(&s->as, > - dp8393x_crda(s) + sizeof(uint16_t) * 5 * width, > - MEMTXATTRS_UNSPECIFIED, s->data, size); > - s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0); > + s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5); > if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { > /* EOL detected */ > s->regs[SONIC_ISR] |= SONIC_ISR_RDE; > } else { > /* Clear in_use */ > - size = sizeof(uint16_t) * width; > - address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; > - dp8393x_put(s, width, 0, 0); > - address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, > - s->data, size); > + dp8393x_put(s, dp8393x_crda(s), 6, 0x0000); > > /* Move to next descriptor */ > s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; This patch breaks networking in its current form, but I was able to make it work by applying the following diff: diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index db9cfd786f..b08843172b 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -218,20 +218,20 @@ static uint32_t dp8393x_wt(dp8393xState *s) return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0]; } -static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned ofs16) +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, int offset) { const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; uint16_t val; if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { - addr += 2 * ofs16; + addr += offset << 2; if (s->big_endian) { val = address_space_ldl_be(&s->as, addr, attrs, NULL); } else { val = address_space_ldl_le(&s->as, addr, attrs, NULL); } } else { - addr += 1 * ofs16; + addr += offset << 1; if (s->big_endian) { val = address_space_lduw_be(&s->as, addr, attrs, NULL); } else { @@ -243,19 +243,19 @@ static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned ofs16) } static void dp8393x_put(dp8393xState *s, - hwaddr addr, unsigned ofs16, uint16_t val) + hwaddr addr, int offset, uint16_t val) { const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { - addr += 2 * ofs16; + addr += offset << 2; if (s->big_endian) { address_space_stl_be(&s->as, addr, val, attrs, NULL); } else { address_space_stl_le(&s->as, addr, val, attrs, NULL); } } else { - addr += 1 * ofs16; + addr += offset << 1; if (s->big_endian) { address_space_stw_be(&s->as, addr, val, attrs, NULL); } else { @@ -431,12 +431,12 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) tx_len = 0; /* Update registers */ - s->regs[SONIC_TCR] = dp8393x_get(s, dp8393x_ttda(s), 0) & 0xf000; - s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 1); - s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 2); - s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 3); - s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 4); - s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 5); + s->regs[SONIC_TCR] = dp8393x_get(s, dp8393x_ttda(s), 1) & 0xf000; + s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 2); + s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 3); + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 4); + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 5); + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 6); /* Handle programmable interrupt */ if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) { @@ -458,9 +458,9 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) i++; if (i != s->regs[SONIC_TFC]) { /* Read next fragment details */ - s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 0); - s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 1); - s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 2); + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 4 + 3 * i); + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 5 + 3 * i); + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 6 + 3 * i); } } The main change is that the offset argument for dp8393x_get() and dp8393x_put() is actually an entry number and not a count of 16 bit words. Other than that there were a couple of offset calculations that needed adjustment to get things working again. Taking git master and applying your outstanding PR + this series without patch 3 + this diff gave me working networking on Linux/m68k, MacOS 8.0 and NetBSD/arc. Unfortunately I don't have a test mips64el image available to see if this combination works for Linux. Phil, do you have a suitable test kernel and rootfs image available to allow this to be tested? ATB, Mark.
On 03/07/2021 17:29, Philippe Mathieu-Daudé wrote:
> On 7/3/21 4:39 PM, Mark Cave-Ayland wrote:
>> On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
>>
>>> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that
>>> all accesses
>>> to the registers were 32-bit but this is actually not the case. The
>>> access size is
>>> determined by the CPU instruction used and not the number of physical
>>> address lines.
>>>
>>> The big_endian workaround applied to the register read/writes was
>>> actually caused
>>> by forcing the access size to 32-bit when the guest OS was using a
>>> 16-bit access.
>>> Since the registers are 16-bit then we can simply set .impl.min_access
>>> to 2 and
>>> then the memory API will automatically do the right thing for both
>>> 16-bit accesses
>>> used by Linux and 32-bit accesses used by the MacOS toolbox ROM.
>>
>> The change should work, but the commit message above needs a slight
>> tweak - maybe something like this?
>>
>> Since the registers are 16-bit then we can simply set both
>> .impl.min_access and .impl.max_access to 2 and then the memory API will
>> automatically do the right thing for both 16-bit accesses used by Linux
>> and 32-bit accesses used by the MacOS toolbox ROM.
>
> Do you mind sending v3 of this patch reworded (and including the .valid
> fields)?
I've sent a v3 with the rewording but dropping the .valid fields since that breaks
MacOS and removed Finn's T-b tag as it may be there is an issue here with mips64el -
whilst it works for me on all of my test images, I'm struggling to keep up with all
the patches flying around everywhere :/
Now that your MIPS PR has been applied, perhaps it is worth sending a rebased v2 of
your RFC "dp8393x: Housekeeping" patchset to ensure that everyone is up to date with
the latest fixes? I won't be able to have a look at the CRC patchset for a few days
though.
ATB,
Mark.
[-- Attachment #1: Type: text/plain, Size: 19831 bytes --] On Sun, 4 Jul 2021, Mark Cave-Ayland wrote: > On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote: > > > Instead of accessing N registers via a single address_space API > > call using a temporary buffer (stored in the device state) and > > updating each register, move the address_space call in the > > register put/get. The load/store and word size checks are moved > > to put/get too. This simplifies a bit, making the code easier > > to read. > > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > hw/net/dp8393x.c | 157 ++++++++++++++++++----------------------------- > > 1 file changed, 60 insertions(+), 97 deletions(-) > > > > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c > > index bbe241ef9db..db9cfd786f5 100644 > > --- a/hw/net/dp8393x.c > > +++ b/hw/net/dp8393x.c > > @@ -162,7 +162,6 @@ struct dp8393xState { > > /* Temporaries */ > > uint8_t tx_buffer[0x10000]; > > - uint16_t data[12]; > > int loopback_packet; > > /* Memory access */ > > @@ -219,34 +218,48 @@ static uint32_t dp8393x_wt(dp8393xState *s) > > return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0]; > > } > > -static uint16_t dp8393x_get(dp8393xState *s, int width, int offset) > > +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned ofs16) > > { > > + const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; > > uint16_t val; > > - if (s->big_endian) { > > - val = be16_to_cpu(s->data[offset * width + width - 1]); > > + if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { > > + addr += 2 * ofs16; > > + if (s->big_endian) { > > + val = address_space_ldl_be(&s->as, addr, attrs, NULL); > > + } else { > > + val = address_space_ldl_le(&s->as, addr, attrs, NULL); > > + } > > } else { > > - val = le16_to_cpu(s->data[offset * width]); > > + addr += 1 * ofs16; > > + if (s->big_endian) { > > + val = address_space_lduw_be(&s->as, addr, attrs, NULL); > > + } else { > > + val = address_space_lduw_le(&s->as, addr, attrs, NULL); > > + } > > } > > + > > return val; > > } > > -static void dp8393x_put(dp8393xState *s, int width, int offset, > > - uint16_t val) > > +static void dp8393x_put(dp8393xState *s, > > + hwaddr addr, unsigned ofs16, uint16_t val) > > { > > - if (s->big_endian) { > > - if (width == 2) { > > - s->data[offset * 2] = 0; > > - s->data[offset * 2 + 1] = cpu_to_be16(val); > > + const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; > > + > > + if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { > > + addr += 2 * ofs16; > > + if (s->big_endian) { > > + address_space_stl_be(&s->as, addr, val, attrs, NULL); > > } else { > > - s->data[offset] = cpu_to_be16(val); > > + address_space_stl_le(&s->as, addr, val, attrs, NULL); > > } > > } else { > > - if (width == 2) { > > - s->data[offset * 2] = cpu_to_le16(val); > > - s->data[offset * 2 + 1] = 0; > > + addr += 1 * ofs16; > > + if (s->big_endian) { > > + address_space_stw_be(&s->as, addr, val, attrs, NULL); > > } else { > > - s->data[offset] = cpu_to_le16(val); > > + address_space_stw_le(&s->as, addr, val, attrs, NULL); > > } > > } > > } > > @@ -277,12 +290,10 @@ static void dp8393x_do_load_cam(dp8393xState *s) > > while (s->regs[SONIC_CDC] & 0x1f) { > > /* Fill current entry */ > > - address_space_read(&s->as, dp8393x_cdp(s), > > - MEMTXATTRS_UNSPECIFIED, s->data, size); > > - index = dp8393x_get(s, width, 0) & 0xf; > > - s->cam[index][0] = dp8393x_get(s, width, 1); > > - s->cam[index][1] = dp8393x_get(s, width, 2); > > - s->cam[index][2] = dp8393x_get(s, width, 3); > > + index = dp8393x_get(s, dp8393x_cdp(s), 0) & 0xf; > > + s->cam[index][0] = dp8393x_get(s, dp8393x_cdp(s), 1); > > + s->cam[index][1] = dp8393x_get(s, dp8393x_cdp(s), 2); > > + s->cam[index][2] = dp8393x_get(s, dp8393x_cdp(s), 3); > > trace_dp8393x_load_cam(index, > > s->cam[index][0] >> 8, s->cam[index][0] & > > 0xff, > > s->cam[index][1] >> 8, s->cam[index][1] & > > 0xff, > > @@ -293,9 +304,7 @@ static void dp8393x_do_load_cam(dp8393xState *s) > > } > > /* Read CAM enable */ > > - address_space_read(&s->as, dp8393x_cdp(s), > > - MEMTXATTRS_UNSPECIFIED, s->data, size); > > - s->regs[SONIC_CE] = dp8393x_get(s, width, 0); > > + s->regs[SONIC_CE] = dp8393x_get(s, dp8393x_cdp(s), 0); > > trace_dp8393x_load_cam_done(s->regs[SONIC_CE]); > > /* Done */ > > @@ -311,14 +320,12 @@ static void dp8393x_do_read_rra(dp8393xState *s) > > /* Read memory */ > > width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1; > > size = sizeof(uint16_t) * 4 * width; > > - address_space_read(&s->as, dp8393x_rrp(s), > > - MEMTXATTRS_UNSPECIFIED, s->data, size); > > /* Update SONIC registers */ > > - s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0); > > - s->regs[SONIC_CRBA1] = dp8393x_get(s, width, 1); > > - s->regs[SONIC_RBWC0] = dp8393x_get(s, width, 2); > > - s->regs[SONIC_RBWC1] = dp8393x_get(s, width, 3); > > + s->regs[SONIC_CRBA0] = dp8393x_get(s, dp8393x_rrp(s), 0); > > + s->regs[SONIC_CRBA1] = dp8393x_get(s, dp8393x_rrp(s), 1); > > + s->regs[SONIC_RBWC0] = dp8393x_get(s, dp8393x_rrp(s), 2); > > + s->regs[SONIC_RBWC1] = dp8393x_get(s, dp8393x_rrp(s), 3); > > trace_dp8393x_read_rra_regs(s->regs[SONIC_CRBA0], > > s->regs[SONIC_CRBA1], > > s->regs[SONIC_RBWC0], > > s->regs[SONIC_RBWC1]); > > @@ -414,28 +421,22 @@ static void dp8393x_do_receiver_disable(dp8393xState > > *s) > > static void dp8393x_do_transmit_packets(dp8393xState *s) > > { > > NetClientState *nc = qemu_get_queue(s->nic); > > - int width, size; > > int tx_len, len; > > uint16_t i; > > - width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1; > > - > > while (1) { > > /* Read memory */ > > - size = sizeof(uint16_t) * 6 * width; > > s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA]; > > trace_dp8393x_transmit_packet(dp8393x_ttda(s)); > > - address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * > > width, > > - MEMTXATTRS_UNSPECIFIED, s->data, size); > > tx_len = 0; > > /* Update registers */ > > - s->regs[SONIC_TCR] = dp8393x_get(s, width, 0) & 0xf000; > > - s->regs[SONIC_TPS] = dp8393x_get(s, width, 1); > > - s->regs[SONIC_TFC] = dp8393x_get(s, width, 2); > > - s->regs[SONIC_TSA0] = dp8393x_get(s, width, 3); > > - s->regs[SONIC_TSA1] = dp8393x_get(s, width, 4); > > - s->regs[SONIC_TFS] = dp8393x_get(s, width, 5); > > + s->regs[SONIC_TCR] = dp8393x_get(s, dp8393x_ttda(s), 0) & 0xf000; > > + s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 1); > > + s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 2); > > + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 3); > > + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 4); > > + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 5); > > /* Handle programmable interrupt */ > > if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) { > > @@ -457,15 +458,9 @@ static void dp8393x_do_transmit_packets(dp8393xState > > *s) > > i++; > > if (i != s->regs[SONIC_TFC]) { > > /* Read next fragment details */ > > - size = sizeof(uint16_t) * 3 * width; > > - address_space_read(&s->as, > > - dp8393x_ttda(s) > > - + sizeof(uint16_t) * width * (4 + 3 * > > i), > > - MEMTXATTRS_UNSPECIFIED, s->data, > > - size); > > - s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0); > > - s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1); > > - s->regs[SONIC_TFS] = dp8393x_get(s, width, 2); > > + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 0); > > + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 1); > > + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 2); > > } > > } > > @@ -498,22 +493,12 @@ static void dp8393x_do_transmit_packets(dp8393xState > > *s) > > s->regs[SONIC_TCR] |= SONIC_TCR_PTX; > > /* Write status */ > > - dp8393x_put(s, width, 0, > > - s->regs[SONIC_TCR] & 0x0fff); /* status */ > > - size = sizeof(uint16_t) * width; > > - address_space_write(&s->as, dp8393x_ttda(s), > > - MEMTXATTRS_UNSPECIFIED, s->data, size); > > + dp8393x_put(s, dp8393x_ttda(s), 0, s->regs[SONIC_TCR] & 0x0fff); > > if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) { > > /* Read footer of packet */ > > - size = sizeof(uint16_t) * width; > > - address_space_read(&s->as, > > - dp8393x_ttda(s) > > - + sizeof(uint16_t) * width > > - * (4 + 3 * s->regs[SONIC_TFC]), > > - MEMTXATTRS_UNSPECIFIED, s->data, > > - size); > > - s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0); > > + s->regs[SONIC_CTDA] = dp8393x_get(s, dp8393x_ttda(s), > > + 4 + 3 * s->regs[SONIC_TFC]); > > if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) { > > /* EOL detected */ > > break; > > @@ -762,7 +747,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const > > uint8_t * buf, > > dp8393xState *s = qemu_get_nic_opaque(nc); > > int packet_type; > > uint32_t available, address; > > - int width, rx_len, padded_len; > > + int rx_len, padded_len; > > uint32_t checksum; > > int size; > > @@ -775,10 +760,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, > > const uint8_t * buf, > > rx_len = pkt_size + sizeof(checksum); > > if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { > > - width = 2; > > padded_len = ((rx_len - 1) | 3) + 1; > > } else { > > - width = 1; > > padded_len = ((rx_len - 1) | 1) + 1; > > } > > @@ -799,11 +782,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, > > const uint8_t * buf, > > /* Check for EOL */ > > if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { > > /* Are we still in resource exhaustion? */ > > - size = sizeof(uint16_t) * 1 * width; > > - address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width; > > - address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED, > > - s->data, size); > > - s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0); > > + s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5); > > if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { > > /* Still EOL ; stop reception */ > > return -1; > > @@ -811,11 +790,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, > > const uint8_t * buf, > > /* Link has been updated by host */ > > /* Clear in_use */ > > - size = sizeof(uint16_t) * width; > > - address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; > > - dp8393x_put(s, width, 0, 0); > > - address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, > > - (uint8_t *)s->data, size); > > + dp8393x_put(s, dp8393x_crda(s), 6, 0x0000); > > /* Move to next descriptor */ > > s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; > > @@ -869,32 +844,20 @@ static ssize_t dp8393x_receive(NetClientState *nc, > > const uint8_t * buf, > > /* Write status to memory */ > > trace_dp8393x_receive_write_status(dp8393x_crda(s)); > > - dp8393x_put(s, width, 0, s->regs[SONIC_RCR]); /* status */ > > - dp8393x_put(s, width, 1, rx_len); /* byte count */ > > - dp8393x_put(s, width, 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */ > > - dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */ > > - dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */ > > - size = sizeof(uint16_t) * 5 * width; > > - address_space_write(&s->as, dp8393x_crda(s), > > - MEMTXATTRS_UNSPECIFIED, > > - s->data, size); > > + dp8393x_put(s, dp8393x_crda(s), 0, s->regs[SONIC_RCR]); /* status */ > > + dp8393x_put(s, dp8393x_crda(s), 1, rx_len); /* byte count */ > > + dp8393x_put(s, dp8393x_crda(s), 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 > > */ > > + dp8393x_put(s, dp8393x_crda(s), 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 > > */ > > + dp8393x_put(s, dp8393x_crda(s), 4, s->regs[SONIC_RSC]); /* seq_no */ > > /* Check link field */ > > - size = sizeof(uint16_t) * width; > > - address_space_read(&s->as, > > - dp8393x_crda(s) + sizeof(uint16_t) * 5 * width, > > - MEMTXATTRS_UNSPECIFIED, s->data, size); > > - s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0); > > + s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5); > > if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { > > /* EOL detected */ > > s->regs[SONIC_ISR] |= SONIC_ISR_RDE; > > } else { > > /* Clear in_use */ > > - size = sizeof(uint16_t) * width; > > - address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; > > - dp8393x_put(s, width, 0, 0); > > - address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, > > - s->data, size); > > + dp8393x_put(s, dp8393x_crda(s), 6, 0x0000); > > /* Move to next descriptor */ > > s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; > > This patch breaks networking in its current form, but I was able to make it > work by applying the following diff: > > > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c > index db9cfd786f..b08843172b 100644 > --- a/hw/net/dp8393x.c > +++ b/hw/net/dp8393x.c > @@ -218,20 +218,20 @@ static uint32_t dp8393x_wt(dp8393xState *s) > return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0]; > } > > -static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned ofs16) > +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, int offset) > { > const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; > uint16_t val; > > if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { > - addr += 2 * ofs16; > + addr += offset << 2; > if (s->big_endian) { > val = address_space_ldl_be(&s->as, addr, attrs, NULL); > } else { > val = address_space_ldl_le(&s->as, addr, attrs, NULL); > } > } else { > - addr += 1 * ofs16; > + addr += offset << 1; > if (s->big_endian) { > val = address_space_lduw_be(&s->as, addr, attrs, NULL); > } else { > @@ -243,19 +243,19 @@ static uint16_t dp8393x_get(dp8393xState *s, hwaddr > addr, unsigned ofs16) > } > > static void dp8393x_put(dp8393xState *s, > - hwaddr addr, unsigned ofs16, uint16_t val) > + hwaddr addr, int offset, uint16_t val) > { > const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; > > if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { > - addr += 2 * ofs16; > + addr += offset << 2; > if (s->big_endian) { > address_space_stl_be(&s->as, addr, val, attrs, NULL); > } else { > address_space_stl_le(&s->as, addr, val, attrs, NULL); > } > } else { > - addr += 1 * ofs16; > + addr += offset << 1; > if (s->big_endian) { > address_space_stw_be(&s->as, addr, val, attrs, NULL); > } else { > @@ -431,12 +431,12 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) > tx_len = 0; > > /* Update registers */ > - s->regs[SONIC_TCR] = dp8393x_get(s, dp8393x_ttda(s), 0) & 0xf000; > - s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 1); > - s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 2); > - s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 3); > - s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 4); > - s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 5); > + s->regs[SONIC_TCR] = dp8393x_get(s, dp8393x_ttda(s), 1) & 0xf000; > + s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 2); > + s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 3); > + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 4); > + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 5); > + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 6); > > /* Handle programmable interrupt */ > if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) { > @@ -458,9 +458,9 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) > i++; > if (i != s->regs[SONIC_TFC]) { > /* Read next fragment details */ > - s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 0); > - s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 1); > - s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 2); > + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 4 + 3 * > i); > + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 5 + 3 * > i); > + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 6 + 3 * > i); > } > } > > > The main change is that the offset argument for dp8393x_get() and > dp8393x_put() is actually an entry number and not a count of 16 bit words. > Other than that there were a couple of offset calculations that needed > adjustment to get things working again. > > Taking git master and applying your outstanding PR + this series without patch > 3 + this diff gave me working networking on Linux/m68k, MacOS 8.0 and > NetBSD/arc. > Nice job getting MacOS 8.0 to run! That functionality could be very useful for working on the Linux bootloaders (Penguin and EMILE) as they don't work under MESS/MAME or Mini VMac etc. > Unfortunately I don't have a test mips64el image available to see if this > combination works for Linux. Phil, do you have a suitable test kernel and > rootfs image available to allow this to be tested? > You can build and boot a mipsel vmlinux by following the steps I described previously. In the kernel messages you'll see the jazzsonic driver attempt to probe the device. When it succeeds, you'll see the MAC address reported. You can also observe the regression I reported with regards to patch 2/6, "dp8393x: don't force 32-bit register access". > > ATB, > > Mark. >
On 05/07/2021 02:36, Finn Thain wrote:
>> Unfortunately I don't have a test mips64el image available to see if this
>> combination works for Linux. Phil, do you have a suitable test kernel and
>> rootfs image available to allow this to be tested?
>>
>
> You can build and boot a mipsel vmlinux by following the steps I described
> previously. In the kernel messages you'll see the jazzsonic driver attempt
> to probe the device. When it succeeds, you'll see the MAC address
> reported. You can also observe the regression I reported with regards to
> patch 2/6, "dp8393x: don't force 32-bit register access".
Those instructions are useful, but since I am not a MIPS developer I don't have an
existing toolchain/kernel tree and rootfs available to test this.
If you can provide me with a link to your vmlinux and rootfs with busybox or similar
in it, I can take a look to see what is happening here. Otherwise it's almost
impossible for me to understand and debug the problem you are seeing on your setup.
ATB,
Mark.
On 7/4/21 4:48 PM, Mark Cave-Ayland wrote: > On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote: > >> Per the DP83932C datasheet from July 1995: >> >> 4.0 SONIC Registers >> 4.1 THE CAM UNIT >> >> The Content Addressable Memory (CAM) consists of sixteen >> 48-bit entries for complete address filtering of network >> packets. Each entry corresponds to a 48-bit destination >> address that is user programmable and can contain any >> combination of Multicast or Physical addresses. Each entry >> is partitioned into three 16-bit CAM cells accessible >> through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with >> CAP0 corresponding to the least significant 16 bits of >> the Destination Address and CAP2 corresponding to the >> most significant bits. >> >> Store the CAM registers as 16-bit as it simplifies the code. >> There is no change in the migration stream. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/net/dp8393x.c | 23 ++++++++++------------- >> 1 file changed, 10 insertions(+), 13 deletions(-) >> @@ -987,7 +984,7 @@ static const VMStateDescription vmstate_dp8393x = { >> .version_id = 0, >> .minimum_version_id = 0, >> .fields = (VMStateField []) { >> - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6), >> + VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2), >> VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40), >> VMSTATE_END_OF_LIST() >> } > > I'd still be inclined to change VMSTATE_BUFFER_UNSAFE for > VMSTATE_UINT16_ARRAY whilst you can do it without having to worry about > the migration stream being already broken, but anyhow: > > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Do you want me to squash: -- >8 -- diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 1d1837dbd38..4c2fa0aabbd 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -951,10 +951,10 @@ static void dp8393x_realize(DeviceState *dev, Error **errp) static const VMStateDescription vmstate_dp8393x = { .name = "dp8393x", - .version_id = 0, - .minimum_version_id = 0, + .version_id = 1, + .minimum_version_id = 1, .fields = (VMStateField []) { - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2), + VMSTATE_UINT16_ARRAY(cam, dp8393xState, 0, 16 * 3), VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40), VMSTATE_END_OF_LIST() } --- Or send it as a new patch?
On 06/07/2021 18:29, Philippe Mathieu-Daudé wrote:
> On 7/4/21 4:48 PM, Mark Cave-Ayland wrote:
>> On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
>>
>>> Per the DP83932C datasheet from July 1995:
>>>
>>> 4.0 SONIC Registers
>>> 4.1 THE CAM UNIT
>>>
>>> The Content Addressable Memory (CAM) consists of sixteen
>>> 48-bit entries for complete address filtering of network
>>> packets. Each entry corresponds to a 48-bit destination
>>> address that is user programmable and can contain any
>>> combination of Multicast or Physical addresses. Each entry
>>> is partitioned into three 16-bit CAM cells accessible
>>> through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with
>>> CAP0 corresponding to the least significant 16 bits of
>>> the Destination Address and CAP2 corresponding to the
>>> most significant bits.
>>>
>>> Store the CAM registers as 16-bit as it simplifies the code.
>>> There is no change in the migration stream.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> hw/net/dp8393x.c | 23 ++++++++++-------------
>>> 1 file changed, 10 insertions(+), 13 deletions(-)
>
>>> @@ -987,7 +984,7 @@ static const VMStateDescription vmstate_dp8393x = {
>>> .version_id = 0,
>>> .minimum_version_id = 0,
>>> .fields = (VMStateField []) {
>>> - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6),
>>> + VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2),
>>> VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
>>> VMSTATE_END_OF_LIST()
>>> }
>>
>> I'd still be inclined to change VMSTATE_BUFFER_UNSAFE for
>> VMSTATE_UINT16_ARRAY whilst you can do it without having to worry about
>> the migration stream being already broken, but anyhow:
>>
>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
> Do you want me to squash:
>
> -- >8 --
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 1d1837dbd38..4c2fa0aabbd 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -951,10 +951,10 @@ static void dp8393x_realize(DeviceState *dev,
> Error **errp)
>
> static const VMStateDescription vmstate_dp8393x = {
> .name = "dp8393x",
> - .version_id = 0,
> - .minimum_version_id = 0,
> + .version_id = 1,
> + .minimum_version_id = 1,
> .fields = (VMStateField []) {
> - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2),
> + VMSTATE_UINT16_ARRAY(cam, dp8393xState, 0, 16 * 3),
> VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
> VMSTATE_END_OF_LIST()
> }
> ---
>
> Or send it as a new patch?
I don't mind either way. I think VMSTATE_UINT16_ARRAY is nicer, and it's very rare
that you get the freedom to make a migration change like this without having to worry
about compatibility. It's also really quick and easy to test.
If it passes your local tests and you would prefer to squash rather than re-post then
you can also add a:
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
to the patchset. I included the list of guests I tested in the cover note, but forgot
to explicitly add the T-b tag.
ATB,
Mark.
On Mon, 5 Jul 2021, Mark Cave-Ayland wrote: > On 05/07/2021 02:36, Finn Thain wrote: > > > > Unfortunately I don't have a test mips64el image available to see if > > > this combination works for Linux. Phil, do you have a suitable test > > > kernel and rootfs image available to allow this to be tested? > > > > > > > You can build and boot a mipsel vmlinux by following the steps I > > described previously. In the kernel messages you'll see the jazzsonic > > driver attempt to probe the device. When it succeeds, you'll see the > > MAC address reported. You can also observe the regression I reported > > with regards to patch 2/6, "dp8393x: don't force 32-bit register > > access". > > Those instructions are useful, but since I am not a MIPS developer I > don't have an existing toolchain/kernel tree and rootfs available to > test this. > You don't need a rootfs to see the jazzsonic driver messages. But if you still want one, you could try the mipsel builds from these distros (not the 64-bit ones): https://ftp.jaist.ac.jp/pub/Linux/Gentoo/experimental/mips/stages/ https://landley.net/aboriginal/downloads/binaries/ > If you can provide me with a link to your vmlinux and rootfs with > busybox or similar in it, I can take a look to see what is happening > here. Otherwise it's almost impossible for me to understand and debug > the problem you are seeing on your setup. > Uploading kernels is a hassle (for me) as it brings a trust question and requires a file hosting service. I really should use PGP and organise a web of trust but that's very difficult given my rural location.
On 07/07/2021 02:30, Finn Thain wrote: > On Mon, 5 Jul 2021, Mark Cave-Ayland wrote: > >> On 05/07/2021 02:36, Finn Thain wrote: >> >>>> Unfortunately I don't have a test mips64el image available to see if >>>> this combination works for Linux. Phil, do you have a suitable test >>>> kernel and rootfs image available to allow this to be tested? >>>> >>> >>> You can build and boot a mipsel vmlinux by following the steps I >>> described previously. In the kernel messages you'll see the jazzsonic >>> driver attempt to probe the device. When it succeeds, you'll see the >>> MAC address reported. You can also observe the regression I reported >>> with regards to patch 2/6, "dp8393x: don't force 32-bit register >>> access". >> >> Those instructions are useful, but since I am not a MIPS developer I >> don't have an existing toolchain/kernel tree and rootfs available to >> test this. >> > > You don't need a rootfs to see the jazzsonic driver messages. But if you > still want one, you could try the mipsel builds from these distros (not > the 64-bit ones): > > https://ftp.jaist.ac.jp/pub/Linux/Gentoo/experimental/mips/stages/ > https://landley.net/aboriginal/downloads/binaries/ That's true, but then this wouldn't enable testing of Phil's proposed CRC changes. Having a simple shell with ping and wget/curl is a real help here. >> If you can provide me with a link to your vmlinux and rootfs with >> busybox or similar in it, I can take a look to see what is happening >> here. Otherwise it's almost impossible for me to understand and debug >> the problem you are seeing on your setup. >> > > Uploading kernels is a hassle (for me) as it brings a trust question and > requires a file hosting service. I really should use PGP and organise a > web of trust but that's very difficult given my rural location. Given that these are only running in a VM I'm not too worried about trust. I also have a VPS with scp access that I could temporarily grant you access via an SSH public key if that helps? ATB, Mark.
On Wed, 7 Jul 2021, Mark Cave-Ayland wrote: > > You don't need a rootfs to see the jazzsonic driver messages. But if > > you still want one, you could try the mipsel builds from these distros > > (not the 64-bit ones): > > > > https://ftp.jaist.ac.jp/pub/Linux/Gentoo/experimental/mips/stages/ When I tried following my own advice I ran into ABI compatibility problems. It looks like my kernel build doesn't like those binaries, but maybe it's a limitation of the Magnum CPU... ... Run /bin/sh as init process request_module: kmod_concurrent_max (0) close to 0 (max_modprobes: 50), for module binfmt-464c, throttling... request_module: modprobe binfmt-464c cannot be processed, kmod busy with 50 threads for more than 5 seconds now Kernel panic - not syncing: Requested init /bin/sh failed (error -8). > > https://landley.net/aboriginal/downloads/binaries/ The binaries from Aboriginal Linux work okay (that is, rootfs.cpio.gz found in system-image-mipsel.tar.gz). I got a shell prompt using init=/bin/sh but there's no networking support in this minimal busybox build unfortunately. Those binaries have the same ABI as the ones in my busybox build: $ file busybox busybox: ELF 32-bit LSB executable, MIPS, MIPS-I version 1 (SYSV), statically linked, stripped Whereas, Debian/mipsel binaries (like the Gentoo ones) look like this: $ file busybox busybox: ELF 32-bit LSB pie executable, MIPS, MIPS32 rel2 version 1 (SYSV), dynamically linked, interpreter /lib/ld.so.1, for GNU/Linux 3.2.0, BuildID[sha1]=febe1809f2ad8dacb067dfd74505b19c6c69ba65, stripped Eventually I found this page, https://wiki.debian.org/MIPSPort which explains that the Debian/mipsel port switched ABI between Debian 8 and Debian 9. Unfortunately, the Debian 7 and 8 installer ISO images have no initrd so they are no use. I got them from this archive: https://cdimage.debian.org/cdimage/archive/ Anyway, the Debian 8 binaries look like this, and they work too: # file bin/dash bin/dash: ELF 32-bit LSB shared object, MIPS, MIPS-II version 1 (SYSV), dynamically linked, interpreter /lib/ld.so.1, for GNU/Linux 2.6.32, BuildID[sha1]=44f7c1d61d9941db2b1de5dd9629c99e06c30ea8, stripped > > That's true, but then this wouldn't enable testing of Phil's proposed > CRC changes. Having a simple shell with ping and wget/curl is a real > help here. > To generate network traffic you can get the kernel to configure the NIC over DHCP but that does require a different kernel config (see below). > > > If you can provide me with a link to your vmlinux and rootfs with > > > busybox or similar in it, I can take a look to see what is happening > > > here. Otherwise it's almost impossible for me to understand and > > > debug the problem you are seeing on your setup. > > > > > > > Uploading kernels is a hassle (for me) as it brings a trust question > > and requires a file hosting service. I really should use PGP and > > organise a web of trust but that's very difficult given my rural > > location. > > Given that these are only running in a VM I'm not too worried about > trust. Well, untrusted images are okay as long as we are talking about debugging QEMU issues that are not exploitable... With a bit of searching I was able to find a Debian/mipsel 8 rootfs at https://github.com/jubinson/debian-rootfs I can't vouch for it though. It appears to be a page of links to tar files in someone's dropbox. $ sha256sum mipsel-rootfs-20170318T103423Z.tar.gz e6ed1871b29317c85170a07621966a013951ced1c5fb8d679b7519996b803fe8 mipsel-rootfs-20170318T103423Z.tar.gz > I also have a VPS with scp access that I could temporarily grant you > access via an SSH public key if that helps? > Thanks for the offer. But that wouldn't help anyone else reading this. In anycase, I wanted to see whether a real distro could be used. So my plan was to cross-compile a Linux kernel and debootstrap a Debian/mipsel rootfs disk image. The first stage of a debootstrap installation runs on the host... sudo -s truncate -s 1G jessie-mipsel.img mke2fs jessie-mipsel.img mount -o loop jessie-mipsel.img /mnt wget -q -O- https://ftp-master.debian.org/keys/release-8.asc | gpg --import --no-default-keyring --keyring ./debian-release-8.gpg debootstrap --keyring=./debian-release-8.gpg --foreign --arch=mipsel jessie /mnt http://archive.debian.org/debian/ umount /mnt exit git clone git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git cd linux git checkout linux-5.10.y make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- clean jazz_defconfig scripts/config -d IPV6 -d WIRELESS -d WLAN -d DEBUG_KERNEL -d EXPERT -d CC_OPTIMIZE_FOR_PERFORMANCE -e CC_OPTIMIZE_FOR_SIZE -e ISO9660_FS -m EXT3_FS -e NFS_FS -e IP_PNP -e ROOT_NFS -e NFS_V2 -e IP_PNP_DHCP -e CMDLINE_BOOL -e MIPS_CMDLINE_BUILTIN_EXTEND --set-str CMDLINE "console=ttyS0 root=/dev/nfs rw" make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- olddefconfig vmlinux mkisofs -o vmlinux.iso -J -iso-level 3 vmlinux qemu-system-mips64el -M magnum -L . -drive if=scsi,unit=0,format=raw,file=jessie-mipsel.img -drive if=scsi,unit=2,readonly=y,media=cdrom,format=raw,file=vmlinux.iso -drive if=scsi,unit=4,readonly=y,media=cdrom,format=raw,file=NetBSD-9.2-arc.iso -global ds1225y.filename=nvram -serial mon:stdio -serial null -net nic,model=dp83932,addr=00:00:00:aa:bb:cc -net bridge Actions: Start Windows NT -> Run a program Run setup scsi(0)cdrom(4)fdisk(0)boot scsi(0)cdrom(2)fdisk(0)vmlinux root=/dev/sda init=/bin/sh This boots to a shell prompt. Well, so far, so good. Now the rest of the debootstrap installation can be completed by the guest... mount -t devtmpfs none /dev mount -t proc none /proc mount -t sysfs none /sys /debootstrap/debootstrap --second-stage Unfortunately, this produced a badly corrupted root filesystem and the installation failed. Seems to be a bug in either Linux/mipsel or QEMU. (Has anyone tried a NetBSD installation?) I was able to avoid all that block IO entirely by using NFS. That way, the installation completed successfully... mount -o loop jessie-mipsel.img /export/jessie-mipsel echo "/export/jessie-mipsel 192.168.66.0/24(sync,rw,insecure,no_root_squash,no_subtree_check)" >> /etc/exports /etc/init.d/nfs start qemu-system-mips64el -M magnum -L . -drive if=scsi,unit=2,readonly=y,media=cdrom,format=raw,file=vmlinux.iso -drive if=scsi,unit=4,readonly=y,media=cdrom,format=raw,file=NetBSD-9.2-arc.iso -global ds1225y.filename=nvram -serial mon:stdio -serial null -net nic,model=dp83932,addr=00:00:00:aa:bb:cc -net bridge Actions: Start Windows NT -> Run a program Run setup scsi(0)cdrom(4)fdisk(0)boot scsi(0)cdrom(2)fdisk(0)vmlinux ip=dhcp nfsroot=192.168.66.1:/export/jessie-mipsel init=/bin/sh NetBSD/arc Bootstrap, Revision 1.1 (Wed May 12 13:15:55 UTC 2021) devopen: scsi(0)cdrom(2)fdisk(0) type disk file vmlinux 5706728+198952 [840960+1017335]=0x767d14 Linux version 5.10.47 (fthain@nippy) (mipsel-linux-gnu-gcc (btc) 6.4.0, GNU ld (btc) 2.28) #2 Fri Jul 9 16:28:12 AEST 2021 ARCH: Microsoft-Jazz PROMLIB: ARC firmware Version 1 Revision 2 CPU0 revision is: 00000400 (R4000PC) FPU revision is: 00000500 Primary instruction cache 8kB, VIPT, direct mapped, linesize 16 bytes. Primary data cache 8kB, direct mapped, VIPT, cache aliases, linesize 16 bytes Zone ranges: DMA [mem 0x0000000000000000-0x0000000000ffffff] Normal [mem 0x0000000001000000-0x0000000007ffffff] Movable zone start for each node Early memory node ranges node 0: [mem 0x0000000000000000-0x0000000007ffffff] Initmem setup node 0 [mem 0x0000000000000000-0x0000000007ffffff] Built 1 zonelists, mobility grouping on. Total pages: 32512 Kernel command line: console=ttyS0 root=/dev/nfs rw scsi(0)cdrom(2)fdisk(0)vmlinux ip=dhcp nfsroot=192.168.66.1:/export/jessie-mipsel init=/bin/sh Dentry cache hash table entries: 16384 (order: 4, 65536 bytes, linear) Inode-cache hash table entries: 8192 (order: 3, 32768 bytes, linear) mem auto-init: stack:off, heap alloc:off, heap free:off Memory: 123668K/131072K available (4249K kernel code, 234K rwdata, 928K rodata, 212K init, 135K bss, 7404K reserved, 0K cma-reserved) NR_IRQS: 256 random: get_random_bytes called from start_kernel+0x300/0x4b0 with crng_init=0 Console: colour dummy device 80x25 sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every 21474836475000000ns Calibrating delay loop... 1404.10 BogoMIPS (lpj=7020544) pid_max: default: 32768 minimum: 301 Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear) Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear) devtmpfs: initialized clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns futex hash table entries: 256 (order: -1, 3072 bytes, linear) NET: Registered protocol family 16 VDMA: R4030 DMA pagetables initialized. SCSI subsystem initialized NET: Registered protocol family 2 IP idents hash table entries: 2048 (order: 2, 16384 bytes, linear) tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 4096 bytes, linear) TCP established hash table entries: 1024 (order: 0, 4096 bytes, linear) TCP bind hash table entries: 1024 (order: 0, 4096 bytes, linear) TCP: Hash tables configured (established 1024 bind 1024) UDP hash table entries: 256 (order: 0, 4096 bytes, linear) UDP-Lite hash table entries: 256 (order: 0, 4096 bytes, linear) NET: Registered protocol family 1 RPC: Registered named UNIX socket transport module. RPC: Registered udp transport module. RPC: Registered tcp transport module. RPC: Registered tcp NFSv4.1 backchannel transport module. workingset: timestamp_bits=30 max_order=15 bucket_order=0 Block layer SCSI generic (bsg) driver version 0.4 loaded (major 254) io scheduler mq-deadline registered io scheduler kyber registered Console: switching to colour frame buffer device 100x37 Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled printk: console [ttyS0] disabled serial8250.0: ttyS0 at MMIO 0xe0006000 (irq = 32, base_baud = 115200) is a 16550A printk: console [ttyS0] enabled serial8250.0: ttyS1 at MMIO 0xe0007000 (irq = 33, base_baud = 115200) is a 16550A jazz_esp jazz_esp.0: esp0: regs[(ptrval):0] irq[29] jazz_esp jazz_esp.0: esp0: is a FAS100A, 40 MHz (ccf=0), SCSI ID 7 random: fast init done scsi host0: esp PDC20230-C/20630 VLB ATA controller detected. scsi host1: pata_legacy ata1: PATA max PIO2 cmd 0x1f0 ctl 0x3f6 irq 14 scsi 0:0:2:0: CD-ROM QEMU QEMU CD-ROM 2.5+ PQ: 0 ANSI: 5 scsi target0:0:2: Beginning Domain Validation VDMA: Channel 0: Memory error! VDMA: Channel 0: Memory error! VDMA: Channel 0: Memory error! scsi target0:0:2: Domain Validation skipping write tests scsi target0:0:2: Ending Domain Validation VDMA: Channel 0: Memory error! scsi 0:0:4:0: CD-ROM QEMU QEMU CD-ROM 2.5+ PQ: 0 ANSI: 5 scsi target0:0:4: Beginning Domain Validation VDMA: Channel 0: Memory error! VDMA: Channel 0: Memory error! VDMA: Channel 0: Memory error! scsi target0:0:4: Domain Validation skipping write tests scsi target0:0:4: Ending Domain Validation VDMA: Channel 0: Memory error! scsi host1: pata_legacy ata2: PATA max PIO4 cmd 0x170 ctl 0x376 irq 15 scsi host1: pata_legacy ata3: PATA max PIO4 cmd 0x1e8 ctl 0x3ee irq 11 scsi host1: pata_legacy ata4: PATA max PIO4 cmd 0x168 ctl 0x36e irq 10 scsi host1: pata_legacy ata5: PATA max PIO4 cmd 0x1e0 ctl 0x3e6 irq 8 scsi host1: pata_legacy ata6: PATA max PIO4 cmd 0x160 ctl 0x366 irq 12 SONIC ethernet @e0001000, MAC 52:54:00:12:34:56, IRQ 28 serio: i8042 KBD port at 0xe0005000,0xe0005001 irq 30 serio: i8042 AUX port at 0xe0005000,0xe0005001 irq 31 input: AT Raw Set 2 keyboard as /devices/platform/i8042/serio0/input/input0 Sending DHCP requests ., OK IP-Config: Got DHCP answer from 192.168.66.1, my address is 192.168.66.112 IP-Config: Complete: device=eth0, hwaddr=52:54:00:12:34:56, ipaddr=192.168.66.112, mask=255.255.255.0, gw=192.168.66.1 host=192.168.66.112, domain=local, nis-domain=(none) bootserver=0.0.0.0, rootserver=192.168.66.1, rootpath= nameserver0=139.130.4.4 input: ImExPS/2 Generic Explorer Mouse as /devices/platform/i8042/serio1/input/input2 VFS: Mounted root (nfs filesystem) on device 0:12. Freeing prom memory: 376k freed Freeing prom memory: 60k freed Freeing prom memory: 4k freed Freeing unused kernel memory: 212K This architecture does not have kernel memory protection. Run /bin/sh as init process process '/bin/dash' started with executable stack /bin/sh: 0: can't access tty; job control turned off # mount -t devtmpfs none /dev # mount -t proc none /proc # mount -t sysfs none /sys # /debootstrap/debootstrap --second-stage ... # ping -c3 192.168.66.1 PING 192.168.66.1 (192.168.66.1) 56(84) bytes of data. 64 bytes from 192.168.66.1: icmp_seq=1 ttl=64 time=10.0 ms 64 bytes from 192.168.66.1: icmp_seq=2 ttl=64 time=0.000 ms 64 bytes from 192.168.66.1: icmp_seq=3 ttl=64 time=0.000 ms --- 192.168.66.1 ping statistics --- 3 packets transmitted, 3 received, 0% packet loss, time 2030ms rtt min/avg/max/mdev = 0.000/3.333/10.000/4.714 ms # After the debootstrap command completes, the last steps are manual configuration, as per the usual debootstrap procedure: https://wiki.debian.org/Debootstrap https://gist.github.com/varqox/42e213b6b2dde2b636ef Note that this only _barely_ works. A slightly larger vmlinux breaks the bootloader, and a slightly longer boot command breaks ARC.