From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Lendacky Subject: Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data Date: Mon, 13 Mar 2017 15:08:52 -0500 Message-ID: References: <148846752022.2349.13667498174822419498.stgit@brijesh-build-machine> <148846759008.2349.8274808454274279039.stgit@brijesh-build-machine> <20170303204209.GA31767@bhelgaas-glaptop.roam.corp.google.com> <20170307000349.GC5305@bhelgaas-glaptop.roam.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: Brijesh Singh , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Return-path: In-Reply-To: <20170307000349.GC5305@bhelgaas-glaptop.roam.corp.google.com> Sender: owner-linux-mm@kvack.org List-Id: linux-crypto.vger.kernel.org On 3/6/2017 6:03 PM, Bjorn Helgaas wrote: > 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. > Ok, that makes sense, will do. > 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. I'll do that. > > Also, change "x86/pci: " to "x86/PCI" so it matches the previous > convention. Will do. Thanks, Tom > >>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753577AbdCMUJU (ORCPT ); Mon, 13 Mar 2017 16:09:20 -0400 Received: from mail-by2nam01on0070.outbound.protection.outlook.com ([104.47.34.70]:50176 "EHLO NAM01-BY2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752344AbdCMUJI (ORCPT ); Mon, 13 Mar 2017 16:09:08 -0400 Authentication-Results: davemloft.net; dkim=none (message not signed) header.d=none;davemloft.net; dmarc=none action=none header.from=amd.com; Subject: Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data To: Bjorn Helgaas References: <148846752022.2349.13667498174822419498.stgit@brijesh-build-machine> <148846759008.2349.8274808454274279039.stgit@brijesh-build-machine> <20170303204209.GA31767@bhelgaas-glaptop.roam.corp.google.com> <20170307000349.GC5305@bhelgaas-glaptop.roam.corp.google.com> CC: Brijesh Singh , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , From: Tom Lendacky Message-ID: Date: Mon, 13 Mar 2017 15:08:52 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170307000349.GC5305@bhelgaas-glaptop.roam.corp.google.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [165.204.77.1] X-ClientProxiedBy: CY4PR21CA0019.namprd21.prod.outlook.com (10.172.122.157) To MWHPR12MB1151.namprd12.prod.outlook.com (10.169.204.15) X-MS-Office365-Filtering-Correlation-Id: c354f5fe-b808-45a9-3d68-08d46a4cccc6 X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(48565401081);SRVR:MWHPR12MB1151; X-Microsoft-Exchange-Diagnostics: 1;MWHPR12MB1151;3:aS4TSjG7R7AvRT/ghSoDjZAXph7pZkS80S5gy4Lhnn78oN63Xpgq89kY2zkCKBYWAJS2VTLpgRaL6HAVkgWCD7QC6qvdcxbACJazZCPzYBw42UoHno9myVPEwlFQPifo5wkXiMZB35kXRlBP870ftkb9VSSjlsonhVgD9tIOE+rWj8fwOR8KRfQ0DQA5XVNIZRxz7mRJ4T4012cCeKNlxI2NLs/A39KP96g2/7OMSEwqSl0zJacgUMp8SHfdrRbMkkR/36IpThGfUGI/F752TDlw9CenVEhiV7EIMH070Fs=;25:9rfKgHRDXHtPNWg6/oeK9RQNB0uVPFLWO1QM/ZXJhF4/1nDu7j0zF/fxFhQ/p9C+k5p7y99JlxobGcT+PBax1Set97BdQiFAyukqaFn0xp8kK2QO4i7dtjf/cV+98KiJ3n/+qvOjcCo2idTZuPxBr1ILi80MrGTyef8itVYKQCddQIDgwNjT+La7coYP2kISFs7xDX+NbByOamyory9qeyFkuzm09JxAYShB5JjOB9gJGzWYaTw/5nCE2XdX2bwFd1+mvfc5oVJ1RgB4HAKThlxECuqHWP20gPeWAz3VCzHQM6e9eOx3O8NFZLvvyPcs1Xjx0KjtCZjZU7P1GOYy6Rf5ZWIekNfPrY/UUr3dvd1ThOUQm5NOETWaZBhLrmbiHXOPrQRt3xWg712N98KjHncm4UlMnoIxPW/FXkQ39fz4gbqaUmwjNysweSr/8iIvz3SP4hjbdaoQO8r0E9BJKw== X-Microsoft-Exchange-Diagnostics: 1;MWHPR12MB1151;31:cAk6YwI8CpEGnjopkkd3SDoc74J0lcZcdt96WN/sd2ZiC1fFHYO9tG6YFFK7NoMr+6ZG9KC+tH9lqbCh2GPj46O0ZEMbxSs6kwYhmHZpQDfbuhDg4V5AR5wmn7EIYLq6O0yw5UHS626onwOfIX0CMl+7QJd0X8dCn1SRRfHdJ26e/7LxMUERmxjEIBegVI+WyXBkusBgQvt65xnZRrejZPRstA7mcdLCd7gjM7NPdxGc6pz7Gm3E5s9yJ0LuIIyx;20:Wj+KeOq1zWlsVgWrb6TmSkTvy7NxBfM84DRBDP50CjXsAHbo1+qTE1ABjjcjP+w2pKxd1rM7RYSRdpvrfieyI69xAZP1P2QcsxUv1LJJoXdJPqSikOhzT4Ofu4AkM0iCv2FKAKF2Ia3VW6mjJhzZrVnpxZu+ZBToqEFyvKWqUTZWSpkW+r70VQ4dFH5rxo0j5Jg71B9OzyQ7upbR97h5pHtjnc6oGg+rz3EyTAbWxN28LflFHazm6hHL5STGlT2OOZBGU++Ln1e77UviuyWOewnud9/BgkJYz73DPap4xdwLyINWSkJrjukzhicsIP33xahuRsNjFfKL/vdAzH7ShSAXZdxBd6y7pvyi1YT+1VgIJvzxv0WA5Xqku74oHXnZGtTfrv6slqr8ieuLofOHT1VPo3RT+vV+snlQCQO8cQN4mmCSxQlFEnHk7mVyJ3YMjckTmDnZvxSlVN3qLr/FvBa9oCs4W56tHYnjNF4awbHCWDmpOnznswLWuRO16Nxj X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(166708455590820)(767451399110)(211936372134217)(21532816269658); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040375)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026)(6041248)(20161123558025)(20161123562025)(20161123560025)(20161123564025)(20161123555025)(6072148);SRVR:MWHPR12MB1151;BCL:0;PCL:0;RULEID:;SRVR:MWHPR12MB1151; X-Microsoft-Exchange-Diagnostics: 1;MWHPR12MB1151;4:lgSyhiGwSiwlEPLCw1fvD9YN88cjyXK09XC7+nvRNqwLtlUl+Egu5eHS6V0gIou2O4t7NNiqZXyikCQAMXUFDZuymKmcoDGJtfjxhETeJBvBXgEjBgG4rJJlLJI6CY4sI6J9fqLjMND/mmAtWPkjYSQqU7t6Qbzce943tY+AEzHUDfEsoWZmm6L5EfHTWg8ZJiK+wfWnSmPTNTbN1GKDH+T4PaxSP1axTZ0VRzefYLHy4U6RZKCUTzjg+1QvLgAda3/sHfEAVt1bUkFL0lNbkpgh4s7WcJekF3vUPugjZrWH0/jAdPnrS8SUbMIOMPj+vfW7KZK4OE/Lqu97YldqpaX9ndG7CXh+LaBYsJ/YBvjWseJA+0KylkRTCph8b7PzCNcmymAPj5X38hWtNE8VmmHgYD5P4DYA5gCIebRlk6IGQhtVjo2Ut7kYSt3fcn+kycR8e+69m09PU8OGAHm5vATEh+GgeoCqdRx5GwqNWSDb3mKuBNWSDbuWCx4fPpkoiumKKmAmj2mBGeMwuCzqvf0OsG5tYvkQHV4wnzbB7fcsGNvDPeSPDjPFMkpwEXQBF4Eeu+/qKAP47VbJcjiUaXVNjSe3mITyAWgQE3Q2/T3rGvlg759cDXWoxPbMCStwpq3nQ0sS6C88XLbpuVvY1zB6KXVShrIVPI98Hj8fCQ7f2fGu5coAHBU6xZ7xoRulnyd5soVuG04rl5hSRxls4MJ5g4fnt7IC1r7WupSLcTa6LLU7pUcytCL4/mY5pwSNwL4tcDemSMBXb+71OeNDEQ== X-Forefront-PRVS: 0245702D7B X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4630300001)(6009001)(6049001)(39450400003)(39850400002)(39840400002)(39410400002)(39860400002)(377454003)(24454002)(5660300001)(4001350100001)(93886004)(31686004)(42186005)(8676002)(7366002)(7416002)(7406005)(25786008)(6916009)(2950100002)(6666003)(23746002)(305945005)(33646002)(230700001)(53936002)(7736002)(50466002)(64126003)(36756003)(6306002)(90366009)(54906002)(2906002)(3846002)(6116002)(31696002)(229853002)(77096006)(86362001)(575784001)(81166006)(6486002)(189998001)(4326008)(65956001)(65806001)(110136004)(76176999)(47776003)(54356999)(6246003)(53546007)(50986999)(66066001)(38730400002)(217873001);DIR:OUT;SFP:1101;SCL:1;SRVR:MWHPR12MB1151;H:[10.236.64.195];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?Windows-1252?Q?1;MWHPR12MB1151;23:6L8cbeNm5+tDKmPOV8BUWJXj3pADKfjnGU3jJ?= =?Windows-1252?Q?8jhZTcEIfnLfcvIvoco33YsbSaxxM9wU4vsjomHl+t2xuU6cuM2elnaV?= =?Windows-1252?Q?HNd3sIxSxS+DN8r0VaX/pwD0kpkY+Gs5c0mQZJmQODot7XdipZOX7Eyn?= =?Windows-1252?Q?IGq45I1lfxmN3nlpTGsQDpzLRwIAafjB+45hVZqkN0Jx/otwAs32PZHX?= =?Windows-1252?Q?7W+ntE/38708UvxzP9E/7LDt7dJuba6M2p6b6ixep7jCZMscWfhP0b3q?= =?Windows-1252?Q?Exo+ayySIqp5Fjg2OQ7SGKJey7o/JOxlPtfMAvmMZcbaD0w2z+mojhV1?= =?Windows-1252?Q?NWEAJKuz6D+at2QBUGFtbTxWciYiehz3UZuT0RWVTQzCGL39w10nyh/+?= =?Windows-1252?Q?hVwQoeqsKhTxQjDj09ujI75qBKeqkZpzHRiLYzYbCy7cev0DIPyDW8LH?= =?Windows-1252?Q?2DFmjGQm4gvfY4qpRW+KjuhAr5Oh9tV75UiE2CZyIWGdTIoqddbwYzCT?= =?Windows-1252?Q?ckJT1BL9emszFHJbu15zxjo1NLcfqtSgM0I0MvJCMMx5iRlJDHT0LzpS?= =?Windows-1252?Q?3/kBPf+8qGxQ3qmCKbD1lVjDnzDVaRCqTpggiYtXBqt18VE62bxNoIU1?= =?Windows-1252?Q?Oai37k5F4Q9faR5HUJGO1WV2E+1twa0U9f1JC/8CPgnomZhMgS6VUDBt?= =?Windows-1252?Q?f8zS3TS4LMrenhk4eCcL7uSsd7XrneQqbQFoT8E+ejprt/58YjxPwPQW?= =?Windows-1252?Q?VjTodLPjI3qctNJ5sS03d/0NiCrwjxdYE7q3Phm5Wfl5kScKjQ0zmwWk?= =?Windows-1252?Q?Tc43jb6Swi0ocxjrF4R1ZNfGxUtgidpneOUR3IICRJf+R6xagO8rBXV/?= =?Windows-1252?Q?iCVlTNKf4ypHcWoDwENotr05pOPK5wIY47iI1YOyp0kI9Gt4WlOEL3Oj?= =?Windows-1252?Q?Rp2hDxTPsUDZVqFG1d3bhh1fD0Ul2E14CAvmCi0HK4xCvpEiRf3TUhZ4?= =?Windows-1252?Q?StTRvpPORKXaWPMWKzBt8OeMixd1ENw89ZDK9qiibwsjVMuF3cf0TjZ1?= =?Windows-1252?Q?vaMizHmKbXBouIxnEGxUkwdJYAs/XS+Hnt/V6ONjk4yb0tKojqBzALRw?= =?Windows-1252?Q?Haop8+ARm28We4vn7/n7ztLZhg4CqItGpVqb8k1j02rxIRZt+YpUaObq?= =?Windows-1252?Q?2dyElKdS91rf+bo12Hmlo12xSNaun7wd2ymk70vbGs5Ar5sKgtQO86Ws?= =?Windows-1252?Q?xBUwucLVrQZHmfQid3co5yWSC6wzIqAO5bvzf7hmsmNBz8qCuME3KXko?= =?Windows-1252?Q?apMOI7+eKvgMxl1Ms0TinBgQT8XtePBXletlUsCyqyVKtoczn09Dbu4j?= =?Windows-1252?Q?YtAJ13eoBL9SIJO+Y7qfStd13VLLYTZcnLHKlduBJVhkw+GGanylMdtA?= =?Windows-1252?Q?TmV0LrvWixR57/fq9O72k1b25ShKnQ+5pV84S/VhrCgiBNa2IAvFUGb5?= =?Windows-1252?Q?LlYrLrCvRoEjA9dQ9a2FIQOlxlYn+W4T4EbKutNDJIIaeVx9Q=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;MWHPR12MB1151;6:b81aAqOd46Xe7080Q7yxQpLOhbleUQJ+znpFTHzynfzNb9EQ2YvUQ/LLnnCToVIf46LJBKhLhoyPLpCsSn2M0brkBtao39c/PT7Crmj1Er+PmiKusZbSpbZ7cFRRZz/mFUVLccXMfPgXtgObAr7bixTwJGrEPv+6tZnk6CP6G7581Et+JwnakWQmqllgSMUrEQb77MQ/bvWMFERn93PdL7m+HyU2snfAMUrVjH2DHhrjp1e8dDkTSnedCZO7saPiSjeZ77lfCZ1nepDrsz41m7ddiF693m5EyGCJnBbWp7XHK0ePkOxFP0iGRHtG1SG5zh8ulLGxyNPxXPaoQy0Hs3R8JDRu+qH95t6O94iEksh2QcOo4pcQHagrGmDby0khZEh/v1DtrguRSCaeVKpFiYZofa5jB/Si29pEYSClY9o=;5:HJQPnVHTSOaVdvxfMbD6X5RGY9jxTC9rOBJaHajniBSj+9WLTc76nFlI9DX6FGVpjQODNh1PwPH7TZFc3nYJTHINzmNxQ2ZgpNS29zU/h+MVynMU4TaL2dPXZCsOOslWObef/HThLidpXLFy3HpKO7eJf8c+Ap3LJJfoErtJn9A=;24:AfLPW38DMqyeJM7BnqTr9feLqQmGSoTfrzIxjrvttnQTgWYB3U7Qmsq6SninGcTCN58ldXCXEx/6JGNvQb3zpAVaygBHygVHnR8ASQqSPRo= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;MWHPR12MB1151;7:yh5Vg3gSi5UtrywcIZ2Z65w0RiVEeQwQ+jhBeeqmvgzMYpuKd0Nk9N6GiPUhH5C2GBr7Cr46vlVUFAT66gwuLMp8UeYJAJERme6aBuOTLk7Itd6GabY6H/rClMZQz1VE11C9jnPKi9yv2ppAhKaFaaHAwisIBaipYehxk2oP0KHC1wrJ84IwunNptTtpLi7tMHiIRfuORtZu9JhewIY3deT69AidOnPDguxejSx5HMKZutUZoCOvuSxe+nR/MjqqdyIA1ASA0sCqx1j187eyDnQjmLhviOi3Yl1aRl68Fu1zQH7wYAf24F7ps1/lvar9XapzX/lFwRoUyYYCIVmknw==;20:EJfh8OA90sz9Tqn4wxyD4N1zTohP5vkFpWTN71G3E9tuhlBCOVf0RielF1945UM3qYbDpo3qOqe5P9F6+w0E4jQ9317rHIkObJ0B9xqvnCpr22B6iZhNnbyURkmSmyRCVZk7gZYuy+HOHTRjfo6c/NoE2h6eUy5xB7omolQHlwgHLPyu7BdeSCv4YXvHAj5AkMc3IpUktXbybeiHCWhuatLLfrNCgfDq/Y4fJEphRDnPIZwNvkka/buCfnl8ZrfA X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2017 20:09:02.1540 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR12MB1151 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 3/6/2017 6:03 PM, Bjorn Helgaas wrote: > 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. > Ok, that makes sense, will do. > 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. I'll do that. > > Also, change "x86/pci: " to "x86/PCI" so it matches the previous > convention. Will do. Thanks, Tom > >>>> 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 From: Tom Lendacky Subject: Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data Date: Mon, 13 Mar 2017 15:08:52 -0500 Message-ID: References: <148846752022.2349.13667498174822419498.stgit@brijesh-build-machine> <148846759008.2349.8274808454274279039.stgit@brijesh-build-machine> <20170303204209.GA31767@bhelgaas-glaptop.roam.corp.google.com> <20170307000349.GC5305@bhelgaas-glaptop.roam.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170307000349.GC5305@bhelgaas-glaptop.roam.corp.google.com> Sender: owner-linux-mm@kvack.org To: Bjorn Helgaas 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.orgbhe List-Id: linux-efi@vger.kernel.org On 3/6/2017 6:03 PM, Bjorn Helgaas wrote: > 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. > Ok, that makes sense, will do. > 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. I'll do that. > > Also, change "x86/pci: " to "x86/PCI" so it matches the previous > convention. Will do. Thanks, Tom > >>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f72.google.com (mail-pg0-f72.google.com [74.125.83.72]) by kanga.kvack.org (Postfix) with ESMTP id 03DDC6B038A for ; Mon, 13 Mar 2017 16:09:08 -0400 (EDT) Received: by mail-pg0-f72.google.com with SMTP id q126so318356237pga.0 for ; Mon, 13 Mar 2017 13:09:07 -0700 (PDT) Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0054.outbound.protection.outlook.com. [104.47.34.54]) by mx.google.com with ESMTPS id n1si12332959pge.384.2017.03.13.13.09.06 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 13 Mar 2017 13:09:07 -0700 (PDT) Subject: Re: [RFC PATCH v2 06/32] x86/pci: Use memremap when walking setup data References: <148846752022.2349.13667498174822419498.stgit@brijesh-build-machine> <148846759008.2349.8274808454274279039.stgit@brijesh-build-machine> <20170303204209.GA31767@bhelgaas-glaptop.roam.corp.google.com> <20170307000349.GC5305@bhelgaas-glaptop.roam.corp.google.com> From: Tom Lendacky Message-ID: Date: Mon, 13 Mar 2017 15:08:52 -0500 MIME-Version: 1.0 In-Reply-To: <20170307000349.GC5305@bhelgaas-glaptop.roam.corp.google.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Bjorn Helgaas 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 3/6/2017 6:03 PM, Bjorn Helgaas wrote: > 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. > Ok, that makes sense, will do. > 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. I'll do that. > > Also, change "x86/pci: " to "x86/PCI" so it matches the previous > convention. Will do. Thanks, Tom > >>>> 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