[Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Cornelia Huck]

info qom-tree shows several devices under unattached that probably
should go somewhere.

The css bridge should attach to the machine, as it has a similar
purpose as e.g. a pci host bridge.

The autogenerated network devices should be in the same bucket as any
other device; I'm just not sure about the way I went about it.

The zpci devices are still problematic: I don't have a good idea where
they should show up.

Remaining in the unattached container are the sysbus, memory regions
and cpus.

Cornelia Huck (2):
  s390x/css: attach css bridge
  s390x: attach autogenerated nics

 hw/s390x/css-bridge.c      | 2 ++
 hw/s390x/s390-virtio-ccw.c | 2 ++
 2 files changed, 4 insertions(+)

-- 
2.13.6

[Qemu-devel] [PATCH RFC 1/2] s390x/css: attach css bridge

[Cornelia Huck]

Logically, the css bridge should be attached to the machine.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/css-bridge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
index c4a9735d71..84d33eafc2 100644
--- a/hw/s390x/css-bridge.c
+++ b/hw/s390x/css-bridge.c
@@ -99,6 +99,8 @@ VirtualCssBus *virtual_css_bus_init(void)
 
     /* Create bridge device */
     dev = qdev_create(NULL, TYPE_VIRTUAL_CSS_BRIDGE);
+    object_property_add_child(qdev_get_machine(), TYPE_VIRTUAL_CSS_BRIDGE,
+                              OBJECT(dev), NULL);
     qdev_init_nofail(dev);
 
     /* Create bus on bridge device */
-- 
2.13.6

[Qemu-devel] [PATCH RFC 2/2] s390x: attach autogenerated nics

[Cornelia Huck]

The autogenerated nics should be treated as any other device; use
qdev_set_id() to have them show up under peripheral-anon.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index a23b8aec9f..830bae9d0f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -35,6 +35,7 @@
 #include "cpu_models.h"
 #include "qapi/qmp/qerror.h"
 #include "hw/nmi.h"
+#include "include/monitor/qdev.h"
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
@@ -259,6 +260,7 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
 
         dev = qdev_create(bus, name);
         qdev_set_nic_properties(dev, nd);
+        qdev_set_id(dev, NULL);
         qdev_init_nofail(dev);
     }
 }
-- 
2.13.6

Re: [Qemu-devel] [PATCH RFC 1/2] s390x/css: attach css bridge

[Christian Borntraeger]

Looks good to me and is similar to the other devices (e.g. the IPL device)

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>



On 11/28/2017 02:46 PM, Cornelia Huck wrote:
> Logically, the css bridge should be attached to the machine.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/css-bridge.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
> index c4a9735d71..84d33eafc2 100644
> --- a/hw/s390x/css-bridge.c
> +++ b/hw/s390x/css-bridge.c
> @@ -99,6 +99,8 @@ VirtualCssBus *virtual_css_bus_init(void)
> 
>      /* Create bridge device */
>      dev = qdev_create(NULL, TYPE_VIRTUAL_CSS_BRIDGE);
> +    object_property_add_child(qdev_get_machine(), TYPE_VIRTUAL_CSS_BRIDGE,
> +                              OBJECT(dev), NULL);
>      qdev_init_nofail(dev);
> 
>      /* Create bus on bridge device */
> 

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Halil Pasic]

On 11/28/2017 02:46 PM, Cornelia Huck wrote:
> info qom-tree shows several devices under unattached that probably
> should go somewhere.

I think this was reported by me. See 
MID: <76f95c6f-641e-2fe0-73b4-3ab24fc1a93f@linux.vnet.ibm.com>

I would not mind a Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>.
Or do you see this differently?

If these are bugs. I would prefer commit messages and titles reflecting
this fact.

Otherwise at first glance both patches seem sane.

Regards,
Halil

> 
> The css bridge should attach to the machine, as it has a similar
> purpose as e.g. a pci host bridge.
> 
> The autogenerated network devices should be in the same bucket as any
> other device; I'm just not sure about the way I went about it.
> 
> The zpci devices are still problematic: I don't have a good idea where
> they should show up.
> 
> Remaining in the unattached container are the sysbus, memory regions
> and cpus.
> 
> Cornelia Huck (2):
>   s390x/css: attach css bridge
>   s390x: attach autogenerated nics
> 
>  hw/s390x/css-bridge.c      | 2 ++
>  hw/s390x/s390-virtio-ccw.c | 2 ++
>  2 files changed, 4 insertions(+)
> 

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Cornelia Huck]

On Tue, 28 Nov 2017 15:17:38 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 11/28/2017 02:46 PM, Cornelia Huck wrote:
> > info qom-tree shows several devices under unattached that probably
> > should go somewhere.  
> 
> I think this was reported by me. See 
> MID: <76f95c6f-641e-2fe0-73b4-3ab24fc1a93f@linux.vnet.ibm.com>
> 
> I would not mind a Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>.
> Or do you see this differently?

Apparently I forgot to add it to the first one. Will do.

> 
> If these are bugs. I would prefer commit messages and titles reflecting
> this fact.

I don't see this as fixing bugs, more removing oddities.

> 
> Otherwise at first glance both patches seem sane.

Can I count this as an ack, or do you plan to do more review?

> 
> Regards,
> Halil
> 
> > 
> > The css bridge should attach to the machine, as it has a similar
> > purpose as e.g. a pci host bridge.
> > 
> > The autogenerated network devices should be in the same bucket as any
> > other device; I'm just not sure about the way I went about it.
> > 
> > The zpci devices are still problematic: I don't have a good idea where
> > they should show up.
> > 
> > Remaining in the unattached container are the sysbus, memory regions
> > and cpus.
> > 
> > Cornelia Huck (2):
> >   s390x/css: attach css bridge
> >   s390x: attach autogenerated nics
> > 
> >  hw/s390x/css-bridge.c      | 2 ++
> >  hw/s390x/s390-virtio-ccw.c | 2 ++
> >  2 files changed, 4 insertions(+)
> >   
> 

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Halil Pasic]

On 11/28/2017 03:27 PM, Cornelia Huck wrote:
> On Tue, 28 Nov 2017 15:17:38 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 11/28/2017 02:46 PM, Cornelia Huck wrote:
>>> info qom-tree shows several devices under unattached that probably
>>> should go somewhere.  
>>
>> I think this was reported by me. See 
>> MID: <76f95c6f-641e-2fe0-73b4-3ab24fc1a93f@linux.vnet.ibm.com>
>>
>> I would not mind a Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>.
>> Or do you see this differently?
> 
> Apparently I forgot to add it to the first one. Will do.

np

It's just that I've spent quite some time writing that email and
identifying the oddities.

> 
>>
>> If these are bugs. I would prefer commit messages and titles reflecting
>> this fact.
> 
> I don't see this as fixing bugs, more removing oddities.
> 
>>
>> Otherwise at first glance both patches seem sane.
> 
> Can I count this as an ack, or do you plan to do more review?
> 

Yes I was planning to give it another look. And I do already
have questions. Isn't the QOM composition tree API? I mean
let's assume the QMP commands working on this tree are not completely
useless. How is client code (management software) supposed to work,
assumed it can rely on paths of e.g. properties being stable. Just
imagine we had this default-cssid property (for the sake of the
argument, not like we want it) on the css bridge.

Now if the composition tree is API then these can only be bug fixes
(IMHO).

There are also other oddities I've spotted. My idea was to put
this composition tree discussion on hold until the vfio-ccw stuff
is sorted out. I would certainly like to build a better understanding.

Halil

>>
>> Regards,
>> Halil
>>
>>>
>>> The css bridge should attach to the machine, as it has a similar
>>> purpose as e.g. a pci host bridge.
>>>
>>> The autogenerated network devices should be in the same bucket as any
>>> other device; I'm just not sure about the way I went about it.
>>>
>>> The zpci devices are still problematic: I don't have a good idea where
>>> they should show up.
>>>
>>> Remaining in the unattached container are the sysbus, memory regions
>>> and cpus.
>>>
>>> Cornelia Huck (2):
>>>   s390x/css: attach css bridge
>>>   s390x: attach autogenerated nics
>>>
>>>  hw/s390x/css-bridge.c      | 2 ++
>>>  hw/s390x/s390-virtio-ccw.c | 2 ++
>>>  2 files changed, 4 insertions(+)
>>>   
>>
> 
> 

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Halil Pasic]

On 11/28/2017 04:21 PM, Halil Pasic wrote:
[..]
>>> Otherwise at first glance both patches seem sane.
>>
>> Can I count this as an ack, or do you plan to do more review?
>>
> 
> Yes I was planning to give it another look. And I do already
> have questions. Isn't the QOM composition tree API? I mean
> let's assume the QMP commands working on this tree are not completely
> useless. How is client code (management software) supposed to work,
> assumed it can rely on paths of e.g. properties being stable. Just
> imagine we had this default-cssid property (for the sake of the
> argument, not like we want it) on the css bridge.

Ping! I would like to get this clarified before proceeding with reviewing
this series.

> 
> Now if the composition tree is API then these can only be bug fixes
> (IMHO).
> 
> There are also other oddities I've spotted. My idea was to put
> this composition tree discussion on hold until the vfio-ccw stuff
> is sorted out. I would certainly like to build a better understanding.
> 
> Halil
> 

[..]

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Cornelia Huck]

On Fri, 1 Dec 2017 15:41:21 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 11/28/2017 04:21 PM, Halil Pasic wrote:
> [..]
> >>> Otherwise at first glance both patches seem sane.  
> >>
> >> Can I count this as an ack, or do you plan to do more review?
> >>  
> > 
> > Yes I was planning to give it another look. And I do already
> > have questions. Isn't the QOM composition tree API? I mean
> > let's assume the QMP commands working on this tree are not completely
> > useless. How is client code (management software) supposed to work,
> > assumed it can rely on paths of e.g. properties being stable. Just
> > imagine we had this default-cssid property (for the sake of the
> > argument, not like we want it) on the css bridge.  
> 
> Ping! I would like to get this clarified before proceeding with reviewing
> this series.

[It might be helpful to not drop cc:s.]

I don't think we really want a static tree. As long as the devices are
locateable, it should be fine.

> 
> > 
> > Now if the composition tree is API then these can only be bug fixes
> > (IMHO).
> > 
> > There are also other oddities I've spotted. My idea was to put
> > this composition tree discussion on hold until the vfio-ccw stuff
> > is sorted out. I would certainly like to build a better understanding.
> > 
> > Halil
> >   
> 
> [..]
> 
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH RFC 2/2] s390x: attach autogenerated nics

[Christian Borntraeger]

On 11/28/2017 02:46 PM, Cornelia Huck wrote:
> The autogenerated nics should be treated as any other device; use
> qdev_set_id() to have them show up under peripheral-anon.
> 
I think this is fine, but then I ask myself how x86 does this. So I tried to 
find out how the pc-q35 machine does this but I somehow failed to understand
how they do it. Do you have any clue?

> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index a23b8aec9f..830bae9d0f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -35,6 +35,7 @@
>  #include "cpu_models.h"
>  #include "qapi/qmp/qerror.h"
>  #include "hw/nmi.h"
> +#include "include/monitor/qdev.h"
> 
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> @@ -259,6 +260,7 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
> 
>          dev = qdev_create(bus, name);
>          qdev_set_nic_properties(dev, nd);
> +        qdev_set_id(dev, NULL);
>          qdev_init_nofail(dev);
>      }
>  }
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH RFC 0/2] s390x: cut down on unattached devices

[David Hildenbrand]

On 28.11.2017 14:46, Cornelia Huck wrote:
> info qom-tree shows several devices under unattached that probably
> should go somewhere.
> 
> The css bridge should attach to the machine, as it has a similar
> purpose as e.g. a pci host bridge.
> 
> The autogenerated network devices should be in the same bucket as any
> other device; I'm just not sure about the way I went about it.
> 
> The zpci devices are still problematic: I don't have a good idea where
> they should show up.
> 
> Remaining in the unattached container are the sysbus, memory regions
> and cpus.
> 
> Cornelia Huck (2):
>   s390x/css: attach css bridge
>   s390x: attach autogenerated nics
> 
>  hw/s390x/css-bridge.c      | 2 ++
>  hw/s390x/s390-virtio-ccw.c | 2 ++
>  2 files changed, 4 insertions(+)
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

My guess is, that this should not break migration (or anything else).

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Halil Pasic]

On 12/04/2017 10:22 AM, Cornelia Huck wrote:
> On Fri, 1 Dec 2017 15:41:21 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 11/28/2017 04:21 PM, Halil Pasic wrote:
>> [..]
>>>>> Otherwise at first glance both patches seem sane.  
>>>>
>>>> Can I count this as an ack, or do you plan to do more review?
>>>>  
>>>
>>> Yes I was planning to give it another look. And I do already
>>> have questions. Isn't the QOM composition tree API? I mean
>>> let's assume the QMP commands working on this tree are not completely
>>> useless. How is client code (management software) supposed to work,
>>> assumed it can rely on paths of e.g. properties being stable. Just
>>> imagine we had this default-cssid property (for the sake of the
>>> argument, not like we want it) on the css bridge.  
>>
>> Ping! I would like to get this clarified before proceeding with reviewing
>> this series.
> 
> [It might be helpful to not drop cc:s.]
> 

Sorry. Wrong button.

> I don't think we really want a static tree. As long as the devices are
> locateable, it should be fine.
> 

What do you mean by locateable in particular?

Let's say I'm management software guy who want's to access a certain
property of a certain device. For that I'm supposed to use qom-get. Now
qom-get takes a path as input argument (absolute or relative). The question
is, how I'm supposed to figure out this path as management software developer?
In other words what is the algorithm?

One naive approach would be, to assume that the path is stable between
releases. So I have to figure it out when I'm implementing the stuff,
and then it keeps working ever after. I read your answer as this naive
approach is wrong.

(BTW I find static also confusing in this context.)

[..]

Re: [Qemu-devel] [qemu-s390x] [PATCH RFC 2/2] s390x: attach autogenerated nics

[Cornelia Huck]

On Mon, 4 Dec 2017 12:17:06 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 11/28/2017 02:46 PM, Cornelia Huck wrote:
> > The autogenerated nics should be treated as any other device; use
> > qdev_set_id() to have them show up under peripheral-anon.
> >   
> I think this is fine, but then I ask myself how x86 does this. So I tried to 
> find out how the pc-q35 machine does this but I somehow failed to understand
> how they do it. Do you have any clue?

It seems they don't. If you start up a machine with only autogenerated
devices, you won't find anything under peripheral{-anon}, but several
devices under unattached.

So, maybe we should change this for everything? Or just leave it alone?

(The css-bridge change is a different thing IMO, it clearly should be
attached to the machine.)

> 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/s390-virtio-ccw.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index a23b8aec9f..830bae9d0f 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -35,6 +35,7 @@
> >  #include "cpu_models.h"
> >  #include "qapi/qmp/qerror.h"
> >  #include "hw/nmi.h"
> > +#include "include/monitor/qdev.h"
> > 
> >  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> >  {
> > @@ -259,6 +260,7 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
> > 
> >          dev = qdev_create(bus, name);
> >          qdev_set_nic_properties(dev, nd);
> > +        qdev_set_id(dev, NULL);
> >          qdev_init_nofail(dev);
> >      }
> >  }
> >   
> 

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Cornelia Huck]

On Mon, 4 Dec 2017 15:47:37 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/04/2017 10:22 AM, Cornelia Huck wrote:
> > On Fri, 1 Dec 2017 15:41:21 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 11/28/2017 04:21 PM, Halil Pasic wrote:
> >> [..]  
> >>>>> Otherwise at first glance both patches seem sane.    
> >>>>
> >>>> Can I count this as an ack, or do you plan to do more review?
> >>>>    
> >>>
> >>> Yes I was planning to give it another look. And I do already
> >>> have questions. Isn't the QOM composition tree API? I mean
> >>> let's assume the QMP commands working on this tree are not completely
> >>> useless. How is client code (management software) supposed to work,
> >>> assumed it can rely on paths of e.g. properties being stable. Just
> >>> imagine we had this default-cssid property (for the sake of the
> >>> argument, not like we want it) on the css bridge.    
> >>
> >> Ping! I would like to get this clarified before proceeding with reviewing
> >> this series.  
> > 
> > [It might be helpful to not drop cc:s.]
> >   
> 
> Sorry. Wrong button.
> 
> > I don't think we really want a static tree. As long as the devices are
> > locateable, it should be fine.
> >   
> 
> What do you mean by locateable in particular?
> 
> Let's say I'm management software guy who want's to access a certain
> property of a certain device. For that I'm supposed to use qom-get. Now
> qom-get takes a path as input argument (absolute or relative). The question
> is, how I'm supposed to figure out this path as management software developer?
> In other words what is the algorithm?

I'd expect qom-tree to put out a path to the right object.

No idea if/how much management software relies on this. But hardcoded
paths sound fragile to me.

> 
> One naive approach would be, to assume that the path is stable between
> releases. So I have to figure it out when I'm implementing the stuff,
> and then it keeps working ever after. I read your answer as this naive
> approach is wrong.
> 
> (BTW I find static also confusing in this context.)
> 
> [..]
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH RFC 2/2] s390x: attach autogenerated nics

[Halil Pasic]

On 12/04/2017 05:40 PM, Cornelia Huck wrote:
> On Mon, 4 Dec 2017 12:17:06 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 11/28/2017 02:46 PM, Cornelia Huck wrote:
>>> The autogenerated nics should be treated as any other device; use
>>> qdev_set_id() to have them show up under peripheral-anon.
>>>   
>> I think this is fine, but then I ask myself how x86 does this. So I tried to 
>> find out how the pc-q35 machine does this but I somehow failed to understand
>> how they do it. Do you have any clue?
> 
> It seems they don't. If you start up a machine with only autogenerated
> devices, you won't find anything under peripheral{-anon}, but several
> devices under unattached.
> 
> So, maybe we should change this for everything? Or just leave it alone?
> 
> (The css-bridge change is a different thing IMO, it clearly should be
> attached to the machine.)
> 

IMHO (try to) change everywhere. The devices are attached to the machine,
and them showing up as unattached is misleading. IMHO we still to have the
'is it API or not' question/problem so we need to be careful.

Another think I was wondering about is ids: there are QMP commands which
designate devices by path and there are commands which designate by id
(and we even have either-or via the same parameter in case of device_del).
Since the paths do not seem to be directly assigned/controlled by the user
([1]] but id's are I would argue that ids are easier to understand and
use. Would generating an id for each auto-generated device be a good idea?

I'm trying to figure out, how the QAPI is supposed to be used, and feel like.
So take my comments with a grain of salt.

[1] One can, but does not have to specify the bus. Libvirt does not seem
to for virtio-ccw devices.  And if one were to, the other patch in the
series could break that code.

Halil

>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>  hw/s390x/s390-virtio-ccw.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index a23b8aec9f..830bae9d0f 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -35,6 +35,7 @@
>>>  #include "cpu_models.h"
>>>  #include "qapi/qmp/qerror.h"
>>>  #include "hw/nmi.h"
>>> +#include "include/monitor/qdev.h"
>>>
>>>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>>  {
>>> @@ -259,6 +260,7 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
>>>
>>>          dev = qdev_create(bus, name);
>>>          qdev_set_nic_properties(dev, nd);
>>> +        qdev_set_id(dev, NULL);
>>>          qdev_init_nofail(dev);
>>>      }
>>>  }
>>>   
>>
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH RFC 2/2] s390x: attach autogenerated nics

[Cornelia Huck]

On Mon, 4 Dec 2017 18:33:24 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/04/2017 05:40 PM, Cornelia Huck wrote:
> > On Mon, 4 Dec 2017 12:17:06 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 11/28/2017 02:46 PM, Cornelia Huck wrote:  
> >>> The autogenerated nics should be treated as any other device; use
> >>> qdev_set_id() to have them show up under peripheral-anon.
> >>>     
> >> I think this is fine, but then I ask myself how x86 does this. So I tried to 
> >> find out how the pc-q35 machine does this but I somehow failed to understand
> >> how they do it. Do you have any clue?  
> > 
> > It seems they don't. If you start up a machine with only autogenerated
> > devices, you won't find anything under peripheral{-anon}, but several
> > devices under unattached.
> > 
> > So, maybe we should change this for everything? Or just leave it alone?
> > 
> > (The css-bridge change is a different thing IMO, it clearly should be
> > attached to the machine.)
> >   
> 
> IMHO (try to) change everywhere. The devices are attached to the machine,
> and them showing up as unattached is misleading. IMHO we still to have the
> 'is it API or not' question/problem so we need to be careful.
> 
> Another think I was wondering about is ids: there are QMP commands which
> designate devices by path and there are commands which designate by id
> (and we even have either-or via the same parameter in case of device_del).
> Since the paths do not seem to be directly assigned/controlled by the user
> ([1]] but id's are I would argue that ids are easier to understand and
> use. Would generating an id for each auto-generated device be a good idea?
> 
> I'm trying to figure out, how the QAPI is supposed to be used, and feel like.
> So take my comments with a grain of salt.
> 
> [1] One can, but does not have to specify the bus. Libvirt does not seem
> to for virtio-ccw devices.  And if one were to, the other patch in the
> series could break that code.

I'm inclined to rather just drop this patch and put it into the backlog
for idle times, before this escalates into a wholesale rewrite of core
infrastructure.

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Bjoern Walk]

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

Cornelia Huck <cohuck@redhat.com> [2017-11-28, 02:46PM +0100]:
> info qom-tree shows several devices under unattached that probably
> should go somewhere.
> 
> The css bridge should attach to the machine, as it has a similar
> purpose as e.g. a pci host bridge.
> 
> The autogenerated network devices should be in the same bucket as any
> other device; I'm just not sure about the way I went about it.
> 
> The zpci devices are still problematic: I don't have a good idea where
> they should show up.
> 
> Remaining in the unattached container are the sysbus, memory regions
> and cpus.
> 
> Cornelia Huck (2):
>   s390x/css: attach css bridge
>   s390x: attach autogenerated nics
> 
>  hw/s390x/css-bridge.c      | 2 ++
>  hw/s390x/s390-virtio-ccw.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> -- 
> 2.13.6
> 
> 

Regarding the discussion about whether the QOM tree is API and what
exploiters like libvirt should do, Halil asked me to chip in.

This patch is fine from libvirt perspective. I did a quick smoke test
and you can have a

    Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>

for what it's worth.

In general, I kind of agree with Halil. Unless somewhere in QEMU it is
documented that the QOM tree is not guaranteed to be stable for
exploiters, I'd consider is part of the API. libvirt does use at least
some hardcoded paths, most of the time for CPUs in /machine/unattached,
so if that relation would change, things break. However, there is also
code to traverse the QOM tree recursively and find a path for a given
type(?) name. If this is the preferred way, we probably should change
this in libvirt to be safe.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 906 bytes --]

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Cornelia Huck]

On Tue, 5 Dec 2017 09:59:06 +0100
Bjoern Walk <bwalk@linux.vnet.ibm.com> wrote:

> Cornelia Huck <cohuck@redhat.com> [2017-11-28, 02:46PM +0100]:
> > info qom-tree shows several devices under unattached that probably
> > should go somewhere.
> > 
> > The css bridge should attach to the machine, as it has a similar
> > purpose as e.g. a pci host bridge.
> > 
> > The autogenerated network devices should be in the same bucket as any
> > other device; I'm just not sure about the way I went about it.
> > 
> > The zpci devices are still problematic: I don't have a good idea where
> > they should show up.
> > 
> > Remaining in the unattached container are the sysbus, memory regions
> > and cpus.
> > 
> > Cornelia Huck (2):
> >   s390x/css: attach css bridge
> >   s390x: attach autogenerated nics
> > 
> >  hw/s390x/css-bridge.c      | 2 ++
> >  hw/s390x/s390-virtio-ccw.c | 2 ++
> >  2 files changed, 4 insertions(+)
> > 
> > -- 
> > 2.13.6
> > 
> >   
> 
> Regarding the discussion about whether the QOM tree is API and what
> exploiters like libvirt should do, Halil asked me to chip in.
> 
> This patch is fine from libvirt perspective. I did a quick smoke test
> and you can have a
> 
>     Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> 
> for what it's worth.

Thanks for checking.

> 
> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
> documented that the QOM tree is not guaranteed to be stable for
> exploiters, I'd consider is part of the API. libvirt does use at least
> some hardcoded paths, most of the time for CPUs in /machine/unattached,
> so if that relation would change, things break. However, there is also
> code to traverse the QOM tree recursively and find a path for a given
> type(?) name. If this is the preferred way, we probably should change
> this in libvirt to be safe.

OK, with that in mind and as we're now adding a property to check on
the css bridge, I vote for including patch 1 now (having a fixed
location under /machine looks saner that having to
check /machine/unattached/device[<n>], which might not be stable).

Patch 2 needs more discussion, as I'm not sure whether what I'm doing
is the correct way to go about this (and other machines are in the same
situation). Not sure whether it is worth trying to attach the zpci
devices somewhere.

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Halil Pasic]

On 12/07/2017 05:34 PM, Cornelia Huck wrote:
> On Tue, 5 Dec 2017 09:59:06 +0100
> Bjoern Walk <bwalk@linux.vnet.ibm.com> wrote:
> 
>> Cornelia Huck <cohuck@redhat.com> [2017-11-28, 02:46PM +0100]:
>>> info qom-tree shows several devices under unattached that probably
>>> should go somewhere.
>>>
>>> The css bridge should attach to the machine, as it has a similar
>>> purpose as e.g. a pci host bridge.
>>>
>>> The autogenerated network devices should be in the same bucket as any
>>> other device; I'm just not sure about the way I went about it.
>>>
>>> The zpci devices are still problematic: I don't have a good idea where
>>> they should show up.
>>>
>>> Remaining in the unattached container are the sysbus, memory regions
>>> and cpus.
>>>
>>> Cornelia Huck (2):
>>>   s390x/css: attach css bridge
>>>   s390x: attach autogenerated nics
>>>
>>>  hw/s390x/css-bridge.c      | 2 ++
>>>  hw/s390x/s390-virtio-ccw.c | 2 ++
>>>  2 files changed, 4 insertions(+)
>>>
>>> -- 
>>> 2.13.6
>>>
>>>   
>>
>> Regarding the discussion about whether the QOM tree is API and what
>> exploiters like libvirt should do, Halil asked me to chip in.
>>
>> This patch is fine from libvirt perspective. I did a quick smoke test
>> and you can have a
>>
>>     Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>>
>> for what it's worth.
> 
> Thanks for checking.
> 
>>
>> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
>> documented that the QOM tree is not guaranteed to be stable for
>> exploiters, I'd consider is part of the API. libvirt does use at least
>> some hardcoded paths, most of the time for CPUs in /machine/unattached,
>> so if that relation would change, things break. However, there is also
>> code to traverse the QOM tree recursively and find a path for a given
>> type(?) name. If this is the preferred way, we probably should change
>> this in libvirt to be safe.
> 
> OK, with that in mind and as we're now adding a property to check on
> the css bridge, I vote for including patch 1 now (having a fixed
> location under /machine looks saner that having to
> check /machine/unattached/device[<n>], which might not be stable).
> 
> Patch 2 needs more discussion, as I'm not sure whether what I'm doing
> is the correct way to go about this (and other machines are in the same
> situation). Not sure whether it is worth trying to attach the zpci
> devices somewhere.
> 

I think, if it's kind of API, then fixing sooner is better than fixing
later.

I also agree that patch 1 should be higher priority.

Before we do patch 1 I would like having agreed and documented whether
this is API or not.

If we decide it's an API, I think we should consider deprecating
the current interface, but keep it working for two releases or
so. I think nothing speaks against introducing a link form unattached
in patch 1 (but I have not tried yet).

Halil

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Cornelia Huck]

On Thu, 7 Dec 2017 18:01:46 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/07/2017 05:34 PM, Cornelia Huck wrote:
> > On Tue, 5 Dec 2017 09:59:06 +0100
> > Bjoern Walk <bwalk@linux.vnet.ibm.com> wrote:
> >   
> >> Cornelia Huck <cohuck@redhat.com> [2017-11-28, 02:46PM +0100]:  
> >>> info qom-tree shows several devices under unattached that probably
> >>> should go somewhere.
> >>>
> >>> The css bridge should attach to the machine, as it has a similar
> >>> purpose as e.g. a pci host bridge.
> >>>
> >>> The autogenerated network devices should be in the same bucket as any
> >>> other device; I'm just not sure about the way I went about it.
> >>>
> >>> The zpci devices are still problematic: I don't have a good idea where
> >>> they should show up.
> >>>
> >>> Remaining in the unattached container are the sysbus, memory regions
> >>> and cpus.
> >>>
> >>> Cornelia Huck (2):
> >>>   s390x/css: attach css bridge
> >>>   s390x: attach autogenerated nics
> >>>
> >>>  hw/s390x/css-bridge.c      | 2 ++
> >>>  hw/s390x/s390-virtio-ccw.c | 2 ++
> >>>  2 files changed, 4 insertions(+)
> >>>
> >>> -- 
> >>> 2.13.6
> >>>
> >>>     
> >>
> >> Regarding the discussion about whether the QOM tree is API and what
> >> exploiters like libvirt should do, Halil asked me to chip in.
> >>
> >> This patch is fine from libvirt perspective. I did a quick smoke test
> >> and you can have a
> >>
> >>     Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> >>
> >> for what it's worth.  
> > 
> > Thanks for checking.
> >   
> >>
> >> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
> >> documented that the QOM tree is not guaranteed to be stable for
> >> exploiters, I'd consider is part of the API. libvirt does use at least
> >> some hardcoded paths, most of the time for CPUs in /machine/unattached,
> >> so if that relation would change, things break. However, there is also
> >> code to traverse the QOM tree recursively and find a path for a given
> >> type(?) name. If this is the preferred way, we probably should change
> >> this in libvirt to be safe.  
> > 
> > OK, with that in mind and as we're now adding a property to check on
> > the css bridge, I vote for including patch 1 now (having a fixed
> > location under /machine looks saner that having to
> > check /machine/unattached/device[<n>], which might not be stable).
> > 
> > Patch 2 needs more discussion, as I'm not sure whether what I'm doing
> > is the correct way to go about this (and other machines are in the same
> > situation). Not sure whether it is worth trying to attach the zpci
> > devices somewhere.
> >   
> 
> I think, if it's kind of API, then fixing sooner is better than fixing
> later.
> 
> I also agree that patch 1 should be higher priority.
> 
> Before we do patch 1 I would like having agreed and documented whether
> this is API or not.
> 
> If we decide it's an API, I think we should consider deprecating
> the current interface, but keep it working for two releases or
> so. I think nothing speaks against introducing a link form unattached
> in patch 1 (but I have not tried yet).

No, just no. That's completely overengineered.

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Halil Pasic]

On 12/07/2017 06:06 PM, Cornelia Huck wrote:
> On Thu, 7 Dec 2017 18:01:46 +0100
[..]
>>>> Regarding the discussion about whether the QOM tree is API and what
>>>> exploiters like libvirt should do, Halil asked me to chip in.
>>>>
>>>> This patch is fine from libvirt perspective. I did a quick smoke test
>>>> and you can have a
>>>>
>>>>     Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>>>>
>>>> for what it's worth.  
>>>
>>> Thanks for checking.
>>>   
>>>>
>>>> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
>>>> documented that the QOM tree is not guaranteed to be stable for
>>>> exploiters, I'd consider is part of the API. libvirt does use at least
>>>> some hardcoded paths, most of the time for CPUs in /machine/unattached,
>>>> so if that relation would change, things break. However, there is also
>>>> code to traverse the QOM tree recursively and find a path for a given
>>>> type(?) name. If this is the preferred way, we probably should change
>>>> this in libvirt to be safe.  
>>>
>>> OK, with that in mind and as we're now adding a property to check on
>>> the css bridge, I vote for including patch 1 now (having a fixed
>>> location under /machine looks saner that having to
>>> check /machine/unattached/device[<n>], which might not be stable).
>>>
>>> Patch 2 needs more discussion, as I'm not sure whether what I'm doing
>>> is the correct way to go about this (and other machines are in the same
>>> situation). Not sure whether it is worth trying to attach the zpci
>>> devices somewhere.
>>>   
>>
>> I think, if it's kind of API, then fixing sooner is better than fixing
>> later.
>>
>> I also agree that patch 1 should be higher priority.
>>
>> Before we do patch 1 I would like having agreed and documented whether
>> this is API or not.
>>
>> If we decide it's an API, I think we should consider deprecating
>> the current interface, but keep it working for two releases or
>> so. I think nothing speaks against introducing a link form unattached
>> in patch 1 (but I have not tried yet).
> 
> No, just no. That's completely overengineered.
> 

Which part is totally overengineered? Having it clear what is API and
what not? Having this documented? Or caring about our deprecation
policy (if it's API)?

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Cornelia Huck]

On Thu, 7 Dec 2017 18:15:57 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/07/2017 06:06 PM, Cornelia Huck wrote:
> > On Thu, 7 Dec 2017 18:01:46 +0100  
> [..]
> >>>> Regarding the discussion about whether the QOM tree is API and what
> >>>> exploiters like libvirt should do, Halil asked me to chip in.
> >>>>
> >>>> This patch is fine from libvirt perspective. I did a quick smoke test
> >>>> and you can have a
> >>>>
> >>>>     Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> >>>>
> >>>> for what it's worth.    
> >>>
> >>> Thanks for checking.
> >>>     
> >>>>
> >>>> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
> >>>> documented that the QOM tree is not guaranteed to be stable for
> >>>> exploiters, I'd consider is part of the API. libvirt does use at least
> >>>> some hardcoded paths, most of the time for CPUs in /machine/unattached,
> >>>> so if that relation would change, things break. However, there is also
> >>>> code to traverse the QOM tree recursively and find a path for a given
> >>>> type(?) name. If this is the preferred way, we probably should change
> >>>> this in libvirt to be safe.    
> >>>
> >>> OK, with that in mind and as we're now adding a property to check on
> >>> the css bridge, I vote for including patch 1 now (having a fixed
> >>> location under /machine looks saner that having to
> >>> check /machine/unattached/device[<n>], which might not be stable).
> >>>
> >>> Patch 2 needs more discussion, as I'm not sure whether what I'm doing
> >>> is the correct way to go about this (and other machines are in the same
> >>> situation). Not sure whether it is worth trying to attach the zpci
> >>> devices somewhere.
> >>>     
> >>
> >> I think, if it's kind of API, then fixing sooner is better than fixing
> >> later.
> >>
> >> I also agree that patch 1 should be higher priority.
> >>
> >> Before we do patch 1 I would like having agreed and documented whether
> >> this is API or not.
> >>
> >> If we decide it's an API, I think we should consider deprecating
> >> the current interface, but keep it working for two releases or
> >> so. I think nothing speaks against introducing a link form unattached
> >> in patch 1 (but I have not tried yet).  
> > 
> > No, just no. That's completely overengineered.
> >   
> 
> Which part is totally overengineered? Having it clear what is API and
> what not? Having this documented? Or caring about our deprecation
> policy (if it's API)?
> 

You're building a monster to fix a non-existing problem. I will not go
down that rabbit hole any further, and just apply patch 1.

Re: [Qemu-devel] [PATCH RFC 1/2] s390x/css: attach css bridge

[Cornelia Huck]

On Tue, 28 Nov 2017 14:46:47 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> Logically, the css bridge should be attached to the machine.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/css-bridge.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
> index c4a9735d71..84d33eafc2 100644
> --- a/hw/s390x/css-bridge.c
> +++ b/hw/s390x/css-bridge.c
> @@ -99,6 +99,8 @@ VirtualCssBus *virtual_css_bus_init(void)
>  
>      /* Create bridge device */
>      dev = qdev_create(NULL, TYPE_VIRTUAL_CSS_BRIDGE);
> +    object_property_add_child(qdev_get_machine(), TYPE_VIRTUAL_CSS_BRIDGE,
> +                              OBJECT(dev), NULL);
>      qdev_init_nofail(dev);
>  
>      /* Create bus on bridge device */

Queued to s390-next.

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

[Halil Pasic]

On 12/08/2017 12:42 PM, Cornelia Huck wrote:
[..]
>>>>>> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
>>>>>> documented that the QOM tree is not guaranteed to be stable for
>>>>>> exploiters, I'd consider is part of the API. libvirt does use at least
>>>>>> some hardcoded paths, most of the time for CPUs in /machine/unattached,
>>>>>> so if that relation would change, things break. However, there is also
>>>>>> code to traverse the QOM tree recursively and find a path for a given
>>>>>> type(?) name. If this is the preferred way, we probably should change
>>>>>> this in libvirt to be safe.    
>>>>>
>>>>> OK, with that in mind and as we're now adding a property to check on
>>>>> the css bridge, I vote for including patch 1 now (having a fixed
>>>>> location under /machine looks saner that having to
>>>>> check /machine/unattached/device[<n>], which might not be stable).
>>>>>
>>>>> Patch 2 needs more discussion, as I'm not sure whether what I'm doing
>>>>> is the correct way to go about this (and other machines are in the same
>>>>> situation). Not sure whether it is worth trying to attach the zpci
>>>>> devices somewhere.
>>>>>     
>>>>
>>>> I think, if it's kind of API, then fixing sooner is better than fixing
>>>> later.
>>>>
>>>> I also agree that patch 1 should be higher priority.
>>>>
>>>> Before we do patch 1 I would like having agreed and documented whether
>>>> this is API or not.
>>>>
>>>> If we decide it's an API, I think we should consider deprecating
>>>> the current interface, but keep it working for two releases or
>>>> so. I think nothing speaks against introducing a link form unattached
>>>> in patch 1 (but I have not tried yet).  
>>>
>>> No, just no. That's completely overengineered.
>>>   
>>
>> Which part is totally overengineered? Having it clear what is API and
>> what not? Having this documented? Or caring about our deprecation
>> policy (if it's API)?
>>
> 
> You're building a monster to fix a non-existing problem. I will not go
> down that rabbit hole any further, and just apply patch 1.
> 

I'm not building anything. I've basically just asked a simple question:
Are  paths in the qom composition tree external API or not (and if not,
what is the canonical way to accomplish certain things)? Then I though
out loud about the branches we can take based on the answer.

Based on the answers I got it seems I'm not particularly good at asking
questions. Sorry about that.

Halil