Re: [Qemu-devel] [PATCH] spapr-pci: enable adding PHB via -device

[Alexey Kardashevskiy]

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 <address@hidden> 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 <address@hidden>
>>> 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 <address@hidden>
>>>     Reviewed-by: Marcel Apfelbaum <address@hidden>
>>>     Signed-off-by: Andreas Färber <address@hidden>
>>>
>>> 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 <address@hidden>
>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>> ---
>>>>
>>>> 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