Re: [PATCH for-5.0 v11 07/20] virtio-iommu: Implement map/unmap

[Auger Eric]

Hi jean,

On 12/10/19 5:43 PM, Jean-Philippe Brucker wrote:
> On Fri, Nov 22, 2019 at 07:29:30PM +0100, Eric Auger wrote:
>> @@ -238,10 +244,35 @@ static int virtio_iommu_map(VirtIOIOMMU *s,
>>      uint64_t virt_start = le64_to_cpu(req->virt_start);
>>      uint64_t virt_end = le64_to_cpu(req->virt_end);
>>      uint32_t flags = le32_to_cpu(req->flags);
>> +    viommu_domain *domain;
>> +    viommu_interval *interval;
>> +    viommu_mapping *mapping;
> 
> Additional checks would be good. Most importantly we need to return
> S_INVAL if we don't recognize a bit in flags (a MUST in the spec). 
Sure

It
> might be good to check that addresses are aligned on the page granule as
> well, and return S_RANGE if they aren't (a SHOULD in the spec), but I
> don't care as much.
with KVM accelerated guest I don't have access to the guest page size,
hence the choice of not checking it.
> 
>> +
>> +    interval = g_malloc0(sizeof(*interval));
>> +
>> +    interval->low = virt_start;
>> +    interval->high = virt_end;
>> +
>> +    domain = g_tree_lookup(s->domains, GUINT_TO_POINTER(domain_id));
>> +    if (!domain) {
>> +        return VIRTIO_IOMMU_S_NOENT;
> 
> Leaks interval, I guess you could allocate it after this block.
Sure

Thanks!

Eric
> 
> Thanks,
> Jean
> 
>> +    }
>> +
>> +    mapping = g_tree_lookup(domain->mappings, (gpointer)interval);
>> +    if (mapping) {
>> +        g_free(interval);
>> +        return VIRTIO_IOMMU_S_INVAL;
>> +    }
>>  
>>      trace_virtio_iommu_map(domain_id, virt_start, virt_end, phys_start, 
>> flags);
>>  
>> -    return VIRTIO_IOMMU_S_UNSUPP;
>> +    mapping = g_malloc0(sizeof(*mapping));
>> +    mapping->phys_addr = phys_start;
>> +    mapping->flags = flags;
>> +
>> +    g_tree_insert(domain->mappings, interval, mapping);
>> +
>> +    return VIRTIO_IOMMU_S_OK;
>