In preparation for CTR_EL0 emulation maintain a per VM for this
register and use it where appropriate.
Signed-off-by: Sebastian Ott <[email protected]>
---
arch/arm64/include/asm/kvm_host.h | 2 ++
arch/arm64/kvm/sys_regs.c | 21 ++++++++++++++-------
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 212ae77eefaf..1259be5e2f3e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -331,6 +331,8 @@ struct kvm_arch {
#define KVM_ARM_ID_REG_NUM (IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
u64 id_regs[KVM_ARM_ID_REG_NUM];
+ u64 ctr_el0;
+
/* Masks for VNCR-baked sysregs */
struct kvm_sysreg_masks *sysreg_masks;
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 41741bf4d2b2..0213c96f73f2 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -219,9 +219,9 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
* Returns the minimum line size for the selected cache, expressed as
* Log2(bytes).
*/
-static u8 get_min_cache_line_size(bool icache)
+static u8 get_min_cache_line_size(struct kvm *kvm, bool icache)
{
- u64 ctr = read_sanitised_ftr_reg(SYS_CTR_EL0);
+ u64 ctr = kvm->arch.ctr_el0;
u8 field;
if (icache)
@@ -248,7 +248,7 @@ static u32 get_ccsidr(struct kvm_vcpu *vcpu, u32 csselr)
if (vcpu->arch.ccsidr)
return vcpu->arch.ccsidr[csselr];
- line_size = get_min_cache_line_size(csselr & CSSELR_EL1_InD);
+ line_size = get_min_cache_line_size(vcpu->kvm, csselr & CSSELR_EL1_InD);
/*
* Fabricate a CCSIDR value as the overriding value does not exist.
@@ -283,7 +283,7 @@ static int set_ccsidr(struct kvm_vcpu *vcpu, u32 csselr, u32 val)
u32 i;
if ((val & CCSIDR_EL1_RES0) ||
- line_size < get_min_cache_line_size(csselr & CSSELR_EL1_InD))
+ line_size < get_min_cache_line_size(vcpu->kvm, csselr & CSSELR_EL1_InD))
return -EINVAL;
if (!ccsidr) {
@@ -1886,7 +1886,7 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
if (p->is_write)
return write_to_read_only(vcpu, p, r);
- p->regval = read_sanitised_ftr_reg(SYS_CTR_EL0);
+ p->regval = vcpu->kvm->arch.ctr_el0;
return true;
}
@@ -1906,7 +1906,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
*/
static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
- u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
+ u64 ctr_el0 = vcpu->kvm->arch.ctr_el0;
u64 clidr;
u8 loc;
@@ -1959,8 +1959,8 @@ static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
u64 val)
{
- u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
u64 idc = !CLIDR_LOC(val) || (!CLIDR_LOUIS(val) && !CLIDR_LOUU(val));
+ u64 ctr_el0 = vcpu->kvm->arch.ctr_el0;
if ((val & CLIDR_EL1_RES0) || (!(ctr_el0 & CTR_EL0_IDC) && idc))
return -EINVAL;
@@ -3557,6 +3557,13 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
struct kvm *kvm = vcpu->kvm;
unsigned long i;
+ if (!kvm_vcpu_initialized(vcpu))
+ /*
+ * Make sure CTR_EL0 is initialized before registers
+ * that depend on it are reset.
+ */
+ kvm->arch.ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
+
for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
const struct sys_reg_desc *r = &sys_reg_descs[i];
--
2.42.0
Hi Sebastian,
On 5/14/24 15:22, Sebastian Ott wrote:
> In preparation for CTR_EL0 emulation maintain a per VM for this
s/per VM/per VM value/ would be better
> register and use it where appropriate.
>
> Signed-off-by: Sebastian Ott <[email protected]>
Reviewed-by: Shaoqin Huang <[email protected]>
Thanks,
Shaoqin
> ---
> arch/arm64/include/asm/kvm_host.h | 2 ++
> arch/arm64/kvm/sys_regs.c | 21 ++++++++++++++-------
> 2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 212ae77eefaf..1259be5e2f3e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -331,6 +331,8 @@ struct kvm_arch {
> #define KVM_ARM_ID_REG_NUM (IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
> u64 id_regs[KVM_ARM_ID_REG_NUM];
>
> + u64 ctr_el0;
> +
> /* Masks for VNCR-baked sysregs */
> struct kvm_sysreg_masks *sysreg_masks;
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 41741bf4d2b2..0213c96f73f2 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -219,9 +219,9 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
> * Returns the minimum line size for the selected cache, expressed as
> * Log2(bytes).
> */
> -static u8 get_min_cache_line_size(bool icache)
> +static u8 get_min_cache_line_size(struct kvm *kvm, bool icache)
> {
> - u64 ctr = read_sanitised_ftr_reg(SYS_CTR_EL0);
> + u64 ctr = kvm->arch.ctr_el0;
> u8 field;
>
> if (icache)
> @@ -248,7 +248,7 @@ static u32 get_ccsidr(struct kvm_vcpu *vcpu, u32 csselr)
> if (vcpu->arch.ccsidr)
> return vcpu->arch.ccsidr[csselr];
>
> - line_size = get_min_cache_line_size(csselr & CSSELR_EL1_InD);
> + line_size = get_min_cache_line_size(vcpu->kvm, csselr & CSSELR_EL1_InD);
>
> /*
> * Fabricate a CCSIDR value as the overriding value does not exist.
> @@ -283,7 +283,7 @@ static int set_ccsidr(struct kvm_vcpu *vcpu, u32 csselr, u32 val)
> u32 i;
>
> if ((val & CCSIDR_EL1_RES0) ||
> - line_size < get_min_cache_line_size(csselr & CSSELR_EL1_InD))
> + line_size < get_min_cache_line_size(vcpu->kvm, csselr & CSSELR_EL1_InD))
> return -EINVAL;
>
> if (!ccsidr) {
> @@ -1886,7 +1886,7 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (p->is_write)
> return write_to_read_only(vcpu, p, r);
>
> - p->regval = read_sanitised_ftr_reg(SYS_CTR_EL0);
> + p->regval = vcpu->kvm->arch.ctr_el0;
> return true;
> }
>
> @@ -1906,7 +1906,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> */
> static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> {
> - u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> + u64 ctr_el0 = vcpu->kvm->arch.ctr_el0;
> u64 clidr;
> u8 loc;
>
> @@ -1959,8 +1959,8 @@ static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> u64 val)
> {
> - u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> u64 idc = !CLIDR_LOC(val) || (!CLIDR_LOUIS(val) && !CLIDR_LOUU(val));
> + u64 ctr_el0 = vcpu->kvm->arch.ctr_el0;
>
> if ((val & CLIDR_EL1_RES0) || (!(ctr_el0 & CTR_EL0_IDC) && idc))
> return -EINVAL;
> @@ -3557,6 +3557,13 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
> struct kvm *kvm = vcpu->kvm;
> unsigned long i;
>
> + if (!kvm_vcpu_initialized(vcpu))
> + /*
> + * Make sure CTR_EL0 is initialized before registers
> + * that depend on it are reset.
> + */
> + kvm->arch.ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +
> for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
> const struct sys_reg_desc *r = &sys_reg_descs[i];
>
--
Shaoqin
Hi Sebastian,
On 5/14/24 09:22, Sebastian Ott wrote:
> In preparation for CTR_EL0 emulation maintain a per VM for this
s/a per VM for this register/a per VM shadow register for this latter?
> register and use it where appropriate.
>
> Signed-off-by: Sebastian Ott <[email protected]>
> ---
> arch/arm64/include/asm/kvm_host.h | 2 ++
> arch/arm64/kvm/sys_regs.c | 21 ++++++++++++++-------
> 2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 212ae77eefaf..1259be5e2f3e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -331,6 +331,8 @@ struct kvm_arch {
> #define KVM_ARM_ID_REG_NUM (IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
> u64 id_regs[KVM_ARM_ID_REG_NUM];
>
> + u64 ctr_el0;
> +
> /* Masks for VNCR-baked sysregs */
> struct kvm_sysreg_masks *sysreg_masks;
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 41741bf4d2b2..0213c96f73f2 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -219,9 +219,9 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
> * Returns the minimum line size for the selected cache, expressed as
> * Log2(bytes).
> */
> -static u8 get_min_cache_line_size(bool icache)
> +static u8 get_min_cache_line_size(struct kvm *kvm, bool icache)
> {
> - u64 ctr = read_sanitised_ftr_reg(SYS_CTR_EL0);
> + u64 ctr = kvm->arch.ctr_el0;
> u8 field;
>
> if (icache)
> @@ -248,7 +248,7 @@ static u32 get_ccsidr(struct kvm_vcpu *vcpu, u32 csselr)
> if (vcpu->arch.ccsidr)
> return vcpu->arch.ccsidr[csselr];
>
> - line_size = get_min_cache_line_size(csselr & CSSELR_EL1_InD);
> + line_size = get_min_cache_line_size(vcpu->kvm, csselr & CSSELR_EL1_InD);
>
> /*
> * Fabricate a CCSIDR value as the overriding value does not exist.
> @@ -283,7 +283,7 @@ static int set_ccsidr(struct kvm_vcpu *vcpu, u32 csselr, u32 val)
> u32 i;
>
> if ((val & CCSIDR_EL1_RES0) ||
> - line_size < get_min_cache_line_size(csselr & CSSELR_EL1_InD))
> + line_size < get_min_cache_line_size(vcpu->kvm, csselr & CSSELR_EL1_InD))
> return -EINVAL;
>
> if (!ccsidr) {
> @@ -1886,7 +1886,7 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (p->is_write)
> return write_to_read_only(vcpu, p, r);
>
> - p->regval = read_sanitised_ftr_reg(SYS_CTR_EL0);
> + p->regval = vcpu->kvm->arch.ctr_el0;
> return true;
> }
>
> @@ -1906,7 +1906,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> */
> static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> {
> - u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> + u64 ctr_el0 = vcpu->kvm->arch.ctr_el0;
> u64 clidr;
> u8 loc;
>
> @@ -1959,8 +1959,8 @@ static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> u64 val)
> {
> - u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> u64 idc = !CLIDR_LOC(val) || (!CLIDR_LOUIS(val) && !CLIDR_LOUU(val));
> + u64 ctr_el0 = vcpu->kvm->arch.ctr_el0;
>
> if ((val & CLIDR_EL1_RES0) || (!(ctr_el0 & CTR_EL0_IDC) && idc))
> return -EINVAL;
nit: you may update the function doc comment which the extra handling of
CTR_EL0.
> @@ -3557,6 +3557,13 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
> struct kvm *kvm = vcpu->kvm;
> unsigned long i;
>
> + if (!kvm_vcpu_initialized(vcpu))
at this stage of the reading, why is the above check needed?
> + /*
> + * Make sure CTR_EL0 is initialized before registers
> + * that depend on it are reset.
> + */
nit: the above comment explains why you need to set the shadow ctr_el0
reg before resetting other id_regs and is not associated to the case
where (!kvm_vcpu_initialized(vcpu)) is true - at least that's my
understanding -. So I would put the comment before the check and while
at it also explain why the check is needed.
> + kvm->arch.ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +
> for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
> const struct sys_reg_desc *r = &sys_reg_descs[i];
>
Thanks
Eric
Hej Eric,
On Wed, 29 May 2024, Eric Auger wrote:
>> static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> u64 val)
>> {
>> - u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
>> u64 idc = !CLIDR_LOC(val) || (!CLIDR_LOUIS(val) && !CLIDR_LOUU(val));
>> + u64 ctr_el0 = vcpu->kvm->arch.ctr_el0;
>>
>> if ((val & CLIDR_EL1_RES0) || (!(ctr_el0 & CTR_EL0_IDC) && idc))
>> return -EINVAL;
> nit: you may update the function doc comment which the extra handling of
> CTR_EL0.
Hm, there's no extra handling of CTR_EL0 it just uses the emulated value.
>> @@ -3557,6 +3557,13 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>> struct kvm *kvm = vcpu->kvm;
>> unsigned long i;
>>
>> + if (!kvm_vcpu_initialized(vcpu))
> at this stage of the reading, why is the above check needed?
To make sure that a later call to this function doesn't overwrite
the value provided by userspace. (See e016333745c "KVM: arm64: Only
reset vCPU-scoped feature ID regs once").
Sebastian
On 5/29/24 17:51, Sebastian Ott wrote:
> Hej Eric,
>
> On Wed, 29 May 2024, Eric Auger wrote:
>>> static int set_clidr(struct kvm_vcpu *vcpu, const struct
>>> sys_reg_desc *rd,
>>> u64 val)
>>> {
>>> - u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
>>> u64 idc = !CLIDR_LOC(val) || (!CLIDR_LOUIS(val) &&
>>> !CLIDR_LOUU(val));
>>> + u64 ctr_el0 = vcpu->kvm->arch.ctr_el0;
>>>
>>> if ((val & CLIDR_EL1_RES0) || (!(ctr_el0 & CTR_EL0_IDC) && idc))
>>> return -EINVAL;
>> nit: you may update the function doc comment which the extra handling of
>> CTR_EL0.
>
> Hm, there's no extra handling of CTR_EL0 it just uses the emulated value.>
>>> @@ -3557,6 +3557,13 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>>> struct kvm *kvm = vcpu->kvm;
>>> unsigned long i;
>>>
>>> + if (!kvm_vcpu_initialized(vcpu))
>> at this stage of the reading, why is the above check needed?
>
> To make sure that a later call to this function doesn't overwrite
> the value provided by userspace. (See e016333745c "KVM: arm64: Only
> reset vCPU-scoped feature ID regs once").
but isn't it overwritten through the .reset=reset_ctr() that is
populated in next patch?
Eric
>
> Sebastian
>
On Wed, 29 May 2024, Eric Auger wrote:
> On 5/29/24 17:51, Sebastian Ott wrote:
>> On Wed, 29 May 2024, Eric Auger wrote:
>>>> @@ -3557,6 +3557,13 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>>>> struct kvm *kvm = vcpu->kvm;
>>>> unsigned long i;
>>>>
>>>> + if (!kvm_vcpu_initialized(vcpu))
>>> at this stage of the reading, why is the above check needed?
>>
>> To make sure that a later call to this function doesn't overwrite
>> the value provided by userspace. (See e016333745c "KVM: arm64: Only
>> reset vCPU-scoped feature ID regs once").
> but isn't it overwritten through the .reset=reset_ctr() that is
> populated in next patch?
No, this is done via reset_vcpu_ftr_id_reg() and also guarded by
kvm_vcpu_initialized().
Sebastian
On 5/30/24 13:24, Sebastian Ott wrote:
> On Wed, 29 May 2024, Eric Auger wrote:
>> On 5/29/24 17:51, Sebastian Ott wrote:
>>> On Wed, 29 May 2024, Eric Auger wrote:
>>>>> @@ -3557,6 +3557,13 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>>>>> struct kvm *kvm = vcpu->kvm;
>>>>> unsigned long i;
>>>>>
>>>>> + if (!kvm_vcpu_initialized(vcpu))
>>>> at this stage of the reading, why is the above check needed?
>>>
>>> To make sure that a later call to this function doesn't overwrite
>>> the value provided by userspace. (See e016333745c "KVM: arm64: Only
>>> reset vCPU-scoped feature ID regs once").
>> but isn't it overwritten through the .reset=reset_ctr() that is
>> populated in next patch?
>
> No, this is done via reset_vcpu_ftr_id_reg() and also guarded by
> kvm_vcpu_initialized().
OK thanks.
Eric
>
> Sebastian