| Message ID | 1410310325-4509-11-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
>>> On 10.09.14 at 02:52, <roy.franz@linaro.org> wrote: > @@ -805,49 +801,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > boot_cpu_data.x86_capability[1] = cpuid_ext_features; > } > > - /* Obtain basic table pointers. */ > - for ( i = 0; i < efi_num_ct; ++i ) > - { > - static EFI_GUID __initdata acpi2_guid = ACPI_20_TABLE_GUID; > - static EFI_GUID __initdata acpi_guid = ACPI_TABLE_GUID; > - static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID; > - static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID; > - > - if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) ) > - efi.acpi20 = (long)efi_ct[i].VendorTable; > - if ( match_guid(&acpi_guid, &efi_ct[i].VendorGuid) ) > - efi.acpi = (long)efi_ct[i].VendorTable; > - if ( match_guid(&mps_guid, &efi_ct[i].VendorGuid) ) > - efi.mps = (long)efi_ct[i].VendorTable; > - if ( match_guid(&smbios_guid, &efi_ct[i].VendorGuid) ) > - efi.smbios = (long)efi_ct[i].VendorTable; > - } The only arch specific things I can see throughout this entire patch are the specific GUIDs and which fields they go into in the internal structure. The GUIDs can be put in a table, and the fields can be represented e.g. via offsetof(), paired with the respective GUID. Jan
On Thu, Sep 11, 2014 at 7:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 10.09.14 at 02:52, <roy.franz@linaro.org> wrote: >> @@ -805,49 +801,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) >> boot_cpu_data.x86_capability[1] = cpuid_ext_features; >> } >> >> - /* Obtain basic table pointers. */ >> - for ( i = 0; i < efi_num_ct; ++i ) >> - { >> - static EFI_GUID __initdata acpi2_guid = ACPI_20_TABLE_GUID; >> - static EFI_GUID __initdata acpi_guid = ACPI_TABLE_GUID; >> - static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID; >> - static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID; >> - >> - if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) ) >> - efi.acpi20 = (long)efi_ct[i].VendorTable; >> - if ( match_guid(&acpi_guid, &efi_ct[i].VendorGuid) ) >> - efi.acpi = (long)efi_ct[i].VendorTable; >> - if ( match_guid(&mps_guid, &efi_ct[i].VendorGuid) ) >> - efi.mps = (long)efi_ct[i].VendorTable; >> - if ( match_guid(&smbios_guid, &efi_ct[i].VendorGuid) ) >> - efi.smbios = (long)efi_ct[i].VendorTable; >> - } > > The only arch specific things I can see throughout this entire patch > are the specific GUIDs and which fields they go into in the internal > structure. The GUIDs can be put in a table, and the fields can be > represented e.g. via offsetof(), paired with the respective GUID. > > Jan > The efi structure is defined in runtime.c for runtime use, which is why I moved it, as I am not addressing runtime services in this patchset. This is an (ab)use of the architecture specific head file, using it for features that are currently only implemented on that architecture, rather than fundamental architectural differences. Roy
On Thu, 11 Sep 2014, Roy Franz wrote: > On Thu, Sep 11, 2014 at 7:44 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>>> On 10.09.14 at 02:52, <roy.franz@linaro.org> wrote: > >> @@ -805,49 +801,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > >> boot_cpu_data.x86_capability[1] = cpuid_ext_features; > >> } > >> > >> - /* Obtain basic table pointers. */ > >> - for ( i = 0; i < efi_num_ct; ++i ) > >> - { > >> - static EFI_GUID __initdata acpi2_guid = ACPI_20_TABLE_GUID; > >> - static EFI_GUID __initdata acpi_guid = ACPI_TABLE_GUID; > >> - static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID; > >> - static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID; > >> - > >> - if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) ) > >> - efi.acpi20 = (long)efi_ct[i].VendorTable; > >> - if ( match_guid(&acpi_guid, &efi_ct[i].VendorGuid) ) > >> - efi.acpi = (long)efi_ct[i].VendorTable; > >> - if ( match_guid(&mps_guid, &efi_ct[i].VendorGuid) ) > >> - efi.mps = (long)efi_ct[i].VendorTable; > >> - if ( match_guid(&smbios_guid, &efi_ct[i].VendorGuid) ) > >> - efi.smbios = (long)efi_ct[i].VendorTable; > >> - } > > > > The only arch specific things I can see throughout this entire patch > > are the specific GUIDs and which fields they go into in the internal > > structure. The GUIDs can be put in a table, and the fields can be > > represented e.g. via offsetof(), paired with the respective GUID. > > > > Jan > > > The efi structure is defined in runtime.c for runtime use, which is > why I moved it, > as I am not addressing runtime services in this patchset. This is an (ab)use of > the architecture specific head file, using it for features that are > currently only implemented > on that architecture, rather than fundamental architectural differences. It might be better to add a couple of flags, such as support_runtime_services and support_smbios. They would be true on x86 and false on arm. The intention would be clearer that way.
>>> On 12.09.14 at 00:03, <roy.franz@linaro.org> wrote: > On Thu, Sep 11, 2014 at 7:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 10.09.14 at 02:52, <roy.franz@linaro.org> wrote: >>> @@ -805,49 +801,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE > *SystemTable) >>> boot_cpu_data.x86_capability[1] = cpuid_ext_features; >>> } >>> >>> - /* Obtain basic table pointers. */ >>> - for ( i = 0; i < efi_num_ct; ++i ) >>> - { >>> - static EFI_GUID __initdata acpi2_guid = ACPI_20_TABLE_GUID; >>> - static EFI_GUID __initdata acpi_guid = ACPI_TABLE_GUID; >>> - static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID; >>> - static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID; >>> - >>> - if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) ) >>> - efi.acpi20 = (long)efi_ct[i].VendorTable; >>> - if ( match_guid(&acpi_guid, &efi_ct[i].VendorGuid) ) >>> - efi.acpi = (long)efi_ct[i].VendorTable; >>> - if ( match_guid(&mps_guid, &efi_ct[i].VendorGuid) ) >>> - efi.mps = (long)efi_ct[i].VendorTable; >>> - if ( match_guid(&smbios_guid, &efi_ct[i].VendorGuid) ) >>> - efi.smbios = (long)efi_ct[i].VendorTable; >>> - } >> >> The only arch specific things I can see throughout this entire patch >> are the specific GUIDs and which fields they go into in the internal >> structure. The GUIDs can be put in a table, and the fields can be >> represented e.g. via offsetof(), paired with the respective GUID. >> > The efi structure is defined in runtime.c for runtime use, which is > why I moved it, > as I am not addressing runtime services in this patchset. This is an > (ab)use of > the architecture specific head file, using it for features that are > currently only implemented > on that architecture, rather than fundamental architectural differences. So how about you enable building runtime.c on ARM right away and for now simply #ifdef out everything but the variable definitions there? Jan
On Fri, Sep 12, 2014 at 12:14 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 12.09.14 at 00:03, <roy.franz@linaro.org> wrote: > > On Thu, Sep 11, 2014 at 7:44 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>>>> On 10.09.14 at 02:52, <roy.franz@linaro.org> wrote: > >>> @@ -805,49 +801,13 @@ efi_start(EFI_HANDLE ImageHandle, > EFI_SYSTEM_TABLE > > *SystemTable) > >>> boot_cpu_data.x86_capability[1] = cpuid_ext_features; > >>> } > >>> > >>> - /* Obtain basic table pointers. */ > >>> - for ( i = 0; i < efi_num_ct; ++i ) > >>> - { > >>> - static EFI_GUID __initdata acpi2_guid = ACPI_20_TABLE_GUID; > >>> - static EFI_GUID __initdata acpi_guid = ACPI_TABLE_GUID; > >>> - static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID; > >>> - static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID; > >>> - > >>> - if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) ) > >>> - efi.acpi20 = (long)efi_ct[i].VendorTable; > >>> - if ( match_guid(&acpi_guid, &efi_ct[i].VendorGuid) ) > >>> - efi.acpi = (long)efi_ct[i].VendorTable; > >>> - if ( match_guid(&mps_guid, &efi_ct[i].VendorGuid) ) > >>> - efi.mps = (long)efi_ct[i].VendorTable; > >>> - if ( match_guid(&smbios_guid, &efi_ct[i].VendorGuid) ) > >>> - efi.smbios = (long)efi_ct[i].VendorTable; > >>> - } > >> > >> The only arch specific things I can see throughout this entire patch > >> are the specific GUIDs and which fields they go into in the internal > >> structure. The GUIDs can be put in a table, and the fields can be > >> represented e.g. via offsetof(), paired with the respective GUID. > >> > > The efi structure is defined in runtime.c for runtime use, which is > > why I moved it, > > as I am not addressing runtime services in this patchset. This is an > > (ab)use of > > the architecture specific head file, using it for features that are > > currently only implemented > > on that architecture, rather than fundamental architectural differences. > > So how about you enable building runtime.c on ARM right away and > for now simply #ifdef out everything but the variable definitions there? > > Jan > > I think this will work pretty well. I have maybe been overly avoiding #ifdefs.. I'll move the runtime.c/compat.c files to common/efi as well. Roy
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 5296d88..013597b 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -55,6 +55,8 @@ static CHAR16 *__init s2w(union string *str); static char *__init w2s(const union string *str); static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, struct file *file, char *name_options); +static bool_t __init __maybe_unused match_guid(const EFI_GUID *guid1, + const EFI_GUID *guid2); static EFI_BOOT_SERVICES *__initdata efi_bs; static EFI_HANDLE __initdata efi_ih; @@ -170,7 +172,7 @@ static char *__init w2s(const union string *str) return str->s; } -static bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2) +static bool_t __init __maybe_unused match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2) { return guid1->Data1 == guid2->Data1 && guid1->Data2 == guid2->Data2 && @@ -581,12 +583,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_ih = ImageHandle; efi_bs = SystemTable->BootServices; - efi_rs = SystemTable->RuntimeServices; - efi_ct = SystemTable->ConfigurationTable; - efi_num_ct = SystemTable->NumberOfTableEntries; - efi_version = SystemTable->Hdr.Revision; - efi_fw_vendor = SystemTable->FirmwareVendor; - efi_fw_revision = SystemTable->FirmwareRevision; StdOut = SystemTable->ConOut; StdErr = SystemTable->StdErr ?: StdOut; @@ -805,49 +801,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) boot_cpu_data.x86_capability[1] = cpuid_ext_features; } - /* Obtain basic table pointers. */ - for ( i = 0; i < efi_num_ct; ++i ) - { - static EFI_GUID __initdata acpi2_guid = ACPI_20_TABLE_GUID; - static EFI_GUID __initdata acpi_guid = ACPI_TABLE_GUID; - static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID; - static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID; - - if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) ) - efi.acpi20 = (long)efi_ct[i].VendorTable; - if ( match_guid(&acpi_guid, &efi_ct[i].VendorGuid) ) - efi.acpi = (long)efi_ct[i].VendorTable; - if ( match_guid(&mps_guid, &efi_ct[i].VendorGuid) ) - efi.mps = (long)efi_ct[i].VendorTable; - if ( match_guid(&smbios_guid, &efi_ct[i].VendorGuid) ) - efi.smbios = (long)efi_ct[i].VendorTable; - } + efi_arch_runtime_setup(SystemTable); - if (efi.smbios != EFI_INVALID_TABLE_ADDR) - dmi_efi_get_table((void *)(long)efi.smbios); + efi_arch_smbios(); /* Collect PCI ROM contents. */ efi_arch_pci(); - /* Get snapshot of variable store parameters. */ - status = (efi_rs->Hdr.Revision >> 16) >= 2 ? - efi_rs->QueryVariableInfo(EFI_VARIABLE_NON_VOLATILE | - EFI_VARIABLE_BOOTSERVICE_ACCESS | - EFI_VARIABLE_RUNTIME_ACCESS, - &efi_boot_max_var_store_size, - &efi_boot_remain_var_store_size, - &efi_boot_max_var_size) : - EFI_INCOMPATIBLE_VERSION; - if ( EFI_ERROR(status) ) - { - efi_boot_max_var_store_size = 0; - efi_boot_remain_var_store_size = 0; - efi_boot_max_var_size = status; - PrintStr(L"Warning: Could not query variable store: "); - DisplayUint(status, 0); - PrintStr(newline); - } - efi_arch_memory(); efi_arch_video(base_video, cols, rows, depth, gop); diff --git a/xen/include/asm-x86/efi-boot.h b/xen/include/asm-x86/efi-boot.h index b09e12c..b2e8599 100644 --- a/xen/include/asm-x86/efi-boot.h +++ b/xen/include/asm-x86/efi-boot.h @@ -984,3 +984,58 @@ static void __init efi_arch_handle_module(struct file *file, char *name, mb_modules[mbi.mods_count].mod_end = file->size; ++mbi.mods_count; } + +static void __init efi_arch_smbios(void) +{ + if (efi.smbios != EFI_INVALID_TABLE_ADDR) + dmi_efi_get_table((void *)(long)efi.smbios); +} + +static void __init efi_arch_runtime_setup(EFI_SYSTEM_TABLE *SystemTable) +{ + EFI_STATUS status; + int i; + + efi_rs = SystemTable->RuntimeServices; + efi_ct = SystemTable->ConfigurationTable; + efi_num_ct = SystemTable->NumberOfTableEntries; + efi_version = SystemTable->Hdr.Revision; + efi_fw_vendor = SystemTable->FirmwareVendor; + efi_fw_revision = SystemTable->FirmwareRevision; + + /* Obtain basic table pointers. */ + for ( i = 0; i < efi_num_ct; ++i ) + { + static EFI_GUID __initdata acpi2_guid = ACPI_20_TABLE_GUID; + static EFI_GUID __initdata acpi_guid = ACPI_TABLE_GUID; + static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID; + static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID; + + if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) ) + efi.acpi20 = (long)efi_ct[i].VendorTable; + if ( match_guid(&acpi_guid, &efi_ct[i].VendorGuid) ) + efi.acpi = (long)efi_ct[i].VendorTable; + if ( match_guid(&mps_guid, &efi_ct[i].VendorGuid) ) + efi.mps = (long)efi_ct[i].VendorTable; + if ( match_guid(&smbios_guid, &efi_ct[i].VendorGuid) ) + efi.smbios = (long)efi_ct[i].VendorTable; + } + /* Get snapshot of variable store parameters. */ + status = (efi_rs->Hdr.Revision >> 16) >= 2 ? + efi_rs->QueryVariableInfo(EFI_VARIABLE_NON_VOLATILE | + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS, + &efi_boot_max_var_store_size, + &efi_boot_remain_var_store_size, + &efi_boot_max_var_size) : + EFI_INCOMPATIBLE_VERSION; + if ( EFI_ERROR(status) ) + { + efi_boot_max_var_store_size = 0; + efi_boot_remain_var_store_size = 0; + efi_boot_max_var_size = status; + PrintStr(L"Warning: Could not query variable store: "); + DisplayUint(status, 0); + PrintStr(newline); + } +}
Add architecture specific funtions for setting up SMBIOS and runtime services data structures. These are not fundamentally x86 specific, but ARM currently lacks implementations. These functions may not be needed when ARM SMBIOS and runtime service support is added. Both SMBIOS and EFI runtime service support depend on this EFI boot patch series. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- xen/common/efi/boot.c | 50 ++++---------------------------------- xen/include/asm-x86/efi-boot.h | 55 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 45 deletions(-)