| Message ID | 20210322191551.25752-2-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x cleanup | expand |
On Mon, 22 Mar 2021 20:15:47 +0100 Claudio Fontana <cfontana@suse.de> wrote: > this allows to remove unneeded stubs for target/s390x. This patch doesn't seem to remove any, though? > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > hw/s390x/tod.c | 9 ++++++++- > hw/s390x/meson.build | 5 ++++- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/tod.c b/hw/s390x/tod.c > index 3c2979175e..322732d7fd 100644 > --- a/hw/s390x/tod.c > +++ b/hw/s390x/tod.c > @@ -14,6 +14,8 @@ > #include "qemu/error-report.h" > #include "qemu/module.h" > #include "sysemu/kvm.h" > +#include "sysemu/tcg.h" > +#include "sysemu/qtest.h" > #include "migration/qemu-file-types.h" > #include "migration/register.h" > > @@ -23,8 +25,13 @@ void s390_init_tod(void) > > if (kvm_enabled()) { > obj = object_new(TYPE_KVM_S390_TOD); > - } else { > + } else if (tcg_enabled()) { > obj = object_new(TYPE_QEMU_S390_TOD); > + } else if (qtest_enabled()) { > + return; > + } else { > + warn_report("current accelerator not handled in s390_init_tod!"); > + return; I'm wondering whether this should be a fatal error. > } > object_property_add_child(qdev_get_machine(), TYPE_S390_TOD, obj); > object_unref(obj); > diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build > index 91495b5631..7f31f9e5d5 100644 > --- a/hw/s390x/meson.build > +++ b/hw/s390x/meson.build > @@ -16,7 +16,6 @@ s390x_ss.add(files( > 'sclp.c', > 'sclpcpu.c', > 'sclpquiesce.c', > - 'tod-qemu.c', > 'tod.c', > )) > s390x_ss.add(when: 'CONFIG_KVM', if_true: files( > @@ -25,6 +24,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files( > 's390-stattrib-kvm.c', > 'pv.c', > )) > +s390x_ss.add(when: 'CONFIG_TCG', if_true: files( > + 'tod-qemu.c', Should we rename this to tod-tcg.c? > +)) > + > s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-virtio-ccw.c')) > s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c')) > s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
Hi Cornelia, On 3/31/21 1:07 PM, Cornelia Huck wrote: > On Mon, 22 Mar 2021 20:15:47 +0100 > Claudio Fontana <cfontana@suse.de> wrote: > >> this allows to remove unneeded stubs for target/s390x. > > This patch doesn't seem to remove any, though? The next patch does... I'll split more the patches so it becomes clearer. > >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> --- >> hw/s390x/tod.c | 9 ++++++++- >> hw/s390x/meson.build | 5 ++++- >> 2 files changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/hw/s390x/tod.c b/hw/s390x/tod.c >> index 3c2979175e..322732d7fd 100644 >> --- a/hw/s390x/tod.c >> +++ b/hw/s390x/tod.c >> @@ -14,6 +14,8 @@ >> #include "qemu/error-report.h" >> #include "qemu/module.h" >> #include "sysemu/kvm.h" >> +#include "sysemu/tcg.h" >> +#include "sysemu/qtest.h" >> #include "migration/qemu-file-types.h" >> #include "migration/register.h" >> >> @@ -23,8 +25,13 @@ void s390_init_tod(void) >> >> if (kvm_enabled()) { >> obj = object_new(TYPE_KVM_S390_TOD); >> - } else { >> + } else if (tcg_enabled()) { >> obj = object_new(TYPE_QEMU_S390_TOD); >> + } else if (qtest_enabled()) { >> + return; >> + } else { >> + warn_report("current accelerator not handled in s390_init_tod!"); >> + return; > > I'm wondering whether this should be a fatal error. I would agree with that. > >> } >> object_property_add_child(qdev_get_machine(), TYPE_S390_TOD, obj); >> object_unref(obj); >> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build >> index 91495b5631..7f31f9e5d5 100644 >> --- a/hw/s390x/meson.build >> +++ b/hw/s390x/meson.build >> @@ -16,7 +16,6 @@ s390x_ss.add(files( >> 'sclp.c', >> 'sclpcpu.c', >> 'sclpquiesce.c', >> - 'tod-qemu.c', >> 'tod.c', >> )) >> s390x_ss.add(when: 'CONFIG_KVM', if_true: files( >> @@ -25,6 +24,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files( >> 's390-stattrib-kvm.c', >> 'pv.c', >> )) >> +s390x_ss.add(when: 'CONFIG_TCG', if_true: files( >> + 'tod-qemu.c', > > Should we rename this to tod-tcg.c? I think so. > >> +)) >> + >> s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-virtio-ccw.c')) >> s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c')) >> s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c')) > > Will prepare a new version, Thanks, Claudio
On 4/19/21 11:11 AM, Claudio Fontana wrote: > Hi Cornelia, > > On 3/31/21 1:07 PM, Cornelia Huck wrote: >> On Mon, 22 Mar 2021 20:15:47 +0100 >> Claudio Fontana <cfontana@suse.de> wrote: >> >>> this allows to remove unneeded stubs for target/s390x. >> >> This patch doesn't seem to remove any, though? > > The next patch does... I'll split more the patches so it becomes clearer. > >> >>> >>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>> --- >>> hw/s390x/tod.c | 9 ++++++++- >>> hw/s390x/meson.build | 5 ++++- >>> 2 files changed, 12 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/s390x/tod.c b/hw/s390x/tod.c >>> index 3c2979175e..322732d7fd 100644 >>> --- a/hw/s390x/tod.c >>> +++ b/hw/s390x/tod.c >>> @@ -14,6 +14,8 @@ >>> #include "qemu/error-report.h" >>> #include "qemu/module.h" >>> #include "sysemu/kvm.h" >>> +#include "sysemu/tcg.h" >>> +#include "sysemu/qtest.h" >>> #include "migration/qemu-file-types.h" >>> #include "migration/register.h" >>> >>> @@ -23,8 +25,13 @@ void s390_init_tod(void) >>> >>> if (kvm_enabled()) { >>> obj = object_new(TYPE_KVM_S390_TOD); >>> - } else { >>> + } else if (tcg_enabled()) { >>> obj = object_new(TYPE_QEMU_S390_TOD); >>> + } else if (qtest_enabled()) { >>> + return; >>> + } else { >>> + warn_report("current accelerator not handled in s390_init_tod!"); >>> + return; >> >> I'm wondering whether this should be a fatal error. > > I would agree with that. > >> >>> } >>> object_property_add_child(qdev_get_machine(), TYPE_S390_TOD, obj); >>> object_unref(obj); >>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build >>> index 91495b5631..7f31f9e5d5 100644 >>> --- a/hw/s390x/meson.build >>> +++ b/hw/s390x/meson.build >>> @@ -16,7 +16,6 @@ s390x_ss.add(files( >>> 'sclp.c', >>> 'sclpcpu.c', >>> 'sclpquiesce.c', >>> - 'tod-qemu.c', >>> 'tod.c', >>> )) >>> s390x_ss.add(when: 'CONFIG_KVM', if_true: files( >>> @@ -25,6 +24,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files( >>> 's390-stattrib-kvm.c', >>> 'pv.c', >>> )) >>> +s390x_ss.add(when: 'CONFIG_TCG', if_true: files( >>> + 'tod-qemu.c', >> >> Should we rename this to tod-tcg.c? > > I think so. Here we are a bit limited though by the fact that the object is currently called: include/hw/s390x/tod.h:26:#define TYPE_QEMU_S390_TOD TYPE_S390_TOD "-qemu" So there might be a compatibility issue in trying to make this consistent, which would mean to replace this with: #define TYPE_TCG_S390_TOD TYPE_S390_TOD "-tcg" What do you think? > >> >>> +)) >>> + >>> s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-virtio-ccw.c')) >>> s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c')) >>> s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c')) >> >> > > Will prepare a new version, > > Thanks, > > Claudio >
On Mon, 19 Apr 2021 18:12:48 +0200 Claudio Fontana <cfontana@suse.de> wrote: > On 4/19/21 11:11 AM, Claudio Fontana wrote: > > Hi Cornelia, > > > > On 3/31/21 1:07 PM, Cornelia Huck wrote: > >> On Mon, 22 Mar 2021 20:15:47 +0100 > >> Claudio Fontana <cfontana@suse.de> wrote: > >>> @@ -25,6 +24,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files( > >>> 's390-stattrib-kvm.c', > >>> 'pv.c', > >>> )) > >>> +s390x_ss.add(when: 'CONFIG_TCG', if_true: files( > >>> + 'tod-qemu.c', > >> > >> Should we rename this to tod-tcg.c? > > > > I think so. > > Here we are a bit limited though by the fact that the object is currently called: > > include/hw/s390x/tod.h:26:#define TYPE_QEMU_S390_TOD TYPE_S390_TOD "-qemu" > > So there might be a compatibility issue in trying to make this consistent, which would mean to replace this with: > > #define TYPE_TCG_S390_TOD TYPE_S390_TOD "-tcg" > > What do you think? How visible is this? I don't think the TOD objects are instantiable by the user.
On 4/19/21 6:20 PM, Cornelia Huck wrote: > On Mon, 19 Apr 2021 18:12:48 +0200 > Claudio Fontana <cfontana@suse.de> wrote: > >> On 4/19/21 11:11 AM, Claudio Fontana wrote: >>> Hi Cornelia, >>> >>> On 3/31/21 1:07 PM, Cornelia Huck wrote: >>>> On Mon, 22 Mar 2021 20:15:47 +0100 >>>> Claudio Fontana <cfontana@suse.de> wrote: > >>>>> @@ -25,6 +24,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files( >>>>> 's390-stattrib-kvm.c', >>>>> 'pv.c', >>>>> )) >>>>> +s390x_ss.add(when: 'CONFIG_TCG', if_true: files( >>>>> + 'tod-qemu.c', >>>> >>>> Should we rename this to tod-tcg.c? >>> >>> I think so. >> >> Here we are a bit limited though by the fact that the object is currently called: >> >> include/hw/s390x/tod.h:26:#define TYPE_QEMU_S390_TOD TYPE_S390_TOD "-qemu" >> >> So there might be a compatibility issue in trying to make this consistent, which would mean to replace this with: >> >> #define TYPE_TCG_S390_TOD TYPE_S390_TOD "-tcg" >> >> What do you think? > > How visible is this? I don't think the TOD objects are instantiable by > the user. > I just remember we were very conservative with the object hierarchy on x86, personally I am fine with the change. I will add this change then, I'd ask for people with concerns about this to speak up: (Paolo?) Ciao, CLaudio
On Mon, 19 Apr 2021 18:24:34 +0200 Claudio Fontana <cfontana@suse.de> wrote: > On 4/19/21 6:20 PM, Cornelia Huck wrote: > > On Mon, 19 Apr 2021 18:12:48 +0200 > > Claudio Fontana <cfontana@suse.de> wrote: > > > >> On 4/19/21 11:11 AM, Claudio Fontana wrote: > >>> Hi Cornelia, > >>> > >>> On 3/31/21 1:07 PM, Cornelia Huck wrote: > >>>> On Mon, 22 Mar 2021 20:15:47 +0100 > >>>> Claudio Fontana <cfontana@suse.de> wrote: > > > >>>>> @@ -25,6 +24,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files( > >>>>> 's390-stattrib-kvm.c', > >>>>> 'pv.c', > >>>>> )) > >>>>> +s390x_ss.add(when: 'CONFIG_TCG', if_true: files( > >>>>> + 'tod-qemu.c', > >>>> > >>>> Should we rename this to tod-tcg.c? > >>> > >>> I think so. > >> > >> Here we are a bit limited though by the fact that the object is currently called: > >> > >> include/hw/s390x/tod.h:26:#define TYPE_QEMU_S390_TOD TYPE_S390_TOD "-qemu" > >> > >> So there might be a compatibility issue in trying to make this consistent, which would mean to replace this with: > >> > >> #define TYPE_TCG_S390_TOD TYPE_S390_TOD "-tcg" > >> > >> What do you think? > > > > How visible is this? I don't think the TOD objects are instantiable by > > the user. > > > > I just remember we were very conservative with the object hierarchy on x86, personally I am fine with the change. > I will add this change then, I'd ask for people with concerns about this to speak up: > > (Paolo?) > > Ciao, > > CLaudio > It was more an argument against changing it, because most people won't see it anyway :)
diff --git a/hw/s390x/tod.c b/hw/s390x/tod.c index 3c2979175e..322732d7fd 100644 --- a/hw/s390x/tod.c +++ b/hw/s390x/tod.c @@ -14,6 +14,8 @@ #include "qemu/error-report.h" #include "qemu/module.h" #include "sysemu/kvm.h" +#include "sysemu/tcg.h" +#include "sysemu/qtest.h" #include "migration/qemu-file-types.h" #include "migration/register.h" @@ -23,8 +25,13 @@ void s390_init_tod(void) if (kvm_enabled()) { obj = object_new(TYPE_KVM_S390_TOD); - } else { + } else if (tcg_enabled()) { obj = object_new(TYPE_QEMU_S390_TOD); + } else if (qtest_enabled()) { + return; + } else { + warn_report("current accelerator not handled in s390_init_tod!"); + return; } object_property_add_child(qdev_get_machine(), TYPE_S390_TOD, obj); object_unref(obj); diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build index 91495b5631..7f31f9e5d5 100644 --- a/hw/s390x/meson.build +++ b/hw/s390x/meson.build @@ -16,7 +16,6 @@ s390x_ss.add(files( 'sclp.c', 'sclpcpu.c', 'sclpquiesce.c', - 'tod-qemu.c', 'tod.c', )) s390x_ss.add(when: 'CONFIG_KVM', if_true: files( @@ -25,6 +24,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files( 's390-stattrib-kvm.c', 'pv.c', )) +s390x_ss.add(when: 'CONFIG_TCG', if_true: files( + 'tod-qemu.c', +)) + s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-virtio-ccw.c')) s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c')) s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
this allows to remove unneeded stubs for target/s390x. Signed-off-by: Claudio Fontana <cfontana@suse.de> --- hw/s390x/tod.c | 9 ++++++++- hw/s390x/meson.build | 5 ++++- 2 files changed, 12 insertions(+), 2 deletions(-)