Re: [PATCH v3 7/8] x86: introduce GADDR based secondary time area registration alternative

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2023 17:50, Roger Pau Monné wrote:
> On Wed, May 03, 2023 at 05:58:01PM +0200, Jan Beulich wrote:
>> The registration by virtual/linear address has downsides: The access is
>> expensive for HVM/PVH domains. Furthermore for 64-bit PV domains the area
>> is inaccessible (and hence cannot be updated by Xen) when in guest-user
>> mode.
>>
>> Introduce a new vCPU operation allowing to register the secondary time
>> area by guest-physical address.
>>
>> An at least theoretical downside to using physically registered areas is
>> that PV then won't see dirty (and perhaps also accessed) bits set in its
>> respective page table entries.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

>> ---
>> v3: Re-base.
>> v2: Forge version in force_update_secondary_system_time().

For your question below, note this revision log entry. I didn't have the
compensation originally, and my made-up XTF test thus failed.

>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1633,6 +1633,16 @@ void force_update_vcpu_system_time(struc
>>      __update_vcpu_system_time(v, 1);
>>  }
>>  
>> +void force_update_secondary_system_time(struct vcpu *v,
>> +                                        struct vcpu_time_info *map)
>> +{
>> +    struct vcpu_time_info u;
>> +
>> +    collect_time_info(v, &u);
>> +    u.version = -1; /* Compensate for version_update_end(). */
> 
> Hm, we do not seem to compensate in
> VCPUOP_register_vcpu_time_memory_area, what's more, in that case the
> initial version is picked from the contents of the guest address.
> Hopefully the guest will have zeroed the memory.
> 
> FWIW, I would be fine with leaving this at 0, so the first version
> guest sees is 1.

No, we can't. Odd numbers mean "update in progress". In
__update_vcpu_system_time() we properly use version_update_begin(),
so there's no need for any compensation. If the guest puts a non-zero
value there, that's also fine from the perspective of the protocol.
Just that if the initial value is odd, the guest will mislead itself
into thinking "oh, the hypervisor is updating this right now" until
the first real update has happened. Yet even that is hypothetical,
since upon registration the area is first populated, so upon the
registration hypercall returning the field will already properly have
an even value.

Jan