Re: [Qemu-devel] [RFC Patch 06/12] IXGBEVF: Add self emulation layer

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Oct 21, 2015 at 01:58:19PM -0700, Alexander Duyck wrote:
> On 10/21/2015 09:37 AM, Lan Tianyu wrote:
> >In order to restore VF function after migration, add self emulation layer
> >to record regs' values during accessing regs.
> >
> >Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> >---
> >  drivers/net/ethernet/intel/ixgbevf/Makefile        |  3 ++-
> >  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c  |  2 +-
> >  .../net/ethernet/intel/ixgbevf/self-emulation.c    | 26 ++++++++++++++++++++++
> >  drivers/net/ethernet/intel/ixgbevf/vf.h            |  5 ++++-
> >  4 files changed, 33 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/net/ethernet/intel/ixgbevf/self-emulation.c
> >
> >diff --git a/drivers/net/ethernet/intel/ixgbevf/Makefile b/drivers/net/ethernet/intel/ixgbevf/Makefile
> >index 4ce4c97..841c884 100644
> >--- a/drivers/net/ethernet/intel/ixgbevf/Makefile
> >+++ b/drivers/net/ethernet/intel/ixgbevf/Makefile
> >@@ -31,7 +31,8 @@
> >  obj-$(CONFIG_IXGBEVF) += ixgbevf.o
> >-ixgbevf-objs := vf.o \
> >+ixgbevf-objs := self-emulation.o \
> >+		vf.o \
> >                  mbx.o \
> >                  ethtool.o \
> >                  ixgbevf_main.o
> >diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> >index a16d267..4446916 100644
> >--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> >+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> >@@ -156,7 +156,7 @@ u32 ixgbevf_read_reg(struct ixgbe_hw *hw, u32 reg)
> >  	if (IXGBE_REMOVED(reg_addr))
> >  		return IXGBE_FAILED_READ_REG;
> >-	value = readl(reg_addr + reg);
> >+	value = ixgbe_self_emul_readl(reg_addr, reg);
> >  	if (unlikely(value == IXGBE_FAILED_READ_REG))
> >  		ixgbevf_check_remove(hw, reg);
> >  	return value;
> >diff --git a/drivers/net/ethernet/intel/ixgbevf/self-emulation.c b/drivers/net/ethernet/intel/ixgbevf/self-emulation.c
> >new file mode 100644
> >index 0000000..d74b2da
> >--- /dev/null
> >+++ b/drivers/net/ethernet/intel/ixgbevf/self-emulation.c
> >@@ -0,0 +1,26 @@
> >+#include <linux/netdevice.h>
> >+#include <linux/pci.h>
> >+#include <linux/delay.h>
> >+#include <linux/interrupt.h>
> >+#include <net/arp.h>
> >+
> >+#include "vf.h"
> >+#include "ixgbevf.h"
> >+
> >+static u32 hw_regs[0x4000];
> >+
> >+u32 ixgbe_self_emul_readl(volatile void __iomem *base, u32 addr)
> >+{
> >+	u32 tmp;
> >+
> >+	tmp = readl(base + addr);
> >+	hw_regs[(unsigned long)addr] = tmp;
> >+
> >+	return tmp;
> >+}
> >+
> >+void ixgbe_self_emul_writel(u32 val, volatile void __iomem *base, u32  addr)
> >+{
> >+	hw_regs[(unsigned long)addr] = val;
> >+	writel(val, (volatile void __iomem *)(base + addr));
> >+}
> 
> So I see what you are doing, however I don't think this adds much value.
> Many of the key registers for the device are not simple Read/Write
> registers.  Most of them are things like write 1 to clear or some other sort
> of value where writing doesn't set the bit but has some other side effect.
> Just take a look through the Datasheet at registers such as the VFCTRL,
> VFMAILBOX, or most of the interrupt registers.  The fact is simply storing
> the values off doesn't give you any real idea of what the state of things
> are.

It doesn't, but I guess the point is to isolate the migration-related logic
in the recovery code.

An alternative would be to have some smart logic all over the place to
only store what's required - that would be much more intrusive.


> >diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.h b/drivers/net/ethernet/intel/ixgbevf/vf.h
> >index d40f036..6a3f4eb 100644
> >--- a/drivers/net/ethernet/intel/ixgbevf/vf.h
> >+++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
> >@@ -39,6 +39,9 @@
> >  struct ixgbe_hw;
> >+u32 ixgbe_self_emul_readl(volatile void __iomem *base, u32 addr);
> >+void ixgbe_self_emul_writel(u32 val, volatile void __iomem *base, u32  addr);
> >+
> >  /* iterator type for walking multicast address lists */
> >  typedef u8* (*ixgbe_mc_addr_itr) (struct ixgbe_hw *hw, u8 **mc_addr_ptr,
> >  				  u32 *vmdq);
> >@@ -182,7 +185,7 @@ static inline void ixgbe_write_reg(struct ixgbe_hw *hw, u32 reg, u32 value)
> >  	if (IXGBE_REMOVED(reg_addr))
> >  		return;
> >-	writel(value, reg_addr + reg);
> >+	ixgbe_self_emul_writel(value, reg_addr, reg);
> >  }
> >  #define IXGBE_WRITE_REG(h, r, v) ixgbe_write_reg(h, r, v)
>