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 B6750EE57DF for ; Mon, 11 Sep 2023 14:58:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=uu+Z/CxTJ9wtwexXLjKGiHJk6dt2zoyJhA5noA6IqCY=; b=RZ9FzoI0yOh0LB LBghhQrjvAiSYgpmrKjmUF5XKWh6JZ3+poyIHbQUWvHc5SaVNRu+wVcuxA2CgDVQ8Nxtdm8aoukbs XSg95WA0d5FANJsjgiuch/zsue0ezFaw4Ksh/Vkkg3Qni/lOx93FrTzE6cdhmGuc0A7H+qXsIsPQw T9Qqr5KLLGIs2w6I3+u7xqb0hDPpTMPmuoUry2cgUwLoRXcpTW8wopj2oJYWv+mun1wXWqCFzr+Yi K46duYGjrPXM/caPkQRfnSvKPQhvnP4db2j0GFWDjrCBhBLubTnoLKzLvC+fzjndviaUdX3rYPL9m TZef00Zo09TOoAewUCzw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qfiMX-000muC-1n; Mon, 11 Sep 2023 14:58:05 +0000 Received: from sin.source.kernel.org ([145.40.73.55]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qfiMT-000mse-3D for linux-arm-kernel@lists.infradead.org; Mon, 11 Sep 2023 14:58:04 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by sin.source.kernel.org (Postfix) with ESMTPS id B0FA3CE174F; Mon, 11 Sep 2023 14:57:59 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id F205AC433C8; Mon, 11 Sep 2023 14:57:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1694444278; bh=bQfMxb8+0HLBcBK15McAVOoJ4uTK8OsVjnEBuWMDElM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=sUsSOGJyRfMKdnb5MxsvDKxeR1+28guLt+j2WSEuS+e+p8RkicfbuGIMJc7m8Patm hmBkAKcEjQ2xQQHJaZPQA1owXs45QxDuFAL3rEarN7G+Jo8GrSB7AEN5ATGsen2LCu CNaSt/78+JcgNNx+X+23m5wJCZdFZ7cFZjyt15ZvhA4BAzxZBmj9pjMY+9YwyRUKZo f7SvOQEADlk7vE2f31yKb6zI3akyx7r10oDUPBRx6a9PP7XITusl3MBE4CWrIYtaG0 mRkGcl59P+Qm12WVPnExYDR0qdkIixyWZLS9UerXj16PSguyNiY+gByAZBUslgEkHQ rDYvj9HinWGPQ== Date: Mon, 11 Sep 2023 16:57:51 +0200 From: Lorenzo Pieralisi To: ankita@nvidia.com Cc: jgg@nvidia.com, maz@kernel.org, oliver.upton@linux.dev, catalin.marinas@arm.com, will@kernel.org, aniketa@nvidia.com, cjia@nvidia.com, kwankhede@nvidia.com, targupta@nvidia.com, vsethi@nvidia.com, acurrid@nvidia.com, apopple@nvidia.com, jhubbard@nvidia.com, danw@nvidia.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 2/2] KVM: arm64: allow the VM to select DEVICE_* and NORMAL_NC for IO memory Message-ID: References: <20230907181459.18145-1-ankita@nvidia.com> <20230907181459.18145-3-ankita@nvidia.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230907181459.18145-3-ankita@nvidia.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230911_075802_441480_C285759D X-CRM114-Status: GOOD ( 36.35 ) 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="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Sep 07, 2023 at 11:14:59AM -0700, ankita@nvidia.com wrote: > From: Ankit Agrawal > = > Linux allows device drivers to map IO memory on a per-page basis using > "write combining" or WC. This is often done using > pgprot_writecombing(). The driver knows which pages can support WC pgprot_writecombine() ? > access and the proper programming model to generate this IO. Generally > the use case is to boost performance by using write combining to > generate larger PCIe MemWr TLPs. First off, this changeset does not affect *only* Linux guests, obviously. I understand that's the use case you are after but this change is targeting all VMs, it must be clear. Then WC and mapping to PCI TLPs, either you describe that in details (NormalNC vs device-nGnRE and resulting SystemBus<->PCI transactions) or you don't describe it at all, as it stands I don't know how to use this information. > Allow VMs to select DEVICE_* or NORMAL_NC on a page by page basis for > all IO memory. This puts the VM in charge of the memory attributes, > and removes the KVM override to DEVICE_nGnRE. > = > Ultimately this makes pgprot_writecombing() work correctly in VMs and pgprot_writecombine() ? > allows drivers like mlx5 to fully operate their HW. > = > After some discussions with ARM and CPU architects we reached the > conclusion there was no need for KVM to prevent the VM from selecting > between DEVICE_* and NORMAL_NC for IO memory in VMs. There was a fear > that NORMAL_NC could result in uncontained failures, but upon deeper > analysis it turns out there are already possible cases for uncontained > failures with DEVICE types too. Ultimately the platform must be > implemented in a way that ensures that all DEVICE_* and NORMAL_NC > accesses have no uncontained failures. I would reorder/rephrase this changelog as follows: - Describe what the problem is (ie KVM default s2 mappings) - Describe how you are solving it - Add a link to the documentation that states why it is safe to do that and the resulting s1/s2 mappings combination It must be clear why from a legacy standpoint this is a safe change to apply. > Fortunately real platforms do tend to implement this. Remove this sentence, it adds no information for someone who is chasing bugs or just wants to understand the change itself. > This patch makes the VM's memory attributes behave as follows: "The resulting stage 1 and stage 2 memory types and cacheability attributes are as follows": > =A0S1=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 |=A0=A0 S2=A0=A0=A0=A0=A0=A0=A0=A0=A0= |=A0 Result > =A0NORMAL-WB=A0=A0=A0=A0|=A0 NORMAL-NC=A0=A0=A0=A0|=A0 NORMAL-NC > =A0NORMAL-WT=A0=A0=A0=A0|=A0 NORMAL-NC=A0=A0=A0=A0|=A0 NORMAL-NC > =A0NORMAL-NC=A0=A0=A0=A0|=A0 NORMAL-NC=A0=A0=A0=A0|=A0 NORMAL-NC > =A0DEVICE=A0|=A0 NORMAL-NC=A0=A0=A0=A0|=A0 DEVICE > = > See section D8.5.5 of DDI0487_I_a_a-profile_architecture_reference_manual= .pdf > for details. D8.5 I assume. I commented on the log because it must be easy to follow, it is an important change (and there is a lot of background information required to understand it - please report it). The patch itself must be reviewed by arm64/kvm maintainers; the changes (given the background work that led to them) seem fine to me (please CC me on next versions). Thanks Lorenzo > Signed-off-by: Ankit Agrawal > --- > arch/arm64/include/asm/memory.h | 2 ++ > arch/arm64/kvm/hyp/pgtable.c | 2 +- > 2 files changed, 3 insertions(+), 1 deletion(-) > = > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/mem= ory.h > index fde4186cc387..c247e5f29d5a 100644 > --- a/arch/arm64/include/asm/memory.h > +++ b/arch/arm64/include/asm/memory.h > @@ -147,6 +147,7 @@ > * Memory types for Stage-2 translation > */ > #define MT_S2_NORMAL 0xf > +#define MT_S2_NORMAL_NC 0x5 > #define MT_S2_DEVICE_nGnRE 0x1 > = > /* > @@ -154,6 +155,7 @@ > * Stage-2 enforces Normal-WB and Device-nGnRE > */ > #define MT_S2_FWB_NORMAL 6 > +#define MT_S2_FWB_NORMAL_NC 5 > #define MT_S2_FWB_DEVICE_nGnRE 1 > = > #ifdef CONFIG_ARM64_4K_PAGES > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index ccd291b6893d..a80949002191 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -696,7 +696,7 @@ static int stage2_set_prot_attr(struct kvm_pgtable *p= gt, enum kvm_pgtable_prot p > kvm_pte_t *ptep) > { > bool device =3D prot & KVM_PGTABLE_PROT_DEVICE; > - kvm_pte_t attr =3D device ? KVM_S2_MEMATTR(pgt, DEVICE_nGnRE) : > + kvm_pte_t attr =3D device ? KVM_S2_MEMATTR(pgt, NORMAL_NC) : > KVM_S2_MEMATTR(pgt, NORMAL); > u32 sh =3D KVM_PTE_LEAF_ATTR_LO_S2_SH_IS; > = > -- = > 2.17.1 > = _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel