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 D24FCC433EF for ; Sun, 21 Nov 2021 12:37:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238260AbhKUMki (ORCPT ); Sun, 21 Nov 2021 07:40:38 -0500 Received: from mail.kernel.org ([198.145.29.99]:44266 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238251AbhKUMkh (ORCPT ); Sun, 21 Nov 2021 07:40:37 -0500 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 72CE460555; Sun, 21 Nov 2021 12:37:32 +0000 (UTC) Received: from ip-185-104-136-29.ptr.icomera.net ([185.104.136.29] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mom66-006sRS-BV; Sun, 21 Nov 2021 12:37:30 +0000 Date: Sun, 21 Nov 2021 12:37:30 +0000 Message-ID: <87h7c5sn05.wl-maz@kernel.org> From: Marc Zyngier To: Reiji Watanabe Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, James Morse , Alexandru Elisei , Suzuki K Poulose , Paolo Bonzini , Will Deacon , Andrew Jones , Peng Liang , Peter Shier , Ricardo Koller , Oliver Upton , Jing Zhang , Raghavendra Rao Anata Subject: Re: [RFC PATCH v3 04/29] KVM: arm64: Make ID_AA64PFR0_EL1 writable In-Reply-To: <20211117064359.2362060-5-reijiw@google.com> References: <20211117064359.2362060-1-reijiw@google.com> <20211117064359.2362060-5-reijiw@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/27.1 (x86_64-pc-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.104.136.29 X-SA-Exim-Rcpt-To: reijiw@google.com, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, pbonzini@redhat.com, will@kernel.org, drjones@redhat.com, liangpeng10@huawei.com, pshier@google.com, ricarkol@google.com, oupton@google.com, jingzhangos@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 Wed, 17 Nov 2021 06:43:34 +0000, Reiji Watanabe wrote: > > This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by > userspace. > > The CSV2/CSV3 fields of the register were already writable and values > that were written for them affected all vCPUs before. Now they only > affect the vCPU. > Return an error if userspace tries to set SVE/GIC field of the register > to a value that conflicts with SVE/GIC configuration for the guest. > SIMD/FP/SVE fields of the requested value are validated according to > Arm ARM. > > Signed-off-by: Reiji Watanabe > --- > arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++-------------- > 1 file changed, 103 insertions(+), 56 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 1552cd5581b7..35400869067a 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > id_reg->init(id_reg); > } > > +#define kvm_has_gic3(kvm) \ > + (irqchip_in_kernel(kvm) && \ > + (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) > + > +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > + const struct id_reg_info *id_reg, u64 val) > +{ > + int fp, simd; > + bool vcpu_has_sve = vcpu_has_sve(vcpu); > + bool pfr0_has_sve = id_aa64pfr0_sve(val); > + int gic; > + > + simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT); > + fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT); > + if (simd != fp) > + return -EINVAL; > + > + /* fp must be supported when sve is supported */ > + if (pfr0_has_sve && (fp < 0)) > + return -EINVAL; > + > + /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */ > + if (vcpu_has_sve ^ pfr0_has_sve) > + return -EPERM; > + > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > + if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm)) > + return -EPERM; > + > + return 0; > +} > + > +static void init_id_aa64pfr0_el1_info(struct id_reg_info *id_reg) > +{ > + u64 limit = id_reg->vcpu_limit_val; > + unsigned int gic; > + > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > + if (!system_supports_sve()) > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > + > + /* > + * The default is to expose CSV2 == 1 and CSV3 == 1 if the HW > + * isn't affected. Userspace can override this as long as it > + * doesn't promise the impossible. > + */ > + limit &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2) | > + ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3)); > + > + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), 1); > + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), 1); > + > + gic = cpuid_feature_extract_unsigned_field(limit, ID_AA64PFR0_GIC_SHIFT); > + if (gic > 1) { > + /* Limit to GICv3.0/4.0 */ > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > + } > + id_reg->vcpu_limit_val = limit; > +} > + > +static u64 get_reset_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > + const struct id_reg_info *idr) > +{ > + u64 val = idr->vcpu_limit_val; > + > + if (!vcpu_has_sve(vcpu)) > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > + > + if (!kvm_has_gic3(vcpu->kvm)) > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); No. As I said in a previous email, this breaks migration, and advertising a GICv3 CPU interface doesn't mean it is usable (the guest OS must check that it can actually enable ICC_SRE_EL1.SRE -- see what the Linux GICv3 driver does for an example). > + > + return val; > +} > + > +static struct id_reg_info id_aa64pfr0_el1_info = { > + .sys_reg = SYS_ID_AA64PFR0_EL1, > + .ftr_check_types = S_FCT(ID_AA64PFR0_ASIMD_SHIFT, FCT_LOWER_SAFE) | > + S_FCT(ID_AA64PFR0_FP_SHIFT, FCT_LOWER_SAFE), > + .init = init_id_aa64pfr0_el1_info, > + .validate = validate_id_aa64pfr0_el1, > + .get_reset_val = get_reset_id_aa64pfr0_el1, > +}; > + > /* > * An ID register that needs special handling to control the value for the > * guest must have its own id_reg_info in id_reg_info_table. > @@ -409,7 +495,9 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > * validation, etc.) > */ > #define GET_ID_REG_INFO(id) (id_reg_info_table[IDREG_IDX(id)]) > -static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = {}; > +static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = { > + [IDREG_IDX(SYS_ID_AA64PFR0_EL1)] = &id_aa64pfr0_el1_info, > +}; > > static int validate_id_reg(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, u64 val) > @@ -1239,20 +1327,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > { > u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > + u64 lim, gic, gic_lim; > + const struct id_reg_info *id_reg; > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > - if (!vcpu_has_sve(vcpu)) > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3); > - if (irqchip_in_kernel(vcpu->kvm) && > - vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > + if (kvm_has_gic3(vcpu->kvm) && (gic == 0)) { > + /* > + * This is a case where userspace configured gic3 after > + * the vcpu was created, and then it didn't set > + * ID_AA64PFR0_EL1. > + */ Shouldn't that be done at the point where a GICv3 is created, rather than after the fact? 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 mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3A7D9C433EF for ; Sun, 21 Nov 2021 12:37:38 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id EA43E4B1CC; Sun, 21 Nov 2021 07:37:37 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id N2Rh3VRs6-Kz; Sun, 21 Nov 2021 07:37:35 -0500 (EST) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id DF3114B1AD; Sun, 21 Nov 2021 07:37:35 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id C44E84B1A5 for ; Sun, 21 Nov 2021 07:37:34 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id UTMCaJUB82rt for ; Sun, 21 Nov 2021 07:37:33 -0500 (EST) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 5342F4B14D for ; Sun, 21 Nov 2021 07:37:33 -0500 (EST) Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 72CE460555; Sun, 21 Nov 2021 12:37:32 +0000 (UTC) Received: from ip-185-104-136-29.ptr.icomera.net ([185.104.136.29] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mom66-006sRS-BV; Sun, 21 Nov 2021 12:37:30 +0000 Date: Sun, 21 Nov 2021 12:37:30 +0000 Message-ID: <87h7c5sn05.wl-maz@kernel.org> From: Marc Zyngier To: Reiji Watanabe Subject: Re: [RFC PATCH v3 04/29] KVM: arm64: Make ID_AA64PFR0_EL1 writable In-Reply-To: <20211117064359.2362060-5-reijiw@google.com> References: <20211117064359.2362060-1-reijiw@google.com> <20211117064359.2362060-5-reijiw@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/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.104.136.29 X-SA-Exim-Rcpt-To: reijiw@google.com, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, pbonzini@redhat.com, will@kernel.org, drjones@redhat.com, liangpeng10@huawei.com, pshier@google.com, ricarkol@google.com, oupton@google.com, jingzhangos@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 Cc: kvm@vger.kernel.org, Will Deacon , Peter Shier , Paolo Bonzini , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On Wed, 17 Nov 2021 06:43:34 +0000, Reiji Watanabe wrote: > > This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by > userspace. > > The CSV2/CSV3 fields of the register were already writable and values > that were written for them affected all vCPUs before. Now they only > affect the vCPU. > Return an error if userspace tries to set SVE/GIC field of the register > to a value that conflicts with SVE/GIC configuration for the guest. > SIMD/FP/SVE fields of the requested value are validated according to > Arm ARM. > > Signed-off-by: Reiji Watanabe > --- > arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++-------------- > 1 file changed, 103 insertions(+), 56 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 1552cd5581b7..35400869067a 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > id_reg->init(id_reg); > } > > +#define kvm_has_gic3(kvm) \ > + (irqchip_in_kernel(kvm) && \ > + (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) > + > +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > + const struct id_reg_info *id_reg, u64 val) > +{ > + int fp, simd; > + bool vcpu_has_sve = vcpu_has_sve(vcpu); > + bool pfr0_has_sve = id_aa64pfr0_sve(val); > + int gic; > + > + simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT); > + fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT); > + if (simd != fp) > + return -EINVAL; > + > + /* fp must be supported when sve is supported */ > + if (pfr0_has_sve && (fp < 0)) > + return -EINVAL; > + > + /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */ > + if (vcpu_has_sve ^ pfr0_has_sve) > + return -EPERM; > + > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > + if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm)) > + return -EPERM; > + > + return 0; > +} > + > +static void init_id_aa64pfr0_el1_info(struct id_reg_info *id_reg) > +{ > + u64 limit = id_reg->vcpu_limit_val; > + unsigned int gic; > + > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > + if (!system_supports_sve()) > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > + > + /* > + * The default is to expose CSV2 == 1 and CSV3 == 1 if the HW > + * isn't affected. Userspace can override this as long as it > + * doesn't promise the impossible. > + */ > + limit &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2) | > + ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3)); > + > + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), 1); > + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), 1); > + > + gic = cpuid_feature_extract_unsigned_field(limit, ID_AA64PFR0_GIC_SHIFT); > + if (gic > 1) { > + /* Limit to GICv3.0/4.0 */ > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > + } > + id_reg->vcpu_limit_val = limit; > +} > + > +static u64 get_reset_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > + const struct id_reg_info *idr) > +{ > + u64 val = idr->vcpu_limit_val; > + > + if (!vcpu_has_sve(vcpu)) > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > + > + if (!kvm_has_gic3(vcpu->kvm)) > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); No. As I said in a previous email, this breaks migration, and advertising a GICv3 CPU interface doesn't mean it is usable (the guest OS must check that it can actually enable ICC_SRE_EL1.SRE -- see what the Linux GICv3 driver does for an example). > + > + return val; > +} > + > +static struct id_reg_info id_aa64pfr0_el1_info = { > + .sys_reg = SYS_ID_AA64PFR0_EL1, > + .ftr_check_types = S_FCT(ID_AA64PFR0_ASIMD_SHIFT, FCT_LOWER_SAFE) | > + S_FCT(ID_AA64PFR0_FP_SHIFT, FCT_LOWER_SAFE), > + .init = init_id_aa64pfr0_el1_info, > + .validate = validate_id_aa64pfr0_el1, > + .get_reset_val = get_reset_id_aa64pfr0_el1, > +}; > + > /* > * An ID register that needs special handling to control the value for the > * guest must have its own id_reg_info in id_reg_info_table. > @@ -409,7 +495,9 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > * validation, etc.) > */ > #define GET_ID_REG_INFO(id) (id_reg_info_table[IDREG_IDX(id)]) > -static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = {}; > +static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = { > + [IDREG_IDX(SYS_ID_AA64PFR0_EL1)] = &id_aa64pfr0_el1_info, > +}; > > static int validate_id_reg(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, u64 val) > @@ -1239,20 +1327,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > { > u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > + u64 lim, gic, gic_lim; > + const struct id_reg_info *id_reg; > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > - if (!vcpu_has_sve(vcpu)) > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3); > - if (irqchip_in_kernel(vcpu->kvm) && > - vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > + if (kvm_has_gic3(vcpu->kvm) && (gic == 0)) { > + /* > + * This is a case where userspace configured gic3 after > + * the vcpu was created, and then it didn't set > + * ID_AA64PFR0_EL1. > + */ Shouldn't that be done at the point where a GICv3 is created, rather than after the fact? Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm 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 D4919C433F5 for ; Sun, 21 Nov 2021 12:38:57 +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=S8x5SGqacclyciTWkeX+FeBqzLjVulPNrq+KfDxITpc=; b=FXsUthIRby6adi Mb0xUamobEQkBO8x3Fn3l/0SamlSw3ND5tFTCltwWEjIcvqXwEIZ5v4SYUHlnPFXBCS8tW+Sx9+Vr ho5JTE43uOR4SRZLBjJ106S9RG55vcVF9W3T9r7gN9MaiAQfJ2ck8sdlPIMXDUBgR8DjCvcDUTDZ+ s1s4++4xO276V5qSUXgS6uVKLMDmOtw1SkHpAjLgX1kdvoiDIsBht7FTV9leonC/OdvPIZBVR+t54 6HOIPBp5lfhDJ5QdZnp/rrPzmxOEYS3ZmPcG3+yJ2dy3Cfxz6vM3ld0+UC8zUj2TAS28Bmno+B7Xl ZQmAcOcUpwgGXf29WI7g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mom6I-00DZlb-VU; Sun, 21 Nov 2021 12:37:43 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mom68-00DZiV-Si for linux-arm-kernel@lists.infradead.org; Sun, 21 Nov 2021 12:37:34 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 72CE460555; Sun, 21 Nov 2021 12:37:32 +0000 (UTC) Received: from ip-185-104-136-29.ptr.icomera.net ([185.104.136.29] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mom66-006sRS-BV; Sun, 21 Nov 2021 12:37:30 +0000 Date: Sun, 21 Nov 2021 12:37:30 +0000 Message-ID: <87h7c5sn05.wl-maz@kernel.org> From: Marc Zyngier To: Reiji Watanabe Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, James Morse , Alexandru Elisei , Suzuki K Poulose , Paolo Bonzini , Will Deacon , Andrew Jones , Peng Liang , Peter Shier , Ricardo Koller , Oliver Upton , Jing Zhang , Raghavendra Rao Anata Subject: Re: [RFC PATCH v3 04/29] KVM: arm64: Make ID_AA64PFR0_EL1 writable In-Reply-To: <20211117064359.2362060-5-reijiw@google.com> References: <20211117064359.2362060-1-reijiw@google.com> <20211117064359.2362060-5-reijiw@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/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.104.136.29 X-SA-Exim-Rcpt-To: reijiw@google.com, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, pbonzini@redhat.com, will@kernel.org, drjones@redhat.com, liangpeng10@huawei.com, pshier@google.com, ricarkol@google.com, oupton@google.com, jingzhangos@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-20211121_043733_005409_B69111B3 X-CRM114-Status: GOOD ( 37.12 ) 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 Wed, 17 Nov 2021 06:43:34 +0000, Reiji Watanabe wrote: > > This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by > userspace. > > The CSV2/CSV3 fields of the register were already writable and values > that were written for them affected all vCPUs before. Now they only > affect the vCPU. > Return an error if userspace tries to set SVE/GIC field of the register > to a value that conflicts with SVE/GIC configuration for the guest. > SIMD/FP/SVE fields of the requested value are validated according to > Arm ARM. > > Signed-off-by: Reiji Watanabe > --- > arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++-------------- > 1 file changed, 103 insertions(+), 56 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 1552cd5581b7..35400869067a 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > id_reg->init(id_reg); > } > > +#define kvm_has_gic3(kvm) \ > + (irqchip_in_kernel(kvm) && \ > + (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) > + > +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > + const struct id_reg_info *id_reg, u64 val) > +{ > + int fp, simd; > + bool vcpu_has_sve = vcpu_has_sve(vcpu); > + bool pfr0_has_sve = id_aa64pfr0_sve(val); > + int gic; > + > + simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT); > + fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT); > + if (simd != fp) > + return -EINVAL; > + > + /* fp must be supported when sve is supported */ > + if (pfr0_has_sve && (fp < 0)) > + return -EINVAL; > + > + /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */ > + if (vcpu_has_sve ^ pfr0_has_sve) > + return -EPERM; > + > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > + if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm)) > + return -EPERM; > + > + return 0; > +} > + > +static void init_id_aa64pfr0_el1_info(struct id_reg_info *id_reg) > +{ > + u64 limit = id_reg->vcpu_limit_val; > + unsigned int gic; > + > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > + if (!system_supports_sve()) > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > + > + /* > + * The default is to expose CSV2 == 1 and CSV3 == 1 if the HW > + * isn't affected. Userspace can override this as long as it > + * doesn't promise the impossible. > + */ > + limit &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2) | > + ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3)); > + > + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), 1); > + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), 1); > + > + gic = cpuid_feature_extract_unsigned_field(limit, ID_AA64PFR0_GIC_SHIFT); > + if (gic > 1) { > + /* Limit to GICv3.0/4.0 */ > + limit &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > + limit |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > + } > + id_reg->vcpu_limit_val = limit; > +} > + > +static u64 get_reset_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > + const struct id_reg_info *idr) > +{ > + u64 val = idr->vcpu_limit_val; > + > + if (!vcpu_has_sve(vcpu)) > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > + > + if (!kvm_has_gic3(vcpu->kvm)) > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); No. As I said in a previous email, this breaks migration, and advertising a GICv3 CPU interface doesn't mean it is usable (the guest OS must check that it can actually enable ICC_SRE_EL1.SRE -- see what the Linux GICv3 driver does for an example). > + > + return val; > +} > + > +static struct id_reg_info id_aa64pfr0_el1_info = { > + .sys_reg = SYS_ID_AA64PFR0_EL1, > + .ftr_check_types = S_FCT(ID_AA64PFR0_ASIMD_SHIFT, FCT_LOWER_SAFE) | > + S_FCT(ID_AA64PFR0_FP_SHIFT, FCT_LOWER_SAFE), > + .init = init_id_aa64pfr0_el1_info, > + .validate = validate_id_aa64pfr0_el1, > + .get_reset_val = get_reset_id_aa64pfr0_el1, > +}; > + > /* > * An ID register that needs special handling to control the value for the > * guest must have its own id_reg_info in id_reg_info_table. > @@ -409,7 +495,9 @@ static void id_reg_info_init(struct id_reg_info *id_reg) > * validation, etc.) > */ > #define GET_ID_REG_INFO(id) (id_reg_info_table[IDREG_IDX(id)]) > -static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = {}; > +static struct id_reg_info *id_reg_info_table[KVM_ARM_ID_REG_MAX_NUM] = { > + [IDREG_IDX(SYS_ID_AA64PFR0_EL1)] = &id_aa64pfr0_el1_info, > +}; > > static int validate_id_reg(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, u64 val) > @@ -1239,20 +1327,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > static u64 __read_id_reg(const struct kvm_vcpu *vcpu, u32 id) > { > u64 val = __vcpu_sys_reg(vcpu, IDREG_SYS_IDX(id)); > + u64 lim, gic, gic_lim; > + const struct id_reg_info *id_reg; > > switch (id) { > case SYS_ID_AA64PFR0_EL1: > - if (!vcpu_has_sve(vcpu)) > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_SVE); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_AMU); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2); > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3); > - if (irqchip_in_kernel(vcpu->kvm) && > - vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { > - val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > - val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > + gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT); > + if (kvm_has_gic3(vcpu->kvm) && (gic == 0)) { > + /* > + * This is a case where userspace configured gic3 after > + * the vcpu was created, and then it didn't set > + * ID_AA64PFR0_EL1. > + */ Shouldn't that be done at the point where a GICv3 is created, rather than after the fact? 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