[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 09/22] virtio-iommu: Record whether a probe request has been i
From: |
Eric Auger |
Subject: |
Re: [PULL 09/22] virtio-iommu: Record whether a probe request has been issued |
Date: |
Thu, 9 Nov 2023 17:56:54 +0100 |
User-agent: |
Mozilla Thunderbird |
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
>
- [PULL 02/22] memory: Introduce memory_region_iommu_set_iova_ranges, (continued)
- [PULL 02/22] memory: Introduce memory_region_iommu_set_iova_ranges, Cédric Le Goater, 2023/11/06
- [PULL 01/22] memory: Let ReservedRegion use Range, Cédric Le Goater, 2023/11/06
- [PULL 04/22] virtio-iommu: Rename reserved_regions into prop_resv_regions, Cédric Le Goater, 2023/11/06
- [PULL 05/22] range: Make range_compare() public, Cédric Le Goater, 2023/11/06
- [PULL 06/22] util/reserved-region: Add new ReservedRegion helpers, Cédric Le Goater, 2023/11/06
- [PULL 07/22] virtio-iommu: Introduce per IOMMUDevice reserved regions, Cédric Le Goater, 2023/11/06
- [PULL 03/22] vfio: Collect container iova range info, Cédric Le Goater, 2023/11/06
- [PULL 08/22] range: Introduce range_inverse_array(), Cédric Le Goater, 2023/11/06
- [PULL 09/22] virtio-iommu: Record whether a probe request has been issued, Cédric Le Goater, 2023/11/06
- [PULL 10/22] virtio-iommu: Implement set_iova_ranges() callback, Cédric Le Goater, 2023/11/06
- [PULL 12/22] test: Add some tests for range and resv-mem helpers, Cédric Le Goater, 2023/11/06
- [PULL 11/22] virtio-iommu: Consolidate host reserved regions and property set ones, Cédric Le Goater, 2023/11/06
- [PULL 13/22] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps, Cédric Le Goater, 2023/11/06
- [PULL 14/22] util/uuid: Add UUID_STR_LEN definition, Cédric Le Goater, 2023/11/06
- [PULL 15/22] vfio/pci: Fix buffer overrun when writing the VF token, Cédric Le Goater, 2023/11/06
- [PULL 16/22] util/uuid: Remove UUID_FMT_LEN, Cédric Le Goater, 2023/11/06
- [PULL 17/22] util/uuid: Define UUID_STR_LEN from UUID_NONE string, Cédric Le Goater, 2023/11/06
- [PULL 18/22] vfio/container: Move IBM EEH related functions into spapr_pci_vfio.c, Cédric Le Goater, 2023/11/06
- [PULL 19/22] vfio/container: Move vfio_container_add/del_section_window into spapr.c, Cédric Le Goater, 2023/11/06