[PATCH v9 1/5] KVM: arm64: Save ID registers' sanitized value per guest
Jing Zhang
jingzhangos at google.com
Thu May 18 12:48:52 PDT 2023
Hi Shameerali,
On Thu, May 18, 2023 at 12:17 AM Shameerali Kolothum Thodi
<shameerali.kolothum.thodi at huawei.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jing Zhang [mailto:jingzhangos at google.com]
> > Sent: 17 May 2023 07:10
> > To: KVM <kvm at vger.kernel.org>; KVMARM <kvmarm at lists.linux.dev>;
> > ARMLinux <linux-arm-kernel at lists.infradead.org>; Marc Zyngier
> > <maz at kernel.org>; Oliver Upton <oupton at google.com>
> > Cc: Will Deacon <will at kernel.org>; Paolo Bonzini <pbonzini at redhat.com>;
> > James Morse <james.morse at arm.com>; Alexandru Elisei
> > <alexandru.elisei at arm.com>; Suzuki K Poulose <suzuki.poulose at arm.com>;
> > Fuad Tabba <tabba at google.com>; Reiji Watanabe <reijiw at google.com>;
> > Raghavendra Rao Ananta <rananta at google.com>; Jing Zhang
> > <jingzhangos at google.com>
> > Subject: [PATCH v9 1/5] KVM: arm64: Save ID registers' sanitized value per
> > guest
> >
> > Introduce id_regs[] in kvm_arch as a storage of guest's ID registers,
> > and save ID registers' sanitized value in the array at KVM_CREATE_VM.
> > Use the saved ones when ID registers are read by the guest or
> > userspace (via KVM_GET_ONE_REG).
> >
> > No functional change intended.
> >
> > Co-developed-by: Reiji Watanabe <reijiw at google.com>
> > Signed-off-by: Reiji Watanabe <reijiw at google.com>
> > Signed-off-by: Jing Zhang <jingzhangos at google.com>
> > ---
> > arch/arm64/include/asm/kvm_host.h | 20 +++++++++
> > arch/arm64/kvm/arm.c | 1 +
> > arch/arm64/kvm/sys_regs.c | 69
> > +++++++++++++++++++++++++------
> > arch/arm64/kvm/sys_regs.h | 7 ++++
> > 4 files changed, 85 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h
> > b/arch/arm64/include/asm/kvm_host.h
> > index 7e7e19ef6993..949a4a782844 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -178,6 +178,21 @@ struct kvm_smccc_features {
> > unsigned long vendor_hyp_bmap;
> > };
> >
> > +/*
> > + * Emulated CPU ID registers per VM
> > + * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it
> > + * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
> > + *
> > + * These emulated idregs are VM-wide, but accessed from the context of a
> > vCPU.
> > + * Access to id regs are guarded by kvm_arch.config_lock.
> > + */
> > +#define KVM_ARM_ID_REG_NUM 56
> > +#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id))
> > +#define IDREG(kvm, id) ((kvm)->arch.idregs.regs[IDREG_IDX(id)])
> > +struct kvm_idregs {
> > + u64 regs[KVM_ARM_ID_REG_NUM];
> > +};
> >
>
> Not sure we really need this struct here. Why can't this array be moved to
> struct kvm_arch directly?
It was put in kvm_arch directly before, then got into its own
structure in v5 according to the comments here:
https://lore.kernel.org/all/861qlaxzyw.wl-maz@kernel.org/#t
>
> > typedef unsigned int pkvm_handle_t;
> >
> > struct kvm_protected_vm {
> > @@ -253,6 +268,9 @@ struct kvm_arch {
> > struct kvm_smccc_features smccc_feat;
> > struct maple_tree smccc_filter;
> >
> > + /* Emulated CPU ID registers */
> > + struct kvm_idregs idregs;
> > +
> > /*
> > * For an untrusted host VM, 'pkvm.handle' is used to lookup
> > * the associated pKVM instance in the hypervisor.
> > @@ -1045,6 +1063,8 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm
> > *kvm,
> > int kvm_vm_ioctl_set_counter_offset(struct kvm *kvm,
> > struct kvm_arm_counter_offset *offset);
> >
> > +void kvm_arm_init_id_regs(struct kvm *kvm);
> > +
> > /* Guest/host FPSIMD coordination helpers */
> > int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> > void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 14391826241c..774656a0718d 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -163,6 +163,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned
> > long type)
> >
> > set_default_spectre(kvm);
> > kvm_arm_init_hypercalls(kvm);
> > + kvm_arm_init_id_regs(kvm);
> >
> > /*
> > * Initialise the default PMUver before there is a chance to
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 71b12094d613..d2ee3a1c7f03 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -41,6 +41,7 @@
> > * 64bit interface.
> > */
> >
> > +static u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id);
> > static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> >
> > static bool read_from_write_only(struct kvm_vcpu *vcpu,
> > @@ -364,7 +365,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 = kvm_arm_read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1);
> > u32 sr = reg_to_encoding(r);
> >
> > if (!(val & (0xfUL << ID_AA64MMFR1_EL1_LO_SHIFT))) {
> > @@ -1208,16 +1209,9 @@ static u8 pmuver_to_perfmon(u8 pmuver)
> > }
> > }
> >
> > -/* 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)
> > +static u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
> > {
> > - u32 id = reg_to_encoding(r);
> > - u64 val;
> > -
> > - if (sysreg_visible_as_raz(vcpu, r))
> > - return 0;
> > -
> > - val = read_sanitised_ftr_reg(id);
> > + u64 val = IDREG(vcpu->kvm, id);
> >
> > switch (id) {
> > case SYS_ID_AA64PFR0_EL1:
> > @@ -1280,6 +1274,26 @@ static u64 read_id_reg(const struct kvm_vcpu
> > *vcpu, struct sys_reg_desc const *r
> > return val;
> > }
> >
> > +/* 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)
> > +{
> > + if (sysreg_visible_as_raz(vcpu, r))
> > + return 0;
> > +
> > + return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r));
> > +}
> > +
> > +/*
> > + * Return true if the register's (Op0, Op1, CRn, CRm, Op2) is
> > + * (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
> > + */
> > +static inline bool is_id_reg(u32 id)
> > +{
> > + return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
> > + sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 1 &&
> > + sys_reg_CRm(id) < 8);
> > +}
> > +
> > static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> > const struct sys_reg_desc *r)
> > {
> > @@ -2244,8 +2258,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 = kvm_arm_read_id_reg(vcpu, SYS_ID_AA64DFR0_EL1);
> > + u64 pfr = kvm_arm_read_id_reg(vcpu, SYS_ID_AA64PFR0_EL1);
>
> Does this change the behavior slightly as now within the kvm_arm_read_id_reg()
> the val will be further adjusted based on KVM/vCPU?
That's a good question. Although the actual behavior would be the same
no matter read idreg with read_sanitised_ftr_reg or
kvm_arm_read_id_reg, it is possible that the behavior would change
potentially in the future.
Since now every guest has its own idregs, for every guest, the idregs
should be read from kvm_arm_read_id_reg instead of
read_sanitised_ftr_reg.
The point is, for trap_dbgdidr, we should read AA64DFR0/AA64PFR0 from
host or the VM-scope?
>
> Thanks,
> Shameer
>
> > u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr,
> > ID_AA64PFR0_EL1_EL3_SHIFT);
> >
> > p->regval = ((((dfr >> ID_AA64DFR0_EL1_WRPs_SHIFT) & 0xf) <<
> > 28) |
> > @@ -3343,6 +3357,37 @@ int kvm_arm_copy_sys_reg_indices(struct
> > kvm_vcpu *vcpu, u64 __user *uindices)
> > return write_demux_regids(uindices);
> > }
> >
> > +/*
> > + * Set the guest's ID registers with ID_SANITISED() to the host's sanitized
> > value.
> > + */
> > +void kvm_arm_init_id_regs(struct kvm *kvm)
> > +{
> > + const struct sys_reg_desc *idreg;
> > + struct sys_reg_params params;
> > + u32 id;
> > +
> > + /* Find the first idreg (SYS_ID_PFR0_EL1) in sys_reg_descs. */
> > + id = SYS_ID_PFR0_EL1;
> > + params = encoding_to_params(id);
> > + idreg = find_reg(¶ms, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> > + if (WARN_ON(!idreg))
> > + return;
> > +
> > + /* Initialize all idregs */
> > + while (is_id_reg(id)) {
> > + /*
> > + * Some hidden ID registers which are not in arm64_ftr_regs[]
> > + * would cause warnings from read_sanitised_ftr_reg().
> > + * Skip those ID registers to avoid the warnings.
> > + */
> > + if (idreg->visibility != raz_visibility)
> > + IDREG(kvm, id) = read_sanitised_ftr_reg(id);
> > +
> > + idreg++;
> > + id = reg_to_encoding(idreg);
> > + }
> > +}
> > +
> > int __init kvm_sys_reg_table_init(void)
> > {
> > bool valid = true;
> > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> > index 6b11f2cc7146..eba10de2e7ae 100644
> > --- a/arch/arm64/kvm/sys_regs.h
> > +++ b/arch/arm64/kvm/sys_regs.h
> > @@ -27,6 +27,13 @@ struct sys_reg_params {
> > bool is_write;
> > };
> >
> > +#define encoding_to_params(reg) \
> > + ((struct sys_reg_params){ .Op0 = sys_reg_Op0(reg), \
> > + .Op1 = sys_reg_Op1(reg), \
> > + .CRn = sys_reg_CRn(reg), \
> > + .CRm = sys_reg_CRm(reg), \
> > + .Op2 = sys_reg_Op2(reg) })
> > +
> > #define esr_sys64_to_params(esr)
> > \
> > ((struct sys_reg_params){ .Op0 = ((esr) >> 20) & 3,
> > \
> > .Op1 = ((esr) >> 14) & 0x7, \
> > --
> > 2.40.1.606.ga4b1b128d6-goog
> >
>
Thanks,
Jing
More information about the linux-arm-kernel
mailing list