From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb1-f174.google.com (mail-yb1-f174.google.com [209.85.219.174]) by mx.groups.io with SMTP id smtpd.web08.1065.1615246461416944403 for ; Mon, 08 Mar 2021 15:34:21 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=hEwccVs1; spf=pass (domain: linaro.org, ip: 209.85.219.174, mailfrom: ilias.apalodimas@linaro.org) Received: by mail-yb1-f174.google.com with SMTP id b10so12014410ybn.3 for ; Mon, 08 Mar 2021 15:34:21 -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=i2UxV5iYToRvTdb06a/f7gVJTYue8X7Tmq6zy8ULzbg=; b=hEwccVs1MG8qyK2bwtmgPwouk3AbYd/PYUcnffDsP7C1SS4ujVNVP+qfxpxpkkApIZ Wd6YfXWgtYr17hinPTYSMO2jBHRKrtk6B5EX9yzTp/UpQBV9IgW4cdFT22tPpihsz4Qn +XDZ2pF7fzd0CuEP2nyKCZsySIqe+S4zAmGBYA1Tws8HLJ5tj+3Vqg7Tz9DXoE6nVXyp I0BHoxRd9p/InKsGrXC9pXy91pXYRS4g2z30bJy5bLMPcMJcVvoKKagchshdfa7tkD+O vhcQKqwglKurTs6U3hNYqHWUcHZxkwDTWbLHmxgRJG/BMO5gDK9YwQFcXOzg2jn4ucb1 dL/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=i2UxV5iYToRvTdb06a/f7gVJTYue8X7Tmq6zy8ULzbg=; b=RuL+f8HCKOb+6ckWSvP5NQX6mBsZlrMKHE5hZjlX1F+13nEjY93CnkkjoinScqnVVj iI3GT5kjGVD8cxzLMmz+i1/tl71FN/6CNmVy5FV3Ex6V56sRGySQw3nJlbp+kp5CHRAT yERTpbYG6ignhMWOdlRSFdBFr4Fc77eITpSaYK8YAyjdSqH0TMVVzzsezIPWZg/l0hzP 04mfXT/9+i28WDovs/Ut9YBUYnsaRbewdEDHsx8UugOoJ09Rg4nNWD0aV3d5nQiOQKH1 CgZBqtmsomFnB9yKiGM2lhYUO5BK6USQOyMfYuQgtyJ9pr+Lifdx6FvanHAoZTfI5ffj nHTA== X-Gm-Message-State: AOAM532E75xoowAdYfBiqbblC9zPQKLw8wqwZi5U/+SeyDvldLjIvqx9 9uqkbxKeMU5XMZQC8cy3LHU0B8r/lfdOuhk1ecKAq/uY78WGHMmh X-Google-Smtp-Source: ABdhPJwD8YFvuIQDTLw7v9VL+f1Lx0a0xAh4aK5LQ49HzOy4gImvbrNG4N2pcAx3tos2D4KZ3wavg5Vn1NoczIf+KoE= X-Received: by 2002:a5b:8ce:: with SMTP id w14mr34563475ybq.201.1615246460337; Mon, 08 Mar 2021 15:34:20 -0800 (PST) MIME-Version: 1.0 References: <20210212173459.508205-1-ilias.apalodimas@linaro.org> <20210212173459.508205-2-ilias.apalodimas@linaro.org> In-Reply-To: From: "Ilias Apalodimas" Date: Tue, 9 Mar 2021 01:33:44 +0200 Message-ID: Subject: Re: [edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver To: Pierre Cc: devel@edk2.groups.io, Sami Mujawar Content-Type: text/plain; charset="UTF-8" On Fri, Mar 05, 2021 at 05:58:49PM +0000, Pierre wrote: > Hi Ilias, > Here is the rest of the review. Sorry to do it in 2 times. No worries, I'll try to pick up all the comments. > > Regards, > > Pierre > > > > > > +/** > > > > + Fixup the Pcd values for variable storage > > > > + > > > > + Since the upper layers of EDK2 expect a memory mapped interface and > > we can't > > > > + offer that from an RPMB, the driver allocates memory on init and > > passes that > > > > + on the upper layers. Since the memory is dynamically allocated and we > > can't set the > > > > + PCD is StMM context, we need to patch it correctly on each access > > > > + > > > > + @retval EFI_SUCCESS Protocol was found and PCDs patched up > The error codes are missing. Yea, but I'll remove the overflow check on v6 so that should be fine as-is. > > > > + > > > > + **/ > > > > +EFI_STATUS > > > > +EFIAPI > > > > [...] > > + ASSERT_EFI_ERROR (Status); > > > > + > > > > + Instance = INSTANCE_FROM_FVB_THIS (FvbProtocol); > > > > + // The Pcd is user defined, so make sure we don't overflow > > > > + if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32 > > (PcdFlashNvStorageVariableSize)) { > I think this can be removed since the next condition is more strict. ditto > > > > + return EFI_INVALID_PARAMETER; > > > > + } > > > > + > > > > > [...] > > +STATIC > > > > +EFI_STATUS > > > > +ReadWriteRpmb ( > > > > + UINTN SvcAct, > > > > + UINTN Addr, > > > > + UINTN NumBytes, > > > > + UINTN Offset > > > > + ) > > > > +{ > > > > + ARM_SVC_ARGS SvcArgs; > > > > + EFI_STATUS Status; > > > > + > > > > + ZeroMem (&SvcArgs, sizeof (SvcArgs)); > > > > + > > > > + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64; > > If this is an FFA call, is it possible to: > - put a reference in the header to the spec (it should be similar to the > one at > edk2/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c) > - check the return status of the SVC call against the ones available at > edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h > - if possible, remove the dependency to > The call is technically an FFA one but at the moment OP-TEE returns the StMM return code which is defined in the last header you mention. The relevant code is in ./core/arch/arm/kernel/stmm_sp.c function tee2stmm_ret_val(). So unless we redefine that in OP-TEE or (better imho), wait for a full FFA mechanism to be in place, I'd prefer leaving it as is. Keep in mind that adding the full FFA will also get rid of the hardcoded IDs on the beginning of the file. > > > > + SvcArgs.Arg1 = mStorageId; > > + // [...] > > > > + if ( (FwVolHeader->Revision != EFI_FVH_REVISION) > > > > + || (FwVolHeader->Signature != EFI_FVH_SIGNATURE) > > > > + || (FwVolHeader->FvLength != FvLength) > > > > + ) > could be on the same line -> ') {' ok > > > > + { > > > > > > + if (VariableStoreHeader->Size != VariableStoreLength) { > > > > + DEBUG ((DEBUG_INFO, "%a: Variable Store Length does not match\n", > > > > + __FUNCTION__)); > > > > + return EFI_VOLUME_CORRUPTED; > > > > + } > > > > + > > > > + return EFI_SUCCESS; > > > empty line, could be removed ok > > + > > > > + (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) + > > > > + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) == > > > > + PcdGet64 (PcdFlashNvStorageFtwSpareBase64)); > > > > + > > > > + // Check if the size of the area is at least one block size > > > > + ASSERT ( > > > > + (PcdGet32 (PcdFlashNvStorageVariableSize) > 0) && > I think the first check (Size > 0) is redundant with the second one (Size > > BlockSize). Yea it seems so. This was again a c/p from other drivers handling the PCD, but we can start clean here. > > > > + (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockSize > 0) > > > > + ); > > > > + ASSERT ( > > > > + (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) && [...] > > + > > > > + SetMem (&mInstance, sizeof (mInstance), 0); > NIT: you can use ZeroMem() Sure > > > > + > > > > + mInstance.FvbProtocol.GetPhysicalAddress = > > OpTeeRpmbFvbGetPhysicalAddress; > > > > + mInstance.FvbProtocol.GetAttributes = OpTeeRpmbFvbGetAttributes; > > > > + mInstance.FvbProtocol.SetAttributes = OpTeeRpmbFvbSetAttributes; > > > > + mInstance.FvbProtocol.GetBlockSize = OpTeeRpmbFvbGetBlockSize; > > > > + mInstance.FvbProtocol.EraseBlocks = OpTeeRpmbFvbErase; > > > > + mInstance.FvbProtocol.Write = OpTeeRpmbFvbWrite; > > > > + mInstance.FvbProtocol.Read = OpTeeRpmbFvbRead; > > > > + > > > > + mInstance.MemBaseAddress = (EFI_PHYSICAL_ADDRESS)Addr; > > > > + mInstance.Signature = FLASH_SIGNATURE; > > > > + mInstance.Initialize = FvbInitialize; > > > > + mInstance.BlockSize = EFI_PAGE_SIZE; > > > > + mInstance.NBlocks = NBlocks; > > > > + > > > > + // The Pcd is user defined, so make sure we don't overflow > > > > + if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32 > > (PcdFlashNvStorageVariableSize)) { > > > I don't think this is necessary to do any check here since the memory has > just been allocated with the right size. Yea it's not. I'll probably remove the same piece of code from the FixupPcd.c as well. > > > > + ); > Do we actually need to set these PCDs ? If the PCDs are persistent, we > should not need to patch them in FixupPcd.c. FvbInitialize() is using them, > but it is not called from OpTeeRpmbFvbInit(). > > I think it is. There's something 'special' about this driver. The upper EDK2 lays expect a byte-addressable interface. So the PCDs are definitely not persistent, they are patchable in module PCDs. Since RPMB isn't one we are allocating memory and handing that memory over to EDK2, while syncing the background on the hardware. Again, it's been a while since I wrote this and memory might be failing, but I think what's happening here is: OpTeeRpmbFvbInit is the entry point, which calls in EDK2 code, so you need to patch them in here, before calling anything. After that the fixup code runs before calling into the module and fixes up the values for us. In any case I'll double check before posting a v6 Thanks /Ilias [...]