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

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

[Jan Beulich]

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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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;
 

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

[dmkhn]

On Wed, Jul 23, 2025 at 03:39:11PM +0200, 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.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Denis Mukhin <dmukhin@ford.com> 

> ---
> 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;
> 
> 

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

[Jan Beulich]

On 23.07.2025 15: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.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v3: Use xv[mz]alloc_array().

May I ask for an ack (or otherwise) please? Perhaps also for Ross'es
"efi: Call FreePages only if needed"?

Thanks, Jan

> --- 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;
>  

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

[Daniel P. Smith]

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@suse.com>
> ---
> 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@apertussolutions.com>

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

[Jan Beulich]

On 12.08.2025 02:19, Daniel P. Smith wrote:
> 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).

My take is that xmalloc_bytes() should never have existed. Hence why I
didn't add xzmalloc_bytes() when introducing that family of interfaces.

> With a stronger description of the change,

So what exactly do you mean by "stronger"? I can add that in the unlikely
event that one of the allocations is (near) PAGE_SIZE or larger, we now
wouldn't require contiguous memory anymore. Yet based on your comment at
the top I'm not quite sure if that's what you're after and/or enough to
satisfy your request.

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

Thanks, but will need clarification first as per above.

Jan

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

[Daniel P. Smith]

On 8/12/25 02:12, Jan Beulich wrote:
> On 12.08.2025 02:19, Daniel P. Smith wrote:
>> 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).
> 
> My take is that xmalloc_bytes() should never have existed. Hence why I
> didn't add xzmalloc_bytes() when introducing that family of interfaces.

Right, which would be a valid argument for replacing it with 
xmalloc_array(). Though, I would note that there is an xzalloc_bytes(). 
My concern was that you stated there was an open coding, which had me 
expecting there was a line of the form, xmanlloc_bytes(count * 
size_of_something bigger), being replaced by 
xvmalloc_arryay(something_bigger, count).

IMHO, while the C spec does specify char as 1 byte and thus 
interchangeable, I would agree that from a contextual perspective, 
xmalloc_array() is the more appropriate call. The use of the allocation 
is a character array and not a chunk of bytes for an arbitrary buffer.

>> With a stronger description of the change,
> 
> So what exactly do you mean by "stronger"? I can add that in the unlikely
> event that one of the allocations is (near) PAGE_SIZE or larger, we now
> wouldn't require contiguous memory anymore. Yet based on your comment at
> the top I'm not quite sure if that's what you're after and/or enough to
> satisfy your request.

The phrasing stronger was meant to be more clear on the change/effect, 
specifically that the underlying allocator is being changed when the 
allocation is greater than a PAGE_SIZE. Not necessarily a long 
explanation, just the fact that the allocation will be coming from the 
dom heap allocator as opposed to the xen heap allocator. There are 
implications to changing the allocater, e.g.,  at a minimum the 
allocation order and nonphysical vs. physically contiguous effects. 
Having it noted in the commit makes it more obvious what this change is 
actually doing. Which may not be obvious when seeing the simple line 
changes occurring in the diff. Later, if there is an unexpected 
consequence caused by this change, a stronger commit will be helpful 
with the bisection investigations.

dps

>> Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> Thanks, but will need clarification first as per above.
> 
> Jan

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

[Jan Beulich]

On 14.08.2025 02:29, Daniel P. Smith wrote:
> On 8/12/25 02:12, Jan Beulich wrote:
>> On 12.08.2025 02:19, Daniel P. Smith wrote:
>>> 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).
>>
>> My take is that xmalloc_bytes() should never have existed. Hence why I
>> didn't add xzmalloc_bytes() when introducing that family of interfaces.
> 
> Right, which would be a valid argument for replacing it with 
> xmalloc_array(). Though, I would note that there is an xzalloc_bytes(). 
> My concern was that you stated there was an open coding, which had me 
> expecting there was a line of the form, xmanlloc_bytes(count * 
> size_of_something bigger), being replaced by 
> xvmalloc_arryay(something_bigger, count).

Both fir this and ...

> IMHO, while the C spec does specify char as 1 byte and thus 
> interchangeable, I would agree that from a contextual perspective, 
> xmalloc_array() is the more appropriate call. The use of the allocation 
> is a character array and not a chunk of bytes for an arbitrary buffer.

... for this: Hence my wording using "effectively".

>>> With a stronger description of the change,
>>
>> So what exactly do you mean by "stronger"? I can add that in the unlikely
>> event that one of the allocations is (near) PAGE_SIZE or larger, we now
>> wouldn't require contiguous memory anymore. Yet based on your comment at
>> the top I'm not quite sure if that's what you're after and/or enough to
>> satisfy your request.
> 
> The phrasing stronger was meant to be more clear on the change/effect, 
> specifically that the underlying allocator is being changed when the 
> allocation is greater than a PAGE_SIZE. Not necessarily a long 
> explanation, just the fact that the allocation will be coming from the 
> dom heap allocator as opposed to the xen heap allocator. There are 
> implications to changing the allocater, e.g.,  at a minimum the 
> allocation order and nonphysical vs. physically contiguous effects. 
> Having it noted in the commit makes it more obvious what this change is 
> actually doing. Which may not be obvious when seeing the simple line 
> changes occurring in the diff. Later, if there is an unexpected 
> consequence caused by this change, a stronger commit will be helpful 
> with the bisection investigations.

First: I don't think each and every such change (there are going to be many)
should re-explain the switch to the xvmalloc() family of functions. This is
already stated clearly at the top of xvmalloc.h: Over time, the entire code
base is meant to be switched.

Beyond that, to achieve the stronger wording you're after, would it perhaps
suffice to have the first sentence say "..., thus also doing away ..."?
Otherwise, may I ask that you please make a more concrete suggestion?

Jan

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

[Jan Beulich]

On 14.08.2025 08:46, Jan Beulich wrote:
> On 14.08.2025 02:29, Daniel P. Smith wrote:
>> On 8/12/25 02:12, Jan Beulich wrote:
>>> On 12.08.2025 02:19, Daniel P. Smith wrote:
>>>> 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).
>>>
>>> My take is that xmalloc_bytes() should never have existed. Hence why I
>>> didn't add xzmalloc_bytes() when introducing that family of interfaces.
>>
>> Right, which would be a valid argument for replacing it with 
>> xmalloc_array(). Though, I would note that there is an xzalloc_bytes(). 
>> My concern was that you stated there was an open coding, which had me 
>> expecting there was a line of the form, xmanlloc_bytes(count * 
>> size_of_something bigger), being replaced by 
>> xvmalloc_arryay(something_bigger, count).
> 
> Both fir this and ...
> 
>> IMHO, while the C spec does specify char as 1 byte and thus 
>> interchangeable, I would agree that from a contextual perspective, 
>> xmalloc_array() is the more appropriate call. The use of the allocation 
>> is a character array and not a chunk of bytes for an arbitrary buffer.
> 
> ... for this: Hence my wording using "effectively".
> 
>>>> With a stronger description of the change,
>>>
>>> So what exactly do you mean by "stronger"? I can add that in the unlikely
>>> event that one of the allocations is (near) PAGE_SIZE or larger, we now
>>> wouldn't require contiguous memory anymore. Yet based on your comment at
>>> the top I'm not quite sure if that's what you're after and/or enough to
>>> satisfy your request.
>>
>> The phrasing stronger was meant to be more clear on the change/effect, 
>> specifically that the underlying allocator is being changed when the 
>> allocation is greater than a PAGE_SIZE. Not necessarily a long 
>> explanation, just the fact that the allocation will be coming from the 
>> dom heap allocator as opposed to the xen heap allocator. There are 
>> implications to changing the allocater, e.g.,  at a minimum the 
>> allocation order and nonphysical vs. physically contiguous effects. 
>> Having it noted in the commit makes it more obvious what this change is 
>> actually doing. Which may not be obvious when seeing the simple line 
>> changes occurring in the diff. Later, if there is an unexpected 
>> consequence caused by this change, a stronger commit will be helpful 
>> with the bisection investigations.
> 
> First: I don't think each and every such change (there are going to be many)
> should re-explain the switch to the xvmalloc() family of functions. This is
> already stated clearly at the top of xvmalloc.h: Over time, the entire code
> base is meant to be switched.
> 
> Beyond that, to achieve the stronger wording you're after, would it perhaps
> suffice to have the first sentence say "..., thus also doing away ..."?
> Otherwise, may I ask that you please make a more concrete suggestion?

Ping. I'd like to get this in, yet I still don't know how exactly to satisfy
your request for a stronger description.

Jan

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

[Daniel P. Smith]

On 8/25/25 7:06 AM, Jan Beulich wrote:
> On 14.08.2025 08:46, Jan Beulich wrote:
>> On 14.08.2025 02:29, Daniel P. Smith wrote:
>>> On 8/12/25 02:12, Jan Beulich wrote:
>>>> On 12.08.2025 02:19, Daniel P. Smith wrote:
>>>>> 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).
>>>>
>>>> My take is that xmalloc_bytes() should never have existed. Hence why I
>>>> didn't add xzmalloc_bytes() when introducing that family of interfaces.
>>>
>>> Right, which would be a valid argument for replacing it with
>>> xmalloc_array(). Though, I would note that there is an xzalloc_bytes().
>>> My concern was that you stated there was an open coding, which had me
>>> expecting there was a line of the form, xmanlloc_bytes(count *
>>> size_of_something bigger), being replaced by
>>> xvmalloc_arryay(something_bigger, count).
>>
>> Both fir this and ...
>>
>>> IMHO, while the C spec does specify char as 1 byte and thus
>>> interchangeable, I would agree that from a contextual perspective,
>>> xmalloc_array() is the more appropriate call. The use of the allocation
>>> is a character array and not a chunk of bytes for an arbitrary buffer.
>>
>> ... for this: Hence my wording using "effectively".
>>
>>>>> With a stronger description of the change,
>>>>
>>>> So what exactly do you mean by "stronger"? I can add that in the unlikely
>>>> event that one of the allocations is (near) PAGE_SIZE or larger, we now
>>>> wouldn't require contiguous memory anymore. Yet based on your comment at
>>>> the top I'm not quite sure if that's what you're after and/or enough to
>>>> satisfy your request.
>>>
>>> The phrasing stronger was meant to be more clear on the change/effect,
>>> specifically that the underlying allocator is being changed when the
>>> allocation is greater than a PAGE_SIZE. Not necessarily a long
>>> explanation, just the fact that the allocation will be coming from the
>>> dom heap allocator as opposed to the xen heap allocator. There are
>>> implications to changing the allocater, e.g.,  at a minimum the
>>> allocation order and nonphysical vs. physically contiguous effects.
>>> Having it noted in the commit makes it more obvious what this change is
>>> actually doing. Which may not be obvious when seeing the simple line
>>> changes occurring in the diff. Later, if there is an unexpected
>>> consequence caused by this change, a stronger commit will be helpful
>>> with the bisection investigations.
>>
>> First: I don't think each and every such change (there are going to be many)
>> should re-explain the switch to the xvmalloc() family of functions. This is
>> already stated clearly at the top of xvmalloc.h: Over time, the entire code
>> base is meant to be switched.
>>
>> Beyond that, to achieve the stronger wording you're after, would it perhaps
>> suffice to have the first sentence say "..., thus also doing away ..."?
>> Otherwise, may I ask that you please make a more concrete suggestion?
> 
> Ping. I'd like to get this in, yet I still don't know how exactly to satisfy
> your request for a stronger description.
> 
> Jan

This fell off my plate and not fair to Jan, I will just rescind my concern.

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