Re: [Qemu-devel] [PATCH v4 2/7] e1000: Add support for migrating the entire MAC registers' array

[Jason Wang]

On 11/03/2015 07:14 PM, Leonid Bloch wrote:
> This patch enables the migration of the entire array of MAC registers
> during live migration. The entire array is just 128 KB long, so
> practically no penalty should be felt when transmitting it, over the
> individual registers. But the advantages are more compact code, and no
> need to introduce new vmstate subsections in the future, when additional
> MAC registers will be implemented.
>
> Backward compatibility is preserved by introducing the e1000-specific
> boolean parameter "full_mac_registers", which is on by default. Setting
> it to off will enable migration to older versions of QEMU.
>
> Additionally, this parameter can be used to control the implementation of
> extra MAC registers in the future. I.e. new MAC registers may be set to
> behave differently, or not to be active at all, if "full_mac_registers"
> will be set to "off", e.g.:
>
>     qemu-system-x86_64 -device e1000,full_mac_registers=off,... ...
>
> As mentioned above, the default value is "on".
>
> Signed-off-by: Leonid Bloch <address@hidden>
> Signed-off-by: Dmitry Fleytman <address@hidden>
> ---
>  hw/net/e1000.c | 105 
> +++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 72 insertions(+), 33 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 7036842..1190bbe 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -135,8 +135,10 @@ typedef struct E1000State_st {
>  /* Compatibility flags for migration to/from qemu 1.3.0 and older */
>  #define E1000_FLAG_AUTONEG_BIT 0
>  #define E1000_FLAG_MIT_BIT 1
> +#define E1000_FLAG_MAC_BIT 2
>  #define E1000_FLAG_AUTONEG (1 << E1000_FLAG_AUTONEG_BIT)
>  #define E1000_FLAG_MIT (1 << E1000_FLAG_MIT_BIT)
> +#define E1000_FLAG_MAC (1 << E1000_FLAG_MAC_BIT)
>      uint32_t compat_flags;
>  } E1000State;
>  
> @@ -1377,6 +1379,18 @@ static bool e1000_mit_state_needed(void *opaque)
>      return s->compat_flags & E1000_FLAG_MIT;
>  }
>  
> +static bool e1000_full_mac_needed(void *opaque)
> +{
> +    E1000State *s = opaque;
> +
> +    return s->compat_flags & E1000_FLAG_MAC;
> +}
> +
> +static bool e1000_compat_mac_needed(void *opaque)
> +{
> +    return !e1000_full_mac_needed(opaque);
> +}
> +
>  static const VMStateDescription vmstate_e1000_mit_state = {
>      .name = "e1000/mit_state",
>      .version_id = 1,
> @@ -1392,41 +1406,23 @@ static const VMStateDescription 
> vmstate_e1000_mit_state = {
>      }
>  };
>  
> -static const VMStateDescription vmstate_e1000 = {
> -    .name = "e1000",
> -    .version_id = 2,
> +static const VMStateDescription vmstate_e1000_full_mac_state = {
> +    .name = "e1000/full_mac_state",
> +    .version_id = 1,
>      .minimum_version_id = 1,
> -    .pre_save = e1000_pre_save,
> -    .post_load = e1000_post_load,
> +    .needed = e1000_full_mac_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(mac_reg, E1000State, 0x8000),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_e1000_compat_mac_state = {
> +    .name = "e1000/compat_mac_state",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = e1000_compat_mac_needed,
>      .fields = (VMStateField[]) {
> -        VMSTATE_PCI_DEVICE(parent_obj, E1000State),
> -        VMSTATE_UNUSED_TEST(is_version_1, 4), /* was instance id */
> -        VMSTATE_UNUSED(4), /* Was mmio_base.  */
> -        VMSTATE_UINT32(rxbuf_size, E1000State),
> -        VMSTATE_UINT32(rxbuf_min_shift, E1000State),
> -        VMSTATE_UINT32(eecd_state.val_in, E1000State),
> -        VMSTATE_UINT16(eecd_state.bitnum_in, E1000State),
> -        VMSTATE_UINT16(eecd_state.bitnum_out, E1000State),
> -        VMSTATE_UINT16(eecd_state.reading, E1000State),
> -        VMSTATE_UINT32(eecd_state.old_eecd, E1000State),
> -        VMSTATE_UINT8(tx.ipcss, E1000State),
> -        VMSTATE_UINT8(tx.ipcso, E1000State),
> -        VMSTATE_UINT16(tx.ipcse, E1000State),
> -        VMSTATE_UINT8(tx.tucss, E1000State),
> -        VMSTATE_UINT8(tx.tucso, E1000State),
> -        VMSTATE_UINT16(tx.tucse, E1000State),
> -        VMSTATE_UINT32(tx.paylen, E1000State),
> -        VMSTATE_UINT8(tx.hdr_len, E1000State),
> -        VMSTATE_UINT16(tx.mss, E1000State),
> -        VMSTATE_UINT16(tx.size, E1000State),
> -        VMSTATE_UINT16(tx.tso_frames, E1000State),
> -        VMSTATE_UINT8(tx.sum_needed, E1000State),
> -        VMSTATE_INT8(tx.ip, E1000State),
> -        VMSTATE_INT8(tx.tcp, E1000State),
> -        VMSTATE_BUFFER(tx.header, E1000State),
> -        VMSTATE_BUFFER(tx.data, E1000State),
> -        VMSTATE_UINT16_ARRAY(eeprom_data, E1000State, 64),
> -        VMSTATE_UINT16_ARRAY(phy_reg, E1000State, 0x20),
>          VMSTATE_UINT32(mac_reg[CTRL], E1000State),
>          VMSTATE_UINT32(mac_reg[EECD], E1000State),
>          VMSTATE_UINT32(mac_reg[EERD], E1000State),
> @@ -1468,9 +1464,50 @@ static const VMStateDescription vmstate_e1000 = {
>          VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, MTA, 128),
>          VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, VFTA, 128),
>          VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_e1000 = {
> +    .name = "e1000",
> +    .version_id = 2,
> +    .minimum_version_id = 1,
> +    .pre_save = e1000_pre_save,
> +    .post_load = e1000_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCI_DEVICE(parent_obj, E1000State),
> +        VMSTATE_UNUSED_TEST(is_version_1, 4), /* was instance id */
> +        VMSTATE_UNUSED(4), /* Was mmio_base.  */
> +        VMSTATE_UINT32(rxbuf_size, E1000State),
> +        VMSTATE_UINT32(rxbuf_min_shift, E1000State),
> +        VMSTATE_UINT32(eecd_state.val_in, E1000State),
> +        VMSTATE_UINT16(eecd_state.bitnum_in, E1000State),
> +        VMSTATE_UINT16(eecd_state.bitnum_out, E1000State),
> +        VMSTATE_UINT16(eecd_state.reading, E1000State),
> +        VMSTATE_UINT32(eecd_state.old_eecd, E1000State),
> +        VMSTATE_UINT8(tx.ipcss, E1000State),
> +        VMSTATE_UINT8(tx.ipcso, E1000State),
> +        VMSTATE_UINT16(tx.ipcse, E1000State),
> +        VMSTATE_UINT8(tx.tucss, E1000State),
> +        VMSTATE_UINT8(tx.tucso, E1000State),
> +        VMSTATE_UINT16(tx.tucse, E1000State),
> +        VMSTATE_UINT32(tx.paylen, E1000State),
> +        VMSTATE_UINT8(tx.hdr_len, E1000State),
> +        VMSTATE_UINT16(tx.mss, E1000State),
> +        VMSTATE_UINT16(tx.size, E1000State),
> +        VMSTATE_UINT16(tx.tso_frames, E1000State),
> +        VMSTATE_UINT8(tx.sum_needed, E1000State),
> +        VMSTATE_INT8(tx.ip, E1000State),
> +        VMSTATE_INT8(tx.tcp, E1000State),
> +        VMSTATE_BUFFER(tx.header, E1000State),
> +        VMSTATE_BUFFER(tx.data, E1000State),
> +        VMSTATE_UINT16_ARRAY(eeprom_data, E1000State, 64),
> +        VMSTATE_UINT16_ARRAY(phy_reg, E1000State, 0x20),
> +        VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {
>          &vmstate_e1000_mit_state,
> +        &vmstate_e1000_full_mac_state,
> +        &vmstate_e1000_compat_mac_state,
>          NULL
>      }

I'm afraid this will break migration to older qemu. Consider the case
when full_mac_registers=off, we save compat mac registers as a
subsection, this breaks the assumption on load who expect all compact
registers just after phy_reg. (You can just test this with a simple
'savevm' with full_mac_register_off and the revert this patch and do a
'loadvm'), Looks like the safe way is to just add a
vmstate_e1000_full_mac_state and keep other fields of vmstate_e1000
unmodified.

>  };
> @@ -1603,6 +1640,8 @@ static Property e1000_properties[] = {
>                      compat_flags, E1000_FLAG_AUTONEG_BIT, true),
>      DEFINE_PROP_BIT("mitigation", E1000State,
>                      compat_flags, E1000_FLAG_MIT_BIT, true),
> +    DEFINE_PROP_BIT("full_mac_registers", E1000State,
> +                    compat_flags, E1000_FLAG_MAC_BIT, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  

Need also turn this off for pre 2.5 machines in HW_COMPAT_2_4.