Re: [PATCH RFC] KVM: arm64: Sidestep stage2_unmap_vm() on vcpu reset when S2FWB is supported

[Alexandru Elisei]

Hi Marc,

On 5/30/20 5:31 PM, Marc Zyngier wrote:
> Hi Alex,
>
> On 2020-05-30 11:46, Alexandru Elisei wrote:
>> Hi,
>
> [...]
>
>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>> index 48d0ec44ad77..e6378162cdef 100644
>>>> --- a/virt/kvm/arm/arm.c
>>>> +++ b/virt/kvm/arm/arm.c
>>>> @@ -983,8 +983,11 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu
>>>> *vcpu,
>>>> ÂÂÂÂ /*
>>>> ÂÂÂÂÂ * Ensure a rebooted VM will fault in RAM pages and detect if the
>>>> ÂÂÂÂÂ * guest MMU is turned off and flush the caches as needed.
>>>> +ÂÂÂÂ *
>>>> +ÂÂÂÂ * S2FWB enforces all memory accesses to RAM being cacheable, we
>>>> +ÂÂÂÂ * ensure that the cache is always coherent.
>>>> ÂÂÂÂÂ */
>>>> -ÂÂÂ if (vcpu->arch.has_run_once)
>>>> +ÂÂÂ if (vcpu->arch.has_run_once && !cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
>>> I think userspace does not invalidate the icache when loading a new kernel image,
>>> and if the guest patched instructions, they could potentially still be in the
>>> icache. Should the icache be invalidated if FWB is present?
>>
>> I noticed that this was included in the current pull request and I
>> remembered that
>> I wasn't sure about this part. Did some more digging and it turns out that FWB
>> implies no cache maintenance needed for *data to instruction*
>> coherence. From ARM
>> DDI 0487F.b, page D5-2635:
>>
>> "When ARMv8.4-S2FWB is implemented, the architecture requires that
>> CLIDR_EL1.{LOUU, LOIUS} are zero so that no levels of data cache need to be
>> cleaned in order to manage coherency with instruction fetches".
>>
>> However, there's no mention that I found for instruction to data coherence,
>> meaning that the icache would still need to be invalidated on each vcpu in order
>> to prevent fetching of patched instructions from the icache. Am I
>> missing something?
>
> I think you are right, and this definitely matches the way we deal with
> the icache on the fault path. For some bizarre reason, I always assume
> that FWB implies DIC, which isn't true at all.
>
> I'm planning to address it as follows. Please let me know what you think.
>
> Thanks,
>
> ÂÂÂÂÂÂÂ M.
>
> From f7860d1d284f41afea176cc17e5c9d895ae665e9 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@xxxxxxxxxx>
> Date: Sat, 30 May 2020 17:22:19 +0100
> Subject: [PATCH] KVM: arm64: Flush the instruction cache if not unmapping the
> ÂVM on reboot
>
> On a system with FWB, we don't need to unmap Stage-2 on reboot,
> as even if userspace takes this opportunity to repaint the whole
> of memory, FWB ensures that the data side stays consistent even
> if the guest uses non-cacheable mappings.
>
> However, the I-side is not necessarily coherent with the D-side
> if CTR_EL0.DIC is 0. In this case, invalidate the i-cache to
> preserve coherency.
>
> Reported-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> Fixes: 892713e97ca1 ("KVM: arm64: Sidestep stage2_unmap_vm() on vcpu reset when
> S2FWB is supported")
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> ---
> Âarch/arm64/kvm/arm.c | 14 ++++++++++----
> Â1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index b0b569f2cdd0..d6988401c22a 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -989,11 +989,17 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu
> *vcpu,
> ÂÂÂÂÂ * Ensure a rebooted VM will fault in RAM pages and detect if the
> ÂÂÂÂÂ * guest MMU is turned off and flush the caches as needed.
> ÂÂÂÂÂ *
> -ÂÂÂÂ * S2FWB enforces all memory accesses to RAM being cacheable, we
> -ÂÂÂÂ * ensure that the cache is always coherent.
> +ÂÂÂÂ * S2FWB enforces all memory accesses to RAM being cacheable,
> +ÂÂÂÂ * ensuring that the data side is always coherent. We still
> +ÂÂÂÂ * need to invalidate the I-cache though, as FWB does *not*
> +ÂÂÂÂ * imply CTR_EL0.DIC.
> ÂÂÂÂÂ */
> -ÂÂÂ if (vcpu->arch.has_run_once && !cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))
> -ÂÂÂÂÂÂÂ stage2_unmap_vm(vcpu->kvm);
> +ÂÂÂ if (vcpu->arch.has_run_once) {
> +ÂÂÂÂÂÂÂ if (!cpus_have_final_cap(ARM64_HAS_STAGE2_FWB))
> +ÂÂÂÂÂÂÂÂÂÂÂ stage2_unmap_vm(vcpu->kvm);
> +ÂÂÂÂÂÂÂ else
> +ÂÂÂÂÂÂÂÂÂÂÂ __flush_icache_all();
> +ÂÂÂ }
>
> ÂÂÂÂ vcpu_reset_hcr(vcpu);
>
>
Looks good, __flush_icache_all checks CTR_EL0.DIC before doing icache maintenance:

Reviewed-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>