* [Patch 1/5] MdeModulePke/Mtftp4Dxe: Support windowsize in read request operation.
2018-09-17 5:43 [Patch 0/5] Support windowsize to benefit tftp/pxe download performance Jiaxin Wu
@ 2018-09-17 5:43 ` Jiaxin Wu
2018-09-17 5:43 ` [Patch 2/5] NetworkPkg/Mtftp6Dxe: " Jiaxin Wu
` (4 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Jiaxin Wu @ 2018-09-17 5:43 UTC (permalink / raw)
To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Shao Ming, Wu Jiaxin
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
This patch is to support the TFTP windowsize option described in RFC 7440.
The feature allows the client and server to negotiate a window size of
consecutive blocks to send as an alternative for replacing the single-block
lockstep schema.
Currently, the windowsize for write request operation is not supported since
there is no real use cases.
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>
---
.../Universal/Network/Mtftp4Dxe/Mtftp4Impl.c | 5 ++
.../Universal/Network/Mtftp4Dxe/Mtftp4Impl.h | 10 ++++
.../Network/Mtftp4Dxe/Mtftp4Option.c | 25 ++++++++-
.../Network/Mtftp4Dxe/Mtftp4Option.h | 8 ++-
.../Universal/Network/Mtftp4Dxe/Mtftp4Rrq.c | 55 +++++++++++++------
.../Network/Mtftp4Dxe/Mtftp4Support.c | 8 +--
.../Network/Mtftp4Dxe/Mtftp4Support.h | 13 -----
.../Universal/Network/Mtftp4Dxe/Mtftp4Wrq.c | 2 +-
8 files changed, 89 insertions(+), 37 deletions(-)
diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c
index 03903640b8..f442e6d7ac 100644
--- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c
+++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c
@@ -78,10 +78,13 @@ Mtftp4CleanOperation (
ZeroMem (&Instance->RequestOption, sizeof (MTFTP4_OPTION));
Instance->Operation = 0;
Instance->BlkSize = MTFTP4_DEFAULT_BLKSIZE;
+ Instance->WindowSize = 1;
+ Instance->TotalBlock = 0;
+ Instance->AckedBlock = 0;
Instance->LastBlock = 0;
Instance->ServerIp = 0;
Instance->ListeningPort = 0;
Instance->ConnectedPort = 0;
Instance->Gateway = 0;
@@ -426,10 +429,11 @@ Mtftp4Start (
if (Token->OptionCount != 0) {
Status = Mtftp4ParseOption (
Token->OptionList,
Token->OptionCount,
TRUE,
+ Instance->Operation,
&Instance->RequestOption
);
if (EFI_ERROR (Status)) {
TokenStatus = EFI_DEVICE_ERROR;
@@ -441,10 +445,11 @@ Mtftp4Start (
// Set the operation parameters from the configuration or override data.
//
Config = &Instance->Config;
Instance->Token = Token;
Instance->BlkSize = MTFTP4_DEFAULT_BLKSIZE;
+ Instance->WindowSize = MTFTP4_DEFAULT_WINDOWSIZE;
CopyMem (&Instance->ServerIp, &Config->ServerIp, sizeof (IP4_ADDR));
Instance->ServerIp = NTOHL (Instance->ServerIp);
Instance->ListeningPort = Config->InitialServerPort;
diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h
index e24890cce8..de304f4e70 100644
--- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h
+++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h
@@ -6,10 +6,11 @@
RFC1350 - THE TFTP PROTOCOL (REVISION 2)
RFC2090 - TFTP Multicast Option
RFC2347 - TFTP Option Extension
RFC2348 - TFTP Blocksize Option
RFC2349 - TFTP Timeout Interval and Transfer Size Options
+ RFC7440 - TFTP Windowsize Option
Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
@@ -54,10 +55,11 @@ typedef struct _MTFTP4_PROTOCOL MTFTP4_PROTOCOL;
#define MTFTP4_DEFAULT_SERVER_PORT 69
#define MTFTP4_DEFAULT_TIMEOUT 3
#define MTFTP4_DEFAULT_RETRY 5
#define MTFTP4_DEFAULT_BLKSIZE 512
+#define MTFTP4_DEFAULT_WINDOWSIZE 1
#define MTFTP4_TIME_TO_GETMAP 5
#define MTFTP4_STATE_UNCONFIGED 0
#define MTFTP4_STATE_CONFIGED 1
#define MTFTP4_STATE_DESTROY 2
@@ -119,10 +121,18 @@ struct _MTFTP4_PROTOCOL {
//
UINT16 BlkSize;
UINT16 LastBlock;
LIST_ENTRY Blocks;
+ UINT16 WindowSize;
+
+ //
+ // Record the total received block number and the already acked block number.
+ //
+ UINT64 TotalBlock;
+ UINT64 AckedBlock;
+
//
// The server's communication end point: IP and two ports. one for
// initial request, one for its selected port.
//
IP4_ADDR ServerIp;
diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Option.c b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Option.c
index e40561a96b..2f77635268 100644
--- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Option.c
+++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Option.c
@@ -1,9 +1,9 @@
/** @file
Routines to process MTFTP4 options.
-Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
http://opensource.org/licenses/bsd-license.php<BR>
@@ -14,10 +14,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include "Mtftp4Impl.h"
CHAR8 *mMtftp4SupportedOptions[MTFTP4_SUPPORTED_OPTIONS] = {
"blksize",
+ "windowsize",
"timeout",
"tsize",
"multicast"
};
@@ -398,10 +399,11 @@ Mtftp4ExtractMcast (
@param Options The option array, which contains addresses of each
option's name/value string.
@param Count The number of options in the Options
@param Request Whether this is a request or OACK. The format of
multicast is different according to this setting.
+ @param Operation The current performed operation.
@param MtftpOption The MTFTP4_OPTION for easy access.
@retval EFI_INVALID_PARAMETER The option is mal-formated
@retval EFI_UNSUPPORTED Some option isn't supported
@retval EFI_SUCCESS The option are OK and has been parsed.
@@ -410,10 +412,11 @@ Mtftp4ExtractMcast (
EFI_STATUS
Mtftp4ParseOption (
IN EFI_MTFTP4_OPTION *Options,
IN UINT32 Count,
IN BOOLEAN Request,
+ IN UINT16 Operation,
OUT MTFTP4_OPTION *MtftpOption
)
{
EFI_STATUS Status;
UINT32 Index;
@@ -479,10 +482,26 @@ Mtftp4ParseOption (
}
}
MtftpOption->Exist |= MTFTP4_MCAST_EXIST;
+ } else if (NetStringEqualNoCase (This->OptionStr, (UINT8 *) "windowsize")) {
+ if (Operation == EFI_MTFTP4_OPCODE_WRQ) {
+ //
+ // Currently, windowsize is not supported in the write operation.
+ //
+ return EFI_UNSUPPORTED;
+ }
+
+ Value = NetStringToU32 (This->ValueStr);
+
+ if (Value < 1) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ MtftpOption->WindowSize = (UINT16) Value;
+ MtftpOption->Exist |= MTFTP4_WINDOWSIZE_EXIST;
} else if (Request) {
//
// Ignore the unsupported option if it is a reply, and return
// EFI_UNSUPPORTED if it's a request according to the UEFI spec.
//
@@ -498,10 +517,11 @@ Mtftp4ParseOption (
Parse the options in the OACK packet to MTFTP4_OPTION which program
can access directly.
@param Packet The OACK packet to parse
@param PacketLen The length of the packet
+ @param Operation The current performed operation.
@param MtftpOption The MTFTP_OPTION for easy access.
@retval EFI_INVALID_PARAMETER The packet option is mal-formated
@retval EFI_UNSUPPORTED Some option isn't supported
@retval EFI_SUCCESS The option are OK and has been parsed.
@@ -509,10 +529,11 @@ Mtftp4ParseOption (
**/
EFI_STATUS
Mtftp4ParseOptionOack (
IN EFI_MTFTP4_PACKET *Packet,
IN UINT32 PacketLen,
+ IN UINT16 Operation,
OUT MTFTP4_OPTION *MtftpOption
)
{
EFI_MTFTP4_OPTION *OptionList;
EFI_STATUS Status;
@@ -525,10 +546,10 @@ Mtftp4ParseOptionOack (
if (EFI_ERROR (Status) || (Count == 0)) {
return Status;
}
ASSERT (OptionList != NULL);
- Status = Mtftp4ParseOption (OptionList, Count, FALSE, MtftpOption);
+ Status = Mtftp4ParseOption (OptionList, Count, FALSE, Operation, MtftpOption);
FreePool (OptionList);
return Status;
}
diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Option.h b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Option.h
index a3fdc4dca2..cb1f987b93 100644
--- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Option.h
+++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Option.h
@@ -14,23 +14,25 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#ifndef __EFI_MTFTP4_OPTION_H__
#define __EFI_MTFTP4_OPTION_H__
-#define MTFTP4_SUPPORTED_OPTIONS 4
+#define MTFTP4_SUPPORTED_OPTIONS 5
#define MTFTP4_OPCODE_LEN 2
#define MTFTP4_ERRCODE_LEN 2
#define MTFTP4_BLKNO_LEN 2
#define MTFTP4_DATA_HEAD_LEN 4
#define MTFTP4_BLKSIZE_EXIST 0x01
#define MTFTP4_TIMEOUT_EXIST 0x02
#define MTFTP4_TSIZE_EXIST 0x04
#define MTFTP4_MCAST_EXIST 0x08
+#define MTFTP4_WINDOWSIZE_EXIST 0x10
typedef struct {
UINT16 BlkSize;
+ UINT16 WindowSize;
UINT8 Timeout;
UINT32 Tsize;
IP4_ADDR McastIp;
UINT16 McastPort;
BOOLEAN Master;
@@ -69,10 +71,11 @@ Mtftp4ExtractOptions (
@param Options The option array, which contains addresses of each
option's name/value string.
@param Count The number of options in the Options
@param Request Whether this is a request or OACK. The format of
multicast is different according to this setting.
+ @param Operation The current performed operation.
@param MtftpOption The MTFTP4_OPTION for easy access.
@retval EFI_INVALID_PARAMETER The option is mal-formated
@retval EFI_UNSUPPORTED Some option isn't supported
@retval EFI_SUCCESS The option are OK and has been parsed.
@@ -81,19 +84,21 @@ Mtftp4ExtractOptions (
EFI_STATUS
Mtftp4ParseOption (
IN EFI_MTFTP4_OPTION *Options,
IN UINT32 Count,
IN BOOLEAN Request,
+ IN UINT16 Operation,
OUT MTFTP4_OPTION *MtftpOption
);
/**
Parse the options in the OACK packet to MTFTP4_OPTION which program
can access directly.
@param Packet The OACK packet to parse
@param PacketLen The length of the packet
+ @param Operation The current performed operation.
@param MtftpOption The MTFTP_OPTION for easy access.
@retval EFI_INVALID_PARAMETER The packet option is mal-formated
@retval EFI_UNSUPPORTED Some option isn't supported
@retval EFI_SUCCESS The option are OK and has been parsed.
@@ -101,10 +106,11 @@ Mtftp4ParseOption (
**/
EFI_STATUS
Mtftp4ParseOptionOack (
IN EFI_MTFTP4_PACKET *Packet,
IN UINT32 PacketLen,
+ IN UINT16 Operation,
OUT MTFTP4_OPTION *MtftpOption
);
extern CHAR8 *mMtftp4SupportedOptions[MTFTP4_SUPPORTED_OPTIONS];
diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Rrq.c b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Rrq.c
index 63115ba519..fedf1cde46 100644
--- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Rrq.c
+++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Rrq.c
@@ -97,10 +97,13 @@ Mtftp4RrqSendAck (
IN UINT16 BlkNo
)
{
EFI_MTFTP4_PACKET *Ack;
NET_BUF *Packet;
+ EFI_STATUS Status;
+
+ Status = EFI_SUCCESS;
Packet = NetbufAlloc (sizeof (EFI_MTFTP4_ACK_HEADER));
if (Packet == NULL) {
return EFI_OUT_OF_RESOURCES;
}
@@ -113,11 +116,16 @@ Mtftp4RrqSendAck (
ASSERT (Ack != NULL);
Ack->Ack.OpCode = HTONS (EFI_MTFTP4_OPCODE_ACK);
Ack->Ack.Block[0] = HTONS (BlkNo);
- return Mtftp4SendPacket (Instance, Packet);
+ Status = Mtftp4SendPacket (Instance, Packet);
+ if (!EFI_ERROR (Status)) {
+ Instance->AckedBlock = Instance->TotalBlock;
+ }
+
+ return Status;
}
/**
Deliver the received data block to the user, which can be saved
@@ -144,11 +152,10 @@ Mtftp4RrqSaveBlock (
EFI_MTFTP4_TOKEN *Token;
EFI_STATUS Status;
UINT16 Block;
UINT64 Start;
UINT32 DataLen;
- UINT64 TotalBlock;
BOOLEAN Completed;
Completed = FALSE;
Token = Instance->Token;
Block = NTOHS (Packet->Data.Block);
@@ -156,11 +163,11 @@ Mtftp4RrqSaveBlock (
//
// This is the last block, save the block no
//
if (DataLen < Instance->BlkSize) {
- Completed = TRUE;
+ Completed = TRUE;
Instance->LastBlock = Block;
Mtftp4SetLastBlockNum (&Instance->Blocks, Block);
}
//
@@ -168,11 +175,11 @@ Mtftp4RrqSaveBlock (
// returns EFI_NOT_FOUND, the block has been saved, don't save it again.
// Note that : For bigger files, allowing the block counter to roll over
// to accept transfers of unlimited size. So TotalBlock is memorised as
// continuous block counter.
//
- Status = Mtftp4RemoveBlockNum (&Instance->Blocks, Block, Completed, &TotalBlock);
+ Status = Mtftp4RemoveBlockNum (&Instance->Blocks, Block, Completed, &Instance->TotalBlock);
if (Status == EFI_NOT_FOUND) {
return EFI_SUCCESS;
} else if (EFI_ERROR (Status)) {
return Status;
@@ -191,11 +198,11 @@ Mtftp4RrqSaveBlock (
return EFI_ABORTED;
}
}
if (Token->Buffer != NULL) {
- Start = MultU64x32 (TotalBlock - 1, Instance->BlkSize);
+ Start = MultU64x32 (Instance->TotalBlock - 1, Instance->BlkSize);
if (Start + DataLen <= Token->BufferSize) {
CopyMem ((UINT8 *) Token->Buffer + Start, Packet->Data.Data, DataLen);
//
@@ -255,23 +262,26 @@ Mtftp4RrqHandleData (
EFI_STATUS Status;
UINT16 BlockNum;
INTN Expected;
*Completed = FALSE;
+ Status = EFI_SUCCESS;
BlockNum = NTOHS (Packet->Data.Block);
Expected = Mtftp4GetNextBlockNum (&Instance->Blocks);
ASSERT (Expected >= 0);
//
- // If we are active and received an unexpected packet, retransmit
- // the last ACK then restart receiving. If we are passive, save
- // the block.
+ // If we are active and received an unexpected packet, transmit
+ // the ACK for the block we received, then restart receiving the
+ // expected one. If we are passive, save the block.
//
if (Instance->Master && (Expected != BlockNum)) {
- Mtftp4Retransmit (Instance);
- return EFI_SUCCESS;
+ //
+ // If Expected is 0, (UINT16) (Expected - 1) is also the expected Ack number (65535).
+ //
+ return Mtftp4RrqSendAck (Instance, (UINT16) (Expected - 1));
}
Status = Mtftp4RrqSaveBlock (Instance, Packet, Len);
if (EFI_ERROR (Status)) {
@@ -307,14 +317,17 @@ Mtftp4RrqHandleData (
} else {
BlockNum = (UINT16) (Expected - 1);
}
- Mtftp4RrqSendAck (Instance, BlockNum);
+ if (Instance->WindowSize == (Instance->TotalBlock - Instance->AckedBlock) || Expected < 0) {
+ Status = Mtftp4RrqSendAck (Instance, BlockNum);
+ }
+
}
- return EFI_SUCCESS;
+ return Status;
}
/**
Validate whether the options received in the server's OACK packet is valid.
@@ -347,15 +360,17 @@ Mtftp4RrqOackValid (
if ((Reply->Exist &~Request->Exist) != 0) {
return FALSE;
}
//
- // Server can only specify a smaller block size to be used and
+ // Server can only specify a smaller block size and window size to be used and
// return the timeout matches that requested.
//
if ((((Reply->Exist & MTFTP4_BLKSIZE_EXIST) != 0)&& (Reply->BlkSize > Request->BlkSize)) ||
- (((Reply->Exist & MTFTP4_TIMEOUT_EXIST) != 0) && (Reply->Timeout != Request->Timeout))) {
+ (((Reply->Exist & MTFTP4_WINDOWSIZE_EXIST) != 0)&& (Reply->WindowSize > Request->WindowSize)) ||
+ (((Reply->Exist & MTFTP4_TIMEOUT_EXIST) != 0) && (Reply->Timeout != Request->Timeout))
+ ) {
return FALSE;
}
//
// The server can send ",,master" to client to change its master
@@ -505,11 +520,11 @@ Mtftp4RrqHandleOack (
//
// Parse and validate the options from server
//
ZeroMem (&Reply, sizeof (MTFTP4_OPTION));
- Status = Mtftp4ParseOptionOack (Packet, Len, &Reply);
+ Status = Mtftp4ParseOptionOack (Packet, Len, Instance->Operation, &Reply);
if (EFI_ERROR (Status) ||
!Mtftp4RrqOackValid (Instance, &Reply, &Instance->RequestOption)) {
//
// Don't send an ERROR packet if the error is EFI_OUT_OF_RESOURCES.
@@ -527,11 +542,11 @@ Mtftp4RrqHandleOack (
if ((Reply.Exist & MTFTP4_MCAST_EXIST) != 0) {
//
// Save the multicast info. Always update the Master, only update the
- // multicast IP address, block size, timeoute at the first time. If IP
+ // multicast IP address, block size, window size, timeoute at the first time. If IP
// address is updated, create a UDP child to receive the multicast.
//
Instance->Master = Reply.Master;
if (Instance->McastIp == 0) {
@@ -597,10 +612,14 @@ Mtftp4RrqHandleOack (
//
if (Reply.BlkSize != 0) {
Instance->BlkSize = Reply.BlkSize;
}
+ if (Reply.WindowSize != 0) {
+ Instance->WindowSize = Reply.WindowSize;
+ }
+
if (Reply.Timeout != 0) {
Instance->Timeout = Reply.Timeout;
}
}
@@ -609,10 +628,14 @@ Mtftp4RrqHandleOack (
if (Reply.BlkSize != 0) {
Instance->BlkSize = Reply.BlkSize;
}
+ if (Reply.WindowSize != 0) {
+ Instance->WindowSize = Reply.WindowSize;
+ }
+
if (Reply.Timeout != 0) {
Instance->Timeout = Reply.Timeout;
}
}
diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.c b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.c
index 71c679ed13..71fd979b3a 100644
--- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.c
+++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.c
@@ -218,19 +218,19 @@ Mtftp4RemoveBlockNum (
// transfers of unlimited size. There is no consensus, however, whether
// the counter should wrap around to zero or to one. Many implementations
// wrap to zero, because this is the simplest to implement. Here we choose
// this solution.
//
- *TotalBlock = Num;
+ *TotalBlock = Num;
if (Range->Round > 0) {
- *TotalBlock += Range->Bound + MultU64x32 ((UINTN) (Range->Round -1), (UINT32) (Range->Bound + 1)) + 1;
- }
+ *TotalBlock += Range->Bound + MultU64x32 ((UINTN) (Range->Round -1), (UINT32) (Range->Bound + 1)) + 1;
+ }
if (Range->Start > Range->Bound) {
Range->Start = 0;
- Range->Round ++;
+ Range->Round ++;
}
if ((Range->Start > Range->End) || Completed) {
RemoveEntryList (&Range->Link);
FreePool (Range);
diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.h b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.h
index df18440a9e..6cc2756bc8 100644
--- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.h
+++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Support.h
@@ -171,23 +171,10 @@ Mtftp4SendError (
IN MTFTP4_PROTOCOL *Instance,
IN UINT16 ErrCode,
IN UINT8 *ErrInfo
);
-/**
- Retransmit the last packet for the instance.
-
- @param Instance The Mtftp instance
-
- @retval EFI_SUCCESS The last packet is retransmitted.
- @retval Others Failed to retransmit.
-
-**/
-EFI_STATUS
-Mtftp4Retransmit (
- IN MTFTP4_PROTOCOL *Instance
- );
/**
The timer ticking function in TPL_NOTIFY level for the Mtftp service instance.
@param Event The ticking event
diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Wrq.c b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Wrq.c
index cf1d50fe40..ea309e2d6b 100644
--- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Wrq.c
+++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Wrq.c
@@ -284,11 +284,11 @@ Mtftp4WrqHandleOack (
//
// Parse and validate the options from server
//
ZeroMem (&Reply, sizeof (MTFTP4_OPTION));
- Status = Mtftp4ParseOptionOack (Packet, Len, &Reply);
+ Status = Mtftp4ParseOptionOack (Packet, Len, Instance->Operation, &Reply);
if (EFI_ERROR (Status) || !Mtftp4WrqOackValid (&Reply, &Instance->RequestOption)) {
//
// Don't send a MTFTP error packet when out of resource, it can
// only make it worse.
--
2.17.1.windows.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Patch 2/5] NetworkPkg/Mtftp6Dxe: Support windowsize in read request operation.
2018-09-17 5:43 [Patch 0/5] Support windowsize to benefit tftp/pxe download performance Jiaxin Wu
2018-09-17 5:43 ` [Patch 1/5] MdeModulePke/Mtftp4Dxe: Support windowsize in read request operation Jiaxin Wu
@ 2018-09-17 5:43 ` Jiaxin Wu
2018-09-17 5:43 ` [Patch 3/5] ShellPkg/TftpDynamicCommand: Add one option for tftp command to specify windowsize Jiaxin Wu
` (3 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Jiaxin Wu @ 2018-09-17 5:43 UTC (permalink / raw)
To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Shao Ming, Wu Jiaxin
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
This patch is to support the TFTP windowsize option described in RFC 7440.
The feature allows the client and server to negotiate a window size of
consecutive blocks to send as an alternative for replacing the single-block
lockstep schema.
Currently, the windowsize for write request operation is not supported since
there is no real use cases.
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>
---
NetworkPkg/Mtftp6Dxe/Mtftp6Impl.h | 13 ++++++-
NetworkPkg/Mtftp6Dxe/Mtftp6Option.c | 22 +++++++++++-
NetworkPkg/Mtftp6Dxe/Mtftp6Option.h | 14 +++++---
NetworkPkg/Mtftp6Dxe/Mtftp6Rrq.c | 53 +++++++++++++++++++---------
NetworkPkg/Mtftp6Dxe/Mtftp6Support.c | 10 ++++++
NetworkPkg/Mtftp6Dxe/Mtftp6Wrq.c | 2 +-
6 files changed, 90 insertions(+), 24 deletions(-)
diff --git a/NetworkPkg/Mtftp6Dxe/Mtftp6Impl.h b/NetworkPkg/Mtftp6Dxe/Mtftp6Impl.h
index 6b1ce7f853..cf1b6abacc 100644
--- a/NetworkPkg/Mtftp6Dxe/Mtftp6Impl.h
+++ b/NetworkPkg/Mtftp6Dxe/Mtftp6Impl.h
@@ -1,9 +1,9 @@
/** @file
Mtftp6 internal data structure and definition declaration.
- Copyright (c) 2009 - 2012, Intel Corporation. All rights reserved. <BR>
+ Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved. <BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
http://opensource.org/licenses/bsd-license.php.
@@ -44,10 +44,11 @@ typedef struct _MTFTP6_INSTANCE MTFTP6_INSTANCE;
#define MTFTP6_DEFAULT_SERVER_CMD_PORT 69
#define MTFTP6_DEFAULT_TIMEOUT 3
#define MTFTP6_GET_MAPPING_TIMEOUT 3
#define MTFTP6_DEFAULT_MAX_RETRY 5
#define MTFTP6_DEFAULT_BLK_SIZE 512
+#define MTFTP6_DEFAULT_WINDOWSIZE 1
#define MTFTP6_TICK_PER_SECOND 10000000U
#define MTFTP6_SERVICE_FROM_THIS(a) CR (a, MTFTP6_SERVICE, ServiceBinding, MTFTP6_SERVICE_SIGNATURE)
#define MTFTP6_INSTANCE_FROM_THIS(a) CR (a, MTFTP6_INSTANCE, Mtftp6, MTFTP6_INSTANCE_SIGNATURE)
@@ -75,10 +76,20 @@ struct _MTFTP6_INSTANCE {
UINT16 BlkSize;
UINT16 LastBlk;
LIST_ENTRY BlkList;
+ UINT16 Operation;
+
+ UINT16 WindowSize;
+
+ //
+ // Record the total received block number and the already acked block number.
+ //
+ UINT64 TotalBlock;
+ UINT64 AckedBlock;
+
EFI_IPv6_ADDRESS ServerIp;
UINT16 ServerCmdPort;
UINT16 ServerDataPort;
UDP_IO *UdpIo;
diff --git a/NetworkPkg/Mtftp6Dxe/Mtftp6Option.c b/NetworkPkg/Mtftp6Dxe/Mtftp6Option.c
index 0dcf546fa8..94790e3ad6 100644
--- a/NetworkPkg/Mtftp6Dxe/Mtftp6Option.c
+++ b/NetworkPkg/Mtftp6Dxe/Mtftp6Option.c
@@ -1,9 +1,9 @@
/** @file
Mtftp6 option parse functions implementation.
- Copyright (c) 2009 - 2010, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
http://opensource.org/licenses/bsd-license.php.
@@ -15,10 +15,11 @@
#include "Mtftp6Impl.h"
CHAR8 *mMtftp6SupportedOptions[MTFTP6_SUPPORTED_OPTIONS_NUM] = {
"blksize",
+ "windowsize",
"timeout",
"tsize",
"multicast"
};
@@ -144,10 +145,11 @@ Mtftp6ParseMcastOption (
@param[in] Options The pointer to the extension options list.
@param[in] Count The num of the extension options.
@param[in] IsRequest If FALSE, the extension options is included
by a request packet.
+ @param[in] Operation The current performed operation.
@param[in] ExtInfo The pointer to the option information to be filled.
@retval EFI_SUCCESS Parse the multicast option successfully.
@retval EFI_INVALID_PARAMETER There is one option is malformatted at least.
@retval EFI_UNSUPPORTED There is one option is not supported at least.
@@ -156,10 +158,11 @@ Mtftp6ParseMcastOption (
EFI_STATUS
Mtftp6ParseExtensionOption (
IN EFI_MTFTP6_OPTION *Options,
IN UINT32 Count,
IN BOOLEAN IsRequest,
+ IN UINT16 Operation,
IN MTFTP6_EXT_OPTION_INFO *ExtInfo
)
{
EFI_STATUS Status;
EFI_MTFTP6_OPTION *Opt;
@@ -226,10 +229,27 @@ Mtftp6ParseExtensionOption (
return EFI_INVALID_PARAMETER;
}
ExtInfo->BitMap |= MTFTP6_OPT_MCAST_BIT;
+ } else if (AsciiStriCmp ((CHAR8 *) Opt->OptionStr, "windowsize") == 0) {
+ if (Operation == EFI_MTFTP6_OPCODE_WRQ) {
+ //
+ // Currently, windowsize is not supported in the write operation.
+ //
+ return EFI_UNSUPPORTED;
+ }
+
+ Value = (UINT32) AsciiStrDecimalToUintn ((CHAR8 *) Opt->ValueStr);
+
+ if ((Value < 1)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ ExtInfo->WindowSize = (UINT16) Value;
+ ExtInfo->BitMap |= MTFTP6_OPT_WINDOWSIZE_BIT;
+
} else if (IsRequest) {
//
// If it's a request, unsupported; else if it's a reply, ignore.
//
return EFI_UNSUPPORTED;
diff --git a/NetworkPkg/Mtftp6Dxe/Mtftp6Option.h b/NetworkPkg/Mtftp6Dxe/Mtftp6Option.h
index 8e2671fa21..08aa45ff63 100644
--- a/NetworkPkg/Mtftp6Dxe/Mtftp6Option.h
+++ b/NetworkPkg/Mtftp6Dxe/Mtftp6Option.h
@@ -1,9 +1,9 @@
/** @file
Mtftp6 option parse functions declaration.
- Copyright (c) 2009 - 2010, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
http://opensource.org/licenses/bsd-license.php.
@@ -24,11 +24,11 @@
#include <Library/UdpIoLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiRuntimeServicesTableLib.h>
-#define MTFTP6_SUPPORTED_OPTIONS_NUM 4
+#define MTFTP6_SUPPORTED_OPTIONS_NUM 5
#define MTFTP6_OPCODE_LEN 2
#define MTFTP6_ERRCODE_LEN 2
#define MTFTP6_BLKNO_LEN 2
#define MTFTP6_DATA_HEAD_LEN 4
@@ -37,15 +37,17 @@
//
#define MTFTP6_OPT_BLKSIZE_BIT 0x01
#define MTFTP6_OPT_TIMEOUT_BIT 0x02
#define MTFTP6_OPT_TSIZE_BIT 0x04
#define MTFTP6_OPT_MCAST_BIT 0x08
+#define MTFTP6_OPT_WINDOWSIZE_BIT 0X10
extern CHAR8 *mMtftp6SupportedOptions[MTFTP6_SUPPORTED_OPTIONS_NUM];
typedef struct {
UINT16 BlkSize;
+ UINT16 WindowSize;
UINT8 Timeout;
UINT32 Tsize;
EFI_IPv6_ADDRESS McastIp;
UINT16 McastPort;
BOOLEAN IsMaster;
@@ -74,22 +76,24 @@ Mtftp6ParseMcastOption (
@param[in] Options The pointer to the extension options list.
@param[in] Count The num of the extension options.
@param[in] IsRequest If FALSE, the extension options is included
by a request packet.
+ @param[in] Operation The current performed operation.
@param[in] ExtInfo The pointer to the option information to be filled.
- @retval EFI_SUCCESS Parse the multi-cast option successfully.
- @retval EFI_INVALID_PARAMETER An option is malformatted.
- @retval EFI_UNSUPPORTED An option is not supported.
+ @retval EFI_SUCCESS Parse the multicast option successfully.
+ @retval EFI_INVALID_PARAMETER There is one option is malformatted at least.
+ @retval EFI_UNSUPPORTED There is one option is not supported at least.
**/
EFI_STATUS
Mtftp6ParseExtensionOption (
IN EFI_MTFTP6_OPTION *Options,
IN UINT32 Count,
IN BOOLEAN IsRequest,
+ IN UINT16 Operation,
IN MTFTP6_EXT_OPTION_INFO *ExtInfo
);
/**
diff --git a/NetworkPkg/Mtftp6Dxe/Mtftp6Rrq.c b/NetworkPkg/Mtftp6Dxe/Mtftp6Rrq.c
index 2aadef076c..1f685b2bfe 100644
--- a/NetworkPkg/Mtftp6Dxe/Mtftp6Rrq.c
+++ b/NetworkPkg/Mtftp6Dxe/Mtftp6Rrq.c
@@ -1,9 +1,9 @@
/** @file
Mtftp6 Rrq process functions implementation.
- Copyright (c) 2009 - 2014, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
http://opensource.org/licenses/bsd-license.php.
@@ -33,10 +33,13 @@ Mtftp6RrqSendAck (
IN UINT16 BlockNum
)
{
EFI_MTFTP6_PACKET *Ack;
NET_BUF *Packet;
+ EFI_STATUS Status;
+
+ Status = EFI_SUCCESS;
//
// Allocate net buffer to create ack packet.
//
Packet = NetbufAlloc (sizeof (EFI_MTFTP6_ACK_HEADER));
@@ -59,11 +62,16 @@ Mtftp6RrqSendAck (
// Reset current retry count of the instance.
//
Instance->CurRetry = 0;
Instance->LastPacket = Packet;
- return Mtftp6TransmitPacket (Instance, Packet);
+ Status = Mtftp6TransmitPacket (Instance, Packet);
+ if (!EFI_ERROR (Status)) {
+ Instance->AckedBlock = Instance->TotalBlock;
+ }
+
+ return Status;
}
/**
Deliver the received data block to the user, which can be saved
@@ -92,11 +100,10 @@ Mtftp6RrqSaveBlock (
EFI_MTFTP6_TOKEN *Token;
EFI_STATUS Status;
UINT16 Block;
UINT64 Start;
UINT32 DataLen;
- UINT64 TotalBlock;
BOOLEAN Completed;
Completed = FALSE;
Token = Instance->Token;
Block = NTOHS (Packet->Data.Block);
@@ -116,11 +123,11 @@ Mtftp6RrqSaveBlock (
// returns EFI_NOT_FOUND, the block has been saved, don't save it again.
// Note that : For bigger files, allowing the block counter to roll over
// to accept transfers of unlimited size. So TotalBlock is memorised as
// continuous block counter.
//
- Status = Mtftp6RemoveBlockNum (&Instance->BlkList, Block, Completed, &TotalBlock);
+ Status = Mtftp6RemoveBlockNum (&Instance->BlkList, Block, Completed, &Instance->TotalBlock);
if (Status == EFI_NOT_FOUND) {
return EFI_SUCCESS;
} else if (EFI_ERROR (Status)) {
return Status;
@@ -152,11 +159,11 @@ Mtftp6RrqSaveBlock (
}
}
if (Token->Buffer != NULL) {
- Start = MultU64x32 (TotalBlock - 1, Instance->BlkSize);
+ Start = MultU64x32 (Instance->TotalBlock - 1, Instance->BlkSize);
if (Start + DataLen <= Token->BufferSize) {
CopyMem ((UINT8 *) Token->Buffer + Start, Packet->Data.Data, DataLen);
//
// Update the file size when received the last block
//
@@ -222,30 +229,33 @@ Mtftp6RrqHandleData (
EFI_STATUS Status;
UINT16 BlockNum;
INTN Expected;
*IsCompleted = FALSE;
+ Status = EFI_SUCCESS;
BlockNum = NTOHS (Packet->Data.Block);
Expected = Mtftp6GetNextBlockNum (&Instance->BlkList);
ASSERT (Expected >= 0);
//
- // If we are active and received an unexpected packet, retransmit
- // the last ACK then restart receiving. If we are passive, save
- // the block.
+ // If we are active and received an unexpected packet, transmit
+ // the ACK for the block we received, then restart receiving the
+ // expected one. If we are passive, save the block.
//
if (Instance->IsMaster && (Expected != BlockNum)) {
//
// Free the received packet before send new packet in ReceiveNotify,
// since the udpio might need to be reconfigured.
//
NetbufFree (*UdpPacket);
*UdpPacket = NULL;
- Mtftp6TransmitPacket (Instance, Instance->LastPacket);
- return EFI_SUCCESS;
+ //
+ // If Expected is 0, (UINT16) (Expected - 1) is also the expected Ack number (65535).
+ //
+ return Mtftp6RrqSendAck (Instance, (UINT16) (Expected - 1));
}
Status = Mtftp6RrqSaveBlock (Instance, Packet, Len, UdpPacket);
if (EFI_ERROR (Status)) {
@@ -286,14 +296,16 @@ Mtftp6RrqHandleData (
// since the udpio might need to be reconfigured.
//
NetbufFree (*UdpPacket);
*UdpPacket = NULL;
- Mtftp6RrqSendAck (Instance, BlockNum);
+ if (Instance->WindowSize == (Instance->TotalBlock - Instance->AckedBlock) || Expected < 0) {
+ Status = Mtftp6RrqSendAck (Instance, BlockNum);
+ }
}
- return EFI_SUCCESS;
+ return Status;
}
/**
Validate whether the options received in the server's OACK packet is valid.
@@ -324,16 +336,17 @@ Mtftp6RrqOackValid (
if ((ReplyInfo->BitMap & ~RequestInfo->BitMap) != 0) {
return FALSE;
}
//
- // Server can only specify a smaller block size to be used and
+ // Server can only specify a smaller block size and windowsize to be used and
// return the timeout matches that requested.
//
if ((((ReplyInfo->BitMap & MTFTP6_OPT_BLKSIZE_BIT) != 0) && (ReplyInfo->BlkSize > RequestInfo->BlkSize)) ||
+ (((ReplyInfo->BitMap & MTFTP6_OPT_WINDOWSIZE_BIT) != 0) && (ReplyInfo->BlkSize > RequestInfo->BlkSize)) ||
(((ReplyInfo->BitMap & MTFTP6_OPT_TIMEOUT_BIT) != 0) && (ReplyInfo->Timeout != RequestInfo->Timeout))
- ) {
+ ) {
return FALSE;
}
//
// The server can send ",,master" to client to change its master
@@ -483,11 +496,11 @@ Mtftp6RrqHandleOack (
ASSERT (Options != NULL);
//
// Parse the extensive options in the packet.
//
- Status = Mtftp6ParseExtensionOption (Options, Count, FALSE, &ExtInfo);
+ Status = Mtftp6ParseExtensionOption (Options, Count, FALSE, Instance->Operation, &ExtInfo);
if (EFI_ERROR (Status) || !Mtftp6RrqOackValid (Instance, &ExtInfo, &Instance->ExtInfo)) {
//
// Don't send an ERROR packet if the error is EFI_OUT_OF_RESOURCES.
//
@@ -513,11 +526,11 @@ Mtftp6RrqHandleOack (
if ((ExtInfo.BitMap & MTFTP6_OPT_MCAST_BIT) != 0) {
//
// Save the multicast info. Always update the Master, only update the
- // multicast IP address, block size, timeoute at the first time. If IP
+ // multicast IP address, block size, window size, timeoute at the first time. If IP
// address is updated, create a UDP child to receive the multicast.
//
Instance->IsMaster = ExtInfo.IsMaster;
if (NetIp6IsUnspecifiedAddr (&Instance->McastIp)) {
@@ -610,10 +623,14 @@ Mtftp6RrqHandleOack (
//
if (ExtInfo.BlkSize != 0) {
Instance->BlkSize = ExtInfo.BlkSize;
}
+ if (ExtInfo.WindowSize != 0) {
+ Instance->WindowSize = ExtInfo.WindowSize;
+ }
+
if (ExtInfo.Timeout != 0) {
Instance->Timeout = ExtInfo.Timeout;
}
}
@@ -623,10 +640,14 @@ Mtftp6RrqHandleOack (
if (ExtInfo.BlkSize != 0) {
Instance->BlkSize = ExtInfo.BlkSize;
}
+ if (ExtInfo.WindowSize != 0) {
+ Instance->WindowSize = ExtInfo.WindowSize;
+ }
+
if (ExtInfo.Timeout != 0) {
Instance->Timeout = ExtInfo.Timeout;
}
}
diff --git a/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c b/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
index 282a9c8e49..275272b89e 100644
--- a/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
+++ b/NetworkPkg/Mtftp6Dxe/Mtftp6Support.c
@@ -977,10 +977,14 @@ Mtftp6OperationClean (
Instance->ServerCmdPort = 0;
Instance->ServerDataPort = 0;
Instance->McastPort = 0;
Instance->BlkSize = 0;
+ Instance->Operation = 0;
+ Instance->WindowSize = 1;
+ Instance->TotalBlock = 0;
+ Instance->AckedBlock = 0;
Instance->LastBlk = 0;
Instance->PacketToLive = 0;
Instance->MaxRetry = 0;
Instance->CurRetry = 0;
Instance->Timeout = 0;
@@ -1049,19 +1053,22 @@ Mtftp6OperationStart (
}
Status = EFI_SUCCESS;
Instance->OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+ Instance->Operation = OpCode;
+
//
// Parse the extension options in the request packet.
//
if (Token->OptionCount != 0) {
Status = Mtftp6ParseExtensionOption (
Token->OptionList,
Token->OptionCount,
TRUE,
+ Instance->Operation,
&Instance->ExtInfo
);
if (EFI_ERROR (Status)) {
goto ON_ERROR;
@@ -1103,10 +1110,13 @@ Mtftp6OperationStart (
Instance->ServerCmdPort = MTFTP6_DEFAULT_SERVER_CMD_PORT;
}
if (Instance->BlkSize == 0) {
Instance->BlkSize = MTFTP6_DEFAULT_BLK_SIZE;
}
+ if (Instance->WindowSize == 0) {
+ Instance->WindowSize = MTFTP6_DEFAULT_WINDOWSIZE;
+ }
if (Instance->MaxRetry == 0) {
Instance->MaxRetry = MTFTP6_DEFAULT_MAX_RETRY;
}
if (Instance->Timeout == 0) {
Instance->Timeout = MTFTP6_DEFAULT_TIMEOUT;
diff --git a/NetworkPkg/Mtftp6Dxe/Mtftp6Wrq.c b/NetworkPkg/Mtftp6Dxe/Mtftp6Wrq.c
index 254b757f7e..055fbe6d1b 100644
--- a/NetworkPkg/Mtftp6Dxe/Mtftp6Wrq.c
+++ b/NetworkPkg/Mtftp6Dxe/Mtftp6Wrq.c
@@ -316,11 +316,11 @@ Mtftp6WrqHandleOack (
if (EFI_ERROR (Status)) {
return Status;
}
ASSERT (Options != NULL);
- Status = Mtftp6ParseExtensionOption (Options, Count, FALSE, &ExtInfo);
+ Status = Mtftp6ParseExtensionOption (Options, Count, FALSE, Instance->Operation, &ExtInfo);
if (EFI_ERROR(Status) || !Mtftp6WrqOackValid (&ExtInfo, &Instance->ExtInfo)) {
//
// Don't send a MTFTP error packet when out of resource, it can
// only make it worse.
--
2.17.1.windows.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Patch 3/5] ShellPkg/TftpDynamicCommand: Add one option for tftp command to specify windowsize.
2018-09-17 5:43 [Patch 0/5] Support windowsize to benefit tftp/pxe download performance Jiaxin Wu
2018-09-17 5:43 ` [Patch 1/5] MdeModulePke/Mtftp4Dxe: Support windowsize in read request operation Jiaxin Wu
2018-09-17 5:43 ` [Patch 2/5] NetworkPkg/Mtftp6Dxe: " Jiaxin Wu
@ 2018-09-17 5:43 ` Jiaxin Wu
2018-09-17 22:18 ` Carsey, Jaben
2018-09-17 5:43 ` [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD for PXE to specify MTFTP windowsize Jiaxin Wu
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Jiaxin Wu @ 2018-09-17 5:43 UTC (permalink / raw)
To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Carsey Jaben, Shao Ming, Wu Jiaxin
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
This patch is to define one new option for TFTP shell command to specify the
windowsize option as defined in RFC 7440. Valid range is between 1 and 64,
default value is 1.
Note that: RFC 7440 does not mention max window size value, but for the
stability reason, the value is limited to 64.
Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Carsey Jaben <jaben.carsey@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>
---
.../DynamicCommand/TftpDynamicCommand/Tftp.c | 65 ++++++++++++++++---
.../TftpDynamicCommand/Tftp.uni | 6 +-
2 files changed, 59 insertions(+), 12 deletions(-)
diff --git a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
index 44be6d4e76..c66be6b9d9 100644
--- a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
+++ b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
@@ -181,10 +181,11 @@ DownloadFile (
IN EFI_MTFTP4_PROTOCOL *Mtftp4,
IN CONST CHAR16 *FilePath,
IN CONST CHAR8 *AsciiFilePath,
IN UINTN FileSize,
IN UINT16 BlockSize,
+ IN UINT16 WindowSize,
OUT VOID **Data
);
/**
Update the progress of a file download
@@ -225,10 +226,11 @@ STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
{L"-l", TypeValue},
{L"-r", TypeValue},
{L"-c", TypeValue},
{L"-t", TypeValue},
{L"-s", TypeValue},
+ {L"-w", TypeValue},
{NULL , TypeMax}
};
///
/// The default block size (512) of tftp is defined in the RFC1350.
@@ -237,11 +239,21 @@ STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
///
/// The valid range of block size option is defined in the RFC2348.
///
#define MTFTP_MIN_BLKSIZE 8
#define MTFTP_MAX_BLKSIZE 65464
-
+///
+/// The default windowsize (1) of tftp.
+///
+#define MTFTP_DEFAULT_WINDOWSIZE 1
+///
+/// The valid range of window size option.
+/// Note that: RFC 7440 does not mention max window size value, but for the
+/// stability reason, the value is limited to 64.
+///
+#define MTFTP_MIN_WINDOWSIZE 1
+#define MTFTP_MAX_WINDOWSIZE 64
/**
Function for 'tftp' command.
@param[in] ImageHandle Handle to the Image (NULL if Internal).
@@ -286,19 +298,21 @@ RunTftp (
UINTN FileSize;
UINTN DataSize;
VOID *Data;
SHELL_FILE_HANDLE FileHandle;
UINT16 BlockSize;
+ UINT16 WindowSize;
ShellStatus = SHELL_INVALID_PARAMETER;
ProblemParam = NULL;
NicFound = FALSE;
AsciiRemoteFilePath = NULL;
Handles = NULL;
FileSize = 0;
DataSize = 0;
BlockSize = MTFTP_DEFAULT_BLKSIZE;
+ WindowSize = MTFTP_DEFAULT_WINDOWSIZE;
//
// Initialize the Shell library (we must be in non-auto-init...)
//
Status = ShellInitialize ();
@@ -434,10 +448,24 @@ RunTftp (
);
goto Error;
}
}
+ ValueStr = ShellCommandLineGetValue (CheckPackage, L"-w");
+ if (ValueStr != NULL) {
+ if (!StringToUint16 (ValueStr, &WindowSize)) {
+ goto Error;
+ }
+ if (WindowSize < MTFTP_MIN_WINDOWSIZE || WindowSize > MTFTP_MAX_WINDOWSIZE) {
+ ShellPrintHiiEx (
+ -1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
+ mTftpHiiHandle, L"tftp", ValueStr
+ );
+ goto Error;
+ }
+ }
+
//
// Locate all MTFTP4 Service Binding protocols
//
ShellStatus = SHELL_NOT_FOUND;
Status = gBS->LocateHandleBuffer (
@@ -508,11 +536,11 @@ RunTftp (
mTftpHiiHandle, RemoteFilePath, NicName, Status
);
goto NextHandle;
}
- Status = DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePath, FileSize, BlockSize, &Data);
+ Status = DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePath, FileSize, BlockSize, WindowSize, &Data);
if (EFI_ERROR (Status)) {
ShellPrintHiiEx (
-1, -1, NULL, STRING_TOKEN (STR_TFTP_ERR_DOWNLOAD),
mTftpHiiHandle, RemoteFilePath, NicName, Status
);
@@ -894,20 +922,21 @@ DownloadFile (
IN EFI_MTFTP4_PROTOCOL *Mtftp4,
IN CONST CHAR16 *FilePath,
IN CONST CHAR8 *AsciiFilePath,
IN UINTN FileSize,
IN UINT16 BlockSize,
+ IN UINT16 WindowSize,
OUT VOID **Data
)
{
EFI_STATUS Status;
EFI_PHYSICAL_ADDRESS PagesAddress;
VOID *Buffer;
DOWNLOAD_CONTEXT *TftpContext;
EFI_MTFTP4_TOKEN Mtftp4Token;
- EFI_MTFTP4_OPTION ReqOpt;
- UINT8 OptBuf[10];
+ UINT8 BlksizeBuf[10];
+ UINT8 WindowsizeBuf[10];
// Downloaded file can be large. BS.AllocatePages() is more faster
// than AllocatePool() and avoid fragmentation.
// The downloaded file could be an EFI application. Marking the
// allocated page as EfiBootServicesCode would allow to execute a
@@ -936,17 +965,29 @@ DownloadFile (
Mtftp4Token.Filename = (UINT8*)AsciiFilePath;
Mtftp4Token.BufferSize = FileSize;
Mtftp4Token.Buffer = Buffer;
Mtftp4Token.CheckPacket = CheckPacket;
Mtftp4Token.Context = (VOID*)TftpContext;
+ Mtftp4Token.OptionCount = 0;
+ Mtftp4Token.OptionList = AllocatePool (sizeof (EFI_MTFTP4_OPTION) * 2);
+ if (Mtftp4Token.OptionList == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Error;
+ }
+
if (BlockSize != MTFTP_DEFAULT_BLKSIZE) {
- ReqOpt.OptionStr = (UINT8 *) "blksize";
- AsciiSPrint ((CHAR8 *)OptBuf, sizeof (OptBuf), "%d", BlockSize);
- ReqOpt.ValueStr = OptBuf;
+ Mtftp4Token.OptionList[Mtftp4Token.OptionCount].OptionStr = (UINT8 *) "blksize";
+ AsciiSPrint ((CHAR8 *) BlksizeBuf, sizeof (BlksizeBuf), "%d", BlockSize);
+ Mtftp4Token.OptionList[Mtftp4Token.OptionCount].ValueStr = BlksizeBuf;
+ Mtftp4Token.OptionCount ++;
+ }
- Mtftp4Token.OptionCount = 1;
- Mtftp4Token.OptionList = &ReqOpt;
+ if (WindowSize != MTFTP_DEFAULT_WINDOWSIZE) {
+ Mtftp4Token.OptionList[Mtftp4Token.OptionCount].OptionStr = (UINT8 *) "windowsize";
+ AsciiSPrint ((CHAR8 *) WindowsizeBuf, sizeof (WindowsizeBuf), "%d", WindowSize);
+ Mtftp4Token.OptionList[Mtftp4Token.OptionCount].ValueStr = WindowsizeBuf;
+ Mtftp4Token.OptionCount ++;
}
ShellPrintHiiEx (
-1, -1, NULL, STRING_TOKEN (STR_TFTP_DOWNLOADING),
mTftpHiiHandle, FilePath
@@ -958,14 +999,18 @@ DownloadFile (
mTftpHiiHandle
);
Error :
- if (TftpContext == NULL) {
+ if (TftpContext != NULL) {
FreePool (TftpContext);
}
+ if (Mtftp4Token.OptionList != NULL) {
+ FreePool (Mtftp4Token.OptionList);
+ }
+
if (EFI_ERROR (Status)) {
gBS->FreePages (PagesAddress, EFI_SIZE_TO_PAGES (FileSize));
return Status;
}
diff --git a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.uni b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.uni
index 1393ba5679..654e42ad23 100644
--- a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.uni
+++ b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.uni
@@ -1,9 +1,9 @@
// /**
//
// (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP<BR>
-// Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved. <BR>
+// Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved. <BR>
// This program and the accompanying materials
// are licensed and made available under the terms and conditions of the BSD License
// which accompanies this distribution. The full text of the license may be found at
// http://opensource.org/licenses/bsd-license.php
//
@@ -48,11 +48,11 @@
".SH NAME\r\n"
"Download a file from TFTP server.\r\n"
".SH SYNOPSIS\r\n"
" \r\n"
"TFTP [-i interface] [-l <port>] [-r <port>] [-c <retry count>] [-t <timeout>]\r\n"
-" [-s <block size>] host remotefilepath [localfilepath]\r\n"
+" [-s <block size>] [-w <window size>] host remotefilepath [localfilepath]\r\n"
".SH OPTIONS\r\n"
" \r\n"
" -i interface - Specifies an adapter name, i.e., eth0.\r\n"
" -l port - Specifies the local port number. Default value is 0\r\n"
" and the port number is automatically assigned.\r\n"
@@ -61,10 +61,12 @@
" wait for a response. The default value is 6.\r\n"
" -t <timeout> - The number of seconds to wait for a response after\r\n"
" sending a request packet. Default value is 4s.\r\n"
" -s <block size> - Specifies the TFTP blksize option as defined in RFC 2348.\r\n"
" Valid range is between 8 and 65464, default value is 512.\r\n"
+" -w <window size> - Specifies the TFTP windowsize option as defined in RFC 7440.\r\n"
+" Valid range is between 1 and 64, default value is 1.\r\n"
" host - Specify TFTP Server IPv4 address.\r\n"
" remotefilepath - TFTP server file path to download the file.\r\n"
" localfilepath - Local destination file path.\r\n"
".SH DESCRIPTION\r\n"
" \r\n"
--
2.17.1.windows.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Patch 3/5] ShellPkg/TftpDynamicCommand: Add one option for tftp command to specify windowsize.
2018-09-17 5:43 ` [Patch 3/5] ShellPkg/TftpDynamicCommand: Add one option for tftp command to specify windowsize Jiaxin Wu
@ 2018-09-17 22:18 ` Carsey, Jaben
0 siblings, 0 replies; 17+ messages in thread
From: Carsey, Jaben @ 2018-09-17 22:18 UTC (permalink / raw)
To: Wu, Jiaxin, edk2-devel@lists.01.org; +Cc: Ye, Ting, Fu, Siyuan, Shao, Ming
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Sunday, September 16, 2018 10:44 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Carsey,
> Jaben <jaben.carsey@intel.com>; Shao, Ming <ming.shao@intel.com>; Wu,
> Jiaxin <jiaxin.wu@intel.com>
> Subject: [Patch 3/5] ShellPkg/TftpDynamicCommand: Add one option for tftp
> command to specify windowsize.
> Importance: High
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
>
> This patch is to define one new option for TFTP shell command to specify the
> windowsize option as defined in RFC 7440. Valid range is between 1 and 64,
> default value is 1.
>
> Note that: RFC 7440 does not mention max window size value, but for the
> stability reason, the value is limited to 64.
>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Carsey Jaben <jaben.carsey@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>
> ---
> .../DynamicCommand/TftpDynamicCommand/Tftp.c | 65
> ++++++++++++++++---
> .../TftpDynamicCommand/Tftp.uni | 6 +-
> 2 files changed, 59 insertions(+), 12 deletions(-)
>
> diff --git a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> index 44be6d4e76..c66be6b9d9 100644
> --- a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> +++ b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> @@ -181,10 +181,11 @@ DownloadFile (
> IN EFI_MTFTP4_PROTOCOL *Mtftp4,
> IN CONST CHAR16 *FilePath,
> IN CONST CHAR8 *AsciiFilePath,
> IN UINTN FileSize,
> IN UINT16 BlockSize,
> + IN UINT16 WindowSize,
> OUT VOID **Data
> );
>
> /**
> Update the progress of a file download
> @@ -225,10 +226,11 @@ STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
> {L"-l", TypeValue},
> {L"-r", TypeValue},
> {L"-c", TypeValue},
> {L"-t", TypeValue},
> {L"-s", TypeValue},
> + {L"-w", TypeValue},
> {NULL , TypeMax}
> };
>
> ///
> /// The default block size (512) of tftp is defined in the RFC1350.
> @@ -237,11 +239,21 @@ STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
> ///
> /// The valid range of block size option is defined in the RFC2348.
> ///
> #define MTFTP_MIN_BLKSIZE 8
> #define MTFTP_MAX_BLKSIZE 65464
> -
> +///
> +/// The default windowsize (1) of tftp.
> +///
> +#define MTFTP_DEFAULT_WINDOWSIZE 1
> +///
> +/// The valid range of window size option.
> +/// Note that: RFC 7440 does not mention max window size value, but for
> the
> +/// stability reason, the value is limited to 64.
> +///
> +#define MTFTP_MIN_WINDOWSIZE 1
> +#define MTFTP_MAX_WINDOWSIZE 64
>
> /**
> Function for 'tftp' command.
>
> @param[in] ImageHandle Handle to the Image (NULL if Internal).
> @@ -286,19 +298,21 @@ RunTftp (
> UINTN FileSize;
> UINTN DataSize;
> VOID *Data;
> SHELL_FILE_HANDLE FileHandle;
> UINT16 BlockSize;
> + UINT16 WindowSize;
>
> ShellStatus = SHELL_INVALID_PARAMETER;
> ProblemParam = NULL;
> NicFound = FALSE;
> AsciiRemoteFilePath = NULL;
> Handles = NULL;
> FileSize = 0;
> DataSize = 0;
> BlockSize = MTFTP_DEFAULT_BLKSIZE;
> + WindowSize = MTFTP_DEFAULT_WINDOWSIZE;
>
> //
> // Initialize the Shell library (we must be in non-auto-init...)
> //
> Status = ShellInitialize ();
> @@ -434,10 +448,24 @@ RunTftp (
> );
> goto Error;
> }
> }
>
> + ValueStr = ShellCommandLineGetValue (CheckPackage, L"-w");
> + if (ValueStr != NULL) {
> + if (!StringToUint16 (ValueStr, &WindowSize)) {
> + goto Error;
> + }
> + if (WindowSize < MTFTP_MIN_WINDOWSIZE || WindowSize >
> MTFTP_MAX_WINDOWSIZE) {
> + ShellPrintHiiEx (
> + -1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
> + mTftpHiiHandle, L"tftp", ValueStr
> + );
> + goto Error;
> + }
> + }
> +
> //
> // Locate all MTFTP4 Service Binding protocols
> //
> ShellStatus = SHELL_NOT_FOUND;
> Status = gBS->LocateHandleBuffer (
> @@ -508,11 +536,11 @@ RunTftp (
> mTftpHiiHandle, RemoteFilePath, NicName, Status
> );
> goto NextHandle;
> }
>
> - Status = DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePath,
> FileSize, BlockSize, &Data);
> + Status = DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePath,
> FileSize, BlockSize, WindowSize, &Data);
> if (EFI_ERROR (Status)) {
> ShellPrintHiiEx (
> -1, -1, NULL, STRING_TOKEN (STR_TFTP_ERR_DOWNLOAD),
> mTftpHiiHandle, RemoteFilePath, NicName, Status
> );
> @@ -894,20 +922,21 @@ DownloadFile (
> IN EFI_MTFTP4_PROTOCOL *Mtftp4,
> IN CONST CHAR16 *FilePath,
> IN CONST CHAR8 *AsciiFilePath,
> IN UINTN FileSize,
> IN UINT16 BlockSize,
> + IN UINT16 WindowSize,
> OUT VOID **Data
> )
> {
> EFI_STATUS Status;
> EFI_PHYSICAL_ADDRESS PagesAddress;
> VOID *Buffer;
> DOWNLOAD_CONTEXT *TftpContext;
> EFI_MTFTP4_TOKEN Mtftp4Token;
> - EFI_MTFTP4_OPTION ReqOpt;
> - UINT8 OptBuf[10];
> + UINT8 BlksizeBuf[10];
> + UINT8 WindowsizeBuf[10];
>
> // Downloaded file can be large. BS.AllocatePages() is more faster
> // than AllocatePool() and avoid fragmentation.
> // The downloaded file could be an EFI application. Marking the
> // allocated page as EfiBootServicesCode would allow to execute a
> @@ -936,17 +965,29 @@ DownloadFile (
> Mtftp4Token.Filename = (UINT8*)AsciiFilePath;
> Mtftp4Token.BufferSize = FileSize;
> Mtftp4Token.Buffer = Buffer;
> Mtftp4Token.CheckPacket = CheckPacket;
> Mtftp4Token.Context = (VOID*)TftpContext;
> + Mtftp4Token.OptionCount = 0;
> + Mtftp4Token.OptionList = AllocatePool (sizeof (EFI_MTFTP4_OPTION) *
> 2);
> + if (Mtftp4Token.OptionList == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto Error;
> + }
> +
> if (BlockSize != MTFTP_DEFAULT_BLKSIZE) {
> - ReqOpt.OptionStr = (UINT8 *) "blksize";
> - AsciiSPrint ((CHAR8 *)OptBuf, sizeof (OptBuf), "%d", BlockSize);
> - ReqOpt.ValueStr = OptBuf;
> + Mtftp4Token.OptionList[Mtftp4Token.OptionCount].OptionStr = (UINT8
> *) "blksize";
> + AsciiSPrint ((CHAR8 *) BlksizeBuf, sizeof (BlksizeBuf), "%d", BlockSize);
> + Mtftp4Token.OptionList[Mtftp4Token.OptionCount].ValueStr =
> BlksizeBuf;
> + Mtftp4Token.OptionCount ++;
> + }
>
> - Mtftp4Token.OptionCount = 1;
> - Mtftp4Token.OptionList = &ReqOpt;
> + if (WindowSize != MTFTP_DEFAULT_WINDOWSIZE) {
> + Mtftp4Token.OptionList[Mtftp4Token.OptionCount].OptionStr = (UINT8
> *) "windowsize";
> + AsciiSPrint ((CHAR8 *) WindowsizeBuf, sizeof (WindowsizeBuf), "%d",
> WindowSize);
> + Mtftp4Token.OptionList[Mtftp4Token.OptionCount].ValueStr =
> WindowsizeBuf;
> + Mtftp4Token.OptionCount ++;
> }
>
> ShellPrintHiiEx (
> -1, -1, NULL, STRING_TOKEN (STR_TFTP_DOWNLOADING),
> mTftpHiiHandle, FilePath
> @@ -958,14 +999,18 @@ DownloadFile (
> mTftpHiiHandle
> );
>
> Error :
>
> - if (TftpContext == NULL) {
> + if (TftpContext != NULL) {
> FreePool (TftpContext);
> }
>
> + if (Mtftp4Token.OptionList != NULL) {
> + FreePool (Mtftp4Token.OptionList);
> + }
> +
> if (EFI_ERROR (Status)) {
> gBS->FreePages (PagesAddress, EFI_SIZE_TO_PAGES (FileSize));
> return Status;
> }
>
> diff --git a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.uni
> b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.uni
> index 1393ba5679..654e42ad23 100644
> --- a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.uni
> +++ b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.uni
> @@ -1,9 +1,9 @@
> // /**
> //
> // (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP<BR>
> -// Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved. <BR>
> +// Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved. <BR>
> // This program and the accompanying materials
> // are licensed and made available under the terms and conditions of the
> BSD License
> // which accompanies this distribution. The full text of the license may be
> found at
> // http://opensource.org/licenses/bsd-license.php
> //
> @@ -48,11 +48,11 @@
> ".SH NAME\r\n"
> "Download a file from TFTP server.\r\n"
> ".SH SYNOPSIS\r\n"
> " \r\n"
> "TFTP [-i interface] [-l <port>] [-r <port>] [-c <retry count>] [-t
> <timeout>]\r\n"
> -" [-s <block size>] host remotefilepath [localfilepath]\r\n"
> +" [-s <block size>] [-w <window size>] host remotefilepath
> [localfilepath]\r\n"
> ".SH OPTIONS\r\n"
> " \r\n"
> " -i interface - Specifies an adapter name, i.e., eth0.\r\n"
> " -l port - Specifies the local port number. Default value is 0\r\n"
> " and the port number is automatically assigned.\r\n"
> @@ -61,10 +61,12 @@
> " wait for a response. The default value is 6.\r\n"
> " -t <timeout> - The number of seconds to wait for a response after\r\n"
> " sending a request packet. Default value is 4s.\r\n"
> " -s <block size> - Specifies the TFTP blksize option as defined in RFC
> 2348.\r\n"
> " Valid range is between 8 and 65464, default value is 512.\r\n"
> +" -w <window size> - Specifies the TFTP windowsize option as defined in
> RFC 7440.\r\n"
> +" Valid range is between 1 and 64, default value is 1.\r\n"
> " host - Specify TFTP Server IPv4 address.\r\n"
> " remotefilepath - TFTP server file path to download the file.\r\n"
> " localfilepath - Local destination file path.\r\n"
> ".SH DESCRIPTION\r\n"
> " \r\n"
> --
> 2.17.1.windows.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD for PXE to specify MTFTP windowsize.
2018-09-17 5:43 [Patch 0/5] Support windowsize to benefit tftp/pxe download performance Jiaxin Wu
` (2 preceding siblings ...)
2018-09-17 5:43 ` [Patch 3/5] ShellPkg/TftpDynamicCommand: Add one option for tftp command to specify windowsize Jiaxin Wu
@ 2018-09-17 5:43 ` Jiaxin Wu
2018-09-18 11:04 ` Laszlo Ersek
2018-09-19 0:38 ` Fu, Siyuan
2018-09-17 5:43 ` [Patch 5/5] NetworkPkg/UefiPxeBcDxe: Use the specified " Jiaxin Wu
2018-09-18 11:23 ` [Patch 0/5] Support windowsize to benefit tftp/pxe download performance Laszlo Ersek
5 siblings, 2 replies; 17+ messages in thread
From: Jiaxin Wu @ 2018-09-17 5:43 UTC (permalink / raw)
To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Shao Ming, Wu Jiaxin
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
--
2.17.1.windows.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD for PXE to specify MTFTP windowsize.
2018-09-17 5:43 ` [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD for PXE to specify MTFTP windowsize Jiaxin Wu
@ 2018-09-18 11:04 ` Laszlo Ersek
2018-09-19 1:55 ` Wu, Jiaxin
2018-09-19 0:38 ` Fu, Siyuan
1 sibling, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2018-09-18 11:04 UTC (permalink / raw)
To: Jiaxin Wu, edk2-devel; +Cc: Ye Ting, Shao Ming, Fu Siyuan
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD for PXE to specify MTFTP windowsize.
2018-09-18 11:04 ` Laszlo Ersek
@ 2018-09-19 1:55 ` Wu, Jiaxin
0 siblings, 0 replies; 17+ messages in thread
From: Wu, Jiaxin @ 2018-09-19 1:55 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ye, Ting, Shao, Ming, Fu, Siyuan
> Subject: Re: [edk2] [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define
> one PCD for PXE to specify MTFTP windowsize.
>
> 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|0x30001
> 026
> > +
> gEfiMdeModulePkgTokenSpaceGuid.PcdTftpWindowSize|0x4|UINT64|0x30
> 00102A
> >
> > ## 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.
>
Agree. I will fix that in the version patch.
> 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".)
>
Agree, I will move the new PCD into the networkpkg.
> ... 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"?
>
Yes, we only add the new feature into the combo driver.
> Thanks,
> Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD for PXE to specify MTFTP windowsize.
2018-09-17 5:43 ` [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD for PXE to specify MTFTP windowsize Jiaxin Wu
2018-09-18 11:04 ` Laszlo Ersek
@ 2018-09-19 0:38 ` Fu, Siyuan
2018-09-19 2:21 ` Wu, Jiaxin
1 sibling, 1 reply; 17+ messages in thread
From: Fu, Siyuan @ 2018-09-19 0:38 UTC (permalink / raw)
To: Wu, Jiaxin, edk2-devel@lists.01.org; +Cc: Ye, Ting, Shao, Ming
Hi, Jiaxin
I think we should also update the comments for PcdTftpWindowSize usage.
And could we change the name to PcdPxeTftpWindowSize since it only impacts the PXE tftp download?
BestRegards
Fu Siyuan
> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Monday, September 17, 2018 1:44 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Shao,
> Ming <ming.shao@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD for PXE
> to specify MTFTP windowsize.
>
> 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
> --
> 2.17.1.windows.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD for PXE to specify MTFTP windowsize.
2018-09-19 0:38 ` Fu, Siyuan
@ 2018-09-19 2:21 ` Wu, Jiaxin
0 siblings, 0 replies; 17+ messages in thread
From: Wu, Jiaxin @ 2018-09-19 2:21 UTC (permalink / raw)
To: Fu, Siyuan, edk2-devel@lists.01.org; +Cc: Ye, Ting, Shao, Ming
Thanks Siyuan, I will rename the PCD name for better understanding.
> -----Original Message-----
> From: Fu, Siyuan
> Sent: Wednesday, September 19, 2018 8:38 AM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Shao, Ming <ming.shao@intel.com>
> Subject: RE: [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one
> PCD for PXE to specify MTFTP windowsize.
>
> Hi, Jiaxin
>
> I think we should also update the comments for PcdTftpWindowSize usage.
> And could we change the name to PcdPxeTftpWindowSize since it only
> impacts the PXE tftp download?
>
> BestRegards
> Fu Siyuan
>
> > -----Original Message-----
> > From: Wu, Jiaxin
> > Sent: Monday, September 17, 2018 1:44 PM
> > To: edk2-devel@lists.01.org
> > Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Shao,
> > Ming <ming.shao@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> > Subject: [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD
> for PXE
> > to specify MTFTP windowsize.
> >
> > 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|0x30001
> 026
> > +
> gEfiMdeModulePkgTokenSpaceGuid.PcdTftpWindowSize|0x4|UINT64|0x30
> 00102A
> >
> > ## 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
> > --
> > 2.17.1.windows.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Patch 5/5] NetworkPkg/UefiPxeBcDxe: Use the specified MTFTP windowsize.
2018-09-17 5:43 [Patch 0/5] Support windowsize to benefit tftp/pxe download performance Jiaxin Wu
` (3 preceding siblings ...)
2018-09-17 5:43 ` [Patch 4/5] MdeModulePkg/MdeModulePkg.dec: Define one PCD for PXE to specify MTFTP windowsize Jiaxin Wu
@ 2018-09-17 5:43 ` Jiaxin Wu
2018-09-18 11:23 ` [Patch 0/5] Support windowsize to benefit tftp/pxe download performance Laszlo Ersek
5 siblings, 0 replies; 17+ messages in thread
From: Jiaxin Wu @ 2018-09-17 5:43 UTC (permalink / raw)
To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Shao Ming, Wu Jiaxin
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
This patch is to use the specified MTFTP windowsize to benefit the PXE
download performance.
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>
---
NetworkPkg/UefiPxeBcDxe/PxeBcImpl.c | 10 +-
NetworkPkg/UefiPxeBcDxe/PxeBcMtftp.c | 137 +++++++++++++++++------
NetworkPkg/UefiPxeBcDxe/PxeBcMtftp.h | 6 +-
NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf | 2 +
4 files changed, 120 insertions(+), 35 deletions(-)
diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcImpl.c b/NetworkPkg/UefiPxeBcDxe/PxeBcImpl.c
index 13396903f5..db463d1b11 100644
--- a/NetworkPkg/UefiPxeBcDxe/PxeBcImpl.c
+++ b/NetworkPkg/UefiPxeBcDxe/PxeBcImpl.c
@@ -847,11 +847,11 @@ EfiPxeBcMtftp (
EFI_MTFTP4_CONFIG_DATA Mtftp4Config;
EFI_MTFTP6_CONFIG_DATA Mtftp6Config;
VOID *Config;
EFI_STATUS Status;
EFI_PXE_BASE_CODE_IP_FILTER IpFilter;
-
+ UINTN WindowSize;
if ((This == NULL) ||
(Filename == NULL) ||
(BufferSize == NULL) ||
(ServerIp == NULL) ||
@@ -871,10 +871,15 @@ EfiPxeBcMtftp (
Config = NULL;
Status = EFI_DEVICE_ERROR;
Private = PXEBC_PRIVATE_DATA_FROM_PXEBC (This);
Mode = Private->PxeBc.Mode;
+ //
+ // Get PcdTftpWindowSize.
+ //
+ WindowSize = (UINTN) PcdGet64 (PcdTftpWindowSize);
+
if (Mode->UsingIpv6) {
if (!NetIp6IsValidUnicast (&ServerIp->v6)) {
return EFI_INVALID_PARAMETER;
}
} else {
@@ -928,10 +933,11 @@ EfiPxeBcMtftp (
Status = PxeBcTftpGetFileSize (
Private,
Config,
Filename,
BlockSize,
+ (WindowSize > 1) ? &WindowSize : NULL,
BufferSize
);
break;
@@ -942,10 +948,11 @@ EfiPxeBcMtftp (
Status = PxeBcTftpReadFile (
Private,
Config,
Filename,
BlockSize,
+ (WindowSize > 1) ? &WindowSize : NULL,
BufferPtr,
BufferSize,
DontUseBuffer
);
@@ -974,10 +981,11 @@ EfiPxeBcMtftp (
Status = PxeBcTftpReadDirectory (
Private,
Config,
Filename,
BlockSize,
+ (WindowSize > 1) ? &WindowSize : NULL,
BufferPtr,
BufferSize,
DontUseBuffer
);
diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcMtftp.c b/NetworkPkg/UefiPxeBcDxe/PxeBcMtftp.c
index 270190d42e..9725fb40dd 100644
--- a/NetworkPkg/UefiPxeBcDxe/PxeBcMtftp.c
+++ b/NetworkPkg/UefiPxeBcDxe/PxeBcMtftp.c
@@ -17,11 +17,12 @@
CHAR8 *mMtftpOptions[PXE_MTFTP_OPTION_MAXIMUM_INDEX] = {
"blksize",
"timeout",
"tsize",
- "multicast"
+ "multicast",
+ "windowsize"
};
/**
This is a callback function when packets are received or transmitted in Mtftp driver.
@@ -120,28 +121,31 @@ EFI_STATUS
PxeBcMtftp6GetFileSize (
IN PXEBC_PRIVATE_DATA *Private,
IN EFI_MTFTP6_CONFIG_DATA *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN OUT UINT64 *BufferSize
)
{
EFI_MTFTP6_PROTOCOL *Mtftp6;
- EFI_MTFTP6_OPTION ReqOpt[2];
+ EFI_MTFTP6_OPTION ReqOpt[3];
EFI_MTFTP6_PACKET *Packet;
EFI_MTFTP6_OPTION *Option;
UINT32 PktLen;
- UINT8 OptBuf[128];
+ UINT8 OptBuf[PXE_MTFTP_OPTBUF_MAXNUM_INDEX];
+ UINTN OptBufSize;
UINT32 OptCnt;
EFI_STATUS Status;
*BufferSize = 0;
Status = EFI_DEVICE_ERROR;
Mtftp6 = Private->Mtftp6;
Packet = NULL;
Option = NULL;
PktLen = 0;
+ OptBufSize = PXE_MTFTP_OPTBUF_MAXNUM_INDEX;
OptCnt = 1;
Config->InitialServerPort = PXEBC_BS_DOWNLOAD_PORT;
Status = Mtftp6->Configure (Mtftp6, Config);
if (EFI_ERROR (Status)) {
@@ -150,17 +154,26 @@ PxeBcMtftp6GetFileSize (
//
// Build the required options for get info.
//
ReqOpt[0].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_TSIZE_INDEX];
- PxeBcUintnToAscDec (0, OptBuf, PXE_MTFTP_OPTBUF_MAXNUM_INDEX);
+ PxeBcUintnToAscDec (0, OptBuf, OptBufSize);
ReqOpt[0].ValueStr = OptBuf;
if (BlockSize != NULL) {
- ReqOpt[1].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
- ReqOpt[1].ValueStr = (UINT8 *) (ReqOpt[0].ValueStr + AsciiStrLen ((CHAR8 *) ReqOpt[0].ValueStr) + 1);
- PxeBcUintnToAscDec (*BlockSize, ReqOpt[1].ValueStr, PXE_MTFTP_OPTBUF_MAXNUM_INDEX - (AsciiStrLen ((CHAR8 *) ReqOpt[0].ValueStr) + 1));
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = (UINT8 *) (ReqOpt[OptCnt-1].ValueStr + AsciiStrLen ((CHAR8 *) ReqOpt[OptCnt-1].ValueStr) + 1);
+ OptBufSize -= (AsciiStrLen ((CHAR8 *) ReqOpt[OptCnt-1].ValueStr) + 1);
+ PxeBcUintnToAscDec (*BlockSize, ReqOpt[OptCnt].ValueStr, OptBufSize);
+ OptCnt++;
+ }
+
+ if (WindowSize != NULL) {
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_WINDOWSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = (UINT8 *) (ReqOpt[OptCnt-1].ValueStr + AsciiStrLen ((CHAR8 *) ReqOpt[OptCnt-1].ValueStr) + 1);
+ OptBufSize -= (AsciiStrLen ((CHAR8 *) ReqOpt[OptCnt-1].ValueStr) + 1);
+ PxeBcUintnToAscDec (*WindowSize, ReqOpt[OptCnt].ValueStr, OptBufSize);
OptCnt++;
}
Status = Mtftp6->GetInfo (
Mtftp6,
@@ -248,20 +261,22 @@ EFI_STATUS
PxeBcMtftp6ReadFile (
IN PXEBC_PRIVATE_DATA *Private,
IN EFI_MTFTP6_CONFIG_DATA *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN UINT8 *BufferPtr,
IN OUT UINT64 *BufferSize,
IN BOOLEAN DontUseBuffer
)
{
EFI_MTFTP6_PROTOCOL *Mtftp6;
EFI_MTFTP6_TOKEN Token;
- EFI_MTFTP6_OPTION ReqOpt[1];
+ EFI_MTFTP6_OPTION ReqOpt[2];
UINT32 OptCnt;
- UINT8 OptBuf[128];
+ UINT8 BlksizeBuf[10];
+ UINT8 WindowsizeBuf[10];
EFI_STATUS Status;
Status = EFI_DEVICE_ERROR;
Mtftp6 = Private->Mtftp6;
OptCnt = 0;
@@ -271,16 +286,24 @@ PxeBcMtftp6ReadFile (
if (EFI_ERROR (Status)) {
return Status;
}
if (BlockSize != NULL) {
- ReqOpt[0].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
- ReqOpt[0].ValueStr = OptBuf;
- PxeBcUintnToAscDec (*BlockSize, ReqOpt[0].ValueStr, PXE_MTFTP_OPTBUF_MAXNUM_INDEX);
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = BlksizeBuf;
+ PxeBcUintnToAscDec (*BlockSize, ReqOpt[OptCnt].ValueStr, sizeof (BlksizeBuf));
OptCnt++;
}
+ if (WindowSize != NULL) {
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_WINDOWSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = WindowsizeBuf;
+ PxeBcUintnToAscDec (*WindowSize, ReqOpt[OptCnt].ValueStr, sizeof (WindowsizeBuf));
+ OptCnt++;
+ }
+
+
Token.Event = NULL;
Token.OverrideData = NULL;
Token.Filename = Filename;
Token.ModeStr = NULL;
Token.OptionCount = OptCnt;
@@ -406,20 +429,22 @@ EFI_STATUS
PxeBcMtftp6ReadDirectory (
IN PXEBC_PRIVATE_DATA *Private,
IN EFI_MTFTP6_CONFIG_DATA *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN UINT8 *BufferPtr,
IN OUT UINT64 *BufferSize,
IN BOOLEAN DontUseBuffer
)
{
EFI_MTFTP6_PROTOCOL *Mtftp6;
EFI_MTFTP6_TOKEN Token;
- EFI_MTFTP6_OPTION ReqOpt[1];
+ EFI_MTFTP6_OPTION ReqOpt[2];
UINT32 OptCnt;
- UINT8 OptBuf[128];
+ UINT8 BlksizeBuf[10];
+ UINT8 WindowsizeBuf[10];
EFI_STATUS Status;
Status = EFI_DEVICE_ERROR;
Mtftp6 = Private->Mtftp6;
OptCnt = 0;
@@ -429,13 +454,20 @@ PxeBcMtftp6ReadDirectory (
if (EFI_ERROR (Status)) {
return Status;
}
if (BlockSize != NULL) {
- ReqOpt[0].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
- ReqOpt[0].ValueStr = OptBuf;
- PxeBcUintnToAscDec (*BlockSize, ReqOpt[0].ValueStr, PXE_MTFTP_OPTBUF_MAXNUM_INDEX);
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = BlksizeBuf;
+ PxeBcUintnToAscDec (*BlockSize, ReqOpt[OptCnt].ValueStr, sizeof (BlksizeBuf));
+ OptCnt++;
+ }
+
+ if (WindowSize != NULL) {
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_WINDOWSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = WindowsizeBuf;
+ PxeBcUintnToAscDec (*WindowSize, ReqOpt[OptCnt].ValueStr, sizeof (WindowsizeBuf));
OptCnt++;
}
Token.Event = NULL;
Token.OverrideData = NULL;
@@ -566,28 +598,31 @@ EFI_STATUS
PxeBcMtftp4GetFileSize (
IN PXEBC_PRIVATE_DATA *Private,
IN EFI_MTFTP4_CONFIG_DATA *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN OUT UINT64 *BufferSize
)
{
EFI_MTFTP4_PROTOCOL *Mtftp4;
- EFI_MTFTP4_OPTION ReqOpt[2];
+ EFI_MTFTP4_OPTION ReqOpt[3];
EFI_MTFTP4_PACKET *Packet;
EFI_MTFTP4_OPTION *Option;
UINT32 PktLen;
- UINT8 OptBuf[128];
+ UINT8 OptBuf[PXE_MTFTP_OPTBUF_MAXNUM_INDEX];
+ UINTN OptBufSize;
UINT32 OptCnt;
EFI_STATUS Status;
*BufferSize = 0;
Status = EFI_DEVICE_ERROR;
Mtftp4 = Private->Mtftp4;
Packet = NULL;
Option = NULL;
PktLen = 0;
+ OptBufSize = PXE_MTFTP_OPTBUF_MAXNUM_INDEX;
OptCnt = 1;
Config->InitialServerPort = PXEBC_BS_DOWNLOAD_PORT;
Status = Mtftp4->Configure (Mtftp4, Config);
if (EFI_ERROR (Status)) {
@@ -596,17 +631,26 @@ PxeBcMtftp4GetFileSize (
//
// Build the required options for get info.
//
ReqOpt[0].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_TSIZE_INDEX];
- PxeBcUintnToAscDec (0, OptBuf, PXE_MTFTP_OPTBUF_MAXNUM_INDEX);
+ PxeBcUintnToAscDec (0, OptBuf, OptBufSize);
ReqOpt[0].ValueStr = OptBuf;
if (BlockSize != NULL) {
- ReqOpt[1].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
- ReqOpt[1].ValueStr = (UINT8 *) (ReqOpt[0].ValueStr + AsciiStrLen ((CHAR8 *) ReqOpt[0].ValueStr) + 1);
- PxeBcUintnToAscDec (*BlockSize, ReqOpt[1].ValueStr, PXE_MTFTP_OPTBUF_MAXNUM_INDEX - (AsciiStrLen ((CHAR8 *) ReqOpt[0].ValueStr) + 1));
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = (UINT8 *) (ReqOpt[OptCnt-1].ValueStr + AsciiStrLen ((CHAR8 *) ReqOpt[OptCnt-1].ValueStr) + 1);
+ OptBufSize -= (AsciiStrLen ((CHAR8 *) ReqOpt[OptCnt-1].ValueStr) + 1);
+ PxeBcUintnToAscDec (*BlockSize, ReqOpt[OptCnt].ValueStr, OptBufSize);
+ OptCnt++;
+ }
+
+ if (WindowSize != NULL) {
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_WINDOWSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = (UINT8 *) (ReqOpt[OptCnt-1].ValueStr + AsciiStrLen ((CHAR8 *) ReqOpt[OptCnt-1].ValueStr) + 1);
+ OptBufSize -= (AsciiStrLen ((CHAR8 *) ReqOpt[OptCnt-1].ValueStr) + 1);
+ PxeBcUintnToAscDec (*WindowSize, ReqOpt[OptCnt].ValueStr, OptBufSize);
OptCnt++;
}
Status = Mtftp4->GetInfo (
Mtftp4,
@@ -694,20 +738,22 @@ EFI_STATUS
PxeBcMtftp4ReadFile (
IN PXEBC_PRIVATE_DATA *Private,
IN EFI_MTFTP4_CONFIG_DATA *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN UINT8 *BufferPtr,
IN OUT UINT64 *BufferSize,
IN BOOLEAN DontUseBuffer
)
{
EFI_MTFTP4_PROTOCOL *Mtftp4;
EFI_MTFTP4_TOKEN Token;
- EFI_MTFTP4_OPTION ReqOpt[1];
+ EFI_MTFTP4_OPTION ReqOpt[2];
UINT32 OptCnt;
- UINT8 OptBuf[128];
+ UINT8 BlksizeBuf[10];
+ UINT8 WindowsizeBuf[10];
EFI_STATUS Status;
Status = EFI_DEVICE_ERROR;
Mtftp4 = Private->Mtftp4;
OptCnt = 0;
@@ -717,13 +763,20 @@ PxeBcMtftp4ReadFile (
if (EFI_ERROR (Status)) {
return Status;
}
if (BlockSize != NULL) {
- ReqOpt[0].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
- ReqOpt[0].ValueStr = OptBuf;
- PxeBcUintnToAscDec (*BlockSize, ReqOpt[0].ValueStr, PXE_MTFTP_OPTBUF_MAXNUM_INDEX);
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = BlksizeBuf;
+ PxeBcUintnToAscDec (*BlockSize, ReqOpt[OptCnt].ValueStr, sizeof (BlksizeBuf));
+ OptCnt++;
+ }
+
+ if (WindowSize != NULL) {
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_WINDOWSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = WindowsizeBuf;
+ PxeBcUintnToAscDec (*WindowSize, ReqOpt[OptCnt].ValueStr, sizeof (WindowsizeBuf));
OptCnt++;
}
Token.Event = NULL;
Token.OverrideData = NULL;
@@ -852,20 +905,22 @@ EFI_STATUS
PxeBcMtftp4ReadDirectory (
IN PXEBC_PRIVATE_DATA *Private,
IN EFI_MTFTP4_CONFIG_DATA *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN UINT8 *BufferPtr,
IN OUT UINT64 *BufferSize,
IN BOOLEAN DontUseBuffer
)
{
EFI_MTFTP4_PROTOCOL *Mtftp4;
EFI_MTFTP4_TOKEN Token;
- EFI_MTFTP4_OPTION ReqOpt[1];
+ EFI_MTFTP4_OPTION ReqOpt[2];
UINT32 OptCnt;
- UINT8 OptBuf[128];
+ UINT8 BlksizeBuf[10];
+ UINT8 WindowsizeBuf[10];
EFI_STATUS Status;
Status = EFI_DEVICE_ERROR;
Mtftp4 = Private->Mtftp4;
OptCnt = 0;
@@ -875,13 +930,20 @@ PxeBcMtftp4ReadDirectory (
if (EFI_ERROR (Status)) {
return Status;
}
if (BlockSize != NULL) {
- ReqOpt[0].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
- ReqOpt[0].ValueStr = OptBuf;
- PxeBcUintnToAscDec (*BlockSize, ReqOpt[0].ValueStr, PXE_MTFTP_OPTBUF_MAXNUM_INDEX);
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_BLKSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = BlksizeBuf;
+ PxeBcUintnToAscDec (*BlockSize, ReqOpt[OptCnt].ValueStr, sizeof (BlksizeBuf));
+ OptCnt++;
+ }
+
+ if (WindowSize != NULL) {
+ ReqOpt[OptCnt].OptionStr = (UINT8 *) mMtftpOptions[PXE_MTFTP_OPTION_WINDOWSIZE_INDEX];
+ ReqOpt[OptCnt].ValueStr = WindowsizeBuf;
+ PxeBcUintnToAscDec (*WindowSize, ReqOpt[OptCnt].ValueStr, sizeof (WindowsizeBuf));
OptCnt++;
}
Token.Event = NULL;
Token.OverrideData = NULL;
@@ -934,27 +996,30 @@ EFI_STATUS
PxeBcTftpGetFileSize (
IN PXEBC_PRIVATE_DATA *Private,
IN VOID *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN OUT UINT64 *BufferSize
)
{
if (Private->PxeBc.Mode->UsingIpv6) {
return PxeBcMtftp6GetFileSize (
Private,
(EFI_MTFTP6_CONFIG_DATA *) Config,
Filename,
BlockSize,
+ WindowSize,
BufferSize
);
} else {
return PxeBcMtftp4GetFileSize (
Private,
(EFI_MTFTP4_CONFIG_DATA *) Config,
Filename,
BlockSize,
+ WindowSize,
BufferSize
);
}
}
@@ -979,10 +1044,11 @@ EFI_STATUS
PxeBcTftpReadFile (
IN PXEBC_PRIVATE_DATA *Private,
IN VOID *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN UINT8 *BufferPtr,
IN OUT UINT64 *BufferSize,
IN BOOLEAN DontUseBuffer
)
{
@@ -990,20 +1056,22 @@ PxeBcTftpReadFile (
return PxeBcMtftp6ReadFile (
Private,
(EFI_MTFTP6_CONFIG_DATA *) Config,
Filename,
BlockSize,
+ WindowSize,
BufferPtr,
BufferSize,
DontUseBuffer
);
} else {
return PxeBcMtftp4ReadFile (
Private,
(EFI_MTFTP4_CONFIG_DATA *) Config,
Filename,
BlockSize,
+ WindowSize,
BufferPtr,
BufferSize,
DontUseBuffer
);
}
@@ -1081,10 +1149,11 @@ EFI_STATUS
PxeBcTftpReadDirectory (
IN PXEBC_PRIVATE_DATA *Private,
IN VOID *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN UINT8 *BufferPtr,
IN OUT UINT64 *BufferSize,
IN BOOLEAN DontUseBuffer
)
{
@@ -1092,20 +1161,22 @@ PxeBcTftpReadDirectory (
return PxeBcMtftp6ReadDirectory (
Private,
(EFI_MTFTP6_CONFIG_DATA *) Config,
Filename,
BlockSize,
+ WindowSize,
BufferPtr,
BufferSize,
DontUseBuffer
);
} else {
return PxeBcMtftp4ReadDirectory (
Private,
(EFI_MTFTP4_CONFIG_DATA *) Config,
Filename,
BlockSize,
+ WindowSize,
BufferPtr,
BufferSize,
DontUseBuffer
);
}
diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcMtftp.h b/NetworkPkg/UefiPxeBcDxe/PxeBcMtftp.h
index f1150762c6..edd8decbd8 100644
--- a/NetworkPkg/UefiPxeBcDxe/PxeBcMtftp.h
+++ b/NetworkPkg/UefiPxeBcDxe/PxeBcMtftp.h
@@ -18,11 +18,12 @@
#define PXE_MTFTP_OPTION_BLKSIZE_INDEX 0
#define PXE_MTFTP_OPTION_TIMEOUT_INDEX 1
#define PXE_MTFTP_OPTION_TSIZE_INDEX 2
#define PXE_MTFTP_OPTION_MULTICAST_INDEX 3
-#define PXE_MTFTP_OPTION_MAXIMUM_INDEX 4
+#define PXE_MTFTP_OPTION_WINDOWSIZE_INDEX 4
+#define PXE_MTFTP_OPTION_MAXIMUM_INDEX 5
#define PXE_MTFTP_OPTBUF_MAXNUM_INDEX 128
#define PXE_MTFTP_ERROR_STRING_LENGTH 127 // refer to definition of struct EFI_PXE_BASE_CODE_TFTP_ERROR.
#define PXE_MTFTP_DEFAULT_BLOCK_SIZE 512 // refer to rfc-1350.
@@ -46,10 +47,11 @@ EFI_STATUS
PxeBcTftpGetFileSize (
IN PXEBC_PRIVATE_DATA *Private,
IN VOID *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN OUT UINT64 *BufferSize
);
/**
@@ -72,10 +74,11 @@ EFI_STATUS
PxeBcTftpReadFile (
IN PXEBC_PRIVATE_DATA *Private,
IN VOID *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN UINT8 *BufferPtr,
IN OUT UINT64 *BufferSize,
IN BOOLEAN DontUseBuffer
);
@@ -128,10 +131,11 @@ EFI_STATUS
PxeBcTftpReadDirectory (
IN PXEBC_PRIVATE_DATA *Private,
IN VOID *Config,
IN UINT8 *Filename,
IN UINTN *BlockSize,
+ IN UINTN *WindowSize,
IN UINT8 *BufferPtr,
IN OUT UINT64 *BufferSize,
IN BOOLEAN DontUseBuffer
);
#endif
diff --git a/NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf b/NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
index 55fa2b3c8c..949596c029 100644
--- a/NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
+++ b/NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
@@ -105,7 +105,9 @@
[Guids]
gEfiAdapterInfoUndiIpv6SupportGuid ## SOMETIMES_CONSUMES ## GUID
[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdTftpBlockSize ## SOMETIMES_CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdTftpWindowSize ## SOMETIMES_CONSUMES
+
[UserExtensions.TianoCore."ExtraFiles"]
UefiPxeBcDxeExtra.uni
--
2.17.1.windows.2
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
2018-09-17 5:43 [Patch 0/5] Support windowsize to benefit tftp/pxe download performance Jiaxin Wu
` (4 preceding siblings ...)
2018-09-17 5:43 ` [Patch 5/5] NetworkPkg/UefiPxeBcDxe: Use the specified " Jiaxin Wu
@ 2018-09-18 11:23 ` Laszlo Ersek
2018-09-19 2:20 ` Wu, Jiaxin
5 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2018-09-18 11:23 UTC (permalink / raw)
To: Jiaxin Wu, edk2-devel; +Cc: Ye Ting, Carsey Jaben, Fu Siyuan, Shao Ming
On 09/17/18 07:43, Jiaxin Wu wrote:
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
>
> The series patches are to support the TFTP windowsize option described in RFC 7440.
> TFTP shell command and UEFI PXE driver will use the feature to benefit the download
> performance.
I tested this series, between two virtual machines running on my laptop.
The TFTP server program that I used was "tftp-server-5.2-22.el7.x86_64".
The downloaded file was 478,150,656 bytes in size. I built OVMF with
NETWORK_IP6_ENABLE, so that the last patch would take effect for both
PXEv4 and PXEv6.
Before the series:
- PXEv4: 75 seconds (~ 6225 KB/s)
- PXEv6: 100 seconds (~ 4669 KB/s)
After the series:
- PXEv4: 48 seconds (~ 9728 KB/s)
- PXEv6: 60 seconds (~ 7782 KB/s)
These measurements are very rough (I didn't run them multiple times
etc), but I think they are still quite good indicators.
For the testing, I used the UEFI boot options in UiApp, and not the
shell command, hence I have no feedback on patch #3.
For patches #1, #2, and #5:
Tested-by: Laszlo Ersek <lersek@redhat.com>
However, as I pointed out elsewhere in the thread, I think:
- You might want to port the changes from patch#5 to
"MdeModulePkg/Universal/Network/UefiPxeBcDxe" as well, in a separate
patch (patch #6).
- If not, then (a) we should document this feature difference in the INF
files of the UefiPxeBcDxe drivers, (b) patch #4 should be re-done so
that it target NetworkPkg, not MdeModulePkg.
- Patch #4 (regardless of package DEC) should be extended with
documentation (both DEC and UNI).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
2018-09-18 11:23 ` [Patch 0/5] Support windowsize to benefit tftp/pxe download performance Laszlo Ersek
@ 2018-09-19 2:20 ` Wu, Jiaxin
2018-09-19 11:24 ` Laszlo Ersek
0 siblings, 1 reply; 17+ messages in thread
From: Wu, Jiaxin @ 2018-09-19 2:20 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Ye, Ting, Carsey, Jaben, Fu, Siyuan, Shao, Ming
> On 09/17/18 07:43, Jiaxin Wu wrote:
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
> >
> > The series patches are to support the TFTP windowsize option described in
> RFC 7440.
> > TFTP shell command and UEFI PXE driver will use the feature to benefit the
> download
> > performance.
>
> I tested this series, between two virtual machines running on my laptop.
> The TFTP server program that I used was "tftp-server-5.2-22.el7.x86_64".
> The downloaded file was 478,150,656 bytes in size. I built OVMF with
> NETWORK_IP6_ENABLE, so that the last patch would take effect for both
> PXEv4 and PXEv6.
>
> Before the series:
> - PXEv4: 75 seconds (~ 6225 KB/s)
> - PXEv6: 100 seconds (~ 4669 KB/s)
>
> After the series:
> - PXEv4: 48 seconds (~ 9728 KB/s)
> - PXEv6: 60 seconds (~ 7782 KB/s)
>
> These measurements are very rough (I didn't run them multiple times
> etc), but I think they are still quite good indicators.
>
> For the testing, I used the UEFI boot options in UiApp, and not the
> shell command, hence I have no feedback on patch #3.
>
> For patches #1, #2, and #5:
>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
>
Thanks the verification.
> However, as I pointed out elsewhere in the thread, I think:
>
> - You might want to port the changes from patch#5 to
> "MdeModulePkg/Universal/Network/UefiPxeBcDxe" as well, in a separate
> patch (patch #6).
>
> - If not, then (a) we should document this feature difference in the INF
> files of the UefiPxeBcDxe drivers, (b) patch #4 should be re-done so
> that it target NetworkPkg, not MdeModulePkg.
>
As I said in the previous email, normally, we only add the new feature into the combo driver. But I think that's depends on the request. If you want to include the feature into MdeModulePkg/Universal/Network/UefiPxeBcDxe since the OVMF platform might use it, I will create patch #6. If not, I will follow the comments (a)/(b).
> - Patch #4 (regardless of package DEC) should be extended with
> documentation (both DEC and UNI).
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
2018-09-19 2:20 ` Wu, Jiaxin
@ 2018-09-19 11:24 ` Laszlo Ersek
2018-09-20 5:54 ` Wu, Jiaxin
0 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2018-09-19 11:24 UTC (permalink / raw)
To: Wu, Jiaxin, edk2-devel@lists.01.org
Cc: Ye, Ting, Carsey, Jaben, Fu, Siyuan, Shao, Ming
On 09/19/18 04:20, Wu, Jiaxin wrote:
>> On 09/17/18 07:43, Jiaxin Wu wrote:
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
>>>
>>> The series patches are to support the TFTP windowsize option described in
>> RFC 7440.
>>> TFTP shell command and UEFI PXE driver will use the feature to benefit the
>> download
>>> performance.
>>
>> I tested this series, between two virtual machines running on my laptop.
>> The TFTP server program that I used was "tftp-server-5.2-22.el7.x86_64".
>> The downloaded file was 478,150,656 bytes in size. I built OVMF with
>> NETWORK_IP6_ENABLE, so that the last patch would take effect for both
>> PXEv4 and PXEv6.
>>
>> Before the series:
>> - PXEv4: 75 seconds (~ 6225 KB/s)
>> - PXEv6: 100 seconds (~ 4669 KB/s)
>>
>> After the series:
>> - PXEv4: 48 seconds (~ 9728 KB/s)
>> - PXEv6: 60 seconds (~ 7782 KB/s)
>>
>> These measurements are very rough (I didn't run them multiple times
>> etc), but I think they are still quite good indicators.
>>
>> For the testing, I used the UEFI boot options in UiApp, and not the
>> shell command, hence I have no feedback on patch #3.
>>
>> For patches #1, #2, and #5:
>>
>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>
>
> Thanks the verification.
>
>
>> However, as I pointed out elsewhere in the thread, I think:
>>
>> - You might want to port the changes from patch#5 to
>> "MdeModulePkg/Universal/Network/UefiPxeBcDxe" as well, in a separate
>> patch (patch #6).
>>
>> - If not, then (a) we should document this feature difference in the INF
>> files of the UefiPxeBcDxe drivers, (b) patch #4 should be re-done so
>> that it target NetworkPkg, not MdeModulePkg.
>>
>
> As I said in the previous email, normally, we only add the new feature into the combo driver. But I think that's depends on the request. If you want to include the feature into MdeModulePkg/Universal/Network/UefiPxeBcDxe since the OVMF platform might use it, I will create patch #6. If not, I will follow the comments (a)/(b).
I don't currently have a use case or "requirement" for the window size
feature to work in an OVMF build that does *not* have
NETWORK_IP6_ENABLE. So from my side, we can delay the porting until such
a need materializes (it might never materialize, of course!)
However, because this information -- i.e., the feature separation
between the IPv4-only, and the combined IPv4/IPv6 driver -- is new to
me, can we please document it somewhere in the code, for example, in the
INF files? We don't have to spell out the TFTP window size feature by
name, just the fact that NetworkPkg's UefiPxeBcDxe has more features in
general (even such features that are orthogonal to internet protocol
version, v4 vs. v6).
If such documentation already exists, then I missed it, sorry!
Thanks!
Laszlo
>> - Patch #4 (regardless of package DEC) should be extended with
>> documentation (both DEC and UNI).
>>
>> Thanks
>> Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
2018-09-19 11:24 ` Laszlo Ersek
@ 2018-09-20 5:54 ` Wu, Jiaxin
[not found] ` <845df55b-b9ba-20f9-25fd-22778253c198@redhat.com>
0 siblings, 1 reply; 17+ messages in thread
From: Wu, Jiaxin @ 2018-09-20 5:54 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Ye, Ting, Carsey, Jaben, Fu, Siyuan, Shao, Ming, Li, Ruth
Hi Laszlo,
I agree there is no document to describe the detailed difference against the overlapped network drivers the between NetworkPkg and MdeModulePkg (except IPv4/IPv6 support ). We only declared that those drivers should not be used at the same (https://github.com/tianocore/tianocore.github.io/wiki/NetworkPkg-Getting-Started-Guide).
Actually, the problem you mentioned here only exists in ISCSI/TCP/PXE drivers - Tcp4Dxe VS TcpDxe, IScsiDxe VS IScsiDxe, UefiPxeBcDxe VS UefiPxeBcDxe. So, as you said, it's the time for us to add some declaration somewhere (inf or wiki) -- For those three drivers in MdeModulePkg, they will remain unchanged until there is any specific requirement that we need fix any issue. That will greatly reduce the effort to maintain/test those combine of two drivers. So, we don’t recommend to use those three drivers in MdeModulePkg because they might some issues, which has been fixed in the NetworkPkg drivers. If you agree, we will add some statement in the corresponding *.inf files.
Thanks,
Jiaxin
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, September 19, 2018 7:25 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>;
> Fu, Siyuan <siyuan.fu@intel.com>; Shao, Ming <ming.shao@intel.com>
> Subject: Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe
> download performance.
>
> On 09/19/18 04:20, Wu, Jiaxin wrote:
> >> On 09/17/18 07:43, Jiaxin Wu wrote:
> >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
> >>>
> >>> The series patches are to support the TFTP windowsize option described
> in
> >> RFC 7440.
> >>> TFTP shell command and UEFI PXE driver will use the feature to benefit
> the
> >> download
> >>> performance.
> >>
> >> I tested this series, between two virtual machines running on my laptop.
> >> The TFTP server program that I used was "tftp-server-5.2-22.el7.x86_64".
> >> The downloaded file was 478,150,656 bytes in size. I built OVMF with
> >> NETWORK_IP6_ENABLE, so that the last patch would take effect for both
> >> PXEv4 and PXEv6.
> >>
> >> Before the series:
> >> - PXEv4: 75 seconds (~ 6225 KB/s)
> >> - PXEv6: 100 seconds (~ 4669 KB/s)
> >>
> >> After the series:
> >> - PXEv4: 48 seconds (~ 9728 KB/s)
> >> - PXEv6: 60 seconds (~ 7782 KB/s)
> >>
> >> These measurements are very rough (I didn't run them multiple times
> >> etc), but I think they are still quite good indicators.
> >>
> >> For the testing, I used the UEFI boot options in UiApp, and not the
> >> shell command, hence I have no feedback on patch #3.
> >>
> >> For patches #1, #2, and #5:
> >>
> >> Tested-by: Laszlo Ersek <lersek@redhat.com>
> >>
> >
> > Thanks the verification.
> >
> >
> >> However, as I pointed out elsewhere in the thread, I think:
> >>
> >> - You might want to port the changes from patch#5 to
> >> "MdeModulePkg/Universal/Network/UefiPxeBcDxe" as well, in a
> separate
> >> patch (patch #6).
> >>
> >> - If not, then (a) we should document this feature difference in the INF
> >> files of the UefiPxeBcDxe drivers, (b) patch #4 should be re-done so
> >> that it target NetworkPkg, not MdeModulePkg.
> >>
> >
> > As I said in the previous email, normally, we only add the new feature into
> the combo driver. But I think that's depends on the request. If you want to
> include the feature into MdeModulePkg/Universal/Network/UefiPxeBcDxe
> since the OVMF platform might use it, I will create patch #6. If not, I will
> follow the comments (a)/(b).
>
> I don't currently have a use case or "requirement" for the window size
> feature to work in an OVMF build that does *not* have
> NETWORK_IP6_ENABLE. So from my side, we can delay the porting until such
> a need materializes (it might never materialize, of course!)
>
> However, because this information -- i.e., the feature separation
> between the IPv4-only, and the combined IPv4/IPv6 driver -- is new to
> me, can we please document it somewhere in the code, for example, in the
> INF files? We don't have to spell out the TFTP window size feature by
> name, just the fact that NetworkPkg's UefiPxeBcDxe has more features in
> general (even such features that are orthogonal to internet protocol
> version, v4 vs. v6).
>
> If such documentation already exists, then I missed it, sorry!
>
> Thanks!
> Laszlo
>
> >> - Patch #4 (regardless of package DEC) should be extended with
> >> documentation (both DEC and UNI).
> >>
> >> Thanks
> >> Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread