Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 16/04/2024 10:50, Julien Grall wrote:
> 
> 
> On 16/04/2024 07:27, Luca Fancellu wrote:
>> Hi Julien,
> 
> Hi Luca,
> 
>>
>>> On 15 Apr 2024, at 19:41, Julien Grall <julien@xxxxxxx> wrote:
>>>
>>> Hi Luca,
>>>
>>> On 09/04/2024 12:45, Luca Fancellu wrote:
>>>> Currently Xen is not exporting the static shared memory regions
>>>> to the device tree as /memory node, this commit is fixing this
>>>> issue.
>>>> The static shared memory banks can be part of the memory range
>>>> available for the domain, so if they are overlapping with the
>>>> normal memory banks, they need to be merged together in order
>>>> to produce a /memory node with non overlapping ranges in 'reg'.
>>>
>>> Before reviewing the code in more details, I would like to understand a bit 
>>> more the use case and whether it should be valid.
>>>
>>>  From my understanding, the case you are trying to prevent is the following 
>>> setup:
>>>   1. The Guest Physical region 0x0000 to 0x8000 is used for RAM
>>>   2. The Guest Physical region 0x0000 to 0x4000 is used for static memory
>>
>> So far, it was possible to map guest physical regions inside the memory 
>> range given to the guest,
>> so the above configuration was allowed and the underlying host physical 
>> regions were of course
>> different and enforced with checks. So I’m not trying to prevent this 
>> behaviour, however ...
>>
>>>
>>> The underlying Host Physical regions may be different. Xen doesn't 
>>> guarantee in which order the regions will be mapped, So whether the 
>>> overlapped region will point to the memory or the shared region is unknown 
>>> (we don't guarantee the order of the mapping). So nothing good will happen 
>>> to the guest.
>>
>> ... now here I don’t understand if this was wrong from the beginning or not, 
>> shall we enforce also that
>> guest physical regions for static shared memory are outside the memory given 
>> to the guest?
> 
> Nothing good will happen if you are trying to overwrite mappings. So I
> think this should be enforced. However, this is a more general problem.
> At the moment, this is pretty much as mess because you can overwrite any
> mapping (e.g. map MMIO on top of the RAM).
> 
> I think the easiest way to enforce is to do it in the P2M code like x86
> does for certain mappings.
> 
> Anyway, I don't think the problem should be solved here or by you (this
> is likely going to be a can of worms). For now, I would consider to
> simply drop the patches that are trying to do the merge.
> 
> Any thoughts?
I agree with your analysis. For now, let's drop this patch.
@Luca: This means I reviewed your series completely and you can send a respin.

~Michal