From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f53.google.com (mail-wm1-f53.google.com [209.85.128.53]) by mx.groups.io with SMTP id smtpd.web08.9679.1614697322665492808 for ; Tue, 02 Mar 2021 07:02:03 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=Jx1hfIub; spf=pass (domain: linaro.org, ip: 209.85.128.53, mailfrom: marcin.juszkiewicz@linaro.org) Received: by mail-wm1-f53.google.com with SMTP id k66so3076891wmf.1 for ; Tue, 02 Mar 2021 07:02:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:organization:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=uT7rMtXYDf+nkXEA4kxf2YnXKjYWavfXbMbuyUE/urE=; b=Jx1hfIubkcKza0kIMUkmrMgpo11weuxgs65nLpsgfp5WGDCH+HUEHLekRvGtDahpqt E3OsdhEWPwHphXyUA8u8JIxadgGLf1m6QTLp9031hGxpPx3GpcAqsXqTf6nL2ILhSXq7 5XhhJNu16U6FAldjk/N14tPQouOOlwjZxm3bn3lzpnncIedIZMA612uLml6kz7E7hYkV FcIIwAtjMDZs+HIse8PnRCiDSnIpapjKC/45b7k9FQLd+i765Cm6hlTaeMmrm4KE5Xz2 E8YjGtVHVShFKvoZqH1shUVdOytwE5UDXyTdXRgUZEtvWGrOrDbbIOF8kh6dEqLZTgPY THCA== 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:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=uT7rMtXYDf+nkXEA4kxf2YnXKjYWavfXbMbuyUE/urE=; b=MGGJsQB0Jy1FQTOVwaoOo2KCH3vWubAkD95pO9j84c7lk0MihLguZ8JWiHRMRNWTmm tJtQOkFqJxOEgIX/yinJo60trw5wBOQqS/yr4Xu+trL11KVCmh6dFBk/mVs4MTkujVB7 0zMOUOJ9QH4RdG1Df2p1lX7FIqCxp23nwkeOihCBcTIvi+5vL0B1gJsobc813wBbuLkT wRvsBQ6lNrP50pA57YSaHrmhbj9AAC1WRsAYqCF1k/x3GJbmQTxG8ID6r6CmpJwN4FRT fhDHV/ImSpbj/SIfx5ht79DJWFgkmKvtUbBpjmTKPRTbhJSxlAHRjRXfr26qcf935pAr aO/Q== X-Gm-Message-State: AOAM531WjMRJ4ayH+XAfyPpdc/WE2RKx+egsCy7iejK1pLeyUL9VlxpS SBJDe2zI+lElYQ3S3cgaj2yq2w== X-Google-Smtp-Source: ABdhPJxE0ZsJwhNFsRiXVWG83imzppqX49ncZir2fdlNO9MVEU+0mXd6y7lmbYf1oo8gwy90uOP90Q== X-Received: by 2002:a7b:c2aa:: with SMTP id c10mr4408706wmk.101.1614697318859; Tue, 02 Mar 2021 07:01:58 -0800 (PST) Return-Path: Received: from puchatek.local (89-67-26-161.dynamic.chello.pl. [89.67.26.161]) by smtp.gmail.com with ESMTPSA id 3sm16499837wry.72.2021.03.02.07.01.57 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 02 Mar 2021 07:01:58 -0800 (PST) Subject: Re: [PATCH edk2-platforms 1/1] Silicon/Qemu: Move SbsaQemu MPIDR-retrieval function to FdtHelperLib To: Graeme Gregory , Leif Lindholm , devel@edk2.groups.io Cc: Ard Biesheuvel , Radoslaw Biernacki , Tanmay Jagdale , Rebecca Cran References: <20210302133828.19321-1-leif@nuviainc.com> <6ba5503b-9686-3218-f79c-b6c8e1e86fde@nuviainc.com> From: Marcin Juszkiewicz Organization: Linaro Message-ID: <924c33ae-e7c1-0145-c38d-1e8c7181516f@linaro.org> Date: Tue, 2 Mar 2021 16:01:56 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: <6ba5503b-9686-3218-f79c-b6c8e1e86fde@nuviainc.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit W dniu 02.03.2021 o 15:14, Graeme Gregory pisze: > 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 Tested-by: Marcin Juszkiewicz sbsa-acs now finish in seconds like before. > >> --- >>   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; >>     } >> >