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 04BA5A034F; Tue, 8 Jun 2021 08:22:52 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7DA73410E7; Tue, 8 Jun 2021 08:22:52 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 657AB4013F for ; Tue, 8 Jun 2021 08:22:50 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1623133369; 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=pwtxpkSOSZmfTwQypnCWC46rfHyUvs3wSldpXLyxqak=; b=OfkUcJ8qL5nkS0fgVyw6z9DcZSoIXE54X7MrQVzDb0BPuOLFfEVIDDu8CDAzrW+Lt/yB6g gazUXgc+e2Tdb5tIkwcuzcX4JIgTWS2R5dCeS8Of+8jgzNrH0jL2ADdzRpii/xMwaNeNFu 0OIiGsri9rCvmPL6AIX2C+aeHuZRjc0= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-490-GXCL5-5pPoGIiTKTSWcyHQ-1; Tue, 08 Jun 2021 02:22:48 -0400 X-MC-Unique: GXCL5-5pPoGIiTKTSWcyHQ-1 Received: by mail-wr1-f72.google.com with SMTP id k11-20020adfe3cb0000b0290115c29d165cso8929262wrm.14 for ; Mon, 07 Jun 2021 23:22:48 -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=pwtxpkSOSZmfTwQypnCWC46rfHyUvs3wSldpXLyxqak=; b=fpRrEkQVlNEEukLdVp0XPQv/RaD/H3uQxKT2FvcTZ+ous9a5Q1t/Lxd8B923XWkGoY 5ieebwlLTgKzSJt7VblxPG576QLE0B94QlVNY5MIglTt96JfYB9JwPdebvlp1QMEbiV1 sLWW5SPcWr5bWRi7JBqQv9PC7xA+Dec9R35cOYuA59Cya5AaMt9rkqGnawMo++KAWaAj Z4uCmjqEM/dMaIlULjeewA8sYpA2JMuKYwmD08R0x/iBHmMM2bb6qYDwQGachhH8cKNJ oHE3W/GcYycojZxOHDKZjjavKNn36gzJah+8kR5RklmFt/FxkFOw7kSGI/mu8/dFoFUr OuKw== X-Gm-Message-State: AOAM5336nnnz7dDU/E+a1i923PXdLRZBqi2OAPKRO9ihbNauiSr/ELt2 RMZ0lBfD4pCZoGuj5O0raeJfwowLEJjZ+/9Ezobne9RIiFRDY2St+jei+WAtuIFLQMiFdUt7s6U Knp4= X-Received: by 2002:adf:f211:: with SMTP id p17mr20425666wro.173.1623133367212; Mon, 07 Jun 2021 23:22:47 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwj+eLJ5FJXM1slYZ9HvN2kP340g1YIjIVcGNnjuDp4bVHtf+lvMo2LMUhQM4L7HJGwXVWiFQ== X-Received: by 2002:adf:f211:: with SMTP id p17mr20425642wro.173.1623133367031; Mon, 07 Jun 2021 23:22:47 -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 i2sm15909246wmo.40.2021.06.07.23.22.46 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 07 Jun 2021 23:22:46 -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: <13284080-d812-8035-e403-73d1160e8f05@redhat.com> Date: Tue, 8 Jun 2021 08:22:46 +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" On 6/8/21 7:29 AM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin >> Sent: Thursday, June 3, 2021 10:29 PM >> To: Xia, Chenbo ; Maxime Coquelin >> ; 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 >>>> 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. > > 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 >