netdev - RE: [Intel-wired-lan] [PATCH v10 iwl-next 03/11] virtchnl: add PTP virtchnl definitions

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: <MW4PR11MB5889CBA3909D6C877DA20CF48EB42@MW4PR11MB5889.namprd11.prod.outlook.com>
Date: Wed, 9 Apr 2025 11:51:06 +0000
From: "Olech, Milena" <milena.olech@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "Lobakin, Aleksander"
	<aleksander.lobakin@...el.com>, Willem de Bruijn <willemb@...gle.com>, "Mina
 Almasry" <almasrymina@...gle.com>, "Salin, Samuel" <samuel.salin@...el.com>
Subject: RE: [Intel-wired-lan] [PATCH v10 iwl-next 03/11] virtchnl: add PTP
 virtchnl definitions

On 4/8/2025 11:12 PM, Jacob Keller wrote:

>On 4/8/2025 3:30 AM, Milena Olech wrote:
>> PTP capabilities are negotiated using virtchnl commands. There are two
>> available modes of the PTP support: direct and mailbox. When the direct
>> access to PTP resources is negotiated, virtchnl messages returns a set
>> of registers that allow read/write directly. When the mailbox access to
>> PTP resources is negotiated, virtchnl messages are used to access
>> PTP clock and to read the timestamp values.
>> 
>> Virtchnl API covers both modes and exposes a set of PTP capabilities.
>> 
>> Using virtchnl API, the driver recognizes also HW abilities - maximum
>> adjustment of the clock and the basic increment value.
>> 
>> Additionally, API allows to configure the secondary mailbox, dedicated
>> exclusively for PTP purposes.
>> 
>> Reviewed-by: Alexander Lobakin <aleksander.lobakin@...el.com>
>> Reviewed-by: Willem de Bruijn <willemb@...gle.com>
>> Signed-off-by: Milena Olech <milena.olech@...el.com>
>> Tested-by: Mina Almasry <almasrymina@...gle.com>
>> Tested-by: Samuel Salin <Samuel.salin@...el.com>
>
>
>Couple of comments, but no real objection. I think the decisions here
>are acceptable trade offs.
>
>Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
>
>> +/**
>> + * struct virtchnl2_ptp_get_cross_time: Associated with message
>> + *					VIRTCHNL2_OP_PTP_GET_CROSS_TIME.
>> + * @sys_time_ns: System counter value expressed in nanoseconds, read
>> + *		 synchronously with device time
>> + * @dev_time_ns: Device clock time value expressed in nanoseconds
>> + *
>> + * PF/VF sends this message to receive the cross time.
>> + */
>> +struct virtchnl2_ptp_get_cross_time {
>> +	__le64 sys_time_ns;
>> +	__le64 dev_time_ns;
>> +};
>
>These are in nano seconds, and there's no room left for extension in the
>structure.. However, 64bits of nanoseconds is 584 years give or take.
>Even if we start from the Unix epoch thats a pretty long way in the
>future. Additionally, it is likely that some sort of software-based
>rollover could be used since the roll-over period would be hundreds of
>years. Ok. I don't think we need to waste additional space for extension
>here. This also applies to the other __le64 fields with nanosecond time.

Right, I've also considered it during creating virtchnl API, but at the
end of the day I agree that we don't need to waste space for extensions.

>
>> +VIRTCHNL2_CHECK_STRUCT_LEN(16, virtchnl2_ptp_get_cross_time);
>> +
>> +/**
>> + * struct virtchnl2_ptp_set_dev_clk_time: Associated with message
>> + *					  VIRTCHNL2_OP_PTP_SET_DEV_CLK_TIME.
>> + * @dev_time_ns: Device time value expressed in nanoseconds to set
>> + *
>> + * PF/VF sends this message to set the time of the main timer.
>> + */
>> +struct virtchnl2_ptp_set_dev_clk_time {
>> +	__le64 dev_time_ns;
>> +};
>> +VIRTCHNL2_CHECK_STRUCT_LEN(8, virtchnl2_ptp_set_dev_clk_time);
>> +
>> +/**
>> + * struct virtchnl2_ptp_adj_dev_clk_fine: Associated with message
>> + *					  VIRTCHNL2_OP_PTP_ADJ_DEV_CLK_FINE.
>> + * @incval: Source timer increment value per clock cycle
>> + *
>> + * PF/VF sends this message to adjust the frequency of the main timer by the
>> + * indicated scaled ppm.
>> + */
>
>Do we want to encode scaled_ppm here in the virtchnl interface? I
>suppose its not that big a deal but it is kind of an implementation
>quirk of the Linux APIs. We could use parts per trillion or something
>similar..
>
>I suppose there is little value in translating from scaled_ppm to some
>other format, due to accumulated error, and scaled_ppm is higher
>precision than ppb. Ok.

I'm not sure I fully understand your concern, but you think that we
could use another naming convention, or provide to control plane raw
scaled ppm value?

Please notice that in current shape, we negotiate also basic increment
value in PTP capabilities, to adjust scaled ppm - as it is done in any
other product - and then the diff is sent through virtchnl message.

Thanks,
Milena