Re: [PATCH 06/12] xen: generate hypercall interface related code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2021 14:51, Juergen Gross wrote:
> Instead of repeating similar data multiple times use a single source
> file and a generator script for producing prototypes and call sequences
> of the hypercalls.
> 
> As the script already knows the number of parameters used add generating
> a macro for populating an array with the number of parameters per
> hypercall.

Isn't that array intended to go away?

> --- a/.gitignore
> +++ b/.gitignore
> @@ -332,10 +332,12 @@ xen/include/asm-x86/asm-macros.h
>  xen/include/compat/*
>  xen/include/config/
>  xen/include/generated/
> +xen/include//hypercall-defs.i

Nit: Stray double slash (unless this has a meaning I'm unaware of).

Yet then I wonder: Shouldn't *.i be among the patterns at the top of
the file, like *.o is?

> @@ -466,6 +468,14 @@ include/asm-$(TARGET_ARCH)/asm-offsets.h: asm-offsets.s
>         echo ""; \
>         echo "#endif") <$< >$@
>  
> +quiet_cmd_genhyp = GEN     $@
> +define cmd_genhyp
> +    awk -f scripts/gen_hypercall.awk <$< >$@
> +endef
> +
> +include/xen/hypercall-defs.h: include/hypercall-defs.i 
> scripts/gen_hypercall.awk FORCE
> +     $(call if_changed,genhyp)

As per patch 5 there are quite a few sources including xen/hypercall.h
and hence (in one of the next patches) the header generated here. If
this one gets re-generated for a benign reason (i.e. without changing
the header), all dependents will get rebuilt for no reason. Use
$(move-if-changed ...)?

> +prefix: do
> +set_timer_op(s_time_t timeout)
> +console_io(unsigned int cmd, unsigned int count, char *buffer)
> +vm_assist(unsigned int cmd, unsigned int type)
> +event_channel_op(int cmd, void *arg)
> +mmuext_op(mmuext_op_t *uops, unsigned int count, unsigned int *pdone, 
> unsigned int foreigndom)
> +multicall(multicall_entry_t *call_list, unsigned int nr_calls)
> +#ifdef CONFIG_PV
> +mmu_update(mmu_update_t *ureqs, unsigned int count, unsigned int *pdone, 
> unsigned int foreigndom)
> +stack_switch(unsigned long ss, unsigned long esp)
> +fpu_taskswitch(int set)
> +set_debugreg(int reg, unsigned long value)
> +get_debugreg(int reg)
> +set_segment_base(unsigned int which, unsigned long base)
> +mca(xen_mc_t *u_xen_mc)
> +set_trap_table(const_trap_info_t *traps)
> +set_gdt(xen_ulong_t *frame_list, unsigned int entries)
> +set_callbacks(unsigned long event_address, unsigned long failsafe_address, 
> unsigned long syscall_address)
> +update_descriptor(uint64_t gaddr, seg_desc_t desc)
> +update_va_mapping(unsigned long va, uint64_t val64, unsigned long flags)
> +update_va_mapping_otherdomain(unsigned long va, uint64_t val64, unsigned 
> long flags, domid_t domid)
> +#endif
> +#ifdef CONFIG_IOREQ_SERVER
> +dm_op(domid_t domid, unsigned int nr_bufs, xen_dm_op_buf_t *bufs)
> +#endif
> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +sysctl(xen_sysctl_t *u_sysctl)
> +domctl(xen_domctl_t *u_domctl)
> +paging_domctl_cont(xen_domctl_t *u_domctl)
> +#endif
> +#ifdef CONFIG_HVM
> +hvm_op(unsigned long op, void *arg)
> +#endif
> +#ifdef CONFIG_HYPFS
> +hypfs_op(unsigned int cmd, const char *arg1, unsigned long arg2, void *arg3, 
> unsigned long arg4)
> +#endif
> +#ifdef CONFIG_X86
> +xenpmu_op(unsigned int op, xen_pmu_params_t *arg)
> +#endif
> +
> +#ifdef CONFIG_PV
> +caller: pv64
> +#ifdef CONFIG_PV32
> +caller: pv32
> +#endif
> +#endif
> +#ifdef CONFIG_HVM
> +caller: hvm64
> +#ifdef CONFIG_COMPAT
> +caller: hvm32
> +#endif

HVM selects COMPAT, so I don't see a point in this inner conditional.

> +#endif
> +#ifdef CONFIG_ARM
> +caller: arm
> +#endif
> +
> +table:                             pv32    pv64    hvm32   hvm64   arm
> +set_trap_table                     compat  do      -       -       -
> +mmu_update                         do      do      -       -       -
> +set_gdt                            compat  do      -       -       -
> +stack_switch                       do      do      -       -       -
> +set_callbacks                      compat  do      -       -       -
> +fpu_taskswitch                     do      do      -       -       -
> +sched_op_compat                    do      do      -       -       dep
> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +platform_op                        compat  do      compat  do      do
> +#endif
> +set_debugreg                       do      do      -       -       -
> +get_debugreg                       do      do      -       -       -
> +update_descriptor                  compat  do      -       -       -
> +memory_op                          compat  do      hvm     hvm     do
> +multicall                          compat  do      compat  do      do
> +update_va_mapping                  compat  do      -       -       -
> +set_timer_op                       compat  do      compat  do      -
> +event_channel_op_compat            do      do      -       -       dep
> +xen_version                        compat  do      compat  do      do
> +console_io                         do      do      do      do      do
> +physdev_op_compat                  compat  do      -       -       dep
> +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM)
> +grant_table_op                     compat  do      hvm     hvm     do
> +#endif
> +vm_assist                          do      do      do      do      do
> +update_va_mapping_otherdomain      compat  do      -       -       -
> +iret                               compat  do      -       -       -
> +vcpu_op                            compat  do      compat  do      do
> +set_segment_base                   do      do      -       -       -
> +#ifdef CONFIG_PV
> +mmuext_op                          compat  do      compat  do      -
> +#endif
> +xsm_op                             compat  do      compat  do      do
> +nmi_op                             compat  do      -       -       -
> +sched_op                           compat  do      compat  do      do
> +callback_op                        compat  do      -       -       -
> +#ifdef CONFIG_XENOPROF
> +xenoprof_op                        compat  do      -       -       -
> +#endif
> +event_channel_op                   do      do      do      do      do
> +physdev_op                         compat  do      hvm     hvm     do
> +#ifdef CONFIG_HVM
> +hvm_op                             do      do      do      do      do
> +#endif
> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +sysctl                             do      do      do      do      do
> +domctl                             do      do      do      do      do
> +#endif
> +#ifdef CONFIG_KEXEC
> +kexec_op                           compat  do      -       -       -
> +#endif
> +tmem_op                            -       -       -       -       -
> +#ifdef CONFIG_ARGO
> +argo_op                            compat  do      compat  do      do
> +#endif
> +xenpmu_op                          do      do      do      do      -
> +#ifdef CONFIG_IOREQ_SERVER
> +dm_op                              compat  do      compat  do      do
> +#endif
> +#ifdef CONFIG_HYPFS
> +hypfs_op                           do      do      do      do      do
> +#endif
> +mca                                do      do      -       -       -
> +#ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +paging_domctl_cont                 do      do      do      do      -
> +#endif

I assume the intention here is to sort by hypercall number. This results
in 3 PV_SHIM_EXCLUSIVE conditionals when one might do. Just a remark for
consideration, not strictly a request to change anything.

Jan