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

[Thomas Huth]

On 01.09.2016 18:34, Laurent Vivier wrote:
> 
> 
> On 01/09/2016 15:13, Thomas Huth wrote:
>> On 01.09.2016 10:10, 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>
>>> ---
>>>  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;
>>
>> As an additional sanity check, you could test whether the MAC address is
>> a proper unicast address, i.e. that the broadcast bit is not set.
> 
> According to LoPAPR_DRAFT_v11_24March2016_cmt.pdf,
>  "16.4.3.7 H_CHANGE_LOGICAL_LAN_MAC":
> ...
> Semantics:
> * Validates the unit address, else H_Parameter
> * Records the low order 6 bytes of mac-address for filtering future
> incoming messages
> * Returns H_Success
> 
> So H_PARAMETER is only for "reg" and we don't have to check mac-address.
> 
> Anyway, the address is checked by the kernel, with "is_valid_ether_addr()".

Ok, should be fine then.

>>> +    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);
>>>  }
>>
>> Patch looks basically fine to me. One more thought though:
>> What about migration? Don't we need to migrate the perm_mac array, too,
>> or is this already covered by the dev->nicconf.macaddr array?
> 
> We don't need to migrate perm_mac because it is initialized from the
> command line (it is only used to reset the card) on the destination side
> as it is on the source side.
> 
> I've tested migration and all seems to work fine, but if you want to
> double-check don't hesitate :)

My concern was that the MAC address on the command line on the
destination side is rather set to the old, original MAC address by the
management tools (libvirt), so that you suddenly end up with the
previous MAC address again after migration. But if you say that you've
tested migration already, we should be fine!

 Thomas