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
