From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Martin Subject: Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths Date: Fri, 5 Apr 2019 10:36:10 +0100 Message-ID: <20190405093610.GN3567@e103592.cambridge.arm.com> References: <1553864452-15080-1-git-send-email-Dave.Martin@arm.com> <1553864452-15080-24-git-send-email-Dave.Martin@arm.com> <20190404203109.lrt5czzlqpritecd@kamzik.brq.redhat.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 122E14A480 for ; Fri, 5 Apr 2019 05:36:17 -0400 (EDT) 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 ScAvy4vgH8ZK for ; Fri, 5 Apr 2019 05:36:15 -0400 (EDT) Received: from foss.arm.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by mm01.cs.columbia.edu (Postfix) with ESMTP id B4DC94A426 for ; Fri, 5 Apr 2019 05:36:15 -0400 (EDT) Content-Disposition: inline In-Reply-To: <20190404203109.lrt5czzlqpritecd@kamzik.brq.redhat.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: Andrew Jones Cc: Okamoto Takayuki , Christoffer Dall , Ard Biesheuvel , Marc Zyngier , Catalin Marinas , Will Deacon , Zhang Lei , Julien Grall , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org List-Id: kvmarm@lists.cs.columbia.edu On Thu, Apr 04, 2019 at 10:31:09PM +0200, Andrew Jones wrote: > On Fri, Mar 29, 2019 at 01:00:48PM +0000, Dave Martin wrote: > > This patch adds a new pseudo-register KVM_REG_ARM64_SVE_VLS to > > allow userspace to set and query the set of vector lengths visible > > to the guest. > > > > In the future, multiple register slices per SVE register may be > > visible through the ioctl interface. Once the set of slices has > > been determined we would not be able to allow the vector length set > > to be changed any more, in order to avoid userspace seeing > > inconsistent sets of registers. For this reason, this patch adds > > support for explicit finalization of the SVE configuration via the > > KVM_ARM_VCPU_FINALIZE ioctl. > > > > Finalization is the proper place to allocate the SVE register state > > storage in vcpu->arch.sve_state, so this patch adds that as > > appropriate. The data is freed via kvm_arch_vcpu_uninit(), which > > was previously a no-op on arm64. > > > > To simplify the logic for determining what vector lengths can be > > supported, some code is added to KVM init to work this out, in the > > kvm_arm_init_arch_resources() hook. > > > > The KVM_REG_ARM64_SVE_VLS pseudo-register is not exposed yet. > > Subsequent patches will allow SVE to be turned on for guest vcpus, > > making it visible. > > > > Signed-off-by: Dave Martin > > Reviewed-by: Julien Thierry > > Tested-by: zhang.lei [...] > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c [...] > > +/* > > + * Finalize vcpu's maximum SVE vector length, allocating > > + * vcpu->arch.sve_state as necessary. > > + */ > > +static int kvm_vcpu_finalize_sve(struct kvm_vcpu *vcpu) > > +{ > > + void *buf; > > + unsigned int vl; > > + > > + vl = vcpu->arch.sve_max_vl; > > + > > + /* > > + * Resposibility for these properties is shared between > > + * kvm_arm_init_arch_resources(), kvm_vcpu_enable_sve() and > > + * set_sve_vls(). Double-check here just to be sure: > > + */ > > + if (WARN_ON(!sve_vl_valid(vl) || vl > sve_max_virtualisable_vl || > > + vl > SVE_VL_ARCH_MAX)) > > + return -EIO; > > + > > + buf = kzalloc(SVE_SIG_REGS_SIZE(sve_vq_from_vl(vl)), GFP_KERNEL); > > + if (!buf) > > + return -ENOMEM; > > + > > + vcpu->arch.sve_state = buf; > > + vcpu->arch.flags |= KVM_ARM64_VCPU_SVE_FINALIZED; > > + return 0; > > +} > > + > > +int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what) > > +{ > > + switch (what) { > > + case KVM_ARM_VCPU_SVE: > > + if (!vcpu_has_sve(vcpu)) > > + return -EINVAL; > > + > > + if (kvm_arm_vcpu_sve_finalized(vcpu)) > > + return -EPERM; > > + > > + return kvm_vcpu_finalize_sve(vcpu); > > In the next patch I see that we already have KVM_ARM64_GUEST_HAS_SVE > set in vcpu->arch.flags at this point. So if this kvm_vcpu_finalize_sve() > call fails, shouldn't we unset KVM_ARM64_GUEST_HAS_SVE and anything > else used to indicate the vcpu has sve? If this fails, either userspace did the wrong thing, or there was some fatal error (like the ENOMEM case). Either way, the vcpu is not runnable and userspace is expected to bail out. Further, KVM_ARM_VCPU_INIT fixes the set of features requested by userspace: we shouldn't change the set of features under userspace's feet and try to limp on somehow. We have no means for userspace to find out that some features went away in the meantime... So, what would you be trying to achieve with this change? [...] 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=-8.5 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED,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 CA0A9C4360F for ; Fri, 5 Apr 2019 09:36:42 +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 99456217D4 for ; Fri, 5 Apr 2019 09:36:42 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="EVWeWecr" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 99456217D4 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=e5vMta4UDpaKHtwbfv69gwneYBAgUbfhyQHdvtqrao4=; b=EVWeWecraXcqIS /jCMyqPmFupv+aVa0+CkT/b8N0LtCIUr2E2aDsybA2HdiP0WxXk5iPF1H8RcYN3ZjhBuj93GoN5Qa xgoNF30BW1al93QeuYLIdxg/8KUKK/XtOIDXN8kGHzo8kbTZVjLyip7B7KDaHUL2FByeh2sGLVc36 bT/LsXgz2bERSRbY4ovZ5WmYCyS8uurp3sOX/UhAbUykKa6yK4RiR1nR1iCMcYF3gNAsjJSfYo4TO NrdJBg0lJI//CLfPIianv0+iJQc4Dot4upGH6MV85Q6+WR7eWmNMIZWOeM6tBBRtwv8ram2MkQVfJ krb3WB/Ng86vsLBA5+tA==; 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 1hCLH3-0003aX-81; Fri, 05 Apr 2019 09:36:37 +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 1hCLGh-00039b-IB for linux-arm-kernel@lists.infradead.org; Fri, 05 Apr 2019 09:36:24 +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 30EDC16A3; Fri, 5 Apr 2019 02:36:15 -0700 (PDT) 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 3FB1A3F721; Fri, 5 Apr 2019 02:36:13 -0700 (PDT) Date: Fri, 5 Apr 2019 10:36:10 +0100 From: Dave Martin To: Andrew Jones Subject: Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths Message-ID: <20190405093610.GN3567@e103592.cambridge.arm.com> References: <1553864452-15080-1-git-send-email-Dave.Martin@arm.com> <1553864452-15080-24-git-send-email-Dave.Martin@arm.com> <20190404203109.lrt5czzlqpritecd@kamzik.brq.redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190404203109.lrt5czzlqpritecd@kamzik.brq.redhat.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-20190405_023616_157074_DBD7FDF4 X-CRM114-Status: GOOD ( 29.07 ) 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 , Marc Zyngier , Catalin Marinas , Will Deacon , Zhang Lei , Julien Grall , 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, Apr 04, 2019 at 10:31:09PM +0200, Andrew Jones wrote: > On Fri, Mar 29, 2019 at 01:00:48PM +0000, Dave Martin wrote: > > This patch adds a new pseudo-register KVM_REG_ARM64_SVE_VLS to > > allow userspace to set and query the set of vector lengths visible > > to the guest. > > > > In the future, multiple register slices per SVE register may be > > visible through the ioctl interface. Once the set of slices has > > been determined we would not be able to allow the vector length set > > to be changed any more, in order to avoid userspace seeing > > inconsistent sets of registers. For this reason, this patch adds > > support for explicit finalization of the SVE configuration via the > > KVM_ARM_VCPU_FINALIZE ioctl. > > > > Finalization is the proper place to allocate the SVE register state > > storage in vcpu->arch.sve_state, so this patch adds that as > > appropriate. The data is freed via kvm_arch_vcpu_uninit(), which > > was previously a no-op on arm64. > > > > To simplify the logic for determining what vector lengths can be > > supported, some code is added to KVM init to work this out, in the > > kvm_arm_init_arch_resources() hook. > > > > The KVM_REG_ARM64_SVE_VLS pseudo-register is not exposed yet. > > Subsequent patches will allow SVE to be turned on for guest vcpus, > > making it visible. > > > > Signed-off-by: Dave Martin > > Reviewed-by: Julien Thierry > > Tested-by: zhang.lei [...] > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c [...] > > +/* > > + * Finalize vcpu's maximum SVE vector length, allocating > > + * vcpu->arch.sve_state as necessary. > > + */ > > +static int kvm_vcpu_finalize_sve(struct kvm_vcpu *vcpu) > > +{ > > + void *buf; > > + unsigned int vl; > > + > > + vl = vcpu->arch.sve_max_vl; > > + > > + /* > > + * Resposibility for these properties is shared between > > + * kvm_arm_init_arch_resources(), kvm_vcpu_enable_sve() and > > + * set_sve_vls(). Double-check here just to be sure: > > + */ > > + if (WARN_ON(!sve_vl_valid(vl) || vl > sve_max_virtualisable_vl || > > + vl > SVE_VL_ARCH_MAX)) > > + return -EIO; > > + > > + buf = kzalloc(SVE_SIG_REGS_SIZE(sve_vq_from_vl(vl)), GFP_KERNEL); > > + if (!buf) > > + return -ENOMEM; > > + > > + vcpu->arch.sve_state = buf; > > + vcpu->arch.flags |= KVM_ARM64_VCPU_SVE_FINALIZED; > > + return 0; > > +} > > + > > +int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what) > > +{ > > + switch (what) { > > + case KVM_ARM_VCPU_SVE: > > + if (!vcpu_has_sve(vcpu)) > > + return -EINVAL; > > + > > + if (kvm_arm_vcpu_sve_finalized(vcpu)) > > + return -EPERM; > > + > > + return kvm_vcpu_finalize_sve(vcpu); > > In the next patch I see that we already have KVM_ARM64_GUEST_HAS_SVE > set in vcpu->arch.flags at this point. So if this kvm_vcpu_finalize_sve() > call fails, shouldn't we unset KVM_ARM64_GUEST_HAS_SVE and anything > else used to indicate the vcpu has sve? If this fails, either userspace did the wrong thing, or there was some fatal error (like the ENOMEM case). Either way, the vcpu is not runnable and userspace is expected to bail out. Further, KVM_ARM_VCPU_INIT fixes the set of features requested by userspace: we shouldn't change the set of features under userspace's feet and try to limp on somehow. We have no means for userspace to find out that some features went away in the meantime... So, what would you be trying to achieve with this change? [...] Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel