public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Huang, Yanbo" <yanbo.huang@intel.com>
To: Taylor Beebe <taylor.d.beebe@gmail.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Wang, Jian J" <jian.j.wang@intel.com>,
	"Gao, Liming" <gaoliming@byosoft.com.cn>,
	"Bi, Dandan" <dandan.bi@intel.com>,
	"Zhou, Jianfeng" <jianfeng.zhou@intel.com>
Subject: Re: [edk2-devel] MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows
Date: Sun, 14 Apr 2024 14:35:38 +0000	[thread overview]
Message-ID: <IA1PR11MB61706B8E64443E3193095B48EF0A2@IA1PR11MB6170.namprd11.prod.outlook.com> (raw)
In-Reply-To: <45b9b2a8-4bbb-4d67-94a9-6c6d6607feb7@gmail.com>

Hi Taylor,

For your mentioned: "In this case, because the memory type of the buffer is EfiRuntimeServicesCode, shouldn't the final pages be EFI_MEMORY_RO?"

After print the attributes, the attribute are not set to EFI_MEMORY_RO, nearly all of the NewRecord->Attribute are set to 0 in SplitRecord API.

Best Regards,
Yanbo Huang
-----Original Message-----
From: Taylor Beebe <taylor.d.beebe@gmail.com> 
Sent: Friday, April 12, 2024 11:10 PM
To: Huang, Yanbo <yanbo.huang@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>; Zhou, Jianfeng <jianfeng.zhou@intel.com>
Subject: Re: MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows

Hi Yanbo,

Can you help me understand the memory layout which causes this issue?

If a single EfiRuntimeServicesCode descriptor needs to be split because an image is within the memory range. I think that descriptor is split like so in the case you're encountering:

-------------------  ---       ---
|       DATA      |     |        |
-------------------     |        |
|       CODE      |     | Image  |
-------------------     | Memory | EfiRuntimeServicesCode
|       DATA      |     |        |
-------------------  ---         |
|   Extra Pages   |              |
-------------------            ---

In this case, because the memory type of the buffer is EfiRuntimeServicesCode, shouldn't the final pages be EFI_MEMORY_RO?

