Re: [PATCH V4 2/3] xen/arm: Add handling of extended regions for Dom0

[Julien Grall <julien@xxxxxxx>]

Hi Oleksandr,

On 06/10/2021 20:15, Oleksandr Tyshchenko wrote:
> On Wed, Oct 6, 2021 at 9:11 PM Julien Grall <julien@xxxxxxx 
> <mailto:julien@xxxxxxx>> wrote:
>     Hi Oleksandr,
>
>
> Hi Julien
>
> [Sorry for the possible format issues]
>
>
>     On 04/10/2021 14:08, Oleksandr wrote:
>      >
>      > On 04.10.21 09:59, Julien Grall wrote:
>      >> Hi Oleksandr,
>      >
>      > Hi Julien
>
>     Hi Oleksandr,
>
>      >
>      >
>      >>
>      >> I saw Stefano committed this patch on Friday. However, I didn't
>     have a
>      >> chance go to through a second time and would like to request some
>      >> follow-up changes.
>      >
>      > ok, do you prefer the follow-up patch to be pushed separately or
>     within
>      > the rest patches of this series (#1 and #3)?  If the former, I
>     will try
>      > to push it today to close this question.
>
>     I don't mind. My main ask is they are addressed for 4.16.
>
>      >
>      >
>      >>
>      >>
>      >> On 30/09/2021 00:52, Oleksandr Tyshchenko wrote:
>      >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx
>     <mailto:oleksandr_tyshchenko@xxxxxxxx>>
>      >>>
>      >>> The extended region (safe range) is a region of guest physical
>      >>> address space which is unused and could be safely used to create
>      >>> grant/foreign mappings instead of wasting real RAM pages from
>      >>> the domain memory for establishing these mappings.
>      >>>
>      >>> The extended regions are chosen at the domain creation time and
>      >>> advertised to it via "reg" property under hypervisor node in
>      >>> the guest device-tree. As region 0 is reserved for grant table
>      >>> space (always present), the indexes for extended regions are 1...N.
>      >>> If extended regions could not be allocated for some reason,
>      >>> Xen doesn't fail and behaves as usual, so only inserts region 0.
>      >>>
>      >>> Please note the following limitations:
>      >>> - The extended region feature is only supported for 64-bit domain
>      >>>    currently.
>      >>> - The ACPI case is not covered.
>      >>>
>      >>> ***
>      >>>
>      >>> As Dom0 is direct mapped domain on Arm (e.g. MFN == GFN)
>      >>> the algorithm to choose extended regions for it is different
>      >>> in comparison with the algorithm for non-direct mapped DomU.
>      >>> What is more, that extended regions should be chosen differently
>      >>> whether IOMMU is enabled or not.
>      >>>
>      >>> Provide RAM not assigned to Dom0 if IOMMU is disabled or memory
>      >>> holes found in host device-tree if otherwise. Make sure that
>      >>> extended regions are 2MB-aligned and located within maximum
>     possible
>      >>> addressable physical memory range. The minimum size of extended
>      >>> region is 64MB.
>      >>
>      >> You explained below why the 128 limits, but I don't see any
>      >> explanation on why 2MB and 64MB.
>      >>
>      >> IIRC, 2MB was to potentally allow superpage mapping. I am not
>     entirely
>      >> sure for 64MB.
>      >>
>      >> Could you add an in-code comment explaining the two limits?
>      >
>      > Yes. There was a discussion at [1]. 64MB was chosen as a reasonable
>      > value to deal with between initial 2MB (we might end up having a
>     lot of
>      > small ranges which are not quite useful but increase bookkeeping)
>     and
>      > suggested 1GB (we might not be able find a suitable regions at all).
>
>     Ok. Please document in the code. Note that I don't think this choice
>     should be advertised to the OS. This would give us some flexibility to
>     change the size in the future (e.g. if we have platform where chunk of
>     less than 64MB is beneficial).
>
>      >> The code below looks like an open-coding version of
>      >> dt_for_each_range(). Can you try to re-use it please? This will
>     help
>      >> to reduce the complexity of this function.
>      >
>      > You are right, it makes sense, will definitely reuse. If I was
>     aware of
>      > that function before I would safe some time I spent on the
>     investigation
>      > how to parse that)
>
>     I have only skimmed through the diff below. This looks fine to me.
>     Please submit a formal patch.
>
>
> Already submitted, please take a look at [1].
>
>   [1] 
> https://lore.kernel.org/xen-devel/1633519346-3686-4-git-send-email-olekstysh@xxxxxxxxx/ 
> <https://lore.kernel.org/xen-devel/1633519346-3686-4-git-send-email-olekstysh@xxxxxxxxx/>
Thanks! Sorry, my inbox has been growing quite a lot while I was away. I 
will have a look when I am fully back next week :).
Cheers,

--
Julien Grall