Re: [PATCH] target/riscv: Fix PMU node property for virt machine

[Yu-Chien Peter Lin]

Hi Conor,

Thank you for your prompt response.

On Fri, Apr 21, 2023 at 06:59:40PM +0100, Conor Dooley wrote:
> On Fri, Apr 21, 2023 at 09:14:37PM +0800, Yu Chien Peter Lin wrote:
> > The length of fdt_event_ctr_map[20] will add 5 dummy cells in
> > "riscv,event-to-mhpmcounters" property, so directly initialize
> > the array without an explicit size.
> > 
> > This patch also fixes the typo of PMU cache operation result ID
> > of MISS (0x1) in the comments, and renames event idx 0x10021 to
> > RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS.
> > 
> > Signed-off-by: Yu Chien Peter Lin 
> > ---
> > 
> >   $ ./build/qemu-system-riscv64 -M virt,dumpdtb=/tmp/virt.dtb -cpu 
> > rv64,sscofpmf=on && dtc /tmp/virt.dtb | grep mhpmcounters
> >   [...]
> > riscv,event-to-mhpmcounters = <0x01 0x01 0x7fff9 
> >0x02 0x02 0x7fffc
> >0x10019 0x10019 0x7fff8
> >0x1001b 0x1001b 0x7fff8
> >0x10021 0x10021 0x7fff8
> >dummy cells --->0x00 0x00 0x00 0x00 0x00>;
> > 
> > This won't break the OpenSBI, but will cause it to incorrectly increment
> > num_hw_events [1] to 6, and DT validation failure in kernel [2].
> > 
> >   $ dt-validate -p Documentation/devicetree/bindings/processed-schema.json 
> > virt.dtb
> >   [...]
> >   virt.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281], [2, 
> > 2, 524284], [65561, 65561, 524280], [65563, 65563, 524280], [65569, 65569, 
> > 524280], [0, 0, 0], [0, 0]], 'compatible': ['riscv,pmu']} should not be 
> > valid under {'type': 'object'}
> 
> I would note that this warning here does not go away with this patch ^^
> It's still on my todo list, unless you want to fix it!

I don't fully understand the warning raised by simple-bus.yaml
is it reasonable to move pmu out of soc node?

> 
> >   From schema: 
> > /home/peterlin/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
> >   virt.dtb: pmu: riscv,event-to-mhpmcounters:6: [0, 0] is too short
> >   [...]
> 
> And with the binding below there is a 3rd warning, but I think it is
> incorrect and I submitted a patch for the binding to fix it.
> 
> That said, this doesn't seem to have been submitted on top of my naive
> s/20/15/ patch that Alistair picked up. Is this intended to replace, or
> for another branch? Replace works fine for me, don't have sentimental
> feelings for that patch!
> 
> I think your approach here was better than my s/20/15/, but seems like a
> bit of a fix and a bit of cleanup all-in-one.

I just find your patch [1], this solves my problem, thanks!
I will resend this patch based on yours, to simply fix some typos.

[1] 
https://patchwork.ozlabs.org/project/qemu-devel/patch/2023040417.35179-1-co...@kernel.org/

Best regards,
Peter Lin

> Either way, warnings gone so WFM :) :)
> 
> Thanks,
> Conor.
> 
> > [1] 
> > https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_pmu.c#L255
> > [2] 
> > https://github.com/torvalds/linux/blob/v6.3-rc7/Documentation/devicetree/bindings/perf/riscv%2Cpmu.yaml#L54-L66
> > ---
> >  target/riscv/cpu.h|  2 +-
> >  target/riscv/cpu_helper.c |  2 +-
> >  target/riscv/pmu.c| 88 +++
> >  3 files changed, 45 insertions(+), 47 deletions(-)
> > 
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 638e47c75a..eab518542c 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -812,7 +812,7 @@ enum riscv_pmu_event_idx {
> >  RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> >  RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> >  RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> > -RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> > +RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS = 0x10021,
> >  };
> >  
> >  /* CSR function table */
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index f88c503cf4..5d3e032ec9 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -1210,7 +1210,7 @@ static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, 
> > MMUAccessType access_type)
> >  
> >  switch (access_type) {
> >  case MMU_INST_FETCH:
> > -pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
> > +pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS;
> >  break;
> >  case MMU_DATA_LOAD:
> >  pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS;
> > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > index b8e56d2b7b..0b21c3fa38 100644
> > --- a/target/riscv/pmu.c
> > +++ b/target/riscv/pmu.c
> > @@ -35,51 +35,49 @@
> >   */
> >  void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
> >  {
> > -uint32_t fdt_event_ctr_map[20] = {};
> > -uint32_t cmask;
> > -
> >  /* All the programmable counters can map to any event */
> > -cmask = MAKE_32BIT_MASK(3, num_ctrs);
> > -
> > -   /*
> > - 

Re: [PATCH] hw/nvram: Avoid unnecessary Xilinx eFuse backstore write

[Alistair Francis]

On Thu, Apr 27, 2023 at 10:31 AM Tong Ho  wrote:
>
> Add a check in the bit-set operation to write the backstore
> only if the affected bit is 0 before.
>
> With this in place, there will be no need for callers to
> do the checking in order to avoid unnecessary writes.
>
> Signed-off-by: Tong Ho 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/nvram/xlnx-efuse.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/hw/nvram/xlnx-efuse.c b/hw/nvram/xlnx-efuse.c
> index fdfffaab99..655c40b8d1 100644
> --- a/hw/nvram/xlnx-efuse.c
> +++ b/hw/nvram/xlnx-efuse.c
> @@ -143,6 +143,8 @@ static bool efuse_ro_bits_find(XlnxEFuse *s, uint32_t k)
>
>  bool xlnx_efuse_set_bit(XlnxEFuse *s, unsigned int bit)
>  {
> +uint32_t set, *row;
> +
>  if (efuse_ro_bits_find(s, bit)) {
>  g_autofree char *path = object_get_canonical_path(OBJECT(s));
>
> @@ -152,8 +154,13 @@ bool xlnx_efuse_set_bit(XlnxEFuse *s, unsigned int bit)
>  return false;
>  }
>
> -s->fuse32[bit / 32] |= 1 << (bit % 32);
> -efuse_bdrv_sync(s, bit);
> +/* Avoid back-end write unless there is a real update */
> +row = >fuse32[bit / 32];
> +set = 1 << (bit % 32);
> +if (!(set & *row)) {
> +*row |= set;
> +efuse_bdrv_sync(s, bit);
> +}
>  return true;
>  }
>
> --
> 2.25.1
>
>

Re: [PATCH v2 0/6] target/i386: Support new Intel platform Instructions in CPUID enumeration

[Tao Su]

On Wed, Apr 26, 2023 at 02:24:18PM +0200, Paolo Bonzini wrote:
> Queued, thanks.
> 
> Paolo
> 

Paolo, thanks!

Tao

Re: [RFC PATCH v2 3/4] target/riscv: check smstateen fcsr flag

[Weiwei Li]

On 2023/4/27 01:18, Mayuresh Chitale wrote:

On Sat, Apr 15, 2023 at 6:55 AM Weiwei Li  wrote:


On 2023/4/15 00:02, Mayuresh Chitale wrote:

If misa.F and smstateen_fcsr_ok flag are clear then all the floating
point instructions must generate an appropriate exception.

Signed-off-by: Mayuresh Chitale 
---
   target/riscv/insn_trans/trans_rvd.c.inc   | 13 
   target/riscv/insn_trans/trans_rvf.c.inc   | 24 +++
   target/riscv/insn_trans/trans_rvzfh.c.inc | 18 ++---
   3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvd.c.inc 
b/target/riscv/insn_trans/trans_rvd.c.inc
index 2c51e01c40..d9e0cf116f 100644
--- a/target/riscv/insn_trans/trans_rvd.c.inc
+++ b/target/riscv/insn_trans/trans_rvd.c.inc
@@ -18,10 +18,15 @@
* this program.  If not, see .
*/

-#define REQUIRE_ZDINX_OR_D(ctx) do { \
-if (!ctx->cfg_ptr->ext_zdinx) { \
-REQUIRE_EXT(ctx, RVD); \
-} \
+#define REQUIRE_ZDINX_OR_D(ctx) do {   \
+if (!has_ext(ctx, RVD)) {  \
+if (!ctx->cfg_ptr->ext_zdinx) {\
+return false;  \
+}  \
+if (!smstateen_fcsr_check(ctx)) {  \
+return false;  \
+}  \
+}  \
   } while (0)

   #define REQUIRE_EVEN(ctx, reg) do { \
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc 
b/target/riscv/insn_trans/trans_rvf.c.inc
index 9e9fa2087a..6bf6fe16be 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -24,10 +24,26 @@
   return false; \
   } while (0)

-#define REQUIRE_ZFINX_OR_F(ctx) do {\
-if (!ctx->cfg_ptr->ext_zfinx) { \
-REQUIRE_EXT(ctx, RVF); \
-} \
+static inline bool smstateen_fcsr_check(DisasContext *ctx)
+{
+#ifndef CONFIG_USER_ONLY
+if (!has_ext(ctx, RVF) && !ctx->smstateen_fcsr_ok) {

We needn't check RVF here. Two reasons:

1. This check only be done when RVF is diabled.

2. ctx->smstateen_fcsr_ok is always be true (set in last patch) when RVF
is enabled

Ok.

+ctx->virt_inst_excp = ctx->virt_enabled;
+return false;
+}
+#endif
+return true;
+}
+
+#define REQUIRE_ZFINX_OR_F(ctx) do {   \
+if (!has_ext(ctx, RVF)) {  \
+if (!ctx->cfg_ptr->ext_zfinx) {\
+return false;  \
+}  \
+if (!smstateen_fcsr_check(ctx)) {  \
+return false;  \
+}  \
+}  \
   } while (0)

   #define REQUIRE_ZCF(ctx) do {  \
diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc 
b/target/riscv/insn_trans/trans_rvzfh.c.inc
index 74dde37ff7..74a125e4c0 100644
--- a/target/riscv/insn_trans/trans_rvzfh.c.inc
+++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
@@ -16,28 +16,40 @@
* this program.  If not, see .
*/

-#define REQUIRE_ZFH(ctx) do { \
+#define REQUIRE_ZFH(ctx) do {  \
   if (!ctx->cfg_ptr->ext_zfh) {  \
-return false; \
-} \
+return false;  \
+}  \
+if (!smstateen_fcsr_check(ctx)) {  \
+return false;  \
+}  \

This is unnecessary here. This check is only for Zhinx.

Similar to REQUIRE_ZFHMIN.


   } while (0)

   #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
   if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
   return false;  \
   }  \
+if (!smstateen_fcsr_check(ctx)) {  \
+return false;  \
+}  \

This check is only for Zhinx here. So it's better to take the similar
way as REQUIRE_ZFINX_OR_F.

Similar to REQUIRE_ZFHMIN_OR_ZHINXMIN.

I am not sure I follow the comments above.
The FCSR check is applicable to all the extensions ie zfinx, zdinx,
zhinx and zhinxmin.


Yeah. FCSR check is applicable to all Z*inx extensions, but unnecessary 
for Zfh.  So I think it's better to be:


  if (!ctx->cfg_ptr->ext_zfh) {\
  if (!ctx->cfg_ptr->ext_zhinx)) { \
  return false;\
  }\
  if (!smstateen_fcsr_check(ctx)) {\
  return false;\
  }\
  }

Regards,

Weiwei Li


Regards,

Weiwei Li


   } while (0)

   #define 

[PATCH] hw/nvram: Avoid unnecessary Xilinx eFuse backstore write

[Tong Ho]

Add a check in the bit-set operation to write the backstore
only if the affected bit is 0 before.

With this in place, there will be no need for callers to
do the checking in order to avoid unnecessary writes.

Signed-off-by: Tong Ho 
---
 hw/nvram/xlnx-efuse.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/nvram/xlnx-efuse.c b/hw/nvram/xlnx-efuse.c
index fdfffaab99..655c40b8d1 100644
--- a/hw/nvram/xlnx-efuse.c
+++ b/hw/nvram/xlnx-efuse.c
@@ -143,6 +143,8 @@ static bool efuse_ro_bits_find(XlnxEFuse *s, uint32_t k)
 
 bool xlnx_efuse_set_bit(XlnxEFuse *s, unsigned int bit)
 {
+uint32_t set, *row;
+
 if (efuse_ro_bits_find(s, bit)) {
 g_autofree char *path = object_get_canonical_path(OBJECT(s));
 
@@ -152,8 +154,13 @@ bool xlnx_efuse_set_bit(XlnxEFuse *s, unsigned int bit)
 return false;
 }
 
-s->fuse32[bit / 32] |= 1 << (bit % 32);
-efuse_bdrv_sync(s, bit);
+/* Avoid back-end write unless there is a real update */
+row = >fuse32[bit / 32];
+set = 1 << (bit % 32);
+if (!(set & *row)) {
+*row |= set;
+efuse_bdrv_sync(s, bit);
+}
 return true;
 }
 
-- 
2.25.1

[PATCH 2/2] tests/tcg/s390x: Test EXECUTE of relative branches

[Ilya Leoshkevich]

Add a small test to prevent regressions.

Signed-off-by: Ilya Leoshkevich 
---
 tests/tcg/s390x/Makefile.target |   1 +
 tests/tcg/s390x/ex-branch.c | 158 
 2 files changed, 159 insertions(+)
 create mode 100644 tests/tcg/s390x/ex-branch.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 0031868b136..23dc8b6a63f 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -34,6 +34,7 @@ TESTS+=cdsg
 TESTS+=chrl
 TESTS+=rxsbg
 TESTS+=ex-relative-long
+TESTS+=ex-branch
 
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
diff --git a/tests/tcg/s390x/ex-branch.c b/tests/tcg/s390x/ex-branch.c
new file mode 100644
index 000..c6067191528
--- /dev/null
+++ b/tests/tcg/s390x/ex-branch.c
@@ -0,0 +1,158 @@
+/* Check EXECUTE with relative branch instructions as targets. */
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct test {
+const char *name;
+void (*func)(long *link, long *magic);
+long exp_link;
+};
+
+/* Branch instructions and their expected effects. */
+#define LINK_64(test) ((long)test ## _exp_link)
+#define LINK_NONE(test) -1L
+#define FOR_EACH_INSN(F)   
\
+F(bras,  "%[link]", LINK_64)   
\
+F(brasl, "%[link]", LINK_64)   
\
+F(brc,   "0x8", LINK_NONE) 
\
+F(brcl,  "0x8", LINK_NONE) 
\
+F(brct,  "%%r0",LINK_NONE) 
\
+F(brctg, "%%r0",LINK_NONE) 
\
+F(brxh,  "%%r2,%%r0",   LINK_NONE) 
\
+F(brxhg, "%%r2,%%r0",   LINK_NONE) 
\
+F(brxle, "%%r0,%%r1",   LINK_NONE) 
\
+F(brxlg, "%%r0,%%r1",   LINK_NONE) 
\
+F(crj,   "%%r0,%%r0,8", LINK_NONE) 
\
+F(cgrj,  "%%r0,%%r0,8", LINK_NONE) 
\
+F(cij,   "%%r0,0,8",LINK_NONE) 
\
+F(cgij,  "%%r0,0,8",LINK_NONE) 
\
+F(clrj,  "%%r0,%%r0,8", LINK_NONE) 
\
+F(clgrj, "%%r0,%%r0,8", LINK_NONE) 
\
+F(clij,  "%%r0,0,8",LINK_NONE) 
\
+F(clgij, "%%r0,0,8",LINK_NONE)
+
+#define INIT_TEST  
\
+"xgr %%r0,%%r0\n"  /* %r0 = 0; %cc = 0 */  
\
+"lghi %%r1,1\n"/* %r1 = 1 */   
\
+"lghi %%r2,2\n"/* %r2 = 2 */
+
+#define CLOBBERS_TEST "cc", "0", "1", "2"
+
+#define DEFINE_TEST(insn, args, exp_link)  
\
+extern char insn ## _exp_link[];   
\
+static void test_ ## insn(long *link, long *magic) 
\
+{  
\
+asm(INIT_TEST  
\
+#insn " " args ",0f\n" 
\
+".globl " #insn "_exp_link\n"  
\
+#insn "_exp_link:\n"   
\
+".org . + 90\n"
\
+"0: lgfi %[magic],0x12345678\n"
\
+: [link] "+r" (*link)  
\
+, [magic] "+r" (*magic)
\
+: : CLOBBERS_TEST);
\
+}  
\
+extern char ex_ ## insn ## _exp_link[];
\
+static void test_ex_ ## insn(long *link, long *magic)  
\
+{  
\
+unsigned long target;  
\
+   
\
+asm(INIT_TEST  
\
+"larl %[target],0f\n"  
\
+"ex %%r0,0(%[target])\n"   
\
+".globl ex_" #insn "_exp_link\n"   
\
+"ex_" #insn "_exp_link:\n"  

[PATCH 1/2] target/s390x: Fix EXECUTE of relative branches

[Ilya Leoshkevich]

Fix a problem similar to the one fixed by commit 703d03a4aaf3
("target/s390x: Fix EXECUTE of relative long instructions"), but now
for relative branches.

Reported-by: Nina Schoetterl-Glausch 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/translate.c | 81 ++--
 1 file changed, 58 insertions(+), 23 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 46b874e94da..2a681428af1 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1534,18 +1534,51 @@ static DisasJumpType op_bal(DisasContext *s, DisasOps 
*o)
 }
 }
 
+/*
+ * Disassemble the target of a branch. The results are returned in a form
+ * suitable for passing into help_branch():
+ *
+ * - bool IS_IMM reflects whether the target is fixed or computed. Non-EXECUTEd
+ *   branches, whose DisasContext *S contains the relative immediate field RI,
+ *   are considered fixed. All the other branches are considered computed.
+ * - int IMM is the value of RI.
+ * - TCGv_i64 CDEST is the address of the computed target.
+ */
+#define disas_jdest(s, ri, is_imm, imm, cdest) do {
\
+if (have_field(s, ri)) {   
\
+if (unlikely(s->ex_value)) {   
\
+cdest = tcg_temp_new_i64();
\
+tcg_gen_ld_i64(cdest, cpu_env, offsetof(CPUS390XState, 
ex_target));\
+tcg_gen_addi_i64(cdest, cdest, (int64_t)get_field(s, ri) * 2); 
\
+is_imm = false;
\
+} else {   
\
+is_imm = true; 
\
+}  
\
+} else {   
\
+is_imm = false;
\
+}  
\
+imm = is_imm ? get_field(s, ri) : 0;   
\
+} while (false)
+
 static DisasJumpType op_basi(DisasContext *s, DisasOps *o)
 {
+DisasCompare c;
+bool is_imm;
+int imm;
+
 pc_to_link_info(o->out, s, s->pc_tmp);
-return help_goto_direct(s, s->base.pc_next + (int64_t)get_field(s, i2) * 
2);
+
+disas_jdest(s, i2, is_imm, imm, o->in2);
+disas_jcc(s, , 0xf);
+return help_branch(s, , is_imm, imm, o->in2);
 }
 
 static DisasJumpType op_bc(DisasContext *s, DisasOps *o)
 {
 int m1 = get_field(s, m1);
-bool is_imm = have_field(s, i2);
-int imm = is_imm ? get_field(s, i2) : 0;
 DisasCompare c;
+bool is_imm;
+int imm;
 
 /* BCR with R2 = 0 causes no branching */
 if (have_field(s, r2) && get_field(s, r2) == 0) {
@@ -1562,6 +1595,7 @@ static DisasJumpType op_bc(DisasContext *s, DisasOps *o)
 return DISAS_NEXT;
 }
 
+disas_jdest(s, i2, is_imm, imm, o->in2);
 disas_jcc(s, , m1);
 return help_branch(s, , is_imm, imm, o->in2);
 }
@@ -1569,10 +1603,10 @@ static DisasJumpType op_bc(DisasContext *s, DisasOps *o)
 static DisasJumpType op_bct32(DisasContext *s, DisasOps *o)
 {
 int r1 = get_field(s, r1);
-bool is_imm = have_field(s, i2);
-int imm = is_imm ? get_field(s, i2) : 0;
 DisasCompare c;
+bool is_imm;
 TCGv_i64 t;
+int imm;
 
 c.cond = TCG_COND_NE;
 c.is_64 = false;
@@ -1584,6 +1618,7 @@ static DisasJumpType op_bct32(DisasContext *s, DisasOps 
*o)
 c.u.s32.b = tcg_constant_i32(0);
 tcg_gen_extrl_i64_i32(c.u.s32.a, t);
 
+disas_jdest(s, i2, is_imm, imm, o->in2);
 return help_branch(s, , is_imm, imm, o->in2);
 }
 
@@ -1611,9 +1646,9 @@ static DisasJumpType op_bcth(DisasContext *s, DisasOps *o)
 static DisasJumpType op_bct64(DisasContext *s, DisasOps *o)
 {
 int r1 = get_field(s, r1);
-bool is_imm = have_field(s, i2);
-int imm = is_imm ? get_field(s, i2) : 0;
 DisasCompare c;
+bool is_imm;
+int imm;
 
 c.cond = TCG_COND_NE;
 c.is_64 = true;
@@ -1622,6 +1657,7 @@ static DisasJumpType op_bct64(DisasContext *s, DisasOps 
*o)
 c.u.s64.a = regs[r1];
 c.u.s64.b = tcg_constant_i64(0);
 
+disas_jdest(s, i2, is_imm, imm, o->in2);
 return help_branch(s, , is_imm, imm, o->in2);
 }
 
@@ -1629,10 +1665,10 @@ static DisasJumpType op_bx32(DisasContext *s, DisasOps 
*o)
 {
 int r1 = get_field(s, r1);
 int r3 = get_field(s, r3);
-bool is_imm = have_field(s, i2);
-int imm = is_imm ? get_field(s, i2) : 0;
 DisasCompare c;
+bool is_imm;
 TCGv_i64 t;
+int imm;
 
 c.cond = (s->insn->data ? TCG_COND_LE : TCG_COND_GT);
 c.is_64 = false;
@@ -1645,6 +1681,7 @@ static DisasJumpType op_bx32(DisasContext *s, DisasOps *o)
 tcg_gen_extrl_i64_i32(c.u.s32.b, 

[PATCH 0/2] target/s390x: Fix EXECUTE of relative branches

[Ilya Leoshkevich]

Hi,

This series fixes EXECUTing relative branches: currently the offset is
incorrectly applied to EXECUTE and not the branch itself. This is
similar to what I previously fixed for load/store instructions.

Unfortunately here it's not feaisble to use the ri2 field, since it
would break the direct branch optimization. Instead, introduce the
disas_jdest() macro and pass its output to help_branch().

Patch 1 is the fix, patch 2 is the test.

Best regards,
Ilya

Ilya Leoshkevich (2):
  target/s390x: Fix EXECUTE of relative branches
  tests/tcg/s390x: Test EXECUTE of relative branches

 target/s390x/tcg/translate.c|  81 +++-
 tests/tcg/s390x/Makefile.target |   1 +
 tests/tcg/s390x/ex-branch.c | 158 
 3 files changed, 217 insertions(+), 23 deletions(-)
 create mode 100644 tests/tcg/s390x/ex-branch.c

-- 
2.40.0

[PATCH v2 1/1] ui/gtk: Added an input mode

[Singh, Satyeshwar]

In a multi-seat scenario where multiple keyboards and mice are connected
to the host but some are dedicated for the guests only (through pass
through mode) and some are only for the host, there is a strong use case
where a customer does not want a HID device connected to the host to be
able to control the guest.
In such a scenario, neither should we bind any input events to Qemu UI,
nor should we show menu options like "Grab on Hover" or "Grab Input".
This patch adds a GTK command line option called "input".
It can be set like this:
  gtk,input=off/on

If set to on or completely left out, it will default to normal
operation where host HID devices can control the guests. However, if
turned off, then host HID devices will not be able to control the guest
windows.

Signed-off-by: Satyeshwar Singh 
Cc: Dongwon Kim 
Cc: Vivek Kasireddy 
Cc: Gerd Hoffmann 
Cc: Marc-André Lureau 
Cc: Daniel P. Berrangé 
---
 qapi/ui.json|  6 +-
 qemu-options.hx |  5 -
 ui/gtk.c| 43 ++-
 3 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 98322342f7..f8644e6f48 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1214,6 +1214,9 @@
 #   Since 7.1
 # @show-menubar: Display the main window menubar. Defaults to "on".
 #Since 8.0
+# @input:   Don't let host's HID devices control the guest. Defaults to 
"on" so
+#   they can control the guest.
+#   Since 8.0
 #
 # Since: 2.12
 ##
@@ -1221,7 +1224,8 @@
   'data': { '*grab-on-hover' : 'bool',
 '*zoom-to-fit'   : 'bool',
 '*show-tabs' : 'bool',
-'*show-menubar'  : 'bool'  } }
+'*show-menubar'  : 'bool',
+'*input' : 'bool'  } }
 
 ##
 # @DisplayEGLHeadless:
diff --git a/qemu-options.hx b/qemu-options.hx
index 59bdf67a2c..22a78e294f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1977,7 +1977,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
 #if defined(CONFIG_GTK)
 "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
 "
[,show-tabs=on|off][,show-cursor=on|off][,window-close=on|off]\n"
-"[,show-menubar=on|off]\n"
+"[,show-menubar=on|off][,input=on|off]\n"
 #endif
 #if defined(CONFIG_VNC)
 "-display vnc=[,]\n"
@@ -2072,6 +2072,9 @@ SRST
 
 ``show-menubar=on|off`` : Display the main window menubar, defaults to 
"on"
 
+``input=on|off``: Don't let host's HID devices control the 
guest
+  if set to "off", defaults to "on"
+
 ``curses[,charset=]``
 Display video output via curses. For graphics device models
 which support a text mode, QEMU can display this output using a
diff --git a/ui/gtk.c b/ui/gtk.c
index f16e0f8dee..00446121c3 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1967,6 +1967,20 @@ static void gd_connect_vc_gfx_signals(VirtualConsole *vc)
  G_CALLBACK(gd_resize_event), vc);
 }
 #endif
+if (qemu_console_is_graphic(vc->gfx.dcl.con)) {
+g_signal_connect(vc->gfx.drawing_area, "configure-event",
+ G_CALLBACK(gd_configure), vc);
+}
+
+/*
+ * Don't configure input events if the user has provided an option
+ * for input and explicitly set it to off. In this case, they want
+ * passthrough HID devices to control the guest.
+ */
+if (vc->s->opts->u.gtk.has_input && !vc->s->opts->u.gtk.input ) {
+return;
+}
+
 if (qemu_console_is_graphic(vc->gfx.dcl.con)) {
 g_signal_connect(vc->gfx.drawing_area, "event",
  G_CALLBACK(gd_event), vc);
@@ -1989,8 +2003,6 @@ static void gd_connect_vc_gfx_signals(VirtualConsole *vc)
  G_CALLBACK(gd_focus_in_event), vc);
 g_signal_connect(vc->gfx.drawing_area, "focus-out-event",
  G_CALLBACK(gd_focus_out_event), vc);
-g_signal_connect(vc->gfx.drawing_area, "configure-event",
- G_CALLBACK(gd_configure), vc);
 g_signal_connect(vc->gfx.drawing_area, "grab-broken-event",
  G_CALLBACK(gd_grab_broken_event), vc);
 } else {
@@ -2033,8 +2045,10 @@ static void gd_connect_signals(GtkDisplayState *s)
  G_CALLBACK(gd_menu_zoom_fixed), s);
 g_signal_connect(s->zoom_fit_item, "activate",
  G_CALLBACK(gd_menu_zoom_fit), s);
-g_signal_connect(s->grab_item, "activate",
+if (!s->opts->u.gtk.has_input || s->opts->u.gtk.input) {
+   g_signal_connect(s->grab_item, "activate",
  G_CALLBACK(gd_menu_grab_input), s);
+}
 g_signal_connect(s->notebook, "switch-page",
  G_CALLBACK(gd_change_page), s);
 }
@@ -2228,18 +2242,21 @@ static GtkWidget *gd_create_menu_view(GtkDisplayState 
*s, DisplayOptions *opts)
 

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

[BALATON Zoltan]

On Wed, 26 Apr 2023, Bernhard Beschow wrote:

Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland 
:

On 22/04/2023 16:07, Bernhard Beschow wrote:


Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
standard-compliant PCI IDE device.

Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  2 --
  hw/ide/pci.c |  4 ++--
  hw/ide/sii3112.c | 50 
  3 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 5025df5b82..dbb4b13161 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
  extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
  -extern const MemoryRegionOps pci_ide_cmd_le_ops;
-extern const MemoryRegionOps pci_ide_data_le_ops;
  #endif
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b2fcc00a64..97ccc75aa6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
  ide_ctrl_write(bus, addr + 2, data);
  }
  -const MemoryRegionOps pci_ide_cmd_le_ops = {
+static const MemoryRegionOps pci_ide_cmd_le_ops = {
  .read = pci_ide_status_read,
  .write = pci_ide_ctrl_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
@@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
  }
  }
  -const MemoryRegionOps pci_ide_data_le_ops = {
+static const MemoryRegionOps pci_ide_data_le_ops = {
  .read = pci_ide_data_read,
  .write = pci_ide_data_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 0af897a9ef..9cf920369f 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
  val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
  val |= (uint32_t)d->i.bmdma[1].status << 16;
  break;
-case 0x80 ... 0x87:
-val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
-break;
-case 0x8a:
-val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
-break;
  case 0xa0:
  val = d->regs[0].confstat;
  break;
-case 0xc0 ... 0xc7:
-val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
-break;
-case 0xca:
-val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
-break;
  case 0xe0:
  val = d->regs[1].confstat;
  break;
@@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
  case 0x0c ... 0x0f:
  bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
  break;
-case 0x80 ... 0x87:
-pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
-break;
-case 0x8a:
-pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
-break;
-case 0xc0 ... 0xc7:
-pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
-break;
-case 0xca:
-pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
-break;
  case 0x100:
  d->regs[0].scontrol = val & 0xfff;
  if (val & 1) {
@@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  pci_config_set_interrupt_pin(dev->config, 1);
  pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
  +pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
+pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
+pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
+pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
+
  /* BAR5 is in PCI memory space */
  memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
   "sii3112.bar5", 0x200);
@@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
/* BAR0-BAR4 are PCI I/O space aliases into BAR5 */
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 0x80, 8);
-pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >data_ops[0], 0,
+ memory_region_size(>data_ops[0]));
+memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 0x88, 4);
-pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >cmd_ops[0], 0,
+ memory_region_size(>cmd_ops[0]));
+memory_region_add_subregion_overlap(>mmio, 0x88, mr, 1);
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", >mmio, 0xc0, 8);
-pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr);
+

Re: [PATCH v4 3/5] parallels: Add checking and repairing duplicate offsets in BAT

[Mike Maslenkin]

On Mon, Apr 24, 2023 at 12:44 PM Alexander Ivanov
 wrote:
>
> Cluster offsets must be unique among all the BAT entries. Find duplicate
> offsets in the BAT and fix it by copying the content of the relevant
> cluster to a newly allocated cluster and set the new cluster offset to the
> duplicated entry.
>
> Add host_cluster_index() helper to deduplicate the code.
>
> Move parallels_fix_leak() call to parallels_co_check() to fix both types
> of leak: real corruption and a leak produced by allocate_clusters()
> during deduplication.
>
> Signed-off-by: Alexander Ivanov 
> ---
>  block/parallels.c | 134 --
>  1 file changed, 129 insertions(+), 5 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index ec89ed894b..3b992e8173 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -136,6 +136,12 @@ static int cluster_remainder(BDRVParallelsState *s, 
> int64_t sector_num,
>  return MIN(nb_sectors, ret);
>  }
>
> +static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
> +{
> +off -= s->header->data_off << BDRV_SECTOR_BITS;
> +return off / s->cluster_size;
> +}
> +

I guess  there should be:
off -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS

Regards,
Mike.

RE: [PATCH 08/21] Hexagon (target/hexagon) Clean up pred_written usage

[Taylor Simpson]

> -Original Message-
> From: Richard Henderson 
> Sent: Wednesday, April 26, 2023 4:23 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: phi...@linaro.org; a...@rev.ng; a...@rev.ng; Brian Cain
> ; Matheus Bernardino (QUIC)
> 
> Subject: Re: [PATCH 08/21] Hexagon (target/hexagon) Clean up
> pred_written usage
> 
> On 4/26/23 01:42, Taylor Simpson wrote:
> > Only endloop instructions will conditionally write to a predicate.
> > When there is an endloop instruction, we preload the values into
> > new_pred_value.
> >
> > The only place pred_written is needed is when HEX_DEBUG is on.
> >
> > We remove the last use of check_for_attrib.  However, new uses will be
> > introduced later in this series, so we change it to "static inline".
> 
> This is insufficient -- clang will warn about unused inline functions within 
> the
> main C file (as opposed to #included).
> 
> Use __attribute__((unused)) instead, and remove it when it gains new
> unconditional uses.

Is it OK to use the attribute itself or is G_GNUC_UNUSED preferred?


> Otherwise,
> Reviewed-by: Richard Henderson 

Thanks,
Taylor

[PATCH] aio-posix: zero out io_uring sqe user_data

[Stefan Hajnoczi]

liburing does not clear sqe->user_data. We must do it ourselves to avoid
undefined behavior in process_cqe() when user_data is used.

Note that fdmon-io_uring is currently disabled, so this is a latent bug
that does not affect users. Let's merge this fix now to make it easier
to enable fdmon-io_uring in the future (and I'm working on that).

Signed-off-by: Stefan Hajnoczi 
---
 util/fdmon-io_uring.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c
index ab43052dd7..35165bcb46 100644
--- a/util/fdmon-io_uring.c
+++ b/util/fdmon-io_uring.c
@@ -184,6 +184,7 @@ static void add_poll_remove_sqe(AioContext *ctx, AioHandler 
*node)
 #else
 io_uring_prep_poll_remove(sqe, node);
 #endif
+io_uring_sqe_set_data(sqe, NULL);
 }
 
 /* Add a timeout that self-cancels when another cqe becomes ready */
@@ -197,6 +198,7 @@ static void add_timeout_sqe(AioContext *ctx, int64_t ns)
 
 sqe = get_sqe(ctx);
 io_uring_prep_timeout(sqe, , 1, 0);
+io_uring_sqe_set_data(sqe, NULL);
 }
 
 /* Add sqes from ctx->submit_list for submission */
-- 
2.40.0

Re: [PATCH 08/21] Hexagon (target/hexagon) Clean up pred_written usage

[Richard Henderson]

On 4/26/23 01:42, Taylor Simpson wrote:

Only endloop instructions will conditionally write to a predicate.
When there is an endloop instruction, we preload the values into
new_pred_value.

The only place pred_written is needed is when HEX_DEBUG is on.

We remove the last use of check_for_attrib.  However, new uses will be
introduced later in this series, so we change it to "static inline".


This is insufficient -- clang will warn about unused inline functions within the main C 
file (as opposed to #included).


Use __attribute__((unused)) instead, and remove it when it gains new 
unconditional uses.

Otherwise,
Reviewed-by: Richard Henderson 


r~

Re: [PATCH 07/21] Hexagon (target/hexagon) Eliminate uses of log_pred_write function

[Richard Henderson]

On 4/26/23 01:41, Taylor Simpson wrote:

These instructions have implicit writes to registers, so we don't
want them to be helpers when idef-parser is off.

The following instructions are overriden
 S2_cabacdecbin
 SA1_cmpeqi

Remove the log_pred_write function from op_helper.c
Remove references in macros.h

Signed-off-by: Taylor Simpson
---
  target/hexagon/gen_tcg.h   | 16 +++
  target/hexagon/helper.h|  2 +
  target/hexagon/macros.h|  4 --
  target/hexagon/genptr.c|  5 ++
  target/hexagon/op_helper.c | 96 --
  5 files changed, 104 insertions(+), 19 deletions(-)


Acked-by: Richard Henderson 

r~

Re: [PATCH 06/21] Hexagon (target/hexagon) Remove log_reg_write from op_helper.[ch]

[Richard Henderson]

On 4/26/23 01:41, Taylor Simpson wrote:

With the overrides added in prior commits, this function is not used
Remove references in macros.h

Signed-off-by: Taylor Simpson
---
  target/hexagon/macros.h| 14 --
  target/hexagon/op_helper.h |  4 
  target/hexagon/op_helper.c | 17 -
  3 files changed, 35 deletions(-)


Yay!

Reviewed-by: Richard Henderson 

Re: [PATCH 05/21] Hexagon (target/hexagon) Add overrides for clr[tf]new

[Richard Henderson]

On 4/26/23 01:41, Taylor Simpson wrote:

These instructions have implicit reads from p0, so we don't want
them in helpers when idef-parser is off.

Signed-off-by: Taylor Simpson 
---
  target/hexagon/gen_tcg.h | 18 ++
  target/hexagon/macros.h  |  4 
  2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/target/hexagon/gen_tcg.h b/target/hexagon/gen_tcg.h
index 7c5cb93297..5e87d1d861 100644
--- a/target/hexagon/gen_tcg.h
+++ b/target/hexagon/gen_tcg.h
@@ -1097,6 +1097,24 @@
  gen_jump(ctx, riV); \
  } while (0)
  
+/* if (p0.new) r0 = #0 */

+#define fGEN_TCG_SA1_clrtnew(SHORTCODE) \
+do { \
+TCGLabel *skip = gen_new_label(); \
+tcg_gen_brcondi_tl(TCG_COND_EQ, hex_new_pred_value[0], 0, skip); \
+tcg_gen_movi_tl(RdV, 0); \
+gen_set_label(skip); \
+} while (0)


This ought to be a movcond.


+
+/* if (!p0.new) r0 = #0 */
+#define fGEN_TCG_SA1_clrfnew(SHORTCODE) \
+do { \
+TCGLabel *skip = gen_new_label(); \
+tcg_gen_brcondi_tl(TCG_COND_NE, hex_new_pred_value[0], 0, skip); \
+tcg_gen_movi_tl(RdV, 0); \
+gen_set_label(skip); \
+} while (0)


Likewise.


r~

RE: [PATCH 1/9] Hexagon (target/hexagon) Add support for v68/v69/v71/v73

[Taylor Simpson]

> -Original Message-
> From: Anton Johansson 
> Sent: Wednesday, April 26, 2023 1:06 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng; Brian Cain
> ; Matheus Bernardino (QUIC)
> 
> Subject: Re: [PATCH 1/9] Hexagon (target/hexagon) Add support for
> v68/v69/v71/v73
> 
> On 4/26/23 04:30, Taylor Simpson wrote:
> > diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index
> > ab40cfc283..8699db8c24 100644
> > --- a/target/hexagon/cpu.c
> > +++ b/target/hexagon/cpu.c
> > @@ -29,6 +29,22 @@ static void hexagon_v67_cpu_init(Object *obj)
> >   {
> >   }
> >
> > +static void hexagon_v68_cpu_init(Object *obj) { }
> > +
> > +static void hexagon_v69_cpu_init(Object *obj) { }
> > +
> > +static void hexagon_v71_cpu_init(Object *obj) { }
> > +
> > +static void hexagon_v73_cpu_init(Object *obj) { }
> > +
> >   static ObjectClass *hexagon_cpu_class_by_name(const char
> *cpu_model)
> >   {
> >   ObjectClass *oc;
> > @@ -382,6 +398,10 @@ static const TypeInfo hexagon_cpu_type_infos[] =
> {
> >   .class_init = hexagon_cpu_class_init,
> >   },
> >   DEFINE_CPU(TYPE_HEXAGON_CPU_V67,
> hexagon_v67_cpu_init),
> > +DEFINE_CPU(TYPE_HEXAGON_CPU_V68,
> hexagon_v68_cpu_init),
> > +DEFINE_CPU(TYPE_HEXAGON_CPU_V69,
> hexagon_v69_cpu_init),
> > +DEFINE_CPU(TYPE_HEXAGON_CPU_V71,
> hexagon_v71_cpu_init),
> > +DEFINE_CPU(TYPE_HEXAGON_CPU_V73,
> hexagon_v73_cpu_init),
> 
> The large spacing to hexagon_v*_cpu_init looks a bit odd.

I'll put them each on a single line with no line in between.

> 
> Also, do we need to provide a *_cpu_init() stub for each version? Seems
> from qom/object.c like we should be able to just default initialize it

I could point them all to a single function, but at some point, we'll want to 
execute only the instructions that are available an the specified version of 
the core.

> 
> Otherwise,
> 
> Reviewed-by: Anton Johansson 

Re: [PATCH 12/13] hw/ide/sii3112: Reuse PCIIDEState::bmdma_ops

[Bernhard Beschow]

Am 26. April 2023 11:44:29 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> Allows to unexport bmdma_addr_ioport_ops and models TYPE_SII3112_PCI as a
>> standard-compliant PCI IDE device.
>
>Nice! I think it's worth adding a brief mention that you've added BMDMA 
>trace-events, but otherwise looks sensible to me.

Will do. I'd also split this patch into two.

Best regards,
Bernhard

>
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   include/hw/ide/pci.h |  1 -
>>   hw/ide/pci.c |  2 +-
>>   hw/ide/sii3112.c | 94 ++--
>>   hw/ide/trace-events  |  6 ++-
>>   4 files changed, 60 insertions(+), 43 deletions(-)
>> 
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index dbb4b13161..81e0370202 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -59,7 +59,6 @@ struct PCIIDEState {
>>   void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
>>   void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops);
>>   void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
>> -extern MemoryRegionOps bmdma_addr_ioport_ops;
>>   void pci_ide_create_devs(PCIDevice *dev);
>> #endif
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index 97ccc75aa6..3539b162b7 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -342,7 +342,7 @@ static void bmdma_addr_write(void *opaque, hwaddr addr,
>>   bm->addr |= ((data & mask) << shift) & ~3;
>>   }
>>   -MemoryRegionOps bmdma_addr_ioport_ops = {
>> +static MemoryRegionOps bmdma_addr_ioport_ops = {
>>   .read = bmdma_addr_read,
>>   .write = bmdma_addr_write,
>>   .endianness = DEVICE_LITTLE_ENDIAN,
>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>> index 9cf920369f..373c0dd1ee 100644
>> --- a/hw/ide/sii3112.c
>> +++ b/hw/ide/sii3112.c
>> @@ -34,47 +34,73 @@ struct SiI3112PCIState {
>>   SiI3112Regs regs[2];
>>   };
>>   -/* The sii3112_reg_read and sii3112_reg_write functions implement the
>> - * Internal Register Space - BAR5 (section 6.7 of the data sheet).
>> - */
>> -
>> -static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
>> -unsigned int size)
>> +static uint64_t sii3112_bmdma_read(void *opaque, hwaddr addr, unsigned int 
>> size)
>>   {
>> -SiI3112PCIState *d = opaque;
>> +BMDMAState *bm = opaque;
>> +SiI3112PCIState *d = SII3112_PCI(bm->pci_dev);
>> +int i = (bm == >pci_dev->bmdma[0]) ? 0 : 1;
>>   uint64_t val;
>> switch (addr) {
>>   case 0x00:
>> -val = d->i.bmdma[0].cmd;
>> +val = bm->cmd;
>>   break;
>>   case 0x01:
>> -val = d->regs[0].swdata;
>> +val = d->regs[i].swdata;
>>   break;
>>   case 0x02:
>> -val = d->i.bmdma[0].status;
>> +val = bm->status;
>>   break;
>>   case 0x03:
>>   val = 0;
>>   break;
>> -case 0x04 ... 0x07:
>> -val = bmdma_addr_ioport_ops.read(>i.bmdma[0], addr - 4, size);
>> -break;
>> -case 0x08:
>> -val = d->i.bmdma[1].cmd;
>> +default:
>> +val = 0;
>>   break;
>> -case 0x09:
>> -val = d->regs[1].swdata;
>> +}
>> +trace_sii3112_bmdma_read(size, addr, val);
>> +return val;
>> +}
>> +
>> +static void sii3112_bmdma_write(void *opaque, hwaddr addr,
>> +uint64_t val, unsigned int size)
>> +{
>> +BMDMAState *bm = opaque;
>> +SiI3112PCIState *d = SII3112_PCI(bm->pci_dev);
>> +int i = (bm == >pci_dev->bmdma[0]) ? 0 : 1;
>> +
>> +trace_sii3112_bmdma_write(size, addr, val);
>> +switch (addr) {
>> +case 0x00:
>> +bmdma_cmd_writeb(bm, val);
>>   break;
>> -case 0x0a:
>> -val = d->i.bmdma[1].status;
>> +case 0x01:
>> +d->regs[i].swdata = val & 0x3f;
>>   break;
>> -case 0x0b:
>> -val = 0;
>> +case 0x02:
>> +bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 
>> 6);
>>   break;
>> -case 0x0c ... 0x0f:
>> -val = bmdma_addr_ioport_ops.read(>i.bmdma[1], addr - 12, size);
>> +default:
>>   break;
>> +}
>> +}
>> +
>> +static const MemoryRegionOps sii3112_bmdma_ops = {
>> +.read = sii3112_bmdma_read,
>> +.write = sii3112_bmdma_write,
>> +};
>> +
>> +/* The sii3112_reg_read and sii3112_reg_write functions implement the
>> + * Internal Register Space - BAR5 (section 6.7 of the data sheet).
>> + */
>> +
>> +static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
>> +unsigned int size)
>> +{
>> +SiI3112PCIState *d = opaque;
>> +uint64_t val;
>> +
>> +switch (addr) {
>>   case 0x10:
>>   val = d->i.bmdma[0].cmd;
>>   val |= (d->regs[0].confstat & (1UL << 11) ? (1 << 4) : 0); 
>> /*SATAINT0*/
>> @@ -127,38 +153,26 @@ static void sii3112_reg_write(void *opaque, hwaddr 
>> addr,
>> trace_sii3112_write(size, 

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

[Bernhard Beschow]

Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
>> standard-compliant PCI IDE device.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   include/hw/ide/pci.h |  2 --
>>   hw/ide/pci.c |  4 ++--
>>   hw/ide/sii3112.c | 50 
>>   3 files changed, 20 insertions(+), 36 deletions(-)
>> 
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index 5025df5b82..dbb4b13161 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
>>   extern MemoryRegionOps bmdma_addr_ioport_ops;
>>   void pci_ide_create_devs(PCIDevice *dev);
>>   -extern const MemoryRegionOps pci_ide_cmd_le_ops;
>> -extern const MemoryRegionOps pci_ide_data_le_ops;
>>   #endif
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index b2fcc00a64..97ccc75aa6 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
>>   ide_ctrl_write(bus, addr + 2, data);
>>   }
>>   -const MemoryRegionOps pci_ide_cmd_le_ops = {
>> +static const MemoryRegionOps pci_ide_cmd_le_ops = {
>>   .read = pci_ide_status_read,
>>   .write = pci_ide_ctrl_write,
>>   .endianness = DEVICE_LITTLE_ENDIAN,
>> @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
>>   }
>>   }
>>   -const MemoryRegionOps pci_ide_data_le_ops = {
>> +static const MemoryRegionOps pci_ide_data_le_ops = {
>>   .read = pci_ide_data_read,
>>   .write = pci_ide_data_write,
>>   .endianness = DEVICE_LITTLE_ENDIAN,
>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>> index 0af897a9ef..9cf920369f 100644
>> --- a/hw/ide/sii3112.c
>> +++ b/hw/ide/sii3112.c
>> @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr 
>> addr,
>>   val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
>>   val |= (uint32_t)d->i.bmdma[1].status << 16;
>>   break;
>> -case 0x80 ... 0x87:
>> -val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
>> -break;
>> -case 0x8a:
>> -val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
>> -break;
>>   case 0xa0:
>>   val = d->regs[0].confstat;
>>   break;
>> -case 0xc0 ... 0xc7:
>> -val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
>> -break;
>> -case 0xca:
>> -val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
>> -break;
>>   case 0xe0:
>>   val = d->regs[1].confstat;
>>   break;
>> @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
>>   case 0x0c ... 0x0f:
>>   bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
>>   break;
>> -case 0x80 ... 0x87:
>> -pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
>> -break;
>> -case 0x8a:
>> -pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
>> -break;
>> -case 0xc0 ... 0xc7:
>> -pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
>> -break;
>> -case 0xca:
>> -pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
>> -break;
>>   case 0x100:
>>   d->regs[0].scontrol = val & 0xfff;
>>   if (val & 1) {
>> @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>>   pci_config_set_interrupt_pin(dev->config, 1);
>>   pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
>>   +pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
>> +pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
>> +pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
>> +pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
>> +
>>   /* BAR5 is in PCI memory space */
>>   memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
>>"sii3112.bar5", 0x200);
>> @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>> /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */
>>   mr = g_new(MemoryRegion, 1);
>> -memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 0x80, 
>> 8);
>> -pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
>> +memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", 
>> >data_ops[0], 0,
>> + memory_region_size(>data_ops[0]));
>> +memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
>>   mr = g_new(MemoryRegion, 1);
>> -memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 0x88, 
>> 4);
>> -pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
>> +memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >cmd_ops[0], 
>> 0,
>> +   

Re: [PATCH 10/13] hw/ide/piix: Reuse PCIIDEState::{cmd,data}_ops

[Bernhard Beschow]

Am 26. April 2023 18:18:35 UTC schrieb Bernhard Beschow :
>
>
>Am 26. April 2023 11:37:48 UTC schrieb Mark Cave-Ayland 
>:
>>On 22/04/2023 16:07, Bernhard Beschow wrote:
>>
>>> Now that PCIIDEState::{cmd,data}_ops are initialized in the base class
>>> constructor there is an opportunity for PIIX to reuse these attributes. This
>>> resolves usage of ide_init_ioport() which would fall back internally to 
>>> using
>>> the isabus global due to NULL being passed as ISADevice by PIIX.
>>> 
>>> Signed-off-by: Bernhard Beschow 
>>> ---
>>>   hw/ide/piix.c | 30 +-
>>>   1 file changed, 13 insertions(+), 17 deletions(-)
>>> 
>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>> index a3a15dc7db..406a67fa0f 100644
>>> --- a/hw/ide/piix.c
>>> +++ b/hw/ide/piix.c
>>> @@ -104,34 +104,32 @@ static void piix_ide_reset(DeviceState *dev)
>>>   pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
>>>   }
>>>   -static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus 
>>> *isa_bus,
>>> -  Error **errp)
>>> +static void pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus)
>>>   {
>>>   static const struct {
>>>   int iobase;
>>>   int iobase2;
>>>   int isairq;
>>>   } port_info[] = {
>>> -{0x1f0, 0x3f6, 14},
>>> -{0x170, 0x376, 15},
>>> +{0x1f0, 0x3f4, 14},
>>> +{0x170, 0x374, 15},
>>>   };
>>> -int ret;
>>> +MemoryRegion *address_space_io = pci_address_space_io(PCI_DEVICE(d));
>>> ide_bus_init(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>> -ret = ide_init_ioport(>bus[i], NULL, port_info[i].iobase,
>>> -  port_info[i].iobase2);
>>> -if (ret) {
>>> -error_setg_errno(errp, -ret, "Failed to realize %s port %u",
>>> - object_get_typename(OBJECT(d)), i);
>>> -return false;
>>> -}
>>> +memory_region_add_subregion(address_space_io, port_info[i].iobase,
>>> +>data_ops[i]);
>>> +/*
>>> + * PIIX forwards the last byte of cmd_ops to ISA. Model this using a 
>>> low
>>> + * prio so competing memory regions take precedence.
>>> + */
>>> +memory_region_add_subregion_overlap(address_space_io, 
>>> port_info[i].iobase2,
>>> +>cmd_ops[i], -1);
>>
>>Interesting. Is this behaviour documented somewhere and/or used in one of 
>>your test images at all? If I'd have seen this myself, I probably thought 
>>that the addresses were a typo...
>
>I first  stumbled upon this and wondered why this code was working with 
>VIA_IDE (through my pc-via branch). Then I found the correct offsets there 
>which are confirmed in the piix datasheet, e.g.: "Secondary Control Block 
>Offset: 0374h"

In case you were wondering about the forwarding of the last byte the datasheet 
says: "Accesses to byte 3 of the Control Block are forwarded to ISA where the 
floppy disk controller responds."

>
>>
>>>   ide_bus_init_output_irq(>bus[i],
>>>   isa_bus_get_irq(isa_bus, 
>>> port_info[i].isairq));
>>> bmdma_init(>bus[i], >bmdma[i], d);
>>>   ide_bus_register_restart_cb(>bus[i]);
>>> -
>>> -return true;
>>>   }
>>> static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>> @@ -160,9 +158,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
>>> **errp)
>>>   }
>>> for (unsigned i = 0; i < 2; i++) {
>>> -if (!pci_piix_init_bus(d, i, isa_bus, errp)) {
>>> -return;
>>> -}
>>> +pci_piix_init_bus(d, i, isa_bus);
>>>   }
>>>   }
>>>   
>>
>>
>>ATB,
>>
>>Mark.

[PATCH v5 2/2] migration: Make dirty_bytes_last_sync atomic

[Juan Quintela]

As we set its value, it needs to be operated with atomics.
We rename it from remaining to better reflect its meaning.

Statistics always return the real reamaining bytes.  This was used to
store how much pages where dirty on the previous generation, so we can
calculate the expected downtime as: dirty_bytes_last_sync /
current_bandwith.

If we use the actual remaining bytes, we would see a very small value
at the end of the iteration.

Signed-off-by: Juan Quintela 
Reviewed-by: Peter Xu 

---

I am open to use ram_bytes_remaining() in its only use and be more
"optimistic" about the downtime.

Don't use __nocheck() functions.
---
 migration/migration.c | 3 ++-
 migration/ram.c   | 2 +-
 migration/ram.h   | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 712f802962..c81c65bf28 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2754,7 +2754,8 @@ static void migration_update_counters(MigrationState *s,
  */
 if (qatomic_read(_counters.dirty_pages_rate) &&
 transferred > 1) {
-s->expected_downtime = ram_counters.remaining / bandwidth;
+s->expected_downtime =
+qatomic_read(_counters.dirty_bytes_last_sync) / bandwidth;
 }
 
 qemu_file_reset_rate_limit(s->to_dst_file);
diff --git a/migration/ram.c b/migration/ram.c
index 7c534a41e0..704df661d1 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1223,7 +1223,7 @@ static void migration_bitmap_sync(RAMState *rs)
 RAMBLOCK_FOREACH_NOT_IGNORED(block) {
 ramblock_sync_dirty_bitmap(rs, block);
 }
-ram_counters.remaining = ram_bytes_remaining();
+qatomic_set(_counters.dirty_bytes_last_sync, 
ram_bytes_remaining());
 }
 qemu_mutex_unlock(>bitmap_mutex);
 
diff --git a/migration/ram.h b/migration/ram.h
index 3db0a9d65c..11a0fde99b 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -41,6 +41,7 @@
  * one thread).
  */
 typedef struct {
+aligned_uint64_t dirty_bytes_last_sync;
 aligned_uint64_t dirty_pages_rate;
 Stat64 dirty_sync_count;
 Stat64 dirty_sync_missed_zero_copy;
@@ -51,7 +52,6 @@ typedef struct {
 Stat64 postcopy_bytes;
 Stat64 postcopy_requests;
 Stat64 precopy_bytes;
-int64_t remaining;
 Stat64 transferred;
 } RAMStats;
 
-- 
2.40.0

[PATCH v5 0/2] Migration: Make more ram_counters atomic

[Juan Quintela]

Hi

In this v5:

Not only change the type of the counters, also use the __nocheck()
variants of the functions.

Please, review.

[v4]
- Change aligned_uint64_t to size_t to make (some) 32bit hosts happy.

Please review.

[v3]
- Addressed reviews
- All counters are now atomic, either Stat64 or atomic.
- Rename duplicated to zero_pages
- Rename normal to zero_pages.

Please review.

[v2]
- fix typos found by David Edmondson
- Add review-by tags.

Please review.

[v1]
On previous series we cerate ram_atomic_counters.  But we basically
need that all counters are atomic.  So move back to only have
ram_counters, just with a new type that allows the atomic counters.

Once there, move update of stats out of RAM mutex.
And make multifd_bytes atomic.

Later, Juan.

Juan Quintela (2):
  migration: Make dirty_pages_rate atomic
  migration: Make dirty_bytes_last_sync atomic

 migration/migration.c | 9 ++---
 migration/ram.c   | 7 ---
 migration/ram.h   | 4 ++--
 3 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.40.0

[PATCH v5 1/2] migration: Make dirty_pages_rate atomic

[Juan Quintela]

In this case we use qatomic operations instead of Stat64 wrapper
because there is no stat64_set().  Defining the 64 bit wrapper is
trivial. The one without atomics is more interesting.

Signed-off-by: Juan Quintela 
Reviewed-by: Peter Xu 

---

Don't use __nocheck() variants
---
 migration/migration.c | 6 --
 migration/ram.c   | 5 +++--
 migration/ram.h   | 2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 22e8586623..712f802962 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1005,7 +1005,8 @@ static void populate_ram_info(MigrationInfo *info, 
MigrationState *s)
 
 if (s->state != MIGRATION_STATUS_COMPLETED) {
 info->ram->remaining = ram_bytes_remaining();
-info->ram->dirty_pages_rate = ram_counters.dirty_pages_rate;
+info->ram->dirty_pages_rate =
+   qatomic_read(_counters.dirty_pages_rate);
 }
 }
 
@@ -2751,7 +2752,8 @@ static void migration_update_counters(MigrationState *s,
  * if we haven't sent anything, we don't want to
  * recalculate. 1 is a small enough number for our purposes
  */
-if (ram_counters.dirty_pages_rate && transferred > 1) {
+if (qatomic_read(_counters.dirty_pages_rate) &&
+transferred > 1) {
 s->expected_downtime = ram_counters.remaining / bandwidth;
 }
 
diff --git a/migration/ram.c b/migration/ram.c
index 01356f60a4..7c534a41e0 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1129,8 +1129,9 @@ static void migration_update_rates(RAMState *rs, int64_t 
end_time)
 double compressed_size;
 
 /* calculate period counters */
-ram_counters.dirty_pages_rate = rs->num_dirty_pages_period * 1000
-/ (end_time - rs->time_last_bitmap_sync);
+qatomic_set(_counters.dirty_pages_rate,
+rs->num_dirty_pages_period * 1000 /
+(end_time - rs->time_last_bitmap_sync));
 
 if (!page_count) {
 return;
diff --git a/migration/ram.h b/migration/ram.h
index a6e0d70226..3db0a9d65c 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -41,7 +41,7 @@
  * one thread).
  */
 typedef struct {
-int64_t dirty_pages_rate;
+aligned_uint64_t dirty_pages_rate;
 Stat64 dirty_sync_count;
 Stat64 dirty_sync_missed_zero_copy;
 Stat64 downtime_bytes;
-- 
2.40.0

Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration

[Vladimir Sementsov-Ogievskiy]

On 26.04.23 07:43, Michael S. Tsirkin wrote:

On Tue, Apr 25, 2023 at 07:14:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:

On incoming migration we have the following sequence to load option
ROM:

1. On device realize we do normal load ROM from the file

2. Than, on incoming migration we rewrite ROM from the incoming RAM
block. If sizes mismatch we fail.


let's mention an example error message:
  Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8: Invalid 
argument




This is not ideal when we migrate to updated distribution: we have to
keep old ROM files in new distribution and be careful around romfile
property to load correct ROM file.



Which is loaded actually just to
allocate the ROM with correct length.
Note, that romsize property doesn't really help: if we try to specify
it when default romfile is larger, it fails with something like:

romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536


Something I'd like to clarify is that the comment applies to uses where
users/distributions supply their own ROM file.  And lots of
users/distributions seem to have already painted themselves into a
corner by supplying a mix of ROM files of unmatching sizes -
basically they don't understand the detail of live migration,
ROM size interaction with it and with memory layout, etc -
as a very small number of people does.
For example, ubuntu doubled ROM file size by padding their ROMs
with 0x at some point, breaking migration for all existing machine
types.

just a web search for
  Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8: Invalid 
argument

will turn up a bunch of confused distros and users.




Let's just ignore ROM file when romsize is specified and we are in
incoming migration state. In other words, we need only to preallocate
ROM of specified size, local ROM file is unrelated.







This way:

If romsize was specified on source, we just use same commandline as on
source, and migration will work independently of local ROM files on
target.

If romsize was not specified on source (and we have mismatching local
ROM file on target host), we have to specify romsize on target to match
source romsize. romfile parameter may be kept same as on source or may
be dropped, the file is not loaded anyway.

As a bonus we avoid extra reading from ROM file on target.

Note: when we don't have romsize parameter on source command line and
need it for target, it may be calculated as aligned up to power of two
size of ROM file on source (if we know, which file is it) or,
alternatively it may be retrieved from source QEMU by QMP qom-get
command, like

   { "execute": "qom-get",
 "arguments": {
   "path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
   "property": "size" } }

Suggested-by: Michael S. Tsirkin 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  hw/pci/pci.c | 77 ++--
  1 file changed, 45 insertions(+), 32 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a442f8fce1..e2cab622e4 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -36,6 +36,7 @@
  #include "migration/vmstate.h"
  #include "net/net.h"
  #include "sysemu/numa.h"
+#include "sysemu/runstate.h"
  #include "sysemu/sysemu.h"
  #include "hw/loader.h"
  #include "qemu/error-report.h"
@@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool 
is_default_rom,
  {
  int64_t size;
  g_autofree char *path = NULL;
-void *ptr;
  char name[32];
  const VMStateDescription *vmsd;
  
+/*

+ * In case of incoming migration ROM will come with migration stream, no
+ * reason to load the file.  Neither we want to fail if local ROM file
+ * mismatches with specified romsize.
+ */
+bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
+
  if (!pdev->romfile) {
  return;
  }


CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check like
this the right way to figure out we are not going to use the
device locally before incoming migration will overwrite ROM contents?


RUN_STATE_INMIGRATE is set in the only one place in qemu_init() when we parse 
cmdline option -incoming. VM is not running for sure. And starting the VM comes 
with changing the state. So it's OK.

The possible problem, if we add netcard on target which we didn't have on 
source. I now checked, this works.. But that doesn't seem correct to add device 
that was not present on source - how would it work - it's not guaranteed anyway.




@@ -2329,32 +2336,35 @@ static void pci_add_option_rom(PCIDevice *pdev, bool 
is_default_rom,
  return;
  }
  
-path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);

-if (path == NULL) {
-path = g_strdup(pdev->romfile);
-}
+if (load_file || pdev->romsize == -1) {
+path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
+if (path == NULL) {
+path = g_strdup(pdev->romfile);
+ 

Re: [PATCH v4 0/2] Migration: Make more ram_counters atomic

[Juan Quintela]

Juan Quintela  wrote:
> Hi
>
> In this v4:
> - Change aligned_uint64_t to size_t to make (some) 32bit hosts happy.
>
> Please review.

self-NACK

Still missing the removal of the __nocheck() functions.

>
> [v3]
> - Addressed reviews
> - All counters are now atomic, either Stat64 or atomic.
> - Rename duplicated to zero_pages
> - Rename normal to zero_pages.
>
> Please review.
>
> [v2]
> - fix typos found by David Edmondson
> - Add review-by tags.
>
> Please review.
>
> [v1]
> On previous series we cerate ram_atomic_counters.  But we basically
> need that all counters are atomic.  So move back to only have
> ram_counters, just with a new type that allows the atomic counters.
>
> Once there, move update of stats out of RAM mutex.
> And make multifd_bytes atomic.
>
> Later, Juan.
>
> Juan Quintela (2):
>   migration: Make dirty_pages_rate atomic
>   migration: Make dirty_bytes_last_sync atomic
>
>  migration/migration.c | 10 +++---
>  migration/ram.c   |  8 +---
>  migration/ram.h   |  4 ++--
>  3 files changed, 14 insertions(+), 8 deletions(-)

Re: [PATCH v9 0/3] Eliminate multifd flush

[Peter Xu]

On Wed, Apr 26, 2023 at 08:18:58PM +0200, Juan Quintela wrote:
> Juan Quintela (3):
>   multifd: Create property multifd-flush-after-each-section
>   multifd: Protect multifd_send_sync_main() calls
>   multifd: Only flush once each full round of memory

Acked-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH v3 09/13] migration: Create migrate_tls_creds() function

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> On 24.04.23 21:32, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela 
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

> could be stricter "const char *"

I change changed the patch just to reflect this.

>
>> @@ -34,20 +34,19 @@ migration_tls_get_creds(MigrationState *s,
>>   Error **errp)
>
> "s" argument becomes unused, may be dropped.

Good catch!

I created the patches for this, will send after I send the PULL request.

Thanks, Juan.

Re: [PATCH 01/13] hw/ide/pci: Expose legacy interrupts as GPIOs

[Bernhard Beschow]

Am 26. April 2023 10:41:30 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> Exposing the legacy IDE interrupts as GPIOs allows them to be connected in 
>> the
>> parent device through qdev_connect_gpio_out(), i.e. without accessing private
>> data of TYPE_PCI_IDE.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   hw/ide/pci.c | 8 
>>   1 file changed, 8 insertions(+)
>> 
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index fc9224bbc9..942e216b9b 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -522,10 +522,18 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, 
>> PCIIDEState *d)
>>   bm->pci_dev = d;
>>   }
>>   +static void pci_ide_init(Object *obj)
>> +{
>> +PCIIDEState *d = PCI_IDE(obj);
>> +
>> +qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq));
>
>Just one minor nit: can we make this qdev_init_gpio_out_named() and call it 
>"isa-irq" to match? This is for 2 reasons: firstly these are PCI devices and 
>so an unnamed IRQ/gpio could be considered to belong to PCI, and secondly it 
>gives the gpio the same name as the struct field.

Yes, makes sense.

>
>From my previous email I think this should supercede Phil's patch at 
>https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-2-phi...@linaro.org/.
>
>> +}
>> +
>>   static const TypeInfo pci_ide_type_info = {
>>   .name = TYPE_PCI_IDE,
>>   .parent = TYPE_PCI_DEVICE,
>>   .instance_size = sizeof(PCIIDEState),
>> +.instance_init = pci_ide_init,
>>   .abstract = true,
>>   .interfaces = (InterfaceInfo[]) {
>>   { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>
>Otherwise:
>
>Reviewed-by: Mark Cave-Ayland 
>
>
>ATB,
>
>Mark.

Re: [RFC PATCH v3 00/20] configure: create a python venv and ensure meson, sphinx

[Paolo Bonzini]

On 4/26/23 18:32, John Snow wrote:


mkvenv: Creating {isolated|non-isolated} virtual environment [based on
/home/pbonzini/myvenv]

"based on ..." for nested venv case only?


Yes.



and when creating the console-scripts shims:

mkvenv: Found avocado v90.0

Sure. Up to the user at that point to figure out if we used that package 
or not.


e.g. "found meson 0.5.0" might occur even if we require >=1.0.


John and I discussed offlist and he'll try to remove --gen so that all 
system packages handling is done at "mkvenv ensure" time.  The "Found" 
message can then be printed at "mkvenv ensure" time too.


Thanks,

Paolo


Simple to implement, though.

Re: [PATCH v11 00/12] parallels: Refactor the code of images checks and fix a bug

[Alexander Ivanov]

The patchests are the same, but the first one has incorrect receivers. 
Please just ignore it.
I've sent a email about it, but i mistook twice and sent it to 
qemu-devel@nongnu.org only.



On 4/26/23 17:07, Denis V. Lunev wrote:

On 4/24/23 11:31, Alexander Ivanov wrote:

Fix image inflation when offset in BAT is out of image.

Replace whole BAT syncing by flushing only dirty blocks.

Move all the checks outside the main check function in
separate functions

Use WITH_QEMU_LOCK_GUARD for simplier code.

Fix incorrect condition in out-of-image check.

v11:
1: Use bdrv_nb_sectors() instead of bdrv_getlength() to get image 
size in sectors.

7,9: Add coroutine_fn and GRAPH_RDLOCK annotations.

v10:
8: Add a comment.
9: Exclude unrelated changes.

v9:
3: Add (high_off == 0) case handling.
7: Move res->image_end_offset setting to 
parallels_check_outside_image().

8: Add a patch with a statistics calculation fix.
9: Remove redundant high_off calculation.
12: Change the condition to (off + s->cluster_size > size).

v8: Rebase on the top of the current master branch.

v7:
1,2: Fix string lengths in the commit messages.
3: Fix a typo in the commit message.

v6:
1: Move the error check inside the loop. Move file size getting
    to the function beginning. Skip out-of-image offsets.
2: A new patch - don't let high_off be more than the end of the last 
cluster.

3: Set data_end without any condition.
7: Move data_end setting to parallels_check_outside_image().
8: Remove s->data_end setting from parallels_check_leak().
    Fix 'i' type.

v5:
2: Change the way of data_end fixing.
6,7: Move data_end check to parallels_check_leak().

v4:
1: Move s->data_end fix to parallels_co_check(). Split the check
    in parallels_open() and the fix in parallels_co_check() to two 
patches.

2: A new patch - a part of the patch 1.
    Add a fix for data_end to parallels_co_check().
3: Move offset convertation to parallels_set_bat_entry().
4: Fix 'ret' rewriting by bdrv_co_flush() results.
7: Keep 'i' as uint32_t.

v3:

1-8: Fix commit message.

v2:

2: A new patch - a part of the splitted patch 2.
3: Patch order was changed so the replacement is done in 
parallels_co_check.

    Now we use a helper to set BAT entry and mark the block dirty.
4: Revert the condition with s->header_unclean.
5: Move unrelated helper parallels_set_bat_entry creation to a 
separate patch.

7: Move fragmentation counting code to this function too.
8: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.

Alexander Ivanov (12):
   parallels: Out of image offset in BAT leads to image inflation
   parallels: Fix high_off calculation in parallels_co_check()
   parallels: Fix image_end_offset and data_end after out-of-image check
   parallels: create parallels_set_bat_entry_helper() to assign BAT 
value

   parallels: Use generic infrastructure for BAT writing in
 parallels_co_check()
   parallels: Move check of unclean image to a separate function
   parallels: Move check of cluster outside image to a separate function
   parallels: Fix statistics calculation
   parallels: Move check of leaks to a separate function
   parallels: Move statistic collection to a separate function
   parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
   parallels: Incorrect condition in out-of-image check

  block/parallels.c | 190 +-
  1 file changed, 136 insertions(+), 54 deletions(-)


I am a little bit puzzled - there are 2 series v11 sent within
15 minutes. Which one is correct?

Den

Re: [PATCH 1/2] Fix libvhost-user.c compilation.

[Paolo Bonzini]

On 4/7/23 09:56, Michael S. Tsirkin wrote:

On Wed, Apr 05, 2023 at 02:59:19PM +0200, David 'Digit' Turner wrote:

The source file uses VIRTIO_F_VERSION_1 which is
not defined by  on Debian 10.

The system-provided  which
does not include the macro definition is included
through , so fix the issue by including
the standard-headers version before that.

Signed-off-by: David 'Digit' Turner 


I don't get it. ./linux-headers/linux/vhost.h does not seem
to use  for me.


The issue is that subprojects/libvhost-user/libvhost-user.c includes 
linux/vhost.h.  Probably should be changed to 
linux-headers/linux/vhost.h, but David's patch makes sense because 
libvhost-user.c does use a symbol from virtio_config.h.


Paolo


$ git grep linux/virtio_config.h
include/hw/virtio/virtio.h:#include "standard-headers/linux/virtio_config.h"
include/standard-headers/linux/vhost_types.h:#include 
"standard-headers/linux/virtio_config.h"
include/standard-headers/linux/virtio_9p.h:#include 
"standard-headers/linux/virtio_config.h"
include/standard-headers/linux/virtio_balloon.h:#include 
"standard-headers/linux/virtio_config.h"
include/standard-headers/linux/virtio_blk.h:#include 
"standard-headers/linux/virtio_config.h"
include/standard-headers/linux/virtio_console.h:#include 
"standard-headers/linux/virtio_config.h"
include/standard-headers/linux/virtio_crypto.h:#include 
"standard-headers/linux/virtio_config.h"
include/standard-headers/linux/virtio_fs.h:#include 
"standard-headers/linux/virtio_config.h"
include/standard-headers/linux/virtio_mem.h:#include 
"standard-headers/linux/virtio_config.h"
include/standard-headers/linux/virtio_net.h:#include 
"standard-headers/linux/virtio_config.h"
include/standard-headers/linux/virtio_pmem.h:#include 
"standard-headers/linux/virtio_config.h"
include/standard-headers/linux/virtio_rng.h:#include 
"standard-headers/linux/virtio_config.h"
include/standard-headers/linux/virtio_vsock.h:#include 
"standard-headers/linux/virtio_config.h"
linux-headers/linux/virtio_config.h:#include 
"standard-headers/linux/virtio_config.h"
scripts/update-linux-headers.sh:cat <$output/linux-headers/linux/virtio_config.h
scripts/update-linux-headers.sh:#include 
"standard-headers/linux/virtio_config.h"
subprojects/libvduse/libvduse.c:#include "linux-headers/linux/virtio_config.h"
tests/qtest/fuzz/virtio_net_fuzz.c:#include 
"standard-headers/linux/virtio_config.h"
tests/qtest/libqos/virtio-gpio.c:#include 
"standard-headers/linux/virtio_config.h"
tests/qtest/libqos/virtio-pci-modern.c:#include 
"standard-headers/linux/virtio_config.h"
tests/qtest/libqos/virtio.c:#include "standard-headers/linux/virtio_config.h"





---
  subprojects/libvhost-user/libvhost-user.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index 0200b78e8e..0a5768cb55 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -32,6 +32,12 @@
  #include 
  #include 
  
+/* Necessary to provide VIRTIO_F_VERSION_1 on system

+ * with older linux headers. Must appear before
+ *  below.
+ */
+#include "standard-headers/linux/virtio_config.h"
+
  #if defined(__linux__)
  #include 
  #include 
--
2.40.0.348.gf938b09366-goog

Re: [PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons

[Peter Xu]

On Wed, Apr 26, 2023 at 08:54:02PM +0200, Juan Quintela wrote:
> This is why I mean that I want the "diff" to be a bit more intelligent
> and "record" the things that we tell them that are correct.

I think I see what you meant. :)

> I will start with the default machine devices.
> Once the mechanism is done, we can wonder with more configurations.
> 
> I will start small and then go from there.

Sounds good!

-- 
Peter Xu

Re: [PATCH v3 13/13] migration: Move migration_properties to options.c

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 0/2] Fix QEMU compilation on Debian 10

[Paolo Bonzini]

Queued, thanks.

Paolo

Re: [RFC PATCH v3 00/20] configure: create a python venv and ensure meson, sphinx

[Paolo Bonzini]

On 4/26/23 18:16, John Snow wrote:


 > - I am ambivalent about keeping --enable/--disable-pypi in the first
 > committed patchset, but in any case I would move patches 16 and 20
 > before patch 15

I might be stubborn but I think I want to keep it in for now. If it 
needs redesigned to fit with the other flags you want to add, I think 
that's OK.


if we vendor the whl directly in qemu.git we won't need PyPI for meson, 
but it's still useful for Sphinx so I think I'm still leaning towards 
keeping it.


Yes, it's definitely useful.  It's just that unifying $python with 
sphinx-build's interpreter is as a separate (and earlier) step than 
introducing PyPI.



I'll try to refactor to keep it at the tail end of the series.


Cool, thanks.


Just one extra thing, since we're changing so much of Python handling
and since the code is written, I would keep the Debian 10 workarounds
for now, and only drop them after we drop support for 3.6.


This series was written assuming we get to drop 3.6 as a prereq. Is that 
not the case?


Or did you mean to write 3.7 there?


Yes, 3.7.

Paolo

Re: [PATCH v3 08/13] migration: Remove MigrationState from block_cleanup_parameters()

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> On 24.04.23 21:32, Juan Quintela wrote:
>> This makes the function more regular with everything else.
>> Signed-off-by: Juan Quintela 
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Thanks.

>> ---
>>   migration/migration.c | 4 ++--
>>   migration/options.c   | 4 +++-
>>   migration/options.h   | 2 +-
>>   3 files changed, 6 insertions(+), 4 deletions(-)
>> diff --git a/migration/migration.c b/migration/migration.c
>> index cefe6da2b8..ef8caa79b9 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1218,7 +1218,7 @@ static void migrate_fd_cleanup(MigrationState *s)
>>   error_report_err(error_copy(s->error));
>>   }
>>   notifier_list_notify(_state_notifiers, s);
>> -block_cleanup_parameters(s);
>> +block_cleanup_parameters();
>>   yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>>   }
>>   @@ -1712,7 +1712,7 @@ void qmp_migrate(const char *uri, bool
>> has_blk, bool blk,
>>  "a valid migration protocol");
>>   migrate_set_state(>state, MIGRATION_STATUS_SETUP,
>> MIGRATION_STATUS_FAILED);
>> -block_cleanup_parameters(s);
>> +block_cleanup_parameters();
>>   return;
>>   }
>>   diff --git a/migration/options.c b/migration/options.c
>> index 26fe00799b..f65b7babef 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -597,8 +597,10 @@ void migrate_set_block_incremental(bool value)
>> /* parameters helpers */
>>   -void block_cleanup_parameters(MigrationState *s)
>> +void block_cleanup_parameters(void)
>>   {
>> +MigrationState *s = migrate_get_current();
>> +
>>   if (s->must_remove_block_options) {
>>   /* setting to false can never fail */
>>   migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, false, _abort);
>> diff --git a/migration/options.h b/migration/options.h
>> index 1fc8d341dd..3948218dbe 100644
>> --- a/migration/options.h
>> +++ b/migration/options.h
>> @@ -90,6 +90,6 @@ void migrate_set_block_incremental(bool value);
>> bool migrate_params_check(MigrationParameters *params, Error
>> **errp);
>>   void migrate_params_init(MigrationParameters *params);
>> -void block_cleanup_parameters(MigrationState *s);
>> +void block_cleanup_parameters(void);
>
> Don't you want to rename it to migrate_* ?

The idea is to deprecate block migration.  There are much better things
on the block layer to migrate disks.  So I think we can let it from now
there.


Later, Juan.

Re: [PATCH v3 06/13] migration: Move migrate_set_block_incremental() to options.c

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> On 24.04.23 21:32, Juan Quintela wrote:
>> Once there, make it more regular and remove th eneed for
>
> some type here
Thanks, fixed.


>
>> MigrationState parameter.
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Re: [PATCH v3 12/13] migration: Create migrate_block_bitmap_mapping() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Notice that we changed the test of ->has_block_bitmap_mapping
for the test that block_bitmap_mapping is not NULL.

Signed-off-by: Juan Quintela 


Reviewed-by: Vladimir Sementsov-Ogievskiy 


---
  migration/block-dirty-bitmap.c | 14 --
  migration/options.c|  7 +++
  migration/options.h|  1 +
  3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index a6ffae0002..62b2352bbb 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -605,11 +605,12 @@ static int init_dirty_bitmap_migration(DBMSaveState *s)
  SaveBitmapState *dbms;
  GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
  BlockBackend *blk;
-const MigrationParameters *mig_params = _get_current()->parameters;
  GHashTable *alias_map = NULL;
+BitmapMigrationNodeAliasList *block_bitmap_mapping =
+migrate_block_bitmap_mapping();
  
-if (mig_params->has_block_bitmap_mapping) {

-alias_map = construct_alias_map(mig_params->block_bitmap_mapping, true,
+if (block_bitmap_mapping) {
+alias_map = construct_alias_map(block_bitmap_mapping, true,
  _abort);
  }
  
@@ -1158,7 +1159,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,

  static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
  {
  GHashTable *alias_map = NULL;
-const MigrationParameters *mig_params = _get_current()->parameters;
+BitmapMigrationNodeAliasList *block_bitmap_mapping =
+migrate_block_bitmap_mapping();
  DBMLoadState *s = &((DBMState *)opaque)->load;
  int ret = 0;
  
@@ -1170,8 +1172,8 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)

  return -EINVAL;
  }
  
-if (mig_params->has_block_bitmap_mapping) {

-alias_map = construct_alias_map(mig_params->block_bitmap_mapping,
+if (block_bitmap_mapping) {
+alias_map = construct_alias_map(block_bitmap_mapping,
  false, _abort);
  }
  
diff --git a/migration/options.c b/migration/options.c

index 9fbba84b9a..ec234bf3ff 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -452,6 +452,13 @@ void 
qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
  
  /* parameters */
  
+BitmapMigrationNodeAliasList *migrate_block_bitmap_mapping(void)


as well, this could return constant pointer. Even construct_alias_map() is 
already prepared for this.


+{
+MigrationState *s = migrate_get_current();
+
+return s->parameters.block_bitmap_mapping;
+}
+
  bool migrate_block_incremental(void)
  {
  MigrationState *s = migrate_get_current();
diff --git a/migration/options.h b/migration/options.h
index 9123fdb5f4..43e8e9cd8f 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -62,6 +62,7 @@ bool migrate_cap_set(int cap, bool value, Error **errp);
  
  /* parameters */
  
+BitmapMigrationNodeAliasList *migrate_block_bitmap_mapping(void);

  bool migrate_block_incremental(void);
  uint32_t migrate_checkpoint_delay(void);
  int migrate_compress_level(void);


--
Best regards,
Vladimir

Re: [PATCH v3 11/13] migration: Create migrate_tls_hostname() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela



Reviewed-by: Vladimir Sementsov-Ogievskiy 

[same recommendations]

--
Best regards,
Vladimir

Re: [PATCH v3 10/13] migration: Create migrate_tls_authz() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela


Reviewed-by: Vladimir Sementsov-Ogievskiy 

Still same recommendations: "const char *" and "s" becomes unused in 
migration_tls_channel_process_incoming()

--
Best regards,
Vladimir

Re: [PATCH v3 09/13] migration: Create migrate_tls_creds() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela 


Reviewed-by: Vladimir Sementsov-Ogievskiy 



---
  migration/options.c | 7 +++
  migration/options.h | 1 +
  migration/tls.c | 9 -
  3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index f65b7babef..9eabb4c25d 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -579,6 +579,13 @@ uint8_t migrate_throttle_trigger_threshold(void)
  return s->parameters.throttle_trigger_threshold;
  }
  
+char *migrate_tls_creds(void)


could be stricter "const char *"


+{
+MigrationState *s = migrate_get_current();
+
+return s->parameters.tls_creds;
+}
+
  uint64_t migrate_xbzrle_cache_size(void)
  {
  MigrationState *s = migrate_get_current();
diff --git a/migration/options.h b/migration/options.h
index 3948218dbe..47cc24585b 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -80,6 +80,7 @@ MultiFDCompression migrate_multifd_compression(void);
  int migrate_multifd_zlib_level(void);
  int migrate_multifd_zstd_level(void);
  uint8_t migrate_throttle_trigger_threshold(void);
+char *migrate_tls_creds(void);
  uint64_t migrate_xbzrle_cache_size(void);
  
  /* parameters setters */

diff --git a/migration/tls.c b/migration/tls.c
index acd38e0b62..0d318516de 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -34,20 +34,19 @@ migration_tls_get_creds(MigrationState *s,
  Error **errp)


"s" argument becomes unused, may be dropped.


  {
  Object *creds;
+char *tls_creds = migrate_tls_creds();
  QCryptoTLSCreds *ret;
  
-creds = object_resolve_path_component(

-object_get_objects_root(), s->parameters.tls_creds);
+creds = object_resolve_path_component(object_get_objects_root(), 
tls_creds);
  if (!creds) {
-error_setg(errp, "No TLS credentials with id '%s'",
-   s->parameters.tls_creds);
+error_setg(errp, "No TLS credentials with id '%s'", tls_creds);
  return NULL;
  }
  ret = (QCryptoTLSCreds *)object_dynamic_cast(
  creds, TYPE_QCRYPTO_TLS_CREDS);
  if (!ret) {
  error_setg(errp, "Object with id '%s' is not TLS credentials",
-   s->parameters.tls_creds);
+   tls_creds);
  return NULL;
  }
  if (!qcrypto_tls_creds_check_endpoint(ret, endpoint, errp)) {


--
Best regards,
Vladimir

Re: [PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons

[Juan Quintela]

Peter Xu  wrote:
> On Wed, Apr 26, 2023 at 06:36:00PM +0200, Juan Quintela wrote:
>> Peter Xu  wrote:
>> > I'm doing some machine type checks to make sure nothing breaks for
>> > 7.2<->8.0.  Along the way I found one false negative report on e1000e using
>> > the static checker, turns out to be an issue in the checker itself.
>> >
>> > The problem is the checker doesn't take VMS_ARRAY into account when
>> > comparing with UNUSED, hence the total size is wrongly calculated.
>> >
>> > Fix that first in qemu by start dumping size of array as "num", then teach
>> > the checker for that.
>> >
>> > NOTE: the patchset will change both behaviors for either -dump-vmstate on
>> > QEMU or the checker, however both patches will be compatible even with old
>> > QEMU dumps or even old vmstate-checker script.  That's not extremely
>> > important, IMHO, but still worth mentioning.
>> >
>> > Thanks,
>> >
>> > Peter Xu (2):
>> >   migration/vmstate-dump: Dump array size too as "num"
>> >   vmstate-static-checker: Recognize "num" field
>> >
>> >  migration/savevm.c|  3 +++
>> >  scripts/vmstate-static-checker.py | 13 ++---
>> >  2 files changed, 13 insertions(+), 3 deletions(-)
>> 
>> Hi
>> 
>> once that you are working with the static checker.
>> 
>> Could we just run two checks in make check:
>> 
>> - qemu- -M  against the one from previous
>>   version, and see that they match.
>> - qemu- -M  against the one from previous version
>>   And we save the diffs each time that we add something incompatible and
>>   fix it on source.
>
> Normally we don't have "latest machine" but only "previous"?  Checking
> "previous" would be enough, right?  E.g. currently we're at 8.1 dev window,
> so we check against 8.0 with whatever new thing coming.

$ qemu-8.0.0 -M pc-q35-8.0.0 > dump-8.0.0-q35

We generate that dump-8.0-q35 on the tree.
We will change that once that we release 8.1, until then that is latests.

This qemu upstream is whatever is in HEAD

$ qemu-upstream -M pc-q35-8.0.0 > dump-8.0.0-upstream-q35

diff dump-8.0.0-q35 dump-8.0.0-upstream-q35

And it should be empty.

$ qemu-upstream -M pc-q35-8.1.0 > dump-8.1.0-upstream-q35

diff dump-8.0.0-q35 dump-8.1.0-upstream-q35

Each time that we find a difference, we know that we have to create a
property for that to make pc-q35-8.0.0 working.  We save that "hunk"
somehow.

Where I have put diff, we can have something more intelligent that is
able to compare json output and have into account that differences that
we already know that exist.

>> I will start with x86_64.  And once that we have it running, the other
>> architectures that care about version compatibility can add to it.
>> 
>> What do you think?
>
> It sounds a good idea to have some way to check compat bits in unit tests.
> I'm just not sure whether it's easy to integrate to make check: the
> comparision requires building two qemu binaries; one normally with an old
> tag that I built manually.

No. Single binary.  For the old binary we just have saved its output on
the tree.

> For the static checker itself, it normally also needs some intervention
> from human, e.g., it doesn't understand things like field_exists() so it
> can report "warnings" only which can be false negative even with ARRAY
> issue fixed.

This is why I mean that I want the "diff" to be a bit more intelligent
and "record" the things that we tell them that are correct.

> But ideally e.g. in a CI env we can always keep an old version qemu binary
> ready for each arch to be tested, then verify forward+backward migration
> with that old machine type with whatever patch applied on top.  One trick
> here is we need to make sure the test cmdline contains the device/anything
> that got changed by the patch.  It may not always be the case.

I will start with the default machine devices.
Once the mechanism is done, we can wonder with more configurations.

I will start small and then go from there.

Later, Juan.

Re: [PATCH v3 08/13] migration: Remove MigrationState from block_cleanup_parameters()

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

This makes the function more regular with everything else.

Signed-off-by: Juan Quintela 


Reviewed-by: Vladimir Sementsov-Ogievskiy 


---
  migration/migration.c | 4 ++--
  migration/options.c   | 4 +++-
  migration/options.h   | 2 +-
  3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index cefe6da2b8..ef8caa79b9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1218,7 +1218,7 @@ static void migrate_fd_cleanup(MigrationState *s)
  error_report_err(error_copy(s->error));
  }
  notifier_list_notify(_state_notifiers, s);
-block_cleanup_parameters(s);
+block_cleanup_parameters();
  yank_unregister_instance(MIGRATION_YANK_INSTANCE);
  }
  
@@ -1712,7 +1712,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,

 "a valid migration protocol");
  migrate_set_state(>state, MIGRATION_STATUS_SETUP,
MIGRATION_STATUS_FAILED);
-block_cleanup_parameters(s);
+block_cleanup_parameters();
  return;
  }
  
diff --git a/migration/options.c b/migration/options.c

index 26fe00799b..f65b7babef 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -597,8 +597,10 @@ void migrate_set_block_incremental(bool value)
  
  /* parameters helpers */
  
-void block_cleanup_parameters(MigrationState *s)

+void block_cleanup_parameters(void)
  {
+MigrationState *s = migrate_get_current();
+
  if (s->must_remove_block_options) {
  /* setting to false can never fail */
  migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, false, _abort);
diff --git a/migration/options.h b/migration/options.h
index 1fc8d341dd..3948218dbe 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -90,6 +90,6 @@ void migrate_set_block_incremental(bool value);
  
  bool migrate_params_check(MigrationParameters *params, Error **errp);

  void migrate_params_init(MigrationParameters *params);
-void block_cleanup_parameters(MigrationState *s);
+void block_cleanup_parameters(void);


Don't you want to rename it to migrate_* ?

--
Best regards,
Vladimir

Re: [PATCH v3 07/13] migration: Move block_cleanup_parameters() to options.c

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v3 06/13] migration: Move migrate_set_block_incremental() to options.c

[Vladimir Sementsov-Ogievskiy]

On 26.04.23 21:45, Vladimir Sementsov-Ogievskiy wrote:

On 24.04.23 21:32, Juan Quintela wrote:

Once there, make it more regular and remove th eneed for


some type here


haha, and my typo here




MigrationState parameter.


Reviewed-by: Vladimir Sementsov-Ogievskiy 



--
Best regards,
Vladimir

Re: [PATCH v3 06/13] migration: Move migrate_set_block_incremental() to options.c

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Once there, make it more regular and remove th eneed for


some type here


MigrationState parameter.


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v3 05/13] migration: Create migrate_downtime_limit() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 05/13] hw/ide: Extract pci_ide_class_init()

[Bernhard Beschow]

Am 26. April 2023 11:04:30 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> Resolves redundant code in every PCI IDE device model.
>
>I think this needs to mention that it's moving the PCIDeviceClass::exit() 
>function from all of the PCI IDE controller implementations to a common 
>implementation in the parent PCI_IDE type.

I'll completely rework this patch.

>
>> ---
>>   include/hw/ide/pci.h |  1 -
>>   hw/ide/cmd646.c  | 15 ---
>>   hw/ide/pci.c | 25 -
>>   hw/ide/piix.c| 19 ---
>>   hw/ide/sii3112.c |  3 ++-
>>   hw/ide/via.c | 15 ---
>>   6 files changed, 26 insertions(+), 52 deletions(-)
>> 
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index 74c127e32f..7bc4e53d02 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -61,7 +61,6 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
>>   extern MemoryRegionOps bmdma_addr_ioport_ops;
>>   void pci_ide_create_devs(PCIDevice *dev);
>>   -extern const VMStateDescription vmstate_ide_pci;
>>   extern const MemoryRegionOps pci_ide_cmd_le_ops;
>>   extern const MemoryRegionOps pci_ide_data_le_ops;
>>   #endif
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index a094a6e12a..9aabf80e52 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -301,17 +301,6 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, 
>> Error **errp)
>>   }
>>   }
>>   -static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>> -{
>> -PCIIDEState *d = PCI_IDE(dev);
>> -unsigned i;
>> -
>> -for (i = 0; i < 2; ++i) {
>> -memory_region_del_subregion(>bmdma_bar, >bmdma[i].extra_io);
>> -memory_region_del_subregion(>bmdma_bar, 
>> >bmdma[i].addr_ioport);
>> -}
>> -}
>> -
>>   static Property cmd646_ide_properties[] = {
>>   DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
>>   DEFINE_PROP_END_OF_LIST(),
>> @@ -323,17 +312,13 @@ static void cmd646_ide_class_init(ObjectClass *klass, 
>> void *data)
>>   PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> dc->reset = cmd646_reset;
>> -dc->vmsd = _ide_pci;
>>   k->realize = pci_cmd646_ide_realize;
>> -k->exit = pci_cmd646_ide_exitfn;
>>   k->vendor_id = PCI_VENDOR_ID_CMD;
>>   k->device_id = PCI_DEVICE_ID_CMD_646;
>>   k->revision = 0x07;
>> -k->class_id = PCI_CLASS_STORAGE_IDE;
>>   k->config_read = cmd646_pci_config_read;
>>   k->config_write = cmd646_pci_config_write;
>>   device_class_set_props(dc, cmd646_ide_properties);
>> -set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>   }
>> static const TypeInfo cmd646_ide_info = {
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index 67e0998ff0..8bea92e394 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -467,7 +467,7 @@ static int ide_pci_post_load(void *opaque, int 
>> version_id)
>>   return 0;
>>   }
>>   -const VMStateDescription vmstate_ide_pci = {
>> +static const VMStateDescription vmstate_ide_pci = {
>>   .name = "ide",
>>   .version_id = 3,
>>   .minimum_version_id = 0,
>> @@ -530,11 +530,34 @@ static void pci_ide_init(Object *obj)
>>   qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq));
>>   }
>>   +static void pci_ide_exitfn(PCIDevice *dev)
>> +{
>> +PCIIDEState *d = PCI_IDE(dev);
>> +unsigned i;
>> +
>> +for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) {
>> +memory_region_del_subregion(>bmdma_bar, >bmdma[i].extra_io);
>> +memory_region_del_subregion(>bmdma_bar, 
>> >bmdma[i].addr_ioport);
>> +}
>> +}
>> +
>> +static void pci_ide_class_init(ObjectClass *klass, void *data)
>> +{
>> +DeviceClass *dc = DEVICE_CLASS(klass);
>> +PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> +
>> +dc->vmsd = _ide_pci;
>> +k->exit = pci_ide_exitfn;
>> +k->class_id = PCI_CLASS_STORAGE_IDE;
>> +set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>> +}
>> +
>>   static const TypeInfo pci_ide_type_info = {
>>   .name = TYPE_PCI_IDE,
>>   .parent = TYPE_PCI_DEVICE,
>>   .instance_size = sizeof(PCIIDEState),
>>   .instance_init = pci_ide_init,
>> +.class_init = pci_ide_class_init,
>>   .abstract = true,
>>   .interfaces = (InterfaceInfo[]) {
>>   { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index a32f7ccece..4e6ca99123 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -159,8 +159,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
>> **errp)
>>   bmdma_setup_bar(d);
>>   pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
>>   -vmstate_register(VMSTATE_IF(dev), 0, _ide_pci, d);
>> -
>
>Presumably this still survives migration between a pre-series and post-series 
>QEMU using the PIIX IDE controller?
>
>>   for (unsigned i = 0; i < 2; i++) {
>>   if (!pci_piix_init_bus(d, i, errp)) {
>>   

Re: [PATCH 09/13] hw/ide/piix: Disuse isa_get_irq()

[Bernhard Beschow]

Am 26. April 2023 11:33:40 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> isa_get_irq() asks for an ISADevice which piix-ide doesn't provide.
>> Passing a NULL pointer works but causes the isabus global to be used
>> then. By fishing out TYPE_ISA_BUS from the QOM tree it is possible to
>> achieve the same as using isa_get_irq().
>> 
>> This is an alternative solution to commit 9405d87be25d 'hw/ide: Fix
>> crash when plugging a piix3-ide device into the x-remote machine' which
>> allows for cleaning up the ISA API while keeping PIIX IDE functions
>> user-createable.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   hw/ide/piix.c | 23 ---
>>   1 file changed, 20 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index 6942b484f9..a3a15dc7db 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -104,7 +104,8 @@ static void piix_ide_reset(DeviceState *dev)
>>   pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
>>   }
>>   -static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>> +static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus,
>> +  Error **errp)
>>   {
>>   static const struct {
>>   int iobase;
>> @@ -124,7 +125,8 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned 
>> i, Error **errp)
>>object_get_typename(OBJECT(d)), i);
>>   return false;
>>   }
>> -ide_bus_init_output_irq(>bus[i], isa_get_irq(NULL, 
>> port_info[i].isairq));
>> +ide_bus_init_output_irq(>bus[i],
>> +isa_bus_get_irq(isa_bus, port_info[i].isairq));
>
>I don't think is the right solution here, since ultimately we want to move the 
>IRQ routing out of the device itself and into the PCI-ISA bridge. I'd go for 
>the same solution as you've done for VIA IDE in patch 2, i.e. update the PIIX 
>interrupt handler to set the legacy_irqs in PCIIDEState, and then wire them up 
>to the ISA IRQs 14 and 15 similar to as Phil as done in his patches:

The problem is user-creatable PIIX-IDE. IMO we should stick to our deprecation 
process before going this step because this will break it.

>
>https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-4-phi...@linaro.org/
>
>https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-5-phi...@linaro.org/
>
>This also reminds me, given that the first patch above is doing wiring in 
>pc_init1() then we are still missing part of your tidy-up series :/
>
>>   bmdma_init(>bus[i], >bmdma[i], d);
>>   ide_bus_register_restart_cb(>bus[i]);
>> @@ -136,14 +138,29 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
>> **errp)
>>   {
>>   PCIIDEState *d = PCI_IDE(dev);
>>   uint8_t *pci_conf = dev->config;
>> +ISABus *isa_bus;
>> +bool ambiguous;
>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>> bmdma_init_ops(d, _bmdma_ops);
>>   pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_ops);
>>   +isa_bus = ISA_BUS(object_resolve_path_type("", TYPE_ISA_BUS, 
>> ));
>> +if (ambiguous) {
>> +error_setg(errp,
>> +   "More than one ISA bus found while %s supports only one",
>> +   object_get_typename(OBJECT(d)));
>> +return;
>> +}
>> +if (!isa_bus) {
>> +error_setg(errp, "No ISA bus found while %s requires one",
>> +   object_get_typename(OBJECT(d)));
>> +return;
>> +}
>
>Again I think this should go away with using PCIIDEState's legacy_irqs, since 
>you simply let the board wire them up to the ISABus (or not) as required.

Same here: This breaks user-creatable PIIX-IDE.

>
>>   for (unsigned i = 0; i < 2; i++) {
>> -if (!pci_piix_init_bus(d, i, errp)) {
>> +if (!pci_piix_init_bus(d, i, isa_bus, errp)) {
>>   return;
>>   }
>>   }
>
>
>ATB,
>
>Mark.

Re: [PATCH 10/13] hw/ide/piix: Reuse PCIIDEState::{cmd,data}_ops

[Bernhard Beschow]

Am 26. April 2023 11:37:48 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> Now that PCIIDEState::{cmd,data}_ops are initialized in the base class
>> constructor there is an opportunity for PIIX to reuse these attributes. This
>> resolves usage of ide_init_ioport() which would fall back internally to using
>> the isabus global due to NULL being passed as ISADevice by PIIX.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   hw/ide/piix.c | 30 +-
>>   1 file changed, 13 insertions(+), 17 deletions(-)
>> 
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index a3a15dc7db..406a67fa0f 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -104,34 +104,32 @@ static void piix_ide_reset(DeviceState *dev)
>>   pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
>>   }
>>   -static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus,
>> -  Error **errp)
>> +static void pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus)
>>   {
>>   static const struct {
>>   int iobase;
>>   int iobase2;
>>   int isairq;
>>   } port_info[] = {
>> -{0x1f0, 0x3f6, 14},
>> -{0x170, 0x376, 15},
>> +{0x1f0, 0x3f4, 14},
>> +{0x170, 0x374, 15},
>>   };
>> -int ret;
>> +MemoryRegion *address_space_io = pci_address_space_io(PCI_DEVICE(d));
>> ide_bus_init(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>> -ret = ide_init_ioport(>bus[i], NULL, port_info[i].iobase,
>> -  port_info[i].iobase2);
>> -if (ret) {
>> -error_setg_errno(errp, -ret, "Failed to realize %s port %u",
>> - object_get_typename(OBJECT(d)), i);
>> -return false;
>> -}
>> +memory_region_add_subregion(address_space_io, port_info[i].iobase,
>> +>data_ops[i]);
>> +/*
>> + * PIIX forwards the last byte of cmd_ops to ISA. Model this using a low
>> + * prio so competing memory regions take precedence.
>> + */
>> +memory_region_add_subregion_overlap(address_space_io, 
>> port_info[i].iobase2,
>> +>cmd_ops[i], -1);
>
>Interesting. Is this behaviour documented somewhere and/or used in one of your 
>test images at all? If I'd have seen this myself, I probably thought that the 
>addresses were a typo...

I first  stumbled upon this and wondered why this code was working with VIA_IDE 
(through my pc-via branch). Then I found the correct offsets there which are 
confirmed in the piix datasheet, e.g.: "Secondary Control Block Offset: 0374h"

>
>>   ide_bus_init_output_irq(>bus[i],
>>   isa_bus_get_irq(isa_bus, port_info[i].isairq));
>> bmdma_init(>bus[i], >bmdma[i], d);
>>   ide_bus_register_restart_cb(>bus[i]);
>> -
>> -return true;
>>   }
>> static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>> @@ -160,9 +158,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
>> **errp)
>>   }
>> for (unsigned i = 0; i < 2; i++) {
>> -if (!pci_piix_init_bus(d, i, isa_bus, errp)) {
>> -return;
>> -}
>> +pci_piix_init_bus(d, i, isa_bus);
>>   }
>>   }
>>   
>
>
>ATB,
>
>Mark.

Re: [PATCH 08/13] hw/ide: Rename PCIIDEState::*_bar attributes

[Bernhard Beschow]

