[PATCH] virtio: finalizing features before using device

[Xuan Zhuo]

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

1. virtio-blk.pci:    put the finalizing features code in front of using device
2. virtio-blk.mmio:   put the finalizing features code in front of using device
3. virtio-scsi.pci:   is ok
4. virtio-scsi.mmio:  add set features before vp_find_vq()

Signed-off-by: Xuan Zhuo 
---
 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 {
+    }
+
+
+    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;
+        }
+    }
+
     if (vp_find_vq(vp, 2, &vq) < 0 ) {
         dprintf(1, "fail to find vq for virtio-scsi-mmio %p\n", mmio);
         goto fail;

[Paul Menzel]

Dear Xuan,

Thank you for your patch.

Am 09.11.22 um 14:17 schrieb Xuan Zhuo:

A small nit for the summary/title (subject), maybe use imperative mood 
*finalize*.
> 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()

Did you hit an error without the patch? If so, could you please document 
how to reproduce the issue?

[…]

Kind regards,

Paul

[Xuan Zhuo]

On Wed, 9 Nov 2022 14:24:06 +0100, Paul Menzel  wrote:
> Dear Xuan,
>
> Thank you for your patch.
>
> Am 09.11.22 um 14:17 schrieb Xuan Zhuo:
>
> A small nit for the summary/title (subject), maybe use imperative mood
> *finalize*.

OK.
> > 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()
>
> Did you hit an error without the patch? If so, could you please document
> how to reproduce the issue?
>
> […]

https://www.mail-archive.com/[e-mail]/msg920776.html

qemu with patch https://www.mail-archive.com/[e-mail]/msg920538.html

Then:
$ build/qemu-system-x86_64 -M accel=kvm -m 1G -cpu host -blockdev
file,node-name=drive0,filename=test.img -device
virtio-blk-pci,drive=drive0
qemu: queue_enable is only suppported in devices of virtio 1.0 or later.

Thanks
> Kind regards,
>
> Paul

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

[Xuan Zhuo]

On Wed, 9 Nov 2022 08:28:00 -0500, "Michael S. Tsirkin"  wrote:
> 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.

I have tested this: virtio-blk.pci

I tried to start virtio-blk and virtio-scsi with MMIO, but it was not
successful.

I want to know how to start Virtio-BLK and Virtio-SCSI based on MMIO?
> > 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?

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

Will fix.
> > 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?

Do we need to distinguish between modern for mmio? Is there any error in my
understanding?

Thanks.
> >

[Michael S. Tsirkin]

On Wed, Nov 09, 2022 at 09:59:17PM +0800, Xuan Zhuo wrote:
> On Wed, 9 Nov 2022 08:28:00 -0500, "Michael S. Tsirkin"  wrote:
> > 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.
>
> I have tested this: virtio-blk.pci

both modern and legacy? and scsi?
> I tried to start virtio-blk and virtio-scsi with MMIO, but it was not
> successful.
>
> I want to know how to start Virtio-BLK and Virtio-SCSI based on MMIO?

with microvm
> > > 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?
>
> Will fix.
> > > 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.
>
> Will fix.
> > > 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?
>
> Do we need to distinguish between modern for mmio? Is there any error in my
> understanding?
>
> Thanks.
> > >

[Laszlo Ersek]

On 11/09/22 14:59, Xuan Zhuo wrote:
> On Wed, 9 Nov 2022 08:28:00 -0500, "Michael S. Tsirkin"  wrote:
> > 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.
>
> I have tested this: virtio-blk.pci
>
> I tried to start virtio-blk and virtio-scsi with MMIO, but it was not
> successful.
>
> I want to know how to start Virtio-BLK and Virtio-SCSI based on MMIO?
> > > 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?
>
> Will fix.
> > > 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.
>
> Will fix.
> > > 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?
>
> Do we need to distinguish between modern for mmio? Is there any error in my
> understanding?

I think Gerd can comment well on MMIO vs. virtio-1.0.

In edk2, we had originally implemented the following combinations (over
time):

- virtio-0.9.5 over MMIO, usable with qemu-system-(arm|aarch64), "virt"
machine

- virtio-0.9.5 over PCI, usable with qemu-system-(arm|aarch64), "virt"
machine, *if* you configure a PCIe host, and also usable (of course)
with qemu-system-(i386|x86_64) with the "pc" and "q35" machine types.

- virtio-1.0 over PCI (same scenarios as in the previous bullet).

I never implemented virtio-1.0 over MMIO because it looked complicated
with no actual use case -- after aarch64/virt started supporting PCIe,
there was no real need to use MMIO, ever. So if you wanted virtio-1.0,
you were forced to use PCI on aarch64 as well, but that was fine (IMO).

However, Gerd then invented the MicroVM (x86) machine type, which wanted
virtio-1.0 over MMIO. Gerd added the necessary *transport* code to edk2
(I was surprised -- I thought it would be much more complex, and IIRC,
no *device driver* was necessary to modify).

In fact I think the MicroVM machine type is the *only* option for
exercising the virtio-mmio transport on x86. (And because this is about
SeaBIOS, you certainly want x86, not aarch64/virt!) But I'm unsure if
MicroVM uses SeaBIOS at all -- Gerd can tell best.

At least in theory, the test matrix is large:
- PCI vs. MMIO transport
- virtio-0.9.5 (legacy) vs. virtio-1.0 (modern) spec
- blk vs. scsi device model

8 cases.

(With edk2, the test matrix was even larger, due to (a) supporting more
architectures than SeaBIOS (+aarch64), (b) supporting many more device
types (+rng +net +gpu +fs), (c) supporting guest memory encryption with
SEV on x86. But that matrix was *also smaller* in a sense, because
virtio-1.0 in combination with MMIO was not meant to be supported --
until Gerd just went ahead and implemented it, that is :))

