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

[Michael S. Tsirkin]

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 <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.
> 
> 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