mbox series

[0/3] s390x: modularize virtio-gpu-ccw

Message ID 20210317095622.2839895-1-kraxel@redhat.com
Headers show
Series s390x: modularize virtio-gpu-ccw | expand

Message

Gerd Hoffmann March 17, 2021, 9:56 a.m. UTC 
Maybe not the most elegant but rather simple approach to the "parent
object missing" problem: Use a symbol reference to make sure ccw modules
load only in case ccw support is present.

Also split the cpu changes to a separate patch.

Gerd Hoffmann (3):
  s390x: move S390_ADAPTER_SUPPRESSIBLE
  s390x: add have_virtio_ccw
  s390x: modularize virtio-gpu-ccw

 hw/s390x/virtio-ccw.h        | 5 +++++
 include/hw/s390x/css.h       | 7 -------
 include/hw/s390x/s390_flic.h | 3 +++
 target/s390x/cpu.h           | 9 ++++++---
 hw/s390x/virtio-ccw-gpu.c    | 4 +++-
 hw/s390x/virtio-ccw.c        | 2 ++
 util/module.c                | 1 +
 hw/s390x/meson.build         | 8 +++++++-
 8 files changed, 27 insertions(+), 12 deletions(-)

Comments

Halil Pasic March 22, 2021, 1:12 p.m. UTC | #1 
On Wed, 17 Mar 2021 10:56:19 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> Maybe not the most elegant but rather simple approach to the "parent
> object missing" problem: Use a symbol reference to make sure ccw modules
> load only in case ccw support is present.

[..]

Hi Gerd! I've tested this and I think mostly does what it should. The
only thing that I'm inclined to consider a little quirky is the first
message:

$ ./qemu-system-x86_64 -device virtio-gpu-ccw
Failed to open module: /home/pasic/build/qemu/hw-s390x-virtio-gpu-ccw.so: undefined symbol: have_virtio_ccw
qemu-system-x86_64: -device virtio-gpu-ccw: 'virtio-gpu-ccw' is not a valid device model name

But with something like --help we don't see it, and I assume neither do
we when probing, because there the modules are loaded with mayfail. So
for me this is acceptable, because it happens only if one tries to use a
device that ain't advertised.

Compared to Daniels suggestion, I find that one conceptually clearer,
and even more flexible. Implementation-wise I find this patch-set
simpler. I don't know how would it scale to modules depending on modules
(and it feels a little hackish), but we can address such problems as they
emerge if they emerge, so I did not think too hard.

Let me also note, that you took authorship of all three patches, which
I'm fine with. All I really care about at this stage is getting this
fixed in a remotely sane way, and this is definitely one. I'm also
willing to invest more work in that symlink based approach, if that is
what the community wants, but I would prefer this fixed as soon as
possible.

If you keep the authorship for all the patches, I would like to offer:
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
Tested-by: Halil Pasic <pasic@linux.ibm.com>
for all three patches. (If I'm going to be the author of some of the
patches, those tags don't make sense for those).

Thanks for your work!

Regards,
Halil
Gerd Hoffmann March 26, 2021, 8:40 a.m. UTC | #2 
Hi,

> Compared to Daniels suggestion, I find that one conceptually clearer,
> and even more flexible. Implementation-wise I find this patch-set
> simpler.

Given we are in fixes-only mode I think we should go for the simple
solution.  Should be easy enough to revert in case we want replace
this with something else after the 6.0 release.

> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> Tested-by: Halil Pasic <pasic@linux.ibm.com>

Added & queued.
CI running, pull req later today when all goes well.

thanks,
  Gerd