| Message ID | 20211009021236.4122790-11-seanjc@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: Halt-polling and x86 APICv overhaul | expand |
On 09/10/21 04:12, Sean Christopherson wrote: > Move the put and reload of the vGIC out of the block/unblock callbacks > and into a dedicated WFI helper. Functionally, this is nearly a nop as > the block hook is called at the very beginning of kvm_vcpu_block(), and > the only code in kvm_vcpu_block() after the unblock hook is to update the > halt-polling controls, i.e. can only affect the next WFI. > > Back when the arch (un)blocking hooks were added by commits 3217f7c25bca > ("KVM: Add kvm_arch_vcpu_{un}blocking callbacks) and d35268da6687 > ("arm/arm64: KVM: arch_timer: Only schedule soft timer on vcpu_block"), > the hooks were invoked only when KVM was about to "block", i.e. schedule > out the vCPU. The use case at the time was to schedule a timer in the > host based on the earliest timer in the guest in order to wake the > blocking vCPU when the emulated guest timer fired. Commit accb99bcd0ca > ("KVM: arm/arm64: Simplify bg_timer programming") reworked the timer > logic to be even more precise, by waiting until the vCPU was actually > scheduled out, and so move the timer logic from the (un)blocking hooks to > vcpu_load/put. > > In the meantime, the hooks gained usage for enabling vGIC v4 doorbells in > commit df9ba95993b9 ("KVM: arm/arm64: GICv4: Use the doorbell interrupt > as an unblocking source"), and added related logic for the VMCR in commit > 5eeaf10eec39 ("KVM: arm/arm64: Sync ICH_VMCR_EL2 back when about to block"). > > Finally, commit 07ab0f8d9a12 ("KVM: Call kvm_arch_vcpu_blocking early > into the blocking sequence") hoisted the (un)blocking hooks so that they > wrapped KVM's halt-polling logic in addition to the core "block" logic. > > In other words, the original need for arch hooks to take action _only_ > in the block path is long since gone. > > Cc: Oliver Upton <oupton@google.com> > Cc: Marc Zyngier <maz@kernel.org> > Signed-off-by: Sean Christopherson <seanjc@google.com> This needs a word on why kvm_psci_vcpu_suspend does not need the hooks. Or it needs to be changed to also use kvm_vcpu_wfi in the PSCI code, I don't know. Marc, can you review and/or advise? Thanks, Paolo > --- > arch/arm64/include/asm/kvm_emulate.h | 2 ++ > arch/arm64/kvm/arm.c | 52 +++++++++++++++++++--------- > arch/arm64/kvm/handle_exit.c | 3 +- > 3 files changed, 38 insertions(+), 19 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index fd418955e31e..de8b4f5922b7 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -41,6 +41,8 @@ void kvm_inject_vabt(struct kvm_vcpu *vcpu); > void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr); > void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr); > > +void kvm_vcpu_wfi(struct kvm_vcpu *vcpu); > + > static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu) > { > return !(vcpu->arch.hcr_el2 & HCR_RW); > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 7838e9fb693e..1346f81b34df 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -359,27 +359,12 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) > > void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) > { > - /* > - * If we're about to block (most likely because we've just hit a > - * WFI), we need to sync back the state of the GIC CPU interface > - * so that we have the latest PMR and group enables. This ensures > - * that kvm_arch_vcpu_runnable has up-to-date data to decide > - * whether we have pending interrupts. > - * > - * For the same reason, we want to tell GICv4 that we need > - * doorbells to be signalled, should an interrupt become pending. > - */ > - preempt_disable(); > - kvm_vgic_vmcr_sync(vcpu); > - vgic_v4_put(vcpu, true); > - preempt_enable(); > + > } > > void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) > { > - preempt_disable(); > - vgic_v4_load(vcpu); > - preempt_enable(); > + > } > > void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) > @@ -662,6 +647,39 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu) > smp_rmb(); > } > > +/** > + * kvm_vcpu_wfi - emulate Wait-For-Interrupt behavior > + * @vcpu: The VCPU pointer > + * > + * Suspend execution of a vCPU until a valid wake event is detected, i.e. until > + * the vCPU is runnable. The vCPU may or may not be scheduled out, depending > + * on when a wake event arrives, e.g. there may already be a pending wake event. > + */ > +void kvm_vcpu_wfi(struct kvm_vcpu *vcpu) > +{ > + /* > + * Sync back the state of the GIC CPU interface so that we have > + * the latest PMR and group enables. This ensures that > + * kvm_arch_vcpu_runnable has up-to-date data to decide whether > + * we have pending interrupts, e.g. when determining if the > + * vCPU should block. > + * > + * For the same reason, we want to tell GICv4 that we need > + * doorbells to be signalled, should an interrupt become pending. > + */ > + 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(); > +} > + > static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu) > { > return vcpu->arch.target >= 0; > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index 275a27368a04..4794563a506b 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -95,8 +95,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu) > } else { > trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false); > vcpu->stat.wfi_exit_stat++; > - kvm_vcpu_block(vcpu); > - kvm_clear_request(KVM_REQ_UNHALT, vcpu); > + kvm_vcpu_wfi(vcpu); > } > > kvm_incr_pc(vcpu); >
On Mon, 25 Oct 2021 14:31:48 +0100, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 09/10/21 04:12, Sean Christopherson wrote: > > Move the put and reload of the vGIC out of the block/unblock callbacks > > and into a dedicated WFI helper. Functionally, this is nearly a nop as > > the block hook is called at the very beginning of kvm_vcpu_block(), and > > the only code in kvm_vcpu_block() after the unblock hook is to update the > > halt-polling controls, i.e. can only affect the next WFI. > > > > Back when the arch (un)blocking hooks were added by commits 3217f7c25bca > > ("KVM: Add kvm_arch_vcpu_{un}blocking callbacks) and d35268da6687 > > ("arm/arm64: KVM: arch_timer: Only schedule soft timer on vcpu_block"), > > the hooks were invoked only when KVM was about to "block", i.e. schedule > > out the vCPU. The use case at the time was to schedule a timer in the > > host based on the earliest timer in the guest in order to wake the > > blocking vCPU when the emulated guest timer fired. Commit accb99bcd0ca > > ("KVM: arm/arm64: Simplify bg_timer programming") reworked the timer > > logic to be even more precise, by waiting until the vCPU was actually > > scheduled out, and so move the timer logic from the (un)blocking hooks to > > vcpu_load/put. > > > > In the meantime, the hooks gained usage for enabling vGIC v4 doorbells in > > commit df9ba95993b9 ("KVM: arm/arm64: GICv4: Use the doorbell interrupt > > as an unblocking source"), and added related logic for the VMCR in commit > > 5eeaf10eec39 ("KVM: arm/arm64: Sync ICH_VMCR_EL2 back when about to block"). > > > > Finally, commit 07ab0f8d9a12 ("KVM: Call kvm_arch_vcpu_blocking early > > into the blocking sequence") hoisted the (un)blocking hooks so that they > > wrapped KVM's halt-polling logic in addition to the core "block" logic. > > > > In other words, the original need for arch hooks to take action _only_ > > in the block path is long since gone. > > > > Cc: Oliver Upton <oupton@google.com> > > Cc: Marc Zyngier <maz@kernel.org> > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > This needs a word on why kvm_psci_vcpu_suspend does not need the > hooks. Or it needs to be changed to also use kvm_vcpu_wfi in the PSCI > code, I don't know. > > Marc, can you review and/or advise? I was looking at that over the weekend, and that's a pre-existing bug. I would have addressed it independently, but it looks like you already have queued the patch. I guess I'll have to revisit this once the whole thing lands somewhere. M.
On 26/10/21 17:41, Marc Zyngier wrote: >> This needs a word on why kvm_psci_vcpu_suspend does not need the >> hooks. Or it needs to be changed to also use kvm_vcpu_wfi in the PSCI >> code, I don't know. >> >> Marc, can you review and/or advise? > I was looking at that over the weekend, and that's a pre-existing > bug. I would have addressed it independently, but it looks like you > already have queued the patch. I have "queued" it, but that's just my queue - it's not on kernel.org and it's not going to be in 5.16, at least not in the first batch. There's plenty of time for me to rebase on top of a fix, if you want to send the fix through your kvm-arm pull request. Just Cc me so that I understand what's going on. Thanks, Paolo
On 10/26/21 18:12, Paolo Bonzini wrote: > On 26/10/21 17:41, Marc Zyngier wrote: >>> This needs a word on why kvm_psci_vcpu_suspend does not need the >>> hooks. Or it needs to be changed to also use kvm_vcpu_wfi in the PSCI >>> code, I don't know. >>> >>> Marc, can you review and/or advise? >> I was looking at that over the weekend, and that's a pre-existing >> bug. I would have addressed it independently, but it looks like you >> already have queued the patch. > > I have "queued" it, but that's just my queue - it's not on kernel.org > and it's not going to be in 5.16, at least not in the first batch. > > There's plenty of time for me to rebase on top of a fix, if you want to > send the fix through your kvm-arm pull request. Just Cc me so that I > understand what's going on. Since a month has passed and I didn't see anything related in the KVM-ARM pull requests, I am going to queue this patch. Any conflicts can be resolved through a kvmarm->kvm merge of either a topic branch or a tag that is destined to 5.16. Paolo
On 2021-11-30 11:39, Paolo Bonzini wrote: > On 10/26/21 18:12, Paolo Bonzini wrote: >> On 26/10/21 17:41, Marc Zyngier wrote: >>>> This needs a word on why kvm_psci_vcpu_suspend does not need the >>>> hooks. Or it needs to be changed to also use kvm_vcpu_wfi in the >>>> PSCI >>>> code, I don't know. >>>> >>>> Marc, can you review and/or advise? >>> I was looking at that over the weekend, and that's a pre-existing >>> bug. I would have addressed it independently, but it looks like you >>> already have queued the patch. >> >> I have "queued" it, but that's just my queue - it's not on kernel.org >> and it's not going to be in 5.16, at least not in the first batch. >> >> There's plenty of time for me to rebase on top of a fix, if you want >> to send the fix through your kvm-arm pull request. Just Cc me so that >> I understand what's going on. > > Since a month has passed and I didn't see anything related in the > KVM-ARM pull requests, I am going to queue this patch. Any conflicts > can be resolved through a kvmarm->kvm merge of either a topic branch > or a tag that is destined to 5.16. Can you at least spell out *when* this will land? There is, in general, a certain lack of clarity about what you are queuing, where you are queuing it, and what release it targets. Thanks, M.
On 11/30/21 13:04, Marc Zyngier wrote: >>> >>> I have "queued" it, but that's just my queue - it's not on kernel.org >>> and it's not going to be in 5.16, at least not in the first batch. >>> >>> There's plenty of time for me to rebase on top of a fix, if you want >>> to send the fix through your kvm-arm pull request. Just Cc me so >>> that I understand what's going on. >> >> Since a month has passed and I didn't see anything related in the >> KVM-ARM pull requests, I am going to queue this patch. Any conflicts >> can be resolved through a kvmarm->kvm merge of either a topic branch >> or a tag that is destined to 5.16. > > Can you at least spell out *when* this will land? It will be in kvm/next as soon as I finish running tests on it, which may take a couple more days because I'm updating my machines to newer operating systems. > There is, in general, a certain lack of clarity about what you are queuing, > where you are queuing it, and what release it targets. Ok, thanks for the suggestion. Generally speaking: - kvm/master is stuff that is merged and will be in the next -rc, right now 5.16-rc4. It shouldn't ever rewind (though it may happen, it is rare) - kvm/next is stuff that is merged and will be in the next merge window, right now 5.17. It also shouldn't rewind. - kvm/queue is stuff that the submitter shouldn't care about, and that other people should only care about to check for conflicts. When I say I "queued" a patch it goes in kvm/queue, and there's time to remove it if something breaks. Regarding this series: - I am queuing it up to this patch - I am queuing it to kvm/next, meaning it targets 5.17 - it looks like the next one (11/43) triggers a known AMD errata, so I'm holding on the rest until we understand if it actually does, and if so if AMD AVIC is doomed. For the time being, it will stay in kvm/queue. Paolo
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index fd418955e31e..de8b4f5922b7 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -41,6 +41,8 @@ void kvm_inject_vabt(struct kvm_vcpu *vcpu); void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr); void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr); +void kvm_vcpu_wfi(struct kvm_vcpu *vcpu); + static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu) { return !(vcpu->arch.hcr_el2 & HCR_RW); diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 7838e9fb693e..1346f81b34df 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -359,27 +359,12 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) { - /* - * If we're about to block (most likely because we've just hit a - * WFI), we need to sync back the state of the GIC CPU interface - * so that we have the latest PMR and group enables. This ensures - * that kvm_arch_vcpu_runnable has up-to-date data to decide - * whether we have pending interrupts. - * - * For the same reason, we want to tell GICv4 that we need - * doorbells to be signalled, should an interrupt become pending. - */ - preempt_disable(); - kvm_vgic_vmcr_sync(vcpu); - vgic_v4_put(vcpu, true); - preempt_enable(); + } void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) { - preempt_disable(); - vgic_v4_load(vcpu); - preempt_enable(); + } void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) @@ -662,6 +647,39 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu) smp_rmb(); } +/** + * kvm_vcpu_wfi - emulate Wait-For-Interrupt behavior + * @vcpu: The VCPU pointer + * + * Suspend execution of a vCPU until a valid wake event is detected, i.e. until + * the vCPU is runnable. The vCPU may or may not be scheduled out, depending + * on when a wake event arrives, e.g. there may already be a pending wake event. + */ +void kvm_vcpu_wfi(struct kvm_vcpu *vcpu) +{ + /* + * Sync back the state of the GIC CPU interface so that we have + * the latest PMR and group enables. This ensures that + * kvm_arch_vcpu_runnable has up-to-date data to decide whether + * we have pending interrupts, e.g. when determining if the + * vCPU should block. + * + * For the same reason, we want to tell GICv4 that we need + * doorbells to be signalled, should an interrupt become pending. + */ + 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(); +} + static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu) { return vcpu->arch.target >= 0; diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index 275a27368a04..4794563a506b 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -95,8 +95,7 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu) } else { trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false); vcpu->stat.wfi_exit_stat++; - kvm_vcpu_block(vcpu); - kvm_clear_request(KVM_REQ_UNHALT, vcpu); + kvm_vcpu_wfi(vcpu); } kvm_incr_pc(vcpu);
Move the put and reload of the vGIC out of the block/unblock callbacks and into a dedicated WFI helper. Functionally, this is nearly a nop as the block hook is called at the very beginning of kvm_vcpu_block(), and the only code in kvm_vcpu_block() after the unblock hook is to update the halt-polling controls, i.e. can only affect the next WFI. Back when the arch (un)blocking hooks were added by commits 3217f7c25bca ("KVM: Add kvm_arch_vcpu_{un}blocking callbacks) and d35268da6687 ("arm/arm64: KVM: arch_timer: Only schedule soft timer on vcpu_block"), the hooks were invoked only when KVM was about to "block", i.e. schedule out the vCPU. The use case at the time was to schedule a timer in the host based on the earliest timer in the guest in order to wake the blocking vCPU when the emulated guest timer fired. Commit accb99bcd0ca ("KVM: arm/arm64: Simplify bg_timer programming") reworked the timer logic to be even more precise, by waiting until the vCPU was actually scheduled out, and so move the timer logic from the (un)blocking hooks to vcpu_load/put. In the meantime, the hooks gained usage for enabling vGIC v4 doorbells in commit df9ba95993b9 ("KVM: arm/arm64: GICv4: Use the doorbell interrupt as an unblocking source"), and added related logic for the VMCR in commit 5eeaf10eec39 ("KVM: arm/arm64: Sync ICH_VMCR_EL2 back when about to block"). Finally, commit 07ab0f8d9a12 ("KVM: Call kvm_arch_vcpu_blocking early into the blocking sequence") hoisted the (un)blocking hooks so that they wrapped KVM's halt-polling logic in addition to the core "block" logic. In other words, the original need for arch hooks to take action _only_ in the block path is long since gone. Cc: Oliver Upton <oupton@google.com> Cc: Marc Zyngier <maz@kernel.org> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/arm64/include/asm/kvm_emulate.h | 2 ++ arch/arm64/kvm/arm.c | 52 +++++++++++++++++++--------- arch/arm64/kvm/handle_exit.c | 3 +- 3 files changed, 38 insertions(+), 19 deletions(-)