The kexec() function of grub-pv is relying on the exact definition of
start_info from Mini-OS by having an "#undef start_info" and a few
lines later a copy of the Mini-OS definition again.

This is bad practice by making all attempts of Mini-OS to change that
definition impossible.

Avoid that dependency by moving the code fragment in question to the
very end of the source file, allowing to drop the copy of the
definition.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 stubdom/grub/kexec.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/stubdom/grub/kexec.c b/stubdom/grub/kexec.c
index 3da80b5b4a..2c426cc378 100644
--- a/stubdom/grub/kexec.c
+++ b/stubdom/grub/kexec.c
@@ -209,6 +209,8 @@ static void tpm_hash2pcr(struct xc_dom_image *dom, char *cmdline)
 	shutdown_tpmfront(tpm);
 }
 
+static void call_start_info_hook(struct xc_dom_image *dom);
+
 void kexec(void *kernel, long kernel_size, void *module, long module_size, char *cmdline, unsigned long flags)
 {
     struct xc_dom_image *dom;
@@ -330,10 +332,7 @@ void kexec(void *kernel, long kernel_size, void *module, long module_size, char
         }
 
     /* start info page */
-#undef start_info
-    if ( dom->arch_hooks->start_info )
-        dom->arch_hooks->start_info(dom);
-#define start_info (start_info_union.start_info)
+    call_start_info_hook(dom);
 
     xc_dom_log_memory_footprint(dom);
 
@@ -432,3 +431,10 @@ out:
     allocated = 0;
     xc_interface_close(xc_handle );
 }
+
+static void call_start_info_hook(struct xc_dom_image *dom)
+{
+#undef start_info
+    if ( dom->arch_hooks->start_info )
+        dom->arch_hooks->start_info(dom);
+}
-- 
2.43.0

On 25.06.2025 13:08, Juergen Gross wrote:
> @@ -432,3 +431,10 @@ out:
>      allocated = 0;
>      xc_interface_close(xc_handle );
>  }
> +
> +static void call_start_info_hook(struct xc_dom_image *dom)
> +{
> +#undef start_info
> +    if ( dom->arch_hooks->start_info )
> +        dom->arch_hooks->start_info(dom);
> +}

Maybe add a comment ahead of the function clarifying that it ought to remain
last?

Jan

On 25.06.25 16:20, Jan Beulich wrote:
> On 25.06.2025 13:08, Juergen Gross wrote:
>> @@ -432,3 +431,10 @@ out:
>>       allocated = 0;
>>       xc_interface_close(xc_handle );
>>   }
>> +
>> +static void call_start_info_hook(struct xc_dom_image *dom)
>> +{
>> +#undef start_info
>> +    if ( dom->arch_hooks->start_info )
>> +        dom->arch_hooks->start_info(dom);
>> +}
> 
> Maybe add a comment ahead of the function clarifying that it ought to remain
> last?

Good idea.

I'll add:

/* No references to start_info of Mini-OS after this function. */


Juergen

On 25/06/2025 3:28 pm, Jürgen Groß wrote:
> On 25.06.25 16:20, Jan Beulich wrote:
>> On 25.06.2025 13:08, Juergen Gross wrote:
>>> @@ -432,3 +431,10 @@ out:
>>>       allocated = 0;
>>>       xc_interface_close(xc_handle );
>>>   }
>>> +
>>> +static void call_start_info_hook(struct xc_dom_image *dom)
>>> +{
>>> +#undef start_info
>>> +    if ( dom->arch_hooks->start_info )
>>> +        dom->arch_hooks->start_info(dom);
>>> +}
>>
>> Maybe add a comment ahead of the function clarifying that it ought to
>> remain
>> last?
>
> Good idea.
>
> I'll add:
>
> /* No references to start_info of Mini-OS after this function. */

Given how few uses of start_info there actually are, can't you just drop
that piece of extreme obfuscation and make this work like regular C?

~Andrew

On 25.06.25 16:30, Andrew Cooper wrote:
> On 25/06/2025 3:28 pm, Jürgen Groß wrote:
>> On 25.06.25 16:20, Jan Beulich wrote:
>>> On 25.06.2025 13:08, Juergen Gross wrote:
>>>> @@ -432,3 +431,10 @@ out:
>>>>        allocated = 0;
>>>>        xc_interface_close(xc_handle );
>>>>    }
>>>> +
>>>> +static void call_start_info_hook(struct xc_dom_image *dom)
>>>> +{
>>>> +#undef start_info
>>>> +    if ( dom->arch_hooks->start_info )
>>>> +        dom->arch_hooks->start_info(dom);
>>>> +}
>>>
>>> Maybe add a comment ahead of the function clarifying that it ought to
>>> remain
>>> last?
>>
>> Good idea.
>>
>> I'll add:
>>
>> /* No references to start_info of Mini-OS after this function. */
> 
> Given how few uses of start_info there actually are, can't you just drop
> that piece of extreme obfuscation and make this work like regular C?

Hmm, you mean by using start_info_ptr directly?

This would be possible, but it would introduce a two-way dependency between
xen.git and minios.git.

Right now the build is basically fine, as Config.mk doesn't reference the
variant of Mini-OS breaking today's grub-pv.

Applying my current patch to grub-pv will still work with the "old" Mini-OS,
which then could be updated to a commit-id containing the related Mini-OS
patch defining start_info differently.

This update strategy would not result in a situation where the build is broken,
while modifying grub-pv to use start_info_ptr directly isn't possible without
either breaking the build in between (we'd need a Mini-OS update for that to
have "EXPORT_SYMBOL(start_info_ptr);" included), or by taking my current patch
as an intermediate step and only then switch grub-pv to use start_info_ptr.

Thoughts?


Juergen

On 2025-06-25 07:08, Juergen Gross wrote:
> The kexec() function of grub-pv is relying on the exact definition of
> start_info from Mini-OS by having an "#undef start_info" and a few
> lines later a copy of the Mini-OS definition again.
> 
> This is bad practice by making all attempts of Mini-OS to change that
> definition impossible.
> 
> Avoid that dependency by moving the code fragment in question to the
> very end of the source file, allowing to drop the copy of the
> definition.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>