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

[Julien Grall <julien@xxxxxxx>]

Hi,

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.
> RFC: Is the GFN 0 offset 0 behavioral change deemed acceptable?
I would say n  (See the previous discussion for more details).

> 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.

>   }                                                               \
>   static inline void arch_set_##field(struct vcpu *v,             \
>                                       type val)                   \
>   {                                                               \
>       if ( !has_32bit_shinfo(v->domain) )                         \
> -        v->vcpu_info->native.arch.field = val;                  \
> +        ((vcpu_info_t *)v->vcpu_info_area.map)->native.arch.field = val; \
>       else                                                        \
> -        v->vcpu_info->compat.arch.field = val;                  \
> +        ((vcpu_info_t *)v->vcpu_info_area.map)->compat.arch.field = val; \
Cheers,

--
Julien Grall