On 20.01.2026 16:27, Tu Dinh wrote:
> On 20/01/2026 13:42, Jan Beulich wrote:
>> On 20.01.2026 13:12, Tu Dinh wrote:
>>> On 20/01/2026 11:35, Jan Beulich wrote:
>>>> On 20.01.2026 10:57, Tu Dinh wrote:
>>>>> time_offset is currently always added to wc_sec. This means that without
>>>>> the actual value of time_offset, guests have no way of knowing what's
>>>>> the actual host clock. Once the guest clock drifts beyond 1 second,
>>>>> updates to the guest RTC would themselves change time_offset and make it
>>>>> impossible to resync guest time to host time.
>>>>
>>>> Despite my earlier comments this part of the description looks unchanged.
>>>> I still don't see why host time (or in fact about any host property) should
>>>> be exposed to guests.
>>>
>>> I've answered this question in a followup reply from November, which
>>> I'll reproduce here:
>>
>> I did read your reply, yet nothing of it appeared here as additional
>> justification.
>
> Is the new description OK for you?
Which new description? So far I only saw your responses to my questions, not
an updated patch description.
>> Plus I fear I don't view any of this a basis to suggest
>> to expose some host property to guests.
>
> The only host property being exposed would be the UTC wallclock as kept
> track by the host (as is specified by XENPF_settime). This information
> (wallclock from an external reference) is necessary for guest timesync,
> whereas an RTC which guests can update by themselves simply cannot be
> used for this purpose.
What the guest can do to its (virtual) RTC is no different from what an OS
running bare metal can do to the real RTC. Running on bare metal, you also
don't have any other reference (without using e.g. NTP).
>>>>> Since there's no way to add more fields to struct shared_info, the
>>>>> addition has to be done through struct arch_shared_info instead. Add two
>>>>> fields in arch_shared_info representing time_offset's low and high
>>>>> 32-bit halves.
>>>>
>>>> Again, despite my earlier question, reasoning of why two halves rather than
>>>> a (signed) 64-bit value isn't supplied here.
>>>
>>> This was also in my last email:
>>>
>>> Both are just for easy consumption of the time offset on 32-bit guests.
>>
>> I don't buy this. I should probably have replied to this effect when
>> you first wrote it. {,u}int64_t is hardly a hurdle anymore there. Nor
>> would I expect any halfway up-to-date 32-bit guest to manage time as
>> a 32-bit quantity anymore.
>>
>>> Unsigned is particularly because these are only parts of an int64_t (and
>>> therefore have no signedness themselves) and I prefer to let the
>>> conversion happen after reading the two fields.
>>
>> There may be benefits to this, yes, but imo they want to be spelled out,
>> rather than left vague.
>>
>>> (Follow up: Also, the alignment of int64_t differs between GCC and MSVC
>>> compilers. Using int64_t here would change the alignment of struct
>>> arch_shared_info)
>>
>> Does it? For which target and in which way? This would, after all, render
>> other uses of {,u}int64_t in the public headers problematic as well.
>
> For the x86 32-bit target, the Windows ABI uses 8-byte alignment for
> (u)int64_t as opposed to 4-byte for the System V ABI [1].
Oh, right, I should have recalled this. Iirc there was an unwritten assumption
that for Windows the public headers may need some massaging.
> Most of the
> other uses of 64-bit integers look to be manually aligned and/or using
> (u)int64_aligned_t (I haven't looked at them all). I can switch
> time_offset to int64_aligned_t and avoid the issues above.
Except that int64_aligned_t can be used in __XEN_TOOLS__ guarded areas only,
for not being possible to make available with plain C89 / C99. So here we're
working out a reason why the field may indeed better be split. Albeit as
said, other areas of the public headers use {,u}int64_t as well, so maybe
this still would only be a pretty weak reason (and you could make sure the
field is properly padded for the x86-32 case).
Jan
