Re: [Qemu-devel] [PATCH v4 16/21] target-arm: Implement SP_EL0, SP_EL1

[Peter Crosthwaite]

On Fri, Mar 7, 2014 at 5:33 AM, Peter Maydell  wrote:
> Implement handling for the AArch64 SP_EL0 system register.
> This holds the EL0 stack pointer, and is only accessible when
> it's not being used as the stack pointer, ie when we're in EL1
> and EL1 is using its own stack pointer. We also provide a
> definition of the SP_EL1 register; this isn't guest visible
> as a system register for an implementation like QEMU which
> doesn't provide EL2 or EL3; however it is useful for ensuring
> the underlying state is migrated.
>
> We need to update the state fields in the CPU state whenever

"whenever we".

> switch stack pointers; this happens when we take an exception
> and also when SPSEL is used to change the bit in PSTATE which
> indicates which stack pointer EL1 should use.
>
> Signed-off-by: Peter Maydell 
> ---
>  target-arm/cpu.h   |  2 ++
>  target-arm/helper.c| 34 ++
>  target-arm/internals.h | 25 +
>  target-arm/kvm64.c | 37 ++---
>  target-arm/machine.c   |  7 ---
>  target-arm/op_helper.c |  2 +-
>  6 files changed, 100 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 7ef2c71..338edc3 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -163,6 +163,7 @@ typedef struct CPUARMState {
>  uint64_t daif; /* exception masks, in the bits they are in in PSTATE */
>
>  uint64_t elr_el1; /* AArch64 ELR_EL1 */
> +uint64_t sp_el[2]; /* AArch64 banked stack pointers */
>

Should the macro AARCH64_MAX_EL_LEVELS exist for this?

>  /* System control coprocessor (cp15) */
>  struct {
> @@ -431,6 +432,7 @@ int cpu_arm_handle_mmu_fault (CPUARMState *env, 
> target_ulong address, int rw,
>   * Only these are valid when in AArch64 mode; in
>   * AArch32 mode SPSRs are basically CPSR-format.
>   */
> +#define PSTATE_SP (1U)
>  #define PSTATE_M (0xFU)
>  #define PSTATE_nRW (1U << 4)
>  #define PSTATE_F (1U << 6)
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 812fc73..6ee4135 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1682,6 +1682,27 @@ static uint64_t aa64_dczid_read(CPUARMState *env, 
> const ARMCPRegInfo *ri)
>  return cpu->dcz_blocksize | dzp_bit;
>  }
>
> +static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +if (!env->pstate & PSTATE_SP) {
> +/* Access to SP_EL0 is undefined if it's being used as
> + * the stack pointer.
> + */
> +return CP_ACCESS_TRAP_UNCATEGORIZED;
> +}
> +return CP_ACCESS_OK;
> +}
> +
> +static uint64_t spsel_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +return env->pstate & PSTATE_SP;
> +}
> +
> +static void spsel_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t 
> val)
> +{
> +update_spsel(env, val);
> +}
> +
>  static const ARMCPRegInfo v8_cp_reginfo[] = {
>  /* Minimal set of EL0-visible registers. This will need to be expanded
>   * significantly for system emulation of AArch64 CPUs.
> @@ -1814,6 +1835,19 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>.type = ARM_CP_NO_MIGRATE,
>.opc0 = 3, .opc1 = 0, .crn = 4, .crm = 0, .opc2 = 1,
>.access = PL1_RW, .fieldoffset = offsetof(CPUARMState, elr_el1) },
> +/* We rely on the access checks not allowing the guest to write to the
> + * state field when SPSel indicates that it's being used as the stack
> + * pointer.
> + */
> +{ .name = "SP_EL0", .state = ARM_CP_STATE_AA64,
> +  .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 1, .opc2 = 0,
> +  .access = PL1_RW, .accessfn = sp_el0_access,
> +  .type = ARM_CP_NO_MIGRATE,
> +  .fieldoffset = offsetof(CPUARMState, sp_el[0]) },
> +{ .name = "SPSel", .state = ARM_CP_STATE_AA64,
> +  .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 2, .opc2 = 0,
> +  .type = ARM_CP_NO_MIGRATE,
> +  .access = PL1_RW, .readfn = spsel_read, .writefn = spsel_write },
>  REGINFO_SENTINEL
>  };
>
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 11a7040..97a76c2 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -60,6 +60,31 @@ enum arm_fprounding {
>
>  int arm_rmode_to_sf(int rmode);
>
> +static inline void update_spsel(CPUARMState *env, uint32_t imm)
> +{
> +/* Update PSTATE SPSel bit; this requires us to update the
> + * working stack pointer in xregs[31].
> + */
> +if (!((imm ^ env->pstate) & PSTATE_SP)) {
> +return;
> +}
> +env->pstate = deposit32(env->pstate, 0, 1, imm);
> +
> +/* EL0 has no access rights to update SPSel, and this code
> + * assumes we are updating SP for EL1 while running as EL1.
> + */
> +assert(arm_current_pl(env) == 1);
> +if (env->pstate & PSTATE_SP) {
> +/* Switch from using SP_EL0 to using SP_ELx */
> +env->sp_el[0] = env->xregs[31];

Does this break on repeated writes to spsel bit of the same value? 

Re: [Qemu-devel] [PATCH v4 18/21] target-arm: Move arm_log_exception() into internals.h

[Peter Crosthwaite]

On Fri, Mar 7, 2014 at 5:33 AM, Peter Maydell  wrote:
> Move arm_log_exception() into internals.h so we can use it from
> helper-a64.c for the AArch64 exception entry code.
>
> Signed-off-by: Peter Maydell 

Reviewed-by: Peter Crosthwaite 

> ---
>  target-arm/helper.c| 31 ---
>  target-arm/internals.h | 31 +++
>  2 files changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3a976f7..e461914 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2849,37 +2849,6 @@ static void do_v7m_exception_exit(CPUARMState *env)
> pointer.  */
>  }
>
> -/* Exception names for debug logging; note that not all of these
> - * precisely correspond to architectural exceptions.
> - */
> -static const char * const excnames[] = {
> -[EXCP_UDEF] = "Undefined Instruction",
> -[EXCP_SWI] = "SVC",
> -[EXCP_PREFETCH_ABORT] = "Prefetch Abort",
> -[EXCP_DATA_ABORT] = "Data Abort",
> -[EXCP_IRQ] = "IRQ",
> -[EXCP_FIQ] = "FIQ",
> -[EXCP_BKPT] = "Breakpoint",
> -[EXCP_EXCEPTION_EXIT] = "QEMU v7M exception exit",
> -[EXCP_KERNEL_TRAP] = "QEMU intercept of kernel commpage",
> -[EXCP_STREX] = "QEMU intercept of STREX",
> -};
> -
> -static inline void arm_log_exception(int idx)
> -{
> -if (qemu_loglevel_mask(CPU_LOG_INT)) {
> -const char *exc = NULL;
> -
> -if (idx >= 0 && idx < ARRAY_SIZE(excnames)) {
> -exc = excnames[idx];
> -}
> -if (!exc) {
> -exc = "unknown";
> -}
> -qemu_log_mask(CPU_LOG_INT, "Taking exception %d [%s]\n", idx, exc);
> -}
> -}
> -
>  void arm_v7m_cpu_do_interrupt(CPUState *cs)
>  {
>  ARMCPU *cpu = ARM_CPU(cs);
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 97a76c2..e15136b 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -39,6 +39,37 @@ static inline bool excp_is_internal(int excp)
>  || excp == EXCP_STREX;
>  }
>
> +/* Exception names for debug logging; note that not all of these
> + * precisely correspond to architectural exceptions.
> + */
> +static const char * const excnames[] = {
> +[EXCP_UDEF] = "Undefined Instruction",
> +[EXCP_SWI] = "SVC",
> +[EXCP_PREFETCH_ABORT] = "Prefetch Abort",
> +[EXCP_DATA_ABORT] = "Data Abort",
> +[EXCP_IRQ] = "IRQ",
> +[EXCP_FIQ] = "FIQ",
> +[EXCP_BKPT] = "Breakpoint",
> +[EXCP_EXCEPTION_EXIT] = "QEMU v7M exception exit",
> +[EXCP_KERNEL_TRAP] = "QEMU intercept of kernel commpage",
> +[EXCP_STREX] = "QEMU intercept of STREX",
> +};
> +
> +static inline void arm_log_exception(int idx)
> +{
> +if (qemu_loglevel_mask(CPU_LOG_INT)) {
> +const char *exc = NULL;
> +
> +if (idx >= 0 && idx < ARRAY_SIZE(excnames)) {
> +exc = excnames[idx];
> +}
> +if (!exc) {
> +exc = "unknown";
> +}
> +qemu_log_mask(CPU_LOG_INT, "Taking exception %d [%s]\n", idx, exc);
> +}
> +}
> +
>  /* Scale factor for generic timers, ie number of ns per tick.
>   * This gives a 62.5MHz timer.
>   */
> --
> 1.9.0
>
>

Re: [Qemu-devel] [PATCH v4 21/21] hw/arm/virt: Add support for Cortex-A57

[Peter Crosthwaite]

On Fri, Mar 7, 2014 at 5:33 AM, Peter Maydell  wrote:
> Support the Cortex-A57 in the virt machine model.
>
> Signed-off-by: Peter Maydell 
> ---
> This should perhaps not be just stealing the a15mpcore_priv
> on the basis that it's a GICv2...

Wont this mean you gets lots of extraneous hardware? Although, with a
pure virtual machine I guess you can do whatever you really want.

> ---
>  hw/arm/virt.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 517f2fe..d985d2e 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -123,6 +123,14 @@ static VirtBoardInfo machines[] = {
>  .irqmap = a15irqmap,
>  },
>  {
> +.cpu_model = "cortex-a57",
> +/* Use the A15 private peripheral model for now: probably wrong! */
> +.qdevname = "a15mpcore_priv",

Can you just change this to gics qdev name? The qdev propnames of gic
and mpcore ("num-cpu" and "num-irq") should just match. Then perhaps a
little callback to set gicv2 version property.

Regards,
Peter

> +.gic_compatible = "arm,cortex-a15-gic",
> +.memmap = a15memmap,
> +.irqmap = a15irqmap,
> +},
> +{
>  .cpu_model = "host",
>  /* We use the A15 private peripheral model to get a V2 GIC */
>  .qdevname = "a15mpcore_priv",
> --
> 1.9.0
>
>

Re: [Qemu-devel] [PATCH v4 01/21] target-arm: Split out private-to-target functions into internals.h

[Peter Crosthwaite]

On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell  wrote:
> Currently cpu.h defines a mixture of functions and types needed by
> the rest of QEMU and those needed only by files within target-arm/.
> Split the latter out into a new header so they aren't needlessly
> exposed further than required.
>
> Signed-off-by: Peter Maydell 

Reviewed-by: Peter Crosthwaite 

> ---
>  target-arm/cpu.c   |  1 +
>  target-arm/cpu.h   | 20 ---
>  target-arm/helper.c|  1 +
>  target-arm/internals.h | 49 
> ++
>  target-arm/kvm32.c |  1 +
>  target-arm/op_helper.c |  1 +
>  target-arm/translate-a64.c |  1 +
>  target-arm/translate.c |  1 +
>  8 files changed, 55 insertions(+), 20 deletions(-)
>  create mode 100644 target-arm/internals.h
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 1ce8a9b..bc8eac9 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -19,6 +19,7 @@
>   */
>
>  #include "cpu.h"
> +#include "internals.h"
>  #include "qemu-common.h"
>  #include "hw/qdev-properties.h"
>  #include "qapi/qmp/qerror.h"
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 49fef3f..6252ff3 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -111,11 +111,6 @@ typedef struct ARMGenericTimer {
>  #define GTIMER_VIRT 1
>  #define NUM_GTIMERS 2
>
> -/* Scale factor for generic timers, ie number of ns per tick.
> - * This gives a 62.5MHz timer.
> - */
> -#define GTIMER_SCALE 16
> -
>  typedef struct CPUARMState {
>  /* Regs for current mode.  */
>  uint32_t regs[16];
> @@ -318,11 +313,7 @@ typedef struct CPUARMState {
>  #include "cpu-qom.h"
>
>  ARMCPU *cpu_arm_init(const char *cpu_model);
> -void arm_translate_init(void);
> -void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu);
>  int cpu_arm_exec(CPUARMState *s);
> -int bank_number(int mode);
> -void switch_mode(CPUARMState *, int);
>  uint32_t do_arm_semihosting(CPUARMState *env);
>
>  static inline bool is_a64(CPUARMState *env)
> @@ -545,17 +536,6 @@ static inline void vfp_set_fpcr(CPUARMState *env, 
> uint32_t val)
>  vfp_set_fpscr(env, new_fpscr);
>  }
>
> -enum arm_fprounding {
> -FPROUNDING_TIEEVEN,
> -FPROUNDING_POSINF,
> -FPROUNDING_NEGINF,
> -FPROUNDING_ZERO,
> -FPROUNDING_TIEAWAY,
> -FPROUNDING_ODD
> -};
> -
> -int arm_rmode_to_sf(int rmode);
> -
>  enum arm_cpu_mode {
>ARM_CPU_MODE_USR = 0x10,
>ARM_CPU_MODE_FIQ = 0x11,
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d44e603..3d65bae 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1,4 +1,5 @@
>  #include "cpu.h"
> +#include "internals.h"
>  #include "exec/gdbstub.h"
>  #include "helper.h"
>  #include "qemu/host-utils.h"
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> new file mode 100644
> index 000..a38a57f
> --- /dev/null
> +++ b/target-arm/internals.h
> @@ -0,0 +1,49 @@
> +/*
> + * QEMU ARM CPU -- internal functions and types
> + *
> + * Copyright (c) 2014 Linaro Ltd
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see
> + * 
> + *
> + * This header defines functions, types, etc which need to be shared
> + * between different source files within target-arm/ but which are
> + * private to it and not required by the rest of QEMU.
> + */
> +
> +#ifndef TARGET_ARM_INTERNALS_H
> +#define TARGET_ARM_INTERNALS_H
> +
> +/* Scale factor for generic timers, ie number of ns per tick.
> + * This gives a 62.5MHz timer.
> + */
> +#define GTIMER_SCALE 16
> +
> +int bank_number(int mode);
> +void switch_mode(CPUARMState *, int);
> +void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu);
> +void arm_translate_init(void);
> +
> +enum arm_fprounding {
> +FPROUNDING_TIEEVEN,
> +FPROUNDING_POSINF,
> +FPROUNDING_NEGINF,
> +FPROUNDING_ZERO,
> +FPROUNDING_TIEAWAY,
> +FPROUNDING_ODD
> +};
> +
> +int arm_rmode_to_sf(int rmode);
> +
> +#endif
> diff --git a/target-arm/kvm32.c b/target-arm/kvm32.c
> index a4fde07..b21f844 100644
> --- a/target-arm/kvm32.c
> +++ b/target-arm/kvm32.c
> @@ -21,6 +21,7 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
>  #include "cpu.h"
> +#include "internals.h"
>  #include "hw/arm/arm.h"
>
>  static inline void set_feature(uint64_t *features, int feature)
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c

Re: [Qemu-devel] [PATCH v4 16/21] target-arm: Implement SP_EL0, SP_EL1

[Peter Crosthwaite]

On Mon, Mar 17, 2014 at 5:02 PM, Peter Crosthwaite
 wrote:
> On Fri, Mar 7, 2014 at 5:33 AM, Peter Maydell  
> wrote:
>> Implement handling for the AArch64 SP_EL0 system register.
>> This holds the EL0 stack pointer, and is only accessible when
>> it's not being used as the stack pointer, ie when we're in EL1
>> and EL1 is using its own stack pointer. We also provide a
>> definition of the SP_EL1 register; this isn't guest visible
>> as a system register for an implementation like QEMU which
>> doesn't provide EL2 or EL3; however it is useful for ensuring
>> the underlying state is migrated.
>>
>> We need to update the state fields in the CPU state whenever
>
> "whenever we".
>
>> switch stack pointers; this happens when we take an exception
>> and also when SPSEL is used to change the bit in PSTATE which
>> indicates which stack pointer EL1 should use.
>>
>> Signed-off-by: Peter Maydell 
>> ---
>>  target-arm/cpu.h   |  2 ++
>>  target-arm/helper.c| 34 ++
>>  target-arm/internals.h | 25 +
>>  target-arm/kvm64.c | 37 ++---
>>  target-arm/machine.c   |  7 ---
>>  target-arm/op_helper.c |  2 +-
>>  6 files changed, 100 insertions(+), 7 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 7ef2c71..338edc3 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -163,6 +163,7 @@ typedef struct CPUARMState {
>>  uint64_t daif; /* exception masks, in the bits they are in in PSTATE */
>>
>>  uint64_t elr_el1; /* AArch64 ELR_EL1 */
>> +uint64_t sp_el[2]; /* AArch64 banked stack pointers */
>>
>
> Should the macro AARCH64_MAX_EL_LEVELS exist for this?
>
>>  /* System control coprocessor (cp15) */
>>  struct {
>> @@ -431,6 +432,7 @@ int cpu_arm_handle_mmu_fault (CPUARMState *env, 
>> target_ulong address, int rw,
>>   * Only these are valid when in AArch64 mode; in
>>   * AArch32 mode SPSRs are basically CPSR-format.
>>   */
>> +#define PSTATE_SP (1U)
>>  #define PSTATE_M (0xFU)
>>  #define PSTATE_nRW (1U << 4)
>>  #define PSTATE_F (1U << 6)
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 812fc73..6ee4135 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -1682,6 +1682,27 @@ static uint64_t aa64_dczid_read(CPUARMState *env, 
>> const ARMCPRegInfo *ri)
>>  return cpu->dcz_blocksize | dzp_bit;
>>  }
>>
>> +static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo 
>> *ri)
>> +{
>> +if (!env->pstate & PSTATE_SP) {
>> +/* Access to SP_EL0 is undefined if it's being used as
>> + * the stack pointer.
>> + */
>> +return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +}
>> +return CP_ACCESS_OK;
>> +}
>> +
>> +static uint64_t spsel_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> +{
>> +return env->pstate & PSTATE_SP;
>> +}
>> +
>> +static void spsel_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t 
>> val)
>> +{
>> +update_spsel(env, val);
>> +}
>> +
>>  static const ARMCPRegInfo v8_cp_reginfo[] = {
>>  /* Minimal set of EL0-visible registers. This will need to be expanded
>>   * significantly for system emulation of AArch64 CPUs.
>> @@ -1814,6 +1835,19 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>>.type = ARM_CP_NO_MIGRATE,
>>.opc0 = 3, .opc1 = 0, .crn = 4, .crm = 0, .opc2 = 1,
>>.access = PL1_RW, .fieldoffset = offsetof(CPUARMState, elr_el1) },
>> +/* We rely on the access checks not allowing the guest to write to the
>> + * state field when SPSel indicates that it's being used as the stack
>> + * pointer.
>> + */
>> +{ .name = "SP_EL0", .state = ARM_CP_STATE_AA64,
>> +  .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 1, .opc2 = 0,
>> +  .access = PL1_RW, .accessfn = sp_el0_access,
>> +  .type = ARM_CP_NO_MIGRATE,
>> +  .fieldoffset = offsetof(CPUARMState, sp_el[0]) },
>> +{ .name = "SPSel", .state = ARM_CP_STATE_AA64,
>> +  .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 2, .opc2 = 0,
>> +  .type = ARM_CP_NO_MIGRATE,
>> +  .access = PL1_RW, .readfn = spsel_read, .writefn = spsel_write },
>>  REGINFO_SENTINEL
>>  };
>>
>> diff --git a/target-arm/internals.h b/target-arm/internals.h
>> index 11a7040..97a76c2 100644
>> --- a/target-arm/internals.h
>> +++ b/target-arm/internals.h
>> @@ -60,6 +60,31 @@ enum arm_fprounding {
>>
>>  int arm_rmode_to_sf(int rmode);
>>
>> +static inline void update_spsel(CPUARMState *env, uint32_t imm)
>> +{
>> +/* Update PSTATE SPSel bit; this requires us to update the
>> + * working stack pointer in xregs[31].
>> + */
>> +if (!((imm ^ env->pstate) & PSTATE_SP)) {
>> +return;
>> +}

Ergh, my bad. Missed your short return path here.

>> +env->pstate = deposit32(env->pstate, 0, 1, imm);
>> +
>> +/* EL0 has no access rights to update SPSel, and this code
>> + * assumes we are updating SP for EL1 while running as EL1.
>> + */

[Qemu-devel] [PATCH v3 0/9] QMP: Introduce incremental drive-backup with in-memory dirty bitmap

[Fam Zheng]

Introduction


This implements incremental backup.

A convenient option "bitmap-use-mode" for drive-backup is introduced since v1.

Commands


A new sync mode for drive-backup is introduced:

drive-backup device=.. mode=.. sync=dirty-bitmap bitmap=bitmap0 
bitmap-use-mode=reset

This mode will scan dirty bitmap "bitmap0" and only copy all dirty sectors to
target.

bitmap-use-mode controls what operation is done on the given bitmap:

 - reset: Clear all the bits (block job save a copy)
 - consume: Block job takes the bitmap and make it invisible to user, and
   release it after use.

A few low level commands related to dirty bitmap are also added:

dirty-bitmap-add
dirty-bitmap-remove
dirty-bitmap-enable
dirty-bitmap-disable

Example
---

(QMP) dirty-bitmap-add device=? name=bitmap0

[guest writes to device]

(QMP) drive-backup device=? target=backup1.qcow2 format=qcow2 \
  sync=dirty-bitmap bitmap=bitmap0 bitmap-use-mode=reset

[guest writes more to device]

(QMP) drive-backup device=? target=backup2.qcow2 format=qcow2 \
  sync=dirty-bitmap bitmap=bitmap0 bitmap-use-mode=reset

...

v3:
Address Benoit's comments.

[01/09] qapi: Add optional field "name" to block dirty bitmap
Don't split line.

[03/09] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap
Add reviewed-by.

[04/09] block: Introduce bdrv_dirty_bitmap_granularity()
Add reviewed-by.

[05/09] hbitmap: Add hbitmap_copy
Fix size calculation.

[08/09] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
Fix typo in commit message.
Add comment for sync_bitmap_gran.
Add (Since 2.1).

Thanks,
Fam



Fam Zheng (9):
  qapi: Add optional field "name" to block dirty bitmap
  qmp: Add dirty-bitmap-add and dirty-bitmap-remove
  block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap
  block: Introduce bdrv_dirty_bitmap_granularity()
  hbitmap: Add hbitmap_copy
  block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
  qmp: Add dirty-bitmap-enable and dirty-bitmap-disable
  qmp: Add support of "dirty-bitmap" sync mode for drive-backup
  qapi: Add transaction support to dirty-bitmap-{add,disable}

 block-migration.c |   3 +-
 block.c   |  89 ++-
 block/backup.c|  53 +-
 block/mirror.c|   6 +-
 blockdev.c| 181 +-
 hmp.c |   4 +-
 include/block/block.h |  16 +++-
 include/block/block_int.h |   3 +
 include/qemu/hbitmap.h|   8 ++
 qapi-schema.json  | 116 +++--
 qmp-commands.hx   |  66 -
 util/hbitmap.c|  16 
 12 files changed, 543 insertions(+), 18 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v3 1/9] qapi: Add optional field "name" to block dirty bitmap

[Fam Zheng]

This field will be set for user created dirty bitmap. Also pass in an
error pointer to bdrv_create_dirty_bitmap, so when a name is already
taken on this BDS, it can report an error message. This is not global
check, two BDSes can have dirty bitmap with a common name.

Implemented bdrv_find_dirty_bitmap to find a dirty bitmap by name, will
be used later when other QMP commands want to reference dirty bitmap by
name.

Add bdrv_dirty_bitmap_make_anon. This unsets the name of dirty bitmap.

Signed-off-by: Fam Zheng 
---
 block-migration.c |  3 ++-
 block.c   | 33 -
 block/mirror.c|  2 +-
 include/block/block.h |  8 +++-
 qapi-schema.json  |  4 +++-
 5 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 897fdba..e6e016a 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -315,7 +315,8 @@ static void set_dirty_tracking(void)
 BlkMigDevState *bmds;
 
 QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
-bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE);
+bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
+  NULL, NULL);
 }
 }
 
diff --git a/block.c b/block.c
index 53f5b44..d240f1e 100644
--- a/block.c
+++ b/block.c
@@ -52,6 +52,7 @@
 
 struct BdrvDirtyBitmap {
 HBitmap *bitmap;
+char *name;
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5063,18 +5064,45 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, 
QEMUIOVector *qiov)
 return true;
 }
 
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int 
granularity)
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
+{
+BdrvDirtyBitmap *bm;
+QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
+if (!strcmp(name, bm->name)) {
+return bm;
+}
+}
+return NULL;
+}
+
+void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+g_free(bitmap->name);
+bitmap->name = NULL;
+}
+
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+  int granularity,
+  const char *name,
+  Error **errp)
 {
 int64_t bitmap_size;
 BdrvDirtyBitmap *bitmap;
 
 assert((granularity & (granularity - 1)) == 0);
 
+if (name && bdrv_find_dirty_bitmap(bs, name)) {
+error_setg(errp, "Bitmap already exists: %s", name);
+return NULL;
+}
 granularity >>= BDRV_SECTOR_BITS;
 assert(granularity);
 bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS);
 bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
 bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+if (name) {
+bitmap->name = g_strdup(name);
+}
 QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
 return bitmap;
 }
