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=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,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 5A8A5C07E9B for ; Wed, 21 Jul 2021 09:48:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3FF666101B for ; Wed, 21 Jul 2021 09:48:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237745AbhGUJHL convert rfc822-to-8bit (ORCPT ); Wed, 21 Jul 2021 05:07:11 -0400 Received: from mail.kernel.org ([198.145.29.99]:33454 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237444AbhGUIvD (ORCPT ); Wed, 21 Jul 2021 04:51:03 -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 1D422611CE; Wed, 21 Jul 2021 09:31:01 +0000 (UTC) Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1m68Z9-00Eflq-2f; Wed, 21 Jul 2021 10:30:59 +0100 Date: Wed, 21 Jul 2021 10:30:58 +0100 Message-ID: <87eebs2eul.wl-maz@kernel.org> From: Marc Zyngier To: Alexandru Elisei Cc: linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, James Morse , Suzuki K Poulose , Alexandre Chartre , Robin Murphy , Andrew Jones , Russell King , kernel-team@android.com Subject: Re: [PATCH v2 4/4] KVM: arm64: Remove PMSWINC_EL0 shadow register In-Reply-To: <9ad0f6a5-fcd0-0179-efa7-7b35ed36e2ff@arm.com> References: <20210719123902.1493805-1-maz@kernel.org> <20210719123902.1493805-5-maz@kernel.org> <9ad0f6a5-fcd0-0179-efa7-7b35ed36e2ff@arm.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: alexandru.elisei@arm.com, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, james.morse@arm.com, suzuki.poulose@arm.com, alexandre.chartre@oracle.com, robin.murphy@arm.com, drjones@redhat.com, linux@arm.linux.org.uk, 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 Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Tue, 20 Jul 2021 17:44:32 +0100, Alexandru Elisei wrote: > > Hi Marc, > > On 7/19/21 5:56 PM, Marc Zyngier wrote: > > Hi Alex, > > > > On 2021-07-19 17:35, Alexandru Elisei wrote: > >> Hi Marc, > >> > >> On 7/19/21 1:39 PM, Marc Zyngier wrote: > >>> We keep an entry for the PMSWINC_EL0 register in the vcpu structure, > >>> while *never* writing anything there outside of reset. > >>> > >>> Given that the register is defined as write-only, that we always > >>> trap when this register is accessed, there is little point in saving > >>> anything anyway. > >>> > >>> Get rid of the entry, and save a mighty 8 bytes per vcpu structure. > >>> > >>> We still need to keep it exposed to userspace in order to preserve > >>> backward compatibility with previously saved VMs. Since userspace > >>> cannot expect any effect of writing to PMSWINC_EL0, treat the > >>> register as RAZ/WI for the purpose of userspace access. > >>> > >>> Signed-off-by: Marc Zyngier > >>> --- > >>>  arch/arm64/include/asm/kvm_host.h |  1 - > >>>  arch/arm64/kvm/sys_regs.c         | 21 ++++++++++++++++++++- > >>>  2 files changed, 20 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/arch/arm64/include/asm/kvm_host.h > >>> b/arch/arm64/include/asm/kvm_host.h > >>> index 41911585ae0c..afc169630884 100644 > >>> --- a/arch/arm64/include/asm/kvm_host.h > >>> +++ b/arch/arm64/include/asm/kvm_host.h > >>> @@ -185,7 +185,6 @@ enum vcpu_sysreg { > >>>      PMCNTENSET_EL0,    /* Count Enable Set Register */ > >>>      PMINTENSET_EL1,    /* Interrupt Enable Set Register */ > >>>      PMOVSSET_EL0,    /* Overflow Flag Status Set Register */ > >>> -    PMSWINC_EL0,    /* Software Increment Register */ > >>>      PMUSERENR_EL0,    /* User Enable Register */ > >>> > >>>      /* Pointer Authentication Registers in a strict increasing order. */ > >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > >>> index f22139658e48..a1f5101f49a3 100644 > >>> --- a/arch/arm64/kvm/sys_regs.c > >>> +++ b/arch/arm64/kvm/sys_regs.c > >>> @@ -1286,6 +1286,20 @@ static int set_raz_id_reg(struct kvm_vcpu *vcpu, const > >>> struct sys_reg_desc *rd, > >>>      return __set_id_reg(vcpu, rd, uaddr, true); > >>>  } > >>> > >>> +static int set_wi_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > >>> +              const struct kvm_one_reg *reg, void __user *uaddr) > >>> +{ > >>> +    int err; > >>> +    u64 val; > >>> + > >>> +    /* Perform the access even if we are going to ignore the value */ > >>> +    err = reg_from_user(&val, uaddr, sys_reg_to_index(rd)); > >> > >> I don't understand why the read still happens if the value is ignored. > >> Just so KVM > >> preserves the previous behaviour and tells userspace there was an error? > > > > If userspace has given us a duff pointer, it needs to know about it. > > Makes sense, thanks. > > > > >>> +    if (err) > >>> +        return err; > >>> + > >>> +    return 0; > >>> +} > >>> + > >>>  static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > >>>                 const struct sys_reg_desc *r) > >>>  { > >>> @@ -1629,8 +1643,13 @@ static const struct sys_reg_desc sys_reg_descs[] = { > >>>        .access = access_pmcnten, .reg = PMCNTENSET_EL0 }, > >>>      { PMU_SYS_REG(SYS_PMOVSCLR_EL0), > >>>        .access = access_pmovs, .reg = PMOVSSET_EL0 }, > >>> +    /* > >>> +     * PM_SWINC_EL0 is exposed to userspace as RAZ/WI, as it was > >>> +     * previously (and pointlessly) advertised in the past... > >>> +     */ > >>>      { PMU_SYS_REG(SYS_PMSWINC_EL0), > >>> -      .access = access_pmswinc, .reg = PMSWINC_EL0 }, > >>> +      .get_user = get_raz_id_reg, .set_user = set_wi_reg, > >> > >> In my opinion, the call chain to return 0 looks pretty confusing to me, as the > >> functions seemed made for ID register accesses, and the leaf function, > >> read_id_reg(), tries to match this register with a list of ID > >> registers. Since we > >> have already added a new function just for PMSWINC_EL0, I was > >> wondering if adding > >> another function, something like get_raz_reg(), would make more sense. > > > > In that case, I'd rather just kill get_raz_id_reg() and replace it with > > this get_raz_reg(). If we trat something as RAZ, who cares whether it is > > an idreg or not? > > I agree, the Arm ARM doesn't make the distinction between ID > registers and other system registers in the definition of RAZ, I > don't think KVM should either. And the way read_id_reg() is written > allows returning a value different than 0 even if raz is true, which > in my opinion could only happen because of a bug in KVM. > > I can have a go at writing the patch(es) on top of this series, if > you want. At the moment I'm rewriting the KVM SPE series, so it will > be a few weeks until I get around to doing it though. We can do that at any time, it is just a cleanup without any guest or userspace visible effect. If a get a spare hour, I'll have a look. Otherwise, this can wait a bit. Thanks, M. -- Without deviation from the norm, progress is not possible.