Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written

[Michael S. Tsirkin]

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 <address@hidden>
> >>---
> >>  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