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=-9.0 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_NEOMUTT 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 93937C4360F for ; Fri, 5 Apr 2019 15:41:48 +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 64D252175B for ; Fri, 5 Apr 2019 15:41:48 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="n8ZDoswG" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 64D252175B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.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=A8MFyusV167CErolDq4uZSBuDp90LLWmnB9iOzwDvdE=; b=n8ZDoswGO3NbxH GHybxg6coINlbuctVInWAFgZ6QGOQVxHYzW2WMWgEQuBpcsVCQr9KFH3gGGWfpAs3BYd/42B8H2Nn 5D5CJ/TNDpv8+ELIqOjn2giFchTzH8/cgsGp0LRF76Z9HqCF/++pQyysyveyIQ6rfVUjXH1fT0M+v Qbm48oAKfM+UMJiL00CYNZwKbMNKj1char42HtM/RleBFWVNgSF2nWBETM72ZV30YAxWMHrOJvaVR /J9l6okqI1RSwgGgl0Pc8EEDHNbXvlabCZHx5u4f0CzMoPBT3ehBWVqOouxeBW7yTNzbG08TYGJCb XXiW2tbrsyETtHHuS4Gw==; 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 1hCQyN-0005Jb-3R; Fri, 05 Apr 2019 15:41:43 +0000 Received: from mx1.redhat.com ([209.132.183.28]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1hCQyH-0005JA-TM for linux-arm-kernel@lists.infradead.org; Fri, 05 Apr 2019 15:41:39 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4CA8A30B4AC7; Fri, 5 Apr 2019 15:41:37 +0000 (UTC) Received: from kamzik.brq.redhat.com (ovpn-204-32.brq.redhat.com [10.40.204.32]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 62040601B3; Fri, 5 Apr 2019 15:41:34 +0000 (UTC) Date: Fri, 5 Apr 2019 17:41:31 +0200 From: Andrew Jones To: Dave Martin Subject: Re: [PATCH v7 23/27] KVM: arm64/sve: Add pseudo-register for the guest's vector lengths Message-ID: <20190405154131.ecbq33b3o7pvfmrw@kamzik.brq.redhat.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> <20190405093610.GN3567@e103592.cambridge.arm.com> <20190405102204.m4depbrkn5f3btnz@kamzik.brq.redhat.com> <20190405140640.GC3567@e103592.cambridge.arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190405140640.GC3567@e103592.cambridge.arm.com> User-Agent: NeoMutt/20180716 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Fri, 05 Apr 2019 15:41:37 +0000 (UTC) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190405_084137_987711_DF59D820 X-CRM114-Status: GOOD ( 37.71 ) 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 Fri, Apr 05, 2019 at 03:06:40PM +0100, Dave Martin wrote: > On Fri, Apr 05, 2019 at 12:22:04PM +0200, Andrew Jones wrote: > > On Fri, Apr 05, 2019 at 10:36:10AM +0100, Dave Martin wrote: > > > 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? > > > > Being able to do additional vcpu ioctls with only partially initialized > > sve (sve_state is still null, but we otherwise believe the vcpu has sve). > > That sounds risky. Although I see we only set KVM_ARM64_VCPU_SVE_FINALIZED > > when everything, like the memory allocation, succeeds, so we're probably > > fine. Indeed having KVM_ARM64_GUEST_HAS_SVE and not KVM_ARM64_VCPU_SVE_FINALIZED > > is likely the safest vcpu state for a vcpu that failed to finalize sve. > > This was my rationale: every non-trivial operation that is affected by > SVE operation checks for kvm_arm_vcpu_sve_finalized(): accessing > KVM_REG_ARM64_SVE_VLS should be the only exception. > > Of course, people might forget that in new code, but I think all the > major ABI code paths that are likely to exist for SVE are already there > now. > > Does this sound OK, or do you still have concerns? > > Clearing KVM_ARM64_GUEST_HAS_SVE could be done on finalization failure; > it just feels a bit weird. Let's leave it. It's safe now, and there's always risk new code could screw something up, no matter which way we go now, so let's pick the simpler way. Thanks, drew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel