Re: [XEN PATCH 3/6] AMD/IOMMU: address violations of MISRA C:2012 Rule 8.2

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 5 Dec 2023, Federico Serafini wrote:
> Add missing parameter names to address violations of MISRA C:2012
> Rule 8.2. Furthermore, remove trailing spaces and use C standard types to
> comply with XEN coding style.
> 
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> ---
>  xen/drivers/passthrough/amd/iommu.h      | 17 ++++++++++-------
>  xen/drivers/passthrough/amd/iommu_init.c | 24 ++++++++++++++----------
>  2 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu.h 
> b/xen/drivers/passthrough/amd/iommu.h
> index d4416ebc43..1b62c083ba 100644
> --- a/xen/drivers/passthrough/amd/iommu.h
> +++ b/xen/drivers/passthrough/amd/iommu.h
> @@ -138,10 +138,12 @@ struct ivrs_mappings {
>  extern unsigned int ivrs_bdf_entries;
>  extern u8 ivhd_type;
>  
> -struct ivrs_mappings *get_ivrs_mappings(u16 seg);
> -int iterate_ivrs_mappings(int (*)(u16 seg, struct ivrs_mappings *));
> -int iterate_ivrs_entries(int (*)(const struct amd_iommu *,
> -                                 struct ivrs_mappings *, uint16_t));
> +struct ivrs_mappings *get_ivrs_mappings(uint16_t seg);
> +int iterate_ivrs_mappings(int (*handler)(uint16_t seg,
> +                                         struct ivrs_mappings *map));
> +int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *iommu,
> +                                        struct ivrs_mappings *map,
> +                                        uint16_t bdf));
>  
>  /* iommu tables in guest space */
>  struct mmio_reg {
> @@ -226,7 +228,7 @@ struct acpi_ivrs_hardware;
>  /* amd-iommu-detect functions */
>  int amd_iommu_get_ivrs_dev_entries(void);
>  int amd_iommu_get_supported_ivhd_type(void);
> -int amd_iommu_detect_one_acpi(const struct acpi_ivrs_hardware *);
> +int amd_iommu_detect_one_acpi(const struct acpi_ivrs_hardware *ivhd_block);
>  int amd_iommu_detect_acpi(void);
>  void get_iommu_features(struct amd_iommu *iommu);
>  
> @@ -295,9 +297,10 @@ struct amd_iommu *find_iommu_for_device(int seg, int 
> bdf);
>  bool cf_check iov_supports_xt(void);
>  int amd_iommu_setup_ioapic_remapping(void);
>  void *amd_iommu_alloc_intremap_table(
> -    const struct amd_iommu *, unsigned long **, unsigned int nr);
> +    const struct amd_iommu *iommu, unsigned long **inuse_map, unsigned int 
> nr);
>  int cf_check amd_iommu_free_intremap_table(
> -    const struct amd_iommu *, struct ivrs_mappings *, uint16_t);
> +    const struct amd_iommu *iommu, struct ivrs_mappings *ivrs_mapping,
> +    uint16_t bdf);
>  unsigned int amd_iommu_intremap_table_order(
>      const void *irt, const struct amd_iommu *iommu);
>  void cf_check amd_iommu_ioapic_update_ire(
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c 
> b/xen/drivers/passthrough/amd/iommu_init.c
> index 5515cb70fd..62f9bfdfc8 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -300,12 +300,13 @@ static void cf_check set_iommu_ppr_log_control(
>  static int iommu_read_log(struct amd_iommu *iommu,
>                            struct ring_buffer *log,
>                            unsigned int entry_size,
> -                          void (*parse_func)(struct amd_iommu *, u32 *))
> +                          void (*parse_func)(struct amd_iommu *iommu,
> +                                             uint32_t *entry))
>  {
>      unsigned int tail, tail_offest, head_offset;
>  
>      BUG_ON(!iommu || ((log != &iommu->event_log) && (log != 
> &iommu->ppr_log)));
> -    
> +
>      spin_lock(&log->lock);
>  
>      /* make sure there's an entry in the log */
> @@ -361,14 +362,15 @@ static int iommu_read_log(struct amd_iommu *iommu,
>  
>   out:
>      spin_unlock(&log->lock);
> -   
> +
>      return 0;
>  }
>  
>  /* reset event log or ppr log when overflow */
>  static void iommu_reset_log(struct amd_iommu *iommu,
>                              struct ring_buffer *log,
> -                            void (*ctrl_func)(struct amd_iommu *iommu, bool))
> +                            void (*ctrl_func)(struct amd_iommu *iommu,
> +                                              bool iommu_control))

instead of iommu_control it should be iommu_enable ?


>  {
>      unsigned int entry, run_bit, loop_count = 1000;
>      bool log_run;
> @@ -1158,14 +1160,15 @@ static void __init amd_iommu_init_cleanup(void)
>      iommuv2_enabled = 0;
>  }
>  
> -struct ivrs_mappings *get_ivrs_mappings(u16 seg)
> +struct ivrs_mappings *get_ivrs_mappings(uint16_t seg)
>  {
>      return radix_tree_lookup(&ivrs_maps, seg);
>  }
>  
> -int iterate_ivrs_mappings(int (*handler)(u16 seg, struct ivrs_mappings *))
> +int iterate_ivrs_mappings(int (*handler)(uint16_t seg,
> +                                         struct ivrs_mappings *map))

Instead of map it should be ivrs_mappings ? Actually it reads better as
map and I know it is not a MISRA requirement to have function pointer
args match. I'll leave this one to Jan.




>  {
> -    u16 seg = 0;
> +    uint16_t seg = 0;
>      int rc = 0;
>  
>      do {
> @@ -1180,10 +1183,11 @@ int iterate_ivrs_mappings(int (*handler)(u16 seg, 
> struct ivrs_mappings *))
>      return rc;
>  }
>  
> -int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *,
> -                                        struct ivrs_mappings *, uint16_t 
> bdf))
> +int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *iommu,
> +                                        struct ivrs_mappings *map,

same here ivrs_mappings instead of map, but actually map reads better

I checked everything else and looks OK