[U-Boot] [PATCH 5/7] smbios: Expose in efi_loader as table

[Alexander Graf <agraf@suse.de>]

On 08/12/2016 10:07 PM, Simon Glass wrote:
> Hi Alex,
>
> On 12 August 2016 at 12:36, Alexander Graf <agraf@suse.de> wrote:
>>
>> On 12.08.16 19:20, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 8 August 2016 at 08:06, Alexander Graf <agraf@suse.de> wrote:
>>>> We can pass SMBIOS easily as EFI configuration table to an EFI payload. This
>>>> patch adds enablement for that case.
>>>>
>>>> While at it, we also enable SMBIOS generation for ARM systems, since they support
>>>> EFI_LOADER.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>>   cmd/bootefi.c        |  3 +++
>>>>   include/efi_api.h    |  4 ++++
>>>>   include/efi_loader.h |  2 ++
>>>>   include/smbios.h     |  1 +
>>>>   lib/Kconfig          |  4 ++--
>>>>   lib/smbios.c         | 36 ++++++++++++++++++++++++++++++++++++
>>>>   6 files changed, 48 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>> index 53a6ee3..e241b7d 100644
>>>> --- a/cmd/bootefi.c
>>>> +++ b/cmd/bootefi.c
>>>> @@ -205,6 +205,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt)
>>>>          if (!memcmp(bootefi_device_path[0].str, "N\0e\0t", 6))
>>>>                  loaded_image_info.device_handle = nethandle;
>>>>   #endif
>>>> +#ifdef CONFIG_GENERATE_SMBIOS_TABLE
>>>> +       efi_smbios_register();
>>>> +#endif
>>>>
>>>>          /* Initialize EFI runtime services */
>>>>          efi_reset_system_init();
>>>> diff --git a/include/efi_api.h b/include/efi_api.h
>>>> index f572b88..bdb600e 100644
>>>> --- a/include/efi_api.h
>>>> +++ b/include/efi_api.h
>>>> @@ -201,6 +201,10 @@ struct efi_runtime_services {
>>>>          EFI_GUID(0xb1b621d5, 0xf19c, 0x41a5, \
>>>>                   0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0)
>>>>
>>>> +#define SMBIOS_TABLE_GUID \
>>>> +       EFI_GUID(0xeb9d2d31, 0x2d88, 0x11d3,  \
>>>> +                0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
>>>> +
>>>>   struct efi_configuration_table
>>>>   {
>>>>          efi_guid_t guid;
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index ac8b77a..b0e8a7f 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -85,6 +85,8 @@ int efi_disk_register(void);
>>>>   int efi_gop_register(void);
>>>>   /* Called by bootefi to make the network interface available */
>>>>   int efi_net_register(void **handle);
>>>> +/* Called by bootefi to make SMBIOS tables available */
>>>> +void efi_smbios_register(void);
>>>>
>>>>   /* Called by networking code to memorize the dhcp ack package */
>>>>   void efi_net_set_dhcp_ack(void *pkt, int len);
>>>> diff --git a/include/smbios.h b/include/smbios.h
>>>> index 5962d4c..fb6396a 100644
>>>> --- a/include/smbios.h
>>>> +++ b/include/smbios.h
>>>> @@ -55,6 +55,7 @@ struct __packed smbios_entry {
>>>>   #define BIOS_CHARACTERISTICS_SELECTABLE_BOOT   (1 << 16)
>>>>
>>>>   #define BIOS_CHARACTERISTICS_EXT1_ACPI         (1 << 0)
>>>> +#define BIOS_CHARACTERISTICS_EXT1_UEFI         (1 << 3)
>>>>   #define BIOS_CHARACTERISTICS_EXT2_TARGET       (1 << 2)
>>>>
>>>>   struct __packed smbios_type0 {
>>>> diff --git a/lib/Kconfig b/lib/Kconfig
>>>> index 5a14530..d7f75fe 100644
>>>> --- a/lib/Kconfig
>>>> +++ b/lib/Kconfig
>>>> @@ -150,12 +150,12 @@ config SPL_OF_LIBFDT
>>>>            version of the device tree.
>>>>
>>>>   menu "System tables"
>>>> -       depends on !EFI && !SYS_COREBOOT
>>>> +       depends on (!EFI && !SYS_COREBOOT) || (ARM && EFI_LOADER)
>>>>
>>>>   config GENERATE_SMBIOS_TABLE
>>>>          bool "Generate an SMBIOS (System Management BIOS) table"
>>>>          default y
>>>> -       depends on X86
>>>> +       depends on X86 || EFI_LOADER
>>>>          help
>>>>            The System Management BIOS (SMBIOS) specification addresses how
>>>>            motherboard and system vendors present management information about
>>>> diff --git a/lib/smbios.c b/lib/smbios.c
>>>> index 8dfd486..b9aa741 100644
>>>> --- a/lib/smbios.c
>>>> +++ b/lib/smbios.c
>>>> @@ -7,10 +7,13 @@
>>>>    */
>>>>
>>>>   #include <common.h>
>>>> +#include <efi_loader.h>
>>>>   #include <smbios.h>
>>>>   #include <tables_csum.h>
>>>>   #include <version.h>
>>>> +#ifdef CONFIG_X86
>>>>   #include <asm/cpu.h>
>>>> +#endif
>>>>
>>>>   DECLARE_GLOBAL_DATA_PTR;
>>>>
>>>> @@ -79,14 +82,20 @@ static int smbios_write_type0(uintptr_t *current, int handle)
>>>>          t->vendor = smbios_add_string(t->eos, "U-Boot");
>>>>          t->bios_ver = smbios_add_string(t->eos, PLAIN_VERSION);
>>>>          t->bios_release_date = smbios_add_string(t->eos, U_BOOT_DMI_DATE);
>>>> +#ifdef CONFIG_ROM_SIZE
>>>>          t->bios_rom_size = (CONFIG_ROM_SIZE / 65536) - 1;
>>>> +#endif
>>>>          t->bios_characteristics = BIOS_CHARACTERISTICS_PCI_SUPPORTED |
>>>>                                    BIOS_CHARACTERISTICS_SELECTABLE_BOOT |
>>>>                                    BIOS_CHARACTERISTICS_UPGRADEABLE;
>>>>   #ifdef CONFIG_GENERATE_ACPI_TABLE
>>>>          t->bios_characteristics_ext1 = BIOS_CHARACTERISTICS_EXT1_ACPI;
>>>>   #endif
>>>> +#ifdef CONFIG_EFI_LOADER
>>>> +       t->bios_characteristics_ext1 |= BIOS_CHARACTERISTICS_EXT1_UEFI;
>>>> +#endif
>>>>          t->bios_characteristics_ext2 = BIOS_CHARACTERISTICS_EXT2_TARGET;
>>>> +
>>>>          t->bios_major_release = 0xff;
>>>>          t->bios_minor_release = 0xff;
>>>>          t->ec_major_release = 0xff;
>>>> @@ -152,6 +161,7 @@ static int smbios_write_type3(uintptr_t *current, int handle)
>>>>          return len;
>>>>   }
>>>>
>>>> +#ifdef CONFIG_X86
>>>>   static int smbios_write_type4(uintptr_t *current, int handle)
>>>>   {
>>>>          struct smbios_type4 *t = (struct smbios_type4 *)*current;
>>>> @@ -184,6 +194,7 @@ static int smbios_write_type4(uintptr_t *current, int handle)
>>>>
>>>>          return len;
>>>>   }
>>>> +#endif
>>>>
>>>>   static int smbios_write_type32(uintptr_t *current, int handle)
>>>>   {
>>>> @@ -216,7 +227,9 @@ static smbios_write_type smbios_write_funcs[] = {
>>>>          smbios_write_type1,
>>>>          smbios_write_type2,
>>>>          smbios_write_type3,
>>>> +#ifdef CONFIG_X86
>>> This should become a parameter I think. Since you are making this into
>>> generic code, it should avoid arch-specific #ifdefs.
>> The type4 table really contains x86 cpu specific information, so I
>> figured an #ifdef CONFIG_X86 makes the most sense here.
>>
>> Looking at
>>
>>
>> https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.0.0.pdf
>>
>> I guess you *could* put one in that addresses ARM as well. Looking at
>> example output from a ThunderX system, that seems to be what other
>> vendors do:
>>
>> Handle 0x0004, DMI type 4, 48 bytes
>> Processor Information
>>          Socket Designation: CPU 0
>>          Type: Central Processor
>>          Family: Other
>>          Manufacturer: www.cavium.com
>>          ID: 00 00 00 00 00 00 00 00
>>          Version: 2.0
>>          Voltage: 3.3 V
>>          External Clock: 156 MHz
>>          Max Speed: 2000 MHz
>>          Current Speed: 2000 MHz
>>          Status: Populated, Enabled
>>          Upgrade: Other
>>          L1 Cache Handle: 0x0080
>>          L2 Cache Handle: 0x0082
>>          L3 Cache Handle: 0x0007
>>          Serial Number: 1.0
>>          Asset Tag: 1.0
>>          Part Number: [...]
>>          Core Count: 48
>>          Core Enabled: 48
>>          Thread Count: 1
>>          Characteristics: None
>>
>> So what we really need is a rename for smbios_write_type4 to
>> smbios_write_type4_x86 which then is still surrounded by the #ifdef
>> CONFIG_X86. We could just simply add another one for arm if we want to ;).
>  From what I can see, the problem is the cpu_get_name() call. Is that
> right? If so, that could move to the CPU uclass and use cpu_get_info()
> (with the name added as a new field) or perhaps a new cpu_get_name()
> call.

I see what you're aiming for, but I just fail to see how we could 
properly abstract away CPU specific information - and I don't think it's 
terribly useful either. I've refactored the code to be slightly more 
pretty now, but I really don't think that we should depend on CONFIG_CPU 
or introduce new calls for every bit:

static void smbios_write_type4_arch(struct smbios_type4 *t)
{
         u16 processor_family = SMBIOS_PROCESSOR_FAMILY_UNKNOWN;
         const char *vendor = "Unknown";
         char *name = "Unknown";

#ifdef CONFIG_X86
         char processor_name[CPU_MAX_NAME_LEN];
         struct cpuid_result res;

         processor_family = gd->arch.x86;
         vendor = cpu_vendor_name(gd->arch.x86_vendor);
         res = cpuid(1);
         t->processor_id[0] = res.eax;
         t->processor_id[1] = res.edx;
         name = cpu_get_name(processor_name);
#elif defined(CONFIG_ARM)
         processor_family = SMBIOS_PROCESSOR_FAMILY_OTHER;
         vendor = "ARM";
#endif

         t->processor_family = processor_family;
         t->processor_manufacturer = smbios_add_string(t->eos, vendor);
         t->processor_version = smbios_add_string(t->eos, name);
}


Alex