1. Patchew
  2. Xen
  3. View series

[PATCH] x86: Address MISRA Rule 13.6

Andrew Cooper posted 1 patch 4 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240402154339.2448435-1-andrew.cooper3@citrix.com
xen/arch/x86/irq.c                       | 3 ++-
xen/arch/x86/time.c                      | 6 ++++--
xen/drivers/passthrough/amd/iommu_intr.c | 3 ++-
3 files changed, 8 insertions(+), 4 deletions(-)
[PATCH] x86: Address MISRA Rule 13.6
Posted by Andrew Cooper 4 weeks ago 
MISRA Rule 13.6 doesn't like having an expression in a sizeof() which
potentially has side effects.

Address several violations by pulling the expression out into a local
variable.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
CC: consulting@bugseng.com <consulting@bugseng.com>

Slightly RFC.
---
 xen/arch/x86/irq.c                       | 3 ++-
 xen/arch/x86/time.c                      | 6 ++++--
 xen/drivers/passthrough/amd/iommu_intr.c | 3 ++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 0487f734a5d2..d73f687f7617 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
     {
         struct domain *d = action->guest[i];
         unsigned int pirq = domain_irq_to_pirq(d, irq);
+        struct pirq *pirq_info = pirq_info(d, pirq);
 
-        if ( test_and_clear_bool(pirq_info(d, pirq)->masked) )
+        if ( test_and_clear_bool(pirq_info->masked) )
             action->in_flight--;
     }
 
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 60870047894b..6f136f4b14bf 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2503,7 +2503,9 @@ static long cmos_utc_offset; /* in seconds */
 
 int time_suspend(void)
 {
-    if ( smp_processor_id() == 0 )
+    unsigned int cpu = smp_processor_id();
+
+    if ( cpu == 0 )
     {
         cmos_utc_offset = -get_wallclock_time();
         cmos_utc_offset += get_sec();
@@ -2514,7 +2516,7 @@ int time_suspend(void)
     }
 
     /* Better to cancel calibration timer for accuracy. */
-    clear_bit(TIME_CALIBRATE_SOFTIRQ, &softirq_pending(smp_processor_id()));
+    clear_bit(TIME_CALIBRATE_SOFTIRQ, &softirq_pending(cpu));
 
     return 0;
 }
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index d9eefcd8e411..7fc796dec25b 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -183,6 +183,7 @@ static void free_intremap_entry(const struct amd_iommu *iommu,
                                 unsigned int bdf, unsigned int index)
 {
     union irte_ptr entry = get_intremap_entry(iommu, bdf, index);
+    struct ivrs_mappings *ivrs = get_ivrs_mappings(iommu->seg);
 
     if ( iommu->ctrl.ga_en )
     {
@@ -201,7 +202,7 @@ static void free_intremap_entry(const struct amd_iommu *iommu,
     else
         ACCESS_ONCE(entry.ptr32->raw) = 0;
 
-    __clear_bit(index, get_ivrs_mappings(iommu->seg)[bdf].intremap_inuse);
+    __clear_bit(index, ivrs[bdf].intremap_inuse);
 }
 
 static void update_intremap_entry(const struct amd_iommu *iommu,

base-commit: 7a09966e7b2823b70f6d56d0cf66c11124f4a3c1
-- 
2.30.2
Re: [PATCH] x86: Address MISRA Rule 13.6
Posted by Jan Beulich 4 weeks ago 
On 02.04.2024 17:43, Andrew Cooper wrote:
> MISRA Rule 13.6 doesn't like having an expression in a sizeof() which
> potentially has side effects.
> 
> Address several violations by pulling the expression out into a local
> variable.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one caveat:

> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
>      {
>          struct domain *d = action->guest[i];
>          unsigned int pirq = domain_irq_to_pirq(d, irq);
> +        struct pirq *pirq_info = pirq_info(d, pirq);

Misra won't like the var's name matching the macro's. Can we go with just
"info"?

Jan
Re: [PATCH] x86: Address MISRA Rule 13.6
Posted by Andrew Cooper 4 weeks ago 
On 02/04/2024 4:46 pm, Jan Beulich wrote:
> On 02.04.2024 17:43, Andrew Cooper wrote:
>> MISRA Rule 13.6 doesn't like having an expression in a sizeof() which
>> potentially has side effects.
>>
>> Address several violations by pulling the expression out into a local
>> variable.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one caveat:
>
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
>>      {
>>          struct domain *d = action->guest[i];
>>          unsigned int pirq = domain_irq_to_pirq(d, irq);
>> +        struct pirq *pirq_info = pirq_info(d, pirq);
> Misra won't like the var's name matching the macro's. Can we go with just
> "info"?

Ah - missed that.

I can name it to just info, but I considered "struct pirq *info" to be a
little odd.

~Andrew
Re: [PATCH] x86: Address MISRA Rule 13.6
Posted by Federico Serafini 4 weeks ago 
On 02/04/24 17:54, Andrew Cooper wrote:
> On 02/04/2024 4:46 pm, Jan Beulich wrote:
>> On 02.04.2024 17:43, Andrew Cooper wrote:
>>> MISRA Rule 13.6 doesn't like having an expression in a sizeof() which
>>> potentially has side effects.
>>>
>>> Address several violations by pulling the expression out into a local
>>> variable.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> with one caveat:
>>
>>> --- a/xen/arch/x86/irq.c
>>> +++ b/xen/arch/x86/irq.c
>>> @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
>>>       {
>>>           struct domain *d = action->guest[i];
>>>           unsigned int pirq = domain_irq_to_pirq(d, irq);
>>> +        struct pirq *pirq_info = pirq_info(d, pirq);
>> Misra won't like the var's name matching the macro's. Can we go with just
>> "info"?
> 
> Ah - missed that.
> 
> I can name it to just info, but I considered "struct pirq *info" to be a
> little odd.

The local variable is a non-callable entity;
"clashes" between function-like macros and non-callable entities
will be deviated (as agreed during MISRA meetings).

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)
Re: [PATCH] x86: Address MISRA Rule 13.6
Posted by Jan Beulich 4 weeks ago 
On 02.04.2024 17:54, Andrew Cooper wrote:
> On 02/04/2024 4:46 pm, Jan Beulich wrote:
>> On 02.04.2024 17:43, Andrew Cooper wrote:
>>> MISRA Rule 13.6 doesn't like having an expression in a sizeof() which
>>> potentially has side effects.
>>>
>>> Address several violations by pulling the expression out into a local
>>> variable.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> with one caveat:
>>
>>> --- a/xen/arch/x86/irq.c
>>> +++ b/xen/arch/x86/irq.c
>>> @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
>>>      {
>>>          struct domain *d = action->guest[i];
>>>          unsigned int pirq = domain_irq_to_pirq(d, irq);
>>> +        struct pirq *pirq_info = pirq_info(d, pirq);
>> Misra won't like the var's name matching the macro's. Can we go with just
>> "info"?
> 
> Ah - missed that.
> 
> I can name it to just info, but I considered "struct pirq *info" to be a
> little odd.

I agree, but what do you do with another "pirq" already there.

Or wait, what about

        struct pirq *pirq = pirq_info(d, domain_irq_to_pirq(d, irq));

?

Jan
Re: [PATCH] x86: Address MISRA Rule 13.6
Posted by Andrew Cooper 4 weeks ago 
On 02/04/2024 5:06 pm, Jan Beulich wrote:
> On 02.04.2024 17:54, Andrew Cooper wrote:
>> On 02/04/2024 4:46 pm, Jan Beulich wrote:
>>> On 02.04.2024 17:43, Andrew Cooper wrote:
>>>> MISRA Rule 13.6 doesn't like having an expression in a sizeof() which
>>>> potentially has side effects.
>>>>
>>>> Address several violations by pulling the expression out into a local
>>>> variable.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> with one caveat:
>>>
>>>> --- a/xen/arch/x86/irq.c
>>>> +++ b/xen/arch/x86/irq.c
>>>> @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
>>>>      {
>>>>          struct domain *d = action->guest[i];
>>>>          unsigned int pirq = domain_irq_to_pirq(d, irq);
>>>> +        struct pirq *pirq_info = pirq_info(d, pirq);
>>> Misra won't like the var's name matching the macro's. Can we go with just
>>> "info"?
>> Ah - missed that.
>>
>> I can name it to just info, but I considered "struct pirq *info" to be a
>> little odd.
> I agree, but what do you do with another "pirq" already there.
>
> Or wait, what about
>
>         struct pirq *pirq = pirq_info(d, domain_irq_to_pirq(d, irq));
>
> ?

That should work.  I'll switch to this locally, and wait for the
feedback on whether the patch works for 13.6.

~Andrew
Re: [PATCH] x86: Address MISRA Rule 13.6
Posted by Federico Serafini 3 weeks, 6 days ago 
On 02/04/24 18:09, Andrew Cooper wrote:
> On 02/04/2024 5:06 pm, Jan Beulich wrote:
>> On 02.04.2024 17:54, Andrew Cooper wrote:
>>> On 02/04/2024 4:46 pm, Jan Beulich wrote:
>>>> On 02.04.2024 17:43, Andrew Cooper wrote:
>>>>> MISRA Rule 13.6 doesn't like having an expression in a sizeof() which
>>>>> potentially has side effects.
>>>>>
>>>>> Address several violations by pulling the expression out into a local
>>>>> variable.
>>>>>
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>> with one caveat:
>>>>
>>>>> --- a/xen/arch/x86/irq.c
>>>>> +++ b/xen/arch/x86/irq.c
>>>>> @@ -1150,8 +1150,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
>>>>>       {
>>>>>           struct domain *d = action->guest[i];
>>>>>           unsigned int pirq = domain_irq_to_pirq(d, irq);
>>>>> +        struct pirq *pirq_info = pirq_info(d, pirq);
>>>> Misra won't like the var's name matching the macro's. Can we go with just
>>>> "info"?
>>> Ah - missed that.
>>>
>>> I can name it to just info, but I considered "struct pirq *info" to be a
>>> little odd.
>> I agree, but what do you do with another "pirq" already there.
>>
>> Or wait, what about
>>
>>          struct pirq *pirq = pirq_info(d, domain_irq_to_pirq(d, irq));
>>
>> ?
> 
> That should work.  I'll switch to this locally, and wait for the
> feedback on whether the patch works for 13.6.

I confirm that both versions of the patch address some violations of
13.6.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.