There is a difference in generated code: xzalloc_bytes() forces
SMP_CACHE_BYTES alignment. I think we not only don't need this here, but
actually don't want it.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -463,7 +463,10 @@ static void * alloc_from_crash_heap(cons
 /* Allocate a crash note buffer for a newly onlined cpu. */
 static int kexec_init_cpu_notes(const unsigned long cpu)
 {
-    Elf_Note * note = NULL;
+    struct elf_notes {
+        Elf_Note first;
+        unsigned char more[];
+    } *notes = NULL;
     int ret = 0;
     int nr_bytes = 0;
 
@@ -477,7 +480,8 @@ static int kexec_init_cpu_notes(const un
 
     /* If we dont care about the position of allocation, malloc. */
     if ( low_crashinfo_mode == LOW_CRASHINFO_NONE )
-        note = xzalloc_bytes(nr_bytes);
+        notes = xzalloc_flex_struct(struct elf_notes, more,
+                                    nr_bytes - sizeof(notes->first));
 
     /* Protect the write into crash_notes[] with a spinlock, as this function
      * is on a hotplug path and a hypercall path. */
@@ -490,26 +494,28 @@ static int kexec_init_cpu_notes(const un
         spin_unlock(&crash_notes_lock);
         /* Always return ok, because whether we successfully allocated or not,
          * another CPU has successfully allocated. */
-        xfree(note);
+        xfree(notes);
     }
     else
     {
         /* If we care about memory possition, alloc from the crash heap,
          * also protected by the crash_notes_lock. */
         if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
-            note = alloc_from_crash_heap(nr_bytes);
+            notes = alloc_from_crash_heap(nr_bytes);
 
-        crash_notes[cpu].start = note;
+        crash_notes[cpu].start = &notes->first;
         crash_notes[cpu].size = nr_bytes;
         spin_unlock(&crash_notes_lock);
 
         /* If the allocation failed, and another CPU did not beat us, give
          * up with ENOMEM. */
-        if ( ! note )
+        if ( ! notes )
             ret = -ENOMEM;
         /* else all is good so lets set up the notes. */
         else
         {
+            Elf_Note *note = &notes->first;
+
             /* Set up CORE note. */
             setup_note(note, "CORE", NT_PRSTATUS, sizeof(ELF_Prstatus));
             note = ELFNOTE_NEXT(note);

On 08/04/2021 13:21, Jan Beulich wrote:
> There is a difference in generated code: xzalloc_bytes() forces
> SMP_CACHE_BYTES alignment. I think we not only don't need this here, but
> actually don't want it.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This honestly looks like a backwards step.  In particular, ...

>
> --- a/xen/common/kexec.c
> +++ b/xen/common/kexec.c
> @@ -463,7 +463,10 @@ static void * alloc_from_crash_heap(cons
>  /* Allocate a crash note buffer for a newly onlined cpu. */
>  static int kexec_init_cpu_notes(const unsigned long cpu)
>  {
> -    Elf_Note * note = NULL;
> +    struct elf_notes {
> +        Elf_Note first;
> +        unsigned char more[];
> +    } *notes = NULL;
>      int ret = 0;
>      int nr_bytes = 0;
>  
> @@ -477,7 +480,8 @@ static int kexec_init_cpu_notes(const un
>  
>      /* If we dont care about the position of allocation, malloc. */
>      if ( low_crashinfo_mode == LOW_CRASHINFO_NONE )
> -        note = xzalloc_bytes(nr_bytes);
> +        notes = xzalloc_flex_struct(struct elf_notes, more,
> +                                    nr_bytes - sizeof(notes->first));

... this is far more error prone than the code you replaced, seeing as
there is now a chance that it can underflow.

~Andrew

On 09.04.2021 14:54, Andrew Cooper wrote:
> On 08/04/2021 13:21, Jan Beulich wrote:
>> There is a difference in generated code: xzalloc_bytes() forces
>> SMP_CACHE_BYTES alignment. I think we not only don't need this here, but
>> actually don't want it.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> This honestly looks like a backwards step.  In particular, ...
> 
>>
>> --- a/xen/common/kexec.c
>> +++ b/xen/common/kexec.c
>> @@ -463,7 +463,10 @@ static void * alloc_from_crash_heap(cons
>>  /* Allocate a crash note buffer for a newly onlined cpu. */
>>  static int kexec_init_cpu_notes(const unsigned long cpu)
>>  {
>> -    Elf_Note * note = NULL;
>> +    struct elf_notes {
>> +        Elf_Note first;
>> +        unsigned char more[];
>> +    } *notes = NULL;
>>      int ret = 0;
>>      int nr_bytes = 0;
>>  
>> @@ -477,7 +480,8 @@ static int kexec_init_cpu_notes(const un
>>  
>>      /* If we dont care about the position of allocation, malloc. */
>>      if ( low_crashinfo_mode == LOW_CRASHINFO_NONE )
>> -        note = xzalloc_bytes(nr_bytes);
>> +        notes = xzalloc_flex_struct(struct elf_notes, more,
>> +                                    nr_bytes - sizeof(notes->first));
> 
> ... this is far more error prone than the code you replaced, seeing as
> there is now a chance that it can underflow.

You're kidding, aren't you? sizeof_cpu_notes() can't possibly return
a value which would allow underflow here. There would be bigger
issues than the underflow if any such happened, in particular with
any theoretical underflow here simply resulting in the allocation to
fail.

The original code is type-unsafe and requests more alignment than
necessary. Besides objecting to my present way of addressing this,
I would find it rather helpful if you could mention some kind of
alternative you'd consider acceptable. As said in the cover letter,
I think mid-term we ought to do away with xmalloc_bytes(). Hence
some alternative would be needed here anyway.

Jan