[RFC v2 0/7] kvm: arm64: emulate ID registers

[RFC v2 0/7] kvm: arm64: emulate ID registers

[Peng Liang]

In AArch64, guest will read the same values of the ID regsiters with
host.  Both of them read the values from arm64_ftr_regs.  This patch
series add support to emulate and configure ID registers so that we can
control the value of ID registers that guest read.

v1 -> v2:
 - save the ID registers in sysreg file instead of a new struct
 - apply a checker before setting the value to the register
 - add doc for new KVM_CAP_ARM_CPU_FEATURE

Peng Liang (7):
  arm64: add a helper function to traverse arm64_ftr_regs
  arm64: introduce check_features
  kvm: arm64: save ID registers to sys_regs file
  kvm: arm64: introduce check_user
  kvm: arm64: implement check_user for ID registers
  kvm: arm64: make ID registers configurable
  kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

 Documentation/virt/kvm/api.rst      |   8 +
 arch/arm64/include/asm/cpufeature.h |   4 +
 arch/arm64/include/asm/kvm_coproc.h |   2 +
 arch/arm64/include/asm/kvm_host.h   |   3 +
 arch/arm64/kernel/cpufeature.c      |  36 +++
 arch/arm64/kvm/arm.c                |   3 +
 arch/arm64/kvm/sys_regs.c           | 481 +++++++++++++++++++++++++++-
 arch/arm64/kvm/sys_regs.h           |   6 +
 include/uapi/linux/kvm.h            |   1 +
 9 files changed, 532 insertions(+), 12 deletions(-)

-- 
2.26.2

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

[RFC v2 0/7] kvm: arm64: emulate ID registers

[Peng Liang]

In AArch64, guest will read the same values of the ID regsiters with
host.  Both of them read the values from arm64_ftr_regs.  This patch
series add support to emulate and configure ID registers so that we can
control the value of ID registers that guest read.

v1 -> v2:
 - save the ID registers in sysreg file instead of a new struct
 - apply a checker before setting the value to the register
 - add doc for new KVM_CAP_ARM_CPU_FEATURE

Peng Liang (7):
  arm64: add a helper function to traverse arm64_ftr_regs
  arm64: introduce check_features
  kvm: arm64: save ID registers to sys_regs file
  kvm: arm64: introduce check_user
  kvm: arm64: implement check_user for ID registers
  kvm: arm64: make ID registers configurable
  kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

 Documentation/virt/kvm/api.rst      |   8 +
 arch/arm64/include/asm/cpufeature.h |   4 +
 arch/arm64/include/asm/kvm_coproc.h |   2 +
 arch/arm64/include/asm/kvm_host.h   |   3 +
 arch/arm64/kernel/cpufeature.c      |  36 +++
 arch/arm64/kvm/arm.c                |   3 +
 arch/arm64/kvm/sys_regs.c           | 481 +++++++++++++++++++++++++++-
 arch/arm64/kvm/sys_regs.h           |   6 +
 include/uapi/linux/kvm.h            |   1 +
 9 files changed, 532 insertions(+), 12 deletions(-)

-- 
2.26.2

[RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs

[Peng Liang]

If we want to emulate ID registers, we need to initialize ID registers
firstly.  This commit is to add a helper function to traverse
arm64_ftr_regs so that we can initialize ID registers from
arm64_ftr_regs.

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      | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 89b4f0142c28..2ba7c4f11d8a 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -79,6 +79,8 @@ struct arm64_ftr_reg {
 
 extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
 
+int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
+
 /*
  * CPU capabilities:
  *
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 6424584be01e..698b32705544 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
 	return regp->sys_val;
 }
 
+int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
+{
+	int i, ret;
+
+	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
+		ret = (*op)(arm64_ftr_regs[i].sys_id,
+			    arm64_ftr_regs[i].reg->sys_val, argp);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
 #define read_sysreg_case(r)	\
 	case r:		return read_sysreg_s(r)
 
-- 
2.26.2

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

[RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs

[Peng Liang]

If we want to emulate ID registers, we need to initialize ID registers
firstly.  This commit is to add a helper function to traverse
arm64_ftr_regs so that we can initialize ID registers from
arm64_ftr_regs.

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      | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 89b4f0142c28..2ba7c4f11d8a 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -79,6 +79,8 @@ struct arm64_ftr_reg {
 
 extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
 
+int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
+
 /*
  * CPU capabilities:
  *
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 6424584be01e..698b32705544 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
 	return regp->sys_val;
 }
 
+int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
+{
+	int i, ret;
+
+	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
+		ret = (*op)(arm64_ftr_regs[i].sys_id,
+			    arm64_ftr_regs[i].reg->sys_val, argp);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
 #define read_sysreg_case(r)	\
 	case r:		return read_sysreg_s(r)
 
-- 
2.26.2

Re: [RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:00:55PM +0800, Peng Liang wrote:
> If we want to emulate ID registers, we need to initialize ID registers
> firstly.  This commit is to add a helper function to traverse
> arm64_ftr_regs so that we can initialize ID registers from
> arm64_ftr_regs.
> 
> 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      | 13 +++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 89b4f0142c28..2ba7c4f11d8a 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -79,6 +79,8 @@ struct arm64_ftr_reg {
>  
>  extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>  
> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
> +
>  /*
>   * CPU capabilities:
>   *
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 6424584be01e..698b32705544 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
>  	return regp->sys_val;
>  }
>  
> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
> +		ret = (*op)(arm64_ftr_regs[i].sys_id,
> +			    arm64_ftr_regs[i].reg->sys_val, argp);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
>  #define read_sysreg_case(r)	\
>  	case r:		return read_sysreg_s(r)
>  
> -- 
> 2.26.2
>

Skimming the rest of the patches to see how this is used I only saw a
single callsite. Why wouldn't we just put this simple for-loop right
there at that callsite? Or, IOW, I think this traverse function should
be dropped.

Thanks,
drew

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

Re: [RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:00:55PM +0800, Peng Liang wrote:
> If we want to emulate ID registers, we need to initialize ID registers
> firstly.  This commit is to add a helper function to traverse
> arm64_ftr_regs so that we can initialize ID registers from
> arm64_ftr_regs.
> 
> 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      | 13 +++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 89b4f0142c28..2ba7c4f11d8a 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -79,6 +79,8 @@ struct arm64_ftr_reg {
>  
>  extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>  
> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
> +
>  /*
>   * CPU capabilities:
>   *
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 6424584be01e..698b32705544 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
>  	return regp->sys_val;
>  }
>  
> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
> +		ret = (*op)(arm64_ftr_regs[i].sys_id,
> +			    arm64_ftr_regs[i].reg->sys_val, argp);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
>  #define read_sysreg_case(r)	\
>  	case r:		return read_sysreg_s(r)
>  
> -- 
> 2.26.2
>

Skimming the rest of the patches to see how this is used I only saw a
single callsite. Why wouldn't we just put this simple for-loop right
there at that callsite? Or, IOW, I think this traverse function should
be dropped.

Thanks,
drew

Re: [RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs

[Peng Liang]

On 9/18/2020 3:18 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:00:55PM +0800, Peng Liang wrote:
>> If we want to emulate ID registers, we need to initialize ID registers
>> firstly.  This commit is to add a helper function to traverse
>> arm64_ftr_regs so that we can initialize ID registers from
>> arm64_ftr_regs.
>>
>> 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      | 13 +++++++++++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index 89b4f0142c28..2ba7c4f11d8a 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -79,6 +79,8 @@ struct arm64_ftr_reg {
>>  
>>  extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>>  
>> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
>> +
>>  /*
>>   * CPU capabilities:
>>   *
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 6424584be01e..698b32705544 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
>>  	return regp->sys_val;
>>  }
>>  
>> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
>> +{
>> +	int i, ret;
>> +
>> +	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
>> +		ret = (*op)(arm64_ftr_regs[i].sys_id,
>> +			    arm64_ftr_regs[i].reg->sys_val, argp);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>>  #define read_sysreg_case(r)	\
>>  	case r:		return read_sysreg_s(r)
>>  
>> -- 
>> 2.26.2
>>
> 
> Skimming the rest of the patches to see how this is used I only saw a
> single callsite. Why wouldn't we just put this simple for-loop right
> there at that callsite? Or, IOW, I think this traverse function should
> be dropped.
> 
> Thanks,
> drew
> 
> .
> 

arm64_ftr_regs is defined as a static array in arch/arm64/kernel/cpufeature.c,
which is not a virtualization-related file.  Putting this simple for-loop
right there will make cpufeature.c depend on kvm_host.h.  Is this a good idea?

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

Re: [RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs

[Peng Liang]

On 9/18/2020 3:18 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:00:55PM +0800, Peng Liang wrote:
>> If we want to emulate ID registers, we need to initialize ID registers
>> firstly.  This commit is to add a helper function to traverse
>> arm64_ftr_regs so that we can initialize ID registers from
>> arm64_ftr_regs.
>>
>> 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      | 13 +++++++++++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index 89b4f0142c28..2ba7c4f11d8a 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -79,6 +79,8 @@ struct arm64_ftr_reg {
>>  
>>  extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>>  
>> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
>> +
>>  /*
>>   * CPU capabilities:
>>   *
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 6424584be01e..698b32705544 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
>>  	return regp->sys_val;
>>  }
>>  
>> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
>> +{
>> +	int i, ret;
>> +
>> +	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
>> +		ret = (*op)(arm64_ftr_regs[i].sys_id,
>> +			    arm64_ftr_regs[i].reg->sys_val, argp);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>>  #define read_sysreg_case(r)	\
>>  	case r:		return read_sysreg_s(r)
>>  
>> -- 
>> 2.26.2
>>
> 
> Skimming the rest of the patches to see how this is used I only saw a
> single callsite. Why wouldn't we just put this simple for-loop right
> there at that callsite? Or, IOW, I think this traverse function should
> be dropped.
> 
> Thanks,
> drew
> 
> .
> 

arm64_ftr_regs is defined as a static array in arch/arm64/kernel/cpufeature.c,
which is not a virtualization-related file.  Putting this simple for-loop
right there will make cpufeature.c depend on kvm_host.h.  Is this a good idea?

Thanks,
Peng

Re: [RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs

[Andrew Jones]

On Fri, Sep 18, 2020 at 05:24:27PM +0800, Peng Liang wrote:
> On 9/18/2020 3:18 PM, Andrew Jones wrote:
> > On Thu, Sep 17, 2020 at 08:00:55PM +0800, Peng Liang wrote:
> >> If we want to emulate ID registers, we need to initialize ID registers
> >> firstly.  This commit is to add a helper function to traverse
> >> arm64_ftr_regs so that we can initialize ID registers from
> >> arm64_ftr_regs.
> >>
> >> 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      | 13 +++++++++++++
> >>  2 files changed, 15 insertions(+)
> >>
> >> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> >> index 89b4f0142c28..2ba7c4f11d8a 100644
> >> --- a/arch/arm64/include/asm/cpufeature.h
> >> +++ b/arch/arm64/include/asm/cpufeature.h
> >> @@ -79,6 +79,8 @@ struct arm64_ftr_reg {
> >>  
> >>  extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
> >>  
> >> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
> >> +
> >>  /*
> >>   * CPU capabilities:
> >>   *
> >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >> index 6424584be01e..698b32705544 100644
> >> --- a/arch/arm64/kernel/cpufeature.c
> >> +++ b/arch/arm64/kernel/cpufeature.c
> >> @@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
> >>  	return regp->sys_val;
> >>  }
> >>  
> >> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
> >> +{
> >> +	int i, ret;
> >> +
> >> +	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
> >> +		ret = (*op)(arm64_ftr_regs[i].sys_id,
> >> +			    arm64_ftr_regs[i].reg->sys_val, argp);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +	}
> >> +	return 0;
> >> +}
> >> +
> >>  #define read_sysreg_case(r)	\
> >>  	case r:		return read_sysreg_s(r)
> >>  
> >> -- 
> >> 2.26.2
> >>
> > 
> > Skimming the rest of the patches to see how this is used I only saw a
> > single callsite. Why wouldn't we just put this simple for-loop right
> > there at that callsite? Or, IOW, I think this traverse function should
> > be dropped.
> > 
> > Thanks,
> > drew
> > 
> > .
> > 
> 
> arm64_ftr_regs is defined as a static array in arch/arm64/kernel/cpufeature.c,
> which is not a virtualization-related file.  Putting this simple for-loop
> right there will make cpufeature.c depend on kvm_host.h.  Is this a good idea?

Well, the fact that arm64_ftr_regs is static to cpufeature.c is a clue
that your implementation is likely playing with internal arm64_ftr
state that it shouldn't be. If there's not an accessor function that
works for you, then you can try adding one. Providing general functions
like this, that are effectively just an odd way of removing 'static'
from arm64_ftr_regs, breaks the encapsulation.

Thanks,
drew

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

Re: [RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs

[Andrew Jones]

On Fri, Sep 18, 2020 at 05:24:27PM +0800, Peng Liang wrote:
> On 9/18/2020 3:18 PM, Andrew Jones wrote:
> > On Thu, Sep 17, 2020 at 08:00:55PM +0800, Peng Liang wrote:
> >> If we want to emulate ID registers, we need to initialize ID registers
> >> firstly.  This commit is to add a helper function to traverse
> >> arm64_ftr_regs so that we can initialize ID registers from
> >> arm64_ftr_regs.
> >>
> >> 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      | 13 +++++++++++++
> >>  2 files changed, 15 insertions(+)
> >>
> >> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> >> index 89b4f0142c28..2ba7c4f11d8a 100644
> >> --- a/arch/arm64/include/asm/cpufeature.h
> >> +++ b/arch/arm64/include/asm/cpufeature.h
> >> @@ -79,6 +79,8 @@ struct arm64_ftr_reg {
> >>  
> >>  extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
> >>  
> >> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
> >> +
> >>  /*
> >>   * CPU capabilities:
> >>   *
> >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >> index 6424584be01e..698b32705544 100644
> >> --- a/arch/arm64/kernel/cpufeature.c
> >> +++ b/arch/arm64/kernel/cpufeature.c
> >> @@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
> >>  	return regp->sys_val;
> >>  }
> >>  
> >> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
> >> +{
> >> +	int i, ret;
> >> +
> >> +	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
> >> +		ret = (*op)(arm64_ftr_regs[i].sys_id,
> >> +			    arm64_ftr_regs[i].reg->sys_val, argp);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +	}
> >> +	return 0;
> >> +}
> >> +
> >>  #define read_sysreg_case(r)	\
> >>  	case r:		return read_sysreg_s(r)
> >>  
> >> -- 
> >> 2.26.2
> >>
> > 
> > Skimming the rest of the patches to see how this is used I only saw a
> > single callsite. Why wouldn't we just put this simple for-loop right
> > there at that callsite? Or, IOW, I think this traverse function should
> > be dropped.
> > 
> > Thanks,
> > drew
> > 
> > .
> > 
> 
> arm64_ftr_regs is defined as a static array in arch/arm64/kernel/cpufeature.c,
> which is not a virtualization-related file.  Putting this simple for-loop
> right there will make cpufeature.c depend on kvm_host.h.  Is this a good idea?

Well, the fact that arm64_ftr_regs is static to cpufeature.c is a clue
that your implementation is likely playing with internal arm64_ftr
state that it shouldn't be. If there's not an accessor function that
works for you, then you can try adding one. Providing general functions
like this, that are effectively just an odd way of removing 'static'
from arm64_ftr_regs, breaks the encapsulation.

Thanks,
drew

Re: [RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs

[Peng Liang]

On 9/18/2020 6:28 PM, Andrew Jones wrote:
> On Fri, Sep 18, 2020 at 05:24:27PM +0800, Peng Liang wrote:
>> On 9/18/2020 3:18 PM, Andrew Jones wrote:
>>> On Thu, Sep 17, 2020 at 08:00:55PM +0800, Peng Liang wrote:
>>>> If we want to emulate ID registers, we need to initialize ID registers
>>>> firstly.  This commit is to add a helper function to traverse
>>>> arm64_ftr_regs so that we can initialize ID registers from
>>>> arm64_ftr_regs.
>>>>
>>>> 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      | 13 +++++++++++++
>>>>  2 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>>>> index 89b4f0142c28..2ba7c4f11d8a 100644
>>>> --- a/arch/arm64/include/asm/cpufeature.h
>>>> +++ b/arch/arm64/include/asm/cpufeature.h
>>>> @@ -79,6 +79,8 @@ struct arm64_ftr_reg {
>>>>  
>>>>  extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>>>>  
>>>> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
>>>> +
>>>>  /*
>>>>   * CPU capabilities:
>>>>   *
>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>> index 6424584be01e..698b32705544 100644
>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>> @@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
>>>>  	return regp->sys_val;
>>>>  }
>>>>  
>>>> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
>>>> +{
>>>> +	int i, ret;
>>>> +
>>>> +	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
>>>> +		ret = (*op)(arm64_ftr_regs[i].sys_id,
>>>> +			    arm64_ftr_regs[i].reg->sys_val, argp);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +	}
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  #define read_sysreg_case(r)	\
>>>>  	case r:		return read_sysreg_s(r)
>>>>  
>>>> -- 
>>>> 2.26.2
>>>>
>>>
>>> Skimming the rest of the patches to see how this is used I only saw a
>>> single callsite. Why wouldn't we just put this simple for-loop right
>>> there at that callsite? Or, IOW, I think this traverse function should
>>> be dropped.
>>>
>>> Thanks,
>>> drew
>>>
>>> .
>>>
>>
>> arm64_ftr_regs is defined as a static array in arch/arm64/kernel/cpufeature.c,
>> which is not a virtualization-related file.  Putting this simple for-loop
>> right there will make cpufeature.c depend on kvm_host.h.  Is this a good idea?
> 
> Well, the fact that arm64_ftr_regs is static to cpufeature.c is a clue
> that your implementation is likely playing with internal arm64_ftr
> state that it shouldn't be. If there's not an accessor function that
> works for you, then you can try adding one. Providing general functions
> like this, that are effectively just an odd way of removing 'static'
> from arm64_ftr_regs, breaks the encapsulation.
> 
> Thanks,
> drew
> 
> .
> 

I found get_arm64_ftr_reg_nowarn and get_arm64_ftr_reg in cpufeature.c which will
search and return the arm64_ftr_reg* according to the sys_id.  But they are all
static.  Hence, I think cpufeature.c don't want other modules to access the
arm64_ftr_reg*.  So I add arm64_cpu_ftr_regs_traverse to traverse the
arm64_ftr_regs and pass the id and value to op instead of the arm64_ftr_reg*.

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

Re: [RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs

[Peng Liang]

On 9/18/2020 6:28 PM, Andrew Jones wrote:
> On Fri, Sep 18, 2020 at 05:24:27PM +0800, Peng Liang wrote:
>> On 9/18/2020 3:18 PM, Andrew Jones wrote:
>>> On Thu, Sep 17, 2020 at 08:00:55PM +0800, Peng Liang wrote:
>>>> If we want to emulate ID registers, we need to initialize ID registers
>>>> firstly.  This commit is to add a helper function to traverse
>>>> arm64_ftr_regs so that we can initialize ID registers from
>>>> arm64_ftr_regs.
>>>>
>>>> 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      | 13 +++++++++++++
>>>>  2 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>>>> index 89b4f0142c28..2ba7c4f11d8a 100644
>>>> --- a/arch/arm64/include/asm/cpufeature.h
>>>> +++ b/arch/arm64/include/asm/cpufeature.h
>>>> @@ -79,6 +79,8 @@ struct arm64_ftr_reg {
>>>>  
>>>>  extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>>>>  
>>>> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
>>>> +
>>>>  /*
>>>>   * CPU capabilities:
>>>>   *
>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>> index 6424584be01e..698b32705544 100644
>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>> @@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
>>>>  	return regp->sys_val;
>>>>  }
>>>>  
>>>> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
>>>> +{
>>>> +	int i, ret;
>>>> +
>>>> +	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
>>>> +		ret = (*op)(arm64_ftr_regs[i].sys_id,
>>>> +			    arm64_ftr_regs[i].reg->sys_val, argp);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +	}
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  #define read_sysreg_case(r)	\
>>>>  	case r:		return read_sysreg_s(r)
>>>>  
>>>> -- 
>>>> 2.26.2
>>>>
>>>
>>> Skimming the rest of the patches to see how this is used I only saw a
>>> single callsite. Why wouldn't we just put this simple for-loop right
>>> there at that callsite? Or, IOW, I think this traverse function should
>>> be dropped.
>>>
>>> Thanks,
>>> drew
>>>
>>> .
>>>
>>
>> arm64_ftr_regs is defined as a static array in arch/arm64/kernel/cpufeature.c,
>> which is not a virtualization-related file.  Putting this simple for-loop
>> right there will make cpufeature.c depend on kvm_host.h.  Is this a good idea?
> 
> Well, the fact that arm64_ftr_regs is static to cpufeature.c is a clue
> that your implementation is likely playing with internal arm64_ftr
> state that it shouldn't be. If there's not an accessor function that
> works for you, then you can try adding one. Providing general functions
> like this, that are effectively just an odd way of removing 'static'
> from arm64_ftr_regs, breaks the encapsulation.
> 
> Thanks,
> drew
> 
> .
> 

I found get_arm64_ftr_reg_nowarn and get_arm64_ftr_reg in cpufeature.c which will
search and return the arm64_ftr_reg* according to the sys_id.  But they are all
static.  Hence, I think cpufeature.c don't want other modules to access the
arm64_ftr_reg*.  So I add arm64_cpu_ftr_regs_traverse to traverse the
arm64_ftr_regs and pass the id and value to op instead of the arm64_ftr_reg*.

Thanks,
Peng

[RFC v2 2/7] arm64: introduce check_features

[Peng Liang]

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;
+		}
+		exposed_mask |= arm64_ftr_mask(ftrp);
+	}
+
+	if (val & ~exposed_mask)
+		return -EINVAL;
+
+	return 0;
+}
-- 
2.26.2

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

[RFC v2 2/7] arm64: introduce check_features

[Peng Liang]

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;
+		}
+		exposed_mask |= arm64_ftr_mask(ftrp);
+	}
+
+	if (val & ~exposed_mask)
+		return -EINVAL;
+
+	return 0;
+}
-- 
2.26.2

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

[Andrew Jones]

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.

> +		}
> +		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

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

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

[Andrew Jones]

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.

> +		}
> +		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

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

[Peng Liang]

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

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

[Peng Liang]

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

[RFC v2 3/7] kvm: arm64: save ID registers to sys_regs file

[Peng Liang]

To emulate the ID registers, we need a place to storage the values of
the ID regsiters.  Maybe putting them in sysreg file is a good idea.

This commit has no functional changes but only code refactor.  When
initializing a vCPU, get the values of the ID registers from
arm64_ftr_regs and storage them in sysreg file.  And we just read
the value from sysreg file when getting/setting the value of the ID
regs.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/include/asm/kvm_coproc.h |  2 ++
 arch/arm64/include/asm/kvm_host.h   |  3 +++
 arch/arm64/kvm/arm.c                |  2 ++
 arch/arm64/kvm/sys_regs.c           | 33 +++++++++++++++++++++++++----
 4 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_coproc.h b/arch/arm64/include/asm/kvm_coproc.h
index d6bb40122fdb..76e8c3cb0662 100644
--- a/arch/arm64/include/asm/kvm_coproc.h
+++ b/arch/arm64/include/asm/kvm_coproc.h
@@ -35,4 +35,6 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
 int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
 unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu);
 
+void kvm_arm_sys_reg_init(struct kvm_vcpu *vcpu);
+
 #endif /* __ARM64_KVM_COPROC_H__ */
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 905c2b87e05a..50152e364c4f 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -184,6 +184,9 @@ enum vcpu_sysreg {
 	CNTP_CVAL_EL0,
 	CNTP_CTL_EL0,
 
+	ID_REG_BASE,
+	ID_REG_END = ID_REG_BASE + 55,
+
 	/* 32bit specific registers. Keep them at the end of the range */
 	DACR32_EL2,	/* Domain Access Control Register */
 	IFSR32_EL2,	/* Instruction Fault Status Register */
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index b588c3b5c2f0..6d961e192268 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -274,6 +274,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	if (err)
 		return err;
 
+	kvm_arm_sys_reg_init(vcpu);
+
 	return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
 }
 
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 077293b5115f..2b0fa8d5ac62 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1119,13 +1119,16 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+#define ID_REG_INDEX(id)						\
+	(ID_REG_BASE + (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id)))
+
 /* Read a sanitised cpufeature ID register by sys_reg_desc */
-static u64 read_id_reg(const struct kvm_vcpu *vcpu,
+static u64 read_id_reg(struct kvm_vcpu *vcpu,
 		struct sys_reg_desc const *r, bool raz)
 {
 	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
 			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
-	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
+	u64 val = raz ? 0 : __vcpu_sys_reg(vcpu, ID_REG_INDEX(id));
 
 	if (id == SYS_ID_AA64PFR0_EL1) {
 		if (!vcpu_has_sve(vcpu))
@@ -1265,7 +1268,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
  * are stored, and for set_id_reg() we don't allow the effective value
  * to be changed.
  */
-static int __get_id_reg(const struct kvm_vcpu *vcpu,
+static int __get_id_reg(struct kvm_vcpu *vcpu,
 			const struct sys_reg_desc *rd, void __user *uaddr,
 			bool raz)
 {
@@ -1275,7 +1278,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
 	return reg_to_user(uaddr, &val, id);
 }
 
-static int __set_id_reg(const struct kvm_vcpu *vcpu,
+static int __set_id_reg(struct kvm_vcpu *vcpu,
 			const struct sys_reg_desc *rd, void __user *uaddr,
 			bool raz)
 {
@@ -2854,3 +2857,25 @@ void kvm_sys_reg_table_init(void)
 	/* Clear all higher bits. */
 	cache_levels &= (1 << (i*3))-1;
 }
+
+static int get_cpu_ftr(u32 id, u64 val, void *argp)
+{
+	struct kvm_vcpu *vcpu = argp;
+
+	/*
+	 * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
+	 * where 1<=crm<8, 0<=op2<8.
+	 */
+	if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
+	    sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0 &&
+	    sys_reg_CRm(id) < 8) {
+		__vcpu_sys_reg(vcpu, ID_REG_INDEX(id)) = val;
+	}
+
+	return 0;
+}
+
+void kvm_arm_sys_reg_init(struct kvm_vcpu *vcpu)
+{
+	arm64_cpu_ftr_regs_traverse(get_cpu_ftr, vcpu);
+}
-- 
2.26.2

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

[RFC v2 3/7] kvm: arm64: save ID registers to sys_regs file

[Peng Liang]

To emulate the ID registers, we need a place to storage the values of
the ID regsiters.  Maybe putting them in sysreg file is a good idea.

This commit has no functional changes but only code refactor.  When
initializing a vCPU, get the values of the ID registers from
arm64_ftr_regs and storage them in sysreg file.  And we just read
the value from sysreg file when getting/setting the value of the ID
regs.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/include/asm/kvm_coproc.h |  2 ++
 arch/arm64/include/asm/kvm_host.h   |  3 +++
 arch/arm64/kvm/arm.c                |  2 ++
 arch/arm64/kvm/sys_regs.c           | 33 +++++++++++++++++++++++++----
 4 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_coproc.h b/arch/arm64/include/asm/kvm_coproc.h
index d6bb40122fdb..76e8c3cb0662 100644
--- a/arch/arm64/include/asm/kvm_coproc.h
+++ b/arch/arm64/include/asm/kvm_coproc.h
@@ -35,4 +35,6 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
 int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
 unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu);
 
+void kvm_arm_sys_reg_init(struct kvm_vcpu *vcpu);
+
 #endif /* __ARM64_KVM_COPROC_H__ */
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 905c2b87e05a..50152e364c4f 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -184,6 +184,9 @@ enum vcpu_sysreg {
 	CNTP_CVAL_EL0,
 	CNTP_CTL_EL0,
 
+	ID_REG_BASE,
+	ID_REG_END = ID_REG_BASE + 55,
+
 	/* 32bit specific registers. Keep them at the end of the range */
 	DACR32_EL2,	/* Domain Access Control Register */
 	IFSR32_EL2,	/* Instruction Fault Status Register */
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index b588c3b5c2f0..6d961e192268 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -274,6 +274,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	if (err)
 		return err;
 
+	kvm_arm_sys_reg_init(vcpu);
+
 	return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
 }
 
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 077293b5115f..2b0fa8d5ac62 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1119,13 +1119,16 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+#define ID_REG_INDEX(id)						\
+	(ID_REG_BASE + (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id)))
+
 /* Read a sanitised cpufeature ID register by sys_reg_desc */
-static u64 read_id_reg(const struct kvm_vcpu *vcpu,
+static u64 read_id_reg(struct kvm_vcpu *vcpu,
 		struct sys_reg_desc const *r, bool raz)
 {
 	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
 			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
-	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
+	u64 val = raz ? 0 : __vcpu_sys_reg(vcpu, ID_REG_INDEX(id));
 
 	if (id == SYS_ID_AA64PFR0_EL1) {
 		if (!vcpu_has_sve(vcpu))
@@ -1265,7 +1268,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
  * are stored, and for set_id_reg() we don't allow the effective value
  * to be changed.
  */
-static int __get_id_reg(const struct kvm_vcpu *vcpu,
+static int __get_id_reg(struct kvm_vcpu *vcpu,
 			const struct sys_reg_desc *rd, void __user *uaddr,
 			bool raz)
 {
@@ -1275,7 +1278,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
 	return reg_to_user(uaddr, &val, id);
 }
 
-static int __set_id_reg(const struct kvm_vcpu *vcpu,
+static int __set_id_reg(struct kvm_vcpu *vcpu,
 			const struct sys_reg_desc *rd, void __user *uaddr,
 			bool raz)
 {
@@ -2854,3 +2857,25 @@ void kvm_sys_reg_table_init(void)
 	/* Clear all higher bits. */
 	cache_levels &= (1 << (i*3))-1;
 }
+
+static int get_cpu_ftr(u32 id, u64 val, void *argp)
+{
+	struct kvm_vcpu *vcpu = argp;
+
+	/*
+	 * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
+	 * where 1<=crm<8, 0<=op2<8.
+	 */
+	if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
+	    sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0 &&
+	    sys_reg_CRm(id) < 8) {
+		__vcpu_sys_reg(vcpu, ID_REG_INDEX(id)) = val;
+	}
+
+	return 0;
+}
+
+void kvm_arm_sys_reg_init(struct kvm_vcpu *vcpu)
+{
+	arm64_cpu_ftr_regs_traverse(get_cpu_ftr, vcpu);
+}
-- 
2.26.2

Re: [RFC v2 3/7] kvm: arm64: save ID registers to sys_regs file

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:00:57PM +0800, Peng Liang wrote:
> To emulate the ID registers, we need a place to storage the values of
> the ID regsiters.  Maybe putting them in sysreg file is a good idea.
> 
> This commit has no functional changes but only code refactor.  When
> initializing a vCPU, get the values of the ID registers from
> arm64_ftr_regs and storage them in sysreg file.  And we just read
> the value from sysreg file when getting/setting the value of the ID
> regs.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/include/asm/kvm_coproc.h |  2 ++
>  arch/arm64/include/asm/kvm_host.h   |  3 +++
>  arch/arm64/kvm/arm.c                |  2 ++
>  arch/arm64/kvm/sys_regs.c           | 33 +++++++++++++++++++++++++----
>  4 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_coproc.h b/arch/arm64/include/asm/kvm_coproc.h
> index d6bb40122fdb..76e8c3cb0662 100644
> --- a/arch/arm64/include/asm/kvm_coproc.h
> +++ b/arch/arm64/include/asm/kvm_coproc.h
> @@ -35,4 +35,6 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
>  int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
>  unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu);
>  
> +void kvm_arm_sys_reg_init(struct kvm_vcpu *vcpu);
> +
>  #endif /* __ARM64_KVM_COPROC_H__ */
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 905c2b87e05a..50152e364c4f 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -184,6 +184,9 @@ enum vcpu_sysreg {
>  	CNTP_CVAL_EL0,
>  	CNTP_CTL_EL0,
>  
> +	ID_REG_BASE,
> +	ID_REG_END = ID_REG_BASE + 55,
> +
>  	/* 32bit specific registers. Keep them at the end of the range */
>  	DACR32_EL2,	/* Domain Access Control Register */
>  	IFSR32_EL2,	/* Instruction Fault Status Register */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index b588c3b5c2f0..6d961e192268 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -274,6 +274,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	if (err)
>  		return err;
>  
> +	kvm_arm_sys_reg_init(vcpu);
> +
>  	return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
>  }
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 077293b5115f..2b0fa8d5ac62 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1119,13 +1119,16 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +#define ID_REG_INDEX(id)						\
> +	(ID_REG_BASE + (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id)))
> +
>  /* Read a sanitised cpufeature ID register by sys_reg_desc */
> -static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> +static u64 read_id_reg(struct kvm_vcpu *vcpu,
>  		struct sys_reg_desc const *r, bool raz)
>  {
>  	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
>  			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
> -	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
> +	u64 val = raz ? 0 : __vcpu_sys_reg(vcpu, ID_REG_INDEX(id));
>  
>  	if (id == SYS_ID_AA64PFR0_EL1) {
>  		if (!vcpu_has_sve(vcpu))
> @@ -1265,7 +1268,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>   * are stored, and for set_id_reg() we don't allow the effective value
>   * to be changed.
>   */
> -static int __get_id_reg(const struct kvm_vcpu *vcpu,
> +static int __get_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
>  			bool raz)
>  {
> @@ -1275,7 +1278,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
>  	return reg_to_user(uaddr, &val, id);
>  }
>  
> -static int __set_id_reg(const struct kvm_vcpu *vcpu,
> +static int __set_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
>  			bool raz)
>  {
> @@ -2854,3 +2857,25 @@ void kvm_sys_reg_table_init(void)
>  	/* Clear all higher bits. */
>  	cache_levels &= (1 << (i*3))-1;
>  }
> +
> +static int get_cpu_ftr(u32 id, u64 val, void *argp)

'get' doesn't seem like the right verb here, but I don't think we need
this function anyway, since we don't need arm64_cpu_ftr_regs_traverse().
Just put the for-loop for arm64_cpu_ftr_regs_traverse() and this code
directly in kvm_arm_sys_reg_init().

> +{
> +	struct kvm_vcpu *vcpu = argp;
> +
> +	/*
> +	 * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
> +	 * where 1<=crm<8, 0<=op2<8.
> +	 */
> +	if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
> +	    sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0 &&
> +	    sys_reg_CRm(id) < 8) {
> +		__vcpu_sys_reg(vcpu, ID_REG_INDEX(id)) = val;
> +	}
> +
> +	return 0;
> +}
> +
> +void kvm_arm_sys_reg_init(struct kvm_vcpu *vcpu)
> +{
> +	arm64_cpu_ftr_regs_traverse(get_cpu_ftr, vcpu);
> +}
> -- 
> 2.26.2
>

Besides the pointless functions, this patch looks fine to me.

Thanks,
drew

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

Re: [RFC v2 3/7] kvm: arm64: save ID registers to sys_regs file

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:00:57PM +0800, Peng Liang wrote:
> To emulate the ID registers, we need a place to storage the values of
> the ID regsiters.  Maybe putting them in sysreg file is a good idea.
> 
> This commit has no functional changes but only code refactor.  When
> initializing a vCPU, get the values of the ID registers from
> arm64_ftr_regs and storage them in sysreg file.  And we just read
> the value from sysreg file when getting/setting the value of the ID
> regs.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/include/asm/kvm_coproc.h |  2 ++
>  arch/arm64/include/asm/kvm_host.h   |  3 +++
>  arch/arm64/kvm/arm.c                |  2 ++
>  arch/arm64/kvm/sys_regs.c           | 33 +++++++++++++++++++++++++----
>  4 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_coproc.h b/arch/arm64/include/asm/kvm_coproc.h
> index d6bb40122fdb..76e8c3cb0662 100644
> --- a/arch/arm64/include/asm/kvm_coproc.h
> +++ b/arch/arm64/include/asm/kvm_coproc.h
> @@ -35,4 +35,6 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
>  int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
>  unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu);
>  
> +void kvm_arm_sys_reg_init(struct kvm_vcpu *vcpu);
> +
>  #endif /* __ARM64_KVM_COPROC_H__ */
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 905c2b87e05a..50152e364c4f 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -184,6 +184,9 @@ enum vcpu_sysreg {
>  	CNTP_CVAL_EL0,
>  	CNTP_CTL_EL0,
>  
> +	ID_REG_BASE,
> +	ID_REG_END = ID_REG_BASE + 55,
> +
>  	/* 32bit specific registers. Keep them at the end of the range */
>  	DACR32_EL2,	/* Domain Access Control Register */
>  	IFSR32_EL2,	/* Instruction Fault Status Register */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index b588c3b5c2f0..6d961e192268 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -274,6 +274,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	if (err)
>  		return err;
>  
> +	kvm_arm_sys_reg_init(vcpu);
> +
>  	return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
>  }
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 077293b5115f..2b0fa8d5ac62 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1119,13 +1119,16 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +#define ID_REG_INDEX(id)						\
> +	(ID_REG_BASE + (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id)))
> +
>  /* Read a sanitised cpufeature ID register by sys_reg_desc */
> -static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> +static u64 read_id_reg(struct kvm_vcpu *vcpu,
>  		struct sys_reg_desc const *r, bool raz)
>  {
>  	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
>  			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
> -	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
> +	u64 val = raz ? 0 : __vcpu_sys_reg(vcpu, ID_REG_INDEX(id));
>  
>  	if (id == SYS_ID_AA64PFR0_EL1) {
>  		if (!vcpu_has_sve(vcpu))
> @@ -1265,7 +1268,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>   * are stored, and for set_id_reg() we don't allow the effective value
>   * to be changed.
>   */
> -static int __get_id_reg(const struct kvm_vcpu *vcpu,
> +static int __get_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
>  			bool raz)
>  {
> @@ -1275,7 +1278,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
>  	return reg_to_user(uaddr, &val, id);
>  }
>  
> -static int __set_id_reg(const struct kvm_vcpu *vcpu,
> +static int __set_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
>  			bool raz)
>  {
> @@ -2854,3 +2857,25 @@ void kvm_sys_reg_table_init(void)
>  	/* Clear all higher bits. */
>  	cache_levels &= (1 << (i*3))-1;
>  }
> +
> +static int get_cpu_ftr(u32 id, u64 val, void *argp)

'get' doesn't seem like the right verb here, but I don't think we need
this function anyway, since we don't need arm64_cpu_ftr_regs_traverse().
Just put the for-loop for arm64_cpu_ftr_regs_traverse() and this code
directly in kvm_arm_sys_reg_init().

> +{
> +	struct kvm_vcpu *vcpu = argp;
> +
> +	/*
> +	 * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
> +	 * where 1<=crm<8, 0<=op2<8.
> +	 */
> +	if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
> +	    sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0 &&
> +	    sys_reg_CRm(id) < 8) {
> +		__vcpu_sys_reg(vcpu, ID_REG_INDEX(id)) = val;
> +	}
> +
> +	return 0;
> +}
> +
> +void kvm_arm_sys_reg_init(struct kvm_vcpu *vcpu)
> +{
> +	arm64_cpu_ftr_regs_traverse(get_cpu_ftr, vcpu);
> +}
> -- 
> 2.26.2
>

Besides the pointless functions, this patch looks fine to me.

Thanks,
drew

[RFC v2 4/7] kvm: arm64: introduce check_user

[Peng Liang]

Currently, if we need to check the value of the register defined by user
space, we should check it in set_user.  However, some system registers
may use the same set_user (for example, almost all ID registers), which
make it difficult to validate the value defined by user space.

Introduce check_user to solve the problem.  And apply check_user before
set_user to make sure that the value of register is valid.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/kvm/sys_regs.c | 7 +++++++
 arch/arm64/kvm/sys_regs.h | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2b0fa8d5ac62..86ebb8093c3c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2684,6 +2684,7 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 {
 	const struct sys_reg_desc *r;
 	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
+	int err;
 
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
 		return demux_c15_set(reg->id, uaddr);
@@ -2699,6 +2700,12 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 	if (sysreg_hidden_from_user(vcpu, r))
 		return -ENOENT;
 
+	if (r->check_user) {
+		err = (r->check_user)(vcpu, r, reg, uaddr);
+		if (err)
+			return err;
+	}
+
 	if (r->set_user)
 		return (r->set_user)(vcpu, r, reg, uaddr);
 
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index 5a6fc30f5989..9bce5e9a3490 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -53,6 +53,12 @@ struct sys_reg_desc {
 			const struct kvm_one_reg *reg, void __user *uaddr);
 	int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 			const struct kvm_one_reg *reg, void __user *uaddr);
+	/*
+	 * Check the value userspace passed.  It should return 0 on success and
+	 * otherwise on failure.
+	 */
+	int (*check_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+			  const struct kvm_one_reg *reg, void __user *uaddr);
 
 	/* Return mask of REG_* runtime visibility overrides */
 	unsigned int (*visibility)(const struct kvm_vcpu *vcpu,
-- 
2.26.2

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

[RFC v2 4/7] kvm: arm64: introduce check_user

[Peng Liang]

Currently, if we need to check the value of the register defined by user
space, we should check it in set_user.  However, some system registers
may use the same set_user (for example, almost all ID registers), which
make it difficult to validate the value defined by user space.

Introduce check_user to solve the problem.  And apply check_user before
set_user to make sure that the value of register is valid.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/kvm/sys_regs.c | 7 +++++++
 arch/arm64/kvm/sys_regs.h | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2b0fa8d5ac62..86ebb8093c3c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2684,6 +2684,7 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 {
 	const struct sys_reg_desc *r;
 	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
+	int err;
 
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
 		return demux_c15_set(reg->id, uaddr);
@@ -2699,6 +2700,12 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 	if (sysreg_hidden_from_user(vcpu, r))
 		return -ENOENT;
 
+	if (r->check_user) {
+		err = (r->check_user)(vcpu, r, reg, uaddr);
+		if (err)
+			return err;
+	}
+
 	if (r->set_user)
 		return (r->set_user)(vcpu, r, reg, uaddr);
 
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index 5a6fc30f5989..9bce5e9a3490 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -53,6 +53,12 @@ struct sys_reg_desc {
 			const struct kvm_one_reg *reg, void __user *uaddr);
 	int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 			const struct kvm_one_reg *reg, void __user *uaddr);
+	/*
+	 * Check the value userspace passed.  It should return 0 on success and
+	 * otherwise on failure.
+	 */
+	int (*check_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+			  const struct kvm_one_reg *reg, void __user *uaddr);
 
 	/* Return mask of REG_* runtime visibility overrides */
 	unsigned int (*visibility)(const struct kvm_vcpu *vcpu,
-- 
2.26.2

Re: [RFC v2 4/7] kvm: arm64: introduce check_user

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:00:58PM +0800, Peng Liang wrote:
> Currently, if we need to check the value of the register defined by user
> space, we should check it in set_user.  However, some system registers
> may use the same set_user (for example, almost all ID registers), which
> make it difficult to validate the value defined by user space.

If sharing set_user no longer makes sense for ID registers, then we need
to rework the code so it's no longer shared. As I keep saying, we need
to address this problem one ID register at a time. So, IMO, the approach
should be to change one ID register at a time from using ID_SANITISED()
to having its own table entry with its own set/get_user code. There may
still be opportunity to share code among the ID registers, in which case
refactoring can be done as needed too.

Thanks,
drew

> 
> Introduce check_user to solve the problem.  And apply check_user before
> set_user to make sure that the value of register is valid.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 7 +++++++
>  arch/arm64/kvm/sys_regs.h | 6 ++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2b0fa8d5ac62..86ebb8093c3c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2684,6 +2684,7 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>  {
>  	const struct sys_reg_desc *r;
>  	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
> +	int err;
>  
>  	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
>  		return demux_c15_set(reg->id, uaddr);
> @@ -2699,6 +2700,12 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>  	if (sysreg_hidden_from_user(vcpu, r))
>  		return -ENOENT;
>  
> +	if (r->check_user) {
> +		err = (r->check_user)(vcpu, r, reg, uaddr);
> +		if (err)
> +			return err;
> +	}
> +
>  	if (r->set_user)
>  		return (r->set_user)(vcpu, r, reg, uaddr);
>  
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index 5a6fc30f5989..9bce5e9a3490 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -53,6 +53,12 @@ struct sys_reg_desc {
>  			const struct kvm_one_reg *reg, void __user *uaddr);
>  	int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  			const struct kvm_one_reg *reg, void __user *uaddr);
> +	/*
> +	 * Check the value userspace passed.  It should return 0 on success and
> +	 * otherwise on failure.
> +	 */
> +	int (*check_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +			  const struct kvm_one_reg *reg, void __user *uaddr);
>  
>  	/* Return mask of REG_* runtime visibility overrides */
>  	unsigned int (*visibility)(const struct kvm_vcpu *vcpu,
> -- 
> 2.26.2
> 

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

Re: [RFC v2 4/7] kvm: arm64: introduce check_user

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:00:58PM +0800, Peng Liang wrote:
> Currently, if we need to check the value of the register defined by user
> space, we should check it in set_user.  However, some system registers
> may use the same set_user (for example, almost all ID registers), which
> make it difficult to validate the value defined by user space.

If sharing set_user no longer makes sense for ID registers, then we need
to rework the code so it's no longer shared. As I keep saying, we need
to address this problem one ID register at a time. So, IMO, the approach
should be to change one ID register at a time from using ID_SANITISED()
to having its own table entry with its own set/get_user code. There may
still be opportunity to share code among the ID registers, in which case
refactoring can be done as needed too.

Thanks,
drew

> 
> Introduce check_user to solve the problem.  And apply check_user before
> set_user to make sure that the value of register is valid.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 7 +++++++
>  arch/arm64/kvm/sys_regs.h | 6 ++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2b0fa8d5ac62..86ebb8093c3c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2684,6 +2684,7 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>  {
>  	const struct sys_reg_desc *r;
>  	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
> +	int err;
>  
>  	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
>  		return demux_c15_set(reg->id, uaddr);
> @@ -2699,6 +2700,12 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>  	if (sysreg_hidden_from_user(vcpu, r))
>  		return -ENOENT;
>  
> +	if (r->check_user) {
> +		err = (r->check_user)(vcpu, r, reg, uaddr);
> +		if (err)
> +			return err;
> +	}
> +
>  	if (r->set_user)
>  		return (r->set_user)(vcpu, r, reg, uaddr);
>  
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index 5a6fc30f5989..9bce5e9a3490 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -53,6 +53,12 @@ struct sys_reg_desc {
>  			const struct kvm_one_reg *reg, void __user *uaddr);
>  	int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  			const struct kvm_one_reg *reg, void __user *uaddr);
> +	/*
> +	 * Check the value userspace passed.  It should return 0 on success and
> +	 * otherwise on failure.
> +	 */
> +	int (*check_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +			  const struct kvm_one_reg *reg, void __user *uaddr);
>  
>  	/* Return mask of REG_* runtime visibility overrides */
>  	unsigned int (*visibility)(const struct kvm_vcpu *vcpu,
> -- 
> 2.26.2
> 

Re: [RFC v2 4/7] kvm: arm64: introduce check_user

[Peng Liang]

On 9/18/2020 3:41 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:00:58PM +0800, Peng Liang wrote:
>> Currently, if we need to check the value of the register defined by user
>> space, we should check it in set_user.  However, some system registers
>> may use the same set_user (for example, almost all ID registers), which
>> make it difficult to validate the value defined by user space.
> 
> If sharing set_user no longer makes sense for ID registers, then we need
> to rework the code so it's no longer shared. As I keep saying, we need
> to address this problem one ID register at a time. So, IMO, the approach
> should be to change one ID register at a time from using ID_SANITISED()
> to having its own table entry with its own set/get_user code. There may
> still be opportunity to share code among the ID registers, in which case
> refactoring can be done as needed too.
> 
> Thanks,
> drew
> 

Thank you for your advise.  Currently, the implementation is a little dirty.
Removing the shared set_user of ID registers should make it clean.  I will
refactor the code to make it in next version.

Thanks,
Peng

>>
>> Introduce check_user to solve the problem.  And apply check_user before
>> set_user to make sure that the value of register is valid.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 7 +++++++
>>  arch/arm64/kvm/sys_regs.h | 6 ++++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 2b0fa8d5ac62..86ebb8093c3c 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2684,6 +2684,7 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>>  {
>>  	const struct sys_reg_desc *r;
>>  	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
>> +	int err;
>>  
>>  	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
>>  		return demux_c15_set(reg->id, uaddr);
>> @@ -2699,6 +2700,12 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>>  	if (sysreg_hidden_from_user(vcpu, r))
>>  		return -ENOENT;
>>  
>> +	if (r->check_user) {
>> +		err = (r->check_user)(vcpu, r, reg, uaddr);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>>  	if (r->set_user)
>>  		return (r->set_user)(vcpu, r, reg, uaddr);
>>  
>> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
>> index 5a6fc30f5989..9bce5e9a3490 100644
>> --- a/arch/arm64/kvm/sys_regs.h
>> +++ b/arch/arm64/kvm/sys_regs.h
>> @@ -53,6 +53,12 @@ struct sys_reg_desc {
>>  			const struct kvm_one_reg *reg, void __user *uaddr);
>>  	int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>>  			const struct kvm_one_reg *reg, void __user *uaddr);
>> +	/*
>> +	 * Check the value userspace passed.  It should return 0 on success and
>> +	 * otherwise on failure.
>> +	 */
>> +	int (*check_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> +			  const struct kvm_one_reg *reg, void __user *uaddr);
>>  
>>  	/* Return mask of REG_* runtime visibility overrides */
>>  	unsigned int (*visibility)(const struct kvm_vcpu *vcpu,
>> -- 
>> 2.26.2
>>
> 
> .
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC v2 4/7] kvm: arm64: introduce check_user

[Peng Liang]

On 9/18/2020 3:41 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:00:58PM +0800, Peng Liang wrote:
>> Currently, if we need to check the value of the register defined by user
>> space, we should check it in set_user.  However, some system registers
>> may use the same set_user (for example, almost all ID registers), which
>> make it difficult to validate the value defined by user space.
> 
> If sharing set_user no longer makes sense for ID registers, then we need
> to rework the code so it's no longer shared. As I keep saying, we need
> to address this problem one ID register at a time. So, IMO, the approach
> should be to change one ID register at a time from using ID_SANITISED()
> to having its own table entry with its own set/get_user code. There may
> still be opportunity to share code among the ID registers, in which case
> refactoring can be done as needed too.
> 
> Thanks,
> drew
> 

Thank you for your advise.  Currently, the implementation is a little dirty.
Removing the shared set_user of ID registers should make it clean.  I will
refactor the code to make it in next version.

Thanks,
Peng

>>
>> Introduce check_user to solve the problem.  And apply check_user before
>> set_user to make sure that the value of register is valid.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 7 +++++++
>>  arch/arm64/kvm/sys_regs.h | 6 ++++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 2b0fa8d5ac62..86ebb8093c3c 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2684,6 +2684,7 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>>  {
>>  	const struct sys_reg_desc *r;
>>  	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
>> +	int err;
>>  
>>  	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
>>  		return demux_c15_set(reg->id, uaddr);
>> @@ -2699,6 +2700,12 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>>  	if (sysreg_hidden_from_user(vcpu, r))
>>  		return -ENOENT;
>>  
>> +	if (r->check_user) {
>> +		err = (r->check_user)(vcpu, r, reg, uaddr);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>>  	if (r->set_user)
>>  		return (r->set_user)(vcpu, r, reg, uaddr);
>>  
>> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
>> index 5a6fc30f5989..9bce5e9a3490 100644
>> --- a/arch/arm64/kvm/sys_regs.h
>> +++ b/arch/arm64/kvm/sys_regs.h
>> @@ -53,6 +53,12 @@ struct sys_reg_desc {
>>  			const struct kvm_one_reg *reg, void __user *uaddr);
>>  	int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>>  			const struct kvm_one_reg *reg, void __user *uaddr);
>> +	/*
>> +	 * Check the value userspace passed.  It should return 0 on success and
>> +	 * otherwise on failure.
>> +	 */
>> +	int (*check_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> +			  const struct kvm_one_reg *reg, void __user *uaddr);
>>  
>>  	/* Return mask of REG_* runtime visibility overrides */
>>  	unsigned int (*visibility)(const struct kvm_vcpu *vcpu,
>> -- 
>> 2.26.2
>>
> 
> .
> 

[RFC v2 5/7] kvm: arm64: implement check_user for ID registers

[Peng Liang]

For most ID registers, only neeed to check each field defined by user
space is no more than that in host and only the fields we want to
exposed to guest is set.  For some ID registers, the relationship
between some fields need to be check or we'd better to keep the same
value as host for some fields.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/kvm/sys_regs.c | 425 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 424 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 86ebb8093c3c..a642ecfebe0a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1385,12 +1385,433 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	return true;
 }
 
+#define ID_CHECKER(reg) __check_ ##reg
+
+static int __general_id_checker(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+
+	return check_features(reg_id, val);
+}
+
+static int ID_CHECKER(ID_PFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_PFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_PFR2_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_DFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val, host_val;
+	u64 mask = ((0xfUL << ID_DFR0_PERFMON_SHIFT) |
+		    (0xfUL << ID_DFR0_MMAPDBG_SHIFT) |
+		    (0xfUL << ID_DFR0_COPDBG_SHIFT) |
+		    (0xfUL << ID_DFR0_COPDBG_SHIFT));
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	host_val = read_sanitised_ftr_reg(reg_id);
+	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(ID_MMFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_MMFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_MMFR2_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_MMFR3_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_MMFR4_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_MMFR5_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR2_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR3_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR4_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR5_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR6_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(MVFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(MVFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val;
+	unsigned int fphp, simdhp;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	fphp = cpuid_feature_extract_signed_field(val, MVFR1_FPHP_SHIFT);
+	simdhp = cpuid_feature_extract_signed_field(val, MVFR1_SIMDHP_SHIFT);
+	return ((fphp == 0 && simdhp == 0) || (fphp == 2 && simdhp == 1) ||
+		(fphp == 3 && simdhp == 2)) ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(MVFR2_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_AA64PFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val;
+	unsigned int fp, asimd;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT);
+	asimd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT);
+	return fp == asimd ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(ID_AA64PFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_AA64DFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val, host_val;
+	u64 mask = ((0xfUL << ID_AA64DFR0_PMUVER_SHIFT) |
+		    (0xfUL << ID_AA64DFR0_DEBUGVER_SHIFT) |
+		    (0xfUL << ID_AA64DFR0_CTX_CMPS_SHIFT) |
+		    (0xfUL << ID_AA64DFR0_WRPS_SHIFT) |
+		    (0xfUL << ID_AA64DFR0_BRPS_SHIFT));
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	host_val = read_sanitised_ftr_reg(reg_id);
+	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(ID_AA64DFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_AA64ISAR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val;
+	unsigned int sm3, sm4, sha1, sha2, sha3;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	sm3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM3_SHIFT);
+	sm4 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM4_SHIFT);
+	/*
+	 * ID_AA64ISAR0_EL1.SM3 and ID_AA64ISAR0_EL1.SM4 must have the same
+	 * value.
+	 */
+	if (sm3 != sm4)
+		return -EINVAL;
+
+	sha1 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA1_SHIFT);
+	sha2 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA2_SHIFT);
+	sha3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA3_SHIFT);
+	/*
+	 * 1. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
+	 *    ID_AA64ISAR0_EL1.SHA2 must have the value 0, and vice versa;
+	 * 2. If the value of ID_AA64ISAR0_EL1.SHA2 is 2, then
+	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 1, and vice versa;
+	 * 3. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
+	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 0;
+	 */
+	if ((sha1 ^ sha2) || ((sha2 == 2) ^ (sha3 == 1)) || (!sha1 && sha3))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ID_CHECKER(ID_AA64ISAR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val;
+	unsigned int gpi, gpa, api, apa;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	gpi = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPI_SHIFT);
+	gpa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPA_SHIFT);
+	api = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_API_SHIFT);
+	apa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_APA_SHIFT);
+	/*
+	 * 1. If the value of ID_AA64ISAR1_EL1.GPA is non-zero, then
+	 *    ID_AA64ISAR1_EL1.GPI must have the value 0;
+	 * 2. If the value of ID_AA64ISAR1_EL1.GPI is non-zero, then
+	 *    ID_AA64ISAR1_EL1.GPA must have the value 0;
+	 * 3. If the value of ID_AA64ISAR1_EL1.APA is non-zero, then
+	 *    ID_AA64ISAR1_EL1.API must have the value 0;
+	 * 4. If the value of ID_AA64ISAR1_EL1.API is non-zero, then
+	 *    ID_AA64ISAR1_EL1.APA must have the value 0;
+	 */
+	if ((gpi && gpa) || (api && apa))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ID_CHECKER(ID_AA64MMFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val, host_val;
+	u64 mask = ((0xfUL << ID_AA64MMFR0_TGRAN4_2_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_TGRAN64_2_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_TGRAN16_2_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_TGRAN4_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_TGRAN64_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_TGRAN16_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_ASID_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_PARANGE_SHIFT));
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	host_val = read_sanitised_ftr_reg(reg_id);
+	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(ID_AA64MMFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val, host_val;
+	unsigned int vmidbits, host_vmidbits;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	vmidbits = cpuid_feature_extract_unsigned_field(val, ID_AA64MMFR1_VMIDBITS_SHIFT);
+	host_val = read_sanitised_ftr_reg(reg_id);
+	host_vmidbits = cpuid_feature_extract_signed_field(host_val, ID_AA64MMFR1_VMIDBITS_SHIFT);
+	return vmidbits == host_vmidbits ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(ID_AA64MMFR2_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
 /* sys_reg_desc initialiser for known cpufeature ID registers */
 #define ID_SANITISED(name) {			\
 	SYS_DESC(SYS_##name),			\
 	.access	= access_id_reg,		\
 	.get_user = get_id_reg,			\
 	.set_user = set_id_reg,			\
+	.check_user = ID_CHECKER(name),		\
 }
 
 /*
@@ -1512,7 +1933,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	ID_SANITISED(ID_AA64PFR1_EL1),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
-	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
+	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1,
+	  .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1,
+	  .check_user = __general_id_checker, .visibility = sve_id_visibility },
 	ID_UNALLOCATED(4,5),
 	ID_UNALLOCATED(4,6),
 	ID_UNALLOCATED(4,7),
-- 
2.26.2

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

[RFC v2 5/7] kvm: arm64: implement check_user for ID registers

[Peng Liang]

For most ID registers, only neeed to check each field defined by user
space is no more than that in host and only the fields we want to
exposed to guest is set.  For some ID registers, the relationship
between some fields need to be check or we'd better to keep the same
value as host for some fields.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/kvm/sys_regs.c | 425 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 424 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 86ebb8093c3c..a642ecfebe0a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1385,12 +1385,433 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	return true;
 }
 
+#define ID_CHECKER(reg) __check_ ##reg
+
+static int __general_id_checker(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+
+	return check_features(reg_id, val);
+}
+
+static int ID_CHECKER(ID_PFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_PFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_PFR2_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_DFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val, host_val;
+	u64 mask = ((0xfUL << ID_DFR0_PERFMON_SHIFT) |
+		    (0xfUL << ID_DFR0_MMAPDBG_SHIFT) |
+		    (0xfUL << ID_DFR0_COPDBG_SHIFT) |
+		    (0xfUL << ID_DFR0_COPDBG_SHIFT));
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	host_val = read_sanitised_ftr_reg(reg_id);
+	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(ID_MMFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_MMFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_MMFR2_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_MMFR3_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_MMFR4_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_MMFR5_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR2_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR3_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR4_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR5_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_ISAR6_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(MVFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(MVFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val;
+	unsigned int fphp, simdhp;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	fphp = cpuid_feature_extract_signed_field(val, MVFR1_FPHP_SHIFT);
+	simdhp = cpuid_feature_extract_signed_field(val, MVFR1_SIMDHP_SHIFT);
+	return ((fphp == 0 && simdhp == 0) || (fphp == 2 && simdhp == 1) ||
+		(fphp == 3 && simdhp == 2)) ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(MVFR2_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_AA64PFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val;
+	unsigned int fp, asimd;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT);
+	asimd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT);
+	return fp == asimd ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(ID_AA64PFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_AA64DFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val, host_val;
+	u64 mask = ((0xfUL << ID_AA64DFR0_PMUVER_SHIFT) |
+		    (0xfUL << ID_AA64DFR0_DEBUGVER_SHIFT) |
+		    (0xfUL << ID_AA64DFR0_CTX_CMPS_SHIFT) |
+		    (0xfUL << ID_AA64DFR0_WRPS_SHIFT) |
+		    (0xfUL << ID_AA64DFR0_BRPS_SHIFT));
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	host_val = read_sanitised_ftr_reg(reg_id);
+	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(ID_AA64DFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
+static int ID_CHECKER(ID_AA64ISAR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val;
+	unsigned int sm3, sm4, sha1, sha2, sha3;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	sm3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM3_SHIFT);
+	sm4 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM4_SHIFT);
+	/*
+	 * ID_AA64ISAR0_EL1.SM3 and ID_AA64ISAR0_EL1.SM4 must have the same
+	 * value.
+	 */
+	if (sm3 != sm4)
+		return -EINVAL;
+
+	sha1 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA1_SHIFT);
+	sha2 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA2_SHIFT);
+	sha3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA3_SHIFT);
+	/*
+	 * 1. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
+	 *    ID_AA64ISAR0_EL1.SHA2 must have the value 0, and vice versa;
+	 * 2. If the value of ID_AA64ISAR0_EL1.SHA2 is 2, then
+	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 1, and vice versa;
+	 * 3. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
+	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 0;
+	 */
+	if ((sha1 ^ sha2) || ((sha2 == 2) ^ (sha3 == 1)) || (!sha1 && sha3))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ID_CHECKER(ID_AA64ISAR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val;
+	unsigned int gpi, gpa, api, apa;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	gpi = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPI_SHIFT);
+	gpa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPA_SHIFT);
+	api = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_API_SHIFT);
+	apa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_APA_SHIFT);
+	/*
+	 * 1. If the value of ID_AA64ISAR1_EL1.GPA is non-zero, then
+	 *    ID_AA64ISAR1_EL1.GPI must have the value 0;
+	 * 2. If the value of ID_AA64ISAR1_EL1.GPI is non-zero, then
+	 *    ID_AA64ISAR1_EL1.GPA must have the value 0;
+	 * 3. If the value of ID_AA64ISAR1_EL1.APA is non-zero, then
+	 *    ID_AA64ISAR1_EL1.API must have the value 0;
+	 * 4. If the value of ID_AA64ISAR1_EL1.API is non-zero, then
+	 *    ID_AA64ISAR1_EL1.APA must have the value 0;
+	 */
+	if ((gpi && gpa) || (api && apa))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ID_CHECKER(ID_AA64MMFR0_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val, host_val;
+	u64 mask = ((0xfUL << ID_AA64MMFR0_TGRAN4_2_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_TGRAN64_2_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_TGRAN16_2_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_TGRAN4_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_TGRAN64_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_TGRAN16_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_ASID_SHIFT) |
+		    (0xfUL << ID_AA64MMFR0_PARANGE_SHIFT));
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	host_val = read_sanitised_ftr_reg(reg_id);
+	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(ID_AA64MMFR1_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+			     (u32)rd->CRm, (u32)rd->Op2);
+	int err;
+	u64 val, host_val;
+	unsigned int vmidbits, host_vmidbits;
+
+	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
+	if (err)
+		return err;
+	err = check_features(reg_id, val);
+	if (err)
+		return err;
+
+	vmidbits = cpuid_feature_extract_unsigned_field(val, ID_AA64MMFR1_VMIDBITS_SHIFT);
+	host_val = read_sanitised_ftr_reg(reg_id);
+	host_vmidbits = cpuid_feature_extract_signed_field(host_val, ID_AA64MMFR1_VMIDBITS_SHIFT);
+	return vmidbits == host_vmidbits ? 0 : -EINVAL;
+}
+
+static int ID_CHECKER(ID_AA64MMFR2_EL1)(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				const struct kvm_one_reg *reg,
+				void __user *uaddr)
+{
+	return __general_id_checker(vcpu, rd, reg, uaddr);
+}
+
 /* sys_reg_desc initialiser for known cpufeature ID registers */
 #define ID_SANITISED(name) {			\
 	SYS_DESC(SYS_##name),			\
 	.access	= access_id_reg,		\
 	.get_user = get_id_reg,			\
 	.set_user = set_id_reg,			\
+	.check_user = ID_CHECKER(name),		\
 }
 
 /*
@@ -1512,7 +1933,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	ID_SANITISED(ID_AA64PFR1_EL1),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
-	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
+	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1,
+	  .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1,
+	  .check_user = __general_id_checker, .visibility = sve_id_visibility },
 	ID_UNALLOCATED(4,5),
 	ID_UNALLOCATED(4,6),
 	ID_UNALLOCATED(4,7),
-- 
2.26.2

Re: [RFC v2 5/7] kvm: arm64: implement check_user for ID registers

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:00:59PM +0800, Peng Liang wrote:
> For most ID registers, only neeed to check each field defined by user
> space is no more than that in host and only the fields we want to
> exposed to guest is set.  For some ID registers, the relationship
> between some fields need to be check or we'd better to keep the same
> value as host for some fields.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 425 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 424 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 86ebb8093c3c..a642ecfebe0a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1385,12 +1385,433 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	return true;
>  }
>  
> +#define ID_CHECKER(reg) __check_ ##reg
> +
> +static int __general_id_checker(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +
> +	return check_features(reg_id, val);
> +}
> +
> +static int ID_CHECKER(ID_PFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_PFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_PFR2_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_DFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val, host_val;
> +	u64 mask = ((0xfUL << ID_DFR0_PERFMON_SHIFT) |
> +		    (0xfUL << ID_DFR0_MMAPDBG_SHIFT) |
> +		    (0xfUL << ID_DFR0_COPDBG_SHIFT) |
> +		    (0xfUL << ID_DFR0_COPDBG_SHIFT));
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	host_val = read_sanitised_ftr_reg(reg_id);
> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(ID_MMFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_MMFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_MMFR2_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_MMFR3_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_MMFR4_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_MMFR5_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR2_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR3_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR4_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR5_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR6_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(MVFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}

There has to be a better way to handle all these redundant functions...

> +
> +static int ID_CHECKER(MVFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val;
> +	unsigned int fphp, simdhp;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	fphp = cpuid_feature_extract_signed_field(val, MVFR1_FPHP_SHIFT);
> +	simdhp = cpuid_feature_extract_signed_field(val, MVFR1_SIMDHP_SHIFT);
> +	return ((fphp == 0 && simdhp == 0) || (fphp == 2 && simdhp == 1) ||
> +		(fphp == 3 && simdhp == 2)) ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(MVFR2_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_AA64PFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val;
> +	unsigned int fp, asimd;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT);
> +	asimd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT);
> +	return fp == asimd ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(ID_AA64PFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_AA64DFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val, host_val;
> +	u64 mask = ((0xfUL << ID_AA64DFR0_PMUVER_SHIFT) |
> +		    (0xfUL << ID_AA64DFR0_DEBUGVER_SHIFT) |
> +		    (0xfUL << ID_AA64DFR0_CTX_CMPS_SHIFT) |
> +		    (0xfUL << ID_AA64DFR0_WRPS_SHIFT) |
> +		    (0xfUL << ID_AA64DFR0_BRPS_SHIFT));
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	host_val = read_sanitised_ftr_reg(reg_id);
> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(ID_AA64DFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_AA64ISAR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val;
> +	unsigned int sm3, sm4, sha1, sha2, sha3;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	sm3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM3_SHIFT);
> +	sm4 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM4_SHIFT);
> +	/*
> +	 * ID_AA64ISAR0_EL1.SM3 and ID_AA64ISAR0_EL1.SM4 must have the same
> +	 * value.
> +	 */
> +	if (sm3 != sm4)
> +		return -EINVAL;
> +
> +	sha1 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA1_SHIFT);
> +	sha2 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA2_SHIFT);
> +	sha3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA3_SHIFT);
> +	/*
> +	 * 1. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
> +	 *    ID_AA64ISAR0_EL1.SHA2 must have the value 0, and vice versa;
> +	 * 2. If the value of ID_AA64ISAR0_EL1.SHA2 is 2, then
> +	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 1, and vice versa;
> +	 * 3. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
> +	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 0;
> +	 */
> +	if ((sha1 ^ sha2) || ((sha2 == 2) ^ (sha3 == 1)) || (!sha1 && sha3))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ID_CHECKER(ID_AA64ISAR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val;
> +	unsigned int gpi, gpa, api, apa;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	gpi = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPI_SHIFT);
> +	gpa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPA_SHIFT);
> +	api = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_API_SHIFT);
> +	apa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_APA_SHIFT);
> +	/*
> +	 * 1. If the value of ID_AA64ISAR1_EL1.GPA is non-zero, then
> +	 *    ID_AA64ISAR1_EL1.GPI must have the value 0;
> +	 * 2. If the value of ID_AA64ISAR1_EL1.GPI is non-zero, then
> +	 *    ID_AA64ISAR1_EL1.GPA must have the value 0;
> +	 * 3. If the value of ID_AA64ISAR1_EL1.APA is non-zero, then
> +	 *    ID_AA64ISAR1_EL1.API must have the value 0;
> +	 * 4. If the value of ID_AA64ISAR1_EL1.API is non-zero, then
> +	 *    ID_AA64ISAR1_EL1.APA must have the value 0;
> +	 */
> +	if ((gpi && gpa) || (api && apa))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ID_CHECKER(ID_AA64MMFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val, host_val;
> +	u64 mask = ((0xfUL << ID_AA64MMFR0_TGRAN4_2_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_TGRAN64_2_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_TGRAN16_2_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_TGRAN4_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_TGRAN64_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_TGRAN16_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_ASID_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_PARANGE_SHIFT));
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	host_val = read_sanitised_ftr_reg(reg_id);
> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(ID_AA64MMFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val, host_val;
> +	unsigned int vmidbits, host_vmidbits;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	vmidbits = cpuid_feature_extract_unsigned_field(val, ID_AA64MMFR1_VMIDBITS_SHIFT);
> +	host_val = read_sanitised_ftr_reg(reg_id);
> +	host_vmidbits = cpuid_feature_extract_signed_field(host_val, ID_AA64MMFR1_VMIDBITS_SHIFT);
> +	return vmidbits == host_vmidbits ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(ID_AA64MMFR2_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
>  /* sys_reg_desc initialiser for known cpufeature ID registers */
>  #define ID_SANITISED(name) {			\
>  	SYS_DESC(SYS_##name),			\
>  	.access	= access_id_reg,		\
>  	.get_user = get_id_reg,			\
>  	.set_user = set_id_reg,			\
> +	.check_user = ID_CHECKER(name),		\

This patch makes it clear that continuing to use ID_SANITISED() for all ID
registers makes no sense.

>  }
>  
>  /*
> @@ -1512,7 +1933,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	ID_SANITISED(ID_AA64PFR1_EL1),
>  	ID_UNALLOCATED(4,2),
>  	ID_UNALLOCATED(4,3),
> -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
> +	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1,
> +	  .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1,
> +	  .check_user = __general_id_checker, .visibility = sve_id_visibility },
>  	ID_UNALLOCATED(4,5),
>  	ID_UNALLOCATED(4,6),
>  	ID_UNALLOCATED(4,7),
> -- 
> 2.26.2
>

Thanks,
drew 

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

Re: [RFC v2 5/7] kvm: arm64: implement check_user for ID registers

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:00:59PM +0800, Peng Liang wrote:
> For most ID registers, only neeed to check each field defined by user
> space is no more than that in host and only the fields we want to
> exposed to guest is set.  For some ID registers, the relationship
> between some fields need to be check or we'd better to keep the same
> value as host for some fields.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 425 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 424 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 86ebb8093c3c..a642ecfebe0a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1385,12 +1385,433 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	return true;
>  }
>  
> +#define ID_CHECKER(reg) __check_ ##reg
> +
> +static int __general_id_checker(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +
> +	return check_features(reg_id, val);
> +}
> +
> +static int ID_CHECKER(ID_PFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_PFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_PFR2_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_DFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val, host_val;
> +	u64 mask = ((0xfUL << ID_DFR0_PERFMON_SHIFT) |
> +		    (0xfUL << ID_DFR0_MMAPDBG_SHIFT) |
> +		    (0xfUL << ID_DFR0_COPDBG_SHIFT) |
> +		    (0xfUL << ID_DFR0_COPDBG_SHIFT));
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	host_val = read_sanitised_ftr_reg(reg_id);
> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(ID_MMFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_MMFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_MMFR2_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_MMFR3_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_MMFR4_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_MMFR5_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR2_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR3_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR4_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR5_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_ISAR6_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(MVFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}

There has to be a better way to handle all these redundant functions...

> +
> +static int ID_CHECKER(MVFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val;
> +	unsigned int fphp, simdhp;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	fphp = cpuid_feature_extract_signed_field(val, MVFR1_FPHP_SHIFT);
> +	simdhp = cpuid_feature_extract_signed_field(val, MVFR1_SIMDHP_SHIFT);
> +	return ((fphp == 0 && simdhp == 0) || (fphp == 2 && simdhp == 1) ||
> +		(fphp == 3 && simdhp == 2)) ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(MVFR2_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_AA64PFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val;
> +	unsigned int fp, asimd;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT);
> +	asimd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT);
> +	return fp == asimd ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(ID_AA64PFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_AA64DFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val, host_val;
> +	u64 mask = ((0xfUL << ID_AA64DFR0_PMUVER_SHIFT) |
> +		    (0xfUL << ID_AA64DFR0_DEBUGVER_SHIFT) |
> +		    (0xfUL << ID_AA64DFR0_CTX_CMPS_SHIFT) |
> +		    (0xfUL << ID_AA64DFR0_WRPS_SHIFT) |
> +		    (0xfUL << ID_AA64DFR0_BRPS_SHIFT));
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	host_val = read_sanitised_ftr_reg(reg_id);
> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(ID_AA64DFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
> +static int ID_CHECKER(ID_AA64ISAR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val;
> +	unsigned int sm3, sm4, sha1, sha2, sha3;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	sm3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM3_SHIFT);
> +	sm4 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM4_SHIFT);
> +	/*
> +	 * ID_AA64ISAR0_EL1.SM3 and ID_AA64ISAR0_EL1.SM4 must have the same
> +	 * value.
> +	 */
> +	if (sm3 != sm4)
> +		return -EINVAL;
> +
> +	sha1 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA1_SHIFT);
> +	sha2 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA2_SHIFT);
> +	sha3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA3_SHIFT);
> +	/*
> +	 * 1. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
> +	 *    ID_AA64ISAR0_EL1.SHA2 must have the value 0, and vice versa;
> +	 * 2. If the value of ID_AA64ISAR0_EL1.SHA2 is 2, then
> +	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 1, and vice versa;
> +	 * 3. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
> +	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 0;
> +	 */
> +	if ((sha1 ^ sha2) || ((sha2 == 2) ^ (sha3 == 1)) || (!sha1 && sha3))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ID_CHECKER(ID_AA64ISAR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val;
> +	unsigned int gpi, gpa, api, apa;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	gpi = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPI_SHIFT);
> +	gpa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPA_SHIFT);
> +	api = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_API_SHIFT);
> +	apa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_APA_SHIFT);
> +	/*
> +	 * 1. If the value of ID_AA64ISAR1_EL1.GPA is non-zero, then
> +	 *    ID_AA64ISAR1_EL1.GPI must have the value 0;
> +	 * 2. If the value of ID_AA64ISAR1_EL1.GPI is non-zero, then
> +	 *    ID_AA64ISAR1_EL1.GPA must have the value 0;
> +	 * 3. If the value of ID_AA64ISAR1_EL1.APA is non-zero, then
> +	 *    ID_AA64ISAR1_EL1.API must have the value 0;
> +	 * 4. If the value of ID_AA64ISAR1_EL1.API is non-zero, then
> +	 *    ID_AA64ISAR1_EL1.APA must have the value 0;
> +	 */
> +	if ((gpi && gpa) || (api && apa))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ID_CHECKER(ID_AA64MMFR0_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val, host_val;
> +	u64 mask = ((0xfUL << ID_AA64MMFR0_TGRAN4_2_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_TGRAN64_2_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_TGRAN16_2_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_TGRAN4_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_TGRAN64_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_TGRAN16_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_ASID_SHIFT) |
> +		    (0xfUL << ID_AA64MMFR0_PARANGE_SHIFT));
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	host_val = read_sanitised_ftr_reg(reg_id);
> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(ID_AA64MMFR1_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +			     (u32)rd->CRm, (u32)rd->Op2);
> +	int err;
> +	u64 val, host_val;
> +	unsigned int vmidbits, host_vmidbits;
> +
> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> +	if (err)
> +		return err;
> +	err = check_features(reg_id, val);
> +	if (err)
> +		return err;
> +
> +	vmidbits = cpuid_feature_extract_unsigned_field(val, ID_AA64MMFR1_VMIDBITS_SHIFT);
> +	host_val = read_sanitised_ftr_reg(reg_id);
> +	host_vmidbits = cpuid_feature_extract_signed_field(host_val, ID_AA64MMFR1_VMIDBITS_SHIFT);
> +	return vmidbits == host_vmidbits ? 0 : -EINVAL;
> +}
> +
> +static int ID_CHECKER(ID_AA64MMFR2_EL1)(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_desc *rd,
> +				const struct kvm_one_reg *reg,
> +				void __user *uaddr)
> +{
> +	return __general_id_checker(vcpu, rd, reg, uaddr);
> +}
> +
>  /* sys_reg_desc initialiser for known cpufeature ID registers */
>  #define ID_SANITISED(name) {			\
>  	SYS_DESC(SYS_##name),			\
>  	.access	= access_id_reg,		\
>  	.get_user = get_id_reg,			\
>  	.set_user = set_id_reg,			\
> +	.check_user = ID_CHECKER(name),		\

This patch makes it clear that continuing to use ID_SANITISED() for all ID
registers makes no sense.

>  }
>  
>  /*
> @@ -1512,7 +1933,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	ID_SANITISED(ID_AA64PFR1_EL1),
>  	ID_UNALLOCATED(4,2),
>  	ID_UNALLOCATED(4,3),
> -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
> +	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1,
> +	  .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1,
> +	  .check_user = __general_id_checker, .visibility = sve_id_visibility },
>  	ID_UNALLOCATED(4,5),
>  	ID_UNALLOCATED(4,6),
>  	ID_UNALLOCATED(4,7),
> -- 
> 2.26.2
>

Thanks,
drew 

Re: [RFC v2 5/7] kvm: arm64: implement check_user for ID registers

[Peng Liang]

On 9/18/2020 3:46 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:00:59PM +0800, Peng Liang wrote:
>> For most ID registers, only neeed to check each field defined by user
>> space is no more than that in host and only the fields we want to
>> exposed to guest is set.  For some ID registers, the relationship
>> between some fields need to be check or we'd better to keep the same
>> value as host for some fields.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 425 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 424 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 86ebb8093c3c..a642ecfebe0a 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1385,12 +1385,433 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>>  	return true;
>>  }
>>  
>> +#define ID_CHECKER(reg) __check_ ##reg
>> +
>> +static int __general_id_checker(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +
>> +	return check_features(reg_id, val);
>> +}
>> +
>> +static int ID_CHECKER(ID_PFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_PFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_PFR2_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_DFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val, host_val;
>> +	u64 mask = ((0xfUL << ID_DFR0_PERFMON_SHIFT) |
>> +		    (0xfUL << ID_DFR0_MMAPDBG_SHIFT) |
>> +		    (0xfUL << ID_DFR0_COPDBG_SHIFT) |
>> +		    (0xfUL << ID_DFR0_COPDBG_SHIFT));
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	host_val = read_sanitised_ftr_reg(reg_id);
>> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR2_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR3_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR4_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR5_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR2_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR3_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR4_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR5_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR6_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(MVFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
> 
> There has to be a better way to handle all these redundant functions...
> 
>> +
>> +static int ID_CHECKER(MVFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val;
>> +	unsigned int fphp, simdhp;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	fphp = cpuid_feature_extract_signed_field(val, MVFR1_FPHP_SHIFT);
>> +	simdhp = cpuid_feature_extract_signed_field(val, MVFR1_SIMDHP_SHIFT);
>> +	return ((fphp == 0 && simdhp == 0) || (fphp == 2 && simdhp == 1) ||
>> +		(fphp == 3 && simdhp == 2)) ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(MVFR2_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64PFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val;
>> +	unsigned int fp, asimd;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT);
>> +	asimd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT);
>> +	return fp == asimd ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64PFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64DFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val, host_val;
>> +	u64 mask = ((0xfUL << ID_AA64DFR0_PMUVER_SHIFT) |
>> +		    (0xfUL << ID_AA64DFR0_DEBUGVER_SHIFT) |
>> +		    (0xfUL << ID_AA64DFR0_CTX_CMPS_SHIFT) |
>> +		    (0xfUL << ID_AA64DFR0_WRPS_SHIFT) |
>> +		    (0xfUL << ID_AA64DFR0_BRPS_SHIFT));
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	host_val = read_sanitised_ftr_reg(reg_id);
>> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64DFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64ISAR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val;
>> +	unsigned int sm3, sm4, sha1, sha2, sha3;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	sm3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM3_SHIFT);
>> +	sm4 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM4_SHIFT);
>> +	/*
>> +	 * ID_AA64ISAR0_EL1.SM3 and ID_AA64ISAR0_EL1.SM4 must have the same
>> +	 * value.
>> +	 */
>> +	if (sm3 != sm4)
>> +		return -EINVAL;
>> +
>> +	sha1 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA1_SHIFT);
>> +	sha2 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA2_SHIFT);
>> +	sha3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA3_SHIFT);
>> +	/*
>> +	 * 1. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
>> +	 *    ID_AA64ISAR0_EL1.SHA2 must have the value 0, and vice versa;
>> +	 * 2. If the value of ID_AA64ISAR0_EL1.SHA2 is 2, then
>> +	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 1, and vice versa;
>> +	 * 3. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
>> +	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 0;
>> +	 */
>> +	if ((sha1 ^ sha2) || ((sha2 == 2) ^ (sha3 == 1)) || (!sha1 && sha3))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64ISAR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val;
>> +	unsigned int gpi, gpa, api, apa;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	gpi = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPI_SHIFT);
>> +	gpa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPA_SHIFT);
>> +	api = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_API_SHIFT);
>> +	apa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_APA_SHIFT);
>> +	/*
>> +	 * 1. If the value of ID_AA64ISAR1_EL1.GPA is non-zero, then
>> +	 *    ID_AA64ISAR1_EL1.GPI must have the value 0;
>> +	 * 2. If the value of ID_AA64ISAR1_EL1.GPI is non-zero, then
>> +	 *    ID_AA64ISAR1_EL1.GPA must have the value 0;
>> +	 * 3. If the value of ID_AA64ISAR1_EL1.APA is non-zero, then
>> +	 *    ID_AA64ISAR1_EL1.API must have the value 0;
>> +	 * 4. If the value of ID_AA64ISAR1_EL1.API is non-zero, then
>> +	 *    ID_AA64ISAR1_EL1.APA must have the value 0;
>> +	 */
>> +	if ((gpi && gpa) || (api && apa))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64MMFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val, host_val;
>> +	u64 mask = ((0xfUL << ID_AA64MMFR0_TGRAN4_2_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_TGRAN64_2_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_TGRAN16_2_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_TGRAN4_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_TGRAN64_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_TGRAN16_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_ASID_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_PARANGE_SHIFT));
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	host_val = read_sanitised_ftr_reg(reg_id);
>> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64MMFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val, host_val;
>> +	unsigned int vmidbits, host_vmidbits;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	vmidbits = cpuid_feature_extract_unsigned_field(val, ID_AA64MMFR1_VMIDBITS_SHIFT);
>> +	host_val = read_sanitised_ftr_reg(reg_id);
>> +	host_vmidbits = cpuid_feature_extract_signed_field(host_val, ID_AA64MMFR1_VMIDBITS_SHIFT);
>> +	return vmidbits == host_vmidbits ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64MMFR2_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>>  /* sys_reg_desc initialiser for known cpufeature ID registers */
>>  #define ID_SANITISED(name) {			\
>>  	SYS_DESC(SYS_##name),			\
>>  	.access	= access_id_reg,		\
>>  	.get_user = get_id_reg,			\
>>  	.set_user = set_id_reg,			\
>> +	.check_user = ID_CHECKER(name),		\
> 
> This patch makes it clear that continuing to use ID_SANITISED() for all ID
> registers makes no sense.
> 
>>  }
>>  
>>  /*
>> @@ -1512,7 +1933,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>  	ID_SANITISED(ID_AA64PFR1_EL1),
>>  	ID_UNALLOCATED(4,2),
>>  	ID_UNALLOCATED(4,3),
>> -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
>> +	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1,
>> +	  .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1,
>> +	  .check_user = __general_id_checker, .visibility = sve_id_visibility },
>>  	ID_UNALLOCATED(4,5),
>>  	ID_UNALLOCATED(4,6),
>>  	ID_UNALLOCATED(4,7),
>> -- 
>> 2.26.2
>>
> 
> Thanks,
> drew 
> 
> .
> 

I also think this patch is a little ugly.  I will refactor the code to
remove the sharing set_user of ID registers in next version.

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

Re: [RFC v2 5/7] kvm: arm64: implement check_user for ID registers

[Peng Liang]

On 9/18/2020 3:46 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:00:59PM +0800, Peng Liang wrote:
>> For most ID registers, only neeed to check each field defined by user
>> space is no more than that in host and only the fields we want to
>> exposed to guest is set.  For some ID registers, the relationship
>> between some fields need to be check or we'd better to keep the same
>> value as host for some fields.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 425 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 424 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 86ebb8093c3c..a642ecfebe0a 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1385,12 +1385,433 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>>  	return true;
>>  }
>>  
>> +#define ID_CHECKER(reg) __check_ ##reg
>> +
>> +static int __general_id_checker(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +
>> +	return check_features(reg_id, val);
>> +}
>> +
>> +static int ID_CHECKER(ID_PFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_PFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_PFR2_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_DFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val, host_val;
>> +	u64 mask = ((0xfUL << ID_DFR0_PERFMON_SHIFT) |
>> +		    (0xfUL << ID_DFR0_MMAPDBG_SHIFT) |
>> +		    (0xfUL << ID_DFR0_COPDBG_SHIFT) |
>> +		    (0xfUL << ID_DFR0_COPDBG_SHIFT));
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	host_val = read_sanitised_ftr_reg(reg_id);
>> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR2_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR3_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR4_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_MMFR5_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR2_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR3_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR4_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR5_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_ISAR6_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(MVFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
> 
> There has to be a better way to handle all these redundant functions...
> 
>> +
>> +static int ID_CHECKER(MVFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val;
>> +	unsigned int fphp, simdhp;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	fphp = cpuid_feature_extract_signed_field(val, MVFR1_FPHP_SHIFT);
>> +	simdhp = cpuid_feature_extract_signed_field(val, MVFR1_SIMDHP_SHIFT);
>> +	return ((fphp == 0 && simdhp == 0) || (fphp == 2 && simdhp == 1) ||
>> +		(fphp == 3 && simdhp == 2)) ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(MVFR2_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64PFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val;
>> +	unsigned int fp, asimd;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT);
>> +	asimd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT);
>> +	return fp == asimd ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64PFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64DFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val, host_val;
>> +	u64 mask = ((0xfUL << ID_AA64DFR0_PMUVER_SHIFT) |
>> +		    (0xfUL << ID_AA64DFR0_DEBUGVER_SHIFT) |
>> +		    (0xfUL << ID_AA64DFR0_CTX_CMPS_SHIFT) |
>> +		    (0xfUL << ID_AA64DFR0_WRPS_SHIFT) |
>> +		    (0xfUL << ID_AA64DFR0_BRPS_SHIFT));
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	host_val = read_sanitised_ftr_reg(reg_id);
>> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64DFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64ISAR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val;
>> +	unsigned int sm3, sm4, sha1, sha2, sha3;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	sm3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM3_SHIFT);
>> +	sm4 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SM4_SHIFT);
>> +	/*
>> +	 * ID_AA64ISAR0_EL1.SM3 and ID_AA64ISAR0_EL1.SM4 must have the same
>> +	 * value.
>> +	 */
>> +	if (sm3 != sm4)
>> +		return -EINVAL;
>> +
>> +	sha1 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA1_SHIFT);
>> +	sha2 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA2_SHIFT);
>> +	sha3 = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR0_SHA3_SHIFT);
>> +	/*
>> +	 * 1. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
>> +	 *    ID_AA64ISAR0_EL1.SHA2 must have the value 0, and vice versa;
>> +	 * 2. If the value of ID_AA64ISAR0_EL1.SHA2 is 2, then
>> +	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 1, and vice versa;
>> +	 * 3. If the value of ID_AA64ISAR0_EL1.SHA1 is 0, then
>> +	 *    ID_AA64ISAR0_EL1.SHA3 must have the value 0;
>> +	 */
>> +	if ((sha1 ^ sha2) || ((sha2 == 2) ^ (sha3 == 1)) || (!sha1 && sha3))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64ISAR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val;
>> +	unsigned int gpi, gpa, api, apa;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	gpi = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPI_SHIFT);
>> +	gpa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_GPA_SHIFT);
>> +	api = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_API_SHIFT);
>> +	apa = cpuid_feature_extract_unsigned_field(val, ID_AA64ISAR1_APA_SHIFT);
>> +	/*
>> +	 * 1. If the value of ID_AA64ISAR1_EL1.GPA is non-zero, then
>> +	 *    ID_AA64ISAR1_EL1.GPI must have the value 0;
>> +	 * 2. If the value of ID_AA64ISAR1_EL1.GPI is non-zero, then
>> +	 *    ID_AA64ISAR1_EL1.GPA must have the value 0;
>> +	 * 3. If the value of ID_AA64ISAR1_EL1.APA is non-zero, then
>> +	 *    ID_AA64ISAR1_EL1.API must have the value 0;
>> +	 * 4. If the value of ID_AA64ISAR1_EL1.API is non-zero, then
>> +	 *    ID_AA64ISAR1_EL1.APA must have the value 0;
>> +	 */
>> +	if ((gpi && gpa) || (api && apa))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64MMFR0_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val, host_val;
>> +	u64 mask = ((0xfUL << ID_AA64MMFR0_TGRAN4_2_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_TGRAN64_2_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_TGRAN16_2_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_TGRAN4_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_TGRAN64_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_TGRAN16_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_ASID_SHIFT) |
>> +		    (0xfUL << ID_AA64MMFR0_PARANGE_SHIFT));
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	host_val = read_sanitised_ftr_reg(reg_id);
>> +	return (val & mask) == (host_val & mask) ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64MMFR1_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +			     (u32)rd->CRm, (u32)rd->Op2);
>> +	int err;
>> +	u64 val, host_val;
>> +	unsigned int vmidbits, host_vmidbits;
>> +
>> +	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
>> +	if (err)
>> +		return err;
>> +	err = check_features(reg_id, val);
>> +	if (err)
>> +		return err;
>> +
>> +	vmidbits = cpuid_feature_extract_unsigned_field(val, ID_AA64MMFR1_VMIDBITS_SHIFT);
>> +	host_val = read_sanitised_ftr_reg(reg_id);
>> +	host_vmidbits = cpuid_feature_extract_signed_field(host_val, ID_AA64MMFR1_VMIDBITS_SHIFT);
>> +	return vmidbits == host_vmidbits ? 0 : -EINVAL;
>> +}
>> +
>> +static int ID_CHECKER(ID_AA64MMFR2_EL1)(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_desc *rd,
>> +				const struct kvm_one_reg *reg,
>> +				void __user *uaddr)
>> +{
>> +	return __general_id_checker(vcpu, rd, reg, uaddr);
>> +}
>> +
>>  /* sys_reg_desc initialiser for known cpufeature ID registers */
>>  #define ID_SANITISED(name) {			\
>>  	SYS_DESC(SYS_##name),			\
>>  	.access	= access_id_reg,		\
>>  	.get_user = get_id_reg,			\
>>  	.set_user = set_id_reg,			\
>> +	.check_user = ID_CHECKER(name),		\
> 
> This patch makes it clear that continuing to use ID_SANITISED() for all ID
> registers makes no sense.
> 
>>  }
>>  
>>  /*
>> @@ -1512,7 +1933,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>  	ID_SANITISED(ID_AA64PFR1_EL1),
>>  	ID_UNALLOCATED(4,2),
>>  	ID_UNALLOCATED(4,3),
>> -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
>> +	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1,
>> +	  .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1,
>> +	  .check_user = __general_id_checker, .visibility = sve_id_visibility },
>>  	ID_UNALLOCATED(4,5),
>>  	ID_UNALLOCATED(4,6),
>>  	ID_UNALLOCATED(4,7),
>> -- 
>> 2.26.2
>>
> 
> Thanks,
> drew 
> 
> .
> 

I also think this patch is a little ugly.  I will refactor the code to
remove the sharing set_user of ID registers in next version.

Thanks,
Peng

[RFC v2 6/7] kvm: arm64: make ID registers configurable

[Peng Liang]

It's time to make ID registers configurable.  When userspace (but not
guest) want to set the values of ID registers, save the value in
sysreg file so that guest can read the modified values.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/kvm/sys_regs.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index a642ecfebe0a..881b66494524 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1263,10 +1263,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
 
 /*
  * cpufeature ID register user accessors
- *
- * For now, these registers are immutable for userspace, so no values
- * are stored, and for set_id_reg() we don't allow the effective value
- * to be changed.
  */
 static int __get_id_reg(struct kvm_vcpu *vcpu,
 			const struct sys_reg_desc *rd, void __user *uaddr,
@@ -1290,9 +1286,15 @@ static int __set_id_reg(struct kvm_vcpu *vcpu,
 	if (err)
 		return err;
 
-	/* This is what we mean by invariant: you can't change it. */
-	if (val != read_id_reg(vcpu, rd, raz))
-		return -EINVAL;
+	if (raz) {
+		if (val != read_id_reg(vcpu, rd, raz))
+			return -EINVAL;
+	} else {
+		u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+				     (u32)rd->CRm, (u32)rd->Op2);
+		/* val should be checked in check_user */
+		__vcpu_sys_reg(vcpu, ID_REG_INDEX(reg_id)) = val;
+	}
 
 	return 0;
 }
-- 
2.26.2

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

[RFC v2 6/7] kvm: arm64: make ID registers configurable

[Peng Liang]

It's time to make ID registers configurable.  When userspace (but not
guest) want to set the values of ID registers, save the value in
sysreg file so that guest can read the modified values.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/kvm/sys_regs.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index a642ecfebe0a..881b66494524 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1263,10 +1263,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
 
 /*
  * cpufeature ID register user accessors
- *
- * For now, these registers are immutable for userspace, so no values
- * are stored, and for set_id_reg() we don't allow the effective value
- * to be changed.
  */
 static int __get_id_reg(struct kvm_vcpu *vcpu,
 			const struct sys_reg_desc *rd, void __user *uaddr,
@@ -1290,9 +1286,15 @@ static int __set_id_reg(struct kvm_vcpu *vcpu,
 	if (err)
 		return err;
 
-	/* This is what we mean by invariant: you can't change it. */
-	if (val != read_id_reg(vcpu, rd, raz))
-		return -EINVAL;
+	if (raz) {
+		if (val != read_id_reg(vcpu, rd, raz))
+			return -EINVAL;
+	} else {
+		u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+				     (u32)rd->CRm, (u32)rd->Op2);
+		/* val should be checked in check_user */
+		__vcpu_sys_reg(vcpu, ID_REG_INDEX(reg_id)) = val;
+	}
 
 	return 0;
 }
-- 
2.26.2

Re: [RFC v2 6/7] kvm: arm64: make ID registers configurable

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:01:00PM +0800, Peng Liang wrote:
> It's time to make ID registers configurable.  When userspace (but not
> guest) want to set the values of ID registers, save the value in
> sysreg file so that guest can read the modified values.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index a642ecfebe0a..881b66494524 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1263,10 +1263,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>  
>  /*
>   * cpufeature ID register user accessors
> - *
> - * For now, these registers are immutable for userspace, so no values
> - * are stored, and for set_id_reg() we don't allow the effective value
> - * to be changed.
>   */
>  static int __get_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
> @@ -1290,9 +1286,15 @@ static int __set_id_reg(struct kvm_vcpu *vcpu,
>  	if (err)
>  		return err;
>  
> -	/* This is what we mean by invariant: you can't change it. */
> -	if (val != read_id_reg(vcpu, rd, raz))
> -		return -EINVAL;
> +	if (raz) {
> +		if (val != read_id_reg(vcpu, rd, raz))

val != 0 ?

> +			return -EINVAL;
> +	} else {
> +		u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +				     (u32)rd->CRm, (u32)rd->Op2);
> +		/* val should be checked in check_user */

It really doesn't make sense to share this trivial set_user function and
have different check functions. Just don't share the set_user function.

> +		__vcpu_sys_reg(vcpu, ID_REG_INDEX(reg_id)) = val;
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.26.2
> 

Thanks,
drew

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

Re: [RFC v2 6/7] kvm: arm64: make ID registers configurable

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:01:00PM +0800, Peng Liang wrote:
> It's time to make ID registers configurable.  When userspace (but not
> guest) want to set the values of ID registers, save the value in
> sysreg file so that guest can read the modified values.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index a642ecfebe0a..881b66494524 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1263,10 +1263,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>  
>  /*
>   * cpufeature ID register user accessors
> - *
> - * For now, these registers are immutable for userspace, so no values
> - * are stored, and for set_id_reg() we don't allow the effective value
> - * to be changed.
>   */
>  static int __get_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
> @@ -1290,9 +1286,15 @@ static int __set_id_reg(struct kvm_vcpu *vcpu,
>  	if (err)
>  		return err;
>  
> -	/* This is what we mean by invariant: you can't change it. */
> -	if (val != read_id_reg(vcpu, rd, raz))
> -		return -EINVAL;
> +	if (raz) {
> +		if (val != read_id_reg(vcpu, rd, raz))

val != 0 ?

> +			return -EINVAL;
> +	} else {
> +		u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +				     (u32)rd->CRm, (u32)rd->Op2);
> +		/* val should be checked in check_user */

It really doesn't make sense to share this trivial set_user function and
have different check functions. Just don't share the set_user function.

> +		__vcpu_sys_reg(vcpu, ID_REG_INDEX(reg_id)) = val;
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.26.2
> 

Thanks,
drew

Re: [RFC v2 6/7] kvm: arm64: make ID registers configurable

[Peng Liang]

On 9/18/2020 3:50 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:01:00PM +0800, Peng Liang wrote:
>> It's time to make ID registers configurable.  When userspace (but not
>> guest) want to set the values of ID registers, save the value in
>> sysreg file so that guest can read the modified values.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index a642ecfebe0a..881b66494524 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1263,10 +1263,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>>  
>>  /*
>>   * cpufeature ID register user accessors
>> - *
>> - * For now, these registers are immutable for userspace, so no values
>> - * are stored, and for set_id_reg() we don't allow the effective value
>> - * to be changed.
>>   */
>>  static int __get_id_reg(struct kvm_vcpu *vcpu,
>>  			const struct sys_reg_desc *rd, void __user *uaddr,
>> @@ -1290,9 +1286,15 @@ static int __set_id_reg(struct kvm_vcpu *vcpu,
>>  	if (err)
>>  		return err;
>>  
>> -	/* This is what we mean by invariant: you can't change it. */
>> -	if (val != read_id_reg(vcpu, rd, raz))
>> -		return -EINVAL;
>> +	if (raz) {
>> +		if (val != read_id_reg(vcpu, rd, raz))
> 
> val != 0 ?

Thanks for you advise.  It will make it much clearer.  I'll change it in next
version.

Thanks,
Peng

> 
>> +			return -EINVAL;
>> +	} else {
>> +		u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +				     (u32)rd->CRm, (u32)rd->Op2);
>> +		/* val should be checked in check_user */
> 
> It really doesn't make sense to share this trivial set_user function and
> have different check functions. Just don't share the set_user function.
> 
>> +		__vcpu_sys_reg(vcpu, ID_REG_INDEX(reg_id)) = val;
>> +	}
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.26.2
>>
> 
> Thanks,
> drew
> 
> .
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC v2 6/7] kvm: arm64: make ID registers configurable

[Peng Liang]

On 9/18/2020 3:50 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:01:00PM +0800, Peng Liang wrote:
>> It's time to make ID registers configurable.  When userspace (but not
>> guest) want to set the values of ID registers, save the value in
>> sysreg file so that guest can read the modified values.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index a642ecfebe0a..881b66494524 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1263,10 +1263,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>>  
>>  /*
>>   * cpufeature ID register user accessors
>> - *
>> - * For now, these registers are immutable for userspace, so no values
>> - * are stored, and for set_id_reg() we don't allow the effective value
>> - * to be changed.
>>   */
>>  static int __get_id_reg(struct kvm_vcpu *vcpu,
>>  			const struct sys_reg_desc *rd, void __user *uaddr,
>> @@ -1290,9 +1286,15 @@ static int __set_id_reg(struct kvm_vcpu *vcpu,
>>  	if (err)
>>  		return err;
>>  
>> -	/* This is what we mean by invariant: you can't change it. */
>> -	if (val != read_id_reg(vcpu, rd, raz))
>> -		return -EINVAL;
>> +	if (raz) {
>> +		if (val != read_id_reg(vcpu, rd, raz))
> 
> val != 0 ?

Thanks for you advise.  It will make it much clearer.  I'll change it in next
version.

Thanks,
Peng

> 
>> +			return -EINVAL;
>> +	} else {
>> +		u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +				     (u32)rd->CRm, (u32)rd->Op2);
>> +		/* val should be checked in check_user */
> 
> It really doesn't make sense to share this trivial set_user function and
> have different check functions. Just don't share the set_user function.
> 
>> +		__vcpu_sys_reg(vcpu, ID_REG_INDEX(reg_id)) = val;
>> +	}
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.26.2
>>
> 
> Thanks,
> drew
> 
> .
>

[RFC v2 7/7] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

[Peng Liang]

Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM
supports to set CPU features in AArch64.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 Documentation/virt/kvm/api.rst | 8 ++++++++
 arch/arm64/kvm/arm.c           | 1 +
 include/uapi/linux/kvm.h       | 1 +
 3 files changed, 10 insertions(+)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index d2b733dc7892..50214ed8f50e 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -6173,3 +6173,11 @@ specific interfaces must be consistent, i.e. if one says the feature
 is supported, than the other should as well and vice versa.  For arm64
 see Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL".
 For x86 see Documentation/virt/kvm/msr.rst "MSR_KVM_STEAL_TIME".
+
+8.25 KVM_CAP_ARM_CPU_FEATURE
+-----------------------------------
+
+:Architecture: arm64
+
+This capability indicates that userspace can modify the ID registers via
+KVM_SET_ONE_REG ioctl.
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 6d961e192268..918a7a56b224 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -178,6 +178,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
 	case KVM_CAP_ARM_NISV_TO_USER:
 	case KVM_CAP_ARM_INJECT_EXT_DABT:
+	case KVM_CAP_ARM_CPU_FEATURE:
 		r = 1;
 		break;
 	case KVM_CAP_ARM_SET_DEVICE_ADDR:
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 7d8eced6f459..12356beadd5a 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1037,6 +1037,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_SMALLER_MAXPHYADDR 185
 #define KVM_CAP_S390_DIAG318 186
 #define KVM_CAP_STEAL_TIME 187
+#define KVM_CAP_ARM_CPU_FEATURE 188
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.26.2

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

[RFC v2 7/7] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

[Peng Liang]

Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM
supports to set CPU features in AArch64.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 Documentation/virt/kvm/api.rst | 8 ++++++++
 arch/arm64/kvm/arm.c           | 1 +
 include/uapi/linux/kvm.h       | 1 +
 3 files changed, 10 insertions(+)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index d2b733dc7892..50214ed8f50e 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -6173,3 +6173,11 @@ specific interfaces must be consistent, i.e. if one says the feature
 is supported, than the other should as well and vice versa.  For arm64
 see Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL".
 For x86 see Documentation/virt/kvm/msr.rst "MSR_KVM_STEAL_TIME".
+
+8.25 KVM_CAP_ARM_CPU_FEATURE
+-----------------------------------
+
+:Architecture: arm64
+
+This capability indicates that userspace can modify the ID registers via
+KVM_SET_ONE_REG ioctl.
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 6d961e192268..918a7a56b224 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -178,6 +178,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
 	case KVM_CAP_ARM_NISV_TO_USER:
 	case KVM_CAP_ARM_INJECT_EXT_DABT:
+	case KVM_CAP_ARM_CPU_FEATURE:
 		r = 1;
 		break;
 	case KVM_CAP_ARM_SET_DEVICE_ADDR:
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 7d8eced6f459..12356beadd5a 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1037,6 +1037,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_SMALLER_MAXPHYADDR 185
 #define KVM_CAP_S390_DIAG318 186
 #define KVM_CAP_STEAL_TIME 187
+#define KVM_CAP_ARM_CPU_FEATURE 188
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.26.2

Re: [RFC v2 7/7] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:01:01PM +0800, Peng Liang wrote:
> Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM
> supports to set CPU features in AArch64.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  Documentation/virt/kvm/api.rst | 8 ++++++++
>  arch/arm64/kvm/arm.c           | 1 +
>  include/uapi/linux/kvm.h       | 1 +
>  3 files changed, 10 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index d2b733dc7892..50214ed8f50e 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6173,3 +6173,11 @@ specific interfaces must be consistent, i.e. if one says the feature
>  is supported, than the other should as well and vice versa.  For arm64
>  see Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL".
>  For x86 see Documentation/virt/kvm/msr.rst "MSR_KVM_STEAL_TIME".
> +
> +8.25 KVM_CAP_ARM_CPU_FEATURE
> +-----------------------------------

Too many '----'

> +
> +:Architecture: arm64
> +
> +This capability indicates that userspace can modify the ID registers via
> +KVM_SET_ONE_REG ioctl.

You should say something like "See KVM_SET_ONE_REG:ARM64 ID Registers"
here and also extend the "KVM_SET_ONE_REG" section with a "ARM64 ID
Register" section that describes the limits and return values.

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 6d961e192268..918a7a56b224 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -178,6 +178,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
>  	case KVM_CAP_ARM_NISV_TO_USER:
>  	case KVM_CAP_ARM_INJECT_EXT_DABT:
> +	case KVM_CAP_ARM_CPU_FEATURE:
>  		r = 1;
>  		break;
>  	case KVM_CAP_ARM_SET_DEVICE_ADDR:
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 7d8eced6f459..12356beadd5a 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1037,6 +1037,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_SMALLER_MAXPHYADDR 185
>  #define KVM_CAP_S390_DIAG318 186
>  #define KVM_CAP_STEAL_TIME 187
> +#define KVM_CAP_ARM_CPU_FEATURE 188
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> -- 
> 2.26.2
>

Thanks,
drew

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

Re: [RFC v2 7/7] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:01:01PM +0800, Peng Liang wrote:
> Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM
> supports to set CPU features in AArch64.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  Documentation/virt/kvm/api.rst | 8 ++++++++
>  arch/arm64/kvm/arm.c           | 1 +
>  include/uapi/linux/kvm.h       | 1 +
>  3 files changed, 10 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index d2b733dc7892..50214ed8f50e 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6173,3 +6173,11 @@ specific interfaces must be consistent, i.e. if one says the feature
>  is supported, than the other should as well and vice versa.  For arm64
>  see Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL".
>  For x86 see Documentation/virt/kvm/msr.rst "MSR_KVM_STEAL_TIME".
> +
> +8.25 KVM_CAP_ARM_CPU_FEATURE
> +-----------------------------------

Too many '----'

> +
> +:Architecture: arm64
> +
> +This capability indicates that userspace can modify the ID registers via
> +KVM_SET_ONE_REG ioctl.

You should say something like "See KVM_SET_ONE_REG:ARM64 ID Registers"
here and also extend the "KVM_SET_ONE_REG" section with a "ARM64 ID
Register" section that describes the limits and return values.

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 6d961e192268..918a7a56b224 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -178,6 +178,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
>  	case KVM_CAP_ARM_NISV_TO_USER:
>  	case KVM_CAP_ARM_INJECT_EXT_DABT:
> +	case KVM_CAP_ARM_CPU_FEATURE:
>  		r = 1;
>  		break;
>  	case KVM_CAP_ARM_SET_DEVICE_ADDR:
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 7d8eced6f459..12356beadd5a 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1037,6 +1037,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_SMALLER_MAXPHYADDR 185
>  #define KVM_CAP_S390_DIAG318 186
>  #define KVM_CAP_STEAL_TIME 187
> +#define KVM_CAP_ARM_CPU_FEATURE 188
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> -- 
> 2.26.2
>

Thanks,
drew

Re: [RFC v2 7/7] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

[Peng Liang]

On 9/18/2020 3:55 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:01:01PM +0800, Peng Liang wrote:
>> Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM
>> supports to set CPU features in AArch64.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  Documentation/virt/kvm/api.rst | 8 ++++++++
>>  arch/arm64/kvm/arm.c           | 1 +
>>  include/uapi/linux/kvm.h       | 1 +
>>  3 files changed, 10 insertions(+)
>>
>> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
>> index d2b733dc7892..50214ed8f50e 100644
>> --- a/Documentation/virt/kvm/api.rst
>> +++ b/Documentation/virt/kvm/api.rst
>> @@ -6173,3 +6173,11 @@ specific interfaces must be consistent, i.e. if one says the feature
>>  is supported, than the other should as well and vice versa.  For arm64
>>  see Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL".
>>  For x86 see Documentation/virt/kvm/msr.rst "MSR_KVM_STEAL_TIME".
>> +
>> +8.25 KVM_CAP_ARM_CPU_FEATURE
>> +-----------------------------------
> 
> Too many '----'
> 
>> +
>> +:Architecture: arm64
>> +
>> +This capability indicates that userspace can modify the ID registers via
>> +KVM_SET_ONE_REG ioctl.
> 
> You should say something like "See KVM_SET_ONE_REG:ARM64 ID Registers"
> here and also extend the "KVM_SET_ONE_REG" section with a "ARM64 ID
> Register" section that describes the limits and return values.
> 
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 6d961e192268..918a7a56b224 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -178,6 +178,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
>>  	case KVM_CAP_ARM_NISV_TO_USER:
>>  	case KVM_CAP_ARM_INJECT_EXT_DABT:
>> +	case KVM_CAP_ARM_CPU_FEATURE:
>>  		r = 1;
>>  		break;
>>  	case KVM_CAP_ARM_SET_DEVICE_ADDR:
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 7d8eced6f459..12356beadd5a 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1037,6 +1037,7 @@ struct kvm_ppc_resize_hpt {
>>  #define KVM_CAP_SMALLER_MAXPHYADDR 185
>>  #define KVM_CAP_S390_DIAG318 186
>>  #define KVM_CAP_STEAL_TIME 187
>> +#define KVM_CAP_ARM_CPU_FEATURE 188
>>  
>>  #ifdef KVM_CAP_IRQ_ROUTING
>>  
>> -- 
>> 2.26.2
>>
> 
> Thanks,
> drew
> 
> .
> 

Thank you for your advise.  I'll change them in next version.

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

Re: [RFC v2 7/7] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

[Peng Liang]

On 9/18/2020 3:55 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:01:01PM +0800, Peng Liang wrote:
>> Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM
>> supports to set CPU features in AArch64.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  Documentation/virt/kvm/api.rst | 8 ++++++++
>>  arch/arm64/kvm/arm.c           | 1 +
>>  include/uapi/linux/kvm.h       | 1 +
>>  3 files changed, 10 insertions(+)
>>
>> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
>> index d2b733dc7892..50214ed8f50e 100644
>> --- a/Documentation/virt/kvm/api.rst
>> +++ b/Documentation/virt/kvm/api.rst
>> @@ -6173,3 +6173,11 @@ specific interfaces must be consistent, i.e. if one says the feature
>>  is supported, than the other should as well and vice versa.  For arm64
>>  see Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL".
>>  For x86 see Documentation/virt/kvm/msr.rst "MSR_KVM_STEAL_TIME".
>> +
>> +8.25 KVM_CAP_ARM_CPU_FEATURE
>> +-----------------------------------
> 
> Too many '----'
> 
>> +
>> +:Architecture: arm64
>> +
>> +This capability indicates that userspace can modify the ID registers via
>> +KVM_SET_ONE_REG ioctl.
> 
> You should say something like "See KVM_SET_ONE_REG:ARM64 ID Registers"
> here and also extend the "KVM_SET_ONE_REG" section with a "ARM64 ID
> Register" section that describes the limits and return values.
> 
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 6d961e192268..918a7a56b224 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -178,6 +178,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
>>  	case KVM_CAP_ARM_NISV_TO_USER:
>>  	case KVM_CAP_ARM_INJECT_EXT_DABT:
>> +	case KVM_CAP_ARM_CPU_FEATURE:
>>  		r = 1;
>>  		break;
>>  	case KVM_CAP_ARM_SET_DEVICE_ADDR:
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 7d8eced6f459..12356beadd5a 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1037,6 +1037,7 @@ struct kvm_ppc_resize_hpt {
>>  #define KVM_CAP_SMALLER_MAXPHYADDR 185
>>  #define KVM_CAP_S390_DIAG318 186
>>  #define KVM_CAP_STEAL_TIME 187
>> +#define KVM_CAP_ARM_CPU_FEATURE 188
>>  
>>  #ifdef KVM_CAP_IRQ_ROUTING
>>  
>> -- 
>> 2.26.2
>>
> 
> Thanks,
> drew
> 
> .
> 

Thank you for your advise.  I'll change them in next version.

Thanks,
Peng

Re: [RFC v2 0/7] kvm: arm64: emulate ID registers

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:00:54PM +0800, Peng Liang wrote:
> In AArch64, guest will read the same values of the ID regsiters with
> host.  Both of them read the values from arm64_ftr_regs.  This patch
> series add support to emulate and configure ID registers so that we can
> control the value of ID registers that guest read.
> 
> v1 -> v2:
>  - save the ID registers in sysreg file instead of a new struct
>  - apply a checker before setting the value to the register
>  - add doc for new KVM_CAP_ARM_CPU_FEATURE
> 
> Peng Liang (7):
>   arm64: add a helper function to traverse arm64_ftr_regs
>   arm64: introduce check_features
>   kvm: arm64: save ID registers to sys_regs file
>   kvm: arm64: introduce check_user
>   kvm: arm64: implement check_user for ID registers
>   kvm: arm64: make ID registers configurable
>   kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension
> 
>  Documentation/virt/kvm/api.rst      |   8 +
>  arch/arm64/include/asm/cpufeature.h |   4 +
>  arch/arm64/include/asm/kvm_coproc.h |   2 +
>  arch/arm64/include/asm/kvm_host.h   |   3 +
>  arch/arm64/kernel/cpufeature.c      |  36 +++
>  arch/arm64/kvm/arm.c                |   3 +
>  arch/arm64/kvm/sys_regs.c           | 481 +++++++++++++++++++++++++++-
>  arch/arm64/kvm/sys_regs.h           |   6 +
>  include/uapi/linux/kvm.h            |   1 +
>  9 files changed, 532 insertions(+), 12 deletions(-)
> 
> -- 
> 2.26.2
>

Hi Peng,

I'd much rather see a series of patches where each patch converts a single
ID register from using ID_SANITISED() to having its own table entry, where
its own set_user() and reset() functions take into account its features
using high level arm64_ftr* functions. Any ID registers that can still
share code can certainly do so with some post-conversion refactoring.

Thanks,
drew

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

Re: [RFC v2 0/7] kvm: arm64: emulate ID registers

[Andrew Jones]

On Thu, Sep 17, 2020 at 08:00:54PM +0800, Peng Liang wrote:
> In AArch64, guest will read the same values of the ID regsiters with
> host.  Both of them read the values from arm64_ftr_regs.  This patch
> series add support to emulate and configure ID registers so that we can
> control the value of ID registers that guest read.
> 
> v1 -> v2:
>  - save the ID registers in sysreg file instead of a new struct
>  - apply a checker before setting the value to the register
>  - add doc for new KVM_CAP_ARM_CPU_FEATURE
> 
> Peng Liang (7):
>   arm64: add a helper function to traverse arm64_ftr_regs
>   arm64: introduce check_features
>   kvm: arm64: save ID registers to sys_regs file
>   kvm: arm64: introduce check_user
>   kvm: arm64: implement check_user for ID registers
>   kvm: arm64: make ID registers configurable
>   kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension
> 
>  Documentation/virt/kvm/api.rst      |   8 +
>  arch/arm64/include/asm/cpufeature.h |   4 +
>  arch/arm64/include/asm/kvm_coproc.h |   2 +
>  arch/arm64/include/asm/kvm_host.h   |   3 +
>  arch/arm64/kernel/cpufeature.c      |  36 +++
>  arch/arm64/kvm/arm.c                |   3 +
>  arch/arm64/kvm/sys_regs.c           | 481 +++++++++++++++++++++++++++-
>  arch/arm64/kvm/sys_regs.h           |   6 +
>  include/uapi/linux/kvm.h            |   1 +
>  9 files changed, 532 insertions(+), 12 deletions(-)
> 
> -- 
> 2.26.2
>

Hi Peng,

I'd much rather see a series of patches where each patch converts a single
ID register from using ID_SANITISED() to having its own table entry, where
its own set_user() and reset() functions take into account its features
using high level arm64_ftr* functions. Any ID registers that can still
share code can certainly do so with some post-conversion refactoring.

Thanks,
drew

Re: [RFC v2 0/7] kvm: arm64: emulate ID registers

[Peng Liang]

On 9/18/2020 4:01 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:00:54PM +0800, Peng Liang wrote:
>> In AArch64, guest will read the same values of the ID regsiters with
>> host.  Both of them read the values from arm64_ftr_regs.  This patch
>> series add support to emulate and configure ID registers so that we can
>> control the value of ID registers that guest read.
>>
>> v1 -> v2:
>>  - save the ID registers in sysreg file instead of a new struct
>>  - apply a checker before setting the value to the register
>>  - add doc for new KVM_CAP_ARM_CPU_FEATURE
>>
>> Peng Liang (7):
>>   arm64: add a helper function to traverse arm64_ftr_regs
>>   arm64: introduce check_features
>>   kvm: arm64: save ID registers to sys_regs file
>>   kvm: arm64: introduce check_user
>>   kvm: arm64: implement check_user for ID registers
>>   kvm: arm64: make ID registers configurable
>>   kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension
>>
>>  Documentation/virt/kvm/api.rst      |   8 +
>>  arch/arm64/include/asm/cpufeature.h |   4 +
>>  arch/arm64/include/asm/kvm_coproc.h |   2 +
>>  arch/arm64/include/asm/kvm_host.h   |   3 +
>>  arch/arm64/kernel/cpufeature.c      |  36 +++
>>  arch/arm64/kvm/arm.c                |   3 +
>>  arch/arm64/kvm/sys_regs.c           | 481 +++++++++++++++++++++++++++-
>>  arch/arm64/kvm/sys_regs.h           |   6 +
>>  include/uapi/linux/kvm.h            |   1 +
>>  9 files changed, 532 insertions(+), 12 deletions(-)
>>
>> -- 
>> 2.26.2
>>
> 
> Hi Peng,
> 
> I'd much rather see a series of patches where each patch converts a single
> ID register from using ID_SANITISED() to having its own table entry, where
> its own set_user() and reset() functions take into account its features
> using high level arm64_ftr* functions. Any ID registers that can still
> share code can certainly do so with some post-conversion refactoring.
> 
> Thanks,
> drew
> 
> .
> 
Hi Andrew,

Thank you for your advise.  I'll rework the code to use different set_user()
for different ID registers (maybe some general registers still use shareing
set_user()) and check the value defined by user space in its own set_user in
next version.

But do we need to implement reset() for ID registers?  I think ID registers
are read-only in guest and guest won't and can't change their values.  And
after 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on reset"),
we won't write junk to sysregs on reset.  So their values won't change on
reset?

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

Re: [RFC v2 0/7] kvm: arm64: emulate ID registers

[Peng Liang]

On 9/18/2020 4:01 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:00:54PM +0800, Peng Liang wrote:
>> In AArch64, guest will read the same values of the ID regsiters with
>> host.  Both of them read the values from arm64_ftr_regs.  This patch
>> series add support to emulate and configure ID registers so that we can
>> control the value of ID registers that guest read.
>>
>> v1 -> v2:
>>  - save the ID registers in sysreg file instead of a new struct
>>  - apply a checker before setting the value to the register
>>  - add doc for new KVM_CAP_ARM_CPU_FEATURE
>>
>> Peng Liang (7):
>>   arm64: add a helper function to traverse arm64_ftr_regs
>>   arm64: introduce check_features
>>   kvm: arm64: save ID registers to sys_regs file
>>   kvm: arm64: introduce check_user
>>   kvm: arm64: implement check_user for ID registers
>>   kvm: arm64: make ID registers configurable
>>   kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension
>>
>>  Documentation/virt/kvm/api.rst      |   8 +
>>  arch/arm64/include/asm/cpufeature.h |   4 +
>>  arch/arm64/include/asm/kvm_coproc.h |   2 +
>>  arch/arm64/include/asm/kvm_host.h   |   3 +
>>  arch/arm64/kernel/cpufeature.c      |  36 +++
>>  arch/arm64/kvm/arm.c                |   3 +
>>  arch/arm64/kvm/sys_regs.c           | 481 +++++++++++++++++++++++++++-
>>  arch/arm64/kvm/sys_regs.h           |   6 +
>>  include/uapi/linux/kvm.h            |   1 +
>>  9 files changed, 532 insertions(+), 12 deletions(-)
>>
>> -- 
>> 2.26.2
>>
> 
> Hi Peng,
> 
> I'd much rather see a series of patches where each patch converts a single
> ID register from using ID_SANITISED() to having its own table entry, where
> its own set_user() and reset() functions take into account its features
> using high level arm64_ftr* functions. Any ID registers that can still
> share code can certainly do so with some post-conversion refactoring.
> 
> Thanks,
> drew
> 
> .
> 
Hi Andrew,

Thank you for your advise.  I'll rework the code to use different set_user()
for different ID registers (maybe some general registers still use shareing
set_user()) and check the value defined by user space in its own set_user in
next version.

But do we need to implement reset() for ID registers?  I think ID registers
are read-only in guest and guest won't and can't change their values.  And
after 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on reset"),
we won't write junk to sysregs on reset.  So their values won't change on
reset?

Thanks,
Peng

Re: [RFC v2 0/7] kvm: arm64: emulate ID registers

[Andrew Jones]

On Fri, Sep 18, 2020 at 05:51:27PM +0800, Peng Liang wrote:
> On 9/18/2020 4:01 PM, Andrew Jones wrote:
> > On Thu, Sep 17, 2020 at 08:00:54PM +0800, Peng Liang wrote:
> >> In AArch64, guest will read the same values of the ID regsiters with
> >> host.  Both of them read the values from arm64_ftr_regs.  This patch
> >> series add support to emulate and configure ID registers so that we can
> >> control the value of ID registers that guest read.
> >>
> >> v1 -> v2:
> >>  - save the ID registers in sysreg file instead of a new struct
> >>  - apply a checker before setting the value to the register
> >>  - add doc for new KVM_CAP_ARM_CPU_FEATURE
> >>
> >> Peng Liang (7):
> >>   arm64: add a helper function to traverse arm64_ftr_regs
> >>   arm64: introduce check_features
> >>   kvm: arm64: save ID registers to sys_regs file
> >>   kvm: arm64: introduce check_user
> >>   kvm: arm64: implement check_user for ID registers
> >>   kvm: arm64: make ID registers configurable
> >>   kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension
> >>
> >>  Documentation/virt/kvm/api.rst      |   8 +
> >>  arch/arm64/include/asm/cpufeature.h |   4 +
> >>  arch/arm64/include/asm/kvm_coproc.h |   2 +
> >>  arch/arm64/include/asm/kvm_host.h   |   3 +
> >>  arch/arm64/kernel/cpufeature.c      |  36 +++
> >>  arch/arm64/kvm/arm.c                |   3 +
> >>  arch/arm64/kvm/sys_regs.c           | 481 +++++++++++++++++++++++++++-
> >>  arch/arm64/kvm/sys_regs.h           |   6 +
> >>  include/uapi/linux/kvm.h            |   1 +
> >>  9 files changed, 532 insertions(+), 12 deletions(-)
> >>
> >> -- 
> >> 2.26.2
> >>
> > 
> > Hi Peng,
> > 
> > I'd much rather see a series of patches where each patch converts a single
> > ID register from using ID_SANITISED() to having its own table entry, where
> > its own set_user() and reset() functions take into account its features
> > using high level arm64_ftr* functions. Any ID registers that can still
> > share code can certainly do so with some post-conversion refactoring.
> > 
> > Thanks,
> > drew
> > 
> > .
> > 
> Hi Andrew,
> 
> Thank you for your advise.  I'll rework the code to use different set_user()
> for different ID registers (maybe some general registers still use shareing
> set_user()) and check the value defined by user space in its own set_user in
> next version.
> 
> But do we need to implement reset() for ID registers?  I think ID registers
> are read-only in guest and guest won't and can't change their values.  And
> after 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on reset"),
> we won't write junk to sysregs on reset.  So their values won't change on
> reset?

If a new reset isn't needed, then that's fine. I just want each ID
register to have its requirements considered independently, rather
than trying to apply the same functions to all of them without any
apparent consideration.

Thanks,
drew

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

Re: [RFC v2 0/7] kvm: arm64: emulate ID registers

[Andrew Jones]

On Fri, Sep 18, 2020 at 05:51:27PM +0800, Peng Liang wrote:
> On 9/18/2020 4:01 PM, Andrew Jones wrote:
> > On Thu, Sep 17, 2020 at 08:00:54PM +0800, Peng Liang wrote:
> >> In AArch64, guest will read the same values of the ID regsiters with
> >> host.  Both of them read the values from arm64_ftr_regs.  This patch
> >> series add support to emulate and configure ID registers so that we can
> >> control the value of ID registers that guest read.
> >>
> >> v1 -> v2:
> >>  - save the ID registers in sysreg file instead of a new struct
> >>  - apply a checker before setting the value to the register
> >>  - add doc for new KVM_CAP_ARM_CPU_FEATURE
> >>
> >> Peng Liang (7):
> >>   arm64: add a helper function to traverse arm64_ftr_regs
> >>   arm64: introduce check_features
> >>   kvm: arm64: save ID registers to sys_regs file
> >>   kvm: arm64: introduce check_user
> >>   kvm: arm64: implement check_user for ID registers
> >>   kvm: arm64: make ID registers configurable
> >>   kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension
> >>
> >>  Documentation/virt/kvm/api.rst      |   8 +
> >>  arch/arm64/include/asm/cpufeature.h |   4 +
> >>  arch/arm64/include/asm/kvm_coproc.h |   2 +
> >>  arch/arm64/include/asm/kvm_host.h   |   3 +
> >>  arch/arm64/kernel/cpufeature.c      |  36 +++
> >>  arch/arm64/kvm/arm.c                |   3 +
> >>  arch/arm64/kvm/sys_regs.c           | 481 +++++++++++++++++++++++++++-
> >>  arch/arm64/kvm/sys_regs.h           |   6 +
> >>  include/uapi/linux/kvm.h            |   1 +
> >>  9 files changed, 532 insertions(+), 12 deletions(-)
> >>
> >> -- 
> >> 2.26.2
> >>
> > 
> > Hi Peng,
> > 
> > I'd much rather see a series of patches where each patch converts a single
> > ID register from using ID_SANITISED() to having its own table entry, where
> > its own set_user() and reset() functions take into account its features
> > using high level arm64_ftr* functions. Any ID registers that can still
> > share code can certainly do so with some post-conversion refactoring.
> > 
> > Thanks,
> > drew
> > 
> > .
> > 
> Hi Andrew,
> 
> Thank you for your advise.  I'll rework the code to use different set_user()
> for different ID registers (maybe some general registers still use shareing
> set_user()) and check the value defined by user space in its own set_user in
> next version.
> 
> But do we need to implement reset() for ID registers?  I think ID registers
> are read-only in guest and guest won't and can't change their values.  And
> after 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on reset"),
> we won't write junk to sysregs on reset.  So their values won't change on
> reset?

If a new reset isn't needed, then that's fine. I just want each ID
register to have its requirements considered independently, rather
than trying to apply the same functions to all of them without any
apparent consideration.

Thanks,
drew