Am 26. April 2023 11:21:28 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> The attributes represent memory regions containing operations which are 
>> mapped
>> by the device models into PCI BARs. Reflect this by changing the suffic into
>> "_ops".
>> 
>> Note that in a few commits piix will also use the {cmd,data}_ops but won't 
>> map
>> them into BARs. This further suggests that the "_bar" suffix doesn't match
>> very well.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   include/hw/ide/pci.h |  6 +++---
>>   hw/ide/cmd646.c  | 10 +-
>>   hw/ide/pci.c | 18 +-
>>   hw/ide/piix.c|  2 +-
>>   hw/ide/via.c | 10 +-
>>   5 files changed, 23 insertions(+), 23 deletions(-)
>> 
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index 597c77c7ad..5025df5b82 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -51,9 +51,9 @@ struct PCIIDEState {
>>   BMDMAState bmdma[2];
>>   qemu_irq isa_irq[2];
>>   uint32_t secondary; /* used only for cmd646 */
>> -MemoryRegion bmdma_bar;
>> -MemoryRegion cmd_bar[2];
>> -MemoryRegion data_bar[2];
>> +MemoryRegion bmdma_ops;
>> +MemoryRegion cmd_ops[2];
>> +MemoryRegion data_ops[2];
>>   };
>> void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index 85716aaf17..b9d005a357 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -251,13 +251,13 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, 
>> Error **errp)
>>   dev->wmask[MRDMODE] = 0x0;
>>   dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;
>>   -pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_bar[0]);
>> -pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_bar[0]);
>> -pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_bar[1]);
>> -pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_bar[1]);
>> +pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
>> +pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
>> +pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
>> +pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
>> bmdma_init_ops(d, _bmdma_ops);
>> -pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
>> +pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_ops);
>> /* TODO: RST# value should be 0 */
>>   pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index a9194313bd..b2fcc00a64 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -527,15 +527,15 @@ void bmdma_init_ops(PCIIDEState *d, const 
>> MemoryRegionOps *bmdma_ops)
>>   {
>>   size_t i;
>>   -memory_region_init(>bmdma_bar, OBJECT(d), "bmdma-container", 16);
>> +memory_region_init(>bmdma_ops, OBJECT(d), "bmdma-container", 16);
>>   for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) {
>>   BMDMAState *bm = >bmdma[i];
>> memory_region_init_io(>extra_io, OBJECT(d), bmdma_ops, bm, 
>> "bmdma-ops", 4);
>> -memory_region_add_subregion(>bmdma_bar, i * 8, >extra_io);
>> +memory_region_add_subregion(>bmdma_ops, i * 8, >extra_io);
>>   memory_region_init_io(>addr_ioport, OBJECT(d), 
>> _addr_ioport_ops, bm,
>> "bmdma-ioport-ops", 4);
>> -memory_region_add_subregion(>bmdma_bar, i * 8 + 4, 
>> >addr_ioport);
>> +memory_region_add_subregion(>bmdma_ops, i * 8 + 4, 
>> >addr_ioport);
>>   }
>>   }
>>   @@ -543,14 +543,14 @@ static void pci_ide_init(Object *obj)
>>   {
>>   PCIIDEState *d = PCI_IDE(obj);
>>   -memory_region_init_io(>data_bar[0], OBJECT(d), 
>> _ide_data_le_ops,
>> +memory_region_init_io(>data_ops[0], OBJECT(d), _ide_data_le_ops,
>> >bus[0], "pci-ide0-data-ops", 8);
>> -memory_region_init_io(>cmd_bar[0], OBJECT(d), _ide_cmd_le_ops,
>> +memory_region_init_io(>cmd_ops[0], OBJECT(d), _ide_cmd_le_ops,
>> >bus[0], "pci-ide0-cmd-ops", 4);
>>   -memory_region_init_io(>data_bar[1], OBJECT(d), 
>> _ide_data_le_ops,
>> +memory_region_init_io(>data_ops[1], OBJECT(d), _ide_data_le_ops,
>> >bus[1], "pci-ide1-data-ops", 8);
>> -memory_region_init_io(>cmd_bar[1], OBJECT(d), _ide_cmd_le_ops,
>> +memory_region_init_io(>cmd_ops[1], OBJECT(d), _ide_cmd_le_ops,
>> >bus[1], "pci-ide1-cmd-ops", 4);
>> qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq));
>> @@ -562,8 +562,8 @@ static void pci_ide_exitfn(PCIDevice *dev)
>>   unsigned i;
>> for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) {
>> -memory_region_del_subregion(>bmdma_bar, >bmdma[i].extra_io);
>> -memory_region_del_subregion(>bmdma_bar, 
>> 

Re: [PATCH v3 04/13] migration: Make all functions check have the same format

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons

[Peter Xu]

On Wed, Apr 26, 2023 at 06:36:00PM +0200, Juan Quintela wrote:
> Peter Xu  wrote:
> > I'm doing some machine type checks to make sure nothing breaks for
> > 7.2<->8.0.  Along the way I found one false negative report on e1000e using
> > the static checker, turns out to be an issue in the checker itself.
> >
> > The problem is the checker doesn't take VMS_ARRAY into account when
> > comparing with UNUSED, hence the total size is wrongly calculated.
> >
> > Fix that first in qemu by start dumping size of array as "num", then teach
> > the checker for that.
> >
> > NOTE: the patchset will change both behaviors for either -dump-vmstate on
> > QEMU or the checker, however both patches will be compatible even with old
> > QEMU dumps or even old vmstate-checker script.  That's not extremely
> > important, IMHO, but still worth mentioning.
> >
> > Thanks,
> >
> > Peter Xu (2):
> >   migration/vmstate-dump: Dump array size too as "num"
> >   vmstate-static-checker: Recognize "num" field
> >
> >  migration/savevm.c|  3 +++
> >  scripts/vmstate-static-checker.py | 13 ++---
> >  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> Hi
> 
> once that you are working with the static checker.
> 
> Could we just run two checks in make check:
> 
> - qemu- -M  against the one from previous
>   version, and see that they match.
> - qemu- -M  against the one from previous version
>   And we save the diffs each time that we add something incompatible and
>   fix it on source.

Normally we don't have "latest machine" but only "previous"?  Checking
"previous" would be enough, right?  E.g. currently we're at 8.1 dev window,
so we check against 8.0 with whatever new thing coming.

> 
> I will start with x86_64.  And once that we have it running, the other
> architectures that care about version compatibility can add to it.
> 
> What do you think?

It sounds a good idea to have some way to check compat bits in unit tests.
I'm just not sure whether it's easy to integrate to make check: the
comparision requires building two qemu binaries; one normally with an old
tag that I built manually.

For the static checker itself, it normally also needs some intervention
from human, e.g., it doesn't understand things like field_exists() so it
can report "warnings" only which can be false negative even with ARRAY
issue fixed.

But ideally e.g. in a CI env we can always keep an old version qemu binary
ready for each arch to be tested, then verify forward+backward migration
with that old machine type with whatever patch applied on top.  One trick
here is we need to make sure the test cmdline contains the device/anything
that got changed by the patch.  It may not always be the case.

Thanks,

-- 
Peter Xu

Re: [PATCH v3 03/13] migration: Create migrate_params_init() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

[PATCH v9 2/3] multifd: Protect multifd_send_sync_main() calls

[Juan Quintela]

We only need to do that on the ram_save_iterate() call on sending and
on destination when we get a RAM_SAVE_FLAG_EOS.

In setup() and complete() we need to synch in both new and old cases,
so don't add a check there.

Signed-off-by: Juan Quintela 
Reviewed-by: Dr. David Alan Gilbert 
Acked-by: Peter Xu 

---

Remove the wrappers that we take out on patch 5.
---
 migration/ram.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 01356f60a4..1e2414d681 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3394,9 +3394,11 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 out:
 if (ret >= 0
 && migration_is_setup_or_active(migrate_get_current()->state)) {
-ret = multifd_send_sync_main(rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
-if (ret < 0) {
-return ret;
+if (migrate_multifd_flush_after_each_section()) {
+ret = 
multifd_send_sync_main(rs->pss[RAM_CHANNEL_PRECOPY].pss_channel);
+if (ret < 0) {
+return ret;
+}
 }
 
 qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
@@ -4153,7 +4155,9 @@ int ram_load_postcopy(QEMUFile *f, int channel)
 
 case RAM_SAVE_FLAG_EOS:
 /* normal exit */
-multifd_recv_sync_main();
+if (migrate_multifd_flush_after_each_section()) {
+multifd_recv_sync_main();
+}
 break;
 default:
 error_report("Unknown combination of migration flags: 0x%x"
@@ -4424,7 +4428,9 @@ static int ram_load_precopy(QEMUFile *f)
 break;
 case RAM_SAVE_FLAG_EOS:
 /* normal exit */
-multifd_recv_sync_main();
+if (migrate_multifd_flush_after_each_section()) {
+multifd_recv_sync_main();
+}
 break;
 default:
 if (flags & RAM_SAVE_FLAG_HOOK) {
-- 
2.40.0

[PATCH v9 3/3] multifd: Only flush once each full round of memory

[Juan Quintela]

We need to add a new flag to mean to flush at that point.
Notice that we still flush at the end of setup and at the end of
complete stages.

Signed-off-by: Juan Quintela 

---

Add missing qemu_fflush(), now it passes all tests always.
In the previous version, the check that changes the default value to
false got lost in some rebase.  Get it back.
---
 migration/migration.c |  2 +-
 migration/migration.h |  3 +--
 migration/options.c   |  6 +-
 migration/ram.c   | 28 +++-
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index e82aa69842..e504f30c2e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3345,7 +3345,7 @@ static Property migration_properties[] = {
 DEFINE_PROP_BOOL("decompress-error-check", MigrationState,
   decompress_error_check, true),
 DEFINE_PROP_BOOL("multifd-flush-after-each-section", MigrationState,
-  multifd_flush_after_each_section, true),
+  multifd_flush_after_each_section, false),
 DEFINE_PROP_UINT8("x-clear-bitmap-shift", MigrationState,
   clear_bitmap_shift, CLEAR_BITMAP_SHIFT_DEFAULT),
 DEFINE_PROP_BOOL("x-preempt-pre-7-2", MigrationState,
diff --git a/migration/migration.h b/migration/migration.h
index e2247d708f..3a918514e7 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -412,8 +412,7 @@ struct MigrationState {
  * only need to do this flush after we have go through all the
  * dirty pages.  For historical reasons, we do that after each
  * section.  This is suboptimal (we flush too many times).
- * Default value is false.  Setting this property has no effect
- * until the patch that removes this comment.  (since 8.1)
+ * Default value is false. (since 8.1)
  */
 bool multifd_flush_after_each_section;
 /*
diff --git a/migration/options.c b/migration/options.c
index b9d54b4ef7..78bfcc8ec0 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -221,11 +221,7 @@ bool migrate_multifd_flush_after_each_section(void)
 {
 MigrationState *s = migrate_get_current();
 
-/*
- * Until the patch that remove this comment, we always return that
- * the property is enabled.
- */
-return true || s->multifd_flush_after_each_section;
+return s->multifd_flush_after_each_section;
 }
 
 bool migrate_postcopy(void)
diff --git a/migration/ram.c b/migration/ram.c
index 1e2414d681..e9dcda8b9d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -86,6 +86,7 @@
 #define RAM_SAVE_FLAG_XBZRLE   0x40
 /* 0x80 is reserved in qemu-file.h for RAM_SAVE_FLAG_HOOK */
 #define RAM_SAVE_FLAG_COMPRESS_PAGE0x100
+#define RAM_SAVE_FLAG_MULTIFD_FLUSH0x200
 /* We can't use any flag that is bigger than 0x200 */
 
 int (*xbzrle_encode_buffer_func)(uint8_t *, uint8_t *, int,
@@ -1581,6 +1582,7 @@ retry:
  * associated with the search process.
  *
  * Returns:
+ * <0: An error happened
  * PAGE_ALL_CLEAN: no dirty page found, give up
  * PAGE_TRY_AGAIN: no dirty page found, retry for next block
  * PAGE_DIRTY_FOUND: dirty page found
@@ -1608,6 +1610,15 @@ static int find_dirty_block(RAMState *rs, 
PageSearchStatus *pss)
 pss->page = 0;
 pss->block = QLIST_NEXT_RCU(pss->block, next);
 if (!pss->block) {
+if (!migrate_multifd_flush_after_each_section()) {
+QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel;
+int ret = multifd_send_sync_main(f);
+if (ret < 0) {
+return ret;
+}
+qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
+qemu_fflush(f);
+}
 /*
  * If memory migration starts over, we will meet a dirtied page
  * which may still exists in compression threads's ring, so we
@@ -2600,6 +2611,9 @@ static int ram_find_and_save_block(RAMState *rs)
 break;
 } else if (res == PAGE_TRY_AGAIN) {
 continue;
+} else if (res < 0) {
+pages = res;
+break;
 }
 }
 }
@@ -3286,6 +3300,10 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
 return ret;
 }
 
+if (!migrate_multifd_flush_after_each_section()) {
+qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
+}
+
 qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
 qemu_fflush(f);
 
@@ -3471,6 +3489,9 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
 return ret;
 }
 
+if (!migrate_multifd_flush_after_each_section()) {
+qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
+}
 qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
 qemu_fflush(f);
 
@@ -4152,7 +4173,9 @@ int ram_load_postcopy(QEMUFile *f, int channel)
 }
 

[PATCH v9 1/3] multifd: Create property multifd-flush-after-each-section

[Juan Quintela]

We used to flush all channels at the end of each RAM section
sent.  That is not needed, so preparing to only flush after a full
iteration through all the RAM.

Default value of the property is false.  But we return "true" in
migrate_multifd_flush_after_each_section() until we implement the code
in following patches.

Signed-off-by: Juan Quintela 
Reviewed-by: Dr. David Alan Gilbert 

---

Rename each-iteration to after-each-section
Rename multifd-sync-after-each-section to
   multifd-flush-after-each-section
Move to machine-8.0 (peter)
---
 hw/core/machine.c |  4 +++-
 migration/migration.c |  2 ++
 migration/migration.h | 12 
 migration/options.c   | 11 +++
 migration/options.h   |  1 +
 5 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2ce97a5d3b..47a34841a5 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -39,7 +39,9 @@
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-pci.h"
 
-GlobalProperty hw_compat_8_0[] = {};
+GlobalProperty hw_compat_8_0[] = {
+{ "migration", "multifd-flush-after-each-section", "on"},
+};
 const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);
 
 GlobalProperty hw_compat_7_2[] = {
diff --git a/migration/migration.c b/migration/migration.c
index 22e8586623..e82aa69842 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3344,6 +3344,8 @@ static Property migration_properties[] = {
  send_section_footer, true),
 DEFINE_PROP_BOOL("decompress-error-check", MigrationState,
   decompress_error_check, true),
+DEFINE_PROP_BOOL("multifd-flush-after-each-section", MigrationState,
+  multifd_flush_after_each_section, true),
 DEFINE_PROP_UINT8("x-clear-bitmap-shift", MigrationState,
   clear_bitmap_shift, CLEAR_BITMAP_SHIFT_DEFAULT),
 DEFINE_PROP_BOOL("x-preempt-pre-7-2", MigrationState,
diff --git a/migration/migration.h b/migration/migration.h
index 2b71df8617..e2247d708f 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -404,6 +404,18 @@ struct MigrationState {
  */
 bool preempt_pre_7_2;
 
+/*
+ * flush every channel after each section sent.
+ *
+ * This assures that we can't mix pages from one iteration through
+ * ram pages with pages for the following iteration.  We really
+ * only need to do this flush after we have go through all the
+ * dirty pages.  For historical reasons, we do that after each
+ * section.  This is suboptimal (we flush too many times).
+ * Default value is false.  Setting this property has no effect
+ * until the patch that removes this comment.  (since 8.1)
+ */
+bool multifd_flush_after_each_section;
 /*
  * This decides the size of guest memory chunk that will be used
  * to track dirty bitmap clearing.  The size of memory chunk will
diff --git a/migration/options.c b/migration/options.c
index c6030587cf..b9d54b4ef7 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -217,6 +217,17 @@ bool migrate_zero_copy_send(void)
 
 /* pseudo capabilities */
 
+bool migrate_multifd_flush_after_each_section(void)
+{
+MigrationState *s = migrate_get_current();
+
+/*
+ * Until the patch that remove this comment, we always return that
+ * the property is enabled.
+ */
+return true || s->multifd_flush_after_each_section;
+}
+
 bool migrate_postcopy(void)
 {
 return migrate_postcopy_ram() || migrate_dirty_bitmaps();
diff --git a/migration/options.h b/migration/options.h
index 89067e59a0..9b9ea0cde8 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -52,6 +52,7 @@ bool migrate_zero_copy_send(void);
  * check, but they are not a capability.
  */
 
+bool migrate_multifd_flush_after_each_section(void);
 bool migrate_postcopy(void);
 bool migrate_tls(void);
 
-- 
2.40.0

[PATCH v9 0/3] Eliminate multifd flush

[Juan Quintela]

Hi

Changes in v9:
- rebase over migration-pull-20230424
  this means that we need to change things for options.c creation.
- make the property be set for 8.0 not 7.0 (thanks peter)
- the check that disabled the property was lost on some rebase, put it back.

Please, review.

[v8]
- rebase over latests

[v7]
- Rebased to last upstream
- Rename the capability to a property.  So we move all the problems
  that we have on last review dissaper because it is not a capability.

So now, it is works as expected.  Enabled for old machine types,
disabled for new machine types.  Users will only found it if they go through 
the migration properties.

[v6]
- Rename multifd-sync-after-each-section to
 multifd-flush-after-each-section
- Redo comments (thanks Markus)
- Redo how to comment capabilities that are enabled/disabled during
  development. (thanks Markus)

[v5]
- Remove RAM Flags documentation (already on PULL request)
- rebase on top of PULL request.

[v4]
- Rebased on top of migration-20230209 PULL request
- Integrate two patches in that pull request
- Rebase
- Address Eric reviews.

[v3]
- update to latest upstream.
- fix checkpatch errors.

[v2]
- update to latest upstream
- change 0, 1, 2 values to defines
- Add documentation for SAVE_VM_FLAGS
- Add missing qemu_fflush(), it made random hangs for migration test
  (only for tls, no clue why).

[v1]
Upstream multifd code synchronize all threads after each RAM section.  This is 
suboptimal.
Change it to only flush after we go trough all ram.

Preserve all semantics for old machine types.

Juan Quintela (3):
  multifd: Create property multifd-flush-after-each-section
  multifd: Protect multifd_send_sync_main() calls
  multifd: Only flush once each full round of memory

 hw/core/machine.c |  4 +++-
 migration/migration.c |  2 ++
 migration/migration.h | 11 +++
 migration/options.c   |  7 +++
 migration/options.h   |  1 +
 migration/ram.c   | 44 +--
 6 files changed, 62 insertions(+), 7 deletions(-)

-- 
2.40.0

[PULL 0/7] Migration 20230426 patches

[Juan Quintela]

The following changes since commit 9c894df3a37d675652390f7dbbe2f65b7bad7efa:

  migration: Create migrate_max_bandwidth() function (2023-04-24 15:01:47 +0200)

are available in the Git repository at:

  https://gitlab.com/juan.quintela/qemu.git tags/migration-20230426-pull-request

for you to fetch changes up to 7b5cd8ff519e9fe3df6cda65428a6f1aa28a6ced:

  vmstate-static-checker: Recognize "num" field (2023-04-26 19:17:55 +0200)


Migration Pull request

Hi

This PULL request is on top of migration-20230424 already queued by
Richard.

It contains:
- MAINTAINERS: make peter and leo reviewers for migration (juan)
- Disable postcopy + multifd together. It needs at least to call
  send_sync before it will work. (juan)
- Improve postcopy error messages (peter)
- vmstate checker: Compare sizes of arrays correctly (peter)
- Move more capability functions to options.c (juan)

Please, apply.

Subject: [PULL 00/30] Migration 20230424 patches
Based-on: <20230424132730.70752-1-quint...@redhat.com>



Juan Quintela (3):
  MAINTAINERS: Add Leonardo and Peter as reviewers
  migration: Move migrate_use_tls() to options.c
  migration: Move qmp_migrate_set_parameters() to options.c

Leonardo Bras (1):
  migration: Disable postcopy + multifd migration

Peter Xu (3):
  migration: Allow postcopy_ram_supported_by_host() to report err
  migration/vmstate-dump: Dump array size too as "num"
  vmstate-static-checker: Recognize "num" field

 MAINTAINERS   |   2 +
 migration/migration.c | 429 -
 migration/migration.h |   2 -
 migration/options.c   | 442 +-
 migration/options.h   |  12 +
 migration/postcopy-ram.c  |  59 ++--
 migration/postcopy-ram.h  |   3 +-
 migration/savevm.c|   6 +-
 migration/tls.c   |   3 +-
 scripts/vmstate-static-checker.py |  13 +-
 10 files changed, 501 insertions(+), 470 deletions(-)

-- 
2.40.0

[PULL 4/7] migration: Move qmp_migrate_set_parameters() to options.c

[Juan Quintela]

Signed-off-by: Juan Quintela 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 migration/migration.c | 420 --
 migration/options.c   | 418 +
 migration/options.h   |  11 ++
 3 files changed, 429 insertions(+), 420 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 02c2355d0d..22e8586623 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -67,19 +67,10 @@
 
 #define MAX_THROTTLE  (128 << 20)  /* Migration transfer speed throttling 
*/
 
-/* Amount of time to allocate to each "chunk" of bandwidth-throttled
- * data. */
-#define BUFFER_DELAY 100
-#define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
-
 /* Time in milliseconds we are allowed to stop the source,
  * for sending the last part */
 #define DEFAULT_MIGRATE_SET_DOWNTIME 300
 
-/* Maximum migrate downtime set to 2000 seconds */
-#define MAX_MIGRATE_DOWNTIME_SECONDS 2000
-#define MAX_MIGRATE_DOWNTIME (MAX_MIGRATE_DOWNTIME_SECONDS * 1000)
-
 /* Default compression thread count */
 #define DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT 8
 /* Default decompression thread count, usually decompression is at
@@ -1140,417 +1131,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
 return info;
 }
 
-/*
- * Check whether the parameters are valid. Error will be put into errp
- * (if provided). Return true if valid, otherwise false.
- */
-static bool migrate_params_check(MigrationParameters *params, Error **errp)
-{
-if (params->has_compress_level &&
-(params->compress_level > 9)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
-   "a value between 0 and 9");
-return false;
-}
-
-if (params->has_compress_threads && (params->compress_threads < 1)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-   "compress_threads",
-   "a value between 1 and 255");
-return false;
-}
-
-if (params->has_decompress_threads && (params->decompress_threads < 1)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-   "decompress_threads",
-   "a value between 1 and 255");
-return false;
-}
-
-if (params->has_throttle_trigger_threshold &&
-(params->throttle_trigger_threshold < 1 ||
- params->throttle_trigger_threshold > 100)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-   "throttle_trigger_threshold",
-   "an integer in the range of 1 to 100");
-return false;
-}
-
-if (params->has_cpu_throttle_initial &&
-(params->cpu_throttle_initial < 1 ||
- params->cpu_throttle_initial > 99)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-   "cpu_throttle_initial",
-   "an integer in the range of 1 to 99");
-return false;
-}
-
-if (params->has_cpu_throttle_increment &&
-(params->cpu_throttle_increment < 1 ||
- params->cpu_throttle_increment > 99)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-   "cpu_throttle_increment",
-   "an integer in the range of 1 to 99");
-return false;
-}
-
-if (params->has_max_bandwidth && (params->max_bandwidth > SIZE_MAX)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-   "max_bandwidth",
-   "an integer in the range of 0 to "stringify(SIZE_MAX)
-   " bytes/second");
-return false;
-}
-
-if (params->has_downtime_limit &&
-(params->downtime_limit > MAX_MIGRATE_DOWNTIME)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-   "downtime_limit",
-   "an integer in the range of 0 to "
-stringify(MAX_MIGRATE_DOWNTIME)" ms");
-return false;
-}
-
-/* x_checkpoint_delay is now always positive */
-
-if (params->has_multifd_channels && (params->multifd_channels < 1)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-   "multifd_channels",
-   "a value between 1 and 255");
-return false;
-}
-
-if (params->has_multifd_zlib_level &&
-(params->multifd_zlib_level > 9)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zlib_level",
-   "a value between 0 and 9");
-return false;
-}
-
-if (params->has_multifd_zstd_level &&
-(params->multifd_zstd_level > 20)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zstd_level",
-   "a value between 0 and 20");
-return false;
-}
-
-if (params->has_xbzrle_cache_size &&
-(params->xbzrle_cache_size < qemu_target_page_size() ||
- !is_power_of_2(params->xbzrle_cache_size))) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-   "xbzrle_cache_size",
- 

[PULL 5/7] migration: Allow postcopy_ram_supported_by_host() to report err

[Juan Quintela]

From: Peter Xu 

Instead of print it to STDERR, bring the error upwards so that it can be
reported via QMP responses.

E.g.:

{ "execute": "migrate-set-capabilities" ,
  "arguments": { "capabilities":
  [ { "capability": "postcopy-ram", "state": true } ] } }

{ "error":
  { "class": "GenericError",
"desc": "Postcopy is not supported: Host backend files need to be TMPFS
or HUGETLBFS only" } }

Signed-off-by: Peter Xu 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/options.c  |  8 ++
 migration/postcopy-ram.c | 59 ++--
 migration/postcopy-ram.h |  3 +-
 migration/savevm.c   |  3 +-
 4 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index fe7d7754c4..c6030587cf 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -302,6 +302,7 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, 
Error **errp)
 {
 MigrationIncomingState *mis = migration_incoming_get_current();
 
+ERRP_GUARD();
 #ifndef CONFIG_LIVE_BLOCK_MIGRATION
 if (new_caps[MIGRATION_CAPABILITY_BLOCK]) {
 error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
@@ -327,11 +328,8 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, 
Error **errp)
  */
 if (!old_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM] &&
 runstate_check(RUN_STATE_INMIGRATE) &&
-!postcopy_ram_supported_by_host(mis)) {
-/* postcopy_ram_supported_by_host will have emitted a more
- * detailed message
- */
-error_setg(errp, "Postcopy is not supported");
+!postcopy_ram_supported_by_host(mis, errp)) {
+error_prepend(errp, "Postcopy is not supported: ");
 return false;
 }
 
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 0711500036..7c280480c2 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -283,11 +283,13 @@ static bool request_ufd_features(int ufd, uint64_t 
features)
 return true;
 }
 
-static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
+static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis,
+Error **errp)
 {
 uint64_t asked_features = 0;
 static uint64_t supported_features;
 
+ERRP_GUARD();
 /*
  * it's not possible to
  * request UFFD_API twice per one fd
@@ -295,7 +297,7 @@ static bool ufd_check_and_apply(int ufd, 
MigrationIncomingState *mis)
  */
 if (!supported_features) {
 if (!receive_ufd_features(_features)) {
-error_report("%s failed", __func__);
+error_setg(errp, "Userfault feature detection failed");
 return false;
 }
 }
@@ -317,8 +319,7 @@ static bool ufd_check_and_apply(int ufd, 
MigrationIncomingState *mis)
  * userfault file descriptor
  */
 if (!request_ufd_features(ufd, asked_features)) {
-error_report("%s failed: features %" PRIu64, __func__,
- asked_features);
+error_setg(errp, "Failed features %" PRIu64, asked_features);
 return false;
 }
 
@@ -329,7 +330,8 @@ static bool ufd_check_and_apply(int ufd, 
MigrationIncomingState *mis)
 have_hp = supported_features & UFFD_FEATURE_MISSING_HUGETLBFS;
 #endif
 if (!have_hp) {
-error_report("Userfault on this host does not support huge pages");
+error_setg(errp,
+   "Userfault on this host does not support huge pages");
 return false;
 }
 }
@@ -338,7 +340,7 @@ static bool ufd_check_and_apply(int ufd, 
MigrationIncomingState *mis)
 
 /* Callback from postcopy_ram_supported_by_host block iterator.
  */
-static int test_ramblock_postcopiable(RAMBlock *rb)
+static int test_ramblock_postcopiable(RAMBlock *rb, Error **errp)
 {
 const char *block_name = qemu_ram_get_idstr(rb);
 ram_addr_t length = qemu_ram_get_used_length(rb);
@@ -346,16 +348,18 @@ static int test_ramblock_postcopiable(RAMBlock *rb)
 QemuFsType fs;
 
 if (length % pagesize) {
-error_report("Postcopy requires RAM blocks to be a page size multiple,"
- " block %s is 0x" RAM_ADDR_FMT " bytes with a "
- "page size of 0x%zx", block_name, length, pagesize);
+error_setg(errp,
+   "Postcopy requires RAM blocks to be a page size multiple,"
+   " block %s is 0x" RAM_ADDR_FMT " bytes with a "
+   "page size of 0x%zx", block_name, length, pagesize);
 return 1;
 }
 
 if (rb->fd >= 0) {
 fs = qemu_fd_getfs(rb->fd);
 if (fs != QEMU_FS_TYPE_TMPFS && fs != QEMU_FS_TYPE_HUGETLBFS) {
-error_report("Host backend files need to be TMPFS or HUGETLBFS 
only");
+error_setg(errp,
+   "Host backend files need to be TMPFS or HUGETLBFS 
only");

[PULL 3/7] migration: Move migrate_use_tls() to options.c

[Juan Quintela]

Once there, rename it to migrate_tls() and make it return bool for
consistency.

Signed-off-by: Juan Quintela 
Reviewed-by: Vladimir Sementsov-Ogievskiy 

---

Fix typos found by fabiano
---
 migration/migration.c |  9 -
 migration/migration.h |  2 --
 migration/options.c   | 11 ++-
 migration/options.h   |  1 +
 migration/tls.c   |  3 ++-
 5 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 53dd59f6f6..02c2355d0d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2177,15 +2177,6 @@ void qmp_migrate_continue(MigrationStatus state, Error 
**errp)
 qemu_sem_post(>pause_sem);
 }
 
-int migrate_use_tls(void)
-{
-MigrationState *s;
-
-s = migrate_get_current();
-
-return s->parameters.tls_creds && *s->parameters.tls_creds;
-}
-
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
diff --git a/migration/migration.h b/migration/migration.h
index dcf906868d..2b71df8617 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -447,8 +447,6 @@ bool migration_is_blocked(Error **errp);
 bool migration_in_postcopy(void);
 MigrationState *migrate_get_current(void);
 
-int migrate_use_tls(void);
-
 uint64_t ram_get_total_transferred_pages(void);
 
 /* Sending on the return path - generic and then for each message type */
diff --git a/migration/options.c b/migration/options.c
index dd97c99391..943936a320 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -214,6 +214,15 @@ bool migrate_postcopy(void)
 return migrate_postcopy_ram() || migrate_dirty_bitmaps();
 }
 
+bool migrate_tls(void)
+{
+MigrationState *s;
+
+s = migrate_get_current();
+
+return s->parameters.tls_creds && *s->parameters.tls_creds;
+}
+
 typedef enum WriteTrackingSupport {
 WT_SUPPORT_UNKNOWN = 0,
 WT_SUPPORT_ABSENT,
@@ -368,7 +377,7 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, 
Error **errp)
  new_caps[MIGRATION_CAPABILITY_COMPRESS] ||
  new_caps[MIGRATION_CAPABILITY_XBZRLE] ||
  migrate_multifd_compression() ||
- migrate_use_tls())) {
+ migrate_tls())) {
 error_setg(errp,
"Zero copy only available for non-compressed non-TLS 
multifd migration");
 return false;
diff --git a/migration/options.h b/migration/options.h
index 1b78fa9f3d..13318a16c7 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -46,6 +46,7 @@ bool migrate_zero_copy_send(void);
  */
 
 bool migrate_postcopy(void);
+bool migrate_tls(void);
 
 /* capabilities helpers */
 
diff --git a/migration/tls.c b/migration/tls.c
index 4d2166a209..acd38e0b62 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -22,6 +22,7 @@
 #include "channel.h"
 #include "migration.h"
 #include "tls.h"
+#include "options.h"
 #include "crypto/tlscreds.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
@@ -165,7 +166,7 @@ void migration_tls_channel_connect(MigrationState *s,
 
 bool migrate_channel_requires_tls_upgrade(QIOChannel *ioc)
 {
-if (!migrate_use_tls()) {
+if (!migrate_tls()) {
 return false;
 }
 
-- 
2.40.0

[PULL 7/7] vmstate-static-checker: Recognize "num" field

[Juan Quintela]

From: Peter Xu 

Recognize this field for VMS_ARRAY typed vmsd fields, then we can do proper
size matching with previous patch.

Note that this is compatible with old -dump-vmstate output, because when
"num" is not there we'll still use the old "size" only.

Signed-off-by: Peter Xu 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 scripts/vmstate-static-checker.py | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/scripts/vmstate-static-checker.py 
b/scripts/vmstate-static-checker.py
index dfeee8231a..9c0e6b81f2 100755
--- a/scripts/vmstate-static-checker.py
+++ b/scripts/vmstate-static-checker.py
@@ -134,6 +134,11 @@ def exists_in_substruct(fields, item):
 return check_fields_match(fields["Description"]["name"],
   substruct_fields[0]["field"], item)
 
+def size_total(entry):
+size = entry["size"]
+if "num" not in entry:
+return size
+return size * entry["num"]
 
 def check_fields(src_fields, dest_fields, desc, sec):
 # This function checks for all the fields in a section.  If some
@@ -249,17 +254,19 @@ def check_fields(src_fields, dest_fields, desc, sec):
 continue
 
 if s_item["field"] == "unused" or d_item["field"] == "unused":
-if s_item["size"] == d_item["size"]:
+s_size = size_total(s_item)
+d_size = size_total(d_item)
+if s_size == d_size:
 continue
 
 if d_item["field"] == "unused":
 advance_dest = False
-unused_count = d_item["size"] - s_item["size"]
+unused_count = d_size - s_size;
 continue
 
 if s_item["field"] == "unused":
 advance_src = False
-unused_count = s_item["size"] - d_item["size"]
+unused_count = s_size - d_size
 continue
 
 print("Section \"" + sec + "\",", end=' ')
-- 
2.40.0

[PULL 2/7] MAINTAINERS: Add Leonardo and Peter as reviewers

[Juan Quintela]

Now that David has stepped down with Migration maintainership,
Leonardo and Peter has volunteer to review the migration patches.
This way they got CC'd on every migration patch.

Signed-off-by: Juan Quintela 
Acked-by: Peter Xu 
Acked-by: Leonardo Bras 
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 24154f5721..1aa50c0fe2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3150,6 +3150,8 @@ F: scripts/checkpatch.pl
 
 Migration
 M: Juan Quintela 
+R: Peter Xu 
+R: Leonardo Bras 
 S: Maintained
 F: hw/core/vmstate-if.c
 F: include/hw/vmstate-if.h
-- 
2.40.0

[PULL 6/7] migration/vmstate-dump: Dump array size too as "num"

[Juan Quintela]

From: Peter Xu 

For VMS_ARRAY typed vmsd fields, also dump the number of entries in the
array in -vmstate-dump.

Without such information, vmstate static checker can report false negatives
of incompatible vmsd on VMS_ARRAY typed fields, when the src/dst do not
have the same type of array defined.  It's because in the checker we only
check against size of fields within a VMSD field.

One example: e1000e used to have a field defined as a boolean array with 5
entries, then removed it and replaced it with UNUSED (in 31e3f318c8b535):

-VMSTATE_BOOL_ARRAY(core.eitr_intr_pending, E1000EState,
-   E1000E_MSIX_VEC_NUM),
+VMSTATE_UNUSED(E1000E_MSIX_VEC_NUM),

It's a legal replacement but vmstate static checker is not happy with it,
because it checks only against the "size" field between the two
fields (here one is BOOL_ARRAY, the other is UNUSED):

For BOOL_ARRAY:

  {
"field": "core.eitr_intr_pending",
"version_id": 0,
"field_exists": false,
"size": 1
  },

For UNUSED:

  {
"field": "unused",
"version_id": 0,
"field_exists": false,
"size": 5
  },

It's not the script to blame because there's just not enough information
dumped to show the total size of the entry for an array.  Add it.

Note that this will not break old vmstate checker because the field will
just be ignored.

Signed-off-by: Peter Xu 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/savevm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 211eff3a8b..a9181b444b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -536,6 +536,9 @@ static void dump_vmstate_vmsf(FILE *out_file, const 
VMStateField *field,
 field->version_id);
 fprintf(out_file, "%*s\"field_exists\": %s,\n", indent, "",
 field->field_exists ? "true" : "false");
+if (field->flags & VMS_ARRAY) {
+fprintf(out_file, "%*s\"num\": %d,\n", indent, "", field->num);
+}
 fprintf(out_file, "%*s\"size\": %zu", indent, "", field->size);
 if (field->vmsd != NULL) {
 fprintf(out_file, ",\n");
-- 
2.40.0

[PULL 1/7] migration: Disable postcopy + multifd migration

[Juan Quintela]

From: Leonardo Bras 

Since the introduction of multifd, it's possible to perform a multifd
migration and finish it using postcopy.

A bug introduced by yank (fixed on cfc3bcf373) was previously preventing
a successful use of this migration scenario, and now thing should be
working on most scenarios.

But since there is not enough testing/support nor any reported users for
this scenario, we should disable this combination before it may cause any
problems for users.

Suggested-by: Dr. David Alan Gilbert 
Signed-off-by: Leonardo Bras 
Acked-by: Peter Xu 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/options.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/migration/options.c b/migration/options.c
index 8e8753d9be..dd97c99391 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -322,6 +322,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, 
Error **errp)
 error_setg(errp, "Postcopy is not compatible with ignore-shared");
 return false;
 }
+
+if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
+error_setg(errp, "Postcopy is not yet compatible with multifd");
+return false;
+}
 }
 
 if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
-- 
2.40.0

Re: [PATCH 1/9] Hexagon (target/hexagon) Add support for v68/v69/v71/v73

[Anton Johansson via]

On 4/26/23 04:30, Taylor Simpson wrote:

Add support for the ELF flags
Move target/hexagon/cpu.[ch] to be v73
Change the compiler flag used by "make check-tcg"

The decbin instruction is removed in Hexagon v73, so check the
version before trying to compile the instruction.

Signed-off-by: Taylor Simpson 
---
  configure |  2 +-
  linux-user/hexagon/target_elf.h   | 13 +
  target/hexagon/cpu.h  |  4 
  target/hexagon/cpu.c  | 20 
  tests/tcg/hexagon/misc.c  | 12 
  tests/tcg/hexagon/Makefile.target |  3 +++
  6 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/configure b/configure
index 77c03315f8..01fa77f6c7 100755
--- a/configure
+++ b/configure
@@ -1857,7 +1857,7 @@ fi
  : ${cross_cc_armeb="$cross_cc_arm"}
  : ${cross_cc_cflags_armeb="-mbig-endian"}
  : ${cross_cc_hexagon="hexagon-unknown-linux-musl-clang"}
-: ${cross_cc_cflags_hexagon="-mv67 -O2 -static"}
+: ${cross_cc_cflags_hexagon="-mv73 -O2 -static"}
  : ${cross_cc_cflags_i386="-m32"}
  : ${cross_cc_cflags_ppc="-m32 -mbig-endian"}
  : ${cross_cc_cflags_ppc64="-m64 -mbig-endian"}
diff --git a/linux-user/hexagon/target_elf.h b/linux-user/hexagon/target_elf.h
index b4e9f40527..a0271a0a2a 100644
--- a/linux-user/hexagon/target_elf.h
+++ b/linux-user/hexagon/target_elf.h
@@ -1,5 +1,5 @@
  /*
- *  Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights 
Reserved.
+ *  Copyright(c) 2019-2023 Qualcomm Innovation Center, Inc. All Rights 
Reserved.
   *
   *  This program is free software; you can redistribute it and/or modify
   *  it under the terms of the GNU General Public License as published by
@@ -20,7 +20,7 @@
  
  static inline const char *cpu_get_model(uint32_t eflags)

  {
-/* For now, treat anything newer than v5 as a v67 */
+/* For now, treat anything newer than v5 as a v73 */
  /* FIXME - Disable instructions that are newer than the specified arch */
  if (eflags == 0x04 ||/* v5  */
  eflags == 0x05 ||/* v55 */
@@ -30,9 +30,14 @@ static inline const char *cpu_get_model(uint32_t eflags)
  eflags == 0x65 ||/* v65 */
  eflags == 0x66 ||/* v66 */
  eflags == 0x67 ||/* v67 */
-eflags == 0x8067 /* v67t */
+eflags == 0x8067 ||  /* v67t */
+eflags == 0x68 ||/* v68 */
+eflags == 0x69 ||/* v69 */
+eflags == 0x71 ||/* v71 */
+eflags == 0x8071 ||  /* v71t */
+eflags == 0x73   /* v73 */
 ) {
-return "v67";
+return "v73";
  }
  return "unknown";
  }
diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
index 81b663ecfb..4d8981d862 100644
--- a/target/hexagon/cpu.h
+++ b/target/hexagon/cpu.h
@@ -43,6 +43,10 @@
  #define CPU_RESOLVING_TYPE TYPE_HEXAGON_CPU
  
  #define TYPE_HEXAGON_CPU_V67 HEXAGON_CPU_TYPE_NAME("v67")

+#define TYPE_HEXAGON_CPU_V68 HEXAGON_CPU_TYPE_NAME("v68")
+#define TYPE_HEXAGON_CPU_V69 HEXAGON_CPU_TYPE_NAME("v69")
+#define TYPE_HEXAGON_CPU_V71 HEXAGON_CPU_TYPE_NAME("v71")
+#define TYPE_HEXAGON_CPU_V73 HEXAGON_CPU_TYPE_NAME("v73")
  
  #define MMU_USER_IDX 0
  
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c

index ab40cfc283..8699db8c24 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -29,6 +29,22 @@ static void hexagon_v67_cpu_init(Object *obj)
  {
  }
  
+static void hexagon_v68_cpu_init(Object *obj)

+{
+}
+
+static void hexagon_v69_cpu_init(Object *obj)
+{
+}
+
+static void hexagon_v71_cpu_init(Object *obj)
+{
+}
+
+static void hexagon_v73_cpu_init(Object *obj)
+{
+}
+
  static ObjectClass *hexagon_cpu_class_by_name(const char *cpu_model)
  {
  ObjectClass *oc;
@@ -382,6 +398,10 @@ static const TypeInfo hexagon_cpu_type_infos[] = {
  .class_init = hexagon_cpu_class_init,
  },
  DEFINE_CPU(TYPE_HEXAGON_CPU_V67,  hexagon_v67_cpu_init),
+DEFINE_CPU(TYPE_HEXAGON_CPU_V68,  hexagon_v68_cpu_init),
+DEFINE_CPU(TYPE_HEXAGON_CPU_V69,  hexagon_v69_cpu_init),
+DEFINE_CPU(TYPE_HEXAGON_CPU_V71,  hexagon_v71_cpu_init),
+DEFINE_CPU(TYPE_HEXAGON_CPU_V73,  hexagon_v73_cpu_init),


The large spacing to hexagon_v*_cpu_init looks a bit odd.

Also, do we need to provide a *_cpu_init() stub for each version? Seems 
from qom/object.c like we should be able to

just default initialize it

Otherwise,

Reviewed-by: Anton Johansson 

[PATCH v11 01/13] target/arm: Move cortex sysregs into a separate file

[Fabiano Rosas]

The file cpu_tcg.c is about to be moved into the tcg/ directory, so
move the register definitions into a new file.

Also move the function declaration to the more appropriate cpregs.h.

Reviewed-by: Richard Henderson 
Signed-off-by: Fabiano Rosas 
---
 target/arm/cortex-regs.c | 69 
 target/arm/cpregs.h  |  6 
 target/arm/cpu64.c   |  1 +
 target/arm/cpu_tcg.c | 59 --
 target/arm/internals.h   |  6 
 target/arm/meson.build   |  1 +
 6 files changed, 77 insertions(+), 65 deletions(-)
 create mode 100644 target/arm/cortex-regs.c

diff --git a/target/arm/cortex-regs.c b/target/arm/cortex-regs.c
new file mode 100644
index 00..17708480e7
--- /dev/null
+++ b/target/arm/cortex-regs.c
@@ -0,0 +1,69 @@
+/*
+ * ARM Cortex-A registers
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "cpregs.h"
+
+
+static uint64_t l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+ARMCPU *cpu = env_archcpu(env);
+
+/* Number of cores is in [25:24]; otherwise we RAZ */
+return (cpu->core_count - 1) << 24;
+}
+
+static const ARMCPRegInfo cortex_a72_a57_a53_cp_reginfo[] = {
+{ .name = "L2CTLR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 1, .crn = 11, .crm = 0, .opc2 = 2,
+  .access = PL1_RW, .readfn = l2ctlr_read,
+  .writefn = arm_cp_write_ignore },
+{ .name = "L2CTLR",
+  .cp = 15, .opc1 = 1, .crn = 9, .crm = 0, .opc2 = 2,
+  .access = PL1_RW, .readfn = l2ctlr_read,
+  .writefn = arm_cp_write_ignore },
+{ .name = "L2ECTLR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 1, .crn = 11, .crm = 0, .opc2 = 3,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "L2ECTLR",
+  .cp = 15, .opc1 = 1, .crn = 9, .crm = 0, .opc2 = 3,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "L2ACTLR", .state = ARM_CP_STATE_BOTH,
+  .opc0 = 3, .opc1 = 1, .crn = 15, .crm = 0, .opc2 = 0,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "CPUACTLR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 1, .crn = 15, .crm = 2, .opc2 = 0,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "CPUACTLR",
+  .cp = 15, .opc1 = 0, .crm = 15,
+  .access = PL1_RW, .type = ARM_CP_CONST | ARM_CP_64BIT, .resetvalue = 0 },
+{ .name = "CPUECTLR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 1, .crn = 15, .crm = 2, .opc2 = 1,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "CPUECTLR",
+  .cp = 15, .opc1 = 1, .crm = 15,
+  .access = PL1_RW, .type = ARM_CP_CONST | ARM_CP_64BIT, .resetvalue = 0 },
+{ .name = "CPUMERRSR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 1, .crn = 15, .crm = 2, .opc2 = 2,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "CPUMERRSR",
+  .cp = 15, .opc1 = 2, .crm = 15,
+  .access = PL1_RW, .type = ARM_CP_CONST | ARM_CP_64BIT, .resetvalue = 0 },
+{ .name = "L2MERRSR_EL1", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 1, .crn = 15, .crm = 2, .opc2 = 3,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "L2MERRSR",
+  .cp = 15, .opc1 = 3, .crm = 15,
+  .access = PL1_RW, .type = ARM_CP_CONST | ARM_CP_64BIT, .resetvalue = 0 },
+};
+
+void define_cortex_a72_a57_a53_cp_reginfo(ARMCPU *cpu)
+{
+define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
+}
diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h
index 1ee64e99de..b04d344a9f 100644
--- a/target/arm/cpregs.h
+++ b/target/arm/cpregs.h
@@ -1071,4 +1071,10 @@ static inline bool arm_cpreg_in_idspace(const 
ARMCPRegInfo *ri)
   ri->crn, ri->crm);
 }
 
+#ifdef CONFIG_USER_ONLY
+static inline void define_cortex_a72_a57_a53_cp_reginfo(ARMCPU *cpu) { }
+#else
+void define_cortex_a72_a57_a53_cp_reginfo(ARMCPU *cpu);
+#endif
+
 #endif /* TARGET_ARM_CPREGS_H */
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 735ca54163..76891c9288 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -30,6 +30,7 @@
 #include "qapi/visitor.h"
 #include "hw/qdev-properties.h"
 #include "internals.h"
+#include "cpregs.h"
 
 static void aarch64_a35_initfn(Object *obj)
 {
diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
index 1911d7ec47..15aa88e40f 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/cpu_tcg.c
@@ -93,65 +93,6 @@ void aa32_max_features(ARMCPU *cpu)
 cpu->isar.id_dfr0 = t;
 }
 
-#ifndef CONFIG_USER_ONLY
-static uint64_t l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
-{
-ARMCPU *cpu = env_archcpu(env);
-
-/* Number of cores is in [25:24]; otherwise we RAZ */
-return (cpu->core_count - 1) << 24;
-}
-
-static const ARMCPRegInfo 

[PATCH v11 05/13] target/arm: Move 64-bit TCG CPUs into tcg/

[Fabiano Rosas]

Move the 64-bit CPUs that are TCG-only:
- cortex-a35
- cortex-a55
- cortex-a72
- cortex-a76
- a64fx
- neoverse-n1

Keep the CPUs that can be used with KVM:
- cortex-a57
- cortex-a53
- max
- host

Signed-off-by: Fabiano Rosas 
Reviewed-by: Richard Henderson 
---
 hw/arm/virt.c  |   6 +-
 target/arm/cpu64.c | 687 +--
 target/arm/internals.h |   4 +
 target/arm/tcg/cpu64.c | 723 +
 target/arm/tcg/meson.build |   1 +
 5 files changed, 735 insertions(+), 686 deletions(-)
 create mode 100644 target/arm/tcg/cpu64.c

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a89d699f0b..1450a9f363 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -208,14 +208,16 @@ static const char *valid_cpus[] = {
 ARM_CPU_TYPE_NAME("cortex-a7"),
 #endif
 ARM_CPU_TYPE_NAME("cortex-a15"),
+#ifdef CONFIG_TCG
 ARM_CPU_TYPE_NAME("cortex-a35"),
-ARM_CPU_TYPE_NAME("cortex-a53"),
 ARM_CPU_TYPE_NAME("cortex-a55"),
-ARM_CPU_TYPE_NAME("cortex-a57"),
 ARM_CPU_TYPE_NAME("cortex-a72"),
 ARM_CPU_TYPE_NAME("cortex-a76"),
 ARM_CPU_TYPE_NAME("a64fx"),
 ARM_CPU_TYPE_NAME("neoverse-n1"),
+#endif
+ARM_CPU_TYPE_NAME("cortex-a53"),
+ARM_CPU_TYPE_NAME("cortex-a57"),
 ARM_CPU_TYPE_NAME("host"),
 ARM_CPU_TYPE_NAME("max"),
 };
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 6a6a2ece2b..6eaf8e32cf 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -34,86 +34,6 @@
 #include "internals.h"
 #include "cpregs.h"
 
-static void aarch64_a35_initfn(Object *obj)
-{
-ARMCPU *cpu = ARM_CPU(obj);
-
-cpu->dtb_compatible = "arm,cortex-a35";
-set_feature(>env, ARM_FEATURE_V8);
-set_feature(>env, ARM_FEATURE_NEON);
-set_feature(>env, ARM_FEATURE_GENERIC_TIMER);
-set_feature(>env, ARM_FEATURE_AARCH64);
-set_feature(>env, ARM_FEATURE_CBAR_RO);
-set_feature(>env, ARM_FEATURE_EL2);
-set_feature(>env, ARM_FEATURE_EL3);
-set_feature(>env, ARM_FEATURE_PMU);
-
-/* From B2.2 AArch64 identification registers. */
-cpu->midr = 0x411fd040;
-cpu->revidr = 0;
-cpu->ctr = 0x84448004;
-cpu->isar.id_pfr0 = 0x0131;
-cpu->isar.id_pfr1 = 0x00011011;
-cpu->isar.id_dfr0 = 0x03010066;
-cpu->id_afr0 = 0;
-cpu->isar.id_mmfr0 = 0x10201105;
-cpu->isar.id_mmfr1 = 0x4000;
-cpu->isar.id_mmfr2 = 0x0126;
-cpu->isar.id_mmfr3 = 0x02102211;
-cpu->isar.id_isar0 = 0x02101110;
-cpu->isar.id_isar1 = 0x13112111;
-cpu->isar.id_isar2 = 0x21232042;
-cpu->isar.id_isar3 = 0x01112131;
-cpu->isar.id_isar4 = 0x00011142;
-cpu->isar.id_isar5 = 0x00011121;
-cpu->isar.id_aa64pfr0 = 0x;
-cpu->isar.id_aa64pfr1 = 0;
-cpu->isar.id_aa64dfr0 = 0x10305106;
-cpu->isar.id_aa64dfr1 = 0;
-cpu->isar.id_aa64isar0 = 0x00011120;
-cpu->isar.id_aa64isar1 = 0;
-cpu->isar.id_aa64mmfr0 = 0x00101122;
-cpu->isar.id_aa64mmfr1 = 0;
-cpu->clidr = 0x0a200023;
-cpu->dcz_blocksize = 4;
-
-/* From B2.4 AArch64 Virtual Memory control registers */
-cpu->reset_sctlr = 0x00c50838;
-
-/* From B2.10 AArch64 performance monitor registers */
-cpu->isar.reset_pmcr_el0 = 0x410a3000;
-
-/* From B2.29 Cache ID registers */
-cpu->ccsidr[0] = 0x700fe01a; /* 32KB L1 dcache */
-cpu->ccsidr[1] = 0x201fe00a; /* 32KB L1 icache */
-cpu->ccsidr[2] = 0x703fe03a; /* 512KB L2 cache */
-
-/* From B3.5 VGIC Type register */
-cpu->gic_num_lrs = 4;
-cpu->gic_vpribits = 5;
-cpu->gic_vprebits = 5;
-cpu->gic_pribits = 5;
-
-/* From C6.4 Debug ID Register */
-cpu->isar.dbgdidr = 0x3516d000;
-/* From C6.5 Debug Device ID Register */
-cpu->isar.dbgdevid = 0x00110f13;
-/* From C6.6 Debug Device ID Register 1 */
-cpu->isar.dbgdevid1 = 0x2;
-
-/* From Cortex-A35 SIMD and Floating-point Support r1p0 */
-/* From 3.2 AArch32 register summary */
-cpu->reset_fpsid = 0x41034043;
-
-/* From 2.2 AArch64 register summary */
-cpu->isar.mvfr0 = 0x10110222;
-cpu->isar.mvfr1 = 0x1211;
-cpu->isar.mvfr2 = 0x0043;
-
-/* These values are the same with A53/A57/A72. */
-define_cortex_a72_a57_a53_cp_reginfo(cpu);
-}
-
 void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
 {
 /*
@@ -313,41 +233,6 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
 cpu->sve_vq.map = vq_map;
 }
 
-static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
-   void *opaque, Error **errp)
-{
-ARMCPU *cpu = ARM_CPU(obj);
-uint32_t value;
-
-/* All vector lengths are disabled when SVE is off. */
-if (!cpu_isar_feature(aa64_sve, cpu)) {
-value = 0;
-} else {
-value = cpu->sve_max_vq;
-}
-visit_type_uint32(v, name, , errp);
-}
-
-static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
-   void *opaque, Error **errp)
-{
-

[PATCH v11 10/13] arm/Kconfig: Always select SEMIHOSTING when TCG is present

[Fabiano Rosas]

We are about to enable the build without TCG, so CONFIG_SEMIHOSTING
and CONFIG_ARM_COMPATIBLE_SEMIHOSTING cannot be unconditionally set in
default.mak anymore. So reflect the change in a Kconfig.

Instead of using semihosting/Kconfig, use a target-specific file, so
that the change doesn't affect other architectures which might
implement semihosting in a way compatible with KVM.

The selection from ARM_v7M needs to be removed to avoid a cycle during
parsing.

Signed-off-by: Fabiano Rosas 
Reviewed-by: Richard Henderson 
---
 configs/devices/arm-softmmu/default.mak | 2 --
 hw/arm/Kconfig  | 1 -
 target/arm/Kconfig  | 7 +++
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/configs/devices/arm-softmmu/default.mak 
b/configs/devices/arm-softmmu/default.mak
index 1b49a7830c..cb3e5aea65 100644
--- a/configs/devices/arm-softmmu/default.mak
+++ b/configs/devices/arm-softmmu/default.mak
@@ -40,6 +40,4 @@ CONFIG_MICROBIT=y
 CONFIG_FSL_IMX25=y
 CONFIG_FSL_IMX7=y
 CONFIG_FSL_IMX6UL=y
-CONFIG_SEMIHOSTING=y
-CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
 CONFIG_ALLWINNER_H3=y
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index b53bd7f0b2..87c1a29c91 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -317,7 +317,6 @@ config ARM_V7M
 # currently v7M must be included in a TCG build due to translate.c
 default y if TCG && (ARM || AARCH64)
 select PTIMER
-select ARM_COMPATIBLE_SEMIHOSTING
 
 config ALLWINNER_A10
 bool
diff --git a/target/arm/Kconfig b/target/arm/Kconfig
index 3f3394a22b..39f05b6420 100644
--- a/target/arm/Kconfig
+++ b/target/arm/Kconfig
@@ -4,3 +4,10 @@ config ARM
 config AARCH64
 bool
 select ARM
+
+# This config exists just so we can make SEMIHOSTING default when TCG
+# is selected without also changing it for other architectures.
+config ARM_SEMIHOSTING
+bool
+default y if TCG && ARM
+select ARM_COMPATIBLE_SEMIHOSTING
-- 
2.35.3

[PATCH v11 06/13] tests/qtest: Adjust and document query-cpu-model-expansion test for arm

[Fabiano Rosas]

We're about to move the 32-bit CPUs under CONFIG_TCG, so adjust the
query-cpu-model-expansion test to check against the cortex-a7, which
is already under CONFIG_TCG. That allows the next patch to contain
only code movement.

While here add comments clarifying what we're testing.

Signed-off-by: Fabiano Rosas 
Suggested-by: Philippe Mathieu-Daudé 
---
 tests/qtest/arm-cpu-features.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 1cb08138ad..3fc33fc24d 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -506,9 +506,23 @@ static void test_query_cpu_model_expansion_kvm(const void 
*data)
 QDict *resp;
 char *error;
 
-assert_error(qts, "cortex-a15",
-"We cannot guarantee the CPU type 'cortex-a15' works "
-"with KVM on this host", NULL);
+/*
+ * When using KVM, only the 'host' and 'max' CPU models are
+ * supported. Test that we're emitting a suitable error for
+ * unsupported CPU models.
+ */
+if (qtest_has_accel("tcg")) {
+assert_error(qts, "cortex-a7",
+ "We cannot guarantee the CPU type 'cortex-a7' works "
+ "with KVM on this host", NULL);
+} else {
+/*
+ * With a KVM-only build the 32-bit CPUs are not present.
+ */
+assert_error(qts, "cortex-a7",
+ "The CPU type 'cortex-a7' is not a "
+ "recognized ARM CPU type", NULL);
+}
 
 assert_has_feature_enabled(qts, "host", "aarch64");
 
-- 
2.35.3

[PATCH v11 07/13] target/arm: move cpu_tcg to tcg/cpu32.c

[Fabiano Rosas]

From: Claudio Fontana 

move the module containing cpu models definitions
for 32bit TCG-only CPUs to tcg/ and rename it for clarity.

Signed-off-by: Claudio Fontana 
Signed-off-by: Fabiano Rosas 
Reviewed-by: Richard Henderson 
Acked-by: Thomas Huth 
---
 hw/arm/virt.c |  2 --
 target/arm/meson.build|  1 -
 target/arm/{cpu_tcg.c => tcg/cpu32.c} | 13 +++--
 target/arm/tcg/cpu64.c|  2 +-
 target/arm/tcg/meson.build|  1 +
 5 files changed, 5 insertions(+), 14 deletions(-)
 rename target/arm/{cpu_tcg.c => tcg/cpu32.c} (99%)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1450a9f363..b99ae18501 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -206,9 +206,7 @@ static const int a15irqmap[] = {
 static const char *valid_cpus[] = {
 #ifdef CONFIG_TCG
 ARM_CPU_TYPE_NAME("cortex-a7"),
-#endif
 ARM_CPU_TYPE_NAME("cortex-a15"),
-#ifdef CONFIG_TCG
 ARM_CPU_TYPE_NAME("cortex-a35"),
 ARM_CPU_TYPE_NAME("cortex-a55"),
 ARM_CPU_TYPE_NAME("cortex-a72"),
diff --git a/target/arm/meson.build b/target/arm/meson.build
index 3469926295..359a649eaf 100644
--- a/target/arm/meson.build
+++ b/target/arm/meson.build
@@ -5,7 +5,6 @@ arm_ss.add(files(
   'gdbstub.c',
   'helper.c',
   'vfp_helper.c',
-  'cpu_tcg.c',
 ))
 arm_ss.add(zlib)
 
diff --git a/target/arm/cpu_tcg.c b/target/arm/tcg/cpu32.c
similarity index 99%
rename from target/arm/cpu_tcg.c
rename to target/arm/tcg/cpu32.c
index 15aa88e40f..47d2e8e781 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/tcg/cpu32.c
@@ -1,5 +1,5 @@
 /*
- * QEMU ARM TCG CPUs.
+ * QEMU ARM TCG-only CPUs.
  *
  * Copyright (c) 2012 SUSE LINUX Products GmbH
  *
@@ -10,9 +10,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#ifdef CONFIG_TCG
 #include "hw/core/tcg-cpu-ops.h"
-#endif /* CONFIG_TCG */
 #include "internals.h"
 #include "target/arm/idau.h"
 #if !defined(CONFIG_USER_ONLY)
@@ -96,7 +94,7 @@ void aa32_max_features(ARMCPU *cpu)
 /* CPU models. These are not needed for the AArch64 linux-user build. */
 #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
 
-#if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG)
+#if !defined(CONFIG_USER_ONLY)
 static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
 CPUClass *cc = CPU_GET_CLASS(cs);
@@ -120,7 +118,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 }
 return ret;
 }
-#endif /* !CONFIG_USER_ONLY && CONFIG_TCG */
+#endif /* !CONFIG_USER_ONLY */
 
 static void arm926_initfn(Object *obj)
 {
@@ -1014,7 +1012,6 @@ static void pxa270c5_initfn(Object *obj)
 cpu->reset_sctlr = 0x0078;
 }
 
-#ifdef CONFIG_TCG
 static const struct TCGCPUOps arm_v7m_tcg_ops = {
 .initialize = arm_translate_init,
 .synchronize_from_tb = arm_cpu_synchronize_from_tb,
@@ -1035,7 +1032,6 @@ static const struct TCGCPUOps arm_v7m_tcg_ops = {
 .debug_check_breakpoint = arm_debug_check_breakpoint,
 #endif /* !CONFIG_USER_ONLY */
 };
-#endif /* CONFIG_TCG */
 
 static void arm_v7m_class_init(ObjectClass *oc, void *data)
 {
@@ -1043,10 +1039,7 @@ static void arm_v7m_class_init(ObjectClass *oc, void 
*data)
 CPUClass *cc = CPU_CLASS(oc);
 
 acc->info = data;
-#ifdef CONFIG_TCG
 cc->tcg_ops = _v7m_tcg_ops;
-#endif /* CONFIG_TCG */
-
 cc->gdb_core_xml_file = "arm-m-profile.xml";
 }
 
diff --git a/target/arm/tcg/cpu64.c b/target/arm/tcg/cpu64.c
index 646aa46ac9..886674a443 100644
--- a/target/arm/tcg/cpu64.c
+++ b/target/arm/tcg/cpu64.c
@@ -525,7 +525,7 @@ static void aarch64_neoverse_n1_initfn(Object *obj)
 
 /*
  * -cpu max: a CPU with as many features enabled as our emulation supports.
- * The version of '-cpu max' for qemu-system-arm is defined in cpu_tcg.c;
+ * The version of '-cpu max' for qemu-system-arm is defined in cpu32.c;
  * this only needs to handle 64 bits.
  */
 void aarch64_max_tcg_initfn(Object *obj)
diff --git a/target/arm/tcg/meson.build b/target/arm/tcg/meson.build
index 128f782816..4d99f6dacb 100644
--- a/target/arm/tcg/meson.build
+++ b/target/arm/tcg/meson.build
@@ -18,6 +18,7 @@ gen = [
 arm_ss.add(gen)
 
 arm_ss.add(files(
+  'cpu32.c',
   'translate.c',
   'translate-m-nocp.c',
   'translate-mve.c',
-- 
2.35.3

[PATCH v11 08/13] tests/qtest: Fix tests when no KVM or TCG are present

[Fabiano Rosas]

It is possible to have a build with both TCG and KVM disabled due to
Xen requiring the i386 and x86_64 binaries to be present in an aarch64
host.

If we build with --disable-tcg on the aarch64 host, we will end-up
with a QEMU binary (x86) that does not support TCG nor KVM.

Skip tests that crash or hang in the above scenario. Do not include
any test cases if TCG and KVM are missing.

Make sure that calls to qtest_has_accel are placed after g_test_init
in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
printed before other messages") to avoid TAP parsing errors.

Reviewed-by: Juan Quintela 
Reviewed-by: Thomas Huth 
Signed-off-by: Fabiano Rosas 
---
 tests/qtest/bios-tables-test.c | 11 +--
 tests/qtest/boot-serial-test.c |  5 +
 tests/qtest/migration-test.c   |  9 -
 tests/qtest/pxe-test.c |  8 +++-
 tests/qtest/vmgenid-test.c |  9 +++--
 5 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 464f87382e..7fd88b0e9c 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -2045,8 +2045,7 @@ static void test_acpi_virt_oem_fields(void)
 int main(int argc, char *argv[])
 {
 const char *arch = qtest_get_arch();
-const bool has_kvm = qtest_has_accel("kvm");
-const bool has_tcg = qtest_has_accel("tcg");
+bool has_kvm, has_tcg;
 char *v_env = getenv("V");
 int ret;
 
@@ -2056,6 +2055,14 @@ int main(int argc, char *argv[])
 
 g_test_init(, , NULL);
 
+has_kvm = qtest_has_accel("kvm");
+has_tcg = qtest_has_accel("tcg");
+
+if (!has_tcg && !has_kvm) {
+g_test_skip("No KVM or TCG accelerator available");
+return 0;
+}
+
 if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
 ret = boot_sector_init(disk);
 if (ret) {
diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 3aef3a97a9..6dd06aeaf4 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -287,6 +287,11 @@ int main(int argc, char *argv[])
 
 g_test_init(, , NULL);
 
+if (!qtest_has_accel("tcg") && !qtest_has_accel("kvm")) {
+g_test_skip("No KVM or TCG accelerator available");
+return 0;
+}
+
 for (i = 0; tests[i].arch != NULL; i++) {
 if (g_str_equal(arch, tests[i].arch) &&
 qtest_has_machine(tests[i].machine)) {
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 60dd53d3ec..be73ec3c06 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2477,7 +2477,7 @@ static bool kvm_dirty_ring_supported(void)
 
 int main(int argc, char **argv)
 {
-bool has_kvm;
+bool has_kvm, has_tcg;
 bool has_uffd;
 const char *arch;
 g_autoptr(GError) err = NULL;
@@ -2486,6 +2486,13 @@ int main(int argc, char **argv)
 g_test_init(, , NULL);
 
 has_kvm = qtest_has_accel("kvm");
+has_tcg = qtest_has_accel("tcg");
+
+if (!has_tcg && !has_kvm) {
+g_test_skip("No KVM or TCG accelerator available");
+return 0;
+}
+
 has_uffd = ufd_version_check();
 arch = qtest_get_arch();
 
diff --git a/tests/qtest/pxe-test.c b/tests/qtest/pxe-test.c
index 62b6eef464..e4b48225a5 100644
--- a/tests/qtest/pxe-test.c
+++ b/tests/qtest/pxe-test.c
@@ -131,11 +131,17 @@ int main(int argc, char *argv[])
 int ret;
 const char *arch = qtest_get_arch();
 
+g_test_init(, , NULL);
+
+if (!qtest_has_accel("tcg") && !qtest_has_accel("kvm")) {
+g_test_skip("No KVM or TCG accelerator available");
+return 0;
+}
+
 ret = boot_sector_init(disk);
 if(ret)
 return ret;
 
-g_test_init(, , NULL);
 
 if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
 test_batch(x86_tests, false);
diff --git a/tests/qtest/vmgenid-test.c b/tests/qtest/vmgenid-test.c
index efba76e716..324db08c7a 100644
--- a/tests/qtest/vmgenid-test.c
+++ b/tests/qtest/vmgenid-test.c
@@ -165,13 +165,18 @@ int main(int argc, char **argv)
 {
 int ret;
 
+g_test_init(, , NULL);
+
+if (!qtest_has_accel("tcg") && !qtest_has_accel("kvm")) {
+g_test_skip("No KVM or TCG accelerator available");
+return 0;
+}
+
 ret = boot_sector_init(disk);
 if (ret) {
 return ret;
 }
 
-g_test_init(, , NULL);
-
 qtest_add_func("/vmgenid/vmgenid/set-guid",
vmgenid_set_guid_test);
 qtest_add_func("/vmgenid/vmgenid/set-guid-auto",
-- 
2.35.3

[PATCH v11 11/13] arm/Kconfig: Do not build TCG-only boards on a KVM-only build

[Fabiano Rosas]

Move all the CONFIG_FOO=y from default.mak into "default y if TCG"
statements in Kconfig. That way they won't be selected when
CONFIG_TCG=n.

I'm leaving CONFIG_ARM_VIRT in default.mak because it allows us to
keep the two default.mak files not empty and keep aarch64-default.mak
including arm-default.mak. That way we don't surprise anyone that's
used to altering these files.

With this change we can start building with --disable-tcg.

Signed-off-by: Fabiano Rosas 
Reviewed-by: Richard Henderson 
---
 configs/devices/aarch64-softmmu/default.mak |  4 --
 configs/devices/arm-softmmu/default.mak | 37 --
 hw/arm/Kconfig  | 42 -
 3 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/configs/devices/aarch64-softmmu/default.mak 
b/configs/devices/aarch64-softmmu/default.mak
index cf43ac8da1..70e05a197d 100644
--- a/configs/devices/aarch64-softmmu/default.mak
+++ b/configs/devices/aarch64-softmmu/default.mak
@@ -2,7 +2,3 @@
 
 # We support all the 32 bit boards so need all their config
 include ../arm-softmmu/default.mak
-
-CONFIG_XLNX_ZYNQMP_ARM=y
-CONFIG_XLNX_VERSAL=y
-CONFIG_SBSA_REF=y
diff --git a/configs/devices/arm-softmmu/default.mak 
b/configs/devices/arm-softmmu/default.mak
index cb3e5aea65..647fbce88d 100644
--- a/configs/devices/arm-softmmu/default.mak
+++ b/configs/devices/arm-softmmu/default.mak
@@ -4,40 +4,3 @@
 # CONFIG_TEST_DEVICES=n
 
 CONFIG_ARM_VIRT=y
-CONFIG_CUBIEBOARD=y
-CONFIG_EXYNOS4=y
-CONFIG_HIGHBANK=y
-CONFIG_INTEGRATOR=y
-CONFIG_FSL_IMX31=y
-CONFIG_MUSICPAL=y
-CONFIG_MUSCA=y
-CONFIG_CHEETAH=y
-CONFIG_SX1=y
-CONFIG_NSERIES=y
-CONFIG_STELLARIS=y
-CONFIG_STM32VLDISCOVERY=y
-CONFIG_REALVIEW=y
-CONFIG_VERSATILE=y
-CONFIG_VEXPRESS=y
-CONFIG_ZYNQ=y
-CONFIG_MAINSTONE=y
-CONFIG_GUMSTIX=y
-CONFIG_SPITZ=y
-CONFIG_TOSA=y
-CONFIG_Z2=y
-CONFIG_NPCM7XX=y
-CONFIG_COLLIE=y
-CONFIG_ASPEED_SOC=y
-CONFIG_NETDUINO2=y
-CONFIG_NETDUINOPLUS2=y
-CONFIG_OLIMEX_STM32_H405=y
-CONFIG_MPS2=y
-CONFIG_RASPI=y
-CONFIG_DIGIC=y
-CONFIG_SABRELITE=y
-CONFIG_EMCRAFT_SF2=y
-CONFIG_MICROBIT=y
-CONFIG_FSL_IMX25=y
-CONFIG_FSL_IMX7=y
-CONFIG_FSL_IMX6UL=y
-CONFIG_ALLWINNER_H3=y
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 87c1a29c91..2d7c457955 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -35,20 +35,24 @@ config ARM_VIRT
 
 config CHEETAH
 bool
+default y if TCG && ARM
 select OMAP
 select TSC210X
 
 config CUBIEBOARD
 bool
+default y if TCG && ARM
 select ALLWINNER_A10
 
 config DIGIC
 bool
+default y if TCG && ARM
 select PTIMER
 select PFLASH_CFI02
 
 config EXYNOS4
 bool
+default y if TCG && ARM
 imply I2C_DEVICES
 select A9MPCORE
 select I2C
@@ -61,6 +65,7 @@ config EXYNOS4
 
 config HIGHBANK
 bool
+default y if TCG && ARM
 select A9MPCORE
 select A15MPCORE
 select AHCI
@@ -75,6 +80,7 @@ config HIGHBANK
 
 config INTEGRATOR
 bool
+default y if TCG && ARM
 select ARM_TIMER
 select INTEGRATOR_DEBUG
 select PL011 # UART
@@ -87,12 +93,14 @@ config INTEGRATOR
 
 config MAINSTONE
 bool
+default y if TCG && ARM
 select PXA2XX
 select PFLASH_CFI01
 select SMC91C111
 
 config MUSCA
 bool
+default y if TCG && ARM
 select ARMSSE
 select PL011
 select PL031
@@ -104,6 +112,7 @@ config MARVELL_88W8618
 
 config MUSICPAL
 bool
+default y if TCG && ARM
 select OR_IRQ
 select BITBANG_I2C
 select MARVELL_88W8618
@@ -114,18 +123,22 @@ config MUSICPAL
 
 config NETDUINO2
 bool
+default y if TCG && ARM
 select STM32F205_SOC
 
 config NETDUINOPLUS2
 bool
+default y if TCG && ARM
 select STM32F405_SOC
 
 config OLIMEX_STM32_H405
 bool
+default y if TCG && ARM
 select STM32F405_SOC
 
 config NSERIES
 bool
+default y if TCG && ARM
 select OMAP
 select TMP105   # temperature sensor
 select BLIZZARD # LCD/TV controller
@@ -158,12 +171,14 @@ config PXA2XX
 
 config GUMSTIX
 bool
+default y if TCG && ARM
 select PFLASH_CFI01
 select SMC91C111
 select PXA2XX
 
 config TOSA
 bool
+default y if TCG && ARM
 select ZAURUS  # scoop
 select MICRODRIVE
 select PXA2XX
@@ -171,6 +186,7 @@ config TOSA
 
 config SPITZ
 bool
+default y if TCG && ARM
 select ADS7846 # touch-screen controller
 select MAX111X # A/D converter
 select WM8750  # audio codec
@@ -183,6 +199,7 @@ config SPITZ
 
 config Z2
 bool
+default y if TCG && ARM
 select PFLASH_CFI01
 select WM8750
 select PL011 # UART
@@ -190,6 +207,7 @@ config Z2
 
 config REALVIEW
 bool
+default y if TCG && ARM
 imply PCI_DEVICES
 imply PCI_TESTDEV
 imply I2C_DEVICES
@@ -218,6 +236,7 @@ config REALVIEW
 
 config SBSA_REF
 bool
+default y if TCG && AARCH64
 imply PCI_DEVICES
 select AHCI
 select ARM_SMMUV3
@@ -233,11 +252,13 @@ config SBSA_REF
 
 config SABRELITE
 bool
+default y if TCG 

[PATCH v11 13/13] gitlab-ci: Check building KVM-only aarch64 target

[Fabiano Rosas]

From: Philippe Mathieu-Daudé 

Add a manual new job to cross-build the aarch64 target with
only the KVM accelerator enabled (in particular, no TCG).

Re-enable running the similar job on the project Aarch64
custom runner.

Signed-off-by: Philippe Mathieu-Daudé 
Signed-off-by: Fabiano Rosas 
Reviewed-by: Thomas Huth 
---
 .gitlab-ci.d/crossbuilds.yml | 11 +++
 .gitlab-ci.d/custom-runners/ubuntu-22.04-aarch64.yml |  4 
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/.gitlab-ci.d/crossbuilds.yml b/.gitlab-ci.d/crossbuilds.yml
index 61b8ac86ee..da787ea9bf 100644
--- a/.gitlab-ci.d/crossbuilds.yml
+++ b/.gitlab-ci.d/crossbuilds.yml
@@ -196,3 +196,14 @@ cross-arm64-xen-only:
 IMAGE: debian-arm64-cross
 ACCEL: xen
 EXTRA_CONFIGURE_OPTS: --disable-tcg --disable-kvm
+
+# Similar job is run by qemu-project's custom runner by default
+cross-arm64-kvm-only:
+  extends: .cross_accel_build_job
+  needs:
+job: arm64-debian-cross-container
+  variables:
+QEMU_JOB_OPTIONAL: 1
+IMAGE: debian-arm64-cross
+ACCEL: kvm
+EXTRA_CONFIGURE_OPTS: --disable-tcg --disable-xen --without-default-devices
diff --git a/.gitlab-ci.d/custom-runners/ubuntu-22.04-aarch64.yml 
b/.gitlab-ci.d/custom-runners/ubuntu-22.04-aarch64.yml
index 13e14a0f87..c61be46b82 100644
--- a/.gitlab-ci.d/custom-runners/ubuntu-22.04-aarch64.yml
+++ b/.gitlab-ci.d/custom-runners/ubuntu-22.04-aarch64.yml
@@ -115,11 +115,7 @@ ubuntu-22.04-aarch64-notcg:
  - aarch64
  rules:
  - if: '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH =~ 
/^staging/'
-   when: manual
-   allow_failure: true
  - if: "$AARCH64_RUNNER_AVAILABLE"
-   when: manual
-   allow_failure: true
  script:
  - mkdir build
  - cd build
-- 
2.35.3

[PATCH v11 12/13] tests/qtest: Restrict tpm-tis-i2c-test to CONFIG_TCG

[Fabiano Rosas]

The test set -accel tcg, so restrict it to when TCG is present.

Signed-off-by: Fabiano Rosas 
---
 tests/qtest/meson.build | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index cfc66ade6f..48cd35b5b2 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -213,7 +213,8 @@ qtests_aarch64 = \
 ['tpm-tis-device-test', 'tpm-tis-device-swtpm-test'] : []) +   
  \
   (config_all_devices.has_key('CONFIG_XLNX_ZYNQMP_ARM') ? ['xlnx-can-test', 
'fuzz-xlnx-dp-test'] : []) + \
   (config_all_devices.has_key('CONFIG_RASPI') ? ['bcm2835-dma-test'] : []) +  \
-  (config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : 
[]) + \
+  (config_all.has_key('CONFIG_TCG') and
\
+   config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : 
[]) + \
   ['arm-cpu-features',
'numa-test',
'boot-serial-test',
-- 
2.35.3

[PATCH v11 04/13] target/arm: Do not expose all -cpu max features to qtests

[Fabiano Rosas]

We're about to move the TCG-only -cpu max configuration code under
CONFIG_TCG. To be able to do that we need to make sure the qtests
still have some cpu configured even when no other accelerator is
available.

Delineate now what is used with TCG-only and what is also used with
qtests to make the subsequent patches cleaner.

Signed-off-by: Fabiano Rosas 
---
 target/arm/cpu64.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index e9161522b8..6a6a2ece2b 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -25,6 +25,8 @@
 #include "qemu/module.h"
 #include "sysemu/kvm.h"
 #include "sysemu/hvf.h"
+#include "sysemu/qtest.h"
+#include "sysemu/tcg.h"
 #include "kvm_arm.h"
 #include "hvf_arm.h"
 #include "qapi/visitor.h"
@@ -1365,10 +1367,14 @@ static void aarch64_max_initfn(Object *obj)
 return;
 }
 
+if (tcg_enabled() || qtest_enabled()) {
+aarch64_a57_initfn(obj);
+}
+
 /* '-cpu max' for TCG: we currently do this as "A57 with extra things" */
-
-aarch64_a57_initfn(obj);
-aarch64_max_tcg_initfn(obj);
+if (tcg_enabled()) {
+aarch64_max_tcg_initfn(obj);
+}
 }
 
 static const ARMCPUInfo aarch64_cpus[] = {
-- 
2.35.3

[PATCH v11 00/13] target/arm: Allow CONFIG_TCG=n builds

[Fabiano Rosas]

Hi,

Some minor changes:

- new patch to move a test under CONFIG_TCG (broken on master);
- new patch to document the unsupported CPU test (Philippe);
- changed the test skip message when no KVM or TCG are present (Igor).

CI run: https://gitlab.com/farosas/qemu/-/pipelines/849926795

v10:
https://lore.kernel.org/r/20230412121829.14452-1-faro...@suse.de

v9:
https://lore.kernel.org/r/20230313151058.19645-1-faro...@suse.de

v8:
https://lore.kernel.org/r/20230309201434.10831-1-faro...@suse.de

v7 resend:
https://lore.kernel.org/r/20230228192628.26140-1-faro...@suse.de

v7:
https://lore.kernel.org/r/20230223130841.25916-1-faro...@suse.de

v6:
https://lore.kernel.org/r/20230217201150.22032-1-faro...@suse.de

v5 resend:
https://lore.kernel.org/r/20230213202927.28992-1-faro...@suse.de

v5:
https://lore.kernel.org/r/20230120184825.31626-1-faro...@suse.de

v4:
https://lore.kernel.org/r/20230119135424.5417-1-faro...@suse.de

v3:
https://lore.kernel.org/r/20230113140419.4013-1-faro...@suse.de

v2:
https://lore.kernel.org/r/20230109224232.11661-1-faro...@suse.de

v1:
https://lore.kernel.org/r/20230104215835.24692-1-faro...@suse.de

Claudio Fontana (1):
  target/arm: move cpu_tcg to tcg/cpu32.c

Fabiano Rosas (11):
  target/arm: Move cortex sysregs into a separate file
  target/arm: Remove dead code from cpu_max_set_sve_max_vq
  target/arm: Extract TCG -cpu max code into a function
  target/arm: Do not expose all -cpu max features to qtests
  target/arm: Move 64-bit TCG CPUs into tcg/
  tests/qtest: Adjust and document query-cpu-model-expansion test for
arm
  tests/qtest: Fix tests when no KVM or TCG are present
  tests/avocado: Pass parameters to migration test
  arm/Kconfig: Always select SEMIHOSTING when TCG is present
  arm/Kconfig: Do not build TCG-only boards on a KVM-only build
  tests/qtest: Restrict tpm-tis-i2c-test to CONFIG_TCG

Philippe Mathieu-Daudé (1):
  gitlab-ci: Check building KVM-only aarch64 target

 .gitlab-ci.d/crossbuilds.yml  |  11 +
 .../custom-runners/ubuntu-22.04-aarch64.yml   |   4 -
 configs/devices/aarch64-softmmu/default.mak   |   4 -
 configs/devices/arm-softmmu/default.mak   |  39 -
 hw/arm/Kconfig|  43 +-
 hw/arm/virt.c |   6 +-
 target/arm/Kconfig|   7 +
 target/arm/cortex-regs.c  |  69 ++
 target/arm/cpregs.h   |   6 +
 target/arm/cpu64.c| 702 +
 target/arm/internals.h|  10 +-
 target/arm/meson.build|   2 +-
 target/arm/{cpu_tcg.c => tcg/cpu32.c} |  72 +-
 target/arm/tcg/cpu64.c| 723 ++
 target/arm/tcg/meson.build|   2 +
 tests/avocado/migration.py|  83 +-
 tests/qtest/arm-cpu-features.c|  20 +-
 tests/qtest/bios-tables-test.c|  11 +-
 tests/qtest/boot-serial-test.c|   5 +
 tests/qtest/meson.build   |   3 +-
 tests/qtest/migration-test.c  |   9 +-
 tests/qtest/pxe-test.c|   8 +-
 tests/qtest/vmgenid-test.c|   9 +-
 23 files changed, 1016 insertions(+), 832 deletions(-)
 create mode 100644 target/arm/cortex-regs.c
 rename target/arm/{cpu_tcg.c => tcg/cpu32.c} (93%)
 create mode 100644 target/arm/tcg/cpu64.c

-- 
2.35.3

[PATCH v11 09/13] tests/avocado: Pass parameters to migration test

[Fabiano Rosas]

The migration tests are currently broken for an aarch64 host because
the tests pass no 'machine' and 'cpu' options on the QEMU command
line.

Add a separate class to each architecture so that we can specify
'machine' and 'cpu' options instead of relying on defaults.

Add a skip decorator to keep the current behavior of only running
migration tests when the qemu target matches the host architecture.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Fabiano Rosas 
---
 tests/avocado/migration.py | 83 +++---
 1 file changed, 78 insertions(+), 5 deletions(-)

diff --git a/tests/avocado/migration.py b/tests/avocado/migration.py
index 4b25680c50..8b2ec0e3c4 100644
--- a/tests/avocado/migration.py
+++ b/tests/avocado/migration.py
@@ -11,6 +11,8 @@
 
 
 import tempfile
+import os
+
 from avocado_qemu import QemuSystemTest
 from avocado import skipUnless
 
@@ -19,7 +21,7 @@
 from avocado.utils.path import find_command
 
 
-class Migration(QemuSystemTest):
+class MigrationTest(QemuSystemTest):
 """
 :avocado: tags=migration
 """
@@ -62,20 +64,91 @@ def _get_free_port(self):
 self.cancel('Failed to find a free port')
 return port
 
-
-def test_migration_with_tcp_localhost(self):
+def migration_with_tcp_localhost(self):
 dest_uri = 'tcp:localhost:%u' % self._get_free_port()
 self.do_migrate(dest_uri)
 
-def test_migration_with_unix(self):
+def migration_with_unix(self):
 with tempfile.TemporaryDirectory(prefix='socket_') as socket_path:
 dest_uri = 'unix:%s/qemu-test.sock' % socket_path
 self.do_migrate(dest_uri)
 
 @skipUnless(find_command('nc', default=False), "'nc' command not found")
-def test_migration_with_exec(self):
+def migration_with_exec(self):
 """The test works for both netcat-traditional and netcat-openbsd 
packages."""
 free_port = self._get_free_port()
 dest_uri = 'exec:nc -l localhost %u' % free_port
 src_uri = 'exec:nc localhost %u' % free_port
 self.do_migrate(dest_uri, src_uri)
+
+
+@skipUnless('aarch64' in os.uname()[4], "host != target")
+class Aarch64(MigrationTest):
+"""
+:avocado: tags=arch:aarch64
+:avocado: tags=machine:virt
+:avocado: tags=cpu:max
+"""
+
+def test_migration_with_tcp_localhost(self):
+self.migration_with_tcp_localhost()
+
+def test_migration_with_unix(self):
+self.migration_with_unix()
+
+def test_migration_with_exec(self):
+self.migration_with_exec()
+
+
+@skipUnless('x86_64' in os.uname()[4], "host != target")
+class X86_64(MigrationTest):
+"""
+:avocado: tags=arch:x86_64
+:avocado: tags=machine:pc
+:avocado: tags=cpu:qemu64
+"""
+
+def test_migration_with_tcp_localhost(self):
+self.migration_with_tcp_localhost()
+
+def test_migration_with_unix(self):
+self.migration_with_unix()
+
+def test_migration_with_exec(self):
+self.migration_with_exec()
+
+
+@skipUnless('ppc64le' in os.uname()[4], "host != target")
+class PPC64(MigrationTest):
+"""
+:avocado: tags=arch:ppc64
+:avocado: tags=machine:pseries
+:avocado: tags=cpu:power9_v2.0
+"""
+
+def test_migration_with_tcp_localhost(self):
+self.migration_with_tcp_localhost()
+
+def test_migration_with_unix(self):
+self.migration_with_unix()
+
+def test_migration_with_exec(self):
+self.migration_with_exec()
+
+
+@skipUnless('s390x' in os.uname()[4], "host != target")
+class S390X(MigrationTest):
+"""
+:avocado: tags=arch:s390x
+:avocado: tags=machine:s390-ccw-virtio
+:avocado: tags=cpu:qemu
+"""
+
+def test_migration_with_tcp_localhost(self):
+self.migration_with_tcp_localhost()
+
+def test_migration_with_unix(self):
+self.migration_with_unix()
+
+def test_migration_with_exec(self):
+self.migration_with_exec()
-- 
2.35.3

[PATCH v11 03/13] target/arm: Extract TCG -cpu max code into a function

[Fabiano Rosas]

Introduce aarch64_max_tcg_initfn that contains the TCG-only part of
-cpu max configuration. We'll need that to be able to restrict this
code to a TCG-only config in the next patches.

Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
Signed-off-by: Fabiano Rosas 
---
 target/arm/cpu64.c | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index fb5e1b69db..e9161522b8 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -1178,27 +1178,17 @@ static void aarch64_host_initfn(Object *obj)
 #endif
 }
 
-/* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
- * otherwise, a CPU with as many features enabled as our emulation supports.
- * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
+/*
+ * -cpu max: a CPU with as many features enabled as our emulation supports.
+ * The version of '-cpu max' for qemu-system-arm is defined in cpu_tcg.c;
  * this only needs to handle 64 bits.
  */
-static void aarch64_max_initfn(Object *obj)
+static void aarch64_max_tcg_initfn(Object *obj)
 {
 ARMCPU *cpu = ARM_CPU(obj);
 uint64_t t;
 uint32_t u;
 
-if (kvm_enabled() || hvf_enabled()) {
-/* With KVM or HVF, '-cpu max' is identical to '-cpu host' */
-aarch64_host_initfn(obj);
-return;
-}
-
-/* '-cpu max' for TCG: we currently do this as "A57 with extra things" */
-
-aarch64_a57_initfn(obj);
-
 /*
  * Reset MIDR so the guest doesn't mistake our 'max' CPU type for a real
  * one and try to apply errata workarounds or use impdef features we
@@ -1367,6 +1357,20 @@ static void aarch64_max_initfn(Object *obj)
 qdev_property_add_static(DEVICE(obj), _cpu_lpa2_property);
 }
 
+static void aarch64_max_initfn(Object *obj)
+{
+if (kvm_enabled() || hvf_enabled()) {
+/* With KVM or HVF, '-cpu max' is identical to '-cpu host' */
+aarch64_host_initfn(obj);
+return;
+}
+
+/* '-cpu max' for TCG: we currently do this as "A57 with extra things" */
+
+aarch64_a57_initfn(obj);
+aarch64_max_tcg_initfn(obj);
+}
+
 static const ARMCPUInfo aarch64_cpus[] = {
 { .name = "cortex-a35", .initfn = aarch64_a35_initfn },
 { .name = "cortex-a57", .initfn = aarch64_a57_initfn },
-- 
2.35.3

[PATCH v11 02/13] target/arm: Remove dead code from cpu_max_set_sve_max_vq

[Fabiano Rosas]

The sve-max-vq property has been removed from the -cpu max used with
KVM, so code under kvm_enabled in cpu_max_set_sve_max_vq is not
reachable.

Fixes: 0baa21be49 ("target/arm: Make KVM -cpu max exactly like -cpu host")
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
Signed-off-by: Fabiano Rosas 
---
 target/arm/cpu64.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 76891c9288..fb5e1b69db 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -336,12 +336,6 @@ static void cpu_max_set_sve_max_vq(Object *obj, Visitor 
*v, const char *name,
 return;
 }
 
-if (kvm_enabled() && !kvm_arm_sve_supported()) {
-error_setg(errp, "cannot set sve-max-vq");
-error_append_hint(errp, "SVE not supported by KVM on this host\n");
-return;
-}
-
 if (max_vq == 0 || max_vq > ARM_MAX_VQ) {
 error_setg(errp, "unsupported SVE vector length");
 error_append_hint(errp, "Valid sve-max-vq in range [1-%d]\n",
-- 
2.35.3

Re: [PATCH] multifd: Fix the number of channels ready

[Fabiano Rosas]

Juan Quintela  writes:

> We don't wait in the sem when we are doing a sync_main.  Make it wait
> there.  To make things clearer, we mark the channel ready at the
> begining of the thread loop.
>
> Signed-off-by: Juan Quintela 

Reviewed-by: Fabiano Rosas 

Re: [PATCH] multifd: Fix the number of channels ready

[Fabiano Rosas]

Juan Quintela  writes:

> Fabiano Rosas  wrote:
>> Juan Quintela  writes:
>>
>>> We don't wait in the sem when we are doing a sync_main.  Make it wait
>>> there.  To make things clearer, we mark the channel ready at the
>>> begining of the thread loop.
>>
>> So in other words we're estabilishing that "channel ready" means ready
>> to send, regardless of having sent the sync packet. Is that it?
>
> Yeap.
>
> There was a bug (from the beggining) that made the counter always get
> up and up.  This fixes it.
>
> It was always supposed to work this way.

Ah, great. I'm proposing a multifd variant without the sync packet in my
fixed-ram series and moving the channels_ready to the top of the loop
means I can stop issuing an extra qemu_sem_post(>sem) just to skip
the sync packet.

Re: [PATCH 04/21] Hexagon (target/hexagon) Add overrides for allocframe/deallocframe

[Richard Henderson]

On 4/26/23 01:41, Taylor Simpson wrote:

+#ifndef CONFIG_HEXAGON_IDEF_PARSER
+/* frame = ((LR << 32) | FP) ^ (FRAMEKEY << 32)) */
+static void gen_frame_scramble(TCGv_i64 result)
+{
+TCGv_i64 framekey = tcg_temp_new_i64();
+tcg_gen_extu_i32_i64(framekey, hex_gpr[HEX_REG_FRAMEKEY]);
+tcg_gen_shli_i64(framekey, framekey, 32);
+tcg_gen_concat_i32_i64(result, hex_gpr[HEX_REG_FP], hex_gpr[HEX_REG_LR]);
+tcg_gen_xor_i64(result, result, framekey);
+}
+#endif


Better as

tmp = tcg_temp_new_i32();
tcg_gen_xor_i32(tmp, hex_gpr[HEX_REG_LR], hex_gpr[HEX_REG_FRAMEKEY]);
tcg_gen_concat_i32_i64(result, hex_gpr[HEX_REG_FRAMEKEY], tmp);


+#ifndef CONFIG_HEXAGON_IDEF_PARSER
+/* Stack overflow check */
+static void gen_framecheck(TCGv EA, int framesize)
+{
+/* Not modelled in linux-user mode */
+/* Placeholder for system mode */
+}


g_assert_not_reached();


+TCGv_i64 frame = tcg_temp_new_i64();
+tcg_gen_addi_tl(r30, r29, -8);
+gen_frame_scramble(frame);


Perhaps better as

TCGv_i64 frame = gen_frame_scramble();

with the allocation inside the subroutine.

Otherwise,
Reviewed-by: Richard Henderson 


r~

Re: [PATCH] multifd: Fix the number of channels ready

[Juan Quintela]

Fabiano Rosas  wrote:
> Juan Quintela  writes:
>
>> We don't wait in the sem when we are doing a sync_main.  Make it wait
>> there.  To make things clearer, we mark the channel ready at the
>> begining of the thread loop.
>
> So in other words we're estabilishing that "channel ready" means ready
> to send, regardless of having sent the sync packet. Is that it?

Yeap.

There was a bug (from the beggining) that made the counter always get
up and up.  This fixes it.

It was always supposed to work this way.

/me puts (second time in the week) a brown paper bag on head

Later, Juan.

Re: [PATCH 03/21] Hexagon (target/hexagon) Add overrides for loop setup instructions

[Richard Henderson]

On 4/26/23 01:41, Taylor Simpson wrote:

These instructions have implicit writes to registers, so we don't
want them to be helpers when idef-parser is off.

Signed-off-by: Taylor Simpson
---
  target/hexagon/gen_tcg.h | 21 +++
  target/hexagon/genptr.c  | 44 
  2 files changed, 65 insertions(+)


Acked-by: Richard Henderson 

r~

RE: [PATCH 01/21] meson.build Add CONFIG_HEXAGON_IDEF_PARSER

[Taylor Simpson]

> -Original Message-
> From: Richard Henderson 
> Sent: Wednesday, April 26, 2023 12:32 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: phi...@linaro.org; a...@rev.ng; a...@rev.ng; Brian Cain
> ; Matheus Bernardino (QUIC)
> ; pbonz...@redhat.com;
> marcandre.lur...@redhat.com; berra...@redhat.com; th...@redhat.com
> Subject: Re: [PATCH 01/21] meson.build Add
> CONFIG_HEXAGON_IDEF_PARSER
> 
> On 4/26/23 01:40, Taylor Simpson wrote:
> > Enable conditional compilation depending on whether idef-parser is
> > configured
> >
> > Signed-off-by: Taylor Simpson
> > ---
> >   meson.build | 1 +
> >   1 file changed, 1 insertion(+)
> 
> Are you not at the point where you want this unconditionally?
> How long do you intend to keep this optional?

The default is ON, but we want to keep the OFF option working.

Thanks,
Taylor

Re: [PATCH 02/21] Hexagon (target/hexagon) Add DisasContext arg to gen_log_reg_write

[Richard Henderson]

On 4/26/23 01:41, Taylor Simpson wrote:

Add DisasContext arg to gen_log_reg_write_pair also

Signed-off-by: Taylor Simpson
---
  target/hexagon/gen_tcg.h|  2 +-
  target/hexagon/genptr.h |  2 +-
  target/hexagon/genptr.c | 10 +-
  target/hexagon/idef-parser/parser-helpers.c |  2 +-
  target/hexagon/README   |  2 +-
  target/hexagon/gen_tcg_funcs.py |  8 +---
  6 files changed, 14 insertions(+), 12 deletions(-)


Reviewed-by: Richard Henderson 

r~

[PATCH v2] Hexagon (target/hexagon) Additional instructions handled by idef-parser

[Taylor Simpson]

 Changes in v2 
Fix bug in imm_print identified in clang build

Currently, idef-parser skips all floating point instructions.  However,
there are some floating point instructions that can be handled.

The following instructions are now parsed
F2_sfimm_p
F2_sfimm_n
F2_dfimm_p
F2_dfimm_n
F2_dfmpyll
F2_dfmpylh

To make these instructions work, we fix some bugs in parser-helpers.c
gen_rvalue_extend
gen_cast_op
imm_print

Test cases added to tests/tcg/hexagon/fpstuff.c

Signed-off-by: Taylor Simpson 
---
 target/hexagon/idef-parser/parser-helpers.h |  2 +-
 target/hexagon/idef-parser/parser-helpers.c | 37 ++
 tests/tcg/hexagon/fpstuff.c | 54 +
 target/hexagon/gen_idef_parser_funcs.py | 10 +++-
 4 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/target/hexagon/idef-parser/parser-helpers.h 
b/target/hexagon/idef-parser/parser-helpers.h
index 1239d23a6a..7c58087169 100644
--- a/target/hexagon/idef-parser/parser-helpers.h
+++ b/target/hexagon/idef-parser/parser-helpers.h
@@ -80,7 +80,7 @@ void reg_compose(Context *c, YYLTYPE *locp, HexReg *reg, char 
reg_id[5]);
 
 void reg_print(Context *c, YYLTYPE *locp, HexReg *reg);
 
-void imm_print(Context *c, YYLTYPE *locp, HexImm *imm);
+void imm_print(Context *c, YYLTYPE *locp, HexValue *rvalue);
 
 void var_print(Context *c, YYLTYPE *locp, HexVar *var);
 
diff --git a/target/hexagon/idef-parser/parser-helpers.c 
b/target/hexagon/idef-parser/parser-helpers.c
index 86511efb62..deaedbb293 100644
--- a/target/hexagon/idef-parser/parser-helpers.c
+++ b/target/hexagon/idef-parser/parser-helpers.c
@@ -167,8 +167,9 @@ void reg_print(Context *c, YYLTYPE *locp, HexReg *reg)
 EMIT(c, "hex_gpr[%u]", reg->id);
 }
 
-void imm_print(Context *c, YYLTYPE *locp, HexImm *imm)
+void imm_print(Context *c, YYLTYPE *locp, HexValue *rvalue)
 {
+HexImm *imm = >imm;
 switch (imm->type) {
 case I:
 EMIT(c, "i");
@@ -177,7 +178,21 @@ void imm_print(Context *c, YYLTYPE *locp, HexImm *imm)
 EMIT(c, "%ciV", imm->id);
 break;
 case VALUE:
-EMIT(c, "((int64_t) %" PRIu64 "ULL)", (int64_t) imm->value);
+if (rvalue->bit_width == 32) {
+if (rvalue->signedness == UNSIGNED) {
+EMIT(c, "((uint32_t) %" PRIu32 ")", (uint32_t) imm->value);
+}  else {
+EMIT(c, "((int32_t) %" PRId32 ")", (int32_t) imm->value);
+}
+} else if (rvalue->bit_width == 64) {
+if (rvalue->signedness == UNSIGNED) {
+EMIT(c, "((uint64_t) %" PRIu64 "ULL)", (uint64_t) imm->value);
+} else {
+EMIT(c, "((int64_t) %" PRId64 "LL)", (int64_t) imm->value);
+}
+} else {
+g_assert_not_reached();
+}
 break;
 case QEMU_TMP:
 EMIT(c, "qemu_tmp_%" PRIu64, imm->index);
@@ -213,7 +228,7 @@ void rvalue_print(Context *c, YYLTYPE *locp, void *pointer)
   tmp_print(c, locp, >tmp);
   break;
   case IMMEDIATE:
-  imm_print(c, locp, >imm);
+  imm_print(c, locp, rvalue);
   break;
   case VARID:
   var_print(c, locp, >var);
@@ -386,13 +401,10 @@ HexValue gen_rvalue_extend(Context *c, YYLTYPE *locp, 
HexValue *rvalue)
 
 if (rvalue->type == IMMEDIATE) {
 HexValue res = gen_imm_qemu_tmp(c, locp, 64, rvalue->signedness);
-bool is_unsigned = (rvalue->signedness == UNSIGNED);
-const char *sign_suffix = is_unsigned ? "u" : "";
 gen_c_int_type(c, locp, 64, rvalue->signedness);
-OUT(c, locp, " ", , " = ");
-OUT(c, locp, "(", sign_suffix, "int64_t) ");
-OUT(c, locp, "(", sign_suffix, "int32_t) ");
-OUT(c, locp, rvalue, ";\n");
+OUT(c, locp, " ", , " = (");
+gen_c_int_type(c, locp, 64, rvalue->signedness);
+OUT(c, locp, ")", rvalue, ";\n");
 return res;
 } else {
 HexValue res = gen_tmp(c, locp, 64, rvalue->signedness);
@@ -963,7 +975,12 @@ HexValue gen_cast_op(Context *c,
 if (src->bit_width == target_width) {
 return *src;
 } else if (src->type == IMMEDIATE) {
-HexValue res = *src;
+HexValue res;
+if (src->bit_width < target_width) {
+res = gen_rvalue_extend(c, locp, src);
+} else {
+res = *src;
+}
 res.bit_width = target_width;
 res.signedness = signedness;
 return res;
diff --git a/tests/tcg/hexagon/fpstuff.c b/tests/tcg/hexagon/fpstuff.c
index 90ce9a6ef3..28f9397155 100644
--- a/tests/tcg/hexagon/fpstuff.c
+++ b/tests/tcg/hexagon/fpstuff.c
@@ -20,6 +20,7 @@
  */
 
 #include 
+#include 
 
 const int FPINVF_BIT = 1; /* Invalid */
 const int FPINVF = 1 << FPINVF_BIT;
@@ -706,6 +707,57 @@ static void check_float2int_convs()
 check_fpstatus(usr, FPINVF);
 }
 
+static void check_float_consts(void)
+{
+int res32;
+unsigned long long res64;
+
+asm("%0 = 

Re: [PATCH 01/21] meson.build Add CONFIG_HEXAGON_IDEF_PARSER

[Richard Henderson]

On 4/26/23 01:40, Taylor Simpson wrote:

Enable conditional compilation depending on whether idef-parser
is configured

Signed-off-by: Taylor Simpson
---
  meson.build | 1 +
  1 file changed, 1 insertion(+)


Are you not at the point where you want this unconditionally?
How long do you intend to keep this optional?


r~

Re: [PATCH] multifd: Fix the number of channels ready

[Fabiano Rosas]

Juan Quintela  writes:

> We don't wait in the sem when we are doing a sync_main.  Make it wait
> there.  To make things clearer, we mark the channel ready at the
> begining of the thread loop.

So in other words we're estabilishing that "channel ready" means ready
to send, regardless of having sent the sync packet. Is that it?

Re: [PATCH v4] acpi: pcihp: allow repeating hot-unplug requests

[Kashyap Chamarthy]

On Wed, Apr 26, 2023 at 07:40:02PM +0300, Michael Tokarev wrote:
> 18.04.2023 12:04, Igor Mammedov wrote:
> > with Q35 using ACPI PCI hotplug by default, user's request to unplug
> > device is ignored when it's issued before guest OS has been booted.
> > And any additional attempt to request device hot-unplug afterwards
> > results in following error:
> > 
> >"Device XYZ is already in the process of unplug"
> > 
> > arguably it can be considered as a regression introduced by [2],
> > before which it was possible to issue unplug request multiple
> > times.
> 
> Stable-8.0 material?

FWIW, I'd say, yes. This fix is useful for stable releases.  As this
solves a real problem for upper-management tools.

I have tested this fix; and it works.  I'll post my testing notes /
reproducer in a follow-up email.  In short, I followed the
reproducer steps from here[1].

[1] https://gitlab.com/libvirt/libvirt/-/issues/309


-- 
/kashyap

Re: [RFC PATCH v2 3/4] target/riscv: check smstateen fcsr flag

[Mayuresh Chitale]

On Sat, Apr 15, 2023 at 6:55 AM Weiwei Li  wrote:
>
>
> On 2023/4/15 00:02, Mayuresh Chitale wrote:
> > If misa.F and smstateen_fcsr_ok flag are clear then all the floating
> > point instructions must generate an appropriate exception.
> >
> > Signed-off-by: Mayuresh Chitale 
> > ---
> >   target/riscv/insn_trans/trans_rvd.c.inc   | 13 
> >   target/riscv/insn_trans/trans_rvf.c.inc   | 24 +++
> >   target/riscv/insn_trans/trans_rvzfh.c.inc | 18 ++---
> >   3 files changed, 44 insertions(+), 11 deletions(-)
> >
> > diff --git a/target/riscv/insn_trans/trans_rvd.c.inc 
> > b/target/riscv/insn_trans/trans_rvd.c.inc
> > index 2c51e01c40..d9e0cf116f 100644
> > --- a/target/riscv/insn_trans/trans_rvd.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvd.c.inc
> > @@ -18,10 +18,15 @@
> >* this program.  If not, see .
> >*/
> >
> > -#define REQUIRE_ZDINX_OR_D(ctx) do { \
> > -if (!ctx->cfg_ptr->ext_zdinx) { \
> > -REQUIRE_EXT(ctx, RVD); \
> > -} \
> > +#define REQUIRE_ZDINX_OR_D(ctx) do {   \
> > +if (!has_ext(ctx, RVD)) {  \
> > +if (!ctx->cfg_ptr->ext_zdinx) {\
> > +return false;  \
> > +}  \
> > +if (!smstateen_fcsr_check(ctx)) {  \
> > +return false;  \
> > +}  \
> > +}  \
> >   } while (0)
> >
> >   #define REQUIRE_EVEN(ctx, reg) do { \
> > diff --git a/target/riscv/insn_trans/trans_rvf.c.inc 
> > b/target/riscv/insn_trans/trans_rvf.c.inc
> > index 9e9fa2087a..6bf6fe16be 100644
> > --- a/target/riscv/insn_trans/trans_rvf.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvf.c.inc
> > @@ -24,10 +24,26 @@
> >   return false; \
> >   } while (0)
> >
> > -#define REQUIRE_ZFINX_OR_F(ctx) do {\
> > -if (!ctx->cfg_ptr->ext_zfinx) { \
> > -REQUIRE_EXT(ctx, RVF); \
> > -} \
> > +static inline bool smstateen_fcsr_check(DisasContext *ctx)
> > +{
> > +#ifndef CONFIG_USER_ONLY
> > +if (!has_ext(ctx, RVF) && !ctx->smstateen_fcsr_ok) {
>
> We needn't check RVF here. Two reasons:
>
> 1. This check only be done when RVF is diabled.
>
> 2. ctx->smstateen_fcsr_ok is always be true (set in last patch) when RVF
> is enabled

Ok.
>
> > +ctx->virt_inst_excp = ctx->virt_enabled;
> > +return false;
> > +}
> > +#endif
> > +return true;
> > +}
> > +
> > +#define REQUIRE_ZFINX_OR_F(ctx) do {   \
> > +if (!has_ext(ctx, RVF)) {  \
> > +if (!ctx->cfg_ptr->ext_zfinx) {\
> > +return false;  \
> > +}  \
> > +if (!smstateen_fcsr_check(ctx)) {  \
> > +return false;  \
> > +}  \
> > +}  \
> >   } while (0)
> >
> >   #define REQUIRE_ZCF(ctx) do {  \
> > diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc 
> > b/target/riscv/insn_trans/trans_rvzfh.c.inc
> > index 74dde37ff7..74a125e4c0 100644
> > --- a/target/riscv/insn_trans/trans_rvzfh.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
> > @@ -16,28 +16,40 @@
> >* this program.  If not, see .
> >*/
> >
> > -#define REQUIRE_ZFH(ctx) do { \
> > +#define REQUIRE_ZFH(ctx) do {  \
> >   if (!ctx->cfg_ptr->ext_zfh) {  \
> > -return false; \
> > -} \
> > +return false;  \
> > +}  \
> > +if (!smstateen_fcsr_check(ctx)) {  \
> > +return false;  \
> > +}  \
>
> This is unnecessary here. This check is only for Zhinx.
>
> Similar to REQUIRE_ZFHMIN.
>
> >   } while (0)
> >
> >   #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
> >   if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
> >   return false;  \
> >   }  \
> > +if (!smstateen_fcsr_check(ctx)) {  \
> > +return false;  \
> > +}  \
>
> This check is only for Zhinx here. So it's better to take the similar
> way as REQUIRE_ZFINX_OR_F.
>
> Similar to REQUIRE_ZFHMIN_OR_ZHINXMIN.

I am not sure I follow the comments above.
The FCSR check is applicable to all the extensions ie zfinx, zdinx,
zhinx and zhinxmin.
>
> Regards,
>
> Weiwei Li
>
> >   } while (0)
> >
> >   #define REQUIRE_ZFHMIN(ctx) do {  \
> >   if (!ctx->cfg_ptr->ext_zfhmin) {  \
> >   return false; \
> >   } \
> > +if (!smstateen_fcsr_check(ctx)) { \
> > +return 

Re: [RFC PATCH v2 2/4] target/riscv: Reuse TB_FLAGS.MSTATUS_HFS_FS

[Mayuresh Chitale]

On Sat, Apr 15, 2023 at 7:15 AM Weiwei Li  wrote:
>
>
> On 2023/4/15 00:02, Mayuresh Chitale wrote:
> > When misa.F is clear, TB_FLAGS.MSTATUS_HS_FS field is unused and can
> > be used to save the current state of smstateen0.FCSR check which is
> > needed by the floating point translation routines.
> >
> > Signed-off-by: Mayuresh Chitale 
> > ---
> >   target/riscv/cpu_helper.c | 12 
> >   target/riscv/translate.c  |  7 +++
> >   2 files changed, 19 insertions(+)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 433ea529b0..fd1731cc39 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -105,6 +105,18 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, 
> > target_ulong *pc,
> >   flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_VS,
> >  get_field(env->mstatus_hs, MSTATUS_VS));
> >   }
> > +/*
> > + * If misa.F is 0 then the MSTATUS_HS_FS field of the tb->flags
> > + * can be used to pass the current state of the smstateen.FCSR bit
> > + * which must be checked for in the floating point translation routines
> > + */
> > +if (!riscv_has_ext(env, RVF)) {
> > +if (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR) == RISCV_EXCP_NONE) {
> > +flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_FS, 1);
> > +} else {
> > +flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_FS, 0);
> > +}
> > +}
> >   if (cpu->cfg.debug && !icount_enabled()) {
> >   flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, 
> > env->itrigger_enabled);
> >   }
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index d0094922b6..e29bbb8b70 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -79,6 +79,7 @@ typedef struct DisasContext {
> >   int frm;
> >   RISCVMXL ol;
> >   bool virt_inst_excp;
> > +bool smstateen_fcsr_ok;
> >   bool virt_enabled;
> >   const RISCVCPUConfig *cfg_ptr;
> >   bool hlsx;
> > @@ -1202,6 +1203,12 @@ static void 
> > riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> >   ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
> >   ctx->zero = tcg_constant_tl(0);
> >   ctx->virt_inst_excp = false;
> > +if (has_ext(ctx, RVF)) {
> > +ctx->smstateen_fcsr_ok = 1;
> > +} else {
> > +ctx->smstateen_fcsr_ok = FIELD_EX32(tb_flags, TB_FLAGS,
> > + MSTATUS_HS_FS);
>
> By the way, it may introduce new question when MSTATUS_FS and
> MSTATUS_HS_FS is merged to save bits in tb_flag
>
> by Richerd's patchset: 20230412114333.118895-5-richard.hender...@linaro.org
>
> such as: the check "s->mstatus_fs == 0" in require_rvf() will be false
> if smstateen_fcsr_ok is true.
>
> However, this should be true in this case to indicate F is diabled.
>
> So we may need to set ctx->mstatus_fs = 0 here once merged with
> Richerd's patchset.

Yes, that is correct.
>
> Regards,
>
> Weiwei Li
>
> > +}
> >   }
> >
> >   static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
>

Re: [RFC PATCH v2 1/4] target/riscv: smstateen check for fcsr

[Mayuresh Chitale]

On Sat, Apr 15, 2023 at 6:32 AM Weiwei Li  wrote:
>
>
> On 2023/4/15 00:01, Mayuresh Chitale wrote:
> > If smstateen is implemented and smtateen0.fcsr is clear and misa.F
> > is off then the floating point operations must return illegal
> > instruction exception or virtual instruction trap, if relevant.
> >
> > Signed-off-by: Mayuresh Chitale 
> > ---
> >   target/riscv/csr.c | 23 +++
> >   1 file changed, 23 insertions(+)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index f4d2dcfdc8..8ae9e95f9f 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -82,6 +82,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
> >   !riscv_cpu_cfg(env)->ext_zfinx) {
> >   return RISCV_EXCP_ILLEGAL_INST;
> >   }
> > +
> > +if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> > +return smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR);
> > +}
> >   #endif
> >   return RISCV_EXCP_NONE;
> >   }
> > @@ -2081,6 +2085,9 @@ static RISCVException write_mstateen0(CPURISCVState 
> > *env, int csrno,
> > target_ulong new_val)
> >   {
> >   uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> > +if (!riscv_has_ext(env, RVF)) {
> > +wr_mask |= SMSTATEEN0_FCSR;
> > +}
> >
> >   return write_mstateen(env, csrno, wr_mask, new_val);
> >   }
> > @@ -2117,6 +2124,10 @@ static RISCVException write_mstateen0h(CPURISCVState 
> > *env, int csrno,
> >   {
> >   uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +if (!riscv_has_ext(env, RVF)) {
> > +wr_mask |= SMSTATEEN0_FCSR;
> > +}
> > +
>
> FCSR is bit 1 of mstateen0.  So it seems unnecessary to be added to
> wr_mask for mstateen0h.
>
> Similar to hstateen0h.
>
> Otherwise,  Reviewed-by: Weiwei Li 
>
Thanks, I will make the change above in the next version.
> Weiwei Li
> >   return write_mstateenh(env, csrno, wr_mask, new_val);
> >   }
> >
> > @@ -2154,6 +2165,10 @@ static RISCVException write_hstateen0(CPURISCVState 
> > *env, int csrno,
> >   {
> >   uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +if (!riscv_has_ext(env, RVF)) {
> > +wr_mask |= SMSTATEEN0_FCSR;
> > +}
> > +
> >   return write_hstateen(env, csrno, wr_mask, new_val);
> >   }
> >
> > @@ -2193,6 +2208,10 @@ static RISCVException write_hstateen0h(CPURISCVState 
> > *env, int csrno,
> >   {
> >   uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +if (!riscv_has_ext(env, RVF)) {
> > +wr_mask |= SMSTATEEN0_FCSR;
> > +}
> > +
> >   return write_hstateenh(env, csrno, wr_mask, new_val);
> >   }
> >
> > @@ -2240,6 +2259,10 @@ static RISCVException write_sstateen0(CPURISCVState 
> > *env, int csrno,
> >   {
> >   uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +if (!riscv_has_ext(env, RVF)) {
> > +wr_mask |= SMSTATEEN0_FCSR;
> > +}
> > +
> >   return write_sstateen(env, csrno, wr_mask, new_val);
> >   }
> >
>

Re: [PATCH] scsi: check inquiry buffer length to prevent crash

[Théo Maillart]

Le mer. 26 avr. 2023 à 15:38, Théo Maillart  a écrit :

> Using linux 6.x guest, at boot time, an inquiry makes qemu crash.
> Here is a trace of the scsi inquiry in question:
>
> scsi_req_parsed target 1 lun 0 tag 0x2cffb48 command 18 dir 1 length 4
> scsi_req_parsed_lba target 1 lun 0 tag 0x2cffb48 command 18 lba 110592
> scsi_req_alloc target 1 lun 0 tag 0x2cffb48
> scsi_inquiry target 1 lun 0 tag 0x2cffb48 page 0x01/0xb0
> scsi_generic_send_command Command: data= 0x12 0x01 0xb0 0x00 0x04 0x00
> scsi_req_continue target 1 lun 0 tag 0x2cffb48
> scsi_generic_read_data scsi_read_data tag=0x2cffb48
> scsi_generic_aio_sgio_command generic aio sgio: tag=0x2cffb48 cmd=0x12
> timeout=3
> scsi_generic_read_complete Data ready tag=0x2cffb48 len=4
> scsi_req_data target 1 lun 0 tag 0x2cffb48 len 4
> scsi_req_continue target 1 lun 0 tag 0x2cffb48
> scsi_generic_read_data scsi_read_data tag=0x2cffb48
> scsi_generic_command_complete_noio Command complete 0x7fb0870b80
> tag=0x2cffb48 status=0
> scsi_req_dequeue target 1 lun 0 tag 0x2cffb48
>
> And here is a backtrace from the crash:
>
>  #0  0x007face68580 in a_crash () at ./src/internal/atomic.h:250
>  #1  get_nominal_size (end=0x7f6758f92c "", p=0x7f6758f920 "") at
> src/malloc/mallocng/meta.h:168
>  #2  __libc_free (p=0x7f6758f920) at src/malloc/mallocng/free.c:110
>  #3  0x005592f93ed8 in scsi_free_request (req=0x7fac2c6b50) at
> ../hw/scsi/scsi-generic.c:70
>  #4  0x005592f869b8 in scsi_req_unref (req=0x7fac2c6b50) at
> ../hw/scsi/scsi-bus.c:1382
>  #5  0x005592f94b7c in scsi_read_complete (opaque=0x7fac2c6b50, ret=0)
> at ../hw/scsi/scsi-generic.c:354
>  #6  0x005593659b90 in blk_aio_complete (acb=0x7f66c206a0) at
> ../block/block-backend.c:1527
>  #7  0x00559365a3c8 in blk_aio_ioctl_entry (opaque=0x7f66c206a0) at
> ../block/block-backend.c:1735
>  #8  0x0055937dee64 in coroutine_bootstrap (self=0x7f672f77e0,
> co=0x7f672f77e0) at ../util/coroutine-sigaltstack.c:104
>  #9  0x0055937deed8 in coroutine_trampoline (signal=12) at
> ../util/coroutine-sigaltstack.c:145
>  #10 
>  #11 __cp_end () at src/thread/aarch64/syscall_cp.s:30
>  #12 0x007facea8214 in __syscall_cp_c (nr=133, u=,
> v=, w=, x=,
>  y=, z=) at
> src/thread/pthread_cancel.c:33
>  #13 0x007facefa020 in ?? ()
>
> Signed-off-by: Théo Maillart 
> ---
>  hw/scsi/scsi-generic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index ac9fa662b4..25246589b7 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -191,7 +191,7 @@ static int scsi_handle_inquiry_reply(SCSIGenericReq
> *r, SCSIDevice *s, int len)
>  if ((s->type == TYPE_DISK || s->type == TYPE_ZBC) &&
>  (r->req.cmd.buf[1] & 0x01)) {
>  page = r->req.cmd.buf[2];
> -if (page == 0xb0) {
> +if (page == 0xb0 && r->buflen >= 12) {


Actually the test should be r->buflen > 12


>  uint64_t max_transfer = calculate_max_transfer(s);
>  stl_be_p(>buf[8], max_transfer);
>  /* Also take care of the opt xfer len. */
> --
> 2.40.0
>
>

Re: [RFC] hw/arm/virt: Provide DT binding generation for PCI eXpander Bridges

[Jonathan Cameron via]

On Tue, 25 Apr 2023 21:15:25 +0100
Peter Maydell  wrote:

> On Tue, 25 Apr 2023 at 18:37, Jonathan Cameron
>  wrote:
> > We could explore only solving the problem for pxb-cxl for now.
> > However, we would still be talking infrastructure in kernel only
> > to support emulated CXL devices and I can see that being
> > controversial. A normal CXL host bridge is not something
> > we can enumerate.  
> 
> Hmm, so what is real hardware doing that our emulation is not?

For real hardware, the host bridges would not typically share
the various windows.  Various options, but most likely they
would be in different PCI segments, with separate ECAM space,
and separate space into which the BARs would be mapped.
That separation would be needed as the Physical Address
routing in the host would need to direct the reads and
writes to the correct bit of hardware and that tends to
be done with very simple address decoders on the appropriate
interconnects - those internal routing tables are normally
effectively fixed for a given system.  We could add another
full host bridge to arm-virt.  That would require separate
emulation as I don't think we can reuse the pxb-cxl stuff.

The PXB approach is to ignore the normal restrictions on
routing being fairly fixed, by building a fixed configuration
before the OS sees it - allowing different host bridges to use
different parts of a single region.  Arguably very early
boot firmware could do that with a physical system but it
would involve some nasty impdef programming of address decoder
logic.  This would be similar to what firmware does to
change the routing dependent on whether it finds a 2 socket
confirmation or a 4 socket configuration in servers. Want
entity would do this is implementation defined - may well
be before any application processor boots.

Jonathan

> 
> -- PMM

Re: [PATCH v8 1/3] multifd: Create property multifd-flush-after-each-section

[Juan Quintela]

Peter Xu  wrote:
> On Tue, Apr 25, 2023 at 06:31:12PM +0200, Juan Quintela wrote:
>> We used to flush all channels at the end of each RAM section
>> sent.  That is not needed, so preparing to only flush after a full
>> iteration through all the RAM.
>> 
>> Default value of the property is false.  But we return "true" in
>> migrate_multifd_flush_after_each_section() until we implement the code
>> in following patches.
>> 
>> Signed-off-by: Juan Quintela 
>> Reviewed-by: Dr. David Alan Gilbert 
>> Signed-off-by: Juan Quintela 
>
> PS: Normally I'll just keep the last Sign-off-by for each person. :)

And here we are again O:-)

I have a hook to put that in.  And at some point it did the wrong thing
(i.e. this), and I always forgot to look into old series for this error.

Sorry, fixed.


>> 
>> ---
>> 
>> Rename each-iteration to after-each-section
>> Rename multifd-sync-after-each-section to
>>multifd-flush-after-each-section
>> ---
>>  hw/core/machine.c |  1 +
>>  migration/migration.c | 13 +
>>  migration/migration.h | 13 +
>>  3 files changed, 27 insertions(+)
>> 
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 2ce97a5d3b..32bd9277b3 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -60,6 +60,7 @@ const size_t hw_compat_7_1_len = 
>> G_N_ELEMENTS(hw_compat_7_1);
>>  GlobalProperty hw_compat_7_0[] = {
>>  { "arm-gicv3-common", "force-8-bit-prio", "on" },
>>  { "nvme-ns", "eui64-default", "on"},
>> +{ "migration", "multifd-flush-after-each-section", "on"},
>>  };
>
> Here we need hw_compat_8_0 instead?

Good catch.

Changed.

Re: [PATCH v4] acpi: pcihp: allow repeating hot-unplug requests

[Michael Tokarev]

18.04.2023 12:04, Igor Mammedov wrote:

with Q35 using ACPI PCI hotplug by default, user's request to unplug
device is ignored when it's issued before guest OS has been booted.
And any additional attempt to request device hot-unplug afterwards
results in following error:

   "Device XYZ is already in the process of unplug"

arguably it can be considered as a regression introduced by [2],
before which it was possible to issue unplug request multiple
times.


Stable-8.0 material?

Thanks,

/mjt