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

[Julien Grall <julien@xxxxxxx>]

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.

Cheers,

--
Julien Grall