[PATCH 0/2] pc-bios/s390-ccw: Two fixes for the virtio initialization

[PATCH 0/2] pc-bios/s390-ccw: Two fixes for the virtio initialization

[Thomas Huth]

Two minor fixes for the virtio initialization that I spotted while
looking at the code. These currently do not fix any problem (since
QEMU is quite forgiving here when doing things wrong), but we should
anyway try to follow the virtio specification here.

Thomas Huth (2):
  pc-bios/s390-ccw/virtio: Set missing status bits while initializing
  pc-bios/s390-ccw/virtio: Read device config after feature negotiation

 pc-bios/s390-ccw/virtio.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

-- 
2.31.1

[PATCH 1/2] pc-bios/s390-ccw/virtio: Set missing status bits while initializing

[Thomas Huth]

According chapter "3.1.1 Driver Requirements: Device Initialization"
of the Virtio specification (v1.1), a driver for a device has to set
the ACKNOWLEDGE and DRIVER bits in the status field after resetting
the device. The s390-ccw bios skipped these steps so far and seems
like QEMU never cared. Anyway, it's better to follow the spec, so
let's set these bits now in the right spots, too.

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

diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index 5d2c6e3381..4e85a2eb82 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -220,7 +220,7 @@ int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd)
 void virtio_setup_ccw(VDev *vdev)
 {
     int i, rc, cfg_size = 0;
-    unsigned char status = VIRTIO_CONFIG_S_DRIVER_OK;
+    uint8_t status;
     struct VirtioFeatureDesc {
         uint32_t features;
         uint8_t index;
@@ -234,6 +234,10 @@ void virtio_setup_ccw(VDev *vdev)
 
     run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0, false);
 
+    status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
+    rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false);
+    IPL_assert(rc == 0, "Could not write ACKNOWLEDGE status to host");
+
     switch (vdev->senseid.cu_model) {
     case VIRTIO_ID_NET:
         vdev->nr_vqs = 2;
@@ -253,6 +257,11 @@ void virtio_setup_ccw(VDev *vdev)
     default:
         panic("Unsupported virtio device\n");
     }
+
+    status |= VIRTIO_CONFIG_S_DRIVER;
+    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");
@@ -291,9 +300,10 @@ void virtio_setup_ccw(VDev *vdev)
             run_ccw(vdev, CCW_CMD_SET_VQ, &info, sizeof(info), false) == 0,
             "Cannot set VQ info");
     }
-    IPL_assert(
-        run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false) == 0,
-        "Could not write status to host");
+
+    status |= VIRTIO_CONFIG_S_DRIVER_OK;
+    rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false);
+    IPL_assert(rc == 0, "Could not write DRIVER_OK status to host");
 }
 
 bool virtio_is_supported(SubChannelId schid)
-- 
2.31.1

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

[Thomas Huth]

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.

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");
+
     for (i = 0; i < vdev->nr_vqs; i++) {
         VqInfo info = {
             .queue = (unsigned long long) ring_area + (i * VIRTIO_RING_SIZE),
-- 
2.31.1

Re: [PATCH 1/2] pc-bios/s390-ccw/virtio: Set missing status bits while initializing

[Christian Borntraeger]

Am 23.06.22 um 09:11 schrieb Thomas Huth:
> According chapter "3.1.1 Driver Requirements: Device Initialization"
> of the Virtio specification (v1.1), a driver for a device has to set
> the ACKNOWLEDGE and DRIVER bits in the status field after resetting
> the device. The s390-ccw bios skipped these steps so far and seems
> like QEMU never cared. Anyway, it's better to follow the spec, so
> let's set these bits now in the right spots, too.

I have not tested that but I agree with this

Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>


> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   pc-bios/s390-ccw/virtio.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index 5d2c6e3381..4e85a2eb82 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -220,7 +220,7 @@ int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd)
>   void virtio_setup_ccw(VDev *vdev)
>   {
>       int i, rc, cfg_size = 0;
> -    unsigned char status = VIRTIO_CONFIG_S_DRIVER_OK;
> +    uint8_t status;
>       struct VirtioFeatureDesc {
>           uint32_t features;
>           uint8_t index;
> @@ -234,6 +234,10 @@ void virtio_setup_ccw(VDev *vdev)
>   
>       run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0, false);
>   
> +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
> +    rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false);
> +    IPL_assert(rc == 0, "Could not write ACKNOWLEDGE status to host");
> +
>       switch (vdev->senseid.cu_model) {
>       case VIRTIO_ID_NET:
>           vdev->nr_vqs = 2;
> @@ -253,6 +257,11 @@ void virtio_setup_ccw(VDev *vdev)
>       default:
>           panic("Unsupported virtio device\n");
>       }
> +
> +    status |= VIRTIO_CONFIG_S_DRIVER;
> +    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");
> @@ -291,9 +300,10 @@ void virtio_setup_ccw(VDev *vdev)
>               run_ccw(vdev, CCW_CMD_SET_VQ, &info, sizeof(info), false) == 0,
>               "Cannot set VQ info");
>       }
> -    IPL_assert(
> -        run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false) == 0,
> -        "Could not write status to host");
> +
> +    status |= VIRTIO_CONFIG_S_DRIVER_OK;
> +    rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false);
> +    IPL_assert(rc == 0, "Could not write DRIVER_OK status to host");
>   }
>   
>   bool virtio_is_supported(SubChannelId schid)

Re: [PATCH 1/2] pc-bios/s390-ccw/virtio: Set missing status bits while initializing

[Cornelia Huck]

On Thu, Jun 23 2022, Thomas Huth <thuth@redhat.com> wrote:

> According chapter "3.1.1 Driver Requirements: Device Initialization"
> of the Virtio specification (v1.1), a driver for a device has to set
> the ACKNOWLEDGE and DRIVER bits in the status field after resetting
> the device. The s390-ccw bios skipped these steps so far and seems
> like QEMU never cared. Anyway, it's better to follow the spec, so
> let's set these bits now in the right spots, too.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/virtio.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)

The QEMU implementation seems to be pretty tolerant :)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

[Cornelia Huck]

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.

>
> 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/ ?

> +
>      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? 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.

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

[Thomas Huth]

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

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

[Cornelia Huck]

On Thu, Jun 23 2022, Thomas Huth <thuth@redhat.com> wrote:

> 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. 

Eww. The bad thing about QEMU being so relaxed is that you don't catch
issues like that... but if it works for now, there's at least no
pressure.

> 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.

It's not ugly, it has grown organically :) (just like that overgrown
spot in my garden ;)

Re: [PATCH 1/2] pc-bios/s390-ccw/virtio: Set missing status bits while initializing

[Eric Farman]

On Thu, 2022-06-23 at 09:11 +0200, Thomas Huth wrote:
> According chapter "3.1.1 Driver Requirements: Device Initialization"
> of the Virtio specification (v1.1), a driver for a device has to set
> the ACKNOWLEDGE and DRIVER bits in the status field after resetting
> the device. The s390-ccw bios skipped these steps so far and seems
> like QEMU never cared. Anyway, it's better to follow the spec, so
> let's set these bits now in the right spots, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  pc-bios/s390-ccw/virtio.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index 5d2c6e3381..4e85a2eb82 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -220,7 +220,7 @@ int virtio_run(VDev *vdev, int vqid, VirtioCmd
> *cmd)
>  void virtio_setup_ccw(VDev *vdev)
>  {
>      int i, rc, cfg_size = 0;
> -    unsigned char status = VIRTIO_CONFIG_S_DRIVER_OK;
> +    uint8_t status;
>      struct VirtioFeatureDesc {
>          uint32_t features;
>          uint8_t index;
> @@ -234,6 +234,10 @@ void virtio_setup_ccw(VDev *vdev)
>  
>      run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0, false);
>  
> +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
> +    rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status,
> sizeof(status), false);
> +    IPL_assert(rc == 0, "Could not write ACKNOWLEDGE status to
> host");
> +
>      switch (vdev->senseid.cu_model) {
>      case VIRTIO_ID_NET:
>          vdev->nr_vqs = 2;
> @@ -253,6 +257,11 @@ void virtio_setup_ccw(VDev *vdev)
>      default:
>          panic("Unsupported virtio device\n");
>      }
> +
> +    status |= VIRTIO_CONFIG_S_DRIVER;
> +    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");
> @@ -291,9 +300,10 @@ void virtio_setup_ccw(VDev *vdev)
>              run_ccw(vdev, CCW_CMD_SET_VQ, &info, sizeof(info),
> false) == 0,
>              "Cannot set VQ info");
>      }
> -    IPL_assert(
> -        run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status),
> false) == 0,
> -        "Could not write status to host");
> +
> +    status |= VIRTIO_CONFIG_S_DRIVER_OK;
> +    rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status,
> sizeof(status), false);
> +    IPL_assert(rc == 0, "Could not write DRIVER_OK status to host");
>  }
>  
>  bool virtio_is_supported(SubChannelId schid)