Re: [PATCH v3] efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.09.2020 15:30, Jan Beulich wrote:
> On 11.09.2020 16:43, Sergey Temerkhanov wrote:
>> @@ -1510,6 +1517,24 @@ void __init efi_init_memory(void)
>>                 desc->PhysicalStart, desc->PhysicalStart + len - 1,
>>                 desc->Type, desc->Attribute);
>>  
>> +        /*
>> +         * EfiRuntimeServicesCode and EfiRuntimeServicesData
>> +         * memory ranges are adjusted here. Any changes
>> +         * or adjustments must be kept in sync with efi_exit_boot()
>> +         */
>> +        if ( efi_enabled(EFI_RS) &&
>> +             (!(desc->Attribute & EFI_MEMORY_RUNTIME) &&
>> +               (desc->Attribute & EFI_MEMORY_CACHEABILITY_MASK) &&
>> +               (desc->Type == EfiRuntimeServicesCode ||
>> +                desc->Type == EfiRuntimeServicesData)) )
>> +        {
>> +            printk(XENLOG_WARNING
>> +                   "Setting EFI_RUNTIME memory attribute for area %013"
>> +                   PRIx64 "-%013" PRIx64 "\n",
>> +                   desc->PhysicalStart, desc->PhysicalStart + len - 1);
>> +            desc->Attribute |= EFI_MEMORY_RUNTIME;
>> +        }
> 
> So you've moved from always checking for EFI_MEMORY_WP to not
> checking it at all. Neither is the way to go imo. Similarly, ...
> 
>> --- a/xen/include/efi/efidef.h
>> +++ b/xen/include/efi/efidef.h
>> @@ -158,6 +158,12 @@ typedef enum {
>>  #define EFI_MEMORY_UCE          0x0000000000000010  
>>  #define EFI_MEMORY_WP           0x0000000000001000
>>  
>> +#define EFI_MEMORY_CACHEABILITY_MASK  ( EFI_MEMORY_UC | \
>> +                                        EFI_MEMORY_WC | \
>> +                                        EFI_MEMORY_WT | \
>> +                                        EFI_MEMORY_WB | \
>> +                                        EFI_MEMORY_UCE )
> 
> ... this now doesn't really cover what its name suggests. As
> indicated before, without such a (questionable) #define having
> appeared in the gnu-efi tree, I don't think we want it, at the
> very least not in this imported header. But given that it
> doesn't express what you want anyway, I can only repeat my
> suggestion to drop this #define altogether.
> 
> In order to save further rounds, I would offer to finish this
> patch to a shape that I'd feel comfortable with - if that's
> okay with you.

Would you mind clarifying whether you intend to finish bringing
this change in shape, or whether I should pick it up?

Jan