Re: [PATCH] OvmfPkg/VirtioScsiDxe: Allocate all required vrings at VirtioScsiInit

[Laszlo Ersek <lersek@redhat.com>]

Hello Zheng Xiang,

I'm adding Maxime for DPDK / vhost-scsi expertise, and Paolo for general
virtio-scsi expertise:

On 12/13/17 04:16, Zheng Xiang wrote:
> VirtioScsiInit() allocate only one request queue which causes the
> address of the other vrings to be 0. In AARCH64 virtual machines,
> the address of system memory starts at 0x40000000 and the address
> of rom starts at 0. This causes an error when QEMU translates the
> guest physical memory of vhost-scsi vrings to host virtual memory.
> 
> So this patch fixes this issue by allocating all the required Vrings.

I would have preferred if you had contacted us first, before putting
quite a bit of work into this patch (judged from the diffstat below);
perhaps via the TianoCore Bugzilla at <https://bugzilla.tianocore.org>.

Because, the approach described in the commit message is wrong. (In
other words, the concrete patch might be implementing the idea
correctly, but the idea itself is incorrect.)

The symptom you are seeing is a problem in vhost-scsi / DPDK -- and,
actually, now I'm thinking it is a bug in the virtio spec even.
Recently, Maxime has fixed a very similar bug in vhost-net:

  https://bugzilla.redhat.com/show_bug.cgi?id=1518884
  http://dpdk.org/ml/archives/dev/2017-December/083501.html

  [dpdk-dev] [PATCH v4 0/4] Vhost: fix mq=on but VIRTIO_NET_F_MQ not
                            negotiated

The vhost-net issue was that vhost-net required the guest driver to set
up *all* of the possible queues, even if the guest driver did not
negotiate VIRTIO_NET_F_MQ.

The same thing applies to virtio-scsi. The guest driver should not be
*required* to set up all of the possible queues -- it makes no sense to
*force* a guest driver to use multi-queue. (And indeed, QEMU does not
present this (invalid) requirement.)

The difference with virtio-net is that the virtio specification [*] does
not currently define an explicit multiqueue feature bit for virtio-scsi
-- VIRTIO_NET_F_MQ is virtio-net only -- so the guest driver has no
means to actively *clear* that bit as part of the negotiation.

[*] http://docs.oasis-open.org/virtio/virtio/v1.0/virtio-v1.0.html
    Committee Specification 04; 03 March 2016

I consider the lack of a "VIRTIO_SCSI_F_MQ" feature bit an issue with
the virtio specification (and consequently with vhost-scsi), not with
the guest driver(s).

Firmware should not be forced to use advanced virtio features. For
example, SeaBIOS does not use multi-queue with virtio-scsi either:

Please refer to the *sole* vp_find_vq() call in init_virtio_scsi(), in
file "src/hw/virtio-scsi.c": only queue #2 is set up, i.e. the first
request queue. Queues #0 and #1 (controlq and eventq, respectively), and
all further request queues (>= #3) are ignored.


Perhaps you can update vhost-scsi similarly to the last patch of
Maxime's v4 series, even without "VIRTIO_SCSI_F_MQ" -- in the
SET_FEATURES request handler, just destroy the unused virtqueues that
have not been configured by the guest driver until that time?

Alternatively: if, for compatibility reasons, a new virtio-scsi feature
flag could only be introduced in the *negative* sense, such as
"VIRTIO_SCSI_F_NO_MQ", then we can add a (very small) patch to OVMF that
negotiates this bit. (I'm unsure why a negative sense flag would be
necessary; just mentioning it for completeness.)

Thanks,
Laszlo