Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC)

[Scott Wood]

On 07/17/2013 05:17:06 AM, Fabien Chouteau wrote:
> On 07/16/2013 07:50 PM, Scott Wood wrote:
> > On 07/16/2013 10:28:28 AM, Fabien Chouteau wrote:
> >> On 07/16/2013 04:06 AM, Scott Wood wrote:
> >> > On 07/10/2013 12:10:02 PM, Fabien Chouteau wrote:
> >> >> +    if (*size == etsec->rx_padding) {
> >> >> +        /* The remaining bytes are for padding which is not  
> actually allocated
> >> >> +           in the buffer */
> >> >> +
> >> >> +        rem = MIN(etsec->regs[MRBLR].value - bd->length,  
> etsec->rx_padding);
> >> >> +
> >> >> +        if (rem > 0) {
> >> >> +            memset(padd, 0x0, sizeof(padd));
> >> >> +            etsec->rx_padding -= rem;
> >> >> +            *size             -= rem;
> >> >> +            bd->length        += rem;
> >> >> +            cpu_physical_memory_write(bufptr, padd, rem);
> >> >> +        }
> >> >> +    }
> >> >
> >> > What if *size > 0 && *size < etsec->rx_padding?
> >>
> >> I don't think it's possible...
> >
> > Maybe throw in an assertion, then?
> >
> > I can see how it might not be possible if rx_padding is being used  
> for padding a short frame, since MRBLR must be a multiple of 64, but  
> what if it's 4 bytes for CRC?
> >
>
> Can you explain a possible error scenario?

126 byte packet, no fcb.  rx_padding is 4 for CRC.  Suppose MRBLR is  
128.  Wouldn't *size be 2 here?

> > Could you at least have a way to diagnose when the guest OS tries to
> > use some functionality that you don't support, rather than silently
> > doing the wrong thing?
> >
>
> This device is so complex, detecting unsupported features would take  
> too
> much work.

I was thinking along the lines of marking registers and bits within  
registers as supported (or which are properly no-ops in QEMU) -- and  
warning the first time you see a non-default-value write to an  
unsupported field or register.  It could save a lot of debugging.

-Scott