From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 884B2A0A0A; Thu, 3 Jun 2021 16:29:00 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 52D7740E78; Thu, 3 Jun 2021 16:29:00 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mails.dpdk.org (Postfix) with ESMTP id 8730940DF6 for ; Thu, 3 Jun 2021 16:28:58 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1622730537; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=+ZlNOnBuVckOSxURyTe+RXgAJ8FkQ+y+6tJSAGXBfVA=; b=AH9gR7Xq2QP9Wlgrn8pa6TxzUETHLUR4s3BmdHni3PEPgkj9TgX4g7PNaA27TiHx4Rs+F8 NDrP5+QdZi6lXG6o+vkecdQnQZtAHmUf7d6y3Br2vRU3NbEdK1E8Fbm9AGDluXfv2B12EC pNvSxX8RbxcimVKhz4ttnwoJCC3pRHY= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-527-nT_xxiLCMZWIBydWFBpBUA-1; Thu, 03 Jun 2021 10:28:55 -0400 X-MC-Unique: nT_xxiLCMZWIBydWFBpBUA-1 Received: by mail-wm1-f72.google.com with SMTP id x20-20020a1c7c140000b029018f49a7efb7so3463572wmc.1 for ; Thu, 03 Jun 2021 07:28:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=+ZlNOnBuVckOSxURyTe+RXgAJ8FkQ+y+6tJSAGXBfVA=; b=j+BagXRW/uTMqYrAp1yMzpIa6me89qfV3POX7ZQpcayi4rShk/CoC/v0OJHWtTUT+u 7+/TJwqpDSNX8/rHG6pKh+5OMP0pwqVu6ox9TjrNiCcWn4BaEVqNMXayf1jT7t43koNm /z2dfK3cHsGwUpsPo8plzcZY7d4sj+NwKbLNkHKFhk0qLORqz5dBrOua0GH0yaOCGK/e MIldd4ysVVaj/QL5WgVJMx+a+45BemM/KHFZUUO/NAy8uAwVEtDINYKsKXU2JVrvx/ts 9eGSrLELlpSS0KrU0e5xb1pzudOU7XLGmaTMsEJ+4GmlfSXeD15i+0XK5j5iXz29s06J pWpw== X-Gm-Message-State: AOAM5320198hR3r+o/AvFrM8frwvK5ox0UsOGd81N3a6pD3fVSDVgWOg 3jDsfncR0Pj3Rx1nO1F50b35tBvZEulXGtLqntuGQ1eRjlKvDz7p3xJzsQasKY62SXVmE6P850+ Xg/4= X-Received: by 2002:a5d:504d:: with SMTP id h13mr252000wrt.133.1622730533610; Thu, 03 Jun 2021 07:28:53 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz5jZ8dX9iGyar6fyvrDc6PPljA/apaH4RW1GYwtkxqocnBu3Z8s1Tb67AdvJaknqaM36qdEw== X-Received: by 2002:a5d:504d:: with SMTP id h13mr251987wrt.133.1622730533445; Thu, 03 Jun 2021 07:28:53 -0700 (PDT) Received: from [192.168.1.205] (219-230-83-45.ftth.cust.kwaoo.net. [45.83.230.219]) by smtp.gmail.com with ESMTPSA id a15sm4249108wrs.63.2021.06.03.07.28.52 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 03 Jun 2021 07:28:52 -0700 (PDT) To: "Xia, Chenbo" , Maxime Coquelin , "dev@dpdk.org" , "amorenoz@redhat.com" , "david.marchand@redhat.com" References: <20210318223526.168614-1-maxime.coquelin@redhat.com> <20210318223526.168614-4-maxime.coquelin@redhat.com> From: Maxime Coquelin Message-ID: Date: Thu, 3 Jun 2021 16:28:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=mcoqueli@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [RFC 3/3] net/virtio: add MAC device config getter and setter X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Chenbo, On 4/19/21 8:24 AM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin >> Sent: Friday, March 19, 2021 6:35 AM >> To: dev@dpdk.org; Xia, Chenbo ; amorenoz@redhat.com; >> david.marchand@redhat.com >> Cc: Maxime Coquelin >> 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 >> --- >> .../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