Re: [Qemu-devel] [PATCH v2 05/20] arm: add Faraday FTGMAC100 1Gbps ethernet support

[Kuo-Jung Su]

2013/1/26 Paul Brook <address@hidden>

> In order to reduce the processing load of the host CPU, the FTGMAC100
> implements TCP, UDP, and IP V4 checksum generation and validation, and
> supports VLAN tagging.

I see no evidence of these features in the code.

My bad .... and yes, the VLAN and checksum offload are not yet implemented.

I simply fotget to remove these description ... :P

but since the checksum offload feature of the real hardware actually malfunctioned.

(becuase of the alignment issue).
I'll only implement the VLAN tagging feature in the next version.

> +static void ftgmac100_read_desc(hwaddr addr, void *desc)
> +{
> +    int i;
> +    uint32_t *p = desc;
> +
> +    cpu_physical_memory_read(addr, desc, 16);
> +
> +    for (i = 0; i < 16; i += 4) {
> +        *p = le32_to_cpu(*p);
> +    }
> +}

You're relying on the compiler choosing a particular bitfield and structure
layout. Don't do that.  Especially when one of the fields is a void*.  Clearly
never been tested on a 64-bit host. "void *desc" is just plain lazy.

yes, it's my bad. It'll be fixed later

 

> +        buf = s->txbuff.buf + s->txbuff.len;
> +        cpu_physical_memory_read(txd.buf, (uint8_t *)buf, txd.len);

Buffer overflow.  In at least two differnt ways.

yes, it's my bad. It'll be fixed later

> +            if (!(s->maccr & MACCR_HT_MULTI_EN)) {
> +                printf("[qemu] ftgmac100_receive: mcst filtered\n");
> +                return -1;

Looks like stray debug code.  Several other occurences.

> +    case REG_TXPD:
> +    case REG_HPTXPD:
> +        qemu_mod_timer(s->qtimer, qemu_get_clock_ns(vm_clock) + 1);

Using a timer here is wrong.  Either you should transmit immediately, or you
should wait for something else to happen.  Delaying by 1ns is never the right
answer.

Paul

yes, it's my bad. I'll try to use mutex later

--
Best wishes,
Kuo-Jung Su