[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Jason Wang
Subject: Re: [Qemu-devel] [PATCH RFC v3 04/14] intel_iommu: fix trace for inv desc handling
Date: Fri, 13 Jan 2017 17:33:16 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1

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.



-- peterx

reply via email to

[Prev in Thread] Current Thread [Next in Thread]