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

[Laurent Vivier]

On 01/09/2016 12:55, Greg Kurz wrote:
> On Thu,  1 Sep 2016 10:10:49 +0200
> Laurent Vivier <address@hidden> 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>
>> ---
>>  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;
>>      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;
> 
> Since ETH_ALEN is a constant, this could have been:
> 
> +        dev->nicconf.macaddr.a[ETH_ALEN - 1 - i] = macaddr & 0xff;
> 
> and spare 1 instruction (at least with GCC 4.8.3/ppc64le) but we don't care
> for speed here, so:

Thanks, I will take care of that next time.

I was guessing the compiler is smart enough to optimize correctly this
kind of code.

Laurent
> Reviewed-by: Greg Kurz <address@hidden>
> 
> and tested with both LE and BE guests on a LE host, and the permanent address
> is restored as expected on reset:
> 
> Tested-by: Greg Kurz <address@hidden>
> 
>> +        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);
>>  }
>>  
>