[Qemu-devel] [PATCH v2 0/2] error-report: introduce {error|warn}_report_once

[Qemu-devel] [PATCH v2 0/2] error-report: introduce {error|warn}_report_once

[Peter Xu]

v2:
- for patch 1: replace tabs, add trivial comment [Markus]
  (I didn't add much comment otherwise I'll need to duplicate what's
   there in error_report())
- add patch 2

Patch 1 introduce the helpers.

Patch 2 use it to replace VT-d trace_vtd_err().

Please review.  Thanks.

Peter Xu (2):
  qemu-error: introduce {error|warn}_report_once
  intel-iommu: start to use error_report_once

 include/qemu/error-report.h | 26 ++++++++++++++++++++++++++
 hw/i386/intel_iommu.c       | 34 +++++++++++++++++-----------------
 hw/i386/trace-events        |  1 -
 3 files changed, 43 insertions(+), 18 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v2 1/2] qemu-error: introduce {error|warn}_report_once

[Peter Xu]

I stole the printk_once() macro.

I always wanted to be able to print some error directly if there is a
buffer to dump, however we can't use error_report() where the code path
can be triggered by DDOS attack.  To avoid that, we can introduce a
print-once-like function for it.  Meanwhile, we also introduce the
corresponding helper for warn_report().

CC: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qemu/error-report.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index e1c8ae1a52..3e6e84801f 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -44,6 +44,32 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void warn_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
+/* Similar to error_report(), but it only prints the message once. */
+#define error_report_once(fmt, ...)             \
+    ({                                          \
+        static bool __print_once;               \
+        bool __ret_print_once = !__print_once;  \
+                                                \
+        if (!__print_once) {                    \
+            __print_once = true;                \
+            error_report(fmt, ##__VA_ARGS__);   \
+        }                                       \
+        unlikely(__ret_print_once);             \
+    })
+
+/* Similar to warn_report(), but it only prints the message once. */
+#define warn_report_once(fmt, ...)              \
+    ({                                          \
+        static bool __print_once;               \
+        bool __ret_print_once = !__print_once;  \
+                                                \
+        if (!__print_once) {                    \
+            __print_once = true;                \
+            warn_report(fmt, ##__VA_ARGS__);   \
+        }                                       \
+        unlikely(__ret_print_once);             \
+    })
+
 const char *error_get_progname(void);
 extern bool enable_timestamp_msg;
 
-- 
2.17.0

[Qemu-devel] [PATCH v2 2/2] intel-iommu: start to use error_report_once

[Peter Xu]

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.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 34 +++++++++++++++++-----------------
 hw/i386/trace-events  |  1 -
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index fb31de9416..cf655fb9f6 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 "
+        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,19 +400,19 @@ 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 "
+        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 "
+        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, "
+        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,7 +421,7 @@ 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, "
+        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++;
@@ -1339,7 +1339,7 @@ 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("Context cache invalidate type error.");
         caig = 0;
     }
     return caig;
@@ -1445,7 +1445,7 @@ 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("IOTLB PSI flush: address mask overflow.");
             iaig = 0;
             break;
         }
@@ -1454,7 +1454,7 @@ static uint64_t vtd_iotlb_flush(IntelIOMMUState *s, uint64_t val)
         break;
 
     default:
-        trace_vtd_err("IOTLB flush: invalid granularity.");
+        error_report_once("IOTLB flush: invalid granularity.");
         iaig = 0;
     }
     return iaig;
@@ -1604,7 +1604,7 @@ 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, "
+            error_report_once("Queued Invalidation enabled, "
                           "should not use register-based invalidation");
             return;
         }
@@ -1625,7 +1625,7 @@ 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, "
+            error_report_once("Queued Invalidation enabled, "
                           "should not use register-based invalidation.");
             return;
         }
@@ -1644,7 +1644,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 +1999,7 @@ 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("Read MMIO over range.");
         return (uint64_t)-1;
     }
 
@@ -2050,7 +2050,7 @@ 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("Write MMIO over range.");
         return;
     }
 
@@ -2432,7 +2432,7 @@ 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("Memory read failed for IRTE.");
         return -VTD_FR_IR_ROOT_INVAL;
     }
 
@@ -2564,14 +2564,14 @@ 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 "
+        error_report_once("MSI address high 32 bits non-zero when "
                       "Interrupt Remapping enabled.");
         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("MSI addr low 32 bit invalid.");
         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"
-- 
2.17.0

Re: [Qemu-devel] [PATCH v2 0/2] error-report: introduce {error|warn}_report_once

[Peter Xu]

On Tue, May 22, 2018 at 11:56:27AM +0800, Peter Xu wrote:
> v2:
> - for patch 1: replace tabs, add trivial comment [Markus]
>   (I didn't add much comment otherwise I'll need to duplicate what's
>    there in error_report())
> - add patch 2
> 
> Patch 1 introduce the helpers.
> 
> Patch 2 use it to replace VT-d trace_vtd_err().
> 
> Please review.  Thanks.

Sorry I forgot to CC Eric in the series.  Adding in.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 2/2] intel-iommu: start to use error_report_once

[Philippe Mathieu-Daudé]

Hi Peter,

On 05/22/2018 12:56 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.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 34 +++++++++++++++++-----------------
>  hw/i386/trace-events  |  1 -
>  2 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index fb31de9416..cf655fb9f6 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 "
> +        error_report_once("There are previous interrupt conditions "
>                        "to be serviced by software, fault event "
>                        "is not generated.");

Can you keep alignment consistent please?

>          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,19 +400,19 @@ 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 "
> +        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 "
> +        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, "
> +        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,7 +421,7 @@ 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, "
> +        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++;
> @@ -1339,7 +1339,7 @@ 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("Context cache invalidate type error.");
>          caig = 0;
>      }
>      return caig;
> @@ -1445,7 +1445,7 @@ 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("IOTLB PSI flush: address mask overflow.");
>              iaig = 0;
>              break;
>          }
> @@ -1454,7 +1454,7 @@ static uint64_t vtd_iotlb_flush(IntelIOMMUState *s, uint64_t val)
>          break;
>  
>      default:
> -        trace_vtd_err("IOTLB flush: invalid granularity.");
> +        error_report_once("IOTLB flush: invalid granularity.");
>          iaig = 0;
>      }
>      return iaig;
> @@ -1604,7 +1604,7 @@ 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, "
> +            error_report_once("Queued Invalidation enabled, "
>                            "should not use register-based invalidation");
>              return;
>          }
> @@ -1625,7 +1625,7 @@ 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, "
> +            error_report_once("Queued Invalidation enabled, "
>                            "should not use register-based invalidation.");
>              return;
>          }
> @@ -1644,7 +1644,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 +1999,7 @@ 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("Read MMIO over range.");
>          return (uint64_t)-1;
>      }
>  
> @@ -2050,7 +2050,7 @@ 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("Write MMIO over range.");
>          return;
>      }
>  
> @@ -2432,7 +2432,7 @@ 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("Memory read failed for IRTE.");
>          return -VTD_FR_IR_ROOT_INVAL;
>      }
>  
> @@ -2564,14 +2564,14 @@ 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 "
> +        error_report_once("MSI address high 32 bits non-zero when "
>                        "Interrupt Remapping enabled.");
>          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("MSI addr low 32 bit invalid.");
>          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"
> 

Re: [Qemu-devel] [PATCH v2 2/2] intel-iommu: start to use error_report_once

[Peter Xu]

On Tue, May 22, 2018 at 06:09:46PM -0300, Philippe Mathieu-Daudé wrote:
> Hi Peter,
> 
> On 05/22/2018 12:56 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.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/i386/intel_iommu.c | 34 +++++++++++++++++-----------------
> >  hw/i386/trace-events  |  1 -
> >  2 files changed, 17 insertions(+), 18 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index fb31de9416..cf655fb9f6 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 "
> > +        error_report_once("There are previous interrupt conditions "
> >                        "to be serviced by software, fault event "
> >                        "is not generated.");
> 
> Can you keep alignment consistent please?

That was actually intended so reviewers don't need to skim those lines
only caused by indents.  If I'm going to touch those lines, then maybe
I can make them even look better by dumping more valid information.

I'll repost.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 1/2] qemu-error: introduce {error|warn}_report_once

[Eric Blake]

On 05/21/2018 10:56 PM, Peter Xu wrote:
> I stole the printk_once() macro.
> 
> I always wanted to be able to print some error directly if there is a
> buffer to dump, however we can't use error_report() where the code path
> can be triggered by DDOS attack.  To avoid that, we can introduce a
> print-once-like function for it.  Meanwhile, we also introduce the
> corresponding helper for warn_report().
> 
> CC: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/qemu/error-report.h | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index e1c8ae1a52..3e6e84801f 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -44,6 +44,32 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>   void warn_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>   void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>   
> +/* Similar to error_report(), but it only prints the message once. */
> +#define error_report_once(fmt, ...)             \
> +    ({                                          \
> +        static bool __print_once;               \

You're still using the reserved namespace.  I asked you to fix that on 
v1; it's disappointing when a later revision doesn't make changes 
requested by a reviewers, without also offering a counter-argument for 
why the request does not need to be considered.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 1/2] qemu-error: introduce {error|warn}_report_once

[Peter Xu]

On Wed, May 23, 2018 at 10:47:29AM -0500, Eric Blake wrote:
> On 05/21/2018 10:56 PM, Peter Xu wrote:
> > I stole the printk_once() macro.
> > 
> > I always wanted to be able to print some error directly if there is a
> > buffer to dump, however we can't use error_report() where the code path
> > can be triggered by DDOS attack.  To avoid that, we can introduce a
> > print-once-like function for it.  Meanwhile, we also introduce the
> > corresponding helper for warn_report().
> > 
> > CC: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   include/qemu/error-report.h | 26 ++++++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> > 
> > diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> > index e1c8ae1a52..3e6e84801f 100644
> > --- a/include/qemu/error-report.h
> > +++ b/include/qemu/error-report.h
> > @@ -44,6 +44,32 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> >   void warn_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> >   void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> > +/* Similar to error_report(), but it only prints the message once. */
> > +#define error_report_once(fmt, ...)             \
> > +    ({                                          \
> > +        static bool __print_once;               \
> 
> You're still using the reserved namespace.  I asked you to fix that on v1;
> it's disappointing when a later revision doesn't make changes requested by a
> reviewers, without also offering a counter-argument for why the request does
> not need to be considered.

I am sorry.  It's totally because I forgot that one!  (It rarely
happens since I note them down in most cases, but there must be
something that even interrupted the procedure when I took notes...)

I will repost with another one soon.  Sorry again.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 2/2] intel-iommu: start to use error_report_once

[Philippe Mathieu-Daudé]

On 05/22/2018 10:19 PM, Peter Xu wrote:
> On Tue, May 22, 2018 at 06:09:46PM -0300, Philippe Mathieu-Daudé wrote:
>> Hi Peter,
>>
>> On 05/22/2018 12:56 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.
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>  hw/i386/intel_iommu.c | 34 +++++++++++++++++-----------------
>>>  hw/i386/trace-events  |  1 -
>>>  2 files changed, 17 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index fb31de9416..cf655fb9f6 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 "
>>> +        error_report_once("There are previous interrupt conditions "
>>>                        "to be serviced by software, fault event "
>>>                        "is not generated.");
>>
>> Can you keep alignment consistent please?
> 
> That was actually intended so reviewers don't need to skim those lines
> only caused by indents.  If I'm going to touch those lines, then maybe
> I can make them even look better by dumping more valid information.

I see.  I agree it is easier to review lines with substantial change,
however I now think it is then nicer to let the code exemplary for the
next one, if possible.  So regardless your preference:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Regards,

Phil.