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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 16 Apr 2024, at 10:06, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 16/04/2024 09:59, Luca Fancellu wrote:
>>> On 16 Apr 2024, at 09:50, Julien Grall <julien@xxxxxxx> 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?
>> Yes I agree with you, I’ll drop the patch that tries to do the merge, I was 
>> thinking about checking that the guest phys static mem region is
>> inside these boundaries:
>> #define GUEST_RAM_BANK_BASES { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
>> #define GUEST_RAM_BANK_SIZES { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
>> and also that doesn’t overlap with (struct kernel_info).mem, does it sounds 
>> right to you?
> 
> I don't fully understand what you want to do. But as I wrote before, the 
> overlaps happen with many different regions (what if you try to use the GIC 
> Distributor regions for the shared memory?).
> 
> So if we want some overlaps check, then it has to be generic.

After a chat in Matrix now I understand what you mean, thanks, I will just drop 
the patch 12 and update this one.

Cheers,
Luca