[XEN PATCH 0/4] x86: address some violations of MISRA C:2012 Rule 5.3

[XEN PATCH 0/4] x86: address some violations of MISRA C:2012 Rule 5.3

[Nicola Vetrini]

Some variables are renamed or deleted in this series to avoid shadowing, which
violates MISRA C:2012 Rule 5.3, whose headline is:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope".

Nicola Vetrini (4):
  x86/mce: address MISRA C:2012 Rule 5.3
  x86/mtrr: address MISRA C:2012 Rule 5.3
  x86/irq: rename variable to address MISRA C:2012 Rule 5.3
  x86/setup: address MISRA C:2012 Rule 5.3

 xen/arch/x86/cpu/mcheck/barrier.c |  8 ++++----
 xen/arch/x86/cpu/mcheck/barrier.h |  8 ++++----
 xen/arch/x86/hvm/hvm.c            |  2 +-
 xen/arch/x86/hvm/mtrr.c           | 32 +++++++++++++++----------------
 xen/arch/x86/include/asm/irq.h    |  2 +-
 xen/arch/x86/include/asm/setup.h  |  2 +-
 xen/arch/x86/io_apic.c            | 10 +++++-----
 xen/arch/x86/irq.c                | 12 ++++++------
 xen/arch/x86/msi.c                |  4 ++--
 xen/arch/x86/setup.c              |  3 +--
 xen/include/xen/irq.h             |  2 +-
 11 files changed, 42 insertions(+), 43 deletions(-)

--
2.34.1

[XEN PATCH 1/4] x86/mce: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

Suitable mechanical renames are made to avoid shadowing, thus
addressing violations of MISRA C:2012 Rule 5.3:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/cpu/mcheck/barrier.c | 8 ++++----
 xen/arch/x86/cpu/mcheck/barrier.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/barrier.c b/xen/arch/x86/cpu/mcheck/barrier.c
index a7e5b19a44..51a1d37a76 100644
--- a/xen/arch/x86/cpu/mcheck/barrier.c
+++ b/xen/arch/x86/cpu/mcheck/barrier.c
@@ -16,11 +16,11 @@ void mce_barrier_dec(struct mce_softirq_barrier *bar)
     atomic_dec(&bar->val);
 }
 
-void mce_barrier_enter(struct mce_softirq_barrier *bar, bool wait)
+void mce_barrier_enter(struct mce_softirq_barrier *bar, bool do_wait)
 {
     int gen;
 
-    if ( !wait )
+    if ( !do_wait )
         return;
     atomic_inc(&bar->ingen);
     gen = atomic_read(&bar->outgen);
@@ -34,11 +34,11 @@ void mce_barrier_enter(struct mce_softirq_barrier *bar, bool wait)
     }
 }
 
-void mce_barrier_exit(struct mce_softirq_barrier *bar, bool wait)
+void mce_barrier_exit(struct mce_softirq_barrier *bar, bool do_wait)
 {
     int gen;
 
-    if ( !wait )
+    if ( !do_wait )
         return;
     atomic_inc(&bar->outgen);
     gen = atomic_read(&bar->ingen);
diff --git a/xen/arch/x86/cpu/mcheck/barrier.h b/xen/arch/x86/cpu/mcheck/barrier.h
index c4d52b6192..5cd1b4e4bf 100644
--- a/xen/arch/x86/cpu/mcheck/barrier.h
+++ b/xen/arch/x86/cpu/mcheck/barrier.h
@@ -32,14 +32,14 @@ void mce_barrier_init(struct mce_softirq_barrier *);
 void mce_barrier_dec(struct mce_softirq_barrier *);
 
 /*
- * If @wait is false, mce_barrier_enter/exit() will return immediately
+ * If @do_wait is false, mce_barrier_enter/exit() will return immediately
  * without touching the barrier. It's used when handling a
  * non-broadcasting MCE (e.g. MCE on some old Intel CPU, MCE on AMD
  * CPU and LMCE on Intel Skylake-server CPU) which is received on only
  * one CPU and thus does not invoke mce_barrier_enter/exit() calls on
  * all CPUs.
  *
- * If @wait is true, mce_barrier_enter/exit() will handle the given
+ * If @do_wait is true, mce_barrier_enter/exit() will handle the given
  * barrier as below.
  *
  * Increment the generation number and the value. The generation number
@@ -53,8 +53,8 @@ void mce_barrier_dec(struct mce_softirq_barrier *);
  * These barrier functions should always be paired, so that the
  * counter value will reach 0 again after all CPUs have exited.
  */
-void mce_barrier_enter(struct mce_softirq_barrier *, bool wait);
-void mce_barrier_exit(struct mce_softirq_barrier *, bool wait);
+void mce_barrier_enter(struct mce_softirq_barrier *, bool do_wait);
+void mce_barrier_exit(struct mce_softirq_barrier *, bool do_wait);
 
 void mce_barrier(struct mce_softirq_barrier *);
 
-- 
2.34.1

[XEN PATCH 2/4] x86/mtrr: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

Rename variables to avoid shadowing and thus address
MISRA C:2012 Rule 5.3:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/hvm/mtrr.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 29f3fb1607..d504d1e43b 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -687,13 +687,13 @@ int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
 
 static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
 {
-    const struct mtrr_state *mtrr_state = &v->arch.hvm.mtrr;
+    const struct mtrr_state *mtrr = &v->arch.hvm.mtrr;
     struct hvm_hw_mtrr hw_mtrr = {
-        .msr_mtrr_def_type = mtrr_state->def_type |
-                             MASK_INSR(mtrr_state->fixed_enabled,
+        .msr_mtrr_def_type = mtrr->def_type |
+                             MASK_INSR(mtrr->fixed_enabled,
                                        MTRRdefType_FE) |
-                            MASK_INSR(mtrr_state->enabled, MTRRdefType_E),
-        .msr_mtrr_cap      = mtrr_state->mtrr_cap,
+                            MASK_INSR(mtrr->enabled, MTRRdefType_E),
+        .msr_mtrr_cap      = mtrr->mtrr_cap,
     };
     unsigned int i;
 
@@ -710,14 +710,14 @@ static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
 
     for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
     {
-        hw_mtrr.msr_mtrr_var[i * 2] = mtrr_state->var_ranges->base;
-        hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr_state->var_ranges->mask;
+        hw_mtrr.msr_mtrr_var[i * 2] = mtrr->var_ranges->base;
+        hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr->var_ranges->mask;
     }
 
     BUILD_BUG_ON(sizeof(hw_mtrr.msr_mtrr_fixed) !=
-                 sizeof(mtrr_state->fixed_ranges));
+                 sizeof(mtrr->fixed_ranges));
 
-    memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges,
+    memcpy(hw_mtrr.msr_mtrr_fixed, mtrr->fixed_ranges,
            sizeof(hw_mtrr.msr_mtrr_fixed));
 
     return hvm_save_entry(MTRR, v->vcpu_id, h, &hw_mtrr);
@@ -727,7 +727,7 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
 {
     unsigned int vcpuid, i;
     struct vcpu *v;
-    struct mtrr_state *mtrr_state;
+    struct mtrr_state *mtrr;
     struct hvm_hw_mtrr hw_mtrr;
 
     vcpuid = hvm_load_instance(h);
@@ -749,26 +749,26 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
         return -EINVAL;
     }
 
-    mtrr_state = &v->arch.hvm.mtrr;
+    mtrr = &v->arch.hvm.mtrr;
 
     hvm_set_guest_pat(v, hw_mtrr.msr_pat_cr);
 
-    mtrr_state->mtrr_cap = hw_mtrr.msr_mtrr_cap;
+    mtrr->mtrr_cap = hw_mtrr.msr_mtrr_cap;
 
     for ( i = 0; i < NUM_FIXED_MSR; i++ )
-        mtrr_fix_range_msr_set(d, mtrr_state, i, hw_mtrr.msr_mtrr_fixed[i]);
+        mtrr_fix_range_msr_set(d, mtrr, i, hw_mtrr.msr_mtrr_fixed[i]);
 
     for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
     {
-        mtrr_var_range_msr_set(d, mtrr_state,
+        mtrr_var_range_msr_set(d, mtrr,
                                MSR_IA32_MTRR_PHYSBASE(i),
                                hw_mtrr.msr_mtrr_var[i * 2]);
-        mtrr_var_range_msr_set(d, mtrr_state,
+        mtrr_var_range_msr_set(d, mtrr,
                                MSR_IA32_MTRR_PHYSMASK(i),
                                hw_mtrr.msr_mtrr_var[i * 2 + 1]);
     }
 
-    mtrr_def_type_msr_set(d, mtrr_state, hw_mtrr.msr_mtrr_def_type);
+    mtrr_def_type_msr_set(d, mtrr, hw_mtrr.msr_mtrr_def_type);
 
     return 0;
 }
-- 
2.34.1

[XEN PATCH 3/4] x86/irq: rename variable to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

The extern variable 'irq_desc' defined in 'irq.h' is shadowed by
local variables in the changed file. To avoid this, the variable is
renamed to 'irq_description'.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/hvm/hvm.c         |  2 +-
 xen/arch/x86/include/asm/irq.h |  2 +-
 xen/arch/x86/io_apic.c         | 10 +++++-----
 xen/arch/x86/irq.c             | 12 ++++++------
 xen/arch/x86/msi.c             |  4 ++--
 xen/include/xen/irq.h          |  2 +-
 6 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2180abeb33..ca5bb96388 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -474,7 +474,7 @@ void hvm_migrate_pirq(struct hvm_pirq_dpci *pirq_dpci, const struct vcpu *v)
 
         if ( !desc )
             return;
-        ASSERT(MSI_IRQ(desc - irq_desc));
+        ASSERT(MSI_IRQ(desc - irq_descriptor));
         irq_set_affinity(desc, cpumask_of(v->processor));
         spin_unlock_irq(&desc->lock);
     }
diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index ad907fc97f..f6df977170 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -172,7 +172,7 @@ int assign_irq_vector(int irq, const cpumask_t *mask);
 
 void cf_check irq_complete_move(struct irq_desc *desc);
 
-extern struct irq_desc *irq_desc;
+extern struct irq_desc *irq_descriptor;
 
 void lock_vector_lock(void);
 void unlock_vector_lock(void);
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index b3afef8933..b59d6cfb9e 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -990,9 +990,9 @@ static inline void ioapic_register_intr(int irq, unsigned long trigger)
 {
     if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
         trigger == IOAPIC_LEVEL)
-        irq_desc[irq].handler = &ioapic_level_type;
+        irq_descriptor[irq].handler = &ioapic_level_type;
     else
-        irq_desc[irq].handler = &ioapic_edge_type;
+        irq_descriptor[irq].handler = &ioapic_edge_type;
 }
 
 static void __init setup_IO_APIC_irqs(void)
@@ -1098,7 +1098,7 @@ static void __init setup_ExtINT_IRQ0_pin(unsigned int apic, unsigned int pin, in
      * The timer IRQ doesn't have to know that behind the
      * scene we have a 8259A-master in AEOI mode ...
      */
-    irq_desc[0].handler = &ioapic_edge_type;
+    irq_descriptor[0].handler = &ioapic_edge_type;
 
     /*
      * Add it to the IO-APIC irq-routing table:
@@ -1912,7 +1912,7 @@ static void __init check_timer(void)
     if ((ret = bind_irq_vector(0, vector, &cpumask_all)))
         printk(KERN_ERR"..IRQ0 is not set correctly with ioapic!!!, err:%d\n", ret);
     
-    irq_desc[0].status &= ~IRQ_DISABLED;
+    irq_descriptor[0].status &= ~IRQ_DISABLED;
 
     /*
      * Subtle, code in do_timer_interrupt() expects an AEOI
@@ -2009,7 +2009,7 @@ static void __init check_timer(void)
     printk(KERN_INFO "...trying to set up timer as Virtual Wire IRQ...");
 
     disable_8259A_irq(irq_to_desc(0));
-    irq_desc[0].handler = &lapic_irq_type;
+    irq_descriptor[0].handler = &lapic_irq_type;
     apic_write(APIC_LVT0, APIC_DM_FIXED | vector);	/* Fixed mode */
     enable_8259A_irq(irq_to_desc(0));
 
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 6abfd81621..ed95896bce 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -45,7 +45,7 @@ integer_param("irq-max-guests", irq_max_guests);
 
 vmask_t global_used_vector_map;
 
-struct irq_desc __read_mostly *irq_desc = NULL;
+struct irq_desc __read_mostly *irq_descriptor = NULL;
 
 static DECLARE_BITMAP(used_vectors, X86_NR_VECTORS);
 
@@ -424,9 +424,9 @@ int __init init_irq_data(void)
     for ( vector = 0; vector < X86_NR_VECTORS; ++vector )
         this_cpu(vector_irq)[vector] = INT_MIN;
 
-    irq_desc = xzalloc_array(struct irq_desc, nr_irqs);
-    
-    if ( !irq_desc )
+    irq_descriptor = xzalloc_array(struct irq_desc, nr_irqs);
+
+    if ( !irq_descriptor )
         return -ENOMEM;
 
     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
@@ -1133,7 +1133,7 @@ static void cf_check set_eoi_ready(void *data);
 static void cf_check irq_guest_eoi_timer_fn(void *data)
 {
     struct irq_desc *desc = data;
-    unsigned int i, irq = desc - irq_desc;
+    unsigned int i, irq = desc - irq_descriptor;
     irq_guest_action_t *action;
 
     spin_lock_irq(&desc->lock);
@@ -1382,7 +1382,7 @@ static void __set_eoi_ready(const struct irq_desc *desc)
     struct pending_eoi *peoi = this_cpu(pending_eoi);
     int                 irq, sp;
 
-    irq = desc - irq_desc;
+    irq = desc - irq_descriptor;
 
     if ( !action || action->in_flight ||
          !cpumask_test_and_clear_cpu(smp_processor_id(),
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index d0bf63df1d..35d417c63a 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -1322,7 +1322,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
         unsigned int i = 0, nr = 1;
 
         irq = entry->irq;
-        desc = &irq_desc[irq];
+        desc = &irq_descriptor[irq];
 
         spin_lock_irqsave(&desc->lock, flags);
 
@@ -1377,7 +1377,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
                 break;
 
             spin_unlock_irqrestore(&desc->lock, flags);
-            desc = &irq_desc[entry[++i].irq];
+            desc = &irq_descriptor[entry[++i].irq];
             spin_lock_irqsave(&desc->lock, flags);
             if ( desc->msi_desc != entry + i )
                 goto bogus;
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 9747e818f7..56a3aa6a29 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -107,7 +107,7 @@ typedef struct irq_desc {
 } __cacheline_aligned irq_desc_t;
 
 #ifndef irq_to_desc
-#define irq_to_desc(irq)    (&irq_desc[irq])
+#define irq_to_desc(irq)    (&irq_descriptor[irq])
 #endif
 
 int init_one_irq_desc(struct irq_desc *desc);
-- 
2.34.1

[XEN PATCH 4/4] x86/setup: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

The parameters renamed in the function declaration caused shadowing
with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
them also addresses Rule 8.3:
"All declarations of an object or function shall use the same names
and type qualifiers".

The local variable 'mask' is removed because it shadows the homonymous
variable defined in an outer scope, with no change to the semantics.
It was introduced by commit 5a771800114c437fb857b44b3ed74f60e87979c2
as a refactoring of the branch that handles 'CONFIG_X86_64' for function
'__start_xen'.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 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 1/4] x86/mce: address MISRA C:2012 Rule 5.3

[Stefano Stabellini]

On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> Suitable mechanical renames are made to avoid shadowing, thus
> addressing violations of MISRA C:2012 Rule 5.3:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/x86/cpu/mcheck/barrier.c | 8 ++++----
>  xen/arch/x86/cpu/mcheck/barrier.h | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/barrier.c b/xen/arch/x86/cpu/mcheck/barrier.c
> index a7e5b19a44..51a1d37a76 100644
> --- a/xen/arch/x86/cpu/mcheck/barrier.c
> +++ b/xen/arch/x86/cpu/mcheck/barrier.c
> @@ -16,11 +16,11 @@ void mce_barrier_dec(struct mce_softirq_barrier *bar)
>      atomic_dec(&bar->val);
>  }
>  
> -void mce_barrier_enter(struct mce_softirq_barrier *bar, bool wait)
> +void mce_barrier_enter(struct mce_softirq_barrier *bar, bool do_wait)

"wait" clashes with xen/common/sched/core.c:wait, which is globally
exported, right?

I think it would be good to add this info to the commit message in this
kind of patches.


>  {
>      int gen;
>  
> -    if ( !wait )
> +    if ( !do_wait )
>          return;
>      atomic_inc(&bar->ingen);
>      gen = atomic_read(&bar->outgen);
> @@ -34,11 +34,11 @@ void mce_barrier_enter(struct mce_softirq_barrier *bar, bool wait)
>      }
>  }
>  
> -void mce_barrier_exit(struct mce_softirq_barrier *bar, bool wait)
> +void mce_barrier_exit(struct mce_softirq_barrier *bar, bool do_wait)
>  {
>      int gen;
>  
> -    if ( !wait )
> +    if ( !do_wait )
>          return;
>      atomic_inc(&bar->outgen);
>      gen = atomic_read(&bar->ingen);
> diff --git a/xen/arch/x86/cpu/mcheck/barrier.h b/xen/arch/x86/cpu/mcheck/barrier.h
> index c4d52b6192..5cd1b4e4bf 100644
> --- a/xen/arch/x86/cpu/mcheck/barrier.h
> +++ b/xen/arch/x86/cpu/mcheck/barrier.h
> @@ -32,14 +32,14 @@ void mce_barrier_init(struct mce_softirq_barrier *);
>  void mce_barrier_dec(struct mce_softirq_barrier *);
>  
>  /*
> - * If @wait is false, mce_barrier_enter/exit() will return immediately
> + * If @do_wait is false, mce_barrier_enter/exit() will return immediately
>   * without touching the barrier. It's used when handling a
>   * non-broadcasting MCE (e.g. MCE on some old Intel CPU, MCE on AMD
>   * CPU and LMCE on Intel Skylake-server CPU) which is received on only
>   * one CPU and thus does not invoke mce_barrier_enter/exit() calls on
>   * all CPUs.
>   *
> - * If @wait is true, mce_barrier_enter/exit() will handle the given
> + * If @do_wait is true, mce_barrier_enter/exit() will handle the given
>   * barrier as below.
>   *
>   * Increment the generation number and the value. The generation number
> @@ -53,8 +53,8 @@ void mce_barrier_dec(struct mce_softirq_barrier *);
>   * These barrier functions should always be paired, so that the
>   * counter value will reach 0 again after all CPUs have exited.
>   */
> -void mce_barrier_enter(struct mce_softirq_barrier *, bool wait);
> -void mce_barrier_exit(struct mce_softirq_barrier *, bool wait);
> +void mce_barrier_enter(struct mce_softirq_barrier *, bool do_wait);
> +void mce_barrier_exit(struct mce_softirq_barrier *, bool do_wait);

You might as well add "bar" as first parameter?


>  void mce_barrier(struct mce_softirq_barrier *);
>  
> -- 
> 2.34.1
> 

Re: [XEN PATCH 2/4] x86/mtrr: address MISRA C:2012 Rule 5.3

[Stefano Stabellini]

On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> Rename variables to avoid shadowing and thus address
> MISRA C:2012 Rule 5.3:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
> 
> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

This one clashes with xen/arch/x86/include/asm/mtrr.h:struct mtrr_state
right?


> ---
>  xen/arch/x86/hvm/mtrr.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> index 29f3fb1607..d504d1e43b 100644
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -687,13 +687,13 @@ int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
>  
>  static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
>  {
> -    const struct mtrr_state *mtrr_state = &v->arch.hvm.mtrr;
> +    const struct mtrr_state *mtrr = &v->arch.hvm.mtrr;

I can see in hvm_vcpu_cacheattr_init, mtrr_get_type and other places
that we use:

  const struct mtrr_state *m

maybe that's better? I'll let Jan comment.

Anyway, the patch looks correct to me.



>      struct hvm_hw_mtrr hw_mtrr = {
> -        .msr_mtrr_def_type = mtrr_state->def_type |
> -                             MASK_INSR(mtrr_state->fixed_enabled,
> +        .msr_mtrr_def_type = mtrr->def_type |
> +                             MASK_INSR(mtrr->fixed_enabled,
>                                         MTRRdefType_FE) |
> -                            MASK_INSR(mtrr_state->enabled, MTRRdefType_E),
> -        .msr_mtrr_cap      = mtrr_state->mtrr_cap,
> +                            MASK_INSR(mtrr->enabled, MTRRdefType_E),
> +        .msr_mtrr_cap      = mtrr->mtrr_cap,
>      };
>      unsigned int i;
>  
> @@ -710,14 +710,14 @@ static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
>  
>      for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
>      {
> -        hw_mtrr.msr_mtrr_var[i * 2] = mtrr_state->var_ranges->base;
> -        hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr_state->var_ranges->mask;
> +        hw_mtrr.msr_mtrr_var[i * 2] = mtrr->var_ranges->base;
> +        hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr->var_ranges->mask;
>      }
>  
>      BUILD_BUG_ON(sizeof(hw_mtrr.msr_mtrr_fixed) !=
> -                 sizeof(mtrr_state->fixed_ranges));
> +                 sizeof(mtrr->fixed_ranges));
>  
> -    memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges,
> +    memcpy(hw_mtrr.msr_mtrr_fixed, mtrr->fixed_ranges,
>             sizeof(hw_mtrr.msr_mtrr_fixed));
>  
>      return hvm_save_entry(MTRR, v->vcpu_id, h, &hw_mtrr);
> @@ -727,7 +727,7 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
>  {
>      unsigned int vcpuid, i;
>      struct vcpu *v;
> -    struct mtrr_state *mtrr_state;
> +    struct mtrr_state *mtrr;
>      struct hvm_hw_mtrr hw_mtrr;
>  
>      vcpuid = hvm_load_instance(h);
> @@ -749,26 +749,26 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
>          return -EINVAL;
>      }
>  
> -    mtrr_state = &v->arch.hvm.mtrr;
> +    mtrr = &v->arch.hvm.mtrr;
>  
>      hvm_set_guest_pat(v, hw_mtrr.msr_pat_cr);
>  
> -    mtrr_state->mtrr_cap = hw_mtrr.msr_mtrr_cap;
> +    mtrr->mtrr_cap = hw_mtrr.msr_mtrr_cap;
>  
>      for ( i = 0; i < NUM_FIXED_MSR; i++ )
> -        mtrr_fix_range_msr_set(d, mtrr_state, i, hw_mtrr.msr_mtrr_fixed[i]);
> +        mtrr_fix_range_msr_set(d, mtrr, i, hw_mtrr.msr_mtrr_fixed[i]);
>  
>      for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
>      {
> -        mtrr_var_range_msr_set(d, mtrr_state,
> +        mtrr_var_range_msr_set(d, mtrr,
>                                 MSR_IA32_MTRR_PHYSBASE(i),
>                                 hw_mtrr.msr_mtrr_var[i * 2]);
> -        mtrr_var_range_msr_set(d, mtrr_state,
> +        mtrr_var_range_msr_set(d, mtrr,
>                                 MSR_IA32_MTRR_PHYSMASK(i),
>                                 hw_mtrr.msr_mtrr_var[i * 2 + 1]);
>      }
>  
> -    mtrr_def_type_msr_set(d, mtrr_state, hw_mtrr.msr_mtrr_def_type);
> +    mtrr_def_type_msr_set(d, mtrr, hw_mtrr.msr_mtrr_def_type);
>  
>      return 0;
>  }
> -- 
> 2.34.1
> 

Re: [XEN PATCH 3/4] x86/irq: rename variable to address MISRA C:2012 Rule 5.3

[Stefano Stabellini]

On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> The extern variable 'irq_desc' defined in 'irq.h' is shadowed by
> local variables in the changed file. To avoid this, the variable is
> renamed to 'irq_description'.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/x86/hvm/hvm.c         |  2 +-
>  xen/arch/x86/include/asm/irq.h |  2 +-
>  xen/arch/x86/io_apic.c         | 10 +++++-----
>  xen/arch/x86/irq.c             | 12 ++++++------
>  xen/arch/x86/msi.c             |  4 ++--
>  xen/include/xen/irq.h          |  2 +-
>  6 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 2180abeb33..ca5bb96388 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -474,7 +474,7 @@ void hvm_migrate_pirq(struct hvm_pirq_dpci *pirq_dpci, const struct vcpu *v)
>  
>          if ( !desc )
>              return;
> -        ASSERT(MSI_IRQ(desc - irq_desc));
> +        ASSERT(MSI_IRQ(desc - irq_descriptor));
>          irq_set_affinity(desc, cpumask_of(v->processor));
>          spin_unlock_irq(&desc->lock);
>      }
> diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
> index ad907fc97f..f6df977170 100644
> --- a/xen/arch/x86/include/asm/irq.h
> +++ b/xen/arch/x86/include/asm/irq.h
> @@ -172,7 +172,7 @@ int assign_irq_vector(int irq, const cpumask_t *mask);
>  
>  void cf_check irq_complete_move(struct irq_desc *desc);
>  
> -extern struct irq_desc *irq_desc;
> +extern struct irq_desc *irq_descriptor;
>  
>  void lock_vector_lock(void);
>  void unlock_vector_lock(void);
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index b3afef8933..b59d6cfb9e 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -990,9 +990,9 @@ static inline void ioapic_register_intr(int irq, unsigned long trigger)
>  {
>      if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
>          trigger == IOAPIC_LEVEL)
> -        irq_desc[irq].handler = &ioapic_level_type;
> +        irq_descriptor[irq].handler = &ioapic_level_type;
>      else
> -        irq_desc[irq].handler = &ioapic_edge_type;
> +        irq_descriptor[irq].handler = &ioapic_edge_type;
>  }
>  
>  static void __init setup_IO_APIC_irqs(void)
> @@ -1098,7 +1098,7 @@ static void __init setup_ExtINT_IRQ0_pin(unsigned int apic, unsigned int pin, in
>       * The timer IRQ doesn't have to know that behind the
>       * scene we have a 8259A-master in AEOI mode ...
>       */
> -    irq_desc[0].handler = &ioapic_edge_type;
> +    irq_descriptor[0].handler = &ioapic_edge_type;
>  
>      /*
>       * Add it to the IO-APIC irq-routing table:
> @@ -1912,7 +1912,7 @@ static void __init check_timer(void)
>      if ((ret = bind_irq_vector(0, vector, &cpumask_all)))
>          printk(KERN_ERR"..IRQ0 is not set correctly with ioapic!!!, err:%d\n", ret);
>      
> -    irq_desc[0].status &= ~IRQ_DISABLED;
> +    irq_descriptor[0].status &= ~IRQ_DISABLED;
>  
>      /*
>       * Subtle, code in do_timer_interrupt() expects an AEOI
> @@ -2009,7 +2009,7 @@ static void __init check_timer(void)
>      printk(KERN_INFO "...trying to set up timer as Virtual Wire IRQ...");
>  
>      disable_8259A_irq(irq_to_desc(0));
> -    irq_desc[0].handler = &lapic_irq_type;
> +    irq_descriptor[0].handler = &lapic_irq_type;
>      apic_write(APIC_LVT0, APIC_DM_FIXED | vector);	/* Fixed mode */
>      enable_8259A_irq(irq_to_desc(0));
>  
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 6abfd81621..ed95896bce 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -45,7 +45,7 @@ integer_param("irq-max-guests", irq_max_guests);
>  
>  vmask_t global_used_vector_map;
>  
> -struct irq_desc __read_mostly *irq_desc = NULL;
> +struct irq_desc __read_mostly *irq_descriptor = NULL;
>  
>  static DECLARE_BITMAP(used_vectors, X86_NR_VECTORS);
>  
> @@ -424,9 +424,9 @@ int __init init_irq_data(void)
>      for ( vector = 0; vector < X86_NR_VECTORS; ++vector )
>          this_cpu(vector_irq)[vector] = INT_MIN;
>  
> -    irq_desc = xzalloc_array(struct irq_desc, nr_irqs);
> -    
> -    if ( !irq_desc )
> +    irq_descriptor = xzalloc_array(struct irq_desc, nr_irqs);
> +
> +    if ( !irq_descriptor )
>          return -ENOMEM;
>  
>      for ( irq = 0; irq < nr_irqs_gsi; irq++ )
> @@ -1133,7 +1133,7 @@ static void cf_check set_eoi_ready(void *data);
>  static void cf_check irq_guest_eoi_timer_fn(void *data)
>  {
>      struct irq_desc *desc = data;
> -    unsigned int i, irq = desc - irq_desc;
> +    unsigned int i, irq = desc - irq_descriptor;
>      irq_guest_action_t *action;
>  
>      spin_lock_irq(&desc->lock);
> @@ -1382,7 +1382,7 @@ static void __set_eoi_ready(const struct irq_desc *desc)
>      struct pending_eoi *peoi = this_cpu(pending_eoi);
>      int                 irq, sp;
>  
> -    irq = desc - irq_desc;
> +    irq = desc - irq_descriptor;
>  
>      if ( !action || action->in_flight ||
>           !cpumask_test_and_clear_cpu(smp_processor_id(),
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index d0bf63df1d..35d417c63a 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1322,7 +1322,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>          unsigned int i = 0, nr = 1;
>  
>          irq = entry->irq;
> -        desc = &irq_desc[irq];
> +        desc = &irq_descriptor[irq];
>  
>          spin_lock_irqsave(&desc->lock, flags);
>  
> @@ -1377,7 +1377,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>                  break;
>  
>              spin_unlock_irqrestore(&desc->lock, flags);
> -            desc = &irq_desc[entry[++i].irq];
> +            desc = &irq_descriptor[entry[++i].irq];
>              spin_lock_irqsave(&desc->lock, flags);
>              if ( desc->msi_desc != entry + i )
>                  goto bogus;
> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index 9747e818f7..56a3aa6a29 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -107,7 +107,7 @@ typedef struct irq_desc {
>  } __cacheline_aligned irq_desc_t;
>  
>  #ifndef irq_to_desc
> -#define irq_to_desc(irq)    (&irq_desc[irq])
> +#define irq_to_desc(irq)    (&irq_descriptor[irq])
>  #endif
>  
>  int init_one_irq_desc(struct irq_desc *desc);
> -- 
> 2.34.1
> 

Re: [XEN PATCH 4/4] x86/setup: address MISRA C:2012 Rule 5.3

[Stefano Stabellini]

On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> The parameters renamed in the function declaration caused shadowing
> with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
> them also addresses Rule 8.3:
> "All declarations of an object or function shall use the same names
> and type qualifiers".
> 
> The local variable 'mask' is removed because it shadows the homonymous
> variable defined in an outer scope, with no change to the semantics.
> It was introduced by commit 5a771800114c437fb857b44b3ed74f60e87979c2
> as a refactoring of the branch that handles 'CONFIG_X86_64' for function
> '__start_xen'.
> 
> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  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 1/4] x86/mce: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 03/08/2023 03:45, Stefano Stabellini wrote:
> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>> Suitable mechanical renames are made to avoid shadowing, thus
>> addressing violations of MISRA C:2012 Rule 5.3:
>> "An identifier declared in an inner scope shall not hide an
>> identifier declared in an outer scope"
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/arch/x86/cpu/mcheck/barrier.c | 8 ++++----
>>  xen/arch/x86/cpu/mcheck/barrier.h | 8 ++++----
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/xen/arch/x86/cpu/mcheck/barrier.c 
>> b/xen/arch/x86/cpu/mcheck/barrier.c
>> index a7e5b19a44..51a1d37a76 100644
>> --- a/xen/arch/x86/cpu/mcheck/barrier.c
>> +++ b/xen/arch/x86/cpu/mcheck/barrier.c
>> @@ -16,11 +16,11 @@ void mce_barrier_dec(struct mce_softirq_barrier 
>> *bar)
>>      atomic_dec(&bar->val);
>>  }
>> 
>> -void mce_barrier_enter(struct mce_softirq_barrier *bar, bool wait)
>> +void mce_barrier_enter(struct mce_softirq_barrier *bar, bool do_wait)
> 
> "wait" clashes with xen/common/sched/core.c:wait, which is globally
> exported, right?
> 
> I think it would be good to add this info to the commit message in this
> kind of patches.
> 

Correct, it's in 'xen/include/xen/wait.h' that makes it visible in the 
file modified
by the patch. I'll add it in v2.

>> -void mce_barrier_enter(struct mce_softirq_barrier *, bool wait);
>> -void mce_barrier_exit(struct mce_softirq_barrier *, bool wait);
>> +void mce_barrier_enter(struct mce_softirq_barrier *, bool do_wait);
>> +void mce_barrier_exit(struct mce_softirq_barrier *, bool do_wait);
> 
> You might as well add "bar" as first parameter?
> 
> 
>>  void mce_barrier(struct mce_softirq_barrier *);
>> 

Will do. I checked that this would not interfere with other patches
related to Rules 8.2 and 8.3.

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

Re: [XEN PATCH 2/4] x86/mtrr: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 03/08/2023 03:51, Stefano Stabellini wrote:
> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>> Rename variables to avoid shadowing and thus address
>> MISRA C:2012 Rule 5.3:
>> "An identifier declared in an inner scope shall not hide an
>> identifier declared in an outer scope"
>> 
>> No functional changes.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> This one clashes with xen/arch/x86/include/asm/mtrr.h:struct mtrr_state
> right?
> 

Yes, indeed. You can also check this when in doubt at
https://saas.eclairit.com, but I do agree that mentioning it directly in 
the commit
would make review easier.

> 
>> ---
>>  xen/arch/x86/hvm/mtrr.c | 32 ++++++++++++++++----------------
>>  1 file changed, 16 insertions(+), 16 deletions(-)
>> 
>> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
>> index 29f3fb1607..d504d1e43b 100644
>> --- a/xen/arch/x86/hvm/mtrr.c
>> +++ b/xen/arch/x86/hvm/mtrr.c
>> @@ -687,13 +687,13 @@ int hvm_set_mem_pinned_cacheattr(struct domain 
>> *d, uint64_t gfn_start,
>> 
>>  static int cf_check hvm_save_mtrr_msr(struct vcpu *v, 
>> hvm_domain_context_t *h)
>>  {
>> -    const struct mtrr_state *mtrr_state = &v->arch.hvm.mtrr;
>> +    const struct mtrr_state *mtrr = &v->arch.hvm.mtrr;
> 
> I can see in hvm_vcpu_cacheattr_init, mtrr_get_type and other places
> that we use:
> 
>   const struct mtrr_state *m
> 
> maybe that's better? I'll let Jan comment.
> 
> Anyway, the patch looks correct to me.
> 
> 

I though about it, and it seemed too generic, so in the end I decided 
for mtrr, but
as always with names, I'm open to alternative suggestions.

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

Re: [XEN PATCH 2/4] x86/mtrr: address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 03.08.2023 03:51, Stefano Stabellini wrote:
> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/hvm/mtrr.c
>> +++ b/xen/arch/x86/hvm/mtrr.c
>> @@ -687,13 +687,13 @@ int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
>>  
>>  static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
>>  {
>> -    const struct mtrr_state *mtrr_state = &v->arch.hvm.mtrr;
>> +    const struct mtrr_state *mtrr = &v->arch.hvm.mtrr;
> 
> I can see in hvm_vcpu_cacheattr_init, mtrr_get_type and other places
> that we use:
> 
>   const struct mtrr_state *m
> 
> maybe that's better? I'll let Jan comment.

When unambiguous, personally I prefer shorter names (and as you say
existing code supports me in that). Obviously the longer a function,
the more potential future ambiguities also need taking into
consideration.

Jan

Re: [XEN PATCH 3/4] x86/irq: rename variable to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 03.08.2023 04:00, Stefano Stabellini wrote:
> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>> The extern variable 'irq_desc' defined in 'irq.h' is shadowed by
>> local variables in the changed file. To avoid this, the variable is
>> renamed to 'irq_description'.
>>
>> No functional change.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Btw, Stefano, could you please trim context when you reply to patches,
and when you add no other remarks besides offering a tag?

Thanks, Jan

Re: [XEN PATCH 3/4] x86/irq: rename variable to address MISRA C:2012 Rule 5.3

[Stefano Stabellini]

On Thu, 3 Aug 2023, Jan Beulich wrote:
> On 03.08.2023 04:00, Stefano Stabellini wrote:
> > On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> >> The extern variable 'irq_desc' defined in 'irq.h' is shadowed by
> >> local variables in the changed file. To avoid this, the variable is
> >> renamed to 'irq_description'.
> >>
> >> No functional change.
> >>
> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > 
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> Btw, Stefano, could you please trim context when you reply to patches,
> and when you add no other remarks besides offering a tag?
> 
> Thanks, Jan

Will do