netdev - Re: [Intel-wired-lan] [PATCH net-next 01/15] virtchnl: add virtchnl version 2 ops

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]

Date:   Wed, 12 Apr 2023 09:58:49 -0700
From:   "Tantilov, Emil S" <emil.s.tantilov@...el.com>
To:     Shannon Nelson <shannon.nelson@....com>,
        "Linga, Pavan Kumar" <pavan.kumar.linga@...el.com>,
        <intel-wired-lan@...ts.osuosl.org>
CC:     <netdev@...r.kernel.org>, <shiraz.saleem@...el.com>,
        <willemb@...gle.com>, <decot@...gle.com>, <joshua.a.hay@...el.com>,
        <sridhar.samudrala@...el.com>, Alan Brady <alan.brady@...el.com>,
        Madhu Chittim <madhu.chittim@...el.com>,
        Phani Burra <phani.r.burra@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH net-next 01/15] virtchnl: add virtchnl
 version 2 ops



On 4/10/2023 3:12 PM, Shannon Nelson wrote:
> On 4/10/23 1:27 PM, Linga, Pavan Kumar wrote:
>>
>> On 4/4/2023 3:31 AM, Shannon Nelson wrote:
>>> On 3/29/23 7:03 AM, Pavan Kumar Linga wrote:
>>>>
>>>> Virtchnl version 1 is an interface used by the current generation of
>>>> foundational NICs to negotiate the capabilities and configure the
>>>> HW resources such as queues, vectors, RSS LUT, etc between the PF
>>>> and VF drivers. It is not extensible to enable new features supported
>>>> in the next generation of NICs/IPUs and to negotiate descriptor types,
>>>> packet types and register offsets.
>>>>
> 
> [...]
> 
>>>> +
>>>> +#include "virtchnl2_lan_desc.h"
>>>> +
>>>> +/* VIRTCHNL2_ERROR_CODES */
>>>> +/* Success */
>>>> +#define VIRTCHNL2_STATUS_SUCCESS       0
>>>
>>> Shouldn't these be enum and not #define?
>>>
>>
>> This header file is describing communication protocol with opcodes,
>> error codes, capabilities etc. that are exchanged between IDPF and
>> device Control Plane. Compiler chooses the size of the enum based on the
>> enumeration constants that are present which is not a constant size. But
>> for virtchnl protocol, we want to have fixed size no matter what. To
>> avoid such cases, we are using defines whereever necessary.
> 
> The field size limitations in an API are one thing, and that can be 
> managed by using a u8/u16/u32 or whatever as necessary.  But that 
> doesn't mean that you can't define values to be assigned in those fields 
> as enums, which are preferred when defining several related constants.
> 
We can certainly look into it, but for the purpose of this series it 
doesn't seem like a meaningful change if it only helps with the grouping 
since the define names already follow a certain pattern to indicate that 
they are related.

> 
> [...]
> 
>>
>>>> +
>>>> +/* VIRTCHNL2_OP_GET_EDT_CAPS
>>>> + * Get EDT granularity and time horizon
>>>> + */
>>>> +struct virtchnl2_edt_caps {
>>>> +       /* Timestamp granularity in nanoseconds */
>>>> +       __le64 tstamp_granularity_ns;
>>>> +       /* Total time window in nanoseconds */
>>>> +       __le64 time_horizon_ns;
>>>> +};
>>>> +
>>>> +VIRTCHNL2_CHECK_STRUCT_LEN(16, virtchnl2_edt_caps);
>>>
>>> Don't put a space between the struct and the check.
>>>
>>
>> Checkpatch reports a warning (actually a 'Check') when the newline is
>> removed. Following is the checkpatch output when the newline is removed:
>>
>> "CHECK: Please use a blank line after function/struct/union/enum
>> declarations"
> 
> Since it has to do directly with the finished definition, one would 
> think it could follow the same rule as EXPORT... does.  It might not be 
> a bad idea at some point for static_assert() to be added to that allowed 
> list.  For now, though, since it is only a CHECK and not WARN or ERROR, 
> you might be able to ignore it.  It might be easier to ignore if you 
> just used the existing static_assert() rather than definigin your own 
> synonym.

OK, we'll remove it.

> 
> 
> [...]
> 
>>>> +/* Queue to vector mapping */
>>>> +struct virtchnl2_queue_vector {
>>>> +       __le32 queue_id;
>>>> +       __le16 vector_id;
>>>> +       u8 pad[2];
>>>> +
>>>> +       /* See VIRTCHNL2_ITR_IDX definitions */
>>>> +       __le32 itr_idx;
>>>> +
>>>> +       /* See VIRTCHNL2_QUEUE_TYPE definitions */
>>>> +       __le32 queue_type;
>>>> +       u8 pad1[8];
>>>> +};
>>>
>>> Why the end padding?  What's wrong with the 16-byte size?
>>>
>>
>> The end padding is added for any possible future additions of the fields
>> to this structure. Didn't get the ask for 16-byte size, can you please
>> elaborate?
> 
> Without the pad1[8], this struct is an even 16 bytes, seems like a 
> logical place to stop.  24 bytes seems odd, if you're going to pad for 
> the future it makes some sense to do it to an even 32 bytes 
> (power-of-2).  And please add a comment for this future thinking.

We can change the name to reserved to make it clearer, but the size 
cannot be changed because it's an ABI already.

Thanks,
Emil

> 
> sln