From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-x22d.google.com (mail-qt0-x22d.google.com [IPv6:2607:f8b0:400d:c0d::22d]) (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 D88B421A16EC1 for ; Tue, 13 Jun 2017 19:48:48 -0700 (PDT) Received: by mail-qt0-x22d.google.com with SMTP id u12so194875897qth.0 for ; Tue, 13 Jun 2017 19:50:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=sx8JD/sSiuLsGXOv4pYLysm5m2UhZbvcbBJ08b+d2pk=; b=WDRCc0P7YqB6XdbpNN5WgmxklRFG1EOJ+KYmYc6s33Lav9lVfZ5qte6lOtGGNvC07r uBejBQ5FUXPUlmoo0p0l9zm9r4WLl7cDLm2lhZMpi8XDEm6KJ+zYMjs6vgvZL2KTCAys sp946oyekrolzbNGDKVnqx56cJEei82btxBqc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=sx8JD/sSiuLsGXOv4pYLysm5m2UhZbvcbBJ08b+d2pk=; b=MhDeM0wOt832hksCvZZNKbgdoBotuV+NSuCHHujEYy66NRbAXr41Av0+8jaJoDzHuz FOXlo9qx55Ln+cH4Uyw7pqpkKQiucvaTF66SlWbtpgUvYH7k9kel0CiWyzFj2HB/m1bK 1Kuae2qXE8r8qPsiJDjEOYdic/URW/V4PkxtoqxrOZKkQ7YCWllLc6X77mTdqsLWss9f ffex0xnV1iA/xZqDZ0h/wh+6FWWAAkXzPaXQkTNNsFqi4WV8FFIz/5h1KNq1erHdHAQw byPxFbObqx1Zppy13M6CZ8gFZG4vE5lpnBHi1vLD55j1yLQ1Q+T/ekZp+lUg5g0bJMgn z0Tw== X-Gm-Message-State: AKS2vOyKtK4n2zZFn0BrQ+k8WJwQTj+Auq+8rke+ZJPSk8mUCnXQMgoX IMAmbMBvq6T83F26TIlrOt0/nZFOLGsm X-Received: by 10.200.8.13 with SMTP id u13mr4068950qth.233.1497408602972; Tue, 13 Jun 2017 19:50:02 -0700 (PDT) MIME-Version: 1.0 Received: by 10.55.180.135 with HTTP; Tue, 13 Jun 2017 19:50:02 -0700 (PDT) In-Reply-To: <20170613091859.GO26676@bivouac.eciton.net> References: <1497232768-8993-1-git-send-email-jun.nie@linaro.org> <20170612155315.GK26676@bivouac.eciton.net> <20170613091859.GO26676@bivouac.eciton.net> From: Jun Nie Date: Wed, 14 Jun 2017 10:50:02 +0800 Message-ID: To: Leif Lindholm Cc: Ard Biesheuvel , Haojian Zhuang , edk2-devel@lists.01.org, Shawn Guo , Jason Liu 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 02:48:49 -0000 Content-Type: text/plain; charset="UTF-8" 2017-06-13 17:18 GMT+08:00 Leif Lindholm : > On Tue, Jun 13, 2017 at 10:14:34AM +0800, Jun Nie wrote: >> 2017-06-12 23:53 GMT+08:00 Leif Lindholm : >> > On Mon, Jun 12, 2017 at 09:59:28AM +0800, Jun Nie wrote: >> >> Add alignment for ECSD data for DMA access. Otherwise >> >> the data is corrupted on Sanechips platform. >> > >> > I never did see a reply to my proposed solution, and the below is not >> > it. Can you explain why you prefer this one? >> >> 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. > >> 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. > >> 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. 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? > > 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. 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 >> >>