[PATCH v1 0/2] virtio: finalize features before using modern device

[Xuan Zhuo]

https://www.mail-archive.com/[e-mail]/msg920776.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.

In this PATCH, it was added to check the FEATURE VIRTIO_1 when enabling queue.
This code should not be executed, but it is reported here. Because Seabios
use the legacy sequence to initialize the modern device.

The driver MUST follow this sequence to initialize a device:
    1. Reset the device.
    2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
    3. Set the DRIVER status bit: the guest OS knows how to drive the device.
> Read device feature bits, and write the subset of feature bits understood by the OS and driver to the

device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration
       fields to check that it can support the device before accepting it.
> Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step.
> Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not

support our subset of features and the device is unusable.
    7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup,
       reading and possibly writing the device’s virtio configuration space, and population of virtqueues.
    8. Set the DRIVER_OK status bit. At this point the device is “live”.

Test with:

qemu-system-x86_64 \
       -M microvm,x-option-roms=off,pit=off,pic=off,isa-serial=off,rtc=off \
       -global virtio-mmio.force-legacy={false,true} \
       -enable-kvm -cpu host -m 512m -smp 2 \
       -nodefaults -no-user-config -nographic \
       -drive id=test,file=./test.img,format=raw,if=none \
       -device virtio-blk-device,drive=test \
       -bios ../../seabios/out/bios.bin \
       -chardev stdio,id=seabios -device isa-debugcon,iobase=0x402,chardev=seabios

qemu-system-x86_64 \
       -M microvm,x-option-roms=off,pit=off,pic=off,isa-serial=off,rtc=off \
       -global virtio-mmio.force-legacy={false,true} \
       -enable-kvm -cpu host -m 512m -smp 2 \
       -nodefaults -no-user-config -nographic \
       -drive id=test,file=./test.img,format=raw,if=none \
       -device virtio-scsi-device \
       -bios ../../seabios/out/bios.bin \
       -chardev stdio,id=seabios -device isa-debugcon,iobase=0x402,chardev=seabios

qemu-system-x86_64 \
       -enable-kvm -cpu host -m 512m -smp 2 \
       -nodefaults -no-user-config -nographic \
       -drive id=test,file=./test.img,format=raw,if=none \
       -device virtio-scsi,disable-legacy={off,on},disable-modern={on,off} \
       -bios ../../seabios/out/bios.bin \
       -chardev stdio,id=seabios -device isa-debugcon,iobase=0x402,chardev=seabios

qemu-system-x86_64 \
       -enable-kvm -cpu host -m 512m -smp 2 \
       -nodefaults -no-user-config -nographic \
       -drive id=test,file=./test.img,format=raw,if=none \
       -device virtio-blk-pci,drive=test,disable-legacy={off,on},disable-modern={on,off} \
       -bios ../../seabios/out/bios.bin \
       -chardev stdio,id=seabios -device isa-debugcon,iobase=0x402,chardev=seabios

Xuan Zhuo (2):
  virtio-mmio: read/write the hi 32 features for mmio
  virtio: finalize features before using device

src/hw/virtio-blk.c  | 22 +++++++++++++---------
 src/hw/virtio-pci.c  |  7 +++++--
 src/hw/virtio-scsi.c | 13 +++++++++++++
 3 files changed, 31 insertions(+), 11 deletions(-)

[PATCH v1 1/2] virtio-mmio: read/write the hi 32 features for mmio

[Xuan Zhuo]

Under mmio, when we read the feature from the device, we should read the
high 32-bit part. Similarly, when writing the feature back, we should
also write back the high 32-bit feature.

Signed-off-by: Xuan Zhuo 
---
 src/hw/virtio-pci.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
index 213c497..89a4f50 100644
--- a/src/hw/virtio-pci.c
+++ b/src/hw/virtio-pci.c
@@ -193,7 +193,8 @@ u64 vp_get_features(struct vp_device *vp)
     if (vp->use_mmio) {
         vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 0);
         f0 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
-        f1 = 0;
+        vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 1);
+        f1 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
     } else if (vp->use_modern) {
         vp_write(&vp->common, virtio_pci_common_cfg, device_feature_select, 0);
         f0 = vp_read(&vp->common, virtio_pci_common_cfg, device_feature);
@@ -214,8 +215,10 @@ void vp_set_features(struct vp_device *vp, u64 features)
     f1 = features >> 32;

if (vp->use_mmio) {
-        vp_write(&vp->common, virtio_mmio_cfg, guest_feature_select, f0);
+        vp_write(&vp->common, virtio_mmio_cfg, guest_feature_select, 0);
         vp_write(&vp->common, virtio_mmio_cfg, guest_feature, f0);
+        vp_write(&vp->common, virtio_mmio_cfg, guest_feature_select, 1);
+        vp_write(&vp->common, virtio_mmio_cfg, guest_feature, f1);
     } else if (vp->use_modern) {
         vp_write(&vp->common, virtio_pci_common_cfg, guest_feature_select, 0);
         vp_write(&vp->common, virtio_pci_common_cfg, guest_feature, f0);

[PATCH v1 1/2] virtio-mmio: read/write the hi 32 features for mmio

[Gerd Hoffmann]

On Mon, Nov 14, 2022 at 11:58:17AM +0800, Xuan Zhuo wrote:
> Under mmio, when we read the feature from the device, we should read the
> high 32-bit part. Similarly, when writing the feature back, we should
> also write back the high 32-bit feature.
> Signed-off-by: Xuan Zhuo
>
> src/hw/virtio-pci.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
> index 213c497..89a4f50 100644
> --- a/src/hw/virtio-pci.c
> +++ b/src/hw/virtio-pci.c
> @@ -193,7 +193,8 @@ u64 vp_get_features(struct vp_device *vp)
>      if (vp->use_mmio) {
>          vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 0);
>          f0 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
>    f1 = 0;
>    vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 1);
>    f1 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);

You need to check the version here, legacy virtio has only 32 feature
bits so you can't read/write f1 like this.

if (vp_read(&vp->common, virtio_mmio_cfg, version) == 2) {
           /* virtio 1.0 */
       } else {
           /* legacy */
       }

(see also vp_find_vq()).

take care,
  Gerd

[PATCH v1 1/2] virtio-mmio: read/write the hi 32 features for mmio

[Michael S. Tsirkin]

On Mon, Nov 14, 2022 at 10:36:52AM +0100, Gerd Hoffmann wrote:
> On Mon, Nov 14, 2022 at 11:58:17AM +0800, Xuan Zhuo wrote:
> > Under mmio, when we read the feature from the device, we should read the
> > high 32-bit part. Similarly, when writing the feature back, we should
> > also write back the high 32-bit feature.
> > Signed-off-by: Xuan Zhuo
> >
> > src/hw/virtio-pci.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
> > index 213c497..89a4f50 100644
> > --- a/src/hw/virtio-pci.c
> > +++ b/src/hw/virtio-pci.c
> > @@ -193,7 +193,8 @@ u64 vp_get_features(struct vp_device *vp)
> >      if (vp->use_mmio) {
> >          vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 0);
> >          f0 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> >    f1 = 0;
> >    vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 1);
> >    f1 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
>
> You need to check the version here, legacy virtio has only 32 feature
> bits so you can't read/write f1 like this.
>    if (vp_read(&vp->common, virtio_mmio_cfg, version) == 2) {
>        /* virtio 1.0 */
>    } else {
>        /* legacy */
>    }
>
> (see also vp_find_vq()).
>
> take care,
>   Gerd

Hmm we won't want to re-read it many times I think.
It probably makes sense to add a new field u8 mmio_version,
and set it in vp_init_simple.

[PATCH v1 1/2] virtio-mmio: read/write the hi 32 features for mmio

