Re: [Qemu-devel] [RFC v4 10/16] virtio-iommu: Implement probe request

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

Hi Tomasz,
On 27/09/2017 12:53, Tomasz Nowicki wrote:
> Hi Eric,
> 
> On 19.09.2017 09:46, Eric Auger wrote:
>> This patch implements the PROBE request. At the moment,
>> no reserved regions are returned.
>>
>> At the moment reserved regions are stored per device.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
> 
> [...]
> 
>> +
>> +static int virtio_iommu_fill_property(int devid, int type,
>> +                                      viommu_property_buffer *bufstate)
>> +{
>> +    int ret = -ENOSPC;
>> +
>> +    if (bufstate->filled + 4 >= VIOMMU_PROBE_SIZE) {
>> +        bufstate->error = true;
>> +        goto out;
>> +    }
>> +
>> +    switch (type) {
>> +    case VIRTIO_IOMMU_PROBE_T_NONE:
>> +        ret = virtio_iommu_fill_none_prop(bufstate);
>> +        break;
>> +    case VIRTIO_IOMMU_PROBE_T_RESV_MEM:
>> +    {
>> +        viommu_dev *dev = bufstate->dev;
>> +
>> +        g_tree_foreach(dev->reserved_regions,
>> +                       virtio_iommu_fill_resv_mem_prop,
>> +                       bufstate);
>> +        if (!bufstate->error) {
>> +            ret = 0;
>> +        }
>> +        break;
>> +    }
>> +    default:
>> +        ret = -ENOENT;
>> +        break;
>> +    }
>> +out:
>> +    if (ret) {
>> +        error_report("%s property of type=%d could not be filled (%d),"
>> +                     " remaining size = 0x%lx",
>> +                     __func__, type, ret, bufstate->filled);
>> +    }
>> +    return ret;
>> +}
>> +
>> +static int virtio_iommu_probe(VirtIOIOMMU *s,
>> +                              struct virtio_iommu_req_probe *req,
>> +                              uint8_t *buf)
>> +{
>> +    uint32_t devid = le32_to_cpu(req->device);
>> +    int16_t prop_types = SUPPORTED_PROBE_PROPERTIES, type;
>> +    viommu_property_buffer bufstate;
>> +    viommu_dev *dev;
>> +    int ret;
>> +
>> +    dev = g_tree_lookup(s->devices, GUINT_TO_POINTER(devid));
>> +    if (!dev) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    bufstate.start = buf;
>> +    bufstate.filled = 0;
>> +    bufstate.dev = dev;
> 
> bufstate.error is not initialized which may cause false alarm in
> virtio_iommu_fill_property()
thanks for spotting that. I owe you several fixes in both vsmmuv3 and
virtio-iommu. Thank you for testing, again!

Best Regards

Eric
> 
>> +
>> +    while ((type = ctz32(prop_types)) != 32) {
>> +        ret = virtio_iommu_fill_property(devid, 1 << type, &bufstate);
>> +        if (ret) {
>> +            break;
>> +        }
>> +        prop_types &= ~(1 << type);
>> +    }
>> +    virtio_iommu_fill_property(devid, VIRTIO_IOMMU_PROBE_T_NONE,
>> &bufstate);
>> +
>> +    return VIRTIO_IOMMU_S_OK;
>> +}
>> +
>>   #define get_payload_size(req) (\
>>   sizeof((req)) - sizeof(struct virtio_iommu_req_tail))
>>   @@ -433,6 +567,24 @@ static int
>> virtio_iommu_handle_unmap(VirtIOIOMMU *s,
>>       return virtio_iommu_unmap(s, &req);
>>   }
> 
> Thanks,
> Tomasz
>