From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by mx.groups.io with SMTP id smtpd.web11.6764.1598482089394717078 for ; Wed, 26 Aug 2020 15:48:09 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=knnfO61S; spf=pass (domain: nuviainc.com, ip: 209.85.128.65, mailfrom: graeme@nuviainc.com) Received: by mail-wm1-f65.google.com with SMTP id s13so3451546wmh.4 for ; Wed, 26 Aug 2020 15:48:09 -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; bh=/2lmUHvT6O4ev8aTOnG5Olpgo4CwHDGlmcbDnHJHWEw=; b=knnfO61SSCDJs/bflRcadCpf8s0Gjwy2P+HizSOFYPOX4EkiUi8hs/qwKS0IexZ5hq 6Uh1gn9y6JturdE2xA4LRlh138gHfTc7ZVjJEkXW4zLJa8GV3Xk8gkw7ASq2XrTn+vaP U0zOsgg7oxhKsA20CGhHVoWEsHsvMKDaMurTQz/XNY1AkQgXHUldWfoLg+kCZsC7RUsi EbDI4u7hkENzcOKgRWv65b5h74wJh9EABP8ls98RhbY+AzcWH1vxh0jB1t++/Ageo5XX 7YS0VTrtySCKKvF1PW4eL4nXi5ybROPkbNSd3EyRSh6L/Jbwh1TtN1B46YB9X/vgTZPK mkBA== 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; bh=/2lmUHvT6O4ev8aTOnG5Olpgo4CwHDGlmcbDnHJHWEw=; b=YfEgcMAp/qemH+WSNHC3TfxzsHg+4JVMlKlhPPKaH5Gr1hcQXIWFc+xYqZ3FClnprH idcaf/nKOH9FYTOYI8sww6vfR/FNC6Iyc08T9H+kfSlsUo+/UX5LP1AElMFc9bX77w1j YGH5QnD2712kQLQgAumHff+4ewunDmg1TiJ/VbnAK0C0LoSAZxXn7qj5S7axaFet+lE1 kSYf0W58kfhTRxfXpCHBE35OChw3N8Q7n+f9V7B40zn9f7yaexWPU2srzpcuuVEW1w6W 8toj4dyNiufby5v1UfzZl00gkvIRHSK0kPqQ2GWvmVAv7C/o3vV38hfnVoI75PflQ5Jr 3lJQ== X-Gm-Message-State: AOAM531lFyxN+SuCZke+4/qgE/dQRt+2PXrFHPdvo3yyBf6x99zicpwH qQEwLzH6lJYWfwlY6xn0LpOb3A== X-Google-Smtp-Source: ABdhPJzC1bl1Ve2BEyWBxKM4hKnuLKFJyz3E0ccfy3f6uhS7aig0EGo6Sh1FHEuczKXzYjTmewmddg== X-Received: by 2002:a1c:1bc2:: with SMTP id b185mr9785966wmb.168.1598482087776; Wed, 26 Aug 2020 15:48:07 -0700 (PDT) Return-Path: Received: from xora-monster ([2a02:8010:64d6::1d89]) by smtp.gmail.com with ESMTPSA id h14sm570428wml.30.2020.08.26.15.48.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 26 Aug 2020 15:48:07 -0700 (PDT) Date: Wed, 26 Aug 2020 23:48:05 +0100 From: "Graeme Gregory" To: Tanmay Jagdale Cc: leif@nuviainc.com, devel@edk2.groups.io, shashi.mallela@linaro.org Subject: Re: [PATCH v3 edk2-platforms 5/8] SbsaQemu: AcpiDxe: Create MADT table at runtime Message-ID: <20200826224805.nd3e6z7nekvkucjx@xora-monster> References: <20200825133958.17372-1-tanmay.jagdale@linaro.org> <20200825133958.17372-6-tanmay.jagdale@linaro.org> <20200826153522.pmsodj7a5r4pz6jf@xora-monster> MIME-Version: 1.0 In-Reply-To: <20200826153522.pmsodj7a5r4pz6jf@xora-monster> Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Aug 26, 2020 at 04:35:22PM +0100, Graeme Gregory wrote: > On Tue, Aug 25, 2020 at 07:09:55PM +0530, Tanmay Jagdale wrote: > > - Add support to create MADT table at runtime. > > - Included a macro for GIC Redistributor structure initialisation. > > > > Signed-off-by: Tanmay Jagdale > > --- > > Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf | 20 ++- > > Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h | 15 ++ > > Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c | 156 ++++++++++++++++++++ > > 3 files changed, 190 insertions(+), 1 deletion(-) > > > > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > > index 3795a7e11639..8125e8ba7553 100644 > > --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf > > @@ -41,9 +41,27 @@ [LibraryClasses] > > UefiRuntimeServicesTableLib > > > > [Pcd] > > + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile > > gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount > > gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdClusterCount > > gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress > > > > [Depex] > > - TRUE > > + gEfiAcpiTableProtocolGuid ## CONSUMES > > + > > +[Guids] > > + gEdkiiPlatformHasAcpiGuid > > + > > +[Protocols] > > + gEfiAcpiTableProtocolGuid ## CONSUMES > > + > > +[FixedPcd] > > + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision > > + gArmTokenSpaceGuid.PcdGicDistributorBase > > + gArmTokenSpaceGuid.PcdGicRedistributorsBase > > + > > + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorId > > + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision > > + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId > > + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId > > + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision > > diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h > > index eac195b0585c..7a9a0061675f 100644 > > --- a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h > > +++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h > > @@ -22,6 +22,21 @@ > > FixedPcdGet32 (PcdAcpiDefaultCreatorRevision)/* UINT32 CreatorRevision */ \ > > } > > > > +// Defines for MADT > > +#define SBSAQEMU_MADT_GIC_VBASE 0x2c020000 > > +#define SBSAQEMU_MADT_GIC_HBASE 0x2c010000 > > +#define SBSAQEMU_MADT_GIC_PMU_IRQ 23 > > +#define SBSAQEMU_MADT_GICR_SIZE 0x4000000 > > + > > +// Macro for MADT GIC Redistributor Structure > > +#define SBSAQEMU_MADT_GICR_INIT() { \ > > + EFI_ACPI_6_0_GICR, /* Type */ \ > > + sizeof (EFI_ACPI_6_0_GICR_STRUCTURE), /* Length */ \ > > + EFI_ACPI_RESERVED_WORD, /* Reserved */ \ > > + FixedPcdGet32 (PcdGicRedistributorsBase), /* DiscoveryRangeBaseAddress */ \ > > + SBSAQEMU_MADT_GICR_SIZE /* DiscoveryRangeLength */ \ > > + } > > + > > #define SBSAQEMU_UART0_BASE 0x60000000 > > > > #define SBSAQEMU_PCI_SEG0_CONFIG_BASE 0xf0000000 > > diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c > > index 75abdae3b8ce..16cb4e904e6f 100644 > > --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c > > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c > > @@ -6,11 +6,17 @@ > > * SPDX-License-Identifier: BSD-2-Clause-Patent > > * > > **/ > > +#include > > +#include > > +#include > > +#include > > #include > > +#include > > #include > > #include > > #include > > #include > > +#include > > #include > > #include > > > > @@ -61,6 +67,137 @@ CountCpusFromFdt ( > > ASSERT_RETURN_ERROR (PcdStatus); > > } > > > > +/* > > + * A Function to Compute the ACPI Table Checksum > > + */ > > +VOID > > +AcpiPlatformChecksum ( > > + IN UINT8 *Buffer, > > + IN UINTN Size > > + ) > > +{ > > + UINTN ChecksumOffset; > > + > > + ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER, Checksum); > > + > > + // Set checksum field to 0 since it is used as part of the calculation > > + Buffer[ChecksumOffset] = 0; > > + > > + Buffer[ChecksumOffset] = CalculateCheckSum8(Buffer, Size); > > +} > > + > > +/* > > + * A function that add the MADT ACPI table. > > + IN EFI_ACPI_COMMON_HEADER *CurrentTable > > + */ > > +EFI_STATUS > > +AddMadtTable ( > > + IN EFI_ACPI_TABLE_PROTOCOL *AcpiTable > > + ) > > +{ > > + EFI_STATUS Status; > > + UINTN TableHandle; > > + UINT32 TableSize; > > + EFI_PHYSICAL_ADDRESS PageAddress; > > + UINT8 *New; > > + UINT32 NumCores; > > + > > + // Initialize MADT ACPI Header > > + EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER Header = { > > + SBSAQEMU_ACPI_HEADER (EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, > > + EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER, > > + EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION), > > + 0, 0 }; > > + > > + // Initialize GICC Structure > > + EFI_ACPI_6_0_GIC_STRUCTURE Gicc = EFI_ACPI_6_0_GICC_STRUCTURE_INIT ( > > + 0, /* GicID */ > > + 0, /* AcpiCpuUid */ > > + 0, /* Mpidr */ > > + EFI_ACPI_6_0_GIC_ENABLED, /* Flags */ > > + SBSAQEMU_MADT_GIC_PMU_IRQ, /* PMU Irq */ > > + FixedPcdGet32 (PcdGicDistributorBase), /* PhysicalBaseAddress */ > > + SBSAQEMU_MADT_GIC_VBASE, /* GicVBase */ > > + SBSAQEMU_MADT_GIC_HBASE, /* GicHBase */ > > + 25, /* GsivId */ > > + 0, /* GicRBase */ > > + 0 /* Efficiency */ > > + ); > > + > > + // Initialize GIC Distributor Structure > > + EFI_ACPI_6_0_GIC_DISTRIBUTOR_STRUCTURE Gicd = > > + EFI_ACPI_6_0_GIC_DISTRIBUTOR_INIT ( > > + 0, > > + FixedPcdGet32 (PcdGicDistributorBase), > > + 0, > > + 3 /* GicVersion */ > > + ); > > + > > + // Initialize GIC Redistributor Structure > > + EFI_ACPI_6_0_GICR_STRUCTURE Gicr = SBSAQEMU_MADT_GICR_INIT(); > > + > > + // Get CoreCount which was determined eariler after parsing device tree > > + NumCores = PcdGet32 (PcdCoreCount); > > + > > + // Calculate the new table size based on the number of cores > > + TableSize = sizeof (EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER) + > > + (sizeof (EFI_ACPI_6_0_GIC_STRUCTURE) * NumCores) + > > + sizeof (EFI_ACPI_6_0_GIC_DISTRIBUTOR_STRUCTURE) + > > + sizeof (EFI_ACPI_6_0_GICR_STRUCTURE); > > + > > + Status = gBS->AllocatePages ( > > + AllocateAnyPages, > > + EfiACPIReclaimMemory, > > + EFI_SIZE_TO_PAGES (TableSize), > > + &PageAddress > > + ); > > + if (EFI_ERROR(Status)) { > > + DEBUG ((DEBUG_ERROR, "Failed to allocate pages for MADT table\n")); > > + return EFI_OUT_OF_RESOURCES; > > + } > > + > > + New = (UINT8 *)(UINTN) PageAddress; > > + ZeroMem (New, TableSize); > > + > > + // Add the ACPI Description table header > > + CopyMem (New, &Header, sizeof (EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER)); > > + ((EFI_ACPI_DESCRIPTION_HEADER*) New)->Length = TableSize; > > + New += sizeof (EFI_ACPI_6_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER); > > + > > + // Add new GICC structures for the Cores > > + for (NumCores = 0; NumCores < PcdGet32 (PcdCoreCount); NumCores++) { > > + EFI_ACPI_6_0_GIC_STRUCTURE *GiccPtr; > > + > > + CopyMem (New, &Gicc, sizeof (EFI_ACPI_6_0_GIC_STRUCTURE)); > > + GiccPtr = (EFI_ACPI_6_0_GIC_STRUCTURE *) New; > > + GiccPtr->AcpiProcessorUid = NumCores; > > + GiccPtr->MPIDR = NumCores; > > This does not seem to be quite correct, if I dump the MPIDRs from ARM-TF when > booting with 12 cpus this is what I get. > > NOTICE: MPIDR 0 > NOTICE: MPIDR 1 > NOTICE: MPIDR 2 > NOTICE: MPIDR 3 > NOTICE: MPIDR 4 > NOTICE: MPIDR 5 > NOTICE: MPIDR 6 > NOTICE: MPIDR 7 > NOTICE: MPIDR 100 > NOTICE: MPIDR 101 > NOTICE: MPIDR 102 > NOTICE: MPIDR 103 > > I think this will make PSCI operations from CPU8 onwards fail. > I can confirm this is wrong I did a quick hack GiccPtr->MPIDR = ((NumCores / 8) << 8) | (NumCores % 8); and I can boot 128 cores (MPIDR also needs fixed in SSDT I think) [ 12.637579] GICv3: CPU127: found redistributor f07 region 0:0x0000000041060000 [ 12.640185] CPU127: Booted secondary processor 0x0000000f07 [0x411fd070] [ 12.676961] smp: Brought up 1 node, 128 CPUs Considering this patch is in the works https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg06223.html which add MPIDR to the DT, i think you will when that is accepted be able to use the MPIDR from there and then you are protected for the CPU topology changing. Thanks Graeme > Thanks > > Graeme > > > > + New += sizeof (EFI_ACPI_6_0_GIC_STRUCTURE); > > + } > > + > > + // GIC Distributor Structure > > + CopyMem (New, &Gicd, sizeof (EFI_ACPI_6_0_GIC_DISTRIBUTOR_STRUCTURE)); > > + New += sizeof (EFI_ACPI_6_0_GIC_DISTRIBUTOR_STRUCTURE); > > + > > + // GIC ReDistributor Structure > > + CopyMem (New, &Gicr, sizeof (EFI_ACPI_6_0_GICR_STRUCTURE)); > > + New += sizeof (EFI_ACPI_6_0_GICR_STRUCTURE); > > + > > + AcpiPlatformChecksum ((UINT8*) PageAddress, TableSize); > > + > > + Status = AcpiTable->InstallAcpiTable ( > > + AcpiTable, > > + (EFI_ACPI_COMMON_HEADER *)PageAddress, > > + TableSize, > > + &TableHandle > > + ); > > + if (EFI_ERROR(Status)) { > > + DEBUG ((DEBUG_ERROR, "Failed to install MADT table\n")); > > + } > > + > > + return Status; > > +} > > + > > EFI_STATUS > > EFIAPI > > InitializeSbsaQemuAcpiDxe ( > > @@ -68,8 +205,27 @@ InitializeSbsaQemuAcpiDxe ( > > IN EFI_SYSTEM_TABLE *SystemTable > > ) > > { > > + EFI_STATUS Status; > > + EFI_ACPI_TABLE_PROTOCOL *AcpiTable; > > + > > // Parse the device tree and get the number of CPUs > > CountCpusFromFdt (); > > > > + // Check if ACPI Table Protocol has been installed > > + Status = gBS->LocateProtocol ( > > + &gEfiAcpiTableProtocolGuid, > > + NULL, > > + (VOID **)&AcpiTable > > + ); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_ERROR, "Failed to locate ACPI Table Protocol\n")); > > + return Status; > > + } > > + > > + Status = AddMadtTable (AcpiTable); > > + if (EFI_ERROR(Status)) { > > + DEBUG ((DEBUG_ERROR, "Failed to add MADT table\n")); > > + } > > + > > return EFI_SUCCESS; > > } > > -- > > 2.28.0 > >