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

Hi Alex,

On 16 August 2016 at 02:38, Alexander Graf agraf@suse.de wrote:

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.p...

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

All you are getting here is the same and the processor ID, I think. It is not hard to do what you need in the cpu uclass.

Please don't introduce this sort of cruft in what is now generic code. You can make EFI_LOADER depend on CPU or you can put an #ifdef CONFIG_CPU here, but what you have is just going to create a mess as more things are added to it. Given that we have a defined CPU interface, it is also unnecessary. With a little more effort you can come to a nice solution.

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

Regards, Simon