Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
Vlad here told me he did some research and this
does not actually match hardware behaviour
for either e1000 or rtl8139.
Vlad, would you like to elaborate on-list?
I think we should revert this for 1.8 and
look at emulating actual hardware behaviour.
---
hw/net/e1000.c | 2 +-
hw/net/rtl8139.c | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ec8ecd7..2d60639 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+if (index == RA || index == RA + 1) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t *)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..7f2b4db 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque, uint8_t
addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
break;
--
1.8.3.1
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On 11/18/2013 10:02 AM, Michael S. Tsirkin wrote:
On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
Vlad here told me he did some research and this
does not actually match hardware behaviour
for either e1000 or rtl8139.
Vlad, would you like to elaborate on-list?
Sure.
I decided to dig into the hardware data-sheets and the OS drivers
that use the HW to see what's really going on and how the
hw expects this data to be programmed.
Here is what I've found so far:
E1000:
e1000 stores each mac address in 2 registers:
RAL - receive register low
RAH - receive register high
The highest order bit of RAH (bit 31) is called the address available
bit. When this bit is set the HW will attempt to use the address for
packet matching. So, when the mac address is initially programmed
into HW, that AV bit is not set until RAH is written. Thus drivers
really should do writes in RAL+RAH order, otherwise AV bit would be
set on a partially set address.
There is a slight issue when the receive address registers already
have a value. Since the address is written in 2 separate writes,
there is a very small window of time when the RAL is set to the new
value and RAH is set to the old value with AV still set. I am
talking to Intel guys about this now.
So from the POV of notifying libvirt/management that address is
changed, it should be done when RAH is set.
RTL8139:
Realtek devices have a 9346CR Command Register that gates write
access to certain configuration regions on the HW. It is placed
into Configuration register write enabled mode before driver can
write to one of a set of configuration spaces. Even though
the data sheet doesn't mention this, it appears that this must
also must be used to guard write access to receive address register
of the card. All variants of BSD and linux drivers that I've found
use this along with comment that say that this is an undocumented
requirement. I am not sure what the HW does to incoming frames when
the command register is to this mode.
I see 2 things that we might be able to do here:
1) A low-impact change might be to only notify the management when
we've detected an address change and currently exiting
config write mode.
2) A more invasive change might be to disable rx_handling while in
config wirte mode. This would prevent attempting to match
packets to a partially written mac address.
I have a patch that does (1) above.
Thoughts?
-vlad
I think we should revert this for 1.8 and
look at emulating actual hardware behaviour.
---
hw/net/e1000.c | 2 +-
hw/net/rtl8139.c | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ec8ecd7..2d60639 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t
val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+if (index == RA || index == RA + 1) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t
*)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..7f2b4db 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque,
uint8_t addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
break;
--
1.8.3.1
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Mon, Nov 18, 2013 at 12:33:20PM -0500, Vlad Yasevich wrote:
On 11/18/2013 10:02 AM, Michael S. Tsirkin wrote:
On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
Vlad here told me he did some research and this
does not actually match hardware behaviour
for either e1000 or rtl8139.
Vlad, would you like to elaborate on-list?
Sure.
I decided to dig into the hardware data-sheets and the OS drivers
that use the HW to see what's really going on and how the
hw expects this data to be programmed.
Here is what I've found so far:
E1000:
e1000 stores each mac address in 2 registers:
RAL - receive register low
RAH - receive register high
The highest order bit of RAH (bit 31) is called the address available
bit. When this bit is set the HW will attempt to use the address for
packet matching. So, when the mac address is initially programmed
into HW, that AV bit is not set until RAH is written. Thus drivers
really should do writes in RAL+RAH order, otherwise AV bit would be
set on a partially set address.
There is a slight issue when the receive address registers already
have a value. Since the address is written in 2 separate writes,
there is a very small window of time when the RAL is set to the new
value and RAH is set to the old value with AV still set. I am
talking to Intel guys about this now.
So from the POV of notifying libvirt/management that address is
changed, it should be done when RAH is set.
RTL8139:
Realtek devices have a 9346CR Command Register that gates write
access to certain configuration regions on the HW. It is placed
into Configuration register write enabled mode before driver can
write to one of a set of configuration spaces. Even though
the data sheet doesn't mention this, it appears that this must
also must be used to guard write access to receive address register
of the card. All variants of BSD and linux drivers that I've found
use this along with comment that say that this is an undocumented
requirement.
What does a windows driver do BTW?
I am not sure what the HW does to incoming frames when
the command register is to this mode.
I see 2 things that we might be able to do here:
1) A low-impact change might be to only notify the management when
we've detected an address change and currently exiting
config write mode.
2) A more invasive change might be to disable rx_handling while in
config wirte mode. This would prevent attempting to match
packets to a partially written mac address.
I have a patch that does (1) above.
Thoughts?
-vlad
Let's start by reverting cd5be5829c1ce87aa6b3a7806524fac07ac9a757.
I think we should revert this for 1.8 and
look at emulating actual hardware behaviour.
---
hw/net/e1000.c | 2 +-
hw/net/rtl8139.c | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ec8ecd7..2d60639 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t
val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+if (index == RA || index == RA + 1) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t
*)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..7f2b4db 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque,
uint8_t addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
break;
--
1.8.3.1
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On 11/18/2013 02:42 PM, Michael S. Tsirkin wrote:
On Mon, Nov 18, 2013 at 12:33:20PM -0500, Vlad Yasevich wrote:
On 11/18/2013 10:02 AM, Michael S. Tsirkin wrote:
On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
Vlad here told me he did some research and this
does not actually match hardware behaviour
for either e1000 or rtl8139.
Vlad, would you like to elaborate on-list?
Sure.
I decided to dig into the hardware data-sheets and the OS drivers
that use the HW to see what's really going on and how the
hw expects this data to be programmed.
Here is what I've found so far:
E1000:
e1000 stores each mac address in 2 registers:
RAL - receive register low
RAH - receive register high
The highest order bit of RAH (bit 31) is called the address available
bit. When this bit is set the HW will attempt to use the address for
packet matching. So, when the mac address is initially programmed
into HW, that AV bit is not set until RAH is written. Thus drivers
really should do writes in RAL+RAH order, otherwise AV bit would be
set on a partially set address.
There is a slight issue when the receive address registers already
have a value. Since the address is written in 2 separate writes,
there is a very small window of time when the RAL is set to the new
value and RAH is set to the old value with AV still set. I am
talking to Intel guys about this now.
So from the POV of notifying libvirt/management that address is
changed, it should be done when RAH is set.
RTL8139:
Realtek devices have a 9346CR Command Register that gates write
access to certain configuration regions on the HW. It is placed
into Configuration register write enabled mode before driver can
write to one of a set of configuration spaces. Even though
the data sheet doesn't mention this, it appears that this must
also must be used to guard write access to receive address register
of the card. All variants of BSD and linux drivers that I've found
use this along with comment that say that this is an undocumented
requirement.
What does a windows driver do BTW?
I'll boot windows and find out.
-vlad
I am not sure what the HW does to incoming frames when
the command register is to this mode.
I see 2 things that we might be able to do here:
1) A low-impact change might be to only notify the management when
we've detected an address change and currently exiting
config write mode.
2) A more invasive change might be to disable rx_handling while in
config wirte mode. This would prevent attempting to match
packets to a partially written mac address.
I have a patch that does (1) above.
Thoughts?
-vlad
Let's start by reverting cd5be5829c1ce87aa6b3a7806524fac07ac9a757.
I think we should revert this for 1.8 and
look at emulating actual hardware behaviour.
---
hw/net/e1000.c | 2 +-
hw/net/rtl8139.c | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ec8ecd7..2d60639 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t
val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+if (index == RA || index == RA + 1) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t
*)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..7f2b4db 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque,
uint8_t addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
break;
--
1.8.3.1
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On 11/10/2013 07:11 AM, Michael S. Tsirkin wrote:
On Fri, Nov 08, 2013 at 02:42:27PM -0500, Vlad Yasevich wrote:
What about this approach? This only updates the monitory when all the
bits have been written to.
-vlad
Thanks!
Some comments below.
-- 8 --
Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
I would rather say it seems better not to make this assumption.
This does look somewhat safer than what Amos proposed.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit has been changed. It will be same as virtio-net.
Signed-off-by: Vlad Yasevich vyase...@redhat.com
---
hw/net/e1000.c | 17 -
hw/net/rtl8139.c | 14 +-
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 8387443..a5967ed 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -149,6 +149,10 @@ typedef struct E1000State_st {
#define E1000_FLAG_AUTONEG (1 E1000_FLAG_AUTONEG_BIT)
#define E1000_FLAG_MIT (1 E1000_FLAG_MIT_BIT)
uint32_t compat_flags;
+uint32_t mac_changed;
Hmm why uint32_t? uint8_t is enough here isn't?
This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version (see compat_flags for one way to
detect this compatibility mode).
Hi Michael
I started looking at migrating this thing and I now starting to question
the whole approach.
The only reason to migrate this is if we can migrate between writes to
the mac address registers. We can fairly easily solve the issue of
migrating from net to old versions. The more interesting question
is migrating from old to new versions.
If someone is migrating from an older version (without this feature)
to a newer version and doing so between writes, the bitmap state will
have no idea that a partial write has already happened. The completing
write will just set one of the bits and notifications that we are
looking for do not happen.
-vlad
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Wed, Nov 13, 2013 at 11:21:18AM -0500, Vlad Yasevich wrote:
On 11/10/2013 07:11 AM, Michael S. Tsirkin wrote:
On Fri, Nov 08, 2013 at 02:42:27PM -0500, Vlad Yasevich wrote:
What about this approach? This only updates the monitory when all the
bits have been written to.
-vlad
Thanks!
Some comments below.
-- 8 --
Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
I would rather say it seems better not to make this assumption.
This does look somewhat safer than what Amos proposed.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit has been changed. It will be same as virtio-net.
Signed-off-by: Vlad Yasevich vyase...@redhat.com
---
hw/net/e1000.c | 17 -
hw/net/rtl8139.c | 14 +-
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 8387443..a5967ed 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -149,6 +149,10 @@ typedef struct E1000State_st {
#define E1000_FLAG_AUTONEG (1 E1000_FLAG_AUTONEG_BIT)
#define E1000_FLAG_MIT (1 E1000_FLAG_MIT_BIT)
uint32_t compat_flags;
+uint32_t mac_changed;
Hmm why uint32_t? uint8_t is enough here isn't?
This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version (see compat_flags for one way to
detect this compatibility mode).
Hi Michael
I started looking at migrating this thing and I now starting to question
the whole approach.
The only reason to migrate this is if we can migrate between writes to
the mac address registers.
Absolutely. For some reason below you only discuss cross version
migration but it needs to be migrated for same version migration too.
We can fairly easily solve the issue of
migrating from net to old versions.
I'm not sure it's easier, but it needs to happen anymore.
The more interesting question
is migrating from old to new versions.
If someone is migrating from an older version (without this feature)
to a newer version and doing so between writes, the bitmap state will
have no idea that a partial write has already happened. The completing
write will just set one of the bits and notifications that we are
looking for do not happen.
-vlad
Yep, that's a problem too.
For 1.8 just send the bitmap across.
For cross version migration I would say we should just detect -M 1.7
and older and just emulate old behaviour, disregard the bitmap
completely.
Don't do special tricks just for migration.
--
MST
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On 11/13/2013 03:00 PM, Michael S. Tsirkin wrote:
On Wed, Nov 13, 2013 at 11:21:18AM -0500, Vlad Yasevich wrote:
On 11/10/2013 07:11 AM, Michael S. Tsirkin wrote:
On Fri, Nov 08, 2013 at 02:42:27PM -0500, Vlad Yasevich wrote:
What about this approach? This only updates the monitory when all the
bits have been written to.
-vlad
Thanks!
Some comments below.
-- 8 --
Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
I would rather say it seems better not to make this assumption.
This does look somewhat safer than what Amos proposed.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit has been changed. It will be same as virtio-net.
Signed-off-by: Vlad Yasevich vyase...@redhat.com
---
hw/net/e1000.c | 17 -
hw/net/rtl8139.c | 14 +-
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 8387443..a5967ed 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -149,6 +149,10 @@ typedef struct E1000State_st {
#define E1000_FLAG_AUTONEG (1 E1000_FLAG_AUTONEG_BIT)
#define E1000_FLAG_MIT (1 E1000_FLAG_MIT_BIT)
uint32_t compat_flags;
+uint32_t mac_changed;
Hmm why uint32_t? uint8_t is enough here isn't?
This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version (see compat_flags for one way to
detect this compatibility mode).
Hi Michael
I started looking at migrating this thing and I now starting to question
the whole approach.
The only reason to migrate this is if we can migrate between writes to
the mac address registers.
Absolutely. For some reason below you only discuss cross version
migration but it needs to be migrated for same version migration too.
We can fairly easily solve the issue of
migrating from net to old versions.
I'm not sure it's easier, but it needs to happen anymore.
The more interesting question
is migrating from old to new versions.
If someone is migrating from an older version (without this feature)
to a newer version and doing so between writes, the bitmap state will
have no idea that a partial write has already happened. The completing
write will just set one of the bits and notifications that we are
looking for do not happen.
-vlad
Yep, that's a problem too.
For 1.8 just send the bitmap across.
Right. That's simple enough.
For cross version migration I would say we should just detect -M 1.7
and older and just emulate old behaviour, disregard the bitmap
completely.
Don't do special tricks just for migration.
The compat flags allow for simple migration to 1.7. However, it's the
migration from 1.7 to 1.8 that's the issue.
There is a version number on the E1000 state and I am thinking of maybe
bumping that so that we can catch older state that doesn't support mac
sate change. Thoughts?
-vlad
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Wed, Nov 13, 2013 at 03:26:36PM -0500, Vlad Yasevich wrote:
On 11/13/2013 03:00 PM, Michael S. Tsirkin wrote:
On Wed, Nov 13, 2013 at 11:21:18AM -0500, Vlad Yasevich wrote:
On 11/10/2013 07:11 AM, Michael S. Tsirkin wrote:
On Fri, Nov 08, 2013 at 02:42:27PM -0500, Vlad Yasevich wrote:
What about this approach? This only updates the monitory when all the
bits have been written to.
-vlad
Thanks!
Some comments below.
-- 8 --
Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
I would rather say it seems better not to make this assumption.
This does look somewhat safer than what Amos proposed.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit has been changed. It will be same as virtio-net.
Signed-off-by: Vlad Yasevich vyase...@redhat.com
---
hw/net/e1000.c | 17 -
hw/net/rtl8139.c | 14 +-
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 8387443..a5967ed 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -149,6 +149,10 @@ typedef struct E1000State_st {
#define E1000_FLAG_AUTONEG (1 E1000_FLAG_AUTONEG_BIT)
#define E1000_FLAG_MIT (1 E1000_FLAG_MIT_BIT)
uint32_t compat_flags;
+uint32_t mac_changed;
Hmm why uint32_t? uint8_t is enough here isn't?
This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version (see compat_flags for one way to
detect this compatibility mode).
Hi Michael
I started looking at migrating this thing and I now starting to question
the whole approach.
The only reason to migrate this is if we can migrate between writes to
the mac address registers.
Absolutely. For some reason below you only discuss cross version
migration but it needs to be migrated for same version migration too.
We can fairly easily solve the issue of
migrating from net to old versions.
I'm not sure it's easier, but it needs to happen anymore.
The more interesting question
is migrating from old to new versions.
If someone is migrating from an older version (without this feature)
to a newer version and doing so between writes, the bitmap state will
have no idea that a partial write has already happened. The completing
write will just set one of the bits and notifications that we are
looking for do not happen.
-vlad
Yep, that's a problem too.
For 1.8 just send the bitmap across.
Right. That's simple enough.
For cross version migration I would say we should just detect -M 1.7
and older and just emulate old behaviour, disregard the bitmap
completely.
Don't do special tricks just for migration.
The compat flags allow for simple migration to 1.7. However, it's the
migration from 1.7 to 1.8 that's the issue.
It's an issue because you are trying to migrate to a machine
that behaves differently from 1.7.
If we update mac on last byte write there would not be an issue.
There is a version number on the E1000 state and I am thinking of maybe
bumping that so that we can catch older state that doesn't support mac
sate change. Thoughts?
-vlad
No, we need migration to work cross version if matching -M flag
is used on both sides.
Stop thinking about migration specifically. think about emulation
with -M 1.7 generally. What it should do is
behave same way as 1.7 behaves.
So just implement that: with -M 1.7 only update on
last byte write.
And then migration becomes simple, with no need for
version bumps.
--
MST
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On 11/10/2013 07:11 AM, Michael S. Tsirkin wrote:
On Fri, Nov 08, 2013 at 02:42:27PM -0500, Vlad Yasevich wrote:
What about this approach? This only updates the monitory when all the
bits have been written to.
-vlad
Thanks!
Some comments below.
-- 8 --
Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
I would rather say it seems better not to make this assumption.
This does look somewhat safer than what Amos proposed.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit has been changed. It will be same as virtio-net.
Signed-off-by: Vlad Yasevich vyase...@redhat.com
---
hw/net/e1000.c | 17 -
hw/net/rtl8139.c | 14 +-
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 8387443..a5967ed 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -149,6 +149,10 @@ typedef struct E1000State_st {
#define E1000_FLAG_AUTONEG (1 E1000_FLAG_AUTONEG_BIT)
#define E1000_FLAG_MIT (1 E1000_FLAG_MIT_BIT)
uint32_t compat_flags;
+uint32_t mac_changed;
Hmm why uint32_t? uint8_t is enough here isn't?
Yes, that should be fine. I'll fix and find a better spot to
put it. (may be a hole in the struct).
This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version (see compat_flags for one way to
detect this compatibility mode).
Good point. Didn't think of that...
+#define E1000_RA0_CHANGED 0
+#define E1000_RA1_CHANGED 1
+#define E1000_RA_ALL_CHANGED (E1000_RA0_CHANGED|E1000_RA1_CHANGED)
I don't get it. So E1000_RA_ALL_CHANGED is 0 | 1 == 1.
it looks like the trigger is when E1000_RA1_CHANGED
so that's more or less equivalent.
Goofed on the bits. That should have been (10) and (11).
} E1000State;
#define TYPE_E1000 e1000
@@ -402,6 +406,7 @@ static void e1000_reset(void *opaque)
d-mac_reg[RA + 1] |= (i 2) ? macaddr[i + 4] (8 * i) : 0;
}
qemu_format_nic_info_str(qemu_get_queue(d-nic), macaddr);
+d-mac_changed = 0;
}
static void
@@ -1106,10 +,20 @@ mac_writereg(E1000State *s, int index, uint32_t val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+switch (index) {
+case RA:
+s-mac_changed |= E1000_RA0_CHANGED;
+break;
+case (RA + 1):
+s-mac_changed |= E1000_RA1_CHANGED;
+break;
+}
+
+if (s-mac_changed == E1000_RA_ALL_CHANGED) {
Some whitespace damage here.
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t *)macaddr);
+ s-mac_changed = 0;
Need to use spaces for indent in qemu.
}
}
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
Best to split out in a separate commit.
OK
index 5329f44..6dac10c 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -476,6 +476,8 @@ typedef struct RTL8139State {
uint16_t CpCmd;
uint8_t TxThresh;
+uint8_t mac_changed;
This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version.
+#define RTL8139_MAC_CHANGED_ALL 0x3F
NICState *nic;
NICConf conf;
@@ -1215,6 +1217,7 @@ static void rtl8139_reset(DeviceState *d)
/* restore MAC address */
memcpy(s-phys, s-conf.macaddr.a, 6);
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed = 0;
/* reset interrupt mask */
s-IntrStatus = 0;
@@ -2741,12 +2744,13 @@ static void rtl8139_io_writeb(void *opaque, uint8_t
addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
-qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed |= (1 (addr - MAC0));
Better drop the external () here otherwise it starts looking like lisp :)
Heh. OK.
Thanks
-vlad
+if (s-mac_changed == RTL8139_MAC_CHANGED_ALL) {
+qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed = 0;
+}
break;
case MAC0+6 ... MAC0+7:
/* reserved */
--
1.8.4.2
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On 11/08/2013 10:43 PM, Amos Kong wrote:
On Fri, Nov 08, 2013 at 02:42:27PM -0500, Vlad Yasevich wrote:
What about this approach? This only updates the monitory when all the
bits have been written to.
Hi Vlad,
Looks good to me.
Using this patch, we don't need to care the writing order.
If we add event notify in future, it can reduce the noise
event.
BTW, can we use a tmp buffer to record the new mac (changing is unfinished),
and write the new mac to registers until all bits is written?
I've thought about this as well, but I am not sure what it would buy us
and what's the best way to do it.
At first I thought that we could have a function local static that we
could accumulate into. But then Michael mentioned migration and what
happens if we migrate in middle of the mac change?
So we put into the device, but then it isn't much different then what
we have now with the possible exception that the mac change happens
slightly more atomically. :) For this, it might make more sense to
temporarily stop the link when the mac address change is happening.
-vlad
Amos
-vlad
-- 8 --
Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
Subject: [PATCH] e1000/rtl8139: update HMP NIC when all bits are written
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit has been changed. It will be same as virtio-net.
Signed-off-by: Vlad Yasevich vyase...@redhat.com
---
hw/net/e1000.c | 17 -
hw/net/rtl8139.c | 14 +-
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 8387443..a5967ed 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -149,6 +149,10 @@ typedef struct E1000State_st {
#define E1000_FLAG_AUTONEG (1 E1000_FLAG_AUTONEG_BIT)
#define E1000_FLAG_MIT (1 E1000_FLAG_MIT_BIT)
uint32_t compat_flags;
+uint32_t mac_changed;
+#define E1000_RA0_CHANGED 0
+#define E1000_RA1_CHANGED 1
+#define E1000_RA_ALL_CHANGED (E1000_RA0_CHANGED|E1000_RA1_CHANGED)
} E1000State;
#define TYPE_E1000 e1000
@@ -402,6 +406,7 @@ static void e1000_reset(void *opaque)
d-mac_reg[RA + 1] |= (i 2) ? macaddr[i + 4] (8 * i) : 0;
}
qemu_format_nic_info_str(qemu_get_queue(d-nic), macaddr);
+d-mac_changed = 0;
}
static void
@@ -1106,10 +,20 @@ mac_writereg(E1000State *s, int index, uint32_t val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+switch (index) {
+case RA:
+s-mac_changed |= E1000_RA0_CHANGED;
+break;
+case (RA + 1):
+s-mac_changed |= E1000_RA1_CHANGED;
+break;
+}
+
+if (s-mac_changed == E1000_RA_ALL_CHANGED) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t *)macaddr);
+ s-mac_changed = 0;
}
}
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..6dac10c 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -476,6 +476,8 @@ typedef struct RTL8139State {
uint16_t CpCmd;
uint8_t TxThresh;
+uint8_t mac_changed;
+#define RTL8139_MAC_CHANGED_ALL 0x3F
NICState *nic;
NICConf conf;
@@ -1215,6 +1217,7 @@ static void rtl8139_reset(DeviceState *d)
/* restore MAC address */
memcpy(s-phys, s-conf.macaddr.a, 6);
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed = 0;
/* reset interrupt mask */
s-IntrStatus = 0;
@@ -2741,12 +2744,13 @@ static void rtl8139_io_writeb(void *opaque, uint8_t
addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
-qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed |= (1 (addr - MAC0));
+if (s-mac_changed == RTL8139_MAC_CHANGED_ALL) {
+qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed = 0;
+}
break;
case MAC0+6 ... MAC0+7:
/* reserved */
--
1.8.4.2
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Fri, Nov 08, 2013 at 02:42:27PM -0500, Vlad Yasevich wrote:
What about this approach? This only updates the monitory when all the
bits have been written to.
-vlad
Thanks!
Some comments below.
-- 8 --
Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
I would rather say it seems better not to make this assumption.
This does look somewhat safer than what Amos proposed.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit has been changed. It will be same as virtio-net.
Signed-off-by: Vlad Yasevich vyase...@redhat.com
---
hw/net/e1000.c | 17 -
hw/net/rtl8139.c | 14 +-
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 8387443..a5967ed 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -149,6 +149,10 @@ typedef struct E1000State_st {
#define E1000_FLAG_AUTONEG (1 E1000_FLAG_AUTONEG_BIT)
#define E1000_FLAG_MIT (1 E1000_FLAG_MIT_BIT)
uint32_t compat_flags;
+uint32_t mac_changed;
Hmm why uint32_t? uint8_t is enough here isn't?
This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version (see compat_flags for one way to
detect this compatibility mode).
+#define E1000_RA0_CHANGED 0
+#define E1000_RA1_CHANGED 1
+#define E1000_RA_ALL_CHANGED (E1000_RA0_CHANGED|E1000_RA1_CHANGED)
I don't get it. So E1000_RA_ALL_CHANGED is 0 | 1 == 1.
it looks like the trigger is when E1000_RA1_CHANGED
so that's more or less equivalent.
} E1000State;
#define TYPE_E1000 e1000
@@ -402,6 +406,7 @@ static void e1000_reset(void *opaque)
d-mac_reg[RA + 1] |= (i 2) ? macaddr[i + 4] (8 * i) : 0;
}
qemu_format_nic_info_str(qemu_get_queue(d-nic), macaddr);
+d-mac_changed = 0;
}
static void
@@ -1106,10 +,20 @@ mac_writereg(E1000State *s, int index, uint32_t val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+switch (index) {
+case RA:
+s-mac_changed |= E1000_RA0_CHANGED;
+break;
+case (RA + 1):
+s-mac_changed |= E1000_RA1_CHANGED;
+break;
+}
+
+if (s-mac_changed == E1000_RA_ALL_CHANGED) {
Some whitespace damage here.
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t *)macaddr);
+ s-mac_changed = 0;
Need to use spaces for indent in qemu.
}
}
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
Best to split out in a separate commit.
index 5329f44..6dac10c 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -476,6 +476,8 @@ typedef struct RTL8139State {
uint16_t CpCmd;
uint8_t TxThresh;
+uint8_t mac_changed;
This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version.
+#define RTL8139_MAC_CHANGED_ALL 0x3F
NICState *nic;
NICConf conf;
@@ -1215,6 +1217,7 @@ static void rtl8139_reset(DeviceState *d)
/* restore MAC address */
memcpy(s-phys, s-conf.macaddr.a, 6);
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed = 0;
/* reset interrupt mask */
s-IntrStatus = 0;
@@ -2741,12 +2744,13 @@ static void rtl8139_io_writeb(void *opaque, uint8_t
addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
-qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed |= (1 (addr - MAC0));
Better drop the external () here otherwise it starts looking like lisp :)
+if (s-mac_changed == RTL8139_MAC_CHANGED_ALL) {
+qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed = 0;
+}
break;
case MAC0+6 ... MAC0+7:
/* reserved */
--
1.8.4.2
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Fri, Nov 08, 2013 at 02:42:27PM -0500, Vlad Yasevich wrote:
What about this approach? This only updates the monitory when all the
bits have been written to.
Hi Vlad,
Looks good to me.
Using this patch, we don't need to care the writing order.
If we add event notify in future, it can reduce the noise
event.
BTW, can we use a tmp buffer to record the new mac (changing is unfinished),
and write the new mac to registers until all bits is written?
Amos
-vlad
-- 8 --
Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
Subject: [PATCH] e1000/rtl8139: update HMP NIC when all bits are written
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit has been changed. It will be same as virtio-net.
Signed-off-by: Vlad Yasevich vyase...@redhat.com
---
hw/net/e1000.c | 17 -
hw/net/rtl8139.c | 14 +-
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 8387443..a5967ed 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -149,6 +149,10 @@ typedef struct E1000State_st {
#define E1000_FLAG_AUTONEG (1 E1000_FLAG_AUTONEG_BIT)
#define E1000_FLAG_MIT (1 E1000_FLAG_MIT_BIT)
uint32_t compat_flags;
+uint32_t mac_changed;
+#define E1000_RA0_CHANGED 0
+#define E1000_RA1_CHANGED 1
+#define E1000_RA_ALL_CHANGED (E1000_RA0_CHANGED|E1000_RA1_CHANGED)
} E1000State;
#define TYPE_E1000 e1000
@@ -402,6 +406,7 @@ static void e1000_reset(void *opaque)
d-mac_reg[RA + 1] |= (i 2) ? macaddr[i + 4] (8 * i) : 0;
}
qemu_format_nic_info_str(qemu_get_queue(d-nic), macaddr);
+d-mac_changed = 0;
}
static void
@@ -1106,10 +,20 @@ mac_writereg(E1000State *s, int index, uint32_t val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+switch (index) {
+case RA:
+s-mac_changed |= E1000_RA0_CHANGED;
+break;
+case (RA + 1):
+s-mac_changed |= E1000_RA1_CHANGED;
+break;
+}
+
+if (s-mac_changed == E1000_RA_ALL_CHANGED) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t *)macaddr);
+ s-mac_changed = 0;
}
}
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..6dac10c 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -476,6 +476,8 @@ typedef struct RTL8139State {
uint16_t CpCmd;
uint8_t TxThresh;
+uint8_t mac_changed;
+#define RTL8139_MAC_CHANGED_ALL 0x3F
NICState *nic;
NICConf conf;
@@ -1215,6 +1217,7 @@ static void rtl8139_reset(DeviceState *d)
/* restore MAC address */
memcpy(s-phys, s-conf.macaddr.a, 6);
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed = 0;
/* reset interrupt mask */
s-IntrStatus = 0;
@@ -2741,12 +2744,13 @@ static void rtl8139_io_writeb(void *opaque, uint8_t
addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
-qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed |= (1 (addr - MAC0));
+if (s-mac_changed == RTL8139_MAC_CHANGED_ALL) {
+qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
+s-mac_changed = 0;
+}
break;
case MAC0+6 ... MAC0+7:
/* reserved */
--
1.8.4.2
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Thu, Nov 07, 2013 at 03:32:29PM +0800, Amos Kong wrote:
On Thu, Nov 07, 2013 at 08:59:22AM +0200, Michael S. Tsirkin wrote:
On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
I'm not sure I buy this.
If we actually implement e.g. mac change notifications,
sending them on writes of random bytes will confuse
the host.
This patch just effects the monitor display of macaddr.
During each writing, the macaddr is used for rx-filter is really
changed.
In the real hardware, it supports to just write part of bits,
the rx-filtering is effected by every bit writing.
Yes but again, the window can just be too small to matter
on real hardware.
Our emulation is not perfect, fixing this to be just like real
hardware just might expose other bugs we can't fix
that easily.
I would say let's leave e1000/rtl8139 well alone unless
we see guests that actually write mac without touching
the last byte.
At least, linux rtl8139cp/e1000 writes macaddr from bit 0 to bit 5.
It works to just watch the last bit.
Thanks, Amos
Then think of ways to detect when mac change is done
for these.
---
hw/net/e1000.c | 2 +-
hw/net/rtl8139.c | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ec8ecd7..2d60639 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+if (index == RA || index == RA + 1) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t
*)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..7f2b4db 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque,
uint8_t addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
break;
--
1.8.3.1
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Thu, 2013-11-07 at 12:26 +0200, Michael S. Tsirkin wrote:
On Thu, Nov 07, 2013 at 03:32:29PM +0800, Amos Kong wrote:
On Thu, Nov 07, 2013 at 08:59:22AM +0200, Michael S. Tsirkin wrote:
On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
I'm not sure I buy this.
If we actually implement e.g. mac change notifications,
sending them on writes of random bytes will confuse
the host.
This patch just effects the monitor display of macaddr.
During each writing, the macaddr is used for rx-filter is really
changed.
In the real hardware, it supports to just write part of bits,
the rx-filtering is effected by every bit writing.
Yes but again, the window can just be too small to matter
on real hardware.
Our emulation is not perfect, fixing this to be just like real
hardware just might expose other bugs we can't fix
that easily.
If we were to implement mac change notification, then every partial
update would send a notify and the host. Is that a problem? It seems
no different than if the device had an atomic mac update procedure and
the guest admin changed the mac several times.
The problem with assuming that a given byte is always written last is
that unless the hardware spec identifies an order, we're just basing our
code on examples where we know what the guest driver does, either by
code inspection or access tracing. If there are examples of guests that
write the last byte first, then the host will never know the correct mac
address. Maybe there are no guests that use the wrong order, but that's
a pretty exhaustive search.
The patch doesn't change anything about how the NIC operates, only when
mac changes are updated. During an update the mac is in a transitory
state and we can't go back in time to make it atomic on this hardware
design to avoid a window where the wrong mac may be seen. I think the
patch is the right thing to do. Thanks,
Alex
I would say let's leave e1000/rtl8139 well alone unless
we see guests that actually write mac without touching
the last byte.
At least, linux rtl8139cp/e1000 writes macaddr from bit 0 to bit 5.
It works to just watch the last bit.
Thanks, Amos
Then think of ways to detect when mac change is done
for these.
---
hw/net/e1000.c | 2 +-
hw/net/rtl8139.c | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ec8ecd7..2d60639 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t
val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+if (index == RA || index == RA + 1) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t
*)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..7f2b4db 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque,
uint8_t addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
break;
--
1.8.3.1
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Thu, Nov 07, 2013 at 07:33:57AM -0700, Alex Williamson wrote:
On Thu, 2013-11-07 at 12:26 +0200, Michael S. Tsirkin wrote:
On Thu, Nov 07, 2013 at 03:32:29PM +0800, Amos Kong wrote:
On Thu, Nov 07, 2013 at 08:59:22AM +0200, Michael S. Tsirkin wrote:
On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
I'm not sure I buy this.
If we actually implement e.g. mac change notifications,
sending them on writes of random bytes will confuse
the host.
This patch just effects the monitor display of macaddr.
During each writing, the macaddr is used for rx-filter is really
changed.
In the real hardware, it supports to just write part of bits,
the rx-filtering is effected by every bit writing.
Yes but again, the window can just be too small to matter
on real hardware.
Our emulation is not perfect, fixing this to be just like real
hardware just might expose other bugs we can't fix
that easily.
If we were to implement mac change notification, then every partial
update would send a notify and the host. Is that a problem? It seems
no different than if the device had an atomic mac update procedure and
the guest admin changed the mac several times.
I think modern nics make address updates atomic.
Problem is, we are emulating an ancient one,
so to make host configuration of a modern one
reasonable we need to resort to tricks.
The problem with assuming that a given byte is always written last is
that unless the hardware spec identifies an order, we're just basing our
code on examples where we know what the guest driver does, either by
code inspection or access tracing. If there are examples of guests that
write the last byte first, then the host will never know the correct mac
address. Maybe there are no guests that use the wrong order, but that's
a pretty exhaustive search.
I agree what we have is a hack. Maybe we need some better hacks.
For example, maybe we should update mac at link state change
(I think link is always down when mac is updated?).
Needs some thought.
The patch doesn't change anything about how the NIC operates, only when
mac changes are updated. During an update the mac is in a transitory
state and we can't go back in time to make it atomic on this hardware
design to avoid a window where the wrong mac may be seen. I think the
patch is the right thing to do. Thanks,
Alex
Yes but this info is propaged to host NIC by libvirt so it better
make sense.
I would say let's leave e1000/rtl8139 well alone unless
we see guests that actually write mac without touching
the last byte.
At least, linux rtl8139cp/e1000 writes macaddr from bit 0 to bit 5.
It works to just watch the last bit.
Thanks, Amos
Then think of ways to detect when mac change is done
for these.
---
hw/net/e1000.c | 2 +-
hw/net/rtl8139.c | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ec8ecd7..2d60639 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t
val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+if (index == RA || index == RA + 1) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t
*)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..7f2b4db 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque,
uint8_t addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
qemu_format_nic_info_str(qemu_get_queue(s-nic),
s-phys);
break;
--
1.8.3.1
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On 11/07/2013 10:27 AM, Michael S. Tsirkin wrote:
On Thu, Nov 07, 2013 at 07:33:57AM -0700, Alex Williamson wrote:
On Thu, 2013-11-07 at 12:26 +0200, Michael S. Tsirkin wrote:
On Thu, Nov 07, 2013 at 03:32:29PM +0800, Amos Kong wrote:
On Thu, Nov 07, 2013 at 08:59:22AM +0200, Michael S. Tsirkin wrote:
On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
I'm not sure I buy this.
If we actually implement e.g. mac change notifications,
sending them on writes of random bytes will confuse
the host.
This patch just effects the monitor display of macaddr.
During each writing, the macaddr is used for rx-filter is really
changed.
In the real hardware, it supports to just write part of bits,
the rx-filtering is effected by every bit writing.
Yes but again, the window can just be too small to matter
on real hardware.
Our emulation is not perfect, fixing this to be just like real
hardware just might expose other bugs we can't fix
that easily.
If we were to implement mac change notification, then every partial
update would send a notify and the host. Is that a problem? It seems
no different than if the device had an atomic mac update procedure and
the guest admin changed the mac several times.
I think modern nics make address updates atomic.
Problem is, we are emulating an ancient one,
so to make host configuration of a modern one
reasonable we need to resort to tricks.
The problem with assuming that a given byte is always written last is
that unless the hardware spec identifies an order, we're just basing our
code on examples where we know what the guest driver does, either by
code inspection or access tracing. If there are examples of guests that
write the last byte first, then the host will never know the correct mac
address. Maybe there are no guests that use the wrong order, but that's
a pretty exhaustive search.
I agree what we have is a hack. Maybe we need some better hacks.
For example, maybe we should update mac at link state change
(I think link is always down when mac is updated?).
Needs some thought.
I thought this too, but checking recent linux kernel, e1000 and rtl8139
seem to allow live mac change so link is up.
So here is a stupid, untested patch for e1000 to notify only once:
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 8387443..b99eba4 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -149,6 +149,10 @@ typedef struct E1000State_st {
#define E1000_FLAG_AUTONEG (1 E1000_FLAG_AUTONEG_BIT)
#define E1000_FLAG_MIT (1 E1000_FLAG_MIT_BIT)
uint32_t compat_flags;
+uint32_t mac_change_flags;
+#define E1000_RA0_CHANGED 0
+#define E1000_RA1_CHANGED 1
+#define E1000_RA_ALL_CHANGED (E1000_RA0_CHANGED|E1000_RA1_CHANGED)
} E1000State;
#define TYPE_E1000 e1000
@@ -402,6 +406,7 @@ static void e1000_reset(void *opaque)
d-mac_reg[RA + 1] |= (i 2) ? macaddr[i + 4] (8 * i) : 0;
}
qemu_format_nic_info_str(qemu_get_queue(d-nic), macaddr);
+d-mac_change_flags = 0;
}
static void
@@ -1106,10 +,20 @@ mac_writereg(E1000State *s, int index, uint32_t val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+switch (index) {
+ case RA:
+ s-mac_change_flags |= E1000_RA0_CHANGED;
+ break;
+ case (RA + 1):
+ s-mac_change_flags |= E1000_RA1_CHANGED;
+ break;
+}
+
+if (!(s-mac_change_flags ^ E1000_RA_ALL_CHANGED)) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t
*)macaddr);
+s-mac_change_flags = 0;
}
}
Any thoughts?
-vlad
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On 11/07/2013 09:33 AM, Alex Williamson wrote:
On Thu, 2013-11-07 at 12:26 +0200, Michael S. Tsirkin wrote:
On Thu, Nov 07, 2013 at 03:32:29PM +0800, Amos Kong wrote:
On Thu, Nov 07, 2013 at 08:59:22AM +0200, Michael S. Tsirkin wrote:
On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
I'm not sure I buy this.
If we actually implement e.g. mac change notifications,
sending them on writes of random bytes will confuse
the host.
This patch just effects the monitor display of macaddr.
During each writing, the macaddr is used for rx-filter is really
changed.
In the real hardware, it supports to just write part of bits,
the rx-filtering is effected by every bit writing.
Yes but again, the window can just be too small to matter
on real hardware.
Our emulation is not perfect, fixing this to be just like real
hardware just might expose other bugs we can't fix
that easily.
If we were to implement mac change notification, then every partial
update would send a notify and the host. Is that a problem? It seems
no different than if the device had an atomic mac update procedure and
the guest admin changed the mac several times.
Yes, but the issue is exercebated in the non-atomic case. RTL8139
is the worst since it has byte oriented writes so that for ever mac
change, we have the worst case potential to generate 6 notificates
(assuming libvirt is so fast as to pick up ever change).
Additionally, once libvirt is updated, this would cause rather serious
churn on the host as for ever update, libvirt is going to push the
address down to the physical host nic and the fewer of these updates
we do the better.
The problem with assuming that a given byte is always written last is
that unless the hardware spec identifies an order, we're just basing our
code on examples where we know what the guest driver does, either by
code inspection or access tracing. If there are examples of guests that
write the last byte first, then the host will never know the correct mac
address. Maybe there are no guests that use the wrong order, but that's
a pretty exhaustive search.
The patch doesn't change anything about how the NIC operates, only when
mac changes are updated. During an update the mac is in a transitory
state and we can't go back in time to make it atomic on this hardware
design to avoid a window where the wrong mac may be seen. I think the
patch is the right thing to do. Thanks,
Reporting half-complete state is not the right thing, IMO. Right now,
it's doesn't have much impact, but if we start writing these addresses
to the host nic, then this will have a much bigger impact as I said above.
-vlad
Alex
I would say let's leave e1000/rtl8139 well alone unless
we see guests that actually write mac without touching
the last byte.
At least, linux rtl8139cp/e1000 writes macaddr from bit 0 to bit 5.
It works to just watch the last bit.
Thanks, Amos
Then think of ways to detect when mac change is done
for these.
---
hw/net/e1000.c | 2 +-
hw/net/rtl8139.c | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ec8ecd7..2d60639 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+if (index == RA || index == RA + 1) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t *)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..7f2b4db 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque, uint8_t
addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
break;
--
1.8.3.1
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Tue, 2013-11-05 at 19:17 +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
---
hw/net/e1000.c | 2 +-
hw/net/rtl8139.c | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
Reviewed-by: Alex Williamson alex.william...@redhat.com
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ec8ecd7..2d60639 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+if (index == RA || index == RA + 1) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t *)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..7f2b4db 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque, uint8_t
addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
break;
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
I'm not sure I buy this.
If we actually implement e.g. mac change notifications,
sending them on writes of random bytes will confuse
the host.
I would say let's leave e1000/rtl8139 well alone unless
we see guests that actually write mac without touching
the last byte.
Then think of ways to detect when mac change is done
for these.
---
hw/net/e1000.c | 2 +-
hw/net/rtl8139.c | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ec8ecd7..2d60639 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+if (index == RA || index == RA + 1) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t *)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..7f2b4db 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque, uint8_t
addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
break;
--
1.8.3.1
Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
On Thu, Nov 07, 2013 at 08:59:22AM +0200, Michael S. Tsirkin wrote:
On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
We currently just update the HMP NIC info when the last bit of macaddr
is written. This assumes that guest driver will write all the macaddr
from bit 0 to bit 5 when it changes the macaddr, this is the current
behavior of linux driver (e1000/rtl8139cp), but we can't do this
assumption.
The macaddr that is used for rx-filter will be updated when every bit
is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
info when every bit is changed. It will be same as virtio-net.
Signed-off-by: Amos Kong ak...@redhat.com
I'm not sure I buy this.
If we actually implement e.g. mac change notifications,
sending them on writes of random bytes will confuse
the host.
This patch just effects the monitor display of macaddr.
During each writing, the macaddr is used for rx-filter is really
changed.
In the real hardware, it supports to just write part of bits,
the rx-filtering is effected by every bit writing.
I would say let's leave e1000/rtl8139 well alone unless
we see guests that actually write mac without touching
the last byte.
At least, linux rtl8139cp/e1000 writes macaddr from bit 0 to bit 5.
It works to just watch the last bit.
Thanks, Amos
Then think of ways to detect when mac change is done
for these.
---
hw/net/e1000.c | 2 +-
hw/net/rtl8139.c | 5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ec8ecd7..2d60639 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t val)
s-mac_reg[index] = val;
-if (index == RA + 1) {
+if (index == RA || index == RA + 1) {
macaddr[0] = cpu_to_le32(s-mac_reg[RA]);
macaddr[1] = cpu_to_le32(s-mac_reg[RA + 1]);
qemu_format_nic_info_str(qemu_get_queue(s-nic), (uint8_t
*)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 5329f44..7f2b4db 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,10 +2741,7 @@ static void rtl8139_io_writeb(void *opaque, uint8_t
addr, uint32_t val)
switch (addr)
{
-case MAC0 ... MAC0+4:
-s-phys[addr - MAC0] = val;
-break;
-case MAC0+5:
+case MAC0 ... MAC0+5:
s-phys[addr - MAC0] = val;
qemu_format_nic_info_str(qemu_get_queue(s-nic), s-phys);
break;
--
1.8.3.1
