Re: [PATCH v6 10/15] efi: switch to new APIs in EFI code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.04.2020 16:09, Hongyan Xia wrote:
> --- a/xen/arch/x86/efi/runtime.h
> +++ b/xen/arch/x86/efi/runtime.h
> @@ -1,12 +1,18 @@
> +#include <xen/domain_page.h>
> +#include <xen/mm.h>
>  #include <asm/atomic.h>
>  #include <asm/mc146818rtc.h>
>  
>  #ifndef COMPAT
> -l4_pgentry_t *__read_mostly efi_l4_pgtable;
> +mfn_t __read_mostly efi_l4_mfn = INVALID_MFN_INITIALIZER;
>  
>  void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e)
>  {
> -    if ( efi_l4_pgtable )
> +    if ( !mfn_eq(efi_l4_mfn, INVALID_MFN) )
> +    {
> +        l4_pgentry_t *efi_l4_pgtable = map_domain_page(efi_l4_mfn);
>          l4e_write(efi_l4_pgtable + l4idx, l4e);

Blank line between declaration(s) and statement(s) please.

Also, while I realize the choice of name of the local variable
is (presumably) to avoid further code churn, I think it isn't
really suitable for a local variable (also elsewhere below).

> @@ -1489,6 +1493,7 @@ static bool __init rt_range_valid(unsigned long smfn, 
> unsigned long emfn)
>  void __init efi_init_memory(void)
>  {
>      unsigned int i;
> +    l4_pgentry_t *efi_l4_pgtable;
>      struct rt_extra {
>          struct rt_extra *next;
>          unsigned long smfn, emfn;
> @@ -1603,8 +1608,9 @@ void __init efi_init_memory(void)
>       * Set up 1:1 page tables for runtime calls. See SetVirtualAddressMap() 
> in
>       * efi_exit_boot().
>       */
> -    efi_l4_pgtable = alloc_xen_pagetable();
> -    BUG_ON(!efi_l4_pgtable);
> +    efi_l4_mfn = alloc_xen_pagetable_new();
> +    BUG_ON(mfn_eq(efi_l4_mfn, INVALID_MFN));
> +    efi_l4_pgtable = map_domain_page(efi_l4_mfn);
>      clear_page(efi_l4_pgtable);
>  
>      copy_mapping(0, max_page, ram_range_valid);

Why don't you pass the already mapped L4 table into this function,
rather than mapping the same page a 2nd time there?

> @@ -1681,11 +1693,17 @@ void __init efi_init_memory(void)
>              extra_head = extra->next;
>              xfree(extra);
>          }
> +
> +        unmap_domain_page(l1t);
> +        unmap_domain_page(pl2e);
> +        unmap_domain_page(pl3e);

All three should be pulled further up, each to the earliest
possible place (and then using the uppercase version of the
construct as suitable).

Jan