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 >>> >>> 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 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 > > > > 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 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 > > 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 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 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 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 > --- > 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/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 --- 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 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 > >>--- > >> 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/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 --- 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 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 --- 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 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 --- 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 (1<<0) and (1<<1). } 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 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 > --- > 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 > --- > 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
[Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
What about this approach? This only updates the monitory when all the bits have been written to. -vlad -- >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. 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 --- 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 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 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 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 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 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 > > > > > > > > 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; > >
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 > > > > > > 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 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 > > > > 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, 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 > > 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
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 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 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 > --- > hw/net/e1000.c | 2 +- > hw/net/rtl8139.c | 5 + > 2 files changed, 2 insertions(+), 5 deletions(-) Reviewed-by: Alex Williamson > > 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;
[Qemu-devel] [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. 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 --- 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