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: <754e6414-cbee-4216-9fe9-36c468d01244@intel.com>
Date: Tue, 8 Apr 2025 14:12:08 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Milena Olech <milena.olech@...el.com>, <intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, <anthony.l.nguyen@...el.com>,
	<przemyslaw.kitszel@...el.com>, Alexander Lobakin
	<aleksander.lobakin@...el.com>, Willem de Bruijn <willemb@...gle.com>, "Mina
 Almasry" <almasrymina@...gle.com>, Samuel Salin <Samuel.salin@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH v10 iwl-next 03/11] virtchnl: add PTP
 virtchnl definitions



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.

> +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.