Re: [PATCH 4/8] efi: Avoid hard-coding the various PAT constants

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 06/12/2022 04:33, Demi Marie Obenour wrote:
> This makes the code much easier to understand, and avoids problems if
> Xen's PAT ever changes in the future.
>
> Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>  xen/common/efi/boot.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 
> 8e880fe30c7541a202dec3e665300d6549953aa3..260997b251b09dae4b48c1b1db665778e02d760a
>  100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1746,21 +1746,21 @@ void __init efi_init_memory(void)
>          if ( desc->Attribute & EFI_MEMORY_WB )
>              /* nothing */;

This is an implicit 0 case which wants changing to _PAGE_WB.

>          else if ( desc->Attribute & EFI_MEMORY_WT )
> -            prot |= _PAGE_PWT | MAP_SMALL_PAGES;
> +            prot |= _PAGE_WT | MAP_SMALL_PAGES;
>          else if ( desc->Attribute & EFI_MEMORY_WC )
> -            prot |= _PAGE_PAT | MAP_SMALL_PAGES;
> +            prot |= _PAGE_WC | MAP_SMALL_PAGES;
>          else if ( desc->Attribute & (EFI_MEMORY_UC | EFI_MEMORY_UCE) )
> -            prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
> +            prot |= _PAGE_UC | MAP_SMALL_PAGES;
>          else if ( efi_bs_revision >= EFI_REVISION(2, 5) &&
>                    (desc->Attribute & EFI_MEMORY_WP) )
> -            prot |= _PAGE_PAT | _PAGE_PWT | MAP_SMALL_PAGES;
> +            prot |= _PAGE_WP | MAP_SMALL_PAGES;

Unrelated to the transformation. I'm unconvinced about the correctness
of using MAP_SMALL_PAGES here.  There's nothing wrong with large pages
of reduced cache-ability, and the framebuffer is going to live in one of
these regions, probably a WC one...

~Andrew

>          else
>          {
>              printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx%s\n",
>                     smfn, emfn - 1, efi_map_uc ? ", assuming UC" : "");
>              if ( !efi_map_uc )
>                  continue;
> -            prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
> +            prot |= _PAGE_UC | MAP_SMALL_PAGES;
>          }
>  
>          if ( desc->Attribute & (efi_bs_revision < EFI_REVISION(2, 5)