From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web08.1516.1621129176907809450 for ; Sat, 15 May 2021 18:39:37 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=PO6wF0Op; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1621129175; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ROkvgwFWRz9MhiQfNlaq4wMJYqEOiEHVJowvSlJXnGA=; b=PO6wF0Opupca/wtVR4mxJBGoL4Y3wiu/wtu5W6V2+cMNgdPJ6FKOTfneaIiBSae8ufDVzj Fe76ceAzGQxi9d0LW/W2/RHaQUJA1VBCTHpLGqE8OXEgmXsUxHoc5aGaPOYgSWcBeVbBia xn9iV8tYLcr3YoSju5rpM1NX/WzcGnY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-306-2tCSEW0aMkWTVNJxiyGTMw-1; Sat, 15 May 2021 21:39:30 -0400 X-MC-Unique: 2tCSEW0aMkWTVNJxiyGTMw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 591A318397A8; Sun, 16 May 2021 01:39:28 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-2.ams2.redhat.com [10.36.112.2]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A6DEA10074EF; Sun, 16 May 2021 01:39:27 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Remove hardcode 48 address size limitation To: devel@edk2.groups.io, ray.ni@intel.com References: <12259.1621037062277250978@groups.io> From: "Laszlo Ersek" Message-ID: <877e17dd-8235-1a56-13c1-c61a505d543e@redhat.com> Date: Sun, 16 May 2021 03:39:25 +0200 MIME-Version: 1.0 In-Reply-To: <12259.1621037062277250978@groups.io> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/15/21 02:04, Ni, Ray wrote: > Laszlo, > Do you think that another API is also needed: GetPhysicalAddressWidth() that returns number 36/52? No. The GetPhysicalAddressBits() function that I proposed already returns this information. It has three outputs: the number of bits (that is, the width), as return value, and the two optional output parameters. So if you only need the the bit count, call GetPhysicalAddressBits (NULL, NULL); These calculations are so cheap and small that keeping them in a single function makes a lot of sense in my opinion. > physical address width is needed by (besides those that rely on the width for mask calculation): > UefiCpuPkg\CpuMpPei\CpuPaging.c > UefiCpuPkg\PiSmmCpuDxeSmm\X64\PageTbl.c > MdeModulePkg\Core\DxeIplPeim\X64\VirtualMemory.c > MdeModulePkg\Universal\Acpi\S3SaveStateDxe\AcpiS3ContextSave.c > MdeModulePkg\Universal\CapsulePei\UefiCapsule.c > MdePkg\Library\SmmIoLib\SmmIoLib.c > OvmfPkg\XenPlatformPei\MemDetect.c > UefiCpuPkg\Universal\Acpi\S3Resume2Pei\S3Resume.c > UefiPayloadPkg\UefiPayloadEntry\X64\VirtualMemory.c Ah, I couldn't find those because the AsmCpuid() calls in them don't even use the symbolic names for 0x80000008 (CPUID_VIR_PHY_ADDRESS_SIZE) and for the least significant byte of EAX on output (CPUID_VIR_PHY_ADDRESS_SIZE_EAX.Bits.PhysicalAddressBits). So it's even worse (much worse) than I expected :( Because of the MdePkg and MdeModulePkg dependencies, we can't even put the helper in UefiCpuPkg; it must go into MdePkg (possibly BaseLib, I'm not sure). > > > GetPhysicalAddressMask() can call GetPhysicalAddressWidth(). To me two functions are not really justified, because the address width, and the bit masks are so closely related. But I'm also not too opposed to having two functions. > > Since it's a large-scale change but the SMM high MMIO access bug is critical/urgent, I prefer to firstly push this bug fix change and then work on the new APIs. > > https://bugzilla.tianocore.org/show_bug.cgi?id=3394 was submitted to capture this. For a critical bugfix, I would prefer not mixing the actual fix with the introduction of the symbolic names. Your patch currently fixes three things at the same time: (1) coding style (it replaces magic constants with macros / type names), (2) a bug in calculation, (3) a missing CPUID "maximum function" check. Maybe writing a separate patch for each of these is unjustified, but I was really unhappy to see that the commit message said nothing about (1) and (3), and I had to hunt down (2) between the other changes. The minimal fix -- that is, the fix for (2) -- would be just one line: diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c index fd6583f9d172..4592b76fe595 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c @@ -1920,7 +1920,7 @@ InitializeMpServiceData ( // AsmCpuid (0x80000008, (UINT32*)&Index, NULL, NULL, NULL); gPhyMask = LShiftU64 (1, (UINT8)Index) - 1; - gPhyMask &= (1ull << 48) - EFI_PAGE_SIZE; + gPhyMask &= 0xfffffffffffff000ULL; // // Create page tables I don't like that the patch currently does three things but only documents one. That said, if you are out of time, feel free to go ahead with Eric's R-b. Thanks Laszlo > > > > > >