Re: [PATCH 03/26] KVM: arm64: Factor out stage 2 page table data from struct kvm

From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	Andre Przywara <andre.przywara@arm.com>,
	Christoffer Dall <christoffer.dall@arm.com>,
	Dave Martin <Dave.Martin@arm.com>,
	Jintack Lim <jintack@cs.columbia.edu>,
	George Cherian <gcherian@marvell.com>,
	"Zengtao (B)" <prime.zeng@hisilicon.com>,
	Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Julien Thierry <julien.thierry.kdev@gmail.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>
Subject: Re: [PATCH 03/26] KVM: arm64: Factor out stage 2 page table data from struct kvm
Date: Wed, 27 May 2020 09:45:37 +0100	[thread overview]
Message-ID: <91b6b062-e39d-a672-52e4-40532068a02e@arm.com> (raw)
In-Reply-To: <ea603b3a7a51a597263e7c8152f4c795@kernel.org>

Hi Marc,

On 5/27/20 9:41 AM, Marc Zyngier wrote:
> Hi Alex,
>
> On 2020-05-12 17:53, Alexandru Elisei wrote:
>> Hi,
>>
>> On 5/12/20 12:17 PM, James Morse wrote:
>>> Hi Alex, Marc,
>>>
>>> (just on this last_vcpu_ran thing...)
>>>
>>> On 11/05/2020 17:38, Alexandru Elisei wrote:
>>>> On 4/22/20 1:00 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.
>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>>>> b/arch/arm64/include/asm/kvm_host.h
>>>>> index 7dd8fefa6aecd..664a5d92ae9b8 100644
>>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>>> @@ -63,19 +63,32 @@ struct kvm_vmid {
>>>>>      u32    vmid;
>>>>>  };
>>>>>
>>>>> -struct kvm_arch {
>>>>> +struct kvm_s2_mmu {
>>>>>      struct kvm_vmid vmid;
>>>>>
>>>>> -    /* stage2 entry level table */
>>>>> -    pgd_t *pgd;
>>>>> -    phys_addr_t pgd_phys;
>>>>> -
>>>>> -    /* VTCR_EL2 value for this VM */
>>>>> -    u64    vtcr;
>>>>> +    /*
>>>>> +     * stage2 entry level table
>>>>> +     *
>>>>> +     * Two kvm_s2_mmu structures in the same VM can point to the same pgd
>>>>> +     * here.  This happens when running a non-VHE guest hypervisor which
>>>>> +     * uses the canonical stage 2 page table for both vEL2 and for vEL1/0
>>>>> +     * with vHCR_EL2.VM == 0.
>>>> It makes more sense to me to say that a non-VHE guest hypervisor will use the
>>>> canonical stage *1* page table when running at EL2
>>> Can KVM say anything about stage1? Its totally under the the guests control
>>> even at vEL2...
>>
>> It just occurred to me that "canonical stage 2 page table" refers to the L0
>> hypervisor stage 2, not to the L1 hypervisor stage 2. If you don't mind my
>> suggestion, perhaps the comment can be slightly improved to avoid any confusion?
>> Maybe something along the lines of "[..] This happens when running a
>> non-VHE guest
>> hypervisor, in which case we use the canonical stage 2 page table for both vEL2
>> and for vEL1/0 with vHCR_EL2.VM == 0".
>
> If the confusion stems from the lack of guest stage-2, how about:
>
> "This happens when running a guest using a translation regime that isn't
>  affected by its own stage-2 translation, such as a non-VHE hypervisor
>  running at vEL2, or for vEL1/EL0 with vHCR_EL2.VM == 0. In that case,
>  we use the canonical stage-2 page tables."
>
> instead? Does this lift the ambiguity?

Yes, that's perfect.

Thanks,
Alex
>
> Thanks,
>
>         M.