From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id D37B3941AC5 for ; Tue, 27 Feb 2024 16:02:17 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=WlgRheyDCCGNGGOIEl0DwdPWyGPCPbcWQey9bNNJaZs=; 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:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1709049736; v=1; b=dWe3sONLO0jHXEK60Okm5k32Z7oXCnO1j+3Zn8PSYlTafbUpB1DR0ZdA6adzsB1eR2jsfBG1 4PWq8DHEuSgT7QyubiX+zEffpzbXq9ZGMi5cEHndGkmaMcVXQBm96RM4l2+8SoTeWN8U3VXZz2Q oBBSfeikKEq5QBNz83MQcEjU= X-Received: by 127.0.0.2 with SMTP id bRWbYY7687511xAXwAwaFEKt; Tue, 27 Feb 2024 08:02:16 -0800 X-Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.15811.1709049735922122906 for ; Tue, 27 Feb 2024 08:02:16 -0800 X-Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id F35AADA7; Tue, 27 Feb 2024 08:02:53 -0800 (PST) X-Received: from [192.168.1.13] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 59AC83F762; Tue, 27 Feb 2024 08:02:14 -0800 (PST) Message-ID: <00278407-b2b9-4acd-b18f-ab21429acc20@arm.com> Date: Tue, 27 Feb 2024 17:02:06 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v1 3/3] DynamicTablesPkg: Adds ACPI WSMT Table generator To: Abdul Lateef Attar , devel@edk2.groups.io Cc: Abdul Lateef Attar , Sami Mujawar References: <5048bd919b4751e9eae2ca15e8347e6fcecf3196.1708411357.git.AbdulLateef.Attar@amd.com> From: "PierreGondois" In-Reply-To: <5048bd919b4751e9eae2ca15e8347e6fcecf3196.1708411357.git.AbdulLateef.Attar@amd.com> 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 Reply-To: devel@edk2.groups.io,pierre.gondois@arm.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 0fFoQC8Ta074lEfe0vwzl5hCx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=dWe3sONL; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=arm.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Hello Abdul, I reviewed this patch first, so comments here may apply to other patches. --- AcpiView (ShellPkg/Library/UefiShellAcpiViewCommandLib) doesn't seem to be able to parse this ACPI table. Maybe a parser could also be added along with the table generation. On 2/20/24 07:48, Abdul Lateef Attar wrote: > From: Abdul Lateef Attar >=20 > Adds generic ACPI WSMT table generator library. > Register/Deregister WSMT table. > Update the WSMT table during boot as per specification. >=20 > Cc: Sami Mujawar > Cc: Pierre Gondois > Signed-off-by: Abdul Lateef Attar > --- > DynamicTablesPkg/DynamicTables.dsc.inc | 2 + > DynamicTablesPkg/Include/AcpiTableGenerator.h | 1 + > .../Include/ArchNameSpaceObjects.h | 10 + > .../Library/Acpi/AcpiWsmtLib/AcpiWsmtLib.inf | 34 +++ > .../Library/Acpi/AcpiWsmtLib/WsmtGenerator.c | 221 ++++++++++++++++++ > 5 files changed, 268 insertions(+) > create mode 100644 DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/AcpiWsmtLi= b.inf > create mode 100644 DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/WsmtGenera= tor.c >=20 > diff --git a/DynamicTablesPkg/DynamicTables.dsc.inc b/DynamicTablesPkg/Dy= namicTables.dsc.inc > index af70785520..5e66f5cd02 100644 > --- a/DynamicTablesPkg/DynamicTables.dsc.inc > +++ b/DynamicTablesPkg/DynamicTables.dsc.inc > @@ -36,6 +36,7 @@ > # > DynamicTablesPkg/Library/Acpi/AcpiFadtLib/AcpiFadtLib.inf > DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf > + DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/AcpiWsmtLib.inf > =20 > # > # Dynamic Table Factory Dxe > @@ -44,6 +45,7 @@ > > NULL|DynamicTablesPkg/Library/Acpi/AcpiFadtLib/AcpiFadtLib.inf > NULL|DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf > + NULL|DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/AcpiWsmtLib.inf The table should be available for all architectures right ? This should not be in a [Components] section instead of a more limiting [Components.IA32, Components.X64] section I assume. > } > =20 > [Components.ARM, Components.AARCH64] > diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h b/DynamicTable= sPkg/Include/AcpiTableGenerator.h > index 18b5f99f47..a32ef46ecb 100644 > --- a/DynamicTablesPkg/Include/AcpiTableGenerator.h > +++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h > @@ -100,6 +100,7 @@ typedef enum StdAcpiTableId { > EStdAcpiTableIdSsdtPciExpress, ///< SSDT Pci Express G= enerator > EStdAcpiTableIdPcct, ///< PCCT Generator > EStdAcpiTableIdHpet, ///< HPET Generator > + EStdAcpiTableIdWsmt, ///< WSMT Generator > EStdAcpiTableIdMax > } ESTD_ACPI_TABLE_ID; > =20 > diff --git a/DynamicTablesPkg/Include/ArchNameSpaceObjects.h b/DynamicTab= lesPkg/Include/ArchNameSpaceObjects.h > index b421c4cd29..ad3045dfec 100644 > --- a/DynamicTablesPkg/Include/ArchNameSpaceObjects.h > +++ b/DynamicTablesPkg/Include/ArchNameSpaceObjects.h > @@ -39,6 +39,7 @@ typedef enum ArchObjectID { > EArchObjFadtArmBootArch, ///< 11 - ARM boot arch information > EArchObjFadtHypervisorVendorId, ///< 12 - Hypervisor vendor identity = information > EArchObjFadtMiscInfo, ///< 13 - Legacy fields; RTC, latency= , flush stride, etc > + EArchObjWsmtProtectionFlags, ///< 14 - WSMT protection flags > EArchObjMax > } E_ARCH_OBJECT_ID; > =20 > @@ -214,4 +215,13 @@ typedef struct CmArchFadtMiscInfo { > UINT8 Century; > } CM_ARCH_FADT_MISC_INFO; > =20 > +/** A structure that describes the > + protection flags for the WSMT fields information. > + > + ID: EArchObjWsmtProtectionFlags > +*/ > +typedef struct CmArchWsmtProtectionFlags { > + UINT32 ProtectionFlags; > +} CM_ARCH_WSMT_PROTECTION_FLAGS; > + There is an ongoing effort to open the package a bit more to other architectures (https://edk2.groups.io/g/devel/message/114790). The common Arch namespace will be created in the (to be created) staging branch. Maybe we could add the object to the Arm namespace for now ? Sami, what do you think ? > #endif > diff --git a/DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/AcpiWsmtLib.inf b/= DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/AcpiWsmtLib.inf > new file mode 100644 > index 0000000000..f8683a5005 > --- /dev/null > +++ b/DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/AcpiWsmtLib.inf > @@ -0,0 +1,34 @@ > +## @file > +# WSMT Table Generator > +# > +# Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +## > + > +[Defines] > + INF_VERSION =3D 1.27 > + BASE_NAME =3D AcpiWsmtLib > + FILE_GUID =3D D6C34086-C914-4F8E-B56A-08329B4D1271 > + VERSION_STRING =3D 1.0 > + MODULE_TYPE =3D DXE_DRIVER > + LIBRARY_CLASS =3D NULL|DXE_DRIVER > + CONSTRUCTOR =3D AcpiWsmtLibConstructor > + DESTRUCTOR =3D AcpiWsmtLibDestructor > + > +[Sources] > + WsmtGenerator.c > + > +[Packages] > + DynamicTablesPkg/DynamicTablesPkg.dec > + MdeModulePkg/MdeModulePkg.dec > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + BaseLib > + DebugLib > + PcdLib > + > +[Pcd] > + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorId > + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision > diff --git a/DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/WsmtGenerator.c b/= DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/WsmtGenerator.c > new file mode 100644 > index 0000000000..c102152974 > --- /dev/null > +++ b/DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/WsmtGenerator.c > @@ -0,0 +1,221 @@ > +/** @file > + WSMT Table Generator > + > + Copyright (c) 2017 - 2023, Arm Limited. All rights reserved. > + Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > + @par Reference(s): > + - ACPI 6.5 Specification, Aug 29, 2022 > + - WSMT spec, version 1.0, April 18, 2016 > + > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include These 2 includes don't seem to be necessary. > +#include > + > +/** This macro expands to a function that retrieves the > + protection flags information for WSMT Table. > +*/ > +GET_OBJECT_LIST ( > + EObjNameSpaceArch, > + EArchObjWsmtProtectionFlags, > + CM_ARCH_WSMT_PROTECTION_FLAGS > + ); > + > +/** The AcpiWsmt is a template EFI_ACPI_WSMT_TABLE > + structure used for generating the WSMT Table. > +*/ > +STATIC > +EFI_ACPI_WSMT_TABLE mAcpiWsmt =3D { > + ACPI_HEADER ( > + EFI_ACPI_WINDOWS_SMM_SECURITY_MITIGATION_TABLE_SIGNATURE, > + EFI_ACPI_WSMT_TABLE, > + EFI_WSMT_TABLE_REVISION > + ), > + EFI_WSMT_PROTECTION_FLAGS_SYSTEM_RESOURCE_PROTECTION|EFI_WSMT_PROTECTI= ON_FLAGS_COMM_BUFFER_NESTED_PTR_PROTECTION|EFI_WSMT_PROTECTION_FLAGS_FIXED_= COMM_BUFFERS I think there is a 100 chars limit. It could be compacted with a macro (lik= e): #define WSMT_DEFAULT_PROTECTION_FLAGS \ EFI_WSMT_PROTECTION_FLAGS_SYSTEM_RESOURCE_PROTECTION \ EFI_WSMT_PROTECTION_FLAGS_COMM_BUFFER_NESTED_PTR_PROTECTION \ EFI_WSMT_PROTECTION_FLAGS_FIXED_COMM_BUFFERS > +}; > + > +/** Construct the WSMT table. > + > + This function invokes the Configuration Manager protocol interface > + to get the required hardware information for generating the ACPI > + table. > + > + If this function allocates any resources then they must be freed > + in the FreeXXXXTableResources function. > + > + @param [in] This Pointer to the table generator. > + @param [in] AcpiTableInfo Pointer to the ACPI Table Info. > + @param [in] CfgMgrProtocol Pointer to the Configuration Manager > + Protocol Interface. > + @param [out] Table Pointer to the constructed ACPI Table. > + > + @retval EFI_SUCCESS Table generated successfully. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The required object was not found. > + @retval EFI_BAD_BUFFER_SIZE The size returned by the Configuration > + Manager is less than the Object size for= the > + requested object. > +**/ > +STATIC > +EFI_STATUS > +BuildWsmtTable ( > + IN CONST ACPI_TABLE_GENERATOR *CONST This, > + IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableInfo, > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol= , > + OUT EFI_ACPI_DESCRIPTION_HEADER **CONST Table > + ) > +{ > + EFI_STATUS Status; > + CM_ARCH_WSMT_PROTECTION_FLAGS *ProtectionFlags; > + > + ASSERT (This !=3D NULL); > + ASSERT (AcpiTableInfo !=3D NULL); > + ASSERT (CfgMgrProtocol !=3D NULL); > + ASSERT (Table !=3D NULL); > + ASSERT (AcpiTableInfo->TableGeneratorId =3D=3D This->GeneratorID); > + ASSERT (AcpiTableInfo->AcpiTableSignature =3D=3D This->AcpiTableSignat= ure); > + > + if ((AcpiTableInfo->AcpiTableRevision < This->MinAcpiTableRevision) || > + (AcpiTableInfo->AcpiTableRevision > This->AcpiTableRevision)) > + { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: WSMT: Requested table revision =3D %d, is not supported." > + "Supported table revision: Minimum =3D %d, Maximum =3D %d\n", > + AcpiTableInfo->AcpiTableRevision, > + This->MinAcpiTableRevision, > + This->AcpiTableRevision > + )); > + return EFI_INVALID_PARAMETER; > + } > + > + *Table =3D NULL; > + > + Status =3D AddAcpiHeader ( > + CfgMgrProtocol, > + This, > + (EFI_ACPI_DESCRIPTION_HEADER *)&mAcpiWsmt, > + AcpiTableInfo, > + sizeof (EFI_ACPI_WSMT_TABLE) > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: WSMT: Failed to add ACPI header. Status =3D %r\n", > + Status > + )); > + return Status; > + } > + > + Status =3D GetEArchObjWsmtProtectionFlags ( > + CfgMgrProtocol, > + CM_NULL_TOKEN, > + &ProtectionFlags, > + NULL > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: WSMT: Failed to get protection flags information." \ > + " Status =3D %r\n", > + Status > + )); > + } else { > + mAcpiWsmt.ProtectionFlags =3D ProtectionFlags->ProtectionFlags; > + } It seems that EFI_WSMT_PROTECTION_FLAGS_COMM_BUFFER_NESTED_PTR_PROTECTION bit can only be set if EFI_WSMT_PROTECTION_FLAGS_FIXED_COMM_BUFFERS is set. Maybe this should be checked here. > + > + *Table =3D (EFI_ACPI_DESCRIPTION_HEADER *)&mAcpiWsmt; > + > + return Status; > +} > + > +/** The interface for the WSMT Table Generator. > +*/ > +STATIC > +CONST > +ACPI_TABLE_GENERATOR mWsmtGenerator =3D { > + // Generator ID > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdWsmt), > + // Generator Description > + L"ACPI.STD.WSMT.GENERATOR", > + // ACPI Table Signature > + EFI_ACPI_WINDOWS_SMM_SECURITY_MITIGATION_TABLE_SIGNATURE, > + // ACPI Table Revision supported by this Generator > + EFI_WSMT_TABLE_REVISION, > + // Minimum supported ACPI Table Revision > + EFI_WSMT_TABLE_REVISION, > + // Creator ID > + FixedPcdGet32 (PcdAcpiDefaultCreatorId), The Creator Id should be, from the ACPI spec: """ The vendor ID of the utility that created the table """ and/or: """ Vendor ID of utility that created the table. For tables containing Definition Blocks, this is the ID for the ASL Compiler. """ As the DynamicTables framework is creating the tables, a value representing that should be used. Other generators use: #define TABLE_GENERATOR_CREATOR_ID_ARM SIGNATURE_32('A', 'R', 'M', 'H') but this might be wrong to use this. It might be necessary to either: - Change the creator Id for all tables and update for instance: #define TABLE_GENERATOR_CREATOR_ID SIGNATURE_32('D', 'Y', 'N', 'T') - Create a Pcd > + // Creator Revision > + FixedPcdGet32 (PcdAcpiDefaultCreatorRevision), Similar remark for the Creator Revision: I think it should dependent on this present generator and not be common to all generators. This would allow to identify which version of the DynamicTables Generator created the table. > + // Build Table function > + BuildWsmtTable, > + // No additional resources are allocated by the generator. > + // Hence the Free Resource function is not required. > + NULL, > + // Extended build function not needed > + NULL, > + // Extended build function not implemented by the generator. > + // Hence extended free resource function is not required. > + NULL > +}; > + > +/** Register the Generator with the ACPI Table Factory. > + > + @param [in] ImageHandle The handle to the image. > + @param [in] SystemTable Pointer to the System Table. > + > + @retval EFI_SUCCESS The Generator is registered. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_ALREADY_STARTED The Generator for the Table ID > + is already registered. > +**/ > +EFI_STATUS > +EFIAPI > +AcpiWsmtLibConstructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + Status =3D RegisterAcpiTableGenerator (&mWsmtGenerator); > + DEBUG ((DEBUG_INFO, "WSMT: Register Generator. Status =3D %r\n", Statu= s)); > + ASSERT_EFI_ERROR (Status); > + return Status; > +} > + > +/** Deregister the Generator from the ACPI Table Factory. > + > + @param [in] ImageHandle The handle to the image. > + @param [in] SystemTable Pointer to the System Table. > + > + @retval EFI_SUCCESS The Generator is deregistered. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The Generator is not registered. > +**/ > +EFI_STATUS > +EFIAPI > +AcpiWsmtLibDestructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + Status =3D DeregisterAcpiTableGenerator (&mWsmtGenerator); > + DEBUG ((DEBUG_INFO, "WSMT: Deregister Generator. Status =3D %r\n", Sta= tus)); > + ASSERT_EFI_ERROR (Status); > + return Status; > +} -=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 (#116051): https://edk2.groups.io/g/devel/message/116051 Mute This Topic: https://groups.io/mt/104463462/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-