[Qemu-devel] [PATCH 1/3] pseries: Fix RTAS based config access

[Qemu-devel] [PATCH 1/3] pseries: Fix RTAS based config access

[David Gibson]

On the pseries platform, access to PCI config space is via RTAS calls(
which go to the hypervisor) rather than MMIO.  This means we don't use
the same code path as nearly everyone else which goes through pci_host.c
and we're missing some of the parameter checking along the way.

We do have some parameter checking in the RTAS calls, but it's not enough.
It checks for overruns, but does not check for unaligned accesses,
oversized accesses (which means the guest could trigger an assertion
failure from pci_host_config_{read,write}_common().  Worse it doesn't do
the basic checking for the number of RTAS arguments and results before
accessing them.

This patch fixes these bugs.

Cc: Michael S. Tsirkin <mst@redhat.com>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/spapr_pci.c |  117 +++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 79 insertions(+), 38 deletions(-)

diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index e7ef551..1cf84e7 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -57,26 +57,38 @@ static PCIDevice *find_dev(sPAPREnvironment *spapr,
 
 static uint32_t rtas_pci_cfgaddr(uint32_t arg)
 {
+    /* This handles the encoding of extended config space addresses */
     return ((arg >> 20) & 0xf00) | (arg & 0xff);
 }
 
-static uint32_t rtas_read_pci_config_do(PCIDevice *pci_dev, uint32_t addr,
-                                        uint32_t limit, uint32_t len)
+static void finish_read_pci_config(sPAPREnvironment *spapr, uint64_t buid,
+                                   uint32_t addr, uint32_t size,
+                                   target_ulong rets)
 {
-    if ((addr + len) <= limit) {
-        return pci_host_config_read_common(pci_dev, addr, limit, len);
-    } else {
-        return ~0x0;
+    PCIDevice *pci_dev;
+    uint32_t val;
+
+    if ((size != 1) && (size != 2) && (size != 4)) {
+        /* access must be 1, 2 oe 4 bytes */
+        rtas_st(rets, 0, -1);
+        return;
     }
-}
 
-static void rtas_write_pci_config_do(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t val,
-                                     uint32_t len)
-{
-    if ((addr + len) <= limit) {
-        pci_host_config_write_common(pci_dev, addr, limit, val, len);
+    pci_dev = find_dev(spapr, buid, addr);
+    addr = rtas_pci_cfgaddr(addr);
+
+    if (!pci_dev || (addr % size) || (addr >= pci_config_size(pci_dev))) {
+        /* Access must be to a valid device, within bounds and
+         * naturally aligned */
+        rtas_st(rets, 0, -1);
+        return;
     }
+
+    val = pci_host_config_read_common(pci_dev, addr,
+                                      pci_config_size(pci_dev), size);
+
+    rtas_st(rets, 0, 0);
+    rtas_st(rets, 1, val);
 }
 
 static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
@@ -84,19 +96,19 @@ static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
                                      target_ulong args,
                                      uint32_t nret, target_ulong rets)
 {
-    uint32_t val, size, addr;
-    uint64_t buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
-    PCIDevice *dev = find_dev(spapr, buid, rtas_ld(args, 0));
+    uint64_t buid;
+    uint32_t size, addr;
 
-    if (!dev) {
+    if ((nargs != 4) || (nret != 2)) {
         rtas_st(rets, 0, -1);
         return;
     }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
     size = rtas_ld(args, 3);
-    addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
-    rtas_st(rets, 0, 0);
-    rtas_st(rets, 1, val);
+    addr = rtas_ld(args, 0);
+
+    finish_read_pci_config(spapr, buid, addr, size, rets);
 }
 
 static void rtas_read_pci_config(sPAPREnvironment *spapr,
@@ -104,18 +116,45 @@ static void rtas_read_pci_config(sPAPREnvironment *spapr,
                                  target_ulong args,
                                  uint32_t nret, target_ulong rets)
 {
-    uint32_t val, size, addr;
-    PCIDevice *dev = find_dev(spapr, 0, rtas_ld(args, 0));
+    uint32_t size, addr;
 
-    if (!dev) {
+    if ((nargs != 2) || (nret != 2)) {
         rtas_st(rets, 0, -1);
         return;
     }
+
     size = rtas_ld(args, 1);
-    addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
+    addr = rtas_ld(args, 0);
+
+    finish_read_pci_config(spapr, 0, addr, size, rets);
+}
+
+static void finish_write_pci_config(sPAPREnvironment *spapr, uint64_t buid,
+                                    uint32_t addr, uint32_t size,
+                                    uint32_t val, target_ulong rets)
+{
+    PCIDevice *pci_dev;
+
+    if ((size != 1) && (size != 2) && (size != 4)) {
+        /* access must be 1, 2 oe 4 bytes */
+        rtas_st(rets, 0, -1);
+        return;
+    }
+
+    pci_dev = find_dev(spapr, buid, addr);
+    addr = rtas_pci_cfgaddr(addr);
+
+    if (!pci_dev || (addr % size) || (addr >= pci_config_size(pci_dev))) {
+        /* Access must be to a valid device, within bounds and
+         * naturally aligned */
+        rtas_st(rets, 0, -1);
+        return;
+    }
+
+    pci_host_config_write_common(pci_dev, addr, pci_config_size(pci_dev),
+                                 val, size);
+
     rtas_st(rets, 0, 0);
-    rtas_st(rets, 1, val);
 }
 
 static void rtas_ibm_write_pci_config(sPAPREnvironment *spapr,
@@ -123,19 +162,20 @@ static void rtas_ibm_write_pci_config(sPAPREnvironment *spapr,
                                       target_ulong args,
                                       uint32_t nret, target_ulong rets)
 {
+    uint64_t buid;
     uint32_t val, size, addr;
-    uint64_t buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
-    PCIDevice *dev = find_dev(spapr, buid, rtas_ld(args, 0));
 
-    if (!dev) {
+    if ((nargs != 5) || (nret != 1)) {
         rtas_st(rets, 0, -1);
         return;
     }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
     val = rtas_ld(args, 4);
     size = rtas_ld(args, 3);
-    addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
-    rtas_st(rets, 0, 0);
+    addr = rtas_ld(args, 0);
+
+    finish_write_pci_config(spapr, buid, addr, size, val, rets);
 }
 
 static void rtas_write_pci_config(sPAPREnvironment *spapr,
@@ -144,17 +184,18 @@ static void rtas_write_pci_config(sPAPREnvironment *spapr,
                                   uint32_t nret, target_ulong rets)
 {
     uint32_t val, size, addr;
-    PCIDevice *dev = find_dev(spapr, 0, rtas_ld(args, 0));
 
-    if (!dev) {
+    if ((nargs != 3) || (nret != 1)) {
         rtas_st(rets, 0, -1);
         return;
     }
+
+
     val = rtas_ld(args, 2);
     size = rtas_ld(args, 1);
-    addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
-    rtas_st(rets, 0, 0);
+    addr = rtas_ld(args, 0);
+
+    finish_write_pci_config(spapr, 0, addr, size, val, rets);
 }
 
 static int pci_spapr_map_irq(PCIDevice *pci_dev, int irq_num)
-- 
1.7.9.1

[Qemu-devel] [PATCH 2/3] pseries: Use more conventional PCI interrupt swizzling

[David Gibson]

Currently the pseries PCI code uses a somewhat strange scheme of PCI irq
allocation - one per slot up to a maximum that's greater than the usual 4.
This scheme more or less worked, because we were able to tell the guest the
irq mapping in the device tree, however it's nonstandard and may break
assumptions in the future.  Worse, the array used to construct the dev
tree interrupt map was mis-sized, we got away with it only because it
happened that our SPAPR_PCI_NUM_LSI value was greater than 7.

This patch changes the pseries PCI code to use the normal PCI interrupt
swizzling scheme instead, and corrects allocation of the irq map.

Cc: Michael S. Tsirkin <mst@redhat.com>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/spapr_pci.c |   49 ++++++++++++++++++++++++++++---------------------
 hw/spapr_pci.h |    5 ++---
 2 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index 1cf84e7..b8a0313 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -198,16 +198,20 @@ static void rtas_write_pci_config(sPAPREnvironment *spapr,
     finish_write_pci_config(spapr, 0, addr, size, val, rets);
 }
 
+static int pci_swizzle(int slot, int pin)
+{
+    return (slot + pin) % PCI_NUM_PINS;
+}
+
 static int pci_spapr_map_irq(PCIDevice *pci_dev, int irq_num)
 {
     /*
      * Here we need to convert pci_dev + irq_num to some unique value
-     * which is less than number of IRQs on the specific bus (now it
-     * is 16).  At the moment irq_num == device_id (number of the
-     * slot?)
-     * FIXME: we should swizzle in fn and irq_num
+     * which is less than number of IRQs on the specific bus (4).  We
+     * use standard PCI swizzling, that is (slot number + pin number)
+     * % 4.
      */
-    return (pci_dev->devfn >> 3) % SPAPR_PCI_NUM_LSI;
+    return pci_swizzle(PCI_SLOT(pci_dev->devfn), irq_num);
 }
 
 static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
@@ -304,13 +308,13 @@ static int spapr_phb_init(SysBusDevice *s)
                            phb->busname ? phb->busname : phb->dtbusname,
                            pci_spapr_set_irq, pci_spapr_map_irq, phb,
                            &phb->memspace, &phb->iospace,
-                           PCI_DEVFN(0, 0), SPAPR_PCI_NUM_LSI);
+                           PCI_DEVFN(0, 0), PCI_NUM_PINS);
     phb->host_state.bus = bus;
 
     QLIST_INSERT_HEAD(&spapr->phbs, phb, list);
 
     /* Initialize the LSI table */
-    for (i = 0; i < SPAPR_PCI_NUM_LSI; i++) {
+    for (i = 0; i < PCI_NUM_PINS; i++) {
         qemu_irq qirq;
         uint32_t num;
 
@@ -392,8 +396,7 @@ int spapr_populate_pci_devices(sPAPRPHBState *phb,
                                uint32_t xics_phandle,
                                void *fdt)
 {
-    PCIBus *bus = phb->host_state.bus;
-    int bus_off, i;
+    int bus_off, i, j;
     char nodename[256];
     uint32_t bus_range[] = { cpu_to_be32(0), cpu_to_be32(0xff) };
     struct {
@@ -415,8 +418,8 @@ int spapr_populate_pci_devices(sPAPRPHBState *phb,
     };
     uint64_t bus_reg[] = { cpu_to_be64(phb->buid), 0 };
     uint32_t interrupt_map_mask[] = {
-        cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, 0x0};
-    uint32_t interrupt_map[bus->nirq][7];
+        cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
+    uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
 
     /* Start populating the FDT */
     sprintf(nodename, "pci@%" PRIx64, phb->buid);
@@ -450,19 +453,23 @@ int spapr_populate_pci_devices(sPAPRPHBState *phb,
      */
     _FDT(fdt_setprop(fdt, bus_off, "interrupt-map-mask",
                      &interrupt_map_mask, sizeof(interrupt_map_mask)));
-    for (i = 0; i < 7; i++) {
-        uint32_t *irqmap = interrupt_map[i];
-        irqmap[0] = cpu_to_be32(b_ddddd(i)|b_fff(0));
-        irqmap[1] = 0;
-        irqmap[2] = 0;
-        irqmap[3] = 0;
-        irqmap[4] = cpu_to_be32(xics_phandle);
-        irqmap[5] = cpu_to_be32(phb->lsi_table[i % SPAPR_PCI_NUM_LSI].dt_irq);
-        irqmap[6] = cpu_to_be32(0x8);
+    for (i = 0; i < PCI_SLOT_MAX; i++) {
+        for (j = 0; j < PCI_NUM_PINS; j++) {
+            uint32_t *irqmap = interrupt_map[i*PCI_NUM_PINS + j];
+            int lsi_num = pci_swizzle(i, j);
+
+            irqmap[0] = cpu_to_be32(b_ddddd(i)|b_fff(0));
+            irqmap[1] = 0;
+            irqmap[2] = 0;
+            irqmap[3] = cpu_to_be32(j+1);
+            irqmap[4] = cpu_to_be32(xics_phandle);
+            irqmap[5] = cpu_to_be32(phb->lsi_table[lsi_num].dt_irq);
+            irqmap[6] = cpu_to_be32(0x8);
+        }
     }
     /* Write interrupt map */
     _FDT(fdt_setprop(fdt, bus_off, "interrupt-map", &interrupt_map,
-                     7 * sizeof(interrupt_map[0])));
+                     sizeof(interrupt_map)));
 
     return 0;
 }
diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
index 039f85b..f54c2e8 100644
--- a/hw/spapr_pci.h
+++ b/hw/spapr_pci.h
@@ -23,11 +23,10 @@
 #if !defined(__HW_SPAPR_PCI_H__)
 #define __HW_SPAPR_PCI_H__
 
+#include "hw/pci.h"
 #include "hw/pci_host.h"
 #include "hw/xics.h"
 
-#define SPAPR_PCI_NUM_LSI   16
-
 typedef struct sPAPRPHBState {
     SysBusDevice busdev;
     PCIHostState host_state;
@@ -43,7 +42,7 @@ typedef struct sPAPRPHBState {
     struct {
         uint32_t dt_irq;
         qemu_irq qirq;
-    } lsi_table[SPAPR_PCI_NUM_LSI];
+    } lsi_table[PCI_NUM_PINS];
 
     QLIST_ENTRY(sPAPRPHBState) list;
 } sPAPRPHBState;
-- 
1.7.9.1

[Qemu-devel] [PATCH 3/3] pseries: Add DPRINTF macros to spapr pci code

[David Gibson]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

This adds DPRINTF() macros with the usual conventions to the spapr_pci
code.

Cc: Michael S. Tsirkin <mst@redhat.com>

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/spapr_pci.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index b8a0313..61a53d5 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -32,6 +32,14 @@
 
 #include "hw/pci_internals.h"
 
+/*#define DEBUG_SPAPR_PCI*/
+
+#ifdef DEBUG_SPAPR_PCI
+# define DPRINTF(format, ...) fprintf(stderr, "QEMU: " format, ## __VA_ARGS__)
+#else
+# define DPRINTF(format, ...) do { } while (0)
+#endif
+
 static PCIDevice *find_dev(sPAPREnvironment *spapr,
                            uint64_t buid, uint32_t config_addr)
 {
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 3/3] pseries: Add DPRINTF macros to spapr pci code

[Blue Swirl]

On Mon, Apr 2, 2012 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> This adds DPRINTF() macros with the usual conventions to the spapr_pci
> code.

Please use tracepoints instead of printf statements. Tracing is more
flexible, more efficient and does not suffer from bitrot.

> Cc: Michael S. Tsirkin <mst@redhat.com>
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/spapr_pci.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index b8a0313..61a53d5 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -32,6 +32,14 @@
>
>  #include "hw/pci_internals.h"
>
> +/*#define DEBUG_SPAPR_PCI*/
> +
> +#ifdef DEBUG_SPAPR_PCI
> +# define DPRINTF(format, ...) fprintf(stderr, "QEMU: " format, ## __VA_ARGS__)
> +#else
> +# define DPRINTF(format, ...) do { } while (0)
> +#endif
> +
>  static PCIDevice *find_dev(sPAPREnvironment *spapr,
>                            uint64_t buid, uint32_t config_addr)
>  {
> --
> 1.7.9.1
>
>

Re: [Qemu-devel] [PATCH 3/3] pseries: Add DPRINTF macros to spapr pci code

[Peter Maydell]

On 4 April 2012 20:18, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Apr 2, 2012 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
>> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>> This adds DPRINTF() macros with the usual conventions to the spapr_pci
>> code.
>
> Please use tracepoints instead of printf statements. Tracing is more
> flexible, more efficient and does not suffer from bitrot.

I'd much rather enable a #define to turn on debugging than faff about
with tracing. It's simple and straightforward, you can do it with a
single obvious change and recompile, and nobody has to look up
documentation to figure out how it works.

If tracepoints were always-compiled-in and enabled at runtime I'd
agree with you: then you could have linux-kernel-style "enable debug
tracing", "enable warnings about odd guest behaviour", "be silent",
etc. But they're not, so they don't gain anything over a simple
DPRINTF for programmer debugging.

-- PMM

Re: [Qemu-devel] [PATCH 3/3] pseries: Add DPRINTF macros to spapr pci code

[Blue Swirl]

On Wed, Apr 4, 2012 at 20:11, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 April 2012 20:18, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Mon, Apr 2, 2012 at 04:17, David Gibson <david@gibson.dropbear.id.au> wrote:
>>> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> This adds DPRINTF() macros with the usual conventions to the spapr_pci
>>> code.
>>
>> Please use tracepoints instead of printf statements. Tracing is more
>> flexible, more efficient and does not suffer from bitrot.
>
> I'd much rather enable a #define to turn on debugging than faff about
> with tracing. It's simple and straightforward, you can do it with a
> single obvious change and recompile, and nobody has to look up
> documentation to figure out how it works.

Laziness. Even the built in help should be sufficient to start using
tracepoints.

> If tracepoints were always-compiled-in and enabled at runtime I'd
> agree with you: then you could have linux-kernel-style "enable debug
> tracing", "enable warnings about odd guest behaviour", "be silent",
> etc. But they're not, so they don't gain anything over a simple
> DPRINTF for programmer debugging.

False. It's easy to compile in tracepoints
(--enable-trace-backend=simple) and the overhead is zero or marginal.
It's not possible to enable or disable DPRINTFs at run time easily
which is trivial with tracepoints. When the tracepoints are enabled,
due to binary format they are faster than text output.  There's no
need to relink the files when triggering on and off tracepoints. Trace
files are much more compact than text. Processing is offline. Delta
timestamps are provided. Tracepoints do not suffer from bitrot. From
programming standpoint, DPRINTF is changed to a function call and the
format text is put to a different file, that's it.

Have you ever waded through hundreds of megabytes of DPRINTF stuff or
other logs, 99.99999% (literally, only a few bytes were interesting!)
of it not what you want, just because the interesting stuff happens to
be near to end of it? Even 'vi' is slow when handling these kind of
files.

Just for fun, try also enabling all DPRINTFs (small sed script
exercise left to reader) and see how many breakages there are due to
bit rotted DPRINTFs.

>
> -- PMM

Re: [Qemu-devel] [PATCH 3/3] pseries: Add DPRINTF macros to spapr pci code

[Peter Maydell]

On 4 April 2012 21:34, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Wed, Apr 4, 2012 at 20:11, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I'd much rather enable a #define to turn on debugging than faff about
>> with tracing. It's simple and straightforward, you can do it with a
>> single obvious change and recompile, and nobody has to look up
>> documentation to figure out how it works.
>
> Laziness. Even the built in help should be sufficient to start using
> tracepoints.

Well, I looked at the docs and said "this gives me no benefit over
DPRINTF, and why do I have to create a file that explicitly lists
every single event the code I'm interested in has defined, and
the quickstart seems to be recommending something that you have to
postprocess, and generally I dunno what problem this is trying to
solve but it doesn't look like it's trying to solve programmer debug
tracing".

If other people want to write trace events that's fine but for me
at the moment it seems a definite step back from simple DPRINTF
macros.

>> If tracepoints were always-compiled-in and enabled at runtime I'd
>> agree with you: then you could have linux-kernel-style "enable debug
>> tracing", "enable warnings about odd guest behaviour", "be silent",
>> etc. But they're not, so they don't gain anything over a simple
>> DPRINTF for programmer debugging.
>
> False. It's easy to compile in tracepoints
> (--enable-trace-backend=simple) and the overhead is zero or marginal.

(a) that gives you a binary dump rather than something actually
readable (and 'simple' can't even handle string output!) and
(b) if it's that good why don't we enable it by default?
Also I can't see anything in the docs about having sensible sets
of levels of tracing or grouping trace events into coherent sets
you can toggle on and off.

> Processing is offline.

For debugging this is not a feature -- you want to see the output
as you step through things in the debugger.

-- PMM

Re: [Qemu-devel] [PATCH 3/3] pseries: Add DPRINTF macros to spapr pci code

[Andreas Färber]

Am 04.04.2012 22:54, schrieb Peter Maydell:
> On 4 April 2012 21:34, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Wed, Apr 4, 2012 at 20:11, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> I'd much rather enable a #define to turn on debugging than faff about
>>> with tracing. It's simple and straightforward, you can do it with a
>>> single obvious change and recompile, and nobody has to look up
>>> documentation to figure out how it works.
>>
>> Laziness. Even the built in help should be sufficient to start using
>> tracepoints.
> 
> Well, I looked at the docs and said "this gives me no benefit over
> DPRINTF, and why do I have to create a file that explicitly lists
> every single event the code I'm interested in has defined, and
> the quickstart seems to be recommending something that you have to
> postprocess, and generally I dunno what problem this is trying to
> solve but it doesn't look like it's trying to solve programmer debug
> tracing".
> 
> If other people want to write trace events that's fine but for me
> at the moment it seems a definite step back from simple DPRINTF
> macros.
> 
>>> If tracepoints were always-compiled-in and enabled at runtime I'd
>>> agree with you: then you could have linux-kernel-style "enable debug
>>> tracing", "enable warnings about odd guest behaviour", "be silent",
>>> etc. But they're not, so they don't gain anything over a simple
>>> DPRINTF for programmer debugging.
>>
>> False. It's easy to compile in tracepoints
>> (--enable-trace-backend=simple) and the overhead is zero or marginal.
> 
> (a) that gives you a binary dump rather than something actually
> readable (and 'simple' can't even handle string output!) and
> (b) if it's that good why don't we enable it by default?
> Also I can't see anything in the docs about having sensible sets
> of levels of tracing or grouping trace events into coherent sets
> you can toggle on and off.
> 
>> Processing is offline.
> 
> For debugging this is not a feature -- you want to see the output
> as you step through things in the debugger.

Seeing this thread today, not being cc'ed properly and the list lagging
yesterday once again, we should not have this discussion without
involving the tracing maintainer, cc'ed.

I agree with Blue that using tracepoints is more flexible output-wise
and avoids bitrot. However trace-events is an annoying single point of
merge conflicts when adding trace points. Is it thinkable of allowing
more localized tracepoint definitions

I agree with Peter that tracing being nop by default is not so helpful
for developers. So what about enabling the stderr backend by default,
with all tracepoints disabled by default?

In addition to what's been said on this thread, the simple tracing
backend potentially collects a lot of garbage into files that are hard
to post-process in a changing development tree due to dependency on
trace-events file.

I would've suggested to enable the platform's native tracing backend by
default, but on Linux there might be SystemTap and LTTng both present.
At least DTrace and SystemTap scripts can also emit printfs when events
occur, for DPRINTF-like output; shipping some examples for specific
areas in scripts/ might not be a bad idea either.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg