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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 4BC00C3DA7F for ; Sun, 4 Aug 2024 11:05:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: References:In-Reply-To:Subject:Cc:To:From:Message-ID:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=26Pn6AT5XmruLM+mjDz8gcP3/eT7oecXIVqQHP6EfTs=; b=q1N4XHlGyo/1w4DnU5oGsO8QEi NvNvrdG2jqKGKmy16l9JSYMrClMBtBlbUkYu6Q1+XBXnb6NfH0fyuRB3ZzasWG/viSdBwlrxROmOn rjlM3hSGNLUbfww5jovMG0ssHUfDJFPN070zwO5s5BUw8W11sZfIH+RPNgwHseewEljf5/yeiDN/x 6kbj8gFIe+PocTgaIpStVNNhHK98VOKrR1n2O7olGLe0iDkh2t9l93PxRC39KoSzf9SL0nBsMAufd Iroe1ydq9NhWF0A+3ItfXjXB0odIPPNoglfgzRdMoHD2tGmcYWgEWIxLvmFsAUEQXUKHqwWfNIzIS wu1HyhEg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1saZ3Z-0000000D7rs-0rXQ; Sun, 04 Aug 2024 11:05:45 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1saZ33-0000000D7oJ-1oR1 for linux-arm-kernel@lists.infradead.org; Sun, 04 Aug 2024 11:05:15 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id AA65560687; Sun, 4 Aug 2024 11:05:11 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3BA8CC32786; Sun, 4 Aug 2024 11:05:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1722769511; bh=Gy6I584IGzicBXyzg6q5/89C48fBtUYKR3xsKS4pyDo=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=exHQCFVNTiJ28lW5EtNMPi34LogQvcUSA3Fpzdi3YolVUN4J51yHn1DEJ+EhXu1cG lNl2/4Q60ct2No56frPw1Z7XHNj65ds+TukisTx0+6/Wu0RIFJm7L1oLMplXYsZhmJ DmklP2aDIGRgS4vwsWhomLSmereRrXfyXxkODO30EWad3Utuc4tHHg+DetK5i0ijBW PJC5Sa/4RWKPpUFWwAAf/KI689o5+Dhep2k2/fZK51vnGWL1usxC2v6HxwumsHp+Vf lYhtZb/WmRqXOGFce+3LTMorccVfnPbdOCrlHNfIsl4S+SMH3YpDz06/rejyMCl3G/ ZwxFQYBDQZeRQ== Received: from sofa.misterjones.org ([185.219.108.64] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1saZ2z-000l1v-06; Sun, 04 Aug 2024 12:05:09 +0100 Date: Sun, 04 Aug 2024 12:05:08 +0100 Message-ID: <87sevk4kgr.wl-maz@kernel.org> From: Marc Zyngier To: Anshuman Khandual Cc: linux-arm-kernel@lists.infradead.org, Oliver Upton , James Morse , Suzuki K Poulose , Catalin Marinas , Will Deacon , Mark Brown , kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [RFC 10/10] KVM: arm64: nv: Add new HDFGRTR2_GROUP & HDFGRTR2_GROUP based FGU handling In-Reply-To: References: <20240620065807.151540-1-anshuman.khandual@arm.com> <20240620065807.151540-11-anshuman.khandual@arm.com> <865xu3kh4r.wl-maz@kernel.org> <4d256df7-1ec7-4300-b5c8-355f46c0e869@arm.com> <878qyy35e5.wl-maz@kernel.org> <47dc4299-52cc-4f98-929b-fb86bd9757ae@arm.com> <86tthhi0nz.wl-maz@kernel.org> <86o76c1b8p.wl-maz@kernel.org> <86bk2b198o.wl-maz@kernel.org> 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/28.2 (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=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: anshuman.khandual@arm.com, linux-arm-kernel@lists.infradead.org, oliver.upton@linux.dev, james.morse@arm.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org, broonie@kernel.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org 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-20240804_040513_875500_0454B035 X-CRM114-Status: GOOD ( 45.02 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sat, 03 Aug 2024 11:38:11 +0100, Anshuman Khandual wrote: > > > On 8/2/24 16:29, Marc Zyngier wrote: > > On Fri, 02 Aug 2024 10:25:44 +0100, > > Anshuman Khandual wrote: > >> > >> On 8/1/24 21:33, Marc Zyngier wrote: > >>> On Thu, 01 Aug 2024 11:46:22 +0100, > >>> Anshuman Khandual wrote: > > > > [...] > > > >>>> + SR_FGT(SYS_SPMACCESSR_EL1, HDFGRTR2, nSPMACCESSR_EL1, 0), > >>> > >>> This (and I take it most of the stuff here) is also gated by > >>> MDCR_EL2.SPM, which is a coarse grained trap. That needs to be > >>> described as well. For every new register that you add here. > >> > >> I did not find a SPM field in MDCR_EL2 either in latest ARM ARM or in > >> the latest XML. But as per current HDFGRTR2_EL2 description the field > >> nSPMACCESSR_EL1 is gated by FEAT_SPMU feature, which is being checked > >> via ID_AA64DFR1_EL1.PMU when required. So could you please give some > >> more details. > > > > I misspelled it. It is MDCR_EL2.EnSPM. > > > > And you are completely missing the point. It is not about > > HDFGRTR2_EL2, but about SPMACCESSR_EL1 (and all its little friends). > > > > To convince yourself, just look at the pseudocode for SPMACCESSR_EL1, > > limited to an EL1 access: > > > > elsif PSTATE.EL == EL1 then > > if HaveEL(EL3) && EL3SDDUndefPriority() && MDCR_EL3.EnPM2 == '0' then > > UNDEFINED; > > elsif EL2Enabled() && IsFeatureImplemented(FEAT_FGT2) && ((HaveEL(EL3) && SCR_EL3.FGTEn2 == '0') || HDFGRTR2_EL2.nSPMACCESSR_EL1 == '0') then > > AArch64.SystemAccessTrap(EL2, 0x18); > > elsif EL2Enabled() && MDCR_EL2.EnSPM == '0' then > > AArch64.SystemAccessTrap(EL2, 0x18); > > elsif HaveEL(EL3) && MDCR_EL3.EnPM2 == '0' then > > if EL3SDDUndef() then > > UNDEFINED; > > else > > AArch64.SystemAccessTrap(EL3, 0x18); > > elsif EffectiveHCR_EL2_NVx() IN {'111'} then > > X[t, 64] = NVMem[0x8E8]; > > else > > X[t, 64] = SPMACCESSR_EL1; > > > > > > Can you spot the *TWO* conditions where we take an exception to EL2 > > with 0x18 as the EC? > > > > - One is when HDFGxTR2_EL2.nSPMACCESSR_EL1 == '0': that's a fine > > grained trap. > > - The other is when MDCR_EL2.EnSPM == '0': that's a coarse grained > > trap. > > > > Both conditions need to be captured in the various tables in this > > file, for each and every register that you describe. > > Ahh, got it now. Because now KVM knows about SPMACCESSR_EL1 register, > access to that register must also be enabled via both fine grained trap > (HDFGxTR2_EL2.nSPMACCESSR_EL1) and coarse grained trap (MDCR_EL2.EnSPM). > > For all the registers that are being added via FEAT_FGT2 here in this > patch, their corresponding CGT based path also needs to be enabled via > corresponding CGT_MDCR_XXX groups. Exactly. > > > > > [...] > > > >>> Now, the main issues are that: > >>> > >>> - you're missing the coarse grained trapping for all the stuff you > >>> have just added. It's not a huge amount of work, but you need, for > >>> each register, to describe what traps apply to it. The fine grained > >>> stuff is most, but not all of it. There should be enough of it > >>> already to guide you through it. > >> > >> Coarse grained trapping for FEAT_FGT2 based fine grained registers ? > > > > Not for FEAT_FGT2. For the registers that FEAT_FGT2 traps. Can you see > > the difference? > > Understood, for example PMIAR_EL1 register which FEAT_FGT2 now traps via > > SR_FGT(SYS_PMIAR_EL1, HDFGRTR2, nPMIAR_EL1, 0), > > also needs to have corresponding coarse grained trap. > > SR_TRAP(SYS_PMIAR_EL1, CGT_MDCR_TPM), Yup. > > Similarly corresponding SR_TRAP() needs to be covered for all registers > that are now being trapped with FEAT_FGT2. > > Example code snippet. > > ........ > + SR_TRAP(SYS_SPMEVFILT2R_EL0(8), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(9), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(10), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(11), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(12), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(13), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(14), CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMEVFILT2R_EL0(15), CGT_MDCR_EnSPM), I think it is a bit more complicated than just that. Again, look at the pseudocode: elsif PSTATE.EL == EL1 then if HaveEL(EL3) && EL3SDDUndefPriority() && MDCR_EL3.EnPM2 == '0' then UNDEFINED; elsif HaveEL(EL3) && EL3SDDUndefPriority() && SPMACCESSR_EL3<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then UNDEFINED; elsif EL2Enabled() && IsFeatureImplemented(FEAT_FGT2) && ((HaveEL(EL3) && SCR_EL3.FGTEn2 == '0') || HDFGRTR2_EL2.nSPMEVTYPERn_EL0 == '0') then AArch64.SystemAccessTrap(EL2, 0x18); elsif EL2Enabled() && MDCR_EL2.EnSPM == '0' then AArch64.SystemAccessTrap(EL2, 0x18); elsif EL2Enabled() && SPMACCESSR_EL2<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then AArch64.SystemAccessTrap(EL2, 0x18); elsif HaveEL(EL3) && MDCR_EL3.EnPM2 == '0' then if EL3SDDUndef() then UNDEFINED; else AArch64.SystemAccessTrap(EL3, 0x18); elsif HaveEL(EL3) && SPMACCESSR_EL3<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then if EL3SDDUndef() then UNDEFINED; else AArch64.SystemAccessTrap(EL3, 0x18); elsif !IsSPMUCounterImplemented(UInt(SPMSELR_EL0.SYSPMUSEL), (UInt(SPMSELR_EL0.BANK) * 16) + m) then X[t, 64] = Zeros(64); else X[t, 64] = SPMEVFILT2R_EL0[UInt(SPMSELR_EL0.SYSPMUSEL), (UInt(SPMSELR_EL0.BANK) * 16) + m]; which shows that an SPMEVFILT2Rn_EL0 access from EL1 traps to EL2 if: - either HDFGRTR2_EL2.nSPMEVTYPERn_EL0 == '0', (check) - or MDCR_EL2.EnSPM == '0', (check) - or SPMACCESSR_EL2<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' and that last condition requires some more handling as you need to evaluate both SPMSELR_EL0.SYSPMUSEL and the corresponding field of SPMACCESSR_EL2 to make a decision. It's not majorly complicated, but it isn't solved by simply setting a static attribute. > + > + SR_TRAP(SYS_SPMCGCR0_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMCGCR1_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMACCESSR_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMCFGR_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMCNTENCLR_EL0, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMCNTENSET_EL0, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMCR_EL0, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMDEVAFF_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMDEVARCH_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMIIDR_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMINTENCLR_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMINTENSET_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMOVSCLR_EL0, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMOVSSET_EL0, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMSCR_EL1, CGT_MDCR_EnSPM), > + SR_TRAP(SYS_SPMSELR_EL0, CGT_MDCR_EnSPM), So please look at the pseudocode for each and every register you add. If there is a trap to EL2 that is described, you must have the corresponding handling. None of that is optional. > ........ > > > > >> Afraid, did not understand this. Could you please give some pointers > >> on similar existing code. > > > > See above. And if you want some example, just took at the file you are > > patching already. Look at how MDCR_EL2 conditions the trapping of all > > the debug, PMU, AMU registers, for example. There is no shortage of > > them. > > Right, I understand your point now. I am planning to send out the following > patches (in reply to the current thread) for your review, before sending out > the entire series as V1. > > - Patch adding VNCR details for virtual EL2 access > - Patch adding FEAT_FGT2 based FGU handling > - Patch adding corresponding CGT handling for registers being added above > > Although, can skip that step and just send out the series directly if you so > prefer. Please do suggest. Thanks for all the detailed information above. My suggestion is that you take a good look at the pseudocode and implement all the existing requirements. Yes, this is undesirably complicated, but I'm not the one making the rules here... Once you think you have all these requirements in place, please post the series. But not before that. If you have any question, just ask. Thanks, M. -- Without deviation from the norm, progress is not possible.