subject:"\[edk2\] \[PATCH\] Increase tftp buffer size"

Re: [edk2] [PATCH] Increase tftp buffer size

2015-11-06 Thread Laszlo Ersek

On 11/06/15 15:19, Laszlo Ersek wrote:
> On 11/06/15 14:31, 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
> 
> You mean the one:
> 
> http://packages.ubuntu.com/wily/tftpd
> http://packages.ubuntu.com/xenial/tftpd
> 
> that is explicitly advertised in Debian (Ubuntu's parent distro):
> 
> https://packages.debian.org/jessie/tftpd
> 
> and in Ubuntu as well:
> 
> http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz
> 
> as unsuitable for PXE booting?
> 
> See also
> 
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638
> 
>> will also
>> be able to use the tftp boot for ARM platforms.
> 
> Since this patch intends to modify the ARM BDS, I don't really have a
> horse in the race, but please be aware that this is just a band-aid.
> Debian and Ubuntu users should use the right tftp server (and/or file a
> bug with their distro to change the default TFTP server).
> 
> I recommend to state the above in the commit message.

(Sorry about the followup.)

Also, the subject should say:

ArmPkg: BdsLib: increase tftp buffer size

(When I first saw the email, I thought the patch was for MdeModulePkg or
NetworkPkg.)

Thanks
Laszlo

> 
>>
>> -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 +, 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 
>>> ---
>>>  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, ) == 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

Re: [edk2] [PATCH] Increase tftp buffer size

2015-11-06 Thread Ryan Harkin

I think the summary here is
- nobody cares about ArmBds (it will be deleted asap anyway)
- the patch is technically no worse than what's there already
- the original code should have dynamically allocated the buffer, but doesn't
- the code should have given a sensible error message mentioning that
the file size wasn't returned by the server, but doesn't
- Ashu is using the "wrong" tftp server, which is why he found this problem

So I'd say we just submit the patch.  Until we get around to deleting
ArmBds, it's one less chance for other innocent victims to stumble
across this problem and come to us for help.

On 6 November 2015 at 14:22, Laszlo Ersek  wrote:
> On 11/06/15 15:19, Laszlo Ersek wrote:
>> On 11/06/15 14:31, 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
>>
>> You mean the one:
>>
>> http://packages.ubuntu.com/wily/tftpd
>> http://packages.ubuntu.com/xenial/tftpd
>>
>> that is explicitly advertised in Debian (Ubuntu's parent distro):
>>
>> https://packages.debian.org/jessie/tftpd
>>
>> and in Ubuntu as well:
>>
>> http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz
>>
>> as unsuitable for PXE booting?
>>
>> See also
>>
>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638
>>
>>> will also
>>> be able to use the tftp boot for ARM platforms.
>>
>> Since this patch intends to modify the ARM BDS, I don't really have a
>> horse in the race, but please be aware that this is just a band-aid.
>> Debian and Ubuntu users should use the right tftp server (and/or file a
>> bug with their distro to change the default TFTP server).
>>
>> I recommend to state the above in the commit message.
>
> (Sorry about the followup.)
>
> Also, the subject should say:
>
> ArmPkg: BdsLib: increase tftp buffer size
>
> (When I first saw the email, I thought the patch was for MdeModulePkg or
> NetworkPkg.)
>
> Thanks
> Laszlo
>
>>
>>>
>>> -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 +, 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 
 ---
  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, ) == 
 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) &

Re: [edk2] [PATCH] Increase tftp buffer size

2015-11-06 Thread Leif Lindholm

On Fri, Nov 06, 2015 at 02:31:46PM +, Ryan Harkin wrote:
> I think the summary here is
> - nobody cares about ArmBds (it will be deleted asap anyway)

Yup.

> - the patch is technically no worse than what's there already

Yup.

> - the original code should have dynamically allocated the buffer,
>   but doesn't

Well, it does try. But instead of returning an error it tried to
guess.

> - the code should have given a sensible error message mentioning that
> the file size wasn't returned by the server, but doesn't

Yup.

> - Ashu is using the "wrong" tftp server, which is why he found this problem

Yup.

> So I'd say we just submit the patch.  Until we get around to deleting
> ArmBds, it's one less chance for other innocent victims to stumble
> across this problem and come to us for help.

Yup.
An arm64 "allyesconfig" linux kernel is currently around 9MB, so this
should be sufficient for the remaining lifetime of the ArmBds.

/
Leif

> On 6 November 2015 at 14:22, Laszlo Ersek  wrote:
> > On 11/06/15 15:19, Laszlo Ersek wrote:
> >> On 11/06/15 14:31, 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
> >>
> >> You mean the one:
> >>
> >> http://packages.ubuntu.com/wily/tftpd
> >> http://packages.ubuntu.com/xenial/tftpd
> >>
> >> that is explicitly advertised in Debian (Ubuntu's parent distro):
> >>
> >> https://packages.debian.org/jessie/tftpd
> >>
> >> and in Ubuntu as well:
> >>
> >> http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz
> >>
> >> as unsuitable for PXE booting?
> >>
> >> See also
> >>
> >> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
> >> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638
> >>
> >>> will also
> >>> be able to use the tftp boot for ARM platforms.
> >>
> >> Since this patch intends to modify the ARM BDS, I don't really have a
> >> horse in the race, but please be aware that this is just a band-aid.
> >> Debian and Ubuntu users should use the right tftp server (and/or file a
> >> bug with their distro to change the default TFTP server).
> >>
> >> I recommend to state the above in the commit message.
> >
> > (Sorry about the followup.)
> >
> > Also, the subject should say:
> >
> > ArmPkg: BdsLib: increase tftp buffer size
> >
> > (When I first saw the email, I thought the patch was for MdeModulePkg or
> > NetworkPkg.)
> >
> > Thanks
> > Laszlo
> >
> >>
> >>>
> >>> -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 +, 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 
>  ---
>   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, ) == 
>

Re: [edk2] [PATCH] Increase tftp buffer size

2015-11-06 Thread Laszlo Ersek

On 11/06/15 15:31, Ryan Harkin wrote:
> I think the summary here is
> - nobody cares about ArmBds (it will be deleted asap anyway)
> - the patch is technically no worse than what's there already
> - the original code should have dynamically allocated the buffer, but doesn't
> - the code should have given a sensible error message mentioning that
> the file size wasn't returned by the server, but doesn't
> - Ashu is using the "wrong" tftp server, which is why he found this problem
> 
> So I'd say we just submit the patch.  Until we get around to deleting
> ArmBds, it's one less chance for other innocent victims to stumble
> across this problem and come to us for help.

Okay with me, but please make the subject conform to the format edk2
uses, plus your nice summary should be included in the commit message in
some form.

I didn't intend to "block" the patch at all. :)

Thanks
Laszlo

> 
> On 6 November 2015 at 14:22, Laszlo Ersek  wrote:
>> On 11/06/15 15:19, Laszlo Ersek wrote:
>>> On 11/06/15 14:31, 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
>>>
>>> You mean the one:
>>>
>>> http://packages.ubuntu.com/wily/tftpd
>>> http://packages.ubuntu.com/xenial/tftpd
>>>
>>> that is explicitly advertised in Debian (Ubuntu's parent distro):
>>>
>>> https://packages.debian.org/jessie/tftpd
>>>
>>> and in Ubuntu as well:
>>>
>>> http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz
>>>
>>> as unsuitable for PXE booting?
>>>
>>> See also
>>>
>>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
>>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638
>>>
 will also
 be able to use the tftp boot for ARM platforms.
>>>
>>> Since this patch intends to modify the ARM BDS, I don't really have a
>>> horse in the race, but please be aware that this is just a band-aid.
>>> Debian and Ubuntu users should use the right tftp server (and/or file a
>>> bug with their distro to change the default TFTP server).
>>>
>>> I recommend to state the above in the commit message.
>>
>> (Sorry about the followup.)
>>
>> Also, the subject should say:
>>
>> ArmPkg: BdsLib: increase tftp buffer size
>>
>> (When I first saw the email, I thought the patch was for MdeModulePkg or
>> NetworkPkg.)
>>
>> Thanks
>> Laszlo
>>
>>>

 -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 +, 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 
> ---
>  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, ) == 
> EFI_SUCCESS) {
>  TftpBufferSize = FileSize;
>} else {
> -TftpBufferSize = SIZE_8MB;
> +TftpBufferSize = SIZE_16MB;
>}
>

Re: [edk2] [PATCH] Increase tftp buffer size

2015-11-06 Thread Ashutosh Singh

Leif: Yes, the commit message seems fine, I will resubmit the patch
with new commit messages.
Laszlo: I understand that it is a band-aid, but I will leave that bigger
fight (of fixing ArmBds) for another day :) . It isn't very obvious to me that
pxe not being supported by default tftpd will cause problems with bare
tftp as well. Though reading rfc2349 does clarify this.

Thanks all for the review, I will resubmit the patch with proper commit message.

BR
Ashu


-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: 06 November 2015 14:48
To: ryan.har...@linaro.org
Cc: Ashutosh Singh; Leif Lindholm; edk2-de...@ml01.01.org; James King; 
ard.biesheu...@linaro.org
Subject: Re: [edk2] [PATCH] Increase tftp buffer size

On 11/06/15 15:31, Ryan Harkin wrote:
> I think the summary here is
> - nobody cares about ArmBds (it will be deleted asap anyway)
> - the patch is technically no worse than what's there already
> - the original code should have dynamically allocated the buffer, but doesn't
> - the code should have given a sensible error message mentioning that
> the file size wasn't returned by the server, but doesn't
> - Ashu is using the "wrong" tftp server, which is why he found this problem
>
> So I'd say we just submit the patch.  Until we get around to deleting
> ArmBds, it's one less chance for other innocent victims to stumble
> across this problem and come to us for help.

Okay with me, but please make the subject conform to the format edk2
uses, plus your nice summary should be included in the commit message in
some form.

I didn't intend to "block" the patch at all. :)

Thanks
Laszlo

>
> On 6 November 2015 at 14:22, Laszlo Ersek <ler...@redhat.com> wrote:
>> On 11/06/15 15:19, Laszlo Ersek wrote:
>>> On 11/06/15 14:31, 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
>>>
>>> You mean the one:
>>>
>>> http://packages.ubuntu.com/wily/tftpd
>>> http://packages.ubuntu.com/xenial/tftpd
>>>
>>> that is explicitly advertised in Debian (Ubuntu's parent distro):
>>>
>>> https://packages.debian.org/jessie/tftpd
>>>
>>> and in Ubuntu as well:
>>>
>>> http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz
>>>
>>> as unsuitable for PXE booting?
>>>
>>> See also
>>>
>>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
>>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638
>>>
>>>> will also
>>>> be able to use the tftp boot for ARM platforms.
>>>
>>> Since this patch intends to modify the ARM BDS, I don't really have a
>>> horse in the race, but please be aware that this is just a band-aid.
>>> Debian and Ubuntu users should use the right tftp server (and/or file a
>>> bug with their distro to change the default TFTP server).
>>>
>>> I recommend to state the above in the commit message.
>>
>> (Sorry about the followup.)
>>
>> Also, the subject should say:
>>
>> ArmPkg: BdsLib: increase tftp buffer size
>>
>> (When I first saw the email, I thought the patch was for MdeModulePkg or
>> NetworkPkg.)
>>
>> Thanks
>> Laszlo
>>
>>>
>>>>
>>>> -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 +

Re: [edk2] [PATCH] Increase tftp buffer size

2015-11-06 Thread Ashutosh Singh

> 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.

-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 +, 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 
> ---
>  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, ) == 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

Re: [edk2] [PATCH] Increase tftp buffer size

2015-11-06 Thread Laszlo Ersek

On 11/06/15 14:31, 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

You mean the one:

http://packages.ubuntu.com/wily/tftpd
http://packages.ubuntu.com/xenial/tftpd

that is explicitly advertised in Debian (Ubuntu's parent distro):

https://packages.debian.org/jessie/tftpd

and in Ubuntu as well:

http://archive.ubuntu.com/ubuntu/pool/universe/n/netkit-tftp/netkit-tftp_0.17-18ubuntu2.diff.gz

as unsuitable for PXE booting?

See also

http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2612
http://thread.gmane.org/gmane.comp.bios.edk2.devel/2515/focus=2638

> will also
> be able to use the tftp boot for ARM platforms.

Since this patch intends to modify the ARM BDS, I don't really have a
horse in the race, but please be aware that this is just a band-aid.
Debian and Ubuntu users should use the right tftp server (and/or file a
bug with their distro to change the default TFTP server).

I recommend to state the above in the commit message.

Thanks
Laszlo

> 
> -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 +, 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 
>> ---
>>  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, ) == 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
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org

[edk2] [PATCH] Increase tftp buffer size

2015-11-05 Thread Ashutosh Singh

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.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ashutosh Singh 
---
 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, ) == 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.

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Increase tftp buffer size

2015-11-05 Thread Leif Lindholm

On Thu, Nov 05, 2015 at 11:37:43AM +, 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 
> ---
>  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, ) == 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.
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Increase tftp buffer size

Re: [edk2] [PATCH] Increase tftp buffer size

Re: [edk2] [PATCH] Increase tftp buffer size

Re: [edk2] [PATCH] Increase tftp buffer size

Re: [edk2] [PATCH] Increase tftp buffer size

Re: [edk2] [PATCH] Increase tftp buffer size

Re: [edk2] [PATCH] Increase tftp buffer size

[edk2] [PATCH] Increase tftp buffer size

Re: [edk2] [PATCH] Increase tftp buffer size

9 matches

Site Navigation

Mail list logo

Footer information