2013/1/26 Paul Brook <p...@codesourcery.com>

> > 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

Reply via email to