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

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

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