From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x22a.google.com (mail-wr0-x22a.google.com [IPv6:2a00:1450:400c:c0c::22a]) (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 7E44F21A143F5 for ; Wed, 14 Jun 2017 08:17:02 -0700 (PDT) Received: by mail-wr0-x22a.google.com with SMTP id q97so4935329wrb.2 for ; Wed, 14 Jun 2017 08:18:18 -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=Of5UMetCdEgyKwe/ZUJ7WF18hvdGWvw6QKwc1Wv9DjM=; b=UcIj0WqNFCFZMTzeoln/SH7n0Okp3pp+RNZkQ6TPGgh6mnllQn+QBgyJxttpKIl5ll p/CiH+QvyMv1rv1Ypj9Zv32SUOY42YMzLut+B6dw9nxWyvBYMXT0jkWcFYEEmqRmeEpE 3g6XfsdmWbf2czVxpwOxqSipQa4Gh4FcqL6Rg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Of5UMetCdEgyKwe/ZUJ7WF18hvdGWvw6QKwc1Wv9DjM=; b=KFORgDwk01qM45a6jH0xsuRoRirBEo9YVv77YppQtKgurSFkeZ/zeGyWIc6DvUNPPD wf/IjiJN/XyFma8DrkKhR5v75Ftjk4k5mllPdKNPwcqx+YLIW1lkz1IpEZe25DfUnEBz VjqE4V9LeF+j5QrKCGAXDXbot1rdxEQ4lBUv+mT1TOyBFbJ09pVypDwJZ44uLr2Jbw6F nBBFO24rdRQfx69wt2tQP85Ufk65yDjdLTrDYFO78DbPlStvtYuVxY9xYJvqwQrzNa7I iPIiuYgIZmPg02DiByPwCYh4P3uCelJ9LQ319mzroSpFN8D2kINjCSdPbOcQzaWNHUUO YJOw== X-Gm-Message-State: AKS2vOzrkVqJJrNTR3HT0GoTDaCcpPYrmTdv5DZL5cv0etTqkuxIYWoB 4E1lIrodLxx7vepI X-Received: by 10.28.13.1 with SMTP id 1mr400394wmn.12.1497453496099; Wed, 14 Jun 2017 08:18:16 -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 y41sm291000wrd.59.2017.06.14.08.18.15 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 14 Jun 2017 08:18:15 -0700 (PDT) Date: Wed, 14 Jun 2017 16:18:13 +0100 From: Leif Lindholm To: Jun Nie Cc: Ard Biesheuvel , Haojian Zhuang , edk2-devel@lists.01.org, Shawn Guo , Jason Liu Message-ID: <20170614151813.GP26676@bivouac.eciton.net> References: <1497232768-8993-1-git-send-email-jun.nie@linaro.org> <20170612155315.GK26676@bivouac.eciton.net> <20170613091859.GO26676@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Jun 2017 15:17:02 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Jun 14, 2017 at 10:50:02AM +0800, Jun Nie wrote: > >> Sorry, just see your email because that thread is not highlighted for > >> new email in gmail for unknown reason. > >> I have concern that "UINT64 VENDOR_SPECIFIC_FIELD[8]" cannot secure > >> the ECSD alignment because it is not the first member. > > > > It being the first member or not has no relevance. > > By my count, it starts at offset 64, with all preceding entries being > > UINT8. > > Maybe you are right. I had have concern that if preceding member > of ECSDData, CSDData, is not 8 bytes aligned in the tail, UINT8 > RESERVED_1[] may start from non 8 bytes aligned address. The alignment of any struct must match the highest alignment requirement of its members. If you add a 64-bit field to ECSDData, that increases the alignment requirements for the ECSDData struct to 64 bits, which increases the alignment requirements for the CARD_INFO struct to 64 bits. The compiler will automatically insert padding as required to maintain this. > >> Changing the first member to "UINT64 RESERVED_1[2]" shall secure the > >> alignment. > > > > Relying on a reserved field for setting alignment constraints risks > > having to find an alternative method at some point in the future, so I > > agree this is not a preferred solution. > > > >> But I preferred Pad method. It is more readable if all ECSD member > >> are UINT8 type. > > > > I confess am not familiar enough with eMMC to make a clean call here, > > but if real alignment constrains exist, using UINT8 is actively > > misleading, not to mention incorrect. If there is no real alignment > > constraint, this is not the code that needs fixing. > > 512 bytes ECSD data may be read either by CPU or DMA via dedicated > eMMC command. DMA may require alignment for the structure and > DMA on different SoC may require different alignment. Hikey DMA is > OK with 4 bytes alignment, while ZTE SoC require 8 bytes alignment. > And pad must not be added in the 512 bytes structure body by compiler. > So all members in ECSD are defined as UINT8 is safe. There is no magic to the padding, it is 100% predictable. So the only safety added is the false sense of not having to consider details of alignment requirements. > >> It is also more clear to add alignment info in > >> CARD_INFO, just before ECSD member. > > > > Why is it more clear to apply alignment constraints at point of use > > instead of point of definition? > > Adding alignment attribute in definition is best way for the > constraint, but I do not know how to make all compilers happy, just > like my version 1 patch. It is more or less obscure to change > VENDOR_SPECIFIC_FIELD or RESERVED_1 to type UINT64. The benefit of inserting UINT64 types is that this is exactly the mechanism the C language provides for resolving this type of issue. But there could be other reasons that would not be preferable. (Say, if the VENDOR_SPECIFIC_FIELD is expected to hold a text string.) > I confess that adding a pad before ECSD usage is compromised method. It > serve as a reminder for developers too, and allow all members of ECSD are > defined with UINT8. > > Another method is to define ECSDData as a pointer and allocate > buffer for it. Which method do you prefer? That would certainly be a much cleaner solution. It would also be more likely to remain usable if we come across future controllers with even higher alignment requirements. It would also let us resolve this issue without me having to fully understand why the Buffer argument to MMC_READBLOCKDATA is explicitly a UINT32 * :) So - yes please, if you could do that change instead, I would be happy to use that. > > Is there any chance you can share the EFI_MMC_HOST_PROTOCOL > > implementation that triggers this error? > > DWMMC controller on ZTE/Sanchip SoC will read corrupted data with current > DwMmc driver in edk2 96boards git repo. I just add several small patches to > enable it on new SoC. I will send all these changes in near future. Or > what detail information do you need? I can share it here. If you could paste the link to the commit(s) on github, that would satisfy my curiosity. Best Regards, Leif > Jun > > > > > / > > Leif > > > >> I do not get point of Andrew, maybe he share the same concern. > >> > >> Jun > >> > >> > > >> >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> >> Signed-off-by: Jun Nie > >> >> --- > >> >> EmbeddedPkg/Universal/MmcDxe/Mmc.h | 1 + > >> >> 1 file changed, 1 insertion(+) > >> >> > >> >> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h > >> >> index 8a7d5a3..6e3ab17 100644 > >> >> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h > >> >> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h > >> >> @@ -319,6 +319,7 @@ typedef struct { > >> >> OCR OCRData; > >> >> CID CIDData; > >> >> CSD CSDData; > >> >> + UINT64 Pad; // For 8 bytes alignment of ECSDData > >> >> ECSD ECSDData; // MMC V4 extended card specific > >> >> } CARD_INFO; > >> >> > >> >> -- > >> >> 1.9.1 > >> >>