Laszlo

[Michael S. Tsirkin]

On Wed, Nov 09, 2022 at 03:33:21PM +0100, Laszlo Ersek wrote:
> On 11/09/22 14:59, Xuan Zhuo wrote:
> > On Wed, 9 Nov 2022 08:28:00 -0500, "Michael S. Tsirkin"  wrote:
> > > 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.
> >
> > I have tested this: virtio-blk.pci
> >
> > I tried to start virtio-blk and virtio-scsi with MMIO, but it was not
> > successful.
> >
> > I want to know how to start Virtio-BLK and Virtio-SCSI based on MMIO?
> > > > 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?
> >
> > Will fix.
> > > > 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.
> >
> > Will fix.
> > > > 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?
> >
> > Do we need to distinguish between modern for mmio? Is there any error in my
> > understanding?
>
> I think Gerd can comment well on MMIO vs. virtio-1.0.
>
> In edk2, we had originally implemented the following combinations (over
> time):
> virtio-0.9.5 over MMIO, usable with qemu-system-(arm|aarch64), "virt"
>
> machine
> virtio-0.9.5 over PCI, usable with qemu-system-(arm|aarch64), "virt"
>
> machine, *if* you configure a PCIe host, and also usable (of course)
> with qemu-system-(i386|x86_64) with the "pc" and "q35" machine types.
> virtio-1.0 over PCI (same scenarios as in the previous bullet).
>
> I never implemented virtio-1.0 over MMIO because it looked complicated
> with no actual use case -- after aarch64/virt started supporting PCIe,
> there was no real need to use MMIO, ever. So if you wanted virtio-1.0,
> you were forced to use PCI on aarch64 as well, but that was fine (IMO).
>
> However, Gerd then invented the MicroVM (x86) machine type, which wanted
> virtio-1.0 over MMIO. Gerd added the necessary *transport* code to edk2
> (I was surprised -- I thought it would be much more complex, and IIRC,
> no *device driver* was necessary to modify).
>
> In fact I think the MicroVM machine type is the *only* option for
> exercising the virtio-mmio transport on x86. (And because this is about
> SeaBIOS, you certainly want x86, not aarch64/virt!) But I'm unsure if
> MicroVM uses SeaBIOS at all -- Gerd can tell best.
>
> At least in theory, the test matrix is large:
> PCI vs. MMIO transport
> virtio-0.9.5 (legacy) vs. virtio-1.0 (modern) spec
> blk vs. scsi device model
>
> 8 cases.
>
> (With edk2, the test matrix was even larger, due to (a) supporting more
> architectures than SeaBIOS (+aarch64), (b) supporting many more device
> types (+rng +net +gpu +fs), (c) supporting guest memory encryption with
> SEV on x86. But that matrix was *also smaller* in a sense, because
> virtio-1.0 in combination with MMIO was not meant to be supported --
> until Gerd just went ahead and implemented it, that is :))
>
> Laszlo

BTW, it is not strictly necessary to keep find vqs
after set features in legacy mode. Linux switched
to setting features first back in 2009 and that
works fine - legacy spec was more a de-facto thing anyway.

[Xuan Zhuo]

On Wed, 9 Nov 2022 09:43:57 -0500, "Michael S. Tsirkin"  wrote:
> On Wed, Nov 09, 2022 at 03:33:21PM +0100, Laszlo Ersek wrote:
> > On 11/09/22 14:59, Xuan Zhuo wrote:
> > > On Wed, 9 Nov 2022 08:28:00 -0500, "Michael S. Tsirkin"  wrote:
> > > > 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.
> > >
> > > I have tested this: virtio-blk.pci
> > >
> > > I tried to start virtio-blk and virtio-scsi with MMIO, but it was not
> > > successful.
> > >
> > > I want to know how to start Virtio-BLK and Virtio-SCSI based on MMIO?
> > > > > 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?
> > >
> > > Will fix.
> > > > > 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.
> > >
> > > Will fix.
> > > > > 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?
> > >
> > > Do we need to distinguish between modern for mmio? Is there any error in my
> > > understanding?
> >
> > I think Gerd can comment well on MMIO vs. virtio-1.0.
> >
> > In edk2, we had originally implemented the following combinations (over
> > time):
> > virtio-0.9.5 over MMIO, usable with qemu-system-(arm|aarch64), "virt"
> >
> > machine
> > virtio-0.9.5 over PCI, usable with qemu-system-(arm|aarch64), "virt"
> >
> > machine, *if* you configure a PCIe host, and also usable (of course)
> > with qemu-system-(i386|x86_64) with the "pc" and "q35" machine types.
> > virtio-1.0 over PCI (same scenarios as in the previous bullet).
> >
> > I never implemented virtio-1.0 over MMIO because it looked complicated
> > with no actual use case -- after aarch64/virt started supporting PCIe,
> > there was no real need to use MMIO, ever. So if you wanted virtio-1.0,
> > you were forced to use PCI on aarch64 as well, but that was fine (IMO).
> >
> > However, Gerd then invented the MicroVM (x86) machine type, which wanted
> > virtio-1.0 over MMIO. Gerd added the necessary *transport* code to edk2
> > (I was surprised -- I thought it would be much more complex, and IIRC,
> > no *device driver* was necessary to modify).
> >
> > In fact I think the MicroVM machine type is the *only* option for
> > exercising the virtio-mmio transport on x86. (And because this is about
> > SeaBIOS, you certainly want x86, not aarch64/virt!) But I'm unsure if
> > MicroVM uses SeaBIOS at all -- Gerd can tell best.
> >
> > At least in theory, the test matrix is large:
> > PCI vs. MMIO transport
> > virtio-0.9.5 (legacy) vs. virtio-1.0 (modern) spec
> > blk vs. scsi device model
> >
> > 8 cases.
> >
> > (With edk2, the test matrix was even larger, due to (a) supporting more
> > architectures than SeaBIOS (+aarch64), (b) supporting many more device
> > types (+rng +net +gpu +fs), (c) supporting guest memory encryption with
> > SEV on x86. But that matrix was *also smaller* in a sense, because
> > virtio-1.0 in combination with MMIO was not meant to be supported --
> > until Gerd just went ahead and implemented it, that is :))
> >
> > Laszlo
>
> BTW, it is not strictly necessary to keep find vqs
> after set features in legacy mode. Linux switched
> to setting features first back in 2009 and that
> works fine - legacy spec was more a de-facto thing anyway.

So we can actually put the setting features directly in the front without
checking modern(VERSION_1).

Is that so?

Thanks.
>

[Michael S. Tsirkin]

