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().
- virtio-blk.pci: put the finalizing features code in front of using device
- virtio-blk.mmio: put the finalizing features code in front of using device
- virtio-scsi.pci: is ok
- virtio-scsi.mmio: add set features before vp_find_vq()
Signed-off-by: Xuan Zhuo 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