From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f179.google.com (mail-pf1-f179.google.com [209.85.210.179]) by mx.groups.io with SMTP id smtpd.web11.5097.1618550238848155831 for ; Thu, 15 Apr 2021 22:17:18 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Wfi1JYb2; spf=pass (domain: gmail.com, ip: 209.85.210.179, mailfrom: kuqin12@gmail.com) Received: by mail-pf1-f179.google.com with SMTP id i190so17576741pfc.12 for ; Thu, 15 Apr 2021 22:17:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=gWQI+kmbhdBSbdlGjrifHhnu97/6koOBkBo/jEoWr0Q=; b=Wfi1JYb22LHohdSvqoO5HmLj7kgftSNcFjKN8vEmoAV+26upletnrdLcmlToO5Vfyh 1TmsGGfxsV6W3eBcFn3S/1FIvdhm2xQ03lIpCnXG/OhhCGVSXa1gEfHB6DFhEidyIds+ 4+z/7+YF4XHuTKSDRI4/FDGk4Tg03Cn+8/J2Ran3WCwzc4kszS9icc3sA+8c1Im+poL3 BD35kmH9a5srKMBuBlQq/SEmgnE74nTFV9DvqMFqc5zNnOm/6e9z4P5sH6VPGCtsr7r/ 7VTdWHyIU/j1NrUxZbpGjm5YDqNAS+vvuEwPLE+KVXPGaSY5KzX1rgvFkrxaCo10yKAF cbVQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=gWQI+kmbhdBSbdlGjrifHhnu97/6koOBkBo/jEoWr0Q=; b=FUuwXhyhu+Iu90xeGwSwpJHF55ssfDnnA9RVn/dpkBSD1FTlCvkaRpm+wlBq/UXyuc zM1qg30VvQtIh/g2RjZJdYpaxRurqQrt78FJnaSlTJXC2apo9hwBtO2or6fP4+NdzJ6N JfQJt87keIbOhGVUb4mAoAKvEZYPxO22FajXaJyCKvpJB0jXetkBZhTvgia7s5+o6f33 RfCBQWMq3Ad3dQ+8x6J67zKPYLrueP6POsPBaPIPRO1fDdNs+sl04NsKUre0ZpsDUU7s FDJTmMDDfLVMZbSTeztjbN7oBTh2AtmRLVu6QsPC8Xd9Va9S4sBsqyq1qCrRzke9uJp5 9qtQ== X-Gm-Message-State: AOAM5305P9EuyKc4DHXmeSWnPoiCfGEQDSdzUlI8mxaB7oqidLp8HHJc X2SNnB3ERUHKDdnNmJbZzzk= X-Google-Smtp-Source: ABdhPJyZ5z7YaTSdywVs/b2SYSFsrT2LpRIqN+BikgJ1/HJ4+d1ZA6Eo3s0ysnen+Kfms/ryRi1JNQ== X-Received: by 2002:a62:1b97:0:b029:24e:44e9:a8c1 with SMTP id b145-20020a621b970000b029024e44e9a8c1mr6667980pfb.19.1618550238407; Thu, 15 Apr 2021 22:17:18 -0700 (PDT) Return-Path: Received: from [192.168.50.18] ([50.35.88.161]) by smtp.gmail.com with ESMTPSA id z1sm3001138pgz.94.2021.04.15.22.17.17 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 15 Apr 2021 22:17:17 -0700 (PDT) Subject: Re: [PATCH v2 1/1] UefiCpuPkg: PiSmmCpuDxeSmm: Not to Change Bitwidth During Static Paging To: Laszlo Ersek , devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Rahul Kumar References: <20210414202547.394-1-kuqin12@gmail.com> <20210414202547.394-2-kuqin12@gmail.com> <9a412dd6-e026-8741-35d7-7943eab7be63@redhat.com> From: "Kun Qin" Message-ID: Date: Thu, 15 Apr 2021 22:17:15 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <9a412dd6-e026-8741-35d7-7943eab7be63@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Laszlo, I kept the renames and only replaced the original local variable assignment with input argument. Thanks for your review. Regards, Kun On 04/15/2021 03:41, Laszlo Ersek wrote: > On 04/14/21 22:25, Kun Qin wrote: >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3300 >> >> Current implementation of SetStaticPageTable routine in PiSmmCpuDxeSmm >> driver will check a global variable mPhysicalAddressBits, and eventually >> cap any value larger than 39 at 39. >> >> This global variable is used in ConvertMemoryPageAttributes, which backs >> SmmSetMemoryAttributes and SmmClearMemoryAttributes. Thus for a processor >> that supports more than 39 bits width, trying to mark page table regions >> higher than 39-bit will always return EFI_UNSUPPROTED. >> >> This change updated the interface of SetStaticPageTable function to take >> PhysicalAddressBits as an input parameter, in order to avoid changing/ >> accessing the global variable. >> >> Cc: Eric Dong >> Cc: Ray Ni >> Cc: Laszlo Ersek >> Cc: Rahul Kumar >> >> Fixes: 4eee0cc7cc0db74489b99c19eba056b53eda6358 >> Signed-off-by: Kun Qin >> --- >> >> Notes: >> v2: >> - SetStaticPageTable interface update [Ray] >> - Commit message updates, variable type change [Laszlo] >> >> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30 +++++++++++--------- >> 1 file changed, 16 insertions(+), 14 deletions(-) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c >> index 6902584b1fbd..d6f8dd94d303 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c >> @@ -211,11 +211,13 @@ CalculateMaximumSupportAddress ( >> /** >> Set static page table. >> >> - @param[in] PageTable Address of page table. >> + @param[in] PageTable Address of page table. >> + @param[in] PhysicalAddressBits The maximum physical address bits supported. >> **/ >> VOID >> SetStaticPageTable ( >> - IN UINTN PageTable >> + IN UINTN PageTable, >> + IN UINT8 PhysicalAddressBits >> ) >> { >> UINT64 PageAddress; >> @@ -237,26 +239,26 @@ SetStaticPageTable ( >> // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses >> // when 5-Level Paging is disabled. >> // >> - ASSERT (mPhysicalAddressBits <= 52); >> - if (!m5LevelPagingNeeded && mPhysicalAddressBits > 48) { >> - mPhysicalAddressBits = 48; >> + ASSERT (PhysicalAddressBits <= 52); >> + if (!m5LevelPagingNeeded && PhysicalAddressBits > 48) { >> + PhysicalAddressBits = 48; >> } >> >> NumberOfPml5EntriesNeeded = 1; >> - if (mPhysicalAddressBits > 48) { >> - NumberOfPml5EntriesNeeded = (UINTN) LShiftU64 (1, mPhysicalAddressBits - 48); >> - mPhysicalAddressBits = 48; >> + if (PhysicalAddressBits > 48) { >> + NumberOfPml5EntriesNeeded = (UINTN) LShiftU64 (1, PhysicalAddressBits - 48); >> + PhysicalAddressBits = 48; >> } >> >> NumberOfPml4EntriesNeeded = 1; >> - if (mPhysicalAddressBits > 39) { >> - NumberOfPml4EntriesNeeded = (UINTN) LShiftU64 (1, mPhysicalAddressBits - 39); >> - mPhysicalAddressBits = 39; >> + if (PhysicalAddressBits > 39) { >> + NumberOfPml4EntriesNeeded = (UINTN) LShiftU64 (1, PhysicalAddressBits - 39); >> + PhysicalAddressBits = 39; >> } >> >> NumberOfPdpEntriesNeeded = 1; >> - ASSERT (mPhysicalAddressBits > 30); >> - NumberOfPdpEntriesNeeded = (UINTN) LShiftU64 (1, mPhysicalAddressBits - 30); >> + ASSERT (PhysicalAddressBits > 30); >> + NumberOfPdpEntriesNeeded = (UINTN) LShiftU64 (1, PhysicalAddressBits - 30); >> >> // >> // By architecture only one PageMapLevel4 exists - so lets allocate storage for it. >> @@ -438,7 +440,7 @@ SmmInitPageTable ( >> // When access to non-SMRAM memory is restricted, create page table >> // that covers all memory space. >> // >> - SetStaticPageTable ((UINTN)PTEntry); >> + SetStaticPageTable ((UINTN)PTEntry, mPhysicalAddressBits); >> } else { >> // >> // Add pages to page pool >> > > Last time I checked the variable renames carefully, now I'm trusting > that those have not been broken/modified since v1. > > Reviewed-by: Laszlo Ersek >