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

[Marcel Apfelbaum]

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