From: Lijun Pan <ljp@linux.vnet.ibm.com>
To: Paul Mackerras <paulus@ozlabs.org>
Cc: linuxppc-dev@ozlabs.org, kvm@vger.kernel.org,
kvm-ppc@vger.kernel.org,
David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH 2/2] powerpc/xive: Implement get_irqchip_state method for XIVE to fix shutdown race
Date: Mon, 12 Aug 2019 22:52:11 -0500 [thread overview]
Message-ID: <E547965E-CC31-470F-8849-0F2A899A121F@linux.vnet.ibm.com> (raw)
In-Reply-To: <20190812050743.aczgcqwmtqpkbx2l@oak.ozlabs.ibm.com>
> On Aug 12, 2019, at 12:07 AM, Paul Mackerras <paulus@ozlabs.org> wrote:
>
> ---
> arch/powerpc/include/asm/xive.h | 8 ++++
> arch/powerpc/kvm/book3s_xive.c | 31 ++++++++++++++
> arch/powerpc/sysdev/xive/common.c | 87 ++++++++++++++++++++++++++++-----------
> 3 files changed, 103 insertions(+), 23 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index e4016985764e..efb0e597b272 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -46,7 +46,15 @@ struct xive_irq_data {
>
> /* Setup/used by frontend */
> int target;
> + /*
> + * saved_p means that there is a queue entry for this interrupt
> + * in some CPU's queue (not including guest vcpu queues), even
> + * if P is not set in the source ESB.
> + * stale_p means that there is no queue entry for this interrupt
> + * in some CPU's queue, even if P is set in the source ESB.
> + */
> bool saved_p;
> + bool stale_p;
> };
> #define XIVE_IRQ_FLAG_STORE_EOI 0x01
> #define XIVE_IRQ_FLAG_LSI 0x02
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index 09f838aa3138..74eea009c095 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -160,6 +160,9 @@ static irqreturn_t xive_esc_irq(int irq, void *data)
> */
> vcpu->arch.xive_esc_on = false;
>
> + /* This orders xive_esc_on = false vs. subsequent stale_p = true */
> + smp_wmb(); /* goes with smp_mb() in cleanup_single_escalation */
> +
> return IRQ_HANDLED;
> }
>
> @@ -1113,6 +1116,31 @@ void kvmppc_xive_disable_vcpu_interrupts(struct kvm_vcpu *vcpu)
> vcpu->arch.xive_esc_raddr = 0;
> }
>
> +/*
> + * In single escalation mode, the escalation interrupt is marked so
> + * that EOI doesn't re-enable it, but just sets the stale_p flag to
> + * indicate that the P bit has already been dealt with. However, the
> + * assembly code that enters the guest sets PQ to 00 without clearing
> + * stale_p (because it has no easy way to address it). Hence we have
> + * to adjust stale_p before shutting down the interrupt.
> + */
> +static void cleanup_single_escalation(struct kvm_vcpu *vcpu,
> + struct kvmppc_xive_vcpu *xc, int irq)
> +{
> + struct irq_data *d = irq_get_irq_data(irq);
> + struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
> +
> + /*
> + * This slightly odd sequence gives the right result
> + * (i.e. stale_p set if xive_esc_on is false) even if
> + * we race with xive_esc_irq() and xive_irq_eoi().
> + */
Hi Paul,
I don’t quite understand the logic here.
Are you saying the code sequence is
vcpu->arch.xive_esc_on = false; (xive_esc_irq)
then
xd->stale_p = true; (cleanup_single_escaltion)
> + xd->stale_p = false;
> + smp_mb(); /* paired with smb_wmb in xive_esc_irq */
> + if (!vcpu->arch.xive_esc_on)
> + xd->stale_p = true;
> +}
> +
> void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
> {
> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> @@ -1137,6 +1165,9 @@ void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
> /* Free escalations */
> for (i = 0; i < KVMPPC_XIVE_Q_COUNT; i++) {
> if (xc->esc_virq[i]) {
> + if (xc->xive->single_escalation)
> + cleanup_single_escalation(vcpu, xc,
> + xc->esc_virq[i]);
> free_irq(xc->esc_virq[i], vcpu);
> irq_dispose_mapping(xc->esc_virq[i]);
> kfree(xc->esc_virq_names[i]);
> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
> index 1cdb39575eae..be86fce1a84e 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -135,7 +135,7 @@ static u32 xive_read_eq(struct xive_q *q, bool just_peek)
> static u32 xive_scan_interrupts(struct xive_cpu *xc, bool just_peek)
> {
> u32 irq = 0;
> - u8 prio;
> + u8 prio = 0;
>
> /* Find highest pending priority */
> while (xc->pending_prio != 0) {
> @@ -148,8 +148,19 @@ static u32 xive_scan_interrupts(struct xive_cpu *xc, bool just_peek)
> irq = xive_read_eq(&xc->queue[prio], just_peek);
>
> /* Found something ? That's it */
> - if (irq)
> - break;
> + if (irq) {
> + if (just_peek || irq_to_desc(irq))
> + break;
> + /*
> + * We should never get here; if we do then we must
> + * have failed to synchronize the interrupt properly
> + * when shutting it down.
> + */
> + pr_crit("xive: got interrupt %d without descriptor, dropping\n",
> + irq);
> + WARN_ON(1);
> + continue;
> + }
>
> /* Clear pending bits */
> xc->pending_prio &= ~(1 << prio);
> @@ -307,6 +318,7 @@ static void xive_do_queue_eoi(struct xive_cpu *xc)
> */
> static void xive_do_source_eoi(u32 hw_irq, struct xive_irq_data *xd)
> {
> + xd->stale_p = false;
> /* If the XIVE supports the new "store EOI facility, use it */
> if (xd->flags & XIVE_IRQ_FLAG_STORE_EOI)
> xive_esb_write(xd, XIVE_ESB_STORE_EOI, 0);
> @@ -350,7 +362,7 @@ static void xive_do_source_eoi(u32 hw_irq, struct xive_irq_data *xd)
> }
> }
>
> -/* irq_chip eoi callback */
> +/* 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);
> @@ -366,6 +378,8 @@ static void xive_irq_eoi(struct irq_data *d)
> if (!irqd_irq_disabled(d) && !irqd_is_forwarded_to_vcpu(d) &&
> !(xd->flags & XIVE_IRQ_NO_EOI))
> xive_do_source_eoi(irqd_to_hwirq(d), xd);
> + else
> + xd->stale_p = true;
>
> /*
> * Clear saved_p to indicate that it's no longer occupying
> @@ -397,11 +411,16 @@ static void xive_do_source_set_mask(struct xive_irq_data *xd,
> */
> if (mask) {
> val = xive_esb_read(xd, XIVE_ESB_SET_PQ_01);
> - xd->saved_p = !!(val & XIVE_ESB_VAL_P);
> - } else if (xd->saved_p)
> + if (!xd->stale_p && !!(val & XIVE_ESB_VAL_P))
> + xd->saved_p = true;
> + xd->stale_p = false;
> + } else if (xd->saved_p) {
> xive_esb_read(xd, XIVE_ESB_SET_PQ_10);
> - else
> + xd->saved_p = false;
Should we also explicitly set xd->stale_p = true; here?
> + } else {
> xive_esb_read(xd, XIVE_ESB_SET_PQ_00);
> + xd->stale_p = false;
Should we also explicitly set xd->saved_p = true; here?
Thanks,
Lijun
WARNING: multiple messages have this Message-ID (diff)
From: Lijun Pan <ljp@linux.vnet.ibm.com>
To: Paul Mackerras <paulus@ozlabs.org>
Cc: linuxppc-dev@ozlabs.org, kvm-ppc@vger.kernel.org,
kvm@vger.kernel.org, David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH 2/2] powerpc/xive: Implement get_irqchip_state method for XIVE to fix shutdown race
Date: Mon, 12 Aug 2019 22:52:11 -0500 [thread overview]
Message-ID: <E547965E-CC31-470F-8849-0F2A899A121F@linux.vnet.ibm.com> (raw)
In-Reply-To: <20190812050743.aczgcqwmtqpkbx2l@oak.ozlabs.ibm.com>
> On Aug 12, 2019, at 12:07 AM, Paul Mackerras <paulus@ozlabs.org> wrote:
>
> ---
> arch/powerpc/include/asm/xive.h | 8 ++++
> arch/powerpc/kvm/book3s_xive.c | 31 ++++++++++++++
> arch/powerpc/sysdev/xive/common.c | 87 ++++++++++++++++++++++++++++-----------
> 3 files changed, 103 insertions(+), 23 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index e4016985764e..efb0e597b272 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -46,7 +46,15 @@ struct xive_irq_data {
>
> /* Setup/used by frontend */
> int target;
> + /*
> + * saved_p means that there is a queue entry for this interrupt
> + * in some CPU's queue (not including guest vcpu queues), even
> + * if P is not set in the source ESB.
> + * stale_p means that there is no queue entry for this interrupt
> + * in some CPU's queue, even if P is set in the source ESB.
> + */
> bool saved_p;
> + bool stale_p;
> };
> #define XIVE_IRQ_FLAG_STORE_EOI 0x01
> #define XIVE_IRQ_FLAG_LSI 0x02
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index 09f838aa3138..74eea009c095 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -160,6 +160,9 @@ static irqreturn_t xive_esc_irq(int irq, void *data)
> */
> vcpu->arch.xive_esc_on = false;
>
> + /* This orders xive_esc_on = false vs. subsequent stale_p = true */
> + smp_wmb(); /* goes with smp_mb() in cleanup_single_escalation */
> +
> return IRQ_HANDLED;
> }
>
> @@ -1113,6 +1116,31 @@ void kvmppc_xive_disable_vcpu_interrupts(struct kvm_vcpu *vcpu)
> vcpu->arch.xive_esc_raddr = 0;
> }
>
> +/*
> + * In single escalation mode, the escalation interrupt is marked so
> + * that EOI doesn't re-enable it, but just sets the stale_p flag to
> + * indicate that the P bit has already been dealt with. However, the
> + * assembly code that enters the guest sets PQ to 00 without clearing
> + * stale_p (because it has no easy way to address it). Hence we have
> + * to adjust stale_p before shutting down the interrupt.
> + */
> +static void cleanup_single_escalation(struct kvm_vcpu *vcpu,
> + struct kvmppc_xive_vcpu *xc, int irq)
> +{
> + struct irq_data *d = irq_get_irq_data(irq);
> + struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
> +
> + /*
> + * This slightly odd sequence gives the right result
> + * (i.e. stale_p set if xive_esc_on is false) even if
> + * we race with xive_esc_irq() and xive_irq_eoi().
> + */
Hi Paul,
I don’t quite understand the logic here.
Are you saying the code sequence is
vcpu->arch.xive_esc_on = false; (xive_esc_irq)
then
xd->stale_p = true; (cleanup_single_escaltion)
> + xd->stale_p = false;
> + smp_mb(); /* paired with smb_wmb in xive_esc_irq */
> + if (!vcpu->arch.xive_esc_on)
> + xd->stale_p = true;
> +}
> +
> void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
> {
> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> @@ -1137,6 +1165,9 @@ void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
> /* Free escalations */
> for (i = 0; i < KVMPPC_XIVE_Q_COUNT; i++) {
> if (xc->esc_virq[i]) {
> + if (xc->xive->single_escalation)
> + cleanup_single_escalation(vcpu, xc,
> + xc->esc_virq[i]);
> free_irq(xc->esc_virq[i], vcpu);
> irq_dispose_mapping(xc->esc_virq[i]);
> kfree(xc->esc_virq_names[i]);
> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
> index 1cdb39575eae..be86fce1a84e 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -135,7 +135,7 @@ static u32 xive_read_eq(struct xive_q *q, bool just_peek)
> static u32 xive_scan_interrupts(struct xive_cpu *xc, bool just_peek)
> {
> u32 irq = 0;
> - u8 prio;
> + u8 prio = 0;
>
> /* Find highest pending priority */
> while (xc->pending_prio != 0) {
> @@ -148,8 +148,19 @@ static u32 xive_scan_interrupts(struct xive_cpu *xc, bool just_peek)
> irq = xive_read_eq(&xc->queue[prio], just_peek);
>
> /* Found something ? That's it */
> - if (irq)
> - break;
> + if (irq) {
> + if (just_peek || irq_to_desc(irq))
> + break;
> + /*
> + * We should never get here; if we do then we must
> + * have failed to synchronize the interrupt properly
> + * when shutting it down.
> + */
> + pr_crit("xive: got interrupt %d without descriptor, dropping\n",
> + irq);
> + WARN_ON(1);
> + continue;
> + }
>
> /* Clear pending bits */
> xc->pending_prio &= ~(1 << prio);
> @@ -307,6 +318,7 @@ static void xive_do_queue_eoi(struct xive_cpu *xc)
> */
> static void xive_do_source_eoi(u32 hw_irq, struct xive_irq_data *xd)
> {
> + xd->stale_p = false;
> /* If the XIVE supports the new "store EOI facility, use it */
> if (xd->flags & XIVE_IRQ_FLAG_STORE_EOI)
> xive_esb_write(xd, XIVE_ESB_STORE_EOI, 0);
> @@ -350,7 +362,7 @@ static void xive_do_source_eoi(u32 hw_irq, struct xive_irq_data *xd)
> }
> }
>
> -/* irq_chip eoi callback */
> +/* 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);
> @@ -366,6 +378,8 @@ static void xive_irq_eoi(struct irq_data *d)
> if (!irqd_irq_disabled(d) && !irqd_is_forwarded_to_vcpu(d) &&
> !(xd->flags & XIVE_IRQ_NO_EOI))
> xive_do_source_eoi(irqd_to_hwirq(d), xd);
> + else
> + xd->stale_p = true;
>
> /*
> * Clear saved_p to indicate that it's no longer occupying
> @@ -397,11 +411,16 @@ static void xive_do_source_set_mask(struct xive_irq_data *xd,
> */
> if (mask) {
> val = xive_esb_read(xd, XIVE_ESB_SET_PQ_01);
> - xd->saved_p = !!(val & XIVE_ESB_VAL_P);
> - } else if (xd->saved_p)
> + if (!xd->stale_p && !!(val & XIVE_ESB_VAL_P))
> + xd->saved_p = true;
> + xd->stale_p = false;
> + } else if (xd->saved_p) {
> xive_esb_read(xd, XIVE_ESB_SET_PQ_10);
> - else
> + xd->saved_p = false;
Should we also explicitly set xd->stale_p = true; here?
> + } else {
> xive_esb_read(xd, XIVE_ESB_SET_PQ_00);
> + xd->stale_p = false;
Should we also explicitly set xd->saved_p = true; here?
Thanks,
Lijun
WARNING: multiple messages have this Message-ID (diff)
From: Lijun Pan <ljp@linux.vnet.ibm.com>
To: Paul Mackerras <paulus@ozlabs.org>
Cc: linuxppc-dev@ozlabs.org, kvm@vger.kernel.org,
kvm-ppc@vger.kernel.org,
David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH 2/2] powerpc/xive: Implement get_irqchip_state method for XIVE to fix shutdown race
Date: Tue, 13 Aug 2019 03:52:11 +0000 [thread overview]
Message-ID: <E547965E-CC31-470F-8849-0F2A899A121F@linux.vnet.ibm.com> (raw)
In-Reply-To: <20190812050743.aczgcqwmtqpkbx2l@oak.ozlabs.ibm.com>
> On Aug 12, 2019, at 12:07 AM, Paul Mackerras <paulus@ozlabs.org> wrote:
>
> ---
> arch/powerpc/include/asm/xive.h | 8 ++++
> arch/powerpc/kvm/book3s_xive.c | 31 ++++++++++++++
> arch/powerpc/sysdev/xive/common.c | 87 ++++++++++++++++++++++++++++-----------
> 3 files changed, 103 insertions(+), 23 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index e4016985764e..efb0e597b272 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -46,7 +46,15 @@ struct xive_irq_data {
>
> /* Setup/used by frontend */
> int target;
> + /*
> + * saved_p means that there is a queue entry for this interrupt
> + * in some CPU's queue (not including guest vcpu queues), even
> + * if P is not set in the source ESB.
> + * stale_p means that there is no queue entry for this interrupt
> + * in some CPU's queue, even if P is set in the source ESB.
> + */
> bool saved_p;
> + bool stale_p;
> };
> #define XIVE_IRQ_FLAG_STORE_EOI 0x01
> #define XIVE_IRQ_FLAG_LSI 0x02
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index 09f838aa3138..74eea009c095 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -160,6 +160,9 @@ static irqreturn_t xive_esc_irq(int irq, void *data)
> */
> vcpu->arch.xive_esc_on = false;
>
> + /* This orders xive_esc_on = false vs. subsequent stale_p = true */
> + smp_wmb(); /* goes with smp_mb() in cleanup_single_escalation */
> +
> return IRQ_HANDLED;
> }
>
> @@ -1113,6 +1116,31 @@ void kvmppc_xive_disable_vcpu_interrupts(struct kvm_vcpu *vcpu)
> vcpu->arch.xive_esc_raddr = 0;
> }
>
> +/*
> + * In single escalation mode, the escalation interrupt is marked so
> + * that EOI doesn't re-enable it, but just sets the stale_p flag to
> + * indicate that the P bit has already been dealt with. However, the
> + * assembly code that enters the guest sets PQ to 00 without clearing
> + * stale_p (because it has no easy way to address it). Hence we have
> + * to adjust stale_p before shutting down the interrupt.
> + */
> +static void cleanup_single_escalation(struct kvm_vcpu *vcpu,
> + struct kvmppc_xive_vcpu *xc, int irq)
> +{
> + struct irq_data *d = irq_get_irq_data(irq);
> + struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
> +
> + /*
> + * This slightly odd sequence gives the right result
> + * (i.e. stale_p set if xive_esc_on is false) even if
> + * we race with xive_esc_irq() and xive_irq_eoi().
> + */
Hi Paul,
I don’t quite understand the logic here.
Are you saying the code sequence is
vcpu->arch.xive_esc_on = false; (xive_esc_irq)
then
xd->stale_p = true; (cleanup_single_escaltion)
> + xd->stale_p = false;
> + smp_mb(); /* paired with smb_wmb in xive_esc_irq */
> + if (!vcpu->arch.xive_esc_on)
> + xd->stale_p = true;
> +}
> +
> void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
> {
> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> @@ -1137,6 +1165,9 @@ void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
> /* Free escalations */
> for (i = 0; i < KVMPPC_XIVE_Q_COUNT; i++) {
> if (xc->esc_virq[i]) {
> + if (xc->xive->single_escalation)
> + cleanup_single_escalation(vcpu, xc,
> + xc->esc_virq[i]);
> free_irq(xc->esc_virq[i], vcpu);
> irq_dispose_mapping(xc->esc_virq[i]);
> kfree(xc->esc_virq_names[i]);
> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
> index 1cdb39575eae..be86fce1a84e 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -135,7 +135,7 @@ static u32 xive_read_eq(struct xive_q *q, bool just_peek)
> static u32 xive_scan_interrupts(struct xive_cpu *xc, bool just_peek)
> {
> u32 irq = 0;
> - u8 prio;
> + u8 prio = 0;
>
> /* Find highest pending priority */
> while (xc->pending_prio != 0) {
> @@ -148,8 +148,19 @@ static u32 xive_scan_interrupts(struct xive_cpu *xc, bool just_peek)
> irq = xive_read_eq(&xc->queue[prio], just_peek);
>
> /* Found something ? That's it */
> - if (irq)
> - break;
> + if (irq) {
> + if (just_peek || irq_to_desc(irq))
> + break;
> + /*
> + * We should never get here; if we do then we must
> + * have failed to synchronize the interrupt properly
> + * when shutting it down.
> + */
> + pr_crit("xive: got interrupt %d without descriptor, dropping\n",
> + irq);
> + WARN_ON(1);
> + continue;
> + }
>
> /* Clear pending bits */
> xc->pending_prio &= ~(1 << prio);
> @@ -307,6 +318,7 @@ static void xive_do_queue_eoi(struct xive_cpu *xc)
> */
> static void xive_do_source_eoi(u32 hw_irq, struct xive_irq_data *xd)
> {
> + xd->stale_p = false;
> /* If the XIVE supports the new "store EOI facility, use it */
> if (xd->flags & XIVE_IRQ_FLAG_STORE_EOI)
> xive_esb_write(xd, XIVE_ESB_STORE_EOI, 0);
> @@ -350,7 +362,7 @@ static void xive_do_source_eoi(u32 hw_irq, struct xive_irq_data *xd)
> }
> }
>
> -/* irq_chip eoi callback */
> +/* 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);
> @@ -366,6 +378,8 @@ static void xive_irq_eoi(struct irq_data *d)
> if (!irqd_irq_disabled(d) && !irqd_is_forwarded_to_vcpu(d) &&
> !(xd->flags & XIVE_IRQ_NO_EOI))
> xive_do_source_eoi(irqd_to_hwirq(d), xd);
> + else
> + xd->stale_p = true;
>
> /*
> * Clear saved_p to indicate that it's no longer occupying
> @@ -397,11 +411,16 @@ static void xive_do_source_set_mask(struct xive_irq_data *xd,
> */
> if (mask) {
> val = xive_esb_read(xd, XIVE_ESB_SET_PQ_01);
> - xd->saved_p = !!(val & XIVE_ESB_VAL_P);
> - } else if (xd->saved_p)
> + if (!xd->stale_p && !!(val & XIVE_ESB_VAL_P))
> + xd->saved_p = true;
> + xd->stale_p = false;
> + } else if (xd->saved_p) {
> xive_esb_read(xd, XIVE_ESB_SET_PQ_10);
> - else
> + xd->saved_p = false;
Should we also explicitly set xd->stale_p = true; here?
> + } else {
> xive_esb_read(xd, XIVE_ESB_SET_PQ_00);
> + xd->stale_p = false;
Should we also explicitly set xd->saved_p = true; here?
Thanks,
Lijun
next prev parent reply other threads:[~2019-08-13 3:52 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-12 5:06 [PATCH 0/2] powerpc/xive: Fix race condition leading to host crashes and hangs Paul Mackerras
2019-08-12 5:06 ` Paul Mackerras
2019-08-12 5:07 ` [PATCH 1/2] KVM: PPC: Book3S HV: Fix race in re-enabling XIVE escalation interrupts Paul Mackerras
2019-08-12 5:07 ` Paul Mackerras
2019-08-13 4:59 ` Paul Mackerras
2019-08-13 4:59 ` Paul Mackerras
2019-08-12 5:07 ` [PATCH 2/2] powerpc/xive: Implement get_irqchip_state method for XIVE to fix shutdown race Paul Mackerras
2019-08-12 5:07 ` Paul Mackerras
2019-08-13 3:52 ` Lijun Pan [this message]
2019-08-13 3:52 ` Lijun Pan
2019-08-13 3:52 ` Lijun Pan
2019-08-13 4:56 ` Paul Mackerras
2019-08-13 4:56 ` Paul Mackerras
2019-08-13 4:56 ` Paul Mackerras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E547965E-CC31-470F-8849-0F2A899A121F@linux.vnet.ibm.com \
--to=ljp@linux.vnet.ibm.com \
--cc=david@gibson.dropbear.id.au \
--cc=kvm-ppc@vger.kernel.org \
--cc=kvm@vger.kernel.org \
--cc=linuxppc-dev@ozlabs.org \
--cc=paulus@ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.