From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM02-CY1-obe.outbound.protection.outlook.com (NAM02-CY1-obe.outbound.protection.outlook.com [40.107.76.82]) by mx.groups.io with SMTP id smtpd.web11.17058.1611355240031896947 for ; Fri, 22 Jan 2021 14:40:40 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@amd.com header.s=selector1 header.b=aJLv2+sV; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: amd.com, ip: 40.107.76.82, mailfrom: thomas.lendacky@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=E8y4FfiU9SGC5yqBJR3rwr2f/m5JZ6HR+Evm1rdhBlXQOfBEIJ8es2+zV4LREiT1VUHN3snDM0neuLpEKxTWeRBJQVDY8ixCx/VGuLWMLlN+nhFW07wIcfEi5kFTqpJ34dREz8uLpx97xZzARnwmdrQ42MApJiRqF7Zmq8MwA5/Bw25UfRUFVoCt8p2xfemRSVtp0IAgQ9agXm9qimD+NKKtqpA++I/ipD8AAjrcbzpWlubWQmYqZq3nCUukwjz6a/Tyfpii/pnRHHc0p2rKYzKT9pySbAmU2Cqd3pijPAC8WmThpluluosJVmJ/m/84sRnRtcv8XAzXq8IX/XCjag== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=T/waxA4zEg/OWLNntXL46hgDO5IXMFMxSGtfDnsSfco=; b=XXQ7fFWkRgkuq3Y1zSih9f53nJK36fBotU3aqs2YrT01LQoL73a+hnDk4LzXH3PCU5Pk5kO0qDFwaDpFXQLYD2aeoVt5BpWUXZXkt3UUAd7P72jkp9cIPw9BCFAiypZfZwGN1vSxPXWfntOnBYmHhVt8w+yS2hT65GP7ZbXK9OYeojAvDO+ygx9mkZm0RmvEcKfvO0hbJEQKex0LaN0HlztBd8l3Wo2cn7CWDvRwpnyglA519LpF/QJAFvVgo9BzvSzN55xiGpnbb/u6OjRM6XlmqGqPA+aLds0Xg0KakgT0d7q/i6YMUvdk3sYmdKWFQYG8Uh1sq2gD+2FYINfskA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=T/waxA4zEg/OWLNntXL46hgDO5IXMFMxSGtfDnsSfco=; b=aJLv2+sV6IKSM0fTDznNOItcLqQihHaYka0VSYI2TthdUQHzYGVclpiVd13xFoP1KzwmuHHh8I6I4Qo3r+h+Fncb1ydGoel2mn7KIRZBraprQcm6ZHECcnwsn/oB+lY0HACW8h0V2c+tRfZWXQcK7ipHZf2R5oboshEJB3ZF9AA= Authentication-Results: kernel.org; dkim=none (message not signed) header.d=none;kernel.org; dmarc=none action=none header.from=amd.com; Received: from DM5PR12MB1355.namprd12.prod.outlook.com (2603:10b6:3:6e::7) by DM6PR12MB4218.namprd12.prod.outlook.com (2603:10b6:5:21b::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3784.13; Fri, 22 Jan 2021 22:40:38 +0000 Received: from DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::d95e:b9d:1d6a:e845]) by DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::d95e:b9d:1d6a:e845%12]) with mapi id 15.20.3763.014; Fri, 22 Jan 2021 22:40:38 +0000 Subject: Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES To: Laszlo Ersek , devel@edk2.groups.io Cc: Brijesh Singh , Jordan Justen , Ard Biesheuvel References: <8bba19920e58813f32889dec522bbcd8de113219.1611338106.git.thomas.lendacky@amd.com> <8d660790-fafd-c0d5-f1eb-1e02830a7fa9@redhat.com> From: "Lendacky, Thomas" Message-ID: Date: Fri, 22 Jan 2021 16:40:36 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 In-Reply-To: <8d660790-fafd-c0d5-f1eb-1e02830a7fa9@redhat.com> X-Originating-IP: [67.79.209.213] X-ClientProxiedBy: SN7PR04CA0068.namprd04.prod.outlook.com (2603:10b6:806:121::13) To DM5PR12MB1355.namprd12.prod.outlook.com (2603:10b6:3:6e::7) Return-Path: thomas.lendacky@amd.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from office-linux.texastahm.com (67.79.209.213) by SN7PR04CA0068.namprd04.prod.outlook.com (2603:10b6:806:121::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3784.12 via Frontend Transport; Fri, 22 Jan 2021 22:40:37 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-HT: Tenant X-MS-Office365-Filtering-Correlation-Id: 45f51ac8-b81b-4709-0585-08d8bf26bda4 X-MS-TrafficTypeDiagnostic: DM6PR12MB4218: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:5236; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: w29v6u3NMHfYq8ZO+lEAOrA1FVYdTzzhavnPpitmh7jVptbs8x/IgQEPgG9x+0KVrpR0tFuwmBu5zfZIk3wjcoeteQBw1T5zimu0ltEBonFhQbcqIaosGclf7ivk2SweCF/Z4STEn+0LPp/SGKbQeAZQsP61fZnUBcMJWLWnLvWQMGGr87F2kJcIqHiCDX7Aam5cyPgWCcF5n7dNjh1cBAm6DXnHocJRuZpiu4PLX5p7BVdt+2zdVGsECLDuOrBdiM4hbN431FYFRghMvWoxu+s6dWUmMk8sdlcaF6q1kOHn7/34Uufkmf+7lELgPWGUlMyvvojcP6Pj0Sjn8PQSUoykReqpCMv7WTRsgbnU1Lw2bT1GI5rKDqdt8lJzAaSCROT+Hjy3yuasjWXeGk9j7+4+cAIPmPrEOxRS/llMqS+qNHvVSm8L5/Vx0Ok8JrMAw/ZrX6MulZpfgjrYeLz+3N4PyAss7OoPkuIs9cV9ALaTP9JQ3KW6rNJtAKESdMxthDfaCzm6VEVaZ/rdQi8LqkMHt+KInuorQsh0IvM3yWEYa0FAPbJHltdaY8KjiHxBAJkQNIDhwzugFeb08WbzP4fwgBXwwZ10P0ln5FEjmX7NsfxiKZPNX9uEOvWvap+MzlRJj00D4jhA5wU5obq+lmKKWb1YceFyiXSbhxyLK1txy9Jm+rRvF1MVA4vX2qvkNcnndJLhS9/M1wWDtAxT0A== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM5PR12MB1355.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(39860400002)(376002)(346002)(396003)(136003)(366004)(6486002)(8936002)(6512007)(478600001)(5660300002)(45080400002)(316002)(54906003)(4326008)(16526019)(83380400001)(86362001)(6506007)(31686004)(2616005)(8676002)(36756003)(2906002)(66476007)(66946007)(26005)(53546011)(52116002)(956004)(31696002)(186003)(966005)(66556008)(213903007)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?OTVsNlhoWWlTTkRWQUY2WG92VEZreVRwK2ZnMW52dFgrQ0l3aWxUc2tNZlBE?= =?utf-8?B?c1JVL1RuWmtDbCtCS2Y2NWhRdnluRWl2WGIveGJEVkJGazdtdmx4MWJyNGZI?= =?utf-8?B?NE5JU3dheThYejdXaW5yQzBtNHlzd2pZRnZHYkdlWHhUQ0VrY09iRllYY1pr?= =?utf-8?B?TGozS0E3QU1mcGlxZG5mSXBmN040ckI5WVJQTzdoVjc5VWhmbVNHTjJMd0Jq?= =?utf-8?B?N1JjOHNFMnNYZU9CN2JXNnlUTzJERXY1ZHRaV01rZlB4MU9WckZER1Z0dVRz?= =?utf-8?B?MTZuVExzeWFYWHJHTm04VzdqUlUybVhVTHhnZkxNS0IzNkgweXJGZGw0N2sz?= =?utf-8?B?dXZSSkNrd2QxZWRLbUtRMGhrRmNPSkdlVEZyL0pEcWpKaTU4KzZZRkNWWXZi?= =?utf-8?B?OEh4VXB3UnVnOXZsTERaZ3pNd1Mvc01yMmVGM1dJZnVvMTBJNkFqYWhpNmIx?= =?utf-8?B?d0tiS1h5WW1tSzlLMXVNTUJwaVZKdEhGakxIT0w4Q3dvUFhsWk42VEN4bVd1?= =?utf-8?B?cFFBblZXdC9CUThxTWY5OE01RlZpVjdDS3R0ZkNTa2RhNmxXRnNWRnd0VUpP?= =?utf-8?B?ZHBQM3VHaTJxRVRpRzEyNVpGcFNLZnMwWmNQWXhuektnRnMzaEZtaENzMUFH?= =?utf-8?B?NFpOekhoUUlVbEpqWDV1VThjY3J6bi9tWTRWYzNNa0dyVWFVKzdET052R0JF?= =?utf-8?B?b3Q4Z0VFWFNiVUdBbzh6YmxZb2FqOHNGQVpWSG5iYk9XS09XTWdJS1F4aGx0?= =?utf-8?B?QzY3bGhaMDFaVW9NZytaeUxlcTgrdjFQNE9LbEpCNkl6enNPeHhGNlNoR1JL?= =?utf-8?B?SFhjV3FEV0dSZzBDUFBmbXZHZGFUTzVJNjlxUHJTR3VRRjZ1Y1ZacWF6VlpY?= =?utf-8?B?WTJteDAzNVBCUll1SFNHTG1ablJ5TXJCNllCaGhhTmwxS050cG5HcXVtSnBa?= =?utf-8?B?Rmh0UFhxaGJtTUQ5NVI2WUYyMEJmRy84dUk4Z1QwU3Z2L3lxblhGcEpnNUxx?= =?utf-8?B?TUgxSTRHRXNxU1BTaHI2dXhRTE1kUjA3WStjSnpzQlM5alpjSk9xZWVHdXVZ?= =?utf-8?B?QWcraFErUlBJcjB1YU5lTEZCMUN1b0VIZGdDVzZBRnY0YkM1TlBjbVdkMXZ2?= =?utf-8?B?MUthYnhPYVJUY3UwQ0ZXN0Z5eEpYbGVEN3BVR2xqN09YaXVRTzIvempEVjgv?= =?utf-8?B?ZDdPOE43eEx6emhrZ3FtUk9leXU2ZzZjWnkxVW9HeS9nV20rTlJnMDNxN1NI?= =?utf-8?B?NTFCOFphYW1EN2hhOG9ENjArbkZZclpzZXNYalZIMTd0amlxUDREU1BvYkh0?= =?utf-8?Q?LC0NExZkM3lVEcbTxIr+xZx26WOnpXvDsv?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 45f51ac8-b81b-4709-0585-08d8bf26bda4 X-MS-Exchange-CrossTenant-AuthSource: DM5PR12MB1355.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jan 2021 22:40:38.6209 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: Vs2hnF8qIfe7IgMvaVEgjpZ4NxXfsmxFWZHTfwTemWlcLvPmkF8FEouC2/yYisK6OzQNQHhsxOMhMHrEKXTo8g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4218 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 1/22/21 4:29 PM, Laszlo Ersek wrote: > On 01/22/21 18:55, Lendacky, Thomas wrote: >> From: Tom Lendacky >> >> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3183&data=04%7C01%7Cthomas.lendacky%40amd.com%7Ced6105527a884016335708d8bf2533d5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637469513810518842%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=%2F%2B2OTbAeYDgp8lHsdx7%2B%2FhsCItm5x4yRFiQbxrAoTw8%3D&reserved=0 >> >> Under SEV-ES, a write to the flash device is done using a direct VMGEXIT >> to perform an MMIO write. The address provided to the MMIO write must be >> the physical address of the MMIO write destitnation. During boot, OVMF >> runs with an identity mapped pagetable structure so that VA == PA and the >> VMGEXIT MMIO write destination is just the virtual address of the flash >> area address being written. >> >> However, when the UEFI SetVitualAddressMap() API is invoked, an identity >> mapped pagetable structure may not be in place and using the virtual >> address for the flash area address is no longer valid. This results in >> writes to the flash not being performed successfully. This can be seen >> by attempting to change the boot order under Linux. The update will >> appear to be performed, based on the output of the command. But rebooting >> the guest will show that the new boot order has not been set. >> >> To remedy this, update the Qemu flash services constructor to maintain a >> copy of the original PA of the flash device. When performing the VMGEXIT >> MMIO write, the PA can be calculated by subtracting the difference between >> the current flash virtual address base and the original physical address >> base. Using the resulting value in the MMIO write produces a successful >> write during boot and during runtime services. >> >> Fixes: 437eb3f7a8db7681afe0e6064d3a8edb12abb766 >> Cc: Jordan Justen >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Signed-off-by: Tom Lendacky >> --- >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h | 1 + >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 2 ++ >> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 10 +++++++++- >> 3 files changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h >> index 219d0d6e83cf..0a91c15d0e81 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h >> @@ -13,6 +13,7 @@ >> #include >> >> extern UINT8 *mFlashBase; >> +extern UINT8 *mFlashBasePhysical; >> >> /** >> Read from QEMU Flash >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >> index d19997032ec9..36ae63ebde31 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >> @@ -26,6 +26,7 @@ >> >> >> UINT8 *mFlashBase; >> +UINT8 *mFlashBasePhysical; >> >> STATIC UINTN mFdBlockSize = 0; >> STATIC UINTN mFdBlockCount = 0; >> @@ -251,6 +252,7 @@ QemuFlashInitialize ( >> ) >> { >> mFlashBase = (UINT8*)(UINTN) PcdGet32 (PcdOvmfFdBaseAddress); >> + mFlashBasePhysical = mFlashBase; >> mFdBlockSize = PcdGet32 (PcdOvmfFirmwareBlockSize); >> ASSERT(PcdGet32 (PcdOvmfFirmwareFdSize) % mFdBlockSize == 0); >> mFdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / mFdBlockSize; >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c >> index 1b0742967f71..db247f324e3c 100644 >> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c >> @@ -52,11 +52,19 @@ QemuFlashPtrWrite ( >> if (MemEncryptSevEsIsEnabled ()) { >> MSR_SEV_ES_GHCB_REGISTER Msr; >> GHCB *Ghcb; >> + UINT64 PtrPa; >> BOOLEAN InterruptState; >> >> Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); >> Ghcb = Msr.Ghcb; >> >> + // >> + // The MMIO write needs to be to the physical address of the flash pointer. >> + // Since this service is available as part of the EFI runtime services, >> + // account for a non-identity mapped VA after SetVitualAddressMap(). >> + // >> + PtrPa = (UINT64) (UINTN) (Ptr - (mFlashBase - mFlashBasePhysical)); >> + > > (1) The formula seen here is indeed correct, IMO, for expressing the PA, > *mathematically speaking*. To explain it to myself, I used a different > formulation: > > - "(Ptr - mFlashBase)" is the relative offset into the flash, where both > "Ptr" and "mFlashBase" are virtual addresses -- the subtraction > basically undoes a part of QemuFlashPtr(), > > - and then that relative offset is added to mFlashBasePhysical: > > mFlashBasePhysical + (Ptr - mFlashBase) > > Mathematically, this is exactly the sum that's in your code too, but I > think this formulation is easier to understand. Upon reading the commit > message, I didn't understand the goal of the (virtual - physical) shift, > until I saw the formula. > > Would you agree to reword the commit message accordingly (the last > paragraph)? Certainly, I can do that. > > > (2) Although mathematically the PtrPa calculation is OK, the C > expression itself is not so nice. It contains a subtraction between > pointers that do not (necessarily) point into the same array (or one > past the last element in the same array). Such a subtraction is > undefined behavior. > > But, I'll address this below, as a part of point (3). > > > (3) It should be possible to implement this change within > "QemuFlashDxe.c"; not having any effect on the SMM build of the driver. > > (3a) I suggest introducing the following variable to "QemuFlashDxe.c": > > STATIC EFI_PHYSICAL_ADDRESS mSevEsFlashPhysBase; > > (3b) The following should be prepended to the body of > QemuFlashConvertPointers(): > > if (MemEncryptSevEsIsEnabled ()) { > mSevEsFlashPhysBase = (UINTN)mFlashBase; > } Can SetVirtualAddressMap() be called more than once? I can always make that: if (MemEncryptSevEsIsEnabled () && mSevEsFlashPhysBase == 0) { > > (QemuFlashConvertPointers() is called from > FvbVirtualAddressChangeEvent(), in "FwBlockServiceDxe.c", which is the > event notification function for SetVitualAddressMap().) > > (3c) I propose the following for the SEV-ES branch of > QemuFlashPtrWrite(), in "QemuFlashDxe.c": > > EFI_PHYSICAL_ADDRESS PhysAddr; > > if (mSevEsFlashPhysBase == 0) { > PhysAddr = (UINTN)Ptr; > } else { > PhysAddr = mSevEsFlashPhysBase + (Ptr - mFlashBase); > } > > (3d) EFI_PHYSICAL_ADDRESS is a typedef to UINT64, per spec, thus you can > pass PhysAddr to the SVM_EXIT_MMIO_WRITE VmgExit() call, without a type > cast. > > I feel that this approach keeps the SEV-ES logic better contained. Yup, I'll update it. > > (4) Please link the patch emails (v1, v2 mailing list URLs) into the > bugzilla ticket. Ah, yes. Will do. Thanks, Tom > > Thanks! > Laszlo > >> // >> // Writing to flash is emulated by the hypervisor through the use of write >> // protection. This won't work for an SEV-ES guest because the write won't >> @@ -68,7 +76,7 @@ QemuFlashPtrWrite ( >> Ghcb->SharedBuffer[0] = Value; >> Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer; >> VmgSetOffsetValid (Ghcb, GhcbSwScratch); >> - VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1); >> + VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, PtrPa, 1); >> VmgDone (Ghcb, InterruptState); >> } else { >> *Ptr = Value; >> >