Re: [XEN PATCH v1] arm: introduce kconfig options to disable hypercalls

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.12.2024 12:43, Sergiy Kibrik wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -516,4 +516,31 @@ config TRACEBUFFER
>         to be collected at run time for debugging or performance analysis.
>         Memory and execution overhead when not active is minimal.
>  
> +menu "Supported hypercall interfaces"
> +     visible if DOM0LESS_BOOT && EXPERT
> +
> +config SYSCTL
> +     bool "Enable sysctl hypercall"
> +     default y
> +
> +config DOMCTL
> +     bool "Enable domctl hypercalls"
> +     default y
> +
> +config HVM_OP
> +     bool "Enable HVM hypercalls"
> +     depends on HVM
> +     default y
> +
> +config PLATFORM_HYP
> +     bool "Enable platform hypercalls"
> +     depends on !PV_SHIM_EXCLUSIVE

Any reason you don't do the shim related conversion also for domctl and
sysctl?

Much like you have HVM_OP, may I suggest PLATFORM_OP here and ...

> +     default y
> +
> +config PHYSDEVOP
> +     bool "Enable physdev hypercall"
> +     default y

... PHYSDEV_OP here?

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1053,7 +1053,9 @@ int domain_kill(struct domain *d)
>          d->is_dying = DOMDYING_dying;
>          rspin_barrier(&d->domain_lock);
>          argo_destroy(d);
> +#ifdef CONFIG_DOMCTL
>          vnuma_destroy(d->vnuma);
> +#endif

There is a stub already for this, just that right now it's shim-specific.

> --- a/xen/include/hypercall-defs.c
> +++ b/xen/include/hypercall-defs.c
> @@ -234,7 +234,7 @@ stack_switch                       do:2     do:2     -    
>     -        -
>  set_callbacks                      compat   do       -        -        -
>  fpu_taskswitch                     do       do       -        -        -
>  sched_op_compat                    do       do       -        -        dep
> -#ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +#if defined(CONFIG_PLATFORM_HYP)

Nit: Why not #ifdef, like it was, and like you have it ...

> @@ -247,7 +247,9 @@ set_timer_op                       compat   do       
> compat   do       -
>  event_channel_op_compat            do       do       -        -        dep
>  xen_version                        do       do       do       do       do
>  console_io                         do       do       do       do       do
> +#ifdef CONFIG_PHYSDEV
>  physdev_op_compat                  compat   do       -        -        dep
> +#endif
>  #if defined(CONFIG_GRANT_TABLE)
>  grant_table_op                     compat   do       hvm      hvm      do
>  #elif defined(CONFIG_PV_SHIM)
> @@ -269,14 +271,20 @@ callback_op                        compat   do       -  
>       -        -
>  xenoprof_op                        compat   do       -        -        -
>  #endif
>  event_channel_op                   do       do       do:1     do:1     do:1
> +#ifdef CONFIG_PHYSDEVOP
>  physdev_op                         compat   do       hvm      hvm      do_arm
> -#ifdef CONFIG_HVM
> +#endif
> +#ifdef CONFIG_HVM_OP
>  hvm_op                             do       do       do       do       do
>  #endif
>  #ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +#ifdef CONFIG_SYSCTL
>  sysctl                             do       do       do       do       do
> +#endif
> +#ifdef CONFIG_DOMCTL
>  domctl                             do       do       do       do       do
>  #endif
> +#endif
>  #ifdef CONFIG_KEXEC
>  kexec_op                           compat   do       -        -        -
>  #endif
> @@ -293,7 +301,9 @@ hypfs_op                           do       do       do   
>     do       do
>  #endif
>  mca                                do       do       -        -        -
>  #ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +#ifdef CONFIG_DOMCTL
>  paging_domctl_cont                 do       do       do       do       -
>  #endif
> +#endif

... everywhere else?

> --- a/xen/include/xen/hypercall.h
> +++ b/xen/include/xen/hypercall.h
> @@ -24,6 +24,18 @@
>  /* Needs to be after asm/hypercall.h. */
>  #include <xen/hypercall-defs.h>
>  
> +#if !defined(CONFIG_DOMCTL) && !defined(CONFIG_DOM0LESS_BOOT)
> +#error "domctl and dom0less can't be disabled simultaneously"
> +#endif
> +
> +#if !defined(CONFIG_PHYSDEVOP) && !defined(CONFIG_DOM0LESS_BOOT)
> +#error "physdevop and dom0less can't be disabled simultaneously"
> +#endif
> +
> +#if !defined(CONFIG_SYSCTL) && !defined(CONFIG_DOM0LESS_BOOT)
> +#error "sysctl and dom0less can't be disabled simultaneously"
> +#endif

I'm puzzled by this: It covers only 3 of the 5, and it really only
re-checks what Kconfig already enforces.

Jan