Re: [PATCH] mini-os: Use a single start_info_ptr variable

[Costin Lupu <costin.lupu@xxxxxxxxx>]

On 8/19/20 4:12 PM, Samuel Thibault wrote:
> Costin Lupu, le mer. 19 août 2020 16:01:18 +0300, a ecrit:
>> I guess we should move it to an arch-independent file. One example is
>> kernel.c where we have a similar scenario for `cmdline`, which is also
>> set in `arch/x86/setup.c`.
> 
> Indeed.
> 
>> And in `init_shutdown` it would be set only
>> if it wasn't set before, i.e. start_info_ptr == 0 (which would happen
>> for ARM), or otherwise we would check that `si` parameter has the same
>> value as `start_info_ptr` (which would happen for x86).
> 
> I would say not to bother setting it again. You can actually remove the
> parameter of init_shutdown, and of start_kernel as well.
> 892b661de6d79a768eb7def9489a80f0c7289f42 added, but without taking care
> of arm. Arm porters will have to fix the HYPERVISOR_suspend() call
> anyway, and possibly they don't even need start_info_ptr in their case,
> so letting it to be NULL will be fine, if arm needs it at some point,
> they will set it up in their setup.c

After looking on that commit I remembered that it tried to port the
changes from NEC's mini-os fork [1], which was intended to work only on
x86 (the fork doesn't have an `arch/arm` directory). Moreover,
start_info structure is defined only for PV guests in `xen.h`, so
because of this I think it makes more sense to keep it in
`arch/x86/setup.c`.

The problem that remains is that the logic in `shutdown.c`, which should
be arch-independent, uses start_info_ptr but it shouldn't. We can fix
this by adding an `arch_suspend()` function which would call the
hypercall on x86 and would be empty on ARM.

However, I tried to build mini-os for ARM but I couldn't. The command I
used on Debian is:

$ make -j32 CROSS_COMPILE=arm-linux-gnueabihf- MINIOS_TARGET_ARCH=arm
Config.mk:99: arch/arm/arch.mk: No such file or directory

It looks like the ARM build has bigger problems. :-) Anyway, for the
patch I will send I'm gonna leave the HYPERVISOR_suspend() hypercall in
shutdown.c as you advised.


[1] https://github.com/sysml/mini-os

Cheers,
Costin