alexxy has quit [Quit: No Ping reply in 180 seconds.]
alexxy has joined #u-boot
naoki has joined #u-boot
monstr has joined #u-boot
monstr has quit [Remote host closed the connection]
monstr has joined #u-boot
<xypron>
sjg1: That patch won't be merged. Please, read take the review comments into account.
persmule has quit [Remote host closed the connection]
<apalos>
sjg1: that patch breaks the EFI spec though, only works for the first EFI app and makes memory permissions a no go,
<apalos>
As Heinrich said we spent a lot of time replying with the problems we see,
ikarso has joined #u-boot
<apalos>
and apart from all the above the TCG layer is allocating EFI_ACPI_MEMORY_NVS. So that patch is more of a half baked bandaid, rather than a fix
jaganteki has joined #u-boot
ldevulder has joined #u-boot
<mkorpershoek>
marex: that is worth exploring !
mckoan|away is now known as mckoan
eballetbo has joined #u-boot
alexxy has quit [Quit: No Ping reply in 180 seconds.]
alexxy has joined #u-boot
enok has joined #u-boot
alexxy has quit [Client Quit]
alperak has joined #u-boot
alexxy has joined #u-boot
prabhakalad has quit [Quit: Konversation terminated!]
prabhakalad has joined #u-boot
alexxy has quit [Quit: No Ping reply in 180 seconds.]
enok71 has joined #u-boot
enok has quit [Ping timeout: 255 seconds]
enok71 has quit [Ping timeout: 260 seconds]
alexxy has joined #u-boot
diederik has joined #u-boot
alexxy has quit [Quit: No Ping reply in 180 seconds.]
<diederik>
The name of the git repo changed from arm-trusted-firmware.git to trusted-firmware-a.git, but the instructions to `cd` into that dir weren't updated
<diederik>
f.e. in board/rockchip/evb_rk3399/README line 42 should be updated and in doc/README.rockchip line 77. There may be others
alexxy has quit [Quit: No Ping reply in 180 seconds.]
___nick___ has joined #u-boot
enok has joined #u-boot
sszy has joined #u-boot
prabhakalad has quit [Quit: Konversation terminated!]
alexxy has joined #u-boot
enok has quit [Ping timeout: 252 seconds]
alexxy has quit [Quit: No Ping reply in 180 seconds.]
enok has joined #u-boot
alexxy has joined #u-boot
alexxy has quit [Client Quit]
alexxy has joined #u-boot
alexxy has quit [Quit: No Ping reply in 180 seconds.]
alexxy has quit [Quit: No Ping reply in 180 seconds.]
<sjg1>
apalos: No, it does not break the spec or affect memory permissions. The bug it fixes only happens before the first app is started. We should have applied the patch ages ago and moved on IMO. Should we meet to discuss it?
<sjg1>
apalos: I have a deep understanding of how U-Boot works, gained over many years with many platforms. I have clearly and succinctly explained the root cause of all of this confusion and effort. I have a small patch which fixes *everything*, without introducing additional complexity with lmb, etc. It doesn't cause any problems for EFI and allows us to move on and respect the memory-management approach U-Boot requires
<sjg1>
apalos: So please, have another look at it, and let's get together to figure this out
swiftgeek has joined #u-boot
mmu_man has joined #u-boot
alexxy has joined #u-boot
<apalos>
sjg1: how long are you going to ignore the fact that the spec says "efi allocate pool allocates *pages*"?
<apalos>
I've had a very close too look to the patch
<apalos>
both me an ?Heirich have been repeating the same objections over 4-5 versions
<apalos>
and it's "fixes everything until an EFI app returns"
<apalos>
Also magic values because "oh it randomly crashes"
<apalos>
Sorry, that's far from my preference of a patch that should be applied
<apalos>
I can have a call though if you think it helps
<apalos>
and the "lmb complexity" is needed, several people responded you on that
<apalos>
So maybe for once, all the other people are right and you aren't
<sjg1>
apalos: The ful quote from the spec is: "This function allocates pages from EfiConventionalMemory as needed to grow the requested pool type". My patch does not change that
<apalos>
it says allocates PAGES because you need to fix memory permissions
<sjg1>
apalos: The project *only exists* before the EFI app starts. After that there is nothing to fix
<apalos>
I wonder how are you going to do that with page fragments malloc does
<sjg1>
apalos: The extra 100 bytes on each allocation is due to bugs in the code, not my patch. We should fix those anyway
<sjg1>
apalos: The EFI spec *does not* allow each pool allocation to have its only memory permissions\
<sjg1>
*own
<apalos>
not pool, pool *page*
<apalos>
u-boot is innefficent, becdause it allocates 4kb each time
<apalos>
The right way to do it allocate concecutive pages and keep one pool
<apalos>
But even the inneficient implementation is ok per spec
<apalos>
allocating 100kb isnt
<apalos>
sorry 100b*
<sjg1>
But we should mark the malloc() region as non-executable
<xypron>
sjg1: U-Boot is still used after an EFI application returns. Let us stop it here your patch is NAKed.
<apalos>
there are other memory attributes as well, not only permissions
<sjg1>
Sure, but if we didn't have the 4KB allocation, the bugs wouldn't have crept in
warpme has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]
<sjg1>
xypron: That isn't going to work, we need to sort this out and figure out a path forward
<sjg1>
apalos: So yes, I think a meeting is the best way to try to make progress
<apalos>
sjg1: EFI_MEMORY_NV, EFI_MEMORY_SP etc
<apalos>
sjg1: we have fiogured this out, LMB works just fine and it was just tested
<apalos>
and it also fixes all the lmb crud Tartarus wanted
<apalos>
sjg1: also I asked you to sent me a reproducer, because I cant reproduce that problem locally
<apalos>
because your exlanation is "oh out of the 8 different memory types, I am going to pikc the one it affects my current problem, and move it to a different area, because its easy"
<apalos>
so *if* it is a problem, allocating for a different memory type, will cause the *same* problem
<apalos>
which LMB fixes universally
warpme has joined #u-boot
alexxy has quit [Ping timeout: 264 seconds]
<sjg1>
apalos: With my patch we never needed the lmb stuff and certainly don't need a follow-on. At this point I believe I understand your point of view. My view is that LMB is for images, not small memory allocations and that it is just wrong to conflate the two. I believe we are running into the limits of email communication, so let's meet and try to discuss it
alexxy has joined #u-boot
<apalos>
also how are you going to fit taht memory in the mwhy is lmb for images?
<apalos>
then why does it protect u-boot?
<apalos>
sorry half typed
<sjg1>
apalos: Can you say that again?
<apalos>
yea I had a half typed sented
<apalos>
why is lmb for images? We also protect u-boot code so it wont get overwritten
<apalos>
its a generic reservation interface
<sjg1>
apalos: yes lmb is for reserving memory for images. It makes sure to exclude the entire U-Boot region (at the top of memory), so that images don't overwrite anything important
<sjg1>
Or perhaps not missed, but not given enough emphasis in the EFI world
<apalos>
You cant send commits to manipulate READMEs as you see fit and then say "oh look, it in the rst"
<apalos>
Because the counter argument, is that the efi.rst says it adheres to the spec,
<apalos>
And you are trying to break it
<apalos>
So that leads nowhere
mmu_man has quit [Ping timeout: 272 seconds]
<apalos>
anyway, I dont have time for this, I've spent more than enough time to explain in the ML
<apalos>
If you want a call, please schedule one
<sjg1>
apalos: See the README, line 2519, dating from 2004. I have clearly not changed anything there. I have no intention of breaking the EFI spec in any way. I am just trying to help correct some misunderstandings now, before they become endemic
<sjg1>
apalos: OK, thank, sent
Lightsword has quit [Ping timeout: 252 seconds]
Lightsword has joined #u-boot
enok has quit [Ping timeout: 276 seconds]
mmu_man has joined #u-boot
slobodan has joined #u-boot
<apalos>
sjg1: also something you never responded on IRC
<apalos>
You keep saying "when the EFI app starts"
<apalos>
But the EFI TCG code for measured boot
<apalos>
Allocates 64k *not* in the pages you want to move in malloc
<apalos>
And that's way bigger than what you are trying to move
<Tartarus>
sjg1: LMB is *not* for just images.
<Tartarus>
And to a large extent, what Wolfgang wrote 20+ years ago is not the most relevant to how we do things *today*
<apalos>
So even your initial assumption of "when the app starts" it completely flawed
<apalos>
is *
<Tartarus>
Should we take the spirit of it in to account? Sure.
<apalos>
sjg1: ret = efi_allocate_pool(EFI_ACPI_MEMORY_NVS, TPM2_EVENT_LOG_SIZE, &event_log.final_buffer);
<Tartarus>
But it's not The Rule. And we've clearly changed usage in the interim and the doc needs to reflect that too, because we didn't just change things randomly and for fun.
<apalos>
so the whole lets' move "EFI_BOOT_SERVICES_DATA" is flawed end to end
<apalos>
I am pretty sure the moment you fix that you'll say "oh oh let's move that as well"
<apalos>
So, no
<Tartarus>
LMB is for reserving areas for whatever reason, so that they can't (setting aside security issues) be overwritten randomly
<pivi>
Trying a not-stricly U-Boot question, more a generic firmware one, in case someone know and is willing to answer.
<apalos>
measured boot and potentially rng if you dont have another hardware
<pivi>
so, if you are implementing secure boot in a different way, and you have a different RNG, there is no need to start the TPM early in the FW. Correct? (just re-phrasing, what you wrote, I know ;-)
<apalos>
so secure boot and measured boot are completely different things
<apalos>
the first one cryptographically authenticated payloads, the latter measures firmware images, OS images etc
<apalos>
for example if someone unsolder your flash with the public keys and solders his own, you can happily boot
<apalos>
But if you *measure* your known keys, you expect to end up with specific sha256/whatever values
<apalos>
and you know something is wrong
<pivi>
apalos: yes, I know the differences. Unfortunate use of words on my side.
enok has joined #u-boot
<pivi>
anyway, clear, thanks. I was pretty confident about it, I was just looking for the final confirmation, thanks for it.
<sjg1>
Tartarus: Sure. I didn't say *just* for images, but that is why it was invented
<Tartarus>
Yes, it was initially and only somewhat successfully used to try and keep the kernel from overwriting the initrd / device tree. But that's also part of history that's not particularly relevant for the past few years now (pre Sughosh's series).
<sjg1>
Yes, if I am reading it right, it was really designed to handle the things that U-Boot's image-handling needs to allocate, like kernel decompression and FDT relocation. With FIT it works pretty well, but with scripts doing all sorts of different loads, it relies on hard-coded addresses for each board (kernel_addr_r etc.), which we have, but are trying to move away from
<Tartarus>
I mean, in hind sight we should have just set some reasonable defaults because the problems are less "where do we put it" but "where can we move things to?" and that's where things go wrong
<Tartarus>
But that can be set aside because the issue is we need a method to say "something is using this area of memory" so that at least in the context of well behaved commands you can't just go and hijack the running system by bringing in a malicious payload
eballetbo has quit [Quit: Connection closed for inactivity]
rvalue has joined #u-boot
vagrantc has joined #u-boot
<sjg1>
Tartarus: Yes the distro-scripts stuff would have been a good time to sort it out. At least lmb could stop it overwriting U-Boot memory and answered 'where can I move things to'. It was really designed for FIT, I believe, which is why it didn't need to be persistent. Perhaps that point was lost sometime in the last 16 years?
<Tartarus>
imho the mistake was when we added the magic for "if _high is 0xffffffff do not move"
<Tartarus>
That should have instead been "set the default window to the Linux lowmem default value"
slobodan_ is now known as slobodan
ldevulder has quit [Quit: Leaving]
<sjg1>
Oh yes, that was odd
<sjg1>
Tartarus: BTW did anything happen with the website?
uzix is now known as mahk
slobodan has quit [Changing host]
slobodan has joined #u-boot
goliath has joined #u-boot
<Tartarus>
No, I'm not sure where that person went off to.
<apalos>
sjg1: there's literally 0 chance to go in a bloblist,
<apalos>
it's not a list to begin with
<apalos>
and you wont keep putting away random stuff in random places because "oh it needs to fit"
rvalue has quit [Read error: Connection reset by peer]
mithro has quit [Ping timeout: 248 seconds]
mithro has joined #u-boot
<apalos>
that patchset is basically a learning process, everytime we point to something that doesnt work, you come up with an extra bandaid
<apalos>
So the proposal is "Instead of accepting the fact that EFI needs 100KB in the top of the stack, let's start stashing away random memory allocation to random places"
smurray has quit [Ping timeout: 248 seconds]
smurray has joined #u-boot
rvalue has joined #u-boot
<apalos>
and since you mentioned that, lets discuss it. You said
<apalos>
sjg1 | apalos: The ful quote from the spec is: "This function allocates pages from EfiConventionalMemory as needed to grow the requested pool type". My patch does not change that
<apalos>
first of all you patchs *does* change* that, for efi conventional memory
<apalos>
second, 'to grow as needed' obviously assumed you initially allocated a page and you depleted it,
<apalos>
unless you assume you start with random 200bytes and then 'you grow as needed', which obviously makes nosense whatsoever
<sjg1>
apalos: What do you mean by 0 chance to go in a bloblist? That's what bloblists are for. Why would we put tables anywhere else?
alan_o has quit [Remote host closed the connection]
alan_o has joined #u-boot
<sjg1>
Yes my patch was a learning process. In fact, as I said, for a long time I had an inkling that something was wrong, but had not investigated. I ran into problem trying to get Ubuntu booting on a Chromebook, and looked at what was wrong
<sjg1>
1. But how does my patch change that? The malloc() region is in EfiConventionalMemory, isn't it? The definition is "Memory available for general use."
<sjg1>
2. I don't really understand this part. I don't see a problem with using malloc(). If you really don't like that, I am OK with creating a separate region for it, but it needs to be within the U-Boot memory-area. But malloc() seems like the obvious choice, to me
<apalos>
the efi eventlog is *not* a table
<apalos>
its a logile, with a specific format
<apalos>
You patches does not "allocate pages to grow as needed" and I just explained, you should start with allocating pages to begin with
<apalos>
which malloc does not do
<apalos>
So if you end up creating a separate region, how is that different from LMB?
<apalos>
let alone that the different region is not clean enough again, since you end up allocating different types from different areas etc and you have size limitations
alperak has quit [Quit: Connection closed for inactivity]
naoki has joined #u-boot
<sjg1>
ACPI is a table too, right? I don't mind if you want to call the eventlog a logfile, but it is still a blob which we can add to a list
<sjg1>
I just don't understand this hang-up with malloc() and trying to micro-parse one sentence of the spec. My patch meets all the spec requirements. What do you mean by areas?
<sjg1>
I think it would be really helpful to just start again, imagine that my patch has been applied, and think about what problems it actually causes
diederik has left #u-boot [Going to see what happens IRL, see ya!]
Stat_headcrabbed has quit [Remote host closed the connection]
<apalos>
I am microparsing a sentence of the spec because after 1 month of explaining you still cant understand why you break the spec
<apalos>
also i dont care what the patch causes, thats how not I review patches
<apalos>
I care about it being half baked, not covering all the cases of the problem you are trying to solve and having random numbers in there which "we will fix"
<apalos>
apart from the architectural part which I just dont like
___nick___ has quit [Ping timeout: 246 seconds]
<sjg1>
It isn't because I don't understand, it's actually because I disagree. It is a genuine disagreement, in good faith, with the best of intentions
slobodan has quit [Read error: Connection reset by peer]
slobodan has joined #u-boot
haritz has quit [Ping timeout: 276 seconds]
ex--parrot is now known as ex-parrot
fatal has quit [Remote host closed the connection]
<Tartarus>
sjg1: and apalos and xypron keep saying that no, you are breaking the spec, and you aren't explaining how the examples they provide are wrong?
goliath has quit [Quit: SIGSEGV]
<sjg1>
OK thanks
<sjg1>
It's not for want of trying. I just cannot seem to get on the same page. I'm happy to change the patch, but I haven't yet figured out what is needed. I believe the lmb confusion has contributed a lot to this
pgreco has quit [Ping timeout: 246 seconds]
pgreco has joined #u-boot
<Tartarus>
Yes, but I think you're stuck on what lmb is for, incorrectly
slobodan has quit [Ping timeout: 252 seconds]
ikarso has quit [Quit: Connection closed for inactivity]
<sjg1>
Perhaps. The complexity of a grand, unified allocator to avoid a 100-line patch doesn't fill me with joy. We need to find some common ground among all of this
<marex>
isnt LMB there to protect chunks of U-Boot memory from being overwritten ?
<marex>
Tartarus: is there any way to test the N: match in MAINTAINERS file ?