Re: [PATCH 1/2] powerpc: kvm: use generic transfer to guest mode work

[Shrikanth Hegde]

On 4/24/25 20:12, Sebastian Andrzej Siewior wrote:

Thanks Sebastian for taking a look.

On 2025-04-21 15:58:36 [+0530], Shrikanth Hegde wrote:

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 19f4d298d..123539642 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -80,8 +80,8 @@
  #include <asm/ultravisor.h>
  #include <asm/dtl.h>
  #include <asm/plpar_wrappers.h>
-
  #include <trace/events/ipi.h>
+#include <linux/entry-kvm.h>

#include "book3s.h"

  #include "book3s_hv.h"
@@ -4901,7 +4901,7 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 
time_limit,
        }

if (need_resched())

-               cond_resched();
+               schedule();

This looks unrelated and odd. I don't why but this should be a
cond_resched() so it can be optimized away on PREEMPT kernels.

This is needed, otherwise KVM on powerVM setup gets stuck on preempt=full/lazy.

        kvmppc_update_vpas(vcpu);

@@ -5097,10 +5097,11 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)

                return -EINVAL;
        }

- /* No need to go into the guest when all we'll do is come back out */

-       if (signal_pending(current)) {
-               run->exit_reason = KVM_EXIT_INTR;
-               return -EINTR;
+       /* use generic frameworks to handle signals, need_resched  */
+       if (__xfer_to_guest_mode_work_pending()) {
+               r = xfer_to_guest_mode_handle_work(vcpu);

This could be unconditional.

+               if (r)
+                       return r;
        }

#ifdef CONFIG_PPC_TRANSACTIONAL_MEM

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 153587741..4ff334532 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -34,6 +34,7 @@
  #endif
  #include <asm/ultravisor.h>
  #include <asm/setup.h>
+#include <linux/entry-kvm.h>

#include "timing.h"

  #include "../mm/mmu_decl.h"
@@ -80,24 +81,17 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
  {
        int r;

+ /* use generic framework to handle need resched and signals */

+       if (__xfer_to_guest_mode_work_pending()) {
+               r = xfer_to_guest_mode_handle_work(vcpu);

there is nothing special you do checking and handling the work. Couldn't
you invoke xfer_to_guest_mode_handle_work() unconditionally?

I followed what was in arch/x86/kvm/x86.c. Since xfer_to_guest_mode_handle_work 
does the same check
it makes sense to call it without checks too.

Will update in v2.

+               if (r)
+                       return r;
+       }
+
        WARN_ON(irqs_disabled());
        hard_irq_disable();

while (true) {

-               if (need_resched()) {
-                       local_irq_enable();
-                       cond_resched();
-                       hard_irq_disable();
-                       continue;
-               }
-
-               if (signal_pending(current)) {
-                       kvmppc_account_exit(vcpu, SIGNAL_EXITS);
-                       vcpu->run->exit_reason = KVM_EXIT_INTR;
-                       r = -EINTR;
-                       break;

I don't how this works but couldn't SIGNAL_EXITS vanish now that it
isn't updated anymore? The stat itself moves in kvm_handle_signal_exit()
to a different counter so it is not lost. The reader just needs to look
somewhere else for it.

ok. thanks for pointing out.

AFAIU it is updating the stats mostly. But below could keep the stats happy.
I will update that in v2.

        if (__xfer_to_guest_mode_work_pending()) {
                r = xfer_to_guest_mode_handle_work(vcpu);
+               /* generic framework doesn't update ppc specific stats*/
+               if (r == -EINTR)
+                       kvmppc_account_exit(vcpu, SIGNAL_EXITS);
                if (r)
                        return r;

-               }
-
                vcpu->mode = IN_GUEST_MODE;

/*

Sebastian