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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan