From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f171.google.com (mail-pg1-f171.google.com [209.85.215.171]) by mx.groups.io with SMTP id smtpd.web09.463.1623446990110265984 for ; Fri, 11 Jun 2021 14:29:50 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=ML3PevEl; spf=pass (domain: gmail.com, ip: 209.85.215.171, mailfrom: kuqin12@gmail.com) Received: by mail-pg1-f171.google.com with SMTP id l184so3459990pgd.8 for ; Fri, 11 Jun 2021 14:29:50 -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=uc5PtjpDReM/xQjXbTNfEQjOCcEXLkBICmeThySeG9E=; b=ML3PevElaDNPckQAlXAT5gRwCxynRAnp1EUMrPjltFLvY5auBgmJX7k3MJCQQMfDnw lBk/iNofRrI74JyZ4JFMwrAmdDHAdcFGaTmbDWhs0+OwQ/s6obZ732znOPB32cdAuMln Wh2c+S0dvT4mefBxlDwTgcRRLiCOZqe0CFmrL1sD8XiLxRw9X4gCrBJQ4HAnmIGhvqWK fEpFL9d1nIhPyUxYFHBlh20/3iunASAYZzba5E+1gvmJTgbjvWxK66ZdvbPfpbOkNIpF C0lyiVKpOMhhDQ7k4x4QC31THo6URqmFHg9C6JjxJia+mOqYnNN83d4e3VoP76XQFcKn VK3A== 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=uc5PtjpDReM/xQjXbTNfEQjOCcEXLkBICmeThySeG9E=; b=C4RSo3+VOmzOM2Iku+0wcpFIgspiH6YxGN6tdfBJbn1PHmaj/KkYhxwMD3GUpRVLNC 7adbswe7fyaEgRc/mZTycz99rfdp3wQRE/ZWN8iN2l40EVEkt0naISo0fNYfbY9ct1Ns 6xGDzp4Ix+rsYJHaBjklEhulKIlf2CfJtRj0+VYve0y+KShzrytLXzmgg5g7m0tnRVE+ PRKTbMwj4KH3GF4K8l9r50FosTF31D82mKY3Y9aFdNKMa6cTGnJwecsZe/rLcy194TCJ qWcoC9+ToyHA2bH3YaNaqTZsGGjAbm6Z8Qc+zkmwEyP3oIY6Q5OAiIAPnrvztGjejTh/ 6ktg== X-Gm-Message-State: AOAM5300kqoaRHmBoIGee2ASFV2in+P71bRfP2lwhkQHC/4w7VKsSbL1 IK1UeVnazOOPBsco4X/68NE= X-Google-Smtp-Source: ABdhPJxgmfssF52BzTz/MdEHgj99o+yEevrvNghp9J4W+uyIS5tNg5RnK0h5mi9JsmYResxHCzONxA== X-Received: by 2002:aa7:8159:0:b029:2c5:dfd8:3ac4 with SMTP id d25-20020aa781590000b02902c5dfd83ac4mr10099006pfn.16.1623446989709; Fri, 11 Jun 2021 14:29:49 -0700 (PDT) Return-Path: Received: from [192.168.50.18] ([50.35.88.161]) by smtp.gmail.com with ESMTPSA id e2sm7244202pjc.37.2021.06.11.14.29.48 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 11 Jun 2021 14:29:49 -0700 (PDT) Subject: Re: [PATCH v1 3/5] MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation To: "Wu, Hao A" , "devel@edk2.groups.io" Cc: "Wang, Jian J" References: <20210610014259.1151-1-kuqin12@gmail.com> <20210610014259.1151-4-kuqin12@gmail.com> From: "Kun Qin" Message-ID: Date: Fri, 11 Jun 2021 14:29:48 -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, Thanks for pointing out the missing place. Will update this accordingly. This patch series needs a PI spec update, I thought I should mark all changes with BZ#### before the spec update is taken. But I can drop them for the next patch version. Regards, Kun On 06/11/2021 00:46, Wu, Hao A wrote: >> -----Original Message----- >> From: Kun Qin >> Sent: Thursday, June 10, 2021 9:43 AM >> To: devel@edk2.groups.io >> Cc: Wang, Jian J ; Wu, Hao A >> Subject: [PATCH v1 3/5] MdeModulePkg: MemoryProfileInfo: Updated >> MessageLength calculation >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398 >> >> This change replaced the calculation of communication buffer size from >> explicitly adding the size of each member with the OFFSET macro function. >> This will make the structure field defition change transparent to consumers. > > > I think there is one missing place in function GetSmramProfileData(): > > MinimalSizeNeeded = sizeof (EFI_GUID) + > sizeof (UINTN) + > MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_INFO), > MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET), > sizeof (SMRAM_PROFILE_PARAMETER_RECORDING_STATE))); > > More inline comments below: > > >> >> Cc: Jian J Wang >> Cc: Hao A Wu >> >> Signed-off-by: Kun Qin >> --- >> MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20 >> +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git >> a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c >> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c >> index 191c31068545..39ed8b2e0484 100644 >> --- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c >> +++ >> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c >> @@ -1190,7 +1190,9 @@ GetSmramProfileData ( >> CommRecordingState->Header.ReturnStatus = (UINT64)-1; >> CommRecordingState->RecordingState = >> MEMORY_PROFILE_RECORDING_DISABLE; >> >> - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader- >>> MessageLength; >> + // BZ3398: Make MessageLength the same size in >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64. >> + // The CommHeader->MessageLength contains a definitive value, thus >> UINTN cast is safe here. > > > Please help to drop the explicit mention of BZ3398 in the comment. > How about using: > // > // The CommHeader->MessageLength contains a definitive value, thus UINTN cast is safe here. > // > > There are 4 more similar cases below. > > Best Regards, > Hao Wu > > >> + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + >> + (UINTN)CommHeader->MessageLength; >> Status = SmmCommunication->Communicate (SmmCommunication, >> CommBuffer, &CommSize); >> if (EFI_ERROR (Status)) { >> DEBUG ((EFI_D_ERROR, "SmramProfile: SmmCommunication - %r\n", >> Status)); @@ -1213,7 +1215,9 @@ GetSmramProfileData ( >> CommRecordingState->Header.ReturnStatus = (UINT64)-1; >> CommRecordingState->RecordingState = >> MEMORY_PROFILE_RECORDING_DISABLE; >> >> - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader- >>> MessageLength; >> + // BZ3398: Make MessageLength the same size in >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64. >> + // The CommHeader->MessageLength contains a definitive value, thus >> UINTN cast is safe here. >> + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + >> + (UINTN)CommHeader->MessageLength; >> SmmCommunication->Communicate (SmmCommunication, CommBuffer, >> &CommSize); >> } >> >> @@ -1230,7 +1234,9 @@ GetSmramProfileData ( >> CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1; >> CommGetProfileInfo->ProfileSize = 0; >> >> - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader- >>> MessageLength; >> + // BZ3398: Make MessageLength the same size in >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64. >> + // The CommHeader->MessageLength contains a definitive value, thus >> UINTN cast is safe here. >> + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + >> + (UINTN)CommHeader->MessageLength; >> Status = SmmCommunication->Communicate (SmmCommunication, >> CommBuffer, &CommSize); >> ASSERT_EFI_ERROR (Status); >> >> @@ -1261,7 +1267,9 @@ GetSmramProfileData ( >> CommGetProfileData->Header.DataLength = sizeof >> (*CommGetProfileData); >> CommGetProfileData->Header.ReturnStatus = (UINT64)-1; >> >> - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader- >>> MessageLength; >> + // BZ3398: Make MessageLength the same size in >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64. >> + // The CommHeader->MessageLength contains a definitive value, thus >> UINTN cast is safe here. >> + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + >> + (UINTN)CommHeader->MessageLength; >> Buffer = (UINT8 *) CommHeader + CommSize; >> Size -= CommSize; >> >> @@ -1320,7 +1328,9 @@ GetSmramProfileData ( >> CommRecordingState->Header.ReturnStatus = (UINT64)-1; >> CommRecordingState->RecordingState = >> MEMORY_PROFILE_RECORDING_ENABLE; >> >> - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader- >>> MessageLength; >> + // BZ3398: Make MessageLength the same size in >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64. >> + // The CommHeader->MessageLength contains a definitive value, thus >> UINTN cast is safe here. >> + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + >> + (UINTN)CommHeader->MessageLength; >> SmmCommunication->Communicate (SmmCommunication, CommBuffer, >> &CommSize); >> } >> >> -- >> 2.31.1.windows.1 >