From: zhukeqian <zhukeqian1@huawei.com>
To: Marc Zyngier <maz@kernel.org>,
Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Zenghui Yu <yuzenghui@huawei.com>, <kvmarm@lists.cs.columbia.edu>,
<linux-arm-kernel@lists.infradead.org>,
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC] KVM: arm64: Sidestep stage2_unmap_vm() on vcpu reset when S2FWB is supported
Date: Mon, 1 Jun 2020 14:26:04 +0800 [thread overview]
Message-ID: <d72e4299-e9f7-064c-98c7-621721e5671c@huawei.com> (raw)
In-Reply-To: <13db879dff56d091f98f7c5416ab1535@kernel.org>
Hi Marc,
On 2020/5/31 0:31, 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@kernel.org>
> 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@arm.com>
> Fixes: 892713e97ca1 ("KVM: arm64: Sidestep stage2_unmap_vm() on vcpu reset when S2FWB is supported")
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> 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();
After I looking into this function, I think it's OK here. Please ignore
my question :-).
> + }
>
> vcpu_reset_hcr(vcpu);
>
Thanks,
Keqian
next prev parent reply other threads:[~2020-06-01 6:26 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-15 7:28 [PATCH RFC] KVM: arm64: Sidestep stage2_unmap_vm() on vcpu reset when S2FWB is supported Zenghui Yu
2020-04-20 16:10 ` Alexandru Elisei
2020-05-30 10:46 ` Alexandru Elisei
2020-05-30 16:31 ` Marc Zyngier
2020-05-31 9:12 ` Alexandru Elisei
2020-06-01 6:26 ` zhukeqian [this message]
2020-06-01 3:24 ` Zenghui Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d72e4299-e9f7-064c-98c7-621721e5671c@huawei.com \
--to=zhukeqian1@huawei.com \
--cc=alexandru.elisei@arm.com \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=yuzenghui@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).