Message ID | 20231106143653.302391-10-clg@redhat.com |
---|---|
State | New |
Headers | show |
Series | [PULL,01/22] memory: Let ReservedRegion use Range | expand |
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
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 --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; }