Re: [edk2-devel] [edk2-platform][PATCH v1 0/4] Platform/RaspberryPi : Enable TFTP shell command

["Pete Batard" <pete@akeo.ie>]

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