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

[Peter Maydell]

On Mon, 3 Feb 2020 at 06:11, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> These changes introduces models for the PCIe Host Bridge (PHB4) of the
> POWER9 processor. It includes the PowerBus logic interface (PBCQ),
> IOMMU support, a single PCIe Gen.4 Root Complex, and support for MSI
> and LSI interrupt sources as found on a POWER9 system using the XIVE
> interrupt controller.

Hi; Coverity has just spotted an error in this old change
(CID 1487176):

> +++ 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...

> +        break;
> +    default:
> +        phb_pec_error(pec, "%s @0x%"HWADDR_PRIx"=%"PRIx64"\n", __func__,
> +                      addr, val);
> +    }
> +}

> +++ b/include/hw/pci-host/pnv_phb4.h

> +struct PnvPhb4PecState {
> +    DeviceState parent;
> +
> +    /* PEC number in chip */
> +    uint32_t index;
> +    uint32_t chip_id;
> +
> +    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?

thanks
-- PMM