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=-6.8 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 739CFC433E0 for ; Mon, 6 Jul 2020 12:15:25 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id EA7992070C for ; Mon, 6 Jul 2020 12:15:24 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="saieF4wX" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EA7992070C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvmarm-bounces@lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 75B714B3D6; Mon, 6 Jul 2020 08:15:24 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@kernel.org 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 JyxYmc4BxrUd; Mon, 6 Jul 2020 08:15:23 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 551374B398; Mon, 6 Jul 2020 08:15:23 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 3662E4B392 for ; Mon, 6 Jul 2020 08:15:22 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu 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 0wvY6ajbtOML for ; Mon, 6 Jul 2020 08:15:21 -0400 (EDT) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id E259E4B1FA for ; Mon, 6 Jul 2020 08:15:20 -0400 (EDT) Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id A2C61206F5; Mon, 6 Jul 2020 12:15:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594037719; bh=Ln+M2VEKf+2mtxon2h+buvSO35oHmKw8UDIzRkCs7sA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=saieF4wXnaKrbWcQ5wO2kfilaGu99az6f3JpECUt5GG3aTMC2oGsFBTUettThgzPo 3SKUHtcXYqY5+H9rHbd8VcoaBwyNyWMMvaKddoHl9Vrb/WbcauHdy9jXJcqGk+M5hs xj8axU4pzxZSt0bkqp09RKxTIR0kqec2TsGaT8xc= Received: from disco-boy.misterjones.org ([51.254.78.96] helo=www.loen.fr) by disco-boy.misterjones.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1jsQ1m-009Rbc-6A; Mon, 06 Jul 2020 13:15:18 +0100 MIME-Version: 1.0 Date: Mon, 06 Jul 2020 13:15:18 +0100 From: Marc Zyngier To: Alexandru Elisei Subject: Re: [PATCH v2 06/17] KVM: arm64: Introduce accessor for ctxt->sys_reg In-Reply-To: References: <20200615132719.1932408-1-maz@kernel.org> <20200615132719.1932408-7-maz@kernel.org> User-Agent: Roundcube Webmail/1.4.5 Message-ID: <2595cd556bcb8bd996f60ef527b512ef@kernel.org> X-Sender: maz@kernel.org X-SA-Exim-Connect-IP: 51.254.78.96 X-SA-Exim-Rcpt-To: alexandru.elisei@arm.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, andre.przywara@arm.com, christoffer.dall@arm.com, Dave.Martin@arm.com, jintack@cs.columbia.edu, gcherian@marvell.com, prime.zeng@hisilicon.com, ascull@google.com, will@kernel.org, catalin.marinas@arm.com, mark.rutland@arm.com, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, kernel-team@android.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Cc: kernel-team@android.com, kvm@vger.kernel.org, Andre Przywara , kvmarm@lists.cs.columbia.edu, George Cherian , "Zengtao \(B\)" , Catalin Marinas , Will Deacon , Dave Martin , linux-arm-kernel@lists.infradead.org X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu Hi Alex, On 2020-06-26 16:39, Alexandru Elisei wrote: > Hi, > > On 6/15/20 2:27 PM, Marc Zyngier wrote: >> In order to allow the disintegration of the per-vcpu sysreg array, >> let's introduce a new helper (ctxt_sys_reg()) that returns the >> in-memory copy of a system register, picked from a given context. >> >> __vcpu_sys_reg() is rewritten to use this helper. >> >> Signed-off-by: Marc Zyngier >> --- >> arch/arm64/include/asm/kvm_host.h | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm64/include/asm/kvm_host.h >> b/arch/arm64/include/asm/kvm_host.h >> index e7fd03271e52..5314399944e7 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -405,12 +405,17 @@ struct kvm_vcpu_arch { >> #define vcpu_gp_regs(v) (&(v)->arch.ctxt.gp_regs) >> >> /* >> - * Only use __vcpu_sys_reg if you know you want the memory backed >> version of a >> - * register, and not the one most recently accessed by a running >> VCPU. For >> - * example, for userspace access or for system registers that are >> never context >> - * switched, but only emulated. >> + * Only use __vcpu_sys_reg/ctxt_sys_reg if you know you want the >> + * memory backed version of a register, and not the one most recently >> + * accessed by a running VCPU. For example, for userspace access or >> + * for system registers that are never context switched, but only >> + * emulated. >> */ >> -#define __vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)]) >> +#define __ctxt_sys_reg(c,r) (&(c)->sys_regs[(r)]) >> + >> +#define ctxt_sys_reg(c,r) (*__ctxt_sys_reg(c,r)) >> + >> +#define __vcpu_sys_reg(v,r) (ctxt_sys_reg(&(v)->arch.ctxt, (r))) > > This is confusing - __vcpu_sys_reg() returns the value, but > __ctxt_sys_reg() > return a pointer to the value. Because of that, I made the mistake of > thinking > that __vcpu_sys_reg() returns a pointer when reviewing the next patch > in the > series, and I got really worried that stuff was seriously broken (it > was not). This is intentional (the behaviour, not the confusing aspect... ;-), as __ctx_sys_reg() gets further rewritten as such: -#define __ctxt_sys_reg(c,r) (&(c)->sys_regs[(r)]) +static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r) +{ + if (unlikely(r >= __VNCR_START__ && ctxt->vncr_array)) + return &ctxt->vncr_array[r - __VNCR_START__]; + + return (u64 *)&ctxt->sys_regs[r]; +} to deal with the VNCR page (depending on whether you use nesting or not, the sysreg is backed by the VNCR page or the usual sysreg array). To be clear, there shouldn't be much use of __ctxt_sys_reg (there is only 3 in the current code), all for good reasons (core_reg_addr definitely wants the address of a register). > I'm not sure what the reasonable solution is, or even if there is one. > > Some thoughts: we could have just one macro, ctxt_sys_reg() and > dereference that > when we want the value; we could keep both and swap the macro > definitions; or we > could encode the fact that a macro returns a pointer in the macro name > (so we > would end up with __ctxt_sys_reg() -> __ctxt_sys_regp() and > ctxt_sys_reg -> > __ctxt_sys_reg()). > > What do you think? I'm not opposed to any of this, provided that it doesn't create unnecessary churn and additional confusion. I'll keep it as such in the meantime, but I'm definitely willing to take a patch going over this if you think this is necessary. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm 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=-7.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS autolearn=unavailable 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 7C6FDC433DF for ; Mon, 6 Jul 2020 12:16:51 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 499BE2070C for ; Mon, 6 Jul 2020 12:16:51 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="lHV+3j2I"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="saieF4wX" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 499BE2070C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+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=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Message-ID:References:In-Reply-To:Subject:To:From: Date:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=sifZvazpisuXd6Eh1TjnpenUybZPBQtpHcqnPi2/4/g=; b=lHV+3j2IUJUFEd5QJJq1dAdOD uxZPdB/diAyyZKQ/xv2RCqITN0pq/19YCyEs6supV0v01VA8TiNzbyWFSbrg/qNUHPADfI2G23Cml FgJb5KaNPVk20OANbIZud7J+y6CL6cZSy4CKj8RE4bTTQksBntF2U/31rTQVR7V68QvVAErHyUsyF lmNCj4iNR8T5cg2uK4+5uBf1NsP90RkM9ZbPIIfApuXy4t2JjOwlb9r+yy5T0Sw4JjNq58SZMyIli sZR60ULY3kTFVczsozItKliJt/Nqiw+DqYF+g9orOPBMUUyOrUCUG9jiqx92PFPhE3MWXEBHyqPAz f/RtAadNA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jsQ1q-0005pC-RU; Mon, 06 Jul 2020 12:15:22 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jsQ1o-0005o6-IO for linux-arm-kernel@lists.infradead.org; Mon, 06 Jul 2020 12:15:21 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id A2C61206F5; Mon, 6 Jul 2020 12:15:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594037719; bh=Ln+M2VEKf+2mtxon2h+buvSO35oHmKw8UDIzRkCs7sA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=saieF4wXnaKrbWcQ5wO2kfilaGu99az6f3JpECUt5GG3aTMC2oGsFBTUettThgzPo 3SKUHtcXYqY5+H9rHbd8VcoaBwyNyWMMvaKddoHl9Vrb/WbcauHdy9jXJcqGk+M5hs xj8axU4pzxZSt0bkqp09RKxTIR0kqec2TsGaT8xc= Received: from disco-boy.misterjones.org ([51.254.78.96] helo=www.loen.fr) by disco-boy.misterjones.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1jsQ1m-009Rbc-6A; Mon, 06 Jul 2020 13:15:18 +0100 MIME-Version: 1.0 Date: Mon, 06 Jul 2020 13:15:18 +0100 From: Marc Zyngier To: Alexandru Elisei Subject: Re: [PATCH v2 06/17] KVM: arm64: Introduce accessor for ctxt->sys_reg In-Reply-To: References: <20200615132719.1932408-1-maz@kernel.org> <20200615132719.1932408-7-maz@kernel.org> User-Agent: Roundcube Webmail/1.4.5 Message-ID: <2595cd556bcb8bd996f60ef527b512ef@kernel.org> X-Sender: maz@kernel.org X-SA-Exim-Connect-IP: 51.254.78.96 X-SA-Exim-Rcpt-To: alexandru.elisei@arm.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, andre.przywara@arm.com, christoffer.dall@arm.com, Dave.Martin@arm.com, jintack@cs.columbia.edu, gcherian@marvell.com, prime.zeng@hisilicon.com, ascull@google.com, will@kernel.org, catalin.marinas@arm.com, mark.rutland@arm.com, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, kernel-team@android.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200706_081520_787312_9FDED15A X-CRM114-Status: GOOD ( 26.79 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , kernel-team@android.com, kvm@vger.kernel.org, Suzuki K Poulose , Jintack Lim , Andre Przywara , Christoffer Dall , kvmarm@lists.cs.columbia.edu, George Cherian , James Morse , Andrew Scull , "Zengtao \(B\)" , Catalin Marinas , Julien Thierry , Will Deacon , Dave Martin , linux-arm-kernel@lists.infradead.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Alex, On 2020-06-26 16:39, Alexandru Elisei wrote: > Hi, > > On 6/15/20 2:27 PM, Marc Zyngier wrote: >> In order to allow the disintegration of the per-vcpu sysreg array, >> let's introduce a new helper (ctxt_sys_reg()) that returns the >> in-memory copy of a system register, picked from a given context. >> >> __vcpu_sys_reg() is rewritten to use this helper. >> >> Signed-off-by: Marc Zyngier >> --- >> arch/arm64/include/asm/kvm_host.h | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm64/include/asm/kvm_host.h >> b/arch/arm64/include/asm/kvm_host.h >> index e7fd03271e52..5314399944e7 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -405,12 +405,17 @@ struct kvm_vcpu_arch { >> #define vcpu_gp_regs(v) (&(v)->arch.ctxt.gp_regs) >> >> /* >> - * Only use __vcpu_sys_reg if you know you want the memory backed >> version of a >> - * register, and not the one most recently accessed by a running >> VCPU. For >> - * example, for userspace access or for system registers that are >> never context >> - * switched, but only emulated. >> + * Only use __vcpu_sys_reg/ctxt_sys_reg if you know you want the >> + * memory backed version of a register, and not the one most recently >> + * accessed by a running VCPU. For example, for userspace access or >> + * for system registers that are never context switched, but only >> + * emulated. >> */ >> -#define __vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)]) >> +#define __ctxt_sys_reg(c,r) (&(c)->sys_regs[(r)]) >> + >> +#define ctxt_sys_reg(c,r) (*__ctxt_sys_reg(c,r)) >> + >> +#define __vcpu_sys_reg(v,r) (ctxt_sys_reg(&(v)->arch.ctxt, (r))) > > This is confusing - __vcpu_sys_reg() returns the value, but > __ctxt_sys_reg() > return a pointer to the value. Because of that, I made the mistake of > thinking > that __vcpu_sys_reg() returns a pointer when reviewing the next patch > in the > series, and I got really worried that stuff was seriously broken (it > was not). This is intentional (the behaviour, not the confusing aspect... ;-), as __ctx_sys_reg() gets further rewritten as such: -#define __ctxt_sys_reg(c,r) (&(c)->sys_regs[(r)]) +static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r) +{ + if (unlikely(r >= __VNCR_START__ && ctxt->vncr_array)) + return &ctxt->vncr_array[r - __VNCR_START__]; + + return (u64 *)&ctxt->sys_regs[r]; +} to deal with the VNCR page (depending on whether you use nesting or not, the sysreg is backed by the VNCR page or the usual sysreg array). To be clear, there shouldn't be much use of __ctxt_sys_reg (there is only 3 in the current code), all for good reasons (core_reg_addr definitely wants the address of a register). > I'm not sure what the reasonable solution is, or even if there is one. > > Some thoughts: we could have just one macro, ctxt_sys_reg() and > dereference that > when we want the value; we could keep both and swap the macro > definitions; or we > could encode the fact that a macro returns a pointer in the macro name > (so we > would end up with __ctxt_sys_reg() -> __ctxt_sys_regp() and > ctxt_sys_reg -> > __ctxt_sys_reg()). > > What do you think? I'm not opposed to any of this, provided that it doesn't create unnecessary churn and additional confusion. I'll keep it as such in the meantime, but I'm definitely willing to take a patch going over this if you think this is necessary. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel 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=-7.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS 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 B056BC433E0 for ; Mon, 6 Jul 2020 12:15:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 90E4620702 for ; Mon, 6 Jul 2020 12:15:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594037721; bh=Ln+M2VEKf+2mtxon2h+buvSO35oHmKw8UDIzRkCs7sA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=GeCAMErQdmP+HZTXhwVJYVxDTmc8Ui5dqcCZoF1VW0Wug13yW5Qsm1hKfKYgYA8MU gC5j3+6EGysu9VoHBrpT3E3I1eXh6v+rZ8CDDbEfh3AxNdi5sGvDg2SNYK/6basjgJ /6Xvz9qSxkNrqxy6fOho1PnX0Z6fwiydJWwn5CE0= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729047AbgGFMPU (ORCPT ); Mon, 6 Jul 2020 08:15:20 -0400 Received: from mail.kernel.org ([198.145.29.99]:52338 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727896AbgGFMPU (ORCPT ); Mon, 6 Jul 2020 08:15:20 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id A2C61206F5; Mon, 6 Jul 2020 12:15:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594037719; bh=Ln+M2VEKf+2mtxon2h+buvSO35oHmKw8UDIzRkCs7sA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=saieF4wXnaKrbWcQ5wO2kfilaGu99az6f3JpECUt5GG3aTMC2oGsFBTUettThgzPo 3SKUHtcXYqY5+H9rHbd8VcoaBwyNyWMMvaKddoHl9Vrb/WbcauHdy9jXJcqGk+M5hs xj8axU4pzxZSt0bkqp09RKxTIR0kqec2TsGaT8xc= Received: from disco-boy.misterjones.org ([51.254.78.96] helo=www.loen.fr) by disco-boy.misterjones.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1jsQ1m-009Rbc-6A; Mon, 06 Jul 2020 13:15:18 +0100 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 06 Jul 2020 13:15:18 +0100 From: Marc Zyngier To: Alexandru Elisei Cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, Andre Przywara , Christoffer Dall , Dave Martin , Jintack Lim , George Cherian , "Zengtao (B)" , Andrew Scull , Will Deacon , Catalin Marinas , Mark Rutland , James Morse , Julien Thierry , Suzuki K Poulose , kernel-team@android.com Subject: Re: [PATCH v2 06/17] KVM: arm64: Introduce accessor for ctxt->sys_reg In-Reply-To: References: <20200615132719.1932408-1-maz@kernel.org> <20200615132719.1932408-7-maz@kernel.org> User-Agent: Roundcube Webmail/1.4.5 Message-ID: <2595cd556bcb8bd996f60ef527b512ef@kernel.org> X-Sender: maz@kernel.org X-SA-Exim-Connect-IP: 51.254.78.96 X-SA-Exim-Rcpt-To: alexandru.elisei@arm.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, andre.przywara@arm.com, christoffer.dall@arm.com, Dave.Martin@arm.com, jintack@cs.columbia.edu, gcherian@marvell.com, prime.zeng@hisilicon.com, ascull@google.com, will@kernel.org, catalin.marinas@arm.com, mark.rutland@arm.com, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, kernel-team@android.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Hi Alex, On 2020-06-26 16:39, Alexandru Elisei wrote: > Hi, > > On 6/15/20 2:27 PM, Marc Zyngier wrote: >> In order to allow the disintegration of the per-vcpu sysreg array, >> let's introduce a new helper (ctxt_sys_reg()) that returns the >> in-memory copy of a system register, picked from a given context. >> >> __vcpu_sys_reg() is rewritten to use this helper. >> >> Signed-off-by: Marc Zyngier >> --- >> arch/arm64/include/asm/kvm_host.h | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm64/include/asm/kvm_host.h >> b/arch/arm64/include/asm/kvm_host.h >> index e7fd03271e52..5314399944e7 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -405,12 +405,17 @@ struct kvm_vcpu_arch { >> #define vcpu_gp_regs(v) (&(v)->arch.ctxt.gp_regs) >> >> /* >> - * Only use __vcpu_sys_reg if you know you want the memory backed >> version of a >> - * register, and not the one most recently accessed by a running >> VCPU. For >> - * example, for userspace access or for system registers that are >> never context >> - * switched, but only emulated. >> + * Only use __vcpu_sys_reg/ctxt_sys_reg if you know you want the >> + * memory backed version of a register, and not the one most recently >> + * accessed by a running VCPU. For example, for userspace access or >> + * for system registers that are never context switched, but only >> + * emulated. >> */ >> -#define __vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)]) >> +#define __ctxt_sys_reg(c,r) (&(c)->sys_regs[(r)]) >> + >> +#define ctxt_sys_reg(c,r) (*__ctxt_sys_reg(c,r)) >> + >> +#define __vcpu_sys_reg(v,r) (ctxt_sys_reg(&(v)->arch.ctxt, (r))) > > This is confusing - __vcpu_sys_reg() returns the value, but > __ctxt_sys_reg() > return a pointer to the value. Because of that, I made the mistake of > thinking > that __vcpu_sys_reg() returns a pointer when reviewing the next patch > in the > series, and I got really worried that stuff was seriously broken (it > was not). This is intentional (the behaviour, not the confusing aspect... ;-), as __ctx_sys_reg() gets further rewritten as such: -#define __ctxt_sys_reg(c,r) (&(c)->sys_regs[(r)]) +static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r) +{ + if (unlikely(r >= __VNCR_START__ && ctxt->vncr_array)) + return &ctxt->vncr_array[r - __VNCR_START__]; + + return (u64 *)&ctxt->sys_regs[r]; +} to deal with the VNCR page (depending on whether you use nesting or not, the sysreg is backed by the VNCR page or the usual sysreg array). To be clear, there shouldn't be much use of __ctxt_sys_reg (there is only 3 in the current code), all for good reasons (core_reg_addr definitely wants the address of a register). > I'm not sure what the reasonable solution is, or even if there is one. > > Some thoughts: we could have just one macro, ctxt_sys_reg() and > dereference that > when we want the value; we could keep both and swap the macro > definitions; or we > could encode the fact that a macro returns a pointer in the macro name > (so we > would end up with __ctxt_sys_reg() -> __ctxt_sys_regp() and > ctxt_sys_reg -> > __ctxt_sys_reg()). > > What do you think? I'm not opposed to any of this, provided that it doesn't create unnecessary churn and additional confusion. I'll keep it as such in the meantime, but I'm definitely willing to take a patch going over this if you think this is necessary. Thanks, M. -- Jazz is not dead. It just smells funny...