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

[Reiji Watanabe <reijiw@google.com>]

Hi Oliver,

On 3/23/22 12:22 PM, Oliver Upton wrote:
> 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.


Will fix this.


> 
>> + */
>> +#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?


What is the reason why you think you might want to allocate it 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.

Even just going through for ID register ranges, we should do the check
to skip hidden/reserved ID registers (not to call read_sanitised_ftr_reg).

Yes, it's certainly possible to avoid walking the entire system register,
and I will fix it.  The reason why I didn't care it so much was just
because the code (walking the entire system register) will be removed by
the following patches:)

Thanks,
Reiji
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm