[Qemu-devel] Re: [PATCH 2/9] eepro100: Fix endianness issues

2011-04-03 Thread Michael S. Tsirkin 
On Fri, Apr 01, 2011 at 07:52:52PM +0200, Stefan Weil wrote:
 [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.

Yes but they don't seem to do much useful,
IMO open-coding will be clearer, they are unlikely
to be generally useful. But if you like to
keep them pls add a eepro100 prefix.

-- 
MST

[Qemu-devel] Re: [PATCH 2/9] eepro100: Fix endianness issues

2011-04-01 Thread Stefan Weil 

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 w...@mail.berlios.de
---
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 http://www.gnu.org/licenses/.
*
* 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[] = {
0x, 0x, 0x, 0x, 0x, 0x, 0x, 0x,
};

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

[Qemu-devel] Re: [PATCH 2/9] eepro100: Fix endianness issues

2011-03-31 Thread 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 w...@mail.berlios.de
 ---
  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 http://www.gnu.org/licenses/.
   *
   * 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[] = {
  0x, 0x, 0x, 0x, 0x, 0x, 0x, 0x,
  };
  
 -/* 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.

  #if defined(DEBUG_EEPRO100)
  static const char *nic_dump(const uint8_t * buf, unsigned size)
  {
 @@ -590,8 +640,7 @@ static void nic_selective_reset(EEPRO100State * s)
  TRACE(EEPROM, logout(checksum=0x%04x\n, eeprom_contents[EEPROM_SIZE - 
 1]));
  
  memset(s-mem, 0, sizeof(s-mem));
 -uint32_t val = BIT(21);
 -memcpy(s-mem[SCBCtrlMDI], val, sizeof(val));
 +e100_write_reg4(s, SCBCtrlMDI, BIT(21));
  
  assert(sizeof(s-mdimem) == sizeof(eepro100_mdi_default));
  memcpy(s-mdimem[0], eepro100_mdi_default[0], sizeof(s-mdimem));
 @@ -739,10 +788,10 @@ static void tx_command(EEPRO100State *s)
  }