diff mbox series

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

Message ID 20231106143653.302391-10-clg@redhat.com (mailing list archive)
State New, archived
Headers show
Series [PULL,01/22] memory: Let ReservedRegion use Range | expand

Commit Message

Cédric Le Goater Nov. 6, 2023, 2:36 p.m. UTC 
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(-)

Comments

Peter Maydell Nov. 9, 2023, 3:08 p.m. UTC | #1 
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
Eric Auger Nov. 9, 2023, 4:56 p.m. UTC | #2 
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
>
diff mbox series

Patch

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;
 }