From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bjorn Helgaas Subject: Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data Date: Mon, 6 Mar 2017 18:03:49 -0600 Message-ID: <20170307000349.GC5305@bhelgaas-glaptop.roam.corp.google.com> References: <148846752022.2349.13667498174822419498.stgit@brijesh-build-machine> <148846759008.2349.8274808454274279039.stgit@brijesh-build-machine> <20170303204209.GA31767@bhelgaas-glaptop.roam.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: linux-efi@vger.kernel.org, Brijesh Singh , labbott@fedoraproject.org, kvm@vger.kernel.org, rkrcmar@redhat.com, matt@codeblueprint.co.uk, linux-pci@vger.kernel.org, linus.walleij@linaro.org, gary.hook@amd.com, linux-mm@kvack.org, hpa@zytor.com, cl@linux.com, tglx@linutronix.de, aarcange@redhat.com, sfr@canb.auug.org.au, mchehab@kernel.org, simon.guinot@sequanux.org, bhe@redhat.com, xemul@parallels.com, joro@8bytes.org, x86@kernel.org, peterz@infradead.org, piotr.luc@intel.com, mingo@redhat.com, msalter@redhat.com, ross.zwisler@linux.intel.com, bp@suse.de, dyoung@redhat.com, jroedel@suse.de, keescook@chromium.org, arnd@arndb.de, toshi.kani@hpe.com, mathieu.desnoyers@efficios.com, luto@kernel.org, pbonzini@redhat.com, bhelgaas@google.com, dan.j.williams@intel.com, andriy.shevchenko@linux.intel.com, akpm@linux-foundation.org, herbert@gondor.apana.org.a To: Tom Lendacky Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" List-Id: linux-crypto.vger.kernel.org On Fri, Mar 03, 2017 at 03:15:34PM -0600, Tom Lendacky wrote: > On 3/3/2017 2:42 PM, Bjorn Helgaas wrote: > >On Thu, Mar 02, 2017 at 10:13:10AM -0500, Brijesh Singh wrote: > >>From: Tom Lendacky > >> > >>The use of ioremap will force the setup data to be mapped decrypted even > >>though setup data is encrypted. Switch to using memremap which will be > >>able to perform the proper mapping. > > > >How should callers decide whether to use ioremap() or memremap()? > > > >memremap() existed before SME and SEV, and this code is used even if > >SME and SEV aren't supported, so the rationale for this change should > >not need the decryption argument. > > When SME or SEV is active an ioremap() will remove the encryption bit > from the pagetable entry when it is mapped. This allows MMIO, which > doesn't support SME/SEV, to be performed successfully. So my take is > that ioremap() should be used for MMIO and memremap() for pages in RAM. OK, thanks. The commit message should say something like "this is RAM, not MMIO, so we should map it with memremap(), not ioremap()". That's the part that determines whether the change is correct. You can mention the encryption part, too, but it's definitely secondary because the change has to make sense on its own, without SME/SEV. The following commits (from https://github.com/codomania/tip/branches) all do basically the same thing so the changelogs (and summaries) should all be basically the same: cb0d0d1eb0a6 x86: Change early_ioremap to early_memremap for BOOT data 91acb68b8333 x86/pci: Use memremap when walking setup data 4f687503e23f x86: Access the setup data through sysfs decrypted e90246b8c229 x86: Access the setup data through debugfs decrypted I would collect them all together and move them to the beginning of your series, since they don't depend on anything else. Also, change "x86/pci: " to "x86/PCI" so it matches the previous convention. > >>Signed-off-by: Tom Lendacky Acked-by: Bjorn Helgaas > >>--- > >> arch/x86/pci/common.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >>diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > >>index a4fdfa7..0b06670 100644 > >>--- a/arch/x86/pci/common.c > >>+++ b/arch/x86/pci/common.c > >>@@ -691,7 +691,7 @@ int pcibios_add_device(struct pci_dev *dev) > >> > >> pa_data = boot_params.hdr.setup_data; > >> while (pa_data) { > >>- data = ioremap(pa_data, sizeof(*rom)); > >>+ data = memremap(pa_data, sizeof(*rom), MEMREMAP_WB); > > > >I can't quite connect the dots here. ioremap() on x86 would do > >ioremap_nocache(). memremap(MEMREMAP_WB) would do arch_memremap_wb(), > >which is ioremap_cache(). Is making a cacheable mapping the important > >difference? > > The memremap(MEMREMAP_WB) will actually check to see if it can perform > a __va(pa_data) in try_ram_remap() and then fallback to the > arch_memremap_wb(). So it's actually the __va() vs the ioremap_cache() > that is the difference. > > Thanks, > Tom > > > > >> if (!data) > >> return -ENOMEM; > >> > >>@@ -710,7 +710,7 @@ int pcibios_add_device(struct pci_dev *dev) > >> } > >> } > >> pa_data = data->next; > >>- iounmap(data); > >>+ memunmap(data); > >> } > >> set_dma_domain_ops(dev); > >> set_dev_domain_options(dev); > >> From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932343AbdCGANX (ORCPT ); Mon, 6 Mar 2017 19:13:23 -0500 Received: from mail.kernel.org ([198.145.29.136]:49750 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754083AbdCGAM7 (ORCPT ); Mon, 6 Mar 2017 19:12:59 -0500 Date: Mon, 6 Mar 2017 18:03:49 -0600 From: Bjorn Helgaas To: Tom Lendacky Cc: Brijesh Singh , simon.guinot@sequanux.org, linux-efi@vger.kernel.org, kvm@vger.kernel.org, rkrcmar@redhat.com, matt@codeblueprint.co.uk, linux-pci@vger.kernel.org, linus.walleij@linaro.org, gary.hook@amd.com, linux-mm@kvack.org, paul.gortmaker@windriver.com, hpa@zytor.com, cl@linux.com, dan.j.williams@intel.com, aarcange@redhat.com, sfr@canb.auug.org.au, andriy.shevchenko@linux.intel.com, herbert@gondor.apana.org.au, bhe@redhat.com, xemul@parallels.com, joro@8bytes.org, x86@kernel.org, peterz@infradead.org, piotr.luc@intel.com, mingo@redhat.com, msalter@redhat.com, ross.zwisler@linux.intel.com, bp@suse.de, dyoung@redhat.com, jroedel@suse.de, keescook@chromium.org, arnd@arndb.de, toshi.kani@hpe.com, mathieu.desnoyers@efficios.com, luto@kernel.org, devel@linuxdriverproject.org, bhelgaas@google.com, tglx@linutronix.de, mchehab@kernel.org, iamjoonsoo.kim@lge.com, labbott@fedoraproject.org, tony.luck@intel.com, alexandre.bounine@idt.com, kuleshovmail@gmail.com, linux-kernel@vger.kernel.org, mcgrof@kernel.org, mst@redhat.com, linux-crypto@vger.kernel.org, tj@kernel.org, pbonzini@redhat.com, akpm@linux-foundation.org, davem@davemloft.net Subject: Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data Message-ID: <20170307000349.GC5305@bhelgaas-glaptop.roam.corp.google.com> References: <148846752022.2349.13667498174822419498.stgit@brijesh-build-machine> <148846759008.2349.8274808454274279039.stgit@brijesh-build-machine> <20170303204209.GA31767@bhelgaas-glaptop.roam.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 03, 2017 at 03:15:34PM -0600, Tom Lendacky wrote: > On 3/3/2017 2:42 PM, Bjorn Helgaas wrote: > >On Thu, Mar 02, 2017 at 10:13:10AM -0500, Brijesh Singh wrote: > >>From: Tom Lendacky > >> > >>The use of ioremap will force the setup data to be mapped decrypted even > >>though setup data is encrypted. Switch to using memremap which will be > >>able to perform the proper mapping. > > > >How should callers decide whether to use ioremap() or memremap()? > > > >memremap() existed before SME and SEV, and this code is used even if > >SME and SEV aren't supported, so the rationale for this change should > >not need the decryption argument. > > When SME or SEV is active an ioremap() will remove the encryption bit > from the pagetable entry when it is mapped. This allows MMIO, which > doesn't support SME/SEV, to be performed successfully. So my take is > that ioremap() should be used for MMIO and memremap() for pages in RAM. OK, thanks. The commit message should say something like "this is RAM, not MMIO, so we should map it with memremap(), not ioremap()". That's the part that determines whether the change is correct. You can mention the encryption part, too, but it's definitely secondary because the change has to make sense on its own, without SME/SEV. The following commits (from https://github.com/codomania/tip/branches) all do basically the same thing so the changelogs (and summaries) should all be basically the same: cb0d0d1eb0a6 x86: Change early_ioremap to early_memremap for BOOT data 91acb68b8333 x86/pci: Use memremap when walking setup data 4f687503e23f x86: Access the setup data through sysfs decrypted e90246b8c229 x86: Access the setup data through debugfs decrypted I would collect them all together and move them to the beginning of your series, since they don't depend on anything else. Also, change "x86/pci: " to "x86/PCI" so it matches the previous convention. > >>Signed-off-by: Tom Lendacky Acked-by: Bjorn Helgaas > >>--- > >> arch/x86/pci/common.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >>diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > >>index a4fdfa7..0b06670 100644 > >>--- a/arch/x86/pci/common.c > >>+++ b/arch/x86/pci/common.c > >>@@ -691,7 +691,7 @@ int pcibios_add_device(struct pci_dev *dev) > >> > >> pa_data = boot_params.hdr.setup_data; > >> while (pa_data) { > >>- data = ioremap(pa_data, sizeof(*rom)); > >>+ data = memremap(pa_data, sizeof(*rom), MEMREMAP_WB); > > > >I can't quite connect the dots here. ioremap() on x86 would do > >ioremap_nocache(). memremap(MEMREMAP_WB) would do arch_memremap_wb(), > >which is ioremap_cache(). Is making a cacheable mapping the important > >difference? > > The memremap(MEMREMAP_WB) will actually check to see if it can perform > a __va(pa_data) in try_ram_remap() and then fallback to the > arch_memremap_wb(). So it's actually the __va() vs the ioremap_cache() > that is the difference. > > Thanks, > Tom > > > > >> if (!data) > >> return -ENOMEM; > >> > >>@@ -710,7 +710,7 @@ int pcibios_add_device(struct pci_dev *dev) > >> } > >> } > >> pa_data = data->next; > >>- iounmap(data); > >>+ memunmap(data); > >> } > >> set_dma_domain_ops(dev); > >> set_dev_domain_options(dev); > >> From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f70.google.com (mail-pg0-f70.google.com [74.125.83.70]) by kanga.kvack.org (Postfix) with ESMTP id C76026B0387 for ; Mon, 6 Mar 2017 19:03:57 -0500 (EST) Received: by mail-pg0-f70.google.com with SMTP id y17so49807105pgh.2 for ; Mon, 06 Mar 2017 16:03:57 -0800 (PST) Received: from mail.kernel.org (mail.kernel.org. [198.145.29.136]) by mx.google.com with ESMTPS id d18si1318758pgi.295.2017.03.06.16.03.56 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 06 Mar 2017 16:03:56 -0800 (PST) Date: Mon, 6 Mar 2017 18:03:49 -0600 From: Bjorn Helgaas Subject: Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data Message-ID: <20170307000349.GC5305@bhelgaas-glaptop.roam.corp.google.com> References: <148846752022.2349.13667498174822419498.stgit@brijesh-build-machine> <148846759008.2349.8274808454274279039.stgit@brijesh-build-machine> <20170303204209.GA31767@bhelgaas-glaptop.roam.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: To: Tom Lendacky Cc: Brijesh Singh , simon.guinot@sequanux.org, linux-efi@vger.kernel.org, kvm@vger.kernel.org, rkrcmar@redhat.com, matt@codeblueprint.co.uk, linux-pci@vger.kernel.org, linus.walleij@linaro.org, gary.hook@amd.com, linux-mm@kvack.org, paul.gortmaker@windriver.com, hpa@zytor.com, cl@linux.com, dan.j.williams@intel.com, aarcange@redhat.com, sfr@canb.auug.org.au, andriy.shevchenko@linux.intel.com, herbert@gondor.apana.org.au, bhe@redhat.com, xemul@parallels.com, joro@8bytes.org, x86@kernel.org, peterz@infradead.org, piotr.luc@intel.com, mingo@redhat.com, msalter@redhat.com, ross.zwisler@linux.intel.com, bp@suse.de, dyoung@redhat.com, jroedel@suse.de, keescook@chromium.org, arnd@arndb.de, toshi.kani@hpe.com, mathieu.desnoyers@efficios.com, luto@kernel.org, devel@linuxdriverproject.org, bhelgaas@google.com, tglx@linutronix.de, mchehab@kernel.org, iamjoonsoo.kim@lge.com, labbott@fedoraproject.org, tony.luck@intel.com, alexandre.bounine@idt.com, kuleshovmail@gmail.com, linux-kernel@vger.kernel.org, mcgrof@kernel.org, mst@redhat.com, linux-crypto@vger.kernel.org, tj@kernel.org, pbonzini@redhat.com, akpm@linux-foundation.org, davem@davemloft.net On Fri, Mar 03, 2017 at 03:15:34PM -0600, Tom Lendacky wrote: > On 3/3/2017 2:42 PM, Bjorn Helgaas wrote: > >On Thu, Mar 02, 2017 at 10:13:10AM -0500, Brijesh Singh wrote: > >>From: Tom Lendacky > >> > >>The use of ioremap will force the setup data to be mapped decrypted even > >>though setup data is encrypted. Switch to using memremap which will be > >>able to perform the proper mapping. > > > >How should callers decide whether to use ioremap() or memremap()? > > > >memremap() existed before SME and SEV, and this code is used even if > >SME and SEV aren't supported, so the rationale for this change should > >not need the decryption argument. > > When SME or SEV is active an ioremap() will remove the encryption bit > from the pagetable entry when it is mapped. This allows MMIO, which > doesn't support SME/SEV, to be performed successfully. So my take is > that ioremap() should be used for MMIO and memremap() for pages in RAM. OK, thanks. The commit message should say something like "this is RAM, not MMIO, so we should map it with memremap(), not ioremap()". That's the part that determines whether the change is correct. You can mention the encryption part, too, but it's definitely secondary because the change has to make sense on its own, without SME/SEV. The following commits (from https://github.com/codomania/tip/branches) all do basically the same thing so the changelogs (and summaries) should all be basically the same: cb0d0d1eb0a6 x86: Change early_ioremap to early_memremap for BOOT data 91acb68b8333 x86/pci: Use memremap when walking setup data 4f687503e23f x86: Access the setup data through sysfs decrypted e90246b8c229 x86: Access the setup data through debugfs decrypted I would collect them all together and move them to the beginning of your series, since they don't depend on anything else. Also, change "x86/pci: " to "x86/PCI" so it matches the previous convention. > >>Signed-off-by: Tom Lendacky Acked-by: Bjorn Helgaas > >>--- > >> arch/x86/pci/common.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >>diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > >>index a4fdfa7..0b06670 100644 > >>--- a/arch/x86/pci/common.c > >>+++ b/arch/x86/pci/common.c > >>@@ -691,7 +691,7 @@ int pcibios_add_device(struct pci_dev *dev) > >> > >> pa_data = boot_params.hdr.setup_data; > >> while (pa_data) { > >>- data = ioremap(pa_data, sizeof(*rom)); > >>+ data = memremap(pa_data, sizeof(*rom), MEMREMAP_WB); > > > >I can't quite connect the dots here. ioremap() on x86 would do > >ioremap_nocache(). memremap(MEMREMAP_WB) would do arch_memremap_wb(), > >which is ioremap_cache(). Is making a cacheable mapping the important > >difference? > > The memremap(MEMREMAP_WB) will actually check to see if it can perform > a __va(pa_data) in try_ram_remap() and then fallback to the > arch_memremap_wb(). So it's actually the __va() vs the ioremap_cache() > that is the difference. > > Thanks, > Tom > > > > >> if (!data) > >> return -ENOMEM; > >> > >>@@ -710,7 +710,7 @@ int pcibios_add_device(struct pci_dev *dev) > >> } > >> } > >> pa_data = data->next; > >>- iounmap(data); > >>+ memunmap(data); > >> } > >> set_dma_domain_ops(dev); > >> set_dev_domain_options(dev); > >> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org