From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-io1-f53.google.com (mail-io1-f53.google.com [209.85.166.53]) by mx.groups.io with SMTP id smtpd.web11.7303.1639642035064579707 for ; Thu, 16 Dec 2021 00:07:15 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=tNeZF72V; spf=pass (domain: linaro.org, ip: 209.85.166.53, mailfrom: masahisa.kojima@linaro.org) Received: by mail-io1-f53.google.com with SMTP id p23so34077022iod.7 for ; Thu, 16 Dec 2021 00:07:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SahgrwasUnHsivxEo/BJnVzXdColl16FEwOBbBgWpL0=; b=tNeZF72Vk3MXS4GX0Yi+lI0G3H96NeyGrAI6FM5ZHiJSdgxxtwGD/P3yZUrHzhOzTA KyiJfPS1UnIwMvHuGQr8TiMOaYkNOgWvGQF4l31cPMxJ4iX/nIaA6xjQSDHMc7Q3Xo2s E9lIWqcXEshEgmkmz5L2Pd6biFylvRsGBVAf2CXGtIUKGIkUBM/15oNG8AwFORinXcyW wRXlPEk6URoMTInZi9OGcKVSpjd97Yr4wIGpRwDb93V0bEip2AUL++VfQuHkxl+flgMC mrrki5hLkdYMMHyden9Y1tGmHvuRHfhWaibGDL8NGJrmBf3O/ZrR2qwNx+C1dTcwQ7AI 62RA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=SahgrwasUnHsivxEo/BJnVzXdColl16FEwOBbBgWpL0=; b=xAOJVIhZL0/S/FUqDJd+fvqk/fNSsmooUvIp9Px2R5AoEc9VjOyuI8kP5Tn1EC5X3M g/InwdL306PdiCFaq4JJoaq3PXsmhsyL9QB+3xMWZHV70fuKWztV1f2Mb3C0PNQNTXBB PaDW0S05QYox8FRDQeHx3ZxMgBjJI17Rh3crlej2oX+/z4BypnVn/SptPjIrgPFWLcGj 0DQ+ys2vdMiyADcDmj4VQXgnr9lFYfnZbT3bExjeE+wuA8RuHMyvznHA4xYH7PPlkJXd ISswTDWZEZSAuVwfEfiQbm7SGxGXSlIy0A3p86Z5JDCAiqxdR5UrxC8Mn66OX7JtsfTi v/4A== X-Gm-Message-State: AOAM5309EuJKj+b77MLe42lD/WoYEO+emfWwglrHTWvVxKfXwm1XYoDj jmCnoBE0tcuwS2RAg3iqbi7YjkVYKW+wwHIdSJiXiQ== X-Google-Smtp-Source: ABdhPJypeay5PBjJ1NTFmO9uaPaO1qjEU19ECJSy2l7AUCvuNQliE9XDKOfy+J1lqp2tfeGyReZtFu8/xq8aV6oUSGk= X-Received: by 2002:a05:6638:2054:: with SMTP id t20mr8558524jaj.42.1639642034282; Thu, 16 Dec 2021 00:07:14 -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: "Masahisa Kojima" Date: Thu, 16 Dec 2021 17:07:02 +0900 Message-ID: Subject: Re: [PATCH edk2 v1 1/3] StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field To: Ard Biesheuvel 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" 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. 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 > >