* Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call
2016-09-01 8:10 [Qemu-devel] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call Laurent Vivier
@ 2016-09-01 10:55 ` Greg Kurz
2016-09-01 11:09 ` Laurent Vivier
2016-09-01 13:13 ` [Qemu-devel] " Thomas Huth
2016-09-02 2:37 ` David Gibson
2 siblings, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2016-09-01 10:55 UTC (permalink / raw)
To: Laurent Vivier; +Cc: david, qemu-ppc, qemu-devel
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);
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call
2016-09-01 10:55 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
@ 2016-09-01 11:09 ` Laurent Vivier
0 siblings, 0 replies; 10+ messages in thread
From: Laurent Vivier @ 2016-09-01 11:09 UTC (permalink / raw)
To: Greg Kurz; +Cc: david, qemu-ppc, qemu-devel
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);
>> }
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call
2016-09-01 8:10 [Qemu-devel] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call Laurent Vivier
2016-09-01 10:55 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
@ 2016-09-01 13:13 ` Thomas Huth
2016-09-01 16:34 ` Laurent Vivier
2016-09-02 2:37 ` David Gibson
2 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2016-09-01 13:13 UTC (permalink / raw)
To: qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call
2016-09-01 13:13 ` [Qemu-devel] " Thomas Huth
@ 2016-09-01 16:34 ` Laurent Vivier
2016-09-02 8:02 ` Thomas Huth
0 siblings, 1 reply; 10+ messages in thread
From: Laurent Vivier @ 2016-09-01 16:34 UTC (permalink / raw)
To: Thomas Huth, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call
2016-09-01 16:34 ` Laurent Vivier
@ 2016-09-02 8:02 ` Thomas Huth
2016-09-02 8:06 ` Laurent Vivier
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2016-09-02 8:02 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call
2016-09-02 8:02 ` Thomas Huth
@ 2016-09-02 8:06 ` Laurent Vivier
0 siblings, 0 replies; 10+ messages in thread
From: Laurent Vivier @ 2016-09-02 8:06 UTC (permalink / raw)
To: Thomas Huth, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call
2016-09-01 8:10 [Qemu-devel] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call Laurent Vivier
2016-09-01 10:55 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2016-09-01 13:13 ` [Qemu-devel] " Thomas Huth
@ 2016-09-02 2:37 ` David Gibson
2016-09-02 7:09 ` Laurent Vivier
2 siblings, 1 reply; 10+ messages in thread
From: David Gibson @ 2016-09-02 2:37 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3919 bytes --]
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?
--
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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call
2016-09-02 2:37 ` David Gibson
@ 2016-09-02 7:09 ` Laurent Vivier
2016-09-05 1:22 ` David Gibson
0 siblings, 1 reply; 10+ messages in thread
From: Laurent Vivier @ 2016-09-02 7:09 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call
2016-09-02 7:09 ` Laurent Vivier
@ 2016-09-05 1:22 ` David Gibson
0 siblings, 0 replies; 10+ messages in thread
From: David Gibson @ 2016-09-05 1:22 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-ppc, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread