[PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Halil Pasic]

Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
module, which provides the type virtio-gpu-device, packaging the
hw-display-virtio-gpu module as a separate package that may or may not
be installed along with the qemu package leads to problems. Namely if
the hw-display-virtio-gpu is absent, qemu continues to advertise
virtio-gpu-ccw, but it aborts not only when one attempts using
virtio-gpu-ccw, but also when libvirtd's capability probing tries
to instantiate the type to introspect it.

Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
was chosen because it is not a portable device.

With virtio-gpu-ccw built as a module, the correct way to package a
modularized qemu is to require that hw-display-virtio-gpu must be
installed whenever the module hw-s390x-virtio-gpu-ccw.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
 hw/s390x/meson.build | 17 ++++++++++++++++-
 util/module.c        |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 2a7818d94b..153b1309fb 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: files('virtio-ccw-balloon.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-ccw-blk.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('virtio-ccw-crypto.c'))
-virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: files('virtio-ccw-gpu.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: files('virtio-ccw-input.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-ccw-net.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
@@ -46,3 +45,19 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'
 s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
 
 hw_arch += {'s390x': s390x_ss}
+
+if target.startswith('s390x')
+  hw_s390x_modules = {}
+
+  hw_s390x_modules_c_args = ['-DNEED_CPU_H',
+	      '-DCONFIG_TARGET="@0@-config-target.h"'.format(target)]
+  hw_s390x_modules_inc = [include_directories('../../target' / config_target['TARGET_BASE_ARCH'])]
+  hw_s390x_modules_dependencies = declare_dependency(
+	       include_directories: hw_s390x_modules_inc, compile_args: hw_s390x_modules_c_args)
+
+  virtio_gpu_ccw_ss = ss.source_set()
+  virtio_gpu_ccw_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: [files('virtio-ccw-gpu.c'), pixman, hw_s390x_modules_dependencies])
+  hw_s390x_modules += {'virtio-gpu-ccw': virtio_gpu_ccw_ss}
+
+  modules += {'hw-s390x': hw_s390x_modules}
+endif
diff --git a/util/module.c b/util/module.c
index c65060c167..cbe89fede6 100644
--- a/util/module.c
+++ b/util/module.c
@@ -304,6 +304,7 @@ static struct {
     { "virtio-gpu-pci-base",   "hw-", "display-virtio-gpu-pci" },
     { "virtio-gpu-pci",        "hw-", "display-virtio-gpu-pci" },
     { "vhost-user-gpu-pci",    "hw-", "display-virtio-gpu-pci" },
+    { "virtio-gpu-ccw",        "hw-", "s390x-virtio-gpu-ccw"   },
     { "virtio-vga-base",       "hw-", "display-virtio-vga"    },
     { "virtio-vga",            "hw-", "display-virtio-vga"    },
     { "vhost-user-vga",        "hw-", "display-virtio-vga"    },

base-commit: 1af5629673bb5c1592d993f9fb6119a62845f576
-- 
2.25.1

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Thomas Huth]

On 18/02/2021 03.22, Halil Pasic wrote:
> Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
> module, which provides the type virtio-gpu-device, packaging the
> hw-display-virtio-gpu module as a separate package that may or may not
> be installed along with the qemu package leads to problems. Namely if
> the hw-display-virtio-gpu is absent, qemu continues to advertise
> virtio-gpu-ccw, but it aborts not only when one attempts using
> virtio-gpu-ccw, but also when libvirtd's capability probing tries
> to instantiate the type to introspect it.
> 
> Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
> is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
> was chosen because it is not a portable device.
> 
> With virtio-gpu-ccw built as a module, the correct way to package a
> modularized qemu is to require that hw-display-virtio-gpu must be
> installed whenever the module hw-s390x-virtio-gpu-ccw.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
>   hw/s390x/meson.build | 17 ++++++++++++++++-
>   util/module.c        |  1 +
>   2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index 2a7818d94b..153b1309fb 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
>   virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: files('virtio-ccw-balloon.c'))
>   virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-ccw-blk.c'))
>   virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('virtio-ccw-crypto.c'))
> -virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: files('virtio-ccw-gpu.c'))
>   virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: files('virtio-ccw-input.c'))
>   virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-ccw-net.c'))
>   virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
> @@ -46,3 +45,19 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'
>   s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
>   
>   hw_arch += {'s390x': s390x_ss}
> +
> +if target.startswith('s390x')
> +  hw_s390x_modules = {}
> +
> +  hw_s390x_modules_c_args = ['-DNEED_CPU_H',
> +	      '-DCONFIG_TARGET="@0@-config-target.h"'.format(target)]
> +  hw_s390x_modules_inc = [include_directories('../../target' / config_target['TARGET_BASE_ARCH'])]
> +  hw_s390x_modules_dependencies = declare_dependency(
> +	       include_directories: hw_s390x_modules_inc, compile_args: hw_s390x_modules_c_args)

Basically the patch looks fine to me, but I wonder why all that above lines 
(related to hw_s390x_modules_dependencies) are requred at all? The other 
display modules in hw/display/meson.build also do not need to re-define 
c_args for example?

  Thomas


> +  virtio_gpu_ccw_ss = ss.source_set()
> +  virtio_gpu_ccw_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: [files('virtio-ccw-gpu.c'), pixman, hw_s390x_modules_dependencies])
> +  hw_s390x_modules += {'virtio-gpu-ccw': virtio_gpu_ccw_ss}
> +
> +  modules += {'hw-s390x': hw_s390x_modules}
> +endif
> diff --git a/util/module.c b/util/module.c
> index c65060c167..cbe89fede6 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -304,6 +304,7 @@ static struct {
>       { "virtio-gpu-pci-base",   "hw-", "display-virtio-gpu-pci" },
>       { "virtio-gpu-pci",        "hw-", "display-virtio-gpu-pci" },
>       { "vhost-user-gpu-pci",    "hw-", "display-virtio-gpu-pci" },
> +    { "virtio-gpu-ccw",        "hw-", "s390x-virtio-gpu-ccw"   },
>       { "virtio-vga-base",       "hw-", "display-virtio-vga"    },
>       { "virtio-vga",            "hw-", "display-virtio-vga"    },
>       { "vhost-user-vga",        "hw-", "display-virtio-vga"    },
> 
> base-commit: 1af5629673bb5c1592d993f9fb6119a62845f576
> 

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Halil Pasic]

On Thu, 18 Feb 2021 10:23:16 +0100
Thomas Huth <thuth@redhat.com> wrote:

> > Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
> > module, which provides the type virtio-gpu-device, packaging the
> > hw-display-virtio-gpu module as a separate package that may or may not
> > be installed along with the qemu package leads to problems. Namely if
> > the hw-display-virtio-gpu is absent, qemu continues to advertise
> > virtio-gpu-ccw, but it aborts not only when one attempts using
> > virtio-gpu-ccw, but also when libvirtd's capability probing tries
> > to instantiate the type to introspect it.
> > 
> > Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
> > is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
> > was chosen because it is not a portable device.
> > 
> > With virtio-gpu-ccw built as a module, the correct way to package a
> > modularized qemu is to require that hw-display-virtio-gpu must be
> > installed whenever the module hw-s390x-virtio-gpu-ccw.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > ---
> >   hw/s390x/meson.build | 17 ++++++++++++++++-
> >   util/module.c        |  1 +
> >   2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> > index 2a7818d94b..153b1309fb 100644
> > --- a/hw/s390x/meson.build
> > +++ b/hw/s390x/meson.build
> > @@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
> >   virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: files('virtio-ccw-balloon.c'))
> >   virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-ccw-blk.c'))
> >   virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('virtio-ccw-crypto.c'))
> > -virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: files('virtio-ccw-gpu.c'))
> >   virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: files('virtio-ccw-input.c'))
> >   virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-ccw-net.c'))
> >   virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
> > @@ -46,3 +45,19 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'
> >   s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
> >   
> >   hw_arch += {'s390x': s390x_ss}
> > +
> > +if target.startswith('s390x')
> > +  hw_s390x_modules = {}
> > +
> > +  hw_s390x_modules_c_args = ['-DNEED_CPU_H',
> > +	      '-DCONFIG_TARGET="@0@-config-target.h"'.format(target)]
> > +  hw_s390x_modules_inc = [include_directories('../../target' / config_target['TARGET_BASE_ARCH'])]
> > +  hw_s390x_modules_dependencies = declare_dependency(
> > +	       include_directories: hw_s390x_modules_inc, compile_args: hw_s390x_modules_c_args)  
> 
> Basically the patch looks fine to me, but I wonder why all that above lines 
> (related to hw_s390x_modules_dependencies) are requred at all? The other 
> display modules in hw/display/meson.build also do not need to re-define 
> c_args for example?

The explanation is simple. Unlike most devices, the ccw devices aren't
portable. In particular both css.c and css.h includes "cpu.h", and
virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains:
#ifdef NEED_CPU_H
#include CONFIG_TARGET
#else
#include "exec/poison.h"
#endif
so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and
CONFIG_KVM is used in "css.h". Frankly, I can't tell under what circumstances
does css need "cpu.h". I managed to build the s390x-softmmu target
without it, but decided to put it back. Regarding "osdep.h", I just
assumed includes are done the way they are done for a good reason. Maybe
the includes can be changed in a way that the things you ask about become
unnecessary, but with the code as is they are necessary. Try to drop them
and check out what happens.

Regards,
Halil

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Cornelia Huck]

On Thu, 18 Feb 2021 11:34:38 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu, 18 Feb 2021 10:23:16 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > > Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
> > > module, which provides the type virtio-gpu-device, packaging the
> > > hw-display-virtio-gpu module as a separate package that may or may not
> > > be installed along with the qemu package leads to problems. Namely if
> > > the hw-display-virtio-gpu is absent, qemu continues to advertise
> > > virtio-gpu-ccw, but it aborts not only when one attempts using
> > > virtio-gpu-ccw, but also when libvirtd's capability probing tries
> > > to instantiate the type to introspect it.
> > > 
> > > Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
> > > is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
> > > was chosen because it is not a portable device.
> > > 
> > > With virtio-gpu-ccw built as a module, the correct way to package a
> > > modularized qemu is to require that hw-display-virtio-gpu must be
> > > installed whenever the module hw-s390x-virtio-gpu-ccw.
> > > 
> > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > ---
> > >   hw/s390x/meson.build | 17 ++++++++++++++++-
> > >   util/module.c        |  1 +
> > >   2 files changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> > > index 2a7818d94b..153b1309fb 100644
> > > --- a/hw/s390x/meson.build
> > > +++ b/hw/s390x/meson.build
> > > @@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
> > >   virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: files('virtio-ccw-balloon.c'))
> > >   virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-ccw-blk.c'))
> > >   virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('virtio-ccw-crypto.c'))
> > > -virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: files('virtio-ccw-gpu.c'))
> > >   virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: files('virtio-ccw-input.c'))
> > >   virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-ccw-net.c'))
> > >   virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
> > > @@ -46,3 +45,19 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'
> > >   s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
> > >   
> > >   hw_arch += {'s390x': s390x_ss}
> > > +
> > > +if target.startswith('s390x')
> > > +  hw_s390x_modules = {}
> > > +
> > > +  hw_s390x_modules_c_args = ['-DNEED_CPU_H',
> > > +	      '-DCONFIG_TARGET="@0@-config-target.h"'.format(target)]
> > > +  hw_s390x_modules_inc = [include_directories('../../target' / config_target['TARGET_BASE_ARCH'])]
> > > +  hw_s390x_modules_dependencies = declare_dependency(
> > > +	       include_directories: hw_s390x_modules_inc, compile_args: hw_s390x_modules_c_args)    
> > 
> > Basically the patch looks fine to me, but I wonder why all that above lines 
> > (related to hw_s390x_modules_dependencies) are requred at all? The other 
> > display modules in hw/display/meson.build also do not need to re-define 
> > c_args for example?  
> 
> The explanation is simple. Unlike most devices, the ccw devices aren't
> portable. In particular both css.c and css.h includes "cpu.h", and
> virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains:
> #ifdef NEED_CPU_H
> #include CONFIG_TARGET
> #else
> #include "exec/poison.h"
> #endif
> so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and
> CONFIG_KVM is used in "css.h". Frankly, I can't tell under what circumstances
> does css need "cpu.h". 

s390_crw_mchk() and s390_io_interrupt() are in cpu.h. Nowadays, they
use the flic to inject interrupts; but their earlier implementations
had a dummy cpu state.

I'm wondering whether s390_flic.h is a better place for functions
injecting floating interrupts, now that we don't have the non-flic
support anymore.

> I managed to build the s390x-softmmu target
> without it, but decided to put it back. Regarding "osdep.h", I just
> assumed includes are done the way they are done for a good reason. Maybe
> the includes can be changed in a way that the things you ask about become
> unnecessary, but with the code as is they are necessary. Try to drop them
> and check out what happens.
> 
> Regards,
> Halil
> 

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Gerd Hoffmann]

  Hi,

> > The explanation is simple. Unlike most devices, the ccw devices aren't
> > portable. In particular both css.c and css.h includes "cpu.h", and
> > virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains:
> > #ifdef NEED_CPU_H
> > #include CONFIG_TARGET
> > #else
> > #include "exec/poison.h"
> > #endif
> > so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and
> > CONFIG_KVM is used in "css.h". Frankly, I can't tell under what circumstances
> > does css need "cpu.h". 
> 
> s390_crw_mchk() and s390_io_interrupt() are in cpu.h. Nowadays, they
> use the flic to inject interrupts; but their earlier implementations
> had a dummy cpu state.
> 
> I'm wondering whether s390_flic.h is a better place for functions
> injecting floating interrupts, now that we don't have the non-flic
> support anymore.

Sounds like the easiest way forward.  Alternatively add support for
target-specific modules (which we don't really have right now).

(modular-izing virtio-gpu-ccw makes sense indeed, virtio-gpu-pci is a
module for pretty much the same reasons).

take care,
  Gerd

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Thomas Huth]

On 18/02/2021 11.34, Halil Pasic wrote:
> On Thu, 18 Feb 2021 10:23:16 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>>> Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
>>> module, which provides the type virtio-gpu-device, packaging the
>>> hw-display-virtio-gpu module as a separate package that may or may not
>>> be installed along with the qemu package leads to problems. Namely if
>>> the hw-display-virtio-gpu is absent, qemu continues to advertise
>>> virtio-gpu-ccw, but it aborts not only when one attempts using
>>> virtio-gpu-ccw, but also when libvirtd's capability probing tries
>>> to instantiate the type to introspect it.
>>>
>>> Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
>>> is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
>>> was chosen because it is not a portable device.
>>>
>>> With virtio-gpu-ccw built as a module, the correct way to package a
>>> modularized qemu is to require that hw-display-virtio-gpu must be
>>> installed whenever the module hw-s390x-virtio-gpu-ccw.
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>>> ---
>>>    hw/s390x/meson.build | 17 ++++++++++++++++-
>>>    util/module.c        |  1 +
>>>    2 files changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>>> index 2a7818d94b..153b1309fb 100644
>>> --- a/hw/s390x/meson.build
>>> +++ b/hw/s390x/meson.build
>>> @@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
>>>    virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: files('virtio-ccw-balloon.c'))
>>>    virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-ccw-blk.c'))
>>>    virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('virtio-ccw-crypto.c'))
>>> -virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: files('virtio-ccw-gpu.c'))
>>>    virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: files('virtio-ccw-input.c'))
>>>    virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-ccw-net.c'))
>>>    virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
>>> @@ -46,3 +45,19 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'
>>>    s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
>>>    
>>>    hw_arch += {'s390x': s390x_ss}
>>> +
>>> +if target.startswith('s390x')
>>> +  hw_s390x_modules = {}
>>> +
>>> +  hw_s390x_modules_c_args = ['-DNEED_CPU_H',
>>> +	      '-DCONFIG_TARGET="@0@-config-target.h"'.format(target)]
>>> +  hw_s390x_modules_inc = [include_directories('../../target' / config_target['TARGET_BASE_ARCH'])]
>>> +  hw_s390x_modules_dependencies = declare_dependency(
>>> +	       include_directories: hw_s390x_modules_inc, compile_args: hw_s390x_modules_c_args)
>>
>> Basically the patch looks fine to me, but I wonder why all that above lines
>> (related to hw_s390x_modules_dependencies) are requred at all? The other
>> display modules in hw/display/meson.build also do not need to re-define
>> c_args for example?
> 
> The explanation is simple. Unlike most devices, the ccw devices aren't
> portable. In particular both css.c and css.h includes "cpu.h", and
> virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains:
> #ifdef NEED_CPU_H
> #include CONFIG_TARGET
> #else
> #include "exec/poison.h"
> #endif
> so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and
> CONFIG_KVM is used in "css.h". Frankly, I can't tell under what circumstances
> does css need "cpu.h".
As far as I can see, the only real reason right now is the CONFIG_KVM 
section in css.h. I think you could simply move that into another header 
file instead (cpu.h ?)

  Thomas

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Halil Pasic]

On Thu, 18 Feb 2021 14:38:20 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > The explanation is simple. Unlike most devices, the ccw devices aren't
> > > portable. In particular both css.c and css.h includes "cpu.h", and
> > > virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains:
> > > #ifdef NEED_CPU_H
> > > #include CONFIG_TARGET
> > > #else
> > > #include "exec/poison.h"
> > > #endif
> > > so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and
> > > CONFIG_KVM is used in "css.h". Frankly, I can't tell under what circumstances
> > > does css need "cpu.h".   
> > 
> > s390_crw_mchk() and s390_io_interrupt() are in cpu.h. Nowadays, they
> > use the flic to inject interrupts; but their earlier implementations
> > had a dummy cpu state.
> > 
> > I'm wondering whether s390_flic.h is a better place for functions
> > injecting floating interrupts, now that we don't have the non-flic
> > support anymore.  
> 
> Sounds like the easiest way forward.  

I believe the easiest way forward is what I propose in this patch. Does
this mean, that the code I propose here is not viable? If yes, why?

I'm not against cleaning up the includes for virtio-ccw devices, but I
tend to see that as a separate, less pressing issue.

> Alternatively add support for
> target-specific modules (which we don't really have right now).

I think a target-specific module is what I did in this patch.
Furthermore, I think any virtio-ccw device that is about to be built as
a module, must be built as a target-specific module. If the realized
(guest) architecture is not s390x, then there are no s390 IO instructions
and ccw won't fly. Yes, in theory other architectures could introduce the
exact same stuff, but I don't see that happening.

Even if I were to move the two functions like Connie suggests, I don't
see a way around making virtio-9p-ccw target specific.

Regards,
Halil

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Halil Pasic]

On Thu, 18 Feb 2021 15:44:47 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 18/02/2021 11.34, Halil Pasic wrote:
> > On Thu, 18 Feb 2021 10:23:16 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >>> Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
> >>> module, which provides the type virtio-gpu-device, packaging the
> >>> hw-display-virtio-gpu module as a separate package that may or may not
> >>> be installed along with the qemu package leads to problems. Namely if
> >>> the hw-display-virtio-gpu is absent, qemu continues to advertise
> >>> virtio-gpu-ccw, but it aborts not only when one attempts using
> >>> virtio-gpu-ccw, but also when libvirtd's capability probing tries
> >>> to instantiate the type to introspect it.
> >>>
> >>> Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
> >>> is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
> >>> was chosen because it is not a portable device.
> >>>
> >>> With virtio-gpu-ccw built as a module, the correct way to package a
> >>> modularized qemu is to require that hw-display-virtio-gpu must be
> >>> installed whenever the module hw-s390x-virtio-gpu-ccw.
> >>>
> >>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> >>> ---
> >>>    hw/s390x/meson.build | 17 ++++++++++++++++-
> >>>    util/module.c        |  1 +
> >>>    2 files changed, 17 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> >>> index 2a7818d94b..153b1309fb 100644
> >>> --- a/hw/s390x/meson.build
> >>> +++ b/hw/s390x/meson.build
> >>> @@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
> >>>    virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true: files('virtio-ccw-balloon.c'))
> >>>    virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-ccw-blk.c'))
> >>>    virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('virtio-ccw-crypto.c'))
> >>> -virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: files('virtio-ccw-gpu.c'))
> >>>    virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: files('virtio-ccw-input.c'))
> >>>    virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-ccw-net.c'))
> >>>    virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
> >>> @@ -46,3 +45,19 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'
> >>>    s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
> >>>    
> >>>    hw_arch += {'s390x': s390x_ss}
> >>> +
> >>> +if target.startswith('s390x')
> >>> +  hw_s390x_modules = {}
> >>> +
> >>> +  hw_s390x_modules_c_args = ['-DNEED_CPU_H',
> >>> +	      '-DCONFIG_TARGET="@0@-config-target.h"'.format(target)]
> >>> +  hw_s390x_modules_inc = [include_directories('../../target' / config_target['TARGET_BASE_ARCH'])]
> >>> +  hw_s390x_modules_dependencies = declare_dependency(
> >>> +	       include_directories: hw_s390x_modules_inc, compile_args: hw_s390x_modules_c_args)  
> >>
> >> Basically the patch looks fine to me, but I wonder why all that above lines
> >> (related to hw_s390x_modules_dependencies) are requred at all? The other
> >> display modules in hw/display/meson.build also do not need to re-define
> >> c_args for example?  
> > 
> > The explanation is simple. Unlike most devices, the ccw devices aren't
> > portable. In particular both css.c and css.h includes "cpu.h", and
> > virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains:
> > #ifdef NEED_CPU_H
> > #include CONFIG_TARGET
> > #else
> > #include "exec/poison.h"
> > #endif
> > so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and
> > CONFIG_KVM is used in "css.h". Frankly, I can't tell under what circumstances
> > does css need "cpu.h".  
> As far as I can see, the only real reason right now is the CONFIG_KVM 
> section in css.h. I think you could simply move that into another header 
> file instead (cpu.h ?)
> 

Since everybody seems to agree, that that is the right way to move
forward, I can try moving things around like you and Connie suggested.

Regards,
Halil

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Halil Pasic]

On Thu, 18 Feb 2021 14:38:20 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> > > The explanation is simple. Unlike most devices, the ccw devices aren't
> > > portable. In particular both css.c and css.h includes "cpu.h", and
> > > virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains:
> > > #ifdef NEED_CPU_H
> > > #include CONFIG_TARGET
> > > #else
> > > #include "exec/poison.h"
> > > #endif
> > > so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and
> > > CONFIG_KVM is used in "css.h". Frankly, I can't tell under what circumstances
> > > does css need "cpu.h".   
> > 
> > s390_crw_mchk() and s390_io_interrupt() are in cpu.h. Nowadays, they
> > use the flic to inject interrupts; but their earlier implementations
> > had a dummy cpu state.
> > 
> > I'm wondering whether s390_flic.h is a better place for functions
> > injecting floating interrupts, now that we don't have the non-flic
> > support anymore.  
> 
> Sounds like the easiest way forward.  Alternatively add support for
> target-specific modules (which we don't really have right now).

Thanks Gerd! 

Now I realize what do you mean by support for target-specific modules.
I'm mostly concerned with the s390x targets and I didn't have a good
enough understanding of this. I didn't realize the modules are shared
for all targets, that's why I've tried to build it only for s390x
targets.

I don't see way around target-specific modules. With the modifications
suggested by Thomas and Connie, I was able to get the new module to
compile regardless of the target, but that "fixes" s390x at the expense
of breaking all the other targets. For example:
./qemu-system-x86_64 -device help
Type 'virtio-gpu-ccw' is missing its parent 'virtio-ccw-device'
Aborted
because each target specific qemu will try to load my module. For s390x
it will work as expected, for everybody else not at all.

Making the list of modules in module.c depend on the target, i.e.
something like
     { "vhost-user-gpu-pci",    "hw-", "display-virtio-gpu-pci" }
+#ifdef TARGET_S390X
     { "virtio-gpu-ccw",        "hw-", "s390x-virtio-gpu-ccw"   },
+#endif
doesn't look viable either.

Since you are the author of 28457744c3 ("module: qom module support") and
7b0de5b796 ("virtio-gpu: build modular"), before I start working on
target-specific modules I would like to ask you, what is in your opinion
the best way to implement these?

Regards,
Halil

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Gerd Hoffmann]

  Hi,

> I'm not against cleaning up the includes for virtio-ccw devices, but I
> tend to see that as a separate, less pressing issue.

Well, it would allow to build virtio-ccw as common code (i.e. move from
specific_ss to softmmu_ss).

> > Alternatively add support for
> > target-specific modules (which we don't really have right now).
> 
> I think a target-specific module is what I did in this patch.

Nope.

Specifically target-specific modules must be built once for each
target and have a target-specific name (prefix or subdir).   So
when modularizing -- for example -- vga you'll get modules such as
x86_64/hw-display-vga.so, ppc64/hw-display-vga.so, ...

Qemu needs support for loading the correct version.

What you did is a rather incomplete version of that which happens to
work for ccw because ccw is used by s390x only so you can shortcut
the "build once for each target" part of the problem.

> Furthermore, I think any virtio-ccw device that is about to be built as
> a module, must be built as a target-specific module. If the realized
> (guest) architecture is not s390x, then there are no s390 IO instructions
> and ccw won't fly.

Well, it can happen that generic modules don't load into some qemu
versions.  Devices which need PCI can't be loaded by qemu-system-avr
for example because the avr target lacks PCI bus support.

ccw modules only loading into qemu-system-s390x because that is the
only target providing the bus needed is pretty much the same.

take care,
  Gerd

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Gerd Hoffmann]

  Hi,

> I don't see way around target-specific modules. With the modifications
> suggested by Thomas and Connie, I was able to get the new module to
> compile regardless of the target,

Cool (should have checked all mails before sending replies ...).

> but that "fixes" s390x at the expense
> of breaking all the other targets. For example:
> ./qemu-system-x86_64 -device help
> Type 'virtio-gpu-ccw' is missing its parent 'virtio-ccw-device'
> Aborted

Hmm, this is a new failure variant.  In the PCI case (see other mail)
the module doesn't load in the first place due to missing symbols.

Maybe we need a type_register_mayfail() variant which doesn't abort in
case the parent isn't found (see also commit
501093207eb1ed4845e0a65ee1ce7db7b9676e0b).

HTH,
  Gerd

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Halil Pasic]

On Fri, 19 Feb 2021 09:45:45 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> > but that "fixes" s390x at the expense
> > of breaking all the other targets. For example:
> > ./qemu-system-x86_64 -device help
> > Type 'virtio-gpu-ccw' is missing its parent 'virtio-ccw-device'
> > Aborted  
> 
> Hmm, this is a new failure variant.  In the PCI case (see other mail)
> the module doesn't load in the first place due to missing symbols.
> 
> Maybe we need a type_register_mayfail() variant which doesn't abort in
> case the parent isn't found (see also commit
> 501093207eb1ed4845e0a65ee1ce7db7b9676e0b).

I was also thinking along the same lines last night, and came up with
this workaround:

diff --git a/hw/s390x/virtio-ccw-gpu.c b/hw/s390x/virtio-ccw-gpu.c
index c301e2586b..bbe591cd62 100644
--- a/hw/s390x/virtio-ccw-gpu.c
+++ b/hw/s390x/virtio-ccw-gpu.c
@@ -62,7 +62,16 @@ static const TypeInfo virtio_ccw_gpu = {
 
 static void virtio_ccw_gpu_register(void)
 {
+#ifdef CONFIG_MODULES
+    /*
+     * Ugly hack: Avoid targets that don't have it aborting when this module
+     * is loaded.
+    if (object_class_by_name(TYPE_VIRTIO_CCW_DEVICE)) {
+        type_register_static(&virtio_ccw_gpu);
+    }
+#else
     type_register_static(&virtio_ccw_gpu);
+#endif
 }
 
 type_init(virtio_ccw_gpu_register)

but then I decided it is too ugly to post. Something like
type_register_mayfail() would be certainly nicer, although I would still
prefer the failing type register if the device ain't built as a module.

Today I'm on a vacation so I will pick this up again next week. I will
also think some more of how the situation with virtio-ccw compares to pci
with respect to modularization.

Regards,
Halil

Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw

[Gerd Hoffmann]

  Hi,

> > Maybe we need a type_register_mayfail() variant which doesn't abort in
> > case the parent isn't found (see also commit
> > 501093207eb1ed4845e0a65ee1ce7db7b9676e0b).
> 
> I was also thinking along the same lines last night, and came up with
> this workaround:
> 
> diff --git a/hw/s390x/virtio-ccw-gpu.c b/hw/s390x/virtio-ccw-gpu.c
> index c301e2586b..bbe591cd62 100644
> --- a/hw/s390x/virtio-ccw-gpu.c
> +++ b/hw/s390x/virtio-ccw-gpu.c
> @@ -62,7 +62,16 @@ static const TypeInfo virtio_ccw_gpu = {
>  
>  static void virtio_ccw_gpu_register(void)
>  {
> +#ifdef CONFIG_MODULES
> +    /*
> +     * Ugly hack: Avoid targets that don't have it aborting when this module
> +     * is loaded.
> +    if (object_class_by_name(TYPE_VIRTIO_CCW_DEVICE)) {
> +        type_register_static(&virtio_ccw_gpu);
> +    }
> +#else
>      type_register_static(&virtio_ccw_gpu);
> +#endif
>  }
>  
>  type_init(virtio_ccw_gpu_register)
> 
> but then I decided it is too ugly to post. Something like
> type_register_mayfail() would be certainly nicer, although I would still
> prefer the failing type register if the device ain't built as a module.

type_register_mayfail() could have different behavior depending on
CONFIG_MODULES (and a comment block explaining why).

take care,
  Gerd