@@ -5086,6 +5114,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
 if (bm == bitmap) {
 QLIST_REMOVE(bitmap, list);
 hbitmap_free(bitmap->bitmap);
+g_free(bitmap->name);
 g_free(bitmap);
 return;
 }
@@ -5104,6 +5133,8 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 info->count = bdrv_get_dirty_count(bs, bm);
 info->granularity =
 ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+info->has_name = bm->name[0] != '\0';
+info->name = g_strdup(bm->name);
 entry->value = info;
 *plist = entry;
 plist = &entry->next;
diff --git a/block/mirror.c b/block/mirror.c
index dd5ee05..be8b2a1 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -596,7 +596,7 @@ static void mirror_start_job(BlockDriverState *bs, 
BlockDriverState *target,
 s->granularity = granularity;
 s->buf_size = MAX(buf_size, granularity);
 
-s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity);
+s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
 bdrv_set_enable_write_cache(s->target, true);
 bdrv_set_on_error(s->target, on_target_error, on_target_error);
 bdrv_iostatus_enable(s->target);
diff --git a/include/block/block.h b/include/block/block.h
index bd34d14..0d8e0d9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -428,7 +428,13 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, 
QEMUIOVector *qiov);
 
 struct HBitmapIter;
 typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int 
granularity);
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+  int granularity,
+  const char *name,
+  Error **errp);
+BdrvDirtyBitmap *bd

[Qemu-devel] [PATCH v3 5/9] hbitmap: Add hbitmap_copy

[Fam Zheng]

This makes a deep copy of an HBitmap.

Signed-off-by: Fam Zheng 
---
 include/qemu/hbitmap.h |  8 
 util/hbitmap.c | 16 
 2 files changed, 24 insertions(+)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 550d7ce..b645cfc 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -65,6 +65,14 @@ struct HBitmapIter {
 HBitmap *hbitmap_alloc(uint64_t size, int granularity);
 
 /**
+ * hbitmap_copy:
+ * @bitmap: The original bitmap to copy.
+ *
+ * Copy a HBitmap.
+ */
+HBitmap *hbitmap_copy(const HBitmap *bitmap);
+
+/**
  * hbitmap_empty:
  * @hb: HBitmap to operate on.
  *
diff --git a/util/hbitmap.c b/util/hbitmap.c
index d936831..d906c06 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -400,3 +400,19 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
 hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
 return hb;
 }
+
+HBitmap *hbitmap_copy(const HBitmap *bitmap)
+{
+int i;
+int64_t size;
+HBitmap *hb = g_memdup(bitmap, sizeof(struct HBitmap));
+
+size = bitmap->size;
+for (i = HBITMAP_LEVELS; i-- > 0; ) {
+size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+hb->levels[i] = g_memdup(bitmap->levels[i],
+ size * sizeof(unsigned long));
+}
+
+return hb;
+}
-- 
1.9.0

[Qemu-devel] [PATCH v3 7/9] qmp: Add dirty-bitmap-enable and dirty-bitmap-disable

[Fam Zheng]

This allows to put the dirty bitmap into a disabled state where no more
writes will be tracked.

It will be used before backup or writing to persistent file.

Signed-off-by: Fam Zheng 
---
 block.c   | 15 +++
 blockdev.c| 44 
 include/block/block.h |  2 ++
 qapi-schema.json  | 32 
 qmp-commands.hx   | 10 ++
 5 files changed, 103 insertions(+)

diff --git a/block.c b/block.c
index 8a8526d..4314591 100644
--- a/block.c
+++ b/block.c
@@ -55,6 +55,7 @@ struct BdrvDirtyBitmap {
 int64_t size;
 int64_t granularity;
 char *name;
+bool enabled;
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5134,6 +5135,7 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
 if (name) {
 bitmap->name = g_strdup(name);
 }
+bitmap->enabled = true;
 QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
 return bitmap;
 }
@@ -5152,6 +5154,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
 }
 }
 
+void bdrv_disable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+bitmap->enabled = false;
+}
+
+void bdrv_enable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+bitmap->enabled = true;
+}
+
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 {
 BdrvDirtyBitmap *bm;
@@ -5200,6 +5212,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
 {
 BdrvDirtyBitmap *bitmap;
 QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+if (!bitmap->enabled) {
+continue;
+}
 hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
 }
 }
diff --git a/blockdev.c b/blockdev.c
index 662c950..aa3ee55 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1773,6 +1773,50 @@ void qmp_dirty_bitmap_remove(const char *device, const 
char *name,
 bdrv_release_dirty_bitmap(bs, bitmap);
 }
 
+void qmp_dirty_bitmap_enable(const char *device, const char *name,
+ bool has_granularity, int64_t granularity,
+ Error **errp)
+{
+BlockDriverState *bs;
+BdrvDirtyBitmap *bitmap;
+
+bs = bdrv_find(device);
+if (!bs) {
+error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+return;
+}
+
+bitmap = bdrv_find_dirty_bitmap(bs, name);
+if (!bitmap) {
+error_setg(errp, "Dirty bitmap not found: %s", name);
+return;
+}
+
+bdrv_enable_dirty_bitmap(bs, bitmap);
+}
+
+void qmp_dirty_bitmap_disable(const char *device, const char *name,
+  bool has_granularity, int64_t granularity,
+  Error **errp)
+{
+BlockDriverState *bs;
+BdrvDirtyBitmap *bitmap;
+
+bs = bdrv_find(device);
+if (!bs) {
+error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+return;
+}
+
+bitmap = bdrv_find_dirty_bitmap(bs, name);
+if (!bitmap) {
+error_setg(errp, "Dirty bitmap not found: %s", name);
+return;
+}
+
+bdrv_disable_dirty_bitmap(bs, bitmap);
+}
+
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
 const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/block.h b/include/block/block.h
index 25acb60..9dab01b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -440,6 +440,8 @@ BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState 
*bs,
 const BdrvDirtyBitmap *bitmap,
 const char *name);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_disable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_enable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
   BdrvDirtyBitmap *bitmap);
diff --git a/qapi-schema.json b/qapi-schema.json
index 86117a3..e100b71 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2254,6 +2254,38 @@
   'data': { 'device': 'str', 'name': 'str' } }
 
 ##
+# @dirty-bitmap-enable
+#
+# Enable a dirty bitmap on the device
+#
+# Setting granularity has no effect here.
+#
+# Returns: nothing on success
+#  If @device is not a valid block device, DeviceNotFound
+#  If @name is not found, GenericError with an explaining message
+#
+# Since 2.1
+##
+{'command': 'dirty-bitmap-enable',
+  'data': 'DirtyBitmap' }
+
+##
+# @dirty-bitmap-disable
+#
+# Disable a dirty bitmap on the device
+#
+# Setting granularity has no effect here.
+#
+# Returns: nothing on success
+#  If @device is not a valid block device, DeviceNotFound
+#  If @name is not found, GenericError with an explaining message
+#
+# Since 2.1
+##
+{'command': 'dirty-bitmap-disable',
+  'data

[Qemu-devel] [PATCH v3 9/9] qapi: Add transaction support to dirty-bitmap-{add, disable}

[Fam Zheng]

This adds dirty-bitmap-add and dirty-bitmap-disable to transactions.
With this, user can stop a dirty bitmap, start backup of it, and start
another dirty bitmap atomically, so that the dirty bitmap is tracked
incrementally and we don't miss any write.

Signed-off-by: Fam Zheng 
---
 blockdev.c   | 68 
 qapi-schema.json |  4 +++-
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 4120dee..38dabe6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1411,6 +1411,64 @@ static void drive_backup_abort(BlkTransactionState 
*common)
 }
 }
 
+static void dirty_bitmap_add_prepare(BlkTransactionState *common, Error **errp)
+{
+DirtyBitmap *action;
+Error *local_err = NULL;
+
+action = common->action->dirty_bitmap_add;
+qmp_dirty_bitmap_add(action->device, action->name, false, 0, &local_err);
+if (error_is_set(&local_err)) {
+error_propagate(errp, local_err);
+}
+}
+
+static void dirty_bitmap_add_abort(BlkTransactionState *common)
+{
+DirtyBitmap *action;
+BdrvDirtyBitmap *bm;
+BlockDriverState *bs;
+
+action = common->action->dirty_bitmap_add;
+bs = bdrv_find(action->device);
+if (bs) {
+bm = bdrv_find_dirty_bitmap(bs, action->name);
+if (bm) {
+bdrv_release_dirty_bitmap(bs, bm);
+}
+}
+}
+
+static void dirty_bitmap_disable_prepare(BlkTransactionState *common,
+ Error **errp)
+{
+DirtyBitmap *action;
+Error *local_err = NULL;
+
+action = common->action->dirty_bitmap_disable;
+qmp_dirty_bitmap_disable(action->device, action->name,
+ false, 0, &local_err);
+if (error_is_set(&local_err)) {
+error_propagate(errp, local_err);
+}
+}
+
+static void dirty_bitmap_disable_abort(BlkTransactionState *common)
+{
+DirtyBitmap *action;
+BdrvDirtyBitmap *bitmap;
+BlockDriverState *bs;
+
+action = common->action->dirty_bitmap_disable;
+bs = bdrv_find(action->device);
+if (bs) {
+bitmap = bdrv_find_dirty_bitmap(bs, action->name);
+if (bitmap) {
+bdrv_enable_dirty_bitmap(bs, bitmap);
+}
+}
+}
+
 static void abort_prepare(BlkTransactionState *common, Error **errp)
 {
 error_setg(errp, "Transaction aborted using Abort action");
@@ -1443,6 +1501,16 @@ static const BdrvActionOps actions[] = {
 .prepare  = internal_snapshot_prepare,
 .abort = internal_snapshot_abort,
 },
+[TRANSACTION_ACTION_KIND_DIRTY_BITMAP_ADD] = {
+.instance_size = sizeof(BlkTransactionState),
+.prepare = dirty_bitmap_add_prepare,
+.abort = dirty_bitmap_add_abort,
+},
+[TRANSACTION_ACTION_KIND_DIRTY_BITMAP_DISABLE] = {
+.instance_size = sizeof(BlkTransactionState),
+.prepare = dirty_bitmap_disable_prepare,
+.abort = dirty_bitmap_disable_abort,
+},
 };
 
 /*
diff --git a/qapi-schema.json b/qapi-schema.json
index f385ef5..ed1845f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1993,7 +1993,9 @@
'blockdev-snapshot-sync': 'BlockdevSnapshot',
'drive-backup': 'DriveBackup',
'abort': 'Abort',
-   'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
+   'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
+   'dirty-bitmap-add': 'DirtyBitmap',
+   'dirty-bitmap-disable': 'DirtyBitmap'
} }
 
 ##
-- 
1.9.0

[Qemu-devel] [PATCH v3 3/9] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap

[Fam Zheng]

bdrv_getlength could fail, check the return value before using it.

Signed-off-by: Fam Zheng 
Reviewed-by: Benoit Canet 
---
 block.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index d240f1e..e32da75 100644
--- a/block.c
+++ b/block.c
@@ -5097,7 +5097,12 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
 }
 granularity >>= BDRV_SECTOR_BITS;
 assert(granularity);
-bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS);
+bitmap_size = bdrv_getlength(bs);
+if (bitmap_size < 0) {
+error_setg(errp, "could not get length of device");
+return NULL;
+}
+bitmap_size >>= BDRV_SECTOR_BITS;
 bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
 bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
 if (name) {
-- 
1.9.0

[Qemu-devel] [PATCH v3 4/9] block: Introduce bdrv_dirty_bitmap_granularity()

[Fam Zheng]

This returns the granularity (in sectors) of dirty bitmap.

Signed-off-by: Fam Zheng 
Reviewed-by: Benoit Canet 
---
 block.c   | 6 ++
 include/block/block.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/block.c b/block.c
index e32da75..ecc0dad 100644
--- a/block.c
+++ b/block.c
@@ -5157,6 +5157,12 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap 
*bitmap, int64_t sector
 }
 }
 
+int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
+  BdrvDirtyBitmap *bitmap)
+{
+return hbitmap_granularity(bitmap->bitmap);
+}
+
 void bdrv_dirty_iter_init(BlockDriverState *bs,
   BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 0d8e0d9..55fa44d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -437,6 +437,8 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState 
*bs,
 void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap 
*bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
+int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
+  BdrvDirtyBitmap *bitmap);
 int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t 
sector);
 void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
 void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int 
nr_sectors);
-- 
1.9.0

[Qemu-devel] [PATCH v3 2/9] qmp: Add dirty-bitmap-add and dirty-bitmap-remove

[Fam Zheng]

The new command pair is added to manage user created dirty bitmap. The
dirty bitmap's name is mandatory and must be unique for the same device,
but different devices can have bitmaps with the same names.

Signed-off-by: Fam Zheng 
---
 blockdev.c   | 60 
 qapi-schema.json | 45 ++
 qmp-commands.hx  | 49 +
 3 files changed, 154 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index c3422a1..662c950 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1713,6 +1713,66 @@ void qmp_block_set_io_throttle(const char *device, 
int64_t bps, int64_t bps_rd,
 }
 }
 
+void qmp_dirty_bitmap_add(const char *device, const char *name,
+  bool has_granularity, int64_t granularity,
+  Error **errp)
+{
+BlockDriverState *bs;
+BdrvDirtyBitmap *bitmap;
+
+bs = bdrv_find(device);
+if (!bs) {
+error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+return;
+}
+
+if (!name || name[0] == '\0') {
+error_setg(errp, "Bitmap name cannot be empty");
+return;
+}
+if (has_granularity) {
+if (granularity & (granularity - 1)) {
+error_setg(errp, "Granularity must be power of 2");
+return;
+}
+} else {
+granularity = 65536;
+}
+
+bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+if (!bitmap) {
+return;
+}
+}
+
+void qmp_dirty_bitmap_remove(const char *device, const char *name,
+ Error **errp)
+{
+BlockDriverState *bs;
+BdrvDirtyBitmap *bitmap;
+
+bs = bdrv_find(device);
+if (!bs) {
+error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+return;
+}
+
+if (!name || name[0] == '\0') {
+error_setg(errp, "Bitmap name cannot be empty");
+return;
+}
+bitmap = bdrv_find_dirty_bitmap(bs, name);
+if (!bitmap) {
+error_setg(errp, "Dirty bitmap not found: %s", name);
+return;
+}
+
+/* Make it invisible to user in case the following
+ * bdrv_release_dirty_bitmap doens't free it because of refcnt */
+bdrv_dirty_bitmap_make_anon(bs, bitmap);
+bdrv_release_dirty_bitmap(bs, bitmap);
+}
+
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
 const char *id = qdict_get_str(qdict, "id");
diff --git a/qapi-schema.json b/qapi-schema.json
index d935fa7..86117a3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2209,6 +2209,51 @@
 '*on-target-error': 'BlockdevOnError' } }
 
 ##
+# @DirtyBitmap
+#
+# @device: name of device which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# @granularity: #optional the bitmap granularity, default is 64k for
+#   dirty-bitmap-add
+#
+# Since 2.1
+##
+{ 'type': 'DirtyBitmap',
+  'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }
+
+##
+# @dirty-bitmap-add
+#
+# Create a dirty bitmap with a name on the device
+#
+# Returns: nothing on success
+#  If @device is not a valid block device, DeviceNotFound
+#  If @name is already taken, GenericError with an explaining message
+#
+# Since 2.1
+##
+{'command': 'dirty-bitmap-add',
+  'data': 'DirtyBitmap' }
+
+##
+# @dirty-bitmap-remove
+#
+# Remove a dirty bitmap on the device
+#
+# Setting granularity has no effect here.
+#
+# Returns: nothing on success
+#  If @device is not a valid block device, DeviceNotFound
+#  If @name is not found, GenericError with an explaining message
+#
+# Since 2.1
+##
+{'command': 'dirty-bitmap-remove',
+  'data': { 'device': 'str', 'name': 'str' } }
+
+##
 # @migrate_cancel
 #
 # Cancel the current executing migration process.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a22621f..bb32612 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1185,6 +1185,55 @@ Example:
 EQMP
 
 {
+.name   = "dirty-bitmap-add",
+.args_type  = "device:B,name:s,granularity:i?",
+.mhandler.cmd_new = qmp_marshal_input_dirty_bitmap_add,
+},
+{
+.name   = "dirty-bitmap-remove",
+.args_type  = "device:B,name:s",
+.mhandler.cmd_new = qmp_marshal_input_dirty_bitmap_remove,
+},
+
+SQMP
+
+dirty-bitmap-add
+
+
+Create a dirty bitmap with a name on the device, and start tracking the writes.
+
+Arguments:
+
+- "device": device name to create dirty bitmap (json-string)
+- "name": name of the new dirty bitmap (json-string)
+- "granularity": granularity to track writes with. (int)
+
+Example:
+
+-> { "execute": "dirty-bitmap-add", "arguments": { "device": "drive0",
+   "name": "bitmap0" } }
+<- { "return": {} }
+
+dirty-bitmap-remove
+
+
+Stop write tracking and remove the dirty bitmap that was created with
+dirty-bitmap-add.
+
+Arguments:
+
+- "device": dev

[Qemu-devel] [PATCH v3 6/9] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 block.c   | 30 --
 include/block/block.h |  4 
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index ecc0dad..8a8526d 100644
--- a/block.c
+++ b/block.c
@@ -52,6 +52,8 @@
 
 struct BdrvDirtyBitmap {
 HBitmap *bitmap;
+int64_t size;
+int64_t granularity;
 char *name;
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
@@ -5081,6 +5083,29 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
 bitmap->name = NULL;
 }
 
+BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
+const BdrvDirtyBitmap *bitmap,
+const char *name)
+{
+BdrvDirtyBitmap *new_bitmap;
+
+new_bitmap = g_memdup(bitmap, sizeof(BdrvDirtyBitmap));
+new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap);
+if (name) {
+new_bitmap->name = g_strdup(name);
+}
+QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
+return new_bitmap;
+}
+
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+HBitmap *original = bitmap->bitmap;
+
+bitmap->bitmap = hbitmap_alloc(bitmap->size, bitmap->granularity);
+hbitmap_free(original);
+}
+
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
   int granularity,
   const char *name,
@@ -5102,9 +5127,10 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
 error_setg(errp, "could not get length of device");
 return NULL;
 }
-bitmap_size >>= BDRV_SECTOR_BITS;
 bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
-bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+bitmap->size = bitmap_size >> BDRV_SECTOR_BITS;
+bitmap->granularity = ffs(granularity) - 1;
+bitmap->bitmap = hbitmap_alloc(bitmap->size, bitmap->granularity);
 if (name) {
 bitmap->name = g_strdup(name);
 }
diff --git a/include/block/block.h b/include/block/block.h
index 55fa44d..25acb60 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -435,6 +435,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 const char *name);
 void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap 
*bitmap);
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
+const BdrvDirtyBitmap *bitmap,
+const char *name);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
-- 
1.9.0

Re: [Qemu-devel] [PATCH v7] vl.c: Output error on invalid machine type

[Markus Armbruster]

mreza...@redhat.com writes:

> From: Miroslav Rezanina 
>
> Output error message using qemu's error_report() function when user
> provides the invalid machine type on the command line. This also saves
> time to find what issue is when you downgrade from one version of qemu
> to another that doesn't support required machine type yet (the version
> user downgraded to have to have this patch applied too, of course).
>
> Signed-off-by: Miroslav Rezanina 
> ---
> v7:
>  - use -machine instead of -M in error help message
>  - rebase to commit 0056ae2
>
> v6:
>  - print help instead of list supported machines on error
> ---
>  vl.c | 21 +
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 862cf20..cbd1381 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2649,15 +2649,20 @@ static MachineClass *machine_parse(const char *name)
>  if (mc) {
>  return mc;
>  }
> -printf("Supported machines are:\n");
> -for (el = machines; el; el = el->next) {
> -MachineClass *mc = el->data;
> -QEMUMachine *m = mc->qemu_machine;
> -if (m->alias) {
> -printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
> +if (name && !is_help_option(name)) {
> +error_report("Unsupported machine type");
> +printf("\nUse -machine help to list supported machines!\n");

Why the newline before 'Use'?

Recommend to write the hint to the same fd as the error message.  An
obvious way to do that is error_printf(), and it's what we do elsewhere.

Both missed this in my review of v1, sorry.

> +} else {
> +printf("Supported machines are:\n");
> +for (el = machines; el; el = el->next) {
> +MachineClass *mc = el->data;
> +QEMUMachine *m = mc->qemu_machine;
> +if (m->alias) {
> +printf("%-20s %s (alias of %s)\n", m->alias, m->desc, 
> m->name);
> +}
> +printf("%-20s %s%s\n", m->name, m->desc,
> +   m->is_default ? " (default)" : "");
>  }
> -printf("%-20s %s%s\n", m->name, m->desc,
> -   m->is_default ? " (default)" : "");
>  }
>  
>  g_slist_free(machines);

The functions logic is a bit tortured (not your fault).  Here's how I'd
clean it up:

static MachineClass *machine_parse(const char *name)
{
MachineClass *mc;
GSList *el, *machines;

if (is_help_option(name)) {
machines = object_class_get_list(TYPE_MACHINE, false);

printf("Supported machines are:\n");
for (el = machines; el; el = el->next) {
MachineClass *mc = el->data;
QEMUMachine *m = mc->qemu_machine;
if (m->alias) {
printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
}
printf("%-20s %s%s\n", m->name, m->desc,
   m->is_default ? " (default)" : "");
}

g_slist_free(machines);
exit(0);
}

mc = find_machine(name);
if (!mc) {
error_report("Unsupported machine type");
error_printf("Use '-machine help' to list supported machines\n");
exit(1);
}
return mc;
}

[Qemu-devel] [PATCH v3 8/9] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[Fam Zheng]

For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.

There are two bitmap use modes for sync=dirty-bitmap:

 - reset: backup job makes a copy of bitmap and resets the original
   one.
 - consume: backup job makes the original anonymous (invisible to user)
   and releases it after use.

Signed-off-by: Fam Zheng 
---
 block/backup.c| 53 ++-
 block/mirror.c|  4 
 blockdev.c|  9 +++-
 hmp.c |  4 +++-
 include/block/block_int.h |  3 +++
 qapi-schema.json  | 31 +++
 qmp-commands.hx   |  7 ---
 7 files changed, 101 insertions(+), 10 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 15a2e55..24b8d2c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,10 @@ typedef struct CowRequest {
 typedef struct BackupBlockJob {
 BlockJob common;
 BlockDriverState *target;
+/* bitmap for sync=dirty-bitmap */
+BdrvDirtyBitmap *sync_bitmap;
+/* dirty bitmap granularity */
+int sync_bitmap_gran;
 MirrorSyncMode sync_mode;
 RateLimit limit;
 BlockdevOnError on_source_error;
@@ -263,7 +267,7 @@ static void coroutine_fn backup_run(void *opaque)
 job->common.busy = true;
 }
 } else {
-/* Both FULL and TOP SYNC_MODE's require copying.. */
+/* FULL, TOP and DIRTY_BITMAP SYNC_MODE's require copying.. */
 for (; start < end; start++) {
 bool error_is_read;
 
@@ -317,7 +321,21 @@ static void coroutine_fn backup_run(void *opaque)
 if (alloced == 0) {
 continue;
 }
+} else if (job->sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+int i, dirty = 0;
+for (i = 0; i < BACKUP_SECTORS_PER_CLUSTER;
+ i += job->sync_bitmap_gran) {
+if (bdrv_get_dirty(bs, job->sync_bitmap,
+start * BACKUP_SECTORS_PER_CLUSTER + i)) {
+dirty = 1;
+break;
+}
+}
+if (!dirty) {
+continue;
+}
 }
+
 /* FULL sync mode we copy the whole drive. */
 ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER,
 BACKUP_SECTORS_PER_CLUSTER, &error_is_read);
@@ -341,6 +359,9 @@ static void coroutine_fn backup_run(void *opaque)
 qemu_co_rwlock_wrlock(&job->flush_rwlock);
 qemu_co_rwlock_unlock(&job->flush_rwlock);
 
+if (job->sync_bitmap) {
+bdrv_release_dirty_bitmap(bs, job->sync_bitmap);
+}
 hbitmap_free(job->bitmap);
 
 bdrv_iostatus_disable(target);
@@ -351,12 +372,15 @@ static void coroutine_fn backup_run(void *opaque)
 
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
   int64_t speed, MirrorSyncMode sync_mode,
+  BdrvDirtyBitmap *sync_bitmap,
+  BitmapUseMode bitmap_mode,
   BlockdevOnError on_source_error,
   BlockdevOnError on_target_error,
   BlockDriverCompletionFunc *cb, void *opaque,
   Error **errp)
 {
 int64_t len;
+BdrvDirtyBitmap *original;
 
 assert(bs);
 assert(target);
@@ -369,6 +393,28 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
 return;
 }
 
+if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP && !sync_bitmap) {
+error_setg(errp, "must provide a valid bitmap name for 
\"dirty-bitmap\""
+ "sync mode");
+return;
+}
+
+if (sync_bitmap) {
+switch (bitmap_mode) {
+case BITMAP_USE_MODE_RESET:
+original = sync_bitmap;
+sync_bitmap = bdrv_copy_dirty_bitmap(bs, sync_bitmap, NULL);
+bdrv_reset_dirty_bitmap(bs, original);
+break;
+case BITMAP_USE_MODE_CONSUME:
+bdrv_dirty_bitmap_make_anon(bs, sync_bitmap);
+break;
+default:
+assert(0);
+}
+bdrv_disable_dirty_bitmap(bs, sync_bitmap);
+}
+
 len = bdrv_getlength(bs);
 if (len < 0) {
 error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -386,6 +432,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
 job->on_target_error = on_target_error;
 job->target = target;
 job->sync_mode = sync_mode;
+job->sync_bitmap = sync_bitmap;
+if (sync_bitmap) {
+job->sync_bitmap_gran =
+bdrv_dirty_bitmap_granularity(bs, job->sync_bitmap);
+}
 job->common.len = len;
 job->common.co = qemu_coroutine_create(backup_run);
 qemu_coroutine_enter(job->common.co, job);

Re: [Qemu-devel] [PATCH] Makefile: Don't find and delete when $(DSOSUF) is empty in "make clean"

[Markus Armbruster]

Stefan Weil  writes:

> Am 14.03.2014 09:38, schrieb Fam Zheng:
>> DANGEROUS: don't try it before you read to the end.
>>
>> A first "make distclean" will unset $(DSOSUF), a following "make
>> distclean" or "make clean" will find all the files and delete it.
>>
>> Including all the files in the .git directory!
>
> If you only use out-of-tree build, you are safe here. Maybe we should no
> longer support in-tree builds. Personally, I nearly never use them.

Same here.  Building in-tree is calling for trouble.  I'd support a
patch that prevents it.

[...]

Re: [Qemu-devel] [PATCH v2] update names in option tables to match with actual command-line spelling

[Markus Armbruster]

[cc: Eric]

Amos Kong  writes:

> This patch makes all names in option table to match with actual
> command-line spelling, it will be helpful when we search in option
> tables.

As discussed in [*], the QemuOptsList member name values are ABI:
changing them can break existing -readconfig configuration files.  If we
decide breaking ABI is okay here (big if!), we need to document it
prominently in the commit message.

> Signed-off-by: Amos Kong 
> ---
> V2: fix name in acpi option table
> ---
>  hw/acpi/core.c|  8 
>  hw/nvram/fw_cfg.c |  4 ++--
>  include/qemu/option.h |  2 +-
>  vl.c  | 14 +++---
>  4 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 79414b4..12e9ae8 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -54,16 +54,16 @@ static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE - 
> ACPI_TABLE_PFX_SIZE] =
>  char unsigned *acpi_tables;
>  size_t acpi_tables_len;
>  
> -static QemuOptsList qemu_acpi_opts = {
> -.name = "acpi",
> +static QemuOptsList qemu_acpitable_opts = {
> +.name = "acpitable",
>  .implied_opt_name = "data",
> -.head = QTAILQ_HEAD_INITIALIZER(qemu_acpi_opts.head),
> +.head = QTAILQ_HEAD_INITIALIZER(qemu_acpitable_opts.head),
>  .desc = { { 0 } } /* validated with OptsVisitor */
>  };
>  
>  static void acpi_register_config(void)
>  {
> -qemu_add_opts(&qemu_acpi_opts);
> +qemu_add_opts(&qemu_acpitable_opts);
>  }
>  
>  machine_init(acpi_register_config);
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index cb36dc2..1c75507 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -125,7 +125,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>  const char *temp;
>  
>  /* get user configuration */
> -QemuOptsList *plist = qemu_find_opts("boot-opts");
> +QemuOptsList *plist = qemu_find_opts("boot");
>  QemuOpts *opts = QTAILQ_FIRST(&plist->head);
>  if (opts != NULL) {
>  temp = qemu_opt_get(opts, "splash");
> @@ -191,7 +191,7 @@ static void fw_cfg_reboot(FWCfgState *s)
>  const char *temp;
>  
>  /* get user configuration */
> -QemuOptsList *plist = qemu_find_opts("boot-opts");
> +QemuOptsList *plist = qemu_find_opts("boot");
>  QemuOpts *opts = QTAILQ_FIRST(&plist->head);
>  if (opts != NULL) {
>  temp = qemu_opt_get(opts, "reboot-timeout");
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 8c0ac34..96b7c29 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -102,7 +102,7 @@ typedef struct QemuOptDesc {
>  } QemuOptDesc;
>  
>  struct QemuOptsList {
> -const char *name;
> +const char *name;  /* option name */
>  const char *implied_opt_name;
>  bool merge_lists;  /* Merge multiple uses of option into a single list? 
> */
>  QTAILQ_HEAD(, QemuOpts) head;

Your patch makes the code adhere to an convention "QemuOptsList name
must match the name of the (non-sugared) command line option using it".
Apart from the comment you add here, it's an unspoken convention.

Making such a convention stick usually takes a tests that fail when it's
violated.

> diff --git a/vl.c b/vl.c
> index 685a7a9..2bcf5fe 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -380,7 +380,7 @@ static QemuOptsList qemu_machine_opts = {
>  };
>  
>  static QemuOptsList qemu_boot_opts = {
> -.name = "boot-opts",
> +.name = "boot",
>  .implied_opt_name = "order",
>  .merge_lists = true,
>  .head = QTAILQ_HEAD_INITIALIZER(qemu_boot_opts.head),
> @@ -1304,7 +1304,7 @@ static void numa_add(const char *optarg)
>  }
>  
>  static QemuOptsList qemu_smp_opts = {
> -.name = "smp-opts",
> +.name = "smp",
>  .implied_opt_name = "cpus",
>  .merge_lists = true,
>  .head = QTAILQ_HEAD_INITIALIZER(qemu_smp_opts.head),
> @@ -3124,7 +3124,7 @@ int main(int argc, char **argv, char **envp)
>  drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
>  break;
>  case QEMU_OPTION_boot:
> -opts = qemu_opts_parse(qemu_find_opts("boot-opts"), optarg, 
> 1);
> +opts = qemu_opts_parse(qemu_find_opts("boot"), optarg, 1);
>  if (!opts) {
>  exit(1);
>  }
> @@ -3493,7 +3493,7 @@ int main(int argc, char **argv, char **envp)
>  break;
>  }
>  case QEMU_OPTION_acpitable:
> -opts = qemu_opts_parse(qemu_find_opts("acpi"), optarg, 1);
> +opts = qemu_opts_parse(qemu_find_opts("acpitable"), optarg, 
> 1);
>  if (!opts) {
>  exit(1);
>  }
> @@ -3560,7 +3560,7 @@ int main(int argc, char **argv, char **envp)
>  }
>  break;
>  case QEMU_OPTION_smp:
> -if (!qemu_opts_parse(qemu_find_opts("smp-opts"), optarg, 1)) 
> {
> +if (!qemu_opts_parse(qemu_find

Re: [Qemu-devel] [PATCH] qemu-nbd: Fix coverity issues

[Markus Armbruster]

Paolo Bonzini  writes:

> There are two issues in qemu-nbd: a missing return value check after
> calling accept(), and file descriptor leaks in nbd_client_thread.
>
> Signed-off-by: Paolo Bonzini 
> ---
>  qemu-nbd.c | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index bdac1f3..899e67c 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -288,19 +288,19 @@ static void *nbd_client_thread(void *arg)
>  ret = nbd_receive_negotiate(sock, NULL, &nbdflags,
>  &size, &blocksize);
>  if (ret < 0) {
> -goto out;
> +goto out_socket;
>  }
>  
>  fd = open(device, O_RDWR);
>  if (fd < 0) {
>  /* Linux-only, we can use %m in printf.  */
>  fprintf(stderr, "Failed to open %s: %m", device);
> -goto out;
> +goto out_socket;
>  }
>  
>  ret = nbd_init(fd, sock, nbdflags, size, blocksize);
>  if (ret < 0) {
> -goto out;
> +goto out_fd;
>  }
>  
>  /* update partition table */
> @@ -316,12 +316,16 @@ static void *nbd_client_thread(void *arg)
>  
>  ret = nbd_client(fd);
>  if (ret) {
> -goto out;
> +goto out_fd;
>  }
>  close(fd);
>  kill(getpid(), SIGTERM);
>  return (void *) EXIT_SUCCESS;
>  
> +out_fd:
> +close(fd);
> +out_socket:
> +closesocket(sock);
>  out:
>  kill(getpid(), SIGTERM);
>  return (void *) EXIT_FAILURE;

The return values are disgusting, but that's not your fault.  Hmm,
actually it is: commit a517e88b.

> @@ -355,6 +359,11 @@ static void nbd_accept(void *opaque)
>  socklen_t addr_len = sizeof(addr);
>  
>  int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
> +if (fd < 0) {
> +perror("accept");
> +return;
> +}
> +
>  if (state >= TERMINATE) {
>  close(fd);
>  return;

Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] 9pfs troubles (was Re: [PATCH 1/4] hw/9pfs: fix error handing in local_ioc_getversion())

[Greg Kurz]

On Wed, 12 Mar 2014 13:34:44 +0200
"Michael S. Tsirkin"  wrote:
> On Fri, Feb 07, 2014 at 10:02:52AM +0100, Greg Kurz wrote:
> > On Wed, 5 Feb 2014 23:31:11 +0200
> > "Michael S. Tsirkin"  wrote:
> > > On Tue, Feb 04, 2014 at 12:51:25PM +0530, Aneesh Kumar K.V wrote:
> > > > "Michael S. Tsirkin"  writes:
> > > > 
> > > > > On Mon, Feb 03, 2014 at 03:05:10PM +0530, Aneesh Kumar K.V wrote:
> > > > >> "Michael S. Tsirkin"  writes:
> > > > >> 
> > > > >> > Haven't used 9pfs in a while.
> > > > >> > I thought these patches are a good time to play with it some more.
> > > > >> > I have encountered two issues.
> > > > >> >
> > > > >> > What I'm doing:
> > > > >> > host: qemu a75143eda2ddf581b51e96c000974bcdfe2cbd10.
> > > > >> >
> > > > >> > /scm/qemu/x86_64-softmmu/qemu-system-x86_64 -enable-kvm -m 1g -cpu
> > > > >> > kvm64 -smp 2  f20-x64.qcow2  -netdev user,id=foo -redir
> > > > >> > tcp:8022::22 -device virtio-net,netdev=foo  -serial stdio -fsdev
> > > > >> > local,security_model=none,id=fsdev0,path=/lib/modules/ -device
> > > > >> > virtio-9p-pci,id=fs0,fsdev=fsdev0,mount_tag=libmodulesshare -fsdev
> > > > >> > local,security_model=none,id=fsdev1,path=/boot -device
> > > > >> > virtio-9p-pci,id=fs1,fsdev=fsdev0,mount_tag=bootshare -no-reboot
> > > > >> > -snapshot
> > > > >> >
> > > > >> > guest: Fedora 20
> > > > >> >
> > > > >> > added this in /etc/fstab:
> > > > >> >
> > > > >> > bootshare   /share/boot 9p
> > > > >> > trans=virtio,version=9p2000.L 0 0
> > > > >> > libmodulesshare /share/lib/modules  9p
> > > > >> > trans=virtio,version=9p2000.L 0 0
> > > > >> >
> > > > >> >
> > > > >> > I have encountered two issues:
> > > > >> >
> > > > >> > 1. mount failure on boot
> > > > >> > If I try to mount on boot through fstab, I get:
> > > > >> > [2.270157] 9pnet: Could not find request transport: virtio
> > > > >> > [2.270158] 9pnet: Could not find request transport: virtio
> > > > >> 
> > > > >> 
> > > > >> Missing 9pnet_virtio.ko module ? 
> > > > >
> > > > > Maybe it's loaded too late. But when I get to plymouth prompt
> > > > > it's loaded fine.
> > > 
> > > Any idea about this one? Do you have guests with 9pfs
> > > and virtio as modules and 9pfs mounted from /etc/fstab?
> > > 
> > 
> > Hi Michael,
> > 
> > I had the very same problem. You probably need to add 9pnet_virtio to the
> > initramfs. 
> > 
> > # mkinitrd -f --with=9pnet_virtio /boot/initramfs-$(uname -r).img $(uname 
> > -r)
> > # gzip -dc /boot/initramfs-$(uname -r).img | cpio -t | grep 9pnet
> > usr/lib/modules/3.11.7-200.fc19.ppc64p7/kernel/net/9p/9pnet_virtio.ko
> > usr/lib/modules/3.11.7-200.fc19.ppc64p7/kernel/net/9p/9pnet.ko
> > 
> > Cheers.
> 
> This seems to help but why is this necessary
> in the initrd?
> After all I am not trying to boot from 9pfs.
> 

I do not know... maybe it has to do with the way systemd deals with fstab... :-\

> 
> > -- 
> > Gregory Kurz kurzg...@fr.ibm.com
> >  gk...@linux.vnet.ibm.com
> > Software Engineer @ IBM/Meiosys  http://www.ibm.com
> > Tel +33 (0)562 165 496
> > 
> > "Anarchy is about taking complete responsibility for yourself."
> > Alan Moore.
> 



-- 
Gregory Kurz kurzg...@fr.ibm.com
 gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
Alan Moore.

Re: [Qemu-devel] [PATCH 3/4] virtio-gpu: v0.3 of the virtio based GPU code.

[Paolo Bonzini]

Il 17/03/2014 05:36, Dave Airlie ha scritto:

I steered away from using config space for anything for the normal
operation of the GPU after looking at overheads and hearing from S390
people that config space has some special properties on their hw,


Right, but this is only for the old s390-virtio implementation, and it 
means that the cursor would not work on s390-virtio either.  The new 
one, which looks like real s390 hardware, doesn't have the problem.



The number of these events should be small in a running system, and
I'm not sure how we'd lose one.


Are they few even if you resize the viewer window, and this causes a 
stream of display-size changes?


Paolo

Re: [Qemu-devel] Coverity scan successes

[Kevin Wolf]

Hi Paolo,

Am 14.03.2014 um 13:20 hat Paolo Bonzini geschrieben:
> Of course, the defect density varies across subsystems:
> 
>   ratio   # defects
>   SLIRP   2.8620
>   9pfs/virtio-9p  1.6916
>   Bluetooth   1.316
>   NBD 1.312
>   User-mode emulation 0.8425
>   Block layer 0.6625

How would I get access to the Coverity results? I feel the block layer
is scoring a bit too high here... :-)

Kevin

Re: [Qemu-devel] Coverity scan successes

[Paolo Bonzini]

Il 17/03/2014 10:31, Kevin Wolf ha scritto:


Am 14.03.2014 um 13:20 hat Paolo Bonzini geschrieben:

> Of course, the defect density varies across subsystems:
>
>ratio   # defects
>SLIRP   2.8620
>9pfs/virtio-9p  1.6916
>Bluetooth   1.316
>NBD 1.312
>User-mode emulation 0.8425
>Block layer 0.6625

How would I get access to the Coverity results?


You ask. :)


I feel the block layer
is scoring a bit too high here... :-)


Well, five of those are simply new unchecked uses of strstart that 
Coverity complains about and I've muted them so you're already down to 
0.5. :)


Most of the problems are overflows caused by int32 multiplications (such 
as number of sectors * 512, or number of clusters * clusters per sector) 
before casting to int64.  Many of them probably cannot really happen, 
because one of the factor is small and related to the size of an L2 
table; for example the number of sectors could be the size of an L2 
table, or the number of clusters could be the number of entries in an L2 
table.


Paolo

Re: [Qemu-devel] [PATCH 3/4] virtio-gpu: v0.3 of the virtio based GPU code.

[Paolo Bonzini]

Il 17/03/2014 06:21, Dave Airlie ha scritto:

Oh I was also going to use this queue to report HW error events from
the guest to the host,

like if the guest tries an illegal operation,


What fields would be present for such an error?

Paolo

Re: [Qemu-devel] [PATCH] target-i386: Add missing 'static' and 'const' attributes

[Paolo Bonzini]

Il 16/03/2014 15:03, Stefan Weil ha scritto:

This fixes warnings from the static code analysis (smatch).

Signed-off-by: Stefan Weil 
---

Why is array para_features in kvm.c terminated by a dummy entry?
It is only used in a for loop with upper limit ARRAY_SIZE(para_features) - 1.


No particular reason.

Paolo

Re: [Qemu-devel] [PATCH for-2.0] rules.mak: Fix per object libs extraction

[Paolo Bonzini]

Il 14/03/2014 03:21, Fam Zheng ha scritto:

Don't sort the extracted options, sort the objects.

Reported-by: Christian Mahnke 
Signed-off-by: Fam Zheng 
---
 rules.mak | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rules.mak b/rules.mak
index 9dda9f7..5c454d8 100644
--- a/rules.mak
+++ b/rules.mak
@@ -23,8 +23,8 @@ QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(*D)/$(*F).d
 QEMU_INCLUDES += -I$(

Thanks, I'll send a pull request with this and the other patch.

Reviewed-by: Paolo Bonzini 

Re: [Qemu-devel] [PATCH for-2.1 2/2] i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF

[Laszlo Ersek]

On 03/16/14 13:31, Michael S. Tsirkin wrote:
> On Fri, Mar 14, 2014 at 11:22:52PM +0100, Laszlo Ersek wrote:
>> Building on the previous patch, raise the maximal count of processor
>> objects / NTFY branches / CPON elements from 255 to 256. This allows the
>> VCPU with APIC ID 0xFF to be hotplugged.
>>
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  hw/i386/acpi-build.c | 16 +++-
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 2bbefb5..51162fc 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -999,11 +999,15 @@ build_ssdt(GArray *table_data, GArray *linker,
>> AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
>> PcPciInfo *pci, PcGuestInfo *guest_info)
>>  {
>> -int acpi_cpus = MIN(0xff, guest_info->apic_id_limit);
> 
> Maybe just make this line
>  int acpi_cpus = guest_info->apic_id_limit;
> and then the patch will be smaller.

I did think of that, but I didn't like the unchecked unsigned --> int
conversion. The limit checks below cover that too.

Also, the patch renders the function similar to the other acpi builder
functions; there are a handful of loops that directly name
"guest_info->apic_id_limit" in their controlling expressions.

Anyway, would you be OK with a patch that did the assignment you suggest
(rather than modifying the loops), and kept the BUILD_BUG and the
g_assert below?

Thanks
Laszlo

> 
>>  int ssdt_start = table_data->len;
>>  uint8_t *ssdt_ptr;
>>  int i;
>>  
>> +/* The current AML generator can cover the APIC ID range [0..255],
>> + * inclusive, for VCPU hotplug. */
>> +QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
>> +g_assert(guest_info->apic_id_limit <= ACPI_CPU_HOTPLUG_ID_LIMIT);
>> +
>>  /* Copy header and patch values in the S3_ / S4_ / S5_ packages */
>>  ssdt_ptr = acpi_data_push(table_data, sizeof(ssdp_misc_aml));
>>  memcpy(ssdt_ptr, ssdp_misc_aml, sizeof(ssdp_misc_aml));
>> @@ -1029,7 +1033,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>  build_append_nameseg(sb_scope, "_SB_");
>>  
>>  /* build Processor object for each processor */
>> -for (i = 0; i < acpi_cpus; i++) {
>> +for (i = 0; i < guest_info->apic_id_limit; i++) {
>>  uint8_t *proc = acpi_data_push(sb_scope, ACPI_PROC_SIZEOF);
>>  memcpy(proc, ACPI_PROC_AML, ACPI_PROC_SIZEOF);
>>  proc[ACPI_PROC_OFFSET_CPUHEX] = acpi_get_hex(i >> 4);
>> @@ -1042,7 +1046,8 @@ build_ssdt(GArray *table_data, GArray *linker,
>>   *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} 
>> ...}
>>   */
>>  /* Arg0 = Processor ID = APIC ID */
>> -build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus);
>> +build_append_notify_method(sb_scope, "NTFY", "CP%0.02X",
>> +   guest_info->apic_id_limit);
>>  
>>  /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" 
>> */
>>  build_append_byte(sb_scope, 0x08); /* NameOp */
>> @@ -1052,8 +1057,9 @@ build_ssdt(GArray *table_data, GArray *linker,
>>  GArray *package = build_alloc_array();
>>  uint8_t op = 0x13; /* VarPackageOp */
>>  
>> -build_append_int(package, acpi_cpus); /* VarNumElements */
>> -for (i = 0; i < acpi_cpus; i++) {
>> +build_append_int(package,
>> + guest_info->apic_id_limit); /* VarNumElements 
>> */
>> +for (i = 0; i < guest_info->apic_id_limit; i++) {
>>  uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
>>  build_append_byte(package, b);
>>  }
>> -- 
>> 1.8.3.1

Re: [Qemu-devel] [RFC PATCH 6/7] hw/arm/virt: Use PSCI v0.2 function IDs when kernel supports its

[Mark Rutland]

On Fri, Mar 14, 2014 at 05:10:34PM +, Rob Herring wrote:
> On Fri, Mar 14, 2014 at 5:23 AM, Mark Rutland  wrote:
> > On Fri, Mar 14, 2014 at 06:53:53AM +, Pranavkumar Sawargaonkar wrote:
> >> Hi Christoffer,
> >>
> >> On 14 March 2014 09:19, Christoffer Dall  
> >> wrote:
> >> > On Thu, Feb 27, 2014 at 12:21:07PM +0530, Pranavkumar Sawargaonkar wrote:
> >> >> If we have in-kernel emulation of PSCI v0.2 for KVM ARM/ARM64 then
> >> >> we enable PSCI v0.2 for each VCPU at the time of VCPU init hence we
> >> >> need to provide PSCI v0.2 function IDs via generated DTB.
> >> >>
> >> >> This patch updates generated DTB to have PSCI v0.2 function IDs when
> >> >> we have in-kernel emulation PSCI v0.2 for KVM ARM/ARM64.
> >> >>
> >> >> Signed-off-by: Pranavkumar Sawargaonkar 
> >> >> Signed-off-by: Anup Patel 
> >> >> ---
> >> >>  hw/arm/virt.c |   25 -
> >> >>  1 file changed, 20 insertions(+), 5 deletions(-)
> >> >>
> >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> >> index 517f2fe..a818a80 100644
> >> >> --- a/hw/arm/virt.c
> >> >> +++ b/hw/arm/virt.c
> >> >> @@ -187,11 +187,26 @@ static void create_fdt(VirtBoardInfo *vbi)
> >> >>  qemu_fdt_add_subnode(fdt, "/psci");
> >> >>  qemu_fdt_setprop_string(fdt, "/psci", "compatible", 
> >> >> "arm,psci");
> >> >
> >> > was there a decision on the format of the psci 0.2 bindings?
> >> >
> >> > I seem to recall that we should add arm,psci-0.2 or something like that.
> >>
> >> Yes there was a discussion related to that by Mark Rutland and Rob Herring 
> >> :
> >> http://www.spinics.net/lists/arm-kernel/msg298509.html
> >>
> >> But there is no dt binding added related to psci 0.2 in kernel (I am
> >> not sure about final conclusion of the dt binding to be added). If the
> >> dt binding gets finalized I will definitely revise this patch.
> >> For now I have added old binding only to test the rfc patch (may now
> >> be the right way to do but do not have any option also).
> >
> > I believe Rob and I were happy with PSCI 0.2 IDs being implicit, though
> > for compatibility with existing kernels the IDs in the "arm,psci"
> > binding might also be listed.
> >
> > The only issue was Calxeda highbank/midway systems using pre-release
> > PSCI 0.2 IDs for functions not in the "arm,psci" binding. For those we
> > could allocate a compatilbe string like "calxeda,highbank-psci-0.2" and
> > allow them to be implicit, or just leave them in their current
> > (unsupported) state.
> >
> > Rob, thoughts?
> 
> We need to add "arm,psci-0.2" compatible since "arm,psci" defines all
> the function IDs as required and with 0.2 they are optional.
> 
> For highbank, since firmware and DTB is essentially frozen now, a
> compatible change is not going to happen. It probably means just
> leaving things as is.

Ok.

> 
> For reference, this is what highbank/midway's binding looks like:
> 
> psci {
> compatible  = "arm,psci";
> method  = "smc";
> cpu_suspend = <0x8402>;
> cpu_off = <0x8404>;
> cpu_on  = <0x8406>;
> system_off  = <0x84000100>;
> system_reset= <0x84000101>;
> };
> 
> I suppose I could just ignore the binding and make highbank reset/off
> functions do custom some PSCI calls. This would mean exposing
> invoke_psci_fn or creating some custom call wrapper.

Is there any good way to tell if a machine is highbank or midway (e.g.
the top-level compatible string)?

We could check for that and if so read the additional values from the
dt. That way highbank and midway get supported but new machines would
still have to follow the IDs from the final release.

This doesn't necessarily have to be documented as an official binding,
it's essentially a workaround for a quirk (though one with a good reason
for existing).

Does that sound reasonable?

Cheers,
Mark.

Re: [Qemu-devel] [PATCH] qcow2: Fix fail path in realloc_refcount_block()

[Laszlo Ersek]

On 03/16/14 00:26, Benoît Canet wrote:
> The Saturday 15 Mar 2014 à 21:55:54 (+0100), Max Reitz wrote :
>> If qcow2_alloc_clusters() fails, new_offset and ret will both be
>> negative after the fail label, thus passing the first if condition and
>> subsequently resulting in a call of qcow2_free_clusters() with an
>> invalid (negative) offset parameter. Fix this by checking for new_offset
>> being positive instead.
>>
>> While we're at it, clean up the whole fail path. qcow2_cache_put()
>> actually can never fail, hence the return value can safely be ignored
>> (aside from asserting that it indeed did not fail).
>>
>> Furthermore, there is no reason to give QCOW2_DISCARD_ALWAYS to
>> qcow2_free_clusters(), a mere QCOW2_DISCARD_OTHER will suffice.
>>
>> Signed-off-by: Max Reitz 
>> Suggested-by: Laszlo Ersek 
>> ---
>>  block/qcow2-refcount.c | 20 +++-
>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 6151148..b111319 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1440,20 +1440,22 @@ static int64_t 
>> realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>>  }
>>  
>>  fail:
>> -if (new_offset && (ret < 0)) {
>> -qcow2_free_clusters(bs, new_offset, s->cluster_size,
>> -QCOW2_DISCARD_ALWAYS);
>> -}
>>  if (refcount_block) {
>> -if (ret < 0) {
>> -qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
>> -} else {
>> -ret = qcow2_cache_put(bs, s->refcount_block_cache, 
>> &refcount_block);
>> -}
>> +/* This should never fail, as it would only do so if the given 
>> refcount
>> + * block cannot be found in the cache. As this is impossible as 
>> long as
>> + * there are no bugs, assert the success. */
>> +int tmp = qcow2_cache_put(bs, s->refcount_block_cache, 
>> &refcount_block);
>> +assert(tmp == 0);
>>  }
>> +
>>  if (ret < 0) {
>> +if (new_offset > 0) {
>> +qcow2_free_clusters(bs, new_offset, s->cluster_size,
>> +QCOW2_DISCARD_OTHER);
>> +}
>>  return ret;
>>  }
>> +
>>  return new_offset;
>>  }
>>  
>> -- 
>> 1.9.0
>>
>>
> 
> In fact in wonder if these if after the fail label could be replaced by a 
> proper
> goto chain.

That was my first idea too. I didn't propose it to Max because it's
complicated by the fact that while you need to release the cache entry
in any case (both success & failure), you roll back the cluster alloc
only in case of failure. Using a proper goto chain (without any "if"s)
means that you'd have to keep it fully separate from the success path,
hence duplicate the cache entry release on the success path (including
the assert()). The ifs above allow you to share the action.

It's a matter of taste; I usually like to code actions only once. But I
don't insist of course.

Laszlo

Re: [Qemu-devel] [PATCH v7] vl.c: Output error on invalid machine type

[Paolo Bonzini]

Il 17/03/2014 09:04, Markus Armbruster ha scritto:

mreza...@redhat.com writes:


From: Miroslav Rezanina 

Output error message using qemu's error_report() function when user
provides the invalid machine type on the command line. This also saves
time to find what issue is when you downgrade from one version of qemu
to another that doesn't support required machine type yet (the version
user downgraded to have to have this patch applied too, of course).

Signed-off-by: Miroslav Rezanina 
---
v7:
 - use -machine instead of -M in error help message
 - rebase to commit 0056ae2

v6:
 - print help instead of list supported machines on error
---
 vl.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/vl.c b/vl.c
index 862cf20..cbd1381 100644
--- a/vl.c
+++ b/vl.c
@@ -2649,15 +2649,20 @@ static MachineClass *machine_parse(const char *name)
 if (mc) {
 return mc;
 }
-printf("Supported machines are:\n");
-for (el = machines; el; el = el->next) {
-MachineClass *mc = el->data;
-QEMUMachine *m = mc->qemu_machine;
-if (m->alias) {
-printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
+if (name && !is_help_option(name)) {
+error_report("Unsupported machine type");
+printf("\nUse -machine help to list supported machines!\n");


Why the newline before 'Use'?

Recommend to write the hint to the same fd as the error message.  An
obvious way to do that is error_printf(), and it's what we do elsewhere.

Both missed this in my review of v1, sorry.


I'm collecting a few patches for 2.0-rc1, so I fixed this printf and 
applied it.


Paolo


+} else {
+printf("Supported machines are:\n");
+for (el = machines; el; el = el->next) {
+MachineClass *mc = el->data;
+QEMUMachine *m = mc->qemu_machine;
+if (m->alias) {
+printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
+}
+printf("%-20s %s%s\n", m->name, m->desc,
+   m->is_default ? " (default)" : "");
 }
-printf("%-20s %s%s\n", m->name, m->desc,
-   m->is_default ? " (default)" : "");
 }

 g_slist_free(machines);


The functions logic is a bit tortured (not your fault).  Here's how I'd
clean it up:

static MachineClass *machine_parse(const char *name)
{
MachineClass *mc;
GSList *el, *machines;

if (is_help_option(name)) {
machines = object_class_get_list(TYPE_MACHINE, false);

printf("Supported machines are:\n");
for (el = machines; el; el = el->next) {
MachineClass *mc = el->data;
QEMUMachine *m = mc->qemu_machine;
if (m->alias) {
printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
}
printf("%-20s %s%s\n", m->name, m->desc,
   m->is_default ? " (default)" : "");
}

g_slist_free(machines);
exit(0);
}

mc = find_machine(name);
if (!mc) {
error_report("Unsupported machine type");
error_printf("Use '-machine help' to list supported machines\n");
exit(1);
}
return mc;
}

Re: [Qemu-devel] [PATCH v2] update names in option tables to match with actual command-line spelling

[Paolo Bonzini]

Il 17/03/2014 09:23, Markus Armbruster ha scritto:

> This patch makes all names in option table to match with actual
> command-line spelling, it will be helpful when we search in option
> tables.

As discussed in [*], the QemuOptsList member name values are ABI:
changing them can break existing -readconfig configuration files.  If we
decide breaking ABI is okay here (big if!), we need to document it
prominently in the commit message.


I think in some (rare) cases breaking the rule is okay.  For example, 
the pending conversion of "-m" to QemuOpts uses "memory".


However, I don't think adding "-opts" is a good thing to do.  Which one 
is most readable?


   [m]
  size = 128M
  max = 512M

   [memory]
  size = 128M
  max = 512M

   [memory-opts]
  size = 128M
  max = 512M

I'm for including this patch in 2.0.

Paolo

Re: [Qemu-devel] [PATCH 3/4] virtio-gpu: v0.3 of the virtio based GPU code.

[Michael S. Tsirkin]

On Mon, Mar 17, 2014 at 02:36:37PM +1000, Dave Airlie wrote:
> On Thu, Mar 13, 2014 at 8:40 PM, Paolo Bonzini  wrote:
> > Il 12/03/2014 21:26, Michael S. Tsirkin ha scritto:
> >>>
> >>> +Event queue:
> >>> +The only current event passed is a message to denote the host
> >>> +wants to update the layout of the screens. It contains the same
> >>> +info as the response to VIRTGPU_CMD_GET_DISPLAY_INFO.
> >
> >
> > I wonder if an event queue is the best mechanism if you can get the same
> > info from a command anyway.  For virtio-scsi I used a queue because I needed
> > to specify which target or LUN the event applied to, but here you do not
> > need it and a queue is susceptible to dropped events.
> >
> > Perhaps a configuration field is better, like this:
> >
> > u32 events_read;
> > u32 events_clear;
> >
> > A new event sets a bit in events_read and generates a configuration change
> > interrupt.  The guest should never write to events_read.
> >
> > Writing to events_clear has the side effect of the device doing "events_read
> > &= ~events_clear".  We cannot have R/W1C fields in virtio, but this
> > approximation is good enough.
> >
> > When the guest receives a configuration change interrupt, it reads
> > event_read.  If it is nonzero, it writes the same value it read to
> > events_clear, and sends the necessary commands to the card in order to
> > retrieve the event data.  It can then read again event_read, and loop if it
> > is again nonzero.
> >
> 
> I steered away from using config space for anything for the normal
> operation of the GPU after looking at overheads and hearing from S390
> people that config space has some special properties on their hw,
> 
> The number of these events should be small in a running system, and
> I'm not sure how we'd lose one.
> 
> Dave.

I think Paolo refers to a case where these events
arrive faster than guest can handle them.

As long as there's a single event like this,
it seems possible to avoid such under-runs by guest,
by first making a new buffer available, then processing
a used buffer.

If we want this extendable to multiple events,
make guest prepend an out buffer with a header, saying what kind of
event is expected in a given buffer.

Yes, config space is slow and should be avoided for
normal operation. I'm not sure how common this
specific operation is. For reporting guest errors
config space has some advantages as it can
be read even if driver/device communication
is completely wedged.

-- 
MST

[Qemu-devel] [PATCH] target-alpha: fix the braces

[Paolo Bonzini]

Conform to coding style, and avoid further occurrences of bugs due to
misplaced braces.

Signed-off-by: Paolo Bonzini 
---
 target-alpha/translate.c | 313 +++
 1 file changed, 180 insertions(+), 133 deletions(-)

diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index e7e319b..29dffb7 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -100,8 +100,9 @@ void alpha_translate_init(void)
 char *p;
 static int done_init = 0;
 
-if (done_init)
+if (done_init) {
 return;
+}
 
 cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
 
@@ -1117,8 +1118,9 @@ static inline uint64_t zapnot_mask(uint8_t lit)
 int i;
 
 for (i = 0; i < 8; ++i) {
-if ((lit >> i) & 1)
+if ((lit >> i) & 1) {
 mask |= 0xffull << (i * 8);
+}
 }
 return mask;
 }
@@ -1152,26 +1154,28 @@ static void gen_zapnoti(TCGv dest, TCGv src, uint8_t 
lit)
 
 static inline void gen_zapnot(int ra, int rb, int rc, int islit, uint8_t lit)
 {
-if (unlikely(rc == 31))
+if (unlikely(rc == 31)) {
 return;
-else if (unlikely(ra == 31))
+} else if (unlikely(ra == 31)) {
 tcg_gen_movi_i64(cpu_ir[rc], 0);
-else if (islit)
+} else if (islit) {
 gen_zapnoti(cpu_ir[rc], cpu_ir[ra], lit);
-else
+} else {
 gen_helper_zapnot (cpu_ir[rc], cpu_ir[ra], cpu_ir[rb]);
+}
 }
 
 static inline void gen_zap(int ra, int rb, int rc, int islit, uint8_t lit)
 {
-if (unlikely(rc == 31))
+if (unlikely(rc == 31)) {
 return;
-else if (unlikely(ra == 31))
+} else if (unlikely(ra == 31)) {
 tcg_gen_movi_i64(cpu_ir[rc], 0);
-else if (islit)
+} else if (islit) {
 gen_zapnoti(cpu_ir[rc], cpu_ir[ra], ~lit);
-else
+} else {
 gen_helper_zap (cpu_ir[rc], cpu_ir[ra], cpu_ir[rb]);
+}
 }
 
 
@@ -1179,11 +1183,11 @@ static inline void gen_zap(int ra, int rb, int rc, int 
islit, uint8_t lit)
 static void gen_ext_h(int ra, int rb, int rc, int islit,
   uint8_t lit, uint8_t byte_mask)
 {
-if (unlikely(rc == 31))
+if (unlikely(rc == 31)) {
 return;
-else if (unlikely(ra == 31))
+} else if (unlikely(ra == 31)) {
 tcg_gen_movi_i64(cpu_ir[rc], 0);
-else {
+} else {
 if (islit) {
 lit = (64 - (lit & 7) * 8) & 0x3f;
 tcg_gen_shli_i64(cpu_ir[rc], cpu_ir[ra], lit);
@@ -1204,11 +1208,11 @@ static void gen_ext_h(int ra, int rb, int rc, int islit,
 static void gen_ext_l(int ra, int rb, int rc, int islit,
   uint8_t lit, uint8_t byte_mask)
 {
-if (unlikely(rc == 31))
+if (unlikely(rc == 31)) {
 return;
-else if (unlikely(ra == 31))
+} else if (unlikely(ra == 31)) {
 tcg_gen_movi_i64(cpu_ir[rc], 0);
-else {
+} else {
 if (islit) {
 tcg_gen_shri_i64(cpu_ir[rc], cpu_ir[ra], (lit & 7) * 8);
 } else {
@@ -1226,11 +1230,11 @@ static void gen_ext_l(int ra, int rb, int rc, int islit,
 static void gen_ins_h(int ra, int rb, int rc, int islit,
   uint8_t lit, uint8_t byte_mask)
 {
-if (unlikely(rc == 31))
+if (unlikely(rc == 31)) {
 return;
-else if (unlikely(ra == 31) || (islit && (lit & 7) == 0))
+} else if (unlikely(ra == 31) || (islit && (lit & 7) == 0)) {
 tcg_gen_movi_i64(cpu_ir[rc], 0);
-else {
+} else {
 TCGv tmp = tcg_temp_new();
 
 /* The instruction description has us left-shift the byte mask
@@ -1268,11 +1272,11 @@ static void gen_ins_h(int ra, int rb, int rc, int islit,
 static void gen_ins_l(int ra, int rb, int rc, int islit,
   uint8_t lit, uint8_t byte_mask)
 {
-if (unlikely(rc == 31))
+if (unlikely(rc == 31)) {
 return;
-else if (unlikely(ra == 31))
+} else if (unlikely(ra == 31)) {
 tcg_gen_movi_i64(cpu_ir[rc], 0);
-else {
+} else {
 TCGv tmp = tcg_temp_new();
 
 /* The instruction description has us left-shift the byte mask
@@ -1298,11 +1302,11 @@ static void gen_ins_l(int ra, int rb, int rc, int islit,
 static void gen_msk_h(int ra, int rb, int rc, int islit,
   uint8_t lit, uint8_t byte_mask)
 {
-if (unlikely(rc == 31))
+if (unlikely(rc == 31)) {
 return;
-else if (unlikely(ra == 31))
+} else if (unlikely(ra == 31)) {
 tcg_gen_movi_i64(cpu_ir[rc], 0);
-else if (islit) {
+} else if (islit) {
 gen_zapnoti (cpu_ir[rc], cpu_ir[ra], ~((byte_mask << (lit & 7)) >> 8));
 } else {
 TCGv shift = tcg_temp_new();
@@ -1336,11 +1340,11 @@ static void gen_msk_h(int ra, int rb, int rc, int islit,
 static void gen_msk_l(int ra, int rb, int rc, int islit,
   uint8_t lit, uint8_t byte_mask)
 {
-if (unlikely(rc == 31))
+if (unlikely(rc == 31)) {
 return;
-else if (unlikely(ra == 31))
+} els

Re: [Qemu-devel] [PATCH for-2.1 2/2] i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF

[Michael S. Tsirkin]

On Mon, Mar 17, 2014 at 11:08:46AM +0100, Laszlo Ersek wrote:
> On 03/16/14 13:31, Michael S. Tsirkin wrote:
> > On Fri, Mar 14, 2014 at 11:22:52PM +0100, Laszlo Ersek wrote:
> >> Building on the previous patch, raise the maximal count of processor
> >> objects / NTFY branches / CPON elements from 255 to 256. This allows the
> >> VCPU with APIC ID 0xFF to be hotplugged.
> >>
> >> Signed-off-by: Laszlo Ersek 
> >> ---
> >>  hw/i386/acpi-build.c | 16 +++-
> >>  1 file changed, 11 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 2bbefb5..51162fc 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -999,11 +999,15 @@ build_ssdt(GArray *table_data, GArray *linker,
> >> AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
> >> PcPciInfo *pci, PcGuestInfo *guest_info)
> >>  {
> >> -int acpi_cpus = MIN(0xff, guest_info->apic_id_limit);
> > 
> > Maybe just make this line
> >  int acpi_cpus = guest_info->apic_id_limit;
> > and then the patch will be smaller.
> 
> I did think of that, but I didn't like the unchecked unsigned --> int
> conversion. The limit checks below cover that too.

Make it
unsigned acpi_cpus
then?

> Also, the patch renders the function similar to the other acpi builder
> functions; there are a handful of loops that directly name
> "guest_info->apic_id_limit" in their controlling expressions.

I'm fine with refactoring this if you feel it's
more readable (I don't bit I wrote this so I know I'm biased),
but let's make it a separate patch then.
 
> Anyway, would you be OK with a patch that did the assignment you suggest
> (rather than modifying the loops), and kept the BUILD_BUG and the
> g_assert below?
> 
> Thanks
> Laszlo

Yes, fine.


> > 
> >>  int ssdt_start = table_data->len;
> >>  uint8_t *ssdt_ptr;
> >>  int i;
> >>  
> >> +/* The current AML generator can cover the APIC ID range [0..255],
> >> + * inclusive, for VCPU hotplug. */
> >> +QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
> >> +g_assert(guest_info->apic_id_limit <= ACPI_CPU_HOTPLUG_ID_LIMIT);
> >> +
> >>  /* Copy header and patch values in the S3_ / S4_ / S5_ packages */
> >>  ssdt_ptr = acpi_data_push(table_data, sizeof(ssdp_misc_aml));
> >>  memcpy(ssdt_ptr, ssdp_misc_aml, sizeof(ssdp_misc_aml));
> >> @@ -1029,7 +1033,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>  build_append_nameseg(sb_scope, "_SB_");
> >>  
> >>  /* build Processor object for each processor */
> >> -for (i = 0; i < acpi_cpus; i++) {
> >> +for (i = 0; i < guest_info->apic_id_limit; i++) {
> >>  uint8_t *proc = acpi_data_push(sb_scope, ACPI_PROC_SIZEOF);
> >>  memcpy(proc, ACPI_PROC_AML, ACPI_PROC_SIZEOF);
> >>  proc[ACPI_PROC_OFFSET_CPUHEX] = acpi_get_hex(i >> 4);
> >> @@ -1042,7 +1046,8 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>   *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, 
> >> Arg1)} ...}
> >>   */
> >>  /* Arg0 = Processor ID = APIC ID */
> >> -build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", 
> >> acpi_cpus);
> >> +build_append_notify_method(sb_scope, "NTFY", "CP%0.02X",
> >> +   guest_info->apic_id_limit);
> >>  
> >>  /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... 
> >> })" */
> >>  build_append_byte(sb_scope, 0x08); /* NameOp */
> >> @@ -1052,8 +1057,9 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>  GArray *package = build_alloc_array();
> >>  uint8_t op = 0x13; /* VarPackageOp */
> >>  
> >> -build_append_int(package, acpi_cpus); /* VarNumElements */
> >> -for (i = 0; i < acpi_cpus; i++) {
> >> +build_append_int(package,
> >> + guest_info->apic_id_limit); /* 
> >> VarNumElements */
> >> +for (i = 0; i < guest_info->apic_id_limit; i++) {
> >>  uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
> >>  build_append_byte(package, b);
> >>  }
> >> -- 
> >> 1.8.3.1

Re: [Qemu-devel] [PATCH] linux-user: Implement capget, capset

[Riku Voipio]

On Fri, Mar 14, 2014 at 06:10:50PM +, Peter Maydell wrote:
> Implement the capget and capset syscalls. This is useful because
> simple programs like 'ls' try to use it in AArch64

I'm not seing this with ubuntu trusty, wookeys debian or my
static busybox. Where is your ls from? Also, runnning qemu-linux
user as root? How very brave :)

> , and otherwise
> we emit a lot of noise about it being unimplemented.

Well, it seems gcc 4.8 isn't smart enough for this patch:

linux-user/syscall.c: In function ‘do_syscall’:
linux-user/syscall.c:7739:46: error: ‘target_data’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   target_data[i].effective = tswap32(data[i].effective);
^
> Signed-off-by: Peter Maydell 
> ---
> Bugfix or feature? You decide :-)

perhaps unimplemented_nowarn for now and a proper implementation for 2.1 ?

Riku

>  linux-user/syscall.c  | 71 
> +--
>  linux-user/syscall_defs.h | 11 
>  2 files changed, 80 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 2a8b66c..53c3d69 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -43,6 +43,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

This is from libcap-dev, which might not be installed by default. The
actual capset/capget functions seems to be in libc.

>  #include 
>  #include 
>  #ifdef __ia64__
> @@ -7641,9 +7642,75 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>  unlock_user(p, arg1, ret);
>  break;
>  case TARGET_NR_capget:
> -goto unimplemented;
>  case TARGET_NR_capset:
> -goto unimplemented;
> +{
> +struct target_user_cap_header *target_header;
> +struct target_user_cap_data *target_data;
> +struct __user_cap_header_struct header;
> +struct __user_cap_data_struct data[2];
> +struct __user_cap_data_struct *dataptr = NULL;
> +int i, target_datalen;
> +int data_items = 1;
> +
> +if (!lock_user_struct(VERIFY_WRITE, target_header, arg1, 1)) {
> +goto efault;
> +}
> +header.version = tswap32(target_header->version);
> +header.pid = tswap32(target_header->pid);
> +
> +if (header.version != _LINUX_CAPABILITY_VERSION_1) {
> +/* Version 2 and up takes pointer to two user_data structs */
> +data_items = 2;
> +}
> +
> +target_datalen = sizeof(*target_data) * data_items;
> +
> +if (arg2) {
> +if (num == TARGET_NR_capget) {
> +target_data = lock_user(VERIFY_WRITE, arg2, target_datalen, 
> 0);
> +} else {
> +target_data = lock_user(VERIFY_READ, arg2, target_datalen, 
> 1);
> +}
> +if (!target_data) {
> +unlock_user_struct(target_header, arg1, 0);
> +goto efault;
> +}
> +
> +if (num == TARGET_NR_capset) {
> +for (i = 0; i < data_items; i++) {
> +data[i].effective = tswap32(target_data[i].effective);
> +data[i].permitted = tswap32(target_data[i].permitted);
> +data[i].inheritable = 
> tswap32(target_data[i].inheritable);
> +}
> +}
> +
> +dataptr = data;
> +}
> +
> +if (num == TARGET_NR_capget) {
> +ret = get_errno(capget(&header, dataptr));
> +} else {
> +ret = get_errno(capset(&header, dataptr));
> +}
> +
> +/* The kernel always updates version for both capget and capset */
> +target_header->version = tswap32(header.version);
> +unlock_user_struct(target_header, arg1, 1);
> +
> +if (arg2) {
> +if (num == TARGET_NR_capget) {
> +for (i = 0; i < data_items; i++) {
> +target_data[i].effective = tswap32(data[i].effective);
> +target_data[i].permitted = tswap32(data[i].permitted);
> +target_data[i].inheritable = 
> tswap32(data[i].inheritable);
> +}
> +unlock_user(target_data, arg2, target_datalen);
> +} else {
> +unlock_user(target_data, arg2, 0);
> +}
> +}
> +break;
> +}
>  case TARGET_NR_sigaltstack:
>  #if defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_MIPS) || \
>  defined(TARGET_SPARC) || defined(TARGET_PPC) || defined(TARGET_ALPHA) || 
> \
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 732c9e3..7db878a 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2559,3 +2559,14 @@ struct target_sigevent {
>  } _sigev_thread;
>  } _sigev_un;
>  };
> +
> +struct target_user_cap_header {
> +uint32_t version;
> +int pid;
> +};
> +
> +struct target_user_cap_

[Qemu-devel] [PATCH] linux-user: don't warn on missing capget/capset

[riku . voipio]

From: Riku Voipio 

some people get numerous unimplemented capget/capset warnings. Since qemu
linux-user is not secure to begin with, just skip the system call
warning for now. Proper capset/capget to be added in Qemu 2.1

Signed-off-by: Riku Voipio 
---
 linux-user/syscall.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e404a32..4bfa3db 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7677,9 +7677,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 unlock_user(p, arg1, ret);
 break;
 case TARGET_NR_capget:
-goto unimplemented;
+goto unimplemented_nowarn;
 case TARGET_NR_capset:
-goto unimplemented;
+goto unimplemented_nowarn;
 case TARGET_NR_sigaltstack:
 #if defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_MIPS) || \
 defined(TARGET_SPARC) || defined(TARGET_PPC) || defined(TARGET_ALPHA) || \
-- 
1.8.5.3

Re: [Qemu-devel] different IDTs of the same VCPU

[Alexander Binun]

Dear friends, great thanks!

To summarize: we are trying to monitor VCPU IDT changes that are done by 
external parties (e.g. rootkits) and not by intra-KVM machinery. Are there 
parameters that witness such changes ?

Best Regards, 
   The KVM Israeli team


On Thu 13 Mar 17:15 2014 Paolo Bonzini wrote:
> Il 13/03/2014 13:59, Alexander Binun ha scritto:
> > Dear Friends,
> >
> >Thanks for your assistance!
> >
> > We would like to ask you a question about the KVM internals.
> >
> > Our module includes a timer which (once in every second) fetches the IDT 
> > value of every online VCPU in the system using the kvm_x86_ops->get_idt ; 
> > the code looks like:
> >
> >   struct kvm_vcpu *curr_vcpu;
> >   struct desc_ptr dt;
> >
> >   list_for_each_entry(kvm, vms_list, vm_list)
> >   {
> > for (i = 0; i < kvm->online_vcpus.counter; i++)
> >{
> >curr_vcpu = kvm->vcpus[i];
> >kvm_x86_ops->get_idt(curr_vcpu, &dt);
> > }
> >   }
> >
> > We have noticed that get_idt returns DIFFERENT values for the same
> > VCPU (i.e. for the same value of i that refers to a given VCPU). We
> > cannot understand this issue; could you explain ?
> >
> > It is very strange since nobody changes the IDT value (as , for example, 
> > rootkits do).
> 
> At the very least, running nested virtualization would lead to different 
> IDT values.
> 
> But more simply, on Intel you can hardly do anything with kvm_x86_ops or 
> kvm_vcpu except on the same physical CPU that is in vcpu->cpu.  The 
> state is not in memory, it is cached inside the physical CPU.
> 
> There is no easy solution to this without modifying KVM.  You can add a 
> request bit to KVM's vcpu->requests field, kick the vcpu and do the 
> check in vcpu_enter_guest.
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH v2] Makefile: Fix "make clean"

[Peter Maydell]

On 17 March 2014 06:15, Stefan Weil  wrote:
> Am 17.03.2014 02:35, schrieb Fam Zheng:
>> This fixes a dangerous bug: "make clean" after "make distclean" will
>> delete every single file including those under .git, if you do in-tree
>> build!
>>
>> Rationale: A first "make distclean" will unset $(DSOSUF), a following
>> "make distclean" or "make clean" will find all the files and delete it.
>>
>> Fix it by explicitly typing the file extensions here, and combine
>> multiple find invocations into one.
>>
>> Signed-off-by: Fam Zheng 

> Reviewed-by: Stefan Weil 
>
> Hello Peter,
>
> this is a bugfix which fixes a potential danger for a developer's
> working directory, so I think it should be fixed directly (not via
> qemu-trivial).

Agreed; applied to master, thanks.

-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Implement capget, capset

[Peter Maydell]

On 17 March 2014 11:51, Riku Voipio  wrote:
> On Fri, Mar 14, 2014 at 06:10:50PM +, Peter Maydell wrote:
>> Implement the capget and capset syscalls. This is useful because
>> simple programs like 'ls' try to use it in AArch64
>
> I'm not seing this with ubuntu trusty, wookeys debian or my
> static busybox. Where is your ls from?

It's from the SuSE rootfs tarball. (The SuSE patchset
wires capget up to nowarn.

> Also, runnning qemu-linux
> user as root? How very brave :)
>
>> , and otherwise
>> we emit a lot of noise about it being unimplemented.
>
> Well, it seems gcc 4.8 isn't smart enough for this patch:
>
> linux-user/syscall.c: In function ‘do_syscall’:
> linux-user/syscall.c:7739:46: error: ‘target_data’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>target_data[i].effective = tswap32(data[i].effective);
> ^

Hohum. Let's just stick an = NULL on the target_data
definition.

>> Signed-off-by: Peter Maydell 
>> ---
>> Bugfix or feature? You decide :-)
>
> perhaps unimplemented_nowarn for now and a proper implementation for 2.1 ?

I guess that might be safer.

>
> Riku
>
>>  linux-user/syscall.c  | 71 
>> +--
>>  linux-user/syscall_defs.h | 11 
>>  2 files changed, 80 insertions(+), 2 deletions(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 2a8b66c..53c3d69 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -43,6 +43,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>
> This is from libcap-dev, which might not be installed by default. The
> actual capset/capget functions seems to be in libc.

Hmm. Looks like we need to use linux/capability.h and
roll our own capget/capset functions then. (We could
just provide the prototypes for the functions in libc
but since we have a _syscall macro anyway we may as well
use it...)

thanks
-- PMM

[Qemu-devel] [PATCH v2] linux-user: Implement capget, capset

[Peter Maydell]

Implement the capget and capset syscalls. This is useful because
simple programs like 'ls' try to use it in AArch64, and otherwise
we emit a lot of noise about it being unimplemented.

Signed-off-by: Peter Maydell 
---
Changes v1->v2: initialize target_data to NULL to suppress a gcc 4.8
warning; roll our own capget() and capset() rather than relying on
the sys/capability.h header that is in an optional package rather
than libc proper.

 linux-user/syscall.c  | 75 +--
 linux-user/syscall_defs.h | 11 +++
 2 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2a8b66c..49244f8 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #ifdef __ia64__
@@ -243,6 +244,10 @@ _syscall3(int, sys_sched_setaffinity, pid_t, pid, unsigned 
int, len,
   unsigned long *, user_mask_ptr);
 _syscall4(int, reboot, int, magic1, int, magic2, unsigned int, cmd,
   void *, arg);
+_syscall2(int, capget, struct __user_cap_header_struct *, header,
+  struct __user_cap_data_struct *, data);
+_syscall2(int, capset, struct __user_cap_header_struct *, header,
+  struct __user_cap_data_struct *, data);
 
 static bitmask_transtbl fcntl_flags_tbl[] = {
   { TARGET_O_ACCMODE,   TARGET_O_WRONLY,O_ACCMODE,   O_WRONLY,},
@@ -7641,9 +7646,75 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 unlock_user(p, arg1, ret);
 break;
 case TARGET_NR_capget:
-goto unimplemented;
 case TARGET_NR_capset:
-goto unimplemented;
+{
+struct target_user_cap_header *target_header;
+struct target_user_cap_data *target_data = NULL;
+struct __user_cap_header_struct header;
+struct __user_cap_data_struct data[2];
+struct __user_cap_data_struct *dataptr = NULL;
+int i, target_datalen;
+int data_items = 1;
+
+if (!lock_user_struct(VERIFY_WRITE, target_header, arg1, 1)) {
+goto efault;
+}
+header.version = tswap32(target_header->version);
+header.pid = tswap32(target_header->pid);
+
+if (header.version != _LINUX_CAPABILITY_VERSION_1) {
+/* Version 2 and up takes pointer to two user_data structs */
+data_items = 2;
+}
+
+target_datalen = sizeof(*target_data) * data_items;
+
+if (arg2) {
+if (num == TARGET_NR_capget) {
+target_data = lock_user(VERIFY_WRITE, arg2, target_datalen, 0);
+} else {
+target_data = lock_user(VERIFY_READ, arg2, target_datalen, 1);
+}
+if (!target_data) {
+unlock_user_struct(target_header, arg1, 0);
+goto efault;
+}
+
+if (num == TARGET_NR_capset) {
+for (i = 0; i < data_items; i++) {
+data[i].effective = tswap32(target_data[i].effective);
+data[i].permitted = tswap32(target_data[i].permitted);
+data[i].inheritable = tswap32(target_data[i].inheritable);
+}
+}
+
+dataptr = data;
+}
+
+if (num == TARGET_NR_capget) {
+ret = get_errno(capget(&header, dataptr));
+} else {
+ret = get_errno(capset(&header, dataptr));
+}
+
+/* The kernel always updates version for both capget and capset */
+target_header->version = tswap32(header.version);
+unlock_user_struct(target_header, arg1, 1);
+
+if (arg2) {
+if (num == TARGET_NR_capget) {
+for (i = 0; i < data_items; i++) {
+target_data[i].effective = tswap32(data[i].effective);
+target_data[i].permitted = tswap32(data[i].permitted);
+target_data[i].inheritable = tswap32(data[i].inheritable);
+}
+unlock_user(target_data, arg2, target_datalen);
+} else {
+unlock_user(target_data, arg2, 0);
+}
+}
+break;
+}
 case TARGET_NR_sigaltstack:
 #if defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_MIPS) || \
 defined(TARGET_SPARC) || defined(TARGET_PPC) || defined(TARGET_ALPHA) || \
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 732c9e3..7db878a 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -2559,3 +2559,14 @@ struct target_sigevent {
 } _sigev_thread;
 } _sigev_un;
 };
+
+struct target_user_cap_header {
+uint32_t version;
+int pid;
+};
+
+struct target_user_cap_data {
+uint32_t effective;
+uint32_t permitted;
+uint32_t inheritable;
+};
-- 
1.9.0

Re: [Qemu-devel] different IDTs of the same VCPU

[Paolo Bonzini]

Il 17/03/2014 12:54, Alexander Binun ha scritto:

Dear friends, great thanks!

To summarize: we are trying to monitor VCPU IDT changes that are done
by external parties (e.g. rootkits) and not by intra-KVM machinery.
Are there parameters that witness such changes ?


There is no way to intercept changes to the interrupt descriptor table.

You can:

* look at the IDTR values on every vmexit, including before injecting an 
interrupt, but that won't protect from hijacking software interrupts 
such as int $0x80;


* protect the IDT from writing using KVM's page table mechanisms, but 
that won't catch the case when the IDT is changed to a whole new page.


Paolo

[Qemu-devel] [PULL 4/4] vl.c: Output error on invalid machine type

[Paolo Bonzini]

From: Miroslav Rezanina 

Output error message using qemu's error_report() function when user
provides the invalid machine type on the command line. This also saves
time to find what issue is when you downgrade from one version of qemu
to another that doesn't support required machine type yet (the version
user downgraded to have to have this patch applied too, of course).

Signed-off-by: Miroslav Rezanina 
[Replace printf with error_printf, suggested by Markus Armbruster. - Paolo]
Signed-off-by: Paolo Bonzini 
---
 vl.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/vl.c b/vl.c
index 842e897..b363a21 100644
--- a/vl.c
+++ b/vl.c
@@ -2651,15 +2651,20 @@ static MachineClass *machine_parse(const char *name)
 if (mc) {
 return mc;
 }
-printf("Supported machines are:\n");
-for (el = machines; el; el = el->next) {
-MachineClass *mc = el->data;
-QEMUMachine *m = mc->qemu_machine;
-if (m->alias) {
-printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
+if (name && !is_help_option(name)) {
+error_report("Unsupported machine type");
+error_printf("Use -machine help to list supported machines!\n");
+} else {
+printf("Supported machines are:\n");
+for (el = machines; el; el = el->next) {
+MachineClass *mc = el->data;
+QEMUMachine *m = mc->qemu_machine;
+if (m->alias) {
+printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
+}
+printf("%-20s %s%s\n", m->name, m->desc,
+   m->is_default ? " (default)" : "");
 }
-printf("%-20s %s%s\n", m->name, m->desc,
-   m->is_default ? " (default)" : "");
 }
 
 g_slist_free(machines);
-- 
1.8.5.3

[Qemu-devel] [PULL 0/4] Misc fixes for 2.0-rc1

[Paolo Bonzini]

Anthony, Peter,

The following changes since commit f4b11eee2f562c23b3efc33b96ba4542c9ca81aa:

  Makefile: Fix "make clean" (2014-03-17 11:50:19 +)

are available in the git repository at:

  git://github.com/bonzini/qemu.git fixes-for-2.0

for you to fetch changes up to 025172d56e11ba3d86d0937933a23aab3b8606b1:

  vl.c: Output error on invalid machine type (2014-03-17 13:21:12 +0100)

I gathered miscellaneous fixes from the mailing list, two of them mine,
that are suitable for hard freeze.  (I originarily had Fam's other patch
in here too).


Fam Zheng (1):
  rules.mak: Fix per object libs extraction

Miroslav Rezanina (1):
  vl.c: Output error on invalid machine type

Paolo Bonzini (2):
  qemu-nbd: Fix coverity issues
  target-alpha: fix subl and s8subl indentation

 qemu-nbd.c   | 17 +
 rules.mak|  4 ++--
 target-alpha/translate.c |  3 ++-
 vl.c | 21 +
 4 files changed, 30 insertions(+), 15 deletions(-)
-- 
1.8.5.3

[Qemu-devel] [PULL 1/4] rules.mak: Fix per object libs extraction

[Paolo Bonzini]

From: Fam Zheng 

Don't sort the extracted options, sort the objects.

Reported-by: Christian Mahnke 
Signed-off-by: Fam Zheng 
Signed-off-by: Paolo Bonzini 
---
 rules.mak | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rules.mak b/rules.mak
index 9dda9f7..5c454d8 100644
--- a/rules.mak
+++ b/rules.mak
@@ -23,8 +23,8 @@ QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(*D)/$(*F).d
 QEMU_INCLUDES += -I$(

[Qemu-devel] [PULL 3/4] target-alpha: fix subl and s8subl indentation

[Paolo Bonzini]

Two missing braces, one close and one open, fabulously let the code
compile.

Reviewed-by: Richard Henderson 
Signed-off-by: Paolo Bonzini 
---
 target-alpha/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index a9ef1a7..e7e319b 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -1927,6 +1927,7 @@ static ExitStatus translate_one(DisasContext *ctx, 
uint32_t insn)
 else {
 tcg_gen_neg_i64(cpu_ir[rc], cpu_ir[rb]);
 tcg_gen_ext32s_i64(cpu_ir[rc], cpu_ir[rc]);
+}
 }
 }
 break;
@@ -1991,7 +1992,7 @@ static ExitStatus translate_one(DisasContext *ctx, 
uint32_t insn)
 } else {
 if (islit)
 tcg_gen_movi_i64(cpu_ir[rc], -lit);
-else
+else {
 tcg_gen_neg_i64(cpu_ir[rc], cpu_ir[rb]);
 tcg_gen_ext32s_i64(cpu_ir[rc], cpu_ir[rc]);
 }
-- 
1.8.5.3

[Qemu-devel] [PULL 2/4] qemu-nbd: Fix coverity issues

[Paolo Bonzini]

There are two issues in qemu-nbd: a missing return value check after
calling accept(), and file descriptor leaks in nbd_client_thread.

Reviewed-by: Markus Armbruster 
Signed-off-by: Paolo Bonzini 
---
 qemu-nbd.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index bdac1f3..899e67c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -288,19 +288,19 @@ static void *nbd_client_thread(void *arg)
 ret = nbd_receive_negotiate(sock, NULL, &nbdflags,
 &size, &blocksize);
 if (ret < 0) {
-goto out;
+goto out_socket;
 }
 
 fd = open(device, O_RDWR);
 if (fd < 0) {
 /* Linux-only, we can use %m in printf.  */
 fprintf(stderr, "Failed to open %s: %m", device);
-goto out;
+goto out_socket;
 }
 
 ret = nbd_init(fd, sock, nbdflags, size, blocksize);
 if (ret < 0) {
-goto out;
+goto out_fd;
 }
 
 /* update partition table */
@@ -316,12 +316,16 @@ static void *nbd_client_thread(void *arg)
 
 ret = nbd_client(fd);
 if (ret) {
-goto out;
+goto out_fd;
 }
 close(fd);
 kill(getpid(), SIGTERM);
 return (void *) EXIT_SUCCESS;
 
+out_fd:
+close(fd);
+out_socket:
+closesocket(sock);
 out:
 kill(getpid(), SIGTERM);
 return (void *) EXIT_FAILURE;
@@ -355,6 +359,11 @@ static void nbd_accept(void *opaque)
 socklen_t addr_len = sizeof(addr);
 
 int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
+if (fd < 0) {
+perror("accept");
+return;
+}
+
 if (state >= TERMINATE) {
 close(fd);
 return;
-- 
1.8.5.3

Re: [Qemu-devel] Windows XP Setup hangs on Fedora FC20 install

[Cole Robinson]

On 03/16/2014 08:40 AM, Gerhard Wiesinger wrote:
> Hello,
> 
> I tried to install XP SP3 (also tried XP SP2) on a new VM (IDE, 1GB RAM,
> VMVGA, more details can be provided if necessary) on Fedora 20 (Packages
> below), but it hangs on "Installing Windows/Installing Devices".
> 
> Was reproduceable 2 times. Anyone has similar problems?
> 
> Looks to me like a regression problem.
> 
> Thank you.
> 
> Ciao,
> Gerhard
> 
> Details:
> qemu-kvm-1.6.1-3.fc20.x86_64
> kernel: 3.13.6-200.fc20.x86_64
> seabios-bin-1.7.3.1-1.fc20.noarch
> Hardware: Intel
> 
> 

Works fine here but I'm using AMD. Please file a Fedora bug against qemu, and
include:

- full qemu command line (if using libvirt: /var/log/libvirt/qemu/$vmname.log)
- Any qemu stdout/stderr output
- Check dmesg for any kvm errors
- Try booting an older Fedora 20 kernel and see if it makes any difference
- Optionally try a rawhide kernel: sudo yum install fedora-release-rawhide &&
sudo yum --enablrepo=rawhide update kernel

Thanks,
Cole

Re: [Qemu-devel] [Qemu-trivial] [PATCH] serial-pci: Set prog interface field of pci config to 16550 compatible

[Gerd Hoffmann]

> >> C 07  Communication controller
> >>00  Serial controller
> >>00  8250
> >>01  16450
> >>02  16550

> >> What about multi_serial_pci_init() -- should it too set prog-if
> >> like this?
> >
> > I'm not completely sure what the correct value should be for 
> > multi_serial_pci_init so I've left that alone. There's also 07 02 00 that 
> > means multiport serial controller that might apply for that case. For the 
> > simple case I've sent the patch for I had a client expecting this value so 
> > I 
> > think it is correct for that at least but I don't know about the multiport 
> > case.
> 
> Also Gerd Hoffmann said he has picked this patch up so I'm adding him to 
> cc too.

The multiport variants are simply multiple 16550 chips mapped one after
the other.  So "02" is better than "00".

I don't know what exactly the multiport serial controller is supposed to
be.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] update names in option tables to match with actual command-line spelling

[Markus Armbruster]

[cc: Eric yet again :)]

Paolo Bonzini  writes:

> Il 17/03/2014 09:23, Markus Armbruster ha scritto:
>> > This patch makes all names in option table to match with actual
>> > command-line spelling, it will be helpful when we search in option
>> > tables.
>>
>> As discussed in [*], the QemuOptsList member name values are ABI:
>> changing them can break existing -readconfig configuration files.  If we
>> decide breaking ABI is okay here (big if!), we need to document it
>> prominently in the commit message.
>
> I think in some (rare) cases breaking the rule is okay.  For example,
> the pending conversion of "-m" to QemuOpts uses "memory".

Breaking naming consistency rules is one thing, breaking ABI an entirely
different one.

While I'm not overly scared of the ABI change bugaboo, I do insist on
considering the implications, and on proper documentation.

> However, I don't think adding "-opts" is a good thing to do.  Which
> one is most readable?
>
>[m]
>   size = 128M
>   max = 512M
>
>[memory]
>   size = 128M
>   max = 512M
>
>[memory-opts]
>   size = 128M
>   max = 512M

That's a no-brainer :)

I'm all for naming configuration file sections sensibly.  While I value
consistency between section names and command line options, I agree with
you that having sensible section names trumps consistency with the
command line.

In this case, consistency with command line could easily be preserved by
making -m sugar for --memory.

I doubt changing existing section names to make them prettier or more
consistent with the command is worth the ABI breakage.  Any inconsistent
or ugly names that haven't been released yet should be fixed right away,
of course.

This patch changes:

fromto  introduced in
acpiacpitable   0c764a9 v1.5.0
boot-opts   boot3d3b830 v1.0
smp-optssmp 12b7f57 v1.6.0

All three have calcified into ABI already.

> I'm for including this patch in 2.0.

Not without explaining the ABI breakage in the commit message.  We
should also make sure to cover it in the release notes[*].

Moreover, if exceptions from the rule "QemuOptsList name must match the
name of the (non-sugared) command line option using it" are or will be
permissible, then Amos's comment on QemuOptsList member name needs to be
clarified.


[*] Release note material is being collected at
http://wiki.qemu.org/ChangeLog/2.0

Re: [Qemu-devel] [PATCH v4 04/21] target-arm: Provide correct syndrome information for cpreg access traps

[Peter Maydell]

On 17 March 2014 03:05, Peter Crosthwaite  wrote:
> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell  
> wrote:
>> For exceptions taken to AArch64, if a coprocessor/system register
>> access fails due to a trap or enable bit then the syndrome information
>> must include details of the failing instruction (crn/crm/opc1/opc2
>> fields, etc). Make the decoder construct the syndrome information
>> at translate time so it can be passed at runtime to the access-check
>> helper function and used as required.

> Can we space out the constants to a consistent tab stop for readability?

Sure.

>> -void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip)
>> +void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t 
>> syndrome)
>>  {
>>  const ARMCPRegInfo *ri = rip;
>>  switch (ri->accessfn(env, ri)) {
>>  case CP_ACCESS_OK:
>>  return;
>>  case CP_ACCESS_TRAP:
>> +env->exception.syndrome = syndrome;
>
> Can TCG just deposit this directly and unconditionally straight to the
> env to avoid the extra syndrome arg?

Hmm. I think in theory that would be possible, but it seems
to me that it would be pretty confusing if exception.syndrome
could be set for anything other than "we're going to take an
exception and this is it". Passing the syndrome as a function
argument (probably in a register) seems better than always
doing a store to memory, as well. Or am I missing something that
would make it slower?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 05/21] target-arm: Add support for generating exceptions with syndrome information

[Peter Maydell]

On 17 March 2014 03:19, Peter Crosthwaite  wrote:
> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell  
> wrote:
>> -static void gen_exception(int excp)
>> +static void gen_exception_internal(int excp)
>>  {
>> -TCGv_i32 tmp = tcg_temp_new_i32();
>> -tcg_gen_movi_i32(tmp, excp);
>> -gen_helper_exception(cpu_env, tmp);
>> -tcg_temp_free_i32(tmp);
>> +TCGv_i32 tcg_excp = tcg_const_i32(excp);
>> +
>> +assert(excp_is_internal(excp));
>> +gen_helper_exception_internal(cpu_env, tcg_excp);
>> +tcg_temp_free_i32(tcg_excp);
>> +}
>> +
>
> AFAICT this is identical to gen_exception_internal in translate-a64.c.
> Can they be de-static'd and prototyped in internals.h?

This is true, but it would break the current situation we
have where translate.c and translate-a64.c are entirely
independent and you never have to worry about breaking
one if you make changes to the other, which is why I didn't
do it. Maybe that's not very important, but it didn't seem
worth going against for the sake of a couple of helpers just
a few lines long. (If we do make them common then translate.h
would be the right place for the prototypes.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 06/21] target-arm: Provide syndrome information for MMU faults

[Peter Maydell]

On 17 March 2014 03:28, Peter Crosthwaite  wrote:
> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell  
> wrote:
>> From: Rob Herring 

>> +/* Set bit 26 for exceptions with no change in EL */
>> +if (arm_current_pl(env)) {
>> +syn |= 1 << ARM_EL_EC_SHIFT;
>> +}
>> +
>
> Perhaps in internals.h:
>
> #define ARM_EL_EC_SAME_LEVEL (1 << ARM_EL_EC_SHIFT)
>
> Then this becomes:
>
> syn |= ARM_EL_EC_SAME_LEVEL
>
> Then in internals.h you can be more self documenting with:
>
> EC_BREAKPOINT_SAME_EL = EC_BREAKPOINT | ARM_EL_EC_SAME_LEVEL

Yeah, seems reasonable.

-- PMM

Re: [Qemu-devel] [PATCH v4 06/21] target-arm: Provide syndrome information for MMU faults

[Peter Maydell]

On 17 March 2014 12:41, Peter Maydell  wrote:
> On 17 March 2014 03:28, Peter Crosthwaite  
> wrote:
>> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell  
>> wrote:
>>> From: Rob Herring 
>
>>> +/* Set bit 26 for exceptions with no change in EL */
>>> +if (arm_current_pl(env)) {
>>> +syn |= 1 << ARM_EL_EC_SHIFT;
>>> +}
>>> +
>>
>> Perhaps in internals.h:
>>
>> #define ARM_EL_EC_SAME_LEVEL (1 << ARM_EL_EC_SHIFT)
>>
>> Then this becomes:
>>
>> syn |= ARM_EL_EC_SAME_LEVEL
>>
>> Then in internals.h you can be more self documenting with:
>>
>> EC_BREAKPOINT_SAME_EL = EC_BREAKPOINT | ARM_EL_EC_SAME_LEVEL
>
> Yeah, seems reasonable.

On the other hand you can't define EC_BREAKPOINT_SAME_EL
like that, because the EC_ enum values aren't shifted.

Perhaps it would be better to have the syn_* functions for
the EC values which have SAME_EL versions (currently just
insn abort and data abort, since we don't implement any
of the hardware debug exceptions) have an extra parameter
bool same_el, and have the syn_ function OR in the extra bit.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 11/21] target-arm: Don't mention PMU in debug feature register

[Peter Maydell]

On 17 March 2014 05:13, Peter Crosthwaite  wrote:
> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell  
> wrote:
>> Suppress the ID_AA64DFR0_EL1 PMUVer field, even if the CPU specific
>> value claims that it exists. QEMU doesn't currently implement it,
>> and not advertising it prevents the guest from trying to use it
>> and getting UNDEFs on unimplemented registers.
>>
>> Signed-off-by: Peter Maydell 
>
> Reviewed-by: Peter Crosthwaite 
>
>> ---
>> This is arguably a hack, but otherwise Linux tries to prod
>> half a dozen PMU sysregs.
>
> Not really. I think sane self-identification trumps dummy feature
> advertising. Although there is a consistency argument to be made, as
> to whether you should also wipe-out any other features advertised by
> this register and friends (e.g. should TraceVer be set?).

The lack of consistency is what makes it a hack :-) Generally
QEMU takes the approach of "report what the h/w reports even
if we don't implement it all"; "report what we provide even
if that's not the same values as h/w" would be a different
approach, but if we wanted that we'd need to do it consistently.
Still I think pragmatism wins out here.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 13/21] target-arm: Use dedicated CPU state fields for ARM946 access bit registers

[Peter Maydell]

On 17 March 2014 05:20, Peter Crosthwaite  wrote:
> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell  
> wrote:
>>  static const ARMCPRegInfo pmsav5_cp_reginfo[] = {
>>  { .name = "DATA_AP", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0,
>>.access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
>> -  .fieldoffset = offsetof(CPUARMState, cp15.c5_data), .resetvalue = 0,
>> +  .fieldoffset = offsetof(CPUARMState, cp15.pmsav5_data_ap),
>> +  .resetvalue = 0,
>
> I know its just motion of existing code, but what's the policy on zero
> resets? Can we leave them out for brevity? Checkpatch complains when a
> global is explictly 0 initialized, so it seems sane that the same rule
> applies to individual fields (just checkpatch probably has hard time
> figuring this one out).

The semantics allow you to leave out .resetvalue if it is 0; I think
generally when I've written reginfo definitions I've tended to put
in the .resetvalue as an indication of "I know this reginfo needs
a reset value and it is zero" as opposed to "I didn't think about
reset when I wrote this".

We definitely don't want checkpatch to warn about explicit zero
initializers in structs like this, otherwise we wouldn't be
able to say ".crm = 0, .opc1 = 0, .opc2 = 0", which would be
less comprehensible than writing them out explicitly I think.

thanks
-- PMM

[Qemu-devel] [PATCH 3/3] qemu-io: Extended "--cmd" description in usage text

[Maria Kustova]

It's not clear from the usage description that "--cmd" option accepts its 
argument as a string, so any special symbols have to be quoted from the shell.
Updates in usage text:
 - Specified parameter format for "--cmd" option.
 - Added an instruction how to get help for "--cmd" option.

Signed-off-by: Maria Kustova 
---
 qemu-io.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index fc38608..be68d96 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -194,10 +194,11 @@ static const cmdinfo_t quit_cmd = {
 static void usage(const char *name)
 {
 printf(
-"Usage: %s [-h] [-V] [-rsnm] [-c cmd] ... [file]\n"
+"Usage: %s [-h] [-V] [-rsnm] [-c STRING] ... [file]\n"
 "QEMU Disk exerciser\n"
 "\n"
-"  -c, --cmdcommand to execute\n"
+"  -c, --cmd STRING execute command with its arguments\n"
+"   from the given string\n"
 "  -r, --read-only  export read-only\n"
 "  -s, --snapshot   use snapshot file\n"
 "  -n, --nocachedisable host cache\n"
@@ -208,8 +209,10 @@ static void usage(const char *name)
 "  -T, --trace FILE enable trace events listed in the given file\n"
 "  -h, --help   display this help and exit\n"
 "  -V, --versionoutput version information and exit\n"
+"\n"
+"See '%s -c help' for information on available commands."
 "\n",
-name);
+name, name);
 }
 
 static char *get_prompt(void)
-- 
1.8.2.1

[Qemu-devel] [PATCH 2/3] block: Replaced old error handling with error reporting API.

[Maria Kustova]

Signed-off-by: Maria Kustova 
---
 block/curl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index 3494c6d..359637e 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -538,7 +538,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, 
int flags,
 return 0;
 
 out:
-fprintf(stderr, "CURL: Error opening file: %s\n", state->errmsg);
+error_setg(errp, "CURL: Error opening file: %s\n", state->errmsg);
 curl_easy_cleanup(state->curl);
 state->curl = NULL;
 out_noclean:
-- 
1.8.2.1

[Qemu-devel] [PATCH 0/3] Trivial patches

[Maria Kustova]

These patches are the part of OPW application.
Two of them update help messages of qemu-io utility.
And last one replaces fprintf() with error_setg() in curl.c

Maria Kustova (3):
  qemu-io-cmds: Fixed typo in example for writev.
  block: Replaced old error handling with error reporting API.
  qemu-io: Extended "--cmd" description in usage text

 block/curl.c   | 2 +-
 qemu-io-cmds.c | 2 +-
 qemu-io.c  | 9 ++---
 3 files changed, 8 insertions(+), 5 deletions(-)

-- 
1.8.2.1

[Qemu-devel] [PATCH 1/3] qemu-io-cmds: Fixed typo in example for writev.

[Maria Kustova]

Signed-off-by: Maria Kustova 
---
 qemu-io-cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index f1de24c..5707bda 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1087,7 +1087,7 @@ writev_help(void)
 " writes a range of bytes from the given offset source from multiple buffers\n"
 "\n"
 " Example:\n"
-" 'write 512 1k 1k' - writes 2 kilobytes at 512 bytes into the open file\n"
+" 'writev 512 1k 1k' - writes 2 kilobytes at 512 bytes into the open file\n"
 "\n"
 " Writes into a segment of the currently open file, using a buffer\n"
 " filled with a set pattern (0xcdcdcdcd).\n"
-- 
1.8.2.1

Re: [Qemu-devel] [PATCH v4 14/21] target-arm: Implement AArch64 views of fault status and data registers

[Peter Maydell]

On 17 March 2014 05:30, Peter Crosthwaite  wrote:
> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell  
> wrote:
>> @@ -2979,20 +2988,23 @@ void arm_cpu_do_interrupt(CPUState *cs)
>>  env->exception.fsr = 2;
>>  /* Fall through to prefetch abort.  */
>>  case EXCP_PREFETCH_ABORT:
>> -env->cp15.c5_insn = env->exception.fsr;
>> -env->cp15.c6_insn = env->exception.vaddress;
>> +env->cp15.ifsr_el2 = env->exception.fsr;
>> +env->cp15.far_el1 = deposit64(env->cp15.far_el1, 32, 32,
>> +  env->exception.vaddress);
>
> Is it better to just grab the CPRegInfo and pass it to raw_write() to
> do the deposit dirty work?

You'd have to do a hash-table lookup and it would be an odd
case compared to the other registers we update here, so I think
just directly depositing to the state field is simpler.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 11/21] target-arm: Don't mention PMU in debug feature register

[Peter Crosthwaite]

On Mon, Mar 17, 2014 at 10:58 PM, Peter Maydell
 wrote:
> On 17 March 2014 05:13, Peter Crosthwaite  
> wrote:
>> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell  
>> wrote:
>>> Suppress the ID_AA64DFR0_EL1 PMUVer field, even if the CPU specific
>>> value claims that it exists. QEMU doesn't currently implement it,
>>> and not advertising it prevents the guest from trying to use it
>>> and getting UNDEFs on unimplemented registers.
>>>
>>> Signed-off-by: Peter Maydell 
>>
>> Reviewed-by: Peter Crosthwaite 
>>
>>> ---
>>> This is arguably a hack, but otherwise Linux tries to prod
>>> half a dozen PMU sysregs.
>>
>> Not really. I think sane self-identification trumps dummy feature
>> advertising. Although there is a consistency argument to be made, as
>> to whether you should also wipe-out any other features advertised by
>> this register and friends (e.g. should TraceVer be set?).
>
> The lack of consistency is what makes it a hack :-) Generally
> QEMU takes the approach of "report what the h/w reports even
> if we don't implement it all"; "report what we provide even
> if that's not the same values as h/w" would be a different
> approach, but if we wanted that we'd need to do it consistently.

