Re: [PATCH 1/2] x86/ucode: allow cpu_request_microcode() to skip memory allocation

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 12/12/2022 14:59, Jan Beulich wrote:
> On 12.12.2022 15:27, Sergey Dyasli wrote:
>> On Thu, Dec 8, 2022 at 3:34 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> On 08.12.2022 14:59, Andrew Cooper wrote:
>>>> On 08/12/2022 13:26, Sergey Dyasli wrote:
>>>>> @@ -240,20 +240,20 @@ static const struct microcode_patch *nmi_patch = 
>>>>> ZERO_BLOCK_PTR;
>>>>>   * patch is found and an error occurs during the parsing process. 
>>>>> Otherwise
>>>>>   * return NULL.
>>>>>   */
>>>>> -static struct microcode_patch *parse_blob(const char *buf, size_t len)
>>>>> +static const struct microcode_patch *parse_blob(const char *buf, size_t 
>>>>> len)
>>>>>  {
>>>>>      alternative_vcall(ucode_ops.collect_cpu_info);
>>>>>
>>>>> -    return alternative_call(ucode_ops.cpu_request_microcode, buf, len);
>>>>> +    return alternative_call(ucode_ops.cpu_request_microcode, buf, len, 
>>>>> true);
>>>>>  }
>>>>>
>>>>> -static void microcode_free_patch(struct microcode_patch *patch)
>>>>> +static void microcode_free_patch(const struct microcode_patch *patch)
>>>>>  {
>>>>> -    xfree(patch);
>>>>> +    xfree((void *)patch);
>>>> This hunk demonstrates why the hook wants to return a non-const
>>>> pointer.  Keeping it non-const will shrink this patch quite a bit.
>>> Alternatively it demonstrates why xfree() should take const void *,
>>> just like e.g. unmap_domain_page() or vunmap() already do. We've
>>> talked about this before, and the argument hasn't changed: Neither
>>> unmapping nor freeing really alters the contents of the pointed to
>>> area from the perspective of the caller, as the contents simply
>>> disappears altogether.
>> Despite my love of const, const correctness in C is quite a pain. I've
>> tried to make xfree() take a const pointer but then issues began with
>> add/strip_padding() functions and I couldn't overcome those without
>> further (void *) casts which just takes the problem to a different
>> layer.
> There is exactly one such cast needed according to my attempt, and that's
> in an internal (static) helper function. See below (and feel free to use).
>
> Jan
>
> mm: make a couple of freeing functions take const void*
>
> freeing functions, from the perspective of their callers, don't alter
> contents of the freed block; the block simply disappears. Plus it is not
> uncommon that some piece of memory is allocated, filled, and henceforth
> supposed to only be read from. In such cases, with the parameters of the
> freeing functions not being pointer-to-const, callers have to either
> omit the use of const where it would be indicated or add risky casts.
>
> The goal being to make xfree() fit the intended pattern, alter other
> functions at the same time to limit the number of casts needed to just
> one. strip_padding() necessarily has to have a such a cast, as it's
> effectively strchr()- or memchr()-like: Input may be pointer-to-const,
> bot for its output to also be usable when the input isn't, const needs
> to be cast away. Note that the risk with such a cast is quite a bit
> lower when it's an internal (static) function wich is affected.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

If you can persuade the C standards committee that your interpretation
of how to use const is more correct than theirs, and get them to change
free() to take const void, then we can continue discussing this patch.

Until then, there's a damn good reason why free() takes a non-const
pointer (nothing handed out by a memory allocator can ever be const, and
it is never acceptable to free via a const alias), and contorted
arguments about how its "technically fine" are missing the point.

~Andrew