[Gerd Hoffmann]

On Mon, Nov 14, 2022 at 05:51:28AM -0500, Michael S. Tsirkin wrote:
> On Mon, Nov 14, 2022 at 10:36:52AM +0100, Gerd Hoffmann wrote:
> > On Mon, Nov 14, 2022 at 11:58:17AM +0800, Xuan Zhuo wrote:
> > > Under mmio, when we read the feature from the device, we should read the
> > > high 32-bit part. Similarly, when writing the feature back, we should
> > > also write back the high 32-bit feature.
> > > Signed-off-by: Xuan Zhuo
> > >
> > > src/hw/virtio-pci.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
> > > index 213c497..89a4f50 100644
> > > --- a/src/hw/virtio-pci.c
> > > +++ b/src/hw/virtio-pci.c
> > > @@ -193,7 +193,8 @@ u64 vp_get_features(struct vp_device *vp)
> > >      if (vp->use_mmio) {
> > >          vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 0);
> > >          f0 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> > >    f1 = 0;
> > >    vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 1);
> > >    f1 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> >
> > You need to check the version here, legacy virtio has only 32 feature
> > bits so you can't read/write f1 like this.
> >    if (vp_read(&vp->common, virtio_mmio_cfg, version) == 2) {
> >        /* virtio 1.0 */
> >    } else {
> >        /* legacy */
> >    }
> >
> > (see also vp_find_vq()).
> >
> > take care,
> >   Gerd
>
> Hmm we won't want to re-read it many times I think.

It's not in any hot path.  It's needed only at initialization time (for
features and queue setup) because the differences between legacy and
modern virtio-mmio are quite small.  So IMHO not required.

take care,
  Gerd

[PATCH v1 1/2] virtio-mmio: read/write the hi 32 features for mmio

[Michael S. Tsirkin]

On Mon, Nov 14, 2022 at 12:37:27PM +0100, Gerd Hoffmann wrote:
> On Mon, Nov 14, 2022 at 05:51:28AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Nov 14, 2022 at 10:36:52AM +0100, Gerd Hoffmann wrote:
> > > On Mon, Nov 14, 2022 at 11:58:17AM +0800, Xuan Zhuo wrote:
> > > > Under mmio, when we read the feature from the device, we should read the
> > > > high 32-bit part. Similarly, when writing the feature back, we should
> > > > also write back the high 32-bit feature.
> > > > Signed-off-by: Xuan Zhuo
> > > >
> > > > src/hw/virtio-pci.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
> > > > index 213c497..89a4f50 100644
> > > > --- a/src/hw/virtio-pci.c
> > > > +++ b/src/hw/virtio-pci.c
> > > > @@ -193,7 +193,8 @@ u64 vp_get_features(struct vp_device *vp)
> > > >      if (vp->use_mmio) {
> > > >          vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 0);
> > > >          f0 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> > > >    f1 = 0;
> > > >    vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 1);
> > > >    f1 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> > >
> > > You need to check the version here, legacy virtio has only 32 feature
> > > bits so you can't read/write f1 like this.
> > >    if (vp_read(&vp->common, virtio_mmio_cfg, version) == 2) {
> > >        /* virtio 1.0 */
> > >    } else {
> > >        /* legacy */
> > >    }
> > >
> > > (see also vp_find_vq()).
> > >
> > > take care,
> > >   Gerd
> >
> > Hmm we won't want to re-read it many times I think.
>
> It's not in any hot path.  It's needed only at initialization time (for
> features and queue setup) because the differences between legacy and
> modern virtio-mmio are quite small.  So IMHO not required.
>
> take care,
>   Gerd

Right but one of the selling points of MMIO is faster boot,
making init time a hot(ter) path (than it would normally be).

[PATCH v1 1/2] virtio-mmio: read/write the hi 32 features for mmio

[Xuan Zhuo]

On Mon, 14 Nov 2022 10:36:52 +0100, Gerd Hoffmann  wrote:
> On Mon, Nov 14, 2022 at 11:58:17AM +0800, Xuan Zhuo wrote:
> > Under mmio, when we read the feature from the device, we should read the
> > high 32-bit part. Similarly, when writing the feature back, we should
> > also write back the high 32-bit feature.
> > Signed-off-by: Xuan Zhuo
> >
> > src/hw/virtio-pci.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
> > index 213c497..89a4f50 100644
> > --- a/src/hw/virtio-pci.c
> > +++ b/src/hw/virtio-pci.c
> > @@ -193,7 +193,8 @@ u64 vp_get_features(struct vp_device *vp)
> >      if (vp->use_mmio) {
> >          vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 0);
> >          f0 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> >    f1 = 0;
> >    vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 1);
> >    f1 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
>
> You need to check the version here, legacy virtio has only 32 feature
> bits so you can't read/write f1 like this.
>    if (vp_read(&vp->common, virtio_mmio_cfg, version) == 2) {
>        /* virtio 1.0 */
>    } else {
>        /* legacy */
>    }

I refer to the implementation of the Linux kernel and confirmed that QEMU's MMIO
Device implementation. Legacy should also support this operation, although it
may return 0. 0.

Hi Michael, do I make mistakes?

Thanks.
> (see also vp_find_vq()).
>
> take care,
>   Gerd

[PATCH v1 1/2] virtio-mmio: read/write the hi 32 features for mmio

[Michael S. Tsirkin]

On Mon, Nov 14, 2022 at 06:52:59PM +0800, Xuan Zhuo wrote:
> On Mon, 14 Nov 2022 10:36:52 +0100, Gerd Hoffmann  wrote:
> > On Mon, Nov 14, 2022 at 11:58:17AM +0800, Xuan Zhuo wrote:
> > > Under mmio, when we read the feature from the device, we should read the
> > > high 32-bit part. Similarly, when writing the feature back, we should
> > > also write back the high 32-bit feature.
> > > Signed-off-by: Xuan Zhuo
> > >
> > > src/hw/virtio-pci.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
> > > index 213c497..89a4f50 100644
> > > --- a/src/hw/virtio-pci.c
> > > +++ b/src/hw/virtio-pci.c
> > > @@ -193,7 +193,8 @@ u64 vp_get_features(struct vp_device *vp)
> > >      if (vp->use_mmio) {
> > >          vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 0);
> > >          f0 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> > >    f1 = 0;
> > >    vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 1);
> > >    f1 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> >
> > You need to check the version here, legacy virtio has only 32 feature
> > bits so you can't read/write f1 like this.
> >    if (vp_read(&vp->common, virtio_mmio_cfg, version) == 2) {
> >        /* virtio 1.0 */
> >    } else {
> >        /* legacy */
> >    }
>
> I refer to the implementation of the Linux kernel and confirmed that QEMU's MMIO
> Device implementation. Legacy should also support this operation, although it
> may return 0. 0.
>
> Hi Michael, do I make mistakes?
>
> Thanks.

Good point. Gerd: in virtio spec (e.g. in 1.0-cs03, 4.2.4 Legacy
interface) it is documented that legacy interface also has
HostFeaturesSel and GuestFeaturesSel registers, at the
same offsets as DeviceFeaturesSel and DriverFeaturesSel.
Accordingly, it should be safe to use these registers
with version == 1. No? Having said that - are there
actual feature bits >= 32 that we want to set in this seabios
version?
> > (see also vp_find_vq()).
> >
> > take care,
> >   Gerd

[PATCH v1 1/2] virtio-mmio: read/write the hi 32 features for mmio

[Gerd Hoffmann]

On Mon, Nov 14, 2022 at 06:30:51AM -0500, Michael S. Tsirkin wrote:
> On Mon, Nov 14, 2022 at 06:52:59PM +0800, Xuan Zhuo wrote:
> > On Mon, 14 Nov 2022 10:36:52 +0100, Gerd Hoffmann  wrote:
> > > On Mon, Nov 14, 2022 at 11:58:17AM +0800, Xuan Zhuo wrote:
> > > > Under mmio, when we read the feature from the device, we should read the
> > > > high 32-bit part. Similarly, when writing the feature back, we should
> > > > also write back the high 32-bit feature.
> > > > Signed-off-by: Xuan Zhuo
> > > >
> > > > src/hw/virtio-pci.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
> > > > index 213c497..89a4f50 100644
> > > > --- a/src/hw/virtio-pci.c
> > > > +++ b/src/hw/virtio-pci.c
> > > > @@ -193,7 +193,8 @@ u64 vp_get_features(struct vp_device *vp)
> > > >      if (vp->use_mmio) {
> > > >          vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 0);
> > > >          f0 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> > > >    f1 = 0;
> > > >    vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 1);
> > > >    f1 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> > >
> > > You need to check the version here, legacy virtio has only 32 feature
> > > bits so you can't read/write f1 like this.
> > >    if (vp_read(&vp->common, virtio_mmio_cfg, version) == 2) {
> > >        /* virtio 1.0 */
> > >    } else {
> > >        /* legacy */
> > >    }
> >
> > I refer to the implementation of the Linux kernel and confirmed that QEMU's MMIO
> > Device implementation. Legacy should also support this operation, although it
> > may return 0. 0.
> >
> > Hi Michael, do I make mistakes?
> >
> > Thanks.
>
> Good point. Gerd: in virtio spec (e.g. in 1.0-cs03, 4.2.4 Legacy
> interface) it is documented that legacy interface also has
> HostFeaturesSel and GuestFeaturesSel registers, at the
> same offsets as DeviceFeaturesSel and DriverFeaturesSel.
> Accordingly, it should be safe to use these registers
> with version == 1. No?

Is the behavior for HostFeaturesSel/GuestFeaturesSel == 1 in legacy mode
defined?  If both specs and code agree that reads return zero I'm fine
with the patch as-is.
> Having said that - are there
> actual feature bits >= 32 that we want to set in this seabios
> version?

No.

take care,
  Gerd

[PATCH v1 1/2] virtio-mmio: read/write the hi 32 features for mmio

[Michael S. Tsirkin]

On Mon, Nov 14, 2022 at 12:41:58PM +0100, Gerd Hoffmann wrote:
> On Mon, Nov 14, 2022 at 06:30:51AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Nov 14, 2022 at 06:52:59PM +0800, Xuan Zhuo wrote:
> > > On Mon, 14 Nov 2022 10:36:52 +0100, Gerd Hoffmann  wrote:
> > > > On Mon, Nov 14, 2022 at 11:58:17AM +0800, Xuan Zhuo wrote:
> > > > > Under mmio, when we read the feature from the device, we should read the
> > > > > high 32-bit part. Similarly, when writing the feature back, we should
> > > > > also write back the high 32-bit feature.
> > > > > Signed-off-by: Xuan Zhuo
> > > > >
> > > > > src/hw/virtio-pci.c | 7 +++++--
> > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
> > > > > index 213c497..89a4f50 100644
> > > > > --- a/src/hw/virtio-pci.c
> > > > > +++ b/src/hw/virtio-pci.c
> > > > > @@ -193,7 +193,8 @@ u64 vp_get_features(struct vp_device *vp)
> > > > >      if (vp->use_mmio) {
> > > > >          vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 0);
> > > > >          f0 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> > > > >    f1 = 0;
> > > > >    vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 1);
> > > > >    f1 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> > > >
> > > > You need to check the version here, legacy virtio has only 32 feature
> > > > bits so you can't read/write f1 like this.
> > > >    if (vp_read(&vp->common, virtio_mmio_cfg, version) == 2) {
> > > >        /* virtio 1.0 */
> > > >    } else {
> > > >        /* legacy */
> > > >    }
> > >
> > > I refer to the implementation of the Linux kernel and confirmed that QEMU's MMIO
> > > Device implementation. Legacy should also support this operation, although it
> > > may return 0. 0.
> > >
> > > Hi Michael, do I make mistakes?
> > >
> > > Thanks.
> >
> > Good point. Gerd: in virtio spec (e.g. in 1.0-cs03, 4.2.4 Legacy
> > interface) it is documented that legacy interface also has
> > HostFeaturesSel and GuestFeaturesSel registers, at the
> > same offsets as DeviceFeaturesSel and DriverFeaturesSel.
> > Accordingly, it should be safe to use these registers
> > with version == 1. No?
>
> Is the behavior for HostFeaturesSel/GuestFeaturesSel == 1 in legacy mode
> defined?  If both specs and code agree that reads return zero I'm fine
> with the patch as-is.

What the 0.9 spec said was:

eg.
 feature bits 0 to 31 if HostFeaturesSel is set to 0 and features bits 32
 to 63 if HostFeaturesSel is set to 1.

so yes it was defined. Of course implementations could be buggy, but
there you go.
> > Having said that - are there
> > actual feature bits >= 32 that we want to set in this seabios
> > version?
>
> No.

In that case I would split this patchset, with the features changes not
necessary for this qemu release.
> take care,
>   Gerd

[PATCH v1 2/2] virtio: finalize 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 code of finalizing features in front of using device
2. virtio-blk.mmio:   put the code of finalizing features in front of using device
3. virtio-scsi.pci:   is ok
4. virtio-scsi.mmio:  add the code of finalizing features before vp_find_vq()

Link: https://www.mail-archive.com/[e-mail]/msg920776.html
Signed-off-by: Xuan Zhuo 
---
 src/hw/virtio-blk.c  | 22 +++++++++++++---------
 src/hw/virtio-scsi.c | 13 +++++++++++++
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c
index 9b4a05a..ab6e79b 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,14 @@ 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 +275,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 +293,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..79bda0b 100644
--- a/src/hw/virtio-scsi.c
+++ b/src/hw/virtio-scsi.c
@@ -239,6 +239,19 @@ 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;
+    if (features & version1) {
+        u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
+
+        vp_set_features(vp, features & (version1 | iommu_platform));
+        vp_set_status(vp, VIRTIO_CONFIG_S_FEATURES_OK);
+        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;

[PATCH v1 2/2] virtio: finalize features before using device

[Gerd Hoffmann]

On Mon, Nov 14, 2022 at 11:58:18AM +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 code of finalizing features in front of using device
> virtio-blk.mmio:   put the code of finalizing features in front of using device
> virtio-scsi.pci:   is ok
> virtio-scsi.mmio:  add the code of finalizing features before vp_find_vq()
>
> Link: https://www.mail-archive.com/[e-mail]/msg920776.html
> Signed-off-by: Xuan Zhuo

Reviewed-by: Gerd Hoffmann

[Michael S. Tsirkin]

On Mon, Nov 14, 2022 at 11:58:16AM +0800, Xuan Zhuo wrote:
> https://www.mail-archive.com/[e-mail]/msg920776.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.
>
> In this PATCH, it was added to check the FEATURE VIRTIO_1 when enabling queue.
> This code should not be executed, but it is reported here. Because Seabios
> use the legacy sequence to initialize the modern device.

Acked-by: Michael S. Tsirkin

Gerd, is there a chance to have the fix included in this QEMU release?
It would be preferable since then we can keep the warning in qemu
(probably with some changes such as machine type and LOG_GUEST_ERROR).
> The driver MUST follow this sequence to initialize a device:
>     1. Reset the device.
>     2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
>     3. Set the DRIVER status bit: the guest OS knows how to drive the device.
> > Read device feature bits, and write the subset of feature bits understood by the OS and driver to the
>    device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration
>    fields to check that it can support the device before accepting it.
> > Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step.
> > Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not
>    support our subset of features and the device is unusable.
> 7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup,
>    reading and possibly writing the device’s virtio configuration space, and population of virtqueues.
> 8. Set the DRIVER_OK status bit. At this point the device is “live”.
>
> Test with:
> qemu-system-x86_64 \
>    -M microvm,x-option-roms=off,pit=off,pic=off,isa-serial=off,rtc=off \
>    -global virtio-mmio.force-legacy={false,true} \
>    -enable-kvm -cpu host -m 512m -smp 2 \
>    -nodefaults -no-user-config -nographic \
>    -drive id=test,file=./test.img,format=raw,if=none \
>    -device virtio-blk-device,drive=test \
>    -bios ../../seabios/out/bios.bin \
>    -chardev stdio,id=seabios -device isa-debugcon,iobase=0x402,chardev=seabios
>
> qemu-system-x86_64 \
>    -M microvm,x-option-roms=off,pit=off,pic=off,isa-serial=off,rtc=off \
>    -global virtio-mmio.force-legacy={false,true} \
>    -enable-kvm -cpu host -m 512m -smp 2 \
>    -nodefaults -no-user-config -nographic \
>    -drive id=test,file=./test.img,format=raw,if=none \
>    -device virtio-scsi-device \
>    -bios ../../seabios/out/bios.bin \
>    -chardev stdio,id=seabios -device isa-debugcon,iobase=0x402,chardev=seabios
>
> qemu-system-x86_64 \
>    -enable-kvm -cpu host -m 512m -smp 2 \
>    -nodefaults -no-user-config -nographic \
>    -drive id=test,file=./test.img,format=raw,if=none \
>    -device virtio-scsi,disable-legacy={off,on},disable-modern={on,off} \
>    -bios ../../seabios/out/bios.bin \
>    -chardev stdio,id=seabios -device isa-debugcon,iobase=0x402,chardev=seabios
>
> qemu-system-x86_64 \
>    -enable-kvm -cpu host -m 512m -smp 2 \
>    -nodefaults -no-user-config -nographic \
>    -drive id=test,file=./test.img,format=raw,if=none \
>    -device virtio-blk-pci,drive=test,disable-legacy={off,on},disable-modern={on,off} \
>    -bios ../../seabios/out/bios.bin \
>    -chardev stdio,id=seabios -device isa-debugcon,iobase=0x402,chardev=seabios
>
> Xuan Zhuo (2):
>   virtio-mmio: read/write the hi 32 features for mmio
>   virtio: finalize features before using device
>
> src/hw/virtio-blk.c  | 22 +++++++++++++---------
>  src/hw/virtio-pci.c  |  7 +++++--
>  src/hw/virtio-scsi.c | 13 +++++++++++++
>  3 files changed, 31 insertions(+), 11 deletions(-)

[Gerd Hoffmann]

On Mon, Nov 14, 2022 at 03:04:39AM -0500, Michael S. Tsirkin wrote:
> On Mon, Nov 14, 2022 at 11:58:16AM +0800, Xuan Zhuo wrote:
> > https://www.mail-archive.com/[e-mail]/msg920776.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.
> >
> > In this PATCH, it was added to check the FEATURE VIRTIO_1 when enabling queue.
> > This code should not be executed, but it is reported here. Because Seabios
> > use the legacy sequence to initialize the modern device.
>
> Acked-by: Michael S. Tsirkin
>
> Gerd, is there a chance to have the fix included in this QEMU release?
> It would be preferable since then we can keep the warning in qemu
> (probably with some changes such as machine type and LOG_GUEST_ERROR).

Phew, that is rather tight timing given we don't even have a release
schedule yet.  I can try, maybe with a 1.15.1 bugfix release, but I
don't feel like promising anything at this point.

take care,
  Gerd