This patch adds vDPA device config space requests support. For now, it only adds MAC address get and set. It may be extended in next revision to support other configs like link state. Regarding the MAC selection strategy, if devargs MAC address is set by the user and valid, the driver tries to store it in the device config space, then it reads the MAC address back from the device config, which will be used. If not set in devargs or invalid, it tries to read it from the device. If it fails, a random MAC will be used. I'm interrested to know your feedback on this strategy. It has been tested with vDPA simulator, which only supports getting the MAC address, and witch CX6 which supports neither getting or setting MAC address (and so devarg or random MAC is used). IFCVF driver seems to support both getting and setting the MAC, I have a try with it before next revision. Maxime Coquelin (3): net/virtio: keep device and frontend features separated net/virtio: add device config support to vDPA net/virtio: add MAC device config getter and setter drivers/net/virtio/virtio_user/vhost.h | 3 + drivers/net/virtio/virtio_user/vhost_vdpa.c | 69 +++++++++++++++ .../net/virtio/virtio_user/virtio_user_dev.c | 88 +++++++++++++++---- .../net/virtio/virtio_user/virtio_user_dev.h | 2 + drivers/net/virtio/virtio_user_ethdev.c | 12 ++- 5 files changed, 151 insertions(+), 23 deletions(-) -- 2.30.2
This patch is preliminary rework to add support for getting and setting device's config space. In order to get or set a device config such as its MAC address, we need to know whether the device itself support the feature, or if it is emulated by the frontend. Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- drivers/net/virtio/virtio_user/virtio_user_dev.c | 10 ++-------- drivers/net/virtio/virtio_user_ethdev.c | 5 +++-- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index 1b54d55bd8..8757a23f6e 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -572,11 +572,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) dev->frontend_features |= (1ull << VIRTIO_NET_F_STATUS); - /* - * Device features = - * (frontend_features | backend_features) & ~unsupported_features; - */ - dev->device_features |= dev->frontend_features; + dev->frontend_features &= ~dev->unsupported_features; dev->device_features &= ~dev->unsupported_features; if (rte_mem_event_callback_register(VIRTIO_USER_MEM_EVENT_CLB_NAME, @@ -940,12 +936,10 @@ virtio_user_dev_server_reconnect(struct virtio_user_dev *dev) return -1; } - dev->device_features |= dev->frontend_features; - /* unmask vhost-user unsupported features */ dev->device_features &= ~(dev->unsupported_features); - dev->features &= dev->device_features; + dev->features &= (dev->device_features | dev->frontend_features); /* For packed ring, resetting queues is required in reconnection. */ if (virtio_with_packed_queue(hw) && diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c index 1810a54694..bb36316186 100644 --- a/drivers/net/virtio/virtio_user_ethdev.c +++ b/drivers/net/virtio/virtio_user_ethdev.c @@ -110,7 +110,8 @@ virtio_user_get_features(struct virtio_hw *hw) struct virtio_user_dev *dev = virtio_user_get_dev(hw); /* unmask feature bits defined in vhost user protocol */ - return dev->device_features & VIRTIO_PMD_SUPPORTED_GUEST_FEATURES; + return (dev->device_features | dev->frontend_features) & + VIRTIO_PMD_SUPPORTED_GUEST_FEATURES; } static void @@ -118,7 +119,7 @@ virtio_user_set_features(struct virtio_hw *hw, uint64_t features) { struct virtio_user_dev *dev = virtio_user_get_dev(hw); - dev->features = features & dev->device_features; + dev->features = features & (dev->device_features | dev->frontend_features); } static int -- 2.30.2
This patch introduces two virtio-user callbacks to get and set device's config, and implements it for vDPA backends. Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- drivers/net/virtio/virtio_user/vhost.h | 3 + drivers/net/virtio/virtio_user/vhost_vdpa.c | 69 +++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/drivers/net/virtio/virtio_user/vhost.h b/drivers/net/virtio/virtio_user/vhost.h index c49e88036d..dfbf6be033 100644 --- a/drivers/net/virtio/virtio_user/vhost.h +++ b/drivers/net/virtio/virtio_user/vhost.h @@ -79,6 +79,9 @@ struct virtio_user_backend_ops { int (*set_vring_addr)(struct virtio_user_dev *dev, struct vhost_vring_addr *addr); int (*get_status)(struct virtio_user_dev *dev, uint8_t *status); int (*set_status)(struct virtio_user_dev *dev, uint8_t status); + int (*get_config)(struct virtio_user_dev *dev, uint8_t *data, uint32_t off, uint32_t len); + int (*set_config)(struct virtio_user_dev *dev, const uint8_t *data, uint32_t off, + uint32_t len); int (*enable_qp)(struct virtio_user_dev *dev, uint16_t pair_idx, int enable); int (*dma_map)(struct virtio_user_dev *dev, void *addr, uint64_t iova, size_t len); int (*dma_unmap)(struct virtio_user_dev *dev, void *addr, uint64_t iova, size_t len); diff --git a/drivers/net/virtio/virtio_user/vhost_vdpa.c b/drivers/net/virtio/virtio_user/vhost_vdpa.c index e2d6d3504d..59bc712d48 100644 --- a/drivers/net/virtio/virtio_user/vhost_vdpa.c +++ b/drivers/net/virtio/virtio_user/vhost_vdpa.c @@ -41,6 +41,8 @@ struct vhost_vdpa_data { #define VHOST_VDPA_GET_DEVICE_ID _IOR(VHOST_VIRTIO, 0x70, __u32) #define VHOST_VDPA_GET_STATUS _IOR(VHOST_VIRTIO, 0x71, __u8) #define VHOST_VDPA_SET_STATUS _IOW(VHOST_VIRTIO, 0x72, __u8) +#define VHOST_VDPA_GET_CONFIG _IOR(VHOST_VIRTIO, 0x73, struct vhost_vdpa_config) +#define VHOST_VDPA_SET_CONFIG _IOW(VHOST_VIRTIO, 0x74, struct vhost_vdpa_config) #define VHOST_VDPA_SET_VRING_ENABLE _IOW(VHOST_VIRTIO, 0x75, struct vhost_vring_state) #define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64) #define VHOST_GET_BACKEND_FEATURES _IOR(VHOST_VIRTIO, 0x26, __u64) @@ -65,6 +67,12 @@ struct vhost_iotlb_msg { #define VHOST_IOTLB_MSG_V2 0x2 +struct vhost_vdpa_config { + uint32_t off; + uint32_t len; + uint8_t buf[0]; +}; + struct vhost_msg { uint32_t type; uint32_t reserved; @@ -440,6 +448,65 @@ vhost_vdpa_set_status(struct virtio_user_dev *dev, uint8_t status) return vhost_vdpa_ioctl(data->vhostfd, VHOST_VDPA_SET_STATUS, &status); } +static int +vhost_vdpa_get_config(struct virtio_user_dev *dev, uint8_t *data, uint32_t off, uint32_t len) +{ + struct vhost_vdpa_data *vdpa_data = dev->backend_data; + struct vhost_vdpa_config *config; + int ret = 0; + + config = malloc(sizeof(*config) + len); + if (!config) { + PMD_DRV_LOG(ERR, "Failed to allocate vDPA config data\n"); + return -1; + } + + config->off = off; + config->len = len; + + ret = vhost_vdpa_ioctl(vdpa_data->vhostfd, VHOST_VDPA_GET_CONFIG, config); + if (ret) { + PMD_DRV_LOG(ERR, "Failed to get vDPA config (offset %x, len %x)\n", off, len); + ret = -1; + goto out; + } + + memcpy(data, config->buf, len); +out: + free(config); + + return ret; +} + +static int +vhost_vdpa_set_config(struct virtio_user_dev *dev, const uint8_t *data, uint32_t off, uint32_t len) +{ + struct vhost_vdpa_data *vdpa_data = dev->backend_data; + struct vhost_vdpa_config *config; + int ret = 0; + + config = malloc(sizeof(*config) + len); + if (!config) { + PMD_DRV_LOG(ERR, "Failed to allocate vDPA config data\n"); + return -1; + } + + config->off = off; + config->len = len; + + memcpy(config->buf, data, len); + + ret = vhost_vdpa_ioctl(vdpa_data->vhostfd, VHOST_VDPA_SET_CONFIG, config); + if (ret) { + PMD_DRV_LOG(ERR, "Failed to set vDPA config (offset %x, len %x)\n", off, len); + ret = -1; + } + + free(config); + + return ret; +} + /** * Set up environment to talk with a vhost vdpa backend. * @@ -559,6 +626,8 @@ struct virtio_user_backend_ops virtio_ops_vdpa = { .set_vring_addr = vhost_vdpa_set_vring_addr, .get_status = vhost_vdpa_get_status, .set_status = vhost_vdpa_set_status, + .get_config = vhost_vdpa_get_config, + .set_config = vhost_vdpa_set_config, .enable_qp = vhost_vdpa_enable_queue_pair, .dma_map = vhost_vdpa_dma_map_batch, .dma_unmap = vhost_vdpa_dma_unmap_batch, -- 2.30.2
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. Signed-off-by: Maxime Coquelin <maxime.coquelin@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; + + /* 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); } else { - /* ignore the wrong mac, use random mac */ - PMD_DRV_LOG(ERR, "wrong format of mac: %s", mac); + ret = virtio_user_dev_get_mac(dev); + if (ret) + PMD_DRV_LOG(ERR, "(%s) No valid MAC in devargs or device, use random\n", + dev->path); + else + dev->mac_specified = 1; } } @@ -508,8 +564,6 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, dev->unsupported_features = 0; dev->backend_type = backend_type; - parse_mac(dev, mac); - if (*ifname) { dev->ifname = *ifname; *ifname = NULL; @@ -537,6 +591,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, return -1; } + virtio_user_dev_init_mac(dev, mac); + if (!mrg_rxbuf) dev->unsupported_features |= (1ull << VIRTIO_NET_F_MRG_RXBUF); diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h index 8a62f7ea79..03bcf95970 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h @@ -78,6 +78,8 @@ uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs); int virtio_user_dev_set_status(struct virtio_user_dev *dev, uint8_t status); int virtio_user_dev_update_status(struct virtio_user_dev *dev); int virtio_user_dev_update_link_state(struct virtio_user_dev *dev); +int virtio_user_dev_set_mac(struct virtio_user_dev *dev); +int virtio_user_dev_get_mac(struct virtio_user_dev *dev); void virtio_user_dev_delayed_handler(void *param); int virtio_user_dev_server_reconnect(struct virtio_user_dev *dev); extern const char * const virtio_user_backend_strings[]; diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c index bb36316186..34c8e7a9b5 100644 --- a/drivers/net/virtio/virtio_user_ethdev.c +++ b/drivers/net/virtio/virtio_user_ethdev.c @@ -60,12 +60,15 @@ virtio_user_write_dev_config(struct virtio_hw *hw, size_t offset, struct virtio_user_dev *dev = virtio_user_get_dev(hw); if ((offset == offsetof(struct virtio_net_config, mac)) && - (length == RTE_ETHER_ADDR_LEN)) + (length == RTE_ETHER_ADDR_LEN)) { for (i = 0; i < RTE_ETHER_ADDR_LEN; ++i) dev->mac_addr[i] = ((const uint8_t *)src)[i]; - else + virtio_user_dev_set_mac(dev); + virtio_user_dev_get_mac(dev); + } else { PMD_DRV_LOG(ERR, "not supported offset=%zu, len=%d", offset, length); + } } static void -- 2.30.2
On 3/18/21 11:35 PM, Maxime Coquelin wrote: > This patch adds vDPA device config space requests support. > For now, it only adds MAC address get and set. It may be > extended in next revision to support other configs like > link state. > > Regarding the MAC selection strategy, if devargs MAC address > is set by the user and valid, the driver tries to store it > in the device config space, then it reads the MAC address > back from the device config, which will be used. If not set > in devargs or invalid, it tries to read it from the device. > If it fails, a random MAC will be used. > > I'm interrested to know your feedback on this strategy. In general, I think it's a reasonable strategy. Once we have cq support, things will be a bit easier. Some questions: How should we interpret failure to configure the mac (i.e: after set and get, they still don't match)? Should we fail virtio_user_dev_init if the configuration provided by devargs is not successfully applied? Should a zero mac be treated differntly as qemu does? [1] [1] https://patchwork.ozlabs.org/project/qemu-devel/patch/20210302142014.141135-3-mst@redhat.com/ > It has been tested with vDPA simulator, which only supports > getting the MAC address, and witch CX6 which supports neither > getting or setting MAC address (and so devarg or random MAC is > used). IFCVF driver seems to support both getting and setting > the MAC, I have a try with it before next revision. Does cx6 negotiate VIRTIO_NET_F_MAC? > > Maxime Coquelin (3): > net/virtio: keep device and frontend features separated > net/virtio: add device config support to vDPA > net/virtio: add MAC device config getter and setter > > drivers/net/virtio/virtio_user/vhost.h | 3 + > drivers/net/virtio/virtio_user/vhost_vdpa.c | 69 +++++++++++++++ > .../net/virtio/virtio_user/virtio_user_dev.c | 88 +++++++++++++++---- > .../net/virtio/virtio_user/virtio_user_dev.h | 2 + > drivers/net/virtio/virtio_user_ethdev.c | 12 ++- > 5 files changed, 151 insertions(+), 23 deletions(-) > -- Adrián Moreno
Hi Adrian, Thanks for your feedback. On 4/16/21 9:28 AM, Adrian Moreno wrote: > > > On 3/18/21 11:35 PM, Maxime Coquelin wrote: >> This patch adds vDPA device config space requests support. >> For now, it only adds MAC address get and set. It may be >> extended in next revision to support other configs like >> link state. >> >> Regarding the MAC selection strategy, if devargs MAC address >> is set by the user and valid, the driver tries to store it >> in the device config space, then it reads the MAC address >> back from the device config, which will be used. If not set >> in devargs or invalid, it tries to read it from the device. >> If it fails, a random MAC will be used. >> >> I'm interrested to know your feedback on this strategy. > > In general, I think it's a reasonable strategy. Once we have cq support, things > will be a bit easier. > > Some questions: > How should we interpret failure to configure the mac (i.e: after set and get, > they still don't match)? Should we fail virtio_user_dev_init if the > configuration provided by devargs is not successfully applied? > > Should a zero mac be treated differntly as qemu does? [1] > > [1] > https://patchwork.ozlabs.org/project/qemu-devel/patch/20210302142014.141135-3-mst@redhat.com/ Testing with ConnectX-6 Dx, I can see that the device does not advertise VIRTIO_NET_F_MAC, so with this is series, it just doesn't try to read it from the device. My understanding is that in Qemu, the feature is forced[0], which explains why it reads zeroes. > >> It has been tested with vDPA simulator, which only supports >> getting the MAC address, and witch CX6 which supports neither >> getting or setting MAC address (and so devarg or random MAC is >> used). IFCVF driver seems to support both getting and setting >> the MAC, I have a try with it before next revision. > > Does cx6 negotiate VIRTIO_NET_F_MAC? Nope. I haven't tested yet with IFCVF. Maxime >> >> Maxime Coquelin (3): >> net/virtio: keep device and frontend features separated >> net/virtio: add device config support to vDPA >> net/virtio: add MAC device config getter and setter >> >> drivers/net/virtio/virtio_user/vhost.h | 3 + >> drivers/net/virtio/virtio_user/vhost_vdpa.c | 69 +++++++++++++++ >> .../net/virtio/virtio_user/virtio_user_dev.c | 88 +++++++++++++++---- >> .../net/virtio/virtio_user/virtio_user_dev.h | 2 + >> drivers/net/virtio/virtio_user_ethdev.c | 12 ++- >> 5 files changed, 151 insertions(+), 23 deletions(-) >> > [0]: https://elixir.bootlin.com/qemu/latest/source/hw/net/virtio-net.c#L3078
On 3/18/21 11:35 PM, Maxime Coquelin wrote: > 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. > > Signed-off-by: Maxime Coquelin <maxime.coquelin@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; > + > + /* 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); Maybe log the mac that we've read if it differs from the one we tried to write? > } else { > - /* ignore the wrong mac, use random mac */ > - PMD_DRV_LOG(ERR, "wrong format of mac: %s", mac); > + ret = virtio_user_dev_get_mac(dev); > + if (ret) > + PMD_DRV_LOG(ERR, "(%s) No valid MAC in devargs or device, use random\n", > + dev->path); > + else > + dev->mac_specified = 1; > } > } > > @@ -508,8 +564,6 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, > dev->unsupported_features = 0; > dev->backend_type = backend_type; > > - parse_mac(dev, mac); > - > if (*ifname) { > dev->ifname = *ifname; > *ifname = NULL; > @@ -537,6 +591,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, > return -1; > } > > + virtio_user_dev_init_mac(dev, mac); > + > if (!mrg_rxbuf) > dev->unsupported_features |= (1ull << VIRTIO_NET_F_MRG_RXBUF); > > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h > index 8a62f7ea79..03bcf95970 100644 > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h > @@ -78,6 +78,8 @@ uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs); > int virtio_user_dev_set_status(struct virtio_user_dev *dev, uint8_t status); > int virtio_user_dev_update_status(struct virtio_user_dev *dev); > int virtio_user_dev_update_link_state(struct virtio_user_dev *dev); > +int virtio_user_dev_set_mac(struct virtio_user_dev *dev); > +int virtio_user_dev_get_mac(struct virtio_user_dev *dev); > void virtio_user_dev_delayed_handler(void *param); > int virtio_user_dev_server_reconnect(struct virtio_user_dev *dev); > extern const char * const virtio_user_backend_strings[]; > diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c > index bb36316186..34c8e7a9b5 100644 > --- a/drivers/net/virtio/virtio_user_ethdev.c > +++ b/drivers/net/virtio/virtio_user_ethdev.c > @@ -60,12 +60,15 @@ virtio_user_write_dev_config(struct virtio_hw *hw, size_t offset, > struct virtio_user_dev *dev = virtio_user_get_dev(hw); > > if ((offset == offsetof(struct virtio_net_config, mac)) && > - (length == RTE_ETHER_ADDR_LEN)) > + (length == RTE_ETHER_ADDR_LEN)) { > for (i = 0; i < RTE_ETHER_ADDR_LEN; ++i) > dev->mac_addr[i] = ((const uint8_t *)src)[i]; > - else > + virtio_user_dev_set_mac(dev); > + virtio_user_dev_get_mac(dev); > + } else { > PMD_DRV_LOG(ERR, "not supported offset=%zu, len=%d", > offset, length); > + } > } > > static void > -- Adrián Moreno
Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Friday, March 19, 2021 6:35 AM > To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com; > david.marchand@redhat.com > Cc: Maxime Coquelin <maxime.coquelin@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.coquelin@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. 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. > + > + /* 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. The patchset overall looks good to me. I'm looking forward to v1 😊 Thanks, Chenbo > } else { > - /* ignore the wrong mac, use random mac */ > - PMD_DRV_LOG(ERR, "wrong format of mac: %s", mac); > + ret = virtio_user_dev_get_mac(dev); > + if (ret) > + PMD_DRV_LOG(ERR, "(%s) No valid MAC in devargs or device, > use random\n", > + dev->path); > + else > + dev->mac_specified = 1; > } > } > > @@ -508,8 +564,6 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char > *path, int queues, > dev->unsupported_features = 0; > dev->backend_type = backend_type; > > - parse_mac(dev, mac); > - > if (*ifname) { > dev->ifname = *ifname; > *ifname = NULL; > @@ -537,6 +591,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char > *path, int queues, > return -1; > } > > + virtio_user_dev_init_mac(dev, mac); > + > if (!mrg_rxbuf) > dev->unsupported_features |= (1ull << VIRTIO_NET_F_MRG_RXBUF); > > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h > b/drivers/net/virtio/virtio_user/virtio_user_dev.h > index 8a62f7ea79..03bcf95970 100644 > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h > @@ -78,6 +78,8 @@ uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, > uint16_t q_pairs); > int virtio_user_dev_set_status(struct virtio_user_dev *dev, uint8_t status); > int virtio_user_dev_update_status(struct virtio_user_dev *dev); > int virtio_user_dev_update_link_state(struct virtio_user_dev *dev); > +int virtio_user_dev_set_mac(struct virtio_user_dev *dev); > +int virtio_user_dev_get_mac(struct virtio_user_dev *dev); > void virtio_user_dev_delayed_handler(void *param); > int virtio_user_dev_server_reconnect(struct virtio_user_dev *dev); > extern const char * const virtio_user_backend_strings[]; > diff --git a/drivers/net/virtio/virtio_user_ethdev.c > b/drivers/net/virtio/virtio_user_ethdev.c > index bb36316186..34c8e7a9b5 100644 > --- a/drivers/net/virtio/virtio_user_ethdev.c > +++ b/drivers/net/virtio/virtio_user_ethdev.c > @@ -60,12 +60,15 @@ virtio_user_write_dev_config(struct virtio_hw *hw, size_t > offset, > struct virtio_user_dev *dev = virtio_user_get_dev(hw); > > if ((offset == offsetof(struct virtio_net_config, mac)) && > - (length == RTE_ETHER_ADDR_LEN)) > + (length == RTE_ETHER_ADDR_LEN)) { > for (i = 0; i < RTE_ETHER_ADDR_LEN; ++i) > dev->mac_addr[i] = ((const uint8_t *)src)[i]; > - else > + virtio_user_dev_set_mac(dev); > + virtio_user_dev_get_mac(dev); > + } else { > PMD_DRV_LOG(ERR, "not supported offset=%zu, len=%d", > offset, length); > + } > } > > static void > -- > 2.30.2
Hi Chenbo, On 4/19/21 8:24 AM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin <maxime.coquelin@redhat.com> >> Sent: Friday, March 19, 2021 6:35 AM >> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com; >> david.marchand@redhat.com >> Cc: Maxime Coquelin <maxime.coquelin@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.coquelin@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. >> + >> + /* 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" What do you think? > The patchset overall looks good to me. I'm looking forward to v1 😊 > > Thanks, > Chenbo Thanks, Maxime
Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <mcoqueli@redhat.com> > Sent: Thursday, June 3, 2021 10:29 PM > To: Xia, Chenbo <chenbo.xia@intel.com>; Maxime Coquelin > <maxime.coquelin@redhat.com>; dev@dpdk.org; amorenoz@redhat.com; > david.marchand@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.coquelin@redhat.com> > >> Sent: Friday, March 19, 2021 6:35 AM > >> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com; > >> david.marchand@redhat.com > >> Cc: Maxime Coquelin <maxime.coquelin@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.coquelin@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
On 6/8/21 7:29 AM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin <mcoqueli@redhat.com> >> Sent: Thursday, June 3, 2021 10:29 PM >> To: Xia, Chenbo <chenbo.xia@intel.com>; Maxime Coquelin >> <maxime.coquelin@redhat.com>; dev@dpdk.org; amorenoz@redhat.com; >> david.marchand@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.coquelin@redhat.com> >>>> Sent: Friday, March 19, 2021 6:35 AM >>>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com; >>>> david.marchand@redhat.com >>>> Cc: Maxime Coquelin <maxime.coquelin@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.coquelin@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? Yes, correct. The idea was to keep the old behaviour, i.e. support it when user specifies the MAC in the devargs, and extend it to support the feature when the device can provide the MAC address. Regards, Maxime >> >>>> + >>>> + /* 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 >