From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 75FB5220EE113 for ; Wed, 13 Dec 2017 00:30:39 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E629525C51; Wed, 13 Dec 2017 08:35:18 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-40.rdu2.redhat.com [10.10.120.40]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4DBB7614E6; Wed, 13 Dec 2017 08:35:09 +0000 (UTC) To: Zheng Xiang Cc: edk2-devel@lists.01.org, Ard Biesheuvel , Jordan Justen , Shannon Zhao , Maxime Coquelin , Paolo Bonzini References: <20171213031632.11856-1-zhengxiang9@huawei.com> From: Laszlo Ersek Message-ID: <06533b91-8ff0-1ad2-56fa-5f2c4f56bb19@redhat.com> Date: Wed, 13 Dec 2017 09:35:06 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20171213031632.11856-1-zhengxiang9@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 13 Dec 2017 08:35:19 +0000 (UTC) Subject: Re: [PATCH] OvmfPkg/VirtioScsiDxe: Allocate all required vrings at VirtioScsiInit X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Dec 2017 08:30:40 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 . 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