run_qemu.sh: replace SATA -drive with -device virtio-blk

[ikitayama]

This is a retry of #201

With the proposed V13 of the CXL series for the QEMU upstream applied, qemu-system-aarch64
fails to boot with the legacy -drive option alone to set root.img:

qemu-system-aarch64: -drive file=root.img,format=raw,media=disk: PCI: Only PCI/PCIe bridges can be plugged into pxb-cxl

Jonathan explains this is due to on arm64 -drive is virtoi-blk by default and we need to set a bus via the -device option. Do it as Jonathan suggested in:

https://lore.kernel.org/all/[email protected]/

without losing backward compatibility.

[marc-hb]

I just tested this and it works and seems even a bit faster but...

If we drop the unstable -drive shortcut, then let's really drop it and replace it with -blockdev + -device as recommended by https://www.qemu.org/docs/master/system/qemu-manpage.html#hxtool-1

I mean let's not replace -drive by ( -blockdev + ... -drive!? )

I just tested this below and it works for me too:

qcmd+=("-blockdev" "driver=file,node-name=maindisk,filename=$_arg_rootfs")
qcmd+=("-device" "virtio-blk,bus=pcie.0,drive=maindisk")

https://www.qemu.org/docs/master/system/qemu-manpage.html#hxtool-1

The QEMU block device handling options have a long history and have gone through several iterations as the feature set and complexity of the block layer have grown. Many online guides to QEMU often reference older and deprecated options, which can lead to confusion.

The most explicit way to describe disks is to use a combination of -device to specify the hardware device and -blockdev to describe the backend. The device defines what the guest sees and the backend describes how QEMU handles the data. It is the only guaranteed stable interface for describing block devices and as such is recommended for management tools and scripting.

The -drive option combines the device and backend into a single command line option which is a more human friendly. There is however no interface stability guarantee although some older board models still need updating to work with the modern blockdev forms.

[marc-hb]

Please limit your commit subject to less than 80 lines which is the standard across projects including the linux kernel but not just

https://www.kernel.org/doc/html/v6.15-rc7/process/submitting-patches.html#the-canonical-patch-format
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#write-a-meaningful-commit-message
https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_Git_commit_message_structure

[run_qemu]$ ../kernel/scripts/checkpatch.pl --no-tree -g HEAD
WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)

[marc-hb]

Also, don't describe the problem in the subject, problems can rarely be described in just 1 line. So, the commit subject must focus on the change made, example: run_qemu.sh: replace SATA -drive with -device virtio-blk

The rest of the commit message has unlimited space to describe the problem, rationale, solution, etc.

[ikitayama]

All noted and I give my Tested-by: tag

Tested-by: Itaru Kitayama <[email protected]>

[marc-hb]

You cannot give a Tested-by: tag to yourself, it's implied :-)

I realize I suggested a lot of changes relative to the small size of this patch but this is still your PR and your work, can you please force-push a revised version? Add my Tested-by tag :-)

There is a "co-developed-by" tag if you think it is important. For a change that small I do not mind.
https://www.kernel.org/doc/html/v6.15-rc7/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Note to self: after this is merged, add cache.no-flush=on to -blockdev when [[ $_arg_rw == "off" ]. On my systems, it makes FIO writes 5 times faster (except when root.img is on tmpfs, then it makes no difference)

[jic23]

Nice work. I was coming here to suggest that change only to find @ikitayama already proposed it and it was already improved on :)

[ikitayama]

You cannot give a Tested-by: tag to yourself, it's implied :-)

I realize I suggested a lot of changes relative to the small size of this patch but this is still your PR and your work, can you please force-push a revised version? Add my Tested-by tag :-)

There is a "co-developed-by" tag if you think it is important. For a change that small I do not mind. https://www.kernel.org/doc/html/v6.15-rc7/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Note to self: after this is merged, add cache.no-flush=on to -blockdev when [[ $_arg_rw == "off" ]. On my systems, it makes FIO writes 5 times faster (except when root.img is on tmpfs, then it makes no difference)

@marc-hb The options you proposed look promising (I did not know it at all) and the rational sounds, while you're at it, would you go ahead and extend this PR or make a new PR as I don't have time today and tomorrow much?

[marc-hb]

I don't think anyone needs this besides you, so there is no rush to get it merged. We can wait until you have the time to get back to it.

[marc-hb]

@ikitayama any time to resume this?

(I can add the cache.no-flush=on later)

[ikitayama]

@marc-hb sorry about the long delay. Would you review your proposed change to drop the -drive option from the script?

[marc-hb]

Re-submitted in #211, @ikitayama , @jic23 please review and test. It's still only 2 lines long.

[marc-hb]

(I can add the cache.no-flush=on later)

I experimented with this and --name=testfio --size=10M --rw=randwrite --runtime=30 --time_based --eta-newline=2 --ioengine=sync and other variations and I could not spot any significant difference. Let's forget it for now.