Re: [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

["Dong, Eric" <eric.dong@intel.com>]

Hi,

Got your points now. I think we can update the code when new library API added.

Thanks,
Eric
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Marvin Häuser
> Sent: Thursday, August 16, 2018 9:00 PM
> To: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Dong, Eric
> <eric.dong@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change
> Memory Type and address limitation.
> 
> Comments inline.
> 
> Thanks and best regards,
> Marvin.
> 
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Thursday, August 16, 2018 2:31 PM
> > To: Dong, Eric <eric.dong@intel.com>; Marvin Häuser
> > <Marvin.Haeuser@outlook.com>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change
> > Memory Type and address limitation.
> >
> > On 08/16/18 02:56, Dong, Eric wrote:
> > > Hi Marvin & Laszlo,
> > >
> > > I'm not very clear about the risk to use this function name. I think
> > > it is just
> > used in a driver as an internal function, other drivers or libraries
> > can't use it. I think we don't need to add internal prefix to all
> > internal functions in drivers, maybe need for the libraries, right?
> > Here we need to add internal prefix just because it has similar name with
> other common API?
> >
> > If I understood correctly, there were two points to Marvin's argument:
> >
> > - AllocateZeroPages() is the most likely function name that
> > "MemoryAllocationLib.h" would add, *if* it ever introduced a function
> > for "allocating boot service data pages, plus zeroing them". In that
> > case, it would cause a conflict.
> 
> Correct
> 
> >
> > - Second, because the function is defined in the same translation unit
> > where it is called from (and there are no other callers), we can make
> > the function STATIC.
> 
> Pretty much, but it was tied to the first point. There are many functions that
> could be static but aren't in edk2, so this isn't significant itself. I mentioned it
> due to my personal naming convention to ensure uniqueness.
> 
> >
> > Regarding the first concern, I don't think it's a very practical one.
> > I'm neutral on the question. My point is only that, if we really want
> > to change the name, I think we should do it separately / incrementally.
> 
> If it's supposed to be done separately, I don't see a point in fixing it either, it
> can still be fixed if such an API is ever introduced. It was meant as a
> "preventive" suggestion to be included in this series. "Just in case"
> 
> >
> > Regarding the second idea, STATIC is a generally good practice, and we
> > should do that everywhere we can. But, because I don't want to re-test
> > / re- review this series after all this effort, I suggest we do the
> > STATIC thing incrementally as well.
> 
> +1, but that's not worth an own patch to be honest. I should see whether there
> is some static analyzer that has checks for "can be static" some day.
> 
> >
> > Thanks
> > Laszlo
> >
> >
> > >
> > > Thanks,
> > > Eric
> > >
> > >> -----Original Message-----
> > >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > >> Of Laszlo Ersek
> > >> Sent: Wednesday, August 15, 2018 11:30 PM
> > >> To: Marvin Häuser <Marvin.Haeuser@outlook.com>;
> > >> edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>
> > >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > >> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change
> > >> Memory Type and address limitation.
> > >>
> > >> On 08/15/18 15:12, Marvin Häuser wrote:
> > >>> Hey Eric and anyone CC'd,
> > >>>
> > >>> Are you sure you want to name the function "AllocateZeroPages"?
> > >>> It's analogous to "AllocateZeroPool", so I could see it becoming
> > >>> an API function at some point, which will conflict with this
> > >>> definition and might silently break UefiCpuPkg compilation if not
> > >>> tested before upstreaming. I usually make any module's private
> > >>> functions static and prefix "Internal" if possible, or, if static
> > >>> cannot be used, non-static plus prefix something derived from the
> > >>> module's name to achieve uniqueness. If I am not mistaken, this
> > >>> could be made static, couldn't it?
> > >>
> > >> I agree that the function's name is not optimal, primarily because
> > >> the
> > >> Allocate*Pages() functions tend to take a page count, not a byte count.
> > >> However, I didn't want to ask for another version just because of
> > >> this; a lot of review (and now testing) has gone into this set, and
> > >> this is
> > just a wart.
> > >>
> > >> I suggest that -- after the stable tag -- we push v4 as-is;
> > >> however, Marvin, please go ahead and file a TianoCore BZ that
> > >> depends on 959 (i.e. on the BZ currently referenced in patch #5),
> > >> about fixing up the function name (and about making it static).
> > >>
> > >> Note that an "AllocateZeroPages" function exists in
> > >> "IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c" as well.
> > >> I guess both functions should be renamed (and likely not to the
> > >> same new name, because they have different parameter lists). And,
> > >> only the UefiCpuPkg one can be made static however. Either way,
> > >> both packages could be covered by the same BZ.
> > >
> > >
> > >>
> > >> Thanks
> > >> Laszlo
> > >> _______________________________________________
> > >> 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