Re: [PATCH v2 1/3] powerpc/xive: Untangle xive from child interrupt controller drivers

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Nam Cao <namcao@linutronix.de> writes:

> xive-specific data is stored in handler_data. This creates a mess, as xive
> has to rely on child interrupt controller drivers to clean up this data, as
> was done by 9a014f45688 ("powerpc/pseries/pci: Add a msi_free() handler to
> clear XIVE data").
>
> Instead, store xive-specific data in chip_data and untangle the child
> drivers.
>
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---
> v2: no change
> ---
>  arch/powerpc/include/asm/xive.h           |  1 -
>  arch/powerpc/platforms/powernv/pci-ioda.c | 21 +-------
>  arch/powerpc/platforms/pseries/msi.c      | 18 +------
>  arch/powerpc/sysdev/xive/common.c         | 63 +++++++++++------------
>  4 files changed, 33 insertions(+), 70 deletions(-)


Hi Nam, 

I am facing kernel crash on host when trying to run kvm pseries guest on
powernv host. Looking it a bit more closely, I see that we are missing
conversion of xxx_irq_handler_data()) to xxx_irq_chip_data() at few other
places, including in powerpc KVM code. 

<Crash signature>

BUG: Kernel NULL pointer dereference on read at 0x00000010
Faulting instruction address: 0xc0000000001c0704
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Radix  SMP NR_CPUS=2048 NUMA PowerNV
CPU: 103 UID: 0 PID: 2742 Comm: qemu-system-ppc Not tainted 6.17.0-01737-g50c19e20ed2e #1 NONE
<...>
NIP:  c0000000001c0704 LR: c0000000001c06f8 CTR: 0000000000000000
REGS: c0000000627476a0 TRAP: 0300   Not tainted  (6.17.0-01737-g50c19e20ed2e)
MSR:  9000000000009033 <SF,HV,EE,ME,IR,DR,RI,LE>  CR: 88044488  XER: 00000036
CFAR: c0000000002c20d8 DAR: 0000000000000010 DSISR: 40000000 IRQMASK: 0 
<...>
NIP [c0000000001c0704] kvmppc_xive_attach_escalation+0x174/0x240
LR [c0000000001c06f8] kvmppc_xive_attach_escalation+0x168/0x240
Call Trace:
  kvmppc_xive_attach_escalation+0x168/0x240 (unreliable)
  kvmppc_xive_connect_vcpu+0x2a0/0x4c0
  kvm_arch_vcpu_ioctl+0x354/0x470
  kvm_vcpu_ioctl+0x488/0x9a0
  sys_ioctl+0x4ec/0x1030
  system_call_exception+0x104/0x2b0
  system_call_vectored_common+0x15c/0x2ec



Here is the diff which fixed this.. 

diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
index 1302b5ac5672..c029c6cc82ef 100644
--- a/arch/powerpc/kvm/book3s_xive.c
+++ b/arch/powerpc/kvm/book3s_xive.c
@@ -917,7 +917,7 @@ int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
         */
        if (single_escalation) {
                struct irq_data *d = irq_get_irq_data(xc->esc_virq[prio]);
-               struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+               struct xive_irq_data *xd = irq_data_get_irq_chip_data(d);

                xive_vm_esb_load(xd, XIVE_ESB_SET_PQ_01);
                vcpu->arch.xive_esc_raddr = xd->eoi_page;
@@ -1612,7 +1612,7 @@ int kvmppc_xive_set_mapped(struct kvm *kvm, unsigned long guest_irq,

        /* Grab info about irq */
        state->pt_number = hw_irq;
-       state->pt_data = irq_data_get_irq_handler_data(host_data);
+       state->pt_data = irq_data_get_irq_chip_data(host_data);

        /*
         * Configure the IRQ to match the existing configuration of
@@ -1788,7 +1788,7 @@ void kvmppc_xive_disable_vcpu_interrupts(struct kvm_vcpu *vcpu)
 void xive_cleanup_single_escalation(struct kvm_vcpu *vcpu, int irq)
 {
        struct irq_data *d = irq_get_irq_data(irq);
-       struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+       struct xive_irq_data *xd = irq_data_get_irq_chip_data(d);

        /*
         * This slightly odd sequence gives the right result
@@ -2829,7 +2829,7 @@ int kvmppc_xive_debug_show_queues(struct seq_file *m, struct kvm_vcpu *vcpu)
                if (xc->esc_virq[i]) {
                        struct irq_data *d = irq_get_irq_data(xc->esc_virq[i]);
                        struct xive_irq_data *xd =
-                               irq_data_get_irq_handler_data(d);
+                               irq_data_get_irq_chip_data(d);
                        u64 pq = xive_vm_esb_load(xd, XIVE_ESB_GET);

                        seq_printf(m, "    ESC %d %c%c EOI @%llx",


... However grepping for "handler_data" in arch/powerpc I see there is
atleast one more place where we may still need the fix.. There are few
more places which grep returned - but I am not sure if they all really need
the fix. But I guess VAS should be fixed i.e :

arch/powerpc/platforms/powernv/vas.c:   xd = irq_get_handler_data(vinst->virq);


Would you like to submit an official patch for converting these other places too?


Thanks!
-ritesh