Message-ID: <b4fb6dbd-a0d4-5ec5-8955-99153e9fdcd3@nvidia.com>
Date:   Mon, 6 Feb 2023 08:48:36 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH v3 2/6] vdpa: conditionally read STATUS in config space
To:     Si-Wei Liu <si-wei.liu@oracle.com>, mst@redhat.com,
        jasowang@redhat.com, parav@nvidia.com
Cc:     virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
References: <1675400523-12519-1-git-send-email-si-wei.liu@oracle.com>
 <1675400523-12519-3-git-send-email-si-wei.liu@oracle.com>
 <0f158916-aaab-0afd-6cd0-382f54ef26d9@nvidia.com>
 <7b3af822-0075-889c-911c-b79860fd5ce2@oracle.com>
Content-Language: en-US
From:   Eli Cohen <elic@nvidia.com>
In-Reply-To: <7b3af822-0075-889c-911c-b79860fd5ce2@oracle.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aFpEbG9URUtOQSsxdHpwc1hKSEV2NTRHRWlQY2l4R2RYWTVKdkcxa01PWi9j?=
 =?utf-8?B?YXlJcEE0SkhGMmc4SG1JN29tT1NXZ3VhUS9tK21Wd3BNQXhjM2lYQmtVaFhV?=
 =?utf-8?B?THkrZlJqWkRlb3FYdE8vZE5JR2pCa1FWS3lsN1oxU2F3YVhUYlhvQnVSMjQr?=
 =?utf-8?B?MjNqdnptd2tpM3QweWdPOTB0L1JnY1MxdVd5bUM1WXpWb0NVUm9vQVRCMUlr?=
 =?utf-8?B?clZpKzNVc2loTERnTkM5RkhoOEhUb0JqYTBrckllNXlBMlFmRmpQd0hEYWow?=
 =?utf-8?B?QW96RER3UWpGQ0RQYk5MQ3ZFSXZwcUNld3ZHaEVML0c5NEMwS2ZzNHJzcG5r?=
 =?utf-8?B?OHBhRmxlUWw3ZXpsb0VlbThoa3dkT0pPbk1zVUJtM2RQdWc4V29MSXc3Q1R5?=
 =?utf-8?B?Tml6S21VTjRRWGo2eVkrbjd3VWdzdVEreUcrelpoZmVDbUh4NDZGamkyS21y?=
 =?utf-8?B?cjBJVEgrSGdIWnY5QitGYkEvZko5d2tFTmMvWlV5NXhTaXg0L2s5STJOaTd3?=
 =?utf-8?B?OUpTazlYcXVtbUg2aFI1L0gxUVNnc0Jna3ZNRHJYWHNQL1ZiZDQzTXlBRWQw?=
 =?utf-8?B?RHVnTHczeWkvL1ZMQUVhWVNJMlRXSkUwNDA2MnAxZndDM0dKdmJkNjhsUm9N?=
 =?utf-8?B?REZMVWd0dUNVcXJlZTRuUDNkNXNsc1Q1dTRuTmRzV29XL2lESUd4TUFIcTRB?=
 =?utf-8?B?bTNuTGZGbTExWXZnT3g0WU1Ib1JqM0F3Nmk3UXc3M1BidzJNOW9vRmJ5UUor?=
 =?utf-8?B?MnFhNmo0dWtiS3pyTExTMWZicHJQK3hPczBSNVVTMXgvUGtQTERlYy9RNXFw?=
 =?utf-8?B?dmdBclVDQUtjT1Y4bWE5cmVXM2hhTFJON1RheCt3L0hEaXZMbEdUMTI3anV0?=
 =?utf-8?B?NXZxZ05tMjd5a3N2NXdRbzMwQlJyUVJ3WlBFSGhBS1BsRkFMRkVLbTB6eDR3?=
 =?utf-8?B?VzVWdW9pR1ZDUVpoK3FwY2VnUWJpV3MzUFRHdXc4V1dma0lxek5kOVZ4aTk5?=
 =?utf-8?B?akhjbFIrMlBKemt0cmI1Q29rUTk1a3ZpSWxCSk14Ni9ZN0VHWnJmZWZXd2Mr?=
 =?utf-8?B?Y3NON1MwNkwzOEZxdk53RnV4eVkxb25DVlJ3SGZkQ0lCTkV4UkJVV1A3ZmhI?=
 =?utf-8?B?WTNvK2RMS1ZYN1NMbWxML3RuaVdodEQ0VjNwcS9YSWJYYm5Nai96RFBTN0tF?=
 =?utf-8?B?RzFwc1V0SWNvS21xaXFzNnR1bVJ3UHhJSC8wY3hhYjZlalcwdnRucXRXUnhM?=
 =?utf-8?B?S1p4eDdZbDQ5anFRejZLUnB0bkFaZVhWalAzZVVlMGM4dThvbXdJTTNoREYr?=
 =?utf-8?B?Y1VleFpQblJpc09Cc3hNUUtGUjVSVjY0T1hSLzNOUm9LUFNVdmxlZk1VL2dv?=
 =?utf-8?B?VHpXTkg2OXdnNTN1Q2FKTjBIbFh2LzZuYlFjVjdRTnNOdWFUaTVpeGtvcGZV?=
 =?utf-8?B?bXhoUzZzLzExaU1INkcvUmpML1NCWHM0bGdRYWNtZnJ6VGdHanV5cVErN0o0?=
 =?utf-8?B?UU9HN0FHTUVsL2lNaE1aOXlaMlhCSElva1M4T3JzWmJRKzZvNWJ0a04zbzdz?=
 =?utf-8?B?TWVSTFRBNHdZSWI5VHFhNHpOSVJ4RTJ3dzh1czRJRFhDdytlaGpnWjdrbHZU?=
 =?utf-8?B?V2NHdFg5Ly8vMXpXbVBWdDMzWS9Gc2xzY1cxamR4Y0xxUnErazVHTjIxdVBn?=
 =?utf-8?B?Z2hPU25DSHhqM1F0MEsxTSt2TFFBT09CUDJKY2xhQ3hBejIvMTBCZkw4Skc3?=
 =?utf-8?B?eURJUER2RmR4bFcvam5PVW1LQTJENDJBWDVJRVJBbmdiU2VsQ3lCblg2SW5j?=
 =?utf-8?B?MXlVQXlGbVRjZk93aWZ6MmtRbkc3MEs4NWtYallqTW4wVENlekxCMnZQbWZP?=
 =?utf-8?B?QmUwMk5nVDZLNzd5d2NTVm53OXQyYW9pOXQzNUZWZHlSSkZSK3gvcUV1cysx?=
 =?utf-8?B?ZTV3SldBQjNFb2pjNndKUGdub1I1Q0dlb094dk83bnJmSkNmMmdjM2lMQXBC?=
 =?utf-8?B?aXRsWUlpWTRVUlRzcmR5OEJNT0k2NW1scStDK3pxUEJQOVZ6eTQwelBTUjRI?=
 =?utf-8?B?aHlEL3RzeHJ0K2c2S0NaZkxocWpIYW4vVEpZZGRqWEhOdEVWWTFyL2N5SFMz?=
 =?utf-8?Q?jcOUZ0DZw6uJKlrF8AAQn5Bta?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0360ff01-580f-4129-92c5-08db080e3018
X-MS-Exchange-CrossTenant-AuthSource: DM8PR12MB5400.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2023 06:48:43.5507
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: o0xnbUvGzhCYFwajngsuLTdW0Txv9gMS+SyQhn9x8BHf55+unVXkSoNiryCmChnY
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB5709
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 06/02/2023 6:53, Si-Wei Liu wrote:
>
>
> On 2/5/2023 12:26 AM, Eli Cohen wrote:
>>
>> On 03/02/2023 7:01, Si-Wei Liu wrote:
>>> The spec says:
>>>      status only exists if VIRTIO_NET_F_STATUS is set
>>>
>>> Similar to MAC and MTU, vdpa_dev_net_config_fill() should read
>>> STATUS conditionally depending on the feature bits.
>>>
>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>> ---
>>>   drivers/vdpa/vdpa.c | 20 +++++++++++++++-----
>>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 3a82ca78..21c8aa3 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -843,18 +843,25 @@ static int vdpa_dev_net_mac_config_fill(struct 
>>> sk_buff *msg, u64 features,
>>>               sizeof(config->mac), config->mac);
>>>   }
>>>   +static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, 
>>> u64 features,
>>> +                       const struct virtio_net_config *config)
>>> +{
>>> +    u16 val_u16;
>>> +
>>> +    if ((features & BIT_ULL(VIRTIO_NET_F_STATUS)) == 0)
>>> +        return 0;
>> Instead of returning 0 here, it would be better to explicitly put 0 
>> in the message field for
>>
>> VDPA_ATTR_DEV_NET_STATUS
> In light of commit 41a2ad927aa2 ("vDPA: conditionally read MTU and MAC 
> in dev cfg space"), the userspace must now show the config space field 
> presented by the device *as-is*. If the feature bit is not offered by 
> device, the relevant field will not be displayed in 'vdpa dev config' 
> output. For instance, MAC address won't be shown if the MAC feature is 
> not supported/offered by the device (note this has nothing to do with 
> negotiated features), even though the vdpa parent may have a non-zero 
> MAC address of its own. I think STATUS should not be different from 
> MAC and MTU here.

OK, I see your point.

Reviewed-by: Eli Cohen <elic@nvidia.com>

>
> Regards,
> -Siwei
>
>>
>>> +
>>> +    val_u16 = __virtio16_to_cpu(true, config->status);
>>> +    return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
>>> +}
>>> +
>>>   static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, 
>>> struct sk_buff *msg)
>>>   {
>>>       struct virtio_net_config config = {};
>>>       u64 features_device;
>>> -    u16 val_u16;
>>>         vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>>   -    val_u16 = __virtio16_to_cpu(true, config.status);
>>> -    if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>>> -        return -EMSGSIZE;
>>> -
>>>       features_device = vdev->config->get_device_features(vdev);
>>>         if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES, 
>>> features_device,
>>> @@ -867,6 +874,9 @@ static int vdpa_dev_net_config_fill(struct 
>>> vdpa_device *vdev, struct sk_buff *ms
>>>       if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
>>>           return -EMSGSIZE;
>>>   +    if (vdpa_dev_net_status_config_fill(msg, features_device, 
>>> &config))
>>> +        return -EMSGSIZE;
>>> +
>>>       return vdpa_dev_net_mq_config_fill(msg, features_device, 
>>> &config);
>>>   }
>