Received: by 10.223.176.5 with SMTP id f5csp502015wra; Sat, 3 Feb 2018 04:13:53 -0800 (PST) X-Google-Smtp-Source: AH8x225llXhPRvidCPuRwE26nUvUMcFx3ttHvCcfcwvChzfGuHuDyarioJrgB/mR11ovNPTvGMvw X-Received: by 2002:a17:902:6c:: with SMTP id 99-v6mr36115019pla.409.1517660033022; Sat, 03 Feb 2018 04:13:53 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1517660032; cv=none; d=google.com; s=arc-20160816; b=ItXLaVsR8jnbzMt7nXnNVvLw34O6d1yGNhjJEkDXKePowPgKfnEnPER+8HEzEyOBDS aeXafRgT2cqmeXmpIwHaTecbrXTSLJsJJfVDSTPczFv1ETsDcsbHpSNZ3eehIE257YXY Kh609KrBxfXBRoeEERfVWK81dsgSE7Okacu+ynkGkNZYAmM9+cg6gIeRJ9KUS2gsrj5b l1MJcc3IP/RTSbe3ygcD79Fxfgl7zjSU3JYWCSW3rmzO3xQ8F46/g5E/42eB6wp5lKYq vGzhn5PHqz64Myq0qwQv9JOR34UAo0vmpFaIUDu62zWvq8BLoezYyPVVP7iGd1kwj/tR Wpwg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :organization:references:in-reply-to:message-id:subject:cc:to:from :date:arc-authentication-results; bh=toogYm4vdDgDcdWpUQL6XwdwmI6vphEnJSmFWo47VRM=; b=FkHmMi8ltxYtyjjE4fNCBe2UrAu4+wJGyqadQbqyxzhwrQmV0sZczlC/UqK5lAMEH1 2/Qzw4TQl+TxFcHkvRFNywHDaGEfDzSQb5JUCnLwV8gMgN0RZOoFqcL6kar9ie9IvTLI 1LhvTYgnlKMws3SzFM/6wf4U0GhYCf0QEKI2e0COem6ksw2cHPQzKDjXWHn3j78rJ+hJ 8DRAfeZesfvJ3p/GXM1ddyE18EhJLnGmh8MvmK88/Le2lopjRc1ZmiCLABKupoy7xPIz FxdKS4cNnH3/vhqstD7fNInvuvU4VK2OLgNjZQI6ljN+Tq9PzBRIcLh+udamPrX6z0Qi IdBQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id l12si2916800pgf.344.2018.02.03.04.13.36; Sat, 03 Feb 2018 04:13:52 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752177AbeBCL7o (ORCPT + 99 others); Sat, 3 Feb 2018 06:59:44 -0500 Received: from foss.arm.com ([217.140.101.70]:38720 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750860AbeBCL7k (ORCPT ); Sat, 3 Feb 2018 06:59:40 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8946E1529; Sat, 3 Feb 2018 03:59:39 -0800 (PST) Received: from why.wild-wind.fr.eu.org (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 45B6B3F24D; Sat, 3 Feb 2018 03:59:36 -0800 (PST) Date: Sat, 3 Feb 2018 11:59:32 +0000 From: Marc Zyngier To: Andrew Jones Cc: , , , Catalin Marinas , Will Deacon , Peter Maydell , Christoffer Dall , Lorenzo Pieralisi , Mark Rutland , Robin Murphy , Ard Biesheuvel , Hanjun Guo , Jayachandran C , Jon Masters , Russell King - ARM Linux Subject: Re: [PATCH v3 08/18] arm/arm64: KVM: Add PSCI version selection API Message-ID: <20180203115932.24c4bcfb@why.wild-wind.fr.eu.org> In-Reply-To: <20180202201706.dj6jmff7lh7bleze@kamzik.brq.redhat.com> References: <20180201114657.7323-1-marc.zyngier@arm.com> <20180201114657.7323-9-marc.zyngier@arm.com> <20180202201706.dj6jmff7lh7bleze@kamzik.brq.redhat.com> Organization: ARM Ltd X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2 Feb 2018 21:17:06 +0100 Andrew Jones wrote: > On Thu, Feb 01, 2018 at 11:46:47AM +0000, Marc Zyngier wrote: > > Although we've implemented PSCI 1.0 and 1.1, nothing can select them > > Since all the new PSCI versions are backward compatible, we decide to > > default to the latest version of the PSCI implementation. This is no > > different from doing a firmware upgrade on KVM. > > > > But in order to give a chance to hypothetical badly implemented guests > > that would have a fit by discovering something other than PSCI 0.2, > > let's provide a new API that allows userspace to pick one particular > > version of the API. > > > > This is implemented as a new class of "firmware" registers, where > > we expose the PSCI version. This allows the PSCI version to be > > save/restored as part of a guest migration, and also set to > > any supported version if the guest requires it. > > > > Signed-off-by: Marc Zyngier > > --- > > Documentation/virtual/kvm/api.txt | 3 +- > > Documentation/virtual/kvm/arm/psci.txt | 30 +++++++++++++++ > > arch/arm/include/asm/kvm_host.h | 3 ++ > > arch/arm/include/uapi/asm/kvm.h | 6 +++ > > arch/arm/kvm/guest.c | 13 +++++++ > > arch/arm64/include/asm/kvm_host.h | 3 ++ > > arch/arm64/include/uapi/asm/kvm.h | 6 +++ > > arch/arm64/kvm/guest.c | 14 ++++++- > > include/kvm/arm_psci.h | 9 +++++ > > virt/kvm/arm/psci.c | 68 +++++++++++++++++++++++++++++++++- > > 10 files changed, 151 insertions(+), 4 deletions(-) > > create mode 100644 Documentation/virtual/kvm/arm/psci.txt > > > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > > index 57d3ee9e4bde..334905202141 100644 > > --- a/Documentation/virtual/kvm/api.txt > > +++ b/Documentation/virtual/kvm/api.txt > > @@ -2493,7 +2493,8 @@ Possible features: > > and execute guest code when KVM_RUN is called. > > - KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode. > > Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only). > > - - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU. > > + - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 (or a future revision > > + backward compatible with v0.2) for the CPU. > > Depends on KVM_CAP_ARM_PSCI_0_2. > > - KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU. > > Depends on KVM_CAP_ARM_PMU_V3. > > diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt > > new file mode 100644 > > index 000000000000..aafdab887b04 > > --- /dev/null > > +++ b/Documentation/virtual/kvm/arm/psci.txt > > @@ -0,0 +1,30 @@ > > +KVM implements the PSCI (Power State Coordination Interface) > > +specification in order to provide services such as CPU on/off, reset > > +and power-off to the guest. > > + > > +The PSCI specification is regularly updated to provide new features, > > +and KVM implements these updates if they make sense from a virtualization > > +point of view. > > + > > +This means that a guest booted on two different versions of KVM can > > +observe two different "firmware" revisions. This could cause issues if > > +a given guest is tied to a particular PSCI revision (unlikely), or if > > +a migration causes a different PSCI version to be exposed out of the > > +blue to an unsuspecting guest. > > + > > +In order to remedy this situation, KVM exposes a set of "firmware > > +pseudo-registers" that can be manipulated using the GET/SET_ONE_REG > > +interface. These registers can be saved/restored by userspace, and set > > +to a convenient value if required. > > + > > +The following register is defined: > > + > > +* KVM_REG_ARM_PSCI_VERSION: > > + > > + - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set > > + (and thus has already been initialized) > > + - Returns the current PSCI version on GET_ONE_REG (defaulting to the > > + highest PSCI version implemented by KVM and compatible with v0.2) > > + - Allows any PSCI version implemented by KVM and compatible with > > + v0.2 to be set with SET_ONE_REG > > + - Affects the whole VM (even if the register view is per-vcpu) > Hi Drew, Thanks for looking into this, and for the exhaustive data. > > I've put some more thought and experimentation into this. I think we > should change to a vcpu feature bit. The feature bit would be used to > force compat mode, v0.2, so KVM would still enable the new PSCI > version by default. Below are two tables describing why I think we > should switch to something other than a new sysreg, and below those > tables are notes as to why I think we should use a vcpu feature. The > asterisks in the tables point out behaviors that aren't what we want. > While both tables have an asterisk, the sysreg approach's issue is > bug. The vcpu feature approach's issue is risk incurred from an > unsupported migration, albeit one that is hard to detect without a > new machine type. > > +-----------------------------------------------------------------------+ > | sysreg approach | > +------------------+-----------+-------+--------------------------------+ > | migration | userspace | works | notes | > | | change | | | > +------------------+-----------+-------+--------------------------------+ > | new -> new | NO | YES | Expected | > +------------------+-----------+-------+--------------------------------+ > | old -> new | NO | YES | PSCI 1.0 is backward compatible| > +------------------+-----------+-------+--------------------------------+ > | new -> old | NO | NO | Migration fails due to the new | > | | | | sysreg. Migration shouldn't | > | | | | have been attempted, but no | > | | | | way to know without a new | > | | | | machine type. | > +------------------+-----------+-------+--------------------------------+ > | compat -> old | YES | NO* | Even when setting PSCI version | > | | | | to 0.2, we add a new sysreg, | > | | | | so migration will still fail. | > +------------------+-----------+-------+--------------------------------+ > | old -> compat | YES | YES | It's OK for the destination to | > | | | | support more sysregs than the | > | | | | source sends. | > +------------------+-----------+-------+--------------------------------+ > > > +-----------------------------------------------------------------------+ > | vcpu feature approach | > +------------------+-----------+-------+--------------------------------+ > | migration | userspace | works | notes | > | | change | | | > +------------------+-----------+-------+--------------------------------+ > | new -> new | NO | YES | Expected | > +------------------+-----------+-------+--------------------------------+ > | old -> new | NO | YES | PSCI 1.0 is backward compatible| > +------------------+-----------+-------+--------------------------------+ > | new -> old | NO | YES* | Migrates, but it's not safe | > | | | | for the guest kernel, and no | > | | | | way to know without a new | > | | | | machine type. | > +------------------+-----------+-------+--------------------------------+ > | compat -> old | YES | YES | Expected | > +------------------+-----------+-------+--------------------------------+ > | old -> compat | YES | YES | Expected | > +------------------+-----------+-------+--------------------------------+ > > > Notes as to why the vcpu feature approach was selected: > > 1) While this is VM state, and thus a VM control would be a more natural > fit, a new vcpu feature bit would be much less new code. We also > already have a PSCI vcpu feature bit, so a new one will actually fit > quite well. > > 2) No new state needs to be migrated, as we already migrate the feature > bitmap. Unlike, KVM, QEMU doesn't track the max number of features, > so bumping it one more in KVM doesn't require a QEMU change. > > > If we switch to a vcpu feature bit, then I think this patch can be > replaced with something like this A couple of remarks: - My worry with this feature bit is that it is a point fix, and it doesn't scale. Come PSCI 1.2 and WORKAROUND_2, what do we do? Add another feature bit that says "force to 1.0"? I'd really like something we can live with in the long run, and "KVM as firmware" needs to be able to evolve without requiring a new userspace interface each time we rev it. - The "compat->old" entry in your sysreg table is not quite fair. In the feature table, you teach userspace about the new feature bit. You could just as well teach userspace about the new sysreg. Yes, things may be different in QEMU, but that's not what we're talking about here. - Allowing a guest to migrate in an unsafe way seems worse than failing a migration unexpectedly. Or at least not any better. To be clear: I'm not dismissing the idea at all, but I want to make sure we're not cornering ourselves into an uncomfortable place. Christoffer, Peter, what are your thoughts on this? Thanks, M. -- Without deviation from the norm, progress is not possible.