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

["Liang, Kan" <kan.liang@linux.intel.com>]

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