From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail03.groups.io (mail03.groups.io [45.79.227.220]) by spool.mail.gandi.net (Postfix) with ESMTPS id 9A8FCAC140C for ; Fri, 12 Apr 2024 15:09:46 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=hvdRSeovi+QxIYN5b/VIiGnzmtUKanQWLhkQ3VS/kjo=; 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-Language:Content-Type:Content-Transfer-Encoding; s=20240206; t=1712934585; v=1; b=r/qzHu5p4iibRmZczmBUbWvuevA8z1Emo219xFPYS8DQbjg6Nr/hmDaq0FfzmP3Z5H0U2LVh 34fLMkrfesZqHuLSgdA402g13Ams9vO9rPiSPIOLQl/Pc+Qppsur+Uqpt7XQsCKO2B7eNrnER5P Q2DF/45AgzXXRVmdGQY4V6MGmEq91jcn1T47vaf9jggoS1EDN1siy7Uf/J3myvfVdITVMJEX62K tqd/sbGDZH86UUz+8bZ7zMayQLk3E2jaH0Mt5uaClSZabuvqJiRb1FvymeJ0MpS99vkscEJ1fYA PurK0oh/um1S95+gnQKs0t8umISOQFzk5B5A0W+b+up+w== X-Received: by 127.0.0.2 with SMTP id V39UYY7687511xnjcWHjkhGW; Fri, 12 Apr 2024 08:09:45 -0700 X-Received: from mail-pf1-f169.google.com (mail-pf1-f169.google.com [209.85.210.169]) by mx.groups.io with SMTP id smtpd.web10.49495.1712934584521951011 for ; Fri, 12 Apr 2024 08:09:44 -0700 X-Received: by mail-pf1-f169.google.com with SMTP id d2e1a72fcca58-6ee13f19e7eso846769b3a.1 for ; Fri, 12 Apr 2024 08:09:44 -0700 (PDT) X-Forwarded-Encrypted: i=1; AJvYcCWl3W+kvd2UQLZ+s/d8yheEJLC21++5g9Cu/wONUilIWp82hqqsM5U0HWSpGFrYZZxqQJZiD2wgCln5dzep8QT46nE9YA== X-Gm-Message-State: 7U8kRW3BEWuaTlNfuLVqxUusx7686176AA= X-Google-Smtp-Source: AGHT+IErBwk7wDUbSC0fmEUkYpB8ZY9zPs9fgzYe55jrj9OQd4U7WuSiyZDL46eaGvyOxT5TLnPhEA== X-Received: by 2002:a05:6a20:734e:b0:1a9:9cce:bffb with SMTP id v14-20020a056a20734e00b001a99ccebffbmr3382749pzc.52.1712934583736; Fri, 12 Apr 2024 08:09:43 -0700 (PDT) X-Received: from [10.0.4.35] ([4.155.48.113]) by smtp.gmail.com with ESMTPSA id j16-20020a62b610000000b006ecceed26bfsm2962663pff.219.2024.04.12.08.09.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 12 Apr 2024 08:09:43 -0700 (PDT) Message-ID: <45b9b2a8-4bbb-4d67-94a9-6c6d6607feb7@gmail.com> Date: Fri, 12 Apr 2024 08:09: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: "Huang, Yanbo" , "devel@edk2.groups.io" Cc: "Wang, Jian J" , "Gao, Liming" , "Bi, Dandan" , "Zhou, Jianfeng" References: <20231127181818.411-1-taylor.d.beebe@gmail.com> <20231127181818.411-11-taylor.d.beebe@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: Fri, 12 Apr 2024 08:09: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-Language: en-CA Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b="r/qzHu5p"; 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.227.220 as permitted sender) smtp.mailfrom=bounce@groups.io 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 On Behalf Of Taylor Beebe > Sent: Tuesday, November 28, 2023 2:18 AM > To: devel@edk2.groups.io > Cc: Wang, Jian J ; Gao, Liming ; Bi, Dandan > 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 > Cc: Liming Gao > Cc: Dandan Bi > Signed-off-by: Taylor Beebe > Reviewed-by: Liming Gao > --- > MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 56 ++++++++++---------- > 1 file changed, 27 insertions(+), 29 deletions(-) > > diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c > index 7c0ecd07c1bb..9d4082280bf5 100644 > --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c > +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecor > +++ 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 (#117710): https://edk2.groups.io/g/devel/message/117710 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] -=-=-=-=-=-=-=-=-=-=-=-