Re: [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines

[Marcel Apfelbaum <marcel@redhat.com>]

On 03/23/2015 07:05 PM, Paolo Bonzini wrote:
> Capture the explicit setting of "usb=no" into a separate bool, and
> use it to skip the update of machine->usb in the board init function.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/core/machine.c     | 1 +
>   hw/ppc/mac_newworld.c | 2 +-
>   hw/ppc/spapr.c        | 2 +-
>   include/hw/boards.h   | 1 +
>   4 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index cb1185a..25c45e6 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -223,6 +223,7 @@ static void machine_set_usb(Object *obj, bool value, Error **errp)
>       MachineState *ms = MACHINE(obj);
>
>       ms->usb = value;
> +    ms->usb_disabled = !value;
Maybe is too late now, but I really not like this pollution of MachineState
with 'usb_disabled'. (Imagine we have this kind of fields for lots of objects and lots
of corner cases...)
I know it comes to solve a bug, but we talked about it in another mail thread and
this change in semantics was approved.

Let me explain *why* I don't like it.
1. We add an "usb_disabled" field to a base class (actually object)
    of all the machines and the only place it is interesting is
    for 2 machines on ppc.
2. Even for these 2 machines, the scenario of defaults=on and usb=off
    is not practical.

I hope I helped,
Thanks,
Marcel

P.S. If you still want it there, maybe move it to a ppc base class?

>   }
>
>   static char *machine_get_firmware(Object *obj, Error **errp)
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index e0397bc..a365bf9 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -371,7 +371,7 @@ static void ppc_core99_init(MachineState *machine)
>           /* 970 gets a U3 bus */
>           pci_bus = pci_pmac_u3_init(pic, get_system_memory(), get_system_io());
>           machine_arch = ARCH_MAC99_U3;
> -        machine->usb |= defaults_enabled();
> +        machine->usb |= defaults_enabled() && !machine->usb_disabled;
>       } else {
>           pci_bus = pci_pmac_init(pic, get_system_memory(), get_system_io());
>           machine_arch = ARCH_MAC99;
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0487f52..dd5e101 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1563,7 +1563,7 @@ static void ppc_spapr_init(MachineState *machine)
>       /* Graphics */
>       if (spapr_vga_init(phb->bus)) {
>           spapr->has_graphics = true;
> -        machine->usb |= defaults_enabled();
> +        machine->usb |= defaults_enabled() && !machine->usb_disabled;
>       }
>
>       if (machine->usb) {
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 1feea2b..be6a0ed 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -141,6 +141,7 @@ struct MachineState {
>       bool dump_guest_core;
>       bool mem_merge;
>       bool usb;
> +    bool usb_disabled;
>       char *firmware;
>       bool iommu;
>       bool suppress_vmdesc;
>