[XEN PATCH 0/5] Fix violations of MISRA C:2012 Rule 8.3 on parameter names

[XEN PATCH 0/5] Fix violations of MISRA C:2012 Rule 8.3 on parameter names

[Federico Serafini]

To comply with Rule 8.3 ("All declarations of an object or function
shall use the same names and type qualifiers") change the parameter
names in order to have function declarations consistent with the
corresponding definitions.

Federico Serafini (5):
  x86: swap parameter names of hvm_copy_context_and_params() declaration
  x86: change parameter names of nestedhvm_vcpu_iomap_get() definition
  x86/vlapic: change parameter names in function definitions
  x86/x86_emulate: change parameter name from 's' to 'state'
  x86: make parameter names of function declarations consistent

 xen/arch/x86/cpu/mcheck/mce.h           |  2 +-
 xen/arch/x86/cpu/mcheck/x86_mca.h       |  2 +-
 xen/arch/x86/hvm/nestedhvm.c            | 10 ++---
 xen/arch/x86/hvm/rtc.c                  |  2 +-
 xen/arch/x86/hvm/svm/nestedhvm.h        |  2 +-
 xen/arch/x86/hvm/vioapic.c              |  2 +-
 xen/arch/x86/hvm/vlapic.c               | 56 ++++++++++++-------------
 xen/arch/x86/include/asm/genapic.h      |  2 +-
 xen/arch/x86/include/asm/guest_pt.h     |  2 +-
 xen/arch/x86/include/asm/hap.h          |  2 +-
 xen/arch/x86/include/asm/hvm/hvm.h      |  2 +-
 xen/arch/x86/include/asm/hvm/io.h       |  2 +-
 xen/arch/x86/include/asm/hvm/monitor.h  |  2 +-
 xen/arch/x86/include/asm/hvm/svm/vmcb.h |  2 +-
 xen/arch/x86/include/asm/hvm/vmx/vmcs.h |  4 +-
 xen/arch/x86/include/asm/hvm/vmx/vvmx.h |  8 ++--
 xen/arch/x86/include/asm/io_apic.h      |  2 +-
 xen/arch/x86/include/asm/irq.h          |  6 +--
 xen/arch/x86/include/asm/mem_access.h   |  2 +-
 xen/arch/x86/include/asm/mpspec.h       |  2 +-
 xen/arch/x86/include/asm/msi.h          |  4 +-
 xen/arch/x86/include/asm/p2m.h          |  8 ++--
 xen/arch/x86/include/asm/paging.h       |  2 +-
 xen/arch/x86/include/asm/psr.h          |  2 +-
 xen/arch/x86/include/asm/setup.h        |  2 +-
 xen/arch/x86/include/asm/uaccess.h      |  6 +--
 xen/arch/x86/include/asm/xstate.h       |  2 +-
 xen/arch/x86/x86_emulate/blk.c          | 38 ++++++++---------
 xen/arch/x86/x86_emulate/util-xen.c     | 46 ++++++++++----------
 xen/arch/x86/x86_emulate/util.c         | 54 ++++++++++++------------
 xen/include/xen/lib/x86/cpu-policy.h    | 29 ++++++-------
 31 files changed, 154 insertions(+), 153 deletions(-)

-- 
2.34.1

[XEN PATCH 1/5] x86: swap parameter names of hvm_copy_context_and_params() declaration

[Federico Serafini]

Swap parameter names 'src' and 'dst' of hvm_copy_context_and_params()
declaration for consistency with the corresponding definition and the
uses of such function.
Also, this fixes a violation of MISRA C:2012 Rule 8.3.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/include/asm/hvm/hvm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index 04cbd4ff24..9555b4c41f 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -366,7 +366,7 @@ const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
                            signed int cr0_pg);
 unsigned long hvm_cr4_guest_valid_bits(const struct domain *d);
 
-int hvm_copy_context_and_params(struct domain *src, struct domain *dst);
+int hvm_copy_context_and_params(struct domain *dst, struct domain *src);
 
 int hvm_get_param(struct domain *d, uint32_t index, uint64_t *value);
 
-- 
2.34.1

Re: [XEN PATCH 1/5] x86: swap parameter names of hvm_copy_context_and_params() declaration

[Stefano Stabellini]

On Thu, 29 Jun 2023, Federico Serafini wrote:
> Swap parameter names 'src' and 'dst' of hvm_copy_context_and_params()
> declaration for consistency with the corresponding definition and the
> uses of such function.
> Also, this fixes a violation of MISRA C:2012 Rule 8.3.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

better to change the title of the patch to:
x86/hvm: swap parameter names of hvm_copy_context_and_params() declaration

It could be done on commit.


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


> ---
>  xen/arch/x86/include/asm/hvm/hvm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
> index 04cbd4ff24..9555b4c41f 100644
> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -366,7 +366,7 @@ const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
>                             signed int cr0_pg);
>  unsigned long hvm_cr4_guest_valid_bits(const struct domain *d);
>  
> -int hvm_copy_context_and_params(struct domain *src, struct domain *dst);
> +int hvm_copy_context_and_params(struct domain *dst, struct domain *src);
>  
>  int hvm_get_param(struct domain *d, uint32_t index, uint64_t *value);
>  
> -- 
> 2.34.1
> 

[XEN PATCH 2/5] x86: change parameter names of nestedhvm_vcpu_iomap_get() definition

[Federico Serafini]

Change parameter names of nestedhvm_vcpu_iomap_get() definition to
those used in the function declaration in order to:
1) improve readability;
2) fix violations of MISRA C:2012 Rule 8.3.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/hvm/nestedhvm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
index ec68551127..64d7eec9a1 100644
--- a/xen/arch/x86/hvm/nestedhvm.c
+++ b/xen/arch/x86/hvm/nestedhvm.c
@@ -155,19 +155,19 @@ static int __init cf_check nestedhvm_setup(void)
 __initcall(nestedhvm_setup);
 
 unsigned long *
-nestedhvm_vcpu_iomap_get(bool_t port_80, bool_t port_ed)
+nestedhvm_vcpu_iomap_get(bool_t ioport_80, bool_t ioport_ed)
 {
     int i;
 
     if (!hvm_port80_allowed)
-        port_80 = 1;
+        ioport_80 = 1;
 
-    if (port_80 == 0) {
-        if (port_ed == 0)
+    if (ioport_80 == 0) {
+        if (ioport_ed == 0)
             return hvm_io_bitmap;
         i = 0;
     } else {
-        if (port_ed == 0)
+        if (ioport_ed == 0)
             i = 1;
         else
             i = 2;
-- 
2.34.1

Re: [XEN PATCH 2/5] x86: change parameter names of nestedhvm_vcpu_iomap_get() definition

[Stefano Stabellini]

On Thu, 29 Jun 2023, Federico Serafini wrote:
> Change parameter names of nestedhvm_vcpu_iomap_get() definition to
> those used in the function declaration in order to:
> 1) improve readability;
> 2) fix violations of MISRA C:2012 Rule 8.3.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

The title would be better as:
x86/nestedhvm: change parameter names of nestedhvm_vcpu_iomap_get() definition

could be done on commit


> ---
>  xen/arch/x86/hvm/nestedhvm.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
> index ec68551127..64d7eec9a1 100644
> --- a/xen/arch/x86/hvm/nestedhvm.c
> +++ b/xen/arch/x86/hvm/nestedhvm.c
> @@ -155,19 +155,19 @@ static int __init cf_check nestedhvm_setup(void)
>  __initcall(nestedhvm_setup);
>  
>  unsigned long *
> -nestedhvm_vcpu_iomap_get(bool_t port_80, bool_t port_ed)
> +nestedhvm_vcpu_iomap_get(bool_t ioport_80, bool_t ioport_ed)
>  {
>      int i;
>  
>      if (!hvm_port80_allowed)
> -        port_80 = 1;
> +        ioport_80 = 1;
>  
> -    if (port_80 == 0) {
> -        if (port_ed == 0)
> +    if (ioport_80 == 0) {
> +        if (ioport_ed == 0)
>              return hvm_io_bitmap;
>          i = 0;
>      } else {
> -        if (port_ed == 0)
> +        if (ioport_ed == 0)
>              i = 1;
>          else
>              i = 2;
> -- 
> 2.34.1
> 

Re: [XEN PATCH 2/5] x86: change parameter names of nestedhvm_vcpu_iomap_get() definition

[Jan Beulich]

On 29.06.2023 21:25, Stefano Stabellini wrote:
> On Thu, 29 Jun 2023, Federico Serafini wrote:
>> Change parameter names of nestedhvm_vcpu_iomap_get() definition to
>> those used in the function declaration in order to:
>> 1) improve readability;

I see this was committed already, so ftaod no request to revert or what
not, but I disagree with this: Longer names are generally hampering
readability. What would have helped readability is if coding style was
fixed at least for all the line which were touched anyway.

Jan

>> 2) fix violations of MISRA C:2012 Rule 8.3.
>>
>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> The title would be better as:
> x86/nestedhvm: change parameter names of nestedhvm_vcpu_iomap_get() definition
> 
> could be done on commit
> 
> 
>> ---
>>  xen/arch/x86/hvm/nestedhvm.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
>> index ec68551127..64d7eec9a1 100644
>> --- a/xen/arch/x86/hvm/nestedhvm.c
>> +++ b/xen/arch/x86/hvm/nestedhvm.c
>> @@ -155,19 +155,19 @@ static int __init cf_check nestedhvm_setup(void)
>>  __initcall(nestedhvm_setup);
>>  
>>  unsigned long *
>> -nestedhvm_vcpu_iomap_get(bool_t port_80, bool_t port_ed)
>> +nestedhvm_vcpu_iomap_get(bool_t ioport_80, bool_t ioport_ed)
>>  {
>>      int i;
>>  
>>      if (!hvm_port80_allowed)
>> -        port_80 = 1;
>> +        ioport_80 = 1;
>>  
>> -    if (port_80 == 0) {
>> -        if (port_ed == 0)
>> +    if (ioport_80 == 0) {
>> +        if (ioport_ed == 0)
>>              return hvm_io_bitmap;
>>          i = 0;
>>      } else {
>> -        if (port_ed == 0)
>> +        if (ioport_ed == 0)
>>              i = 1;
>>          else
>>              i = 2;
>> -- 
>> 2.34.1
>>

[XEN PATCH 3/5] x86/vlapic: change parameter names in function definitions

[Federico Serafini]

Change parameter names in guest_wrmsr_x2apic() and
guest_wrmsr_apic_base() definitions in order to:
1) keep consistency with parameter names used in guest_* function
   declarations;
2) fix violations of MISRA C:2012 Rule 8.3.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/hvm/vlapic.c | 56 +++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 785d5d88d9..5e0e12a1d7 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -959,7 +959,7 @@ int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
     return X86EMUL_OKAY;
 }
 
-int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
+int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
     uint32_t offset = (msr - MSR_X2APIC_FIRST) << 4;
@@ -973,38 +973,38 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
     switch ( offset )
     {
     case APIC_TASKPRI:
-        if ( msr_content & ~APIC_TPRI_MASK )
+        if ( val & ~APIC_TPRI_MASK )
             return X86EMUL_EXCEPTION;
         break;
 
     case APIC_SPIV:
-        if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
-                             APIC_SPIV_FOCUS_DISABLED |
-                             (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
-                              ? APIC_SPIV_DIRECTED_EOI : 0)) )
+        if ( val & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
+                     APIC_SPIV_FOCUS_DISABLED |
+                     (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
+                      ? APIC_SPIV_DIRECTED_EOI : 0)) )
             return X86EMUL_EXCEPTION;
         break;
 
     case APIC_LVTT:
-        if ( msr_content & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
+        if ( val & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
             return X86EMUL_EXCEPTION;
         break;
 
     case APIC_LVTTHMR:
     case APIC_LVTPC:
     case APIC_CMCI:
-        if ( msr_content & ~(LVT_MASK | APIC_MODE_MASK) )
+        if ( val & ~(LVT_MASK | APIC_MODE_MASK) )
             return X86EMUL_EXCEPTION;
         break;
 
     case APIC_LVT0:
     case APIC_LVT1:
-        if ( msr_content & ~LINT_MASK )
+        if ( val & ~LINT_MASK )
             return X86EMUL_EXCEPTION;
         break;
 
     case APIC_LVTERR:
-        if ( msr_content & ~LVT_MASK )
+        if ( val & ~LVT_MASK )
             return X86EMUL_EXCEPTION;
         break;
 
@@ -1012,35 +1012,35 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
         break;
 
     case APIC_TDCR:
-        if ( msr_content & ~APIC_TDR_DIV_MASK )
+        if ( val & ~APIC_TDR_DIV_MASK )
             return X86EMUL_EXCEPTION;
         break;
 
     case APIC_ICR:
-        if ( (uint32_t)msr_content & ~(APIC_VECTOR_MASK | APIC_MODE_MASK |
+        if ( (uint32_t)val & ~(APIC_VECTOR_MASK | APIC_MODE_MASK |
                                        APIC_DEST_MASK | APIC_INT_ASSERT |
                                        APIC_INT_LEVELTRIG | APIC_SHORT_MASK) )
             return X86EMUL_EXCEPTION;
-        vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
+        vlapic_set_reg(vlapic, APIC_ICR2, val >> 32);
         break;
 
     case APIC_SELF_IPI:
-        if ( msr_content & ~APIC_VECTOR_MASK )
+        if ( val & ~APIC_VECTOR_MASK )
             return X86EMUL_EXCEPTION;
         offset = APIC_ICR;
-        msr_content = APIC_DEST_SELF | (msr_content & APIC_VECTOR_MASK);
+        val = APIC_DEST_SELF | (val & APIC_VECTOR_MASK);
         break;
 
     case APIC_EOI:
     case APIC_ESR:
-        if ( msr_content )
+        if ( val )
         {
     default:
             return X86EMUL_EXCEPTION;
         }
     }
 
-    vlapic_reg_write(v, array_index_nospec(offset, PAGE_SIZE), msr_content);
+    vlapic_reg_write(v, array_index_nospec(offset, PAGE_SIZE), val);
 
     return X86EMUL_OKAY;
 }
@@ -1070,7 +1070,7 @@ static void set_x2apic_id(struct vlapic *vlapic)
     vlapic_set_reg(vlapic, APIC_LDR, ldr);
 }
 
-int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value)
+int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val)
 {
     const struct cpu_policy *cp = v->domain->arch.cpu_policy;
     struct vlapic *vlapic = vcpu_vlapic(v);
@@ -1079,8 +1079,8 @@ int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value)
         return X86EMUL_EXCEPTION;
 
     /* Attempting to set reserved bits? */
-    if ( value & ~(APIC_BASE_ADDR_MASK | APIC_BASE_ENABLE | APIC_BASE_BSP |
-                   (cp->basic.x2apic ? APIC_BASE_EXTD : 0)) )
+    if ( val & ~(APIC_BASE_ADDR_MASK | APIC_BASE_ENABLE | APIC_BASE_BSP |
+                 (cp->basic.x2apic ? APIC_BASE_EXTD : 0)) )
         return X86EMUL_EXCEPTION;
 
     /*
@@ -1118,21 +1118,21 @@ int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value)
      * fault will be far more obvious to debug than a malfunctioning MMIO
      * window.
      */
-    if ( ((value & (APIC_BASE_EXTD | APIC_BASE_ENABLE)) == APIC_BASE_ENABLE) &&
-         ((value & APIC_BASE_ADDR_MASK) != APIC_DEFAULT_PHYS_BASE) )
+    if ( ((val & (APIC_BASE_EXTD | APIC_BASE_ENABLE)) == APIC_BASE_ENABLE) &&
+         ((val & APIC_BASE_ADDR_MASK) != APIC_DEFAULT_PHYS_BASE) )
     {
         printk(XENLOG_G_INFO
                "%pv tried to move the APIC MMIO window: val 0x%08"PRIx64"\n",
-               v, value);
+               v, val);
         return X86EMUL_EXCEPTION;
     }
 
-    if ( (vlapic->hw.apic_base_msr ^ value) & APIC_BASE_ENABLE )
+    if ( (vlapic->hw.apic_base_msr ^ val) & APIC_BASE_ENABLE )
     {
-        if ( unlikely(value & APIC_BASE_EXTD) )
+        if ( unlikely(val & APIC_BASE_EXTD) )
             return X86EMUL_EXCEPTION;
 
-        if ( value & APIC_BASE_ENABLE )
+        if ( val & APIC_BASE_ENABLE )
         {
             vlapic_reset(vlapic);
             vlapic->hw.disabled &= ~VLAPIC_HW_DISABLED;
@@ -1144,11 +1144,11 @@ int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value)
             pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
         }
     }
-    else if ( ((vlapic->hw.apic_base_msr ^ value) & APIC_BASE_EXTD) &&
+    else if ( ((vlapic->hw.apic_base_msr ^ val) & APIC_BASE_EXTD) &&
               unlikely(!vlapic_xapic_mode(vlapic)) )
         return X86EMUL_EXCEPTION;
 
-    vlapic->hw.apic_base_msr = value;
+    vlapic->hw.apic_base_msr = val;
     memset(&vlapic->loaded, 0, sizeof(vlapic->loaded));
 
     if ( vlapic_x2apic_mode(vlapic) )
-- 
2.34.1

Re: [XEN PATCH 3/5] x86/vlapic: change parameter names in function definitions

[Stefano Stabellini]

On Thu, 29 Jun 2023, Federico Serafini wrote:
> Change parameter names in guest_wrmsr_x2apic() and
> guest_wrmsr_apic_base() definitions in order to:
> 1) keep consistency with parameter names used in guest_* function
>    declarations;
> 2) fix violations of MISRA C:2012 Rule 8.3.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

One minor comment below


> ---
>  xen/arch/x86/hvm/vlapic.c | 56 +++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 785d5d88d9..5e0e12a1d7 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -959,7 +959,7 @@ int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
>      return X86EMUL_OKAY;
>  }
>  
> -int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
> +int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val)
>  {
>      struct vlapic *vlapic = vcpu_vlapic(v);
>      uint32_t offset = (msr - MSR_X2APIC_FIRST) << 4;
> @@ -973,38 +973,38 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
>      switch ( offset )
>      {
>      case APIC_TASKPRI:
> -        if ( msr_content & ~APIC_TPRI_MASK )
> +        if ( val & ~APIC_TPRI_MASK )
>              return X86EMUL_EXCEPTION;
>          break;
>  
>      case APIC_SPIV:
> -        if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
> -                             APIC_SPIV_FOCUS_DISABLED |
> -                             (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
> -                              ? APIC_SPIV_DIRECTED_EOI : 0)) )
> +        if ( val & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
> +                     APIC_SPIV_FOCUS_DISABLED |
> +                     (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
> +                      ? APIC_SPIV_DIRECTED_EOI : 0)) )
>              return X86EMUL_EXCEPTION;
>          break;
>  
>      case APIC_LVTT:
> -        if ( msr_content & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
> +        if ( val & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
>              return X86EMUL_EXCEPTION;
>          break;
>  
>      case APIC_LVTTHMR:
>      case APIC_LVTPC:
>      case APIC_CMCI:
> -        if ( msr_content & ~(LVT_MASK | APIC_MODE_MASK) )
> +        if ( val & ~(LVT_MASK | APIC_MODE_MASK) )
>              return X86EMUL_EXCEPTION;
>          break;
>  
>      case APIC_LVT0:
>      case APIC_LVT1:
> -        if ( msr_content & ~LINT_MASK )
> +        if ( val & ~LINT_MASK )
>              return X86EMUL_EXCEPTION;
>          break;
>  
>      case APIC_LVTERR:
> -        if ( msr_content & ~LVT_MASK )
> +        if ( val & ~LVT_MASK )
>              return X86EMUL_EXCEPTION;
>          break;
>  
> @@ -1012,35 +1012,35 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
>          break;
>  
>      case APIC_TDCR:
> -        if ( msr_content & ~APIC_TDR_DIV_MASK )
> +        if ( val & ~APIC_TDR_DIV_MASK )
>              return X86EMUL_EXCEPTION;
>          break;
>  
>      case APIC_ICR:
> -        if ( (uint32_t)msr_content & ~(APIC_VECTOR_MASK | APIC_MODE_MASK |
> +        if ( (uint32_t)val & ~(APIC_VECTOR_MASK | APIC_MODE_MASK |
>                                         APIC_DEST_MASK | APIC_INT_ASSERT |
>                                         APIC_INT_LEVELTRIG | APIC_SHORT_MASK) )

code style: the alignment here should be fixed, it can be done on commit



>              return X86EMUL_EXCEPTION;
> -        vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
> +        vlapic_set_reg(vlapic, APIC_ICR2, val >> 32);
>          break;
>  
>      case APIC_SELF_IPI:
> -        if ( msr_content & ~APIC_VECTOR_MASK )
> +        if ( val & ~APIC_VECTOR_MASK )
>              return X86EMUL_EXCEPTION;
>          offset = APIC_ICR;
> -        msr_content = APIC_DEST_SELF | (msr_content & APIC_VECTOR_MASK);
> +        val = APIC_DEST_SELF | (val & APIC_VECTOR_MASK);
>          break;
>  
>      case APIC_EOI:
>      case APIC_ESR:
> -        if ( msr_content )
> +        if ( val )
>          {
>      default:
>              return X86EMUL_EXCEPTION;
>          }
>      }
>  
> -    vlapic_reg_write(v, array_index_nospec(offset, PAGE_SIZE), msr_content);
> +    vlapic_reg_write(v, array_index_nospec(offset, PAGE_SIZE), val);
>  
>      return X86EMUL_OKAY;
>  }
> @@ -1070,7 +1070,7 @@ static void set_x2apic_id(struct vlapic *vlapic)
>      vlapic_set_reg(vlapic, APIC_LDR, ldr);
>  }
>  
> -int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value)
> +int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val)
>  {
>      const struct cpu_policy *cp = v->domain->arch.cpu_policy;
>      struct vlapic *vlapic = vcpu_vlapic(v);
> @@ -1079,8 +1079,8 @@ int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value)
>          return X86EMUL_EXCEPTION;
>  
>      /* Attempting to set reserved bits? */
> -    if ( value & ~(APIC_BASE_ADDR_MASK | APIC_BASE_ENABLE | APIC_BASE_BSP |
> -                   (cp->basic.x2apic ? APIC_BASE_EXTD : 0)) )
> +    if ( val & ~(APIC_BASE_ADDR_MASK | APIC_BASE_ENABLE | APIC_BASE_BSP |
> +                 (cp->basic.x2apic ? APIC_BASE_EXTD : 0)) )
>          return X86EMUL_EXCEPTION;
>  
>      /*
> @@ -1118,21 +1118,21 @@ int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value)
>       * fault will be far more obvious to debug than a malfunctioning MMIO
>       * window.
>       */
> -    if ( ((value & (APIC_BASE_EXTD | APIC_BASE_ENABLE)) == APIC_BASE_ENABLE) &&
> -         ((value & APIC_BASE_ADDR_MASK) != APIC_DEFAULT_PHYS_BASE) )
> +    if ( ((val & (APIC_BASE_EXTD | APIC_BASE_ENABLE)) == APIC_BASE_ENABLE) &&
> +         ((val & APIC_BASE_ADDR_MASK) != APIC_DEFAULT_PHYS_BASE) )
>      {
>          printk(XENLOG_G_INFO
>                 "%pv tried to move the APIC MMIO window: val 0x%08"PRIx64"\n",
> -               v, value);
> +               v, val);
>          return X86EMUL_EXCEPTION;
>      }
>  
> -    if ( (vlapic->hw.apic_base_msr ^ value) & APIC_BASE_ENABLE )
> +    if ( (vlapic->hw.apic_base_msr ^ val) & APIC_BASE_ENABLE )
>      {
> -        if ( unlikely(value & APIC_BASE_EXTD) )
> +        if ( unlikely(val & APIC_BASE_EXTD) )
>              return X86EMUL_EXCEPTION;
>  
> -        if ( value & APIC_BASE_ENABLE )
> +        if ( val & APIC_BASE_ENABLE )
>          {
>              vlapic_reset(vlapic);
>              vlapic->hw.disabled &= ~VLAPIC_HW_DISABLED;
> @@ -1144,11 +1144,11 @@ int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value)
>              pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
>          }
>      }
> -    else if ( ((vlapic->hw.apic_base_msr ^ value) & APIC_BASE_EXTD) &&
> +    else if ( ((vlapic->hw.apic_base_msr ^ val) & APIC_BASE_EXTD) &&
>                unlikely(!vlapic_xapic_mode(vlapic)) )
>          return X86EMUL_EXCEPTION;
>  
> -    vlapic->hw.apic_base_msr = value;
> +    vlapic->hw.apic_base_msr = val;
>      memset(&vlapic->loaded, 0, sizeof(vlapic->loaded));
>  
>      if ( vlapic_x2apic_mode(vlapic) )
> -- 
> 2.34.1
> 

[XEN PATCH 4/5] x86/x86_emulate: change parameter name from 's' to 'state'

[Federico Serafini]

Change parameter name from 's' to 'state' in function definitions in
order to:
1) keep consistency with the parameter names used in the corresponding
   declarations;
2) keep consistency with parameter names used within x86_emulate.h;
3) fix violations of MISRA C:2012 Rule 8.3.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/x86_emulate/blk.c      | 38 ++++++++++----------
 xen/arch/x86/x86_emulate/util-xen.c | 46 ++++++++++++------------
 xen/arch/x86/x86_emulate/util.c     | 54 ++++++++++++++---------------
 3 files changed, 69 insertions(+), 69 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/blk.c b/xen/arch/x86/x86_emulate/blk.c
