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

[Fabien Chouteau]

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:
>> >> +obj-$(CONFIG_ETSEC) += etsec.o etsec_registers.o etsec_rings.o 
>> >> etsec_miim.o
>> >
>> > Maybe an fsl_etsec directory?
>> >
>>
>> Is it really necessary?
> 
> Not strictly necessary, but it would help keep things tidy.
> 

I will try :)

>> >> +DeviceState *etsec_create(hwaddr         base,
>> >> +                          MemoryRegion * mr,
>> >> +                          NICInfo      * nd,
>> >> +                          qemu_irq       tx_irq,
>> >> +                          qemu_irq       rx_irq,
>> >> +                          qemu_irq       err_irq)
>> >>
>> > Do you plan to update hw/ppc/e500.c (or maybe some other platform?) to
>> > call this?
>> >
>>
>> No I don't, not for the moment. I use it in one of our machine (that is not 
>> in mainstream).
> 
> Someone should, so we can test this without your out-of-tree code.
> 
>> e500.c would require PCI support I think.
> 
> e500.c has PCI support, but how is that relevant to eTSEC?

I guess it's not. We would have to remove:

    if (pci_bus) {
        /* Register network interfaces. */
        for (i = 0; i < nb_nics; i++) {
            pci_nic_init_nofail(&nd_table[i], pci_bus, "virtio", NULL);
        }
    }

> 
>> >> +    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?

>> >> +static void rx_init_frame(eTSEC *etsec, const uint8_t *buf, size_t size)
>> >> +{
>> >> +    uint32_t fcb_size = 0;
>> >> +    uint8_t  prsdep   = (etsec->regs[RCTRL].value >> RCTRL_PRSDEP_OFFSET)
>> >> +        & RCTRL_PRSDEP_MASK;
>> >> +
>> >> +    if (prsdep != 0) {
>> >> +        /* Prepend FCB */
>> >> +        fcb_size = 8 + 2;          /* FCB size + align */
>> >> +        /* I can't find this 2 bytes alignement in fsl documentation but 
>> >> VxWorks
>> >> +           expects them */
>> >
>> > Could these 2 bytes be from RCTRL[PAD], which you seem to ignore?
>>
>> Did you mean RCTRL[PAL]? It could be that.
> 
> Yes, sorry.
> 

Thanks for pointing it ;)

>> >> +    etsec->rx_padding    = 4;
>>
>> CRC.
>>
>> >> +    if (size < 60) {
>> >> +        etsec->rx_padding += 60 - size;
>> >> +    }
>> >
>> > Could you explain what you're doing with rx_padding?
>>
>> Avoiding short frames. I'll add a comments.
> 
> This is for when RCTRL[RSF] is set, to accept short frames?  Is it documented 
> anywhere that these frames are zero-padded to 64 bytes on rx?  If not, is 
> this the observed behavior of real hardware?
> 
> In any case, please add some comments.
> 

Actually, this might be obsolete. At the time QEMU was sending short
frames, but vxWorks drop them, so I made this change to always have 64B
frames. Now I have another patch to fix this and I will remove this hack
and implement RCTRL[RSF].


>> >> +    /* ring_base = (etsec->regs[RBASEH].value & 0xF) << 32; */
>> >> +    ring_base     += etsec->regs[RBASE0 + ring_nbr].value & ~0x7;
>> >> +    start_bd_addr  = bd_addr = etsec->regs[RBPTR0 + ring_nbr].value & 
>> >> ~0x7;
>> >
>> > What about RBDBPH (upper bits of physical address)?  Likewise for TX.
>> >
>>
>> I'm only interested in 32bits address spaces, so RBASEH, TBASEH, RBDBPH or 
>> TBDBPH.
>
> When adding code to mainline, it's about more than what you're personally 
> interested in...
>

If I'm not "personally" interested, I will not have time to develop or
test a feature. If someone wants to do 36bit addressing, feel free to do
it, that's why I send the patch on QEMU-devel, but I won't implement
full support of eTSEC myself...

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

Regards,

-- 
Fabien Chouteau