| Message ID | 1472717449-12501-1-git-send-email-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 1 Sep 2016 10:10:49 +0200 Laurent Vivier <lvivier@redhat.com> 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> > --- > 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; > + > + for (i = 0; i < ETH_ALEN; i++) { > + dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff; Since ETH_ALEN is a constant, this could have been: + dev->nicconf.macaddr.a[ETH_ALEN - 1 - i] = macaddr & 0xff; and spare 1 instruction (at least with GCC 4.8.3/ppc64le) but we don't care for speed here, so: Reviewed-by: Greg Kurz <groug@kaod.org> and tested with both LE and BE guests on a LE host, and the permanent address is restored as expected on reset: Tested-by: Greg Kurz <groug@kaod.org> > + 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); > } >
On 01/09/2016 12:55, Greg Kurz wrote: > On Thu, 1 Sep 2016 10:10:49 +0200 > Laurent Vivier <lvivier@redhat.com> 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> >> --- >> 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; >> + >> + for (i = 0; i < ETH_ALEN; i++) { >> + dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff; > > Since ETH_ALEN is a constant, this could have been: > > + dev->nicconf.macaddr.a[ETH_ALEN - 1 - i] = macaddr & 0xff; > > and spare 1 instruction (at least with GCC 4.8.3/ppc64le) but we don't care > for speed here, so: Thanks, I will take care of that next time. I was guessing the compiler is smart enough to optimize correctly this kind of code. Laurent > Reviewed-by: Greg Kurz <groug@kaod.org> > > and tested with both LE and BE guests on a LE host, and the permanent address > is restored as expected on reset: > > Tested-by: Greg Kurz <groug@kaod.org> > >> + 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); >> } >> >
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 <lvivier@redhat.com> > --- > 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. > + 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? Thomas
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 <lvivier@redhat.com> >> --- >> 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()". >> + 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 :) Thanks, Laurent
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? > 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?
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 >> 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. 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". Thanks, Laurent
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 <lvivier@redhat.com> >>> --- >>> 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
On 02/09/2016 10:02, Thomas Huth wrote: > On 01.09.2016 18:34, Laurent Vivier wrote: >> >> >> On 01/09/2016 15:13, Thomas Huth wrote: ... >>> 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), Right > so that you suddenly end up with the > previous MAC address again after migration. No, the dynamically set mac address is migrated, the one from the command line is only used if a reset occurs. Thanks, Laurent
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.
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; + + 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); }
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> --- hw/net/spapr_llan.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)