Re: [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD for PXE to specify MTFTP windowsize.

[Laszlo Ersek <lersek@redhat.com>]

On 09/17/18 07:43, Jiaxin Wu wrote:
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
> 
> This patch is to define one new PCD for PXE driver to specify MTFTP windowsize so as
> to improve the PXE download performance. The default value is set to 4.
> 
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Shao Ming <ming.shao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> ---
>  MdeModulePkg/MdeModulePkg.dec | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 74a699cbb7..bfc63e5fcb 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1203,10 +1203,11 @@
>    ## This setting can override the default TFTP block size. A value of 0 computes
>    # the default from MTU information. A non-zero value will be used as block size
>    # in bytes.
>    # @Prompt TFTP block size.
>    gEfiMdeModulePkgTokenSpaceGuid.PcdTftpBlockSize|0x0|UINT64|0x30001026
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdTftpWindowSize|0x4|UINT64|0x3000102A
>  
>    ## Maximum address that the DXE Core will allocate the EFI_SYSTEM_TABLE_POINTER
>    #  structure. The default value for this PCD is 0, which means that the DXE Core
>    #  will allocate the buffer from the EFI_SYSTEM_TABLE_POINTER structure on a 4MB
>    #  boundary as close to the top of memory as feasible.  If this PCD is set to a
> 

The new PCD is missing documentation -- and not just in the DEC file,
but also in the corresponding UNI file.

Furthermore, given that the PCD is only used in the next patch (which is
for NetworkPkg), the PCD should arguably be introduced to NetworkPkg.

("PcdTftpBlockSize" is different, that one is used in both
"MdeModulePkg/Universal/Network/UefiPxeBcDxe/" and
"NetworkPkg/UefiPxeBcDxe".)

... In fact, that brings me to my next question -- patch #5 only
modifies "NetworkPkg/UefiPxeBcDxe"; it doesn't modify
"MdeModulePkg/Universal/Network/UefiPxeBcDxe". The former module is
usually included as a replacement for the latter, when the platform
build enables IPv6. Is this intentional? I.e., is it the intent to *not*
bring this feature to "MdeModulePkg/Universal/Network/UefiPxeBcDxe"?

Thanks,
Laszlo