[Qemu-devel] [PATCH 0/2] minor fixes to lan9118 emulation

[Qemu-devel] [PATCH 0/2] minor fixes to lan9118 emulation

[Andrew Baumann]

This series fixes two niggles in the lan9118 emulation. Together,
these are enough for it to work with a simple driver for the SMSC 9221
(which is very similar).

Cheers,
Andrew

Andrew Baumann (2):
  lan9118: fix emulation of MAC address loaded bit in E2P_CMD register
  lan9118: log and ignore access to invalid registers, rather than
    aborting

 hw/net/lan9118.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

-- 
2.5.3

[Qemu-devel] [PATCH 1/2] lan9118: fix emulation of MAC address loaded bit in E2P_CMD register

[Andrew Baumann]

There appears to have been a longstanding typo in the implementation
of the "MAC address loaded" bit in the E2P_CMD (EEPROM command)
register. The code was using 0x10, but the controller spec says it
should be bit 8 (0x100).

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
This may have slipped through, because the Linux driver doesn't
check that bit; it simply reads the MAC address and assumes it is
valid.

 hw/net/lan9118.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 4f0e840..133fd3d 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -56,6 +56,8 @@ do { fprintf(stderr, "lan9118: error: " fmt , ## __VA_ARGS__);} while (0)
 #define CSR_E2P_CMD     0xb0
 #define CSR_E2P_DATA    0xb4
 
+#define E2P_CMD_MAC_ADDR_LOADED 0x100
+
 /* IRQ_CFG */
 #define IRQ_INT         0x00001000
 #define IRQ_EN          0x00000100
@@ -352,14 +354,14 @@ static void lan9118_reload_eeprom(lan9118_state *s)
 {
     int i;
     if (s->eeprom[0] != 0xa5) {
-        s->e2p_cmd &= ~0x10;
+        s->e2p_cmd &= ~E2P_CMD_MAC_ADDR_LOADED;
         DPRINTF("MACADDR load failed\n");
         return;
     }
     for (i = 0; i < 6; i++) {
         s->conf.macaddr.a[i] = s->eeprom[i + 1];
     }
-    s->e2p_cmd |= 0x10;
+    s->e2p_cmd |= E2P_CMD_MAC_ADDR_LOADED;
     DPRINTF("MACADDR loaded from eeprom\n");
     lan9118_mac_changed(s);
 }
@@ -937,7 +939,7 @@ static uint32_t do_mac_read(lan9118_state *s, int reg)
 
 static void lan9118_eeprom_cmd(lan9118_state *s, int cmd, int addr)
 {
-    s->e2p_cmd = (s->e2p_cmd & 0x10) | (cmd << 28) | addr;
+    s->e2p_cmd = (s->e2p_cmd & E2P_CMD_MAC_ADDR_LOADED) | (cmd << 28) | addr;
     switch (cmd) {
     case 0:
         s->e2p_data = s->eeprom[addr];
-- 
2.5.3

Re: [Qemu-devel] [PATCH 1/2] lan9118: fix emulation of MAC address loaded bit in E2P_CMD register

[Jason Wang]

On 12/05/2015 02:58 AM, Andrew Baumann wrote:
> There appears to have been a longstanding typo in the implementation
> of the "MAC address loaded" bit in the E2P_CMD (EEPROM command)
> register. The code was using 0x10, but the controller spec says it
> should be bit 8 (0x100).
>
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
> This may have slipped through, because the Linux driver doesn't
> check that bit; it simply reads the MAC address and assumes it is
> valid.

Applied in my -net for 2.5. Thanks

>
>  hw/net/lan9118.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
> index 4f0e840..133fd3d 100644
> --- a/hw/net/lan9118.c
> +++ b/hw/net/lan9118.c
> @@ -56,6 +56,8 @@ do { fprintf(stderr, "lan9118: error: " fmt , ## __VA_ARGS__);} while (0)
>  #define CSR_E2P_CMD     0xb0
>  #define CSR_E2P_DATA    0xb4
>  
> +#define E2P_CMD_MAC_ADDR_LOADED 0x100
> +
>  /* IRQ_CFG */
>  #define IRQ_INT         0x00001000
>  #define IRQ_EN          0x00000100
> @@ -352,14 +354,14 @@ static void lan9118_reload_eeprom(lan9118_state *s)
>  {
>      int i;
>      if (s->eeprom[0] != 0xa5) {
> -        s->e2p_cmd &= ~0x10;
> +        s->e2p_cmd &= ~E2P_CMD_MAC_ADDR_LOADED;
>          DPRINTF("MACADDR load failed\n");
>          return;
>      }
>      for (i = 0; i < 6; i++) {
>          s->conf.macaddr.a[i] = s->eeprom[i + 1];
>      }
> -    s->e2p_cmd |= 0x10;
> +    s->e2p_cmd |= E2P_CMD_MAC_ADDR_LOADED;
>      DPRINTF("MACADDR loaded from eeprom\n");
>      lan9118_mac_changed(s);
>  }
> @@ -937,7 +939,7 @@ static uint32_t do_mac_read(lan9118_state *s, int reg)
>  
>  static void lan9118_eeprom_cmd(lan9118_state *s, int cmd, int addr)
>  {
> -    s->e2p_cmd = (s->e2p_cmd & 0x10) | (cmd << 28) | addr;
> +    s->e2p_cmd = (s->e2p_cmd & E2P_CMD_MAC_ADDR_LOADED) | (cmd << 28) | addr;
>      switch (cmd) {
>      case 0:
>          s->e2p_data = s->eeprom[addr];

[Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting

[Andrew Baumann]

With this change, access to invalid/unimplemented device registers are
logged as a "guest error" rather than aborting qemu with
hw_error. This enables drivers for similar devices (e.g. SMSC 9221),
by simply ignoring the unimplemented writes. It's also closer to what
real hardware does.

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
 hw/net/lan9118.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 133fd3d..1734b52 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -904,7 +904,8 @@ static void do_mac_write(lan9118_state *s, int reg, uint32_t val)
          */
         break;
     default:
-        hw_error("lan9118: Unimplemented MAC register write: %d = 0x%x\n",
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "lan9118: Unimplemented MAC register write: %d = 0x%x\n",
                  s->mac_cmd & 0xf, val);
     }
 }
@@ -932,8 +933,10 @@ static uint32_t do_mac_read(lan9118_state *s, int reg)
     case MAC_FLOW:
         return s->mac_flow;
     default:
-        hw_error("lan9118: Unimplemented MAC register read: %d\n",
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "lan9118: Unimplemented MAC register read: %d\n",
                  s->mac_cmd & 0xf);
+        return 0;
     }
 }
 
@@ -1130,7 +1133,8 @@ static void lan9118_writel(void *opaque, hwaddr offset,
         break;
 
     default:
-        hw_error("lan9118_write: Bad reg 0x%x = %x\n", (int)offset, (int)val);
+        qemu_log_mask(LOG_GUEST_ERROR, "lan9118_write: Bad reg 0x%x = %x\n",
+                      (int)offset, (int)val);
         break;
     }
     lan9118_update(s);
@@ -1248,7 +1252,7 @@ static uint64_t lan9118_readl(void *opaque, hwaddr offset,
     case CSR_E2P_DATA:
         return s->e2p_data;
     }
-    hw_error("lan9118_read: Bad reg 0x%x\n", (int)offset);
+    qemu_log_mask(LOG_GUEST_ERROR, "lan9118_read: Bad reg 0x%x\n", (int)offset);
     return 0;
 }
 
-- 
2.5.3

Re: [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting

[Jason Wang]

On 12/05/2015 02:58 AM, Andrew Baumann wrote:
> With this change, access to invalid/unimplemented device registers are
> logged as a "guest error" rather than aborting qemu with
> hw_error. This enables drivers for similar devices (e.g. SMSC 9221),
> by simply ignoring the unimplemented writes. It's also closer to what
> real hardware does.
>
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
>  hw/net/lan9118.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

Applied in my -net for 2.5. Thanks

Btw, looks like there're two more hw_error() in this file, better remove
them also?

Thanks

>
> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
> index 133fd3d..1734b52 100644
> --- a/hw/net/lan9118.c
> +++ b/hw/net/lan9118.c
> @@ -904,7 +904,8 @@ static void do_mac_write(lan9118_state *s, int reg, uint32_t val)
>           */
>          break;
>      default:
> -        hw_error("lan9118: Unimplemented MAC register write: %d = 0x%x\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "lan9118: Unimplemented MAC register write: %d = 0x%x\n",
>                   s->mac_cmd & 0xf, val);
>      }
>  }
> @@ -932,8 +933,10 @@ static uint32_t do_mac_read(lan9118_state *s, int reg)
>      case MAC_FLOW:
>          return s->mac_flow;
>      default:
> -        hw_error("lan9118: Unimplemented MAC register read: %d\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "lan9118: Unimplemented MAC register read: %d\n",
>                   s->mac_cmd & 0xf);
> +        return 0;
>      }
>  }
>  
> @@ -1130,7 +1133,8 @@ static void lan9118_writel(void *opaque, hwaddr offset,
>          break;
>  
>      default:
> -        hw_error("lan9118_write: Bad reg 0x%x = %x\n", (int)offset, (int)val);
> +        qemu_log_mask(LOG_GUEST_ERROR, "lan9118_write: Bad reg 0x%x = %x\n",
> +                      (int)offset, (int)val);
>          break;
>      }
>      lan9118_update(s);
> @@ -1248,7 +1252,7 @@ static uint64_t lan9118_readl(void *opaque, hwaddr offset,
>      case CSR_E2P_DATA:
>          return s->e2p_data;
>      }
> -    hw_error("lan9118_read: Bad reg 0x%x\n", (int)offset);
> +    qemu_log_mask(LOG_GUEST_ERROR, "lan9118_read: Bad reg 0x%x\n", (int)offset);
>      return 0;
>  }
>  

