On 5/15/21 5:19 PM, Simon Glass wrote:
Hi Heinrich,
On Fri, 14 May 2021 at 18:34, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
Am 14. Mai 2021 22:44:32 MESZ schrieb Simon Glass <s...@chromium.org>:
Hi Heinrich,
On Fri, 14 May 2021 at 00:11, Heinrich Schuchardt <xypron.g...@gmx.de>
wrote:
On 5/14/21 1:56 AM, Simon Glass wrote:
Hi Heinrich,
On Thu, 13 May 2021 at 08:53, Heinrich Schuchardt
<xypron.g...@gmx.de> wrote:
On 5/13/21 4:36 PM, Simon Glass wrote:
Hi Heinrich,
On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt
<xypron.g...@gmx.de> wrote:
Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass
<s...@chromium.org>:
Hi Heinrich,
On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt
<xypron.g...@gmx.de>
wrote:
Addresses in state->ram_buf must be in the low 4 GiB of the
address
space.
Otherwise we cannot correctly fill SMBIOS tables. This shows
up in
warnings
like:
WARNING: SMBIOS table_address overflow 7f752735e020
This sounds like a bug in the smbios-table code. For sandbox it
should
perhaps use addresses instead of pointers.
I think that code (that I unfortunately wrote) was an
expeditious way
of getting it running, but is not correct.
The field you are filling is only 32bit wide. I wonder how that
table is meant to work on systems where the lowest memory address is
above 4 GiB. Such ARMv8 systems exist.
map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS
is
legacy and designed for 4GB.
I know map_to_sysmem(). But you wrote in lib/smbios.c:487:
/*
* We must use a pointer here so things work correctly on
sandbox. The
* user of this table is not aware of the mapping of addresses
to
* sandbox's DRAM buffer.
*/
For testing you could start a binary with command 'bootefi' or
'booti'
and that binary would analyze the table. So yes, your comment
holds true
and you must not use map_to_sysmem() here.
Yes, that is a good point. Perhaps I was not mad when I wrote that.
What is using the tables on sandbox?
The UEFI shell has a command smbiosview to display the SMBIOS table.
With the current U-Boot sandbox it crashes. I do not know what the
cause is:
FS0:\> smbiosview
Segmentation violation
pc = 0x7fb0df88d17e, pc_reloc = 0x7fb0cf88d17e
UEFI image [0x00007fb0df836000:0x00007fb0df920c5f] pc=0x5717e
'/Shell_x64.efi'
Here is some of the output for my laptop:
SMBIOS Entry Point Structure:
Anchor String: _SM_
EPS Checksum: 0xEE
Entry Point Len: 31
Version: 3.1
Number of Structures: 62
Max Struct size: 145
Table Address: 0x986E9000
Table Length: 2316
Entry Point revision: 0x0
SMBIOS BCD Revision: 0x31
Inter Anchor: _DMI_
Inter Checksum: 0x4E
Formatted Area:
00000000: 00 00 00 00 00 *.....*
=========================================================
Query Structure, conditions are:
QueryType = Random
QueryHandle = Random
ShowType = SHOW_DETAIL
Enter to continue, :q to exit, :[0-3] to change mode, /? for help
$
=========================================================
Type=18, Handle=0x0
Dump Structure as:
Index=0,Length=0x19,Addr=0x986E9019
00000000: 12 17 00 00 03 02 02 00-00 00 00 00 00 00 80 00
*................*
00000010: 00 00 80 00 00 00 80 00-00
*.........*
Enter to continue, :q to exit, :[0-3] to change mode, /? for help
$Structure Type: 32-bit Memory Error Information
Format part Len : 23
Structure Handle: 0
32-bit Memory Error Information - Type: OK
Memory Error - Error granularity: Unknown
Memory Error - Error Operation: Unknown
VendorSyndrome: 0x0
MemoryArrayErrorAddress: 0x80000000
DeviceErrorAddress: 0x80000000
ErrorResolution: 0x80000000
OK, I am not sure what to make of that except that it is every bit as
impenetrable as I would expect from UEFI.
This is a dump of the SMBIOS tables. Why do you refer to UEFI?
How do you fit so many questions in one email? :-)
I assumed smbiosview is a UEFI command because of the prompt. I have
never heard of the command.
Is this on a Risc-V host? I am not sure why the first alloc would get
the requested address and not the second. Is it because we specify
0x10000000 as the requested address? Is that what you actually get in
this case?
The observation is reproducible on x86_64.
Two mmap calls cannot receive the same address without calling munmap in
between.
The first call *might* return the address 0x10000000 that you ask for.
If the first call cannot receive it because it is not available, why should
subsequent calls succeed?
The goal is to keep the addresses small since 64-bit addresses are a
pain when debugging. I had hoped that mmap() would try to find an
address near the one requested, for subsequent calls, but perhaps that
hope is forlorn. It certainly doesn't seem to do that on x86.
If we drop that value on subsequent calls to mmap(), does it fix the
problem?
If you drop that value you get a random address. What would be better in this
case?
I just tried it on x86 and we get a 'random' address anyway, whatever
is passed, once it has used the requested address. I think the address
hint only works if it works. If not, then linux starts allocating from
another address. Perhaps this linux behaviour has changed in recent
years.
I think we should update the commit message to say we are hoping to
get 0x10000000 for the RAM buffer (although of course there is a 4K
region at the start of it) and that xx arch provides large addresses
for subsequent mmap()s.
What 4 KiB region are you referring to?
If you look at os_malloc() you will see that it adds a 4KB header
before each block it allocates.
Why do you expect any specific address in subsequent calls? The man-page states "If
another mapping already exists there, the kernel picks a new address that may or may not
depend on the hint."
For performance reasons the kernel should not care about the hint in this case.
OK.
Also, how about upgrading the smbios warning to an error. I think that
is what it is and it should stop execution.
For errors we use log_err(). But where is the error? mmap() comes with
absolutely no guarantee that it will assign the address that you are requesting
or anything close to it.
Smbios is not important enough to make the sandbox unusable in this case.
Well if the tables are invalid we should not pass them on...can we at
least do that?
That would be separate patch?
Best regards
Heinrich
Regards,
Simon
[..]
