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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksandr,

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>
> > >
> > > 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.
Cheers,

--
Julien Grall