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
