Re: [PATCH v3 05/15] KVM: arm64: Refactor sys_regs.h, c for nVHE reuse
On Mon, Jul 19, 2021 at 05:03:36PM +0100, Fuad Tabba wrote: > Refactor sys_regs.h and sys_regs.c to make it easier to reuse > common code. It will be used in nVHE in a later patch. > > Note that the refactored code uses __inline_bsearch for find_reg > instead of bsearch to avoid copying the bsearch code for nVHE. > > No functional change intended. > > Signed-off-by: Fuad Tabba > --- > arch/arm64/include/asm/sysreg.h | 3 +++ > arch/arm64/kvm/sys_regs.c | 30 +- > arch/arm64/kvm/sys_regs.h | 31 +++ > 3 files changed, 35 insertions(+), 29 deletions(-) With the naming change suggested by Drew: Acked-by: Will Deacon https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 05/15] KVM: arm64: Refactor sys_regs.h, c for nVHE reuse
Hi,
On Tue, Jul 20, 2021 at 2:38 PM Andrew Jones wrote:
>
> On Mon, Jul 19, 2021 at 05:03:36PM +0100, Fuad Tabba wrote:
> > Refactor sys_regs.h and sys_regs.c to make it easier to reuse
> > common code. It will be used in nVHE in a later patch.
> >
> > Note that the refactored code uses __inline_bsearch for find_reg
> > instead of bsearch to avoid copying the bsearch code for nVHE.
> >
> > No functional change intended.
> >
> > Signed-off-by: Fuad Tabba
> > ---
> > arch/arm64/include/asm/sysreg.h | 3 +++
> > arch/arm64/kvm/sys_regs.c | 30 +-
> > arch/arm64/kvm/sys_regs.h | 31 +++
> > 3 files changed, 35 insertions(+), 29 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/sysreg.h
> > b/arch/arm64/include/asm/sysreg.h
> > index 7b9c3acba684..326f49e7bd42 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -1153,6 +1153,9 @@
> > #define ICH_VTR_A3V_SHIFT21
> > #define ICH_VTR_A3V_MASK (1 << ICH_VTR_A3V_SHIFT)
> >
> > +/* Extract the feature specified from the feature id register. */
> > +#define FEATURE(x) (GENMASK_ULL(x##_SHIFT + 3, x##_SHIFT))
>
> I think the comment would be better as
>
> Create a mask for the feature bits of the specified feature.
I agree. I'll use this instead.
> And, I think a more specific name than FEATURE would be better. Maybe
> FEATURE_MASK or even ARM64_FEATURE_MASK ?
I think so too. ARM64_FEATURE_MASK is more descriptive than just FEATURE.
Thanks,
/fuad
> > +
> > #ifdef __ASSEMBLY__
> >
> > .irp
> > num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 80a6e41cadad..1a939c464858 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -44,10 +44,6 @@
> > * 64bit interface.
> > */
> >
> > -#define reg_to_encoding(x) \
> > - sys_reg((u32)(x)->Op0, (u32)(x)->Op1, \
> > - (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2)
> > -
> > static bool read_from_write_only(struct kvm_vcpu *vcpu,
> >struct sys_reg_params *params,
> >const struct sys_reg_desc *r)
> > @@ -1026,8 +1022,6 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
> > return true;
> > }
> >
> > -#define FEATURE(x) (GENMASK_ULL(x##_SHIFT + 3, x##_SHIFT))
> > -
> > /* Read a sanitised cpufeature ID register by sys_reg_desc */
> > static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > struct sys_reg_desc const *r, bool raz)
> > @@ -2106,23 +2100,6 @@ static int check_sysreg_table(const struct
> > sys_reg_desc *table, unsigned int n,
> > return 0;
> > }
> >
> > -static int match_sys_reg(const void *key, const void *elt)
> > -{
> > - const unsigned long pval = (unsigned long)key;
> > - const struct sys_reg_desc *r = elt;
> > -
> > - return pval - reg_to_encoding(r);
> > -}
> > -
> > -static const struct sys_reg_desc *find_reg(const struct sys_reg_params
> > *params,
> > - const struct sys_reg_desc table[],
> > - unsigned int num)
> > -{
> > - unsigned long pval = reg_to_encoding(params);
> > -
> > - return bsearch((void *)pval, table, num, sizeof(table[0]),
> > match_sys_reg);
> > -}
> > -
> > int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu)
> > {
> > kvm_inject_undefined(vcpu);
> > @@ -2365,13 +2342,8 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
> >
> > trace_kvm_handle_sys_reg(esr);
> >
> > - params.Op0 = (esr >> 20) & 3;
> > - params.Op1 = (esr >> 14) & 0x7;
> > - params.CRn = (esr >> 10) & 0xf;
> > - params.CRm = (esr >> 1) & 0xf;
> > - params.Op2 = (esr >> 17) & 0x7;
> > + params = esr_sys64_to_params(esr);
> > params.regval = vcpu_get_reg(vcpu, Rt);
> > - params.is_write = !(esr & 1);
> >
> > ret = emulate_sys_reg(vcpu, ¶ms);
> >
> > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> > index 9d0621417c2a..cc0cc95a0280 100644
> > --- a/arch/arm64/kvm/sys_regs.h
> > +++ b/arch/arm64/kvm/sys_regs.h
> > @@ -11,6 +11,12 @@
> > #ifndef __ARM64_KVM_SYS_REGS_LOCAL_H__
> > #define __ARM64_KVM_SYS_REGS_LOCAL_H__
> >
> > +#include
> > +
> > +#define reg_to_encoding(x) \
> > + sys_reg((u32)(x)->Op0, (u32)(x)->Op1, \
> > + (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2)
> > +
> > struct sys_reg_params {
> > u8 Op0;
> > u8 Op1;
> > @@ -21,6 +27,14 @@ struct sys_reg_params {
> > boolis_write;
> > };
> >
> > +#define esr_sys64_to_params(esr)
> > \
> > + ((struct sys_reg_params){ .Op0 = ((esr) >> 20) & 3,
Re: [PATCH v3 05/15] KVM: arm64: Refactor sys_regs.h, c for nVHE reuse
On Mon, Jul 19, 2021 at 05:03:36PM +0100, Fuad Tabba wrote:
> Refactor sys_regs.h and sys_regs.c to make it easier to reuse
> common code. It will be used in nVHE in a later patch.
>
> Note that the refactored code uses __inline_bsearch for find_reg
> instead of bsearch to avoid copying the bsearch code for nVHE.
>
> No functional change intended.
>
> Signed-off-by: Fuad Tabba
> ---
> arch/arm64/include/asm/sysreg.h | 3 +++
> arch/arm64/kvm/sys_regs.c | 30 +-
> arch/arm64/kvm/sys_regs.h | 31 +++
> 3 files changed, 35 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 7b9c3acba684..326f49e7bd42 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -1153,6 +1153,9 @@
> #define ICH_VTR_A3V_SHIFT21
> #define ICH_VTR_A3V_MASK (1 << ICH_VTR_A3V_SHIFT)
>
> +/* Extract the feature specified from the feature id register. */
> +#define FEATURE(x) (GENMASK_ULL(x##_SHIFT + 3, x##_SHIFT))
I think the comment would be better as
Create a mask for the feature bits of the specified feature.
And, I think a more specific name than FEATURE would be better. Maybe
FEATURE_MASK or even ARM64_FEATURE_MASK ?
> +
> #ifdef __ASSEMBLY__
>
> .irp
> num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 80a6e41cadad..1a939c464858 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -44,10 +44,6 @@
> * 64bit interface.
> */
>
> -#define reg_to_encoding(x) \
> - sys_reg((u32)(x)->Op0, (u32)(x)->Op1, \
> - (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2)
> -
> static bool read_from_write_only(struct kvm_vcpu *vcpu,
>struct sys_reg_params *params,
>const struct sys_reg_desc *r)
> @@ -1026,8 +1022,6 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
> return true;
> }
>
> -#define FEATURE(x) (GENMASK_ULL(x##_SHIFT + 3, x##_SHIFT))
> -
> /* Read a sanitised cpufeature ID register by sys_reg_desc */
> static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> struct sys_reg_desc const *r, bool raz)
> @@ -2106,23 +2100,6 @@ static int check_sysreg_table(const struct
> sys_reg_desc *table, unsigned int n,
> return 0;
> }
>
> -static int match_sys_reg(const void *key, const void *elt)
> -{
> - const unsigned long pval = (unsigned long)key;
> - const struct sys_reg_desc *r = elt;
> -
> - return pval - reg_to_encoding(r);
> -}
> -
> -static const struct sys_reg_desc *find_reg(const struct sys_reg_params
> *params,
> - const struct sys_reg_desc table[],
> - unsigned int num)
> -{
> - unsigned long pval = reg_to_encoding(params);
> -
> - return bsearch((void *)pval, table, num, sizeof(table[0]),
> match_sys_reg);
> -}
> -
> int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu)
> {
> kvm_inject_undefined(vcpu);
> @@ -2365,13 +2342,8 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
>
> trace_kvm_handle_sys_reg(esr);
>
> - params.Op0 = (esr >> 20) & 3;
> - params.Op1 = (esr >> 14) & 0x7;
> - params.CRn = (esr >> 10) & 0xf;
> - params.CRm = (esr >> 1) & 0xf;
> - params.Op2 = (esr >> 17) & 0x7;
> + params = esr_sys64_to_params(esr);
> params.regval = vcpu_get_reg(vcpu, Rt);
> - params.is_write = !(esr & 1);
>
> ret = emulate_sys_reg(vcpu, ¶ms);
>
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index 9d0621417c2a..cc0cc95a0280 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -11,6 +11,12 @@
> #ifndef __ARM64_KVM_SYS_REGS_LOCAL_H__
> #define __ARM64_KVM_SYS_REGS_LOCAL_H__
>
> +#include
> +
> +#define reg_to_encoding(x) \
> + sys_reg((u32)(x)->Op0, (u32)(x)->Op1, \
> + (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2)
> +
> struct sys_reg_params {
> u8 Op0;
> u8 Op1;
> @@ -21,6 +27,14 @@ struct sys_reg_params {
> boolis_write;
> };
>
> +#define esr_sys64_to_params(esr)
> \
> + ((struct sys_reg_params){ .Op0 = ((esr) >> 20) & 3,\
> + .Op1 = ((esr) >> 14) & 0x7, \
> + .CRn = ((esr) >> 10) & 0xf, \
> + .CRm = ((esr) >> 1) & 0xf, \
> + .Op2 = ((esr) >> 17) & 0x7, \
> + .is_write = !((esr) & 1) })
> +
> struct sys_reg_
[PATCH v3 05/15] KVM: arm64: Refactor sys_regs.h,c for nVHE reuse
Refactor sys_regs.h and sys_regs.c to make it easier to reuse
common code. It will be used in nVHE in a later patch.
Note that the refactored code uses __inline_bsearch for find_reg
instead of bsearch to avoid copying the bsearch code for nVHE.
No functional change intended.
Signed-off-by: Fuad Tabba
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/sys_regs.c | 30 +-
arch/arm64/kvm/sys_regs.h | 31 +++
3 files changed, 35 insertions(+), 29 deletions(-)
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 7b9c3acba684..326f49e7bd42 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -1153,6 +1153,9 @@
#define ICH_VTR_A3V_SHIFT 21
#define ICH_VTR_A3V_MASK (1 << ICH_VTR_A3V_SHIFT)
+/* Extract the feature specified from the feature id register. */
+#define FEATURE(x) (GENMASK_ULL(x##_SHIFT + 3, x##_SHIFT))
+
#ifdef __ASSEMBLY__
.irp
num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 80a6e41cadad..1a939c464858 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -44,10 +44,6 @@
* 64bit interface.
*/
-#define reg_to_encoding(x) \
- sys_reg((u32)(x)->Op0, (u32)(x)->Op1, \
- (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2)
-
static bool read_from_write_only(struct kvm_vcpu *vcpu,
struct sys_reg_params *params,
const struct sys_reg_desc *r)
@@ -1026,8 +1022,6 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
return true;
}
-#define FEATURE(x) (GENMASK_ULL(x##_SHIFT + 3, x##_SHIFT))
-
/* Read a sanitised cpufeature ID register by sys_reg_desc */
static u64 read_id_reg(const struct kvm_vcpu *vcpu,
struct sys_reg_desc const *r, bool raz)
@@ -2106,23 +2100,6 @@ static int check_sysreg_table(const struct sys_reg_desc
*table, unsigned int n,
return 0;
}
-static int match_sys_reg(const void *key, const void *elt)
-{
- const unsigned long pval = (unsigned long)key;
- const struct sys_reg_desc *r = elt;
-
- return pval - reg_to_encoding(r);
-}
-
-static const struct sys_reg_desc *find_reg(const struct sys_reg_params *params,
-const struct sys_reg_desc table[],
-unsigned int num)
-{
- unsigned long pval = reg_to_encoding(params);
-
- return bsearch((void *)pval, table, num, sizeof(table[0]),
match_sys_reg);
-}
-
int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu)
{
kvm_inject_undefined(vcpu);
@@ -2365,13 +2342,8 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
trace_kvm_handle_sys_reg(esr);
- params.Op0 = (esr >> 20) & 3;
- params.Op1 = (esr >> 14) & 0x7;
- params.CRn = (esr >> 10) & 0xf;
- params.CRm = (esr >> 1) & 0xf;
- params.Op2 = (esr >> 17) & 0x7;
+ params = esr_sys64_to_params(esr);
params.regval = vcpu_get_reg(vcpu, Rt);
- params.is_write = !(esr & 1);
ret = emulate_sys_reg(vcpu, ¶ms);
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index 9d0621417c2a..cc0cc95a0280 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -11,6 +11,12 @@
#ifndef __ARM64_KVM_SYS_REGS_LOCAL_H__
#define __ARM64_KVM_SYS_REGS_LOCAL_H__
+#include
+
+#define reg_to_encoding(x) \
+ sys_reg((u32)(x)->Op0, (u32)(x)->Op1, \
+ (u32)(x)->CRn, (u32)(x)->CRm, (u32)(x)->Op2)
+
struct sys_reg_params {
u8 Op0;
u8 Op1;
@@ -21,6 +27,14 @@ struct sys_reg_params {
boolis_write;
};
+#define esr_sys64_to_params(esr)
\
+ ((struct sys_reg_params){ .Op0 = ((esr) >> 20) & 3,\
+ .Op1 = ((esr) >> 14) & 0x7, \
+ .CRn = ((esr) >> 10) & 0xf, \
+ .CRm = ((esr) >> 1) & 0xf, \
+ .Op2 = ((esr) >> 17) & 0x7, \
+ .is_write = !((esr) & 1) })
+
struct sys_reg_desc {
/* Sysreg string for debug */
const char *name;
@@ -152,6 +166,23 @@ static inline int cmp_sys_reg(const struct sys_reg_desc
*i1,
return i1->Op2 - i2->Op2;
}
+static inline int match_sys_reg(const void *key, const void *elt)
+{
+ const unsigned long pval = (unsigned long)key;
+ const struct sys_reg_desc *r = elt;
+
+ return pval - reg_to_encoding(r);
+}
+
+static inline const struct sys_reg
