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,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, 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 B555AC433ED for ; Mon, 12 Apr 2021 07:51:27 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (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 4C3E761358 for ; Mon, 12 Apr 2021 07:51:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4C3E761358 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=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Subject:Cc:To: From:Message-ID:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=4Er+hH06VpHIuyyYA9gYOIApRL2eDxcJ4saEyrcT2nc=; b=S4EmtxOFVt9EY/G0AcDmcstp3 MLNMDwG+kj8oxAs6a0v8lLlcArpKr+eATLmicpvEaf12fgzeJHW/L/nDAdILiPSfj3D5qWDyPHub0 ghzzAAqt2HQ+sOdvTPxWLB4sW2wkKb7adHk+KFgeSITiFtuo5fTZC8KtvXslftVmsIkU6gQpKT3gv nlLd3iL3H9dhUcN66wD82jCIe07O8pqUSTb4CRrjwmCgH6LB49TjeOlXteeGjWiCXRd9ojqfLJ7qy No5N08bcumt3g4Rz9k/1KNZpSNsPywgJKMnDIZFomAjowaQLWS/PDa6F8yLEDOtFmZ6sr3YaTtoP9 ALS91uhmg==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lVrK9-0061Dh-Nm; Mon, 12 Apr 2021 07:49:33 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lVrK2-0061Cw-R7 for linux-arm-kernel@desiato.infradead.org; Mon, 12 Apr 2021 07:49:28 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Content-Type:MIME-Version:References: In-Reply-To:Subject:Cc:To:From:Message-ID:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=R3sSUeQzZlYKF+u2fy8aKykuEEuVSsoBXaFGhkJbsdg=; b=izrnICURl6whaX3cuYohL9BPpW Xqbd07L7eG7ZEpzOdeP50gJZXb79Q3UwJYXhRaPWlUC33Zx78So64xrp+RvuhknDdNKq7hs+VwXub rZxK8F11f1m4rICXvxDTUDBjxmm8RZ1mOBreuQV2kn6AVFKofvSf8XKMg+Tp1zv8WCiYoSGQSI3ta rTTNF0LTan0FJ88PFrNFyazGELrgA6Hxk+aoGkeAH7Kg3n+Ix7xuEj4XpUG7U0b2eyAF3n58+v3Cp OQ+ANshjby7+IlDrhuS96JbvEYQAUmmumZymirHYij53Sy2OyjTWGjO5pTTDDe2kkIe2y+eLS6m+7 OmyLyLOQ==; Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lVrJz-005wMq-W3 for linux-arm-kernel@lists.infradead.org; Mon, 12 Apr 2021 07:49:25 +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 DBC6561360; Mon, 12 Apr 2021 07:49:22 +0000 (UTC) Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] 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) (envelope-from ) id 1lVrJw-006vIv-N7; Mon, 12 Apr 2021 08:49:20 +0100 Date: Mon, 12 Apr 2021 08:49:19 +0100 Message-ID: <87h7kcnds0.wl-maz@kernel.org> From: Marc Zyngier To: Shaokun Zhang Cc: , , James Morse , Alexandru Elisei , Suzuki K Poulose Subject: Re: [RFC] KVM: arm64: Support FEAT_CCIDX In-Reply-To: References: <1618042597-59294-1-git-send-email-zhangshaokun@hisilicon.com> <875z0ufori.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/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: zhangshaokun@hisilicon.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.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-20210412_004924_103539_2BEC36FC X-CRM114-Status: GOOD ( 49.88 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Shaokun, On Mon, 12 Apr 2021 03:22:03 +0100, Shaokun Zhang wrote: > > Hi Marc, > > On 2021/4/10 17:54, Marc Zyngier wrote: > > On Sat, 10 Apr 2021 09:16:37 +0100, > > Shaokun Zhang wrote: > >> > >> CCSIDR_EL1 can be implemented as 64-bit format inferred by CCIDX field > >> in ID_AA64MMFR2_EL1. The bits of Numsets and Associativity are different > >> from the 32-bit format. Let's support this feature. > >> > >> Cc: Marc Zyngier > >> Cc: James Morse > >> Cc: Alexandru Elisei > >> Cc: Suzuki K Poulose > >> Signed-off-by: Shaokun Zhang > >> --- > >> arch/arm64/kvm/sys_regs.c | 27 +++++++++++++++++++-------- > >> 1 file changed, 19 insertions(+), 8 deletions(-) > >> > >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > >> index 52fdb9a015a4..0dc822cef20b 100644 > >> --- a/arch/arm64/kvm/sys_regs.c > >> +++ b/arch/arm64/kvm/sys_regs.c > >> @@ -18,6 +18,7 @@ > >> > >> #include > >> #include > >> +#include > > > > If you are going to add this (why?), at least add it in alphabetic order. > > cpuid_feature_extract_unsigned_field will be used later. > It shall do in alphabetic order. We already use this function all over the place, and it is already dragged in via many other paths. Anyway, that's not the biggest problem. > > > > >> #include > >> #include > >> #include > >> @@ -95,9 +96,9 @@ static u32 cache_levels; > >> #define CSSELR_MAX 14 > >> > >> /* Which cache CCSIDR represents depends on CSSELR value. */ > >> -static u32 get_ccsidr(u32 csselr) > >> +static u64 get_ccsidr(u32 csselr) > >> { > >> - u32 ccsidr; > >> + u64 ccsidr; > >> > >> /* Make sure noone else changes CSSELR during this! */ > >> local_irq_disable(); > >> @@ -1275,12 +1276,16 @@ static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > >> static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > >> const struct sys_reg_desc *r) > >> { > >> - u32 csselr; > >> + u32 csselr, ccidx; > >> + u64 mmfr2; > >> > >> if (p->is_write) > >> return write_to_read_only(vcpu, p, r); > >> > >> csselr = vcpu_read_sys_reg(vcpu, CSSELR_EL1); > >> + mmfr2 = read_sysreg_s(SYS_ID_AA64MMFR2_EL1); > >> + ccidx = cpuid_feature_extract_unsigned_field(mmfr2, > >> + ID_AA64MMFR2_CCIDX_SHIFT); > > > > What happens on an asymmetric system where only some of the CPUs have > > FEAT_CCIDX? > > If other CPUs don't have FEAT_CCIDX, its value is 0b0000 while > CCSIDR_EL1 is 32-bit format. > You are missing the point: CPU-A has CCIDX, CPU-B doesn't. My vcpu runs on CPU-A, gets preempted right after the read of ID_AA64MMFR2_EL1, and then scheduled on CPU-B. You will now compute the guest view of CCSIDR_EL1 with CPU-B's value, but interpreting it with CPU-A's configuration. That's totally broken. > > > >> p->regval = get_ccsidr(csselr); > >> > >> /* > >> @@ -1295,8 +1300,13 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > >> * geometry (which is not permitted by the architecture), they would > >> * only do so for virtually indexed caches.] > >> */ > >> - if (!(csselr & 1)) // data or unified cache > >> - p->regval &= ~GENMASK(27, 3); > >> + if (!(csselr & 1)) { // data or unified cache > >> + if (ccidx) > >> + p->regval &= ~(GENMASK(23, 3) | GENMASK(55, 32)); > >> + else > >> + p->regval &= ~GENMASK(27, 3); > >> + } > >> + > >> return true; > >> } > >> > >> @@ -2521,7 +2531,7 @@ static bool is_valid_cache(u32 val) > >> static int demux_c15_get(u64 id, void __user *uaddr) > >> { > >> u32 val; > >> - u32 __user *uval = uaddr; > >> + u64 __user *uval = uaddr; > > > > What? Has the user API changed while I wasn't looking? Getting CCSIDR > > can only return a 32bit quantity on AArch32. In fact, CCSIDR is > > *always* 32bit, CCIDX or not. I have no idea what you are trying to do > > here, but at best you are now corrupting 32bit of userspace. > > > > Oops, I really missed this. > > >> > >> /* Fail if we have unknown bits set. */ > >> if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK > >> @@ -2545,8 +2555,9 @@ static int demux_c15_get(u64 id, void __user *uaddr) > >> > >> static int demux_c15_set(u64 id, void __user *uaddr) > >> { > >> - u32 val, newval; > >> - u32 __user *uval = uaddr; > >> + u32 val; > >> + u64 newval; > >> + u64 __user *uval = uaddr; > > > > Same brokenness. > > > >> > >> /* Fail if we have unknown bits set. */ > >> if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK > > > > What about CCSIDR2_EL1, which is mandatory when FEAT_CCSIDX is > > present? How about the AArch32 handling of that register? I don't > > think you have though this one through. > > To be honest, AArch32 is not considered directly and I sent this only > as RFC. If you don't consider AArch32 when writing a patch, please don't send it. AArch32 guests are fully supported, and a patch that breaks them isn't acceptable. Also, a RFC patch doesn't mean things are allowed to break. We use RFC as way to ask for people feedback on a design, but the proposed implementation has to be a valid one. That's not a license to send broken stuff. > When I wrote some flush cache code using by set/way mode and > noticed that CCSIDR_EL1 is used here. > > > > > Another question is: why should we care at all? Why don't we drop all > > that and only implement a virtual cache topology? A VM shouldn't care > > at all about this, and we are already lying about the topology anyway. > > We could even get the VMM to set whatever stupid topology it wants for > > the sake of it (and to restore previously created VMs). > > Got it, > > Thanks for your detailed comments. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel