Re: [PATCH 1/2] KVM: arm64: Use appropriate mmu pointer in stage2 page table init.

From: Marc Zyngier <maz@kernel.org>
To: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
Cc: catalin.marinas@arm.com, will@kernel.org, andre.przywara@arm.com,
	linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	darren@os.amperecomputing.com,
	Quentin Perret <qperret@google.com>,
	D Scott Phillips <scott@os.amperecomputing.com>
Subject: Re: [PATCH 1/2] KVM: arm64: Use appropriate mmu pointer in stage2 page table init.
Date: Fri, 26 Nov 2021 10:50:59 +0000	[thread overview]
Message-ID: <875ysfchrg.wl-maz@kernel.org> (raw)
In-Reply-To: <84b7602f-93c1-74e3-bebf-23ed9e795b9b@os.amperecomputing.com>

Hi Ganapatro,

On Fri, 26 Nov 2021 05:45:26 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> Hi Marc,
> 
> 
> On 25-11-2021 07:19 pm, Marc Zyngier wrote:
> > [+ Quentin]
> > 
> > Hi Ganapatro,
> > 
> > On Mon, 22 Nov 2021 09:58:02 +0000,
> > Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> >> 
> >> The kvm_pgtable_stage2_init/kvm_pgtable_stage2_init_flags function
> >> assume arch->mmu is same across all stage 2 mmu and initializes
> >> the pgt(page table) using arch->mmu.
> >> Using armc->mmu is not appropriate when nested virtualization is enabled
> >> since there are multiple stage 2 mmu tables are initialized to manage
> >> Guest-Hypervisor as well as Nested VM for the same vCPU.
> >> 
> >> Add a mmu argument to kvm_pgtable_stage2_init that can be used during
> >> initialization. This patch is a preparatory patch for the
> >> nested virtualization series and no functional changes.
> > 
> > Thanks for having had a look, and for the analysis. This is obviously
> > a result of a hasty conversion to the 'new' page table code, and a
> > total oversight on my part.
> > 
> > I'm however not particularly thrilled with the approach you have taken
> > though, as carrying both the kvm->arch pointer *and* the mmu pointer
> > seems totally redundant (the mmu structure already has a backpointer
> > to kvm->arch or its pkvm equivalent). All we need is to rework the
> > initialisation for this pointer to be correct at the point of where we
> > follow it first.
> > 
> > I've pushed out my own version of this[1]. Please have a look.
> > 
> > Thanks,
> > 
> > 	M.
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/nv-5.16-WIP&id=21790a24d88c3ed37989533709dad3d40905f5c3
> > 
> 
> Thanks for the rework and rebasing to 5.16.
> 
> I went through the patch, the gist of the patch seems to me same.
> Please free feel to add,
> Reviewed-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>

Thanks!

> Looks like kvm-arm64/nv-5.16-WIP branch is broken for NV.
> I tried booting Guest hypervisor using lkvm and the vcpu init from
> lkvm is failing(Fatal: Unable to initialise vcpu). Did not dig/debug
> more in to the issue yet.

I'm still trying to iron a few issues, but you should be able to boot
a NV guest. However, the way it is enabled has changed: you need to
pass 'kvm-arm.mode=nested' to the command line instead of the previous
'kvm-arm.nested=1' which I have got rid of. That could well be the
issue.

With the current state of the tree (I just pushed another fix), you
should be able to boot a L1 guest hypervisor and a L2 guest. I'm
getting a crash at the point where the L2 guest reaches userspace
though, so something is broken in the PSTATE or ERET tracking, I'd
expect.

	M.

-- 
Without deviation from the norm, progress is not possible.