| Message ID | 1431202542-31670-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 9 May 2015 at 22:39, Andrew Fish <afish@apple.com> wrote: > Ard, > > Would removing the Reserved field and making Size a UINT64 also fix the issue? > Yes, but since the Size field is assigned to POOL_TAIL::Size as well, and used in comparisons in CoreFreePoolI(), I thought this would be the cleaner approach. But if you prefer that, I am happy to update the patch. > Contributed-under: TianoCore Contribution Agreement 1.0 > Reviewed-by: Andrew Fish <afish@apple.com> > > Thanks, > > Andrew Fish > > >> On May 9, 2015, at 1:15 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >> According to the UEFIv2.5 spec section 6.2, the allocations returned >> by the AllocatePool () boot service must be 8 byte aligned. >> >> So make our implementation conform to the spec, by rearranging the >> pool head struct so that its size is always a multiple of 8 bytes. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> MdeModulePkg/Core/Dxe/Mem/Pool.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c >> index ac717fb65f7a..6f8f5cfb295d 100644 >> --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c >> +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c >> @@ -26,9 +26,9 @@ typedef struct { >> #define POOL_HEAD_SIGNATURE SIGNATURE_32('p','h','d','0') >> typedef struct { >> UINT32 Signature; >> - UINT32 Reserved; >> EFI_MEMORY_TYPE Type; >> UINTN Size; >> + UINTN Reserved; >> CHAR8 Data[1]; >> } POOL_HEAD; >> >> -- >> 1.9.1 >> >> >> ------------------------------------------------------------------------------ >> One dashboard for servers and applications across Physical-Virtual-Cloud >> Widest out-of-the-box monitoring support with 50+ applications >> Performance metrics, stats and reports that give you Actionable Insights >> Deep dive visibility with transaction tracing using APM Insight. >> http://ad.doubleclick.net/ddm/clk/290420510;117567292;y >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > ------------------------------------------------------------------------------ > One dashboard for servers and applications across Physical-Virtual-Cloud > Widest out-of-the-box monitoring support with 50+ applications > Performance metrics, stats and reports that give you Actionable Insights > Deep dive visibility with transaction tracing using APM Insight. > http://ad.doubleclick.net/ddm/clk/290420510;117567292;y > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
On 10 May 2015 at 07:30, Jordan Justen <jordan.l.justen@intel.com> wrote: > On 2015-05-09 13:15:42, Ard Biesheuvel wrote: >> According to the UEFIv2.5 spec section 6.2, the allocations returned >> by the AllocatePool () boot service must be 8 byte aligned. >> >> So make our implementation conform to the spec, by rearranging the >> pool head struct so that its size is always a multiple of 8 bytes. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> MdeModulePkg/Core/Dxe/Mem/Pool.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c >> index ac717fb65f7a..6f8f5cfb295d 100644 >> --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c >> +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c >> @@ -26,9 +26,9 @@ typedef struct { >> #define POOL_HEAD_SIGNATURE SIGNATURE_32('p','h','d','0') >> typedef struct { >> UINT32 Signature; >> - UINT32 Reserved; >> EFI_MEMORY_TYPE Type; >> UINTN Size; >> + UINTN Reserved; >> CHAR8 Data[1]; >> } POOL_HEAD; > > So the issue is if sizeof(EFI_MEMORY_TYPE) == 4 on a 64-bit machine and the > compiler doesn't 64-bit align the 64-bit Size field? > Yes, that was the issue, see below. > With your change, couldn't there be an issue if the compiler made > EFI_MEMORY_TYPE 64-bits, and once again chose not to 64-bit align Size > and Reserved? > C defines enum as an int type, and GCC has special command line options only to make it smaller, not larger, so I don't think that is a concern here. But after reading your comment about aligning the 64-bit size field, and going back to my code to see why that is not happening, I can no longer reproduce my failure case, so please disregard this patch for now. I will come back to it if the issue resurfaces.
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c index ac717fb65f7a..6f8f5cfb295d 100644 --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c @@ -26,9 +26,9 @@ typedef struct { #define POOL_HEAD_SIGNATURE SIGNATURE_32('p','h','d','0') typedef struct { UINT32 Signature; - UINT32 Reserved; EFI_MEMORY_TYPE Type; UINTN Size; + UINTN Reserved; CHAR8 Data[1]; } POOL_HEAD;
According to the UEFIv2.5 spec section 6.2, the allocations returned by the AllocatePool () boot service must be 8 byte aligned. So make our implementation conform to the spec, by rearranging the pool head struct so that its size is always a multiple of 8 bytes. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- MdeModulePkg/Core/Dxe/Mem/Pool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)