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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5488 bytes --]

On Fri, Sep 02, 2016 at 09:09:48AM +0200, Laurent Vivier wrote:
> 
> 
> 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 <lvivier@redhat.com>
> > 
> > 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

Ah, yes, I see.  I think the virtio-net approach is a little cleaner,
but it's not worth reorganizing the driver just for that.

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

Ah, good.

> 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".

Great, thanks.

I've merged this into ppc-for-2.8.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]