From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web08.17520.1643539497288599040 for ; Sun, 30 Jan 2022 02:44:57 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=OC7pEMXQ; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 6E7CD6115D for ; Sun, 30 Jan 2022 10:44:56 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CB974C36AE3 for ; Sun, 30 Jan 2022 10:44:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1643539495; bh=o8MwIJc/qdMQxWdyS8HBuzk6BDAhIvKQyMfmmDigjK8=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=OC7pEMXQCtXGKLfAV480DU3Ch6PmQZBlc2q1673RJQXMMiS2Euvk5FUMoPteZ9+uP mimsWlnffZZ7PCAt9VCTO0qD5urZNaXOIeKT9nadoz4iknthP4AuknN2oSc1zghc9E aQK/uDNJaAHlokMACDfkOY4fsTTpbiodP+lrTsEYpLkBOu9UGG9gddqNMEUaCIeyVU mk6elgq5tauxr8k0DsWnWV+awUbyNA009cWns6dHOt36Pp5/aO3l6UPW/OxmLefrjQ nLK1ehj5hMTpW2rzW68XHG3gQlbN8xAEA0cZ721Aygh01IH0ReWw/bKR+sWDwEwTmV QYwxFSxAt7PMQ== Received: by mail-wm1-f48.google.com with SMTP id c2so8118306wml.1 for ; Sun, 30 Jan 2022 02:44:55 -0800 (PST) X-Gm-Message-State: AOAM530yxhbQXOV1mzHAIpaOP44Kr7bYHUvvmuZT6Yw2RPKfehmrAlD9 9DuzQEWYaVu416kVrMVPmGD14alAVIUWbWuNWZM= X-Google-Smtp-Source: ABdhPJxMGB9tyvIv5jdm5NBCnuy0C+P05UZUyNLipPb1xnNGuAgJQWTdX3WmLAbvHRo+aypH+ZvAwMYYREbM3AXjquM= X-Received: by 2002:a05:600c:2b89:: with SMTP id j9mr14172385wmc.190.1643539494137; Sun, 30 Jan 2022 02:44:54 -0800 (PST) MIME-Version: 1.0 References: <20211216034634.15468-1-rebecca@nuviainc.com> <20211216034634.15468-2-rebecca@nuviainc.com> In-Reply-To: <20211216034634.15468-2-rebecca@nuviainc.com> From: "Ard Biesheuvel" Date: Sun, 30 Jan 2022 11:44:42 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 1/4] ArmPkg: Replace CoreId and ClusterId with Mpidr in ARM_CORE_INFO struct To: Rebecca Cran Cc: edk2-devel-groups-io , Ard Biesheuvel , Gerd Hoffmann , Samer El-Haj-Mahmoud , Leif Lindholm , Sami Mujawar Content-Type: text/plain; charset="UTF-8" Hello Rebecca, On Thu, 16 Dec 2021 at 04:46, Rebecca Cran wrote: > > Remove the ClusterId and CoreId fields in the ARM_CORE_INFO structure in > favor of a new Mpidr field. Update code in > ArmPlatformPkg/PrePeiCore/MainMPCore and ArmPlatformPkg/PrePi/MainMPCore.c > to use the new field and call new macros GET_MPIDR_AFF0 and GET_MPIDR_AFF1 > instead. > > Signed-off-by: Rebecca Cran I am going to merge this patch into EDK2 in isolation, along with the edk2-platforms changes to keep things working. For the remainder of the series, there are two issues that I think should be resolved. Apologies for not mentioning this before. 1) Can we make the MP protocol a separate driver? That would be cleaner in terms of breakage of other platforms re MpInitLib, and it would also help with the next point. 2) I don't see any management of coherency between the BSP and the APs (unless I am missing something). I think it would be best to treat APs as non-coherent masters (given that they boot with the MMU disabled), which means that every variable in memory that is used to pass information between BSP and AP needs to be DMA mapped and unmapped appropriately, and follow the ownership rules of DMA mappings. On platforms where none of this is needed, the DmaLib dependency can be satisfied by CoherentDmaLib, whereas other platforms can use the non-coherent version instead, which does all the tedious cache maintenance. Given that library dependencies can be specified per-driver in .DSC file, using a separate driver permits us to pick the right DmaLib without forcing it upon other parts of the code. It should also prevent circular dependency issues for DmaLib implementations that DEPEX on the CPU arch protocol produced by CpuDxe. Thanks, Ard. > --- > ArmPkg/Include/Guid/ArmMpCoreInfo.h | 3 +-- > ArmPkg/Include/Library/ArmLib.h | 4 ++++ > ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c | 8 ++++---- > ArmPlatformPkg/PrePeiCore/MainMPCore.c | 4 +++- > ArmPlatformPkg/PrePi/MainMPCore.c | 4 +++- > 5 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/ArmPkg/Include/Guid/ArmMpCoreInfo.h b/ArmPkg/Include/Guid/ArmMpCoreInfo.h > index 06f9326ca021..43f0848e78b8 100644 > --- a/ArmPkg/Include/Guid/ArmMpCoreInfo.h > +++ b/ArmPkg/Include/Guid/ArmMpCoreInfo.h > @@ -14,8 +14,7 @@ > #define MPIDR_U_BIT_MASK 0x40000000 > > typedef struct { > - UINT32 ClusterId; > - UINT32 CoreId; > + UINT64 Mpidr; > > // MP Core Mailbox > EFI_PHYSICAL_ADDRESS MailboxSetAddress; > diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h > index e4d0476090c7..6566deebdde2 100644 > --- a/ArmPkg/Include/Library/ArmLib.h > +++ b/ArmPkg/Include/Library/ArmLib.h > @@ -111,6 +111,10 @@ typedef enum { > #define GET_CORE_ID(MpId) ((MpId) & ARM_CORE_MASK) > #define GET_CLUSTER_ID(MpId) (((MpId) & ARM_CLUSTER_MASK) >> 8) > #define GET_MPID(ClusterId, CoreId) (((ClusterId) << 8) | (CoreId)) > +#define GET_MPIDR_AFF0(MpId) ((MpId) & ARM_CORE_AFF0) > +#define GET_MPIDR_AFF1(MpId) (((MpId) & ARM_CORE_AFF1) >> 8) > +#define GET_MPIDR_AFF2(MpId) (((MpId) & ARM_CORE_AFF2) >> 16) > +#define GET_MPIDR_AFF3(MpId) (((MpId) & ARM_CORE_AFF3) >> 32) > #define PRIMARY_CORE_ID (PcdGet32(PcdArmPrimaryCore) & ARM_CORE_MASK) > > /** Reads the CCSIDR register for the specified cache. > diff --git a/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c b/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c > index eeab58805e67..852275f44fc3 100644 > --- a/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c > +++ b/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c > @@ -14,7 +14,7 @@ > ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = { > { > // Cluster 0, Core 0 > - 0x0, 0x0, > + 0x0, > > // MP Core MailBox Set/Get/Clear Addresses and Clear Value > (EFI_PHYSICAL_ADDRESS)0, > @@ -24,7 +24,7 @@ ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = { > }, > { > // Cluster 0, Core 1 > - 0x0, 0x1, > + 0x1, > > // MP Core MailBox Set/Get/Clear Addresses and Clear Value > (EFI_PHYSICAL_ADDRESS)0, > @@ -34,7 +34,7 @@ ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = { > }, > { > // Cluster 0, Core 2 > - 0x0, 0x2, > + 0x2, > > // MP Core MailBox Set/Get/Clear Addresses and Clear Value > (EFI_PHYSICAL_ADDRESS)0, > @@ -44,7 +44,7 @@ ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = { > }, > { > // Cluster 0, Core 3 > - 0x0, 0x3, > + 0x3, > > // MP Core MailBox Set/Get/Clear Addresses and Clear Value > (EFI_PHYSICAL_ADDRESS)0, > diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c > index 0b8e5dfb3f30..b5d0d3a6442f 100644 > --- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c > +++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c > @@ -68,7 +68,9 @@ SecondaryMain ( > > // Find the core in the ArmCoreTable > for (Index = 0; Index < ArmCoreCount; Index++) { > - if ((ArmCoreInfoTable[Index].ClusterId == ClusterId) && (ArmCoreInfoTable[Index].CoreId == CoreId)) { > + if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) && > + (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId)) > + { > break; > } > } > diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c > index ce53cea6367c..68a7c13298d0 100644 > --- a/ArmPlatformPkg/PrePi/MainMPCore.c > +++ b/ArmPlatformPkg/PrePi/MainMPCore.c > @@ -67,7 +67,9 @@ SecondaryMain ( > > // Find the core in the ArmCoreTable > for (Index = 0; Index < ArmCoreCount; Index++) { > - if ((ArmCoreInfoTable[Index].ClusterId == ClusterId) && (ArmCoreInfoTable[Index].CoreId == CoreId)) { > + if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) && > + (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId)) > + { > break; > } > } > -- > 2.31.1 >