On Thu, Nov 10, 2022 at 10:31:37AM +0800, Xuan Zhuo wrote:
> On Wed, 9 Nov 2022 09:43:57 -0500, "Michael S. Tsirkin"  wrote:
> > On Wed, Nov 09, 2022 at 03:33:21PM +0100, Laszlo Ersek wrote:
> > > On 11/09/22 14:59, Xuan Zhuo wrote:
> > > > On Wed, 9 Nov 2022 08:28:00 -0500, "Michael S. Tsirkin"  wrote:
> > > > > 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.
> > > >
> > > > I have tested this: virtio-blk.pci
> > > >
> > > > I tried to start virtio-blk and virtio-scsi with MMIO, but it was not
> > > > successful.
> > > >
> > > > I want to know how to start Virtio-BLK and Virtio-SCSI based on MMIO?
> > > > > > 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?
> > > >
> > > > Will fix.
> > > > > > 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.
> > > >
> > > > Will fix.
> > > > > > 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?
> > > >
> > > > Do we need to distinguish between modern for mmio? Is there any error in my
> > > > understanding?
> > >
> > > I think Gerd can comment well on MMIO vs. virtio-1.0.
> > >
> > > In edk2, we had originally implemented the following combinations (over
> > > time):
> > > virtio-0.9.5 over MMIO, usable with qemu-system-(arm|aarch64), "virt"
> > >
> > > machine
> > > virtio-0.9.5 over PCI, usable with qemu-system-(arm|aarch64), "virt"
> > >
> > > machine, *if* you configure a PCIe host, and also usable (of course)
> > > with qemu-system-(i386|x86_64) with the "pc" and "q35" machine types.
> > > virtio-1.0 over PCI (same scenarios as in the previous bullet).
> > >
> > > I never implemented virtio-1.0 over MMIO because it looked complicated
> > > with no actual use case -- after aarch64/virt started supporting PCIe,
> > > there was no real need to use MMIO, ever. So if you wanted virtio-1.0,
> > > you were forced to use PCI on aarch64 as well, but that was fine (IMO).
> > >
> > > However, Gerd then invented the MicroVM (x86) machine type, which wanted
> > > virtio-1.0 over MMIO. Gerd added the necessary *transport* code to edk2
> > > (I was surprised -- I thought it would be much more complex, and IIRC,
> > > no *device driver* was necessary to modify).
> > >
> > > In fact I think the MicroVM machine type is the *only* option for
> > > exercising the virtio-mmio transport on x86. (And because this is about
> > > SeaBIOS, you certainly want x86, not aarch64/virt!) But I'm unsure if
> > > MicroVM uses SeaBIOS at all -- Gerd can tell best.
> > >
> > > At least in theory, the test matrix is large:
> > > PCI vs. MMIO transport
> > > virtio-0.9.5 (legacy) vs. virtio-1.0 (modern) spec
> > > blk vs. scsi device model
> > >
> > > 8 cases.
> > >
> > > (With edk2, the test matrix was even larger, due to (a) supporting more
> > > architectures than SeaBIOS (+aarch64), (b) supporting many more device
> > > types (+rng +net +gpu +fs), (c) supporting guest memory encryption with
> > > SEV on x86. But that matrix was *also smaller* in a sense, because
> > > virtio-1.0 in combination with MMIO was not meant to be supported --
> > > until Gerd just went ahead and implemented it, that is :))
> > >
> > > Laszlo
> >
> > BTW, it is not strictly necessary to keep find vqs
> > after set features in legacy mode. Linux switched
> > to setting features first back in 2009 and that
> > works fine - legacy spec was more a de-facto thing anyway.
>
> So we can actually put the setting features directly in the front without
> checking modern(VERSION_1).
>
> Is that so?
>
> Thanks.

Yes, if that helps.
> >

[Gerd Hoffmann]

Hi,
> I want to know how to start Virtio-BLK and Virtio-SCSI based on MMIO?

qemu	-machine microvm \
    -global virtio-mmio.force-legacy={true,false} \
    -device virtio-scsi-device \

take care,
  Gerd

[Xuan Zhuo]

On Thu, 10 Nov 2022 11:30:17 +0100, Gerd Hoffmann  wrote:
> Hi,
> > I want to know how to start Virtio-BLK and Virtio-SCSI based on MMIO?
>
> qemu	-machine microvm \
>    -global virtio-mmio.force-legacy={true,false} \

Thanks. This help me.
> -device virtio-scsi-device \
>
> take care,
>   Gerd