Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics

[David Gibson]

On Thu, May 27, 2021 at 11:20:01AM +1000, David Gibson wrote:
> On Wed, May 26, 2021 at 05:21:01PM -0300, Bruno Larsen (billionai) wrote:
> > This function requires surce code modification to be useful, which means
> > it probably is not used often, and the move to using decodetree means
> > the statistics won't even be collected anymore.
> > 
> > Also removed setting dump_statistics in ppc_cpu_realize, since it was
> > only useful when in conjunction with ppc_cpu_dump_statistics.
> > 
> > Suggested-by: Richard Henderson
> > Signed-off-by: Bruno Larsen (billionai) 
> > ---
> >  target/ppc/cpu.h   |  1 -
> >  target/ppc/cpu_init.c  |  3 ---
> >  target/ppc/translate.c | 51 --
> >  3 files changed, 55 deletions(-)
> > 
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index 203f07e48e..c3d1b492e4 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1256,7 +1256,6 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, 
> > PPCVirtualHypervisorClass,
> >  void ppc_cpu_do_interrupt(CPUState *cpu);
> >  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
> >  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> > -void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
> >  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> >  int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
> >  int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int 
> > reg);
> > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> > index f5ae2f150d..bd05f53fa4 100644
> > --- a/target/ppc/cpu_init.c
> > +++ b/target/ppc/cpu_init.c
> > @@ -9250,9 +9250,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void 
> > *data)
> >  cc->class_by_name = ppc_cpu_class_by_name;
> >  cc->has_work = ppc_cpu_has_work;
> >  cc->dump_state = ppc_cpu_dump_state;
> > -#ifdef CONFIG_TCG
> > -cc->dump_statistics = ppc_cpu_dump_statistics;
> > -#endif
> 
> This confuses me.  The ifdefs you're removing aren't present in my
> tree, and AFAICT they never existed since your own patch created
> cpu_init.c.
> 
> So.. please rebase and check that.

Duh, sorry, I looked at this set out of order with your latest !tcg
patches.  Now that I've applied those, I've applied those one as well.

> 
> >  cc->set_pc = ppc_cpu_set_pc;
> >  cc->gdb_read_register = ppc_cpu_gdb_read_register;
> >  cc->gdb_write_register = ppc_cpu_gdb_write_register;
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 6c0f424d81..fc9fd790ca 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -8881,57 +8881,6 @@ int ppc_fixup_cpu(PowerPCCPU *cpu)
> >  return 0;
> >  }
> >  
> > -
> > -void ppc_cpu_dump_statistics(CPUState *cs, int flags)
> > -{
> > -#if defined(DO_PPC_STATISTICS)
> > -PowerPCCPU *cpu = POWERPC_CPU(cs);
> > -opc_handler_t **t1, **t2, **t3, *handler;
> > -int op1, op2, op3;
> > -
> > -t1 = cpu->env.opcodes;
> > -for (op1 = 0; op1 < 64; op1++) {
> > -handler = t1[op1];
> > -if (is_indirect_opcode(handler)) {
> > -t2 = ind_table(handler);
> > -for (op2 = 0; op2 < 32; op2++) {
> > -handler = t2[op2];
> > -if (is_indirect_opcode(handler)) {
> > -t3 = ind_table(handler);
> > -for (op3 = 0; op3 < 32; op3++) {
> > -handler = t3[op3];
> > -if (handler->count == 0) {
> > -continue;
> > -}
> > -qemu_printf("%02x %02x %02x (%02x %04d) %16s: "
> > -"%016" PRIx64 " %" PRId64 "\n",
> > -op1, op2, op3, op1, (op3 << 5) | op2,
> > -handler->oname,
> > -handler->count, handler->count);
> > -}
> > -} else {
> > -if (handler->count == 0) {
> > -continue;
> > -}
> > -qemu_printf("%02x %02x(%02x %04d) %16s: "
> > -"%016" PRIx64 " %" PRId64 "\n",
> > -op1, op2, op1, op2, handler->oname,
> > -handler->count, handler->count);
> > -}
> > -}
> > -} else {
> > -if (handler->count == 0) {
> > -continue;
> > -}
> > -qemu_printf("%02x   (%02x ) %16s: %016" PRIx64
> > -" %" PRId64 "\n",
> > -op1, op1, handler->oname,
> > -handler->count, handler->count);
> > -}
> > -}
> > -#endif
> > -}
> > -
> >  static bool decode_legacy(PowerPCCPU *cpu, DisasContext *ctx, uint32_t 
> > insn)
> >  {
> >  opc_handler_t **table, *handle

Re: [PATCH v5 1/4] target/ppc: used ternary operator when registering MAS

[David Gibson]

On Tue, May 25, 2021 at 08:53:52AM -0300, Bruno Larsen (billionai) wrote:
> The write calback decision when registering the MAS SPR has been turned
> into a ternary operation, rather than an if-then-else block.
> 
> This was done because when building without TCG, even though the
> compiler will optimize away the pointers to spr_write_generic*, it
> doesn't optimize away the decision and assignment to the local pointer,
> creating compiler errors. This cleanup looked better than using ifdefs,
> so  we decided to with it.
> 
> Signed-off-by: Bruno Larsen (billionai) 
> Reviewed-by: Richard Henderson 

Applied to ppc-for-6.1, thanks.

> ---
>  target/ppc/cpu_init.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index b696469d1a..40719f6480 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -1205,15 +1205,12 @@ static void register_BookE206_sprs(CPUPPCState *env, 
> uint32_t mas_mask,
>  /* TLB assist registers */
>  /* XXX : not implemented */
>  for (i = 0; i < 8; i++) {
> -void (*uea_write)(DisasContext *ctx, int sprn, int gprn) =
> -&spr_write_generic32;
> -if (i == 2 && (mas_mask & (1 << i)) && (env->insns_flags & PPC_64B)) 
> {
> -uea_write = &spr_write_generic;
> -}
>  if (mas_mask & (1 << i)) {
>  spr_register(env, mas_sprn[i], mas_names[i],
>   SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, uea_write,
> + &spr_read_generic,
> + (i == 2 && (env->insns_flags & PPC_64B))
> + ? &spr_write_generic : &spr_write_generic32,
>   0x);
>  }
>  }

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 4/5] monitor: removed cpustats command

[David Gibson]

On Wed, May 26, 2021 at 02:28:48PM -0700, Richard Henderson wrote:
> On 5/26/21 1:21 PM, Bruno Larsen (billionai) wrote:
> > Since ppc was the last architecture to collect these statistics and
> > it is currently phasing this collection out, the command that would query
> > this information is being removed.
> > 
> > Suggested-by: Richard Henderson 
> > Signed-off-by: Bruno Larsen (billionai) 
> > ---
> >   hmp-commands-info.hx | 13 -
> >   monitor/misc.c   | 11 ---
> >   2 files changed, 24 deletions(-)
> 
> Cc: Dr. David Alan Gilbert 
> Reviewed-by: Richard Henderson 

Applied to ppc-for-6.1.  I'm staging this in my tree, but an Ack from
Dave Gilbert would be appreciated.

> 
> 
> r~
> 
> > 
> > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > index ab0c7aa5ee..b2347a6aea 100644
> > --- a/hmp-commands-info.hx
> > +++ b/hmp-commands-info.hx
> > @@ -500,19 +500,6 @@ SRST
> >   Show the current VM UUID.
> >   ERST
> > -{
> > -.name   = "cpustats",
> > -.args_type  = "",
> > -.params = "",
> > -.help   = "show CPU statistics",
> > -.cmd= hmp_info_cpustats,
> > -},
> > -
> > -SRST
> > -  ``info cpustats``
> > -Show CPU statistics.
> > -ERST
> > -
> >   #if defined(CONFIG_SLIRP)
> >   {
> >   .name   = "usernet",
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index f3a393ea59..1539e18557 100644
> > --- a/monitor/misc.c
> > +++ b/monitor/misc.c
> > @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict 
> > *qdict)
> >   }
> >   }
> > -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> > -{
> > -CPUState *cs = mon_get_cpu(mon);
> > -
> > -if (!cs) {
> > -monitor_printf(mon, "No CPU available\n");
> > -return;
> > -}
> > -cpu_dump_statistics(cs, 0);
> > -}
> > -
> >   static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
> >   {
> >   const char *name = qdict_get_try_str(qdict, "name");
> > 
> 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH qemu v20] spapr: Implement Open Firmware client interface

[David Gibson]

On Tue, May 25, 2021 at 11:55:43AM +0200, BALATON Zoltan wrote:
> On Tue, 25 May 2021, David Gibson wrote:
> > On Mon, May 24, 2021 at 02:42:30PM +0200, BALATON Zoltan wrote:
> > > On Mon, 24 May 2021, David Gibson wrote:
> > > > On Sun, May 23, 2021 at 07:09:26PM +0200, BALATON Zoltan wrote:
> > > > > On Sun, 23 May 2021, BALATON Zoltan wrote:
> > > > > > On Sun, 23 May 2021, Alexey Kardashevskiy wrote:
> > > > > > > One thing to note about PCI is that normally I think the client
> > > > > > > expects the firmware to do PCI probing and SLOF does it. But VOF
> > > > > > > does not and Linux scans PCI bus(es) itself. Might be a problem 
> > > > > > > for
> > > > > > > you kernel.
> > > > > > 
> > > > > > I'm not sure what info does MorphOS get from the device tree and 
> > > > > > what it
> > > > > > probes itself but I think it may at least need device ids and info 
> > > > > > about
> > > > > > the PCI bus to be able to access the config regs, after that it 
> > > > > > should
> > > > > > set the devices up hopefully. I could add these from the board code 
> > > > > > to
> > > > > > device tree so VOF does not need to do anything about it. However 
> > > > > > I'm
> > > > > > not getting to that point yet because it crashes on something that 
> > > > > > it's
> > > > > > missing and couldn't yet find out what is that.
> > > > > > 
> > > > > > I'd like to get Linux working now as that would be enough to test 
> > > > > > this
> > > > > > and then if for MorphOS we still need a ROM it's not a problem if at
> > > > > > least we can boot Linux without the original firmware. But I can't 
> > > > > > make
> > > > > > Linux open a serial console and I don't know what it needs for 
> > > > > > that. Do
> > > > > > you happen to know? I've looked at the sources in 
> > > > > > Linux/arch/powerpc but
> > > > > > not sure how it would find and open a serial port on pegasos2. It 
> > > > > > seems
> > > > > > to work with the board firmware and now I can get it to boot with 
> > > > > > VOF
> > > > > > but then it does not open serial so it probably needs something in 
> > > > > > the
> > > > > > device tree or expects the firmware to set something up that we 
> > > > > > should
> > > > > > add in pegasos2.c when using VOF.
> > > > > 
> > > > > I've now found that Linux uses rtas methods read-pci-config and
> > > > > write-pci-config for PCI access on pegasos2 so this means that we'll
> > > > > probably need rtas too (I hoped we could get away without it if it 
> > > > > were only
> > > > > used for shutdown/reboot or so but seems Linux needs it for PCI as 
> > > > > well and
> > > > > does not scan the bus and won't find some devices without it).
> > > > 
> > > > Yes, definitely sounds like you'll need an RTAS implementation.
> > > > 
> > > > > While VOF can do rtas, this causes a problem with the hypercall 
> > > > > method using
> > > > > sc 1 that goes through vhyp but trips the assert in ppc_store_sdr1() 
> > > > > so
> > > > > cannot work after guest is past quiesce.
> > > > 
> > > > > So the question is why is that
> > > > > assert there
> > > > 
> > > > Ah.. right.  So, vhyp was designed for the PAPR use case, where we
> > > > want to model the CPU when it's in supervisor and user mode, but not
> > > > when it's in hypervisor mode.  We want qemu to mimic the behaviour of
> > > > the hypervisor, rather than attempting to actually execute hypervisor
> > > > code in the virtual CPU.
> > > > 
> > > > On systems that have a hypervisor mode, SDR1 is hypervisor privileged,
> > > > so it makes no sense for the guest to attempt to set it.  That should
> > > > be caught by the general SPR code and turned into a 0x700, hence the
> > > > assert() if we somehow reach ppc_store_sdr1().
> > > 
> > > This seems to work to avoid my problem so I can leave vhyp enabled after
> > > qiuesce for now:
> > > 
> > > diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
> > > index d957d1a687..13b87b9b36 100644
> > > --- a/target/ppc/cpu.c
> > > +++ b/target/ppc/cpu.c
> > > @@ -70,7 +70,7 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong 
> > > value)
> > >  {
> > >  PowerPCCPU *cpu = env_archcpu(env);
> > >  qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, 
> > > value);
> > > -assert(!cpu->vhyp);
> > > +assert(!cpu->env.has_hv_mode || !cpu->vhyp);
> > >  #if defined(TARGET_PPC64)
> > >  if (mmu_is_64bit(env->mmu_model)) {
> > >  target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE;
> > > 
> > > But I wonder if the assert should also be moved within the TARGET_PPC64
> > > block and if we may need to generate some exception here instead. Not sure
> > > what a real CPU would do in this case but if accessing sdr1 is privileged 
> > > in
> > > HV mode then there should be an exception or if that's catched
> > > elsewhere
> > 
> > It should be caught elsehwere.  Specifically, when the SDR1 SPR is
> > registered, on CPUs with a hypervisor mode it should be registered as
> > hypervisor privilege

Re: [PATCH qemu v20] spapr: Implement Open Firmware client interface

[David Gibson]

On Tue, May 25, 2021 at 12:08:45PM +0200, BALATON Zoltan wrote:
> On Tue, 25 May 2021, David Gibson wrote:
> > On Mon, May 24, 2021 at 12:55:07PM +0200, BALATON Zoltan wrote:
> > > On Mon, 24 May 2021, David Gibson wrote:
> > > > On Sun, May 23, 2021 at 07:09:26PM +0200, BALATON Zoltan wrote:
> > > > > On Sun, 23 May 2021, BALATON Zoltan wrote:
> > > > > > On Sun, 23 May 2021, Alexey Kardashevskiy wrote:
> > > > > > > One thing to note about PCI is that normally I think the client
> > > > > > > expects the firmware to do PCI probing and SLOF does it. But VOF
> > > > > > > does not and Linux scans PCI bus(es) itself. Might be a problem 
> > > > > > > for
> > > > > > > you kernel.
> > > > > > 
> > > > > > I'm not sure what info does MorphOS get from the device tree and 
> > > > > > what it
> > > > > > probes itself but I think it may at least need device ids and info 
> > > > > > about
> > > > > > the PCI bus to be able to access the config regs, after that it 
> > > > > > should
> > > > > > set the devices up hopefully. I could add these from the board code 
> > > > > > to
> > > > > > device tree so VOF does not need to do anything about it. However 
> > > > > > I'm
> > > > > > not getting to that point yet because it crashes on something that 
> > > > > > it's
> > > > > > missing and couldn't yet find out what is that.
> > > > > > 
> > > > > > I'd like to get Linux working now as that would be enough to test 
> > > > > > this
> > > > > > and then if for MorphOS we still need a ROM it's not a problem if at
> > > > > > least we can boot Linux without the original firmware. But I can't 
> > > > > > make
> > > > > > Linux open a serial console and I don't know what it needs for 
> > > > > > that. Do
> > > > > > you happen to know? I've looked at the sources in 
> > > > > > Linux/arch/powerpc but
> > > > > > not sure how it would find and open a serial port on pegasos2. It 
> > > > > > seems
> > > > > > to work with the board firmware and now I can get it to boot with 
> > > > > > VOF
> > > > > > but then it does not open serial so it probably needs something in 
> > > > > > the
> > > > > > device tree or expects the firmware to set something up that we 
> > > > > > should
> > > > > > add in pegasos2.c when using VOF.
> > > > > 
> > > > > I've now found that Linux uses rtas methods read-pci-config and
> > > > > write-pci-config for PCI access on pegasos2 so this means that we'll
> > > > > probably need rtas too (I hoped we could get away without it if it 
> > > > > were only
> > > > > used for shutdown/reboot or so but seems Linux needs it for PCI as 
> > > > > well and
> > > > > does not scan the bus and won't find some devices without it).
> > > > 
> > > > Yes, definitely sounds like you'll need an RTAS implementation.
> > > 
> > > I plan to fix that after managed to get serial working as that seems to 
> > > not
> > > need it. If I delete the rtas-size property from /rtas on the original
> > > firmware that makes Linux skip instantiating rtas, but I still get serial
> > > output just not accessing PCI devices. So I think it should work and keeps
> > > things simpler at first. Then I'll try rtas later.
> > > 
> > > > > While VOF can do rtas, this causes a problem with the hypercall 
> > > > > method using
> > > > > sc 1 that goes through vhyp but trips the assert in ppc_store_sdr1() 
> > > > > so
> > > > > cannot work after guest is past quiesce.
> > > > 
> > > > > So the question is why is that
> > > > > assert there
> > > > 
> > > > Ah.. right.  So, vhyp was designed for the PAPR use case, where we
> > > > want to model the CPU when it's in supervisor and user mode, but not
> > > > when it's in hypervisor mode.  We want qemu to mimic the behaviour of
> > > > the hypervisor, rather than attempting to actually execute hypervisor
> > > > code in the virtual CPU.
> > > > 
> > > > On systems that have a hypervisor mode, SDR1 is hypervisor privileged,
> > > > so it makes no sense for the guest to attempt to set it.  That should
> > > > be caught by the general SPR code and turned into a 0x700, hence the
> > > > assert() if we somehow reach ppc_store_sdr1().
> > > > 
> > > > So, we are seeing a problem here because you want the 'sc 1'
> > > > interception of vhyp, but not the rest of the stuff that goes with it.
> > > > 
> > > > > and would using sc 1 for hypercalls on pegasos2 cause other
> > > > > problems later even if the assert could be removed?
> > > > 
> > > > At least in the short term, I think you probably can remove the
> > > > assert.  In your case the 'sc 1' calls aren't truly to a hypervisor,
> > > > but a special case escape to qemu for the firmware emulation.  I think
> > > > it's unlikely to cause problems later, because nothing on a 32-bit
> > > > system should be attempting an 'sc 1'.  The only thing I can think of
> > > > that would fail is some test case which explicitly verified that 'sc
> > > > 1' triggered a 0x700 (SIGILL from userspace).
> > > 
> > > OK so the assert should check if the CPU

Re: [PATCH v5 4/4] target/ppc: updated meson.build to support disable-tcg

[David Gibson]

On Tue, May 25, 2021 at 08:53:55AM -0300, Bruno Larsen (billionai) wrote:
> updated build file to not compile some sources that are unnecessary if
> TCG is disabled on the system.
> 
> Signed-off-by: Bruno Larsen (billionai)
> 

Applied to ppc-for-6.1, thanks.

> ---
>  target/ppc/meson.build | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/meson.build b/target/ppc/meson.build
> index 848e625302..a6a53a8d5c 100644
> --- a/target/ppc/meson.build
> +++ b/target/ppc/meson.build
> @@ -3,11 +3,14 @@ ppc_ss.add(files(
>'cpu-models.c',
>'cpu.c',
>'cpu_init.c',
> -  'dfp_helper.c',
>'excp_helper.c',
> -  'fpu_helper.c',
>'gdbstub.c',
>'helper_regs.c',
> +))
> +
> +ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
> +  'dfp_helper.c',
> +  'fpu_helper.c',
>'int_helper.c',
>'mem_helper.c',
>'misc_helper.c',

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v5 3/4] target/ppc: created tcg-stub.c file

[David Gibson]

On Tue, May 25, 2021 at 08:53:54AM -0300, Bruno Larsen (billionai) wrote:
> Created a file with stubs needed to compile disabling TCG. *_ppc_opcodes
> were created to make cpu_init.c have a few less ifdefs, since they are
> not needed. softmmu_resize_hpt_* have to be created because the compiler
> can't automatically know they aren't used, but they should never be
> reached.
> 
> Signed-off-by: Bruno Larsen (billionai)
> 

Applied to ppc-for-6.1, thanks.

> ---
>  target/ppc/meson.build |  4 
>  target/ppc/tcg-stub.c  | 45 ++
>  2 files changed, 49 insertions(+)
>  create mode 100644 target/ppc/tcg-stub.c
> 
> diff --git a/target/ppc/meson.build b/target/ppc/meson.build
> index d1aa7d5d39..848e625302 100644
> --- a/target/ppc/meson.build
> +++ b/target/ppc/meson.build
> @@ -28,6 +28,10 @@ ppc_softmmu_ss.add(files(
>'mmu_helper.c',
>'monitor.c',
>  ))
> +ppc_softmmu_ss.add(when: 'CONFIG_TCG', if_false: files(
> +  'tcg-stub.c'
> +))
> +
>  ppc_softmmu_ss.add(when: 'TARGET_PPC64', if_true: files(
>'compat.c',
>'mmu-book3s-v3.c',
> diff --git a/target/ppc/tcg-stub.c b/target/ppc/tcg-stub.c
> new file mode 100644
> index 00..aadcf59d26
> --- /dev/null
> +++ b/target/ppc/tcg-stub.c
> @@ -0,0 +1,45 @@
> +/*
> + *  PowerPC CPU initialization for qemu.
> + *
> + *  Copyright (C) 2021 Instituto de Pesquisas Eldorado (eldorado.org.br)
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> .
> + */
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "internal.h"
> +#include "hw/ppc/spapr.h"
> +
> +void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp)
> +{
> +}
> +
> +void destroy_ppc_opcodes(PowerPCCPU *cpu)
> +{
> +}
> +
> +target_ulong softmmu_resize_hpt_prepare(PowerPCCPU *cpu,
> +SpaprMachineState *spapr,
> +target_ulong shift)
> +{
> +g_assert_not_reached();
> +}
> +
> +target_ulong softmmu_resize_hpt_commit(PowerPCCPU *cpu,
> +   SpaprMachineState *spapr,
> +   target_ulong flags,
> +   target_ulong shift)
> +{
> +g_assert_not_reached();
> +}

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH] spapr: propagate LPCR to hot-plugged CPUs

[David Gibson]

On Mon, May 24, 2021 at 01:41:32PM +0200, Cédric Le Goater wrote:
> Distros have started using the 'scv' instructions (glibc 2.33) which
> relies on the LPCR AIL bits. Unfortunately, the LPCR of hot-plugged
> CPUs is not synchronized with the rest of machine and it breaks the
> guest OS.
> 
> Fix that by using the first CPU to set the LPCR value of all hot-plugged
> CPUs.
> 
> Signed-off-by: Cédric Le Goater 

I'm assuming this is obsoleted by Nick Piggin's rework of LPCR
initialization.  This patch does fix a real bug, but it leaves LPCR
initialization a bit of a mess.

> ---
>  hw/ppc/spapr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c23bcc449071..e463c2570c7a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3890,6 +3890,8 @@ static void spapr_core_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev)
>  for (i = 0; i < cc->nr_threads; i++) {
>  ppc_set_compat(core->threads[i], 
> POWERPC_CPU(first_cpu)->compat_pvr,
> &error_abort);
> +ppc_store_lpcr(core->threads[i],
> +   POWERPC_CPU(first_cpu)->env.spr[SPR_LPCR]);
>  }
>  }
>  

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PULL 19/19] gitlab: Split gprof-gcov job

[Philippe Mathieu-Daudé]

On 5/26/21 11:18 PM, Philippe Mathieu-Daudé wrote:
> This job is hitting the 70min limit, so split it in 2 tasks.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Willian Rampazzo 
> Message-Id: <20210525082556.4011380-7-f4...@amsat.org>
> ---
>  .gitlab-ci.d/buildtest.yml | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
> index 807040c1c71..7bfbfab8f20 100644
> --- a/.gitlab-ci.d/buildtest.yml
> +++ b/.gitlab-ci.d/buildtest.yml
> @@ -558,16 +558,27 @@ check-deprecated:
>allow_failure: true
>  
>  # gprof/gcov are GCC features
> -gprof-gcov:
> +build-gprof-gcov:
>extends: .native_build_job_template
>needs:
>  job: amd64-ubuntu2004-container
>variables:
>  IMAGE: ubuntu2004
>  CONFIGURE_ARGS: --enable-gprof --enable-gcov
> -MAKE_CHECK_ARGS: check
>  TARGETS: aarch64-softmmu ppc64-softmmu s390x-softmmu x86_64-softmmu
> -  timeout: 70m
> +  artifacts:
> +expire_in: 1 days
> +paths:
> +  - build
> +
> +check-gprof-gcov:
> +  extends: .native_test_job_template
> +  needs:
> +- job: build-gprof-gcov
> +  artifacts: true
> +  variables:
> +IMAGE: ubuntu2004
> +MAKE_CHECK_ARGS: check
>after_script:
>  - ${CI_PROJECT_DIR}/scripts/ci/coverage-summary.sh
>  
> 

This patch was not supposed to be in this pull request. Since
the request hasn't been processed, I pushed an updated tag with
this patch removed.

Re: [PATCH v5 2/4] target/ppc: added ifdefs around TCG-only code

[David Gibson]

On Wed, May 26, 2021 at 02:24:32PM -0300, Bruno Piazera Larsen wrote:
> 
> On 25/05/2021 10:02, Philippe Mathieu-Daudé wrote:
> > On 5/25/21 1:53 PM, Bruno Larsen (billionai) wrote:
> > > excp_helper.c, mmu-hash64.c and mmu_helper.c have some function
> > > declarations that are TCG-only, and couldn't be easily moved to a
> > > TCG only file, so ifdefs were added around them.
> > > 
> > > We also needed ifdefs around some header files because helper-proto.h
> > > includes trace/generated-helpers.h, which is never created when building
> > > without TCG, and cpu_ldst.h includes tcg/tcg.h, whose containing folder
> > > is not included as a -iquote. As future cleanup, we could change the
> > > part of the configuration script to add those.
> > > 
> > > cpu_init.c also had a callback definition that is TCG only and could be
> > > removed as part of a future cleanup (all the dump_statistics part is
> > > almost never used and will become obsolete as we transition to using
> > > decodetree).
> > > 
> > > Signed-off-by: Bruno Larsen (billionai) 
> > > ---
> > >   target/ppc/cpu_init.c|  2 ++
> > >   target/ppc/excp_helper.c | 21 ++---
> > >   target/ppc/mmu-hash64.c  | 11 ++-
> > >   target/ppc/mmu_helper.c  | 16 ++--
> > >   4 files changed, 44 insertions(+), 6 deletions(-)
> > Please have a look at commit range 0a31c16c9ce..a2b0a27d33e
> > for the MIPS convertion.
> > 
> > >   #if !defined(CONFIG_USER_ONLY)
> > > +#ifdef CONFIG_TCG
> > >   void helper_store_msr(CPUPPCState *env, target_ulong val)
> > >   {
> > For example this one is similar to commit d60146a9389, you
> > could simply move this function to tcg/sysemu/msr_helpers.c
> > and modify the meson file, then when TCG is not available,
> > the file isn't built, without having to use #ifdef'ry.
> 
> I can see what you mean, but I think the point was to not create separate
> files solely based on the accelerator type.
> 
> It's up to dgibson if we use that approach, but I agree that it could make
> the code quite a bit cleaner.
> 
> The next question would then be: should we go the whole 9 yards and add
> tcg/sysemu/* and tcg/linux-user/*, or can we just use tcg/* and rely on devs
> reading and understanding the meson.build file? I believe tcg/sysemu/* is
> going to be very empty (for now, only what is in mmu-hash64.c and is
> TCG-only, IIRC), so it sounds like a bit of an overkill, but I also see the
> argument for future-proofing.

The ifdefs are pretty messy, but drafts based on splitting into
separate files have hit their own complications.  So, I'm inclined to
just go with the ifdefs for now, and hope to clean things up further
in future.

So, applied to ppc-for-6.1.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 3/5] target/ppc: removed mentions to DO_PPC_STATISTICS

[David Gibson]

On Wed, May 26, 2021 at 05:21:02PM -0300, Bruno Larsen (billionai) wrote:
> Removed the commented out definition and all ifdefs relating to
> PPC_DUMP_STATISTICS, as it's hardly ever used.
> 
> Suggested-by: Richard Henderson 
> Signed-off-by: Bruno Larsen (billionai)
> 

Applied to ppc-for-6.1, thanks.

> ---
>  target/ppc/translate.c | 16 
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index fc9fd790ca..0525e1939f 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -47,7 +47,6 @@
>  
>  /* Include definitions for instructions classes and implementations flags */
>  /* #define PPC_DEBUG_DISAS */
> -/* #define DO_PPC_STATISTICS */
>  
>  #ifdef PPC_DEBUG_DISAS
>  #  define LOG_DISAS(...) qemu_log_mask(CPU_LOG_TB_IN_ASM, ## __VA_ARGS__)
> @@ -217,12 +216,9 @@ struct opc_handler_t {
>  uint64_t type2;
>  /* handler */
>  void (*handler)(DisasContext *ctx);
> -#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
> +#if defined(PPC_DUMP_CPU)
>  const char *oname;
>  #endif
> -#if defined(DO_PPC_STATISTICS)
> -uint64_t count;
> -#endif
>  };
>  
>  /* SPR load/store helpers */
> @@ -8546,7 +8542,7 @@ static int register_direct_insn(opc_handler_t 
> **ppc_opcodes,
>  if (insert_in_table(ppc_opcodes, idx, handler) < 0) {
>  printf("*** ERROR: opcode %02x already assigned in main "
> "opcode table\n", idx);
> -#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
> +#if defined(PPC_DUMP_CPU)
>  printf("   Registered handler '%s' - new handler '%s'\n",
> ppc_opcodes[idx]->oname, handler->oname);
>  #endif
> @@ -8570,7 +8566,7 @@ static int register_ind_in_table(opc_handler_t **table,
>  if (!is_indirect_opcode(table[idx1])) {
>  printf("*** ERROR: idx %02x already assigned to a direct "
> "opcode\n", idx1);
> -#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
> +#if defined(PPC_DUMP_CPU)
>  printf("   Registered handler '%s' - new handler '%s'\n",
> ind_table(table[idx1])[idx2]->oname, handler->oname);
>  #endif
> @@ -8581,7 +8577,7 @@ static int register_ind_in_table(opc_handler_t **table,
>  insert_in_table(ind_table(table[idx1]), idx2, handler) < 0) {
>  printf("*** ERROR: opcode %02x already assigned in "
> "opcode table %02x\n", idx2, idx1);
> -#if defined(DO_PPC_STATISTICS) || defined(PPC_DUMP_CPU)
> +#if defined(PPC_DUMP_CPU)
>  printf("   Registered handler '%s' - new handler '%s'\n",
> ind_table(table[idx1])[idx2]->oname, handler->oname);
>  #endif
> @@ -9036,10 +9032,6 @@ static void ppc_tr_translate_insn(DisasContextBase 
> *dcbase, CPUState *cs)
>  gen_invalid(ctx);
>  }
>  
> -#if defined(DO_PPC_STATISTICS)
> -handler->count++;
> -#endif
> -
>  translator_loop_temp_check(&ctx->base);
>  }
>  

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PULL V2 0/3] Net patches

[Jason Wang]

在 2021/5/27 下午3:14, Bin Meng 写道:

On Thu, May 27, 2021 at 2:35 PM Jason Wang  wrote:


在 2021/5/27 下午2:13, Bin Meng 写道:

Hi Jason,

On Thu, May 27, 2021 at 12:24 PM Jason Wang  wrote:

The following changes since commit d90f154867ec0ec22fd719164b88716e8fd48672:

Merge remote-tracking branch 'remotes/dg-gitlab/tags/ppc-for-6.1-20210504' 
into staging (2021-05-05 20:29:14 +0100)

are available in the git repository at:

https://github.com/jasowang/qemu.git tags/net-pull-request

for you to fetch changes up to 4f8a39494aded9f2026a26b137378ea2ee3d5338:

tap-bsd: Remove special casing for older OpenBSD releases (2021-05-27 
11:03:55 +0800)




Brad Smith (1):
tap-bsd: Remove special casing for older OpenBSD releases

Guenter Roeck (1):
hw/net/imx_fec: return 0x when accessing non-existing PHY

Laurent Vivier (1):
virtio-net: failover: add missing 
remove_migration_state_change_notifier()

   hw/net/imx_fec.c| 8 +++-
   hw/net/trace-events | 2 ++
   hw/net/virtio-net.c | 1 +
   net/tap-bsd.c   | 8 
   4 files changed, 6 insertions(+), 13 deletions(-)

What happened to patch 5-12 in the following series?
http://patchwork.ozlabs.org/project/qemu-devel/cover/20210317062638.72626-1-bmeng...@gmail.com/


I want to do some test before the merging. Or if possible, could you
please write a test for this function?


For each of these network adapter models?



e1000 and virtio-net should be sufficient.



  What kind of tests are
needed?



Test whether padding works.



  Any pointers?



You can start to look at virtio-net-test.c.

Thanks




Regards,
Bin

Re: [PULL V2 0/3] Net patches

[Bin Meng]

On Thu, May 27, 2021 at 2:35 PM Jason Wang  wrote:
>
>
> 在 2021/5/27 下午2:13, Bin Meng 写道:
> > Hi Jason,
> >
> > On Thu, May 27, 2021 at 12:24 PM Jason Wang  wrote:
> >> The following changes since commit 
> >> d90f154867ec0ec22fd719164b88716e8fd48672:
> >>
> >>Merge remote-tracking branch 
> >> 'remotes/dg-gitlab/tags/ppc-for-6.1-20210504' into staging (2021-05-05 
> >> 20:29:14 +0100)
> >>
> >> are available in the git repository at:
> >>
> >>https://github.com/jasowang/qemu.git tags/net-pull-request
> >>
> >> for you to fetch changes up to 4f8a39494aded9f2026a26b137378ea2ee3d5338:
> >>
> >>tap-bsd: Remove special casing for older OpenBSD releases (2021-05-27 
> >> 11:03:55 +0800)
> >>
> >> 
> >>
> >> 
> >> Brad Smith (1):
> >>tap-bsd: Remove special casing for older OpenBSD releases
> >>
> >> Guenter Roeck (1):
> >>hw/net/imx_fec: return 0x when accessing non-existing PHY
> >>
> >> Laurent Vivier (1):
> >>virtio-net: failover: add missing 
> >> remove_migration_state_change_notifier()
> >>
> >>   hw/net/imx_fec.c| 8 +++-
> >>   hw/net/trace-events | 2 ++
> >>   hw/net/virtio-net.c | 1 +
> >>   net/tap-bsd.c   | 8 
> >>   4 files changed, 6 insertions(+), 13 deletions(-)
> > What happened to patch 5-12 in the following series?
> > http://patchwork.ozlabs.org/project/qemu-devel/cover/20210317062638.72626-1-bmeng...@gmail.com/
>
>
> I want to do some test before the merging. Or if possible, could you
> please write a test for this function?
>

For each of these network adapter models? What kind of tests are
needed? Any pointers?

Regards,
Bin

Re: [PATCH v6 10/19] i386: move eVMCS enablement to hyperv_init_vcpu()

[Vitaly Kuznetsov]

Eduardo Habkost  writes:

> On Mon, May 24, 2021 at 02:00:37PM +0200, Vitaly Kuznetsov wrote:
> [...]
>> >> @@ -1455,6 +1454,21 @@ static int hyperv_init_vcpu(X86CPU *cpu)
>> >>  }
>> >>  }
>> >>  
>> >> +if (hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS)) {
>> >> +uint16_t evmcs_version;
>> >> +
>> >> +ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
>> >> +  (uintptr_t)&evmcs_version);
>> >> +
>> >> +if (ret < 0) {
>> >> +fprintf(stderr, "Hyper-V %s is not supported by kernel\n",
>> >> +kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
>> >> +return ret;
>> >> +}
>> >> +
>> >> +cpu->hyperv_nested[0] = evmcs_version;
>> >
>> > Wait, won't this break guest ABI?  Do we want to make
>> > HYPERV_FEAT_EVMCS a migration blocker until this is fixed?
>> >
>> 
>> Could you please elaborate on the issue? I read the above is: when 
>> evmcs' feature was requested, make an attempt to enable
>> KVM_CAP_HYPERV_ENLIGHTENED_VMCS, and bail out if this fails. Propagate
>> the the acquired evmcs version to 'cpu->hyperv_nested[]' otherwise.
>
> This will be visible to the guest at CPUID[0x400A].EAX,
> correct?  You are initializing CPUID data with a value that
> change depending on the host.
>
> What is supposed to happen if live migrating to to a host with a
> different evmcs_version?

(Note: 'evmcs_version' here is the 'maximum supported evmcs version',
not 'used evmcs version').

This is a purely theoretical question at this moment as there's only one
existing (and supported) eVMCS version: 1.

In future, when (and if) e.g. EVMCSv2 appears, we'll have to introduce a
different QEMU option for it most likely (or something like
'hv-evmcs=1', 'hv-evmcs=2' ... ) as how else would we prevent migration
to a host which doesn't support certain eVMCS version (e.g. EVMCSv2 ->
EVMCSv1)?

I'd be fine with hardcoding '1' and just checking that the returned
version is >= 1 for now. Migration blocker seems to be an overkill (as
there's no real problem, we're just trying to make the code future
proof). 

-- 
Vitaly

Re: [PATCH v6 15/19] i386: expand Hyper-V features during CPU feature expansion time

[Vitaly Kuznetsov]

Eduardo Habkost  writes:

> On Mon, May 24, 2021 at 02:13:09PM +0200, Vitaly Kuznetsov wrote:
> [...]
>> >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> >> index a42263b24fca..d5551c4ab5cf 100644
>> >> --- a/target/i386/kvm/kvm.c
>> >> +++ b/target/i386/kvm/kvm.c
>> >> @@ -1216,13 +1216,22 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, 
>> >> uint32_t func, int reg)
>> >>   * of 'hv_passthrough' mode and fills the environment with all supported
>> >>   * Hyper-V features.
>> >>   */
>> >> -static void hyperv_expand_features(CPUState *cs, Error **errp)
>> >> +void kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
>> >>  {
>> >> -X86CPU *cpu = X86_CPU(cs);
>> >> +CPUState *cs = CPU(cpu);
>> >>  
>> >>  if (!hyperv_enabled(cpu))
>> >>  return;
>> >>  
>> >> +/*
>> >> + * When kvm_hyperv_expand_features is called at CPU feature expansion
>> >> + * time per-CPU kvm_state is not available yet so we can only proceed
>> >> + * when KVM_CAP_SYS_HYPERV_CPUID is supported.
>> >> + */
>> >> +if (!cs->kvm_state &&
>> >> +!kvm_check_extension(kvm_state, KVM_CAP_SYS_HYPERV_CPUID))
>> >> +return;
>> >> +
>> >>  if (cpu->hyperv_passthrough) {
>> >>  cpu->hyperv_vendor_id[0] =
>> >>  hv_cpuid_get_host(cs, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, 
>> >> R_EBX);
>> >> @@ -1556,7 +1565,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>> >>  env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY;
>> >>  
>> >>  /* Paravirtualization CPUIDs */
>> >> -hyperv_expand_features(cs, &local_err);
>> >> +kvm_hyperv_expand_features(cpu, &local_err);
>> >
>> > Do we still need to call the function again here?
>> >
>> > If the first expansion isn't expanding everything, I'm afraid
>> > this second call will hide bugs in query-cpu-model-expansion.
>> >
>> 
>> The first expansion will do nothing if KVM_CAP_SYS_HYPERV_CPUID is not
>> supported, calling it here allows us to proceed. The series makes
>> 'query-cpu-model-expansion' output correct only with
>> KVM_CAP_SYS_HYPERV_CPUID, without it we don't seem to be able to do much
>> (unless we decide to create a 'scratch' CPU or something like that).
>
> Oh, I see.  I suggest adding a comment explaining that.
> Developers might be tempted to delete it and not notice it breaks
> under older kernels.

Will do, thanks!

-- 
Vitaly

Re: [PATCH v6 17/19] i386: HV_HYPERCALL_AVAILABLE privilege bit is always needed

[Vitaly Kuznetsov]

Eduardo Habkost  writes:

> On Mon, May 24, 2021 at 02:22:47PM +0200, Vitaly Kuznetsov wrote:
>> Eduardo Habkost  writes:
>> 
>> > On Thu, Apr 22, 2021 at 06:11:28PM +0200, Vitaly Kuznetsov wrote:
>> >> According to TLFS, Hyper-V guest is supposed to check
>> >> HV_HYPERCALL_AVAILABLE privilege bit before accessing
>> >> HV_X64_MSR_GUEST_OS_ID/HV_X64_MSR_HYPERCALL MSRs but at least some
>> >> Windows versions ignore that. As KVM is very permissive and allows
>> >> accessing these MSRs unconditionally, no issue is observed. We may,
>> >> however, want to tighten the checks eventually. Conforming to the
>> >> spec is probably also a good idea.
>> >> 
>> >> Add HV_HYPERCALL_AVAILABLE to all 'leaf' features with no dependencies.
>> >> 
>> >> Signed-off-by: Vitaly Kuznetsov 
>> >
>> > Are all VMs being created with HV_HYPERCALL_AVAILABLE unset,
>> > today?
>> >
>> 
>> No, we have HV_HYPERCALL_AVAILABLE encoded in 'hv-relaxed','hv-vapic'
>> and 'hv-time' features but not 
>> 
>> 
>> > Wouldn't it be simpler to simply add a new
>> > HYPERV_FEAT_HYPERCALL_AVAILABLE bit to hyperv_features, and
>> > enabling it by default?
>> >
>> 
>> We could do that but as I note above, we already have it for three
>> features.
>
> Do we have any cases where we do not want to enable
> HV_HYPERCALL_AVAILABLE?
>
> Would it be OK to just hardcoded it in hyperv_fill_cpuids() like
> we do with HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE?
>

struct kvm_hyperv_properties[] serves two purposes:
1) Set corresponding guest visible CPUID bits when certain features are
enabled.

2) Check, that KVM supports certain features before we expose them to the
  guest.

Whatever we hardcode in hyperv_fill_cpuids() gives us 1) but not 2). For
this particular bit it probably doesn't matter as even the oldest
supported kernel (v4.5) has it. That said, I'm OK with moving this to
hyperv_fill_cpuids().

>> 
>> 
>> > We don't necessarily need to make it configurable by the user,
>> > but probably it would be a good idea to keep the bit unset by
>> > default on older machine types.  Even if guests don't mind seeing
>> > the bit changing under their feet, it would make it easier for
>> > automated test cases that check for unexpected changes in raw
>> > CPUID data.
>> 
>> I see current situation as a bug. While most likely nobody runs with
>> a configuration like 'hv-vpindexem,hv-synic' it is still valid. And if KVM
>> was enforcing the features (not yet), Windows would've just crashed in
>> early boot. Normal configurations will likely always include at least
>> 'hv-time' which has HYPERV_FEAT_HYPERCALL_AVAILABLE enabled.
>> 
>> That being said, I'm not sure we need to maintain 'bug compatibility'
>> even for older machine types. I'm also not aware of any specific tests
>> for such 'crazy' configurations out there. The last patch of the series
>> adds a very simple test to qtest but this is about it.
>
> If you are 100% sure the CPUID change can't crash or confuse a
> guest, then that's OK.  I agree that bug compatibility is not a
> must if the bit is simply ignored by most guests and by KVM
> emulation code.

Strictly speaking, this bit has to be set or the guest can't use any of
the Hyper-V features. It was proven that at least certain Windows
versions don't even check it assuming it's always set. Moreover, we
already set it for some very basic enlightenments ('hv-time') so there's
going to be no change at all for real world configutations.

>
>
>> 
>> >
>> >
>> >> ---
>> >>  target/i386/kvm/kvm.c | 15 +--
>> >>  1 file changed, 9 insertions(+), 6 deletions(-)
>> >> 
>> >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> >> index 2c1a77f9b00f..d81451276cd8 100644
>> >> --- a/target/i386/kvm/kvm.c
>> >> +++ b/target/i386/kvm/kvm.c
>> >> @@ -835,6 +835,8 @@ static struct {
>> >>  [HYPERV_FEAT_CRASH] = {
>> >>  .desc = "crash MSRs (hv-crash)",
>> >>  .flags = {
>> >> +{.func = HV_CPUID_FEATURES, .reg = R_EAX,
>> >> + .bits = HV_HYPERCALL_AVAILABLE},
>> >>  {.func = HV_CPUID_FEATURES, .reg = R_EDX,
>> >>   .bits = HV_GUEST_CRASH_MSR_AVAILABLE}
>> >>  }
>> >> @@ -843,28 +845,28 @@ static struct {
>> >>  .desc = "reset MSR (hv-reset)",
>> >>  .flags = {
>> >>  {.func = HV_CPUID_FEATURES, .reg = R_EAX,
>> >> - .bits = HV_RESET_AVAILABLE}
>> >> + .bits = HV_HYPERCALL_AVAILABLE | HV_RESET_AVAILABLE}
>> >>  }
>> >>  },
>> >>  [HYPERV_FEAT_VPINDEX] = {
>> >>  .desc = "VP_INDEX MSR (hv-vpindex)",
>> >>  .flags = {
>> >>  {.func = HV_CPUID_FEATURES, .reg = R_EAX,
>> >> - .bits = HV_VP_INDEX_AVAILABLE}
>> >> + .bits = HV_HYPERCALL_AVAILABLE | HV_VP_INDEX_AVAILABLE}
>> >>  }
>> >>  },
>> >>  [HYPERV_FEAT_RUNTIME] = {
>> >>  .desc = "VP_RUNTIME MSR (hv-runtime)",
>> >>  .flags = {
>> >>  {.func = HV_CPUID_FEATURES, .re

Re: [PATCH v6 00/19] i386: KVM: expand Hyper-V features early

[Vitaly Kuznetsov]

Eduardo Habkost  writes:

> On Thu, Apr 22, 2021 at 06:11:11PM +0200, Vitaly Kuznetsov wrote:
>> Vitaly Kuznetsov (19):
>>   i386: keep hyperv_vendor string up-to-date
>>   i386: invert hyperv_spinlock_attempts setting logic with
>> hv_passthrough
>>   i386: always fill Hyper-V CPUID feature leaves from X86CPU data
>>   i386: stop using env->features[] for filling Hyper-V CPUIDs
>>   i386: introduce hyperv_feature_supported()
>>   i386: introduce hv_cpuid_get_host()
>>   i386: drop FEAT_HYPERV feature leaves
>>   i386: introduce hv_cpuid_cache
>>   i386: split hyperv_handle_properties() into
>> hyperv_expand_features()/hyperv_fill_cpuids()
>>   i386: move eVMCS enablement to hyperv_init_vcpu()
>>   i386: switch hyperv_expand_features() to using error_setg()
>>   i386: adjust the expected KVM_GET_SUPPORTED_HV_CPUID array size
>>   i386: prefer system KVM_GET_SUPPORTED_HV_CPUID ioctl over vCPU's one
>>   i386: use global kvm_state in hyperv_enabled() check
>
> I'm queueing patches 1-14 (the ones above) on my x86-next branch.

Thank you! Is it published somewhere so I can base next version[s] on it? 

-- 
Vitaly

Re: [PATCH v12 7/8] KVM: arm64: ioctl to fetch/store tags in a guest

[Steven Price]

On 24/05/2021 19:11, Catalin Marinas wrote:
> On Fri, May 21, 2021 at 10:42:09AM +0100, Steven Price wrote:
>> On 20/05/2021 18:27, Catalin Marinas wrote:
>>> On Thu, May 20, 2021 at 04:58:01PM +0100, Steven Price wrote:
 On 20/05/2021 13:05, Catalin Marinas wrote:
> On Mon, May 17, 2021 at 01:32:38PM +0100, Steven Price wrote:
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index e89a5e275e25..4b6c83beb75d 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -1309,6 +1309,65 @@ static int kvm_vm_ioctl_set_device_addr(struct 
>> kvm *kvm,
>>  }
>>  }
>>  
>> +static int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
>> +  struct kvm_arm_copy_mte_tags 
>> *copy_tags)
>> +{
>> +gpa_t guest_ipa = copy_tags->guest_ipa;
>> +size_t length = copy_tags->length;
>> +void __user *tags = copy_tags->addr;
>> +gpa_t gfn;
>> +bool write = !(copy_tags->flags & KVM_ARM_TAGS_FROM_GUEST);
>> +int ret = 0;
>> +
>> +if (copy_tags->reserved[0] || copy_tags->reserved[1])
>> +return -EINVAL;
>> +
>> +if (copy_tags->flags & ~KVM_ARM_TAGS_FROM_GUEST)
>> +return -EINVAL;
>> +
>> +if (length & ~PAGE_MASK || guest_ipa & ~PAGE_MASK)
>> +return -EINVAL;
>> +
>> +gfn = gpa_to_gfn(guest_ipa);
>> +
>> +mutex_lock(&kvm->slots_lock);
>> +
>> +while (length > 0) {
>> +kvm_pfn_t pfn = gfn_to_pfn_prot(kvm, gfn, write, NULL);
>> +void *maddr;
>> +unsigned long num_tags = PAGE_SIZE / MTE_GRANULE_SIZE;
>> +
>> +if (is_error_noslot_pfn(pfn)) {
>> +ret = -EFAULT;
>> +goto out;
>> +}
>> +
>> +maddr = page_address(pfn_to_page(pfn));
>> +
>> +if (!write) {
>> +num_tags = mte_copy_tags_to_user(tags, maddr, 
>> num_tags);
>> +kvm_release_pfn_clean(pfn);
>
> Do we need to check if PG_mte_tagged is set? If the page was not faulted
> into the guest address space but the VMM has the page, does the
> gfn_to_pfn_prot() guarantee that a kvm_set_spte_gfn() was called? If
> not, this may read stale tags.

 Ah, I hadn't thought about that... No I don't believe gfn_to_pfn_prot()
 will fault it into the guest.
>>>
>>> It doesn't indeed. What it does is a get_user_pages() but it's not of
>>> much help since the VMM pte wouldn't be tagged (we would have solved
>>> lots of problems if we required PROT_MTE in the VMM...)
>>
>> Sadly it solves some problems and creates others :(
> 
> I had some (random) thoughts on how to make things simpler, maybe. I
> think most of these races would have been solved if we required PROT_MTE
> in the VMM but this has an impact on the VMM if it wants to use MTE
> itself. If such requirement was in place, all KVM needed to do is check
> PG_mte_tagged.
> 
> So what we actually need is a set_pte_at() in the VMM to clear the tags
> and set PG_mte_tagged. Currently, we only do this if the memory type is
> tagged (PROT_MTE) but it's not strictly necessary.
> 
> As an optimisation for normal programs, we don't want to do this all the
> time but the visible behaviour wouldn't change (well, maybe for ptrace
> slightly). However, it doesn't mean we couldn't for a VMM, with an
> opt-in via prctl(). This would add a MMCF_MTE_TAG_INIT bit (couldn't
> think of a better name) to mm_context_t.flags and set_pte_at() would
> behave as if the pte was tagged without actually mapping the memory in
> user space as tagged (protection flags not changed). Pages that don't
> support tagging are still safe, just some unnecessary ignored tag
> writes. This would need to be set before the mmap() for the guest
> memory.
> 
> If we want finer-grained control we'd have to store this information in
> the vma flags, in addition to VM_MTE (e.g. VM_MTE_TAG_INIT) but without
> affecting the actual memory type. The easiest would be another pte bit,
> though we are short on them. A more intrusive (not too bad) approach is
> to introduce a set_pte_at_vma() and read the flags directly in the arch
> code. In most places where set_pte_at() is called on a user mm, the vma
> is also available.
> 
> Anyway, I'm not saying we go this route, just thinking out loud, get
> some opinions.

Does get_user_pages() actually end up calling set_pte_at() normally? If
not then on the normal user_mem_abort() route although we can easily
check VM_MTE_TAG_INIT there's no obvious place to hook in to ensure that
the pages actually allocated have the PG_mte_tagged flag.

I'm also not sure how well this would work with the MMU notifiers path
in KVM. W

[Bug 1879672] Re: QEMU installer with WHPX support

[Philippe Mathieu-Daudé]

But looking at the latest pipeline:
https://gitlab.com/qemu-project/qemu/-/pipelines/310113928
in particular the cross-win64-system job:
https://gitlab.com/qemu-project/qemu/-/jobs/1296341064

WHPX isn't built anymore:

  Targets and accelerators
  KVM support: NO
  HAX support: YES
  HVF support: NO
 WHPX support: NO
 NVMM support: NO
  Xen support: NO
  TCG support: YES
  TCG backend: native (x86_64)

We likely lost the coverage with commit 93cc0506f6c
("tests/docker: Use Fedora containers for MinGW cross-builds in the gitlab-CI")

Should we open a new issue?

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1879672

Title:
  QEMU installer with WHPX support

Status in QEMU:
  Fix Released

Bug description:
  People often ask the community to add WHPX support to the QEMU installer for 
Windows,
  but it is impossible due to the license limitations of the WHPX SDK.

  The WinHvEmulation.h and WinHvPlatform.h header files needed are "All
  rights reserved".

  However these headers only contain struct definitions and integer constants,
  no functional code in macros or inline functions. See:
  https://www.mail-archive.com/qemu-devel@nongnu.org/msg645815.html
  It is questionable whether the headers alone can be considered copyrightable 
material.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1879672/+subscriptions

Re: Windows fails to boot after rebase to QEMU master

[Claudio Fontana]

On 5/26/21 9:30 PM, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (m...@redhat.com) wrote:
>> On Fri, May 21, 2021 at 11:17:19AM +0200, Siddharth Chandrasekaran wrote:
>>> After a rebase to QEMU master, I am having trouble booting windows VMs.
>>> Git bisect indicates commit f5cc5a5c1686 ("i386: split cpu accelerators
>>> from cpu.c, using AccelCPUClass") to have introduced the issue. I spent
>>> some time looking at into it yesterday without much luck.
>>>
>>> Steps to reproduce:
>>>
>>> $ ./configure --enable-kvm --disable-xen --target-list=x86_64-softmmu 
>>> --enable-debug
>>> $ make -j `nproc`
>>> $ ./build/x86_64-softmmu/qemu-system-x86_64 \
>>> -cpu host,hv_synic,hv_vpindex,hv_time,hv_runtime,hv_stimer,hv_crash 
>>> \
>>> -enable-kvm \
>>> -name test,debug-threads=on \
>>> -smp 1,threads=1,cores=1,sockets=1 \
>>> -m 4G \
>>> -net nic -net user \
>>> -boot d,menu=on \
>>> -usbdevice tablet \
>>> -vnc :3 \
>>> -machine q35,smm=on \
>>> -drive 
>>> if=pflash,format=raw,readonly=on,unit=0,file="../OVMF_CODE.secboot.fd" \
>>> -drive if=pflash,format=raw,unit=1,file="../OVMF_VARS.secboot.fd" \
>>> -global ICH9-LPC.disable_s3=1 \
>>> -global driver=cfi.pflash01,property=secure,value=on \
>>> -cdrom "../Windows_Server_2016_14393.ISO" \
>>> -drive 
>>> file="../win_server_2016.qcow2",format=qcow2,if=none,id=rootfs_drive \
>>> -device ahci,id=ahci \
>>> -device ide-hd,drive=rootfs_drive,bus=ahci.0
>>>
>>> If the issue is not obvious, I'd like some pointers on how to go about
>>> fixing this issue.
>>>
>>> ~ Sid.
>>>
>>
>> At a guess this commit inadvertently changed something in the CPU ID.
>> I'd start by using a linux guest to dump cpuid before and after the
>> change.
> 
> I've not had a chance to do that yet, however I did just end up with a
> bisect of a linux guest failure bisecting to the same patch:
> 
> [dgilbert@dgilbert-t580 qemu]$ git bisect bad
> f5cc5a5c168674f84bf061cdb307c2d25fba5448 is the first bad commit
> commit f5cc5a5c168674f84bf061cdb307c2d25fba5448
> Author: Claudio Fontana 
> Date:   Mon Mar 22 14:27:40 2021 +0100
> 
> i386: split cpu accelerators from cpu.c, using AccelCPUClass
> 
> i386 is the first user of AccelCPUClass, allowing to split
> cpu.c into:
> 
> cpu.ccpuid and common x86 cpu functionality
> host-cpu.c   host x86 cpu functions and "host" cpu type
> kvm/kvm-cpu.cKVM x86 AccelCPUClass
> hvf/hvf-cpu.cHVF x86 AccelCPUClass
> tcg/tcg-cpu.cTCG x86 AccelCPUClass
> 
> 

Paolo, it seems to me that something went wrong in the merge of this commit.

The last version of the series I sent had this comment in the commit message,
as part of a very long series of rebases after review.

[claudio]: Rebased on commit b8184135 ("target/i386: allow modifying TCG 
phys-addr-bits")


While I do not see this comment in the commit posted here. So I suspect that an 
older version of the series was included?

The series is also available as:

https://github.com/hw-claudio/qemu.git "i386_cleanup_9"

Thanks,

Claudio




> The guest crash is:
> [   85.008985][ T1524] BUG: unable to handle page fault for address: 
> 810d9c42
> [   85.012868][ T1524] #PF: supervisor write access in kernel mode
> [   85.012962][ T1524] #PF: error_code(0x0003) - permissions violation
> [   85.013043][ T1524] PGD 2224067 P4D 2224067 PUD 2225063 PMD 10001e1 
> [   85.013180][ T1524] Oops: 0003 [#1] SMP NOPTI
> [   85.013295][ T1524] CPU: 2 PID: 1524 Comm: blogbench Not tainted 
> 5.11.0-rc7 #100
> [   85.013395][ T1524] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
> BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
> [   85.013546][ T1524] RIP: 0010:kvm_kick_cpu+0x22/0x30
> [   85.013630][ T1524] Code: 0f 1f 84 00 00 00 00 00 55 48 63 ff 48 c7 c0 78 
> 11 01 00 48 8b 14 fd c0 36 11 82 48 89 e5 53 31 db 0f b7 0c 02 b8 05 00 00 00 
> <0f> 01 d9 5b 5d c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 53 48 89 fb
> [   85.013852][ T1524] RSP: 0018:c9747c08 EFLAGS: 00010046
> [   85.013951][ T1524] RAX: 0005 RBX:  RCX: 
> 
> [   85.014058][ T1524] RDX: 88807c60 RSI: 0100 RDI: 
> 
> [   85.014153][ T1524] RBP: c9747c10 R08: 88807c72a800 R09: 
> 88807ffd6000
> [   85.014248][ T1524] R10: 0001 R11: 0046 R12: 
> 88807c72a800
> [   85.014343][ T1524] R13:  R14: 888005409940 R15: 
> 88807c72a818
> [   85.014437][ T1524] FS:  7fa2f750a700() GS:88807c70() 
> knlGS:
> [   85.014559][ T1524] CS:  0010 DS:  ES:  CR0: 80050033
> [   85.014644][ T1524] CR2: 810d9c42 CR3: 09016003 CR4: 
> 00370ea0
> [   85.014741][ T1524] DR0:  DR1: 000

Re: [PATCH 4/5] monitor: removed cpustats command

[Dr. David Alan Gilbert]

* Bruno Larsen (billionai) (bruno.lar...@eldorado.org.br) wrote:
> Since ppc was the last architecture to collect these statistics and
> it is currently phasing this collection out, the command that would query
> this information is being removed.
> 
> Suggested-by: Richard Henderson 
> Signed-off-by: Bruno Larsen (billionai) 

Acked-by: Dr. David Alan Gilbert 

> ---
>  hmp-commands-info.hx | 13 -
>  monitor/misc.c   | 11 ---
>  2 files changed, 24 deletions(-)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ab0c7aa5ee..b2347a6aea 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -500,19 +500,6 @@ SRST
>  Show the current VM UUID.
>  ERST
>  
> -{
> -.name   = "cpustats",
> -.args_type  = "",
> -.params = "",
> -.help   = "show CPU statistics",
> -.cmd= hmp_info_cpustats,
> -},
> -
> -SRST
> -  ``info cpustats``
> -Show CPU statistics.
> -ERST
> -
>  #if defined(CONFIG_SLIRP)
>  {
>  .name   = "usernet",
> diff --git a/monitor/misc.c b/monitor/misc.c
> index f3a393ea59..1539e18557 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict 
> *qdict)
>  }
>  }
>  
> -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> -{
> -CPUState *cs = mon_get_cpu(mon);
> -
> -if (!cs) {
> -monitor_printf(mon, "No CPU available\n");
> -return;
> -}
> -cpu_dump_statistics(cs, 0);
> -}
> -
>  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
>  {
>  const char *name = qdict_get_try_str(qdict, "name");
> -- 
> 2.17.1
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 4/5] monitor: removed cpustats command

[Dr. David Alan Gilbert]

* Greg Kurz (gr...@kaod.org) wrote:
> On Wed, 26 May 2021 17:21:03 -0300
> "Bruno Larsen (billionai)"  wrote:
> 
> > Since ppc was the last architecture to collect these statistics and
> > it is currently phasing this collection out, the command that would query
> > this information is being removed.
> > 
> 
> So this is removing an obviously user visible feature. This should be
> mentioned in docs/system/removed-features.rst... but, wait, I don't
> see anything for it in docs/system/deprecated.rst. This is dropping
> a feature without following the usual deprecation policy, i.e.
> marking the feature as deprecated and only remove it 2 QEMU versions
> later. Any justification for that ?

As long as the command really isn't useful any more, I wouldn't object
to that from an HMP point of view.

Dave

> David,
> 
> Unrelated, I saw that you already applied this to ppc-for-6.1 on gitlab
> but the commit title appears to be broken:
> 
> '65;6401;1cmonitor: removed cpustats command
> 
> https://gitlab.com/dgibson/qemu/-/commit/532be563eae6b8ae834ff7e9ebb1428f53569a69
> 
> > Suggested-by: Richard Henderson 
> > Signed-off-by: Bruno Larsen (billionai) 
> > ---
> >  hmp-commands-info.hx | 13 -
> >  monitor/misc.c   | 11 ---
> >  2 files changed, 24 deletions(-)
> > 
> > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > index ab0c7aa5ee..b2347a6aea 100644
> > --- a/hmp-commands-info.hx
> > +++ b/hmp-commands-info.hx
> > @@ -500,19 +500,6 @@ SRST
> >  Show the current VM UUID.
> >  ERST
> >  
> > -{
> > -.name   = "cpustats",
> > -.args_type  = "",
> > -.params = "",
> > -.help   = "show CPU statistics",
> > -.cmd= hmp_info_cpustats,
> > -},
> > -
> > -SRST
> > -  ``info cpustats``
> > -Show CPU statistics.
> > -ERST
> > -
> >  #if defined(CONFIG_SLIRP)
> >  {
> >  .name   = "usernet",
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index f3a393ea59..1539e18557 100644
> > --- a/monitor/misc.c
> > +++ b/monitor/misc.c
> > @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict 
> > *qdict)
> >  }
> >  }
> >  
> > -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> > -{
> > -CPUState *cs = mon_get_cpu(mon);
> > -
> > -if (!cs) {
> > -monitor_printf(mon, "No CPU available\n");
> > -return;
> > -}
> > -cpu_dump_statistics(cs, 0);
> > -}
> > -
> >  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
> >  {
> >  const char *name = qdict_get_try_str(qdict, "name");
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 4/5] monitor: removed cpustats command

[Greg Kurz]

On Thu, 27 May 2021 09:09:55 +0100
"Dr. David Alan Gilbert"  wrote:

> * Greg Kurz (gr...@kaod.org) wrote:
> > On Wed, 26 May 2021 17:21:03 -0300
> > "Bruno Larsen (billionai)"  wrote:
> > 
> > > Since ppc was the last architecture to collect these statistics and
> > > it is currently phasing this collection out, the command that would query
> > > this information is being removed.
> > > 
> > 
> > So this is removing an obviously user visible feature. This should be
> > mentioned in docs/system/removed-features.rst... but, wait, I don't
> > see anything for it in docs/system/deprecated.rst. This is dropping
> > a feature without following the usual deprecation policy, i.e.
> > marking the feature as deprecated and only remove it 2 QEMU versions
> > later. Any justification for that ?
> 
> As long as the command really isn't useful any more, I wouldn't object
> to that from an HMP point of view.
> 

Ok then this should be documented in docs/system/removed-features.rst at
least.

> Dave
> 
> > David,
> > 
> > Unrelated, I saw that you already applied this to ppc-for-6.1 on gitlab
> > but the commit title appears to be broken:
> > 
> > '65;6401;1cmonitor: removed cpustats command
> > 
> > https://gitlab.com/dgibson/qemu/-/commit/532be563eae6b8ae834ff7e9ebb1428f53569a69
> > 
> > > Suggested-by: Richard Henderson 
> > > Signed-off-by: Bruno Larsen (billionai) 
> > > ---
> > >  hmp-commands-info.hx | 13 -
> > >  monitor/misc.c   | 11 ---
> > >  2 files changed, 24 deletions(-)
> > > 
> > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > > index ab0c7aa5ee..b2347a6aea 100644
> > > --- a/hmp-commands-info.hx
> > > +++ b/hmp-commands-info.hx
> > > @@ -500,19 +500,6 @@ SRST
> > >  Show the current VM UUID.
> > >  ERST
> > >  
> > > -{
> > > -.name   = "cpustats",
> > > -.args_type  = "",
> > > -.params = "",
> > > -.help   = "show CPU statistics",
> > > -.cmd= hmp_info_cpustats,
> > > -},
> > > -
> > > -SRST
> > > -  ``info cpustats``
> > > -Show CPU statistics.
> > > -ERST
> > > -
> > >  #if defined(CONFIG_SLIRP)
> > >  {
> > >  .name   = "usernet",
> > > diff --git a/monitor/misc.c b/monitor/misc.c
> > > index f3a393ea59..1539e18557 100644
> > > --- a/monitor/misc.c
> > > +++ b/monitor/misc.c
> > > @@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const 
> > > QDict *qdict)
> > >  }
> > >  }
> > >  
> > > -static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
> > > -{
> > > -CPUState *cs = mon_get_cpu(mon);
> > > -
> > > -if (!cs) {
> > > -monitor_printf(mon, "No CPU available\n");
> > > -return;
> > > -}
> > > -cpu_dump_statistics(cs, 0);
> > > -}
> > > -
> > >  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
> > >  {
> > >  const char *name = qdict_get_try_str(qdict, "name");
> > 

[Bug 1929710] Re: virDomainGetBlockJobInfo fails during swap_volume as disk '$disk' not found in domain

[Lee Yarwood]

I've added the QEMU project directly to this bug to see if anyone can
help us understand what the underlying block job failure is within QEMU
and why it then appears to remove the entire device from the instance
causing libvirt and Nova to fallover.

** Description changed:

  Description
  ===
  
  The error handling around swap_volume is missing the following failure
  when calling virDomainGetBlockJobInfo() after the entire device is
- detached by QEMU (?) after it encounters a job during the block copy job
- that at first pauses and then somehow resumes the job:
+ detached by QEMU (?) after it encounters a failure during the block copy
+ job that at first pauses and then somehow resumes:
  
  https://8a5fc27780098c5ee1bc-
  3ac81d180a9c011938b2cbb0293272f3.ssl.cf5.rackcdn.com/790660/5/gate/nova-
  next/e915ed4/controller/logs/screen-n-cpu.txt
  
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver [None 
req-7cfcd661-29d4-4cc3-bc54-db0e7fed1a6e tempest-TestVolumeSwap-1841575704 
tempest-TestVolumeSwap-1841575704-project-admin] Failure rebasing volume 
/dev/sdb on vdb.: libvirt.libvirtError: invalid argument: disk 'vdb' not found 
in domain
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver Traceback (most recent 
call last):
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver   File 
"/opt/stack/nova/nova/virt/libvirt/driver.py", line 2107, in _swap_volume
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver while not 
dev.is_job_complete():
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver   File 
"/opt/stack/nova/nova/virt/libvirt/guest.py", line 800, in is_job_complete
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver status = 
self.get_job_info()
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver   File 
"/opt/stack/nova/nova/virt/libvirt/guest.py", line 707, in get_job_info
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver status = 
self._guest._domain.blockJobInfo(self._disk, flags=0)
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver   File 
"/usr/local/lib/python3.8/dist-packages/eventlet/tpool.py", line 190, in doit
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver result = 
proxy_call(self._autowrap, f, *args, **kwargs)
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver   File 
"/usr/local/lib/python3.8/dist-packages/eventlet/tpool.py", line 148, in 
proxy_call
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver rv = execute(f, *args, 
**kwargs)
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver   File 
"/usr/local/lib/python3.8/dist-packages/eventlet/tpool.py", line 129, in execute
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver six.reraise(c, e, tb)
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver   File 
"/usr/local/lib/python3.8/dist-packages/six.py", line 719, in reraise
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver raise value
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver   File 
"/usr/local/lib/python3.8/dist-packages/eventlet/tpool.py", line 83, in tworker
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver rv = meth(*args, 
**kwargs)
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver   File 
"/usr/local/lib/python3.8/dist-packages/libvirt.py", line 985, in blockJobInfo
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver raise 
libvirtError('virDomainGetBlockJobInfo() failed')
  May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERROR nova.virt.libvirt.driver libvirt.libvirtError: 
invalid argument: disk 'vdb' not found in domain
- May 26 09:49:47.314813 ubuntu-focal-vexxhost-ca-ymq-1-0024823853 
nova-compute[114649]: ERR

Re: Windows fails to boot after rebase to QEMU master

[Dr. David Alan Gilbert]

* Claudio Fontana (cfont...@suse.de) wrote:
> On 5/26/21 9:30 PM, Dr. David Alan Gilbert wrote:
> > * Michael S. Tsirkin (m...@redhat.com) wrote:
> >> On Fri, May 21, 2021 at 11:17:19AM +0200, Siddharth Chandrasekaran wrote:
> >>> After a rebase to QEMU master, I am having trouble booting windows VMs.
> >>> Git bisect indicates commit f5cc5a5c1686 ("i386: split cpu accelerators
> >>> from cpu.c, using AccelCPUClass") to have introduced the issue. I spent
> >>> some time looking at into it yesterday without much luck.
> >>>
> >>> Steps to reproduce:
> >>>
> >>> $ ./configure --enable-kvm --disable-xen --target-list=x86_64-softmmu 
> >>> --enable-debug
> >>> $ make -j `nproc`
> >>> $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> >>> -cpu 
> >>> host,hv_synic,hv_vpindex,hv_time,hv_runtime,hv_stimer,hv_crash \
> >>> -enable-kvm \
> >>> -name test,debug-threads=on \
> >>> -smp 1,threads=1,cores=1,sockets=1 \
> >>> -m 4G \
> >>> -net nic -net user \
> >>> -boot d,menu=on \
> >>> -usbdevice tablet \
> >>> -vnc :3 \
> >>> -machine q35,smm=on \
> >>> -drive 
> >>> if=pflash,format=raw,readonly=on,unit=0,file="../OVMF_CODE.secboot.fd" \
> >>> -drive if=pflash,format=raw,unit=1,file="../OVMF_VARS.secboot.fd" 
> >>> \
> >>> -global ICH9-LPC.disable_s3=1 \
> >>> -global driver=cfi.pflash01,property=secure,value=on \
> >>> -cdrom "../Windows_Server_2016_14393.ISO" \
> >>> -drive 
> >>> file="../win_server_2016.qcow2",format=qcow2,if=none,id=rootfs_drive \
> >>> -device ahci,id=ahci \
> >>> -device ide-hd,drive=rootfs_drive,bus=ahci.0
> >>>
> >>> If the issue is not obvious, I'd like some pointers on how to go about
> >>> fixing this issue.
> >>>
> >>> ~ Sid.
> >>>
> >>
> >> At a guess this commit inadvertently changed something in the CPU ID.
> >> I'd start by using a linux guest to dump cpuid before and after the
> >> change.
> > 
> > I've not had a chance to do that yet, however I did just end up with a
> > bisect of a linux guest failure bisecting to the same patch:
> > 
> > [dgilbert@dgilbert-t580 qemu]$ git bisect bad
> > f5cc5a5c168674f84bf061cdb307c2d25fba5448 is the first bad commit
> > commit f5cc5a5c168674f84bf061cdb307c2d25fba5448
> > Author: Claudio Fontana 
> > Date:   Mon Mar 22 14:27:40 2021 +0100
> > 
> > i386: split cpu accelerators from cpu.c, using AccelCPUClass
> > 
> > i386 is the first user of AccelCPUClass, allowing to split
> > cpu.c into:
> > 
> > cpu.ccpuid and common x86 cpu functionality
> > host-cpu.c   host x86 cpu functions and "host" cpu type
> > kvm/kvm-cpu.cKVM x86 AccelCPUClass
> > hvf/hvf-cpu.cHVF x86 AccelCPUClass
> > tcg/tcg-cpu.cTCG x86 AccelCPUClass
> > 
> > 
> 
> Paolo, it seems to me that something went wrong in the merge of this commit.
> 
> The last version of the series I sent had this comment in the commit message,
> as part of a very long series of rebases after review.
> 
> [claudio]: Rebased on commit b8184135 ("target/i386: allow modifying TCG 
> phys-addr-bits")
> 
> 
> While I do not see this comment in the commit posted here. So I suspect that 
> an older version of the series was included?

That comment is there in the one merged:
[claudio]:
Rebased on commit b8184135 ("target/i386: allow modifying TCG 
phys-addr-bits")

and I don't see any difference in this commit or the 2 previous ones in
the upstream compared with your i386_cleanup_9 branch.

Dave


> The series is also available as:
> 
> https://github.com/hw-claudio/qemu.git "i386_cleanup_9"
> 
> Thanks,
> 
> Claudio
> 
> 
> 
> 
> > The guest crash is:
> > [   85.008985][ T1524] BUG: unable to handle page fault for address: 
> > 810d9c42
> > [   85.012868][ T1524] #PF: supervisor write access in kernel mode
> > [   85.012962][ T1524] #PF: error_code(0x0003) - permissions violation
> > [   85.013043][ T1524] PGD 2224067 P4D 2224067 PUD 2225063 PMD 10001e1 
> > [   85.013180][ T1524] Oops: 0003 [#1] SMP NOPTI
> > [   85.013295][ T1524] CPU: 2 PID: 1524 Comm: blogbench Not tainted 
> > 5.11.0-rc7 #100
> > [   85.013395][ T1524] Hardware name: QEMU Standard PC (i440FX + PIIX, 
> > 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
> > [   85.013546][ T1524] RIP: 0010:kvm_kick_cpu+0x22/0x30
> > [   85.013630][ T1524] Code: 0f 1f 84 00 00 00 00 00 55 48 63 ff 48 c7 c0 
> > 78 11 01 00 48 8b 14 fd c0 36 11 82 48 89 e5 53 31 db 0f b7 0c 02 b8 05 00 
> > 00 00 <0f> 01 d9 5b 5d c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 53 48 89 fb
> > [   85.013852][ T1524] RSP: 0018:c9747c08 EFLAGS: 00010046
> > [   85.013951][ T1524] RAX: 0005 RBX:  RCX: 
> > 
> > [   85.014058][ T1524] RDX: 88807c60 RSI: 0100 RDI: 
> > 
> > [   85.014153][ T1524] RBP: c9747c10 R08: 88807c72a800 R09: 

Re: [PATCH v3] target/riscv: fix VS interrupts forwarding to HS

[LIU Zhiwei]

On 5/26/21 7:50 PM, Jose Martins wrote:

Hello Zhiwei, thank you for reviewing the patch.

I'll split the patch in a series as you suggest. But first can you
help me understand what the problems are with
riscv_cpu_local_irq_pending?


I think there are two errors in riscv_cpu_local_irq_pending.

1) VS interrupts can't be forwarded to hs-mode rightly . It has
nothing to do with delegate or not in hideleg. The reason is that
VS interrupts are always discarded when V=0 in
riscv_cpu_local_irq_pending.

I don't see why this is the case. The way I see it, VS interrupts are
only discarded for V=0 *iff* they are delegated in mideleg/hideleg.

First I paste the code to ensure we are talking about the same version.

 static int riscv_cpu_local_irq_pending(CPURISCVState *env)

 {

 target_ulong irqs;

 


 target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);

 target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);

 target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE);

 


 target_ulong pending = env->mip & env->mie &

    ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP);

 target_ulong vspending = (env->mip & env->mie &

   (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP));

 


 target_ulong mie    = env->priv < PRV_M ||

   (env->priv == PRV_M && mstatus_mie);

 target_ulong sie    = env->priv < PRV_S ||

   (env->priv == PRV_S && mstatus_sie);

 target_ulong hs_sie = env->priv < PRV_S ||

   (env->priv == PRV_S && hs_mstatus_sie);

 


 if (riscv_cpu_virt_enabled(env)) {

 target_ulong pending_hs_irq = pending & -hs_sie;

 


 if (pending_hs_irq) {

 riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP);

 return ctz64(pending_hs_irq);

 }

 


 pending = vspending;

 }

 


 irqs = (pending & ~env->mideleg & -mie) | (pending &  env->mideleg & -sie);

 


 if (irqs) {

 return ctz64(irqs); /* since non-zero */

 } else {

 return RISCV_EXCP_NONE; /* indicates no pending interrupt */

 }

 }

Only when VS = 0, the variable vspending can transfer to
variable pending. Any interrupt not included in variable pending
is discarded.

That's why I say VS interrupts are always discarded
when V=0 in riscv_cpu_local_irq_pending.

I
actually tested it and I see the correct forwarding of vs-mode
interrupts to hs-mode. I tested it by running in hs-mode with all the
needed interrupt enables set, the interrupts not delegated in hideleg,
and forcing the trigger of the interrupt by writing hvip. But maybe
there are some corner cases I'm not taking into account. Can you
explain this further? Maybe walk me through an example of when this
issue might occur.


2) Use MSTATUS_SIE in mstatus_hs to select pending_hs_irqs.


I mean the second error is to misuse MSATUS_SIE in mstatus_hs to
select pending_hs_irqs.


I don't think you need to go through mstatus_hs to get the correct sie
state.

Agree.

My logic behind this is: env->mstatus will have the vs-level
sie if V=1 and hs-level sie if V=0. Due to the short-circuiting
property of the logic operators the sie variable will only have an
effect on hsie if V=0 and on vsie if V=1. So the value of sie is only
used in the correct context.


The  swap regs funciton has done the right thing.

I think V mode and hideleg/mideleg  make it possible to process

VS interrupt or HS interrupt like other interrupts.

Zhiwei



Again, please correct me if I'm wrong. I might be missing something.

Best,
José

Re: [PATCH 1/1] yank: Unregister function when using TLS migration

[Daniel P . Berrangé]

On Wed, May 26, 2021 at 05:58:58PM -0400, Peter Xu wrote:
> On Wed, May 26, 2021 at 11:21:03PM +0200, Lukas Straub wrote:
> > On Wed, 26 May 2021 16:40:35 -0400
> > Peter Xu  wrote:
> > 
> > > On Wed, May 26, 2021 at 05:05:40PM -0300, Leonardo Bras wrote:
> > > > After yank feature was introduced, whenever migration is started using 
> > > > TLS,
> > > > the following error happens in both source and destination hosts:
> > > > 
> > > > (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> > > > Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> > > > 
> > > > This happens because of a missing yank_unregister_function() when using
> > > > qio-channel-tls.
> > > > 
> > > > Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to perform
> > > > yank_unregister_function() in channel_close() and 
> > > > multifd_load_cleanup().
> > > > 
> > > > Fixes: 50186051f ("Introduce yank feature")
> > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> > > > Signed-off-by: Leonardo Bras   
> > > 
> > > Leo,
> > > 
> > > Thanks for looking into it!
> > > 
> > > So before looking int the fix... I do have a doubt on why we only enable 
> > > yank
> > > on socket typed, as I think tls should also work with 
> > > qio_channel_shutdown().
> > > 
> > > IIUC the confused thing here is we register only for qio-socket, however 
> > > tls
> > > will actually call migration_channel_connect() twice, first with a 
> > > qio-socket,
> > > then with the real tls-socket.  For tls I feel like we have registered 
> > > with the
> > > wrong channel - instead of the wrapper socket ioc, we should register to 
> > > the
> > > final tls ioc?
> > > 
> > > Lukas, is there a reason?
> > > 
> > 
> > Hi,
> > There is no specific reason. Both ways work equally well in preventing
> > qemu from hanging. shutdown() for tls-channel just makes it abort a
> > little sooner (by not attempting to encrypt and send data anymore).
> > 
> > I don't lean either way. I guess registering it on the tls-channel
> > makes is a bit more explicit and clearer.
> 
> Agreed, because IMHO logically the migration code should not be aware of
> internals of IOChannels, e.g., we shouldn't need to know ioc->master is the
> socket ioc of tls ioc to unregister.

I think it is atually better to ignore the TLS channel and *always* yank
on the undering socket IO channel. The yank functionality is intended to
be used in a scenario where we know the channels are broken.  If yank
calls the high level IO channel it is potentially going to try to do a
cleanup shutdown that we know will fail because of the broken network.

Conceptually we just want to yank out the socket channel and leave
everything above that to just deal with the fallout of the terminated
socket.

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 :|

[PATCH 1/1] target/riscv: Fix vsip vsie CSR ops in M and HS mode

[LIU Zhiwei]

When V=1, instructions that normally read or modify a supervisor CSR
shall instead access the corresponding VS CSR. And the VS CSRs can be
accessed as themselves from M-mode or HS-mode.

In M and HS mode, VSIP or VSIE should be written normally instead of
shift by 1.

Signed-off-by: LIU Zhiwei 
---
 target/riscv/csr.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index fe5628fea6..0cce474d3d 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -837,16 +837,16 @@ static RISCVException read_sie(CPURISCVState *env, int 
csrno,
 static RISCVException write_vsie(CPURISCVState *env, int csrno,
  target_ulong val)
 {
-/* Shift the S bits to their VS bit location in mie */
 target_ulong newval = (env->mie & ~VS_MODE_INTERRUPTS) |
-  ((val << 1) & env->hideleg & VS_MODE_INTERRUPTS);
+  (val & env->hideleg & VS_MODE_INTERRUPTS);
 return write_mie(env, CSR_MIE, newval);
 }
 
 static int write_sie(CPURISCVState *env, int csrno, target_ulong val)
 {
 if (riscv_cpu_virt_enabled(env)) {
-write_vsie(env, CSR_VSIE, val);
+/* Shift the S bits to their VS bit location in mie */
+write_vsie(env, CSR_VSIE, val << 1);
 } else {
 target_ulong newval = (env->mie & ~S_MODE_INTERRUPTS) |
   (val & S_MODE_INTERRUPTS);
@@ -950,12 +950,9 @@ static RISCVException rmw_vsip(CPURISCVState *env, int 
csrno,
target_ulong *ret_value,
target_ulong new_value, target_ulong write_mask)
 {
-/* Shift the S bits to their VS bit location in mip */
-int ret = rmw_mip(env, 0, ret_value, new_value << 1,
-  (write_mask << 1) & vsip_writable_mask & env->hideleg);
+int ret = rmw_mip(env, 0, ret_value, new_value,
+  write_mask & vsip_writable_mask & env->hideleg);
 *ret_value &= VS_MODE_INTERRUPTS;
-/* Shift the VS bits to their S bit location in vsip */
-*ret_value >>= 1;
 return ret;
 }
 
@@ -966,7 +963,11 @@ static RISCVException rmw_sip(CPURISCVState *env, int 
csrno,
 int ret;
 
 if (riscv_cpu_virt_enabled(env)) {
-ret = rmw_vsip(env, CSR_VSIP, ret_value, new_value, write_mask);
+/* Shift the S bits to their VS bit location in mip */
+ret = rmw_vsip(env, CSR_VSIP, ret_value, new_value << 1,
+   write_mask << 1);
+/* Shift the VS bits to their S bit location in vsip */
+*ret_value >>= 1;
 } else {
 ret = rmw_mip(env, CSR_MSTATUS, ret_value, new_value,
   write_mask & env->mideleg & sip_writable_mask);
-- 
2.25.1

Re: Windows fails to boot after rebase to QEMU master

[Philippe Mathieu-Daudé]

On 5/27/21 10:31 AM, Dr. David Alan Gilbert wrote:
> * Claudio Fontana (cfont...@suse.de) wrote:
>> On 5/26/21 9:30 PM, Dr. David Alan Gilbert wrote:
>>> * Michael S. Tsirkin (m...@redhat.com) wrote:
 On Fri, May 21, 2021 at 11:17:19AM +0200, Siddharth Chandrasekaran wrote:
> After a rebase to QEMU master, I am having trouble booting windows VMs.
> Git bisect indicates commit f5cc5a5c1686 ("i386: split cpu accelerators
> from cpu.c, using AccelCPUClass") to have introduced the issue. I spent
> some time looking at into it yesterday without much luck.
>
> Steps to reproduce:
>
> $ ./configure --enable-kvm --disable-xen --target-list=x86_64-softmmu 
> --enable-debug
> $ make -j `nproc`
> $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> -cpu 
> host,hv_synic,hv_vpindex,hv_time,hv_runtime,hv_stimer,hv_crash \
> -enable-kvm \
> -name test,debug-threads=on \
> -smp 1,threads=1,cores=1,sockets=1 \
> -m 4G \
> -net nic -net user \
> -boot d,menu=on \
> -usbdevice tablet \
> -vnc :3 \
> -machine q35,smm=on \
> -drive 
> if=pflash,format=raw,readonly=on,unit=0,file="../OVMF_CODE.secboot.fd" \
> -drive if=pflash,format=raw,unit=1,file="../OVMF_VARS.secboot.fd" 
> \
> -global ICH9-LPC.disable_s3=1 \
> -global driver=cfi.pflash01,property=secure,value=on \
> -cdrom "../Windows_Server_2016_14393.ISO" \
> -drive 
> file="../win_server_2016.qcow2",format=qcow2,if=none,id=rootfs_drive \
> -device ahci,id=ahci \
> -device ide-hd,drive=rootfs_drive,bus=ahci.0
>
> If the issue is not obvious, I'd like some pointers on how to go about
> fixing this issue.
>
> ~ Sid.
>

 At a guess this commit inadvertently changed something in the CPU ID.
 I'd start by using a linux guest to dump cpuid before and after the
 change.
>>>
>>> I've not had a chance to do that yet, however I did just end up with a
>>> bisect of a linux guest failure bisecting to the same patch:
>>>
>>> [dgilbert@dgilbert-t580 qemu]$ git bisect bad
>>> f5cc5a5c168674f84bf061cdb307c2d25fba5448 is the first bad commit
>>> commit f5cc5a5c168674f84bf061cdb307c2d25fba5448
>>> Author: Claudio Fontana 
>>> Date:   Mon Mar 22 14:27:40 2021 +0100
>>>
>>> i386: split cpu accelerators from cpu.c, using AccelCPUClass
>>> 
>>> i386 is the first user of AccelCPUClass, allowing to split
>>> cpu.c into:
>>> 
>>> cpu.ccpuid and common x86 cpu functionality
>>> host-cpu.c   host x86 cpu functions and "host" cpu type
>>> kvm/kvm-cpu.cKVM x86 AccelCPUClass
>>> hvf/hvf-cpu.cHVF x86 AccelCPUClass
>>> tcg/tcg-cpu.cTCG x86 AccelCPUClass

Well this is a big commit... I'm not custom to x86 target, and am
having hard time following the cpu host/max change.

Is it working when you use '-cpu max,...' instead of '-cpu host,'?

Re: [PATCH v3 1/3] hw/arm/boot: Abort if set_kernel_args() fails

[Peter Maydell]

On Thu, 20 May 2021 at 06:15, Philippe Mathieu-Daudé  wrote:
>
> If a address_space_write() call fails while calling
> set_kernel_args(), the guest kernel will boot using
> crap data. Avoid that by aborting if this ever occurs.
>
> Signed-off-by: Philippe Mathieu-Daudé 

> @@ -786,10 +811,16 @@ static void do_cpu_reset(void *opaque)
>  cpu_set_pc(cs, info->loader_start);
>
>  if (!have_dtb(info)) {
> +int err;
> +
>  if (old_param) {
> -set_kernel_args_old(info, as);
> +err = set_kernel_args_old(info, as);
>  } else {
> -set_kernel_args(info, as);
> +err = set_kernel_args(info, as);
> +}
> +if (err) {
> +error_report("could not set kernel arguments");
> +exit(1);
>  }
>  }
>  } else {

Since this is in the 'reset' method it's in theory possible that
we might end up exit()ing here in mid-run if the simulation
does a reset and the second reset fails but the one on bootup
didn't. But that seems pretty unlikely, and in any case this
code is all in the "booting Linux, but no DTB" codepath, which
is nowadays a pretty rare case.

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH v3 3/3] hw/core/loader: Warn if we fail to load ROM regions at reset

[Peter Maydell]

On Thu, 20 May 2021 at 06:15, Philippe Mathieu-Daudé  wrote:
>
> If the user provides an ELF file that's been linked to a wrong
> address, we try to load it, fails, and keep going silently.
> Instead,
> Display a warning instead, but keep going to not disrupt users
> accidentally relying on this 'continues-anyway' behaviour.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/core/loader.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index b3c4a654b45..37a2f2c4959 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1147,8 +1147,16 @@ static void rom_reset(void *unused)
>  void *host = memory_region_get_ram_ptr(rom->mr);
>  memcpy(host, rom->data, rom->datasize);
>  } else {
> -address_space_write_rom(rom->as, rom->addr, 
> MEMTXATTRS_UNSPECIFIED,
> -rom->data, rom->datasize);
> +MemTxResult res;
> +
> +res = address_space_write_rom(rom->as, rom->addr,
> +  MEMTXATTRS_UNSPECIFIED,
> +  rom->data, rom->datasize);
> +if (res != MEMTX_OK) {
> +warn_report("rom: unable to write data (file '%s', "
> +"addr=0x" TARGET_FMT_plx ", size=0x%zu)",
> +rom->name, rom->addr, rom->datasize);
> +}
>  }
>  if (rom->isrom) {
>  /* rom needs to be written only once */

address_space_write_rom() as currently implemented cannot ever
fail: it always returns MEMTX_OK. (This is because it completely
ignores attempts to write to devices, and writing to devices backed
by host RAM always works.)

But perhaps this change is reasonable enough as future-proofing
in case we decide to allow address_space_write_rom() to write
rom blobs to devices in future?

-- PMM

Re: Windows fails to boot after rebase to QEMU master

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> On 5/27/21 10:31 AM, Dr. David Alan Gilbert wrote:
> > * Claudio Fontana (cfont...@suse.de) wrote:
> >> On 5/26/21 9:30 PM, Dr. David Alan Gilbert wrote:
> >>> * Michael S. Tsirkin (m...@redhat.com) wrote:
>  On Fri, May 21, 2021 at 11:17:19AM +0200, Siddharth Chandrasekaran wrote:
> > After a rebase to QEMU master, I am having trouble booting windows VMs.
> > Git bisect indicates commit f5cc5a5c1686 ("i386: split cpu accelerators
> > from cpu.c, using AccelCPUClass") to have introduced the issue. I spent
> > some time looking at into it yesterday without much luck.
> >
> > Steps to reproduce:
> >
> > $ ./configure --enable-kvm --disable-xen 
> > --target-list=x86_64-softmmu --enable-debug
> > $ make -j `nproc`
> > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> > -cpu 
> > host,hv_synic,hv_vpindex,hv_time,hv_runtime,hv_stimer,hv_crash \
> > -enable-kvm \
> > -name test,debug-threads=on \
> > -smp 1,threads=1,cores=1,sockets=1 \
> > -m 4G \
> > -net nic -net user \
> > -boot d,menu=on \
> > -usbdevice tablet \
> > -vnc :3 \
> > -machine q35,smm=on \
> > -drive 
> > if=pflash,format=raw,readonly=on,unit=0,file="../OVMF_CODE.secboot.fd" \
> > -drive 
> > if=pflash,format=raw,unit=1,file="../OVMF_VARS.secboot.fd" \
> > -global ICH9-LPC.disable_s3=1 \
> > -global driver=cfi.pflash01,property=secure,value=on \
> > -cdrom "../Windows_Server_2016_14393.ISO" \
> > -drive 
> > file="../win_server_2016.qcow2",format=qcow2,if=none,id=rootfs_drive \
> > -device ahci,id=ahci \
> > -device ide-hd,drive=rootfs_drive,bus=ahci.0
> >
> > If the issue is not obvious, I'd like some pointers on how to go about
> > fixing this issue.
> >
> > ~ Sid.
> >
> 
>  At a guess this commit inadvertently changed something in the CPU ID.
>  I'd start by using a linux guest to dump cpuid before and after the
>  change.
> >>>
> >>> I've not had a chance to do that yet, however I did just end up with a
> >>> bisect of a linux guest failure bisecting to the same patch:
> >>>
> >>> [dgilbert@dgilbert-t580 qemu]$ git bisect bad
> >>> f5cc5a5c168674f84bf061cdb307c2d25fba5448 is the first bad commit
> >>> commit f5cc5a5c168674f84bf061cdb307c2d25fba5448
> >>> Author: Claudio Fontana 
> >>> Date:   Mon Mar 22 14:27:40 2021 +0100
> >>>
> >>> i386: split cpu accelerators from cpu.c, using AccelCPUClass
> >>> 
> >>> i386 is the first user of AccelCPUClass, allowing to split
> >>> cpu.c into:
> >>> 
> >>> cpu.ccpuid and common x86 cpu functionality
> >>> host-cpu.c   host x86 cpu functions and "host" cpu type
> >>> kvm/kvm-cpu.cKVM x86 AccelCPUClass
> >>> hvf/hvf-cpu.cHVF x86 AccelCPUClass
> >>> tcg/tcg-cpu.cTCG x86 AccelCPUClass
> 
> Well this is a big commit... I'm not custom to x86 target, and am
> having hard time following the cpu host/max change.
> 
> Is it working when you use '-cpu max,...' instead of '-cpu host,'?

No; and in fact the cpuid's are almost entirely different with and
without this patch! (both with -cpu host).  It looks like with this
patch we're getting the cpuid for the TCG cpuid rather than the host:

Prior to this patch:
:/# cat /proc/cpuinfo
processor   : 0
vendor_id   : GenuineIntel
cpu family  : 6
model   : 142
model name  : Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
stepping: 10
microcode   : 0xe0
cpu MHz : 2111.998
cache size  : 16384 KB
physical id : 0
siblings: 1
core id : 0
cpu cores   : 1
apicid  : 0
initial apicid  : 0
fpu : yes
fpu_exception   : yes
cpuid level : 22
wp  : yes
flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov 
pat pse36 clflush mmx fxsr sse sse2 ss syscall nx pdpe1gb rdtscp lm constant
_tsc arch_perfmon rep_good nopl xtopology cpuid tsc_known_freq pni pclmulqdq 
vmx ssse3 fma cx16 pdcm pcid sse4_1 sse4_2 x2apic movbe popcnt tsc_deadline_tim
er aes xsave avx f16c rdrand hypervisor lahf_lm abm 3dnowprefetch cpuid_fault 
invpcid_single pti ssbd ibrs ibpb stibp tpr_shadow vnmi flexpriority ept vpid
ept_ad fsgsbase tsc_adjust bmi1 hle avx2 smep bmi2 erms invpcid rtm mpx rdseed 
adx smap clflushopt xsaveopt xsavec xgetbv1 xsaves arat umip md_clear arch_ca
pabilities
vmx flags   : vnmi preemption_timer invvpid ept_x_only ept_ad ept_1gb 
flexpriority tsc_offset vtpr mtf vapic ept vpid unrestricted_guest shadow_vmcs 
pml
bugs: cpu_meltdown spectre_v1 spectre_v2 spec_store_bypass l1tf mds 
swapgs taa srbds
bogomips: 4223.99
clflush size: 64
cache_alignment : 64
address sizes   : 39 bits physical, 48 bits

Re: [PATCH 1/3] hw/usb/Kconfig: Introduce USB_CHIPIDEA symbol

[Peter Maydell]

On Wed, 19 May 2021 at 21:09, Philippe Mathieu-Daudé  wrote:
>
> Currently the TYPE_CHIPIDEA model is only built when the IMX
> machines are built. Since it is not specific to IMX, add its
> symbol to allow other machines to use it.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/arm/Kconfig | 4 
>  hw/usb/Kconfig | 3 +++
>  hw/usb/meson.build | 2 +-
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index b887f6a5b17..585de92f00a 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -385,6 +385,7 @@ config FSL_IMX25
>  select IMX_I2C
>  select WDT_IMX2
>  select DS1338
> +select USB_CHIPIDEA
>
>  config FSL_IMX31
>  bool
> @@ -403,6 +404,7 @@ config FSL_IMX6
>  select IMX_USBPHY
>  select WDT_IMX2
>  select SDHCI
> +select USB_CHIPIDEA
>
>  config ASPEED_SOC
>  bool
> @@ -447,6 +449,7 @@ config FSL_IMX7
>  select PCI_EXPRESS_DESIGNWARE
>  select SDHCI
>  select UNIMP
> +select USB_CHIPIDEA
>
>  config ARM_SMMUV3
>  bool
> @@ -460,6 +463,7 @@ config FSL_IMX6UL
>  select WDT_IMX2
>  select SDHCI
>  select UNIMP
> +select USB_CHIPIDEA
>
>  config MICROBIT
>  bool

Missing "select USB_CHIPIDEA" in "config FSL_IMX31" ?

(Side note: is it worth moving the FSL_* config sections so
they're all next to each other? We seem to have allowed them
to become separated by some other unrelated sections...)

thanks
-- PMM

Re: [PATCH 2/3] hw/arm/Kconfig: Add missing dependency ZYNQ -> USB_CHIPIDEA

[Peter Maydell]

On Wed, 19 May 2021 at 21:09, Philippe Mathieu-Daudé  wrote:
>
> When using a binary built using --without-default-devices we get:
>
>   $ qemu-system-arm -M xilinx-zynq-a9
>   **
>   ERROR:qom/object.c:714:object_new_with_type: assertion failed: (type != 
> NULL)
>   Bail out! ERROR:qom/object.c:714:object_new_with_type: assertion failed: 
> (type != NULL)
>
> Looking at the stack trace:
>
> (gdb) bt
>   #3  0x76e229ff in g_assertion_message_expr () at 
> /lib64/libglib-2.0.so.0
>   #4  0x558d3f01 in object_new_with_type (type=) at 
> qom/object.c:714
>   #5  0x558d3f59 in object_new 
> (typename=typename@entry=0x55ad4545 "usb-chipidea") at qom/object.c:747
>   #6  0x558cd526 in qdev_new (name=name@entry=0x55ad4545 
> "usb-chipidea") at hw/core/qdev.c:153
>   #7  0x55739fc6 in sysbus_create_varargs 
> (name=name@entry=0x55ad4545 "usb-chipidea", addr=addr@entry=3758104576) 
> at hw/core/sysbus.c:234
>   #8  0x55796b57 in sysbus_create_simple (irq=, 
> addr=3758104576, name=0x55ad4545 "usb-chipidea") at 
> include/hw/sysbus.h:104
>   #9  zynq_init (machine=0x55de07a0) at hw/arm/xilinx_zynq.c:254
>   #10 0x5573707c in machine_run_board_init 
> (machine=machine@entry=0x55de07a0) at hw/core/machine.c:1238
>
> When replacing the xlnx,ps7-usb device by the IDEA one in commit
> 616ec12d0fc ("hw/arm/xilinx_zynq: Fix USB port instantiation") we
> forgot to add the Kconfig dependency on the new device. Do it now.
>
> Fixes: 616ec12d0fc ("hw/arm/xilinx_zynq: Fix USB port instantiation")
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 3/3] hw/arm/Kconfig: Add missing dependency IMX_USBPHY -> FSL_IMX6UL

[Peter Maydell]

On Wed, 19 May 2021 at 21:10, Philippe Mathieu-Daudé  wrote:
>
> We added IMX_USBPHY in commit 0701a5efa01 ("hw/usb: Add basic i.MX
> USB Phy support") and had the FSL_IMX6 machine select it, however
> this machine did not use the IMX_USBPHY until commit 49cd55789bb
> ("hw/arm/fsl-imx6: Wire up USB controllers"). Commit 17372bd812d
> ("hw/arm/fsl-imx6ul: Wire up USB controllers") added the similar
> dependency but forgot to select the Kconfig symbol. Do it now to
> solve when building using --without-default-devices:
>
>   $ qemu-system-arm -M mcimx6ul-evk
>   qemu-system-arm: missing object type 'imx.usbphy'
>   Aborted (core dumped)
>
> Fixes: 17372bd812d ("hw/arm/fsl-imx6ul: Wire up USB controllers")
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/arm/Kconfig | 1 +

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 1/2] hw/arm: add quanta-gbs-bmc machine

[Peter Maydell]

On Tue, 18 May 2021 at 20:54, Patrick Venture  wrote:
>
> Adds initial quanta-gbs-bmc machine support.
>
> Tested: Boots to userspace.
> Signed-off-by: Patrick Venture 
> Reviewed-by: Brandon Kim 
> Reviewed-by: Hao Wu 
> ---
>  hw/arm/npcm7xx_boards.c | 33 +
>  1 file changed, 33 insertions(+)
>
> diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
> index d4553e3786..34a214fe79 100644
> --- a/hw/arm/npcm7xx_boards.c
> +++ b/hw/arm/npcm7xx_boards.c
> @@ -29,6 +29,7 @@
>
>  #define NPCM750_EVB_POWER_ON_STRAPS 0x1ff7
>  #define QUANTA_GSJ_POWER_ON_STRAPS 0x1fff
> +#define QUANTA_GBS_POWER_ON_STRAPS 0x17ff /* TODO: Get real values. */

Any chance you could fix this TODO ? If it's not fixed now before
the code goes in it seems unlikely that it'll ever be fixed -- you're
probably in the best position to know the right value...

>  static const char npcm7xx_default_bootrom[] = "npcm7xx_bootrom.bin";
>
> @@ -268,6 +269,22 @@ static void quanta_gsj_init(MachineState *machine)
>  npcm7xx_load_kernel(machine, soc);
>  }
>
> +static void quanta_gbs_init(MachineState *machine)
> +{
> +NPCM7xxState *soc;
> +
> +soc = npcm7xx_create_soc(machine, QUANTA_GBS_POWER_ON_STRAPS);
> +npcm7xx_connect_dram(soc, machine->ram);
> +qdev_realize(DEVICE(soc), NULL, &error_fatal);
> +
> +npcm7xx_load_bootrom(machine, soc);
> +
> +npcm7xx_connect_flash(&soc->fiu[0], 0, "mx66u51235f",
> +  drive_get(IF_MTD, 0, 0));
> +
> +npcm7xx_load_kernel(machine, soc);
> +}
> +
>  static void npcm7xx_set_soc_type(NPCM7xxMachineClass *nmc, const char *type)
>  {
>  NPCM7xxClass *sc = NPCM7XX_CLASS(object_class_by_name(type));
> @@ -316,6 +333,18 @@ static void gsj_machine_class_init(ObjectClass *oc, void 
> *data)
>  mc->default_ram_size = 512 * MiB;
>  };
>
> +static void gbs_bmc_machine_class_init(ObjectClass *oc, void *data)
> +{
> +NPCM7xxMachineClass *nmc = NPCM7XX_MACHINE_CLASS(oc);
> +MachineClass *mc = MACHINE_CLASS(oc);
> +
> +npcm7xx_set_soc_type(nmc, TYPE_NPCM730);
> +
> +mc->desc = "Quanta GBS (Cortex A9)";

"Cortex-A9", with a hyphen.


thanks
-- PMM

Re: Windows fails to boot after rebase to QEMU master

[Claudio Fontana]

On 5/27/21 11:15 AM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
>> On 5/27/21 10:31 AM, Dr. David Alan Gilbert wrote:
>>> * Claudio Fontana (cfont...@suse.de) wrote:
 On 5/26/21 9:30 PM, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (m...@redhat.com) wrote:
>> On Fri, May 21, 2021 at 11:17:19AM +0200, Siddharth Chandrasekaran wrote:
>>> After a rebase to QEMU master, I am having trouble booting windows VMs.
>>> Git bisect indicates commit f5cc5a5c1686 ("i386: split cpu accelerators
>>> from cpu.c, using AccelCPUClass") to have introduced the issue. I spent
>>> some time looking at into it yesterday without much luck.
>>>
>>> Steps to reproduce:
>>>
>>> $ ./configure --enable-kvm --disable-xen 
>>> --target-list=x86_64-softmmu --enable-debug
>>> $ make -j `nproc`
>>> $ ./build/x86_64-softmmu/qemu-system-x86_64 \
>>> -cpu 
>>> host,hv_synic,hv_vpindex,hv_time,hv_runtime,hv_stimer,hv_crash \
>>> -enable-kvm \
>>> -name test,debug-threads=on \
>>> -smp 1,threads=1,cores=1,sockets=1 \
>>> -m 4G \
>>> -net nic -net user \
>>> -boot d,menu=on \
>>> -usbdevice tablet \
>>> -vnc :3 \
>>> -machine q35,smm=on \
>>> -drive 
>>> if=pflash,format=raw,readonly=on,unit=0,file="../OVMF_CODE.secboot.fd" \
>>> -drive 
>>> if=pflash,format=raw,unit=1,file="../OVMF_VARS.secboot.fd" \
>>> -global ICH9-LPC.disable_s3=1 \
>>> -global driver=cfi.pflash01,property=secure,value=on \
>>> -cdrom "../Windows_Server_2016_14393.ISO" \
>>> -drive 
>>> file="../win_server_2016.qcow2",format=qcow2,if=none,id=rootfs_drive \
>>> -device ahci,id=ahci \
>>> -device ide-hd,drive=rootfs_drive,bus=ahci.0
>>>
>>> If the issue is not obvious, I'd like some pointers on how to go about
>>> fixing this issue.
>>>
>>> ~ Sid.
>>>
>>
>> At a guess this commit inadvertently changed something in the CPU ID.
>> I'd start by using a linux guest to dump cpuid before and after the
>> change.
>
> I've not had a chance to do that yet, however I did just end up with a
> bisect of a linux guest failure bisecting to the same patch:
>
> [dgilbert@dgilbert-t580 qemu]$ git bisect bad
> f5cc5a5c168674f84bf061cdb307c2d25fba5448 is the first bad commit
> commit f5cc5a5c168674f84bf061cdb307c2d25fba5448
> Author: Claudio Fontana 
> Date:   Mon Mar 22 14:27:40 2021 +0100
>
> i386: split cpu accelerators from cpu.c, using AccelCPUClass
> 
> i386 is the first user of AccelCPUClass, allowing to split
> cpu.c into:
> 
> cpu.ccpuid and common x86 cpu functionality
> host-cpu.c   host x86 cpu functions and "host" cpu type
> kvm/kvm-cpu.cKVM x86 AccelCPUClass
> hvf/hvf-cpu.cHVF x86 AccelCPUClass
> tcg/tcg-cpu.cTCG x86 AccelCPUClass
>>
>> Well this is a big commit... I'm not custom to x86 target, and am
>> having hard time following the cpu host/max change.
>>
>> Is it working when you use '-cpu max,...' instead of '-cpu host,'?
> 
> No; and in fact the cpuid's are almost entirely different with and
> without this patch! (both with -cpu host).  It looks like with this
> patch we're getting the cpuid for the TCG cpuid rather than the host:
> 
> Prior to this patch:
> :/# cat /proc/cpuinfo
> processor   : 0
> vendor_id   : GenuineIntel
> cpu family  : 6
> model   : 142
> model name  : Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
> stepping: 10
> microcode   : 0xe0
> cpu MHz : 2111.998
> cache size  : 16384 KB
> physical id : 0
> siblings: 1
> core id : 0
> cpu cores   : 1
> apicid  : 0
> initial apicid  : 0
> fpu : yes
> fpu_exception   : yes
> cpuid level : 22
> wp  : yes
> flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca 
> cmov pat pse36 clflush mmx fxsr sse sse2 ss syscall nx pdpe1gb rdtscp lm 
> constant
> _tsc arch_perfmon rep_good nopl xtopology cpuid tsc_known_freq pni pclmulqdq 
> vmx ssse3 fma cx16 pdcm pcid sse4_1 sse4_2 x2apic movbe popcnt 
> tsc_deadline_tim
> er aes xsave avx f16c rdrand hypervisor lahf_lm abm 3dnowprefetch cpuid_fault 
> invpcid_single pti ssbd ibrs ibpb stibp tpr_shadow vnmi flexpriority ept vpid
> ept_ad fsgsbase tsc_adjust bmi1 hle avx2 smep bmi2 erms invpcid rtm mpx 
> rdseed adx smap clflushopt xsaveopt xsavec xgetbv1 xsaves arat umip md_clear 
> arch_ca
> pabilities
> vmx flags   : vnmi preemption_timer invvpid ept_x_only ept_ad ept_1gb 
> flexpriority tsc_offset vtpr mtf vapic ept vpid unrestricted_guest 
> shadow_vmcs pml
> bugs: cpu_meltdown spectre_v1 spectre_v2 spec_store_b

[PATCH] arm: Consistently use "Cortex-Axx", not "Cortex Axx"

[Peter Maydell]

The official punctuation for Arm CPU names uses a hyphen, like
"Cortex-A9". We mostly follow this, but in a few places usage
without the hyphen has crept in. Fix those so we consistently
use the same way of writing the CPU name.

This commit was created with:
  git grep -z -l 'Cortex ' | xargs -0 sed -i 's/Cortex /Cortex-/'

Signed-off-by: Peter Maydell 
---
 docs/system/arm/aspeed.rst| 4 ++--
 docs/system/arm/nuvoton.rst   | 6 +++---
 docs/system/arm/sabrelite.rst | 2 +-
 include/hw/arm/allwinner-h3.h | 2 +-
 hw/arm/aspeed.c   | 6 +++---
 hw/arm/mcimx6ul-evk.c | 2 +-
 hw/arm/mcimx7d-sabre.c| 2 +-
 hw/arm/npcm7xx_boards.c   | 4 ++--
 hw/arm/sabrelite.c| 2 +-
 hw/misc/npcm7xx_clk.c | 2 +-
 10 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index a1911f94031..57ee2bd94fc 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -5,7 +5,7 @@ The QEMU Aspeed machines model BMCs of various OpenPOWER 
systems and
 Aspeed evaluation boards. They are based on different releases of the
 Aspeed SoC : the AST2400 integrating an ARM926EJ-S CPU (400MHz), the
 AST2500 with an ARM1176JZS CPU (800MHz) and more recently the AST2600
-with dual cores ARM Cortex A7 CPUs (1.2GHz).
+with dual cores ARM Cortex-A7 CPUs (1.2GHz).
 
 The SoC comes with RAM, Gigabit ethernet, USB, SD/MMC, USB, SPI, I2C,
 etc.
@@ -24,7 +24,7 @@ AST2500 SoC based machines :
 
 AST2600 SoC based machines :
 
-- ``ast2600-evb``  Aspeed AST2600 Evaluation board (Cortex A7)
+- ``ast2600-evb``  Aspeed AST2600 Evaluation board (Cortex-A7)
 - ``tacoma-bmc``   OpenPOWER Witherspoon POWER9 AST2600 BMC
 
 Supported devices
diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst
index d3cf2d9cd7e..ca011bd4797 100644
--- a/docs/system/arm/nuvoton.rst
+++ b/docs/system/arm/nuvoton.rst
@@ -3,19 +3,19 @@ Nuvoton iBMC boards (``npcm750-evb``, ``quanta-gsj``)
 
 The `Nuvoton iBMC`_ chips (NPCM7xx) are a family of ARM-based SoCs that are
 designed to be used as Baseboard Management Controllers (BMCs) in various
-servers. They all feature one or two ARM Cortex A9 CPU cores, as well as an
+servers. They all feature one or two ARM Cortex-A9 CPU cores, as well as an
 assortment of peripherals targeted for either Enterprise or Data Center /
 Hyperscale applications. The former is a superset of the latter, so NPCM750 has
 all the peripherals of NPCM730 and more.
 
 .. _Nuvoton iBMC: https://www.nuvoton.com/products/cloud-computing/ibmc/
 
-The NPCM750 SoC has two Cortex A9 cores and is targeted for the Enterprise
+The NPCM750 SoC has two Cortex-A9 cores and is targeted for the Enterprise
 segment. The following machines are based on this chip :
 
 - ``npcm750-evb``   Nuvoton NPCM750 Evaluation board
 
-The NPCM730 SoC has two Cortex A9 cores and is targeted for Data Center and
+The NPCM730 SoC has two Cortex-A9 cores and is targeted for Data Center and
 Hyperscale applications. The following machines are based on this chip :
 
 - ``quanta-gsj``Quanta GSJ server BMC
diff --git a/docs/system/arm/sabrelite.rst b/docs/system/arm/sabrelite.rst
index 71713310e3a..4ccb0560afe 100644
--- a/docs/system/arm/sabrelite.rst
+++ b/docs/system/arm/sabrelite.rst
@@ -10,7 +10,7 @@ Supported devices
 
 The SABRE Lite machine supports the following devices:
 
- * Up to 4 Cortex A9 cores
+ * Up to 4 Cortex-A9 cores
  * Generic Interrupt Controller
  * 1 Clock Controller Module
  * 1 System Reset Controller
diff --git a/include/hw/arm/allwinner-h3.h b/include/hw/arm/allwinner-h3.h
index cc308a5d2c9..63025fb27c8 100644
--- a/include/hw/arm/allwinner-h3.h
+++ b/include/hw/arm/allwinner-h3.h
@@ -18,7 +18,7 @@
  */
 
 /*
- * The Allwinner H3 is a System on Chip containing four ARM Cortex A7
+ * The Allwinner H3 is a System on Chip containing four ARM Cortex-A7
  * processor cores. Features and specifications include DDR2/DDR3 memory,
  * SD/MMC storage cards, 10/100/1000Mbit Ethernet, USB 2.0, HDMI and
  * various I/O modules.
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 3fe6c55744f..0eafc791540 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -947,7 +947,7 @@ static void 
aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
 MachineClass *mc = MACHINE_CLASS(oc);
 AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
 
-mc->desc   = "Aspeed AST2600 EVB (Cortex A7)";
+mc->desc   = "Aspeed AST2600 EVB (Cortex-A7)";
 amc->soc_name  = "ast2600-a1";
 amc->hw_strap1 = AST2600_EVB_HW_STRAP1;
 amc->hw_strap2 = AST2600_EVB_HW_STRAP2;
@@ -966,7 +966,7 @@ static void aspeed_machine_tacoma_class_init(ObjectClass 
*oc, void *data)
 MachineClass *mc = MACHINE_CLASS(oc);
 AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
 
-mc->desc   = "OpenPOWER Tacoma BMC (Cortex A7)";
+mc->desc   = "OpenPOWER Tacoma BMC (Cortex-A7)";
 amc->soc_n

Re: [PATCH] arm: Consistently use "Cortex-Axx", not "Cortex Axx"

[Philippe Mathieu-Daudé]

On 5/27/21 11:51 AM, Peter Maydell wrote:
> The official punctuation for Arm CPU names uses a hyphen, like
> "Cortex-A9". We mostly follow this, but in a few places usage
> without the hyphen has crept in. Fix those so we consistently
> use the same way of writing the CPU name.
> 
> This commit was created with:
>   git grep -z -l 'Cortex ' | xargs -0 sed -i 's/Cortex /Cortex-/'
> 
> Signed-off-by: Peter Maydell 
> ---
>  docs/system/arm/aspeed.rst| 4 ++--
>  docs/system/arm/nuvoton.rst   | 6 +++---
>  docs/system/arm/sabrelite.rst | 2 +-
>  include/hw/arm/allwinner-h3.h | 2 +-
>  hw/arm/aspeed.c   | 6 +++---
>  hw/arm/mcimx6ul-evk.c | 2 +-
>  hw/arm/mcimx7d-sabre.c| 2 +-
>  hw/arm/npcm7xx_boards.c   | 4 ++--
>  hw/arm/sabrelite.c| 2 +-
>  hw/misc/npcm7xx_clk.c | 2 +-
>  10 files changed, 16 insertions(+), 16 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v8 01/19] hvf: Move assert_hvf_ok() into common directory

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:35PM +0200, Alexander Graf wrote:
> Until now, Hypervisor.framework has only been available on x86_64 systems.
> With Apple Silicon shipping now, it extends its reach to aarch64. To
> prepare for support for multiple architectures, let's start moving common
> code out into its own accel directory.
> 
> This patch moves assert_hvf_ok() and introduces generic build infrastructure.
> 
> Signed-off-by: Alexander Graf 
> ---
>  MAINTAINERS  |  8 +++
>  accel/hvf/hvf-all.c  | 47 
>  accel/hvf/meson.build|  6 +
>  accel/meson.build|  1 +
>  include/sysemu/hvf_int.h | 18 +++
>  target/i386/hvf/hvf.c| 33 +---
>  6 files changed, 81 insertions(+), 32 deletions(-)
>  create mode 100644 accel/hvf/hvf-all.c
>  create mode 100644 accel/hvf/meson.build
>  create mode 100644 include/sysemu/hvf_int.h

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 04/19] hvf: Move hvf internal definitions into common header

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:38PM +0200, Alexander Graf wrote:
> Until now, Hypervisor.framework has only been available on x86_64 systems.
> With Apple Silicon shipping now, it extends its reach to aarch64. To
> prepare for support for multiple architectures, let's start moving common
> code out into its own accel directory.
> 
> This patch moves a few internal struct and constant defines over.
> 
> Signed-off-by: Alexander Graf 
> ---
>  include/sysemu/hvf_int.h   | 30 ++
>  target/i386/hvf/hvf-i386.h | 31 +--
>  2 files changed, 31 insertions(+), 30 deletions(-)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 05/19] hvf: Make hvf_set_phys_mem() static

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:39PM +0200, Alexander Graf wrote:
> The hvf_set_phys_mem() function is only called within the same file.
> Make it static.
> 
> Signed-off-by: Alexander Graf 
> ---
>  accel/hvf/hvf-accel-ops.c | 2 +-
>  include/sysemu/hvf_int.h  | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 02/19] hvf: Move vcpu thread functions into common directory

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:36PM +0200, Alexander Graf wrote:
> Until now, Hypervisor.framework has only been available on x86_64 systems.
> With Apple Silicon shipping now, it extends its reach to aarch64. To
> prepare for support for multiple architectures, let's start moving common
> code out into its own accel directory.
> 
> This patch moves the vCPU thread loop over.
> 
> Signed-off-by: Alexander Graf 
> ---
>  {target/i386 => accel}/hvf/hvf-accel-ops.c | 0
>  {target/i386 => accel}/hvf/hvf-accel-ops.h | 0
>  accel/hvf/meson.build  | 1 +
>  target/i386/hvf/meson.build| 1 -
>  target/i386/hvf/x86hvf.c   | 2 +-
>  5 files changed, 2 insertions(+), 2 deletions(-)
>  rename {target/i386 => accel}/hvf/hvf-accel-ops.c (100%)
>  rename {target/i386 => accel}/hvf/hvf-accel-ops.h (100%)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 07/19] hvf: Split out common code on vcpu init and destroy

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:41PM +0200, Alexander Graf wrote:
> Until now, Hypervisor.framework has only been available on x86_64 systems.
> With Apple Silicon shipping now, it extends its reach to aarch64. To
> prepare for support for multiple architectures, let's start moving common
> code out into its own accel directory.
> 
> This patch splits the vcpu init and destroy functions into a generic and
> an architecture specific portion. This also allows us to move the generic
> functions into the generic hvf code, removing exported functions.
> 
> Signed-off-by: Alexander Graf 
> ---
>  accel/hvf/hvf-accel-ops.c | 30 ++
>  accel/hvf/hvf-accel-ops.h |  2 --
>  include/sysemu/hvf_int.h  |  2 ++
>  target/i386/hvf/hvf.c | 23 ++-
>  4 files changed, 34 insertions(+), 23 deletions(-)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 10/19] hvf: Remove hvf-accel-ops.h

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:44PM +0200, Alexander Graf wrote:
> We can move the definition of hvf_vcpu_exec() into our internal
> hvf header, obsoleting the need for hvf-accel-ops.h.
> 
> Signed-off-by: Alexander Graf 
> ---
>  accel/hvf/hvf-accel-ops.c |  2 --
>  accel/hvf/hvf-accel-ops.h | 17 -
>  include/sysemu/hvf_int.h  |  1 +
>  target/i386/hvf/hvf.c |  2 --
>  4 files changed, 1 insertion(+), 21 deletions(-)
>  delete mode 100644 accel/hvf/hvf-accel-ops.h

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 06/19] hvf: Remove use of hv_uvaddr_t and hv_gpaddr_t

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:40PM +0200, Alexander Graf wrote:
> The ARM version of Hypervisor.framework no longer defines these two
> types, so let's just revert to standard ones.
> 
> Signed-off-by: Alexander Graf 
> ---
>  accel/hvf/hvf-accel-ops.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 03/19] hvf: Move cpu functions into common directory

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:37PM +0200, Alexander Graf wrote:
> Until now, Hypervisor.framework has only been available on x86_64 systems.
> With Apple Silicon shipping now, it extends its reach to aarch64. To
> prepare for support for multiple architectures, let's start moving common
> code out into its own accel directory.
> 
> This patch moves CPU and memory operations over. While at it, make sure
> the code is consumable on non-i386 systems.
> 
> Signed-off-by: Alexander Graf 
> ---
>  accel/hvf/hvf-accel-ops.c  | 308 -
>  include/sysemu/hvf_int.h   |   4 +
>  target/i386/hvf/hvf-i386.h |   2 -
>  target/i386/hvf/hvf.c  | 302 
>  target/i386/hvf/x86hvf.h   |   2 -
>  5 files changed, 311 insertions(+), 307 deletions(-)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 09/19] hvf: Make synchronize functions static

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:43PM +0200, Alexander Graf wrote:
> The hvf accel synchronize functions are only used as input for local
> callback functions, so we can make them static.
> 
> Signed-off-by: Alexander Graf 
> ---
>  accel/hvf/hvf-accel-ops.c | 6 +++---
>  accel/hvf/hvf-accel-ops.h | 3 ---
>  2 files changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 11/19] hvf: Introduce hvf vcpu struct

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:45PM +0200, Alexander Graf wrote:
> We will need more than a single field for hvf going forward. To keep
> the global vcpu struct uncluttered, let's allocate a special hvf vcpu
> struct, similar to how hax does it.
> 
> Signed-off-by: Alexander Graf 
> Reviewed-by: Roman Bolshakov 
> Tested-by: Roman Bolshakov 
> Reviewed-by: Alex Bennée 
> 
> ---
> 
> v4 -> v5:
> 
>   - Use g_free() on destroy
> ---
>  accel/hvf/hvf-accel-ops.c   |   8 +-
>  include/hw/core/cpu.h   |   3 +-
>  include/sysemu/hvf_int.h|   4 +
>  target/i386/hvf/hvf.c   | 104 +-
>  target/i386/hvf/vmx.h   |  24 +++--
>  target/i386/hvf/x86.c   |  28 ++---
>  target/i386/hvf/x86_descr.c |  26 ++---
>  target/i386/hvf/x86_emu.c   |  62 +--
>  target/i386/hvf/x86_mmu.c   |   4 +-
>  target/i386/hvf/x86_task.c  |  12 +--
>  target/i386/hvf/x86hvf.c| 210 ++--
>  11 files changed, 248 insertions(+), 237 deletions(-)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH 0/2] vvfat: fix two crashes.

[Kevin Wolf]

Am 24.05.2021 um 12:12 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi!
> 
> As reported by Programmingkid, command 
> 
> qemu-system-ppc -usb -device usb-storage,drive=fat16 -drive 
> file=fat:rw:fat-type=16:"",id=fat16,format=raw,if=none
> 
> crashes.
> 
> I tested it with qemu-system-x86_64 and it reproduces for me. I even
> kept "" as is :).
> 
> So, here are two fixes.

Thanks, applied to the block branch.

Kevin

Re: [PATCH v8 08/19] hvf: Use cpu_synchronize_state()

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:42PM +0200, Alexander Graf wrote:
> There is no reason to call the hvf specific hvf_cpu_synchronize_state()
> when we can just use the generic cpu_synchronize_state() instead. This
> allows us to have less dependency on internal function definitions and
> allows us to make hvf_cpu_synchronize_state() static.
> 
> Signed-off-by: Alexander Graf 
> ---
>  accel/hvf/hvf-accel-ops.c | 2 +-
>  accel/hvf/hvf-accel-ops.h | 1 -
>  target/i386/hvf/x86hvf.c  | 9 -
>  3 files changed, 5 insertions(+), 7 deletions(-)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH] linux-user: make process_pending_signals thread-safe

[Hamza Mahfooz]

On Thu, May 27 2021 at 11:16:56 AM +0100, Peter Maydell 
 wrote:

If we do want to change from sigprocmask() to pthread_sigmask(), we
should be consistent about doing that, not just change this call 
only.)
On that note, do you think it would worthwhile to have a Coccinelle 
script replace all instances of sigprocmask with pthread_sigmask (in 
linux-user, of course)?

Re: [PATCH] linux-user: make process_pending_signals thread-safe

[Peter Maydell]

On Mon, 24 May 2021 at 03:48, Hamza Mahfooz  wrote:
>
> Use pthread_sigmask instead of sigprocmask inside process_pending_signals
> to ensure that race conditions aren't possible.
>
> Signed-off-by: Hamza Mahfooz 
> ---
>  linux-user/signal.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 7eecec46c4..81ff753c01 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1005,9 +1005,8 @@ void process_pending_signals(CPUArchState *cpu_env)
>  sigset_t *blocked_set;
>
>  while (qatomic_read(&ts->signal_pending)) {
> -/* FIXME: This is not threadsafe.  */
>  sigfillset(&set);
> -sigprocmask(SIG_SETMASK, &set, 0);
> +pthread_sigmask(SIG_SETMASK, &set, 0);

We use sigprocmask() in plenty more places than this one in linux-user,
so it seems unlikely that the FIXME comment is simply noting that we've
used sigprocmask() rather than pthread_sigmask(). Indeed, the comment
dates back to before this function called sigprocmask() at all (the
sigprocmask() call was added in commit 3d3efba020da which just preserves
the FIXME comment.

So I think we cannot remove this FIXME comment like this: we need to
more carefully analyze the code/dig through the history to identify
what race condition/threadsafety issue the comment is attempting to
point out, because it's not "we didn't use pthread_sigmask()".

(As it happens, on Linux/glibc sigprocmask() is implemented as simply
calling pthread_sigmask():
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/sigprocmask.c;h=9dfd8076d12aff9014fa40f7e93111760a1a8bad;hb=HEAD
If we do want to change from sigprocmask() to pthread_sigmask(), we
should be consistent about doing that, not just change this call only.)

thanks
-- PMM

Re: [PATCH] linux-user: make process_pending_signals thread-safe

[Peter Maydell]

On Thu, 27 May 2021 at 11:37, Hamza Mahfooz  wrote:
>
>
>
> On Thu, May 27 2021 at 11:16:56 AM +0100, Peter Maydell
>  wrote:
> > If we do want to change from sigprocmask() to pthread_sigmask(), we
> > should be consistent about doing that, not just change this call
> > only.)
> On that note, do you think it would worthwhile to have a Coccinelle
> script replace all instances of sigprocmask with pthread_sigmask (in
> linux-user, of course)?

What issue are we trying to fix by making this change ?

There are only 7 calls so a coccinelle script seems like overkill.

thanks
-- PMM

Re: [PATCH v8 12/19] hvf: Simplify post reset/init/loadvm hooks

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:46PM +0200, Alexander Graf wrote:
> The hooks we have that call us after reset, init and loadvm really all
> just want to say "The reference of all register state is in the QEMU
> vcpu struct, please push it".
> 
> We already have a working pushing mechanism though called cpu->vcpu_dirty,
> so we can just reuse that for all of the above, syncing state properly the
> next time we actually execute a vCPU.
> 
> This fixes PSCI resets on ARM, as they modify CPU state even after the
> post init call has completed, but before we execute the vCPU again.
> 
> To also make the scheme work for x86, we have to make sure we don't
> move stale eflags into our env when the vcpu state is dirty.
> 
> Signed-off-by: Alexander Graf 
> Reviewed-by: Roman Bolshakov 
> Tested-by: Roman Bolshakov 
> ---
>  accel/hvf/hvf-accel-ops.c | 27 +++
>  target/i386/hvf/x86hvf.c  |  5 -
>  2 files changed, 11 insertions(+), 21 deletions(-)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 13/19] hvf: Add Apple Silicon support

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:47PM +0200, Alexander Graf wrote:
> With Apple Silicon available to the masses, it's a good time to add support
> for driving its virtualization extensions from QEMU.
> 
> This patch adds all necessary architecture specific code to get basic VMs
> working. It's still pretty raw, but definitely functional.
> 
> Known limitations:
> 
>   - Vtimer acknowledgement is hacky
>   - Should implement more sysregs and fault on invalid ones then
>   - WFI handling is missing, need to marry it with vtimer
> 
> Signed-off-by: Alexander Graf 
> Reviewed-by: Roman Bolshakov 
> 
> ---
> 
> v1 -> v2:
> 
>   - Merge vcpu kick function patch
>   - Implement WFI handling (allows vCPUs to sleep)
>   - Synchronize system registers (fixes OVMF crashes and reboot)
>   - Don't always call cpu_synchronize_state()
>   - Use more fine grained iothread locking
>   - Populate aa64mmfr0 from hardware
> 
> v2 -> v3:
> 
>   - Advance PC on SMC
>   - Use cp list interface for sysreg syncs
>   - Do not set current_cpu
>   - Fix sysreg isread mask
>   - Move sysreg handling to functions
>   - Remove WFI logic again
>   - Revert to global iothread locking
>   - Use Hypervisor.h on arm, hv.h does not contain aarch64 definitions
> 
> v3 -> v4:
> 
>   - No longer include Hypervisor.h
> 
> v5 -> v6:
> 
>   - Swap sysreg definition order. This way we're in line with asm outputs.
> 
> v6 -> v7:
> 
>   - Remove osdep.h include from hvf_int.h
>   - Synchronize SIMD registers as well
>   - Prepend 0x for hex values
>   - Convert DPRINTF to trace points
>   - Use main event loop (fixes gdbstub issues)
>   - Remove PSCI support, inject UDEF on HVC/SMC
>   - Change vtimer logic to look at ctl.istatus for vtimer mask sync
>   - Add kick callback again (fixes remote CPU notification)
> 
> v7 -> v8:
> 
>   - Fix checkpatch errors
> ---
>  MAINTAINERS |   5 +
>  accel/hvf/hvf-accel-ops.c   |  14 +
>  include/sysemu/hvf_int.h|   9 +-
>  meson.build |   1 +
>  target/arm/hvf/hvf.c| 703 
>  target/arm/hvf/trace-events |  10 +
>  6 files changed, 741 insertions(+), 1 deletion(-)
>  create mode 100644 target/arm/hvf/hvf.c
>  create mode 100644 target/arm/hvf/trace-events

Reviewed-by: Sergio Lopez 
Tested-by: Sergio Lopez 


signature.asc
Description: PGP signature

[Bug 1846816] Re: Booting error on AIX 6.1 "Illegal Trap Instruction Interrupt in Kernel""

[David]

Hello,

I just wanted to confirm that this bug also affects me and that it also
appears in qemu 6.0.0.

When I try to boot up AIX 7.1 (and 6.1 as well), it prints the following line 
multiple times during boot:
Unimplemented SPAPR hcall 0x02b8

followed by crash:
Illegal Trap Instruction Interrupt in Kernel
05861AAC  tweqir0,0r0=0


I executed the following command to start qemu:
qemu-system-ppc64 -cpu POWER8 -machine pseries,max-cpu-compat=power7 -m 4096 -d 
unimp -serial stdio -drive file=hdisk0.qcow2,if=none,id=drive-virtio-disk0 
-device virtio-scsi-pci,id=scsi -device scsi-hd,drive=drive-virtio-disk0 -cdrom 
/mnt/hgfs/Images/AIX7.1/install-1.iso -prom-env "boot-command=boot cdrom:" 
-prom-env "input-device=/vdevice/vty@7100" -prom-env 
"output-device=/vdevice/vty@7100"

Since I am not the original author of this issue, I am not able to
change its state to New again

Sincerely
David

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1846816

Title:
  Booting error on AIX 6.1 "Illegal Trap Instruction Interrupt in
  Kernel""

Status in QEMU:
  Incomplete

Bug description:
  # ls -ltr
  total 8750584
  -rw-rw-r--  1 linux linux 4274997248 Oct  4 18:33 AIX.vol1.iso
  -rw-rw-r--  1 linux linux 4293888000 Oct  4 18:45 AIX.vol2.iso
  -rw-rw-r--  1 linux linux  391485440 Oct  4 18:50 AIX.vol3.iso
  -rw-r--r--  1 root  root  204608 Oct  4 19:00 AIX61.img

  # qemu-system-ppc64 -cpu POWER8,compat=power7 -machine pseries -m 8192 
-serial mon:stdio \
  > -drive file=/qemu/AIX61.img,if=none,id=drive-virtio-disk0 \
  > -device virtio-scsi-pci,id=scsi -device scsi-hd,drive=drive-virtio-disk0 \
  > -cdrom /qemu/AIX.vol1.iso \
  > -prom-env boot-command='boot cdrom: -s verbose'

  VNC server running on ::1:5900
  qemu-system-ppc64: warning: TCG doesn't support requested feature, 
cap-ibs=workaround

  SLOF **
  QEMU Starting
   Build Date = Jul  3 2019 12:26:14
   FW Version = git-ba1ab360eebe6338
   Press "s" to enter Open Firmware.

  Populating /vdevice methods
  Populating /vdevice/vty@7100
  Populating /vdevice/nvram@7101
  Populating /vdevice/l-lan@7102
  Populating /vdevice/v-scsi@7103
     SCSI: Looking for devices
    8200 CD-ROM   : "QEMU QEMU CD-ROM  2.5+"
  Populating /pci@8002000
   00  (D) : 1234 qemu vga
   00 0800 (D) : 1033 0194serial bus [ usb-xhci ]
   00 1000 (D) : 1af4 1004virtio [ scsi ]
  Populating /pci@8002000/scsi@2
     SCSI: Looking for devices
    100 DISK : "QEMU QEMU HARDDISK2.5+"
  Installing QEMU fb

  Scanning USB
    XHCI: Initializing
  USB Keyboard
  USB mouse
  No console specified using screen & keyboard

    Welcome to Open Firmware

    Copyright (c) 2004, 2017 IBM Corporation All rights reserved.
    This program and the accompanying materials are made available
    under the terms of the BSD License available at
    http://www.opensource.org/licenses/bsd-license.php

  Trying to load: -s verbose from: 
/vdevice/v-scsi@7103/disk@8200: ...   Successfully loaded
  qemu-system-ppc64: Couldn't negotiate a suitable PVR during CAS
  AIX
  StarLED{814}

  AIX Version 6.1
  exec(/etc/init){1,0}

  INIT: EXECUTING /sbin/rc.boot 1
  exec(/usr/bin/sh,-c,/sbin/rc.boot 1){1114146,1}
  exec(/sbin/rc.boot,/sbin/rc.boot,1){1114146,1}
  + PHASE=1
  + + bootinfo -p
  exec(/usr/sbin/bootinfo,-p){1179684,1114146}
  PLATFORM=chrp
  + [ ! -x /usr/lib/boot/bin/bootinfo_chrp ]
  + [ 1 -eq 1 ]
  + 1> /usr/lib/libc.a
  + init -c unlink /usr/lib/boot/bin/!(*_chrp)
  exec(/etc/init,-c,unlink /usr/lib/boot/bin/!(*_chrp)){1179686,1114146}
  + chramfs -t
  exec(/usr/sbin/chramfs,-t){1179688,1114146}
  + init -c unlink /usr/sbin/chramfs
  + 1> /dev/null
  exec(/etc/init,-c,unlink /usr/sbin/chramfs){1179690,1114146}
  + + bootinfo -t
  exec(/usr/sbin/bootinfo,-t){1179692,1114146}
  BOOTYPE=3
  + [ 0 -ne 0 ]
  + [ -z 3 ]
  + unset pdev_to_ldev undolt native_netboot_cfg
  + unset disknet_odm_init config_ATM
  + /usr/lib/methods/showled 0x510 DEV CFG 1 START
  exec(/usr/lib/methods/showled,0x510,DEV CFG 1 START){1179694,1114146}
  + cfgmgr -f -v
  exec(/usr/sbin/cfgmgr,-f,-v){1179696,1114146}
  cfgmgr is running in phase 1
  
  Time: 0 LEDS: 0x538
  Invoking top level program -- "/etc/methods/defsys"
  exec(/bin/sh,-c,/etc/methods/defsys ){1245222,1179696}
  exec(/etc/methods/defsys){1245222,1179696}
  exec(/bin/sh,-c,/usr/lib/methods/define_rspc -n -c sys -s node -t 
chrp){1310760,1245222}
  exec(/usr/lib/methods/define_rspc,-n,-c,sys,-s,node,-t,chrp){1310760,1245222}
  Time: 0 LEDS: 0x539
  Return code = 0
  * stdout *
  sys0

  *** no stderr 
  
  Attempting t

Re: [PATCH v4 04/15] qemu-iotests: add option to attach gdbserver

[Emanuele Giuseppe Esposito]

On 26/05/2021 21:15, Vladimir Sementsov-Ogievskiy wrote:

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:

Define -gdb flag and GDB_OPTIONS environment variable
to python tests to attach a gdbserver to each qemu instance.
This patch only adds and parses this flag, it does not yet add
the implementation for it.

if -gdb is not provided but $GDB_OPTIONS is set, ignore the
environment variable.

Signed-off-by: Emanuele Giuseppe Esposito 
---
  tests/qemu-iotests/check  |  6 +-
  tests/qemu-iotests/iotests.py |  5 +
  tests/qemu-iotests/testenv.py | 19 ---
  3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index d1c87ceaf1..b9820fdaaf 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -33,6 +33,9 @@ def make_argparser() -> argparse.ArgumentParser:
 help='pretty print output for make check')
  p.add_argument('-d', dest='debug', action='store_true', 
help='debug')

+    p.add_argument('-gdb', action='store_true',
+   help="start gdbserver with $GDB_OPTIONS options \
+    ('localhost:12345' if $GDB_OPTIONS is empty)")
  p.add_argument('-misalign', action='store_true',
 help='misalign memory allocations')
  p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -112,7 +115,8 @@ if __name__ == '__main__':
  env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
    aiomode=args.aiomode, cachemode=args.cachemode,
    imgopts=args.imgopts, misalign=args.misalign,
-  debug=args.debug, valgrind=args.valgrind)
+  debug=args.debug, valgrind=args.valgrind,
+  gdb=args.gdb)
  testfinder = TestFinder(test_dir=env.source_iotests)
diff --git a/tests/qemu-iotests/iotests.py 
b/tests/qemu-iotests/iotests.py

index 5d78de0f0b..d667fde6f8 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -75,6 +75,11 @@
  qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
  qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
+gdb_qemu_env = os.environ.get('GDB_OPTIONS')


should we specify a default? otherwise get() should raise an exception 
when GDB_OPTIONS is not set..


or you need os.getenv, which will return None if variable is absent.


No, os.environ.get does not raise any exception. I tested it myself, plus:
https://stackoverflow.com/questions/16924471/difference-between-os-getenv-and-os-environ-get




+qemu_gdb = []
+if gdb_qemu_env:
+    qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
+
  imgfmt = os.environ.get('IMGFMT', 'raw')
  imgproto = os.environ.get('IMGPROTO', 'file')
  output_dir = os.environ.get('OUTPUT_DIR', '.')
diff --git a/tests/qemu-iotests/testenv.py 
b/tests/qemu-iotests/testenv.py

index 6d27712617..49ddd586ef 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -27,6 +27,7 @@
  import glob
  from typing import Dict, Any, Optional, ContextManager
+DEF_GDB_OPTIONS = 'localhost:12345'
  def isxfile(path: str) -> bool:
  return os.path.isfile(path) and os.access(path, os.X_OK)
@@ -72,7 +73,8 @@ class TestEnv(ContextManager['TestEnv']):
   'QEMU_NBD_OPTIONS', 'IMGOPTS', 'IMGFMT', 
'IMGPROTO',

   'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
   'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 
'IMGOPTSSYNTAX',
- 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 
'MALLOC_PERTURB_']
+ 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 
'MALLOC_PERTURB_',

+ 'GDB_OPTIONS']
  def get_env(self) -> Dict[str, str]:
  env = {}
@@ -163,7 +165,8 @@ def __init__(self, imgfmt: str, imgproto: str, 
aiomode: str,

   imgopts: Optional[str] = None,
   misalign: bool = False,
   debug: bool = False,
- valgrind: bool = False) -> None:
+ valgrind: bool = False,
+ gdb: bool = False) -> None:
  self.imgfmt = imgfmt
  self.imgproto = imgproto
  self.aiomode = aiomode
@@ -171,6 +174,14 @@ def __init__(self, imgfmt: str, imgproto: str, 
aiomode: str,

  self.misalign = misalign
  self.debug = debug
+    if gdb:
+    self.gdb_options = os.environ.get('GDB_OPTIONS', 
DEF_GDB_OPTIONS)


everywhere in the file os.getenv is used, let's be consistent on it.


+    if not self.gdb_options:
+    # cover the case 'export GDB_OPTIONS='
+    self.gdb_options = DEF_GDB_OPTIONS


Hmm, a bit strange to handle 'export X=' only for this new variable, we 
don't have such logic for other variables.. I'm not against still, if 
you need it.



+    elif 'GDB_OPTIONS' in os.environ:
+    del os.environ['GDB_OPTIONS']


Don't need to remove variable from envirton, it has no effect. The task 

Re: [PATCH v8 15/19] hvf: arm: Implement -cpu host

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:49PM +0200, Alexander Graf wrote:
> Now that we have working system register sync, we push more target CPU
> properties into the virtual machine. That might be useful in some
> situations, but is not the typical case that users want.
> 
> So let's add a -cpu host option that allows them to explicitly pass all
> CPU capabilities of their host CPU into the guest.
> 
> Signed-off-by: Alexander Graf 
> Acked-by: Roman Bolshakov 
> 
> ---
> 
> v6 -> v7:
> 
>   - Move function define to own header
>   - Do not propagate SVE features for HVF
>   - Remove stray whitespace change
>   - Verify that EL0 and EL1 do not allow AArch32 mode
>   - Only probe host CPU features once
> ---
>  target/arm/cpu.c |  9 --
>  target/arm/cpu.h |  2 ++
>  target/arm/hvf/hvf.c | 72 
>  target/arm/hvf_arm.h | 19 
>  target/arm/kvm_arm.h |  2 --
>  5 files changed, 100 insertions(+), 4 deletions(-)
>  create mode 100644 target/arm/hvf_arm.h

Reviewed-by: Sergio Lopez 
Tested-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 14/19] arm/hvf: Add a WFI handler

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:48PM +0200, Alexander Graf wrote:
> From: Peter Collingbourne 
> 
> Sleep on WFI until the VTIMER is due but allow ourselves to be woken
> up on IPI.
> 
> In this implementation IPI is blocked on the CPU thread at startup and
> pselect() is used to atomically unblock the signal and begin sleeping.
> The signal is sent unconditionally so there's no need to worry about
> races between actually sleeping and the "we think we're sleeping"
> state. It may lead to an extra wakeup but that's better than missing
> it entirely.
> 
> Signed-off-by: Peter Collingbourne 
> [agraf: Remove unused 'set' variable, always advance PC on WFX trap]
> Signed-off-by: Alexander Graf 
> Acked-by: Roman Bolshakov 
> 
> ---
> 
> v6 -> v7:
> 
>   - Move WFI into function
>   - Improve comment wording
> ---
>  accel/hvf/hvf-accel-ops.c |  5 ++-
>  include/sysemu/hvf_int.h  |  1 +
>  target/arm/hvf/hvf.c  | 68 +++
>  3 files changed, 71 insertions(+), 3 deletions(-)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

[Bug 1846816] Re: Booting error on AIX 6.1 "Illegal Trap Instruction Interrupt in Kernel""

[Thomas Huth]

** Changed in: qemu
   Status: Incomplete => 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/1846816

Title:
  Booting error on AIX 6.1 "Illegal Trap Instruction Interrupt in
  Kernel""

Status in QEMU:
  New

Bug description:
  # ls -ltr
  total 8750584
  -rw-rw-r--  1 linux linux 4274997248 Oct  4 18:33 AIX.vol1.iso
  -rw-rw-r--  1 linux linux 4293888000 Oct  4 18:45 AIX.vol2.iso
  -rw-rw-r--  1 linux linux  391485440 Oct  4 18:50 AIX.vol3.iso
  -rw-r--r--  1 root  root  204608 Oct  4 19:00 AIX61.img

  # qemu-system-ppc64 -cpu POWER8,compat=power7 -machine pseries -m 8192 
-serial mon:stdio \
  > -drive file=/qemu/AIX61.img,if=none,id=drive-virtio-disk0 \
  > -device virtio-scsi-pci,id=scsi -device scsi-hd,drive=drive-virtio-disk0 \
  > -cdrom /qemu/AIX.vol1.iso \
  > -prom-env boot-command='boot cdrom: -s verbose'

  VNC server running on ::1:5900
  qemu-system-ppc64: warning: TCG doesn't support requested feature, 
cap-ibs=workaround

  SLOF **
  QEMU Starting
   Build Date = Jul  3 2019 12:26:14
   FW Version = git-ba1ab360eebe6338
   Press "s" to enter Open Firmware.

  Populating /vdevice methods
  Populating /vdevice/vty@7100
  Populating /vdevice/nvram@7101
  Populating /vdevice/l-lan@7102
  Populating /vdevice/v-scsi@7103
     SCSI: Looking for devices
    8200 CD-ROM   : "QEMU QEMU CD-ROM  2.5+"
  Populating /pci@8002000
   00  (D) : 1234 qemu vga
   00 0800 (D) : 1033 0194serial bus [ usb-xhci ]
   00 1000 (D) : 1af4 1004virtio [ scsi ]
  Populating /pci@8002000/scsi@2
     SCSI: Looking for devices
    100 DISK : "QEMU QEMU HARDDISK2.5+"
  Installing QEMU fb

  Scanning USB
    XHCI: Initializing
  USB Keyboard
  USB mouse
  No console specified using screen & keyboard

    Welcome to Open Firmware

    Copyright (c) 2004, 2017 IBM Corporation All rights reserved.
    This program and the accompanying materials are made available
    under the terms of the BSD License available at
    http://www.opensource.org/licenses/bsd-license.php

  Trying to load: -s verbose from: 
/vdevice/v-scsi@7103/disk@8200: ...   Successfully loaded
  qemu-system-ppc64: Couldn't negotiate a suitable PVR during CAS
  AIX
  StarLED{814}

  AIX Version 6.1
  exec(/etc/init){1,0}

  INIT: EXECUTING /sbin/rc.boot 1
  exec(/usr/bin/sh,-c,/sbin/rc.boot 1){1114146,1}
  exec(/sbin/rc.boot,/sbin/rc.boot,1){1114146,1}
  + PHASE=1
  + + bootinfo -p
  exec(/usr/sbin/bootinfo,-p){1179684,1114146}
  PLATFORM=chrp
  + [ ! -x /usr/lib/boot/bin/bootinfo_chrp ]
  + [ 1 -eq 1 ]
  + 1> /usr/lib/libc.a
  + init -c unlink /usr/lib/boot/bin/!(*_chrp)
  exec(/etc/init,-c,unlink /usr/lib/boot/bin/!(*_chrp)){1179686,1114146}
  + chramfs -t
  exec(/usr/sbin/chramfs,-t){1179688,1114146}
  + init -c unlink /usr/sbin/chramfs
  + 1> /dev/null
  exec(/etc/init,-c,unlink /usr/sbin/chramfs){1179690,1114146}
  + + bootinfo -t
  exec(/usr/sbin/bootinfo,-t){1179692,1114146}
  BOOTYPE=3
  + [ 0 -ne 0 ]
  + [ -z 3 ]
  + unset pdev_to_ldev undolt native_netboot_cfg
  + unset disknet_odm_init config_ATM
  + /usr/lib/methods/showled 0x510 DEV CFG 1 START
  exec(/usr/lib/methods/showled,0x510,DEV CFG 1 START){1179694,1114146}
  + cfgmgr -f -v
  exec(/usr/sbin/cfgmgr,-f,-v){1179696,1114146}
  cfgmgr is running in phase 1
  
  Time: 0 LEDS: 0x538
  Invoking top level program -- "/etc/methods/defsys"
  exec(/bin/sh,-c,/etc/methods/defsys ){1245222,1179696}
  exec(/etc/methods/defsys){1245222,1179696}
  exec(/bin/sh,-c,/usr/lib/methods/define_rspc -n -c sys -s node -t 
chrp){1310760,1245222}
  exec(/usr/lib/methods/define_rspc,-n,-c,sys,-s,node,-t,chrp){1310760,1245222}
  Time: 0 LEDS: 0x539
  Return code = 0
  * stdout *
  sys0

  *** no stderr 
  
  Attempting to configure device 'sys0'
  Time: 0 LEDS: 0x811
  Invoking /usr/lib/methods/cfgsys_chrp -1 -l sys0
  exec(/bin/sh,-c,/usr/lib/methods/cfgsys_chrp -1 -l sys0){1245224,1179696}
  Number of running methods: 1
  exec(/usr/lib/methods/cfgsys_chrp,-1,-l,sys0){1245224,1179696}
  LED{A20}
  Illegal Trap Instruction Interrupt in Kernel
  04151A74  tweqir0,0r0=0
  KDB(0)>

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1846816/+subscriptions

Re: [PATCH v8 17/19] arm: Add Hypervisor.framework build target

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:51PM +0200, Alexander Graf wrote:
> Now that we have all logic in place that we need to handle 
> Hypervisor.framework
> on Apple Silicon systems, let's add CONFIG_HVF for aarch64 as well so that we
> can build it.
> 
> Signed-off-by: Alexander Graf 
> Reviewed-by: Roman Bolshakov 
> Tested-by: Roman Bolshakov  (x86 only)
> 
> ---
> 
> v1 -> v2:
> 
>   - Fix build on 32bit arm
> 
> v3 -> v4:
> 
>   - Remove i386-softmmu target
> 
> v6 -> v7:
> 
>   - Simplify HVF matching logic in meson build file
> ---
>  meson.build| 7 +++
>  target/arm/hvf/meson.build | 3 +++
>  target/arm/meson.build | 2 ++
>  3 files changed, 12 insertions(+)
>  create mode 100644 target/arm/hvf/meson.build

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: Windows fails to boot after rebase to QEMU master

[Claudio Fontana]

On 5/27/21 11:48 AM, Claudio Fontana wrote:
> On 5/27/21 11:15 AM, Dr. David Alan Gilbert wrote:
>> * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
>>> On 5/27/21 10:31 AM, Dr. David Alan Gilbert wrote:
 * Claudio Fontana (cfont...@suse.de) wrote:
> On 5/26/21 9:30 PM, Dr. David Alan Gilbert wrote:
>> * Michael S. Tsirkin (m...@redhat.com) wrote:
>>> On Fri, May 21, 2021 at 11:17:19AM +0200, Siddharth Chandrasekaran 
>>> wrote:
 After a rebase to QEMU master, I am having trouble booting windows VMs.
 Git bisect indicates commit f5cc5a5c1686 ("i386: split cpu accelerators
 from cpu.c, using AccelCPUClass") to have introduced the issue. I spent
 some time looking at into it yesterday without much luck.

 Steps to reproduce:

 $ ./configure --enable-kvm --disable-xen 
 --target-list=x86_64-softmmu --enable-debug
 $ make -j `nproc`
 $ ./build/x86_64-softmmu/qemu-system-x86_64 \
 -cpu 
 host,hv_synic,hv_vpindex,hv_time,hv_runtime,hv_stimer,hv_crash \
 -enable-kvm \
 -name test,debug-threads=on \
 -smp 1,threads=1,cores=1,sockets=1 \
 -m 4G \
 -net nic -net user \
 -boot d,menu=on \
 -usbdevice tablet \
 -vnc :3 \
 -machine q35,smm=on \
 -drive 
 if=pflash,format=raw,readonly=on,unit=0,file="../OVMF_CODE.secboot.fd" 
 \
 -drive 
 if=pflash,format=raw,unit=1,file="../OVMF_VARS.secboot.fd" \
 -global ICH9-LPC.disable_s3=1 \
 -global driver=cfi.pflash01,property=secure,value=on \
 -cdrom "../Windows_Server_2016_14393.ISO" \
 -drive 
 file="../win_server_2016.qcow2",format=qcow2,if=none,id=rootfs_drive \
 -device ahci,id=ahci \
 -device ide-hd,drive=rootfs_drive,bus=ahci.0

 If the issue is not obvious, I'd like some pointers on how to go about
 fixing this issue.

 ~ Sid.

>>>
>>> At a guess this commit inadvertently changed something in the CPU ID.
>>> I'd start by using a linux guest to dump cpuid before and after the
>>> change.
>>
>> I've not had a chance to do that yet, however I did just end up with a
>> bisect of a linux guest failure bisecting to the same patch:
>>
>> [dgilbert@dgilbert-t580 qemu]$ git bisect bad
>> f5cc5a5c168674f84bf061cdb307c2d25fba5448 is the first bad commit
>> commit f5cc5a5c168674f84bf061cdb307c2d25fba5448
>> Author: Claudio Fontana 
>> Date:   Mon Mar 22 14:27:40 2021 +0100
>>
>> i386: split cpu accelerators from cpu.c, using AccelCPUClass
>> 
>> i386 is the first user of AccelCPUClass, allowing to split
>> cpu.c into:
>> 
>> cpu.ccpuid and common x86 cpu functionality
>> host-cpu.c   host x86 cpu functions and "host" cpu type
>> kvm/kvm-cpu.cKVM x86 AccelCPUClass
>> hvf/hvf-cpu.cHVF x86 AccelCPUClass
>> tcg/tcg-cpu.cTCG x86 AccelCPUClass
>>>
>>> Well this is a big commit... I'm not custom to x86 target, and am
>>> having hard time following the cpu host/max change.
>>>
>>> Is it working when you use '-cpu max,...' instead of '-cpu host,'?
>>
>> No; and in fact the cpuid's are almost entirely different with and
>> without this patch! (both with -cpu host).  It looks like with this
>> patch we're getting the cpuid for the TCG cpuid rather than the host:
>>
>> Prior to this patch:
>> :/# cat /proc/cpuinfo
>> processor   : 0
>> vendor_id   : GenuineIntel
>> cpu family  : 6
>> model   : 142
>> model name  : Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
>> stepping: 10
>> microcode   : 0xe0
>> cpu MHz : 2111.998
>> cache size  : 16384 KB
>> physical id : 0
>> siblings: 1
>> core id : 0
>> cpu cores   : 1
>> apicid  : 0
>> initial apicid  : 0
>> fpu : yes
>> fpu_exception   : yes
>> cpuid level : 22
>> wp  : yes
>> flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca 
>> cmov pat pse36 clflush mmx fxsr sse sse2 ss syscall nx pdpe1gb rdtscp lm 
>> constant
>> _tsc arch_perfmon rep_good nopl xtopology cpuid tsc_known_freq pni pclmulqdq 
>> vmx ssse3 fma cx16 pdcm pcid sse4_1 sse4_2 x2apic movbe popcnt 
>> tsc_deadline_tim
>> er aes xsave avx f16c rdrand hypervisor lahf_lm abm 3dnowprefetch 
>> cpuid_fault invpcid_single pti ssbd ibrs ibpb stibp tpr_shadow vnmi 
>> flexpriority ept vpid
>> ept_ad fsgsbase tsc_adjust bmi1 hle avx2 smep bmi2 erms invpcid rtm mpx 
>> rdseed adx smap clflushopt xsaveopt xsavec xgetbv1 xsaves arat umip md_clear 
>> arch_ca
>> pabilities
>> vmx flags   : vnmi preemption_timer invvpid ep

Re: Windows fails to boot after rebase to QEMU master

[Claudio Fontana]

On 5/27/21 12:53 PM, Claudio Fontana wrote:
> On 5/27/21 11:48 AM, Claudio Fontana wrote:
>> On 5/27/21 11:15 AM, Dr. David Alan Gilbert wrote:
>>> * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
 On 5/27/21 10:31 AM, Dr. David Alan Gilbert wrote:
> * Claudio Fontana (cfont...@suse.de) wrote:
>> On 5/26/21 9:30 PM, Dr. David Alan Gilbert wrote:
>>> * Michael S. Tsirkin (m...@redhat.com) wrote:
 On Fri, May 21, 2021 at 11:17:19AM +0200, Siddharth Chandrasekaran 
 wrote:
> After a rebase to QEMU master, I am having trouble booting windows 
> VMs.
> Git bisect indicates commit f5cc5a5c1686 ("i386: split cpu 
> accelerators
> from cpu.c, using AccelCPUClass") to have introduced the issue. I 
> spent
> some time looking at into it yesterday without much luck.
>
> Steps to reproduce:
>
> $ ./configure --enable-kvm --disable-xen 
> --target-list=x86_64-softmmu --enable-debug
> $ make -j `nproc`
> $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> -cpu 
> host,hv_synic,hv_vpindex,hv_time,hv_runtime,hv_stimer,hv_crash \
> -enable-kvm \
> -name test,debug-threads=on \
> -smp 1,threads=1,cores=1,sockets=1 \
> -m 4G \
> -net nic -net user \
> -boot d,menu=on \
> -usbdevice tablet \
> -vnc :3 \
> -machine q35,smm=on \
> -drive 
> if=pflash,format=raw,readonly=on,unit=0,file="../OVMF_CODE.secboot.fd"
>  \
> -drive 
> if=pflash,format=raw,unit=1,file="../OVMF_VARS.secboot.fd" \
> -global ICH9-LPC.disable_s3=1 \
> -global driver=cfi.pflash01,property=secure,value=on \
> -cdrom "../Windows_Server_2016_14393.ISO" \
> -drive 
> file="../win_server_2016.qcow2",format=qcow2,if=none,id=rootfs_drive \
> -device ahci,id=ahci \
> -device ide-hd,drive=rootfs_drive,bus=ahci.0
>
> If the issue is not obvious, I'd like some pointers on how to go about
> fixing this issue.
>
> ~ Sid.
>

 At a guess this commit inadvertently changed something in the CPU ID.
 I'd start by using a linux guest to dump cpuid before and after the
 change.
>>>
>>> I've not had a chance to do that yet, however I did just end up with a
>>> bisect of a linux guest failure bisecting to the same patch:
>>>
>>> [dgilbert@dgilbert-t580 qemu]$ git bisect bad
>>> f5cc5a5c168674f84bf061cdb307c2d25fba5448 is the first bad commit
>>> commit f5cc5a5c168674f84bf061cdb307c2d25fba5448
>>> Author: Claudio Fontana 
>>> Date:   Mon Mar 22 14:27:40 2021 +0100
>>>
>>> i386: split cpu accelerators from cpu.c, using AccelCPUClass
>>> 
>>> i386 is the first user of AccelCPUClass, allowing to split
>>> cpu.c into:
>>> 
>>> cpu.ccpuid and common x86 cpu functionality
>>> host-cpu.c   host x86 cpu functions and "host" cpu type
>>> kvm/kvm-cpu.cKVM x86 AccelCPUClass
>>> hvf/hvf-cpu.cHVF x86 AccelCPUClass
>>> tcg/tcg-cpu.cTCG x86 AccelCPUClass

 Well this is a big commit... I'm not custom to x86 target, and am
 having hard time following the cpu host/max change.

 Is it working when you use '-cpu max,...' instead of '-cpu host,'?
>>>
>>> No; and in fact the cpuid's are almost entirely different with and
>>> without this patch! (both with -cpu host).  It looks like with this
>>> patch we're getting the cpuid for the TCG cpuid rather than the host:
>>>
>>> Prior to this patch:
>>> :/# cat /proc/cpuinfo
>>> processor   : 0
>>> vendor_id   : GenuineIntel
>>> cpu family  : 6
>>> model   : 142
>>> model name  : Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
>>> stepping: 10
>>> microcode   : 0xe0
>>> cpu MHz : 2111.998
>>> cache size  : 16384 KB
>>> physical id : 0
>>> siblings: 1
>>> core id : 0
>>> cpu cores   : 1
>>> apicid  : 0
>>> initial apicid  : 0
>>> fpu : yes
>>> fpu_exception   : yes
>>> cpuid level : 22
>>> wp  : yes
>>> flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca 
>>> cmov pat pse36 clflush mmx fxsr sse sse2 ss syscall nx pdpe1gb rdtscp lm 
>>> constant
>>> _tsc arch_perfmon rep_good nopl xtopology cpuid tsc_known_freq pni 
>>> pclmulqdq vmx ssse3 fma cx16 pdcm pcid sse4_1 sse4_2 x2apic movbe popcnt 
>>> tsc_deadline_tim
>>> er aes xsave avx f16c rdrand hypervisor lahf_lm abm 3dnowprefetch 
>>> cpuid_fault invpcid_single pti ssbd ibrs ibpb stibp tpr_shadow vnmi 
>>> flexpriority ept vpid
>>> ept_ad fsgsbase tsc_adjust bmi1 hle avx2

Re: [PATCH v8 19/19] hvf: arm: Handle Windows 10 SMC call

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:53PM +0200, Alexander Graf wrote:
> Windows 10 calls an SMCCC call via SMC unconditionally on boot. It lives
> in the trusted application call number space, but its purpose is unknown.
> 
> In our current SMC implementation, we inject a UDEF for unknown SMC calls,
> including this one. However, Windows breaks on boot when we do this. Instead,
> let's return an error code.
> 
> With this patch applied I can successfully boot the current Windows 10
> Insider Preview in HVF.
> 
> Signed-off-by: Alexander Graf 
> 
> ---
> 
> v7 -> v8:
> 
>   - fix checkpatch
> ---
>  target/arm/hvf/hvf.c | 4 
>  1 file changed, 4 insertions(+)

Reviewed-by: Sergio Lopez 
Tested-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: [PATCH v8 16/19] hvf: arm: Implement PSCI handling

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:50PM +0200, Alexander Graf wrote:
> We need to handle PSCI calls. Most of the TCG code works for us,
> but we can simplify it to only handle aa64 mode and we need to
> handle SUSPEND differently.
> 
> This patch takes the TCG code as template and duplicates it in HVF.
> 
> To tell the guest that we support PSCI 0.2 now, update the check in
> arm_cpu_initfn() as well.
> 
> Signed-off-by: Alexander Graf 
> 
> ---
> 
> v6 -> v7:
> 
>   - This patch integrates "arm: Set PSCI to 0.2 for HVF"
> 
> v7 -> v8:
> 
>   - Do not advance for HVC, PC is already updated by hvf
>   - Fix checkpatch error
> ---
>  target/arm/cpu.c|   4 +-
>  target/arm/hvf/hvf.c| 123 ++--
>  target/arm/hvf/trace-events |   1 +
>  3 files changed, 122 insertions(+), 6 deletions(-)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

Re: Windows fails to boot after rebase to QEMU master

[Siddharth Chandrasekaran]

On Thu, May 27, 2021 at 01:36:37PM +0200, Claudio Fontana wrote:
> Just to check whether this is actually the issue we are talking about,
> Sid et al, could you try this?
> 
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index c496bfa1c2..810c46281b 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4252,6 +4252,7 @@ static void max_x86_cpu_initfn(Object *obj)
>  object_property_set_str(OBJECT(cpu), "model-id",
>  "QEMU TCG CPU version " QEMU_HW_VERSION,
>  &error_abort);
> +accel_cpu_instance_init(CPU(cpu));
>  }
> 
>  static const TypeInfo max_x86_cpu_type_info = {
> --
> 
> Does this band-aid happen to cover-up the issue?

Yes it does fix the issue for me. Thanks.

~ Sid.

> I need to think about the proper fix though, any suggestions Paolo,
> Eduardo?
> 
> The pickle here is that we'd need to call the accelerator specific
> initialization of the x86 accel-cpu only after the x86 cpu subclass
> initfn, otherwise the accel-specific cpu initialization code has no
> chance to see the subclass (max) trying to set ->max_features.
> 
> C.



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] linux-user: make process_pending_signals thread-safe

[Hamza Mahfooz]

On Thu, May 27 2021 at 11:45:26 AM +0100, Peter Maydell 
 wrote:

What issue are we trying to fix by making this change ?
I suppose that it wouldn't fix any issues in the current state of 
affairs,

maybe it is something to reconsider if glibc ever changes such that,
sigprocmask is no longer MT-safe (which is within reason since the MT
behavior of sigprocmask is undefined by POSIX).

Re: [PATCH v8 18/19] arm: Enable Windows 10 trusted SMCCC boot call

[Sergio Lopez]

On Wed, May 19, 2021 at 10:22:52PM +0200, Alexander Graf wrote:
> Windows 10 calls an SMCCC call via SMC unconditionally on boot. It lives
> in the trusted application call number space, but its purpose is unknown.
> 
> In our current SMC implementation, we inject a UDEF for unknown SMC calls,
> including this one. However, Windows breaks on boot when we do this. Instead,
> let's return an error code.
> 
> With this and -M virt,virtualization=on I can successfully boot the current
> Windows 10 Insider Preview in TCG.
> 
> Signed-off-by: Alexander Graf 
> ---
>  target/arm/kvm-consts.h | 2 ++
>  target/arm/psci.c   | 2 ++
>  2 files changed, 4 insertions(+)

Reviewed-by: Sergio Lopez 


signature.asc
Description: PGP signature

[Bug 1846816] Re: Booting error on AIX 6.1 "Illegal Trap Instruction Interrupt in Kernel""

[Greg Kurz]

The only AIX version that _might_ run with QEMU is 7.2. Older ones don't
have virtio AFAIK.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1846816

Title:
  Booting error on AIX 6.1 "Illegal Trap Instruction Interrupt in
  Kernel""

Status in QEMU:
  New

Bug description:
  # ls -ltr
  total 8750584
  -rw-rw-r--  1 linux linux 4274997248 Oct  4 18:33 AIX.vol1.iso
  -rw-rw-r--  1 linux linux 4293888000 Oct  4 18:45 AIX.vol2.iso
  -rw-rw-r--  1 linux linux  391485440 Oct  4 18:50 AIX.vol3.iso
  -rw-r--r--  1 root  root  204608 Oct  4 19:00 AIX61.img

  # qemu-system-ppc64 -cpu POWER8,compat=power7 -machine pseries -m 8192 
-serial mon:stdio \
  > -drive file=/qemu/AIX61.img,if=none,id=drive-virtio-disk0 \
  > -device virtio-scsi-pci,id=scsi -device scsi-hd,drive=drive-virtio-disk0 \
  > -cdrom /qemu/AIX.vol1.iso \
  > -prom-env boot-command='boot cdrom: -s verbose'

  VNC server running on ::1:5900
  qemu-system-ppc64: warning: TCG doesn't support requested feature, 
cap-ibs=workaround

  SLOF **
  QEMU Starting
   Build Date = Jul  3 2019 12:26:14
   FW Version = git-ba1ab360eebe6338
   Press "s" to enter Open Firmware.

  Populating /vdevice methods
  Populating /vdevice/vty@7100
  Populating /vdevice/nvram@7101
  Populating /vdevice/l-lan@7102
  Populating /vdevice/v-scsi@7103
     SCSI: Looking for devices
    8200 CD-ROM   : "QEMU QEMU CD-ROM  2.5+"
  Populating /pci@8002000
   00  (D) : 1234 qemu vga
   00 0800 (D) : 1033 0194serial bus [ usb-xhci ]
   00 1000 (D) : 1af4 1004virtio [ scsi ]
  Populating /pci@8002000/scsi@2
     SCSI: Looking for devices
    100 DISK : "QEMU QEMU HARDDISK2.5+"
  Installing QEMU fb

  Scanning USB
    XHCI: Initializing
  USB Keyboard
  USB mouse
  No console specified using screen & keyboard

    Welcome to Open Firmware

    Copyright (c) 2004, 2017 IBM Corporation All rights reserved.
    This program and the accompanying materials are made available
    under the terms of the BSD License available at
    http://www.opensource.org/licenses/bsd-license.php

  Trying to load: -s verbose from: 
/vdevice/v-scsi@7103/disk@8200: ...   Successfully loaded
  qemu-system-ppc64: Couldn't negotiate a suitable PVR during CAS
  AIX
  StarLED{814}

  AIX Version 6.1
  exec(/etc/init){1,0}

  INIT: EXECUTING /sbin/rc.boot 1
  exec(/usr/bin/sh,-c,/sbin/rc.boot 1){1114146,1}
  exec(/sbin/rc.boot,/sbin/rc.boot,1){1114146,1}
  + PHASE=1
  + + bootinfo -p
  exec(/usr/sbin/bootinfo,-p){1179684,1114146}
  PLATFORM=chrp
  + [ ! -x /usr/lib/boot/bin/bootinfo_chrp ]
  + [ 1 -eq 1 ]
  + 1> /usr/lib/libc.a
  + init -c unlink /usr/lib/boot/bin/!(*_chrp)
  exec(/etc/init,-c,unlink /usr/lib/boot/bin/!(*_chrp)){1179686,1114146}
  + chramfs -t
  exec(/usr/sbin/chramfs,-t){1179688,1114146}
  + init -c unlink /usr/sbin/chramfs
  + 1> /dev/null
  exec(/etc/init,-c,unlink /usr/sbin/chramfs){1179690,1114146}
  + + bootinfo -t
  exec(/usr/sbin/bootinfo,-t){1179692,1114146}
  BOOTYPE=3
  + [ 0 -ne 0 ]
  + [ -z 3 ]
  + unset pdev_to_ldev undolt native_netboot_cfg
  + unset disknet_odm_init config_ATM
  + /usr/lib/methods/showled 0x510 DEV CFG 1 START
  exec(/usr/lib/methods/showled,0x510,DEV CFG 1 START){1179694,1114146}
  + cfgmgr -f -v
  exec(/usr/sbin/cfgmgr,-f,-v){1179696,1114146}
  cfgmgr is running in phase 1
  
  Time: 0 LEDS: 0x538
  Invoking top level program -- "/etc/methods/defsys"
  exec(/bin/sh,-c,/etc/methods/defsys ){1245222,1179696}
  exec(/etc/methods/defsys){1245222,1179696}
  exec(/bin/sh,-c,/usr/lib/methods/define_rspc -n -c sys -s node -t 
chrp){1310760,1245222}
  exec(/usr/lib/methods/define_rspc,-n,-c,sys,-s,node,-t,chrp){1310760,1245222}
  Time: 0 LEDS: 0x539
  Return code = 0
  * stdout *
  sys0

  *** no stderr 
  
  Attempting to configure device 'sys0'
  Time: 0 LEDS: 0x811
  Invoking /usr/lib/methods/cfgsys_chrp -1 -l sys0
  exec(/bin/sh,-c,/usr/lib/methods/cfgsys_chrp -1 -l sys0){1245224,1179696}
  Number of running methods: 1
  exec(/usr/lib/methods/cfgsys_chrp,-1,-l,sys0){1245224,1179696}
  LED{A20}
  Illegal Trap Instruction Interrupt in Kernel
  04151A74  tweqir0,0r0=0
  KDB(0)>

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1846816/+subscriptions

Re: [PATCH 4/5] monitor: removed cpustats command

[Bruno Piazera Larsen]

On 27/05/2021 05:30, Greg Kurz wrote:

On Thu, 27 May 2021 09:09:55 +0100
"Dr. David Alan Gilbert"  wrote:


* Greg Kurz (gr...@kaod.org) wrote:

On Wed, 26 May 2021 17:21:03 -0300
"Bruno Larsen (billionai)"  wrote:


Since ppc was the last architecture to collect these statistics and
it is currently phasing this collection out, the command that would query
this information is being removed.


So this is removing an obviously user visible feature. This should be
mentioned in docs/system/removed-features.rst... but, wait, I don't
see anything for it in docs/system/deprecated.rst. This is dropping
a feature without following the usual deprecation policy, i.e.
marking the feature as deprecated and only remove it 2 QEMU versions
later. Any justification for that ?


The command called a function that ultimately called an empty callback 
unless you changed target/ppc/translate.c and removed the comments 
around #define DO_PPC_STATISTICS. And like I mentioned in the cover 
letter (which may not have been CC'ed to you, sorry) ppc was the last 
architecture to support this feature while they were using the legacy 
decode system, but as they move to decodetree, this data wouldn't even 
be collected.


That's the justification, basically.


As long as the command really isn't useful any more, I wouldn't object
to that from an HMP point of view.


Ok then this should be documented in docs/system/removed-features.rst at
least.

Sure, will send a patch later today with the update



Dave


David,

Unrelated, I saw that you already applied this to ppc-for-6.1 on gitlab
but the commit title appears to be broken:

'65;6401;1cmonitor: removed cpustats command

https://gitlab.com/dgibson/qemu/-/commit/532be563eae6b8ae834ff7e9ebb1428f53569a69


Suggested-by: Richard Henderson 
Signed-off-by: Bruno Larsen (billionai) 
---
  hmp-commands-info.hx | 13 -
  monitor/misc.c   | 11 ---
  2 files changed, 24 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ab0c7aa5ee..b2347a6aea 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -500,19 +500,6 @@ SRST
  Show the current VM UUID.
  ERST
  
-{

-.name   = "cpustats",
-.args_type  = "",
-.params = "",
-.help   = "show CPU statistics",
-.cmd= hmp_info_cpustats,
-},
-
-SRST
-  ``info cpustats``
-Show CPU statistics.
-ERST
-
  #if defined(CONFIG_SLIRP)
  {
  .name   = "usernet",
diff --git a/monitor/misc.c b/monitor/misc.c
index f3a393ea59..1539e18557 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -369,17 +369,6 @@ static void hmp_info_history(Monitor *mon, const QDict 
*qdict)
  }
  }
  
-static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)

-{
-CPUState *cs = mon_get_cpu(mon);
-
-if (!cs) {
-monitor_printf(mon, "No CPU available\n");
-return;
-}
-cpu_dump_statistics(cs, 0);
-}
-
  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
  {
  const char *name = qdict_get_try_str(qdict, "name");

--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO 


Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer 

Re: [PATCH v2 4/7] block-copy: add a CoMutex to the BlockCopyTask list

[Stefan Hajnoczi]

On Tue, May 18, 2021 at 12:07:54PM +0200, Emanuele Giuseppe Esposito wrote:
> -static BlockCopyTask *find_conflicting_task(BlockCopyState *s,
> -int64_t offset, int64_t bytes)
> +/* Called with lock held */

s/lock/tasks_lock/


signature.asc
Description: PGP signature

Re: [PATCH v2 1/7] block-copy: streamline choice of copy_range vs. read/write

[Stefan Hajnoczi]

On Tue, May 18, 2021 at 12:07:51PM +0200, Emanuele Giuseppe Esposito wrote:
>  } else {
>  /*
>   * We enable copy-range, but keep small copy_size, until first
>   * successful copy_range (look at block_copy_do_copy).
>   */

Is this comment still correct? It appears that this branch of the if
statement does not always enable copy-range (the !use_copy_range case).

> -s->use_copy_range = use_copy_range;
> -s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
> +s->method = use_copy_range ? COPY_RANGE_SMALL : COPY_READ_WRITE;


signature.asc
Description: PGP signature

Re: [PATCH qemu v20] spapr: Implement Open Firmware client interface

[BALATON Zoltan]

On Thu, 27 May 2021, David Gibson wrote:

On Tue, May 25, 2021 at 12:08:45PM +0200, BALATON Zoltan wrote:

On Tue, 25 May 2021, David Gibson wrote:

On Mon, May 24, 2021 at 12:55:07PM +0200, BALATON Zoltan wrote:

On Mon, 24 May 2021, David Gibson wrote:

On Sun, May 23, 2021 at 07:09:26PM +0200, BALATON Zoltan wrote:

On Sun, 23 May 2021, BALATON Zoltan wrote:

On Sun, 23 May 2021, Alexey Kardashevskiy wrote:

One thing to note about PCI is that normally I think the client
expects the firmware to do PCI probing and SLOF does it. But VOF
does not and Linux scans PCI bus(es) itself. Might be a problem for
you kernel.


I'm not sure what info does MorphOS get from the device tree and what it
probes itself but I think it may at least need device ids and info about
the PCI bus to be able to access the config regs, after that it should
set the devices up hopefully. I could add these from the board code to
device tree so VOF does not need to do anything about it. However I'm
not getting to that point yet because it crashes on something that it's
missing and couldn't yet find out what is that.

I'd like to get Linux working now as that would be enough to test this
and then if for MorphOS we still need a ROM it's not a problem if at
least we can boot Linux without the original firmware. But I can't make
Linux open a serial console and I don't know what it needs for that. Do
you happen to know? I've looked at the sources in Linux/arch/powerpc but
not sure how it would find and open a serial port on pegasos2. It seems
to work with the board firmware and now I can get it to boot with VOF
but then it does not open serial so it probably needs something in the
device tree or expects the firmware to set something up that we should
add in pegasos2.c when using VOF.


I've now found that Linux uses rtas methods read-pci-config and
write-pci-config for PCI access on pegasos2 so this means that we'll
probably need rtas too (I hoped we could get away without it if it were only
used for shutdown/reboot or so but seems Linux needs it for PCI as well and
does not scan the bus and won't find some devices without it).


Yes, definitely sounds like you'll need an RTAS implementation.


I plan to fix that after managed to get serial working as that seems to not
need it. If I delete the rtas-size property from /rtas on the original
firmware that makes Linux skip instantiating rtas, but I still get serial
output just not accessing PCI devices. So I think it should work and keeps
things simpler at first. Then I'll try rtas later.


While VOF can do rtas, this causes a problem with the hypercall method using
sc 1 that goes through vhyp but trips the assert in ppc_store_sdr1() so
cannot work after guest is past quiesce.



So the question is why is that
assert there


Ah.. right.  So, vhyp was designed for the PAPR use case, where we
want to model the CPU when it's in supervisor and user mode, but not
when it's in hypervisor mode.  We want qemu to mimic the behaviour of
the hypervisor, rather than attempting to actually execute hypervisor
code in the virtual CPU.

On systems that have a hypervisor mode, SDR1 is hypervisor privileged,
so it makes no sense for the guest to attempt to set it.  That should
be caught by the general SPR code and turned into a 0x700, hence the
assert() if we somehow reach ppc_store_sdr1().

So, we are seeing a problem here because you want the 'sc 1'
interception of vhyp, but not the rest of the stuff that goes with it.


and would using sc 1 for hypercalls on pegasos2 cause other
problems later even if the assert could be removed?


At least in the short term, I think you probably can remove the
assert.  In your case the 'sc 1' calls aren't truly to a hypervisor,
but a special case escape to qemu for the firmware emulation.  I think
it's unlikely to cause problems later, because nothing on a 32-bit
system should be attempting an 'sc 1'.  The only thing I can think of
that would fail is some test case which explicitly verified that 'sc
1' triggered a 0x700 (SIGILL from userspace).


OK so the assert should check if the CPU has an HV bit. I think there was a
#detine for that somewhere that I can add to the assert then I can try that.
What I wasn't sure about is that sc 1 would conflict with the guest's usage
of normal sc calls or are these going through different paths and only sc 1
will trigger vhyp callback not affecting notmal sc calls?


The vhyp shouldn't affect normal system calls, 'sc 1' is specifically
for hypercalls, as opposed to normal 'sc' (a.k.a. 'sc 0'), and the
vhyp only intercepts the hypercall version (after all Linux on PAPR
certainly uses its own system calls, and hypercalls are active for the
lifetime of the guest there).


(Or if this causes
an otherwise unnecessary VM exit on KVM even when it works then maybe
looking for a different way in the future might be needed.


What you're doing here won't work with KVM as it stands.  There are
basically two paths into the vhyp hypercall path: 1) f

[PATCH] vfio: Fix unregister SaveVMHandler in vfio_migration_finalize

[Kunkun Jiang]

In the vfio_migration_init(), the SaveVMHandler is registered for
VFIO device. But it lacks the operation of 'unregister'. It will
lead to 'Segmentation fault (core dumped)' in
qemu_savevm_state_setup(), if performing live migration after a
VFIO device is hot deleted.

Fixes: 7c2f5f75f94 (vfio: Register SaveVMHandlers for VFIO device)
Reported-by: Qixin Gan 
Signed-off-by: Kunkun Jiang 
---
 hw/vfio/migration.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 201642d75e..ef397ebe6c 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -892,6 +892,7 @@ void vfio_migration_finalize(VFIODevice *vbasedev)
 
 remove_migration_state_change_notifier(&migration->migration_state);
 qemu_del_vm_change_state_handler(migration->vm_state);
+unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
 vfio_migration_exit(vbasedev);
 }
 
-- 
2.23.0

Re: [PATCH v2 0/7] block-copy: protect block-copy internal structures

[Stefan Hajnoczi]

On Tue, May 18, 2021 at 12:07:50PM +0200, Emanuele Giuseppe Esposito wrote:
> This serie of patches aims to reduce the usage of the global
> AioContexlock in block-copy, by introducing smaller granularity
> locks thus on making the block layer thread safe. 
> 
> This serie depends on Paolo's coroutine_sleep API and my previous
> serie that brings thread safety to the smaller API used by block-copy,
> like ratelimit, progressmeter abd co-shared-resource.
> 
> What's missing for block-copy to be fully thread-safe is fixing
> the CoSleep API to allow cross-thread sleep and wakeup.
> Paolo is working on it and will post the patches once his new
> CoSleep API is accepted.
> 
> Patch 1 introduces the .method field instead of .use_copy_range
> and .copy_size, so that it can be later used as atomic.
> Patch 2-3 provide comments and refactoring in preparation to
> the locks added in patch 4 on BlockCopyTask, patch 5-6 on
> BlockCopyCallState and 7 BlockCopyState.
> 
> Based-on: <20210517100548.28806-1-pbonz...@redhat.com>
> Based-on: <20210518094058.25952-1-eespo...@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito 

Here is my understanding of thread-safety in your
https://gitlab.com/eesposit/qemu.git dataplane_new branch:

Reading the code was much more satisfying than trying to review the
patches. That's probably because I'm not familiar with the block-copy.c
implementation and needed the context. I noticed you already addressed
some of Vladimir's comments in your git branch, so that may have helped
too.

The backup block job and the backup-top filter driver have a
BlockCopyState. QEMU threads that call bdrv_co_preadv/pwritev() invoke
the block_copy() coroutine function, so BlockCopyState needs to be
protected between threads. Additionally, the backup block job invokes
block_copy_async() to perform a background copy operation.

The relationships are as follows:

BlockCopyState - shared state that any thread can access
BlockCopyCallState - per-block_copy()/block_copy_async() state, not
 accessed by other coroutines/threads
BlockCopyTask - per-aiotask state, find_conflicting_task_locked()
accesses this for a given BlockCopyState

What is the purpose of the BlockCopyState->calls list?

Existing issue: the various
block_copy_call_finished/succeeded/failed/cancelled/status() APIs are a
bit extreme. They all end up being called by block/backup.c in
succession when it seems like a single call should be enough to report
the status.

It's not immediately obvious to me that BlockCopyCallState needs to be
thread-safe. So I wondered why finished needs to be atomic. Then I
realized the set_speed/cancel code runs in the monitor, so at least
block_copy_call_cancel() and block_copy_kick() need to be thread-safe. I
guess the BlockCopyCallState status APIs were made thread-safe for
consistency even though it's not needed at the moment?

Please add doc comments to block-copy.h explaining the thread-safety of
the APIs (it might be as simple as "all APIs are thread-safe" at the top
of the file).

Summarizing everything, this series adds BlockCopyState->lock to protect
shared state and makes BlockCopyCallState's status atomic so it can be
queried from threads other than the one performing the copy operation.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH 1/1] yank: Unregister function when using TLS migration

[Daniel P . Berrangé]

On Thu, May 27, 2021 at 08:23:52AM -0400, Peter Xu wrote:
> On Thu, May 27, 2021 at 09:46:54AM +0100, Daniel P. Berrangé wrote:
> > On Wed, May 26, 2021 at 05:58:58PM -0400, Peter Xu wrote:
> > > On Wed, May 26, 2021 at 11:21:03PM +0200, Lukas Straub wrote:
> > > > On Wed, 26 May 2021 16:40:35 -0400
> > > > Peter Xu  wrote:
> > > > 
> > > > > On Wed, May 26, 2021 at 05:05:40PM -0300, Leonardo Bras wrote:
> > > > > > After yank feature was introduced, whenever migration is started 
> > > > > > using TLS,
> > > > > > the following error happens in both source and destination hosts:
> > > > > > 
> > > > > > (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> > > > > > Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> > > > > > 
> > > > > > This happens because of a missing yank_unregister_function() when 
> > > > > > using
> > > > > > qio-channel-tls.
> > > > > > 
> > > > > > Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to 
> > > > > > perform
> > > > > > yank_unregister_function() in channel_close() and 
> > > > > > multifd_load_cleanup().
> > > > > > 
> > > > > > Fixes: 50186051f ("Introduce yank feature")
> > > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> > > > > > Signed-off-by: Leonardo Bras   
> > > > > 
> > > > > Leo,
> > > > > 
> > > > > Thanks for looking into it!
> > > > > 
> > > > > So before looking int the fix... I do have a doubt on why we only 
> > > > > enable yank
> > > > > on socket typed, as I think tls should also work with 
> > > > > qio_channel_shutdown().
> > > > > 
> > > > > IIUC the confused thing here is we register only for qio-socket, 
> > > > > however tls
> > > > > will actually call migration_channel_connect() twice, first with a 
> > > > > qio-socket,
> > > > > then with the real tls-socket.  For tls I feel like we have 
> > > > > registered with the
> > > > > wrong channel - instead of the wrapper socket ioc, we should register 
> > > > > to the
> > > > > final tls ioc?
> > > > > 
> > > > > Lukas, is there a reason?
> > > > > 
> > > > 
> > > > Hi,
> > > > There is no specific reason. Both ways work equally well in preventing
> > > > qemu from hanging. shutdown() for tls-channel just makes it abort a
> > > > little sooner (by not attempting to encrypt and send data anymore).
> > > > 
> > > > I don't lean either way. I guess registering it on the tls-channel
> > > > makes is a bit more explicit and clearer.
> > > 
> > > Agreed, because IMHO logically the migration code should not be aware of
> > > internals of IOChannels, e.g., we shouldn't need to know ioc->master is 
> > > the
> > > socket ioc of tls ioc to unregister.
> > 
> > I think it is atually better to ignore the TLS channel and *always* yank
> > on the undering socket IO channel. The yank functionality is intended to
> > be used in a scenario where we know the channels are broken.  If yank
> > calls the high level IO channel it is potentially going to try to do a
> > cleanup shutdown that we know will fail because of the broken network.
> 
> Could you elaborate what's the "cleanup shutdown"?
> 
> The yank calls migration_yank_iochannel:
> 
> void migration_yank_iochannel(void *opaque)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> 
> qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> }
> 
> Where qio_channel_shutdown for tls is nothing but delivers that to the master
> channel:
> 
> static int qio_channel_tls_shutdown(QIOChannel *ioc,
> QIOChannelShutdown how,
> Error **errp)
> {
> QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
> 
> qatomic_or(&tioc->shutdown, how);
> 
> return qio_channel_shutdown(tioc->master, how, errp);
> }
> 
> So I thought it was a nice wrapper just for things like this, and I didn't see
> anything it does more than the io_shutdown for the socket channel.  Did I miss
> something?

Today thats the case, but don't assume it will be the case forever.
There is a mechanism in TLS for doing clean shutdown which we've
debated including.

In general apps *can* just call the shutdown method on the QIOChannelTLS
object no matter what.  Yank is just a little bit special because of its
need to be guaranteed to work even when the network is dead. So yank
should always directly call the low level QIOChannelSocket, so thre is
a strong guarantee it can't block on something.


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: [PATCH v4 04/15] qemu-iotests: add option to attach gdbserver

[Vladimir Sementsov-Ogievskiy]

27.05.2021 14:06, Emanuele Giuseppe Esposito wrote:



On 26/05/2021 21:15, Vladimir Sementsov-Ogievskiy wrote:

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:

Define -gdb flag and GDB_OPTIONS environment variable
to python tests to attach a gdbserver to each qemu instance.
This patch only adds and parses this flag, it does not yet add
the implementation for it.

if -gdb is not provided but $GDB_OPTIONS is set, ignore the
environment variable.

Signed-off-by: Emanuele Giuseppe Esposito 
---
  tests/qemu-iotests/check  |  6 +-
  tests/qemu-iotests/iotests.py |  5 +
  tests/qemu-iotests/testenv.py | 19 ---
  3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index d1c87ceaf1..b9820fdaaf 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -33,6 +33,9 @@ def make_argparser() -> argparse.ArgumentParser:
 help='pretty print output for make check')
  p.add_argument('-d', dest='debug', action='store_true', help='debug')
+    p.add_argument('-gdb', action='store_true',
+   help="start gdbserver with $GDB_OPTIONS options \
+    ('localhost:12345' if $GDB_OPTIONS is empty)")
  p.add_argument('-misalign', action='store_true',
 help='misalign memory allocations')
  p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -112,7 +115,8 @@ if __name__ == '__main__':
  env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
    aiomode=args.aiomode, cachemode=args.cachemode,
    imgopts=args.imgopts, misalign=args.misalign,
-  debug=args.debug, valgrind=args.valgrind)
+  debug=args.debug, valgrind=args.valgrind,
+  gdb=args.gdb)
  testfinder = TestFinder(test_dir=env.source_iotests)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5d78de0f0b..d667fde6f8 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -75,6 +75,11 @@
  qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
  qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
+gdb_qemu_env = os.environ.get('GDB_OPTIONS')


should we specify a default? otherwise get() should raise an exception when 
GDB_OPTIONS is not set..

or you need os.getenv, which will return None if variable is absent.


No, os.environ.get does not raise any exception. I tested it myself, plus:
https://stackoverflow.com/questions/16924471/difference-between-os-getenv-and-os-environ-get


Ah, I'm wrong than. OK.






+qemu_gdb = []
+if gdb_qemu_env:
+    qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
+
  imgfmt = os.environ.get('IMGFMT', 'raw')
  imgproto = os.environ.get('IMGPROTO', 'file')
  output_dir = os.environ.get('OUTPUT_DIR', '.')
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 6d27712617..49ddd586ef 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -27,6 +27,7 @@
  import glob
  from typing import Dict, Any, Optional, ContextManager
+DEF_GDB_OPTIONS = 'localhost:12345'
  def isxfile(path: str) -> bool:
  return os.path.isfile(path) and os.access(path, os.X_OK)
@@ -72,7 +73,8 @@ class TestEnv(ContextManager['TestEnv']):
   'QEMU_NBD_OPTIONS', 'IMGOPTS', 'IMGFMT', 'IMGPROTO',
   'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
   'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 
'IMGOPTSSYNTAX',
- 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_']
+ 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
+ 'GDB_OPTIONS']
  def get_env(self) -> Dict[str, str]:
  env = {}
@@ -163,7 +165,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
   imgopts: Optional[str] = None,
   misalign: bool = False,
   debug: bool = False,
- valgrind: bool = False) -> None:
+ valgrind: bool = False,
+ gdb: bool = False) -> None:
  self.imgfmt = imgfmt
  self.imgproto = imgproto
  self.aiomode = aiomode
@@ -171,6 +174,14 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: 
str,
  self.misalign = misalign
  self.debug = debug
+    if gdb:
+    self.gdb_options = os.environ.get('GDB_OPTIONS', DEF_GDB_OPTIONS)


everywhere in the file os.getenv is used, let's be consistent on it.


+    if not self.gdb_options:
+    # cover the case 'export GDB_OPTIONS='
+    self.gdb_options = DEF_GDB_OPTIONS


Hmm, a bit strange to handle 'export X=' only for this new variable, we don't 
have such logic for other variables.. I'm not against still, if you need it.


+    elif 'GDB_OPTIONS' in os.environ:
+    del os.environ['GDB_OPTIONS']


Don't need 

Re: [PATCH v12 7/8] KVM: arm64: ioctl to fetch/store tags in a guest

[Catalin Marinas]

On Thu, May 27, 2021 at 08:50:30AM +0100, Steven Price wrote:
> On 24/05/2021 19:11, Catalin Marinas wrote:
> > I had some (random) thoughts on how to make things simpler, maybe. I
> > think most of these races would have been solved if we required PROT_MTE
> > in the VMM but this has an impact on the VMM if it wants to use MTE
> > itself. If such requirement was in place, all KVM needed to do is check
> > PG_mte_tagged.
> > 
> > So what we actually need is a set_pte_at() in the VMM to clear the tags
> > and set PG_mte_tagged. Currently, we only do this if the memory type is
> > tagged (PROT_MTE) but it's not strictly necessary.
> > 
> > As an optimisation for normal programs, we don't want to do this all the
> > time but the visible behaviour wouldn't change (well, maybe for ptrace
> > slightly). However, it doesn't mean we couldn't for a VMM, with an
> > opt-in via prctl(). This would add a MMCF_MTE_TAG_INIT bit (couldn't
> > think of a better name) to mm_context_t.flags and set_pte_at() would
> > behave as if the pte was tagged without actually mapping the memory in
> > user space as tagged (protection flags not changed). Pages that don't
> > support tagging are still safe, just some unnecessary ignored tag
> > writes. This would need to be set before the mmap() for the guest
> > memory.
> > 
> > If we want finer-grained control we'd have to store this information in
> > the vma flags, in addition to VM_MTE (e.g. VM_MTE_TAG_INIT) but without
> > affecting the actual memory type. The easiest would be another pte bit,
> > though we are short on them. A more intrusive (not too bad) approach is
> > to introduce a set_pte_at_vma() and read the flags directly in the arch
> > code. In most places where set_pte_at() is called on a user mm, the vma
> > is also available.
> > 
> > Anyway, I'm not saying we go this route, just thinking out loud, get
> > some opinions.
> 
> Does get_user_pages() actually end up calling set_pte_at() normally?

Not always, at least as how it's called from hva_to_pfn(). My reading of
the get_user_page_fast_only() is that it doesn't touch the pte, just
walks the page tables and pins the page. Of course, it expects a valid
pte to have been set in the VMM already, otherwise it doesn't pin any
page and the caller falls back to the slow path.

The slow path, get_user_pages_unlocked(), passes FOLL_TOUCH and
set_pte_at() will be called either in follow_pfn_pte() if it was valid
or via faultin_page() -> handle_mm_fault().

> If not then on the normal user_mem_abort() route although we can
> easily check VM_MTE_TAG_INIT there's no obvious place to hook in to
> ensure that the pages actually allocated have the PG_mte_tagged flag.

I don't think it helps if we checked such vma flag in user_mem_abort(),
we'd still have the race with set_pte_at() on the page flags. What I was
trying to avoid is touching the page flags in too many places, so
deferring this always to set_pte_at() in the VMM.

> I'm also not sure how well this would work with the MMU notifiers path
> in KVM. With MMU notifiers (i.e. the VMM replacing a page in the
> memslot) there's not even an obvious hook to enforce the VMA flag. So I
> think we'd end up with something like the sanitise_mte_tags() function
> to at least check that the PG_mte_tagged flag is set on the pages
> (assuming that the trigger for the MMU notifier has done the
> corresponding set_pte_at()). Admittedly this might close the current
> race documented there.

If we kept this check to the VMM set_pte_at(), I think we can ignore the
notifiers.

> It also feels wrong to me to tie this to a process with prctl(), it
> seems much more normal to implement this as a new mprotect() flag as
> this is really a memory property not a process property. And I think
> we'll find some scary corner cases if we try to associate everything
> back to a process - although I can't instantly think of anything that
> will actually break.

I agree, tying it to the process looks wrong, only that it's less
intrusive. I don't think it would break anything, only potential
performance regression. A process would still need to pass PROT_MTE to
be able to get tag checking. That's basically what I had in an early MTE
implementation with clear_user_page() always zeroing the tags.

I agree with you that a vma flag would be better but it's more
complicated without an additional pte bit. We could also miss some
updates as mprotect() for example checks for pte_same() before calling
set_pte_at() (it would need to check the updated vma flags).

I'll review the latest series but I'm tempted to move the logic in
santise_mte_tags() to mte.c and take the big lock in there if
PG_mte_tagged is not already set. If we hit performance issues, we can
optimise this later to have the page flag set already on creation (new
PROT flag, prctl etc.).

-- 
Catalin

[Bug 1829682] Re: QEMU PPC SYSTEM regression - 3.1.0 and GIT - Fail to boot AIX

[Thomas Huth]

*** This bug is a duplicate of bug 1874264 ***
https://bugs.launchpad.net/bugs/1874264

We already have a different ticket to track the AIX 7.2 issue here:
 https://gitlab.com/qemu-project/qemu/-/issues/269
Please continue with the discussion there instead, thanks!

** This bug has been marked a duplicate of bug 1874264
   AIX 7.2 TL4 SP1 cannot IPL with QEMU >2.11.2 ppc64-softmmu

** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #269
   https://gitlab.com/qemu-project/qemu/-/issues/269

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1829682

Title:
  QEMU PPC SYSTEM regression - 3.1.0 and GIT - Fail to boot AIX

Status in QEMU:
  Incomplete

Bug description:
  Built from source on a debian system

  Linux db08 4.9.0-8-amd64 #1 SMP Debian 4.9.130-2 (2018-10-27) x86_64 GNU/Linux
  gcc version 6.3.0 20170516 (Debian 6.3.0-18+deb9u1)

  Last git commit (from queued gdibson repository)

  starting AIX 7.2 TL 2 SP 2 with the following : (the install was done
  under qemu 3.1.0)

  qemu-system-ppc64 -M pseries \
  -cpu power7 \
  -cdrom AIX_v7.2_Install_7200-02-02-1806_DVD_1_of_2_32018.iso \
  -net nic \
  -net tap,ifname=tap2,script=no \
  -drive file=DISK1.IMG,if=none,id=drive-virtio-disk0 \
  -device virtio-scsi-pci,id=scsi -device scsi-hd,drive=drive-virtio-disk0 \
  -m 4G \
  -serial stdio \
  -monitor unix:ms,server,nowait \
  -accel tcg \
  -k fr \
  -nographic \
  -prom-env input-device=/vdevice/vty@7100 \
  -prom-env output-device=/vdevice/vty@7100 \
  -prom-env diag-switch?=false \
  -prom-env boot-command="boot 
/pci@8002000/scsi@2/disk@100 -s verbose"

  Yields this :

  
  ^M
  SLOF^[[0m^[[?25l 
**^M
  ^[[1mQEMU Starting^M
  ^[[0m Build Date = Jan 14 2019 18:00:39^M
   FW Version = git-a5b428e1c1eae703^M
   Press "s" to enter Open Firmware.^M^M
  ^M^M
  
^[[0m^[[?25hC^MC0100^MC0120^MC0140^MC0200^MC0240^MC0260^MC02E0^MC0300^MC0320^MC0340^MC0360^MC0370^MC0380^MC0371^MC0372^MC0373^MC0374^MC03F0^MC0400^MC0480^MC04C0^MC04D0^MC0500^MPopulating
 /vdevice methods^M
  Populating /vdevice/vty@7100^M
  Populating /vdevice/nvram@7101^M
  Populating /vdevice/l-lan@7102^M
  Populating /vdevice/v-scsi@7103^M
 SCSI: Looking for devices^M
8200 CD-ROM   : "QEMU QEMU CD-ROM  2.5+"^M
  C05A0^MPopulating /pci@8002000^M
   00  (D) : 1234 qemu vga^M
   00 0800 (D) : 1033 0194serial bus [ usb-xhci ]^M
   00 1000 (D) : 1af4 1004virtio [ scsi ]^M
  Populating /pci@8002000/scsi@2^M
 SCSI: Looking for devices^M
100 DISK : "QEMU QEMU HARDDISK2.5+"^M
  C0600^MC06C0^MC0700^MC0800^MC0880^MC0890^MC08A0^MC08A8^MInstalling QEMU fb^M
  ^M
  ^M
  ^M
  C08B0^MScanning USB ^M
XHCI: Initializing^M
  USB Keyboard ^M
  USB mouse ^M
  C08C0^MC08D0^MNo console specified using screen & keyboard^M
  User selected input-device console: /vdevice/vty@7100^M
  User selected output-device console: /vdevice/vty@7100^M
  C08E0^MC08E8^MC08FF^M ^M
Welcome to Open Firmware^M
  ^M
Copyright (c) 2004, 2017 IBM Corporation All rights reserved.^M
This program and the accompanying materials are made available^M
under the terms of the BSD License available at^M
http://www.opensource.org/licenses/bsd-license.php^M
  ^M
  ^M
  Trying to load: -s verbose from: 
/pci@8002000/scsi@2/disk@100 ...   Successfully loaded^M
  ^M
  ---> qemu,pseries detected <---^M
  ^M
  ^M
  ^M
  ^M
  ^M
  ^M
  ^M
  
---^M
  Welcome to AIX.^M
 boot image timestamp: 05:56:13 04/20/2019^M
  processor count: 1;  memory size: 4096MB;  kernel size: 38426884^M
   boot device: /pci@8002000/scsi@2/disk@100^M
  ^M
  8000FFEC bytes of free memory remain at address 7FFF0014^M
  load address: 0x4000   aixmon size: 0x000D2C00   boot image size: 
0x01A6B430^M
  ^LAIX vm,uuid property contains invalid data^Mload address: 0x4000   
aixmon size: 0x000D2C00   boot image size: 0x01A6B430^M
  ^LAIX vm,uuid property contains invalid data^M
  get_ppp return code: 0xFFFE^M
  ^M
  AKVM: hcall-multi-tce detected but overridden, allow with "multce" boot 
argument^M
  The temporary memory region list is at 1 percent capacity.^M
  The temporary IPLCB is at 1 percent capacity.^M
  The IPLCB address is 0x0FFF9000^M
  name offset   size^M
  ipl_cb_and_bit_map  ..5958^M
  bit_map... 0790 ..0006^M
  ipl_info.. 01C8 ..00

Re: [PATCH 1/1] yank: Unregister function when using TLS migration

[Peter Xu]

On Thu, May 27, 2021 at 09:46:54AM +0100, Daniel P. Berrangé wrote:
> On Wed, May 26, 2021 at 05:58:58PM -0400, Peter Xu wrote:
> > On Wed, May 26, 2021 at 11:21:03PM +0200, Lukas Straub wrote:
> > > On Wed, 26 May 2021 16:40:35 -0400
> > > Peter Xu  wrote:
> > > 
> > > > On Wed, May 26, 2021 at 05:05:40PM -0300, Leonardo Bras wrote:
> > > > > After yank feature was introduced, whenever migration is started 
> > > > > using TLS,
> > > > > the following error happens in both source and destination hosts:
> > > > > 
> > > > > (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> > > > > Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> > > > > 
> > > > > This happens because of a missing yank_unregister_function() when 
> > > > > using
> > > > > qio-channel-tls.
> > > > > 
> > > > > Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to perform
> > > > > yank_unregister_function() in channel_close() and 
> > > > > multifd_load_cleanup().
> > > > > 
> > > > > Fixes: 50186051f ("Introduce yank feature")
> > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> > > > > Signed-off-by: Leonardo Bras   
> > > > 
> > > > Leo,
> > > > 
> > > > Thanks for looking into it!
> > > > 
> > > > So before looking int the fix... I do have a doubt on why we only 
> > > > enable yank
> > > > on socket typed, as I think tls should also work with 
> > > > qio_channel_shutdown().
> > > > 
> > > > IIUC the confused thing here is we register only for qio-socket, 
> > > > however tls
> > > > will actually call migration_channel_connect() twice, first with a 
> > > > qio-socket,
> > > > then with the real tls-socket.  For tls I feel like we have registered 
> > > > with the
> > > > wrong channel - instead of the wrapper socket ioc, we should register 
> > > > to the
> > > > final tls ioc?
> > > > 
> > > > Lukas, is there a reason?
> > > > 
> > > 
> > > Hi,
> > > There is no specific reason. Both ways work equally well in preventing
> > > qemu from hanging. shutdown() for tls-channel just makes it abort a
> > > little sooner (by not attempting to encrypt and send data anymore).
> > > 
> > > I don't lean either way. I guess registering it on the tls-channel
> > > makes is a bit more explicit and clearer.
> > 
> > Agreed, because IMHO logically the migration code should not be aware of
> > internals of IOChannels, e.g., we shouldn't need to know ioc->master is the
> > socket ioc of tls ioc to unregister.
> 
> I think it is atually better to ignore the TLS channel and *always* yank
> on the undering socket IO channel. The yank functionality is intended to
> be used in a scenario where we know the channels are broken.  If yank
> calls the high level IO channel it is potentially going to try to do a
> cleanup shutdown that we know will fail because of the broken network.

Could you elaborate what's the "cleanup shutdown"?

The yank calls migration_yank_iochannel:

void migration_yank_iochannel(void *opaque)
{
QIOChannel *ioc = QIO_CHANNEL(opaque);

qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
}

Where qio_channel_shutdown for tls is nothing but delivers that to the master
channel:

static int qio_channel_tls_shutdown(QIOChannel *ioc,
QIOChannelShutdown how,
Error **errp)
{
QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);

qatomic_or(&tioc->shutdown, how);

return qio_channel_shutdown(tioc->master, how, errp);
}

So I thought it was a nice wrapper just for things like this, and I didn't see
anything it does more than the io_shutdown for the socket channel.  Did I miss
something?

Thanks,

-- 
Peter Xu

Re: [PATCH 6/9] gitlab-ci: Split gprof-gcov job

[Alex Bennée]

Thomas Huth  writes:

> On 25/05/2021 12.21, Philippe Mathieu-Daudé wrote:
>> On 5/25/21 10:25 AM, Philippe Mathieu-Daudé wrote:
>>> This job is hitting the 70min limit, so split it in 2 tasks.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé 
>>> ---
>>>   .gitlab-ci.d/buildtest.yml | 17 ++---
>>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
>>> index f903fdea3f4..894732e203d 100644
>>> --- a/.gitlab-ci.d/buildtest.yml
>>> +++ b/.gitlab-ci.d/buildtest.yml
>>> @@ -564,16 +564,27 @@ check-deprecated:
>>> allow_failure: true
>>> # gprof/gcov are GCC features
>>> -gprof-gcov:
>>> +build-gprof-gcov:
>>> extends: .native_build_job_template
>>> needs:
>>>   job: amd64-ubuntu2004-container
>>> variables:
>>>   IMAGE: ubuntu2004
>>>   CONFIGURE_ARGS: --enable-gprof --enable-gcov
>>> -MAKE_CHECK_ARGS: check
>>>   TARGETS: aarch64-softmmu ppc64-softmmu s390x-softmmu x86_64-softmmu
>>> -  timeout: 70m
>>> +  artifacts:
>>> +expire_in: 1 days
>>> +paths:
>>> +  - build
>> FYI this job takes 28min:
>> https://gitlab.com/philmd/qemu/-/jobs/1290778672
>> 
>>> +
>>> +check-gprof-gcov:
>>> +  extends: .native_test_job_template
>>> +  needs:
>>> +- job: build-gprof-gcov
>>> +  artifacts: true
>>> +  variables:
>>> +IMAGE: ubuntu2004
>>> +MAKE_CHECK_ARGS: check
>>> after_script:
>>>   - ${CI_PROJECT_DIR}/scripts/ci/coverage-summary.sh
>> and this one 23min:
>> https://gitlab.com/philmd/qemu/-/jobs/1290778751
>
> So why are they taking less than 70 minutes when split in two parts,
> but are exceeding  the 70 minutes when done in one job?
> That does not make sense...

The time just seems to be highly variable. Normally I see succeeding
runs take ~40 minutes. I suspect there is some aspect of the profiling
code that interacts poorly under load leading to much longer runtimes.

-- 
Alex Bennée

[Bug 1846816] Re: Booting error on AIX 6.1 "Illegal Trap Instruction Interrupt in Kernel""

[Thomas Huth]

Ok, so closing this ticket since AIX older than 7.2 cannot work. For AIX
>= 7.2, we already have a different ticket opened instead:
https://gitlab.com/qemu-project/qemu/-/issues/269

** Bug watch added: gitlab.com/qemu-project/qemu/-/issues #269
   https://gitlab.com/qemu-project/qemu/-/issues/269

** Changed in: qemu
   Status: New => Won't Fix

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1846816

Title:
  Booting error on AIX 6.1 "Illegal Trap Instruction Interrupt in
  Kernel""

Status in QEMU:
  Won't Fix

Bug description:
  # ls -ltr
  total 8750584
  -rw-rw-r--  1 linux linux 4274997248 Oct  4 18:33 AIX.vol1.iso
  -rw-rw-r--  1 linux linux 4293888000 Oct  4 18:45 AIX.vol2.iso
  -rw-rw-r--  1 linux linux  391485440 Oct  4 18:50 AIX.vol3.iso
  -rw-r--r--  1 root  root  204608 Oct  4 19:00 AIX61.img

  # qemu-system-ppc64 -cpu POWER8,compat=power7 -machine pseries -m 8192 
-serial mon:stdio \
  > -drive file=/qemu/AIX61.img,if=none,id=drive-virtio-disk0 \
  > -device virtio-scsi-pci,id=scsi -device scsi-hd,drive=drive-virtio-disk0 \
  > -cdrom /qemu/AIX.vol1.iso \
  > -prom-env boot-command='boot cdrom: -s verbose'

  VNC server running on ::1:5900
  qemu-system-ppc64: warning: TCG doesn't support requested feature, 
cap-ibs=workaround

  SLOF **
  QEMU Starting
   Build Date = Jul  3 2019 12:26:14
   FW Version = git-ba1ab360eebe6338
   Press "s" to enter Open Firmware.

  Populating /vdevice methods
  Populating /vdevice/vty@7100
  Populating /vdevice/nvram@7101
  Populating /vdevice/l-lan@7102
  Populating /vdevice/v-scsi@7103
     SCSI: Looking for devices
    8200 CD-ROM   : "QEMU QEMU CD-ROM  2.5+"
  Populating /pci@8002000
   00  (D) : 1234 qemu vga
   00 0800 (D) : 1033 0194serial bus [ usb-xhci ]
   00 1000 (D) : 1af4 1004virtio [ scsi ]
  Populating /pci@8002000/scsi@2
     SCSI: Looking for devices
    100 DISK : "QEMU QEMU HARDDISK2.5+"
  Installing QEMU fb

  Scanning USB
    XHCI: Initializing
  USB Keyboard
  USB mouse
  No console specified using screen & keyboard

    Welcome to Open Firmware

    Copyright (c) 2004, 2017 IBM Corporation All rights reserved.
    This program and the accompanying materials are made available
    under the terms of the BSD License available at
    http://www.opensource.org/licenses/bsd-license.php

  Trying to load: -s verbose from: 
/vdevice/v-scsi@7103/disk@8200: ...   Successfully loaded
  qemu-system-ppc64: Couldn't negotiate a suitable PVR during CAS
  AIX
  StarLED{814}

  AIX Version 6.1
  exec(/etc/init){1,0}

  INIT: EXECUTING /sbin/rc.boot 1
  exec(/usr/bin/sh,-c,/sbin/rc.boot 1){1114146,1}
  exec(/sbin/rc.boot,/sbin/rc.boot,1){1114146,1}
  + PHASE=1
  + + bootinfo -p
  exec(/usr/sbin/bootinfo,-p){1179684,1114146}
  PLATFORM=chrp
  + [ ! -x /usr/lib/boot/bin/bootinfo_chrp ]
  + [ 1 -eq 1 ]
  + 1> /usr/lib/libc.a
  + init -c unlink /usr/lib/boot/bin/!(*_chrp)
  exec(/etc/init,-c,unlink /usr/lib/boot/bin/!(*_chrp)){1179686,1114146}
  + chramfs -t
  exec(/usr/sbin/chramfs,-t){1179688,1114146}
  + init -c unlink /usr/sbin/chramfs
  + 1> /dev/null
  exec(/etc/init,-c,unlink /usr/sbin/chramfs){1179690,1114146}
  + + bootinfo -t
  exec(/usr/sbin/bootinfo,-t){1179692,1114146}
  BOOTYPE=3
  + [ 0 -ne 0 ]
  + [ -z 3 ]
  + unset pdev_to_ldev undolt native_netboot_cfg
  + unset disknet_odm_init config_ATM
  + /usr/lib/methods/showled 0x510 DEV CFG 1 START
  exec(/usr/lib/methods/showled,0x510,DEV CFG 1 START){1179694,1114146}
  + cfgmgr -f -v
  exec(/usr/sbin/cfgmgr,-f,-v){1179696,1114146}
  cfgmgr is running in phase 1
  
  Time: 0 LEDS: 0x538
  Invoking top level program -- "/etc/methods/defsys"
  exec(/bin/sh,-c,/etc/methods/defsys ){1245222,1179696}
  exec(/etc/methods/defsys){1245222,1179696}
  exec(/bin/sh,-c,/usr/lib/methods/define_rspc -n -c sys -s node -t 
chrp){1310760,1245222}
  exec(/usr/lib/methods/define_rspc,-n,-c,sys,-s,node,-t,chrp){1310760,1245222}
  Time: 0 LEDS: 0x539
  Return code = 0
  * stdout *
  sys0

  *** no stderr 
  
  Attempting to configure device 'sys0'
  Time: 0 LEDS: 0x811
  Invoking /usr/lib/methods/cfgsys_chrp -1 -l sys0
  exec(/bin/sh,-c,/usr/lib/methods/cfgsys_chrp -1 -l sys0){1245224,1179696}
  Number of running methods: 1
  exec(/usr/lib/methods/cfgsys_chrp,-1,-l,sys0){1245224,1179696}
  LED{A20}
  Illegal Trap Instruction Interrupt in Kernel
  04151A74  tweqir0,0r0=0
  KDB(0)>

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1846816/+subscriptions

Re: [PATCH 1/1] yank: Unregister function when using TLS migration

[Peter Xu]

On Thu, May 27, 2021 at 01:37:42PM +0100, Daniel P. Berrangé wrote:
> On Thu, May 27, 2021 at 08:23:52AM -0400, Peter Xu wrote:
> > On Thu, May 27, 2021 at 09:46:54AM +0100, Daniel P. Berrangé wrote:
> > > On Wed, May 26, 2021 at 05:58:58PM -0400, Peter Xu wrote:
> > > > On Wed, May 26, 2021 at 11:21:03PM +0200, Lukas Straub wrote:
> > > > > On Wed, 26 May 2021 16:40:35 -0400
> > > > > Peter Xu  wrote:
> > > > > 
> > > > > > On Wed, May 26, 2021 at 05:05:40PM -0300, Leonardo Bras wrote:
> > > > > > > After yank feature was introduced, whenever migration is started 
> > > > > > > using TLS,
> > > > > > > the following error happens in both source and destination hosts:
> > > > > > > 
> > > > > > > (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> > > > > > > Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> > > > > > > 
> > > > > > > This happens because of a missing yank_unregister_function() when 
> > > > > > > using
> > > > > > > qio-channel-tls.
> > > > > > > 
> > > > > > > Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to 
> > > > > > > perform
> > > > > > > yank_unregister_function() in channel_close() and 
> > > > > > > multifd_load_cleanup().
> > > > > > > 
> > > > > > > Fixes: 50186051f ("Introduce yank feature")
> > > > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> > > > > > > Signed-off-by: Leonardo Bras   
> > > > > > 
> > > > > > Leo,
> > > > > > 
> > > > > > Thanks for looking into it!
> > > > > > 
> > > > > > So before looking int the fix... I do have a doubt on why we only 
> > > > > > enable yank
> > > > > > on socket typed, as I think tls should also work with 
> > > > > > qio_channel_shutdown().
> > > > > > 
> > > > > > IIUC the confused thing here is we register only for qio-socket, 
> > > > > > however tls
> > > > > > will actually call migration_channel_connect() twice, first with a 
> > > > > > qio-socket,
> > > > > > then with the real tls-socket.  For tls I feel like we have 
> > > > > > registered with the
> > > > > > wrong channel - instead of the wrapper socket ioc, we should 
> > > > > > register to the
> > > > > > final tls ioc?
> > > > > > 
> > > > > > Lukas, is there a reason?
> > > > > > 
> > > > > 
> > > > > Hi,
> > > > > There is no specific reason. Both ways work equally well in preventing
> > > > > qemu from hanging. shutdown() for tls-channel just makes it abort a
> > > > > little sooner (by not attempting to encrypt and send data anymore).
> > > > > 
> > > > > I don't lean either way. I guess registering it on the tls-channel
> > > > > makes is a bit more explicit and clearer.
> > > > 
> > > > Agreed, because IMHO logically the migration code should not be aware of
> > > > internals of IOChannels, e.g., we shouldn't need to know ioc->master is 
> > > > the
> > > > socket ioc of tls ioc to unregister.
> > > 
> > > I think it is atually better to ignore the TLS channel and *always* yank
> > > on the undering socket IO channel. The yank functionality is intended to
> > > be used in a scenario where we know the channels are broken.  If yank
> > > calls the high level IO channel it is potentially going to try to do a
> > > cleanup shutdown that we know will fail because of the broken network.
> > 
> > Could you elaborate what's the "cleanup shutdown"?
> > 
> > The yank calls migration_yank_iochannel:
> > 
> > void migration_yank_iochannel(void *opaque)
> > {
> > QIOChannel *ioc = QIO_CHANNEL(opaque);
> > 
> > qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> > }
> > 
> > Where qio_channel_shutdown for tls is nothing but delivers that to the 
> > master
> > channel:
> > 
> > static int qio_channel_tls_shutdown(QIOChannel *ioc,
> > QIOChannelShutdown how,
> > Error **errp)
> > {
> > QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
> > 
> > qatomic_or(&tioc->shutdown, how);
> > 
> > return qio_channel_shutdown(tioc->master, how, errp);
> > }
> > 
> > So I thought it was a nice wrapper just for things like this, and I didn't 
> > see
> > anything it does more than the io_shutdown for the socket channel.  Did I 
> > miss
> > something?
> 
> Today thats the case, but don't assume it will be the case forever.
> There is a mechanism in TLS for doing clean shutdown which we've
> debated including.
> 
> In general apps *can* just call the shutdown method on the QIOChannelTLS
> object no matter what.  Yank is just a little bit special because of its
> need to be guaranteed to work even when the network is dead. So yank
> should always directly call the low level QIOChannelSocket, so thre is
> a strong guarantee it can't block on something.

Hmm, I am still not fully convinced that that's a valid reason the migration
code should be aware of how the socket is managed in tls channels...

Does that sound more like a good reason to introduce QIOChannelShutdown with
QIO_CHANNEL_SHUTDOWN_FORCE so it'll always not block if FORCE set?  Then we ca

Re: [PATCH 1/1] yank: Unregister function when using TLS migration

[Daniel P . Berrangé]

On Thu, May 27, 2021 at 09:09:09AM -0400, Peter Xu wrote:
> On Thu, May 27, 2021 at 01:37:42PM +0100, Daniel P. Berrangé wrote:
> > On Thu, May 27, 2021 at 08:23:52AM -0400, Peter Xu wrote:
> > > On Thu, May 27, 2021 at 09:46:54AM +0100, Daniel P. Berrangé wrote:
> > > > On Wed, May 26, 2021 at 05:58:58PM -0400, Peter Xu wrote:
> > > > > On Wed, May 26, 2021 at 11:21:03PM +0200, Lukas Straub wrote:
> > > > > > On Wed, 26 May 2021 16:40:35 -0400
> > > > > > Peter Xu  wrote:
> > > > > > 
> > > > > > > On Wed, May 26, 2021 at 05:05:40PM -0300, Leonardo Bras wrote:
> > > > > > > > After yank feature was introduced, whenever migration is 
> > > > > > > > started using TLS,
> > > > > > > > the following error happens in both source and destination 
> > > > > > > > hosts:
> > > > > > > > 
> > > > > > > > (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> > > > > > > > Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> > > > > > > > 
> > > > > > > > This happens because of a missing yank_unregister_function() 
> > > > > > > > when using
> > > > > > > > qio-channel-tls.
> > > > > > > > 
> > > > > > > > Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to 
> > > > > > > > perform
> > > > > > > > yank_unregister_function() in channel_close() and 
> > > > > > > > multifd_load_cleanup().
> > > > > > > > 
> > > > > > > > Fixes: 50186051f ("Introduce yank feature")
> > > > > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> > > > > > > > Signed-off-by: Leonardo Bras   
> > > > > > > 
> > > > > > > Leo,
> > > > > > > 
> > > > > > > Thanks for looking into it!
> > > > > > > 
> > > > > > > So before looking int the fix... I do have a doubt on why we only 
> > > > > > > enable yank
> > > > > > > on socket typed, as I think tls should also work with 
> > > > > > > qio_channel_shutdown().
> > > > > > > 
> > > > > > > IIUC the confused thing here is we register only for qio-socket, 
> > > > > > > however tls
> > > > > > > will actually call migration_channel_connect() twice, first with 
> > > > > > > a qio-socket,
> > > > > > > then with the real tls-socket.  For tls I feel like we have 
> > > > > > > registered with the
> > > > > > > wrong channel - instead of the wrapper socket ioc, we should 
> > > > > > > register to the
> > > > > > > final tls ioc?
> > > > > > > 
> > > > > > > Lukas, is there a reason?
> > > > > > > 
> > > > > > 
> > > > > > Hi,
> > > > > > There is no specific reason. Both ways work equally well in 
> > > > > > preventing
> > > > > > qemu from hanging. shutdown() for tls-channel just makes it abort a
> > > > > > little sooner (by not attempting to encrypt and send data anymore).
> > > > > > 
> > > > > > I don't lean either way. I guess registering it on the tls-channel
> > > > > > makes is a bit more explicit and clearer.
> > > > > 
> > > > > Agreed, because IMHO logically the migration code should not be aware 
> > > > > of
> > > > > internals of IOChannels, e.g., we shouldn't need to know ioc->master 
> > > > > is the
> > > > > socket ioc of tls ioc to unregister.
> > > > 
> > > > I think it is atually better to ignore the TLS channel and *always* yank
> > > > on the undering socket IO channel. The yank functionality is intended to
> > > > be used in a scenario where we know the channels are broken.  If yank
> > > > calls the high level IO channel it is potentially going to try to do a
> > > > cleanup shutdown that we know will fail because of the broken network.
> > > 
> > > Could you elaborate what's the "cleanup shutdown"?
> > > 
> > > The yank calls migration_yank_iochannel:
> > > 
> > > void migration_yank_iochannel(void *opaque)
> > > {
> > > QIOChannel *ioc = QIO_CHANNEL(opaque);
> > > 
> > > qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> > > }
> > > 
> > > Where qio_channel_shutdown for tls is nothing but delivers that to the 
> > > master
> > > channel:
> > > 
> > > static int qio_channel_tls_shutdown(QIOChannel *ioc,
> > > QIOChannelShutdown how,
> > > Error **errp)
> > > {
> > > QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
> > > 
> > > qatomic_or(&tioc->shutdown, how);
> > > 
> > > return qio_channel_shutdown(tioc->master, how, errp);
> > > }
> > > 
> > > So I thought it was a nice wrapper just for things like this, and I 
> > > didn't see
> > > anything it does more than the io_shutdown for the socket channel.  Did I 
> > > miss
> > > something?
> > 
> > Today thats the case, but don't assume it will be the case forever.
> > There is a mechanism in TLS for doing clean shutdown which we've
> > debated including.
> > 
> > In general apps *can* just call the shutdown method on the QIOChannelTLS
> > object no matter what.  Yank is just a little bit special because of its
> > need to be guaranteed to work even when the network is dead. So yank
> > should always directly call the low level QIOChannelSocket, so thre is
> > a strong guarantee it can't bl

Re: Windows fails to boot after rebase to QEMU master

[Dr. David Alan Gilbert]

* Claudio Fontana (cfont...@suse.de) wrote:
> On 5/27/21 12:53 PM, Claudio Fontana wrote:
> > On 5/27/21 11:48 AM, Claudio Fontana wrote:
> >> On 5/27/21 11:15 AM, Dr. David Alan Gilbert wrote:
> >>> * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
>  On 5/27/21 10:31 AM, Dr. David Alan Gilbert wrote:
> > * Claudio Fontana (cfont...@suse.de) wrote:
> >> On 5/26/21 9:30 PM, Dr. David Alan Gilbert wrote:
> >>> * Michael S. Tsirkin (m...@redhat.com) wrote:
>  On Fri, May 21, 2021 at 11:17:19AM +0200, Siddharth Chandrasekaran 
>  wrote:
> > After a rebase to QEMU master, I am having trouble booting windows 
> > VMs.
> > Git bisect indicates commit f5cc5a5c1686 ("i386: split cpu 
> > accelerators
> > from cpu.c, using AccelCPUClass") to have introduced the issue. I 
> > spent
> > some time looking at into it yesterday without much luck.
> >
> > Steps to reproduce:
> >
> > $ ./configure --enable-kvm --disable-xen 
> > --target-list=x86_64-softmmu --enable-debug
> > $ make -j `nproc`
> > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> > -cpu 
> > host,hv_synic,hv_vpindex,hv_time,hv_runtime,hv_stimer,hv_crash \
> > -enable-kvm \
> > -name test,debug-threads=on \
> > -smp 1,threads=1,cores=1,sockets=1 \
> > -m 4G \
> > -net nic -net user \
> > -boot d,menu=on \
> > -usbdevice tablet \
> > -vnc :3 \
> > -machine q35,smm=on \
> > -drive 
> > if=pflash,format=raw,readonly=on,unit=0,file="../OVMF_CODE.secboot.fd"
> >  \
> > -drive 
> > if=pflash,format=raw,unit=1,file="../OVMF_VARS.secboot.fd" \
> > -global ICH9-LPC.disable_s3=1 \
> > -global driver=cfi.pflash01,property=secure,value=on \
> > -cdrom "../Windows_Server_2016_14393.ISO" \
> > -drive 
> > file="../win_server_2016.qcow2",format=qcow2,if=none,id=rootfs_drive
> >  \
> > -device ahci,id=ahci \
> > -device ide-hd,drive=rootfs_drive,bus=ahci.0
> >
> > If the issue is not obvious, I'd like some pointers on how to go 
> > about
> > fixing this issue.
> >
> > ~ Sid.
> >
> 
>  At a guess this commit inadvertently changed something in the CPU ID.
>  I'd start by using a linux guest to dump cpuid before and after the
>  change.
> >>>
> >>> I've not had a chance to do that yet, however I did just end up with a
> >>> bisect of a linux guest failure bisecting to the same patch:
> >>>
> >>> [dgilbert@dgilbert-t580 qemu]$ git bisect bad
> >>> f5cc5a5c168674f84bf061cdb307c2d25fba5448 is the first bad commit
> >>> commit f5cc5a5c168674f84bf061cdb307c2d25fba5448
> >>> Author: Claudio Fontana 
> >>> Date:   Mon Mar 22 14:27:40 2021 +0100
> >>>
> >>> i386: split cpu accelerators from cpu.c, using AccelCPUClass
> >>> 
> >>> i386 is the first user of AccelCPUClass, allowing to split
> >>> cpu.c into:
> >>> 
> >>> cpu.ccpuid and common x86 cpu functionality
> >>> host-cpu.c   host x86 cpu functions and "host" cpu type
> >>> kvm/kvm-cpu.cKVM x86 AccelCPUClass
> >>> hvf/hvf-cpu.cHVF x86 AccelCPUClass
> >>> tcg/tcg-cpu.cTCG x86 AccelCPUClass
> 
>  Well this is a big commit... I'm not custom to x86 target, and am
>  having hard time following the cpu host/max change.
> 
>  Is it working when you use '-cpu max,...' instead of '-cpu host,'?
> >>>
> >>> No; and in fact the cpuid's are almost entirely different with and
> >>> without this patch! (both with -cpu host).  It looks like with this
> >>> patch we're getting the cpuid for the TCG cpuid rather than the host:
> >>>
> >>> Prior to this patch:
> >>> :/# cat /proc/cpuinfo
> >>> processor   : 0
> >>> vendor_id   : GenuineIntel
> >>> cpu family  : 6
> >>> model   : 142
> >>> model name  : Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
> >>> stepping: 10
> >>> microcode   : 0xe0
> >>> cpu MHz : 2111.998
> >>> cache size  : 16384 KB
> >>> physical id : 0
> >>> siblings: 1
> >>> core id : 0
> >>> cpu cores   : 1
> >>> apicid  : 0
> >>> initial apicid  : 0
> >>> fpu : yes
> >>> fpu_exception   : yes
> >>> cpuid level : 22
> >>> wp  : yes
> >>> flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge 
> >>> mca cmov pat pse36 clflush mmx fxsr sse sse2 ss syscall nx pdpe1gb rdtscp 
> >>> lm constant
> >>> _tsc arch_perfmon rep_good nopl xtopology cpuid tsc_known_freq pni 
> >>> pclmulqdq vmx

Re: Windows fails to boot after rebase to QEMU master

[Claudio Fontana]

On 5/27/21 1:36 PM, Claudio Fontana wrote:
> On 5/27/21 12:53 PM, Claudio Fontana wrote:
>> On 5/27/21 11:48 AM, Claudio Fontana wrote:
>>> On 5/27/21 11:15 AM, Dr. David Alan Gilbert wrote:
 * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> On 5/27/21 10:31 AM, Dr. David Alan Gilbert wrote:
>> * Claudio Fontana (cfont...@suse.de) wrote:
>>> On 5/26/21 9:30 PM, Dr. David Alan Gilbert wrote:
 * Michael S. Tsirkin (m...@redhat.com) wrote:
> On Fri, May 21, 2021 at 11:17:19AM +0200, Siddharth Chandrasekaran 
> wrote:
>> After a rebase to QEMU master, I am having trouble booting windows 
>> VMs.
>> Git bisect indicates commit f5cc5a5c1686 ("i386: split cpu 
>> accelerators
>> from cpu.c, using AccelCPUClass") to have introduced the issue. I 
>> spent
>> some time looking at into it yesterday without much luck.
>>
>> Steps to reproduce:
>>
>> $ ./configure --enable-kvm --disable-xen 
>> --target-list=x86_64-softmmu --enable-debug
>> $ make -j `nproc`
>> $ ./build/x86_64-softmmu/qemu-system-x86_64 \
>> -cpu 
>> host,hv_synic,hv_vpindex,hv_time,hv_runtime,hv_stimer,hv_crash \
>> -enable-kvm \
>> -name test,debug-threads=on \
>> -smp 1,threads=1,cores=1,sockets=1 \
>> -m 4G \
>> -net nic -net user \
>> -boot d,menu=on \
>> -usbdevice tablet \
>> -vnc :3 \
>> -machine q35,smm=on \
>> -drive 
>> if=pflash,format=raw,readonly=on,unit=0,file="../OVMF_CODE.secboot.fd"
>>  \
>> -drive 
>> if=pflash,format=raw,unit=1,file="../OVMF_VARS.secboot.fd" \
>> -global ICH9-LPC.disable_s3=1 \
>> -global driver=cfi.pflash01,property=secure,value=on \
>> -cdrom "../Windows_Server_2016_14393.ISO" \
>> -drive 
>> file="../win_server_2016.qcow2",format=qcow2,if=none,id=rootfs_drive 
>> \
>> -device ahci,id=ahci \
>> -device ide-hd,drive=rootfs_drive,bus=ahci.0
>>
>> If the issue is not obvious, I'd like some pointers on how to go 
>> about
>> fixing this issue.
>>
>> ~ Sid.
>>
>
> At a guess this commit inadvertently changed something in the CPU ID.
> I'd start by using a linux guest to dump cpuid before and after the
> change.

 I've not had a chance to do that yet, however I did just end up with a
 bisect of a linux guest failure bisecting to the same patch:

 [dgilbert@dgilbert-t580 qemu]$ git bisect bad
 f5cc5a5c168674f84bf061cdb307c2d25fba5448 is the first bad commit
 commit f5cc5a5c168674f84bf061cdb307c2d25fba5448
 Author: Claudio Fontana 
 Date:   Mon Mar 22 14:27:40 2021 +0100

 i386: split cpu accelerators from cpu.c, using AccelCPUClass
 
 i386 is the first user of AccelCPUClass, allowing to split
 cpu.c into:
 
 cpu.ccpuid and common x86 cpu functionality
 host-cpu.c   host x86 cpu functions and "host" cpu type
 kvm/kvm-cpu.cKVM x86 AccelCPUClass
 hvf/hvf-cpu.cHVF x86 AccelCPUClass
 tcg/tcg-cpu.cTCG x86 AccelCPUClass
>
> Well this is a big commit... I'm not custom to x86 target, and am
> having hard time following the cpu host/max change.
>
> Is it working when you use '-cpu max,...' instead of '-cpu host,'?

 No; and in fact the cpuid's are almost entirely different with and
 without this patch! (both with -cpu host).  It looks like with this
 patch we're getting the cpuid for the TCG cpuid rather than the host:

 Prior to this patch:
 :/# cat /proc/cpuinfo
 processor   : 0
 vendor_id   : GenuineIntel
 cpu family  : 6
 model   : 142
 model name  : Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
 stepping: 10
 microcode   : 0xe0
 cpu MHz : 2111.998
 cache size  : 16384 KB
 physical id : 0
 siblings: 1
 core id : 0
 cpu cores   : 1
 apicid  : 0
 initial apicid  : 0
 fpu : yes
 fpu_exception   : yes
 cpuid level : 22
 wp  : yes
 flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca 
 cmov pat pse36 clflush mmx fxsr sse sse2 ss syscall nx pdpe1gb rdtscp lm 
 constant
 _tsc arch_perfmon rep_good nopl xtopology cpuid tsc_known_freq pni 
 pclmulqdq vmx ssse3 fma cx16 pdcm pcid sse4_1 sse4_2 x2apic movbe popcnt 
 tsc_deadline_tim
 er aes xsave avx f16c rdran

Re: [PATCH 1/1] yank: Unregister function when using TLS migration

[Peter Xu]

On Thu, May 27, 2021 at 02:17:55PM +0100, Daniel P. Berrangé wrote:
> On Thu, May 27, 2021 at 09:09:09AM -0400, Peter Xu wrote:
> > On Thu, May 27, 2021 at 01:37:42PM +0100, Daniel P. Berrangé wrote:
> > > On Thu, May 27, 2021 at 08:23:52AM -0400, Peter Xu wrote:
> > > > On Thu, May 27, 2021 at 09:46:54AM +0100, Daniel P. Berrangé wrote:
> > > > > On Wed, May 26, 2021 at 05:58:58PM -0400, Peter Xu wrote:
> > > > > > On Wed, May 26, 2021 at 11:21:03PM +0200, Lukas Straub wrote:
> > > > > > > On Wed, 26 May 2021 16:40:35 -0400
> > > > > > > Peter Xu  wrote:
> > > > > > > 
> > > > > > > > On Wed, May 26, 2021 at 05:05:40PM -0300, Leonardo Bras wrote:
> > > > > > > > > After yank feature was introduced, whenever migration is 
> > > > > > > > > started using TLS,
> > > > > > > > > the following error happens in both source and destination 
> > > > > > > > > hosts:
> > > > > > > > > 
> > > > > > > > > (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> > > > > > > > > Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> > > > > > > > > 
> > > > > > > > > This happens because of a missing yank_unregister_function() 
> > > > > > > > > when using
> > > > > > > > > qio-channel-tls.
> > > > > > > > > 
> > > > > > > > > Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to 
> > > > > > > > > perform
> > > > > > > > > yank_unregister_function() in channel_close() and 
> > > > > > > > > multifd_load_cleanup().
> > > > > > > > > 
> > > > > > > > > Fixes: 50186051f ("Introduce yank feature")
> > > > > > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> > > > > > > > > Signed-off-by: Leonardo Bras   
> > > > > > > > 
> > > > > > > > Leo,
> > > > > > > > 
> > > > > > > > Thanks for looking into it!
> > > > > > > > 
> > > > > > > > So before looking int the fix... I do have a doubt on why we 
> > > > > > > > only enable yank
> > > > > > > > on socket typed, as I think tls should also work with 
> > > > > > > > qio_channel_shutdown().
> > > > > > > > 
> > > > > > > > IIUC the confused thing here is we register only for 
> > > > > > > > qio-socket, however tls
> > > > > > > > will actually call migration_channel_connect() twice, first 
> > > > > > > > with a qio-socket,
> > > > > > > > then with the real tls-socket.  For tls I feel like we have 
> > > > > > > > registered with the
> > > > > > > > wrong channel - instead of the wrapper socket ioc, we should 
> > > > > > > > register to the
> > > > > > > > final tls ioc?
> > > > > > > > 
> > > > > > > > Lukas, is there a reason?
> > > > > > > > 
> > > > > > > 
> > > > > > > Hi,
> > > > > > > There is no specific reason. Both ways work equally well in 
> > > > > > > preventing
> > > > > > > qemu from hanging. shutdown() for tls-channel just makes it abort 
> > > > > > > a
> > > > > > > little sooner (by not attempting to encrypt and send data 
> > > > > > > anymore).
> > > > > > > 
> > > > > > > I don't lean either way. I guess registering it on the tls-channel
> > > > > > > makes is a bit more explicit and clearer.
> > > > > > 
> > > > > > Agreed, because IMHO logically the migration code should not be 
> > > > > > aware of
> > > > > > internals of IOChannels, e.g., we shouldn't need to know 
> > > > > > ioc->master is the
> > > > > > socket ioc of tls ioc to unregister.
> > > > > 
> > > > > I think it is atually better to ignore the TLS channel and *always* 
> > > > > yank
> > > > > on the undering socket IO channel. The yank functionality is intended 
> > > > > to
> > > > > be used in a scenario where we know the channels are broken.  If yank
> > > > > calls the high level IO channel it is potentially going to try to do a
> > > > > cleanup shutdown that we know will fail because of the broken network.
> > > > 
> > > > Could you elaborate what's the "cleanup shutdown"?
> > > > 
> > > > The yank calls migration_yank_iochannel:
> > > > 
> > > > void migration_yank_iochannel(void *opaque)
> > > > {
> > > > QIOChannel *ioc = QIO_CHANNEL(opaque);
> > > > 
> > > > qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> > > > }
> > > > 
> > > > Where qio_channel_shutdown for tls is nothing but delivers that to the 
> > > > master
> > > > channel:
> > > > 
> > > > static int qio_channel_tls_shutdown(QIOChannel *ioc,
> > > > QIOChannelShutdown how,
> > > > Error **errp)
> > > > {
> > > > QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
> > > > 
> > > > qatomic_or(&tioc->shutdown, how);
> > > > 
> > > > return qio_channel_shutdown(tioc->master, how, errp);
> > > > }
> > > > 
> > > > So I thought it was a nice wrapper just for things like this, and I 
> > > > didn't see
> > > > anything it does more than the io_shutdown for the socket channel.  Did 
> > > > I miss
> > > > something?
> > > 
> > > Today thats the case, but don't assume it will be the case forever.
> > > There is a mechanism in TLS for doing clean shutdown which we've
> > > debated including.
> >

Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics

[Bruno Piazera Larsen]

On 27/05/2021 01:35, David Gibson wrote:

On Thu, May 27, 2021 at 11:20:01AM +1000, David Gibson wrote:

On Wed, May 26, 2021 at 05:21:01PM -0300, Bruno Larsen (billionai) wrote:

This function requires surce code modification to be useful, which means
it probably is not used often, and the move to using decodetree means
the statistics won't even be collected anymore.

Also removed setting dump_statistics in ppc_cpu_realize, since it was
only useful when in conjunction with ppc_cpu_dump_statistics.

Suggested-by: Richard Henderson
Signed-off-by: Bruno Larsen (billionai) 
---
  target/ppc/cpu.h   |  1 -
  target/ppc/cpu_init.c  |  3 ---
  target/ppc/translate.c | 51 --
  3 files changed, 55 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 203f07e48e..c3d1b492e4 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1256,7 +1256,6 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, 
PPCVirtualHypervisorClass,
  void ppc_cpu_do_interrupt(CPUState *cpu);
  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
-void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
  int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
  int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int reg);
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index f5ae2f150d..bd05f53fa4 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -9250,9 +9250,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void 
*data)
  cc->class_by_name = ppc_cpu_class_by_name;
  cc->has_work = ppc_cpu_has_work;
  cc->dump_state = ppc_cpu_dump_state;
-#ifdef CONFIG_TCG
-cc->dump_statistics = ppc_cpu_dump_statistics;
-#endif

This confuses me.  The ifdefs you're removing aren't present in my
tree, and AFAICT they never existed since your own patch created
cpu_init.c.

So.. please rebase and check that.

Duh, sorry, I looked at this set out of order with your latest !tcg
patches.  Now that I've applied those, I've applied those one as well.
Let me just check, where do you keep your most updated tree? I'm 
rebasing on your github tree, but ppc-for-6.1 there seems quite outdated 
(still the same as main)

--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO 


Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer 

Re: [PATCH 0/5] linux-user changes to run docker

[Alex Bennée]

Takashi Yamamoto  writes:

> On Tue, May 25, 2021 at 8:22 AM Takashi Yamamoto  
> wrote:
>>
>> On Tue, May 25, 2021 at 2:49 AM Alex Bennée  wrote:
>> >
>> >
>> > YAMAMOTO Takashi  writes:
>> >
>> > > These patches, along with a few more hacks [1] I didn't include
>> > > in this patchset, allowed me to run arm64 and armv7 version of
>> > > dind image on amd64.
>> > >
>> > > [1] https://github.com/yamt/qemu/tree/linux-user-for-docker
>> >
>> > Might be worth posting those patches next time (even if they have a RFC
>> > or !MERGE in the title for now).
>>
>> ok.
>
> while RFC is mentioned in eg. git format-patch --help,
> i couldn't find what !MERGE is.
> can you provide a reference?

It's usually just an annotation to the subject line of the commit, e.g:

  foo/bar: hacky fix to frobulator (!MERGE)

  rest of commit message

or something like:

  baz/quack: invert the tachyon beam (WIP)

  reason for the fix.

  [AJB: still WIP as this breaks foo]

AFAIK the only subject lines supported by the tooling are the squash:
and fixup: prefixes.

> is there a nice way to express that some patches in a post are meant
> for application and the others are RFC?

Aside from a description in the cover letter not really. The main reason
to include patches that aren't ready for merging is to show where your
work is going so the full context of earlier changes can be seen. Having
an ALL CAPS tag in the subject line is just handy for the maintainer
when scanning what might get cherry picked. Obviously if a patch totally
breaks the build it's not worth including as it just makes review harder
when giving the patches a spin so you should exercise your judgement.

-- 
Alex Bennée

Re: [PATCH 1/1] yank: Unregister function when using TLS migration

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berra...@redhat.com) wrote:
> On Thu, May 27, 2021 at 09:09:09AM -0400, Peter Xu wrote:
> > On Thu, May 27, 2021 at 01:37:42PM +0100, Daniel P. Berrangé wrote:
> > > On Thu, May 27, 2021 at 08:23:52AM -0400, Peter Xu wrote:
> > > > On Thu, May 27, 2021 at 09:46:54AM +0100, Daniel P. Berrangé wrote:
> > > > > On Wed, May 26, 2021 at 05:58:58PM -0400, Peter Xu wrote:
> > > > > > On Wed, May 26, 2021 at 11:21:03PM +0200, Lukas Straub wrote:
> > > > > > > On Wed, 26 May 2021 16:40:35 -0400
> > > > > > > Peter Xu  wrote:
> > > > > > > 
> > > > > > > > On Wed, May 26, 2021 at 05:05:40PM -0300, Leonardo Bras wrote:
> > > > > > > > > After yank feature was introduced, whenever migration is 
> > > > > > > > > started using TLS,
> > > > > > > > > the following error happens in both source and destination 
> > > > > > > > > hosts:
> > > > > > > > > 
> > > > > > > > > (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> > > > > > > > > Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> > > > > > > > > 
> > > > > > > > > This happens because of a missing yank_unregister_function() 
> > > > > > > > > when using
> > > > > > > > > qio-channel-tls.
> > > > > > > > > 
> > > > > > > > > Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to 
> > > > > > > > > perform
> > > > > > > > > yank_unregister_function() in channel_close() and 
> > > > > > > > > multifd_load_cleanup().
> > > > > > > > > 
> > > > > > > > > Fixes: 50186051f ("Introduce yank feature")
> > > > > > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> > > > > > > > > Signed-off-by: Leonardo Bras   
> > > > > > > > 
> > > > > > > > Leo,
> > > > > > > > 
> > > > > > > > Thanks for looking into it!
> > > > > > > > 
> > > > > > > > So before looking int the fix... I do have a doubt on why we 
> > > > > > > > only enable yank
> > > > > > > > on socket typed, as I think tls should also work with 
> > > > > > > > qio_channel_shutdown().
> > > > > > > > 
> > > > > > > > IIUC the confused thing here is we register only for 
> > > > > > > > qio-socket, however tls
> > > > > > > > will actually call migration_channel_connect() twice, first 
> > > > > > > > with a qio-socket,
> > > > > > > > then with the real tls-socket.  For tls I feel like we have 
> > > > > > > > registered with the
> > > > > > > > wrong channel - instead of the wrapper socket ioc, we should 
> > > > > > > > register to the
> > > > > > > > final tls ioc?
> > > > > > > > 
> > > > > > > > Lukas, is there a reason?
> > > > > > > > 
> > > > > > > 
> > > > > > > Hi,
> > > > > > > There is no specific reason. Both ways work equally well in 
> > > > > > > preventing
> > > > > > > qemu from hanging. shutdown() for tls-channel just makes it abort 
> > > > > > > a
> > > > > > > little sooner (by not attempting to encrypt and send data 
> > > > > > > anymore).
> > > > > > > 
> > > > > > > I don't lean either way. I guess registering it on the tls-channel
> > > > > > > makes is a bit more explicit and clearer.
> > > > > > 
> > > > > > Agreed, because IMHO logically the migration code should not be 
> > > > > > aware of
> > > > > > internals of IOChannels, e.g., we shouldn't need to know 
> > > > > > ioc->master is the
> > > > > > socket ioc of tls ioc to unregister.
> > > > > 
> > > > > I think it is atually better to ignore the TLS channel and *always* 
> > > > > yank
> > > > > on the undering socket IO channel. The yank functionality is intended 
> > > > > to
> > > > > be used in a scenario where we know the channels are broken.  If yank
> > > > > calls the high level IO channel it is potentially going to try to do a
> > > > > cleanup shutdown that we know will fail because of the broken network.
> > > > 
> > > > Could you elaborate what's the "cleanup shutdown"?
> > > > 
> > > > The yank calls migration_yank_iochannel:
> > > > 
> > > > void migration_yank_iochannel(void *opaque)
> > > > {
> > > > QIOChannel *ioc = QIO_CHANNEL(opaque);
> > > > 
> > > > qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> > > > }
> > > > 
> > > > Where qio_channel_shutdown for tls is nothing but delivers that to the 
> > > > master
> > > > channel:
> > > > 
> > > > static int qio_channel_tls_shutdown(QIOChannel *ioc,
> > > > QIOChannelShutdown how,
> > > > Error **errp)
> > > > {
> > > > QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
> > > > 
> > > > qatomic_or(&tioc->shutdown, how);
> > > > 
> > > > return qio_channel_shutdown(tioc->master, how, errp);
> > > > }
> > > > 
> > > > So I thought it was a nice wrapper just for things like this, and I 
> > > > didn't see
> > > > anything it does more than the io_shutdown for the socket channel.  Did 
> > > > I miss
> > > > something?
> > > 
> > > Today thats the case, but don't assume it will be the case forever.
> > > There is a mechanism in TLS for doing clean shutdown which we've
> > > debated including.
> > > 
> > > In gener

Re: [PULL v2 00/28] Misc patches for 2021-05-24

[Peter Maydell]

On Wed, 26 May 2021 at 17:51, Paolo Bonzini  wrote:
>
> The following changes since commit 92f8c6fef13b31ba222c4d20ad8afd2b79c4c28e:
>
>   Merge remote-tracking branch 
> 'remotes/pmaydell/tags/pull-target-arm-20210525' into staging (2021-05-25 
> 16:17:06 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to 7cf333a37260c4aafa465453adc8e073e408967e:
>
>   gitlab-ci: use --meson=git for CFI jobs (2021-05-26 14:50:05 +0200)
>
> 
> * submodule cleanups (Philippe, myself)
> * tiny step towards a usable preconfig mode (myself)
> * Kconfig and LOCK_GUARD cleanups (philippe)
> * new x86 CPUID feature (Yang Zhong)
> * "-object qtest" support (myself)
> * Dirty ring support for KVM (Peter)
> * Fixes for 6.0 command line parsing breakage (myself)
> * Fix for macOS 11.3 SDK (Katsuhiro)
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1
for any user-visible changes.

-- PMM

Re: [PATCH] vfio: Fix unregister SaveVMHandler in vfio_migration_finalize

[Philippe Mathieu-Daudé]

On 5/27/21 2:31 PM, Kunkun Jiang wrote:
> In the vfio_migration_init(), the SaveVMHandler is registered for
> VFIO device. But it lacks the operation of 'unregister'. It will
> lead to 'Segmentation fault (core dumped)' in
> qemu_savevm_state_setup(), if performing live migration after a
> VFIO device is hot deleted.
> 
> Fixes: 7c2f5f75f94 (vfio: Register SaveVMHandlers for VFIO device)
> Reported-by: Qixin Gan 
> Signed-off-by: Kunkun Jiang 

Cc: qemu-sta...@nongnu.org

> ---
>  hw/vfio/migration.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 201642d75e..ef397ebe6c 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -892,6 +892,7 @@ void vfio_migration_finalize(VFIODevice *vbasedev)
>  
>  remove_migration_state_change_notifier(&migration->migration_state);
>  qemu_del_vm_change_state_handler(migration->vm_state);
> +unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
Hmm what about devices using "%s/vfio" id?

>  vfio_migration_exit(vbasedev);
>  }
>  
> 

Re: [PATCH 0/5] stop collection of instruction usage statistics

[Alex Bennée]

"Bruno Larsen (billionai)"  writes:

> Based-on: <20210525115355.8254-1-bruno.lar...@eldorado.org.br>
>
> The functionality of counting how many instructions were being executed and
> being able to show it through the monitor, although neat, was only
> supported by ppc, and now that it is migrating to use decodetree (at
> least partially), those statistics won't be used anymore. Therefore,
> this patch removes that functinality completely.

I have no particular comment to make about the PPC stuff but with the
common translator loop we have hooks across all converted front ends to
identify the start of each instruction. It's needed for the TCG plugin
instrumentation and we could in theory use it for more integrated stats
across the board.

Out of interest what was the main aim of this code - a view of total
executed instructions or something more detailed like a breakdown of
types and ops?

>
> This series was suggested by Richard Henderson
>
> Bruno Larsen (billionai) (5):
>   target/ppc: fixed GEN_OPCODE behavior when PPC_DUMP_CPU is set
>   target/ppc: remove ppc_cpu_dump_statistics
>   target/ppc: removed mentions to DO_PPC_STATISTICS
>   monitor: removed cpustats command
>   hw/core/cpu: removed cpu_dump_statistics function
>
>  hmp-commands-info.hx   | 13 
>  hw/core/cpu.c  |  9 --
>  include/hw/core/cpu.h  | 12 
>  monitor/misc.c | 11 ---
>  target/ppc/cpu.h   |  1 -
>  target/ppc/cpu_init.c  |  3 --
>  target/ppc/translate.c | 69 +++---
>  7 files changed, 5 insertions(+), 113 deletions(-)


-- 
Alex Bennée

Re: [PATCH 1/1] tests/acceptance: change armbian archive to a faster host

[Willian Rampazzo]

On Wed, May 26, 2021 at 8:41 PM Cleber Rosa  wrote:
>
> On Wed, May 26, 2021 at 05:56:01PM -0300, Willian Rampazzo wrote:
> > The current host for the image
> > Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img.xz
> > (archive.armbian.com) is extremely slow in the last couple of weeks,
> > making the job running the test
> > tests/system/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08
> > for the first time when the image is not yet on GitLab cache, time out
> > while the image is being downloaded.
> >
> > This changes the host to one faster, so new users with an empty cache
> > are not impacted.
> >
> > Signed-off-by: Willian Rampazzo 
> > ---
> >  tests/acceptance/boot_linux_console.py | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/acceptance/boot_linux_console.py 
> > b/tests/acceptance/boot_linux_console.py
> > index 276a53f146..51c23b822c 100644
> > --- a/tests/acceptance/boot_linux_console.py
> > +++ b/tests/acceptance/boot_linux_console.py
> > @@ -804,7 +804,8 @@ def test_arm_orangepi_bionic_20_08(self):
> >  # to 1036 MiB, but the underlying filesystem is 1552 MiB...
> >  # As we expand it to 2 GiB we are safe.
> >
> > -image_url = ('https://archive.armbian.com/orangepipc/archive/'
> > +image_url = ('https://armbian.systemonachip.net/'
> > + 'archive/orangepipc/archive/'
>
> Hi Willian,
>
> I was pretty annoyed by my pipeline failures, that I came up with:
>
>
> https://gitlab.com/cleber.gnu/qemu/-/commit/917b3e376e682e9c35c6f7f597ffca110c719e13
>
> To prove that it was a GitLab <-> archive.arbian.com issue.

When I tried both links, the slow link, and this new link, on my
machine, I could see the slow link is also slow locally. Not as slow
as on GitLab, but 10 times slower than this new link. I was thinking
about open an issue on GitLab. In the worst case, they will say it is
not their fault, but a problem on the other end.

> But I wonder:
>
>  1. how susceptible to the same situation is this other mirror?

Unfortunately, having tests depending on external artifacts will bring
this kind of situation. Unless GitLab is doing traffic shaping, we
will never know how susceptible an external server is to any kind of
instability.

>  2. how trustworthy is this mirror, say, stability wise? Maybe
> people in the armbian community would have some info?

This new link is the same link that
https://www.armbian.com/orange-pi-pc/ "Archived versions" is pointing,
so I consider it an official mirror from Armbian. That's why I have
not thought much about changing it.

Now, stability wise, we never know :) I don't think we have this
answer for any of the links related to external artifacts QEMU
acceptance tests use.

>
> Depending on the feedback we get about, this can be a very valid
> hotfix/workaround indeed.  But the core issues we need to look into
> are:
>
>  a. applying a timeout when fetching assets.  If the asset fails to be
> fetched within the timeout, the test simply gets canceled.

But this is failing during the download before the test starts, or in
the pre-phase. The test suite was not created and Avocado don't have a
mapping asset <=> test yet.

>
>  b. evaluate the use of the multiple "locations" support that the
> avocado.utils.asset library has (and improve it if necessary).
>

This may be an option with a timeout in the location. If the download
on one location times out, try another.

> Anyway, thanks for looking into this, and let's wait a bit for
> feedback.
>
> - Cleber.

Re: [PULL 00/19] gitlab-ci patches for 2021-05-26

[Peter Maydell]

On Wed, 26 May 2021 at 22:27, Philippe Mathieu-Daudé  wrote:
>
> The following changes since commit 2ab2dad01f6dc3667c0d53d2b1ba46b511031207:
>
>   Merge remote-tracking branch 
> 'remotes/kraxel/tags/input-20210526-pull-request' into staging (2021-05-26 
> 15:27:20 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/philmd/qemu.git tags/gitlab-ci-20210526
>
> for you to fetch changes up to 740890e8935fff09023bb34c52e74ab1d539b775:
>
>   gitlab: Split gprof-gcov job (2021-05-26 23:01:47 +0200)
>
> 
> GitLab CI patches queue
>
> - Explode .gitlab-ci.yml in reusable templates
> - Add job to cross build/test TCI on i386 host
> - Remove CentOS 7 linux-user build job
> - Split gprof-gcov job
> - Temporarily set Avocado-based jobs in manual mode
> - Increase time to hold Avocado reports to 1 week
>

Conflict in .gitlab-ci.yml -- can you fix up and resend, please?

thanks
-- PMM

Re: [PATCH 2/5] target/ppc: remove ppc_cpu_dump_statistics

[Greg Kurz]

On Thu, 27 May 2021 10:22:50 -0300
Bruno Piazera Larsen  wrote:

> 
> On 27/05/2021 01:35, David Gibson wrote:
> > On Thu, May 27, 2021 at 11:20:01AM +1000, David Gibson wrote:
> >> On Wed, May 26, 2021 at 05:21:01PM -0300, Bruno Larsen (billionai) wrote:
> >>> This function requires surce code modification to be useful, which means
> >>> it probably is not used often, and the move to using decodetree means
> >>> the statistics won't even be collected anymore.
> >>>
> >>> Also removed setting dump_statistics in ppc_cpu_realize, since it was
> >>> only useful when in conjunction with ppc_cpu_dump_statistics.
> >>>
> >>> Suggested-by: Richard Henderson
> >>> Signed-off-by: Bruno Larsen (billionai) 
> >>> ---
> >>>   target/ppc/cpu.h   |  1 -
> >>>   target/ppc/cpu_init.c  |  3 ---
> >>>   target/ppc/translate.c | 51 --
> >>>   3 files changed, 55 deletions(-)
> >>>
> >>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> >>> index 203f07e48e..c3d1b492e4 100644
> >>> --- a/target/ppc/cpu.h
> >>> +++ b/target/ppc/cpu.h
> >>> @@ -1256,7 +1256,6 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, 
> >>> PPCVirtualHypervisorClass,
> >>>   void ppc_cpu_do_interrupt(CPUState *cpu);
> >>>   bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
> >>>   void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> >>> -void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
> >>>   hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> >>>   int ppc_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
> >>>   int ppc_cpu_gdb_read_register_apple(CPUState *cpu, GByteArray *buf, int 
> >>> reg);
> >>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> >>> index f5ae2f150d..bd05f53fa4 100644
> >>> --- a/target/ppc/cpu_init.c
> >>> +++ b/target/ppc/cpu_init.c
> >>> @@ -9250,9 +9250,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, 
> >>> void *data)
> >>>   cc->class_by_name = ppc_cpu_class_by_name;
> >>>   cc->has_work = ppc_cpu_has_work;
> >>>   cc->dump_state = ppc_cpu_dump_state;
> >>> -#ifdef CONFIG_TCG
> >>> -cc->dump_statistics = ppc_cpu_dump_statistics;
> >>> -#endif
> >> This confuses me.  The ifdefs you're removing aren't present in my
> >> tree, and AFAICT they never existed since your own patch created
> >> cpu_init.c.
> >>
> >> So.. please rebase and check that.
> > Duh, sorry, I looked at this set out of order with your latest !tcg
> > patches.  Now that I've applied those, I've applied those one as well.
> Let me just check, where do you keep your most updated tree? I'm 
> rebasing on your github tree, but ppc-for-6.1 there seems quite outdated 
> (still the same as main)

Try here:

https://gitlab.com/dgibson/qemu/-/commits/ppc-for-6.1/

Cheers,

--
Greg

[PULL 00/23] Vga 20210527 patches

[Gerd Hoffmann]

The following changes since commit 2ab2dad01f6dc3667c0d53d2b1ba46b511031207:

  Merge remote-tracking branch 
'remotes/kraxel/tags/input-20210526-pull-request' into staging (2021-05-26 
15:27:20 +0100)

are available in the Git repository at:

  git://git.kraxel.org/qemu tags/vga-20210527-pull-request

for you to fetch changes up to bdd53f739273e97b5e5617b699d1763c42a5ea7e:

  virtio-gpu: Update cursor data using blob (2021-05-27 12:07:37 +0200)


virtio-gpu: add blob resource support.
vhost-user-gpu: security fixes.



Li Qiang (8):
  vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
(CVE-2021-3545)
  vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
(CVE-2021-3544)
  vhost-user-gpu: fix memory leak in vg_resource_attach_backing
(CVE-2021-3544)
  vhost-user-gpu: fix memory leak while calling 'vg_resource_unref'
(CVE-2021-3544)
  vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
(CVE-2021-3544)
  vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
(CVE-2021-3544)
  vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'
(CVE-2021-3546)
  vhost-user-gpu: abstract vg_cleanup_mapping_iov

Vivek Kasireddy (14):
  ui: Get the fd associated with udmabuf driver
  headers: Add udmabuf.h
  virtio-gpu: Add udmabuf helpers
  stubs: Add stubs for udmabuf helpers
  virtio-gpu: Add virtio_gpu_find_check_resource
  virtio-gpu: Refactor virtio_gpu_set_scanout
  virtio-gpu: Refactor virtio_gpu_create_mapping_iov
  virtio-gpu: Add initial definitions for blob resources
  virtio-gpu: Add virtio_gpu_resource_create_blob
  ui/pixman: Add qemu_pixman_to_drm_format()
  virtio-gpu: Add helpers to create and destroy dmabuf objects
  virtio-gpu: Factor out update scanout
  virtio-gpu: Add virtio_gpu_set_scanout_blob
  virtio-gpu: Update cursor data using blob

maobibo (1):
  hw/display/qxl: Set pci rom address aligned with page size

 contrib/vhost-user-gpu/vugpu.h   |   2 +-
 include/hw/virtio/virtio-gpu-bswap.h |  16 +
 include/hw/virtio/virtio-gpu.h   |  39 +-
 include/standard-headers/linux/udmabuf.h |  32 ++
 include/ui/console.h |   3 +
 include/ui/qemu-pixman.h |   1 +
 contrib/vhost-user-gpu/vhost-user-gpu.c  |  29 +-
 contrib/vhost-user-gpu/virgl.c   |  20 +-
 hw/display/qxl.c |   2 +-
 hw/display/virtio-gpu-base.c |   3 +
 hw/display/virtio-gpu-udmabuf.c  | 223 
 hw/display/virtio-gpu-virgl.c|   3 +-
 hw/display/virtio-gpu.c  | 441 +--
 stubs/virtio-gpu-udmabuf.c   |  27 ++
 ui/qemu-pixman.c |  35 +-
 ui/udmabuf.c |  40 ++
 hw/display/meson.build   |   1 +
 hw/display/trace-events  |   2 +
 meson.build  |   2 +-
 scripts/update-linux-headers.sh  |   3 +
 stubs/meson.build|   1 +
 ui/meson.build   |   1 +
 22 files changed, 804 insertions(+), 122 deletions(-)
 create mode 100644 include/standard-headers/linux/udmabuf.h
 create mode 100644 hw/display/virtio-gpu-udmabuf.c
 create mode 100644 stubs/virtio-gpu-udmabuf.c
 create mode 100644 ui/udmabuf.c

-- 
2.31.1

[PULL 03/23] vhost-user-gpu: fix memory leak in vg_resource_attach_backing (CVE-2021-3544)

[Gerd Hoffmann]

From: Li Qiang 

Check whether the 'res' has already been attach_backing to avoid
memory leak.

Fixes: CVE-2021-3544
Reported-by: Li Qiang 
virtio-gpu fix: 204f01b309 ("virtio-gpu: fix memory leak
in resource attach backing")

Signed-off-by: Li Qiang 
Reviewed-by: Marc-André Lureau 
Message-Id: <20210516030403.107723-4-liq...@163.com>
Signed-off-by: Gerd Hoffmann 
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c 
b/contrib/vhost-user-gpu/vhost-user-gpu.c
index b5e153d0d648..0437e52b6460 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -489,6 +489,11 @@ vg_resource_attach_backing(VuGpu *g,
 return;
 }
 
+if (res->iov) {
+cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+return;
+}
+
 ret = vg_create_mapping_iov(g, &ab, cmd, &res->iov);
 if (ret != 0) {
 cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
-- 
2.31.1

[PULL 04/23] vhost-user-gpu: fix memory leak while calling 'vg_resource_unref' (CVE-2021-3544)

[Gerd Hoffmann]

From: Li Qiang 

If the guest trigger following sequences, the attach_backing will be leaked:

vg_resource_create_2d
vg_resource_attach_backing
vg_resource_unref

This patch fix this by freeing 'res->iov' in vg_resource_destroy.

Fixes: CVE-2021-3544
Reported-by: Li Qiang 
virtio-gpu fix: 5e8e3c4c75 ("virtio-gpu: fix resource leak
in virgl_cmd_resource_unref")

Reviewed-by: Prasad J Pandit 
Signed-off-by: Li Qiang 
Reviewed-by: Marc-André Lureau 
Message-Id: <20210516030403.107723-5-liq...@163.com>
Signed-off-by: Gerd Hoffmann 
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c 
b/contrib/vhost-user-gpu/vhost-user-gpu.c
index 0437e52b6460..770dfad52989 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -400,6 +400,7 @@ vg_resource_destroy(VuGpu *g,
 }
 
 vugbm_buffer_destroy(&res->buffer);
+g_free(res->iov);
 pixman_image_unref(res->image);
 QTAILQ_REMOVE(&g->reslist, res, next);
 g_free(res);
-- 
2.31.1