| 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
| ||
|
Message-ID: <1bc99068-d0bf-21a6-f3c7-31f74e094b92@intel.com>
Date: Tue, 18 Oct 2022 15:55:20 +0800
From: "Zhu, Lingshan" <lingshan.zhu@...el.com>
To: Jason Wang <jasowang@...hat.com>
Cc: mst@...hat.com, stephen@...workplumber.org, dsahern@...nel.org,
virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
hang.yuan@...el.com, Si-Wei Liu <si-wei.liu@...cle.com>
Subject: Re: [PATCH] iproute2/vdpa: Add support for reading device features
On 10/18/2022 3:49 PM, Jason Wang wrote:
> On Tue, Oct 18, 2022 at 3:46 PM Zhu, Lingshan <lingshan.zhu@...el.com> wrote:
>>
>>
>> On 10/18/2022 3:30 PM, Jason Wang wrote:
>>> On Tue, Oct 18, 2022 at 3:28 PM Zhu, Lingshan <lingshan.zhu@...el.com> wrote:
>>>>
>>>> On 10/18/2022 2:44 PM, Jason Wang wrote:
>>>>> On Tue, Oct 18, 2022 at 10:20 AM Zhu, Lingshan <lingshan.zhu@...el.com> wrote:
>>>>>> On 10/17/2022 3:13 PM, Jason Wang wrote:
>>>>>>> On Mon, Oct 17, 2022 at 3:13 PM Jason Wang <jasowang@...hat.com> wrote:
>>>>>>>> On Fri, Oct 14, 2022 at 5:50 PM Zhu Lingshan <lingshan.zhu@...el.com> wrote:
>>>>>>>>> This commit implements support for reading vdpa device
>>>>>>>>> features in iproute2.
>>>>>>>>>
>>>>>>>>> Example:
>>>>>>>>> $ vdpa dev config show vdpa0
>>>>>>>>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 4 mtu 1500
>>>>>>>>> negotiated_features MRG_RXBUF CTRL_VQ MQ VERSION_1 ACCESS_PLATFORM
>>>>>>>>> dev_features MTU MAC MRG_RXBUF CTRL_VQ MQ ANY_LAYOUT VERSION_1 ACCESS_PLATFORM
>>>>>>>>>
>>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@...el.com>
>>>>>>>> Note that Si Wei proposed to unify the two new attributes:
>>>>>>> https://patchew.org/linux/1665422823-18364-1-git-send-email-si-wei.liu@oracle.com/
>>>>>> I think we have discussed this before, there should be two netlink
>>>>>> attributes to report management device features and vDPA device features,
>>>>>> they are different type of devices, this unification introduces
>>>>>> unnecessary couplings
>>>>> I suggest going through the above patch, both attributes are for the
>>>>> vDPA device only.
>>>> It seems not vDPA device only, from above patch, we see it re-uses
>>>> VDPA_ATTR_DEV_FEATURES for reporting vDPA device features
>>> Yes, anything wrong with this? The device features could be
>>> provisioned via netlink.
>> I think the best netlink practice is to let every attr has its own
>> and unique purpose, to prevent potential bugs. I think we have discussed
>> this before that re-using
>> an attr does not save any resource.
> They have exactly the same semantic which is the device features for vDPA.
>
> VDPA_ATTR_DEV_FEATURES is introduced by my features provisioning
> series, which is used for the userspace to "set" the device features.
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES is introduced in one of your
> previous patches, which is used for userspace to "get" the device
> features.
so basically we are just combining and renaming the attr,
if so, fine with me, get and set may never conflict and in
totally different netlink contexts.
Thanks
>
>> And iprout2 has already updated the uapi header.
> Yes, but iproute2 has the same schedule as kernel release, so it's not
> too late to fix.
>
> Thanks
>
>> Thanks
>>> Thanks
>>>
>>>> Thanks
>>>>> Thanks
>>>>>
>>>>>> Thanks
>>>>>>> Thanks
>>>>>>>
>>>>>>>>> ---
>>>>>>>>> vdpa/vdpa.c | 15 +++++++++++++--
>>>>>>>>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c
>>>>>>>>> index b73e40b4..89844e92 100644
>>>>>>>>> --- a/vdpa/vdpa.c
>>>>>>>>> +++ b/vdpa/vdpa.c
>>>>>>>>> @@ -87,6 +87,8 @@ static const enum mnl_attr_data_type vdpa_policy[VDPA_ATTR_MAX + 1] = {
>>>>>>>>> [VDPA_ATTR_DEV_NEGOTIATED_FEATURES] = MNL_TYPE_U64,
>>>>>>>>> [VDPA_ATTR_DEV_MGMTDEV_MAX_VQS] = MNL_TYPE_U32,
>>>>>>>>> [VDPA_ATTR_DEV_SUPPORTED_FEATURES] = MNL_TYPE_U64,
>>>>>>>>> + [VDPA_ATTR_DEV_FEATURES] = MNL_TYPE_U64,
>>>>>>>>> + [VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES] = MNL_TYPE_U64,
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> static int attr_cb(const struct nlattr *attr, void *data)
>>>>>>>>> @@ -482,7 +484,7 @@ static const char * const *dev_to_feature_str[] = {
>>>>>>>>>
>>>>>>>>> #define NUM_FEATURE_BITS 64
>>>>>>>>>
>>>>>>>>> -static void print_features(struct vdpa *vdpa, uint64_t features, bool mgmtdevf,
>>>>>>>>> +static void print_features(struct vdpa *vdpa, uint64_t features, bool devf,
>>>>>>>>> uint16_t dev_id)
>>>>>>>>> {
>>>>>>>>> const char * const *feature_strs = NULL;
>>>>>>>>> @@ -492,7 +494,7 @@ static void print_features(struct vdpa *vdpa, uint64_t features, bool mgmtdevf,
>>>>>>>>> if (dev_id < ARRAY_SIZE(dev_to_feature_str))
>>>>>>>>> feature_strs = dev_to_feature_str[dev_id];
>>>>>>>>>
>>>>>>>>> - if (mgmtdevf)
>>>>>>>>> + if (devf)
>>>>>>>>> pr_out_array_start(vdpa, "dev_features");
>>>>>>>>> else
>>>>>>>>> pr_out_array_start(vdpa, "negotiated_features");
>>>>>>>>> @@ -771,6 +773,15 @@ static void pr_out_dev_net_config(struct vdpa *vdpa, struct nlattr **tb)
>>>>>>>>> val_u64 = mnl_attr_get_u64(tb[VDPA_ATTR_DEV_NEGOTIATED_FEATURES]);
>>>>>>>>> print_features(vdpa, val_u64, false, dev_id);
>>>>>>>>> }
>>>>>>>>> + if (tb[VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES]) {
>>>>>>>>> + uint16_t dev_id = 0;
>>>>>>>>> +
>>>>>>>>> + if (tb[VDPA_ATTR_DEV_ID])
>>>>>>>>> + dev_id = mnl_attr_get_u32(tb[VDPA_ATTR_DEV_ID]);
>>>>>>>>> +
>>>>>>>>> + val_u64 = mnl_attr_get_u64(tb[VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES]);
>>>>>>>>> + print_features(vdpa, val_u64, true, dev_id);
>>>>>>>>> + }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> static void pr_out_dev_config(struct vdpa *vdpa, struct nlattr **tb)
>>>>>>>>> --
>>>>>>>>> 2.31.1
>>>>>>>>>
Powered by blists - more mailing lists