[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.