From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) by mx.groups.io with SMTP id smtpd.web12.9143.1614694453401255568 for ; Tue, 02 Mar 2021 06:14:13 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=BjOAVAe5; spf=pass (domain: nuviainc.com, ip: 209.85.221.52, mailfrom: graeme@nuviainc.com) Received: by mail-wr1-f52.google.com with SMTP id u14so20036252wri.3 for ; Tue, 02 Mar 2021 06:14:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=V26NFt1o3Mk9t6LCcrkka1nNT+vW6gf+N6IZFEDTSQ4=; b=BjOAVAe58/Gg133jIn+YpsRDD0Dw8E+S/UCSjOaXUXjFq90ImN/QkeZCkcW1F7+U6s xhVq2h5OocEpBzOHHLZBb1WFv0rXLnChSGB2PdWNsFR2dB8teR7UrgZhS3qwaBeZdUyv OJXQDU7zfsFDb7nICRPG4H7Cn9T0V6RM69Kdw5CPgMfLYDGzDIEeSPgiN77gcrwItU2G tVEQUU+f6l+wklFKvBPExGXyggw+lV4hqsBWkYVS6ZVFQfG8YYjGByE0jpRB+u8/9ogk JciTGcPI23BjpQjHHxHbBsRwsxd9pr1OnTh+JM5SHsXsaVT9WBQgCPQQZ1TQuqBZUfOF na8g== 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=V26NFt1o3Mk9t6LCcrkka1nNT+vW6gf+N6IZFEDTSQ4=; b=P97PO7i/vZw551BJqhKa9By4HOxFKFrZlq4P70ihngPsfL5jq14M96XvLP1LKY1YWa r5AUWabXqXUMqgTuEjxSMNeLS3MBfAqcneP1B+FsTmebrEOChoe3f6iv+yPlkIhokrD3 zNRamllIlU2AE+0jBlPoNj13bw5BY8ZyHJg6C0uiMsUpZPzJCMmZ1i6hZKmFLB4nNC9E opIE+DgSbIBRDTH728TWb1yKC8bOxlW93cD+/6vMMIcMLjZziLQOdhGhf3Ay1+Ut8C2C urK3/MBIB8nc0gGO+FmFVF3ZJEHhPV+1jaglXxoLcgckeP58mjRA75qeUqixOcgDM0uN ZWGA== X-Gm-Message-State: AOAM530+6/DpERsuTUBLAlOAhm5bDy/Cog0PIDDKVIUKDgES58SWYCBN 5kVIh7J6Dmqwtjm1I58yBIn8lw== X-Google-Smtp-Source: ABdhPJxZptDU5EHnXQOAQd69gbr504EQy0lYH5A0YQtrRg3iT6hA6Jwk1R13fCQAvPMQ5QKpnORLOw== X-Received: by 2002:a5d:6307:: with SMTP id i7mr7944199wru.305.1614694451919; Tue, 02 Mar 2021 06:14:11 -0800 (PST) Return-Path: Received: from XorA-MBP.local ([2a02:8010:64d6:1446:8072:ce36:eaca:110b]) by smtp.gmail.com with ESMTPSA id n186sm2746084wmn.22.2021.03.02.06.14.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 02 Mar 2021 06:14:11 -0800 (PST) Subject: Re: [PATCH edk2-platforms 1/1] Silicon/Qemu: Move SbsaQemu MPIDR-retrieval function to FdtHelperLib To: Leif Lindholm , devel@edk2.groups.io Cc: Ard Biesheuvel , Radoslaw Biernacki , Tanmay Jagdale , Rebecca Cran , Marcin Juszkiewicz References: <20210302133828.19321-1-leif@nuviainc.com> From: "Graeme Gregory" Message-ID: <6ba5503b-9686-3218-f79c-b6c8e1e86fde@nuviainc.com> Date: Tue, 2 Mar 2021 14:14:10 +0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.16; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <20210302133828.19321-1-leif@nuviainc.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit On 02/03/2021 13:38, Leif Lindholm wrote: > Commit 822634fc1bf1 ("SbsaQemu: Update SbsaQemuAcpiDxe to use FdtHelperLib") > replaced the CountCpusFromFdt() function in SbsaQemuAcpiDxe with a call to > FdtHelperCountCpus() in FdtHelperLib. This ended up leaving static variables > FdtFirstCpuOffset and FdtCpuNodeSize uninitialised, such that the GetMpidr() > function kept returning the value for cpu 0. > > Resolve this by moving the GetMpidr() function over to FdtHelperLib, where > it can again share these variables with FdtHelperCountCpus(). > > Fix up coding style issues as part of copy: > - Add m prefix to module-global variables. > - Add doxygen function comment header. > > Cc: Ard Biesheuvel > Cc: Graeme Gregory > Cc: Radoslaw Biernacki > Cc: Tanmay Jagdale > Cc: Rebecca Cran > Reported-by: Marcin Juszkiewicz > Signed-off-by: Leif Lindholm Tested-By: Graeme Gregory This fixes the issue from inspection of APIC table with acpiview! > --- > Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf | 2 -- > Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.inf | 5 +++ > Silicon/Qemu/SbsaQemu/Include/Library/FdtHelperLib.h | 12 +++++++ > Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c | 35 +------------------ > Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c | 36 ++++++++++++++++++++ > 5 files changed, 54 insertions(+), 36 deletions(-) > > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > index a58ebfaf76d5..c6de685bd2c4 100644 > --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > @@ -35,7 +35,6 @@ [LibraryClasses] > DebugLib > DxeServicesLib > FdtHelperLib > - FdtLib > PcdLib > PrintLib > UefiDriverEntryPoint > @@ -46,7 +45,6 @@ [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile > gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount > gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdClusterCount > - gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress > > [Depex] > gEfiAcpiTableProtocolGuid ## CONSUMES > diff --git a/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.inf b/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.inf > index d84c16f888d1..9c059f3e5851 100644 > --- a/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.inf > +++ b/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.inf > @@ -24,5 +24,10 @@ [Packages] > MdePkg/MdePkg.dec > Silicon/Qemu/SbsaQemu/SbsaQemu.dec > > +[LibraryClasses] > + DebugLib > + FdtLib > + PcdLib > + > [FixedPcd] > gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress > diff --git a/Silicon/Qemu/SbsaQemu/Include/Library/FdtHelperLib.h b/Silicon/Qemu/SbsaQemu/Include/Library/FdtHelperLib.h > index f9045fd5df45..ea9159857215 100644 > --- a/Silicon/Qemu/SbsaQemu/Include/Library/FdtHelperLib.h > +++ b/Silicon/Qemu/SbsaQemu/Include/Library/FdtHelperLib.h > @@ -10,6 +10,18 @@ > #ifndef FDT_HELPER_LIB_ > #define FDT_HELPER_LIB_ > > +/** > + Get MPIDR for a given cpu from device tree passed by Qemu. > + > + @param [in] CpuId Index of cpu to retrieve MPIDR value for. > + > + @retval MPIDR value of CPU at index > +**/ > +UINT64 > +FdtHelperGetMpidr ( > + IN UINTN CpuId > + ); > + > /** Walks through the Device Tree created by Qemu and counts the number > of CPUs present in it. > > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c > index 84120f1c1b51..b8901030ecd0 100644 > --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c > @@ -20,39 +20,6 @@ > #include > #include > #include > -#include > -#include > - > -STATIC INT32 FdtFirstCpuOffset; > -STATIC INT32 FdtCpuNodeSize; > - > -/* > - * Get MPIDR from device tree passed by Qemu > - */ > -STATIC > -UINT64 > -GetMpidr ( > - IN UINTN CpuId > - ) > -{ > - VOID *DeviceTreeBase; > - CONST UINT64 *RegVal; > - INT32 Len; > - > - DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeBaseAddress); > - ASSERT (DeviceTreeBase != NULL); > - > - RegVal = fdt_getprop (DeviceTreeBase, > - FdtFirstCpuOffset + (CpuId * FdtCpuNodeSize), > - "reg", > - &Len); > - if (!RegVal) { > - DEBUG ((DEBUG_ERROR, "Couldn't find reg property for CPU:%d\n", CpuId)); > - return 0; > - } > - > - return (fdt64_to_cpu (ReadUnaligned64 (RegVal))); > -} > > /* > * A Function to Compute the ACPI Table Checksum > @@ -159,7 +126,7 @@ AddMadtTable ( > CopyMem (New, &Gicc, sizeof (EFI_ACPI_6_0_GIC_STRUCTURE)); > GiccPtr = (EFI_ACPI_6_0_GIC_STRUCTURE *) New; > GiccPtr->AcpiProcessorUid = CoreIndex; > - GiccPtr->MPIDR = GetMpidr (CoreIndex); > + GiccPtr->MPIDR = FdtHelperGetMpidr (CoreIndex); > New += sizeof (EFI_ACPI_6_0_GIC_STRUCTURE); > } > > diff --git a/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c b/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c > index bbd756f01a21..7fdfb055db76 100644 > --- a/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c > +++ b/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c > @@ -14,6 +14,40 @@ > #include > #include > > +STATIC INT32 mFdtFirstCpuOffset; > +STATIC INT32 mFdtCpuNodeSize; > + > +/** > + Get MPIDR for a given cpu from device tree passed by Qemu. > + > + @param [in] CpuId Index of cpu to retrieve MPIDR value for. > + > + @retval MPIDR value of CPU at index > +**/ > +UINT64 > +FdtHelperGetMpidr ( > + IN UINTN CpuId > + ) > +{ > + VOID *DeviceTreeBase; > + CONST UINT64 *RegVal; > + INT32 Len; > + > + DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeBaseAddress); > + ASSERT (DeviceTreeBase != NULL); > + > + RegVal = fdt_getprop (DeviceTreeBase, > + mFdtFirstCpuOffset + (CpuId * mFdtCpuNodeSize), > + "reg", > + &Len); > + if (!RegVal) { > + DEBUG ((DEBUG_ERROR, "Couldn't find reg property for CPU:%d\n", CpuId)); > + return 0; > + } > + > + return (fdt64_to_cpu (ReadUnaligned64 (RegVal))); > +} > + > /** Walks through the Device Tree created by Qemu and counts the number > of CPUs present in it. > > @@ -49,12 +83,14 @@ FdtHelperCountCpus ( > // The count of these subnodes corresponds to the number of > // CPUs created by Qemu. > Prev = fdt_first_subnode (DeviceTreeBase, CpuNode); > + mFdtFirstCpuOffset = Prev; > while (1) { > CpuCount++; > Node = fdt_next_subnode (DeviceTreeBase, Prev); > if (Node < 0) { > break; > } > + mFdtCpuNodeSize = Node - Prev; > Prev = Node; > } > >