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 <[email protected]> > > > > 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
signature.asc
Description: PGP signature
