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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.12.2022 12:17, Andrew Cooper wrote:
> On 06/12/2022 04:33, Demi Marie Obenour wrote:
>> --- 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.

Oh, yes. Demi, feel free to retain my R-b with the adjustment.

>>          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,

Hmm, back in the 32-bit days we needed to be afraid of hardware issues
in that area. Hence we had a global policy of never allowing non-WB
large pages. Maybe we don't need to be concerned anymore ...

> and the framebuffer is going to live in one of these regions, probably
> a WC one...

I very much hope it won't live anywhere there, unless you think of non-
PCI devices supplying framebuffers. As long as it's described by a BAR,
it better wouldn't be covered by a memory map entry.

Jan