Thanks!
-Taylor
On 4/11/2024 10:14 PM, Huang, Yanbo wrote:
> Hi Beebe,
>
> Recently we found this commit " MdeModulePkg: Fix MAT SplitRecord() Logic " will cause SUT reset after enable some knobs.
> I filed one Bugzilla for it: 
> https://bugzilla.tianocore.org/show_bug.cgi?id=4751
>
> After debug, we found in SplitRecord API, many entries attribute are set to 0, not align with the UEFI spec:
> "Memory Attributes Table (MAT):
> EFI_MEMORY_ATTRIBUTES_TABLE. The entire UEFI runtime must be described by this table.
> All entries must include attributes EFI_MEMORY_RO, EFI_MEMORY_XP, or both. Memory MUST be either readable and executable OR writeable and non-executable."
> This should be the root cause of this issue.
> When we update "NewRecord->Attribute     = TempRecord.Attribute;" to "NewRecord->Attribute     = TempRecord.Attribute | EFI_MEMORY_XP;", SUT can boot to windows.
>
> @taylor.d.beebe@gmail.com Could you please help to send one formal fix patch for this issue?
> Thanks!
>
> Best Regards,
> Yanbo Huang
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Taylor 
> Beebe
> Sent: Tuesday, November 28, 2023 2:18 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming 
> <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>
> Subject: [edk2-devel] [PATCH v5 10/16] MdeModulePkg: Fix MAT 
> SplitRecord() Logic
>
> SplitRecord() does not handle the case where a memory descriptor describes an image region plus extra pages before or after the image region. This patch fixes this case by carving off the unrelated regions into their own descriptors.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Signed-off-by: Taylor Beebe <taylor.d.beebe@gmail.com>
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> ---
>   MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 56 ++++++++++----------
>   1 file changed, 27 insertions(+), 29 deletions(-)
>
> diff --git 
> a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordL
> ib.c 
> b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordL
> ib.c index 7c0ecd07c1bb..9d4082280bf5 100644
> --- 
> a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordL
> ib.c
> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRec
> +++ or
> +++ dLib.c
> @@ -323,7 +323,6 @@ SplitRecord (
>     UINT64                   PhysicalEnd;
>     UINTN                    NewRecordCount;
>     UINTN                    TotalNewRecordCount;
> -  BOOLEAN                  IsLastRecordData;
>   
>     if (MaxSplitRecordCount == 0) {
>       CopyMem (NewRecord, OldRecord, DescriptorSize); @@ -344,35 +343,16 @@ SplitRecord (
>       NewImageRecord = GetImageRecordByAddress (PhysicalStart, PhysicalEnd - PhysicalStart, ImageRecordList);
>       if (NewImageRecord == NULL) {
>         //
> -      // No more image covered by this range, stop
> +      // No more images cover this range, check if we've reached the end of the old descriptor. If not,
> +      // add the remaining range to the new descriptor list.
>         //
> -      if ((PhysicalEnd > PhysicalStart) && (ImageRecord != NULL)) {
> -        //
> -        // If this is still address in this record, need record.
> -        //
> -        NewRecord        = PREVIOUS_MEMORY_DESCRIPTOR (NewRecord, DescriptorSize);
> -        IsLastRecordData = FALSE;
> -        if ((NewRecord->Attribute & EFI_MEMORY_XP) != 0) {
> -          IsLastRecordData = TRUE;
> -        }
> -
> -        if (IsLastRecordData) {
> -          //
> -          // Last record is DATA, just merge it.
> -          //
> -          NewRecord->NumberOfPages = EfiSizeToPages (PhysicalEnd - NewRecord->PhysicalStart);
> -        } else {
> -          //
> -          // Last record is CODE, create a new DATA entry.
> -          //
> -          NewRecord                = NEXT_MEMORY_DESCRIPTOR (NewRecord, DescriptorSize);
> -          NewRecord->Type          = TempRecord.Type;
> -          NewRecord->PhysicalStart = TempRecord.PhysicalStart;
> -          NewRecord->VirtualStart  = 0;
> -          NewRecord->NumberOfPages = TempRecord.NumberOfPages;
> -          NewRecord->Attribute     = TempRecord.Attribute | EFI_MEMORY_XP;
> -          TotalNewRecordCount++;
> -        }
> +      if (PhysicalEnd > PhysicalStart) {
> +        NewRecord->Type          = TempRecord.Type;
> +        NewRecord->PhysicalStart = PhysicalStart;
> +        NewRecord->VirtualStart  = 0;
> +        NewRecord->NumberOfPages = EfiSizeToPages (PhysicalEnd - PhysicalStart);
> +        NewRecord->Attribute     = TempRecord.Attribute;
> +        TotalNewRecordCount++;
>         }
>   
>         break;
> @@ -380,6 +360,24 @@ SplitRecord (
>   
>       ImageRecord = NewImageRecord;
>   
> +    //
> +    // Update PhysicalStart to exclude the portion before the image buffer
> +    //
> +    if (TempRecord.PhysicalStart < ImageRecord->ImageBase) {
> +      NewRecord->Type          = TempRecord.Type;
> +      NewRecord->PhysicalStart = TempRecord.PhysicalStart;
> +      NewRecord->VirtualStart  = 0;
> +      NewRecord->NumberOfPages = EfiSizeToPages (ImageRecord->ImageBase - TempRecord.PhysicalStart);
> +      NewRecord->Attribute     = TempRecord.Attribute;
> +      TotalNewRecordCount++;
> +
> +      PhysicalStart            = ImageRecord->ImageBase;
> +      TempRecord.PhysicalStart = PhysicalStart;
> +      TempRecord.NumberOfPages = EfiSizeToPages (PhysicalEnd - 
> + PhysicalStart);
> +
> +      NewRecord = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)NewRecord + DescriptorSize);
> +    }
> +
>       //
>       // Set new record
>       //
> --
> 2.42.0.windows.2
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117721): https://edk2.groups.io/g/devel/message/117721
Mute This Topic: https://groups.io/mt/105477564/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2024-04-14 14:35 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-27 18:17 [edk2-devel] [PATCH v5 00/16] Add ImagePropertiesRecordLib and Fix MAT Bugs​ Taylor Beebe
2023-11-27 18:17 ` [edk2-devel] [PATCH v5 01/16] MdeModulePkg: Add ImagePropertiesRecordLib Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 02/16] ArmVirtPkg: Add ImagePropertiesRecordLib Instance Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 03/16] EmulatorPkg: " Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 04/16] OvmfPkg: " Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 05/16] UefiPayloadPkg: " Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 06/16] MdeModulePkg: Update MemoryAttributesTable.c to Reduce Global Variable Use Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 07/16] MdeModulePkg: Move Some DXE MAT Logic to ImagePropertiesRecordLib Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 08/16] MdeModulePkg: Add ImagePropertiesRecordLib Host-Based Unit Test Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 09/16] MdeModulePkg: Fix MAT Descriptor Count Calculation Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 10/16] MdeModulePkg: Fix MAT SplitRecord() Logic Taylor Beebe
2024-04-12  5:14   ` [edk2-devel] MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows Huang, Yanbo
2024-04-12 15:09     ` Taylor Beebe
2024-04-14 14:35       ` Huang, Yanbo [this message]
2024-04-15 10:57         ` Dandan Bi
2024-04-16  1:17           ` Taylor Beebe
2024-04-17  2:32             ` Taylor Beebe
2024-04-17 14:04               ` Huang, Yanbo
2024-04-17 23:53                 ` Taylor Beebe
2024-04-18 13:02                   ` Dandan Bi
2024-04-18 13:17                     ` Ard Biesheuvel
2024-04-18 13:56                       ` Huang, Yanbo
2024-04-18 14:21                         ` Oliver Smith-Denny
2024-04-19  6:43                           ` Ni, Ray
2024-04-23 14:33                             ` Oliver Smith-Denny
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 11/16] MdeModulePkg: Fix MAT SplitTable() Logic Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 12/16] MdeModulePkg: Add NULL checks and Return Status to ImagePropertiesRecordLib Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 13/16] UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 14/16] MdeModulePkg: Transition SMM MAT Logic to Use ImagePropertiesRecordLib Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 15/16] MdeModulePkg: Add Logic to Create/Delete Image Properties Records Taylor Beebe
2023-11-27 18:18 ` [edk2-devel] [PATCH v5 16/16] MdeModulePkg: Update DumpImageRecord() in ImagePropertiesRecordLib Taylor Beebe
2023-11-27 18:40 ` [edk2-devel] [PATCH v5 00/16] Add ImagePropertiesRecordLib and Fix MAT Bugs​ Ard Biesheuvel
2023-11-28 10:22   ` Ni, Ray

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=IA1PR11MB61706B8E64443E3193095B48EF0A2@IA1PR11MB6170.namprd11.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox