Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

[Kun Qin]

Thanks for the clarification. I will work on v-next with flexible array 
as Data field.


Regards,
Kun

On 06/29/2021 18:07, Kinney, Michael D wrote:

If it breaks in the future, then that would be due to a new compiler that
or changes to the configuration of an existing compiler that break compatibility
with UEFI ABI.  The compiler issue must be resolved before the new compiler
or change to existing compiler are accepted.

Mike


-Original Message-
From: Kun Qin 
Sent: Tuesday, June 29, 2021 4:11 PM
To: Kinney, Michael D ; devel@edk2.groups.io; 
bret.barke...@microsoft.com; Marvin Häuser
; Laszlo Ersek 
Cc: Wang, Jian J ; Wu, Hao A ; Dong, Eric 
; Ni, Ray
; Liming Gao ; Liu, Zhiguang 
; Andrew Fish
; Lindholm, Leif ; Michael Kubacki 

Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI 
Specification: Update
EFI_MM_COMMUNICATE_HEADER

Hi Mike,

Thanks for the note. I will add this check for sanity check in v-next,
assuming this will not fail for currently supported compilers.

Just curious, what do we normally do if this type of check start to
break in the future?

Thanks,
Kun

On 06/29/2021 10:28, Kinney, Michael D wrote:

Good idea on use of STATIC_ASSERT().

For structures that use flexible array members, we can add a
STATIC_ASSERT() for the sizeof() and OFFSET_OF() returning the same result.

For example:

STATIC_ASSERT (sizeof (EFI_MM_COMMUNICATE_HEADER) == OFFSET_OF
(EFI_MM_COMMUNICATE_HEADER, Data));

Mike

*From:*devel@edk2.groups.io  *On Behalf Of *Bret
Barkelew via groups.io
*Sent:* Tuesday, June 29, 2021 9:00 AM
*To:* Marvin Häuser ; Laszlo Ersek
; Kun Qin ; Kinney, Michael D
; devel@edk2.groups.io
*Cc:* Wang, Jian J ; Wu, Hao A
; Dong, Eric ; Ni, Ray
; Liming Gao ; Liu, Zhiguang
; Andrew Fish ; Lindholm, Leif
; Michael Kubacki 
*Subject:* Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code
First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

Good note. Thanks!

- Bret

*From: *Marvin Häuser <mailto:mhaeu...@posteo.de>
*Sent: *Tuesday, June 29, 2021 1:58 AM
*To: *Bret Barkelew <mailto:bret.barke...@microsoft.com>; Laszlo Ersek
<mailto:ler...@redhat.com>; Kun Qin <mailto:kuqi...@gmail.com>; Kinney,
Michael D <mailto:michael.d.kin...@intel.com>; devel@edk2.groups.io
<mailto:devel@edk2.groups.io>
*Cc: *Wang, Jian J <mailto:jian.j.w...@intel.com>; Wu, Hao A
<mailto:hao.a...@intel.com>; Dong, Eric <mailto:eric.d...@intel.com>;
Ni, Ray <mailto:ray...@intel.com>; Liming Gao
<mailto:gaolim...@byosoft.com.cn>; Liu, Zhiguang
<mailto:zhiguang@intel.com>; Andrew Fish <mailto:af...@apple.com>;
Lindholm, Leif <mailto:l...@nuviainc.com>; Michael Kubacki
<mailto:michael.kuba...@microsoft.com>
*Subject: *Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code
First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit
alignment for 64-bit integers on IA32 (which led to a (non-critical)
mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to)
successfully dictate natural alignment consistently. Possibly we could
introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on
32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF
macro is introduced.

Best regards,
Marvin

On 29.06.21 08:49, Bret Barkelew wrote:
  >
  > The fact that it may vary per ABI seems like a pretty big gotcha if
  > the SMM/MM Core was compiled at a different time or on a different
  > system than the module that’s invoking the communication.
  >
  > - Bret
  >
  > *From: *Marvin Häuser <mailto:mhaeu...@posteo.de
<mailto:mhaeu...@posteo.de>>
  > *Sent: *Monday, June 28, 2021 8:43 AM
  > *To: *Laszlo Ersek <mailto:ler...@redhat.com
<mailto:ler...@redhat.com>>; Kun Qin
  > <mailto:kuqi...@gmail.com <mailto:kuqi...@gmail.com>>; Kinney, Michael D
  > <mailto:michael.d.kin...@intel.com
<mailto:michael.d.kin...@intel.com>>; devel@edk2.groups.io
<mailto:devel@edk2.groups.io>
  > <mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>>
  > *Cc: *Wang, Jian J <mailto:jian.j.w...@intel.com
<mailto:jian.j.w...@intel.com>>; Wu, Hao A
  > <mailto:hao.a...@intel.com <mailto:hao.a...@intel.com>>; Dong, Eric
<mailto:eric.d...@intel.com <mailto:eric.d...@intel.com>>;
  > Ni, Ray <mailto:ray...@intel.com <mailto:ray...@intel.com>>; Liming Gao
  > <mailto:gaolim...@byosoft.com.cn <mailto:gaolim...@byosoft.com.cn>>;
Liu, Zhiguang
  > <mailto:zhiguang@intel.com <mailto:zhiguang@intel.com>>;
Andrew Fish <mailto:af...@apple.com <mailto:af...@apple.com>>;
  > Lindholm, Leif <mailto:l...@nuviainc.com <mailto:l...@nuviainc.com>>;
Bret Barkelew
  > <mailto:bret.barke...@microsoft.com
<mailto

Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

[Michael D Kinney]

If it breaks in the future, then that would be due to a new compiler that
or changes to the configuration of an existing compiler that break compatibility
with UEFI ABI.  The compiler issue must be resolved before the new compiler
or change to existing compiler are accepted.

Mike

> -Original Message-
> From: Kun Qin 
> Sent: Tuesday, June 29, 2021 4:11 PM
> To: Kinney, Michael D ; devel@edk2.groups.io; 
> bret.barke...@microsoft.com; Marvin Häuser
> ; Laszlo Ersek 
> Cc: Wang, Jian J ; Wu, Hao A ; 
> Dong, Eric ; Ni, Ray
> ; Liming Gao ; Liu, Zhiguang 
> ; Andrew Fish
> ; Lindholm, Leif ; Michael Kubacki 
> 
> Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI 
> Specification: Update
> EFI_MM_COMMUNICATE_HEADER
> 
> Hi Mike,
> 
> Thanks for the note. I will add this check for sanity check in v-next,
> assuming this will not fail for currently supported compilers.
> 
> Just curious, what do we normally do if this type of check start to
> break in the future?
> 
> Thanks,
> Kun
> 
> On 06/29/2021 10:28, Kinney, Michael D wrote:
> > Good idea on use of STATIC_ASSERT().
> >
> > For structures that use flexible array members, we can add a
> > STATIC_ASSERT() for the sizeof() and OFFSET_OF() returning the same result.
> >
> > For example:
> >
> > STATIC_ASSERT (sizeof (EFI_MM_COMMUNICATE_HEADER) == OFFSET_OF
> > (EFI_MM_COMMUNICATE_HEADER, Data));
> >
> > Mike
> >
> > *From:*devel@edk2.groups.io  *On Behalf Of *Bret
> > Barkelew via groups.io
> > *Sent:* Tuesday, June 29, 2021 9:00 AM
> > *To:* Marvin Häuser ; Laszlo Ersek
> > ; Kun Qin ; Kinney, Michael D
> > ; devel@edk2.groups.io
> > *Cc:* Wang, Jian J ; Wu, Hao A
> > ; Dong, Eric ; Ni, Ray
> > ; Liming Gao ; Liu, Zhiguang
> > ; Andrew Fish ; Lindholm, Leif
> > ; Michael Kubacki 
> > *Subject:* Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code
> > First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
> >
> > Good note. Thanks!
> >
> > - Bret
> >
> > *From: *Marvin Häuser <mailto:mhaeu...@posteo.de>
> > *Sent: *Tuesday, June 29, 2021 1:58 AM
> > *To: *Bret Barkelew <mailto:bret.barke...@microsoft.com>; Laszlo Ersek
> > <mailto:ler...@redhat.com>; Kun Qin <mailto:kuqi...@gmail.com>; Kinney,
> > Michael D <mailto:michael.d.kin...@intel.com>; devel@edk2.groups.io
> > <mailto:devel@edk2.groups.io>
> > *Cc: *Wang, Jian J <mailto:jian.j.w...@intel.com>; Wu, Hao A
> > <mailto:hao.a...@intel.com>; Dong, Eric <mailto:eric.d...@intel.com>;
> > Ni, Ray <mailto:ray...@intel.com>; Liming Gao
> > <mailto:gaolim...@byosoft.com.cn>; Liu, Zhiguang
> > <mailto:zhiguang@intel.com>; Andrew Fish <mailto:af...@apple.com>;
> > Lindholm, Leif <mailto:l...@nuviainc.com>; Michael Kubacki
> > <mailto:michael.kuba...@microsoft.com>
> > *Subject: *Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code
> > First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
> >
> > Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit
> > alignment for 64-bit integers on IA32 (which led to a (non-critical)
> > mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to)
> > successfully dictate natural alignment consistently. Possibly we could
> > introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on
> > 32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF
> > macro is introduced.
> >
> > Best regards,
> > Marvin
> >
> > On 29.06.21 08:49, Bret Barkelew wrote:
> >  >
> >  > The fact that it may vary per ABI seems like a pretty big gotcha if
> >  > the SMM/MM Core was compiled at a different time or on a different
> >  > system than the module that’s invoking the communication.
> >  >
> >  > - Bret
> >  >
> >  > *From: *Marvin Häuser <mailto:mhaeu...@posteo.de
> > <mailto:mhaeu...@posteo.de>>
> >  > *Sent: *Monday, June 28, 2021 8:43 AM
> >  > *To: *Laszlo Ersek <mailto:ler...@redhat.com
> > <mailto:ler...@redhat.com>>; Kun Qin
> >  > <mailto:kuqi...@gmail.com <mailto:kuqi...@gmail.com>>; Kinney, Michael D
> >  > <mailto:michael.d.kin...@intel.com
> > <mailto:michael.d.kin...@intel.com>>; devel@edk2.groups.io
> > <mailto:devel@edk2.groups.io>
> >  > <mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>>
> >  > *Cc: *Wang, Jian J <mailto:jian.j.w...@intel.c

Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

[Kun Qin]

Hi Mike,

Thanks for the note. I will add this check for sanity check in v-next, 
assuming this will not fail for currently supported compilers.


Just curious, what do we normally do if this type of check start to 
break in the future?


Thanks,
Kun

On 06/29/2021 10:28, Kinney, Michael D wrote:

Good idea on use of STATIC_ASSERT().

For structures that use flexible array members, we can add a 
STATIC_ASSERT() for the sizeof() and OFFSET_OF() returning the same result.


For example:

STATIC_ASSERT (sizeof (EFI_MM_COMMUNICATE_HEADER) == OFFSET_OF 
(EFI_MM_COMMUNICATE_HEADER, Data));


Mike

*From:*devel@edk2.groups.io  *On Behalf Of *Bret 
Barkelew via groups.io

*Sent:* Tuesday, June 29, 2021 9:00 AM
*To:* Marvin Häuser ; Laszlo Ersek 
; Kun Qin ; Kinney, Michael D 
; devel@edk2.groups.io
*Cc:* Wang, Jian J ; Wu, Hao A 
; Dong, Eric ; Ni, Ray 
; Liming Gao ; Liu, Zhiguang 
; Andrew Fish ; Lindholm, Leif 
; Michael Kubacki 
*Subject:* Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code 
First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER


Good note. Thanks!

- Bret

*From: *Marvin Häuser <mailto:mhaeu...@posteo.de>
*Sent: *Tuesday, June 29, 2021 1:58 AM
*To: *Bret Barkelew <mailto:bret.barke...@microsoft.com>; Laszlo Ersek 
<mailto:ler...@redhat.com>; Kun Qin <mailto:kuqi...@gmail.com>; Kinney, 
Michael D <mailto:michael.d.kin...@intel.com>; devel@edk2.groups.io 
<mailto:devel@edk2.groups.io>
*Cc: *Wang, Jian J <mailto:jian.j.w...@intel.com>; Wu, Hao A 
<mailto:hao.a...@intel.com>; Dong, Eric <mailto:eric.d...@intel.com>; 
Ni, Ray <mailto:ray...@intel.com>; Liming Gao 
<mailto:gaolim...@byosoft.com.cn>; Liu, Zhiguang 
<mailto:zhiguang@intel.com>; Andrew Fish <mailto:af...@apple.com>; 
Lindholm, Leif <mailto:l...@nuviainc.com>; Michael Kubacki 
<mailto:michael.kuba...@microsoft.com>
*Subject: *Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code 
First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER


Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit
alignment for 64-bit integers on IA32 (which led to a (non-critical)
mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to)
successfully dictate natural alignment consistently. Possibly we could
introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on
32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF
macro is introduced.

Best regards,
Marvin

On 29.06.21 08:49, Bret Barkelew wrote:
 >
 > The fact that it may vary per ABI seems like a pretty big gotcha if
 > the SMM/MM Core was compiled at a different time or on a different
 > system than the module that’s invoking the communication.
 >
 > - Bret
 >
 > *From: *Marvin Häuser <mailto:mhaeu...@posteo.de 
<mailto:mhaeu...@posteo.de>>

 > *Sent: *Monday, June 28, 2021 8:43 AM
 > *To: *Laszlo Ersek <mailto:ler...@redhat.com 
<mailto:ler...@redhat.com>>; Kun Qin

 > <mailto:kuqi...@gmail.com <mailto:kuqi...@gmail.com>>; Kinney, Michael D
 > <mailto:michael.d.kin...@intel.com 
<mailto:michael.d.kin...@intel.com>>; devel@edk2.groups.io 
<mailto:devel@edk2.groups.io>

 > <mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>>
 > *Cc: *Wang, Jian J <mailto:jian.j.w...@intel.com 
<mailto:jian.j.w...@intel.com>>; Wu, Hao A
 > <mailto:hao.a...@intel.com <mailto:hao.a...@intel.com>>; Dong, Eric 
<mailto:eric.d...@intel.com <mailto:eric.d...@intel.com>>;

 > Ni, Ray <mailto:ray...@intel.com <mailto:ray...@intel.com>>; Liming Gao
 > <mailto:gaolim...@byosoft.com.cn <mailto:gaolim...@byosoft.com.cn>>; 
Liu, Zhiguang
 > <mailto:zhiguang@intel.com <mailto:zhiguang@intel.com>>; 
Andrew Fish <mailto:af...@apple.com <mailto:af...@apple.com>>;
 > Lindholm, Leif <mailto:l...@nuviainc.com <mailto:l...@nuviainc.com>>; 
Bret Barkelew
 > <mailto:bret.barke...@microsoft.com 
<mailto:bret.barke...@microsoft.com>>; Michael Kubacki
 > <mailto:michael.kuba...@microsoft.com 
<mailto:michael.kuba...@microsoft.com>>

 > *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First:
 > PI Specification: Update EFI_MM_COMMUNICATE_HEADER
 >
 > On 28.06.21 16:57, Laszlo Ersek wrote:
 > > On 06/25/21 20:47, Kun Qin wrote:
 > >> Hi Mike,
 > >>
 > >> Thanks for the information. I can update the patch and proposed spec
 > >> change to use flexible array in v-next if there is no other concerns.
 > >>
 > >> After switching to flexible array, OFFSET_OF (Data) should lead to the
 > >> same result as sizeof.
 > > This may be true on specific compilers, but it is *not* guaranteed by
 > > the C language standard.
 >

Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

[Michael D Kinney]

Good idea on use of STATIC_ASSERT().

For structures that use flexible array members, we can add a STATIC_ASSERT() 
for the sizeof() and OFFSET_OF() returning the same result.

For example:

STATIC_ASSERT (sizeof (EFI_MM_COMMUNICATE_HEADER) == OFFSET_OF 
(EFI_MM_COMMUNICATE_HEADER, Data));

Mike

From: devel@edk2.groups.io  On Behalf Of Bret Barkelew 
via groups.io
Sent: Tuesday, June 29, 2021 9:00 AM
To: Marvin Häuser ; Laszlo Ersek ; Kun 
Qin ; Kinney, Michael D ; 
devel@edk2.groups.io
Cc: Wang, Jian J ; Wu, Hao A ; Dong, 
Eric ; Ni, Ray ; Liming Gao 
; Liu, Zhiguang ; Andrew Fish 
; Lindholm, Leif ; Michael Kubacki 

Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI 
Specification: Update EFI_MM_COMMUNICATE_HEADER

Good note. Thanks!

- Bret

From: Marvin Häuser<mailto:mhaeu...@posteo.de>
Sent: Tuesday, June 29, 2021 1:58 AM
To: Bret Barkelew<mailto:bret.barke...@microsoft.com>; Laszlo 
Ersek<mailto:ler...@redhat.com>; Kun Qin<mailto:kuqi...@gmail.com>; Kinney, 
Michael D<mailto:michael.d.kin...@intel.com>; 
devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Wang, Jian J<mailto:jian.j.w...@intel.com>; Wu, Hao 
A<mailto:hao.a...@intel.com>; Dong, Eric<mailto:eric.d...@intel.com>; Ni, 
Ray<mailto:ray...@intel.com>; Liming Gao<mailto:gaolim...@byosoft.com.cn>; Liu, 
Zhiguang<mailto:zhiguang@intel.com>; Andrew Fish<mailto:af...@apple.com>; 
Lindholm, Leif<mailto:l...@nuviainc.com>; Michael 
Kubacki<mailto:michael.kuba...@microsoft.com>
Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI 
Specification: Update EFI_MM_COMMUNICATE_HEADER

Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit
alignment for 64-bit integers on IA32 (which led to a (non-critical)
mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to)
successfully dictate natural alignment consistently. Possibly we could
introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on
32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF
macro is introduced.

Best regards,
Marvin

On 29.06.21 08:49, Bret Barkelew wrote:
>
> The fact that it may vary per ABI seems like a pretty big gotcha if
> the SMM/MM Core was compiled at a different time or on a different
> system than the module that’s invoking the communication.
>
> - Bret
>
> *From: *Marvin Häuser <mailto:mhaeu...@posteo.de>
> *Sent: *Monday, June 28, 2021 8:43 AM
> *To: *Laszlo Ersek <mailto:ler...@redhat.com>; Kun Qin
> <mailto:kuqi...@gmail.com>; Kinney, Michael D
> <mailto:michael.d.kin...@intel.com>; 
> devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> <mailto:devel@edk2.groups.io>
> *Cc: *Wang, Jian J <mailto:jian.j.w...@intel.com>; Wu, Hao A
> <mailto:hao.a...@intel.com>; Dong, Eric <mailto:eric.d...@intel.com>;
> Ni, Ray <mailto:ray...@intel.com>; Liming Gao
> <mailto:gaolim...@byosoft.com.cn>; Liu, Zhiguang
> <mailto:zhiguang@intel.com>; Andrew Fish <mailto:af...@apple.com>;
> Lindholm, Leif <mailto:l...@nuviainc.com>; Bret Barkelew
> <mailto:bret.barke...@microsoft.com>; Michael Kubacki
> <mailto:michael.kuba...@microsoft.com>
> *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First:
> PI Specification: Update EFI_MM_COMMUNICATE_HEADER
>
> On 28.06.21 16:57, Laszlo Ersek wrote:
> > On 06/25/21 20:47, Kun Qin wrote:
> >> Hi Mike,
> >>
> >> Thanks for the information. I can update the patch and proposed spec
> >> change to use flexible array in v-next if there is no other concerns.
> >>
> >> After switching to flexible array, OFFSET_OF (Data) should lead to the
> >> same result as sizeof.
> > This may be true on specific compilers, but it is *not* guaranteed by
> > the C language standard.
>
> Sorry, for completeness sake... :)
>
> I don't think it really depends on the compiler (but can vary per ABI),
> but it's a conceptual issue with alignment requirements. Assuming
> natural alignment and the usual stuff, for "struct s { uint64_t a;
> uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
> there are 4 Bytes of padding after "b" (as "c" may as well be empty).
> "c" however is guaranteed to start after b in the same fashion as if it
> was declared with the maximum amount of elements that fit the memory. So
> if we take 4 elements for "c", and note that "c" has an alignment
> requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
> "sizeof" this means that the padding is included, whereas for "offsetof"
> it is not, yielding "sizeof

Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

[Bret Barkelew via groups.io]

Good note. Thanks!

- Bret

From: Marvin Häuser<mailto:mhaeu...@posteo.de>
Sent: Tuesday, June 29, 2021 1:58 AM
To: Bret Barkelew<mailto:bret.barke...@microsoft.com>; Laszlo 
Ersek<mailto:ler...@redhat.com>; Kun Qin<mailto:kuqi...@gmail.com>; Kinney, 
Michael D<mailto:michael.d.kin...@intel.com>; 
devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Wang, Jian J<mailto:jian.j.w...@intel.com>; Wu, Hao 
A<mailto:hao.a...@intel.com>; Dong, Eric<mailto:eric.d...@intel.com>; Ni, 
Ray<mailto:ray...@intel.com>; Liming Gao<mailto:gaolim...@byosoft.com.cn>; Liu, 
Zhiguang<mailto:zhiguang@intel.com>; Andrew Fish<mailto:af...@apple.com>; 
Lindholm, Leif<mailto:l...@nuviainc.com>; Michael 
Kubacki<mailto:michael.kuba...@microsoft.com>
Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI 
Specification: Update EFI_MM_COMMUNICATE_HEADER

Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit
alignment for 64-bit integers on IA32 (which led to a (non-critical)
mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to)
successfully dictate natural alignment consistently. Possibly we could
introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on
32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF
macro is introduced.

Best regards,
Marvin

On 29.06.21 08:49, Bret Barkelew wrote:
>
> The fact that it may vary per ABI seems like a pretty big gotcha if
> the SMM/MM Core was compiled at a different time or on a different
> system than the module that’s invoking the communication.
>
> - Bret
>
> *From: *Marvin Häuser <mailto:mhaeu...@posteo.de>
> *Sent: *Monday, June 28, 2021 8:43 AM
> *To: *Laszlo Ersek <mailto:ler...@redhat.com>; Kun Qin
> <mailto:kuqi...@gmail.com>; Kinney, Michael D
> <mailto:michael.d.kin...@intel.com>; devel@edk2.groups.io
> <mailto:devel@edk2.groups.io>
> *Cc: *Wang, Jian J <mailto:jian.j.w...@intel.com>; Wu, Hao A
> <mailto:hao.a...@intel.com>; Dong, Eric <mailto:eric.d...@intel.com>;
> Ni, Ray <mailto:ray...@intel.com>; Liming Gao
> <mailto:gaolim...@byosoft.com.cn>; Liu, Zhiguang
> <mailto:zhiguang@intel.com>; Andrew Fish <mailto:af...@apple.com>;
> Lindholm, Leif <mailto:l...@nuviainc.com>; Bret Barkelew
> <mailto:bret.barke...@microsoft.com>; Michael Kubacki
> <mailto:michael.kuba...@microsoft.com>
> *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First:
> PI Specification: Update EFI_MM_COMMUNICATE_HEADER
>
> On 28.06.21 16:57, Laszlo Ersek wrote:
> > On 06/25/21 20:47, Kun Qin wrote:
> >> Hi Mike,
> >>
> >> Thanks for the information. I can update the patch and proposed spec
> >> change to use flexible array in v-next if there is no other concerns.
> >>
> >> After switching to flexible array, OFFSET_OF (Data) should lead to the
> >> same result as sizeof.
> > This may be true on specific compilers, but it is *not* guaranteed by
> > the C language standard.
>
> Sorry, for completeness sake... :)
>
> I don't think it really depends on the compiler (but can vary per ABI),
> but it's a conceptual issue with alignment requirements. Assuming
> natural alignment and the usual stuff, for "struct s { uint64_t a;
> uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
> there are 4 Bytes of padding after "b" (as "c" may as well be empty).
> "c" however is guaranteed to start after b in the same fashion as if it
> was declared with the maximum amount of elements that fit the memory. So
> if we take 4 elements for "c", and note that "c" has an alignment
> requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
> "sizeof" this means that the padding is included, whereas for "offsetof"
> it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4".
> That is what I meant by "wasted space" earlier, but this could possibly
> be made nicer with macros as necessary.
>
> As for this specific struct, the values should be identical as it is
> padded.
>
> Best regards,
> Marvin
>
> >
> > Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
> >
> > "In most situations, the flexible array member is ignored. In
> > particular, the size of the structure is as if the flexible array member
> > were omitted except that it may have more trailing padding than the
> > omission would imply."
> >
> > Quoting footnotes 17 and 19,
> >
> > (17)  [...]
> >s

Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

[Marvin Häuser]

Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit 
alignment for 64-bit integers on IA32 (which led to a (non-critical) 
mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to) 
successfully dictate natural alignment consistently. Possibly we could 
introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on 
32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF 
macro is introduced.


Best regards,
Marvin

On 29.06.21 08:49, Bret Barkelew wrote:


The fact that it may vary per ABI seems like a pretty big gotcha if 
the SMM/MM Core was compiled at a different time or on a different 
system than the module that’s invoking the communication.


- Bret

*From: *Marvin Häuser 
*Sent: *Monday, June 28, 2021 8:43 AM
*To: *Laszlo Ersek ; Kun Qin 
; Kinney, Michael D 
; devel@edk2.groups.io 

*Cc: *Wang, Jian J ; Wu, Hao A 
; Dong, Eric ; 
Ni, Ray ; Liming Gao 
; Liu, Zhiguang 
; Andrew Fish ; 
Lindholm, Leif ; Bret Barkelew 
; Michael Kubacki 

*Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: 
PI Specification: Update EFI_MM_COMMUNICATE_HEADER


On 28.06.21 16:57, Laszlo Ersek wrote:
> On 06/25/21 20:47, Kun Qin wrote:
>> Hi Mike,
>>
>> Thanks for the information. I can update the patch and proposed spec
>> change to use flexible array in v-next if there is no other concerns.
>>
>> After switching to flexible array, OFFSET_OF (Data) should lead to the
>> same result as sizeof.
> This may be true on specific compilers, but it is *not* guaranteed by
> the C language standard.

Sorry, for completeness sake... :)

I don't think it really depends on the compiler (but can vary per ABI),
but it's a conceptual issue with alignment requirements. Assuming
natural alignment and the usual stuff, for "struct s { uint64_t a;
uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
there are 4 Bytes of padding after "b" (as "c" may as well be empty).
"c" however is guaranteed to start after b in the same fashion as if it
was declared with the maximum amount of elements that fit the memory. So
if we take 4 elements for "c", and note that "c" has an alignment
requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
"sizeof" this means that the padding is included, whereas for "offsetof"
it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4".
That is what I meant by "wasted space" earlier, but this could possibly
be made nicer with macros as necessary.

As for this specific struct, the values should be identical as it is 
padded.


Best regards,
Marvin

>
> Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
>
> "In most situations, the flexible array member is ignored. In
> particular, the size of the structure is as if the flexible array member
> were omitted except that it may have more trailing padding than the
> omission would imply."
>
> Quoting footnotes 17 and 19,
>
> (17)  [...]
>    struct s { int n; double d[]; };
>    [...]
>
> (19)  [...]
>    the expressions:
>    [...]
>    sizeof (struct s) >= offsetof(struct s, d)
>
>    are always equal to 1.
>
> Thanks
> Laszlo
>
>
>
>> While sizeof would be a preferred way to move
>> forward.
>>
>> Regards,
>> Kun
>>
>> On 06/24/2021 08:25, Kinney, Michael D wrote:
>>> Hello,
>>>
>>> Flexible array members are supported and should be used.  The old 
style

>>> of adding an array of size [1] at the end of a structure was used at a
>>> time
>>> flexible array members were not supported by all compilers (late 
1990's).

>>> The workarounds used to handle the array of size [1] are very
>>> confusing when
>>> reading the C  code and the fact that sizeof() does not produce the
>>> expected
>>> result make it even worse.
>>>
>>> If we use flexible array members in this proposed change then there is
>>> no need to use OFFSET_OF().  Correct?
>>>
>>> Mike
>>>
>>>
 -Original Message-
 From: Marvin Häuser 
 Sent: Thursday, June 24, 2021 1:00 AM
 To: Kun Qin ; Laszlo Ersek ;
 devel@edk2.groups.io
 Cc: Wang, Jian J ; Wu, Hao A
 ; Dong, Eric ; Ni, Ray
 ; Kinney, Michael D ;
 Liming Gao ; Liu, Zhiguang
 ; Andrew Fish ; Leif
 Lindholm ; Bret Barkelew
 ; michael.kuba...@microsoft.com
 Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
 Specification: Update EFI_MM_COMMUNICATE_HEADER

 Hey Kun,

 Why would you rely on undefined behaviours? The OFFSET_OF macro is
 well-defined for GCC and Clang as it's implemen

Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

[Bret Barkelew via groups.io]

The fact that it may vary per ABI seems like a pretty big gotcha if the SMM/MM 
Core was compiled at a different time or on a different system than the module 
that’s invoking the communication.

- Bret

From: Marvin Häuser
Sent: Monday, June 28, 2021 8:43 AM
To: Laszlo Ersek; Kun Qin; 
Kinney, Michael D; 
devel@edk2.groups.io
Cc: Wang, Jian J; Wu, Hao 
A; Dong, Eric; Ni, 
Ray; Liming Gao; Liu, 
Zhiguang; Andrew Fish; 
Lindholm, Leif; Bret 
Barkelew; Michael 
Kubacki
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI 
Specification: Update EFI_MM_COMMUNICATE_HEADER

On 28.06.21 16:57, Laszlo Ersek wrote:
> On 06/25/21 20:47, Kun Qin wrote:
>> Hi Mike,
>>
>> Thanks for the information. I can update the patch and proposed spec
>> change to use flexible array in v-next if there is no other concerns.
>>
>> After switching to flexible array, OFFSET_OF (Data) should lead to the
>> same result as sizeof.
> This may be true on specific compilers, but it is *not* guaranteed by
> the C language standard.

Sorry, for completeness sake... :)

I don't think it really depends on the compiler (but can vary per ABI),
but it's a conceptual issue with alignment requirements. Assuming
natural alignment and the usual stuff, for "struct s { uint64_t a;
uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
there are 4 Bytes of padding after "b" (as "c" may as well be empty).
"c" however is guaranteed to start after b in the same fashion as if it
was declared with the maximum amount of elements that fit the memory. So
if we take 4 elements for "c", and note that "c" has an alignment
requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
"sizeof" this means that the padding is included, whereas for "offsetof"
it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4".
That is what I meant by "wasted space" earlier, but this could possibly
be made nicer with macros as necessary.

As for this specific struct, the values should be identical as it is padded.

Best regards,
Marvin

>
> Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
>
> "In most situations, the flexible array member is ignored. In
> particular, the size of the structure is as if the flexible array member
> were omitted except that it may have more trailing padding than the
> omission would imply."
>
> Quoting footnotes 17 and 19,
>
> (17)  [...]
>struct s { int n; double d[]; };
>[...]
>
> (19)  [...]
>the expressions:
>[...]
>sizeof (struct s) >= offsetof(struct s, d)
>
>are always equal to 1.
>
> Thanks
> Laszlo
>
>
>
>> While sizeof would be a preferred way to move
>> forward.
>>
>> Regards,
>> Kun
>>
>> On 06/24/2021 08:25, Kinney, Michael D wrote:
>>> Hello,
>>>
>>> Flexible array members are supported and should be used.  The old style
>>> of adding an array of size [1] at the end of a structure was used at a
>>> time
>>> flexible array members were not supported by all compilers (late 1990's).
>>> The workarounds used to handle the array of size [1] are very
>>> confusing when
>>> reading the C  code and the fact that sizeof() does not produce the
>>> expected
>>> result make it even worse.
>>>
>>> If we use flexible array members in this proposed change then there is
>>> no need to use OFFSET_OF().  Correct?
>>>
>>> Mike
>>>
>>>
 -Original Message-
 From: Marvin Häuser 
 Sent: Thursday, June 24, 2021 1:00 AM
 To: Kun Qin ; Laszlo Ersek ;
 devel@edk2.groups.io
 Cc: Wang, Jian J ; Wu, Hao A
 ; Dong, Eric ; Ni, Ray
 ; Kinney, Michael D ;
 Liming Gao ; Liu, Zhiguang
 ; Andrew Fish ; Leif
 Lindholm ; Bret Barkelew
 ; michael.kuba...@microsoft.com
 Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
 Specification: Update EFI_MM_COMMUNICATE_HEADER

 Hey Kun,

 Why would you rely on undefined behaviours? The OFFSET_OF macro is
 well-defined for GCC and Clang as it's implemented by an intrinsic, and
 while the expression for the MSVC compiler is undefined behaviour as per
 the C standard, it is well-defined for MSVC due to their own
 implementation being identical. From my standpoint, all supported
 compilers will yield well-defined behaviour even this way. OFFSET_OF on
 flexible arrays is not UB in any case to my knowledge.

 However, the same way as your new suggestion, you can replace OFFSET_OF
 with sizeof. While this *can* lead to wasted space with certain
 structure layouts