[Qemu-devel] Re: [PATCH 8/9] eepro100: Pad received short frames

[Stefan Weil <weil@mail.berlios.de>]

Am 31.03.2011 23:41, schrieb Michael S. Tsirkin:
> On Thu, Mar 31, 2011 at 10:33:30PM +0200, Stefan Weil wrote:
>> QEMU sends frames smaller than 60 bytes to ethernet nics.
>> This should be fixed in the networking code because normally
>> such frames are rejected by real NICs and their emulations.
>> To avoid this behaviour, other NIC emulations pad received
>> frames. This patch enables this workaround for eepro100, too.
>>
>> All related code is marked with CONFIG_PAD_RECEIVED_FRAMES,
>> so emulation of the correct handling for short frames can
>> be restored as soon as QEMU's networking code is fixed.
>>
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>
> qemu networking core isn't ethernet-specific
> (slirp is). That's why we don't do this in core,
> so the uglification isn't worth it.

Technically it is possible to add the padding needed for ethernet frames
in slirp code (a patch was already sent to qemu-devel) and any other
code location. Ethernet devices could also set a flag about their padding
requirements, so unnecessary padding could be avoided for non-ethernet
devices like virtio.

The last time when padding was discussed here, not everybody was happy
with the current solution: it was simply the solution which required
the least efforts.

I don't think that the current solution will last forever, simply
because it does not allow testing systems which do create short frames
because they want to test the reaction on short frames or for any
other reason.

That's why I want to be able to disable the padding in the device
emulation.


>
>> ---
>> hw/eepro100.c | 25 ++++++++++++++++++++++++-
>> 1 files changed, 24 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index 500a3af..a740d2e 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -47,6 +47,14 @@
>> #include "eeprom93xx.h"
>> #include "sysemu.h"
>>
>> +/* QEMU sends frames smaller than 60 bytes to ethernet nics.
>> + * This should be fixed in the networking code because normally
>> + * such frames are rejected by real nics and their emulations.
>> + * To avoid this behaviour, other nic emulations pad received
>> + * frames. The following definition enables this workaround for
>> + * eepro100, too. */
>> +#define CONFIG_PAD_RECEIVED_FRAMES
>> +
>> #define KiB 1024
>>
>> /* Debug EEPRO100 card. */
>> @@ -1756,19 +1764,32 @@ static ssize_t nic_receive(VLANClientState 
>> *nc, const uint8_t * buf, size_t size
>> */
>> EEPRO100State *s = DO_UPCAST(NICState, nc, nc)->opaque;
>> uint16_t rfd_status = 0xa000;
>> +#if defined(CONFIG_PAD_RECEIVED_FRAMES)
>> + uint8_t min_buf[60];
>> +#endif
>> static const uint8_t broadcast_macaddr[6] =
>> { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>>
>> +#if defined(CONFIG_PAD_RECEIVED_FRAMES)
>> + /* Pad to minimum Ethernet frame length */
>> + if (size < sizeof(min_buf)) {
>> + memcpy(min_buf, buf, size);
>> + memset(&min_buf[size], 0, sizeof(min_buf) - size);
>> + buf = min_buf;
>> + size = sizeof(min_buf);
>> + }
>> +#endif
>> +
>> if (s->configuration[8] & 0x80) {
>> /* CSMA is disabled. */
>> logout("%p received while CSMA is disabled\n", s);
>> return -1;
>> +#if !defined(CONFIG_PAD_RECEIVED_FRAMES)
>> } else if (size < 64 && (s->configuration[7] & BIT(0))) {
>> /* Short frame and configuration byte 7/0 (discard short receive) set:
>> * Short frame is discarded */
>> logout("%p received short frame (%zu byte)\n", s, size);
>> s->statistics.rx_short_frame_errors++;
>> -#if 0
>> return -1;
>> #endif
>> } else if ((size > MAX_ETH_FRAME_SIZE + 4) && !(s->configuration[18] 
>> & BIT(3))) {
>> @@ -1847,9 +1868,11 @@ static ssize_t nic_receive(VLANClientState 
>> *nc, const uint8_t * buf, size_t size
>> "(%zu bytes); data truncated\n", rfd_size, size);
>> size = rfd_size;
>> }
>> +#if !defined(CONFIG_PAD_RECEIVED_FRAMES)
>> if (size < 64) {
>> rfd_status |= 0x0080;
>> }
>> +#endif
>> TRACE(OTHER, logout("command 0x%04x, link 0x%08x, addr 0x%08x, size 
>> %u\n",
>> rfd_command, rx.link, rx.rx_buf_addr, rfd_size));
>> stw_le_phys(s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, status),
>> -- 
>> 1.7.2.5
>