|
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
[Prev in Thread] | Current Thread | [Next in Thread] |