[bug report] PCI: rockchip: Fix window mapping and address translation for endpoint

[Dan Carpenter]

On Tue, Jun 27, 2023 at 11:39:16AM +0200, Rick Wertenbroek wrote:
> On Tue, Jun 27, 2023 at 9:19 AM Dan Carpenter <dan.carpenter at linaro.org> wrote:
> >
> > Hello Rick Wertenbroek,
> >
> > The patch dc73ed0f1b8b: "PCI: rockchip: Fix window mapping and
> > address translation for endpoint" from Apr 18, 2023, leads to the
> > following Smatch static checker warning:
> >
> >         drivers/pci/controller/pcie-rockchip-ep.c:405 rockchip_pcie_ep_send_msi_irq()
> >         warn: was expecting a 64 bit value instead of '~4294967040'
> >
> > drivers/pci/controller/pcie-rockchip-ep.c
> >     351 static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
> >     352                                          u8 interrupt_num)
> >     353 {
> >     354         struct rockchip_pcie *rockchip = &ep->rockchip;
> >     355         u32 flags, mme, data, data_mask;
> >     356         u8 msi_count;
> >     357         u64 pci_addr;
> >                 ^^^^^^^^^^^^^
> >
> >     358         u32 r;
> >     359
> >     360         /* Check MSI enable bit */
> >     361         flags = rockchip_pcie_read(&ep->rockchip,
> >     362                                    ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
> >     363                                    ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
> >     364         if (!(flags & ROCKCHIP_PCIE_EP_MSI_CTRL_ME))
> >     365                 return -EINVAL;
> >     366
> >     367         /* Get MSI numbers from MME */
> >     368         mme = ((flags & ROCKCHIP_PCIE_EP_MSI_CTRL_MME_MASK) >>
> >     369                         ROCKCHIP_PCIE_EP_MSI_CTRL_MME_OFFSET);
> >     370         msi_count = 1 << mme;
> >     371         if (!interrupt_num || interrupt_num > msi_count)
> >     372                 return -EINVAL;
> >     373
> >     374         /* Set MSI private data */
> >     375         data_mask = msi_count - 1;
> >     376         data = rockchip_pcie_read(rockchip,
> >     377                                   ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
> >     378                                   ROCKCHIP_PCIE_EP_MSI_CTRL_REG +
> >     379                                   PCI_MSI_DATA_64);
> >     380         data = (data & ~data_mask) | ((interrupt_num - 1) & data_mask);
> >     381
> >     382         /* Get MSI PCI address */
> >     383         pci_addr = rockchip_pcie_read(rockchip,
> >     384                                       ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
> >     385                                       ROCKCHIP_PCIE_EP_MSI_CTRL_REG +
> >     386                                       PCI_MSI_ADDRESS_HI);
> >     387         pci_addr <<= 32;
> >
> > The high 32 bits are definitely set.
> >
> >     388         pci_addr |= rockchip_pcie_read(rockchip,
> >     389                                        ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
> >     390                                        ROCKCHIP_PCIE_EP_MSI_CTRL_REG +
> >     391                                        PCI_MSI_ADDRESS_LO);
> >     392
> >     393         /* Set the outbound region if needed. */
> >     394         if (unlikely(ep->irq_pci_addr != (pci_addr & PCIE_ADDR_MASK) ||
> >     395                      ep->irq_pci_fn != fn)) {
> >     396                 r = rockchip_ob_region(ep->irq_phys_addr);
> >     397                 rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
> >     398                                              ep->irq_phys_addr,
> >     399                                              pci_addr & PCIE_ADDR_MASK,
> >     400                                              ~PCIE_ADDR_MASK + 1);
> >     401                 ep->irq_pci_addr = (pci_addr & PCIE_ADDR_MASK);
> >     402                 ep->irq_pci_fn = fn;
> >     403         }
> >     404
> > --> 405         writew(data, ep->irq_cpu_addr + (pci_addr & ~PCIE_ADDR_MASK));
> >
> > PCIE_ADDR_MASK is 0xffffff00 (which is unsigned int).  What Smatch is
> > saying here is that pci_addr is a u64 it looks like the intention was to
> > zero out the last byte, but instead we are zeroing the high 32 bits as
> > well as the last 8 bits.
> 
> Hello,
> You are right there is a problem.
> 
> The warning at line 405, however, seems to be a false positive. Because the
> mask is 0xffffff00 (which is unsigned int) and the ~ is applied before promotion
> this results in 0xff, which is then promoted to 0x00000000000000ff when applied
> to pci_addr so this is fine.
> 

Hm...  Yeah.  This warning message is quite old and I haven't thought
about it properly in some time.  Probably I should silence the warning
if BIT(31) is zero and only the lowest bits are 1.

I'll take a look at all these warnings again.

> What you describe about zeroing the upper 32 bits and not only the lower 8
> refers to line 399 where we apply the PCI_ADDR_MASK 0xffffff00 with the
> & operator to pci_addr, this is the real issue, as you said this
> zeroes the upper
> 32-bits, which is not the intended behavior. Thank you for pointing this out.

Heh.  No, I don't get credit for spotting line 399.  That's all you.

I don't know how to identify the issue on line 399 through static
analysis.  It think it's impossible to tell unintentional masking from
intentional masking on that line.

regards,
dan carpenter