[Qemu-devel] eepro100.c nic_receive() asserts

["Bjørn Mork" <bjorn@mork.no>]

Hello,

I've been running FreeBSD-based guests for a while using a slightly
patched version of the eepro100 driver (model=i82559er) taken from
http://svn.berlios.de/viewcvs/ar7-firmware/qemu/trunk/hw/eepro100.c?rev=985
The reason for using the patched version is a need for working multicast
on emulated 82559ER NICs.

Guests based on FreeBSD 4.10 have been pretty stable, except for always
crashing on reboot.  Guests based on FreeBSD 6.1 have however been less
stable.

Yesterday I updated to the current revision from the ar7-firmware repository
(http://svn.berlios.de/viewcvs/ar7-firmware/qemu/trunk/hw/eepro100.c?rev=1099 )
and at the same time did some research into the crashes.  Both seem to
come from nic_receive(), and the code in question has not been modfied
by the patch.  The same problems are there both in qemu 0.9.1 (and hence
kvm-65), and in the current ar7-firmware.  I would have liked to just
provide a patch, but I hesitate to do that as I really don't understand
what the code I'm qeustioning is supposed to do.  So I'm really hoping
someone more competent could look into it and suggest a proper fix.

The last part of nic_receive() (from QEMU 0.9.1/kvm-65) looks like this:


    eepro100_rx_t rx;
    cpu_physical_memory_read(s->ru_base + s->ru_offset, (uint8_t *) & rx,
                             offsetof(eepro100_rx_t, packet));
    uint16_t rfd_command = le16_to_cpu(rx.command);
    uint16_t rfd_size = le16_to_cpu(rx.size);
    assert(size <= rfd_size);
    if (size < 64) {
        rfd_status |= 0x0080;
    }
    logout("command 0x%04x, link 0x%08x, addr 0x%08x, size %u\n", rfd_command,
           rx.link, rx.rx_buf_addr, rfd_size);
    stw_phys(s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, status),
             rfd_status);
    stw_phys(s->ru_base + s->ru_offset + offsetof(eepro100_rx_t, count), size);
    /* Early receive interrupt not supported. */
    //~ eepro100_er_interrupt(s);
    /* Receive CRC Transfer not supported. */
    assert(!(s->configuration[18] & 4));
    /* TODO: check stripping enable bit. */
    //~ assert(!(s->configuration[17] & 1));
    cpu_physical_memory_write(s->ru_base + s->ru_offset +
                              offsetof(eepro100_rx_t, packet), buf, size);
    s->statistics.rx_good_frames++;
    eepro100_fr_interrupt(s);
    s->ru_offset = le32_to_cpu(rx.link);
    if (rfd_command & 0x8000) {
        /* EL bit is set, so this was the last frame. */
        assert(0);
    }
    if (rfd_command & 0x4000) {
        /* S bit is set. */
        set_ru_state(s, ru_suspended);
    }
}



The crash-at-reboot problem is caused by     
    if (rfd_command & 0x8000) {
        /* EL bit is set, so this was the last frame. */
        assert(0);
    }

I see a single frame where rfd_command == 0x8000 just before the system
is reset.  I do not know why.   I have not observed frames like that at
other times.  Replacing the assert(0) by a return makes qemu reboot
properly instead of crashing.


The other random crashes seem to be caused by the
    assert(size <= rfd_size);

from time to time I observe frames where rfd_command, rx.link,
rx.rx_buf_addr, rfd_size all are 0, while size > 0.  I suspect that this
may be due to cpu_physical_memory_read() failing, but my knowledge of
qemu internals is far too limited to do anything more than guessing. 

returning at size > rfd_size (or maybe rfd_size == 0?) avoids these
crashes too.  But I have a feeling that this is not the proper fix...
Maybe there should be some sanity checking of the values returned by
cpu_physical_memory_read()?   Or something else?




Bjørn