From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by mx.groups.io with SMTP id smtpd.web12.9943.1633604560170701926 for ; Thu, 07 Oct 2021 04:02:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20210112.gappssmtp.com header.s=20210112 header.b=IzrmwBL/; spf=pass (domain: nuviainc.com, ip: 209.85.221.48, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f48.google.com with SMTP id k7so17693190wrd.13 for ; Thu, 07 Oct 2021 04:02:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20210112.gappssmtp.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=KlTAL42Qi0bfbreau/ZUWjiYVaL2iX6GnrRWIl/Oc8I=; b=IzrmwBL/VtZBnlLKGucq6gJXbpEXCmhj/sPx723qYiaM6kZF/04LQUtseh0diMxEM/ SSjyns4/fT9bY5WozTGsa81yLhNax6rGfCsXiw0qWqg7QzGGvyO6+yY88mEgPfkUpMIE g0xreGP0lRvxnqoldET0yMaa92rxieqYyfhWPFG2X6m60IvPPfiHT8Gvw7+yQgsUDYMl bmVsS/8jHZXTEo//krTLlUAFImEI1e0O5KLRdJ+uHQ1b8QwDXSjPSqhwDLhhRcBz0eRf 09zKAF/aCLFvpM/faDrQ/3P2aXeaeUW37ZkhEROxLILyfYrA2AdKn8ygDHem9DTYxq2D yEYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=KlTAL42Qi0bfbreau/ZUWjiYVaL2iX6GnrRWIl/Oc8I=; b=sDcpi/EuUJbPjCKeIpfnSjiXa+Xw7Cc1ZKS7r9MlGuowkc44GyTn0R362Sp5m6jDJS LjauW09RJ6hYRWfsQuofrnv718xm6hSk3gbubxQ2J2SZvQcQOp2ju6dIDasoy3qeSqPg 4ZnxMVp/JAReBrva5ZrbFE3VJmlDwV641w0TUb2cQJi2/ZQvLJOJ9zXAuxUz7pBBXp2d vsPeGyjAsA0pLPQ6hE4fG5tk5LXK4BvSos1cQ2maKv/fQsTzZjsuQvW63pT5ulxl/4NB o8EkxNqFjMGq5vOvZs3iLJi3ItYbYrIH0oUQ5EqAxKwtm80VKoZxyLSj2PWXRxaozFxR j2mw== X-Gm-Message-State: AOAM530+gec647Hn9BNMl8WARYWHs9PLqZoYDizgZkhdiOMbv9O6AjgB uOyqkriEIDgl+hHCv5/ngX0ZPQ== X-Google-Smtp-Source: ABdhPJz6dpkTKQBTgapSEhab03Z1i+7uNBfMFhfk7I+j7KjovR7ZpNobR8ijHY+mEw14Bt06A7Mb0g== X-Received: by 2002:a05:6000:1684:: with SMTP id y4mr4539333wrd.252.1633604558399; Thu, 07 Oct 2021 04:02:38 -0700 (PDT) Return-Path: Received: from leviathan (cpc92314-cmbg19-2-0-cust559.5-4.cable.virginm.net. [82.11.186.48]) by smtp.gmail.com with ESMTPSA id 143sm8728038wma.37.2021.10.07.04.02.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Oct 2021 04:02:38 -0700 (PDT) Date: Thu, 7 Oct 2021 12:02:36 +0100 From: "Leif Lindholm" To: Ard Biesheuvel Cc: Ard Biesheuvel , Sami Mujawar , edk2-devel-groups-io , Rebecca Cran , Gerd Hoffmann , edk2 RFC list Subject: Re: [RFC] [PATCH 0/2] Proposal to add EFI_MP_SERVICES_PROTOCOL support for AARCH64 Message-ID: <20211007110236.5p63sfsxnlrx7tix@leviathan> References: <20210925021752.20678-1-rebecca@nuviainc.com> <20210928111435.poztq4cksagsogbw@leviathan> <20211007094125.soldrzx7wfoa5kyw@leviathan> MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Oct 07, 2021 at 12:03:30 +0200, Ard Biesheuvel wrote: > On Thu, 7 Oct 2021 at 11:41, Leif Lindholm wrote: > > > > Ard/Sami - any comments? > > > > The code changes by itself look fine to me. The only problem I see is > that we cannot run arbitrary code on other cores with the MMU off, > given that in that case, This is a good point, and something we at the very least should point out more explicitly - and perhaps ought to provide a helper library to do - ... > they don't comply with the UEFI AArch64 > platform requirements, most notably the one that says that unaligned > accesses must be permitted. ... but I'll note that EFI_MP_SERVICES_PROTOCOL is one of those incorrectly named protocols defined in PI, not UEFI. > So either we severely constrain the kind of code that we permit to run > on other cores, or we enable the MMU and caches on each core as it > comes out of reset, as well as do any other CPU specific > initialization that we do for the primary core as well. The description for StartupAllAPs() has a note: It is the responsibility of the consumer of the EFI_MP_SERVICES_PROTOCOL.StartupAllAPs() to make sure that the nature of the code that is executed on the BSP and the dispatched APs is well controlled. The MP Services Protocol does not guarantee that the Procedure function is MP-safe. Hence, the tasks that can be run in parallel are limited to certain independent tasks and well-controlled exclusive code. EFI services and protocols may not be called by APs unless otherwise specified. So I think this is actually fine, implementation-wise. *Except* for the SwitchBSP function (where we're currently bailing out anyway). Regards, Leif > > On Tue, Sep 28, 2021 at 12:14:35 +0100, Leif Lindholm wrote: > > > On Fri, Sep 24, 2021 at 20:17:50 -0600, Rebecca Cran wrote: > > > > I'd like to propose adding EFI_MP_SERVICES_PROTOCOL support for > > > > AARCH64 systems. I've attached two patches to implement support for it > > > > in the DXE phase, based on code in EmulatorPkg and UefiCpuPkg. It's added > > > > under ArmPkg for now, but longer term it should probably be moved into > > > > UefiCpuPkg. > > > > > > > > Patch 1/2 is the start of addressing the issue that the Aff0 field of > > > > the MPIDR is no longer guaranteed to be the core, and should be referred > > > > to in a more generic way: for example it could be the thread, with Aff1 > > > > being the core and Aff2 the cluster. Clearly much more work is needed > > > > to fully remove that assumption. > > > > > > Just to add to this: > > > Aff0 was never defined by the architecture to be the "core", it was > > > just the smallest schedulable entity. The intent being that whether > > > you had multiple hardware threads per core or not, you could just use > > > the affinity to determine whether > > > There is also a bit in the MPIDR to indicate whether the core *had* > > > multiple hardware threads. > > > > > > In recent processors (without any change to the architecture), ARM > > > thought it would be beneficial to keep software developers on their > > > toes by starting to use the hyperthreading layout even for processors > > > without hyperthreading support. I.e. Aff0 is always 0 even though MT > > > is 0: > > > https://developer.arm.com/documentation/100798/0301/Register-descriptions/AArch64-system-registers/MPIDR-EL1--Multiprocessor-Affinity-Register--EL1 > > > The justification being that an SoC might contain both processors > > > with and without multiple hardware threads per core. > > > > > > Anyway, the point is that from at least Cortex-A76 onwards, Aff0 no > > > longer maps to CoreId universally, and Aff1 no longer maps to > > > ClusterId, for all non-threaded implementations. > > > So we need to start cleaning up this use. > > > This will possibly break some out-of-tree platforms, but I figure > > > we're far enough from next stable tag for that not to matter too > > > much. > > > > > > > Patch 2/2 implements the EFI_MP_SERVICES_PROTOCOL for DXE in Library/MpInitLib. > > > > > > Worth calling out in the cover letter that this is backed by PSCI. > > > > > > / > > > Leif > > > > > > > CpuDxe initializes the MP support, and as a result gains a dependency on > > > > MpInitLib. ArmVirt.dsc.inc needs updated to add the new library, as will > > > > all AARCH64 platforms. > > > > > > > > I sent out a patch for MdeModulePkg last week to add a corresponding test > > > > application, MpServicesTest (see https://edk2.groups.io/g/devel/message/80878). > > > > > > > > Rebecca Cran (2): > > > > ArmPkg: Replace CoreId and ClusterId with Mpidr in ARM_CORE_INFO > > > > struct > > > > ArmPkg: Add Library/MpInitLib to support EFI_MP_SERVICES_PROTOCOL > > > > > > > > ArmPkg/ArmPkg.dec | 4 + > > > > ArmPkg/ArmPkg.dsc | 4 + > > > > ArmPkg/Drivers/CpuDxe/AArch64/Arch.c | 22 + > > > > ArmPkg/Drivers/CpuDxe/Arm/Arch.c | 22 + > > > > ArmPkg/Drivers/CpuDxe/CpuDxe.c | 2 + > > > > ArmPkg/Drivers/CpuDxe/CpuDxe.h | 10 + > > > > ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 6 + > > > > ArmPkg/Drivers/CpuDxe/CpuMpInit.c | 604 ++++++++ > > > > ArmPkg/Include/Guid/ArmMpCoreInfo.h | 3 +- > > > > ArmPkg/Include/Library/ArmLib.h | 4 + > > > > ArmPkg/Include/Library/MpInitLib.h | 366 +++++ > > > > ArmPkg/Library/MpInitLib/AArch64/MpFuncs.S | 61 + > > > > ArmPkg/Library/MpInitLib/DxeMpInitLib.inf | 53 + > > > > ArmPkg/Library/MpInitLib/DxeMpLib.c | 1498 ++++++++++++++++++++ > > > > ArmPkg/Library/MpInitLib/InternalMpInitLib.h | 358 +++++ > > > > ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c | 8 +- > > > > ArmPlatformPkg/PrePeiCore/MainMPCore.c | 2 +- > > > > ArmPlatformPkg/PrePi/MainMPCore.c | 2 +- > > > > ArmVirtPkg/ArmVirt.dsc.inc | 3 + > > > > 19 files changed, 3024 insertions(+), 8 deletions(-) > > > > create mode 100644 ArmPkg/Drivers/CpuDxe/AArch64/Arch.c > > > > create mode 100644 ArmPkg/Drivers/CpuDxe/Arm/Arch.c > > > > create mode 100644 ArmPkg/Drivers/CpuDxe/CpuMpInit.c > > > > create mode 100644 ArmPkg/Include/Library/MpInitLib.h > > > > create mode 100644 ArmPkg/Library/MpInitLib/AArch64/MpFuncs.S > > > > create mode 100644 ArmPkg/Library/MpInitLib/DxeMpInitLib.inf > > > > create mode 100644 ArmPkg/Library/MpInitLib/DxeMpLib.c > > > > create mode 100644 ArmPkg/Library/MpInitLib/InternalMpInitLib.h > > > > > > > > -- > > > > 2.31.1 > > > >