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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Bertrand, see comment on ID_AA64MMFR0_EL1 below, any ideas?


On Fri, 1 Oct 2021, Oleksandr wrote:
> On 01.10.21 10:50, Jan Beulich wrote:
> > On 01.10.2021 01:00, Stefano Stabellini wrote:
> > > On Thu, 30 Sep 2021, 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>
> > > > Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> > > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > I have to admit that I'm a little puzzled to see these R-b-s when ...
> > 
> > > > Please note, that review comments for the RFC version [1] haven't been
> > > > addressed yet.
> > > > It is not forgotten, some clarification is needed. It will be addressed
> > > > for the next version.
> > > > 
> > > > [1]
> > > > https://lore.kernel.org/xen-devel/973f5344-aa10-3ad6-ff02-ad5f358ad279@xxxxxxxxxx/
> > ... Oleksandr makes clear this patch isn't really ready yet.
> 
> Unfortunately, this is true. I am still waiting for the clarification [1]

Although I was aware of comments to older versions, this is actually the
first version of this patch that I reviewed with any level of details; I
didn't read previous comments very closely. I tried to find any bugs or
problems with it and I couldn't see any, so I gave my reviewed-by. I
should have clarified that was meant for the ARM part as I don't have a
full understanding of the implications of using hap_paddr_bits on x86
for VM migration.


But let me take this opportunity to say that although I think the
hypercall is OK, I wish we didn't need this patch at all: it is
problematic because it touches tools, x86 and ARM hypervisor code all
together. It needs at least three acks/reviewed-by to get accepted: from
an x86 maintainer, an arm maintainer and from a tools maintainer. I
don't say this to criticize the patch acceptance process: this patch
makes changes to an existing hypercall so it is only fair that it needs
to go through extra levels of scrutiny. For the sake of simplicity and
decoupling (reducing dependencies between patches and between
components), I think it would be best to introduce an #define for the
minimum value of gpaddr_bits and then move this patch at the end of the
series; that way it becomes optional. Unfortunately the minimum value
is 32 (in practice I have never seen less than 40 but the architecture
supports 32 as minimum).


Actually, the info we are looking for is already exposed via
ID_AA64MMFR0_EL1. ID_AA64MMFR0_EL1 can be read from a virtual machine,
and Linux let userspace read it [1]. Regardless of this patch series, we
should make sure that Xen exposes the right mm64.pa_range value to guest
virtual machines. If that is done right, then you can just add support
for reading ID_AA64MMFR0_EL1 in libxl/libxc and then we don't need any
hypercall modifications changes.

So, in theory we already have all the interfaces we need, but in
practice they don't work: unfortunaly both Xen and Linux mark
ID_AA64MMFR0_EL1 as FTR_HIDDEN in cpufeature.c so neither Linux from
Xen, not userspace from Linux can actually read the real value :-/
They always read zero.

(Also I think we have an issue today with p2m_restrict_ipa_bits not
updating the mm64.pa_range value. I think that it should be fixed.)

Bertrand, do you have any ideas in regards to ID_AA64MMFR0_EL1?

If not, maybe we could just go with
#define MIN_GPADDR_BITS 32


[1] https://01.org/linuxgraphics/gfx-docs/drm/arm64/cpu-feature-registers.html