From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 254D9C7619A for ; Mon, 27 Mar 2023 10:15:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233538AbjC0KPL (ORCPT ); Mon, 27 Mar 2023 06:15:11 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48848 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233506AbjC0KPK (ORCPT ); Mon, 27 Mar 2023 06:15:10 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 868D44C1B for ; Mon, 27 Mar 2023 03:15:08 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 361E6B81072 for ; Mon, 27 Mar 2023 10:15:07 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DF152C433EF; Mon, 27 Mar 2023 10:15:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1679912105; bh=13hrWAgfZQrYOohFgk5p4rU9Mu8DpOU4EbMcrSeixb4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Ag12dtK2/YfA79qJcvu9s3Cpeb74e+71dT+1Jzo3ZDSmCT7i+rW+9WigNJqHcFGW9 msRe8r9aFkfKnBEDUMNhBo43XmzNyRKm9+GGSL9gdLzpK0iDIk2bEEahC5HyR9lOLY aS5lkrzVPs2HLniU4Ni8znXQSykUjTVob7S0S6goVAKPL6UjaHrWADCUwvF4ZiDl35 W6AK4d1GheL2It7IQvbvNAKkFioKlX6HtJRwrjdPw1T/Qc8rKPWINBQ1kwJ78UPY2w K7eS19dlMpFislOOhATPAixGk3NOun03gRkszkvQSmezZIHoXIZ48H7w2tKMZvdCvP lRhcXW7R3qBfg== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1pgjsV-003Pfd-IH; Mon, 27 Mar 2023 11:15:03 +0100 Date: Mon, 27 Mar 2023 11:15:03 +0100 Message-ID: <861qlaxzyw.wl-maz@kernel.org> From: Marc Zyngier To: Jing Zhang Cc: KVM , KVMARM , ARMLinux , Oliver Upton , Will Deacon , Paolo Bonzini , James Morse , Alexandru Elisei , Suzuki K Poulose , Fuad Tabba , Reiji Watanabe , Ricardo Koller , Raghavendra Rao Ananta Subject: Re: [PATCH v4 2/6] KVM: arm64: Save ID registers' sanitized value per guest In-Reply-To: <20230317050637.766317-3-jingzhangos@google.com> References: <20230317050637.766317-1-jingzhangos@google.com> <20230317050637.766317-3-jingzhangos@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: jingzhangos@google.com, kvm@vger.kernel.org, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, oupton@google.com, will@kernel.org, pbonzini@redhat.com, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, tabba@google.com, reijiw@google.com, ricarkol@google.com, rananta@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Fri, 17 Mar 2023 05:06:33 +0000, Jing Zhang wrote: > > From: Reiji Watanabe > > 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 > Co-developed-by: Jing Zhang > Signed-off-by: Jing Zhang > --- > arch/arm64/include/asm/kvm_host.h | 11 ++++++++ > 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, 49 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index a1892a8f6032..fb6b50b1f111 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -245,6 +245,15 @@ 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)) > + u64 id_regs[KVM_ARM_ID_REG_NUM]; Place these registers in their own structure, and place this structure *before* the pvm structure. Document what guards these registers when updated (my hunch is that this should rely on Oliver's locking fixes if the update comes from a vcpu). > }; > > struct kvm_vcpu_fault_info { > @@ -1005,6 +1014,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 kvm_arm_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 3bd732eaf087..4579c878ab30 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > set_default_spectre(kvm); > kvm_arm_init_hypercalls(kvm); > + kvm_arm_set_default_id_regs(kvm); > > /* > * Initialise the default PMUver before there is a chance to > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c > index 08b738852955..e393b5730557 100644 > --- a/arch/arm64/kvm/id_regs.c > +++ b/arch/arm64/kvm/id_regs.c > @@ -52,16 +52,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) > +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 = vcpu->kvm->arch.id_regs[IDREG_IDX(id)]; > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r > return val; > } > > +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)); > +} > + > /* 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; > + > + if (id_reg_descs[i].visibility == raz_visibility) > + /* Hidden or reserved ID register */ > + continue; Relying on function pointer comparison is really fragile. If I wrap raz_visibility() in another function, this won't catch it. It also doesn't bode well with your 'inline' definition of this function. More importantly, why do we care about checking for visibility at all? We can happily populate the array and rely on the runtime visibility. > + > + val = read_sanitised_ftr_reg(id); > + kvm->arch.id_regs[IDREG_IDX(id)] = val; > + } > +} Thanks, M. -- Without deviation from the norm, progress is not possible. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 53D70C7619A for ; Mon, 27 Mar 2023 10:16:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=cTjhXAq794+RC3vRJ/l1o2pHTK3H9pOMes4DOpQ+MOo=; b=KC+d/tH316LUEN Xs1gVbqWUx13ZG5Qf+MnWAXUev8IKFGJN0t1RcWyAJjOBEWpLM8qOSpf5pWehuK4kg4qPUcNmIEwJ FX+SiQfP4qo8J+HHqp9JccoCrz11PnHHAdxoUfYXdCZFcsN5PSOoZ/6ZDVPCuLF6Gql9L/tVqVTlp 3569DIOAsncn1v40AsffvP6DzPQeKDSdcXKFGQhecngFK/WnCrROw4m5GdgPOSykOeyBHgYdCm/sh KV1jfvSU1yc+1vAKx2crjH703fAId896dUu38E9aWtysJeZuYfMdYoUP8KGp6Q0mBV2RnMJhqzhNh ZBQr1xF5Wdgh2xvjPAcw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pgjsb-00Abfo-2s; Mon, 27 Mar 2023 10:15:09 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pgjsZ-00Abf5-0J for linux-arm-kernel@lists.infradead.org; Mon, 27 Mar 2023 10:15:08 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 7893161184; Mon, 27 Mar 2023 10:15:06 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DF152C433EF; Mon, 27 Mar 2023 10:15:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1679912105; bh=13hrWAgfZQrYOohFgk5p4rU9Mu8DpOU4EbMcrSeixb4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Ag12dtK2/YfA79qJcvu9s3Cpeb74e+71dT+1Jzo3ZDSmCT7i+rW+9WigNJqHcFGW9 msRe8r9aFkfKnBEDUMNhBo43XmzNyRKm9+GGSL9gdLzpK0iDIk2bEEahC5HyR9lOLY aS5lkrzVPs2HLniU4Ni8znXQSykUjTVob7S0S6goVAKPL6UjaHrWADCUwvF4ZiDl35 W6AK4d1GheL2It7IQvbvNAKkFioKlX6HtJRwrjdPw1T/Qc8rKPWINBQ1kwJ78UPY2w K7eS19dlMpFislOOhATPAixGk3NOun03gRkszkvQSmezZIHoXIZ48H7w2tKMZvdCvP lRhcXW7R3qBfg== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1pgjsV-003Pfd-IH; Mon, 27 Mar 2023 11:15:03 +0100 Date: Mon, 27 Mar 2023 11:15:03 +0100 Message-ID: <861qlaxzyw.wl-maz@kernel.org> From: Marc Zyngier To: Jing Zhang Cc: KVM , KVMARM , ARMLinux , Oliver Upton , Will Deacon , Paolo Bonzini , James Morse , Alexandru Elisei , Suzuki K Poulose , Fuad Tabba , Reiji Watanabe , Ricardo Koller , Raghavendra Rao Ananta Subject: Re: [PATCH v4 2/6] KVM: arm64: Save ID registers' sanitized value per guest In-Reply-To: <20230317050637.766317-3-jingzhangos@google.com> References: <20230317050637.766317-1-jingzhangos@google.com> <20230317050637.766317-3-jingzhangos@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: jingzhangos@google.com, kvm@vger.kernel.org, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, oupton@google.com, will@kernel.org, pbonzini@redhat.com, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, tabba@google.com, reijiw@google.com, ricarkol@google.com, rananta@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230327_031507_214167_C5E12BC6 X-CRM114-Status: GOOD ( 35.98 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, 17 Mar 2023 05:06:33 +0000, Jing Zhang wrote: > > From: Reiji Watanabe > > 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 > Co-developed-by: Jing Zhang > Signed-off-by: Jing Zhang > --- > arch/arm64/include/asm/kvm_host.h | 11 ++++++++ > 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, 49 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index a1892a8f6032..fb6b50b1f111 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -245,6 +245,15 @@ 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)) > + u64 id_regs[KVM_ARM_ID_REG_NUM]; Place these registers in their own structure, and place this structure *before* the pvm structure. Document what guards these registers when updated (my hunch is that this should rely on Oliver's locking fixes if the update comes from a vcpu). > }; > > struct kvm_vcpu_fault_info { > @@ -1005,6 +1014,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 kvm_arm_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 3bd732eaf087..4579c878ab30 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > set_default_spectre(kvm); > kvm_arm_init_hypercalls(kvm); > + kvm_arm_set_default_id_regs(kvm); > > /* > * Initialise the default PMUver before there is a chance to > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c > index 08b738852955..e393b5730557 100644 > --- a/arch/arm64/kvm/id_regs.c > +++ b/arch/arm64/kvm/id_regs.c > @@ -52,16 +52,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) > +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 = vcpu->kvm->arch.id_regs[IDREG_IDX(id)]; > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r > return val; > } > > +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)); > +} > + > /* 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; > + > + if (id_reg_descs[i].visibility == raz_visibility) > + /* Hidden or reserved ID register */ > + continue; Relying on function pointer comparison is really fragile. If I wrap raz_visibility() in another function, this won't catch it. It also doesn't bode well with your 'inline' definition of this function. More importantly, why do we care about checking for visibility at all? We can happily populate the array and rely on the runtime visibility. > + > + val = read_sanitised_ftr_reg(id); > + kvm->arch.id_regs[IDREG_IDX(id)] = val; > + } > +} Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel