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

[Eric Auger <eric.auger@redhat.com>]

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
>