From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932444AbdJJPA7 (ORCPT ); Tue, 10 Oct 2017 11:00:59 -0400 Received: from mail-bl2nam02on0063.outbound.protection.outlook.com ([104.47.38.63]:27904 "EHLO NAM02-BL2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751385AbdJJPAv (ORCPT ); Tue, 10 Oct 2017 11:00:51 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=brijesh.singh@amd.com; Cc: brijesh.singh@amd.com, Paolo Bonzini , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Herbert Xu , Gary Hook , Tom Lendacky , linux-crypto@vger.kernel.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support To: Borislav Petkov References: <20171004131412.13038-13-brijesh.singh@amd.com> <20171007010607.78088-1-brijesh.singh@amd.com> <20171007184049.jrbxebb4jlciu3hj@pd.tnic> <20171008140019.flvyovgq2xpqdcoq@pd.tnic> <7131ad30-6c07-16d5-8cfc-06d446a66dca@amd.com> <20171009152130.vo2lpwdvcs4lyb2l@pd.tnic> From: Brijesh Singh Message-ID: <9bee3ad7-2a2c-137f-1f2f-f6b0d4128474@amd.com> Date: Tue, 10 Oct 2017 10:00:43 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171009152130.vo2lpwdvcs4lyb2l@pd.tnic> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [165.204.77.1] X-ClientProxiedBy: MWHPR18CA0028.namprd18.prod.outlook.com (10.175.9.142) To SN1PR12MB0159.namprd12.prod.outlook.com (10.162.3.146) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 534e2103-8fc4-4147-c5aa-08d50fefb130 X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(2017030254152)(48565401081)(2017052603199)(201703131423075)(201703031133081)(201702281549075);SRVR:SN1PR12MB0159; X-Microsoft-Exchange-Diagnostics: 1;SN1PR12MB0159;3:v713ktCe0fQ1q4HatUd725J1IuAVzIKOH9VRllZy+CrJwW6bpdFxVA1EgqzLxsn5t709X49CVbVar7U8wh2UgLrEvAwRlQmkAPb1bK6Pa6us3rh4oE+URP0EUJtOVd1R6apab7ifpqLEN2PqpzsVtD0oootnSonhSHmrkngCn7Dv7YvrxgtLr5TJio37jvKuKSELvf5LZhqalYgtPYecEaY7XUkrLHUHb2yQb9+0xQ4f+MTnn6WyjvTselzz9vLh;25:XQFPNvM9qd8zPvGWjdvlflPoAWBiUZbK7DDbVNFVAKkc2BphbjPzr4tEwRPftV52fBE3jTSLqskUgefs6oCq3+zCLdAycq2/CMwccBKuyL0bko4yDaY0YSKn9H6/FNOe3qQ6fGOnnz2Zl60EDv15UaY21c45QvFRWfX7EP3+L5/Pz2bzY9LpqhdHRvZEEuYwCcq7Xj2KgiODvJUDIokSYvsyTrXrG/SKjrvS4P8KzZXlZ5+gBeMQZCLjo/cIhpnN2UO/H4NJQXdvUlaUzr+nh4nlrhrXhQcijQu+YDNY4kzkeD7Jjz3azTk35IThLCrMA2AIe0wnpiIcY0gUSgqs0A==;31:pr4ZXVXU9nwrpgaT8crOBql4IJtjFkEoyAJhuDs9PXWQjFI22qnFRG+sUtPQ6PuosVrQ9Zh984yxiTMC8deeyJeZcuvekDi1bYm+mPurx4Dgr5SQ6WSqhffavv/DECDhaxOYGdEDZ2tbwLUC9P88LSX32eWW2V9EQ/x++CK3OSWITLulFzsDbhETeENRmNh+xWAIGXYBSfIDJwJL17MB9bjOtTihElPpfEqkDLqfVwk= X-MS-TrafficTypeDiagnostic: SN1PR12MB0159: X-Microsoft-Exchange-Diagnostics: 1;SN1PR12MB0159;20:nOsdtdcwvRZiJWb+1fduy6EqI4fQ7/QQAwhGKGbaisx+ZL0UWa5uz3GNh/L9mStKIbwQ4/5yP+p6Ta1iftotsKSWgmMdi/Lql7rBdSuwJv9vN3rNd3exlAcIB0WQ4PLC5Ym9wLVMfDED5xHUEMEqipFigl6/Zc5KstTwmxVz9bqu1Z2ikwKCmk6Pd0a8qGZIvUparRCOLjnrtTmAUWDkTAf40ICIUhPdBAso+rMvrXLFO50Zzthe/ZkNk5EI04cSkW2/Qg2REpxCr2RuiiFiHP1UAoRE8p34BBYJeRQ/6YrbcWKKV9Nq8g3iXHbVKxgSZs07WYcV6VUhjGu0dwLVuldivE3FGWD6WGwEx4GuXeITQGfOganJ0W3jrpWyi3UBNmVnnjdlOLHqlvit0/+Ii6u6YcYZTFxXM/urir9nqsbA7aLpHr294Cm8QzkNXPwJ1XM6iZWnMJvGmo7IDRIp8ZAd3Tg+Gt7c3x7YQsCeK2I3IPg6UjmcpYaP5jJ4p/YN;4:ktce3mzUMGhcFHV72IeGr79ww+J98WMwWv1vzR8olyZt7mkVi5LWU7WFIv6qTlxOOasMDCfu7dent9I9+n6+HF8sGuVAnYBkLq1c8OJsyRXQtZwrBUWnvgiqgKf7vJag2qgmHzCmRjrO5OUmV5hro6xaWWsylRVUmc/ejMizbd5ZsULGbPoMqLSIroYWta6WwcEvBuYEoeSAPLf1L+lqGCCnpQYcuNCwx4iTiIUO1k2GyYYkUd00wJRpw7OSL+Y7h7wRo5y9wbiJvJ3xglhUWWShCSTO43+9bS6hZu4s+QateI6iU3N6tmJG7sMSvQFE97TJQSaRnMn84U7mNvrGNw== X-Exchange-Antispam-Report-Test: UriScan:(788757137089)(17755550239193); X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(8121501046)(5005006)(3002001)(10201501046)(93006095)(93001095)(100000703101)(100105400095)(6055026)(6041248)(20161123555025)(20161123564025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123560025)(20161123558100)(20161123562025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);SRVR:SN1PR12MB0159;BCL:0;PCL:0;RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);SRVR:SN1PR12MB0159; X-Forefront-PRVS: 04569283F9 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6049001)(6009001)(346002)(376002)(39860400002)(377454003)(5423002)(24454002)(189002)(199003)(58126008)(31686004)(16576012)(16526018)(90366009)(50466002)(54906003)(64126003)(5660300001)(305945005)(6486002)(229853002)(77096006)(65826007)(4326008)(7736002)(23676002)(53546010)(6116002)(2870700001)(3846002)(33646002)(97736004)(2906002)(25786009)(189998001)(8936002)(93886005)(6246003)(31696002)(106356001)(53936002)(36756003)(101416001)(76176999)(50986999)(54356999)(8676002)(47776003)(65806001)(86362001)(65956001)(6916009)(2950100002)(316002)(6666003)(83506001)(68736007)(66066001)(81156014)(478600001)(81166006)(105586002);DIR:OUT;SFP:1101;SCL:1;SRVR:SN1PR12MB0159;H:[10.236.136.62];FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtTTjFQUjEyTUIwMTU5OzIzOkI0VlR1bk52Y0FYd1VmSkUvK3RkMHpZSURa?= =?utf-8?B?b2NsS3pPNlZEaUxBT0tqdlIxNklhZXlDWEtNM1g2OHhSNWVoTVZBYVZQdkZs?= =?utf-8?B?dmdkdDRFZEc4aHFERjVxQUFEZmxVSjlKRTJiWHBsQXZ6czNMNC9FUnBGNElI?= =?utf-8?B?MG56SE4zSGNmMmdaQjBRV2xhTXpidXJiRmFkNEFCaU5Xc1BSZzdsV0NrN0o4?= =?utf-8?B?QlJCajBqVzJlaEpqVTdSWFRJZUMvOVZDbnoxTkxxNUdPSmxqY0UxRzB2UWQw?= =?utf-8?B?QkZrbytuSjhnbFdLYXdTaU85V3hPTzlXVUprY1VTelZ1K1phajNKN2dIVzNm?= =?utf-8?B?RlkweHk5cWxPaks2cHpXd1E2QWxoSXJqUURyZzZ1WFhycFQxRG03dXpadXV5?= =?utf-8?B?MW50TmFGa0tHNGVuOEdSM3hJQmhZTmJCTXNkbUF6aklYV2JzK2VnUTNsRlI5?= =?utf-8?B?U1ZQd2kyNm5yQzJBZUlBeGVLQytTT2hxY0FkRjhiNW9taUM1RVphMUMwVFdN?= =?utf-8?B?K24vOEU5UXpZTHZRaGdvb05ueWpVS1dRSXdRMmVHdzhDWllnK0R6emJqbk4r?= =?utf-8?B?clFmZDZBSGNVQkJSRUttc0xtTGdFUE9tTlh2dG9pYzNKcUhyQlZ0emlGWGdt?= =?utf-8?B?UXFqU3lGWlpvZEg4Tmt4VjBJOHZoZnNxUnRidnNQZjA0b24rUGJ2MFRvQk53?= =?utf-8?B?SWcrcEJrMVpkQ1MvbUkwUkloV0Rqa2FtZ1pRUFNWMHBkbHJuSjM1MjNoSVBN?= =?utf-8?B?cXdLNndHdVl3NW1iWEljY2h1YVVTV05QZEVTRm42SFZoaVhpY3YyUUJMUEl6?= =?utf-8?B?bzBWQmVrMDJPbis4RUFjRysxTVZsbXBnR3BXUXA0MkR0NytrUjQrbmw1S1Bj?= =?utf-8?B?aUY3cU1yUkF5MjlMTU1icXVoQVZGOC9EbXpjbzVXZm9LdXViV1o1bDAvNUda?= =?utf-8?B?RWpiK1dsaFRMd0VvVDJaamlGb2lLTUVaN09WOTdTZXFBU1hUY3p6b0U4WWtY?= =?utf-8?B?VUdKaUMxV2E2SFZHczJ3d21leWhzL000ME8xWklFQmdxWW95MjZvZzZGUTRn?= =?utf-8?B?SjY1UjA1amlFdFAyUEY5bGk4Q0RJNk9Wc2dJdEpndE5EU2V6aTB3Snlxakdn?= =?utf-8?B?THR1Rk5JWEFzaTRhaUhiVUpHMUo4d282TzA5TzR4Nyt5N0pqRHJ2NzZ2QVJH?= =?utf-8?B?MFZCbnoyUmVEWlFTSUpISzJEQStSaE9jUkZ1V2dmM2ppMmRJY05FYlExT1k2?= =?utf-8?B?MFZvWUxGbFBIOENOVHdtTmFHNFVLRTdtaThwZEMwd2RueW1iQnRCSHgwSmZQ?= =?utf-8?B?bEtxV2ZHWUJoY21mNVNrclVyWW9ldnFaSkF3N2g4U2tLcG1ZUVNPbDZJN1ht?= =?utf-8?B?MGJSeStodlRvODRNVmR6SExnQVNsNEhBN3o0Tm8rdVNiMHNPczVXYXdmNUFJ?= =?utf-8?B?aUVMUVA2dk8vYy9EUU1CRmRXejBZcFZoV3BKZXlJYXVoVEt3RDVSNkpkMllC?= =?utf-8?B?K2JqUS9CMWN5SE9WZm5uOHVSZlBvL2NyK2JXN05laGxPZmxUc3FFL1gxYlJ6?= =?utf-8?B?OFpaa0JlMkZJNStzQ0RxTFRkRS9SZDdrNEZhdmVOcHppNkJydlBlL1BxbWhV?= =?utf-8?B?Yi9LU0MySVdnS3NqelVUaWFzU2NiZU5pOC9WUDVURUsxWHFNUm5waS8reFZQ?= =?utf-8?B?SnZqQWxiSlBaa0JQdzQ1aGdoQnRuWWhYTk9uNkNUN2VNVy9sclR2TmJFcCty?= =?utf-8?B?ZVJzSjhENmZ3UEpwbjdDVXVtV0hvZ08yNUN6bFYvNzNwa0Y1ZjhpZkNCM2kr?= =?utf-8?B?enptRDM0MUtxSHVjQ0hMMXV5Um1yUUhyejZQaEJkMFBkRWV2SjExU1BZQ2F6?= =?utf-8?B?S3pVM1p4UUhWYXhQR2JQSnBINWJINkZWaXJ5M3N1RndNSTlDaWMxV1hnNHRB?= =?utf-8?Q?EqiAG3mOseLR7rnyjJhJnOvmomcxoE=3D?= X-Microsoft-Exchange-Diagnostics: 1;SN1PR12MB0159;6:MKA+n/4g3p7LQqqlHjtmtyvXxbo8gSGmyKXRd1eohXOb7s4pzc0JKchZBybSmMPmmT7EH3yBm9gERRaHA/NvjRoIn5GdYvEQn0cY38I1VkMEH7erVQ9pMx0+RaJz3cnldUAxDN8EidkETks1EXPBoNmIsmbIF88wslCh+GkP2JIfPSBBFzGmQ8g55rrIgQrLqITEUjJXxxJbvBLVeL3gVRbHM83x3v3GuQdU4c7Zp2BX4h32mqCCLfZbjWu5Nulbl579/y7fXgj3FP0DDwsK88sXmMmrWqYwwLuQ0uOvKymJlPANxQfRRxpHAsyHbyMAoGE9zInlSBKtYqb5gFuTOg==;5:soWvWelRj1zITA33nh+sSIT/UydIsQ3XnikXcETo9Oo2wpDORgizgmJPjc5MbqSPian9ppSg7BZSTEIRc0sehVa8z+o91MVCh3RP76cClPgBXxhBlqZnJiF9Jw9CYJhSl5RHqHICiM+pf25Vz5YhlA==;24:JB3SPh4Buv5dGGUPwFz2ohFOFxoajZlJHSDYTIrW6bWdxjRO/PDBlncQmL/J9fjeNtG9sVPEsrE3lVC+Lpkr8r21leUwHSr30oEfRaKCSms=;7:5QWJKk1YLWwMy0nkP3OM5NG+4ZIRU/dyjpun5JgpryXJqRPVhQ7vp048IIlx8yaXHMq1M+q9TtI2I5h86u3iCdI0/YhBgeZLJyUAzhORD1ci72rZcewCv3VL7eKaWNGgkkcvb+jGQosqgvZ70s5JhyrZM5gIKnxXOtmA14s0tiO/rVhS4o8qHPS6Le3XlLXkiQPpIXycEksokc18ytT1iJztc9+sWVbah5G/Z/X955k= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;SN1PR12MB0159;20:ZU+nLk7j2BFLfplPGMypAZ0VRZiBAhBU0ugtNxj5cZGGme8v3HiGD1CGeC5f/9qJ1O3nFq7D+FlqVH4tpHuhHHKSYq8kyfDPTNjksm4Imy7FKFUBEpy8NMhZzES5ITPGpJgNtiL9hAWHG4qlqRTVBZMnKaR8rzAFzrddUD8EAGfBVonToLOBCfDVEO5PYSTdGK+PWKNOG0BkXGwbB4KC9uPqwFH7xy60oySfXQg1lWKvzkEIW9vUgL/uSR1FLpya X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Oct 2017 15:00:47.4970 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB0159 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/09/2017 10:21 AM, Borislav Petkov wrote: ... > >> 03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device >> 1468 >> 13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device >> 1456 > > Btw, what do those PCI functions each do? Public PPR doesn't have them > documented. Looking at the pci_device_id table (sp-pci.c), the devices id 0x1468 provides the support CCP support directly on the x86-side and device id 0x1456 provides the support for both CCP and PSP features through the AMD Secure Processor (AMD-SP). > > Sure, and if you manage all the devices in a single driver, you can > simply keep them all in a linked list or in an array and iterating over > them is trivial. > > Because right now you have > > 1. sp-pci.c::sp_pci_probe() execute upon the PCI device detection > > 2. at some point, it does sp-dev.c::sp_init() which decides whether CCP or PSP > > 3. If PSP, it calls pcp-dev.c::psp_dev_init() which gets that > sp->dev_vdata->psp_vdata which is nothing more than a simple offset > 0x10500 which is where the PSP io regs are. For example, if this offset > is hardcoded, why are we even passing that vdata? Just set psp->io_regs = > 0x10500. No need for all that passing of structs around. > > 4. and finally, after that *whole* init has been done, you get to do > ->set_psp_master_device(sp); > > Or, you can save yourself all that jumping through hoops, merge sp-pci.c > and sp-dev.c into a single sp.c and put *everything* sp-related into > it. And then do the whole work of picking hw apart, detection and > configuration in sp_pci_probe() and have helper functions preparing and > configuring the device. > > At the end, it adds it to the list of devices sp.c manages and done. You > actually have that list already: > > static LIST_HEAD(sp_units); > > in sp-dev.c. > > You don't need the set_master thing either - you simply set the > sp_dev_master pointer inside sp.c > I was trying to avoid putting PSP/SEV specific changes in sp-dev.* files. But if sp.c approach is acceptable to the maintainer then I can work towards merging sp-dev.c and sp-pci.c into sp.c and then add the PSP/SEV support. > sp_init() can then go and you can replace it with its function body, > deciding whether it is a CCP or PSP and then call the respective > function which is also in sp.c or ccp-dev.c > > And then all those separate compilation units and the interfaces between > them disappear - you have only the calls into the PSP and that's it. > > Btw, the CCP thing could remain separate initially, I guess, with all > that ccp-* stuff in there. > Yep, if we decide to go with your recommended approach then we should leave the CCP as-is for now. >> I was trying to follow the CCP  model -- in which sp-dev.c simply >> forwards the call to ccp-dev.c which does the real work. > > And you don't really need that - you can do the real work directly in > sp-dev.c or sp.c or whatever. > >> Currently, sev-dev.c contains barebone common code. IMO, keeping all >> the PSP private functions and data structure outside the sp-dev.c/.h >> is right thing. > > By this model probably, but it causes all that init and registration > jump-through-hoops for no real reason. It is basically wasting cycles > and energy. > > I'm all for splitting if it makes sense. But right now I don't see much > sense in this - it is basically a bunch of small compilation units > calling each other. And they could be merged into a single sp.c which > does it all in one go, without a lot of blabla. >> Additionally, I would like to highlight that if we decide to go with >> moving all the PSP functionality in sp-dev.c then we have to add #ifdef >> CONFIG_CRYPTO_DEV_SP_PSP because PSP feature depends on X86_66, whereas >> the sp-dev.c gets compiled for all architectures (including aarch64, >> i386 and x86_64). > > That's fine. You can build it on 32-bit but add to the init function > > if (IS_ENABLED(CONFIG_X86_32)) > return -ENODEV; > > and be done with it. No need for the ifdeffery. > OK, i will use IS_ENABLED where applicable.