Re: [Xen-devel] [PATCH v7 2/3] AMD/IOMMU: allow callers to request allocate_buffer() to skip its memset()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/09/2019 15:29, Jan Beulich wrote:
> The command ring buffer doesn't need clearing up front in any event.
> Subsequently we'll also want to avoid clearing the device tables.
>
> While playing with functions signatures replace undue use of fixed width
> types at the same time, and extend this to deallocate_buffer() as well.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v7: New.
>
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -994,12 +994,12 @@ static unsigned int __init dt_alloc_size
>                                               IOMMU_DEV_TABLE_ENTRY_SIZE);
>  }
>  
> -static void __init deallocate_buffer(void *buf, uint32_t sz)
> +static void __init deallocate_buffer(void *buf, unsigned long sz)

Probably best to use size_t here, being both shorter, and guaranteed not
to require modification in the future.

>  {
> -    int order = 0;
>      if ( buf )
>      {
> -        order = get_order_from_bytes(sz);
> +        unsigned int order = get_order_from_bytes(sz);
> +
>          __free_amd_iommu_tables(buf, order);
>      }
>  }

How about simply

if ( buf )
    __free_amd_iommu_tables(buf, get_order_from_bytes(sz));

which drops the order variable entirely?

Ideally with both of these modifications, Reviewed-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>

> @@ -1050,21 +1055,23 @@ static void * __init allocate_cmd_buffer
>      /* allocate 'command buffer' in power of 2 increments of 4K */
>      return allocate_ring_buffer(&iommu->cmd_buffer, sizeof(cmd_entry_t),
>                                  IOMMU_CMD_BUFFER_DEFAULT_ENTRIES,
> -                                "Command Buffer");
> +                                "Command Buffer", false);
>  }
>  
>  static void * __init allocate_event_log(struct amd_iommu *iommu)
>  {
>      /* allocate 'event log' in power of 2 increments of 4K */
>      return allocate_ring_buffer(&iommu->event_log, sizeof(event_entry_t),
> -                                IOMMU_EVENT_LOG_DEFAULT_ENTRIES, "Event 
> Log");
> +                                IOMMU_EVENT_LOG_DEFAULT_ENTRIES, "Event Log",
> +                                true);

Well - this means I've got yet another AMD IOMMU bugfix hiding somewhere
in a branch.  I could have sworn I posted it.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel