Re: [PATCH v6 5/6] KVM: arm64: Reuse fields of sys_reg_desc for idreg

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jing,

On Tue, Apr 04, 2023 at 03:53:43AM +0000, Jing Zhang wrote:
> Since reset() and val are not used for idreg in sys_reg_desc, they would
> be used with other purposes for idregs.
> The callback reset() would be used to return KVM sanitised id register
> values. The u64 val would be used as mask for writable fields in idregs.
> Only bits with 1 in val are writable from userspace.
> 
> Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> ---
>  arch/arm64/kvm/id_regs.c  | 44 +++++++++++++++++++----------
>  arch/arm64/kvm/sys_regs.c | 59 +++++++++++++++++++++++++++------------
>  arch/arm64/kvm/sys_regs.h | 10 ++++---
>  3 files changed, 77 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> index 6f65d30693fe..fe37b6786b4c 100644
> --- a/arch/arm64/kvm/id_regs.c
> +++ b/arch/arm64/kvm/id_regs.c
> @@ -55,6 +55,11 @@ static u8 pmuver_to_perfmon(u8 pmuver)
>  	}
>  }
>  
> +static u64 general_read_kvm_sanitised_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
> +{
> +	return read_sanitised_ftr_reg(reg_to_encoding(rd));
> +}
> +
>  u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
>  {
>  	u64 val = IDREG(vcpu->kvm, id);
> @@ -324,6 +329,17 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +/*
> + * Since reset() callback and field val are not used for idregs, they will be
> + * used for specific purposes for idregs.
> + * The reset() would return KVM sanitised register value. The value would be the
> + * same as the host kernel sanitised value if there is no KVM sanitisation.
> + * The val would be used as a mask indicating writable fields for the idreg.
> + * Only bits with 1 are writable from userspace. This mask might not be

Nit: This comment update seems to be in the next patch,
since 'val' for AA64PFR0, AA64DFR0 and DFR0 is zero yet.


> + * necessary in the future whenever all ID registers are enabled as writable
> + * from userspace.
> + */
> +
>  /* sys_reg_desc initialiser for known cpufeature ID registers */
>  #define ID_SANITISED(name) {			\
>  	SYS_DESC(SYS_##name),			\
> @@ -331,6 +347,8 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
>  	.get_user = get_id_reg,			\
>  	.set_user = set_id_reg,			\
>  	.visibility = id_visibility,		\
> +	.reset = general_read_kvm_sanitised_reg,\
> +	.val = 0,				\
>  }
>  
>  /* sys_reg_desc initialiser for known cpufeature ID registers */
> @@ -340,6 +358,8 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
>  	.get_user = get_id_reg,			\
>  	.set_user = set_id_reg,			\
>  	.visibility = aa32_id_visibility,	\
> +	.reset = general_read_kvm_sanitised_reg,\
> +	.val = 0,				\
>  }
>  
>  /*
> @@ -352,7 +372,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
>  	.access = access_id_reg,			\
>  	.get_user = get_id_reg,				\
>  	.set_user = set_id_reg,				\
> -	.visibility = raz_visibility			\
> +	.visibility = raz_visibility,			\
> +	.reset = NULL,					\
> +	.val = 0,					\
>  }
>  
>  /*
> @@ -366,6 +388,8 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
>  	.get_user = get_id_reg,			\
>  	.set_user = set_id_reg,			\
>  	.visibility = raz_visibility,		\
> +	.reset = NULL,				\
> +	.val = 0,				\
>  }
>  
>  const struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
> @@ -476,10 +500,7 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params)
>  	return 1;
>  }
>  
> -/*
> - * Set the guest's ID registers that are defined in id_reg_descs[]
> - * with ID_SANITISED() to the host's sanitized value.
> - */
> +/* Initialize the guest's ID registers with KVM sanitised values. */
>  void kvm_arm_init_id_regs(struct kvm *kvm)
>  {
>  	int i;
> @@ -492,16 +513,11 @@ void kvm_arm_init_id_regs(struct kvm *kvm)
>  			/* Shouldn't happen */
>  			continue;
>  
> -		/*
> -		 * Some hidden ID registers which are not in arm64_ftr_regs[]
> -		 * would cause warnings from read_sanitised_ftr_reg().
> -		 * Skip those ID registers to avoid the warnings.
> -		 */
> -		if (id_reg_descs[i].visibility == raz_visibility)
> -			/* Hidden or reserved ID register */
> -			continue;
> +		val = 0;
> +		/* Read KVM sanitised register value if available */
> +		if (id_reg_descs[i].reset)
> +			val = id_reg_descs[i].reset(NULL, &id_reg_descs[i]);
>  
> -		val = read_sanitised_ftr_reg(id);
>  		IDREG(kvm, id) = val;
>  	}
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 15979c2b87ab..703cf833345a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -540,10 +540,11 @@ static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	return 0;
>  }
>  
> -static void reset_bvr(struct kvm_vcpu *vcpu,
> +static u64 reset_bvr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
>  	vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = rd->val;
> +	return rd->val;
>  }
>  
>  static bool trap_bcr(struct kvm_vcpu *vcpu,
> @@ -576,10 +577,11 @@ static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	return 0;
>  }
>  
> -static void reset_bcr(struct kvm_vcpu *vcpu,
> +static u64 reset_bcr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
>  	vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = rd->val;
> +	return rd->val;
>  }
>  
>  static bool trap_wvr(struct kvm_vcpu *vcpu,
> @@ -613,10 +615,11 @@ static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	return 0;
>  }
>  
> -static void reset_wvr(struct kvm_vcpu *vcpu,
> +static u64 reset_wvr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
>  	vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = rd->val;
> +	return rd->val;
>  }
>  
>  static bool trap_wcr(struct kvm_vcpu *vcpu,
> @@ -649,25 +652,28 @@ static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	return 0;
>  }
>  
> -static void reset_wcr(struct kvm_vcpu *vcpu,
> +static u64 reset_wcr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
>  	vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = rd->val;
> +	return rd->val;
>  }
>  
> -static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +static u64 reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	u64 amair = read_sysreg(amair_el1);
>  	vcpu_write_sys_reg(vcpu, amair, AMAIR_EL1);
> +	return amair;
>  }
>  
> -static void reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +static u64 reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	u64 actlr = read_sysreg(actlr_el1);
>  	vcpu_write_sys_reg(vcpu, actlr, ACTLR_EL1);
> +	return actlr;
>  }
>  
> -static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	u64 mpidr;
>  
> @@ -681,7 +687,10 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  	mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
>  	mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
>  	mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> -	vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
> +	mpidr |= (1ULL << 31);
> +	vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
> +
> +	return mpidr;
>  }
>  
>  static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
> @@ -693,13 +702,13 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
>  	return REG_HIDDEN;
>  }
>  
> -static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
>  
>  	/* No PMU available, any PMU reg may UNDEF... */
>  	if (!kvm_arm_support_pmu_v3())
> -		return;
> +		return 0;
>  
>  	n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
>  	n &= ARMV8_PMU_PMCR_N_MASK;
> @@ -708,33 +717,41 @@ static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  
>  	reset_unknown(vcpu, r);
>  	__vcpu_sys_reg(vcpu, r->reg) &= mask;
> +
> +	return __vcpu_sys_reg(vcpu, r->reg);
>  }
>  
> -static void reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	reset_unknown(vcpu, r);
>  	__vcpu_sys_reg(vcpu, r->reg) &= GENMASK(31, 0);
> +
> +	return __vcpu_sys_reg(vcpu, r->reg);
>  }
>  
> -static void reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +static u64 reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	reset_unknown(vcpu, r);
>  	__vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_EVTYPE_MASK;
> +
> +	return __vcpu_sys_reg(vcpu, r->reg);
>  }
>  
> -static void reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +static u64 reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	reset_unknown(vcpu, r);
>  	__vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_COUNTER_MASK;
> +
> +	return __vcpu_sys_reg(vcpu, r->reg);
>  }
>  
> -static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	u64 pmcr;
>  
>  	/* No PMU available, PMCR_EL0 may UNDEF... */
>  	if (!kvm_arm_support_pmu_v3())
> -		return;
> +		return 0;
>  
>  	/* Only preserve PMCR_EL0.N, and reset the rest to 0 */
>  	pmcr = read_sysreg(pmcr_el0) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
> @@ -742,6 +759,8 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  		pmcr |= ARMV8_PMU_PMCR_LC;
>  
>  	__vcpu_sys_reg(vcpu, r->reg) = pmcr;
> +
> +	return __vcpu_sys_reg(vcpu, r->reg);
>  }
>  
>  static bool check_pmu_access_disabled(struct kvm_vcpu *vcpu, u64 flags)
> @@ -1221,7 +1240,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>   * Fabricate a CLIDR_EL1 value instead of using the real value, which can vary
>   * by the physical CPU which the vcpu currently resides in.
>   */
> -static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
>  	u64 clidr;
> @@ -1269,6 +1288,8 @@ static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  		clidr |= 2 << CLIDR_TTYPE_SHIFT(loc);
>  
>  	__vcpu_sys_reg(vcpu, r->reg) = clidr;
> +
> +	return __vcpu_sys_reg(vcpu, r->reg);
>  }
>  
>  static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> @@ -2622,19 +2643,21 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
>   */
>  
>  #define FUNCTION_INVARIANT(reg)						\
> -	static void get_##reg(struct kvm_vcpu *v,			\
> +	static u64 get_##reg(struct kvm_vcpu *v,			\
>  			      const struct sys_reg_desc *r)		\
>  	{								\
>  		((struct sys_reg_desc *)r)->val = read_sysreg(reg);	\
> +		return ((struct sys_reg_desc *)r)->val;			\
>  	}
>  
>  FUNCTION_INVARIANT(midr_el1)
>  FUNCTION_INVARIANT(revidr_el1)
>  FUNCTION_INVARIANT(aidr_el1)
>  
> -static void get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
> +static u64 get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
>  {
>  	((struct sys_reg_desc *)r)->val = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +	return ((struct sys_reg_desc *)r)->val;
>  }
>  
>  /* ->val is filled in by kvm_sys_reg_table_init() */
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index e88fd77309b2..21869319f6e1 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -65,12 +65,12 @@ struct sys_reg_desc {
>  		       const struct sys_reg_desc *);
>  
>  	/* Initialization for vcpu. */
> -	void (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
> +	u64 (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);

Could you add a comment what is return from reset() ?

Thank you,
Reiji

>  
>  	/* Index into sys_reg[], or 0 if we don't need to save it. */
>  	int reg;
>  
> -	/* Value (usually reset value) */
> +	/* Value (usually reset value), or write mask for idregs */
>  	u64 val;
>  
>  	/* Custom get/set_user functions, fallback to generic if NULL */
> @@ -123,19 +123,21 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
>  }
>  
>  /* Reset functions */
> -static inline void reset_unknown(struct kvm_vcpu *vcpu,
> +static inline u64 reset_unknown(struct kvm_vcpu *vcpu,
>  				 const struct sys_reg_desc *r)
>  {
>  	BUG_ON(!r->reg);
>  	BUG_ON(r->reg >= NR_SYS_REGS);
>  	__vcpu_sys_reg(vcpu, r->reg) = 0x1de7ec7edbadc0deULL;
> +	return __vcpu_sys_reg(vcpu, r->reg);
>  }
>  
> -static inline void reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> +static inline u64 reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	BUG_ON(!r->reg);
>  	BUG_ON(r->reg >= NR_SYS_REGS);
>  	__vcpu_sys_reg(vcpu, r->reg) = r->val;
> +	return __vcpu_sys_reg(vcpu, r->reg);
>  }
>  
>  static inline unsigned int sysreg_visibility(const struct kvm_vcpu *vcpu,
> -- 
> 2.40.0.348.gf938b09366-goog
> 



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux