.../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Just like CPU _UID, ETE UID also needs to be unique so
use AcpiProcessorUid instead of CpuName
Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
---
.../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
index 8228c7845a..724f33c660 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
@@ -359,6 +359,7 @@ CreateAmlCpcNode (
@param [in] Generator The SSDT Cpu Topology generator.
@param [in] ParentNode Parent node to attach the Cpu node to.
+ @param [in] GicCInfo CM_ARM_GICC_INFO object used to create the node.
@param [in] CpuName Value used to generate the node name.
@param [out] EtNodePtr If not NULL, return the created Cpu node.
@@ -372,6 +373,7 @@ EFIAPI
CreateAmlEtd (
IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator,
IN AML_NODE_HANDLE ParentNode,
+ IN CM_ARM_GICC_INFO *GicCInfo,
IN UINT32 CpuName,
OUT AML_OBJECT_NODE_HANDLE *EtNodePtr OPTIONAL
)
@@ -397,7 +399,7 @@ CreateAmlEtd (
Status = AmlCodeGenNameInteger (
"_UID",
- CpuName,
+ GicCInfo->AcpiProcessorUid,
EtNode,
NULL
);
@@ -474,6 +476,7 @@ CreateAmlEtNode (
Status = CreateAmlEtd (
Generator,
Node,
+ GicCInfo,
CpuName,
NULL
);
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111231): https://edk2.groups.io/g/devel/message/111231
Mute This Topic: https://groups.io/mt/102598848/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-Hi Ashish, Thank you for this fix. These changes look good to me. Reviewed-by: Sami Mujawar <sami.mujawar@arm.com> Regards, Sami Mujawar On 15/11/2023 03:19 am, Ashish Singhal wrote: > Just like CPU _UID, ETE UID also needs to be unique so > use AcpiProcessorUid instead of CpuName > > Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com> > --- > .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > index 8228c7845a..724f33c660 100644 > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > @@ -359,6 +359,7 @@ CreateAmlCpcNode ( > > @param [in] Generator The SSDT Cpu Topology generator. > @param [in] ParentNode Parent node to attach the Cpu node to. > + @param [in] GicCInfo CM_ARM_GICC_INFO object used to create the node. > @param [in] CpuName Value used to generate the node name. > @param [out] EtNodePtr If not NULL, return the created Cpu node. > > @@ -372,6 +373,7 @@ EFIAPI > CreateAmlEtd ( > IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator, > IN AML_NODE_HANDLE ParentNode, > + IN CM_ARM_GICC_INFO *GicCInfo, > IN UINT32 CpuName, > OUT AML_OBJECT_NODE_HANDLE *EtNodePtr OPTIONAL > ) > @@ -397,7 +399,7 @@ CreateAmlEtd ( > > Status = AmlCodeGenNameInteger ( > "_UID", > - CpuName, > + GicCInfo->AcpiProcessorUid, > EtNode, > NULL > ); > @@ -474,6 +476,7 @@ CreateAmlEtNode ( > Status = CreateAmlEtd ( > Generator, > Node, > + GicCInfo, > CpuName, > NULL > ); IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111469): https://edk2.groups.io/g/devel/message/111469 Mute This Topic: https://groups.io/mt/102598848/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Ashish, I have created a bugzilla ticket for this at https://bugzilla.tianocore.org/show_bug.cgi?id=4600. Regards, Sami Mujawar -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111471): https://edk2.groups.io/g/devel/message/111471 Mute This Topic: https://groups.io/mt/102598848/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 11/15/23 04:19, Ashish Singhal via groups.io wrote: > Just like CPU _UID, ETE UID also needs to be unique so > use AcpiProcessorUid instead of CpuName > > Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com> > --- > .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) Is this a fixup for the recent feature [PATCH v3 00/11] Update MADT for ACPI 6.5, and add TRBE & ETE support https://edk2.groups.io/g/devel/message/108996 ? If so, then I *think* this qualifies to be merged during the hard feature freeze (+Liming +Mike), but: - I think we should have a "Fixes:" tag in the commit message (for pointing out the commit that should have contained the code being added/updated now) - I think we should have a BZ too (also linked into the commit message). Laszlo > > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > index 8228c7845a..724f33c660 100644 > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > @@ -359,6 +359,7 @@ CreateAmlCpcNode ( > > @param [in] Generator The SSDT Cpu Topology generator. > @param [in] ParentNode Parent node to attach the Cpu node to. > + @param [in] GicCInfo CM_ARM_GICC_INFO object used to create the node. > @param [in] CpuName Value used to generate the node name. > @param [out] EtNodePtr If not NULL, return the created Cpu node. > > @@ -372,6 +373,7 @@ EFIAPI > CreateAmlEtd ( > IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator, > IN AML_NODE_HANDLE ParentNode, > + IN CM_ARM_GICC_INFO *GicCInfo, > IN UINT32 CpuName, > OUT AML_OBJECT_NODE_HANDLE *EtNodePtr OPTIONAL > ) > @@ -397,7 +399,7 @@ CreateAmlEtd ( > > Status = AmlCodeGenNameInteger ( > "_UID", > - CpuName, > + GicCInfo->AcpiProcessorUid, > EtNode, > NULL > ); > @@ -474,6 +476,7 @@ CreateAmlEtNode ( > Status = CreateAmlEtd ( > Generator, > Node, > + GicCInfo, > CpuName, > NULL > ); -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111346): https://edk2.groups.io/g/devel/message/111346 Mute This Topic: https://groups.io/mt/102598848/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Laszlo, Liming, Mike, Please see my response inline marked [SAMI]. Regards, Sami Mujawar On 17/11/2023 09:20 am, Laszlo Ersek wrote: > On 11/15/23 04:19, Ashish Singhal via groups.io wrote: >> Just like CPU _UID, ETE UID also needs to be unique so >> use AcpiProcessorUid instead of CpuName >> >> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com> >> --- >> .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) > Is this a fixup for the recent feature > > [PATCH v3 00/11] Update MADT for ACPI 6.5, and add TRBE & ETE support > https://edk2.groups.io/g/devel/message/108996 > > ? > > If so, then I *think* this qualifies to be merged during the hard > feature freeze (+Liming +Mike), but: [SAMI] I raised a bugzilla for this issue at https://bugzilla.tianocore.org/show_bug.cgi?id=4600 and have also created a pull request at https://github.com/tianocore/edk2/pull/5061. This patch has also passed the CI checks when I did a draft pull request. I am not sure if I can apply the push label as we are in the code freeze stage. Can you advise on how to proceed, please? [/SAMI] > - I think we should have a "Fixes:" tag in the commit message (for > pointing out the commit that should have contained the code being > added/updated now) > > - I think we should have a BZ too (also linked into the commit message). > > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111483): https://edk2.groups.io/g/devel/message/111483 Mute This Topic: https://groups.io/mt/102709920/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Sami: I am OK to merge this patch for this stable bug. I will add push label for https://github.com/tianocore/edk2/pull/5061 Thanks Liming > -----邮件原件----- > 发件人: Sami Mujawar <sami.mujawar@arm.com> > 发送时间: 2023年11月21日 0:07 > 收件人: Laszlo Ersek <lersek@redhat.com>; Liming Gao (Byosoft address) > <gaoliming@byosoft.com.cn>; Michael Kinney <michael.d.kinney@intel.com>; > devel@edk2.groups.io; ashishsingha@nvidia.com; quic_llindhol@quicinc.com; > ardb+tianocore@kernel.org; jbrasen@nvidia.com > 抄送: nd@arm.com > 主题: edk2-stable202311: Re: [edk2-devel] [PATCH] DynamicTablesPkg: Fix > ETE _UID Creation > > Hi Laszlo, Liming, Mike, > > Please see my response inline marked [SAMI]. > > Regards, > > Sami Mujawar > > On 17/11/2023 09:20 am, Laszlo Ersek wrote: > > On 11/15/23 04:19, Ashish Singhal via groups.io wrote: > >> Just like CPU _UID, ETE UID also needs to be unique so > >> use AcpiProcessorUid instead of CpuName > >> > >> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com> > >> --- > >> .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 > ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > > Is this a fixup for the recent feature > > > > [PATCH v3 00/11] Update MADT for ACPI 6.5, and add TRBE & ETE support > > https://edk2.groups.io/g/devel/message/108996 > > > > ? > > > > If so, then I *think* this qualifies to be merged during the hard > > feature freeze (+Liming +Mike), but: > > [SAMI] I raised a bugzilla for this issue at > https://bugzilla.tianocore.org/show_bug.cgi?id=4600 > > and have also created a pull request at > https://github.com/tianocore/edk2/pull/5061. > > This patch has also passed the CI checks when I did a draft pull request. > > I am not sure if I can apply the push label as we are in the code freeze > stage. > > Can you advise on how to proceed, please? > > [/SAMI] > > > - I think we should have a "Fixes:" tag in the commit message (for > > pointing out the commit that should have contained the code being > > added/updated now) > > > > - I think we should have a BZ too (also linked into the commit message). > > > > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111492): https://edk2.groups.io/g/devel/message/111492 Mute This Topic: https://groups.io/mt/102720225/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
________________________________ From: Laszlo Ersek <lersek@redhat.com> Sent: Friday, November 17, 2023 2:20 AM To: devel@edk2.groups.io <devel@edk2.groups.io>; Ashish Singhal <ashishsingha@nvidia.com>; quic_llindhol@quicinc.com <quic_llindhol@quicinc.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; sami.mujawar@arm.com <sami.mujawar@arm.com>; Jeff Brasen <jbrasen@nvidia.com> Cc: Michael Kinney <michael.d.kinney@intel.com>; Liming Gao (Byosoft address) <gaoliming@byosoft.com.cn> Subject: Re: [edk2-devel] [PATCH] DynamicTablesPkg: Fix ETE _UID Creation External email: Use caution opening links or attachments On 11/15/23 04:19, Ashish Singhal via groups.io wrote: > Just like CPU _UID, ETE UID also needs to be unique so > use AcpiProcessorUid instead of CpuName > > Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com> > --- > .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) Is this a fixup for the recent feature [PATCH v3 00/11] Update MADT for ACPI 6.5, and add TRBE & ETE support https://edk2.groups.io/g/devel/message/108996 ? If so, then I *think* this qualifies to be merged during the hard feature freeze (+Liming +Mike), but: - I think we should have a "Fixes:" tag in the commit message (for pointing out the commit that should have contained the code being added/updated now) - I think we should have a BZ too (also linked into the commit message). Laszlo Hello Laszlo, The issue was indeed introduced in the patch series you pointed to and precisely in the commit https://github.com/tianocore/edk2/commit/3ee23713e1ce09faa6fa66ee6799e3e336deb58b. This is indeed a bug and should ideally be fixed as soon as possible. Do you need me to file BZ bug and link that in commit message? Thanks Ashish > > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > index 8228c7845a..724f33c660 100644 > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > @@ -359,6 +359,7 @@ CreateAmlCpcNode ( > > @param [in] Generator The SSDT Cpu Topology generator. > @param [in] ParentNode Parent node to attach the Cpu node to. > + @param [in] GicCInfo CM_ARM_GICC_INFO object used to create the node. > @param [in] CpuName Value used to generate the node name. > @param [out] EtNodePtr If not NULL, return the created Cpu node. > > @@ -372,6 +373,7 @@ EFIAPI > CreateAmlEtd ( > IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator, > IN AML_NODE_HANDLE ParentNode, > + IN CM_ARM_GICC_INFO *GicCInfo, > IN UINT32 CpuName, > OUT AML_OBJECT_NODE_HANDLE *EtNodePtr OPTIONAL > ) > @@ -397,7 +399,7 @@ CreateAmlEtd ( > > Status = AmlCodeGenNameInteger ( > "_UID", > - CpuName, > + GicCInfo->AcpiProcessorUid, > EtNode, > NULL > ); > @@ -474,6 +476,7 @@ CreateAmlEtNode ( > Status = CreateAmlEtd ( > Generator, > Node, > + GicCInfo, > CpuName, > NULL > ); -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111404): https://edk2.groups.io/g/devel/message/111404 Mute This Topic: https://groups.io/mt/102598848/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 11/17/23 17:37, Ashish Singhal wrote: > > > ------------------------------------------------------------------------ > *From:* Laszlo Ersek <lersek@redhat.com> > *Sent:* Friday, November 17, 2023 2:20 AM > *To:* devel@edk2.groups.io <devel@edk2.groups.io>; Ashish Singhal > <ashishsingha@nvidia.com>; quic_llindhol@quicinc.com > <quic_llindhol@quicinc.com>; ardb+tianocore@kernel.org > <ardb+tianocore@kernel.org>; sami.mujawar@arm.com > <sami.mujawar@arm.com>; Jeff Brasen <jbrasen@nvidia.com> > *Cc:* Michael Kinney <michael.d.kinney@intel.com>; Liming Gao (Byosoft > address) <gaoliming@byosoft.com.cn> > *Subject:* Re: [edk2-devel] [PATCH] DynamicTablesPkg: Fix ETE _UID Creation > > External email: Use caution opening links or attachments > > > On 11/15/23 04:19, Ashish Singhal via groups.io wrote: >> Just like CPU _UID, ETE UID also needs to be unique so >> use AcpiProcessorUid instead of CpuName >> >> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com> >> --- >> .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) > > Is this a fixup for the recent feature > > [PATCH v3 00/11] Update MADT for ACPI 6.5, and add TRBE & ETE support > https://edk2.groups.io/g/devel/message/108996 > <https://edk2.groups.io/g/devel/message/108996> > > ? > > If so, then I *think* this qualifies to be merged during the hard > feature freeze (+Liming +Mike), but: > > - I think we should have a "Fixes:" tag in the commit message (for > pointing out the commit that should have contained the code being > added/updated now) > > - I think we should have a BZ too (also linked into the commit message). > > Laszlo > > Hello Laszlo, > > The issue was indeed introduced in the patch series you pointed to and > precisely in the commit > https://github.com/tianocore/edk2/commit/3ee23713e1ce09faa6fa66ee6799e3e336deb58b <https://github.com/tianocore/edk2/commit/3ee23713e1ce09faa6fa66ee6799e3e336deb58b>. This is indeed a bug and should ideally be fixed as soon as possible. Do you need me to file BZ bug and link that in commit message? A BZ ticket would be great, yes. It helps with the release notes (determining the contents of a release). If/when you file the new BZ, please add the old BZ's URL to the See Also field. ... Oh, wait, the original series didn't have a BZ. That's a pity. Thanks Laszlo > > Thanks > Ashish > >> >> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c >> index 8228c7845a..724f33c660 100644 >> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c >> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c >> @@ -359,6 +359,7 @@ CreateAmlCpcNode ( >> >> @param [in] Generator The SSDT Cpu Topology generator. >> @param [in] ParentNode Parent node to attach the Cpu node to. >> + @param [in] GicCInfo CM_ARM_GICC_INFO object used to create the node. >> @param [in] CpuName Value used to generate the node name. >> @param [out] EtNodePtr If not NULL, return the created Cpu node. >> >> @@ -372,6 +373,7 @@ EFIAPI >> CreateAmlEtd ( >> IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator, >> IN AML_NODE_HANDLE ParentNode, >> + IN CM_ARM_GICC_INFO *GicCInfo, >> IN UINT32 CpuName, >> OUT AML_OBJECT_NODE_HANDLE *EtNodePtr OPTIONAL >> ) >> @@ -397,7 +399,7 @@ CreateAmlEtd ( >> >> Status = AmlCodeGenNameInteger ( >> "_UID", >> - CpuName, >> + GicCInfo->AcpiProcessorUid, >> EtNode, >> NULL >> ); >> @@ -474,6 +476,7 @@ CreateAmlEtNode ( >> Status = CreateAmlEtd ( >> Generator, >> Node, >> + GicCInfo, >> CpuName, >> NULL >> ); > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111413): https://edk2.groups.io/g/devel/message/111413 Mute This Topic: https://groups.io/mt/102598848/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
It seems like my BZ account has been disabled and I am not able to create an issue there myself. Can one of you create it so that the bug can be fixed? I am meanwhile working to restore my account. ________________________________ From: Laszlo Ersek <lersek@redhat.com> Sent: Friday, November 17, 2023 1:30 PM To: Ashish Singhal <ashishsingha@nvidia.com>; devel@edk2.groups.io <devel@edk2.groups.io>; quic_llindhol@quicinc.com <quic_llindhol@quicinc.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; sami.mujawar@arm.com <sami.mujawar@arm.com>; Jeff Brasen <jbrasen@nvidia.com> Cc: Michael Kinney <michael.d.kinney@intel.com>; Liming Gao (Byosoft address) <gaoliming@byosoft.com.cn> Subject: Re: [edk2-devel] [PATCH] DynamicTablesPkg: Fix ETE _UID Creation External email: Use caution opening links or attachments On 11/17/23 17:37, Ashish Singhal wrote: > > > ------------------------------------------------------------------------ > *From:* Laszlo Ersek <lersek@redhat.com> > *Sent:* Friday, November 17, 2023 2:20 AM > *To:* devel@edk2.groups.io <devel@edk2.groups.io>; Ashish Singhal > <ashishsingha@nvidia.com>; quic_llindhol@quicinc.com > <quic_llindhol@quicinc.com>; ardb+tianocore@kernel.org > <ardb+tianocore@kernel.org>; sami.mujawar@arm.com > <sami.mujawar@arm.com>; Jeff Brasen <jbrasen@nvidia.com> > *Cc:* Michael Kinney <michael.d.kinney@intel.com>; Liming Gao (Byosoft > address) <gaoliming@byosoft.com.cn> > *Subject:* Re: [edk2-devel] [PATCH] DynamicTablesPkg: Fix ETE _UID Creation > > External email: Use caution opening links or attachments > > > On 11/15/23 04:19, Ashish Singhal via groups.io wrote: >> Just like CPU _UID, ETE UID also needs to be unique so >> use AcpiProcessorUid instead of CpuName >> >> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com> >> --- >> .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) > > Is this a fixup for the recent feature > > [PATCH v3 00/11] Update MADT for ACPI 6.5, and add TRBE & ETE support > https://edk2.groups.io/g/devel/message/108996 > <https://edk2.groups.io/g/devel/message/108996> > > ? > > If so, then I *think* this qualifies to be merged during the hard > feature freeze (+Liming +Mike), but: > > - I think we should have a "Fixes:" tag in the commit message (for > pointing out the commit that should have contained the code being > added/updated now) > > - I think we should have a BZ too (also linked into the commit message). > > Laszlo > > Hello Laszlo, > > The issue was indeed introduced in the patch series you pointed to and > precisely in the commit > https://github.com/tianocore/edk2/commit/3ee23713e1ce09faa6fa66ee6799e3e336deb58b <https://github.com/tianocore/edk2/commit/3ee23713e1ce09faa6fa66ee6799e3e336deb58b>. This is indeed a bug and should ideally be fixed as soon as possible. Do you need me to file BZ bug and link that in commit message? A BZ ticket would be great, yes. It helps with the release notes (determining the contents of a release). If/when you file the new BZ, please add the old BZ's URL to the See Also field. ... Oh, wait, the original series didn't have a BZ. That's a pity. Thanks Laszlo > > Thanks > Ashish > >> >> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c >> index 8228c7845a..724f33c660 100644 >> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c >> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c >> @@ -359,6 +359,7 @@ CreateAmlCpcNode ( >> >> @param [in] Generator The SSDT Cpu Topology generator. >> @param [in] ParentNode Parent node to attach the Cpu node to. >> + @param [in] GicCInfo CM_ARM_GICC_INFO object used to create the node. >> @param [in] CpuName Value used to generate the node name. >> @param [out] EtNodePtr If not NULL, return the created Cpu node. >> >> @@ -372,6 +373,7 @@ EFIAPI >> CreateAmlEtd ( >> IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator, >> IN AML_NODE_HANDLE ParentNode, >> + IN CM_ARM_GICC_INFO *GicCInfo, >> IN UINT32 CpuName, >> OUT AML_OBJECT_NODE_HANDLE *EtNodePtr OPTIONAL >> ) >> @@ -397,7 +399,7 @@ CreateAmlEtd ( >> >> Status = AmlCodeGenNameInteger ( >> "_UID", >> - CpuName, >> + GicCInfo->AcpiProcessorUid, >> EtNode, >> NULL >> ); >> @@ -474,6 +476,7 @@ CreateAmlEtNode ( >> Status = CreateAmlEtd ( >> Generator, >> Node, >> + GicCInfo, >> CpuName, >> NULL >> ); > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111419): https://edk2.groups.io/g/devel/message/111419 Mute This Topic: https://groups.io/mt/102598848/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Tue, Nov 14, 2023 at 20:19:04 -0700, Ashish Singhal wrote:
> Just like CPU _UID, ETE UID also needs to be unique so
> use AcpiProcessorUid instead of CpuName
>
> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> ---
> .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> index 8228c7845a..724f33c660 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> @@ -359,6 +359,7 @@ CreateAmlCpcNode (
>
> @param [in] Generator The SSDT Cpu Topology generator.
> @param [in] ParentNode Parent node to attach the Cpu node to.
> + @param [in] GicCInfo CM_ARM_GICC_INFO object used to create the node.
> @param [in] CpuName Value used to generate the node name.
Can that replace both uses of CpuName in the function (so it can be
dropped), or does
Status = WriteAslName ('E', CpuName, AslName);
have other requirements?
/
Leif
> @param [out] EtNodePtr If not NULL, return the created Cpu node.
>
> @@ -372,6 +373,7 @@ EFIAPI
> CreateAmlEtd (
> IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator,
> IN AML_NODE_HANDLE ParentNode,
> + IN CM_ARM_GICC_INFO *GicCInfo,
> IN UINT32 CpuName,
> OUT AML_OBJECT_NODE_HANDLE *EtNodePtr OPTIONAL
> )
> @@ -397,7 +399,7 @@ CreateAmlEtd (
>
> Status = AmlCodeGenNameInteger (
> "_UID",
> - CpuName,
> + GicCInfo->AcpiProcessorUid,
> EtNode,
> NULL
> );
> @@ -474,6 +476,7 @@ CreateAmlEtNode (
> Status = CreateAmlEtd (
> Generator,
> Node,
> + GicCInfo,
> CpuName,
> NULL
> );
> --
> 2.17.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111279): https://edk2.groups.io/g/devel/message/111279
Mute This Topic: https://groups.io/mt/102598848/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
________________________________
From: Leif Lindholm <quic_llindhol@quicinc.com>
Sent: Wednesday, November 15, 2023 9:21 AM
To: Ashish Singhal <ashishsingha@nvidia.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; sami.mujawar@arm.com <sami.mujawar@arm.com>; Jeff Brasen <jbrasen@nvidia.com>
Subject: Re: [PATCH] DynamicTablesPkg: Fix ETE _UID Creation
External email: Use caution opening links or attachments
On Tue, Nov 14, 2023 at 20:19:04 -0700, Ashish Singhal wrote:
> Just like CPU _UID, ETE UID also needs to be unique so
> use AcpiProcessorUid instead of CpuName
>
> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> ---
> .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> index 8228c7845a..724f33c660 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> @@ -359,6 +359,7 @@ CreateAmlCpcNode (
>
> @param [in] Generator The SSDT Cpu Topology generator.
> @param [in] ParentNode Parent node to attach the Cpu node to.
> + @param [in] GicCInfo CM_ARM_GICC_INFO object used to create the node.
> @param [in] CpuName Value used to generate the node name.
Can that replace both uses of CpuName in the function (so it can be
dropped), or does
Status = WriteAslName ('E', CpuName, AslName);
have other requirements?
/
Leif
Hello Leif,
CPU Name can be more logical, and you may have the same CPU name in different clusters for example. _UID however needs to be unique.
Thanks
Ashish
> @param [out] EtNodePtr If not NULL, return the created Cpu node.
>
> @@ -372,6 +373,7 @@ EFIAPI
> CreateAmlEtd (
> IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator,
> IN AML_NODE_HANDLE ParentNode,
> + IN CM_ARM_GICC_INFO *GicCInfo,
> IN UINT32 CpuName,
> OUT AML_OBJECT_NODE_HANDLE *EtNodePtr OPTIONAL
> )
> @@ -397,7 +399,7 @@ CreateAmlEtd (
>
> Status = AmlCodeGenNameInteger (
> "_UID",
> - CpuName,
> + GicCInfo->AcpiProcessorUid,
> EtNode,
> NULL
> );
> @@ -474,6 +476,7 @@ CreateAmlEtNode (
> Status = CreateAmlEtd (
> Generator,
> Node,
> + GicCInfo,
> CpuName,
> NULL
> );
> --
> 2.17.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111280): https://edk2.groups.io/g/devel/message/111280
Mute This Topic: https://groups.io/mt/102598848/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Wed, Nov 15, 2023 at 16:24:46 +0000, Ashish Singhal via groups.io wrote:
> On Tue, Nov 14, 2023 at 20:19:04 -0700, Ashish Singhal wrote:
> > Just like CPU _UID, ETE UID also needs to be unique so
> > use AcpiProcessorUid instead of CpuName
> >
> > Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> > ---
> > .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> > index 8228c7845a..724f33c660 100644
> > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> > @@ -359,6 +359,7 @@ CreateAmlCpcNode (
> >
> > @param [in] Generator The SSDT Cpu Topology generator.
> > @param [in] ParentNode Parent node to attach the Cpu node to.
> > + @param [in] GicCInfo CM_ARM_GICC_INFO object used to create the node.
> > @param [in] CpuName Value used to generate the node name.
>
> Can that replace both uses of CpuName in the function (so it can be
> dropped), or does
>
> Status = WriteAslName ('E', CpuName, AslName);
>
> have other requirements?
>
> /
> Leif
>
> Hello Leif,
>
> CPU Name can be more logical, and you may have the same CPU name in
> different clusters for example. _UID however needs to be unique.
Sure, makes sense.
I just dislike functions that take too many arguments, so wanted to
make sure we weren't missing an opportunity to drop one as we were
adding this new one.
Never mind me :)
Thanks,
Leif
> Thanks
> Ashish
>
> > @param [out] EtNodePtr If not NULL, return the created Cpu node.
> >
> > @@ -372,6 +373,7 @@ EFIAPI
> > CreateAmlEtd (
> > IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator,
> > IN AML_NODE_HANDLE ParentNode,
> > + IN CM_ARM_GICC_INFO *GicCInfo,
> > IN UINT32 CpuName,
> > OUT AML_OBJECT_NODE_HANDLE *EtNodePtr OPTIONAL
> > )
> > @@ -397,7 +399,7 @@ CreateAmlEtd (
> >
> > Status = AmlCodeGenNameInteger (
> > "_UID",
> > - CpuName,
> > + GicCInfo->AcpiProcessorUid,
> > EtNode,
> > NULL
> > );
> > @@ -474,6 +476,7 @@ CreateAmlEtNode (
> > Status = CreateAmlEtd (
> > Generator,
> > Node,
> > + GicCInfo,
> > CpuName,
> > NULL
> > );
> > --
> > 2.17.1
> >
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111281): https://edk2.groups.io/g/devel/message/111281
Mute This Topic: https://groups.io/mt/102598848/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
________________________________
From: Leif Lindholm <quic_llindhol@quicinc.com>
Sent: Wednesday, November 15, 2023 10:20 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Ashish Singhal <ashishsingha@nvidia.com>
Cc: ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; sami.mujawar@arm.com <sami.mujawar@arm.com>; Jeff Brasen <jbrasen@nvidia.com>
Subject: Re: [edk2-devel] [PATCH] DynamicTablesPkg: Fix ETE _UID Creation
External email: Use caution opening links or attachments
On Wed, Nov 15, 2023 at 16:24:46 +0000, Ashish Singhal via groups.io wrote:
> On Tue, Nov 14, 2023 at 20:19:04 -0700, Ashish Singhal wrote:
> > Just like CPU _UID, ETE UID also needs to be unique so
> > use AcpiProcessorUid instead of CpuName
> >
> > Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> > ---
> > .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> > index 8228c7845a..724f33c660 100644
> > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> > @@ -359,6 +359,7 @@ CreateAmlCpcNode (
> >
> > @param [in] Generator The SSDT Cpu Topology generator.
> > @param [in] ParentNode Parent node to attach the Cpu node to.
> > + @param [in] GicCInfo CM_ARM_GICC_INFO object used to create the node.
> > @param [in] CpuName Value used to generate the node name.
>
> Can that replace both uses of CpuName in the function (so it can be
> dropped), or does
>
> Status = WriteAslName ('E', CpuName, AslName);
>
> have other requirements?
>
> /
> Leif
>
> Hello Leif,
>
> CPU Name can be more logical, and you may have the same CPU name in
> different clusters for example. _UID however needs to be unique.
Sure, makes sense.
I just dislike functions that take too many arguments, so wanted to
make sure we weren't missing an opportunity to drop one as we were
adding this new one.
Never mind me :)
Thanks,
Leif
No worries at all. Please let me know if you have any other questions before this patch can be accepted.
Thanks
Ashish
> Thanks
> Ashish
>
> > @param [out] EtNodePtr If not NULL, return the created Cpu node.
> >
> > @@ -372,6 +373,7 @@ EFIAPI
> > CreateAmlEtd (
> > IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator,
> > IN AML_NODE_HANDLE ParentNode,
> > + IN CM_ARM_GICC_INFO *GicCInfo,
> > IN UINT32 CpuName,
> > OUT AML_OBJECT_NODE_HANDLE *EtNodePtr OPTIONAL
> > )
> > @@ -397,7 +399,7 @@ CreateAmlEtd (
> >
> > Status = AmlCodeGenNameInteger (
> > "_UID",
> > - CpuName,
> > + GicCInfo->AcpiProcessorUid,
> > EtNode,
> > NULL
> > );
> > @@ -474,6 +476,7 @@ CreateAmlEtNode (
> > Status = CreateAmlEtd (
> > Generator,
> > Node,
> > + GicCInfo,
> > CpuName,
> > NULL
> > );
> > --
> > 2.17.1
> >
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111282): https://edk2.groups.io/g/devel/message/111282
Mute This Topic: https://groups.io/mt/102598848/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.