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

[Peter Xu <peterx@redhat.com>]

On Tue, Sep 03, 2019 at 01:45:22PM +0200, Auger Eric wrote:
> 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).

I see. It's a bit unclear at least to me on how to use these.  I have
seen, and used error_report*() to report guest misbehaves as well just
for the debugging and triaging simply because error_report*() will
always be there even without "-d" (because when issue happens most
users are without it...).  Then with these information captured by
either libvirt or direct QEMU users we can triage guest bugs easier.
I hope I'm not severly wrong, and please feel free to use
qemu_log_mask() no matter what.

Regards,

-- 
Peter Xu