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

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

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