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

Xuan Zhuo posted 2 patches 1 year, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/20221114035818.109511-1-xuanzhuo@linux.alibaba.com
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(-)
[SeaBIOS] [PATCH v1 0/2] virtio: finalize features before using modern device
Posted by Xuan Zhuo 1 year, 5 months ago
https://www.mail-archive.com/qemu-devel@nongnu.org/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.
>   4. 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.
>   5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step.
>   6. 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(-)

--
2.32.0.3.g01195cf9f

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH v1 0/2] virtio: finalize features before using modern device
Posted by Michael S. Tsirkin 1 year, 5 months ago
On Mon, Nov 14, 2022 at 11:58:16AM +0800, Xuan Zhuo wrote:
> https://www.mail-archive.com/qemu-devel@nongnu.org/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 <mst@redhat.com>


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.
> >   4. 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.
> >   5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step.
> >   6. 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(-)
> 
> --
> 2.32.0.3.g01195cf9f

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH v1 0/2] virtio: finalize features before using modern device
Posted by Gerd Hoffmann 1 year, 5 months ago
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/qemu-devel@nongnu.org/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 <mst@redhat.com>
> 
> 
> 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

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