Re: [PATCH 2/2][4.17?] x86: wire up VCPUOP_register_vcpu_time_memory_area for 32-bit guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.09.2022 13:37, Roger Pau Monné wrote:
> On Thu, Sep 29, 2022 at 11:51:40AM +0200, Jan Beulich wrote:
>> Forever sinced its introduction VCPUOP_register_vcpu_time_memory_area
>> was available only to native domains. Linux, for example, would attempt
>> to use it irrespective of guest bitness (including in its so called
>> PVHVM mode) as long as it finds XEN_PVCLOCK_TSC_STABLE_BIT set (which we
>> set only for clocksource=tsc, which in turn needs engaging via command
>> line option).
>>
>> Fixes: a5d39947cb89 ("Allow guests to register secondary vcpu_time_info")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

> Albeit I have concerns with the notes you raise below, not sure we
> also want to introduce a (broken') compat version of the same
> hypercall wrt v != current.

Since "compat" is ambiguous in this context - I guess you don't mean
just the variant for compat guests? Independent of that I'm afraid I
don't see how a separate variant would help: If the cross-vCPU
copying is not okay, then existing users as affected already, and
would rather have the single variant work correctly. And if the newly
added variant would be the one with the broken behavior, why would
anyone switch to using it?

>> ---
>> Is it actually correct for us to do cross-vCPU updates of the area here
>> (and also in the native counterpart as well as for runstate area
>> updates)? The virtual address may be valid for the given vCPU only, but
>> may be mapped to something else on the current vCPU (yet we only deal
>> with it not being mapped at all). Note how HVM code already calls
>> update_vcpu_system_time() only when v == current.
>>
>> I'm surprised by Linux not using the secondary area in a broader
>> fashion. But I'm also surprised that they would only ever register an
>> area for vCPU 0.
> 
> Would be better to update locally just when v == current, otherwise
> issue an IPI to the remote vCPU dirty mask and force an update on
> resume to guest path?

Yes, that's the outline of how to deal with this _if_ we determine the
present behavior is flawed. Determination is difficult since, like with
so many things, this is something that's not spelled out anywhere.

>> --- a/xen/arch/x86/x86_64/domain.c
>> +++ b/xen/arch/x86/x86_64/domain.c
>> @@ -58,6 +58,26 @@ compat_vcpu_op(int cmd, unsigned int vcp
>>          break;
>>      }
>>  
>> +    case VCPUOP_register_vcpu_time_memory_area:
>> +    {
>> +        struct compat_vcpu_register_time_memory_area area = { .addr.p = 0 };
> 
> Why not just use { } to initialize?

I wanted to match (a) the VCPUOP_register_runstate_memory_area handling
further up, just without using a separate assignment and (b) this line

        if ( area.addr.h.c != area.addr.p ||

And yes, initially I did consider using just { }.

Jan