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(-)
diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
index 92930b0b5d0e..efb0f5effcc6 100644
--- a/arch/powerpc/include/asm/xive.h
+++ b/arch/powerpc/include/asm/xive.h
@@ -111,7 +111,6 @@ void xive_native_free_vp_block(u32 vp_base);
int xive_native_populate_irq_data(u32 hw_irq,
struct xive_irq_data *data);
void xive_cleanup_irq_data(struct xive_irq_data *xd);
-void xive_irq_free_data(unsigned int virq);
void xive_native_free_irq(u32 irq);
int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq);
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index d8ccf2c9b98a..fb37098d4d58 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -37,7 +37,6 @@
#include <asm/firmware.h>
#include <asm/pnv-pci.h>
#include <asm/mmzone.h>
-#include <asm/xive.h>
#include "powernv.h"
#include "pci.h"
@@ -1707,23 +1706,6 @@ static int __pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
return 0;
}
-/*
- * The msi_free() op is called before irq_domain_free_irqs_top() when
- * the handler data is still available. Use that to clear the XIVE
- * controller.
- */
-static void pnv_msi_ops_msi_free(struct irq_domain *domain,
- struct msi_domain_info *info,
- unsigned int irq)
-{
- if (xive_enabled())
- xive_irq_free_data(irq);
-}
-
-static struct msi_domain_ops pnv_pci_msi_domain_ops = {
- .msi_free = pnv_msi_ops_msi_free,
-};
-
static void pnv_msi_shutdown(struct irq_data *d)
{
d = d->parent_data;
@@ -1754,7 +1736,6 @@ static struct irq_chip pnv_pci_msi_irq_chip = {
static struct msi_domain_info pnv_msi_domain_info = {
.flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX),
- .ops = &pnv_pci_msi_domain_ops,
.chip = &pnv_pci_msi_irq_chip,
};
@@ -1870,7 +1851,7 @@ static void pnv_irq_domain_free(struct irq_domain *domain, unsigned int virq,
virq, d->hwirq, nr_irqs);
msi_bitmap_free_hwirqs(&phb->msi_bmp, d->hwirq, nr_irqs);
- /* XIVE domain is cleared through ->msi_free() */
+ irq_domain_free_irqs_parent(domain, virq, nr_irqs);
}
static const struct irq_domain_ops pnv_irq_domain_ops = {
diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index ee1c8c6898a3..10712477938e 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -15,7 +15,6 @@
#include <asm/hw_irq.h>
#include <asm/ppc-pci.h>
#include <asm/machdep.h>
-#include <asm/xive.h>
#include "pseries.h"
@@ -436,19 +435,6 @@ static int pseries_msi_ops_prepare(struct irq_domain *domain, struct device *dev
return rtas_prepare_msi_irqs(pdev, nvec, type, arg);
}
-/*
- * ->msi_free() is called before irq_domain_free_irqs_top() when the
- * handler data is still available. Use that to clear the XIVE
- * controller data.
- */
-static void pseries_msi_ops_msi_free(struct irq_domain *domain,
- struct msi_domain_info *info,
- unsigned int irq)
-{
- if (xive_enabled())
- xive_irq_free_data(irq);
-}
-
/*
* RTAS can not disable one MSI at a time. It's all or nothing. Do it
* at the end after all IRQs have been freed.
@@ -463,7 +449,6 @@ static void pseries_msi_post_free(struct irq_domain *domain, struct device *dev)
static struct msi_domain_ops pseries_pci_msi_domain_ops = {
.msi_prepare = pseries_msi_ops_prepare,
- .msi_free = pseries_msi_ops_msi_free,
.msi_post_free = pseries_msi_post_free,
};
@@ -604,8 +589,7 @@ static void pseries_irq_domain_free(struct irq_domain *domain, unsigned int virq
struct pci_controller *phb = irq_data_get_irq_chip_data(d);
pr_debug("%s bridge %pOF %d #%d\n", __func__, phb->dn, virq, nr_irqs);
-
- /* XIVE domain data is cleared through ->msi_free() */
+ irq_domain_free_irqs_parent(domain, virq, nr_irqs);
}
static const struct irq_domain_ops pseries_irq_domain_ops = {
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index f10592405024..625361a15424 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -317,7 +317,7 @@ int xmon_xive_get_irq_config(u32 hw_irq, struct irq_data *d)
if (d) {
char buffer[128];
- xive_irq_data_dump(irq_data_get_irq_handler_data(d),
+ xive_irq_data_dump(irq_data_get_irq_chip_data(d),
buffer, sizeof(buffer));
xmon_printf("%s", buffer);
}
@@ -437,7 +437,7 @@ static void xive_do_source_eoi(struct xive_irq_data *xd)
/* irq_chip eoi callback, called with irq descriptor lock held */
static void xive_irq_eoi(struct irq_data *d)
{
- struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+ struct xive_irq_data *xd = irq_data_get_irq_chip_data(d);
struct xive_cpu *xc = __this_cpu_read(xive_cpu);
DBG_VERBOSE("eoi_irq: irq=%d [0x%lx] pending=%02x\n",
@@ -595,7 +595,7 @@ static int xive_pick_irq_target(struct irq_data *d,
const struct cpumask *affinity)
{
static unsigned int fuzz;
- struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+ struct xive_irq_data *xd = irq_data_get_irq_chip_data(d);
cpumask_var_t mask;
int cpu = -1;
@@ -628,7 +628,7 @@ static int xive_pick_irq_target(struct irq_data *d,
static unsigned int xive_irq_startup(struct irq_data *d)
{
- struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+ struct xive_irq_data *xd = irq_data_get_irq_chip_data(d);
unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
int target, rc;
@@ -673,7 +673,7 @@ static unsigned int xive_irq_startup(struct irq_data *d)
/* called with irq descriptor lock held */
static void xive_irq_shutdown(struct irq_data *d)
{
- struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+ struct xive_irq_data *xd = irq_data_get_irq_chip_data(d);
unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
pr_debug("%s: irq %d [0x%x] data @%p\n", __func__, d->irq, hw_irq, d);
@@ -698,7 +698,7 @@ static void xive_irq_shutdown(struct irq_data *d)
static void xive_irq_unmask(struct irq_data *d)
{
- struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+ struct xive_irq_data *xd = irq_data_get_irq_chip_data(d);
pr_debug("%s: irq %d data @%p\n", __func__, d->irq, xd);
@@ -707,7 +707,7 @@ static void xive_irq_unmask(struct irq_data *d)
static void xive_irq_mask(struct irq_data *d)
{
- struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+ struct xive_irq_data *xd = irq_data_get_irq_chip_data(d);
pr_debug("%s: irq %d data @%p\n", __func__, d->irq, xd);
@@ -718,7 +718,7 @@ static int xive_irq_set_affinity(struct irq_data *d,
const struct cpumask *cpumask,
bool force)
{
- struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+ struct xive_irq_data *xd = irq_data_get_irq_chip_data(d);
unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
u32 target, old_target;
int rc = 0;
@@ -776,7 +776,7 @@ static int xive_irq_set_affinity(struct irq_data *d,
static int xive_irq_set_type(struct irq_data *d, unsigned int flow_type)
{
- struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+ struct xive_irq_data *xd = irq_data_get_irq_chip_data(d);
/*
* We only support these. This has really no effect other than setting
@@ -815,7 +815,7 @@ static int xive_irq_set_type(struct irq_data *d, unsigned int flow_type)
static int xive_irq_retrigger(struct irq_data *d)
{
- 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 should be only for MSIs */
if (WARN_ON(xd->flags & XIVE_IRQ_FLAG_LSI))
@@ -837,7 +837,7 @@ static int xive_irq_retrigger(struct irq_data *d)
*/
static int xive_irq_set_vcpu_affinity(struct irq_data *d, void *state)
{
- struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
+ struct xive_irq_data *xd = irq_data_get_irq_chip_data(d);
unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
int rc;
u8 pq;
@@ -951,7 +951,7 @@ static int xive_irq_set_vcpu_affinity(struct irq_data *d, void *state)
static int xive_get_irqchip_state(struct irq_data *data,
enum irqchip_irq_state which, bool *state)
{
- struct xive_irq_data *xd = irq_data_get_irq_handler_data(data);
+ struct xive_irq_data *xd = irq_data_get_irq_chip_data(data);
u8 pq;
switch (which) {
@@ -1011,21 +1011,20 @@ void xive_cleanup_irq_data(struct xive_irq_data *xd)
}
EXPORT_SYMBOL_GPL(xive_cleanup_irq_data);
-static int xive_irq_alloc_data(unsigned int virq, irq_hw_number_t hw)
+static struct xive_irq_data *xive_irq_alloc_data(unsigned int virq, irq_hw_number_t hw)
{
struct xive_irq_data *xd;
int rc;
xd = kzalloc(sizeof(struct xive_irq_data), GFP_KERNEL);
if (!xd)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
rc = xive_ops->populate_irq_data(hw, xd);
if (rc) {
kfree(xd);
- return rc;
+ return ERR_PTR(rc);
}
xd->target = XIVE_INVALID_TARGET;
- irq_set_handler_data(virq, xd);
/*
* Turn OFF by default the interrupt being mapped. A side
@@ -1036,20 +1035,19 @@ static int xive_irq_alloc_data(unsigned int virq, irq_hw_number_t hw)
*/
xive_esb_read(xd, XIVE_ESB_SET_PQ_01);
- return 0;
+ return xd;
}
-void xive_irq_free_data(unsigned int virq)
+static void xive_irq_free_data(unsigned int virq)
{
- struct xive_irq_data *xd = irq_get_handler_data(virq);
+ struct xive_irq_data *xd = irq_get_chip_data(virq);
if (!xd)
return;
- irq_set_handler_data(virq, NULL);
+ irq_set_chip_data(virq, NULL);
xive_cleanup_irq_data(xd);
kfree(xd);
}
-EXPORT_SYMBOL_GPL(xive_irq_free_data);
#ifdef CONFIG_SMP
@@ -1286,7 +1284,7 @@ void __init xive_smp_probe(void)
static int xive_irq_domain_map(struct irq_domain *h, unsigned int virq,
irq_hw_number_t hw)
{
- int rc;
+ struct xive_irq_data *xd;
/*
* Mark interrupts as edge sensitive by default so that resend
@@ -1294,11 +1292,12 @@ static int xive_irq_domain_map(struct irq_domain *h, unsigned int virq,
*/
irq_clear_status_flags(virq, IRQ_LEVEL);
- rc = xive_irq_alloc_data(virq, hw);
- if (rc)
- return rc;
+ xd = xive_irq_alloc_data(virq, hw);
+ if (IS_ERR(xd))
+ return PTR_ERR(xd);
irq_set_chip_and_handler(virq, &xive_irq_chip, handle_fasteoi_irq);
+ irq_set_chip_data(virq, xd);
return 0;
}
@@ -1366,7 +1365,7 @@ static void xive_irq_domain_debug_show(struct seq_file *m, struct irq_domain *d,
seq_printf(m, "%*sXIVE:\n", ind, "");
ind++;
- xd = irq_data_get_irq_handler_data(irqd);
+ xd = irq_data_get_irq_chip_data(irqd);
if (!xd) {
seq_printf(m, "%*snot assigned\n", ind, "");
return;
@@ -1403,6 +1402,7 @@ static int xive_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
unsigned int nr_irqs, void *arg)
{
struct irq_fwspec *fwspec = arg;
+ struct xive_irq_data *xd;
irq_hw_number_t hwirq;
unsigned int type = IRQ_TYPE_NONE;
int i, rc;
@@ -1423,12 +1423,11 @@ static int xive_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
irq_clear_status_flags(virq, IRQ_LEVEL);
/* allocates and sets handler data */
- rc = xive_irq_alloc_data(virq + i, hwirq + i);
- if (rc)
- return rc;
+ xd = xive_irq_alloc_data(virq + i, hwirq + i);
+ if (IS_ERR(xd))
+ return PTR_ERR(xd);
- irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
- &xive_irq_chip, domain->host_data);
+ irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, &xive_irq_chip, xd);
irq_set_handler(virq + i, handle_fasteoi_irq);
}
@@ -1764,7 +1763,7 @@ static void xive_debug_show_irq(struct seq_file *m, struct irq_data *d)
seq_printf(m, "IRQ 0x%08x : target=0x%x prio=%02x lirq=0x%x ",
hw_irq, target, prio, lirq);
- xive_irq_data_dump(irq_data_get_irq_handler_data(d), buffer, sizeof(buffer));
+ xive_irq_data_dump(irq_data_get_irq_chip_data(d), buffer, sizeof(buffer));
seq_puts(m, buffer);
seq_puts(m, "\n");
}
--
2.39.5Nam 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
Hi Ritesh, Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes: > 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. [snip] > Here is the diff which fixed this.. [snip] > ... 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 for the report. I didn't expect struct xive_irq_data to be used in multiple files while making that patch, sorry about that! Something like your patch should do the job. However, my gut feeling is that multiple files shouldn't share a single irq struct this way. Let me stare at it... Nam
Nam Cao <namcao@linutronix.de> writes: > Hi Ritesh, > > Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes: >> 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. > [snip] >> Here is the diff which fixed this.. > [snip] >> ... 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 for the report. I didn't expect struct xive_irq_data to be used > in multiple files while making that patch, sorry about that! > > Something like your patch should do the job. However, my gut feeling is > that multiple files shouldn't share a single irq struct this way. Let me > stare at it... I *think* we can do a cleanup. But I don't think it would be trivial enough for 6.18. Let's do as you suggested for now to get KVM functional again. Nam
© 2016 - 2026 Red Hat, Inc.