On Fri, Aug 08, 2025 at 11:57:32AM -0500, Miles Glenn wrote:
> On Fri, 2025-08-08 at 15:39 +0530, Gautam Menghani wrote:
> > Coverity reported an integer overflow warning in xive2_redistribute()
> > where the code does a left shift operation "0xffffffff << crowd". Fix the
> > warning by using a 64 byte integer type. Also refactor the calculation
> > into dedicated routines.
> >
> > Resolves: Coverity CID 1612608
> > Fixes: 555e446019f5 ("ppc/xive2: Support redistribution of group
> > interrupts")
> > Signed-off-by: Gautam Menghani <[email protected]>
> > ---
> > hw/intc/xive2.c | 45 +++++++++++++++++++++++++++++++--------------
> > 1 file changed, 31 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/intc/xive2.c b/hw/intc/xive2.c
> > index ee5fa26178..90fe9c883b 100644
> > --- a/hw/intc/xive2.c
> > +++ b/hw/intc/xive2.c
> > @@ -95,6 +95,35 @@ static void xive2_nvgc_set_backlog(Xive2Nvgc *nvgc,
> > uint8_t priority,
> > }
> > }
> >
> > +static inline uint32_t xive2_nvgc_get_idx(uint32_t nvp_idx, uint8_t group)
> > +{
> > + uint32_t nvgc_idx;
> > +
> > + if (group > 0) {
> > + nvgc_idx = (nvp_idx & (0xffffffffULL << group)) |
> > + ((1 << (group - 1)) - 1);
> > + } else {
> > + nvgc_idx = nvp_idx;
> > + }
> > +
> > + return nvgc_idx;
> > +}
> > +
> > +static inline uint8_t xive2_nvgc_get_blk(uint8_t nvp_blk, uint8_t crowd)
> > +{
> > + uint8_t nvgc_blk;
> > +
> > + if (crowd > 0) {
> > + crowd = (crowd == 3) ? 4 : crowd;
> > + nvgc_blk = (nvp_blk & (0xffffffffULL << crowd)) |
> > + ((1 << (crowd - 1)) - 1);
> > + } else {
> > + nvgc_blk = nvp_blk;
> > + }
> > +
> > + return nvgc_blk;
> > +}
> > +
> > uint64_t xive2_presenter_nvgc_backlog_op(XivePresenter *xptr,
> > bool crowd,
> > uint8_t blk, uint32_t idx,
> > @@ -638,20 +667,8 @@ static void xive2_redistribute(Xive2Router *xrtr,
> > XiveTCTX *tctx, uint8_t ring)
> >
> > trace_xive_redistribute(tctx->cs->cpu_index, ring, nvp_blk, nvp_idx);
> > /* convert crowd/group to blk/idx */
> > - if (group > 0) {
> > - nvgc_idx = (nvp_idx & (0xffffffff << group)) |
> > - ((1 << (group - 1)) - 1);
> > - } else {
> > - nvgc_idx = nvp_idx;
> > - }
> > -
> > - if (crowd > 0) {
> > - crowd = (crowd == 3) ? 4 : crowd;
> > - nvgc_blk = (nvp_blk & (0xffffffff << crowd)) |
> > - ((1 << (crowd - 1)) - 1);
> > - } else {
> > - nvgc_blk = nvp_blk;
> > - }
> > + nvgc_idx = xive2_nvgc_get_idx(nvp_idx, group);
> > + nvgc_blk = xive2_nvgc_get_blk(nvp_blk, crowd);
> >
> > /* Use blk/idx to retrieve the NVGC */
> > if (xive2_router_get_nvgc(xrtr, crowd, nvgc_blk, nvgc_idx, &nvgc)) {
>
> Thanks for fixing that, Gautam! I was wondering, do we really need the
> inline keywords here? Maybe it's better to let the compiler decide if
> they should be inlined (which it probably will since they are only
> called by a single function in the same file)?
>
Sure, thanks for the feedback and RB. I'll send a v2.
> Either way...
>
> Reviewed-by: Glenn Miles <[email protected]>
>
> Thanks,
>
> Glenn
>
Thanks,
Gautam
