Re: [PATCH v6 02/25] KVM: arm64: Save ID registers' sanitized value per guest

[Oliver Upton <oupton@google.com>]

Hi Reiji,

On Thu, Mar 10, 2022 at 08:47:48PM -0800, Reiji Watanabe wrote:
> Introduce id_regs[] in kvm_arch as a storage of guest's ID registers,
> and save ID registers' sanitized value in the array at KVM_CREATE_VM.
> Use the saved ones when ID registers are read by the guest or
> userspace (via KVM_GET_ONE_REG).
> 
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 12 ++++++
>  arch/arm64/kvm/arm.c              |  1 +
>  arch/arm64/kvm/sys_regs.c         | 65 ++++++++++++++++++++++++-------
>  3 files changed, 63 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2869259e10c0..c041e5afe3d2 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -101,6 +101,13 @@ struct kvm_s2_mmu {
>  struct kvm_arch_memory_slot {
>  };
>  
> +/*
> + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
> + * where 0<=crm<8, 0<=op2<8.

Doesn't the Feature ID register scheme only apply to CRm={1-7},
op2={0-7}? I believe CRm=0, op2={1-4,7} are in fact UNDEFINED, not RAZ
like the other ranges. Furthermore, the registers that are defined in
that range do not go through the read_id_reg() plumbing.

> + */
> +#define KVM_ARM_ID_REG_MAX_NUM	64
> +#define IDREG_IDX(id)		((sys_reg_CRm(id) << 3) | sys_reg_Op2(id))
> +
>  struct kvm_arch {
>  	struct kvm_s2_mmu mmu;
>  
> @@ -137,6 +144,9 @@ struct kvm_arch {
>  	/* Memory Tagging Extension enabled for the guest */
>  	bool mte_enabled;
>  	bool ran_once;
> +
> +	/* ID registers for the guest. */
> +	u64 id_regs[KVM_ARM_ID_REG_MAX_NUM];

This is a decently large array. Should we embed it in kvm_arch or
allocate at init?

[...]

> +
> +/*
> + * Set the guest's ID registers that are defined in sys_reg_descs[]
> + * with ID_SANITISED() to the host's sanitized value.
> + */
> +void set_default_id_regs(struct kvm *kvm)

nit, more relevant if you take the above suggestion: maybe call it
kvm_init_id_regs()?

> +{
> +	int i;
> +	u32 id;
> +	const struct sys_reg_desc *rd;
> +	u64 val;
> +
> +	for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {

You could avoid walking the entire system register table, since we
already know the start and end values for the Feature ID register range.

maybe:

  #define FEATURE_ID_RANGE_START	SYS_ID_PFR0_EL1
  #define FEATURE_ID_RANGE_END		sys_reg(3, 0, 0, 7, 7)

  u32 sys_reg;

  for (sys_reg = FEATURE_ID_RANGE_START; sys_reg <= FEATURE_ID_RANGE_END; sys_reg++)

But, it depends on if this check is necessary:

> +		rd = &sys_reg_descs[i];
> +		if (rd->access != access_id_reg)
> +			/* Not ID register, or hidden/reserved ID register */
> +			continue;

Which itself is dependent on whether KVM is going to sparsely or
verbosely define its feature filtering tables per the other thread. So
really only bother with this if that is the direction you're going.

--
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel