Re: [PULL 19/35] ppc/pnv: Add models for POWER9 PHB4 PCIe Host bridge

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2022-03-31 at 18:51 +0100, Peter Maydell wrote:
> 
> Hi; Coverity has just spotted an error in this old change
> (CID 1487176):

Oh my this is old ... I don't work for IBM anymore but I found the
relevant doc here: 
https://wiki.raptorcs.com/w/images/a/a5/POWER9_PCIe_controller_v11_27JUL2018_pub.pdf

So....

> > +++ b/hw/pci-host/pnv_phb4_pec.c
> > +static void pnv_pec_pci_xscom_write(void *opaque, hwaddr addr,
> > +                                    uint64_t val, unsigned size)
> > +{
> > +    PnvPhb4PecState *pec = PNV_PHB4_PEC(opaque);
> > +    uint32_t reg = addr >> 3;
> > +
> > +    switch (reg) {
> > +    case PEC_PCI_PBAIB_HW_CONFIG:
> > +    case PEC_PCI_PBAIB_READ_STK_OVR:
> > +        pec->pci_regs[reg] = val;
> 
> This write function switches on 'reg' and is written assuming
> that these PEC_PCI* constants are valid array indexes...

They should be but...

> > +        break;
> > +    default:
> > +        phb_pec_error(pec, "%s @0x%"HWADDR_PRIx"=%"PRIx64"\n",
> > __func__,
> > +                      addr, val);
> > +    }
> > +}
> > +++ b/include/hw/pci-host/pnv_phb4.h
> > +struct PnvPhb4PecStatimages/images/e {
> > +    DeviceState parent;
> > +
> > +    /* PEC number in chip */
> > +    uint32_t index;
> > +    uint32_t chip_id;images/
> > +
> > +    MemoryRegion *system_memory;
> > +
> > +    /* Nest registers, excuding per-stack */
> > +#define PHB4_PEC_NEST_REGS_COUNT    0xf
> > +    uint64_t nest_regs[PHB4_PEC_NEST_REGS_COUNT];
> > +    MemoryRegion nest_regs_mr;
> > +
> > +    /* PCI registers, excluding per-stack */
> > +#define PHB4_PEC_PCI_REGS_COUNT     0x2
> > +    uint64_t pci_regs[PHB4_PEC_PCI_REGS_COUNT];
> > +    MemoryRegion pci_regs_mr;
> 
> ...but we define the pci_regs[] array in PnvPhb4PecState to
> have only 2 elements...
> 
> > +++ b/include/hw/pci-host/pnv_phb4_regs.h
> > +/* XSCOM PCI global registers */
> > +#define PEC_PCI_PBAIB_HW_CONFIG         0x00
> > +#define PEC_PCI_PBAIB_READ_STK_OVR      0x02
> 
> ...and here we define PEC_PCI_PBAIB_READ_STK_OVR as 2, which makes
> it not a valid index into pci_regs[].
> 
> 
> Which of these is wrong?

This one:

#define PHB4_PEC_PCI_REGS_COUNT     0x2

Should be

#define PHB4_PEC_PCI_REGS_COUNT     0x3

There is no register at 0x1 though.

Cheers,
Ben.