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

Gerd Hoffmann posted 3 patches 4 years, 8 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210317095622.2839895-1-kraxel@redhat.com
Maintainers: Cornelia Huck <cohuck@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Christian Borntraeger <borntraeger@de.ibm.com>, Thomas Huth <thuth@redhat.com>, David Hildenbrand <david@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Halil Pasic <pasic@linux.ibm.com>
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(-)
[PATCH 0/3] s390x: modularize virtio-gpu-ccw
Posted by Gerd Hoffmann 4 years, 8 months ago
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(-)

-- 
2.30.2



Re: [PATCH 0/3] s390x: modularize virtio-gpu-ccw
Posted by Halil Pasic 4 years, 8 months ago
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

Re: [PATCH 0/3] s390x: modularize virtio-gpu-ccw
Posted by Gerd Hoffmann 4 years, 7 months ago
  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