Re: [PATCH v6 1/9] xen: move do_vcpu_op() to arch specific code

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 24, 2022 at 03:01:31PM +0100, Juergen Gross wrote:
> The entry point used for the vcpu_op hypercall on Arm is different
> from the one on x86 today, as some of the common sub-ops are not
> supported on Arm. The Arm specific handler filters out the not
> supported sub-ops and then calls the common handler. This leads to the
> weird call hierarchy:
> 
>   do_arm_vcpu_op()
>     do_vcpu_op()
>       arch_do_vcpu_op()
> 
> Clean this up by renaming do_vcpu_op() to common_vcpu_op() and
> arch_do_vcpu_op() in each architecture to do_vcpu_op(). This way one
> of above calls can be avoided without restricting any potential
> future use of common sub-ops for Arm.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxxx>

>From an x86 only PoV I prefer the previous arrangement (do_vcpu_op() ->
arch_do_vcpu_op()), but this approach seems to be better for Arm, so
that's a reasonable argument for changing it.

My preference would have been to move the handling of hypercalls not
used by Arm into arch_do_vcpu_op() for x86, but that's also not widely
liked.

> ---
> V4:
> - don't remove HYPERCALL_ARM()
> V4.1:
> - add missing cf_check (Andrew Cooper)
> V5:
> - use v instead of current (Julien Grall)
> ---
>  xen/arch/arm/domain.c                | 15 ++++++++-------
>  xen/arch/arm/include/asm/hypercall.h |  2 --
>  xen/arch/arm/traps.c                 |  2 +-
>  xen/arch/x86/domain.c                | 12 ++++++++----
>  xen/arch/x86/include/asm/hypercall.h |  2 +-
>  xen/arch/x86/x86_64/domain.c         | 18 +++++++++++++-----
>  xen/common/compat/domain.c           | 15 ++++++---------
>  xen/common/domain.c                  | 12 ++++--------
>  xen/include/xen/hypercall.h          |  2 +-
>  9 files changed, 42 insertions(+), 38 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 8110c1df86..2f8eaab7b5 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -1079,23 +1079,24 @@ void arch_dump_domain_info(struct domain *d)
>  }
>  
>  
> -long do_arm_vcpu_op(int cmd, unsigned int vcpuid, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
> +long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>  {
> +    struct domain *d = current->domain;
> +    struct vcpu *v;
> +
> +    if ( (v = domain_vcpu(d, vcpuid)) == NULL )
> +        return -ENOENT;

My preference (here and in x86) code would be to do the initialization
at definition, and then just check for v here, ie:

struct vcpu *v = domain_vcpu(d, vcpuid);

if ( !v )
    return -ENOENT;

But that's just my taste.

Thanks, Roger.