Re: [PATCH RFC 10/10] common: convert vCPU info area registration

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2022 16:53, Julien Grall wrote:
> On 19/10/2022 08:45, Jan Beulich wrote:
>> Switch to using map_guest_area(). Noteworthy differences from
>> map_vcpu_info():
>> - remote vCPU-s are paused rather than checked for being down (which in
>>    principle can change right after the check),
>> - the domain lock is taken for a much smaller region,
>> - areas could now be registered more than once, if we want this,
>> - as a corner case registering the area at GFN 0 offset 0 is no longer
>>    possible (this is considered an invalid de-registration request).
>>
>> Note that this eliminates a bug in copy_vcpu_settings(): The function
>> did allocate a new page regardless of the GFN already having a mapping,
>> thus in particular breaking the case of two vCPU-s having their info
>> areas on the same page.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> RFC: While the "no re-registration" check is retained, it is now racy.
>>       If we really think it needs retaining _and_ properly enforcing,
>>       then locking will be needed, but I don't think we can hold the
>>       domain lock around a call to map_guest_area(), first and foremost
>>       because of its use of vcpu_pause().
> 
> function like evtchn_2l_unmask() may access vcpu_info that is not the 
> current one.
> 
> So I believe the check needs to be reatined and properly enforced. 
> Otherwise, the old structure may still be used for a short time even if 
> it has been freed.

Good point, I'll try to come up with something suitable.

>> RFC: Is the GFN 0 offset 0 behavioral change deemed acceptable?
> 
> I would say n  (See the previous discussion for more details).

Of course - with 0 going to be replaced in map_guest_area(), the check
there needs to be adjusted accordingly. And ~0 was already invalid to
pass in here, due to the alignment check.

>> RFC: To have just a single instance of casts to vcpu_info_t *,
>>       introducing a macro (or inline function if header dependencies
>>       permit) might be nice. However, the only sensible identifier for it
>>       would imo be vcpu_info(). Yet we already have a macro of that name.
>>       With some trickery it might be possible to overload the macro
>>       (making the "field" argument optional), but this may not be
>>       desirable for other reasons (it could e.g. be deemed confusing).
> 
> You might be able to reduce the number of cast by using local variables.
> 
> But it is not entirely clear why we can't use the existing vcpu_info() 
> in many of the cases. For instance...
> 
> 
>>
>> --- a/xen/arch/x86/include/asm/shared.h
>> +++ b/xen/arch/x86/include/asm/shared.h
>> @@ -27,16 +27,16 @@ static inline void arch_set_##field(stru
>>   static inline type arch_get_##field(const struct vcpu *v)       \
>>   {                                                               \
>>       return !has_32bit_shinfo(v->domain) ?                       \
>> -           v->vcpu_info->native.arch.field :                    \
>> -           v->vcpu_info->compat.arch.field;                     \
>> +           ((const vcpu_info_t *)v->vcpu_info_area.map)->native.arch.field 
>> : \
>> +           ((const vcpu_info_t *)v->vcpu_info_area.map)->compat.arch.field; 
>>  \
> 
> ... here.

vcpu_info() has a property which gets in the way of using the macro
here. Since __vcpu_info() wants to return something which can also
be used as lvalue, the 2nd and 3rd operands of the conditional
operator need to resolve to the same pointer type. Hence the smaller
(compat) type is the only one which is safe to use. See the comment
next to __shared_info()'s definition (which is the one __vcpu_info()'s
refers to).

Jan