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

[Alexander Graf]

On 17.07.2013, at 12:17, 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:
>>>>> +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);
>        }
>    }

Well, this code is pretty broken anyways. It should honor the IF type that we 
request from it. Then we can also add eTSEC as one.

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

In this case a simple if(<upper bits of addresses>) hw_error(...) would be good 
enough :). It's just to make sure that all this valuable knowledge doesn't get 
lost any someone who eventually does run into 36bit addressing doesn't have to 
debug for a week to figure out who randomly overwrites his kernel ;).


Alex