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 4A338C433FE for ; Fri, 4 Nov 2022 07:00:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231217AbiKDHAo (ORCPT ); Fri, 4 Nov 2022 03:00:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44328 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229952AbiKDHAl (ORCPT ); Fri, 4 Nov 2022 03:00:41 -0400 Received: from mail-pl1-x632.google.com (mail-pl1-x632.google.com [IPv6:2607:f8b0:4864:20::632]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EFBE425EA4 for ; Fri, 4 Nov 2022 00:00:39 -0700 (PDT) Received: by mail-pl1-x632.google.com with SMTP id 4so4131663pli.0 for ; Fri, 04 Nov 2022 00:00:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=OkaYBFTkTJOLws8unjECEd5re67/MivC1vGoCWgNdIA=; b=FKp6NbI+dU8fh5ZxoU8J2+rCaRu9ZcC1CSDUb+/iuOBdmi10+ZDR0lXKy7YyIBokBa hHH0sWInFczWPfNvzFp/C56hOZTbU2C+QY6zH3mN3aXSQ+n044oJ0ggkZB8h61ulJ0wJ HbsbkBptoJhgELan0IfRFRGRwB6sEAa/OLWKhTARy3vm4Y6W19RyJ2jnh+f0VGmBsqpu niSGI2IkoRU+ctTYkKJ9lYFjIzSYjRY8mWGuTuI8vjtr4ouBQDJqIO0d/L1ismojNUhx TDDFfzVWxpDXYPovwbBJYcGimOSXfURipT+2TMsKJmH84+yVug1aDYX0BJXM8wb5rF7e aO+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=OkaYBFTkTJOLws8unjECEd5re67/MivC1vGoCWgNdIA=; b=p//ucVkPiiySm8njDLH0QqRtUco87XfGmXlHj+r1xvjznWXejVSOEEVYfwY3lkNo7r 3PisGDU7B3tnEVNhRXBETZC4fN2FxtU5gpdYb30EolYDwn2tc8zfsPrpQAWmAm/isK4E Gid7SqcDr0Xor4c9tCOM6bFQMv5FZhv5bOQxV6idul+6n+hcrYQ6LfUlYe1+j/UviQ6n zESRntRDZ4M6tKcWyK0qZGKOhbLCSmdkV7IBan7f4naFtbGM3B/q7BxlYMdjgS7z8gRo Z+cI02x7co/L2Evu6eU+L2YN5sjCmWUr3pz2N1R0mmcBQ5mEB3aaePz66+gQSehrq5xj axIQ== X-Gm-Message-State: ACrzQf09yeeLVpPKolMx9lFuwhbUyqwndv1orsOjESVsu0z0GFqOmsUf TNMkPBil7eLzgdotS9X14qbNP3CMtWdujvmIT2Qu7A== X-Google-Smtp-Source: AMsMyM4au6JdSEM3oUwq6uwtgVjKg0ofbyk0NdD0oa6I18qKO8X0z9DSu94dwOgaWrxqawwY5CdkCKG2mxEd+r1/z9I= X-Received: by 2002:a17:90b:4a02:b0:213:9ba4:206a with SMTP id kk2-20020a17090b4a0200b002139ba4206amr37355261pjb.102.1667545239102; Fri, 04 Nov 2022 00:00:39 -0700 (PDT) MIME-Version: 1.0 References: <20221028105402.2030192-1-maz@kernel.org> <20221028105402.2030192-12-maz@kernel.org> <87tu3gfi8u.wl-maz@kernel.org> In-Reply-To: <87tu3gfi8u.wl-maz@kernel.org> From: Reiji Watanabe Date: Fri, 4 Nov 2022 00:00:22 -0700 Message-ID: Subject: Re: [PATCH v2 11/14] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace To: Marc Zyngier Cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvmarm@lists.linux.dev, kvm@vger.kernel.org, James Morse , Suzuki K Poulose , Alexandru Elisei , Oliver Upton , Ricardo Koller Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Hi Marc, On Thu, Nov 3, 2022 at 3:25 AM Marc Zyngier wrote: > > On Thu, 03 Nov 2022 05:31:56 +0000, > Reiji Watanabe wrote: > > > > Hi Marc, > > > > On Fri, Oct 28, 2022 at 4:16 AM Marc Zyngier wrote: > > > > > > Allow userspace to write ID_AA64DFR0_EL1, on the condition that only > > > the PMUver field can be altered and be at most the one that was > > > initially computed for the guest. > > > > > > Signed-off-by: Marc Zyngier > > > --- > > > arch/arm64/kvm/sys_regs.c | 37 ++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 36 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > > index 7a4cd644b9c0..4fa14b4ae2a6 100644 > > > --- a/arch/arm64/kvm/sys_regs.c > > > +++ b/arch/arm64/kvm/sys_regs.c > > > @@ -1247,6 +1247,40 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > > return 0; > > > } > > > > > > +static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, > > > + const struct sys_reg_desc *rd, > > > + u64 val) > > > +{ > > > + u8 pmuver, host_pmuver; > > > + > > > + host_pmuver = kvm_arm_pmu_get_pmuver_limit(); > > > + > > > + /* > > > + * Allow AA64DFR0_EL1.PMUver to be set from userspace as long > > > + * as it doesn't promise more than what the HW gives us. We > > > + * allow an IMPDEF PMU though, only if no PMU is supported > > > + * (KVM backward compatibility handling). > > > + */ > > > > It appears the patch allows userspace to set IMPDEF even > > when host_pmuver == 0. Shouldn't it be allowed only when > > host_pmuver == IMPDEF (as before)? > > Probably, it may not cause any real problems though. > > Given that we don't treat the two cases any differently, I thought it > would be reasonable to relax this particular case, and I can't see any > reason why we shouldn't tolerate this sort of migration. > Given that we don't treat the two cases any differently, I thought it > would be reasonable to relax this particular case, and I can't see any > reason why we shouldn't tolerate this sort of migration. That's true. I assume it won't cause any functional issues. I have another comment related to this. KVM allows userspace to create a guest with a mix of vCPUs with and without PMU. For such a guest, if the register for the vCPU without PMU is set last, I think the PMUVER value for vCPUs with PMU could become no PMU (0) or IMPDEF (0xf). Also, with the current patch, userspace can set PMUv3 support value (non-zero or non-IMPDEF) for vCPUs without the PMU. IMHO, KVM shouldn't allow userspace to set PMUVER to the value that is inconsistent with PMU configuration for the vCPU. What do you think ? I'm thinking of the following code (not tested). diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 4fa14b4ae2a6..ddd849027cc3 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1265,10 +1265,17 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, if (pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && pmuver > host_pmuver) return -EINVAL; - /* We already have a PMU, don't try to disable it... */ - if (kvm_vcpu_has_pmu(vcpu) && - (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)) - return -EINVAL; + if (kvm_vcpu_has_pmu(vcpu)) { + /* We already have a PMU, don't try to disable it... */ + if (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF) { + return -EINVAL; + } + } else { + /* We don't have a PMU, don't try to enable it... */ + if (pmuver > 0 && pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF) { + return -EINVAL; + } + } /* We can only differ with PMUver, and anything else is an error */ val ^= read_id_reg(vcpu, rd); @@ -1276,7 +1283,8 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, if (val) return -EINVAL; - vcpu->kvm->arch.dfr0_pmuver = pmuver; + if (kvm_vcpu_has_pmu(vcpu)) + vcpu->kvm->arch.dfr0_pmuver = pmuver; return 0; } Thank you, Reiji > > > > > > > > + pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), val); > > > + if (pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && pmuver > host_pmuver) > > > + return -EINVAL; > > > + > > > + /* We already have a PMU, don't try to disable it... */ > > > + if (kvm_vcpu_has_pmu(vcpu) && > > > + (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)) > > > + return -EINVAL; > > > > Nit: Perhaps it might be useful to return a different error code for the > > above two (new) error cases (I plan to use -E2BIG and -EPERM > > respectively for those cases with my ID register series). > > My worry in doing so is that we don't have an established practice for > these cases. I'm fine with introducing new error codes, but I'm not > sure there is an existing practice in userspace to actually interpret > them. > > Even -EPERM has a slightly different meaning, and although there is > some language there saying that it is all nonsense, we should be very > careful. > > 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 D8F47C4332F for ; Fri, 4 Nov 2022 07:00:45 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 45AB949673; Fri, 4 Nov 2022 03:00:45 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@google.com 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 r1vr+qDApxN5; Fri, 4 Nov 2022 03:00:43 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id D8D7343479; Fri, 4 Nov 2022 03:00:43 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id F2AE343479 for ; Fri, 4 Nov 2022 03:00:41 -0400 (EDT) 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 hWbuuWLYnugr for ; Fri, 4 Nov 2022 03:00:40 -0400 (EDT) Received: from mail-pl1-f176.google.com (mail-pl1-f176.google.com [209.85.214.176]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 95A6E40BA3 for ; Fri, 4 Nov 2022 03:00:40 -0400 (EDT) Received: by mail-pl1-f176.google.com with SMTP id io19so4075183plb.8 for ; Fri, 04 Nov 2022 00:00:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=OkaYBFTkTJOLws8unjECEd5re67/MivC1vGoCWgNdIA=; b=FKp6NbI+dU8fh5ZxoU8J2+rCaRu9ZcC1CSDUb+/iuOBdmi10+ZDR0lXKy7YyIBokBa hHH0sWInFczWPfNvzFp/C56hOZTbU2C+QY6zH3mN3aXSQ+n044oJ0ggkZB8h61ulJ0wJ HbsbkBptoJhgELan0IfRFRGRwB6sEAa/OLWKhTARy3vm4Y6W19RyJ2jnh+f0VGmBsqpu niSGI2IkoRU+ctTYkKJ9lYFjIzSYjRY8mWGuTuI8vjtr4ouBQDJqIO0d/L1ismojNUhx TDDFfzVWxpDXYPovwbBJYcGimOSXfURipT+2TMsKJmH84+yVug1aDYX0BJXM8wb5rF7e aO+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=OkaYBFTkTJOLws8unjECEd5re67/MivC1vGoCWgNdIA=; b=6MkbrqgZ1FCnQa/Wwbql0AFOEGDF6BJGswMHjc4a0elv0ONxOet7woTjLaDfceyZVV EhKGGZAZnvEYtPK9CuoBixRHQkGoc8xYmhpGILp8ghgrsDMy/S/q00BVf5uQOP88h+im dPdPDOFCq8IaOZG+15iv6zt/7GJJa9n+d1qyGXY+fmNicV712cI24tjsLil0pzpGKThk PljcK0F6lGixNKGIcchfLe3q0yrDp7yPp6J2979Qrgrbby7PVTRGIRcgPBrvVM3eDvS2 LpUjBcy5iSD3szP7lD/FJ0kXQ5dnJt/I2tkFYrknfoDFZ7bLBFjvlkvRazZmLzOKmJZc YB7A== X-Gm-Message-State: ACrzQf0oz2pOahltFBsHabONgPDrij86ziVwJwj3Buns39zD66y48Hoz 5r8YesYg6womMduHKTgv9FQYjVJmz+JBoAPQ0Okvkw== X-Google-Smtp-Source: AMsMyM4au6JdSEM3oUwq6uwtgVjKg0ofbyk0NdD0oa6I18qKO8X0z9DSu94dwOgaWrxqawwY5CdkCKG2mxEd+r1/z9I= X-Received: by 2002:a17:90b:4a02:b0:213:9ba4:206a with SMTP id kk2-20020a17090b4a0200b002139ba4206amr37355261pjb.102.1667545239102; Fri, 04 Nov 2022 00:00:39 -0700 (PDT) MIME-Version: 1.0 References: <20221028105402.2030192-1-maz@kernel.org> <20221028105402.2030192-12-maz@kernel.org> <87tu3gfi8u.wl-maz@kernel.org> In-Reply-To: <87tu3gfi8u.wl-maz@kernel.org> From: Reiji Watanabe Date: Fri, 4 Nov 2022 00:00:22 -0700 Message-ID: Subject: Re: [PATCH v2 11/14] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace To: Marc Zyngier Cc: kvm@vger.kernel.org, kvmarm@lists.linux.dev, 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 Hi Marc, On Thu, Nov 3, 2022 at 3:25 AM Marc Zyngier wrote: > > On Thu, 03 Nov 2022 05:31:56 +0000, > Reiji Watanabe wrote: > > > > Hi Marc, > > > > On Fri, Oct 28, 2022 at 4:16 AM Marc Zyngier wrote: > > > > > > Allow userspace to write ID_AA64DFR0_EL1, on the condition that only > > > the PMUver field can be altered and be at most the one that was > > > initially computed for the guest. > > > > > > Signed-off-by: Marc Zyngier > > > --- > > > arch/arm64/kvm/sys_regs.c | 37 ++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 36 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > > index 7a4cd644b9c0..4fa14b4ae2a6 100644 > > > --- a/arch/arm64/kvm/sys_regs.c > > > +++ b/arch/arm64/kvm/sys_regs.c > > > @@ -1247,6 +1247,40 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > > return 0; > > > } > > > > > > +static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, > > > + const struct sys_reg_desc *rd, > > > + u64 val) > > > +{ > > > + u8 pmuver, host_pmuver; > > > + > > > + host_pmuver = kvm_arm_pmu_get_pmuver_limit(); > > > + > > > + /* > > > + * Allow AA64DFR0_EL1.PMUver to be set from userspace as long > > > + * as it doesn't promise more than what the HW gives us. We > > > + * allow an IMPDEF PMU though, only if no PMU is supported > > > + * (KVM backward compatibility handling). > > > + */ > > > > It appears the patch allows userspace to set IMPDEF even > > when host_pmuver == 0. Shouldn't it be allowed only when > > host_pmuver == IMPDEF (as before)? > > Probably, it may not cause any real problems though. > > Given that we don't treat the two cases any differently, I thought it > would be reasonable to relax this particular case, and I can't see any > reason why we shouldn't tolerate this sort of migration. > Given that we don't treat the two cases any differently, I thought it > would be reasonable to relax this particular case, and I can't see any > reason why we shouldn't tolerate this sort of migration. That's true. I assume it won't cause any functional issues. I have another comment related to this. KVM allows userspace to create a guest with a mix of vCPUs with and without PMU. For such a guest, if the register for the vCPU without PMU is set last, I think the PMUVER value for vCPUs with PMU could become no PMU (0) or IMPDEF (0xf). Also, with the current patch, userspace can set PMUv3 support value (non-zero or non-IMPDEF) for vCPUs without the PMU. IMHO, KVM shouldn't allow userspace to set PMUVER to the value that is inconsistent with PMU configuration for the vCPU. What do you think ? I'm thinking of the following code (not tested). diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 4fa14b4ae2a6..ddd849027cc3 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1265,10 +1265,17 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, if (pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && pmuver > host_pmuver) return -EINVAL; - /* We already have a PMU, don't try to disable it... */ - if (kvm_vcpu_has_pmu(vcpu) && - (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)) - return -EINVAL; + if (kvm_vcpu_has_pmu(vcpu)) { + /* We already have a PMU, don't try to disable it... */ + if (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF) { + return -EINVAL; + } + } else { + /* We don't have a PMU, don't try to enable it... */ + if (pmuver > 0 && pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF) { + return -EINVAL; + } + } /* We can only differ with PMUver, and anything else is an error */ val ^= read_id_reg(vcpu, rd); @@ -1276,7 +1283,8 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, if (val) return -EINVAL; - vcpu->kvm->arch.dfr0_pmuver = pmuver; + if (kvm_vcpu_has_pmu(vcpu)) + vcpu->kvm->arch.dfr0_pmuver = pmuver; return 0; } Thank you, Reiji > > > > > > > > + pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), val); > > > + if (pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && pmuver > host_pmuver) > > > + return -EINVAL; > > > + > > > + /* We already have a PMU, don't try to disable it... */ > > > + if (kvm_vcpu_has_pmu(vcpu) && > > > + (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)) > > > + return -EINVAL; > > > > Nit: Perhaps it might be useful to return a different error code for the > > above two (new) error cases (I plan to use -E2BIG and -EPERM > > respectively for those cases with my ID register series). > > My worry in doing so is that we don't have an established practice for > these cases. I'm fine with introducing new error codes, but I'm not > sure there is an existing practice in userspace to actually interpret > them. > > Even -EPERM has a slightly different meaning, and although there is > some language there saying that it is all nonsense, we should be very > careful. > > 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 F047EC433FE for ; Fri, 4 Nov 2022 07:02:30 +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:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Zc0OungbDIVlKL6MrDT8ZaP2MQobIAy0UuDUBOVeU6Q=; b=02zYoOW2VIftCK 5KaZbmxpIEz9Er9JIPrieUpk7YzmRLr2gnizy43uFkU2bLxbYf/q/52R3kbPajsFoz5XoLii51nyp yFCkfDGdoIAGNB59oM/3Ibe4JjYbbYfneFcnMYblhVpALtRAvOHeZRDV7vP70ypVl3izXsc6QwuRw 1vvRL0U5BNvhXVLfRC8GLeJtTDiOZJIPwDm6TRH8axbbvW+VC2naJiVcYdCj6S1Wy99cdex/zytrj qgcfwDn0z4jCY0oehrKVQqthmOp/pZAnYR5CJcF1EFaaF/6u9SNDThD1hMbvXF24RiCjzccbuLZJt vgOkoY8iInXJ7SiHxcxA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oqqh9-002gps-PQ; Fri, 04 Nov 2022 07:00:52 +0000 Received: from mail-pj1-x1031.google.com ([2607:f8b0:4864:20::1031]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oqqgz-002gnZ-Uj for linux-arm-kernel@lists.infradead.org; Fri, 04 Nov 2022 07:00:43 +0000 Received: by mail-pj1-x1031.google.com with SMTP id d59-20020a17090a6f4100b00213202d77e1so7397908pjk.2 for ; Fri, 04 Nov 2022 00:00:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=OkaYBFTkTJOLws8unjECEd5re67/MivC1vGoCWgNdIA=; b=FKp6NbI+dU8fh5ZxoU8J2+rCaRu9ZcC1CSDUb+/iuOBdmi10+ZDR0lXKy7YyIBokBa hHH0sWInFczWPfNvzFp/C56hOZTbU2C+QY6zH3mN3aXSQ+n044oJ0ggkZB8h61ulJ0wJ HbsbkBptoJhgELan0IfRFRGRwB6sEAa/OLWKhTARy3vm4Y6W19RyJ2jnh+f0VGmBsqpu niSGI2IkoRU+ctTYkKJ9lYFjIzSYjRY8mWGuTuI8vjtr4ouBQDJqIO0d/L1ismojNUhx TDDFfzVWxpDXYPovwbBJYcGimOSXfURipT+2TMsKJmH84+yVug1aDYX0BJXM8wb5rF7e aO+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=OkaYBFTkTJOLws8unjECEd5re67/MivC1vGoCWgNdIA=; b=qDFfVbGACpaF1pHK7hwO7Gsg56HiVoSomoG9wFGf94njS2YBAZkVwyG6iStiis4QQ7 yEEcfIH+V/kmHpUi15CKY4JGh5LSQ7O1DonYAE2gAnQCfGPf433zkzBJHilE/P4AZobu 6pLdkQhtm1S2l1OkZWN/fdQgbaI8ptjlGnWbwIF477iVQV8ZV3Ob1VFi+yC0iFYR/ZnB k/HWq0ewWC12w/9hTL/cC2ZVCPxDmFyqKQYFp2Kp9NUoMnz/0apX/dyOwK+HuizgKdz9 6+LtLY9/SXis8T7ydB8vBrdZ2g7q94kvUrnTxls/+WPuvk4FH+72ngLUd9SegijXc2DU 4SBg== X-Gm-Message-State: ACrzQf1FBpXY4uhYpsn+rSj+rS68AwwMXvJogif6dVmPczC6FZvrnnMr kUqVKrY8rhyy99ETs8XCBw+zF1OT6I5CgvhOEiOsnw== X-Google-Smtp-Source: AMsMyM4au6JdSEM3oUwq6uwtgVjKg0ofbyk0NdD0oa6I18qKO8X0z9DSu94dwOgaWrxqawwY5CdkCKG2mxEd+r1/z9I= X-Received: by 2002:a17:90b:4a02:b0:213:9ba4:206a with SMTP id kk2-20020a17090b4a0200b002139ba4206amr37355261pjb.102.1667545239102; Fri, 04 Nov 2022 00:00:39 -0700 (PDT) MIME-Version: 1.0 References: <20221028105402.2030192-1-maz@kernel.org> <20221028105402.2030192-12-maz@kernel.org> <87tu3gfi8u.wl-maz@kernel.org> In-Reply-To: <87tu3gfi8u.wl-maz@kernel.org> From: Reiji Watanabe Date: Fri, 4 Nov 2022 00:00:22 -0700 Message-ID: Subject: Re: [PATCH v2 11/14] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace To: Marc Zyngier Cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvmarm@lists.linux.dev, kvm@vger.kernel.org, James Morse , Suzuki K Poulose , Alexandru Elisei , Oliver Upton , Ricardo Koller X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221104_000042_059786_6B824DA7 X-CRM114-Status: GOOD ( 47.21 ) 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 Hi Marc, On Thu, Nov 3, 2022 at 3:25 AM Marc Zyngier wrote: > > On Thu, 03 Nov 2022 05:31:56 +0000, > Reiji Watanabe wrote: > > > > Hi Marc, > > > > On Fri, Oct 28, 2022 at 4:16 AM Marc Zyngier wrote: > > > > > > Allow userspace to write ID_AA64DFR0_EL1, on the condition that only > > > the PMUver field can be altered and be at most the one that was > > > initially computed for the guest. > > > > > > Signed-off-by: Marc Zyngier > > > --- > > > arch/arm64/kvm/sys_regs.c | 37 ++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 36 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > > index 7a4cd644b9c0..4fa14b4ae2a6 100644 > > > --- a/arch/arm64/kvm/sys_regs.c > > > +++ b/arch/arm64/kvm/sys_regs.c > > > @@ -1247,6 +1247,40 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > > return 0; > > > } > > > > > > +static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, > > > + const struct sys_reg_desc *rd, > > > + u64 val) > > > +{ > > > + u8 pmuver, host_pmuver; > > > + > > > + host_pmuver = kvm_arm_pmu_get_pmuver_limit(); > > > + > > > + /* > > > + * Allow AA64DFR0_EL1.PMUver to be set from userspace as long > > > + * as it doesn't promise more than what the HW gives us. We > > > + * allow an IMPDEF PMU though, only if no PMU is supported > > > + * (KVM backward compatibility handling). > > > + */ > > > > It appears the patch allows userspace to set IMPDEF even > > when host_pmuver == 0. Shouldn't it be allowed only when > > host_pmuver == IMPDEF (as before)? > > Probably, it may not cause any real problems though. > > Given that we don't treat the two cases any differently, I thought it > would be reasonable to relax this particular case, and I can't see any > reason why we shouldn't tolerate this sort of migration. > Given that we don't treat the two cases any differently, I thought it > would be reasonable to relax this particular case, and I can't see any > reason why we shouldn't tolerate this sort of migration. That's true. I assume it won't cause any functional issues. I have another comment related to this. KVM allows userspace to create a guest with a mix of vCPUs with and without PMU. For such a guest, if the register for the vCPU without PMU is set last, I think the PMUVER value for vCPUs with PMU could become no PMU (0) or IMPDEF (0xf). Also, with the current patch, userspace can set PMUv3 support value (non-zero or non-IMPDEF) for vCPUs without the PMU. IMHO, KVM shouldn't allow userspace to set PMUVER to the value that is inconsistent with PMU configuration for the vCPU. What do you think ? I'm thinking of the following code (not tested). diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 4fa14b4ae2a6..ddd849027cc3 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1265,10 +1265,17 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, if (pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && pmuver > host_pmuver) return -EINVAL; - /* We already have a PMU, don't try to disable it... */ - if (kvm_vcpu_has_pmu(vcpu) && - (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)) - return -EINVAL; + if (kvm_vcpu_has_pmu(vcpu)) { + /* We already have a PMU, don't try to disable it... */ + if (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF) { + return -EINVAL; + } + } else { + /* We don't have a PMU, don't try to enable it... */ + if (pmuver > 0 && pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF) { + return -EINVAL; + } + } /* We can only differ with PMUver, and anything else is an error */ val ^= read_id_reg(vcpu, rd); @@ -1276,7 +1283,8 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, if (val) return -EINVAL; - vcpu->kvm->arch.dfr0_pmuver = pmuver; + if (kvm_vcpu_has_pmu(vcpu)) + vcpu->kvm->arch.dfr0_pmuver = pmuver; return 0; } Thank you, Reiji > > > > > > > > + pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), val); > > > + if (pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && pmuver > host_pmuver) > > > + return -EINVAL; > > > + > > > + /* We already have a PMU, don't try to disable it... */ > > > + if (kvm_vcpu_has_pmu(vcpu) && > > > + (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)) > > > + return -EINVAL; > > > > Nit: Perhaps it might be useful to return a different error code for the > > above two (new) error cases (I plan to use -E2BIG and -EPERM > > respectively for those cases with my ID register series). > > My worry in doing so is that we don't have an established practice for > these cases. I'm fine with introducing new error codes, but I'm not > sure there is an existing practice in userspace to actually interpret > them. > > Even -EPERM has a slightly different meaning, and although there is > some language there saying that it is all nonsense, we should be very > careful. > > 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