Re: [Qemu-devel] [PATCH 00/16] pc: Eliminate struct PcGuestInfo

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

On 12/02/2015 03:46 AM, Eduardo Habkost wrote:
> This moves all data from PcGuestInfo to either PCMachineState or
> PCMachineClass.
>
> This series depends on other two series:
> * [PATCH v3 0/6] pc: Initialization and compat function cleanup
> * [PATCH V3 0/3]  hw/pcie: Multi-root support for Q35
>
> For reference, there's a git tree containing this series plus all
> the dependencies, at:
>    git://github.com/ehabkost/qemu-hacks.git work/pcguestinfo-eliminate
>
> Eduardo Habkost (16):
>    pc: Move PcGuestInfo declaration to top of file
>    pc: Eliminate struct PcGuestInfoState
>    pc: Remove guest_info parameter from pc_memory_init()
>    acpi: Make acpi_setup() get PCMachineState as argument
>    acpi: Remove unused build_facs() PcGuestInfo paramter
>    acpi: Save PCMachineState on AcpiBuildState
>    acpi: Make acpi_build() get PCMachineState as argument
>    acpi: Make build_srat() get PCMachineState as argument
>    acpi: Remove ram size fields fron PcGuestInfo
>    pc: Move PcGuestInfo.fw_cfg field to PCMachineState
>    pc: Simplify signature of xen_load_linux()
>    pc: Remove PcGuestInfo.isapc_ram_fw field
>    q35: Remove MCHPCIState.guest_info field
>    acpi: Use PCMachineClass fields directly
>    pc: Move PcGuestInfo.apic_xrupt_override field to PCMachineState
>    pc: Move APIC and NUMA data from PcGuestInfo to PCMachineState

Hi,

I mainly agree with the removal of PcGuestInfo , I commented on some patches.

I do have a minor reservation, we kind of loose some information about the fields.
Until now it was pretty clear that the fields were related to guest because
they were part of PcGuestInfo. Now this information is lost and the fields
appear as yet other machine attributes.

I suppose this can be addressed by:
- a prefix for guest fields (e.g numa_nodes-> guest_numa_nodes),
- a comment in the class /* guest fields */,
- keeping the fields in PcGuestInfo struct but make the machine field short: guest so we can call machine->guest.numa_nodes
- or not be addressed at all :)


Thanks,
Marcel



>
>   hw/i386/acpi-build.c      | 75 ++++++++++++++++++++++++-----------------------
>   hw/i386/acpi-build.h      |  2 +-
>   hw/i386/pc.c              | 71 ++++++++++++++++++--------------------------
>   hw/i386/pc_piix.c         | 14 ++-------
>   hw/i386/pc_q35.c          | 15 ++--------
>   include/hw/i386/pc.h      | 30 +++++++------------
>   include/hw/pci-host/q35.h |  1 -
>   7 files changed, 82 insertions(+), 126 deletions(-)
>