On 12/08/2015 07:53 PM, Eduardo Habkost wrote:
On Mon, Dec 07, 2015 at 08:57:03PM +0200, Marcel Apfelbaum wrote:
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.
But they really are just machine attributes, aren't they?
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 :)
I don't see your point. Could you explain what you mean by
"related to the guest" and "guest fields"?
They are just machine attributes, and they happen to be used as
input when building ACPI tables (just like other machine
attributes are used as input for other guest-visible data, like
CPUID, SMBIOS, and other tables). What exactly make them "related
to guest"?
Maybe I wasn't clear indeed, let me try again please.
I (personally) don't like structures with a lot of not related fields.
The reason is, it will be very hard for someone reading the code to understand
the use
of each field => a global code query will be necessary, *exactly* like a global
variable.
(given that a machine is also one per system)
I do understand that sometimes, machine class included, there is a need for a
lot of fields.
What I am suggesting is grouping the fields by their purpose/subsystem.
If "guest visible" does not do the trick, maybe other logical partition can be
made.
For example (this is only an example): acpi fields, cpu fields, ...
In this way the code reviewer can understand with a quick look what are the
"parts" of a machine
and where are used.
Since the (very good!) re-factoring you are doing makes the code less complex
by removing an
unnecessary artifact (PcGuestInfo), I wouldn't want to miss the opportunity to
point to
another code complexity we may get into.
Thanks,
Marcel
