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 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?

So in this patch I will only populate the /memory nodes with the static shared 
memory banks.


> 
> Cheers,
> 
> -- 
> Julien Grall