Re: [PATCH] spapr: Don't hijack current_machine->boot_order

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3383 bytes --]

On Fri, May 21, 2021 at 06:07:35PM +0200, Greg Kurz wrote:
> QEMU 6.0 moved all the -boot variables to the machine. Especially, the
> removal of the boot_order static changed the handling of '-boot once'
> from:
> 
>     if (boot_once) {
>         qemu_boot_set(boot_once, &error_fatal);
>         qemu_register_reset(restore_boot_order, g_strdup(boot_order));
>     }
> 
> to
> 
>     if (current_machine->boot_once) {
>         qemu_boot_set(current_machine->boot_once, &error_fatal);
>         qemu_register_reset(restore_boot_order,
>                             g_strdup(current_machine->boot_order));
>     }
> 
> This means that we now register as subsequent boot order a copy
> of current_machine->boot_once that was just set with the previous
> call to qemu_boot_set(), i.e. we never transition away from the
> once boot order.
> 
> It is certainly fragile^Wwrong for the spapr code to hijack a
> field of the base machine type object like that. The boot order
> rework simply turned this software boundary violation into an
> actual bug.
> 
> Have the spapr code to handle that with its own field in
> SpaprMachineState. Also kfree() the initial boot device
> string when "once" was used.
> 
> Fixes: 4b7acd2ac821 ("vl: clean up -boot variables")
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1960119
> Cc: pbonzini@redhat.com
> Signed-off-by: Greg Kurz <groug@kaod.org>


Applied to ppc-for-6.1, thanks.

> ---
>  include/hw/ppc/spapr.h | 3 +++
>  hw/ppc/spapr.c         | 8 +++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index bbf817af4647..f05219f75ef6 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -223,6 +223,9 @@ struct SpaprMachineState {
>      int fwnmi_machine_check_interlock;
>      QemuCond fwnmi_machine_check_interlock_cond;
>  
> +    /* Set by -boot */
> +    char *boot_device;
> +
>      /*< public >*/
>      char *kvm_type;
>      char *host_model;
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c23bcc449071..4dd90b75cc52 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1005,7 +1005,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
>      _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
>  
>      if (reset) {
> -        const char *boot_device = machine->boot_order;
> +        const char *boot_device = spapr->boot_device;
>          char *stdout_path = spapr_vio_stdout_path(spapr->vio_bus);
>          size_t cb = 0;
>          char *bootlist = get_boot_devices_list(&cb);
> @@ -2376,8 +2376,10 @@ static SaveVMHandlers savevm_htab_handlers = {
>  static void spapr_boot_set(void *opaque, const char *boot_device,
>                             Error **errp)
>  {
> -    MachineState *machine = MACHINE(opaque);
> -    machine->boot_order = g_strdup(boot_device);
> +    SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
> +
> +    g_free(spapr->boot_device);
> +    spapr->boot_device = g_strdup(boot_device);
>  }
>  
>  static void spapr_create_lmb_dr_connectors(SpaprMachineState *spapr)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]