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

[Fabien Chouteau <chouteau@adacore.com>]

On 07/17/2013 11:02 PM, Scott Wood wrote:
> 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?
> 

Yes, at the end of the function, but then rx_padding is 2 as well.

value of "to_write" will be 126:

*size = etsec->rx_remaining_data + etsec->rx_padding;
      = 126 + 4;
      = 130;

to_write = MIN(etsec->rx_fcb_size + *size - etsec->rx_padding, etsec->regs[MRBLR].value);
         = MIN(0 + 130 - 4, 128);
         = MIN(126, 128);
         = 126;

So we write the packet in the first part of the BD, and there's 2 bytes
left in the BD.

*size -= to_write;
       = 4;
bd->length = to_write;
           = 126;

So *size == etsec->rx_padding (This is expected as the first write
operation can only write data and no padding, I will comment this fact)

rem = MIN(etsec->regs[MRBLR].value - bd->length, etsec->rx_padding);
    = MIN(128 - 126, 4);
    = MIN(2, 4);
    = 2;

We write 2 bytes of padding.

etsec->rx_padding -= rem;
                   = 2;
*size             -= rem;
                   = 2;
bd->length        += rem;
                   = 128;

The BD is full, we will have to put the rest of padding in the next one.

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

I think we'll spend more time implementing this than debugging. Another
solution is to enable debug output and see which registers are read or
write.

Regards,

-- 
Fabien Chouteau