Re: [PATCH 13/21] target/arm/kvm: Move kvm_arm_reg_syncs_via_cpreg_list and unexport

[Gavin Shan]

Hi Richard,

On 11/23/23 15:42, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/kvm_arm.h | 10 ----------
>   target/arm/kvm.c     | 23 +++++++++++++++++++++++
>   target/arm/kvm64.c   | 15 ---------------
>   3 files changed, 23 insertions(+), 25 deletions(-)

With the following nits addressed:

Reviewed-by: Gavin Shan <gshan@redhat.com>

> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index 2755ee8366..1043123cc7 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -77,16 +77,6 @@ void kvm_arm_register_device(MemoryRegion *mr, uint64_t 
> devid, uint64_t group,
>    */
>   int kvm_arm_init_cpreg_list(ARMCPU *cpu);
>   
> -/**
> - * kvm_arm_reg_syncs_via_cpreg_list:
> - * @regidx: KVM register index
> - *
> - * Return true if this KVM register should be synchronized via the
> - * cpreg list of arbitrary system registers, false if it is synchronized
> - * by hand using code in kvm_arch_get/put_registers().
> - */
> -bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx);
> -
>   /**
>    * write_list_to_kvmstate:
>    * @cpu: ARMCPU
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index dadc3fd755..9bca6baf35 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -740,6 +740,29 @@ static uint64_t *kvm_arm_get_cpreg_ptr(ARMCPU *cpu, 
> uint64_t regidx)
>       return &cpu->cpreg_values[res - cpu->cpreg_indexes];
>   }
>   
> +/**
> + * kvm_arm_reg_syncs_via_cpreg_list:
> + * @regidx: KVM register index
> + *
> + * Return true if this KVM register should be synchronized via the
> + * cpreg list of arbitrary system registers, false if it is synchronized
> + * by hand using code in kvm_arch_get/put_registers().
> + */
> +static bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx)
> +{
> +    /* Return true if the regidx is a register we should synchronize
> +     * via the cpreg_tuples array (ie is not a core or sve reg that
> +     * we sync by hand in kvm_arch_get/put_registers())
> +     */

The comments are fully or partially duplicate to the function's description 
above.
I think the comments here can be dropped or combined to the function's 
descrption
above.

> +    switch (regidx & KVM_REG_ARM_COPROC_MASK) {
> +    case KVM_REG_ARM_CORE:
> +    case KVM_REG_ARM64_SVE:
> +        return false;
> +    default:
> +        return true;
> +    }
> +}
> +
>   /* Initialize the ARMCPU cpreg list according to the kernel's
>    * definition of what CPU registers it knows about (and throw away
>    * the previous TCG-created cpreg list).
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index a184cca4dc..52c0a6d3af 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -346,21 +346,6 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>       return 0;
>   }
>   
> -bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx)
> -{
> -    /* Return true if the regidx is a register we should synchronize
> -     * via the cpreg_tuples array (ie is not a core or sve reg that
> -     * we sync by hand in kvm_arch_get/put_registers())
> -     */
> -    switch (regidx & KVM_REG_ARM_COPROC_MASK) {
> -    case KVM_REG_ARM_CORE:
> -    case KVM_REG_ARM64_SVE:
> -        return false;
> -    default:
> -        return true;
> -    }
> -}
> -
>   /* Callers must hold the iothread mutex lock */
>   static void kvm_inject_arm_sea(CPUState *c)
>   {

Thanks,
Gavin