I think there is an argument to decide it case by case ..

> Still I think pragmatism wins out here.
>

In cases where QEMU can validly nop the feature in question (like
caches etc.) then faking up to match real HW is cool. But if a guest
can take a feature advertisment then if() on it to bang on
non-existant hardware causing bus errors or exceptions then I think we
should remove the advertisements even if it is a deviation from real
hw register state. Supporting a good guest that has self identificaton
correct seems more worthwhile than support a guest that somehow
requires a feature advertisment without actually using the feature.

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v4 14/21] target-arm: Implement AArch64 views of fault status and data registers

[Peter Crosthwaite]

On Mon, Mar 17, 2014 at 11:06 PM, Peter Maydell
 wrote:
> On 17 March 2014 05:30, Peter Crosthwaite  
> wrote:
>> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell  
>> wrote:
>>> @@ -2979,20 +2988,23 @@ void arm_cpu_do_interrupt(CPUState *cs)
>>>  env->exception.fsr = 2;
>>>  /* Fall through to prefetch abort.  */
>>>  case EXCP_PREFETCH_ABORT:
>>> -env->cp15.c5_insn = env->exception.fsr;
>>> -env->cp15.c6_insn = env->exception.vaddress;
>>> +env->cp15.ifsr_el2 = env->exception.fsr;
>>> +env->cp15.far_el1 = deposit64(env->cp15.far_el1, 32, 32,
>>> +  env->exception.vaddress);
>>
>> Is it better to just grab the CPRegInfo and pass it to raw_write() to
>> do the deposit dirty work?
>
> You'd have to do a hash-table lookup and it would be an odd
> case compared to the other registers we update here, so I think
> just directly depositing to the state field is simpler.
>

OK fair enough. I was thinking just pass the & of static const
CPRegInfo (if its even visible here) rather than doing a lookup. But
then I spose you have a preformatted CPRegInfo that may or may not be
correct.

FWIW I know you dislike unions, but it would solve this one. In
general its verbose and cumbersome, but I think its applicable to the
ones where you have wierd register sharing policys (like the somewhat
numerically unrealted IFAR and DFAR being high and low words of FAR).

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH 2/3] block: Replaced old error handling with error reporting API.

[Peter Crosthwaite]

On Mon, Mar 17, 2014 at 7:10 AM, Maria Kustova  wrote:
> Signed-off-by: Maria Kustova 
> ---
>  block/curl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 3494c6d..359637e 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -538,7 +538,7 @@ static int curl_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  return 0;
>
>  out:
> -fprintf(stderr, "CURL: Error opening file: %s\n", state->errmsg);
> +error_setg(errp, "CURL: Error opening file: %s\n", state->errmsg);

You should drop \n from error_setg calls.

Regards,
Peter

>  curl_easy_cleanup(state->curl);
>  state->curl = NULL;
>  out_noclean:
> --
> 1.8.2.1
>
>

Re: [Qemu-devel] [PATCH 0/3] Trivial patches

[Peter Crosthwaite]

Hi Maria,

Thanks for the patches. We have a trivial patches lists mailing lists
(that you cc in addition to qemu-devel) that is intended for this kind
of stuff.

CCd.

Regards,
Peter

On Mon, Mar 17, 2014 at 7:10 AM, Maria Kustova  wrote:
> These patches are the part of OPW application.
> Two of them update help messages of qemu-io utility.
> And last one replaces fprintf() with error_setg() in curl.c
>
> Maria Kustova (3):
>   qemu-io-cmds: Fixed typo in example for writev.
>   block: Replaced old error handling with error reporting API.
>   qemu-io: Extended "--cmd" description in usage text
>
>  block/curl.c   | 2 +-
>  qemu-io-cmds.c | 2 +-
>  qemu-io.c  | 9 ++---
>  3 files changed, 8 insertions(+), 5 deletions(-)
>
> --
> 1.8.2.1
>
>

Re: [Qemu-devel] [PULL 0/3] gtk fixes.

[Gerd Hoffmann]

  Hi,

> Doesn't build if CONFIG_GTK isn't defined:
> 
> /root/qemu/vl.c:146:14: error: 'grab_on_hover' defined but not used
> [-Werror=unused-variable]

Oops.  /me wonders how that escaped my attention, my mingw test build
should have hit this.  Updated pull on the way.

sorry for the trouble,
  Gerd

[Qemu-devel] [PULL 3/3] gtk: Don't warp absolute pointer

[Gerd Hoffmann]

From: Cole Robinson 

This matches the behavior of SDL, and makes the mouse usable when
using -display gtk -vga qxl

https://bugzilla.redhat.com/show_bug.cgi?id=1051724
Signed-off-by: Cole Robinson 
Signed-off-by: Gerd Hoffmann 
---
 ui/gtk.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/ui/gtk.c b/ui/gtk.c
