From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-sn1nam01on0054.outbound.protection.outlook.com ([104.47.32.54]:55405 "EHLO NAM01-SN1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751037AbdCQPIS (ORCPT ); Fri, 17 Mar 2017 11:08:18 -0400 Subject: Re: [RFC PATCH v2 08/32] x86: Use PAGE_KERNEL protection for ioremap of memory page To: Borislav Petkov , Brijesh Singh References: <148846752022.2349.13667498174822419498.stgit@brijesh-build-machine> <148846761276.2349.4899767672892365544.stgit@brijesh-build-machine> <20170307145954.l2fqy5s5h65wbtyz@pd.tnic> <413f12e9-818a-745d-374b-3dbc439e972c@amd.com> CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , From: Tom Lendacky Message-ID: <0a7de265-1352-6327-ef3a-4287bfca732d@amd.com> Date: Fri, 17 Mar 2017 09:32:15 -0500 MIME-Version: 1.0 In-Reply-To: <413f12e9-818a-745d-374b-3dbc439e972c@amd.com> Content-Type: text/plain; charset="utf-8"; format=flowed Sender: linux-pci-owner@vger.kernel.org List-ID: On 3/16/2017 3:04 PM, Tom Lendacky wrote: > On 3/7/2017 8:59 AM, Borislav Petkov wrote: >> On Thu, Mar 02, 2017 at 10:13:32AM -0500, Brijesh Singh wrote: >>> From: Tom Lendacky >>> >>> In order for memory pages to be properly mapped when SEV is active, we >>> need to use the PAGE_KERNEL protection attribute as the base protection. >>> This will insure that memory mapping of, e.g. ACPI tables, receives the >>> proper mapping attributes. >>> >>> Signed-off-by: Tom Lendacky >>> --- >> >>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c >>> index c400ab5..481c999 100644 >>> --- a/arch/x86/mm/ioremap.c >>> +++ b/arch/x86/mm/ioremap.c >>> @@ -151,7 +151,15 @@ static void __iomem >>> *__ioremap_caller(resource_size_t phys_addr, >>> pcm = new_pcm; >>> } >>> >>> + /* >>> + * If the page being mapped is in memory and SEV is active then >>> + * make sure the memory encryption attribute is enabled in the >>> + * resulting mapping. >>> + */ >>> prot = PAGE_KERNEL_IO; >>> + if (sev_active() && page_is_mem(pfn)) >> >> Hmm, a resource tree walk per ioremap call. This could get expensive for >> ioremap-heavy workloads. >> >> __ioremap_caller() gets called here during boot 55 times so not a whole >> lot but I wouldn't be surprised if there were some nasty use cases which >> ioremap a lot. >> >> ... >> >>> diff --git a/kernel/resource.c b/kernel/resource.c >>> index 9b5f044..db56ba3 100644 >>> --- a/kernel/resource.c >>> +++ b/kernel/resource.c >>> @@ -518,6 +518,46 @@ int __weak page_is_ram(unsigned long pfn) >>> } >>> EXPORT_SYMBOL_GPL(page_is_ram); >>> >>> +/* >>> + * This function returns true if the target memory is marked as >>> + * IORESOURCE_MEM and IORESOUCE_BUSY and described as other than >>> + * IORES_DESC_NONE (e.g. IORES_DESC_ACPI_TABLES). >>> + */ >>> +static int walk_mem_range(unsigned long start_pfn, unsigned long >>> nr_pages) >>> +{ >>> + struct resource res; >>> + unsigned long pfn, end_pfn; >>> + u64 orig_end; >>> + int ret = -1; >>> + >>> + res.start = (u64) start_pfn << PAGE_SHIFT; >>> + res.end = ((u64)(start_pfn + nr_pages) << PAGE_SHIFT) - 1; >>> + res.flags = IORESOURCE_MEM | IORESOURCE_BUSY; >>> + orig_end = res.end; >>> + while ((res.start < res.end) && >>> + (find_next_iomem_res(&res, IORES_DESC_NONE, true) >= 0)) { >>> + pfn = (res.start + PAGE_SIZE - 1) >> PAGE_SHIFT; >>> + end_pfn = (res.end + 1) >> PAGE_SHIFT; >>> + if (end_pfn > pfn) >>> + ret = (res.desc != IORES_DESC_NONE) ? 1 : 0; >>> + if (ret) >>> + break; >>> + res.start = res.end + 1; >>> + res.end = orig_end; >>> + } >>> + return ret; >>> +} >> >> So the relevant difference between this one and walk_system_ram_range() >> is this: >> >> - ret = (*func)(pfn, end_pfn - pfn, arg); >> + ret = (res.desc != IORES_DESC_NONE) ? 1 : 0; >> >> so it seems to me you can have your own *func() pointer which does that >> IORES_DESC_NONE comparison. And then you can define your own workhorse >> __walk_memory_range() which gets called by both walk_mem_range() and >> walk_system_ram_range() instead of almost duplicating them. >> >> And looking at walk_system_ram_res(), that one looks similar too except >> the pfn computation. But AFAICT the pfn/end_pfn things are computed from >> res.start and res.end so it looks to me like all those three functions >> are crying for unification... > > I'll take a look at what it takes to consolidate these with a pre-patch. > Then I'll add the new support. It looks pretty straight forward to combine walk_iomem_res_desc() and walk_system_ram_res(). The walk_system_ram_range() function would fit easily into this, also, except for the fact that the callback function takes unsigned longs vs the u64s of the other functions. Is it worth modifying all of the callers of walk_system_ram_range() (which are only about 8 locations) to change the callback functions to accept u64s in order to consolidate the walk_system_ram_range() function, too? Thanks, Tom > > Thanks, > Tom > >>