From: Jing Zhang <jingzhangos@google.com> To: Oliver Upton <oliver.upton@linux.dev> Cc: KVM <kvm@vger.kernel.org>, KVMARM <kvmarm@lists.linux.dev>, ARMLinux <linux-arm-kernel@lists.infradead.org>, Marc Zyngier <maz@kernel.org>, Oliver Upton <oupton@google.com>, Will Deacon <will@kernel.org>, Paolo Bonzini <pbonzini@redhat.com>, James Morse <james.morse@arm.com>, Alexandru Elisei <alexandru.elisei@arm.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Fuad Tabba <tabba@google.com>, Reiji Watanabe <reijiw@google.com>, Ricardo Koller <ricarkol@google.com>, Raghavendra Rao Ananta <rananta@google.com> Subject: Re: [PATCH v3 2/6] KVM: arm64: Save ID registers' sanitized value per guest Date: Thu, 9 Mar 2023 18:14:21 -0800 [thread overview] Message-ID: <CAAdAUtjL-4izpM6fOu7q6OtVav3MCOUED86z-=zX4V6xoz-XsQ@mail.gmail.com> (raw) In-Reply-To: <ZAe+XettpauZe84X@linux.dev> Hi Oliver, On Tue, Mar 7, 2023 at 2:44 PM Oliver Upton <oliver.upton@linux.dev> wrote: > > Hi Jing, > > On Tue, Feb 28, 2023 at 06:22:42AM +0000, Jing Zhang wrote: > > From: Reiji Watanabe <reijiw@google.com> > > > > 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. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > Co-developed-by: Jing Zhang <jingzhangos@google.com> > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 12 +++++++++ > > arch/arm64/kvm/arm.c | 1 + > > arch/arm64/kvm/id_regs.c | 44 ++++++++++++++++++++++++------- > > arch/arm64/kvm/sys_regs.c | 2 +- > > arch/arm64/kvm/sys_regs.h | 1 + > > 5 files changed, 50 insertions(+), 10 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index a1892a8f6032..5c1cec4efa37 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -245,6 +245,16 @@ struct kvm_arch { > > * the associated pKVM instance in the hypervisor. > > */ > > struct kvm_protected_vm pkvm; > > + > > + /* > > + * Save ID registers for the guest in id_regs[]. > > + * (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. > > + */ > > +#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.id_regs[IDREG_IDX(id)] > > I feel like the IDREG(...) macro just obfuscates what is otherwise a > simple array access. > Sure, will use array access. > > +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_with_encoding(vcpu, reg_to_encoding(r)); > > nit: you could probably drop the '_with_encoding' suffix, as I don't > believe there are any other flavors of accessors. > Yes, will do. > > +} > > + > > /* cpufeature ID register access trap handlers */ > > > > static bool access_id_reg(struct kvm_vcpu *vcpu, > > @@ -504,3 +505,28 @@ int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind) > > } > > return total; > > } > > + > > +/* > > + * Set the guest's ID registers that are defined in id_reg_descs[] > > + * with ID_SANITISED() to the host's sanitized value. > > + */ > > +void kvm_arm_set_default_id_regs(struct kvm *kvm) > > +{ > > + int i; > > + u32 id; > > + u64 val; > > + > > + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { > > + id = reg_to_encoding(&id_reg_descs[i]); > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > + /* Shouldn't happen */ > > + continue; > > Could you instead wire in a check to kvm_sys_reg_table_init() or do > something similar to it? Benefit of going that route is we outright > refuse to run KVM with such an egregious bug. > The check is already added in the first commit, which is kvm_arm_check_idreg_table() and improved progressively in later commits. > > + > > + if (id_reg_descs[i].visibility == raz_visibility) > > + /* Hidden or reserved ID register */ > > + continue; > > This sort of check works only for registers known to be RAZ at compile > time, but not for others that depend on runtime configuration. Is it > possible to reset the ID register values on the first call to > KVM_ARM_VCPU_INIT? > > The set of configured features is available at that point so you can > actually handle things like SVE and 32-bit ID registers correctly. > This function actually will be replaced with another init function as you suggested in a later commit (6/6), which would be called in kvm_arch_init_vm. > -- > Thanks, > Oliver
WARNING: multiple messages have this Message-ID (diff)
From: Jing Zhang <jingzhangos@google.com> To: Oliver Upton <oliver.upton@linux.dev> Cc: KVM <kvm@vger.kernel.org>, KVMARM <kvmarm@lists.linux.dev>, ARMLinux <linux-arm-kernel@lists.infradead.org>, Marc Zyngier <maz@kernel.org>, Oliver Upton <oupton@google.com>, Will Deacon <will@kernel.org>, Paolo Bonzini <pbonzini@redhat.com>, James Morse <james.morse@arm.com>, Alexandru Elisei <alexandru.elisei@arm.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Fuad Tabba <tabba@google.com>, Reiji Watanabe <reijiw@google.com>, Ricardo Koller <ricarkol@google.com>, Raghavendra Rao Ananta <rananta@google.com> Subject: Re: [PATCH v3 2/6] KVM: arm64: Save ID registers' sanitized value per guest Date: Thu, 9 Mar 2023 18:14:21 -0800 [thread overview] Message-ID: <CAAdAUtjL-4izpM6fOu7q6OtVav3MCOUED86z-=zX4V6xoz-XsQ@mail.gmail.com> (raw) In-Reply-To: <ZAe+XettpauZe84X@linux.dev> Hi Oliver, On Tue, Mar 7, 2023 at 2:44 PM Oliver Upton <oliver.upton@linux.dev> wrote: > > Hi Jing, > > On Tue, Feb 28, 2023 at 06:22:42AM +0000, Jing Zhang wrote: > > From: Reiji Watanabe <reijiw@google.com> > > > > 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. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > Co-developed-by: Jing Zhang <jingzhangos@google.com> > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 12 +++++++++ > > arch/arm64/kvm/arm.c | 1 + > > arch/arm64/kvm/id_regs.c | 44 ++++++++++++++++++++++++------- > > arch/arm64/kvm/sys_regs.c | 2 +- > > arch/arm64/kvm/sys_regs.h | 1 + > > 5 files changed, 50 insertions(+), 10 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index a1892a8f6032..5c1cec4efa37 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -245,6 +245,16 @@ struct kvm_arch { > > * the associated pKVM instance in the hypervisor. > > */ > > struct kvm_protected_vm pkvm; > > + > > + /* > > + * Save ID registers for the guest in id_regs[]. > > + * (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. > > + */ > > +#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.id_regs[IDREG_IDX(id)] > > I feel like the IDREG(...) macro just obfuscates what is otherwise a > simple array access. > Sure, will use array access. > > +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_with_encoding(vcpu, reg_to_encoding(r)); > > nit: you could probably drop the '_with_encoding' suffix, as I don't > believe there are any other flavors of accessors. > Yes, will do. > > +} > > + > > /* cpufeature ID register access trap handlers */ > > > > static bool access_id_reg(struct kvm_vcpu *vcpu, > > @@ -504,3 +505,28 @@ int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind) > > } > > return total; > > } > > + > > +/* > > + * Set the guest's ID registers that are defined in id_reg_descs[] > > + * with ID_SANITISED() to the host's sanitized value. > > + */ > > +void kvm_arm_set_default_id_regs(struct kvm *kvm) > > +{ > > + int i; > > + u32 id; > > + u64 val; > > + > > + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) { > > + id = reg_to_encoding(&id_reg_descs[i]); > > + if (WARN_ON_ONCE(!is_id_reg(id))) > > + /* Shouldn't happen */ > > + continue; > > Could you instead wire in a check to kvm_sys_reg_table_init() or do > something similar to it? Benefit of going that route is we outright > refuse to run KVM with such an egregious bug. > The check is already added in the first commit, which is kvm_arm_check_idreg_table() and improved progressively in later commits. > > + > > + if (id_reg_descs[i].visibility == raz_visibility) > > + /* Hidden or reserved ID register */ > > + continue; > > This sort of check works only for registers known to be RAZ at compile > time, but not for others that depend on runtime configuration. Is it > possible to reset the ID register values on the first call to > KVM_ARM_VCPU_INIT? > > The set of configured features is available at that point so you can > actually handle things like SVE and 32-bit ID registers correctly. > This function actually will be replaced with another init function as you suggested in a later commit (6/6), which would be called in kvm_arch_init_vm. > -- > Thanks, > Oliver _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-03-10 2:14 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-02-28 6:22 [PATCH v3 0/6] Support writable CPU ID registers from userspace Jing Zhang 2023-02-28 6:22 ` Jing Zhang 2023-02-28 6:22 ` [PATCH v3 1/6] KVM: arm64: Move CPU ID feature registers emulation into a separate file Jing Zhang 2023-02-28 6:22 ` Jing Zhang 2023-02-28 6:22 ` [PATCH v3 2/6] KVM: arm64: Save ID registers' sanitized value per guest Jing Zhang 2023-02-28 6:22 ` Jing Zhang 2023-03-07 22:44 ` Oliver Upton 2023-03-07 22:44 ` Oliver Upton 2023-03-10 2:14 ` Jing Zhang [this message] 2023-03-10 2:14 ` Jing Zhang 2023-02-28 6:22 ` [PATCH v3 3/6] KVM: arm64: Use per guest ID register for ID_AA64PFR0_EL1.[CSV2|CSV3] Jing Zhang 2023-02-28 6:22 ` Jing Zhang 2023-02-28 6:22 ` [PATCH v3 4/6] KVM: arm64: Use per guest ID register for ID_AA64DFR0_EL1.PMUVer Jing Zhang 2023-02-28 6:22 ` Jing Zhang 2023-03-08 16:42 ` Reiji Watanabe 2023-03-08 16:42 ` Reiji Watanabe 2023-03-10 2:38 ` Jing Zhang 2023-03-10 2:38 ` Jing Zhang 2023-03-13 4:13 ` Reiji Watanabe 2023-03-13 4:13 ` Reiji Watanabe 2023-03-14 4:36 ` Jing Zhang 2023-03-14 4:36 ` Jing Zhang 2023-03-14 5:14 ` Reiji Watanabe 2023-03-14 5:14 ` Reiji Watanabe 2023-03-14 5:35 ` Reiji Watanabe 2023-03-14 5:35 ` Reiji Watanabe 2023-03-14 17:25 ` Jing Zhang 2023-03-14 17:25 ` Jing Zhang 2023-03-14 6:09 ` Jing Zhang 2023-03-14 6:09 ` Jing Zhang 2023-02-28 6:22 ` [PATCH v3 5/6] KVM: arm64: Introduce ID register specific descriptor Jing Zhang 2023-02-28 6:22 ` Jing Zhang 2023-03-13 4:15 ` Reiji Watanabe 2023-03-13 4:15 ` Reiji Watanabe 2023-03-14 4:26 ` Jing Zhang 2023-03-14 4:26 ` Jing Zhang 2023-02-28 6:22 ` [PATCH v3 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3 Jing Zhang 2023-02-28 6:22 ` Jing Zhang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAAdAUtjL-4izpM6fOu7q6OtVav3MCOUED86z-=zX4V6xoz-XsQ@mail.gmail.com' \ --to=jingzhangos@google.com \ --cc=alexandru.elisei@arm.com \ --cc=james.morse@arm.com \ --cc=kvm@vger.kernel.org \ --cc=kvmarm@lists.linux.dev \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=maz@kernel.org \ --cc=oliver.upton@linux.dev \ --cc=oupton@google.com \ --cc=pbonzini@redhat.com \ --cc=rananta@google.com \ --cc=reijiw@google.com \ --cc=ricarkol@google.com \ --cc=suzuki.poulose@arm.com \ --cc=tabba@google.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.