date:20190807

On 8/7/19 6:53 AM, Richard Henderson wrote:
> This function is used in two different contexts, and it will be
> clearer if the function is given the address to which it applies.
> 
> Signed-off-by: Richard Henderson 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  target/arm/translate.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 7853462b21..1f15f14022 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -9261,11 +9261,11 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>  }
>  }
>  
> -static bool thumb_insn_is_16bit(DisasContext *s, uint32_t insn)
> +static bool thumb_insn_is_16bit(DisasContext *s, uint32_t pc, uint32_t insn)
>  {
> -/* Return true if this is a 16 bit instruction. We must be precise
> - * about this (matching the decode).  We assume that s->pc still
> - * points to the first 16 bits of the insn.
> +/*
> + * Return true if this is a 16 bit instruction. We must be precise
> + * about this (matching the decode).
>   */
>  if ((insn >> 11) < 0x1d) {
>  /* Definitely a 16-bit instruction */
> @@ -9285,7 +9285,7 @@ static bool thumb_insn_is_16bit(DisasContext *s, 
> uint32_t insn)
>  return false;
>  }
>  
> -if ((insn >> 11) == 0x1e && s->pc - s->page_start < TARGET_PAGE_SIZE - 
> 3) {
> +if ((insn >> 11) == 0x1e && pc - s->page_start < TARGET_PAGE_SIZE - 3) {
>  /* 0b_0xxx__ : BL/BLX prefix, and the suffix
>   * is not on the next page; we merge this into a 32-bit
>   * insn.
> @@ -11824,7 +11824,7 @@ static bool insn_crosses_page(CPUARMState *env, 
> DisasContext *s)
>   */
>  uint16_t insn = arm_lduw_code(env, s->pc, s->sctlr_b);
>  
> -return !thumb_insn_is_16bit(s, insn);
> +return !thumb_insn_is_16bit(s, s->pc, insn);
>  }
>  
>  static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> @@ -12122,7 +12122,7 @@ static void thumb_tr_translate_insn(DisasContextBase 
> *dcbase, CPUState *cpu)
>  }
>  
>  insn = arm_lduw_code(env, dc->pc, dc->sctlr_b);
> -is_16bit = thumb_insn_is_16bit(dc, insn);
> +is_16bit = thumb_insn_is_16bit(dc, dc->pc, insn);
>  dc->pc += 2;
>  if (!is_16bit) {
>  uint32_t insn2 = arm_lduw_code(env, dc->pc, dc->sctlr_b);
>

Re: [Qemu-devel] [PATCH 04/11] target/arm: Introduce add_reg_for_lit

On 8/7/19 6:53 AM, Richard Henderson wrote:
> Provide a common routine for the places that require ALIGN(PC, 4)
> as the base address as opposed to plain PC.  The two are always
> the same for A32, but the difference is meaningful for thumb mode.
> 
> Signed-off-by: Richard Henderson 
> ---
> Note: This patch is easier to read with -b, as there are several
> large-ish indentation changes as the if statements fold together.
> ---
>  target/arm/translate-vfp.inc.c |  38 ++--
>  target/arm/translate.c | 166 +++--
>  2 files changed, 82 insertions(+), 122 deletions(-)
> 
> diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
> index 092eb5ec53..262d4177e5 100644
> --- a/target/arm/translate-vfp.inc.c
> +++ b/target/arm/translate-vfp.inc.c
> @@ -941,14 +941,8 @@ static bool trans_VLDR_VSTR_sp(DisasContext *s, 
> arg_VLDR_VSTR_sp *a)
>  offset = -offset;
>  }
>  
> -if (s->thumb && a->rn == 15) {
> -/* This is actually UNPREDICTABLE */
> -addr = tcg_temp_new_i32();
> -tcg_gen_movi_i32(addr, s->pc & ~2);
> -} else {
> -addr = load_reg(s, a->rn);
> -}
> -tcg_gen_addi_i32(addr, addr, offset);
> +/* For thumb, use of PC is UNPREDICTABLE.  */
> +addr = add_reg_for_lit(s, a->rn, offset);
>  tmp = tcg_temp_new_i32();
>  if (a->l) {
>  gen_aa32_ld32u(s, tmp, addr, get_mem_index(s));
> @@ -983,14 +977,8 @@ static bool trans_VLDR_VSTR_dp(DisasContext *s, 
> arg_VLDR_VSTR_dp *a)
>  offset = -offset;
>  }
>  
> -if (s->thumb && a->rn == 15) {
> -/* This is actually UNPREDICTABLE */
> -addr = tcg_temp_new_i32();
> -tcg_gen_movi_i32(addr, s->pc & ~2);
> -} else {
> -addr = load_reg(s, a->rn);
> -}
> -tcg_gen_addi_i32(addr, addr, offset);
> +/* For thumb, use of PC is UNPREDICTABLE.  */
> +addr = add_reg_for_lit(s, a->rn, offset);
>  tmp = tcg_temp_new_i64();
>  if (a->l) {
>  gen_aa32_ld64(s, tmp, addr, get_mem_index(s));
> @@ -1029,13 +1017,8 @@ static bool trans_VLDM_VSTM_sp(DisasContext *s, 
> arg_VLDM_VSTM_sp *a)
>  return true;
>  }
>  
> -if (s->thumb && a->rn == 15) {
> -/* This is actually UNPREDICTABLE */
> -addr = tcg_temp_new_i32();
> -tcg_gen_movi_i32(addr, s->pc & ~2);
> -} else {
> -addr = load_reg(s, a->rn);
> -}
> +/* For thumb, use of PC is UNPREDICTABLE.  */
> +addr = add_reg_for_lit(s, a->rn, 0);
>  if (a->p) {
>  /* pre-decrement */
>  tcg_gen_addi_i32(addr, addr, -(a->imm << 2));
> @@ -1112,13 +1095,8 @@ static bool trans_VLDM_VSTM_dp(DisasContext *s, 
> arg_VLDM_VSTM_dp *a)
>  return true;
>  }
>  
> -if (s->thumb && a->rn == 15) {
> -/* This is actually UNPREDICTABLE */
> -addr = tcg_temp_new_i32();
> -tcg_gen_movi_i32(addr, s->pc & ~2);
> -} else {
> -addr = load_reg(s, a->rn);
> -}
> +/* For thumb, use of PC is UNPREDICTABLE.  */
> +addr = add_reg_for_lit(s, a->rn, 0);
>  if (a->p) {
>  /* pre-decrement */
>  tcg_gen_addi_i32(addr, addr, -(a->imm << 2));
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 61933865d5..71d94c053b 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -220,6 +220,23 @@ static inline TCGv_i32 load_reg(DisasContext *s, int reg)
>  return tmp;
>  }
>  
> +/*
> + * Create a new temp, REG + OFS, except PC is ALIGN(PC, 4).
> + * This is used for load/store for which use of PC implies (literal),
> + * or ADD that implies ADR.
> + */
> +static TCGv_i32 add_reg_for_lit(DisasContext *s, int reg, int ofs)
> +{
> +TCGv_i32 tmp = tcg_temp_new_i32();
> +
> +if (reg == 15) {
> +tcg_gen_movi_i32(tmp, (read_pc(s) & ~3) + ofs);
> +} else {
> +tcg_gen_addi_i32(tmp, cpu_R[reg], ofs);
> +}
> +return tmp;
> +}
> +
>  /* Set a CPU register.  The source must be a temporary and will be
> marked as dead.  */
>  static void store_reg(DisasContext *s, int reg, TCGv_i32 var)
> @@ -9472,16 +9489,12 @@ static void disas_thumb2_insn(DisasContext *s, 
> uint32_t insn)
>   */
>  bool wback = extract32(insn, 21, 1);
>  
> -if (rn == 15) {
> -if (insn & (1 << 21)) {
> -/* UNPREDICTABLE */
> -goto illegal_op;
> -}
> -addr = tcg_temp_new_i32();
> -tcg_gen_movi_i32(addr, s->pc & ~3);
> -} else {
> -addr = load_reg(s, rn);
> +if (rn == 15 && (insn & (1 << 21))) {
> +/* UNPREDICTABLE */
> +goto illegal_op;
>  }
> +
> +addr = add_reg_for_lit(s, rn, 0);
>  offset = (insn & 0xff) * 4;
>  if ((insn & (1 << 23)) ==

Re: [Qemu-devel] Is network backend netmap worth keeping?

2019-08-07 Thread Jason Wang




On 2019/8/8 下午12:48, Markus Armbruster wrote:

Please excuse the attention-grabbing subject.

Philippe Mathieu-Daudé  writes:


On 8/7/19 10:16 PM, Markus Armbruster wrote:

[...]

Can you tell me offhand what I have to install so configure enables
CONFIG_NETMAP?

The steps are listed in tests/docker/dockerfiles/debian-amd64.docker,
but you can get to this point running:

   $ make docker-image-debian-amd64 V=1 DEBUG=1

This will build the docker image with netmap (so you don't have to mess
with your workstation setup), then build QEMU within the image.

So, to make use of QEMU's netmap backend (CONFIG_NETMAP), you have to
build and install netmap software from sources.  Which pretty much
ensures nobody uses it.  It was added in commit 58952137b0b (Nov 2013).
The commit message points to ,
which gives me "connection timed out" right now.

On the other hand, it's covered in MAINTAINERS, and has seen
non-janitorial activity as late as Dec 2018 (commit c693fc748a).

Luigi, Giuseppe, Vincenzo, what's the status of the netmap project?

Why is the QEMU netmap backend worth keeping?

Who is using the netmap backend?



Netmap was supported by freebsd: 
https://www.freebsd.org/cgi/man.cgi?query=netmap=4. So I guess 
there should be real users.





How do they obtain a netmap-enabled QEMU?  Compile it from sources
themselves?



Yes.




Would it make sense to have netmap packaged in common Linux distros?



It requires Linux kernel support which is tough task especially Linux 
has AF_XDP support recently.


Thanks

Re: [Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()

Hi,

On 8/8/19 6:44 AM, Bin Meng wrote:
> When CADENCE_GEM_ERR_DEBUG is turned on, there are several
> compilation errors in DB_PRINT(). Fix them.
> 
> Signed-off-by: Bin Meng 
> 
> ---
> 
> Changes in v2:

Please don't reply to previous version, post as a new thread (it is
harder to notice your new versions in a emails threaded view).

> - use HWADDR_PRIx instead of TARGET_FMT_plx for consistency
> - use 'z' modifier to print sizeof(..)
> 
>  hw/net/cadence_gem.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index d412085..b6ff2c1 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -983,8 +983,9 @@ static ssize_t gem_receive(NetClientState *nc, const 
> uint8_t *buf, size_t size)
>  return -1;
>  }
>  
> -DB_PRINT("copy %d bytes to 0x%x\n", MIN(bytes_to_copy, rxbufsize),
> -rx_desc_get_buffer(s->rx_desc[q]));
> +DB_PRINT("copy %d bytes to 0x%" HWADDR_PRIx "\n",

rx_desc_get_buffer() returns a uint64_t, shouldn't you use a PRIx64
format here?

> + MIN(bytes_to_copy, rxbufsize),

Nitpick #1: since you are cleaning this file up, bytes_to_copy and
rxbufsize are both unsigned, so the first format should be %u instead of %d.

> + rx_desc_get_buffer(s, s->rx_desc[q]));
>  
>  /* Copy packet data to emulated DMA buffer */
>  address_space_write(>dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) 
> +
> @@ -1157,7 +1158,7 @@ static void gem_transmit(CadenceGEMState *s)
>  if (tx_desc_get_length(desc) > sizeof(tx_packet) -
> (p - tx_packet)) {
>  DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space " \
> - "0x%x\n", (unsigned)packet_desc_addr,
> + "0x%zx\n", (unsigned)packet_desc_addr,

Nitpick #2: packet_desc_addr is of type hwaddr, so removing the cast the
1st format is HWADDR_PRIx, also removing the 2nd cast the 2nd format is
PRIx64.

Then the 3rd format is now correct.

>   (unsigned)tx_desc_get_length(desc),
>   sizeof(tx_packet) - (p - tx_packet));
>  break;
>

Re: [Qemu-devel] [PATCH v2 12/29] Include hw/irq.h a lot less

On 8/7/19 9:27 PM, Markus Armbruster wrote:
  typedef void SaveStateHandler(QEMUFile *f, void *opaque);
  typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
 +typedef void (*qemu_irq_handler)(void *opaque, int n, int level);
>>
>> Should we prefer a consistent form for function pointer typedefs?  Here,
>> we've mixed 'rettype Name(params)' with 'rettype (*name)(params)'.
> 
> Which of the two difference I can see do you mean?

Eric asked about function type vs pointer-to-function type.

In usage,

  SaveStateHandler *variable;
vs
  qemu_irq_handler variable;


r~

[Qemu-devel] Is network backend netmap worth keeping? (was: [PATCH v2 27/29] Include sysemu/sysemu.h a lot less)

Please excuse the attention-grabbing subject.

Philippe Mathieu-Daudé  writes:

> On 8/7/19 10:16 PM, Markus Armbruster wrote:
[...]
>> Can you tell me offhand what I have to install so configure enables
>> CONFIG_NETMAP?
>
> The steps are listed in tests/docker/dockerfiles/debian-amd64.docker,
> but you can get to this point running:
>
>   $ make docker-image-debian-amd64 V=1 DEBUG=1
>
> This will build the docker image with netmap (so you don't have to mess
> with your workstation setup), then build QEMU within the image.

So, to make use of QEMU's netmap backend (CONFIG_NETMAP), you have to
build and install netmap software from sources.  Which pretty much
ensures nobody uses it.  It was added in commit 58952137b0b (Nov 2013).
The commit message points to ,
which gives me "connection timed out" right now.

On the other hand, it's covered in MAINTAINERS, and has seen
non-janitorial activity as late as Dec 2018 (commit c693fc748a).

Luigi, Giuseppe, Vincenzo, what's the status of the netmap project?

Why is the QEMU netmap backend worth keeping?

Who is using the netmap backend?

How do they obtain a netmap-enabled QEMU?  Compile it from sources
themselves?

Would it make sense to have netmap packaged in common Linux distros?

Re: [Qemu-devel] [PATCH] hw: net: cadence_gem: Fix build errors in DB_PRINT()

On Tue, Aug 6, 2019 at 6:57 PM Stefano Garzarella  wrote:
>
> On Mon, Aug 05, 2019 at 08:52:54AM -0700, Bin Meng wrote:
> > When CADENCE_GEM_ERR_DEBUG is turned on, there are several
> > compilation errors in DB_PRINT(). Fix them.
> >
> > Signed-off-by: Bin Meng 
> > ---
> >
> >  hw/net/cadence_gem.c | 7 ---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > index d412085..7516e8f 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -983,8 +983,9 @@ static ssize_t gem_receive(NetClientState *nc, const 
> > uint8_t *buf, size_t size)
> >  return -1;
> >  }
> >
> > -DB_PRINT("copy %d bytes to 0x%x\n", MIN(bytes_to_copy, rxbufsize),
> > -rx_desc_get_buffer(s->rx_desc[q]));
> > +DB_PRINT("copy %d bytes to " TARGET_FMT_plx "\n",
> > + MIN(bytes_to_copy, rxbufsize),
> > + rx_desc_get_buffer(s, s->rx_desc[q]));
> >
> >  /* Copy packet data to emulated DMA buffer */
> >  address_space_write(>dma_as, rx_desc_get_buffer(s, 
> > s->rx_desc[q]) +
> > @@ -1157,7 +1158,7 @@ static void gem_transmit(CadenceGEMState *s)
> >  if (tx_desc_get_length(desc) > sizeof(tx_packet) -
> > (p - tx_packet)) {
> >  DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space 
> > " \
> > - "0x%x\n", (unsigned)packet_desc_addr,
> > + "0x%lx\n", (unsigned)packet_desc_addr,
>
> What about using 'z' modifier? I mean "0x%zx" to print sizeof(..).

Yes, good idea. Will do in v2. Thanks!

Regards,
Bin

[Qemu-devel] [PATCH v2] hw: net: cadence_gem: Fix build errors in DB_PRINT()

When CADENCE_GEM_ERR_DEBUG is turned on, there are several
compilation errors in DB_PRINT(). Fix them.

Signed-off-by: Bin Meng 

---

Changes in v2:
- use HWADDR_PRIx instead of TARGET_FMT_plx for consistency
- use 'z' modifier to print sizeof(..)

 hw/net/cadence_gem.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index d412085..b6ff2c1 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -983,8 +983,9 @@ static ssize_t gem_receive(NetClientState *nc, const 
uint8_t *buf, size_t size)
 return -1;
 }
 
-DB_PRINT("copy %d bytes to 0x%x\n", MIN(bytes_to_copy, rxbufsize),
-rx_desc_get_buffer(s->rx_desc[q]));
+DB_PRINT("copy %d bytes to 0x%" HWADDR_PRIx "\n",
+ MIN(bytes_to_copy, rxbufsize),
+ rx_desc_get_buffer(s, s->rx_desc[q]));
 
 /* Copy packet data to emulated DMA buffer */
 address_space_write(>dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) +
@@ -1157,7 +1158,7 @@ static void gem_transmit(CadenceGEMState *s)
 if (tx_desc_get_length(desc) > sizeof(tx_packet) -
(p - tx_packet)) {
 DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space " \
- "0x%x\n", (unsigned)packet_desc_addr,
+ "0x%zx\n", (unsigned)packet_desc_addr,
  (unsigned)tx_desc_get_length(desc),
  sizeof(tx_packet) - (p - tx_packet));
 break;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 12/29] Include hw/irq.h a lot less

Eric Blake  writes:

> On 8/7/19 8:04 AM, Philippe Mathieu-Daudé wrote:
>> On 8/6/19 5:14 PM, Markus Armbruster wrote:
>>> In my "build everything" tree, changing hw/irq.h triggers a recompile
>>> of some 5400 out of 6600 objects (not counting tests and objects that
>>> don't depend on qemu/osdep.h).
>>>
>>> hw/hw.h supposedly includes it for convenience.  Several other headers
>>> include it just to get qemu_irq and.or qemu_irq_handler.
>>>
>>> Move the qemu_irq and qemu_irq_handler typedefs from hw/irq.h to
>>> qemu/typedefs.h, and then include hw/irq.h only where it's still
>>> needed.  Touching it now recompiles only some 500 objects.
>>>
>
>>>  /*
>>>   * Function types
>>>   */
>>>  typedef void SaveStateHandler(QEMUFile *f, void *opaque);
>>>  typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
>>> +typedef void (*qemu_irq_handler)(void *opaque, int n, int level);
>
> Should we prefer a consistent form for function pointer typedefs?  Here,
> we've mixed 'rettype Name(params)' with 'rettype (*name)(params)'.

Which of the two difference I can see do you mean?

CamelCase vs. lower_case_with_underscore?

Parenthesis around the type name?  I wouldn't call that inconsistent, we
simply use parenthesis only when needed.

Re: [Qemu-devel] [PATCH v2 01/29] include: Make headers more self-contained

Alex Bennée  writes:

> Markus Armbruster  writes:
>
>> Alex Bennée  writes:
>>
>>> Markus Armbruster  writes:
>>>
 Back in 2016, we discussed[1] rules for headers, and these were
 generally liked:

 1. Have a carefully curated header that's included everywhere first.  We
got that already thanks to Peter: osdep.h.

 2. Headers should normally include everything they need beyond osdep.h.
If exceptions are needed for some reason, they must be documented in
the header.  If all that's needed from a header is typedefs, put
those into qemu/typedefs.h instead of including the header.

 3. Cyclic inclusion is forbidden.

 This patch gets include/ closer to obeying 2.

 It's actually extracted from my "[RFC] Baby steps towards saner
 headers" series[2], which demonstrates a possible path towards
 checking 2 automatically.  It passes the RFC test there.

 [1] Message-ID: <87h9g8j57d@blackfin.pond.sub.org>
 https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html
 [2] Message-Id: <20190711122827.18970-1-arm...@redhat.com>
 https://lists.nongnu.org/archive/html/qemu-devel/2019-07/msg02715.html

 Signed-off-by: Markus Armbruster 
 Reviewed-by: Alistair Francis 
[...]
 diff --git a/include/fpu/softfloat-macros.h 
 b/include/fpu/softfloat-macros.h
 index c55aa6d174..be83a833ec 100644
 --- a/include/fpu/softfloat-macros.h
 +++ b/include/fpu/softfloat-macros.h
 @@ -82,6 +82,8 @@ this code that are retained.
  #ifndef FPU_SOFTFLOAT_MACROS_H
  #define FPU_SOFTFLOAT_MACROS_H

 +#include "fpu/softfloat.h"
 +
>>>
>>> What does softfloat-macros actually need from the core softfloat API?
>>> These are lower level functions used by softfloat itself (and m68k for
>>> it's own bit fiddling).
>>
>> I extracted this patch out of "[PATCH RFC v5 0/3] Baby steps towards
>> saner headers".  PATCH 1/3 creates make target "check-source", which is
>> what I used to find headers that aren't self-contained.  In this case:
>>
>>   CC  cris-softmmu/tests/headers-tgt/include/fpu/softfloat.o
>> In file included from tests/headers-tgt/include/fpu/softfloat-macros.c:21:
>> /work/armbru/qemu/include/fpu/softfloat-macros.h: In function 
>> ‘estimateDiv128To64’:
>> /work/armbru/qemu/include/fpu/softfloat-macros.h:623:27: error: implicit 
>> declaration of function ‘LIT64’ [-Werror=implicit-function-declaration]
>>   623 | if ( b <= a0 ) return LIT64( 0x );
>
> The LIT64 definition should be moved to softfloat-types.h which is
> already included by softfloat.h unless we already have a QEMU expansion
> we should be using. The softfloat-macros.h can include softfloat-types.h
> as well and we should only include the full softfloat.h if they need it.
>
> Do you want me to spin up a patch?

Yes, please!

[...]

[Qemu-devel] [PATCH v2] migration: rename migration_bitmap_sync_range to ramblock_sync_dirty_bitmap

Rename for better understanding of the code.

Suggested-by: Paolo Bonzini 
Signed-off-by: Wei Yang 

---
v2:
  * rebase on top of "just pass RAMBlock is enough"

---
 migration/ram.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index eee68a7991..0d12fa8e92 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1748,7 +1748,7 @@ static inline bool migration_bitmap_clear_dirty(RAMState 
*rs,
 }
 
 /* Called with RCU critical section */
-static void migration_bitmap_sync_range(RAMState *rs, RAMBlock *rb)
+static void ramblock_sync_dirty_bitmap(RAMState *rs, RAMBlock *rb)
 {
 rs->migration_dirty_pages +=
 cpu_physical_memory_sync_dirty_bitmap(rb, 0, rb->used_length,
@@ -1840,7 +1840,7 @@ static void migration_bitmap_sync(RAMState *rs)
 qemu_mutex_lock(>bitmap_mutex);
 rcu_read_lock();
 RAMBLOCK_FOREACH_NOT_IGNORED(block) {
-migration_bitmap_sync_range(rs, block);
+ramblock_sync_dirty_bitmap(rs, block);
 }
 ram_counters.remaining = ram_bytes_remaining();
 rcu_read_unlock();
@@ -4261,7 +4261,7 @@ static void colo_flush_ram_cache(void)
 memory_global_dirty_log_sync();
 rcu_read_lock();
 RAMBLOCK_FOREACH_NOT_IGNORED(block) {
-migration_bitmap_sync_range(ram_state, block);
+ramblock_sync_dirty_bitmap(ram_state, block);
 }
 rcu_read_unlock();
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH] migration: rename migration_bitmap_sync_range to ramblock_sync_dirty_bitmap

On Wed, Aug 07, 2019 at 06:49:48PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> Rename for better understanding of the code.
>> 
>> Suggested-by: Paolo Bonzini 
>> Signed-off-by: Wei Yang 
>
>this needs fixing after 'just pass RAMBlock is enough'
>

Ok, I got your point.

>Dave
>
>> ---
>>  migration/ram.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index c5f9f4b0ef..66792568e2 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1669,7 +1669,7 @@ static inline bool 
>> migration_bitmap_clear_dirty(RAMState *rs,
>>  return ret;
>>  }
>>  
>> -static void migration_bitmap_sync_range(RAMState *rs, RAMBlock *rb,
>> +static void ramblock_sync_dirty_bitmap(RAMState *rs, RAMBlock *rb,
>>  ram_addr_t length)
>>  {
>>  rs->migration_dirty_pages +=
>> @@ -1762,7 +1762,7 @@ static void migration_bitmap_sync(RAMState *rs)
>>  qemu_mutex_lock(>bitmap_mutex);
>>  rcu_read_lock();
>>  RAMBLOCK_FOREACH_NOT_IGNORED(block) {
>> -migration_bitmap_sync_range(rs, block, block->used_length);
>> +ramblock_sync_dirty_bitmap(rs, block, block->used_length);
>>  }
>>  ram_counters.remaining = ram_bytes_remaining();
>>  rcu_read_unlock();
>> @@ -4175,7 +4175,7 @@ static void colo_flush_ram_cache(void)
>>  memory_global_dirty_log_sync();
>>  rcu_read_lock();
>>  RAMBLOCK_FOREACH_NOT_IGNORED(block) {
>> -migration_bitmap_sync_range(ram_state, block, block->used_length);
>> +ramblock_sync_dirty_bitmap(ram_state, block, block->used_length);
>>  }
>>  rcu_read_unlock();
>>  
>> -- 
>> 2.17.1
>> 
>> 
>--
>Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

[Qemu-devel] [PATCH v2] riscv: hmp: Add a command to show virtual memory mappings

This adds 'info mem' command for RISC-V, to show virtual memory
mappings that aids debugging.

Rather than showing every valid PTE, the command compacts the
output by merging all contiguous physical address mappings into
one block and only shows the merged block mapping details.

Signed-off-by: Bin Meng 
Acked-by: Dr. David Alan Gilbert 

---

Changes in v2:
- promote ppn to hwaddr when doing page table address calculation

 hmp-commands-info.hx   |   2 +-
 target/riscv/Makefile.objs |   4 +
 target/riscv/monitor.c | 229 +
 3 files changed, 234 insertions(+), 1 deletion(-)
 create mode 100644 target/riscv/monitor.c

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index c59444c..257ee7d 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -249,7 +249,7 @@ STEXI
 Show virtual to physical memory mappings.
 ETEXI
 
-#if defined(TARGET_I386)
+#if defined(TARGET_I386) || defined(TARGET_RISCV)
 {
 .name   = "mem",
 .args_type  = "",
diff --git a/target/riscv/Makefile.objs b/target/riscv/Makefile.objs
index b1c79bc..a8ceccd 100644
--- a/target/riscv/Makefile.objs
+++ b/target/riscv/Makefile.objs
@@ -1,5 +1,9 @@
 obj-y += translate.o op_helper.o cpu_helper.o cpu.o csr.o fpu_helper.o 
gdbstub.o pmp.o
 
+ifeq ($(CONFIG_SOFTMMU),y)
+obj-y += monitor.o
+endif
+
 DECODETREE = $(SRC_PATH)/scripts/decodetree.py
 
 decode32-y = $(SRC_PATH)/target/riscv/insn32.decode
diff --git a/target/riscv/monitor.c b/target/riscv/monitor.c
new file mode 100644
index 000..d725a7a
--- /dev/null
+++ b/target/riscv/monitor.c
@@ -0,0 +1,229 @@
+/*
+ * QEMU monitor for RISC-V
+ *
+ * Copyright (c) 2019 Bin Meng 
+ *
+ * RISC-V specific monitor commands implementation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "cpu_bits.h"
+#include "monitor/monitor.h"
+#include "monitor/hmp-target.h"
+
+#ifdef TARGET_RISCV64
+#define PTE_HEADER_FIELDS   "vaddrpaddr"\
+"size attr\n"
+#define PTE_HEADER_DELIMITER"  "\
+" ---\n"
+#else
+#define PTE_HEADER_FIELDS   "vaddrpaddrsize attr\n"
+#define PTE_HEADER_DELIMITER"   ---\n"
+#endif
+
+/* Perform linear address sign extension */
+static target_ulong addr_canonical(int va_bits, target_ulong addr)
+{
+#ifdef TARGET_RISCV64
+if (addr & (1UL << (va_bits - 1))) {
+addr |= (hwaddr)-(1L << va_bits);
+}
+#endif
+
+return addr;
+}
+
+static void print_pte_header(Monitor *mon)
+{
+monitor_printf(mon, PTE_HEADER_FIELDS);
+monitor_printf(mon, PTE_HEADER_DELIMITER);
+}
+
+static void print_pte(Monitor *mon, int va_bits, target_ulong vaddr,
+  hwaddr paddr, target_ulong size, int attr)
+{
+/* santity check on vaddr */
+if (vaddr >= (1UL << va_bits)) {
+return;
+}
+
+if (!size) {
+return;
+}
+
+monitor_printf(mon, TARGET_FMT_lx " " TARGET_FMT_plx " " TARGET_FMT_lx
+   " %c%c%c%c%c%c%c\n",
+   addr_canonical(va_bits, vaddr),
+   paddr, size,
+   attr & PTE_R ? 'r' : '-',
+   attr & PTE_W ? 'w' : '-',
+   attr & PTE_X ? 'x' : '-',
+   attr & PTE_U ? 'u' : '-',
+   attr & PTE_G ? 'g' : '-',
+   attr & PTE_A ? 'a' : '-',
+   attr & PTE_D ? 'd' : '-');
+}
+
+static void walk_pte(Monitor *mon, hwaddr base, target_ulong start,
+ int level, int ptidxbits, int ptesize, int va_bits,
+ target_ulong *vbase, hwaddr *pbase, hwaddr *last_paddr,
+ target_ulong *last_size, int *last_attr)
+{
+hwaddr pte_addr;
+hwaddr paddr;
+target_ulong pgsize;
+target_ulong pte;
+int ptshift;
+int attr;
+int idx;
+
+if (level < 0) {
+return;
+}
+
+ptshift = level * ptidxbits;
+pgsize = 1UL << (PGSHIFT + ptshift);
+
+for (idx = 0; idx < (1UL << ptidxbits); idx++) {
+pte_addr = base + idx * ptesize;
+cpu_physical_memory_read(pte_addr, , ptesize);
+
+paddr = (hwaddr)(pte >> PTE_PPN_SHIFT) << PGSHIFT;
+attr = pte & 0xff;
+
+

Re: [Qemu-devel] [PATCH-for-4.2 v3 2/5] memory: Add IOMMU_ATTR_VFIO_NESTED IOMMU memory region attribute

2019-08-07 Thread Alex Williamson

On Thu, 11 Jul 2019 08:18:54 +0200
Eric Auger  wrote:

> We introduce a new IOMMU Memory Region attribute,
> IOMMU_ATTR_VFIO_NESTED that tells whether the virtual IOMMU
> requires HW nested paging for VFIO integration.
> 
> Current Intel virtual IOMMU device supports "Caching
> Mode" and does not require 2 stages at physical level to be
> integrated with VFIO. However SMMUv3 does not implement such
> "caching mode" and requires to use HW nested paging.
> 
> As such SMMUv3 is the first IOMMU device to advertise this
> attribute.
> 
> Signed-off-by: Eric Auger 
> ---
>  hw/arm/smmuv3.c   | 12 
>  include/exec/memory.h |  3 ++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index e96d5beb9a..384c02cb91 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1490,6 +1490,17 @@ static void 
> smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>  }
>  }
>  
> +static int smmuv3_get_attr(IOMMUMemoryRegion *iommu,
> +   enum IOMMUMemoryRegionAttr attr,
> +   void *data)
> +{
> +if (attr == IOMMU_ATTR_VFIO_NESTED) {
> +*(bool *) data = true;
> +return 0;
> +}
> +return -EINVAL;
> +}
> +
>  static void smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
>void *data)
>  {
> @@ -1497,6 +1508,7 @@ static void 
> smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
>  
>  imrc->translate = smmuv3_translate;
>  imrc->notify_flag_changed = smmuv3_notify_flag_changed;
> +imrc->get_attr = smmuv3_get_attr;
>  }
>  
>  static const TypeInfo smmuv3_type_info = {
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index a078cd033f..e477a630a8 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -204,7 +204,8 @@ struct MemoryRegionOps {
>  };
>  
>  enum IOMMUMemoryRegionAttr {
> -IOMMU_ATTR_SPAPR_TCE_FD
> +IOMMU_ATTR_SPAPR_TCE_FD,
> +IOMMU_ATTR_VFIO_NESTED,
>  };
>  
>  /**

Why VFIO_NESTED vs simply NESTED?  I figure any time we need to include
"VFIO" in the descriptions of something, we're probably not describing
the requirement correctly and it just becomes a meaningless tag that
gets ignored outside of VFIO related things.  If we're trying to
describe an IOMMU MemoryRegion that supports dynamic faulting rather
than requiring a replay to pre-populate it, then simply define that
semantic rather than hand waving some vfio specific interaction.
Thanks,

Alex

[Qemu-devel] [PATCH v2] riscv: rv32: Root page table address can be larger than 32-bit

For RV32, the root page table's PPN has 22 bits hence its address
bits could be larger than the maximum bits that target_ulong is
able to represent. Use hwaddr instead.

Signed-off-by: Bin Meng 

---

Changes in v2:
- promote ppn, env->satp/env->sptbl to hwaddr otherwise the page
  table base will not be correctly calculated

 target/riscv/cpu_helper.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e32b612..b2b4f3a 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -176,12 +176,12 @@ static int get_physical_address(CPURISCVState *env, 
hwaddr *physical,
 
 *prot = 0;
 
-target_ulong base;
+hwaddr base;
 int levels, ptidxbits, ptesize, vm, sum;
 int mxr = get_field(env->mstatus, MSTATUS_MXR);
 
 if (env->priv_ver >= PRIV_VERSION_1_10_0) {
-base = get_field(env->satp, SATP_PPN) << PGSHIFT;
+base = (hwaddr)get_field(env->satp, SATP_PPN) << PGSHIFT;
 sum = get_field(env->mstatus, MSTATUS_SUM);
 vm = get_field(env->satp, SATP_MODE);
 switch (vm) {
@@ -201,7 +201,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr 
*physical,
   g_assert_not_reached();
 }
 } else {
-base = env->sptbr << PGSHIFT;
+base = (hwaddr)(env->sptbr) << PGSHIFT;
 sum = !get_field(env->mstatus, MSTATUS_PUM);
 vm = get_field(env->mstatus, MSTATUS_VM);
 switch (vm) {
@@ -239,7 +239,7 @@ restart:
((1 << ptidxbits) - 1);
 
 /* check that physical address of PTE is legal */
-target_ulong pte_addr = base + idx * ptesize;
+hwaddr pte_addr = base + idx * ptesize;
 
 if (riscv_feature(env, RISCV_FEATURE_PMP) &&
 !pmp_hart_has_privs(env, pte_addr, sizeof(target_ulong),
@@ -251,7 +251,7 @@ restart:
 #elif defined(TARGET_RISCV64)
 target_ulong pte = ldq_phys(cs->as, pte_addr);
 #endif
-target_ulong ppn = pte >> PTE_PPN_SHIFT;
+hwaddr ppn = pte >> PTE_PPN_SHIFT;
 
 if (!(pte & PTE_V)) {
 /* Invalid PTE */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

On Thu, Aug 08, 2019 at 02:30:02AM +, Zeng, Star wrote:
>> -Original Message-
>> From: Wei Yang [mailto:richardw.y...@linux.intel.com]
>> Sent: Thursday, August 8, 2019 10:13 AM
>> To: Zeng, Star 
>> Cc: Wei Yang ; qemu-devel@nongnu.org;
>> imamm...@redhat.com; da...@redhat.com; m...@redhat.com
>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>> use goto for the last check
>> 
>> On Thu, Aug 08, 2019 at 01:42:14AM +, Zeng, Star wrote:
>> >> -Original Message-
>> >> From: Qemu-devel [mailto:qemu-devel-
>> >> bounces+star.zeng=intel@nongnu.org] On Behalf Of Wei Yang
>> >> Sent: Tuesday, July 30, 2019 8:38 AM
>> >> To: qemu-devel@nongnu.org
>> >> Cc: imamm...@redhat.com; da...@redhat.com; Wei Yang
>> >> ; m...@redhat.com
>> >> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>> >> use goto for the last check
>> >>
>> >> We are already at the last condition check.
>> >>
>> >> Signed-off-by: Wei Yang 
>> >> Reviewed-by: Igor Mammedov 
>> >> Reviewed-by: David Hildenbrand 
>> >> ---
>> >>  hw/mem/memory-device.c | 1 -
>> >>  1 file changed, 1 deletion(-)
>> >>
>> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index
>> >> 5f2c408036..df3261b32a 100644
>> >> --- a/hw/mem/memory-device.c
>> >> +++ b/hw/mem/memory-device.c
>> >> @@ -186,7 +186,6 @@ static uint64_t
>> >> memory_device_get_free_addr(MachineState *ms,
>> >>  if (!range_contains_range(, )) {
>> >>  error_setg(errp, "could not find position in guest address space 
>> >> for "
>> >> "memory device - memory fragmented due to 
>> >> alignments");
>> >> -goto out;
>> >
>> >Is it better to return 0 (or set new_addr to 0) for this error path and 
>> >another
>> remaining "goto out" path?
>> >
>> 
>> I may not get your point.
>> 
>> We set errp which is handled in its caller. By doing so, the error is 
>> propagated.
>> 
>> Do I miss something?
>
>Yes, you are right. Currently, the caller is checking errp, but not the 
>returned address, so there should be no issue.
>But when you see other error paths, you will find they all return 0. To be 
>aligned (return 0 when error), so just suggest also returning 0 for these two 
>"goto out" error path. :)
>

You may have some point.

Let's see whether others have the same taste, or we can refine it separately.

>Thanks,
>Star
>
>> 
>> >
>> >Thanks,
>> >Star
>> >
>> >>  }
>> >>  out:
>> >>  g_slist_free(list);
>> >> --
>> >> 2.17.1
>> >>
>> 
>> --
>> Wei Yang
>> Help you, Help me

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

2019-08-07 Thread Zeng, Star

> -Original Message-
> From: Wei Yang [mailto:richardw.y...@linux.intel.com]
> Sent: Thursday, August 8, 2019 10:13 AM
> To: Zeng, Star 
> Cc: Wei Yang ; qemu-devel@nongnu.org;
> imamm...@redhat.com; da...@redhat.com; m...@redhat.com
> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
> use goto for the last check
> 
> On Thu, Aug 08, 2019 at 01:42:14AM +, Zeng, Star wrote:
> >> -Original Message-
> >> From: Qemu-devel [mailto:qemu-devel-
> >> bounces+star.zeng=intel@nongnu.org] On Behalf Of Wei Yang
> >> Sent: Tuesday, July 30, 2019 8:38 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: imamm...@redhat.com; da...@redhat.com; Wei Yang
> >> ; m...@redhat.com
> >> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
> >> use goto for the last check
> >>
> >> We are already at the last condition check.
> >>
> >> Signed-off-by: Wei Yang 
> >> Reviewed-by: Igor Mammedov 
> >> Reviewed-by: David Hildenbrand 
> >> ---
> >>  hw/mem/memory-device.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index
> >> 5f2c408036..df3261b32a 100644
> >> --- a/hw/mem/memory-device.c
> >> +++ b/hw/mem/memory-device.c
> >> @@ -186,7 +186,6 @@ static uint64_t
> >> memory_device_get_free_addr(MachineState *ms,
> >>  if (!range_contains_range(, )) {
> >>  error_setg(errp, "could not find position in guest address space 
> >> for "
> >> "memory device - memory fragmented due to alignments");
> >> -goto out;
> >
> >Is it better to return 0 (or set new_addr to 0) for this error path and 
> >another
> remaining "goto out" path?
> >
> 
> I may not get your point.
> 
> We set errp which is handled in its caller. By doing so, the error is 
> propagated.
> 
> Do I miss something?

Yes, you are right. Currently, the caller is checking errp, but not the 
returned address, so there should be no issue.
But when you see other error paths, you will find they all return 0. To be 
aligned (return 0 when error), so just suggest also returning 0 for these two 
"goto out" error path. :)

Thanks,
Star

> 
> >
> >Thanks,
> >Star
> >
> >>  }
> >>  out:
> >>  g_slist_free(list);
> >> --
> >> 2.17.1
> >>
> 
> --
> Wei Yang
> Help you, Help me

Re: [Qemu-devel] [PATCH v3 05/14] hw/machine: add helper to query the memory encryption state

2019-08-07 Thread Singh, Brijesh


On 8/7/19 11:14 AM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.si...@amd.com) wrote:
>> To enable a memory encryption inside a VM, user must pass the object
>> name used for the encryption in command line parameter as shown below.
>>
>> # $(QEMU) \
>>   -machine memory-encryption=
>>
>> Add a helper machine_memory_encryption_enabled() which will return a bool
>> indicating whether the encryption object has been specified in the command
>> line parameter.
>>
>> Signed-off-by: Brijesh Singh 
> Reviewed-by: Dr. David Alan Gilbert 
>
> There's a check in accel/kvm/kvm-all.c:kvm_init which has:
>if (ms->memory_encryption) {
>
> which you might want to replace by this.


Ah, sure will make the changes in next rev. thanks


> Dave
>
>> ---
>>  hw/core/machine.c   | 5 +
>>  include/hw/boards.h | 1 +
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index c58a8e594e..f1e1b3661f 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -1031,6 +1031,11 @@ bool machine_mem_merge(MachineState *machine)
>>  return machine->mem_merge;
>>  }
>>  
>> +bool machine_memory_encryption_enabled(MachineState *machine)
>> +{
>> +return machine->memory_encryption ? true : false;
>> +}
>> +
>>  static char *cpu_slot_to_string(const CPUArchId *cpu)
>>  {
>>  GString *s = g_string_new(NULL);
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index a71d1a53a5..c5446a39cf 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -76,6 +76,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
>> Error **errp);
>>  
>>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char 
>> *type);
>> +bool machine_memory_encryption_enabled(MachineState *machine);
>>  
>>  
>>  /**
>> -- 
>> 2.17.1
>>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 03/14] migration.json: add AMD SEV specific migration parameters

2019-08-07 Thread Singh, Brijesh


On 8/7/19 6:06 AM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.si...@amd.com) wrote:
>> AMD SEV migration flow requires that target machine's public Diffie-Hellman
>> key (PDH) and certificate chain must be passed before initiating the guest
>> migration. User can use QMP 'migrate-set-parameters' to pass the certificate
>> chain. The certificate chain will be used while creating the outgoing
>> encryption context.
>>
>> Signed-off-by: Brijesh Singh 
>> ---
>>
>> I was able to pass the certificate chain through the HMP but somehow
>> QMP socket interface is not working for me. If anyone has any tips on
>> what I am missing in the patch then please let me know. In meantime,
>> I will also continue my investigation on why its not working for me.
> It looks OK to me; what's the qmp you're trying and what's the failure
> error?


I am not seeing any error. I am using the below command through qmp-shell.

(qmp) migrate-set-paramaters sev-pdh="" sev-plat-cert=""
sev-amd-cert="..."


The command does not return any error. I added some debugs in
migrate_params_test_apply() and qmp_migrate_set_parameters() to see the
valye of params->has_sev_pdh and its always zero. The functions are
getting called when I issue the migrate-set-parameters qmp but the
values are not passed hence the memory_encryption->setup() never gets
called.


> Dave
>
>>  migration/migration.c | 61 +++
>>  monitor/hmp-cmds.c| 18 +
>>  qapi/migration.json   | 41 ++---
>>  3 files changed, 116 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 8a607fe1e2..de66a0eb7e 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -783,6 +783,12 @@ MigrationParameters *qmp_query_migrate_parameters(Error 
>> **errp)
>>  params->announce_rounds = s->parameters.announce_rounds;
>>  params->has_announce_step = true;
>>  params->announce_step = s->parameters.announce_step;
>> +params->has_sev_pdh = true;
>> +params->sev_pdh = g_strdup(s->parameters.sev_pdh);
>> +params->has_sev_plat_cert = true;
>> +params->sev_plat_cert = g_strdup(s->parameters.sev_plat_cert);
>> +params->has_sev_amd_cert = true;
>> +params->sev_amd_cert = g_strdup(s->parameters.sev_amd_cert);
>>  
>>  return params;
>>  }
>> @@ -1289,6 +1295,18 @@ static void 
>> migrate_params_test_apply(MigrateSetParameters *params,
>>  if (params->has_announce_step) {
>>  dest->announce_step = params->announce_step;
>>  }
>> +if (params->has_sev_pdh) {
>> +assert(params->sev_pdh->type == QTYPE_QSTRING);
>> +dest->sev_pdh = g_strdup(params->sev_pdh->u.s);
>> +}
>> +if (params->has_sev_plat_cert) {
>> +assert(params->sev_plat_cert->type == QTYPE_QSTRING);
>> +dest->sev_plat_cert = g_strdup(params->sev_plat_cert->u.s);
>> +}
>> +if (params->has_sev_amd_cert) {
>> +assert(params->sev_amd_cert->type == QTYPE_QSTRING);
>> +dest->sev_amd_cert = g_strdup(params->sev_amd_cert->u.s);
>> +}
>>  }
>>  
>>  static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>> @@ -1390,6 +1408,21 @@ static void migrate_params_apply(MigrateSetParameters 
>> *params, Error **errp)
>>  if (params->has_announce_step) {
>>  s->parameters.announce_step = params->announce_step;
>>  }
>> +if (params->has_sev_pdh) {
>> +g_free(s->parameters.sev_pdh);
>> +assert(params->sev_pdh->type == QTYPE_QSTRING);
>> +s->parameters.sev_pdh = g_strdup(params->sev_pdh->u.s);
>> +}
>> +if (params->has_sev_plat_cert) {
>> +g_free(s->parameters.sev_plat_cert);
>> +assert(params->sev_plat_cert->type == QTYPE_QSTRING);
>> +s->parameters.sev_plat_cert = g_strdup(params->sev_plat_cert->u.s);
>> +}
>> +if (params->has_sev_amd_cert) {
>> +g_free(s->parameters.sev_amd_cert);
>> +assert(params->sev_amd_cert->type == QTYPE_QSTRING);
>> +s->parameters.sev_amd_cert = g_strdup(params->sev_amd_cert->u.s);
>> +}
>>  }
>>  
>>  void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>> @@ -1410,6 +1443,27 @@ void qmp_migrate_set_parameters(MigrateSetParameters 
>> *params, Error **errp)
>>  params->tls_hostname->type = QTYPE_QSTRING;
>>  params->tls_hostname->u.s = strdup("");
>>  }
>> +/* TODO Rewrite "" to null instead */
>> +if (params->has_sev_pdh
>> +&& params->sev_pdh->type == QTYPE_QNULL) {
>> +qobject_unref(params->sev_pdh->u.n);
>> +params->sev_pdh->type = QTYPE_QSTRING;
>> +params->sev_pdh->u.s = strdup("");
>> +}
>> +/* TODO Rewrite "" to null instead */
>> +if (params->has_sev_plat_cert
>> +&& params->sev_plat_cert->type == QTYPE_QNULL) {
>> +qobject_unref(params->sev_plat_cert->u.n);
>> +params->sev_plat_cert->type =

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

On Thu, Aug 08, 2019 at 01:42:14AM +, Zeng, Star wrote:
>> -Original Message-
>> From: Qemu-devel [mailto:qemu-devel-
>> bounces+star.zeng=intel@nongnu.org] On Behalf Of Wei Yang
>> Sent: Tuesday, July 30, 2019 8:38 AM
>> To: qemu-devel@nongnu.org
>> Cc: imamm...@redhat.com; da...@redhat.com; Wei Yang
>> ; m...@redhat.com
>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use
>> goto for the last check
>> 
>> We are already at the last condition check.
>> 
>> Signed-off-by: Wei Yang 
>> Reviewed-by: Igor Mammedov 
>> Reviewed-by: David Hildenbrand 
>> ---
>>  hw/mem/memory-device.c | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index
>> 5f2c408036..df3261b32a 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -186,7 +186,6 @@ static uint64_t
>> memory_device_get_free_addr(MachineState *ms,
>>  if (!range_contains_range(, )) {
>>  error_setg(errp, "could not find position in guest address space 
>> for "
>> "memory device - memory fragmented due to alignments");
>> -goto out;
>
>Is it better to return 0 (or set new_addr to 0) for this error path and 
>another remaining "goto out" path?
>

I may not get your point.

We set errp which is handled in its caller. By doing so, the error is
propagated.

Do I miss something?

>
>Thanks,
>Star
>
>>  }
>>  out:
>>  g_slist_free(list);
>> --
>> 2.17.1
>> 

-- 
Wei Yang
Help you, Help me

[Qemu-devel] [Fail] tests/test-util-filemonitor fails

Current qemu fails tests/test-util-filemonitor.

By bisect, it shows this commit introduced the error. 

commit ff3dc8fefe953fd3650279e064bf63b212c5699a
Author: Daniel P. Berrang茅 
Date:   Wed Mar 13 17:36:18 2019 +

filemon: ensure watch IDs are unique to QFileMonitor scope

The watch IDs are mistakenly only unique within the scope of the
directory being monitored. This is not useful for clients which are
monitoring multiple directories. They require watch IDs to be unique
globally within the QFileMonitor scope.

After revert 

"filemon: ensure watch IDs are unique to QFileMonitor scope"
"filemon: fix watch IDs to avoid potential wraparound issues"

The test pass.

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v7 4/9] block/nbd: add cmdline and qapi parameter reconnect-delay

On 6/18/19 6:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> Reconnect will be implemented in the following commit, so for now,
> in semantics below, disconnect itself is a "serious error".
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Eric Blake 
> ---
>  qapi/block-core.json | 11 ++-
>  block/nbd.c  | 16 +++-
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 61124431d8..17faf723e0 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3858,13 +3858,22 @@
>  #  traditional "base:allocation" block status (see
>  #  NBD_OPT_LIST_META_CONTEXT in the NBD protocol) (since 3.0)
>  #
> +# @reconnect-delay: On an unexpected disconnect, the nbd client tries to
> +#   connect again until succeeding or encountering a serious
> +#   error.  During the first @reconnect-delay seconds, all
> +#   requests are paused and will be rerun on a successful
> +#   reconnect. After that time, any delayed requests and all
> +#   future requests before a successful reconnect will
> +#   immediately fail. Default 0 (Since 4.1)

4.2 now; sorry for the delays.  I can make that change; I'll definitely
stage at least through this patch for my first pull request as soon as
4.2 opens next week; while still looking at the rest of the series to
see if it is ready to go as well.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3] migration/postcopy: use mis->bh instead of allocating a QEMUBH

On Wed, Aug 07, 2019 at 07:35:34PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> For migration incoming side, it either quit in precopy or postcopy. It
>> is safe to use the mis->bh for both instead of allocating a dedicated
>> QEMUBH for postcopy.
>> 
>> Signed-off-by: Wei Yang 
>> Reviewed-by: Dr. David Alan Gilbert 
>
>Hi Wei,
>  Can you check this, the patchew tests came back with a failure which
>seems bh related; I've not tried it, but can you just see if you can
>reproduce it?
>

I tried make check, which looks good.

(Some other upstream commit introduced error. I revert them to make check work)

>Dave
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v7 3/9] block/nbd: move from quit to state

On 6/18/19 6:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> To implement reconnect we need several states for the client:
> CONNECTED, QUIT and two different CONNECTING states. CONNECTING states
> will be added in the following patches. This patch implements CONNECTED
> and QUIT.
> 
> QUIT means, that we should close the connection and fail all current
> and further requests (like old quit = true).
> 
> CONNECTED means that connection is ok, we can send requests (like old
> quit = false).
> 
> For receiving loop we use a comparison of the current state with QUIT,
> because reconnect will be in the same loop, so it should be looping
> until the end.
> 
> Opposite, for requests we use a comparison of the current state with
> CONNECTED, as we don't want to send requests in future CONNECTING
> states.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Eric Blake 
> ---
>  block/nbd.c | 58 ++---
>  1 file changed, 37 insertions(+), 21 deletions(-)

> @@ -556,7 +572,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
>  s->requests[i].receiving = true;
>  qemu_coroutine_yield();
>  s->requests[i].receiving = false;
> -if (s->quit) {
> +if (s->state != NBD_CLIENT_CONNECTED) {
>  error_setg(errp, "Connection closed");
>  return -EIO;
>  }
> @@ -640,7 +656,7 @@ static coroutine_fn int nbd_co_receive_one_chunk(
>request_ret, qiov, payload, errp);
>  
>  if (ret < 0) {
> -s->quit = true;
> +nbd_channel_error(s, ret);

Minor merge conflict with changes in the meantime; easy enough to sort out.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit

Hi Palmer,

On Thu, Aug 8, 2019 at 4:55 AM Palmer Dabbelt  wrote:
>
> On Thu, Aug 1, 2019 at 7:58 AM Bin Meng  wrote:
>>
>> On Thu, Aug 1, 2019 at 10:16 PM Richard Henderson
>>  wrote:
>> >
>> > On 7/31/19 6:53 PM, Bin Meng wrote:
>> > > I am not sure how (idx * ptesize) could overflow. It represents the
>> > > offset by a page table which is [0, 4096).
>> >
>> > You're right, I mis-read what was going on there.
>> >
>> > However, lower down, "target_ulong ppn" needs to be promoted to hwaddr, so 
>> > that
>> >
>> > ppn = pte >> PTE_PPN_SHIFT;
>> > ...
>> > base = ppn << PGSHIFT;
>> >
>> > does not overflow.  (Which is the part of the page table walk that I 
>> > thought I
>> > had gleaned from the patch without actually reading the entire function.)
>>
>> Ah, yes. ppn should be promoted. Thanks for the review!
>
>
> Did I miss a v2?

No, I will send a v2 soon.

Regards,
Bin

Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

2019-08-07 Thread Zeng, Star

> -Original Message-
> From: Qemu-devel [mailto:qemu-devel-
> bounces+star.zeng=intel@nongnu.org] On Behalf Of Wei Yang
> Sent: Tuesday, July 30, 2019 8:38 AM
> To: qemu-devel@nongnu.org
> Cc: imamm...@redhat.com; da...@redhat.com; Wei Yang
> ; m...@redhat.com
> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use
> goto for the last check
> 
> We are already at the last condition check.
> 
> Signed-off-by: Wei Yang 
> Reviewed-by: Igor Mammedov 
> Reviewed-by: David Hildenbrand 
> ---
>  hw/mem/memory-device.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index
> 5f2c408036..df3261b32a 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -186,7 +186,6 @@ static uint64_t
> memory_device_get_free_addr(MachineState *ms,
>  if (!range_contains_range(, )) {
>  error_setg(errp, "could not find position in guest address space for 
> "
> "memory device - memory fragmented due to alignments");
> -goto out;

Is it better to return 0 (or set new_addr to 0) for this error path and another 
remaining "goto out" path?


Thanks,
Star

>  }
>  out:
>  g_slist_free(list);
> --
> 2.17.1
>

Re: [Qemu-devel] [PATCH v8] qemu-io: add pattern file for write command

On 8/7/19 2:06 AM, Denis Plotnikov wrote:
> The patch allows to provide a pattern file for write
> command. There was no similar ability before.
> 
> Signed-off-by: Denis Plotnikov 
> ---

>  
> +static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
> + const char *file_name)
> +{

No comment on the usage of this function? Existing practice in this file
is not the best, but new code can do better.

> +char *buf, *buf_origin;
> +FILE *f = fopen(file_name, "r");
> +int pattern_len;
> +
> +if (!f) {
> +perror(file_name);
> +return NULL;
> +}
> +
> +if (qemuio_misalign) {
> +len += MISALIGN_OFFSET;
> +}
> +
> +buf_origin = buf = blk_blockalign(blk, len);
> +
> +if (qemuio_misalign) {
> +buf_origin += MISALIGN_OFFSET;

Here, you are changing where you point...

> +}
> +
> +pattern_len = fread(buf_origin, 1, len, f);
> +
> +if (ferror(f)) {
> +perror(file_name);
> +goto error;
> +}

...but if you fail here...


> +
> +error:
> +qemu_vfree(buf_origin);

...then you free the wrong pointer.  This MUST use
qemu_io_free(buf_origin) (the same as write_f correctly does with the
misaligned pointer that you return on success).

> @@ -1051,8 +1114,9 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>  return -EINVAL;
>  }
>  
> -if (zflag && Pflag) {
> -printf("-z and -P cannot be specified at the same time\n");
> +if ((int)zflag + (int)Pflag + (int)sflag > 1) {

The casts to int are not necessary.  Adding two bools promotes to int
naturally.

> +printf("Only one of -z, -P, and -s"
> +   "can be specified at the same time\n");

Missing a space; you don't want your user to see "and -scan be".

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3] migration/postcopy: use mis->bh instead of allocating a QEMUBH

On Wed, Aug 07, 2019 at 07:35:34PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> For migration incoming side, it either quit in precopy or postcopy. It
>> is safe to use the mis->bh for both instead of allocating a dedicated
>> QEMUBH for postcopy.
>> 
>> Signed-off-by: Wei Yang 
>> Reviewed-by: Dr. David Alan Gilbert 
>
>Hi Wei,
>  Can you check this, the patchew tests came back with a failure which
>seems bh related; I've not tried it, but can you just see if you can
>reproduce it?

Hmm... the error message in mail is a little confusion.

I see following error, but have no idea about what it is.

==8174==ERROR: AddressSanitizer: heap-use-after-free on address 0x6122c7f0 
at pc 0x5566916cbf76 bp 0x7fc74f4b8680 sp 0x7fc74f4b8678
WRITE of size 1 at 0x6122c7f0 thread T9
==8179==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
PASS 3 ahci-test /x86_64/ahci/pci_enable
==8192==WARNING: ASan doesn't fully support makecontext/swapcontext functions 
and may produce false positives in some cases!
#0 0x5566916cbf75 in aio_notify /tmp/qemu-test/src/util/async.c:351:9
#1 0x5566916cdbab in qemu_bh_schedule /tmp/qemu-test/src/util/async.c:167:9
#2 0x5566916d0db0 in aio_co_schedule /tmp/qemu-test/src/util/async.c:464:5
---
  Right alloca redzone:cb
  Shadow gap:  cc
==8174==ABORTING
ERROR - too few tests run (expected 40, got 18)
make: *** [/tmp/qemu-test/src/tests/Makefile.include:904: check-unit] Error 1
make: *** Waiting for unfinished jobs
PASS 4 ahci-test /x86_64/ahci/hba_spec

I am trying to reproduce it. I guess docker should be used. Would you mind
sharing some link on setup the environment.

Thanks

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression

On 7/4/19 8:09 AM, Denis Plotnikov wrote:
> zstd significantly reduces cluster compression time.
> It provides better compression performance maintaining
> the same level of compression ratio in comparison with
> zlib, which, by the moment, has been the only compression

s/by/at/

> method available.
> 
> The performance test results:
> Test compresses and decompresses qemu qcow2 image with just
> installed rhel-7.6 guest.
> Image cluster size: 64K. Image on disk size: 2.2G
> 
> The test was conducted with brd disk to reduce the influence
> of disk subsystem to the test results.
> The results is given in seconds.
> 
> compress cmd:
>   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
>   src.img [zlib|zstd]_compressed.img
> decompress cmd
>   time ./qemu-img convert -O qcow2
>   [zlib|zstd]_compressed.img uncompressed.img
> 
>compression   decompression
>  zlib   zstd   zlib zstd
> 
> real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
> user 65.0   15.85.3  2.5
> sys   3.30.22.0  2.0
> 
> Both ZLIB and ZSTD gave the same compression ratio: 1.57
> compressed image size in both cases: 1.4G
> 

Nice numbers.

> +++ b/docs/interop/qcow2.txt
> @@ -538,6 +538,9 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 
> 8)):
>  Another compressed cluster may map to the tail of the 
> final
>  sector used by this compressed cluster.
>  
> +The layout of the compressed data depends on the 
> compression
> +type used for the image (see compressed cluster layout).
> +
>  If a cluster is unallocated, read requests shall read the data from the 
> backing
>  file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
>  no backing file or the backing file is smaller than the image, they shall 
> read
> @@ -790,3 +793,19 @@ In the image file the 'enabled' state is reflected by 
> the 'auto' flag. If this
>  flag is set, the software must consider the bitmap as 'enabled' and start
>  tracking virtual disk changes to this bitmap from the first write to the
>  virtual disk. If this flag is not set then the bitmap is disabled.
> +
> +=== Compressed cluster layout ===
> +
> +The compressed cluster data may have a different layout depending on the
> +compression type used for the image, and store specific data for the 
> particular
> +compression type.
> +
> +Compressed data layout for the available compression types:
> +(x = data_space_length - 1)
> +
> +zlib:
> +Byte  0 -  x: the compressed data content
> +  all the space provided used for compressed data
> +zstd:
> +Byte  0 -  3: the length of compressed data
> +  4 -  x: the compressed data content

Missing a change to the header description at bytes 104-107 calling out
'1' as meaning zstd (it only calls out '0' or absent as meaning zlib).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature

On 7/4/19 8:09 AM, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature to QCOW2 header that indicates that *all* compressed clusters
> must be (de)compressed using a certain compression type.
> 
> It is implied that the compression type is set on the image creation and
> can be changed only later by image conversion, thus compression type
> defines the only compression algorithm used for the image.
> 
> The goal of the feature is to add support of other compression algorithms
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
> It works roughly 2x faster than ZLIB providing a comparable compression ratio
> and therefore provide a performance advantage in backup scenarios.

provides

> 
> The default compression is ZLIB. Images created with ZLIB compression type
> are backward compatible with older qemu versions.
> 
> Signed-off-by: Denis Plotnikov 

> +++ b/docs/interop/qcow2.txt
> @@ -109,7 +109,12 @@ in the description of a field.
>  An External Data File Name header extension 
> may
>  be present if this bit is set.
>  
> -Bits 3-63:  Reserved (set to 0)
> +Bit 3:  Compression type bit. The bit must be set if
> +the compression type differs from default: 
> zlib.

I'd word this 'from the default of zlib.'

> +If the compression type is default the bit 
> must
> +be unset.

Why? I see no reason to forbid a qcow2 image that has the incompatible
bit set but still uses zlib compression.  True, such an image is not as
friendly to older clients, but it is not technically wrong, and newer
clients would still be able to use the image if not for this sentence
telling them they must not.  I'd drop this sentence.

> +
> +Bits 4-63:  Reserved (set to 0)
>  
>   80 -  87:  compatible_features
>  Bitmask of compatible features. An implementation can
> @@ -165,6 +170,20 @@ in the description of a field.
>  Length of the header structure in bytes. For version 2
>  images, the length is always assumed to be 72 bytes.
>  
> +104 - 107:  compression_type
> +Defines the compression method used for compressed 
> clusters.
> +A single compression type is applied to all compressed 
> image
> +clusters.
> +The compression type is set on image creation only.
> +The default compression type is zlib (value: 0).
> +When the compression type differs from the default
> +the compression type bit (incompatible feature bit 3)
> +must be set.

So far, so good.

> +Qemu versions older than 4.1 can use images created with
> +the default compression type without any additional
> +preparations and cannot use images created with any other
> +compression type.

I'm wondering whether we need to spell this out in the spec.  Yes, I
know we spell out other qemu limitations elsewhere, but with a version
number.  But the spec would not be any less correct if you omitted this
sentence.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature

On 8/7/19 6:12 PM, Max Reitz wrote:

>>  
>> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
>> +{
>> +switch (s->compression_type) {
>> +case QCOW2_COMPRESSION_TYPE_ZLIB:
>> +break;
>> +
>> +default:
>> +error_setg(errp, "qcow2: unknown compression type: %u",
>> +   s->compression_type);
>> +return -ENOTSUP;
>> +}
>> +
>> +/*
>> + * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
>> + * the incompatible feature flag must be set
>> + */
>> +
>> +if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB &&
>> +!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>> +error_setg(errp, "qcow2: Invalid compression type setting");
>> +return -EINVAL;
> 
> (1) Why is this block indented twice?
> 
> (2) Do we need this at all?  Sure, it’s a corruption, but do we need to
> reject the image because of it?

Yes, because otherwise there is a high risk of some application
misinterpreting the contents (whether an older qemu that silently
ignores unrecognized headers, and so assumes it can decode compressed
clusters with zlib even though the decode will only succeed with zstd,
or can write a compressed cluster with zlib which then causes corruption
when the newer qemu tries to read it with zstd).  The whole point of an
incompatible bit is to reject opening an image that can't be interpreted
correctly, and where writing may break later readers.

> 
>> +}
>> +
>> +return 0;
>> +}
>> +
> 
> Overall, I don’t see the purpose of this function.  I don’t see any need
> to call it in qcow2_update_header().  And without “does non-zlib
> compression imply the respective incompatible flag?” check, you can just
> inline the rest (checking that we recognize the compression type) into
> qcow2_do_open().
> 

Inlining may indeed be possible; the real question is whether the
function expands later in the series to the point that inlining no
longer makes sense.

>>  /* Called with s->lock held.  */
>>  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>int flags, Error **errp)
>> @@ -1318,6 +1344,35 @@ static int coroutine_fn 
>> qcow2_do_open(BlockDriverState *bs, QDict *options,
>>  s->compatible_features  = header.compatible_features;
>>  s->autoclear_features   = header.autoclear_features;
>>  
>> +/*
>> + * Handle compression type
>> + * Older qcow2 images don't contain the compression type header.
>> + * Distinguish them by the header length and use
>> + * the only valid (default) compression type in that case
>> + */
>> +if (header.header_length > offsetof(QCowHeader, compression_type)) {
>> +/* sanity check that we can read a compression type */
>> +size_t min_len = offsetof(QCowHeader, compression_type) +
>> + sizeof(header.compression_type);
>> +if (header.header_length < min_len) {
>> +error_setg(errp,
>> +   "Could not read compression type."
>> +   "qcow2 header is too short");
> 
> This will read as “Could not read compression type.qcow2 header is too
> short”.  There should be a space after the full stop (and the full stop
> should maybe be a comma instead).

Indeed, error_setg() should generally not contain '.'

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] tests/test-hbitmap: test next_zero and _next_dirty_area after truncate

2019-08-07 Thread John Snow




On 8/5/19 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
> Test that hbitmap_next_zero and hbitmap_next_dirty_area can find things
> after old bitmap end.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
> 
> It's a follow-up for 
> 
> [PATCH for-4.1] util/hbitmap: update orig_size on truncate
> 
>  tests/test-hbitmap.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
> index 592d8219db..eed5d288cb 100644
> --- a/tests/test-hbitmap.c
> +++ b/tests/test-hbitmap.c
> @@ -1004,6 +1004,15 @@ static void test_hbitmap_next_zero_4(TestHBitmapData 
> *data, const void *unused)
>  test_hbitmap_next_zero_do(data, 4);
>  }
>  
> +static void test_hbitmap_next_zero_after_truncate(TestHBitmapData *data,
> +  const void *unused)
> +{
> +hbitmap_test_init(data, L1, 0);
> +hbitmap_test_truncate_impl(data, L1 * 2);
> +hbitmap_set(data->hb, 0, L1);
> +test_hbitmap_next_zero_check(data, 0);
> +}
> +
>  static void test_hbitmap_next_dirty_area_check(TestHBitmapData *data,
> uint64_t offset,
> uint64_t count)
> @@ -1104,6 +1113,15 @@ static void 
> test_hbitmap_next_dirty_area_4(TestHBitmapData *data,
>  test_hbitmap_next_dirty_area_do(data, 4);
>  }
>  
> +static void test_hbitmap_next_dirty_area_after_truncate(TestHBitmapData 
> *data,
> +const void *unused)
> +{
> +hbitmap_test_init(data, L1, 0);
> +hbitmap_test_truncate_impl(data, L1 * 2);
> +hbitmap_set(data->hb, L1 + 1, 1);
> +test_hbitmap_next_dirty_area_check(data, 0, UINT64_MAX);
> +}
> +
>  int main(int argc, char **argv)
>  {
>  g_test_init(, , NULL);
> @@ -1169,6 +1187,8 @@ int main(int argc, char **argv)
>   test_hbitmap_next_zero_0);
>  hbitmap_test_add("/hbitmap/next_zero/next_zero_4",
>   test_hbitmap_next_zero_4);
> +hbitmap_test_add("/hbitmap/next_zero/next_zero_after_truncate",
> + test_hbitmap_next_zero_after_truncate);
>  
>  hbitmap_test_add("/hbitmap/next_dirty_area/next_dirty_area_0",
>   test_hbitmap_next_dirty_area_0);
> @@ -1176,6 +1196,8 @@ int main(int argc, char **argv)
>   test_hbitmap_next_dirty_area_1);
>  hbitmap_test_add("/hbitmap/next_dirty_area/next_dirty_area_4",
>   test_hbitmap_next_dirty_area_4);
> +
> hbitmap_test_add("/hbitmap/next_dirty_area/next_dirty_area_after_truncate",
> + test_hbitmap_next_dirty_area_after_truncate);
>  
>  g_test_run();
>  
> 

Tested-by: John Snow 
Reviewed-by: John Snow 

And staged:

Thanks, applied to my bitmaps tree:

https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-devel] [PATCH 0/3] backup fixes for 4.1?

2019-08-07 Thread John Snow




On 7/31/19 6:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.07.2019 21:41, John Snow wrote:
>>
>>
>> On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here are two small fixes.
>>>
>>> 01 is not a degradation at all, so it's OK for 4.2
>>> 02 is degradation of 3.0, so it's possibly OK for 4.2 too,
>>> but it seems to be real bug and fix is very simple, so,
>>> may be 4.1 is better
>>>
>>> Or you may take the whole series to 4.1 if you want.
>>>
>>
>> I think (1) and (2) can go in for stable after review, but they're not
>> crucial for 4.1 especially at this late of a stage. Should be cataclysms
>> only right now.

You found a cataclysm :(

>>
>> --js
>>
> 
> I can rebase it than on your bitmaps branch. Or, if we want it for stable, 
> maybe,
> I shouldn't?
> 

I rebased these two patches (1 and 3) on top of bitmaps, on top of rc4.

Thanks, applied to my bitmaps tree:

https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature

On 04.07.19 15:09, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature to QCOW2 header that indicates that *all* compressed clusters
> must be (de)compressed using a certain compression type.
> 
> It is implied that the compression type is set on the image creation and
> can be changed only later by image conversion, thus compression type
> defines the only compression algorithm used for the image.
> 
> The goal of the feature is to add support of other compression algorithms
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
> It works roughly 2x faster than ZLIB providing a comparable compression ratio
> and therefore provide a performance advantage in backup scenarios.
> 
> The default compression is ZLIB. Images created with ZLIB compression type
> are backward compatible with older qemu versions.
> 
> Signed-off-by: Denis Plotnikov 
> ---
>  block/qcow2.c | 95 +++
>  block/qcow2.h | 26 ---
>  docs/interop/qcow2.txt| 21 -
>  include/block/block_int.h |  1 +
>  qapi/block-core.json  | 22 -
>  5 files changed, 155 insertions(+), 10 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 3ace3b2209..8fa932a349 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1202,6 +1202,32 @@ static int qcow2_update_options(BlockDriverState *bs, 
> QDict *options,
>  return ret;
>  }
>  
> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
> +{
> +switch (s->compression_type) {
> +case QCOW2_COMPRESSION_TYPE_ZLIB:
> +break;
> +
> +default:
> +error_setg(errp, "qcow2: unknown compression type: %u",
> +   s->compression_type);
> +return -ENOTSUP;
> +}
> +
> +/*
> + * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
> + * the incompatible feature flag must be set
> + */
> +
> +if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB &&
> +!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> +error_setg(errp, "qcow2: Invalid compression type setting");
> +return -EINVAL;

(1) Why is this block indented twice?

(2) Do we need this at all?  Sure, it’s a corruption, but do we need to
reject the image because of it?

> +}
> +
> +return 0;
> +}
> +

Overall, I don’t see the purpose of this function.  I don’t see any need
to call it in qcow2_update_header().  And without “does non-zlib
compression imply the respective incompatible flag?” check, you can just
inline the rest (checking that we recognize the compression type) into
qcow2_do_open().

>  /* Called with s->lock held.  */
>  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>int flags, Error **errp)
> @@ -1318,6 +1344,35 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>  s->compatible_features  = header.compatible_features;
>  s->autoclear_features   = header.autoclear_features;
>  
> +/*
> + * Handle compression type
> + * Older qcow2 images don't contain the compression type header.
> + * Distinguish them by the header length and use
> + * the only valid (default) compression type in that case
> + */
> +if (header.header_length > offsetof(QCowHeader, compression_type)) {
> +/* sanity check that we can read a compression type */
> +size_t min_len = offsetof(QCowHeader, compression_type) +
> + sizeof(header.compression_type);
> +if (header.header_length < min_len) {
> +error_setg(errp,
> +   "Could not read compression type."
> +   "qcow2 header is too short");

This will read as “Could not read compression type.qcow2 header is too
short”.  There should be a space after the full stop (and the full stop
should maybe be a comma instead).

> +   ret = -EINVAL;
> +   goto fail;

These two lines are incorrectly aligned.

> +}
> +
> +header.compression_type = be32_to_cpu(header.compression_type);
> +s->compression_type = header.compression_type;
> +} else {
> +s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
> +}
> +
> +ret = check_compression_type(s, errp);
> +if (ret) {
> +goto fail;
> +}
> +
>  if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
>  void *feature_table = NULL;
>  qcow2_read_extensions(bs, header.header_length, ext_end,
> @@ -2434,6 +2489,13 @@ int qcow2_update_header(BlockDriverState *bs)
>  total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
>  refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 
> 3);
>  
> +ret = check_compression_type(s, NULL);
> +
> +if (ret) {
> +goto fail;
> +}
> +
> +

Again, I don’t see why this

Re: [Qemu-devel] [PATCH] migration: rename migration_bitmap_sync_range to ramblock_sync_dirty_bitmap

On Wed, Aug 07, 2019 at 06:49:48PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> Rename for better understanding of the code.
>> 
>> Suggested-by: Paolo Bonzini 
>> Signed-off-by: Wei Yang 
>
>this needs fixing after 'just pass RAMBlock is enough'
>

You mean rebase it? Hmm... let me take a look.

>Dave
>
>> ---
>>  migration/ram.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index c5f9f4b0ef..66792568e2 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1669,7 +1669,7 @@ static inline bool 
>> migration_bitmap_clear_dirty(RAMState *rs,
>>  return ret;
>>  }
>>  
>> -static void migration_bitmap_sync_range(RAMState *rs, RAMBlock *rb,
>> +static void ramblock_sync_dirty_bitmap(RAMState *rs, RAMBlock *rb,
>>  ram_addr_t length)
>>  {
>>  rs->migration_dirty_pages +=
>> @@ -1762,7 +1762,7 @@ static void migration_bitmap_sync(RAMState *rs)
>>  qemu_mutex_lock(>bitmap_mutex);
>>  rcu_read_lock();
>>  RAMBLOCK_FOREACH_NOT_IGNORED(block) {
>> -migration_bitmap_sync_range(rs, block, block->used_length);
>> +ramblock_sync_dirty_bitmap(rs, block, block->used_length);
>>  }
>>  ram_counters.remaining = ram_bytes_remaining();
>>  rcu_read_unlock();
>> @@ -4175,7 +4175,7 @@ static void colo_flush_ram_cache(void)
>>  memory_global_dirty_log_sync();
>>  rcu_read_lock();
>>  RAMBLOCK_FOREACH_NOT_IGNORED(block) {
>> -migration_bitmap_sync_range(ram_state, block, block->used_length);
>> +ramblock_sync_dirty_bitmap(ram_state, block, block->used_length);
>>  }
>>  rcu_read_unlock();
>>  
>> -- 
>> 2.17.1
>> 
>> 
>--
>Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

[Qemu-devel] bitmaps branch rebase

2019-08-07 Thread John Snow

FYI: I rebased jsnow/bitmaps on top of kwolf/block-next, itself based on
top of v4.1.0-rc4.

I'll post this along with the eventual pull request, but here's the
diffstat against the published patches:

011/33:[0003] [FC] 'block/backup: upgrade copy_bitmap to BdrvDirtyBitmap'
016/33:[] [-C] 'iotests: Add virtio-scsi device helper'
017/33:[0002] [FC] 'iotests: add test 257 for bitmap-mode backups'
030/33:[0001] [FC] 'block/backup: teach TOP to never copy unallocated
regions'
032/33:[0018] [FC] 'iotests/257: test traditional sync modes'

11: A new hbitmap call was added upstream, changed to
bdrv_dirty_bitmap_next_zero.
16: Context-only (self.has_quit is new context in 040)
17: Removed 'auto' to follow upstream trends in iotest fashion
30: Remove ret = -ECANCELED as agreed on-list;
Context changes for dirty_end patches
32: Fix capitalization in test, as mentioned on list.

I think the changes are actually fairly minimal and translate fairly
directly; let's review the rebase on-list in response to the PULL mails
when I send them.

Thanks,
--js

Re: [Qemu-devel] [PATCH v2 3/3] block-backend: Queue requests while drained

On 07.08.19 16:46, Kevin Wolf wrote:
> This fixes devices like IDE that can still start new requests from I/O
> handlers in the CPU thread while the block backend is drained.
> 
> The basic assumption is that in a drain section, no new requests should
> be allowed through a BlockBackend (blk_drained_begin/end don't exist,
> we get drain sections only on the node level). However, there are two
> special cases where requests should not be queued:
> 
> 1. Block jobs: We already make sure that block jobs are paused in a
>drain section, so they won't start new requests. However, if the
>drain_begin is called on the job's BlockBackend first, it can happen
>that we deadlock because the job stays busy until it reaches a pause
>point - which it can't if its requests aren't processed any more.
> 
>The proper solution here would be to make all requests through the
>job's filter node instead of using a BlockBackend. For now, just
>disabling request queuing on the job BlockBackend is simpler.
> 
> 2. In test cases where making requests through bdrv_* would be
>cumbersome because we'd need a BdrvChild. As we already got the
>functionality to disable request queuing from 1., use it in tests,
>too, for convenience.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/sysemu/block-backend.h |  1 +
>  block/backup.c |  1 +
>  block/block-backend.c  | 53 --
>  block/commit.c |  2 ++
>  block/mirror.c |  1 +
>  blockjob.c |  3 ++
>  tests/test-bdrv-drain.c|  1 +
>  7 files changed, 59 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 01/29] include: Make headers more self-contained

2019-08-07 Thread Alex Bennée



Markus Armbruster  writes:

> Alex Bennée  writes:
>
>> Markus Armbruster  writes:
>>
>>> Back in 2016, we discussed[1] rules for headers, and these were
>>> generally liked:
>>>
>>> 1. Have a carefully curated header that's included everywhere first.  We
>>>got that already thanks to Peter: osdep.h.
>>>
>>> 2. Headers should normally include everything they need beyond osdep.h.
>>>If exceptions are needed for some reason, they must be documented in
>>>the header.  If all that's needed from a header is typedefs, put
>>>those into qemu/typedefs.h instead of including the header.
>>>
>>> 3. Cyclic inclusion is forbidden.
>>>
>>> This patch gets include/ closer to obeying 2.
>>>
>>> It's actually extracted from my "[RFC] Baby steps towards saner
>>> headers" series[2], which demonstrates a possible path towards
>>> checking 2 automatically.  It passes the RFC test there.
>>>
>>> [1] Message-ID: <87h9g8j57d@blackfin.pond.sub.org>
>>> https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html
>>> [2] Message-Id: <20190711122827.18970-1-arm...@redhat.com>
>>> https://lists.nongnu.org/archive/html/qemu-devel/2019-07/msg02715.html
>>>
>>> Signed-off-by: Markus Armbruster 
>>> Reviewed-by: Alistair Francis 
>>> ---
>> 
>>>  include/exec/cputlb.h | 2 ++
>>>  include/exec/exec-all.h   | 1 +
>>>  include/exec/ioport.h | 2 ++
>>>  include/exec/memory-internal.h| 2 ++
>>>  include/exec/ram_addr.h   | 1 +
>>>  include/exec/softmmu-semi.h   | 2 ++
>>>  include/exec/tb-hash.h| 2 ++
>>>  include/exec/user/thunk.h | 1 +
>>>  include/fpu/softfloat-macros.h| 2 ++
>> 
>>>
>>>  /*
>>>   * bdrv_write_threshold_set:
>>> diff --git a/include/disas/disas.h b/include/disas/disas.h
>>> index 15da511f49..ba47e9197c 100644
>>> --- a/include/disas/disas.h
>>> +++ b/include/disas/disas.h
>>> @@ -1,6 +1,7 @@
>>>  #ifndef QEMU_DISAS_H
>>>  #define QEMU_DISAS_H
>>>
>>> +#include "exec/hwaddr.h"
>>>
>>>  #ifdef NEED_CPU_H
>>>  #include "cpu.h"
>>> diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h
>>> index 5373188be3..23abd71579 100644
>>> --- a/include/exec/cputlb.h
>>> +++ b/include/exec/cputlb.h
>>> @@ -19,6 +19,8 @@
>>>  #ifndef CPUTLB_H
>>>  #define CPUTLB_H
>>>
>>> +#include "exec/cpu-common.h"
>>> +
>>>  #if !defined(CONFIG_USER_ONLY)
>>>  /* cputlb.c */
>>>  void tlb_protect_code(ram_addr_t ram_addr);
>>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>>> index 16034ee651..135aeaab0d 100644
>>> --- a/include/exec/exec-all.h
>>> +++ b/include/exec/exec-all.h
>>> @@ -20,6 +20,7 @@
>>>  #ifndef EXEC_ALL_H
>>>  #define EXEC_ALL_H
>>>
>>> +#include "cpu.h"
>>>  #include "exec/tb-context.h"
>>>  #include "sysemu/cpus.h"
>>>
>>> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
>>> index a298b89ce1..97feb296d2 100644
>>> --- a/include/exec/ioport.h
>>> +++ b/include/exec/ioport.h
>>> @@ -24,6 +24,8 @@
>>>  #ifndef IOPORT_H
>>>  #define IOPORT_H
>>>
>>> +#include "exec/memory.h"
>>> +
>>>  #define MAX_IOPORTS (64 * 1024)
>>>  #define IOPORTS_MASK(MAX_IOPORTS - 1)
>>>
>>> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
>>> index d1a9dd1ec8..ef4fb92371 100644
>>> --- a/include/exec/memory-internal.h
>>> +++ b/include/exec/memory-internal.h
>>> @@ -20,6 +20,8 @@
>>>  #ifndef MEMORY_INTERNAL_H
>>>  #define MEMORY_INTERNAL_H
>>>
>>> +#include "cpu.h"
>>> +
>>>  #ifndef CONFIG_USER_ONLY
>>>  static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
>>>  {
>>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>>> index b7b2e60ff6..a327a80cfe 100644
>>> --- a/include/exec/ram_addr.h
>>> +++ b/include/exec/ram_addr.h
>>> @@ -20,6 +20,7 @@
>>>  #define RAM_ADDR_H
>>>
>>>  #ifndef CONFIG_USER_ONLY
>>> +#include "cpu.h"
>>>  #include "hw/xen/xen.h"
>>>  #include "sysemu/tcg.h"
>>>  #include "exec/ramlist.h"
>>> diff --git a/include/exec/softmmu-semi.h b/include/exec/softmmu-semi.h
>>> index 970837992e..fbcae88f4b 100644
>>> --- a/include/exec/softmmu-semi.h
>>> +++ b/include/exec/softmmu-semi.h
>>> @@ -10,6 +10,8 @@
>>>  #ifndef SOFTMMU_SEMI_H
>>>  #define SOFTMMU_SEMI_H
>>>
>>> +#include "cpu.h"
>>> +
>>>  static inline uint64_t softmmu_tget64(CPUArchState *env, target_ulong addr)
>>>  {
>>>  uint64_t val;
>>> diff --git a/include/exec/tb-hash.h b/include/exec/tb-hash.h
>>> index 4f3a37d927..805235d321 100644
>>> --- a/include/exec/tb-hash.h
>>> +++ b/include/exec/tb-hash.h
>>> @@ -20,6 +20,8 @@
>>>  #ifndef EXEC_TB_HASH_H
>>>  #define EXEC_TB_HASH_H
>>>
>>> +#include "exec/cpu-defs.h"
>>> +#include "exec/exec-all.h"
>>>  #include "qemu/xxhash.h"
>>>
>>>  #ifdef CONFIG_SOFTMMU
>>> diff --git a/include/exec/user/thunk.h b/include/exec/user/thunk.h
>>> index 8d3af5a3be..d05a8a4dab 100644
>>> --- a/include/exec/user/thunk.h
>>> +++ b/include/exec/user/thunk.h
>>> @@ -20,6 +20,7 @@
>>>  #define THUNK_H
>>>
>>>

Re: [Qemu-devel] [PATCH v2 2/3] mirror: Keep mirror_top_bs drained after dropping permissions

On 07.08.19 16:46, Kevin Wolf wrote:
> mirror_top_bs is currently implicitly drained through its connection to
> the source or the target node. However, the drain section for target_bs
> ends early after moving mirror_top_bs from src to target_bs, so that
> requests can already be restarted while mirror_top_bs is still present
> in the chain, but has dropped all permissions and therefore runs into an
> assertion failure like this:
> 
> qemu-system-x86_64: block/io.c:1634: bdrv_co_write_req_prepare:
> Assertion `child->perm & BLK_PERM_WRITE' failed.
> 
> Keep mirror_top_bs drained until all graph changes have completed.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/mirror.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 9f5c59ece1..642d6570cc 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -656,7 +656,10 @@ static int mirror_exit_common(Job *job)
>  s->target = NULL;
>  
>  /* We don't access the source any more. Dropping any WRITE/RESIZE is
> - * required before it could become a backing file of target_bs. */
> + * required before it could become a backing file of target_bs. Not 
> having
> + * these permissions any more means that we can't allow any new requests 
> on
> + * mirror_top_bs from now on, so keep it drained. */

You’re lucky Patchew broke or it would have complained about multi-line
comments needing /* and */ on separate lines. :-)

I’m perfectly happy with this style, though:

Reviewed-by: Max Reitz 

> +bdrv_drained_begin(mirror_top_bs);
>  bs_opaque->stop = true;
>  bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
>   _abort);
> @@ -724,6 +727,7 @@ static int mirror_exit_common(Job *job)
>  bs_opaque->job = NULL;
>  
>  bdrv_drained_end(src);
> +bdrv_drained_end(mirror_top_bs);
>  s->in_drain = false;
>  bdrv_unref(mirror_top_bs);
>  bdrv_unref(src);
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 12/29] Include hw/irq.h a lot less

On 8/7/19 8:04 AM, Philippe Mathieu-Daudé wrote:
> On 8/6/19 5:14 PM, Markus Armbruster wrote:
>> In my "build everything" tree, changing hw/irq.h triggers a recompile
>> of some 5400 out of 6600 objects (not counting tests and objects that
>> don't depend on qemu/osdep.h).
>>
>> hw/hw.h supposedly includes it for convenience.  Several other headers
>> include it just to get qemu_irq and.or qemu_irq_handler.
>>
>> Move the qemu_irq and qemu_irq_handler typedefs from hw/irq.h to
>> qemu/typedefs.h, and then include hw/irq.h only where it's still
>> needed.  Touching it now recompiles only some 500 objects.
>>

>>  /*
>>   * Function types
>>   */
>>  typedef void SaveStateHandler(QEMUFile *f, void *opaque);
>>  typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
>> +typedef void (*qemu_irq_handler)(void *opaque, int n, int level);

Should we prefer a consistent form for function pointer typedefs?  Here,
we've mixed 'rettype Name(params)' with 'rettype (*name)(params)'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 27/29] Include sysemu/sysemu.h a lot less

On 8/7/19 10:16 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé  writes:
> 
>> On 8/6/19 5:14 PM, Markus Armbruster wrote:
>>> In my "build everything" tree, changing sysemu/sysemu.h triggers a
>>> recompile of some 5400 out of 6600 objects (not counting tests and
>>> objects that don't depend on qemu/osdep.h).
>>>
>>> hw/qdev-core.h includes sysemu/sysemu.h since recent commit e965ffa70a
>>> "qdev: add qdev_add_vm_change_state_handler()".  This is a bad idea:
>>> hw/qdev-core.h is widely included.
>>>
>>> Move the declaration of qdev_add_vm_change_state_handler() to
>>> sysemu/sysemu.h, and drop the problematic include from hw/qdev-core.h.
>>>
>>> Touching sysemu/sysemu.h now recompiles some 1800 objects.
>>> qemu/uuid.h also drops from 5400 to 1800.  A few more headers show
>>> smaller improvement: qemu/notify.h drops from 5600 to 5200,
>>> qemu/timer.h from 5600 to 4500, and qapi/qapi-types-run-state.h from
>>> 5500 to 5000.
>>>
>>> Cc: Stefan Hajnoczi 
>>> Signed-off-by: Markus Armbruster 
>>> ---
>>>  accel/kvm/kvm-all.c   | 1 +
>>>  backends/hostmem.c| 1 +
>>>  cpus.c| 1 +
>>>  hw/arm/allwinner-a10.c| 1 +
>>>  hw/arm/aspeed_soc.c   | 1 +
>>>  hw/arm/kzm.c  | 1 +
>>>  hw/arm/msf2-soc.c | 1 +
>>>  hw/arm/stm32f205_soc.c| 1 +
>>>  hw/char/serial-isa.c  | 1 +
>>>  hw/char/xen_console.c | 1 +
>>>  hw/core/numa.c| 1 +
>>>  hw/core/vm-change-state-handler.c | 1 +
>>>  hw/display/qxl-render.c   | 1 +
>>>  hw/i386/xen/xen-hvm.c | 1 +
>>>  hw/i386/xen/xen-mapcache.c| 1 +
>>>  hw/intc/ioapic.c  | 1 +
>>>  hw/pci/pci.c  | 1 +
>>>  hw/riscv/sifive_e.c   | 1 +
>>>  hw/riscv/sifive_u.c   | 1 +
>>>  hw/riscv/spike.c  | 1 +
>>>  hw/riscv/virt.c   | 1 +
>>>  hw/sparc64/niagara.c  | 2 +-
>>>  hw/usb/hcd-ehci.h | 1 +
>>>  hw/xen/xen-common.c   | 1 +
>>>  hw/xen/xen_devconfig.c| 1 +
>>>  hw/xenpv/xen_machine_pv.c | 1 +
>>>  include/hw/qdev-core.h| 5 -
>>>  include/sysemu/sysemu.h   | 3 +++
>>>  migration/global_state.c  | 1 +
>>>  migration/migration.c | 1 +
>>>  migration/savevm.c| 1 +
>>>  31 files changed, 32 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index e1a44eccf5..fc38d0b9e3 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -29,6 +29,7 @@
>>>  #include "exec/gdbstub.h"
>>>  #include "sysemu/kvm_int.h"
>>>  #include "sysemu/cpus.h"
>>> +#include "sysemu/sysemu.h"
>>>  #include "qemu/bswap.h"
>>>  #include "exec/memory.h"
>>>  #include "exec/ram_addr.h"
>>
>> Include missing in net/netmap.c:
>>
>>   CC  net/netmap.o
>> net/netmap.c: In function 'netmap_update_fd_handler':
>> net/netmap.c:108:5: error: implicit declaration of function
>> 'qemu_set_fd_handler' [-Werror=implicit-function-declaration]
>>  qemu_set_fd_handler(s->nmd->fd,
>>  ^~~
>> net/netmap.c:108:5: error: nested extern declaration of
>> 'qemu_set_fd_handler' [-Werror=nested-externs]
> 
> Can you tell me offhand what I have to install so configure enables
> CONFIG_NETMAP?

The steps are listed in tests/docker/dockerfiles/debian-amd64.docker,
but you can get to this point running:

  $ make docker-image-debian-amd64 V=1 DEBUG=1

This will build the docker image with netmap (so you don't have to mess
with your workstation setup), then build QEMU within the image.

Re: [Qemu-devel] [Virtio-fs] [PATCH 0/4] virtiofsd: multithreading preparation part 3

2019-08-07 Thread Vivek Goyal

On Wed, Aug 07, 2019 at 07:03:55PM +0100, Stefan Hajnoczi wrote:
> On Thu, Aug 01, 2019 at 05:54:05PM +0100, Stefan Hajnoczi wrote:
> > Performance
> > ---
> > Please try these patches out and share your results.
> 
> Here are the performance numbers:
> 
>   Threadpool | iodepth | iodepth
>  size|1|   64
>   ---+-+
>   None   |   4451  |  4876
>   1  |   4360  |  4858
>   64 |   4359  | 33,266
> 
> A graph is available here:
> https://vmsplice.net/~stefan/virtiofsd-threadpool-performance.png
> 
> Summary:
> 
>  * iodepth=64 performance is increased by 6.8 times.
>  * iodepth=1 performance degrades by 2%.
>  * DAX is bottlenecked by QEMU's single-threaded
>VHOST_USER_SLAVE_FS_MAP/UNMAP handler.
> 
> Threadpool size "none" is virtiofsd commit 813a824b707 ("virtiofsd: use
> fuse_lowlevel_is_virtio() in fuse_session_destroy()") without any of the
> multithreading preparation patches.  I benchmarked this to check whether
> the patches introduce a regression for iodepth=1.  They do, but it's
> only around 2%.
> 
> I also ran with DAX but found there was not much difference between
> iodepth=1 and iodepth=64.  This might be because the host mmap(2)
> syscall becomes the bottleneck and a serialization point.  QEMU only
> processes one VHOST_USER_SLAVE_FS_MAP/UNMAP at a time.  If we want to
> accelerate DAX it may be necessary to parallelize mmap, assuming the
> host kernel can do them in parallel on a single file.  This performance
> optimization is future work and not directly related to this patch
> series.

Good to see nice improvement with higher queue depth.

Kernel also serializes MAP/UNMAP on one inode. So you will need to run
multiple jobs operating on different inodes to see parallel MAP/UNMAP
(atleast from kernel's point of view).

Thanks
Vivek

Re: [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit

2019-08-07 Thread Palmer Dabbelt

On Thu, Aug 1, 2019 at 7:58 AM Bin Meng  wrote:

> On Thu, Aug 1, 2019 at 10:16 PM Richard Henderson
>  wrote:
> >
> > On 7/31/19 6:53 PM, Bin Meng wrote:
> > > I am not sure how (idx * ptesize) could overflow. It represents the
> > > offset by a page table which is [0, 4096).
> >
> > You're right, I mis-read what was going on there.
> >
> > However, lower down, "target_ulong ppn" needs to be promoted to hwaddr,
> so that
> >
> > ppn = pte >> PTE_PPN_SHIFT;
> > ...
> > base = ppn << PGSHIFT;
> >
> > does not overflow.  (Which is the part of the page table walk that I
> thought I
> > had gleaned from the patch without actually reading the entire function.)
>
> Ah, yes. ppn should be promoted. Thanks for the review!
>

Did I miss a v2?

[Qemu-devel] [PATCH v2] RISC-V: Ignore the S and U extensions when formatting ISA strings

2019-08-07 Thread Palmer Dabbelt

The ISA strings we're providing from QEMU aren't actually legal RISC-V
ISA strings, as both the S and U extensions cannot exist as
single-letter extensions and must instead be multi-letter strings.
We're still using the ISA strings inside QEMU to track the availiable
extensions, so this patch just strips out the S and U extensions when
formatting ISA strings.

This boots Linux on top of 4.1-rc3, which no longer has the U extension
in /proc/cpuinfo.

Signed-off-by: Palmer Dabbelt 
---
 target/riscv/cpu.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f8d07bd20ad7..74f3449c4918 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -501,7 +501,22 @@ char *riscv_isa_string(RISCVCPU *cpu)
 char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
 for (i = 0; i < sizeof(riscv_exts); i++) {
 if (cpu->env.misa & RV(riscv_exts[i])) {
-*p++ = qemu_tolower(riscv_exts[i]);
+char lower = qemu_tolower(riscv_exts[i]);
+switch (lower) {
+case 's':
+case 'u':
+/*
+ * The 's' and 'u' extensions shouldn't be passed in the device
+ * tree, but we still use them internally to track extension
+ * sets.  Here we just explicitly remove them when formatting
+ * an ISA string.
+ */
+break;
+
+default:
+*p++ = lower;
+break;
+}
 }
 }
 *p = '\0';
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 00/29] Tame a few "touch this, recompile the world" headers

Alex Bennée  writes:

> Markus Armbruster  writes:
>
>> We have quite a few "touch this, recompile the world" headers.  My
>> "build everything" tree has some 6600 objects (not counting tests and
>> objects that don't depend on qemu/osdep.h).  Touching any of 54
>> headers triggers a recompile of more than half of them.
>>
>> This series reduces them to 46.
>
> I think this series is going the right way but there seems to be quite a
> lot of breakage introduced to the cross compiles:
>
>   https://app.shippable.com/github/stsquad/qemu/runs/939/summary/console
>
> I guess there is more subtlety involved when host != target. I'd
> recommend setting up a shippable account:
>
>   https://wiki.qemu.org/Testing/CI/Shippable
>
> You can of course just run:
>
>   make docker-test-build J=n
>
> And watch your machine slowly grind through all the options.

Thanks for the tips.

Re: [Qemu-devel] [PATCH v2 28/29] sysemu: Move the VMChangeStateEntry typedef to qemu/typedefs.h

Alex Bennée  writes:

> Markus Armbruster  writes:
>
>> In my "build everything" tree, changing sysemu/sysemu.h triggers a
>> recompile of some 1800 out of 6600 objects (not counting tests and
>> objects that don't depend on qemu/osdep.h, down from 5400 due to the
>> previous commit).
>>
>> Several headers include sysemu/sysemu.h just to get typedef
>> VMChangeStateEntry.  Move it from sysemu/sysemu.h to qemu/typedefs.h.
>> Spell its structure tag the same while there.
>>
>> Touching sysemu/sysemu.h now recompiles some 1100 objects.
>> qemu/uuid.h also drops from 1800 to 1100, and
>> qapi/qapi-types-run-state.h from 5000 to 4400.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>  hw/block/vhost-user-blk.c   | 1 +
>>  hw/block/virtio-blk.c   | 1 +
>>  hw/display/virtio-gpu.c | 1 +
>>  hw/misc/macio/macio.c   | 1 +
>>  hw/net/virtio-net.c | 1 +
>>  hw/s390x/s390-ccw.c | 1 +
>>  hw/s390x/s390-virtio-ccw.c  | 1 +
>>  hw/scsi/scsi-bus.c  | 1 +
>>  hw/scsi/vhost-scsi.c| 1 +
>>  hw/scsi/vhost-user-scsi.c   | 1 +
>>  hw/usb/hcd-ehci.c   | 1 +
>>  hw/usb/hcd-ehci.h   | 1 -
>>  hw/virtio/virtio-rng.c  | 1 +
>>  hw/virtio/virtio.c  | 1 +
>>  include/hw/ide/internal.h   | 3 ++-
>>  include/hw/ppc/spapr_xive.h | 1 -
>>  include/hw/scsi/scsi.h  | 1 -
>>  include/hw/virtio/virtio.h  | 1 -
>>  include/qemu/typedefs.h | 1 +
>>  include/sysemu/sysemu.h | 1 -
>>  vl.c| 6 +++---
>>  21 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index 7b44cca6d9..6b6cd07362 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -28,6 +28,7 @@
>>  #include "hw/virtio/virtio.h"
>>  #include "hw/virtio/virtio-bus.h"
>>  #include "hw/virtio/virtio-access.h"
>> +#include "sysemu/sysemu.h"
>>
>>  static const int user_feature_bits[] = {
>>  VIRTIO_BLK_F_SIZE_MAX,
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 8cc2a232e0..78ac371eba 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -20,6 +20,7 @@
>>  #include "hw/block/block.h"
>>  #include "hw/qdev-properties.h"
>>  #include "sysemu/blockdev.h"
>> +#include "sysemu/sysemu.h"
>>  #include "hw/virtio/virtio-blk.h"
>>  #include "dataplane/virtio-blk.h"
>>  #include "scsi/constants.h"
>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>> index 6de9689a30..28e868c021 100644
>> --- a/hw/display/virtio-gpu.c
>> +++ b/hw/display/virtio-gpu.c
>> @@ -17,6 +17,7 @@
>>  #include "ui/console.h"
>>  #include "trace.h"
>>  #include "sysemu/dma.h"
>> +#include "sysemu/sysemu.h"
>>  #include "hw/virtio/virtio.h"
>>  #include "migration/qemu-file-types.h"
>>  #include "hw/virtio/virtio-gpu.h"
>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>> index b59df4e3b8..50f20d8206 100644
>> --- a/hw/misc/macio/macio.c
>> +++ b/hw/misc/macio/macio.c
>> @@ -35,6 +35,7 @@
>>  #include "hw/char/escc.h"
>>  #include "hw/misc/macio/macio.h"
>>  #include "hw/intc/heathrow_pic.h"
>> +#include "sysemu/sysemu.h"
>>  #include "trace.h"
>>
>>  /* Note: this code is strongly inspirated from the corresponding code
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 4113729fcf..9f11422337 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -31,6 +31,7 @@
>>  #include "hw/virtio/virtio-access.h"
>>  #include "migration/misc.h"
>>  #include "standard-headers/linux/ethtool.h"
>> +#include "sysemu/sysemu.h"
>>  #include "trace.h"
>>
>>  #define VIRTIO_NET_VM_VERSION11
>> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
>> index 22c6878b84..0c5a5b60bd 100644
>> --- a/hw/s390x/s390-ccw.c
>> +++ b/hw/s390x/s390-ccw.c
>> @@ -19,6 +19,7 @@
>>  #include "hw/s390x/css.h"
>>  #include "hw/s390x/css-bridge.h"
>>  #include "hw/s390x/s390-ccw.h"
>> +#include "sysemu/sysemu.h"
>>
>>  IOInstEnding s390_ccw_cmd_request(SubchDev *sch)
>>  {
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index a543b64e56..434d933ec9 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -40,6 +40,7 @@
>>  #include "hw/nmi.h"
>>  #include "hw/qdev-properties.h"
>>  #include "hw/s390x/tod.h"
>> +#include "sysemu/sysemu.h"
>>
>>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>  {
>> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
>> index db785e6001..cb8e8d1f36 100644
>> --- a/hw/scsi/scsi-bus.c
>> +++ b/hw/scsi/scsi-bus.c
>> @@ -10,6 +10,7 @@
>>  #include "scsi/constants.h"
>>  #include "sysemu/block-backend.h"
>>  #include "sysemu/blockdev.h"
>> +#include "sysemu/sysemu.h"
>>  #include "trace.h"
>>  #include "sysemu/dma.h"
>>  #include "qemu/cutils.h"
>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>> index cd5cf1679d..c693fc748a 100644
>> --- a/hw/scsi/vhost-scsi.c
>> +++ b/hw/scsi/vhost-scsi.c
>> @@ -30,6 +30,7 @@
>>  #include "hw/fw-path-provider.h"
>>  #include

Re: [Qemu-devel] [PATCH v2 27/29] Include sysemu/sysemu.h a lot less

Philippe Mathieu-Daudé  writes:

> On 8/6/19 5:14 PM, Markus Armbruster wrote:
>> In my "build everything" tree, changing sysemu/sysemu.h triggers a
>> recompile of some 5400 out of 6600 objects (not counting tests and
>> objects that don't depend on qemu/osdep.h).
>> 
>> hw/qdev-core.h includes sysemu/sysemu.h since recent commit e965ffa70a
>> "qdev: add qdev_add_vm_change_state_handler()".  This is a bad idea:
>> hw/qdev-core.h is widely included.
>> 
>> Move the declaration of qdev_add_vm_change_state_handler() to
>> sysemu/sysemu.h, and drop the problematic include from hw/qdev-core.h.
>> 
>> Touching sysemu/sysemu.h now recompiles some 1800 objects.
>> qemu/uuid.h also drops from 5400 to 1800.  A few more headers show
>> smaller improvement: qemu/notify.h drops from 5600 to 5200,
>> qemu/timer.h from 5600 to 4500, and qapi/qapi-types-run-state.h from
>> 5500 to 5000.
>> 
>> Cc: Stefan Hajnoczi 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  accel/kvm/kvm-all.c   | 1 +
>>  backends/hostmem.c| 1 +
>>  cpus.c| 1 +
>>  hw/arm/allwinner-a10.c| 1 +
>>  hw/arm/aspeed_soc.c   | 1 +
>>  hw/arm/kzm.c  | 1 +
>>  hw/arm/msf2-soc.c | 1 +
>>  hw/arm/stm32f205_soc.c| 1 +
>>  hw/char/serial-isa.c  | 1 +
>>  hw/char/xen_console.c | 1 +
>>  hw/core/numa.c| 1 +
>>  hw/core/vm-change-state-handler.c | 1 +
>>  hw/display/qxl-render.c   | 1 +
>>  hw/i386/xen/xen-hvm.c | 1 +
>>  hw/i386/xen/xen-mapcache.c| 1 +
>>  hw/intc/ioapic.c  | 1 +
>>  hw/pci/pci.c  | 1 +
>>  hw/riscv/sifive_e.c   | 1 +
>>  hw/riscv/sifive_u.c   | 1 +
>>  hw/riscv/spike.c  | 1 +
>>  hw/riscv/virt.c   | 1 +
>>  hw/sparc64/niagara.c  | 2 +-
>>  hw/usb/hcd-ehci.h | 1 +
>>  hw/xen/xen-common.c   | 1 +
>>  hw/xen/xen_devconfig.c| 1 +
>>  hw/xenpv/xen_machine_pv.c | 1 +
>>  include/hw/qdev-core.h| 5 -
>>  include/sysemu/sysemu.h   | 3 +++
>>  migration/global_state.c  | 1 +
>>  migration/migration.c | 1 +
>>  migration/savevm.c| 1 +
>>  31 files changed, 32 insertions(+), 6 deletions(-)
>> 
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index e1a44eccf5..fc38d0b9e3 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -29,6 +29,7 @@
>>  #include "exec/gdbstub.h"
>>  #include "sysemu/kvm_int.h"
>>  #include "sysemu/cpus.h"
>> +#include "sysemu/sysemu.h"
>>  #include "qemu/bswap.h"
>>  #include "exec/memory.h"
>>  #include "exec/ram_addr.h"
>
> Include missing in net/netmap.c:
>
>   CC  net/netmap.o
> net/netmap.c: In function 'netmap_update_fd_handler':
> net/netmap.c:108:5: error: implicit declaration of function
> 'qemu_set_fd_handler' [-Werror=implicit-function-declaration]
>  qemu_set_fd_handler(s->nmd->fd,
>  ^~~
> net/netmap.c:108:5: error: nested extern declaration of
> 'qemu_set_fd_handler' [-Werror=nested-externs]

Can you tell me offhand what I have to install so configure enables
CONFIG_NETMAP?

Re: [Qemu-devel] [PATCH v2 26/29] Clean up inclusion of sysemu/sysemu.h

Alex Bennée  writes:

> Markus Armbruster  writes:
>
>> In my "build everything" tree, changing sysemu/sysemu.h triggers a
>> recompile of some 5400 out of 6600 objects (not counting tests and
>> objects that don't depend on qemu/osdep.h).
>>
>> 119 of 380 #include directives are actually superfluous.  Delete them.
>> Downgrade two more to qapi/qapi-types-run-state.h, and move one from
>> char/serial.h to char/serial.c.
>>
>> This doesn't reduce actual use much, as it's still included into
>> widely included headers.  The next commit will tackle that.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
> 
>>  hw/semihosting/config.c | 1 +
> 
>>  stubs/semihost.c| 1 +
> 
>> diff --git a/hw/semihosting/config.c b/hw/semihosting/config.c
>> index 2a8e7e1045..9807f10cb0 100644
>> --- a/hw/semihosting/config.c
>> +++ b/hw/semihosting/config.c
>> @@ -24,6 +24,7 @@
>>  #include "qemu/error-report.h"
>>  #include "hw/semihosting/semihost.h"
>>  #include "chardev/char.h"
>> +#include "sysemu/sysemu.h"
>>
>>  QemuOptsList qemu_semihosting_config_opts = {
>>  .name = "semihosting-config",
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index b8332150f1..9f3cff5fb6 100644
> 
>>
>> diff --git a/stubs/semihost.c b/stubs/semihost.c
>> index 4d5b3c0653..f90589259c 100644
>> --- a/stubs/semihost.c
>> +++ b/stubs/semihost.c
>> @@ -12,6 +12,7 @@
>>  #include "qemu/option.h"
>>  #include "qemu/error-report.h"
>>  #include "hw/semihosting/semihost.h"
>> +#include "sysemu/sysemu.h"
> 
>
> These additions seem out of place. If I comment them out I can still
> build fine

sysemu/sysemu.h declares qemu_semihosting_config_opts,
hw/semihosting/config.c and stubs/semihost.c define it.

Gcc warns when you do that for functions (-Wmissing-declarations
-Wmissing-prototypes), but not for variables.  I like to include the
header anyway, to make sure the compiler checks the declaration is
consistent with the definition.

>- I think the only place that needs them is vl.c so it has a
> typedef for the semihosting configure options. Arguably the extern
> declaration could be moved into semihostings own headers to avoid
> polluting sysemu.h more than it needs to?

I'm not sure I'm following you.

What would you like me to move where?

Re: [Qemu-devel] [PATCH v2 09/29] Include migration/qemu-file-types.h a lot less

Philippe Mathieu-Daudé  writes:

> On 8/7/19 2:25 PM, Philippe Mathieu-Daudé wrote:
>> On 8/6/19 5:14 PM, Markus Armbruster wrote:
>>> In my "build everything" tree, changing migration/qemu-file-types.h
>>> triggers a recompile of some 2600 out of 6600 objects (not counting
>>> tests and objects that don't depend on qemu/osdep.h).
>>>
>>> The culprit is again hw/hw.h, which supposedly includes it for
>>> convenience.
>>>
>>> Include migration/qemu-file-types.h only where it's needed.  Touching
>>> it now recompiles less than 200 objects.
>>>
>>> Signed-off-by: Markus Armbruster 
>>> ---
>>>  hw/acpi/piix4.c | 1 +
>>>  hw/block/virtio-blk.c   | 1 +
>>>  hw/char/virtio-serial-bus.c | 1 +
>>>  hw/display/virtio-gpu.c | 1 +
>>>  hw/intc/apic_common.c   | 1 +
>>>  hw/nvram/eeprom93xx.c   | 1 +
>>>  hw/nvram/fw_cfg.c   | 1 +
>>>  hw/pci-host/piix.c  | 1 +
>>>  hw/pci/msix.c   | 1 +
>>>  hw/pci/pci.c| 1 +
>>>  hw/pci/shpc.c   | 1 +
>>>  hw/ppc/spapr.c  | 1 +
>>>  hw/s390x/s390-skeys.c   | 1 +
>>>  hw/s390x/tod.c  | 1 +
>>>  hw/s390x/virtio-ccw.c   | 1 +
>>>  hw/scsi/mptsas.c| 1 +
>>>  hw/scsi/scsi-bus.c  | 1 +
>>>  hw/scsi/scsi-disk.c | 1 +
>>>  hw/scsi/scsi-generic.c  | 1 +
>>>  hw/scsi/virtio-scsi.c   | 1 +
>>>  hw/timer/i8254_common.c | 1 +
>>>  hw/timer/twl92230.c | 1 +
>>>  hw/usb/redirect.c   | 1 +
>>>  hw/virtio/vhost.c   | 1 +
>>>  hw/virtio/virtio-mmio.c | 1 +
>>>  hw/virtio/virtio-pci.c  | 1 +
>>>  hw/virtio/virtio.c  | 1 +
>>>  include/hw/hw.h | 1 -
>>>  include/migration/cpu.h | 1 +
>>>  target/ppc/kvm.c| 1 +
>>>  30 files changed, 29 insertions(+), 1 deletion(-)
>> [...]
>>> diff --git a/include/hw/hw.h b/include/hw/hw.h
>>> index a4fb2390e8..b399627cbe 100644
>>> --- a/include/hw/hw.h
>>> +++ b/include/hw/hw.h
>>> @@ -11,7 +11,6 @@
>>>  #include "exec/memory.h"
>>>  #include "hw/irq.h"
>>>  #include "migration/vmstate.h"
>>> -#include "migration/qemu-file-types.h"
>> 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Tested-by: Philippe Mathieu-Daudé 
>
> Oops another miss:
>
> hw/intc/s390_flic_kvm.c: In function 'kvm_flic_save':
> hw/intc/s390_flic_kvm.c:395:9: error: implicit declaration of function
> 'qemu_put_be64' [-Werror=implicit-function-declaration]
>  qemu_put_be64(f, FLIC_FAILED);
>  ^

Patchew found this, too.  I'll fix it.  Thanks!

Re: [Qemu-devel] [PATCH v2 01/29] include: Make headers more self-contained

Alex Bennée  writes:

> Markus Armbruster  writes:
>
>> Back in 2016, we discussed[1] rules for headers, and these were
>> generally liked:
>>
>> 1. Have a carefully curated header that's included everywhere first.  We
>>got that already thanks to Peter: osdep.h.
>>
>> 2. Headers should normally include everything they need beyond osdep.h.
>>If exceptions are needed for some reason, they must be documented in
>>the header.  If all that's needed from a header is typedefs, put
>>those into qemu/typedefs.h instead of including the header.
>>
>> 3. Cyclic inclusion is forbidden.
>>
>> This patch gets include/ closer to obeying 2.
>>
>> It's actually extracted from my "[RFC] Baby steps towards saner
>> headers" series[2], which demonstrates a possible path towards
>> checking 2 automatically.  It passes the RFC test there.
>>
>> [1] Message-ID: <87h9g8j57d@blackfin.pond.sub.org>
>> https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html
>> [2] Message-Id: <20190711122827.18970-1-arm...@redhat.com>
>> https://lists.nongnu.org/archive/html/qemu-devel/2019-07/msg02715.html
>>
>> Signed-off-by: Markus Armbruster 
>> Reviewed-by: Alistair Francis 
>> ---
> 
>>  include/exec/cputlb.h | 2 ++
>>  include/exec/exec-all.h   | 1 +
>>  include/exec/ioport.h | 2 ++
>>  include/exec/memory-internal.h| 2 ++
>>  include/exec/ram_addr.h   | 1 +
>>  include/exec/softmmu-semi.h   | 2 ++
>>  include/exec/tb-hash.h| 2 ++
>>  include/exec/user/thunk.h | 1 +
>>  include/fpu/softfloat-macros.h| 2 ++
> 
>>
>>  /*
>>   * bdrv_write_threshold_set:
>> diff --git a/include/disas/disas.h b/include/disas/disas.h
>> index 15da511f49..ba47e9197c 100644
>> --- a/include/disas/disas.h
>> +++ b/include/disas/disas.h
>> @@ -1,6 +1,7 @@
>>  #ifndef QEMU_DISAS_H
>>  #define QEMU_DISAS_H
>>
>> +#include "exec/hwaddr.h"
>>
>>  #ifdef NEED_CPU_H
>>  #include "cpu.h"
>> diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h
>> index 5373188be3..23abd71579 100644
>> --- a/include/exec/cputlb.h
>> +++ b/include/exec/cputlb.h
>> @@ -19,6 +19,8 @@
>>  #ifndef CPUTLB_H
>>  #define CPUTLB_H
>>
>> +#include "exec/cpu-common.h"
>> +
>>  #if !defined(CONFIG_USER_ONLY)
>>  /* cputlb.c */
>>  void tlb_protect_code(ram_addr_t ram_addr);
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index 16034ee651..135aeaab0d 100644
>> --- a/include/exec/exec-all.h
>> +++ b/include/exec/exec-all.h
>> @@ -20,6 +20,7 @@
>>  #ifndef EXEC_ALL_H
>>  #define EXEC_ALL_H
>>
>> +#include "cpu.h"
>>  #include "exec/tb-context.h"
>>  #include "sysemu/cpus.h"
>>
>> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
>> index a298b89ce1..97feb296d2 100644
>> --- a/include/exec/ioport.h
>> +++ b/include/exec/ioport.h
>> @@ -24,6 +24,8 @@
>>  #ifndef IOPORT_H
>>  #define IOPORT_H
>>
>> +#include "exec/memory.h"
>> +
>>  #define MAX_IOPORTS (64 * 1024)
>>  #define IOPORTS_MASK(MAX_IOPORTS - 1)
>>
>> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
>> index d1a9dd1ec8..ef4fb92371 100644
>> --- a/include/exec/memory-internal.h
>> +++ b/include/exec/memory-internal.h
>> @@ -20,6 +20,8 @@
>>  #ifndef MEMORY_INTERNAL_H
>>  #define MEMORY_INTERNAL_H
>>
>> +#include "cpu.h"
>> +
>>  #ifndef CONFIG_USER_ONLY
>>  static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
>>  {
>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>> index b7b2e60ff6..a327a80cfe 100644
>> --- a/include/exec/ram_addr.h
>> +++ b/include/exec/ram_addr.h
>> @@ -20,6 +20,7 @@
>>  #define RAM_ADDR_H
>>
>>  #ifndef CONFIG_USER_ONLY
>> +#include "cpu.h"
>>  #include "hw/xen/xen.h"
>>  #include "sysemu/tcg.h"
>>  #include "exec/ramlist.h"
>> diff --git a/include/exec/softmmu-semi.h b/include/exec/softmmu-semi.h
>> index 970837992e..fbcae88f4b 100644
>> --- a/include/exec/softmmu-semi.h
>> +++ b/include/exec/softmmu-semi.h
>> @@ -10,6 +10,8 @@
>>  #ifndef SOFTMMU_SEMI_H
>>  #define SOFTMMU_SEMI_H
>>
>> +#include "cpu.h"
>> +
>>  static inline uint64_t softmmu_tget64(CPUArchState *env, target_ulong addr)
>>  {
>>  uint64_t val;
>> diff --git a/include/exec/tb-hash.h b/include/exec/tb-hash.h
>> index 4f3a37d927..805235d321 100644
>> --- a/include/exec/tb-hash.h
>> +++ b/include/exec/tb-hash.h
>> @@ -20,6 +20,8 @@
>>  #ifndef EXEC_TB_HASH_H
>>  #define EXEC_TB_HASH_H
>>
>> +#include "exec/cpu-defs.h"
>> +#include "exec/exec-all.h"
>>  #include "qemu/xxhash.h"
>>
>>  #ifdef CONFIG_SOFTMMU
>> diff --git a/include/exec/user/thunk.h b/include/exec/user/thunk.h
>> index 8d3af5a3be..d05a8a4dab 100644
>> --- a/include/exec/user/thunk.h
>> +++ b/include/exec/user/thunk.h
>> @@ -20,6 +20,7 @@
>>  #define THUNK_H
>>
>>  #include "cpu.h"
>> +#include "exec/user/abitypes.h"
>>
>>  /* types enums definitions */
>
> These all seem OK.
>
>>
>> diff --git a/include/fpu/softfloat-macros.h

[Qemu-devel] [Bug 1811533] Re: Unstable Win10 guest with qemu 3.1 + huge pages + hv_stimer

2019-08-07 Thread Damir

Still broken with Qemu 4.1rc2 /w Kernel 5.2.

This is a huge problem, as it breaks performance, either in networking
(you can't use the virtio net which is the only 100G adapter afaik), or
you have to disable huge pages, which is a blow to any large vm host, or
it breaks stimer, which increases cpu usage, generally breaking
virtualization.

Thank you!

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

Title:
  Unstable Win10 guest with qemu 3.1 + huge pages + hv_stimer

Status in QEMU:
  New

Bug description:
  Host:
  Gentoo linux x86_64, kernel 4.20.1
  Qemu 3.1.0 
  CPU: Intel i7 6850K
  Chipset: X99

  Guest:
  Windows 10 Pro 64bit (1809)
  Machine type: pc-q35_3.1
  Hyper-V enlightenments: 
hv_stimer,hv_reenlightenment,hv_frequencies,hv_vapic,hv_reset,hv_synic,hv_runtime,hv_vpindex,hv_time,hv_relaxed,hv_spinlocks=0x1fff
  Memory: 16GB backed by 2MB huge pages

  Issue:
  Once guest is started, log gets flooded with:

  qemu-system-x86_64: vhost_region_add_section: Overlapping but not
  coherent sections at 103000

  or

  qemu-system-x86_64: vhost_region_add_section:Section rounded to 0
  prior to previous 1f000

  (line endings change)

  and as time goes guest loses network access (virtio-net-pci) and
  general performance diminishes to extent of freezing applications.

  Observations:
  1) problem disappears when hv_stimer is removed
  2) problem disappears when memory backing with huge pages is disabled
  3) problem disappears when machine type is downgraded to pc-q35_3.0

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

Re: [Qemu-devel] [PATCH v2 0/3] migration: add speed limit for multifd migration

* Ivan Ren (reny...@gmail.com) wrote:
> From: Ivan Ren 
> 
> Currently multifd migration has not been limited and it will consume
> the whole bandwidth of Nic. These two patches add speed limitation to
> it.

Queued

> 
> This is the v3 patches:
> 
> v3 VS v2:
> Add Reviewed info and Suggested info.
> 
> v2 VS v1:
> 1. change qemu_file_update_rate_transfer interface name
>to qemu_file_update_transfer
> 2. add a new patch to update ram_counters for multifd sync packet
> 
> Ivan Ren (3):
>   migration: add qemu_file_update_transfer interface
>   migration: add speed limit for multifd migration
>   migration: update ram_counters for multifd sync packet
> 
>  migration/qemu-file.c |  5 +
>  migration/qemu-file.h |  1 +
>  migration/ram.c   | 24 ++--
>  3 files changed, 20 insertions(+), 10 deletions(-)
> 
> -- 
> 2.17.2 (Apple Git-113)
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3] migration: always initial ram_counters for a new migration

* Ivan Ren (reny...@gmail.com) wrote:
> From: Ivan Ren 
> 
> This patch fix a multifd migration bug in migration speed calculation, this
> problem can be reproduced as follows:
> 1. start a vm and give a heavy memory write stress to prevent the vm be
>successfully migrated to destination
> 2. begin a migration with multifd
> 3. migrate for a long time [actually, this can be measured by transferred 
> bytes]
> 4. migrate cancel
> 5. begin a new migration with multifd, the migration will directly run into
>migration_completion phase
> 
> Reason as follows:
> 
> Migration update bandwidth and s->threshold_size in function
> migration_update_counters after BUFFER_DELAY time:
> 
> current_bytes = migration_total_bytes(s);
> transferred = current_bytes - s->iteration_initial_bytes;
> time_spent = current_time - s->iteration_start_time;
> bandwidth = (double)transferred / time_spent;
> s->threshold_size = bandwidth * s->parameters.downtime_limit;
> 
> In multifd migration, migration_total_bytes function return
> qemu_ftell(s->to_dst_file) + ram_counters.multifd_bytes.
> s->iteration_initial_bytes will be initialized to 0 at every new migration,
> but ram_counters is a global variable, and history migration data will be
> accumulated. So if the ram_counters.multifd_bytes is big enough, it may lead
> pending_size >= s->threshold_size become false in migration_iteration_run
> after the first migration_update_counters.
> 
> Signed-off-by: Ivan Ren 
> Reviewed-by: Juan Quintela 
> Suggested-by: Wei Yang 

Thank you,

Queued

> ---
> v2->v3:
> - fix the bug of update_iteration_initial_status function prototype
> 
> v1->v2:
> - Add interface update_iteration_initial_status to update statistic fields
>   at the same time to avoid info mismatch lead wrong calculation result.
> 
>  migration/migration.c | 25 +++--
>  migration/savevm.c|  1 +
>  2 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 8a607fe1e2..bea9b1d796 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1908,6 +1908,11 @@ static bool migrate_prepare(MigrationState *s, bool 
> blk, bool blk_inc,
>  }
>  
>  migrate_init(s);
> +/*
> + * set ram_counters memory to zero for a
> + * new migration
> + */
> +memset(_counters, 0, sizeof(ram_counters));
>  
>  return true;
>  }
> @@ -3025,6 +3030,17 @@ static void 
> migration_calculate_complete(MigrationState *s)
>  }
>  }
>  
> +static void update_iteration_initial_status(MigrationState *s)
> +{
> +/*
> + * Update these three fields at the same time to avoid mismatch info lead
> + * wrong speed calculation.
> + */
> +s->iteration_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +s->iteration_initial_bytes = migration_total_bytes(s);
> +s->iteration_initial_pages = ram_get_total_transferred_pages();
> +}
> +
>  static void migration_update_counters(MigrationState *s,
>int64_t current_time)
>  {
> @@ -3060,9 +3076,7 @@ static void migration_update_counters(MigrationState *s,
>  
>  qemu_file_reset_rate_limit(s->to_dst_file);
>  
> -s->iteration_start_time = current_time;
> -s->iteration_initial_bytes = current_bytes;
> -s->iteration_initial_pages = ram_get_total_transferred_pages();
> +update_iteration_initial_status(s);
>  
>  trace_migrate_transferred(transferred, time_spent,
>bandwidth, s->threshold_size);
> @@ -3186,7 +3200,7 @@ static void *migration_thread(void *opaque)
>  rcu_register_thread();
>  
>  object_ref(OBJECT(s));
> -s->iteration_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +update_iteration_initial_status(s);
>  
>  qemu_savevm_state_header(s->to_dst_file);
>  
> @@ -3251,8 +3265,7 @@ static void *migration_thread(void *opaque)
>   * the local variables. This is important to avoid
>   * breaking transferred_bytes and bandwidth calculation
>   */
> -s->iteration_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> -s->iteration_initial_bytes = 0;
> +update_iteration_initial_status(s);
>  }
>  
>  current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 79ed44d475..480c511b19 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1424,6 +1424,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  }
>  
>  migrate_init(ms);
> +memset(_counters, 0, sizeof(ram_counters));
>  ms->to_dst_file = f;
>  
>  qemu_mutex_unlock_iothread();
> -- 
> 2.17.2 (Apple Git-113)
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: remove unused field bytes_xfer

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> MigrationState->bytes_xfer is only set to 0 in migrate_init().
> 
> Remove this unnecessary field.
> 
> Signed-off-by: Wei Yang 

Queued (finally!)

> ---
>  migration/migration.c | 1 -
>  migration/migration.h | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index dea7078bf4..c929cf8d0f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1681,7 +1681,6 @@ void migrate_init(MigrationState *s)
>   * parameters/capabilities that the user set, and
>   * locks.
>   */
> -s->bytes_xfer = 0;
>  s->cleanup_bh = 0;
>  s->to_dst_file = NULL;
>  s->rp_state.from_dst_file = NULL;
> diff --git a/migration/migration.h b/migration/migration.h
> index 852eb3c4e9..b9efbe9168 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -116,7 +116,6 @@ struct MigrationState
>  DeviceState parent_obj;
>  
>  /*< public >*/
> -size_t bytes_xfer;
>  QemuThread thread;
>  QEMUBH *cleanup_bh;
>  QEMUFile *to_dst_file;
> -- 
> 2.19.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 8/8] block/backup: backup_do_cow: use bdrv_dirty_bitmap_next_dirty_area

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> Use effective bdrv_dirty_bitmap_next_dirty_area interface.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 56 ++
>  1 file changed, 24 insertions(+), 32 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index f19c9195fe..5ede0c8290 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -235,25 +235,28 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>  {
>  CowRequest cow_request;
>  int ret = 0;
> -int64_t start, end; /* bytes */
> +uint64_t off, cur_bytes;
> +int64_t aligned_offset, aligned_bytes, aligned_end;
>  BdrvRequestFlags read_flags =
>  is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
>  
>  qemu_co_rwlock_rdlock(>flush_rwlock);
>  
> -start = QEMU_ALIGN_DOWN(offset, job->cluster_size);
> -end = QEMU_ALIGN_UP(bytes + offset, job->cluster_size);
> +aligned_offset = QEMU_ALIGN_DOWN(offset, job->cluster_size);
> +aligned_end = QEMU_ALIGN_UP(bytes + offset, job->cluster_size);
> +aligned_bytes = aligned_end - aligned_offset;
>  
> -trace_backup_do_cow_enter(job, start, offset, bytes);
> +trace_backup_do_cow_enter(job, aligned_offset, offset, bytes);
>  
> -wait_for_overlapping_requests(job, start, end);
> -cow_request_begin(_request, job, start, end);
> +wait_for_overlapping_requests(job, aligned_offset, aligned_end);
> +cow_request_begin(_request, job, aligned_offset, aligned_end);
>  
>  if (job->initializing_bitmap) {
> -int64_t off, chunk;
> +int64_t chunk;
>  
> -for (off = offset; offset < end; offset += chunk) {
> -ret = backup_bitmap_reset_unallocated(job, off, end - off, 
> );
> +for (off = aligned_offset; off < aligned_end; off += chunk) {
> +ret = backup_bitmap_reset_unallocated(job, off, aligned_end - 
> off,
> +  );
>  if (ret < 0) {
>  chunk = job->cluster_size;
>  }
> @@ -261,47 +264,36 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>  }
>  ret = 0;
>  
> -while (start < end) {
> -int64_t dirty_end;
> -int64_t cur_bytes;
> -
> -if (!bdrv_dirty_bitmap_get(job->copy_bitmap, start)) {
> -trace_backup_do_cow_skip(job, start);
> -start += job->cluster_size;
> -continue; /* already copied */
> -}
> -
> -dirty_end = bdrv_dirty_bitmap_next_zero(job->copy_bitmap, start,
> -end - start);
> -if (dirty_end < 0) {
> -dirty_end = end;
> -}
> -
> -trace_backup_do_cow_process(job, start);
> -cur_bytes = MIN(dirty_end - start, job->len - start);
> -bdrv_reset_dirty_bitmap(job->copy_bitmap, start, dirty_end - start);
> +off = aligned_offset;
> +cur_bytes = aligned_bytes;
> +while (bdrv_dirty_bitmap_next_dirty_area(job->copy_bitmap,
> + , _bytes))
> +{
> +trace_backup_do_cow_process(job, off);
> +bdrv_reset_dirty_bitmap(job->copy_bitmap, off, cur_bytes);
>  
>  if (job->use_copy_range) {
> -ret = backup_cow_with_offload(job, start, cur_bytes, read_flags);
> +ret = backup_cow_with_offload(job, off, cur_bytes, read_flags);
>  if (ret < 0) {
>  job->use_copy_range = false;
>  }
>  }
>  if (!job->use_copy_range) {
> -ret = backup_cow_with_bounce_buffer(job, start, cur_bytes,
> +ret = backup_cow_with_bounce_buffer(job, off, cur_bytes,
>  read_flags, error_is_read);
>  }
>  if (ret < 0) {
> -bdrv_set_dirty_bitmap(job->copy_bitmap, start, dirty_end - 
> start);
> +bdrv_set_dirty_bitmap(job->copy_bitmap, off, cur_bytes);
>  break;
>  }
>  
>  /* Publish progress, guest I/O counts as progress too.  Note that the
>   * offset field is an opaque progress value, it is not a disk offset.
>   */
> -start += cur_bytes;
> +off += cur_bytes;
>  job->bytes_read += cur_bytes;
>  job_progress_update(>common.job, cur_bytes);
> +cur_bytes = offset + bytes - off;

Hm, why not aligned_end - off?

(You could also drop aligned_bytes altogether and always set cur_bytes
to aligned_end - off.)

Max

>  }
>  
>  cow_request_end(_request);
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3] hmp: Remove migration capabilities from "info migrate"

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> With the growth of migration capabilities, it is not proper to display
> them in "info migrate". Users are recommended to use "info
> migrate_capabiltiies" to list them.
> 
> Signed-off-by: Wei Yang 
> Suggested-by: Dr. David Alan Gilbert 

Queued

> ---
> v3:
>   * remove un-used variable caps
> v2:
>   * remove capabilities from "info migrate"
> ---
>  monitor/hmp-cmds.c | 14 --
>  1 file changed, 14 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 5ca3ebe942..35788c0645 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -220,24 +220,11 @@ static char *SocketAddress_to_str(SocketAddress *addr)
>  void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>  {
>  MigrationInfo *info;
> -MigrationCapabilityStatusList *caps, *cap;
>  
>  info = qmp_query_migrate(NULL);
> -caps = qmp_query_migrate_capabilities(NULL);
>  
>  migration_global_dump(mon);
>  
> -/* do not display parameters during setup */
> -if (info->has_status && caps) {
> -monitor_printf(mon, "capabilities: ");
> -for (cap = caps; cap; cap = cap->next) {
> -monitor_printf(mon, "%s: %s ",
> -   MigrationCapability_str(cap->value->capability),
> -   cap->value->state ? "on" : "off");
> -}
> -monitor_printf(mon, "\n");
> -}
> -
>  if (info->has_status) {
>  monitor_printf(mon, "Migration status: %s",
> MigrationStatus_str(info->status));
> @@ -370,7 +357,6 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>  monitor_printf(mon, "]\n");
>  }
>  qapi_free_MigrationInfo(info);
> -qapi_free_MigrationCapabilityStatusList(caps);
>  }
>  
>  void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict)
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/2] migration/postcopy: simplify postcopy_chunk_hostpages_pass

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> When looking into function postcopy_chunk_hostpages_pass(), we could use
> alignment calculation to simplify it.
> 
> Wei Yang (2):
>   migration/postcopy: simplify calculation of run_start and
> fixup_start_addr
>   migration/postcopy: use QEMU_IS_ALIGNED to replace host_offset
> 
>  migration/ram.c | 37 +++--
>  1 file changed, 7 insertions(+), 30 deletions(-)

Queued

> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] RISC-V: insn32.decode: Confusing encodings

On 8/6/19 5:48 AM, Maxim Blinov wrote:
> slli 00 ... 001 . 0010011 @sh
> srli 00 ... 101 . 0010011 @sh
> srai 01 ... 101 . 0010011 @sh
> 
> First question: Why does the %sh10 field exist? There are no 10-bit
> shamt fields anywhere in the spec.
> 
> Second question: For rv32i, "SLLI" is defined as follows in the spec:
> 
> 000 shamt[4:0] rs1[4:0] 001 rd[4:0] 0010011  |  SLLI

Bits [9:5] of the field are checked against zero later, with

if (a->shamt >= TARGET_LONG_BITS) {
return false;
}

It was done this way to be compatible between rv32, rv64, and a future rv128.
Which I admit would only need 7 bits not 10, but it didn't seem to matter
either way.

> Consider the case that we have a 32 bit cpu and we wanted to have a
> custom instruction encoded like so:
> 
>   |This bit set
>   v
> 001 shamt[4:0] rs1[4:0] 001 rd[4:0] 0010011  |  MY_INSN
> 
> In 64 bit risc-v, we can't have that instruction because that bit is
> used in the shift field for the SLLI instruction.  But it should be
> fine to use in 32-bit risc-v.

Ah, well, for that you would in fact need to adjust the decode files.

I do question why you'd want to define MY_INSN in such a way as to be
incompatible with an rv64 implementation.  Why not place your new bit higher in
the field?

> Why not have two separate insn32.decode and insn64.decode files?

To avoid unnecessary duplication, of course.

r~

Re: [Qemu-devel] [Patch v2] migration/postcopy: make PostcopyDiscardState a static variable

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> In postcopy-ram.c, we provide three functions to discard certain
> RAMBlock range:
> 
>   * postcopy_discard_send_init()
>   * postcopy_discard_send_range()
>   * postcopy_discard_send_finish()
> 
> Currently, we allocate/deallocate PostcopyDiscardState for each RAMBlock
> on sending discard information to destination. This is not necessary and
> the same data area could be reused for each RAMBlock.
> 
> This patch defines PostcopyDiscardState a static variable. By doing so:
> 
>   1) avoid memory allocation and deallocation to the system
>   2) avoid potential failure of memory allocation
>   3) hide some details for their users
> 
> Signed-off-by: Wei Yang 

Queued

> 
> ---
> v2:
>   * make it a static variable, suggested by Dave
> ---
>  migration/postcopy-ram.c | 70 +---
>  migration/postcopy-ram.h | 13 +++-
>  migration/ram.c  | 30 +++--
>  3 files changed, 46 insertions(+), 67 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 7b3e198538..cf2400b47e 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -1375,22 +1375,16 @@ void 
> postcopy_fault_thread_notify(MigrationIncomingState *mis)
>   *   asking to discard individual ranges.
>   *
>   * @ms: The current migration state.
> - * @offset: the bitmap offset of the named RAMBlock in the migration
> - *   bitmap.
> + * @offset: the bitmap offset of the named RAMBlock in the migration bitmap.
>   * @name: RAMBlock that discards will operate on.
> - *
> - * returns: a new PDS.
>   */
> -PostcopyDiscardState *postcopy_discard_send_init(MigrationState *ms,
> - const char *name)
> +static PostcopyDiscardState pds = {0};
> +void postcopy_discard_send_init(MigrationState *ms, const char *name)
>  {
> -PostcopyDiscardState *res = g_malloc0(sizeof(PostcopyDiscardState));
> -
> -if (res) {
> -res->ramblock_name = name;
> -}
> -
> -return res;
> +pds.ramblock_name = name;
> +pds.cur_entry = 0;
> +pds.nsentwords = 0;
> +pds.nsentcmds = 0;
>  }
>  
>  /**
> @@ -1399,30 +1393,29 @@ PostcopyDiscardState 
> *postcopy_discard_send_init(MigrationState *ms,
>   *   be sent later.
>   *
>   * @ms: Current migration state.
> - * @pds: Structure initialised by postcopy_discard_send_init().
>   * @start,@length: a range of pages in the migration bitmap in the
>   *   RAM block passed to postcopy_discard_send_init() (length=1 is one page)
>   */
> -void postcopy_discard_send_range(MigrationState *ms, PostcopyDiscardState 
> *pds,
> -unsigned long start, unsigned long length)
> +void postcopy_discard_send_range(MigrationState *ms, unsigned long start,
> + unsigned long length)
>  {
>  size_t tp_size = qemu_target_page_size();
>  /* Convert to byte offsets within the RAM block */
> -pds->start_list[pds->cur_entry] = start  * tp_size;
> -pds->length_list[pds->cur_entry] = length * tp_size;
> -trace_postcopy_discard_send_range(pds->ramblock_name, start, length);
> -pds->cur_entry++;
> -pds->nsentwords++;
> +pds.start_list[pds.cur_entry] = start  * tp_size;
> +pds.length_list[pds.cur_entry] = length * tp_size;
> +trace_postcopy_discard_send_range(pds.ramblock_name, start, length);
> +pds.cur_entry++;
> +pds.nsentwords++;
>  
> -if (pds->cur_entry == MAX_DISCARDS_PER_COMMAND) {
> +if (pds.cur_entry == MAX_DISCARDS_PER_COMMAND) {
>  /* Full set, ship it! */
>  qemu_savevm_send_postcopy_ram_discard(ms->to_dst_file,
> -  pds->ramblock_name,
> -  pds->cur_entry,
> -  pds->start_list,
> -  pds->length_list);
> -pds->nsentcmds++;
> -pds->cur_entry = 0;
> +  pds.ramblock_name,
> +  pds.cur_entry,
> +  pds.start_list,
> +  pds.length_list);
> +pds.nsentcmds++;
> +pds.cur_entry = 0;
>  }
>  }
>  
> @@ -1431,24 +1424,21 @@ void postcopy_discard_send_range(MigrationState *ms, 
> PostcopyDiscardState *pds,
>   * bitmap code. Sends any outstanding discard messages, frees the PDS
>   *
>   * @ms: Current migration state.
> - * @pds: Structure initialised by postcopy_discard_send_init().
>   */
> -void postcopy_discard_send_finish(MigrationState *ms, PostcopyDiscardState 
> *pds)
> +void postcopy_discard_send_finish(MigrationState *ms)
>  {
>  /* Anything unsent? */
> -if (pds->cur_entry) {
> +if (pds.cur_entry) {
>  qemu_savevm_send_postcopy_ram_discard(ms->to_dst_file,
> -

Re: [Qemu-devel] [PATCH 7/8] block/backup: merge duplicated logic into backup_do_cow

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> backup_cow_with_offload and backup_cow_with_bounce_buffer contains a
> lot of duplicated logic. Move it into backup_do_cow.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 83 +++---
>  1 file changed, 31 insertions(+), 52 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3] migration/postcopy: use mis->bh instead of allocating a QEMUBH

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> For migration incoming side, it either quit in precopy or postcopy. It
> is safe to use the mis->bh for both instead of allocating a dedicated
> QEMUBH for postcopy.
> 
> Signed-off-by: Wei Yang 
> Reviewed-by: Dr. David Alan Gilbert 

Hi Wei,
  Can you check this, the patchew tests came back with a failure which
seems bh related; I've not tried it, but can you just see if you can
reproduce it?

Dave

> ---
> v3: rebase on latest upstream
> v2: fix a typo in change log
> ---
>  migration/savevm.c | 17 -
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 621b6c0465..9bf9d2e5fe 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1867,16 +1867,10 @@ static int 
> loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
>  return 0;
>  }
>  
> -
> -typedef struct {
> -QEMUBH *bh;
> -} HandleRunBhData;
> -
>  static void loadvm_postcopy_handle_run_bh(void *opaque)
>  {
>  Error *local_err = NULL;
> -HandleRunBhData *data = opaque;
> -MigrationIncomingState *mis = migration_incoming_get_current();
> +MigrationIncomingState *mis = opaque;
>  
>  /* TODO we should move all of this lot into postcopy_ram.c or a shared 
> code
>   * in migration.c
> @@ -1908,15 +1902,13 @@ static void loadvm_postcopy_handle_run_bh(void 
> *opaque)
>  runstate_set(RUN_STATE_PAUSED);
>  }
>  
> -qemu_bh_delete(data->bh);
> -g_free(data);
> +qemu_bh_delete(mis->bh);
>  }
>  
>  /* After all discards we can start running and asking for pages */
>  static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
>  {
>  PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
> -HandleRunBhData *data;
>  
>  trace_loadvm_postcopy_handle_run();
>  if (ps != POSTCOPY_INCOMING_LISTENING) {
> @@ -1924,9 +1916,8 @@ static int 
> loadvm_postcopy_handle_run(MigrationIncomingState *mis)
>  return -1;
>  }
>  
> -data = g_new(HandleRunBhData, 1);
> -data->bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, data);
> -qemu_bh_schedule(data->bh);
> +mis->bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, mis);
> +qemu_bh_schedule(mis->bh);
>  
>  /* We need to finish reading the stream from the package
>   * and also stop reading anything more from the stream that loaded the
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 6/8] block/backup: teach backup_cow_with_bounce_buffer to copy more at once

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> backup_cow_with_offload can transfer more than on cluster. Let
> backup_cow_with_bounce_buffer behave similarly. It reduces number
> of IO and there are no needs to copy cluster by cluster.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 30 +++---
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index eb41e4af4f..c765c073ad 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -104,22 +104,24 @@ static int coroutine_fn 
> backup_cow_with_bounce_buffer(BackupBlockJob *job,
>int64_t start,
>int64_t end,
>bool is_write_notifier,
> -  bool *error_is_read,
> -  void **bounce_buffer)
> +  bool *error_is_read)
>  {
>  int ret;
>  BlockBackend *blk = job->common.blk;
>  int nbytes;
>  int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
> +void *bounce_buffer = blk_try_blockalign(blk, end);

s/end/end - start/ (or probably rather s/end/nbytes/ after that has been
set).

Rest looks good.

Max

>  
> -assert(QEMU_IS_ALIGNED(start, job->cluster_size));
> -bdrv_reset_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
> -nbytes = MIN(job->cluster_size, job->len - start);
> -if (!*bounce_buffer) {
> -*bounce_buffer = blk_blockalign(blk, job->cluster_size);
> +if (!bounce_buffer) {
> +return -ENOMEM;
>  }



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc

On 8/7/19 11:16 AM, Peter Maydell wrote:
> How about we add this to the commit message?
> 
> This changes the behaviour for load_reg() and load_reg_var()
> when called with reg==15 from a 32-bit Thumb instruction:
> previously they would have returned the incorrect value
> of pc_curr + 6, and now they will return the architecturally
> correct value of PC, which is pc_curr + 4. This will not
> affect well-behaved guest software, because all of the places
> we call these functions from T32 code are instructions where
> using r15 is UNPREDICTABLE. Using the architectural PC value
> here is more consistent with the T16 and A32 behaviour.

Looks good to me.


r~

Re: [Qemu-devel] [PATCH v6 20/26] memory: Access MemoryRegion with endianness

On 8/7/19 11:00 AM, Paolo Bonzini wrote:
> On 07/08/19 19:49, Richard Henderson wrote:
>> On 8/7/19 1:33 AM, tony.ngu...@bt.com wrote:
>>> @@ -551,6 +551,7 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
>>> hwaddr addr,
>>>  /* As length is under guest control, handle illegal values. */
>>>  return;
>>>  }
>>> +/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>>>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>>>   MEMTXATTRS_UNSPECIFIED);
>>>  }
>>
>> Here is an example of where Paolo is quite right -- you cannot simply add 
>> MO_TE
>> via size_memop().  In patch 22 we see
>>
>>> @@ -542,16 +542,15 @@ void virtio_address_space_write(VirtIOPCIProxy 
>>> *proxy, hwaddr addr,
>>>  val = pci_get_byte(buf);
>>>  break;
>>>  case 2:
>>> -val = cpu_to_le16(pci_get_word(buf));
>>> +val = pci_get_word(buf);
>>>  break;
>>>  case 4:
>>> -val = cpu_to_le32(pci_get_long(buf));
>>> +val = pci_get_long(buf);
>>>  break;
>>>  default:
>>>  /* As length is under guest control, handle illegal values. */
>>>  return;
>>>  }
>>> -/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>>>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>>>   MEMTXATTRS_UNSPECIFIED);
>>
>> This is a little-endian store -- MO_LE not MO_TE.
> 
> Or leave the switch statement aside and request host endianness.  Either
> is fine.

The goal is to minimize the number of places and the number of times that
bswaps happen.  That's why I think pushing the cpu_to_le16 into
memory_region_dispatch_write is a good thing.

However, leaving a host-endian might be the easiest short-term solution for the
more complicated cases.  It also seems like a way to split the patch further if
we think that's desirable.


r~

Re: [Qemu-devel] [PATCH v2 22/29] Include hw/boards.h a bit less

2019-08-07 Thread Eduardo Habkost

On Wed, Aug 07, 2019 at 08:05:50PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/7/19 7:57 PM, Eduardo Habkost wrote:
> > On Wed, Aug 07, 2019 at 07:26:56PM +0200, Philippe Mathieu-Daudé wrote:
> >> On 8/6/19 5:14 PM, Markus Armbruster wrote:
> >>> hw/boards.h pulls in almost 60 headers.  The less we include it into
> >>> headers, the better.  As a first step, drop superfluous inclusions,
> >>> and downgrade some more to what's actually needed.  Gets rid of just
> >>> one inclusion into a header.
> >>>
> >>> Cc: Eduardo Habkost 
> >>> Cc: Marcel Apfelbaum 
> >>> Signed-off-by: Markus Armbruster 
> >>> ---
> > [...]
> >>> diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
> >>> index bcacdd1d8f..34a9f6f7a9 100644
> >>> --- a/target/i386/hax-all.c
> >>> +++ b/target/i386/hax-all.c
> >>> @@ -33,7 +33,6 @@
> >>>  #include "sysemu/reset.h"
> >>>  #include "sysemu/sysemu.h"
> >>>  #include "qemu/main-loop.h"
> >>> -#include "hw/boards.h"
> >>>  
> >>>  #define DEBUG_HAX 0
> >>
> >> include/sysemu/hax.h misses to include "hw/boards.h":
> > 
> > I don't understand.  I don't see any reason for the sysemu/hax.h
> > header to include hw/boards.h.
> 
> Ah, you are right, the AccelClass is defined in include/sysemu/accel.h:
> 
> typedef struct AccelClass {
> /*< private >*/
> ObjectClass parent_class;
> /*< public >*/
> 
> const char *name;
> int (*init_machine)(MachineState *ms);
> ...
> 
> So this is where "hw/boards.h" has to be included (it is where
> MachineState is defined).

As far as I can see, accel.h doesn't need the full MachineState
struct definition and doesn't need boards.h.  It just needs the
MachineState typedef, which is defined at typedefs.h.

hax-all.c, on the other hand, needs the full MachineState struct
definition and is expected to include hw/boards.h.

> 
> >>
> >> target/i386/hax-all.c: In function 'hax_accel_init':
> >> target/i386/hax-all.c:354:26: error: dereferencing pointer to incomplete
> >> type 'MachineState {aka struct MachineState}'
> >>  int ret = hax_init(ms->ram_size);
> >>   ^
> > 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc

2019-08-07 Thread Peter Maydell

On Wed, 7 Aug 2019 at 19:04, Richard Henderson
 wrote:
>
> On 8/7/19 10:27 AM, Peter Maydell wrote:
> >> +/* The architectural value of PC.  */
> >> +static uint32_t read_pc(DisasContext *s)
> >> +{
> >> +return s->pc_curr + (s->thumb ? 4 : 8);
> >> +}
> >> +
> >>  /* Set a variable to the value of a CPU register.  */
> >>  static void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
> >>  {
> >>  if (reg == 15) {
> >> -uint32_t addr;
> >> -/* normally, since we updated PC, we need only to add one insn */
> >> -if (s->thumb)
> >> -addr = (long)s->pc + 2;
> >> -else
> >> -addr = (long)s->pc + 4;
> >> -tcg_gen_movi_i32(var, addr);
> >> +tcg_gen_movi_i32(var, read_pc(s));
> >
> > So previously:
> >  * for A32 we would return s->pc + 4, which is the same as s->pc_curr + 8
> >  * for T16 we would return s->pc + 2, which is the same as s->pc_curr + 4
> >  * for T32 we would return s->pc + 2 -- but that's not the same as
> >s->pc_curr + 4, it's s->pc_curr + 6...
> >
> > Since s->pc_curr + 4 is the right architectural answer, are we
> > fixing a bug here? Or are all the places where T32 code calls
> > this function UNPREDICTABLE for the reg == 15 case ?
>
> I believe that this is UNPREDICTABLE.
>
> The T32 cases that reference the PC that are not UNPREDICTABLE, literal memory
> references and adr, are all of the form (s->pc & ~3) and do not come through
> load_reg_var().  Those will be unified by add_reg_for_lit() in the next patch.

Yeah, that was my assumption -- at some previous point rather
than making load_reg/load_reg_var do the right thing for 32-bit
Thumb insns we just fixed up all the callsites to specialcase 15...

How about we add this to the commit message?

This changes the behaviour for load_reg() and load_reg_var()
when called with reg==15 from a 32-bit Thumb instruction:
previously they would have returned the incorrect value
of pc_curr + 6, and now they will return the architecturally
correct value of PC, which is pc_curr + 4. This will not
affect well-behaved guest software, because all of the places
we call these functions from T32 code are instructions where
using r15 is UNPREDICTABLE. Using the architectural PC value
here is more consistent with the T16 and A32 behaviour.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/8] block/backup: fix backup_cow_with_offload for last cluster

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> We shouldn't try to copy bytes beyond EOF. Fix it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 22/29] Include hw/boards.h a bit less

On 8/7/19 7:57 PM, Eduardo Habkost wrote:
> On Wed, Aug 07, 2019 at 07:26:56PM +0200, Philippe Mathieu-Daudé wrote:
>> On 8/6/19 5:14 PM, Markus Armbruster wrote:
>>> hw/boards.h pulls in almost 60 headers.  The less we include it into
>>> headers, the better.  As a first step, drop superfluous inclusions,
>>> and downgrade some more to what's actually needed.  Gets rid of just
>>> one inclusion into a header.
>>>
>>> Cc: Eduardo Habkost 
>>> Cc: Marcel Apfelbaum 
>>> Signed-off-by: Markus Armbruster 
>>> ---
> [...]
>>> diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
>>> index bcacdd1d8f..34a9f6f7a9 100644
>>> --- a/target/i386/hax-all.c
>>> +++ b/target/i386/hax-all.c
>>> @@ -33,7 +33,6 @@
>>>  #include "sysemu/reset.h"
>>>  #include "sysemu/sysemu.h"
>>>  #include "qemu/main-loop.h"
>>> -#include "hw/boards.h"
>>>  
>>>  #define DEBUG_HAX 0
>>
>> include/sysemu/hax.h misses to include "hw/boards.h":
> 
> I don't understand.  I don't see any reason for the sysemu/hax.h
> header to include hw/boards.h.

Ah, you are right, the AccelClass is defined in include/sysemu/accel.h:

typedef struct AccelClass {
/*< private >*/
ObjectClass parent_class;
/*< public >*/

const char *name;
int (*init_machine)(MachineState *ms);
...

So this is where "hw/boards.h" has to be included (it is where
MachineState is defined).

>>
>> target/i386/hax-all.c: In function 'hax_accel_init':
>> target/i386/hax-all.c:354:26: error: dereferencing pointer to incomplete
>> type 'MachineState {aka struct MachineState}'
>>  int ret = hax_init(ms->ram_size);
>>   ^
>

Re: [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc

On 8/7/19 10:27 AM, Peter Maydell wrote:
>> +/* The architectural value of PC.  */
>> +static uint32_t read_pc(DisasContext *s)
>> +{
>> +return s->pc_curr + (s->thumb ? 4 : 8);
>> +}
>> +
>>  /* Set a variable to the value of a CPU register.  */
>>  static void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
>>  {
>>  if (reg == 15) {
>> -uint32_t addr;
>> -/* normally, since we updated PC, we need only to add one insn */
>> -if (s->thumb)
>> -addr = (long)s->pc + 2;
>> -else
>> -addr = (long)s->pc + 4;
>> -tcg_gen_movi_i32(var, addr);
>> +tcg_gen_movi_i32(var, read_pc(s));
> 
> So previously:
>  * for A32 we would return s->pc + 4, which is the same as s->pc_curr + 8
>  * for T16 we would return s->pc + 2, which is the same as s->pc_curr + 4
>  * for T32 we would return s->pc + 2 -- but that's not the same as
>s->pc_curr + 4, it's s->pc_curr + 6...
> 
> Since s->pc_curr + 4 is the right architectural answer, are we
> fixing a bug here? Or are all the places where T32 code calls
> this function UNPREDICTABLE for the reg == 15 case ?

I believe that this is UNPREDICTABLE.

The T32 cases that reference the PC that are not UNPREDICTABLE, literal memory
references and adr, are all of the form (s->pc & ~3) and do not come through
load_reg_var().  Those will be unified by add_reg_for_lit() in the next patch.


r~

Re: [Qemu-devel] [PATCH 0/4] virtiofsd: multithreading preparation part 3

2019-08-07 Thread Stefan Hajnoczi

On Thu, Aug 01, 2019 at 05:54:05PM +0100, Stefan Hajnoczi wrote:
> Performance
> ---
> Please try these patches out and share your results.

Here are the performance numbers:

  Threadpool | iodepth | iodepth
 size|1|   64
  ---+-+
  None   |   4451  |  4876
  1  |   4360  |  4858
  64 |   4359  | 33,266

A graph is available here:
https://vmsplice.net/~stefan/virtiofsd-threadpool-performance.png

Summary:

 * iodepth=64 performance is increased by 6.8 times.
 * iodepth=1 performance degrades by 2%.
 * DAX is bottlenecked by QEMU's single-threaded
   VHOST_USER_SLAVE_FS_MAP/UNMAP handler.

Threadpool size "none" is virtiofsd commit 813a824b707 ("virtiofsd: use
fuse_lowlevel_is_virtio() in fuse_session_destroy()") without any of the
multithreading preparation patches.  I benchmarked this to check whether
the patches introduce a regression for iodepth=1.  They do, but it's
only around 2%.

I also ran with DAX but found there was not much difference between
iodepth=1 and iodepth=64.  This might be because the host mmap(2)
syscall becomes the bottleneck and a serialization point.  QEMU only
processes one VHOST_USER_SLAVE_FS_MAP/UNMAP at a time.  If we want to
accelerate DAX it may be necessary to parallelize mmap, assuming the
host kernel can do them in parallel on a single file.  This performance
optimization is future work and not directly related to this patch
series.

The following fio job was run with cache=none and no DAX:

  [global]
  runtime=60
  ramp_time=30
  filename=/var/tmp/fio.dat
  direct=1
  rw=randread
  bs=4k
  size=4G
  ioengine=libaio
  iodepth=1

  [read]

Guest configuration:
1 vCPU
4 GB RAM
Linux 5.1 (vivek-aug-06-2019)

Host configuration:
Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz (2 cores x 2 threads)
8 GB RAM
Linux 5.1.20-300.fc30.x86_64
XFS + dm-thin + dm-crypt
Toshiba THNSFJ256GDNU (256 GB SATA SSD)

Stefan

signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 4/8] block/backup: improve unallocated clusters skipping

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> Limit block_status querying to request bounds on write notifier to
> avoid extra seeking.

I don’t understand this reasoning.  Checking whether something is
allocated for qcow2 should just mean an L2 cache lookup.  Which we have
to do anyway when we try to copy data off the source.

I could understand saying this makes the code easier, but I actually
don’t think it does.  If you implemented checking the allocation status
only for areas where the bitmap is reset (which I think this patch
should), then it’d just duplicate the existing loop.

> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 38 +-
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 11e27c844d..a4d37d2d62 100644
> --- a/block/backup.c
> +++ b/block/backup.c

[...]

> @@ -267,6 +267,18 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>  wait_for_overlapping_requests(job, start, end);
>  cow_request_begin(_request, job, start, end);
>  
> +if (job->initializing_bitmap) {
> +int64_t off, chunk;
> +
> +for (off = offset; offset < end; offset += chunk) {

This is what I’m referring to, I think this loop should skip areas that
are clean.

> +ret = backup_bitmap_reset_unallocated(job, off, end - off, 
> );
> +if (ret < 0) {
> +chunk = job->cluster_size;
> +}
> +}
> +}
> +ret = 0;
> +
>  while (start < end) {
>  int64_t dirty_end;
>  
> @@ -276,15 +288,6 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>  continue; /* already copied */
>  }
>  
> -if (job->initializing_bitmap) {
> -ret = backup_bitmap_reset_unallocated(job, start, _bytes);
> -if (ret == 0) {
> -trace_backup_do_cow_skip_range(job, start, skip_bytes);
> -start += skip_bytes;
> -continue;
> -}
> -}
> -
>  dirty_end = bdrv_dirty_bitmap_next_zero(job->copy_bitmap, start,
>  end - start);
>  if (dirty_end < 0) {

Hm, you resolved that conflict differently from me.

I decided the bdrv_dirty_bitmap_next_zero() call should go before the
backup_bitmap_reset_unallocated() call so that we can then do a

  dirty_end = MIN(dirty_end, start + skip_bytes);

because we probably don’t want to copy anything past what
backup_bitmap_reset_unallocated() has inquired.


But then again this patch eliminates the difference anyway...

Max

> @@ -546,7 +549,8 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
>  goto out;
>  }
>  
> -ret = backup_bitmap_reset_unallocated(s, offset, );
> +ret = backup_bitmap_reset_unallocated(s, offset, s->len - offset,
> +  );
>  if (ret < 0) {
>  goto out;
>  }
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v6 20/26] memory: Access MemoryRegion with endianness

2019-08-07 Thread Paolo Bonzini

On 07/08/19 19:49, Richard Henderson wrote:
> On 8/7/19 1:33 AM, tony.ngu...@bt.com wrote:
>> @@ -551,6 +551,7 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
>> hwaddr addr,
>>  /* As length is under guest control, handle illegal values. */
>>  return;
>>  }
>> +/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>>   MEMTXATTRS_UNSPECIFIED);
>>  }
> 
> Here is an example of where Paolo is quite right -- you cannot simply add 
> MO_TE
> via size_memop().  In patch 22 we see
> 
>> @@ -542,16 +542,15 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
>> hwaddr addr,
>>  val = pci_get_byte(buf);
>>  break;
>>  case 2:
>> -val = cpu_to_le16(pci_get_word(buf));
>> +val = pci_get_word(buf);
>>  break;
>>  case 4:
>> -val = cpu_to_le32(pci_get_long(buf));
>> +val = pci_get_long(buf);
>>  break;
>>  default:
>>  /* As length is under guest control, handle illegal values. */
>>  return;
>>  }
>> -/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>>   MEMTXATTRS_UNSPECIFIED);
> 
> This is a little-endian store -- MO_LE not MO_TE.

Or leave the switch statement aside and request host endianness.  Either
is fine.

Paolo

Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings

2019-08-07 Thread Alistair Francis

On Wed, Aug 7, 2019 at 8:00 AM Palmer Dabbelt  wrote:
>
> The ISA strings we're providing from QEMU aren't actually legal RISC-V
> ISA strings, as both the S and U extensions cannot exist as
> single-letter extensions and must instead be multi-letter strings.
> We're still using the ISA strings inside QEMU to track the availiable

s/availiable/available/g

> extensions, so this patch just strips out the S and U extensions when
> formatting ISA strings.

Atish and I were talking about this and we concluded that S and U
aren't extensions, but should be reported in the misa CSR.

>
> This boots Linux on top of 4.1-rc3, which no longer has the U extension
> in /proc/cpuinfo.
>
> Signed-off-by: Palmer Dabbelt 
> ---
> This is another late one, but I'd like to target it for 4.1 as we're
> providing illegal ISA strings and I don't want to bake that into a bunch
> of other code.
> ---
>  target/riscv/cpu.c | 17 -
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f8d07bd20ad7..4df14433d789 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -501,7 +501,22 @@ char *riscv_isa_string(RISCVCPU *cpu)
>  char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
>  for (i = 0; i < sizeof(riscv_exts); i++) {
>  if (cpu->env.misa & RV(riscv_exts[i])) {
> -*p++ = qemu_tolower(riscv_exts[i]);
> +char lower = qemu_tolower(riscv_exts[i]);
> +switch (lower) {
> +case 's':
> +case 'u':
> +/*
> + * The 's' and 'u' extensions shouldn't be passed in the 
> device
> + * tree, but we still use them internally to track extension
> + * sets.  Here we just explicitly remove them when formatting
> + * an ISA string.

This should be updated to note mention 's' and 'u' as extensions, but
clarify that they are correctly include in the misa CSR.

Alistair

> + */
> +break;
> +
> +default:
> +*p++ = qemu_tolower(riscv_exts[i]);
> +break;
> +}
>  }
>  }
>  *p = '\0';
> --
> 2.21.0
>
>

Re: [Qemu-devel] [PATCH v2 22/29] Include hw/boards.h a bit less

2019-08-07 Thread Eduardo Habkost

On Wed, Aug 07, 2019 at 07:26:56PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/6/19 5:14 PM, Markus Armbruster wrote:
> > hw/boards.h pulls in almost 60 headers.  The less we include it into
> > headers, the better.  As a first step, drop superfluous inclusions,
> > and downgrade some more to what's actually needed.  Gets rid of just
> > one inclusion into a header.
> > 
> > Cc: Eduardo Habkost 
> > Cc: Marcel Apfelbaum 
> > Signed-off-by: Markus Armbruster 
> > ---
[...]
> > diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
> > index bcacdd1d8f..34a9f6f7a9 100644
> > --- a/target/i386/hax-all.c
> > +++ b/target/i386/hax-all.c
> > @@ -33,7 +33,6 @@
> >  #include "sysemu/reset.h"
> >  #include "sysemu/sysemu.h"
> >  #include "qemu/main-loop.h"
> > -#include "hw/boards.h"
> >  
> >  #define DEBUG_HAX 0
> 
> include/sysemu/hax.h misses to include "hw/boards.h":

I don't understand.  I don't see any reason for the sysemu/hax.h
header to include hw/boards.h.

> 
> target/i386/hax-all.c: In function 'hax_accel_init':
> target/i386/hax-all.c:354:26: error: dereferencing pointer to incomplete
> type 'MachineState {aka struct MachineState}'
>  int ret = hax_init(ms->ram_size);
>   ^

-- 
Eduardo

[Qemu-devel] [Bug 1839367] [NEW] Wrong interrupts generated for I.MX6 FEC controller

2019-08-07 Thread Aaron Hill

Public bug reported:

The imx_eth_update function in hw/net/imx_fec.c has the following
comment
(https://github.com/qemu/qemu/blob/864ab314f1d924129d06ac7b571f105a2b76a4b2/hw/net/imx_fec.c#L421-L445):

/*
* Previous versions of qemu had the ENET_INT_MAC and ENET_INT_MAC
* interrupts swapped. This worked with older versions of Linux (4.14
* and older) since Linux associated both interrupt lines with Ethernet
* MAC interrupts. Specifically,
* - Linux 4.15 and later have separate interrupt handlers for the MAC and
* timer interrupts. Those versions of Linux fail with versions of QEMU
* with swapped interrupt assignments.
* - In linux 4.14, both interrupt lines were registered with the Ethernet
* MAC interrupt handler. As a result, all versions of qemu happen to
* work, though that is accidental.
* - In Linux 4.9 and older, the timer interrupt was registered directly
* with the Ethernet MAC interrupt handler. The MAC interrupt was
* redirected to a GPIO interrupt to work around erratum ERR006687.
* This was implemented using the SOC's IOMUX block. In qemu, this GPIO
* interrupt never fired since IOMUX is currently not supported in qemu.
* Linux instead received MAC interrupts on the timer interrupt.
* As a result, qemu versions with the swapped interrupt assignment work,
* albeit accidentally, but qemu versions with the correct interrupt
* assignment fail.
*
* To ensure that all versions of Linux work, generate ENET_INT_MAC
* interrrupts on both interrupt lines. This should be changed if and when
* qemu supports IOMUX.
*/

Unfortunately, this behavior causes the QNX Sabrelite BSP
(http://blackberry.qnx.com/en/developers/bsp) to hang on ethernet
initialization. This is caused by the fact that QEMU is firing the
ENET_INT_TS_TIMER timer interrupt unexpectedly (when the ENET_INT_MAC
flag is set). The BSP functions correctly on the actual hardware, but it
is unable to handle the deliberately incorrect interrupt firing by QEMU.

>From reading the comment, it appears that this behavior is necessary to
support certain versions of Linux. However, it would be very useful to
be able to restore the correct interrupt behavior (possibly via a
command-line flag).

** Affects: qemu
Importance: Undecided
Status: New

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

Title:
Wrong interrupts generated for I.MX6 FEC controller

Status in QEMU:
New

Bug description:
The imx_eth_update function in hw/net/imx_fec.c has the following
comment

(https://github.com/qemu/qemu/blob/864ab314f1d924129d06ac7b571f105a2b76a4b2/hw/net/imx_fec.c#L421-L445):

/*
* Previous versions of qemu had the ENET_INT_MAC and ENET_INT_MAC
* interrupts swapped. This worked with older versions of Linux (4.14
* and older) since Linux associated both interrupt lines with Ethernet
* MAC interrupts. Specifically,
* - Linux 4.15 and later have separate interrupt handlers for the MAC and
* timer interrupts. Those versions of Linux fail with versions of QEMU
* with swapped interrupt assignments.
* - In linux 4.14, both interrupt lines were registered with the Ethernet
* MAC interrupt handler. As a result, all versions of qemu happen to
* work, though that is accidental.
* - In Linux 4.9 and older, the timer interrupt was registered directly
* with the Ethernet MAC interrupt handler. The MAC interrupt was
* redirected to a GPIO interrupt to work around erratum ERR006687.
* This was implemented using the SOC's IOMUX block. In qemu, this GPIO
* interrupt never fired since IOMUX is currently not supported in qemu.
* Linux instead received MAC interrupts on the timer interrupt.
* As a result, qemu versions with the swapped interrupt assignment
work,
* albeit accidentally, but qemu versions with the correct interrupt
* assignment fail.
*
* To ensure that all versions of Linux work, generate ENET_INT_MAC
* interrrupts on both interrupt lines. This should be changed if and when
* qemu supports IOMUX.
*/

Unfortunately, this behavior causes the QNX Sabrelite BSP
(http://blackberry.qnx.com/en/developers/bsp) to hang on ethernet
initialization. This is caused by the fact that QEMU is firing the
ENET_INT_TS_TIMER timer interrupt unexpectedly (when the ENET_INT_MAC
flag is set). The BSP functions correctly on the actual hardware, but
it is unable to handle the deliberately incorrect interrupt firing by
QEMU.

From reading the comment, it appears that this behavior is necessary
to support certain versions of Linux. However, it would be very useful
to be able to restore the correct interrupt

Re: [Qemu-devel] [PATCH v2 0/2] migration: cleanup ram_load

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> Two cleanup for ram_load:
> 
> * return -EINVAL for version_id mismatch
> * extract ram_load_precopy for better readability
> 
> v2: fix a comment

Queued

> Wei Yang (2):
>   migration: return -EINVAL directly when version_id mismatch
>   migration: extract ram_load_precopy
> 
>  migration/ram.c | 73 +++--
>  1 file changed, 46 insertions(+), 27 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 00/11] target/arm: decodetree prep patches

2019-08-07 Thread Peter Maydell

On Wed, 7 Aug 2019 at 05:53, Richard Henderson
 wrote:
>
> These are split out of my decodetree conversion of the
> aarch32 general instructions.  With one exception, these
> are all related to cleaning up how we refer to "PC".
>
>
> r~
>
>
> Richard Henderson (11):
>   target/arm: Pass in pc to thumb_insn_is_16bit
>   target/arm: Introduce pc_curr
>   target/arm: Introduce read_pc
>   target/arm: Introduce add_reg_for_lit
>   target/arm: Remove redundant s->pc & ~1
>   target/arm: Replace s->pc with s->base.pc_next
>   target/arm: Replace offset with pc in gen_exception_insn
>   target/arm: Replace offset with pc in gen_exception_internal_insn
>   target/arm: Remove offset argument to gen_exception_bkpt_insn
>   target/arm: Use unallocated_encoding for aarch32
>   target/arm: Remove helper_double_saturate

Whole series
Reviewed-by: Peter Maydell 

I have one query on 3/11 but the change itself is clearly
correct so it's just a question of if we need to tweak the
commit message. Once we've figured that out I'll apply the
series to target-arm.next.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] numa: Introduce MachineClass::auto_enable_numa for implicit NUMA node

2019-08-07 Thread Eduardo Habkost

On Tue, Aug 06, 2019 at 02:50:55PM +0200, Igor Mammedov wrote:
> On Mon,  5 Aug 2019 15:13:02 +0800
> Tao Xu  wrote:
> 
> > Add MachineClass::auto_enable_numa field. When it is true, a NUMA node
> > is expected to be created implicitly.
> > 
> > Acked-by: David Gibson 
> > Suggested-by: Igor Mammedov 
> > Suggested-by: Eduardo Habkost 
> > Signed-off-by: Tao Xu 
[...]
> > +mc->auto_enable_numa = true;
> 
> this will always create a numa node (that will affect not only RAM but
> also all other components that depends on numa state (like CPUs)),
> where as spapr_populate_memory() was only faking numa node in DT for RAM.
> It makes non-numa configuration impossible.
> Seeing David's ACK on the patch it might be fine, but I believe
> commit message should capture that and explain why the change in
> behavior is fine.

After a quick look, all spapr code seems to have the same
behavior when nb_numa_nodes==0 and nb_numa_nodes==1, but I'd like
to be sure.

David and/or Tao Xu: do you confirm there's no ABI change at all
on spapr after implicitly creating a NUMA node?

> 
> >  smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> >  smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 2eb9a0b4e0..4a350b87d2 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -220,6 +220,7 @@ struct MachineClass {
> >  bool smbus_no_migration_support;
> >  bool nvdimm_supported;
> >  bool numa_mem_supported;
> > +bool auto_enable_numa;
> >  
> >  HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > DeviceState *dev);
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] migration: rename migration_bitmap_sync_range to ramblock_sync_dirty_bitmap

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> Rename for better understanding of the code.
> 
> Suggested-by: Paolo Bonzini 
> Signed-off-by: Wei Yang 

this needs fixing after 'just pass RAMBlock is enough'

Dave

> ---
>  migration/ram.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index c5f9f4b0ef..66792568e2 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1669,7 +1669,7 @@ static inline bool 
> migration_bitmap_clear_dirty(RAMState *rs,
>  return ret;
>  }
>  
> -static void migration_bitmap_sync_range(RAMState *rs, RAMBlock *rb,
> +static void ramblock_sync_dirty_bitmap(RAMState *rs, RAMBlock *rb,
>  ram_addr_t length)
>  {
>  rs->migration_dirty_pages +=
> @@ -1762,7 +1762,7 @@ static void migration_bitmap_sync(RAMState *rs)
>  qemu_mutex_lock(>bitmap_mutex);
>  rcu_read_lock();
>  RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> -migration_bitmap_sync_range(rs, block, block->used_length);
> +ramblock_sync_dirty_bitmap(rs, block, block->used_length);
>  }
>  ram_counters.remaining = ram_bytes_remaining();
>  rcu_read_unlock();
> @@ -4175,7 +4175,7 @@ static void colo_flush_ram_cache(void)
>  memory_global_dirty_log_sync();
>  rcu_read_lock();
>  RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> -migration_bitmap_sync_range(ram_state, block, block->used_length);
> +ramblock_sync_dirty_bitmap(ram_state, block, block->used_length);
>  }
>  rcu_read_unlock();
>  
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: equation is more proper than and to check LOADVM_QUIT

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> LOADVM_QUIT allows a command to quit all layers of nested loadvm loops,
> while current return value check is not that proper even it works now.
> 
> Current return value check "ret & LOADVM_QUIT" would return true if
> bit[0] is 1. This would be true when ret is -1 which is used to indicate
> an error of handling a command.
> 
> Since there is only one place return LOADVM_QUIT and no other
> combination of return value, use "ret == LOADVM_QUIT" would be more
> proper.
> 
> Signed-off-by: Wei Yang 

Queued

> ---
>  migration/savevm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 1a9b1f411e..25fe7ea05a 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2429,7 +2429,7 @@ retry:
>  case QEMU_VM_COMMAND:
>  ret = loadvm_process_command(f);
>  trace_qemu_loadvm_state_section_command(ret);
> -if ((ret < 0) || (ret & LOADVM_QUIT)) {
> +if ((ret < 0) || (ret == LOADVM_QUIT)) {
>  goto out;
>  }
>  break;
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 20/26] memory: Access MemoryRegion with endianness

On 8/7/19 1:33 AM, tony.ngu...@bt.com wrote:
> @@ -551,6 +551,7 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
> hwaddr addr,
>  /* As length is under guest control, handle illegal values. */
>  return;
>  }
> +/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>   MEMTXATTRS_UNSPECIFIED);
>  }

Here is an example of where Paolo is quite right -- you cannot simply add MO_TE
via size_memop().  In patch 22 we see

> @@ -542,16 +542,15 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
> hwaddr addr,
>  val = pci_get_byte(buf);
>  break;
>  case 2:
> -val = cpu_to_le16(pci_get_word(buf));
> +val = pci_get_word(buf);
>  break;
>  case 4:
> -val = cpu_to_le32(pci_get_long(buf));
> +val = pci_get_long(buf);
>  break;
>  default:
>  /* As length is under guest control, handle illegal values. */
>  return;
>  }
> -/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>   MEMTXATTRS_UNSPECIFIED);

This is a little-endian store -- MO_LE not MO_TE.


r~

Re: [Qemu-devel] [PATCH] migration: just pass RAMBlock is enough

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> RAMBlock->used_length is always passed to migration_bitmap_sync_range(),
> which could be retrieved from RAMBlock.
> 
> Suggested-by: Paolo Bonzini 
> Signed-off-by: Wei Yang 

Queued

> ---
>  migration/ram.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 908517fc2b..0a6070d787 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1669,11 +1669,10 @@ static inline bool 
> migration_bitmap_clear_dirty(RAMState *rs,
>  return ret;
>  }
>  
> -static void migration_bitmap_sync_range(RAMState *rs, RAMBlock *rb,
> -ram_addr_t length)
> +static void migration_bitmap_sync_range(RAMState *rs, RAMBlock *rb)
>  {
>  rs->migration_dirty_pages +=
> -cpu_physical_memory_sync_dirty_bitmap(rb, 0, length,
> +cpu_physical_memory_sync_dirty_bitmap(rb, 0, rb->used_length,
>>num_dirty_pages_period);
>  }
>  
> @@ -1762,7 +1761,7 @@ static void migration_bitmap_sync(RAMState *rs)
>  qemu_mutex_lock(>bitmap_mutex);
>  rcu_read_lock();
>  RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> -migration_bitmap_sync_range(rs, block, block->used_length);
> +migration_bitmap_sync_range(rs, block);
>  }
>  ram_counters.remaining = ram_bytes_remaining();
>  rcu_read_unlock();
> @@ -4193,7 +4192,7 @@ static void colo_flush_ram_cache(void)
>  memory_global_dirty_log_sync();
>  rcu_read_lock();
>  RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> -migration_bitmap_sync_range(ram_state, block, block->used_length);
> +migration_bitmap_sync_range(ram_state, block);
>  }
>  rcu_read_unlock();
>  
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 21/26] cputlb: Replace size and endian operands for MemOp

On 8/7/19 1:33 AM, tony.ngu...@bt.com wrote:
> @@ -1246,7 +1246,7 @@ typedef uint64_t FullLoadHelper(CPUArchState *env, 
> target_ulong addr,
> 
>  static inline uint64_t __attribute__((always_inline))
>  load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
> -uintptr_t retaddr, size_t size, bool big_endian, bool code_read,
> +uintptr_t retaddr, MemOp op, bool code_read,

I assume the code generation is the same, or nearly so, for these functions?
It seems like it should be, since we're replacing one set of constant arguments
with another, and the compiler should be able to fold away the same set of 
tests.

But we should at least have a look...

> +switch (op) {
> +case MO_8:
>  res = ldub_p(haddr);
>  break;
> +case MO_BEUW:
> +res = lduw_be_p(haddr);

I don't like mixing a bare size with size+sign+endian.
I think you should go ahead and pass MO_UB.

> @@ -1605,30 +1605,27 @@ store_helper(CPUArchState *env, target_ulong addr, 
> uint64_t val,
> 
>   do_aligned_access:
>  haddr = (void *)((uintptr_t)addr + entry->addend);
> -switch (size) {
> -case 1:
> +switch (op) {
> +case MO_8:

Likewise.

r~

Re: [Qemu-devel] [PATCH] migration: use migration_in_postcopy() to check POSTCOPY_ACTIVE

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> Use common helper function to check the state.
> 
> Signed-off-by: Wei Yang 

queued

> ---
>  migration/rdma.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 3036221ee8..0e73e759ca 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -3140,7 +3140,7 @@ static size_t qemu_rdma_save_page(QEMUFile *f, void 
> *opaque,
>  
>  CHECK_ERROR_STATE();
>  
> -if (migrate_get_current()->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> +if (migration_in_postcopy()) {
>  rcu_read_unlock();
>  return RAM_SAVE_CONTROL_NOT_SUPP;
>  }
> @@ -3775,7 +3775,7 @@ static int qemu_rdma_registration_start(QEMUFile *f, 
> void *opaque,
>  
>  CHECK_ERROR_STATE();
>  
> -if (migrate_get_current()->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> +if (migration_in_postcopy()) {
>  rcu_read_unlock();
>  return 0;
>  }
> @@ -3810,7 +3810,7 @@ static int qemu_rdma_registration_stop(QEMUFile *f, 
> void *opaque,
>  
>  CHECK_ERROR_STATE();
>  
> -if (migrate_get_current()->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> +if (migration_in_postcopy()) {
>  rcu_read_unlock();
>  return 0;
>  }
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 10/11] target/arm: Use unallocated_encoding for aarch32

On 8/7/19 6:53 AM, Richard Henderson wrote:
> Promote this function from aarch64 to fully general use.
> Use it to unify the code sequences for generating illegal
> opcode exceptions.
> 
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/translate-a64.h |  2 --
>  target/arm/translate.h |  2 ++
>  target/arm/translate-a64.c |  7 ---
>  target/arm/translate-vfp.inc.c |  3 +--
>  target/arm/translate.c | 22 --
>  5 files changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h
> index 9cd2b3d238..12ad8ac6ed 100644
> --- a/target/arm/translate-a64.h
> +++ b/target/arm/translate-a64.h
> @@ -18,8 +18,6 @@
>  #ifndef TARGET_ARM_TRANSLATE_A64_H
>  #define TARGET_ARM_TRANSLATE_A64_H
>  
> -void unallocated_encoding(DisasContext *s);
> -
>  #define unsupported_encoding(s, insn)\
>  do { \
>  qemu_log_mask(LOG_UNIMP, \
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index de600073d8..6a65df0b27 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -98,6 +98,8 @@ typedef struct DisasCompare {
>  bool value_global;
>  } DisasCompare;
>  
> +void unallocated_encoding(DisasContext *s);
> +
>  /* Share the TCG temporaries common between 32 and 64 bit modes.  */
>  extern TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF;
>  extern TCGv_i64 cpu_exclusive_addr;
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index d68bfc66d3..9e1ffe9cfb 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -352,13 +352,6 @@ static inline void gen_goto_tb(DisasContext *s, int n, 
> uint64_t dest)
>  }
>  }
>  
> -void unallocated_encoding(DisasContext *s)
> -{
> -/* Unallocated and reserved encodings are uncategorized */
> -gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -   default_exception_el(s));
> -}
> -
>  static void init_tmp_a64_array(DisasContext *s)
>  {
>  #ifdef CONFIG_DEBUG_TCG
> diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
> index 5065d4524c..3e8ea80493 100644
> --- a/target/arm/translate-vfp.inc.c
> +++ b/target/arm/translate-vfp.inc.c
> @@ -108,8 +108,7 @@ static bool full_vfp_access_check(DisasContext *s, bool 
> ignore_vfp_enabled)
>  
>  if (!s->vfp_enabled && !ignore_vfp_enabled) {
>  assert(!arm_dc_feature(s, ARM_FEATURE_M));
> -gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -   default_exception_el(s));
> +unallocated_encoding(s);
>  return false;
>  }
>  
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index d6b0ab7247..2d447d4b90 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -1285,6 +1285,13 @@ static void gen_exception_bkpt_insn(DisasContext *s, 
> uint32_t syn)
>  s->base.is_jmp = DISAS_NORETURN;
>  }
>  
> +void unallocated_encoding(DisasContext *s)
> +{
> +/* Unallocated and reserved encodings are uncategorized */
> +gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> +   default_exception_el(s));
> +}
> +
>  /* Force a TB lookup after an instruction that changes the CPU state.  */
>  static inline void gen_lookup_tb(DisasContext *s)
>  {
> @@ -1315,8 +1322,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
>  return;
>  }
>  
> -gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -   default_exception_el(s));
> +unallocated_encoding(s);
>  }
>  
>  static inline void gen_add_data_offset(DisasContext *s, unsigned int insn,
> @@ -7638,8 +7644,7 @@ static void gen_srs(DisasContext *s,
>  }
>  
>  if (undef) {
> -gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -   default_exception_el(s));
> +unallocated_encoding(s);
>  return;
>  }
>  
> @@ -9266,8 +9271,7 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>  break;
>  default:
>  illegal_op:
> -gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -   default_exception_el(s));
> +unallocated_encoding(s);
>  break;
>  }
>  }
> @@ -10955,8 +10959,7 @@ static void disas_thumb2_insn(DisasContext *s, 
> uint32_t insn)
>  }
>  return;
>  illegal_op:
> -gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -   default_exception_el(s));
> +unallocated_encoding(s);
>  }
>  
>  static void disas_thumb_insn(DisasContext *s, uint32_t insn)
> @@ -11779,8 +11782,7 @@ static void disas_thumb_insn(DisasContext *s, 
> uint32_t insn)
>  return;
>

Re: [Qemu-devel] [PATCH] migration/postcopy: start_postcopy could be true only when migrate_postcopy() return true

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> There is only one place to set start_postcopy to true,
> qmp_migrate_start_postcopy(), which make sure start_postcopy could be
> set to true when migrate_postcopy() return true.
> 
> So start_postcopy is true implies the other one.
> 
> Signed-off-by: Wei Yang 

Queued

> ---
>  migration/migration.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 719d125041..27ca10122f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3101,8 +3101,7 @@ static MigIterateState 
> migration_iteration_run(MigrationState *s)
>  
>  if (pending_size && pending_size >= s->threshold_size) {
>  /* Still a significant amount to transfer */
> -if (migrate_postcopy() && !in_postcopy &&
> -pend_pre <= s->threshold_size &&
> +if (!in_postcopy && pend_pre <= s->threshold_size &&
>  atomic_read(>start_postcopy)) {
>  if (postcopy_start(s)) {
>  error_report("%s: postcopy failed to start", __func__);
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration/postcopy: PostcopyState is already set in loadvm_postcopy_handle_advise()

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> PostcopyState is already set to ADVISE at the beginning of
> loadvm_postcopy_handle_advise().
> 
> Remove the redundant set.
> 
> Signed-off-by: Wei Yang 

Queued

> ---
>  migration/savevm.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 8a2ada529e..2350e219fc 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1648,8 +1648,6 @@ static int 
> loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
>  return -1;
>  }
>  
> -postcopy_state_set(POSTCOPY_INCOMING_ADVISE);
> -
>  return 0;
>  }
>  
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 27/29] Include sysemu/sysemu.h a lot less

On 8/6/19 5:14 PM, Markus Armbruster wrote:
> In my "build everything" tree, changing sysemu/sysemu.h triggers a
> recompile of some 5400 out of 6600 objects (not counting tests and
> objects that don't depend on qemu/osdep.h).
> 
> hw/qdev-core.h includes sysemu/sysemu.h since recent commit e965ffa70a
> "qdev: add qdev_add_vm_change_state_handler()".  This is a bad idea:
> hw/qdev-core.h is widely included.
> 
> Move the declaration of qdev_add_vm_change_state_handler() to
> sysemu/sysemu.h, and drop the problematic include from hw/qdev-core.h.
> 
> Touching sysemu/sysemu.h now recompiles some 1800 objects.
> qemu/uuid.h also drops from 5400 to 1800.  A few more headers show
> smaller improvement: qemu/notify.h drops from 5600 to 5200,
> qemu/timer.h from 5600 to 4500, and qapi/qapi-types-run-state.h from
> 5500 to 5000.
> 
> Cc: Stefan Hajnoczi 
> Signed-off-by: Markus Armbruster 
> ---
>  accel/kvm/kvm-all.c   | 1 +
>  backends/hostmem.c| 1 +
>  cpus.c| 1 +
>  hw/arm/allwinner-a10.c| 1 +
>  hw/arm/aspeed_soc.c   | 1 +
>  hw/arm/kzm.c  | 1 +
>  hw/arm/msf2-soc.c | 1 +
>  hw/arm/stm32f205_soc.c| 1 +
>  hw/char/serial-isa.c  | 1 +
>  hw/char/xen_console.c | 1 +
>  hw/core/numa.c| 1 +
>  hw/core/vm-change-state-handler.c | 1 +
>  hw/display/qxl-render.c   | 1 +
>  hw/i386/xen/xen-hvm.c | 1 +
>  hw/i386/xen/xen-mapcache.c| 1 +
>  hw/intc/ioapic.c  | 1 +
>  hw/pci/pci.c  | 1 +
>  hw/riscv/sifive_e.c   | 1 +
>  hw/riscv/sifive_u.c   | 1 +
>  hw/riscv/spike.c  | 1 +
>  hw/riscv/virt.c   | 1 +
>  hw/sparc64/niagara.c  | 2 +-
>  hw/usb/hcd-ehci.h | 1 +
>  hw/xen/xen-common.c   | 1 +
>  hw/xen/xen_devconfig.c| 1 +
>  hw/xenpv/xen_machine_pv.c | 1 +
>  include/hw/qdev-core.h| 5 -
>  include/sysemu/sysemu.h   | 3 +++
>  migration/global_state.c  | 1 +
>  migration/migration.c | 1 +
>  migration/savevm.c| 1 +
>  31 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index e1a44eccf5..fc38d0b9e3 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -29,6 +29,7 @@
>  #include "exec/gdbstub.h"
>  #include "sysemu/kvm_int.h"
>  #include "sysemu/cpus.h"
> +#include "sysemu/sysemu.h"
>  #include "qemu/bswap.h"
>  #include "exec/memory.h"
>  #include "exec/ram_addr.h"

Include missing in net/netmap.c:

  CC  net/netmap.o
net/netmap.c: In function 'netmap_update_fd_handler':
net/netmap.c:108:5: error: implicit declaration of function
'qemu_set_fd_handler' [-Werror=implicit-function-declaration]
 qemu_set_fd_handler(s->nmd->fd,
 ^~~
net/netmap.c:108:5: error: nested extern declaration of
'qemu_set_fd_handler' [-Werror=nested-externs]

Re: [Qemu-devel] [PATCH 0/3] migration/savevm: move non SaveStateEntry condition check out of iteration

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> qemu_savevm_state_complete_precopy() iterates SaveStateEntry and does proper
> tasks for migration.
> 
> For each iteration, in_postcopy and iterable_only would be checked to see
> whether it should skip. Since these two conditions are not SaveStateEntry
> specific, it is proper to move the check out of iteration.
> 
> These three patches prepare the code and move the condition check out of the
> iteration.
> 

Queued

> 
> Wei Yang (3):
>   migration/savevm: flush file for iterable_only case
>   migration/savevm: split qemu_savevm_state_complete_precopy() into two
> parts
>   migration/savevm: move non SaveStateEntry condition check out of
> iteration
> 
>  migration/savevm.c | 68 ++
>  1 file changed, 50 insertions(+), 18 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 09/29] Include migration/qemu-file-types.h a lot less

On 8/7/19 2:25 PM, Philippe Mathieu-Daudé wrote:
> On 8/6/19 5:14 PM, Markus Armbruster wrote:
>> In my "build everything" tree, changing migration/qemu-file-types.h
>> triggers a recompile of some 2600 out of 6600 objects (not counting
>> tests and objects that don't depend on qemu/osdep.h).
>>
>> The culprit is again hw/hw.h, which supposedly includes it for
>> convenience.
>>
>> Include migration/qemu-file-types.h only where it's needed.  Touching
>> it now recompiles less than 200 objects.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>  hw/acpi/piix4.c | 1 +
>>  hw/block/virtio-blk.c   | 1 +
>>  hw/char/virtio-serial-bus.c | 1 +
>>  hw/display/virtio-gpu.c | 1 +
>>  hw/intc/apic_common.c   | 1 +
>>  hw/nvram/eeprom93xx.c   | 1 +
>>  hw/nvram/fw_cfg.c   | 1 +
>>  hw/pci-host/piix.c  | 1 +
>>  hw/pci/msix.c   | 1 +
>>  hw/pci/pci.c| 1 +
>>  hw/pci/shpc.c   | 1 +
>>  hw/ppc/spapr.c  | 1 +
>>  hw/s390x/s390-skeys.c   | 1 +
>>  hw/s390x/tod.c  | 1 +
>>  hw/s390x/virtio-ccw.c   | 1 +
>>  hw/scsi/mptsas.c| 1 +
>>  hw/scsi/scsi-bus.c  | 1 +
>>  hw/scsi/scsi-disk.c | 1 +
>>  hw/scsi/scsi-generic.c  | 1 +
>>  hw/scsi/virtio-scsi.c   | 1 +
>>  hw/timer/i8254_common.c | 1 +
>>  hw/timer/twl92230.c | 1 +
>>  hw/usb/redirect.c   | 1 +
>>  hw/virtio/vhost.c   | 1 +
>>  hw/virtio/virtio-mmio.c | 1 +
>>  hw/virtio/virtio-pci.c  | 1 +
>>  hw/virtio/virtio.c  | 1 +
>>  include/hw/hw.h | 1 -
>>  include/migration/cpu.h | 1 +
>>  target/ppc/kvm.c| 1 +
>>  30 files changed, 29 insertions(+), 1 deletion(-)
> [...]
>> diff --git a/include/hw/hw.h b/include/hw/hw.h
>> index a4fb2390e8..b399627cbe 100644
>> --- a/include/hw/hw.h
>> +++ b/include/hw/hw.h
>> @@ -11,7 +11,6 @@
>>  #include "exec/memory.h"
>>  #include "hw/irq.h"
>>  #include "migration/vmstate.h"
>> -#include "migration/qemu-file-types.h"
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 

Oops another miss:

hw/intc/s390_flic_kvm.c: In function 'kvm_flic_save':
hw/intc/s390_flic_kvm.c:395:9: error: implicit declaration of function
'qemu_put_be64' [-Werror=implicit-function-declaration]
 qemu_put_be64(f, FLIC_FAILED);
 ^

>>  void QEMU_NORETURN hw_error(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>>  
>> diff --git a/include/migration/cpu.h b/include/migration/cpu.h
>> index 8424f1631a..21c4fc9eab 100644
>> --- a/include/migration/cpu.h
>> +++ b/include/migration/cpu.h
>> @@ -3,6 +3,7 @@
>>  #define MIGRATION_CPU_H
>>  
>>  #include "exec/cpu-defs.h"
>> +#include "migration/qemu-file-types.h"
>>  
>>  #if TARGET_LONG_BITS == 64
>>  #define qemu_put_betl qemu_put_be64

Re: [Qemu-devel] [PATCH 3/8] block/io: handle alignment and max_transfer for copy_range

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> copy_range ignores these limitations, let's improve it. block/backup
> code handles max_transfer for copy_range by itself, now it's not needed
> more, drop it.

Shouldn’t this be two separate patches?

> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 11 ++-
>  block/io.c | 41 +
>  2 files changed, 35 insertions(+), 17 deletions(-)

[...]

> diff --git a/block/io.c b/block/io.c
> index 06305c6ea6..5abbd0fff2 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3005,6 +3005,12 @@ static int coroutine_fn bdrv_co_copy_range_internal(
>  {
>  BdrvTrackedRequest req;
>  int ret;
> +uint32_t align = MAX(src->bs->bl.request_alignment,
> + dst->bs->bl.request_alignment);
> +uint32_t max_transfer =
> +
> QEMU_ALIGN_DOWN(MIN_NON_ZERO(MIN_NON_ZERO(src->bs->bl.max_transfer,
> +  
> dst->bs->bl.max_transfer),
> + INT_MAX), align);

I suppose the outer QEMU_ALIGN_DOWN() may result in @max_transfer of 0
(in theory, if one’s max_transfer is smaller than the other’s alignment).

Not likely, but should still be handled.

The rest looks good to me.

Max

>  /* TODO We can support BDRV_REQ_NO_FALLBACK here */
>  assert(!(read_flags & BDRV_REQ_NO_FALLBACK));



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc

2019-08-07 Thread Peter Maydell

On Wed, 7 Aug 2019 at 05:53, Richard Henderson
 wrote:
>
> We currently have 3 different ways of computing the architectural
> value of "PC" as seen in the ARM ARM.
>
> The value of s->pc has been incremented past the current insn,
> but that is all.  Thus for a32, PC = s->pc + 4; for t32, PC = s->pc;
> for t16, PC = s->pc + 2.  These differing computations make it
> impossible at present to unify the various code paths.
>
> With the newly introduced s->pc_curr, we can compute the correct
> value for all cases, using the formula given in the ARM ARM.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/translate.c | 59 --
>  1 file changed, 23 insertions(+), 36 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 59e35aafbf..61933865d5 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -196,17 +196,17 @@ static inline void store_cpu_offset(TCGv_i32 var, int 
> offset)
>  #define store_cpu_field(var, name) \
>  store_cpu_offset(var, offsetof(CPUARMState, name))
>
> +/* The architectural value of PC.  */
> +static uint32_t read_pc(DisasContext *s)
> +{
> +return s->pc_curr + (s->thumb ? 4 : 8);
> +}
> +
>  /* Set a variable to the value of a CPU register.  */
>  static void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
>  {
>  if (reg == 15) {
> -uint32_t addr;
> -/* normally, since we updated PC, we need only to add one insn */
> -if (s->thumb)
> -addr = (long)s->pc + 2;
> -else
> -addr = (long)s->pc + 4;
> -tcg_gen_movi_i32(var, addr);
> +tcg_gen_movi_i32(var, read_pc(s));

So previously:
 * for A32 we would return s->pc + 4, which is the same as s->pc_curr + 8
 * for T16 we would return s->pc + 2, which is the same as s->pc_curr + 4
 * for T32 we would return s->pc + 2 -- but that's not the same as
   s->pc_curr + 4, it's s->pc_curr + 6...

Since s->pc_curr + 4 is the right architectural answer, are we
fixing a bug here? Or are all the places where T32 code calls
this function UNPREDICTABLE for the reg == 15 case ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 22/29] Include hw/boards.h a bit less

On 8/6/19 5:14 PM, Markus Armbruster wrote:
> hw/boards.h pulls in almost 60 headers.  The less we include it into
> headers, the better.  As a first step, drop superfluous inclusions,
> and downgrade some more to what's actually needed.  Gets rid of just
> one inclusion into a header.
> 
> Cc: Eduardo Habkost 
> Cc: Marcel Apfelbaum 
> Signed-off-by: Markus Armbruster 
> ---
>  backends/cryptodev-builtin.c| 1 -
>  backends/cryptodev-vhost-user.c | 1 -
>  backends/cryptodev.c| 1 -
>  hw/acpi/ich9.c  | 1 +
>  hw/alpha/dp264.c| 1 -
>  hw/alpha/typhoon.c  | 1 +
>  hw/arm/boot.c   | 1 -
>  hw/arm/exynos4210.c | 2 +-
>  hw/arm/fsl-imx25.c  | 1 -
>  hw/arm/fsl-imx31.c  | 1 -
>  hw/arm/msf2-soc.c   | 1 -
>  hw/arm/nrf51_soc.c  | 1 -
>  hw/arm/omap1.c  | 1 +
>  hw/arm/omap2.c  | 1 +
>  hw/arm/smmuv3.c | 1 -
>  hw/arm/virt.c   | 1 +
>  hw/core/numa.c  | 2 ++
>  hw/i386/pc_piix.c   | 1 -
>  hw/i386/pc_q35.c| 1 -
>  hw/i386/pc_sysfw.c  | 1 -
>  hw/ppc/e500plat.c   | 1 -
>  hw/ppc/mpc8544ds.c  | 1 -
>  hw/ppc/pnv.c| 1 +
>  hw/ppc/ppc405_uc.c  | 1 -
>  hw/ppc/spapr_cpu_core.c | 1 -
>  hw/ppc/spapr_vio.c  | 1 -
>  hw/riscv/boot.c | 2 +-
>  hw/s390x/s390-stattrib.c| 1 -
>  hw/xtensa/xtensa_memory.c   | 1 -
>  include/hw/mem/pc-dimm.h| 1 -
>  monitor/qmp-cmds.c  | 1 -
>  target/alpha/machine.c  | 1 -
>  target/arm/machine.c| 1 -
>  target/arm/monitor.c| 1 -
>  target/hppa/machine.c   | 1 -
>  target/i386/hax-all.c   | 1 -
>  target/i386/hvf/hvf.c   | 1 -
>  target/i386/hvf/x86_task.c  | 1 -
>  target/i386/machine.c   | 1 -
>  target/i386/whpx-all.c  | 1 -
>  target/lm32/machine.c   | 1 -
>  target/moxie/machine.c  | 1 -
>  target/openrisc/machine.c   | 1 -
>  target/ppc/machine.c| 1 -
>  target/sparc/machine.c  | 1 -
>  45 files changed, 10 insertions(+), 38 deletions(-)
[...]
> diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
> index bcacdd1d8f..34a9f6f7a9 100644
> --- a/target/i386/hax-all.c
> +++ b/target/i386/hax-all.c
> @@ -33,7 +33,6 @@
>  #include "sysemu/reset.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/main-loop.h"
> -#include "hw/boards.h"
>  
>  #define DEBUG_HAX 0

include/sysemu/hax.h misses to include "hw/boards.h":

target/i386/hax-all.c: In function 'hax_accel_init':
target/i386/hax-all.c:354:26: error: dereferencing pointer to incomplete
type 'MachineState {aka struct MachineState}'
 int ret = hax_init(ms->ram_size);
  ^

Re: [Qemu-devel] [PATCH 0/2] migration/postcopy: cleanup function postcopy_chunk_hostpages_pass

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> Here are two trivial function cleanup.

Queued

> BTW, I didn't test them since TPS == HPS. How could I setup a guest with
> TPS != HPS?
> 
> Wei Yang (2):
>   migration/postcopy: reduce one operation to calculate fixup_start_addr
>   migration/postcopy: do_fixup is true when host_offset is non-zero
> 
>  migration/ram.c | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings

2019-08-07 Thread Palmer Dabbelt


On Wed, 07 Aug 2019 09:41:17 PDT (-0700), Peter Maydell wrote:

On Wed, 7 Aug 2019 at 16:02, Palmer Dabbelt  wrote:


The ISA strings we're providing from QEMU aren't actually legal RISC-V
ISA strings, as both the S and U extensions cannot exist as
single-letter extensions and must instead be multi-letter strings.
We're still using the ISA strings inside QEMU to track the availiable
extensions, so this patch just strips out the S and U extensions when
formatting ISA strings.

This boots Linux on top of 4.1-rc3, which no longer has the U extension
in /proc/cpuinfo.

Signed-off-by: Palmer Dabbelt 
---
This is another late one, but I'd like to target it for 4.1 as we're
providing illegal ISA strings and I don't want to bake that into a bunch
of other code.
---
 target/riscv/cpu.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f8d07bd20ad7..4df14433d789 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -501,7 +501,22 @@ char *riscv_isa_string(RISCVCPU *cpu)
 char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
 for (i = 0; i < sizeof(riscv_exts); i++) {
 if (cpu->env.misa & RV(riscv_exts[i])) {
-*p++ = qemu_tolower(riscv_exts[i]);
+char lower = qemu_tolower(riscv_exts[i]);
+switch (lower) {
+case 's':
+case 'u':
+/*
+ * The 's' and 'u' extensions shouldn't be passed in the device
+ * tree, but we still use them internally to track extension
+ * sets.  Here we just explicitly remove them when formatting
+ * an ISA string.
+ */
+break;
+
+default:
+*p++ = qemu_tolower(riscv_exts[i]);


 *p++ = lower;  ?


Yes, thanks -- that's why I pulled the variable out in the first place :)



thanks
-- PMM

Re: [Qemu-devel] libvhost-user: Fix the VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD check

2019-08-07 Thread Boeuf, Sebastien

>From 734625fe0c031d26e612800cd9331235f58ae2e0 Mon Sep 17 00:00:00 2001
From: Sebastien Boeuf 
Date: Wed, 7 Aug 2019 07:15:32 -0700
Subject: [PATCH] libvhost-user: Fix the VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
 check

Vhost user protocol features are set as a bitmask. And the following
constant VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD value is 10 because the bit
10 indicates if the features is set or not.

The proper way to check for the presence or absence of this feature is
to shift 1 by the value of this constant and then mask it with the
actual bitmask representing the supported protocol features.

This patch aims to fix the current code as it was not doing the
shifting, but instead it was masking directly with the value of the
constant itself.

Signed-off-by: Sebastien Boeuf 
---
 contrib/libvhost-user/libvhost-user.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/libvhost-user/libvhost-user.c 
b/contrib/libvhost-user/libvhost-user.c
index fb61142bcc..d75a9c86ed 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -1112,7 +1112,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, 
int fd,
 
 vmsg.fd_num = fd_num;
 
-if ((dev->protocol_features & VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD) == 0) {
+if ((dev->protocol_features &
+(1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) == 0) {
 return false;
 }
 
@@ -2537,7 +2538,8 @@ int64_t vu_fs_cache_request(VuDev *dev, 
VhostUserSlaveRequest req, int fd,
 
 vmsg.fd_num = fd_num;
 
-if ((dev->protocol_features & VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD) == 0) {
+if ((dev->protocol_features &
+(1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) == 0) {
 return -EINVAL;
 }
 
-- 
2.17.1


From: Boeuf, Sebastien
Sent: Wednesday, August 07, 2019 9:35 AM
To: dgilb...@redhat.com
Cc: marcandre.lur...@redhat.com; qemu-devel@nongnu.org
Subject: RE: libvhost-user: Fix the VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD check

>From 950c62dd450c8f6c3fc04269bbefa3a368bb39b6 Mon Sep 17 00:00:00 2001
From: Sebastien Boeuf 
Date: Wed, 7 Aug 2019 07:15:32 -0700
Subject: [PATCH] libvhost-user: Fix the VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
 check

Vhost user protocol features are set as a bitmask. And the following
constant VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD value is 10 because the bit
10 indicates if the features is set or not.

The proper way to check for the presence or absence of this feature is
to shift 1 by the value of this constant and then mask it with the
actual bitmask representing the supported protocol features.

This patch aims to fix the current code as it was not doing the
shifting, but instead it was masking directly with the value of the
constant itself.

Signed-off-by: Sebastien Boeuf 
---
 contrib/libvhost-user/libvhost-user.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/contrib/libvhost-user/libvhost-user.c 
b/contrib/libvhost-user/libvhost-user.c
index fb61142bcc..8ff387deff 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -71,7 +71,7 @@

 /* The version of the protocol we support */
 #define VHOST_USER_VERSION 1
-#define LIBVHOST_USER_DEBUG 0
+#define LIBVHOST_USER_DEBUG 1

 #define DPRINT(...) \
 do {\
@@ -1112,7 +1112,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, 
int fd,

 vmsg.fd_num = fd_num;

-if ((dev->protocol_features & VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD) == 0) {
+if ((dev->protocol_features &
+(1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) == 0) {
 return false;
 }

@@ -2537,7 +2538,8 @@ int64_t vu_fs_cache_request(VuDev *dev, 
VhostUserSlaveRequest req, int fd,

 vmsg.fd_num = fd_num;

-if ((dev->protocol_features & VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD) == 0) {
+if ((dev->protocol_features &
+(1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) == 0) {
 return -EINVAL;
 }

--
2.17.1


From: Boeuf, Sebastien
Sent: Wednesday, August 07, 2019 9:24 AM
To: dgilb...@redhat.com
Cc: marcandre.lur...@redhat.com; qemu-devel@nongnu.org
Subject: Re: libvhost-user: Fix the VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD check

On Wed, 2019-08-07 at 17:09 +0100, Dr. David Alan Gilbert wrote:
> * Boeuf, Sebastien (sebastien.bo...@intel.com) wrote:
> > From 0a53a81db6dd069f9b7bcdcd386845bceb3a2ac6 Mon Sep 17 00:00:00
> > 2001
> > From: Sebastien Boeuf 
> > Date: Wed, 7 Aug 2019 07:15:32 -0700
> > Subject: [PATCH] libvhost-user: Fix the
> > VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
> >  check
> >
> > Vhost user protocol features are set as a bitmask. And the
> > following
> > constant VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD value is 10 because
> > the
> > bit
> > 10 indicates if the features is set or not.
> >
> > The proper way to check for the presence or absence of

Re: [Qemu-devel] [PATCH v3 07/33] automatically add vmstate for reset support in devices

2019-08-07 Thread Damien Hedde




On 8/7/19 5:07 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:
>>
>> This add the reset related sections for every QOM
>> device.
> 
> A bit more detail in the commit message would help, I think --
> this is adding extra machinery which has to copy and modify
> the VMStateDescription passed in by the device in order to
> add the subsection that handles reset.

Sorry for that, thought I've added some...

I've kept this patch separate from previous one because this it is
awkward. I'm not sure this is the right place (I mean in qdev files) do
this kind of stuff. It basically replaces every static vmsd by dynamic
ones, so it makes it harder to follow when debugging since there is no
symbol associated to them. But on the other hand, I don't see an
alternative.

I copy there what I've put in the cover-letter:
For devices, I've added a patch to automate the addition of reset
related subsection. In consequence it is not necessary to explicitly add
the reset subsection in every device specialization requiring it.
Right know this is kind of a hack into qdev to dynamically modify the
vmsd before the registration. There is probably a much cleaner way to do
this but I prefered to demonstrate it by keeping modification local to qdev.
As far as I can tell it's ok to dynamically add subsections at the end.
This does not prevent from further adding subsections in the orignal
vmsd: the subsections order in the array is irrelevant from migration
point-of-view. The loading part just lookup each subsection in the array
by name uppon reception.

> 
> I've added Dave Gilbert to the already long cc list since this
> is migration related.
> 
>> Signed-off-by: Damien Hedde 
>> ---
>>  hw/core/qdev-vmstate.c | 41 +
>>  hw/core/qdev.c | 12 +++-
>>  include/hw/qdev-core.h |  3 +++
>>  stubs/Makefile.objs|  1 +
>>  stubs/device.c |  7 +++
>>  5 files changed, 63 insertions(+), 1 deletion(-)
>>  create mode 100644 stubs/device.c
>>
>> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
>> index 07b010811f..24f8465c61 100644
>> --- a/hw/core/qdev-vmstate.c
>> +++ b/hw/core/qdev-vmstate.c
>> @@ -43,3 +43,44 @@ const struct VMStateDescription device_vmstate_reset = {
>>  VMSTATE_END_OF_LIST()
>>  },
>>  };
>> +
>> +static VMStateDescription *vmsd_duplicate_and_append(
>> +const VMStateDescription *old_vmsd,
>> +const VMStateDescription *new_subsection)
>> +{
>> +VMStateDescription *vmsd;
>> +int n = 0;
>> +
>> +assert(old_vmsd && new_subsection);
>> +
>> +vmsd = (VMStateDescription *) g_memdup(old_vmsd, sizeof(*vmsd));
>> +
>> +if (old_vmsd->subsections) {
>> +while (old_vmsd->subsections[n]) {
>> +n += 1;
>> +}
>> +}
>> +vmsd->subsections = g_new(const VMStateDescription *, n + 2);
>> +
>> +if (old_vmsd->subsections) {
>> +memcpy(vmsd->subsections, old_vmsd->subsections,
>> +   sizeof(VMStateDescription *) * n);
>> +}
>> +vmsd->subsections[n] = new_subsection;
>> +vmsd->subsections[n + 1] = NULL;
>> +
>> +return vmsd;
>> +}
>> +
>> +void device_class_build_extended_vmsd(DeviceClass *dc)
>> +{
>> +assert(dc->vmsd);
>> +assert(!dc->vmsd_ext);
>> +
>> +/* forge a subsection with proper name */
>> +VMStateDescription *reset;
>> +reset = g_memdup(_vmstate_reset, sizeof(*reset));
>> +reset->name = g_strdup_printf("%s/device_reset", dc->vmsd->name);
>> +
>> +dc->vmsd_ext = vmsd_duplicate_and_append(dc->vmsd, reset);
>> +}
> 
> This will allocate memory, but there is no corresponding
> code which frees it. This means you'll have a memory leak
> across device realize->unrealize for hotplug devices.

Right. I'll handle this along with the existing vmsd_unregister
in realize/unrealize method.

> 
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index e9e5f2d5f9..88387d3743 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -45,7 +45,17 @@ bool qdev_hot_removed = false;
>>  const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
>>  {
>>  DeviceClass *dc = DEVICE_GET_CLASS(dev);
>> -return dc->vmsd;
>> +
>> +if (!dc->vmsd) {
>> +return NULL;
>> +}
>> +
>> +if (!dc->vmsd_ext) {
>> +/* build it first time we need it */
>> +device_class_build_extended_vmsd(dc);
>> +}
>> +
>> +return dc->vmsd_ext;
>>  }
> 
> Unfortunately not everything that wants the VMSD calls
> this function. migration/savevm.c:dump_vmstate_json_to_file()
> does a direct access to dc->vmsd, so we need to fix that first.
> 
> Devices which don't use dc->vmsd won't get this and so
> their reset state won't be migrated. That's OK for anything
> that's still not yet a QOM device, I guess -- it's not possible
> for them to be in a 'held in reset' state anyway, so the
> extra subsection would never be needed.
> 
> The one I'm less sure about is the 'virtio'

Re: [Qemu-devel] [PATCH 0/3] migration/postcopy: cleanup function postcopy_send_discard_bm_ram

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> Some cleanup of function postcopy_send_discard_bm_ram:
> 
> * use a more restrict check for discard page
> * break the loop when no more page to discard
> * it is for sure discard_length is not 0
> 
> No functional change.
> 

Queued

> Wei Yang (3):
>   migration/postcopy: the valid condition is one less then end
>   migration/postcopy: break the loop when there is no more page to
> discard
>   migration/postcopy: discard_length must not be 0
> 
>  migration/ram.c | 24 +++-
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> -- 
> 2.19.1
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 14/29] migration: Move the VMStateDescription typedef to typedefs.h

On 8/6/19 5:14 PM, Markus Armbruster wrote:
> We declare incomplete struct VMStateDescription in a couple of places
> so we don't have to include migration/vmstate.h for the typedef.
> That's fine with me.  However, the next commit will drop
> migration/vmstate.h from a massive number of compiles.  Move the
> typedef to qemu/typedefs.h now, so I don't have to insert struct in
> front of VMStateDescription all over the place then.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  include/hw/qdev-core.h  | 6 ++
>  include/migration/vmstate.h | 1 -
>  include/qemu/typedefs.h | 1 +
>  include/qom/cpu.h   | 4 ++--
>  target/alpha/cpu.h  | 2 +-
>  target/arm/cpu.h| 2 +-
>  target/cris/cpu.h   | 2 +-
>  target/hppa/cpu.h   | 2 +-
>  target/i386/cpu.h   | 2 +-
>  target/lm32/cpu.h   | 2 +-
>  target/mips/internal.h  | 2 +-
>  target/openrisc/cpu.h   | 2 +-
>  target/ppc/cpu-qom.h| 2 +-
>  target/ppc/cpu.h| 2 +-
>  target/s390x/cpu.h  | 2 +-
>  target/sparc/cpu.h  | 2 +-
>  16 files changed, 17 insertions(+), 19 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé

Re: [Qemu-devel] [PATCH] migration: consolidate time info into populate_time_info