* [RFC PATCH 0/6] dp8393x: Housekeeping @ 2021-07-03 14:19 Philippe Mathieu-Daudé 2021-07-03 14:19 ` [PATCH 1/6] dp8393x: fix CAM descriptor entry index Philippe Mathieu-Daudé ` (5 more replies) 0 siblings, 6 replies; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-03 14:19 UTC (permalink / raw) To: qemu-devel Cc: Jason Wang, Mark Cave-Ayland, Laurent Vivier, Finn Thain, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/6] dp8393x: fix CAM descriptor entry index 2021-07-03 14:19 [RFC PATCH 0/6] dp8393x: Housekeeping Philippe Mathieu-Daudé @ 2021-07-03 14:19 ` Philippe Mathieu-Daudé 2021-07-03 14:19 ` [PATCH 2/6] dp8393x: don't force 32-bit register access Philippe Mathieu-Daudé ` (4 subsequent siblings) 5 siblings, 0 replies; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-03 14:19 UTC (permalink / raw) To: qemu-devel Cc: Jason Wang, Mark Cave-Ayland, Laurent Vivier, Finn Thain, Philippe Mathieu-Daudé 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/6] dp8393x: don't force 32-bit register access 2021-07-03 14:19 [RFC PATCH 0/6] dp8393x: Housekeeping Philippe Mathieu-Daudé 2021-07-03 14:19 ` [PATCH 1/6] dp8393x: fix CAM descriptor entry index Philippe Mathieu-Daudé @ 2021-07-03 14:19 ` Philippe Mathieu-Daudé 2021-07-03 14:39 ` Mark Cave-Ayland 2021-07-04 2:06 ` Finn Thain 2021-07-03 14:19 ` [RFC PATCH 3/6] dp8393x: Restrict bus access to 16/32-bit operations Philippe Mathieu-Daudé ` (3 subsequent siblings) 5 siblings, 2 replies; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-03 14:19 UTC (permalink / raw) To: qemu-devel Cc: Jason Wang, Mark Cave-Ayland, Laurent Vivier, Finn Thain, Philippe Mathieu-Daudé 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 2/6] dp8393x: don't force 32-bit register access 2021-07-03 14:19 ` [PATCH 2/6] dp8393x: don't force 32-bit register access Philippe Mathieu-Daudé @ 2021-07-03 14:39 ` Mark Cave-Ayland 2021-07-03 16:29 ` Philippe Mathieu-Daudé 2021-07-04 2:06 ` Finn Thain 1 sibling, 1 reply; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-03 14:39 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Jason Wang, Laurent Vivier, Finn Thain 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/6] dp8393x: don't force 32-bit register access 2021-07-03 14:39 ` Mark Cave-Ayland @ 2021-07-03 16:29 ` Philippe Mathieu-Daudé 2021-07-04 15:34 ` Mark Cave-Ayland 0 siblings, 1 reply; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-03 16:29 UTC (permalink / raw) To: Mark Cave-Ayland, qemu-devel; +Cc: Jason Wang, Laurent Vivier, Finn Thain 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(-) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/6] dp8393x: don't force 32-bit register access 2021-07-03 16:29 ` Philippe Mathieu-Daudé @ 2021-07-04 15:34 ` Mark Cave-Ayland 0 siblings, 0 replies; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-04 15:34 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Jason Wang, Laurent Vivier, Finn Thain 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/6] dp8393x: don't force 32-bit register access 2021-07-03 14:19 ` [PATCH 2/6] dp8393x: don't force 32-bit register access Philippe Mathieu-Daudé 2021-07-03 14:39 ` Mark Cave-Ayland @ 2021-07-04 2:06 ` Finn Thain 1 sibling, 0 replies; 28+ messages in thread From: Finn Thain @ 2021-07-04 2:06 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Jason Wang, Mark Cave-Ayland, qemu-devel, Laurent Vivier [-- 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> ^ permalink raw reply [flat|nested] 28+ messages in thread
* [RFC PATCH 3/6] dp8393x: Restrict bus access to 16/32-bit operations 2021-07-03 14:19 [RFC PATCH 0/6] dp8393x: Housekeeping Philippe Mathieu-Daudé 2021-07-03 14:19 ` [PATCH 1/6] dp8393x: fix CAM descriptor entry index Philippe Mathieu-Daudé 2021-07-03 14:19 ` [PATCH 2/6] dp8393x: don't force 32-bit register access Philippe Mathieu-Daudé @ 2021-07-03 14:19 ` Philippe Mathieu-Daudé 2021-07-03 14:52 ` Mark Cave-Ayland 2021-07-04 14:45 ` Mark Cave-Ayland 2021-07-03 14:19 ` [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit Philippe Mathieu-Daudé ` (2 subsequent siblings) 5 siblings, 2 replies; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-03 14:19 UTC (permalink / raw) To: qemu-devel Cc: Jason Wang, Mark Cave-Ayland, Laurent Vivier, Finn Thain, Philippe Mathieu-Daudé 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 3/6] dp8393x: Restrict bus access to 16/32-bit operations 2021-07-03 14:19 ` [RFC PATCH 3/6] dp8393x: Restrict bus access to 16/32-bit operations Philippe Mathieu-Daudé @ 2021-07-03 14:52 ` Mark Cave-Ayland 2021-07-04 14:45 ` Mark Cave-Ayland 1 sibling, 0 replies; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-03 14:52 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Jason Wang, Laurent Vivier, Finn Thain 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 3/6] dp8393x: Restrict bus access to 16/32-bit operations 2021-07-03 14:19 ` [RFC PATCH 3/6] dp8393x: Restrict bus access to 16/32-bit operations Philippe Mathieu-Daudé 2021-07-03 14:52 ` Mark Cave-Ayland @ 2021-07-04 14:45 ` Mark Cave-Ayland 1 sibling, 0 replies; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-04 14:45 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Jason Wang, Laurent Vivier, Finn Thain 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit 2021-07-03 14:19 [RFC PATCH 0/6] dp8393x: Housekeeping Philippe Mathieu-Daudé ` (2 preceding siblings ...) 2021-07-03 14:19 ` [RFC PATCH 3/6] dp8393x: Restrict bus access to 16/32-bit operations Philippe Mathieu-Daudé @ 2021-07-03 14:19 ` Philippe Mathieu-Daudé 2021-07-03 14:56 ` Mark Cave-Ayland 2021-07-04 14:48 ` Mark Cave-Ayland 2021-07-03 14:19 ` [PATCH 5/6] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() Philippe Mathieu-Daudé 2021-07-03 14:19 ` [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() Philippe Mathieu-Daudé 5 siblings, 2 replies; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-03 14:19 UTC (permalink / raw) To: qemu-devel Cc: Jason Wang, Mark Cave-Ayland, Laurent Vivier, Finn Thain, Philippe Mathieu-Daudé 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit 2021-07-03 14:19 ` [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit Philippe Mathieu-Daudé @ 2021-07-03 14:56 ` Mark Cave-Ayland 2021-07-04 14:48 ` Mark Cave-Ayland 1 sibling, 0 replies; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-03 14:56 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Jason Wang, Laurent Vivier, Finn Thain 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit 2021-07-03 14:19 ` [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit Philippe Mathieu-Daudé 2021-07-03 14:56 ` Mark Cave-Ayland @ 2021-07-04 14:48 ` Mark Cave-Ayland 2021-07-06 17:29 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-04 14:48 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Jason Wang, Laurent Vivier, Finn Thain 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit 2021-07-04 14:48 ` Mark Cave-Ayland @ 2021-07-06 17:29 ` Philippe Mathieu-Daudé 2021-07-06 19:27 ` Mark Cave-Ayland 0 siblings, 1 reply; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-06 17:29 UTC (permalink / raw) To: Mark Cave-Ayland, qemu-devel; +Cc: Jason Wang, Laurent Vivier, Finn Thain 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? ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit 2021-07-06 17:29 ` Philippe Mathieu-Daudé @ 2021-07-06 19:27 ` Mark Cave-Ayland 0 siblings, 0 replies; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-06 19:27 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Jason Wang, Laurent Vivier, Finn Thain 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 5/6] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() 2021-07-03 14:19 [RFC PATCH 0/6] dp8393x: Housekeeping Philippe Mathieu-Daudé ` (3 preceding siblings ...) 2021-07-03 14:19 ` [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit Philippe Mathieu-Daudé @ 2021-07-03 14:19 ` Philippe Mathieu-Daudé 2021-07-03 14:57 ` Mark Cave-Ayland 2021-07-04 14:49 ` Mark Cave-Ayland 2021-07-03 14:19 ` [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() Philippe Mathieu-Daudé 5 siblings, 2 replies; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-03 14:19 UTC (permalink / raw) To: qemu-devel Cc: Jason Wang, Mark Cave-Ayland, Laurent Vivier, Finn Thain, Philippe Mathieu-Daudé 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 5/6] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() 2021-07-03 14:19 ` [PATCH 5/6] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() Philippe Mathieu-Daudé @ 2021-07-03 14:57 ` Mark Cave-Ayland 2021-07-04 14:49 ` Mark Cave-Ayland 1 sibling, 0 replies; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-03 14:57 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Jason Wang, Laurent Vivier, Finn Thain 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/6] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() 2021-07-03 14:19 ` [PATCH 5/6] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() Philippe Mathieu-Daudé 2021-07-03 14:57 ` Mark Cave-Ayland @ 2021-07-04 14:49 ` Mark Cave-Ayland 1 sibling, 0 replies; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-04 14:49 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Jason Wang, Laurent Vivier, Finn Thain 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() 2021-07-03 14:19 [RFC PATCH 0/6] dp8393x: Housekeeping Philippe Mathieu-Daudé ` (4 preceding siblings ...) 2021-07-03 14:19 ` [PATCH 5/6] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() Philippe Mathieu-Daudé @ 2021-07-03 14:19 ` Philippe Mathieu-Daudé 2021-07-03 15:00 ` Mark Cave-Ayland ` (2 more replies) 5 siblings, 3 replies; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-03 14:19 UTC (permalink / raw) To: qemu-devel Cc: Jason Wang, Mark Cave-Ayland, Laurent Vivier, Finn Thain, Philippe Mathieu-Daudé 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() 2021-07-03 14:19 ` [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() Philippe Mathieu-Daudé @ 2021-07-03 15:00 ` Mark Cave-Ayland 2021-07-03 15:04 ` Philippe Mathieu-Daudé 2021-07-04 1:46 ` Finn Thain 2021-07-04 15:07 ` Mark Cave-Ayland 2 siblings, 1 reply; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-03 15:00 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Jason Wang, Laurent Vivier, Finn Thain 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() 2021-07-03 15:00 ` Mark Cave-Ayland @ 2021-07-03 15:04 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-03 15:04 UTC (permalink / raw) To: Mark Cave-Ayland, qemu-devel; +Cc: Jason Wang, Laurent Vivier, Finn Thain 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). ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() 2021-07-03 14:19 ` [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() Philippe Mathieu-Daudé 2021-07-03 15:00 ` Mark Cave-Ayland @ 2021-07-04 1:46 ` Finn Thain 2021-07-04 15:07 ` Mark Cave-Ayland 2 siblings, 0 replies; 28+ messages in thread From: Finn Thain @ 2021-07-04 1:46 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Jason Wang, Mark Cave-Ayland, qemu-devel, Laurent Vivier [-- 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). ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() 2021-07-03 14:19 ` [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() Philippe Mathieu-Daudé 2021-07-03 15:00 ` Mark Cave-Ayland 2021-07-04 1:46 ` Finn Thain @ 2021-07-04 15:07 ` Mark Cave-Ayland 2021-07-05 1:36 ` Finn Thain 2 siblings, 1 reply; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-04 15:07 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Jason Wang, Laurent Vivier, Finn Thain 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. ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() 2021-07-04 15:07 ` Mark Cave-Ayland @ 2021-07-05 1:36 ` Finn Thain 2021-07-05 6:34 ` Mark Cave-Ayland 0 siblings, 1 reply; 28+ messages in thread From: Finn Thain @ 2021-07-05 1:36 UTC (permalink / raw) To: Mark Cave-Ayland Cc: Laurent Vivier, Jason Wang, Philippe Mathieu-Daudé, qemu-devel [-- 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. > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() 2021-07-05 1:36 ` Finn Thain @ 2021-07-05 6:34 ` Mark Cave-Ayland 2021-07-07 1:30 ` Finn Thain 0 siblings, 1 reply; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-05 6:34 UTC (permalink / raw) To: Finn Thain Cc: Laurent Vivier, Jason Wang, Philippe Mathieu-Daudé, qemu-devel 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() 2021-07-05 6:34 ` Mark Cave-Ayland @ 2021-07-07 1:30 ` Finn Thain 2021-07-07 10:12 ` Mark Cave-Ayland 0 siblings, 1 reply; 28+ messages in thread From: Finn Thain @ 2021-07-07 1:30 UTC (permalink / raw) To: Mark Cave-Ayland Cc: Laurent Vivier, Jason Wang, Philippe Mathieu-Daudé, qemu-devel 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() 2021-07-07 1:30 ` Finn Thain @ 2021-07-07 10:12 ` Mark Cave-Ayland 2021-07-09 9:13 ` Finn Thain 0 siblings, 1 reply; 28+ messages in thread From: Mark Cave-Ayland @ 2021-07-07 10:12 UTC (permalink / raw) To: Finn Thain Cc: qemu-devel, Jason Wang, Laurent Vivier, Philippe Mathieu-Daudé 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() 2021-07-07 10:12 ` Mark Cave-Ayland @ 2021-07-09 9:13 ` Finn Thain 0 siblings, 0 replies; 28+ messages in thread From: Finn Thain @ 2021-07-09 9:13 UTC (permalink / raw) To: Mark Cave-Ayland Cc: qemu-devel, Jason Wang, Laurent Vivier, Philippe Mathieu-Daudé 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2021-07-09 9:15 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-03 14:19 [RFC PATCH 0/6] dp8393x: Housekeeping Philippe Mathieu-Daudé 2021-07-03 14:19 ` [PATCH 1/6] dp8393x: fix CAM descriptor entry index Philippe Mathieu-Daudé 2021-07-03 14:19 ` [PATCH 2/6] dp8393x: don't force 32-bit register access Philippe Mathieu-Daudé 2021-07-03 14:39 ` Mark Cave-Ayland 2021-07-03 16:29 ` Philippe Mathieu-Daudé 2021-07-04 15:34 ` Mark Cave-Ayland 2021-07-04 2:06 ` Finn Thain 2021-07-03 14:19 ` [RFC PATCH 3/6] dp8393x: Restrict bus access to 16/32-bit operations Philippe Mathieu-Daudé 2021-07-03 14:52 ` Mark Cave-Ayland 2021-07-04 14:45 ` Mark Cave-Ayland 2021-07-03 14:19 ` [RFC PATCH 4/6] dp8393x: Store CAM registers as 16-bit Philippe Mathieu-Daudé 2021-07-03 14:56 ` Mark Cave-Ayland 2021-07-04 14:48 ` Mark Cave-Ayland 2021-07-06 17:29 ` Philippe Mathieu-Daudé 2021-07-06 19:27 ` Mark Cave-Ayland 2021-07-03 14:19 ` [PATCH 5/6] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() Philippe Mathieu-Daudé 2021-07-03 14:57 ` Mark Cave-Ayland 2021-07-04 14:49 ` Mark Cave-Ayland 2021-07-03 14:19 ` [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put() Philippe Mathieu-Daudé 2021-07-03 15:00 ` Mark Cave-Ayland 2021-07-03 15:04 ` Philippe Mathieu-Daudé 2021-07-04 1:46 ` Finn Thain 2021-07-04 15:07 ` Mark Cave-Ayland 2021-07-05 1:36 ` Finn Thain 2021-07-05 6:34 ` Mark Cave-Ayland 2021-07-07 1:30 ` Finn Thain 2021-07-07 10:12 ` Mark Cave-Ayland 2021-07-09 9:13 ` Finn Thain
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.