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

[David Gibson]

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?

>      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?

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature