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

[Scott Wood <scottwood@freescale.com>]

On 07/18/2013 04:27:50 AM, Fabien Chouteau wrote:
> 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.

How is rx_padding 2?  rx_init_frame() will set it to 4 (since the  
packet size is not less than 60).  The only other place I see that  
modifies rx_padding is "etsec->rx_padding -= rem", but that doesn't  
happen because the "*size == etsec->rx_padding" check happens first.

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

What rest of padding?  I thought you said rx_padding was 2 somehow?  If  
that were true, then it would be zero at the end.

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

I don't think it'd be that hard -- you already have an array of  
registers.  The information could easily go in there, and be checked by  
common infrastructure.

-Scott