Hi Alex,
On 16 August 2016 at 02:38, Alexander Graf <ag...@suse.de> wrote:
> On 08/12/2016 10:07 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 12 August 2016 at 12:36, Alexander Graf <ag...@suse.de> wrote:
>>>
>>>
>>> On 12.08.16 19:20, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 8 August 2016 at 08:06, Alexander Graf <ag...@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 <ag...@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
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
