From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33028) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bgioa-0004kx-UQ for qemu-devel@nongnu.org; Sun, 04 Sep 2016 21:35:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bgioV-0004Dr-Q9 for qemu-devel@nongnu.org; Sun, 04 Sep 2016 21:35:11 -0400 Date: Mon, 5 Sep 2016 11:22:11 +1000 From: David Gibson Message-ID: <20160905012211.GA2587@voom.fritz.box> References: <1472717449-12501-1-git-send-email-lvivier@redhat.com> <20160902023751.GA2990@voom.fritz.box> <03080ff3-a087-dd3f-7cc4-919602e995d3@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="uAKRQypu60I7Lcqm" Content-Disposition: inline In-Reply-To: <03080ff3-a087-dd3f-7cc4-919602e995d3@redhat.com> Subject: Re: [Qemu-devel] [PATCH] spapr: implement H_CHANGE_LOGICAL_LAN_MAC h_call List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org --uAKRQypu60I7Lcqm Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 02, 2016 at 09:09:48AM +0200, Laurent Vivier wrote: >=20 >=20 > 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 > >=20 > > Mostly looks good, but I have a couple of queries. > >=20 > >> --- > >> 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; > >=20 > > 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? >=20 > I didn't see that, I've copied the perm_mac idea from vmxnet3. >=20 > But it's not possible as in qemu_new_nic() nic->conf is &nicconf: >=20 > NICState *qemu_new_nic(NetClientInfo *info, > NICConf *conf, > ... > nic->conf =3D conf; > ... >=20 > static void spapr_vlan_realize(VIOsPAPRDevice *sdev, Error **errp) > ... > dev->nic =3D qemu_new_nic(&net_spapr_vlan_info, &dev->nicconf, > ... >=20 > So "dev->nic->conf" =3D=3D &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.m= acaddr.a); > >> } > >> =20 > >> static void spapr_vlan_realize(VIOsPAPRDevice *sdev, Error **errp) > >> @@ -324,6 +329,8 @@ static void spapr_vlan_realize(VIOsPAPRDevice *sde= v, Error **errp) > >> =20 > >> qemu_macaddr_default_if_unset(&dev->nicconf.macaddr); > >> =20 > >> + memcpy(&dev->perm_mac.a, &dev->nicconf.macaddr.a, sizeof(dev->per= m_mac.a)); > >> + > >> dev->nic =3D 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.m= acaddr.a); > >> @@ -756,6 +763,27 @@ static target_ulong h_multicast_ctrl(PowerPCCPU *= cpu, sPAPRMachineState *spapr, > >> return H_SUCCESS; > >> } > >> =20 > >> +static target_ulong h_change_logical_lan_mac(PowerPCCPU *cpu, > >> + sPAPRMachineState *spapr, > >> + target_ulong opcode, > >> + target_ulong *args) > >> +{ > >> + target_ulong reg =3D args[0]; > >> + target_ulong macaddr =3D args[1]; > >> + VIOsPAPRDevice *sdev =3D spapr_vio_find_by_reg(spapr->vio_bus, re= g); > >> + VIOsPAPRVLANDevice *dev =3D VIO_SPAPR_VLAN_DEVICE(sdev); > >> + int i; > >> + > >> + for (i =3D 0; i < ETH_ALEN; i++) { > >> + dev->nicconf.macaddr.a[ETH_ALEN - i - 1] =3D macaddr & 0xff; > >> + macaddr >>=3D 8; > >> + } > >> + > >> + qemu_format_nic_info_str(qemu_get_queue(dev->nic), dev->nicconf.m= acaddr.a); > >> + > >> + return H_SUCCESS; > >> +} > >> + > >> static Property spapr_vlan_properties[] =3D { > >> 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); > >> } > >=20 > > 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? >=20 > 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. --=20 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 --uAKRQypu60I7Lcqm Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJXzMjDAAoJEGw4ysog2bOSYdcP/AostcrhtBeawHJyjsRGH89A oII4yFy04Cw8J/05JFTpgVUQuMtzriE8flvAPanudx/PpUq+i1RNLLnFjyGq5DXK 1XBGUL02QuksYSur4eLKm/x0TGE/FMhbqVJDmggrD+GnOynOK8T9z6Zsyb0DEiIn VFMHoSw5y62/XIdXdHHoIdzdYdbHgR55/+7ugUipNLLPz/oOVZkSXUJPGH9fr7P/ eINOww0AhXHU5WaDs5ak72hYIFJOBOZpbfP/ay0/Tb6P8S9qTSdUnyOsRTIHR2lP 8ABapr6xR+JnUkunNB7WHGVPer+qDItyLnmtYk/8GOkuW5ACNrgTOvUqSTB8eDR5 r/H/AO3nR8QUV8UmmUa9hkTbRF6l6qhFwT6akZC0hFsDnIRPRF5Lj0aqidEvjZKb LHwcMMWlO0dIkaIHON0rFmR1sCzmiohtlqBwXu0QkPmtaSjmzmAFryqgTtSo6x1A ENCExFcvIGw7Dfv7NJ1dnLPN11zo8IJ7ry9LVkdi/VflisuhRe5uZ0HzduT5XmA3 NSppE8DreMlaWlZbcEEtE3mmpcjiv7n4492cfQExU9LtM6xLWBn7um/00n3QEPGl snLhLYGAChEs+86d1EbrnwvkWYkgI8GCXGGWk6ARJFFWZD0XuAUMwSE41Em6cUno GFZnHeEOaB5d3zcHAmPq =0Olh -----END PGP SIGNATURE----- --uAKRQypu60I7Lcqm--