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 ECC21C4332F for ; Fri, 4 Nov 2022 15:53:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230376AbiKDPxl (ORCPT ); Fri, 4 Nov 2022 11:53:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58870 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230099AbiKDPxj (ORCPT ); Fri, 4 Nov 2022 11:53:39 -0400 Received: from mail-pj1-x102a.google.com (mail-pj1-x102a.google.com [IPv6:2607:f8b0:4864:20::102a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D4B6D2F39A for ; Fri, 4 Nov 2022 08:53:38 -0700 (PDT) Received: by mail-pj1-x102a.google.com with SMTP id u8-20020a17090a5e4800b002106dcdd4a0so8605362pji.1 for ; Fri, 04 Nov 2022 08:53:38 -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=xZr6sJYTNthILEKnYyiTIDKUQg5L0mQcLAjlSQHKvMA=; b=DiPJ7hBYkHlGfJ3nf30pKFGntHtz5evEjXdhztqq8D/A407dMn5AvpQqylNiNLOMgz nc3YDeYheeuuT9ftKl++CoVBnE/AP2CXvbMWD4eFsUsKk87usV/usn1bcTiwreMbOlnn oj96CfLSJujMO64hMTma39wr1Pegh0jD6d/HM3v3N04CX9SCv4+6rKcukonUQpXHsLXz C+MC4lvOQ6VjVb5UlfDRziD4JvPEGDO73B2J23nJbSOm5YSurMQ/M2etiRgDKVYl3cKv mie5iAg0h4RxBTAn0on5XteEtmlTH/HJ/8i5Uss7Si1C4JqtOGUixIgTx9l+Rx4CHGzg DTCg== 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=xZr6sJYTNthILEKnYyiTIDKUQg5L0mQcLAjlSQHKvMA=; b=lldXrahCTKYQz126imwi9d4tNlB2mbg6K4O5IZ1bO0sqpwMSkaYINXFKCtzLBnhJEu R3RVMXNJZbrYnmEZEeNW8SqzR6znF+qTFcNfNP5IY/ECwBDckOs3TLAf8an1Uk4l8z+i P+OzQqrZiUBRM2+0nTwhs3u2B1J3lqXglkwBFnpBC3vTR77Va0kluunu4KOcDHVL5FFM 62rcavWGiZCx3Z+u059rM2rcelSYSN8Jo6G6UmwXTDPJl2HCr/LYU6oKtah2deHQ4qnc AFWPgN2PNc14tp3uW43Gchz7FI03BnHJLUuSNbkEGpOWwRbIN+gGfgYsmngCpfKw+K9w 65OA== X-Gm-Message-State: ACrzQf1+pAUVTGjvN174vetRJVuL1gr16p8y5o/nHkZmA79+B+lHhwFa 7AbTlQkAzJvfAlwynvDx1zgb4FmVVgCR0jdwcEgmtQ== X-Google-Smtp-Source: AMsMyM5RjIpowH+EtUVsYugWjK1YsJUmZijSJNVPnbkraoQT7CYAkaAcrlZjjwisP8bWz4zMTHopTTUtp17kS/ninN4= X-Received: by 2002:a17:902:7145:b0:187:2356:c29d with SMTP id u5-20020a170902714500b001872356c29dmr26043238plm.154.1667577218174; Fri, 04 Nov 2022 08:53:38 -0700 (PDT) MIME-Version: 1.0 References: <20221028105402.2030192-1-maz@kernel.org> <20221028105402.2030192-12-maz@kernel.org> <87tu3gfi8u.wl-maz@kernel.org> <86bkpmrjv8.wl-maz@kernel.org> In-Reply-To: <86bkpmrjv8.wl-maz@kernel.org> From: Reiji Watanabe Date: Fri, 4 Nov 2022 08:53:21 -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 Fri, Nov 4, 2022 at 5:21 AM Marc Zyngier wrote: > > Hi Reiji, > > On Fri, 04 Nov 2022 07:00:22 +0000, > Reiji Watanabe wrote: > > > > On Thu, Nov 3, 2022 at 3:25 AM Marc Zyngier wrote: > > > > > > On Thu, 03 Nov 2022 05:31:56 +0000, > > > Reiji Watanabe wrote: > > > > > > > > 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. > > > > 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 ? > > Yes, this seems sensible, and we only do it one way at the moment. > > > 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; > > + } > > + } > > This is a bit ugly. I came up with this instead: > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 3b28ef48a525..e104fde1a0ee 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1273,6 +1273,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, > u64 val) > { > u8 pmuver, host_pmuver; > + bool valid_pmu; > > host_pmuver = kvm_arm_pmu_get_pmuver_limit(); > > @@ -1286,9 +1287,10 @@ 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)) > + valid_pmu = (pmuver != 0 && pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF); > + > + /* Make sure view register and PMU support do match */ > + if (kvm_vcpu_has_pmu(vcpu) != valid_pmu) > return -EINVAL; Thanks, much better! > > /* We can only differ with PMUver, and anything else is an error */ > > and the similar check for the 32bit counterpart. > > > > > /* 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; > > We need to update this unconditionally if we want to be able to > restore an IMPDEF PMU view to the guest. Yes, right. BTW, if we have no intention of supporting a mix of vCPUs with and without PMU, I think it would be nice if we have a clear comment on that in the code. Or I'm hoping to disallow it if possible though. Thank you, Reiji 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 42D32C433FE for ; Fri, 4 Nov 2022 15:53:44 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id C87A340FD3; Fri, 4 Nov 2022 11:53:43 -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 b14sh9EI9aOb; Fri, 4 Nov 2022 11:53:42 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 9E8C440D23; Fri, 4 Nov 2022 11:53:42 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 8ACA140801 for ; Fri, 4 Nov 2022 11:53:40 -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 RW164+qE2mpU for ; Fri, 4 Nov 2022 11:53:39 -0400 (EDT) Received: from mail-pj1-f49.google.com (mail-pj1-f49.google.com [209.85.216.49]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 4CA694079D for ; Fri, 4 Nov 2022 11:53:39 -0400 (EDT) Received: by mail-pj1-f49.google.com with SMTP id v4-20020a17090a088400b00212cb0ed97eso4881470pjc.5 for ; Fri, 04 Nov 2022 08:53: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=xZr6sJYTNthILEKnYyiTIDKUQg5L0mQcLAjlSQHKvMA=; b=DiPJ7hBYkHlGfJ3nf30pKFGntHtz5evEjXdhztqq8D/A407dMn5AvpQqylNiNLOMgz nc3YDeYheeuuT9ftKl++CoVBnE/AP2CXvbMWD4eFsUsKk87usV/usn1bcTiwreMbOlnn oj96CfLSJujMO64hMTma39wr1Pegh0jD6d/HM3v3N04CX9SCv4+6rKcukonUQpXHsLXz C+MC4lvOQ6VjVb5UlfDRziD4JvPEGDO73B2J23nJbSOm5YSurMQ/M2etiRgDKVYl3cKv mie5iAg0h4RxBTAn0on5XteEtmlTH/HJ/8i5Uss7Si1C4JqtOGUixIgTx9l+Rx4CHGzg DTCg== 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=xZr6sJYTNthILEKnYyiTIDKUQg5L0mQcLAjlSQHKvMA=; b=pI3qr+BTqOLE7iw676E2RdFiESys+PTCP9lQ/5Q4gqx+5tnnA+MZwymMUropnQa5Ox zZlOwzjj1M2dUPw6YmeLxSCoQVGsQy653UV+bLT7ZeIeWMN8M0EEcmlkiJZf/EyJkGpl /gwXBYeNFR8rdzsR+L0iroeePwqswuFlpPUZGagXEU5bm04CYT5BPaXnYovY3mcGc2ep Xlb/9OiSp74l4g9cG4PKD+lg1hKh+8704RfwDRbjiGmTSsHM8j0+5RnL+vw3TRz5K0VA M7KgoTK2NBMPWNbBnWgXq6AFJBroFOuxFoVq7PwJsv49h+3GAxNAENzKlOlpiHppuo4a 0Sxg== X-Gm-Message-State: ACrzQf0LVPj/TnxCc0RWIHw8KmEXcOlVkumHSl/S0VAa2qjvVOIE+333 9xpYHCiT6nsZwhSrU/tPBGSZwQ9kXvredt3pjYrM7g== X-Google-Smtp-Source: AMsMyM5RjIpowH+EtUVsYugWjK1YsJUmZijSJNVPnbkraoQT7CYAkaAcrlZjjwisP8bWz4zMTHopTTUtp17kS/ninN4= X-Received: by 2002:a17:902:7145:b0:187:2356:c29d with SMTP id u5-20020a170902714500b001872356c29dmr26043238plm.154.1667577218174; Fri, 04 Nov 2022 08:53:38 -0700 (PDT) MIME-Version: 1.0 References: <20221028105402.2030192-1-maz@kernel.org> <20221028105402.2030192-12-maz@kernel.org> <87tu3gfi8u.wl-maz@kernel.org> <86bkpmrjv8.wl-maz@kernel.org> In-Reply-To: <86bkpmrjv8.wl-maz@kernel.org> From: Reiji Watanabe Date: Fri, 4 Nov 2022 08:53:21 -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 Fri, Nov 4, 2022 at 5:21 AM Marc Zyngier wrote: > > Hi Reiji, > > On Fri, 04 Nov 2022 07:00:22 +0000, > Reiji Watanabe wrote: > > > > On Thu, Nov 3, 2022 at 3:25 AM Marc Zyngier wrote: > > > > > > On Thu, 03 Nov 2022 05:31:56 +0000, > > > Reiji Watanabe wrote: > > > > > > > > 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. > > > > 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 ? > > Yes, this seems sensible, and we only do it one way at the moment. > > > 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; > > + } > > + } > > This is a bit ugly. I came up with this instead: > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 3b28ef48a525..e104fde1a0ee 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1273,6 +1273,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, > u64 val) > { > u8 pmuver, host_pmuver; > + bool valid_pmu; > > host_pmuver = kvm_arm_pmu_get_pmuver_limit(); > > @@ -1286,9 +1287,10 @@ 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)) > + valid_pmu = (pmuver != 0 && pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF); > + > + /* Make sure view register and PMU support do match */ > + if (kvm_vcpu_has_pmu(vcpu) != valid_pmu) > return -EINVAL; Thanks, much better! > > /* We can only differ with PMUver, and anything else is an error */ > > and the similar check for the 32bit counterpart. > > > > > /* 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; > > We need to update this unconditionally if we want to be able to > restore an IMPDEF PMU view to the guest. Yes, right. BTW, if we have no intention of supporting a mix of vCPUs with and without PMU, I think it would be nice if we have a clear comment on that in the code. Or I'm hoping to disallow it if possible though. Thank you, Reiji _______________________________________________ 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 67659C4332F for ; Fri, 4 Nov 2022 15:54:52 +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=j/Ep3VNShMU5QVNxxp27tvPBut33KLvn9cF+Y7OG1r0=; b=IexvO+8p/j/Wua UOMBpMWxymPjoKXo5zSXx0JszmZJoDCRpGwk3K5/wTAS6G9auV0hneT90q8YifSbBXFhu+2FekUOa VD37eIwvysOL7fGHn3qF5oOHqjpr3Om7EwFyqcG6wfgjoS2u/hFV44OwTposfXDgYlWvd/BDOzJKX z9tJEbQT+zL5ZBF3JjP27aaJHXfNulCCAuaAy4rD1aulqsjF0x6xczL9SJzHKvQ44Zqr3Uv3MaoOn 5h8C3AUaLRANeDbNU3BJniWlIedSh/ZQpU2IlBXGUevUMBOFTc+gF0WyvQLKUFYZPWRzzILOwIl3Y b6k0JIa1RkubclKYkIPw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oqz0n-004L5s-Uj; Fri, 04 Nov 2022 15:53:42 +0000 Received: from mail-pj1-x1036.google.com ([2607:f8b0:4864:20::1036]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oqz0l-004L4f-6Y for linux-arm-kernel@lists.infradead.org; Fri, 04 Nov 2022 15:53:40 +0000 Received: by mail-pj1-x1036.google.com with SMTP id d59-20020a17090a6f4100b00213202d77e1so8601371pjk.2 for ; Fri, 04 Nov 2022 08:53:38 -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=xZr6sJYTNthILEKnYyiTIDKUQg5L0mQcLAjlSQHKvMA=; b=DiPJ7hBYkHlGfJ3nf30pKFGntHtz5evEjXdhztqq8D/A407dMn5AvpQqylNiNLOMgz nc3YDeYheeuuT9ftKl++CoVBnE/AP2CXvbMWD4eFsUsKk87usV/usn1bcTiwreMbOlnn oj96CfLSJujMO64hMTma39wr1Pegh0jD6d/HM3v3N04CX9SCv4+6rKcukonUQpXHsLXz C+MC4lvOQ6VjVb5UlfDRziD4JvPEGDO73B2J23nJbSOm5YSurMQ/M2etiRgDKVYl3cKv mie5iAg0h4RxBTAn0on5XteEtmlTH/HJ/8i5Uss7Si1C4JqtOGUixIgTx9l+Rx4CHGzg DTCg== 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=xZr6sJYTNthILEKnYyiTIDKUQg5L0mQcLAjlSQHKvMA=; b=BsHJut4Q48k/n0WPSjotRoLrVx02irkWJnn2/JCw9r2DNaTTMxQpevuojf6Jv3UdJ0 Dmztuam4XNbOMjOFwNlk5XoEeVGwSIq9pp95hwv0f5KPzKmulG1nWf/CStoSLStrc4CE tw+SiEy+oSyw+LqVhzxY0qw6chj0QMMJX1mO/F79d5s5Y+gISnsNfc/X5SoRQkfYYgJU 5rPjMdsbcZ9RZLK+5veMrUZrCL55MyW+SzVnTfvKKlP8K0ZHQb63MhHMwoxXP1Wk3Gnx /bACTqMyoL71oBn9nkrnK8bZLTAYzjsVbcGDhPauRf1ojacDY1BhM49q+FugeU1UOt01 6BqQ== X-Gm-Message-State: ACrzQf1bzkUqB3Y4gIMdmK9N0gLJuFJozIRPRkl5dTvYPY0niN6PvkJC H7BIZ+ttA+VviCxAWFDOE4Sped/J9GxP2YX1pmhSux7+NLGQWg== X-Google-Smtp-Source: AMsMyM5RjIpowH+EtUVsYugWjK1YsJUmZijSJNVPnbkraoQT7CYAkaAcrlZjjwisP8bWz4zMTHopTTUtp17kS/ninN4= X-Received: by 2002:a17:902:7145:b0:187:2356:c29d with SMTP id u5-20020a170902714500b001872356c29dmr26043238plm.154.1667577218174; Fri, 04 Nov 2022 08:53:38 -0700 (PDT) MIME-Version: 1.0 References: <20221028105402.2030192-1-maz@kernel.org> <20221028105402.2030192-12-maz@kernel.org> <87tu3gfi8u.wl-maz@kernel.org> <86bkpmrjv8.wl-maz@kernel.org> In-Reply-To: <86bkpmrjv8.wl-maz@kernel.org> From: Reiji Watanabe Date: Fri, 4 Nov 2022 08:53:21 -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_085339_261108_F5EC418A X-CRM114-Status: GOOD ( 42.43 ) 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 Fri, Nov 4, 2022 at 5:21 AM Marc Zyngier wrote: > > Hi Reiji, > > On Fri, 04 Nov 2022 07:00:22 +0000, > Reiji Watanabe wrote: > > > > On Thu, Nov 3, 2022 at 3:25 AM Marc Zyngier wrote: > > > > > > On Thu, 03 Nov 2022 05:31:56 +0000, > > > Reiji Watanabe wrote: > > > > > > > > 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. > > > > 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 ? > > Yes, this seems sensible, and we only do it one way at the moment. > > > 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; > > + } > > + } > > This is a bit ugly. I came up with this instead: > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 3b28ef48a525..e104fde1a0ee 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1273,6 +1273,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, > u64 val) > { > u8 pmuver, host_pmuver; > + bool valid_pmu; > > host_pmuver = kvm_arm_pmu_get_pmuver_limit(); > > @@ -1286,9 +1287,10 @@ 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)) > + valid_pmu = (pmuver != 0 && pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF); > + > + /* Make sure view register and PMU support do match */ > + if (kvm_vcpu_has_pmu(vcpu) != valid_pmu) > return -EINVAL; Thanks, much better! > > /* We can only differ with PMUver, and anything else is an error */ > > and the similar check for the 32bit counterpart. > > > > > /* 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; > > We need to update this unconditionally if we want to be able to > restore an IMPDEF PMU view to the guest. Yes, right. BTW, if we have no intention of supporting a mix of vCPUs with and without PMU, I think it would be nice if we have a clear comment on that in the code. Or I'm hoping to disallow it if possible though. Thank you, Reiji _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel