| Message ID | 20211117064359.2362060-3-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make CPU ID registers writable by userspace | expand |
Hi Reiji, On 11/17/21 7:43 AM, Reiji Watanabe wrote: > Extend sys_regs[] of kvm_cpu_context for ID registers and save ID > registers' sanitized value in the array for the vCPU at the first > vCPU reset. Use the saved ones when ID registers are read by > userspace (via KVM_GET_ONE_REG) or the guest. > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 10 +++++++ > arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ > 2 files changed, 37 insertions(+), 16 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index edbe2cb21947..72db73c79403 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { > u64 disr_el1; /* Deferred [SError] Status Register */ > }; > > +/* > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > + * where 0<=crm<8, 0<=op2<8. > + */ > +#define KVM_ARM_ID_REG_MAX_NUM 64 > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) > + > enum vcpu_sysreg { > __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ > MPIDR_EL1, /* MultiProcessor Affinity Register */ > @@ -210,6 +218,8 @@ enum vcpu_sysreg { > CNTP_CVAL_EL0, > CNTP_CTL_EL0, > > + ID_REG_BASE, > + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, > /* Memory Tagging Extension registers */ > RGSR_EL1, /* Random Allocation Tag Seed Register */ > GCR_EL1, /* Tag Control Register */ > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index e3ec1a44f94d..5608d3410660 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -33,6 +33,8 @@ > > #include "trace.h" > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); > + > /* > * All of this file is extremely similar to the ARM coproc.c, but the > * types are different. My gut feeling is that it should be pretty > @@ -273,7 +275,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, > struct sys_reg_params *p, > const struct sys_reg_desc *r) > { > - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); > + u64 val = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); > u32 sr = reg_to_encoding(r); > > if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { > @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > return true; > } > > -/* Read a sanitised cpufeature ID register by sys_reg_desc */ > -static u64 read_id_reg(const struct kvm_vcpu *vcpu, > - struct sys_reg_desc const *r, bool raz) > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > { > - u32 id = reg_to_encoding(r); > - u64 val; > - > - if (raz) > - return 0; > - > - val = read_sanitised_ftr_reg(id); > + u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > return val; > } > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, > + struct sys_reg_desc const *r, bool raz) > +{ > + u32 id = reg_to_encoding(r); > + > + return raz ? 0 : __read_id_reg(vcpu, id); > +} > + > static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > const struct sys_reg_desc *r) > { > @@ -1178,6 +1180,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, > return REG_HIDDEN; > } > > +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) > +{ > + u32 id = reg_to_encoding(rd); > + > + if (vcpu_has_reset_once(vcpu)) > + return; > + > + __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)) = read_sanitised_ftr_reg(id); > +} > + > static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, > const struct kvm_one_reg *reg, void __user *uaddr) > @@ -1223,9 +1235,7 @@ static int set_id_aa64pfr0_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. > + * We don't allow the effective value to be changed. This change may be moved to a subsequent patch as this patch does not change the behavior yet. > */ > static int __get_id_reg(const struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, void __user *uaddr, > @@ -1382,6 +1392,7 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu, > #define ID_SANITISED(name) { \ > SYS_DESC(SYS_##name), \ > .access = access_id_reg, \ > + .reset = reset_id_reg, \ > .get_user = get_id_reg, \ > .set_user = set_id_reg, \ > .visibility = id_visibility, \ > @@ -1837,8 +1848,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu, > if (p->is_write) { > return ignore_write(vcpu, p); > } else { > - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); > - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); > + u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1); > + u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1); > u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT); > > p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) | > Thanks Eric
Hi Eric, On Thu, Nov 18, 2021 at 12:37 PM Eric Auger <eauger@redhat.com> wrote: > > Hi Reiji, > > On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > Extend sys_regs[] of kvm_cpu_context for ID registers and save ID > > registers' sanitized value in the array for the vCPU at the first > > vCPU reset. Use the saved ones when ID registers are read by > > userspace (via KVM_GET_ONE_REG) or the guest. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 10 +++++++ > > arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ > > 2 files changed, 37 insertions(+), 16 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index edbe2cb21947..72db73c79403 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { > > u64 disr_el1; /* Deferred [SError] Status Register */ > > }; > > > > +/* > > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > > + * where 0<=crm<8, 0<=op2<8. > > + */ > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) > > + > > enum vcpu_sysreg { > > __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ > > MPIDR_EL1, /* MultiProcessor Affinity Register */ > > @@ -210,6 +218,8 @@ enum vcpu_sysreg { > > CNTP_CVAL_EL0, > > CNTP_CTL_EL0, > > > > + ID_REG_BASE, > > + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, > > /* Memory Tagging Extension registers */ > > RGSR_EL1, /* Random Allocation Tag Seed Register */ > > GCR_EL1, /* Tag Control Register */ > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index e3ec1a44f94d..5608d3410660 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -33,6 +33,8 @@ > > > > #include "trace.h" > > > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); > > + > > /* > > * All of this file is extremely similar to the ARM coproc.c, but the > > * types are different. My gut feeling is that it should be pretty > > @@ -273,7 +275,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, > > struct sys_reg_params *p, > > const struct sys_reg_desc *r) > > { > > - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); > > + u64 val = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); > > u32 sr = reg_to_encoding(r); > > > > if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { > > @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > > return true; > > } > > > > -/* Read a sanitised cpufeature ID register by sys_reg_desc */ > > -static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > - struct sys_reg_desc const *r, bool raz) > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > > { > > - u32 id = reg_to_encoding(r); > > - u64 val; > > - > > - if (raz) > > - return 0; > > - > > - val = read_sanitised_ftr_reg(id); > > + u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > > > > switch (id) { > > case SYS_ID_AA64PFR0_EL1: > > @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > return val; > > } > > > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > + struct sys_reg_desc const *r, bool raz) > > +{ > > + u32 id = reg_to_encoding(r); > > + > > + return raz ? 0 : __read_id_reg(vcpu, id); > > +} > > + > > static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *r) > > { > > @@ -1178,6 +1180,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, > > return REG_HIDDEN; > > } > > > > +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) > > +{ > > + u32 id = reg_to_encoding(rd); > > + > > + if (vcpu_has_reset_once(vcpu)) > > + return; > > + > > + __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)) = read_sanitised_ftr_reg(id); > > +} > > + > > static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *rd, > > const struct kvm_one_reg *reg, void __user *uaddr) > > @@ -1223,9 +1235,7 @@ static int set_id_aa64pfr0_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. > > + * We don't allow the effective value to be changed. > This change may be moved to a subsequent patch as this patch does not > change the behavior yet. Thank you for the review. There are three main parts in the original comments. (A) these registers are immutable for userspace (B) no values are stored (C) we don't allow the effective value to be changed This patch stores ID register values in sys_regs[]. So, I don't think (B) should be there, and I removed (B). Since (A) is essentially the same as (C), I removed (A) (and left (C)). Do you think it is better to leave (A) in this patch, too ? Thanks, Reiji > > */ > > static int __get_id_reg(const struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *rd, void __user *uaddr, > > @@ -1382,6 +1392,7 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu, > > #define ID_SANITISED(name) { \ > > SYS_DESC(SYS_##name), \ > > .access = access_id_reg, \ > > + .reset = reset_id_reg, \ > > .get_user = get_id_reg, \ > > .set_user = set_id_reg, \ > > .visibility = id_visibility, \ > > @@ -1837,8 +1848,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu, > > if (p->is_write) { > > return ignore_write(vcpu, p); > > } else { > > - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); > > - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); > > + u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1); > > + u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1); > > u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT); > > > > p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) | > > > Thanks > > Eric >
On Wed, 17 Nov 2021 06:43:32 +0000, Reiji Watanabe <reijiw@google.com> wrote: > > Extend sys_regs[] of kvm_cpu_context for ID registers and save ID > registers' sanitized value in the array for the vCPU at the first > vCPU reset. Use the saved ones when ID registers are read by > userspace (via KVM_GET_ONE_REG) or the guest. > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 10 +++++++ > arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ > 2 files changed, 37 insertions(+), 16 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index edbe2cb21947..72db73c79403 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { > u64 disr_el1; /* Deferred [SError] Status Register */ > }; > > +/* > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > + * where 0<=crm<8, 0<=op2<8. > + */ > +#define KVM_ARM_ID_REG_MAX_NUM 64 > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) > + > enum vcpu_sysreg { > __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ > MPIDR_EL1, /* MultiProcessor Affinity Register */ > @@ -210,6 +218,8 @@ enum vcpu_sysreg { > CNTP_CVAL_EL0, > CNTP_CTL_EL0, > > + ID_REG_BASE, > + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, It is rather unclear to me why we want these registers to be replicated on a per-CPU basis. Yes, this fits the architecture, but that's also a total waste of memory if you have more than a single CPU, because we make a point in only exposing homogeneous properties to the VM (I don't think anyone intends to support vcpu asymmetry in a VM, and 64 registers per vcpu is not an insignificant memory usage). If there are no reasons for this to be per-CPU, please move it to be global to the VM. This also mean that once a vcpu has reset, it shouldn't be possible to affect the registers. This shouldn't affect the userspace API though. Thanks, M.
On Sun, Nov 21, 2021 at 4:37 AM Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 17 Nov 2021 06:43:32 +0000, > Reiji Watanabe <reijiw@google.com> wrote: > > > > Extend sys_regs[] of kvm_cpu_context for ID registers and save ID > > registers' sanitized value in the array for the vCPU at the first > > vCPU reset. Use the saved ones when ID registers are read by > > userspace (via KVM_GET_ONE_REG) or the guest. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 10 +++++++ > > arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ > > 2 files changed, 37 insertions(+), 16 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index edbe2cb21947..72db73c79403 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { > > u64 disr_el1; /* Deferred [SError] Status Register */ > > }; > > > > +/* > > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > > + * where 0<=crm<8, 0<=op2<8. > > + */ > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) > > + > > enum vcpu_sysreg { > > __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ > > MPIDR_EL1, /* MultiProcessor Affinity Register */ > > @@ -210,6 +218,8 @@ enum vcpu_sysreg { > > CNTP_CVAL_EL0, > > CNTP_CTL_EL0, > > > > + ID_REG_BASE, > > + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, > > It is rather unclear to me why we want these registers to be > replicated on a per-CPU basis. Yes, this fits the architecture, but > that's also a total waste of memory if you have more than a single > CPU, because we make a point in only exposing homogeneous properties > to the VM (I don't think anyone intends to support vcpu asymmetry in a > VM, and 64 registers per vcpu is not an insignificant memory usage). > > If there are no reasons for this to be per-CPU, please move it to be > global to the VM. This also mean that once a vcpu has reset, it > shouldn't be possible to affect the registers. This shouldn't affect > the userspace API though. Currently, userspace can configure different CPU features for each vCPU with KVM_ARM_VCPU_INIT, which indirectly affect ID registers. I'm not sure if anyone actually does that though. Since I personally thought having ID registers per vCPU more naturally fits the behavior of KVM_ARM_VCPU_INIT and makes more straightforward behavior of KVM_SET_ONE_REG, I chose that. (That would be also better in terms of vCPUs scalability for live migration considering a case where userspace tries to restore ID registers for many vCPUs in parallel during live migration. Userspace could avoid that, and there are ways for KVM to mitigate that though.) Having ID registers per-VM is of course feasible even while maintaining the current behavior of KVM_ARM_VCPU_INIT though. Thanks, Reiji
On Tue, 23 Nov 2021 04:39:27 +0000, Reiji Watanabe <reijiw@google.com> wrote: > > On Sun, Nov 21, 2021 at 4:37 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Wed, 17 Nov 2021 06:43:32 +0000, > > Reiji Watanabe <reijiw@google.com> wrote: > > > > > > Extend sys_regs[] of kvm_cpu_context for ID registers and save ID > > > registers' sanitized value in the array for the vCPU at the first > > > vCPU reset. Use the saved ones when ID registers are read by > > > userspace (via KVM_GET_ONE_REG) or the guest. > > > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > > --- > > > arch/arm64/include/asm/kvm_host.h | 10 +++++++ > > > arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ > > > 2 files changed, 37 insertions(+), 16 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > > index edbe2cb21947..72db73c79403 100644 > > > --- a/arch/arm64/include/asm/kvm_host.h > > > +++ b/arch/arm64/include/asm/kvm_host.h > > > @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { > > > u64 disr_el1; /* Deferred [SError] Status Register */ > > > }; > > > > > > +/* > > > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > > > + * where 0<=crm<8, 0<=op2<8. > > > + */ > > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > > +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) > > > + > > > enum vcpu_sysreg { > > > __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ > > > MPIDR_EL1, /* MultiProcessor Affinity Register */ > > > @@ -210,6 +218,8 @@ enum vcpu_sysreg { > > > CNTP_CVAL_EL0, > > > CNTP_CTL_EL0, > > > > > > + ID_REG_BASE, > > > + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, > > > > It is rather unclear to me why we want these registers to be > > replicated on a per-CPU basis. Yes, this fits the architecture, but > > that's also a total waste of memory if you have more than a single > > CPU, because we make a point in only exposing homogeneous properties > > to the VM (I don't think anyone intends to support vcpu asymmetry in a > > VM, and 64 registers per vcpu is not an insignificant memory usage). > > > > If there are no reasons for this to be per-CPU, please move it to be > > global to the VM. This also mean that once a vcpu has reset, it > > shouldn't be possible to affect the registers. This shouldn't affect > > the userspace API though. > > > Currently, userspace can configure different CPU features for each vCPU > with KVM_ARM_VCPU_INIT, which indirectly affect ID registers. > I'm not sure if anyone actually does that though. But the way the ID regs are affected is always global AFAICT. For example, if you instantiate a PMU, you do so on all vcpus, even of the architecture allows you to build something completely asymmetric. > Since I personally thought having ID registers per vCPU more naturally > fits the behavior of KVM_ARM_VCPU_INIT and makes more straightforward > behavior of KVM_SET_ONE_REG, I chose that. I agree that this is the logical approach from an architectural PoV. > (That would be also better in terms of vCPUs scalability for live migration > considering a case where userspace tries to restore ID registers for > many vCPUs in parallel during live migration. Userspace could avoid that, > and there are ways for KVM to mitigate that though.) I think these are orthogonal things. We can expose a per-vcpu view, but there is no need to have per-vcpu storage and to allow asymmetric VMs. If I have anything to say about the future of KVM/arm64, it will be that I don't want to support this at all. > Having ID registers per-VM is of course feasible even while maintaining > the current behavior of KVM_ARM_VCPU_INIT though. Exactly. per-VM storage, and per-vcpu visibility. It will prevent all sort of odd behaviours. M.
On Tue, Nov 23, 2021 at 2:03 AM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 23 Nov 2021 04:39:27 +0000, > Reiji Watanabe <reijiw@google.com> wrote: > > > > On Sun, Nov 21, 2021 at 4:37 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Wed, 17 Nov 2021 06:43:32 +0000, > > > Reiji Watanabe <reijiw@google.com> wrote: > > > > > > > > Extend sys_regs[] of kvm_cpu_context for ID registers and save ID > > > > registers' sanitized value in the array for the vCPU at the first > > > > vCPU reset. Use the saved ones when ID registers are read by > > > > userspace (via KVM_GET_ONE_REG) or the guest. > > > > > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > > > --- > > > > arch/arm64/include/asm/kvm_host.h | 10 +++++++ > > > > arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ > > > > 2 files changed, 37 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > > > index edbe2cb21947..72db73c79403 100644 > > > > --- a/arch/arm64/include/asm/kvm_host.h > > > > +++ b/arch/arm64/include/asm/kvm_host.h > > > > @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { > > > > u64 disr_el1; /* Deferred [SError] Status Register */ > > > > }; > > > > > > > > +/* > > > > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > > > > + * where 0<=crm<8, 0<=op2<8. > > > > + */ > > > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > > > +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) > > > > + > > > > enum vcpu_sysreg { > > > > __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ > > > > MPIDR_EL1, /* MultiProcessor Affinity Register */ > > > > @@ -210,6 +218,8 @@ enum vcpu_sysreg { > > > > CNTP_CVAL_EL0, > > > > CNTP_CTL_EL0, > > > > > > > > + ID_REG_BASE, > > > > + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, > > > > > > It is rather unclear to me why we want these registers to be > > > replicated on a per-CPU basis. Yes, this fits the architecture, but > > > that's also a total waste of memory if you have more than a single > > > CPU, because we make a point in only exposing homogeneous properties > > > to the VM (I don't think anyone intends to support vcpu asymmetry in a > > > VM, and 64 registers per vcpu is not an insignificant memory usage). > > > > > > If there are no reasons for this to be per-CPU, please move it to be > > > global to the VM. This also mean that once a vcpu has reset, it > > > shouldn't be possible to affect the registers. This shouldn't affect > > > the userspace API though. > > > > > > Currently, userspace can configure different CPU features for each vCPU > > with KVM_ARM_VCPU_INIT, which indirectly affect ID registers. > > I'm not sure if anyone actually does that though. > > But the way the ID regs are affected is always global AFAICT. For > example, if you instantiate a PMU, you do so on all vcpus, even of the > architecture allows you to build something completely asymmetric. > > > Since I personally thought having ID registers per vCPU more naturally > > fits the behavior of KVM_ARM_VCPU_INIT and makes more straightforward > > behavior of KVM_SET_ONE_REG, I chose that. > > I agree that this is the logical approach from an architectural PoV. > > > (That would be also better in terms of vCPUs scalability for live migration > > considering a case where userspace tries to restore ID registers for > > many vCPUs in parallel during live migration. Userspace could avoid that, > > and there are ways for KVM to mitigate that though.) > > I think these are orthogonal things. We can expose a per-vcpu view, > but there is no need to have per-vcpu storage and to allow asymmetric > VMs. If I have anything to say about the future of KVM/arm64, it will > be that I don't want to support this at all. > > > Having ID registers per-VM is of course feasible even while maintaining > > the current behavior of KVM_ARM_VCPU_INIT though. > > Exactly. per-VM storage, and per-vcpu visibility. It will prevent all > sort of odd behaviours. Thank you so much for all your comments. I will make it per VM storage. Regards, Reiji
Hi Reiji, On 11/18/21 11:00 PM, Reiji Watanabe wrote: > Hi Eric, > > On Thu, Nov 18, 2021 at 12:37 PM Eric Auger <eauger@redhat.com> wrote: >> >> Hi Reiji, >> >> On 11/17/21 7:43 AM, Reiji Watanabe wrote: >>> Extend sys_regs[] of kvm_cpu_context for ID registers and save ID >>> registers' sanitized value in the array for the vCPU at the first >>> vCPU reset. Use the saved ones when ID registers are read by >>> userspace (via KVM_GET_ONE_REG) or the guest. >>> >>> Signed-off-by: Reiji Watanabe <reijiw@google.com> >>> --- >>> arch/arm64/include/asm/kvm_host.h | 10 +++++++ >>> arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ >>> 2 files changed, 37 insertions(+), 16 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>> index edbe2cb21947..72db73c79403 100644 >>> --- a/arch/arm64/include/asm/kvm_host.h >>> +++ b/arch/arm64/include/asm/kvm_host.h >>> @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { >>> u64 disr_el1; /* Deferred [SError] Status Register */ >>> }; >>> >>> +/* >>> + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), >>> + * where 0<=crm<8, 0<=op2<8. >>> + */ >>> +#define KVM_ARM_ID_REG_MAX_NUM 64 >>> +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) >>> +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) >>> + >>> enum vcpu_sysreg { >>> __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ >>> MPIDR_EL1, /* MultiProcessor Affinity Register */ >>> @@ -210,6 +218,8 @@ enum vcpu_sysreg { >>> CNTP_CVAL_EL0, >>> CNTP_CTL_EL0, >>> >>> + ID_REG_BASE, >>> + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, >>> /* Memory Tagging Extension registers */ >>> RGSR_EL1, /* Random Allocation Tag Seed Register */ >>> GCR_EL1, /* Tag Control Register */ >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>> index e3ec1a44f94d..5608d3410660 100644 >>> --- a/arch/arm64/kvm/sys_regs.c >>> +++ b/arch/arm64/kvm/sys_regs.c >>> @@ -33,6 +33,8 @@ >>> >>> #include "trace.h" >>> >>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); >>> + >>> /* >>> * All of this file is extremely similar to the ARM coproc.c, but the >>> * types are different. My gut feeling is that it should be pretty >>> @@ -273,7 +275,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, >>> struct sys_reg_params *p, >>> const struct sys_reg_desc *r) >>> { >>> - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); >>> + u64 val = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); >>> u32 sr = reg_to_encoding(r); >>> >>> if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { >>> @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, >>> return true; >>> } >>> >>> -/* Read a sanitised cpufeature ID register by sys_reg_desc */ >>> -static u64 read_id_reg(const struct kvm_vcpu *vcpu, >>> - struct sys_reg_desc const *r, bool raz) >>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) >>> { >>> - u32 id = reg_to_encoding(r); >>> - u64 val; >>> - >>> - if (raz) >>> - return 0; >>> - >>> - val = read_sanitised_ftr_reg(id); >>> + u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); >>> >>> switch (id) { >>> case SYS_ID_AA64PFR0_EL1: >>> @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, >>> return val; >>> } >>> >>> +static u64 read_id_reg(const struct kvm_vcpu *vcpu, >>> + struct sys_reg_desc const *r, bool raz) >>> +{ >>> + u32 id = reg_to_encoding(r); >>> + >>> + return raz ? 0 : __read_id_reg(vcpu, id); >>> +} >>> + >>> static unsigned int id_visibility(const struct kvm_vcpu *vcpu, >>> const struct sys_reg_desc *r) >>> { >>> @@ -1178,6 +1180,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, >>> return REG_HIDDEN; >>> } >>> >>> +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) >>> +{ >>> + u32 id = reg_to_encoding(rd); >>> + >>> + if (vcpu_has_reset_once(vcpu)) >>> + return; >>> + >>> + __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)) = read_sanitised_ftr_reg(id); >>> +} >>> + >>> static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, >>> const struct sys_reg_desc *rd, >>> const struct kvm_one_reg *reg, void __user *uaddr) >>> @@ -1223,9 +1235,7 @@ static int set_id_aa64pfr0_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. >>> + * We don't allow the effective value to be changed. >> This change may be moved to a subsequent patch as this patch does not >> change the behavior yet. > > Thank you for the review. > > There are three main parts in the original comments. > > (A) these registers are immutable for userspace > (B) no values are stored > (C) we don't allow the effective value to be changed > > This patch stores ID register values in sys_regs[]. > So, I don't think (B) should be there, and I removed (B). > Since (A) is essentially the same as (C), I removed (A) > (and left (C)). > > Do you think it is better to leave (A) in this patch, too ? yes I think I would leave 'for now, these registers are immutable for userspace' Eric > > Thanks, > Reiji > > >>> */ >>> static int __get_id_reg(const struct kvm_vcpu *vcpu, >>> const struct sys_reg_desc *rd, void __user *uaddr, >>> @@ -1382,6 +1392,7 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu, >>> #define ID_SANITISED(name) { \ >>> SYS_DESC(SYS_##name), \ >>> .access = access_id_reg, \ >>> + .reset = reset_id_reg, \ >>> .get_user = get_id_reg, \ >>> .set_user = set_id_reg, \ >>> .visibility = id_visibility, \ >>> @@ -1837,8 +1848,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu, >>> if (p->is_write) { >>> return ignore_write(vcpu, p); >>> } else { >>> - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); >>> - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); >>> + u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1); >>> + u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1); >>> u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT); >>> >>> p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) | >>> >> Thanks >> >> Eric >> >
Hi Reiji, On 11/17/21 7:43 AM, Reiji Watanabe wrote: > Extend sys_regs[] of kvm_cpu_context for ID registers and save ID > registers' sanitized value in the array for the vCPU at the first > vCPU reset. Use the saved ones when ID registers are read by > userspace (via KVM_GET_ONE_REG) or the guest. > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 10 +++++++ > arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ > 2 files changed, 37 insertions(+), 16 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index edbe2cb21947..72db73c79403 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { > u64 disr_el1; /* Deferred [SError] Status Register */ > }; > > +/* > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > + * where 0<=crm<8, 0<=op2<8. > + */ > +#define KVM_ARM_ID_REG_MAX_NUM 64 > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) > + > enum vcpu_sysreg { > __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ > MPIDR_EL1, /* MultiProcessor Affinity Register */ > @@ -210,6 +218,8 @@ enum vcpu_sysreg { > CNTP_CVAL_EL0, > CNTP_CTL_EL0, > > + ID_REG_BASE, > + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, > /* Memory Tagging Extension registers */ > RGSR_EL1, /* Random Allocation Tag Seed Register */ > GCR_EL1, /* Tag Control Register */ > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index e3ec1a44f94d..5608d3410660 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -33,6 +33,8 @@ > > #include "trace.h" > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); > + > /* > * All of this file is extremely similar to the ARM coproc.c, but the > * types are different. My gut feeling is that it should be pretty > @@ -273,7 +275,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, > struct sys_reg_params *p, > const struct sys_reg_desc *r) > { > - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); > + u64 val = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); > u32 sr = reg_to_encoding(r); > > if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { > @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > return true; > } > > -/* Read a sanitised cpufeature ID register by sys_reg_desc */ > -static u64 read_id_reg(const struct kvm_vcpu *vcpu, > - struct sys_reg_desc const *r, bool raz) > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > { > - u32 id = reg_to_encoding(r); > - u64 val; > - > - if (raz) > - return 0; > - > - val = read_sanitised_ftr_reg(id); > + u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > return val; > } > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, > + struct sys_reg_desc const *r, bool raz) > +{ > + u32 id = reg_to_encoding(r); > + > + return raz ? 0 : __read_id_reg(vcpu, id); > +} > + > static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > const struct sys_reg_desc *r) > { > @@ -1178,6 +1180,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, > return REG_HIDDEN; > } > > +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) > +{ > + u32 id = reg_to_encoding(rd); > + > + if (vcpu_has_reset_once(vcpu)) > + return; The KVM API allows to call VCPU_INIT several times (with same target/feature). With above check on the second call the ID_REGS won't be reset. Somehow this is aligned with target/feature behavior. However if this is what we want, I think we would need to document it in the KVM API doc. Thanks Eric > + > + __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)) = read_sanitised_ftr_reg(id); > +} > + > static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, > const struct kvm_one_reg *reg, void __user *uaddr) > @@ -1223,9 +1235,7 @@ static int set_id_aa64pfr0_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. > + * We don't allow the effective value to be changed. > */ > static int __get_id_reg(const struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, void __user *uaddr, > @@ -1382,6 +1392,7 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu, > #define ID_SANITISED(name) { \ > SYS_DESC(SYS_##name), \ > .access = access_id_reg, \ > + .reset = reset_id_reg, \ > .get_user = get_id_reg, \ > .set_user = set_id_reg, \ > .visibility = id_visibility, \ > @@ -1837,8 +1848,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu, > if (p->is_write) { > return ignore_write(vcpu, p); > } else { > - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); > - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); > + u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1); > + u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1); > u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT); > > p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) | >
Hi Eric, On Thu, Dec 2, 2021 at 2:58 AM Eric Auger <eauger@redhat.com> wrote: > > Hi Reiji, > > On 11/17/21 7:43 AM, Reiji Watanabe wrote: > > Extend sys_regs[] of kvm_cpu_context for ID registers and save ID > > registers' sanitized value in the array for the vCPU at the first > > vCPU reset. Use the saved ones when ID registers are read by > > userspace (via KVM_GET_ONE_REG) or the guest. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 10 +++++++ > > arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ > > 2 files changed, 37 insertions(+), 16 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index edbe2cb21947..72db73c79403 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { > > u64 disr_el1; /* Deferred [SError] Status Register */ > > }; > > > > +/* > > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > > + * where 0<=crm<8, 0<=op2<8. > > + */ > > +#define KVM_ARM_ID_REG_MAX_NUM 64 > > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > > +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) > > + > > enum vcpu_sysreg { > > __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ > > MPIDR_EL1, /* MultiProcessor Affinity Register */ > > @@ -210,6 +218,8 @@ enum vcpu_sysreg { > > CNTP_CVAL_EL0, > > CNTP_CTL_EL0, > > > > + ID_REG_BASE, > > + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, > > /* Memory Tagging Extension registers */ > > RGSR_EL1, /* Random Allocation Tag Seed Register */ > > GCR_EL1, /* Tag Control Register */ > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index e3ec1a44f94d..5608d3410660 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -33,6 +33,8 @@ > > > > #include "trace.h" > > > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); > > + > > /* > > * All of this file is extremely similar to the ARM coproc.c, but the > > * types are different. My gut feeling is that it should be pretty > > @@ -273,7 +275,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, > > struct sys_reg_params *p, > > const struct sys_reg_desc *r) > > { > > - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); > > + u64 val = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); > > u32 sr = reg_to_encoding(r); > > > > if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { > > @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > > return true; > > } > > > > -/* Read a sanitised cpufeature ID register by sys_reg_desc */ > > -static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > - struct sys_reg_desc const *r, bool raz) > > +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > > { > > - u32 id = reg_to_encoding(r); > > - u64 val; > > - > > - if (raz) > > - return 0; > > - > > - val = read_sanitised_ftr_reg(id); > > + u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > > > > switch (id) { > > case SYS_ID_AA64PFR0_EL1: > > @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > return val; > > } > > > > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, > > + struct sys_reg_desc const *r, bool raz) > > +{ > > + u32 id = reg_to_encoding(r); > > + > > + return raz ? 0 : __read_id_reg(vcpu, id); > > +} > > + > > static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > > const struct sys_reg_desc *r) > > { > > @@ -1178,6 +1180,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, > > return REG_HIDDEN; > > } > > > > +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) > > +{ > > + u32 id = reg_to_encoding(rd); > > + > > + if (vcpu_has_reset_once(vcpu)) > > + return; > The KVM API allows to call VCPU_INIT several times (with same > target/feature). With above check on the second call the ID_REGS won't > be reset. Somehow this is aligned with target/feature behavior. However > if this is what we want, I think we would need to document it in the KVM > API doc. Thank you for the comment. That is what we want. Since ID registers are read only registers, their values must not change across the reset. '4.82 KVM_ARM_VCPU_INIT' in api.rst says: System registers: Reset to their architecturally defined values as for a warm reset to EL1 (resp. SVC) Since this reset behavior for the ID registers follows what is described above, I'm not sure if we need to document the reset behavior of the ID registers specifically. If KVM changes the values across the resets, I would think it rather needs to be documented though. Thanks, Reiji
Hi Reiji, On 12/4/21 2:45 AM, Reiji Watanabe wrote: > Hi Eric, > > On Thu, Dec 2, 2021 at 2:58 AM Eric Auger <eauger@redhat.com> wrote: >> >> Hi Reiji, >> >> On 11/17/21 7:43 AM, Reiji Watanabe wrote: >>> Extend sys_regs[] of kvm_cpu_context for ID registers and save ID >>> registers' sanitized value in the array for the vCPU at the first >>> vCPU reset. Use the saved ones when ID registers are read by >>> userspace (via KVM_GET_ONE_REG) or the guest. >>> >>> Signed-off-by: Reiji Watanabe <reijiw@google.com> >>> --- >>> arch/arm64/include/asm/kvm_host.h | 10 +++++++ >>> arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ >>> 2 files changed, 37 insertions(+), 16 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>> index edbe2cb21947..72db73c79403 100644 >>> --- a/arch/arm64/include/asm/kvm_host.h >>> +++ b/arch/arm64/include/asm/kvm_host.h >>> @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { >>> u64 disr_el1; /* Deferred [SError] Status Register */ >>> }; >>> >>> +/* >>> + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), >>> + * where 0<=crm<8, 0<=op2<8. >>> + */ >>> +#define KVM_ARM_ID_REG_MAX_NUM 64 >>> +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) >>> +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) >>> + >>> enum vcpu_sysreg { >>> __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ >>> MPIDR_EL1, /* MultiProcessor Affinity Register */ >>> @@ -210,6 +218,8 @@ enum vcpu_sysreg { >>> CNTP_CVAL_EL0, >>> CNTP_CTL_EL0, >>> >>> + ID_REG_BASE, >>> + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, >>> /* Memory Tagging Extension registers */ >>> RGSR_EL1, /* Random Allocation Tag Seed Register */ >>> GCR_EL1, /* Tag Control Register */ >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>> index e3ec1a44f94d..5608d3410660 100644 >>> --- a/arch/arm64/kvm/sys_regs.c >>> +++ b/arch/arm64/kvm/sys_regs.c >>> @@ -33,6 +33,8 @@ >>> >>> #include "trace.h" >>> >>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); >>> + >>> /* >>> * All of this file is extremely similar to the ARM coproc.c, but the >>> * types are different. My gut feeling is that it should be pretty >>> @@ -273,7 +275,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, >>> struct sys_reg_params *p, >>> const struct sys_reg_desc *r) >>> { >>> - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); >>> + u64 val = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); >>> u32 sr = reg_to_encoding(r); >>> >>> if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { >>> @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, >>> return true; >>> } >>> >>> -/* Read a sanitised cpufeature ID register by sys_reg_desc */ >>> -static u64 read_id_reg(const struct kvm_vcpu *vcpu, >>> - struct sys_reg_desc const *r, bool raz) >>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) >>> { >>> - u32 id = reg_to_encoding(r); >>> - u64 val; >>> - >>> - if (raz) >>> - return 0; >>> - >>> - val = read_sanitised_ftr_reg(id); >>> + u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); >>> >>> switch (id) { >>> case SYS_ID_AA64PFR0_EL1: >>> @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, >>> return val; >>> } >>> >>> +static u64 read_id_reg(const struct kvm_vcpu *vcpu, >>> + struct sys_reg_desc const *r, bool raz) >>> +{ >>> + u32 id = reg_to_encoding(r); >>> + >>> + return raz ? 0 : __read_id_reg(vcpu, id); >>> +} >>> + >>> static unsigned int id_visibility(const struct kvm_vcpu *vcpu, >>> const struct sys_reg_desc *r) >>> { >>> @@ -1178,6 +1180,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, >>> return REG_HIDDEN; >>> } >>> >>> +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) >>> +{ >>> + u32 id = reg_to_encoding(rd); >>> + >>> + if (vcpu_has_reset_once(vcpu)) >>> + return; >> The KVM API allows to call VCPU_INIT several times (with same >> target/feature). With above check on the second call the ID_REGS won't >> be reset. Somehow this is aligned with target/feature behavior. However >> if this is what we want, I think we would need to document it in the KVM >> API doc. > > Thank you for the comment. > > That is what we want. Since ID registers are read only registers, > their values must not change across the reset. > > '4.82 KVM_ARM_VCPU_INIT' in api.rst says: > > System registers: Reset to their architecturally defined > values as for a warm reset to EL1 (resp. SVC) > > Since this reset behavior for the ID registers follows what is > described above, I'm not sure if we need to document the reset > behavior of the ID registers specifically. > If KVM changes the values across the resets, I would think it > rather needs to be documented though. Makes sense to freeze the ID REGs on the 1st reset. Was just wondering if we shouldn't add that the ID REG values are immutable after the 1st VCPU_INIT. Thanks Eric > > Thanks, > Reiji >
Hi Eric, On Tue, Dec 7, 2021 at 1:34 AM Eric Auger <eauger@redhat.com> wrote: > > Hi Reiji, > > On 12/4/21 2:45 AM, Reiji Watanabe wrote: > > Hi Eric, > > > > On Thu, Dec 2, 2021 at 2:58 AM Eric Auger <eauger@redhat.com> wrote: > >> > >> Hi Reiji, > >> > >> On 11/17/21 7:43 AM, Reiji Watanabe wrote: > >>> Extend sys_regs[] of kvm_cpu_context for ID registers and save ID > >>> registers' sanitized value in the array for the vCPU at the first > >>> vCPU reset. Use the saved ones when ID registers are read by > >>> userspace (via KVM_GET_ONE_REG) or the guest. > >>> > >>> Signed-off-by: Reiji Watanabe <reijiw@google.com> > >>> --- > >>> arch/arm64/include/asm/kvm_host.h | 10 +++++++ > >>> arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ > >>> 2 files changed, 37 insertions(+), 16 deletions(-) > >>> > >>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >>> index edbe2cb21947..72db73c79403 100644 > >>> --- a/arch/arm64/include/asm/kvm_host.h > >>> +++ b/arch/arm64/include/asm/kvm_host.h > >>> @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { > >>> u64 disr_el1; /* Deferred [SError] Status Register */ > >>> }; > >>> > >>> +/* > >>> + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > >>> + * where 0<=crm<8, 0<=op2<8. > >>> + */ > >>> +#define KVM_ARM_ID_REG_MAX_NUM 64 > >>> +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > >>> +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) > >>> + > >>> enum vcpu_sysreg { > >>> __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ > >>> MPIDR_EL1, /* MultiProcessor Affinity Register */ > >>> @@ -210,6 +218,8 @@ enum vcpu_sysreg { > >>> CNTP_CVAL_EL0, > >>> CNTP_CTL_EL0, > >>> > >>> + ID_REG_BASE, > >>> + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, > >>> /* Memory Tagging Extension registers */ > >>> RGSR_EL1, /* Random Allocation Tag Seed Register */ > >>> GCR_EL1, /* Tag Control Register */ > >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > >>> index e3ec1a44f94d..5608d3410660 100644 > >>> --- a/arch/arm64/kvm/sys_regs.c > >>> +++ b/arch/arm64/kvm/sys_regs.c > >>> @@ -33,6 +33,8 @@ > >>> > >>> #include "trace.h" > >>> > >>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); > >>> + > >>> /* > >>> * All of this file is extremely similar to the ARM coproc.c, but the > >>> * types are different. My gut feeling is that it should be pretty > >>> @@ -273,7 +275,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, > >>> struct sys_reg_params *p, > >>> const struct sys_reg_desc *r) > >>> { > >>> - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); > >>> + u64 val = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); > >>> u32 sr = reg_to_encoding(r); > >>> > >>> if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { > >>> @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > >>> return true; > >>> } > >>> > >>> -/* Read a sanitised cpufeature ID register by sys_reg_desc */ > >>> -static u64 read_id_reg(const struct kvm_vcpu *vcpu, > >>> - struct sys_reg_desc const *r, bool raz) > >>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > >>> { > >>> - u32 id = reg_to_encoding(r); > >>> - u64 val; > >>> - > >>> - if (raz) > >>> - return 0; > >>> - > >>> - val = read_sanitised_ftr_reg(id); > >>> + u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > >>> > >>> switch (id) { > >>> case SYS_ID_AA64PFR0_EL1: > >>> @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > >>> return val; > >>> } > >>> > >>> +static u64 read_id_reg(const struct kvm_vcpu *vcpu, > >>> + struct sys_reg_desc const *r, bool raz) > >>> +{ > >>> + u32 id = reg_to_encoding(r); > >>> + > >>> + return raz ? 0 : __read_id_reg(vcpu, id); > >>> +} > >>> + > >>> static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > >>> const struct sys_reg_desc *r) > >>> { > >>> @@ -1178,6 +1180,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, > >>> return REG_HIDDEN; > >>> } > >>> > >>> +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) > >>> +{ > >>> + u32 id = reg_to_encoding(rd); > >>> + > >>> + if (vcpu_has_reset_once(vcpu)) > >>> + return; > >> The KVM API allows to call VCPU_INIT several times (with same > >> target/feature). With above check on the second call the ID_REGS won't > >> be reset. Somehow this is aligned with target/feature behavior. However > >> if this is what we want, I think we would need to document it in the KVM > >> API doc. > > > > Thank you for the comment. > > > > That is what we want. Since ID registers are read only registers, > > their values must not change across the reset. > > > > '4.82 KVM_ARM_VCPU_INIT' in api.rst says: > > > > System registers: Reset to their architecturally defined > > values as for a warm reset to EL1 (resp. SVC) > > > > Since this reset behavior for the ID registers follows what is > > described above, I'm not sure if we need to document the reset > > behavior of the ID registers specifically. > > If KVM changes the values across the resets, I would think it > > rather needs to be documented though. > > Makes sense to freeze the ID REGs on the 1st reset. Was just wondering > if we shouldn't add that the ID REG values are immutable after the 1st > VCPU_INIT. > Makes sense to freeze the ID REGs on the 1st reset. Was just wondering > if we shouldn't add that the ID REG values are immutable after the 1st > VCPU_INIT. Even after the 1st VCPU_INIT, ID REG values can be changed by KVM_SET_ONE_REG (KVM_SET_ONE_REG/KVM_GET_ONE_REG are allowed only after the 1st VCPU_INIT). The ID REG values are immutable after the 1st KVM_RUN, and I think we should document that. Is that what you meant ? Thanks, Reiji
Hi Reiji, On 12/8/21 6:57 AM, Reiji Watanabe wrote: > Hi Eric, > > On Tue, Dec 7, 2021 at 1:34 AM Eric Auger <eauger@redhat.com> wrote: >> >> Hi Reiji, >> >> On 12/4/21 2:45 AM, Reiji Watanabe wrote: >>> Hi Eric, >>> >>> On Thu, Dec 2, 2021 at 2:58 AM Eric Auger <eauger@redhat.com> wrote: >>>> >>>> Hi Reiji, >>>> >>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: >>>>> Extend sys_regs[] of kvm_cpu_context for ID registers and save ID >>>>> registers' sanitized value in the array for the vCPU at the first >>>>> vCPU reset. Use the saved ones when ID registers are read by >>>>> userspace (via KVM_GET_ONE_REG) or the guest. >>>>> >>>>> Signed-off-by: Reiji Watanabe <reijiw@google.com> >>>>> --- >>>>> arch/arm64/include/asm/kvm_host.h | 10 +++++++ >>>>> arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ >>>>> 2 files changed, 37 insertions(+), 16 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>>>> index edbe2cb21947..72db73c79403 100644 >>>>> --- a/arch/arm64/include/asm/kvm_host.h >>>>> +++ b/arch/arm64/include/asm/kvm_host.h >>>>> @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { >>>>> u64 disr_el1; /* Deferred [SError] Status Register */ >>>>> }; >>>>> >>>>> +/* >>>>> + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), >>>>> + * where 0<=crm<8, 0<=op2<8. >>>>> + */ >>>>> +#define KVM_ARM_ID_REG_MAX_NUM 64 >>>>> +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) >>>>> +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) >>>>> + >>>>> enum vcpu_sysreg { >>>>> __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ >>>>> MPIDR_EL1, /* MultiProcessor Affinity Register */ >>>>> @@ -210,6 +218,8 @@ enum vcpu_sysreg { >>>>> CNTP_CVAL_EL0, >>>>> CNTP_CTL_EL0, >>>>> >>>>> + ID_REG_BASE, >>>>> + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, >>>>> /* Memory Tagging Extension registers */ >>>>> RGSR_EL1, /* Random Allocation Tag Seed Register */ >>>>> GCR_EL1, /* Tag Control Register */ >>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>>>> index e3ec1a44f94d..5608d3410660 100644 >>>>> --- a/arch/arm64/kvm/sys_regs.c >>>>> +++ b/arch/arm64/kvm/sys_regs.c >>>>> @@ -33,6 +33,8 @@ >>>>> >>>>> #include "trace.h" >>>>> >>>>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); >>>>> + >>>>> /* >>>>> * All of this file is extremely similar to the ARM coproc.c, but the >>>>> * types are different. My gut feeling is that it should be pretty >>>>> @@ -273,7 +275,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, >>>>> struct sys_reg_params *p, >>>>> const struct sys_reg_desc *r) >>>>> { >>>>> - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); >>>>> + u64 val = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); >>>>> u32 sr = reg_to_encoding(r); >>>>> >>>>> if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { >>>>> @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, >>>>> return true; >>>>> } >>>>> >>>>> -/* Read a sanitised cpufeature ID register by sys_reg_desc */ >>>>> -static u64 read_id_reg(const struct kvm_vcpu *vcpu, >>>>> - struct sys_reg_desc const *r, bool raz) >>>>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) >>>>> { >>>>> - u32 id = reg_to_encoding(r); >>>>> - u64 val; >>>>> - >>>>> - if (raz) >>>>> - return 0; >>>>> - >>>>> - val = read_sanitised_ftr_reg(id); >>>>> + u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); >>>>> >>>>> switch (id) { >>>>> case SYS_ID_AA64PFR0_EL1: >>>>> @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, >>>>> return val; >>>>> } >>>>> >>>>> +static u64 read_id_reg(const struct kvm_vcpu *vcpu, >>>>> + struct sys_reg_desc const *r, bool raz) >>>>> +{ >>>>> + u32 id = reg_to_encoding(r); >>>>> + >>>>> + return raz ? 0 : __read_id_reg(vcpu, id); >>>>> +} >>>>> + >>>>> static unsigned int id_visibility(const struct kvm_vcpu *vcpu, >>>>> const struct sys_reg_desc *r) >>>>> { >>>>> @@ -1178,6 +1180,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, >>>>> return REG_HIDDEN; >>>>> } >>>>> >>>>> +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) >>>>> +{ >>>>> + u32 id = reg_to_encoding(rd); >>>>> + >>>>> + if (vcpu_has_reset_once(vcpu)) >>>>> + return; >>>> The KVM API allows to call VCPU_INIT several times (with same >>>> target/feature). With above check on the second call the ID_REGS won't >>>> be reset. Somehow this is aligned with target/feature behavior. However >>>> if this is what we want, I think we would need to document it in the KVM >>>> API doc. >>> >>> Thank you for the comment. >>> >>> That is what we want. Since ID registers are read only registers, >>> their values must not change across the reset. >>> >>> '4.82 KVM_ARM_VCPU_INIT' in api.rst says: >>> >>> System registers: Reset to their architecturally defined >>> values as for a warm reset to EL1 (resp. SVC) >>> >>> Since this reset behavior for the ID registers follows what is >>> described above, I'm not sure if we need to document the reset >>> behavior of the ID registers specifically. >>> If KVM changes the values across the resets, I would think it >>> rather needs to be documented though. >> >> Makes sense to freeze the ID REGs on the 1st reset. Was just wondering >> if we shouldn't add that the ID REG values are immutable after the 1st >> VCPU_INIT. > >> Makes sense to freeze the ID REGs on the 1st reset. Was just wondering >> if we shouldn't add that the ID REG values are immutable after the 1st >> VCPU_INIT. > > Even after the 1st VCPU_INIT, ID REG values can be changed by > KVM_SET_ONE_REG (KVM_SET_ONE_REG/KVM_GET_ONE_REG are allowed > only after the 1st VCPU_INIT). > > The ID REG values are immutable after the 1st KVM_RUN, > and I think we should document that. Is that what you meant ? Yes that's what I meant sorry. Eric > > Thanks, > Reiji >
Hi Eric, On Tue, Dec 7, 2021 at 11:09 PM Eric Auger <eauger@redhat.com> wrote: > > Hi Reiji, > > On 12/8/21 6:57 AM, Reiji Watanabe wrote: > > Hi Eric, > > > > On Tue, Dec 7, 2021 at 1:34 AM Eric Auger <eauger@redhat.com> wrote: > >> > >> Hi Reiji, > >> > >> On 12/4/21 2:45 AM, Reiji Watanabe wrote: > >>> Hi Eric, > >>> > >>> On Thu, Dec 2, 2021 at 2:58 AM Eric Auger <eauger@redhat.com> wrote: > >>>> > >>>> Hi Reiji, > >>>> > >>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote: > >>>>> Extend sys_regs[] of kvm_cpu_context for ID registers and save ID > >>>>> registers' sanitized value in the array for the vCPU at the first > >>>>> vCPU reset. Use the saved ones when ID registers are read by > >>>>> userspace (via KVM_GET_ONE_REG) or the guest. > >>>>> > >>>>> Signed-off-by: Reiji Watanabe <reijiw@google.com> > >>>>> --- > >>>>> arch/arm64/include/asm/kvm_host.h | 10 +++++++ > >>>>> arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ > >>>>> 2 files changed, 37 insertions(+), 16 deletions(-) > >>>>> > >>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >>>>> index edbe2cb21947..72db73c79403 100644 > >>>>> --- a/arch/arm64/include/asm/kvm_host.h > >>>>> +++ b/arch/arm64/include/asm/kvm_host.h > >>>>> @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { > >>>>> u64 disr_el1; /* Deferred [SError] Status Register */ > >>>>> }; > >>>>> > >>>>> +/* > >>>>> + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), > >>>>> + * where 0<=crm<8, 0<=op2<8. > >>>>> + */ > >>>>> +#define KVM_ARM_ID_REG_MAX_NUM 64 > >>>>> +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) > >>>>> +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) > >>>>> + > >>>>> enum vcpu_sysreg { > >>>>> __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ > >>>>> MPIDR_EL1, /* MultiProcessor Affinity Register */ > >>>>> @@ -210,6 +218,8 @@ enum vcpu_sysreg { > >>>>> CNTP_CVAL_EL0, > >>>>> CNTP_CTL_EL0, > >>>>> > >>>>> + ID_REG_BASE, > >>>>> + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, > >>>>> /* Memory Tagging Extension registers */ > >>>>> RGSR_EL1, /* Random Allocation Tag Seed Register */ > >>>>> GCR_EL1, /* Tag Control Register */ > >>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > >>>>> index e3ec1a44f94d..5608d3410660 100644 > >>>>> --- a/arch/arm64/kvm/sys_regs.c > >>>>> +++ b/arch/arm64/kvm/sys_regs.c > >>>>> @@ -33,6 +33,8 @@ > >>>>> > >>>>> #include "trace.h" > >>>>> > >>>>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); > >>>>> + > >>>>> /* > >>>>> * All of this file is extremely similar to the ARM coproc.c, but the > >>>>> * types are different. My gut feeling is that it should be pretty > >>>>> @@ -273,7 +275,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, > >>>>> struct sys_reg_params *p, > >>>>> const struct sys_reg_desc *r) > >>>>> { > >>>>> - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); > >>>>> + u64 val = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); > >>>>> u32 sr = reg_to_encoding(r); > >>>>> > >>>>> if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { > >>>>> @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > >>>>> return true; > >>>>> } > >>>>> > >>>>> -/* Read a sanitised cpufeature ID register by sys_reg_desc */ > >>>>> -static u64 read_id_reg(const struct kvm_vcpu *vcpu, > >>>>> - struct sys_reg_desc const *r, bool raz) > >>>>> +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > >>>>> { > >>>>> - u32 id = reg_to_encoding(r); > >>>>> - u64 val; > >>>>> - > >>>>> - if (raz) > >>>>> - return 0; > >>>>> - > >>>>> - val = read_sanitised_ftr_reg(id); > >>>>> + u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > >>>>> > >>>>> switch (id) { > >>>>> case SYS_ID_AA64PFR0_EL1: > >>>>> @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > >>>>> return val; > >>>>> } > >>>>> > >>>>> +static u64 read_id_reg(const struct kvm_vcpu *vcpu, > >>>>> + struct sys_reg_desc const *r, bool raz) > >>>>> +{ > >>>>> + u32 id = reg_to_encoding(r); > >>>>> + > >>>>> + return raz ? 0 : __read_id_reg(vcpu, id); > >>>>> +} > >>>>> + > >>>>> static unsigned int id_visibility(const struct kvm_vcpu *vcpu, > >>>>> const struct sys_reg_desc *r) > >>>>> { > >>>>> @@ -1178,6 +1180,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, > >>>>> return REG_HIDDEN; > >>>>> } > >>>>> > >>>>> +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) > >>>>> +{ > >>>>> + u32 id = reg_to_encoding(rd); > >>>>> + > >>>>> + if (vcpu_has_reset_once(vcpu)) > >>>>> + return; > >>>> The KVM API allows to call VCPU_INIT several times (with same > >>>> target/feature). With above check on the second call the ID_REGS won't > >>>> be reset. Somehow this is aligned with target/feature behavior. However > >>>> if this is what we want, I think we would need to document it in the KVM > >>>> API doc. > >>> > >>> Thank you for the comment. > >>> > >>> That is what we want. Since ID registers are read only registers, > >>> their values must not change across the reset. > >>> > >>> '4.82 KVM_ARM_VCPU_INIT' in api.rst says: > >>> > >>> System registers: Reset to their architecturally defined > >>> values as for a warm reset to EL1 (resp. SVC) > >>> > >>> Since this reset behavior for the ID registers follows what is > >>> described above, I'm not sure if we need to document the reset > >>> behavior of the ID registers specifically. > >>> If KVM changes the values across the resets, I would think it > >>> rather needs to be documented though. > >> > >> Makes sense to freeze the ID REGs on the 1st reset. Was just wondering > >> if we shouldn't add that the ID REG values are immutable after the 1st > >> VCPU_INIT. > > > >> Makes sense to freeze the ID REGs on the 1st reset. Was just wondering > >> if we shouldn't add that the ID REG values are immutable after the 1st > >> VCPU_INIT. > > > > Even after the 1st VCPU_INIT, ID REG values can be changed by > > KVM_SET_ONE_REG (KVM_SET_ONE_REG/KVM_GET_ONE_REG are allowed > > only after the 1st VCPU_INIT). > > > > The ID REG values are immutable after the 1st KVM_RUN, > > and I think we should document that. Is that what you meant ? > Yes that's what I meant sorry. Thank you for the clarification ! I will document that. Thanks, Reiji
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index edbe2cb21947..72db73c79403 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -146,6 +146,14 @@ struct kvm_vcpu_fault_info { u64 disr_el1; /* Deferred [SError] Status Register */ }; +/* + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2), + * where 0<=crm<8, 0<=op2<8. + */ +#define KVM_ARM_ID_REG_MAX_NUM 64 +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id)) +#define IDREG_SYS_IDX(id) (ID_REG_BASE + IDREG_IDX(id)) + enum vcpu_sysreg { __INVALID_SYSREG__, /* 0 is reserved as an invalid value */ MPIDR_EL1, /* MultiProcessor Affinity Register */ @@ -210,6 +218,8 @@ enum vcpu_sysreg { CNTP_CVAL_EL0, CNTP_CTL_EL0, + ID_REG_BASE, + ID_REG_END = ID_REG_BASE + KVM_ARM_ID_REG_MAX_NUM - 1, /* Memory Tagging Extension registers */ RGSR_EL1, /* Random Allocation Tag Seed Register */ GCR_EL1, /* Tag Control Register */ diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index e3ec1a44f94d..5608d3410660 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -33,6 +33,8 @@ #include "trace.h" +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id); + /* * All of this file is extremely similar to the ARM coproc.c, but the * types are different. My gut feeling is that it should be pretty @@ -273,7 +275,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1); + u64 val = __read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1); u32 sr = reg_to_encoding(r); if (!(val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))) { @@ -1059,17 +1061,9 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, return true; } -/* Read a sanitised cpufeature ID register by sys_reg_desc */ -static u64 read_id_reg(const struct kvm_vcpu *vcpu, - struct sys_reg_desc const *r, bool raz) +static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) { - u32 id = reg_to_encoding(r); - u64 val; - - if (raz) - return 0; - - val = read_sanitised_ftr_reg(id); + u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); switch (id) { case SYS_ID_AA64PFR0_EL1: @@ -1119,6 +1113,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, return val; } +static u64 read_id_reg(const struct kvm_vcpu *vcpu, + struct sys_reg_desc const *r, bool raz) +{ + u32 id = reg_to_encoding(r); + + return raz ? 0 : __read_id_reg(vcpu, id); +} + static unsigned int id_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { @@ -1178,6 +1180,16 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu, return REG_HIDDEN; } +static void reset_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) +{ + u32 id = reg_to_encoding(rd); + + if (vcpu_has_reset_once(vcpu)) + return; + + __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)) = read_sanitised_ftr_reg(id); +} + static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, const struct kvm_one_reg *reg, void __user *uaddr) @@ -1223,9 +1235,7 @@ static int set_id_aa64pfr0_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. + * We don't allow the effective value to be changed. */ static int __get_id_reg(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, void __user *uaddr, @@ -1382,6 +1392,7 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu, #define ID_SANITISED(name) { \ SYS_DESC(SYS_##name), \ .access = access_id_reg, \ + .reset = reset_id_reg, \ .get_user = get_id_reg, \ .set_user = set_id_reg, \ .visibility = id_visibility, \ @@ -1837,8 +1848,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu, if (p->is_write) { return ignore_write(vcpu, p); } else { - u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); - u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); + u64 dfr = __read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1); + u64 pfr = __read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1); u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL3_SHIFT); p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) |
Extend sys_regs[] of kvm_cpu_context for ID registers and save ID registers' sanitized value in the array for the vCPU at the first vCPU reset. Use the saved ones when ID registers are read by userspace (via KVM_GET_ONE_REG) or the guest. Signed-off-by: Reiji Watanabe <reijiw@google.com> --- arch/arm64/include/asm/kvm_host.h | 10 +++++++ arch/arm64/kvm/sys_regs.c | 43 +++++++++++++++++++------------ 2 files changed, 37 insertions(+), 16 deletions(-)