Re: [PATCH][4.17] EFI: don't convert memory marked for runtime use to ordinary RAM

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.10.2022 10:39, Jan Beulich wrote:
> On 05.10.2022 20:09, Julien Grall wrote:
>> Hi Jan,
>>
>> On 05/10/2022 12:55, Jan Beulich wrote:
>>> On 05.10.2022 12:44, Julien Grall wrote:
>>>> On 04/10/2022 16:58, Jan Beulich wrote:
>>>>> On 30.09.2022 14:51, Bertrand Marquis wrote:
>>>>>>> On 30 Sep 2022, at 09:50, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>>>>
>>>>>>> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
>>>>>>> higher priority than the type of the range. To avoid accessing memory at
>>>>>>> runtime which was re-used for other purposes, make
>>>>>>> efi_arch_process_memory_map() follow suit. While on x86 in theory the
>>>>>>> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
>>>>>>> E820_ACPI memory there and hence that type's handling can be left alone.
>>>>>>>
>>>>>>> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
>>>>>>> Fixes: facac0af87ef ("x86-64: EFI runtime code")
>>>>>>> Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>>
>>>>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx> #arm
>>>>>
>>>>> Thanks. However ...
>>>>>
>>>>>>> ---
>>>>>>> Partly RFC for Arm, for two reasons:
>>>>>>>
>>>>>>> On Arm I question the conversion of EfiACPIReclaimMemory, in two ways:
>>>>>>> For one like on x86 such ranges would likely better be retained, as Dom0
>>>>>>> may (will?) have a need to look at tables placed there. Plus converting
>>>>>>> such ranges to RAM even if EFI_MEMORY_WB is not set looks suspicious to
>>>>>>> me as well. I'd be inclined to make the latter adjustment right here
>>>>>>> (while the other change probably would better be separate, if there
>>>>>>> aren't actually reasons for the present behavior).
>>>>>
>>>>> ... any views on this WB aspect at least (also Stefano or Julien)? Would 
>>>>> be
>>>>> good to know before I send v2.
>>>>
>>>> I don't quite understand what you are questioning here. Looking at the
>>>> code, EfiACPIReclaimMemory will not be converted to RAM but added in a
>>>> separate array.
>>>>
>>>> Furthermore, all the EfiACPIReclaimMemory regions will be passed to dom0
>>>> (see acpi_create_efi_mmap_table()).
>>>>
>>>> So to me the code looks correct.
>>>
>>> Oh, I've indeed not paid enough attention to the first argument passed
>>> to meminfo_add_bank(). I'm sorry for the extra noise. However, the
>>> question I wanted to have addressed before sending out v3 was that
>>> regarding the present using of memory when EFI_MEMORY_WB is not set.
>>> Is that correct for the EfiACPIReclaimMemory case, i.e. is the
>>> consumer (Dom0) aware that there might be a restriction?
>>
>> Looking at the code, we always set EFI_MEMORY_WB for the reclaimable 
>> region and the stage-2 mapping will be cachable.
>>
>> So it looks like there would be a mismatch if EFI_MEMORY_WB is not set. 
>> However, given the region is reclaimable, shouldn't this imply that the 
>> flag is always set?
> 
> Possibly (but then again consider [perhaps hypothetical] systems with e.g.
> just WT caches, where specifying WB simply wouldn't make sense). In any
> event, even if that's the case, being on the safe side and doing
> 
>         if ( (desc_ptr->Attribute & EFI_MEMORY_RUNTIME) ||
>              !(desc_ptr->Attribute & EFI_MEMORY_WB) )
>             /* nothing */;
>         else if ( ...
> 
> would seem better to me. However, if the mapping you mention above
> would be adjusted and ...
> 
>>> And would
>>> this memory then be guaranteed to never be freed into the general pool
>>> of RAM pages?
>>
>> The region is not treated as RAM by Xen and not owned by the dom0. 
>> Therefore, it should not be possible to free the page because 
>> get_page_from_gfn() would not be able to get a reference.
> 
> ... the space cannot become ordinary RAM, then such a precaution
> wouldn't be necessary. After all hiding EfiACPIReclaimMemory from
> Dom0 just because it can't be mapped WB wouldn't be very nice
> either. I guess I'll submit v2 with this part of the change left
> as it was.

And while already in the process of committing the patch I came to
realize that if the WB conditional isn't supposed to move, isn't
the change done for Arm then wrong as well? Shouldn't it then be

        if ( !(desc_ptr->Attribute & EFI_MEMORY_RUNTIME) &&
             (desc_ptr->Attribute & EFI_MEMORY_WB) &&
             (desc_ptr->Type == EfiConventionalMemory ||
             ...

leaving the EfiACPIReclaimMemory case entirely unaffected by the
change?

Jan