From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:55:37
The main purpose of this series is differentiate between "halt" and a more
generic "block", where "halt" aligns with x86's HLT instruction, the
halt-polling mechanisms, and associated stats, and "block" means any guest
action that causes the vCPU to block/wait.
This series arose out of a discussion over adding a stat to track if a
vCPU is blocked/halted[*]. The TL;DR of the discussion is that x86 has
several non-halt "wait" states, and arguably those states should not
participate in halt-polling. In practice, it really doesn't matter from
a functionality perspective because there are typically so few occurences
of the non-halt waits that they're in the noise compared to the number of
actual HLTs, especially for a long-running VM. So, my justification for
the rename is that because it doesn't truly affect functionality, KVM
might as well be technically correct and only use halt-polling for HLT.
The other annoyance this series addresses is that KVM mixes "halt" and
"block", e.g. the existing function is kvm_vcpu_block(), but all the stats
and the tracepoint use "halt". Ideally, KVM would probably avoid "block"
altogether as people often think of "blocked" as meaning the vCPU is
blocked due to _host_ activity. But I don't have a better alternative,
e.g. "halt" is obviously taken, "wait" is equivalent to "halt" on arm64,
"stop" has specific meaning on s390, etc... I tried to address the host
vs. guest issue by naming the new stat "blocking" instead of "blocked",
e.g. to convey that the vCPU is "actively blocking" instead of "being
blocked".
Patch 01 fixes a theoretical, benign s390 bug, and sets the stage for
additional cleanups.
Patches 02-04 reconcile discrepancies in when KVM considers halt-polling
to be "successful". Some stats consider it a success so long as KVM
doesn't schedule() away, others consider it a success if and only if a
wake event is detected in the halt-polling loop.
Patches 05-06 are prep cleanup to split out the core "block" routine.
Patch 07 is more prep, and should also be a small perf optimization for
halt-polling on arm64.
Patch 08 is x86 cleanup to free up the name kvm_vcpu_halt().
Patches 09-10 rename the existing kvm_vcpu_block() to kvm_vcpu_halt(), and
split out the core "block" routine to a new helper.
Patches 11-12 are minor cleanups to avoid unnecessary ktime_get().
Patches 13-14 convert non-HLT x86 flows to use kvm_vcpu_block().
[*] https://lkml.kernel.org/r/20210817230508.142907-1-jingzhangos@google.com
Jing Zhang (1):
KVM: stats: Add stat to detect if vcpu is currently blocking
Sean Christopherson (13):
KVM: s390: Ensure kvm_arch_no_poll() is read once when blocking vCPU
KVM: Update halt-polling stats if and only if halt-polling was
attempted
KVM: Refactor and document halt-polling stats update helper
KVM: Reconcile discrepancies in halt-polling stats
KVM: s390: Clear valid_wakeup in kvm_s390_handle_wait(), not in arch
hook
KVM: Drop obsolete kvm_arch_vcpu_block_finish()
KVM: Don't block+unblock when halt-polling is successful
KVM: x86: Tweak halt emulation helper names to free up kvm_vcpu_halt()
KVM: Rename kvm_vcpu_block() => kvm_vcpu_halt()
KVM: Split out a kvm_vcpu_block() helper from kvm_vcpu_halt()
KVM: Don't redo ktime_get() when calculating halt-polling
stop/deadline
KVM: x86: Directly block (instead of "halting") UNINITIALIZED vCPUs
KVM: x86: Invoke kvm_vcpu_block() directly for non-HALTED wait states
arch/arm64/include/asm/kvm_host.h | 1 -
arch/arm64/kvm/arch_timer.c | 2 +-
arch/arm64/kvm/handle_exit.c | 4 +-
arch/arm64/kvm/psci.c | 2 +-
arch/mips/include/asm/kvm_host.h | 1 -
arch/mips/kvm/emulate.c | 2 +-
arch/powerpc/include/asm/kvm_host.h | 1 -
arch/powerpc/kvm/book3s_pr.c | 2 +-
arch/powerpc/kvm/book3s_pr_papr.c | 2 +-
arch/powerpc/kvm/booke.c | 2 +-
arch/powerpc/kvm/powerpc.c | 2 +-
arch/s390/include/asm/kvm_host.h | 2 -
arch/s390/kvm/interrupt.c | 3 +-
arch/s390/kvm/kvm-s390.c | 7 +-
arch/x86/include/asm/kvm_host.h | 4 +-
arch/x86/kvm/vmx/nested.c | 2 +-
arch/x86/kvm/vmx/vmx.c | 4 +-
arch/x86/kvm/x86.c | 25 ++++--
include/linux/kvm_host.h | 6 +-
include/linux/kvm_types.h | 1 +
virt/kvm/kvm_main.c | 131 +++++++++++++++++-----------
21 files changed, 118 insertions(+), 88 deletions(-)
--
2.33.0.685.g46640cef36-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:55:41
Don't update halt-polling stats if halt-polling wasn't attempted. This
is a nop as @poll_ns is guaranteed to be '0' (poll_end == start), but it
will allow a future patch to move the histogram stats into the helper to
resolve a discrepancy in what is considered a "successful" halt-poll.
No functional change intended.
Cc: David Matlack <dmatlack@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/kvm_main.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:55:48
Wrap s390's halt_poll_max_steal with READ_ONCE and snapshot the result of
kvm_arch_no_poll() in kvm_vcpu_block() to avoid a mostly-theoretical,
largely benign bug on s390 where the result of kvm_arch_no_poll() could
change due to userspace modifying halt_poll_max_steal while the vCPU is
blocking. The bug is largely benign as it will either cause KVM to skip
updating halt-polling times (no_poll toggles false=>true) or to update
halt-polling times with a slightly flawed block_ns.
Note, READ_ONCE is unnecessary in the current code, add it in case the
arch hook is ever inlined, and to provide a hint that userspace can
change the param at will.
Fixes: 8b905d28ee17 ("KVM: s390: provide kvm_arch_no_poll function")
Cc: Christian Borntraeger <redacted>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/s390/kvm/kvm-s390.c | 2 +-
virt/kvm/kvm_main.c | 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
@@ -3446,7 +3446,7 @@ bool kvm_arch_no_poll(struct kvm_vcpu *vcpu){/* do not poll with more than halt_poll_max_steal percent of steal time */if(S390_lowcore.avg_steal_timer*100/(TICK_USEC<<12)>=-halt_poll_max_steal){+READ_ONCE(halt_poll_max_steal)){vcpu->stat.halt_no_poll_steal++;returntrue;}
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:55:48
Add a comment to document that halt-polling is considered successful even
if the polling loop itself didn't detect a wake event, i.e. if a wake
event was detect in the final kvm_vcpu_check_block(). Invert the param
to the update helper so that the helper is a dumb function that is "told"
whether or not polling was successful, as opposed to having it determinine
success/failure based on blocking behavior.
Opportunistically tweak the params to the update helper to reduce the
line length for the call site so that it fits on a single line, and so
that the prototype conforms to the more traditional kernel style.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/kvm_main.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:55:51
Move the halt-polling "success" and histogram stats update into the
dedicated helper to fix a discrepancy where the success/fail "time" stats
consider polling successful so long as the wait is avoided, but the main
"success" and histogram stats consider polling successful if and only if
a wake event was detected by the halt-polling loop.
Move halt_attempted_poll to the helper as well so that all the stats are
updated in a single location. While it's a bit odd to update the stat
well after the fact, practically speaking there's no meaningful advantage
to updating before polling.
Note, there is a functional change in addition to the success vs. fail
change. The histogram updates previously called ktime_get() instead of
using "cur". But that change is desirable as it means all the stats are
now updated with the same polling time, and avoids the extra ktime_get(),
which isn't expensive but isn't free either.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/kvm_main.c | 35 ++++++++++++++++-------------------
1 file changed, 16 insertions(+), 19 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:55:54
Move the clearing of valid_wakeup out of kvm_arch_vcpu_block_finish() so
that a future patch can drop said arch hook. Unlike the other blocking-
related arch hooks (vcpu_blocking/unblocking()), vcpu_block_finish() needs
to be called even if the KVM doesn't actually block the vCPU. This will
allow future patches to differentiate between truly blocking the vCPU and
emulating a halt condition without introducing a contradiction.
Alternatively, the hook could be renamed to kvm_arch_vcpu_halt_finish(),
but there's literally one call site in s390, and future cleanup can also
be done to handle valid_wakeup fully within kvm_s390_handle_wait() and
allow generic KVM to drop vcpu_valid_wakeup().
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/s390/kvm/interrupt.c | 1 +
arch/s390/kvm/kvm-s390.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:56:03
Invoke the arch hooks for block+unblock if and only if KVM actually
attempts to block the vCPU. The only non-nop implementation is on arm64,
and if halt-polling is successful, there is no need for arm64 to put/load
the vGIC as KVM hasn't relinquished control of the vCPU in any way.
The primary motivation is to allow future cleanup to split out "block"
from "halt", but this is also likely a small performance boost on arm64
when halt-polling is successful.
Adjust the post-block path to update "cur" after unblocking, i.e. include
vGIC load time in halt_wait_ns and halt_wait_hist, so that the behavior
is consistent. Moving just the pre-block arch hook would result in only
the vGIC put latency being included in the halt_wait stats. There is no
obvious evidence that one way or the other is correct, so just ensure KVM
is consistent.
Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <redacted>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/kvm_main.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:56:31
Rename a variety of HLT-related helpers to free up the function name
"kvm_vcpu_halt" for future use in generic KVM code, e.g. to differentiate
between "block" and "halt".
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/vmx/nested.c | 2 +-
arch/x86/kvm/vmx/vmx.c | 4 ++--
arch/x86/kvm/x86.c | 13 +++++++------
4 files changed, 11 insertions(+), 10 deletions(-)
@@ -8655,11 +8655,11 @@ static int __kvm_vcpu_halt(struct kvm_vcpu *vcpu, int state, int reason)}}-intkvm_vcpu_halt(structkvm_vcpu*vcpu)+intkvm_emulate_halt_noskip(structkvm_vcpu*vcpu){-return__kvm_vcpu_halt(vcpu,KVM_MP_STATE_HALTED,KVM_EXIT_HLT);+return__kvm_emulate_halt(vcpu,KVM_MP_STATE_HALTED,KVM_EXIT_HLT);}-EXPORT_SYMBOL_GPL(kvm_vcpu_halt);+EXPORT_SYMBOL_GPL(kvm_emulate_halt_noskip);intkvm_emulate_halt(structkvm_vcpu*vcpu){
@@ -8668,7 +8668,7 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu)*TODO:wemightbesquashingaGUESTDBG_SINGLESTEP-triggered*KVM_EXIT_DEBUGhere.*/-returnkvm_vcpu_halt(vcpu)&&ret;+returnkvm_emulate_halt_noskip(vcpu)&&ret;}EXPORT_SYMBOL_GPL(kvm_emulate_halt);
@@ -8676,7 +8676,8 @@ int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu){intret=kvm_skip_emulated_instruction(vcpu);-return__kvm_vcpu_halt(vcpu,KVM_MP_STATE_AP_RESET_HOLD,KVM_EXIT_AP_RESET_HOLD)&&ret;+return__kvm_emulate_halt(vcpu,KVM_MP_STATE_AP_RESET_HOLD,+KVM_EXIT_AP_RESET_HOLD)&&ret;}EXPORT_SYMBOL_GPL(kvm_emulate_ap_reset_hold);
--
2.33.0.685.g46640cef36-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:56:40
Rename kvm_vcpu_block() to kvm_vcpu_halt() in preparation for splitting
the actual "block" sequences into a separate helper (to be named
kvm_vcpu_block()). x86 will use the standalone block-only path to handle
non-halt cases where the vCPU is not runnable.
Rename block_ns to halt_ns to match the new function name.
Opportunistically move an x86-specific comment to x86, and enhance it, too.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/arm64/kvm/arch_timer.c | 2 +-
arch/arm64/kvm/handle_exit.c | 4 ++--
arch/arm64/kvm/psci.c | 2 +-
arch/mips/kvm/emulate.c | 2 +-
arch/powerpc/kvm/book3s_pr.c | 2 +-
arch/powerpc/kvm/book3s_pr_papr.c | 2 +-
arch/powerpc/kvm/booke.c | 2 +-
arch/powerpc/kvm/powerpc.c | 2 +-
arch/s390/kvm/interrupt.c | 2 +-
arch/x86/kvm/x86.c | 11 +++++++++--
include/linux/kvm_host.h | 2 +-
virt/kvm/kvm_main.c | 20 +++++++++-----------
12 files changed, 29 insertions(+), 24 deletions(-)
@@ -3273,7 +3270,8 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)}out:-block_ns=ktime_to_ns(cur)-ktime_to_ns(start);+/* The total time the vCPU was "halted", including polling time. */+halt_ns=ktime_to_ns(cur)-ktime_to_ns(start);/**Note,halt-pollingisconsideredsuccessfulsolongasthevCPUwas
@@ -3287,24 +3285,24 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)if(!vcpu_valid_wakeup(vcpu)){shrink_halt_poll_ns(vcpu);}elseif(vcpu->kvm->max_halt_poll_ns){-if(block_ns<=vcpu->halt_poll_ns)+if(halt_ns<=vcpu->halt_poll_ns);/* we had a long block, shrink polling */elseif(vcpu->halt_poll_ns&&-block_ns>vcpu->kvm->max_halt_poll_ns)+halt_ns>vcpu->kvm->max_halt_poll_ns)shrink_halt_poll_ns(vcpu);/* we had a short halt and our poll time is too small */elseif(vcpu->halt_poll_ns<vcpu->kvm->max_halt_poll_ns&&-block_ns<vcpu->kvm->max_halt_poll_ns)+halt_ns<vcpu->kvm->max_halt_poll_ns)grow_halt_poll_ns(vcpu);}else{vcpu->halt_poll_ns=0;}}-trace_kvm_vcpu_wakeup(block_ns,waited,vcpu_valid_wakeup(vcpu));+trace_kvm_vcpu_wakeup(halt_ns,waited,vcpu_valid_wakeup(vcpu));}-EXPORT_SYMBOL_GPL(kvm_vcpu_block);+EXPORT_SYMBOL_GPL(kvm_vcpu_halt);boolkvm_vcpu_wake_up(structkvm_vcpu*vcpu){
--
2.33.0.685.g46640cef36-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:56:51
Factor out the "block" part of kvm_vcpu_halt() so that x86 can emulate
non-halt wait/sleep/block conditions that should not be subjected to
halt-polling.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
include/linux/kvm_host.h | 1 +
virt/kvm/kvm_main.c | 50 ++++++++++++++++++++++++++++------------
2 files changed, 36 insertions(+), 15 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:56:54
From: Jing Zhang <redacted>
Add a "blocking" stat that userspace can use to detect the case where a
vCPU is not being run because of a vCPU/guest action, e.g. HLT or WFS on
x86, WFI on arm64, etc... Current guest/host/halt stats don't show this
well, e.g. if a guest halts for a long period of time then the vCPU could
appear pathologically blocked due to a host condition, when in reality the
vCPU has been put into a not-runnable state by the guest.
Originally-by: Cannon Matthews [off-list ref]
Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Jing Zhang <redacted>
[sean: renamed stat to "blocking", massaged changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
include/linux/kvm_host.h | 3 ++-
include/linux/kvm_types.h | 1 +
virt/kvm/kvm_main.c | 2 ++
3 files changed, 5 insertions(+), 1 deletion(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:57:21
Calculate the halt-polling "stop" time using "cur" instead of redoing
ktime_get(). In the happy case where hardware correctly predicts
do_halt_poll, "cur" is only a few cycles old. And if the branch is
mispredicted, arguably that extra latency should count toward the
halt-polling time.
In all likelihood, the numbers involved are in the noise and either
approach is perfectly ok.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/kvm_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:57:34
Go directly to kvm_vcpu_block() when handling the case where userspace
attempts to run an UNINITIALIZED vCPU. The vCPU isn't halted and its time
spent in limbo arguably should not be factored into halt-polling as the
behavior of the VM at this point is not at all indicative of the behavior
of the VM once it is up and running, i.e. executing HLT in idle tasks.
Note, because this case is encountered only on the first run of an AP vCPU,
vcpu->halt_poll_ns is guaranteed to be '0', and so KVM will not attempt
halt-polling, i.e. this really only affects the post-block bookkeeping.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/x86.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-25 00:57:35
Call kvm_vcpu_block() directly for all wait states except HALTED so that
kvm_vcpu_halt() is no longer a misnomer on x86.
Functionally, this means KVM will never attempt halt-polling or adjust
vcpu->halt_poll_ns for INIT_RECEIVED (a.k.a. Wait-For-SIPI (WFS)) or
AP_RESET_HOLD; UNINITIALIZED is handled in kvm_arch_vcpu_ioctl_run(),
and x86 doesn't use any other "wait" states.
As mentioned above, the motivation of this is purely so that "halt" isn't
overloaded on x86, e.g. in KVM's stats. Skipping halt-polling for WFS
(and RESET_HOLD) has no meaningful effect on guest performance as there
are typically single-digit numbers of INIT-SIPI sequences per AP vCPU,
per boot, versus thousands of HLTs just to boot to console.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/x86.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
From: Marc Zyngier <maz@kernel.org> Date: 2021-09-25 09:50:42
On Sat, 25 Sep 2021 01:55:21 +0100,
Sean Christopherson [off-list ref] wrote:
Invoke the arch hooks for block+unblock if and only if KVM actually
attempts to block the vCPU. The only non-nop implementation is on arm64,
and if halt-polling is successful, there is no need for arm64 to put/load
the vGIC as KVM hasn't relinquished control of the vCPU in any way.
This doesn't mean that there is no requirement for any state
change. The put/load on GICv4 is crucial for performance, and the VMCR
resync is a correctness requirement.
The primary motivation is to allow future cleanup to split out "block"
from "halt", but this is also likely a small performance boost on arm64
when halt-polling is successful.
Adjust the post-block path to update "cur" after unblocking, i.e. include
vGIC load time in halt_wait_ns and halt_wait_hist, so that the behavior
is consistent. Moving just the pre-block arch hook would result in only
the vGIC put latency being included in the halt_wait stats. There is no
obvious evidence that one way or the other is correct, so just ensure KVM
is consistent.
This effectively reverts 07ab0f8d9a12 ("KVM: Call
kvm_arch_vcpu_blocking early into the blocking sequence"), which was a
huge gain on arm64, not to mention a correctness fix.
Without this, a GICv4 machine will always pay for the full poll
penalty, going into schedule(), and only then get a doorbell interrupt
signalling telling the kernel that there was an interrupt.
On a non-GICv4 machine, it means that interrupts injected by another
thread during the pooling will be evaluated with an outdated priority
mask, which can result in either a spurious wake-up or a missed
wake-up.
If it means introducing a new set of {pre,post}-poll arch-specific
hooks, so be it. But I don't think this change is acceptable as is.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2021-09-26 06:27:40
On 25/09/21 11:50, Marc Zyngier wrote:
quoted
there is no need for arm64 to put/load
the vGIC as KVM hasn't relinquished control of the vCPU in any way.
This doesn't mean that there is no requirement for any state
change. The put/load on GICv4 is crucial for performance, and the VMCR
resync is a correctness requirement.
I wouldn't even say it's crucial for performance: halt polling cannot
work and is a waste of time without (the current implementation of)
put/load.
However, is activating the doorbell necessary? If possible, polling the
VGIC directly for pending VLPIs without touching the ITS (for example by
emulating IAR reads) may make sense. IIUC that must be done at EL2
though, so maybe it would even make sense to move all of halt polling to
EL2 for the nVHE case. It all depends on benchmark results, of course.
Sorry for the many stupid questions I'm asking lately, but I'm trying to
pay more attention to ARM and understand the VGIC and EL1/EL2 split better.
Paolo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-09-26 09:02:35
On Sun, 26 Sep 2021 07:27:28 +0100,
Paolo Bonzini [off-list ref] wrote:
On 25/09/21 11:50, Marc Zyngier wrote:
quoted
quoted
there is no need for arm64 to put/load
the vGIC as KVM hasn't relinquished control of the vCPU in any way.
This doesn't mean that there is no requirement for any state
change. The put/load on GICv4 is crucial for performance, and the VMCR
resync is a correctness requirement.
I wouldn't even say it's crucial for performance: halt polling cannot
work and is a waste of time without (the current implementation of)
put/load.
Not quite. A non-V{LPI,SGI} could still be used as the a wake-up from
WFI (which is the only reason we end-up on this path). Only LPIs (and
SGIs on GICv4.1) can be directly injected, meaning that SPIs and PPIs
still follow the standard SW injection model.
However, there is still the ICH_VMCR_EL2 requirement (to get the
up-to-date priority mask and group enable bits) for SW-injected
interrupt wake-up to work correctly, and I really don't want to save
that one eagerly on each shallow exit.
However, is activating the doorbell necessary? If possible, polling
the VGIC directly for pending VLPIs without touching the ITS (for
example by emulating IAR reads) may make sense. IIUC that must be
done at EL2 though, so maybe it would even make sense to move all of
halt polling to EL2 for the nVHE case. It all depends on benchmark
results, of course.
No, there is no architectural way to observe the VLPI state. EL2
cannot impersonate the guest an read ICV_IAR1_EL1 (because it
conveniently has the same encoding as ICC_IAR1_EL1), and if it could,
it would be *destructive* (not what you want). The equivalent of the
LR that is used to hold the highest priority VLPI presented to the
virtual CPU interface is not visible to SW at all.
There are exactly two ways for the hypervisor to get a hint about the
VLPI state (and that's only a hint, as everything can be spurious):
- Make the vPE non resident and use GICR_VPENDBASER.PendingLast bit to
find out whether there are pending VLPIs
- Make the vPE non resident and get a doorbell interrupt
See the common pattern?
There is no polling mechanism, and the only way to flush the VLPI
state to memory is to destroy the GIC view of the vPE, which is a bit
counter-productive. It also only work on GICv4.1, and not GICv4 (which
is why we don't support live migration on GICv4).
Sorry for the many stupid questions I'm asking lately, but I'm trying
to pay more attention to ARM and understand the VGIC and EL1/EL2 split
better.
Feel free to ask any question. The more people understand how the
architecture works, the better.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Christian Borntraeger <hidden> Date: 2021-09-27 06:55:32
Am 25.09.21 um 02:55 schrieb Sean Christopherson:
Wrap s390's halt_poll_max_steal with READ_ONCE and snapshot the result of
kvm_arch_no_poll() in kvm_vcpu_block() to avoid a mostly-theoretical,
largely benign bug on s390 where the result of kvm_arch_no_poll() could
change due to userspace modifying halt_poll_max_steal while the vCPU is
blocking. The bug is largely benign as it will either cause KVM to skip
updating halt-polling times (no_poll toggles false=>true) or to update
halt-polling times with a slightly flawed block_ns.
Note, READ_ONCE is unnecessary in the current code, add it in case the
arch hook is ever inlined, and to provide a hint that userspace can
change the param at will.
Fixes: 8b905d28ee17 ("KVM: s390: provide kvm_arch_no_poll function")
Cc: Christian Borntraeger <redacted>
Signed-off-by: Sean Christopherson <seanjc@google.com>
@@ -3446,7 +3446,7 @@ bool kvm_arch_no_poll(struct kvm_vcpu *vcpu){/* do not poll with more than halt_poll_max_steal percent of steal time */if(S390_lowcore.avg_steal_timer*100/(TICK_USEC<<12)>=-halt_poll_max_steal){+READ_ONCE(halt_poll_max_steal)){vcpu->stat.halt_no_poll_steal++;returntrue;}
From: Christian Borntraeger <hidden> Date: 2021-09-27 06:59:17
Am 25.09.21 um 02:55 schrieb Sean Christopherson:
Move the clearing of valid_wakeup out of kvm_arch_vcpu_block_finish() so
that a future patch can drop said arch hook. Unlike the other blocking-
related arch hooks (vcpu_blocking/unblocking()), vcpu_block_finish() needs
to be called even if the KVM doesn't actually block the vCPU. This will
allow future patches to differentiate between truly blocking the vCPU and
emulating a halt condition without introducing a contradiction.
Alternatively, the hook could be renamed to kvm_arch_vcpu_halt_finish(),
but there's literally one call site in s390, and future cleanup can also
be done to handle valid_wakeup fully within kvm_s390_handle_wait() and
allow generic KVM to drop vcpu_valid_wakeup().
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: Christian Borntraeger <hidden> Date: 2021-09-27 06:59:31
Am 25.09.21 um 02:55 schrieb Sean Christopherson:
Drop kvm_arch_vcpu_block_finish() now that all arch implementations are
nops.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: Christian Borntraeger <hidden> Date: 2021-09-27 07:07:04
Am 25.09.21 um 02:55 schrieb Sean Christopherson:
Rename kvm_vcpu_block() to kvm_vcpu_halt() in preparation for splitting
the actual "block" sequences into a separate helper (to be named
kvm_vcpu_block()). x86 will use the standalone block-only path to handle
non-halt cases where the vCPU is not runnable.
Rename block_ns to halt_ns to match the new function name.
Opportunistically move an x86-specific comment to x86, and enhance it, too.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
@@ -3273,7 +3270,8 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)}out:-block_ns=ktime_to_ns(cur)-ktime_to_ns(start);+/* The total time the vCPU was "halted", including polling time. */+halt_ns=ktime_to_ns(cur)-ktime_to_ns(start);/**Note,halt-pollingisconsideredsuccessfulsolongasthevCPUwas
@@ -3287,24 +3285,24 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)if(!vcpu_valid_wakeup(vcpu)){shrink_halt_poll_ns(vcpu);}elseif(vcpu->kvm->max_halt_poll_ns){-if(block_ns<=vcpu->halt_poll_ns)+if(halt_ns<=vcpu->halt_poll_ns);/* we had a long block, shrink polling */elseif(vcpu->halt_poll_ns&&-block_ns>vcpu->kvm->max_halt_poll_ns)+halt_ns>vcpu->kvm->max_halt_poll_ns)shrink_halt_poll_ns(vcpu);/* we had a short halt and our poll time is too small */elseif(vcpu->halt_poll_ns<vcpu->kvm->max_halt_poll_ns&&-block_ns<vcpu->kvm->max_halt_poll_ns)+halt_ns<vcpu->kvm->max_halt_poll_ns)grow_halt_poll_ns(vcpu);}else{vcpu->halt_poll_ns=0;}}-trace_kvm_vcpu_wakeup(block_ns,waited,vcpu_valid_wakeup(vcpu));+trace_kvm_vcpu_wakeup(halt_ns,waited,vcpu_valid_wakeup(vcpu));}-EXPORT_SYMBOL_GPL(kvm_vcpu_block);+EXPORT_SYMBOL_GPL(kvm_vcpu_halt);boolkvm_vcpu_wake_up(structkvm_vcpu*vcpu){
From: Christian Borntraeger <hidden> Date: 2021-09-27 07:23:51
While looking into this series,
I realized that Davids patch
commit acd05785e48c01edb2c4f4d014d28478b5f19fb5
Author: David Matlack [off-list ref]
AuthorDate: Fri Apr 17 15:14:46 2020 -0700
Commit: Paolo Bonzini [off-list ref]
CommitDate: Fri Apr 24 12:53:17 2020 -0400
kvm: add capability for halt polling
broke the possibility for an admin to disable halt polling for already running KVM guests.
In past times doing
echo 0 > /sys/module/kvm/parameters/halt_poll_ns
stopped polling system wide.
Now all KVM guests will use the halt_poll_ns value that was active during startup - even those that do not use KVM_CAP_HALT_POLL.
I guess this was not intended?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Christian Borntraeger <hidden> Date: 2021-09-27 07:42:48
Am 25.09.21 um 02:55 schrieb Sean Christopherson:
Factor out the "block" part of kvm_vcpu_halt() so that x86 can emulate
non-halt wait/sleep/block conditions that should not be subjected to
halt-polling.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-27 15:00:01
On Mon, Sep 27, 2021, Christian Borntraeger wrote:
While looking into this series,
I realized that Davids patch
commit acd05785e48c01edb2c4f4d014d28478b5f19fb5
Author: David Matlack [off-list ref]
AuthorDate: Fri Apr 17 15:14:46 2020 -0700
Commit: Paolo Bonzini [off-list ref]
CommitDate: Fri Apr 24 12:53:17 2020 -0400
kvm: add capability for halt polling
broke the possibility for an admin to disable halt polling for already running KVM guests.
In past times doing
echo 0 > /sys/module/kvm/parameters/halt_poll_ns
stopped polling system wide.
Now all KVM guests will use the halt_poll_ns value that was active during
startup - even those that do not use KVM_CAP_HALT_POLL.
I guess this was not intended?
Ouch. I would go so far as to say that halt_poll_ns should be a hard limit on
the capability. What about having the per-VM variable track only the capability,
and then use the module param to cap the max when doing adjustments? E.g. add
a variant of this early in the series?
@@ -3304,19 +3304,25 @@ void kvm_vcpu_halt(struct kvm_vcpu *vcpu)update_halt_poll_stats(vcpu,start,poll_end,!waited);if(halt_poll_allowed){+max_halt_poll_ns=vcpu->kvm->max_halt_poll_ns;+if(max_halt_poll_ns)+max_halt_poll_ns=min(max_halt_poll_ns,halt_poll_ns);+else+max_halt_poll_ns=halt_poll_ns;+if(!vcpu_valid_wakeup(vcpu)){shrink_halt_poll_ns(vcpu);-}elseif(vcpu->kvm->max_halt_poll_ns){+}elseif(max_halt_poll_ns){if(halt_ns<=vcpu->halt_poll_ns);/* we had a long block, shrink polling */elseif(vcpu->halt_poll_ns&&-halt_ns>vcpu->kvm->max_halt_poll_ns)+halt_ns>max_halt_poll_ns)shrink_halt_poll_ns(vcpu);/* we had a short halt and our poll time is too small */-elseif(vcpu->halt_poll_ns<vcpu->kvm->max_halt_poll_ns&&-halt_ns<vcpu->kvm->max_halt_poll_ns)-grow_halt_poll_ns(vcpu);+elseif(vcpu->halt_poll_ns<max_halt_poll_ns&&+halt_ns<max_halt_poll_ns)+grow_halt_poll_ns(vcpu,max_halt_poll_ns);}else{vcpu->halt_poll_ns=0;}
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2021-09-27 15:04:06
On 27/09/21 16:59, Sean Christopherson wrote:
quoted
commit acd05785e48c01edb2c4f4d014d28478b5f19fb5
Author: David Matlack[off-list ref]
AuthorDate: Fri Apr 17 15:14:46 2020 -0700
Commit: Paolo Bonzini[off-list ref]
CommitDate: Fri Apr 24 12:53:17 2020 -0400
kvm: add capability for halt polling
broke the possibility for an admin to disable halt polling for already running KVM guests.
In past times doing
echo 0 > /sys/module/kvm/parameters/halt_poll_ns
stopped polling system wide.
Now all KVM guests will use the halt_poll_ns value that was active during
startup - even those that do not use KVM_CAP_HALT_POLL.
I guess this was not intended?
No, but...
I would go so far as to say that halt_poll_ns should be a hard limit on
the capability
... this would not be a good idea I think. Anything that wants to do a
lot of polling can just do "for (;;)".
So I think there are two possibilities that makes sense:
* track what is using KVM_CAP_HALT_POLL, and make writes to halt_poll_ns
follow that
* just make halt_poll_ns read-only.
Paolo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-27 15:16:02
On Mon, Sep 27, 2021, Paolo Bonzini wrote:
On 27/09/21 16:59, Sean Christopherson wrote:
quoted
quoted
commit acd05785e48c01edb2c4f4d014d28478b5f19fb5
Author: David Matlack[off-list ref]
AuthorDate: Fri Apr 17 15:14:46 2020 -0700
Commit: Paolo Bonzini[off-list ref]
CommitDate: Fri Apr 24 12:53:17 2020 -0400
kvm: add capability for halt polling
broke the possibility for an admin to disable halt polling for already running KVM guests.
In past times doing
echo 0 > /sys/module/kvm/parameters/halt_poll_ns
stopped polling system wide.
Now all KVM guests will use the halt_poll_ns value that was active during
startup - even those that do not use KVM_CAP_HALT_POLL.
I guess this was not intended?
No, but...
quoted
I would go so far as to say that halt_poll_ns should be a hard limit on
the capability
... this would not be a good idea I think. Anything that wants to do a lot
of polling can just do "for (;;)".
Hmm, true, there is no danger to the system in having the capability override the
module param.
So I think there are two possibilities that makes sense:
* track what is using KVM_CAP_HALT_POLL, and make writes to halt_poll_ns
follow that
I think this option makes more sense, making halt_poll_ns read-only is basically
forcing users to switch to KVM_CAP_HALT_POLL.
From: Christian Borntraeger <hidden> Date: 2021-09-27 15:17:57
Am 27.09.21 um 17:03 schrieb Paolo Bonzini:
On 27/09/21 16:59, Sean Christopherson wrote:
quoted
quoted
commit acd05785e48c01edb2c4f4d014d28478b5f19fb5
Author: David Matlack[off-list ref]
AuthorDate: Fri Apr 17 15:14:46 2020 -0700
Commit: Paolo Bonzini[off-list ref]
CommitDate: Fri Apr 24 12:53:17 2020 -0400
kvm: add capability for halt polling
broke the possibility for an admin to disable halt polling for already running KVM guests.
In past times doing
echo 0 > /sys/module/kvm/parameters/halt_poll_ns
stopped polling system wide.
Now all KVM guests will use the halt_poll_ns value that was active during
startup - even those that do not use KVM_CAP_HALT_POLL.
I guess this was not intended?
No, but...
quoted
I would go so far as to say that halt_poll_ns should be a hard limit on
the capability
... this would not be a good idea I think. Anything that wants to do a lot of polling can just do "for (;;)".
So I think there are two possibilities that makes sense:
* track what is using KVM_CAP_HALT_POLL, and make writes to halt_poll_ns follow that
what about using halt_poll_ns for those VMs that did not uses KVM_CAP_HALT_POLL and the private number for those that did.
From: David Matlack <dmatlack@google.com> Date: 2021-09-27 16:59:23
On Mon, Sep 27, 2021 at 8:17 AM Christian Borntraeger
[off-list ref] wrote:
Am 27.09.21 um 17:03 schrieb Paolo Bonzini:
quoted
On 27/09/21 16:59, Sean Christopherson wrote:
quoted
quoted
commit acd05785e48c01edb2c4f4d014d28478b5f19fb5
Author: David Matlack[off-list ref]
AuthorDate: Fri Apr 17 15:14:46 2020 -0700
Commit: Paolo Bonzini[off-list ref]
CommitDate: Fri Apr 24 12:53:17 2020 -0400
kvm: add capability for halt polling
broke the possibility for an admin to disable halt polling for already running KVM guests.
In past times doing
echo 0 > /sys/module/kvm/parameters/halt_poll_ns
stopped polling system wide.
Now all KVM guests will use the halt_poll_ns value that was active during
startup - even those that do not use KVM_CAP_HALT_POLL.
I guess this was not intended?
No, but...
quoted
I would go so far as to say that halt_poll_ns should be a hard limit on
the capability
... this would not be a good idea I think. Anything that wants to do a lot of polling can just do "for (;;)".
I agree. It would also be a maintenance burden and subtle "gotcha" to
have to increase halt_poll_ns anytime one wants to increase
KVM_CAP_HALT_POLL.
quoted
So I think there are two possibilities that makes sense:
* track what is using KVM_CAP_HALT_POLL, and make writes to halt_poll_ns follow that
what about using halt_poll_ns for those VMs that did not uses KVM_CAP_HALT_POLL and the private number for those that did.
None of these options would cover Christian's original use-case
though. (Write to module to disable halt-polling system-wide.)
What about adding a writable "enable_halt_polling" module parameter
that affects all VMs? Once that is in place we could also consider
getting rid of halt_poll_ns entirely.
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2021-09-27 17:29:27
On Mon, Sep 27, 2021 at 5:17 PM Christian Borntraeger
[off-list ref] wrote:
quoted
So I think there are two possibilities that makes sense:
* track what is using KVM_CAP_HALT_POLL, and make writes to halt_poll_ns follow that
what about using halt_poll_ns for those VMs that did not uses KVM_CAP_HALT_POLL and the private number for those that did.
Yes, that's what I meant. David pointed out that doesn't allow you to
disable halt polling altogether, but for that you can always ask each
VM's userspace one by one, or just not use KVM_CAP_HALT_POLL. (Also, I
don't know about Google's usecase, but mine was actually more about
using KVM_CAP_HALT_POLL to *disable* halt polling on some VMs!).
Paolo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-27 17:44:58
On Sun, Sep 26, 2021, Marc Zyngier wrote:
On Sun, 26 Sep 2021 07:27:28 +0100,
Paolo Bonzini [off-list ref] wrote:
quoted
On 25/09/21 11:50, Marc Zyngier wrote:
quoted
quoted
there is no need for arm64 to put/load
the vGIC as KVM hasn't relinquished control of the vCPU in any way.
This doesn't mean that there is no requirement for any state
change. The put/load on GICv4 is crucial for performance, and the VMCR
resync is a correctness requirement.
Ah crud, I didn't blame that code beforehand, I simply assumed
kvm_arch_vcpu_blocking() was purely for the blocking/schedule() sequence. The
comment in arm64's kvm_arch_vcpu_blocking() about kvm_arch_vcpu_runnable() makes
more sense now too.
quoted
I wouldn't even say it's crucial for performance: halt polling cannot
work and is a waste of time without (the current implementation of)
put/load.
Not quite. A non-V{LPI,SGI} could still be used as the a wake-up from
WFI (which is the only reason we end-up on this path). Only LPIs (and
SGIs on GICv4.1) can be directly injected, meaning that SPIs and PPIs
still follow the standard SW injection model.
However, there is still the ICH_VMCR_EL2 requirement (to get the
up-to-date priority mask and group enable bits) for SW-injected
interrupt wake-up to work correctly, and I really don't want to save
that one eagerly on each shallow exit.
IIUC, VMCR is resident in hardware while the guest is running, and KVM needs to
retrieve the VMCR when processing interrupts to determine if a interrupt is above
the priority threshold. If that's the case, then IMO handling the VMCR via an
arch hook is unnecessarily fragile, e.g. any generic call that leads to
kvm_arch_vcpu_runnable() needs to know that arm64 lazily retrieves a guest
register. A better approach for VMCR would be to retrieve the value from
hardware on-demand, e.g. via a hook in vgic_get_vmcr(), so that it's all but
impossible to have bugs where KVM is working with a stale VMCR, e.g.
Regarding vGIC v4, does KVM require it to be resident in hardware while the vCPU
is loaded? If not, then we could do something like this, which would eliminate
the arch hooks entirely if the VMCR is handled as above.
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-27 17:46:22
On Mon, Sep 27, 2021, Paolo Bonzini wrote:
On Mon, Sep 27, 2021 at 5:17 PM Christian Borntraeger
[off-list ref] wrote:
quoted
quoted
So I think there are two possibilities that makes sense:
* track what is using KVM_CAP_HALT_POLL, and make writes to halt_poll_ns follow that
what about using halt_poll_ns for those VMs that did not uses KVM_CAP_HALT_POLL and the private number for those that did.
Yes, that's what I meant. David pointed out that doesn't allow you to
disable halt polling altogether, but for that you can always ask each
VM's userspace one by one, or just not use KVM_CAP_HALT_POLL. (Also, I
don't know about Google's usecase, but mine was actually more about
using KVM_CAP_HALT_POLL to *disable* halt polling on some VMs!).
I kinda like the idea if special-casing halt_poll_ns=0, e.g. for testing or
in-the-field mitigation if halt-polling is broken. It'd be trivial to support, e.g.
@@ -3304,19 +3304,23 @@ void kvm_vcpu_halt(struct kvm_vcpu *vcpu) update_halt_poll_stats(vcpu, start, poll_end, !waited); if (halt_poll_allowed) {+ max_halt_poll_ns = vcpu->kvm->max_halt_poll_ns;+ if (!max_halt_poll_ns || !halt_poll_ns) <------ squish the max if halt_poll_ns==0+ max_halt_poll_ns = halt_poll_ns;+ if (!vcpu_valid_wakeup(vcpu)) { shrink_halt_poll_ns(vcpu);- } else if (vcpu->kvm->max_halt_poll_ns) {+ } else if (max_halt_poll_ns) { if (halt_ns <= vcpu->halt_poll_ns) ; /* we had a long block, shrink polling */ else if (vcpu->halt_poll_ns &&- halt_ns > vcpu->kvm->max_halt_poll_ns)+ halt_ns > max_halt_poll_ns) shrink_halt_poll_ns(vcpu); /* we had a short halt and our poll time is too small */- else if (vcpu->halt_poll_ns < vcpu->kvm->max_halt_poll_ns &&- halt_ns < vcpu->kvm->max_halt_poll_ns)- grow_halt_poll_ns(vcpu);+ else if (vcpu->halt_poll_ns < max_halt_poll_ns &&+ halt_ns < max_halt_poll_ns)+ grow_halt_poll_ns(vcpu, max_halt_poll_ns); } else { vcpu->halt_poll_ns = 0; }
From: Marc Zyngier <maz@kernel.org> Date: 2021-09-28 09:24:41
On Mon, 27 Sep 2021 18:28:14 +0100,
Sean Christopherson [off-list ref] wrote:
On Sun, Sep 26, 2021, Marc Zyngier wrote:
quoted
On Sun, 26 Sep 2021 07:27:28 +0100,
Paolo Bonzini [off-list ref] wrote:
quoted
On 25/09/21 11:50, Marc Zyngier wrote:
quoted
quoted
there is no need for arm64 to put/load
the vGIC as KVM hasn't relinquished control of the vCPU in any way.
This doesn't mean that there is no requirement for any state
change. The put/load on GICv4 is crucial for performance, and the VMCR
resync is a correctness requirement.
Ah crud, I didn't blame that code beforehand, I simply assumed
kvm_arch_vcpu_blocking() was purely for the blocking/schedule()
sequence. The comment in arm64's kvm_arch_vcpu_blocking() about
kvm_arch_vcpu_runnable() makes more sense now too.
quoted
quoted
I wouldn't even say it's crucial for performance: halt polling cannot
work and is a waste of time without (the current implementation of)
put/load.
Not quite. A non-V{LPI,SGI} could still be used as the a wake-up from
WFI (which is the only reason we end-up on this path). Only LPIs (and
SGIs on GICv4.1) can be directly injected, meaning that SPIs and PPIs
still follow the standard SW injection model.
However, there is still the ICH_VMCR_EL2 requirement (to get the
up-to-date priority mask and group enable bits) for SW-injected
interrupt wake-up to work correctly, and I really don't want to save
that one eagerly on each shallow exit.
IIUC, VMCR is resident in hardware while the guest is running, and
KVM needs to retrieve the VMCR when processing interrupts to
determine if a interrupt is above the priority threshold. If that's
the case, then IMO handling the VMCR via an arch hook is
unnecessarily fragile, e.g. any generic call that leads to
kvm_arch_vcpu_runnable() needs to know that arm64 lazily retrieves a
guest register.
Not quite. We only need to retrieve the VMCR if we are in a situation
where we need to trigger a wake-up from WFI at the point where we have
not done a vcpu_put() yet. All the other cases where the interrupt is
injected are managed by the HW. And the only case where
kvm_arch_vcpu_runnable() gets called is when blocking.
I also don't get why a hook would be fragile, as long as it has well
defined semantics.
quoted hunk
A better approach for VMCR would be to retrieve the value from
hardware on-demand, e.g. via a hook in vgic_get_vmcr(), so that it's all but
impossible to have bugs where KVM is working with a stale VMCR, e.g.
But most of the uses of vgic_get_vmcr() are in contexts where the vcpu
isn't running at all (such as save/restore). It really only operates
on the shadow state, and what you have above will only lead to state
corruption.
if (kvm_vgic_global_state.type == VGIC_V2)
vgic_v2_get_vmcr(vcpu, vmcr);
else
Regarding vGIC v4, does KVM require it to be resident in hardware
while the vCPU is loaded?
It is a requirement. Otherwise, we end-up with an inconsistent state
between the delivery of doorbells and the state of the vgic. Also,
reloading the GICv4 state can be pretty expensive (multiple MMIO
accesses), which is why we really don't want to do that on the hot
path (kvm_arch_vcpu_ioctl_run() *is* a hot path).
quoted hunk
If not, then we could do something like
this, which would eliminate the arch hooks entirely if the VMCR is
handled as above.
@@ -365,31 +365,6 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)returnkvm_timer_is_pending(vcpu);}-voidkvm_arch_vcpu_blocking(structkvm_vcpu*vcpu)-{-/*-*Ifwe'reabouttoblock(mostlikelybecausewe'vejusthita-*WFI),weneedtosyncbackthestateoftheGICCPUinterface-*sothatwehavethelatestPMRandgroupenables.Thisensures-*thatkvm_arch_vcpu_runnablehasup-to-datedatatodecide-*whetherwehavependinginterrupts.-*-*Forthesamereason,wewanttotellGICv4thatweneed-*doorbellstobesignalled,shouldaninterruptbecomepending.-*/-preempt_disable();-kvm_vgic_vmcr_sync(vcpu);-vgic_v4_put(vcpu,true);-preempt_enable();-}--voidkvm_arch_vcpu_unblocking(structkvm_vcpu*vcpu)-{-preempt_disable();-vgic_v4_load(vcpu);-preempt_enable();-}-voidkvm_arch_vcpu_load(structkvm_vcpu*vcpu,intcpu){structkvm_s2_mmu*mmu;
@@ -697,7 +672,6 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)/* The distributor enable bits were changed */preempt_disable();vgic_v4_put(vcpu,false);-vgic_v4_load(vcpu);preempt_enable();}
@@ -813,6 +787,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)*/preempt_disable();+/*+*ReloadvGICv4ifnecessary,asitmaybeputon-demandso+*thatKVMcandetectdirectlyinjectedinterrupts,e.g.when+*determiningifthevCPUisrunnableduetoapendingevent.+*/+vgic_v4_load(vcpu);
You'd need to detect that a previous put has been done. But overall,
it puts the complexity at the wrong place. WFI (aka kvm_vcpu_block) is
the place where we want to handle this synchronisation, and not the
run loop.
Instead of having a well defined interface with the blocking code
where we implement the required synchronisation, you spray the vgic
crap all over, and it becomes much harder to reason about it. Guess
what, I'm not keen on it.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Christopherson <seanjc@google.com> Date: 2021-09-28 16:21:21
On Tue, Sep 28, 2021, Marc Zyngier wrote:
On Mon, 27 Sep 2021 18:28:14 +0100,
Sean Christopherson [off-list ref] wrote:
quoted
On Sun, Sep 26, 2021, Marc Zyngier wrote:
quoted
On Sun, 26 Sep 2021 07:27:28 +0100,
Paolo Bonzini [off-list ref] wrote:
quoted
On 25/09/21 11:50, Marc Zyngier wrote:
quoted
quoted
there is no need for arm64 to put/load
the vGIC as KVM hasn't relinquished control of the vCPU in any way.
This doesn't mean that there is no requirement for any state
change. The put/load on GICv4 is crucial for performance, and the VMCR
resync is a correctness requirement.
Ah crud, I didn't blame that code beforehand, I simply assumed
kvm_arch_vcpu_blocking() was purely for the blocking/schedule()
sequence. The comment in arm64's kvm_arch_vcpu_blocking() about
kvm_arch_vcpu_runnable() makes more sense now too.
quoted
quoted
I wouldn't even say it's crucial for performance: halt polling cannot
work and is a waste of time without (the current implementation of)
put/load.
Not quite. A non-V{LPI,SGI} could still be used as the a wake-up from
WFI (which is the only reason we end-up on this path). Only LPIs (and
SGIs on GICv4.1) can be directly injected, meaning that SPIs and PPIs
still follow the standard SW injection model.
However, there is still the ICH_VMCR_EL2 requirement (to get the
up-to-date priority mask and group enable bits) for SW-injected
interrupt wake-up to work correctly, and I really don't want to save
that one eagerly on each shallow exit.
IIUC, VMCR is resident in hardware while the guest is running, and
KVM needs to retrieve the VMCR when processing interrupts to
determine if a interrupt is above the priority threshold. If that's
the case, then IMO handling the VMCR via an arch hook is
unnecessarily fragile, e.g. any generic call that leads to
kvm_arch_vcpu_runnable() needs to know that arm64 lazily retrieves a
guest register.
Not quite. We only need to retrieve the VMCR if we are in a situation
where we need to trigger a wake-up from WFI at the point where we have
not done a vcpu_put() yet. All the other cases where the interrupt is
injected are managed by the HW. And the only case where
kvm_arch_vcpu_runnable() gets called is when blocking.
I also don't get why a hook would be fragile, as long as it has well
defined semantics.
Generic KVM should not have to know that a seemingly benign arch hook,
kvm_arch_vcpu_runnable(), cannot be safely called without first calling another
arch hook. E.g. I suspect there's a (benign?) race in kvm_vcpu_on_spin(). If
the loop is delayed between checking rcuwait_active() and vcpu_dy_runnable(),
and the target vCPU is awakened during that period, KVM can call
kvm_arch_vcpu_runnable() while the vCPU is running.
It's kind of a counter-example to my below suggestion as putting the vGIC would
indeed lead to state corruption if the vCPU is running, but I would argue that
arm64 should override kvm_arch_dy_runnable() so that its correctness is guaranteed,
e.g. by not calling kvm_arch_vcpu_runnable() if the vCPU is already running.
quoted
A better approach for VMCR would be to retrieve the value from
hardware on-demand, e.g. via a hook in vgic_get_vmcr(), so that it's all but
impossible to have bugs where KVM is working with a stale VMCR, e.g.
But most of the uses of vgic_get_vmcr() are in contexts where the vcpu
isn't running at all (such as save/restore). It really only operates
on the shadow state, and what you have above will only lead to state
corruption.
Ignoring the kvm_arch_dy_runnable() case for the moment, how would it lead to
corruption? The idea is that the 'vmcr_available' flag would be cleared when the
vCPU is run, i.e. it tracks whether or not the shadow state may be stale.
quoted
if (kvm_vgic_global_state.type == VGIC_V2)
vgic_v2_get_vmcr(vcpu, vmcr);
else
Regarding vGIC v4, does KVM require it to be resident in hardware
while the vCPU is loaded?
It is a requirement. Otherwise, we end-up with an inconsistent state
between the delivery of doorbells and the state of the vgic.
For my own understanding, does KVM require it to be resident in hardware while
the vCPU is loaded but _not_ running? What I don't fully understand is how KVM
can safely load/put the vCPU if that true, i.e. wouldn't there always be a window
for badness?
Also, reloading the GICv4 state can be pretty expensive (multiple MMIO
accesses), which is why we really don't want to do that on the hot path
(kvm_arch_vcpu_ioctl_run() *is* a hot path).
I wasn't suggesting to reload GICv4 on every entry, it would only be reloaded
if it was put between vcpu_load() and entry to the guest.
quoted
If not, then we could do something like
this, which would eliminate the arch hooks entirely if the VMCR is
handled as above.
...
quoted
@@ -813,6 +787,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) */ preempt_disable();+ /*+ * Reload vGIC v4 if necessary, as it may be put on-demand so+ * that KVM can detect directly injected interrupts, e.g. when+ * determining if the vCPU is runnable due to a pending event.+ */+ vgic_v4_load(vcpu);
You'd need to detect that a previous put has been done.
Not that it will likely matter, but doesn't the its_vpe.resident check automatically
handle this?
But overall, it puts the complexity at the wrong place. WFI (aka
kvm_vcpu_block) is the place where we want to handle this synchronisation,
and not the run loop.
Instead of having a well defined interface with the blocking code
where we implement the required synchronisation, you spray the vgic
crap all over, and it becomes much harder to reason about it. Guess
what, I'm not keen on it.
My objection to the arch hooks is that, from generic KVM's perspective, the
direct dependency is not on blocking, it's on calling kvm_arch_vcpu_runnable().
That's why I suggested handling this by tracking whether or not the VMCR is
up-to-date/stale, as it allows generic KVM to safely call kvm_arch_vcpu_runnable()
whenever the vCPU is loaded.
I don't have a strong opinion on arm64 preferring the sync to be specific to
WFI, but if that's the case then IMO this should be handled fully in arm64, e.g.
a patch like so (or with a wrapper around the call to kvm_vcpu_block() if we
want to guard against future calls into generic KVM)
From: David Matlack <dmatlack@google.com> Date: 2021-09-28 18:57:56
On Fri, Sep 24, 2021 at 05:55:16PM -0700, Sean Christopherson wrote:
Don't update halt-polling stats if halt-polling wasn't attempted. This
is a nop as @poll_ns is guaranteed to be '0' (poll_end == start), but it
will allow a future patch to move the histogram stats into the helper to
resolve a discrepancy in what is considered a "successful" halt-poll.
No functional change intended.
Cc: David Matlack <dmatlack@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: David Matlack <dmatlack@google.com> Date: 2021-09-28 19:01:45
On Fri, Sep 24, 2021 at 05:55:17PM -0700, Sean Christopherson wrote:
Add a comment to document that halt-polling is considered successful even
if the polling loop itself didn't detect a wake event, i.e. if a wake
event was detect in the final kvm_vcpu_check_block(). Invert the param
to the update helper so that the helper is a dumb function that is "told"
whether or not polling was successful, as opposed to having it determinine
success/failure based on blocking behavior.
Opportunistically tweak the params to the update helper to reduce the
line length for the call site so that it fits on a single line, and so
that the prototype conforms to the more traditional kernel style.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: David Matlack <dmatlack@google.com> Date: 2021-09-28 21:26:22
On Fri, Sep 24, 2021 at 05:55:18PM -0700, Sean Christopherson wrote:
Move the halt-polling "success" and histogram stats update into the
dedicated helper to fix a discrepancy where the success/fail "time" stats
consider polling successful so long as the wait is avoided, but the main
"success" and histogram stats consider polling successful if and only if
a wake event was detected by the halt-polling loop.
Move halt_attempted_poll to the helper as well so that all the stats are
updated in a single location. While it's a bit odd to update the stat
well after the fact, practically speaking there's no meaningful advantage
to updating before polling.
Note, there is a functional change in addition to the success vs. fail
change. The histogram updates previously called ktime_get() instead of
using "cur". But that change is desirable as it means all the stats are
now updated with the same polling time, and avoids the extra ktime_get(),
which isn't expensive but isn't free either.
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: David Matlack <dmatlack@google.com> Date: 2021-09-28 21:28:46
On Fri, Sep 24, 2021 at 05:55:20PM -0700, Sean Christopherson wrote:
Drop kvm_arch_vcpu_block_finish() now that all arch implementations are
nops.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: David Matlack <dmatlack@google.com> Date: 2021-09-28 21:59:37
On Fri, Sep 24, 2021 at 05:55:22PM -0700, Sean Christopherson wrote:
Rename a variety of HLT-related helpers to free up the function name
"kvm_vcpu_halt" for future use in generic KVM code, e.g. to differentiate
between "block" and "halt".
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
@@ -8655,11 +8655,11 @@ static int __kvm_vcpu_halt(struct kvm_vcpu *vcpu, int state, int reason)}}-intkvm_vcpu_halt(structkvm_vcpu*vcpu)+intkvm_emulate_halt_noskip(structkvm_vcpu*vcpu){-return__kvm_vcpu_halt(vcpu,KVM_MP_STATE_HALTED,KVM_EXIT_HLT);+return__kvm_emulate_halt(vcpu,KVM_MP_STATE_HALTED,KVM_EXIT_HLT);}-EXPORT_SYMBOL_GPL(kvm_vcpu_halt);+EXPORT_SYMBOL_GPL(kvm_emulate_halt_noskip);intkvm_emulate_halt(structkvm_vcpu*vcpu){
@@ -8668,7 +8668,7 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu)*TODO:wemightbesquashingaGUESTDBG_SINGLESTEP-triggered*KVM_EXIT_DEBUGhere.*/-returnkvm_vcpu_halt(vcpu)&&ret;+returnkvm_emulate_halt_noskip(vcpu)&&ret;}EXPORT_SYMBOL_GPL(kvm_emulate_halt);
@@ -8676,7 +8676,8 @@ int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu){intret=kvm_skip_emulated_instruction(vcpu);-return__kvm_vcpu_halt(vcpu,KVM_MP_STATE_AP_RESET_HOLD,KVM_EXIT_AP_RESET_HOLD)&&ret;+return__kvm_emulate_halt(vcpu,KVM_MP_STATE_AP_RESET_HOLD,+KVM_EXIT_AP_RESET_HOLD)&&ret;}EXPORT_SYMBOL_GPL(kvm_emulate_ap_reset_hold);
From: David Matlack <dmatlack@google.com> Date: 2021-09-28 22:02:07
On Fri, Sep 24, 2021 at 05:55:23PM -0700, Sean Christopherson wrote:
Rename kvm_vcpu_block() to kvm_vcpu_halt() in preparation for splitting
the actual "block" sequences into a separate helper (to be named
kvm_vcpu_block()). x86 will use the standalone block-only path to handle
non-halt cases where the vCPU is not runnable.
Rename block_ns to halt_ns to match the new function name.
Opportunistically move an x86-specific comment to x86, and enhance it, too.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
@@ -3273,7 +3270,8 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)}out:-block_ns=ktime_to_ns(cur)-ktime_to_ns(start);+/* The total time the vCPU was "halted", including polling time. */+halt_ns=ktime_to_ns(cur)-ktime_to_ns(start);/**Note,halt-pollingisconsideredsuccessfulsolongasthevCPUwas
@@ -3287,24 +3285,24 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)if(!vcpu_valid_wakeup(vcpu)){shrink_halt_poll_ns(vcpu);}elseif(vcpu->kvm->max_halt_poll_ns){-if(block_ns<=vcpu->halt_poll_ns)+if(halt_ns<=vcpu->halt_poll_ns);/* we had a long block, shrink polling */elseif(vcpu->halt_poll_ns&&-block_ns>vcpu->kvm->max_halt_poll_ns)+halt_ns>vcpu->kvm->max_halt_poll_ns)shrink_halt_poll_ns(vcpu);/* we had a short halt and our poll time is too small */elseif(vcpu->halt_poll_ns<vcpu->kvm->max_halt_poll_ns&&-block_ns<vcpu->kvm->max_halt_poll_ns)+halt_ns<vcpu->kvm->max_halt_poll_ns)grow_halt_poll_ns(vcpu);}else{vcpu->halt_poll_ns=0;}}-trace_kvm_vcpu_wakeup(block_ns,waited,vcpu_valid_wakeup(vcpu));+trace_kvm_vcpu_wakeup(halt_ns,waited,vcpu_valid_wakeup(vcpu));}-EXPORT_SYMBOL_GPL(kvm_vcpu_block);+EXPORT_SYMBOL_GPL(kvm_vcpu_halt);boolkvm_vcpu_wake_up(structkvm_vcpu*vcpu){
From: David Matlack <dmatlack@google.com> Date: 2021-09-28 22:03:21
On Fri, Sep 24, 2021 at 05:55:24PM -0700, Sean Christopherson wrote:
Factor out the "block" part of kvm_vcpu_halt() so that x86 can emulate
non-halt wait/sleep/block conditions that should not be subjected to
halt-polling.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: David Matlack <dmatlack@google.com> Date: 2021-09-28 22:04:57
On Fri, Sep 24, 2021 at 05:55:25PM -0700, Sean Christopherson wrote:
From: Jing Zhang <redacted>
Add a "blocking" stat that userspace can use to detect the case where a
vCPU is not being run because of a vCPU/guest action, e.g. HLT or WFS on
x86, WFI on arm64, etc... Current guest/host/halt stats don't show this
well, e.g. if a guest halts for a long period of time then the vCPU could
appear pathologically blocked due to a host condition, when in reality the
vCPU has been put into a not-runnable state by the guest.
Originally-by: Cannon Matthews [off-list ref]
Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Jing Zhang <redacted>
[sean: renamed stat to "blocking", massaged changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: David Matlack <dmatlack@google.com> Date: 2021-09-28 22:08:42
On Fri, Sep 24, 2021 at 05:55:26PM -0700, Sean Christopherson wrote:
Calculate the halt-polling "stop" time using "cur" instead of redoing
ktime_get(). In the happy case where hardware correctly predicts
do_halt_poll, "cur" is only a few cycles old. And if the branch is
mispredicted, arguably that extra latency should count toward the
halt-polling time.
In all likelihood, the numbers involved are in the noise and either
approach is perfectly ok.
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: David Matlack <dmatlack@google.com> Date: 2021-09-28 22:13:00
On Fri, Sep 24, 2021 at 05:55:27PM -0700, Sean Christopherson wrote:
Go directly to kvm_vcpu_block() when handling the case where userspace
attempts to run an UNINITIALIZED vCPU. The vCPU isn't halted and its time
spent in limbo arguably should not be factored into halt-polling as the
behavior of the VM at this point is not at all indicative of the behavior
of the VM once it is up and running, i.e. executing HLT in idle tasks.
Note, because this case is encountered only on the first run of an AP vCPU,
vcpu->halt_poll_ns is guaranteed to be '0', and so KVM will not attempt
halt-polling, i.e. this really only affects the post-block bookkeeping.
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: David Matlack <dmatlack@google.com> Date: 2021-09-28 22:15:08
On Fri, Sep 24, 2021 at 05:55:28PM -0700, Sean Christopherson wrote:
Call kvm_vcpu_block() directly for all wait states except HALTED so that
kvm_vcpu_halt() is no longer a misnomer on x86.
Functionally, this means KVM will never attempt halt-polling or adjust
vcpu->halt_poll_ns for INIT_RECEIVED (a.k.a. Wait-For-SIPI (WFS)) or
AP_RESET_HOLD; UNINITIALIZED is handled in kvm_arch_vcpu_ioctl_run(),
and x86 doesn't use any other "wait" states.
As mentioned above, the motivation of this is purely so that "halt" isn't
overloaded on x86, e.g. in KVM's stats. Skipping halt-polling for WFS
(and RESET_HOLD) has no meaningful effect on guest performance as there
are typically single-digit numbers of INIT-SIPI sequences per AP vCPU,
per boot, versus thousands of HLTs just to boot to console.
Signed-off-by: Sean Christopherson <seanjc@google.com>
From: Christian Borntraeger <hidden> Date: 2021-09-29 06:57:50
Am 27.09.21 um 18:58 schrieb David Matlack:
On Mon, Sep 27, 2021 at 8:17 AM Christian Borntraeger
[off-list ref] wrote:
quoted
Am 27.09.21 um 17:03 schrieb Paolo Bonzini:
quoted
On 27/09/21 16:59, Sean Christopherson wrote:
quoted
quoted
commit acd05785e48c01edb2c4f4d014d28478b5f19fb5
Author: David Matlack[off-list ref]
AuthorDate: Fri Apr 17 15:14:46 2020 -0700
Commit: Paolo Bonzini[off-list ref]
CommitDate: Fri Apr 24 12:53:17 2020 -0400
kvm: add capability for halt polling
broke the possibility for an admin to disable halt polling for already running KVM guests.
In past times doing
echo 0 > /sys/module/kvm/parameters/halt_poll_ns
stopped polling system wide.
Now all KVM guests will use the halt_poll_ns value that was active during
startup - even those that do not use KVM_CAP_HALT_POLL.
I guess this was not intended?
No, but...
quoted
I would go so far as to say that halt_poll_ns should be a hard limit on
the capability
... this would not be a good idea I think. Anything that wants to do a lot of polling can just do "for (;;)".
I agree. It would also be a maintenance burden and subtle "gotcha" to
have to increase halt_poll_ns anytime one wants to increase
KVM_CAP_HALT_POLL.
I think the idea of the upper bound is not about preventing wasting CPUs
but to reconfigure existing poll intervals on a global level. So I think
this idea is a bad idea in itself. Especially as the admin might not have
access to the monitor of user QEMUs.
quoted
quoted
So I think there are two possibilities that makes sense:
* track what is using KVM_CAP_HALT_POLL, and make writes to halt_poll_ns follow that
what about using halt_poll_ns for those VMs that did not uses KVM_CAP_HALT_POLL and the private number for those that did.
None of these options would cover Christian's original use-case
though. (Write to module to disable halt-polling system-wide.)
What about adding a writable "enable_halt_polling" module parameter
that would then affect both classes with and without KVM_CAP_HALT_POLL.
that affects all VMs? Once that is in place we could also consider
getting rid of halt_poll_ns entirely.
As far as I can tell QEMU does not yet use KVM_CAP_HALT_POLL.
So having a system wide halt_poll_ns makes sense. And I think for all
processes not using KVM_CAP_HALT_POLL we should really follow what
halt_poll_ns is NOW and not what it used to be.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-09-30 09:36:26
On Tue, 28 Sep 2021 17:21:12 +0100,
Sean Christopherson [off-list ref] wrote:
On Tue, Sep 28, 2021, Marc Zyngier wrote:
quoted
On Mon, 27 Sep 2021 18:28:14 +0100,
Sean Christopherson [off-list ref] wrote:
quoted
On Sun, Sep 26, 2021, Marc Zyngier wrote:
quoted
On Sun, 26 Sep 2021 07:27:28 +0100,
Paolo Bonzini [off-list ref] wrote:
quoted
On 25/09/21 11:50, Marc Zyngier wrote:
quoted
quoted
there is no need for arm64 to put/load
the vGIC as KVM hasn't relinquished control of the vCPU in any way.
This doesn't mean that there is no requirement for any state
change. The put/load on GICv4 is crucial for performance, and the VMCR
resync is a correctness requirement.
Ah crud, I didn't blame that code beforehand, I simply assumed
kvm_arch_vcpu_blocking() was purely for the blocking/schedule()
sequence. The comment in arm64's kvm_arch_vcpu_blocking() about
kvm_arch_vcpu_runnable() makes more sense now too.
quoted
quoted
I wouldn't even say it's crucial for performance: halt polling cannot
work and is a waste of time without (the current implementation of)
put/load.
Not quite. A non-V{LPI,SGI} could still be used as the a wake-up from
WFI (which is the only reason we end-up on this path). Only LPIs (and
SGIs on GICv4.1) can be directly injected, meaning that SPIs and PPIs
still follow the standard SW injection model.
However, there is still the ICH_VMCR_EL2 requirement (to get the
up-to-date priority mask and group enable bits) for SW-injected
interrupt wake-up to work correctly, and I really don't want to save
that one eagerly on each shallow exit.
IIUC, VMCR is resident in hardware while the guest is running, and
KVM needs to retrieve the VMCR when processing interrupts to
determine if a interrupt is above the priority threshold. If that's
the case, then IMO handling the VMCR via an arch hook is
unnecessarily fragile, e.g. any generic call that leads to
kvm_arch_vcpu_runnable() needs to know that arm64 lazily retrieves a
guest register.
Not quite. We only need to retrieve the VMCR if we are in a situation
where we need to trigger a wake-up from WFI at the point where we have
not done a vcpu_put() yet. All the other cases where the interrupt is
injected are managed by the HW. And the only case where
kvm_arch_vcpu_runnable() gets called is when blocking.
I also don't get why a hook would be fragile, as long as it has well
defined semantics.
Generic KVM should not have to know that a seemingly benign arch hook,
kvm_arch_vcpu_runnable(), cannot be safely called without first calling another
arch hook. E.g. I suspect there's a (benign?) race in kvm_vcpu_on_spin(). If
the loop is delayed between checking rcuwait_active() and vcpu_dy_runnable(),
and the target vCPU is awakened during that period, KVM can call
kvm_arch_vcpu_runnable() while the vCPU is running.
Humph. Indeed, there is a potential gold-plated turd there.
It's kind of a counter-example to my below suggestion as putting the vGIC would
indeed lead to state corruption if the vCPU is running, but I would argue that
arm64 should override kvm_arch_dy_runnable() so that its correctness is guaranteed,
e.g. by not calling kvm_arch_vcpu_runnable() if the vCPU is already running.
I'll work something out for that case.
quoted
quoted
A better approach for VMCR would be to retrieve the value from
hardware on-demand, e.g. via a hook in vgic_get_vmcr(), so that it's all but
impossible to have bugs where KVM is working with a stale VMCR, e.g.
But most of the uses of vgic_get_vmcr() are in contexts where the vcpu
isn't running at all (such as save/restore). It really only operates
on the shadow state, and what you have above will only lead to state
corruption.
Ignoring the kvm_arch_dy_runnable() case for the moment, how would
it lead to corruption? The idea is that the 'vmcr_available' flag
would be cleared when the vCPU is run, i.e. it tracks whether or not
the shadow state may be stale.
I guess that 'vmcr_available' would have to be initialised to 'true'
at vcpu reset time so that the userspace side cannot trigger a read
from the HW.
quoted
quoted
if (kvm_vgic_global_state.type == VGIC_V2)
vgic_v2_get_vmcr(vcpu, vmcr);
else
Regarding vGIC v4, does KVM require it to be resident in hardware
while the vCPU is loaded?
It is a requirement. Otherwise, we end-up with an inconsistent state
between the delivery of doorbells and the state of the vgic.
For my own understanding, does KVM require it to be resident in
hardware while the vCPU is loaded but _not_ running? What I don't
fully understand is how KVM can safely load/put the vCPU if that
true, i.e. wouldn't there always be a window for badness?
No, that part is fine. It is when you start running the vcpu without
the GICv4 context loaded that ugly stuff happens (get a doorbell that
tells you to schedule the currently running vcpu, for example).
quoted
Also, reloading the GICv4 state can be pretty expensive (multiple MMIO
accesses), which is why we really don't want to do that on the hot path
(kvm_arch_vcpu_ioctl_run() *is* a hot path).
I wasn't suggesting to reload GICv4 on every entry, it would only be reloaded
if it was put between vcpu_load() and entry to the guest.
quoted
quoted
If not, then we could do something like
this, which would eliminate the arch hooks entirely if the VMCR is
handled as above.
...
quoted
quoted
@@ -813,6 +787,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) */ preempt_disable();+ /*+ * Reload vGIC v4 if necessary, as it may be put on-demand so+ * that KVM can detect directly injected interrupts, e.g. when+ * determining if the vCPU is runnable due to a pending event.+ */+ vgic_v4_load(vcpu);
You'd need to detect that a previous put has been done.
Not that it will likely matter, but doesn't the its_vpe.resident
check automatically handle this?
Sort of. I eventually want to get rid of this as it papers over all
sort of sins. I introduced it exactly because of the nesting that
vcpu_block triggers, but this is a bit of a layering violation between
KVM and the underlying GICv4 driver.
quoted hunk
quoted
But overall, it puts the complexity at the wrong place. WFI (aka
kvm_vcpu_block) is the place where we want to handle this synchronisation,
and not the run loop.
Instead of having a well defined interface with the blocking code
where we implement the required synchronisation, you spray the vgic
crap all over, and it becomes much harder to reason about it. Guess
what, I'm not keen on it.
My objection to the arch hooks is that, from generic KVM's
perspective, the direct dependency is not on blocking, it's on
calling kvm_arch_vcpu_runnable(). That's why I suggested handling
this by tracking whether or not the VMCR is up-to-date/stale, as it
allows generic KVM to safely call kvm_arch_vcpu_runnable() whenever
the vCPU is loaded.
I don't have a strong opinion on arm64 preferring the sync to be
specific to WFI, but if that's the case then IMO this should be
handled fully in arm64, e.g. a patch like so (or with a wrapper
around the call to kvm_vcpu_block() if we want to guard against
future calls into generic KVM)
@@ -95,8 +95,28 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu)}else{trace_kvm_wfx_arm64(*vcpu_pc(vcpu),false);vcpu->stat.wfi_exit_stat++;++/*+*SyncbackthestateoftheGICCPUinterfacesothatwehave+*thelatestPMRandgroupenables.Thisensuresthat+*kvm_arch_vcpu_runnablehasup-to-datedatatodecidewhether+*wehavependinginterrupts,e.g.whendeterminingifthe+*vCPUshouldblock.+*+*Forthesamereason,wewanttotellGICv4thatweneed+*doorbellstobesignalled,shouldaninterruptbecomepending.+*/+preempt_disable();+kvm_vgic_vmcr_sync(vcpu);+vgic_v4_put(vcpu,true);+preempt_enable();+kvm_vcpu_block(vcpu);kvm_clear_request(KVM_REQ_UNHALT,vcpu);++preempt_disable();+vgic_v4_load(vcpu);+preempt_enable();}kvm_incr_pc(vcpu);
I actually largely prefer this approach, which is massively more
readable than the current setup. Feel free to wrap that in your
series.
I'll also have a look at the vcpu_dy_runnable() asap.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Sean, Paolo,
I recently also notice the behavior change of param halt_poll_ns.
Now it loses the ability to:
1) dynamically disable halt polling for all the running VMs
by `echo 0 > /sys`
2) dynamically adjust the halt polling interval for all the
running VMs by `echo * > /sys`
While in our cases, we usually use above two abilities, and
KVM_CAP_HALT_POLL is not used yet.
On 2021/9/28 1:33, Sean Christopherson wrote:
On Mon, Sep 27, 2021, Paolo Bonzini wrote:
quoted
On Mon, Sep 27, 2021 at 5:17 PM Christian Borntraeger
[off-list ref] wrote:
quoted
quoted
So I think there are two possibilities that makes sense:
* track what is using KVM_CAP_HALT_POLL, and make writes to halt_poll_ns follow that
what about using halt_poll_ns for those VMs that did not uses KVM_CAP_HALT_POLL and the private number for those that did.
Yes, that's what I meant. David pointed out that doesn't allow you to
disable halt polling altogether, but for that you can always ask each
VM's userspace one by one, or just not use KVM_CAP_HALT_POLL. (Also, I
don't know about Google's usecase, but mine was actually more about
using KVM_CAP_HALT_POLL to *disable* halt polling on some VMs!).
I kinda like the idea if special-casing halt_poll_ns=0, e.g. for testing or
in-the-field mitigation if halt-polling is broken. It'd be trivial to support, e.g.
Do we have any plan to repost the diff as a fix?
I would be very nice that this issue can be solved.
Besides, I think we may need some Doc for users to describe
how halt_poll_ns works with KVM_CAP_HALT_POLL, like
"Documentation/virt/guest-halt-polling.rst".
quoted hunk
@@ -3304,19 +3304,23 @@ void kvm_vcpu_halt(struct kvm_vcpu *vcpu) update_halt_poll_stats(vcpu, start, poll_end, !waited); if (halt_poll_allowed) {+ max_halt_poll_ns = vcpu->kvm->max_halt_poll_ns;+ if (!max_halt_poll_ns || !halt_poll_ns) <------ squish the max if halt_poll_ns==0+ max_halt_poll_ns = halt_poll_ns;+
Does this mean that KVM_CAP_HALT_POLL will not be able to
disable halt polling for a VM individually when halt_poll_ns !=0?
if (!vcpu_valid_wakeup(vcpu)) {
shrink_halt_poll_ns(vcpu);
- } else if (vcpu->kvm->max_halt_poll_ns) {
+ } else if (max_halt_poll_ns) {
if (halt_ns <= vcpu->halt_poll_ns)
;
/* we had a long block, shrink polling */
else if (vcpu->halt_poll_ns &&
- halt_ns > vcpu->kvm->max_halt_poll_ns)
+ halt_ns > max_halt_poll_ns)
shrink_halt_poll_ns(vcpu);
/* we had a short halt and our poll time is too small */
- else if (vcpu->halt_poll_ns < vcpu->kvm->max_halt_poll_ns &&
- halt_ns < vcpu->kvm->max_halt_poll_ns)
- grow_halt_poll_ns(vcpu);
+ else if (vcpu->halt_poll_ns < max_halt_poll_ns &&
+ halt_ns < max_halt_poll_ns)
+ grow_halt_poll_ns(vcpu, max_halt_poll_ns);
} else {
vcpu->halt_poll_ns = 0;
}
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
.
From: David Matlack <dmatlack@google.com> Date: 2022-11-16 17:19:51
On Tue, Nov 15, 2022 at 11:28:56AM +0800, wangyanan (Y) wrote:
Hi Sean, Paolo,
I recently also notice the behavior change of param halt_poll_ns.
Now it loses the ability to:
1) dynamically disable halt polling for all the running VMs
by `echo 0 > /sys`
2) dynamically adjust the halt polling interval for all the
running VMs by `echo * > /sys`
While in our cases, we usually use above two abilities, and
KVM_CAP_HALT_POLL is not used yet.
I think the right path forward is to make KVM_CAP_HALT_POLL a pure
override of halt_poll_ns, and restore the pre-existing behavior of
halt_poll_ns whenever KVM_CAP_HALT_POLL is not used. e.g. see the patch
below.
That will fix issues (1) and (2) above for any VM not using
KVM_CAP_HALT_POLL. If a VM is using KVM_CAP_HALT_POLL, it will ignore
all changes to halt_poll_ns. If we truly need a mechanism for admins to
disable halt-polling on VMs using KVM_CAP_HALT_POLL, we can introduce a
separate module parameter for that. But IMO, any setup that is
sophisticated enough to use KVM_CAP_HALT_POLL should also be able to use
KVM_CAP_HALT_POLL to disable halt polling.
If everyone is happy with this approach I can test and send a real patch
to the mailing list.
@@ -3545,17 +3550,16 @@ void kvm_vcpu_halt(struct kvm_vcpu *vcpu)if(halt_poll_allowed){if(!vcpu_valid_wakeup(vcpu)){shrink_halt_poll_ns(vcpu);-}elseif(vcpu->kvm->max_halt_poll_ns){+}elseif(max_halt_poll_ns){if(halt_ns<=vcpu->halt_poll_ns);/* we had a long block, shrink polling */-elseif(vcpu->halt_poll_ns&&-halt_ns>vcpu->kvm->max_halt_poll_ns)+elseif(vcpu->halt_poll_ns&&halt_ns>max_halt_poll_ns)shrink_halt_poll_ns(vcpu);/* we had a short halt and our poll time is too small */-elseif(vcpu->halt_poll_ns<vcpu->kvm->max_halt_poll_ns&&-halt_ns<vcpu->kvm->max_halt_poll_ns)-grow_halt_poll_ns(vcpu);+elseif(vcpu->halt_poll_ns<max_halt_poll_ns&&+halt_ns<max_halt_poll_ns)+grow_halt_poll_ns(vcpu,max_halt_poll_ns);}else{vcpu->halt_poll_ns=0;}
@@ -4588,6 +4592,7 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,if(cap->flags||cap->args[0]!=(unsignedint)cap->args[0])return-EINVAL;+kvm->override_halt_poll_ns=true;kvm->max_halt_poll_ns=cap->args[0];return0;}
On 2021/9/28 1:33, Sean Christopherson wrote:
quoted
On Mon, Sep 27, 2021, Paolo Bonzini wrote:
quoted
On Mon, Sep 27, 2021 at 5:17 PM Christian Borntraeger
[off-list ref] wrote:
quoted
quoted
So I think there are two possibilities that makes sense:
* track what is using KVM_CAP_HALT_POLL, and make writes to halt_poll_ns follow that
what about using halt_poll_ns for those VMs that did not uses KVM_CAP_HALT_POLL and the private number for those that did.
Yes, that's what I meant. David pointed out that doesn't allow you to
disable halt polling altogether, but for that you can always ask each
VM's userspace one by one, or just not use KVM_CAP_HALT_POLL. (Also, I
don't know about Google's usecase, but mine was actually more about
using KVM_CAP_HALT_POLL to *disable* halt polling on some VMs!).
I kinda like the idea if special-casing halt_poll_ns=0, e.g. for testing or
in-the-field mitigation if halt-polling is broken. It'd be trivial to support, e.g.
Do we have any plan to repost the diff as a fix?
I would be very nice that this issue can be solved.
Besides, I think we may need some Doc for users to describe
how halt_poll_ns works with KVM_CAP_HALT_POLL, like
"Documentation/virt/guest-halt-polling.rst".
quoted
@@ -3304,19 +3304,23 @@ void kvm_vcpu_halt(struct kvm_vcpu *vcpu) update_halt_poll_stats(vcpu, start, poll_end, !waited); if (halt_poll_allowed) {+ max_halt_poll_ns = vcpu->kvm->max_halt_poll_ns;+ if (!max_halt_poll_ns || !halt_poll_ns) <------ squish the max if halt_poll_ns==0+ max_halt_poll_ns = halt_poll_ns;+
Does this mean that KVM_CAP_HALT_POLL will not be able to
disable halt polling for a VM individually when halt_poll_ns !=0?
quoted
if (!vcpu_valid_wakeup(vcpu)) {
shrink_halt_poll_ns(vcpu);
- } else if (vcpu->kvm->max_halt_poll_ns) {
+ } else if (max_halt_poll_ns) {
if (halt_ns <= vcpu->halt_poll_ns)
;
/* we had a long block, shrink polling */
else if (vcpu->halt_poll_ns &&
- halt_ns > vcpu->kvm->max_halt_poll_ns)
+ halt_ns > max_halt_poll_ns)
shrink_halt_poll_ns(vcpu);
/* we had a short halt and our poll time is too small */
- else if (vcpu->halt_poll_ns < vcpu->kvm->max_halt_poll_ns &&
- halt_ns < vcpu->kvm->max_halt_poll_ns)
- grow_halt_poll_ns(vcpu);
+ else if (vcpu->halt_poll_ns < max_halt_poll_ns &&
+ halt_ns < max_halt_poll_ns)
+ grow_halt_poll_ns(vcpu, max_halt_poll_ns);
} else {
vcpu->halt_poll_ns = 0;
}
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
.
On Tue, Nov 15, 2022 at 11:28:56AM +0800, wangyanan (Y) wrote:
quoted
Hi Sean, Paolo,
I recently also notice the behavior change of param halt_poll_ns.
Now it loses the ability to:
1) dynamically disable halt polling for all the running VMs
by `echo 0 > /sys`
2) dynamically adjust the halt polling interval for all the
running VMs by `echo * > /sys`
While in our cases, we usually use above two abilities, and
KVM_CAP_HALT_POLL is not used yet.
I think the right path forward is to make KVM_CAP_HALT_POLL a pure
override of halt_poll_ns, and restore the pre-existing behavior of
halt_poll_ns whenever KVM_CAP_HALT_POLL is not used. e.g. see the patch
below.
Agree with this.
kvm.halt_poll_ns serves like a legacy method to control halt polling
globally. Once KVM_CAP_HALT_POLL is used for a VM, it should
hold 100% responsibility to control on the VM, including disabling
the polling. This strategy helps to keep the two mechanisms
decoupled.
quoted hunk
That will fix issues (1) and (2) above for any VM not using
KVM_CAP_HALT_POLL. If a VM is using KVM_CAP_HALT_POLL, it will ignore
all changes to halt_poll_ns. If we truly need a mechanism for admins to
disable halt-polling on VMs using KVM_CAP_HALT_POLL, we can introduce a
separate module parameter for that. But IMO, any setup that is
sophisticated enough to use KVM_CAP_HALT_POLL should also be able to use
KVM_CAP_HALT_POLL to disable halt polling.
If everyone is happy with this approach I can test and send a real patch
to the mailing list.
@@ -3545,17 +3550,16 @@ void kvm_vcpu_halt(struct kvm_vcpu *vcpu)if(halt_poll_allowed){if(!vcpu_valid_wakeup(vcpu)){shrink_halt_poll_ns(vcpu);-}elseif(vcpu->kvm->max_halt_poll_ns){+}elseif(max_halt_poll_ns){if(halt_ns<=vcpu->halt_poll_ns);/* we had a long block, shrink polling */-elseif(vcpu->halt_poll_ns&&-halt_ns>vcpu->kvm->max_halt_poll_ns)+elseif(vcpu->halt_poll_ns&&halt_ns>max_halt_poll_ns)shrink_halt_poll_ns(vcpu);/* we had a short halt and our poll time is too small */-elseif(vcpu->halt_poll_ns<vcpu->kvm->max_halt_poll_ns&&-halt_ns<vcpu->kvm->max_halt_poll_ns)-grow_halt_poll_ns(vcpu);+elseif(vcpu->halt_poll_ns<max_halt_poll_ns&&+halt_ns<max_halt_poll_ns)+grow_halt_poll_ns(vcpu,max_halt_poll_ns);}else{vcpu->halt_poll_ns=0;}
@@ -4588,6 +4592,7 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,if(cap->flags||cap->args[0]!=(unsignedint)cap->args[0])return-EINVAL;+kvm->override_halt_poll_ns=true;kvm->max_halt_poll_ns=cap->args[0];return0;}
Looks sensible to me overall.
I will look at the RFC series, thanks for your quick response.
Yanan
.
quoted
On 2021/9/28 1:33, Sean Christopherson wrote:
quoted
On Mon, Sep 27, 2021, Paolo Bonzini wrote:
quoted
On Mon, Sep 27, 2021 at 5:17 PM Christian Borntraeger
[off-list ref] wrote:
quoted
quoted
So I think there are two possibilities that makes sense:
* track what is using KVM_CAP_HALT_POLL, and make writes to halt_poll_ns follow that
what about using halt_poll_ns for those VMs that did not uses KVM_CAP_HALT_POLL and the private number for those that did.
Yes, that's what I meant. David pointed out that doesn't allow you to
disable halt polling altogether, but for that you can always ask each
VM's userspace one by one, or just not use KVM_CAP_HALT_POLL. (Also, I
don't know about Google's usecase, but mine was actually more about
using KVM_CAP_HALT_POLL to *disable* halt polling on some VMs!).
I kinda like the idea if special-casing halt_poll_ns=0, e.g. for testing or
in-the-field mitigation if halt-polling is broken. It'd be trivial to support, e.g.
Do we have any plan to repost the diff as a fix?
I would be very nice that this issue can be solved.
Besides, I think we may need some Doc for users to describe
how halt_poll_ns works with KVM_CAP_HALT_POLL, like
"Documentation/virt/guest-halt-polling.rst".
quoted
@@ -3304,19 +3304,23 @@ void kvm_vcpu_halt(struct kvm_vcpu *vcpu) update_halt_poll_stats(vcpu, start, poll_end, !waited); if (halt_poll_allowed) {+ max_halt_poll_ns = vcpu->kvm->max_halt_poll_ns;+ if (!max_halt_poll_ns || !halt_poll_ns) <------ squish the max if halt_poll_ns==0+ max_halt_poll_ns = halt_poll_ns;+
Does this mean that KVM_CAP_HALT_POLL will not be able to
disable halt polling for a VM individually when halt_poll_ns !=0?
quoted
if (!vcpu_valid_wakeup(vcpu)) {
shrink_halt_poll_ns(vcpu);
- } else if (vcpu->kvm->max_halt_poll_ns) {
+ } else if (max_halt_poll_ns) {
if (halt_ns <= vcpu->halt_poll_ns)
;
/* we had a long block, shrink polling */
else if (vcpu->halt_poll_ns &&
- halt_ns > vcpu->kvm->max_halt_poll_ns)
+ halt_ns > max_halt_poll_ns)
shrink_halt_poll_ns(vcpu);
/* we had a short halt and our poll time is too small */
- else if (vcpu->halt_poll_ns < vcpu->kvm->max_halt_poll_ns &&
- halt_ns < vcpu->kvm->max_halt_poll_ns)
- grow_halt_poll_ns(vcpu);
+ else if (vcpu->halt_poll_ns < max_halt_poll_ns &&
+ halt_ns < max_halt_poll_ns)
+ grow_halt_poll_ns(vcpu, max_halt_poll_ns);
} else {
vcpu->halt_poll_ns = 0;
}
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
.