From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail04.groups.io (mail04.groups.io [45.79.224.9]) by spool.mail.gandi.net (Postfix) with ESMTPS id 01630740034 for ; Tue, 16 Apr 2024 01:17:45 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=2ZuLo8tOQP0aZaify/TnmYHqZQUBuyOUeh4wgrOI2QQ=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Language; s=20240206; t=1713230264; v=1; b=4YG8h3oEM4CFKZ5GxqeZ2IWgV7DZ4oQgAxUGzuOmkHT0f+mFTuzSt1IT/42eSIF+zBp0N7NH JtfId59uXE8lWUAZM4J+FFE6LPCsjT1I39rjL/TBBaArWROCVRsdyoPuH4eKfeSXtB4+BYxt+kY W45wP+X+KJfnPzojgLXZEI57CeIH5mk7TyA7rSmWpuN/qki0upAQpzTwUT+5jzJfjYZb5Co9uld UmHW/a1aDSLeTsCEwBqCcbZyrjAlAyMPc+CKPsdEosKBSd2KYnVt2JVRyCFEVKa3lIJMOv76Y7C HMiYJCiGxfZmtELG1nqShUgbkyfocVzioualtMqF+Nu1w== X-Received: by 127.0.0.2 with SMTP id RgIYYY7687511xZN5pUXoHOB; Mon, 15 Apr 2024 18:17:44 -0700 X-Received: from mail-pl1-f180.google.com (mail-pl1-f180.google.com [209.85.214.180]) by mx.groups.io with SMTP id smtpd.web11.9394.1713230263930713298 for ; Mon, 15 Apr 2024 18:17:43 -0700 X-Received: by mail-pl1-f180.google.com with SMTP id d9443c01a7336-1e51398cc4eso36893325ad.2 for ; Mon, 15 Apr 2024 18:17:43 -0700 (PDT) X-Forwarded-Encrypted: i=1; AJvYcCW1mf6KbJ/ilUsa3+U/Nkak9yXK/BRJZ0SaFsT7PfQP9LDVwbNiy8GUm5W7dkMvwIzhNLrp3RC0/xQdV4AT4WYwarLoFw== X-Gm-Message-State: HyjHWXaDl2GqoDb3HzSDcksix7686176AA= X-Google-Smtp-Source: AGHT+IGtGanuz4jLdlFtGs3CPOaSUYPTDBBT7qdqnf1QCu7uEnZhcjJ6rXqjCMNE9iTKu2AHmvuY9A== X-Received: by 2002:a17:903:482:b0:1e7:d482:9d96 with SMTP id jj2-20020a170903048200b001e7d4829d96mr393847plb.10.1713230263233; Mon, 15 Apr 2024 18:17:43 -0700 (PDT) X-Received: from [10.0.4.35] ([4.155.48.121]) by smtp.gmail.com with ESMTPSA id lg7-20020a170902fb8700b001e259719a5fsm8567252plb.103.2024.04.15.18.17.42 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 15 Apr 2024 18:17:42 -0700 (PDT) Message-ID: Date: Mon, 15 Apr 2024 18:17:42 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows To: "Bi, Dandan" , "Huang, Yanbo" , "devel@edk2.groups.io" Cc: "Wang, Jian J" , "Gao, Liming" , "Zhou, Jianfeng" References: <20231127181818.411-1-taylor.d.beebe@gmail.com> <20231127181818.411-11-taylor.d.beebe@gmail.com> <45b9b2a8-4bbb-4d67-94a9-6c6d6607feb7@gmail.com> From: "Taylor Beebe" In-Reply-To: Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Mon, 15 Apr 2024 18:17:44 -0700 Resent-From: taylor.d.beebe@gmail.com Reply-To: devel@edk2.groups.io,taylor.d.beebe@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="------------DuQJ1qI20N0IDRxX4XNLMkNR" Content-Language: en-CA X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=4YG8h3oE; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.9 as permitted sender) smtp.mailfrom=bounce@groups.io --------------DuQJ1qI20N0IDRxX4XNLMkNR Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable On 4/15/2024 3:57 AM, Bi, Dandan wrote: > Hi Taylor, > > With this patch, MAT contains some entries with Attribute - 0x80000000000= 00000, doesn't have EFI_MEMORY_RO or EFI_MEMORY_XP. > After revert this patch, don't see such entries in MAT. > > a. MAT with this patch: > Entry (0x609E4268) > Type - 0x5 > PhysicalStart - 0x00000000769CF000 > VirtualStart - 0x0000000000000000 > NumberOfPages - 0x0000000000000016 > Attribute - 0x8000000000000000 > Entry (0x609E4298) > Type - 0x5 > PhysicalStart - 0x00000000769E5000 > VirtualStart - 0x0000000000000000 > NumberOfPages - 0x0000000000000001 > Attribute - 0x8000000000004000 > Entry (0x609E42C8) > Type - 0x5 > PhysicalStart - 0x00000000769E6000 > VirtualStart - 0x0000000000000000 > NumberOfPages - 0x0000000000000002 > Attribute - 0x8000000000020000 > > b. MAT without this patch: > Entry (0x609E4268) > Type - 0x5 > PhysicalStart - 0x00000000769CF000 > VirtualStart - 0x0000000000000000 > NumberOfPages - 0x0000000000000017 > Attribute - 0x8000000000004000 > Entry (0x609E4298) > Type - 0x5 > PhysicalStart - 0x00000000769E6000 > VirtualStart - 0x0000000000000000 > NumberOfPages - 0x0000000000000002 > Attribute - 0x8000000000020000 > > 1. For example, when OldRecord in old memory map with: > Type - 0x00000005 > Attribute - 0x800000000000000F > PhysicalStart - 0x769CF000 > PhysicalStart is smaller than ImageBase 0x769E5000, with this patch,= it will create a new memory descriptor entry for range 0x769CF000~0x769E50= 00 and without EFI_MEMORY_RO or EFI_MEMORY_XP Attribute. > Then it will only contain EFI_MEMORY_RUNTIME Attribute in MAT as doi= ng MemoryAttributesEntry->Attribute &=3D (EFI_MEMORY_RO|EFI_MEMORY_XP|EFI_= MEMORY_RUNTIME); when install MAT. > It seems not aligned with UEFI Spec " The only valid bits for Attrib= ute field currently are EFI_MEMORY_RO ,EFI_MEMORY_XP , plus EFI_MEMORY_RUNT= IME "? > Could you please help double check? Thanks. Agreed that this is currently the behaviour and that the range should=20 have a restrictive access attribute. More below. > 2. In function SetNewRecord, it semes already cover the DATA entry before= the CODE and the DATA entry after the CODE. > And old SplitRecord function without this patch, also has the entry = to cover the reaming range of this record if no more image covered by this = range. > Why do we still need this patch? Could you please help explain? Than= ks. GetMemoryMap() will merge adjacent descriptors which have the same type=20 and attributes. This means that a single EfiRuntimeServicesCode=20 descriptor within the memory map returned by CoreGetMemoryMap() could=20 describe memory with the following layout (NOTE: this layout is odd but=20 needs to be handled to fulfill the SplitTable() contract): ------------------------- Some EfiRuntimeServicesCode memory ------------------------- Runtime Image DATA Section ------------------------- Runtime Image CODE Section ------------------------- Runtime Image DATA Section ------------------------- Some EfiRuntimeServicesCode memory ------------------------- In this possible layout, the pre-patch logic would assume that the=20 regions before and after the image were part of the image's data=20 sections and would mark them as EFI_MEMORY_XP. The post-patch logic does=20 not mark them with any access attributes which is fine but the DXE MAT=20 logic needs to walk the memory map returned by SplitTable() to add=20 access attributes to runtime regions which don't have any. In your example, because PhysicalStart < ImageBase and=20 0769CF000-0x769E5000 is EfiRuntimeServicesCode, the access attribute=20 should technically be EFI_MEMORY_RO. It's likely that=20 0769CF000-0x769E5000is just the unused memory bucket and so it might be=20 best to mark it both EFI_MEMORY_RO and EFI_MEMORY_XP. *An open question to the community:* Are there cases where runtime=20 executable code is in a buffer separate from a loaded runtime image? Can=20 the EfiRuntimeServicesCode memory regions not part of an image be=20 specified in the MAT as both EFI_MEMORY_RO and EFI_MEMORY_XP, or even=20 dropped entirely? Thanks :) -Taylor -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117813): https://edk2.groups.io/g/devel/message/117813 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --------------DuQJ1qI20N0IDRxX4XNLMkNR Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On 4/15/2024 3:57 AM, Bi, Dandan wrote:<= br> 
Hi Taylor,

