netdev - Re: [PATCH V3 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <685241b9-3487-489c-2784-2a2209f660ad@intel.com>
Date:   Thu, 28 Jul 2022 10:44:40 +0800
From:   "Zhu, Lingshan" <lingshan.zhu@...el.com>
To:     Si-Wei Liu <si-wei.liu@...cle.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Parav Pandit <parav@...dia.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "xieyongji@...edance.com" <xieyongji@...edance.com>,
        "gautam.dawar@....com" <gautam.dawar@....com>
Subject: Re: [PATCH V3 5/6] vDPA: answer num of queue pairs = 1 to userspace
 when VIRTIO_NET_F_MQ == 0



On 7/28/2022 9:41 AM, Si-Wei Liu wrote:
>
>
> On 7/27/2022 4:54 AM, Zhu, Lingshan wrote:
>>
>>
>> On 7/27/2022 6:09 PM, Si-Wei Liu wrote:
>>>
>>>
>>> On 7/27/2022 2:01 AM, Michael S. Tsirkin wrote:
>>>> On Wed, Jul 27, 2022 at 12:50:33AM -0700, Si-Wei Liu wrote:
>>>>>
>>>>> On 7/26/2022 11:01 PM, Michael S. Tsirkin wrote:
>>>>>> On Wed, Jul 27, 2022 at 03:47:35AM +0000, Parav Pandit wrote:
>>>>>>>> From: Zhu, Lingshan <lingshan.zhu@...el.com>
>>>>>>>> Sent: Tuesday, July 26, 2022 10:53 PM
>>>>>>>>
>>>>>>>> On 7/27/2022 10:17 AM, Parav Pandit wrote:
>>>>>>>>>> From: Zhu, Lingshan <lingshan.zhu@...el.com>
>>>>>>>>>> Sent: Tuesday, July 26, 2022 10:15 PM
>>>>>>>>>>
>>>>>>>>>> On 7/26/2022 11:56 PM, Parav Pandit wrote:
>>>>>>>>>>>> From: Zhu, Lingshan <lingshan.zhu@...el.com>
>>>>>>>>>>>> Sent: Tuesday, July 12, 2022 11:46 PM
>>>>>>>>>>>>> When the user space which invokes netlink commands, 
>>>>>>>>>>>>> detects that
>>>>>>>>>> _MQ
>>>>>>>>>>>> is not supported, hence it takes max_queue_pair = 1 by itself.
>>>>>>>>>>>> I think the kernel module have all necessary information 
>>>>>>>>>>>> and it is
>>>>>>>>>>>> the only one which have precise information of a device, so it
>>>>>>>>>>>> should answer precisely than let the user space guess. The 
>>>>>>>>>>>> kernel
>>>>>>>>>>>> module should be reliable than stay silent, leave the 
>>>>>>>>>>>> question to
>>>>>>>>>>>> the user space
>>>>>>>>>> tool.
>>>>>>>>>>> Kernel is reliable. It doesn’t expose a config space field 
>>>>>>>>>>> if the
>>>>>>>>>>> field doesn’t
>>>>>>>>>> exist regardless of field should have default or no default.
>>>>>>>>>> so when you know it is one queue pair, you should answer one, 
>>>>>>>>>> not try
>>>>>>>>>> to guess.
>>>>>>>>>>> User space should not guess either. User space gets to see 
>>>>>>>>>>> if _MQ
>>>>>>>>>> present/not present. If _MQ present than get reliable data 
>>>>>>>>>> from kernel.
>>>>>>>>>>> If _MQ not present, it means this device has one VQ pair.
>>>>>>>>>> it is still a guess, right? And all user space tools 
>>>>>>>>>> implemented this
>>>>>>>>>> feature need to guess
>>>>>>>>> No. it is not a guess.
>>>>>>>>> It is explicitly checking the _MQ feature and deriving the value.
>>>>>>>>> The code you proposed will be present in the user space.
>>>>>>>>> It will be uniform for _MQ and 10 other features that are 
>>>>>>>>> present now and
>>>>>>>> in the future.
>>>>>>>> MQ and other features like RSS are different. If there is no 
>>>>>>>> _RSS_XX, there
>>>>>>>> are no attributes like max_rss_key_size, and there is not a 
>>>>>>>> default value.
>>>>>>>> But for MQ, we know it has to be 1 wihtout _MQ.
>>>>>>> "we" = user space.
>>>>>>> To keep the consistency among all the config space fields.
>>>>>> Actually I looked and the code some more and I'm puzzled:
>>>>>>
>>>>>>
>>>>>>     struct virtio_net_config config = {};
>>>>>>     u64 features;
>>>>>>     u16 val_u16;
>>>>>>
>>>>>>     vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config));
>>>>>>
>>>>>>     if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, 
>>>>>> sizeof(config.mac),
>>>>>>             config.mac))
>>>>>>         return -EMSGSIZE;
>>>>>>
>>>>>>
>>>>>> Mac returned even without VIRTIO_NET_F_MAC
>>>>>>
>>>>>>
>>>>>>     val_u16 = le16_to_cpu(config.status);
>>>>>>     if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>>>>>>         return -EMSGSIZE;
>>>>>>
>>>>>>
>>>>>> status returned even without VIRTIO_NET_F_STATUS
>>>>>>
>>>>>>     val_u16 = le16_to_cpu(config.mtu);
>>>>>>     if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>>>>>>         return -EMSGSIZE;
>>>>>>
>>>>>>
>>>>>> MTU returned even without VIRTIO_NET_F_MTU
>>>>>>
>>>>>>
>>>>>> What's going on here?
>>>>>>
>>>>>>
>>>>> I guess this is spec thing (historical debt), I vaguely recall 
>>>>> these fields
>>>>> are always present in config space regardless the existence of 
>>>>> corresponding
>>>>> feature bit.
>>>>>
>>>>> -Siwei
>>>> Nope:
>>>>
>>>> 2.5.1  Driver Requirements: Device Configuration Space
>>>>
>>>> ...
>>>>
>>>> For optional configuration space fields, the driver MUST check that 
>>>> the corresponding feature is offered
>>>> before accessing that part of the configuration space.
>>> Well, this is driver side of requirement. As this interface is for 
>>> host admin tool to query or configure vdpa device, we don't have to 
>>> wait until feature negotiation is done on guest driver to extract 
>>> vdpa attributes/parameters, say if we want to replicate another vdpa 
>>> device with the same config on migration destination. I think what 
>>> may need to be fix is to move off from using 
>>> .vdpa_get_config_unlocked() which depends on feature negotiation. 
>>> And/or expose config space register values through another set of 
>>> attributes.
>> Yes, we don't have to wait for FEATURES_OK. In another patch in this 
>> series, I have added a new netlink attr to report the device 
>> features, and removed the blocker. So the LM orchestration SW can 
>> query the device features of the devices at the destination cluster, 
>> and pick a proper one, even mask out some features to meet the LM 
>> requirements.
> For that end, you'd need to move off from using 
> vdpa_get_config_unlocked() which depends on feature negotiation. Since 
> this would slightly change the original semantics of each field that 
> "vdpa dev config" shows, it probably need another netlink command and 
> new uAPI.
why not show both device_features and driver_features in "vdpa dev 
config show"?
>
> -Siwei
>
>
>>
>> Thanks,
>> Zhu Lingshan
>>> -Siwei
>>>
>>>
>>>
>>>
>>
>