[Qemu-devel] [PULL 0/6] target-arm queue

[Qemu-devel] [PULL 0/6] target-arm queue

[Peter Maydell]

The following changes since commit fc8ead74674b7129e8f31c2595c76658e5622197:

  Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2013-10-18 10:03:24 -0700)

are available in the git repository at:


  git://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20131025

for you to fetch changes up to 71c903cc3b78fc563122fe40c5cadd050068b91a:

  integrator: fix Linux boot failure by emulating dbg region (2013-10-25 18:27:07 +0100)

----------------------------------------------------------------
target-arm queue: a couple of trivial features to improve support
for some guest emulation cases, notably running UEFI images:
 * support VBAR (vector base address register)
 * allow running without specifying a kernel (ie just running
   an image from flash)
Plus some bugfixes.

----------------------------------------------------------------
Alex Bennée (1):
      integrator: fix Linux boot failure by emulating dbg region

Alvise Rigo (2):
      target-arm: sort TCG cpreg list by KVM-style 64 bit ID number
      target-arm: fix sorting issue of KVM cpreg list

Nathan Rossi (1):
      target-arm: Add CP15 VBAR support

Peter Maydell (2):
      hw/arm/boot: Make user not specifying a kernel not an error
      hw/arm: Tidy up conditional calls to arm_load_kernel

 default-configs/arm-softmmu.mak        |    1 +
 hw/arm/boot.c                          |    6 +-
 hw/arm/integratorcp.c                  |    2 +
 hw/arm/omap_sx1.c                      |   10 ++--
 hw/arm/palm.c                          |   10 ++--
 hw/arm/z2.c                            |   12 ++--
 hw/misc/Makefile.objs                  |    1 +
 hw/misc/arm_integrator_debug.c         |   99 ++++++++++++++++++++++++++++++++
 include/hw/misc/arm_integrator_debug.h |   18 ++++++
 target-arm/cpu.h                       |    1 +
 target-arm/helper.c                    |   33 ++++++++++-
 target-arm/kvm.c                       |    8 ++-
 12 files changed, 176 insertions(+), 25 deletions(-)
 create mode 100644 hw/misc/arm_integrator_debug.c
 create mode 100644 include/hw/misc/arm_integrator_debug.h

[Qemu-devel] [PULL 1/6] hw/arm/boot: Make user not specifying a kernel not an error

[Peter Maydell]

Typically ARM boards will have some kind of flash which might contain
a boot ROM; it's therefore a valid use case to provide only an
image for the boot ROM and not require QEMU's internal boot loader
at all. Remove the fatal error if -kernel isn't specified.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-id: 1379980897-21277-2-git-send-email-peter.maydell@linaro.org
---
 hw/arm/boot.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 1e313af..583ec79 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -354,8 +354,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 
     /* Load the kernel.  */
     if (!info->kernel_filename) {
-        fprintf(stderr, "Kernel image must be specified\n");
-        exit(1);
+        /* If no kernel specified, do nothing; we will start from address 0
+         * (typically a boot ROM image) in the same way as hardware.
+         */
+        return;
     }
 
     info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
-- 
1.7.9.5

[Qemu-devel] [PULL 2/6] hw/arm: Tidy up conditional calls to arm_load_kernel

[Peter Maydell]

Now that arm_load_kernel doesn't insist on a kernel filename
being present, we can remove some unnecessary conditionals
in board models.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-id: 1379980897-21277-3-git-send-email-peter.maydell@linaro.org
---
 hw/arm/omap_sx1.c |   10 ++++------
 hw/arm/palm.c     |   10 ++++------
 hw/arm/z2.c       |   12 +++++-------
 3 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/hw/arm/omap_sx1.c b/hw/arm/omap_sx1.c
index b0f8664..03b3816 100644
--- a/hw/arm/omap_sx1.c
+++ b/hw/arm/omap_sx1.c
@@ -194,12 +194,10 @@ static void sx1_init(QEMUMachineInitArgs *args, const int version)
     }
 
     /* Load the kernel.  */
-    if (args->kernel_filename) {
-        sx1_binfo.kernel_filename = args->kernel_filename;
-        sx1_binfo.kernel_cmdline = args->kernel_cmdline;
-        sx1_binfo.initrd_filename = args->initrd_filename;
-        arm_load_kernel(mpu->cpu, &sx1_binfo);
-    }
+    sx1_binfo.kernel_filename = args->kernel_filename;
+    sx1_binfo.kernel_cmdline = args->kernel_cmdline;
+    sx1_binfo.initrd_filename = args->initrd_filename;
+    arm_load_kernel(mpu->cpu, &sx1_binfo);
 
     /* TODO: fix next line */
     //~ qemu_console_resize(ds, 640, 480);
diff --git a/hw/arm/palm.c b/hw/arm/palm.c
index 3e39044..0b72bbe 100644
--- a/hw/arm/palm.c
+++ b/hw/arm/palm.c
@@ -261,12 +261,10 @@ static void palmte_init(QEMUMachineInitArgs *args)
     }
 
     /* Load the kernel.  */
-    if (kernel_filename) {
-        palmte_binfo.kernel_filename = kernel_filename;
-        palmte_binfo.kernel_cmdline = kernel_cmdline;
-        palmte_binfo.initrd_filename = initrd_filename;
-        arm_load_kernel(mpu->cpu, &palmte_binfo);
-    }
+    palmte_binfo.kernel_filename = kernel_filename;
+    palmte_binfo.kernel_cmdline = kernel_cmdline;
+    palmte_binfo.initrd_filename = initrd_filename;
+    arm_load_kernel(mpu->cpu, &palmte_binfo);
 }
 
 static QEMUMachine palmte_machine = {
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index 2e0d5d4..a00fcc0 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -360,13 +360,11 @@ static void z2_init(QEMUMachineInitArgs *args)
     qdev_connect_gpio_out(mpu->gpio, Z2_GPIO_LCD_CS,
         qemu_allocate_irqs(z2_lcd_cs, z2_lcd, 1)[0]);
 
-    if (kernel_filename) {
-        z2_binfo.kernel_filename = kernel_filename;
-        z2_binfo.kernel_cmdline = kernel_cmdline;
-        z2_binfo.initrd_filename = initrd_filename;
-        z2_binfo.board_id = 0x6dd;
-        arm_load_kernel(mpu->cpu, &z2_binfo);
-    }
+    z2_binfo.kernel_filename = kernel_filename;
+    z2_binfo.kernel_cmdline = kernel_cmdline;
+    z2_binfo.initrd_filename = initrd_filename;
+    z2_binfo.board_id = 0x6dd;
+    arm_load_kernel(mpu->cpu, &z2_binfo);
 }
 
 static QEMUMachine z2_machine = {
-- 
1.7.9.5

[Qemu-devel] [PULL 3/6] target-arm: Add CP15 VBAR support

[Peter Maydell]

From: Nathan Rossi <nathan.rossi@xilinx.com>

Added Vector Base Address remapping on ARM v7.

Signed-off-by: Nathan Rossi <nathan.rossi@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
[PMM: removed spurious mask of value with 1<<31]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h    |    1 +
 target-arm/helper.c |   21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 2c56740..9f110f1 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -176,6 +176,7 @@ typedef struct CPUARMState {
         uint32_t c9_pmxevtyper; /* perf monitor event type */
         uint32_t c9_pmuserenr; /* perf monitor user enable */
         uint32_t c9_pminten; /* perf monitor interrupt enables */
+        uint32_t c12_vbar; /* vector base address register */
         uint32_t c13_fcse; /* FCSE PID.  */
         uint32_t c13_context; /* Context ID.  */
         uint32_t c13_tls1; /* User RW Thread register.  */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index c63bbd7..73476ed 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -537,6 +537,13 @@ static int pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     return 0;
 }
 
+static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                      uint64_t value)
+{
+    env->cp15.c12_vbar = value & ~0x1Ful;
+    return 0;
+}
+
 static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t *value)
 {
@@ -622,6 +629,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .resetvalue = 0, .writefn = pmintenclr_write, },
+    { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
+      .access = PL1_RW, .writefn = vbar_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
+      .resetvalue = 0 },
     { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
       .resetvalue = 0, },
@@ -2470,7 +2481,17 @@ void arm_cpu_do_interrupt(CPUState *cs)
     }
     /* High vectors.  */
     if (env->cp15.c1_sys & (1 << 13)) {
+        /* when enabled, base address cannot be remapped.  */
         addr += 0xffff0000;
+    } else {
+        /* ARM v7 architectures provide a vector base address register to remap
+         * the interrupt vector table.
+         * This register is only followed in non-monitor mode, and has a secure
+         * and un-secure copy. Since the cpu is always in a un-secure operation
+         * and is never in monitor mode this feature is always active.
+         * Note: only bits 31:5 are valid.
+         */
+        addr += env->cp15.c12_vbar;
     }
     switch_mode (env, new_mode);
     env->spsr = cpsr_read(env);
-- 
1.7.9.5

[Qemu-devel] [PULL 4/6] target-arm: sort TCG cpreg list by KVM-style 64 bit ID number

[Peter Maydell]

From: Alvise Rigo <a.rigo@virtualopensystems.com>

Both KVM and TCG populate the cpreg_list with 64 bit register IDs,
but in the TCG side the cpreg_list is sorted using the 32 bit ID
version while in the kvm side the 64 bit ID version is used.  This
patch makes the sorting of the cpreg_list consistent between KVM and
TCG.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
Message-id: 1381513125-26802-1-git-send-email-a.rigo@virtualopensystems.com
[PMM: fixed indent, coding style and commit message formatting]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 73476ed..3445813 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -225,10 +225,16 @@ static void count_cpreg(gpointer key, gpointer opaque)
 
 static gint cpreg_key_compare(gconstpointer a, gconstpointer b)
 {
-    uint32_t aidx = *(uint32_t *)a;
-    uint32_t bidx = *(uint32_t *)b;
+    uint64_t aidx = cpreg_to_kvm_id(*(uint32_t *)a);
+    uint64_t bidx = cpreg_to_kvm_id(*(uint32_t *)b);
 
-    return aidx - bidx;
+    if (aidx > bidx) {
+        return 1;
+    }
+    if (aidx < bidx) {
+        return -1;
+    }
+    return 0;
 }
 
 static void cpreg_make_keylist(gpointer key, gpointer value, gpointer udata)
-- 
1.7.9.5

[Qemu-devel] [PULL 5/6] target-arm: fix sorting issue of KVM cpreg list

[Peter Maydell]

From: Alvise Rigo <a.rigo@virtualopensystems.com>

The compare_u64 function was not sorting the KVM cpreg_list in the
right way due to the wrong returned value.  Since we are comparing
two 64bit values we can't simply return their difference if the
returned type is int.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
Message-id: 1381513125-26802-2-git-send-email-a.rigo@virtualopensystems.com
[PMM: fixed coding style, indent and commit message formatting]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/kvm.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index b92e00d..6e5cd36 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -67,7 +67,13 @@ static bool reg_syncs_via_tuple_list(uint64_t regidx)
 
 static int compare_u64(const void *a, const void *b)
 {
-    return *(uint64_t *)a - *(uint64_t *)b;
+    if (*(uint64_t *)a > *(uint64_t *)b) {
+        return 1;
+    }
+    if (*(uint64_t *)a < *(uint64_t *)b) {
+        return -1;
+    }
+    return 0;
 }
 
 int kvm_arch_init_vcpu(CPUState *cs)
-- 
1.7.9.5

[Qemu-devel] [PULL 6/6] integrator: fix Linux boot failure by emulating dbg region

[Peter Maydell]

From: Alex Bennée <alex@bennee.com>

Commit 9b8c69243 (since reverted) broke the ability to boot the kernel
as the value returned by unassigned_mem_read returned non-zero and left
the kernel looping forever waiting for it to change (see
integrator_led_set in the kernel code).

Relying on a varying implementation detail is incorrect anyway so this
introduces a basic stub of a memory region for the debug/LED section
on the integrator board.

Signed-off-by: Alex Bennée <alex@bennee.com>
Message-id: 1382451366-9539-1-git-send-email-alex.bennee@linaro.org
[PMM: removed three unused fields from struct IntegratorDebugState]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 default-configs/arm-softmmu.mak        |    1 +
 hw/arm/integratorcp.c                  |    2 +
 hw/misc/Makefile.objs                  |    1 +
 hw/misc/arm_integrator_debug.c         |   99 ++++++++++++++++++++++++++++++++
 include/hw/misc/arm_integrator_debug.h |   18 ++++++
 5 files changed, 121 insertions(+)
 create mode 100644 hw/misc/arm_integrator_debug.c
 create mode 100644 include/hw/misc/arm_integrator_debug.h

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index d13bc2b..7e69137 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -79,3 +79,4 @@ CONFIG_VERSATILE_PCI=y
 CONFIG_VERSATILE_I2C=y
 
 CONFIG_SDHCI=y
+CONFIG_INTEGRATOR_DEBUG=y
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 2ef93ed..c44b2a4 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -11,6 +11,7 @@
 #include "hw/devices.h"
 #include "hw/boards.h"
 #include "hw/arm/arm.h"
+#include "hw/misc/arm_integrator_debug.h"
 #include "net/net.h"
 #include "exec/address-spaces.h"
 #include "sysemu/sysemu.h"
@@ -508,6 +509,7 @@ static void integratorcp_init(QEMUMachineInitArgs *args)
     icp_control_init(0xcb000000);
     sysbus_create_simple("pl050_keyboard", 0x18000000, pic[3]);
     sysbus_create_simple("pl050_mouse", 0x19000000, pic[4]);
+    sysbus_create_simple(TYPE_INTEGRATOR_DEBUG, 0x1a000000, 0);
     sysbus_create_varargs("pl181", 0x1c000000, pic[23], pic[24], NULL);
     if (nd_table[0].used)
         smc91c111_init(&nd_table[0], 0xc8000000, pic[27]);
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 2578e29..cca5c05 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -10,6 +10,7 @@ obj-$(CONFIG_VMPORT) += vmport.o
 
 # ARM devices
 common-obj-$(CONFIG_PL310) += arm_l2x0.o
+common-obj-$(CONFIG_INTEGRATOR_DEBUG) += arm_integrator_debug.o
 
 # PKUnity SoC devices
 common-obj-$(CONFIG_PUV3) += puv3_pm.o
diff --git a/hw/misc/arm_integrator_debug.c b/hw/misc/arm_integrator_debug.c
new file mode 100644
index 0000000..99b720f
--- /dev/null
+++ b/hw/misc/arm_integrator_debug.c
@@ -0,0 +1,99 @@
+/*
+ * LED, Switch and Debug control registers for ARM Integrator Boards
+ *
+ * This is currently a stub for this functionality but at least
+ * ensures something other than unassigned_mem_read() handles access
+ * to this area.
+ *
+ * The real h/w is described at:
+ *  http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html
+ *
+ * Copyright (c) 2013 Alex Bennée <alex@bennee.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "hw/hw.h"
+#include "hw/sysbus.h"
+#include "exec/address-spaces.h"
+#include "hw/misc/arm_integrator_debug.h"
+
+#define INTEGRATOR_DEBUG(obj) \
+    OBJECT_CHECK(IntegratorDebugState, (obj), TYPE_INTEGRATOR_DEBUG)
+
+typedef struct {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+} IntegratorDebugState;
+
+static uint64_t intdbg_control_read(void *opaque, hwaddr offset,
+                                    unsigned size)
+{
+    switch (offset >> 2) {
+    case 0: /* ALPHA */
+    case 1: /* LEDS */
+    case 2: /* SWITCHES */
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: returning zero from %" HWADDR_PRIx ":%u\n",
+                      __func__, offset, size);
+        return 0;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Bad offset %" HWADDR_PRIx,
+                      __func__, offset);
+        return 0;
+    }
+}
+
+static void intdbg_control_write(void *opaque, hwaddr offset,
+                                 uint64_t value, unsigned size)
+{
+    switch (offset >> 2) {
+    case 1: /* ALPHA */
+    case 2: /* LEDS */
+    case 3: /* SWITCHES */
+        /* Nothing interesting implemented yet.  */
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: ignoring write of %" PRIu64
+                      " to %" HWADDR_PRIx ":%u\n",
+                      __func__, value, offset, size);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: write of %" PRIu64
+                      " to bad offset %" HWADDR_PRIx "\n",
+                      __func__, value, offset);
+    }
+}
+
+static const MemoryRegionOps intdbg_control_ops = {
+    .read = intdbg_control_read,
+    .write = intdbg_control_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void intdbg_control_init(Object *obj)
+{
+    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+    IntegratorDebugState *s = INTEGRATOR_DEBUG(obj);
+
+    memory_region_init_io(&s->iomem, NULL, &intdbg_control_ops,
+                          NULL, "dbg-leds", 0x1000000);
+    sysbus_init_mmio(sd, &s->iomem);
+}
+
+static const TypeInfo intdbg_info = {
+    .name          = TYPE_INTEGRATOR_DEBUG,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(IntegratorDebugState),
+    .instance_init = intdbg_control_init,
+};
+
+static void intdbg_register_types(void)
+{
+    type_register_static(&intdbg_info);
+}
+
+type_init(intdbg_register_types)
diff --git a/include/hw/misc/arm_integrator_debug.h b/include/hw/misc/arm_integrator_debug.h
new file mode 100644
index 0000000..37789b6
--- /dev/null
+++ b/include/hw/misc/arm_integrator_debug.h
@@ -0,0 +1,18 @@
+/*
+ * ARM Integrator Board Debug, switch and LED section
+ *
+ * Browse the data sheet:
+ *
+ *    http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html
+ *
+ * Copyright (c) 2013 Alex Bennée <alex@bennee.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef QEMU_INTEGRATOR_DEBUG_H
+#define QEMU_INTEGRATOR_DEBUG_H
+
+#define TYPE_INTEGRATOR_DEBUG "integrator_debug"
+
+#endif
-- 
1.7.9.5

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Edgar E. Iglesias]

On Fri, Oct 25, 2013 at 07:07:23PM +0100, Peter Maydell wrote:
> The following changes since commit fc8ead74674b7129e8f31c2595c76658e5622197:
> 
>   Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2013-10-18 10:03:24 -0700)
> 
> are available in the git repository at:
> 
> 
>   git://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20131025
> 
> for you to fetch changes up to 71c903cc3b78fc563122fe40c5cadd050068b91a:
> 
>   integrator: fix Linux boot failure by emulating dbg region (2013-10-25 18:27:07 +0100)


Applied, thanks all.

Cheers,
Edgar


> 
> ----------------------------------------------------------------
> target-arm queue: a couple of trivial features to improve support
> for some guest emulation cases, notably running UEFI images:
>  * support VBAR (vector base address register)
>  * allow running without specifying a kernel (ie just running
>    an image from flash)
> Plus some bugfixes.
> 
> ----------------------------------------------------------------
> Alex Bennée (1):
>       integrator: fix Linux boot failure by emulating dbg region
> 
> Alvise Rigo (2):
>       target-arm: sort TCG cpreg list by KVM-style 64 bit ID number
>       target-arm: fix sorting issue of KVM cpreg list
> 
> Nathan Rossi (1):
>       target-arm: Add CP15 VBAR support
> 
> Peter Maydell (2):
>       hw/arm/boot: Make user not specifying a kernel not an error
>       hw/arm: Tidy up conditional calls to arm_load_kernel
> 
>  default-configs/arm-softmmu.mak        |    1 +
>  hw/arm/boot.c                          |    6 +-
>  hw/arm/integratorcp.c                  |    2 +
>  hw/arm/omap_sx1.c                      |   10 ++--
>  hw/arm/palm.c                          |   10 ++--
>  hw/arm/z2.c                            |   12 ++--
>  hw/misc/Makefile.objs                  |    1 +
>  hw/misc/arm_integrator_debug.c         |   99 ++++++++++++++++++++++++++++++++
>  include/hw/misc/arm_integrator_debug.h |   18 ++++++
>  target-arm/cpu.h                       |    1 +
>  target-arm/helper.c                    |   33 ++++++++++-
>  target-arm/kvm.c                       |    8 ++-
>  12 files changed, 176 insertions(+), 25 deletions(-)
>  create mode 100644 hw/misc/arm_integrator_debug.c
>  create mode 100644 include/hw/misc/arm_integrator_debug.h
> 

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Andreas Färber]

Hi,

Am 31.10.2013 15:02, schrieb Edgar E. Iglesias:
> On Fri, Oct 25, 2013 at 07:07:23PM +0100, Peter Maydell wrote:
>> The following changes since commit fc8ead74674b7129e8f31c2595c76658e5622197:
>>
>>   Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2013-10-18 10:03:24 -0700)
>>
>> are available in the git repository at:
>>
>>
>>   git://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20131025
>>
>> for you to fetch changes up to 71c903cc3b78fc563122fe40c5cadd050068b91a:
>>
>>   integrator: fix Linux boot failure by emulating dbg region (2013-10-25 18:27:07 +0100)
> 
> 
> Applied, thanks all.

Edgar, there is no merge commit in qemu.git despite this being a signed
pull. Do you maybe need to upgrade your version of git?

Peter, since I had picked up the first two patches into my still pending
qom-next pull, as per the QEMU Summit discussion those patches should've
gotten an Acked-by.

Regards,
Andreas

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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Anthony Liguori]

On Thu, Oct 31, 2013 at 3:18 PM, Andreas Färber <afaerber@suse.de> wrote:
> Hi,
>
> Am 31.10.2013 15:02, schrieb Edgar E. Iglesias:
>> On Fri, Oct 25, 2013 at 07:07:23PM +0100, Peter Maydell wrote:
>>> The following changes since commit fc8ead74674b7129e8f31c2595c76658e5622197:
>>>
>>>   Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2013-10-18 10:03:24 -0700)
>>>
>>> are available in the git repository at:
>>>
>>>
>>>   git://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20131025
>>>
>>> for you to fetch changes up to 71c903cc3b78fc563122fe40c5cadd050068b91a:
>>>
>>>   integrator: fix Linux boot failure by emulating dbg region (2013-10-25 18:27:07 +0100)
>>
>>
>> Applied, thanks all.
>
> Edgar, there is no merge commit in qemu.git despite this being a signed
> pull. Do you maybe need to upgrade your version of git?

Need to add:

[merge]
 ff = false

To your git config to prevent fast forwards on merging.

Regards,

Anthony Liguori

> Peter, since I had picked up the first two patches into my still pending
> qom-next pull, as per the QEMU Summit discussion those patches should've
> gotten an Acked-by.
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Peter Maydell]

On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
> Peter, since I had picked up the first two patches into my still pending
> qom-next pull, as per the QEMU Summit discussion those patches should've
> gotten an Acked-by.

Hmm? I don't recall this part of the discussion. If you want the
patches to have an Acked-by from you you need to send mail
to the list with an Acked-by line.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Andreas Färber]

Am 31.10.2013 15:31, schrieb Peter Maydell:
> On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
>> Peter, since I had picked up the first two patches into my still pending
>> qom-next pull, as per the QEMU Summit discussion those patches should've
>> gotten an Acked-by.
> 
> Hmm? I don't recall this part of the discussion. If you want the
> patches to have an Acked-by from you you need to send mail
> to the list with an Acked-by line.

No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
needs to be explicitly sent as reply but that "looks okay" should in
exactly such a case where sender=submaintainer should be recorded as
Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.

Andreas

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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Anthony Liguori]

On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 31.10.2013 15:31, schrieb Peter Maydell:
>> On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
>>> Peter, since I had picked up the first two patches into my still pending
>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>> gotten an Acked-by.
>>
>> Hmm? I don't recall this part of the discussion. If you want the
>> patches to have an Acked-by from you you need to send mail
>> to the list with an Acked-by line.
>
> No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
> needs to be explicitly sent as reply but that "looks okay" should in
> exactly such a case where sender=submaintainer should be recorded as
> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.

Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
make people infer your Acked-bys.

And adding tags is a nice-to-have.  There is no "rule" stating that
you must include everyone that appears on the mailing list.  But I
expect that maintainers try to

Regards,

Anthony Liguori

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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Andreas Färber]

Am 31.10.2013 15:39, schrieb Anthony Liguori:
> On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 31.10.2013 15:31, schrieb Peter Maydell:
>>> On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
>>>> Peter, since I had picked up the first two patches into my still pending
>>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>>> gotten an Acked-by.
>>>
>>> Hmm? I don't recall this part of the discussion. If you want the
>>> patches to have an Acked-by from you you need to send mail
>>> to the list with an Acked-by line.
>>
>> No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
>> needs to be explicitly sent as reply but that "looks okay" should in
>> exactly such a case where sender=submaintainer should be recorded as
>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
> 
> Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
> make people infer your Acked-bys.

Yes, that's in the minutes. And yes, that's what I got as answer there.
Please reply to the minutes if you think otherwise.

I brought up exactly this situation where I am contributor to CPU and
submaintainer of CPU and often not getting Reviewed-bys but if at all,
such as from Paolo recently, some verbal "looks OK" for a series. I was
told that that should be turned into an Acked-by on the patches to
satisfy your criteria that contributors may not just send patches as
pull without Reviewed-by.

> And adding tags is a nice-to-have.  There is no "rule" stating that
> you must include everyone that appears on the mailing list.  But I
> expect that maintainers try to

Again, at QEMU Summit you pushed for making Reviewed-by a must-have and
we discussed whether a submaintainer must add a Reviewed-by then and
what to do if author==submaintainer. If you dropped that thought, then
fine with me.

Regards,
Andreas

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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Anthony Liguori]

On Thu, Oct 31, 2013 at 3:45 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 31.10.2013 15:39, schrieb Anthony Liguori:
>> On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 31.10.2013 15:31, schrieb Peter Maydell:
>>>> On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
>>>>> Peter, since I had picked up the first two patches into my still pending
>>>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>>>> gotten an Acked-by.
>>>>
>>>> Hmm? I don't recall this part of the discussion. If you want the
>>>> patches to have an Acked-by from you you need to send mail
>>>> to the list with an Acked-by line.
>>>
>>> No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
>>> needs to be explicitly sent as reply but that "looks okay" should in
>>> exactly such a case where sender=submaintainer should be recorded as
>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>
>> Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
>> make people infer your Acked-bys.
>
> Yes, that's in the minutes. And yes, that's what I got as answer there.
> Please reply to the minutes if you think otherwise.

I

> I brought up exactly this situation where I am contributor to CPU and
> submaintainer of CPU and often not getting Reviewed-bys but if at all,
> such as from Paolo recently, some verbal "looks OK" for a series. I was
> told that that should be turned into an Acked-by on the patches to
> satisfy your criteria that contributors may not just send patches as
> pull without Reviewed-by.
>
>> And adding tags is a nice-to-have.  There is no "rule" stating that
>> you must include everyone that appears on the mailing list.  But I
>> expect that maintainers try to
>
> Again, at QEMU Summit you pushed for making Reviewed-by a must-have and
> we discussed whether a submaintainer must add a Reviewed-by then and
> what to do if author==submaintainer. If you dropped that thought, then
> fine with me.
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Anthony Liguori]

On Thu, Oct 31, 2013 at 3:45 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 31.10.2013 15:39, schrieb Anthony Liguori:
>> On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 31.10.2013 15:31, schrieb Peter Maydell:
>>>> On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
>>>>> Peter, since I had picked up the first two patches into my still pending
>>>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>>>> gotten an Acked-by.
>>>>
>>>> Hmm? I don't recall this part of the discussion. If you want the
>>>> patches to have an Acked-by from you you need to send mail
>>>> to the list with an Acked-by line.
>>>
>>> No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
>>> needs to be explicitly sent as reply but that "looks okay" should in
>>> exactly such a case where sender=submaintainer should be recorded as
>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>
>> Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
>> make people infer your Acked-bys.
>
> Yes, that's in the minutes. And yes, that's what I got as answer there.
> Please reply to the minutes if you think otherwise.

I explicitly said that Acked-bys are useless too.

The minutes say that you said the kernel treats "Acked-bys" as "looks
good".  You did say that.  At no point did a "rule" get made though.

> I brought up exactly this situation where I am contributor to CPU and
> submaintainer of CPU and often not getting Reviewed-bys but if at all,
> such as from Paolo recently, some verbal "looks OK" for a series. I was
> told that that should be turned into an Acked-by on the patches to
> satisfy your criteria that contributors may not just send patches as
> pull without Reviewed-by.

I think you misunderstood.

I don't care about Acked-bys.  They are useless.

A third of patches are being committed with Reviewed-bys.  There are
certainly many cases where patches are going in from submaintainers
that have been reviewed which comes implicitly with Signed-off-by.

But I worry that we're not reviewing enough on list and that there are
patches from maintainers going in through maintainer trees that aren't
getting outside review.

There's no immediate action for this other than we should all try to
review more patches on list to prevent the above situation.

>> And adding tags is a nice-to-have.  There is no "rule" stating that
>> you must include everyone that appears on the mailing list.  But I
>> expect that maintainers try to
>
> Again, at QEMU Summit you pushed for making Reviewed-by a must-have and
> we discussed whether a submaintainer must add a Reviewed-by then and
> what to do if author==submaintainer. If you dropped that thought, then
> fine with me.

Yes, patches should get reviewed.  I hope this is obvious to all of us :-)

I also suggested that I have tooling that people can use to simplify
adding collected Reviewed-bys on the list.

But none of this has anything to do with inferred Acked-bys.  I'll go
a step further and say that I would be very unhappy if anyone every
added any kind of tag to a patch with my name on it that I didn't send
myself.

Regards,

Anthony Liguori

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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Andreas Färber]

Am 31.10.2013 16:04, schrieb Anthony Liguori:
> On Thu, Oct 31, 2013 at 3:45 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 31.10.2013 15:39, schrieb Anthony Liguori:
>>> On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber <afaerber@suse.de> wrote:
>>>> Am 31.10.2013 15:31, schrieb Peter Maydell:
>>>>> On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
>>>>>> Peter, since I had picked up the first two patches into my still pending
>>>>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>>>>> gotten an Acked-by.
>>>>>
>>>>> Hmm? I don't recall this part of the discussion. If you want the
>>>>> patches to have an Acked-by from you you need to send mail
>>>>> to the list with an Acked-by line.
>>>>
>>>> No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
>>>> needs to be explicitly sent as reply but that "looks okay" should in
>>>> exactly such a case where sender=submaintainer should be recorded as
>>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>>
>>> Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
>>> make people infer your Acked-bys.
>>
>> Yes, that's in the minutes. And yes, that's what I got as answer there.
>> Please reply to the minutes if you think otherwise.
> 
> I explicitly said that Acked-bys are useless too.
> 
> The minutes say that you said the kernel treats "Acked-bys" as "looks
> good".  You did say that.

I *asked* about what to do with my QEMU CPU patches that only get a
"looks okay" and got only positive answers for whether that should be an
Acked-by and no objection, including none from you.
I certainly said nothing at all about the kernel.

>  At no point did a "rule" get made though.

The new rule you made was: no patch without Reviewed-by.
Peter sending that PULL and Edgar merging it both violate that rule.
No objection against a particular patch function-wise.

Point is, had Peter ping'ed me before sending out that pull, he would've
actually gotten a Reviewed-by from me, thereby satisfying your rule! He
didn't, ignoring that he himself had actually told me to queue the
patches before his vacation, for which obviously I reviewed and tested them.

Maybe there's no obligation for picking up tags, but then again you
can't go ahead and do statistics over incompletely recorded tags.

Regards,
Andreas

>> I brought up exactly this situation where I am contributor to CPU and
>> submaintainer of CPU and often not getting Reviewed-bys but if at all,
>> such as from Paolo recently, some verbal "looks OK" for a series. I was
>> told that that should be turned into an Acked-by on the patches to
>> satisfy your criteria that contributors may not just send patches as
>> pull without Reviewed-by.
> 
> I think you misunderstood.
> 
> I don't care about Acked-bys.  They are useless.
> 
> A third of patches are being committed with Reviewed-bys.  There are
> certainly many cases where patches are going in from submaintainers
> that have been reviewed which comes implicitly with Signed-off-by.
> 
> But I worry that we're not reviewing enough on list and that there are
> patches from maintainers going in through maintainer trees that aren't
> getting outside review.
> 
> There's no immediate action for this other than we should all try to
> review more patches on list to prevent the above situation.
> 
>>> And adding tags is a nice-to-have.  There is no "rule" stating that
>>> you must include everyone that appears on the mailing list.  But I
>>> expect that maintainers try to
>>
>> Again, at QEMU Summit you pushed for making Reviewed-by a must-have and
>> we discussed whether a submaintainer must add a Reviewed-by then and
>> what to do if author==submaintainer. If you dropped that thought, then
>> fine with me.
> 
> Yes, patches should get reviewed.  I hope this is obvious to all of us :-)
> 
> I also suggested that I have tooling that people can use to simplify
> adding collected Reviewed-bys on the list.
> 
> But none of this has anything to do with inferred Acked-bys.  I'll go
> a step further and say that I would be very unhappy if anyone every
> added any kind of tag to a patch with my name on it that I didn't send
> myself.
> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> Regards,
>> Andreas
>>
>> --
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Anthony Liguori]

On Thu, Oct 31, 2013 at 5:52 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 31.10.2013 16:04, schrieb Anthony Liguori:
>> On Thu, Oct 31, 2013 at 3:45 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 31.10.2013 15:39, schrieb Anthony Liguori:
>>>> On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber <afaerber@suse.de> wrote:
>>>>> Am 31.10.2013 15:31, schrieb Peter Maydell:
>>>>>> On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
>>>>>>> Peter, since I had picked up the first two patches into my still pending
>>>>>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>>>>>> gotten an Acked-by.
>>>>>>
>>>>>> Hmm? I don't recall this part of the discussion. If you want the
>>>>>> patches to have an Acked-by from you you need to send mail
>>>>>> to the list with an Acked-by line.
>>>>>
>>>>> No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
>>>>> needs to be explicitly sent as reply but that "looks okay" should in
>>>>> exactly such a case where sender=submaintainer should be recorded as
>>>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>>>
>>>> Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
>>>> make people infer your Acked-bys.
>>>
>>> Yes, that's in the minutes. And yes, that's what I got as answer there.
>>> Please reply to the minutes if you think otherwise.
>>
>> I explicitly said that Acked-bys are useless too.
>>
>> The minutes say that you said the kernel treats "Acked-bys" as "looks
>> good".  You did say that.
>
> I *asked* about what to do with my QEMU CPU patches that only get a
> "looks okay" and got only positive answers for whether that should be an
> Acked-by and no objection, including none from you.
> I certainly said nothing at all about the kernel.
>
>>  At no point did a "rule" get made though.
>
> The new rule you made was: no patch without Reviewed-by.
> Peter sending that PULL and Edgar merging it both violate that rule.

I never said anything like that.

Regards,

Anthony Liguori

> No objection against a particular patch function-wise.
>
> Point is, had Peter ping'ed me before sending out that pull, he would've
> actually gotten a Reviewed-by from me, thereby satisfying your rule! He
> didn't, ignoring that he himself had actually told me to queue the
> patches before his vacation, for which obviously I reviewed and tested them.
>
> Maybe there's no obligation for picking up tags, but then again you
> can't go ahead and do statistics over incompletely recorded tags.
>
> Regards,
> Andreas
>
>>> I brought up exactly this situation where I am contributor to CPU and
>>> submaintainer of CPU and often not getting Reviewed-bys but if at all,
>>> such as from Paolo recently, some verbal "looks OK" for a series. I was
>>> told that that should be turned into an Acked-by on the patches to
>>> satisfy your criteria that contributors may not just send patches as
>>> pull without Reviewed-by.
>>
>> I think you misunderstood.
>>
>> I don't care about Acked-bys.  They are useless.
>>
>> A third of patches are being committed with Reviewed-bys.  There are
>> certainly many cases where patches are going in from submaintainers
>> that have been reviewed which comes implicitly with Signed-off-by.
>>
>> But I worry that we're not reviewing enough on list and that there are
>> patches from maintainers going in through maintainer trees that aren't
>> getting outside review.
>>
>> There's no immediate action for this other than we should all try to
>> review more patches on list to prevent the above situation.
>>
>>>> And adding tags is a nice-to-have.  There is no "rule" stating that
>>>> you must include everyone that appears on the mailing list.  But I
>>>> expect that maintainers try to
>>>
>>> Again, at QEMU Summit you pushed for making Reviewed-by a must-have and
>>> we discussed whether a submaintainer must add a Reviewed-by then and
>>> what to do if author==submaintainer. If you dropped that thought, then
>>> fine with me.
>>
>> Yes, patches should get reviewed.  I hope this is obvious to all of us :-)
>>
>> I also suggested that I have tooling that people can use to simplify
>> adding collected Reviewed-bys on the list.
>>
>> But none of this has anything to do with inferred Acked-bys.  I'll go
>> a step further and say that I would be very unhappy if anyone every
>> added any kind of tag to a patch with my name on it that I didn't send
>> myself.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>
>>> Regards,
>>> Andreas
>>>
>>> --
>>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Andreas Färber]

Am 31.10.2013 17:54, schrieb Anthony Liguori:
> On Thu, Oct 31, 2013 at 5:52 PM, Andreas Färber <afaerber@suse.de> wrote:
>> The new rule you made was: no patch without Reviewed-by.
>> Peter sending that PULL and Edgar merging it both violate that rule.
> 
> I never said anything like that.

I could've sworn you did and that prompted Peter(?) to ask whether
submaintainers taking a patch from someone else should add a
Reviewed-by, too...

Then this whole discussion is moot and we just need to fix the minutes:

http://www.mail-archive.com/qemu-devel@nongnu.org/msg199693.html

Regards,
Andreas

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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Peter Maydell]

On 31 October 2013 16:52, Andreas Färber <afaerber@suse.de> wrote:
> I *asked* about what to do with my QEMU CPU patches that only get a
> "looks okay" and got only positive answers for whether that should be an
> Acked-by and no objection, including none from you.

I agreed with that because IMHO you may treat a "looks ok" from
a relevant subsystem maintainer like an acked-by. There is no
*obligation* to do so -- it's merely that if you think it's worth
noting and it will help get your patches upstream you can.

> Point is, had Peter ping'ed me before sending out that pull, he would've
> actually gotten a Reviewed-by from me, thereby satisfying your rule! He
> didn't, ignoring that he himself had actually told me to queue the
> patches before his vacation, for which obviously I reviewed and tested them.

I told you to queue the patches because you needed them as prereqs
and I was expecting the timing to work out such that you'd get a pullreq
taken so they'd get upstream while I was away.
Since it didn't and I wanted them in 1.7 I put them in my pullreq (which
is technically the better place for them since they're ARM patches, not
QOM ones). I don't see this as a big deal.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Peter Maydell]

On 31 October 2013 17:02, Peter Maydell <peter.maydell@linaro.org> wrote:
> I told you to queue the patches because you needed them as prereqs
> and I was expecting the timing to work out such that you'd get a pullreq
> taken so they'd get upstream while I was away.
> Since it didn't and I wanted them in 1.7 I put them in my pullreq (which
> is technically the better place for them since they're ARM patches, not
> QOM ones). I don't see this as a big deal.

...also, to be honest, by the time I got back from holiday I'd pretty
much forgotten about this and they were just another set of patches
in my list of "this should go in and isn't in upstream yet". Sorry for
any confusion.

-- PMM

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Anthony Liguori]

On Thu, Oct 31, 2013 at 5:52 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 31.10.2013 16:04, schrieb Anthony Liguori:
>> On Thu, Oct 31, 2013 at 3:45 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 31.10.2013 15:39, schrieb Anthony Liguori:
>>>> On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber <afaerber@suse.de> wrote:
>>>>> Am 31.10.2013 15:31, schrieb Peter Maydell:
>>>>>> On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
>>>>>>> Peter, since I had picked up the first two patches into my still pending
>>>>>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>>>>>> gotten an Acked-by.
>>>>>>
>>>>>> Hmm? I don't recall this part of the discussion. If you want the
>>>>>> patches to have an Acked-by from you you need to send mail
>>>>>> to the list with an Acked-by line.
>>>>>
>>>>> No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
>>>>> needs to be explicitly sent as reply but that "looks okay" should in
>>>>> exactly such a case where sender=submaintainer should be recorded as
>>>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>>>
>>>> Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
>>>> make people infer your Acked-bys.
>>>
>>> Yes, that's in the minutes. And yes, that's what I got as answer there.
>>> Please reply to the minutes if you think otherwise.
>>
>> I explicitly said that Acked-bys are useless too.
>>
>> The minutes say that you said the kernel treats "Acked-bys" as "looks
>> good".  You did say that.
>
> I *asked* about what to do with my QEMU CPU patches that only get a
> "looks okay" and got only positive answers for whether that should be an
> Acked-by and no objection, including none from you.
> I certainly said nothing at all about the kernel.
>
>>  At no point did a "rule" get made though.
>
> The new rule you made was: no patch without Reviewed-by.

Andreas, I have no idea where you're getting this from.  I think you
misunderstood what was discussed at the QEMU Summit.  Again, there are
no new rules.  I spoke about encouraging more reviews on list because
it's something we need to focus on as a community.

I think you need to step back a bit and give folks the benefit of the
doubt.  No one is doing anything malicious here.

Regards,

Anthony Liguori

> Peter sending that PULL and Edgar merging it both violate that rule.
> No objection against a particular patch function-wise.
>
> Point is, had Peter ping'ed me before sending out that pull, he would've
> actually gotten a Reviewed-by from me, thereby satisfying your rule! He
> didn't, ignoring that he himself had actually told me to queue the
> patches before his vacation, for which obviously I reviewed and tested them.
>
> Maybe there's no obligation for picking up tags, but then again you
> can't go ahead and do statistics over incompletely recorded tags.
>
> Regards,
> Andreas
>
>>> I brought up exactly this situation where I am contributor to CPU and
>>> submaintainer of CPU and often not getting Reviewed-bys but if at all,
>>> such as from Paolo recently, some verbal "looks OK" for a series. I was
>>> told that that should be turned into an Acked-by on the patches to
>>> satisfy your criteria that contributors may not just send patches as
>>> pull without Reviewed-by.
>>
>> I think you misunderstood.
>>
>> I don't care about Acked-bys.  They are useless.
>>
>> A third of patches are being committed with Reviewed-bys.  There are
>> certainly many cases where patches are going in from submaintainers
>> that have been reviewed which comes implicitly with Signed-off-by.
>>
>> But I worry that we're not reviewing enough on list and that there are
>> patches from maintainers going in through maintainer trees that aren't
>> getting outside review.
>>
>> There's no immediate action for this other than we should all try to
>> review more patches on list to prevent the above situation.
>>
>>>> And adding tags is a nice-to-have.  There is no "rule" stating that
>>>> you must include everyone that appears on the mailing list.  But I
>>>> expect that maintainers try to
>>>
>>> Again, at QEMU Summit you pushed for making Reviewed-by a must-have and
>>> we discussed whether a submaintainer must add a Reviewed-by then and
>>> what to do if author==submaintainer. If you dropped that thought, then
>>> fine with me.
>>
>> Yes, patches should get reviewed.  I hope this is obvious to all of us :-)
>>
>> I also suggested that I have tooling that people can use to simplify
>> adding collected Reviewed-bys on the list.
>>
>> But none of this has anything to do with inferred Acked-bys.  I'll go
>> a step further and say that I would be very unhappy if anyone every
>> added any kind of tag to a patch with my name on it that I didn't send
>> myself.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>
>>> Regards,
>>> Andreas
>>>
>>> --
>>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Peter Maydell]

On 31 October 2013 14:36, Andreas Färber <afaerber@suse.de> wrote:
> Am 31.10.2013 15:31, schrieb Peter Maydell:
>> On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
>>> Peter, since I had picked up the first two patches into my still pending
>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>> gotten an Acked-by.
>>
>> Hmm? I don't recall this part of the discussion. If you want the
>> patches to have an Acked-by from you you need to send mail
>> to the list with an Acked-by line.
>
> No, I added a Signed-off-by.

I checked my mail and the only thing I can find in reply to those
patches is a note from you saying you added them to your queue.

> It was clearly stated that a Reviewed-by
> needs to be explicitly sent as reply but that "looks okay" should in
> exactly such a case where sender=submaintainer should be recorded as
> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.

...but you're not the submaintainer here so I don't think this applies.

The point about the kernel practice as I understood it was that
the kernel folks treat acked-by at about the same level of review as
"looks ok to me" (ie, very little), not that there's some obligation to
treat any informal 'looks ok' note as an acked-by. I'm in full agreement
with Anthony that if you want a tag to appear you should send it
properly.

-- PMM

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Andreas Färber]

Am 31.10.2013 16:16, schrieb Peter Maydell:
> On 31 October 2013 14:36, Andreas Färber <afaerber@suse.de> wrote:
>> Am 31.10.2013 15:31, schrieb Peter Maydell:
>>> On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
>>>> Peter, since I had picked up the first two patches into my still pending
>>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>>> gotten an Acked-by.
>>>
>>> Hmm? I don't recall this part of the discussion. If you want the
>>> patches to have an Acked-by from you you need to send mail
>>> to the list with an Acked-by line.
>>
>> No, I added a Signed-off-by.
> 
> I checked my mail and the only thing I can find in reply to those
> patches is a note from you saying you added them to your queue.

Right, and as such they got a Signed-off-by, which should've been
visible in the link I usually add. Here's the pull messages you
should've been cc'ed on:

http://patchwork.ozlabs.org/patch/281630/
http://patchwork.ozlabs.org/patch/281575/

I don't see why I should reply with a Reviewed-by when I pick up patches
- again, same discussion as at QEMU Summit.

>> It was clearly stated that a Reviewed-by
>> needs to be explicitly sent as reply but that "looks okay" should in
>> exactly such a case where sender=submaintainer should be recorded as
>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
> 
> ...but you're not the submaintainer here so I don't think this applies.

It does, because you are the patch author and the ARM submaintainer
sending the pull.

> The point about the kernel practice as I understood it was that
> the kernel folks treat acked-by at about the same level of review as
> "looks ok to me" (ie, very little), not that there's some obligation to
> treat any informal 'looks ok' note as an acked-by. I'm in full agreement
> with Anthony that if you want a tag to appear you should send it
> properly.

If Anthony had been and would be more responsive as to why he didn't
pull the queue containing these patches with two different Sobs, we
wouldn't be having this conversation in the first place. Or had you not
gone on vacation or sent another pull before etc. etc.

Andreas

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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Peter Maydell]

On 31 October 2013 17:14, Andreas Färber <afaerber@suse.de> wrote:
> Am 31.10.2013 16:16, schrieb Peter Maydell:
>> On 31 October 2013 14:36, Andreas Färber <afaerber@suse.de> wrote:
>>> It was clearly stated that a Reviewed-by
>>> needs to be explicitly sent as reply but that "looks okay" should in
>>> exactly such a case where sender=submaintainer should be recorded as
>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>
>> ...but you're not the submaintainer here so I don't think this applies.
>
> It does, because you are the patch author and the ARM submaintainer
> sending the pull.

Er, no, because they're ARM subsystem patches. If they'd gone through
your queue and been written by somebody other than me and I'd given
them an acked-by, that would be worth noting (maybe) because it tells
the person applying your queue that I'm happy with these ARM related
patches even though they're not coming through the ARM queue.
Similarly if there were some QOM patches coming through my queue
that might make an acked-by from you useful. But these aren't QOM
patches, they're plain ARM patches, so the only person whose "ack"
is important is mine. Basically an 'ack' says "I have some kind of veto
over these patches and I'm not exercising it".

-- PMM

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Andreas Färber]

Am 31.10.2013 18:18, schrieb Peter Maydell:
> On 31 October 2013 17:14, Andreas Färber <afaerber@suse.de> wrote:
>> Am 31.10.2013 16:16, schrieb Peter Maydell:
>>> On 31 October 2013 14:36, Andreas Färber <afaerber@suse.de> wrote:
>>>> It was clearly stated that a Reviewed-by
>>>> needs to be explicitly sent as reply but that "looks okay" should in
>>>> exactly such a case where sender=submaintainer should be recorded as
>>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>>
>>> ...but you're not the submaintainer here so I don't think this applies.
>>
>> It does, because you are the patch author and the ARM submaintainer
>> sending the pull.
> 
> Er, no, because they're ARM subsystem patches.

You misunderstand. You sending an ARM patch in your ARM PULL with just
your Sob is the same as me sending a CPU patch with just my Sob in my
CPU PULL. That's what I was saying.

It is NOT about whether someone can veto something, it's about getting
external review and formally recognizing that review.
If Anthony is apparently making a retreat on that front, then we don't
necessarily need external review on our own subsystems, but if we want
to evaluate which or how many patches have been reviewed by someone else
then we need to record that in the commit message in *some* way. I don't
care what -by it is as long as we have and respect a clear rule.

Andreas

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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Peter Maydell]

On 31 October 2013 17:27, Andreas Färber <afaerber@suse.de> wrote:
> Am 31.10.2013 18:18, schrieb Peter Maydell:
>> On 31 October 2013 17:14, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 31.10.2013 16:16, schrieb Peter Maydell:
>>>> On 31 October 2013 14:36, Andreas Färber <afaerber@suse.de> wrote:
>>>>> It was clearly stated that a Reviewed-by
>>>>> needs to be explicitly sent as reply but that "looks okay" should in
>>>>> exactly such a case where sender=submaintainer should be recorded as
>>>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>>>
>>>> ...but you're not the submaintainer here so I don't think this applies.
>>>
>>> It does, because you are the patch author and the ARM submaintainer
>>> sending the pull.
>>
>> Er, no, because they're ARM subsystem patches.
>
> You misunderstand. You sending an ARM patch in your ARM PULL with just
> your Sob is the same as me sending a CPU patch with just my Sob in my
> CPU PULL.

I agree with this...

> That's what I was saying.

...it's just not at all what you seemed to be saying. I think this is
related to a disagreement about whether acked-by is at all meaningful
for anybody who's not the relevant subsystem maintainer or otherwise
an "authoritative person".

> It is NOT about whether someone can veto something, it's about getting
> external review and formally recognizing that review.

No, that's what Reviewed-by is for. Acked-by is exactly a statement
that "I think this looks OK and my opinion matters", which is implicitly
making the statement that it's not a NAK, ie not a veto. It's a handy
way to avoid somebody further upstream having to make an explicit
query of that person about whether they'd seen this stuff and were
happy with it, nothing more.

So, to be clear:
 * I welcome external review
 * If I get review and people send emails to the list with reviewed-by:
   tags I'll do my best (and my workflow generally helps) to pick up
   and reflect those tags in the pull requests
 * I'm not going to attempt to infer reviewed-by tags from anything
   other than a specific reply to the list with a tag in the proper format
 * pragmatically speaking there are some patches for ARM which do
   not get any third-party review and where patches have been on list
   for a reasonable period of time I'm going to put them in pull requests,
   since we can't stop the world just because we don't have enough
   people willing to code review things
 * acked-by doesn't imply (to me) any kind of level of review beyond "I don't
   object to this", so it is irrelevant for the purposes of "try to make sure
   patches get review" (which is a goal I agree with)
 * nonetheless I'll generally reflect specifically sent acked-by tags
   where I get them, simply because my usual workflow tends to
   result in that
 * I think a general rule that all tags should be sent to the list explicitly
   and nobody should infer them will be simpler and less confusing
   for all concerned

> If Anthony is apparently making a retreat on that front

I don't recall Anthony ever saying that external review was going
to be mandatory. I think it's certainly something we should try to
do better with, but pragmatically speaking we're not going to get
to 100% reviewed overnight. I'd definitely object to any proposal
to enforce full code review by simply refusing to apply nonreviewed
patches now (and I don't think anybody's proposed that).

>, then we don't
> necessarily need external review on our own subsystems, but if we want
> to evaluate which or how many patches have been reviewed by someone else
> then we need to record that in the commit message in *some* way. I don't
> care what -by it is as long as we have and respect a clear rule.

I don't think the rules have ever changed here; they've been broadly
described in the kernel doc that our wiki page points to for at least a
year. If you've reviewed a patch you mark that with Reviewed-by.

-- PMM

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Anthony Liguori]

On Thu, Oct 31, 2013 at 6:14 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 31.10.2013 16:16, schrieb Peter Maydell:
>> On 31 October 2013 14:36, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 31.10.2013 15:31, schrieb Peter Maydell:
>>>> On 31 October 2013 14:18, Andreas Färber <afaerber@suse.de> wrote:
>>>>> Peter, since I had picked up the first two patches into my still pending
>>>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>>>> gotten an Acked-by.
>>>>
>>>> Hmm? I don't recall this part of the discussion. If you want the
>>>> patches to have an Acked-by from you you need to send mail
>>>> to the list with an Acked-by line.
>>>
>>> No, I added a Signed-off-by.
>>
>> I checked my mail and the only thing I can find in reply to those
>> patches is a note from you saying you added them to your queue.
>
> Right, and as such they got a Signed-off-by, which should've been
> visible in the link I usually add. Here's the pull messages you
> should've been cc'ed on:
>
> http://patchwork.ozlabs.org/patch/281630/
> http://patchwork.ozlabs.org/patch/281575/
>
> I don't see why I should reply with a Reviewed-by when I pick up patches
> - again, same discussion as at QEMU Summit.
>
>>> It was clearly stated that a Reviewed-by
>>> needs to be explicitly sent as reply but that "looks okay" should in
>>> exactly such a case where sender=submaintainer should be recorded as
>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>
>> ...but you're not the submaintainer here so I don't think this applies.
>
> It does, because you are the patch author and the ARM submaintainer
> sending the pull.
>
>> The point about the kernel practice as I understood it was that
>> the kernel folks treat acked-by at about the same level of review as
>> "looks ok to me" (ie, very little), not that there's some obligation to
>> treat any informal 'looks ok' note as an acked-by. I'm in full agreement
>> with Anthony that if you want a tag to appear you should send it
>> properly.
>
> If Anthony had been and would be more responsive as to why he didn't
> pull the queue containing these patches with two different Sobs, we
> wouldn't be having this conversation in the first place. Or had you not
> gone on vacation or sent another pull before etc. etc.

Your tree is broken.  I gave you the errors that it produced.  You
were able to produce your own errors.  It's your responsibility, as a
subsystem maintainer, to test (and fix) your own tree.

Regards,

Anthony Liguori

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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Edgar E. Iglesias]

On Thu, Oct 31, 2013 at 03:18:41PM +0100, Andreas Färber wrote:
> Hi,
> 
> Am 31.10.2013 15:02, schrieb Edgar E. Iglesias:
> > On Fri, Oct 25, 2013 at 07:07:23PM +0100, Peter Maydell wrote:
> >> The following changes since commit fc8ead74674b7129e8f31c2595c76658e5622197:
> >>
> >>   Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2013-10-18 10:03:24 -0700)
> >>
> >> are available in the git repository at:
> >>
> >>
> >>   git://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20131025
> >>
> >> for you to fetch changes up to 71c903cc3b78fc563122fe40c5cadd050068b91a:
> >>
> >>   integrator: fix Linux boot failure by emulating dbg region (2013-10-25 18:27:07 +0100)
> > 
> > 
> > Applied, thanks all.
> 
> Edgar, there is no merge commit in qemu.git despite this being a signed
> pull. Do you maybe need to upgrade your version of git?

Hi, thanks for letting me know, I'll make sure to keep the merge commit
next time.

Cheers,
Edgar