| Message ID | 20240801203008.11224-12-kowal@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | XIVE2 changes for TIMA operations | expand |
On 8/1/24 22:30, Michael Kowal wrote: > From: Glenn Miles <milesg@linux.vnet.ibm.com> > > Current code was updating the PIPR inside the xive_tctx_accept() function > instead of the xive_tctx_set_cppr function, which is where the HW would > have it updated. Did you confirm with the HW designer ? AFAIR, the PIPR is constructed from the IPB and the later is it updated the better. However, if now, both PIPR (HW and Pool) are required to identify the ctx to notify, I agree set_cppr() needs a change but what about xive_tctx_ipb_update() which is called when an interrupt needs a resend ? Thanks, C. > Moved the update to the xive_tctx_set_cppr function which required > additional support for pool interrupts. > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > Signed-off-by: Michael Kowal <kowal@linux.ibm.com> > --- > hw/intc/xive.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index 5c4ca7f6e0..d951aac3a0 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -89,7 +89,6 @@ static uint64_t xive_tctx_accept(XiveTCTX *tctx, uint8_t ring) > > /* Reset the pending buffer bit */ > aregs[TM_IPB] &= ~xive_priority_to_ipb(cppr); > - regs[TM_PIPR] = ipb_to_pipr(aregs[TM_IPB]); > > /* Drop Exception bit */ > regs[TM_NSR] &= ~mask; > @@ -143,6 +142,8 @@ void xive_tctx_reset_signal(XiveTCTX *tctx, uint8_t ring) > static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr) > { > uint8_t *regs = &tctx->regs[ring]; > + uint8_t pipr_min; > + uint8_t ring_min; > > trace_xive_tctx_set_cppr(tctx->cs->cpu_index, ring, > regs[TM_IPB], regs[TM_PIPR], > @@ -154,8 +155,37 @@ static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr) > > tctx->regs[ring + TM_CPPR] = cppr; > > + /* > + * Recompute the PIPR based on local pending interrupts. The PHYS > + * ring must take the minimum of both the PHYS and POOL PIPR values. > + */ > + pipr_min = ipb_to_pipr(regs[TM_IPB]); > + ring_min = ring; > + > + /* PHYS updates also depend on POOL values */ > + if (ring == TM_QW3_HV_PHYS) { > + uint8_t *pregs = &tctx->regs[TM_QW2_HV_POOL]; > + > + /* POOL values only matter if POOL ctx is valid */ > + if (pregs[TM_WORD2] & 0x80) { > + > + uint8_t pool_pipr = ipb_to_pipr(pregs[TM_IPB]); > + > + /* > + * Determine highest priority interrupt and > + * remember which ring has it. > + */ > + if (pool_pipr < pipr_min) { > + pipr_min = pool_pipr; > + ring_min = TM_QW2_HV_POOL; > + } > + } > + } > + > + regs[TM_PIPR] = pipr_min; > + > /* CPPR has changed, check if we need to raise a pending exception */ > - xive_tctx_notify(tctx, ring); > + xive_tctx_notify(tctx, ring_min); > } > > void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb)
On 8/1/24 22:30, Michael Kowal wrote: > From: Glenn Miles <milesg@linux.vnet.ibm.com> > > Current code was updating the PIPR inside the xive_tctx_accept() function > instead of the xive_tctx_set_cppr function, which is where the HW would > have it updated. > > Moved the update to the xive_tctx_set_cppr function which required > additional support for pool interrupts. > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > Signed-off-by: Michael Kowal <kowal@linux.ibm.com> > --- > hw/intc/xive.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index 5c4ca7f6e0..d951aac3a0 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -89,7 +89,6 @@ static uint64_t xive_tctx_accept(XiveTCTX *tctx, uint8_t ring) > > /* Reset the pending buffer bit */ > aregs[TM_IPB] &= ~xive_priority_to_ipb(cppr); > - regs[TM_PIPR] = ipb_to_pipr(aregs[TM_IPB]); > > /* Drop Exception bit */ > regs[TM_NSR] &= ~mask; > @@ -143,6 +142,8 @@ void xive_tctx_reset_signal(XiveTCTX *tctx, uint8_t ring) > static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr) > { > uint8_t *regs = &tctx->regs[ring]; > + uint8_t pipr_min; > + uint8_t ring_min; > > trace_xive_tctx_set_cppr(tctx->cs->cpu_index, ring, > regs[TM_IPB], regs[TM_PIPR], > @@ -154,8 +155,37 @@ static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr) > > tctx->regs[ring + TM_CPPR] = cppr; > > + /* > + * Recompute the PIPR based on local pending interrupts. The PHYS > + * ring must take the minimum of both the PHYS and POOL PIPR values. > + */ > + pipr_min = ipb_to_pipr(regs[TM_IPB]); > + ring_min = ring; > + > + /* PHYS updates also depend on POOL values */ > + if (ring == TM_QW3_HV_PHYS) { > + uint8_t *pregs = &tctx->regs[TM_QW2_HV_POOL]; 'pool_regs' would be clearer > + > + /* POOL values only matter if POOL ctx is valid */ > + if (pregs[TM_WORD2] & 0x80) { > + > + uint8_t pool_pipr = ipb_to_pipr(pregs[TM_IPB]); > + > + /* > + * Determine highest priority interrupt and > + * remember which ring has it. > + */ > + if (pool_pipr < pipr_min) { > + pipr_min = pool_pipr; > + ring_min = TM_QW2_HV_POOL; > + } > + } > + } > + > + regs[TM_PIPR] = pipr_min; > + > /* CPPR has changed, check if we need to raise a pending exception */ > - xive_tctx_notify(tctx, ring); > + xive_tctx_notify(tctx, ring_min); > } > > void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb)
On 8/29/2024 7:29 AM, Cédric Le Goater wrote: > On 8/1/24 22:30, Michael Kowal wrote: >> From: Glenn Miles <milesg@linux.vnet.ibm.com> >> >> Current code was updating the PIPR inside the xive_tctx_accept() >> function >> instead of the xive_tctx_set_cppr function, which is where the HW would >> have it updated. > > Did you confirm with the HW designer ? > > AFAIR, the PIPR is constructed from the IPB and the later is it updated > the better. However, if now, both PIPR (HW and Pool) are required to > identify the ctx to notify, I agree set_cppr() needs a change but what > about xive_tctx_ipb_update() which is called when an interrupt > needs a resend ? This was fix to a bug and matches what is specified in the XIVE2 architecture document CPPR flows (MMIO CPPR xxx processing). > > > Thanks, > > C. > > > >> Moved the update to the xive_tctx_set_cppr function which required >> additional support for pool interrupts. >> >> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> >> Signed-off-by: Michael Kowal <kowal@linux.ibm.com> >> --- >> hw/intc/xive.c | 34 ++++++++++++++++++++++++++++++++-- >> 1 file changed, 32 insertions(+), 2 deletions(-) >> >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c >> index 5c4ca7f6e0..d951aac3a0 100644 >> --- a/hw/intc/xive.c >> +++ b/hw/intc/xive.c >> @@ -89,7 +89,6 @@ static uint64_t xive_tctx_accept(XiveTCTX *tctx, >> uint8_t ring) >> /* Reset the pending buffer bit */ >> aregs[TM_IPB] &= ~xive_priority_to_ipb(cppr); >> - regs[TM_PIPR] = ipb_to_pipr(aregs[TM_IPB]); >> /* Drop Exception bit */ >> regs[TM_NSR] &= ~mask; >> @@ -143,6 +142,8 @@ void xive_tctx_reset_signal(XiveTCTX *tctx, >> uint8_t ring) >> static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, >> uint8_t cppr) >> { >> uint8_t *regs = &tctx->regs[ring]; >> + uint8_t pipr_min; >> + uint8_t ring_min; >> trace_xive_tctx_set_cppr(tctx->cs->cpu_index, ring, >> regs[TM_IPB], regs[TM_PIPR], >> @@ -154,8 +155,37 @@ static void xive_tctx_set_cppr(XiveTCTX *tctx, >> uint8_t ring, uint8_t cppr) >> tctx->regs[ring + TM_CPPR] = cppr; >> + /* >> + * Recompute the PIPR based on local pending interrupts. The PHYS >> + * ring must take the minimum of both the PHYS and POOL PIPR >> values. >> + */ >> + pipr_min = ipb_to_pipr(regs[TM_IPB]); >> + ring_min = ring; >> + >> + /* PHYS updates also depend on POOL values */ >> + if (ring == TM_QW3_HV_PHYS) { >> + uint8_t *pregs = &tctx->regs[TM_QW2_HV_POOL]; >> + >> + /* POOL values only matter if POOL ctx is valid */ >> + if (pregs[TM_WORD2] & 0x80) { >> + >> + uint8_t pool_pipr = ipb_to_pipr(pregs[TM_IPB]); >> + >> + /* >> + * Determine highest priority interrupt and >> + * remember which ring has it. >> + */ >> + if (pool_pipr < pipr_min) { >> + pipr_min = pool_pipr; >> + ring_min = TM_QW2_HV_POOL; >> + } >> + } >> + } >> + >> + regs[TM_PIPR] = pipr_min; >> + >> /* CPPR has changed, check if we need to raise a pending >> exception */ >> - xive_tctx_notify(tctx, ring); >> + xive_tctx_notify(tctx, ring_min); >> } >> void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb) >
On 8/29/24 22:35, Mike Kowal wrote: > > On 8/29/2024 7:29 AM, Cédric Le Goater wrote: >> On 8/1/24 22:30, Michael Kowal wrote: >>> From: Glenn Miles <milesg@linux.vnet.ibm.com> >>> >>> Current code was updating the PIPR inside the xive_tctx_accept() function >>> instead of the xive_tctx_set_cppr function, which is where the HW would >>> have it updated. >> >> Did you confirm with the HW designer ? >> >> AFAIR, the PIPR is constructed from the IPB and the later is it updated >> the better. However, if now, both PIPR (HW and Pool) are required to >> identify the ctx to notify, I agree set_cppr() needs a change but what >> about xive_tctx_ipb_update() which is called when an interrupt >> needs a resend ? > > > This was fix to a bug and matches what is specified in the XIVE2 architecture document CPPR flows (MMIO CPPR xxx processing). ok. I was also wondering if this was fixing a bug. Do you think this is the correct commit id ? cdd4de68edb6 ("ppc/xive: notify the CPU when the interrupt priority is more privileged") If so, could you please add a Fixes tags ? Thanks, C. > > >> >> >> Thanks, >> >> C. >> >> >> >>> Moved the update to the xive_tctx_set_cppr function which required >>> additional support for pool interrupts. >>> >>> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> >>> Signed-off-by: Michael Kowal <kowal@linux.ibm.com> >>> --- >>> hw/intc/xive.c | 34 ++++++++++++++++++++++++++++++++-- >>> 1 file changed, 32 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c >>> index 5c4ca7f6e0..d951aac3a0 100644 >>> --- a/hw/intc/xive.c >>> +++ b/hw/intc/xive.c >>> @@ -89,7 +89,6 @@ static uint64_t xive_tctx_accept(XiveTCTX *tctx, uint8_t ring) >>> /* Reset the pending buffer bit */ >>> aregs[TM_IPB] &= ~xive_priority_to_ipb(cppr); >>> - regs[TM_PIPR] = ipb_to_pipr(aregs[TM_IPB]); >>> /* Drop Exception bit */ >>> regs[TM_NSR] &= ~mask; >>> @@ -143,6 +142,8 @@ void xive_tctx_reset_signal(XiveTCTX *tctx, uint8_t ring) >>> static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr) >>> { >>> uint8_t *regs = &tctx->regs[ring]; >>> + uint8_t pipr_min; >>> + uint8_t ring_min; >>> trace_xive_tctx_set_cppr(tctx->cs->cpu_index, ring, >>> regs[TM_IPB], regs[TM_PIPR], >>> @@ -154,8 +155,37 @@ static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr) >>> tctx->regs[ring + TM_CPPR] = cppr; >>> + /* >>> + * Recompute the PIPR based on local pending interrupts. The PHYS >>> + * ring must take the minimum of both the PHYS and POOL PIPR values. >>> + */ >>> + pipr_min = ipb_to_pipr(regs[TM_IPB]); >>> + ring_min = ring; >>> + >>> + /* PHYS updates also depend on POOL values */ >>> + if (ring == TM_QW3_HV_PHYS) { >>> + uint8_t *pregs = &tctx->regs[TM_QW2_HV_POOL]; >>> + >>> + /* POOL values only matter if POOL ctx is valid */ >>> + if (pregs[TM_WORD2] & 0x80) { >>> + >>> + uint8_t pool_pipr = ipb_to_pipr(pregs[TM_IPB]); >>> + >>> + /* >>> + * Determine highest priority interrupt and >>> + * remember which ring has it. >>> + */ >>> + if (pool_pipr < pipr_min) { >>> + pipr_min = pool_pipr; >>> + ring_min = TM_QW2_HV_POOL; >>> + } >>> + } >>> + } >>> + >>> + regs[TM_PIPR] = pipr_min; >>> + >>> /* CPPR has changed, check if we need to raise a pending exception */ >>> - xive_tctx_notify(tctx, ring); >>> + xive_tctx_notify(tctx, ring_min); >>> } >>> void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb) >>
On 8/30/2024 3:25 AM, Cédric Le Goater wrote: > On 8/29/24 22:35, Mike Kowal wrote: >> >> On 8/29/2024 7:29 AM, Cédric Le Goater wrote: >>> On 8/1/24 22:30, Michael Kowal wrote: >>>> From: Glenn Miles <milesg@linux.vnet.ibm.com> >>>> >>>> Current code was updating the PIPR inside the xive_tctx_accept() >>>> function >>>> instead of the xive_tctx_set_cppr function, which is where the HW >>>> would >>>> have it updated. >>> >>> Did you confirm with the HW designer ? >>> >>> AFAIR, the PIPR is constructed from the IPB and the later is it updated >>> the better. However, if now, both PIPR (HW and Pool) are required to >>> identify the ctx to notify, I agree set_cppr() needs a change but what >>> about xive_tctx_ipb_update() which is called when an interrupt >>> needs a resend ? >> >> >> This was fix to a bug and matches what is specified in the XIVE2 >> architecture document CPPR flows (MMIO CPPR xxx processing). > > ok. I was also wondering if this was fixing a bug. Do you think this > is the correct commit id ? > > cdd4de68edb6 ("ppc/xive: notify the CPU when the interrupt priority > is more privileged") > > If so, could you please add a Fixes tags ? > > Thanks, > > C. > Many of these parts have been changed multiple time for different things. I am not sure which commit this fixes. I am upstreaming other peoples work that was done over the last couple of years so it hard to tell. Also, the original xive support was only complete enough to support Linux. Much of this I would consider 'new development' expanding XIVE support for Power VM. If you think it should still have a fixes-tag, I will add it. MAK > > >> >> >>> >>> >>> Thanks, >>> >>> C. >>> >>> >>> >>>> Moved the update to the xive_tctx_set_cppr function which required >>>> additional support for pool interrupts. >>>> >>>> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> >>>> Signed-off-by: Michael Kowal <kowal@linux.ibm.com> >>>> --- >>>> hw/intc/xive.c | 34 ++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 32 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c >>>> index 5c4ca7f6e0..d951aac3a0 100644 >>>> --- a/hw/intc/xive.c >>>> +++ b/hw/intc/xive.c >>>> @@ -89,7 +89,6 @@ static uint64_t xive_tctx_accept(XiveTCTX *tctx, >>>> uint8_t ring) >>>> /* Reset the pending buffer bit */ >>>> aregs[TM_IPB] &= ~xive_priority_to_ipb(cppr); >>>> - regs[TM_PIPR] = ipb_to_pipr(aregs[TM_IPB]); >>>> /* Drop Exception bit */ >>>> regs[TM_NSR] &= ~mask; >>>> @@ -143,6 +142,8 @@ void xive_tctx_reset_signal(XiveTCTX *tctx, >>>> uint8_t ring) >>>> static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, >>>> uint8_t cppr) >>>> { >>>> uint8_t *regs = &tctx->regs[ring]; >>>> + uint8_t pipr_min; >>>> + uint8_t ring_min; >>>> trace_xive_tctx_set_cppr(tctx->cs->cpu_index, ring, >>>> regs[TM_IPB], regs[TM_PIPR], >>>> @@ -154,8 +155,37 @@ static void xive_tctx_set_cppr(XiveTCTX *tctx, >>>> uint8_t ring, uint8_t cppr) >>>> tctx->regs[ring + TM_CPPR] = cppr; >>>> + /* >>>> + * Recompute the PIPR based on local pending interrupts. The PHYS >>>> + * ring must take the minimum of both the PHYS and POOL PIPR >>>> values. >>>> + */ >>>> + pipr_min = ipb_to_pipr(regs[TM_IPB]); >>>> + ring_min = ring; >>>> + >>>> + /* PHYS updates also depend on POOL values */ >>>> + if (ring == TM_QW3_HV_PHYS) { >>>> + uint8_t *pregs = &tctx->regs[TM_QW2_HV_POOL]; >>>> + >>>> + /* POOL values only matter if POOL ctx is valid */ >>>> + if (pregs[TM_WORD2] & 0x80) { >>>> + >>>> + uint8_t pool_pipr = ipb_to_pipr(pregs[TM_IPB]); >>>> + >>>> + /* >>>> + * Determine highest priority interrupt and >>>> + * remember which ring has it. >>>> + */ >>>> + if (pool_pipr < pipr_min) { >>>> + pipr_min = pool_pipr; >>>> + ring_min = TM_QW2_HV_POOL; >>>> + } >>>> + } >>>> + } >>>> + >>>> + regs[TM_PIPR] = pipr_min; >>>> + >>>> /* CPPR has changed, check if we need to raise a pending >>>> exception */ >>>> - xive_tctx_notify(tctx, ring); >>>> + xive_tctx_notify(tctx, ring_min); >>>> } >>>> void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t >>>> ipb) >>> >
On 8/30/24 19:06, Mike Kowal wrote: > > On 8/30/2024 3:25 AM, Cédric Le Goater wrote: >> On 8/29/24 22:35, Mike Kowal wrote: >>> >>> On 8/29/2024 7:29 AM, Cédric Le Goater wrote: >>>> On 8/1/24 22:30, Michael Kowal wrote: >>>>> From: Glenn Miles <milesg@linux.vnet.ibm.com> >>>>> >>>>> Current code was updating the PIPR inside the xive_tctx_accept() function >>>>> instead of the xive_tctx_set_cppr function, which is where the HW would >>>>> have it updated. >>>> >>>> Did you confirm with the HW designer ? >>>> >>>> AFAIR, the PIPR is constructed from the IPB and the later is it updated >>>> the better. However, if now, both PIPR (HW and Pool) are required to >>>> identify the ctx to notify, I agree set_cppr() needs a change but what >>>> about xive_tctx_ipb_update() which is called when an interrupt >>>> needs a resend ? >>> >>> >>> This was fix to a bug and matches what is specified in the XIVE2 architecture document CPPR flows (MMIO CPPR xxx processing). >> >> ok. I was also wondering if this was fixing a bug. Do you think this >> is the correct commit id ? >> >> cdd4de68edb6 ("ppc/xive: notify the CPU when the interrupt priority is more privileged") >> >> If so, could you please add a Fixes tags ? >> >> Thanks, >> >> C. >> > > Many of these parts have been changed multiple time for different things. I am not sure which commit this fixes. I use 'tig blame <file>' to dig change history. > I am upstreaming other peoples work that was done over the last couple of years so it hard to tell. Also, the original xive support was only complete enough to support Linux. Much of this I would consider 'new development' expanding XIVE support for Power VM. If you think it should still have a fixes-tag, I will add it. I think this is the right commit, please add it. Could you also add your self as a Reviewer ? Thanks, C.
diff --git a/hw/intc/xive.c b/hw/intc/xive.c index 5c4ca7f6e0..d951aac3a0 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -89,7 +89,6 @@ static uint64_t xive_tctx_accept(XiveTCTX *tctx, uint8_t ring) /* Reset the pending buffer bit */ aregs[TM_IPB] &= ~xive_priority_to_ipb(cppr); - regs[TM_PIPR] = ipb_to_pipr(aregs[TM_IPB]); /* Drop Exception bit */ regs[TM_NSR] &= ~mask; @@ -143,6 +142,8 @@ void xive_tctx_reset_signal(XiveTCTX *tctx, uint8_t ring) static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr) { uint8_t *regs = &tctx->regs[ring]; + uint8_t pipr_min; + uint8_t ring_min; trace_xive_tctx_set_cppr(tctx->cs->cpu_index, ring, regs[TM_IPB], regs[TM_PIPR], @@ -154,8 +155,37 @@ static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr) tctx->regs[ring + TM_CPPR] = cppr; + /* + * Recompute the PIPR based on local pending interrupts. The PHYS + * ring must take the minimum of both the PHYS and POOL PIPR values. + */ + pipr_min = ipb_to_pipr(regs[TM_IPB]); + ring_min = ring; + + /* PHYS updates also depend on POOL values */ + if (ring == TM_QW3_HV_PHYS) { + uint8_t *pregs = &tctx->regs[TM_QW2_HV_POOL]; + + /* POOL values only matter if POOL ctx is valid */ + if (pregs[TM_WORD2] & 0x80) { + + uint8_t pool_pipr = ipb_to_pipr(pregs[TM_IPB]); + + /* + * Determine highest priority interrupt and + * remember which ring has it. + */ + if (pool_pipr < pipr_min) { + pipr_min = pool_pipr; + ring_min = TM_QW2_HV_POOL; + } + } + } + + regs[TM_PIPR] = pipr_min; + /* CPPR has changed, check if we need to raise a pending exception */ - xive_tctx_notify(tctx, ring); + xive_tctx_notify(tctx, ring_min); } void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb)