From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web08.9291.1639653009026615849 for ; Thu, 16 Dec 2021 03:10:09 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=otCaCrOQ; spf=pass (domain: kernel.org, ip: 145.40.68.75, 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 ams.source.kernel.org (Postfix) with ESMTPS id 7BA01B8239C for ; Thu, 16 Dec 2021 11:10:06 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3CA29C36AE2 for ; Thu, 16 Dec 2021 11:10:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1639653005; bh=XEIA7KdFYfqxJG7dxltN670DU6yaH71d1VY75zDz87U=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=otCaCrOQx3Bsau8i4umYF/sP2HpzmsEnwoQUoYL9ZTvKFXz4qqdakLoMpXiV434DP fxrlrg4YRq1hxo9WeMugqnkCoCBaC0s5kfAV2LHPEBJyXh5cwZKA6gD1Ad55PHBHs5 pV7S3zufTPhUPNFMrDEWJXMUqZJzNgErKoQI6b+hdVkoUFLaLudo0p6IEILSbEQksF U5cp/V06tpj06Nkb6X+Mi1qrejsB07EnTlfDoUbgpyRWkr1HiPlkDDwNDliClLaeoJ Iefm5YTf0jtSC4Aqu9H3fOIRgACORkzNgviCjJi0ty12mIx4FFT5QJN8Lo9SkmjLUB oZTcEvzQ2jsYA== Received: by mail-wr1-f45.google.com with SMTP id t18so43433443wrg.11 for ; Thu, 16 Dec 2021 03:10:05 -0800 (PST) X-Gm-Message-State: AOAM531Ln5WbbhSCfUfW/BtVLYL/HBlp8wSWJ9VNEi4M0+J8DkmXFWdx Ggb++HC5SkPgvi7GBkyjvQkmpx/jxpPQsVqEvHM= X-Google-Smtp-Source: ABdhPJy4OZGsFDM4nYyPNPyHDV+AQyLUIfeSyBwdWkdPKeHPY83vEoUwYFoDNv6Zs/YAEp95NIgp0JwXoqRp+1mbtJA= X-Received: by 2002:adf:9bdb:: with SMTP id e27mr8549496wrc.417.1639653003489; Thu, 16 Dec 2021 03:10:03 -0800 (PST) MIME-Version: 1.0 References: <20211015090623.52511-1-huangming@linux.alibaba.com> <20211015090623.52511-2-huangming@linux.alibaba.com> In-Reply-To: From: "Ard Biesheuvel" Date: Thu, 16 Dec 2021 12:09:51 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH edk2 v1 1/3] StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field To: Masahisa Kojima Cc: Ming Huang , Masami Hiramatsu , edk2-devel-groups-io , Sami Mujawar , Ard Biesheuvel , Jiewen Yao , Supreeth Venkatesh , ming.huang-@outlook.com Content-Type: text/plain; charset="UTF-8" On Thu, 16 Dec 2021 at 09:07, Masahisa Kojima wrote: > > Hi Ard, > > On Wed, 15 Dec 2021 at 17:45, Ard Biesheuvel wrote: > > > > (+ Masahisa, Masami) > > > > > > On Fri, 15 Oct 2021 at 11:07, Ming Huang wrote: > > > > > > TF-A: TrustedFirmware-A > > > SPM: Secure Partition Manager(MM) > > > > > > In TF-A, the name of this field is sp_shared_buf_size. This field is > > > the size of range for transmit data from TF-A to standaloneMM when > > > SPM enable. > > > > > > SpPcpuSharedBufSize is pass from TF-A while StandaloneMM initialize. > > > So, SpPcpuSharedBufSize should be rename to SpSharedBufSize and this field > > > should no multiply by PayloadBootInfo->NumCpus; > > > > > > Signed-off-by: Ming Huang > > > > Could someone please check how this change of interpretation affects > > standalone MM running on SynQuacer? > > In conclusion, this patch does not affect the standalone MM implementation > on SynQuacer. > > In my check, this buffer is used for data sharing between EL3 and Secure-EL1/0. > Currently this buffer is only used to pass the struct spm_mm_boot_info_t > from EL3 to StMM during Stmm initialization, I could not find other use cases. > > TF-A maps the buffer only 64KB, not multiplied by the number of cores. > https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/socionext/synquacer/include/platform_def.h#n98 > So the following definition is something strange and leads to > misunderstanding, but there is no problem. > #define PLAT_SPM_BUF_BASE (BL32_LIMIT - 32 * PLAT_SPM_BUF_SIZE) > Arm platform statically allocates 1MB buffer in TF-A, > so I think it is better to follow the arm platform definition. > > Thanks for confirming > This is different topic, but assertion occurs in DEBUG build if StMM > and SECURE_BOOT_ENABLED=TRUE. > > === > Loading driver at 0x000FABB0000 EntryPoint=0x000FABC447C CapsuleRuntimeDxe.efi > add-symbol-file > /home/ubuntu/src/sbsa-qemu/Build/DeveloperBox/DEBUG_GCC5/AARCH64/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe/DEBUG/SecureBootConfigDxe.dll > 0xFA6BA000 > Loading driver at 0x000FA6B9000 EntryPoint=0x000FA6C9610 SecureBootConfigDxe.efi > add-symbol-file > /home/ubuntu/src/sbsa-qemu/Build/DeveloperBox/DEBUG_GCC5/AARCH64/MdeModulePkg/Universal/BdsDxe/BdsDxe/DEBUG/BdsDxe.dll > 0xFA69D000 > Loading driver at 0x000FA69C000 EntryPoint=0x000FA6A7E98 BdsDxe.efi > DxeCapsuleLibFmp: Failed to lock variable > 39B68C46-F7FB-441B-B6EC-16B0F69821F3 CapsuleMax. Status = Invalid > Parameter > > ASSERT_EFI_ERROR (Status = Invalid Parameter) > ASSERT [BdsDxe] > /home/ubuntu/src/sbsa-qemu/edk2/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleReportLib.c(133): > !(((INTN)(RETURN_STATUS)(Status)) < 0) > === > > Probably it is due to the missing support of VariablePolicy, StMM > outputs the following log output. > === > CommBuffer - 0xFC85CC90, CommBufferSize - 0x52 > !!! DEPRECATED INTERFACE !!! VariableLockRequestToLock() will go away soon! > !!! DEPRECATED INTERFACE !!! Please move to use Variable Policy! > !!! DEPRECATED INTERFACE !!! Variable: > 8BE4DF61-93CA-11D2-AA0D-00E098032B8C PlatformRecovery0000 > === > > That's all for now. > > Thanks, > Masahisa Kojima > > > > > > > > --- > > > StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h | 2 +- > > > StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c | 2 +- > > > StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c | 2 +- > > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h > > > index c44f7066c6..f1683ecb61 100644 > > > --- a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h > > > +++ b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h > > > @@ -41,7 +41,7 @@ typedef struct { > > > UINT64 SpPcpuStackSize; > > > UINT64 SpHeapSize; > > > UINT64 SpNsCommBufSize; > > > - UINT64 SpPcpuSharedBufSize; > > > + UINT64 SpSharedBufSize; > > > UINT32 NumSpMemRegions; > > > UINT32 NumCpus; > > > EFI_SECURE_PARTITION_CPU_INFO *CpuInfo; > > > diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c > > > index 85f8194687..93773c9fe8 100644 > > > --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c > > > +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c > > > @@ -173,7 +173,7 @@ CreateHobListFromBootInfo ( > > > // Base and size of buffer shared with privileged Secure world software > > > MmramRanges[1].PhysicalStart = PayloadBootInfo->SpSharedBufBase; > > > MmramRanges[1].CpuStart = PayloadBootInfo->SpSharedBufBase; > > > - MmramRanges[1].PhysicalSize = PayloadBootInfo->SpPcpuSharedBufSize * PayloadBootInfo->NumCpus; > > > + MmramRanges[1].PhysicalSize = PayloadBootInfo->SpSharedBufSize; > > > MmramRanges[1].RegionState = EFI_CACHEABLE | EFI_ALLOCATED; > > > > > > // Base and size of buffer used for synchronous communication with Normal > > > diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c > > > index 49cf51a789..5db7019dda 100644 > > > --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c > > > +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c > > > @@ -87,7 +87,7 @@ GetAndPrintBootinformation ( > > > DEBUG ((DEBUG_INFO, "SpPcpuStackSize - 0x%x\n", PayloadBootInfo->SpPcpuStackSize)); > > > DEBUG ((DEBUG_INFO, "SpHeapSize - 0x%x\n", PayloadBootInfo->SpHeapSize)); > > > DEBUG ((DEBUG_INFO, "SpNsCommBufSize - 0x%x\n", PayloadBootInfo->SpNsCommBufSize)); > > > - DEBUG ((DEBUG_INFO, "SpPcpuSharedBufSize - 0x%x\n", PayloadBootInfo->SpPcpuSharedBufSize)); > > > + DEBUG ((DEBUG_INFO, "SpSharedBufSize - 0x%x\n", PayloadBootInfo->SpSharedBufSize)); > > > > > > DEBUG ((DEBUG_INFO, "NumCpus - 0x%x\n", PayloadBootInfo->NumCpus)); > > > DEBUG ((DEBUG_INFO, "CpuInfo - 0x%p\n", PayloadBootInfo->CpuInfo)); > > > -- > > > 2.17.1 > > >