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

[Eduardo Habkost]

On Thu, Dec 10, 2015 at 01:27:50PM +0200, Marcel Apfelbaum wrote:
> 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, ...

I see. I believe "guest fields" wouldn't be a clear partitioning,
but grouping CPU/RAM/NUMA/ACPI fields would be nice.

I will send a new version of this series to implement the
qdev_get_machine() suggestion, and try to group and document
related fields in PCMachineState/PCMachienClass.

> 
> 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.

Yes, it does makes sense.

-- 
Eduardo