Re: [Xen-devel] [PATCH v1 2/3] EFI/early: Add /map to map EfiBootServicesData and Code

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx> 04/24/15 10:48 PM >>>
>--- a/xen/arch/x86/efi/efi-boot.h
>+++ b/xen/arch/x86/efi/efi-boot.h
>@@ -133,7 +133,8 @@ static void __init 
>efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
                                                >void *map,
                                                >UINTN map_size,
                                                >UINTN desc_size,
>-                                               UINT32 desc_ver)
>+                                               UINT32 desc_ver,
>+                                               UINT32 map_bootservices)
 
There's no reason I can see for this not to be bool_t.

>@@ -151,9 +152,14 @@ static void __init 
>efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
         >default:
             >type = E820_RESERVED;
             >break;
>-        case EfiConventionalMemory:
         >case EfiBootServicesCode:
         >case EfiBootServicesData:
>+            if ( map_bootservices )
>+            {
>+                type = E820_RESERVED;
>+                break;
>+            }
>+        case EfiConventionalMemory:
             
For Coverity's sake please add a fall-through comment here.

>@@ -755,6 +756,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>*SystemTable)
                     >base_video = 1;
                 >else if ( wstrcmp(ptr + 1, L"noexit") == 0 )
                     >exit_boot_services = 0;
>+                else if ( wstrcmp(ptr + 1, L"map") == 0 )
>+                    map_bs = 1;
                 
"map" is surely ambiguous - please make this at least "mapbs".

>for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
     >{
         >EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
         >u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
         >unsigned long smfn, emfn;
         >unsigned int prot = PAGE_HYPERVISOR;
>+        unsigned int skip = 1;
 
bool_t.

         >-        if ( !efi_rs_enable || !(desc->Attribute & 
EFI_MEMORY_RUNTIME) )
>+        if ( desc->Attribute & EFI_MEMORY_RUNTIME )
>+            skip = 0;
>+
>+        if ( desc->Type == 4 && desc->Attribute != 0 && efi_map )
>+            skip = 0;
>+
>+        if ( desc->Type == 3 && desc->Attribute != 0 && efi_map )
>+            skip = 0;

Please don't use 3 and 4 here, but the proper EFI #define-s. Or if there is any
reason preventing their use, please at least at these as comments.

Also what's the reason the the ->Attribute != 0 check? (Perhaps worth adding a
comment if this is really needed.)

>--- a/xen/common/efi/efi.h
>+++ b/xen/common/efi/efi.h
>@@ -20,7 +20,7 @@ struct efi_pci_rom {
 >extern unsigned int efi_num_ct;
 >extern const EFI_CONFIGURATION_TABLE *efi_ct;
 >
>-extern unsigned int efi_version, efi_fw_revision;
>+extern unsigned int efi_version, efi_fw_revision, efi_map;

Does this really need to be extern (rather than static __initdata)?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel