Hi Ashutosh,
On Fri, Nov 06, 2015 at 01:31:52PM +0000, Ashutosh Singh wrote:
> > So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
> > of code duplicates functionality already available elsewhere in the
> > tree ... but I also don't care much about ArmBds.
> >
> > What I'm curious about is - why is your Mtftp4GetFileSize call failing
> > such that you need to use a hardcoded buffer size?
> > Does your TFTP server not support rfc2349?
> > Would changing that (for example by switching to tftpd-hpa over the
> > unmaintained netkit-tftpd) not make more sense?
>
> Took a while to test with tftpd-hpa, with this it does work fine, i.e.
> Mtftp4GetFileSize does succeed and buffer size is allocated accordingly.
> I would still request you to merge the change, as it will mean people
> using the default tftpd that comes with ubuntu distribution will also
> be able to use the tftp boot for ARM platforms.
Thanks for checking this - it's good to know what caused the
behaviour.
As I said, I don't really care much about this bit of code, so I'll go
ahead and commit the patch for this time. Long (or even medium) term,
we need to get rid of this kind of reimplementation of common
functionality inside the Bds.
I do however think the commit message can be more descriptive. My
suggestion would be:
---
ArmPkg/BdsLib: Increase fallback tftp buffer size
When performing a tftp download from a server which does not support
rfc2349 transfer size option (such as netkit-tftpd), the existing code
falls back to allocating an 8MB buffer. Since this is insufficient for
an uncompressed AArch64 Linux kernel image, double the size to 16MB.
---
Are you happy with that message?
/
Leif
> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: 05 November 2015 12:26
> To: Ashutosh Singh
> Cc: edk2-de...@ml01.01.org; ard.biesheu...@linaro.org;
> ryan.har...@linaro.org; James King
> Subject: Re: [PATCH] Increase tftp buffer size
>
> On Thu, Nov 05, 2015 at 11:37:43AM +0000, Ashutosh Singh wrote:
> > Default tftp buffer size (8MB) is too small for standard
> > ARM kernel images, which results in multiple aborted tftp fetches.
> > This fix increase the buffer size to 16MB.
>
> So, I'm not a fan of hard-coded buffer sizes, or how this existing bit
> of code duplicates functionality already available elsewhere in the
> tree ... but I also don't care much about ArmBds.
>
> What I'm curious about is - why is your Mtftp4GetFileSize call failing
> such that you need to use a hardcoded buffer size?
> Does your TFTP server not support rfc2349?
> Would changing that (for example by switching to tftpd-hpa over the
> unmaintained netkit-tftpd) not make more sense?
>
> /
> Leif
>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Ashutosh Singh <ashutosh.si...@arm.com>
> > ---
> > ArmPkg/Library/BdsLib/BdsFilePath.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c
> > b/ArmPkg/Library/BdsLib/BdsFilePath.c
> > index ff42175..0410236 100644
> > --- a/ArmPkg/Library/BdsLib/BdsFilePath.c
> > +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
> > @@ -1198,7 +1198,7 @@ BdsTftpLoadImage (
> > if (Mtftp4GetFileSize (Mtftp4, AsciiFilePath, &FileSize) == EFI_SUCCESS)
> > {
> > TftpBufferSize = FileSize;
> > } else {
> > - TftpBufferSize = SIZE_8MB;
> > + TftpBufferSize = SIZE_16MB;
> > }
> >
> > TftpContext = AllocatePool (sizeof (BDS_TFTP_CONTEXT));
> > @@ -1209,7 +1209,7 @@ BdsTftpLoadImage (
> > TftpContext->FileSize = FileSize;
> >
> > for (; TftpBufferSize <= FixedPcdGet32 (PcdMaxTftpFileSize);
> > - TftpBufferSize = (TftpBufferSize + SIZE_8MB) & (~(SIZE_8MB-1))) {
> > + TftpBufferSize = (TftpBufferSize + SIZE_16MB) & (~(SIZE_16MB-1)))
> > {
> > //
> > // Allocate a buffer to hold the whole file.
> > //
> > --
> > 1.7.9.5
> >
> >
> > ________________________________
> >
> > -- IMPORTANT NOTICE: The contents of this email and any attachments are
> > confidential and may also be privileged. If you are not the intended
> > recipient, please notify the sender immediately and do not disclose the
> > contents to any other person, use it for any purpose, or store or copy the
> > information in any medium. Thank you.
> >
>
>
> ________________________________
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
