From: Reiji Watanabe <reijiw@google.com>
To: Ricardo Koller <ricarkol@google.com>
Cc: Marc Zyngier <maz@kernel.org>,
kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
James Morse <james.morse@arm.com>,
Alexandru Elisei <alexandru.elisei@arm.com>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Will Deacon <will@kernel.org>, Andrew Jones <drjones@redhat.com>,
Peng Liang <liangpeng10@huawei.com>,
Peter Shier <pshier@google.com>, Oliver Upton <oupton@google.com>,
Jing Zhang <jingzhangos@google.com>,
Raghavendra Rao Anata <rananta@google.com>
Subject: Re: [RFC PATCH v4 02/26] KVM: arm64: Save ID registers' sanitized value per guest
Date: Thu, 27 Jan 2022 22:24:25 -0800 [thread overview]
Message-ID: <CAAeT=FzNSvzz-Ok0Ka95=kkdDGsAMmzf9xiRfD5gYCdvmEfifg@mail.gmail.com> (raw)
In-Reply-To: <YfDaiUbSkpi9/5YY@google.com>
Hi Ricardo,
On Tue, Jan 25, 2022 at 9:22 PM Ricardo Koller <ricarkol@google.com> wrote:
>
> On Wed, Jan 05, 2022 at 08:26:44PM -0800, Reiji Watanabe wrote:
> > 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).
> >
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > ---
> > arch/arm64/include/asm/kvm_host.h | 16 ++++++++
> > arch/arm64/kvm/arm.c | 1 +
> > arch/arm64/kvm/sys_regs.c | 62 ++++++++++++++++++++++---------
> > 3 files changed, 62 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 2a5f7f38006f..c789a0137f58 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -102,6 +102,17 @@ struct kvm_s2_mmu {
> > struct kvm_arch_memory_slot {
> > };
> >
> > +/*
> > + * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
> > + * where 0<=crm<8, 0<=op2<8.
>
> Is this observation based on this table?
>
> Table D12-2 System instruction encodings for non-Debug System register accesses
> in that case, it seems that the ID registers list might grow after
> crm=7, and as CRm has 4 bits, why not 16*8=128?
This is basically for registers that are already reserved as RAZ in the
table (sys_reg_descs[] has entries for the reserved ones as well).
Registers with crm > 7 are not reserved yet, and that will be expanded
later as needed later.
>
> > + */
> > +#define KVM_ARM_ID_REG_MAX_NUM 64
> > +#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id))
> > +#define is_id_reg(id) \
> > + (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 && \
> > + sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 0 && \
> > + sys_reg_CRm(id) < 8)
> > +
> > struct kvm_arch {
> > struct kvm_s2_mmu mmu;
> >
> > @@ -137,6 +148,9 @@ struct kvm_arch {
> >
> > /* Memory Tagging Extension enabled for the guest */
> > bool mte_enabled;
> > +
> > + /* ID registers for the guest. */
> > + u64 id_regs[KVM_ARM_ID_REG_MAX_NUM];
> > };
> >
> > struct kvm_vcpu_fault_info {
> > @@ -734,6 +748,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> > long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> > struct kvm_arm_copy_mte_tags *copy_tags);
> >
> > +void set_default_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 e4727dc771bf..5f497a0af254 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -156,6 +156,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> > kvm->arch.max_vcpus = kvm_arm_default_max_vcpus();
> >
> > set_default_spectre(kvm);
> > + set_default_id_regs(kvm);
> >
> > return ret;
> > out_free_stage2_pgd:
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index e3ec1a44f94d..80dc62f98ef0 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->kvm->arch.id_regs[IDREG_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)
> > {
> > @@ -1223,9 +1225,8 @@ 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.
> > + * For now, these registers are immutable for userspace, so for set_id_reg()
> > + * 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,
> > @@ -1237,7 +1238,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
> > return reg_to_user(uaddr, &val, id);
> > }
> >
> > -static int __set_id_reg(const struct kvm_vcpu *vcpu,
> > +static int __set_id_reg(struct kvm_vcpu *vcpu,
> > const struct sys_reg_desc *rd, void __user *uaddr,
> > bool raz)
> > {
> > @@ -1837,8 +1838,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) |
> > @@ -2850,3 +2851,30 @@ void kvm_sys_reg_table_init(void)
> > /* Clear all higher bits. */
> > cache_levels &= (1 << (i*3))-1;
> > }
> > +
> > +/*
> > + * Set the guest's ID registers that are defined in sys_reg_descs[]
> > + * with ID_SANITISED() to the host's sanitized value.
> > + */
> > +void set_default_id_regs(struct kvm *kvm)
> > +{
> > + int i;
> > + u32 id;
> > + const struct sys_reg_desc *rd;
> > + u64 val;
> > +
> > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
> > + rd = &sys_reg_descs[i];
> > + if (rd->access != access_id_reg)
> > + /* Not ID register, or hidden/reserved ID register */
> > + continue;
> > +
> > + id = reg_to_encoding(rd);
> > + if (WARN_ON_ONCE(!is_id_reg(id)))
> > + /* Shouldn't happen */
> > + continue;
> > +
> > + val = read_sanitised_ftr_reg(id);
>
> I'm a bit confused. Shouldn't the default+sanitized values already use
> arm64_ftr_bits_kvm (instead of arm64_ftr_regs)?
I'm not sure if I understand your question.
arm64_ftr_bits_kvm is used for feature support checkings when
userspace tries to modify a value of ID registers.
With this patch, KVM just saves the sanitized values in the kvm's
buffer, but userspace is still not allowed to modify values of ID
registers yet.
I hope it answers your question.
Thanks,
Reiji
>
> > + kvm->arch.id_regs[IDREG_IDX(id)] = val;
> > + }
> > +}
> > --
> > 2.34.1.448.ga2b2bfdf31-goog
> >
next prev parent reply other threads:[~2022-01-28 6:24 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-06 4:26 [RFC PATCH v4 00/26] KVM: arm64: Make CPU ID registers writable by userspace Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 01/26] KVM: arm64: Introduce a validation function for an ID register Reiji Watanabe
2022-01-07 7:12 ` Reiji Watanabe
2022-01-24 16:20 ` Fuad Tabba
2022-01-26 6:04 ` Reiji Watanabe
2022-02-01 14:13 ` Fuad Tabba
2022-02-02 6:46 ` Reiji Watanabe
2022-01-26 4:30 ` Ricardo Koller
2022-01-28 6:01 ` Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 02/26] KVM: arm64: Save ID registers' sanitized value per guest Reiji Watanabe
2022-01-24 16:21 ` Fuad Tabba
2022-02-09 2:26 ` Reiji Watanabe
2022-01-26 5:22 ` Ricardo Koller
2022-01-28 6:24 ` Reiji Watanabe [this message]
2022-01-28 19:27 ` Ricardo Koller
2022-01-29 5:52 ` Reiji Watanabe
2022-01-31 3:40 ` Ricardo Koller
2022-02-01 6:00 ` Reiji Watanabe
2022-02-01 18:38 ` Ricardo Koller
2022-02-03 6:31 ` Reiji Watanabe
2022-02-04 14:41 ` Ricardo Koller
2022-01-06 4:26 ` [RFC PATCH v4 03/26] KVM: arm64: Introduce struct id_reg_info Reiji Watanabe
2022-01-24 16:28 ` Fuad Tabba
2022-01-26 6:46 ` Reiji Watanabe
2022-02-01 14:13 ` Fuad Tabba
2022-01-06 4:26 ` [RFC PATCH v4 04/26] KVM: arm64: Make ID_AA64PFR0_EL1 writable Reiji Watanabe
2022-01-24 16:51 ` Fuad Tabba
2022-01-27 4:01 ` Reiji Watanabe
2022-02-01 14:14 ` Fuad Tabba
2022-02-10 5:33 ` Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 05/26] KVM: arm64: Make ID_AA64PFR1_EL1 writable Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 06/26] KVM: arm64: Make ID_AA64ISAR0_EL1 writable Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 07/26] KVM: arm64: Make ID_AA64ISAR1_EL1 writable Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 08/26] KVM: arm64: Make ID_AA64MMFR0_EL1 writable Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 09/26] KVM: arm64: Hide IMPLEMENTATION DEFINED PMU support for the guest Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 10/26] KVM: arm64: Make ID_AA64DFR0_EL1 writable Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 11/26] KVM: arm64: Make ID_DFR0_EL1 writable Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 12/26] KVM: arm64: Make MVFR1_EL1 writable Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 13/26] KVM: arm64: Make ID registers without id_reg_info writable Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 14/26] KVM: arm64: Add consistency checking for frac fields of ID registers Reiji Watanabe
2022-01-24 17:00 ` Fuad Tabba
2022-01-27 5:03 ` Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 15/26] KVM: arm64: Introduce KVM_CAP_ARM_ID_REG_CONFIGURABLE capability Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 16/26] KVM: arm64: Add kunit test for ID register validation Reiji Watanabe
2022-01-06 4:26 ` [RFC PATCH v4 17/26] KVM: arm64: Use vcpu->arch cptr_el2 to track value of cptr_el2 for VHE Reiji Watanabe
2022-01-06 4:27 ` [RFC PATCH v4 18/26] KVM: arm64: Use vcpu->arch.mdcr_el2 to track value of mdcr_el2 Reiji Watanabe
2022-01-06 4:27 ` [RFC PATCH v4 19/26] KVM: arm64: Introduce framework to trap disabled features Reiji Watanabe
2022-01-06 4:27 ` [RFC PATCH v4 20/26] KVM: arm64: Trap disabled features of ID_AA64PFR0_EL1 Reiji Watanabe
2022-01-24 17:16 ` Fuad Tabba
2022-01-27 7:19 ` Reiji Watanabe
2022-02-01 14:14 ` Fuad Tabba
2022-02-10 4:15 ` Reiji Watanabe
2022-01-06 4:27 ` [RFC PATCH v4 21/26] KVM: arm64: Trap disabled features of ID_AA64PFR1_EL1 Reiji Watanabe
2022-01-06 4:27 ` [RFC PATCH v4 22/26] KVM: arm64: Trap disabled features of ID_AA64DFR0_EL1 Reiji Watanabe
2022-01-24 17:19 ` Fuad Tabba
2022-01-28 5:40 ` Reiji Watanabe
2022-01-06 4:27 ` [RFC PATCH v4 23/26] KVM: arm64: Trap disabled features of ID_AA64MMFR1_EL1 Reiji Watanabe
2022-01-24 17:37 ` Fuad Tabba
2022-01-28 5:43 ` Reiji Watanabe
2022-02-09 4:51 ` Reiji Watanabe
2022-01-06 4:27 ` [RFC PATCH v4 24/26] KVM: arm64: Trap disabled features of ID_AA64ISAR1_EL1 Reiji Watanabe
2022-01-06 4:27 ` [RFC PATCH v4 25/26] KVM: arm64: Add kunit test for trap initialization Reiji Watanabe
2022-01-06 4:27 ` [RFC PATCH v4 26/26] KVM: arm64: selftests: Introduce id_reg_test Reiji Watanabe
2022-01-18 4:24 ` [RFC PATCH v4 00/26] KVM: arm64: Make CPU ID registers writable by userspace Reiji Watanabe
2022-01-24 16:18 ` Fuad Tabba
2022-01-25 6:31 ` Reiji Watanabe
2022-02-01 14:12 ` Fuad Tabba
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='CAAeT=FzNSvzz-Ok0Ka95=kkdDGsAMmzf9xiRfD5gYCdvmEfifg@mail.gmail.com' \
--to=reijiw@google.com \
--cc=alexandru.elisei@arm.com \
--cc=drjones@redhat.com \
--cc=james.morse@arm.com \
--cc=jingzhangos@google.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=liangpeng10@huawei.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=maz@kernel.org \
--cc=oupton@google.com \
--cc=pbonzini@redhat.com \
--cc=pshier@google.com \
--cc=rananta@google.com \
--cc=ricarkol@google.com \
--cc=suzuki.poulose@arm.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).