From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by mx.groups.io with SMTP id smtpd.web11.11308.1598354118634558274 for ; Tue, 25 Aug 2020 04:15:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=FzRkagFZ; spf=pass (domain: nuviainc.com, ip: 209.85.221.66, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f66.google.com with SMTP id a5so12321946wrm.6 for ; Tue, 25 Aug 2020 04:15:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=tDi6aNWP4o3KbZnteT2U/3o5dOx64O7Uq5mwp/5JL94=; b=FzRkagFZzdNw2E2HLjVdEZtsb+tj14mLTs2P2y76RUyIGQOCQ8Hq5Pb4Zlcvti52Ho we1kugp33l5gzuVNBVRR6plUwrzJEPZPokEgY3bp6iutSePkcJJ6ertQCIRaKlD+6Nnj tq3/IQN6ACxdun6dmoidBI/Dy77PlmzBhV3oI/IOVlSp4NBp5P2AHkjGv5lcvQCQb59D 3KA43MXZBYbVcCpd45en47hPXomWmoJiqZfl+MWGiF82e7at1nETCZNqR8Yl7X7Vx87B 3VD7EbXQi5oTxuNV5F3A8C4VZ12R6KQLV7DIUEI1JBVh6R7EVka6V0JuGe5DroCBj/nB Y+8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=tDi6aNWP4o3KbZnteT2U/3o5dOx64O7Uq5mwp/5JL94=; b=HjxBI+KlhWXjQZW5l2P9YqmpdPvvR9nO+EG31wuZFbg5vQ8R+NYLWs+xM+xWOJk7Lf Kc4y+p5DBkzrzCQ4EbeKtcJNZjcq0UoRs9QmMISTxMCAYZbTLVACErBAdyLkkMGwlP5P XI4Rirax/6yveaWBtYL0AincxR88xM7kb83nq0ZGQMs2QqF9nSzox9Gsx+/0eire7dmB CythDr9xKS+KZmhAeMG9Eic/F06ZZUATWfuEF3KGzwL2DXJMAuEatOuUnG+TyFIcNKUI I7dQHqP8Q4cDis2BhWvcTbRSIUsL+H/pQEORWszbFdVeJWWLbmDiz83yDjwsuuiFPzRJ lVtg== X-Gm-Message-State: AOAM533LCW6K3+Eq9rZXfFq5SmtgR6P5syuobGdMKbkEW6Y/hQs9jRDx u33OaP7q/TXpzZpB0bLxCeaDSQ== X-Google-Smtp-Source: ABdhPJwe2mjxpoQcm5vxmzSA5r0z3rScaK1KyTgZQ70wVhdEKs0jk0XZGTLZRqNwjzQTM/okcJwZuA== X-Received: by 2002:adf:d847:: with SMTP id k7mr9762680wrl.22.1598354117001; Tue, 25 Aug 2020 04:15:17 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id q6sm5008848wmq.19.2020.08.25.04.15.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Aug 2020 04:15:16 -0700 (PDT) Date: Tue, 25 Aug 2020 12:15:14 +0100 From: "Leif Lindholm" To: Tanmay Jagdale Cc: graeme@nuviainc.com, shashi.mallela@linaro.org, devel@edk2.groups.io, paul.isaacs@linaro.org, tanmay@marvell.com Subject: Re: [PATCH v2 edk2-platforms 4/8] SbsaQemu: Add new ACPI driver and FDT parser to count CPUs Message-ID: <20200825111514.GO1191@vanye> References: <20200825072322.10848-1-tanmay.jagdale@linaro.org> <20200825072322.10848-5-tanmay.jagdale@linaro.org> MIME-Version: 1.0 In-Reply-To: <20200825072322.10848-5-tanmay.jagdale@linaro.org> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline While debugging why we always ended up with 4 PPTT cpu entries when not explicitly specifying a -smp option on the qemu command line (spoiler alert: because 4 is the default number of cores in sbsa-ref), I spotted another thing: On Tue, Aug 25, 2020 at 12:53:18 +0530, Tanmay Jagdale wrote: > - Add a new ACPI driver for the SbsaQemu platform which would > handle any modifications needed for the ACPI tables. > > - Add a parser function in this driver which parses the FDT created > by Qemu to determine the number of CPUs and hence update the > PcdCoreCount variable. > > Signed-off-by: Tanmay Jagdale > --- > Silicon/Qemu/SbsaQemu/Acpi.dsc.inc | 1 + > Platform/Qemu/SbsaQemu/SbsaQemu.fdf | 1 + > Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf | 49 +++++++++++++ > Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c | 76 ++++++++++++++++++++ > 4 files changed, 127 insertions(+) > > diff --git a/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc b/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc > index c4a8d7a27b78..593670383750 100644 > --- a/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc > +++ b/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc > @@ -33,3 +33,4 @@ [Components.common] > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf > MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatformDxe.inf > Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf > + Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf > index 4526eaaa02c5..3bcf0bf0040a 100644 > --- a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf > +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf > @@ -232,6 +232,7 @@ [FV.FvMain] > # > INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf > INF MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatformDxe.inf > + INF Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > INF RuleOverride = ACPITABLE Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf > INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf > > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > new file mode 100644 > index 000000000000..3795a7e11639 > --- /dev/null > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > @@ -0,0 +1,49 @@ > +## @file > +# This driver modifies ACPI tables for the Qemu SBSA platform > +# > +# Copyright (c) 2020, Linaro Ltd. All rights reserved. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +## > + > +[Defines] > + INF_VERSION = 0x0001001d > + BASE_NAME = SbsaQemuAcpiDxe > + FILE_GUID = 6c592dc9-76c8-474f-93b2-bf1e8f15ae35 > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + > + ENTRY_POINT = InitializeSbsaQemuAcpiDxe > + > +[Sources] > + SbsaQemuAcpiDxe.c > + > +[Packages] > + ArmPkg/ArmPkg.dec > + ArmPlatformPkg/ArmPlatformPkg.dec > + ArmVirtPkg/ArmVirtPkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + MdeModulePkg/MdeModulePkg.dec > + MdePkg/MdePkg.dec > + Silicon/Qemu/SbsaQemu/SbsaQemu.dec > + > +[LibraryClasses] > + ArmLib > + BaseMemoryLib > + BaseLib > + DebugLib > + DxeServicesLib > + FdtLib > + PcdLib > + UefiDriverEntryPoint > + UefiLib > + UefiRuntimeServicesTableLib > + > +[Pcd] > + gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount > + gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdClusterCount > + gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress > + > +[Depex] > + TRUE > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c > new file mode 100644 > index 000000000000..d0d413891e81 > --- /dev/null > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c > @@ -0,0 +1,76 @@ > +/** @file > +* This file is an ACPI driver for the Qemu SBSA platform. > +* > +* Copyright (c) 2020, Linaro Ltd. All rights reserved. > +* > +* SPDX-License-Identifier: BSD-2-Clause-Patent > +* > +**/ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* > + * A function that walks through the Device Tree created > + * by Qemu and counts the number of CPUs present in it. > + */ > +STATIC > +VOID > +CountCpusFromFdt ( > + VOID > +) > +{ > + VOID *DeviceTreeBase; > + INT32 Node, Prev; > + RETURN_STATUS PcdStatus; > + INT32 CpuNode; > + INT32 CpuCount; > + > + DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeBaseAddress); > + ASSERT (DeviceTreeBase != NULL); > + > + // Make sure we have a valid device tree blob > + ASSERT (fdt_check_header (DeviceTreeBase) == 0); > + > + CpuNode = fdt_path_offset (DeviceTreeBase, "/cpus"); > + if (CpuNode <= 0) { > + DEBUG((EFI_D_ERROR, "Unable to locate /cpus in device tree\n")); > + return; > + } > + > + CpuCount = 0; > + > + // Walk through /cpus node and count the number of subnodes. > + // The count of these subnodes corresponds to the number of > + // CPUs created by Qemu. > + Prev = fdt_first_subnode (DeviceTreeBase, CpuNode); > + while (1) { while(1) always happens > + CpuCount++; so CpuCount always happens at least once > + Node = fdt_next_subnode (DeviceTreeBase, Prev); > + if (Node < 0) { > + break; > + } > + Prev = Node; > + } > + ASSERT (CpuCount > 0); so if we believe our CPU is able to do arithmetic, this ASSERT is pointless. Please drop that and spin a v3. While you're at that, please also run PatchCheck.py on all of the commits and fix the errors it reports for 4 out of 8 of them. / Leif > + > + PcdStatus = PcdSet32S (PcdCoreCount, CpuCount); > + ASSERT_RETURN_ERROR (PcdStatus); > +} > + > +EFI_STATUS > +EFIAPI > +InitializeSbsaQemuAcpiDxe ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + // Parse the device tree and get the number of CPUs > + CountCpusFromFdt (); > + > + return EFI_SUCCESS; > +} > -- > 2.28.0 >