> On 16 Apr 2024, at 09:50, Julien Grall <jul...@xen.org> wrote:
>
>
>
> On 16/04/2024 07:27, Luca Fancellu wrote:
>> Hi Julien,
>
> Hi Luca,
>
>>> On 15 Apr 2024, at 19:41, Julien Grall <jul...@xen.org> 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
