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

[Oleksandr <olekstysh@xxxxxxxxx>]

Hi Stefano


On 06.08.21 03:20, Stefano Stabellini wrote:
> 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.
ok, great.


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


> 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.
ok, please note, I have already asked DT folks for the clarification 
(how to make it properly), so let's wait for input.

--
Regards,

Oleksandr Tyshchenko