1. Patchew
  2. QEMU
  3. [PULL 00/22] vfio queue
  4. View patch

[PULL 09/22] virtio-iommu: Record whether a probe request has been issued

Cédric Le Goater posted 22 patches 1 year ago
Maintainers: Stefan Hajnoczi <stefanha@redhat.com>, "Denis V. Lunev" <den@openvz.org>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Stefan Weil <sw@weilnetz.de>, Richard Henderson <richard.henderson@linaro.org>, Eric Auger <eric.auger@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Peter Xu <peterx@redhat.com>, Jason Wang <jasowang@redhat.com>, Helge Deller <deller@gmx.de>, Andrey Smirnov <andrew.smirnov@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, Nicholas Piggin <npiggin@gmail.com>, "Frédéric Barrat" <fbarrat@linux.ibm.com>, "Hervé Poussineau" <hpoussin@reactos.org>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, BALATON Zoltan <balaton@eik.bme.hu>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Elena Ufimtseva <elena.ufimtseva@oracle.com>, Jagannathan Raman <jag.raman@oracle.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Tony Krowiak <akrowiak@linux.ibm.com>, Jason Herne <jjherne@linux.ibm.com>, Alex Williamson <alex.williamson@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Fam Zheng <fam@euphon.net>, Juan Quintela <quintela@redhat.com>, Fabiano Rosas <farosas@suse.de>, Leonardo Bras <leobras@redhat.com>
There is a newer version of this series
[PULL 09/22] virtio-iommu: Record whether a probe request has been issued
Posted by Cédric Le Goater 1 year ago 
From: Eric Auger <eric.auger@redhat.com>

Add an IOMMUDevice 'probe_done' flag to record that the driver
already issued a probe request on that device.

This will be useful to double check host reserved regions aren't
notified after the probe and hence are not taken into account
by the driver.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Suggested-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: "Michael S. Tsirkin" <mst@redhat.com>
Tested-by: Yanghang Liu <yanghliu@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/hw/virtio/virtio-iommu.h |  1 +
 hw/virtio/virtio-iommu.c         | 20 +++++++++++---------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index 70b8ace34dfb7f24ff6cf41a21ecd283ca9ee512..1dd11ae81aeac25410f6f0a9bff89414b8edd48c 100644
--- a/include/hw/virtio/virtio-iommu.h
+++ b/include/hw/virtio/virtio-iommu.h
@@ -40,6 +40,7 @@ typedef struct IOMMUDevice {
     MemoryRegion root;          /* The root container of the device */
     MemoryRegion bypass_mr;     /* The alias of shared memory MR */
     GList *resv_regions;
+    bool probe_done;
 } IOMMUDevice;
 
 typedef struct IOMMUPciBus {
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 0e2370663d348e60678343dabd1f943792051315..13c3c087fe2ed7a4163ade5b40e11e6c8ea90b6e 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -639,19 +639,13 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
     return ret;
 }
 
-static ssize_t virtio_iommu_fill_resv_mem_prop(VirtIOIOMMU *s, uint32_t ep,
+static ssize_t virtio_iommu_fill_resv_mem_prop(IOMMUDevice *sdev, uint32_t ep,
                                                uint8_t *buf, size_t free)
 {
     struct virtio_iommu_probe_resv_mem prop = {};
     size_t size = sizeof(prop), length = size - sizeof(prop.head), total;
-    IOMMUDevice *sdev;
     GList *l;
 
-    sdev = container_of(virtio_iommu_mr(s, ep), IOMMUDevice, iommu_mr);
-    if (!sdev) {
-        return -EINVAL;
-    }
-
     total = size * g_list_length(sdev->resv_regions);
     if (total > free) {
         return -ENOSPC;
@@ -688,19 +682,27 @@ static int virtio_iommu_probe(VirtIOIOMMU *s,
                               uint8_t *buf)
 {
     uint32_t ep_id = le32_to_cpu(req->endpoint);
+    IOMMUMemoryRegion *iommu_mr = virtio_iommu_mr(s, ep_id);
     size_t free = VIOMMU_PROBE_SIZE;
+    IOMMUDevice *sdev;
     ssize_t count;
 
-    if (!virtio_iommu_mr(s, ep_id)) {
+    if (!iommu_mr) {
         return VIRTIO_IOMMU_S_NOENT;
     }
 
-    count = virtio_iommu_fill_resv_mem_prop(s, ep_id, buf, free);
+    sdev = container_of(iommu_mr, IOMMUDevice, iommu_mr);
+    if (!sdev) {
+        return -EINVAL;
+    }
+
+    count = virtio_iommu_fill_resv_mem_prop(sdev, ep_id, buf, free);
     if (count < 0) {
         return VIRTIO_IOMMU_S_INVAL;
     }
     buf += count;
     free -= count;
+    sdev->probe_done = true;
 
     return VIRTIO_IOMMU_S_OK;
 }
-- 
2.41.0
Re: [PULL 09/22] virtio-iommu: Record whether a probe request has been issued
Posted by Peter Maydell 1 year ago 
On Mon, 6 Nov 2023 at 14:48, Cédric Le Goater <clg@redhat.com> wrote:
>
> From: Eric Auger <eric.auger@redhat.com>
>
> Add an IOMMUDevice 'probe_done' flag to record that the driver
> already issued a probe request on that device.
>
> This will be useful to double check host reserved regions aren't
> notified after the probe and hence are not taken into account
> by the driver.

Hi; Coverity points out (CID 1523901) that this change introduced
dead code (but improves on the previous bad code!):


> -static ssize_t virtio_iommu_fill_resv_mem_prop(VirtIOIOMMU *s, uint32_t ep,
> +static ssize_t virtio_iommu_fill_resv_mem_prop(IOMMUDevice *sdev, uint32_t ep,
>                                                 uint8_t *buf, size_t free)
>  {
>      struct virtio_iommu_probe_resv_mem prop = {};
>      size_t size = sizeof(prop), length = size - sizeof(prop.head), total;
> -    IOMMUDevice *sdev;
>      GList *l;
>
> -    sdev = container_of(virtio_iommu_mr(s, ep), IOMMUDevice, iommu_mr);
> -    if (!sdev) {
> -        return -EINVAL;
> -    }

In the old code this check on sdev was wrong -- because iommu_mr
is not the first field in IOMMUDevice, if virtio_iommu_mr() returns
NULL that doesn't mean that container_of(...) is going to be NULL.

> -
>      total = size * g_list_length(sdev->resv_regions);
>      if (total > free) {
>          return -ENOSPC;
> @@ -688,19 +682,27 @@ static int virtio_iommu_probe(VirtIOIOMMU *s,
>                                uint8_t *buf)
>  {
>      uint32_t ep_id = le32_to_cpu(req->endpoint);
> +    IOMMUMemoryRegion *iommu_mr = virtio_iommu_mr(s, ep_id);
>      size_t free = VIOMMU_PROBE_SIZE;
> +    IOMMUDevice *sdev;
>      ssize_t count;
>
> -    if (!virtio_iommu_mr(s, ep_id)) {
> +    if (!iommu_mr) {
>          return VIRTIO_IOMMU_S_NOENT;
>      }
>
> -    count = virtio_iommu_fill_resv_mem_prop(s, ep_id, buf, free);
> +    sdev = container_of(iommu_mr, IOMMUDevice, iommu_mr);
> +    if (!sdev) {
> +        return -EINVAL;
> +    }

In the new code we already check directly whether virtio_iommu_mr()
returned NULL. So the check on sdev being NULL is simply dead
code -- it can never be true and we should just delete it.

> +
> +    count = virtio_iommu_fill_resv_mem_prop(sdev, ep_id, buf, free);
>      if (count < 0) {
>          return VIRTIO_IOMMU_S_INVAL;
>      }
>      buf += count;
>      free -= count;
> +    sdev->probe_done = true;
>
>      return VIRTIO_IOMMU_S_OK;
>  }

thanks
-- PMM
Re: [PULL 09/22] virtio-iommu: Record whether a probe request has been issued
Posted by Eric Auger 1 year ago 
Hi Peter,

On 11/9/23 16:08, Peter Maydell wrote:
> On Mon, 6 Nov 2023 at 14:48, Cédric Le Goater <clg@redhat.com> wrote:
>> From: Eric Auger <eric.auger@redhat.com>
>>
>> Add an IOMMUDevice 'probe_done' flag to record that the driver
>> already issued a probe request on that device.
>>
>> This will be useful to double check host reserved regions aren't
>> notified after the probe and hence are not taken into account
>> by the driver.
> Hi; Coverity points out (CID 1523901) that this change introduced
> dead code (but improves on the previous bad code!):
>
>
>> -static ssize_t virtio_iommu_fill_resv_mem_prop(VirtIOIOMMU *s, uint32_t ep,
>> +static ssize_t virtio_iommu_fill_resv_mem_prop(IOMMUDevice *sdev, uint32_t ep,
>>                                                 uint8_t *buf, size_t free)
>>  {
>>      struct virtio_iommu_probe_resv_mem prop = {};
>>      size_t size = sizeof(prop), length = size - sizeof(prop.head), total;
>> -    IOMMUDevice *sdev;
>>      GList *l;
>>
>> -    sdev = container_of(virtio_iommu_mr(s, ep), IOMMUDevice, iommu_mr);
>> -    if (!sdev) {
>> -        return -EINVAL;
>> -    }
> In the old code this check on sdev was wrong -- because iommu_mr
> is not the first field in IOMMUDevice, if virtio_iommu_mr() returns
> NULL that doesn't mean that container_of(...) is going to be NULL.
indeed thank you for pointing this out
>
>> -
>>      total = size * g_list_length(sdev->resv_regions);
>>      if (total > free) {
>>          return -ENOSPC;
>> @@ -688,19 +682,27 @@ static int virtio_iommu_probe(VirtIOIOMMU *s,
>>                                uint8_t *buf)
>>  {
>>      uint32_t ep_id = le32_to_cpu(req->endpoint);
>> +    IOMMUMemoryRegion *iommu_mr = virtio_iommu_mr(s, ep_id);
>>      size_t free = VIOMMU_PROBE_SIZE;
>> +    IOMMUDevice *sdev;
>>      ssize_t count;
>>
>> -    if (!virtio_iommu_mr(s, ep_id)) {
>> +    if (!iommu_mr) {
>>          return VIRTIO_IOMMU_S_NOENT;
>>      }
>>
>> -    count = virtio_iommu_fill_resv_mem_prop(s, ep_id, buf, free);
>> +    sdev = container_of(iommu_mr, IOMMUDevice, iommu_mr);
>> +    if (!sdev) {
>> +        return -EINVAL;
>> +    }
> In the new code we already check directly whether virtio_iommu_mr()
> returned NULL. So the check on sdev being NULL is simply dead
> code -- it can never be true and we should just delete it.

OK. Sending a fix ...

Thanks!

Eric


>
>> +
>> +    count = virtio_iommu_fill_resv_mem_prop(sdev, ep_id, buf, free);
>>      if (count < 0) {
>>          return VIRTIO_IOMMU_S_INVAL;
>>      }
>>      buf += count;
>>      free -= count;
>> +    sdev->probe_done = true;
>>
>>      return VIRTIO_IOMMU_S_OK;
>>  }
> thanks
> -- PMM
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.