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 975B6C4360F for ; Tue, 2 Apr 2019 08:59:37 +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 65DAF2075E for ; Tue, 2 Apr 2019 08:59:37 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="OiXezaqG" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 65DAF2075E 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=pYAovVdqIVZnbKK/iDC5Qfg50HD2rEhYr2jqO/b2akk=; b=OiXezaqGvDpxWg P6xA3VQ3P6aeQoL5m47cm6rEVL3VAXDjR2vU/NpcWCFajcDkt9C+74ia54Dg3H+a6lX/CQuFDj05R C7qyMPq/nLxjYylgRgcsRjFzqFehs03PsMF+f3HoVgfGSreDO1JtzJQPNpv4iN+kC/jg+FjVxM8vO J1LnFWbwVDhLYeKOUZlheWx0XUeiDjzVIhbnK0/eiPOLT0eckS62XqDmGFQguplhigNtkknI5VMDu QA4gY9qGhDj2rzPNxqP8OQcttozvu+1vk/RG/rSwvDqYrLK4fxlJtpTrdCjSAyMi8J4h2rdQkH3cb j3lEF2mv27bK27Ql45/Q==; 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 1hBFGU-0001aM-0z; Tue, 02 Apr 2019 08:59:30 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hBFGQ-0001Zj-46 for linux-arm-kernel@lists.infradead.org; Tue, 02 Apr 2019 08:59:27 +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 152D880D; Tue, 2 Apr 2019 01:59:24 -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 47A8B3F690; Tue, 2 Apr 2019 01:59:22 -0700 (PDT) Date: Tue, 2 Apr 2019 09:59:19 +0100 From: Dave Martin To: Marc Zyngier Subject: Re: [PATCH v7 16/27] KVM: arm64: Factor out core register ID enumeration Message-ID: <20190402085917.GF3567@e103592.cambridge.arm.com> References: <1553864452-15080-1-git-send-email-Dave.Martin@arm.com> <1553864452-15080-17-git-send-email-Dave.Martin@arm.com> <86ftr15dsb.wl-marc.zyngier@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <86ftr15dsb.wl-marc.zyngier@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-20190402_015926_178000_CBAC3476 X-CRM114-Status: GOOD ( 33.37 ) 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 , 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 Tue, Apr 02, 2019 at 03:41:56AM +0100, Marc Zyngier wrote: > On Fri, 29 Mar 2019 13:00:41 +0000, > Dave Martin wrote: > > > > In preparation for adding logic to filter out some KVM_REG_ARM_CORE > > registers from the KVM_GET_REG_LIST output, this patch factors out > > the core register enumeration into a separate function and rebuilds > > num_core_regs() on top of it. > > > > This may be a little more expensive (depending on how good a job > > the compiler does of specialising the code), but KVM_GET_REG_LIST > > is not a hot path. > > > > This will make it easier to consolidate ID filtering code in one > > place. > > > > No functional change. > > > > Signed-off-by: Dave Martin > > Reviewed-by: Julien Thierry > > Tested-by: zhang.lei > > > > --- > > > > Changes since v5: > > > > * New patch. > > > > This reimplements part of the separately-posted patch "KVM: arm64: > > Factor out KVM_GET_REG_LIST core register enumeration", minus aspects > > that potentially break the ABI. > > > > As a result, the opportunity to truly consolidate all the ID reg > > filtering in one place is deliberately left on the floor, for now. > > This will be addressed in a separate series later on. > > --- > > arch/arm64/kvm/guest.c | 33 +++++++++++++++++++++++++-------- > > 1 file changed, 25 insertions(+), 8 deletions(-) > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > index 3e38eb2..a391a61 100644 > > --- a/arch/arm64/kvm/guest.c > > +++ b/arch/arm64/kvm/guest.c [...] > > @@ -276,15 +296,12 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu) > > */ > > int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) > > { > > - unsigned int i; > > - const u64 core_reg = KVM_REG_ARM64 | KVM_REG_SIZE_U64 | KVM_REG_ARM_CORE; > > int ret; > > > > - for (i = 0; i < sizeof(struct kvm_regs) / sizeof(__u32); i++) { > > - if (put_user(core_reg | i, uindices)) > > - return -EFAULT; > > - uindices++; > > - } > > + ret = kvm_arm_copy_core_reg_indices(uindices); > > + if (ret) > > + return ret; > > + uindices += ret; > > Interesting snippet. Given that most implementations have at least one > register, this can hardly work. Please do test things with QEMU, and > not only kvmtool which obviously doesn't exercise this path. My bad: this used to work to do the right thing, but I broke it when splitting up [1] for v6 to avoid the dependency. kvm_arm_copy_core_reg_indices() used to take &uindices and update it directly, returning 0 on success instead of the number of registers. But this seemed less consistent with the way the other functions are called. > For the sake of getting -next back to a vaguely usable state, I've now > queued the following patch on top. > > M. > > From 832401c8912680ee56dc5cb6ab101266b3db416a Mon Sep 17 00:00:00 2001 > From: Marc Zyngier > Date: Tue, 2 Apr 2019 03:28:39 +0100 > Subject: [PATCH] arm64: KVM: Fix system register enumeration > > The introduction of the SVE registers to userspace started with a > refactoring of the way we expose any register via the ONE_REG > interface. > > Unfortunately, this change doesn't exactly behave as expected > if the number of registers is non-zero and consider everything > to be an error. The visible result is that QEMU barfs very early > when creating vcpus. > > Make sure we only exit early in case there is an actual error, rather > than a positive number of registers... > > be25bbb392fa ("KVM: arm64: Factor out core register ID enumeration") > Signed-off-by: Marc Zyngier > --- > arch/arm64/kvm/guest.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 086ab0508d69..4f7b26bbf671 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -604,22 +604,22 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) > int ret; > > ret = copy_core_reg_indices(vcpu, uindices); > - if (ret) > + if (ret < 0) > return ret; > uindices += ret; > > ret = copy_sve_reg_indices(vcpu, uindices); > - if (ret) > + if (ret < 0) > return ret; > uindices += ret; ^ Ack > ret = kvm_arm_copy_fw_reg_indices(vcpu, uindices); > - if (ret) > + if (ret < 0) > return ret; > uindices += kvm_arm_get_fw_num_regs(vcpu); > > ret = copy_timer_indices(vcpu, uindices); > - if (ret) > + if (ret < 0) > return ret; > uindices += NUM_TIMER_REGS; For these two, the interface is not really the same. These don't return the number of registers, so return 0 on success. "< 0" here could be a trap for the future, though the risk looks low. I can have a go at some rework on top to make this more consistent, but I'd rather not muddy the water further for the moment. Any view on that? Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel