[Qemu-devel] [PATCH 02/67] target/arm: Remove offset argument to gen_exception_insn
The address of the current insn is still available in s->base.pc_next.
Signed-off-by: Richard Henderson
---
target/arm/translate-vfp.inc.c | 6 +++---
target/arm/translate.c | 32
2 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
index 092eb5ec53..e7389bc057 100644
--- a/target/arm/translate-vfp.inc.c
+++ b/target/arm/translate-vfp.inc.c
@@ -96,10 +96,10 @@ static bool full_vfp_access_check(DisasContext *s, bool
ignore_vfp_enabled)
{
if (s->fp_excp_el) {
if (arm_dc_feature(s, ARM_FEATURE_M)) {
-gen_exception_insn(s, 4, EXCP_NOCP, syn_uncategorized(),
+gen_exception_insn(s, EXCP_NOCP, syn_uncategorized(),
s->fp_excp_el);
} else {
-gen_exception_insn(s, 4, EXCP_UDEF,
+gen_exception_insn(s, EXCP_UDEF,
syn_fp_access_trap(1, 0xe, false),
s->fp_excp_el);
}
@@ -108,7 +108,7 @@ static bool full_vfp_access_check(DisasContext *s, bool
ignore_vfp_enabled)
if (!s->vfp_enabled && !ignore_vfp_enabled) {
assert(!arm_dc_feature(s, ARM_FEATURE_M));
-gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),
+gen_exception_insn(s, EXCP_UDEF, syn_uncategorized(),
default_exception_el(s));
return false;
}
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 7853462b21..33f78296eb 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1247,11 +1247,11 @@ static void gen_exception_internal_insn(DisasContext
*s, int offset, int excp)
s->base.is_jmp = DISAS_NORETURN;
}
-static void gen_exception_insn(DisasContext *s, int offset, int excp,
- int syn, uint32_t target_el)
+static void gen_exception_insn(DisasContext *s, int excp, int syn,
+ uint32_t target_el)
{
gen_set_condexec(s);
-gen_set_pc_im(s, s->pc - offset);
+gen_set_pc_im(s, s->base.pc_next);
gen_exception(excp, syn, target_el);
s->base.is_jmp = DISAS_NORETURN;
}
@@ -1298,7 +1298,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
return;
}
-gen_exception_insn(s, s->thumb ? 2 : 4, EXCP_UDEF, syn_uncategorized(),
+gen_exception_insn(s, EXCP_UDEF, syn_uncategorized(),
default_exception_el(s));
}
@@ -3175,7 +3175,7 @@ static bool msr_banked_access_decode(DisasContext *s, int
r, int sysm, int rn,
undef:
/* If we get here then some access check did not pass */
-gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(), exc_target);
+gen_exception_insn(s, EXCP_UDEF, syn_uncategorized(), exc_target);
return false;
}
@@ -3569,7 +3569,7 @@ static int disas_neon_ls_insn(DisasContext *s, uint32_t
insn)
* for attempts to execute invalid vfp/neon encodings with FP disabled.
*/
if (s->fp_excp_el) {
-gen_exception_insn(s, 4, EXCP_UDEF,
+gen_exception_insn(s, EXCP_UDEF,
syn_simd_access_trap(1, 0xe, false), s->fp_excp_el);
return 0;
}
@@ -4840,7 +4840,7 @@ static int disas_neon_data_insn(DisasContext *s, uint32_t
insn)
* for attempts to execute invalid vfp/neon encodings with FP disabled.
*/
if (s->fp_excp_el) {
-gen_exception_insn(s, 4, EXCP_UDEF,
+gen_exception_insn(s, EXCP_UDEF,
syn_simd_access_trap(1, 0xe, false), s->fp_excp_el);
return 0;
}
@@ -6968,7 +6968,7 @@ static int disas_neon_insn_3same_ext(DisasContext *s,
uint32_t insn)
}
if (s->fp_excp_el) {
-gen_exception_insn(s, 4, EXCP_UDEF,
+gen_exception_insn(s, EXCP_UDEF,
syn_simd_access_trap(1, 0xe, false), s->fp_excp_el);
return 0;
}
@@ -7091,7 +7091,7 @@ static int disas_neon_insn_2reg_scalar_ext(DisasContext
*s, uint32_t insn)
off_rm = vfp_reg_offset(0, rm);
}
if (s->fp_excp_el) {
-gen_exception_insn(s, 4, EXCP_UDEF,
+gen_exception_insn(s, EXCP_UDEF,
syn_simd_access_trap(1, 0xe, false), s->fp_excp_el);
return 0;
}
@@ -7584,7 +7584,7 @@ static void gen_srs(DisasContext *s,
* For the UNPREDICTABLE cases we choose to UNDEF.
*/
if (s->current_el == 1 && !s->ns && mode == ARM_CPU_MODE_MON) {
-gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(), 3);
+gen_exception_insn(s, EXCP_UDEF, syn_uncategorized(), 3);
return;
}
@@ -7620,7 +7620,7 @@ static void gen_srs(DisasContext *s,
}
if (undef) {
-gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),
+gen_exception_insn(s, EXCP_UDEF, syn_uncategorized(),
default_exception_el(s));
return;
}
@@ -7711,7 +7711,7
Re: [Qemu-devel] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4
On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis wrote: > Update the Hypervisor CSR addresses to match the v0.4 spec. > > Signed-off-by: Alistair Francis > --- > target/riscv/cpu_bits.h | 35 ++- > 1 file changed, 18 insertions(+), 17 deletions(-) > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > index 11f971ad5d..97b96c4e19 100644 > --- a/target/riscv/cpu_bits.h > +++ b/target/riscv/cpu_bits.h > @@ -173,6 +173,24 @@ > #define CSR_SPTBR 0x180 > #define CSR_SATP0x180 > > +/* Hpervisor CSRs */ > +#define CSR_HSTATUS 0x600 > +#define CSR_HEDELEG 0x602 > +#define CSR_HIDELEG 0x603 > +#define CSR_HCOUNTERNEN 0x606 > +#define CSR_HGATP 0x680 > + > +#if defined(TARGET_RISCV32) > +#define HGATP_MODE SATP32_MODE > +#define HGATP_ASID SATP32_ASID > +#define HGATP_PPNSATP32_PPN > +#endif > +#if defined(TARGET_RISCV64) > +#define HGATP_MODE SATP64_MODE > +#define HGATP_ASID SATP64_ASID > +#define HGATP_PPNSATP64_PPN > +#endif > + > Basd on spec, is HGATP_VMID preferable ? chihmin > /* Physical Memory Protection */ > #define CSR_PMPCFG0 0x3a0 > #define CSR_PMPCFG1 0x3a1 > @@ -206,23 +224,6 @@ > #define CSR_DPC 0x7b1 > #define CSR_DSCRATCH0x7b2 > > -/* Hpervisor CSRs */ > -#define CSR_HSTATUS 0xa00 > -#define CSR_HEDELEG 0xa02 > -#define CSR_HIDELEG 0xa03 > -#define CSR_HGATP 0xa80 > - > -#if defined(TARGET_RISCV32) > -#define HGATP_MODE SATP32_MODE > -#define HGATP_ASID SATP32_ASID > -#define HGATP_PPNSATP32_PPN > -#endif > -#if defined(TARGET_RISCV64) > -#define HGATP_MODE SATP64_MODE > -#define HGATP_ASID SATP64_ASID > -#define HGATP_PPNSATP64_PPN > -#endif > - > /* Performance Counters */ > #define CSR_MHPMCOUNTER30xb03 > #define CSR_MHPMCOUNTER40xb04 > -- > 2.22.0 > > >
[Qemu-devel] [PULL 1/1] linux-user: Make sigaltstack stacks per-thread
From: Peter Maydell
The alternate signal stack set up by the sigaltstack syscall is
supposed to be per-thread. We were incorrectly implementing it as
process-wide. This causes problems for guest binaries that rely on
this. Notably the Go runtime does, and so we were seeing crashes
caused by races where two guest threads might incorrectly both
execute on the same stack simultaneously.
Replace the global target_sigaltstack_used with a field
sigaltstack_used in the TaskState, and make all the references to the
old global instead get a pointer to the TaskState and use the field.
Fixes: https://bugs.launchpad.net/qemu/+bug/1696773
Signed-off-by: Peter Maydell
Reviewed-by: Richard Henderson
Tested-by: Laurent Vivier
Reviewed-by: Laurent Vivier
Message-Id: <20190725131645.19501-1-peter.mayd...@linaro.org>
Signed-off-by: Laurent Vivier
---
linux-user/hppa/signal.c | 3 ++-
linux-user/main.c | 5 +
linux-user/qemu.h | 2 ++
linux-user/signal-common.h | 1 -
linux-user/signal.c| 35 +++
5 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/linux-user/hppa/signal.c b/linux-user/hppa/signal.c
index b6927ee67351..d1a58feeb36f 100644
--- a/linux-user/hppa/signal.c
+++ b/linux-user/hppa/signal.c
@@ -111,10 +111,11 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
abi_ulong frame_addr, sp, haddr;
struct target_rt_sigframe *frame;
int i;
+TaskState *ts = (TaskState *)thread_cpu->opaque;
sp = get_sp_from_cpustate(env);
if ((ka->sa_flags & TARGET_SA_ONSTACK) && !sas_ss_flags(sp)) {
-sp = (target_sigaltstack_used.ss_sp + 0x7f) & ~0x3f;
+sp = (ts->sigaltstack_used.ss_sp + 0x7f) & ~0x3f;
}
frame_addr = QEMU_ALIGN_UP(sp, 64);
sp = frame_addr + PARISC_RT_SIGFRAME_SIZE32;
diff --git a/linux-user/main.c b/linux-user/main.c
index a59ae9439de1..8ffc52519552 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -180,6 +180,11 @@ void stop_all_tasks(void)
void init_task_state(TaskState *ts)
{
ts->used = 1;
+ts->sigaltstack_used = (struct target_sigaltstack) {
+.ss_sp = 0,
+.ss_size = 0,
+.ss_flags = TARGET_SS_DISABLE,
+};
}
CPUArchState *cpu_copy(CPUArchState *env)
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 4258e4162d26..aac033462700 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -151,6 +151,8 @@ typedef struct TaskState {
*/
int signal_pending;
+/* This thread's sigaltstack, if it has one */
+struct target_sigaltstack sigaltstack_used;
} __attribute__((aligned(16))) TaskState;
extern char *exec_path;
diff --git a/linux-user/signal-common.h b/linux-user/signal-common.h
index 51030a93069a..1df1068552fb 100644
--- a/linux-user/signal-common.h
+++ b/linux-user/signal-common.h
@@ -19,7 +19,6 @@
#ifndef SIGNAL_COMMON_H
#define SIGNAL_COMMON_H
-extern struct target_sigaltstack target_sigaltstack_used;
int on_sig_stack(unsigned long sp);
int sas_ss_flags(unsigned long sp);
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5cd237834d9a..5ca6d62b15d3 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -25,12 +25,6 @@
#include "trace.h"
#include "signal-common.h"
-struct target_sigaltstack target_sigaltstack_used = {
-.ss_sp = 0,
-.ss_size = 0,
-.ss_flags = TARGET_SS_DISABLE,
-};
-
static struct target_sigaction sigact_table[TARGET_NSIG];
static void host_signal_handler(int host_signum, siginfo_t *info,
@@ -251,13 +245,17 @@ void set_sigmask(const sigset_t *set)
int on_sig_stack(unsigned long sp)
{
-return (sp - target_sigaltstack_used.ss_sp
-< target_sigaltstack_used.ss_size);
+TaskState *ts = (TaskState *)thread_cpu->opaque;
+
+return (sp - ts->sigaltstack_used.ss_sp
+< ts->sigaltstack_used.ss_size);
}
int sas_ss_flags(unsigned long sp)
{
-return (target_sigaltstack_used.ss_size == 0 ? SS_DISABLE
+TaskState *ts = (TaskState *)thread_cpu->opaque;
+
+return (ts->sigaltstack_used.ss_size == 0 ? SS_DISABLE
: on_sig_stack(sp) ? SS_ONSTACK : 0);
}
@@ -266,17 +264,21 @@ abi_ulong target_sigsp(abi_ulong sp, struct
target_sigaction *ka)
/*
* This is the X/Open sanctioned signal stack switching.
*/
+TaskState *ts = (TaskState *)thread_cpu->opaque;
+
if ((ka->sa_flags & TARGET_SA_ONSTACK) && !sas_ss_flags(sp)) {
-return target_sigaltstack_used.ss_sp + target_sigaltstack_used.ss_size;
+return ts->sigaltstack_used.ss_sp + ts->sigaltstack_used.ss_size;
}
return sp;
}
void target_save_altstack(target_stack_t *uss, CPUArchState *env)
{
-__put_user(target_sigaltstack_used.ss_sp, >ss_sp);
+TaskState *ts = (TaskState *)thread_cpu->opaque;
+
+__put_user(ts->sigaltstack_used.ss_sp, >ss_sp);
__put_user(sas_ss_flags(get_sp_from_cpustate(env)), >ss_flags);
-__put_user(target_sigaltstack_used.ss_size, >ss_size);
[Qemu-devel] [PULL 0/1] Linux user for 4.1 patches
The following changes since commit fff3159900d2b95613a9cb75fc3703e67a674729: Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20190726' into staging (2019-07-26 16:23:07 +0100) are available in the Git repository at: git://github.com/vivier/qemu.git tags/linux-user-for-4.1-pull-request for you to fetch changes up to 5bfce0b74fbd5d53089bb866919d685c47edad9e: linux-user: Make sigaltstack stacks per-thread (2019-07-26 19:24:33 +0200) Fix multi-threaded go runtime crash Peter Maydell (1): linux-user: Make sigaltstack stacks per-thread linux-user/hppa/signal.c | 3 ++- linux-user/main.c | 5 + linux-user/qemu.h | 2 ++ linux-user/signal-common.h | 1 - linux-user/signal.c| 35 +++ 5 files changed, 28 insertions(+), 18 deletions(-) -- 2.21.0
Re: [Qemu-devel] [PATCH v3 0/3] qapi: block-dirty-bitmap-remove transaction action
24.07.2019 15:52, John Snow wrote: > > > On 7/24/19 7:12 AM, Vladimir Sementsov-Ogievskiy wrote: >> 15.07.2019 22:48, John Snow wrote: >>> >>> >>> On 7/8/19 6:04 PM, John Snow wrote: Hi, this is a proposal based off of Vladimir's patchset: [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action === V3: === 001/3:[] [--] 'blockdev: reduce aio_context locked sections in bitmap add/remove' 002/3:[0024] [FC] 'qapi: implement block-dirty-bitmap-remove transaction action' 003/3:[] [--] 'iotests: test bitmap moving inside 254' - Changed "squelch_persistence" to "skip_store" - Use Max's suggestion for return expr === V2: === It replaces patches two and three with a modified patch (now patch 2) that foregoes the need for a hide()/unhide() bitmap API. I think it's suitable as a smaller alternative, but I'm not sure if it covers all of the use cases of the original series. Patches 1 and 3 (formerly 4) included as-is. John Snow (1): qapi: implement block-dirty-bitmap-remove transaction action Vladimir Sementsov-Ogievskiy (2): blockdev: reduce aio_context locked sections in bitmap add/remove iotests: test bitmap moving inside 254 block.c| 2 +- block/dirty-bitmap.c | 15 +++-- blockdev.c | 105 ++--- include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 2 +- qapi/transaction.json | 2 + tests/qemu-iotests/254 | 30 +- tests/qemu-iotests/254.out | 82 + 8 files changed, 206 insertions(+), 34 deletions(-) >>> >>> Thanks, applied to my bitmaps tree: >>> >>> https://github.com/jnsnow/qemu/commits/bitmaps >>> https://github.com/jnsnow/qemu.git >>> >>> --js >>> >>> >>> (Vladimir: if this isn't amenable to you, it's going in for 4.2, so we >>> have until the next freeze to change it. Let me know, OK?) >>> >> >> >> And finally I'm here :) >> >> Thanks a lot for doing this job and for your explanations in other threads >> which >> I'm reading today and sorry for the delay! I'll look through these series >> soon. >> >> Actually, my second child (girl:) was born a month ago, and then her elder >> brother >> was ill, so I took two weeks sick leave after about two weeks vacation and >> forget >> about work for a month. >> >> Hmm. And Nikolay, who doing libvirt part is on vocation now (I started >> bitmap remove >> transaction series by his request), so I don't know about the end of the >> story with >> release and this functionality.. >> >> Anyway, it's cool, thanks! >> > > Wow! > > Congratulations Vladimir! > Thank you! -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH for-4.1] linux-user: Make sigaltstack stacks per-thread
On Fri, 26 Jul 2019 at 18:17, Laurent Vivier wrote: > Tested-by: Laurent Vivier > Reviewed-by: Laurent Vivier > > This patch seems also to fix failure of LTP test waitpid02. Well, that's a bonus :-) Could you submit a pullreq in time for rc3 (Tuesday), please? thanks -- PMM
Re: [Qemu-devel] [PATCH for-4.1] linux-user: Make sigaltstack stacks per-thread
Le 25/07/2019 à 15:16, Peter Maydell a écrit : > The alternate signal stack set up by the sigaltstack syscall is > supposed to be per-thread. We were incorrectly implementing it as > process-wide. This causes problems for guest binaries that rely on > this. Notably the Go runtime does, and so we were seeing crashes > caused by races where two guest threads might incorrectly both > execute on the same stack simultaneously. > > Replace the global target_sigaltstack_used with a field > sigaltstack_used in the TaskState, and make all the references to the > old global instead get a pointer to the TaskState and use the field. > > Fixes: https://bugs.launchpad.net/qemu/+bug/1696773 > Signed-off-by: Peter Maydell > --- > I've marked this as "for-4.1" but it is quite late in the release > cycle and I think this could use more testing than I have given it... > > Thanks are due to: > * the original bug reporter, for providing a nice simple test case > * rr, for allowing me to capture and forensically examine a single >example of the failure > * the Go project for having a good clear HACKING.md that explained >their stack usage and mentioned specifically that signal stacks >are per-thread (per-M, in their terms) > * a colleague, for prodding me into actually spending the necessary >two days grovelling through gdb sessions and logs to figure out >what was actually going wrong > --- > linux-user/qemu.h | 2 ++ > linux-user/signal-common.h | 1 - > linux-user/hppa/signal.c | 3 ++- > linux-user/main.c | 5 + > linux-user/signal.c| 35 +++ > 5 files changed, 28 insertions(+), 18 deletions(-) Tested-by: Laurent Vivier Reviewed-by: Laurent Vivier This patch seems also to fix failure of LTP test waitpid02. Thanks, Laurent
Re: [Qemu-devel] [PATCH for-3.1.1 0/2] tpm: Improve on error handling
Patchew URL: https://patchew.org/QEMU/20190726164921.1655115-1-stef...@linux.vnet.ibm.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Subject: [Qemu-devel] [PATCH for-3.1.1 0/2] tpm: Improve on error handling Message-id: 20190726164921.1655115-1-stef...@linux.vnet.ibm.com === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone' Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc' Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers' Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF' Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2' Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe' Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios' Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware' Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi' Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode' Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios' Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa' Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios' Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot' Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot' Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex' Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp' Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3' Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3' Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into 'capstone'... Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf' Cloning into 'dtc'... Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536' Cloning into 'roms/QemuMacDrivers'... Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266' Cloning into 'roms/SLOF'... Submodule path 'roms/SLOF': checked out 'ba1ab360eebe6338bb8d7d83a9220ccf7e213af3' Cloning into 'roms/edk2'... Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54' Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3' Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl' Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'... Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037' Cloning into 'CryptoPkg/Library/OpensslLib/openssl'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687' Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl' Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5' Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography' Cloning into 'boringssl'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f' Cloning into 'krb5'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168' Cloning into 'pyca-cryptography'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f' Cloning into 'roms/ipxe'... Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17' Cloning into 'roms/openbios'... Submodule path 'roms/openbios': checked out
Re: [Qemu-devel] [PATCH v3] tests/boot_linux_console: add a test for riscv64 + virt
On Thu, Jul 25, 2019 at 8:12 AM Alistair Francis
wrote:
> On Tue, Jul 23, 2019 at 11:46 PM Chih-Min Chao
> wrote:
> >
> > Similar to the mips + malta test, it boots a Linux kernel on a virt
> > board and verify the serial is working. Also, it relies on the serial
> > device set by the machine itself.
> >
> > If riscv64 is a target being built, "make check-acceptance" will
> > automatically include this test by the use of the "arch:riscv64" tags.
> >
> > Alternatively, this test can be run using:
> >
> > $ avocado run -t arch:riscv64 tests/acceptance
> >
> > packages
> > debian official
> > binutils-riscv64-linux-gnu_2.32-8
> > opensbi_0.4-1_all
> > linux-image-5.0.0-trunk-riscv64 5.0.2-1~exp1
> > third-party
> >
> https://github.com/groeck/linux-build-test/rootfs/riscv64/rootfs.cpio.gz
> > (the repo is also used in mips target acceptance)
> >
> > Signed-off-by: Chih-Min Chao
> > ---
> > .travis.yml| 2 +-
> > tests/acceptance/boot_linux_console.py | 67
> ++
> > 2 files changed, 68 insertions(+), 1 deletion(-)
> >
> > diff --git a/.travis.yml b/.travis.yml
> > index caf0a1f..7ba9952 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -232,7 +232,7 @@ matrix:
> >
> > # Acceptance (Functional) tests
> > - env:
> > -- CONFIG="--python=/usr/bin/python3
> --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu"
> > +- CONFIG="--python=/usr/bin/python3
> --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,riscv64-softmmu"
> > - TEST_CMD="make check-acceptance"
> >after_failure:
> > - cat tests/results/latest/job.log
> > diff --git a/tests/acceptance/boot_linux_console.py
> b/tests/acceptance/boot_linux_console.py
> > index 3215950..b0569b9 100644
> > --- a/tests/acceptance/boot_linux_console.py
> > +++ b/tests/acceptance/boot_linux_console.py
> > @@ -354,3 +354,70 @@ class BootLinuxConsole(Test):
> > self.vm.launch()
> > console_pattern = 'Kernel command line: %s' %
> kernel_command_line
> > self.wait_for_console_pattern(console_pattern)
> > +
> > +def test_riscv64_virt(self):
> > +"""
> > +:avocado: tags=arch:riscv64
> > +:avocado: tags=machine:virt
> > +"""
> > +deb_url = ('https://snapshot.debian.org/archive/debian/'
> > + '20190424T171759Z/pool/main/b/binutils/'
> > + 'binutils-riscv64-linux-gnu_2.32-8_amd64.deb')
> > +deb_hash = ('7fe376fd4452696c03acd508d6d613ca553ea15e')
> > +deb_path = self.fetch_asset(deb_url, asset_hash=deb_hash)
> > +objcopy_path = '/usr/bin/riscv64-linux-gnu-objcopy'
> > +objcopy_path = self.extract_from_deb(deb_path, objcopy_path)
> > +libbfd_path = '/usr/lib/x86_64-linux-gnu/libbfd-2.32-riscv64.so
> '
> > +libbfd_path = self.extract_from_deb(deb_path, libbfd_path)
> > +process.run('ls -al %s' % (objcopy_path))
>
> Why do we need objcopy? Won't this not work on non x86 architectures?
>
> > +
> > +deb_url = ('https://snapshot.debian.org/archive/debian/'
> > + '20190708T032337Z/pool/main/o/opensbi/'
> > + 'opensbi_0.4-1_all.deb')
> > +deb_hash = ('2319dcd702958291d323acf5649fd98a11d90112')
> > +deb_path = self.fetch_asset(deb_url, asset_hash=deb_hash)
> > +opensbi_path = ('/usr/lib/riscv64-linux-gnu/opensbi/'
> > +'qemu/virt/fw_jump.elf')
> > +opensbi_path = self.extract_from_deb(deb_path, opensbi_path)
> > +
> > +deb_url = ('https://snapshot.debian.org/archive/debian-ports/'
> > + '20190319T205124Z/pool-riscv64/main/l/linux/'
> > +
> 'linux-image-5.0.0-trunk-riscv64_5.0.2-1~exp1_riscv64.deb')
> > +deb_hash = ('90155ed4b36673cbf7746a37cf3159c8f0b2857a')
> > +deb_path = self.fetch_asset(deb_url, asset_hash=deb_hash)
> > +kernel_path = '/boot/vmlinux-5.0.0-trunk-riscv64'
>
> I thought we were swapping to using an Image file?
>
> Alistair
>
>
For objcopy,
Philippe had the same question in PATCH v2 thread. Debian
linux-kernel package only has vmlinux and riscv vmlinux ELF header doesn't
contain the correct physical loading address 0x8020_.
What I think is to extract the Image from vmlinux by objcopy.
This is what kernel do when generating arch/riscv/boot/Image.
For swapping to using an image file,
I think what you talk about is we have included opensbi.bin into
qemu master ?
The reason is the test use -initrd and -append to pass rootfs and
kernel command line option and the two options requires using of -kernel
option.
But -kernel option can't load the opensbi.bin to expected address.
chihmin
> > +kernel_path =
Re: [Qemu-devel] [PATCH 2/3] spapr/pci: Free MSIs during reset
On Fri, 26 Jul 2019 18:17:57 +0200
Philippe Mathieu-Daudé wrote:
> Cc'ing qemu-stable@
>
This patch relies on the previous one, otherwise g_hash_table_remove_all() will
just g_free() the spapr_pci_msi structures, but it will not tear down the MSIs
in the interrupt controller.
Also, this bug is so old that I'm not sure it qualifies for stable.
> On 7/26/19 4:44 PM, Greg Kurz wrote:
> > When the machine is reset, the MSI bitmap is cleared but the allocated
> > MSIs are not freed. Some operating systems, such as AIX, can detect the
> > previous configuration and assert.
> >
> > Empty the MSI cache, this performs the needed cleanup.
> >
> > Signed-off-by: Greg Kurz
> > ---
> > hw/ppc/spapr_pci.c |2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index bc22568bfa71..e45507bf2b53 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -2078,6 +2078,8 @@ static void spapr_phb_reset(DeviceState *qdev)
> > if (spapr_phb_eeh_available(SPAPR_PCI_HOST_BRIDGE(qdev))) {
> > spapr_phb_vfio_reset(qdev);
> > }
> > +
> > +g_hash_table_remove_all(sphb->msi);
>
> It is not clear to my why spapr_phb_unrealize() doesn't require the same
> call, but this is not related to this patch.
>
Because spapr_phb_unrealize() does this:
if (sphb->msi) {
g_hash_table_unref(sphb->msi);
sphb->msi = NULL;
}
and
https://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-unref
g_hash_table_unref ()
void
g_hash_table_unref (GHashTable *hash_table);
Atomically decrements the reference count of hash_table by one. If the reference
count drops to 0, all keys and values will be destroyed, and all memory
allocated
by the hash table is released. This function is MT-safe and may be called from
any
thread.
If I have to re-post, I can make it clear with a comment.
> Reviewed-by: Philippe Mathieu-Daudé
>
> > }
> >
> > static Property spapr_phb_properties[] = {
> >
> >
Re: [Qemu-devel] [PATCH for-3.1.1 0/2] tpm: Improve on error handling
Patchew URL: https://patchew.org/QEMU/20190726164921.1655115-1-stef...@linux.vnet.ibm.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Subject: [Qemu-devel] [PATCH for-3.1.1 0/2] tpm: Improve on error handling Message-id: 20190726164921.1655115-1-stef...@linux.vnet.ibm.com === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20190726164921.1655115-1-stef...@linux.vnet.ibm.com -> patchew/20190726164921.1655115-1-stef...@linux.vnet.ibm.com Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone' Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc' Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers' Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF' Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2' Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe' Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios' Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware' Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi' Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode' Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios' Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa' Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios' Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot' Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot' Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex' Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp' Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3' Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3' Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into 'capstone'... Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf' Cloning into 'dtc'... Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536' Cloning into 'roms/QemuMacDrivers'... Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266' Cloning into 'roms/SLOF'... Submodule path 'roms/SLOF': checked out 'ba1ab360eebe6338bb8d7d83a9220ccf7e213af3' Cloning into 'roms/edk2'... Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54' Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3' Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl' Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'... Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037' Cloning into 'CryptoPkg/Library/OpensslLib/openssl'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687' Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl' Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5' Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography' Cloning into 'boringssl'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f' Cloning into 'krb5'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168' Cloning into 'pyca-cryptography'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out
[Qemu-devel] [Bug 1838066] [NEW] unexpected error: raw_reconfigure_getfd(): qemu-system-x86_64: Could not reopen file
Public bug reported: Unexpected error in raw_reconfigure_getfd() at block/file-posix.c:923: qemu-system-x86_64: Could not reopen file: Permission denied Aborted Is what i sometimes (only) get, mostly for Linux guests i'd say (Arch just a few moments ago). This is on CRUX-Linux, thus a self-compiled qemu 4.0.0 with default recipe, without special compiler flags (-O2 -march=x86-64 -pipe) on an Intel i5 laptop. But what i do is running this via sudo: sudo='sudo --preserve-env=VMNAME,VMADDR' runas='-runas vm -chroot .' fi VMADDR=$addr VMNAME=$vmname export VMADDR VMNAME eval exec $sudo qemu-system-x86_64 -name $vmname $runas \ $host $accel $display $net $vmcustom $vmimg $redir the last run ends up like (via sudo) qemu-system-x86_64 -name arch-2019 -runas vm -chroot . -cpu host -m size=1984 -smp cpus=2 -enable-kvm -accel accel=kvm,thread=multi -display curses -net nic,netdev=net0,macaddr=.. -netdev tap,id=net0,script=./.ifup.sh,downscript=./.ifdown.sh,ifname=vm_arch-2019 vm is a user effectively living in the chroot only without any rights anywhere. Hope this helps, thanks a lot for qemu!! ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1838066 Title: unexpected error: raw_reconfigure_getfd(): qemu-system-x86_64: Could not reopen file Status in QEMU: New Bug description: Unexpected error in raw_reconfigure_getfd() at block/file-posix.c:923: qemu-system-x86_64: Could not reopen file: Permission denied Aborted Is what i sometimes (only) get, mostly for Linux guests i'd say (Arch just a few moments ago). This is on CRUX-Linux, thus a self-compiled qemu 4.0.0 with default recipe, without special compiler flags (-O2 -march=x86-64 -pipe) on an Intel i5 laptop. But what i do is running this via sudo: sudo='sudo --preserve-env=VMNAME,VMADDR' runas='-runas vm -chroot .' fi VMADDR=$addr VMNAME=$vmname export VMADDR VMNAME eval exec $sudo qemu-system-x86_64 -name $vmname $runas \ $host $accel $display $net $vmcustom $vmimg $redir the last run ends up like (via sudo) qemu-system-x86_64 -name arch-2019 -runas vm -chroot . -cpu host -m size=1984 -smp cpus=2 -enable-kvm -accel accel=kvm,thread=multi -display curses -net nic,netdev=net0,macaddr=.. -netdev tap,id=net0,script=./.ifup.sh,downscript=./.ifdown.sh,ifname=vm_arch-2019 vm is a user effectively living in the chroot only without any rights anywhere. Hope this helps, thanks a lot for qemu!! To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1838066/+subscriptions
Re: [Qemu-devel] [PATCH v4 6/7] monitor: adding tb_stats hmp command
vandersonmr writes:
> Adding tb_stats [start|pause|stop|filter] command to hmp.
> This allows controlling the collection of statistics.
> It is also possible to set the level of collection:
> all, jit, or exec.
>
> The goal of this command is to allow the dynamic exploration
> of the TCG behavior and quality. Therefore, for now, a
> corresponding QMP command is not worthwhile.
>
> Signed-off-by: Vanderson M. do Rosario
> ---
> accel/tcg/tb-stats.c| 107
> hmp-commands.hx | 17 +++
> include/exec/tb-stats.h | 13 +
> include/qemu/log.h | 1 +
> monitor/misc.c | 40 +++
> 5 files changed, 178 insertions(+)
>
> diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
> index 44497d4f9b..6c330e1b02 100644
> --- a/accel/tcg/tb-stats.c
> +++ b/accel/tcg/tb-stats.c
> @@ -6,6 +6,9 @@
>
> #include "qemu/qemu-print.h"
>
> +/* only accessed in safe work */
> +static GList *last_search;
> +
> struct jit_profile_info {
> uint64_t translations;
> uint64_t aborted;
> @@ -104,4 +107,108 @@ void dump_jit_profile_info(TCGProfile *s)
> }
> }
>
> +static void dessaloc_tbstats(void *p, uint32_t hash, void *userp)
> +{
> +g_free(p);
> +}
surely free_tbstats would be a better name?
> +
> +void clean_tbstats(void)
> +{
> +/* remove all tb_stats */
> +qht_iter(_ctx.tb_stats, dessaloc_tbstats, NULL);
> +qht_destroy(_ctx.tb_stats);
> +}
> +
> +static void reset_tbstats_flag(void *p, uint32_t hash, void *userp)
> +{
> +uint32_t flag = *((int *)userp);
> +TBStatistics *tbs = p;
> +tbs->stats_enabled = flag;
> +}
> +
> +void set_tbstats_flags(uint32_t flag)
> +{
> +/* iterate over tbstats setting their flag as TB_NOTHING */
> +qht_iter(_ctx.tb_stats, reset_tbstats_flag, );
> +}
> +
> +void do_hmp_tbstats_safe(CPUState *cpu, run_on_cpu_data icmd)
> +{
> +struct TbstatsCommand *cmdinfo = icmd.host_ptr;
> +int cmd = cmdinfo->cmd;
> +uint32_t level = cmdinfo->level;
> +
> +/* for safe, always pause TB generation while running this commands */
> +mmap_lock();
This doesn't serialise in all cases - in fact it's running as safe work
which is our serial case. The mmap_lock is really only needed for code
generation in linux-user. I think the lock/unlock can be dropped from
these operations.
> +
> +switch (cmd) {
> +case START:
> +if (tb_stats_collection_paused()) {
> +set_tbstats_flags(level);
> +} else {
> +if (tb_stats_collection_enabled()) {
> +qemu_printf("TB information already being recorded");
> +return;
> +}
> +qht_init(_ctx.tb_stats, tb_stats_cmp, CODE_GEN_HTABLE_SIZE,
> +QHT_MODE_AUTO_RESIZE);
I'm wary of initialising this in multiple places. Maybe move this to
enable_collect_tb_stats and drop the initialisation in tb_htable_init.
> +}
> +
> +set_default_tbstats_flag(level);
> +enable_collect_tb_stats();
> +tb_flush(cpu);
> +break;
> +case PAUSE:
> +if (!tb_stats_collection_enabled()) {
> +qemu_printf("TB information not being recorded");
> +return;
> +}
> +
> +/* Continue to create TBStatistic structures but stop collecting
> statistics */
> +pause_collect_tb_stats();
> +tb_flush(cpu);
> +set_default_tbstats_flag(TB_NOTHING);
> +set_tbstats_flags(TB_PAUSED);
Minor style suggestion - make the tb_flush the last thing you do once
you have moved things around. You are in safe work so nothing should be
happening anyway and it reads better.
> +break;
> +case STOP:
> +if (!tb_stats_collection_enabled()) {
> +qemu_printf("TB information not being recorded");
> +return;
> +}
> +
> +/* Dissalloc all TBStatistics structures and stop creating new ones
> */
> +disable_collect_tb_stats();
> +tb_flush(cpu);
> +clean_tbstats();
As above.
> +break;
> +case FILTER:
> +if (!tb_stats_collection_enabled()) {
> +qemu_printf("TB information not being recorded");
> +return;
> +}
> +if (!last_search) {
> +qemu_printf("no search on record! execute info tbs before
> filtering!");
> +return;
> +}
> +
> +tb_flush(cpu);
> +set_default_tbstats_flag(TB_NOTHING);
> +
> +/* Set all tbstats as paused, then return only the ones from
> last_search */
> +pause_collect_tb_stats();
> +set_tbstats_flags(TB_PAUSED);
> +
> +for (GList *iter = last_search; iter; iter = g_list_next(iter)) {
> +TBStatistics *tbs = iter->data;
> +tbs->stats_enabled = level;
> +}
If we are only interested in tracking the N hotestest at this point we
want to prevent allocation of new TBStats and free all the unused
Re: [Qemu-devel] [PATCH 12/28] Include hw/irq.h a lot less
On Fri, Jul 26, 2019 at 5:10 AM Markus Armbruster wrote: > > In my "build everything" tree, changing hw/irq.h triggers a recompile > of some 5400 out of 6600 objects (not counting tests and objects that > don't depend on qemu/osdep.h). > > hw/hw.h supposedly includes it for convenience. Several other headers > include it just to get qemu_irq and.or qemu_irq_handler. > > Move the qemu_irq and qemu_irq_handler typedefs from hw/irq.h to > qemu/typedefs.h, and then include hw/irq.h only where it's still > needed. Touching it now recompiles only some 500 objects. > > Signed-off-by: Markus Armbruster Reviewed-by: Alistair Francis Alistair > --- > hw/acpi/core.c | 1 + > hw/acpi/piix4.c | 1 + > hw/alpha/alpha_sys.h | 1 - > hw/alpha/typhoon.c | 1 + > hw/arm/armsse.c | 1 + > hw/arm/exynos4210.c | 1 + > hw/arm/exynos4_boards.c | 1 + > hw/arm/integratorcp.c| 1 + > hw/arm/msf2-soc.c| 1 + > hw/arm/musicpal.c| 1 + > hw/arm/omap1.c | 1 + > hw/arm/omap2.c | 1 + > hw/arm/palm.c| 2 ++ > hw/arm/pxa2xx.c | 1 + > hw/arm/pxa2xx_gpio.c | 1 + > hw/arm/realview.c| 1 + > hw/arm/smmuv3.c | 1 + > hw/arm/spitz.c | 1 + > hw/arm/stellaris.c | 1 + > hw/arm/strongarm.c | 1 + > hw/arm/tosa.c| 1 + > hw/arm/versatilepb.c | 1 + > hw/arm/virt.c| 1 + > hw/arm/z2.c | 1 + > hw/audio/cs4231a.c | 1 + > hw/audio/gus.c | 1 + > hw/audio/marvell_88w8618.c | 1 + > hw/audio/milkymist-ac97.c| 1 + > hw/audio/pl041.c | 1 + > hw/audio/sb16.c | 1 + > hw/block/fdc.c | 1 + > hw/char/bcm2835_aux.c| 1 + > hw/char/cadence_uart.c | 1 + > hw/char/cmsdk-apb-uart.c | 1 + > hw/char/escc.c | 1 + > hw/char/etraxfs_ser.c| 1 + > hw/char/exynos4210_uart.c| 1 + > hw/char/grlib_apbuart.c | 1 + > hw/char/imx_serial.c | 1 + > hw/char/ipoctal232.c | 1 + > hw/char/lm32_uart.c | 1 + > hw/char/mcf_uart.c | 1 + > hw/char/milkymist-uart.c | 1 + > hw/char/nrf51_uart.c | 1 + > hw/char/parallel.c | 1 + > hw/char/pl011.c | 1 + > hw/char/serial-pci-multi.c | 1 + > hw/char/serial-pci.c | 1 + > hw/char/serial.c | 1 + > hw/char/sh_serial.c | 2 ++ > hw/char/spapr_vty.c | 1 + > hw/char/stm32f2xx_usart.c| 1 + > hw/char/xilinx_uartlite.c| 1 + > hw/core/or-irq.c | 1 + > hw/core/qdev.c | 1 + > hw/core/split-irq.c | 1 + > hw/cpu/a15mpcore.c | 1 + > hw/cpu/a9mpcore.c| 1 + > hw/cpu/arm11mpcore.c | 1 + > hw/cpu/realview_mpcore.c | 1 + > hw/display/ads7846.c | 1 + > hw/display/bcm2835_fb.c | 1 + > hw/display/cg3.c | 1 + > hw/display/exynos4210_fimd.c | 1 + > hw/display/g364fb.c | 1 + > hw/display/milkymist-tmu2.c | 1 + > hw/display/omap_dss.c| 2 ++ > hw/display/omap_lcdc.c | 2 ++ > hw/display/pl110.c | 1 + > hw/display/pxa2xx_lcd.c | 1 + > hw/display/tc6393xb.c| 2 ++ > hw/display/xlnx_dp.c | 1 + > hw/dma/bcm2835_dma.c | 1 + > hw/dma/etraxfs_dma.c | 2 ++ > hw/dma/pl080.c | 1 + > hw/dma/pl330.c | 1 + > hw/dma/pxa2xx_dma.c | 1 + > hw/dma/rc4030.c | 1 + > hw/dma/sparc32_dma.c | 1 + > hw/dma/xilinx_axidma.c | 1 + > hw/dma/xlnx-zdma.c | 1 + > hw/dma/xlnx-zynq-devcfg.c| 1 + > hw/dma/xlnx_dpdma.c | 1 + > hw/gpio/bcm2835_gpio.c | 1 + > hw/gpio/gpio_key.c | 1 + > hw/gpio/imx_gpio.c | 1 + > hw/gpio/max7310.c| 1 + > hw/gpio/mpc8xxx.c| 1 + > hw/gpio/nrf51_gpio.c | 1 + > hw/gpio/omap_gpio.c | 1 + > hw/gpio/pl061.c | 1 + > hw/gpio/zaurus.c | 1 + > hw/hppa/dino.c | 1 + > hw/hppa/hppa_sys.h | 1 - > hw/i2c/aspeed_i2c.c | 1 + > hw/i2c/bitbang_i2c.c | 1 + > hw/i2c/exynos4210_i2c.c | 1 + > hw/i2c/imx_i2c.c | 1 + > hw/i2c/mpc_i2c.c | 1 + > hw/i2c/omap_i2c.c| 1 + > hw/i2c/ppc4xx_i2c.c | 1 + > hw/i386/kvm/i8259.c | 1 + > hw/i386/kvm/ioapic.c | 1 + >
Re: [Qemu-devel] [PATCH 01/28] include: Make headers more self-contained
On Fri, Jul 26, 2019 at 5:08 AM Markus Armbruster wrote: > > Back in 2016, we discussed[1] rules for headers, and these were > generally liked: > > 1. Have a carefully curated header that's included everywhere first. We >got that already thanks to Peter: osdep.h. > > 2. Headers should normally include everything they need beyond osdep.h. >If exceptions are needed for some reason, they must be documented in >the header. If all that's needed from a header is typedefs, put >those into qemu/typedefs.h instead of including the header. > > 3. Cyclic inclusion is forbidden. > > This patch gets include/ closer to obeying 2. > > It's actually extracted from my "[RFC] Baby steps towards saner > headers" series[2], which demonstrates a possible path towards > checking 2 automatically. It passes the RFC test there. > > [1] Message-ID: <87h9g8j57d@blackfin.pond.sub.org> > https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html > [2] Message-Id: <20190711122827.18970-1-arm...@redhat.com> > https://lists.nongnu.org/archive/html/qemu-devel/2019-07/msg02715.html > > Signed-off-by: Markus Armbruster Reviewed-by: Alistair Francis Alistair > --- > include/block/raw-aio.h | 1 + > include/block/write-threshold.h | 1 + > include/disas/disas.h | 1 + > include/exec/cputlb.h | 2 ++ > include/exec/exec-all.h | 1 + > include/exec/ioport.h | 2 ++ > include/exec/memory-internal.h| 2 ++ > include/exec/ram_addr.h | 1 + > include/exec/softmmu-semi.h | 2 ++ > include/exec/tb-hash.h| 2 ++ > include/exec/user/thunk.h | 1 + > include/fpu/softfloat-macros.h| 2 ++ > include/hw/acpi/pci.h | 2 ++ > include/hw/acpi/tco.h | 2 ++ > include/hw/adc/stm32f2xx_adc.h| 2 ++ > include/hw/arm/allwinner-a10.h| 1 + > include/hw/arm/aspeed_soc.h | 1 + > include/hw/arm/bcm2836.h | 1 + > include/hw/arm/exynos4210.h | 2 +- > include/hw/arm/fsl-imx25.h| 1 + > include/hw/arm/fsl-imx31.h| 1 + > include/hw/arm/sharpsl.h | 2 ++ > include/hw/arm/xlnx-zynqmp.h | 1 + > include/hw/block/fdc.h| 2 ++ > include/hw/block/flash.h | 1 + > include/hw/char/escc.h| 1 + > include/hw/char/xilinx_uartlite.h | 2 ++ > include/hw/core/generic-loader.h | 1 + > include/hw/cris/etraxfs.h | 1 + > include/hw/cris/etraxfs_dma.h | 3 +++ > include/hw/display/i2c-ddc.h | 1 + > include/hw/empty_slot.h | 2 ++ > include/hw/gpio/bcm2835_gpio.h| 1 + > include/hw/i2c/aspeed_i2c.h | 1 + > include/hw/i386/apic_internal.h | 1 + > include/hw/i386/ioapic_internal.h | 1 + > include/hw/intc/allwinner-a10-pic.h | 2 ++ > include/hw/intc/heathrow_pic.h| 2 ++ > include/hw/intc/mips_gic.h| 1 + > include/hw/isa/vt82c686.h | 2 ++ > include/hw/mips/cps.h | 1 + > include/hw/misc/macio/cuda.h | 2 ++ > include/hw/misc/macio/gpio.h | 3 +++ > include/hw/misc/macio/macio.h | 2 ++ > include/hw/misc/macio/pmu.h | 3 +++ > include/hw/misc/mips_cmgcr.h | 2 ++ > include/hw/misc/mips_cpc.h| 2 ++ > include/hw/misc/pvpanic.h | 2 ++ > include/hw/net/allwinner_emac.h | 1 + > include/hw/net/lance.h| 1 + > include/hw/nvram/chrp_nvram.h | 2 ++ > include/hw/pci-host/sabre.h | 2 ++ > include/hw/pci-host/uninorth.h| 2 +- > include/hw/pci/pcie_aer.h | 1 + > include/hw/ppc/pnv_core.h | 1 + > include/hw/ppc/ppc4xx.h | 4 > include/hw/ppc/spapr_irq.h| 3 +++ > include/hw/ppc/spapr_vio.h| 1 + > include/hw/ppc/spapr_xive.h | 2 ++ > include/hw/ppc/xive_regs.h| 3 +++ > include/hw/riscv/boot.h | 2 ++ > include/hw/riscv/riscv_hart.h | 3 +++ > include/hw/riscv/sifive_clint.h | 2 ++ > include/hw/riscv/sifive_e.h | 1 + > include/hw/riscv/sifive_plic.h| 2 +- > include/hw/riscv/sifive_prci.h| 2 ++ > include/hw/riscv/sifive_test.h| 2 ++ > include/hw/riscv/sifive_u.h | 1 + > include/hw/riscv/sifive_uart.h| 3 +++ > include/hw/riscv/spike.h | 3 +++ > include/hw/riscv/virt.h | 3 +++ > include/hw/s390x/ap-device.h | 2 ++ > include/hw/s390x/css-bridge.h | 3 ++- > include/hw/s390x/css.h| 1 + > include/hw/s390x/tod.h| 2 +- > include/hw/semihosting/console.h | 2 ++ > include/hw/sh4/sh_intc.h | 1 + > include/hw/sparc/sparc64.h| 2 ++ > include/hw/ssi/aspeed_smc.h | 1 + >
Re: [Qemu-devel] [PATCH 08/28] Include sysemu/reset.h a lot less
On Fri, Jul 26, 2019 at 9:03 AM Philippe Mathieu-Daudé wrote: > > On 7/26/19 2:05 PM, Markus Armbruster wrote: > > In my "build everything" tree, changing sysemu/reset.h triggers a > > recompile of some 2600 out of 6600 objects (not counting tests and > > objects that don't depend on qemu/osdep.h). > > > > The main culprit is hw/hw.h, which supposedly includes it for > > convenience. > > > > Include sysemu/reset.h only where it's needed. Touching it now > > recompiles less than 200 objects. > > > > Signed-off-by: Markus Armbruster > > Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Alistair > > > --- > > hw/acpi/ich9.c | 2 ++ > > hw/acpi/piix4.c| 2 ++ > > hw/acpi/vmgenid.c | 1 + > > hw/arm/armv7m.c| 1 + > > hw/arm/boot.c | 1 + > > hw/arm/nseries.c | 1 + > > hw/arm/omap1.c | 1 + > > hw/arm/omap2.c | 1 + > > hw/arm/virt-acpi-build.c | 1 + > > hw/char/parallel.c | 1 + > > hw/char/serial.c | 1 + > > hw/core/generic-loader.c | 1 + > > hw/core/loader.c | 1 + > > hw/cris/boot.c | 1 + > > hw/display/cirrus_vga.c| 1 + > > hw/display/ramfb.c | 2 ++ > > hw/display/vga.c | 2 ++ > > hw/hppa/machine.c | 1 + > > hw/i386/acpi-build.c | 1 + > > hw/i386/pc.c | 1 + > > hw/ide/cmd646.c| 1 + > > hw/ide/piix.c | 1 + > > hw/ide/sii3112.c | 1 + > > hw/ide/via.c | 1 + > > hw/input/lm832x.c | 1 + > > hw/input/pckbd.c | 2 ++ > > hw/input/ps2.c | 2 ++ > > hw/input/tsc2005.c | 1 + > > hw/input/tsc210x.c | 1 + > > hw/intc/mips_gic.c | 1 + > > hw/intc/pnv_xive.c | 1 + > > hw/intc/spapr_xive.c | 1 + > > hw/intc/xics.c | 1 + > > hw/intc/xive.c | 1 + > > hw/isa/piix4.c | 1 + > > hw/isa/vt82c686.c | 1 + > > hw/lm32/lm32_boards.c | 1 + > > hw/lm32/milkymist.c| 1 + > > hw/microblaze/boot.c | 1 + > > hw/mips/cps.c | 1 + > > hw/mips/mips_fulong2e.c| 1 + > > hw/mips/mips_jazz.c| 1 + > > hw/mips/mips_malta.c | 1 + > > hw/mips/mips_mipssim.c | 2 ++ > > hw/mips/mips_r4k.c | 2 ++ > > hw/misc/vmcoreinfo.c | 1 + > > hw/moxie/moxiesim.c| 2 ++ > > hw/net/eepro100.c | 1 + > > hw/nios2/boot.c| 1 + > > hw/nvram/fw_cfg.c | 1 + > > hw/openrisc/openrisc_sim.c | 1 + > > hw/pci-host/bonito.c | 1 + > > hw/pci-host/piix.c | 1 + > > hw/ppc/e500.c | 1 + > > hw/ppc/mac_newworld.c | 1 + > > hw/ppc/mac_oldworld.c | 1 + > > hw/ppc/pnv.c | 1 + > > hw/ppc/pnv_core.c | 1 + > > hw/ppc/pnv_psi.c | 1 + > > hw/ppc/ppc405_boards.c | 2 ++ > > hw/ppc/ppc405_uc.c | 2 ++ > > hw/ppc/ppc440_bamboo.c | 1 + > > hw/ppc/ppc440_uc.c | 1 + > > hw/ppc/ppc4xx_devs.c | 2 ++ > > hw/ppc/ppc4xx_pci.c| 1 + > > hw/ppc/ppc_booke.c | 2 ++ > > hw/ppc/prep.c | 2 ++ > > hw/ppc/sam460ex.c | 1 + > > hw/ppc/spapr.c | 1 + > > hw/ppc/spapr_cpu_core.c| 2 ++ > > hw/ppc/spapr_drc.c | 1 + > > hw/ppc/virtex_ml507.c | 1 + > > hw/riscv/riscv_hart.c | 1 + > > hw/s390x/ipl.c | 1 + > > hw/s390x/s390-virtio-ccw.c | 1 + > > hw/sh4/r2d.c | 1 + > > hw/sparc/leon3.c | 2 ++ > > hw/sparc/sun4m.c | 2 ++ > > hw/sparc64/sparc64.c | 1 + > > hw/timer/etraxfs_timer.c | 1 + > > hw/timer/mc146818rtc.c | 1 + > > hw/tpm/tpm_ppi.c | 1 - > > hw/vfio/common.c | 1 + > > hw/watchdog/wdt_diag288.c | 1 + > > hw/xtensa/sim.c| 1 + > > hw/xtensa/xtfpga.c | 1 + > > include/hw/hw.h| 1 - > > target/i386/cpu.c | 1 + > > target/i386/hax-all.c | 1 + > > target/i386/kvm.c | 1 + > > target/s390x/cpu.c | 1 + > > vl.c | 1 + > > 92 files changed, 107 insertions(+), 2 deletions(-) > > > > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c > > index e53dfe1ee3..b4d987c811 100644 > > --- a/hw/acpi/ich9.c > > +++ b/hw/acpi/ich9.c > > @@ -23,6 +23,7 @@ > > * Contributions after 2012-01-13 are licensed under the terms of the > > * GNU GPL, version 2 or (at your option) any later version. > > */ > > + > > #include "qemu/osdep.h" > > #include "hw/hw.h" > > #include "qapi/error.h" > > @@ -30,6 +31,7 @@ > > #include "hw/i386/pc.h" > > #include "hw/pci/pci.h" > > #include "qemu/timer.h" > > +#include "sysemu/reset.h" > > #include "sysemu/sysemu.h" > > #include "hw/acpi/acpi.h" > > #include "hw/acpi/tco.h" > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > > index ec4e186cec..a59e58d937 100644 > > --- a/hw/acpi/piix4.c > > +++ b/hw/acpi/piix4.c > > @@ -18,6
[Qemu-devel] [PATCH for-3.1.1 1/2] tpm: Exit in reset when backend indicates failure
Exit() in the frontend reset function when the backend indicates
intialization failure.
Signed-off-by: Stefan Berger
Reviewed-by: Marc-André Lureau
(git cherry-pick bcfd16fe26d6bb6eabfd2dfb46b9fda59d5493db)
---
hw/tpm/tpm_crb.c | 4 +++-
hw/tpm/tpm_tis.c | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index a92dd50437..e062f597f9 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -265,7 +265,9 @@ static void tpm_crb_reset(void *dev)
s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->tpmbe),
CRB_CTRL_CMD_SIZE);
-tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size);
+if (tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size) < 0) {
+exit(1);
+}
}
static void tpm_crb_realize(DeviceState *dev, Error **errp)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index d9322692ee..5c218fc7fc 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -891,7 +891,9 @@ static void tpm_tis_reset(DeviceState *dev)
s->rw_offset = 0;
}
-tpm_backend_startup_tpm(s->be_driver, s->be_buffer_size);
+if (tpm_backend_startup_tpm(s->be_driver, s->be_buffer_size) < 0) {
+exit(1);
+}
}
/* persistent state handling */
--
2.20.1
[Qemu-devel] [PATCH for-3.1.1 2/2] tpm_emulator: Translate TPM error codes to strings
Implement a function to translate TPM error codes to strings so that
at least the most common error codes can be translated to human
readable strings.
Signed-off-by: Stefan Berger
Reviewed-by: Marc-André Lureau
(git cherry-pick 7e095e84ba0b7c0a1ac45bc6824dace2fd352e56)
---
hw/tpm/tpm_emulator.c | 60 +++
hw/tpm/tpm_int.h | 13 ++
2 files changed, 63 insertions(+), 10 deletions(-)
diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 70f4b10284..58894dc320 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -81,6 +81,40 @@ typedef struct TPMEmulator {
TPMBlobBuffers state_blobs;
} TPMEmulator;
+struct tpm_error {
+uint32_t tpm_result;
+const char *string;
+};
+
+static const struct tpm_error tpm_errors[] = {
+/* TPM 1.2 error codes */
+{ TPM_BAD_PARAMETER , "a parameter is bad" },
+{ TPM_FAIL, "operation failed" },
+{ TPM_KEYNOTFOUND , "key could not be found" },
+{ TPM_BAD_PARAM_SIZE , "bad parameter size"},
+{ TPM_ENCRYPT_ERROR , "encryption error" },
+{ TPM_DECRYPT_ERROR , "decryption error" },
+{ TPM_BAD_KEY_PROPERTY, "bad key property" },
+{ TPM_BAD_MODE, "bad (encryption) mode" },
+{ TPM_BAD_VERSION , "bad version identifier" },
+{ TPM_BAD_LOCALITY, "bad locality" },
+/* TPM 2 error codes */
+{ TPM_RC_FAILURE , "operation failed" },
+{ TPM_RC_LOCALITY, "bad locality" },
+{ TPM_RC_INSUFFICIENT, "insufficient amount of data" },
+};
+
+static const char *tpm_emulator_strerror(uint32_t tpm_result)
+{
+size_t i;
+
+for (i = 0; i < ARRAY_SIZE(tpm_errors); i++) {
+if (tpm_errors[i].tpm_result == tpm_result) {
+return tpm_errors[i].string;
+}
+}
+return "";
+}
static int tpm_emulator_ctrlcmd(TPMEmulator *tpm, unsigned long cmd, void *msg,
size_t msg_len_in, size_t msg_len_out)
@@ -263,7 +297,8 @@ static int tpm_emulator_stop_tpm(TPMBackend *tb)
res = be32_to_cpu(res);
if (res) {
-error_report("tpm-emulator: TPM result for CMD_STOP: 0x%x", res);
+error_report("tpm-emulator: TPM result for CMD_STOP: 0x%x %s", res,
+ tpm_emulator_strerror(res));
return -1;
}
@@ -292,8 +327,9 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
psbs.u.resp.tpm_result = be32_to_cpu(psbs.u.resp.tpm_result);
if (psbs.u.resp.tpm_result != 0) {
-error_report("tpm-emulator: TPM result for set buffer size : 0x%x",
- psbs.u.resp.tpm_result);
+error_report("tpm-emulator: TPM result for set buffer size : 0x%x %s",
+ psbs.u.resp.tpm_result,
+ tpm_emulator_strerror(psbs.u.resp.tpm_result));
return -1;
}
@@ -338,7 +374,8 @@ static int tpm_emulator_startup_tpm_resume(TPMBackend *tb,
size_t buffersize,
res = be32_to_cpu(init.u.resp.tpm_result);
if (res) {
-error_report("tpm-emulator: TPM result for CMD_INIT: 0x%x", res);
+error_report("tpm-emulator: TPM result for CMD_INIT: 0x%x %s", res,
+ tpm_emulator_strerror(res));
goto err_exit;
}
return 0;
@@ -398,8 +435,9 @@ static int
tpm_emulator_reset_tpm_established_flag(TPMBackend *tb,
res = be32_to_cpu(reset_est.u.resp.tpm_result);
if (res) {
-error_report("tpm-emulator: TPM result for rest establixhed flag:
0x%x",
- res);
+error_report(
+"tpm-emulator: TPM result for rest established flag: 0x%x %s",
+res, tpm_emulator_strerror(res));
return -1;
}
@@ -637,7 +675,8 @@ static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
res = be32_to_cpu(pgs.u.resp.tpm_result);
if (res != 0 && (res & 0x800) == 0) {
error_report("tpm-emulator: Getting the stateblob (type %d) failed "
- "with a TPM error 0x%x", type, res);
+ "with a TPM error 0x%x %s", type, res,
+ tpm_emulator_strerror(res));
return -1;
}
@@ -757,7 +796,8 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
tpm_result = be32_to_cpu(pss.u.resp.tpm_result);
if (tpm_result != 0) {
error_report("tpm-emulator: Setting the stateblob (type %d) failed "
- "with a TPM error 0x%x", type, tpm_result);
+ "with a TPM error 0x%x %s", type, tpm_result,
+ tpm_emulator_strerror(tpm_result));
return -1;
}
@@ -887,8 +927,8 @@ static void tpm_emulator_shutdown(TPMEmulator *tpm_emu)
error_report("tpm-emulator: Could not cleanly shutdown the TPM: %s",
strerror(errno));
} else if (res != 0) {
-error_report("tpm-emulator: TPM result for sutdown: 0x%x",
- be32_to_cpu(res));
+
[Qemu-devel] [PATCH for-3.1.1 0/2] tpm: Improve on error handling
This series of patches improves error handling with the TPM backend. The have just been applied to master and can be cherry-pick'ed. I added the cherry-pick command in the 2 patches. Stefan Stefan Berger (2): tpm: Exit in reset when backend indicates failure tpm_emulator: Translate TPM error codes to strings hw/tpm/tpm_crb.c | 4 ++- hw/tpm/tpm_emulator.c | 60 +++ hw/tpm/tpm_int.h | 13 ++ hw/tpm/tpm_tis.c | 4 ++- 4 files changed, 69 insertions(+), 12 deletions(-) -- 2.20.1
Re: [Qemu-devel] [PATCH v4 5/7] log: adding -d tb_stats to control tbstats
vandersonmr writes:
> Adding -d tb_stats:[limit:[all|jit|exec]] to control TBStatistics
> collection. "limit" is used to limit the number of TBStats in the
> linux-user dump. [all|jit|exec] control the profilling level used
> by the TBStats: all, only jit stats or only execution count stats.
>
> Signed-off-by: Vanderson M. do Rosario
> ---
> include/qemu/log.h | 1 +
> util/log.c | 34 ++
> 2 files changed, 35 insertions(+)
>
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index 8cdfc268ca..93642603e5 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -45,6 +45,7 @@ static inline bool qemu_log_separate(void)
> /* LOG_TRACE (1 << 15) is defined in log-for-trace.h */
> #define CPU_LOG_TB_OP_IND (1 << 16)
> #define CPU_LOG_TB_FPU (1 << 17)
> +#define CPU_LOG_TB_STATS (1 << 18)
>
> /* Lock output for a series of related logs. Since this is not needed
> * for a single qemu_log / qemu_log_mask / qemu_log_mask_and_addr, we
> diff --git a/util/log.c b/util/log.c
> index f81653d712..8109d19afb 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -30,6 +30,7 @@ FILE *qemu_logfile;
> int qemu_loglevel;
> static int log_append = 0;
> static GArray *debug_regions;
> +int32_t max_num_hot_tbs_to_dump;
>
> /* Return the number of characters emitted. */
> int qemu_log(const char *fmt, ...)
> @@ -273,6 +274,8 @@ const QEMULogItem qemu_log_items[] = {
> { CPU_LOG_TB_NOCHAIN, "nochain",
>"do not chain compiled TBs so that \"exec\" and \"cpu\" show\n"
>"complete traces" },
> +{ CPU_LOG_TB_STATS, "tb_stats[:limit:(all,jit,exec)]",
> + "show TBs statistics (until given a limit) ordered by their
> hotness.\n" },
> { 0, NULL, NULL },
> };
>
> @@ -294,6 +297,37 @@ int qemu_str_to_log_mask(const char *str)
> trace_enable_events((*tmp) + 6);
> mask |= LOG_TRACE;
> #endif
> +} else if (g_str_has_prefix(*tmp, "tb_stats")) {
> +if (g_str_has_prefix(*tmp, "tb_stats:") && (*tmp)[9] != '\0') {
> +
> +if (!g_ascii_isdigit(*((*tmp) + 9))) {
> +fprintf(stderr,
> +"should be a number follow by [all|jit|exec], as
> tb_stats:10:all\n");
> +exit(1);
> +}
> +/* get limit */
> +max_num_hot_tbs_to_dump = atoi((*tmp) + 9);
We probably want to handle -d tb_stats:[all|jit|exec] as well because we
might be doing interactive exploration for softmmu. I'd suggest
splitting the number processing off to a new patch and merging the exit
processing part of the later patches with it. As you don't have a HMP
for linux-user you can make the number required for linux-user or only
optional for softmmu.
> +
> +/* get profilling level */
> +char *s = (*tmp) + 9;
> +while (*s != '\0') {
> +if (*s++ == ':') {
> +break;
> +}
> +}
> +if (g_str_equal(s, "jit") == 0) {
> +set_default_tbstats_flag(TB_JIT_STATS);
> +} else if (g_str_equal(s, "exec") == 0) {
> +set_default_tbstats_flag(TB_EXEC_STATS);
> +} else {
> +set_default_tbstats_flag(TB_JIT_STATS | TB_EXEC_STATS);
> +}
If additional stats types get added we shall want to be additive:
-d host_tbs:jit,exec
> +} else {
> +set_default_tbstats_flag(TB_JIT_STATS | TB_EXEC_STATS);
> +}
> +
> +mask |= CPU_LOG_TB_STATS;
> +enable_collect_tb_stats();
> } else {
> for (item = qemu_log_items; item->mask != 0; item++) {
> if (g_str_equal(*tmp, item->name)) {
--
Alex Bennée
Re: [Qemu-devel] [PATCH 2/3] spapr/pci: Free MSIs during reset
Cc'ing qemu-stable@
On 7/26/19 4:44 PM, Greg Kurz wrote:
> When the machine is reset, the MSI bitmap is cleared but the allocated
> MSIs are not freed. Some operating systems, such as AIX, can detect the
> previous configuration and assert.
>
> Empty the MSI cache, this performs the needed cleanup.
>
> Signed-off-by: Greg Kurz
> ---
> hw/ppc/spapr_pci.c |2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index bc22568bfa71..e45507bf2b53 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2078,6 +2078,8 @@ static void spapr_phb_reset(DeviceState *qdev)
> if (spapr_phb_eeh_available(SPAPR_PCI_HOST_BRIDGE(qdev))) {
> spapr_phb_vfio_reset(qdev);
> }
> +
> +g_hash_table_remove_all(sphb->msi);
It is not clear to my why spapr_phb_unrealize() doesn't require the same
call, but this is not related to this patch.
Reviewed-by: Philippe Mathieu-Daudé
> }
>
> static Property spapr_phb_properties[] = {
>
>
Re: [Qemu-devel] [PULL 0/5] target-arm queue
On Fri, 26 Jul 2019 at 16:19, Peter Maydell wrote: > > Handful of bug fixes to sneak in before rc3. > > thanks > -- PMM > > The following changes since commit c985266ea5b50e46e07b3568c1346e10064205c9: > > Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20190726' into > staging (2019-07-26 13:52:06 +0100) > > are available in the Git repository at: > > https://git.linaro.org/people/pmaydell/qemu-arm.git > tags/pull-target-arm-20190726 > > for you to fetch changes up to 67505c114e6acc26f3a1a2b74833c61b6a34ff95: > > hw/arm/boot: Further improve initrd positioning code (2019-07-26 16:17:56 > +0100) > > > target-arm queue: > * Fix broken migration on pl330 device > * Fix broken migration on stellaris-input device > * Add type checks to vmstate varry macros to avoid this class of bugs > * hw/arm/boot: Fix some remaining cases where we would put the >initrd on top of the kernel image > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/4.1 for any user-visible changes. -- PMM
Re: [Qemu-devel] [PATCH 04/28] memory: Fix type of IOMMUMemoryRegionClass member @parent_class
On 7/26/19 2:05 PM, Markus Armbruster wrote:
> TYPE_IOMMU_MEMORY_REGION is a direct subtype of TYPE_MEMORY_REGION.
> Its instance struct is IOMMUMemoryRegion, and its first member is a
> MemoryRegion. Correct. Its class struct is IOMMUMemoryRegionClass,
> and its first member is a DeviceClass. Wrong. Messed up when commit
> 1221a474676 introduced the QOM type. It even included hw/qdev-core.h
> just for that.
>
> TYPE_MEMORY_REGION doesn't bother to define a class struct. This is
> fine, it simply defaults to its super-type TYPE_OBJECT's class struct
> ObjectClass. Changing IOMMUMemoryRegionClass's first member's type to
> ObjectClass would be a minimal fix, if a bit brittle: if
> TYPE_MEMORY_REGION ever acquired own class struct, we'd have to update
> IOMMUMemoryRegionClass to use it.
>
> Fix it the clean and robust way instead: give TYPE_MEMORY_REGION its
> own class struct MemoryRegionClass now, and use it for
> IOMMUMemoryRegionClass's first member.
>
> Revert the include of hw/qdev-core.h, and fix the few files that have
> come to rely on it.
>
> Cc: Paolo Bonzini
> Signed-off-by: Markus Armbruster
> ---
> hw/display/vga-isa-mm.c | 1 +
> hw/net/pcnet.h | 1 +
> include/exec/memory.h | 9 +++--
> memory.c| 1 +
> 4 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/vga-isa-mm.c b/hw/display/vga-isa-mm.c
> index 215e649719..a790f69b6d 100644
> --- a/hw/display/vga-isa-mm.c
> +++ b/hw/display/vga-isa-mm.c
> @@ -22,6 +22,7 @@
> * THE SOFTWARE.
> */
> #include "qemu/osdep.h"
> +#include "qemu/bitops.h"
> #include "qemu/units.h"
> #include "hw/hw.h"
> #include "hw/display/vga.h"
> diff --git a/hw/net/pcnet.h b/hw/net/pcnet.h
> index 40831a7845..28d19a5c6f 100644
> --- a/hw/net/pcnet.h
> +++ b/hw/net/pcnet.h
> @@ -8,6 +8,7 @@
> #define PCNET_LOOPTEST_NOCRC 2
>
> #include "exec/memory.h"
> +#include "hw/irq.h"
>
> /* BUS CONFIGURATION REGISTERS */
> #define BCR_MSRDA0
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index bb0961ddb9..238370a2ff 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -25,7 +25,6 @@
> #include "qemu/notify.h"
> #include "qom/object.h"
> #include "qemu/rcu.h"
> -#include "hw/qdev-core.h"
>
> #define RAM_ADDR_INVALID (~(ram_addr_t)0)
>
> @@ -205,6 +204,12 @@ struct MemoryRegionOps {
> } impl;
> };
>
> +typedef struct MemoryRegionClass {
> +/* private */
> +ObjectClass parent_class;
> +} MemoryRegionClass;
> +
> +
> enum IOMMUMemoryRegionAttr {
> IOMMU_ATTR_SPAPR_TCE_FD
> };
> @@ -237,7 +242,7 @@ enum IOMMUMemoryRegionAttr {
> */
> typedef struct IOMMUMemoryRegionClass {
> /* private */
> -struct DeviceClass parent_class;
> +MemoryRegionClass parent_class;
>
> /*
> * Return a TLB entry that contains a given address.
> diff --git a/memory.c b/memory.c
> index d4579bbaec..bf108b596e 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -3223,6 +3223,7 @@ void memory_region_init_rom_device(MemoryRegion *mr,
> static const TypeInfo memory_region_info = {
> .parent = TYPE_OBJECT,
> .name = TYPE_MEMORY_REGION,
> +.class_size = sizeof(MemoryRegionClass),
> .instance_size = sizeof(MemoryRegion),
> .instance_init = memory_region_initfn,
> .instance_finalize = memory_region_finalize,
>
Reviewed-by: Philippe Mathieu-Daudé
Re: [Qemu-devel] [PATCH 06/28] trace: Eliminate use of TARGET_FMT_plx
On 7/26/19 2:05 PM, Markus Armbruster wrote:
> hw/tpm/trace-events uses TARGET_FMT_plx formats with uint64_t
> arguments. That's wrong, TARGET_FMT_plx takes hwaddr. Since hwaddr
> happens to be uint64_t, it works anyway. Messed up in commit
> ec427498da5, v2.12.0. Clean up by replacing TARGET_FMT_plx with its
> macro expansion.
>
> scripts/tracetool/format/log_stap.py (commit 62dd1048c0b, v4.0.0) has
> a special case for TARGET_FMT_plx. Delete it.
>
> Cc: Stefan Hajnoczi
> Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
> ---
> hw/tpm/trace-events | 4 ++--
> scripts/tracetool/format/log_stap.py | 3 ---
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> index 0b94aa1526..25e47a595a 100644
> --- a/hw/tpm/trace-events
> +++ b/hw/tpm/trace-events
> @@ -1,8 +1,8 @@
> # See docs/devel/tracing.txt for syntax documentation.
>
> # tpm_crb.c
> -tpm_crb_mmio_read(uint64_t addr, unsigned size, uint32_t val) "CRB read 0x"
> TARGET_FMT_plx " len:%u val: 0x%" PRIx32
> -tpm_crb_mmio_write(uint64_t addr, unsigned size, uint32_t val) "CRB write
> 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
> +tpm_crb_mmio_read(uint64_t addr, unsigned size, uint32_t val) "CRB read
> 0x016%" PRIx64 " len:%u val: 0x%" PRIx32
> +tpm_crb_mmio_write(uint64_t addr, unsigned size, uint32_t val) "CRB write
> 0x%016" PRIx64 " len:%u val: 0x%" PRIx32
>
> # tpm_passthrough.c
> tpm_passthrough_handle_request(void *cmd) "processing command %p"
> diff --git a/scripts/tracetool/format/log_stap.py
> b/scripts/tracetool/format/log_stap.py
> index 3ccbc09d61..9ab0cf2cce 100644
> --- a/scripts/tracetool/format/log_stap.py
> +++ b/scripts/tracetool/format/log_stap.py
> @@ -30,9 +30,6 @@ def c_macro_to_format(macro):
> if macro.startswith("PRI"):
> return macro[3]
>
> -if macro == "TARGET_FMT_plx":
> -return "%016x"
> -
> raise Exception("Unhandled macro '%s'" % macro)
>
> def c_fmt_to_stap(fmt):
>
Re: [Qemu-devel] [PATCH 08/28] Include sysemu/reset.h a lot less
On 7/26/19 2:05 PM, Markus Armbruster wrote: > In my "build everything" tree, changing sysemu/reset.h triggers a > recompile of some 2600 out of 6600 objects (not counting tests and > objects that don't depend on qemu/osdep.h). > > The main culprit is hw/hw.h, which supposedly includes it for > convenience. > > Include sysemu/reset.h only where it's needed. Touching it now > recompiles less than 200 objects. > > Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé > --- > hw/acpi/ich9.c | 2 ++ > hw/acpi/piix4.c| 2 ++ > hw/acpi/vmgenid.c | 1 + > hw/arm/armv7m.c| 1 + > hw/arm/boot.c | 1 + > hw/arm/nseries.c | 1 + > hw/arm/omap1.c | 1 + > hw/arm/omap2.c | 1 + > hw/arm/virt-acpi-build.c | 1 + > hw/char/parallel.c | 1 + > hw/char/serial.c | 1 + > hw/core/generic-loader.c | 1 + > hw/core/loader.c | 1 + > hw/cris/boot.c | 1 + > hw/display/cirrus_vga.c| 1 + > hw/display/ramfb.c | 2 ++ > hw/display/vga.c | 2 ++ > hw/hppa/machine.c | 1 + > hw/i386/acpi-build.c | 1 + > hw/i386/pc.c | 1 + > hw/ide/cmd646.c| 1 + > hw/ide/piix.c | 1 + > hw/ide/sii3112.c | 1 + > hw/ide/via.c | 1 + > hw/input/lm832x.c | 1 + > hw/input/pckbd.c | 2 ++ > hw/input/ps2.c | 2 ++ > hw/input/tsc2005.c | 1 + > hw/input/tsc210x.c | 1 + > hw/intc/mips_gic.c | 1 + > hw/intc/pnv_xive.c | 1 + > hw/intc/spapr_xive.c | 1 + > hw/intc/xics.c | 1 + > hw/intc/xive.c | 1 + > hw/isa/piix4.c | 1 + > hw/isa/vt82c686.c | 1 + > hw/lm32/lm32_boards.c | 1 + > hw/lm32/milkymist.c| 1 + > hw/microblaze/boot.c | 1 + > hw/mips/cps.c | 1 + > hw/mips/mips_fulong2e.c| 1 + > hw/mips/mips_jazz.c| 1 + > hw/mips/mips_malta.c | 1 + > hw/mips/mips_mipssim.c | 2 ++ > hw/mips/mips_r4k.c | 2 ++ > hw/misc/vmcoreinfo.c | 1 + > hw/moxie/moxiesim.c| 2 ++ > hw/net/eepro100.c | 1 + > hw/nios2/boot.c| 1 + > hw/nvram/fw_cfg.c | 1 + > hw/openrisc/openrisc_sim.c | 1 + > hw/pci-host/bonito.c | 1 + > hw/pci-host/piix.c | 1 + > hw/ppc/e500.c | 1 + > hw/ppc/mac_newworld.c | 1 + > hw/ppc/mac_oldworld.c | 1 + > hw/ppc/pnv.c | 1 + > hw/ppc/pnv_core.c | 1 + > hw/ppc/pnv_psi.c | 1 + > hw/ppc/ppc405_boards.c | 2 ++ > hw/ppc/ppc405_uc.c | 2 ++ > hw/ppc/ppc440_bamboo.c | 1 + > hw/ppc/ppc440_uc.c | 1 + > hw/ppc/ppc4xx_devs.c | 2 ++ > hw/ppc/ppc4xx_pci.c| 1 + > hw/ppc/ppc_booke.c | 2 ++ > hw/ppc/prep.c | 2 ++ > hw/ppc/sam460ex.c | 1 + > hw/ppc/spapr.c | 1 + > hw/ppc/spapr_cpu_core.c| 2 ++ > hw/ppc/spapr_drc.c | 1 + > hw/ppc/virtex_ml507.c | 1 + > hw/riscv/riscv_hart.c | 1 + > hw/s390x/ipl.c | 1 + > hw/s390x/s390-virtio-ccw.c | 1 + > hw/sh4/r2d.c | 1 + > hw/sparc/leon3.c | 2 ++ > hw/sparc/sun4m.c | 2 ++ > hw/sparc64/sparc64.c | 1 + > hw/timer/etraxfs_timer.c | 1 + > hw/timer/mc146818rtc.c | 1 + > hw/tpm/tpm_ppi.c | 1 - > hw/vfio/common.c | 1 + > hw/watchdog/wdt_diag288.c | 1 + > hw/xtensa/sim.c| 1 + > hw/xtensa/xtfpga.c | 1 + > include/hw/hw.h| 1 - > target/i386/cpu.c | 1 + > target/i386/hax-all.c | 1 + > target/i386/kvm.c | 1 + > target/s390x/cpu.c | 1 + > vl.c | 1 + > 92 files changed, 107 insertions(+), 2 deletions(-) > > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c > index e53dfe1ee3..b4d987c811 100644 > --- a/hw/acpi/ich9.c > +++ b/hw/acpi/ich9.c > @@ -23,6 +23,7 @@ > * Contributions after 2012-01-13 are licensed under the terms of the > * GNU GPL, version 2 or (at your option) any later version. > */ > + > #include "qemu/osdep.h" > #include "hw/hw.h" > #include "qapi/error.h" > @@ -30,6 +31,7 @@ > #include "hw/i386/pc.h" > #include "hw/pci/pci.h" > #include "qemu/timer.h" > +#include "sysemu/reset.h" > #include "sysemu/sysemu.h" > #include "hw/acpi/acpi.h" > #include "hw/acpi/tco.h" > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > index ec4e186cec..a59e58d937 100644 > --- a/hw/acpi/piix4.c > +++ b/hw/acpi/piix4.c > @@ -18,6 +18,7 @@ > * Contributions after 2012-01-13 are licensed under the terms of the > * GNU GPL, version 2 or (at your option) any later version. > */ > + > #include "qemu/osdep.h" > #include "hw/hw.h" > #include "hw/i386/pc.h" > @@ -25,6 +26,7 @@ > #include "hw/i2c/pm_smbus.h" > #include "hw/pci/pci.h" > #include "hw/acpi/acpi.h" > +#include "sysemu/reset.h" >
Re: [Qemu-devel] [PATCH 25/28] numa: Move remaining NUMA declarations from sysemu.h to numa.h
On 7/26/19 2:05 PM, Markus Armbruster wrote:
> Commit e35704ba9c "numa: Move NUMA declarations from sysemu.h to
> numa.h" left a few NUMA-related macros behind. Move them now.
>
> Cc: Eduardo Habkost
> Cc: Marcel Apfelbaum
> Signed-off-by: Markus Armbruster
:)
Reviewed-by: Philippe Mathieu-Daudé
> ---
> exec.c | 2 +-
> hw/core/numa.c | 1 +
> hw/mem/pc-dimm.c | 1 +
> hw/pci/pci.c | 2 +-
> hw/ppc/spapr.c | 1 +
> include/sysemu/hostmem.h | 2 +-
> include/sysemu/numa.h| 9 +++--
> include/sysemu/sysemu.h | 7 ---
> 8 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 4d9e146c79..f0ac29aa26 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -45,7 +45,7 @@
> #include "exec/memory.h"
> #include "exec/ioport.h"
> #include "sysemu/dma.h"
> -#include "sysemu/numa.h"
> +#include "sysemu/hostmem.h"
> #include "sysemu/hw_accel.h"
> #include "exec/address-spaces.h"
> #include "sysemu/xen-mapcache.h"
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index d817f06ead..450c522dd8 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -23,6 +23,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "sysemu/hostmem.h"
> #include "sysemu/numa.h"
> #include "exec/cpu-common.h"
> #include "exec/ramlist.h"
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index fa90d4fc6c..938706d5a7 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -28,6 +28,7 @@
> #include "qapi/error.h"
> #include "qapi/visitor.h"
> #include "qemu/module.h"
> +#include "sysemu/hostmem.h"
> #include "sysemu/numa.h"
> #include "trace.h"
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 9001b81daa..4b6ffab13d 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -34,7 +34,7 @@
> #include "migration/vmstate.h"
> #include "monitor/monitor.h"
> #include "net/net.h"
> -#include "sysemu/sysemu.h"
> +#include "sysemu/numa.h"
> #include "hw/loader.h"
> #include "qemu/error-report.h"
> #include "qemu/range.h"
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 06d23a5004..4044e61a0c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -29,6 +29,7 @@
> #include "qapi/error.h"
> #include "qapi/visitor.h"
> #include "sysemu/sysemu.h"
> +#include "sysemu/hostmem.h"
> #include "sysemu/numa.h"
> #include "sysemu/qtest.h"
> #include "sysemu/reset.h"
> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> index afeb5db1b1..4dbdadd39e 100644
> --- a/include/sysemu/hostmem.h
> +++ b/include/sysemu/hostmem.h
> @@ -13,7 +13,7 @@
> #ifndef SYSEMU_HOSTMEM_H
> #define SYSEMU_HOSTMEM_H
>
> -#include "sysemu/sysemu.h" /* for MAX_NODES */
> +#include "sysemu/numa.h"
> #include "qapi/qapi-types-machine.h"
> #include "qom/object.h"
> #include "exec/memory.h"
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 4c4c1dee9b..7a4ce89765 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -2,13 +2,18 @@
> #define SYSEMU_NUMA_H
>
> #include "qemu/bitmap.h"
> -#include "sysemu/sysemu.h"
> -#include "sysemu/hostmem.h"
> #include "qapi/qapi-types-machine.h"
> #include "exec/cpu-common.h"
>
> struct CPUArchId;
>
> +#define MAX_NODES 128
> +#define NUMA_NODE_UNASSIGNED MAX_NODES
> +#define NUMA_DISTANCE_MIN 10
> +#define NUMA_DISTANCE_DEFAULT 20
> +#define NUMA_DISTANCE_MAX 254
> +#define NUMA_DISTANCE_UNREACHABLE 255
> +
> extern int nb_numa_nodes; /* Number of NUMA nodes */
> extern bool have_numa_distance;
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index ac18a1184a..227202999d 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -117,13 +117,6 @@ extern QEMUClockType rtc_clock;
> extern const char *mem_path;
> extern int mem_prealloc;
>
> -#define MAX_NODES 128
> -#define NUMA_NODE_UNASSIGNED MAX_NODES
> -#define NUMA_DISTANCE_MIN 10
> -#define NUMA_DISTANCE_DEFAULT 20
> -#define NUMA_DISTANCE_MAX 254
> -#define NUMA_DISTANCE_UNREACHABLE 255
> -
> #define MAX_OPTION_ROMS 16
> typedef struct QEMUOptionRom {
> const char *name;
>
Re: [Qemu-devel] [PATCH 24/28] Include sysemu/hostmem.h less
On 7/26/19 2:05 PM, Markus Armbruster wrote: > Move the HostMemoryBackend typedef from sysemu/hostmem.h to > qemu/typedefs.h. This renders a few inclusions of sysemu/hostmem.h > superflouous; drop them. > > Cc: Eduardo Habkost > Cc: Igor Mammedov > Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé > --- > hw/mem/nvdimm.c | 1 + > hw/virtio/virtio-pmem.c | 1 + > include/hw/mem/pc-dimm.h| 1 - > include/hw/virtio/virtio-pmem.h | 1 - > include/qemu/typedefs.h | 1 + > include/sysemu/hostmem.h| 1 - > 6 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c > index 6fefd65092..375f9a588a 100644 > --- a/hw/mem/nvdimm.c > +++ b/hw/mem/nvdimm.c > @@ -30,6 +30,7 @@ > #include "hw/mem/nvdimm.h" > #include "hw/qdev-properties.h" > #include "hw/mem/memory-device.h" > +#include "sysemu/hostmem.h" > > static void nvdimm_get_label_size(Object *obj, Visitor *v, const char *name, >void *opaque, Error **errp) > diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c > index ff1a2ddb36..c0c9395e55 100644 > --- a/hw/virtio/virtio-pmem.c > +++ b/hw/virtio/virtio-pmem.c > @@ -21,6 +21,7 @@ > #include "hw/virtio/virtio-access.h" > #include "standard-headers/linux/virtio_ids.h" > #include "standard-headers/linux/virtio_pmem.h" > +#include "sysemu/hostmem.h" > #include "block/aio.h" > #include "block/thread-pool.h" > > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > index 47b246f95c..289edc0f3d 100644 > --- a/include/hw/mem/pc-dimm.h > +++ b/include/hw/mem/pc-dimm.h > @@ -17,7 +17,6 @@ > #define QEMU_PC_DIMM_H > > #include "exec/memory.h" > -#include "sysemu/hostmem.h" > #include "hw/qdev-core.h" > > #define TYPE_PC_DIMM "pc-dimm" > diff --git a/include/hw/virtio/virtio-pmem.h b/include/hw/virtio/virtio-pmem.h > index 8bf2ae780f..33f1999320 100644 > --- a/include/hw/virtio/virtio-pmem.h > +++ b/include/hw/virtio/virtio-pmem.h > @@ -16,7 +16,6 @@ > > #include "hw/virtio/virtio.h" > #include "qapi/qapi-types-misc.h" > -#include "sysemu/hostmem.h" > > #define TYPE_VIRTIO_PMEM "virtio-pmem" > > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > index 9e1283aacf..f569f5f270 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h > @@ -33,6 +33,7 @@ typedef struct FWCfgEntry FWCfgEntry; > typedef struct FWCfgIoState FWCfgIoState; > typedef struct FWCfgMemState FWCfgMemState; > typedef struct FWCfgState FWCfgState; > +typedef struct HostMemoryBackend HostMemoryBackend; > typedef struct HVFX86EmulatorState HVFX86EmulatorState; > typedef struct I2CBus I2CBus; > typedef struct I2SCodec I2SCodec; > diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h > index 92fa0e458c..afeb5db1b1 100644 > --- a/include/sysemu/hostmem.h > +++ b/include/sysemu/hostmem.h > @@ -27,7 +27,6 @@ > #define MEMORY_BACKEND_CLASS(klass) \ > OBJECT_CLASS_CHECK(HostMemoryBackendClass, (klass), TYPE_MEMORY_BACKEND) > > -typedef struct HostMemoryBackend HostMemoryBackend; > typedef struct HostMemoryBackendClass HostMemoryBackendClass; > > /** >
Re: [Qemu-devel] [PATCH 17/28] Include qom/object.h slightly less
On 7/26/19 2:05 PM, Markus Armbruster wrote:
> hw/hw.h used to include headers hardware emulation "usually" needs.
> The previous commits removed all but one of them, to good effect.
> Only qom/object.h is left. Remove that one, too.
>
> Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
> ---
> include/hw/display/edid.h | 2 +-
> include/hw/hw.h | 2 --
> 2 files changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/include/hw/display/edid.h b/include/hw/display/edid.h
> index 7948bd2c40..ff99dc0a05 100644
> --- a/include/hw/display/edid.h
> +++ b/include/hw/display/edid.h
> @@ -1,7 +1,7 @@
> #ifndef EDID_H
> #define EDID_H
>
> -#include "hw/hw.h"
> +#include "qom/object.h"
>
> typedef struct qemu_edid_info {
> const char *vendor; /* http://www.uefi.org/pnp_id_list */
> diff --git a/include/hw/hw.h b/include/hw/hw.h
> index 8e18358e6a..8079b911fa 100644
> --- a/include/hw/hw.h
> +++ b/include/hw/hw.h
> @@ -6,8 +6,6 @@
> #error Cannot include hw/hw.h from user emulation
> #endif
>
> -#include "qom/object.h"
> -
> void QEMU_NORETURN hw_error(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>
> #endif
>
Re: [Qemu-devel] [PATCH 14/28] migration: Move the VMStateDescription typedef to typedefs.h
On 7/26/19 2:05 PM, Markus Armbruster wrote:
> We declare incomplete struct VMStateDescription in a couple of places
> so we don't have to include migration/vmstate.h for the typedef.
> That's fine with me. However, the next commit will drop
> migration/vmstate.h from a massive number of compiles. Move the
> typedef to qemu/typedefs.h now, so I don't have to insert struct in
> front of VMStateDescription all over the place then.
>
> Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
> ---
> include/hw/qdev-core.h | 6 ++
> include/migration/vmstate.h | 1 -
> include/qemu/typedefs.h | 1 +
> include/qom/cpu.h | 4 ++--
> target/alpha/cpu.h | 2 +-
> target/arm/cpu.h| 2 +-
> target/cris/cpu.h | 2 +-
> target/hppa/cpu.h | 2 +-
> target/i386/cpu.h | 2 +-
> target/lm32/cpu.h | 2 +-
> target/mips/internal.h | 2 +-
> target/openrisc/cpu.h | 2 +-
> target/ppc/cpu-qom.h| 2 +-
> target/ppc/cpu.h| 2 +-
> target/s390x/cpu.h | 2 +-
> target/sparc/cpu.h | 2 +-
> 16 files changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index b0c1d0e410..80fdb38d94 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -35,8 +35,6 @@ typedef void (*DeviceReset)(DeviceState *dev);
> typedef void (*BusRealize)(BusState *bus, Error **errp);
> typedef void (*BusUnrealize)(BusState *bus, Error **errp);
>
> -struct VMStateDescription;
> -
> /**
> * DeviceClass:
> * @props: Properties accessing state fields.
> @@ -112,7 +110,7 @@ typedef struct DeviceClass {
> DeviceUnrealize unrealize;
>
> /* device state */
> -const struct VMStateDescription *vmsd;
> +const VMStateDescription *vmsd;
>
> /* Private to qdev / bus. */
> const char *bus_type;
> @@ -422,7 +420,7 @@ void device_class_set_parent_unrealize(DeviceClass *dc,
> DeviceUnrealize dev_unrealize,
> DeviceUnrealize *parent_unrealize);
>
> -const struct VMStateDescription *qdev_get_vmsd(DeviceState *dev);
> +const VMStateDescription *qdev_get_vmsd(DeviceState *dev);
>
> const char *qdev_fw_name(DeviceState *dev);
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index ca68584eba..a547517dc7 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -28,7 +28,6 @@
> #define QEMU_VMSTATE_H
>
> typedef struct VMStateInfo VMStateInfo;
> -typedef struct VMStateDescription VMStateDescription;
> typedef struct VMStateField VMStateField;
>
> /* VMStateInfo allows customized migration of objects that don't fit in
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index c32efb5b18..9e1283aacf 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -101,6 +101,7 @@ typedef struct SHPCDevice SHPCDevice;
> typedef struct SSIBus SSIBus;
> typedef struct VirtIODevice VirtIODevice;
> typedef struct Visitor Visitor;
> +typedef struct VMStateDescription VMStateDescription;
>
> /*
> * Pointer types
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 5ee0046b62..ddb91bbaff 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -215,7 +215,7 @@ typedef struct CPUClass {
> int (*write_elf32_qemunote)(WriteCoreDumpFunction f, CPUState *cpu,
> void *opaque);
>
> -const struct VMStateDescription *vmsd;
> +const VMStateDescription *vmsd;
> const char *gdb_core_xml_file;
> gchar * (*gdb_arch_name)(CPUState *cpu);
> const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname);
> @@ -1108,7 +1108,7 @@ bool target_words_bigendian(void);
> #ifdef NEED_CPU_H
>
> #ifdef CONFIG_SOFTMMU
> -extern const struct VMStateDescription vmstate_cpu_common;
> +extern const VMStateDescription vmstate_cpu_common;
> #else
> #define vmstate_cpu_common vmstate_dummy
> #endif
> diff --git a/target/alpha/cpu.h b/target/alpha/cpu.h
> index b3e8a823e1..4619530660 100644
> --- a/target/alpha/cpu.h
> +++ b/target/alpha/cpu.h
> @@ -277,7 +277,7 @@ struct AlphaCPU {
>
>
> #ifndef CONFIG_USER_ONLY
> -extern const struct VMStateDescription vmstate_alpha_cpu;
> +extern const VMStateDescription vmstate_alpha_cpu;
> #endif
>
> void alpha_cpu_do_interrupt(CPUState *cpu);
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 94c990cddb..2cdde6c4bc 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -922,7 +922,7 @@ void arm_cpu_post_init(Object *obj);
> uint64_t arm_cpu_mp_affinity(int idx, uint8_t clustersz);
>
> #ifndef CONFIG_USER_ONLY
> -extern const struct VMStateDescription vmstate_arm_cpu;
> +extern const VMStateDescription vmstate_arm_cpu;
> #endif
>
> void arm_cpu_do_interrupt(CPUState *cpu);
> diff --git a/target/cris/cpu.h
Re: [Qemu-devel] [PATCH 0/2] ssh: add password and privkey auth methods
On Fri, Jul 26, 2019 at 04:35:27PM +0100, Richard W.M. Jones wrote:
> On Fri, Jul 26, 2019 at 10:06:43AM -0500, Eric Blake wrote:
> > On 7/26/19 9:45 AM, Pino Toscano wrote:
> > > On Friday, 26 July 2019 16:27:11 CEST Richard W.M. Jones wrote:
> > >> On Fri, Jul 26, 2019 at 04:09:52PM +0200, Pino Toscano wrote:
> > >>> These two patches add the password and private key authentication
> > >>> methods to the ssh block driver, using secure objects for
> > >>> passwords/passphrases.
> > >>
> > >> I was attempting to test this but couldn't work out the full command
> > >> line to use it (with qemu-img). I got as far as:
> > >>
> > >> $ ./qemu-img convert -p 'json:{ "file.driver": "ssh", "file.host":
> > >> "devr7", "file.path": "/var/tmp/root", "file.password-secret": "..." }'
> > >> /var/tmp/root
> > >>
> > >> I guess the secret should be specified using --object, but at that
> > >> point I gave up.
> > >
> > > Almost there :) add e.g.
> > > --object 'secret,id=sec0,file=passwd'
> > > as parameter for the convert command (so after it, not before), and then
> > > set 'sec0' as value for file.password-secret. Of course 'sec0' is
> > > arbitrary, any other QEMU id will do.
> > >
> > > A long helpful comment in include/crypto/secret.h explains the basics
> > > of the crypto objects.
> >
> > That is useful information, but even more useful if you amend the commit
> > message to include a working example command line rather than making
> > readers chase down the docs :)
> >
> > Untested, but piecing together what I know from my work on qemu-nbd
> > encryption, it seems like this should be a starting point for such a
> > command:
> >
> > qemu-img convert -p --imageopts --object secret,id=sec0,file=passwd \
> > driver=ssh,host=devr7,path=/var/tmp/root,password-secret=sec0 \
> > /var/tmp/copy
>
> --imageopts isn't necessary. This was the command that worked for me:
>
> unset SSH_AUTH_SOCK; ./qemu-img convert -p --object
> 'secret,id=sec0,file=/tmp/passwd' 'json:{ "file.driver": "ssh", "file.host":
> "devr7", "file.path": "/var/tmp/root", "file.password-secret": "sec0" }'
> /var/tmp/root
Right you didn't need --imageopts because you used the json filename
syntax.
--imageopts is for telling it to intepret the filename as key,value pairs
as in Eric's example.
json & imageopts syntaxes are equally expressive, so pick which you
prefer.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 0/2] ssh: add password and privkey auth methods
On Fri, Jul 26, 2019 at 10:06:43AM -0500, Eric Blake wrote:
> On 7/26/19 9:45 AM, Pino Toscano wrote:
> > On Friday, 26 July 2019 16:27:11 CEST Richard W.M. Jones wrote:
> >> On Fri, Jul 26, 2019 at 04:09:52PM +0200, Pino Toscano wrote:
> >>> These two patches add the password and private key authentication
> >>> methods to the ssh block driver, using secure objects for
> >>> passwords/passphrases.
> >>
> >> I was attempting to test this but couldn't work out the full command
> >> line to use it (with qemu-img). I got as far as:
> >>
> >> $ ./qemu-img convert -p 'json:{ "file.driver": "ssh", "file.host":
> >> "devr7", "file.path": "/var/tmp/root", "file.password-secret": "..." }'
> >> /var/tmp/root
> >>
> >> I guess the secret should be specified using --object, but at that
> >> point I gave up.
> >
> > Almost there :) add e.g.
> > --object 'secret,id=sec0,file=passwd'
> > as parameter for the convert command (so after it, not before), and then
> > set 'sec0' as value for file.password-secret. Of course 'sec0' is
> > arbitrary, any other QEMU id will do.
> >
> > A long helpful comment in include/crypto/secret.h explains the basics
> > of the crypto objects.
>
> That is useful information, but even more useful if you amend the commit
> message to include a working example command line rather than making
> readers chase down the docs :)
>
> Untested, but piecing together what I know from my work on qemu-nbd
> encryption, it seems like this should be a starting point for such a
> command:
>
> qemu-img convert -p --imageopts --object secret,id=sec0,file=passwd \
> driver=ssh,host=devr7,path=/var/tmp/root,password-secret=sec0 \
> /var/tmp/copy
--imageopts isn't necessary. This was the command that worked for me:
unset SSH_AUTH_SOCK; ./qemu-img convert -p --object
'secret,id=sec0,file=/tmp/passwd' 'json:{ "file.driver": "ssh", "file.host":
"devr7", "file.path": "/var/tmp/root", "file.password-secret": "sec0" }'
/var/tmp/root
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
Re: [Qemu-devel] [PATCH v4 4/7] accel: replacing part of CONFIG_PROFILER with TBStats
vandersonmr writes:
> We add some of the statistics collected in the TCGProfiler
> into the TBStats, having the statistics not only for the whole
> emulation but for each TB. Then, we removed these stats
> from TCGProfiler and reconstruct the information for the
> "info jit" using the sum of all TBStats statistics.
>
> The goal is to have one unique and better way of collecting
> emulation statistics. Moreover, checking dynamiclly if the
> profiling is enabled showed to have an insignificant impact
> on the performance:
> https://wiki.qemu.org/Internships/ProjectIdeas/TCGCodeQuality#Overheads.
>
> Signed-off-by: Vanderson M. do Rosario
> ---
> accel/tcg/Makefile.objs | 2 +-
> accel/tcg/tb-stats.c | 107 ++
> accel/tcg/translate-all.c | 10 ++--
> include/exec/tb-stats.h | 9
> tcg/tcg.c | 93 -
> tcg/tcg.h | 10
> 6 files changed, 131 insertions(+), 100 deletions(-)
> create mode 100644 accel/tcg/tb-stats.c
>
> diff --git a/accel/tcg/Makefile.objs b/accel/tcg/Makefile.objs
> index d381a02f34..49ffe81b5d 100644
> --- a/accel/tcg/Makefile.objs
> +++ b/accel/tcg/Makefile.objs
> @@ -2,7 +2,7 @@ obj-$(CONFIG_SOFTMMU) += tcg-all.o
> obj-$(CONFIG_SOFTMMU) += cputlb.o
> obj-y += tcg-runtime.o tcg-runtime-gvec.o
> obj-y += cpu-exec.o cpu-exec-common.o translate-all.o
> -obj-y += translator.o
> +obj-y += translator.o tb-stats.o
>
> obj-$(CONFIG_USER_ONLY) += user-exec.o
> obj-$(call lnot,$(CONFIG_SOFTMMU)) += user-exec-stub.o
> diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
> new file mode 100644
> index 00..44497d4f9b
> --- /dev/null
> +++ b/accel/tcg/tb-stats.c
> @@ -0,0 +1,107 @@
> +#include "qemu/osdep.h"
> +
> +#include "disas/disas.h"
> +#include "exec/exec-all.h"
> +#include "tcg.h"
> +
> +#include "qemu/qemu-print.h"
> +
> +struct jit_profile_info {
> +uint64_t translations;
> +uint64_t aborted;
> +uint64_t ops;
> +unsigned ops_max;
> +uint64_t del_ops;
> +uint64_t temps;
> +unsigned temps_max;
> +uint64_t host;
> +uint64_t guest;
> +uint64_t host_ins;
> +uint64_t search_data;
> +};
> +
> +/* accumulate the statistics from all TBs */
> +static void collect_jit_profile_info(void *p, uint32_t hash, void *userp)
> +{
> +struct jit_profile_info *jpi = userp;
> +TBStatistics *tbs = p;
> +
> +jpi->translations += tbs->translations.total;
> +jpi->ops += tbs->code.num_tcg_ops;
> +if (tbs->translations.total && tbs->code.num_tcg_ops /
> tbs->translations.total
> +> jpi->ops_max) {
> +jpi->ops_max = tbs->code.num_tcg_ops / tbs->translations.total;
> +}
> +jpi->del_ops += tbs->code.deleted_ops;
> +jpi->temps += tbs->code.temps;
> +if (tbs->translations.total && tbs->code.temps / tbs->translations.total
> >
> +jpi->temps_max) {
> +jpi->temps_max = tbs->code.temps / tbs->translations.total;
> +}
> +jpi->host += tbs->code.out_len;
> +jpi->guest += tbs->code.in_len;
> +jpi->host_ins += tbs->code.num_host_inst;
> +jpi->search_data += tbs->code.search_out_len;
> +}
> +
> +/* dump JIT statisticis using TCGProfile and TBStats */
> +void dump_jit_profile_info(TCGProfile *s)
> +{
> +if (!tb_stats_collection_enabled()) {
> +return;
> +}
> +
> +struct jit_profile_info *jpi = g_new0(struct jit_profile_info,
> 1);
This leaks and needs to be freed after we are done.
> +
> +qht_iter(_ctx.tb_stats, collect_jit_profile_info, jpi);
> +
> +if (jpi->translations) {
> +qemu_printf("translated TBs %" PRId64 "\n", jpi->translations);
> +qemu_printf("avg ops/TB %0.1f max=%d\n",
> +jpi->ops / (double) jpi->translations, jpi->ops_max);
> +qemu_printf("deleted ops/TB %0.2f\n",
> +jpi->del_ops / (double) jpi->translations);
> +qemu_printf("avg temps/TB%0.2f max=%d\n",
> +jpi->temps / (double) jpi->translations, jpi->temps_max);
> +qemu_printf("avg host code/TB%0.1f\n",
> +jpi->host / (double) jpi->translations);
> +qemu_printf("avg host ins/TB %0.1f\n",
> +jpi->host_ins / (double) jpi->translations);
> +qemu_printf("avg search data/TB %0.1f\n",
> +jpi->search_data / (double) jpi->translations);
> +
> +if (s) {
> +int64_t tot = s->interm_time + s->code_time;
> +qemu_printf("JIT cycles %" PRId64 " (%0.3f s at 2.4
> GHz)\n",
> +tot, tot / 2.4e9);
> +qemu_printf("cycles/op %0.1f\n",
> +jpi->ops ? (double)tot / jpi->ops : 0);
> +qemu_printf("cycles/in byte %0.1f\n",
> +jpi->guest ? (double)tot / jpi->guest : 0);
> +qemu_printf("cycles/out byte %0.1f\n",
> +
Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions
Reviewed-by: Jonathan Behrens
On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis
wrote:
> Signed-off-by: Alistair Francis
> ---
> hw/riscv/sifive_plic.c | 12
> include/hw/riscv/sifive_plic.h | 3 ---
> 2 files changed, 15 deletions(-)
>
> diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
> index 0950e89e15..864a1bed42 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -161,18 +161,6 @@ static void sifive_plic_update(SiFivePLICState *plic)
> }
> }
>
> -void sifive_plic_raise_irq(SiFivePLICState *plic, uint32_t irq)
> -{
> -sifive_plic_set_pending(plic, irq, true);
> -sifive_plic_update(plic);
> -}
> -
> -void sifive_plic_lower_irq(SiFivePLICState *plic, uint32_t irq)
> -{
> -sifive_plic_set_pending(plic, irq, false);
> -sifive_plic_update(plic);
> -}
> -
> static uint32_t sifive_plic_claim(SiFivePLICState *plic, uint32_t addrid)
> {
> int i, j;
> diff --git a/include/hw/riscv/sifive_plic.h
> b/include/hw/riscv/sifive_plic.h
> index ce8907f6aa..3b8a623919 100644
> --- a/include/hw/riscv/sifive_plic.h
> +++ b/include/hw/riscv/sifive_plic.h
> @@ -69,9 +69,6 @@ typedef struct SiFivePLICState {
> uint32_t aperture_size;
> } SiFivePLICState;
>
> -void sifive_plic_raise_irq(SiFivePLICState *plic, uint32_t irq);
> -void sifive_plic_lower_irq(SiFivePLICState *plic, uint32_t irq);
> -
> DeviceState *sifive_plic_create(hwaddr addr, char *hart_config,
> uint32_t num_sources, uint32_t num_priorities,
> uint32_t priority_base, uint32_t pending_base,
> --
> 2.22.0
>
>
>
[Qemu-devel] [PULL 4/5] hw/arm/boot: Rename elf_{low, high}_addr to image_{low, high}_addr
Rename the elf_low_addr and elf_high_addr variables to image_low_addr
and image_high_addr -- in the next commit we will extend them to
be set for other kinds of image file and not just ELF files.
Signed-off-by: Peter Maydell
Reviewed-by: Alex Bennée
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Mark Rutland
Message-id: 20190722151804.25467-2-peter.mayd...@linaro.org
---
hw/arm/boot.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 1fb24fbef27..b7b31753aca 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -986,7 +986,9 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
int kernel_size;
int initrd_size;
int is_linux = 0;
-uint64_t elf_entry, elf_low_addr, elf_high_addr;
+uint64_t elf_entry;
+/* Addresses of first byte used and first byte not used by the image */
+uint64_t image_low_addr, image_high_addr;
int elf_machine;
hwaddr entry;
static const ARMInsnFixup *primary_loader;
@@ -1014,24 +1016,24 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
info->nb_cpus = 1;
/* Assume that raw images are linux kernels, and ELF images are not. */
-kernel_size = arm_load_elf(info, _entry, _low_addr,
- _high_addr, elf_machine, as);
+kernel_size = arm_load_elf(info, _entry, _low_addr,
+ _high_addr, elf_machine, as);
if (kernel_size > 0 && have_dtb(info)) {
/*
* If there is still some room left at the base of RAM, try and put
* the DTB there like we do for images loaded with -bios or -pflash.
*/
-if (elf_low_addr > info->loader_start
-|| elf_high_addr < info->loader_start) {
+if (image_low_addr > info->loader_start
+|| image_high_addr < info->loader_start) {
/*
- * Set elf_low_addr as address limit for arm_load_dtb if it may be
+ * Set image_low_addr as address limit for arm_load_dtb if it may
be
* pointing into RAM, otherwise pass '0' (no limit)
*/
-if (elf_low_addr < info->loader_start) {
-elf_low_addr = 0;
+if (image_low_addr < info->loader_start) {
+image_low_addr = 0;
}
info->dtb_start = info->loader_start;
-info->dtb_limit = elf_low_addr;
+info->dtb_limit = image_low_addr;
}
}
entry = elf_entry;
--
2.20.1
[Qemu-devel] [PULL 5/5] hw/arm/boot: Further improve initrd positioning code
In commit e6b2b20d9735d4ef we made the boot loader code try to avoid
putting the initrd on top of the kernel. However the expression used
to calculate the start of the initrd:
info->initrd_start = info->loader_start +
MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);
incorrectly uses 'kernel_size' as the offset within RAM of the
highest address to avoid. This is incorrect because the kernel
doesn't start at address 0, but slightly higher than that. This
means that we can still incorrectly end up overlaying the initrd on
the kernel in some cases, for example:
* The kernel's image_size is 0x0a7a8000
* The kernel was loaded at 0x4008
* The end of the kernel is 0x4A828000
* The DTB was loaded at 0x4a80
To get this right we need to track the actual highest address used
by the kernel and use that rather than kernel_size. We already
set image_low_addr and image_high_addr for ELF images; set them
also for the various other image types we support, and then use
image_high_addr as the lowest allowed address for the initrd.
(We don't use image_low_addr, but we set it for consistency
with the existing code path for ELF files.)
Fixes: e6b2b20d9735d4ef
Reported-by: Mark Rutland
Signed-off-by: Peter Maydell
Reviewed-by: Alex Bennée
Tested-by: Mark Rutland
Message-id: 20190722151804.25467-3-peter.mayd...@linaro.org
---
hw/arm/boot.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index b7b31753aca..c2b89b3bb9b 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -988,7 +988,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
int is_linux = 0;
uint64_t elf_entry;
/* Addresses of first byte used and first byte not used by the image */
-uint64_t image_low_addr, image_high_addr;
+uint64_t image_low_addr = 0, image_high_addr = 0;
int elf_machine;
hwaddr entry;
static const ARMInsnFixup *primary_loader;
@@ -1041,17 +1041,29 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
uint64_t loadaddr = info->loader_start + KERNEL_NOLOAD_ADDR;
kernel_size = load_uimage_as(info->kernel_filename, , ,
_linux, NULL, NULL, as);
+if (kernel_size >= 0) {
+image_low_addr = loadaddr;
+image_high_addr = image_low_addr + kernel_size;
+}
}
if (arm_feature(>env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
kernel_size = load_aarch64_image(info->kernel_filename,
info->loader_start, , as);
is_linux = 1;
+if (kernel_size >= 0) {
+image_low_addr = entry;
+image_high_addr = image_low_addr + kernel_size;
+}
} else if (kernel_size < 0) {
/* 32-bit ARM */
entry = info->loader_start + KERNEL_LOAD_ADDR;
kernel_size = load_image_targphys_as(info->kernel_filename, entry,
ram_end - KERNEL_LOAD_ADDR, as);
is_linux = 1;
+if (kernel_size >= 0) {
+image_low_addr = entry;
+image_high_addr = image_low_addr + kernel_size;
+}
}
if (kernel_size < 0) {
error_report("could not load kernel '%s'", info->kernel_filename);
@@ -1083,7 +1095,10 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
* we might still make a bad choice here.
*/
info->initrd_start = info->loader_start +
-MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);
+MIN(info->ram_size / 2, 128 * 1024 * 1024);
+if (image_high_addr) {
+info->initrd_start = MAX(info->initrd_start, image_high_addr);
+}
info->initrd_start = TARGET_PAGE_ALIGN(info->initrd_start);
if (is_linux) {
--
2.20.1
[Qemu-devel] [PULL 3/5] vmstate.h: Type check VMSTATE_STRUCT_VARRAY macros
The VMSTATE_STRUCT_VARRAY_UINT32 macro is intended to handle
migrating a field which is an array of structs, but where instead of
migrating the entire array we only migrate a variable number of
elements of it.
The VMSTATE_STRUCT_VARRAY_POINTER_UINT32 macro is intended to handle
migrating a field which is of pointer type, and points to a
dynamically allocated array of structs of variable size.
We weren't actually checking that the field passed to
VMSTATE_STRUCT_VARRAY_UINT32 really is an array, with the result that
accidentally using it where the _POINTER_ macro was intended would
compile but silently corrupt memory on migration.
Add type-checking that enforces that the field passed in is
really of the right array type. This applies to all the VMSTATE
macros which use flags including VMS_VARRAY_* but not VMS_POINTER.
Signed-off-by: Peter Maydell
Reviewed-by: Dr. David Alan Gilbert
Reviewed-by: Damien Hedde
Tested-by: Damien Hedde
Message-id: 20190725163710.11703-3-peter.mayd...@linaro.org
---
include/migration/vmstate.h | 30 --
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index ca68584eba4..c2bfa7a7f0c 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -227,8 +227,22 @@ extern const VMStateInfo vmstate_info_bitmap;
extern const VMStateInfo vmstate_info_qtailq;
#define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
+/*
+ * Check that type t2 is an array of type t1 of size n,
+ * e.g. if t1 is 'foo' and n is 32 then t2 must be 'foo[32]'
+ */
#define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
#define type_check_pointer(t1,t2) ((t1**)0 - (t2*)0)
+/*
+ * type of element 0 of the specified (array) field of the type.
+ * Note that if the field is a pointer then this will return the
+ * pointed-to type rather than complaining.
+ */
+#define typeof_elt_of_field(type, field) typeof(((type *)0)->field[0])
+/* Check that field f in struct type t2 is an array of t1, of any size */
+#define type_check_varray(t1, t2, f) \
+(type_check(t1, typeof_elt_of_field(t2, f)) \
+ + QEMU_BUILD_BUG_ON_ZERO(!QEMU_IS_ARRAY(((t2 *)0)->f)))
#define vmstate_offset_value(_state, _field, _type) \
(offsetof(_state, _field) + \
@@ -253,6 +267,10 @@ extern const VMStateInfo vmstate_info_qtailq;
vmstate_offset_array(_state, _field, uint8_t,\
sizeof(typeof_field(_state, _field)))
+#define vmstate_offset_varray(_state, _field, _type) \
+(offsetof(_state, _field) + \
+ type_check_varray(_type, _state, _field))
+
/* In the macros below, if there is a _version, that means the macro's
* field will be processed only if the version being received is >=
* the _version specified. In general, if you add a new field, you
@@ -347,7 +365,7 @@ extern const VMStateInfo vmstate_info_qtailq;
.info = &(_info), \
.size = sizeof(_type), \
.flags = VMS_VARRAY_UINT32|VMS_MULTIPLY_ELEMENTS, \
-.offset = offsetof(_state, _field), \
+.offset = vmstate_offset_varray(_state, _field, _type), \
}
#define VMSTATE_ARRAY_TEST(_field, _state, _num, _test, _info, _type) {\
@@ -376,7 +394,7 @@ extern const VMStateInfo vmstate_info_qtailq;
.info = &(_info), \
.size = sizeof(_type), \
.flags = VMS_VARRAY_INT32, \
-.offset = offsetof(_state, _field), \
+.offset = vmstate_offset_varray(_state, _field, _type), \
}
#define VMSTATE_VARRAY_INT32(_field, _state, _field_num, _version, _info,
_type) {\
@@ -416,7 +434,7 @@ extern const VMStateInfo vmstate_info_qtailq;
.info = &(_info), \
.size = sizeof(_type), \
.flags = VMS_VARRAY_UINT16, \
-.offset = offsetof(_state, _field), \
+.offset = vmstate_offset_varray(_state, _field, _type), \
}
#define VMSTATE_VSTRUCT_TEST(_field, _state, _test, _version, _vmsd, _type,
_struct_version) { \
@@ -520,7 +538,7 @@ extern const VMStateInfo vmstate_info_qtailq;
.vmsd = &(_vmsd), \
.size = sizeof(_type), \
.flags = VMS_STRUCT|VMS_VARRAY_UINT8, \
-.offset = offsetof(_state, _field), \
+.offset = vmstate_offset_varray(_state, _field, _type),
[Qemu-devel] [PULL 1/5] pl330: fix vmstate description
From: Damien Hedde
Fix the pl330 main and queue vmstate description.
There were missing POINTER flags causing crashes during
incoming migration because:
+ PL330State chan field is a pointer to an array
+ PL330Queue queue field is a pointer to an array
Also bump corresponding vmsd version numbers.
Signed-off-by: Damien Hedde
Reviewed-by: Philippe Mathieu-Daude
Acked-by: Dr. David Alan Gilbert
Message-id: 20190724143553.21557-1-damien.he...@greensocs.com
Signed-off-by: Peter Maydell
---
hw/dma/pl330.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index 58df965a468..a56a3e77713 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -218,11 +218,12 @@ typedef struct PL330Queue {
static const VMStateDescription vmstate_pl330_queue = {
.name = "pl330_queue",
-.version_id = 1,
-.minimum_version_id = 1,
+.version_id = 2,
+.minimum_version_id = 2,
.fields = (VMStateField[]) {
-VMSTATE_STRUCT_VARRAY_UINT32(queue, PL330Queue, queue_size, 1,
- vmstate_pl330_queue_entry, PL330QueueEntry),
+VMSTATE_STRUCT_VARRAY_POINTER_UINT32(queue, PL330Queue, queue_size,
+ vmstate_pl330_queue_entry,
+ PL330QueueEntry),
VMSTATE_END_OF_LIST()
}
};
@@ -278,12 +279,12 @@ struct PL330State {
static const VMStateDescription vmstate_pl330 = {
.name = "pl330",
-.version_id = 1,
-.minimum_version_id = 1,
+.version_id = 2,
+.minimum_version_id = 2,
.fields = (VMStateField[]) {
VMSTATE_STRUCT(manager, PL330State, 0, vmstate_pl330_chan, PL330Chan),
-VMSTATE_STRUCT_VARRAY_UINT32(chan, PL330State, num_chnls, 0,
- vmstate_pl330_chan, PL330Chan),
+VMSTATE_STRUCT_VARRAY_POINTER_UINT32(chan, PL330State, num_chnls,
+ vmstate_pl330_chan, PL330Chan),
VMSTATE_VBUFFER_UINT32(lo_seqn, PL330State, 1, NULL, num_chnls),
VMSTATE_VBUFFER_UINT32(hi_seqn, PL330State, 1, NULL, num_chnls),
VMSTATE_STRUCT(fifo, PL330State, 0, vmstate_pl330_fifo, PL330Fifo),
--
2.20.1
[Qemu-devel] [PULL 2/5] stellaris_input: Fix vmstate description of buttons field
gamepad_state::buttons is a pointer to an array of structs,
not an array of structs, so should be declared in the vmstate
with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we
corrupt memory on incoming migration.
We bump the vmstate version field as the easiest way to
deal with the migration break, since migration wouldn't have
worked reliably before anyway.
Signed-off-by: Peter Maydell
Reviewed-by: Dr. David Alan Gilbert
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Damien Hedde
Message-id: 20190725163710.11703-2-peter.mayd...@linaro.org
---
hw/input/stellaris_input.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c
index 20c87d86f40..3a666d61d47 100644
--- a/hw/input/stellaris_input.c
+++ b/hw/input/stellaris_input.c
@@ -60,12 +60,14 @@ static const VMStateDescription vmstate_stellaris_button = {
static const VMStateDescription vmstate_stellaris_gamepad = {
.name = "stellaris_gamepad",
-.version_id = 1,
-.minimum_version_id = 1,
+.version_id = 2,
+.minimum_version_id = 2,
.fields = (VMStateField[]) {
VMSTATE_INT32(extension, gamepad_state),
-VMSTATE_STRUCT_VARRAY_INT32(buttons, gamepad_state, num_buttons, 0,
- vmstate_stellaris_button, gamepad_button),
+VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, gamepad_state,
+num_buttons,
+vmstate_stellaris_button,
+gamepad_button),
VMSTATE_END_OF_LIST()
}
};
--
2.20.1
[Qemu-devel] [PULL 0/5] target-arm queue
Handful of bug fixes to sneak in before rc3.
thanks
-- PMM
The following changes since commit c985266ea5b50e46e07b3568c1346e10064205c9:
Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20190726' into
staging (2019-07-26 13:52:06 +0100)
are available in the Git repository at:
https://git.linaro.org/people/pmaydell/qemu-arm.git
tags/pull-target-arm-20190726
for you to fetch changes up to 67505c114e6acc26f3a1a2b74833c61b6a34ff95:
hw/arm/boot: Further improve initrd positioning code (2019-07-26 16:17:56
+0100)
target-arm queue:
* Fix broken migration on pl330 device
* Fix broken migration on stellaris-input device
* Add type checks to vmstate varry macros to avoid this class of bugs
* hw/arm/boot: Fix some remaining cases where we would put the
initrd on top of the kernel image
Damien Hedde (1):
pl330: fix vmstate description
Peter Maydell (4):
stellaris_input: Fix vmstate description of buttons field
vmstate.h: Type check VMSTATE_STRUCT_VARRAY macros
hw/arm/boot: Rename elf_{low, high}_addr to image_{low, high}_addr
hw/arm/boot: Further improve initrd positioning code
include/migration/vmstate.h | 30 --
hw/arm/boot.c | 37 +++--
hw/dma/pl330.c | 17 +
hw/input/stellaris_input.c | 10 ++
4 files changed, 66 insertions(+), 28 deletions(-)
Re: [Qemu-devel] [PATCH v5] target/arm: generate a custom MIDR for -cpu max
On 7/26/19 4:39 AM, Alex Bennée wrote: > While most features are now detected by probing the ID_* registers > kernels can (and do) use MIDR_EL1 for working out of they have to > apply errata. This can trip up warnings in the kernel as it tries to > work out if it should apply workarounds to features that don't > actually exist in the reported CPU type. > > Avoid this problem by synthesising our own MIDR value. > > Signed-off-by: Alex Bennée > Reviewed-by: Peter Maydell > > --- > v2 > - don't leak QEMU version into ID reg > v3 > - move comment into one block > - explicit setting of more fields > v4 > - minor reword of comment > v5 > - VARIANT->PARTNUM and extra words > --- > target/arm/cpu.h | 6 ++ > target/arm/cpu64.c | 19 +++ > 2 files changed, 25 insertions(+) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH 3/3] spapr/irq: Drop spapr_irq_msi_reset()
On Fri, 26 Jul 2019 17:01:36 +0200
Cédric Le Goater wrote:
> On 26/07/2019 16:44, Greg Kurz wrote:
> > PHBs already take care of clearing the MSIs from the bitmap during reset
> > or unplug. No need to do this globally from the machine code. Rather add
> > an assert to ensure that PHBs have acted as expected.
>
> This works because spar_irq_reset() is called after qemu_devices_reset().
> I guess this is fine.
>
Yeah and we have this comment in spapr_machine_reset():
/*
* This is fixing some of the default configuration of the XIVE
* devices. To be called after the reset of the machine devices.
*/
spapr_irq_reset(spapr, _fatal);
I guess this is enough to prevent someone to break things.
> Reviewed-by: Cédric Le Goater
>
> Thanks,
>
> C.
>
> > Signed-off-by: Greg Kurz
> > ---
> > hw/ppc/spapr.c |4
> > hw/ppc/spapr_irq.c |7 ++-
> > include/hw/ppc/spapr_irq.h |1 -
> > 3 files changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 5894329f29a9..855e9fbd9805 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1739,10 +1739,6 @@ static void spapr_machine_reset(MachineState
> > *machine)
> > ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, _fatal);
> > }
> >
> > -if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> > -spapr_irq_msi_reset(spapr);
> > -}
> > -
> > /*
> > * NVLink2-connected GPU RAM needs to be placed on a separate NUMA
> > node.
> > * We assign a new numa ID per GPU in spapr_pci_collect_nvgpu() which
> > is
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index d07aed8ca9f9..c72d8433681d 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -57,11 +57,6 @@ void spapr_irq_msi_free(SpaprMachineState *spapr, int
> > irq, uint32_t num)
> > bitmap_clear(spapr->irq_map, irq - SPAPR_IRQ_MSI, num);
> > }
> >
> > -void spapr_irq_msi_reset(SpaprMachineState *spapr)
> > -{
> > -bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
> > -}
> > -
> > static void spapr_irq_init_kvm(SpaprMachineState *spapr,
> >SpaprIrq *irq, Error **errp)
> > {
> > @@ -729,6 +724,8 @@ int spapr_irq_post_load(SpaprMachineState *spapr, int
> > version_id)
> >
> > void spapr_irq_reset(SpaprMachineState *spapr, Error **errp)
> > {
> > +assert(bitmap_empty(spapr->irq_map, spapr->irq_map_nr));
> > +
> > if (spapr->irq->reset) {
> > spapr->irq->reset(spapr, errp);
> > }
> > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> > index f965a58f8954..44fe4f9e0e2e 100644
> > --- a/include/hw/ppc/spapr_irq.h
> > +++ b/include/hw/ppc/spapr_irq.h
> > @@ -28,7 +28,6 @@ void spapr_irq_msi_init(SpaprMachineState *spapr,
> > uint32_t nr_msis);
> > int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
> > Error **errp);
> > void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
> > -void spapr_irq_msi_reset(SpaprMachineState *spapr);
> >
> > typedef struct SpaprIrq {
> > uint32_tnr_irqs;
> >
>
Re: [Qemu-devel] [PATCH 0/2] ssh: add password and privkey auth methods
On 7/26/19 9:45 AM, Pino Toscano wrote:
> On Friday, 26 July 2019 16:27:11 CEST Richard W.M. Jones wrote:
>> On Fri, Jul 26, 2019 at 04:09:52PM +0200, Pino Toscano wrote:
>>> These two patches add the password and private key authentication
>>> methods to the ssh block driver, using secure objects for
>>> passwords/passphrases.
>>
>> I was attempting to test this but couldn't work out the full command
>> line to use it (with qemu-img). I got as far as:
>>
>> $ ./qemu-img convert -p 'json:{ "file.driver": "ssh", "file.host": "devr7",
>> "file.path": "/var/tmp/root", "file.password-secret": "..." }' /var/tmp/root
>>
>> I guess the secret should be specified using --object, but at that
>> point I gave up.
>
> Almost there :) add e.g.
> --object 'secret,id=sec0,file=passwd'
> as parameter for the convert command (so after it, not before), and then
> set 'sec0' as value for file.password-secret. Of course 'sec0' is
> arbitrary, any other QEMU id will do.
>
> A long helpful comment in include/crypto/secret.h explains the basics
> of the crypto objects.
That is useful information, but even more useful if you amend the commit
message to include a working example command line rather than making
readers chase down the docs :)
Untested, but piecing together what I know from my work on qemu-nbd
encryption, it seems like this should be a starting point for such a
command:
qemu-img convert -p --imageopts --object secret,id=sec0,file=passwd \
driver=ssh,host=devr7,path=/var/tmp/root,password-secret=sec0 \
/var/tmp/copy
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 3/3] spapr/irq: Drop spapr_irq_msi_reset()
On 26/07/2019 16:44, Greg Kurz wrote:
> PHBs already take care of clearing the MSIs from the bitmap during reset
> or unplug. No need to do this globally from the machine code. Rather add
> an assert to ensure that PHBs have acted as expected.
This works because spar_irq_reset() is called after qemu_devices_reset().
I guess this is fine.
Reviewed-by: Cédric Le Goater
Thanks,
C.
> Signed-off-by: Greg Kurz
> ---
> hw/ppc/spapr.c |4
> hw/ppc/spapr_irq.c |7 ++-
> include/hw/ppc/spapr_irq.h |1 -
> 3 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5894329f29a9..855e9fbd9805 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1739,10 +1739,6 @@ static void spapr_machine_reset(MachineState *machine)
> ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, _fatal);
> }
>
> -if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> -spapr_irq_msi_reset(spapr);
> -}
> -
> /*
> * NVLink2-connected GPU RAM needs to be placed on a separate NUMA node.
> * We assign a new numa ID per GPU in spapr_pci_collect_nvgpu() which is
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index d07aed8ca9f9..c72d8433681d 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -57,11 +57,6 @@ void spapr_irq_msi_free(SpaprMachineState *spapr, int irq,
> uint32_t num)
> bitmap_clear(spapr->irq_map, irq - SPAPR_IRQ_MSI, num);
> }
>
> -void spapr_irq_msi_reset(SpaprMachineState *spapr)
> -{
> -bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
> -}
> -
> static void spapr_irq_init_kvm(SpaprMachineState *spapr,
>SpaprIrq *irq, Error **errp)
> {
> @@ -729,6 +724,8 @@ int spapr_irq_post_load(SpaprMachineState *spapr, int
> version_id)
>
> void spapr_irq_reset(SpaprMachineState *spapr, Error **errp)
> {
> +assert(bitmap_empty(spapr->irq_map, spapr->irq_map_nr));
> +
> if (spapr->irq->reset) {
> spapr->irq->reset(spapr, errp);
> }
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index f965a58f8954..44fe4f9e0e2e 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -28,7 +28,6 @@ void spapr_irq_msi_init(SpaprMachineState *spapr, uint32_t
> nr_msis);
> int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
> Error **errp);
> void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
> -void spapr_irq_msi_reset(SpaprMachineState *spapr);
>
> typedef struct SpaprIrq {
> uint32_tnr_irqs;
>
Re: [Qemu-devel] [PATCH 1/3] spapr/pci: Consolidate de-allocation of MSIs
On 26/07/2019 16:44, Greg Kurz wrote:
> When freeing MSIs, we need to:
> - remove them from the machine's MSI bitmap
> - remove them from the IC backend
> - remove them from the PHB's MSI cache
>
> This is currently open coded in two places in rtas_ibm_change_msi(),
> and we're about to need this in spapr_phb_reset() as well. Instead of
> duplicating this code again, make it a destroy function for the PHB's
> MSI cache. Removing an MSI device from the cache will call the destroy
> function internally.
This looks good.
Reviewed-by: Cédric Le Goater
Thanks,
C.
> Signed-off-by: Greg Kurz
> ---
> hw/ppc/spapr_pci.c | 24 +++-
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2d5697d119f4..bc22568bfa71 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -336,10 +336,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
> return;
> }
>
> -if (!smc->legacy_irq_allocation) {
> -spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
> -}
> -spapr_irq_free(spapr, msi->first_irq, msi->num);
> if (msi_present(pdev)) {
> spapr_msi_setmsg(pdev, 0, false, 0, 0);
> }
> @@ -409,10 +405,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
>
> /* Release previous MSIs */
> if (msi) {
> -if (!smc->legacy_irq_allocation) {
> -spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
> -}
> -spapr_irq_free(spapr, msi->first_irq, msi->num);
> g_hash_table_remove(phb->msi, _addr);
> }
>
> @@ -1806,6 +1798,19 @@ static void spapr_phb_unrealize(DeviceState *dev,
> Error **errp)
> memory_region_del_subregion(get_system_memory(), >mem32window);
> }
>
> +static void spapr_phb_destroy_msi(gpointer opaque)
> +{
> +SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +spapr_pci_msi *msi = opaque;
> +
> +if (!smc->legacy_irq_allocation) {
> +spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
> +}
> +spapr_irq_free(spapr, msi->first_irq, msi->num);
> +g_free(msi);
> +}
> +
> static void spapr_phb_realize(DeviceState *dev, Error **errp)
> {
> /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
> @@ -2017,7 +2022,8 @@ static void spapr_phb_realize(DeviceState *dev, Error
> **errp)
> spapr_tce_get_iommu(tcet));
> }
>
> -sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free,
> g_free);
> +sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free,
> + spapr_phb_destroy_msi);
> return;
>
> unrealize:
>
Re: [Qemu-devel] [PATCH v5 14/15] target/sparc: Add TLB entry with attributes
On 7/25/19 11:48 PM, tony.ngu...@bt.com wrote: > Append MemTxAttrs to interfaces so we can pass along up coming Invert > Endian TTE bit on SPARC64. > > Signed-off-by: Tony Nguyen > --- > target/sparc/mmu_helper.c | 32 ++-- > 1 file changed, 18 insertions(+), 14 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH v5 15/15] target/sparc: sun4u Invert Endian TTE bit
On 7/25/19 11:49 PM, tony.ngu...@bt.com wrote: > This bit configures endianness of PCI MMIO devices. It is used by > Solaris and OpenBSD sunhme drivers. > > Tested working on OpenBSD. > > Unfortunately Solaris 10 had a unrelated keyboard issue blocking > testing... another inch towards Solaris 10 on SPARC64 =) > > Signed-off-by: Tony Nguyen > --- > target/sparc/cpu.h| 2 ++ > target/sparc/mmu_helper.c | 8 +++- > 2 files changed, 9 insertions(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH 2/3] spapr/pci: Free MSIs during reset
On 26/07/2019 16:44, Greg Kurz wrote:
> When the machine is reset, the MSI bitmap is cleared but the allocated
> MSIs are not freed. Some operating systems, such as AIX, can detect the
> previous configuration and assert.
>
> Empty the MSI cache, this performs the needed cleanup.
This is fixing the reset bug.
Reviewed-by: Cédric Le Goater
Thanks,
C.
>
> Signed-off-by: Greg Kurz
> ---
> hw/ppc/spapr_pci.c |2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index bc22568bfa71..e45507bf2b53 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2078,6 +2078,8 @@ static void spapr_phb_reset(DeviceState *qdev)
> if (spapr_phb_eeh_available(SPAPR_PCI_HOST_BRIDGE(qdev))) {
> spapr_phb_vfio_reset(qdev);
> }
> +
> +g_hash_table_remove_all(sphb->msi);
> }
>
> static Property spapr_phb_properties[] = {
>
Re: [Qemu-devel] [PATCH v5 13/15] cputlb: Byte swap memory transaction attribute
On 7/25/19 11:48 PM, tony.ngu...@bt.com wrote:
> Notice new attribute, byte swap, and force the transaction through the
> memory slow path.
>
> Required by architectures that can invert endianness of memory
> transaction, e.g. SPARC64 has the Invert Endian TTE bit.
>
> Signed-off-by: Tony Nguyen
> ---
> accel/tcg/cputlb.c | 11 +++
> include/exec/memattrs.h | 2 ++
> 2 files changed, 13 insertions(+)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index e61b1eb..f292a87 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -738,6 +738,9 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong
> vaddr,
> */
> address |= TLB_RECHECK;
> }
> +if (attrs.byte_swap) {
> +address |= TLB_FORCE_SLOW;
> +}
> if (!memory_region_is_ram(section->mr) &&
> !memory_region_is_romd(section->mr)) {
> /* IO memory case */
> @@ -891,6 +894,10 @@ static uint64_t io_readx(CPUArchState *env,
> CPUIOTLBEntry *iotlbentry,
> bool locked = false;
> MemTxResult r;
>
> +if (iotlbentry->attrs.byte_swap) {
> +op ^= MO_BSWAP;
> +}
> +
> section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
> mr = section->mr;
> mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
> @@ -933,6 +940,10 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry
> *iotlbentry,
> bool locked = false;
> MemTxResult r;
>
> +if (iotlbentry->attrs.byte_swap) {
> +op ^= MO_BSWAP;
> +}
> +
> section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
> mr = section->mr;
> mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index d4a3477..a0644eb 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -37,6 +37,8 @@ typedef struct MemTxAttrs {
> unsigned int user:1;
> /* Requester ID (for MSI for example) */
> unsigned int requester_id:16;
> +/* SPARC64: TTE invert endianness */
> +unsigned int byte_swap:1;
Don't mention Sparc here, otherwise it seems like it only applies to Sparc,
when it is really a generic feature only currently used by Sparc.
Just say "Invert endianness for this page".
With that,
Reviewed-by: Richard Henderson
r~
Re: [Qemu-devel] [PATCH 0/2] ssh: add password and privkey auth methods
On Fri, Jul 26, 2019 at 04:45:03PM +0200, Pino Toscano wrote:
> On Friday, 26 July 2019 16:27:11 CEST Richard W.M. Jones wrote:
> > On Fri, Jul 26, 2019 at 04:09:52PM +0200, Pino Toscano wrote:
> > > These two patches add the password and private key authentication
> > > methods to the ssh block driver, using secure objects for
> > > passwords/passphrases.
> >
> > I was attempting to test this but couldn't work out the full command
> > line to use it (with qemu-img). I got as far as:
> >
> > $ ./qemu-img convert -p 'json:{ "file.driver": "ssh", "file.host": "devr7",
> > "file.path": "/var/tmp/root", "file.password-secret": "..." }' /var/tmp/root
> >
> > I guess the secret should be specified using --object, but at that
> > point I gave up.
>
> Almost there :) add e.g.
> --object 'secret,id=sec0,file=passwd'
> as parameter for the convert command (so after it, not before), and then
> set 'sec0' as value for file.password-secret. Of course 'sec0' is
> arbitrary, any other QEMU id will do.
>
> A long helpful comment in include/crypto/secret.h explains the basics
> of the crypto objects.
OK, the password part of this patch does work, so:
Tested-by: Richard W.M. Jones
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
Re: [Qemu-devel] [PATCH v27 5/8] target/avr: Add limited support for USART and 16 bit timer peripherals
Hi Michael and Pavel, The USART was based on the ATMega2560. It was designed for testing so its functionality is somewhat limited. Peripherals seem to vary between AVR chips so the configuration in the 2560 may not match other chips, especially the older ones. >From memory, the only shared registers in the 2560 USART are the PRR's, which >we implemented by adding single byte memory regions during board >initialisation (so that the memory region wasn't part of any one device). I expect there are cleaner ways to do it. Kind regards, Sarah Harris On Thu, 25 Jul 2019 20:52:33 +0300 Michael Rolnik wrote: > Hi Pavel. > > Please see my answers below. > > On Thu, Jul 25, 2019 at 1:00 PM Pavel Dovgalyuk wrote: > > > > From: Qemu-devel [mailto:qemu-devel-bounces+patchwork-qemu- > > > devel=patchwork.kernel@nongnu.org] On Behalf Of Michael Rolnik > > > From: Sarah Harris > > > > > > These were designed to facilitate testing but should provide enough > > function to be useful in > > > other contexts. > > > > USART is very useful for testing, but to which model of AVR is belongs? > > We also started implementation of USART and other devices in our > > internship program, > > using prior version of your patches. > > There were other register addresses for the registers and some of them > > even intersect > > making read/write logic more complex (we looked at Atmega8). > > > > This is a question for Sara as she built it. (I think it was ATmega2560) > > > > > > You also mix the board and the SoC into one file, making hardware-on-chip > > harder to reuse. > > > > I think that the structure can be revised in the following way: > > Board -> SoC -> Devices > > > > Board includes SoC, loads the firmware, and adds some external peripheral > > devices, if needed. > > > > SoC includes embedded peripherals. It dispatches IO memory accesses and > > passes them > > to the devices. In this case you can have different register addresses for > > different SoCs, but > > the embedded device emulation code can be mostly the same for simple > > devices like USART. > > > > > Only a subset of the functions of each peripheral is implemented, mainly > > due to the lack of a > > > standard way to handle electrical connections (like GPIO pins). > > > You are right. > > > > > We did not got too much results, you can check for our changes here: > > https://github.com/Dovgalyuk/qemu/tree/avr8 > > > > But we can help you in development of better version of the patches and > > split the work > > for making this platform more usable. > > > > Gladly. > You are welcome. > My initial intent was to provide CPU only and I hoped that others will > model the devices. > > *Richard, Phil & all,* > what would be the right mechanism / procedure to split the work? > > > > > > Pavel Dovgalyuk > > > > > > -- > Best Regards, > Michael Rolnik
Re: [Qemu-devel] [PATCH v5 12/15] cpu: TLB_FLAGS_MASK bit to force memory slow path
On 7/25/19 11:48 PM, tony.ngu...@bt.com wrote: > The fast path is taken when TLB_FLAGS_MASK is all zero. > > TLB_FORCE_SLOW is simply a TLB_FLAGS_MASK bit to force the slow path, > there are no other side effects. > > Signed-off-by: Tony Nguyen > --- > include/exec/cpu-all.h | 10 -- > 1 file changed, 8 insertions(+), 2 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH v4 3/7] accel: collecting JIT statistics
vandersonmr writes:
> If a TB has a TBS (TBStatistics) with the TB_JIT_STATS
> enabled then we collect statistics of its translation
> processes and code translation. To collect the number
> of host instructions we used a modified version of the
> disas function to pass through the whole code without
> printing anything (fake_fprintf) but counting the number
> of instructions.
>
> Signed-off-by: vandersonmr
> ---
> accel/tcg/translate-all.c | 18 +++
> accel/tcg/translator.c| 5 ++
> disas.c | 108 ++
> include/disas/disas.h | 1 +
> include/exec/tb-stats.h | 14 +
> include/qemu/log.h| 1 +
> tcg/tcg.c | 23
> tcg/tcg.h | 2 +
> 8 files changed, 172 insertions(+)
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 7497dae508..3a47ac6f2c 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1793,6 +1793,10 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> }
> }
>
> +if (flag & TB_JIT_STATS) {
> +tb->tb_stats->stats_enabled |= TB_JIT_STATS;
> +atomic_inc(>tb_stats->translations.total);
> +}
> } else {
> tb->tb_stats = NULL;
> }
> @@ -1870,6 +1874,17 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> atomic_set(>search_out_len, prof->search_out_len + search_size);
> #endif
>
> +if (tb_stats_enabled(tb, TB_JIT_STATS)) {
> +size_t code_size = gen_code_size;
> +if (tcg_ctx->data_gen_ptr) {
> +code_size = tcg_ctx->data_gen_ptr - tb->tc.ptr;
> +}
> +
> +atomic_add(>tb_stats->code.num_host_inst,
> +get_num_insts(tb->tc.ptr, code_size));
This is what is causing my crashes. I think invoking the disassembler stuff is
too clunky here. Maybe we should just change the counter to
num_host_insn_byte, the ratio will still be interesting but it saves the
complication of re-counting.
Ideally the we'd like the core tcg code to tell us how many host
instructions it emitted for each tcg_op but I suspect that involves
heavier surgery.
> +}
> +
> +
> #ifdef DEBUG_DISAS
> if (qemu_loglevel_mask(CPU_LOG_TB_OUT_ASM) &&
> qemu_log_in_addr_range(tb->pc)) {
> @@ -1927,6 +1942,9 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> phys_page2 = -1;
> if ((pc & TARGET_PAGE_MASK) != virt_page2) {
> phys_page2 = get_page_addr_code(env, virt_page2);
> +if (tb_stats_enabled(tb, TB_JIT_STATS)) {
> +atomic_inc(>tb_stats->translations.spanning);
> +}
> }
> /*
> * No explicit memory barrier is required -- tb_link_page() makes the
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index 396a11e828..03c00bdb1b 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -117,6 +117,11 @@ void translator_loop(const TranslatorOps *ops,
> DisasContextBase *db,
> db->tb->size = db->pc_next - db->pc_first;
> db->tb->icount = db->num_insns;
>
> +if (tb_stats_enabled(tb, TB_JIT_STATS)) {
> +atomic_add(>tb->tb_stats->code.num_guest_inst, db->num_insns);
> +}
> +
> +
> #ifdef DEBUG_DISAS
> if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
> && qemu_log_in_addr_range(db->pc_first)) {
> diff --git a/disas.c b/disas.c
> index 3e2bfa572b..5dec754992 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -475,6 +475,114 @@ void target_disas(FILE *out, CPUState *cpu,
> target_ulong code,
> }
> }
>
> +static int fprintf_fake(struct _IO_FILE *a, const char *b, ...)
> +{
> +return 1;
> +}
> +
> +/*
> + * This is a work around to get the number of host instructions with
> + * a small effort. It reuses the disas function with a fake printf to
> + * print nothing but count the number of instructions.
> + *
> + */
> +unsigned get_num_insts(void *code, unsigned long size)
> +{
> +uintptr_t pc;
> +int count;
> +CPUDebug s;
> +int (*print_insn)(bfd_vma pc, disassemble_info *info) = NULL;
> +
> +INIT_DISASSEMBLE_INFO(s.info, NULL, fprintf_fake);
> +s.info.print_address_func = generic_print_host_address;
> +
> +s.info.buffer = code;
> +s.info.buffer_vma = (uintptr_t)code;
> +s.info.buffer_length = size;
> +s.info.cap_arch = -1;
> +s.info.cap_mode = 0;
> +s.info.cap_insn_unit = 4;
> +s.info.cap_insn_split = 4;
> +
> +#ifdef HOST_WORDS_BIGENDIAN
> +s.info.endian = BFD_ENDIAN_BIG;
> +#else
> +s.info.endian = BFD_ENDIAN_LITTLE;
> +#endif
> +#if defined(CONFIG_TCG_INTERPRETER)
> +print_insn = print_insn_tci;
> +#elif defined(__i386__)
> +s.info.mach = bfd_mach_i386_i386;
> +print_insn = print_insn_i386;
> +s.info.cap_arch = CS_ARCH_X86;
> +s.info.cap_mode = CS_MODE_32;
> +s.info.cap_insn_unit = 1;
> +s.info.cap_insn_split = 8;
> +#elif defined(__x86_64__)
> +s.info.mach = bfd_mach_x86_64;
> +print_insn =
[Qemu-devel] [PATCH 3/3] spapr/irq: Drop spapr_irq_msi_reset()
PHBs already take care of clearing the MSIs from the bitmap during reset
or unplug. No need to do this globally from the machine code. Rather add
an assert to ensure that PHBs have acted as expected.
Signed-off-by: Greg Kurz
---
hw/ppc/spapr.c |4
hw/ppc/spapr_irq.c |7 ++-
include/hw/ppc/spapr_irq.h |1 -
3 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5894329f29a9..855e9fbd9805 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1739,10 +1739,6 @@ static void spapr_machine_reset(MachineState *machine)
ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, _fatal);
}
-if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
-spapr_irq_msi_reset(spapr);
-}
-
/*
* NVLink2-connected GPU RAM needs to be placed on a separate NUMA node.
* We assign a new numa ID per GPU in spapr_pci_collect_nvgpu() which is
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index d07aed8ca9f9..c72d8433681d 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -57,11 +57,6 @@ void spapr_irq_msi_free(SpaprMachineState *spapr, int irq,
uint32_t num)
bitmap_clear(spapr->irq_map, irq - SPAPR_IRQ_MSI, num);
}
-void spapr_irq_msi_reset(SpaprMachineState *spapr)
-{
-bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
-}
-
static void spapr_irq_init_kvm(SpaprMachineState *spapr,
SpaprIrq *irq, Error **errp)
{
@@ -729,6 +724,8 @@ int spapr_irq_post_load(SpaprMachineState *spapr, int
version_id)
void spapr_irq_reset(SpaprMachineState *spapr, Error **errp)
{
+assert(bitmap_empty(spapr->irq_map, spapr->irq_map_nr));
+
if (spapr->irq->reset) {
spapr->irq->reset(spapr, errp);
}
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index f965a58f8954..44fe4f9e0e2e 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -28,7 +28,6 @@ void spapr_irq_msi_init(SpaprMachineState *spapr, uint32_t
nr_msis);
int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
Error **errp);
void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
-void spapr_irq_msi_reset(SpaprMachineState *spapr);
typedef struct SpaprIrq {
uint32_tnr_irqs;
Re: [Qemu-devel] [PATCH 0/2] ssh: add password and privkey auth methods
On Friday, 26 July 2019 16:27:11 CEST Richard W.M. Jones wrote:
> On Fri, Jul 26, 2019 at 04:09:52PM +0200, Pino Toscano wrote:
> > These two patches add the password and private key authentication
> > methods to the ssh block driver, using secure objects for
> > passwords/passphrases.
>
> I was attempting to test this but couldn't work out the full command
> line to use it (with qemu-img). I got as far as:
>
> $ ./qemu-img convert -p 'json:{ "file.driver": "ssh", "file.host": "devr7",
> "file.path": "/var/tmp/root", "file.password-secret": "..." }' /var/tmp/root
>
> I guess the secret should be specified using --object, but at that
> point I gave up.
Almost there :) add e.g.
--object 'secret,id=sec0,file=passwd'
as parameter for the convert command (so after it, not before), and then
set 'sec0' as value for file.password-secret. Of course 'sec0' is
arbitrary, any other QEMU id will do.
A long helpful comment in include/crypto/secret.h explains the basics
of the crypto objects.
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH v5 11/15] memory: Single byte swap along the I/O path
On 7/26/19 2:39 AM, Paolo Bonzini wrote:
> Then memory_region_endianness_inverted can be:
>
> if (mr->ops->endianness == DEVICE_NATIVE_ENDIAN)
> return (op & MO_BSWAP) != MO_TE;
> else if (mr->ops->endianness == DEVICE_BIG_ENDIAN)
> return (op & MO_BSWAP) != MO_BE;
> else if (mr->ops->endianness == DEVICE_LITTLE_ENDIAN)
> return (op & MO_BSWAP) != MO_LE;
Possibly outside the scope of this patch set, I'd like to replace enum
device_endian with MemOp, with exactly the above enumerator replacement.
In the meantime, perhaps a conversion function
static MemOp devendian_memop(enum device_endian d)
{
switch (d) {
case DEVICE_NATIVE_ENDIAN:
return MO_TE;
case DEVICE_BIG_ENDIAN:
return MO_BE;
case DEVICE_LITTLE_ENDIAN:
return MO_LE;
default:
g_assert_not_reached();
}
}
With that, this would simplify to
return (op & MO_BSWAP) != devendian_memop(mr->ops->endianness);
> I think the changes should be split in two parts. Before this patch,
> you modify all callers of memory_region_dispatch_* so that they already
> pass the right endianness op; however, you leave the existing swap in
> place. So for example in load_helper you'd have in a previous patch
>
> +/* FIXME: io_readx ignores MO_BSWAP. */
> +op = SIZE_MEMOP(size) | (big_endian ? MO_BE : MO_LE);
> res = io_readx(env, _tlb(env)->d[mmu_idx].iotlb[index],
> - mmu_idx, addr, retaddr, access_type,
> SIZE_MEMOP(size));
> + mmu_idx, addr, retaddr, access_type, op);
> return handle_bswap(res, size, big_endian);
>
> Then, in this patch, you remove the handle_bswap call as well as the
> FIXME comment.
Agreed.
r~
[Qemu-devel] [PATCH 2/3] spapr/pci: Free MSIs during reset
When the machine is reset, the MSI bitmap is cleared but the allocated
MSIs are not freed. Some operating systems, such as AIX, can detect the
previous configuration and assert.
Empty the MSI cache, this performs the needed cleanup.
Signed-off-by: Greg Kurz
---
hw/ppc/spapr_pci.c |2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index bc22568bfa71..e45507bf2b53 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2078,6 +2078,8 @@ static void spapr_phb_reset(DeviceState *qdev)
if (spapr_phb_eeh_available(SPAPR_PCI_HOST_BRIDGE(qdev))) {
spapr_phb_vfio_reset(qdev);
}
+
+g_hash_table_remove_all(sphb->msi);
}
static Property spapr_phb_properties[] = {
[Qemu-devel] [PATCH 1/3] spapr/pci: Consolidate de-allocation of MSIs
When freeing MSIs, we need to:
- remove them from the machine's MSI bitmap
- remove them from the IC backend
- remove them from the PHB's MSI cache
This is currently open coded in two places in rtas_ibm_change_msi(),
and we're about to need this in spapr_phb_reset() as well. Instead of
duplicating this code again, make it a destroy function for the PHB's
MSI cache. Removing an MSI device from the cache will call the destroy
function internally.
Signed-off-by: Greg Kurz
---
hw/ppc/spapr_pci.c | 24 +++-
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2d5697d119f4..bc22568bfa71 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -336,10 +336,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu,
SpaprMachineState *spapr,
return;
}
-if (!smc->legacy_irq_allocation) {
-spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
-}
-spapr_irq_free(spapr, msi->first_irq, msi->num);
if (msi_present(pdev)) {
spapr_msi_setmsg(pdev, 0, false, 0, 0);
}
@@ -409,10 +405,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu,
SpaprMachineState *spapr,
/* Release previous MSIs */
if (msi) {
-if (!smc->legacy_irq_allocation) {
-spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
-}
-spapr_irq_free(spapr, msi->first_irq, msi->num);
g_hash_table_remove(phb->msi, _addr);
}
@@ -1806,6 +1798,19 @@ static void spapr_phb_unrealize(DeviceState *dev, Error
**errp)
memory_region_del_subregion(get_system_memory(), >mem32window);
}
+static void spapr_phb_destroy_msi(gpointer opaque)
+{
+SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+spapr_pci_msi *msi = opaque;
+
+if (!smc->legacy_irq_allocation) {
+spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
+}
+spapr_irq_free(spapr, msi->first_irq, msi->num);
+g_free(msi);
+}
+
static void spapr_phb_realize(DeviceState *dev, Error **errp)
{
/* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
@@ -2017,7 +2022,8 @@ static void spapr_phb_realize(DeviceState *dev, Error
**errp)
spapr_tce_get_iommu(tcet));
}
-sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
+sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free,
+ spapr_phb_destroy_msi);
return;
unrealize:
[Qemu-devel] [PATCH 0/3] spapr/pci: Improve MSI tracking
Some recent tests with AIX guests showed that we don't tear down MSIs that were allocated with the "change-msi" RTAS call, when the guest is rebooted. This series teach PHBs to do the cleanup at reset time. This bug has always been there. Not sure it is worth the pain to have this fixed in 4.1. -- Greg --- Greg Kurz (3): spapr/pci: Consolidate de-allocation of MSIs spapr/pci: Free MSIs during reset spapr/irq: Drop spapr_irq_msi_reset() hw/ppc/spapr.c |4 hw/ppc/spapr_irq.c |7 ++- hw/ppc/spapr_pci.c | 26 +- include/hw/ppc/spapr_irq.h |1 - 4 files changed, 19 insertions(+), 19 deletions(-)
Re: [Qemu-devel] [PATCH 1/2] net: assert that tx packets have nonzero size
On Tue, 2019-07-23 at 11:38 +0800, Jason Wang wrote:
> On 2019/7/20 上午2:52, Oleinik, Alexander wrote:
> > Virtual devices should not try to send zero-sized packets. The
> > caller
> > should check the size prior to calling qemu_sendv_packet_async.
> >
> > Signed-off-by: Alexander Oleinik
> > ---
> > net/net.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/net/net.c b/net/net.c
> > index 7d4098254f..fad20bc611 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -741,6 +741,9 @@ ssize_t qemu_sendv_packet_async(NetClientState
> > *sender,
> > size_t size = iov_size(iov, iovcnt);
> > int ret;
> >
> > +/* 0-sized packets are unsupported. Check size in the caller
> > */
> > +assert(size);
>
> Can this be triggered through a buggy device by guest? If yes, we
> need
> avoid using assert() here.
Yes - for example, virtio-net could trigger trigger it because there is
no size check prior to qemu_sendv_packet_async, although PATCH 2/2
should fix this.
Instead of the assertion, should we return a negative value?
> Thanks
>
>
> > +
> > if (size > NET_BUFSIZE) {
> > return size;
> > }
Re: [Qemu-devel] [PULL for-4.1 0/1] s390x: vfio-ccw maintainership update
On Fri, 26 Jul 2019 at 12:27, Cornelia Huck wrote: > > The following changes since commit bf8b024372bf8abf5a9f40bfa65eeefad23ff988: > > Update version for v4.1.0-rc2 release (2019-07-23 18:28:08 +0100) > > are available in the Git repository at: > > https://github.com/cohuck/qemu tags/s390x-20190726 > > for you to fetch changes up to 6ef2d01abf44fa86f2de4bcde3be9391259bf718: > > MAINTAINERS: vfio-ccw: Remove myself as the maintainer (2019-07-25 09:36:14 > +0200) > > > MAINTAINERS update for vfio-ccw > > > > Farhan Ali (1): > MAINTAINERS: vfio-ccw: Remove myself as the maintainer > > MAINTAINERS | 1 - > 1 file changed, 1 deletion(-) > > -- > 2.20.1 Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/4.1 for any user-visible changes. -- PMM
Re: [Qemu-devel] [PATCH 03/28] qapi: Split error.json off common.json
Eric Blake writes:
> On 7/26/19 7:05 AM, Markus Armbruster wrote:
>> In my "build everything" tree, changing a type in qapi/common.json
>> triggers a recompile of some 3600 out of 6600 objects (not counting
>> tests and objects that don't depend on qemu/osdep.h).
>>
>> One common dependency is QapiErrorClass: it's used only in in
>> qapi/error.h, which uses nothing else, and is widely included.
>>
>> Move QapiErrorClass from common.json to new error.json. Touching
>> common.json now recompiles only some 2900 objects.
>>
>> Cc: Eric Blake
>> Signed-off-by: Markus Armbruster
>> ---
>> MAINTAINERS | 2 ++
>> include/qapi/error.h | 2 +-
>> qapi/Makefile.objs| 2 +-
>> qapi/common.json | 24
>> qapi/error.json | 29 +
>> qapi/qapi-schema.json | 1 +
>> 6 files changed, 34 insertions(+), 26 deletions(-)
>> create mode 100644 qapi/error.json
>
>
>> +++ b/qapi/qapi-schema.json
>> @@ -80,6 +80,7 @@
>> # stable order, it's best to include each sub-schema just once, or
>> # include it first right here.
>>
>> +{ 'include': 'error.json' }
>> { 'include': 'common.json' }
>> { 'include': 'sockets.json' }
>> { 'include': 'run-state.json' }
>
> Any reason why error.json is needed before common.json? But I don't see
> it as being a problem, so
The QAPI language doesn't require definition before use. We commonly do
it anyway, because we find it easier to read.
error.json uses nothing and defines only enum QapiErrorClass, which is
implicitly used by commands that can fail. I like to put the enum
before the first command. Putting it before any other module is
easiest.
> Reviewed-by: Eric Blake
Thanks!
Re: [Qemu-devel] [PATCH v5 11/15] memory: Single byte swap along the I/O path
On 7/26/19 2:26 AM, Paolo Bonzini wrote:
> On 26/07/19 08:47, tony.ngu...@bt.com wrote:
>> + op = SIZE_MEMOP(size);
>> + if (need_bswap(big_endian)) {
>> + op ^= MO_BSWAP;
>> + }
>
> And this has the same issue as the first version. It should be
>
> op = SIZE_MEMOP(size) | (big_endian ? MO_BE : MO_LE);
>
> and everything should work. If it doesn't (and indeed it doesn't :)) it
> means you have bugs somewhere else.
As I mentioned against patch 9, which also touches this area, it should be
using the MemOp that is already passed in to this function instead of building
a new one from scratch.
But, yes, any failure in that would mean bugs somewhere else. ;-)
r~
Re: [Qemu-devel] [PATCH 2/2] ssh: implement private key authentication
On Fri, Jul 26, 2019 at 09:24:34AM -0500, Eric Blake wrote: > On a different topic, how much of this work overlaps with the nbdkit ssh > plugin? Should we be duplicating efforts with both projects supporting > ssh natively, or is it worth considering getting qemu out of the ssh > business and instead connecting to an nbd device provided by nbdkit > connecting to ssh? (For comparison, we've already decided that nbdkit > does not plan on writing a qcow2 plugin, because it defers to qemu to be > the expert there; or in the other direction, qemu-nbd has deprecated its > partial support for exposing only a partition of a disk in favor of > qemu-nbd having much more partition support through its filters) I think it would be good if libvirt could handle this usage, so it would set up the nbdkit process, set up seccomp or SELinux to confine it, and kill nbdkit afterwards. See also: https://rwmj.wordpress.com/2018/10/30/split-block-drivers-from-qemu-with-nbdkit/ Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Re: [Qemu-devel] [PATCH 0/2] ssh: add password and privkey auth methods
On Fri, Jul 26, 2019 at 04:09:52PM +0200, Pino Toscano wrote:
> These two patches add the password and private key authentication
> methods to the ssh block driver, using secure objects for
> passwords/passphrases.
I was attempting to test this but couldn't work out the full command
line to use it (with qemu-img). I got as far as:
$ ./qemu-img convert -p 'json:{ "file.driver": "ssh", "file.host": "devr7",
"file.path": "/var/tmp/root", "file.password-secret": "..." }' /var/tmp/root
I guess the secret should be specified using --object, but at that
point I gave up.
Could do with documentation, even if merely in the commit message or a
blog post.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
Re: [Qemu-devel] [PATCH v5 10/15] memory: Access MemoryRegion with MemOp semantics
On 7/25/19 11:47 PM, tony.ngu...@bt.com wrote:
> To convert interfaces of MemoryRegion access, MEMOP_SIZE and
> SIZE_MEMOP no-op stubs were introduced to change syntax while keeping
> the existing semantics.
>
> Now with interfaces converted, we fill the stubs and use MemOp
> semantics.
>
> Signed-off-by: Tony Nguyen
> ---
> include/exec/memop.h | 5 ++---
> include/exec/memory.h | 4 ++--
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/include/exec/memop.h b/include/exec/memop.h
> index 09c8d20..f2847e8 100644
> --- a/include/exec/memop.h
> +++ b/include/exec/memop.h
> @@ -106,8 +106,7 @@ typedef enum MemOp {
> MO_SSIZE = MO_SIZE | MO_SIGN,
> } MemOp;
>
> -/* No-op while memory_region_dispatch_[read|write] is converted to MemOp */
> -#define MEMOP_SIZE(op) (op)/* MemOp to size. */
> -#define SIZE_MEMOP(ul) (ul)/* Size to MemOp. */
> +#define MEMOP_SIZE(op) (1 << ((op) & MO_SIZE)) /* MemOp to size. */
> +#define SIZE_MEMOP(ul) (ctzl(ul)) /* Size to MemOp. */
As mentioned, I'd prefer inline functions.
I think it wouldn't go amiss to do
static inline MemOp size_memop(unsigned size)
{
#ifdef CONFIG_DEBUG_TCG
/* power of 2 up to 8 */
assert((size & (size - 1)) == 0 && size >= 1 && size <= 8);
#endif
return ctz32(size);
}
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 0ea4843..975b86a 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1732,7 +1732,7 @@ void mtree_info(bool flatview, bool dispatch_tree, bool
> owner);
> * @mr: #MemoryRegion to access
> * @addr: address within that region
> * @pval: pointer to uint64_t which the data is written to
> - * @op: size of the access in bytes
> + * @op: size, sign, and endianness of the memory operation
> * @attrs: memory transaction attributes to use for the access
> */
> MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
> @@ -1747,7 +1747,7 @@ MemTxResult memory_region_dispatch_read(MemoryRegion
> *mr,
> * @mr: #MemoryRegion to access
> * @addr: address within that region
> * @data: data to write
> - * @op: size of the access in bytes
> + * @op: size, sign, and endianness of the memory operation
> * @attrs: memory transaction attributes to use for the access
> */
> MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
As I mentioned, now is when the actual interface change to these functions
should occur.
r~
Re: [Qemu-devel] [PATCH 2/2] ssh: implement private key authentication
On 7/26/19 9:09 AM, Pino Toscano wrote:
> Add a 'private-key' option which represents the path of a private key
> to use for authentication, and 'private-key-secret' as the name of an
> object with its passphrase.
>
> Signed-off-by: Pino Toscano
> +++ b/qapi/block-core.json
> @@ -3226,6 +3226,11 @@
> # @password-secret: ID of a QCryptoSecret object providing a password
> # for authentication (since 4.2)
> #
> +# @private-key: path to the private key (since 4.2)
> +#
> +# @private-key-secret: ID of a QCryptoSecret object providing the passphrase
> +# for 'private-key' (since 4.2)
Is password-secret intended to be mutually-exclusive with
private-key/private-key-secret? If so, this should probably utilize an
enum for a discriminator
{ 'enum': 'SshAuth', 'data': ['ssh-agent', 'password', 'private'key'] }
then update BlockdevOptionsSsh to be a union type with an optional
discriminator (defaulting to ssh-agent) for back-compat, where
'auth':'ssh-agent' needs no further fields, 'auth':'password' adds in a
'secret' field for use as password, or where 'auth':'private-key' adds
in both 'key-file' and 'secret' for use as the two pieces needed for
private key use.
Markus may have other suggestions on how best to represent this sort of
union type in QAPI.
> +#
> # Since: 2.9
> ##
> { 'struct': 'BlockdevOptionsSsh',
> @@ -3233,7 +3238,9 @@
> 'path': 'str',
> '*user': 'str',
> '*host-key-check': 'SshHostKeyCheck',
> -'*password-secret': 'str' } }
> +'*password-secret': 'str',
> +'*private-key': 'str',
> +'*private-key-secret': 'str' } }
>
>
> ##
>
On a different topic, how much of this work overlaps with the nbdkit ssh
plugin? Should we be duplicating efforts with both projects supporting
ssh natively, or is it worth considering getting qemu out of the ssh
business and instead connecting to an nbd device provided by nbdkit
connecting to ssh? (For comparison, we've already decided that nbdkit
does not plan on writing a qcow2 plugin, because it defers to qemu to be
the expert there; or in the other direction, qemu-nbd has deprecated its
partial support for exposing only a partition of a disk in favor of
qemu-nbd having much more partition support through its filters)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
On Fri, Jul 26, 2019 at 03:43:43PM +0200, Igor Mammedov wrote:
> On Wed, 24 Jul 2019 15:15:28 -0300
> Eduardo Habkost wrote:
>
> > On Wed, Jul 24, 2019 at 05:48:11PM +0200, Igor Mammedov wrote:
> > > On Wed, 24 Jul 2019 12:02:41 -0300
> > > Eduardo Habkost wrote:
> > >
> > > > On Wed, Jul 24, 2019 at 04:27:21PM +0200, Igor Mammedov wrote:
> > > > > On Tue, 23 Jul 2019 12:23:57 -0300
> > > > > Eduardo Habkost wrote:
> > > > >
> > > > > > On Tue, Jul 23, 2019 at 04:56:41PM +0200, Igor Mammedov wrote:
> > > > > > > On Tue, 16 Jul 2019 22:51:12 +0800
> > > > > > > Tao Xu wrote:
> > > > > > >
> > > > > > > > Add struct NumaState in MachineState and move existing numa
> > > > > > > > global
> > > > > > > > nb_numa_nodes(renamed as "num_nodes") into NumaState. And add
> > > > > > > > variable
> > > > > > > > numa_support into MachineClass to decide which submachines
> > > > > > > > support NUMA.
> > > > > > > >
> > > > > > > > Suggested-by: Igor Mammedov
> > > > > > > > Suggested-by: Eduardo Habkost
> > > > > > > > Signed-off-by: Tao Xu
> > > > > > > > ---
> > > > > > > >
> > > > > > > > No changes in v7.
> > > > > > > >
> > > > > > > > Changes in v6:
> > > > > > > > - Rebase to upstream, move globals in arm/sbsa-ref and use
> > > > > > > > numa_mem_supported
> > > > > > > > - When used once or twice in the function, use
> > > > > > > > ms->numa_state->num_nodes directly
> > > > > > > > - Correct some mistakes
> > > > > > > > - Use once monitor_printf in hmp_info_numa
> > > > > > > > ---
> > > > > > [...]
> > > > > > > > if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
> > > > > > > > -pxb->numa_node >= nb_numa_nodes) {
> > > > > > > > +pxb->numa_node >= ms->numa_state->num_nodes) {
> > > > > > > this will crash if user tries to use device on machine that
> > > > > > > doesn't support numa
> > > > > > > check that numa_state is not NULL before dereferencing
> > > > > >
> > > > > > That's exactly why the machine_num_numa_nodes() was created in
> > > > > > v5, but then you asked for its removal.
> > > > > V4 to more precise.
> > > > > I dislike small wrappers because they usually doesn't simplify code
> > > > > and make it more obscure,
> > > > > forcing to jump around to see what's really going on.
> > > > > Like it's implemented in this patch it's obvious what's wrong right
> > > > > away.
> > > > >
> > > > > In that particular case machine_num_numa_nodes() was also misused
> > > > > since only a handful
> > > > > of places (6) really need NULL check while majority (48) can directly
> > > > > access ms->numa_state->num_nodes.
> > > > > without NULL check.
> > > >
> > > > I strongly disagree, here. Avoiding a ms->numa_state==NULL check
> > > > is pointless optimization,
> > > I see it not as optimization (compiler probably would manage to optimize
> > > out most of them)
> > > but as rather properly self documented code. Doing check in places where
> > > it's
> > > not needed is confusing at best and can mask/introduce later subtle bugs
> > > at worst.
> > >
> > > > and leads to hard to spot bugs like
> > > > the one you saw above.
> > > That one was actually easy to spot because of the way it's written in
> > > this patch.
> >
> > When somebody is looking at a line of code containing
> > "ms->numa_state->num_nodes", how exactly are they supposed to
> > know if ms->numa_state is already guaranteed to be non-NULL, or
> > not?
> read the code/patch
> (at least I don't review just by looking at one line. And less time
> I have to spend, on reading extra code and finding answers why it's
> written the way it's, the better)
>
> In this patch code touching ms->numa_state, is divided in 2 categories
> generic code (memory API, CLI entry point, generic machine call
> site for numa specific code, devices, monitor/qmp) and numa aware code
> (huma parser and numa aware machines). The later one is majority of
> affected code where ms->numa_state != NULL.
>
> Even after I forget how this works and read code later, it would be
> easy to do educated guess/check where NULL check is not need seeing
> related code.
It's even easier to not have to check if/when numa_state can be
NULL because the code is safe on either case.
You don't review code by looking at a single line, but you don't
need to make it harder than it is.
> With machine_num_numa_nodes() would have to look for answer why we
> are doing it (unless we add a comment that check is there for noreason
> in most cases and it's exercise for reader to find out where
> it it's really need).
Sorry, your justification doesn't make sense to me. You don't
need to look for any answer at all, if the code makes it not
matter if numa_state is NULL. Having a single caller with
numa_state==NULL would be enough justification for the check.
>
> I don't see any justification for wrapper this case,
> could we stop bikeshedding and just let author to move on with fixing
Re: [Qemu-devel] [PATCH v5 09/15] cputlb: Access MemoryRegion with MemOp
On 7/25/19 11:46 PM, tony.ngu...@bt.com wrote:
> No-op MEMOP_SIZE and SIZE_MEMOP macros allows us to later easily
> convert memory_region_dispatch_{read|write} paramter "unsigned size"
> into a size+sign+endianness encoded "MemOp op".
>
> Being a no-op macro, this patch does not introduce any logical change.
>
> Signed-off-by: Tony Nguyen
> ---
> accel/tcg/cputlb.c | 21 ++---
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 523be4c..5d88cec 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -881,7 +881,7 @@ static void tlb_fill(CPUState *cpu, target_ulong addr,
> int size,
>
> static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
> int mmu_idx, target_ulong addr, uintptr_t retaddr,
> - MMUAccessType access_type, int size)
> + MMUAccessType access_type, MemOp op)
As I mentioned for patch 2, don't change this now, wait until after patch 10.
> -r = memory_region_dispatch_read(mr, mr_offset,
> -, size, iotlbentry->attrs);
> +r = memory_region_dispatch_read(mr, mr_offset, , op,
> iotlbentry->attrs);
So size_memop here,
> -cpu_transaction_failed(cpu, physaddr, addr, size, access_type,
> +cpu_transaction_failed(cpu, physaddr, addr, MEMOP_SIZE(op),
> access_type,
> mmu_idx, iotlbentry->attrs, r, retaddr);
but no memop_size here.
> static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>int mmu_idx, uint64_t val, target_ulong addr,
> - uintptr_t retaddr, int size)
> + uintptr_t retaddr, MemOp op)
Likewise.
> res = io_readx(env, _tlb(env)->d[mmu_idx].iotlb[index],
> - mmu_idx, addr, retaddr, access_type, size);
> + mmu_idx, addr, retaddr, access_type,
> SIZE_MEMOP(size));
And when you do come back to change the types after patch 10, at the top of the
function:
-unsigned a_bits = get_alignment_bits(get_memop(oi));
+MemOp op = get_memop(oi);
+unsigned a_bits = get_alignment_bits(op);
and then pass along op directly. Which will fix some of the weirdness in patch
11.
r~
[Qemu-devel] [PATCH 2/2] ssh: implement private key authentication
Add a 'private-key' option which represents the path of a private key
to use for authentication, and 'private-key-secret' as the name of an
object with its passphrase.
Signed-off-by: Pino Toscano
---
block/ssh.c | 98
block/trace-events | 1 +
docs/qemu-block-drivers.texi | 12 -
qapi/block-core.json | 9 +++-
4 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index 04ae223282..1b7c1f4108 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -500,6 +500,89 @@ static int check_host_key(BDRVSSHState *s, SshHostKeyCheck
*hkc, Error **errp)
return -EINVAL;
}
+static int authenticate_privkey(BDRVSSHState *s, BlockdevOptionsSsh *opts,
+Error **errp)
+{
+int err;
+int ret;
+char *pubkey_file = NULL;
+ssh_key public_key = NULL;
+ssh_key private_key = NULL;
+char *passphrase;
+
+pubkey_file = g_strdup_printf("%s.pub", opts->private_key);
+
+/* load the private key */
+trace_ssh_auth_key_passphrase(opts->private_key_secret, opts->private_key);
+passphrase = qcrypto_secret_lookup_as_utf8(opts->private_key_secret, errp);
+if (!passphrase) {
+err = SSH_AUTH_ERROR;
+goto error;
+}
+ret = ssh_pki_import_privkey_file(opts->private_key, passphrase,
+ NULL, NULL, _key);
+g_free(passphrase);
+if (ret == SSH_EOF) {
+error_setg(errp, "Cannot read private key '%s'", opts->private_key);
+err = SSH_AUTH_ERROR;
+goto error;
+} else if (ret == SSH_ERROR) {
+error_setg(errp,
+ "Cannot open private key '%s', maybe the passphrase is "
+ "wrong",
+ opts->private_key);
+err = SSH_AUTH_ERROR;
+goto error;
+}
+
+/* try to open the public part of the private key */
+ret = ssh_pki_import_pubkey_file(pubkey_file, _key);
+if (ret == SSH_ERROR) {
+error_setg(errp, "Cannot read public key '%s'", pubkey_file);
+err = SSH_AUTH_ERROR;
+goto error;
+} else if (ret == SSH_EOF) {
+/* create the public key from the private key */
+ret = ssh_pki_export_privkey_to_pubkey(private_key, _key);
+if (ret == SSH_ERROR) {
+error_setg(errp,
+ "Cannot export the public key from the private key "
+ "'%s'",
+ opts->private_key);
+err = SSH_AUTH_ERROR;
+goto error;
+}
+}
+
+ret = ssh_userauth_try_publickey(s->session, NULL, public_key);
+if (ret != SSH_AUTH_SUCCESS) {
+err = SSH_AUTH_DENIED;
+goto error;
+}
+
+ret = ssh_userauth_publickey(s->session, NULL, private_key);
+if (ret != SSH_AUTH_SUCCESS) {
+err = SSH_AUTH_DENIED;
+goto error;
+}
+
+ssh_key_free(private_key);
+ssh_key_free(public_key);
+g_free(pubkey_file);
+
+return SSH_AUTH_SUCCESS;
+
+ error:
+if (private_key) {
+ssh_key_free(private_key);
+}
+if (public_key) {
+ssh_key_free(public_key);
+}
+g_free(pubkey_file);
+return err;
+}
+
static int authenticate(BDRVSSHState *s, BlockdevOptionsSsh *opts,
Error **errp)
{
@@ -538,6 +621,21 @@ static int authenticate(BDRVSSHState *s,
BlockdevOptionsSsh *opts,
ret = 0;
goto out;
}
+
+/*
+ * Try to authenticate with private key, if available.
+ */
+if (opts->has_private_key && opts->has_private_key_secret) {
+r = authenticate_privkey(s, opts, errp);
+if (r == SSH_AUTH_ERROR) {
+ret = -EINVAL;
+goto out;
+} else if (r == SSH_AUTH_SUCCESS) {
+/* Authenticated! */
+ret = 0;
+goto out;
+}
+}
}
/*
diff --git a/block/trace-events b/block/trace-events
index 391aae03e6..ccb51b9992 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -187,6 +187,7 @@ ssh_seek(int64_t offset) "seeking to offset=%" PRIi64
ssh_auth_methods(int methods) "auth methods=0x%x"
ssh_server_status(int status) "server status=%d"
ssh_option_secret_object(const char *path) "using password from object %s"
+ssh_auth_key_passphrase(const char *path, const char *key) "using passphrase
from object %s for private key %s"
# curl.c
curl_timer_cb(long timeout_ms) "timer callback timeout_ms %ld"
diff --git a/docs/qemu-block-drivers.texi b/docs/qemu-block-drivers.texi
index c77ef2dd69..5513bf261c 100644
--- a/docs/qemu-block-drivers.texi
+++ b/docs/qemu-block-drivers.texi
@@ -774,8 +774,16 @@ tools only use MD5 to print fingerprints).
The optional @var{password-secret} parameter provides the ID of a
@code{secret} object that contains the password for authenticating.
-Currently authentication must be
[Qemu-devel] [PATCH 1/2] ssh: implement password authentication
Add a 'password-secret' option which represents the name of an object
with the password of the user.
Signed-off-by: Pino Toscano
---
block/ssh.c | 35 ---
block/trace-events | 1 +
docs/qemu-block-drivers.texi | 7 +--
qapi/block-core.json | 6 +-
tests/qemu-iotests/207.out | 2 +-
5 files changed, 44 insertions(+), 7 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index 501933b855..04ae223282 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -43,6 +43,7 @@
#include "qapi/qmp/qstring.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/qobject-output-visitor.h"
+#include "crypto/secret.h"
#include "trace.h"
/*
@@ -499,7 +500,8 @@ static int check_host_key(BDRVSSHState *s, SshHostKeyCheck
*hkc, Error **errp)
return -EINVAL;
}
-static int authenticate(BDRVSSHState *s, Error **errp)
+static int authenticate(BDRVSSHState *s, BlockdevOptionsSsh *opts,
+Error **errp)
{
int r, ret;
int method;
@@ -538,9 +540,35 @@ static int authenticate(BDRVSSHState *s, Error **errp)
}
}
+/*
+ * Try to authenticate with password, if available.
+ */
+if (method & SSH_AUTH_METHOD_PASSWORD && opts->has_password_secret) {
+char *password;
+
+trace_ssh_option_secret_object(opts->password_secret);
+password = qcrypto_secret_lookup_as_utf8(opts->password_secret, errp);
+if (!password) {
+ret = -EINVAL;
+goto out;
+}
+r = ssh_userauth_password(s->session, NULL, password);
+g_free(password);
+if (r == SSH_AUTH_ERROR) {
+ret = -EINVAL;
+session_error_setg(errp, s, "failed to authenticate using "
+"password authentication");
+goto out;
+} else if (r == SSH_AUTH_SUCCESS) {
+/* Authenticated! */
+ret = 0;
+goto out;
+}
+}
+
ret = -EPERM;
error_setg(errp, "failed to authenticate using publickey authentication "
- "and the identities held by your ssh-agent");
+ "and the identities held by your ssh-agent, or using password");
out:
return ret;
@@ -785,7 +813,7 @@ static int connect_to_ssh(BDRVSSHState *s,
BlockdevOptionsSsh *opts,
}
/* Authenticate. */
-ret = authenticate(s, errp);
+ret = authenticate(s, opts, errp);
if (ret < 0) {
goto err;
}
@@ -1376,6 +1404,7 @@ static const char *const ssh_strong_runtime_opts[] = {
"user",
"host_key_check",
"server.",
+"password-secret",
NULL
};
diff --git a/block/trace-events b/block/trace-events
index d724df0117..391aae03e6 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -186,6 +186,7 @@ ssh_write_return(ssize_t ret, int sftp_err) "sftp_write
returned %zd (sftp error
ssh_seek(int64_t offset) "seeking to offset=%" PRIi64
ssh_auth_methods(int methods) "auth methods=0x%x"
ssh_server_status(int status) "server status=%d"
+ssh_option_secret_object(const char *path) "using password from object %s"
# curl.c
curl_timer_cb(long timeout_ms) "timer callback timeout_ms %ld"
diff --git a/docs/qemu-block-drivers.texi b/docs/qemu-block-drivers.texi
index 91ab0eceae..c77ef2dd69 100644
--- a/docs/qemu-block-drivers.texi
+++ b/docs/qemu-block-drivers.texi
@@ -771,8 +771,11 @@ matches a specific fingerprint:
(@code{sha1:} can also be used as a prefix, but note that OpenSSH
tools only use MD5 to print fingerprints).
-Currently authentication must be done using ssh-agent. Other
-authentication methods may be supported in future.
+The optional @var{password-secret} parameter provides the ID of a
+@code{secret} object that contains the password for authenticating.
+
+Currently authentication must be done using ssh-agent, or providing a
+password. Other authentication methods may be supported in future.
Note: Many ssh servers do not support an @code{fsync}-style operation.
The ssh driver cannot guarantee that disk flush requests are
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0d43d4f37c..1244562c7b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3223,13 +3223,17 @@
# @host-key-check: Defines how and what to check the host key against
# (default: known_hosts)
#
+# @password-secret: ID of a QCryptoSecret object providing a password
+# for authentication (since 4.2)
+#
# Since: 2.9
##
{ 'struct': 'BlockdevOptionsSsh',
'data': { 'server': 'InetSocketAddress',
'path': 'str',
'*user': 'str',
-'*host-key-check': 'SshHostKeyCheck' } }
+'*host-key-check': 'SshHostKeyCheck',
+'*password-secret': 'str' } }
##
diff --git a/tests/qemu-iotests/207.out b/tests/qemu-iotests/207.out
index 1239d9d648..5bfdf626b9 100644
--- a/tests/qemu-iotests/207.out
+++
[Qemu-devel] [PATCH 0/2] ssh: add password and privkey auth methods
These two patches add the password and private key authentication methods to the ssh block driver, using secure objects for passwords/passphrases. Pino Toscano (2): ssh: implement password authentication ssh: implement private key authentication block/ssh.c | 133 ++- block/trace-events | 2 + docs/qemu-block-drivers.texi | 15 +++- qapi/block-core.json | 13 +++- tests/qemu-iotests/207.out | 2 +- 5 files changed, 158 insertions(+), 7 deletions(-) -- 2.21.0
Re: [Qemu-devel] [PATCH v5 02/15] memory: Access MemoryRegion with MemOp
On 7/26/19 6:36 AM, Richard Henderson wrote:
> On 7/25/19 11:43 PM, tony.ngu...@bt.com wrote:
>> } MemOp;
>>
>> +/* No-op while memory_region_dispatch_[read|write] is converted to MemOp */
>> +#define MEMOP_SIZE(op) (op)/* MemOp to size. */
>> +#define SIZE_MEMOP(ul) (ul)/* Size to MemOp. */
>> +
>
> This doesn't thrill me, because for 9 patches these macros don't do what they
> say on the tin, but I'll accept it.
>
> I would prefer lower-case and that the real implementation in patch 10 be
> inline functions with proper types instead of typeless macros. In particular,
> "unsigned" not "unsigned long" as you imply from "ul" here, since that's what
> was used ...
>
>> MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
>> hwaddr addr,
>> uint64_t *pval,
>> -unsigned size,
>> +MemOp op,
>> MemTxAttrs attrs);
Actually, I thought of something that would make me happier:
Do not make any change to memory_region_dispatch_{read,write} now. Let the
operand continue to be "unsigned size", because it still is, because of the
no-op macros.
Make the change to to the proper type at the same time that you flip the switch
to use the proper conversion function. This will make patch 10 about 5 lines
longer, but we'll have proper typing at all points in between.
r~
>
> ... here.
>
> With the name case change,
> Reviewed-by: Richard Henderson
>
>
> r~
>
Re: [Qemu-devel] [PATCH 03/28] qapi: Split error.json off common.json
On 7/26/19 7:05 AM, Markus Armbruster wrote:
> In my "build everything" tree, changing a type in qapi/common.json
> triggers a recompile of some 3600 out of 6600 objects (not counting
> tests and objects that don't depend on qemu/osdep.h).
>
> One common dependency is QapiErrorClass: it's used only in in
> qapi/error.h, which uses nothing else, and is widely included.
>
> Move QapiErrorClass from common.json to new error.json. Touching
> common.json now recompiles only some 2900 objects.
>
> Cc: Eric Blake
> Signed-off-by: Markus Armbruster
> ---
> MAINTAINERS | 2 ++
> include/qapi/error.h | 2 +-
> qapi/Makefile.objs| 2 +-
> qapi/common.json | 24
> qapi/error.json | 29 +
> qapi/qapi-schema.json | 1 +
> 6 files changed, 34 insertions(+), 26 deletions(-)
> create mode 100644 qapi/error.json
> +++ b/qapi/qapi-schema.json
> @@ -80,6 +80,7 @@
> # stable order, it's best to include each sub-schema just once, or
> # include it first right here.
>
> +{ 'include': 'error.json' }
> { 'include': 'common.json' }
> { 'include': 'sockets.json' }
> { 'include': 'run-state.json' }
Any reason why error.json is needed before common.json? But I don't see
it as being a problem, so
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 08/15] exec: Access MemoryRegion with MemOp
On 7/25/19 11:46 PM, tony.ngu...@bt.com wrote:
> No-op SIZE_MEMOP macro allows us to later easily convert
> memory_region_dispatch_{read|write} paramter "unsigned size" into a
> size+sign+endianness encoded "MemOp op".
>
> Being a no-op macro, this patch does not introduce any logical change.
>
> Signed-off-by: Tony Nguyen
> Reviewed-by: Philippe Mathieu-Daudé
> ---
> exec.c| 6 --
> memory_ldst.inc.c | 18 +-
> 2 files changed, 13 insertions(+), 11 deletions(-)
Reviewed-by: Richard Henderson
> /* I/O case */
> -r = memory_region_dispatch_read(mr, addr1, , 4, attrs);
> +r = memory_region_dispatch_read(mr, addr1, , SIZE_MEMOP(4),
> attrs);
MO_32, eventually, as well as
> -r = memory_region_dispatch_read(mr, addr1, , 8, attrs);
> +r = memory_region_dispatch_read(mr, addr1, , SIZE_MEMOP(8),
> attrs);
MO_64
> -r = memory_region_dispatch_read(mr, addr1, , 1, attrs);
> +r = memory_region_dispatch_read(mr, addr1, , SIZE_MEMOP(1),
> attrs);
MO_8
> -r = memory_region_dispatch_read(mr, addr1, , 2, attrs);
> +r = memory_region_dispatch_read(mr, addr1, , SIZE_MEMOP(2),
> attrs);
MO_16, and so on.
r~
Re: [Qemu-devel] [PATCH v5 03/15] target/mips: Access MemoryRegion with MemOp
On 7/25/19 11:44 PM, tony.ngu...@bt.com wrote: > memory_region_dispatch_read(env->itc_tag, index, >CP0_TagLo, > -8, MEMTXATTRS_UNSPECIFIED); > +SIZE_MEMOP(8), MEMTXATTRS_UNSPECIFIED); As an example of why I'm not especially keen on the temporary incorrect implementation of size_memop, you'll need a second pass through these files to replace this with the proper MO_64. That said, Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
On Wed, 24 Jul 2019 15:15:28 -0300
Eduardo Habkost wrote:
> On Wed, Jul 24, 2019 at 05:48:11PM +0200, Igor Mammedov wrote:
> > On Wed, 24 Jul 2019 12:02:41 -0300
> > Eduardo Habkost wrote:
> >
> > > On Wed, Jul 24, 2019 at 04:27:21PM +0200, Igor Mammedov wrote:
> > > > On Tue, 23 Jul 2019 12:23:57 -0300
> > > > Eduardo Habkost wrote:
> > > >
> > > > > On Tue, Jul 23, 2019 at 04:56:41PM +0200, Igor Mammedov wrote:
> > > > > > On Tue, 16 Jul 2019 22:51:12 +0800
> > > > > > Tao Xu wrote:
> > > > > >
> > > > > > > Add struct NumaState in MachineState and move existing numa global
> > > > > > > nb_numa_nodes(renamed as "num_nodes") into NumaState. And add
> > > > > > > variable
> > > > > > > numa_support into MachineClass to decide which submachines
> > > > > > > support NUMA.
> > > > > > >
> > > > > > > Suggested-by: Igor Mammedov
> > > > > > > Suggested-by: Eduardo Habkost
> > > > > > > Signed-off-by: Tao Xu
> > > > > > > ---
> > > > > > >
> > > > > > > No changes in v7.
> > > > > > >
> > > > > > > Changes in v6:
> > > > > > > - Rebase to upstream, move globals in arm/sbsa-ref and use
> > > > > > > numa_mem_supported
> > > > > > > - When used once or twice in the function, use
> > > > > > > ms->numa_state->num_nodes directly
> > > > > > > - Correct some mistakes
> > > > > > > - Use once monitor_printf in hmp_info_numa
> > > > > > > ---
> > > > > [...]
> > > > > > > if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
> > > > > > > -pxb->numa_node >= nb_numa_nodes) {
> > > > > > > +pxb->numa_node >= ms->numa_state->num_nodes) {
> > > > > > this will crash if user tries to use device on machine that doesn't
> > > > > > support numa
> > > > > > check that numa_state is not NULL before dereferencing
> > > > >
> > > > > That's exactly why the machine_num_numa_nodes() was created in
> > > > > v5, but then you asked for its removal.
> > > > V4 to more precise.
> > > > I dislike small wrappers because they usually doesn't simplify code and
> > > > make it more obscure,
> > > > forcing to jump around to see what's really going on.
> > > > Like it's implemented in this patch it's obvious what's wrong right
> > > > away.
> > > >
> > > > In that particular case machine_num_numa_nodes() was also misused since
> > > > only a handful
> > > > of places (6) really need NULL check while majority (48) can directly
> > > > access ms->numa_state->num_nodes.
> > > > without NULL check.
> > >
> > > I strongly disagree, here. Avoiding a ms->numa_state==NULL check
> > > is pointless optimization,
> > I see it not as optimization (compiler probably would manage to optimize
> > out most of them)
> > but as rather properly self documented code. Doing check in places where
> > it's
> > not needed is confusing at best and can mask/introduce later subtle bugs at
> > worst.
> >
> > > and leads to hard to spot bugs like
> > > the one you saw above.
> > That one was actually easy to spot because of the way it's written in this
> > patch.
>
> When somebody is looking at a line of code containing
> "ms->numa_state->num_nodes", how exactly are they supposed to
> know if ms->numa_state is already guaranteed to be non-NULL, or
> not?
read the code/patch
(at least I don't review just by looking at one line. And less time
I have to spend, on reading extra code and finding answers why it's
written the way it's, the better)
In this patch code touching ms->numa_state, is divided in 2 categories
generic code (memory API, CLI entry point, generic machine call
site for numa specific code, devices, monitor/qmp) and numa aware code
(huma parser and numa aware machines). The later one is majority of
affected code where ms->numa_state != NULL.
Even after I forget how this works and read code later, it would be
easy to do educated guess/check where NULL check is not need seeing
related code.
With machine_num_numa_nodes() would have to look for answer why we
are doing it (unless we add a comment that check is there for noreason
in most cases and it's exercise for reader to find out where
it it's really need).
I don't see any justification for wrapper this case,
could we stop bikeshedding and just let author to move on with fixing bugs, pls?
Re: [Qemu-devel] [PULL 00/22] virtio, pc, pci: features, fixes, cleanups
On Fri, Jul 26, 2019 at 01:39:26PM +0100, Peter Maydell wrote: > On Tue, 2 Jul 2019 at 19:27, Peter Maydell wrote: > > > > On Tue, 2 Jul 2019 at 19:22, Michael S. Tsirkin wrote: > > > > > > On Tue, Jul 02, 2019 at 06:20:01PM +0100, Peter Maydell wrote: > > > > On Tue, 2 Jul 2019 at 18:01, Michael S. Tsirkin wrote: > > > > > This isn't from mainline. We have a bit of a deadlock with linux merge > > > > > window opening soon. I think it's reasonable temporarily > > > > > and then before release either virtio-pmem gets there or I will > > > > > revert it and drop the header. > > > > > > > > It's definitely not ideal: until the headers are actually > > > > upstream there's no guarantee that they won't change ABI. > > > > > > But then I'm watching it, if I see that I'll drop the device from qemu for > > > now. > > > > OK; I guess we can take this for now if we make sure we revert > > if the headers aren't upstream by the time we get to say rc2 > > (23rd July). (That is, we'd want to do any revert shortly after > > rc2, since rc3 might be the last rc before release.) > > Ping for status update -- have the headers hit the mainline > kernel yet? Do they match the versions we put into QEMU? > > thanks > -- PMM Yes they do: here's the diff: diff --git a/include/standard-headers/linux/virtio_pmem.h b/include/standard-headers/linux/virtio_pmem.h index 7e3d43b121..c5f610ed6b 100644 --- a/include/standard-headers/linux/virtio_pmem.h +++ b/include/standard-headers/linux/virtio_pmem.h @@ -7,8 +7,8 @@ * Author(s): Pankaj Gupta */ -#ifndef _UAPI_LINUX_VIRTIO_PMEM_H -#define _UAPI_LINUX_VIRTIO_PMEM_H +#ifndef _LINUX_VIRTIO_PMEM_H +#define _LINUX_VIRTIO_PMEM_H #include "standard-headers/linux/types.h" #include "standard-headers/linux/virtio_ids.h" Didn't decide whether I want to update it now or after the pull, but it shouldn't matter. If you want to do an update now, go ahead. -- MST
Re: [Qemu-devel] [PATCH v5 05/15] hw/intc/armv7m_nic: Access MemoryRegion with MemOp
On 7/25/19 11:45 PM, tony.ngu...@bt.com wrote:
> No-op SIZE_MEMOP macro allows us to later easily convert
> memory_region_dispatch_{read|write} paramter "unsigned size" into a
> size+sign+endianness encoded "MemOp op".
>
> Being a no-op macro, this patch does not introduce any logical change.
>
> Signed-off-by: Tony Nguyen
> Reviewed-by: Philippe Mathieu-Daudé
> ---
> hw/intc/armv7m_nvic.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
Reviewed-by: Richard Henderson
r~
Re: [Qemu-devel] [PATCH v5 04/15] hw/s390x: Access MemoryRegion with MemOp
On 7/25/19 11:44 PM, tony.ngu...@bt.com wrote:
> No-op SIZE_MEMOP macro allows us to later easily convert
> memory_region_dispatch_{read|write} paramter "unsigned size" into a
> size+sign+endianness encoded "MemOp op".
>
> Being a no-op macro, this patch does not introduce any logical change.
>
> Signed-off-by: Tony Nguyen
> ---
> hw/s390x/s390-pci-inst.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Richard Henderson
> for (i = 0; i < len / 8; i++) {
> result = memory_region_dispatch_write(mr, offset + i * 8,
> - ldq_p(buffer + i * 8), 8,
> + ldq_p(buffer + i * 8),
> + SIZE_MEMOP(8),
MO_64, eventually.
r~
Re: [Qemu-devel] [PATCH v4 2/7] accel: collecting TB execution count
vandersonmr writes:
> If a TB has a TBS (TBStatistics) with the TB_EXEC_STATS
> enabled, then we instrument the start code of the TB
> to atomically count the number of times it is executed.
> The execution count of the TB is stored in its respective
> TBS.
>
> Signed-off-by: Vanderson M. do Rosario
> ---
> accel/tcg/tcg-runtime.c | 7 +++
> accel/tcg/tcg-runtime.h | 2 ++
> accel/tcg/translate-all.c | 8
> accel/tcg/translator.c| 1 +
> include/exec/gen-icount.h | 9 +
> include/exec/tb-stats.h | 11 +++
> include/qemu/log.h| 6 ++
> util/log.c| 11 +++
> 8 files changed, 55 insertions(+)
>
> diff --git a/accel/tcg/tcg-runtime.c b/accel/tcg/tcg-runtime.c
> index 8a1e408e31..f332eae334 100644
> --- a/accel/tcg/tcg-runtime.c
> +++ b/accel/tcg/tcg-runtime.c
> @@ -167,3 +167,10 @@ void HELPER(exit_atomic)(CPUArchState *env)
> {
> cpu_loop_exit_atomic(env_cpu(env), GETPC());
> }
> +
> +void HELPER(inc_exec_freq)(void *ptr)
> +{
> +TBStatistics *stats = (TBStatistics *) ptr;
> +g_assert(stats);
> +atomic_inc(>executions.total);
> +}
> diff --git a/accel/tcg/tcg-runtime.h b/accel/tcg/tcg-runtime.h
> index 4fa61b49b4..bf0b75dbe8 100644
> --- a/accel/tcg/tcg-runtime.h
> +++ b/accel/tcg/tcg-runtime.h
> @@ -28,6 +28,8 @@ DEF_HELPER_FLAGS_1(lookup_tb_ptr, TCG_CALL_NO_WG_SE, ptr,
> env)
>
> DEF_HELPER_FLAGS_1(exit_atomic, TCG_CALL_NO_WG, noreturn, env)
>
> +DEF_HELPER_FLAGS_1(inc_exec_freq, TCG_CALL_NO_RWG, void, ptr)
> +
> #ifdef CONFIG_SOFTMMU
>
> DEF_HELPER_FLAGS_5(atomic_cmpxchgb, TCG_CALL_NO_WG,
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index a574890a80..7497dae508 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1785,6 +1785,14 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> */
> if (tb_stats_collection_enabled()) {
> tb->tb_stats = tb_get_stats(phys_pc, pc, cs_base, flags);
> +uint32_t flag = get_default_tbstats_flag();
> +
> +if (qemu_log_in_addr_range(tb->pc)) {
> +if (flag & TB_EXEC_STATS) {
> +tb->tb_stats->stats_enabled |= TB_EXEC_STATS;
> +}
> +}
> +
> } else {
> tb->tb_stats = NULL;
> }
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index 9226a348a3..396a11e828 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -46,6 +46,7 @@ void translator_loop(const TranslatorOps *ops,
> DisasContextBase *db,
>
> ops->init_disas_context(db, cpu);
> tcg_debug_assert(db->is_jmp == DISAS_NEXT); /* no early exit */
> +gen_tb_exec_count(tb);
>
> /* Reset the temp count so that we can identify leaks */
> tcg_clear_temp_count();
> diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
> index f7669b6841..b3efe41894 100644
> --- a/include/exec/gen-icount.h
> +++ b/include/exec/gen-icount.h
> @@ -7,6 +7,15 @@
>
> static TCGOp *icount_start_insn;
>
> +static inline void gen_tb_exec_count(TranslationBlock *tb)
> +{
> +if (tb_stats_enabled(tb, TB_EXEC_STATS)) {
> +TCGv_ptr ptr = tcg_const_ptr(tb->tb_stats);
> +gen_helper_inc_exec_freq(ptr);
> +tcg_temp_free_ptr(ptr);
> +}
> +}
> +
> static inline void gen_tb_start(TranslationBlock *tb)
> {
> TCGv_i32 count, imm;
> diff --git a/include/exec/tb-stats.h b/include/exec/tb-stats.h
> index 0913155ec3..ee1e8de0d3 100644
> --- a/include/exec/tb-stats.h
> +++ b/include/exec/tb-stats.h
> @@ -6,6 +6,9 @@
> #include "exec/tb-context.h"
> #include "tcg.h"
>
> +#define tb_stats_enabled(tb, JIT_STATS) \
> +(tb && tb->tb_stats && (tb->tb_stats->stats_enabled & JIT_STATS))
> +
> typedef struct TBStatistics TBStatistics;
>
> /*
> @@ -22,6 +25,14 @@ struct TBStatistics {
> uint32_t flags;
> /* cs_base isn't included in the hash but we do check for matches */
> target_ulong cs_base;
> +
> +uint32_t stats_enabled;
> +
> +/* Execution stats */
> +struct {
> +unsigned long total;
> +unsigned long atomic;
We are not incrementing atomic in this patch. Also it's not total so
maybe "normal" makes more sense. Something like:
fixup! accel: collecting TB execution count
4 files changed, 11 insertions(+), 6 deletions(-)
accel/tcg/cpu-exec.c| 4
accel/tcg/tb-stats.c| 9 +
accel/tcg/tcg-runtime.c | 2 +-
include/exec/tb-stats.h | 2 +-
modified accel/tcg/cpu-exec.c
@@ -252,6 +252,10 @@ void cpu_exec_step_atomic(CPUState *cpu)
start_exclusive();
+if (tb_stats_enabled(tb, TB_EXEC_STATS)) {
+tb->tb_stats->executions.atomic++;
+}
+
/* Since we got here, we know that parallel_cpus must be true. */
parallel_cpus = false;
in_exclusive_region = true;
modified accel/tcg/tb-stats.c
@@ -233,11 +233,12 @@ static void dump_tb_header(TBStatistics *tbs)
float guest_host_prop = g ? ((float)
Re: [Qemu-devel] [PATCH v5 30/42] qemu-img: Use child access functions
25.07.2019 19:34, Max Reitz wrote:
> On 24.07.19 11:54, Vladimir Sementsov-Ogievskiy wrote:
>> 21.06.2019 16:15, Vladimir Sementsov-Ogievskiy wrote:
>>> 19.06.2019 18:49, Max Reitz wrote:
On 19.06.19 11:18, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> This changes iotest 204's output, because blkdebug on top of a COW node
>> used to make qemu-img map disregard the rest of the backing chain (the
>> backing chain was broken by the filter). With this patch, the
>> allocation in the base image is reported correctly.
>>
>> Signed-off-by: Max Reitz
>> ---
>> qemu-img.c | 36
>> tests/qemu-iotests/204.out | 1 +
>> 2 files changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 07b6e2a808..7bfa6e5d40 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -992,7 +992,7 @@ static int img_commit(int argc, char **argv)
>> if (!blk) {
>> return 1;
>> }
>> - bs = blk_bs(blk);
>> + bs = bdrv_skip_implicit_filters(blk_bs(blk));
>
> if filename is json, describing explicit filter over normal node, bs will
> be
> explicit filter ...
>
>> qemu_progress_init(progress, 1.f);
>> qemu_progress_print(0.f, 100);
>> @@ -1009,7 +1009,7 @@ static int img_commit(int argc, char **argv)
>> /* This is different from QMP, which by default uses the
>> deepest file in
>> * the backing chain (i.e., the very base); however, the
>> traditional
>> * behavior of qemu-img commit is using the immediate
>> backing file. */
>> - base_bs = backing_bs(bs);
>> + base_bs = bdrv_filtered_cow_bs(bs);
>> if (!base_bs) {
>
> and here we'll fail.
Right, will change to bdrv_backing_chain_next().
>> error_setg(_err, "Image does not have a backing
>> file");
>> goto done;
>> @@ -1626,19 +1626,18 @@ static int
>> convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>> if (s->sector_next_status <= sector_num) {
>> int64_t count = n * BDRV_SECTOR_SIZE;
>> + BlockDriverState *src_bs = blk_bs(s->src[src_cur]);
>> + BlockDriverState *base;
>> if (s->target_has_backing) {
>> -
>> - ret = bdrv_block_status(blk_bs(s->src[src_cur]),
>> - (sector_num - src_cur_offset) *
>> - BDRV_SECTOR_SIZE,
>> - count, , NULL, NULL);
>> + base = bdrv_backing_chain_next(src_bs);
>
> As you described in another patch, will not we here get allocated in base
> as allocated, because of
> counting filters above base?
Damn, yes. So
base = bdrv_filtered_cow_bs(bdrv_skip_rw_filters(src_bs));
I suppose.
> Hmm. I now think, why filters don't report everything as unallocated,
> would not it be more correct
> than fallthrough to child?
I don’t know, actually. Maybe, maybe not.
>> } else {
>> - ret = bdrv_block_status_above(blk_bs(s->src[src_cur]), NULL,
>> - (sector_num - src_cur_offset)
>> *
>> - BDRV_SECTOR_SIZE,
>> - count, , NULL, NULL);
>> + base = NULL;
>> }
>> + ret = bdrv_block_status_above(src_bs, base,
>> + (sector_num - src_cur_offset) *
>> + BDRV_SECTOR_SIZE,
>> + count, , NULL, NULL);
>> if (ret < 0) {
>> error_report("error while reading block status of sector
>> %" PRId64
>> ": %s", sector_num, strerror(-ret));
[...]
>> @@ -2949,7 +2950,7 @@ static int img_map(int argc, char **argv)
>> if (!blk) {
>> return 1;
>> }
>> - bs = blk_bs(blk);
>> + bs = bdrv_skip_implicit_filters(blk_bs(blk));
>
> Hmm, another thought about implicit filters, how they could be here in
> qemu-img?
Hm, I don’t think they can.
> If implicit are only
> job filters. Do you prepared it for implicit COR? But we discussed with
> Kevin that we'd better deprecate
> copy-on-read option..
>
> So, if implicit filters are for compatibility, we'll have them only in
> block-jobs. So, seems no reason to support
> them in qemu-img.
Seems reasonable, yes.
> Also, in block-jobs, we
Re: [Qemu-devel] [PATCH v5 07/15] hw/vfio: Access MemoryRegion with MemOp
On 7/25/19 11:46 PM, tony.ngu...@bt.com wrote:
> No-op SIZE_MEMOP macro allows us to later easily convert
> memory_region_dispatch_{read|write} paramter "unsigned size" into a
> size+sign+endianness encoded "MemOp op".
>
> Being a no-op macro, this patch does not introduce any logical change.
>
> Signed-off-by: Tony Nguyen
> ---
> hw/vfio/pci-quirks.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Richard Henderson
r~
Re: [Qemu-devel] [PATCH v5 06/15] hw/virtio: Access MemoryRegion with MemOp
On 7/25/19 11:45 PM, tony.ngu...@bt.com wrote:
> No-op SIZE_MEMOP macro allows us to later easily convert
> memory_region_dispatch_{read|write} paramter "unsigned size" into a
> size+sign+endianness encoded "MemOp op".
>
> Being a no-op macro, this patch does not introduce any logical change.
>
> Signed-off-by: Tony Nguyen
> ---
> hw/virtio/virtio-pci.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
Reviewed-by: Richard Henderson
r~
Re: [Qemu-devel] [PATCH v5 02/15] memory: Access MemoryRegion with MemOp
On 7/25/19 11:43 PM, tony.ngu...@bt.com wrote: > } MemOp; > > +/* No-op while memory_region_dispatch_[read|write] is converted to MemOp */ > +#define MEMOP_SIZE(op) (op)/* MemOp to size. */ > +#define SIZE_MEMOP(ul) (ul)/* Size to MemOp. */ > + This doesn't thrill me, because for 9 patches these macros don't do what they say on the tin, but I'll accept it. I would prefer lower-case and that the real implementation in patch 10 be inline functions with proper types instead of typeless macros. In particular, "unsigned" not "unsigned long" as you imply from "ul" here, since that's what was used ... > MemTxResult memory_region_dispatch_read(MemoryRegion *mr, > hwaddr addr, > uint64_t *pval, > -unsigned size, > +MemOp op, > MemTxAttrs attrs); ... here. With the name case change, Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH v5 01/15] tcg: TCGMemOp is now accelerator independent MemOp
On 7/25/19 11:43 PM, tony.ngu...@bt.com wrote: > +#ifdef NEED_CPU_H > +#ifdef ALIGNED_ONLY > +MO_ALIGN = 0, > +MO_UNALN = MO_AMASK, You need the configure patch got TARGET_ALIGNED_ONLY that you posted separately as patch 1 in order for this to work. Otherwise, Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [RFC 00/19] Add virtual device fuzzing support
On Thu, Jul 25, 2019 at 03:23:43AM +, Oleinik, Alexander wrote: > As part of Google Summer of Code 2019, I'm working on integrating > fuzzing of virtual devices into QEMU [1]. This is a highly WIP patchset > adding this functionality. > > Fuzzers provide random data to a program and monitor its execution for > errors. Coverage-guided fuzzers also observe the parts of the program > that are exercised by each input, and use this information to > mutate/guide the inputs to reach additional parts of the program. They > are quite effective for finding bugs in a wide range of software. Good start! The overall approach is maintainable and not too invasive. Some iteration on the current patch series will be necessary to clean things up, but the fundamentals look promising to me. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [RFC 19/19] fuzz: Add documentation about the fuzzer to docs/
On Thu, Jul 25, 2019 at 03:24:00AM +, Oleinik, Alexander wrote: > +== Main Modifications required for Fuzzing == > + > +Fuzzing is enabled with the -enable-fuzzing flag, which adds the needed > cflags > +to enable Libfuzzer and AddressSanitizer. In the code, most of the changes to > +existing qemu source are surrounded by #ifdef CONFIG_FUZZ statements. Here > are > +the key areas that are changed: > + > +=== General Changes === The audience of this file are people wishing to run existing fuzz tests and/or add new fuzz tests. Changes are of limited use to someone who wants to write fuzz tests but isn't familiar with QEMU internals. Instead I suggest documenting fuzzing in terms of: 1. How to run existing fuzz tests. 2. How to add new fuzz tests. 3. Advice on achieving good code coverage and explanation of the fuzz test development cycle. Focus less on the fuzz infrastructure internals and more on how to use fuzzing. signature.asc Description: PGP signature
Re: [Qemu-devel] [RFC 13/19] fuzz: add ctrl vq support to virtio-net in libqos
On Thu, Jul 25, 2019 at 05:05:25PM +, Oleinik, Alexander wrote:
> On Thu, 2019-07-25 at 12:25 -0400, John Snow wrote:
> >
> > On 7/24/19 11:23 PM, Oleinik, Alexander wrote:
> > > Signed-off-by: Alexander Oleinik
> >
> > Is there some explanation for why the below patch does what the
> > subject
> > line claims for the uninitiated?
> When multiqueue mode (VIRTIO_NET_F_MQ) is disabled, virtio-net sets up
> three queues. 0:receiveq, 1:transmitq and 2:controlq.
> > I don't know why increasing the number of queues from 2 to 3 here is
> > correct in the general case, OR why it would "add ctrl vq support".
> > (Or what it has to do with fuzzing, in general.)
>
> Prior to the change, accessing the ctrl vq through QOS, would trigger a
> segfault, since only two queues were allocated to QVirtioDevice*
> interface->queues.
>
> Also, when VIRTIO_NET_F_MQ is enabled, the number of queues is 2*N + 1,
> so I think in that case n->n_queues is also short by one in the code
> below.
I think the patch could be changed to:
> > [Only responding because this landed in tests/libqos, which I do try
> > to
> > keep an eye on, but this patch is opaque to me. --js]
> >
> > > ---
> > > tests/libqos/virtio-net.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tests/libqos/virtio-net.c b/tests/libqos/virtio-net.c
> > > index 66405b646e..247a0a17a8 100644
> > > --- a/tests/libqos/virtio-net.c
> > > +++ b/tests/libqos/virtio-net.c
> > > @@ -51,7 +51,7 @@ static void virtio_net_setup(QVirtioNet
> > > *interface)
> > > if (features & (1u << VIRTIO_NET_F_MQ)) {
> > > interface->n_queues = qvirtio_config_readw(vdev, 8) * 2;
> > > } else {
> > > -interface->n_queues = 2;
> > > +interface->n_queues = 3;
> > > }
interface->n_queues++; /* ctrl vq */
And a comment added to the QVirtQueue::n_queues field definition:
/* total number of virtqueues (rx, tx, ctrl) */
This will prevent confusion about whether the ctrl queue is counted or
not.
signature.asc
Description: PGP signature
Re: [Qemu-devel] [RFC 09/19] fuzz: use mtree_info to find mapped addresses
On Thu, Jul 25, 2019 at 03:23:51AM +, Oleinik, Alexander wrote:
> Locate mmio and port i/o addresses that are mapped to devices so we can
> limit the fuzzer to only these addresses. This should be replaced with
> a sane way of enumaring these memory regions.
>
> Signed-off-by: Alexander Oleinik
> ---
> memory.c | 34 ++
> 1 file changed, 34 insertions(+)
>
> diff --git a/memory.c b/memory.c
> index 5d8c9a9234..fa6cbe4f1d 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -34,6 +34,11 @@
> #include "hw/qdev-properties.h"
> #include "hw/boards.h"
> #include "migration/vmstate.h"
> +#ifdef CONFIG_FUZZ
> +#include "tests/fuzz/fuzz.h"
> +#include "tests/fuzz/qos_fuzz.h"
> +#endif
> +
>
> //#define DEBUG_UNASSIGNED
>
> @@ -3016,12 +3021,20 @@ static void mtree_print_flatview(gpointer key,
> gpointer value,
> int n = view->nr;
> int i;
> AddressSpace *as;
> +#ifdef CONFIG_FUZZ
> +bool io=false;
> +#endif
> +
>
> qemu_printf("FlatView #%d\n", fvi->counter);
> ++fvi->counter;
>
> for (i = 0; i < fv_address_spaces->len; ++i) {
> as = g_array_index(fv_address_spaces, AddressSpace*, i);
> +#ifdef CONFIG_FUZZ
> +if(strcmp("I/O",as->name) == 0)
> +io = true;
> +#endif
> qemu_printf(" AS \"%s\", root: %s",
> as->name, memory_region_name(as->root));
> if (as->root->alias) {
> @@ -3062,6 +3075,27 @@ static void mtree_print_flatview(gpointer key,
> gpointer value,
> range->readonly ? "rom" : memory_region_type(mr),
> memory_region_name(mr));
> }
> +#ifdef CONFIG_FUZZ
> +if(strcmp("i/o", memory_region_type(mr))==0 && strcmp("io",
> memory_region_name(mr))){
> +fuzz_memory_region *fmr = g_new0(fuzz_memory_region, 1);
> +if(!fuzz_memory_region_head)
> +{
> +fuzz_memory_region_head = fmr;
> +fuzz_memory_region_tail = fmr;
> +}
> +fmr->io = io;
> +fmr->start = int128_get64(range->addr.start);
> +fmr->length = MR_SIZE(range->addr.size);
> +fmr->next = fuzz_memory_region_head;
> +fuzz_memory_region_tail->next = fmr;
> +fuzz_memory_region_tail = fmr;
> +if(io == true){
> +total_io_mem += MR_SIZE(range->addr.size)+1;
> +} else {
> +total_ram_mem += MR_SIZE(range->addr.size)+1;
> +}
> +}
> +#endif
Why is this patch modifying a print function? I think the goal is to
build the fuzz_memory_region list and calculate
total_io_mem/total_ram_mem. This should be done by a separate function.
Can you use memory_region_is_ram() instead of the string compares?
Stefan
signature.asc
Description: PGP signature
Re: [Qemu-devel] [RFC 08/19] fuzz: add shims to intercept libfuzzer init
On Thu, Jul 25, 2019 at 10:21:18AM +0200, Paolo Bonzini wrote:
> On 25/07/19 05:23, Oleinik, Alexander wrote:
> > Intercept coverage buffer registration calls and use this information to
> > copy them to shared memory, if using fork() to avoid resetting device
> > state.
> >
> > Signed-off-by: Alexander Oleinik
> > ---
> > tests/fuzz/fuzzer_hooks.c | 106 ++
> > tests/fuzz/fuzzer_hooks.h | 9
> > 2 files changed, 115 insertions(+)
> > create mode 100644 tests/fuzz/fuzzer_hooks.c
> > create mode 100644 tests/fuzz/fuzzer_hooks.h
> >
> > diff --git a/tests/fuzz/fuzzer_hooks.c b/tests/fuzz/fuzzer_hooks.c
> > new file mode 100644
> > index 00..5a0bbec413
> > --- /dev/null
> > +++ b/tests/fuzz/fuzzer_hooks.c
> > @@ -0,0 +1,106 @@
> > +#include "qemu/osdep.h"
> > +#include "qemu/units.h"
> > +#include "qapi/error.h"
> > +#include "qemu-common.h"
> > +#include "fuzzer_hooks.h"
> > +
> > +#include
> > +#include
> > +
> > +
> > +extern void* _ZN6fuzzer3TPCE;
>
> Would it make sense to make this a C++ source, so that you can avoid
> using the mangled names (in this case, "namespace fuzzer { extern void
> *TPC; }" and then using fuzzer::TPC)? Even if it's just a single symbol.
A proper libfuzzer API is nicest in the long term.
Alexander: Could you send a patch to libfuzzer to see if they are
willing to support this via their API?
Stefan
signature.asc
Description: PGP signature
Re: [Qemu-devel] [RFC 07/19] fuzz: Modify libqtest to directly invoke qtest.c
On Thu, Jul 25, 2019 at 03:23:49AM +, Oleinik, Alexander wrote:
> @@ -379,9 +399,18 @@ static void socket_sendf(int fd, const char *fmt,
> va_list ap)
> {
> gchar *str = g_strdup_vprintf(fmt, ap);
> size_t size = strlen(str);
> +#ifdef CONFIG_FUZZ
> +// Directly call qtest_process_inbuf in the qtest server
> +GString *gstr = g_string_new_len(str, size);
> + /* printf(">>> %s",gstr->str); */
> +qtest_server_recv(gstr);
> +g_string_free(gstr, true);
> +g_free(str);
> +#else
>
> socket_send(fd, str, size);
> g_free(str);
> +#endif
> }
This should use indirection: a function pointer to dispatch to either
the socket or the internal qtest_process_inbuf() call.
With a bit of refactoring you can eliminate the #ifdefs and treat the
socket fd as one backend and direct invocation as another backend.
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PULL v1 0/2] Merge tpm 2019/07/25 v1
On Thu, 25 Jul 2019 at 16:48, Stefan Berger wrote: > > This series of patches improves error handling with the TPM backend. > >Stefan > > The following changes since commit 9d2e1fcd14c2bae5be1992214a03c0ddff714c80: > > Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into > staging (2019-07-22 13:20:49 +0100) > > are available in the Git repository at: > > git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2019-07-25-1 > > for you to fetch changes up to 7e095e84ba0b7c0a1ac45bc6824dace2fd352e56: > > tpm_emulator: Translate TPM error codes to strings (2019-07-25 11:37:10 > -0400) > > > Stefan Berger (2): > tpm: Exit in reset when backend indicates failure > tpm_emulator: Translate TPM error codes to strings > > hw/tpm/tpm_crb.c | 4 +++- > hw/tpm/tpm_emulator.c | 60 > ++-- > hw/tpm/tpm_int.h | 13 + > hw/tpm/tpm_tis.c | 4 +++- > 4 files changed, 69 insertions(+), 12 deletions(-) Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/4.1 for any user-visible changes. -- PMM
Re: [Qemu-devel] [PATCH v4 0/7] Measure Tiny Code Generation Quality
vandersonmr writes: > This patch is part of Google Summer of Code (GSoC) 2019. > More about the project can be found in: > https://wiki.qemu.org/Internships/ProjectIdeas/TCGCodeQuality > > The goal of this patch is to add infrastructure to collect > execution and JIT statistics during the emulation with accel/TCG. > The statistics are stored in TBStatistic structures (TBStats) > with each TB having its respective TBStats. > > We added -d tb_stats and HMP tb_stats commands to allow the control > of this statistics collection. And info tb, tbs, and coverset commands > were also added to allow dumping and exploring all this information > while emulating. Hmm with: -d tb_stats:10:all I'm seeing the following bt: Thread 4 "qemu-system-aar" received signal SIGABRT, Aborted. [Switching to Thread 0x7fffe5285700 (LWP 898)] __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. (gdb) bt #0 0x751787bb in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x75163535 in __GI_abort () at abort.c:79 #2 0x55bab663 in OP_E (bytemode=2, sizeflag=3) at /home/alex/lsrc/qemu.git/disas/i386.c:5094 #3 0x55ba972f in print_insn (pc=140735911299712, info=0x7fffe5281c00) at /home/alex/lsrc/qemu.git/disas/i386.c:4071 #4 0x55ba8623 in print_insn_i386 (pc=140735911299712, info=0x7fffe5281c00) at /home/alex/lsrc/qemu.git/disas/i386.c:3705 #5 0x55881241 in get_num_insts (code=0x7fffa2000a80 , size=88) at /home/alex/lsrc/qemu.git/disas.c:575 #6 0x558d89f1 in tb_gen_code (cpu=0x56a1f6a0, pc=18446743524230025728, cs_base=0, flags=2415924229, cflags=-16252928) at /home/alex/lsrc/qemu.git/accel/tcg/translate-all.c:1881 #7 0x558d4984 in tb_find (cpu=0x56a1f6a0, last_tb=0x0, tb_exit=0, cf_mask=524288) at /home/alex/lsrc/qemu.git/accel/tcg/cpu-exec.c:409 #8 0x558d524f in cpu_exec (cpu=0x56a1f6a0) at /home/alex/lsrc/qemu.git/accel/tcg/cpu-exec.c:731 #9 0x55884806 in tcg_cpu_exec (cpu=0x56a1f6a0) at /home/alex/lsrc/qemu.git/cpus.c:1435 #10 0x5588505c in qemu_tcg_cpu_thread_fn (arg=0x56a1f6a0) at /home/alex/lsrc/qemu.git/cpus.c:1743 #11 0x56010875 in qemu_thread_start (args=0x56a75450) at /home/alex/lsrc/qemu.git/util/qemu-thread-posix.c:502 #12 0x7530bfa3 in start_thread (arg=) at pthread_create.c:486 #13 0x7523a4cf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 > > Collecting these statistics and information is useful to understand > qemu performance and to help to add the support for traces to QEMU. > > vandersonmr (7): > accel: introducing TBStatistics structure > accel: collecting TB execution count > accel: collecting JIT statistics > accel: replacing part of CONFIG_PROFILER with TBStats > log: adding -d tb_stats to control tbstats > monitor: adding tb_stats hmp command > monitor: adding info tbs, tb, and coverset > > accel/tcg/Makefile.objs | 2 +- > accel/tcg/tb-stats.c | 489 +++ > accel/tcg/tcg-runtime.c | 7 + > accel/tcg/tcg-runtime.h | 2 + > accel/tcg/translate-all.c| 93 ++- > accel/tcg/translator.c | 6 + > disas.c | 108 > hmp-commands-info.hx | 23 ++ > hmp-commands.hx | 17 ++ > include/disas/disas.h| 1 + > include/exec/exec-all.h | 15 +- > include/exec/gen-icount.h| 9 + > include/exec/tb-context.h| 12 + > include/exec/tb-hash.h | 7 + > include/exec/tb-stats.h | 113 > include/qemu/log-for-trace.h | 2 + > include/qemu/log.h | 16 ++ > linux-user/exit.c| 4 + > monitor/misc.c | 111 > tcg/tcg.c| 114 +++- > tcg/tcg.h| 12 +- > util/log.c | 99 ++- > 22 files changed, 1144 insertions(+), 118 deletions(-) > create mode 100644 accel/tcg/tb-stats.c > create mode 100644 include/exec/tb-stats.h -- Alex Bennée
Re: [Qemu-devel] [RFC 07/19] fuzz: Modify libqtest to directly invoke qtest.c
On Thu, Jul 25, 2019 at 11:04:11AM +0200, Thomas Huth wrote:
> On 25/07/2019 05.23, Oleinik, Alexander wrote:
> > @@ -379,9 +399,18 @@ static void socket_sendf(int fd, const char *fmt,
> > va_list ap)
> > {
> > gchar *str = g_strdup_vprintf(fmt, ap);
> > size_t size = strlen(str);
> > +#ifdef CONFIG_FUZZ
> > +// Directly call qtest_process_inbuf in the qtest server
> > +GString *gstr = g_string_new_len(str, size);
> > + /* printf(">>> %s",gstr->str); */
>
> Please check your patches with scripts/checkpatch.pl - e.g. don't use
> TABs for indentation like in the above line, don't use //-comments, etc.
You can set up a git-hook with checkpatch.pl to scan code automatically
before each commit:
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html
Stefan
signature.asc
Description: PGP signature
Re: [Qemu-devel] [RFC 06/19] fuzz: Add ramfile for fast vmstate/vmload
On Thu, Jul 25, 2019 at 03:23:49AM +, Oleinik, Alexander wrote:
> The ramfile allows vmstate to be saved and restored directly onto the
> heap.
>
> Signed-off-by: Alexander Oleinik
> ---
> tests/fuzz/ramfile.c | 127 +++
> tests/fuzz/ramfile.h | 20 +++
> 2 files changed, 147 insertions(+)
> create mode 100644 tests/fuzz/ramfile.c
> create mode 100644 tests/fuzz/ramfile.h
>
> diff --git a/tests/fuzz/ramfile.c b/tests/fuzz/ramfile.c
> new file mode 100644
> index 00..8da242e9ee
> --- /dev/null
> +++ b/tests/fuzz/ramfile.c
Please put this in migration/. This code doesn't do fuzzing and is
general-purpose enough to be used by other parts of QEMU dealing with
live migration.
> @@ -0,0 +1,127 @@
> +/*
> + *
> =
> + *
> + * Filename: ramfile.c
> + *
> + *Description: QEMUFile stored in dynamically allocated RAM for fast
> VMRestore
> + *
> + * Author: Alexander Oleinik (), alx...@bu.edu
> + * Organization:
> + *
> + *
> =
> + */
Please use license headers with all new files that are created.
Fine-grained filename and authorship information is already kept by git
so it's not necessary to duplicate it here.
> +#include
> +#include "qemu/osdep.h"
osdep.h already includes stdlib.h.
> +#include "qemu-common.h"
> +#include "exec/memory.h"
> +#include "migration/qemu-file.h"
> +#include "migration/migration.h"
> +#include "migration/savevm.h"
> +#include "ramfile.h"
> +
> +#define INCREMENT 10240
> +#define IO_BUF_SIZE 32768
> +#define MAX_IOV_SIZE MIN(IOV_MAX, 64)
> +
> +struct QEMUFile {
> +const QEMUFileOps *ops;
> +const QEMUFileHooks *hooks;
> +void *opaque;
> +
> +int64_t bytes_xfer;
> +int64_t xfer_limit;
> +
> +int64_t pos; /* start of buffer when writing, end of buffer
> +when reading */
> +int buf_index;
> +int buf_size; /* 0 when writing */
> +uint8_t buf[IO_BUF_SIZE];
> +
> +DECLARE_BITMAP(may_free, MAX_IOV_SIZE);
> +struct iovec iov[MAX_IOV_SIZE];
> +unsigned int iovcnt;
> +
> +int last_error;
> +};
Wait, what?! :)
Please add the ram file to qemu-file.c instead of duplicating QEMUFile.
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v3] block/rbd: add preallocation support
On Fri, Jul 26, 2019 at 4:48 AM Stefano Garzarella wrote:
>
> On Thu, Jul 25, 2019 at 09:30:30AM -0400, Jason Dillaman wrote:
> > On Thu, Jul 25, 2019 at 4:13 AM Stefano Garzarella
> > wrote:
> > >
> > > On Wed, Jul 24, 2019 at 01:48:42PM -0400, Jason Dillaman wrote:
> > > > On Tue, Jul 23, 2019 at 3:13 AM Stefano Garzarella
> > > > wrote:
> > > > >
> > > > > This patch adds the support of preallocation (off/full) for the RBD
> > > > > block driver.
> > > > > If rbd_writesame() is available and supports zeroed buffers, we use
> > > > > it to quickly fill the image when full preallocation is required.
> > > > >
> > > > > Signed-off-by: Stefano Garzarella
> > > > > ---
> > > > > v3:
> > > > > - rebased on master
> > > > > - filled with zeroed buffer [Max]
> > > > > - used rbd_writesame() only when we can disable the discard of zeroed
> > > > >buffers
> > > > > - added 'since: 4.2' in qapi/block-core.json [Max]
> > > > > - used buffer as large as the "stripe unit"
> > > > > ---
> > > > > block/rbd.c | 202
> > > > > ---
> > > > > qapi/block-core.json | 5 +-
> > > > > 2 files changed, 192 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/block/rbd.c b/block/rbd.c
> > > > > index 59757b3120..d923a5a26c 100644
> > > > > --- a/block/rbd.c
> > > > > +++ b/block/rbd.c
> > > > > @@ -64,6 +64,7 @@
> > > > > #define OBJ_MAX_SIZE (1UL << OBJ_DEFAULT_OBJ_ORDER)
> > > > >
> > > > > #define RBD_MAX_SNAPS 100
> > > > > +#define RBD_DEFAULT_CONCURRENT_OPS 10
> > > > >
> > > > > /* The LIBRBD_SUPPORTS_IOVEC is defined in librbd.h */
> > > > > #ifdef LIBRBD_SUPPORTS_IOVEC
> > > > > @@ -104,6 +105,7 @@ typedef struct BDRVRBDState {
> > > > > char *image_name;
> > > > > char *snap;
> > > > > uint64_t image_size;
> > > > > +bool ws_zero_supported; /* rbd_writesame() supports zeroed
> > > > > buffers */
> > > > > } BDRVRBDState;
> > > > >
> > > > > static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
> > > > > @@ -333,6 +335,155 @@ static void qemu_rbd_memset(RADOSCB *rcb,
> > > > > int64_t offs)
> > > > > }
> > > > > }
> > > > >
> > > > > +static int qemu_rbd_get_max_concurrent_ops(rados_t cluster)
> > > > > +{
> > > > > +char buf[16];
> > > > > +int ret, max_concurrent_ops;
> > > > > +
> > > > > +ret = rados_conf_get(cluster, "rbd_concurrent_management_ops",
> > > > > buf,
> > > > > + sizeof(buf));
> > > > > +if (ret < 0) {
> > > > > +return RBD_DEFAULT_CONCURRENT_OPS;
> > > > > +}
> > > > > +
> > > > > +ret = qemu_strtoi(buf, NULL, 10, _concurrent_ops);
> > > > > +if (ret < 0) {
> > > > > +return RBD_DEFAULT_CONCURRENT_OPS;
> > > > > +}
> > > > > +
> > > > > +return max_concurrent_ops;
> > > > > +}
> > > > > +
> > > > > +static int qemu_rbd_do_truncate(rados_t cluster, rbd_image_t image,
> > > > > +int64_t offset, PreallocMode
> > > > > prealloc,
> > > > > +bool ws_zero_supported, Error **errp)
> > > > > +{
> > > > > +uint64_t current_length;
> > > > > +char *buf = NULL;
> > > > > +int ret;
> > > > > +
> > > > > +ret = rbd_get_size(image, _length);
> > > > > +if (ret < 0) {
> > > > > +error_setg_errno(errp, -ret, "Failed to get file length");
> > > > > +goto out;
> > > > > +}
> > > > > +
> > > > > +if (current_length > offset && prealloc != PREALLOC_MODE_OFF) {
> > > > > +error_setg(errp, "Cannot use preallocation for shrinking
> > > > > files");
> > > > > +ret = -ENOTSUP;
> > > > > +goto out;
> > > > > +}
> > > > > +
> > > > > +switch (prealloc) {
> > > > > +case PREALLOC_MODE_FULL: {
> > > > > +uint64_t buf_size, current_offset = current_length;
> > > > > +ssize_t bytes;
> > > > > +
> > > > > +ret = rbd_get_stripe_unit(image, _size);
> > > > > +if (ret < 0) {
> > > > > +error_setg_errno(errp, -ret, "Failed to get stripe
> > > > > unit");
> > > > > +goto out;
> > > > > +}
> > > > > +
> > > > > +ret = rbd_resize(image, offset);
> > > > > +if (ret < 0) {
> > > > > +error_setg_errno(errp, -ret, "Failed to resize file");
> > > > > +goto out;
> > > > > +}
> > > > > +
> > > > > +buf = g_malloc0(buf_size);
> > > > > +
> > > > > +#ifdef LIBRBD_SUPPORTS_WRITESAME
> > > > > +if (ws_zero_supported) {
> > > > > +uint64_t writesame_max_size;
> > > > > +int max_concurrent_ops;
> > > > > +
> > > > > +max_concurrent_ops =
> > > > > qemu_rbd_get_max_concurrent_ops(cluster);
> > > > > +/*
> > > > > + * We limit the rbd_writesame() size to avoid to spawn
> > > > > more then
> > > > > + * 'rbd_concurrent_management_ops' concurrent operations.
> > > > > + */
> > > > > +
Re: [Qemu-devel] [PATCH v3 0/4] Introduce the microvm machine type
On Thu, 25 Jul 2019 13:38:48 -0400 "Michael S. Tsirkin" wrote: > On Thu, Jul 25, 2019 at 05:39:39PM +0200, Paolo Bonzini wrote: > > On 25/07/19 17:01, Michael S. Tsirkin wrote: > > >> It would be educational to try to enable ACPI core but disable all > > >> optional features. > > > > A lot of them are select'ed so it's not easy. > > > > > Trying with ACPI_REDUCED_HARDWARE_ONLY would also be educational. > > > > That's what the NEMU guys experimented with. It's not supported by our > > DSDT since it uses ACPI GPE, > > Well there are two GPE blocks in FADT. We could just switch to > these if necesary I think. if it's simplistic vm we could build dedicated DSDT (or whole set of tables) for it and use reduced profile like arm-virt machine does (just a newer version of FADT with need flags set). That probably would cut acpi cost on QEMU side. > > and the reduction in code size is small > > (about 15000 lines of code in ACPICA, perhaps 100k if you're lucky?). > > > > Paolo > > Well ACPI is 150k loc I think, right? > > linux]$ wc -l `find drivers/acpi/ -name '*.c' `|tail -1 > 145926 total > > So 100k wouldn't be too shabby. >
Re: [Qemu-devel] [PATCH 4/4] block-backend: Queue requests while drained
On 26.07.19 13:49, Kevin Wolf wrote:
> Am 26.07.2019 um 12:50 hat Max Reitz geschrieben:
>> On 25.07.19 18:27, Kevin Wolf wrote:
>>> This fixes device like IDE that can still start new requests from I/O
>>
>> *devices
>>
>>> handlers in the CPU thread while the block backend is drained.
>>>
>>> The basic assumption is that in a drain section, no new requests should
>>> be allowed through a BlockBackend (blk_drained_begin/end don't exist,
>>> we get drain sections only on the node level). However, there are two
>>> special cases where requests should not be queued:
>>>
>>> 1. Block jobs: We already make sure that block jobs are paused in a
>>>drain section, so they won't start new requests. However, if the
>>>drain_begin is called on the job's BlockBackend first, it can happen
>>>that we deadlock because the job stays busy until it reaches a pause
>>>point - which it can't if it's requests aren't processed any more.
>>>
>>>The proper solution here would be to make all requests through the
>>>job's filter node instead of using a BlockBackend. For now, just
>>>disabling request queuin on the job BlockBackend is simpler.
>>
>> Yep, seems reasonable.
>>
>> (We’d need a relationship that a BB is owned by some job, and then pause
>> the job when the BB is drained, I suppose. But that’s exactly
>> accomplished by not making the job use a BB, but its BdrvChild
>> references instead.)
>
> We actually had this before commit ad90feba, when we changed it to use
> the job's BdrvChild objects instead. All block jobs have both currently,
> they just don't use their BdrvChild objects much.
>
>>> 2. In test cases where making requests through bdrv_* would be
>>>cumbersome because we'd need a BdrvChild. As we already got the
>>>functionality to disable request queuing from 1., use it in tests,
>>>too, for convenience.
>>>
>>> Signed-off-by: Kevin Wolf
>>> ---
>>> include/sysemu/block-backend.h | 11 +++---
>>> block/backup.c | 1 +
>>> block/block-backend.c | 69 +-
>>> block/commit.c | 2 +
>>> block/mirror.c | 6 ++-
>>> blockjob.c | 3 ++
>>> tests/test-bdrv-drain.c| 1 +
>>> 7 files changed, 76 insertions(+), 17 deletions(-)
>>
>> [...]
>>
>>> diff --git a/block/block-backend.c b/block/block-backend.c
>>> index fdd6b01ecf..603b281743 100644
>>> --- a/block/block-backend.c
>>> +++ b/block/block-backend.c
>>
>> [...]
>>
>>> @@ -1127,13 +1136,26 @@ static int blk_check_byte_request(BlockBackend
>>> *blk, int64_t offset,
>>> return 0;
>>> }
>>>
>>> +static void blk_wait_while_drained(BlockBackend *blk)
>>
>> +coroutine_fn? (Maybe even blk_co_wait...)
>>
>>> +{
>>> +if (blk->quiesce_counter && !blk->disable_request_queuing) {
>>> +qemu_co_queue_wait(>queued_requests, NULL);
>>> +}
>>> +}
>>> +
>>> int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
>>> unsigned int bytes, QEMUIOVector *qiov,
>>> - BdrvRequestFlags flags)
>>> + BdrvRequestFlags flags, bool
>>> wait_while_drained)
>>
>> What’s the purpose of this parameter? How would it hurt to always
>> wait_while_drained?
>>
>> I see the following callers of blk_co_p{read,write}v() that call it with
>> wait_while_drained=false:
>>
>> 1. blk_aio_{read,write}_entry(): They wait themselves, so they don’t
>>need these functions to wait. But OTOH, because they have waited, we
>>know that the BB is not quiesced here, so we won’t wait here anyway.
>>(These functions should be coroutine_fn, too, by the way)
>
> I think I was worried that the coroutine might yield between the two
> places. Later I noticed that blk_wait_while_drained() must be the very
> first thing anyway, so maybe it doesn't matter any more now.
>
> If we did yield here for requests coming from blk_aio_prwv(), in_flight
> would be increased and drain would deadlock.
>
> Would you prefer if I just unconditionally wait if we're drained?
I think I would, yes.
>> 2. mirror: It disables request queuing anyway, so wait_while_drained
>>doesn’t have any effect.
>
> Yes, I wasn't sure what to use there. false seemed like it would be
> less likely to cause misunderstandings because it just repeats what
> would happen anyway.
>
>>> {
>>> int ret;
>>> -BlockDriverState *bs = blk_bs(blk);
>>> +BlockDriverState *bs;
>>>
>>> +if (wait_while_drained) {
>>> +blk_wait_while_drained(blk);
>>> +}
>>
>> [...]
>>
>> What about blk_co_flush()? Should that wait, too?
>
> Hm, probably, yes.
>
>>> @@ -2232,6 +2278,9 @@ static void blk_root_drained_end(BdrvChild *child,
>>> int *drained_end_counter)
>>> if (blk->dev_ops && blk->dev_ops->drained_end) {
>>> blk->dev_ops->drained_end(blk->dev_opaque);
>>> }
>>> +while
Re: [Qemu-devel] [PULL 00/22] virtio, pc, pci: features, fixes, cleanups
On Tue, 2 Jul 2019 at 19:27, Peter Maydell wrote: > > On Tue, 2 Jul 2019 at 19:22, Michael S. Tsirkin wrote: > > > > On Tue, Jul 02, 2019 at 06:20:01PM +0100, Peter Maydell wrote: > > > On Tue, 2 Jul 2019 at 18:01, Michael S. Tsirkin wrote: > > > > This isn't from mainline. We have a bit of a deadlock with linux merge > > > > window opening soon. I think it's reasonable temporarily > > > > and then before release either virtio-pmem gets there or I will > > > > revert it and drop the header. > > > > > > It's definitely not ideal: until the headers are actually > > > upstream there's no guarantee that they won't change ABI. > > > > But then I'm watching it, if I see that I'll drop the device from qemu for > > now. > > OK; I guess we can take this for now if we make sure we revert > if the headers aren't upstream by the time we get to say rc2 > (23rd July). (That is, we'd want to do any revert shortly after > rc2, since rc3 might be the last rc before release.) Ping for status update -- have the headers hit the mainline kernel yet? Do they match the versions we put into QEMU? thanks -- PMM
Re: [Qemu-devel] [RFC 03/19] fuzz: add fuzz accelerator
On Thu, Jul 25, 2019 at 03:23:46AM +, Oleinik, Alexander wrote: > Much like the qtest accelerator, the fuzz accelerator skips the CPU > emulation > > Signed-off-by: Alexander Oleinik > --- > include/sysemu/qtest.h | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/sysemu/qtest.h b/include/sysemu/qtest.h > index cd114b8d80..adfbd10d20 100644 > --- a/include/sysemu/qtest.h > +++ b/include/sysemu/qtest.h > @@ -23,7 +23,12 @@ static inline bool qtest_enabled(void) > } > > bool qtest_driver(void); > - > +#ifdef CONFIG_FUZZ > +/* Both the client and the server have qtest_init's, Rename on of them... */ s/on/one/ > +void qtest_init_server(const char *qtest_chrdev, const char *qtest_log, > Error **errp); > +void qtest_server_recv(GString *inbuf); /* Client sends commands using this > */ qtest_server_init() is more consistent since the other function is called qtest_server_recv(). signature.asc Description: PGP signature
