Re: [PATCH v3] EFI/runtime: switch to xv[mz]alloc_array()

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 7/23/25 09:39, Jan Beulich wrote:
> Use the more "modern" form, thus doing away with effectively open-coding
> xmalloc_array() at the same time. While there is a difference in
> generated code, as xmalloc_bytes() forces SMP_CACHE_BYTES alignment, if
> code really cared about such higher than default alignment, it should
> request so explicitly.
While I don't object to the change itself, I think this description is a 
bit over simplification of the change. If the allocation is under 
PAGE_SIZE, then they are equivalent, but if it is over the page size 
there are a few more differences than just cache alignment. It 
completely changes the underlying allocator. I personally also find it a 
bit of a stretch to call xmalloc_bytes(size) an open coded version of 
xmalloc_array(char, size).
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v3: Use xv[mz]alloc_array().
>
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -6,6 +6,7 @@
>   #include <xen/irq.h>
>   #include <xen/sections.h>
>   #include <xen/time.h>
> +#include <xen/xvmalloc.h>
>   
>   DEFINE_XEN_GUEST_HANDLE(CHAR16);
>   
> @@ -500,23 +501,23 @@ int efi_runtime_call(struct xenpf_efi_ru
>           len = gwstrlen(guest_handle_cast(op->u.get_variable.name, CHAR16));
>           if ( len < 0 )
>               return len;
> -        name = xmalloc_array(CHAR16, ++len);
> +        name = xvmalloc_array(CHAR16, ++len);
>           if ( !name )
>              return -ENOMEM;
>           if ( __copy_from_guest(name, op->u.get_variable.name, len) ||
>                wmemchr(name, 0, len) != name + len - 1 )
>           {
> -            xfree(name);
> +            xvfree(name);
>               return -EIO;
>           }
>   
>           size = op->u.get_variable.size;
>           if ( size )
>           {
> -            data = xmalloc_bytes(size);
> +            data = xvmalloc_array(unsigned char, size);
>               if ( !data )
>               {
> -                xfree(name);
> +                xvfree(name);
>                   return -ENOMEM;
>               }
>           }
> @@ -539,8 +540,8 @@ int efi_runtime_call(struct xenpf_efi_ru
>           else
>               rc = -EOPNOTSUPP;
>   
> -        xfree(data);
> -        xfree(name);
> +        xvfree(data);
> +        xvfree(name);
>       }
>       break;
>   
> @@ -553,17 +554,17 @@ int efi_runtime_call(struct xenpf_efi_ru
>           len = gwstrlen(guest_handle_cast(op->u.set_variable.name, CHAR16));
>           if ( len < 0 )
>               return len;
> -        name = xmalloc_array(CHAR16, ++len);
> +        name = xvmalloc_array(CHAR16, ++len);
>           if ( !name )
>              return -ENOMEM;
>           if ( __copy_from_guest(name, op->u.set_variable.name, len) ||
>                wmemchr(name, 0, len) != name + len - 1 )
>           {
> -            xfree(name);
> +            xvfree(name);
>               return -EIO;
>           }
>   
> -        data = xmalloc_bytes(op->u.set_variable.size);
> +        data = xvmalloc_array(unsigned char, op->u.set_variable.size);
>           if ( !data )
>               rc = -ENOMEM;
>           else if ( copy_from_guest(data, op->u.set_variable.data,
> @@ -581,8 +582,8 @@ int efi_runtime_call(struct xenpf_efi_ru
>               efi_rs_leave(&state);
>           }
>   
> -        xfree(data);
> -        xfree(name);
> +        xvfree(data);
> +        xvfree(name);
>       }
>       break;
>   
> @@ -598,13 +599,13 @@ int efi_runtime_call(struct xenpf_efi_ru
>               return -EINVAL;
>   
>           size = op->u.get_next_variable_name.size;
> -        name.raw = xzalloc_bytes(size);
> +        name.raw = xvzalloc_array(unsigned char, size);
>           if ( !name.raw )
>               return -ENOMEM;
>           if ( copy_from_guest(name.raw, op->u.get_next_variable_name.name,
>                                size) )
>           {
> -            xfree(name.raw);
> +            xvfree(name.raw);
>               return -EFAULT;
>           }
>   
> @@ -629,7 +630,7 @@ int efi_runtime_call(struct xenpf_efi_ru
>           else
>               rc = -EOPNOTSUPP;
>   
> -        xfree(name.raw);
> +        xvfree(name.raw);
>       }
>       break;
>   
With a stronger description of the change,

Acked-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>