| Message ID | 20171211134740.8235-5-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: flic rework, tcg flic support and tcg | expand |
On 12/11/2017 02:47 PM, David Hildenbrand wrote: > This makes it clearer, which device is used for which accelerator. > > Signed-off-by: David Hildenbrand <david@redhat.com> nice. Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > hw/intc/s390_flic.c | 9 +++++++-- > hw/intc/s390_flic_kvm.c | 12 ------------ > include/hw/s390x/s390_flic.h | 9 --------- > 3 files changed, 7 insertions(+), 23 deletions(-) > > diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c > index 6eaf178d79..a78bdf1d90 100644 > --- a/hw/intc/s390_flic.c > +++ b/hw/intc/s390_flic.c > @@ -40,11 +40,16 @@ void s390_flic_init(void) > { > DeviceState *dev; > > - dev = s390_flic_kvm_create(); > - if (!dev) { > + if (kvm_enabled()) { > + dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); > + object_property_add_child(qdev_get_machine(), TYPE_KVM_S390_FLIC, > + OBJECT(dev), NULL); > + } else if (tcg_enabled()) { > dev = qdev_create(NULL, TYPE_QEMU_S390_FLIC); > object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC, > OBJECT(dev), NULL); > + } else { > + g_assert_not_reached(); > } > qdev_init_nofail(dev); > } > diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c > index d208cb81c4..0cb5feab0c 100644 > --- a/hw/intc/s390_flic_kvm.c > +++ b/hw/intc/s390_flic_kvm.c > @@ -35,18 +35,6 @@ typedef struct KVMS390FLICState { > bool clear_io_supported; > } KVMS390FLICState; > > -DeviceState *s390_flic_kvm_create(void) > -{ > - DeviceState *dev = NULL; > - > - if (kvm_enabled()) { > - dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); > - object_property_add_child(qdev_get_machine(), TYPE_KVM_S390_FLIC, > - OBJECT(dev), NULL); > - } > - return dev; > -} > - > /** > * flic_get_all_irqs - store all pending irqs in buffer > * @buf: pointer to buffer which is passed to kernel > diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h > index 7aab6ef7f0..5b00e936fa 100644 > --- a/include/hw/s390x/s390_flic.h > +++ b/include/hw/s390x/s390_flic.h > @@ -91,13 +91,4 @@ void s390_flic_init(void); > S390FLICState *s390_get_flic(void); > bool ais_needed(void *opaque); > > -#ifdef CONFIG_KVM > -DeviceState *s390_flic_kvm_create(void); > -#else > -static inline DeviceState *s390_flic_kvm_create(void) > -{ > - return NULL; > -} > -#endif > - > #endif /* HW_S390_FLIC_H */ >
On Mon, 11 Dec 2017 14:47:29 +0100 David Hildenbrand <david@redhat.com> wrote: > This makes it clearer, which device is used for which accelerator. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/intc/s390_flic.c | 9 +++++++-- > hw/intc/s390_flic_kvm.c | 12 ------------ > include/hw/s390x/s390_flic.h | 9 --------- > 3 files changed, 7 insertions(+), 23 deletions(-) > > diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c > index 6eaf178d79..a78bdf1d90 100644 > --- a/hw/intc/s390_flic.c > +++ b/hw/intc/s390_flic.c > @@ -40,11 +40,16 @@ void s390_flic_init(void) > { > DeviceState *dev; > > - dev = s390_flic_kvm_create(); > - if (!dev) { > + if (kvm_enabled()) { > + dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); > + object_property_add_child(qdev_get_machine(), TYPE_KVM_S390_FLIC, > + OBJECT(dev), NULL); > + } else if (tcg_enabled()) { > dev = qdev_create(NULL, TYPE_QEMU_S390_FLIC); > object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC, > OBJECT(dev), NULL); Can you use TYPE_S390_FLIC_COMMON for attaching the flic to the machine? > + } else { > + g_assert_not_reached(); Checking for tcg_enabled() explicitly does not seem the common pattern, although it is fine with me (I doubt we'll support other accelerators for s390x in the foreseeable future). > } > qdev_init_nofail(dev); > } Do we want to switch to the same pattern for the storage attribute device as well? Change looks fine to me.
On 11.12.2017 18:17, Cornelia Huck wrote: > On Mon, 11 Dec 2017 14:47:29 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> This makes it clearer, which device is used for which accelerator. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/intc/s390_flic.c | 9 +++++++-- >> hw/intc/s390_flic_kvm.c | 12 ------------ >> include/hw/s390x/s390_flic.h | 9 --------- >> 3 files changed, 7 insertions(+), 23 deletions(-) >> >> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c >> index 6eaf178d79..a78bdf1d90 100644 >> --- a/hw/intc/s390_flic.c >> +++ b/hw/intc/s390_flic.c >> @@ -40,11 +40,16 @@ void s390_flic_init(void) >> { >> DeviceState *dev; >> >> - dev = s390_flic_kvm_create(); >> - if (!dev) { >> + if (kvm_enabled()) { >> + dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); >> + object_property_add_child(qdev_get_machine(), TYPE_KVM_S390_FLIC, >> + OBJECT(dev), NULL); >> + } else if (tcg_enabled()) { >> dev = qdev_create(NULL, TYPE_QEMU_S390_FLIC); >> object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC, >> OBJECT(dev), NULL); > > Can you use TYPE_S390_FLIC_COMMON for attaching the flic to the machine? I suggest doing that in a separate patch. (I remember that changing the name should not harm migration). > >> + } else { >> + g_assert_not_reached(); > > Checking for tcg_enabled() explicitly does not seem the common pattern, > although it is fine with me (I doubt we'll support other accelerators > for s390x in the foreseeable future). Indeed, I can drop that. > >> } >> qdev_init_nofail(dev); >> } > > Do we want to switch to the same pattern for the storage attribute > device as well? Yes, can have a look, thanks! > > Change looks fine to me. >
On Mon, 11 Dec 2017 21:34:20 +0100 David Hildenbrand <david@redhat.com> wrote: > On 11.12.2017 18:17, Cornelia Huck wrote: > > On Mon, 11 Dec 2017 14:47:29 +0100 > > David Hildenbrand <david@redhat.com> wrote: > > > >> This makes it clearer, which device is used for which accelerator. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> hw/intc/s390_flic.c | 9 +++++++-- > >> hw/intc/s390_flic_kvm.c | 12 ------------ > >> include/hw/s390x/s390_flic.h | 9 --------- > >> 3 files changed, 7 insertions(+), 23 deletions(-) > >> > >> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c > >> index 6eaf178d79..a78bdf1d90 100644 > >> --- a/hw/intc/s390_flic.c > >> +++ b/hw/intc/s390_flic.c > >> @@ -40,11 +40,16 @@ void s390_flic_init(void) > >> { > >> DeviceState *dev; > >> > >> - dev = s390_flic_kvm_create(); > >> - if (!dev) { > >> + if (kvm_enabled()) { > >> + dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); > >> + object_property_add_child(qdev_get_machine(), TYPE_KVM_S390_FLIC, > >> + OBJECT(dev), NULL); > >> + } else if (tcg_enabled()) { > >> dev = qdev_create(NULL, TYPE_QEMU_S390_FLIC); > >> object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC, > >> OBJECT(dev), NULL); > > > > Can you use TYPE_S390_FLIC_COMMON for attaching the flic to the machine? > > I suggest doing that in a separate patch. (I remember that changing the > name should not harm migration). I don't think it harms migration, as we hook up the same device as before (just via the common flic device). Probably does not make sense as a separate patch, though, as you touch this code anyway. > > > > >> + } else { > >> + g_assert_not_reached(); > > > > Checking for tcg_enabled() explicitly does not seem the common pattern, > > although it is fine with me (I doubt we'll support other accelerators > > for s390x in the foreseeable future). > > Indeed, I can drop that. Yup.
On 12.12.2017 10:15, Cornelia Huck wrote: > On Mon, 11 Dec 2017 21:34:20 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> On 11.12.2017 18:17, Cornelia Huck wrote: >>> On Mon, 11 Dec 2017 14:47:29 +0100 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> This makes it clearer, which device is used for which accelerator. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> hw/intc/s390_flic.c | 9 +++++++-- >>>> hw/intc/s390_flic_kvm.c | 12 ------------ >>>> include/hw/s390x/s390_flic.h | 9 --------- >>>> 3 files changed, 7 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c >>>> index 6eaf178d79..a78bdf1d90 100644 >>>> --- a/hw/intc/s390_flic.c >>>> +++ b/hw/intc/s390_flic.c >>>> @@ -40,11 +40,16 @@ void s390_flic_init(void) >>>> { >>>> DeviceState *dev; >>>> >>>> - dev = s390_flic_kvm_create(); >>>> - if (!dev) { >>>> + if (kvm_enabled()) { >>>> + dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); >>>> + object_property_add_child(qdev_get_machine(), TYPE_KVM_S390_FLIC, >>>> + OBJECT(dev), NULL); >>>> + } else if (tcg_enabled()) { >>>> dev = qdev_create(NULL, TYPE_QEMU_S390_FLIC); >>>> object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC, >>>> OBJECT(dev), NULL); >>> >>> Can you use TYPE_S390_FLIC_COMMON for attaching the flic to the machine? >> >> I suggest doing that in a separate patch. (I remember that changing the >> name should not harm migration). > > I don't think it harms migration, as we hook up the same device as > before (just via the common flic device). > > Probably does not make sense as a separate patch, though, as you touch > this code anyway. Please be aware that for object_property_add_child(), TYPE... is for our example the name of the child /machine/s390-flic-qemu/ /machine/s390x-flic-kvm/ Not a type. And that would mean a change.
On Tue, 12 Dec 2017 10:58:12 +0100 David Hildenbrand <david@redhat.com> wrote: > On 12.12.2017 10:15, Cornelia Huck wrote: > > On Mon, 11 Dec 2017 21:34:20 +0100 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 11.12.2017 18:17, Cornelia Huck wrote: > >>> On Mon, 11 Dec 2017 14:47:29 +0100 > >>> David Hildenbrand <david@redhat.com> wrote: > >>> > >>>> This makes it clearer, which device is used for which accelerator. > >>>> > >>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>> --- > >>>> hw/intc/s390_flic.c | 9 +++++++-- > >>>> hw/intc/s390_flic_kvm.c | 12 ------------ > >>>> include/hw/s390x/s390_flic.h | 9 --------- > >>>> 3 files changed, 7 insertions(+), 23 deletions(-) > >>>> > >>>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c > >>>> index 6eaf178d79..a78bdf1d90 100644 > >>>> --- a/hw/intc/s390_flic.c > >>>> +++ b/hw/intc/s390_flic.c > >>>> @@ -40,11 +40,16 @@ void s390_flic_init(void) > >>>> { > >>>> DeviceState *dev; > >>>> > >>>> - dev = s390_flic_kvm_create(); > >>>> - if (!dev) { > >>>> + if (kvm_enabled()) { > >>>> + dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); > >>>> + object_property_add_child(qdev_get_machine(), TYPE_KVM_S390_FLIC, > >>>> + OBJECT(dev), NULL); > >>>> + } else if (tcg_enabled()) { > >>>> dev = qdev_create(NULL, TYPE_QEMU_S390_FLIC); > >>>> object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC, > >>>> OBJECT(dev), NULL); > >>> > >>> Can you use TYPE_S390_FLIC_COMMON for attaching the flic to the machine? > >> > >> I suggest doing that in a separate patch. (I remember that changing the > >> name should not harm migration). > > > > I don't think it harms migration, as we hook up the same device as > > before (just via the common flic device). > > > > Probably does not make sense as a separate patch, though, as you touch > > this code anyway. > > Please be aware that for object_property_add_child(), TYPE... is for our > example the name of the child > > /machine/s390-flic-qemu/ > /machine/s390x-flic-kvm/ > > Not a type. And that would mean a change. > > Ugh. Ok, let's keep it. The skeys device (for example) used the common name from the start.
On 12.12.2017 10:15, Cornelia Huck wrote: > On Mon, 11 Dec 2017 21:34:20 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> On 11.12.2017 18:17, Cornelia Huck wrote: >>> On Mon, 11 Dec 2017 14:47:29 +0100 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> This makes it clearer, which device is used for which accelerator. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> hw/intc/s390_flic.c | 9 +++++++-- >>>> hw/intc/s390_flic_kvm.c | 12 ------------ >>>> include/hw/s390x/s390_flic.h | 9 --------- >>>> 3 files changed, 7 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c >>>> index 6eaf178d79..a78bdf1d90 100644 >>>> --- a/hw/intc/s390_flic.c >>>> +++ b/hw/intc/s390_flic.c >>>> @@ -40,11 +40,16 @@ void s390_flic_init(void) >>>> { >>>> DeviceState *dev; >>>> >>>> - dev = s390_flic_kvm_create(); >>>> - if (!dev) { >>>> + if (kvm_enabled()) { >>>> + dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); >>>> + object_property_add_child(qdev_get_machine(), TYPE_KVM_S390_FLIC, >>>> + OBJECT(dev), NULL); >>>> + } else if (tcg_enabled()) { >>>> dev = qdev_create(NULL, TYPE_QEMU_S390_FLIC); >>>> object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC, >>>> OBJECT(dev), NULL); >>> >>> Can you use TYPE_S390_FLIC_COMMON for attaching the flic to the machine? >> >> I suggest doing that in a separate patch. (I remember that changing the >> name should not harm migration). > > I don't think it harms migration, as we hook up the same device as > before (just via the common flic device). > > Probably does not make sense as a separate patch, though, as you touch > this code anyway. > This is what it looks like now: commit dec1ff5cfc72fa0998e28a25dd23f0695ddfe21b Author: David Hildenbrand <david@redhat.com> Date: Mon Nov 13 23:09:56 2017 +0100 s390x/flic: simplify flic initialization This makes it clearer, which device is used for which accelerator. We can directly attach both to /machine/s390-flic/ instead of two different locations (/machine/s390x-flic[qemu|kvm]). Should not harm migration. Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> Signed-off-by: David Hildenbrand <david@redhat.com> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c index 6eaf178d79..dd5b157392 100644 --- a/hw/intc/s390_flic.c +++ b/hw/intc/s390_flic.c @@ -40,12 +40,13 @@ void s390_flic_init(void) { DeviceState *dev; - dev = s390_flic_kvm_create(); - if (!dev) { + if (kvm_enabled()) { + dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); + } else { dev = qdev_create(NULL, TYPE_QEMU_S390_FLIC); - object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC, - OBJECT(dev), NULL); } + object_property_add_child(qdev_get_machine(), TYPE_S390_FLIC_COMMON, + OBJECT(dev), NULL); qdev_init_nofail(dev); } ...
> > Do we want to switch to the same pattern for the storage attribute > device as well? > Unfortunately not that easy due to the KVM capability check. Will leave it as is for now. > Change looks fine to me. >
On Tue, 12 Dec 2017 11:45:22 +0100 David Hildenbrand <david@redhat.com> wrote: > This is what it looks like now: > > commit dec1ff5cfc72fa0998e28a25dd23f0695ddfe21b > Author: David Hildenbrand <david@redhat.com> > Date: Mon Nov 13 23:09:56 2017 +0100 > > s390x/flic: simplify flic initialization > > This makes it clearer, which device is used for which accelerator. > > We can directly attach both to /machine/s390-flic/ instead of two > different locations (/machine/s390x-flic[qemu|kvm]). Should not > harm migration. Not sure about libvirt usage, though. Let's keep it as-is? > > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > > diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c > index 6eaf178d79..dd5b157392 100644 > --- a/hw/intc/s390_flic.c > +++ b/hw/intc/s390_flic.c > @@ -40,12 +40,13 @@ void s390_flic_init(void) > { > DeviceState *dev; > > - dev = s390_flic_kvm_create(); > - if (!dev) { > + if (kvm_enabled()) { > + dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); > + } else { > dev = qdev_create(NULL, TYPE_QEMU_S390_FLIC); > - object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC, > - OBJECT(dev), NULL); > } > + object_property_add_child(qdev_get_machine(), TYPE_S390_FLIC_COMMON, > + OBJECT(dev), NULL); Dropping the check for tcg_enabled() is fine with me, though. > qdev_init_nofail(dev); > } > > ... >
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c index 6eaf178d79..a78bdf1d90 100644 --- a/hw/intc/s390_flic.c +++ b/hw/intc/s390_flic.c @@ -40,11 +40,16 @@ void s390_flic_init(void) { DeviceState *dev; - dev = s390_flic_kvm_create(); - if (!dev) { + if (kvm_enabled()) { + dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); + object_property_add_child(qdev_get_machine(), TYPE_KVM_S390_FLIC, + OBJECT(dev), NULL); + } else if (tcg_enabled()) { dev = qdev_create(NULL, TYPE_QEMU_S390_FLIC); object_property_add_child(qdev_get_machine(), TYPE_QEMU_S390_FLIC, OBJECT(dev), NULL); + } else { + g_assert_not_reached(); } qdev_init_nofail(dev); } diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c index d208cb81c4..0cb5feab0c 100644 --- a/hw/intc/s390_flic_kvm.c +++ b/hw/intc/s390_flic_kvm.c @@ -35,18 +35,6 @@ typedef struct KVMS390FLICState { bool clear_io_supported; } KVMS390FLICState; -DeviceState *s390_flic_kvm_create(void) -{ - DeviceState *dev = NULL; - - if (kvm_enabled()) { - dev = qdev_create(NULL, TYPE_KVM_S390_FLIC); - object_property_add_child(qdev_get_machine(), TYPE_KVM_S390_FLIC, - OBJECT(dev), NULL); - } - return dev; -} - /** * flic_get_all_irqs - store all pending irqs in buffer * @buf: pointer to buffer which is passed to kernel diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h index 7aab6ef7f0..5b00e936fa 100644 --- a/include/hw/s390x/s390_flic.h +++ b/include/hw/s390x/s390_flic.h @@ -91,13 +91,4 @@ void s390_flic_init(void); S390FLICState *s390_get_flic(void); bool ais_needed(void *opaque); -#ifdef CONFIG_KVM -DeviceState *s390_flic_kvm_create(void); -#else -static inline DeviceState *s390_flic_kvm_create(void) -{ - return NULL; -} -#endif - #endif /* HW_S390_FLIC_H */
This makes it clearer, which device is used for which accelerator. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/intc/s390_flic.c | 9 +++++++-- hw/intc/s390_flic_kvm.c | 12 ------------ include/hw/s390x/s390_flic.h | 9 --------- 3 files changed, 7 insertions(+), 23 deletions(-)