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

[Peter Maydell]

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