From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Sat, 15 May 2021 10:09:23 -0600 Subject: [PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory In-Reply-To: References: <20210511190316.29710-1-xypron.glpk@gmx.de> <7FCA8780-BB02-4E66-9F65-D10EB533A037@gmx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Heinrich, On Sat, 15 May 2021 at 09:33, Heinrich Schuchardt wrote: > > On 5/15/21 5:19 PM, Simon Glass wrote: > > Hi Heinrich, > > > > On Fri, 14 May 2021 at 18:34, Heinrich Schuchardt wrote: > >> > >> Am 14. Mai 2021 22:44:32 MESZ schrieb Simon Glass : > >>> Hi Heinrich, > >>> > >>> On Fri, 14 May 2021 at 00:11, Heinrich Schuchardt > >>> wrote: > >>>> > >>>> On 5/14/21 1:56 AM, Simon Glass wrote: > >>>>> Hi Heinrich, > >>>>> > >>>>> On Thu, 13 May 2021 at 08:53, Heinrich Schuchardt > >>> wrote: > >>>>>> > >>>>>> On 5/13/21 4:36 PM, Simon Glass wrote: > >>>>>>> Hi Heinrich, > >>>>>>> > >>>>>>> On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt > >>> wrote: > >>>>>>>> > >>>>>>>> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass > >>> : > >>>>>>>>> Hi Heinrich, > >>>>>>>>> > >>>>>>>>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt > >>> > >>>>>>>>> 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? Yes indeed. Regards, Simon