With this patch, MAT contains some entries with Attribute - 0x8000000000000=
000, doesn't have EFI_MEMORY_RO or EFI_MEMORY_XP.
After revert this patch, don't see such entries in MAT.

a. MAT with this patch:
Entry (0x609E4268)
  Type              - 0x5
  PhysicalStart     - 0x00000000769CF000
  VirtualStart      - 0x0000000000000000
  NumberOfPages     - 0x0000000000000016
  Attribute         - 0x8000000000000000
Entry (0x609E4298)
  Type              - 0x5
  PhysicalStart     - 0x00000000769E5000
  VirtualStart      - 0x0000000000000000
  NumberOfPages     - 0x0000000000000001
  Attribute         - 0x8000000000004000
Entry (0x609E42C8)
  Type              - 0x5
  PhysicalStart     - 0x00000000769E6000
  VirtualStart      - 0x0000000000000000
  NumberOfPages     - 0x0000000000000002
  Attribute         - 0x8000000000020000

b. MAT without this patch:
Entry (0x609E4268)
  Type              - 0x5
  PhysicalStart     - 0x00000000769CF000
  VirtualStart      - 0x0000000000000000
  NumberOfPages     - 0x0000000000000017
  Attribute         - 0x8000000000004000
Entry (0x609E4298)
  Type              - 0x5
  PhysicalStart     - 0x00000000769E6000
  VirtualStart      - 0x0000000000000000
  NumberOfPages     - 0x0000000000000002
  Attribute         - 0x8000000000020000

1. For example, when OldRecord in old memory map with:
        Type - 0x00000005
        Attribute - 0x800000000000000F
        PhysicalStart - 0x769CF000
    PhysicalStart is smaller than ImageBase 0x769E5000, with this patch, it=
 will create a new memory descriptor entry for range 0x769CF000~0x769E5000 =
and without EFI_MEMORY_RO or EFI_MEMORY_XP Attribute.
    Then it will only contain EFI_MEMORY_RUNTIME Attribute in MAT as doing =
 MemoryAttributesEntry->Attribute &=3D (EFI_MEMORY_RO|EFI_MEMORY_XP|=
EFI_MEMORY_RUNTIME); when install MAT.
    It seems not aligned with UEFI Spec " The only valid bits for Attribute=
 field currently are EFI_MEMORY_RO ,EFI_MEMORY_XP , plus EFI_MEMORY_RUNTIME=
 "?
    Could you please help double check? Thanks.
Agreed that this is currently the behaviour and that the range should have a restrictive access attribute. More below. 
2. In function SetNewRecord, i=
t semes already cover the DATA entry before the CODE and the DATA entry aft=
er the CODE.
    And old SplitRecord function without this patch, also has the entry to =
cover the reaming range of this record if no more image covered by this ran=
ge.
    Why do we still need this patch? Could you please help explain? Thanks.=

GetMemoryMap() will merge adjacent descriptors which have the same type and attributes. This means that a single EfiRuntimeServicesCode descriptor within the memory map returned by CoreGetMemoryMap() could describe memory with the following layout (NOTE: this layout is odd but needs to be handled to fulfill the SplitTable() contract):

-------------------------
Some EfiRuntimeServicesCode memory
-------------------------
Runtime Image DATA Section
-------------------------
Runtime Image CODE Section
-------------------------
Runtime Image DATA Section
-------------------------
Some EfiRuntimeServicesCode memory
-------------------------

In this possible layout, the pre-patch logic would assume that the regions before and after the image were part of the image's data sections and would mark them as EFI_MEMORY_XP. The post-patch logic does not mark them with any access attributes which is fine but the DXE MAT logic needs to walk the memory map returned by SplitTable() to add access attributes to runtime regions which don't have any.

In your example, because Physi= calStart < ImageBase and 0769CF000-0x769E5000 is EfiRuntimeServicesCode,= the access attribute should technically be EFI_MEMORY_RO. It's likely that= 0769CF000-0x769E5000 is just the unused memory bucket and= so it might be best to mark it both EFI_MEMORY_RO and EFI_MEMORY_XP.

An open question to the com= munity: Are there cases where runtime executable code is in a buffer se= parate from a loaded runtime image? Can the EfiRuntimeServicesCode memory regions= not part of an image be specified in the MAT as both EFI_MEMORY_RO and EFI_MEMORY_XP, or e= ven dropped entirely?

Thanks :)
-Taylor
_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#117813) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------DuQJ1qI20N0IDRxX4XNLMkNR--