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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>

--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2231,7 +2231,7 @@ void *alloc_xenheap_pages(unsigned int o
 }
 
 
-void free_xenheap_pages(void *v, unsigned int order)
+void free_xenheap_pages(const void *v, unsigned int order)
 {
     ASSERT_ALLOC_CONTEXT();
 
@@ -2279,7 +2279,7 @@ void *alloc_xenheap_pages(unsigned int o
     return page_to_virt(pg);
 }
 
-void free_xenheap_pages(void *v, unsigned int order)
+void free_xenheap_pages(const void *v, unsigned int order)
 {
     struct page_info *pg;
     unsigned int i;
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -453,7 +453,7 @@ void *xmem_pool_alloc(unsigned long size
     return NULL;
 }
 
-void xmem_pool_free(void *ptr, struct xmem_pool *pool)
+void xmem_pool_free(const void *ptr, struct xmem_pool *pool)
 {
     struct bhdr *b, *tmp_b;
     int fl = 0, sl = 0;
@@ -563,7 +563,7 @@ static void tlsf_init(void)
  * xmalloc()
  */
 
-static void *strip_padding(void *p)
+static void *strip_padding(const void *p)
 {
     const struct bhdr *b = p - BHDR_OVERHEAD;
 
@@ -574,7 +574,7 @@ static void *strip_padding(void *p)
         ASSERT(!(b->size & FREE_BLOCK));
     }
 
-    return p;
+    return (void *)p;
 }
 
 static void *add_padding(void *p, unsigned long align)
@@ -699,7 +699,7 @@ void *_xrealloc(void *ptr, unsigned long
     return p;
 }
 
-void xfree(void *p)
+void xfree(const void *p)
 {
     ASSERT_ALLOC_CONTEXT();
 
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -73,7 +73,7 @@ void end_boot_allocator(void);
 void init_xenheap_pages(paddr_t ps, paddr_t pe);
 void xenheap_max_mfn(unsigned long mfn);
 void *alloc_xenheap_pages(unsigned int order, unsigned int memflags);
-void free_xenheap_pages(void *v, unsigned int order);
+void free_xenheap_pages(const void *v, unsigned int order);
 bool scrub_free_pages(void);
 #define alloc_xenheap_page() (alloc_xenheap_pages(0,0))
 #define free_xenheap_page(v) (free_xenheap_pages(v,0))
--- a/xen/include/xen/xmalloc.h
+++ b/xen/include/xen/xmalloc.h
@@ -63,7 +63,7 @@
     })
 
 /* Free any of the above. */
-extern void xfree(void *);
+extern void xfree(const void *);
 
 /* Free an allocation, and zero the pointer to it. */
 #define XFREE(p) do { \
@@ -148,7 +148,7 @@ int xmem_pool_maxalloc(struct xmem_pool
  * @ptr: address of memory to be freed
  * @mem_pool: pool to free from
  */
-void xmem_pool_free(void *ptr, struct xmem_pool *pool);
+void xmem_pool_free(const void *ptr, struct xmem_pool *pool);
 
 /**
  * xmem_pool_get_used_size - get memory currently used by given pool