Re: [PATCH v2 01/17] KVM: arm64: Factor out stage 2 page table data from struct kvm

From: Marc Zyngier <maz@kernel.org>
To: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	kernel-team@android.com, kvm@vger.kernel.org,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Jintack Lim <jintack@cs.columbia.edu>,
	Andre Przywara <andre.przywara@arm.com>,
	Christoffer Dall <christoffer.dall@arm.com>,
	kvmarm@lists.cs.columbia.edu,
	George Cherian <gcherian@marvell.com>,
	James Morse <james.morse@arm.com>,
	Andrew Scull <ascull@google.com>,
	"Zengtao \(B\)" <prime.zeng@hisilicon.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Julien Thierry <julien.thierry.kdev@gmail.com>,
	Will Deacon <will@kernel.org>, Dave Martin <Dave.Martin@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 01/17] KVM: arm64: Factor out stage 2 page table data from struct kvm
Date: Tue, 16 Jun 2020 17:18:11 +0100	[thread overview]
Message-ID: <9c0044564885d3356f76b55f35426987@kernel.org> (raw)
In-Reply-To: <17d37bde-2fc8-d165-ee02-7640fc561167@arm.com>

Hi Alexandru,

On 2020-06-16 16:59, Alexandru Elisei wrote:
> Hi,
> 
> IMO, this patch does two different things: adds a new structure, 
> kvm_s2_mmu, and
> converts the memory management code to use the 4 level page table API. 
> I realize
> it's painful to convert the MMU code to use the p4d functions, and then 
> modify
> everything to use kvm_s2_mmu in a separate patch, but I believe
> splitting it into
> 2 would be better in the long run. The resulting patches will be
> smaller and both
> will have a better chance of being reviewed by the right people.

I'm not sure how you want me to do this. The whole p4d mess is already 
in mainline (went via akpm directly to Linus), and I can't really revert 
it.

> Either way, there were still some suggestions left over from v1, I 
> don't know if
> they were were too minor/subjective to implement, or they were 
> overlooked. I'll
> re-post them here and I'll try to review the patch again once I figure
> out how the  p4d changes fit in.

Sorry, I must have dropped the ball on your comments.

> 
> On 6/15/20 2:27 PM, Marc Zyngier wrote:
>> From: Christoffer Dall <christoffer.dall@arm.com>
>> 
>> As we are about to reuse our stage 2 page table manipulation code for
>> shadow stage 2 page tables in the context of nested virtualization, we
>> are going to manage multiple stage 2 page tables for a single VM.
>> 
>> This requires some pretty invasive changes to our data structures,
>> which moves the vmid and pgd pointers into a separate structure and
>> change pretty much all of our mmu code to operate on this structure
>> instead.
>> 
>> The new structure is called struct kvm_s2_mmu.
>> 
>> There is no intended functional change by this patch alone.
>> 
>> Reviewed-by: James Morse <james.morse@arm.com>
>> [Designed data structure layout in collaboration]
>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>> Co-developed-by: Marc Zyngier <maz@kernel.org>
>> [maz: Moved the last_vcpu_ran down to the S2 MMU structure as well]
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/include/asm/kvm_asm.h  |   7 +-
>>  arch/arm64/include/asm/kvm_host.h |  32 +++-
>>  arch/arm64/include/asm/kvm_mmu.h  |  16 +-
>>  arch/arm64/kvm/arm.c              |  36 ++--
>>  arch/arm64/kvm/hyp/switch.c       |   8 +-
>>  arch/arm64/kvm/hyp/tlb.c          |  52 +++---
>>  arch/arm64/kvm/mmu.c              | 278 
>> +++++++++++++++++-------------
>>  7 files changed, 233 insertions(+), 196 deletions(-)
>> 
>> [..]
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 90cb90561446..360396ecc6d3 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
> 
> There's still one comment in the file that refers to arch.vmid:
> 
> static bool need_new_vmid_gen(struct kvm_vmid *vmid)
> {
>     u64 current_vmid_gen = atomic64_read(&kvm_vmid_gen);
>     smp_rmb(); /* Orders read of kvm_vmid_gen and kvm->arch.vmid */
>     return unlikely(READ_ONCE(vmid->vmid_gen) != current_vmid_gen);
> }
> 
> The comment could be rephrased to remove the reference to
> kvm->arch.vmid: "Orders
> read of kvm_vmid_gen and kvm_s2_mmu->vmid".

Yup, definitely useful.

> 
>> [..]
>> 
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index 8c0035cab6b6..4a4437be4bc5 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> 
>> [..]
>> 
>>  /**
>> - * kvm_alloc_stage2_pgd - allocate level-1 table for stage-2 
>> translation.
>> - * @kvm:	The KVM struct pointer for the VM.
>> + * kvm_init_stage2_mmu - Initialise a S2 MMU strucrure
>> + * @kvm:	The pointer to the KVM structure
>> + * @mmu:	The pointer to the s2 MMU structure
>>   *
>>   * Allocates only the stage-2 HW PGD level table(s) of size defined 
>> by
>> - * stage2_pgd_size(kvm).
>> + * stage2_pgd_size(mmu->kvm).
>>   *
>>   * Note we don't need locking here as this is only called when the VM 
>> is
>>   * created, which can only be done once.
>>   */
>> -int kvm_alloc_stage2_pgd(struct kvm *kvm)
>> +int kvm_init_stage2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
>>  {
>>  	phys_addr_t pgd_phys;
>>  	pgd_t *pgd;
>> +	int cpu;
>> 
>> -	if (kvm->arch.pgd != NULL) {
>> +	if (mmu->pgd != NULL) {
>>  		kvm_err("kvm_arch already initialized?\n");
>>  		return -EINVAL;
>>  	}
>> @@ -1024,8 +1040,20 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
>>  	if (WARN_ON(pgd_phys & ~kvm_vttbr_baddr_mask(kvm)))
>>  		return -EINVAL;
> 
> We don't free the pgd if we get the error above, but we do free it 
> below, if
> allocating last_vcpu_ran fails. Shouldn't we free it in both cases?

Worth investigating. This code gets majorly revamped in the NV series, 
so it is likely that I missed something in the middle.

Thanks for the heads up!

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel