[PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[Jun Nie]

Add alignment for ECSD data for DMA access. Otherwise
the data is corrupted on Sanechips platform.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 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

Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[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?

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  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
> 

Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[Andrew Fish]

> On Jun 12, 2017, at 8:53 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 
> 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?
> 

And it is still exposed to enum optimization changing the alignment of the structure. 

Thanks,

Andrew Fish

> /
>    Leif
> 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>> 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
>> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[Jun Nie]

2017-06-12 23:53 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
> 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?
>
> /
>     Leif

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. Changing the
first member to "UINT64 RESERVED_1[2]" shall secure the alignment. But
I preferred Pad method. It is more readable if all ECSD member are
UINT8 type. It is also more clear to add alignment info in CARD_INFO,
just before ECSD member.
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 <jun.nie@linaro.org>
>> ---
>>  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
>>

Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[Andrew Fish]

> On Jun 12, 2017, at 7:14 PM, Jun Nie <jun.nie@linaro.org> wrote:
> 
> 2017-06-12 23:53 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> 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?
>> 
>> /
>>    Leif
> 
> 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. Changing the
> first member to "UINT64 RESERVED_1[2]" shall secure the alignment. But
> I preferred Pad method. It is more readable if all ECSD member are
> UINT8 type. It is also more clear to add alignment info in CARD_INFO,
> just before ECSD member.
> I do not get point of Andrew, maybe he share the same concern.
> 

Jun

typedef enum {
  UNKNOWN_CARD,
  MMC_CARD,              //MMC card
  MMC_CARD_HIGH,         //MMC Card with High capacity
  EMMC_CARD,             //eMMC 4.41 card
  SD_CARD,               //SD 1.1 card
  SD_CARD_2,             //SD 2.0 or above standard card
  SD_CARD_2_HIGH         //SD 2.0 or above high capacity card
} CARD_TYPE;

Per C spec sizeof(CARD_TYPE) can be 1, 2, 4, or 8 (64-bit integer), and it is legal for the compiler to pick any of these. So it is not portable C code to use an enum in a data structure when layout maters.

Thanks,

Andrew Fish


> Jun
> 
>> 
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>> ---
>>> 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
>>> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[Jun Nie]

2017-06-13 12:01 GMT+08:00 Andrew Fish <afish@apple.com>:
>
>> On Jun 12, 2017, at 7:14 PM, Jun Nie <jun.nie@linaro.org> wrote:
>>
>> 2017-06-12 23:53 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
>>> 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?
>>>
>>> /
>>>    Leif
>>
>> 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. Changing the
>> first member to "UINT64 RESERVED_1[2]" shall secure the alignment. But
>> I preferred Pad method. It is more readable if all ECSD member are
>> UINT8 type. It is also more clear to add alignment info in CARD_INFO,
>> just before ECSD member.
>> I do not get point of Andrew, maybe he share the same concern.
>>
>
> Jun
>
> typedef enum {
>   UNKNOWN_CARD,
>   MMC_CARD,              //MMC card
>   MMC_CARD_HIGH,         //MMC Card with High capacity
>   EMMC_CARD,             //eMMC 4.41 card
>   SD_CARD,               //SD 1.1 card
>   SD_CARD_2,             //SD 2.0 or above standard card
>   SD_CARD_2_HIGH         //SD 2.0 or above high capacity card
> } CARD_TYPE;
>
> Per C spec sizeof(CARD_TYPE) can be 1, 2, 4, or 8 (64-bit integer), and it is legal for the compiler to pick any of these. So it is not portable C code to use an enum in a data structure when layout maters.
>
> Thanks,
>
> Andrew Fish

CARD_TYPE CardType is 2nd member of CARD_INFO, while ECSDData is the
last member and I just want to align it to 8 bytes. I had assume pad
will be added automatically by compiler if CARD_TYPE is not 8 bytes
aligned and UNIT64 type appear in following member. Does enum will
impact the later member alignment? Could you help elaborate more about
this?

Thank you!
Jun

>>
>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>>> ---
>>>> 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
>>>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>

Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[Andrew Fish]

> On Jun 12, 2017, at 9:13 PM, Jun Nie <jun.nie@linaro.org> wrote:
> 
> 2017-06-13 12:01 GMT+08:00 Andrew Fish <afish@apple.com <mailto:afish@apple.com>>:
>> 
>>> On Jun 12, 2017, at 7:14 PM, Jun Nie <jun.nie@linaro.org> wrote:
>>> 
>>> 2017-06-12 23:53 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
>>>> 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?
>>>> 
>>>> /
>>>>   Leif
>>> 
>>> 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. Changing the
>>> first member to "UINT64 RESERVED_1[2]" shall secure the alignment. But
>>> I preferred Pad method. It is more readable if all ECSD member are
>>> UINT8 type. It is also more clear to add alignment info in CARD_INFO,
>>> just before ECSD member.
>>> I do not get point of Andrew, maybe he share the same concern.
>>> 
>> 
>> Jun
>> 
>> typedef enum {
>>  UNKNOWN_CARD,
>>  MMC_CARD,              //MMC card
>>  MMC_CARD_HIGH,         //MMC Card with High capacity
>>  EMMC_CARD,             //eMMC 4.41 card
>>  SD_CARD,               //SD 1.1 card
>>  SD_CARD_2,             //SD 2.0 or above standard card
>>  SD_CARD_2_HIGH         //SD 2.0 or above high capacity card
>> } CARD_TYPE;
>> 
>> Per C spec sizeof(CARD_TYPE) can be 1, 2, 4, or 8 (64-bit integer), and it is legal for the compiler to pick any of these. So it is not portable C code to use an enum in a data structure when layout maters.
>> 
>> Thanks,
>> 
>> Andrew Fish
> 
> CARD_TYPE CardType is 2nd member of CARD_INFO, while ECSDData is the
> last member and I just want to align it to 8 bytes. I had assume pad
> will be added automatically by compiler if CARD_TYPE is not 8 bytes
> aligned and UNIT64 type appear in following member. Does enum will
> impact the later member alignment? Could you help elaborate more about
> this?
> 

Sure

type struct {
  UINT16 RCA;
  UINT8   CardType;
  UINT32 OCRData;
...

Has different alignment than:
type struct {
  UINT16 RCA;
  UINT32 CardType;
  UINT32 OCRData;
...

Both are legal things for the C compiler to due given the type is an enum. 
1st example OCRData starts at offset 4
2nd example OCRData starts at offset 8

An integer type is not an int. 

Thanks,

Andrew Fish

> Thank you!
> Jun
> 
>>> 
>>>> 
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>>>> ---
>>>>> 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
>>>>> 
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>

Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[Jun Nie]

2017-06-13 12:25 GMT+08:00 Andrew Fish <afish@apple.com>:
>
> On Jun 12, 2017, at 9:13 PM, Jun Nie <jun.nie@linaro.org> wrote:
>
> 2017-06-13 12:01 GMT+08:00 Andrew Fish <afish@apple.com>:
>
>
> On Jun 12, 2017, at 7:14 PM, Jun Nie <jun.nie@linaro.org> wrote:
>
> 2017-06-12 23:53 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
>
> 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?
>
> /
>   Leif
>
>
> 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. Changing the
> first member to "UINT64 RESERVED_1[2]" shall secure the alignment. But
> I preferred Pad method. It is more readable if all ECSD member are
> UINT8 type. It is also more clear to add alignment info in CARD_INFO,
> just before ECSD member.
> I do not get point of Andrew, maybe he share the same concern.
>
>
> Jun
>
> typedef enum {
>  UNKNOWN_CARD,
>  MMC_CARD,              //MMC card
>  MMC_CARD_HIGH,         //MMC Card with High capacity
>  EMMC_CARD,             //eMMC 4.41 card
>  SD_CARD,               //SD 1.1 card
>  SD_CARD_2,             //SD 2.0 or above standard card
>  SD_CARD_2_HIGH         //SD 2.0 or above high capacity card
> } CARD_TYPE;
>
> Per C spec sizeof(CARD_TYPE) can be 1, 2, 4, or 8 (64-bit integer), and it
> is legal for the compiler to pick any of these. So it is not portable C code
> to use an enum in a data structure when layout maters.
>
> Thanks,
>
> Andrew Fish
>
>
> CARD_TYPE CardType is 2nd member of CARD_INFO, while ECSDData is the
> last member and I just want to align it to 8 bytes. I had assume pad
> will be added automatically by compiler if CARD_TYPE is not 8 bytes
> aligned and UNIT64 type appear in following member. Does enum will
> impact the later member alignment? Could you help elaborate more about
> this?
>
>
> Sure
>
> type struct {
>   UINT16 RCA;
>   UINT8   CardType;
>   UINT32 OCRData;
> ...
>
> Has different alignment than:
> type struct {
>   UINT16 RCA;
>   UINT32 CardType;
>   UINT32 OCRData;
> ...
>
> Both are legal things for the C compiler to due given the type is an enum.
> 1st example OCRData starts at offset 4
> 2nd example OCRData starts at offset 8
>
> An integer type is not an int.
>
> Thanks,
>
> Andrew Fish

But I do not care about OCRData. I just care the alignment of
ECSDData, which is the last member and I assume it shall be impacted
just by the previous member alignment. Does ECSDData alignment cannot
be secured with Pad change if enum is the 2nd member? If so, is it
safe if I make sure the first member in ECSDData is UINT64?

 typedef struct  {
  UINT16 RCA;
  UINT8   CardType;
  OCR       OCRData;
  CID       CIDData;
  CSD       CSDData;
+  UINT64    Pad;                              // For 8 bytes
alignment of ECSDData
   ECSD      ECSDData;                         // MMC V4 extended card specific
 } CARD_INFO;

Thanks!
Jun

>
>
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
> 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
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
>

Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[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 <leif.lindholm@linaro.org>:
> > 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.

> 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.

> 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?

Is there any chance you can share the EFI_MMC_HOST_PROTOCOL
implementation that triggers this error?

/
    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 <jun.nie@linaro.org>
> >> ---
> >>  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
> >>

Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[Jun Nie]

2017-06-13 17:18 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Jun 13, 2017 at 10:14:34AM +0800, Jun Nie wrote:
>> 2017-06-12 23:53 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> > 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 <jun.nie@linaro.org>
>> >> ---
>> >>  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
>> >>

Re: [PATCH v2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[Leif Lindholm]

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 <jun.nie@linaro.org>
> >> >> ---
> >> >>  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
> >> >>