On 05/23/2018 12:18 AM, Peter Xu wrote:
> Replace existing trace_vtd_err() with error_report_once() then stderr
> will capture something if any of the error happens, meanwhile we don't
> suffer from any DDOS. Then remove the trace point. Since at it,
> provide more information where proper (now we can pass parameters into
> the report function).
>
> Signed-off-by: Peter Xu <pet...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> ---
> hw/i386/intel_iommu.c | 59 ++++++++++++++++++++++++-------------------
> hw/i386/trace-events | 1 -
> 2 files changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index fb31de9416..fa6df921ee 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -285,14 +285,14 @@ static void vtd_generate_fault_event(IntelIOMMUState
> *s, uint32_t pre_fsts)
> {
> if (pre_fsts & VTD_FSTS_PPF || pre_fsts & VTD_FSTS_PFO ||
> pre_fsts & VTD_FSTS_IQE) {
> - trace_vtd_err("There are previous interrupt conditions "
> - "to be serviced by software, fault event "
> - "is not generated.");
> + error_report_once("There are previous interrupt conditions "
> + "to be serviced by software, fault event "
> + "is not generated");
> return;
> }
> vtd_set_clear_mask_long(s, DMAR_FECTL_REG, 0, VTD_FECTL_IP);
> if (vtd_get_long_raw(s, DMAR_FECTL_REG) & VTD_FECTL_IM) {
> - trace_vtd_err("Interrupt Mask set, irq is not generated.");
> + error_report_once("Interrupt Mask set, irq is not generated");
> } else {
> vtd_generate_interrupt(s, DMAR_FEADDR_REG, DMAR_FEDATA_REG);
> vtd_set_clear_mask_long(s, DMAR_FECTL_REG, VTD_FECTL_IP, 0);
> @@ -400,20 +400,20 @@ static void vtd_report_dmar_fault(IntelIOMMUState *s,
> uint16_t source_id,
> trace_vtd_dmar_fault(source_id, fault, addr, is_write);
>
> if (fsts_reg & VTD_FSTS_PFO) {
> - trace_vtd_err("New fault is not recorded due to "
> - "Primary Fault Overflow.");
> + error_report_once("New fault is not recorded due to "
> + "Primary Fault Overflow");
> return;
> }
>
> if (vtd_try_collapse_fault(s, source_id)) {
> - trace_vtd_err("New fault is not recorded due to "
> - "compression of faults.");
> + error_report_once("New fault is not recorded due to "
> + "compression of faults");
> return;
> }
>
> if (vtd_is_frcd_set(s, s->next_frcd_reg)) {
> - trace_vtd_err("Next Fault Recording Reg is used, "
> - "new fault is not recorded, set PFO field.");
> + error_report_once("Next Fault Recording Reg is used, "
> + "new fault is not recorded, set PFO field");
> vtd_set_clear_mask_long(s, DMAR_FSTS_REG, 0, VTD_FSTS_PFO);
> return;
> }
> @@ -421,8 +421,8 @@ static void vtd_report_dmar_fault(IntelIOMMUState *s,
> uint16_t source_id,
> vtd_record_frcd(s, s->next_frcd_reg, source_id, addr, fault, is_write);
>
> if (fsts_reg & VTD_FSTS_PPF) {
> - trace_vtd_err("There are pending faults already, "
> - "fault event is not generated.");
> + error_report_once("There are pending faults already, "
> + "fault event is not generated");
> vtd_set_frcd_and_update_ppf(s, s->next_frcd_reg);
> s->next_frcd_reg++;
> if (s->next_frcd_reg == DMAR_FRCD_REG_NR) {
> @@ -1339,7 +1339,8 @@ static uint64_t
> vtd_context_cache_invalidate(IntelIOMMUState *s, uint64_t val)
> break;
>
> default:
> - trace_vtd_err("Context cache invalidate type error.");
> + error_report_once("%s: invalid context: 0x%"PRIx64,
> + __func__, val);
> caig = 0;
> }
> return caig;
> @@ -1445,7 +1446,8 @@ static uint64_t vtd_iotlb_flush(IntelIOMMUState *s,
> uint64_t val)
> am = VTD_IVA_AM(addr);
> addr = VTD_IVA_ADDR(addr);
> if (am > VTD_MAMV) {
> - trace_vtd_err("IOTLB PSI flush: address mask overflow.");
> + error_report_once("%s: address mask overflow: 0x%"PRIx64,
> + __func__, vtd_get_quad_raw(s, DMAR_IVA_REG));
> iaig = 0;
> break;
> }
> @@ -1454,7 +1456,8 @@ static uint64_t vtd_iotlb_flush(IntelIOMMUState *s,
> uint64_t val)
> break;
>
> default:
> - trace_vtd_err("IOTLB flush: invalid granularity.");
> + error_report_once("%s: invalid granularity: 0x%"PRIx64,
> + __func__, val);
> iaig = 0;
> }
> return iaig;
> @@ -1604,8 +1607,8 @@ static void vtd_handle_ccmd_write(IntelIOMMUState *s)
> /* Context-cache invalidation request */
> if (val & VTD_CCMD_ICC) {
> if (s->qi_enabled) {
> - trace_vtd_err("Queued Invalidation enabled, "
> - "should not use register-based invalidation");
> + error_report_once("Queued Invalidation enabled, "
> + "should not use register-based invalidation");
> return;
> }
> ret = vtd_context_cache_invalidate(s, val);
> @@ -1625,8 +1628,8 @@ static void vtd_handle_iotlb_write(IntelIOMMUState *s)
> /* IOTLB invalidation request */
> if (val & VTD_TLB_IVT) {
> if (s->qi_enabled) {
> - trace_vtd_err("Queued Invalidation enabled, "
> - "should not use register-based invalidation.");
> + error_report_once("Queued Invalidation enabled, "
> + "should not use register-based invalidation");
> return;
> }
> ret = vtd_iotlb_flush(s, val);
> @@ -1644,7 +1647,7 @@ static bool vtd_get_inv_desc(dma_addr_t base_addr,
> uint32_t offset,
> dma_addr_t addr = base_addr + offset * sizeof(*inv_desc);
> if (dma_memory_read(&address_space_memory, addr, inv_desc,
> sizeof(*inv_desc))) {
> - trace_vtd_err("Read INV DESC failed.");
> + error_report_once("Read INV DESC failed");
> inv_desc->lo = 0;
> inv_desc->hi = 0;
> return false;
> @@ -1999,7 +2002,8 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr,
> unsigned size)
> trace_vtd_reg_read(addr, size);
>
> if (addr + size > DMAR_REG_SIZE) {
> - trace_vtd_err("Read MMIO over range.");
> + error_report_once("%s: MMIO over range: addr=0x%"PRIx64
> + " size=0x%u", __func__, addr, size);
> return (uint64_t)-1;
> }
>
> @@ -2050,7 +2054,8 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
> trace_vtd_reg_write(addr, size, val);
>
> if (addr + size > DMAR_REG_SIZE) {
> - trace_vtd_err("Write MMIO over range.");
> + error_report_once("%s: MMIO over range: addr=0x%"PRIx64
> + " size=0x%u", __func__, addr, size);
> return;
> }
>
> @@ -2432,7 +2437,8 @@ static int vtd_irte_get(IntelIOMMUState *iommu,
> uint16_t index,
> addr = iommu->intr_root + index * sizeof(*entry);
> if (dma_memory_read(&address_space_memory, addr, entry,
> sizeof(*entry))) {
> - trace_vtd_err("Memory read failed for IRTE.");
> + error_report_once("%s: read failed: ind=0x%"PRIu16
> + " addr=0x%"PRIx64, __func__, index, addr);
> return -VTD_FR_IR_ROOT_INVAL;
> }
>
> @@ -2564,14 +2570,15 @@ static int vtd_interrupt_remap_msi(IntelIOMMUState
> *iommu,
> }
>
> if (origin->address & VTD_MSI_ADDR_HI_MASK) {
> - trace_vtd_err("MSI address high 32 bits non-zero when "
> - "Interrupt Remapping enabled.");
> + error_report_once("%s: MSI address high 32 bits non-zero detected: "
> + "address=0x%"PRIx64, __func__, origin->address);
> return -VTD_FR_IR_REQ_RSVD;
> }
>
> addr.data = origin->address & VTD_MSI_ADDR_LO_MASK;
> if (addr.addr.__head != 0xfee) {
> - trace_vtd_err("MSI addr low 32 bit invalid.");
> + error_report_once("%s: MSI address low 32 bit invalid: 0x%"PRIx32,
> + __func__, addr.data);
> return -VTD_FR_IR_REQ_RSVD;
> }
>
> diff --git a/hw/i386/trace-events b/hw/i386/trace-events
> index 22d44648af..e08cf2a9a7 100644
> --- a/hw/i386/trace-events
> +++ b/hw/i386/trace-events
> @@ -68,7 +68,6 @@ vtd_ir_remap_msi_req(uint64_t addr, uint64_t data) "addr
> 0x%"PRIx64" data 0x%"PR
> vtd_fsts_ppf(bool set) "FSTS PPF bit set to %d"
> vtd_fsts_clear_ip(void) ""
> vtd_frr_new(int index, uint64_t hi, uint64_t lo) "index %d high 0x%"PRIx64"
> low 0x%"PRIx64
> -vtd_err(const char *str) "%s"
> vtd_err_dmar_iova_overflow(uint64_t iova) "iova 0x%"PRIx64
> vtd_err_dmar_slpte_read_error(uint64_t iova, int level) "iova 0x%"PRIx64"
> level %d"
> vtd_err_dmar_slpte_perm_error(uint64_t iova, int level, uint64_t slpte, bool
> is_write) "iova 0x%"PRIx64" level %d slpte 0x%"PRIx64" write %d"
>
