From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50849) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W5awO-0006yS-Vv for qemu-devel@nongnu.org; Tue, 21 Jan 2014 08:00:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W5awE-00082G-HO for qemu-devel@nongnu.org; Tue, 21 Jan 2014 08:00:28 -0500 Received: from mail-pd0-f173.google.com ([209.85.192.173]:62381) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W5awE-00081g-6j for qemu-devel@nongnu.org; Tue, 21 Jan 2014 08:00:18 -0500 Received: by mail-pd0-f173.google.com with SMTP id y10so4598386pdj.4 for ; Tue, 21 Jan 2014 05:00:17 -0800 (PST) Message-ID: <52DE6F5B.4050107@ozlabs.ru> Date: Wed, 22 Jan 2014 00:00:11 +1100 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1389605349-1989-1-git-send-email-aik@ozlabs.ru> <87a9eqbrn0.fsf@blackfin.pond.sub.org> <52DDCF46.7030209@ozlabs.ru> <52DE49AE.3040400@suse.de> In-Reply-To: <52DE49AE.3040400@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH] spapr-pci: enable adding PHB via -device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: Alexander Graf , qemu-ppc@nongnu.org, Markus Armbruster , qemu-devel@nongnu.org On 01/21/2014 09:19 PM, Andreas Färber wrote: > Am 21.01.2014 02:37, schrieb Alexey Kardashevskiy: >> On 01/21/2014 02:27 AM, Markus Armbruster wrote: >>> Alexey Kardashevskiy writes: >>> >>>> Recent changes introduced cannot_instantiate_with_device_add_yet >>>> and removed capability of adding yet another PCI host bridge via >>>> command line for SPAPR platform (POWERPC64 server). >>> >>> Specifically: >>> >>> commit 837d37167dc446af8a91189108b363c04609e296 >>> Author: Markus Armbruster >>> Date: Thu Nov 28 17:26:55 2013 +0100 >>> >>> sysbus: Set cannot_instantiate_with_device_add_yet >>> >>> device_add plugs devices into suitable bus. For "real" buses, that >>> actually connects the device. For sysbus, the connections need to be >>> made separately, and device_add can't do that. The device would be >>> left unconnected, and could not possibly work. >>> >>> Quite a few, but not all sysbus devices already set >>> cannot_instantiate_with_device_add_yet in their class init function. >>> >>> Set it in their abstract base's class init function >>> sysbus_device_class_init(), and remove the now redundant assignments >>> from device class init functions. >>> >>> Signed-off-by: Markus Armbruster >>> Reviewed-by: Marcel Apfelbaum >>> Signed-off-by: Andreas Färber >>> >>> Always good to point to specific commits in commit messages instead of >>> hand-waving "recent changes". >> >> >> My bad, I'll do this next time. Just lost myself in that patch series. >> >> >>>> This brings the capability back and puts SPAPR PHB into "bridge" >>>> category. >>> >>> Look, a sysbus device that grabs the resources it needs from its init() >>> callback instead of getting connected to them by the code that creates >>> it! I'm not sure that's proper, but if it works... Maybe Andreas >>> (cc'ed) can advise. > > I did point out that SysBus devices may do that, but I considered it > more helpful to enforce the rule than the exception to the rule. > Overriding the flag in those devices is the right thing to do. > >> Sorry, I am not following you. SPAPR PHB allocates resources (memory >> regions...) as (for example) E1000 ethernet device does. >> >> >>>> This is not much use for emulated PHB but it is absolutely required >>>> for VFIO as we put an IOMMU group onto a separate PHB on SPAPR. >>>> >>>> Cc: Markus Armbruster >>>> Signed-off-by: Alexey Kardashevskiy >>>> --- >>>> >>>> Are -device and device_add considered synonims? SPAPR PHB can be added >>>> via the command line just fine but cannot from device_add as >>>> "Bus 'main-system-bus' does not support hotplugging". >>> >>> -device is cold plug, device_add is hot plug. device_add could be >>> improved to do cold plug when used before the machine starts. >> >> >> Soooo? We figured that out on IRC :) At the moment it is regression - >> -device used to work for PHB and now it does not. >> >> Alex Graf applied to his ppc-next, just to be clear - are you ack'ing or >> nack'ing this patch? >> >> In any cace, what do you think I should change in what I do in spapr_pci.c? >> I most probably will, just need some directions. > > You should add a test case. :) A tests/spapr_phb-test.c exercising this > -device usage as part of `make check` would be a good start and would've > avoided regressions in the first place - predates your time obviously. > And once you have that in place your next step would be to compare what > has been implemented for the x86 PHB to actually test PCI devices on it. > > Regards, > Andreas > Like this? Fails without discussed patch and works with it, yeah. #include "libqtest.h" #include #define TYPE_SPAPR_PCI_HOST_BRIDGE "spapr-pci-host-bridge" static void test_phb_device(void) { qtest_start("-device " TYPE_SPAPR_PCI_HOST_BRIDGE ",index=100"); qtest_end(); } int main(int argc, char **argv) { g_test_init(&argc, &argv, NULL); qtest_add_func("/qmp/phb_device", test_phb_device); return g_test_run(); } -- Alexey