Re: [RFC PATCH 1/3] xen: Introduce "gpaddr_bits" field to XEN_SYSCTL_physinfo

[Oleksandr <olekstysh@xxxxxxxxx>]

Hi Andrew.


On 08.09.21 00:28, Oleksandr wrote:
> On 07.09.21 20:35, Andrew Cooper wrote:
>
> Hi Andrew
>
> > On 07/09/2021 18:09, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > >
> > > We need to pass info about maximum supported guest address
> > > space size to the toolstack on Arm in order to properly
> > > calculate the base and size of the extended region (safe range)
> > > for the guest. The extended region is unused address space which
> > > could be safely used by domain for foreign/grant mappings on Arm.
> > > The extended region itself will be handled by the subsequents
> > > patch.
> > >
> > > Use p2m_ipa_bits variable on Arm, the x86 equivalent is
> > > hap_paddr_bits.
> > >
> > > As we change the size of structure bump the interface version.
> > >
> > > Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > So while I think this is a suitable way forward, you're painting
> > yourself into a corner WRT migration.
> >
> > On x86, the correct value is d->arch.cpuid->extd.maxphysaddr and this
> > value is under toolstack control, not Xen control.  In particular, it
> > needs to be min(hostA, hostB) to make migration safe, and yes - this is
> > currently a hole in x86's migration logic that will cause large VMs to
> > explode.
> >
> > The same will be true on ARM as/when you gain migration support.
> Oh, I admit, I didn't keep in mind migration support while creating 
> this patch.
>
> > I think this would be better as a domctl.  On ARM, it can reference
> > p2m_ipa_bits for now along with a /* TODO - make per-domain for
> > migration support */, while on x86 it can take the appropriate value
> > (which will soon actually be safe in migration scenarios).
> OK, could you please clarify, did you mean to introduce new domctl 
> (something like get_maxphysaddr) or reuse some existing?

May I please ask for clarification here ^ and ...


> And ...
>
> > However, that does change the ordering requirements in the toolstack -
> > this hypercall would need to be made after the domain is created, and
> > has been levelled, and before its main memory layout is decided.
> ... I am not sure I totally understand the ordering requirements in 
> the toolstack.
> On Arm this information (gpaddr_bits) should be obtained by the time 
> we call libxl__arch_domain_finalise_hw_description()
> where we actually calculate extended region and insert it in domain's 
> device-tree. At that time, the domain memory is already populated, so 
> it's layout is known.
> Please see the last patch of this series, which demonstrates the usage:
>
> https://lore.kernel.org/xen-devel/1631034578-12598-4-git-send-email-olekstysh@xxxxxxxxx/ 
>
>
> > Alternatively, the abstraction could be hidden in libxl itself in arch
> > specific code, with x86 referring to the local cpu policy (as libxl has
> > the copy it is/has worked on), and ARM making a hypercall.  This does
> > make the ordering more obvious.
> May I please ask what would be the preferred option to move forward? I 
> am fine with both proposed options, with a little preference for the 
> former (common domctl, abstraction is hidden in Xen itself in arch 
> specific code). It is highly appreciated if we could choose the option 
> which would satisfy all parties, this would save me some time.

... here ^ ?


I have just pushed third version [1] of this series without these being 
addressed. I decided to push V3 to let the reviewers to focus on the 
main aspects, but the way how the information in question is passed to 
the toolstack also needs to be clarified and addressed.

[1] 
https://lore.kernel.org/xen-devel/1632437334-12015-1-git-send-email-olekstysh@xxxxxxxxx/

[snip]


--
Regards,

Oleksandr Tyshchenko