virtio: finalizing features before using device

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

Posted by Xuan Zhuo 1 year, 5 months ago

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 <xuanzhuo@linux.alibaba.com>
---
 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;
-- 
2.32.0.3.g01195cf9f

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Michael S. Tsirkin 1 year, 5 months ago

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().
> 
> 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 <xuanzhuo@linux.alibaba.com>


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?


> -- 
> 2.32.0.3.g01195cf9f

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Xuan Zhuo 1 year, 5 months ago

On Wed, 9 Nov 2022 08:28:00 -0500, "Michael S. Tsirkin" <mst@redhat.com> 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().
> >
> > 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 <xuanzhuo@linux.alibaba.com>
>
>
> 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.


>
>
> > --
> > 2.32.0.3.g01195cf9f
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Gerd Hoffmann 1 year, 5 months ago

  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

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Xuan Zhuo 1 year, 5 months ago

On Thu, 10 Nov 2022 11:30:17 +0100, Gerd Hoffmann <kraxel@redhat.com> 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
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Laszlo Ersek 1 year, 5 months ago

On 11/09/22 14:59, Xuan Zhuo wrote:
> On Wed, 9 Nov 2022 08:28:00 -0500, "Michael S. Tsirkin" <mst@redhat.com> 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().
>>>
>>> 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 <xuanzhuo@linux.alibaba.com>
>>
>>
>> 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

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Michael S. Tsirkin 1 year, 5 months ago

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" <mst@redhat.com> 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().
> >>>
> >>> 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 <xuanzhuo@linux.alibaba.com>
> >>
> >>
> >> 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.

-- 
MST

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Xuan Zhuo 1 year, 5 months ago

On Wed, 9 Nov 2022 09:43:57 -0500, "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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().
> > >>>
> > >>> 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 <xuanzhuo@linux.alibaba.com>
> > >>
> > >>
> > >> 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.

>
> --
> MST
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Michael S. Tsirkin 1 year, 5 months ago

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" <mst@redhat.com> 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" <mst@redhat.com> 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().
> > > >>>
> > > >>> 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 <xuanzhuo@linux.alibaba.com>
> > > >>
> > > >>
> > > >> 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.

> >
> > --
> > MST
> >

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Michael S. Tsirkin 1 year, 5 months ago

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" <mst@redhat.com> 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().
> > >
> > > 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 <xuanzhuo@linux.alibaba.com>
> >
> >
> > 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.
> 
> 
> >
> >
> > > --
> > > 2.32.0.3.g01195cf9f
> >

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Paul Menzel 1 year, 5 months ago

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

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

[…]


Kind regards,

Paul
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Xuan Zhuo 1 year, 5 months ago

On Wed, 9 Nov 2022 14:24:06 +0100, Paul Menzel <pmenzel@molgen.mpg.de> 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().
> >
> > 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()
>
> Did you hit an error without the patch? If so, could you please document
> how to reproduce the issue?
>
> […]

https://www.mail-archive.com/qemu-devel@nongnu.org/msg920776.html

qemu with patch https://www.mail-archive.com/qemu-devel@nongnu.org/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
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org