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

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 9086 bytes --]

On 24.03.22 15:01, 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@suse.com>
> Reviewed-by: Julien Grall <jgrall@amazon.com>

There is still an Ack missing for x86 side. Jan already said he isn't
happy to give one, but won't Nack it. Roger, Andrew, any comments for
this patch? It is blocking further cleanup patches to go in (patches
2 and 4 of this series).


Juergen

> ---
> 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;
> +
>       switch ( cmd )
>       {
>           case VCPUOP_register_vcpu_info:
>           case VCPUOP_register_runstate_memory_area:
> -            return do_vcpu_op(cmd, vcpuid, arg);
> +            return common_vcpu_op(cmd, v, arg);
>           default:
>               return -EINVAL;
>       }
>   }
>   
> -long arch_do_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
> -{
> -    return -ENOSYS;
> -}
> -
>   void arch_dump_vcpu_info(struct vcpu *v)
>   {
>       gic_dump_info(v);
> diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h
> index 39d2e7889d..fac4d60f17 100644
> --- a/xen/arch/arm/include/asm/hypercall.h
> +++ b/xen/arch/arm/include/asm/hypercall.h
> @@ -4,8 +4,6 @@
>   #include <public/domctl.h> /* for arch_do_domctl */
>   int do_arm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
>   
> -long do_arm_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg);
> -
>   long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>   
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 43f30747cf..e906bb4a89 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1380,7 +1380,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
>   #endif
>       HYPERCALL(multicall, 2),
>       HYPERCALL(platform_op, 1),
> -    HYPERCALL_ARM(vcpu_op, 3),
> +    HYPERCALL(vcpu_op, 3),
>       HYPERCALL(vm_assist, 2),
>   #ifdef CONFIG_ARGO
>       HYPERCALL(argo_op, 5),
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index a5048ed654..d566fc82b4 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1489,11 +1489,15 @@ int arch_vcpu_reset(struct vcpu *v)
>       return 0;
>   }
>   
> -long
> -arch_do_vcpu_op(
> -    int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
> +long cf_check do_vcpu_op(int cmd, unsigned int vcpuid,
> +                         XEN_GUEST_HANDLE_PARAM(void) arg)
>   {
>       long rc = 0;
> +    struct domain *d = current->domain;
> +    struct vcpu *v;
> +
> +    if ( (v = domain_vcpu(d, vcpuid)) == NULL )
> +        return -ENOENT;
>   
>       switch ( cmd )
>       {
> @@ -1545,7 +1549,7 @@ arch_do_vcpu_op(
>       }
>   
>       default:
> -        rc = -ENOSYS;
> +        rc = common_vcpu_op(cmd, v, arg);
>           break;
>       }
>   
> diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
> index 61bf897147..d6daa7e4cb 100644
> --- a/xen/arch/x86/include/asm/hypercall.h
> +++ b/xen/arch/x86/include/asm/hypercall.h
> @@ -145,7 +145,7 @@ compat_physdev_op(
>       XEN_GUEST_HANDLE_PARAM(void) arg);
>   
>   extern int
> -arch_compat_vcpu_op(
> +compat_common_vcpu_op(
>       int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg);
>   
>   extern int cf_check compat_mmuext_op(
> diff --git a/xen/arch/x86/x86_64/domain.c b/xen/arch/x86/x86_64/domain.c
> index c46dccc25a..9c559aa3ea 100644
> --- a/xen/arch/x86/x86_64/domain.c
> +++ b/xen/arch/x86/x86_64/domain.c
> @@ -12,11 +12,15 @@
>   CHECK_vcpu_get_physid;
>   #undef xen_vcpu_get_physid
>   
> -int
> -arch_compat_vcpu_op(
> -    int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
> +int cf_check
> +compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
>   {
> -    int rc = -ENOSYS;
> +    int rc;
> +    struct domain *d = current->domain;
> +    struct vcpu *v;
> +
> +    if ( (v = domain_vcpu(d, vcpuid)) == NULL )
> +        return -ENOENT;
>   
>       switch ( cmd )
>       {
> @@ -55,7 +59,11 @@ arch_compat_vcpu_op(
>       }
>   
>       case VCPUOP_get_physid:
> -        rc = arch_do_vcpu_op(cmd, v, arg);
> +        rc = do_vcpu_op(cmd, vcpuid, arg);
> +        break;
> +
> +    default:
> +        rc = compat_common_vcpu_op(cmd, v, arg);
>           break;
>       }
>   
> diff --git a/xen/common/compat/domain.c b/xen/common/compat/domain.c
> index afae27eeba..1119534679 100644
> --- a/xen/common/compat/domain.c
> +++ b/xen/common/compat/domain.c
> @@ -38,15 +38,12 @@ CHECK_vcpu_hvm_context;
>   
>   #endif
>   
> -int cf_check compat_vcpu_op(
> -    int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
> +int compat_common_vcpu_op(int cmd, struct vcpu *v,
> +                          XEN_GUEST_HANDLE_PARAM(void) arg)
>   {
> -    struct domain *d = current->domain;
> -    struct vcpu *v;
>       int rc = 0;
> -
> -    if ( (v = domain_vcpu(d, vcpuid)) == NULL )
> -        return -ENOENT;
> +    struct domain *d = current->domain;
> +    unsigned int vcpuid = v->vcpu_id;
>   
>       switch ( cmd )
>       {
> @@ -103,7 +100,7 @@ int cf_check compat_vcpu_op(
>       case VCPUOP_stop_singleshot_timer:
>       case VCPUOP_register_vcpu_info:
>       case VCPUOP_send_nmi:
> -        rc = do_vcpu_op(cmd, vcpuid, arg);
> +        rc = common_vcpu_op(cmd, v, arg);
>           break;
>   
>       case VCPUOP_get_runstate_info:
> @@ -134,7 +131,7 @@ int cf_check compat_vcpu_op(
>       }
>   
>       default:
> -        rc = arch_compat_vcpu_op(cmd, v, arg);
> +        rc = -ENOSYS;
>           break;
>       }
>   
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 351029f8b2..70747c02e6 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1570,15 +1570,11 @@ int default_initialise_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
>       return rc;
>   }
>   
> -long cf_check do_vcpu_op(
> -    int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
> +long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
>   {
> -    struct domain *d = current->domain;
> -    struct vcpu *v;
>       long rc = 0;
> -
> -    if ( (v = domain_vcpu(d, vcpuid)) == NULL )
> -        return -ENOENT;
> +    struct domain *d = v->domain;
> +    unsigned int vcpuid = v->vcpu_id;
>   
>       switch ( cmd )
>       {
> @@ -1750,7 +1746,7 @@ long cf_check do_vcpu_op(
>       }
>   
>       default:
> -        rc = arch_do_vcpu_op(cmd, v, arg);
> +        rc = -ENOSYS;
>           break;
>       }
>   
> diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
> index a1b6575976..81aae7a662 100644
> --- a/xen/include/xen/hypercall.h
> +++ b/xen/include/xen/hypercall.h
> @@ -110,7 +110,7 @@ do_vcpu_op(
>   
>   struct vcpu;
>   extern long
> -arch_do_vcpu_op(int cmd,
> +common_vcpu_op(int cmd,
>       struct vcpu *v,
>       XEN_GUEST_HANDLE_PARAM(void) arg);
>   


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]