From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web10.15312.1598521311870476665 for ; Thu, 27 Aug 2020 02:41:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=F/c+ulGH; spf=pass (domain: nuviainc.com, ip: 209.85.221.67, mailfrom: graeme@nuviainc.com) Received: by mail-wr1-f67.google.com with SMTP id y3so4708698wrl.4 for ; Thu, 27 Aug 2020 02:41:51 -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:content-transfer-encoding:in-reply-to; bh=q1da7YN3VE1AfJPClfvGXe938USj8nbAFCNu4sf34zI=; b=F/c+ulGHbXRyY2kkZ57U5dnx7HdqLyFQ0eFiTo4MGnpcMdb+ki0A2q/bSa8q/Am0OP nqp7U1sxQFqcRa1Ax3HxplPXJtPGf9wXNR7mzVwXUvGrQQ4O0yGsYV9ynm/diDTPrG6n S/BqxV9rkT9qmFaZ+O6LontdF0iMwUa/1Zs5Eiv/q55lfPAYumQCf7/fN/psYkvYQXEb AwF+wz5TazSZNSghdLSF7z2bLcrHALw3FB0V95PkrB/+IFeHkFC87brzT+EeBW1gmaKf I8ikmYGmxtcbyXSwTwpC1oK67RNMPir7bW8qDxgcJwP7rbApc4rlxzmQEYUnF1kEjOhH Ff/Q== 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:content-transfer-encoding :in-reply-to; bh=q1da7YN3VE1AfJPClfvGXe938USj8nbAFCNu4sf34zI=; b=dMUCYMPf3lk2x9O1wt6sDhwiuegLpYHcQtiy0Wp9kR8G0C5ziqe514b+zAOFruFWcU OCPuiDBSccjHzNjwvpnXj5ZgpkxaSV9BZENhUWOGrVEx8B1Qcrw2vPav33iON3ah3pO9 40qlMicpNBeonV3QK3qbG2xvJeEMHDi+t2wUvGqScRHAfihO97NqRg9EnbjCvP+0mkH/ UX7NlBIyX8oo3/iJe95DUQwjoUja5ZFPlmSZlehbc0lvdBUtj/3bEfVnNwy4QvmAzLwq XGJCtayRWHLGQ02PIgsmdZdCt7j0BLNMPcFIAtJ6aeynLO7kqKbe4j380RPfUG+K0TUu yt+A== X-Gm-Message-State: AOAM531ZbOSlzDzU2uQK1eRb46D+tV9YsR1A1Jn1GTwZ1gLqsEO3d0oO dAGUR0rAz+ldHFHveykrCiMI/w== X-Google-Smtp-Source: ABdhPJzYrqClFKt0Ax7ch6pQKd7U/hPqA3KqHR9vAfN/rlwB5cdNlEpy0PGOVLo68gcv4rf2KKgJdg== X-Received: by 2002:adf:91e1:: with SMTP id 88mr19197086wri.364.1598521310125; Thu, 27 Aug 2020 02:41:50 -0700 (PDT) Return-Path: Received: from xora-monster ([2a02:8010:64d6::1d89]) by smtp.gmail.com with ESMTPSA id b8sm4782640wrx.76.2020.08.27.02.41.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Aug 2020 02:41:49 -0700 (PDT) Date: Thu, 27 Aug 2020 10:41:47 +0100 From: "Graeme Gregory" To: Tanmay Jagdale Cc: Leif Lindholm , devel@edk2.groups.io, Shashi Mallela Subject: Re: [PATCH v3 edk2-platforms 5/8] SbsaQemu: AcpiDxe: Create MADT table at runtime Message-ID: <20200827094147.jvbcumhjr4r3p2wt@xora-monster> References: <20200825133958.17372-1-tanmay.jagdale@linaro.org> <20200825133958.17372-6-tanmay.jagdale@linaro.org> <20200826153522.pmsodj7a5r4pz6jf@xora-monster> <20200826224805.nd3e6z7nekvkucjx@xora-monster> MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Thu, Aug 27, 2020 at 08:26:21AM +0530, Tanmay Jagdale wrote: > Hi Graeme, > > > On Thu, 27 Aug 2020 at 04:18, Graeme Gregory <[1]graeme@nuviainc.com> wrote: > > 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 <[2]tanmay.jagdale@linaro.org> > > > --- > > >  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); > > > Thanks for the fix. >   > > 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 > > [3]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. > > > So shall I push an interim patch that uses the aforementioned formula > to derive the MPIDR ? Or wait for the qemu patch to get merged and > then read MPIDR directly from there ? > IMHO wait for to see progress on Leif's patch, then issue an update to read the MPIDR from the FDT. Obviously you can start the development/review process for that before patch lands. The formula only works because I happen to know what qemu generates today. This might change with a qemu change in future though. Graeme