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 7A362740032 for ; Tue, 12 Sep 2023 08:12:47 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=FctBIM5xv78uma6lLcapu9SIkA3sCgpypmdyiWPHYBs=; 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=1694506366; v=1; b=uZFFSAkFgUV/VZHp5xM7FrQPqwv3W7/rETNXWMF096Zt4J5PkatpGO2KzqOG/Fj5p7CvuddW Zz5zTrvRS/9aUDGq2Q6cmIHJ/FMGIN0W1pTo4Qwz3n9BhfsDokwXTdkIJpgAvnocYPFRbt4FOY+ TpGTfQPsEDH2eRjdq8wNBlAE= X-Received: by 127.0.0.2 with SMTP id LFkpYY7687511xIaipIOo9t4; Tue, 12 Sep 2023 01:12:46 -0700 X-Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.20407.1694506365672210669 for ; Tue, 12 Sep 2023 01:12:45 -0700 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 47B55C15; Tue, 12 Sep 2023 01:13:22 -0700 (PDT) X-Received: from [10.34.100.121] (e126645.nice.arm.com [10.34.100.121]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 252AC3F738; Tue, 12 Sep 2023 01:12:44 -0700 (PDT) Message-ID: <8020385f-7884-ae09-6267-fc3c48d76833@arm.com> Date: Tue, 12 Sep 2023 10:12:35 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.0 Subject: Re: [edk2-devel] [PATCH v3 2/4] DynamicTablesPkg: Add support for simple method invocation. To: Jeff Brasen , devel@edk2.groups.io Cc: Alexei.Fedorov@arm.com, Sami.Mujawar@arm.com, Swatisri Kantamsetti , Ashish Singhal References: From: "PierreGondois" 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 Reply-To: devel@edk2.groups.io,pierre.gondois@arm.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: bYu8he58RKimFIpyZlbcwxOjx7686176AA= 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=uZFFSAkF; 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 Jeff, Thanks for the new serie, I think there is still a minor issue in the error handling in this patch, On 9/11/23 20:07, Jeff Brasen wrote: > Add support to add Return objects via AML that pass a single integer > argument to the named method. >=20 > Bug 4063340 All the patches have this Bug id, is it related to https://bugzilla.tianocore.org/ ? If this is a private id, I think it should be removed, >=20 > Signed-off-by: Jeff Brasen > Reviewed-by: Swatisri Kantamsetti > Reviewed-by: Ashish Singhal > --- > .../Include/Library/AmlLib/AmlLib.h | 54 ++++ > .../Common/AmlLib/CodeGen/AmlCodeGen.c | 236 ++++++++++++++++++ > 2 files changed, 290 insertions(+) >=20 > diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h b/DynamicTa= blesPkg/Include/Library/AmlLib/AmlLib.h > index d201ae9499..b82c7a3ce8 100644 > --- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > +++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > @@ -1194,6 +1194,60 @@ AmlCodeGenMethodRetInteger ( > OUT AML_OBJECT_NODE_HANDLE *NewObjectNode OPTIONAL > ); > =20 > +/** AML code generation for a method returning a NameString that takes a= n > + integer argument. > + > + AmlCodeGenMethodRetNameStringIntegerArgument ( > + "MET0", "MET1", 1, TRUE, 3, 5, ParentNode, NewObjectNode > + ); > + is equivalent of the following ASL code: > + Method(MET0, 1, Serialized, 3) { > + Return (MET1 (5)) > + } > + > + The ASL parameters "ReturnType" and "ParameterTypes" are not asked > + in this function. They are optional parameters in ASL. > + > + @param [in] MethodNameString The new Method's name. > + Must be a NULL-terminated ASL NameSt= ring > + e.g.: "MET0", "_SB.MET0", etc. > + The input string is copied. > + @param [in] ReturnedNameString The name of the object returned by t= he > + method. Optional parameter, can be: > + - NULL (ignored). > + - A NULL-terminated ASL NameString. > + e.g.: "MET0", "_SB.MET0", etc. > + The input string is copied. > + @param [in] NumArgs Number of arguments. > + Must be 0 <=3D NumArgs <=3D 6. > + @param [in] IsSerialized TRUE is equivalent to Serialized. > + FALSE is equivalent to NotSerialized= . > + Default is NotSerialized in ASL spec= . > + @param [in] SyncLevel Synchronization level for the method= . > + Must be 0 <=3D SyncLevel <=3D 15. > + Default is 0 in ASL. > + @param [in] IntegerArgument Argument to pass to the NameString. > + @param [in] ParentNode If provided, set ParentNode as the p= arent > + of the node created. > + @param [out] NewObjectNode If success, contains the created nod= e. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER Invalid parameter. > + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. > +**/ > +EFI_STATUS > +EFIAPI > +AmlCodeGenMethodRetNameStringIntegerArgument ( > + IN CONST CHAR8 *MethodNameString, > + IN CONST CHAR8 *ReturnedNameString OPTIONAL, > + IN UINT8 NumArgs, > + IN BOOLEAN IsSerialized, > + IN UINT8 SyncLevel, > + IN UINT64 IntegerArgument, > + IN AML_NODE_HANDLE ParentNode OPTIONAL, > + OUT AML_OBJECT_NODE_HANDLE *NewObjectNode OPTIONAL > + ); > + > /** Create a _LPI name. > =20 > AmlCreateLpiNode ("_LPI", 0, 1, ParentNode, &LpiNode) is > diff --git a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c = b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c > index 88537b7e2d..53c537e2d3 100644 > --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c > +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlCodeGen.c > @@ -1881,6 +1881,130 @@ AmlCodeGenReturnInteger ( > return Status; > } > =20 > +/** AML code generation for a Return object node, > + returning the object as an input NameString with a integer argument. > + > + AmlCodeGenReturn ("NAM1", 6, ParentNode, NewObjectNode) is > + equivalent of the following ASL code: > + Return(NAM1 (6)) > + > + The ACPI 6.3 specification, s20.2.5.3 "Type 1 Opcodes Encoding" states= : > + DefReturn :=3D ReturnOp ArgObject > + ReturnOp :=3D 0xA4 > + ArgObject :=3D TermArg =3D> DataRefObject > + > + Thus, the ReturnNode must be evaluated as a DataRefObject. It can > + be a NameString referencing an object. As this CodeGen Api doesn't > + do semantic checking, it is strongly advised to check the AML bytecode > + generated by this function against an ASL compiler. > + > + The ReturnNode must be generated inside a Method body scope. > + > + @param [in] NameString The object referenced by this NameString > + is returned by the Return ASL statement. > + Must be a NULL-terminated ASL NameString > + e.g.: "NAM1", "_SB.NAM1", etc. > + The input string is copied. > + @param [in] Integer Argument to pass to the NameString > + @param [in] ParentNode If provided, set ParentNode as the parent > + of the node created. > + Must be a MethodOp node. > + @param [out] NewObjectNode If success, contains the created node. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER Invalid parameter. > + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +AmlCodeGenReturnNameStringIntegerArgument ( > + IN CONST CHAR8 *NameString, > + IN UINT64 Integer, > + IN AML_NODE_HEADER *ParentNode OPTIONAL, > + OUT AML_OBJECT_NODE **NewObjectNode OPTIONAL > + ) > +{ > + EFI_STATUS Status; > + AML_DATA_NODE *DataNode; > + AML_OBJECT_NODE *IntNode; > + CHAR8 *AmlNameString; > + UINT32 AmlNameStringSize; > + AML_OBJECT_NODE *ObjectNode; > + > + DataNode =3D NULL; > + IntNode =3D NULL; > + ObjectNode =3D NULL; > + > + Status =3D ConvertAslNameToAmlName (NameString, &AmlNameString); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + return Status; > + } > + > + Status =3D AmlGetNameStringSize (AmlNameString, &AmlNameStringSize); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + goto exit_handler; > + } > + > + Status =3D AmlCreateDataNode ( > + EAmlNodeDataTypeNameString, > + (UINT8 *)AmlNameString, > + AmlNameStringSize, > + &DataNode > + ); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + goto exit_handler; > + } > + > + Status =3D AmlCodeGenInteger (Integer, &IntNode); > + if (EFI_ERROR (Status)) { > + ASSERT (0); If we fail here, I think DataNode would not be freed, > + goto exit_handler; > + } > + > + // AmlCodeGenReturn() deletes DataNode if error. > + Status =3D AmlCodeGenReturn ( > + (AML_NODE_HEADER *)DataNode, > + ParentNode, > + &ObjectNode > + ); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + goto exit_handler; > + } > + > + Status =3D AmlVarListAddTail ( > + (AML_NODE_HANDLE)ObjectNode, > + (AML_NODE_HANDLE)IntNode > + ); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + goto exit_handler; > + } > + > + if (NewObjectNode !=3D 0) { > + *NewObjectNode =3D ObjectNode; > + } If everything succeeds, then IntNode and ObjectNode will be deleted. I think this is problematic as these are nodes that have been attached to the AML tree, so if I'm not mistaken, I don't think the function should produce the expected result. I think it should look like this instead (this is not tested): ------ Status =3D AmlVarListAddTail ( (AML_NODE_HANDLE)ObjectNode, (AML_NODE_HANDLE)IntNode ); if (EFI_ERROR (Status)) { ASSERT (0); goto exit_handler; } IntNode =3D NULL; if (NewObjectNode !=3D 0) { *NewObjectNode =3D ObjectNode; } goto exit_handler; error_handler: if (IntNode !=3D NULL) { AmlDeleteTree ((AML_NODE_HANDLE)IntNode); } if (ObjectNode !=3D NULL) { AmlDeleteTree ((AML_NODE_HANDLE)ObjectNode); } exit_handler: if (AmlNameString !=3D NULL) { FreePool (AmlNameString); } return Status; } ------ Some 'goto exit_handler' might also need to be changed to 'goto error_handler', Otherwise, the other patches look good to me, Regards, Pierre > + > +exit_handler: > + if (AmlNameString !=3D NULL) { > + FreePool (AmlNameString); > + } > + > + if (IntNode !=3D NULL) { > + AmlDeleteTree ((AML_NODE_HANDLE)IntNode); > + } > + > + if (ObjectNode !=3D NULL) { > + AmlDeleteTree ((AML_NODE_HANDLE)ObjectNode); > + } > + > + return Status; > +} > + > /** AML code generation for a method returning a NameString. > =20 > AmlCodeGenMethodRetNameString ( > @@ -1989,6 +2113,118 @@ error_handler: > return Status; > } > =20 > +/** AML code generation for a method returning a NameString that takes a= n > + integer argument. > + > + AmlCodeGenMethodRetNameStringIntegerArgument ( > + "MET0", "MET1", 1, TRUE, 3, 5, ParentNode, NewObjectNode > + ); > + is equivalent of the following ASL code: > + Method(MET0, 1, Serialized, 3) { > + Return (MET1 (5)) > + } > + > + The ASL parameters "ReturnType" and "ParameterTypes" are not asked > + in this function. They are optional parameters in ASL. > + > + @param [in] MethodNameString The new Method's name. > + Must be a NULL-terminated ASL NameSt= ring > + e.g.: "MET0", "_SB.MET0", etc. > + The input string is copied. > + @param [in] ReturnedNameString The name of the object returned by t= he > + method. Optional parameter, can be: > + - NULL (ignored). > + - A NULL-terminated ASL NameString. > + e.g.: "MET0", "_SB.MET0", etc. > + The input string is copied. > + @param [in] NumArgs Number of arguments. > + Must be 0 <=3D NumArgs <=3D 6. > + @param [in] IsSerialized TRUE is equivalent to Serialized. > + FALSE is equivalent to NotSerialized= . > + Default is NotSerialized in ASL spec= . > + @param [in] SyncLevel Synchronization level for the method= . > + Must be 0 <=3D SyncLevel <=3D 15. > + Default is 0 in ASL. > + @param [in] IntegerArgument Argument to pass to the NameString. > + @param [in] ParentNode If provided, set ParentNode as the p= arent > + of the node created. > + @param [out] NewObjectNode If success, contains the created nod= e. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER Invalid parameter. > + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. > +**/ > +EFI_STATUS > +EFIAPI > +AmlCodeGenMethodRetNameStringIntegerArgument ( > + IN CONST CHAR8 *MethodNameString, > + IN CONST CHAR8 *ReturnedNameString OPTIONAL, > + IN UINT8 NumArgs, > + IN BOOLEAN IsSerialized, > + IN UINT8 SyncLevel, > + IN UINT64 IntegerArgument, > + IN AML_NODE_HANDLE ParentNode OPTIONAL, > + OUT AML_OBJECT_NODE_HANDLE *NewObjectNode OPTIONAL > + ) > +{ > + EFI_STATUS Status; > + AML_OBJECT_NODE_HANDLE MethodNode; > + > + if ((MethodNameString =3D=3D NULL) || > + ((ParentNode =3D=3D NULL) && (NewObjectNode =3D=3D NULL))) > + { > + ASSERT (0); > + return EFI_INVALID_PARAMETER; > + } > + > + // Create a Method named MethodNameString. > + Status =3D AmlCodeGenMethod ( > + MethodNameString, > + NumArgs, > + IsSerialized, > + SyncLevel, > + NULL, > + &MethodNode > + ); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + return Status; > + } > + > + // Return ReturnedNameString if provided. > + if (ReturnedNameString !=3D NULL) { > + Status =3D AmlCodeGenReturnNameStringIntegerArgument ( > + ReturnedNameString, > + IntegerArgument, > + (AML_NODE_HANDLE)MethodNode, > + NULL > + ); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + goto error_handler; > + } > + } > + > + Status =3D LinkNode ( > + MethodNode, > + ParentNode, > + NewObjectNode > + ); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + goto error_handler; > + } > + > + return Status; > + > +error_handler: > + if (MethodNode !=3D NULL) { > + AmlDeleteTree ((AML_NODE_HANDLE)MethodNode); > + } > + > + return Status; > +} > + > /** AML code generation for a method returning an Integer. > =20 > AmlCodeGenMethodRetInteger ( -=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 (#108519): https://edk2.groups.io/g/devel/message/108519 Mute This Topic: https://groups.io/mt/101303763/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-