Re: [RFC v2 2/7] arm64: introduce check_features

From: Peng Liang <liangpeng10@huawei.com>
To: Andrew Jones <drjones@redhat.com>
Cc: zhang.zhanghailiang@huawei.com, kvm@vger.kernel.org,
	maz@kernel.org, will@kernel.org, kvmarm@lists.cs.columbia.edu
Subject: Re: [RFC v2 2/7] arm64: introduce check_features
Date: Fri, 18 Sep 2020 17:25:10 +0800	[thread overview]
Message-ID: <bc37052f-3719-ac71-ed86-0427e7fdecf2@huawei.com> (raw)
In-Reply-To: <20200918073059.izmscvrtbnsbgnlj@kamzik.brq.redhat.com>

On 9/18/2020 3:30 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:00:56PM +0800, Peng Liang wrote:
>> To emulate ID registers, we need to validate the value of the register
>> defined by user space.  For most ID registers, we need to check whether
>> each field defined by user space is no more than that of host (whether
>> host support the corresponding features) and whether the fields are
>> supposed to be exposed to guest.  Introduce check_features to do those
>> jobs.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  arch/arm64/include/asm/cpufeature.h |  2 ++
>>  arch/arm64/kernel/cpufeature.c      | 23 +++++++++++++++++++++++
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index 2ba7c4f11d8a..954adc5ca72f 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -579,6 +579,8 @@ void check_local_cpu_capabilities(void);
>>  
>>  u64 read_sanitised_ftr_reg(u32 id);
>>  
>> +int check_features(u32 sys_reg, u64 val);
>> +
>>  static inline bool cpu_supports_mixed_endian_el0(void)
>>  {
>>  	return id_aa64mmfr0_mixed_endian_el0(read_cpuid(ID_AA64MMFR0_EL1));
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 698b32705544..e58926992a70 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -2850,3 +2850,26 @@ ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr,
>>  
>>  	return sprintf(buf, "Vulnerable\n");
>>  }
>> +
>> +int check_features(u32 sys_reg, u64 val)
>> +{
>> +	struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
>> +	const struct arm64_ftr_bits *ftrp;
>> +	u64 exposed_mask = 0;
>> +
>> +	if (!reg)
>> +		return -ENOENT;
>> +
>> +	for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
>> +		if (arm64_ftr_value(ftrp, reg->sys_val) <
>> +		    arm64_ftr_value(ftrp, val)) {
>> +			return -EINVAL;
> 
> This assumes that 0b1111 is invalid if the host has e.g. 0b0001,
> but, IIRC, there are some ID registers where 0b1111 means the
> feature is disabled.

I think arm64_ftr_value will handle it correctly.  If the value of
the field is 0b1111 and the field is signed, arm64_ftr_value will
return -1.

> 
>> +		}
>> +		exposed_mask |= arm64_ftr_mask(ftrp);
>> +	}
>> +
>> +	if (val & ~exposed_mask)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> -- 
>> 2.26.2
>>
> 
> I don't think we should be trying to do the verification at the ftr_bits
> level, at least not generally. Trying to handle all ID registers the
> same way is bound to fail, for the 0b1111 vs. 0b0000 reason pointed
> out above, and probably other reasons. As I stated before, we should be
> validating each feature of each ID register on a case by case basis,
> and we should be using higher level CPU feature checking APIs to get
> that right.
> 
> Also, what about validating that all VCPUs have consistent features
> exposed? Each VCPU could select a valid feature mask by this check,
> but different ones, which will obviously create a completely broken
> guest.
> 
> Thanks,
> drew
> 
> .
> 
Thank you for pointing this.  I haven't thought about it yet...

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