kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] KVM: arm64: nv: Fix issue with Stage 2 MMU init for Nested case.
@ 2021-11-22  9:58 Ganapatrao Kulkarni
  2021-11-22  9:58 ` [PATCH 1/2] KVM: arm64: Use appropriate mmu pointer in stage2 page table init Ganapatrao Kulkarni
  2021-11-22  9:58 ` [PATCH 2/2] KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures Ganapatrao Kulkarni
  0 siblings, 2 replies; 13+ messages in thread
From: Ganapatrao Kulkarni @ 2021-11-22  9:58 UTC (permalink / raw)
  To: maz
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, d.scott.phillips, gankulkarni

The Guest Hypervisor stage 2 mmu table was used while creating map
and subsequent tlb flush for Nested VM. This resulted in unresolvable
stage 2 fault for Nested VM since tlb was invalidated with
Guest-Hypervisor VMID.

Patch 1/2 should be applied before the NV patchset[1].
Patch 2/2 can be squashed in to Commit 1776c91346b6 ("KVM: arm64: nv:
Support multiple nested Stage-2 mmu structures")[2].

[1] https://lore.kernel.org/kvmarm/20210510165920.1913477-1-maz@kernel.org/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/
branch kvm-arm64/nv-5.13

Ganapatrao Kulkarni (2):
  KVM: arm64: Use appropriate mmu pointer in stage2 page table init.
  KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures

 arch/arm64/include/asm/kvm_pgtable.h  | 6 ++++--
 arch/arm64/kvm/hyp/nvhe/mem_protect.c | 2 +-
 arch/arm64/kvm/hyp/pgtable.c          | 3 ++-
 arch/arm64/kvm/mmu.c                  | 2 +-
 arch/arm64/kvm/nested.c               | 9 +++++++++
 5 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/2] KVM: arm64: Use appropriate mmu pointer in stage2 page table init.
  2021-11-22  9:58 [PATCH 0/2] KVM: arm64: nv: Fix issue with Stage 2 MMU init for Nested case Ganapatrao Kulkarni
@ 2021-11-22  9:58 ` Ganapatrao Kulkarni
  2021-11-25 13:49   ` Marc Zyngier
  2021-11-22  9:58 ` [PATCH 2/2] KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures Ganapatrao Kulkarni
  1 sibling, 1 reply; 13+ messages in thread
From: Ganapatrao Kulkarni @ 2021-11-22  9:58 UTC (permalink / raw)
  To: maz
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, d.scott.phillips, gankulkarni

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.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/include/asm/kvm_pgtable.h  | 6 ++++--
 arch/arm64/kvm/hyp/nvhe/mem_protect.c | 2 +-
 arch/arm64/kvm/hyp/pgtable.c          | 3 ++-
 arch/arm64/kvm/mmu.c                  | 2 +-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index 4f432ea3094c..9c0c380f8e3b 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -223,16 +223,18 @@ u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift);
  * @arch:	Arch-specific KVM structure representing the guest virtual
  *		machine.
  * @mm_ops:	Memory management callbacks.
+ * @mmu:	The pointer to the s2 MMU structure
  * @flags:	Stage-2 configuration flags.
  *
  * Return: 0 on success, negative error code on failure.
  */
 int kvm_pgtable_stage2_init_flags(struct kvm_pgtable *pgt, struct kvm_arch *arch,
 				  struct kvm_pgtable_mm_ops *mm_ops,
+				  struct kvm_s2_mmu *mmu,
 				  enum kvm_pgtable_stage2_flags flags);
 
-#define kvm_pgtable_stage2_init(pgt, arch, mm_ops) \
-	kvm_pgtable_stage2_init_flags(pgt, arch, mm_ops, 0)
+#define kvm_pgtable_stage2_init(pgt, arch, mm_ops, mmu) \
+	kvm_pgtable_stage2_init_flags(pgt, arch, mm_ops, mmu, 0)
 
 /**
  * kvm_pgtable_stage2_destroy() - Destroy an unused guest stage-2 page-table.
diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index 4b60c0056c04..cf7e034a0453 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -99,7 +99,7 @@ int kvm_host_prepare_stage2(void *mem_pgt_pool, void *dev_pgt_pool)
 		return ret;
 
 	ret = kvm_pgtable_stage2_init_flags(&host_kvm.pgt, &host_kvm.arch,
-					    &host_kvm.mm_ops, KVM_HOST_S2_FLAGS);
+					    &host_kvm.mm_ops, mmu, KVM_HOST_S2_FLAGS);
 	if (ret)
 		return ret;
 
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index fa85da30c9b8..85acd9e19ed0 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1018,6 +1018,7 @@ int kvm_pgtable_stage2_flush(struct kvm_pgtable *pgt, u64 addr, u64 size)
 
 int kvm_pgtable_stage2_init_flags(struct kvm_pgtable *pgt, struct kvm_arch *arch,
 				  struct kvm_pgtable_mm_ops *mm_ops,
+				  struct kvm_s2_mmu *mmu,
 				  enum kvm_pgtable_stage2_flags flags)
 {
 	size_t pgd_sz;
@@ -1034,7 +1035,7 @@ int kvm_pgtable_stage2_init_flags(struct kvm_pgtable *pgt, struct kvm_arch *arch
 	pgt->ia_bits		= ia_bits;
 	pgt->start_level	= start_level;
 	pgt->mm_ops		= mm_ops;
-	pgt->mmu		= &arch->mmu;
+	pgt->mmu		= mmu;
 	pgt->flags		= flags;
 
 	/* Ensure zeroed PGD pages are visible to the hardware walker */
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 0cf6ab944adc..6cf86cafc65a 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -495,7 +495,7 @@ int kvm_init_stage2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
 	if (!pgt)
 		return -ENOMEM;
 
-	err = kvm_pgtable_stage2_init(pgt, &kvm->arch, &kvm_s2_mm_ops);
+	err = kvm_pgtable_stage2_init(pgt, &kvm->arch, &kvm_s2_mm_ops, mmu);
 	if (err)
 		goto out_free_pgtable;
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures
  2021-11-22  9:58 [PATCH 0/2] KVM: arm64: nv: Fix issue with Stage 2 MMU init for Nested case Ganapatrao Kulkarni
  2021-11-22  9:58 ` [PATCH 1/2] KVM: arm64: Use appropriate mmu pointer in stage2 page table init Ganapatrao Kulkarni
@ 2021-11-22  9:58 ` Ganapatrao Kulkarni
  2021-11-25 14:23   ` Marc Zyngier
  1 sibling, 1 reply; 13+ messages in thread
From: Ganapatrao Kulkarni @ 2021-11-22  9:58 UTC (permalink / raw)
  To: maz
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, d.scott.phillips, gankulkarni

Commit 1776c91346b6 ("KVM: arm64: nv: Support multiple nested Stage-2 mmu
structures")[1] added a function kvm_vcpu_init_nested which expands the
stage-2 mmu structures array when ever a new vCPU is created. The array
is expanded using krealloc() and results in a stale mmu address pointer
in pgt->mmu. Adding a fix to update the pointer with the new address after
successful krealloc.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/
branch kvm-arm64/nv-5.13

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/kvm/nested.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index 4ffbc14d0245..57ad8d8f4ee5 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -68,6 +68,8 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
 		       num_mmus * sizeof(*kvm->arch.nested_mmus),
 		       GFP_KERNEL | __GFP_ZERO);
 	if (tmp) {
+		int i;
+
 		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
 		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
 			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
@@ -80,6 +82,13 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
 		}
 
 		kvm->arch.nested_mmus = tmp;
+
+		/* Fixup pgt->mmu after krealloc */
+		for (i = 0; i < kvm->arch.nested_mmus_size; i++) {
+			struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
+
+			mmu->pgt->mmu = mmu;
+		}
 	}
 
 	mutex_unlock(&kvm->lock);
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: Use appropriate mmu pointer in stage2 page table init.
  2021-11-22  9:58 ` [PATCH 1/2] KVM: arm64: Use appropriate mmu pointer in stage2 page table init Ganapatrao Kulkarni
@ 2021-11-25 13:49   ` Marc Zyngier
  2021-11-26  5:45     ` Ganapatrao Kulkarni
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Zyngier @ 2021-11-25 13:49 UTC (permalink / raw)
  To: Ganapatrao Kulkarni
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, d.scott.phillips, Quentin Perret

[+ 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

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures
  2021-11-22  9:58 ` [PATCH 2/2] KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures Ganapatrao Kulkarni
@ 2021-11-25 14:23   ` Marc Zyngier
  2021-11-26  5:59     ` Ganapatrao Kulkarni
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Zyngier @ 2021-11-25 14:23 UTC (permalink / raw)
  To: Ganapatrao Kulkarni
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, d.scott.phillips

On Mon, 22 Nov 2021 09:58:03 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> Commit 1776c91346b6 ("KVM: arm64: nv: Support multiple nested Stage-2 mmu
> structures")[1] added a function kvm_vcpu_init_nested which expands the
> stage-2 mmu structures array when ever a new vCPU is created. The array
> is expanded using krealloc() and results in a stale mmu address pointer
> in pgt->mmu. Adding a fix to update the pointer with the new address after
> successful krealloc.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/
> branch kvm-arm64/nv-5.13
> 
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/nested.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 4ffbc14d0245..57ad8d8f4ee5 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -68,6 +68,8 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>  		       num_mmus * sizeof(*kvm->arch.nested_mmus),
>  		       GFP_KERNEL | __GFP_ZERO);
>  	if (tmp) {
> +		int i;
> +
>  		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
>  		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
>  			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
> @@ -80,6 +82,13 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>  		}
>  
>  		kvm->arch.nested_mmus = tmp;
> +
> +		/* Fixup pgt->mmu after krealloc */
> +		for (i = 0; i < kvm->arch.nested_mmus_size; i++) {
> +			struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
> +
> +			mmu->pgt->mmu = mmu;
> +		}
>  	}
>  
>  	mutex_unlock(&kvm->lock);

Another good catch. I've tweaked a bit to avoid some unnecessary
repainting, see below.

Thanks again,

	M.

diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index a4dfffa1dae0..92b225db59ac 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -66,8 +66,19 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
 	num_mmus = atomic_read(&kvm->online_vcpus) * 2;
 	tmp = krealloc(kvm->arch.nested_mmus,
 		       num_mmus * sizeof(*kvm->arch.nested_mmus),
-		       GFP_KERNEL | __GFP_ZERO);
+		       GFP_KERNEL_ACCOUNT | __GFP_ZERO);
 	if (tmp) {
+		/*
+		 * If we went through a realocation, adjust the MMU
+		 * back-pointers in the pg_table structures.
+		 */
+		if (kvm->arch.nested_mmus != tmp) {
+			int i;
+
+			for (i = 0; i < num_mms - 2; i++)
+				tmp[i].pgt->mmu = &tmp[i];
+		}
+
 		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
 		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
 			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: Use appropriate mmu pointer in stage2 page table init.
  2021-11-25 13:49   ` Marc Zyngier
@ 2021-11-26  5:45     ` Ganapatrao Kulkarni
  2021-11-26 10:50       ` Marc Zyngier
  0 siblings, 1 reply; 13+ messages in thread
From: Ganapatrao Kulkarni @ 2021-11-26  5:45 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, Quentin Perret, D Scott Phillips

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>

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.

Thanks,
Ganapat

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures
  2021-11-25 14:23   ` Marc Zyngier
@ 2021-11-26  5:59     ` Ganapatrao Kulkarni
  2021-11-26 15:28       ` Marc Zyngier
                         ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Ganapatrao Kulkarni @ 2021-11-26  5:59 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, D Scott Phillips

Hi Marc,

On 25-11-2021 07:53 pm, Marc Zyngier wrote:
> On Mon, 22 Nov 2021 09:58:03 +0000,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>> Commit 1776c91346b6 ("KVM: arm64: nv: Support multiple nested Stage-2 mmu
>> structures")[1] added a function kvm_vcpu_init_nested which expands the
>> stage-2 mmu structures array when ever a new vCPU is created. The array
>> is expanded using krealloc() and results in a stale mmu address pointer
>> in pgt->mmu. Adding a fix to update the pointer with the new address after
>> successful krealloc.
>>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/
>> branch kvm-arm64/nv-5.13
>>
>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>> ---
>>   arch/arm64/kvm/nested.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
>> index 4ffbc14d0245..57ad8d8f4ee5 100644
>> --- a/arch/arm64/kvm/nested.c
>> +++ b/arch/arm64/kvm/nested.c
>> @@ -68,6 +68,8 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>>   		       num_mmus * sizeof(*kvm->arch.nested_mmus),
>>   		       GFP_KERNEL | __GFP_ZERO);
>>   	if (tmp) {
>> +		int i;
>> +
>>   		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
>>   		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
>>   			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
>> @@ -80,6 +82,13 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>>   		}
>>   
>>   		kvm->arch.nested_mmus = tmp;
>> +
>> +		/* Fixup pgt->mmu after krealloc */
>> +		for (i = 0; i < kvm->arch.nested_mmus_size; i++) {
>> +			struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
>> +
>> +			mmu->pgt->mmu = mmu;
>> +		}
>>   	}
>>   
>>   	mutex_unlock(&kvm->lock);
> 
> Another good catch. I've tweaked a bit to avoid some unnecessary
> repainting, see below.
> 
> Thanks again,
> 
> 	M.
> 
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index a4dfffa1dae0..92b225db59ac 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -66,8 +66,19 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>   	num_mmus = atomic_read(&kvm->online_vcpus) * 2;
>   	tmp = krealloc(kvm->arch.nested_mmus,
>   		       num_mmus * sizeof(*kvm->arch.nested_mmus),
> -		       GFP_KERNEL | __GFP_ZERO);
> +		       GFP_KERNEL_ACCOUNT | __GFP_ZERO);
>   	if (tmp) {
> +		/*
> +		 * If we went through a realocation, adjust the MMU

Is it more precise to say?
> +		 * back-pointers in the pg_table structures.
* back-pointers in the pg_table structures of previous inits.

> +		 */
> +		if (kvm->arch.nested_mmus != tmp) {
> +			int i;
> +
> +			for (i = 0; i < num_mms - 2; i++)
> +				tmp[i].pgt->mmu = &tmp[i];
> +		}

Thanks for this optimization, it saves 2 redundant iterations.
> +
>   		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
>   		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
>   			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
> 

Feel free to add,
Reviewed-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>


Thanks,
Ganapat

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: Use appropriate mmu pointer in stage2 page table init.
  2021-11-26  5:45     ` Ganapatrao Kulkarni
@ 2021-11-26 10:50       ` Marc Zyngier
  2021-11-26 16:51         ` Ganapatrao Kulkarni
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Zyngier @ 2021-11-26 10:50 UTC (permalink / raw)
  To: Ganapatrao Kulkarni
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, Quentin Perret, D Scott Phillips

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures
  2021-11-26  5:59     ` Ganapatrao Kulkarni
@ 2021-11-26 15:28       ` Marc Zyngier
  2021-11-26 16:33       ` Marc Zyngier
  2021-11-26 19:20       ` Marc Zyngier
  2 siblings, 0 replies; 13+ messages in thread
From: Marc Zyngier @ 2021-11-26 15:28 UTC (permalink / raw)
  To: Ganapatrao Kulkarni
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, D Scott Phillips

On Fri, 26 Nov 2021 05:59:00 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> Hi Marc,
> 
> On 25-11-2021 07:53 pm, Marc Zyngier wrote:
> > On Mon, 22 Nov 2021 09:58:03 +0000,
> > Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> >> 
> >> Commit 1776c91346b6 ("KVM: arm64: nv: Support multiple nested Stage-2 mmu
> >> structures")[1] added a function kvm_vcpu_init_nested which expands the
> >> stage-2 mmu structures array when ever a new vCPU is created. The array
> >> is expanded using krealloc() and results in a stale mmu address pointer
> >> in pgt->mmu. Adding a fix to update the pointer with the new address after
> >> successful krealloc.
> >> 
> >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/
> >> branch kvm-arm64/nv-5.13
> >> 
> >> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> >> ---
> >>   arch/arm64/kvm/nested.c | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >> 
> >> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> >> index 4ffbc14d0245..57ad8d8f4ee5 100644
> >> --- a/arch/arm64/kvm/nested.c
> >> +++ b/arch/arm64/kvm/nested.c
> >> @@ -68,6 +68,8 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> >>   		       num_mmus * sizeof(*kvm->arch.nested_mmus),
> >>   		       GFP_KERNEL | __GFP_ZERO);
> >>   	if (tmp) {
> >> +		int i;
> >> +
> >>   		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
> >>   		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
> >>   			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
> >> @@ -80,6 +82,13 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> >>   		}
> >>     		kvm->arch.nested_mmus = tmp;
> >> +
> >> +		/* Fixup pgt->mmu after krealloc */
> >> +		for (i = 0; i < kvm->arch.nested_mmus_size; i++) {
> >> +			struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
> >> +
> >> +			mmu->pgt->mmu = mmu;
> >> +		}
> >>   	}
> >>     	mutex_unlock(&kvm->lock);
> > 
> > Another good catch. I've tweaked a bit to avoid some unnecessary
> > repainting, see below.
> > 
> > Thanks again,
> > 
> > 	M.
> > 
> > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > index a4dfffa1dae0..92b225db59ac 100644
> > --- a/arch/arm64/kvm/nested.c
> > +++ b/arch/arm64/kvm/nested.c
> > @@ -66,8 +66,19 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> >   	num_mmus = atomic_read(&kvm->online_vcpus) * 2;
> >   	tmp = krealloc(kvm->arch.nested_mmus,
> >   		       num_mmus * sizeof(*kvm->arch.nested_mmus),
> > -		       GFP_KERNEL | __GFP_ZERO);
> > +		       GFP_KERNEL_ACCOUNT | __GFP_ZERO);
> >   	if (tmp) {
> > +		/*
> > +		 * If we went through a realocation, adjust the MMU
> 
> Is it more precise to say?
> > +		 * back-pointers in the pg_table structures.
> * back-pointers in the pg_table structures of previous inits.

Yes. I have added something along those lines.

> > +		 */
> > +		if (kvm->arch.nested_mmus != tmp) {
> > +			int i;
> > +
> > +			for (i = 0; i < num_mms - 2; i++)
> > +				tmp[i].pgt->mmu = &tmp[i];
> > +		}
> 
> Thanks for this optimization, it saves 2 redundant iterations.
> > +
> >   		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
> >   		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
> >   			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
> > 
> 
> Feel free to add,
> Reviewed-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>

Given that this was a fixup, I haven't taken this tag. I will Cc you
on the whole series, and you can give you tag on the whole patch if
you are happy with it.

BTW, I have now fixed the bug that was preventing L2 userspace from
running (bad interaction with the pgtable code which was unhappy about
my use of the SW bits when relaxing the permissions). You should now
be able to test the whole series.

Thanks,

	M.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures
  2021-11-26  5:59     ` Ganapatrao Kulkarni
  2021-11-26 15:28       ` Marc Zyngier
@ 2021-11-26 16:33       ` Marc Zyngier
  2021-11-26 19:20       ` Marc Zyngier
  2 siblings, 0 replies; 13+ messages in thread
From: Marc Zyngier @ 2021-11-26 16:33 UTC (permalink / raw)
  To: Ganapatrao Kulkarni
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, D Scott Phillips

[Resending, as kernel.org just had a hickup]

On Fri, 26 Nov 2021 05:59:00 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> Hi Marc,
> 
> On 25-11-2021 07:53 pm, Marc Zyngier wrote:
> > On Mon, 22 Nov 2021 09:58:03 +0000,
> > Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> >> 
> >> Commit 1776c91346b6 ("KVM: arm64: nv: Support multiple nested Stage-2 mmu
> >> structures")[1] added a function kvm_vcpu_init_nested which expands the
> >> stage-2 mmu structures array when ever a new vCPU is created. The array
> >> is expanded using krealloc() and results in a stale mmu address pointer
> >> in pgt->mmu. Adding a fix to update the pointer with the new address after
> >> successful krealloc.
> >> 
> >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/
> >> branch kvm-arm64/nv-5.13
> >> 
> >> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> >> ---
> >>   arch/arm64/kvm/nested.c | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >> 
> >> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> >> index 4ffbc14d0245..57ad8d8f4ee5 100644
> >> --- a/arch/arm64/kvm/nested.c
> >> +++ b/arch/arm64/kvm/nested.c
> >> @@ -68,6 +68,8 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> >>   		       num_mmus * sizeof(*kvm->arch.nested_mmus),
> >>   		       GFP_KERNEL | __GFP_ZERO);
> >>   	if (tmp) {
> >> +		int i;
> >> +
> >>   		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
> >>   		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
> >>   			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
> >> @@ -80,6 +82,13 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> >>   		}
> >>     		kvm->arch.nested_mmus = tmp;
> >> +
> >> +		/* Fixup pgt->mmu after krealloc */
> >> +		for (i = 0; i < kvm->arch.nested_mmus_size; i++) {
> >> +			struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
> >> +
> >> +			mmu->pgt->mmu = mmu;
> >> +		}
> >>   	}
> >>     	mutex_unlock(&kvm->lock);
> > 
> > Another good catch. I've tweaked a bit to avoid some unnecessary
> > repainting, see below.
> > 
> > Thanks again,
> > 
> > 	M.
> > 
> > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > index a4dfffa1dae0..92b225db59ac 100644
> > --- a/arch/arm64/kvm/nested.c
> > +++ b/arch/arm64/kvm/nested.c
> > @@ -66,8 +66,19 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> >   	num_mmus = atomic_read(&kvm->online_vcpus) * 2;
> >   	tmp = krealloc(kvm->arch.nested_mmus,
> >   		       num_mmus * sizeof(*kvm->arch.nested_mmus),
> > -		       GFP_KERNEL | __GFP_ZERO);
> > +		       GFP_KERNEL_ACCOUNT | __GFP_ZERO);
> >   	if (tmp) {
> > +		/*
> > +		 * If we went through a realocation, adjust the MMU
> 
> Is it more precise to say?
> > +		 * back-pointers in the pg_table structures.
> * back-pointers in the pg_table structures of previous inits.

Yes. I have added something along those lines.

> > +		 */
> > +		if (kvm->arch.nested_mmus != tmp) {
> > +			int i;
> > +
> > +			for (i = 0; i < num_mms - 2; i++)
> > +				tmp[i].pgt->mmu = &tmp[i];
> > +		}
> 
> Thanks for this optimization, it saves 2 redundant iterations.
> > +
> >   		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
> >   		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
> >   			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
> > 
> 
> Feel free to add,
> Reviewed-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>

Given that this was a fixup, I haven't taken this tag. I will Cc you
on the whole series, and you can give you tag on the whole patch if
you are happy with it.

BTW, I have now fixed the bug that was preventing L2 userspace from
running (bad interaction with the pgtable code which was unhappy about
my use of the SW bits when relaxing the permissions). You should now
be able to test the whole series.

Thanks,

	M.

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

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: Use appropriate mmu pointer in stage2 page table init.
  2021-11-26 10:50       ` Marc Zyngier
@ 2021-11-26 16:51         ` Ganapatrao Kulkarni
  0 siblings, 0 replies; 13+ messages in thread
From: Ganapatrao Kulkarni @ 2021-11-26 16:51 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, Quentin Perret, D Scott Phillips



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures
  2021-11-26  5:59     ` Ganapatrao Kulkarni
  2021-11-26 15:28       ` Marc Zyngier
  2021-11-26 16:33       ` Marc Zyngier
@ 2021-11-26 19:20       ` Marc Zyngier
  2021-11-29  6:00         ` Ganapatrao Kulkarni
  2 siblings, 1 reply; 13+ messages in thread
From: Marc Zyngier @ 2021-11-26 19:20 UTC (permalink / raw)
  To: Ganapatrao Kulkarni
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, D Scott Phillips

[resending after having sorted my email config]

On Fri, 26 Nov 2021 05:59:00 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> Hi Marc,
> 
> On 25-11-2021 07:53 pm, Marc Zyngier wrote:
> > On Mon, 22 Nov 2021 09:58:03 +0000,
> > Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> >> 
> >> Commit 1776c91346b6 ("KVM: arm64: nv: Support multiple nested Stage-2 mmu
> >> structures")[1] added a function kvm_vcpu_init_nested which expands the
> >> stage-2 mmu structures array when ever a new vCPU is created. The array
> >> is expanded using krealloc() and results in a stale mmu address pointer
> >> in pgt->mmu. Adding a fix to update the pointer with the new address after
> >> successful krealloc.
> >> 
> >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/
> >> branch kvm-arm64/nv-5.13
> >> 
> >> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> >> ---
> >>   arch/arm64/kvm/nested.c | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >> 
> >> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> >> index 4ffbc14d0245..57ad8d8f4ee5 100644
> >> --- a/arch/arm64/kvm/nested.c
> >> +++ b/arch/arm64/kvm/nested.c
> >> @@ -68,6 +68,8 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> >>   		       num_mmus * sizeof(*kvm->arch.nested_mmus),
> >>   		       GFP_KERNEL | __GFP_ZERO);
> >>   	if (tmp) {
> >> +		int i;
> >> +
> >>   		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
> >>   		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
> >>   			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
> >> @@ -80,6 +82,13 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> >>   		}
> >>     		kvm->arch.nested_mmus = tmp;
> >> +
> >> +		/* Fixup pgt->mmu after krealloc */
> >> +		for (i = 0; i < kvm->arch.nested_mmus_size; i++) {
> >> +			struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
> >> +
> >> +			mmu->pgt->mmu = mmu;
> >> +		}
> >>   	}
> >>     	mutex_unlock(&kvm->lock);
> > 
> > Another good catch. I've tweaked a bit to avoid some unnecessary
> > repainting, see below.
> > 
> > Thanks again,
> > 
> > 	M.
> > 
> > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > index a4dfffa1dae0..92b225db59ac 100644
> > --- a/arch/arm64/kvm/nested.c
> > +++ b/arch/arm64/kvm/nested.c
> > @@ -66,8 +66,19 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> >   	num_mmus = atomic_read(&kvm->online_vcpus) * 2;
> >   	tmp = krealloc(kvm->arch.nested_mmus,
> >   		       num_mmus * sizeof(*kvm->arch.nested_mmus),
> > -		       GFP_KERNEL | __GFP_ZERO);
> > +		       GFP_KERNEL_ACCOUNT | __GFP_ZERO);
> >   	if (tmp) {
> > +		/*
> > +		 * If we went through a realocation, adjust the MMU
> 
> Is it more precise to say?
> > +		 * back-pointers in the pg_table structures.
> * back-pointers in the pg_table structures of previous inits.

Yes. I have added something along those lines.

> > +		 */
> > +		if (kvm->arch.nested_mmus != tmp) {
> > +			int i;
> > +
> > +			for (i = 0; i < num_mms - 2; i++)
> > +				tmp[i].pgt->mmu = &tmp[i];
> > +		}
> 
> Thanks for this optimization, it saves 2 redundant iterations.
> > +
> >   		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
> >   		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
> >   			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
> > 
> 
> Feel free to add,
> Reviewed-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>

Given that this was a fixup, I haven't taken this tag. I will Cc you
on the whole series, and you can give you tag on the whole patch if
you are happy with it.

BTW, I have now fixed the bug that was preventing L2 userspace from
running (bad interaction with the pgtable code which was unhappy about
my use of the SW bits when relaxing the permissions). You should now
be able to test the whole series.

Thanks,

	M.

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

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures
  2021-11-26 19:20       ` Marc Zyngier
@ 2021-11-29  6:00         ` Ganapatrao Kulkarni
  0 siblings, 0 replies; 13+ messages in thread
From: Ganapatrao Kulkarni @ 2021-11-29  6:00 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: catalin.marinas, will, andre.przywara, linux-arm-kernel, kvmarm,
	kvm, darren, D Scott Phillips


On 27-11-2021 12:50 am, Marc Zyngier wrote:
> [resending after having sorted my email config]
> 
> On Fri, 26 Nov 2021 05:59:00 +0000,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>> Hi Marc,
>>
>> On 25-11-2021 07:53 pm, Marc Zyngier wrote:
>>> On Mon, 22 Nov 2021 09:58:03 +0000,
>>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>>
>>>> Commit 1776c91346b6 ("KVM: arm64: nv: Support multiple nested Stage-2 mmu
>>>> structures")[1] added a function kvm_vcpu_init_nested which expands the
>>>> stage-2 mmu structures array when ever a new vCPU is created. The array
>>>> is expanded using krealloc() and results in a stale mmu address pointer
>>>> in pgt->mmu. Adding a fix to update the pointer with the new address after
>>>> successful krealloc.
>>>>
>>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/
>>>> branch kvm-arm64/nv-5.13
>>>>
>>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>>> ---
>>>>    arch/arm64/kvm/nested.c | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
>>>> index 4ffbc14d0245..57ad8d8f4ee5 100644
>>>> --- a/arch/arm64/kvm/nested.c
>>>> +++ b/arch/arm64/kvm/nested.c
>>>> @@ -68,6 +68,8 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>>>>    		       num_mmus * sizeof(*kvm->arch.nested_mmus),
>>>>    		       GFP_KERNEL | __GFP_ZERO);
>>>>    	if (tmp) {
>>>> +		int i;
>>>> +
>>>>    		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
>>>>    		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
>>>>    			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
>>>> @@ -80,6 +82,13 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>>>>    		}
>>>>      		kvm->arch.nested_mmus = tmp;
>>>> +
>>>> +		/* Fixup pgt->mmu after krealloc */
>>>> +		for (i = 0; i < kvm->arch.nested_mmus_size; i++) {
>>>> +			struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
>>>> +
>>>> +			mmu->pgt->mmu = mmu;
>>>> +		}
>>>>    	}
>>>>      	mutex_unlock(&kvm->lock);
>>>
>>> Another good catch. I've tweaked a bit to avoid some unnecessary
>>> repainting, see below.
>>>
>>> Thanks again,
>>>
>>> 	M.
>>>
>>> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
>>> index a4dfffa1dae0..92b225db59ac 100644
>>> --- a/arch/arm64/kvm/nested.c
>>> +++ b/arch/arm64/kvm/nested.c
>>> @@ -66,8 +66,19 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>>>    	num_mmus = atomic_read(&kvm->online_vcpus) * 2;
>>>    	tmp = krealloc(kvm->arch.nested_mmus,
>>>    		       num_mmus * sizeof(*kvm->arch.nested_mmus),
>>> -		       GFP_KERNEL | __GFP_ZERO);
>>> +		       GFP_KERNEL_ACCOUNT | __GFP_ZERO);
>>>    	if (tmp) {
>>> +		/*
>>> +		 * If we went through a realocation, adjust the MMU
>>
>> Is it more precise to say?
>>> +		 * back-pointers in the pg_table structures.
>> * back-pointers in the pg_table structures of previous inits.
> 
> Yes. I have added something along those lines.
> 
>>> +		 */
>>> +		if (kvm->arch.nested_mmus != tmp) {
>>> +			int i;
>>> +
>>> +			for (i = 0; i < num_mms - 2; i++)
>>> +				tmp[i].pgt->mmu = &tmp[i];
>>> +		}
>>
>> Thanks for this optimization, it saves 2 redundant iterations.
>>> +
>>>    		if (kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 1]) ||
>>>    		    kvm_init_stage2_mmu(kvm, &tmp[num_mmus - 2])) {
>>>    			kvm_free_stage2_pgd(&tmp[num_mmus - 1]);
>>>
>>
>> Feel free to add,
>> Reviewed-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> 
> Given that this was a fixup, I haven't taken this tag. I will Cc you

no problem, makes sense to fold this in to original patch.

> on the whole series, and you can give you tag on the whole patch if
> you are happy with it.

Sure.
> 
> BTW, I have now fixed the bug that was preventing L2 userspace from
> running (bad interaction with the pgtable code which was unhappy about
> my use of the SW bits when relaxing the permissions). You should now
> be able to test the whole series.

Yes, I have rebased to latest branch kvm-arm64/nv-5.16 and I am able to 
boot L1 and L2.

> 
> Thanks,
> 
> 	M.
> 

Thanks,
Ganapat

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-11-29  6:04 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-22  9:58 [PATCH 0/2] KVM: arm64: nv: Fix issue with Stage 2 MMU init for Nested case Ganapatrao Kulkarni
2021-11-22  9:58 ` [PATCH 1/2] KVM: arm64: Use appropriate mmu pointer in stage2 page table init Ganapatrao Kulkarni
2021-11-25 13:49   ` Marc Zyngier
2021-11-26  5:45     ` Ganapatrao Kulkarni
2021-11-26 10:50       ` Marc Zyngier
2021-11-26 16:51         ` Ganapatrao Kulkarni
2021-11-22  9:58 ` [PATCH 2/2] KVM: arm64: nv: fixup! Support multiple nested Stage-2 mmu structures Ganapatrao Kulkarni
2021-11-25 14:23   ` Marc Zyngier
2021-11-26  5:59     ` Ganapatrao Kulkarni
2021-11-26 15:28       ` Marc Zyngier
2021-11-26 16:33       ` Marc Zyngier
2021-11-26 19:20       ` Marc Zyngier
2021-11-29  6:00         ` Ganapatrao Kulkarni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).