public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
@ 2020-04-19 13:04 Samer El-Haj-Mahmoud
  2020-04-19 13:04 ` [edk2-platform][PATCH v1 1/4] Platform/RaspberryPi/RPi3: Fix TFTP dynamic command initialization Samer El-Haj-Mahmoud
                   ` (5 more replies)
  0 siblings, 6 replies; 25+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-04-19 13:04 UTC (permalink / raw)
  To: devel; +Cc: Leif Lindholm, Ard Biesheuvel, Pete Batard, Andrei Warkentin

Fix an ASSERT with the TFTP dynamic Shell command on the 
RPi3 and RPi4 when running DEBUG builds. Also, enable the 
command by default for all builds.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>

Samer El-Haj-Mahmoud (4):
  Platform/RaspberryPi/RPi3: Fix TFTP dynamic command initialization
  Platform/RaspberryPi/RPi4: Fix TFTP dynamic command initialization
  Platform/RaspberryPi/RPi3: Enable TFTP command by default
  Platform/RaspberryPi/RPi4: Enable TFTP command by default

 Platform/RaspberryPi/RPi3/RPi3.dsc | 7 +++++--
 Platform/RaspberryPi/RPi4/RPi4.dsc | 7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [edk2-platform][PATCH v1 1/4] Platform/RaspberryPi/RPi3: Fix TFTP dynamic command initialization
  2020-04-19 13:04 [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Samer El-Haj-Mahmoud
@ 2020-04-19 13:04 ` Samer El-Haj-Mahmoud
  2020-04-19 13:04 ` [edk2-platform][PATCH v1 2/4] Platform/RaspberryPi/RPi4: " Samer El-Haj-Mahmoud
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 25+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-04-19 13:04 UTC (permalink / raw)
  To: devel; +Cc: Leif Lindholm, Ard Biesheuvel, Pete Batard, Andrei Warkentin

Set the PcdShellLibAutoInitialize for the TFTP dynamic Shell command.
This fixes an ASSERT observed in https://github.com/pftf/RPi3/issues/11

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Signed-off-by: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
---
 Platform/RaspberryPi/RPi3/RPi3.dsc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index a451e2a82b59..54ebfdfbb05a 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -662,5 +662,8 @@ [Components.common]
       gEfiShellPkgTokenSpaceGuid.PcdShellFileOperationSize|0x200000
   }
 !if $(INCLUDE_TFTP_COMMAND) == TRUE
-  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
+  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf  {
+    <PcdsFixedAtBuild>
+      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
+  }
 !endif
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [edk2-platform][PATCH v1 2/4] Platform/RaspberryPi/RPi4: Fix TFTP dynamic command initialization
  2020-04-19 13:04 [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Samer El-Haj-Mahmoud
  2020-04-19 13:04 ` [edk2-platform][PATCH v1 1/4] Platform/RaspberryPi/RPi3: Fix TFTP dynamic command initialization Samer El-Haj-Mahmoud
@ 2020-04-19 13:04 ` Samer El-Haj-Mahmoud
  2020-04-19 13:04 ` [edk2-platform][PATCH v1 3/4] Platform/RaspberryPi/RPi3: Enable TFTP command by default Samer El-Haj-Mahmoud
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 25+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-04-19 13:04 UTC (permalink / raw)
  To: devel; +Cc: Leif Lindholm, Ard Biesheuvel, Pete Batard, Andrei Warkentin

Set the PcdShellLibAutoInitialize for the TFTP dynamic Shell command.
This fixes an ASSERT observed in https://github.com/pftf/RPi4/issues/38

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Signed-off-by: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
---
 Platform/RaspberryPi/RPi4/RPi4.dsc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 835b558423a5..de32aab2df3c 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -716,5 +716,8 @@ [Components.common]
       gEfiShellPkgTokenSpaceGuid.PcdShellFileOperationSize|0x200000
   }
 !if $(INCLUDE_TFTP_COMMAND) == TRUE
-  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
+  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf  {
+    <PcdsFixedAtBuild>
+      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
+  }
 !endif
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [edk2-platform][PATCH v1 3/4] Platform/RaspberryPi/RPi3: Enable TFTP command by default
  2020-04-19 13:04 [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Samer El-Haj-Mahmoud
  2020-04-19 13:04 ` [edk2-platform][PATCH v1 1/4] Platform/RaspberryPi/RPi3: Fix TFTP dynamic command initialization Samer El-Haj-Mahmoud
  2020-04-19 13:04 ` [edk2-platform][PATCH v1 2/4] Platform/RaspberryPi/RPi4: " Samer El-Haj-Mahmoud
@ 2020-04-19 13:04 ` Samer El-Haj-Mahmoud
  2020-04-19 13:04 ` [edk2-platform][PATCH v1 4/4] Platform/RaspberryPi/RPi4: " Samer El-Haj-Mahmoud
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 25+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-04-19 13:04 UTC (permalink / raw)
  To: devel; +Cc: Leif Lindholm, Ard Biesheuvel, Pete Batard, Andrei Warkentin

Enable the TFTP dynamic shell command by default.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Signed-off-by: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
---
 Platform/RaspberryPi/RPi3/RPi3.dsc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index 54ebfdfbb05a..b4f1c1fe5d45 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -30,7 +30,7 @@ [Defines]
   # -D FLAG=VALUE
   #
   DEFINE SECURE_BOOT_ENABLE      = FALSE
-  DEFINE INCLUDE_TFTP_COMMAND    = FALSE
+  DEFINE INCLUDE_TFTP_COMMAND    = TRUE
   DEFINE DEBUG_PRINT_ERROR_LEVEL = 0x8000004F
 
 ################################################################################
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [edk2-platform][PATCH v1 4/4] Platform/RaspberryPi/RPi4: Enable TFTP command by default
  2020-04-19 13:04 [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Samer El-Haj-Mahmoud
                   ` (2 preceding siblings ...)
  2020-04-19 13:04 ` [edk2-platform][PATCH v1 3/4] Platform/RaspberryPi/RPi3: Enable TFTP command by default Samer El-Haj-Mahmoud
@ 2020-04-19 13:04 ` Samer El-Haj-Mahmoud
  2020-04-19 13:33 ` [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Ard Biesheuvel
  2020-04-20 13:24 ` Ard Biesheuvel
  5 siblings, 0 replies; 25+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-04-19 13:04 UTC (permalink / raw)
  To: devel; +Cc: Leif Lindholm, Ard Biesheuvel, Pete Batard, Andrei Warkentin

Enable the TFTP dynamic shell command by default.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Signed-off-by: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
---
 Platform/RaspberryPi/RPi4/RPi4.dsc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index de32aab2df3c..9ba32af209b6 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -30,7 +30,7 @@ [Defines]
   # -D FLAG=VALUE
   #
   DEFINE SECURE_BOOT_ENABLE      = FALSE
-  DEFINE INCLUDE_TFTP_COMMAND    = FALSE
+  DEFINE INCLUDE_TFTP_COMMAND    = TRUE
   DEFINE DEBUG_PRINT_ERROR_LEVEL = 0x8000004F
 
 !ifndef TFA_BUILD_ARTIFACTS
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 13:04 [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Samer El-Haj-Mahmoud
                   ` (3 preceding siblings ...)
  2020-04-19 13:04 ` [edk2-platform][PATCH v1 4/4] Platform/RaspberryPi/RPi4: " Samer El-Haj-Mahmoud
@ 2020-04-19 13:33 ` Ard Biesheuvel
  2020-04-19 14:00   ` [edk2-devel] " Samer El-Haj-Mahmoud
  2020-04-19 14:19   ` Pete Batard
  2020-04-20 13:24 ` Ard Biesheuvel
  5 siblings, 2 replies; 25+ messages in thread
From: Ard Biesheuvel @ 2020-04-19 13:33 UTC (permalink / raw)
  To: Samer El-Haj-Mahmoud, devel; +Cc: Leif Lindholm, Pete Batard, Andrei Warkentin

On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
> Fix an ASSERT with the TFTP dynamic Shell command on the
> RPi3 and RPi4 when running DEBUG builds. Also, enable the
> command by default for all builds.
> 

Fixing the ASSERT is fine but I am reluctant to enable this by default. 
It is a non-standard hack that ARM contributed in the past, and is not 
covered by the EFI of Shell specifications. If RPi4 is intended to be a 
showcase for UEFI on ARM done right, we should not enable this at all.




> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Pete Batard <pete@akeo.ie>
> Cc: Andrei Warkentin <awarkentin@vmware.com>
> 
> Samer El-Haj-Mahmoud (4):
>    Platform/RaspberryPi/RPi3: Fix TFTP dynamic command initialization
>    Platform/RaspberryPi/RPi4: Fix TFTP dynamic command initialization
>    Platform/RaspberryPi/RPi3: Enable TFTP command by default
>    Platform/RaspberryPi/RPi4: Enable TFTP command by default
> 
>   Platform/RaspberryPi/RPi3/RPi3.dsc | 7 +++++--
>   Platform/RaspberryPi/RPi4/RPi4.dsc | 7 +++++--
>   2 files changed, 10 insertions(+), 4 deletions(-)
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 13:33 ` [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Ard Biesheuvel
@ 2020-04-19 14:00   ` Samer El-Haj-Mahmoud
  2020-04-19 14:04     ` Ard Biesheuvel
  2020-04-19 14:19   ` Pete Batard
  1 sibling, 1 reply; 25+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-04-19 14:00 UTC (permalink / raw)
  To: devel@edk2.groups.io, Ard Biesheuvel, Samer El-Haj-Mahmoud
  Cc: Leif Lindholm, Pete Batard,
	Andrei Warkentin (awarkentin@vmware.com), Samer El-Haj-Mahmoud,
	nd

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel via groups.io
> Sent: Sunday, April 19, 2020 9:34 AM
> To: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>;
> devel@edk2.groups.io
> Cc: Leif Lindholm <leif@nuviainc.com>; Pete Batard <pete@akeo.ie>; Andrei
> Warkentin (awarkentin@vmware.com) <awarkentin@vmware.com>
> Subject: Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi
> : Enable TFTP shell command
> 
> On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
> > Fix an ASSERT with the TFTP dynamic Shell command on the
> > RPi3 and RPi4 when running DEBUG builds. Also, enable the command by
> > default for all builds.
> >
> 
> Fixing the ASSERT is fine but I am reluctant to enable this by default.
> It is a non-standard hack that ARM contributed in the past, and is not covered
> by the EFI of Shell specifications. If RPi4 is intended to be a showcase for UEFI
> on ARM done right, we should not enable this at all.
> 

That is OK. 

Are you fine just reviewing/pushing the PCD patches (and dropping the enable ones), or want me to send a new series without the enable patches ?

> 
> 
> 
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > Cc: Pete Batard <pete@akeo.ie>
> > Cc: Andrei Warkentin <awarkentin@vmware.com>
> >
> > Samer El-Haj-Mahmoud (4):
> >    Platform/RaspberryPi/RPi3: Fix TFTP dynamic command initialization
> >    Platform/RaspberryPi/RPi4: Fix TFTP dynamic command initialization
> >    Platform/RaspberryPi/RPi3: Enable TFTP command by default
> >    Platform/RaspberryPi/RPi4: Enable TFTP command by default
> >
> >   Platform/RaspberryPi/RPi3/RPi3.dsc | 7 +++++--
> >   Platform/RaspberryPi/RPi4/RPi4.dsc | 7 +++++--
> >   2 files changed, 10 insertions(+), 4 deletions(-)
> >
> 
> 
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 14:00   ` [edk2-devel] " Samer El-Haj-Mahmoud
@ 2020-04-19 14:04     ` Ard Biesheuvel
  0 siblings, 0 replies; 25+ messages in thread
From: Ard Biesheuvel @ 2020-04-19 14:04 UTC (permalink / raw)
  To: Samer El-Haj-Mahmoud, devel@edk2.groups.io, Samer El-Haj-Mahmoud
  Cc: Leif Lindholm, Pete Batard,
	Andrei Warkentin (awarkentin@vmware.com), nd

On 4/19/20 4:00 PM, Samer El-Haj-Mahmoud wrote:
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
>> Biesheuvel via groups.io
>> Sent: Sunday, April 19, 2020 9:34 AM
>> To: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>;
>> devel@edk2.groups.io
>> Cc: Leif Lindholm <leif@nuviainc.com>; Pete Batard <pete@akeo.ie>; Andrei
>> Warkentin (awarkentin@vmware.com) <awarkentin@vmware.com>
>> Subject: Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi
>> : Enable TFTP shell command
>>
>> On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
>>> Fix an ASSERT with the TFTP dynamic Shell command on the
>>> RPi3 and RPi4 when running DEBUG builds. Also, enable the command by
>>> default for all builds.
>>>
>>
>> Fixing the ASSERT is fine but I am reluctant to enable this by default.
>> It is a non-standard hack that ARM contributed in the past, and is not covered
>> by the EFI of Shell specifications. If RPi4 is intended to be a showcase for UEFI
>> on ARM done right, we should not enable this at all.
>>
> 
> That is OK.
> 
> Are you fine just reviewing/pushing the PCD patches (and dropping the enable ones), or want me to send a new series without the enable patches ?
> 

No that's fine, I'll just take the first two patches.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 13:33 ` [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Ard Biesheuvel
  2020-04-19 14:00   ` [edk2-devel] " Samer El-Haj-Mahmoud
@ 2020-04-19 14:19   ` Pete Batard
  2020-04-19 19:56     ` Andrei Warkentin
  1 sibling, 1 reply; 25+ messages in thread
From: Pete Batard @ 2020-04-19 14:19 UTC (permalink / raw)
  To: Ard Biesheuvel, Samer El-Haj-Mahmoud, devel
  Cc: Leif Lindholm, Andrei Warkentin

On 2020.04.19 14:33, Ard Biesheuvel wrote:
> On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
>> Fix an ASSERT with the TFTP dynamic Shell command on the
>> RPi3 and RPi4 when running DEBUG builds. Also, enable the
>> command by default for all builds.
>>
> 
> Fixing the ASSERT is fine but I am reluctant to enable this by default.

I'm going to second this.

To answer a question Samer was asking elsewhere, this is actually part 
of the reason why TFTP is not enabled in the DEBUG builds we produce at 
https://github.com/pftf/RPi4 (See build_firmware.sh), the reasoning 
being that if someone encounters an issue with RELEASE and we ask them 
to troubleshoot with the DEBUG artifact, we want to eliminate potential 
troublemakers when they try that.

> It is a non-standard hack that ARM contributed in the past, and is not 
> covered by the EFI of Shell specifications. If RPi4 is intended to be a 
> showcase for UEFI on ARM done right, we should not enable this at all.

Here I have to point out that RPi4 becoming a showcase because we intend 
to is not what we are pursuing (because if it was a matter of "willing" 
a showcase into existence, we would have picked a platform with a lot 
less quirks, more comprehensive documentation, and so on).

Instead, we estimate that due to its price point and widespread 
availability, it *is* going to become a de facto showcase, whether 
everybody likes it or not. And that is the reason we want to treat is as 
a showcase where possible.

Regards,

/Pete

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 14:19   ` Pete Batard
@ 2020-04-19 19:56     ` Andrei Warkentin
  2020-04-19 20:06       ` [edk2-devel] " Pete Batard
  2020-04-20 11:23       ` Leif Lindholm
  0 siblings, 2 replies; 25+ messages in thread
From: Andrei Warkentin @ 2020-04-19 19:56 UTC (permalink / raw)
  To: Pete Batard, Ard Biesheuvel, Samer El-Haj-Mahmoud,
	devel@edk2.groups.io
  Cc: Leif Lindholm

[-- Attachment #1: Type: text/plain, Size: 2807 bytes --]

Hi folks,

If we have to choose abstract goodness over functionality, why wouldn't we choose functionality? Functionality that's part of Tiano? The real world doesn't care about the TFTP command being an "unsupported hack" or not. So there's Tiano-specific code here. Big deal? To rephrase differently, why would either Pi 4 developers or Pi 4 UEFI users pay the cost of Tiano carrying code that somehow isn't "legit enough" to be enabled?

I mean here we are again, where what goes into the code is being dictated by some abstract ideology instead of technical reasons?

A
________________________________
From: Pete Batard <pete@akeo.ie>
Sent: Sunday, April 19, 2020 9:19 AM
To: Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Leif Lindholm <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>
Subject: Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command

On 2020.04.19 14:33, Ard Biesheuvel wrote:
> On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
>> Fix an ASSERT with the TFTP dynamic Shell command on the
>> RPi3 and RPi4 when running DEBUG builds. Also, enable the
>> command by default for all builds.
>>
>
> Fixing the ASSERT is fine but I am reluctant to enable this by default.

I'm going to second this.

To answer a question Samer was asking elsewhere, this is actually part
of the reason why TFTP is not enabled in the DEBUG builds we produce at
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpftf%2FRPi4&amp;data=02%7C01%7Cawarkentin%40vmware.com%7C27a7ee62d7734fc2000b08d7e46cb8be%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637229027923593475&amp;sdata=rKm%2BRZE%2FPWkvNTinHgQ5qgA9EDcD4lyIZSZMsHf66d0%3D&amp;reserved=0 (See build_firmware.sh), the reasoning
being that if someone encounters an issue with RELEASE and we ask them
to troubleshoot with the DEBUG artifact, we want to eliminate potential
troublemakers when they try that.

> It is a non-standard hack that ARM contributed in the past, and is not
> covered by the EFI of Shell specifications. If RPi4 is intended to be a
> showcase for UEFI on ARM done right, we should not enable this at all.

Here I have to point out that RPi4 becoming a showcase because we intend
to is not what we are pursuing (because if it was a matter of "willing"
a showcase into existence, we would have picked a platform with a lot
less quirks, more comprehensive documentation, and so on).

Instead, we estimate that due to its price point and widespread
availability, it *is* going to become a de facto showcase, whether
everybody likes it or not. And that is the reason we want to treat is as
a showcase where possible.

Regards,

/Pete

[-- Attachment #2: Type: text/html, Size: 4746 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 19:56     ` Andrei Warkentin
@ 2020-04-19 20:06       ` Pete Batard
  2020-04-19 20:21         ` Andrei Warkentin
  2020-04-20 11:23       ` Leif Lindholm
  1 sibling, 1 reply; 25+ messages in thread
From: Pete Batard @ 2020-04-19 20:06 UTC (permalink / raw)
  To: devel, awarkentin, Ard Biesheuvel, Samer El-Haj-Mahmoud; +Cc: Leif Lindholm

Andrei,

In case this is your concern, please note that we are not removing TFTP 
support at all, which is enabled for the RELEASE builds we produce and 
will remain so (and which anyone can enable with the macro if they wish).

All that will be changed by the updated proposal is that the current 
DEBUG ASSERT will be fixed and TFTP support will remain optional, like 
it is today.

So, in this case, I don't think your concern is warranted, because we're 
not actually taking any step to deprive anyone of any functionality they 
might wish for, and, even with the revised patch, TFTP will remain 
enabled in our RELEASE binaries, exactly as it has been before.

Regards,

/Pete

On 2020.04.19 20:56, Andrei Warkentin wrote:
> Hi folks,
> 
> If we have to choose abstract goodness over functionality, why wouldn't 
> we choose functionality? Functionality that's part of Tiano? The real 
> world doesn't care about the TFTP command being an "unsupported hack" or 
> not. So there's Tiano-specific code here. Big deal? To rephrase 
> differently, why would either Pi 4 developers or Pi 4 UEFI users pay the 
> cost of Tiano carrying code that somehow isn't "legit enough" to be enabled?
> 
> I mean here we are again, where what goes into the code is being 
> dictated by some abstract ideology instead of technical reasons?
> 
> A
> ------------------------------------------------------------------------
> *From:* Pete Batard <pete@akeo.ie>
> *Sent:* Sunday, April 19, 2020 9:19 AM
> *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud 
> <samer@elhajmahmoud.com>; devel@edk2.groups.io <devel@edk2.groups.io>
> *Cc:* Leif Lindholm <leif@nuviainc.com>; Andrei Warkentin 
> <awarkentin@vmware.com>
> *Subject:* Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : 
> Enable TFTP shell command
> On 2020.04.19 14:33, Ard Biesheuvel wrote:
>> On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
>>> Fix an ASSERT with the TFTP dynamic Shell command on the
>>> RPi3 and RPi4 when running DEBUG builds. Also, enable the
>>> command by default for all builds.
>>>
>> 
>> Fixing the ASSERT is fine but I am reluctant to enable this by default.
> 
> I'm going to second this.
> 
> To answer a question Samer was asking elsewhere, this is actually part
> of the reason why TFTP is not enabled in the DEBUG builds we produce at
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpftf%2FRPi4&amp;data=02%7C01%7Cawarkentin%40vmware.com%7C27a7ee62d7734fc2000b08d7e46cb8be%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637229027923593475&amp;sdata=rKm%2BRZE%2FPWkvNTinHgQ5qgA9EDcD4lyIZSZMsHf66d0%3D&amp;reserved=0 
> (See build_firmware.sh), the reasoning
> being that if someone encounters an issue with RELEASE and we ask them
> to troubleshoot with the DEBUG artifact, we want to eliminate potential
> troublemakers when they try that.
> 
>> It is a non-standard hack that ARM contributed in the past, and is not 
>> covered by the EFI of Shell specifications. If RPi4 is intended to be a 
>> showcase for UEFI on ARM done right, we should not enable this at all.
> 
> Here I have to point out that RPi4 becoming a showcase because we intend
> to is not what we are pursuing (because if it was a matter of "willing"
> a showcase into existence, we would have picked a platform with a lot
> less quirks, more comprehensive documentation, and so on).
> 
> Instead, we estimate that due to its price point and widespread
> availability, it *is* going to become a de facto showcase, whether
> everybody likes it or not. And that is the reason we want to treat is as
> a showcase where possible.
> 
> Regards,
> 
> /Pete
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 20:06       ` [edk2-devel] " Pete Batard
@ 2020-04-19 20:21         ` Andrei Warkentin
  2020-04-19 20:24           ` Pete Batard
  0 siblings, 1 reply; 25+ messages in thread
From: Andrei Warkentin @ 2020-04-19 20:21 UTC (permalink / raw)
  To: devel@edk2.groups.io, Ard Biesheuvel, Samer El-Haj-Mahmoud,
	pete@akeo.ie
  Cc: Leif Lindholm

[-- Attachment #1: Type: text/plain, Size: 4403 bytes --]

So if I understood correctly:

  *   If a random person off the street builds edk2 - they don't get TFTP command out of the box
  *   Our builds retain TFTP command

Correct?
________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Pete Batard via groups.io <pete=akeo.ie@groups.io>
Sent: Sunday, April 19, 2020 3:06 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Andrei Warkentin <awarkentin@vmware.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Subject: Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command

Andrei,

In case this is your concern, please note that we are not removing TFTP
support at all, which is enabled for the RELEASE builds we produce and
will remain so (and which anyone can enable with the macro if they wish).

All that will be changed by the updated proposal is that the current
DEBUG ASSERT will be fixed and TFTP support will remain optional, like
it is today.

So, in this case, I don't think your concern is warranted, because we're
not actually taking any step to deprive anyone of any functionality they
might wish for, and, even with the revised patch, TFTP will remain
enabled in our RELEASE binaries, exactly as it has been before.

Regards,

/Pete

On 2020.04.19 20:56, Andrei Warkentin wrote:
> Hi folks,
>
> If we have to choose abstract goodness over functionality, why wouldn't
> we choose functionality? Functionality that's part of Tiano? The real
> world doesn't care about the TFTP command being an "unsupported hack" or
> not. So there's Tiano-specific code here. Big deal? To rephrase
> differently, why would either Pi 4 developers or Pi 4 UEFI users pay the
> cost of Tiano carrying code that somehow isn't "legit enough" to be enabled?
>
> I mean here we are again, where what goes into the code is being
> dictated by some abstract ideology instead of technical reasons?
>
> A
> ------------------------------------------------------------------------
> *From:* Pete Batard <pete@akeo.ie>
> *Sent:* Sunday, April 19, 2020 9:19 AM
> *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud
> <samer@elhajmahmoud.com>; devel@edk2.groups.io <devel@edk2.groups.io>
> *Cc:* Leif Lindholm <leif@nuviainc.com>; Andrei Warkentin
> <awarkentin@vmware.com>
> *Subject:* Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi :
> Enable TFTP shell command
> On 2020.04.19 14:33, Ard Biesheuvel wrote:
>> On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
>>> Fix an ASSERT with the TFTP dynamic Shell command on the
>>> RPi3 and RPi4 when running DEBUG builds. Also, enable the
>>> command by default for all builds.
>>>
>>
>> Fixing the ASSERT is fine but I am reluctant to enable this by default.
>
> I'm going to second this.
>
> To answer a question Samer was asking elsewhere, this is actually part
> of the reason why TFTP is not enabled in the DEBUG builds we produce at
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpftf%2FRPi4&amp;data=02%7C01%7Cawarkentin%40vmware.com%7C56cfed340a494707ab0d08d7e49d2d64%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637229236035259614&amp;sdata=Ed8iabD5ARgBBYtsWgGw2Webu7dAW5Q9ju3qyqyVzX4%3D&amp;reserved=0
> (See build_firmware.sh), the reasoning
> being that if someone encounters an issue with RELEASE and we ask them
> to troubleshoot with the DEBUG artifact, we want to eliminate potential
> troublemakers when they try that.
>
>> It is a non-standard hack that ARM contributed in the past, and is not
>> covered by the EFI of Shell specifications. If RPi4 is intended to be a
>> showcase for UEFI on ARM done right, we should not enable this at all.
>
> Here I have to point out that RPi4 becoming a showcase because we intend
> to is not what we are pursuing (because if it was a matter of "willing"
> a showcase into existence, we would have picked a platform with a lot
> less quirks, more comprehensive documentation, and so on).
>
> Instead, we estimate that due to its price point and widespread
> availability, it *is* going to become a de facto showcase, whether
> everybody likes it or not. And that is the reason we want to treat is as
> a showcase where possible.
>
> Regards,
>
> /Pete
>





[-- Attachment #2: Type: text/html, Size: 6261 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 20:21         ` Andrei Warkentin
@ 2020-04-19 20:24           ` Pete Batard
  2020-04-19 20:42             ` Andrei Warkentin
       [not found]             ` <160753416257B0CA.29096@groups.io>
  0 siblings, 2 replies; 25+ messages in thread
From: Pete Batard @ 2020-04-19 20:24 UTC (permalink / raw)
  To: awarkentin, devel@edk2.groups.io, Ard Biesheuvel,
	Samer El-Haj-Mahmoud
  Cc: Leif Lindholm

On 2020.04.19 21:21, awarkentin@vmware.com wrote:
> So if I understood correctly:
> 
>   * If a random person off the street builds edk2 - they don't get TFTP
>     command out of the box

Yup. For the reasons that Ard pointed out (current TFTP being a 
non-standard hack that should be replaced by something more suitable... 
eventually).

>   * Our builds retain TFTP command

Yup.

> 
> Correct?
> ------------------------------------------------------------------------
> *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Pete 
> Batard via groups.io <pete=akeo.ie@groups.io>
> *Sent:* Sunday, April 19, 2020 3:06 PM
> *To:* devel@edk2.groups.io <devel@edk2.groups.io>; Andrei Warkentin 
> <awarkentin@vmware.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer 
> El-Haj-Mahmoud <samer@elhajmahmoud.com>
> *Cc:* Leif Lindholm <leif@nuviainc.com>
> *Subject:* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] 
> Platform/RaspberryPi : Enable TFTP shell command
> Andrei,
> 
> In case this is your concern, please note that we are not removing TFTP
> support at all, which is enabled for the RELEASE builds we produce and
> will remain so (and which anyone can enable with the macro if they wish).
> 
> All that will be changed by the updated proposal is that the current
> DEBUG ASSERT will be fixed and TFTP support will remain optional, like
> it is today.
> 
> So, in this case, I don't think your concern is warranted, because we're
> not actually taking any step to deprive anyone of any functionality they
> might wish for, and, even with the revised patch, TFTP will remain
> enabled in our RELEASE binaries, exactly as it has been before.
> 
> Regards,
> 
> /Pete
> 
> On 2020.04.19 20:56, Andrei Warkentin wrote:
>> Hi folks,
>> 
>> If we have to choose abstract goodness over functionality, why wouldn't 
>> we choose functionality? Functionality that's part of Tiano? The real 
>> world doesn't care about the TFTP command being an "unsupported hack" or 
>> not. So there's Tiano-specific code here. Big deal? To rephrase 
>> differently, why would either Pi 4 developers or Pi 4 UEFI users pay the 
>> cost of Tiano carrying code that somehow isn't "legit enough" to be enabled?
>> 
>> I mean here we are again, where what goes into the code is being 
>> dictated by some abstract ideology instead of technical reasons?
>> 
>> A
>> ------------------------------------------------------------------------
>> *From:* Pete Batard <pete@akeo.ie>
>> *Sent:* Sunday, April 19, 2020 9:19 AM
>> *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud 
>> <samer@elhajmahmoud.com>; devel@edk2.groups.io <devel@edk2.groups.io>
>> *Cc:* Leif Lindholm <leif@nuviainc.com>; Andrei Warkentin 
>> <awarkentin@vmware.com>
>> *Subject:* Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : 
>> Enable TFTP shell command
>> On 2020.04.19 14:33, Ard Biesheuvel wrote:
>>> On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
>>>> Fix an ASSERT with the TFTP dynamic Shell command on the
>>>> RPi3 and RPi4 when running DEBUG builds. Also, enable the
>>>> command by default for all builds.
>>>>
>>> 
>>> Fixing the ASSERT is fine but I am reluctant to enable this by default.
>> 
>> I'm going to second this.
>> 
>> To answer a question Samer was asking elsewhere, this is actually part
>> of the reason why TFTP is not enabled in the DEBUG builds we produce at
>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpftf%2FRPi4&amp;data=02%7C01%7Cawarkentin%40vmware.com%7C56cfed340a494707ab0d08d7e49d2d64%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637229236035259614&amp;sdata=Ed8iabD5ARgBBYtsWgGw2Webu7dAW5Q9ju3qyqyVzX4%3D&amp;reserved=0 
> 
>> (See build_firmware.sh), the reasoning
>> being that if someone encounters an issue with RELEASE and we ask them
>> to troubleshoot with the DEBUG artifact, we want to eliminate potential
>> troublemakers when they try that.
>> 
>>> It is a non-standard hack that ARM contributed in the past, and is not 
>>> covered by the EFI of Shell specifications. If RPi4 is intended to be a 
>>> showcase for UEFI on ARM done right, we should not enable this at all.
>> 
>> Here I have to point out that RPi4 becoming a showcase because we intend
>> to is not what we are pursuing (because if it was a matter of "willing"
>> a showcase into existence, we would have picked a platform with a lot
>> less quirks, more comprehensive documentation, and so on).
>> 
>> Instead, we estimate that due to its price point and widespread
>> availability, it *is* going to become a de facto showcase, whether
>> everybody likes it or not. And that is the reason we want to treat is as
>> a showcase where possible.
>> 
>> Regards,
>> 
>> /Pete
>> 
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 20:24           ` Pete Batard
@ 2020-04-19 20:42             ` Andrei Warkentin
       [not found]             ` <160753416257B0CA.29096@groups.io>
  1 sibling, 0 replies; 25+ messages in thread
From: Andrei Warkentin @ 2020-04-19 20:42 UTC (permalink / raw)
  To: Pete Batard, devel@edk2.groups.io, Ard Biesheuvel,
	Samer El-Haj-Mahmoud
  Cc: Leif Lindholm

[-- Attachment #1: Type: text/plain, Size: 5820 bytes --]

It's part of Tiano, no? We didn't develop it. Yet I see it being used in many Tiano-derived UEFI implementations in the Arm world. I don't see a contract anywhere that all Tiano implementations ought to avoid components that don't fit the UEFI/PI/Shell specs. Can someone point me to such a contract?

We're entering the "victimless crime" territory here, and also violating the principle of least surprise.

I do agree that DEBUG profile may choose a subset of configuration options, for reasons such as sticking to a smaller configuration (for size, complexity, etc).

A
________________________________
From: Pete Batard <pete@akeo.ie>
Sent: Sunday, April 19, 2020 3:24 PM
To: Andrei Warkentin <awarkentin@vmware.com>; devel@edk2.groups.io <devel@edk2.groups.io>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Subject: Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command

On 2020.04.19 21:21, awarkentin@vmware.com wrote:
> So if I understood correctly:
>
>   * If a random person off the street builds edk2 - they don't get TFTP
>     command out of the box

Yup. For the reasons that Ard pointed out (current TFTP being a
non-standard hack that should be replaced by something more suitable...
eventually).

>   * Our builds retain TFTP command

Yup.

>
> Correct?
> ------------------------------------------------------------------------
> *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Pete
> Batard via groups.io <pete=akeo.ie@groups.io>
> *Sent:* Sunday, April 19, 2020 3:06 PM
> *To:* devel@edk2.groups.io <devel@edk2.groups.io>; Andrei Warkentin
> <awarkentin@vmware.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer
> El-Haj-Mahmoud <samer@elhajmahmoud.com>
> *Cc:* Leif Lindholm <leif@nuviainc.com>
> *Subject:* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4]
> Platform/RaspberryPi : Enable TFTP shell command
> Andrei,
>
> In case this is your concern, please note that we are not removing TFTP
> support at all, which is enabled for the RELEASE builds we produce and
> will remain so (and which anyone can enable with the macro if they wish).
>
> All that will be changed by the updated proposal is that the current
> DEBUG ASSERT will be fixed and TFTP support will remain optional, like
> it is today.
>
> So, in this case, I don't think your concern is warranted, because we're
> not actually taking any step to deprive anyone of any functionality they
> might wish for, and, even with the revised patch, TFTP will remain
> enabled in our RELEASE binaries, exactly as it has been before.
>
> Regards,
>
> /Pete
>
> On 2020.04.19 20:56, Andrei Warkentin wrote:
>> Hi folks,
>>
>> If we have to choose abstract goodness over functionality, why wouldn't
>> we choose functionality? Functionality that's part of Tiano? The real
>> world doesn't care about the TFTP command being an "unsupported hack" or
>> not. So there's Tiano-specific code here. Big deal? To rephrase
>> differently, why would either Pi 4 developers or Pi 4 UEFI users pay the
>> cost of Tiano carrying code that somehow isn't "legit enough" to be enabled?
>>
>> I mean here we are again, where what goes into the code is being
>> dictated by some abstract ideology instead of technical reasons?
>>
>> A
>> ------------------------------------------------------------------------
>> *From:* Pete Batard <pete@akeo.ie>
>> *Sent:* Sunday, April 19, 2020 9:19 AM
>> *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud
>> <samer@elhajmahmoud.com>; devel@edk2.groups.io <devel@edk2.groups.io>
>> *Cc:* Leif Lindholm <leif@nuviainc.com>; Andrei Warkentin
>> <awarkentin@vmware.com>
>> *Subject:* Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi :
>> Enable TFTP shell command
>> On 2020.04.19 14:33, Ard Biesheuvel wrote:
>>> On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
>>>> Fix an ASSERT with the TFTP dynamic Shell command on the
>>>> RPi3 and RPi4 when running DEBUG builds. Also, enable the
>>>> command by default for all builds.
>>>>
>>>
>>> Fixing the ASSERT is fine but I am reluctant to enable this by default.
>>
>> I'm going to second this.
>>
>> To answer a question Samer was asking elsewhere, this is actually part
>> of the reason why TFTP is not enabled in the DEBUG builds we produce at
>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpftf%2FRPi4&amp;data=02%7C01%7Cawarkentin%40vmware.com%7Cff25433f108e490d28fc08d7e49fa694%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637229246665197560&amp;sdata=rgUNgoQgPZGgfcQaQfzE6hn3WNj1Y5sgv6Zr9pbCJGg%3D&amp;reserved=0
>
>> (See build_firmware.sh), the reasoning
>> being that if someone encounters an issue with RELEASE and we ask them
>> to troubleshoot with the DEBUG artifact, we want to eliminate potential
>> troublemakers when they try that.
>>
>>> It is a non-standard hack that ARM contributed in the past, and is not
>>> covered by the EFI of Shell specifications. If RPi4 is intended to be a
>>> showcase for UEFI on ARM done right, we should not enable this at all.
>>
>> Here I have to point out that RPi4 becoming a showcase because we intend
>> to is not what we are pursuing (because if it was a matter of "willing"
>> a showcase into existence, we would have picked a platform with a lot
>> less quirks, more comprehensive documentation, and so on).
>>
>> Instead, we estimate that due to its price point and widespread
>> availability, it *is* going to become a de facto showcase, whether
>> everybody likes it or not. And that is the reason we want to treat is as
>> a showcase where possible.
>>
>> Regards,
>>
>> /Pete
>>
>
>
> 
>


[-- Attachment #2: Type: text/html, Size: 8696 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
       [not found]             ` <160753416257B0CA.29096@groups.io>
@ 2020-04-19 23:50               ` Andrei Warkentin
  2020-04-20  1:03                 ` Andrew Fish
  0 siblings, 1 reply; 25+ messages in thread
From: Andrei Warkentin @ 2020-04-19 23:50 UTC (permalink / raw)
  To: Pete Batard, devel@edk2.groups.io, Ard Biesheuvel,
	Samer El-Haj-Mahmoud, Andrei Warkentin
  Cc: Leif Lindholm

[-- Attachment #1: Type: text/plain, Size: 6954 bytes --]

Stepping back, can we do a roll call among stakeholders so we start treating contentious issues as a community?

So far:

Samer (implicit, unless he says otherwise, because he proposed the patch): +1
Ard: -1
Pete: -1
Andrei: +1

Anyone else?

How many points do folks thing we need for a merge? I'd say +2.

A
________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Andrei Warkentin via groups.io <awarkentin=vmware.com@groups.io>
Sent: Sunday, April 19, 2020 3:42 PM
To: Pete Batard <pete@akeo.ie>; devel@edk2.groups.io <devel@edk2.groups.io>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Subject: Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command

It's part of Tiano, no? We didn't develop it. Yet I see it being used in many Tiano-derived UEFI implementations in the Arm world. I don't see a contract anywhere that all Tiano implementations ought to avoid components that don't fit the UEFI/PI/Shell specs. Can someone point me to such a contract?

We're entering the "victimless crime" territory here, and also violating the principle of least surprise.

I do agree that DEBUG profile may choose a subset of configuration options, for reasons such as sticking to a smaller configuration (for size, complexity, etc).

A
________________________________
From: Pete Batard <pete@akeo.ie>
Sent: Sunday, April 19, 2020 3:24 PM
To: Andrei Warkentin <awarkentin@vmware.com>; devel@edk2.groups.io <devel@edk2.groups.io>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Subject: Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command

On 2020.04.19 21:21, awarkentin@vmware.com wrote:
> So if I understood correctly:
>
>   * If a random person off the street builds edk2 - they don't get TFTP
>     command out of the box

Yup. For the reasons that Ard pointed out (current TFTP being a
non-standard hack that should be replaced by something more suitable...
eventually).

>   * Our builds retain TFTP command

Yup.

>
> Correct?
> ------------------------------------------------------------------------
> *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Pete
> Batard via groups.io <pete=akeo.ie@groups.io>
> *Sent:* Sunday, April 19, 2020 3:06 PM
> *To:* devel@edk2.groups.io <devel@edk2.groups.io>; Andrei Warkentin
> <awarkentin@vmware.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer
> El-Haj-Mahmoud <samer@elhajmahmoud.com>
> *Cc:* Leif Lindholm <leif@nuviainc.com>
> *Subject:* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4]
> Platform/RaspberryPi : Enable TFTP shell command
> Andrei,
>
> In case this is your concern, please note that we are not removing TFTP
> support at all, which is enabled for the RELEASE builds we produce and
> will remain so (and which anyone can enable with the macro if they wish).
>
> All that will be changed by the updated proposal is that the current
> DEBUG ASSERT will be fixed and TFTP support will remain optional, like
> it is today.
>
> So, in this case, I don't think your concern is warranted, because we're
> not actually taking any step to deprive anyone of any functionality they
> might wish for, and, even with the revised patch, TFTP will remain
> enabled in our RELEASE binaries, exactly as it has been before.
>
> Regards,
>
> /Pete
>
> On 2020.04.19 20:56, Andrei Warkentin wrote:
>> Hi folks,
>>
>> If we have to choose abstract goodness over functionality, why wouldn't
>> we choose functionality? Functionality that's part of Tiano? The real
>> world doesn't care about the TFTP command being an "unsupported hack" or
>> not. So there's Tiano-specific code here. Big deal? To rephrase
>> differently, why would either Pi 4 developers or Pi 4 UEFI users pay the
>> cost of Tiano carrying code that somehow isn't "legit enough" to be enabled?
>>
>> I mean here we are again, where what goes into the code is being
>> dictated by some abstract ideology instead of technical reasons?
>>
>> A
>> ------------------------------------------------------------------------
>> *From:* Pete Batard <pete@akeo.ie>
>> *Sent:* Sunday, April 19, 2020 9:19 AM
>> *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud
>> <samer@elhajmahmoud.com>; devel@edk2.groups.io <devel@edk2.groups.io>
>> *Cc:* Leif Lindholm <leif@nuviainc.com>; Andrei Warkentin
>> <awarkentin@vmware.com>
>> *Subject:* Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi :
>> Enable TFTP shell command
>> On 2020.04.19 14:33, Ard Biesheuvel wrote:
>>> On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
>>>> Fix an ASSERT with the TFTP dynamic Shell command on the
>>>> RPi3 and RPi4 when running DEBUG builds. Also, enable the
>>>> command by default for all builds.
>>>>
>>>
>>> Fixing the ASSERT is fine but I am reluctant to enable this by default.
>>
>> I'm going to second this.
>>
>> To answer a question Samer was asking elsewhere, this is actually part
>> of the reason why TFTP is not enabled in the DEBUG builds we produce at
>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpftf%2FRPi4&amp;data=02%7C01%7Cawarkentin%40vmware.com%7Cff25433f108e490d28fc08d7e49fa694%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637229246665197560&amp;sdata=rgUNgoQgPZGgfcQaQfzE6hn3WNj1Y5sgv6Zr9pbCJGg%3D&amp;reserved=0<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpftf%2FRPi4&data=02%7C01%7Cawarkentin%40vmware.com%7Ccdee787f93404c06347908d7e4a224c2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637229257370688870&sdata=HMeXi%2FIPSgNEcMZkoVAOjfFob9KVafL8tffomlyD7Ko%3D&reserved=0>
>
>> (See build_firmware.sh), the reasoning
>> being that if someone encounters an issue with RELEASE and we ask them
>> to troubleshoot with the DEBUG artifact, we want to eliminate potential
>> troublemakers when they try that.
>>
>>> It is a non-standard hack that ARM contributed in the past, and is not
>>> covered by the EFI of Shell specifications. If RPi4 is intended to be a
>>> showcase for UEFI on ARM done right, we should not enable this at all.
>>
>> Here I have to point out that RPi4 becoming a showcase because we intend
>> to is not what we are pursuing (because if it was a matter of "willing"
>> a showcase into existence, we would have picked a platform with a lot
>> less quirks, more comprehensive documentation, and so on).
>>
>> Instead, we estimate that due to its price point and widespread
>> availability, it *is* going to become a de facto showcase, whether
>> everybody likes it or not. And that is the reason we want to treat is as
>> a showcase where possible.
>>
>> Regards,
>>
>> /Pete
>>
>
>
>
>



[-- Attachment #2: Type: text/html, Size: 11699 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 23:50               ` Andrei Warkentin
@ 2020-04-20  1:03                 ` Andrew Fish
  2020-04-20  2:41                   ` Andrei Warkentin
  0 siblings, 1 reply; 25+ messages in thread
From: Andrew Fish @ 2020-04-20  1:03 UTC (permalink / raw)
  To: devel, awarkentin
  Cc: Pete Batard, Ard Biesheuvel, Samer El-Haj-Mahmoud, Leif Lindholm

[-- Attachment #1: Type: text/plain, Size: 8701 bytes --]



> On Apr 19, 2020, at 4:50 PM, Andrei Warkentin <awarkentin@vmware.com> wrote:
> 
> Stepping back, can we do a roll call among stakeholders so we start treating contentious issues as a community?
> 
> So far:
> 
> Samer (implicit, unless he says otherwise, because he proposed the patch): +1
> Ard: -1
> Pete: -1
> Andrei: +1
> 
> Anyone else?
> 

-100 since we don't use a points system :). 

The process is if the maintainers can not agree we let the Stewards decide. The next Stewards meeting is May 5th, so in the mean time we can try to reach consensus as that is our preference for a process. 

Maybe it would be useful to summarize the issue and state the pro and cons?


Thanks,

Andrew Fish

> How many points do folks thing we need for a merge? I'd say +2.
> 
> A
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> on behalf of Andrei Warkentin via groups.io <http://groups.io/> <awarkentin=vmware.com@groups.io <mailto:awarkentin=vmware.com@groups.io>>
> Sent: Sunday, April 19, 2020 3:42 PM
> To: Pete Batard <pete@akeo.ie <mailto:pete@akeo.ie>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>>; Ard Biesheuvel <ard.biesheuvel@arm.com <mailto:ard.biesheuvel@arm.com>>; Samer El-Haj-Mahmoud <samer@elhajmahmoud.com <mailto:samer@elhajmahmoud.com>>
> Cc: Leif Lindholm <leif@nuviainc.com <mailto:leif@nuviainc.com>>
> Subject: Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
>
> It's part of Tiano, no? We didn't develop it. Yet I see it being used in many Tiano-derived UEFI implementations in the Arm world. I don't see a contract anywhere that all Tiano implementations ought to avoid components that don't fit the UEFI/PI/Shell specs. Can someone point me to such a contract?
> 
> We're entering the "victimless crime" territory here, and also violating the principle of least surprise.
> 
> I do agree that DEBUG profile may choose a subset of configuration options, for reasons such as sticking to a smaller configuration (for size, complexity, etc).
> 
> A
> From: Pete Batard <pete@akeo.ie <mailto:pete@akeo.ie>>
> Sent: Sunday, April 19, 2020 3:24 PM
> To: Andrei Warkentin <awarkentin@vmware.com <mailto:awarkentin@vmware.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>>; Ard Biesheuvel <ard.biesheuvel@arm.com <mailto:ard.biesheuvel@arm.com>>; Samer El-Haj-Mahmoud <samer@elhajmahmoud.com <mailto:samer@elhajmahmoud.com>>
> Cc: Leif Lindholm <leif@nuviainc.com <mailto:leif@nuviainc.com>>
> Subject: Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
>
> On 2020.04.19 21:21, awarkentin@vmware.com <mailto:awarkentin@vmware.com> wrote:
> > So if I understood correctly:
> > 
> >   * If a random person off the street builds edk2 - they don't get TFTP
> >     command out of the box
> 
> Yup. For the reasons that Ard pointed out (current TFTP being a 
> non-standard hack that should be replaced by something more suitable... 
> eventually).
> 
> >   * Our builds retain TFTP command
> 
> Yup.
> 
> > 
> > Correct?
> > ------------------------------------------------------------------------
> > *From:* devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> on behalf of Pete 
> > Batard via groups.io <http://groups.io/> <pete=akeo.ie@groups.io <mailto:pete=akeo.ie@groups.io>>
> > *Sent:* Sunday, April 19, 2020 3:06 PM
> > *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>>; Andrei Warkentin 
> > <awarkentin@vmware.com <mailto:awarkentin@vmware.com>>; Ard Biesheuvel <ard.biesheuvel@arm.com <mailto:ard.biesheuvel@arm.com>>; Samer 
> > El-Haj-Mahmoud <samer@elhajmahmoud.com <mailto:samer@elhajmahmoud.com>>
> > *Cc:* Leif Lindholm <leif@nuviainc.com <mailto:leif@nuviainc.com>>
> > *Subject:* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] 
> > Platform/RaspberryPi : Enable TFTP shell command
> > Andrei,
> > 
> > In case this is your concern, please note that we are not removing TFTP
> > support at all, which is enabled for the RELEASE builds we produce and
> > will remain so (and which anyone can enable with the macro if they wish).
> > 
> > All that will be changed by the updated proposal is that the current
> > DEBUG ASSERT will be fixed and TFTP support will remain optional, like
> > it is today.
> > 
> > So, in this case, I don't think your concern is warranted, because we're
> > not actually taking any step to deprive anyone of any functionality they
> > might wish for, and, even with the revised patch, TFTP will remain
> > enabled in our RELEASE binaries, exactly as it has been before.
> > 
> > Regards,
> > 
> > /Pete
> > 
> > On 2020.04.19 20:56, Andrei Warkentin wrote:
> >> Hi folks,
> >> 
> >> If we have to choose abstract goodness over functionality, why wouldn't 
> >> we choose functionality? Functionality that's part of Tiano? The real 
> >> world doesn't care about the TFTP command being an "unsupported hack" or 
> >> not. So there's Tiano-specific code here. Big deal? To rephrase 
> >> differently, why would either Pi 4 developers or Pi 4 UEFI users pay the 
> >> cost of Tiano carrying code that somehow isn't "legit enough" to be enabled?
> >> 
> >> I mean here we are again, where what goes into the code is being 
> >> dictated by some abstract ideology instead of technical reasons?
> >> 
> >> A
> >> ------------------------------------------------------------------------
> >> *From:* Pete Batard <pete@akeo.ie <mailto:pete@akeo.ie>>
> >> *Sent:* Sunday, April 19, 2020 9:19 AM
> >> *To:* Ard Biesheuvel <ard.biesheuvel@arm.com <mailto:ard.biesheuvel@arm.com>>; Samer El-Haj-Mahmoud 
> >> <samer@elhajmahmoud.com <mailto:samer@elhajmahmoud.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>>
> >> *Cc:* Leif Lindholm <leif@nuviainc.com <mailto:leif@nuviainc.com>>; Andrei Warkentin 
> >> <awarkentin@vmware.com <mailto:awarkentin@vmware.com>>
> >> *Subject:* Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : 
> >> Enable TFTP shell command
> >> On 2020.04.19 14:33, Ard Biesheuvel wrote:
> >>> On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
> >>>> Fix an ASSERT with the TFTP dynamic Shell command on the
> >>>> RPi3 and RPi4 when running DEBUG builds. Also, enable the
> >>>> command by default for all builds.
> >>>>
> >>> 
> >>> Fixing the ASSERT is fine but I am reluctant to enable this by default.
> >> 
> >> I'm going to second this.
> >> 
> >> To answer a question Samer was asking elsewhere, this is actually part
> >> of the reason why TFTP is not enabled in the DEBUG builds we produce at
> >> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpftf%2FRPi4&amp;data=02%7C01%7Cawarkentin%40vmware.com%7Cff25433f108e490d28fc08d7e49fa694%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637229246665197560&amp;sdata=rgUNgoQgPZGgfcQaQfzE6hn3WNj1Y5sgv6Zr9pbCJGg%3D&amp;reserved=0 <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpftf%2FRPi4&data=02%7C01%7Cawarkentin%40vmware.com%7Ccdee787f93404c06347908d7e4a224c2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637229257370688870&sdata=HMeXi%2FIPSgNEcMZkoVAOjfFob9KVafL8tffomlyD7Ko%3D&reserved=0> 
> > 
> >> (See build_firmware.sh), the reasoning
> >> being that if someone encounters an issue with RELEASE and we ask them
> >> to troubleshoot with the DEBUG artifact, we want to eliminate potential
> >> troublemakers when they try that.
> >> 
> >>> It is a non-standard hack that ARM contributed in the past, and is not 
> >>> covered by the EFI of Shell specifications. If RPi4 is intended to be a 
> >>> showcase for UEFI on ARM done right, we should not enable this at all.
> >> 
> >> Here I have to point out that RPi4 becoming a showcase because we intend
> >> to is not what we are pursuing (because if it was a matter of "willing"
> >> a showcase into existence, we would have picked a platform with a lot
> >> less quirks, more comprehensive documentation, and so on).
> >> 
> >> Instead, we estimate that due to its price point and widespread
> >> availability, it *is* going to become a de facto showcase, whether
> >> everybody likes it or not. And that is the reason we want to treat is as
> >> a showcase where possible.
> >> 
> >> Regards,
> >> 
> >> /Pete
> >> 
> > 
> > 
> > 
> > 
> 
> 


[-- Attachment #2: Type: text/html, Size: 23510 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-20  1:03                 ` Andrew Fish
@ 2020-04-20  2:41                   ` Andrei Warkentin
  2020-04-20  6:45                     ` Ard Biesheuvel
  0 siblings, 1 reply; 25+ messages in thread
From: Andrei Warkentin @ 2020-04-20  2:41 UTC (permalink / raw)
  To: Andrew Fish, devel@edk2.groups.io
  Cc: Pete Batard, Ard Biesheuvel, Samer El-Haj-Mahmoud, Leif Lindholm

[-- Attachment #1: Type: text/plain, Size: 4599 bytes --]

Hi Andrew,

This particular issue revolves around enabling a feature by default in the DSC for Pi 3/4 platforms - the TFTP Shell command. Clearly, enabling this for Pi bears no effect on other Tiano platforms. So far I've heard something about the TFTP code being somehow bad, but what does that have to do with the Pi platform using it? If it's bad, delete it from Tiano. Or rewrite it.

.../last/ time we got into a spat about the value of revving the Pi ACPI to 6.3 (the ACPI support we contributed in the first place). So that patch just got ignored. Not even sure what to do about that one...

I don't know, the maintainers may in fact be in perfect agreement (I certainly do not know) - but certainly at odds with actual developers. So, there appears to be a problem with the way edk2-platforms is run. Or at the very least, I don't understand why it's run the way it's run.

The RPi3/4 support is developed by a group of folks (https://rpi4-uefi.dev, https://github.com/pftf). These folks (from Arm, from VMware, or unaffiliated) all follow their own goals, with some alignment, but there's overall consensus that working upstream is A Good Thing and is worth the effort. We loosely  coordinate with each other (Discord, have a bug tracker, etc). However, it seems that every patch we propose, to a code base that we largely wrote and chose to upstream into edk2-platforms, gets put under the microscope. That would be fair and square when we're talking about changes to shared components, or code that for technical reasons (quality or design) does not fit the implementation/design goals of Tiano. However, what ends up happening is _maintainers_ taking arbitrary decisions, contrary to the community of developers behind the port. As a final say. I don't mean an abstract mute community of developers. I mean the folks that specifically do all development today for Tiano on Pi platforms. We have no control. We have no say. Maintainers are certainly developers, and may have a stake and interest in the code being reviewed, but at the end of the day they are maintainers - they need to abstract away and agree to disagree on matters that are subjective and opinion-based.

Why are maintainers making these calls? We brought these platforms into your garden because we were sold on the idea. Let us water them and take care of them together. Instead, everything we do gets frisked with a strip search. We have to spend our time constantly proving the value of our work instead of using that very little valuable time we have to actually move the Pi support forward.

The outcomes are pretty serious. If you ask folks in the Arm community, they'll tell you "upstream first", and specifically cite lack of firmware being upstreamed as being a serious problem. However, the way edk2-platforms is structured, that goal is literally unobtainable because edk2-platforms development doesn't scale, as all code gets funneled through the few maintainers. I mean, why would anyone choose to upstream to edk2-platforms? For the lack of positive reinforcement? For the tedious process with no light at the end of the tunnel? It turns developers away. It makes folks feel unvalued and disenfranchised. You get to burn the midnight oil - pro bono - into something only to have others tell you that you should have done something else or just get your proposals ignored. It's not really a community - our choices, our code and our desire to CONTRIBUTE and BE RESPONSIBLE for a proper polished UEFI experience on the Raspberry Pi are worth absolutely nothing here.

Why does a maintainer have any say over platform-specific code, where there is developer consensus on the trajectory and there are no over-arching technical concerns? Why can't individual platforms have their own maintainers?

A

From: Andrew Fish <afish@apple.com>
Sent: Sunday, April 19, 2020 8:03 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Andrei Warkentin <awarkentin@vmware.com>
Cc: Pete Batard <pete@akeo.ie>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>; Leif Lindholm <leif@nuviainc.com>
Subject: Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command

-100 since we don't use a points system :).

The process is if the maintainers can not agree we let the Stewards decide. The next Stewards meeting is May 5th, so in the mean time we can try to reach consensus as that is our preference for a process.

Maybe it would be useful to summarize the issue and state the pro and cons?


Thanks,

Andrew Fish


[-- Attachment #2: Type: text/html, Size: 7481 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-20  2:41                   ` Andrei Warkentin
@ 2020-04-20  6:45                     ` Ard Biesheuvel
  2020-04-20 11:55                       ` Pete Batard
  0 siblings, 1 reply; 25+ messages in thread
From: Ard Biesheuvel @ 2020-04-20  6:45 UTC (permalink / raw)
  To: awarkentin, Andrew Fish, devel@edk2.groups.io
  Cc: Pete Batard, Samer El-Haj-Mahmoud, Leif Lindholm

On 4/20/20 4:41 AM, awarkentin@vmware.com wrote:
> Hi Andrew,
> 
> This particular issue revolves around enabling a feature by default in 
> the DSC for Pi 3/4 platforms - the TFTP Shell command. Clearly, enabling 
> this for Pi bears no effect on other Tiano platforms. So far I've heard 
> something about the TFTP code being somehow bad, but what does that have 
> to do with the Pi platform using it? If it's bad, delete it from Tiano. 
> Or rewrite it.
> 

The TFTP command can be included by a -D option on the build command 
line, but I am opposing to enabling it by default. The reason for this 
is that we have been trying to push back on custom vendor tools to do 
firmware upgrades and other things from the shell, and instead, use 
capsule update.

I really don't get what the drama is all about, given that the TFTP 
command is only a -D option away when you want it, and we don't 
distribute binary builds from edk2-platforms anyway.

> .../last/ time we got into a spat about the value of revving the Pi ACPI 
> to 6.3 (the ACPI support we contributed in the first place). So that 
> patch just got ignored. Not even sure what to do about that one...
> 

I rejected ACPI 6.3 table upgrades because, in their current form, the 
only thing they achieve is losing the ability to boot an OS that 
predates ACPI 6.3. Every piece of the platform currently being described 
via ACPI can be described using 5.1 tables.

ACPI 6.3 tables are not 'better' or 'more current' than 5.1 ones. ACPI 
6.3 tables should be used if/when they are needed, and not before.

> I don't know, the maintainers may in fact be in perfect agreement (I 
> certainly do not know) - but certainly at odds with actual developers. 
> So, there appears to be a problem with the way edk2-platforms is run. Or 
> at the very least, I don't understand why it's run the way it's run.
> 
> The RPi3/4 support is developed by a group of folks 
> (https://rpi4-uefi.dev, https://github.com/pftf). These folks (from Arm, 
> from VMware, or unaffiliated) all follow their own goals, with some 
> alignment, but there's overall consensus that working upstream is A Good 
> Thing and is worth the effort. We loosely  coordinate with each other 
> (Discord, have a bug tracker, etc). However, it seems that every patch 
> we propose, to a code base that we largely wrote and chose to upstream 
> into edk2-platforms, gets put under the microscope. That would be fair 
> and square when we're talking about changes to shared components, or 
> code that for technical reasons (quality or design) does not fit the 
> implementation/design goals of Tiano. However, what ends up happening is 
> _maintainers_ taking arbitrary decisions, contrary to the community of 
> developers behind the port. As a final say. I don't mean an abstract 
> mute community of developers. I mean the folks that specifically do all 
> development today for Tiano on Pi platforms. We have no control. We have 
> no say. Maintainers are certainly developers, and may have a stake and 
> interest in the code being reviewed, but at the end of the day they are 
> maintainers - they need to abstract away and agree to disagree on 
> matters that are subjective and opinion-based.
> 

It is funny how it depends on the context whether I am considered just a 
maintainer or one of the developers. Please don't misrepresent my 
involvement as sitting passively on the receiving end of a patch 
pipeline: I wrote the original barebones 64-bit port for Raspberry Pi in 
the first place, and I am [somewhat] active in the development community 
that you started around it.

> Why are maintainers making these calls? We brought these platforms into 
> your garden because we were sold on the idea. Let us water them and take 
> care of them together. Instead, everything we do gets frisked with a 
> strip search. We have to spend our time constantly proving the value of 
> our work instead of using that very little valuable time we have to 
> actually move the Pi support forward.
> 

Upstreaming code is a negotiation (via review) where the contributors 
and the maintainers converge on the terms under which the maintainers 
are willing to take charge of the code, and assume the responsibility 
that it will remain current and in working order going forward.

> The outcomes are pretty serious. If you ask folks in the Arm community, 
> they'll tell you "upstream first", and specifically cite lack of 
> firmware being upstreamed as being a serious problem. However, the way 
> edk2-platforms is structured, that goal is literally unobtainable 
> because edk2-platforms development doesn't scale, as all code gets 
> funneled through the few maintainers. I mean, why would anyone choose to 
> upstream to edk2-platforms? For the lack of positive reinforcement? For 
> the tedious process with no light at the end of the tunnel? It turns 
> developers away. It makes folks feel unvalued and disenfranchised. You 
> get to burn the midnight oil - pro bono - into something only to have 
> others tell you that you should have done something else or just get 
> your proposals ignored. It's not really a community - our choices, our 
> code and our desire to CONTRIBUTE and BE RESPONSIBLE for a proper 
> polished UEFI experience on the Raspberry Pi are worth absolutely 
> nothing here.
> 

Again, can we please cut the drama? I have objected to
a) replacing ACPI 5.1 tables with ACPI 6.3 tables for a piece of 
hardware that is 100% covered by the ACPI 5.1 spec
b) enabling TFTP by default

Beyond that, I don't remember any exchanges where we did not converge in 
the end on something we were all happy with.

> Why does a maintainer have any say over platform-specific code, where 
> there is developer consensus on the trajectory and there are no 
> over-arching technical concerns? Why can't individual platforms have 
> their own maintainers?
> 

If you think 'developer consensus' should be sufficient justification to 
take any change without scrutiny, I think you are looking for a hosting 
platform, not an upstream open source project.

Individual platforms can have their own maintainers. However, 
maintainership is a role you typically have to earn in a community.

What gets lost in the debate is the fact that we are all on the same 
side here: we all want the best possible support for Raspberry Pi 3/4 to 
be available in Tianocore. Escalating a silly difference of opinion such 
as the TFTP issue to these levels only distracts from that.

I have stated before that I *really* like the Raspberry Pi port for its 
polish and looks [none of which are my accomplishments], but the fact 
(intentional or not) that it may become an example followed by others 
when creating EDK2 ports for ARM means that I have a responsibility to 
my employer *and* the community to ensure that it is a good 
approximation of what state of art means for ARM platforms.

So better integration with the UEFI driver model for the onboard 
peripherals is high on my list, as well as moving away from PrePi, so we 
can start thinking about things like Capsule Update and measured boot 
(if you have the hardware).

I am not saying this is your responsibility (or anyone else's) but 
simply that the port is not finished, and so we haven't entered the 
phase yet where we're just dotting Is and crossing Ts. So if enabling 
TFTP support makes it easier to use a board specific hack to update your 
firmware, it shouldn't be enabled by default, and distract from the fact 
that we are still lacking support for UpdateCapsule().

-- 
Ard.


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 19:56     ` Andrei Warkentin
  2020-04-19 20:06       ` [edk2-devel] " Pete Batard
@ 2020-04-20 11:23       ` Leif Lindholm
  2020-04-20 13:17         ` [edk2-devel] " Samer El-Haj-Mahmoud
  1 sibling, 1 reply; 25+ messages in thread
From: Leif Lindholm @ 2020-04-20 11:23 UTC (permalink / raw)
  To: awarkentin, Ard Biesheuvel
  Cc: Pete Batard, Samer El-Haj-Mahmoud, devel@edk2.groups.io

Hi Andrei,

On Sun, Apr 19, 2020 at 19:56:32 +0000, awarkentin@vmware.com wrote:
> If we have to choose abstract goodness over functionality, why
> wouldn't we choose functionality? Functionality that's part of
> Tiano? The real world doesn't care about the TFTP command being an
> "unsupported hack" or not. So there's Tiano-specific code here. Big
> deal? To rephrase differently, why would either Pi 4 developers or
> Pi 4 UEFI users pay the cost of Tiano carrying code that somehow
> isn't "legit enough" to be enabled?

I agree there is confusion caused by this weird second implementation
of TFTP in Tianocore. It is yet another piece of unfortunate legacy
caused by ARM's initial focus on trying to make EDK2 look and work
like u-boot instead of understanding how to use it as a generic UEFI
implementation.

For that reason, it should have gone the same way as the ArmBds and
the built-in linux loader, but people kept arguing it was really
useful for debugging - so we let it be, and permitted platforms to
include it as long as it was not included by default.

But since its main contribution to TianoCore seems to be causing
arguments on the mailing list, perhaps we should finally bite the
bullet and nuke it.

The idea (which was probably mine) that "only permit platforms to
include it if it is disabled by default and people will get the hint"
has demonstrably failed.

/
    Leif

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-20  6:45                     ` Ard Biesheuvel
@ 2020-04-20 11:55                       ` Pete Batard
  2020-04-20 12:43                         ` Ard Biesheuvel
  0 siblings, 1 reply; 25+ messages in thread
From: Pete Batard @ 2020-04-20 11:55 UTC (permalink / raw)
  To: Ard Biesheuvel, awarkentin, Andrew Fish, devel@edk2.groups.io
  Cc: Samer El-Haj-Mahmoud, Leif Lindholm

On 2020.04.20 07:45, Ard Biesheuvel wrote:
> On 4/20/20 4:41 AM, awarkentin@vmware.com wrote:
>> Hi Andrew,
>>
>> This particular issue revolves around enabling a feature by default in 
>> the DSC for Pi 3/4 platforms - the TFTP Shell command. Clearly, 
>> enabling this for Pi bears no effect on other Tiano platforms. So far 
>> I've heard something about the TFTP code being somehow bad, but what 
>> does that have to do with the Pi platform using it? If it's bad, 
>> delete it from Tiano. Or rewrite it.
>>
> 
> The TFTP command can be included by a -D option on the build command 
> line, but I am opposing to enabling it by default. The reason for this 
> is that we have been trying to push back on custom vendor tools to do 
> firmware upgrades and other things from the shell, and instead, use 
> capsule update.
> 
> I really don't get what the drama is all about, given that the TFTP 
> command is only a -D option away when you want it, and we don't 
> distribute binary builds from edk2-platforms anyway.

Sorry Andrei, but I'm with Ard on that.

I don't get how something that is not going to affect end-users and, at 
worst, can only be construed as a minor developer inconvenience, is 
becoming a major contention point, especially if you are not pushing for 
Secure Boot to be enabled at the same time as TFTP, since, apart from 
the hackish nature of the code, we do have the exact same situation for 
Secure Boot (which we currently have as another -D option that is 
disabled by default).

Just like most end-users are likely to want tftp available as a shell 
command, I expect that most end-users are likely to want to have Secure 
Boot available, regardless of whether they plan to use it or not. That 
is effectively the reason why we are enabling both in the images we 
produce. Yet, you did indicate (on a different channel) that you don't 
see enabling Secure Boot by default as much as a necessity as TFTP, 
which doesn't make a lot of sense to me...

>> .../last/ time we got into a spat about the value of revving the Pi 
>> ACPI to 6.3 (the ACPI support we contributed in the first place). So 
>> that patch just got ignored. Not even sure what to do about that one...
>>
> 
> I rejected ACPI 6.3 table upgrades because, in their current form, the 
> only thing they achieve is losing the ability to boot an OS that 
> predates ACPI 6.3. Every piece of the platform currently being described 
> via ACPI can be described using 5.1 tables.

Actually, that is not technically correct.

But I will concede that you probably aren't aware of this, because we 
didn't bring up this point when we discussed the matter earlier.

One thing you may want to know is that most of the binary blobs we 
picked up from the Microsoft IoT endeavour were ACPI 6.0, and, 
especially, the MADT we got had actually been abusing the GICR Base 
Address field, which was introduced in 6.0, to place then  "nonsensical" 
value of 1 in there.

And for reasons that only Microsoft knows, unless you do have a value of 
1 there, Windows 10 on the Pi 3 will not boot at all. In other words, if 
you are to use ACPI 6.x with carry-over defaults from 5.1, Windows 10 
can simply not boot, at least on the Pi 3.

Now, what happened is that, when we started introducing some 5.1 tables 
elsewhere, I went with trying to harmonize everything to 5.1 and 
effectively *downgraded* the 6.0 tables we got from Microsoft to 5.1 
(which nobody seemed to mind then, on account, I will also assert, that 
nobody realized that we actually had 6.0 tables where 6.0 features might 
be relevant).

It turns out that, if you do use 5.1, Windows 10 *seems* to be happy 
about the MADT, even as it is obviously missing a GICR Base Address 
field with a 1 value, and *appears* to work as expected.

However, because of the proprietary nature of the OS, we have absolutely 
no idea whether having a "properly manipulated" GICR Base Address for 
Windows is important or not. The obvious reasoning is that, if Microsoft 
abused that field in their tables and especially if, when using ACPI 
6.x, Windows 10 doesn't boot unless it sees the expected value there, an 
ACPI 6.x MADT with the relevant value populated might be something that 
we actually want to see.

> ACPI 6.3 tables are not 'better' or 'more current' than 5.1 ones.

The example above begs to differ (though of course, short of actually 
knowing why the heck Microsoft decided to abuse GICR Base Address, it's 
hard to know what 'better' means in this case).

It does seem to me like Microsoft were effectively using the most recent 
ACPI version they could use when they produced their IoT tables. And we 
certainly did happen to downgrade some of that, and may have gotten 
lucky that it all seemed to work (with the point being that, if we seem 
to be happy that downgrading ACPI tables from 6.0 to 5.1 and validating 
that we aren't observing obvious ill effects is okay, then upgrading 
ACPI tables to 6.3 and validating that we aren't observing ill effects 
should be fine too).

> ACPI 
> 6.3 tables should be used if/when they are needed, and not before.

I disagree here again, on account of trying to showcase elements where 
we can, and this is one place we can do this.

I see absolutely zero technical reasons to want to stick with ACPI 5.1 
especially as we already have one 6.3 table (PPTT) and we're going to 
have to upgrade a couple others to at least 6.0 anyway to get back to 
the position we were in with the Microsoft's blobs, so that we can be 
more confident about Windows' behaviour.

As a matter of fact, using ACPI 6.3 before it is effectively needed is 
exactly the kind of move I see as wanting to apply when we know that a 
platform is going to be used as a de facto showcase. It may seem like a 
simple "newer is always better!" push to you, but the way I see it is 
that, by doing so, we are going to help developers of new platforms, who 
will be looking at the Pi for reference (whether we like it or not) and, 
if they are developing for modern hardware, are going to be looking at 
using new features that introduced only in recent ACPI. Thus, even if we 
don't make any use of these features on the Pi's, those developers might 
be grateful to find that there exists a working example of using a 
relatively up to date ACPI, and that they won't have to reinvent the wheel.

Especially, as opposed to what might be the case for other platforms, we 
don't have a baggage of "older" OSes that we may hinder support for if 
we do move to ACPI 6.3 (especially as, outside of Windows, most Pi 3 
OSes will be using DT rather than ACPI, and, without SD and Genet 
support, the idea of older OSes needing to work with Pi 4 is a bit 
ludicrous).

So, whereas I agree that your reasoning is generally sound for most 
platforms, the fact that the Pi is going to be used as a de-facto 
showcase or starting point by folks interested in developing a new ARM 
platform, makes, I will assert, the situation a bit different here, and 
I would really appreciate if you could give some more thoughts to the 
points I am trying to make. In this case, I believe that the Pi should 
be the "exception that proves the rule".

Furthermore, and this IMO is the most important point, we are not going 
to be sitting idle if we do upgrade to 6.3 and someone comes to us with 
a report that using modern ACPI appears to be causing them trouble 
(which, I will also assert, may actually be something that might be of 
great interest to folks on this list if that happens). As opposed to 
proprietary platforms, where ACPI is pretty much set in stone by the 
vendor and where addressing issues is a struggle, and, even if the 
vendor is very reactive and produces an update, where flashing a new 
firmware can be a tricky operation for some users, we are Open Source, 
easy to reach for reports, and the firmware "update" process of the Pi 
could not be any simpler (copy a file to an SD card and you're done). So 
I'd really like to help squelch the idea that if we do decide to upgrade 
to ACPI 6.3, and we effectively find out that it causes issues, we're 
simply going to stand by and let these unaddressed, whilst telling users 
of the platform "too bad". None of what we are doing is ever meant to be 
static or immutable, even after a goal has been reached. Thus, if there 
are issues with upgrading to 6.3, we're going to make darn sure that we 
address them, in the same way we have been planning to address issues 
that might be raised from some of our forced downgrades from 6.0 to 5.1.

Regards,

/Pete

> 
>> I don't know, the maintainers may in fact be in perfect agreement (I 
>> certainly do not know) - but certainly at odds with actual developers. 
>> So, there appears to be a problem with the way edk2-platforms is run. 
>> Or at the very least, I don't understand why it's run the way it's run.
>>
>> The RPi3/4 support is developed by a group of folks 
>> (https://rpi4-uefi.dev, https://github.com/pftf). These folks (from 
>> Arm, from VMware, or unaffiliated) all follow their own goals, with 
>> some alignment, but there's overall consensus that working upstream is 
>> A Good Thing and is worth the effort. We loosely  coordinate with each 
>> other (Discord, have a bug tracker, etc). However, it seems that every 
>> patch we propose, to a code base that we largely wrote and chose to 
>> upstream into edk2-platforms, gets put under the microscope. That 
>> would be fair and square when we're talking about changes to shared 
>> components, or code that for technical reasons (quality or design) 
>> does not fit the implementation/design goals of Tiano. However, what 
>> ends up happening is _maintainers_ taking arbitrary decisions, 
>> contrary to the community of developers behind the port. As a final 
>> say. I don't mean an abstract mute community of developers. I mean the 
>> folks that specifically do all development today for Tiano on Pi 
>> platforms. We have no control. We have no say. Maintainers are 
>> certainly developers, and may have a stake and interest in the code 
>> being reviewed, but at the end of the day they are maintainers - they 
>> need to abstract away and agree to disagree on matters that are 
>> subjective and opinion-based.
>>
> 
> It is funny how it depends on the context whether I am considered just a 
> maintainer or one of the developers. Please don't misrepresent my 
> involvement as sitting passively on the receiving end of a patch 
> pipeline: I wrote the original barebones 64-bit port for Raspberry Pi in 
> the first place, and I am [somewhat] active in the development community 
> that you started around it.
> 
>> Why are maintainers making these calls? We brought these platforms 
>> into your garden because we were sold on the idea. Let us water them 
>> and take care of them together. Instead, everything we do gets frisked 
>> with a strip search. We have to spend our time constantly proving the 
>> value of our work instead of using that very little valuable time we 
>> have to actually move the Pi support forward.
>>
> 
> Upstreaming code is a negotiation (via review) where the contributors 
> and the maintainers converge on the terms under which the maintainers 
> are willing to take charge of the code, and assume the responsibility 
> that it will remain current and in working order going forward.
> 
>> The outcomes are pretty serious. If you ask folks in the Arm 
>> community, they'll tell you "upstream first", and specifically cite 
>> lack of firmware being upstreamed as being a serious problem. However, 
>> the way edk2-platforms is structured, that goal is literally 
>> unobtainable because edk2-platforms development doesn't scale, as all 
>> code gets funneled through the few maintainers. I mean, why would 
>> anyone choose to upstream to edk2-platforms? For the lack of positive 
>> reinforcement? For the tedious process with no light at the end of the 
>> tunnel? It turns developers away. It makes folks feel unvalued and 
>> disenfranchised. You get to burn the midnight oil - pro bono - into 
>> something only to have others tell you that you should have done 
>> something else or just get your proposals ignored. It's not really a 
>> community - our choices, our code and our desire to CONTRIBUTE and BE 
>> RESPONSIBLE for a proper polished UEFI experience on the Raspberry Pi 
>> are worth absolutely nothing here.
>>
> 
> Again, can we please cut the drama? I have objected to
> a) replacing ACPI 5.1 tables with ACPI 6.3 tables for a piece of 
> hardware that is 100% covered by the ACPI 5.1 spec
> b) enabling TFTP by default
> 
> Beyond that, I don't remember any exchanges where we did not converge in 
> the end on something we were all happy with.
> 
>> Why does a maintainer have any say over platform-specific code, where 
>> there is developer consensus on the trajectory and there are no 
>> over-arching technical concerns? Why can't individual platforms have 
>> their own maintainers?
>>
> 
> If you think 'developer consensus' should be sufficient justification to 
> take any change without scrutiny, I think you are looking for a hosting 
> platform, not an upstream open source project.
> 
> Individual platforms can have their own maintainers. However, 
> maintainership is a role you typically have to earn in a community.
> 
> What gets lost in the debate is the fact that we are all on the same 
> side here: we all want the best possible support for Raspberry Pi 3/4 to 
> be available in Tianocore. Escalating a silly difference of opinion such 
> as the TFTP issue to these levels only distracts from that.
> 
> I have stated before that I *really* like the Raspberry Pi port for its 
> polish and looks [none of which are my accomplishments], but the fact 
> (intentional or not) that it may become an example followed by others 
> when creating EDK2 ports for ARM means that I have a responsibility to 
> my employer *and* the community to ensure that it is a good 
> approximation of what state of art means for ARM platforms.
> 
> So better integration with the UEFI driver model for the onboard 
> peripherals is high on my list, as well as moving away from PrePi, so we 
> can start thinking about things like Capsule Update and measured boot 
> (if you have the hardware).
> 
> I am not saying this is your responsibility (or anyone else's) but 
> simply that the port is not finished, and so we haven't entered the 
> phase yet where we're just dotting Is and crossing Ts. So if enabling 
> TFTP support makes it easier to use a board specific hack to update your 
> firmware, it shouldn't be enabled by default, and distract from the fact 
> that we are still lacking support for UpdateCapsule().
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-20 11:55                       ` Pete Batard
@ 2020-04-20 12:43                         ` Ard Biesheuvel
  2020-04-20 14:18                           ` Pete Batard
  0 siblings, 1 reply; 25+ messages in thread
From: Ard Biesheuvel @ 2020-04-20 12:43 UTC (permalink / raw)
  To: Pete Batard, awarkentin, Andrew Fish, devel@edk2.groups.io
  Cc: Samer El-Haj-Mahmoud, Leif Lindholm

On 4/20/20 1:55 PM, Pete Batard wrote:
> On 2020.04.20 07:45, Ard Biesheuvel wrote:
...
>> I rejected ACPI 6.3 table upgrades because, in their current form, the 
>> only thing they achieve is losing the ability to boot an OS that 
>> predates ACPI 6.3. Every piece of the platform currently being 
>> described via ACPI can be described using 5.1 tables.
> 
> Actually, that is not technically correct.
> 
> But I will concede that you probably aren't aware of this, because we 
> didn't bring up this point when we discussed the matter earlier.
> 
> One thing you may want to know is that most of the binary blobs we 
> picked up from the Microsoft IoT endeavour were ACPI 6.0, and, 
> especially, the MADT we got had actually been abusing the GICR Base 
> Address field, which was introduced in 6.0, to place then  "nonsensical" 
> value of 1 in there.
> 
> And for reasons that only Microsoft knows, unless you do have a value of 
> 1 there, Windows 10 on the Pi 3 will not boot at all. In other words, if 
> you are to use ACPI 6.x with carry-over defaults from 5.1, Windows 10 
> can simply not boot, at least on the Pi 3.
> 
> Now, what happened is that, when we started introducing some 5.1 tables 
> elsewhere, I went with trying to harmonize everything to 5.1 and 
> effectively *downgraded* the 6.0 tables we got from Microsoft to 5.1 
> (which nobody seemed to mind then, on account, I will also assert, that 
> nobody realized that we actually had 6.0 tables where 6.0 features might 
> be relevant).
> 
> It turns out that, if you do use 5.1, Windows 10 *seems* to be happy 
> about the MADT, even as it is obviously missing a GICR Base Address 
> field with a 1 value, and *appears* to work as expected.
> 
> However, because of the proprietary nature of the OS, we have absolutely 
> no idea whether having a "properly manipulated" GICR Base Address for 
> Windows is important or not. The obvious reasoning is that, if Microsoft 
> abused that field in their tables and especially if, when using ACPI 
> 6.x, Windows 10 doesn't boot unless it sees the expected value there, an 
> ACPI 6.x MADT with the relevant value populated might be something that 
> we actually want to see.
> 

I agree that it makes sense to harmonize the table versions, and avoid 
mixing and matching different versions of the spec. The structures are 
usually defined in a way where new fields added at then end, and the 
sizes are explicitly defined, so that an older OS can consume newer 
versions of the tables.

At least, that is the theory ...

>> ACPI 6.3 tables are not 'better' or 'more current' than 5.1 ones.
> 
> The example above begs to differ (though of course, short of actually 
> knowing why the heck Microsoft decided to abuse GICR Base Address, it's 
> hard to know what 'better' means in this case).
> 
> It does seem to me like Microsoft were effectively using the most recent 
> ACPI version they could use when they produced their IoT tables. And we 
> certainly did happen to downgrade some of that, and may have gotten 
> lucky that it all seemed to work (with the point being that, if we seem 
> to be happy that downgrading ACPI tables from 6.0 to 5.1 and validating 
> that we aren't observing obvious ill effects is okay, then upgrading 
> ACPI tables to 6.3 and validating that we aren't observing ill effects 
> should be fine too).
> 

If compatibility with Microsoft Windows 10 requires a certain minimum 
version of the tables, then I don't mind adhering to that.

>> ACPI 6.3 tables should be used if/when they are needed, and not before.
> 
> I disagree here again, on account of trying to showcase elements where 
> we can, and this is one place we can do this.
> 
> I see absolutely zero technical reasons to want to stick with ACPI 5.1 
> especially as we already have one 6.3 table (PPTT) and we're going to 
> have to upgrade a couple others to at least 6.0 anyway to get back to 
> the position we were in with the Microsoft's blobs, so that we can be 
> more confident about Windows' behaviour.
> 
> As a matter of fact, using ACPI 6.3 before it is effectively needed is 
> exactly the kind of move I see as wanting to apply when we know that a 
> platform is going to be used as a de facto showcase. It may seem like a 
> simple "newer is always better!" push to you, but the way I see it is 
> that, by doing so, we are going to help developers of new platforms, who 
> will be looking at the Pi for reference (whether we like it or not) and, 
> if they are developing for modern hardware, are going to be looking at 
> using new features that introduced only in recent ACPI. Thus, even if we 
> don't make any use of these features on the Pi's, those developers might 
> be grateful to find that there exists a working example of using a 
> relatively up to date ACPI, and that they won't have to reinvent the wheel.
> 
> Especially, as opposed to what might be the case for other platforms, we 
> don't have a baggage of "older" OSes that we may hinder support for if 
> we do move to ACPI 6.3 (especially as, outside of Windows, most Pi 3 
> OSes will be using DT rather than ACPI, and, without SD and Genet 
> support, the idea of older OSes needing to work with Pi 4 is a bit 
> ludicrous).
> 
> So, whereas I agree that your reasoning is generally sound for most 
> platforms, the fact that the Pi is going to be used as a de-facto 
> showcase or starting point by folks interested in developing a new ARM 
> platform, makes, I will assert, the situation a bit different here, and 
> I would really appreciate if you could give some more thoughts to the 
> points I am trying to make. In this case, I believe that the Pi should 
> be the "exception that proves the rule".
> 
> Furthermore, and this IMO is the most important point, we are not going 
> to be sitting idle if we do upgrade to 6.3 and someone comes to us with 
> a report that using modern ACPI appears to be causing them trouble 
> (which, I will also assert, may actually be something that might be of 
> great interest to folks on this list if that happens). As opposed to 
> proprietary platforms, where ACPI is pretty much set in stone by the 
> vendor and where addressing issues is a struggle, and, even if the 
> vendor is very reactive and produces an update, where flashing a new 
> firmware can be a tricky operation for some users, we are Open Source, 
> easy to reach for reports, and the firmware "update" process of the Pi 
> could not be any simpler (copy a file to an SD card and you're done). So 
> I'd really like to help squelch the idea that if we do decide to upgrade 
> to ACPI 6.3, and we effectively find out that it causes issues, we're 
> simply going to stand by and let these unaddressed, whilst telling users 
> of the platform "too bad". None of what we are doing is ever meant to be 
> static or immutable, even after a goal has been reached. Thus, if there 
> are issues with upgrading to 6.3, we're going to make darn sure that we 
> address them, in the same way we have been planning to address issues 
> that might be raised from some of our forced downgrades from 6.0 to 5.1.
> 

Thanks Pete.

I still don't agree with the whole showcase aspect of this discussion, 
but since there are apparently other good reasons to upgrade these 
tables that nobody brought up before, I am not going to oppose any 
changes on this front, provided that the commit logs articulate in 
sufficient detail why we think this is necessary (so that people looking 
at this port for guidance have the background as well)

Out of curiosity, why did we need a 6.3 PPTT for RPi4?




^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-20 11:23       ` Leif Lindholm
@ 2020-04-20 13:17         ` Samer El-Haj-Mahmoud
  0 siblings, 0 replies; 25+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-04-20 13:17 UTC (permalink / raw)
  To: devel@edk2.groups.io, leif@nuviainc.com,
	Andrei Warkentin (awarkentin@vmware.com), Ard Biesheuvel
  Cc: Pete Batard, Samer El-Haj-Mahmoud

I have no objection to dropping the "enable TFTP by default". I already stated that to Ard in my original reply. That is not really a big issue (or an issue at all) since we have the -D build option to enable the command in some builds, and the net is no functionality change for end-users.

In general, I am in favor of having Shell commands that are wrappers around useful UEFI protocols/functionality where it makes sense (e.g. would be nice to have a CLI HII forms processor/browser..). I thought the TFTP command offered that (a wrapper around the MTFTP4 protocol), which helps in some Shell remote scripting (although honestly, not very useful on its own). I understand that there is hesitation in including non-standard commands in the Shell by default. I did not consider TFTP as a replacement for Capsule Updates, but I can see how it can be abused to be so. I agree that usage should not be encouraged.

I still have not seen a review-by or ack for the ASSERT fix. Can we get patch 1 and 2 in the series reviewed and pushed please, and ignore patches 3/4?

Thanks,
--Samer




> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif
> Lindholm via groups.io
> Sent: Monday, April 20, 2020 7:23 AM
> To: Andrei Warkentin (awarkentin@vmware.com)
> <awarkentin@vmware.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>
> Cc: Pete Batard <pete@akeo.ie>; Samer El-Haj-Mahmoud
> <samer@elhajmahmoud.com>; devel@edk2.groups.io
> Subject: Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi
> : Enable TFTP shell command
>
> Hi Andrei,
>
> On Sun, Apr 19, 2020 at 19:56:32 +0000, awarkentin@vmware.com wrote:
> > If we have to choose abstract goodness over functionality, why
> > wouldn't we choose functionality? Functionality that's part of Tiano?
> > The real world doesn't care about the TFTP command being an
> > "unsupported hack" or not. So there's Tiano-specific code here. Big
> > deal? To rephrase differently, why would either Pi 4 developers or Pi
> > 4 UEFI users pay the cost of Tiano carrying code that somehow isn't
> > "legit enough" to be enabled?
>
> I agree there is confusion caused by this weird second implementation of TFTP
> in Tianocore. It is yet another piece of unfortunate legacy caused by ARM's
> initial focus on trying to make EDK2 look and work like u-boot instead of
> understanding how to use it as a generic UEFI implementation.
>
> For that reason, it should have gone the same way as the ArmBds and the built-
> in linux loader, but people kept arguing it was really useful for debugging - so
> we let it be, and permitted platforms to include it as long as it was not included
> by default.
>
> But since its main contribution to TianoCore seems to be causing arguments on
> the mailing list, perhaps we should finally bite the bullet and nuke it.
>
> The idea (which was probably mine) that "only permit platforms to include it if
> it is disabled by default and people will get the hint"
> has demonstrably failed.
>
> /
>     Leif
>
> 

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.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-19 13:04 [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Samer El-Haj-Mahmoud
                   ` (4 preceding siblings ...)
  2020-04-19 13:33 ` [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Ard Biesheuvel
@ 2020-04-20 13:24 ` Ard Biesheuvel
  2020-04-20 13:30   ` [edk2-devel] " Samer El-Haj-Mahmoud
  5 siblings, 1 reply; 25+ messages in thread
From: Ard Biesheuvel @ 2020-04-20 13:24 UTC (permalink / raw)
  To: Samer El-Haj-Mahmoud, devel; +Cc: Leif Lindholm, Pete Batard, Andrei Warkentin

On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
> Fix an ASSERT with the TFTP dynamic Shell command on the
> RPi3 and RPi4 when running DEBUG builds. Also, enable the
> command by default for all builds.
> 
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Pete Batard <pete@akeo.ie>
> Cc: Andrei Warkentin <awarkentin@vmware.com>
> 
> Samer El-Haj-Mahmoud (4):
>    Platform/RaspberryPi/RPi3: Fix TFTP dynamic command initialization
>    Platform/RaspberryPi/RPi4: Fix TFTP dynamic command initialization

Patches #1 and #2

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

Pushed as 944af47a8c83..f09ea1a6227f


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-20 13:24 ` Ard Biesheuvel
@ 2020-04-20 13:30   ` Samer El-Haj-Mahmoud
  0 siblings, 0 replies; 25+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-04-20 13:30 UTC (permalink / raw)
  To: devel@edk2.groups.io, Ard Biesheuvel, Samer El-Haj-Mahmoud
  Cc: Leif Lindholm, Pete Batard,
	Andrei Warkentin (awarkentin@vmware.com)

Thanks Ard!

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel via groups.io
> Sent: Monday, April 20, 2020 9:25 AM
> To: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>;
> devel@edk2.groups.io
> Cc: Leif Lindholm <leif@nuviainc.com>; Pete Batard <pete@akeo.ie>; Andrei
> Warkentin (awarkentin@vmware.com) <awarkentin@vmware.com>
> Subject: Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi
> : Enable TFTP shell command
>
> On 4/19/20 3:04 PM, Samer El-Haj-Mahmoud wrote:
> > Fix an ASSERT with the TFTP dynamic Shell command on the
> > RPi3 and RPi4 when running DEBUG builds. Also, enable the command by
> > default for all builds.
> >
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > Cc: Pete Batard <pete@akeo.ie>
> > Cc: Andrei Warkentin <awarkentin@vmware.com>
> >
> > Samer El-Haj-Mahmoud (4):
> >    Platform/RaspberryPi/RPi3: Fix TFTP dynamic command initialization
> >    Platform/RaspberryPi/RPi4: Fix TFTP dynamic command initialization
>
> Patches #1 and #2
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>
> Pushed as 944af47a8c83..f09ea1a6227f
>
>
> 

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.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command
  2020-04-20 12:43                         ` Ard Biesheuvel
@ 2020-04-20 14:18                           ` Pete Batard
  0 siblings, 0 replies; 25+ messages in thread
From: Pete Batard @ 2020-04-20 14:18 UTC (permalink / raw)
  To: Ard Biesheuvel, awarkentin, Andrew Fish, devel@edk2.groups.io
  Cc: Samer El-Haj-Mahmoud, Leif Lindholm

On 2020.04.20 13:43, Ard Biesheuvel wrote:
> On 4/20/20 1:55 PM, Pete Batard wrote:
>> On 2020.04.20 07:45, Ard Biesheuvel wrote:
> ...
>>> I rejected ACPI 6.3 table upgrades because, in their current form, 
>>> the only thing they achieve is losing the ability to boot an OS that 
>>> predates ACPI 6.3. Every piece of the platform currently being 
>>> described via ACPI can be described using 5.1 tables.
>>
>> Actually, that is not technically correct.
>>
>> But I will concede that you probably aren't aware of this, because we 
>> didn't bring up this point when we discussed the matter earlier.
>>
>> One thing you may want to know is that most of the binary blobs we 
>> picked up from the Microsoft IoT endeavour were ACPI 6.0, and, 
>> especially, the MADT we got had actually been abusing the GICR Base 
>> Address field, which was introduced in 6.0, to place then  
>> "nonsensical" value of 1 in there.
>>
>> And for reasons that only Microsoft knows, unless you do have a value 
>> of 1 there, Windows 10 on the Pi 3 will not boot at all. In other 
>> words, if you are to use ACPI 6.x with carry-over defaults from 5.1, 
>> Windows 10 can simply not boot, at least on the Pi 3.
>>
>> Now, what happened is that, when we started introducing some 5.1 
>> tables elsewhere, I went with trying to harmonize everything to 5.1 
>> and effectively *downgraded* the 6.0 tables we got from Microsoft to 
>> 5.1 (which nobody seemed to mind then, on account, I will also assert, 
>> that nobody realized that we actually had 6.0 tables where 6.0 
>> features might be relevant).
>>
>> It turns out that, if you do use 5.1, Windows 10 *seems* to be happy 
>> about the MADT, even as it is obviously missing a GICR Base Address 
>> field with a 1 value, and *appears* to work as expected.
>>
>> However, because of the proprietary nature of the OS, we have 
>> absolutely no idea whether having a "properly manipulated" GICR Base 
>> Address for Windows is important or not. The obvious reasoning is 
>> that, if Microsoft abused that field in their tables and especially 
>> if, when using ACPI 6.x, Windows 10 doesn't boot unless it sees the 
>> expected value there, an ACPI 6.x MADT with the relevant value 
>> populated might be something that we actually want to see.
>>
> 
> I agree that it makes sense to harmonize the table versions, and avoid 
> mixing and matching different versions of the spec. The structures are 
> usually defined in a way where new fields added at then end, and the 
> sizes are explicitly defined, so that an older OS can consume newer 
> versions of the tables.
> 
> At least, that is the theory ...
> 
>>> ACPI 6.3 tables are not 'better' or 'more current' than 5.1 ones.
>>
>> The example above begs to differ (though of course, short of actually 
>> knowing why the heck Microsoft decided to abuse GICR Base Address, 
>> it's hard to know what 'better' means in this case).
>>
>> It does seem to me like Microsoft were effectively using the most 
>> recent ACPI version they could use when they produced their IoT 
>> tables. And we certainly did happen to downgrade some of that, and may 
>> have gotten lucky that it all seemed to work (with the point being 
>> that, if we seem to be happy that downgrading ACPI tables from 6.0 to 
>> 5.1 and validating that we aren't observing obvious ill effects is 
>> okay, then upgrading ACPI tables to 6.3 and validating that we aren't 
>> observing ill effects should be fine too).
>>
> 
> If compatibility with Microsoft Windows 10 requires a certain minimum 
> version of the tables, then I don't mind adhering to that.
> 
>>> ACPI 6.3 tables should be used if/when they are needed, and not before.
>>
>> I disagree here again, on account of trying to showcase elements where 
>> we can, and this is one place we can do this.
>>
>> I see absolutely zero technical reasons to want to stick with ACPI 5.1 
>> especially as we already have one 6.3 table (PPTT) and we're going to 
>> have to upgrade a couple others to at least 6.0 anyway to get back to 
>> the position we were in with the Microsoft's blobs, so that we can be 
>> more confident about Windows' behaviour.
>>
>> As a matter of fact, using ACPI 6.3 before it is effectively needed is 
>> exactly the kind of move I see as wanting to apply when we know that a 
>> platform is going to be used as a de facto showcase. It may seem like 
>> a simple "newer is always better!" push to you, but the way I see it 
>> is that, by doing so, we are going to help developers of new 
>> platforms, who will be looking at the Pi for reference (whether we 
>> like it or not) and, if they are developing for modern hardware, are 
>> going to be looking at using new features that introduced only in 
>> recent ACPI. Thus, even if we don't make any use of these features on 
>> the Pi's, those developers might be grateful to find that there exists 
>> a working example of using a relatively up to date ACPI, and that they 
>> won't have to reinvent the wheel.
>>
>> Especially, as opposed to what might be the case for other platforms, 
>> we don't have a baggage of "older" OSes that we may hinder support for 
>> if we do move to ACPI 6.3 (especially as, outside of Windows, most Pi 
>> 3 OSes will be using DT rather than ACPI, and, without SD and Genet 
>> support, the idea of older OSes needing to work with Pi 4 is a bit 
>> ludicrous).
>>
>> So, whereas I agree that your reasoning is generally sound for most 
>> platforms, the fact that the Pi is going to be used as a de-facto 
>> showcase or starting point by folks interested in developing a new ARM 
>> platform, makes, I will assert, the situation a bit different here, 
>> and I would really appreciate if you could give some more thoughts to 
>> the points I am trying to make. In this case, I believe that the Pi 
>> should be the "exception that proves the rule".
>>
>> Furthermore, and this IMO is the most important point, we are not 
>> going to be sitting idle if we do upgrade to 6.3 and someone comes to 
>> us with a report that using modern ACPI appears to be causing them 
>> trouble (which, I will also assert, may actually be something that 
>> might be of great interest to folks on this list if that happens). As 
>> opposed to proprietary platforms, where ACPI is pretty much set in 
>> stone by the vendor and where addressing issues is a struggle, and, 
>> even if the vendor is very reactive and produces an update, where 
>> flashing a new firmware can be a tricky operation for some users, we 
>> are Open Source, easy to reach for reports, and the firmware "update" 
>> process of the Pi could not be any simpler (copy a file to an SD card 
>> and you're done). So I'd really like to help squelch the idea that if 
>> we do decide to upgrade to ACPI 6.3, and we effectively find out that 
>> it causes issues, we're simply going to stand by and let these 
>> unaddressed, whilst telling users of the platform "too bad". None of 
>> what we are doing is ever meant to be static or immutable, even after 
>> a goal has been reached. Thus, if there are issues with upgrading to 
>> 6.3, we're going to make darn sure that we address them, in the same 
>> way we have been planning to address issues that might be raised from 
>> some of our forced downgrades from 6.0 to 5.1.
>>
> 
> Thanks Pete.
> 
> I still don't agree with the whole showcase aspect of this discussion, 
> but since there are apparently other good reasons to upgrade these 
> tables that nobody brought up before, I am not going to oppose any 
> changes on this front,

I appreciate that, thanks.

> provided that the commit logs articulate in 
> sufficient detail why we think this is necessary (so that people looking 
> at this port for guidance have the background as well)

Okay. I think I can do that.

> Out of curiosity, why did we need a 6.3 PPTT for RPi4?

Well, PPTT was introduced in 6.2, and I am going to speculate that since 
6.3 introduced a bit more Processor Structure Flags and other elements 
(haven't checked the differences in detail), we probably went for 6.3 so 
that we could easily toggle those if needed, without having to go 
through a version upgrade. In other words, since we were introducing a 
recent ACPI feature from scratch, I surmise that the idea was that we 
might as well go all the way in order to potentially spare us some work 
later. But you'd have to ask Jeremy for the actual details.

And since we're talking about this, I just want to make clear that, once 
we update to 6.3, I don't have plans to update our tables to 6.4 or 
later unless we explicitly need those features, as the objective here is 
to use the most recent version of ACPI that exists at the time where a 
new platform (that we somewhat want to use as a showcase) is being 
introduced and nothing more.

Regards,

/Pete

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2020-04-20 14:18 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-19 13:04 [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Samer El-Haj-Mahmoud
2020-04-19 13:04 ` [edk2-platform][PATCH v1 1/4] Platform/RaspberryPi/RPi3: Fix TFTP dynamic command initialization Samer El-Haj-Mahmoud
2020-04-19 13:04 ` [edk2-platform][PATCH v1 2/4] Platform/RaspberryPi/RPi4: " Samer El-Haj-Mahmoud
2020-04-19 13:04 ` [edk2-platform][PATCH v1 3/4] Platform/RaspberryPi/RPi3: Enable TFTP command by default Samer El-Haj-Mahmoud
2020-04-19 13:04 ` [edk2-platform][PATCH v1 4/4] Platform/RaspberryPi/RPi4: " Samer El-Haj-Mahmoud
2020-04-19 13:33 ` [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command Ard Biesheuvel
2020-04-19 14:00   ` [edk2-devel] " Samer El-Haj-Mahmoud
2020-04-19 14:04     ` Ard Biesheuvel
2020-04-19 14:19   ` Pete Batard
2020-04-19 19:56     ` Andrei Warkentin
2020-04-19 20:06       ` [edk2-devel] " Pete Batard
2020-04-19 20:21         ` Andrei Warkentin
2020-04-19 20:24           ` Pete Batard
2020-04-19 20:42             ` Andrei Warkentin
     [not found]             ` <160753416257B0CA.29096@groups.io>
2020-04-19 23:50               ` Andrei Warkentin
2020-04-20  1:03                 ` Andrew Fish
2020-04-20  2:41                   ` Andrei Warkentin
2020-04-20  6:45                     ` Ard Biesheuvel
2020-04-20 11:55                       ` Pete Batard
2020-04-20 12:43                         ` Ard Biesheuvel
2020-04-20 14:18                           ` Pete Batard
2020-04-20 11:23       ` Leif Lindholm
2020-04-20 13:17         ` [edk2-devel] " Samer El-Haj-Mahmoud
2020-04-20 13:24 ` Ard Biesheuvel
2020-04-20 13:30   ` [edk2-devel] " Samer El-Haj-Mahmoud

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox