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

[Halil Pasic <pasic@linux.vnet.ibm.com>]

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);
>>>      }
>>>  }
>>>   
>>
>