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

[Leonid Bloch]

On Wed, Nov 4, 2015 at 4:35 AM, Jason Wang <address@hidden> wrote:
>
>
> 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.

You are right! Thanks.
>
>>  };
>> @@ -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.

Thanks for reminding!
BTW, HW_COMPAT is simply the new version of PC_COMPAT, or am I wrong?

Leonid.