Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <mcoqu...@redhat.com> > Sent: Thursday, June 3, 2021 10:29 PM > To: Xia, Chenbo <chenbo....@intel.com>; Maxime Coquelin > <maxime.coque...@redhat.com>; dev@dpdk.org; amore...@redhat.com; > david.march...@redhat.com > Subject: Re: [RFC 3/3] net/virtio: add MAC device config getter and setter > > Hi Chenbo, > > On 4/19/21 8:24 AM, Xia, Chenbo wrote: > > Hi Maxime, > > > >> -----Original Message----- > >> From: Maxime Coquelin <maxime.coque...@redhat.com> > >> Sent: Friday, March 19, 2021 6:35 AM > >> To: dev@dpdk.org; Xia, Chenbo <chenbo....@intel.com>; amore...@redhat.com; > >> david.march...@redhat.com > >> Cc: Maxime Coquelin <maxime.coque...@redhat.com> > >> Subject: [RFC 3/3] net/virtio: add MAC device config getter and setter > >> > >> This patch uses the new device config ops to get and set > >> the MAC address if supported. > >> > >> If a valid MAC address is passed as devarg of the > >> Virtio-user PMD, the driver will try to store it in the > >> device config space. Otherwise the one provided in > >> the device config space will be used, if available. > > > > I agree with the MAC selection strategy you proposed. > > > >> > >> Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com> > >> --- > >> .../net/virtio/virtio_user/virtio_user_dev.c | 78 ++++++++++++++++--- > >> .../net/virtio/virtio_user/virtio_user_dev.h | 2 + > >> drivers/net/virtio/virtio_user_ethdev.c | 7 +- > >> 3 files changed, 74 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> b/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> index 8757a23f6e..61517692b3 100644 > >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c > >> @@ -259,20 +259,76 @@ int virtio_user_stop_device(struct virtio_user_dev > *dev) > >> return -1; > >> } > >> > >> -static inline void > >> -parse_mac(struct virtio_user_dev *dev, const char *mac) > >> +int > >> +virtio_user_dev_set_mac(struct virtio_user_dev *dev) > >> { > >> - struct rte_ether_addr tmp; > >> + int ret = 0; > >> > >> - if (!mac) > >> - return; > >> + if (!(dev->device_features & (1ULL << VIRTIO_NET_F_MAC))) > >> + return -ENOTSUP; > >> + > >> + if (!dev->ops->set_config) > >> + return -ENOTSUP; > >> + > >> + ret = dev->ops->set_config(dev, dev->mac_addr, > >> + offsetof(struct virtio_net_config, mac), > >> + RTE_ETHER_ADDR_LEN); > >> + if (ret) > >> + PMD_DRV_LOG(ERR, "(%s) Failed to set MAC address in device\n", > >> dev->path); > >> + > >> + return ret; > >> +} > >> + > >> +int > >> +virtio_user_dev_get_mac(struct virtio_user_dev *dev) > >> +{ > >> + int ret = 0; > >> + > >> + if (!(dev->device_features & (1ULL << VIRTIO_NET_F_MAC))) > >> + return -ENOTSUP; > >> + > >> + if (!dev->ops->get_config) > >> + return -ENOTSUP; > >> + > >> + ret = dev->ops->get_config(dev, dev->mac_addr, > >> + offsetof(struct virtio_net_config, mac), > >> + RTE_ETHER_ADDR_LEN); > >> + if (ret) > >> + PMD_DRV_LOG(ERR, "(%s) Failed to get MAC address from device\n", > >> dev->path); > >> > >> - if (rte_ether_unformat_addr(mac, &tmp) == 0) { > >> - memcpy(dev->mac_addr, &tmp, RTE_ETHER_ADDR_LEN); > >> + return ret; > >> +} > >> + > >> +static void > >> +virtio_user_dev_init_mac(struct virtio_user_dev *dev, const char *mac) > >> +{ > >> + struct rte_ether_addr cmdline_mac; > >> + int ret; > >> + > >> + if (mac && rte_ether_unformat_addr(mac, &cmdline_mac) == 0) { > >> + /* > >> + * MAC address was passed from command-line, try to store > >> + * it in the device if it supports it. Otherwise try to use > >> + * the device one. > >> + */ > >> + memcpy(dev->mac_addr, &cmdline_mac, RTE_ETHER_ADDR_LEN); > >> dev->mac_specified = 1; > > > > How do we define mac_specified? If I understand correctly, it means the mac > > we see is from device (we set it or we just use device's). Then 'dev- > >mac_specified = 1' > > should be after get_mac succeeds. > > You are correct, mac_specified=1 means either user or device specified > MAC address. If get_mac fails below then we use the user specified MAC > address, so mac_specified = 1 is still valid in this case. > > > Note that during virtio_user_dev_init, we also use > > this val to set VIRTIO_NET_F_MAC. But here the val is set without making > sure the > > feature exists. > > I am not sure to get youre point, but it sets VIRTIO_NET_F_MAC in the > frontend features there, that does not mean the feature is negotiated in > the end.
I think you are correct, I may misunderstood something when I review this first time. And I want to make sure we are on the same page: since 'mac_specified=1' will set VIRTIO_NET_F_MAC in frontend_features, so only when user don't set mac and we don't get mac in device will lead to this feature unsupported, right? > > >> + > >> + /* Setting MAC may fail, continue to get the device one in this > >> case */ > >> + virtio_user_dev_set_mac(dev); > >> + ret = virtio_user_dev_get_mac(dev); > >> + if (ret == -ENOTSUP) > >> + return; > >> + > >> + if (memcmp(&cmdline_mac, dev->mac_addr, RTE_ETHER_ADDR_LEN)) > >> + PMD_DRV_LOG(INFO, "(%s) Device MAC update failed\n", > >> dev- > >>> path); > > > > Besides Adrian's comments, if we decide to return no error on this, it may > also > > be good to add something like 'using random MAC' to tell users that the > driver will > > use random mac. Adding here or in the function that generates mac is both > > ok. > > If it fails here, it won't be using a random MAC, but the MAC provided > by the user. The log could be improved with something like: > "Device MAC update failed, using MAC xx:xx:xx:xx:xx" Yeah! That's good. Thanks, Chenbo > > What do you think? > > > The patchset overall looks good to me. I'm looking forward to v1 😊 > > > > Thanks, > > Chenbo > > Thanks, > Maxime