[U-Boot] [PATCH v13 1/4] sandbox: smbios: Update to support sandbox

[Simon Glass]

Hi Alex,

On 14 November 2018 at 02:18, Alexander Graf <agraf at suse.de> wrote:
>
> On 11/14/2018 07:50 AM, Simon Glass wrote:
>>
>> At present this code casts addresses to pointers so cannot be used with
>> sandbox. Update it to use mapmem instead.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>> Changes in v13:
>> - Update code to deal with the struct_table_address member
>>
>> Changes in v12: None
>> Changes in v11:
>> - Fix the EFI code that has since been added and relies on broken behaviour
>>
>> Changes in v9: None
>> Changes in v7: None
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3:
>> - Drop incorrect map_sysmem() in write_smbios_table()
>>
>>   lib/efi_loader/efi_smbios.c | 20 +++++++++-----
>>   lib/smbios.c                | 52 ++++++++++++++++++++++++++++++-------
>>   2 files changed, 56 insertions(+), 16 deletions(-)
>>

[..]

>> +
>> +       /*
>> +        * 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.
>> +        */
>> +       table_addr = (ulong)map_sysmem(tables, 0);
>> +       if (sizeof(table_addr) >= sizeof(u32) && table_addr >= (1ULL << 32)) {
>
>
> sizeof(long) >= sizeof(u32) will always be true on any platform U-Boot supports, no?
>
> How about
>
>   if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX)

Yes that seems right, thanks.

>
>> +               /*
>> +                * We need to put this >32-bit pointer into the table but the
>> +                * field is only 32 bits wide.
>> +                */
>> +               printf("WARNING: SMBIOS table_address overflow %llx\n",
>> +                      (unsigned long long)table_addr);
>> +               table_addr = 0;
>
>
> I'm also not sure what to do about the error case. IMHO ideally we should propagate the error to the upper layers, but consider it non-fatal.

Yes agreed, but it involves changing the function signature of this
and everything else in table_write_funcs[] so I decided to leave it
alone for now, since the effect is the same.

Regards,
Simon