From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f170.google.com (mail-pf1-f170.google.com [209.85.210.170]) by mx.groups.io with SMTP id smtpd.web11.716.1623790273214085308 for ; Tue, 15 Jun 2021 13:51:13 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=gscU+em2; spf=pass (domain: gmail.com, ip: 209.85.210.170, mailfrom: kuqin12@gmail.com) Received: by mail-pf1-f170.google.com with SMTP id k6so375733pfk.12 for ; Tue, 15 Jun 2021 13:51:13 -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=EU/NrfLh9TAdjVmyrExXIMOvFqlis7XAi/wLDC0I+FM=; b=gscU+em2K3z7x3fpiyweRlY2dD78gE2uXpCi1n5UOsDTwSe2J2esaN6KCwowficUIk 9T5j+hyV88+PshdB93hGYWMRCu4HaaZERjAZmH5KcdJITeWscubaP0ou7lh43v4kadI3 tZIoqMuhKDFJgnC7Z2Og/DdXK9RyA5r/TtISiu4viZVQvDpk5H5ig3joKoPRL9tSAoiY SZtTDZG+E8mcagHWPSr6coG23imgfRsg3udWkJqHObbdYaPUph9KDjNS6cUrUHV8xnVv wADcZfG6JXNvsEv2Z1dJXYbNqhLKpaBYqEnBUhwIvMYKDBqzQdX2lUzp3yhgZDxOqmPK TpBQ== 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=EU/NrfLh9TAdjVmyrExXIMOvFqlis7XAi/wLDC0I+FM=; b=XxmD2B39+XXRdjMivOVkzpWnpEOyzngjmWo7fJUqx8+hHm4kxUpxsVhDFtgZl7Nd9R RLTYUXN1oIGH7nC1EHkX828dVwqSRyzHa3vWeZHtNybRDalmhNE20DwODoWOxfGDd524 X4QvmsbNBiEQmDBfTgDxP8rF95Bw4E+J1czTAvscVCcYf2bMENe2K59INwVIkjs+6VAY hpqXOL+xjDMQCOBjb1XVUERN9EMIWMTjDS1VXkisv1QVKMVam89anCspNP7NETExMxXf lBiQQdeOnVaHXQhhNiqm7eH9/sXenaJ1hXSLjeAqvd+dLevyu3dXYVegkDthD1F7dvZo 4w7w== X-Gm-Message-State: AOAM533Tg1P9aXC5W2WSsXfGd4+i0e2WMhNQkcfw48I1+tHa8zURV5wg PnQoVUUwOGwmj5Z01RNIQfU= X-Google-Smtp-Source: ABdhPJwGUvbTO1DWySzzTJxyY0lBQYgLTWUf6h/JLZmXVu6jZNNbAzCzeaX+3GRx8JKrUci7ZG3L3w== X-Received: by 2002:a63:5019:: with SMTP id e25mr1348655pgb.280.1623790272755; Tue, 15 Jun 2021 13:51:12 -0700 (PDT) Return-Path: Received: from [192.168.50.18] ([50.35.88.161]) by smtp.gmail.com with ESMTPSA id w71sm58886pfc.164.2021.06.15.13.51.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 15 Jun 2021 13:51:12 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update To: "Wu, Hao A" , "devel@edk2.groups.io" Cc: "Kinney, Michael D" , Liming Gao , "Liu, Zhiguang" , Andrew Fish , Laszlo Ersek , Leif Lindholm References: <20210610014259.1151-1-kuqin12@gmail.com> <20210610014259.1151-2-kuqin12@gmail.com> From: "Kun Qin" Message-ID: Date: Tue, 15 Jun 2021 13:51:11 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Hao, Sorry that I missed comments for this patch earlier. You are correct. I only inspected SmmLockBoxPeiLib. The PEI instance handled mode switch with ```OFFSET_OF ``` function. But DXE instance still have a few use cases that will be impacted. I will update this read me file and add a code change patch for this library in v2. Thanks for pointing this out. Regards, Kun On 06/11/2021 00:46, Wu, Hao A wrote: >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Kun Qin >> Sent: Thursday, June 10, 2021 9:43 AM >> To: devel@edk2.groups.io >> Cc: Kinney, Michael D ; Liming Gao >> ; Liu, Zhiguang ; >> Andrew Fish ; Laszlo Ersek ; Leif >> Lindholm >> Subject: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification: >> EFI_MM_COMMUNICATE_HEADER Update >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430 >> >> This change includes specification update markdown file that describes the >> proposed PI Specification v1.7 Errata A in detail and potential impact to the >> existing codebase. >> >> Cc: Michael D Kinney >> Cc: Liming Gao >> Cc: Zhiguang Liu >> Cc: Andrew Fish >> Cc: Laszlo Ersek >> Cc: Leif Lindholm >> >> Signed-off-by: Kun Qin >> --- >> BZ3430-SpecChange.md | 88 ++++++++++++++++++++ >> 1 file changed, 88 insertions(+) >> >> diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md new file mode >> 100644 index 000000000000..33a1ffda447b >> --- /dev/null >> +++ b/BZ3430-SpecChange.md >> @@ -0,0 +1,88 @@ >> +# Title: Change MessageLength Field of EFI_MM_COMMUNICATE_HEADER >> to >> +UINT64 >> + >> +## Status: Draft >> + >> +## Document: UEFI Platform Initialization Specification Version 1.7 >> +Errata A >> + >> +## License >> + >> +SPDX-License-Identifier: CC-BY-4.0 >> + >> +## Submitter: [TianoCore Community](https://www.tianocore.org) >> + >> +## Summary of the change >> + >> +Change the `MessageLength` Field of `EFI_MM_COMMUNICATE_HEADER` >> from UINTN to UINT64 to remove architecture dependency: >> + >> +```c >> +typedef struct { >> + EFI_GUID HeaderGuid; >> + UINT64 MessageLength; >> + UINT8 Data[ANYSIZE_ARRAY]; >> +} EFI_MM_COMMUNICATE_HEADER; >> +``` >> + >> +## Benefits of the change >> + >> +In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol, the >> MessageLength field of `EFI_MM_COMMUNICATE_HEADER` (also defined as >> `EFI_SMM_COMMUNICATE_HEADER`) is defined as type UINTN. >> + >> +But this structure, as a generic definition, could be used for both PEI and >> DXE MM communication. Thus for a system that supports PEI MM launch, >> but operates PEI in 32bit mode and MM foundation in 64bit, the current >> `EFI_MM_COMMUNICATE_HEADER` definition will cause structure parse >> error due to UINTN used. >> + >> +## Impact of the change >> + >> +This change will impact the known structure consumers including: >> + >> +```bash >> +MdeModulePkg/Core/PiSmmCore/PiSmmIpl >> +MdeModulePkg/Application/SmiHandlerProfileInfo >> +MdeModulePkg/Application/MemoryProfileInfo >> +``` >> + >> +For consumers that are not using >> `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)`, but performing >> explicit addition such as the existing >> MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c, >> one will need to change code implementation to match new structure >> definition. Otherwise, the code compiled on IA32 architecture will >> experience structure field dereference error. >> + >> +User who currently uses UINTN local variables as place holder of >> MessageLength will need to use caution to make cast from UINTN to UINT64 >> and vice versa. It is recommended to use `SafeUint64ToUintn` for such >> operations when the value is indeterministic. >> + >> +Note: MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib is also >> consuming this structure, but it handled this size discrepancy internally. If this > > > Hello Kun, > > Sorry for a question. > I am not sure why the current codes in file SmmLockBoxDxeLib.c will work properly after the UINTN -> UINT64 change. > > For example: > LockBoxGetSmmCommBuffer(): > MinimalSizeNeeded = sizeof (EFI_GUID) + > sizeof (UINTN) + > MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SAVE), > MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES), > MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_UPDATE), > MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE), > sizeof (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE))))); > > SaveLockBox(): > UINT8 TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)]; > > Is the series missed changes for SmmLockBoxDxeLib or I got something wrong? > > Best Regards, > Hao Wu > > >> potential spec change is not applied, all applicable PEI MM communicate >> callers will need to use the same routine as that of SmmLockBoxPeiLib to >> invoke a properly populated EFI_MM_COMMUNICATE_HEADER to be used >> in X64 MM foundation. >> + >> +## Detailed description of the change [normative updates] >> + >> +### Specification Changes >> + >> +1. In PI Specification v1.7 Errata A: Vol. 4 Page-91, the definition of >> `EFI_MM_COMMUNICATE_HEADER` should be changed from current: >> + >> +```c >> +typedef struct { >> + EFI_GUID HeaderGuid; >> + UINTN MessageLength; >> + UINT8 Data[ANYSIZE_ARRAY]; >> +} EFI_MM_COMMUNICATE_HEADER; >> +``` >> + >> +to: >> + >> +```c >> +typedef struct { >> + EFI_GUID HeaderGuid; >> + UINT64 MessageLength; >> + UINT8 Data[ANYSIZE_ARRAY]; >> +} EFI_MM_COMMUNICATE_HEADER; >> +``` >> + >> +### Code Changes >> + >> +1. Remove the explicit calculation of the offset of `Data` in >> `EFI_MM_COMMUNICATE_HEADER`. Thus applicable calculations of >> `sizeof(EFI_GUID) + sizeof(UINTN)` should be replaced with >> `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)` or similar >> alternatives. These calculations are identified in: >> + >> +```bash >> +MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo. >> c >> +MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c >> +``` >> + >> +1. Resolve potentially mismatched type between `UINTN` and `UINT64`. >> This would occur when `MessageLength` or its derivitive are used for local >> calculation with existing `UINTN` typed variables. Code change regarding this >> perspective is per case evaluation: if the variables involved are all >> deterministic values, and there is no overflow or underflow risk, a cast >> operation (from `UINTN` to `UINT64`) can be safely used. Otherwise, the >> calculation will be performed in `UINT64` bitwidth and then convert to >> `UINTN` using `SafeUint64*` and `SafeUint64ToUintn`, respectively. These >> operations are identified in: >> + >> +```bash >> +MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c >> +MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo. >> c >> +MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c >> +``` >> + >> +1. After all above changes applied and specification updated, >> `MdePkg/Include/Protocol/MmCommunication.h` will need to be updated to >> match new definition that includes the field type update. >> -- >> 2.31.1.windows.1 >> >> >> >> >> >