| Message ID | 20171128134648.21530-3-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: cut down on unattached devices | expand |
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); > } > } >
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); > > } > > } > > >
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); >>> } >>> } >>> >> >
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.
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); } }
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(+)