* Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib
2023-10-31 9:15 ` Attar, AbdulLateef (Abdul Lateef) via groups.io
@ 2023-10-31 11:44 ` Laszlo Ersek
2023-10-31 11:51 ` Paolo Bonzini
1 sibling, 0 replies; 4+ messages in thread
From: Laszlo Ersek @ 2023-10-31 11:44 UTC (permalink / raw)
To: Attar, AbdulLateef (Abdul Lateef), Lin, Jacque,
devel@edk2.groups.io, Gerd Hoffmann, Paolo Bonzini
Cc: Chang, Abner
Hi,
On 10/31/23 10:15, Attar, AbdulLateef (Abdul Lateef) wrote:
> [Public]
>
> +Laszlo, +Gerd, +Paolo
> PR: https://github.com/tianocore/edk2/pull/4982
... My opinion, stated elsewhere in this thread in detail, is that this
patch is wrong, and should not be merged.
Laszlo
>
> -----Original Message-----
> From: Lin, Jacque <HsienChieh.Lin@amd.com>
> Sent: Tuesday, October 31, 2023 11:07 AM
> To: devel@edk2.groups.io
> Cc: Lin, Jacque <HsienChieh.Lin@amd.com>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; Chang, Abner <Abner.Chang@amd.com>
> Subject: [PATCH v2] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib
>
> Remove checking SMM Rev ID in AMD Save State lib when reading Save State Register EFI_MM_SAVE_STATE_REGISTER_IO.
> For AMD, it is not necessary to check SmmRevId when reading Save State Register EFI_MM_SAVE_STATE_REGISTER_IO.
>
> Cc: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Signed-off-by: Jacque Lin <hsienchieh.lin@amd.com>
> ---
> UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c | 13 -------------
> 1 file changed, 13 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c b/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> index 3315a6cc44..c4bf6ad4bb 100644
> --- a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> +++ b/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> @@ -102,7 +102,6 @@ MmSaveStateReadRegister (
> OUT VOID *Buffer ) {- UINT32 SmmRevId; EFI_MM_SAVE_STATE_IO_INFO *IoInfo; AMD_SMRAM_SAVE_STATE_MAP *CpuSaveState; UINT8 DataWidth;@@ -124,18 +123,6 @@ MmSaveStateReadRegister (
> // Check for special EFI_MM_SAVE_STATE_REGISTER_IO if (Register == EFI_MM_SAVE_STATE_REGISTER_IO) {- //- // Get SMM Revision ID- //- MmSaveStateReadRegisterByIndex (CpuIndex, AMD_MM_SAVE_STATE_REGISTER_SMMREVID_INDEX, sizeof (SmmRevId), &SmmRevId);-- //- // See if the CPU supports the IOMisc register in the save state- //- if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) {- return EFI_NOT_FOUND;- }- // Check if IO Restart Dword [IO Trap] is valid or not using bit 1. if (!(CpuSaveState->x64.IO_DWord & 0x02u)) { return EFI_NOT_FOUND;--
> 2.36.1.windows.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110401): https://edk2.groups.io/g/devel/message/110401
Mute This Topic: https://groups.io/mt/102292190/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib
2023-10-31 9:15 ` Attar, AbdulLateef (Abdul Lateef) via groups.io
2023-10-31 11:44 ` Laszlo Ersek
@ 2023-10-31 11:51 ` Paolo Bonzini
1 sibling, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2023-10-31 11:51 UTC (permalink / raw)
To: Attar, AbdulLateef (Abdul Lateef)
Cc: Lin, Jacque, devel@edk2.groups.io, Laszlo Ersek, Gerd Hoffmann,
Chang, Abner
On Tue, Oct 31, 2023 at 10:16 AM Attar, AbdulLateef (Abdul Lateef)
<AbdulLateef.Attar@amd.com> wrote:
>
> [Public]
>
> +Laszlo, +Gerd, +Paolo
> PR: https://github.com/tianocore/edk2/pull/4982
I left a comment in the PR.
The patch is only correct if this code is only ever used on 64-bit
processors. Note that KVM uses the legacy 32-bit format of the SMRAM
state save area, if the virtual machine is configured to clear the LM
bit of CPUID.
Second, the commit message does not explain why you are doing this.
Without such an explanation, it is impossible to provide more
constructive feedback.
Paolo
> -----Original Message-----
> From: Lin, Jacque <HsienChieh.Lin@amd.com>
> Sent: Tuesday, October 31, 2023 11:07 AM
> To: devel@edk2.groups.io
> Cc: Lin, Jacque <HsienChieh.Lin@amd.com>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; Chang, Abner <Abner.Chang@amd.com>
> Subject: [PATCH v2] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib
>
> Remove checking SMM Rev ID in AMD Save State lib when reading Save State Register EFI_MM_SAVE_STATE_REGISTER_IO.
> For AMD, it is not necessary to check SmmRevId when reading Save State Register EFI_MM_SAVE_STATE_REGISTER_IO.
>
> Cc: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Signed-off-by: Jacque Lin <hsienchieh.lin@amd.com>
> ---
> UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c | 13 -------------
> 1 file changed, 13 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c b/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> index 3315a6cc44..c4bf6ad4bb 100644
> --- a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> +++ b/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
> @@ -102,7 +102,6 @@ MmSaveStateReadRegister (
> OUT VOID *Buffer ) {- UINT32 SmmRevId; EFI_MM_SAVE_STATE_IO_INFO *IoInfo; AMD_SMRAM_SAVE_STATE_MAP *CpuSaveState; UINT8 DataWidth;@@ -124,18 +123,6 @@ MmSaveStateReadRegister (
> // Check for special EFI_MM_SAVE_STATE_REGISTER_IO if (Register == EFI_MM_SAVE_STATE_REGISTER_IO) {- //- // Get SMM Revision ID- //- MmSaveStateReadRegisterByIndex (CpuIndex, AMD_MM_SAVE_STATE_REGISTER_SMMREVID_INDEX, sizeof (SmmRevId), &SmmRevId);-- //- // See if the CPU supports the IOMisc register in the save state- //- if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) {- return EFI_NOT_FOUND;- }- // Check if IO Restart Dword [IO Trap] is valid or not using bit 1. if (!(CpuSaveState->x64.IO_DWord & 0x02u)) { return EFI_NOT_FOUND;--
> 2.36.1.windows.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110402): https://edk2.groups.io/g/devel/message/110402
Mute This Topic: https://groups.io/mt/102292190/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 4+ messages in thread