From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x229.google.com (mail-wm0-x229.google.com [IPv6:2a00:1450:400c:c09::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E52AC1A1DF9 for ; Fri, 9 Sep 2016 05:34:32 -0700 (PDT) Received: by mail-wm0-x229.google.com with SMTP id b187so28948042wme.1 for ; Fri, 09 Sep 2016 05:34:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Dz4vVMbp0SYY3BhMnFHCMtloRPCK7tBZkHm7HALmvig=; b=OG4ZzjVB5LIlUtoyT6HG7QvOOx9oAasQQek+2/lEQNOwqXSE718CDUQtfTSpvY3mET YQUmF1lUJv0fNpXR84K4kVDR9xd1CZ3e204tJeVhDTLj4WpQLk8U9d2IvtrSznUFenaZ oWSvrsurJui0Gni/+2X2dhXA+l4OknJelnbqs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Dz4vVMbp0SYY3BhMnFHCMtloRPCK7tBZkHm7HALmvig=; b=IvKEa0MZi0YYBQ4nYxTXhaLYhv9KBL5SsLEj3RnBAXgXsHeU69cmfDZmgRghBEwLnn yazqr6I5lJXiPampvZvKGXJ9oxrrwsRcDGr8Yn9tC4vO1q9+uIGPMpfVhwjV723SYe1E nYWsVyEnVO95i/axkNM0zIN02YcK5wzjncgcTaezXEGx5gStClXuPG3PfVrMddpUKwqM pACismlKpbmPDtPrJkeKTbGfxHPveFvyuyC9vl2DNeY6lWg5LURgInglYw3hgdnU8Z9q HZfWt3aXsn5AbbqHRBfphG7uT8HSJTy0kCDc0arEFTNBK2iWyDZB0qaWyJALfbjeMHXV PoVg== X-Gm-Message-State: AE9vXwNX8ubsAcTR4CXmOSrj7Bny/R2ynCu42VwFElNHIDcLGP4N4bSTtpXiys+eMQI7QRYJ X-Received: by 10.194.58.178 with SMTP id s18mr3413613wjq.0.1473424471415; Fri, 09 Sep 2016 05:34:31 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id r4sm3165105wme.21.2016.09.09.05.34.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 09 Sep 2016 05:34:30 -0700 (PDT) Date: Fri, 9 Sep 2016 13:34:29 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org Message-ID: <20160909123429.GP16080@bivouac.eciton.net> References: <1473423502-7061-1-git-send-email-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <1473423502-7061-1-git-send-email-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v2] ArmPlatformPkg/NorFlashDxe: use strictly aligned CopyMem() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Sep 2016 12:34:33 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Sep 09, 2016 at 01:18:22PM +0100, Ard Biesheuvel wrote: > The UEFI spec stipulates that unaligned accesses should be enabled > on CPUs that support them, which means all of them, given that we > no longer support pre-v7 ARM cores, and the AARCH64 bindings mandate > support for unaligned accesses unconditionally. > > This means that one should not assume that CopyMem () is safe to call > on regions that may be mapped using device attributes, which is the > case for the NOR flash. Since we have no control over the mappings when > running under the OS, and given that write accesses require device > mappings, we should not call CopyMem () in the read path either, but > use our own implementation that is guaranteed to take alignment into > account. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel That's a lot less complex, thanks. Reviewed-by: Leif Lindholm > --- > v2: > - simplify the AlignedCopyMem () implementation, given that we do not require > memmove() semantics for copying between NOR flash and memory > - document the above > - add macro to perform alignment check > - drop redundant/unnecessary casts and parentheses > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 63 +++++++++++++++++++- > 1 file changed, 61 insertions(+), 2 deletions(-) > > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > index 3abbe5cb32bc..ca61ac5e1983 100644 > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > @@ -744,6 +744,65 @@ NorFlashWriteBlocks ( > return Status; > } > > +#define BOTH_ALIGNED(a, b, align) ((((UINTN)(a) | (UINTN)(b)) & ((align) - 1)) == 0) > + > +/** > + Copy Length bytes from Source to Destination, using aligned accesses only. > + Note that this implementation uses memcpy() semantics rather then memmove() > + semantics, i.e., SourceBuffer and DestinationBuffer should not overlap. > + > + @param DestinationBuffer The target of the copy request. > + @param SourceBuffer The place to copy from. > + @param Length The number of bytes to copy. > + > + @return Destination > + > +**/ > +STATIC > +VOID * > +AlignedCopyMem ( > + OUT VOID *DestinationBuffer, > + IN CONST VOID *SourceBuffer, > + IN UINTN Length > + ) > +{ > + UINT8 *Destination8; > + CONST UINT8 *Source8; > + UINT32 *Destination32; > + CONST UINT32 *Source32; > + UINT64 *Destination64; > + CONST UINT64 *Source64; > + > + if (BOTH_ALIGNED(DestinationBuffer, SourceBuffer, 8) && Length >= 8) { > + Destination64 = DestinationBuffer; > + Source64 = SourceBuffer; > + while (Length >= 8) { > + *Destination64++ = *Source64++; > + Length -= 8; > + } > + > + Destination8 = (UINT8 *)Destination64; > + Source8 = (CONST UINT8 *)Source64; > + } else if (BOTH_ALIGNED(DestinationBuffer, SourceBuffer, 4) && Length >= 4) { > + Destination32 = DestinationBuffer; > + Source32 = SourceBuffer; > + while (Length >= 4) { > + *Destination32++ = *Source32++; > + Length -= 4; > + } > + > + Destination8 = (UINT8 *)Destination32; > + Source8 = (CONST UINT8 *)Source32; > + } else { > + Destination8 = DestinationBuffer; > + Source8 = SourceBuffer; > + } > + while (Length-- != 0) { > + *Destination8++ = *Source8++; > + } > + return DestinationBuffer; > +} > + > EFI_STATUS > NorFlashReadBlocks ( > IN NOR_FLASH_INSTANCE *Instance, > @@ -791,7 +850,7 @@ NorFlashReadBlocks ( > SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY); > > // Readout the data > - CopyMem(Buffer, (UINTN *)StartAddress, BufferSizeInBytes); > + AlignedCopyMem (Buffer, (VOID *)StartAddress, BufferSizeInBytes); > > return EFI_SUCCESS; > } > @@ -832,7 +891,7 @@ NorFlashRead ( > SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY); > > // Readout the data > - CopyMem (Buffer, (UINTN *)(StartAddress + Offset), BufferSizeInBytes); > + AlignedCopyMem (Buffer, (VOID *)StartAddress + Offset, BufferSizeInBytes); > > return EFI_SUCCESS; > } > -- > 2.7.4 >