From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58639) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6zOA-0006p2-On for qemu-devel@nongnu.org; Thu, 10 Dec 2015 06:27:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a6zO6-0007Cn-LZ for qemu-devel@nongnu.org; Thu, 10 Dec 2015 06:27:58 -0500 Received: from mail-wm0-x22e.google.com ([2a00:1450:400c:c09::22e]:35430) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a6zO6-0007Cb-C8 for qemu-devel@nongnu.org; Thu, 10 Dec 2015 06:27:54 -0500 Received: by mail-wm0-x22e.google.com with SMTP id u63so19544349wmu.0 for ; Thu, 10 Dec 2015 03:27:54 -0800 (PST) References: <1449020831-8414-1-git-send-email-ehabkost@redhat.com> <5665D67F.2000001@gmail.com> <20151208175339.GM14490@thinpad.lan.raisama.net> From: Marcel Apfelbaum Message-ID: <566961B6.6020207@gmail.com> Date: Thu, 10 Dec 2015 13:27:50 +0200 MIME-Version: 1.0 In-Reply-To: <20151208175339.GM14490@thinpad.lan.raisama.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 00/16] pc: Eliminate struct PcGuestInfo Reply-To: marcel@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost , marcel@redhat.com Cc: Igor Mammedov , Marcel Apfelbaum , qemu-devel@nongnu.org, "Michael S. Tsirkin" 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