| Message ID | 1258664086-20264-1-git-send-email-weil@mail.berlios.de |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 19, 2009 at 09:54:46PM +0100, Stefan Weil wrote: > Handling of transmit commands is rather complex, > so about 80 lines of code were moved from function > action_command to the new function tx_command. > > The two new values "tx" and "cb_address" in the > eepro100 status structure made this possible without > passing too many parameters. > > In addition, the moved code was cleaned a little bit: > old comments marked with //~ were removed, C++ style > comments were replaced by C style comments, C++ like > variable declarations after code were reordered. > > Simplified mode is still broken. Nor did I fix > endianess issues. Both problems will be fixed in > additional patches (which need this one). > > Signed-off-by: Stefan Weil <weil@mail.berlios.de> > --- > hw/eepro100.c | 192 +++++++++++++++++++++++++++++---------------------------- > 1 files changed, 98 insertions(+), 94 deletions(-) > > diff --git a/hw/eepro100.c b/hw/eepro100.c > index 4210d8a..7093af8 100644 > --- a/hw/eepro100.c > +++ b/hw/eepro100.c > @@ -213,6 +213,10 @@ typedef struct { > uint32_t ru_offset; /* RU address offset */ > uint32_t statsaddr; /* pointer to eepro100_stats_t */ > > + /* Temporary data. */ > + eepro100_tx_t tx; > + uint32_t cb_address; > + That's pretty evil, as it makes routines non-reentrant. How bad is it to pass 2 additional arguments around? If not, maybe define struct tx_command and put necessary stuff there, then pass pointer to that? > /* Statistical counters. Also used for wake-up packet (i82559). */ > eepro100_stats_t statistics; > > @@ -748,17 +752,100 @@ static void dump_statistics(EEPRO100State * s) > //~ missing("CU dump statistical counters"); > } > > +static void tx_command(EEPRO100State *s) > +{ > + uint32_t tbd_array = le32_to_cpu(s->tx.tx_desc_addr); > + uint16_t tcb_bytes = (le16_to_cpu(s->tx.tcb_bytes) & 0x3fff); > + /* Sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes. */ > + uint8_t buf[2600]; > + uint16_t size = 0; > + uint32_t tbd_address = s->cb_address + 0x10; > + TRACE(RXTX, logout > + ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count %u\n", > + tbd_array, tcb_bytes, s->tx.tbd_count)); > + > + if (tcb_bytes > 2600) { > + logout("TCB byte count too large, using 2600\n"); > + tcb_bytes = 2600; > + } > + if (!((tcb_bytes > 0) || (tbd_array != 0xffffffff))) { > + logout > + ("illegal values of TBD array address and TCB byte count!\n"); > + } > + assert(tcb_bytes <= sizeof(buf)); > + while (size < tcb_bytes) { > + uint32_t tx_buffer_address = ldl_phys(tbd_address); > + uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); > + //~ uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); > + tbd_address += 8; > + TRACE(RXTX, logout > + ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n", > + tx_buffer_address, tx_buffer_size)); > + tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); > + cpu_physical_memory_read(tx_buffer_address, &buf[size], > + tx_buffer_size); > + size += tx_buffer_size; > + } > + if (tbd_array == 0xffffffff) { > + /* Simplified mode. Was already handled by code above. */ > + } else { > + /* Flexible mode. */ > + uint8_t tbd_count = 0; > + if (s->has_extended_tcb_support && !(s->configuration[6] & BIT(4))) { > + /* Extended Flexible TCB. */ > + for (; tbd_count < 2; tbd_count++) { > + uint32_t tx_buffer_address = ldl_phys(tbd_address); > + uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); > + uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); > + tbd_address += 8; > + TRACE(RXTX, logout > + ("TBD (extended flexible mode): buffer address 0x%08x, size 0x%04x\n", > + tx_buffer_address, tx_buffer_size)); > + tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); > + cpu_physical_memory_read(tx_buffer_address, &buf[size], > + tx_buffer_size); > + size += tx_buffer_size; > + if (tx_buffer_el & 1) { > + break; > + } > + } > + } > + tbd_address = tbd_array; > + for (; tbd_count < s->tx.tbd_count; tbd_count++) { > + uint32_t tx_buffer_address = ldl_phys(tbd_address); > + uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); > + uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); > + tbd_address += 8; > + TRACE(RXTX, logout > + ("TBD (flexible mode): buffer address 0x%08x, size 0x%04x\n", > + tx_buffer_address, tx_buffer_size)); > + tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); > + cpu_physical_memory_read(tx_buffer_address, &buf[size], > + tx_buffer_size); > + size += tx_buffer_size; > + if (tx_buffer_el & 1) { > + break; > + } > + } > + } > + TRACE(RXTX, logout("%p sending frame, len=%d,%s\n", s, size, nic_dump(buf, size))); > + qemu_send_packet(s->vc, buf, size); > + s->statistics.tx_good_frames++; > + /* Transmit with bad status would raise an CX/TNO interrupt. > + * (82557 only). Emulation never has bad status. */ > + //~ eepro100_cx_interrupt(s); > +} > + > static void action_command(EEPRO100State *s) > { > for (;;) { > - uint32_t cb_address = s->cu_base + s->cu_offset; > - eepro100_tx_t tx; > - cpu_physical_memory_read(cb_address, (uint8_t *) & tx, sizeof(tx)); > - uint16_t status = le16_to_cpu(tx.status); > - uint16_t command = le16_to_cpu(tx.command); > + s->cb_address = s->cu_base + s->cu_offset; > + cpu_physical_memory_read(s->cb_address, (uint8_t *)&s->tx, sizeof(s->tx)); > + uint16_t status = le16_to_cpu(s->tx.status); > + uint16_t command = le16_to_cpu(s->tx.command); > logout > ("val=0x%02x (cu start), status=0x%04x, command=0x%04x, link=0x%08x\n", > - val, status, command, tx.link); > + val, status, command, s->tx.link); > bool bit_el = ((command & 0x8000) != 0); > bool bit_s = ((command & 0x4000) != 0); > bool bit_i = ((command & 0x2000) != 0); > @@ -766,17 +853,17 @@ static void action_command(EEPRO100State *s) > bool success = true; > //~ bool bit_sf = ((command & 0x0008) != 0); > uint16_t cmd = command & 0x0007; > - s->cu_offset = le32_to_cpu(tx.link); > + s->cu_offset = le32_to_cpu(s->tx.link); > switch (cmd) { > case CmdNOp: > /* Do nothing. */ > break; > case CmdIASetup: > - cpu_physical_memory_read(cb_address + 8, &s->conf.macaddr.a[0], 6); > + cpu_physical_memory_read(s->cb_address + 8, &s->conf.macaddr.a[0], 6); > TRACE(OTHER, logout("macaddr: %s\n", nic_dump(&s->macaddr[0], 6))); > break; > case CmdConfigure: > - cpu_physical_memory_read(cb_address + 8, &s->configuration[0], > + cpu_physical_memory_read(s->cb_address + 8, &s->configuration[0], > sizeof(s->configuration)); > TRACE(OTHER, logout("configuration: %s\n", nic_dump(&s->configuration[0], 16))); > break; > @@ -784,95 +871,12 @@ static void action_command(EEPRO100State *s) > //~ missing("multicast list"); > break; > case CmdTx: > - (void)0; > - uint32_t tbd_array = le32_to_cpu(tx.tx_desc_addr); > - uint16_t tcb_bytes = (le16_to_cpu(tx.tcb_bytes) & 0x3fff); > - TRACE(RXTX, logout > - ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count %u\n", > - tbd_array, tcb_bytes, tx.tbd_count)); > - > if (bit_nc) { > missing("CmdTx: NC = 0"); > success = false; > break; > } > - //~ assert(!bit_sf); > - if (tcb_bytes > 2600) { > - logout("TCB byte count too large, using 2600\n"); > - tcb_bytes = 2600; > - } > - /* Next assertion fails for local configuration. */ > - //~ assert((tcb_bytes > 0) || (tbd_array != 0xffffffff)); > - if (!((tcb_bytes > 0) || (tbd_array != 0xffffffff))) { > - logout > - ("illegal values of TBD array address and TCB byte count!\n"); > - } > - // sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes > - uint8_t buf[2600]; > - uint16_t size = 0; > - uint32_t tbd_address = cb_address + 0x10; > - assert(tcb_bytes <= sizeof(buf)); > - while (size < tcb_bytes) { > - uint32_t tx_buffer_address = ldl_phys(tbd_address); > - uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); > - //~ uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); > - tbd_address += 8; > - TRACE(RXTX, logout > - ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n", > - tx_buffer_address, tx_buffer_size)); > - tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); > - cpu_physical_memory_read(tx_buffer_address, &buf[size], > - tx_buffer_size); > - size += tx_buffer_size; > - } > - if (tbd_array == 0xffffffff) { > - /* Simplified mode. Was already handled by code above. */ > - } else { > - /* Flexible mode. */ > - uint8_t tbd_count = 0; > - if (s->has_extended_tcb_support && !(s->configuration[6] & BIT(4))) { > - /* Extended Flexible TCB. */ > - for (; tbd_count < 2; tbd_count++) { > - uint32_t tx_buffer_address = ldl_phys(tbd_address); > - uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); > - uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); > - tbd_address += 8; > - TRACE(RXTX, logout > - ("TBD (extended flexible mode): buffer address 0x%08x, size 0x%04x\n", > - tx_buffer_address, tx_buffer_size)); > - tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); > - cpu_physical_memory_read(tx_buffer_address, &buf[size], > - tx_buffer_size); > - size += tx_buffer_size; > - if (tx_buffer_el & 1) { > - break; > - } > - } > - } > - tbd_address = tbd_array; > - for (; tbd_count < tx.tbd_count; tbd_count++) { > - uint32_t tx_buffer_address = ldl_phys(tbd_address); > - uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); > - uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); > - tbd_address += 8; > - TRACE(RXTX, logout > - ("TBD (flexible mode): buffer address 0x%08x, size 0x%04x\n", > - tx_buffer_address, tx_buffer_size)); > - tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); > - cpu_physical_memory_read(tx_buffer_address, &buf[size], > - tx_buffer_size); > - size += tx_buffer_size; > - if (tx_buffer_el & 1) { > - break; > - } > - } > - } > - TRACE(RXTX, logout("%p sending frame, len=%d,%s\n", s, size, nic_dump(buf, size))); > - qemu_send_packet(s->vc, buf, size); > - s->statistics.tx_good_frames++; > - /* Transmit with bad status would raise an CX/TNO interrupt. > - * (82557 only). Emulation never has bad status. */ > - //~ eepro100_cx_interrupt(s); > + tx_command(s); > break; > case CmdTDR: > TRACE(OTHER, logout("load microcode\n")); > @@ -885,7 +889,7 @@ static void action_command(EEPRO100State *s) > break; > } > /* Write new status. */ > - stw_phys(cb_address, status | 0x8000 | (success ? 0x2000 : 0)); > + stw_phys(s->cb_address, status | 0x8000 | (success ? 0x2000 : 0)); > if (bit_i) { > /* CU completed action. */ > eepro100_cx_interrupt(s); > -- > 1.5.6.5 > >
Michael S. Tsirkin schrieb: > On Thu, Nov 19, 2009 at 09:54:46PM +0100, Stefan Weil wrote: >> Handling of transmit commands is rather complex, >> so about 80 lines of code were moved from function >> action_command to the new function tx_command. >> >> The two new values "tx" and "cb_address" in the >> eepro100 status structure made this possible without >> passing too many parameters. >> >> In addition, the moved code was cleaned a little bit: >> old comments marked with //~ were removed, C++ style >> comments were replaced by C style comments, C++ like >> variable declarations after code were reordered. >> >> Simplified mode is still broken. Nor did I fix >> endianess issues. Both problems will be fixed in >> additional patches (which need this one). >> >> Signed-off-by: Stefan Weil <weil@mail.berlios.de> >> --- >> hw/eepro100.c | 192 >> +++++++++++++++++++++++++++++---------------------------- >> 1 files changed, 98 insertions(+), 94 deletions(-) >> >> diff --git a/hw/eepro100.c b/hw/eepro100.c >> index 4210d8a..7093af8 100644 >> --- a/hw/eepro100.c >> +++ b/hw/eepro100.c >> @@ -213,6 +213,10 @@ typedef struct { >> uint32_t ru_offset; /* RU address offset */ >> uint32_t statsaddr; /* pointer to eepro100_stats_t */ >> >> + /* Temporary data. */ >> + eepro100_tx_t tx; >> + uint32_t cb_address; >> + > > That's pretty evil, as it makes routines non-reentrant. > How bad is it to pass 2 additional arguments around? > If not, maybe define struct tx_command and put necessary stuff there, > then pass pointer to that? Yes, I know that it makes routines non-reentrant, or to be more exact: it makes routines non-reentrant for the same device instance. Different device instances are reentrant because each instance maintains its own status. No, it's not evil. The state machine of the real hardware is not expected to be used in a reentrant way. The same applies to the emulated hardware. Do you expect reentrant calls of transmit or receive functions for one device instance? Regards, Stefan
On Mon, Nov 30, 2009 at 06:05:31PM +0100, Stefan Weil wrote: > Michael S. Tsirkin schrieb: > > On Thu, Nov 19, 2009 at 09:54:46PM +0100, Stefan Weil wrote: > >> Handling of transmit commands is rather complex, > >> so about 80 lines of code were moved from function > >> action_command to the new function tx_command. > >> > >> The two new values "tx" and "cb_address" in the > >> eepro100 status structure made this possible without > >> passing too many parameters. > >> > >> In addition, the moved code was cleaned a little bit: > >> old comments marked with //~ were removed, C++ style > >> comments were replaced by C style comments, C++ like > >> variable declarations after code were reordered. > >> > >> Simplified mode is still broken. Nor did I fix > >> endianess issues. Both problems will be fixed in > >> additional patches (which need this one). > >> > >> Signed-off-by: Stefan Weil <weil@mail.berlios.de> > >> --- > >> hw/eepro100.c | 192 > >> +++++++++++++++++++++++++++++---------------------------- > >> 1 files changed, 98 insertions(+), 94 deletions(-) > >> > >> diff --git a/hw/eepro100.c b/hw/eepro100.c > >> index 4210d8a..7093af8 100644 > >> --- a/hw/eepro100.c > >> +++ b/hw/eepro100.c > >> @@ -213,6 +213,10 @@ typedef struct { > >> uint32_t ru_offset; /* RU address offset */ > >> uint32_t statsaddr; /* pointer to eepro100_stats_t */ > >> > >> + /* Temporary data. */ > >> + eepro100_tx_t tx; > >> + uint32_t cb_address; > >> + > > > > That's pretty evil, as it makes routines non-reentrant. > > How bad is it to pass 2 additional arguments around? > > If not, maybe define struct tx_command and put necessary stuff there, > > then pass pointer to that? > > Yes, I know that it makes routines non-reentrant, or > to be more exact: it makes routines non-reentrant > for the same device instance. Different device instances > are reentrant because each instance maintains its > own status. > > No, it's not evil. "Temporary data" comment is very evil. We not only don't want to document code, we document this fact. > The state machine of the real hardware > is not expected to be used in a reentrant way. The same > applies to the emulated hardware. That code does not appear currently structured as a state machine. > Do you expect reentrant calls of transmit or receive > functions for one device instance? > > Regards, > Stefan Datastructures should make sense. This one does not seem to make sense. What's the benefit here? Why is it good to pass less parameters to functions?
Stefan Weil schrieb: > Handling of transmit commands is rather complex, > so about 80 lines of code were moved from function > action_command to the new function tx_command. > > The two new values "tx" and "cb_address" in the > eepro100 status structure made this possible without > passing too many parameters. > > In addition, the moved code was cleaned a little bit: > old comments marked with //~ were removed, C++ style > comments were replaced by C style comments, C++ like > variable declarations after code were reordered. > > Simplified mode is still broken. Nor did I fix > endianess issues. Both problems will be fixed in > additional patches (which need this one). > > Signed-off-by: Stefan Weil <weil@mail.berlios.de> > --- > hw/eepro100.c | 192 > +++++++++++++++++++++++++++++---------------------------- > 1 files changed, 98 insertions(+), 94 deletions(-) This patch is still missing in QEMU master. Because of other patches (which were sent later!), a rebase is needed again (it was rebased 3 times now). Only the line with qemu_send_packet changed. Please apply the rebased patch if there are no really important reasons against it (there are none), hopefully without waiting several weeks again. It is needed for very important bug fixes (multicast, simple transmit mode). Regards Stefan Weil
Stefan Weil wrote: > Stefan Weil schrieb: > >> Handling of transmit commands is rather complex, >> so about 80 lines of code were moved from function >> action_command to the new function tx_command. >> >> The two new values "tx" and "cb_address" in the >> eepro100 status structure made this possible without >> passing too many parameters. >> >> In addition, the moved code was cleaned a little bit: >> old comments marked with //~ were removed, C++ style >> comments were replaced by C style comments, C++ like >> variable declarations after code were reordered. >> >> Simplified mode is still broken. Nor did I fix >> endianess issues. Both problems will be fixed in >> additional patches (which need this one). >> >> Signed-off-by: Stefan Weil <weil@mail.berlios.de> >> --- >> hw/eepro100.c | 192 >> +++++++++++++++++++++++++++++---------------------------- >> 1 files changed, 98 insertions(+), 94 deletions(-) >> > > This patch is still missing in QEMU master. > Because of other patches (which were sent later!), a rebase is needed again > (it was rebased 3 times now). Only the line with qemu_send_packet changed. > > Please apply the rebased patch if there are no really important > reasons against it (there are none), hopefully without waiting several > weeks again. > It is needed for very important bug fixes (multicast, simple transmit mode). > Michael had given the patch some feedback that I think is valuable. I haven't seen a response to his feedback nor have I seen a resubmit incorporating the feedback. Storing intermediate state of a function as global state (instead of on the stack) is odd. Even if there's a great justification (I don't think there is), such a thing is sufficiently unusual that it deserves a better comment. BTW, part of the trouble your having is you're sending one patch that does too many things. If you submitted a series that contained code cleanups, and then code motion, the first patches would have been no brainers to commit while the code motion would have been treated separately making rebasing less painful. Regards, Anthony Liguori
Michael S. Tsirkin schrieb: > On Mon, Nov 30, 2009 at 06:05:31PM +0100, Stefan Weil wrote: >> Michael S. Tsirkin schrieb: >>> On Thu, Nov 19, 2009 at 09:54:46PM +0100, Stefan Weil wrote: >>>> Handling of transmit commands is rather complex, >>>> so about 80 lines of code were moved from function >>>> action_command to the new function tx_command. >>>> >>>> The two new values "tx" and "cb_address" in the >>>> eepro100 status structure made this possible without >>>> passing too many parameters. >>>> >>>> In addition, the moved code was cleaned a little bit: >>>> old comments marked with //~ were removed, C++ style >>>> comments were replaced by C style comments, C++ like >>>> variable declarations after code were reordered. >>>> >>>> Simplified mode is still broken. Nor did I fix >>>> endianess issues. Both problems will be fixed in >>>> additional patches (which need this one). >>>> >>>> Signed-off-by: Stefan Weil <weil@mail.berlios.de> >>>> --- >>>> hw/eepro100.c | 192 >>>> +++++++++++++++++++++++++++++---------------------------- >>>> 1 files changed, 98 insertions(+), 94 deletions(-) >>>> >>>> diff --git a/hw/eepro100.c b/hw/eepro100.c >>>> index 4210d8a..7093af8 100644 >>>> --- a/hw/eepro100.c >>>> +++ b/hw/eepro100.c >>>> @@ -213,6 +213,10 @@ typedef struct { >>>> uint32_t ru_offset; /* RU address offset */ >>>> uint32_t statsaddr; /* pointer to eepro100_stats_t */ >>>> >>>> + /* Temporary data. */ >>>> + eepro100_tx_t tx; >>>> + uint32_t cb_address; >>>> + >>> That's pretty evil, as it makes routines non-reentrant. >>> How bad is it to pass 2 additional arguments around? >>> If not, maybe define struct tx_command and put necessary stuff there, >>> then pass pointer to that? >> Yes, I know that it makes routines non-reentrant, or >> to be more exact: it makes routines non-reentrant >> for the same device instance. Different device instances >> are reentrant because each instance maintains its >> own status. >> >> No, it's not evil. > > "Temporary data" comment is very evil. > We not only don't want to document code, > we document this fact. > >> The state machine of the real hardware >> is not expected to be used in a reentrant way. The same >> applies to the emulated hardware. > > That code does not appear currently structured as a state machine. > >> Do you expect reentrant calls of transmit or receive >> functions for one device instance? >> >> Regards, >> Stefan > > Datastructures should make sense. This one does not seem to make sense. > What's the benefit here? Why is it good to pass less > parameters to functions? Hello Michael, I don't think that "very evil" is a good way to describe code someone has written. But maybe there is a misunderstanding caused because you and I are not native english speakers. Antony, perhaps a better comment for the two temporary values would be "Temporary data used to pass status information between functions, don't save it." Feel free to change this comment. Michael, maybe you have a better suggestion? Both values will be used in more functions with patches to come, sometimes also as output parameter. So adding a new status structure for temporary status values or even passing simple data types would make the code less comprehensible. Kind regards, Stefan
On Sat, Dec 12, 2009 at 08:45:06PM +0100, Stefan Weil wrote: > Michael S. Tsirkin schrieb: > > On Mon, Nov 30, 2009 at 06:05:31PM +0100, Stefan Weil wrote: > >> Michael S. Tsirkin schrieb: > >>> On Thu, Nov 19, 2009 at 09:54:46PM +0100, Stefan Weil wrote: > >>>> Handling of transmit commands is rather complex, > >>>> so about 80 lines of code were moved from function > >>>> action_command to the new function tx_command. > >>>> > >>>> The two new values "tx" and "cb_address" in the > >>>> eepro100 status structure made this possible without > >>>> passing too many parameters. > >>>> > >>>> In addition, the moved code was cleaned a little bit: > >>>> old comments marked with //~ were removed, C++ style > >>>> comments were replaced by C style comments, C++ like > >>>> variable declarations after code were reordered. > >>>> > >>>> Simplified mode is still broken. Nor did I fix > >>>> endianess issues. Both problems will be fixed in > >>>> additional patches (which need this one). > >>>> > >>>> Signed-off-by: Stefan Weil <weil@mail.berlios.de> > >>>> --- > >>>> hw/eepro100.c | 192 > >>>> +++++++++++++++++++++++++++++---------------------------- > >>>> 1 files changed, 98 insertions(+), 94 deletions(-) > >>>> > >>>> diff --git a/hw/eepro100.c b/hw/eepro100.c > >>>> index 4210d8a..7093af8 100644 > >>>> --- a/hw/eepro100.c > >>>> +++ b/hw/eepro100.c > >>>> @@ -213,6 +213,10 @@ typedef struct { > >>>> uint32_t ru_offset; /* RU address offset */ > >>>> uint32_t statsaddr; /* pointer to eepro100_stats_t */ > >>>> > >>>> + /* Temporary data. */ > >>>> + eepro100_tx_t tx; > >>>> + uint32_t cb_address; > >>>> + > >>> That's pretty evil, as it makes routines non-reentrant. > >>> How bad is it to pass 2 additional arguments around? > >>> If not, maybe define struct tx_command and put necessary stuff there, > >>> then pass pointer to that? > >> Yes, I know that it makes routines non-reentrant, or > >> to be more exact: it makes routines non-reentrant > >> for the same device instance. Different device instances > >> are reentrant because each instance maintains its > >> own status. > >> > >> No, it's not evil. > > > > "Temporary data" comment is very evil. > > We not only don't want to document code, > > we document this fact. > > > >> The state machine of the real hardware > >> is not expected to be used in a reentrant way. The same > >> applies to the emulated hardware. > > > > That code does not appear currently structured as a state machine. > > > >> Do you expect reentrant calls of transmit or receive > >> functions for one device instance? > >> > >> Regards, > >> Stefan > > > > Datastructures should make sense. This one does not seem to make sense. > > What's the benefit here? Why is it good to pass less > > parameters to functions? > > Hello Michael, > > I don't think that "very evil" is a good way to describe code someone > has written. > But maybe there is a misunderstanding caused because you and I are not > native > english speakers. > > Antony, perhaps a better comment for the two temporary values would be > "Temporary data used to pass status information between functions, don't > save it." This does not address the basic problems that 1. just figuring out what data should a field contain requires reading code that initializes it 2. the code becomes non-reentrant and more fragile. > Feel free to change this comment. Michael, maybe you have a better > suggestion? Yes. Add a couple of extra parameters to the functions that you call, with names specifying what do the values contain. > Both values will be used in more functions with patches to come, > sometimes also as output parameter. So adding a new status structure for > temporary status values or even passing simple data types > would make the code less comprehensible. > > Kind regards, > Stefan I haven't seen that code so I can not judge. On the surface, it sounds like using parameters will be better. It is a *good idea* to pass input parameters as int X and output parameters as int *X, rather than sticking them in some random structure just because same function happens to already get a pointer to it.
Michael S. Tsirkin schrieb: > On Sat, Dec 12, 2009 at 08:45:06PM +0100, Stefan Weil wrote: >> Michael S. Tsirkin schrieb: >>> On Mon, Nov 30, 2009 at 06:05:31PM +0100, Stefan Weil wrote: >>>> Michael S. Tsirkin schrieb: >>>>> On Thu, Nov 19, 2009 at 09:54:46PM +0100, Stefan Weil wrote: >>>>>> Handling of transmit commands is rather complex, >>>>>> so about 80 lines of code were moved from function >>>>>> action_command to the new function tx_command. >>>>>> >>>>>> The two new values "tx" and "cb_address" in the >>>>>> eepro100 status structure made this possible without >>>>>> passing too many parameters. >>>>>> >>>>>> In addition, the moved code was cleaned a little bit: >>>>>> old comments marked with //~ were removed, C++ style >>>>>> comments were replaced by C style comments, C++ like >>>>>> variable declarations after code were reordered. >>>>>> >>>>>> Simplified mode is still broken. Nor did I fix >>>>>> endianess issues. Both problems will be fixed in >>>>>> additional patches (which need this one). >>>>>> >>>>>> Signed-off-by: Stefan Weil <weil@mail.berlios.de> >>>>>> --- >>>>>> hw/eepro100.c | 192 >>>>>> +++++++++++++++++++++++++++++---------------------------- >>>>>> 1 files changed, 98 insertions(+), 94 deletions(-) >>>>>> >>>>>> diff --git a/hw/eepro100.c b/hw/eepro100.c >>>>>> index 4210d8a..7093af8 100644 >>>>>> --- a/hw/eepro100.c >>>>>> +++ b/hw/eepro100.c >>>>>> @@ -213,6 +213,10 @@ typedef struct { >>>>>> uint32_t ru_offset; /* RU address offset */ >>>>>> uint32_t statsaddr; /* pointer to eepro100_stats_t */ >>>>>> >>>>>> + /* Temporary data. */ >>>>>> + eepro100_tx_t tx; >>>>>> + uint32_t cb_address; >>>>>> + >>>>> That's pretty evil, as it makes routines non-reentrant. >>>>> How bad is it to pass 2 additional arguments around? >>>>> If not, maybe define struct tx_command and put necessary stuff there, >>>>> then pass pointer to that? >>>> Yes, I know that it makes routines non-reentrant, or >>>> to be more exact: it makes routines non-reentrant >>>> for the same device instance. Different device instances >>>> are reentrant because each instance maintains its >>>> own status. >>>> >>>> No, it's not evil. >>> "Temporary data" comment is very evil. >>> We not only don't want to document code, >>> we document this fact. >>> >>>> The state machine of the real hardware >>>> is not expected to be used in a reentrant way. The same >>>> applies to the emulated hardware. >>> That code does not appear currently structured as a state machine. >>> >>>> Do you expect reentrant calls of transmit or receive >>>> functions for one device instance? >>>> >>>> Regards, >>>> Stefan >>> Datastructures should make sense. This one does not seem to make sense. >>> What's the benefit here? Why is it good to pass less >>> parameters to functions? >> Hello Michael, >> >> I don't think that "very evil" is a good way to describe code someone >> has written. >> But maybe there is a misunderstanding caused because you and I are not >> native >> english speakers. >> >> Antony, perhaps a better comment for the two temporary values would be >> "Temporary data used to pass status information between functions, don't >> save it." > > This does not address the basic problems that 1. just figuring out > what data should a field contain requires reading code that > initializes it 2. the code becomes non-reentrant and more fragile. 1. Yes, of course you have to read code to understand it. I don't see your point here - maybe a problem of the language. 2. Reentrancy is not a problem here (I explained that already). "More fragile" is a subjective feedback, my feeling is different. > >> Feel free to change this comment. Michael, maybe you have a better >> suggestion? > > Yes. Add a couple of extra parameters to the functions that > you call, with names specifying what do the values contain. There are many ways to do things in programming. I decided to do it the way it is, and you prefer a different way. That's ok, but as long as there are no better arguments, let me do it my way (up to now, eepro100.c was written and maintained by me). > >> Both values will be used in more functions with patches to come, >> sometimes also as output parameter. So adding a new status structure for >> temporary status values or even passing simple data types >> would make the code less comprehensible. >> >> Kind regards, >> Stefan > > I haven't seen that code so I can not judge. Here is the URL: http://repo.or.cz/w/qemu/ar7.git/blob/HEAD:/hw/eepro100.c > > On the surface, it sounds like using parameters will be better. > It is a *good idea* to pass input parameters as int X and > output parameters as int *X, rather than sticking them in some random > structure just because same function happens to already get a pointer > to it. If it were some random structure, you would be right. I repeat myself: it is not some random structure but the device status. Both values are device status values.
On Sun, Dec 13, 2009 at 09:59:57PM +0100, Stefan Weil wrote: > Michael S. Tsirkin schrieb: > > On Sat, Dec 12, 2009 at 08:45:06PM +0100, Stefan Weil wrote: > >> Michael S. Tsirkin schrieb: > >>> On Mon, Nov 30, 2009 at 06:05:31PM +0100, Stefan Weil wrote: > >>>> Michael S. Tsirkin schrieb: > >>>>> On Thu, Nov 19, 2009 at 09:54:46PM +0100, Stefan Weil wrote: > >>>>>> Handling of transmit commands is rather complex, > >>>>>> so about 80 lines of code were moved from function > >>>>>> action_command to the new function tx_command. > >>>>>> > >>>>>> The two new values "tx" and "cb_address" in the > >>>>>> eepro100 status structure made this possible without > >>>>>> passing too many parameters. > >>>>>> > >>>>>> In addition, the moved code was cleaned a little bit: > >>>>>> old comments marked with //~ were removed, C++ style > >>>>>> comments were replaced by C style comments, C++ like > >>>>>> variable declarations after code were reordered. > >>>>>> > >>>>>> Simplified mode is still broken. Nor did I fix > >>>>>> endianess issues. Both problems will be fixed in > >>>>>> additional patches (which need this one). > >>>>>> > >>>>>> Signed-off-by: Stefan Weil <weil@mail.berlios.de> > >>>>>> --- > >>>>>> hw/eepro100.c | 192 > >>>>>> +++++++++++++++++++++++++++++---------------------------- > >>>>>> 1 files changed, 98 insertions(+), 94 deletions(-) > >>>>>> > >>>>>> diff --git a/hw/eepro100.c b/hw/eepro100.c > >>>>>> index 4210d8a..7093af8 100644 > >>>>>> --- a/hw/eepro100.c > >>>>>> +++ b/hw/eepro100.c > >>>>>> @@ -213,6 +213,10 @@ typedef struct { > >>>>>> uint32_t ru_offset; /* RU address offset */ > >>>>>> uint32_t statsaddr; /* pointer to eepro100_stats_t */ > >>>>>> > >>>>>> + /* Temporary data. */ > >>>>>> + eepro100_tx_t tx; > >>>>>> + uint32_t cb_address; > >>>>>> + > >>>>> That's pretty evil, as it makes routines non-reentrant. > >>>>> How bad is it to pass 2 additional arguments around? > >>>>> If not, maybe define struct tx_command and put necessary stuff there, > >>>>> then pass pointer to that? > >>>> Yes, I know that it makes routines non-reentrant, or > >>>> to be more exact: it makes routines non-reentrant > >>>> for the same device instance. Different device instances > >>>> are reentrant because each instance maintains its > >>>> own status. > >>>> > >>>> No, it's not evil. > >>> "Temporary data" comment is very evil. > >>> We not only don't want to document code, > >>> we document this fact. > >>> > >>>> The state machine of the real hardware > >>>> is not expected to be used in a reentrant way. The same > >>>> applies to the emulated hardware. > >>> That code does not appear currently structured as a state machine. > >>> > >>>> Do you expect reentrant calls of transmit or receive > >>>> functions for one device instance? > >>>> > >>>> Regards, > >>>> Stefan > >>> Datastructures should make sense. This one does not seem to make sense. > >>> What's the benefit here? Why is it good to pass less > >>> parameters to functions? > >> Hello Michael, > >> > >> I don't think that "very evil" is a good way to describe code someone > >> has written. > >> But maybe there is a misunderstanding caused because you and I are not > >> native > >> english speakers. > >> > >> Antony, perhaps a better comment for the two temporary values would be > >> "Temporary data used to pass status information between functions, don't > >> save it." > > > > This does not address the basic problems that 1. just figuring out > > what data should a field contain requires reading code that > > initializes it 2. the code becomes non-reentrant and more fragile. > > 1. Yes, of course you have to read code to understand it. > I don't see your point here - maybe a problem of the language. > 2. Reentrancy is not a problem here (I explained that already). > "More fragile" is a subjective feedback, my feeling is different. > > > > >> Feel free to change this comment. Michael, maybe you have a better > >> suggestion? > > > > Yes. Add a couple of extra parameters to the functions that > > you call, with names specifying what do the values contain. > > There are many ways to do things in programming. > I decided to do it the way it is, and you prefer a different way. > That's ok, but as long as there are no better arguments, > let me do it my way (up to now, eepro100.c was written > and maintained by me). > > >> Both values will be used in more functions with patches to come, > >> sometimes also as output parameter. So adding a new status structure for > >> temporary status values or even passing simple data types > >> would make the code less comprehensible. > >> > >> Kind regards, > >> Stefan > > > > I haven't seen that code so I can not judge. > > Here is the URL: > http://repo.or.cz/w/qemu/ar7.git/blob/HEAD:/hw/eepro100.c > > > > On the surface, it sounds like using parameters will be better. > > It is a *good idea* to pass input parameters as int X and > > output parameters as int *X, rather than sticking them in some random > > structure just because same function happens to already get a pointer > > to it. > > If it were some random structure, you would be right. > I repeat myself: it is not some random structure but the > device status. Both values are device status values. Well, can you please try to document them harder than "Temporary values"? E.g. write what they represent and when are they valid.
diff --git a/hw/eepro100.c b/hw/eepro100.c index 4210d8a..7093af8 100644 --- a/hw/eepro100.c +++ b/hw/eepro100.c @@ -213,6 +213,10 @@ typedef struct { uint32_t ru_offset; /* RU address offset */ uint32_t statsaddr; /* pointer to eepro100_stats_t */ + /* Temporary data. */ + eepro100_tx_t tx; + uint32_t cb_address; + /* Statistical counters. Also used for wake-up packet (i82559). */ eepro100_stats_t statistics; @@ -748,17 +752,100 @@ static void dump_statistics(EEPRO100State * s) //~ missing("CU dump statistical counters"); } +static void tx_command(EEPRO100State *s) +{ + uint32_t tbd_array = le32_to_cpu(s->tx.tx_desc_addr); + uint16_t tcb_bytes = (le16_to_cpu(s->tx.tcb_bytes) & 0x3fff); + /* Sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes. */ + uint8_t buf[2600]; + uint16_t size = 0; + uint32_t tbd_address = s->cb_address + 0x10; + TRACE(RXTX, logout + ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count %u\n", + tbd_array, tcb_bytes, s->tx.tbd_count)); + + if (tcb_bytes > 2600) { + logout("TCB byte count too large, using 2600\n"); + tcb_bytes = 2600; + } + if (!((tcb_bytes > 0) || (tbd_array != 0xffffffff))) { + logout + ("illegal values of TBD array address and TCB byte count!\n"); + } + assert(tcb_bytes <= sizeof(buf)); + while (size < tcb_bytes) { + uint32_t tx_buffer_address = ldl_phys(tbd_address); + uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); + //~ uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); + tbd_address += 8; + TRACE(RXTX, logout + ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n", + tx_buffer_address, tx_buffer_size)); + tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); + cpu_physical_memory_read(tx_buffer_address, &buf[size], + tx_buffer_size); + size += tx_buffer_size; + } + if (tbd_array == 0xffffffff) { + /* Simplified mode. Was already handled by code above. */ + } else { + /* Flexible mode. */ + uint8_t tbd_count = 0; + if (s->has_extended_tcb_support && !(s->configuration[6] & BIT(4))) { + /* Extended Flexible TCB. */ + for (; tbd_count < 2; tbd_count++) { + uint32_t tx_buffer_address = ldl_phys(tbd_address); + uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); + uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); + tbd_address += 8; + TRACE(RXTX, logout + ("TBD (extended flexible mode): buffer address 0x%08x, size 0x%04x\n", + tx_buffer_address, tx_buffer_size)); + tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); + cpu_physical_memory_read(tx_buffer_address, &buf[size], + tx_buffer_size); + size += tx_buffer_size; + if (tx_buffer_el & 1) { + break; + } + } + } + tbd_address = tbd_array; + for (; tbd_count < s->tx.tbd_count; tbd_count++) { + uint32_t tx_buffer_address = ldl_phys(tbd_address); + uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); + uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); + tbd_address += 8; + TRACE(RXTX, logout + ("TBD (flexible mode): buffer address 0x%08x, size 0x%04x\n", + tx_buffer_address, tx_buffer_size)); + tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); + cpu_physical_memory_read(tx_buffer_address, &buf[size], + tx_buffer_size); + size += tx_buffer_size; + if (tx_buffer_el & 1) { + break; + } + } + } + TRACE(RXTX, logout("%p sending frame, len=%d,%s\n", s, size, nic_dump(buf, size))); + qemu_send_packet(s->vc, buf, size); + s->statistics.tx_good_frames++; + /* Transmit with bad status would raise an CX/TNO interrupt. + * (82557 only). Emulation never has bad status. */ + //~ eepro100_cx_interrupt(s); +} + static void action_command(EEPRO100State *s) { for (;;) { - uint32_t cb_address = s->cu_base + s->cu_offset; - eepro100_tx_t tx; - cpu_physical_memory_read(cb_address, (uint8_t *) & tx, sizeof(tx)); - uint16_t status = le16_to_cpu(tx.status); - uint16_t command = le16_to_cpu(tx.command); + s->cb_address = s->cu_base + s->cu_offset; + cpu_physical_memory_read(s->cb_address, (uint8_t *)&s->tx, sizeof(s->tx)); + uint16_t status = le16_to_cpu(s->tx.status); + uint16_t command = le16_to_cpu(s->tx.command); logout ("val=0x%02x (cu start), status=0x%04x, command=0x%04x, link=0x%08x\n", - val, status, command, tx.link); + val, status, command, s->tx.link); bool bit_el = ((command & 0x8000) != 0); bool bit_s = ((command & 0x4000) != 0); bool bit_i = ((command & 0x2000) != 0); @@ -766,17 +853,17 @@ static void action_command(EEPRO100State *s) bool success = true; //~ bool bit_sf = ((command & 0x0008) != 0); uint16_t cmd = command & 0x0007; - s->cu_offset = le32_to_cpu(tx.link); + s->cu_offset = le32_to_cpu(s->tx.link); switch (cmd) { case CmdNOp: /* Do nothing. */ break; case CmdIASetup: - cpu_physical_memory_read(cb_address + 8, &s->conf.macaddr.a[0], 6); + cpu_physical_memory_read(s->cb_address + 8, &s->conf.macaddr.a[0], 6); TRACE(OTHER, logout("macaddr: %s\n", nic_dump(&s->macaddr[0], 6))); break; case CmdConfigure: - cpu_physical_memory_read(cb_address + 8, &s->configuration[0], + cpu_physical_memory_read(s->cb_address + 8, &s->configuration[0], sizeof(s->configuration)); TRACE(OTHER, logout("configuration: %s\n", nic_dump(&s->configuration[0], 16))); break; @@ -784,95 +871,12 @@ static void action_command(EEPRO100State *s) //~ missing("multicast list"); break; case CmdTx: - (void)0; - uint32_t tbd_array = le32_to_cpu(tx.tx_desc_addr); - uint16_t tcb_bytes = (le16_to_cpu(tx.tcb_bytes) & 0x3fff); - TRACE(RXTX, logout - ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count %u\n", - tbd_array, tcb_bytes, tx.tbd_count)); - if (bit_nc) { missing("CmdTx: NC = 0"); success = false; break; } - //~ assert(!bit_sf); - if (tcb_bytes > 2600) { - logout("TCB byte count too large, using 2600\n"); - tcb_bytes = 2600; - } - /* Next assertion fails for local configuration. */ - //~ assert((tcb_bytes > 0) || (tbd_array != 0xffffffff)); - if (!((tcb_bytes > 0) || (tbd_array != 0xffffffff))) { - logout - ("illegal values of TBD array address and TCB byte count!\n"); - } - // sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes - uint8_t buf[2600]; - uint16_t size = 0; - uint32_t tbd_address = cb_address + 0x10; - assert(tcb_bytes <= sizeof(buf)); - while (size < tcb_bytes) { - uint32_t tx_buffer_address = ldl_phys(tbd_address); - uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); - //~ uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); - tbd_address += 8; - TRACE(RXTX, logout - ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n", - tx_buffer_address, tx_buffer_size)); - tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); - cpu_physical_memory_read(tx_buffer_address, &buf[size], - tx_buffer_size); - size += tx_buffer_size; - } - if (tbd_array == 0xffffffff) { - /* Simplified mode. Was already handled by code above. */ - } else { - /* Flexible mode. */ - uint8_t tbd_count = 0; - if (s->has_extended_tcb_support && !(s->configuration[6] & BIT(4))) { - /* Extended Flexible TCB. */ - for (; tbd_count < 2; tbd_count++) { - uint32_t tx_buffer_address = ldl_phys(tbd_address); - uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); - uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); - tbd_address += 8; - TRACE(RXTX, logout - ("TBD (extended flexible mode): buffer address 0x%08x, size 0x%04x\n", - tx_buffer_address, tx_buffer_size)); - tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); - cpu_physical_memory_read(tx_buffer_address, &buf[size], - tx_buffer_size); - size += tx_buffer_size; - if (tx_buffer_el & 1) { - break; - } - } - } - tbd_address = tbd_array; - for (; tbd_count < tx.tbd_count; tbd_count++) { - uint32_t tx_buffer_address = ldl_phys(tbd_address); - uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); - uint16_t tx_buffer_el = lduw_phys(tbd_address + 6); - tbd_address += 8; - TRACE(RXTX, logout - ("TBD (flexible mode): buffer address 0x%08x, size 0x%04x\n", - tx_buffer_address, tx_buffer_size)); - tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size); - cpu_physical_memory_read(tx_buffer_address, &buf[size], - tx_buffer_size); - size += tx_buffer_size; - if (tx_buffer_el & 1) { - break; - } - } - } - TRACE(RXTX, logout("%p sending frame, len=%d,%s\n", s, size, nic_dump(buf, size))); - qemu_send_packet(s->vc, buf, size); - s->statistics.tx_good_frames++; - /* Transmit with bad status would raise an CX/TNO interrupt. - * (82557 only). Emulation never has bad status. */ - //~ eepro100_cx_interrupt(s); + tx_command(s); break; case CmdTDR: TRACE(OTHER, logout("load microcode\n")); @@ -885,7 +889,7 @@ static void action_command(EEPRO100State *s) break; } /* Write new status. */ - stw_phys(cb_address, status | 0x8000 | (success ? 0x2000 : 0)); + stw_phys(s->cb_address, status | 0x8000 | (success ? 0x2000 : 0)); if (bit_i) { /* CU completed action. */ eepro100_cx_interrupt(s);
Handling of transmit commands is rather complex, so about 80 lines of code were moved from function action_command to the new function tx_command. The two new values "tx" and "cb_address" in the eepro100 status structure made this possible without passing too many parameters. In addition, the moved code was cleaned a little bit: old comments marked with //~ were removed, C++ style comments were replaced by C style comments, C++ like variable declarations after code were reordered. Simplified mode is still broken. Nor did I fix endianess issues. Both problems will be fixed in additional patches (which need this one). Signed-off-by: Stefan Weil <weil@mail.berlios.de> --- hw/eepro100.c | 192 +++++++++++++++++++++++++++++---------------------------- 1 files changed, 98 insertions(+), 94 deletions(-)