index e790f4f900..23eeb00db2 100644
--- a/xen/arch/x86/x86_emulate/blk.c
+++ b/xen/arch/x86/x86_emulate/blk.c
@@ -22,12 +22,12 @@ int x86_emul_blk(
     void *data,
     unsigned int bytes,
     uint32_t *eflags,
-    struct x86_emulate_state *s,
+    struct x86_emulate_state *state,
     struct x86_emulate_ctxt *ctxt)
 {
     int rc = X86EMUL_OKAY;
 
-    switch ( s->blk )
+    switch ( state->blk )
     {
         bool zf;
 #ifndef X86EMUL_NO_FPU
@@ -72,13 +72,13 @@ int x86_emul_blk(
     case blk_fld:
         ASSERT(!data);
 
-        /* s->rex_prefix carries CR0.PE && !EFLAGS.VM setting */
+        /* state->rex_prefix carries CR0.PE && !EFLAGS.VM setting */
         switch ( bytes )
         {
         case sizeof(fpstate.env): /* 32-bit FLDENV */
         case sizeof(fpstate):     /* 32-bit FRSTOR */
             memcpy(&fpstate.env, ptr, sizeof(fpstate.env));
-            if ( !s->rex_prefix )
+            if ( !state->rex_prefix )
             {
                 /* Convert 32-bit real/vm86 to 32-bit prot format. */
                 unsigned int fip = fpstate.env.mode.real.fip_lo +
@@ -109,7 +109,7 @@ int x86_emul_blk(
             fpstate.env.fsw = env->fsw;
             fpstate.env.ftw = env->ftw;
 
-            if ( s->rex_prefix )
+            if ( state->rex_prefix )
             {
                 /* Convert 16-bit prot to 32-bit prot format. */
                 fpstate.env.mode.prot.fip = env->mode.prot.fip;
@@ -165,12 +165,12 @@ int x86_emul_blk(
         else
             asm ( "fnstenv %0" : "+m" (fpstate.env) );
 
-        /* s->rex_prefix carries CR0.PE && !EFLAGS.VM setting */
+        /* state->rex_prefix carries CR0.PE && !EFLAGS.VM setting */
         switch ( bytes )
         {
         case sizeof(fpstate.env): /* 32-bit FNSTENV */
         case sizeof(fpstate):     /* 32-bit FNSAVE */
-            if ( !s->rex_prefix )
+            if ( !state->rex_prefix )
             {
                 /* Convert 32-bit prot to 32-bit real/vm86 format. */
                 unsigned int fip = fpstate.env.mode.prot.fip +
@@ -195,7 +195,7 @@ int x86_emul_blk(
 
         case sizeof(struct x87_env16):                        /* 16-bit FNSTENV */
         case sizeof(struct x87_env16) + sizeof(fpstate.freg): /* 16-bit FNSAVE */
-            if ( s->rex_prefix )
+            if ( state->rex_prefix )
             {
                 /* Convert 32-bit prot to 16-bit prot format. */
                 struct x87_env16 *env = ptr;
@@ -254,11 +254,11 @@ int x86_emul_blk(
 
         ASSERT(!data);
         ASSERT(bytes == sizeof(*fxsr));
-        ASSERT(s->op_bytes <= bytes);
+        ASSERT(state->op_bytes <= bytes);
 
-        if ( s->op_bytes < sizeof(*fxsr) )
+        if ( state->op_bytes < sizeof(*fxsr) )
         {
-            if ( s->rex_prefix & REX_W )
+            if ( state->rex_prefix & REX_W )
             {
                 /*
                  * The only way to force fxsaveq on a wide range of gas
@@ -278,13 +278,13 @@ int x86_emul_blk(
          * data FXRSTOR may actually consume in some way: Copy only the
          * defined portion, and zero the rest.
          */
-        memcpy(fxsr, ptr, min(s->op_bytes,
+        memcpy(fxsr, ptr, min(state->op_bytes,
                               (unsigned int)offsetof(struct x86_fxsr, rsvd)));
         memset(fxsr->rsvd, 0, sizeof(*fxsr) - offsetof(struct x86_fxsr, rsvd));
 
         generate_exception_if(fxsr->mxcsr & ~mxcsr_mask, X86_EXC_GP, 0);
 
-        if ( s->rex_prefix & REX_W )
+        if ( state->rex_prefix & REX_W )
         {
             /* See above for why operand/constraints are this way. */
             asm volatile ( ".byte 0x48; fxrstor (%1)"
@@ -301,15 +301,15 @@ int x86_emul_blk(
 
         ASSERT(!data);
         ASSERT(bytes == sizeof(*fxsr));
-        ASSERT(s->op_bytes <= bytes);
+        ASSERT(state->op_bytes <= bytes);
 
-        if ( s->op_bytes < sizeof(*fxsr) )
+        if ( state->op_bytes < sizeof(*fxsr) )
             /* Don't chance consuming uninitialized data. */
-            memset(fxsr, 0, s->op_bytes);
+            memset(fxsr, 0, state->op_bytes);
         else
             fxsr = ptr;
 
-        if ( s->rex_prefix & REX_W )
+        if ( state->rex_prefix & REX_W )
         {
             /* See above for why operand/constraints are this way. */
             asm volatile ( ".byte 0x48; fxsave (%1)"
@@ -318,8 +318,8 @@ int x86_emul_blk(
         else
             asm volatile ( "fxsave %0" : "=m" (*fxsr) );
 
-        if ( fxsr != ptr ) /* i.e. s->op_bytes < sizeof(*fxsr) */
-            memcpy(ptr, fxsr, s->op_bytes);
+        if ( fxsr != ptr ) /* i.e. state->op_bytes < sizeof(*fxsr) */
+            memcpy(ptr, fxsr, state->op_bytes);
         break;
     }
 
diff --git a/xen/arch/x86/x86_emulate/util-xen.c b/xen/arch/x86/x86_emulate/util-xen.c
index 5e90818010..b36acbe1b0 100644
--- a/xen/arch/x86/x86_emulate/util-xen.c
+++ b/xen/arch/x86/x86_emulate/util-xen.c
@@ -14,26 +14,26 @@
 #include <asm/xstate.h>
 
 #ifndef NDEBUG
-void x86_emulate_free_state(struct x86_emulate_state *s)
+void x86_emulate_free_state(struct x86_emulate_state *state)
 {
-    check_state(s);
-    s->caller = NULL;
+    check_state(state);
+    state->caller = NULL;
 }
 #endif
 
-unsigned int x86_insn_opsize(const struct x86_emulate_state *s)
+unsigned int x86_insn_opsize(const struct x86_emulate_state *state)
 {
-    check_state(s);
+    check_state(state);
 
-    return s->op_bytes << 3;
+    return state->op_bytes << 3;
 }
 
-int x86_insn_modrm(const struct x86_emulate_state *s,
+int x86_insn_modrm(const struct x86_emulate_state *state,
                    unsigned int *rm, unsigned int *reg)
 {
-    check_state(s);
+    check_state(state);
 
-    if ( unlikely(s->modrm_mod > 3) )
+    if ( unlikely(state->modrm_mod > 3) )
     {
         if ( rm )
             *rm = ~0U;
@@ -43,24 +43,24 @@ int x86_insn_modrm(const struct x86_emulate_state *s,
     }
 
     if ( rm )
-        *rm = s->modrm_rm;
+        *rm = state->modrm_rm;
     if ( reg )
-        *reg = s->modrm_reg;
+        *reg = state->modrm_reg;
 
-    return s->modrm_mod;
+    return state->modrm_mod;
 }
 
-unsigned long x86_insn_operand_ea(const struct x86_emulate_state *s,
+unsigned long x86_insn_operand_ea(const struct x86_emulate_state *state,
                                   enum x86_segment *seg)
 {
-    *seg = s->ea.type == OP_MEM ? s->ea.mem.seg : x86_seg_none;
+    *seg = state->ea.type == OP_MEM ? state->ea.mem.seg : x86_seg_none;
 
-    check_state(s);
+    check_state(state);
 
-    return s->ea.mem.off;
+    return state->ea.mem.off;
 }
 
-bool cf_check x86_insn_is_portio(const struct x86_emulate_state *s,
+bool cf_check x86_insn_is_portio(const struct x86_emulate_state *state,
                                  const struct x86_emulate_ctxt *ctxt)
 {
     switch ( ctxt->opcode )
@@ -74,7 +74,7 @@ bool cf_check x86_insn_is_portio(const struct x86_emulate_state *s,
     return false;
 }
 
-bool cf_check x86_insn_is_cr_access(const struct x86_emulate_state *s,
+bool cf_check x86_insn_is_cr_access(const struct x86_emulate_state *state,
                                     const struct x86_emulate_ctxt *ctxt)
 {
     switch ( ctxt->opcode )
@@ -82,7 +82,7 @@ bool cf_check x86_insn_is_cr_access(const struct x86_emulate_state *s,
         unsigned int ext;
 
     case X86EMUL_OPC(0x0f, 0x01):
-        if ( x86_insn_modrm(s, NULL, &ext) >= 0
+        if ( x86_insn_modrm(state, NULL, &ext) >= 0
              && (ext & 5) == 4 ) /* SMSW / LMSW */
             return true;
         break;
@@ -96,17 +96,17 @@ bool cf_check x86_insn_is_cr_access(const struct x86_emulate_state *s,
     return false;
 }
 
-unsigned long x86_insn_immediate(const struct x86_emulate_state *s,
+unsigned long x86_insn_immediate(const struct x86_emulate_state *state,
                                  unsigned int nr)
 {
-    check_state(s);
+    check_state(state);
 
     switch ( nr )
     {
     case 0:
-        return s->imm1;
+        return state->imm1;
     case 1:
-        return s->imm2;
+        return state->imm2;
     }
 
     return 0;
diff --git a/xen/arch/x86/x86_emulate/util.c b/xen/arch/x86/x86_emulate/util.c
index 4becd054c2..34daa8467f 100644
--- a/xen/arch/x86/x86_emulate/util.c
+++ b/xen/arch/x86/x86_emulate/util.c
@@ -8,12 +8,12 @@
 
 #include "private.h"
 
-unsigned int x86_insn_length(const struct x86_emulate_state *s,
+unsigned int x86_insn_length(const struct x86_emulate_state *state,
                              const struct x86_emulate_ctxt *ctxt)
 {
-    check_state(s);
+    check_state(state);
 
-    return s->ip - ctxt->regs->r(ip);
+    return state->ip - ctxt->regs->r(ip);
 }
 
 /*
@@ -22,13 +22,13 @@ unsigned int x86_insn_length(const struct x86_emulate_state *s,
  * memory operand (like POP), but it does not mean e.g. segment selector
  * loads, where the descriptor table access is considered an implicit one.
  */
-bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *s,
+bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *state,
                                      const struct x86_emulate_ctxt *ctxt)
 {
-    if ( mode_64bit() && s->not_64bit )
+    if ( mode_64bit() && state->not_64bit )
         return false;
 
-    if ( s->ea.type == OP_MEM )
+    if ( state->ea.type == OP_MEM )
     {
         switch ( ctxt->opcode )
         {
@@ -49,13 +49,13 @@ bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *s,
             return false;
 
         case X86EMUL_OPC(0x0f, 0x01):
-            return (s->modrm_reg & 7) != 7; /* INVLPG */
+            return (state->modrm_reg & 7) != 7; /* INVLPG */
 
         case X86EMUL_OPC(0x0f, 0xae):
-            return (s->modrm_reg & 7) != 7; /* CLFLUSH */
+            return (state->modrm_reg & 7) != 7; /* CLFLUSH */
 
         case X86EMUL_OPC_66(0x0f, 0xae):
-            return (s->modrm_reg & 7) < 6; /* CLWB, CLFLUSHOPT */
+            return (state->modrm_reg & 7) < 6; /* CLWB, CLFLUSHOPT */
         }
 
         return true;
@@ -91,7 +91,7 @@ bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *s,
         return true;
 
     case 0xff:
-        switch ( s->modrm_reg & 7 )
+        switch ( state->modrm_reg & 7 )
         {
         case 2: /* CALL (near, indirect) */
         case 6: /* PUSH r/m */
@@ -101,7 +101,7 @@ bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *s,
 
     case X86EMUL_OPC(0x0f, 0x01):
         /* Cover CLZERO. */
-        return (s->modrm_rm & 7) == 4 && (s->modrm_reg & 7) == 7;
+        return (state->modrm_rm & 7) == 4 && (state->modrm_reg & 7) == 7;
     }
 
     return false;
@@ -114,17 +114,17 @@ bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *s,
  * loads, where the (possible) descriptor table write is considered an
  * implicit access.
  */
-bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
+bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *state,
                                     const struct x86_emulate_ctxt *ctxt)
 {
-    if ( mode_64bit() && s->not_64bit )
+    if ( mode_64bit() && state->not_64bit )
         return false;
 
-    switch ( s->desc & DstMask )
+    switch ( state->desc & DstMask )
     {
     case DstMem:
         /* The SrcMem check is to cover {,V}MASKMOV{Q,DQU}. */
-        return s->modrm_mod != 3 || (s->desc & SrcMask) == SrcMem;
+        return state->modrm_mod != 3 || (state->desc & SrcMask) == SrcMem;
 
     case DstBitBase:
     case DstImplicit:
@@ -147,13 +147,13 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
              X86EMUL_OPC_EVEX_F3(0x0f38, 0x25): /* VPMOVS* */
         case X86EMUL_OPC_EVEX_F3(0x0f38, 0x30) ...
              X86EMUL_OPC_EVEX_F3(0x0f38, 0x35): /* VPMOV{D,Q,W}* */
-            return s->modrm_mod != 3;
+            return state->modrm_mod != 3;
         }
 
         return false;
     }
 
-    if ( s->modrm_mod == 3 )
+    if ( state->modrm_mod == 3 )
     {
         switch ( ctxt->opcode )
         {
@@ -161,7 +161,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
             break;
 
         case X86EMUL_OPC(0x0f, 0x01): /* CLZERO is the odd one. */
-            return (s->modrm_rm & 7) == 4 && (s->modrm_reg & 7) == 7;
+            return (state->modrm_rm & 7) == 4 && (state->modrm_reg & 7) == 7;
 
         default:
             return false;
@@ -192,7 +192,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
         return true;
 
     case 0xd9:
-        switch ( s->modrm_reg & 7 )
+        switch ( state->modrm_reg & 7 )
         {
         case 2: /* FST m32fp */
         case 3: /* FSTP m32fp */
@@ -203,7 +203,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
         break;
 
     case 0xdb:
-        switch ( s->modrm_reg & 7 )
+        switch ( state->modrm_reg & 7 )
         {
         case 1: /* FISTTP m32i */
         case 2: /* FIST m32i */
@@ -214,7 +214,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
         break;
 
     case 0xdd:
-        switch ( s->modrm_reg & 7 )
+        switch ( state->modrm_reg & 7 )
         {
         case 1: /* FISTTP m64i */
         case 2: /* FST m64fp */
@@ -226,7 +226,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
         break;
 
     case 0xdf:
-        switch ( s->modrm_reg & 7 )
+        switch ( state->modrm_reg & 7 )
         {
         case 1: /* FISTTP m16i */
         case 2: /* FIST m16i */
@@ -238,7 +238,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
         break;
 
     case 0xff:
-        switch ( s->modrm_reg & 7 )
+        switch ( state->modrm_reg & 7 )
         {
         case 2: /* CALL (near, indirect) */
         case 3: /* CALL (far, indirect) */
@@ -248,7 +248,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
         break;
 
     case X86EMUL_OPC(0x0f, 0x01):
-        switch ( s->modrm_reg & 7 )
+        switch ( state->modrm_reg & 7 )
         {
         case 0: /* SGDT */
         case 1: /* SIDT */
@@ -258,7 +258,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
         break;
 
     case X86EMUL_OPC(0x0f, 0xae):
-        switch ( s->modrm_reg & 7 )
+        switch ( state->modrm_reg & 7 )
         {
         case 0: /* FXSAVE */
         /* case 3: STMXCSR - handled above */
@@ -269,10 +269,10 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
         break;
 
     case X86EMUL_OPC(0x0f, 0xba):
-        return (s->modrm_reg & 7) > 4; /* BTS / BTR / BTC */
+        return (state->modrm_reg & 7) > 4; /* BTS / BTR / BTC */
 
     case X86EMUL_OPC(0x0f, 0xc7):
-        switch ( s->modrm_reg & 7 )
+        switch ( state->modrm_reg & 7 )
         {
         case 1: /* CMPXCHG{8,16}B */
         case 4: /* XSAVEC */
-- 
2.34.1

Re: [XEN PATCH 4/5] x86/x86_emulate: change parameter name from 's' to 'state'

[Stefano Stabellini]

On Thu, 29 Jun 2023, Federico Serafini wrote:
> Change parameter name from 's' to 'state' in function definitions in
> order to:
> 1) keep consistency with the parameter names used in the corresponding
>    declarations;
> 2) keep consistency with parameter names used within x86_emulate.h;
> 3) fix violations of MISRA C:2012 Rule 8.3.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

You could use x86emul: as tag in the title. I'll let Jan choose the tag
he prefers.

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


> ---
>  xen/arch/x86/x86_emulate/blk.c      | 38 ++++++++++----------
>  xen/arch/x86/x86_emulate/util-xen.c | 46 ++++++++++++------------
>  xen/arch/x86/x86_emulate/util.c     | 54 ++++++++++++++---------------
>  3 files changed, 69 insertions(+), 69 deletions(-)
> 
> diff --git a/xen/arch/x86/x86_emulate/blk.c b/xen/arch/x86/x86_emulate/blk.c
> index e790f4f900..23eeb00db2 100644
> --- a/xen/arch/x86/x86_emulate/blk.c
> +++ b/xen/arch/x86/x86_emulate/blk.c
> @@ -22,12 +22,12 @@ int x86_emul_blk(
>      void *data,
>      unsigned int bytes,
>      uint32_t *eflags,
> -    struct x86_emulate_state *s,
> +    struct x86_emulate_state *state,
>      struct x86_emulate_ctxt *ctxt)
>  {
>      int rc = X86EMUL_OKAY;
>  
> -    switch ( s->blk )
> +    switch ( state->blk )
>      {
>          bool zf;
>  #ifndef X86EMUL_NO_FPU
> @@ -72,13 +72,13 @@ int x86_emul_blk(
>      case blk_fld:
>          ASSERT(!data);
>  
> -        /* s->rex_prefix carries CR0.PE && !EFLAGS.VM setting */
> +        /* state->rex_prefix carries CR0.PE && !EFLAGS.VM setting */
>          switch ( bytes )
>          {
>          case sizeof(fpstate.env): /* 32-bit FLDENV */
>          case sizeof(fpstate):     /* 32-bit FRSTOR */
>              memcpy(&fpstate.env, ptr, sizeof(fpstate.env));
> -            if ( !s->rex_prefix )
> +            if ( !state->rex_prefix )
>              {
>                  /* Convert 32-bit real/vm86 to 32-bit prot format. */
>                  unsigned int fip = fpstate.env.mode.real.fip_lo +
> @@ -109,7 +109,7 @@ int x86_emul_blk(
>              fpstate.env.fsw = env->fsw;
>              fpstate.env.ftw = env->ftw;
>  
> -            if ( s->rex_prefix )
> +            if ( state->rex_prefix )
>              {
>                  /* Convert 16-bit prot to 32-bit prot format. */
>                  fpstate.env.mode.prot.fip = env->mode.prot.fip;
> @@ -165,12 +165,12 @@ int x86_emul_blk(
>          else
>              asm ( "fnstenv %0" : "+m" (fpstate.env) );
>  
> -        /* s->rex_prefix carries CR0.PE && !EFLAGS.VM setting */
> +        /* state->rex_prefix carries CR0.PE && !EFLAGS.VM setting */
>          switch ( bytes )
>          {
>          case sizeof(fpstate.env): /* 32-bit FNSTENV */
>          case sizeof(fpstate):     /* 32-bit FNSAVE */
> -            if ( !s->rex_prefix )
> +            if ( !state->rex_prefix )
>              {
>                  /* Convert 32-bit prot to 32-bit real/vm86 format. */
>                  unsigned int fip = fpstate.env.mode.prot.fip +
> @@ -195,7 +195,7 @@ int x86_emul_blk(
>  
>          case sizeof(struct x87_env16):                        /* 16-bit FNSTENV */
>          case sizeof(struct x87_env16) + sizeof(fpstate.freg): /* 16-bit FNSAVE */
> -            if ( s->rex_prefix )
> +            if ( state->rex_prefix )
>              {
>                  /* Convert 32-bit prot to 16-bit prot format. */
>                  struct x87_env16 *env = ptr;
> @@ -254,11 +254,11 @@ int x86_emul_blk(
>  
>          ASSERT(!data);
>          ASSERT(bytes == sizeof(*fxsr));
> -        ASSERT(s->op_bytes <= bytes);
> +        ASSERT(state->op_bytes <= bytes);
>  
> -        if ( s->op_bytes < sizeof(*fxsr) )
> +        if ( state->op_bytes < sizeof(*fxsr) )
>          {
> -            if ( s->rex_prefix & REX_W )
> +            if ( state->rex_prefix & REX_W )
>              {
>                  /*
>                   * The only way to force fxsaveq on a wide range of gas
> @@ -278,13 +278,13 @@ int x86_emul_blk(
>           * data FXRSTOR may actually consume in some way: Copy only the
>           * defined portion, and zero the rest.
>           */
> -        memcpy(fxsr, ptr, min(s->op_bytes,
> +        memcpy(fxsr, ptr, min(state->op_bytes,
>                                (unsigned int)offsetof(struct x86_fxsr, rsvd)));
>          memset(fxsr->rsvd, 0, sizeof(*fxsr) - offsetof(struct x86_fxsr, rsvd));
>  
>          generate_exception_if(fxsr->mxcsr & ~mxcsr_mask, X86_EXC_GP, 0);
>  
> -        if ( s->rex_prefix & REX_W )
> +        if ( state->rex_prefix & REX_W )
>          {
>              /* See above for why operand/constraints are this way. */
>              asm volatile ( ".byte 0x48; fxrstor (%1)"
> @@ -301,15 +301,15 @@ int x86_emul_blk(
>  
>          ASSERT(!data);
>          ASSERT(bytes == sizeof(*fxsr));
> -        ASSERT(s->op_bytes <= bytes);
> +        ASSERT(state->op_bytes <= bytes);
>  
> -        if ( s->op_bytes < sizeof(*fxsr) )
> +        if ( state->op_bytes < sizeof(*fxsr) )
>              /* Don't chance consuming uninitialized data. */
> -            memset(fxsr, 0, s->op_bytes);
> +            memset(fxsr, 0, state->op_bytes);
>          else
>              fxsr = ptr;
>  
> -        if ( s->rex_prefix & REX_W )
> +        if ( state->rex_prefix & REX_W )
>          {
>              /* See above for why operand/constraints are this way. */
>              asm volatile ( ".byte 0x48; fxsave (%1)"
> @@ -318,8 +318,8 @@ int x86_emul_blk(
>          else
>              asm volatile ( "fxsave %0" : "=m" (*fxsr) );
>  
> -        if ( fxsr != ptr ) /* i.e. s->op_bytes < sizeof(*fxsr) */
> -            memcpy(ptr, fxsr, s->op_bytes);
> +        if ( fxsr != ptr ) /* i.e. state->op_bytes < sizeof(*fxsr) */
> +            memcpy(ptr, fxsr, state->op_bytes);
>          break;
>      }
>  
> diff --git a/xen/arch/x86/x86_emulate/util-xen.c b/xen/arch/x86/x86_emulate/util-xen.c
> index 5e90818010..b36acbe1b0 100644
> --- a/xen/arch/x86/x86_emulate/util-xen.c
> +++ b/xen/arch/x86/x86_emulate/util-xen.c
> @@ -14,26 +14,26 @@
>  #include <asm/xstate.h>
>  
>  #ifndef NDEBUG
> -void x86_emulate_free_state(struct x86_emulate_state *s)
> +void x86_emulate_free_state(struct x86_emulate_state *state)
>  {
> -    check_state(s);
> -    s->caller = NULL;
> +    check_state(state);
> +    state->caller = NULL;
>  }
>  #endif
>  
> -unsigned int x86_insn_opsize(const struct x86_emulate_state *s)
> +unsigned int x86_insn_opsize(const struct x86_emulate_state *state)
>  {
> -    check_state(s);
> +    check_state(state);
>  
> -    return s->op_bytes << 3;
> +    return state->op_bytes << 3;
>  }
>  
> -int x86_insn_modrm(const struct x86_emulate_state *s,
> +int x86_insn_modrm(const struct x86_emulate_state *state,
>                     unsigned int *rm, unsigned int *reg)
>  {
> -    check_state(s);
> +    check_state(state);
>  
> -    if ( unlikely(s->modrm_mod > 3) )
> +    if ( unlikely(state->modrm_mod > 3) )
>      {
>          if ( rm )
>              *rm = ~0U;
> @@ -43,24 +43,24 @@ int x86_insn_modrm(const struct x86_emulate_state *s,
>      }
>  
>      if ( rm )
> -        *rm = s->modrm_rm;
> +        *rm = state->modrm_rm;
>      if ( reg )
> -        *reg = s->modrm_reg;
> +        *reg = state->modrm_reg;
>  
> -    return s->modrm_mod;
> +    return state->modrm_mod;
>  }
>  
> -unsigned long x86_insn_operand_ea(const struct x86_emulate_state *s,
> +unsigned long x86_insn_operand_ea(const struct x86_emulate_state *state,
>                                    enum x86_segment *seg)
>  {
> -    *seg = s->ea.type == OP_MEM ? s->ea.mem.seg : x86_seg_none;
> +    *seg = state->ea.type == OP_MEM ? state->ea.mem.seg : x86_seg_none;
>  
> -    check_state(s);
> +    check_state(state);
>  
> -    return s->ea.mem.off;
> +    return state->ea.mem.off;
>  }
>  
> -bool cf_check x86_insn_is_portio(const struct x86_emulate_state *s,
> +bool cf_check x86_insn_is_portio(const struct x86_emulate_state *state,
>                                   const struct x86_emulate_ctxt *ctxt)
>  {
>      switch ( ctxt->opcode )
> @@ -74,7 +74,7 @@ bool cf_check x86_insn_is_portio(const struct x86_emulate_state *s,
>      return false;
>  }
>  
> -bool cf_check x86_insn_is_cr_access(const struct x86_emulate_state *s,
> +bool cf_check x86_insn_is_cr_access(const struct x86_emulate_state *state,
>                                      const struct x86_emulate_ctxt *ctxt)
>  {
>      switch ( ctxt->opcode )
> @@ -82,7 +82,7 @@ bool cf_check x86_insn_is_cr_access(const struct x86_emulate_state *s,
>          unsigned int ext;
>  
>      case X86EMUL_OPC(0x0f, 0x01):
> -        if ( x86_insn_modrm(s, NULL, &ext) >= 0
> +        if ( x86_insn_modrm(state, NULL, &ext) >= 0
>               && (ext & 5) == 4 ) /* SMSW / LMSW */
>              return true;
>          break;
> @@ -96,17 +96,17 @@ bool cf_check x86_insn_is_cr_access(const struct x86_emulate_state *s,
>      return false;
>  }
>  
> -unsigned long x86_insn_immediate(const struct x86_emulate_state *s,
> +unsigned long x86_insn_immediate(const struct x86_emulate_state *state,
>                                   unsigned int nr)
>  {
> -    check_state(s);
> +    check_state(state);
>  
>      switch ( nr )
>      {
>      case 0:
> -        return s->imm1;
> +        return state->imm1;
>      case 1:
> -        return s->imm2;
> +        return state->imm2;
>      }
>  
>      return 0;
> diff --git a/xen/arch/x86/x86_emulate/util.c b/xen/arch/x86/x86_emulate/util.c
> index 4becd054c2..34daa8467f 100644
> --- a/xen/arch/x86/x86_emulate/util.c
> +++ b/xen/arch/x86/x86_emulate/util.c
> @@ -8,12 +8,12 @@
>  
>  #include "private.h"
>  
> -unsigned int x86_insn_length(const struct x86_emulate_state *s,
> +unsigned int x86_insn_length(const struct x86_emulate_state *state,
>                               const struct x86_emulate_ctxt *ctxt)
>  {
> -    check_state(s);
> +    check_state(state);
>  
> -    return s->ip - ctxt->regs->r(ip);
> +    return state->ip - ctxt->regs->r(ip);
>  }
>  
>  /*
> @@ -22,13 +22,13 @@ unsigned int x86_insn_length(const struct x86_emulate_state *s,
>   * memory operand (like POP), but it does not mean e.g. segment selector
>   * loads, where the descriptor table access is considered an implicit one.
>   */
> -bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *s,
> +bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *state,
>                                       const struct x86_emulate_ctxt *ctxt)
>  {
> -    if ( mode_64bit() && s->not_64bit )
> +    if ( mode_64bit() && state->not_64bit )
>          return false;
>  
> -    if ( s->ea.type == OP_MEM )
> +    if ( state->ea.type == OP_MEM )
>      {
>          switch ( ctxt->opcode )
>          {
> @@ -49,13 +49,13 @@ bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *s,
>              return false;
>  
>          case X86EMUL_OPC(0x0f, 0x01):
> -            return (s->modrm_reg & 7) != 7; /* INVLPG */
> +            return (state->modrm_reg & 7) != 7; /* INVLPG */
>  
>          case X86EMUL_OPC(0x0f, 0xae):
> -            return (s->modrm_reg & 7) != 7; /* CLFLUSH */
> +            return (state->modrm_reg & 7) != 7; /* CLFLUSH */
>  
>          case X86EMUL_OPC_66(0x0f, 0xae):
> -            return (s->modrm_reg & 7) < 6; /* CLWB, CLFLUSHOPT */
> +            return (state->modrm_reg & 7) < 6; /* CLWB, CLFLUSHOPT */
>          }
>  
>          return true;
> @@ -91,7 +91,7 @@ bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *s,
>          return true;
>  
>      case 0xff:
> -        switch ( s->modrm_reg & 7 )
> +        switch ( state->modrm_reg & 7 )
>          {
>          case 2: /* CALL (near, indirect) */
>          case 6: /* PUSH r/m */
> @@ -101,7 +101,7 @@ bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *s,
>  
>      case X86EMUL_OPC(0x0f, 0x01):
>          /* Cover CLZERO. */
> -        return (s->modrm_rm & 7) == 4 && (s->modrm_reg & 7) == 7;
> +        return (state->modrm_rm & 7) == 4 && (state->modrm_reg & 7) == 7;
>      }
>  
>      return false;
> @@ -114,17 +114,17 @@ bool cf_check x86_insn_is_mem_access(const struct x86_emulate_state *s,
>   * loads, where the (possible) descriptor table write is considered an
>   * implicit access.
>   */
> -bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
> +bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *state,
>                                      const struct x86_emulate_ctxt *ctxt)
>  {
> -    if ( mode_64bit() && s->not_64bit )
> +    if ( mode_64bit() && state->not_64bit )
>          return false;
>  
> -    switch ( s->desc & DstMask )
> +    switch ( state->desc & DstMask )
>      {
>      case DstMem:
>          /* The SrcMem check is to cover {,V}MASKMOV{Q,DQU}. */
> -        return s->modrm_mod != 3 || (s->desc & SrcMask) == SrcMem;
> +        return state->modrm_mod != 3 || (state->desc & SrcMask) == SrcMem;
>  
>      case DstBitBase:
>      case DstImplicit:
> @@ -147,13 +147,13 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
>               X86EMUL_OPC_EVEX_F3(0x0f38, 0x25): /* VPMOVS* */
>          case X86EMUL_OPC_EVEX_F3(0x0f38, 0x30) ...
>               X86EMUL_OPC_EVEX_F3(0x0f38, 0x35): /* VPMOV{D,Q,W}* */
> -            return s->modrm_mod != 3;
> +            return state->modrm_mod != 3;
>          }
>  
>          return false;
>      }
>  
> -    if ( s->modrm_mod == 3 )
> +    if ( state->modrm_mod == 3 )
>      {
>          switch ( ctxt->opcode )
>          {
> @@ -161,7 +161,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
>              break;
>  
>          case X86EMUL_OPC(0x0f, 0x01): /* CLZERO is the odd one. */
> -            return (s->modrm_rm & 7) == 4 && (s->modrm_reg & 7) == 7;
> +            return (state->modrm_rm & 7) == 4 && (state->modrm_reg & 7) == 7;
>  
>          default:
>              return false;
> @@ -192,7 +192,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
>          return true;
>  
>      case 0xd9:
> -        switch ( s->modrm_reg & 7 )
> +        switch ( state->modrm_reg & 7 )
>          {
>          case 2: /* FST m32fp */
>          case 3: /* FSTP m32fp */
> @@ -203,7 +203,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
>          break;
>  
>      case 0xdb:
> -        switch ( s->modrm_reg & 7 )
> +        switch ( state->modrm_reg & 7 )
>          {
>          case 1: /* FISTTP m32i */
>          case 2: /* FIST m32i */
> @@ -214,7 +214,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
>          break;
>  
>      case 0xdd:
> -        switch ( s->modrm_reg & 7 )
> +        switch ( state->modrm_reg & 7 )
>          {
>          case 1: /* FISTTP m64i */
>          case 2: /* FST m64fp */
> @@ -226,7 +226,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
>          break;
>  
>      case 0xdf:
> -        switch ( s->modrm_reg & 7 )
> +        switch ( state->modrm_reg & 7 )
>          {
>          case 1: /* FISTTP m16i */
>          case 2: /* FIST m16i */
> @@ -238,7 +238,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
>          break;
>  
>      case 0xff:
> -        switch ( s->modrm_reg & 7 )
> +        switch ( state->modrm_reg & 7 )
>          {
>          case 2: /* CALL (near, indirect) */
>          case 3: /* CALL (far, indirect) */
> @@ -248,7 +248,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
>          break;
>  
>      case X86EMUL_OPC(0x0f, 0x01):
> -        switch ( s->modrm_reg & 7 )
> +        switch ( state->modrm_reg & 7 )
>          {
>          case 0: /* SGDT */
>          case 1: /* SIDT */
> @@ -258,7 +258,7 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
>          break;
>  
>      case X86EMUL_OPC(0x0f, 0xae):
> -        switch ( s->modrm_reg & 7 )
> +        switch ( state->modrm_reg & 7 )
>          {
>          case 0: /* FXSAVE */
>          /* case 3: STMXCSR - handled above */
> @@ -269,10 +269,10 @@ bool cf_check x86_insn_is_mem_write(const struct x86_emulate_state *s,
>          break;
>  
>      case X86EMUL_OPC(0x0f, 0xba):
> -        return (s->modrm_reg & 7) > 4; /* BTS / BTR / BTC */
> +        return (state->modrm_reg & 7) > 4; /* BTS / BTR / BTC */
>  
>      case X86EMUL_OPC(0x0f, 0xc7):
> -        switch ( s->modrm_reg & 7 )
> +        switch ( state->modrm_reg & 7 )
>          {
>          case 1: /* CMPXCHG{8,16}B */
>          case 4: /* XSAVEC */
> -- 
> 2.34.1
> 

Re: [XEN PATCH 4/5] x86/x86_emulate: change parameter name from 's' to 'state'

[Jan Beulich]

On 29.06.2023 21:31, Stefano Stabellini wrote:
> On Thu, 29 Jun 2023, Federico Serafini wrote:
>> Change parameter name from 's' to 'state' in function definitions in
>> order to:
>> 1) keep consistency with the parameter names used in the corresponding
>>    declarations;
>> 2) keep consistency with parameter names used within x86_emulate.h;
>> 3) fix violations of MISRA C:2012 Rule 8.3.
>>
>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> 
> You could use x86emul: as tag in the title. I'll let Jan choose the tag
> he prefers.

x86emul: or x86/emul: is what we commonly use. That said, I don't like
this change. The files touched are pretty new, and it was deliberate
that I used s, not state, for the names. This is shorthand much like
(globally) we use v (instead of vcpu) and d (instead of domain).

Jan

Re: [XEN PATCH 4/5] x86/x86_emulate: change parameter name from 's' to 'state'

[Stefano Stabellini]

On Tue, 4 Jul 2023, Jan Beulich wrote:
> On 29.06.2023 21:31, Stefano Stabellini wrote:
> > On Thu, 29 Jun 2023, Federico Serafini wrote:
> >> Change parameter name from 's' to 'state' in function definitions in
> >> order to:
> >> 1) keep consistency with the parameter names used in the corresponding
> >>    declarations;
> >> 2) keep consistency with parameter names used within x86_emulate.h;
> >> 3) fix violations of MISRA C:2012 Rule 8.3.
> >>
> >> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> > 
> > You could use x86emul: as tag in the title. I'll let Jan choose the tag
> > he prefers.
> 
> x86emul: or x86/emul: is what we commonly use. That said, I don't like
> this change. The files touched are pretty new, and it was deliberate
> that I used s, not state, for the names. This is shorthand much like
> (globally) we use v (instead of vcpu) and d (instead of domain).

Are you suggesting that the functions changed in this patch should be
adapted in the other direction instead?  Meaning that the declaration is
changed to match the definition instead of the opposite?

If so, are you referring to all the functions changed in this patch? Or
only some?

I am asking so that Federico can know how to proceed exactly.

Re: [XEN PATCH 4/5] x86/x86_emulate: change parameter name from 's' to 'state'

[Jan Beulich]

On 06.07.2023 00:49, Stefano Stabellini wrote:
> On Tue, 4 Jul 2023, Jan Beulich wrote:
>> On 29.06.2023 21:31, Stefano Stabellini wrote:
>>> On Thu, 29 Jun 2023, Federico Serafini wrote:
>>>> Change parameter name from 's' to 'state' in function definitions in
>>>> order to:
>>>> 1) keep consistency with the parameter names used in the corresponding
>>>>    declarations;
>>>> 2) keep consistency with parameter names used within x86_emulate.h;
>>>> 3) fix violations of MISRA C:2012 Rule 8.3.
>>>>
>>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>>>
>>> You could use x86emul: as tag in the title. I'll let Jan choose the tag
>>> he prefers.
>>
>> x86emul: or x86/emul: is what we commonly use. That said, I don't like
>> this change. The files touched are pretty new, and it was deliberate
>> that I used s, not state, for the names. This is shorthand much like
>> (globally) we use v (instead of vcpu) and d (instead of domain).
> 
> Are you suggesting that the functions changed in this patch should be
> adapted in the other direction instead?  Meaning that the declaration is
> changed to match the definition instead of the opposite?
> 
> If so, are you referring to all the functions changed in this patch? Or
> only some?

All of the files touched here are ones which were recently introduced,
and which are deliberately the way they are. This "deliberately" really
goes as far as declarations and definitions disagreeing in names: For
the former, what matters are adjacent declarations in the header. For
the latter what matters is code readability. I'm sorry, I think the
Misra rule simply gets in the way of the original intentions here (and
I continue to disagree with there being any confusion from name
mismatches between declarations and definitions, the more that in the
case here it would be easy to avoid by simply omitting names in
declarations, but that is violating yet another rule I don't fully
agree with either, as voiced when discussing it).

My preferred course of action here would be to simply drop the patch.
The least bad adjustment, if one is absolutely necessary, would be to
change the declarations, but then in a way that all adjacent ones
remain consistent (which may in turn require some _other_ definitions
to change). The mid- to long-term goal certainly is to use "s" more
where "state" may be used right now.

Jan

Re: [XEN PATCH 4/5] x86/x86_emulate: change parameter name from 's' to 'state'

[Stefano Stabellini]

On Thu, 6 Jul 2023, Jan Beulich wrote:
> On 06.07.2023 00:49, Stefano Stabellini wrote:
> > On Tue, 4 Jul 2023, Jan Beulich wrote:
> >> On 29.06.2023 21:31, Stefano Stabellini wrote:
> >>> On Thu, 29 Jun 2023, Federico Serafini wrote:
> >>>> Change parameter name from 's' to 'state' in function definitions in
> >>>> order to:
> >>>> 1) keep consistency with the parameter names used in the corresponding
> >>>>    declarations;
> >>>> 2) keep consistency with parameter names used within x86_emulate.h;
> >>>> 3) fix violations of MISRA C:2012 Rule 8.3.
> >>>>
> >>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> >>>
> >>> You could use x86emul: as tag in the title. I'll let Jan choose the tag
> >>> he prefers.
> >>
> >> x86emul: or x86/emul: is what we commonly use. That said, I don't like
> >> this change. The files touched are pretty new, and it was deliberate
> >> that I used s, not state, for the names. This is shorthand much like
> >> (globally) we use v (instead of vcpu) and d (instead of domain).
> > 
> > Are you suggesting that the functions changed in this patch should be
> > adapted in the other direction instead?  Meaning that the declaration is
> > changed to match the definition instead of the opposite?
> > 
> > If so, are you referring to all the functions changed in this patch? Or
> > only some?
> 
> All of the files touched here are ones which were recently introduced,
> and which are deliberately the way they are. This "deliberately" really
> goes as far as declarations and definitions disagreeing in names: For
> the former, what matters are adjacent declarations in the header. For
> the latter what matters is code readability. I'm sorry, I think the
> Misra rule simply gets in the way of the original intentions here (and
> I continue to disagree with there being any confusion from name
> mismatches between declarations and definitions, the more that in the
> case here it would be easy to avoid by simply omitting names in
> declarations, but that is violating yet another rule I don't fully
> agree with either, as voiced when discussing it).
> 
> My preferred course of action here would be to simply drop the patch.
> The least bad adjustment, if one is absolutely necessary, would be to
> change the declarations, but then in a way that all adjacent ones
> remain consistent (which may in turn require some _other_ definitions
> to change). The mid- to long-term goal certainly is to use "s" more
> where "state" may be used right now.


If we drop this patch then we have the problem that Eclair and other
scanners will detect these as violations. Also this source file would
not be consistent with the rest of the codebase causing issues in the
future. Any patch updating this file would trigger new MISRA C
violations in the scanners.

So I don't think we can drop this patch but we could add deviations. I
think your suggestion of "changing the declaration in a way that all
adjacent ones remain consistent" is better, but let me also provide this
option.

Basically, we would keep these declarations/definitions as is, and add
a one-line in-code comment for each deviation. Such as:

/* SAF-2-safe R8.3 */
bool cf_check
x86_insn_is_mem_write(const struct x86_emulate_state *state,
                      const struct x86_emulate_ctxt *ctxt);

Your suggestion of changing the declaration is better in my opinion. Do
you agree?

Re: [XEN PATCH 4/5] x86/x86_emulate: change parameter name from 's' to 'state'

[Jan Beulich]

On 07.07.2023 00:05, Stefano Stabellini wrote:
> On Thu, 6 Jul 2023, Jan Beulich wrote:
>> On 06.07.2023 00:49, Stefano Stabellini wrote:
>>> On Tue, 4 Jul 2023, Jan Beulich wrote:
>>>> On 29.06.2023 21:31, Stefano Stabellini wrote:
>>>>> On Thu, 29 Jun 2023, Federico Serafini wrote:
>>>>>> Change parameter name from 's' to 'state' in function definitions in
>>>>>> order to:
>>>>>> 1) keep consistency with the parameter names used in the corresponding
>>>>>>    declarations;
>>>>>> 2) keep consistency with parameter names used within x86_emulate.h;
>>>>>> 3) fix violations of MISRA C:2012 Rule 8.3.
>>>>>>
>>>>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>>>>>
>>>>> You could use x86emul: as tag in the title. I'll let Jan choose the tag
>>>>> he prefers.
>>>>
>>>> x86emul: or x86/emul: is what we commonly use. That said, I don't like
>>>> this change. The files touched are pretty new, and it was deliberate
>>>> that I used s, not state, for the names. This is shorthand much like
>>>> (globally) we use v (instead of vcpu) and d (instead of domain).
>>>
>>> Are you suggesting that the functions changed in this patch should be
>>> adapted in the other direction instead?  Meaning that the declaration is
>>> changed to match the definition instead of the opposite?
>>>
>>> If so, are you referring to all the functions changed in this patch? Or
>>> only some?
>>
>> All of the files touched here are ones which were recently introduced,
>> and which are deliberately the way they are. This "deliberately" really
>> goes as far as declarations and definitions disagreeing in names: For
>> the former, what matters are adjacent declarations in the header. For
>> the latter what matters is code readability. I'm sorry, I think the
>> Misra rule simply gets in the way of the original intentions here (and
>> I continue to disagree with there being any confusion from name
>> mismatches between declarations and definitions, the more that in the
>> case here it would be easy to avoid by simply omitting names in
>> declarations, but that is violating yet another rule I don't fully
>> agree with either, as voiced when discussing it).
>>
>> My preferred course of action here would be to simply drop the patch.
>> The least bad adjustment, if one is absolutely necessary, would be to
>> change the declarations, but then in a way that all adjacent ones
>> remain consistent (which may in turn require some _other_ definitions
>> to change). The mid- to long-term goal certainly is to use "s" more
>> where "state" may be used right now.
> 
> 
> If we drop this patch then we have the problem that Eclair and other
> scanners will detect these as violations. Also this source file would
> not be consistent with the rest of the codebase causing issues in the
> future. Any patch updating this file would trigger new MISRA C
> violations in the scanners.
> 
> So I don't think we can drop this patch but we could add deviations. I
> think your suggestion of "changing the declaration in a way that all
> adjacent ones remain consistent" is better, but let me also provide this
> option.
> 
> Basically, we would keep these declarations/definitions as is, and add
> a one-line in-code comment for each deviation. Such as:
> 
> /* SAF-2-safe R8.3 */
> bool cf_check
> x86_insn_is_mem_write(const struct x86_emulate_state *state,
>                       const struct x86_emulate_ctxt *ctxt);
> 
> Your suggestion of changing the declaration is better in my opinion. Do
> you agree?

Yes. I'm not really happy with any of the options resulting from us following
the various involved rules (also in particular 8.2), but this looks to be the
least bad of them.

Jan

[XEN PATCH 5/5] x86: make parameter names of function declarations consistent

[Federico Serafini]

Change the parameter names of function declarations to be consistent
with the names used in the corresponding function definitions
so as to fix violations of MISRA C:2012 Rule 8.3.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/cpu/mcheck/mce.h           |  2 +-
 xen/arch/x86/cpu/mcheck/x86_mca.h       |  2 +-
 xen/arch/x86/hvm/rtc.c                  |  2 +-
 xen/arch/x86/hvm/svm/nestedhvm.h        |  2 +-
 xen/arch/x86/hvm/vioapic.c              |  2 +-
 xen/arch/x86/include/asm/genapic.h      |  2 +-
 xen/arch/x86/include/asm/guest_pt.h     |  2 +-
 xen/arch/x86/include/asm/hap.h          |  2 +-
 xen/arch/x86/include/asm/hvm/io.h       |  2 +-
 xen/arch/x86/include/asm/hvm/monitor.h  |  2 +-
 xen/arch/x86/include/asm/hvm/svm/vmcb.h |  2 +-
 xen/arch/x86/include/asm/hvm/vmx/vmcs.h |  4 ++--
 xen/arch/x86/include/asm/hvm/vmx/vvmx.h |  8 +++----
 xen/arch/x86/include/asm/io_apic.h      |  2 +-
 xen/arch/x86/include/asm/irq.h          |  6 ++---
 xen/arch/x86/include/asm/mem_access.h   |  2 +-
 xen/arch/x86/include/asm/mpspec.h       |  2 +-
 xen/arch/x86/include/asm/msi.h          |  4 ++--
 xen/arch/x86/include/asm/p2m.h          |  8 +++----
 xen/arch/x86/include/asm/paging.h       |  2 +-
 xen/arch/x86/include/asm/psr.h          |  2 +-
 xen/arch/x86/include/asm/setup.h        |  2 +-
 xen/arch/x86/include/asm/uaccess.h      |  6 ++---
 xen/arch/x86/include/asm/xstate.h       |  2 +-
 xen/include/xen/lib/x86/cpu-policy.h    | 29 +++++++++++++------------
 25 files changed, 51 insertions(+), 50 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
index bea08bdc74..72d8dbc471 100644
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -44,7 +44,7 @@ extern uint8_t cmci_apic_vector;
 extern bool lmce_support;
 
 /* Init functions */
-enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *c);
+enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *ci);
 enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool bsp);
 
 void amd_nonfatal_mcheck_init(struct cpuinfo_x86 *c);
diff --git a/xen/arch/x86/cpu/mcheck/x86_mca.h b/xen/arch/x86/cpu/mcheck/x86_mca.h
index 18116737af..36b6127a37 100644
--- a/xen/arch/x86/cpu/mcheck/x86_mca.h
+++ b/xen/arch/x86/cpu/mcheck/x86_mca.h
@@ -113,7 +113,7 @@ static inline int mcabanks_test(int bit, struct mca_banks* banks)
     return test_bit(bit, banks->bank_map);
 }
 
-struct mca_banks *mcabanks_alloc(unsigned int nr);
+struct mca_banks *mcabanks_alloc(unsigned int nr_mce_banks);
 void mcabanks_free(struct mca_banks *banks);
 extern struct mca_banks *mca_allbanks;
 
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index c1ab6c7d58..ebb259586a 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -59,7 +59,7 @@ enum rtc_mode {
 static void rtc_copy_date(RTCState *s);
 static void rtc_set_time(RTCState *s);
 static inline int from_bcd(RTCState *s, int a);
-static inline int convert_hour(RTCState *s, int hour);
+static inline int convert_hour(RTCState *s, int raw);
 
 static void rtc_update_irq(RTCState *s)
 {
diff --git a/xen/arch/x86/hvm/svm/nestedhvm.h b/xen/arch/x86/hvm/svm/nestedhvm.h
index 43245e13de..eb9c416307 100644
--- a/xen/arch/x86/hvm/svm/nestedhvm.h
+++ b/xen/arch/x86/hvm/svm/nestedhvm.h
@@ -42,7 +42,7 @@ int cf_check nsvm_vcpu_initialise(struct vcpu *v);
 int cf_check nsvm_vcpu_reset(struct vcpu *v);
 int nsvm_vcpu_vmrun(struct vcpu *v, struct cpu_user_regs *regs);
 int cf_check nsvm_vcpu_vmexit_event(struct vcpu *v,
-                                    const struct x86_event *event);
+                                    const struct x86_event *trap);
 uint64_t cf_check nsvm_vcpu_hostcr3(struct vcpu *v);
 bool cf_check nsvm_vmcb_guest_intercepts_event(
     struct vcpu *v, unsigned int vector, int errcode);
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 41e3c4d5e4..4e40d3609a 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -43,7 +43,7 @@
 /* HACK: Route IRQ0 only to VCPU0 to prevent time jumps. */
 #define IRQ0_SPECIAL_ROUTING 1
 
-static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int irq);
+static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin);
 
 static struct hvm_vioapic *addr_vioapic(const struct domain *d,
                                         unsigned long addr)
diff --git a/xen/arch/x86/include/asm/genapic.h b/xen/arch/x86/include/asm/genapic.h
index beeaddf19d..970df8ffe0 100644
--- a/xen/arch/x86/include/asm/genapic.h
+++ b/xen/arch/x86/include/asm/genapic.h
@@ -43,7 +43,7 @@ void cf_check send_IPI_self_legacy(uint8_t vector);
 
 void cf_check init_apic_ldr_flat(void);
 unsigned int cf_check cpu_mask_to_apicid_flat(const cpumask_t *cpumask);
-void cf_check send_IPI_mask_flat(const cpumask_t *mask, int vector);
+void cf_check send_IPI_mask_flat(const cpumask_t *cpumask, int vector);
 const cpumask_t *cf_check vector_allocation_cpumask_flat(int cpu);
 #define GENAPIC_FLAT \
 	.int_delivery_mode = dest_LowestPrio, \
diff --git a/xen/arch/x86/include/asm/guest_pt.h b/xen/arch/x86/include/asm/guest_pt.h
index bde7588342..f616357107 100644
--- a/xen/arch/x86/include/asm/guest_pt.h
+++ b/xen/arch/x86/include/asm/guest_pt.h
@@ -422,7 +422,7 @@ static inline unsigned int guest_walk_to_page_order(const walk_t *gw)
 
 bool
 guest_walk_tables(const struct vcpu *v, struct p2m_domain *p2m,
-                  unsigned long va, walk_t *gw, uint32_t pfec,
+                  unsigned long va, walk_t *gw, uint32_t walk,
                   gfn_t top_gfn, mfn_t top_mfn, void *top_map);
 
 /* Pretty-print the contents of a guest-walk */
diff --git a/xen/arch/x86/include/asm/hap.h b/xen/arch/x86/include/asm/hap.h
index 9d12327b12..05e124ad57 100644
--- a/xen/arch/x86/include/asm/hap.h
+++ b/xen/arch/x86/include/asm/hap.h
@@ -30,7 +30,7 @@ void  hap_vcpu_init(struct vcpu *v);
 int   hap_track_dirty_vram(struct domain *d,
                            unsigned long begin_pfn,
                            unsigned int nr_frames,
-                           XEN_GUEST_HANDLE(void) dirty_bitmap);
+                           XEN_GUEST_HANDLE(void) guest_dirty_bitmap);
 
 extern const struct paging_mode *hap_paging_get_mode(struct vcpu *);
 int hap_set_allocation(struct domain *d, unsigned int pages, bool *preempted);
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 8df33eb6cc..cad082f020 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -90,7 +90,7 @@ bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
                                   struct npfec);
 bool handle_pio(uint16_t port, unsigned int size, int dir);
 void hvm_interrupt_post(struct vcpu *v, int vector, int type);
-void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq);
+void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi);
 void msix_write_completion(struct vcpu *);
 
 #ifdef CONFIG_HVM
diff --git a/xen/arch/x86/include/asm/hvm/monitor.h b/xen/arch/x86/include/asm/hvm/monitor.h
index 5276b0af08..02021be47b 100644
--- a/xen/arch/x86/include/asm/hvm/monitor.h
+++ b/xen/arch/x86/include/asm/hvm/monitor.h
@@ -25,7 +25,7 @@ bool hvm_monitor_cr(unsigned int index, unsigned long value,
                     unsigned long old);
 #define hvm_monitor_crX(cr, new, old) \
                         hvm_monitor_cr(VM_EVENT_X86_##cr, new, old)
-bool hvm_monitor_msr(unsigned int msr, uint64_t value, uint64_t old_value);
+bool hvm_monitor_msr(unsigned int msr, uint64_t new_value, uint64_t old_value);
 void hvm_monitor_descriptor_access(uint64_t exit_info,
                                    uint64_t vmx_exit_qualification,
                                    uint8_t descriptor, bool is_write);
diff --git a/xen/arch/x86/include/asm/hvm/svm/vmcb.h b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
index a1a8a7fd25..91221ff4e2 100644
--- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
+++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
@@ -607,7 +607,7 @@ void setup_vmcb_dump(void);
 #define MSR_INTERCEPT_READ    1
 #define MSR_INTERCEPT_WRITE   2
 #define MSR_INTERCEPT_RW      (MSR_INTERCEPT_WRITE | MSR_INTERCEPT_READ)
-void svm_intercept_msr(struct vcpu *v, uint32_t msr, int enable);
+void svm_intercept_msr(struct vcpu *v, uint32_t msr, int flags);
 #define svm_disable_intercept_for_msr(v, msr) svm_intercept_msr((v), (msr), MSR_INTERCEPT_NONE)
 #define svm_enable_intercept_for_msr(v, msr) svm_intercept_msr((v), (msr), MSR_INTERCEPT_RW)
 
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
index d07fcb2bc9..24bf409d8f 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -656,10 +656,10 @@ bool vmx_msr_is_intercepted(struct vmx_msr_bitmap *msr_bitmap,
                             unsigned int msr, bool is_write) __nonnull(1);
 void virtual_vmcs_enter(const struct vcpu *);
 void virtual_vmcs_exit(const struct vcpu *);
-u64 virtual_vmcs_vmread(const struct vcpu *, u32 encoding);
+u64 virtual_vmcs_vmread(const struct vcpu *, u32 vmcs_encoding);
 enum vmx_insn_errno virtual_vmcs_vmread_safe(const struct vcpu *v,
                                              u32 vmcs_encoding, u64 *val);
-void virtual_vmcs_vmwrite(const struct vcpu *, u32 encoding, u64 val);
+void virtual_vmcs_vmwrite(const struct vcpu *, u32 vmcs_encoding, u64 val);
 enum vmx_insn_errno virtual_vmcs_vmwrite_safe(const struct vcpu *v,
                                               u32 vmcs_encoding, u64 val);
 
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vvmx.h b/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
index dc9db69258..1e4bbc0d78 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
@@ -144,9 +144,9 @@ enum vvmcs_encoding_type {
     VVMCS_TYPE_HSTATE,
 };
 
-u64 get_vvmcs_virtual(void *vvmcs, u32 encoding);
+u64 get_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding);
 u64 get_vvmcs_real(const struct vcpu *, u32 encoding);
-void set_vvmcs_virtual(void *vvmcs, u32 encoding, u64 val);
+void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val);
 void set_vvmcs_real(const struct vcpu *, u32 encoding, u64 val);
 enum vmx_insn_errno get_vvmcs_virtual_safe(void *vvmcs, u32 encoding, u64 *val);
 enum vmx_insn_errno get_vvmcs_real_safe(const struct vcpu *, u32 encoding,
@@ -180,9 +180,9 @@ int nvmx_handle_vmx_insn(struct cpu_user_regs *regs, unsigned int exit_reason);
 int nvmx_msr_read_intercept(unsigned int msr,
                                 u64 *msr_content);
 
-void nvmx_update_exec_control(struct vcpu *v, u32 value);
+void nvmx_update_exec_control(struct vcpu *v, u32 host_cntrl);
 void nvmx_update_secondary_exec_control(struct vcpu *v,
-                                        unsigned long value);
+                                        unsigned long host_cntrl);
 void nvmx_update_exception_bitmap(struct vcpu *v, unsigned long value);
 void nvmx_switch_guest(void);
 void nvmx_idtv_handling(void);
diff --git a/xen/arch/x86/include/asm/io_apic.h b/xen/arch/x86/include/asm/io_apic.h
index ef0878b09e..9b22448ef0 100644
--- a/xen/arch/x86/include/asm/io_apic.h
+++ b/xen/arch/x86/include/asm/io_apic.h
@@ -207,6 +207,6 @@ extern int restore_IO_APIC_setup(struct IO_APIC_route_entry **ioapic_entries,
 unsigned highest_gsi(void);
 
 int ioapic_guest_read( unsigned long physbase, unsigned int reg, u32 *pval);
-int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 pval);
+int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val);
 
 #endif
diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index 424b0e1af8..f49388a47f 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -112,7 +112,7 @@ void cf_check enable_8259A_irq(struct irq_desc *);
 int i8259A_irq_pending(unsigned int irq);
 void mask_8259A(void);
 void unmask_8259A(void);
-void init_8259A(int aeoi);
+void init_8259A(int auto_eoi);
 void make_8259A_irq(unsigned int irq);
 bool bogus_8259A_irq(unsigned int irq);
 int i8259A_suspend(void);
@@ -141,7 +141,7 @@ struct arch_pirq {
 #define pirq_dpci(pirq) ((pirq) ? &(pirq)->arch.hvm.dpci : NULL)
 #define dpci_pirq(pd) container_of(pd, struct pirq, arch.hvm.dpci)
 
-int pirq_shared(struct domain *d , int irq);
+int pirq_shared(struct domain *d , int pirq);
 
 int map_domain_pirq(struct domain *d, int pirq, int irq, int type,
                            void *data);
@@ -149,7 +149,7 @@ int unmap_domain_pirq(struct domain *d, int pirq);
 int get_free_pirq(struct domain *d, int type);
 int get_free_pirqs(struct domain *, unsigned int nr);
 void free_domain_pirqs(struct domain *d);
-int map_domain_emuirq_pirq(struct domain *d, int pirq, int irq);
+int map_domain_emuirq_pirq(struct domain *d, int pirq, int emuirq);
 int unmap_domain_pirq_emuirq(struct domain *d, int pirq);
 
 /* Reset irq affinities to match the given CPU mask. */
diff --git a/xen/arch/x86/include/asm/mem_access.h b/xen/arch/x86/include/asm/mem_access.h
index 8957e1181c..1a52a10322 100644
--- a/xen/arch/x86/include/asm/mem_access.h
+++ b/xen/arch/x86/include/asm/mem_access.h
@@ -39,7 +39,7 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
 
 struct xen_hvm_altp2m_suppress_ve_multi;
 int p2m_set_suppress_ve_multi(struct domain *d,
-                              struct xen_hvm_altp2m_suppress_ve_multi *suppress_ve);
+                              struct xen_hvm_altp2m_suppress_ve_multi *sve);
 
 int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
                         unsigned int altp2m_idx);
diff --git a/xen/arch/x86/include/asm/mpspec.h b/xen/arch/x86/include/asm/mpspec.h
index 1246eece0b..cf8b360259 100644
--- a/xen/arch/x86/include/asm/mpspec.h
+++ b/xen/arch/x86/include/asm/mpspec.h
@@ -26,7 +26,7 @@ extern void mp_register_lapic_address (u64 address);
 extern void mp_register_ioapic (u8 id, u32 address, u32 gsi_base);
 extern void mp_override_legacy_irq (u8 bus_irq, u8 polarity, u8 trigger, u32 gsi);
 extern void mp_config_acpi_legacy_irqs (void);
-extern int mp_register_gsi (u32 gsi, int edge_level, int active_high_low);
+extern int mp_register_gsi (u32 gsi, int triggering, int polarity);
 #endif /* CONFIG_ACPI */
 
 #define PHYSID_ARRAY_SIZE	BITS_TO_LONGS(MAX_APICS)
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index a53ade95c9..687df5942a 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -82,7 +82,7 @@ struct hw_interrupt_type;
 struct msi_desc;
 /* Helper functions */
 extern int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc);
-extern void pci_disable_msi(struct msi_desc *desc);
+extern void pci_disable_msi(struct msi_desc *msi_desc);
 extern int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool off);
 extern void pci_cleanup_msi(struct pci_dev *pdev);
 extern int setup_msi_irq(struct irq_desc *, struct msi_desc *);
@@ -220,7 +220,7 @@ struct arch_msix {
 };
 
 void early_msi_init(void);
-void msi_compose_msg(unsigned vector, const cpumask_t *mask,
+void msi_compose_msg(unsigned vector, const cpumask_t *cpu_mask,
                      struct msi_msg *msg);
 void __msi_set_enable(u16 seg, u8 bus, u8 slot, u8 func, int pos, int enable);
 void cf_check mask_msi_irq(struct irq_desc *);
diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h
index 40545f5fa8..67bd3201bc 100644
--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -632,7 +632,7 @@ void p2m_change_type_range(struct domain *d,
                            p2m_type_t ot, p2m_type_t nt);
 
 /* Compare-exchange the type of a single p2m entry */
-int p2m_change_type_one(struct domain *d, unsigned long gfn,
+int p2m_change_type_one(struct domain *d, unsigned long gfn_l,
                         p2m_type_t ot, p2m_type_t nt);
 
 /* Synchronously change the p2m type for a range of gfns */
@@ -661,9 +661,9 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
                            p2m_access_t p2ma, unsigned int flag);
 int clear_identity_p2m_entry(struct domain *d, unsigned long gfn);
 /* HVM-only callers can use these directly: */
-int p2m_add_identity_entry(struct domain *d, unsigned long gfn,
+int p2m_add_identity_entry(struct domain *d, unsigned long gfn_l,
                            p2m_access_t p2ma, unsigned int flag);
-int p2m_remove_identity_entry(struct domain *d, unsigned long gfn);
+int p2m_remove_identity_entry(struct domain *d, unsigned long gfn_l);
 
 /* 
  * Populate-on-demand
@@ -924,7 +924,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
                                 p2m_type_t p2mt, p2m_access_t p2ma);
 
 /* Set a specific p2m view visibility */
-int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int idx,
+int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
                                    uint8_t visible);
 #else /* !CONFIG_HVM */
 struct p2m_domain *p2m_get_altp2m(struct vcpu *v);
diff --git a/xen/arch/x86/include/asm/paging.h b/xen/arch/x86/include/asm/paging.h
index 403243bfbd..a72a8d63ce 100644
--- a/xen/arch/x86/include/asm/paging.h
+++ b/xen/arch/x86/include/asm/paging.h
@@ -246,7 +246,7 @@ paging_fault(unsigned long va, struct cpu_user_regs *regs)
 }
 
 /* Handle invlpg requests on vcpus. */
-void paging_invlpg(struct vcpu *v, unsigned long va);
+void paging_invlpg(struct vcpu *v, unsigned long linear);
 
 /*
  * Translate a guest virtual address to the frame number that the
diff --git a/xen/arch/x86/include/asm/psr.h b/xen/arch/x86/include/asm/psr.h
index c2257da7fc..8ecb7a0eea 100644
--- a/xen/arch/x86/include/asm/psr.h
+++ b/xen/arch/x86/include/asm/psr.h
@@ -81,7 +81,7 @@ int psr_get_info(unsigned int socket, enum psr_type type,
 int psr_get_val(struct domain *d, unsigned int socket,
                 uint32_t *val, enum psr_type type);
 int psr_set_val(struct domain *d, unsigned int socket,
-                uint64_t val, enum psr_type type);
+                uint64_t new_val, enum psr_type type);
 
 void psr_domain_init(struct domain *d);
 void psr_domain_free(struct domain *d);
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index ae0dd3915a..29ccacfd7f 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,
     char *cmdline);
 void setup_io_bitmap(struct domain *d);
diff --git a/xen/arch/x86/include/asm/uaccess.h b/xen/arch/x86/include/asm/uaccess.h
index 684fccd95c..7443519d5b 100644
--- a/xen/arch/x86/include/asm/uaccess.h
+++ b/xen/arch/x86/include/asm/uaccess.h
@@ -10,10 +10,10 @@
 #include <asm/x86_64/uaccess.h>
 
 unsigned int copy_to_guest_pv(void __user *to, const void *from,
-                              unsigned int len);
-unsigned int clear_guest_pv(void __user *to, unsigned int len);
+                              unsigned int n);
+unsigned int clear_guest_pv(void __user *to, unsigned int n);
 unsigned int copy_from_guest_pv(void *to, const void __user *from,
-                                unsigned int len);
+                                unsigned int n);
 
 /* Handles exceptions in both to and from, but doesn't do access_ok */
 unsigned int copy_to_guest_ll(void __user*to, const void *from, unsigned int n);
diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index 7ab0bdde89..4fb4312192 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -87,7 +87,7 @@ struct xstate_bndcsr {
 };
 
 /* extended state operations */
-bool __must_check set_xcr0(u64 xfeatures);
+bool __must_check set_xcr0(u64 val);
 uint64_t get_xcr0(void);
 void set_msr_xss(u64 xss);
 uint64_t get_msr_xss(void);
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index 6d5e9edd26..bab3eecda6 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -451,23 +451,24 @@ typedef xen_msr_entry_t msr_entry_buffer_t[];
  * Serialise the CPUID leaves of a cpu_policy object into an array of cpuid
  * leaves.
  *
- * @param policy     The cpu_policy to serialise.
- * @param leaves     The array of leaves to serialise into.
- * @param nr_entries The number of entries in 'leaves'.
+ * @param p            The cpu_policy to serialise.
+ * @param leaves       The array of leaves to serialise into.
+ * @param nr_entries_p The number of entries in 'leaves'.
  * @returns -errno
  *
  * Writes at most CPUID_MAX_SERIALISED_LEAVES.  May fail with -ENOBUFS if the
  * leaves array is too short.  On success, nr_entries is updated with the
  * actual number of leaves written.
  */
-int x86_cpuid_copy_to_buffer(const struct cpu_policy *policy,
-                             cpuid_leaf_buffer_t leaves, uint32_t *nr_entries);
+int x86_cpuid_copy_to_buffer(const struct cpu_policy *p,
+                             cpuid_leaf_buffer_t leaves,
+                             uint32_t *nr_entries_p);
 
 /**
  * Unserialise the CPUID leaves of a cpu_policy object into an array of cpuid
  * leaves.
  *
- * @param policy      The cpu_policy to unserialise into.
+ * @param p           The cpu_policy to unserialise into.
  * @param leaves      The array of leaves to unserialise from.
  * @param nr_entries  The number of entries in 'leaves'.
  * @param err_leaf    Optional hint for error diagnostics.
@@ -481,7 +482,7 @@ int x86_cpuid_copy_to_buffer(const struct cpu_policy *policy,
  * No content validation of in-range leaves is performed.  Synthesised data is
  * recalculated.
  */
-int x86_cpuid_copy_from_buffer(struct cpu_policy *policy,
+int x86_cpuid_copy_from_buffer(struct cpu_policy *p,
                                const cpuid_leaf_buffer_t leaves,
                                uint32_t nr_entries, uint32_t *err_leaf,
                                uint32_t *err_subleaf);
@@ -489,22 +490,22 @@ int x86_cpuid_copy_from_buffer(struct cpu_policy *policy,
 /**
  * Serialise the MSRs of a cpu_policy object into an array.
  *
- * @param policy     The cpu_policy to serialise.
- * @param msrs       The array of msrs to serialise into.
- * @param nr_entries The number of entries in 'msrs'.
+ * @param p            The cpu_policy to serialise.
+ * @param msrs         The array of msrs to serialise into.
+ * @param nr_entries_p The number of entries in 'msrs'.
  * @returns -errno
  *
  * Writes at most MSR_MAX_SERIALISED_ENTRIES.  May fail with -ENOBUFS if the
  * buffer array is too short.  On success, nr_entries is updated with the
  * actual number of msrs written.
  */
-int x86_msr_copy_to_buffer(const struct cpu_policy *policy,
-                           msr_entry_buffer_t msrs, uint32_t *nr_entries);
+int x86_msr_copy_to_buffer(const struct cpu_policy *p,
+                           msr_entry_buffer_t msrs, uint32_t *nr_entries_p);
 
 /**
  * Unserialise the MSRs of a cpu_policy object from an array of msrs.
  *
- * @param policy     The cpu_policy object to unserialise into.
+ * @param p          The cpu_policy object to unserialise into.
  * @param msrs       The array of msrs to unserialise from.
  * @param nr_entries The number of entries in 'msrs'.
  * @param err_msr    Optional hint for error diagnostics.
@@ -518,7 +519,7 @@ int x86_msr_copy_to_buffer(const struct cpu_policy *policy,
  *
  * No content validation is performed on the data stored in the policy object.
  */
-int x86_msr_copy_from_buffer(struct cpu_policy *policy,
+int x86_msr_copy_from_buffer(struct cpu_policy *p,
                              const msr_entry_buffer_t msrs, uint32_t nr_entries,
                              uint32_t *err_msr);
 
-- 
2.34.1

Re: [XEN PATCH 5/5] x86: make parameter names of function declarations consistent

[Stefano Stabellini]

On Thu, 29 Jun 2023, Federico Serafini wrote:
> Change the parameter names of function declarations to be consistent
> with the names used in the corresponding function definitions
> so as to fix violations of MISRA C:2012 Rule 8.3.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
>  xen/arch/x86/cpu/mcheck/mce.h           |  2 +-
>  xen/arch/x86/cpu/mcheck/x86_mca.h       |  2 +-
>  xen/arch/x86/hvm/rtc.c                  |  2 +-
>  xen/arch/x86/hvm/svm/nestedhvm.h        |  2 +-
>  xen/arch/x86/hvm/vioapic.c              |  2 +-
>  xen/arch/x86/include/asm/genapic.h      |  2 +-
>  xen/arch/x86/include/asm/guest_pt.h     |  2 +-
>  xen/arch/x86/include/asm/hap.h          |  2 +-
>  xen/arch/x86/include/asm/hvm/io.h       |  2 +-
>  xen/arch/x86/include/asm/hvm/monitor.h  |  2 +-
>  xen/arch/x86/include/asm/hvm/svm/vmcb.h |  2 +-
>  xen/arch/x86/include/asm/hvm/vmx/vmcs.h |  4 ++--
>  xen/arch/x86/include/asm/hvm/vmx/vvmx.h |  8 +++----
>  xen/arch/x86/include/asm/io_apic.h      |  2 +-
>  xen/arch/x86/include/asm/irq.h          |  6 ++---
>  xen/arch/x86/include/asm/mem_access.h   |  2 +-
>  xen/arch/x86/include/asm/mpspec.h       |  2 +-
>  xen/arch/x86/include/asm/msi.h          |  4 ++--
>  xen/arch/x86/include/asm/p2m.h          |  8 +++----
>  xen/arch/x86/include/asm/paging.h       |  2 +-
>  xen/arch/x86/include/asm/psr.h          |  2 +-
>  xen/arch/x86/include/asm/setup.h        |  2 +-
>  xen/arch/x86/include/asm/uaccess.h      |  6 ++---
>  xen/arch/x86/include/asm/xstate.h       |  2 +-
>  xen/include/xen/lib/x86/cpu-policy.h    | 29 +++++++++++++------------
>  25 files changed, 51 insertions(+), 50 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
> index bea08bdc74..72d8dbc471 100644
> --- a/xen/arch/x86/cpu/mcheck/mce.h
> +++ b/xen/arch/x86/cpu/mcheck/mce.h
> @@ -44,7 +44,7 @@ extern uint8_t cmci_apic_vector;
>  extern bool lmce_support;
>  
>  /* Init functions */
> -enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *c);
> +enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *ci);
>  enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool bsp);
>  
>  void amd_nonfatal_mcheck_init(struct cpuinfo_x86 *c);
> diff --git a/xen/arch/x86/cpu/mcheck/x86_mca.h b/xen/arch/x86/cpu/mcheck/x86_mca.h
> index 18116737af..36b6127a37 100644
> --- a/xen/arch/x86/cpu/mcheck/x86_mca.h
> +++ b/xen/arch/x86/cpu/mcheck/x86_mca.h
> @@ -113,7 +113,7 @@ static inline int mcabanks_test(int bit, struct mca_banks* banks)
>      return test_bit(bit, banks->bank_map);
>  }
>  
> -struct mca_banks *mcabanks_alloc(unsigned int nr);
> +struct mca_banks *mcabanks_alloc(unsigned int nr_mce_banks);
>  void mcabanks_free(struct mca_banks *banks);
>  extern struct mca_banks *mca_allbanks;
>  
> diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
> index c1ab6c7d58..ebb259586a 100644
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -59,7 +59,7 @@ enum rtc_mode {
>  static void rtc_copy_date(RTCState *s);
>  static void rtc_set_time(RTCState *s);
>  static inline int from_bcd(RTCState *s, int a);
> -static inline int convert_hour(RTCState *s, int hour);
> +static inline int convert_hour(RTCState *s, int raw);
>  
>  static void rtc_update_irq(RTCState *s)
>  {
> diff --git a/xen/arch/x86/hvm/svm/nestedhvm.h b/xen/arch/x86/hvm/svm/nestedhvm.h
> index 43245e13de..eb9c416307 100644
> --- a/xen/arch/x86/hvm/svm/nestedhvm.h
> +++ b/xen/arch/x86/hvm/svm/nestedhvm.h
> @@ -42,7 +42,7 @@ int cf_check nsvm_vcpu_initialise(struct vcpu *v);
>  int cf_check nsvm_vcpu_reset(struct vcpu *v);
>  int nsvm_vcpu_vmrun(struct vcpu *v, struct cpu_user_regs *regs);
>  int cf_check nsvm_vcpu_vmexit_event(struct vcpu *v,
> -                                    const struct x86_event *event);
> +                                    const struct x86_event *trap);
>  uint64_t cf_check nsvm_vcpu_hostcr3(struct vcpu *v);
>  bool cf_check nsvm_vmcb_guest_intercepts_event(
>      struct vcpu *v, unsigned int vector, int errcode);
> diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
> index 41e3c4d5e4..4e40d3609a 100644
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -43,7 +43,7 @@
>  /* HACK: Route IRQ0 only to VCPU0 to prevent time jumps. */
>  #define IRQ0_SPECIAL_ROUTING 1
>  
> -static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int irq);
> +static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin);
>  
>  static struct hvm_vioapic *addr_vioapic(const struct domain *d,
>                                          unsigned long addr)
> diff --git a/xen/arch/x86/include/asm/genapic.h b/xen/arch/x86/include/asm/genapic.h
> index beeaddf19d..970df8ffe0 100644
> --- a/xen/arch/x86/include/asm/genapic.h
> +++ b/xen/arch/x86/include/asm/genapic.h
> @@ -43,7 +43,7 @@ void cf_check send_IPI_self_legacy(uint8_t vector);
>  
>  void cf_check init_apic_ldr_flat(void);
>  unsigned int cf_check cpu_mask_to_apicid_flat(const cpumask_t *cpumask);
> -void cf_check send_IPI_mask_flat(const cpumask_t *mask, int vector);
> +void cf_check send_IPI_mask_flat(const cpumask_t *cpumask, int vector);
>  const cpumask_t *cf_check vector_allocation_cpumask_flat(int cpu);
>  #define GENAPIC_FLAT \
>  	.int_delivery_mode = dest_LowestPrio, \
> diff --git a/xen/arch/x86/include/asm/guest_pt.h b/xen/arch/x86/include/asm/guest_pt.h
> index bde7588342..f616357107 100644
> --- a/xen/arch/x86/include/asm/guest_pt.h
> +++ b/xen/arch/x86/include/asm/guest_pt.h
> @@ -422,7 +422,7 @@ static inline unsigned int guest_walk_to_page_order(const walk_t *gw)
>  
>  bool
>  guest_walk_tables(const struct vcpu *v, struct p2m_domain *p2m,
> -                  unsigned long va, walk_t *gw, uint32_t pfec,
> +                  unsigned long va, walk_t *gw, uint32_t walk,
>                    gfn_t top_gfn, mfn_t top_mfn, void *top_map);
>  
>  /* Pretty-print the contents of a guest-walk */
> diff --git a/xen/arch/x86/include/asm/hap.h b/xen/arch/x86/include/asm/hap.h
> index 9d12327b12..05e124ad57 100644
> --- a/xen/arch/x86/include/asm/hap.h
> +++ b/xen/arch/x86/include/asm/hap.h
> @@ -30,7 +30,7 @@ void  hap_vcpu_init(struct vcpu *v);
>  int   hap_track_dirty_vram(struct domain *d,
>                             unsigned long begin_pfn,
>                             unsigned int nr_frames,
> -                           XEN_GUEST_HANDLE(void) dirty_bitmap);
> +                           XEN_GUEST_HANDLE(void) guest_dirty_bitmap);
>  
>  extern const struct paging_mode *hap_paging_get_mode(struct vcpu *);
>  int hap_set_allocation(struct domain *d, unsigned int pages, bool *preempted);
> diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
> index 8df33eb6cc..cad082f020 100644
> --- a/xen/arch/x86/include/asm/hvm/io.h
> +++ b/xen/arch/x86/include/asm/hvm/io.h
> @@ -90,7 +90,7 @@ bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
>                                    struct npfec);
>  bool handle_pio(uint16_t port, unsigned int size, int dir);
>  void hvm_interrupt_post(struct vcpu *v, int vector, int type);
> -void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq);
> +void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi);
>  void msix_write_completion(struct vcpu *);
>  
>  #ifdef CONFIG_HVM
> diff --git a/xen/arch/x86/include/asm/hvm/monitor.h b/xen/arch/x86/include/asm/hvm/monitor.h
> index 5276b0af08..02021be47b 100644
> --- a/xen/arch/x86/include/asm/hvm/monitor.h
> +++ b/xen/arch/x86/include/asm/hvm/monitor.h
> @@ -25,7 +25,7 @@ bool hvm_monitor_cr(unsigned int index, unsigned long value,
>                      unsigned long old);
>  #define hvm_monitor_crX(cr, new, old) \
>                          hvm_monitor_cr(VM_EVENT_X86_##cr, new, old)
> -bool hvm_monitor_msr(unsigned int msr, uint64_t value, uint64_t old_value);
> +bool hvm_monitor_msr(unsigned int msr, uint64_t new_value, uint64_t old_value);
>  void hvm_monitor_descriptor_access(uint64_t exit_info,
>                                     uint64_t vmx_exit_qualification,
>                                     uint8_t descriptor, bool is_write);
> diff --git a/xen/arch/x86/include/asm/hvm/svm/vmcb.h b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
> index a1a8a7fd25..91221ff4e2 100644
> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
> @@ -607,7 +607,7 @@ void setup_vmcb_dump(void);
>  #define MSR_INTERCEPT_READ    1
>  #define MSR_INTERCEPT_WRITE   2
>  #define MSR_INTERCEPT_RW      (MSR_INTERCEPT_WRITE | MSR_INTERCEPT_READ)
> -void svm_intercept_msr(struct vcpu *v, uint32_t msr, int enable);
> +void svm_intercept_msr(struct vcpu *v, uint32_t msr, int flags);
>  #define svm_disable_intercept_for_msr(v, msr) svm_intercept_msr((v), (msr), MSR_INTERCEPT_NONE)
>  #define svm_enable_intercept_for_msr(v, msr) svm_intercept_msr((v), (msr), MSR_INTERCEPT_RW)
>  
> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> index d07fcb2bc9..24bf409d8f 100644
> --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> @@ -656,10 +656,10 @@ bool vmx_msr_is_intercepted(struct vmx_msr_bitmap *msr_bitmap,
>                              unsigned int msr, bool is_write) __nonnull(1);
>  void virtual_vmcs_enter(const struct vcpu *);
>  void virtual_vmcs_exit(const struct vcpu *);
> -u64 virtual_vmcs_vmread(const struct vcpu *, u32 encoding);
> +u64 virtual_vmcs_vmread(const struct vcpu *, u32 vmcs_encoding);

Shouldn't the first parameter be "v" to match the definition?

Or is that a different MISRA C rule?


>  enum vmx_insn_errno virtual_vmcs_vmread_safe(const struct vcpu *v,
>                                               u32 vmcs_encoding, u64 *val);
> -void virtual_vmcs_vmwrite(const struct vcpu *, u32 encoding, u64 val);
> +void virtual_vmcs_vmwrite(const struct vcpu *, u32 vmcs_encoding, u64 val);

same here


>  enum vmx_insn_errno virtual_vmcs_vmwrite_safe(const struct vcpu *v,
>                                                u32 vmcs_encoding, u64 val);
>  
> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vvmx.h b/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
> index dc9db69258..1e4bbc0d78 100644
> --- a/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
> @@ -144,9 +144,9 @@ enum vvmcs_encoding_type {
>      VVMCS_TYPE_HSTATE,
>  };
>  
> -u64 get_vvmcs_virtual(void *vvmcs, u32 encoding);
> +u64 get_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding);

This one should return uint64_t to match the definition. Or is that a
different MISRA C rule?


>  u64 get_vvmcs_real(const struct vcpu *, u32 encoding);
> -void set_vvmcs_virtual(void *vvmcs, u32 encoding, u64 val);
> +void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val);

This one should be:
void set_vvmcs_virtual(void *vvmcs, uint32_t vmcs_encoding, uint64_t val)

Other than these, everything else checks out

Re: [XEN PATCH 5/5] x86: make parameter names of function declarations consistent

[Federico Serafini]

Hello Stefano,

On 29/06/23 21:47, Stefano Stabellini wrote:
> On Thu, 29 Jun 2023, Federico Serafini wrote:
>> Change the parameter names of function declarations to be consistent
>> with the names used in the corresponding function definitions
>> so as to fix violations of MISRA C:2012 Rule 8.3.
>>
>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>> ---
>>   
>> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>> index d07fcb2bc9..24bf409d8f 100644
>> --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>> @@ -656,10 +656,10 @@ bool vmx_msr_is_intercepted(struct vmx_msr_bitmap *msr_bitmap,
>>                               unsigned int msr, bool is_write) __nonnull(1);
>>   void virtual_vmcs_enter(const struct vcpu *);
>>   void virtual_vmcs_exit(const struct vcpu *);
>> -u64 virtual_vmcs_vmread(const struct vcpu *, u32 encoding);
>> +u64 virtual_vmcs_vmread(const struct vcpu *, u32 vmcs_encoding);
> 
> Shouldn't the first parameter be "v" to match the definition?
> 
> Or is that a different MISRA C rule?

This is a violation of MISRA C:2012 Rule 8.2:
"function types shall be in prototype form with named parameters".
However, I can propose a new patch version to fix it as well.

>>   enum vmx_insn_errno virtual_vmcs_vmread_safe(const struct vcpu *v,
>>                                                u32 vmcs_encoding, u64 *val);
>> -void virtual_vmcs_vmwrite(const struct vcpu *, u32 encoding, u64 val);
>> +void virtual_vmcs_vmwrite(const struct vcpu *, u32 vmcs_encoding, u64 val);
> 
> same here
> 
> 
>>   enum vmx_insn_errno virtual_vmcs_vmwrite_safe(const struct vcpu *v,
>>                                                 u32 vmcs_encoding, u64 val);
>>   
>> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vvmx.h b/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
>> index dc9db69258..1e4bbc0d78 100644
>> --- a/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
>> +++ b/xen/arch/x86/include/asm/hvm/vmx/vvmx.h
>> @@ -144,9 +144,9 @@ enum vvmcs_encoding_type {
>>       VVMCS_TYPE_HSTATE,
>>   };
>>   
>> -u64 get_vvmcs_virtual(void *vvmcs, u32 encoding);
>> +u64 get_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding);
> 
> This one should return uint64_t to match the definition. Or is that a
> different MISRA C rule?

This is a violation of the same rule (8.3) but the focus of this patch 
series was intended to be on be only on parameter names.
I can propose a new version of the patch to also address the additional 
violations discussed.

>>   u64 get_vvmcs_real(const struct vcpu *, u32 encoding);
>> -void set_vvmcs_virtual(void *vvmcs, u32 encoding, u64 val);
>> +void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val);
> 
> This one should be:
> void set_vvmcs_virtual(void *vvmcs, uint32_t vmcs_encoding, uint64_t val)
> 
> Other than these, everything else checks out

Regards

-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH 5/5] x86: make parameter names of function declarations consistent

[Andrew Cooper]

On 30/06/2023 8:36 am, Federico Serafini wrote:
> Hello Stefano,
>
> On 29/06/23 21:47, Stefano Stabellini wrote:
>> On Thu, 29 Jun 2023, Federico Serafini wrote:
>>> Change the parameter names of function declarations to be consistent
>>> with the names used in the corresponding function definitions
>>> so as to fix violations of MISRA C:2012 Rule 8.3.
>>>
>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>>> ---
>>>   diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>>> b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>>> index d07fcb2bc9..24bf409d8f 100644
>>> --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>>> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>>> @@ -656,10 +656,10 @@ bool vmx_msr_is_intercepted(struct
>>> vmx_msr_bitmap *msr_bitmap,
>>>                               unsigned int msr, bool is_write)
>>> __nonnull(1);
>>>   void virtual_vmcs_enter(const struct vcpu *);
>>>   void virtual_vmcs_exit(const struct vcpu *);
>>> -u64 virtual_vmcs_vmread(const struct vcpu *, u32 encoding);
>>> +u64 virtual_vmcs_vmread(const struct vcpu *, u32 vmcs_encoding);
>>
>> Shouldn't the first parameter be "v" to match the definition?
>>
>> Or is that a different MISRA C rule?
>
> This is a violation of MISRA C:2012 Rule 8.2:
> "function types shall be in prototype form with named parameters".
> However, I can propose a new patch version to fix it as well.

As a general note - if you need to make multiple changes like this, it's
far better to do them as a single patch.

The end result tends to be easier to review, and it reduces the textural
dependencies between the various patches floating about on list.

~Andrew

Re: [XEN PATCH 5/5] x86: make parameter names of function declarations consistent

[Federico Serafini]

On 30/06/23 16:21, Andrew Cooper wrote:
> On 30/06/2023 8:36 am, Federico Serafini wrote:
>> Hello Stefano,
>>
>> On 29/06/23 21:47, Stefano Stabellini wrote:
>>> On Thu, 29 Jun 2023, Federico Serafini wrote:
>>>> Change the parameter names of function declarations to be consistent
>>>> with the names used in the corresponding function definitions
>>>> so as to fix violations of MISRA C:2012 Rule 8.3.
>>>>
>>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>>>> ---
>>>>    diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>>>> b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>>>> index d07fcb2bc9..24bf409d8f 100644
>>>> --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>>>> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>>>> @@ -656,10 +656,10 @@ bool vmx_msr_is_intercepted(struct
>>>> vmx_msr_bitmap *msr_bitmap,
>>>>                                unsigned int msr, bool is_write)
>>>> __nonnull(1);
>>>>    void virtual_vmcs_enter(const struct vcpu *);
>>>>    void virtual_vmcs_exit(const struct vcpu *);
>>>> -u64 virtual_vmcs_vmread(const struct vcpu *, u32 encoding);
>>>> +u64 virtual_vmcs_vmread(const struct vcpu *, u32 vmcs_encoding);
>>>
>>> Shouldn't the first parameter be "v" to match the definition?
>>>
>>> Or is that a different MISRA C rule?
>>
>> This is a violation of MISRA C:2012 Rule 8.2:
>> "function types shall be in prototype form with named parameters".
>> However, I can propose a new patch version to fix it as well.
> 
> As a general note - if you need to make multiple changes like this, it's
> far better to do them as a single patch.
> 
> The end result tends to be easier to review, and it reduces the textural
> dependencies between the various patches floating about on list.
> 
> ~Andrew

Hello Andrew,
I will try to do it.

Regards

-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH 5/5] x86: make parameter names of function declarations consistent

[Jan Beulich]

On 29.06.2023 17:55, Federico Serafini wrote:
> --- a/xen/arch/x86/cpu/mcheck/mce.h
> +++ b/xen/arch/x86/cpu/mcheck/mce.h
> @@ -44,7 +44,7 @@ extern uint8_t cmci_apic_vector;
>  extern bool lmce_support;
>  
>  /* Init functions */
> -enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *c);
> +enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *ci);

Supported even by ...

>  enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool bsp);

... immediate context, c is the name we use for parameters of this type.
You want to change the definition in such cases instead of the
declaration.

I also think this patch could do with splitting, not the least to
reduce the Cc list(s) needed.

Jan

Re: [XEN PATCH 5/5] x86: make parameter names of function declarations consistent

[Stefano Stabellini]

On Tue, 4 Jul 2023, Jan Beulich wrote:
> On 29.06.2023 17:55, Federico Serafini wrote:
> > --- a/xen/arch/x86/cpu/mcheck/mce.h
> > +++ b/xen/arch/x86/cpu/mcheck/mce.h
> > @@ -44,7 +44,7 @@ extern uint8_t cmci_apic_vector;
> >  extern bool lmce_support;
> >  
> >  /* Init functions */
> > -enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *c);
> > +enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *ci);
> 
> Supported even by ...
> 
> >  enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool bsp);
> 
> ... immediate context, c is the name we use for parameters of this type.
> You want to change the definition in such cases instead of the
> declaration.
> 
> I also think this patch could do with splitting, not the least to
> reduce the Cc list(s) needed.

This is a very large patch but it is entirely mechanical (good!)

How would you see it split? By individual files (too many in my
opinion), or maybe by directory? By directory it would be something
like:

xen/arch/x86/cpu/mcheck
xen/arch/x86/hvm
xen/arch/x86/include/asm

Re: [XEN PATCH 5/5] x86: make parameter names of function declarations consistent

[Stefano Stabellini]

On Wed, 5 Jul 2023, Stefano Stabellini wrote:
> On Tue, 4 Jul 2023, Jan Beulich wrote:
> > On 29.06.2023 17:55, Federico Serafini wrote:
> > > --- a/xen/arch/x86/cpu/mcheck/mce.h
> > > +++ b/xen/arch/x86/cpu/mcheck/mce.h
> > > @@ -44,7 +44,7 @@ extern uint8_t cmci_apic_vector;
> > >  extern bool lmce_support;
> > >  
> > >  /* Init functions */
> > > -enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *c);
> > > +enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *ci);
> > 
> > Supported even by ...
> > 
> > >  enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool bsp);
> > 
> > ... immediate context, c is the name we use for parameters of this type.
> > You want to change the definition in such cases instead of the
> > declaration.

I should add that if we are going to change some functions in the other
direction then it would be best to move those changes in a separate
patch to make it easier to review.


> > I also think this patch could do with splitting, not the least to
> > reduce the Cc list(s) needed.
> 
> This is a very large patch but it is entirely mechanical (good!)
> 
> How would you see it split? By individual files (too many in my
> opinion), or maybe by directory? By directory it would be something
> like:
> 
> xen/arch/x86/cpu/mcheck
> xen/arch/x86/hvm
> xen/arch/x86/include/asm
> 

Re: [XEN PATCH 5/5] x86: make parameter names of function declarations consistent

[Jan Beulich]

On 06.07.2023 00:52, Stefano Stabellini wrote:
> On Wed, 5 Jul 2023, Stefano Stabellini wrote:
>> On Tue, 4 Jul 2023, Jan Beulich wrote:
>>> On 29.06.2023 17:55, Federico Serafini wrote:
>>>> --- a/xen/arch/x86/cpu/mcheck/mce.h
>>>> +++ b/xen/arch/x86/cpu/mcheck/mce.h
>>>> @@ -44,7 +44,7 @@ extern uint8_t cmci_apic_vector;
>>>>  extern bool lmce_support;
>>>>  
>>>>  /* Init functions */
>>>> -enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *c);
>>>> +enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *ci);
>>>
>>> Supported even by ...
>>>
>>>>  enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool bsp);
>>>
>>> ... immediate context, c is the name we use for parameters of this type.
>>> You want to change the definition in such cases instead of the
>>> declaration.
> 
> I should add that if we are going to change some functions in the other
> direction then it would be best to move those changes in a separate
> patch to make it easier to review.

+1

Jan

Re: [XEN PATCH 5/5] x86: make parameter names of function declarations consistent

[Jan Beulich]

On 06.07.2023 00:51, Stefano Stabellini wrote:
> On Tue, 4 Jul 2023, Jan Beulich wrote:
>> On 29.06.2023 17:55, Federico Serafini wrote:
>>> --- a/xen/arch/x86/cpu/mcheck/mce.h
>>> +++ b/xen/arch/x86/cpu/mcheck/mce.h
>>> @@ -44,7 +44,7 @@ extern uint8_t cmci_apic_vector;
>>>  extern bool lmce_support;
>>>  
>>>  /* Init functions */
>>> -enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *c);
>>> +enum mcheck_type amd_mcheck_init(struct cpuinfo_x86 *ci);
>>
>> Supported even by ...
>>
>>>  enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool bsp);
>>
>> ... immediate context, c is the name we use for parameters of this type.
>> You want to change the definition in such cases instead of the
>> declaration.
>>
>> I also think this patch could do with splitting, not the least to
>> reduce the Cc list(s) needed.
> 
> This is a very large patch but it is entirely mechanical (good!)
> 
> How would you see it split? By individual files (too many in my
> opinion), or maybe by directory? By directory it would be something
> like:
> 
> xen/arch/x86/cpu/mcheck
> xen/arch/x86/hvm
> xen/arch/x86/include/asm

As for the other patch and as mentioned in the earlier replay: By
maintainership area, such that we don't need to accumulate a dozen
of acks (easy to miss one) in order for the patch to go in. As much
as the patch is mechanical, it ought to be largely mechanical to
split the patch at maintainership boundaries.

Jan

Re: [XEN PATCH 0/5] Fix violations of MISRA C:2012 Rule 8.3 on parameter names

[Andrew Cooper]

On 29/06/2023 4:55 pm, Federico Serafini wrote:
> Federico Serafini (5):
>   x86: swap parameter names of hvm_copy_context_and_params() declaration
>   x86: change parameter names of nestedhvm_vcpu_iomap_get() definition
>   x86/vlapic: change parameter names in function definitions

I've acked/committed the first 3 patches, with the various tweaks that
Stefano identified.

nestedhvm_vcpu_iomap_get() is utter nonsense (it would absolutely not
have passed review if I'd seen it before it went in), but untangling
nested virt is multiple cans of worms, so I've put the name changes in
as-are to avoid blocking the MISRA effort.

The final two patches need a bit more consideration time.

~Andrew