Re: [Qemu-devel] [PATCH 02/16] pc: Eliminate struct PcGuestInfoState

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

On 12/02/2015 03:46 AM, Eduardo Habkost wrote:
> Instead of allocating a new struct just for PcGuestInfo and the
> mchine_done Notifier, place them inside PCMachineState.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/pc.c         | 27 ++++++++++-----------------
>   include/hw/i386/pc.h |  2 ++
>   2 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f32000a..30cdfaf 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1155,18 +1155,12 @@ typedef struct PcRomPciInfo {
>       uint64_t w64_max;
>   } PcRomPciInfo;
>
> -typedef struct PcGuestInfoState {
> -    PcGuestInfo info;
> -    Notifier machine_done;
> -} PcGuestInfoState;
> -
>   static
> -void pc_guest_info_machine_done(Notifier *notifier, void *data)
> +void pc_machine_done(Notifier *notifier, void *data)
>   {
> -    PcGuestInfoState *guest_info_state = container_of(notifier,
> -                                                      PcGuestInfoState,
> -                                                      machine_done);
> -    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
> +    PCMachineState *pcms = container_of(notifier,
> +                                        PCMachineState, machine_done);
> +    PCIBus *bus = pcms->bus;
>
>       if (bus) {
>           int extra_hosts = 0;
> @@ -1177,21 +1171,20 @@ void pc_guest_info_machine_done(Notifier *notifier, void *data)
>                   extra_hosts++;
>               }
>           }
> -        if (extra_hosts && guest_info_state->info.fw_cfg) {
> +        if (extra_hosts && pcms->acpi_guest_info.fw_cfg) {
>               uint64_t *val = g_malloc(sizeof(*val));
>               *val = cpu_to_le64(extra_hosts);
> -            fw_cfg_add_file(guest_info_state->info.fw_cfg,
> +            fw_cfg_add_file(pcms->acpi_guest_info.fw_cfg,
>                       "etc/extra-pci-roots", val, sizeof(*val));
>           }
>       }
>
> -    acpi_setup(&guest_info_state->info);
> +    acpi_setup(&pcms->acpi_guest_info);
>   }
>
>   PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
>   {
> -    PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
> -    PcGuestInfo *guest_info = &guest_info_state->info;
> +    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>       int i, j;
>
>       guest_info->ram_size_below_4g = pcms->below_4g_mem_size;
> @@ -1219,8 +1212,8 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
>           }
>       }
>
> -    guest_info_state->machine_done.notify = pc_guest_info_machine_done;
> -    qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> +    pcms->machine_done.notify = pc_machine_done;
> +    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>       return guest_info;
>   }
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index a74bded..61aa6ee 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -55,6 +55,8 @@ struct PCMachineState {
>       OnOffAuto smm;
>       ram_addr_t below_4g_mem_size, above_4g_mem_size;
>       PCIBus *bus;
> +    PcGuestInfo acpi_guest_info;
> +    Notifier machine_done;

Hi,

Arm's virt machine also has a  machine_done notifier
and a PcGuestInfo like structure (for building the ACPI).

Maybe it worth checking moving machine_done up to Machine
and investigate also the same possibility for a common
acpi guest info ancestor. For the moment all they have in
common is fw_config.

If anybody is looking for an adventure we can make a common
base class for ARM virt and PC machine, something like
ACPI_MACHINE. :)

Just a thought anyway,
Thanks,
Marcel

>   };
>
>   #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
>