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

[Jan Beulich <jbeulich@xxxxxxxx>]

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