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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.10.2022 17:20, Roger Pau Monné wrote:
> On Tue, Oct 04, 2022 at 04:39:26PM +0200, Jan Beulich wrote:
>> On 04.10.2022 16:01, Roger Pau Monné wrote:
>>> On Tue, Oct 04, 2022 at 03:10:57PM +0200, Jan Beulich wrote:
>>>> On 04.10.2022 14:52, Roger Pau Monné wrote:
>>>>> On Tue, Oct 04, 2022 at 02:18:31PM +0200, Jan Beulich wrote:
>>>>>> On 04.10.2022 12:54, Roger Pau Monné wrote:
>>>>>>> On Tue, Oct 04, 2022 at 12:44:16PM +0200, Jan Beulich wrote:
>>>>>>>> On 04.10.2022 12:38, Roger Pau Monné wrote:
>>>>>>>>> On Tue, Oct 04, 2022 at 12:23:23PM +0200, Jan Beulich wrote:
>>>>>>>>>> On 04.10.2022 11:33, Roger Pau Monné wrote:
>>>>>>>>>>> On Tue, Oct 04, 2022 at 10:06:36AM +0200, Jan Beulich wrote:
>>>>>>>>>>>> On 30.09.2022 16:28, Roger Pau Monné wrote:
>>>>>>>>>>>>> On Fri, Sep 30, 2022 at 09:50:40AM +0200, Jan Beulich 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.
>>>>>>>>>>>>>
>>>>>>>>>>>>> What about dom0?  Should it be translated to E820_RESERVED so that
>>>>>>>>>>>>> dom0 doesn't try to use it either?
>>>>>>>>>>>>
>>>>>>>>>>>> I'm afraid I don't understand the questions. Not the least because 
>>>>>>>>>>>> I
>>>>>>>>>>>> think "it" can't really mean "dom0" from the earlier sentence.
>>>>>>>>>>>
>>>>>>>>>>> Sorry, let me try again:
>>>>>>>>>>>
>>>>>>>>>>> The memory map provided to dom0 will contain E820_ACPI entries for
>>>>>>>>>>> memory ranges with the EFI_MEMORY_RUNTIME attributes in the EFI 
>>>>>>>>>>> memory
>>>>>>>>>>> map.  Is there a risk from dom0 reclaiming such E820_ACPI ranges,
>>>>>>>>>>> overwriting the data needed for runtime services?
>>>>>>>>>>
>>>>>>>>>> How would Dom0 go about doing so? It has no control over what we hand
>>>>>>>>>> to the page allocator - it can only free pages which were actually
>>>>>>>>>> allocated to it. E820_ACPI and E820_RESERVED pages are assigned to
>>>>>>>>>> DomIO - Dom0 can map and access them, but it cannot free them.
>>>>>>>>>
>>>>>>>>> Maybe I'm very confused, but what about dom0 overwriting the data
>>>>>>>>> there, won't it cause issues to runtime services?
>>>>>>>>
>>>>>>>> If it overwrites it, of course there are going to be issues. Just like
>>>>>>>> there are going to be problems from anything else Dom0 does wrong.
>>>>>>>
>>>>>>> But would dom0 know it's doing something wrong?
>>>>>>
>>>>>> Yes. Please also see my reply to Andrew.
>>>>>>
>>>>>>> The region is just marked as E820_ACPI from dom0 PoV, so it doesn't
>>>>>>> know it's required by EFI runtime services, and dom0 could
>>>>>>> legitimately overwrite the region once it considers all ACPI parsing
>>>>>>> done from it's side.
>>>>>>
>>>>>> PV Dom0 won't ever see E820_ACPI in the relevant E820 map; this type can
>>>>>> only appear in the machine E820. In how far PVH Dom0 might need to take
>>>>>> special care I can't tell right now (but at least for kexec purposes I
>>>>>> expect Linux isn't going to recycle E820_ACPI regions even going 
>>>>>> forward).
>>>>>
>>>>> Even if unlikely, couldn't some dom0 OS look at the machine map after
>>>>> processing ACPI and just decide to overwrite the ACPI regions?
>>>>>
>>>>> Not that it's useful from an OS PoV, but also we have no statement
>>>>> saying that E820_ACPI in the machine memory map shouldn't be
>>>>> overwritten.
>>>>
>>>> There are many things we have no statements for, yet we imply certain
>>>> behavior or restrictions. The machine memory map, imo, clearly isn't
>>>> intended for this kind of use.
>>>
>>> There isn't much I can say then.  I do feel we are creating rules out
>>> of thin air.
>>>
>>> I do think the commit message should mention that we rely on dom0 not
>>> overwriting the data in the E820_ACPI regions on the machine memory
>>> map.
>>
>> Hmm, am I getting it right that you think I need to add further
>> justification for a change I'm _not_ making?
> 
> In the commit message you explicitly mentioned 'we don't actually
> "reclaim" E820_ACPI memory' and I assumed that "we" in the sentence to
> only include Xen.  Now I see that the "we" there seems to include both
> Xen and the dom0 kernel.  This wasn't clear to me at first sight.

It was clear, actually, as I did mean Xen alone. It didn't even occur to
me that one could consider Dom0 potentially trying to do so.

>> And which, if we wanted
>> to change our behavior, would require a similar change (or perhaps a
>> change elsewhere) in E820 (i.e. non-EFI) handling?
> 
> Why would that be required?

Because if EFI can (ab)use that type for other purposes, why couldn't
legacy firmware, too?

> Without EFI dom0 should be fine in overwriting (some?) of the data in
> E820_ACPI regions once it's finished with all ACPI processing, as a
> region of type E820_ACPI is reclaimable and Xen won't try to access it
> once handled to dom0.
> 
>> The modification
>> I'm making is solely towards Xen's internal memory management. I'm
>> really having a hard time seeing how commenting on expected Dom0
>> behavior would fit here
> 
> The type in the e820 memory map also gets propagated to dom0 in the
> machine memory map hypercall, so it can have effect outside of Xen
> itself.

If used beyond the very limited intended purposes, yes.

>> (leaving aside that I'm still puzzled by both
>> you and Andrew thinking that there's any whatsoever remote indication
>> anywhere that Dom0 recycling E820_ACPI could be an okay thing in a PV
>> Dom0 kernel). The more that marking EfiACPIReclaimMemory anything
>> other than E820_ACPI might, as iirc you did say yourself, also confuse
>> e.g. the ACPI subsystem of Dom0's kernel.
> 
> Indeed.  There's no good way to convert a region of type
> EfiACPIReclaimMemory that has the EFI_MEMORY_RUNTIME attribute set, as
> there's no mapping to an e820 type.
> 
> One of the quirks of trying to retrofit an EFI memory map into e820
> format.
> 
>> But well, would extending that sentence to "While on x86 in theory the
>> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
>> E820_ACPI memory there (and it would be a bug if the Dom0 kernel tried
>> to do so, bypassing Xen's memory management), hence that type's
>> handling can be left alone" satisfy your request?
> 
> I think that would indeed make it clearer.

Okay, I'll make the adjustment then and submit a v2. This will now need
an ack also by Henry anyway.

Jan