Hi Julien, On 16/04/2024 10:50, Julien Grall 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? I agree with your analysis. For now, let's drop this patch. @Luca: This means I reviewed your series completely and you can send a respin.
~Michal
