From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Martin Subject: Re: [PATCH v5 22/26] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths Date: Thu, 7 Mar 2019 15:30:45 +0000 Message-ID: <20190307153042.GL3567@e103592.cambridge.arm.com> References: <1550519559-15915-1-git-send-email-Dave.Martin@arm.com> <1550519559-15915-23-git-send-email-Dave.Martin@arm.com> <34b7691f-7acb-c9aa-6e53-2857e429f148@arm.com> <20190226121347.GT3567@e103592.cambridge.arm.com> <20190301145503.GI3567@e103592.cambridge.arm.com> <5a3fda6d-5da8-8aa9-9b0e-36d6bcd7441b@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id E865D4A4B0 for ; Thu, 7 Mar 2019 10:30:51 -0500 (EST) 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 XZcluNHk95Gq for ; Thu, 7 Mar 2019 10:30:50 -0500 (EST) Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 3E08F4A4AB for ; Thu, 7 Mar 2019 10:30:50 -0500 (EST) Content-Disposition: inline In-Reply-To: <5a3fda6d-5da8-8aa9-9b0e-36d6bcd7441b@arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: Marc Zyngier Cc: Okamoto Takayuki , Christoffer Dall , Ard Biesheuvel , Catalin Marinas , Will Deacon , Zhang Lei , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org List-Id: kvmarm@lists.cs.columbia.edu On Thu, Mar 07, 2019 at 01:47:09PM +0000, Marc Zyngier wrote: > Hi Dave, > > On 01/03/2019 14:55, Dave Martin wrote: > > [FYI Peter, your views on the proposal outlined torward the end of the > > mail would be appreciated.] > > > > On Fri, Mar 01, 2019 at 01:28:19PM +0000, Julien Thierry wrote: > > [...] > > >> I might be over-thinking it, but if there is a way to move that > >> finalization call I'd prefer that. > > > > I know what you mean, but there's not really another clear place to put > > it either, right now. Making finalization a side-effect of only KVM_RUN > > and KVM_GET_REG_LIST would mean that if userspace is squirting in the > > state of a migrated vcpu, a dummy KVM_GET_REG_LIST call would need to be > > inserted between setting KVM_REG_ARM64_SVE_VLS and setting the other SVE > > regs. > > > > One thing we could to is get rid of the implicit finalization behaviour > > completely, and require an explicit ioctl KVM_ARM_VCPU_FINALIZE to do > > +1. In the past, implicit finalization has been a major pain, and the > "implicit GICv2 configuration" has been an absolute disaster. > > > this. This makes a clear barrier between reg writes that manipulate the > > "physical" configuration of the vcpu, and reg reads/writes that merely > > manipulate the vcpu's runtime state. Say: > > > > KVM_ARM_VCPU_INIT(features[] = KVM_ARM_VCPU_SVE) -> ok > > KVM_RUN -> -EPERM (too early) > > KVM_GET_REG_LIST -> -EPERM (too early) > > ... > > KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_VLS) -> ok > > KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_ZREG(0, 0)) -> -EPERM (too early) > > ... > > KVM_RUN -> -EPERM (too early) > > ... > > KVM_ARM_VCPU_FINALIZE -> ok > > KVM_ARM_VCPU_FINALIZE -> -EPERM (too late) > > ... > > KVM_REG_REG_LIST -> ok > > KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_VLS) -> -EPERM (too late) > > KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_ZREG(0, 0)) -> ok > > KVM_RUN -> ok > > KVM_ARM_VCPU_FINALIZE -> -EPERM (too late) > > > > This would change all existing kvm_arm_vcpu_finalize() calls to > > > > if (!kvm_arm_vcpu_finalized()) > > return -EPERM; > > > > (or similar). > > I find this rather compelling, assuming we can easily map features that > are associated to finalization. OK ... thanks for taking a look. > > Without an affected vcpu feature enabled, for backwards compatibility > > we would not require the KVM_ARM_VCPU_FINALIZE call (or perhaps even > > forbid it, since userspace that wants to backwards compatible cannot > > use the new ioctl anyway. I'm in two minds about this. Either way, > > calling _FINALIZE on an old kvm is harmless, so long as userspace knows > > to ignore this failure case.) > > > > The backwards-compatible flow would be: > > > > KVM_ARM_VCPU_INIT(features[] = 0) -> ok > > ... > > KVM_ARM_VCPU_FINALIZE -> ok (or -EPERM) > > ... > > KVM_RUN -> ok > > KVM_ARM_VCPU_FINALIZE -> -EPERM (too late) > > > > > > Thoughts? > > This goes back to the above question: how do we ensure that userspace > knows which features are subject to being finalized. As it is currently > described, KVM_ARM_VCPU_FINALIZE isn't qualified by a feature set, nor > can the kernel report what feature flags requires being finalized. It is > also unclear to me whether all "finalizable" features would have the > same init cycle. So, it's not clear whether any other features will ever need finalization, and even if they do there's a fair chance they won't be interdependent with SVE in such a way as to require multiple finalization steps. For now, it's seems reasonable to make the finalization call a no-op when no finalization is required. Where finalization is required, KVM_ARM_VCPU_FINALIZE becomes mandatory, but the requirement is a) strictly opt-in, and b) userspace will quickly discover if it gets this wrong. For this reason I'd rather not have any explicit reporting of whether finalization is needed or not (or why): we just document and enforce at run-time. > So either we take the simplest approach and make KVM_ARM_VCPU_FINALIZE > strictly SVE specific (renaming it in the process), or it takes some > argument that allow specifying the feature set it applies to. Maybe we > can get away with the kernel not reporting which features require to be > finalized as long that it is clearly documented. OK, what about: * Userspace treats KVM_ARM_VCPU_FINALIZE() -> EINVAL as no error (so that userspace can simply insert this into its init sequence, even on old kernels). * We add an argument, so that KVM_ARM_VCPU_FINALIZE(0) means "finalize default stuff, including SVE". We can add explicit flags later if needed to finalize individual features separately. I don't know whether any features ever will have interdependent finalization requirements, but this should help get us off the hook if so. Question: * KVM_REG_ARM64_SVE_VLS is a weird register because of its special sequencing requirements. The main reason for this is to make it easier to detect migration to a mismatched destination node. But userspace needs some explicit code to make all this work anyway, so should we just have a couple of ioctls to get/set it instead? If there's no strong view either way, I'll leave it as-is, to minimise churn. [...] > Please do your best to have something as close as possible to the final > version for -rc1. From that point on, I'd expect changes to be mostly > cosmetic. This ought to be feasible. The changes being discussed so far are non- invasive. Cheers ---Dave 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 X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 68291C10F03 for ; Thu, 7 Mar 2019 15:31:03 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 2E9BB20851 for ; Thu, 7 Mar 2019 15:31:03 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="kXKA5PY6" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2E9BB20851 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=P2RGwGh5jbCziX5RNpYg39Mg/LE9ZhNMIen+VXo8sXk=; b=kXKA5PY6DUmzPq SdoUqd5P9ShDWYt/cv6eJ+sZitcDE2rUobyqJcVzHFrVZBqzUWAReMIK/BP/lu8SOadZNXbvqXuyt ReNQkF0jOfQag0q/bqM+bP9FWwG9Dbx9sLAKeTNYwCh7M18QK9PX0dSrrw2joFuCfRTvcvL57FPoe ks1omNT4F430diBTnaTEHYwmMQfm8ZDIH3dE/VeyH1PDPblAwMSyC+bAqqtQfIeUk8Jm7PDdQcyQP WxfzZQVWssS7Iww0pz6L7N+M2TN+VOj8y2/zifMXX1kEJfWpxYRBktfCA22o1ozOb54c2g3rM8UvO MJBq9wDvUYnJwVSUlt8w==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h1uz2-0004wP-0T; Thu, 07 Mar 2019 15:30:56 +0000 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70] helo=foss.arm.com) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h1uyx-0004uR-W9 for linux-arm-kernel@lists.infradead.org; Thu, 07 Mar 2019 15:30:53 +0000 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 858E0EBD; Thu, 7 Mar 2019 07:30:49 -0800 (PST) Received: from e103592.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B7DF03F706; Thu, 7 Mar 2019 07:30:47 -0800 (PST) Date: Thu, 7 Mar 2019 15:30:45 +0000 From: Dave Martin To: Marc Zyngier Subject: Re: [PATCH v5 22/26] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths Message-ID: <20190307153042.GL3567@e103592.cambridge.arm.com> References: <1550519559-15915-1-git-send-email-Dave.Martin@arm.com> <1550519559-15915-23-git-send-email-Dave.Martin@arm.com> <34b7691f-7acb-c9aa-6e53-2857e429f148@arm.com> <20190226121347.GT3567@e103592.cambridge.arm.com> <20190301145503.GI3567@e103592.cambridge.arm.com> <5a3fda6d-5da8-8aa9-9b0e-36d6bcd7441b@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5a3fda6d-5da8-8aa9-9b0e-36d6bcd7441b@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190307_073052_044479_52839681 X-CRM114-Status: GOOD ( 30.61 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Okamoto Takayuki , Christoffer Dall , Ard Biesheuvel , Catalin Marinas , Julien Thierry , Will Deacon , Zhang Lei , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Mar 07, 2019 at 01:47:09PM +0000, Marc Zyngier wrote: > Hi Dave, > > On 01/03/2019 14:55, Dave Martin wrote: > > [FYI Peter, your views on the proposal outlined torward the end of the > > mail would be appreciated.] > > > > On Fri, Mar 01, 2019 at 01:28:19PM +0000, Julien Thierry wrote: > > [...] > > >> I might be over-thinking it, but if there is a way to move that > >> finalization call I'd prefer that. > > > > I know what you mean, but there's not really another clear place to put > > it either, right now. Making finalization a side-effect of only KVM_RUN > > and KVM_GET_REG_LIST would mean that if userspace is squirting in the > > state of a migrated vcpu, a dummy KVM_GET_REG_LIST call would need to be > > inserted between setting KVM_REG_ARM64_SVE_VLS and setting the other SVE > > regs. > > > > One thing we could to is get rid of the implicit finalization behaviour > > completely, and require an explicit ioctl KVM_ARM_VCPU_FINALIZE to do > > +1. In the past, implicit finalization has been a major pain, and the > "implicit GICv2 configuration" has been an absolute disaster. > > > this. This makes a clear barrier between reg writes that manipulate the > > "physical" configuration of the vcpu, and reg reads/writes that merely > > manipulate the vcpu's runtime state. Say: > > > > KVM_ARM_VCPU_INIT(features[] = KVM_ARM_VCPU_SVE) -> ok > > KVM_RUN -> -EPERM (too early) > > KVM_GET_REG_LIST -> -EPERM (too early) > > ... > > KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_VLS) -> ok > > KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_ZREG(0, 0)) -> -EPERM (too early) > > ... > > KVM_RUN -> -EPERM (too early) > > ... > > KVM_ARM_VCPU_FINALIZE -> ok > > KVM_ARM_VCPU_FINALIZE -> -EPERM (too late) > > ... > > KVM_REG_REG_LIST -> ok > > KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_VLS) -> -EPERM (too late) > > KVM_SET_ONE_REG(KVM_REG_ARM64_SVE_ZREG(0, 0)) -> ok > > KVM_RUN -> ok > > KVM_ARM_VCPU_FINALIZE -> -EPERM (too late) > > > > This would change all existing kvm_arm_vcpu_finalize() calls to > > > > if (!kvm_arm_vcpu_finalized()) > > return -EPERM; > > > > (or similar). > > I find this rather compelling, assuming we can easily map features that > are associated to finalization. OK ... thanks for taking a look. > > Without an affected vcpu feature enabled, for backwards compatibility > > we would not require the KVM_ARM_VCPU_FINALIZE call (or perhaps even > > forbid it, since userspace that wants to backwards compatible cannot > > use the new ioctl anyway. I'm in two minds about this. Either way, > > calling _FINALIZE on an old kvm is harmless, so long as userspace knows > > to ignore this failure case.) > > > > The backwards-compatible flow would be: > > > > KVM_ARM_VCPU_INIT(features[] = 0) -> ok > > ... > > KVM_ARM_VCPU_FINALIZE -> ok (or -EPERM) > > ... > > KVM_RUN -> ok > > KVM_ARM_VCPU_FINALIZE -> -EPERM (too late) > > > > > > Thoughts? > > This goes back to the above question: how do we ensure that userspace > knows which features are subject to being finalized. As it is currently > described, KVM_ARM_VCPU_FINALIZE isn't qualified by a feature set, nor > can the kernel report what feature flags requires being finalized. It is > also unclear to me whether all "finalizable" features would have the > same init cycle. So, it's not clear whether any other features will ever need finalization, and even if they do there's a fair chance they won't be interdependent with SVE in such a way as to require multiple finalization steps. For now, it's seems reasonable to make the finalization call a no-op when no finalization is required. Where finalization is required, KVM_ARM_VCPU_FINALIZE becomes mandatory, but the requirement is a) strictly opt-in, and b) userspace will quickly discover if it gets this wrong. For this reason I'd rather not have any explicit reporting of whether finalization is needed or not (or why): we just document and enforce at run-time. > So either we take the simplest approach and make KVM_ARM_VCPU_FINALIZE > strictly SVE specific (renaming it in the process), or it takes some > argument that allow specifying the feature set it applies to. Maybe we > can get away with the kernel not reporting which features require to be > finalized as long that it is clearly documented. OK, what about: * Userspace treats KVM_ARM_VCPU_FINALIZE() -> EINVAL as no error (so that userspace can simply insert this into its init sequence, even on old kernels). * We add an argument, so that KVM_ARM_VCPU_FINALIZE(0) means "finalize default stuff, including SVE". We can add explicit flags later if needed to finalize individual features separately. I don't know whether any features ever will have interdependent finalization requirements, but this should help get us off the hook if so. Question: * KVM_REG_ARM64_SVE_VLS is a weird register because of its special sequencing requirements. The main reason for this is to make it easier to detect migration to a mismatched destination node. But userspace needs some explicit code to make all this work anyway, so should we just have a couple of ioctls to get/set it instead? If there's no strong view either way, I'll leave it as-is, to minimise churn. [...] > Please do your best to have something as close as possible to the final > version for -rc1. From that point on, I'd expect changes to be mostly > cosmetic. This ought to be feasible. The changes being discussed so far are non- invasive. Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel