Re: [PATCH 2/2] MdePkg/UefiGpt.h: Add new definition for enable GPT support

["Gao, Liming" <liming.gao@intel.com>]

Chen:
  I agree with Hao. For UEFI definition, please add EFI_ prefix. And, reuse the existing definition, not introduce new one. 

Thanks
Liming
>-----Original Message-----
>From: Wu, Hao A
>Sent: Friday, January 25, 2019 3:47 PM
>To: Chen, Chen A <chen.a.chen@intel.com>; Gao, Liming
><liming.gao@intel.com>; edk2-devel@lists.01.org
>Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Zhang, Chao B
><chao.b.zhang@intel.com>
>Subject: RE: [edk2] [PATCH 2/2] MdePkg/UefiGpt.h: Add new definition for
>enable GPT support
>
>> -----Original Message-----
>> From: Chen, Chen A
>> Sent: Friday, January 25, 2019 3:45 PM
>> To: Wu, Hao A; Gao, Liming; edk2-devel@lists.01.org
>> Cc: Kinney, Michael D; Zhang, Chao B
>> Subject: RE: [edk2] [PATCH 2/2] MdePkg/UefiGpt.h: Add new definition for
>> enable GPT support
>>
>>
>>
>> -----Original Message-----
>> From: Wu, Hao A
>> Sent: Friday, January 25, 2019 3:27 PM
>> To: Chen, Chen A <chen.a.chen@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; edk2-devel@lists.01.org
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Zhang, Chao B
>> <chao.b.zhang@intel.com>
>> Subject: RE: [edk2] [PATCH 2/2] MdePkg/UefiGpt.h: Add new definition for
>> enable GPT support
>>
>> > -----Original Message-----
>> > From: Chen, Chen A
>> > Sent: Friday, January 25, 2019 3:16 PM
>> > To: Wu, Hao A; edk2-devel@lists.01.org
>> > Cc: Kinney, Michael D; Zhang, Chao B; Gao, Liming
>> > Subject: RE: [edk2] [PATCH 2/2] MdePkg/UefiGpt.h: Add new definition
>> > for enable GPT support
>> >
>> >
>> >
>> > -----Original Message-----
>> > From: Wu, Hao A
>> > Sent: Friday, January 25, 2019 11:04 AM
>> > To: Chen, Chen A <chen.a.chen@intel.com>; edk2-devel@lists.01.org
>> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Zhang, Chao B
>> > <chao.b.zhang@intel.com>; Gao, Liming <liming.gao@intel.com>
>> > Subject: RE: [edk2] [PATCH 2/2] MdePkg/UefiGpt.h: Add new definition
>> > for enable GPT support
>> >
>> > > -----Original Message-----
>> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>> > > Of Chen A Chen
>> > > Sent: Thursday, January 17, 2019 10:03 AM
>> > > To: edk2-devel@lists.01.org
>> > > Cc: Kinney, Michael D; Zhang, Chao B; Gao, Liming
>> > > Subject: [edk2] [PATCH 2/2] MdePkg/UefiGpt.h: Add new definition for
>> > > enable GPT support
>> > >
>> > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1470
>> > > This two new definitions are defined for GPT in FatPei diver.
>> > >
>> > > Cc: Liming Gao <liming.gao@intel.com>
>> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> > > Cc: Zhang Chao B <chao.b.zhang@intel.com>
>> > > Contributed-under: TianoCore Contribution Agreement 1.1
>> > > Signed-off-by: Chen A Chen <chen.a.chen@intel.com>
>> > > ---
>> > >  MdePkg/Include/Uefi/UefiGpt.h | 16 ++++++++++++++++
>> > >  1 file changed, 16 insertions(+)
>> > >
>> > > diff --git a/MdePkg/Include/Uefi/UefiGpt.h
>> > > b/MdePkg/Include/Uefi/UefiGpt.h index f635b05390..8665c8cbc9
>100644
>> > > --- a/MdePkg/Include/Uefi/UefiGpt.h
>> > > +++ b/MdePkg/Include/Uefi/UefiGpt.h
>> > > @@ -24,9 +24,25 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
>> > ANY KIND,
>> > > EITHER EXPRESS OR IMPLIED.
>> > >  /// EFI Partition Table Signature: "EFI PART".
>> > >  ///
>> > >  #define EFI_PTAB_HEADER_ID      SIGNATURE_64 ('E','F','I',' ','P','A','R','T')
>> > > +///
>> > > +/// Minimum bytes reserve for EFI entry array buffer.
>> > > +///
>> > > +#define GPT_PART_ENTRY_MIN_SIZE 16384
>> >
>> > May I know where this definition comes from?
>> > Does it come from the UEFI spec?
>> >
>> > Chen: The MACRO is not explicitly defined in UEFI Spec, But UEFI Spec
>> > specifies a minimum value for GPT entry araray.
>>
>> Does it comes from the below content within the UEFI spec?
>>
>> > A minimum of 16,384 bytes of space must be reserved for the GPT
>> > Partition Entry Array.
>>
>> If so, I am not sure whether 'EFI_' prefix should be added before
>> 'GPT_PART_ENTRY_MIN_SIZE'.
>>
>> Liming, could you help to confirm?
>>
>> Chen: I have no strong opinion on ' GPT_PART_ENTRY_MIN_SIZE' and
>> 'EFI_GPT_PART_ENTRY_MIN_SIZE'.
>>
>> >
>> > >
>> > >  #pragma pack(1)
>> > >
>> > > +///
>> > > +/// MBR Partition Entry
>> > > +///
>> > > +typedef struct {
>> > > +  UINT8   BootIndicator;
>> > > +  UINT8   StartingCHS[3];
>> > > +  UINT8   OSType;
>> > > +  UINT8   EndingCHS[3];
>> > > +  UINT32  StartingLBA;
>> > > +  UINT32  SizeInLBA;
>> > > +} MBR_PARTITION_ENTRY;
>> > > +
>> >
>> > What about using the 'MBR_PARTITION_RECORD' definition within
>> > edk2/MdePkg/Include/IndustryStandard/Mbr.h
>> >
>> > and thus get rid of adding this one?
>> >
>> > Chen: This structure defined as the following format in Mbr.h
>> >
>> > typedef struct {
>> >   ..
>> >   UINT8 StartingLBA[4];
>> >   UINT8 SizeInLBA[4];
>> > } MBR_PARTITION_RECORD;
>> >
>> > For StartingLBA, this field is represented the LBA, so I think it
>> > should be
>> > UINT32 type not an array type.
>>
>> I do not get your point, what prevents you from getting the LBA information
>> for the byte array?
>>
>> Chen: Yes, GPT DXE driver use UNPACK_UINT32 macro to extract UINT32
>> 	from array. But I thought use UINT32 type get the value more directly.
>
>I would suggest to use the existing definition and avoid adding a
>'duplicated' definition.
>
>Best Regards,
>Hao Wu
>
>>
>> Best Regards,
>> Hao Wu
>>
>> >
>> > The same meaning is also for SizeInLba.
>> >
>> > Best Regards,
>> > Hao Wu
>> >
>> > >  ///
>> > >  /// GPT Partition Table Header.
>> > >  ///
>> > > --
>> > > 2.16.2.windows.1
>> > >
>> > > _______________________________________________
>> > > edk2-devel mailing list
>> > > edk2-devel@lists.01.org
>> > > https://lists.01.org/mailman/listinfo/edk2-devel