From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=53644 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q5iWs-0007a4-DT for qemu-devel@nongnu.org; Fri, 01 Apr 2011 13:53:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q5iWo-0006Bt-Ra for qemu-devel@nongnu.org; Fri, 01 Apr 2011 13:53:00 -0400 Received: from moutng.kundenserver.de ([212.227.17.8]:53428) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q5iWo-0006BO-GE for qemu-devel@nongnu.org; Fri, 01 Apr 2011 13:52:58 -0400 Message-ID: <4D9610F4.6040704@mail.berlios.de> Date: Fri, 01 Apr 2011 19:52:52 +0200 From: Stefan Weil MIME-Version: 1.0 References: <1301603611-7964-1-git-send-email-weil@mail.berlios.de> <1301603611-7964-3-git-send-email-weil@mail.berlios.de> <20110331215253.GD27264@redhat.com> In-Reply-To: <20110331215253.GD27264@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 2/9] eepro100: Fix endianness issues List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: QEMU Developers Am 31.03.2011 23:52, schrieb Michael S. Tsirkin: > On Thu, Mar 31, 2011 at 10:33:24PM +0200, Stefan Weil wrote: >> Like other Intel devices, e100 (eepro100) uses little endian byte order. >> >> This patch was tested with these combinations: >> >> i386 host, i386 + mipsel guests (le-le) >> mipsel host, i386 guest (le-le) >> i386 host, mips + ppc guests (le-be) >> mips host, i386 guest (be-le) >> >> mips and mipsel hosts were emulated machines. >> >> Signed-off-by: Stefan Weil >> --- >> hw/eepro100.c | 113 >> ++++++++++++++++++++++++++++++++++++++++----------------- >> 1 files changed, 80 insertions(+), 33 deletions(-) >> >> diff --git a/hw/eepro100.c b/hw/eepro100.c >> index f89ff17..c789767 100644 >> --- a/hw/eepro100.c >> +++ b/hw/eepro100.c >> @@ -20,11 +20,10 @@ >> * along with this program. If not, see . >> * >> * Tested features (i82559): >> - * PXE boot (i386) ok >> + * PXE boot (i386 guest, i386 / mips / mipsel / ppc host) ok >> * Linux networking (i386) ok >> * >> * Untested: >> - * non-i386 platforms >> * Windows networking >> * >> * References: >> @@ -130,7 +129,7 @@ typedef struct { >> >> /* Offsets to the various registers. >> All accesses need not be longword aligned. */ >> -enum speedo_offsets { >> +typedef enum { >> SCBStatus = 0, /* Status Word. */ >> SCBAck = 1, >> SCBCmd = 2, /* Rx/Command Unit command and status. */ >> @@ -145,7 +144,7 @@ enum speedo_offsets { >> SCBpmdr = 27, /* Power Management Driver. */ >> SCBgctrl = 28, /* General Control. */ >> SCBgstat = 29, /* General Status. */ >> -}; >> +} E100RegisterOffset; >> >> /* A speedo3 transmit buffer descriptor with two buffers... */ >> typedef struct { >> @@ -307,7 +306,32 @@ static const uint16_t eepro100_mdi_mask[] = { >> 0xffff, 0xffff, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, >> }; >> >> -/* XXX: optimize */ >> +/* Read a 16 bit little endian value from physical memory. */ >> +static uint16_t lduw_le_phys(target_phys_addr_t addr) >> +{ >> + /* Load 16 bit (little endian) word from emulated hardware. */ >> + uint16_t val; >> + cpu_physical_memory_read(addr, (uint8_t *)&val, sizeof(val)); >> + return le16_to_cpu(val); >> +} >> + >> +/* Read a 32 bit little endian value from physical memory. */ >> +static uint32_t ldl_le_phys(target_phys_addr_t addr) >> +{ >> + /* Load 32 bit (little endian) word from emulated hardware. */ >> + uint32_t val; >> + cpu_physical_memory_read(addr, (uint8_t *)&val, sizeof(val)); >> + return le32_to_cpu(val); >> +} >> + >> +/* Write a 16 bit little endian value to physical memory. */ >> +static void stw_le_phys(target_phys_addr_t addr, uint16_t val) >> +{ >> + val = cpu_to_le16(val); >> + cpu_physical_memory_write(addr, (const uint8_t *)&val, sizeof(val)); >> +} >> + >> +/* Write a 32 bit little endian value to physical memory. */ > > So why not opencode e.g. > le32_to_cpu(ldl_phys(addr)) > > wrappers really worth it? What do I miss? > > If you insist on these online wrappers, pls prefix > them with eepro100_. > Also, why not use lduw_phys and friends internally? > cpu_physical_ is slower ... > >> static void stl_le_phys(target_phys_addr_t addr, uint32_t val) >> { >> val = cpu_to_le32(val); >> @@ -339,6 +363,32 @@ static unsigned compute_mcast_idx(const uint8_t >> * ep) >> return (crc & BITS(7, 2)) >> 2; >> } >> >> +/* Read a 16 bit control/status (CSR) register. */ >> +static uint16_t e100_read_reg2(EEPRO100State *s, E100RegisterOffset >> addr) >> +{ >> + return le16_to_cpup((uint16_t *)&s->mem[addr]); >> +} >> + >> +/* Read a 32 bit control/status (CSR) register. */ >> +static uint32_t e100_read_reg4(EEPRO100State *s, E100RegisterOffset >> addr) >> +{ >> + return le32_to_cpup((uint32_t *)&s->mem[addr]); >> +} >> + >> +/* Write a 16 bit control/status (CSR) register. */ >> +static void e100_write_reg2(EEPRO100State *s, E100RegisterOffset addr, >> + uint16_t val) >> +{ >> + cpu_to_le16w((uint16_t *)&s->mem[addr], val); >> +} >> + >> +/* Read a 32 bit control/status (CSR) register. */ >> +static void e100_write_reg4(EEPRO100State *s, E100RegisterOffset addr, >> + uint32_t val) >> +{ >> + cpu_to_le32w((uint32_t *)&s->mem[addr], val); >> +} >> + > > Note that cpu_to_le32w requires an aligned address, unlike > memcpy, and there's no guarantee > addr is aligned apparently? > > If true you need to memcpy to a 32 bit variable, then > cpu_to_le32w ther result. > [snip] Thank you for your review, especially for the hints at lduw_phys and potential alignment issues. I'll apply them to a new version of this patch. There was already a function without prefix (stl_le_phys), and the new ones belong to the same "family". There is nothing e100 specific in them, so they might be added to qemu-common.h as well. That was (and is) the reason why I did not add a prefix.