Re: [Qemu-ppc] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call

[Laurent Vivier]

On 02/09/2016 04:37, David Gibson wrote:
> On Thu, Sep 01, 2016 at 10:10:49AM +0200, Laurent Vivier wrote:
>> Since kernel v4.0, linux uses H_CHANGE_LOGICAL_LAN_MAC to change lively
>> the MAC address of an ibmveth interface.
>>
>> As QEMU doesn't implement this h_call, we can't change anymore the
>> MAC address of an spapr-vlan interface.
>>
>> Signed-off-by: Laurent Vivier <address@hidden>
> 
> Mostly looks good, but I have a couple of queries.
> 
>> ---
>>  hw/net/spapr_llan.c | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
>> index b273eda..4bb95a5 100644
>> --- a/hw/net/spapr_llan.c
>> +++ b/hw/net/spapr_llan.c
>> @@ -106,6 +106,7 @@ typedef struct VIOsPAPRVLANDevice {
>>      VIOsPAPRDevice sdev;
>>      NICConf nicconf;
>>      NICState *nic;
>> +    MACAddr perm_mac;
> 
> Looking at virtio-net, I see that it copies the mac address from
> nic->conf on reset.  Could we do that, to avoid creating an extra
> field in the state?

I didn't see that, I've copied the perm_mac idea from vmxnet3.

But it's not possible as in qemu_new_nic() nic->conf is &nicconf:

NICState *qemu_new_nic(NetClientInfo *info,
                       NICConf *conf,
...
    nic->conf = conf;
...

static void spapr_vlan_realize(VIOsPAPRDevice *sdev, Error **errp)
...
    dev->nic = qemu_new_nic(&net_spapr_vlan_info, &dev->nicconf,
...

So "dev->nic->conf" == &dev->nicconf


>>      bool isopen;
>>      hwaddr buf_list;
>>      uint32_t add_buf_ptr, use_buf_ptr, rx_bufs;
>> @@ -316,6 +317,10 @@ static void spapr_vlan_reset(VIOsPAPRDevice *sdev)
>>              spapr_vlan_reset_rx_pool(dev->rx_pool[i]);
>>          }
>>      }
>> +
>> +    memcpy(&dev->nicconf.macaddr.a, &dev->perm_mac.a,
>> +           sizeof(dev->nicconf.macaddr.a));
>> +    qemu_format_nic_info_str(qemu_get_queue(dev->nic), 
>> dev->nicconf.macaddr.a);
>>  }
>>  
>>  static void spapr_vlan_realize(VIOsPAPRDevice *sdev, Error **errp)
>> @@ -324,6 +329,8 @@ static void spapr_vlan_realize(VIOsPAPRDevice *sdev, 
>> Error **errp)
>>  
>>      qemu_macaddr_default_if_unset(&dev->nicconf.macaddr);
>>  
>> +    memcpy(&dev->perm_mac.a, &dev->nicconf.macaddr.a, 
>> sizeof(dev->perm_mac.a));
>> +
>>      dev->nic = qemu_new_nic(&net_spapr_vlan_info, &dev->nicconf,
>>                              object_get_typename(OBJECT(sdev)), 
>> sdev->qdev.id, dev);
>>      qemu_format_nic_info_str(qemu_get_queue(dev->nic), 
>> dev->nicconf.macaddr.a);
>> @@ -756,6 +763,27 @@ static target_ulong h_multicast_ctrl(PowerPCCPU *cpu, 
>> sPAPRMachineState *spapr,
>>      return H_SUCCESS;
>>  }
>>  
>> +static target_ulong h_change_logical_lan_mac(PowerPCCPU *cpu,
>> +                                             sPAPRMachineState *spapr,
>> +                                             target_ulong opcode,
>> +                                             target_ulong *args)
>> +{
>> +    target_ulong reg = args[0];
>> +    target_ulong macaddr = args[1];
>> +    VIOsPAPRDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
>> +    VIOsPAPRVLANDevice *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
>> +    int i;
>> +
>> +    for (i = 0; i < ETH_ALEN; i++) {
>> +        dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
>> +        macaddr >>= 8;
>> +    }
>> +
>> +    qemu_format_nic_info_str(qemu_get_queue(dev->nic), 
>> dev->nicconf.macaddr.a);
>> +
>> +    return H_SUCCESS;
>> +}
>> +
>>  static Property spapr_vlan_properties[] = {
>>      DEFINE_SPAPR_PROPERTIES(VIOsPAPRVLANDevice, sdev),
>>      DEFINE_NIC_PROPERTIES(VIOsPAPRVLANDevice, nicconf),
>> @@ -854,6 +882,8 @@ static void spapr_vlan_register_types(void)
>>      spapr_register_hypercall(H_ADD_LOGICAL_LAN_BUFFER,
>>                               h_add_logical_lan_buffer);
>>      spapr_register_hypercall(H_MULTICAST_CTRL, h_multicast_ctrl);
>> +    spapr_register_hypercall(H_CHANGE_LOGICAL_LAN_MAC,
>> +                             h_change_logical_lan_mac);
>>      type_register_static(&spapr_vlan_info);
>>  }
> 
> Now that the MAC is guest changeable, do we need to add something to
> let it be migrated?  Or is that already included in the migration
> state for the base NIC info?

As I said to Thomas, perm_mac is initialized from the command line and
thus does not need to be migrated, and nicconf.macaddr (because of
nic->conf pointer) is already migrated by the networking part.

I've tested migration (again, with LE guest and host only) while a ping
is running, and the dynamic macaddress is migrated correctly, and on
reset (after and before migration) the macaddress is correctly reset.
I've checked the macaddress on the host using "arp -a".

Thanks,
Laurent