From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UzOnG-0003Cg-Ew for qemu-devel@nongnu.org; Wed, 17 Jul 2013 06:17:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UzOnF-0003E5-1c for qemu-devel@nongnu.org; Wed, 17 Jul 2013 06:17:10 -0400 Message-ID: <51E66F22.6030202@adacore.com> Date: Wed, 17 Jul 2013 12:17:06 +0200 From: Fabien Chouteau MIME-Version: 1.0 References: <1373997013.8183.332@snotra> In-Reply-To: <1373997013.8183.332@snotra> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Scott Wood Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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