[SeaBIOS] Re: [PATCH] virtio: finalizing features before using device

[Michael S. Tsirkin]

On Wed, Nov 09, 2022 at 09:17:25PM +0800, Xuan Zhuo wrote:
> Under the standard of Virtio 1.0, the initialization process of the
> device must first write sub-features back to device before
> using device, such as finding vqs.
>
> There are four places using vp_find_vq().
> virtio-blk.pci:    put the finalizing features code in front of using device
> virtio-blk.mmio:   put the finalizing features code in front of using device
> virtio-scsi.pci:   is ok
> virtio-scsi.mmio:  add set features before vp_find_vq()
>
> Signed-off-by: Xuan Zhuo

Please confirm you tested this with both modern and legacy
and all these device types. Because e.g. scsi does not look
like it would work.
> src/hw/virtio-blk.c  | 23 ++++++++++++++---------
>  src/hw/virtio-scsi.c | 18 ++++++++++++++++++
>  2 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c
> index 9b4a05a..52eb08f 100644
> --- a/src/hw/virtio-blk.c
> +++ b/src/hw/virtio-blk.c
> @@ -151,10 +151,6 @@ init_virtio_blk(void *data)
>      vdrive->drive.cntl_id = pci->bdf;
>  vp_init_simple(&vdrive->vp, pci);
>
> if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
>    dprintf(1, "fail to find vq for virtio-blk %pP\n", pci);
>    goto fail;
>
> }
>
> if (vdrive->vp.use_modern) {
>     struct vp_device *vp = &vdrive->vp;
>
> @@ -212,7 +208,15 @@ init_virtio_blk(void *data)
>              vp_read(&vp->device, struct virtio_blk_config, heads);
>          vdrive->drive.pchs.sector =
>              vp_read(&vp->device, struct virtio_blk_config, sectors);
> } else {
> }

Why two empty lines?
> if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
>    dprintf(1, "fail to find vq for virtio-blk %pP\n", pci);
>    goto fail;
> }
> if (!vdrive->vp.use_modern) {
>     struct virtio_blk_config cfg;
>     vp_get_legacy(&vdrive->vp, 0, &cfg, sizeof(cfg));
>
> @@ -272,10 +276,6 @@ init_virtio_blk_mmio(void *mmio)
>      vdrive->drive.cntl_id = (u32)mmio;
>  vp_init_mmio(&vdrive->vp, mmio);
>
> if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
>    dprintf(1, "fail to find vq for virtio-blk-mmio %p\n", mmio);
>    goto fail;
>
> }
>
> struct vp_device *vp = &vdrive->vp;
> u64 features = vp_get_features(vp);
>
> @@ -294,6 +294,11 @@ init_virtio_blk_mmio(void *mmio)
>          goto fail;
>      }
> if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
>    dprintf(1, "fail to find vq for virtio-blk-mmio %p\n", mmio);
>    goto fail;
> }
> if (features & max_segment_size)
>        vdrive->drive.max_segment_size =
>            vp_read(&vp->device, struct virtio_blk_config, size_max);
>
> diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c
> index 369c981..cf131fb 100644
> --- a/src/hw/virtio-scsi.c
> +++ b/src/hw/virtio-scsi.c
> @@ -239,6 +239,24 @@ init_virtio_scsi_mmio(void *mmio)
>      vp_init_mmio(vp, mmio);
>      u8 status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> {
>    u64 features = vp_get_features(vp);
>    u64 version1 = 1ull << VIRTIO_F_VERSION_1;
>    u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
>    if (!(features & version1)) {
>        dprintf(1, "modern device without virtio_1 feature bit: %pP\n", mmio);
>        goto fail;
>    }
>    vp_set_features(vp, features & (version1 | iommu_platform));
>    status |= VIRTIO_CONFIG_S_FEATURES_OK;
>    vp_set_status(vp, status);
>    if (!(vp_get_status(vp) & VIRTIO_CONFIG_S_FEATURES_OK)) {
>        dprintf(1, "device didn't accept features: %pP\n", mmio);
>        goto fail;
>    }
> }

This extra scope is quite ugly.
> if (vp_find_vq(vp, 2, &vq) < 0 ) {
>        dprintf(1, "fail to find vq for virtio-scsi-mmio %p\n", mmio);
>        goto fail;

I don't get it. Don't you need to test use_modern? And where is
the code coming from?
>