* [Qemu-devel] [PATCH 0/2] minor fixes to lan9118 emulation
@ 2015-12-04 18:58 Andrew Baumann
2015-12-04 18:58 ` [Qemu-devel] [PATCH 1/2] lan9118: fix emulation of MAC address loaded bit in E2P_CMD register Andrew Baumann
2015-12-04 18:58 ` [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting Andrew Baumann
0 siblings, 2 replies; 9+ messages in thread
From: Andrew Baumann @ 2015-12-04 18:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Jason Wang, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 1/2] lan9118: fix emulation of MAC address loaded bit in E2P_CMD register
2015-12-04 18:58 [Qemu-devel] [PATCH 0/2] minor fixes to lan9118 emulation Andrew Baumann
@ 2015-12-04 18:58 ` Andrew Baumann
2015-12-07 2:42 ` Jason Wang
2015-12-04 18:58 ` [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting Andrew Baumann
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Baumann @ 2015-12-04 18:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Jason Wang, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting
2015-12-04 18:58 [Qemu-devel] [PATCH 0/2] minor fixes to lan9118 emulation Andrew Baumann
2015-12-04 18:58 ` [Qemu-devel] [PATCH 1/2] lan9118: fix emulation of MAC address loaded bit in E2P_CMD register Andrew Baumann
@ 2015-12-04 18:58 ` Andrew Baumann
2015-12-07 2:43 ` Jason Wang
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Baumann @ 2015-12-04 18:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Jason Wang, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] lan9118: fix emulation of MAC address loaded bit in E2P_CMD register
2015-12-04 18:58 ` [Qemu-devel] [PATCH 1/2] lan9118: fix emulation of MAC address loaded bit in E2P_CMD register Andrew Baumann
@ 2015-12-07 2:42 ` Jason Wang
0 siblings, 0 replies; 9+ messages in thread
From: Jason Wang @ 2015-12-07 2:42 UTC (permalink / raw)
To: Andrew Baumann, qemu-devel
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];
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting
2015-12-04 18:58 ` [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting Andrew Baumann
@ 2015-12-07 2:43 ` Jason Wang
2015-12-07 5:20 ` Andrew Baumann
0 siblings, 1 reply; 9+ messages in thread
From: Jason Wang @ 2015-12-07 2:43 UTC (permalink / raw)
To: Andrew Baumann, qemu-devel
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;
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting
2015-12-07 2:43 ` Jason Wang
@ 2015-12-07 5:20 ` Andrew Baumann
2015-12-07 9:52 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Baumann @ 2015-12-07 5:20 UTC (permalink / raw)
To: Jason Wang, qemu-devel
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting
2015-12-07 5:20 ` Andrew Baumann
@ 2015-12-07 9:52 ` Paolo Bonzini
2015-12-07 21:53 ` Andrew Baumann
0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2015-12-07 9:52 UTC (permalink / raw)
To: Andrew Baumann, Jason Wang, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting
2015-12-07 9:52 ` Paolo Bonzini
@ 2015-12-07 21:53 ` Andrew Baumann
2015-12-09 14:58 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Baumann @ 2015-12-07 21:53 UTC (permalink / raw)
To: Paolo Bonzini, Jason Wang, qemu-devel
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting
2015-12-07 21:53 ` Andrew Baumann
@ 2015-12-09 14:58 ` Paolo Bonzini
0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2015-12-09 14:58 UTC (permalink / raw)
To: Andrew Baumann, Jason Wang, qemu-devel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-12-09 14:58 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-04 18:58 [Qemu-devel] [PATCH 0/2] minor fixes to lan9118 emulation Andrew Baumann
2015-12-04 18:58 ` [Qemu-devel] [PATCH 1/2] lan9118: fix emulation of MAC address loaded bit in E2P_CMD register Andrew Baumann
2015-12-07 2:42 ` Jason Wang
2015-12-04 18:58 ` [Qemu-devel] [PATCH 2/2] lan9118: log and ignore access to invalid registers, rather than aborting Andrew Baumann
2015-12-07 2:43 ` Jason Wang
2015-12-07 5:20 ` Andrew Baumann
2015-12-07 9:52 ` Paolo Bonzini
2015-12-07 21:53 ` Andrew Baumann
2015-12-09 14:58 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.