Re: [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting

[Andrew Baumann]

> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Sunday, 6 December 2015 18:44
> On 12/05/2015 02:58 AM, Andrew Baumann wrote:
> > With this change, access to invalid/unimplemented device registers are
> > logged as a "guest error" rather than aborting qemu with
> > hw_error. This enables drivers for similar devices (e.g. SMSC 9221),
> > by simply ignoring the unimplemented writes. It's also closer to what
> > real hardware does.
> >
> > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> > ---
> >  hw/net/lan9118.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> Applied in my -net for 2.5. Thanks
> 
> Btw, looks like there're two more hw_error() in this file, better remove
> them also?

Yeah, I considered doing that, but figured that those cases (incorrectly-sized register writes in 16-bit mode) are indicative of a pretty badly screwed-up guest, and was going for a minimal patch. It probably makes sense to change them for consistency, though.

Andrew

Re: [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting

[Paolo Bonzini]

On 07/12/2015 06:20, Andrew Baumann wrote:
> Yeah, I considered doing that, but figured that those cases
> (incorrectly-sized register writes in 16-bit mode) are indicative of
> a pretty badly screwed-up guest, and was going for a minimal patch.
> It probably makes sense to change them for consistency, though.

I think those should be fixed by modifying lan9118_*_mem_ops and adding
.valid.{min,max}_access_size.  Not for 2.5, however.  (Probably these
patches should also be 2.6 + qemu-stable rather than 2.5).

Paolo

Re: [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting

[Andrew Baumann]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Monday, 7 December 2015 01:53
> On 07/12/2015 06:20, Andrew Baumann wrote:
> > Yeah, I considered doing that, but figured that those cases
> > (incorrectly-sized register writes in 16-bit mode) are indicative of
> > a pretty badly screwed-up guest, and was going for a minimal patch.
> > It probably makes sense to change them for consistency, though.
> 
> I think those should be fixed by modifying lan9118_*_mem_ops and adding
> .valid.{min,max}_access_size.  Not for 2.5, however.  (Probably these
> patches should also be 2.6 + qemu-stable rather than 2.5).

Just to clarify: would you guys like me to prepare such a patch? I'm not familiar with the memory op APIs, and don't have a good setup for testing this device emulation any more (and certainly not in 16-bit mode!), so would prefer to defer to someone else.

BTW, I also see no great urgency for these patches. They're minor fixes, and it would be good to have them in, but it's certainly not a regression as the code has been that way for ages.

Andrew

Re: [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting

[Paolo Bonzini]

On 07/12/2015 22:53, Andrew Baumann wrote:
>>> I think those should be fixed by modifying lan9118_*_mem_ops and
>>> adding .valid.{min,max}_access_size.  Not for 2.5, however.
>>> (Probably these patches should also be 2.6 + qemu-stable rather
>>> than 2.5).
> Just to clarify: would you guys like me to prepare such a patch?

No, it's not necessary.

Paolo

> I'm
> not familiar with the memory op APIs, and don't have a good setup for
> testing this device emulation any more (and certainly not in 16-bit
> mode!), so would prefer to defer to someone else.
> 
> BTW, I also see no great urgency for these patches. They're minor
> fixes, and it would be good to have them in, but it's certainly not a
> regression as the code has been that way for ages.