Re: [Qemu-arm] [PATCH for-4.2 v10 10/15] virtio-iommu: Implement probe request

[Auger Eric]

Hi Peter,

On 8/19/19 2:08 PM, Peter Xu wrote:
> On Tue, Jul 30, 2019 at 07:21:32PM +0200, Eric Auger wrote:
> 
> [...]
> 
>> +/* Fill the properties[] buffer with properties of type @type */
>> +static int virtio_iommu_fill_property(int type,
>> +                                      viommu_property_buffer *bufstate)
>> +{
>> +    int ret = -ENOSPC;
>> +
>> +    if (bufstate->filled + sizeof(struct virtio_iommu_probe_property)
>> +            >= VIOMMU_PROBE_SIZE) {
>> +        /* no space left for the header */
>> +        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_endpoint *ep = bufstate->endpoint;
>> +
>> +        g_tree_foreach(ep->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);
> 
> Nit: If this can really be triggered then we might still change it to
> error_report_once()?  If it's not (which it seems to), maybe assert
> directly?
I put error_report_once() at the moment. The reserved regions may be
passed through cfg or device properties. I think it may happen that
their size get larger than the size set in the device config.


> 
> Other than that it looks good to me:
> 
> Reviewed-by: Peter Xu <address@hidden>

Thank you for the review!

Best Regards

Eric
> 
> Regards,
>