Re: [RFC PATCH] xen/memory: Introduce a hypercall to provide unallocated space

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 5 Aug 2021, Julien Grall wrote:
> Hi Oleksandr,
> 
> On 05/08/2021 15:52, Oleksandr wrote:
> > 
> > On 05.08.21 01:00, Julien Grall wrote:
> > > 
> > > 
> > > On 04/08/2021 21:56, Oleksandr wrote:
> > > > 
> > > > Hi Julien, Stefano.
> > > 
> > > Hi Oleksandr,
> > 
> > 
> > Hi, Julien
> > 
> > 
> > Thank you for the prompt reply and explanations.
> > 
> > 
> > > 
> > > > 
> > > > On 02.08.21 22:12, Oleksandr wrote:
> > > > I have done some experiments with Xen and toolstack according to the
> > > > discussion above. So, I re-used DTB to pass a safe range to the domain.
> > > > For the range I borrowed some space from the second RAM bank.
> > > > 
> > > > -#define GUEST_RAM1_BASE   xen_mk_ullong(0x0200000000) /* 1016GB of RAM
> > > > @ 8GB */
> > > > -#define GUEST_RAM1_SIZE   xen_mk_ullong(0xfe00000000)
> > > > +#define GUEST_RAM1_BASE   xen_mk_ullong(0x0200000000) /* 888GB of RAM @
> > > > 8GB */
> > > > +#define GUEST_RAM1_SIZE   xen_mk_ullong(0xDE00000000)
> > > > +
> > > 
> > > I am a bit split with reducing the amount of RAM. On one hand large guest
> > > is not unheard on the server side (at least in the x86 world). On the
> > > other hand, I am not aware of anyone using Xen on Arm in such setup.
> > > 
> > > So technically this will be a regression, but it may be OK.
> > 
> > I got it.
> > 
> > 
> > > 
> > > 
> > > Regarding the range, this will be a problem as Xen configure the number of
> > > the IPA bits based on the PA bits. The lowest possible address space ize
> > > on 64-bit is 4GB.
> > > 
> > > From my understanding, this is because the number of IPA bits supported is
> > > contrained by the PA bits. So the position and the size of the region
> > > would need to depend on the P2M configuration.
> > 
> > Indeed, I missed these bits that IPA bits on Arm64 might be < 40 bit, I
> > remember, we select p2m_ipa_bits in setup_virt_paging() depending on pabits,
> > moreover the p2m_ipa_bits might be even restricted by some external entity
> > (IOMMU, if P2M is shared).
> > 
> > 
> > > 
> > > For simplicity, this could be the last few X bytes of the supported
> > > address space.
> > ok, agree. To summarize, so it sounds like we can't use the fixed safe range
> > as in my example, it must be variable. Well, I hope, we will be able to
> > achieve this without reducing the total amount of domain RAM in front
> > (GUEST_RAM1_SIZE). After all, we know the IPA size and the domain RAM in
> > advance, so we certainly can choose the start and size of the range. In
> > case, we won't be able to find a suitable large chunk (for example, when IPA
> > bits = 32, and domain has a lot of RAM assigned and as the result - almost
> > all address space below 4GB is in use), we won't expose a safe range to a
> > domain at all, and domain will just fall back to use real pages instead
> > (actually, how it currently behaves on Arm).
> 
> I think it would be fine for a first approach. We can refine it in the future.
> What matters is that we correctly define the binding/hypercall.

I agree with Julien on both points. Looking at the existing device tree
binding, it is almost exactly what we need, so I think it would be OK to
use it.


> > > For 32-bit domain, we also need to make sure the address is usable for
> > > domain short page tables (not too long ago Debian was shipping the kernel
> > > with them rather than LPAE). I haven't yet checked what's the limit here.
> > 
> > Hmm, I didn't take this use-case into the account. So, I assume we need the
> > safe range to be located below 4GB if is_32bit_domain() returns true.
> 
> Yes. Or we can say that if you are using a 32-bit domain then we don't (yet)
> support a safe range for range.
> > > So we would need some heuristic to decide whether to stole some RAM or use
> > > the safe space.
> > > Another possibility would be to add a new compatible in the DT that
> > > indicates the region is "big" enough.
> > I like the last idea, did you perhaps mean new property (optional) rather
> > than new compatible? Let's say "xen, safe-range" or "xen, extended-regions" 
> > ...
> 
> I actually meant adding an extra compatible because this is technically a
> change of the binding (even though it is backward compatible).
> 
> Although, I would be OK with the property. You may first want to ask the
> Device-Tree folks how they expect a binding to be extended in a backward
> compatible way.

I think we should expand the description in
Documentation/devicetree/bindings/arm/xen.txt to cover things other than
the grant table.

I don't think we necessarely need a new compatible string because as you
said the change is fully compatible. At the same time it is trivial to
add compatible strings, so that could be done as well.