[bug report] perf/x86: Add Intel Tiger Lake uncore support

[bug report] perf/x86: Add Intel Tiger Lake uncore support

[Dan Carpenter]

Hello Kan Liang,

The patch fdb64822443e: "perf/x86: Add Intel Tiger Lake uncore
support" from Feb 6, 2020, leads to the following Smatch static
checker warning:

	arch/x86/events/intel/uncore_snb.c:1486 __uncore_imc_init_box()
	warn: cast after binop

arch/x86/events/intel/uncore_snb.c
    1465 static void __uncore_imc_init_box(struct intel_uncore_box *box,
    1466                                   unsigned int base_offset)
    1467 {
    1468         struct pci_dev *pdev = tgl_uncore_get_mc_dev();
    1469         struct intel_uncore_pmu *pmu = box->pmu;
    1470         struct intel_uncore_type *type = pmu->type;
    1471         resource_size_t addr;
    1472         u32 mch_bar;
    1473 
    1474         if (!pdev) {
    1475                 pr_warn("perf uncore: Cannot find matched IMC device.\n");
    1476                 return;
    1477         }
    1478 
    1479         pci_read_config_dword(pdev, SNB_UNCORE_PCI_IMC_BAR_OFFSET, &mch_bar);
    1480         /* MCHBAR is disabled */
    1481         if (!(mch_bar & BIT(0))) {
    1482                 pr_warn("perf uncore: MCHBAR is disabled. Failed to map IMC free-running counters.\n");
    1483                 return;
    1484         }
    1485         mch_bar &= ~BIT(0);
--> 1486         addr = (resource_size_t)(mch_bar + TGL_UNCORE_MMIO_IMC_MEM_OFFSET * pmu->pmu_idx);
                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
All of these are 32 bit values.  If we really need a 64bit type, these
will overflow before the cast to resource_size_t.  This code is 2 years
old, so I suspect the cast is only required on line 1490 and that one is
correct.

    1487 
    1488 #ifdef CONFIG_PHYS_ADDR_T_64BIT
    1489         pci_read_config_dword(pdev, SNB_UNCORE_PCI_IMC_BAR_OFFSET + 4, &mch_bar);
    1490         addr |= ((resource_size_t)mch_bar << 32);
    1491 #endif
    1492 
    1493         addr += base_offset;
    1494         box->io_addr = ioremap(addr, type->mmio_map_size);
    1495         if (!box->io_addr)
    1496                 pr_warn("perf uncore: Failed to ioremap for %s.\n", type->name);
    1497 }

regards,
dan carpenter

Re: [bug report] perf/x86: Add Intel Tiger Lake uncore support

[Liang, Kan]

Hi Dan

On 1/25/2022 2:43 AM, Dan Carpenter wrote:
> Hello Kan Liang,
> 
> The patch fdb64822443e: "perf/x86: Add Intel Tiger Lake uncore
> support" from Feb 6, 2020, leads to the following Smatch static
> checker warning:
> 
> 	arch/x86/events/intel/uncore_snb.c:1486 __uncore_imc_init_box()
> 	warn: cast after binop
> 
> arch/x86/events/intel/uncore_snb.c
>      1465 static void __uncore_imc_init_box(struct intel_uncore_box *box,
>      1466                                   unsigned int base_offset)
>      1467 {
>      1468         struct pci_dev *pdev = tgl_uncore_get_mc_dev();
>      1469         struct intel_uncore_pmu *pmu = box->pmu;
>      1470         struct intel_uncore_type *type = pmu->type;
>      1471         resource_size_t addr;
>      1472         u32 mch_bar;
>      1473
>      1474         if (!pdev) {
>      1475                 pr_warn("perf uncore: Cannot find matched IMC device.\n");
>      1476                 return;
>      1477         }
>      1478
>      1479         pci_read_config_dword(pdev, SNB_UNCORE_PCI_IMC_BAR_OFFSET, &mch_bar);
>      1480         /* MCHBAR is disabled */
>      1481         if (!(mch_bar & BIT(0))) {
>      1482                 pr_warn("perf uncore: MCHBAR is disabled. Failed to map IMC free-running counters.\n");
>      1483                 return;
>      1484         }
>      1485         mch_bar &= ~BIT(0);
> --> 1486         addr = (resource_size_t)(mch_bar + TGL_UNCORE_MMIO_IMC_MEM_OFFSET * pmu->pmu_idx);
>                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> All of these are 32 bit values.  If we really need a 64bit type, these
> will overflow before the cast to resource_size_t.  This code is 2 years
> old, so I suspect the cast is only required on line 1490 and that one is
> correct.
> 

Thanks for the report.

For 32bit system, I'm not sure whether it's OK to remove the 
resource_size_t for 1486?
Shouldn't we explicitly cast to resource_size_t for the possible 
overflow part for a 32bit system as below?

diff --git a/arch/x86/events/intel/uncore_snb.c 
b/arch/x86/events/intel/uncore_snb.c
index 36b6c20..fc6cfcb 100644
--- a/arch/x86/events/intel/uncore_snb.c
+++ b/arch/x86/events/intel/uncore_snb.c
@@ -1720,7 +1720,7 @@ static void __uncore_imc_init_box(struct 
intel_uncore_box *box,
                 return;
         }
         mch_bar &= ~BIT(0);
-       addr = (resource_size_t)(mch_bar + 
TGL_UNCORE_MMIO_IMC_MEM_OFFSET * pmu->pmu_idx);
+       addr = mch_bar + 
(resource_size_t)(TGL_UNCORE_MMIO_IMC_MEM_OFFSET * pmu->pmu_idx);

  #ifdef CONFIG_PHYS_ADDR_T_64BIT
         pci_read_config_dword(pdev, SNB_UNCORE_PCI_IMC_BAR_OFFSET + 4, 
&mch_bar);

Thanks,
Kan

>      1487
>      1488 #ifdef CONFIG_PHYS_ADDR_T_64BIT
>      1489         pci_read_config_dword(pdev, SNB_UNCORE_PCI_IMC_BAR_OFFSET + 4, &mch_bar);
>      1490         addr |= ((resource_size_t)mch_bar << 32);
>      1491 #endif
>      1492
>      1493         addr += base_offset;
>      1494         box->io_addr = ioremap(addr, type->mmio_map_size);
>      1495         if (!box->io_addr)
>      1496                 pr_warn("perf uncore: Failed to ioremap for %s.\n", type->name);
>      1497 }
> 
> regards,
> dan carpenter