Re: [PATCH 4/6] hw/hppa: Introduce HppaMachineState::boot_info::firmware structure

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 9/10/25 19:20, Richard Henderson wrote:
> On 10/9/25 07:31, Philippe Mathieu-Daudé wrote:
>> Current code uses CPUHPPAState::@kernel_entry to hold either:
>>   - kernel entry virtual address
>>   - firmware interactive mode
>> and CPUHPPAState::@cmdline_or_bootorder to hold either:
>>   - kernel &cmdline physical address
>>   - firmware boot order
>>
>> Besides, these variables don't belong to CPUHPPAState, they
>> depend on how the machine is started, and only apply to the
>> first CPU.
>>
>> Introduce firmware specific fields in HppaMachineState, using
>> their correct type. Introduce the @is_kernel field, to allow
>> distinguishing between firmware or kernel mode.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/hppa/machine.c | 36 ++++++++++++++++++++++++++++--------
>>   1 file changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
>> index a2996ef7682..138cd97efd9 100644
>> --- a/hw/hppa/machine.c
>> +++ b/hw/hppa/machine.c
>> @@ -41,6 +41,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(HppaMachineState, 
>> HPPA_COMMON_MACHINE)
>>   struct HppaMachineState {
>>       MachineState parent_obj;
>> +
>> +    struct {
>> +        bool is_kernel; /* kernel:1 firmware:0 */
>> +        union {
>> +            struct {
>> +                char bootorder;
>> +                bool interactive_mode;
>> +            } firmware;
>> +        };
>> +    } boot_info;
> 
> I think this is more complicated than necessary.

Certainly...

>> @@ -481,6 +492,7 @@ static void 
>> machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
>>                         "-0x%08" PRIx64 ", entry at 0x%08" PRIx64
>>                         ", size %" PRIu64 " kB\n",
>>                         kernel_low, kernel_high, kernel_entry, size / 
>> KiB);
>> +        hms->boot_info.is_kernel = true;
>>           if (kernel_cmdline) {
>>               cpu[0]->env.cmdline_or_bootorder = 0x4000;
>> @@ -519,13 +531,15 @@ static void 
>> machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
>>           }
>>       }
>> -    if (!kernel_entry) {
>> +    if (!hms->boot_info.is_kernel) {
>>           /* When booting via firmware, tell firmware if we want 
>> interactive
>> -         * mode (kernel_entry=1), and to boot from CD 
>> (cmdline_or_bootorder='d')
>> -         * or hard disc (cmdline_or_bootorder='c').
>> +         * mode (interactive_mode=1), and to boot from CD 
>> (bootorder='d')
>> +         * or hard disc (bootorder='c').
>>            */
>> -        kernel_entry = machine->boot_config.has_menu ? machine- 
>> >boot_config.menu : 0;
>> -        cpu[0]->env.cmdline_or_bootorder = machine- 
>> >boot_config.order[0];
>> +        hms->boot_info.firmware.interactive_mode = machine- 
>> >boot_config.has_menu
>> +                                                  ? machine- 
>> >boot_config.menu
>> +                                                  : 0;
>> +        hms->boot_info.firmware.bootorder = machine- 
>> >boot_config.order[0];
> ...
>> @@ -668,8 +683,12 @@ static void hppa_machine_reset(MachineState *ms, 
>> ResetType type)
>>       }
>>       cpu[0]->env.gr[26] = ms->ram_size;
>> -    cpu[0]->env.gr[25] = cpu[0]->env.kernel_entry;
>> -    cpu[0]->env.gr[24] = cpu[0]->env.cmdline_or_bootorder;
>> +    cpu[0]->env.gr[25] = hms->boot_info.is_kernel
>> +                         ? cpu[0]->env.kernel_entry
>> +                         : hms->boot_info.firmware.interactive_mode;
>> +    cpu[0]->env.gr[24] = hms->boot_info.is_kernel
>> +                         ? cpu[0]->env.cmdline_or_bootorder
>> +                         : hms->boot_info.firmware.bootorder;
> 
> 
> I think you'd be better served by placing
> 
>      uint64_t boot_gr24;
>      uint64_t boot_gr25;

Clever :)

> in HppaMachineState and set them in machine_HP_common_init_tail.
> 
>> @@ -679,7 +698,8 @@ static void hppa_machine_reset(MachineState *ms, 
>> ResetType type)
>>       cpu[0]->env.kernel_entry = 0;
>>       cpu[0]->env.initrd_base = 0;
>>       cpu[0]->env.initrd_end = 0;
>> -    cpu[0]->env.cmdline_or_bootorder = mc->default_boot_order[0];
>> +    cpu[0]->env.cmdline_or_bootorder = 0;
>> +    hms->boot_info.firmware.bootorder = mc->default_boot_order[0];
> 
> Setting hms in reset seems wrong.

Yes, a pre-existing issue. I couldn't figure that piece of code (and
comment) myself.