[XEN PATCH v3 0/2] x86: address MISRA C:2012 Rule 5.3 and 8.3

[XEN PATCH v3 0/2] x86: address MISRA C:2012 Rule 5.3 and 8.3

[Nicola Vetrini]

This series is the part of series [1] that needed further adjustments. The first
two patches of that series are already committed.
The changes in this version are detailed in the individual commit messages.

[1] https://lists.xenproject.org/archives/html/xen-devel/2023-08/msg00492.html

Nicola Vetrini (2):
  x86/vmsi: rename variables to address MISRA C:2012 Rule 5.3
  x86/setup: address MISRA C:2012 Rule 5.3 and 8.3

 xen/arch/x86/hvm/vmsi.c          | 46 ++++++++++++++++----------------
 xen/arch/x86/include/asm/setup.h |  2 +-
 xen/arch/x86/setup.c             |  3 +--
 3 files changed, 25 insertions(+), 26 deletions(-)

-- 
2.34.1

[XEN PATCH v3 1/2] x86/vmsi: rename variables to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

The local variables 'irq_desc' shadow the homonymous global variable,
declared in 'xen/arch/x86/include/asm/irq.h', therefore they are renamed
'irqd' for consistency with ARM code. Other variables of the same type
in the file are also renamed 'irqd' for consistency.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- Renamed the local variables instead of the global.
- Edited subject from x86/irq to x86/vmsi

Renaming everything to 'desc' would have been the most obvious choice,
but given that there's also 'msi_desc' used in the same functions, 'irqd'
is less error-prone in my opinion.
---
 xen/arch/x86/hvm/vmsi.c | 46 ++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 3cd4923060..55d5e26a04 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -281,7 +281,7 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
     unsigned int nr_entry, index;
     int r = X86EMUL_UNHANDLEABLE;
     unsigned long flags;
-    struct irq_desc *desc;
+    struct irq_desc *irqd;
 
     if ( (len != 4 && len != 8) || (address & (len - 1)) )
         return r;
@@ -330,21 +330,21 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
     if ( !msi_desc || msi_desc->irq < 0 )
         goto out;
     
-    desc = irq_to_desc(msi_desc->irq);
-    if ( !desc )
+    irqd = irq_to_desc(msi_desc->irq);
+    if ( !irqd )
         goto out;
 
-    spin_lock_irqsave(&desc->lock, flags);
+    spin_lock_irqsave(&irqd->lock, flags);
 
-    if ( !desc->msi_desc )
+    if ( !irqd->msi_desc )
         goto unlock;
 
-    ASSERT(msi_desc == desc->msi_desc);
+    ASSERT(msi_desc == irqd->msi_desc);
    
-    guest_mask_msi_irq(desc, !!(val & PCI_MSIX_VECTOR_BITMASK));
+    guest_mask_msi_irq(irqd, !!(val & PCI_MSIX_VECTOR_BITMASK));
 
 unlock:
-    spin_unlock_irqrestore(&desc->lock, flags);
+    spin_unlock_irqrestore(&irqd->lock, flags);
     if ( len == 4 )
         r = X86EMUL_OKAY;
 
@@ -462,7 +462,7 @@ static void del_msixtbl_entry(struct msixtbl_entry *entry)
 
 int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
 {
-    struct irq_desc *irq_desc;
+    struct irq_desc *irqd;
     struct msi_desc *msi_desc;
     struct pci_dev *pdev;
     struct msixtbl_entry *entry, *new_entry;
@@ -482,14 +482,14 @@ int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
     if ( !new_entry )
         return -ENOMEM;
 
-    irq_desc = pirq_spin_lock_irq_desc(pirq, NULL);
-    if ( !irq_desc )
+    irqd = pirq_spin_lock_irq_desc(pirq, NULL);
+    if ( !irqd )
     {
         xfree(new_entry);
         return r;
     }
 
-    msi_desc = irq_desc->msi_desc;
+    msi_desc = irqd->msi_desc;
     if ( !msi_desc )
         goto out;
 
@@ -508,7 +508,7 @@ found:
     r = 0;
 
 out:
-    spin_unlock_irq(&irq_desc->lock);
+    spin_unlock_irq(&irqd->lock);
     xfree(new_entry);
 
     if ( !r )
@@ -533,7 +533,7 @@ out:
 
 void msixtbl_pt_unregister(struct domain *d, struct pirq *pirq)
 {
-    struct irq_desc *irq_desc;
+    struct irq_desc *irqd;
     struct msi_desc *msi_desc;
     struct pci_dev *pdev;
     struct msixtbl_entry *entry;
@@ -544,11 +544,11 @@ void msixtbl_pt_unregister(struct domain *d, struct pirq *pirq)
     if ( !msixtbl_initialised(d) )
         return;
 
-    irq_desc = pirq_spin_lock_irq_desc(pirq, NULL);
-    if ( !irq_desc )
+    irqd = pirq_spin_lock_irq_desc(pirq, NULL);
+    if ( !irqd )
         return;
 
-    msi_desc = irq_desc->msi_desc;
+    msi_desc = irqd->msi_desc;
     if ( !msi_desc )
         goto out;
 
@@ -559,14 +559,14 @@ void msixtbl_pt_unregister(struct domain *d, struct pirq *pirq)
             goto found;
 
 out:
-    spin_unlock_irq(&irq_desc->lock);
+    spin_unlock_irq(&irqd->lock);
     return;
 
 found:
     if ( !atomic_dec_and_test(&entry->refcnt) )
         del_msixtbl_entry(entry);
 
-    spin_unlock_irq(&irq_desc->lock);
+    spin_unlock_irq(&irqd->lock);
 }
 
 void msixtbl_init(struct domain *d)
@@ -664,12 +664,12 @@ static unsigned int msi_gflags(uint16_t data, uint64_t addr, bool masked)
 static void vpci_mask_pirq(struct domain *d, int pirq, bool mask)
 {
     unsigned long flags;
-    struct irq_desc *desc = domain_spin_lock_irq_desc(d, pirq, &flags);
+    struct irq_desc *irqd = domain_spin_lock_irq_desc(d, pirq, &flags);
 
-    if ( !desc )
+    if ( !irqd )
         return;
-    guest_mask_msi_irq(desc, mask);
-    spin_unlock_irqrestore(&desc->lock, flags);
+    guest_mask_msi_irq(irqd, mask);
+    spin_unlock_irqrestore(&irqd->lock, flags);
 }
 
 void vpci_msi_arch_mask(struct vpci_msi *msi, const struct pci_dev *pdev,
-- 
2.34.1

[XEN PATCH v3 2/2] x86/setup: address MISRA C:2012 Rule 5.3 and 8.3

[Nicola Vetrini]

The parameters in the function declaration 'construct_dom0' violate
Rule 8.3:
"All declarations of an object or function shall use the same names
and type qualifiers", but also cause shadowing inside the declaration
scope with the variable "static struct file __initdata kernel;" in
'xen/common/efi/boot.c'. Renaming the parameters in the declaration
resolves both issues

The local variable 'mask' is removed because it shadows the homonymous
variable defined in an outer scope. There's no change to the semantics since
the last use of this variable is in the scope touched by this commit.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in v2:
- Clarified commit message

As mentioned in the preivious iteration's thread:
the declaration itself is a scope and shadowing can happen, as in:

int x;
void f(int x, int arr[x]);

Now, the example is a bit contrived, but the fact that the rule does not list any
exception motivates the declaration of 'construct_dom0' being considered a
violation.
---
 xen/arch/x86/include/asm/setup.h | 2 +-
 xen/arch/x86/setup.c             | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 51fce66607..b0e6a39e23 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -33,7 +33,7 @@ static inline void vesa_init(void) {};
 
 int construct_dom0(
     struct domain *d,
-    const module_t *kernel, unsigned long kernel_headroom,
+    const module_t *image, unsigned long image_headroom,
     module_t *initrd,
     const char *cmdline);
 void setup_io_bitmap(struct domain *d);
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2dbe9857aa..80ae973d64 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1577,8 +1577,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         s = map_s;
         if ( s < map_e )
         {
-            uint64_t mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
-
+            mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
             map_s = (s + mask) & ~mask;
             map_e &= ~mask;
             init_boot_pages(map_s, map_e);
-- 
2.34.1

Re: [XEN PATCH v3 1/2] x86/vmsi: rename variables to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 08.08.2023 14:22, Nicola Vetrini wrote:
> The local variables 'irq_desc' shadow the homonymous global variable,
> declared in 'xen/arch/x86/include/asm/irq.h', therefore they are renamed
> 'irqd' for consistency with ARM code. Other variables of the same type
> in the file are also renamed 'irqd' for consistency.

I'm pretty sure I pointed out that Arm uses a mix of "desc" and "irqd".
So "consistency with ARM code" doesn't ...

> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -281,7 +281,7 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
>      unsigned int nr_entry, index;
>      int r = X86EMUL_UNHANDLEABLE;
>      unsigned long flags;
> -    struct irq_desc *desc;
> +    struct irq_desc *irqd;

... require e.g. this rename. As mentioned before: Let's limit code
churn where possible, and where going beyond what's strictly necessary
isn't otherwise useful; there's already enough of it with all these not
really helpful Misra changes.

> @@ -462,7 +462,7 @@ static void del_msixtbl_entry(struct msixtbl_entry *entry)
>  
>  int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
>  {
> -    struct irq_desc *irq_desc;
> +    struct irq_desc *irqd;

This one indeed wants renaming, but then - consistent within the file -
to "desc". At least that's my view.

Jan

Re: [XEN PATCH v3 2/2] x86/setup: address MISRA C:2012 Rule 5.3 and 8.3

[Jan Beulich]

On 08.08.2023 14:22, Nicola Vetrini wrote:
> The parameters in the function declaration 'construct_dom0' violate
> Rule 8.3:
> "All declarations of an object or function shall use the same names
> and type qualifiers", but also cause shadowing inside the declaration
> scope with the variable "static struct file __initdata kernel;" in
> 'xen/common/efi/boot.c'. Renaming the parameters in the declaration
> resolves both issues
> 
> The local variable 'mask' is removed because it shadows the homonymous
> variable defined in an outer scope. There's no change to the semantics since
> the last use of this variable is in the scope touched by this commit.
> 
> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [XEN PATCH v3 1/2] x86/vmsi: rename variables to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 08/08/2023 15:22, Jan Beulich wrote:
> On 08.08.2023 14:22, Nicola Vetrini wrote:
>> The local variables 'irq_desc' shadow the homonymous global variable,
>> declared in 'xen/arch/x86/include/asm/irq.h', therefore they are 
>> renamed
>> 'irqd' for consistency with ARM code. Other variables of the same type
>> in the file are also renamed 'irqd' for consistency.
> 
> I'm pretty sure I pointed out that Arm uses a mix of "desc" and "irqd".
> So "consistency with ARM code" doesn't ...
> 
>> --- a/xen/arch/x86/hvm/vmsi.c
>> +++ b/xen/arch/x86/hvm/vmsi.c
>> @@ -281,7 +281,7 @@ static int msixtbl_write(struct vcpu *v, unsigned 
>> long address,
>>      unsigned int nr_entry, index;
>>      int r = X86EMUL_UNHANDLEABLE;
>>      unsigned long flags;
>> -    struct irq_desc *desc;
>> +    struct irq_desc *irqd;
> 
> ... require e.g. this rename. As mentioned before: Let's limit code
> churn where possible, and where going beyond what's strictly necessary
> isn't otherwise useful; there's already enough of it with all these not
> really helpful Misra changes.
> 
>> @@ -462,7 +462,7 @@ static void del_msixtbl_entry(struct msixtbl_entry 
>> *entry)
>> 
>>  int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t 
>> gtable)
>>  {
>> -    struct irq_desc *irq_desc;
>> +    struct irq_desc *irqd;
> 
> This one indeed wants renaming, but then - consistent within the file -
> to "desc". At least that's my view.
> 
> Jan

Well, but having

struct irq_desc *desc;
struct msi_desc *msi_desc;

and then using them both within the function doesn't seem that readable, 
but if you
prefer "desc" I have no objection (just two local variables that need to 
be changed).

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH v3 1/2] x86/vmsi: rename variables to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 08.08.2023 15:38, Nicola Vetrini wrote:
> On 08/08/2023 15:22, Jan Beulich wrote:
>> On 08.08.2023 14:22, Nicola Vetrini wrote:
>>> The local variables 'irq_desc' shadow the homonymous global variable,
>>> declared in 'xen/arch/x86/include/asm/irq.h', therefore they are 
>>> renamed
>>> 'irqd' for consistency with ARM code. Other variables of the same type
>>> in the file are also renamed 'irqd' for consistency.
>>
>> I'm pretty sure I pointed out that Arm uses a mix of "desc" and "irqd".
>> So "consistency with ARM code" doesn't ...
>>
>>> --- a/xen/arch/x86/hvm/vmsi.c
>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>> @@ -281,7 +281,7 @@ static int msixtbl_write(struct vcpu *v, unsigned 
>>> long address,
>>>      unsigned int nr_entry, index;
>>>      int r = X86EMUL_UNHANDLEABLE;
>>>      unsigned long flags;
>>> -    struct irq_desc *desc;
>>> +    struct irq_desc *irqd;
>>
>> ... require e.g. this rename. As mentioned before: Let's limit code
>> churn where possible, and where going beyond what's strictly necessary
>> isn't otherwise useful; there's already enough of it with all these not
>> really helpful Misra changes.
>>
>>> @@ -462,7 +462,7 @@ static void del_msixtbl_entry(struct msixtbl_entry 
>>> *entry)
>>>
>>>  int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t 
>>> gtable)
>>>  {
>>> -    struct irq_desc *irq_desc;
>>> +    struct irq_desc *irqd;
>>
>> This one indeed wants renaming, but then - consistent within the file -
>> to "desc". At least that's my view.
> 
> Well, but having
> 
> struct irq_desc *desc;
> struct msi_desc *msi_desc;
> 
> and then using them both within the function doesn't seem that readable, 

You have a point there, yes. Still I'd then probably follow up with a
change to rename msi_desc -> msi (and I say this despite seeing that
farther down in the file "msi" is also used for another pointer type
variables/parameters). But with what you say in mind I'd also be okay
with you renaming to irqd where renaming is needed, but leaving "desc"
alone.

Jan

Re: [XEN PATCH v3 1/2] x86/vmsi: rename variables to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

>>> 
>>>> @@ -462,7 +462,7 @@ static void del_msixtbl_entry(struct 
>>>> msixtbl_entry
>>>> *entry)
>>>> 
>>>>  int msixtbl_pt_register(struct domain *d, struct pirq *pirq, 
>>>> uint64_t
>>>> gtable)
>>>>  {
>>>> -    struct irq_desc *irq_desc;
>>>> +    struct irq_desc *irqd;
>>> 
>>> This one indeed wants renaming, but then - consistent within the file 
>>> -
>>> to "desc". At least that's my view.
>> 
>> Well, but having
>> 
>> struct irq_desc *desc;
>> struct msi_desc *msi_desc;
>> 
>> and then using them both within the function doesn't seem that 
>> readable,
> 
> You have a point there, yes. Still I'd then probably follow up with a
> change to rename msi_desc -> msi (and I say this despite seeing that
> farther down in the file "msi" is also used for another pointer type
> variables/parameters). But with what you say in mind I'd also be okay
> with you renaming to irqd where renaming is needed, but leaving "desc"
> alone.
> 
> Jan

I'll go for the latter (it's quicker) as a separate patch, since 
hopefully the other patch
in the series can go in unmodified.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)