Re: [Xen-devel] [PATCH] x86/efi: Reserve EFI properties table

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.17 at 18:17, <ross.lagerwall@xxxxxxxxxx> wrote:
> Some EFI firmware implementations may place the EFI properties table in
> RAM marked as BootServicesData, which Xen does not consider as reserved.

And which is correct. Hence ...

> When dom0 tries to access the EFI properties table (which Linux >= 4.4
> does), it crashes with a page fault.  Fix this by unconditionally
> marking the EFI properties table as reserved in the E820,

... "fix this by" is the wrong term, "work around this by" would be
more suitable.

But what's worse - Linux has no business looking at the sole bit
defined in MemoryProtectionAttribute, as that's relevant to Xen
(as the exclusive entity dealing with the machine memory map)
only. While I view this by itself as a reason to NAK this patch,
I'll nevertheless give a few comments below.

> much like is done with the dmi regions.

??? efi_arch_process_memory_map() doesn't have any DMI
specific code, and you don't even come close to introducing
behavior similar to dmi_efi_get_table() / dmi_get_table(), which
results in a proper call to reserve_e820_ram() as opposed to ...

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -209,6 +209,14 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>          }
>      }
>  
> +    if ( efi_properties_tbl_addr && efi_properties_tbl_size )
> +    {
> +        ++e;
> +        e->addr = efi_properties_tbl_addr;
> +        e->size = efi_properties_tbl_size;
> +        e->type = E820_RESERVED;
> +        ++e820_raw.nr_map;
> +    }

... you creating an (in the case you want to deal with) overlapping
entry, which we should really avoid.

> @@ -171,6 +173,15 @@ static char __section(".bss.page_aligned") 
> __aligned(PAGE_SIZE)
>      ebmalloc_mem[EBMALLOC_SIZE];
>  static unsigned long __initdata ebmalloc_allocated;
>  
> +struct efi_properties_table {
> +    u32 version;
> +    u32 length;
> +    u64 memory_protection_attribute;
> +};
> +
> +u64 __initdata efi_properties_tbl_addr;
> +u32 __initdata efi_properties_tbl_size;

No new uses of u32 / u64 please. Namely considering the patch
context of the header change, it should have occurred to you to
use UINT64 / UINT32 / UINTN here.

Also please use the properly spelled structure tag and field names,
as per the UEFI spec. Eventually I'd expect these to appear in
one of the canonical EFI headers, at which time it should be
possible to simply drop the definition here without the need to
adjust any other code.

> @@ -820,6 +832,14 @@ static void __init efi_tables(void)
>              efi.smbios = (long)efi_ct[i].VendorTable;
>          if ( match_guid(&smbios3_guid, &efi_ct[i].VendorGuid) )
>              efi.smbios3 = (long)efi_ct[i].VendorTable;
> +        if ( match_guid(&properties_guid, &efi_ct[i].VendorGuid) )
> +        {
> +            struct efi_properties_table *properties;

const

> +            efi_properties_tbl_addr = (long)efi_ct[i].VendorTable;
> +            properties = (struct efi_properties_table 
> *)efi_properties_tbl_addr;

This second cast could be easily avoided if you assigned from
efi_ct[i].VendorTable, which is VOID *.

> @@ -39,3 +40,6 @@ extern UINT64 efi_boot_max_var_store_size, 
> efi_boot_remain_var_store_size,
>  
>  extern UINT64 efi_apple_properties_addr;
>  extern UINTN efi_apple_properties_len;
> +
> +extern u64 __initdata efi_properties_tbl_addr;
> +extern u32 __initdata efi_properties_tbl_size;

__initdata does not belong onto declarations. Only for definitions
this annotation is meaningful.

Jan


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