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: <ffd66203-4349-0986-2130-f8b156f1923a@intel.com> 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
Powered by blists - more mailing lists