Re: [PATCH net-next v8 3/3] gve: implement PTP gettimex64

[Thomas Gleixner <tglx@kernel.org>]

On Fri, May 22 2026 at 01:12, David Woodhouse wrote:
> On Fri, 2026-05-22 at 00:43 +0200, Thomas Gleixner wrote:
>> > So surely a device can only support this if it *does* have a precise
>> > hardware timestamp, e.g. from ART? And the corresponding sys_realtime
>> > and sys_monoraw fields also have to be from the *same* value, surely?
>> 
>> That is correct, but you still fail to explain how this new made up
>> att.counter_id/att.counter_value is filled in coherently without the
>> driver doing something abysmal?
>>
>> You can't explain it, because the only way to do that correctly is by
>> extending the cross time stamp struct and get_device_system_crosststamp().
>
> Yes. Isn't that what I asked Arthur to do?

You maybe asked him, but I can't see any evidence of it anywhere. All I
can see is a magic struct attr, which conveys CS related data that is
supposed to be produced magically at some undefined place.

>> > Is that not how it works? If not.... WTF *is* it for?
>> 
>> Yes, that's the way it works since get_device_system_crosststamp() got
>> introduced, but that does not give a random driver access to the actual
>> converted (TSC) counter value. The driver can only observe the PTM value
>> which is ART on x86 and whatever on other architectures. So it does not
>> know anything about it.
>
> Sure, the driver can only return what it *knows*.
>
> We're certainly missing some infrastructure here... maybe the pci_bus
> should have a CSID which corresponds to its PTM domain, which on x86
> would be CSID_X86_ART?

Yes, that's correct. But that does not solve the underlying problem:

The driver gets the PTP/SystemTime pair from the hardware/firmware,
which is what it hands back to get_device_system_crosststamp(). What
does SystemTime mean?

Here it gets interesting as that depends on the NIC/PTP magic firmware
and the architecture:

   On X86:

     - The plain ART value, which is converted to TSC in
       get_device_system_crosststamp().

     - The ART value which is magically converted to "nanoseconds" by
       the firmware.

    That is conveyed in system_counterval_t and the 'use_nsec' flag,
    which is a misnomer, determines the conversion in the core.
   
     - If set it uses the nominal frequency of the underlying base clock
       (ART) for conversion.
   
     - If not it uses the nominator/denominator pair of the underlying base
       clock (ART) for conversion.
   
   On ARM64:
   
     - The ARM ARCH counter value (however that is converted from PTM
       magically)
   
     - The ARM ARCH counter which is magically converted to "nanoseconds"
       by the firmware. Actually it's not, but the drivers claim it is.
   
     It does not matter because the core does no conversion for ARM64 as
     the SystemTime CSID is the same as the core timekeeper CSID.

Q: How is a driver supposed to fill in attr->cs_cycles coherently?

A: Not at all.

The consequence of this is that the next driver writer will just go and
do:

       attr->cs_id = IS_ENABLED(X86) ? CSID_X86_TSC : CSID_ARM_COUNTER;
       attr->cs_cycles = get_cycles();
   or
       attr->cs_cycles = ptm_cycles * pulledout_of_thin_air_factor +
                         pulledout_of_thin_air_offset;

and this mess is going to proliferate faster than you can breathe.

The same applies to the pre/post timestamp mechanism for
gettimexattrs64().

These new attr IOCTLs are not restricted to your favorite vmclock PTP
toy. They are available for every PTP driver which implements them, but
there is no infrastructure to actually provide the required data. That
ENA driver implementing gettimexattrs64() is demonstrating it. It at
least does not try to make them up, but that's just a matter of time to
happen.

I know you don't care because that's outside of your sandpit, but I care
because providing this interface is going to result in random drivers
growing the most absurd warts just to work around the lack of core
infrastructure.

Are you going to monitor drivers/net/ for those instances and make sure
they are burned before seeing the light of day?

Surely not and neither is netdev going to care as demonstrated with
this GEV mess.

It took me half an hour to add basic support for that into the core
infrastructure, but this new IOCTL stuff has been in the making for
almost a year without anyone spending a single brain cycle to even think
about it.

And you just have proven it by still claiming that this is a completely
cosmetic problem.

Seriously?

>> I agree, but the way how this attr.counter_id/value extension is defined
>> either requires unholy hacks to get access to the ART/TSC conversion or
>> resort to get_cycles().

> Never get_cycles(). Let's perhaps make it *not* need unholy hacks to
> interpret a PTM counter value. Otherwise what's the point in ART and
> PTM at all?

Perhaps? There is no perhaps and aside of that I gave you the solution
for the problem already in the mail you just replied to, no?

I'm glad we agree on "Never get_cycles()" at least. May I ask you then
to get a stiff drink ready and run:

  # git blame drivers/ptp/ptp_vmclock.c | grep get_cycles

:)

Thanks,

        tglx