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

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

[Alexey Kardashevskiy]

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).

This brings the capability back and puts SPAPR PHB into "bridge"
category.

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 <armbru@redhat.com>
Signed-off-by: Alexey Kardashevskiy <aik@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".
---
 hw/ppc/spapr_pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ec00300..66ddf10 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -728,6 +728,8 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
     dc->props = spapr_phb_properties;
     dc->reset = spapr_phb_reset;
     dc->vmsd = &vmstate_spapr_pci;
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    dc->cannot_instantiate_with_device_add_yet = false;
 }
 
 static const TypeInfo spapr_phb_info = {
-- 
1.8.4.rc4

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

[Alexander Graf]

On 13.01.2014, at 10:29, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> 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).
> 
> This brings the capability back and puts SPAPR PHB into "bridge"
> category.
> 
> 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 <armbru@redhat.com>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Thanks, applied to ppc-next.


Alex

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

[Markus Armbruster]

Alexey Kardashevskiy <aik@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 <armbru@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 <armbru@redhat.com>
    Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
    Signed-off-by: Andreas Färber <afaerber@suse.de>

Always good to point to specific commits in commit messages instead of
hand-waving "recent changes".

> 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.

> 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 <armbru@redhat.com>
> Signed-off-by: Alexey Kardashevskiy <aik@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.

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

[Alexey Kardashevskiy]

On 01/21/2014 02:27 AM, Markus Armbruster wrote:
> Alexey Kardashevskiy <aik@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 <armbru@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 <armbru@redhat.com>
>     Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
>     Signed-off-by: Andreas Färber <afaerber@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.

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 <armbru@redhat.com>
>> Signed-off-by: Alexey Kardashevskiy <aik@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.


-- 
Alexey

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

[Markus Armbruster]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 01/21/2014 02:27 AM, Markus Armbruster wrote:
>> Alexey Kardashevskiy <aik@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 <armbru@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 <armbru@redhat.com>
>>     Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>     Signed-off-by: Andreas Färber <afaerber@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.
>
> Sorry, I am not following you. SPAPR PHB allocates resources (memory
> regions...) as (for example) E1000 ethernet device does.

"Resources" was a poor choice of word.

I'm talking about connections to other devices.  An ordinary device on a
proper bus like PCI or USB gets all its connections via its bus.  The
"sysbus" doesn't provide any connection opportunities for its devices.
Instead, the connections are made by the code creating the device.

You tell me "-device spapr-pci-host-bridge" works (with your patch).
That either means it doesn't need any such connections, or it sets them
up itself somehow, or I'm missing something.  The first two would be
unusual, the latter not so much :)

>>> 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 <armbru@redhat.com>
>>> Signed-off-by: Alexey Kardashevskiy <aik@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.

IRC is ephemeral, the list is not.

> Alex Graf applied to his ppc-next, just to be clear - are you ack'ing or
> nack'ing this patch?

I'm certainly not vetoing anything here!

> 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.

Nothing, unless further discussion leads to action items.

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

[Alexey Kardashevskiy]

On 01/21/2014 06:56 PM, Markus Armbruster wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> On 01/21/2014 02:27 AM, Markus Armbruster wrote:
>>> Alexey Kardashevskiy <aik@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 <armbru@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 <armbru@redhat.com>
>>>     Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>>     Signed-off-by: Andreas Färber <afaerber@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.
>>
>> Sorry, I am not following you. SPAPR PHB allocates resources (memory
>> regions...) as (for example) E1000 ethernet device does.
> 
> "Resources" was a poor choice of word.
> 
> I'm talking about connections to other devices.  An ordinary device on a
> proper bus like PCI or USB gets all its connections via its bus.  The
> "sysbus" doesn't provide any connection opportunities for its devices.
> Instead, the connections are made by the code creating the device.
> 
> You tell me "-device spapr-pci-host-bridge" works (with your patch).
> That either means it doesn't need any such connections, or it sets them
> up itself somehow, or I'm missing something.  The first two would be
> unusual, the latter not so much :)


It maps itself (MMIO/IO windows) into the guest physical RAM at some
predefined addresses which are calculated from its @index property, and
that is pretty much it. I am not sure what else connections it might ever
need :) The rest is done via platform hyper- or rtas-calls, and this is
that connection I guess.



-- 
Alexey

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

[Andreas Färber]

Am 21.01.2014 02:37, schrieb Alexey Kardashevskiy:
> On 01/21/2014 02:27 AM, Markus Armbruster wrote:
>> Alexey Kardashevskiy <aik@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 <armbru@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 <armbru@redhat.com>
>>     Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>     Signed-off-by: Andreas Färber <afaerber@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 <armbru@redhat.com>
>>> Signed-off-by: Alexey Kardashevskiy <aik@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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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 <aik@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 <armbru@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 <armbru@redhat.com>
>>>     Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>>     Signed-off-by: Andreas Färber <afaerber@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 <armbru@redhat.com>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@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