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

[Oleksandr Tyshchenko <olekstysh@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3860 bytes --]

On Wed, Oct 6, 2021 at 9:11 PM Julien Grall <julien@xen.org> 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@epam.com>
> >>>
> >>> 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@gmail.com/

-- 
Regards,

Oleksandr Tyshchenko

[-- Attachment #2: Type: text/html, Size: 5807 bytes --]