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

From: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
To: Marc Zyngier <maz@kernel.org>
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 22:21:32 +0530	[thread overview]
Message-ID: <73ca09b8-189a-eea7-15d7-b17a8d07cb60@os.amperecomputing.com> (raw)
In-Reply-To: <875ysfchrg.wl-maz@kernel.org>


On 26-11-2021 04:20 pm, Marc Zyngier wrote:
> 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.

Thanks Marc.
It is booting now, i could boot L1/Guest-Hypervisor and L2(NestedVM) as 
well.
> 
> 	M.
> 

Thanks,
Ganapat