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 <lviv...@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
signature.asc
Description: PGP signature
