Re: [Qemu-devel] [PATCH RFC v3 04/14] intel_iommu: fix trace for inv desc handling

[Jason Wang <jasowang@redhat.com>]

On 2017年01月13日 17:13, Peter Xu wrote:
> On Fri, Jan 13, 2017 at 03:46:31PM +0800, Jason Wang wrote:
>>
>> On 2017年01月13日 11:06, Peter Xu wrote:
>>> VT-d codes are still using static DEBUG_INTEL_IOMMU macro. That's not
>>> good, and we should end the day when we need to recompile the code
>>> before getting useful debugging information for vt-d. Time to switch to
>>> the trace system.
>>>
>>> This is the first patch to do it.
>>>
>>> Generally, the rule of mine is:
>>>
>>> - for the old GENERAL typed message, I use error_report() directly if
>>>    apply. Those are something shouldn't happen, and we should print those
>>>    errors in all cases, even without enabling debug and tracing.
>> Looks like some were guest trigger-able. If yes, let's try not use
>> error_report() for not being flooded.
> Yes, it's intended. Most of the error_report()s in this patch can be
> triggered by guest, but only by illegal guest behaviors (e.g.,
> non-zero reserved fields, or illegal descriptors, etc.). In that
> sense, shall we keep them even guest can trigger them? Since people
> will never see them if they are running generic and good kernels. More
> importantly, these error_report()s can be good hints when guest
> encounters issues, for better debugging and triaging.
>
> Actually we have such usage in existing QEMU as well. For example,
> when we maintain the DMA mapping in vfio-pci, it's possible that the
> shadow page table is mapped illegally due to some reason (that depends
> on the guest as well, may not be guest kernel, but DPDK applications
> inside guest), and the map() can fail. Here we have:
>
>      ret = vfio_dma_map(container, iova,
>                          iotlb->addr_mask + 1, vaddr,
>                          !(iotlb->perm & IOMMU_WO) || mr->readonly);
>      if (ret) {
>          error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
>                          "0x%"HWADDR_PRIx", %p) = %d (%m)",
>                          container, iova,
>                          iotlb->addr_mask + 1, vaddr, ret);
>      }
>
> Which I think is playing the same role here - we will never see these
> lines if the guest is normal, and these lines will be useful when bad
> things happen.
>
> So I would slightly prefer that we keep these error_reports() for now,
> as long as they won't flush the screen for most of the users. (during
> the time I played with this series, none of them jumped out :)

I think the point is just surviving from malicious guests. So we need 
avoid guest trigger-able thing likes this, consider if we redirect 
stderr to a log file, malicious guest may exhaust disk space which is a 
DOS. So we'd better avoid them.

Thanks

>
> Thanks,
>
> -- peterx