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

[Julien Grall <julien@xxxxxxx>]

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?

Cheers,

--
Julien Grall