qemu-s390x
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/2] pc-bios/s390-ccw/virtio: Read device config after featur


From: Thomas Huth
Subject: Re: [PATCH 2/2] pc-bios/s390-ccw/virtio: Read device config after feature negotiation
Date: Thu, 23 Jun 2022 11:55:53 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0

On 23/06/2022 10.44, Cornelia Huck wrote:
On Thu, Jun 23 2022, Thomas Huth <thuth@redhat.com> wrote:

Feature negotiation should be done first, since some fields in the
config area can depend on the negotiated features and thus should
rather be read afterwards.

I suppose we don't negotiate any features that might affect the size of
the config space? Anyway, restricting ourselves to the minimum length
should be fine.

Actually, even the virtio spec 0.9.5 already talks about VIRTIO_BLK_F_BLK_SIZE and VIRTIO_BLK_F_GEOMETRY being necessary to get the corresponding values in the config space ... so we're currently depending on the good will of QEMU to also provide the values without these feature bits. I'm already thinking about providing a patch to properly request these feature bits in the s390-ccw bios ... but the whole code there is so ugly that I need some time to think about the right steps to clean it up first.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
  pc-bios/s390-ccw/virtio.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index 4e85a2eb82..0e92e994df 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -262,10 +262,6 @@ void virtio_setup_ccw(VDev *vdev)
      rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false);
      IPL_assert(rc == 0, "Could not write DRIVER status to host");
- IPL_assert(
-        run_ccw(vdev, CCW_CMD_READ_CONF, &vdev->config, cfg_size, false) == 0,
-       "Could not get block device configuration");
-
      /* Feature negotiation */
      for (i = 0; i < ARRAY_SIZE(vdev->guest_features); i++) {
          feats.features = 0;
@@ -278,6 +274,9 @@ void virtio_setup_ccw(VDev *vdev)
          IPL_assert(rc == 0, "Could not set features bits");
      }
+ rc = run_ccw(vdev, CCW_CMD_READ_CONF, &vdev->config, cfg_size, false);
+    IPL_assert(rc == 0, "Could not get block device configuration");

Since you move this anyway: s/block device/boot device/ ?

Good idea, thanks, I'll clean that up!

+
      for (i = 0; i < vdev->nr_vqs; i++) {
          VqInfo info = {
              .queue = (unsigned long long) ring_area + (i * VIRTIO_RING_SIZE),

Related to feature negotiation: It seems that the bios currently
supports legacy virtio only, doesn't it?

Right.

It's probably fine for now, but
there might be a way in the future to disable legacy virtio for all
devices in QEMU, so we'll probably want to add virtio-1 support some
day.

Yes, that's certainly one thing we have to look into at one point in time ...

 Thomas





reply via email to

[Prev in Thread] Current Thread [Next in Thread]