From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60021) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3o8H-0004WF-G1 for qemu-devel@nongnu.org; Tue, 01 Dec 2015 11:50:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3o8C-0007cP-CW for qemu-devel@nongnu.org; Tue, 01 Dec 2015 11:50:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52250) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3o8C-0007cI-4o for qemu-devel@nongnu.org; Tue, 01 Dec 2015 11:50:20 -0500 References: <1448553628-5446-1-git-send-email-marcel@redhat.com> <1448553628-5446-4-git-send-email-marcel@redhat.com> <20151127172852.GZ23717@thinpad.lan.raisama.net> <565ABB4B.9080703@redhat.com> <20151130150733.GG23717@thinpad.lan.raisama.net> <565DA9A5.8010408@redhat.com> <20151201144816.GI23717@thinpad.lan.raisama.net> <565DB4FD.7050909@redhat.com> <20151201150911.GJ23717@thinpad.lan.raisama.net> From: Marcel Apfelbaum Message-ID: <565DCFC7.8060603@redhat.com> Date: Tue, 1 Dec 2015 18:50:15 +0200 MIME-Version: 1.0 In-Reply-To: <20151201150911.GJ23717@thinpad.lan.raisama.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V3 3/3] hw/i386: extend pxb query for all PC machines List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: mst@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, kraxel@redhat.com, laine@redhat.com, pbonzini@redhat.com, imammedo@redhat.com, lersek@redhat.com, rth@twiddle.net On 12/01/2015 05:09 PM, Eduardo Habkost wrote: > On Tue, Dec 01, 2015 at 04:55:57PM +0200, Marcel Apfelbaum wrote: >> On 12/01/2015 04:48 PM, Eduardo Habkost wrote: >>> On Tue, Dec 01, 2015 at 04:07:33PM +0200, Marcel Apfelbaum wrote: >>>> On 11/30/2015 05:07 PM, Eduardo Habkost wrote: >>>>> On Sun, Nov 29, 2015 at 10:46:03AM +0200, Marcel Apfelbaum wrote: >>>>>> On 11/27/2015 07:28 PM, Eduardo Habkost wrote: >>>>>>> On Thu, Nov 26, 2015 at 06:00:28PM +0200, Marcel Apfelbaum wrote: >>>>>>>> Add bus property to PC machines and use it when looking >>>>>>>> for primary PCI root bus (bus 0). >>>>>>>> >>>>>>>> Signed-off-by: Marcel Apfelbaum >>>>>>> >>>>>>> I can't pretend I have reviewed the q35 part, but the changes are >>>>>>> an improvement to the existing code that depended on >>>>>>> find_i440fx(). >>>>>>> >>>>>>> Acked-by: Eduardo Habkost >>>>>> >>>>>> Thanks! >>>>>> >>>>>>> >>>>>>> BTW, what's missing to allow us to change acpi_set_pci_info() to >>>>>>> use PCMachine::bus instead of find_i440fx(), too? How much of the >>>>>>> PCI hotplug stuff is different in q35? >>>>>> >>>>>> It is pretty different. >>>>>> i440fx has acpi based hotplug while q35 has PCIe native hotplug. Since is >>>>>> "native", no acpi info is necessary. >>>>>> >>>>>> Having said that, if we have an PCIe-PCI bridge, the pci devices behind it >>>>>> cannot be hotplugged/unplugged right now. >>>>>> >>>>>> Once we decide to add hotplug support for this scenario, maybe we can get rid of >>>>>> find_i440fx(). >>>>> >>>>> Thanks for the explanation. I wonder if there's a better way to >>>>> check if ACPI-based hotplug is needed by looking at >>>>> PCMachineState or PCIBus, so we don't couple the ACPI code to >>>>> piix.c. >>>>> >>>> >>>> I suppose we can do something about it, like adding a property to PCMachineState, >>>> lets say bool acpi_hotplug and set it false for Q35. >>>> >>>> Then we have: >>>> pcm = PC_MACHINE(current_machine); >>>> if(pcm->acpi_hotplug) { >>>> bus = pcm->bus; >>>> ... >>>> } >>>> >>>> Sounds acceptable? If yes, I'll send a patch on top since is not directly related.S >>> >>> There's no existing field or method in PCIBus that can be already >>> used for that? >> >> Hmm, you can derive the info you need from pci_bus_is_express. >> If express-> no acpi_hotplug. This is not 100% true, but since >> we don't support acpi hotplug on PCIe machines, it should be OK for now. > > What about just checking if AcpiPmInfo.pcihp_io_base is set? > Because this contradicts the "do not probe for piix" requirement. pcihp_io_base depends on piix query for pm (piix4_pm_find). So pcihp_io_base is an i440fx only "artifact". Also, acpi_set_pci_info is called before acpi_build that populates acpi_get_pm_info. All of that can be taken care of, of course. At the end of the day, as long as the functionality is preserved, I personally have no objection in re-factoring. Thanks, Marcel