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