| Message ID | 20250207110248.1580465-3-cohuck@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: rework id register storage | expand |
On 2/7/25 03:02, Cornelia Huck wrote: > +/* read a 32b sysreg value and store it in the idregs */ > +static int get_host_cpu_reg32(int fd, ARMHostCPUFeatures *ahcf, ARMSysRegs sysreg) > +{ > + int index = get_sysreg_idx(sysreg); > + uint64_t *reg; > + int ret; > + > + if (index < 0) { > + return -ERANGE; > + } > + reg = &ahcf->isar.idregs[index]; > + ret = read_sys_reg32(fd, (uint32_t *)reg, idregs_sysreg_to_kvm_reg(sysreg)); > + return ret; > +} I'm not keen on the casting. If we want to retain read_sys_reg32 at all, then uint32_t tmp; ret = read_sys_reg32(fd, &tmp, idregs_sysreg_to_kvm_reg(sysreg)); if (ret == 0) { ahcf->isar.idregs[index] = tmp; } return ret; That said, read_sys_reg32 does exactly the opposite, using a uint64_t temporary. Therefore I would say that we should simply use read_sys_reg64. r~
On 2/7/25 03:02, Cornelia Huck wrote: > +/* read a 32b sysreg value and store it in the idregs */ > +static int get_host_cpu_reg32(int fd, ARMHostCPUFeatures *ahcf, ARMSysRegs sysreg) > +{ > + int index = get_sysreg_idx(sysreg); > + uint64_t *reg; > + int ret; > + > + if (index < 0) { > + return -ERANGE; > + } > + reg = &ahcf->isar.idregs[index]; > + ret = read_sys_reg32(fd, (uint32_t *)reg, idregs_sysreg_to_kvm_reg(sysreg)); > + return ret; > +} > + > +/* read a 64b sysreg value and store it in the idregs */ > +static int get_host_cpu_reg64(int fd, ARMHostCPUFeatures *ahcf, ARMSysRegs sysreg) > +{ > + int index = get_sysreg_idx(sysreg); Why pass the ARMSysRegs value instead of the ARMIDRegisterIdx value? You save yourself a linear search over the id_register_sysreg array, and you can't use this interface with a sysreg that doesn't have an index anyway -- ERANGE is a new failure mode. r~
Hi Connie On 2/7/25 12:02 PM, Cornelia Huck wrote: > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > target/arm/cpu-sysregs.h | 3 +++ > target/arm/cpu64.c | 25 +++++++++++++++++++++++++ > target/arm/kvm.c | 30 ++++++++++++++++++++++++++++++ > 3 files changed, 58 insertions(+) > > diff --git a/target/arm/cpu-sysregs.h b/target/arm/cpu-sysregs.h > index de09ebae91a5..54a4fadbf0c1 100644 > --- a/target/arm/cpu-sysregs.h > +++ b/target/arm/cpu-sysregs.h > @@ -128,4 +128,7 @@ static const uint32_t id_register_sysreg[NUM_ID_IDX] = { > [CTR_EL0_IDX] = SYS_CTR_EL0, > }; > > +int get_sysreg_idx(ARMSysRegs sysreg); > +uint64_t idregs_sysreg_to_kvm_reg(ARMSysRegs sysreg); > + > #endif /* ARM_CPU_SYSREGS_H */ > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > index 8188ede5cc8a..9ae78253cb34 100644 > --- a/target/arm/cpu64.c > +++ b/target/arm/cpu64.c > @@ -736,6 +736,31 @@ static void aarch64_a53_initfn(Object *obj) > define_cortex_a72_a57_a53_cp_reginfo(cpu); > } > > +#ifdef CONFIG_KVM > + > +int get_sysreg_idx(ARMSysRegs sysreg) > +{ > + int i; > + > + for (i = 0; i < NUM_ID_IDX; i++) { > + if (id_register_sysreg[i] == sysreg) { I agree with Richard that if we could get rid of this linear search it would be nicer. > + return i; > + } > + } > + return -1; > +} > + > +uint64_t idregs_sysreg_to_kvm_reg(ARMSysRegs sysreg) > +{ > + return ARM64_SYS_REG((sysreg & CP_REG_ARM64_SYSREG_OP0_MASK) >> CP_REG_ARM64_SYSREG_OP0_SHIFT, > + (sysreg & CP_REG_ARM64_SYSREG_OP1_MASK) >> CP_REG_ARM64_SYSREG_OP1_SHIFT, > + (sysreg & CP_REG_ARM64_SYSREG_CRN_MASK) >> CP_REG_ARM64_SYSREG_CRN_SHIFT, > + (sysreg & CP_REG_ARM64_SYSREG_CRM_MASK) >> CP_REG_ARM64_SYSREG_CRM_SHIFT, > + (sysreg & CP_REG_ARM64_SYSREG_OP2_MASK) >> CP_REG_ARM64_SYSREG_OP2_SHIFT); > +} > + > +#endif > + > static void aarch64_host_initfn(Object *obj) > { > #if defined(CONFIG_KVM) > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > index da30bdbb2349..3b8bb5661f2b 100644 > --- a/target/arm/kvm.c > +++ b/target/arm/kvm.c > @@ -246,6 +246,36 @@ static bool kvm_arm_pauth_supported(void) > kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC)); > } > > +/* read a 32b sysreg value and store it in the idregs */ > +static int get_host_cpu_reg32(int fd, ARMHostCPUFeatures *ahcf, ARMSysRegs sysreg) those are defined as static but there is no user so this will break compilation locally Eric > +{ > + int index = get_sysreg_idx(sysreg); > + uint64_t *reg; > + int ret; > + > + if (index < 0) { > + return -ERANGE; > + } > + reg = &ahcf->isar.idregs[index]; > + ret = read_sys_reg32(fd, (uint32_t *)reg, idregs_sysreg_to_kvm_reg(sysreg)); > + return ret; > +} > + > +/* read a 64b sysreg value and store it in the idregs */ > +static int get_host_cpu_reg64(int fd, ARMHostCPUFeatures *ahcf, ARMSysRegs sysreg) > +{ > + int index = get_sysreg_idx(sysreg); > + uint64_t *reg; > + int ret; > + > + if (index < 0) { > + return -ERANGE; > + } > + reg = &ahcf->isar.idregs[index]; > + ret = read_sys_reg64(fd, reg, idregs_sysreg_to_kvm_reg(sysreg)); > + return ret; > +} > + > static bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > { > /* Identify the feature bits corresponding to the host CPU, and
On Tue, Feb 18 2025, Eric Auger <eric.auger@redhat.com> wrote: > Hi Connie > > On 2/7/25 12:02 PM, Cornelia Huck wrote: >> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >> --- >> target/arm/cpu-sysregs.h | 3 +++ >> target/arm/cpu64.c | 25 +++++++++++++++++++++++++ >> target/arm/kvm.c | 30 ++++++++++++++++++++++++++++++ >> 3 files changed, 58 insertions(+) >> >> diff --git a/target/arm/cpu-sysregs.h b/target/arm/cpu-sysregs.h >> index de09ebae91a5..54a4fadbf0c1 100644 >> --- a/target/arm/cpu-sysregs.h >> +++ b/target/arm/cpu-sysregs.h >> @@ -128,4 +128,7 @@ static const uint32_t id_register_sysreg[NUM_ID_IDX] = { >> [CTR_EL0_IDX] = SYS_CTR_EL0, >> }; >> >> +int get_sysreg_idx(ARMSysRegs sysreg); >> +uint64_t idregs_sysreg_to_kvm_reg(ARMSysRegs sysreg); >> + >> #endif /* ARM_CPU_SYSREGS_H */ >> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c >> index 8188ede5cc8a..9ae78253cb34 100644 >> --- a/target/arm/cpu64.c >> +++ b/target/arm/cpu64.c >> @@ -736,6 +736,31 @@ static void aarch64_a53_initfn(Object *obj) >> define_cortex_a72_a57_a53_cp_reginfo(cpu); >> } >> >> +#ifdef CONFIG_KVM >> + >> +int get_sysreg_idx(ARMSysRegs sysreg) >> +{ >> + int i; >> + >> + for (i = 0; i < NUM_ID_IDX; i++) { >> + if (id_register_sysreg[i] == sysreg) { > I agree with Richard that if we could get rid of this linear search it > would be nicer. FWIW, I have a local branch which reworks the lookup and the macros that is currently in the "it compiles" stage. Hopefully progressing to the "it works" stage (provided I'm not preempted by other things again.) >> + return i; >> + } >> + } >> + return -1; >> +} >> + >> +uint64_t idregs_sysreg_to_kvm_reg(ARMSysRegs sysreg) >> +{ >> + return ARM64_SYS_REG((sysreg & CP_REG_ARM64_SYSREG_OP0_MASK) >> CP_REG_ARM64_SYSREG_OP0_SHIFT, >> + (sysreg & CP_REG_ARM64_SYSREG_OP1_MASK) >> CP_REG_ARM64_SYSREG_OP1_SHIFT, >> + (sysreg & CP_REG_ARM64_SYSREG_CRN_MASK) >> CP_REG_ARM64_SYSREG_CRN_SHIFT, >> + (sysreg & CP_REG_ARM64_SYSREG_CRM_MASK) >> CP_REG_ARM64_SYSREG_CRM_SHIFT, >> + (sysreg & CP_REG_ARM64_SYSREG_OP2_MASK) >> CP_REG_ARM64_SYSREG_OP2_SHIFT); >> +} >> + >> +#endif >> + >> static void aarch64_host_initfn(Object *obj) >> { >> #if defined(CONFIG_KVM) >> diff --git a/target/arm/kvm.c b/target/arm/kvm.c >> index da30bdbb2349..3b8bb5661f2b 100644 >> --- a/target/arm/kvm.c >> +++ b/target/arm/kvm.c >> @@ -246,6 +246,36 @@ static bool kvm_arm_pauth_supported(void) >> kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC)); >> } >> >> +/* read a 32b sysreg value and store it in the idregs */ >> +static int get_host_cpu_reg32(int fd, ARMHostCPUFeatures *ahcf, ARMSysRegs sysreg) > those are defined as static but there is no user so this will break > compilation locally Hm, didn't see that, but noticed a few other places that are b0rken. I think I need to fiddle with my config to broaden compilation coverage.
diff --git a/target/arm/cpu-sysregs.h b/target/arm/cpu-sysregs.h index de09ebae91a5..54a4fadbf0c1 100644 --- a/target/arm/cpu-sysregs.h +++ b/target/arm/cpu-sysregs.h @@ -128,4 +128,7 @@ static const uint32_t id_register_sysreg[NUM_ID_IDX] = { [CTR_EL0_IDX] = SYS_CTR_EL0, }; +int get_sysreg_idx(ARMSysRegs sysreg); +uint64_t idregs_sysreg_to_kvm_reg(ARMSysRegs sysreg); + #endif /* ARM_CPU_SYSREGS_H */ diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index 8188ede5cc8a..9ae78253cb34 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -736,6 +736,31 @@ static void aarch64_a53_initfn(Object *obj) define_cortex_a72_a57_a53_cp_reginfo(cpu); } +#ifdef CONFIG_KVM + +int get_sysreg_idx(ARMSysRegs sysreg) +{ + int i; + + for (i = 0; i < NUM_ID_IDX; i++) { + if (id_register_sysreg[i] == sysreg) { + return i; + } + } + return -1; +} + +uint64_t idregs_sysreg_to_kvm_reg(ARMSysRegs sysreg) +{ + return ARM64_SYS_REG((sysreg & CP_REG_ARM64_SYSREG_OP0_MASK) >> CP_REG_ARM64_SYSREG_OP0_SHIFT, + (sysreg & CP_REG_ARM64_SYSREG_OP1_MASK) >> CP_REG_ARM64_SYSREG_OP1_SHIFT, + (sysreg & CP_REG_ARM64_SYSREG_CRN_MASK) >> CP_REG_ARM64_SYSREG_CRN_SHIFT, + (sysreg & CP_REG_ARM64_SYSREG_CRM_MASK) >> CP_REG_ARM64_SYSREG_CRM_SHIFT, + (sysreg & CP_REG_ARM64_SYSREG_OP2_MASK) >> CP_REG_ARM64_SYSREG_OP2_SHIFT); +} + +#endif + static void aarch64_host_initfn(Object *obj) { #if defined(CONFIG_KVM) diff --git a/target/arm/kvm.c b/target/arm/kvm.c index da30bdbb2349..3b8bb5661f2b 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -246,6 +246,36 @@ static bool kvm_arm_pauth_supported(void) kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC)); } +/* read a 32b sysreg value and store it in the idregs */ +static int get_host_cpu_reg32(int fd, ARMHostCPUFeatures *ahcf, ARMSysRegs sysreg) +{ + int index = get_sysreg_idx(sysreg); + uint64_t *reg; + int ret; + + if (index < 0) { + return -ERANGE; + } + reg = &ahcf->isar.idregs[index]; + ret = read_sys_reg32(fd, (uint32_t *)reg, idregs_sysreg_to_kvm_reg(sysreg)); + return ret; +} + +/* read a 64b sysreg value and store it in the idregs */ +static int get_host_cpu_reg64(int fd, ARMHostCPUFeatures *ahcf, ARMSysRegs sysreg) +{ + int index = get_sysreg_idx(sysreg); + uint64_t *reg; + int ret; + + if (index < 0) { + return -ERANGE; + } + reg = &ahcf->isar.idregs[index]; + ret = read_sys_reg64(fd, reg, idregs_sysreg_to_kvm_reg(sysreg)); + return ret; +} + static bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) { /* Identify the feature bits corresponding to the host CPU, and
Signed-off-by: Cornelia Huck <cohuck@redhat.com> --- target/arm/cpu-sysregs.h | 3 +++ target/arm/cpu64.c | 25 +++++++++++++++++++++++++ target/arm/kvm.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+)