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 <a...@ozlabs.ru> 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 <arm...@redhat.com>
>>> 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 <arm...@redhat.com>
>>> Reviewed-by: Marcel Apfelbaum <marce...@redhat.com>
>>> Signed-off-by: Andreas Färber <afaer...@suse.de>
>>>
>>> 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 <arm...@redhat.com>
>>>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>>>> ---
>>>>
>>>> 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 <glib.h>
#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
