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

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

[Zheng Xiang]

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.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Shannon Zhao <zhaoshenglong@huawei.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zheng Xiang <zhengxiang9@huawei.com>
---
When using qemu with vhost-scsi on ARM64, it will fail with below error:
qemu-kvm: Error start vhost dev
qemu-kvm: unable to start vhost-scsi: Cannot allocate memory
---
 OvmfPkg/Include/IndustryStandard/VirtioScsi.h |   1 +
 OvmfPkg/VirtioScsiDxe/VirtioScsi.c            | 192 ++++++++++++++++----------
 OvmfPkg/VirtioScsiDxe/VirtioScsi.h            |   7 +-
 3 files changed, 122 insertions(+), 78 deletions(-)

diff --git a/OvmfPkg/Include/IndustryStandard/VirtioScsi.h b/OvmfPkg/Include/IndustryStandard/VirtioScsi.h
index 0c9b520..9f4e6be 100644
--- a/OvmfPkg/Include/IndustryStandard/VirtioScsi.h
+++ b/OvmfPkg/Include/IndustryStandard/VirtioScsi.h
@@ -81,6 +81,7 @@ typedef struct {
 // selector of first virtio queue usable for request transfer
 //
 #define VIRTIO_SCSI_REQUEST_QUEUE 2
+#define VIRTIO_SCSI_QUEUE_MAX     1024
 
 //
 // host response codes
diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index 1a68f06..b9e72c8 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -590,19 +590,19 @@ VirtioScsiPassThru (
     goto FreeResponseBuffer;
   }
 
-  VirtioPrepare (&Dev->Ring, &Indices);
+  VirtioPrepare (&Dev->Ring[VIRTIO_SCSI_REQUEST_QUEUE], &Indices);
 
   //
   // ensured by VirtioScsiInit() -- this predicate, in combination with the
   // lock-step progress, ensures we don't have to track free descriptors.
   //
-  ASSERT (Dev->Ring.QueueSize >= 4);
+  ASSERT (Dev->Ring[VIRTIO_SCSI_REQUEST_QUEUE].QueueSize >= 4);
 
   //
   // enqueue Request
   //
   VirtioAppendDesc (
-    &Dev->Ring,
+    &Dev->Ring[VIRTIO_SCSI_REQUEST_QUEUE],
     RequestDeviceAddress,
     sizeof Request,
     VRING_DESC_F_NEXT,
@@ -614,7 +614,7 @@ VirtioScsiPassThru (
   //
   if (Packet->OutTransferLength > 0) {
     VirtioAppendDesc (
-      &Dev->Ring,
+      &Dev->Ring[VIRTIO_SCSI_REQUEST_QUEUE],
       OutDataDeviceAddress,
       Packet->OutTransferLength,
       VRING_DESC_F_NEXT,
@@ -626,7 +626,7 @@ VirtioScsiPassThru (
   // enqueue Response, to be written by the host
   //
   VirtioAppendDesc (
-    &Dev->Ring,
+    &Dev->Ring[VIRTIO_SCSI_REQUEST_QUEUE],
     ResponseDeviceAddress,
     sizeof *Response,
     VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? VRING_DESC_F_NEXT : 0),
@@ -638,7 +638,7 @@ VirtioScsiPassThru (
   //
   if (Packet->InTransferLength > 0) {
     VirtioAppendDesc (
-      &Dev->Ring,
+      &Dev->Ring[VIRTIO_SCSI_REQUEST_QUEUE],
       InDataDeviceAddress,
       Packet->InTransferLength,
       VRING_DESC_F_WRITE,
@@ -650,7 +650,8 @@ VirtioScsiPassThru (
   // EFI_NOT_READY would save us the effort, but it would also suggest that the
   // caller retry.
   //
-  if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
+  if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE,
+        &Dev->Ring[VIRTIO_SCSI_REQUEST_QUEUE],
         &Indices, NULL) != EFI_SUCCESS) {
     Status = ReportHostAdapterError (Packet);
     goto UnmapResponseBuffer;
@@ -912,6 +913,88 @@ VirtioScsiGetNextTarget (
   return EFI_NOT_FOUND;
 }
 
+STATIC
+EFI_STATUS
+EFIAPI
+VirtioScsiInitRing (
+ IN OUT VSCSI_DEV  *Dev,
+ IN     UINT16     Selector,
+ OUT    VRING      *Ring,
+ OUT    VOID       **Mapping
+ )
+{
+  EFI_STATUS Status;
+  UINT16     QueueSize;
+  UINT64     RingBaseShift;
+  VOID       *MapInfo;
+
+  //
+  // step 4b -- allocate request virtqueue
+  //
+  Status = Dev->VirtIo->SetQueueSel (Dev->VirtIo, Selector);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  Status = Dev->VirtIo->GetQueueNumMax (Dev->VirtIo, &QueueSize);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  //
+  // VirtioScsiPassThru() uses at most four descriptors
+  //
+  if (QueueSize < 4) {
+    Status = EFI_UNSUPPORTED;
+    return Status;
+  }
+
+  Status = VirtioRingInit (Dev->VirtIo, QueueSize, Ring);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  //
+  // If anything fails from here on, we must release the ring resources
+  //
+  Status = VirtioRingMap (Dev->VirtIo, Ring, &RingBaseShift, &MapInfo);
+  if (EFI_ERROR (Status)) {
+    goto ReleaseQueue;
+  }
+
+  //
+  // Additional steps for MMIO: align the queue appropriately, and set the
+  // size. If anything fails from here on, we must unmap the ring resources.
+  //
+  Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
+  if (EFI_ERROR (Status)) {
+    goto UnmapQueue;
+  }
+
+  Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
+  if (EFI_ERROR (Status)) {
+    goto UnmapQueue;
+  }
+
+  //
+  // step 4c -- Report GPFN (guest-physical frame number) of queue.
+  //
+  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, Ring, RingBaseShift);
+  if (EFI_ERROR (Status)) {
+    goto UnmapQueue;
+  }
+
+  *Mapping = MapInfo;
+
+  return EFI_SUCCESS;
+
+UnmapQueue:
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, MapInfo);
+
+ReleaseQueue:
+  VirtioRingUninit (Dev->VirtIo, Ring);
+
+  return Status;
+}
+
 
 STATIC
 EFI_STATUS
@@ -922,11 +1005,10 @@ VirtioScsiInit (
 {
   UINT8      NextDevStat;
   EFI_STATUS Status;
-  UINT64     RingBaseShift;
   UINT64     Features;
   UINT16     MaxChannel; // for validation only
-  UINT32     NumQueues;  // for validation only
-  UINT16     QueueSize;
+  UINT16     Count;
+  UINT16     Index;
 
   //
   // Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence.
@@ -978,11 +1060,11 @@ VirtioScsiInit (
     goto Failed;
   }
 
-  Status = VIRTIO_CFG_READ (Dev, NumQueues, &NumQueues);
+  Status = VIRTIO_CFG_READ (Dev, NumQueues, &Dev->NumQueues);
   if (EFI_ERROR (Status)) {
     goto Failed;
   }
-  if (NumQueues < 1) {
+  if (Dev->NumQueues < 1 || Dev->NumQueues > VIRTIO_SCSI_QUEUE_MAX - 2) {
     Status = EFI_UNSUPPORTED;
     goto Failed;
   }
@@ -1031,66 +1113,18 @@ VirtioScsiInit (
   }
 
   //
-  // step 4b -- allocate request virtqueue
-  //
-  Status = Dev->VirtIo->SetQueueSel (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE);
-  if (EFI_ERROR (Status)) {
-    goto Failed;
-  }
-  Status = Dev->VirtIo->GetQueueNumMax (Dev->VirtIo, &QueueSize);
-  if (EFI_ERROR (Status)) {
-    goto Failed;
-  }
-  //
-  // VirtioScsiPassThru() uses at most four descriptors
-  //
-  if (QueueSize < 4) {
-    Status = EFI_UNSUPPORTED;
-    goto Failed;
-  }
-
-  Status = VirtioRingInit (Dev->VirtIo, QueueSize, &Dev->Ring);
-  if (EFI_ERROR (Status)) {
-    goto Failed;
-  }
-
-  //
-  // If anything fails from here on, we must release the ring resources
+  // step 4b, 4c -- allocate and report virtqueues
   //
-  Status = VirtioRingMap (
-             Dev->VirtIo,
-             &Dev->Ring,
-             &RingBaseShift,
-             &Dev->RingMap
+  for (Count = 0; Count < Dev->NumQueues + 2; Count++) {
+    Status = VirtioScsiInitRing (
+             Dev,
+             Count,
+             &Dev->Ring[Count],
+             &Dev->RingMap[Count]
              );
-  if (EFI_ERROR (Status)) {
-    goto ReleaseQueue;
-  }
-
-  //
-  // Additional steps for MMIO: align the queue appropriately, and set the
-  // size. If anything fails from here on, we must unmap the ring resources.
-  //
-  Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
-  if (EFI_ERROR (Status)) {
-    goto UnmapQueue;
-  }
-
-  Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
-  if (EFI_ERROR (Status)) {
-    goto UnmapQueue;
-  }
-
-  //
-  // step 4c -- Report GPFN (guest-physical frame number) of queue.
-  //
-  Status = Dev->VirtIo->SetQueueAddress (
-                          Dev->VirtIo,
-                          &Dev->Ring,
-                          RingBaseShift
-                          );
-  if (EFI_ERROR (Status)) {
-    goto UnmapQueue;
+    if (EFI_ERROR (Status)) {
+      goto UnmapQueue;
+    }
   }
 
   //
@@ -1160,10 +1194,10 @@ VirtioScsiInit (
   return EFI_SUCCESS;
 
 UnmapQueue:
-  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
-
-ReleaseQueue:
-  VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
+  for (Index = Count - 1; Index >= 0; Index--) {
+    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap[Index]);
+    VirtioRingUninit (Dev->VirtIo, &Dev->Ring[Index]);
+  }
 
 Failed:
   //
@@ -1177,6 +1211,7 @@ Failed:
   Dev->MaxTarget      = 0;
   Dev->MaxLun         = 0;
   Dev->MaxSectors     = 0;
+  Dev->NumQueues      = 0;
 
   return Status; // reached only via Failed above
 }
@@ -1189,6 +1224,7 @@ VirtioScsiUninit (
   IN OUT VSCSI_DEV *Dev
   )
 {
+  INT16    Index;
   //
   // Reset the virtual device -- see virtio-0.9.5, 2.2.2.1 Device Status. When
   // VIRTIO_CFG_WRITE() returns, the host will have learned to stay away from
@@ -1201,8 +1237,12 @@ VirtioScsiUninit (
   Dev->MaxLun         = 0;
   Dev->MaxSectors     = 0;
 
-  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
-  VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
+  for (Index = 0; Index < Dev->NumQueues + 2; Index++) {
+    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap[Index]);
+    VirtioRingUninit (Dev->VirtIo, &Dev->Ring[Index]);
+  }
+
+  Dev->NumQueues      = 0;
 
   SetMem (&Dev->PassThru,     sizeof Dev->PassThru,     0x00);
   SetMem (&Dev->PassThruMode, sizeof Dev->PassThruMode, 0x00);
diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
index 05a6bf5..40ee6ba 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
@@ -57,10 +57,13 @@ typedef struct {
   UINT16                          MaxTarget;      // VirtioScsiInit      1
   UINT32                          MaxLun;         // VirtioScsiInit      1
   UINT32                          MaxSectors;     // VirtioScsiInit      1
-  VRING                           Ring;           // VirtioRingInit      2
+  UINT32                          NumQueues;      // VirtioScsiInit      1
+  VRING                           Ring[VIRTIO_SCSI_QUEUE_MAX];
+                                                  // VirtioScsiInitRing  2
   EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;       // VirtioScsiInit      1
   EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;   // VirtioScsiInit      1
-  VOID                            *RingMap;       // VirtioRingMap       2
+  VOID                            *RingMap[VIRTIO_SCSI_QUEUE_MAX];
+                                                  // VirtioRingMap       3
 } VSCSI_DEV;
 
 #define VIRTIO_SCSI_FROM_PASS_THRU(PassThruPointer) \
-- 
1.8.3.1

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

[Laszlo Ersek]

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

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

[Paolo Bonzini]

On 13/12/2017 09:35, Laszlo Ersek wrote:
> 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).

VIRTIO_SCSI_F_MQ does not exist because virtio-scsi has _always_
supported multiqueue and has always had a "num_queues" field in the
configuration space.  For virtio-net, VIRTIO_NET_F_MQ does not say "the
device or driver knows about multiqueue", it says "the device or driver
wants to read max_virtqueue_pairs" from configuration space.  It's
perfectly fine for a device to propose VIRTIO_NET_F_MQ and set
max_virtqueue_pairs=1, or for a driver to negotiate VIRTIO_NET_F_MQ and
then skip initialization of some virtqueues.

This also means that Maxime's patch to DPDK is also not enough. :)
Virtio-net actually does have a configuration mechanism for multiqueue,
namely the VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command; the driver sends
that command specifying the number of the transmit and receive queues to
use.  However, in my understanding, that command is only needed for the
device to configure receive flow steering, so virtio-scsi doesn't need
that either.

> 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?

Yes, this is the right solution.  We can assume that if the descriptor
address is equal to zero, the queue is not in use.  This is not in the
spec as far as I can see, but it is QEMU's assumption.  I will send a
patch to the virtio specification.

Paolo

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

[Laszlo Ersek]

On 12/13/17 10:29, Paolo Bonzini wrote:
> On 13/12/2017 09:35, Laszlo Ersek wrote:
>> 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).
> 
> VIRTIO_SCSI_F_MQ does not exist because virtio-scsi has _always_
> supported multiqueue and has always had a "num_queues" field in the
> configuration space.  For virtio-net, VIRTIO_NET_F_MQ does not say "the
> device or driver knows about multiqueue", it says "the device or driver
> wants to read max_virtqueue_pairs" from configuration space.  It's
> perfectly fine for a device to propose VIRTIO_NET_F_MQ and set
> max_virtqueue_pairs=1, or for a driver to negotiate VIRTIO_NET_F_MQ and
> then skip initialization of some virtqueues.
> 
> This also means that Maxime's patch to DPDK is also not enough. :)
> Virtio-net actually does have a configuration mechanism for multiqueue,
> namely the VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command; the driver sends
> that command specifying the number of the transmit and receive queues to
> use.  However, in my understanding, that command is only needed for the
> device to configure receive flow steering, so virtio-scsi doesn't need
> that either.
> 
>> 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?
> 
> Yes, this is the right solution.  We can assume that if the descriptor
> address is equal to zero, the queue is not in use.  This is not in the
> spec as far as I can see, but it is QEMU's assumption.  I will send a
> patch to the virtio specification.

Thank you Paolo!
Laszlo

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

[Laszlo Ersek]

On 12/13/17 10:29, Paolo Bonzini wrote:
> On 13/12/2017 09:35, Laszlo Ersek wrote:
>> 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).
>
> VIRTIO_SCSI_F_MQ does not exist because virtio-scsi has _always_
> supported multiqueue and has always had a "num_queues" field in the
> configuration space.  For virtio-net, VIRTIO_NET_F_MQ does not say
> "the device or driver knows about multiqueue", it says "the device or
> driver wants to read max_virtqueue_pairs" from configuration space.
> It's perfectly fine for a device to propose VIRTIO_NET_F_MQ and set
> max_virtqueue_pairs=1, or for a driver to negotiate VIRTIO_NET_F_MQ
> and then skip initialization of some virtqueues.
>
> This also means that Maxime's patch to DPDK is also not enough. :)
> Virtio-net actually does have a configuration mechanism for
> multiqueue, namely the VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command; the
> driver sends that command specifying the number of the transmit and
> receive queues to use.  However, in my understanding, that command is
> only needed for the device to configure receive flow steering, so
> virtio-scsi doesn't need that either.
>
>> 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?
>
> Yes, this is the right solution.  We can assume that if the descriptor
> address is equal to zero, the queue is not in use.  This is not in the
> spec as far as I can see, but it is QEMU's assumption.  I will send a
> patch to the virtio specification.

Hmmm, I'm not so sure about this, on a second thought. Reviewing the
OVMF code, I see that I added a comment (to all of the virtio drivers
actually):

>   //
>   // In virtio-1.0, feature negotiation is expected to complete before queue
>   // discovery, and the device can also reject the selected set of features.
>   //

I added this because of the following sections in the 1.0 spec:
- 3.1.1 Driver Requirements: Device Initialization
- 3.1.2 Legacy Interface: Device Initialization

In particular 3.1.2 writes, "The result was [...] steps 4, 7 and 8 were
conflated.".

(When I added virtio-1.0 support to OVMF, I paid attention to conform to
the new ordering for modern transports, and to keep the ordering
unchanged otherwise.)

I think this is a problem then; if a 1.0 driver is required to finish
feature negotiation (steps 4-6) before configuring the queues (step 7),
then the host side cannot derive any clues from the state of the queues
when the guest completes step 5 (= set FEATURES_OK).

Am I wrong?

... On the other hand, when the driver sets DRIVER_OK (step 8), then the
host *can* derive clues from the state of the queues; I think.

Thanks
Laszlo

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

[zhengxiang (A)]

Hello Laszlo and Paolo,

Thanks for your review!

On 2017/12/13 19:16, Laszlo Ersek wrote:
> On 12/13/17 10:29, Paolo Bonzini wrote:
>> On 13/12/2017 09:35, Laszlo Ersek wrote:
>>> 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).
>> VIRTIO_SCSI_F_MQ does not exist because virtio-scsi has _always_
>> supported multiqueue and has always had a "num_queues" field in the
>> configuration space.  For virtio-net, VIRTIO_NET_F_MQ does not say
>> "the device or driver knows about multiqueue", it says "the device or
>> driver wants to read max_virtqueue_pairs" from configuration space.
>> It's perfectly fine for a device to propose VIRTIO_NET_F_MQ and set
>> max_virtqueue_pairs=1, or for a driver to negotiate VIRTIO_NET_F_MQ
>> and then skip initialization of some virtqueues.
>>
>> This also means that Maxime's patch to DPDK is also not enough. :)
>> Virtio-net actually does have a configuration mechanism for
>> multiqueue, namely the VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command; the
>> driver sends that command specifying the number of the transmit and
>> receive queues to use.  However, in my understanding, that command is
>> only needed for the device to configure receive flow steering, so
>> virtio-scsi doesn't need that either.
>>
>>> 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?
>> Yes, this is the right solution.  We can assume that if the descriptor
>> address is equal to zero, the queue is not in use.  This is not in the
>> spec as far as I can see, but it is QEMU's assumption.  I will send a
>> patch to the virtio specification.

I would try this solution! However, is there any possibility that the allocated
descriptor address is exactly equal to zero and the queue is in use?

Moreover, is it feasible to skip the vhost_virtqueue_start() call for the unused
queues in vhost_dev_start() in QEMU?



Thanks,
Xiang

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

[Paolo Bonzini]

On 14/12/2017 07:55, zhengxiang (A) wrote:
> Hello Laszlo and Paolo,
> 
> Thanks for your review!
> 
> On 2017/12/13 19:16, Laszlo Ersek wrote:
>> On 12/13/17 10:29, Paolo Bonzini wrote:
>>> On 13/12/2017 09:35, Laszlo Ersek wrote:
>>>> 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?
>>> Yes, this is the right solution.  We can assume that if the descriptor
>>> address is equal to zero, the queue is not in use.  This is not in the
>>> spec as far as I can see, but it is QEMU's assumption.  I will send a
>>> patch to the virtio specification.
> 
> I would try this solution! However, is there any possibility that the allocated
> descriptor address is exactly equal to zero and the queue is in use?

That would break QEMU's virtio implementation, so it's pretty unlikely.

Paolo

> Moreover, is it feasible to skip the vhost_virtqueue_start() call for the unused
> queues in vhost_dev_start() in QEMU?
> 
> 
> 
> Thanks,
> Xiang
> 
> 

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

[zhengxiang (A)]

On 2017/12/14 17:06, Paolo Bonzini wrote:
> On 14/12/2017 07:55, zhengxiang (A) wrote:
>> Hello Laszlo and Paolo,
>>
>> Thanks for your review!
>>
>> On 2017/12/13 19:16, Laszlo Ersek wrote:
>>> On 12/13/17 10:29, Paolo Bonzini wrote:
>>>> On 13/12/2017 09:35, Laszlo Ersek wrote:
>>>>> 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?
>>>> Yes, this is the right solution.  We can assume that if the descriptor
>>>> address is equal to zero, the queue is not in use.  This is not in the
>>>> spec as far as I can see, but it is QEMU's assumption.  I will send a
>>>> patch to the virtio specification.
>>
>> I would try this solution! However, is there any possibility that the allocated
>> descriptor address is exactly equal to zero and the queue is in use?
> 
> That would break QEMU's virtio implementation, so it's pretty unlikely.
> 
> Paolo
> 

So could I judge the not-in-use queues by adding the below sentence in order
to skip calling vhost_virtqueue_start?

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e4290ce..05c3322 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1532,6 +1532,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
         goto fail_mem;
     }
     for (i = 0; i < hdev->nvqs; ++i) {
+        if (virtio_queue_get_desc_addr(vdev, i) == 0) continue;
         r = vhost_virtqueue_start(hdev,
                                   vdev,
                                   hdev->vqs + i,


Thanks,
Xiang

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

[Maxime Coquelin]

Hi Xiang,

On 12/14/2017 02:25 PM, zhengxiang (A) wrote:
> 
> 
> On 2017/12/14 17:06, Paolo Bonzini wrote:
>> On 14/12/2017 07:55, zhengxiang (A) wrote:
>>> Hello Laszlo and Paolo,
>>>
>>> Thanks for your review!
>>>
>>> On 2017/12/13 19:16, Laszlo Ersek wrote:
>>>> On 12/13/17 10:29, Paolo Bonzini wrote:
>>>>> On 13/12/2017 09:35, Laszlo Ersek wrote:
>>>>>> 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?
>>>>> Yes, this is the right solution.  We can assume that if the descriptor
>>>>> address is equal to zero, the queue is not in use.  This is not in the
>>>>> spec as far as I can see, but it is QEMU's assumption.  I will send a
>>>>> patch to the virtio specification.
>>>
>>> I would try this solution! However, is there any possibility that the allocated
>>> descriptor address is exactly equal to zero and the queue is in use?
>>
>> That would break QEMU's virtio implementation, so it's pretty unlikely.
>>
>> Paolo
>>
> 
> So could I judge the not-in-use queues by adding the below sentence in order
> to skip calling vhost_virtqueue_start?
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index e4290ce..05c3322 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1532,6 +1532,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>           goto fail_mem;
>       }
>       for (i = 0; i < hdev->nvqs; ++i) {
> +        if (virtio_queue_get_desc_addr(vdev, i) == 0) continue;
>           r = vhost_virtqueue_start(hdev,
>                                     vdev,
>                                     hdev->vqs + i,
> 

I think it should work, or you could detect it by checking that desc,
used and avail rings have the same address.

We would need this also for virtio-net, as Windows guest only setup as
much queue pairs as vcpus, but vhost_virtqueue_start is called for all
queue pairs declred in QEMU. With DPDK Vhost-user backend, it turns out
that it uses these uninitialized queues, corrupting guest's physical
address 0.

Do you plan to post the fix, or you'd like me to propose it?

Thanks,
Maxime
> Thanks,
> Xiang
> 

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

[Maxime Coquelin]

On 01/11/2018 02:23 PM, Maxime Coquelin wrote:
> Hi Xiang,
> 
> On 12/14/2017 02:25 PM, zhengxiang (A) wrote:
>>
>>
>> On 2017/12/14 17:06, Paolo Bonzini wrote:
>>> On 14/12/2017 07:55, zhengxiang (A) wrote:
>>>> Hello Laszlo and Paolo,
>>>>
>>>> Thanks for your review!
>>>>
>>>> On 2017/12/13 19:16, Laszlo Ersek wrote:
>>>>> On 12/13/17 10:29, Paolo Bonzini wrote:
>>>>>> On 13/12/2017 09:35, Laszlo Ersek wrote:
>>>>>>> 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?
>>>>>> Yes, this is the right solution.  We can assume that if the 
>>>>>> descriptor
>>>>>> address is equal to zero, the queue is not in use.  This is not in 
>>>>>> the
>>>>>> spec as far as I can see, but it is QEMU's assumption.  I will send a
>>>>>> patch to the virtio specification.
>>>>
>>>> I would try this solution! However, is there any possibility that 
>>>> the allocated
>>>> descriptor address is exactly equal to zero and the queue is in use?
>>>
>>> That would break QEMU's virtio implementation, so it's pretty unlikely.
>>>
>>> Paolo
>>>
>>
>> So could I judge the not-in-use queues by adding the below sentence in 
>> order
>> to skip calling vhost_virtqueue_start?
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index e4290ce..05c3322 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1532,6 +1532,7 @@ int vhost_dev_start(struct vhost_dev *hdev, 
>> VirtIODevice *vdev)
>>           goto fail_mem;
>>       }
>>       for (i = 0; i < hdev->nvqs; ++i) {
>> +        if (virtio_queue_get_desc_addr(vdev, i) == 0) continue;

BTW, the queue index is wrong here, it should be:

if (virtio_queue_get_desc_addr(vdev, hdev->vq_index + i) == 0)
     continue;

>>           r = vhost_virtqueue_start(hdev,
>>                                     vdev,
>>                                     hdev->vqs + i,
>>
> 

Maxime

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

[zhengxiang (A)]

Hi Maxime,

On 2018/1/11 22:46, Maxime Coquelin wrote:
> 
> 
> On 01/11/2018 02:23 PM, Maxime Coquelin wrote:
>> Hi Xiang,
>>
>> On 12/14/2017 02:25 PM, zhengxiang (A) wrote:
>>>
>>>
>>> On 2017/12/14 17:06, Paolo Bonzini wrote:
>>>> On 14/12/2017 07:55, zhengxiang (A) wrote:
>>>>> Hello Laszlo and Paolo,
>>>>>
>>>>> Thanks for your review!
>>>>>
>>>>> On 2017/12/13 19:16, Laszlo Ersek wrote:
>>>>>> On 12/13/17 10:29, Paolo Bonzini wrote:
>>>>>>> On 13/12/2017 09:35, Laszlo Ersek wrote:
>>>>>>>> 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?
>>>>>>> Yes, this is the right solution.  We can assume that if the descriptor
>>>>>>> address is equal to zero, the queue is not in use.  This is not in the
>>>>>>> spec as far as I can see, but it is QEMU's assumption.  I will send a
>>>>>>> patch to the virtio specification.
>>>>>
>>>>> I would try this solution! However, is there any possibility that the allocated
>>>>> descriptor address is exactly equal to zero and the queue is in use?
>>>>
>>>> That would break QEMU's virtio implementation, so it's pretty unlikely.
>>>>
>>>> Paolo
>>>>
>>>
>>> So could I judge the not-in-use queues by adding the below sentence in order
>>> to skip calling vhost_virtqueue_start?
>>>
>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>> index e4290ce..05c3322 100644
>>> --- a/hw/virtio/vhost.c
>>> +++ b/hw/virtio/vhost.c
>>> @@ -1532,6 +1532,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>>>           goto fail_mem;
>>>       }
>>>       for (i = 0; i < hdev->nvqs; ++i) {
>>> +        if (virtio_queue_get_desc_addr(vdev, i) == 0) continue;
> 
> BTW, the queue index is wrong here, it should be:
> 
> if (virtio_queue_get_desc_addr(vdev, hdev->vq_index + i) == 0)
>     continue;
> 
Thank you for correcting my mistake. I will post this patch for more discussions.
>>>           r = vhost_virtqueue_start(hdev,
>>>                                     vdev,
>>>                                     hdev->vqs + i,
>>>
>>
> 
> Maxime
> 
> .
> 

Thanks,
Xiang