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