index e2394ac..baabf86 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -340,6 +340,10 @@ static void gd_mouse_set(DisplayChangeListener *dcl,
 GdkDeviceManager *mgr;
 gint x_root, y_root;
 
+if (qemu_input_is_absolute()) {
+return;
+}
+
 dpy = gtk_widget_get_display(s->drawing_area);
 mgr = gdk_display_get_device_manager(dpy);
 gdk_window_get_root_coords(gtk_widget_get_window(s->drawing_area),
@@ -355,6 +359,10 @@ static void gd_mouse_set(DisplayChangeListener *dcl,
 GtkDisplayState *s = container_of(dcl, GtkDisplayState, dcl);
 gint x_root, y_root;
 
+if (qemu_input_is_absolute()) {
+return;
+}
+
 gdk_window_get_root_coords(gtk_widget_get_window(s->drawing_area),
x, y, &x_root, &y_root);
 gdk_display_warp_pointer(gtk_widget_get_display(s->drawing_area),
-- 
1.8.3.1

[Qemu-devel] [PULL 1/3] gtk: Allow to activate grab-on-hover from the command line

[Gerd Hoffmann]

From: Jan Kiszka 

As long as we have no persistent GTK configuration, this allows to
enable the useful grab-on-hover feature already when starting the VM.

Signed-off-by: Jan Kiszka 

[ kraxel: fix warning with CONFIG_GTK=n ]

Signed-off-by: Gerd Hoffmann 
---
 include/ui/console.h |  2 +-
 qemu-options.hx  |  5 +
 ui/gtk.c |  5 -
 vl.c | 24 +++-
 4 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 08a38ea..8a86617 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -345,6 +345,6 @@ int index_from_key(const char *key);
 
 /* gtk.c */
 void early_gtk_display_init(void);
-void gtk_display_init(DisplayState *ds, bool full_screen);
+void gtk_display_init(DisplayState *ds, bool full_screen, bool grab_on_hover);
 
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index 068da2d..ee5437b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -810,6 +810,7 @@ ETEXI
 DEF("display", HAS_ARG, QEMU_OPTION_display,
 "-display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]\n"
 "[,window_close=on|off]|curses|none|\n"
+"gtk[,grab_on_hover=on|off]|\n"
 "vnc=[,]\n"
 "select display type\n", QEMU_ARCH_ALL)
 STEXI
@@ -833,6 +834,10 @@ graphics card, but its output will not be displayed to the 
QEMU
 user. This option differs from the -nographic option in that it
 only affects what is done with video output; -nographic also changes
 the destination of the serial and parallel port data.
+@item gtk
+Display video output in a GTK window. This interface provides drop-down
+menus and other UI elements to configure and control the VM during
+runtime.
 @item vnc
 Start a VNC server on display 
 @end table
diff --git a/ui/gtk.c b/ui/gtk.c
index c3ac448..016804d 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1438,7 +1438,7 @@ static const DisplayChangeListenerOps dcl_ops = {
 .dpy_cursor_define = gd_cursor_define,
 };
 
-void gtk_display_init(DisplayState *ds, bool full_screen)
+void gtk_display_init(DisplayState *ds, bool full_screen, bool grab_on_hover)
 {
 GtkDisplayState *s = g_malloc0(sizeof(*s));
 char *filename;
@@ -1517,6 +1517,9 @@ void gtk_display_init(DisplayState *ds, bool full_screen)
 if (full_screen) {
 gtk_menu_item_activate(GTK_MENU_ITEM(s->full_screen_item));
 }
+if (grab_on_hover) {
+gtk_menu_item_activate(GTK_MENU_ITEM(s->grab_on_hover_item));
+}
 
 register_displaychangelistener(&s->dcl);
 
diff --git a/vl.c b/vl.c
index 842e897..1feb0ff 100644
--- a/vl.c
+++ b/vl.c
@@ -143,6 +143,9 @@ int vga_interface_type = VGA_NONE;
 static int full_screen = 0;
 static int no_frame = 0;
 int no_quit = 0;
+#ifdef CONFIG_GTK
+static bool grab_on_hover;
+#endif
 CharDriverState *serial_hds[MAX_SERIAL_PORTS];
 CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
 CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
@@ -2276,6 +2279,25 @@ static DisplayType select_display(const char *p)
 } else if (strstart(p, "gtk", &opts)) {
 #ifdef CONFIG_GTK
 display = DT_GTK;
+while (*opts) {
+const char *nextopt;
+
+if (strstart(opts, ",grab_on_hover=", &nextopt)) {
+opts = nextopt;
+if (strstart(opts, "on", &nextopt)) {
+grab_on_hover = true;
+} else if (strstart(opts, "off", &nextopt)) {
+grab_on_hover = false;
+} else {
+goto invalid_gtk_args;
+}
+} else {
+invalid_gtk_args:
+fprintf(stderr, "Invalid GTK option string: %s\n", p);
+exit(1);
+}
+opts = nextopt;
+}
 #else
 fprintf(stderr, "GTK support is disabled\n");
 exit(1);
@@ -4399,7 +4421,7 @@ int main(int argc, char **argv, char **envp)
 #endif
 #if defined(CONFIG_GTK)
 case DT_GTK:
-gtk_display_init(ds, full_screen);
+gtk_display_init(ds, full_screen, grab_on_hover);
 break;
 #endif
 default:
-- 
1.8.3.1

[Qemu-devel] [PULL 0/3] gtk fixes.

[Gerd Hoffmann]

  Hi,

gtk pull third round, with buildfix applied to patch #1.

please pull,
  Gerd

The following changes since commit f4b11eee2f562c23b3efc33b96ba4542c9ca81aa:

  Makefile: Fix "make clean" (2014-03-17 11:50:19 +)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/pull-gtk-3

for you to fetch changes up to 2bda66028b4962c36d4eabe2995edab12df93691:

  gtk: Don't warp absolute pointer (2014-03-17 14:34:28 +0100)


gtk: warp bugfixes.
gtk: Allow to activate grab-on-hover from the command line


Cole Robinson (2):
  gtk: Fix mouse warping with gtk3
  gtk: Don't warp absolute pointer

Jan Kiszka (1):
  gtk: Allow to activate grab-on-hover from the command line

 include/ui/console.h |  2 +-
 qemu-options.hx  |  5 +
 ui/gtk.c | 15 +--
 vl.c | 24 +++-
 4 files changed, 42 insertions(+), 4 deletions(-)

[Qemu-devel] [PULL 2/3] gtk: Fix mouse warping with gtk3

[Gerd Hoffmann]

From: Cole Robinson 

We were using the wrong coordinates, this fixes things to match the
original gtk2 implementation.

You can see this error in action by using -vga qxl, however even after this
patch the mouse warps in small increments up and to the left, -7x and -3y
pixels at a time, until the pointer is warped off the widget. I think it's
a qxl bug, but the next patch covers it up.

Signed-off-by: Cole Robinson 
Signed-off-by: Gerd Hoffmann 
---
 ui/gtk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 016804d..e2394ac 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -346,7 +346,7 @@ static void gd_mouse_set(DisplayChangeListener *dcl,
x, y, &x_root, &y_root);
 gdk_device_warp(gdk_device_manager_get_client_pointer(mgr),
 gtk_widget_get_screen(s->drawing_area),
-x, y);
+x_root, y_root);
 }
 #else
 static void gd_mouse_set(DisplayChangeListener *dcl,
-- 
1.8.3.1

Re: [Qemu-devel] vnc regression with -vga vmware

[Gerd Hoffmann]

On Fr, 2014-03-14 at 11:06 -0500, Serge Hallyn wrote:
> Hi,
> 
> upstream git HEAD appears to have regressed with -vga vmware -vnc.
> 
> If I run 
> 
>   ./qemu-system-x86_64 -enable-kvm -vnc :1 -m 1024 -cdrom 
> ~/trusty-desktop-amd64.iso -vga vmware
> 
> then tightvncviewer gives me:
> 
> Connected to RFB server, using protocol version 3.8
> No authentication needed
> Authentication successful
> Desktop name "QEMU"
> VNC server default format:
> 32 bits per pixel.
> Least significant byte first in each pixel.
> True colour: max red 255 green 255 blue 255, shift red 16 green 8
> blue 0
> Using default colormap which is TrueColor.  Pixel format:
> 32 bits per pixel.
> Least significant byte first in each pixel.
> True colour: max red 255 green 255 blue 255, shift red 16
> green 8 blue 0
> Same machine: preferring raw encoding
> Rect too large: 16x4 at (2352, 1766)
> 
> gvncviewer simply says 'Disconnected from server'.
> 
> It works fine if I don't use -vga vmware.
> 
> I bisected it to commit 12b316d: ui/vnc: optimize dirty bitmap tracking

Peter, that is yours, any idea what this is?

cheers,
  Gerd

Re: [Qemu-devel] vnc regression with -vga vmware

[Peter Lieven]

On 17.03.2014 14:44, Gerd Hoffmann wrote:

On Fr, 2014-03-14 at 11:06 -0500, Serge Hallyn wrote:

Hi,

upstream git HEAD appears to have regressed with -vga vmware -vnc.

If I run

./qemu-system-x86_64 -enable-kvm -vnc :1 -m 1024 -cdrom 
~/trusty-desktop-amd64.iso -vga vmware

then tightvncviewer gives me:

Connected to RFB server, using protocol version 3.8
No authentication needed
Authentication successful
Desktop name "QEMU"
VNC server default format:
32 bits per pixel.
Least significant byte first in each pixel.
True colour: max red 255 green 255 blue 255, shift red 16 green 8
blue 0
Using default colormap which is TrueColor.  Pixel format:
32 bits per pixel.
Least significant byte first in each pixel.
True colour: max red 255 green 255 blue 255, shift red 16
green 8 blue 0
Same machine: preferring raw encoding
Rect too large: 16x4 at (2352, 1766)

gvncviewer simply says 'Disconnected from server'.

It works fine if I don't use -vga vmware.

I bisected it to commit 12b316d: ui/vnc: optimize dirty bitmap tracking

Peter, that is yours, any idea what this is?

not yet, I will have a look. at which resolution does -vga vmware work?

Peter


cheers,
   Gerd

Re: [Qemu-devel] n ways block filters

[Benoît Canet]

The Monday 17 Mar 2014 à 11:12:31 (+0800), Fam Zheng wrote :
> On Fri, 03/14 16:57, Benoît Canet wrote:
> > 
> > Hello list,
> > 
> > I plan to convert throttling to a block filter and write n way throttling
> > support.
> > 
> > I discussed a bit with Stefan on the list and we came to the conclusion 
> > that the
> > block filter API need group support.
> > 
> > filter group:
> > -
> > 
> > My current plan to implement this is to add the following fields to the 
> > BlockDriver
> > structure.
> > 
> > int bdrv_add_filter_group(const char *name, QDict options);
> > int bdrv_reconfigure_filter_group(const char *name, QDict options);
> > int bdrv_destroy_filter_group(const char *name);
> > 
> > These three extra method would allow to create, reconfigure or destroy a 
> > block
> > filter group. A block filter group contain the shared or non shared state 
> > of the
> > blockfilter. For throttling it would contains the ThrottleState structure.
> > 
> > Each block filter driver would contains a linked list of linked list where 
> > the
> > BDS are registered grouped by filter groups state.
> 
> Sorry I don't fully understand this. Does a filter group contain multiple 
> block
> filters, and every block filter has effect on multiple BDSes? Could you give 
> an
> example?

The driver module says block/throttle.c would contains a linked list of 
ThrottleGroup
in a static variable.

Each ThrottleGroup would have a name, a ThrottleState (as in utils/throttle.c)
and a linked list of members BDS.

The s variable of the bdrv_* method of throttle.c would have a pointer to the
ThrottleGroup this BDS is a member of hence the driver can access the
ThrottleState

So each BDS filter would have only one BDS parent and one BDS ->file child but
would also be member of a ThrottleGroup.

This would allow to do throttling on a group of devices.

Throttling only one device would create a ThrottleGroup of name bds->device_name
containing only the throttled BDS.

> 
> > 
> > The regular bdrv_open callback would be used to instantiate a block filter 
> > and
> > add it to a filter group. This method would also take a new-node-name for 
> > the new
> > filter. This node-name would become the name of the new filter.
> > bdrv_close would cleanup and deregister from a filter group.
> > 
> > An extra filter-group field in the option dict would allow the bdrv_open 
> > method
> > to register the newly opened block filter in it's filter group.
> > The BDS structure would have a direct pointer to it's filter group state.
> > 
> > Utility methods to do the bdrv_add_filter_group bdrv_open then bdrv_swap to
> > install a new filter can be provided by block.c. The same can be done for 
> > filter
> > close and desinstallation.
> 
> So you are defining block filter as a new kind of block driver. Is a filter
> always on top above everything else by definition?

Yes,

A filter is instanciated on top of another BDS.
Filters can be stacked on top of another to form a chain.
 
> But I am afraid BlockDriverState is already taking too many responsibilities
> here (backend, protocol driver, format driver, filter...). I was wondering if
> it is clearer to rather introduce bs->filter_list to point to a list of
> BlockFilter (a new sturcture, tailored for a block filter), and don't bother
> with bdrv_swap.

Hmm blkverify and quorum are already block filters reusing the BlockDriver
interface. The good this with this is that it would allow to move some code from
block.c to block/filter_name*.c: block throttling for example.
Crypto would be another use case.

Best regards

Benoît

> 
> Thanks,
> Fam
> 
> > 
> > Legacy throttling QMP API
> > -
> > 
> > The legacy throttling API would create throttling filters groups containing 
> > only
> > one BDS.
> > 
> > By default for every 1 way block filter block.c would create a filter group
> > using the BDS id or node-name as group name. This allow for easy filer 
> > removal
> > with the bds reference.
> > 
> > Group throttling API
> > 
> > 
> > Commands would be added to create throttling filter groups reconfigure and 
> > remove
> > them.
> > 
> > Two additional commands would be added to create and insert a block filter 
> > in a
> > given group or close and remove it.
> > 
> > Before I start implementing something what are your thougths on this ?
> > 
> > Best regards
> > 
> > Benoît
> > 
> > 
> > 
> 

Re: [Qemu-devel] vnc regression with -vga vmware

[Peter Lieven]

Serge,

can you confirm this happens at a resolution change?

Peter

On 17.03.2014 14:44, Gerd Hoffmann wrote:

On Fr, 2014-03-14 at 11:06 -0500, Serge Hallyn wrote:

Hi,

upstream git HEAD appears to have regressed with -vga vmware -vnc.

If I run

./qemu-system-x86_64 -enable-kvm -vnc :1 -m 1024 -cdrom 
~/trusty-desktop-amd64.iso -vga vmware

then tightvncviewer gives me:

Connected to RFB server, using protocol version 3.8
No authentication needed
Authentication successful
Desktop name "QEMU"
VNC server default format:
32 bits per pixel.
Least significant byte first in each pixel.
True colour: max red 255 green 255 blue 255, shift red 16 green 8
blue 0
Using default colormap which is TrueColor.  Pixel format:
32 bits per pixel.
Least significant byte first in each pixel.
True colour: max red 255 green 255 blue 255, shift red 16
green 8 blue 0
Same machine: preferring raw encoding
Rect too large: 16x4 at (2352, 1766)

gvncviewer simply says 'Disconnected from server'.

It works fine if I don't use -vga vmware.

I bisected it to commit 12b316d: ui/vnc: optimize dirty bitmap tracking

Peter, that is yours, any idea what this is?

cheers,
   Gerd





--

Mit freundlichen Grüßen

Peter Lieven

...

  KAMP Netzwerkdienste GmbH
  Vestische Str. 89-91 | 46117 Oberhausen
  Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
  p...@kamp.de | http://www.kamp.de

  Geschäftsführer: Heiner Lante | Michael Lante
  Amtsgericht Duisburg | HRB Nr. 12154
  USt-Id-Nr.: DE 120607556

...

Re: [Qemu-devel] [PULL 0/4] Misc fixes for 2.0-rc1

[Peter Maydell]

On 17 March 2014 12:23, Paolo Bonzini  wrote:
> Anthony, Peter,
>
> The following changes since commit f4b11eee2f562c23b3efc33b96ba4542c9ca81aa:
>
>   Makefile: Fix "make clean" (2014-03-17 11:50:19 +)
>
> are available in the git repository at:
>
>   git://github.com/bonzini/qemu.git fixes-for-2.0
>
> for you to fetch changes up to 025172d56e11ba3d86d0937933a23aab3b8606b1:
>
>   vl.c: Output error on invalid machine type (2014-03-17 13:21:12 +0100)
>
> I gathered miscellaneous fixes from the mailing list, two of them mine,
> that are suitable for hard freeze.  (I originarily had Fam's other patch
> in here too).
>

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v2] update names in option tables to match with actual command-line spelling

[Paolo Bonzini]

Il 17/03/2014 13:33, Markus Armbruster ha scritto:

This patch changes:

fromto  introduced in
acpiacpitable   0c764a9 v1.5.0
boot-opts   boot3d3b830 v1.0
smp-optssmp 12b7f57 v1.6.0

All three have calcified into ABI already.


What has calcified into ABI is only what has likely been used as ABI. 
The other is an ABI that won't be nice to break, but it is not calcified 
yet.


There are two aspects of this:

1) QMP queries.  Did any of the above conversions introduce new 
sub-options?  Or was any suboption introduced after that point?  If not, 
there's hardly a reason for anyone to query for acpi/boot-opts/smp-opts 
QemuOpts


2) readconfig.  I think readconfig is not practically usable until at 
least memory will be part of it.  For this reason, -readconfig is not a 
worry; not yet at least: it will be in 2.1 once -m is converted.



> I'm for including this patch in 2.0.

Not without explaining the ABI breakage in the commit message.  We
should also make sure to cover it in the release notes[*].


I agree.


Moreover, if exceptions from the rule "QemuOptsList name must match the
name of the (non-sugared) command line option using it" are or will be
permissible, then Amos's comment on QemuOptsList member name needs to be
clarified.


I like your idea of -memory as a synonym for -m.

Paolo

Re: [Qemu-devel] vnc regression with -vga vmware

[Serge Hallyn]

It does happen then as well (I suppose), but after X is done setting
up, it happens every time I try to connect.

Quoting Peter Lieven (p...@kamp.de):
> Serge,
> 
> can you confirm this happens at a resolution change?
> 
> Peter
> 
> On 17.03.2014 14:44, Gerd Hoffmann wrote:
> >On Fr, 2014-03-14 at 11:06 -0500, Serge Hallyn wrote:
> >>Hi,
> >>
> >>upstream git HEAD appears to have regressed with -vga vmware -vnc.
> >>
> >>If I run
> >>
> >>./qemu-system-x86_64 -enable-kvm -vnc :1 -m 1024 -cdrom 
> >> ~/trusty-desktop-amd64.iso -vga vmware
> >>
> >>then tightvncviewer gives me:
> >>
> >>Connected to RFB server, using protocol version 3.8
> >>No authentication needed
> >>Authentication successful
> >>Desktop name "QEMU"
> >>VNC server default format:
> >>32 bits per pixel.
> >>Least significant byte first in each pixel.
> >>True colour: max red 255 green 255 blue 255, shift red 16 green 8
> >>blue 0
> >>Using default colormap which is TrueColor.  Pixel format:
> >>32 bits per pixel.
> >>Least significant byte first in each pixel.
> >>True colour: max red 255 green 255 blue 255, shift red 16
> >>green 8 blue 0
> >>Same machine: preferring raw encoding
> >>Rect too large: 16x4 at (2352, 1766)
> >>
> >>gvncviewer simply says 'Disconnected from server'.
> >>
> >>It works fine if I don't use -vga vmware.
> >>
> >>I bisected it to commit 12b316d: ui/vnc: optimize dirty bitmap tracking
> >Peter, that is yours, any idea what this is?
> >
> >cheers,
> >   Gerd
> >
> >
> 
> 
> -- 
> 
> Mit freundlichen Grüßen
> 
> Peter Lieven
> 
> ...
> 
>   KAMP Netzwerkdienste GmbH
>   Vestische Str. 89-91 | 46117 Oberhausen
>   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
>   p...@kamp.de | http://www.kamp.de
> 
>   Geschäftsführer: Heiner Lante | Michael Lante
>   Amtsgericht Duisburg | HRB Nr. 12154
>   USt-Id-Nr.: DE 120607556
> 
> ...
> 

Re: [Qemu-devel] [PATCH v4 2/3] qapi: Add a primitive to include other files from a QAPI schema file

[Benoît Canet]

The Thursday 13 Mar 2014 à 09:54:15 (-0600), Eric Blake wrote :
> On 03/13/2014 09:33 AM, Benoît Canet wrote:
> 
> >> We certainly can't do without comments.
> >>
> >> JSON is designed for easy data exchange, but we use it as programming
> >> language syntax.  Its restrictions make sense for easy data exchange,
> >> but hurt our use.  We're not the first ones experiencing that pain:
> >> http://json5.org/
> >>
> >> No idea how much momentum this JSON5 thingy has...
> 
> If we 's,#,//,', our comments magically fall in line with JSON5 syntax;
> everything else in our files is already compliant with JSON5.

Not really qapi-schema.json is missing comas between types to be a
valid json file.

The fragment:

{ 'type'  : 'InputBtnEvent',
  'data'  : { 'button'  : 'InputButton',
  'down': 'bool' } }

{ 'type'  : 'InputMoveEvent',
  'data'  : { 'axis': 'InputAxis',
  'value'   : 'int' } }

Should be:

[

{ 'type'  : 'InputBtnEvent',
  'data'  : { 'button'  : 'InputButton',
  'down': 'bool' } },

{ 'type'  : 'InputMoveEvent',
  'data'  : { 'axis': 'InputAxis',
  'value'   : 'int' } }

]

to hope being a valid json file.

Best regards

Benoît

> 
> >>
> >> Switch to JSON5 and call it qapi-schema.json5?
> 
> This actually seems like a rather nice idea - but due to our choice of
> comments, it means rewriting the bulk of the file and tweaking our parser.
> 
> >>
> > 
> > Hmm don't we want something that python and other language know how to 
> > parse out
> > of the box ? Or will we write yet another delicate work of art to parse it ?
> 
> Our existing parser would only need to learn a new comment syntax to
> parse the subset of JSON5 that we currently actually use.  Parsing FULL
> JSON5 would mean also learning about trailing commas, unquoted names in
> name:value pairs, multiline strings, and alternative numeric
> representations.  But a point made on the JSON5 page is that ES5
> JavaScript already parses JSON5, just as it already parses original JSON.
> 
> -- 
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] virtio device error reporting best practice?

[Laszlo Ersek]

On 03/17/14 07:02, Dave Airlie wrote:
> So I'm looking at how best to do virtio gpu device error reporting,
> and how to deal with illegal stuff,
> 
> I've two levels of errors I want to support,
> 
> a) unrecoverable or bad guest kernel programming errors,
> 
> b) per 3D context errors from the renderer backend,
> 
> (b) I can easily report in an event queue and the guest kernel can in
> theory blow away the offenders, this is how GL works with some
> extensions,
> 
> For (a) I can expect a response from every command I put into the main
> GPU control queue, the response should always be no error, but in some
> cases it will be because the guest hit some host resource error, or
> asked for something insane, (guest kernel drivers would be broken in
> most of these cases).
> 
> Alternately I can use the separate event queue to send async errors
> when the guest does something bad,
> 
> I'm also considering adding some sort of flag in config space saying
> the device needs a reset before it will continue doing anything,
> 
> The main reason I'm considering this stuff is for security reasons if
> the guest asks for something really illegal or crazy what should the
> expected behaviour of the host be? (at least secure I know that).

exit(1).

If you grep qemu for it, you'll find such examples. Notably,
"hw/virtio/virtio.c" is chock full of them; if the guest doesn't speak
the basic protocol, there's nothing for the host to do. See also
virtio-blk.c (missing or incorrect headers), virtio-net.c (similar
protocol violations), virtio-scsi.c (wrong header size, bad config etc).

For later, we have a use case on the horizon where all such exits -- not
just virtio, but exit(1) or abort() on invalid guest behavior in general
-- should be optionally coupled (dependent on the qemu command line)
with an automatic dump-guest-memory, in order to help debugging the guest.

Thanks
Laszlo

Re: [Qemu-devel] virtio device error reporting best practice?

[Peter Maydell]

On 17 March 2014 14:28, Laszlo Ersek  wrote:
> On 03/17/14 07:02, Dave Airlie wrote:
>> The main reason I'm considering this stuff is for security reasons if
>> the guest asks for something really illegal or crazy what should the
>> expected behaviour of the host be? (at least secure I know that).
>
> exit(1).

No thanks -- the guest should never be able to cause QEMU
to exit (in an ideal world). Use
   qemu_log_mask(LOG_GUEST_ERROR, ...)
and continue.

> If you grep qemu for it, you'll find such examples. Notably,
> "hw/virtio/virtio.c" is chock full of them; if the guest doesn't speak
> the basic protocol, there's nothing for the host to do. See also
> virtio-blk.c (missing or incorrect headers), virtio-net.c (similar
> protocol violations), virtio-scsi.c (wrong header size, bad config etc).

I think these are all examples of legacy code written before we
had a sensible logging API for this kind of thing.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] linux-user: Implement capget, capset

[Riku Voipio]

On Mon, Mar 17, 2014 at 12:15:35PM +, Peter Maydell wrote:
> Implement the capget and capset syscalls. This is useful because
> simple programs like 'ls' try to use it in AArch64, and otherwise
> we emit a lot of noise about it being unimplemented.
 
> Signed-off-by: Peter Maydell 
> ---
> Changes v1->v2: initialize target_data to NULL to suppress a gcc 4.8
> warning; roll our own capget() and capset() rather than relying on
> the sys/capability.h header that is in an optional package rather
> than libc proper.

Thanks, looks good and passes the ltp tests. Honestly, I think putting
this to 2.0 past freeze is sloppy maintainership from me - but I trust
you would fix any issues caused by this patch rapidly ;)

I've included in the to-be-pull req:

http://git.linaro.org/people/riku.voipio/qemu.git/shortlog/refs/heads/linux-user-for-upstream

If someone protests, I can replace it with nowarn version.

Riku

>  linux-user/syscall.c  | 75 
> +--
>  linux-user/syscall_defs.h | 11 +++
>  2 files changed, 84 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 2a8b66c..49244f8 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -43,6 +43,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #ifdef __ia64__
> @@ -243,6 +244,10 @@ _syscall3(int, sys_sched_setaffinity, pid_t, pid, 
> unsigned int, len,
>unsigned long *, user_mask_ptr);
>  _syscall4(int, reboot, int, magic1, int, magic2, unsigned int, cmd,
>void *, arg);
> +_syscall2(int, capget, struct __user_cap_header_struct *, header,
> +  struct __user_cap_data_struct *, data);
> +_syscall2(int, capset, struct __user_cap_header_struct *, header,
> +  struct __user_cap_data_struct *, data);
>  
>  static bitmask_transtbl fcntl_flags_tbl[] = {
>{ TARGET_O_ACCMODE,   TARGET_O_WRONLY,O_ACCMODE,   O_WRONLY,},
> @@ -7641,9 +7646,75 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>  unlock_user(p, arg1, ret);
>  break;
>  case TARGET_NR_capget:
> -goto unimplemented;
>  case TARGET_NR_capset:
> -goto unimplemented;
> +{
> +struct target_user_cap_header *target_header;
> +struct target_user_cap_data *target_data = NULL;
> +struct __user_cap_header_struct header;
> +struct __user_cap_data_struct data[2];
> +struct __user_cap_data_struct *dataptr = NULL;
> +int i, target_datalen;
> +int data_items = 1;
> +
> +if (!lock_user_struct(VERIFY_WRITE, target_header, arg1, 1)) {
> +goto efault;
> +}
> +header.version = tswap32(target_header->version);
> +header.pid = tswap32(target_header->pid);
> +
> +if (header.version != _LINUX_CAPABILITY_VERSION_1) {
> +/* Version 2 and up takes pointer to two user_data structs */
> +data_items = 2;
> +}
> +
> +target_datalen = sizeof(*target_data) * data_items;
> +
> +if (arg2) {
> +if (num == TARGET_NR_capget) {
> +target_data = lock_user(VERIFY_WRITE, arg2, target_datalen, 
> 0);
> +} else {
> +target_data = lock_user(VERIFY_READ, arg2, target_datalen, 
> 1);
> +}
> +if (!target_data) {
> +unlock_user_struct(target_header, arg1, 0);
> +goto efault;
> +}
> +
> +if (num == TARGET_NR_capset) {
> +for (i = 0; i < data_items; i++) {
> +data[i].effective = tswap32(target_data[i].effective);
> +data[i].permitted = tswap32(target_data[i].permitted);
> +data[i].inheritable = 
> tswap32(target_data[i].inheritable);
> +}
> +}
> +
> +dataptr = data;
> +}
> +
> +if (num == TARGET_NR_capget) {
> +ret = get_errno(capget(&header, dataptr));
> +} else {
> +ret = get_errno(capset(&header, dataptr));
> +}
> +
> +/* The kernel always updates version for both capget and capset */
> +target_header->version = tswap32(header.version);
> +unlock_user_struct(target_header, arg1, 1);
> +
> +if (arg2) {
> +if (num == TARGET_NR_capget) {
> +for (i = 0; i < data_items; i++) {
> +target_data[i].effective = tswap32(data[i].effective);
> +target_data[i].permitted = tswap32(data[i].permitted);
> +target_data[i].inheritable = 
> tswap32(data[i].inheritable);
> +}
> +unlock_user(target_data, arg2, target_datalen);
> +} else {
> +unlock_user(target_data, arg2, 0);
> +}
> +}
> +break;
> +}
>  case TARGET_NR_sigaltstack:
>  #if defined(TARGET_I386) || 

Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-s390x: Add missing 'static' and 'const' attributes

[Michael Tokarev]

16.03.2014 17:49, Stefan Weil wrote:
> This fixes warnings from the static code analysis (smatch).

Thanks, applied to -trivial.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-arm: Add missing 'static' attribute

[Michael Tokarev]

16.03.2014 22:07, Stefan Weil wrote:
> This fixes a warning from the static code analysis (smatch).

Thanks, applied to -trivial.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH] hw/ide: Add missing 'static' attributes

[Michael Tokarev]

16.03.2014 22:13, Stefan Weil wrote:
> This fixes a warning from the static code analysis (smatch).

Thanks, applied to -trivial.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH] util: Add 'static' attribute to function implementation

[Michael Tokarev]

16.03.2014 22:02, Stefan Weil wrote:
> The static code analyzer smatch complains because of a missing 'static'
> attribute:

Thanks, applied to -trivial.

/mjt

Re: [Qemu-devel] virtio device error reporting best practice?

[Laszlo Ersek]

On 03/17/14 15:40, Peter Maydell wrote:
> On 17 March 2014 14:28, Laszlo Ersek  wrote:
>> On 03/17/14 07:02, Dave Airlie wrote:
>>> The main reason I'm considering this stuff is for security reasons if
>>> the guest asks for something really illegal or crazy what should the
>>> expected behaviour of the host be? (at least secure I know that).
>>
>> exit(1).
> 
> No thanks -- the guest should never be able to cause QEMU
> to exit (in an ideal world). Use
>qemu_log_mask(LOG_GUEST_ERROR, ...)
> and continue.

How do you continue with a garbled virtio ring? Say you detect an error
that would cause integer overflow or buffer overflow in the host, a
clear virtio protocol violation etc. Error reporting is just one thing
-- what are the semantics of continuing?

Exit(1) is considered guest crash. "The guest should never be able to
cause QEMU to exit" is identical to "a bare metal kernel should never be
able to crash a physical machine, even if it wants to". Some behavior is
left undefined even in hardware manuals. Crashing as early as possible
is safe and helpful.

Just my opinion, naturally.

Thanks,
Laszlo

Re: [Qemu-devel] virtio device error reporting best practice?

[Gerd Hoffmann]

On Mo, 2014-03-17 at 16:02 +1000, Dave Airlie wrote:
> So I'm looking at how best to do virtio gpu device error reporting,
> and how to deal with illegal stuff,
> 
> I've two levels of errors I want to support,
> 
> a) unrecoverable or bad guest kernel programming errors,
> 
> b) per 3D context errors from the renderer backend,

What you find in modern real hardware (xhci for example) and which would
also make sense for virtio is:

 * stick an error message into a event queue (for non-fatal errors),
   which would be a good fit for (b)
 * set a bit in a error status register, maybe raise IRQ, stop
   processing until reset (for fatal errors, i.e. your (a) case).
   such a register can live in config space for virtio.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 4/4] xen-all: Pass max_ram_below_4g to xen_hvm_init.

[Don Slutz]

On 03/16/14 12:10, Stefano Stabellini wrote:

On Tue, 11 Mar 2014, Don Slutz wrote:

This is the xen part of "pc & q35: Add new object pc-memory-layout."

Signed-off-by: Don Slutz
---
  hw/i386/pc_piix.c|  4 ++--
  hw/i386/pc_q35.c |  4 ++--
  include/hw/xen/xen.h |  4 ++--
  xen-all.c| 41 ++---
  xen-stub.c   |  4 ++--
  5 files changed, 30 insertions(+), 27 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 964ea33..691fc5d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -131,8 +131,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
  below_4g_mem_size = args->ram_size;
  }
  
-if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,

-  &ram_memory) != 0) {
+if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
+  &above_4g_mem_size, &ram_memory) != 0) {
  fprintf(stderr, "xen hardware virtual machine initialisation 
failed\n");
  exit(1);
  }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c95e6e2..8e1c417 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -120,8 +120,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
  below_4g_mem_size = args->ram_size;
  }
  
-if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,

-  &ram_memory) != 0) {
+if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
+  &above_4g_mem_size, &ram_memory) != 0) {
  fprintf(stderr, "xen hardware virtual machine initialisation 
failed\n");
  exit(1);
  }
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 0769db2..fda559a 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -41,8 +41,8 @@ int xen_init(QEMUMachine *machine);
  void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
  
  #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)

-int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
- MemoryRegion **ram_memory);
+int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
+ ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory);
  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
 struct MemoryRegion *mr);
  void xen_modified_memory(ram_addr_t start, ram_addr_t length);
diff --git a/xen-all.c b/xen-all.c
index c64300c..48ba335 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -155,32 +155,34 @@ qemu_irq *xen_interrupt_controller_init(void)
  
  /* Memory Ops */
  
-static void xen_ram_init(ram_addr_t *below_4g_mem_size,

+static void xen_ram_init(ram_addr_t ram_size, ram_addr_t max_ram_below_4g,
+ ram_addr_t *below_4g_mem_size,
   ram_addr_t *above_4g_mem_size,
- ram_addr_t ram_size, MemoryRegion **ram_memory_p)
+ MemoryRegion **ram_memory_p)
  {
  MemoryRegion *sysmem = get_system_memory();
  ram_addr_t block_len;
  
-block_len = ram_size;

-if (ram_size >= HVM_BELOW_4G_RAM_END) {
-/* Xen does not allocate the memory continuously, and keep a hole at
- * HVM_BELOW_4G_MMIO_START of HVM_BELOW_4G_MMIO_LENGTH
- */
-block_len += HVM_BELOW_4G_MMIO_LENGTH;
+if (!max_ram_below_4g) {
+if (ram_size >= HVM_BELOW_4G_RAM_END) {
+*above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
+*below_4g_mem_size = HVM_BELOW_4G_RAM_END;
+} else {
+*above_4g_mem_size = 0;
+*below_4g_mem_size = ram_size;
+}
+}

Instead of treating max_ram_below_4g as a special case, couldn't
initialize it to HVM_BELOW_4G_RAM_END?


Since max_ram_below_4g is used and initialize in normal QEMU code
where HVM_BELOW_4G_RAM_END is not defined, I do not see how to
do this outside of this routine without adding a new routine for this.


We could consider HVM_BELOW_4G_RAM_END as the upper bound of
max_ram_below_4g.


Yes.  Currently the only limit on max_ram_below_4g is 4G.  I am on
the fence on this.  I think there are valid values > HVM_BELOW_4G_RAM_END,
but not clear they need to be supported.


I would just change the code to:

 if (ram_size >= max_ram_below_4g) {
 above_4g_mem_size = ram_size - max_ram_below_4g;
 below_4g_mem_size = max_ram_below_4g;
 } else {
 below_4g_mem_size = ram_size;
 }



This is the code that is in patch #3 in the normal QEMU path but using
the variable lowmem so that xen can know if max_ram_below_4g was
set to non-zero.  The issue that I am looking at handling here is the case
of an older xen (does not know about  max_ram_below_4g) and the
newer QEMU, where xen expects QEMUs memory layout to change to
HVM_BELOW_4G_RAM_END.

Because of this (QEMU is

Re: [Qemu-devel] virtio device error reporting best practice?

[Peter Maydell]

On 17 March 2014 14:49, Laszlo Ersek  wrote:
> On 03/17/14 15:40, Peter Maydell wrote:
>> On 17 March 2014 14:28, Laszlo Ersek  wrote:
>>> On 03/17/14 07:02, Dave Airlie wrote:
 The main reason I'm considering this stuff is for security reasons if
 the guest asks for something really illegal or crazy what should the
 expected behaviour of the host be? (at least secure I know that).
>>>
>>> exit(1).
>>
>> No thanks -- the guest should never be able to cause QEMU
>> to exit (in an ideal world). Use
>>qemu_log_mask(LOG_GUEST_ERROR, ...)
>> and continue.
>
> How do you continue with a garbled virtio ring? Say you detect an error
> that would cause integer overflow or buffer overflow in the host, a
> clear virtio protocol violation etc. Error reporting is just one thing
> -- what are the semantics of continuing?

Whatever the device likes. If you're emulating real hardware
then the ideal is to match that. For virtio, whatever seems
easiest. Something that would allow the guest to work after
it does a warm reboot would be nice.

> Exit(1) is considered guest crash. "The guest should never be able to
> cause QEMU to exit" is identical to "a bare metal kernel should never be
> able to crash a physical machine, even if it wants to". Some behavior is
> left undefined even in hardware manuals. Crashing as early as possible
> is safe and helpful.

This would mean we couldn't model systems with a hardware
watchdog timer that looks out for guest crashes and does a reboot,
for instance. exit() is approximately equivalent to "machine
caught fire" since it means that there's no resurrecting
the VM via watchdog device, forcing reset through a management
interface or anything else.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] dataplane: fix bogus coverity warnings

[Stefan Hajnoczi]

On Fri, Mar 14, 2014 at 06:25:34PM +0100, Paolo Bonzini wrote:
> Il 14/03/2014 16:14, Stefan Hajnoczi ha scritto:
> >Coverity detects when variable are accessed under a mutex most of the time.  
> >It
> >warns when they are not accessed under the mutex.  I initialized variables
> >before the mutex and threads that access them even exist - Coverity doesn't
> >like that.  Fix the code.
> >
> >Stefan Hajnoczi (2):
> >  iothread: fix bogus coverity warning
> >  rfifolock: fix bogus coverity warning
> >
> > iothread.c   | 5 -
> > util/rfifolock.c | 4 +++-
> > 2 files changed, 7 insertions(+), 2 deletions(-)
> >
> 
> Nah, I think Coverity is wrong.  It should detect initialization of
> the mutex, and treat surrounding code as single-threaded.  I
> silenced the defects in the report, like others before.

Okay, let's drop these patches.

Stefan

Re: [Qemu-devel] virtio device error reporting best practice?

[Gerd Hoffmann]

On Mo, 2014-03-17 at 15:49 +0100, Laszlo Ersek wrote:
> On 03/17/14 15:40, Peter Maydell wrote:
> > On 17 March 2014 14:28, Laszlo Ersek  wrote:
> >> On 03/17/14 07:02, Dave Airlie wrote:
> >>> The main reason I'm considering this stuff is for security reasons if
> >>> the guest asks for something really illegal or crazy what should the
> >>> expected behaviour of the host be? (at least secure I know that).
> >>
> >> exit(1).
> > 
> > No thanks -- the guest should never be able to cause QEMU
> > to exit (in an ideal world). Use
> >qemu_log_mask(LOG_GUEST_ERROR, ...)
> > and continue.
> 
> How do you continue with a garbled virtio ring? Say you detect an error
> that would cause integer overflow or buffer overflow in the host, a
> clear virtio protocol violation etc. Error reporting is just one thing
> -- what are the semantics of continuing?

Stop processing until device is reset.  This is what real hardware does,
and there are places in qemu (xhci emulation for example) which does
this too.

We don't have a standard error register in virtio to report this to the
guest though.  Maybe something to consider for virtio 1.0?  mst?

cheers,
  Gerd

Re: [Qemu-devel] virtio device error reporting best practice?

[Richard W.M. Jones]

On Mon, Mar 17, 2014 at 02:40:09PM +, Peter Maydell wrote:
> On 17 March 2014 14:28, Laszlo Ersek  wrote:
> > On 03/17/14 07:02, Dave Airlie wrote:
> >> The main reason I'm considering this stuff is for security reasons if
> >> the guest asks for something really illegal or crazy what should the
> >> expected behaviour of the host be? (at least secure I know that).
> >
> > exit(1).
> 
> No thanks -- the guest should never be able to cause QEMU
> to exit (in an ideal world). Use
>qemu_log_mask(LOG_GUEST_ERROR, ...)
> and continue.

Don't look too closely at the spice backend ...

https://bugzilla.redhat.com/buglist.cgi?component=spice&list_id=2320267

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH v7 for 2.0 1/4] qapi: introduce PreallocMode and a new PreallocMode full.

[Eric Blake]

On 03/17/2014 12:53 AM, Hu Tao wrote:
> This patch prepares for the subsequent patches.
> 
> Reviewed-by: Fam Zheng 
> Signed-off-by: Hu Tao 
> ---
>  block/qcow2.c|  8 
>  qapi-schema.json | 14 ++
>  2 files changed, 18 insertions(+), 4 deletions(-)

I'll leave it up to maintainers on whether we are still in time for 2.0
(the feature has been present on list prior to freeze - but at this
point it is a feature addition rather than a bug fix), but assuming the
answer is yes:

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2] convert fprintf() calls to error_setg() in block/qed.c:bdrv_qed_create()

[Stefan Hajnoczi]

On Sat, Mar 15, 2014 at 03:05:23PM +0530, Aakriti Gupta wrote:
> This patch converts fprintf() calls to error_setg() in 
> block/qed.c:bdrv_qed_create()
> (error_setg() is part of error reporting API in include/qapi/error.h)
> 
> Signed-off-by: Aakriti Gupta 
> ---
>  block/qed.c |8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Acked-by: Stefan Hajnoczi 

>  if (!qed_is_cluster_size_valid(cluster_size)) {
> -fprintf(stderr, "QED cluster size must be within range [%u, %u] and 
> power of 2\n",
> +error_setg(errp, "QED cluster size must be within range [%u, %u] and 
> power of 2",
>  QED_MIN_CLUSTER_SIZE, QED_MAX_CLUSTER_SIZE);

Kevin: Do you want to fix up the indentation of the next line when
merging?

Re: [Qemu-devel] virtio device error reporting best practice?

[Richard W.M. Jones]

On Mon, Mar 17, 2014 at 02:57:41PM +, Richard W.M. Jones wrote:
> 
> On Mon, Mar 17, 2014 at 02:40:09PM +, Peter Maydell wrote:
> > On 17 March 2014 14:28, Laszlo Ersek  wrote:
> > > On 03/17/14 07:02, Dave Airlie wrote:
> > >> The main reason I'm considering this stuff is for security reasons if
> > >> the guest asks for something really illegal or crazy what should the
> > >> expected behaviour of the host be? (at least secure I know that).
> > >
> > > exit(1).
> > 
> > No thanks -- the guest should never be able to cause QEMU
> > to exit (in an ideal world). Use
> >qemu_log_mask(LOG_GUEST_ERROR, ...)
> > and continue.
> 
> Don't look too closely at the spice backend ...
> 
> https://bugzilla.redhat.com/buglist.cgi?component=spice&list_id=2320267

A better link might be:
https://bugzilla.redhat.com/show_bug.cgi?id=997932#c13

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] vnc regression with -vga vmware

[Peter Lieven]

I found 2 issues:

- with vmware VGA the server surface and the client desktop size are out of 
sync for some time
at a resolution change. the server surface gets updates for x coordinates that 
are out of bound
for the client.
- the max width of the client (2360) is not dividable by 16 
(VNC_DIRTY_PIXELS_PER_BIT).

I will try to fix this in ui/vnc but we should definetly look for the root 
cause.

Peter



On 17.03.2014 15:10, Serge Hallyn wrote:

It does happen then as well (I suppose), but after X is done setting
up, it happens every time I try to connect.

Quoting Peter Lieven (p...@kamp.de):

Serge,

can you confirm this happens at a resolution change?

Peter

On 17.03.2014 14:44, Gerd Hoffmann wrote:

On Fr, 2014-03-14 at 11:06 -0500, Serge Hallyn wrote:

Hi,

upstream git HEAD appears to have regressed with -vga vmware -vnc.

If I run

./qemu-system-x86_64 -enable-kvm -vnc :1 -m 1024 -cdrom 
~/trusty-desktop-amd64.iso -vga vmware

then tightvncviewer gives me:

Connected to RFB server, using protocol version 3.8
No authentication needed
Authentication successful
Desktop name "QEMU"
VNC server default format:
32 bits per pixel.
Least significant byte first in each pixel.
True colour: max red 255 green 255 blue 255, shift red 16 green 8
blue 0
Using default colormap which is TrueColor.  Pixel format:
32 bits per pixel.
Least significant byte first in each pixel.
True colour: max red 255 green 255 blue 255, shift red 16
green 8 blue 0
Same machine: preferring raw encoding
Rect too large: 16x4 at (2352, 1766)

gvncviewer simply says 'Disconnected from server'.

It works fine if I don't use -vga vmware.

I bisected it to commit 12b316d: ui/vnc: optimize dirty bitmap tracking

Peter, that is yours, any idea what this is?

cheers,
   Gerd




--

Mit freundlichen Grüßen

Peter Lieven

...

   KAMP Netzwerkdienste GmbH
   Vestische Str. 89-91 | 46117 Oberhausen
   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
   p...@kamp.de | http://www.kamp.de

   Geschäftsführer: Heiner Lante | Michael Lante
   Amtsgericht Duisburg | HRB Nr. 12154
   USt-Id-Nr.: DE 120607556

...

Re: [Qemu-devel] [PATCH 2/3] block: Replaced old error handling with error reporting API.

[Stefan Hajnoczi]

On Mon, Mar 17, 2014 at 01:10:58AM +0400, Maria Kustova wrote:
> Signed-off-by: Maria Kustova 
> ---
>  block/curl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 3494c6d..359637e 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -538,7 +538,7 @@ static int curl_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  return 0;
>  
>  out:
> -fprintf(stderr, "CURL: Error opening file: %s\n", state->errmsg);
> +error_setg(errp, "CURL: Error opening file: %s\n", state->errmsg);

error_setg() does not require newline at the end of the line.  Please
remove the '\n'.

Re: [Qemu-devel] [PATCH 1/3] qemu-io-cmds: Fixed typo in example for writev.

[Stefan Hajnoczi]

On Mon, Mar 17, 2014 at 01:10:57AM +0400, Maria Kustova wrote:
> Signed-off-by: Maria Kustova 
> ---
>  qemu-io-cmds.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi 

Re: [Qemu-devel] [PATCH 3/3] qemu-io: Extended "--cmd" description in usage text

[Stefan Hajnoczi]

On Mon, Mar 17, 2014 at 01:10:59AM +0400, Maria Kustova wrote:
> It's not clear from the usage description that "--cmd" option accepts its 
> argument as a string, so any special symbols have to be quoted from the shell.
> Updates in usage text:
>  - Specified parameter format for "--cmd" option.
>  - Added an instruction how to get help for "--cmd" option.
> 
> Signed-off-by: Maria Kustova 
> ---
>  qemu-io.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi 

Re: [Qemu-devel] vnc regression with -vga vmware

[Serge Hallyn]

Quoting Peter Lieven (lieven-li...@dlhnet.de):
> I found 2 issues:
> 
> - with vmware VGA the server surface and the client desktop size are out of 
> sync for some time
> at a resolution change. the server surface gets updates for x coordinates 
> that are out of bound
> for the client.
> - the max width of the client (2360) is not dividable by 16 
> (VNC_DIRTY_PIXELS_PER_BIT).
> 
> I will try to fix this in ui/vnc but we should definetly look for the root 
> cause.

Thanks, Peter!

-serge

Re: [Qemu-devel] vnc regression with -vga vmware

[Peter Lieven]

On 17.03.2014 16:19, Serge Hallyn wrote:

Quoting Peter Lieven (lieven-li...@dlhnet.de):

I found 2 issues:

- with vmware VGA the server surface and the client desktop size are out of 
sync for some time
at a resolution change. the server surface gets updates for x coordinates that 
are out of bound
for the client.
- the max width of the client (2360) is not dividable by 16 
(VNC_DIRTY_PIXELS_PER_BIT).

I will try to fix this in ui/vnc but we should definetly look for the root 
cause.

Thanks, Peter!

-serge



The vmware vga driver seems to do some nasty things.
I receive the msg like this (independent of ui/vnc):
vmsvga_update_rect: update x was < 0 (-65)

Can you try the following patch:

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index bd2c108..6ae3348 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -25,6 +25,7 @@
 #include "hw/loader.h"
 #include "trace.h"
 #include "ui/console.h"
+#include "ui/vnc.h"
 #include "hw/pci/pci.h"

 #undef VERBOSE
@@ -218,7 +219,7 @@ enum {

 /* These values can probably be changed arbitrarily.  */
 #define SVGA_SCRATCH_SIZE   0x8000
-#define SVGA_MAX_WIDTH  2360
+#define SVGA_MAX_WIDTH  ROUND_UP(2360, 
VNC_DIRTY_PIXELS_PER_BIT)
 #define SVGA_MAX_HEIGHT 1770

 #ifdef VERBOSE
diff --git a/ui/vnc.c b/ui/vnc.c
index 9c84b3e..5925774 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -888,7 +888,7 @@ static int vnc_update_client(VncState *vs, int has_dirty, 
bool sync)
 VncDisplay *vd = vs->vd;
 VncJob *job;
 int y;
-int height;
+int height, width;
 int n = 0;

 if (vs->output.offset && !vs->audio_cap && !vs->force_update)
@@ -907,6 +907,7 @@ static int vnc_update_client(VncState *vs, int has_dirty, 
bool sync)
 job = vnc_job_new(vs);

 height = MIN(pixman_image_get_height(vd->server), vs->client_height);
+width = MIN(pixman_image_get_width(vd->server), vs->client_width);

 y = 0;
 for (;;) {
@@ -925,8 +926,11 @@ static int vnc_update_client(VncState *vs, int has_dirty, 
bool sync)
 VNC_DIRTY_BPL(vs), x);
 bitmap_clear(vs->dirty[y], x, x2 - x);
 h = find_and_clear_dirty_height(vs, y, x, x2, height);
-n += vnc_job_add_rect(job, x * VNC_DIRTY_PIXELS_PER_BIT, y,
-  (x2 - x) * VNC_DIRTY_PIXELS_PER_BIT, h);
+x2 = MIN(x2, width / VNC_DIRTY_PIXELS_PER_BIT);
+if (x2 > x) {
+n += vnc_job_add_rect(job, x * VNC_DIRTY_PIXELS_PER_BIT, y,
+  (x2 - x) * VNC_DIRTY_PIXELS_PER_BIT, h);
+}
 }

 vnc_job_push(job);

Peter