Re: [PATCH v5 5/7] ppc: Reset the interrupt presenter from the CPU reset handler
On Tue, Oct 22, 2019 at 06:38:10PM +0200, Cédric Le Goater wrote:
> On the sPAPR machine and PowerNV machine, the interrupt presenters are
> created by a machine handler at the core level and are reset
> independently. This is not consistent and it raises issues when it
> comes to handle hot-plugged CPUs. In that case, the presenters are not
> reset. This is less of an issue in XICS, although a zero MFFR could
> be a concern, but in XIVE, the OS CAM line is not set and this breaks
> the presenting algorithm. The current code has workarounds which need
> a global cleanup.
>
> Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
> cpu_intc_reset() handler called by the CPU reset handler and remove
> the XiveTCTX reset handler which is now redundant.
>
> Signed-off-by: Cédric Le Goater
Both XiveTCTX and ICPState are QOM subclasses of TYPE_DEVICE. I think
that means you could avoid the new hook by using dc->reset on those
classes instead. It wouldn't get called automatically, of course,
since it's not on a bus, but you could call it explicitly via
device_reset() from the place you currently call the new hook.
> ---
> include/hw/ppc/pnv.h | 1 +
> include/hw/ppc/spapr_irq.h | 2 ++
> include/hw/ppc/xics.h | 1 +
> include/hw/ppc/xive.h | 1 +
> hw/intc/spapr_xive.c | 9 +
> hw/intc/xics.c | 8 ++--
> hw/intc/xics_spapr.c | 7 +++
> hw/intc/xive.c | 12 +---
> hw/ppc/pnv.c | 18 ++
> hw/ppc/pnv_core.c | 7 +--
> hw/ppc/spapr_cpu_core.c| 5 -
> hw/ppc/spapr_irq.c | 14 ++
> 12 files changed, 65 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 1cdbe55bf86c..2a780e633f23 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -111,6 +111,7 @@ typedef struct PnvChipClass {
>
> uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
> void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
> +void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
> ISABus *(*isa_create)(PnvChip *chip, Error **errp);
> void (*dt_populate)(PnvChip *chip, void *fdt);
> void (*pic_print_info)(PnvChip *chip, Monitor *mon);
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..09232999b07e 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
> */
> int (*cpu_intc_create)(SpaprInterruptController *intc,
> PowerPCCPU *cpu, Error **errp);
> +void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
> int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
> Error **errp);
> void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +69,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>
> int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>PowerPCCPU *cpu, Error **errp);
> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu);
> void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon);
> void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
>void *fdt, uint32_t phandle);
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 1e6a9300eb2b..602173c12250 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
> uint32_t icp_accept(ICPState *ss);
> uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
> void icp_eoi(ICPState *icp, uint32_t xirr);
> +void icp_reset(ICPState *icp);
>
> void ics_write_xive(ICSState *ics, int nr, int server,
> uint8_t priority, uint8_t saved_priority);
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index fd3319bd3202..99381639f50c 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -415,6 +415,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset,
> unsigned size);
>
> void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
> Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
> +void xive_tctx_reset(XiveTCTX *tctx);
>
> static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
> {
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index ba32d2cc5b0f..20a8d8285f64 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,14 @@ static int
> spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu)
> +{
> +XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> +
> +xive_tctx_reset(tctx);
>
Re: [PATCH v5 5/7] ppc: Reset the interrupt presenter from the CPU reset handler
On 10/22/19 6:38 PM, Cédric Le Goater wrote:
On the sPAPR machine and PowerNV machine, the interrupt presenters are
created by a machine handler at the core level and are reset
independently. This is not consistent and it raises issues when it
comes to handle hot-plugged CPUs. In that case, the presenters are not
reset. This is less of an issue in XICS, although a zero MFFR could
be a concern, but in XIVE, the OS CAM line is not set and this breaks
the presenting algorithm. The current code has workarounds which need
a global cleanup.
Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
cpu_intc_reset() handler called by the CPU reset handler and remove
the XiveTCTX reset handler which is now redundant.
Signed-off-by: Cédric Le Goater
---
include/hw/ppc/pnv.h | 1 +
include/hw/ppc/spapr_irq.h | 2 ++
include/hw/ppc/xics.h | 1 +
include/hw/ppc/xive.h | 1 +
hw/intc/spapr_xive.c | 9 +
hw/intc/xics.c | 8 ++--
hw/intc/xics_spapr.c | 7 +++
hw/intc/xive.c | 12 +---
hw/ppc/pnv.c | 18 ++
hw/ppc/pnv_core.c | 7 +--
hw/ppc/spapr_cpu_core.c| 5 -
hw/ppc/spapr_irq.c | 14 ++
12 files changed, 65 insertions(+), 20 deletions(-)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 1cdbe55bf86c..2a780e633f23 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -111,6 +111,7 @@ typedef struct PnvChipClass {
uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
+void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
ISABus *(*isa_create)(PnvChip *chip, Error **errp);
void (*dt_populate)(PnvChip *chip, void *fdt);
void (*pic_print_info)(PnvChip *chip, Monitor *mon);
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index 5e150a667902..09232999b07e 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
*/
int (*cpu_intc_create)(SpaprInterruptController *intc,
PowerPCCPU *cpu, Error **errp);
+void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
Error **errp);
void (*free_irq)(SpaprInterruptController *intc, int irq);
@@ -68,6 +69,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
PowerPCCPU *cpu, Error **errp);
+void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu);
void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon);
void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
void *fdt, uint32_t phandle);
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 1e6a9300eb2b..602173c12250 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
uint32_t icp_accept(ICPState *ss);
uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
void icp_eoi(ICPState *icp, uint32_t xirr);
+void icp_reset(ICPState *icp);
void ics_write_xive(ICSState *ics, int nr, int server,
uint8_t priority, uint8_t saved_priority);
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index fd3319bd3202..99381639f50c 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -415,6 +415,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset,
unsigned size);
void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
+void xive_tctx_reset(XiveTCTX *tctx);
static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
{
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index ba32d2cc5b0f..20a8d8285f64 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -553,6 +553,14 @@ static int
spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
return 0;
}
+static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
+ PowerPCCPU *cpu)
Indent off :|
+{
+XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
+
+xive_tctx_reset(tctx);
+}
+
static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int
val)
{
SpaprXive *xive = SPAPR_XIVE(intc);
@@ -697,6 +705,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void
*data)
sicc->activate = spapr_xive_activate;
sicc->deactivate = spapr_xive_deactivate;
sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
+sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
sicc->claim_irq = spapr_xive_claim_irq;
Re: [PATCH v5 5/7] ppc: Reset the interrupt presenter from the CPU reset handler
On Tue, 22 Oct 2019 18:38:10 +0200
Cédric Le Goater wrote:
> On the sPAPR machine and PowerNV machine, the interrupt presenters are
> created by a machine handler at the core level and are reset
> independently. This is not consistent and it raises issues when it
> comes to handle hot-plugged CPUs. In that case, the presenters are not
> reset. This is less of an issue in XICS, although a zero MFFR could
> be a concern, but in XIVE, the OS CAM line is not set and this breaks
> the presenting algorithm. The current code has workarounds which need
> a global cleanup.
>
> Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
> cpu_intc_reset() handler called by the CPU reset handler and remove
> the XiveTCTX reset handler which is now redundant.
>
> Signed-off-by: Cédric Le Goater
> ---
Reviewed-by: Greg Kurz
> include/hw/ppc/pnv.h | 1 +
> include/hw/ppc/spapr_irq.h | 2 ++
> include/hw/ppc/xics.h | 1 +
> include/hw/ppc/xive.h | 1 +
> hw/intc/spapr_xive.c | 9 +
> hw/intc/xics.c | 8 ++--
> hw/intc/xics_spapr.c | 7 +++
> hw/intc/xive.c | 12 +---
> hw/ppc/pnv.c | 18 ++
> hw/ppc/pnv_core.c | 7 +--
> hw/ppc/spapr_cpu_core.c| 5 -
> hw/ppc/spapr_irq.c | 14 ++
> 12 files changed, 65 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 1cdbe55bf86c..2a780e633f23 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -111,6 +111,7 @@ typedef struct PnvChipClass {
>
> uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
> void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
> +void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
> ISABus *(*isa_create)(PnvChip *chip, Error **errp);
> void (*dt_populate)(PnvChip *chip, void *fdt);
> void (*pic_print_info)(PnvChip *chip, Monitor *mon);
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..09232999b07e 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
> */
> int (*cpu_intc_create)(SpaprInterruptController *intc,
> PowerPCCPU *cpu, Error **errp);
> +void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
> int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
> Error **errp);
> void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +69,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>
> int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>PowerPCCPU *cpu, Error **errp);
> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu);
> void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon);
> void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
>void *fdt, uint32_t phandle);
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 1e6a9300eb2b..602173c12250 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
> uint32_t icp_accept(ICPState *ss);
> uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
> void icp_eoi(ICPState *icp, uint32_t xirr);
> +void icp_reset(ICPState *icp);
>
> void ics_write_xive(ICSState *ics, int nr, int server,
> uint8_t priority, uint8_t saved_priority);
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index fd3319bd3202..99381639f50c 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -415,6 +415,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset,
> unsigned size);
>
> void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
> Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
> +void xive_tctx_reset(XiveTCTX *tctx);
>
> static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
> {
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index ba32d2cc5b0f..20a8d8285f64 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,14 @@ static int
> spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> return 0;
> }
>
> +static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> + PowerPCCPU *cpu)
> +{
> +XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> +
> +xive_tctx_reset(tctx);
> +}
> +
> static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int
> val)
> {
> SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +705,7 @@ static void spapr_xive_class_init(ObjectClass *klass,
> void *data)
> sicc->activate = spapr_xive_activate;
> sicc->deactivate =
[PATCH v5 5/7] ppc: Reset the interrupt presenter from the CPU reset handler
On the sPAPR machine and PowerNV machine, the interrupt presenters are
created by a machine handler at the core level and are reset
independently. This is not consistent and it raises issues when it
comes to handle hot-plugged CPUs. In that case, the presenters are not
reset. This is less of an issue in XICS, although a zero MFFR could
be a concern, but in XIVE, the OS CAM line is not set and this breaks
the presenting algorithm. The current code has workarounds which need
a global cleanup.
Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
cpu_intc_reset() handler called by the CPU reset handler and remove
the XiveTCTX reset handler which is now redundant.
Signed-off-by: Cédric Le Goater
---
include/hw/ppc/pnv.h | 1 +
include/hw/ppc/spapr_irq.h | 2 ++
include/hw/ppc/xics.h | 1 +
include/hw/ppc/xive.h | 1 +
hw/intc/spapr_xive.c | 9 +
hw/intc/xics.c | 8 ++--
hw/intc/xics_spapr.c | 7 +++
hw/intc/xive.c | 12 +---
hw/ppc/pnv.c | 18 ++
hw/ppc/pnv_core.c | 7 +--
hw/ppc/spapr_cpu_core.c| 5 -
hw/ppc/spapr_irq.c | 14 ++
12 files changed, 65 insertions(+), 20 deletions(-)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 1cdbe55bf86c..2a780e633f23 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -111,6 +111,7 @@ typedef struct PnvChipClass {
uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
+void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
ISABus *(*isa_create)(PnvChip *chip, Error **errp);
void (*dt_populate)(PnvChip *chip, void *fdt);
void (*pic_print_info)(PnvChip *chip, Monitor *mon);
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index 5e150a667902..09232999b07e 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
*/
int (*cpu_intc_create)(SpaprInterruptController *intc,
PowerPCCPU *cpu, Error **errp);
+void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
Error **errp);
void (*free_irq)(SpaprInterruptController *intc, int irq);
@@ -68,6 +69,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
PowerPCCPU *cpu, Error **errp);
+void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu);
void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon);
void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
void *fdt, uint32_t phandle);
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 1e6a9300eb2b..602173c12250 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
uint32_t icp_accept(ICPState *ss);
uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
void icp_eoi(ICPState *icp, uint32_t xirr);
+void icp_reset(ICPState *icp);
void ics_write_xive(ICSState *ics, int nr, int server,
uint8_t priority, uint8_t saved_priority);
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index fd3319bd3202..99381639f50c 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -415,6 +415,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset,
unsigned size);
void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
+void xive_tctx_reset(XiveTCTX *tctx);
static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
{
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index ba32d2cc5b0f..20a8d8285f64 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -553,6 +553,14 @@ static int
spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
return 0;
}
+static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
+ PowerPCCPU *cpu)
+{
+XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
+
+xive_tctx_reset(tctx);
+}
+
static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int
val)
{
SpaprXive *xive = SPAPR_XIVE(intc);
@@ -697,6 +705,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void
*data)
sicc->activate = spapr_xive_activate;
sicc->deactivate = spapr_xive_deactivate;
sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
+sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
sicc->claim_irq = spapr_xive_claim_irq;
sicc->free_irq = spapr_xive_free_irq;
sicc->set_irq = spapr_xive_set_irq;
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index
