Re: [Qemu-devel] [PATCH for-4.2 v10 09/15] virtio-iommu: Implement translate

[Auger Eric]

Hi Peter,

On 8/19/19 10:24 AM, Peter Xu wrote:
> On Tue, Jul 30, 2019 at 07:21:31PM +0200, Eric Auger wrote:
>> @@ -464,19 +464,75 @@ static IOMMUTLBEntry 
>> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>                                              int iommu_idx)
>>  {
>>      IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr);
>> +    VirtIOIOMMU *s = sdev->viommu;
>>      uint32_t sid;
>> +    viommu_endpoint *ep;
>> +    viommu_mapping *mapping;
>> +    viommu_interval interval;
>> +    bool bypass_allowed;
>> +
>> +    interval.low = addr;
>> +    interval.high = addr + 1;
>>  
>>      IOMMUTLBEntry entry = {
>>          .target_as = &address_space_memory,
>>          .iova = addr,
>>          .translated_addr = addr,
>> -        .addr_mask = ~(hwaddr)0,
>> +        .addr_mask = (1 << ctz32(s->config.page_size_mask)) - 1,
>>          .perm = IOMMU_NONE,
>>      };
>>  
>> +    bypass_allowed = virtio_has_feature(s->acked_features,
>> +                                        VIRTIO_IOMMU_F_BYPASS);
>> +
>>      sid = virtio_iommu_get_sid(sdev);
>>  
>>      trace_virtio_iommu_translate(mr->parent_obj.name, sid, addr, flag);
>> +    qemu_mutex_lock(&s->mutex);
>> +
>> +    ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(sid));
>> +    if (!ep) {
>> +        if (!bypass_allowed) {
>> +            error_report("%s sid=%d is not known!!", __func__, sid);
> 
> Maybe use error_report_once() to avoid DOS attack?  Also would it be
> good to unify the debug prints?  I see both error_report() and
> qemu_log_mask() are used in the whole patchset.  Or is that attempted?

I switched to error_report_once()

I understand that qemu_log_mask() should be used whenever the root cause
is a bad action of the guest OS (in below case, the EP was not attached
to any domain). Above, there is an EP that attempts to talk through the
IOMMU and this was not expected (rather a platform description issue or
a qemu bug).

Thanks

Eric
> 
>> +        } else {
>> +            entry.perm = flag;
>> +        }
>> +        goto unlock;
>> +    }
>> +
>> +    if (!ep->domain) {
>> +        if (!bypass_allowed) {
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "%s %02x:%02x.%01x not attached to any domain\n",
>> +                          __func__, PCI_BUS_NUM(sid),
>> +                          PCI_SLOT(sid), PCI_FUNC(sid));
>> +        } else {
>> +            entry.perm = flag;
>> +        }
>> +        goto unlock;
>> +    }
>> +
>> +    mapping = g_tree_lookup(ep->domain->mappings, (gpointer)(&interval));
>> +    if (!mapping) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s no mapping for 0x%"PRIx64" for sid=%d\n",
>> +                      __func__, addr, sid);
>> +        goto unlock;
>> +    }
>> +
>> +    if (((flag & IOMMU_RO) && !(mapping->flags & VIRTIO_IOMMU_MAP_F_READ)) 
>> ||
>> +        ((flag & IOMMU_WO) && !(mapping->flags & 
>> VIRTIO_IOMMU_MAP_F_WRITE))) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "Permission error on 0x%"PRIx64"(%d): allowed=%d\n",
>> +                      addr, flag, mapping->flags);
>> +        goto unlock;
>> +    }
>> +    entry.translated_addr = addr - mapping->virt_addr + mapping->phys_addr;
>> +    entry.perm = flag;
>> +    trace_virtio_iommu_translate_out(addr, entry.translated_addr, sid);
>> +
>> +unlock:
>> +    qemu_mutex_unlock(&s->mutex);
>>      return entry;
>>  }
>>  
>> -- 
>> 2.20.1
>>
> 
> Regards,
>