Re: [PATCH v8 2/9] perf/amd/iommu: Clean up perf_iommu_enable_event

[Borislav Petkov]

On Mon, Jan 16, 2017 at 01:23:29AM -0600, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit 
> 
> * Clean up various bitwise operations in perf_iommu_enable_event
  ()

It is a function.

> * Make use macros BIT(x)

"Make use"?

> This should not affect logic and functionality.
> 
> Cc: Peter Zijlstra 
> Cc: Borislav Petkov 
> Signed-off-by: Suravee Suthikulpanit 
> ---
>  arch/x86/events/amd/iommu.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 44638d0..1aa25d8 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -164,11 +164,11 @@ static int get_next_avail_iommu_bnk_cntr(struct 
> perf_amd_iommu *perf_iommu)
>   for (bank = 0, shift = 0; bank < max_banks; bank++) {
>   for (cntr = 0; cntr < max_cntrs; cntr++) {
>   shift = bank + (bank*3) + cntr;
> - if (perf_iommu->cntr_assign_mask & (1ULL< + if (perf_iommu->cntr_assign_mask & BIT(shift)) {

   BIT_ULL()

otherwise you're introducing a bug.

>   continue;
>   } else {
> - perf_iommu->cntr_assign_mask |= (1ULL< - retval = ((u16)((u16)bank<<8) | (u8)(cntr));
> + perf_iommu->cntr_assign_mask |= BIT(shift);

Ditto.

> + retval = ((u16)((u16)bank << 8) | (u8)(cntr));

That's some ugly casting. Why?

(u8)(cntr) is supposed to do what exactly? Cut off to 255 if max_cntrs
grows bigger? Can that even happen?

Same for bank?

Then you're casting to u16 even though retval is int. Ah, because it can
be negative too.

How about this instead?

retval = ((bank & 0xff) << 8) | (cntr & 0xff);

This way it is really obvious what you're trying to do.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v8 2/9] perf/amd/iommu: Clean up perf_iommu_enable_event

[Suravee Suthikulpanit]

From: Suravee Suthikulpanit 

* Clean up various bitwise operations in perf_iommu_enable_event
* Make use macros BIT(x)

This should not affect logic and functionality.

Cc: Peter Zijlstra 
Cc: Borislav Petkov 
Signed-off-by: Suravee Suthikulpanit 
---
 arch/x86/events/amd/iommu.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
index 44638d0..1aa25d8 100644
--- a/arch/x86/events/amd/iommu.c
+++ b/arch/x86/events/amd/iommu.c
@@ -164,11 +164,11 @@ static int get_next_avail_iommu_bnk_cntr(struct 
perf_amd_iommu *perf_iommu)
for (bank = 0, shift = 0; bank < max_banks; bank++) {
for (cntr = 0; cntr < max_cntrs; cntr++) {
shift = bank + (bank*3) + cntr;
-   if (perf_iommu->cntr_assign_mask & (1ULL<cntr_assign_mask & BIT(shift)) {
continue;
} else {
-   perf_iommu->cntr_assign_mask |= (1ULL<cntr_assign_mask |= BIT(shift);
+   retval = ((u16)((u16)bank << 8) | (u8)(cntr));
goto out;
}
}
@@ -265,23 +265,23 @@ static void perf_iommu_enable_event(struct perf_event *ev)
_GET_BANK(ev), _GET_CNTR(ev) ,
 IOMMU_PC_COUNTER_SRC_REG, , true);
 
-   reg = 0ULL | devid | (_GET_DEVID_MASK(ev) << 32);
+   reg = devid | (_GET_DEVID_MASK(ev) << 32);
if (reg)
-   reg |= (1UL << 31);
+   reg |= BIT(31);
amd_iommu_pc_get_set_reg_val(devid,
_GET_BANK(ev), _GET_CNTR(ev) ,
 IOMMU_PC_DEVID_MATCH_REG, , true);
 
-   reg = 0ULL | _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
+   reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32);
if (reg)
-   reg |= (1UL << 31);
+   reg |= BIT(31);
amd_iommu_pc_get_set_reg_val(devid,
_GET_BANK(ev), _GET_CNTR(ev) ,
 IOMMU_PC_PASID_MATCH_REG, , true);
 
-   reg = 0ULL | _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
+   reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32);
if (reg)
-   reg |= (1UL << 31);
+   reg |= BIT(31);
amd_iommu_pc_get_set_reg_val(devid,
_GET_BANK(ev), _GET_CNTR(ev) ,
 IOMMU_PC_DOMID_MATCH_REG, , true);
-- 
1.8.3.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu