Re: [PATCH] tcg/tcg: Avoid TS_DEAD for basic block ending

[LIU Zhiwei]

On 2023/3/21 14:06, Richard Henderson wrote:

On 3/20/23 21:53, LIU Zhiwei wrote:
TS_DEAD means we will release the register allocated for this 
temporary. But

at basic block ending, we can still use the allocted register.

Signed-off-by: LIU Zhiwei 


Test case?


I have run an Ubuntu image after this patch. It can boot.

But I can't find a direct test case.  Because the IRs supported with 
flags TCG_OPF_BB_END do not have  input or output parameter, such as the 
set_label or br.


Best Regards,
Zhiwei




r~


---
  tcg/tcg.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index bb52bc060b..0c93e6e6f8 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2822,7 +2822,7 @@ static void la_bb_end(TCGContext *s, int ng, 
int nt)

  case TEMP_FIXED:
  case TEMP_GLOBAL:
  case TEMP_TB:
-    state = TS_DEAD | TS_MEM;
+    state = TS_MEM;
  break;
  case TEMP_EBB:
  case TEMP_CONST:

[PATCH] target/riscv: reduce overhead of MSTATUS_SUM change

[fei2 . wu]

From: Fei Wu 

Kernel needs to access user mode memory e.g. during syscalls, the window
is usually opened up for a very limited time through MSTATUS.SUM, the
overhead is too much if tlb_flush() gets called for every SUM change.
This patch saves addresses accessed when SUM=1, and flushs only these
pages when SUM changes to 0. If the buffer is not large enough to save
all the pages during SUM=1, it will fall back to tlb_flush when
necessary.

The buffer size is set to 4 since in this MSTATUS.SUM open-up window,
most of the time kernel accesses 1 or 2 pages, it's very rare to see
more than 4 pages accessed.

It's not necessary to save/restore these new added status, as
tlb_flush() is always called after restore.

Result of 'pipe 10' from unixbench boosts from 223656 to 1327407. Many
other syscalls benefit a lot from this one too.

Signed-off-by: Fei Wu 
Reviewed-by: LIU Zhiwei 
---
 target/riscv/cpu.h|  4 
 target/riscv/cpu_helper.c |  7 +++
 target/riscv/csr.c| 14 +-
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..926dbce59f 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -383,6 +383,10 @@ struct CPUArchState {
 uint64_t kvm_timer_compare;
 uint64_t kvm_timer_state;
 uint64_t kvm_timer_frequency;
+
+#define MAX_CACHED_SUM_U_ADDR_NUM 4
+uint64_t sum_u_count;
+uint64_t sum_u_addr[MAX_CACHED_SUM_U_ADDR_NUM];
 };
 
 OBJECT_DECLARE_CPU_TYPE(RISCVCPU, RISCVCPUClass, RISCV_CPU)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f88c503cf4..5ad0418eb6 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1068,6 +1068,13 @@ restart:
 (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
 *prot |= PAGE_WRITE;
 }
+if ((pte & PTE_U) && (mode & PRV_S) &&
+get_field(env->mstatus, MSTATUS_SUM)) {
+if (env->sum_u_count < MAX_CACHED_SUM_U_ADDR_NUM) {
+env->sum_u_addr[env->sum_u_count] = addr;
+}
+++env->sum_u_count;
+}
 return TRANSLATE_SUCCESS;
 }
 }
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ab566639e5..74b7638c8a 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1246,9 +1246,21 @@ static RISCVException write_mstatus(CPURISCVState *env, 
int csrno,
 
 /* flush tlb on mstatus fields that affect VM */
 if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
-MSTATUS_MPRV | MSTATUS_SUM)) {
+MSTATUS_MPRV)) {
 tlb_flush(env_cpu(env));
+env->sum_u_count = 0;
+} else if ((mstatus & MSTATUS_SUM) && !(val & MSTATUS_SUM)) {
+if (env->sum_u_count > MAX_CACHED_SUM_U_ADDR_NUM) {
+tlb_flush(env_cpu(env));
+} else {
+for (int i = 0; i < env->sum_u_count; ++i) {
+tlb_flush_page_by_mmuidx(env_cpu(env), env->sum_u_addr[i],
+ 1 << PRV_S | 1 << PRV_M);
+}
+}
+env->sum_u_count = 0;
 }
+
 mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
 MSTATUS_SPP | MSTATUS_MPRV | MSTATUS_SUM |
 MSTATUS_MPP | MSTATUS_MXR | MSTATUS_TVM | MSTATUS_TSR |
-- 
2.25.1

[PATCH] hw/acpi/cxl: Drop device-memory support from CFMWS entries

[Dan Williams]

While it was a reasonable idea to specify no window restricitions at the
outset of the CXL emulation support, it turns out that in practice a
platform will never follow the QEMU example of specifying simultaneous
support for HDM-H and HDM-D[B] in a single window.

HDM-D mandates extra bus cycles for host/device bias protocol, and HDM-DB
mandates extra bus cycles for back-invalidate protocol, so hardware must
be explicitly prepared for device-memory unlike host-only memory
(HDM-H).

In preparation for the kernel dropping support for windows that do not
select between device and host-only memory, move QEMU exclusively to
declaring host-only windows.

Signed-off-by: Dan Williams 
---
 hw/acpi/cxl.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
index 2bf8c0799359..defb289e2fef 100644
--- a/hw/acpi/cxl.c
+++ b/hw/acpi/cxl.c
@@ -103,8 +103,8 @@ static void cedt_build_cfmws(GArray *table_data, CXLState 
*cxls)
 /* Host Bridge Interleave Granularity */
 build_append_int_noprefix(table_data, fw->enc_int_gran, 4);
 
-/* Window Restrictions */
-build_append_int_noprefix(table_data, 0x0f, 2); /* No restrictions */
+/* Window Restrictions: Host-only ram and pmem */
+build_append_int_noprefix(table_data, 0x0e, 2);
 
 /* QTG ID */
 build_append_int_noprefix(table_data, 0, 2);

Re: [PATCH] tcg/tcg: Avoid TS_DEAD for basic block ending

[Richard Henderson]

On 3/20/23 21:53, LIU Zhiwei wrote:

TS_DEAD means we will release the register allocated for this temporary. But
at basic block ending, we can still use the allocted register.

Signed-off-by: LIU Zhiwei 


Test case?


r~


---
  tcg/tcg.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index bb52bc060b..0c93e6e6f8 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2822,7 +2822,7 @@ static void la_bb_end(TCGContext *s, int ng, int nt)
  case TEMP_FIXED:
  case TEMP_GLOBAL:
  case TEMP_TB:
-state = TS_DEAD | TS_MEM;
+state = TS_MEM;
  break;
  case TEMP_EBB:
  case TEMP_CONST:

Re: [PATCH 08/10] accel/tcg: push i386 specific hacks into handle_cpu_interrupt callback

[Richard Henderson]

On 3/20/23 10:14, Alex Bennée wrote:

This should be a tcg hook, not a sysemu hook, per the previous one.
I would very much like it to never be NULL, but instead your new
common_cpu_handle_interrupt function.


I was trying to figure out how to instantiate a default but ran into
const problems eventually forcing me to give up.


You initialize it for each instance individually, not in one central place.


Why a TCG hook? Do we not process any interrupts for KVM or HVF?


No.


r~

Re: [PATCH] target/riscv: Fix priv version dependency for vector and zfh

[liweiwei]

On 2023/3/21 12:34, LIU Zhiwei wrote:

Vector implicitly enables zve64d, zve64f, zve32f sub extensions. As vector
only requires PRIV_1_10_0, these sub extensions should not require priv version
higher than that.

The same for Zfh.

Signed-off-by: LIU Zhiwei 


LGTM.

Reviewed-by: Weiwei Li 

Weiwei Li


---
  target/riscv/cpu.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1e97473af2..eaf75a00a6 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -84,7 +84,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
  ISA_EXT_DATA_ENTRY(zihintpause, true, PRIV_VERSION_1_10_0, 
ext_zihintpause),
  ISA_EXT_DATA_ENTRY(zawrs, true, PRIV_VERSION_1_12_0, ext_zawrs),
  ISA_EXT_DATA_ENTRY(zfh, true, PRIV_VERSION_1_11_0, ext_zfh),
-ISA_EXT_DATA_ENTRY(zfhmin, true, PRIV_VERSION_1_12_0, ext_zfhmin),
+ISA_EXT_DATA_ENTRY(zfhmin, true, PRIV_VERSION_1_11_0, ext_zfhmin),
  ISA_EXT_DATA_ENTRY(zfinx, true, PRIV_VERSION_1_12_0, ext_zfinx),
  ISA_EXT_DATA_ENTRY(zdinx, true, PRIV_VERSION_1_12_0, ext_zdinx),
  ISA_EXT_DATA_ENTRY(zba, true, PRIV_VERSION_1_12_0, ext_zba),
@@ -104,9 +104,9 @@ static const struct isa_ext_data isa_edata_arr[] = {
  ISA_EXT_DATA_ENTRY(zksed, true, PRIV_VERSION_1_12_0, ext_zksed),
  ISA_EXT_DATA_ENTRY(zksh, true, PRIV_VERSION_1_12_0, ext_zksh),
  ISA_EXT_DATA_ENTRY(zkt, true, PRIV_VERSION_1_12_0, ext_zkt),
-ISA_EXT_DATA_ENTRY(zve32f, true, PRIV_VERSION_1_12_0, ext_zve32f),
-ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_12_0, ext_zve64f),
-ISA_EXT_DATA_ENTRY(zve64d, true, PRIV_VERSION_1_12_0, ext_zve64d),
+ISA_EXT_DATA_ENTRY(zve32f, true, PRIV_VERSION_1_10_0, ext_zve32f),
+ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_10_0, ext_zve64f),
+ISA_EXT_DATA_ENTRY(zve64d, true, PRIV_VERSION_1_10_0, ext_zve64d),
  ISA_EXT_DATA_ENTRY(zvfh, true, PRIV_VERSION_1_12_0, ext_zvfh),
  ISA_EXT_DATA_ENTRY(zvfhmin, true, PRIV_VERSION_1_12_0, ext_zvfhmin),
  ISA_EXT_DATA_ENTRY(zhinx, true, PRIV_VERSION_1_12_0, ext_zhinx),

[PATCH] tcg/tcg: Avoid TS_DEAD for basic block ending

[LIU Zhiwei]

TS_DEAD means we will release the register allocated for this temporary. But
at basic block ending, we can still use the allocted register.

Signed-off-by: LIU Zhiwei 
---
 tcg/tcg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index bb52bc060b..0c93e6e6f8 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2822,7 +2822,7 @@ static void la_bb_end(TCGContext *s, int ng, int nt)
 case TEMP_FIXED:
 case TEMP_GLOBAL:
 case TEMP_TB:
-state = TS_DEAD | TS_MEM;
+state = TS_MEM;
 break;
 case TEMP_EBB:
 case TEMP_CONST:
-- 
2.17.1

Re: [PATCH for-8.1 v3 05/26] target/riscv/cpu.c: add priv_spec validate/disable_exts helpers

[LIU Zhiwei]

On 2023/3/19 4:04, Daniel Henrique Barboza wrote:

We're doing env->priv_spec validation and assignment at the start of
riscv_cpu_realize(), which is fine, but then we're doing a force disable
on extensions that aren't compatible with the priv version.

This second step is being done too early. The disabled extensions might be
re-enabled again in riscv_cpu_validate_set_extensions() by accident.


It is not by accident. We should make sure if two extensions have 
dependency, their privilege
requirements should have the related dependency.  And we should write 
this dependency to the  isa_edata_arr.


I have sent the patch to the mail list to make this comment here clearer.

https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg05361.html

Therefore it doesn't matter when do the priv_check or the 
validate_set_extension. Both work for me.


Based on that patch, this patch looks good to me.

Reviewed-by: LIU Zhiwei 

Zhiwei


  A
better place to put this code is at the end of
riscv_cpu_validate_set_extensions() after all the validations are
completed.

Add a new helper, riscv_cpu_disable_priv_spec_isa_exts(), to disable the
extesions after the validation is done. While we're at it, create a
riscv_cpu_validate_priv_spec() helper to host all env->priv_spec related
validation to unclog riscv_cpu_realize a bit.

Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/cpu.c | 91 --
  1 file changed, 56 insertions(+), 35 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1ee322001b..17b301967c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -833,6 +833,52 @@ static void riscv_cpu_validate_v(CPURISCVState *env, 
RISCVCPUConfig *cfg,
  env->vext_ver = vext_version;
  }
  
+static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)

+{
+CPURISCVState *env = &cpu->env;
+int priv_version = -1;
+
+if (cpu->cfg.priv_spec) {
+if (!g_strcmp0(cpu->cfg.priv_spec, "v1.12.0")) {
+priv_version = PRIV_VERSION_1_12_0;
+} else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.11.0")) {
+priv_version = PRIV_VERSION_1_11_0;
+} else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
+priv_version = PRIV_VERSION_1_10_0;
+} else {
+error_setg(errp,
+   "Unsupported privilege spec version '%s'",
+   cpu->cfg.priv_spec);
+return;
+}
+
+env->priv_ver = priv_version;
+}
+}
+
+static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
+{
+CPURISCVState *env = &cpu->env;
+int i;
+
+/* Force disable extensions if priv spec version does not match */
+for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
+if (isa_ext_is_enabled(cpu, &isa_edata_arr[i]) &&
+(env->priv_ver < isa_edata_arr[i].min_version)) {
+isa_ext_update_enabled(cpu, &isa_edata_arr[i], false);
+#ifndef CONFIG_USER_ONLY
+warn_report("disabling %s extension for hart 0x" TARGET_FMT_lx
+" because privilege spec version does not match",
+isa_edata_arr[i].name, env->mhartid);
+#else
+warn_report("disabling %s extension because "
+"privilege spec version does not match",
+isa_edata_arr[i].name);
+#endif
+}
+}
+}
+
  /*
   * Check consistency between chosen extensions while setting
   * cpu->cfg accordingly, doing a set_misa() in the end.
@@ -1002,6 +1048,12 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU 
*cpu, Error **errp)
  cpu->cfg.ext_zksh = true;
  }
  
+/*

+ * Disable isa extensions based on priv spec after we
+ * validated and set everything we need.
+ */
+riscv_cpu_disable_priv_spec_isa_exts(cpu);
+
  if (cpu->cfg.ext_i) {
  ext |= RVI;
  }
@@ -1131,7 +1183,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
  CPURISCVState *env = &cpu->env;
  RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
  CPUClass *cc = CPU_CLASS(mcc);
-int i, priv_version = -1;
  Error *local_err = NULL;
  
  cpu_exec_realizefn(cs, &local_err);

@@ -1140,40 +1191,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
  return;
  }
  
-if (cpu->cfg.priv_spec) {

-if (!g_strcmp0(cpu->cfg.priv_spec, "v1.12.0")) {
-priv_version = PRIV_VERSION_1_12_0;
-} else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.11.0")) {
-priv_version = PRIV_VERSION_1_11_0;
-} else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
-priv_version = PRIV_VERSION_1_10_0;
-} else {
-error_setg(errp,
-   "Unsupported privilege spec version '%s'",
-   cpu->cfg.priv_spec);
-return;
-}
-}
-
-if (priv_version >= PRIV_VERSION_1_10_0) {
-env->priv_ve

[PATCH] target/riscv: Fix priv version dependency for vector and zfh

[LIU Zhiwei]

Vector implicitly enables zve64d, zve64f, zve32f sub extensions. As vector
only requires PRIV_1_10_0, these sub extensions should not require priv version
higher than that.

The same for Zfh.

Signed-off-by: LIU Zhiwei 
---
 target/riscv/cpu.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1e97473af2..eaf75a00a6 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -84,7 +84,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
 ISA_EXT_DATA_ENTRY(zihintpause, true, PRIV_VERSION_1_10_0, 
ext_zihintpause),
 ISA_EXT_DATA_ENTRY(zawrs, true, PRIV_VERSION_1_12_0, ext_zawrs),
 ISA_EXT_DATA_ENTRY(zfh, true, PRIV_VERSION_1_11_0, ext_zfh),
-ISA_EXT_DATA_ENTRY(zfhmin, true, PRIV_VERSION_1_12_0, ext_zfhmin),
+ISA_EXT_DATA_ENTRY(zfhmin, true, PRIV_VERSION_1_11_0, ext_zfhmin),
 ISA_EXT_DATA_ENTRY(zfinx, true, PRIV_VERSION_1_12_0, ext_zfinx),
 ISA_EXT_DATA_ENTRY(zdinx, true, PRIV_VERSION_1_12_0, ext_zdinx),
 ISA_EXT_DATA_ENTRY(zba, true, PRIV_VERSION_1_12_0, ext_zba),
@@ -104,9 +104,9 @@ static const struct isa_ext_data isa_edata_arr[] = {
 ISA_EXT_DATA_ENTRY(zksed, true, PRIV_VERSION_1_12_0, ext_zksed),
 ISA_EXT_DATA_ENTRY(zksh, true, PRIV_VERSION_1_12_0, ext_zksh),
 ISA_EXT_DATA_ENTRY(zkt, true, PRIV_VERSION_1_12_0, ext_zkt),
-ISA_EXT_DATA_ENTRY(zve32f, true, PRIV_VERSION_1_12_0, ext_zve32f),
-ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_12_0, ext_zve64f),
-ISA_EXT_DATA_ENTRY(zve64d, true, PRIV_VERSION_1_12_0, ext_zve64d),
+ISA_EXT_DATA_ENTRY(zve32f, true, PRIV_VERSION_1_10_0, ext_zve32f),
+ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_10_0, ext_zve64f),
+ISA_EXT_DATA_ENTRY(zve64d, true, PRIV_VERSION_1_10_0, ext_zve64d),
 ISA_EXT_DATA_ENTRY(zvfh, true, PRIV_VERSION_1_12_0, ext_zvfh),
 ISA_EXT_DATA_ENTRY(zvfhmin, true, PRIV_VERSION_1_12_0, ext_zvfhmin),
 ISA_EXT_DATA_ENTRY(zhinx, true, PRIV_VERSION_1_12_0, ext_zhinx),
-- 
2.17.1

Re: [PATCH for-8.1 v3 11/26] target/riscv/cpu.c: set cpu config in set_misa()

[liweiwei]

On 2023/3/19 04:04, Daniel Henrique Barboza wrote:

set_misa() is setting all 'misa' related env states and nothing else.
But other functions, namely riscv_cpu_validate_set_extensions(), uses
the config object to do its job.

This creates a need to set the single letter extensions in the cfg
object to keep both in sync. At this moment this is being done by
register_cpu_props(), forcing every CPU to do a call to this function.

Let's beef up set_misa() and make the function do the sync for us. This
will relieve named CPUs to having to call register_cpu_props(), which
will then be redesigned to a more specialized role next.

Signed-off-by: Daniel Henrique Barboza
---
  target/riscv/cpu.c | 40 
  target/riscv/cpu.h |  4 ++--
  2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 36c55abda0..7841676473 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -236,8 +236,40 @@ const char *riscv_cpu_get_trap_name(target_ulong cause, 
bool async)
  
  static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)

  {
+RISCVCPU *cpu;
+
  env->misa_mxl_max = env->misa_mxl = mxl;
  env->misa_ext_mask = env->misa_ext = ext;
+
+/*
+ * ext = 0 will only be a thing during cpu_init() functions
+ * as a way of setting an extension-agnostic CPU. We do
+ * not support clearing misa_ext* and the ext_N flags in
+ * RISCVCPUConfig in regular circunstances.
+ */
+if (ext == 0) {
+return;
+}
+
+/*
+ * We can't use riscv_cpu_cfg() in this case because it is
+ * a read-only inline and we're going to change the values
+ * of cpu->cfg.
+ */
+cpu = env_archcpu(env);
+
+cpu->cfg.ext_i = ext & RVI;
+cpu->cfg.ext_e = ext & RVE;
+cpu->cfg.ext_m = ext & RVM;
+cpu->cfg.ext_a = ext & RVA;
+cpu->cfg.ext_f = ext & RVF;
+cpu->cfg.ext_d = ext & RVD;
+cpu->cfg.ext_v = ext & RVV;
+cpu->cfg.ext_c = ext & RVC;
+cpu->cfg.ext_s = ext & RVS;
+cpu->cfg.ext_u = ext & RVU;
+cpu->cfg.ext_h = ext & RVH;
+cpu->cfg.ext_j = ext & RVJ;
  }
  
  #ifndef CONFIG_USER_ONLY

@@ -340,7 +372,6 @@ static void riscv_any_cpu_init(Object *obj)
  #endif
  
  env->priv_ver = PRIV_VERSION_LATEST;

-register_cpu_props(obj);
  
  /* inherited from parent obj via riscv_cpu_init() */

  cpu->cfg.ext_ifencei = true;
@@ -368,7 +399,6 @@ static void rv64_sifive_u_cpu_init(Object *obj)
  RISCVCPU *cpu = RISCV_CPU(obj);
  CPURISCVState *env = &cpu->env;
  set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
-register_cpu_props(obj);
  env->priv_ver = PRIV_VERSION_1_10_0;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV39);
@@ -387,7 +417,6 @@ static void rv64_sifive_e_cpu_init(Object *obj)
  RISCVCPU *cpu = RISCV_CPU(obj);
  
  set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);

-register_cpu_props(obj);
  env->priv_ver = PRIV_VERSION_1_10_0;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -472,8 +501,6 @@ static void rv32_sifive_u_cpu_init(Object *obj)
  RISCVCPU *cpu = RISCV_CPU(obj);
  CPURISCVState *env = &cpu->env;
  set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
-
-register_cpu_props(obj);
  env->priv_ver = PRIV_VERSION_1_10_0;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
@@ -492,7 +519,6 @@ static void rv32_sifive_e_cpu_init(Object *obj)
  RISCVCPU *cpu = RISCV_CPU(obj);
  
  set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);

-register_cpu_props(obj);
  env->priv_ver = PRIV_VERSION_1_10_0;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -510,7 +536,6 @@ static void rv32_ibex_cpu_init(Object *obj)
  RISCVCPU *cpu = RISCV_CPU(obj);
  
  set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);

-register_cpu_props(obj);
  env->priv_ver = PRIV_VERSION_1_11_0;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -529,7 +554,6 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
  RISCVCPU *cpu = RISCV_CPU(obj);
  
  set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);

-register_cpu_props(obj);
  env->priv_ver = PRIV_VERSION_1_10_0;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 76f81c6b68..ebe0fff668 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -66,8 +66,8 @@
  #define RV(x) ((target_ulong)1 << (x - 'A'))
  
  /*

- * Consider updating register_cpu_props() when adding
- * new MISA bits here.
+ * Consider updating set_misa() when adding new
+ * MISA bits here.
   */
  #define RVI RV('I')
  #define RVE RV('E') /* E and I are mutually exclusive */


The assignment for ext_c/u/s in rv64_thea

Re: [PATCH for-8.1 v3 26/26] target/riscv: allow write_misa() to enable RVV

[liweiwei]

On 2023/3/19 04:04, Daniel Henrique Barboza wrote:

Allow write_misa() to enable RVV like we did with RVG. We'll need a
riscv_cpu_enable_v() to enable all related misa bits and Z extensions.
This new helper validates the existing 'env' conf by using the existing
riscv_cpu_validate_v(). We'll also check if we'll be able to enable 'F'
by checking for ext_zfinx.

As with RVG, enabling RVV is considered to be a standalone operation in
write_misa(). This means that we'll guarantee that we're not being
inconsistent in riscv_cpu_enable_v() and that we're okay with skipping
regular validation.

Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/cpu.c | 35 +++
  target/riscv/cpu.h |  1 +
  target/riscv/csr.c | 14 ++
  3 files changed, 50 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 73a5fa46ee..9c16b29f27 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -983,6 +983,41 @@ static void riscv_cpu_validate_v(CPURISCVState *env,
  env->vext_ver = vext_version;
  }
  
+target_ulong riscv_cpu_enable_v(RISCVCPU *cpu, Error **errp)

+{
+CPURISCVState *env = &cpu->env;
+RISCVCPUConfig *cfg = &cpu->cfg;
+Error *local_err = NULL;
+
+riscv_cpu_validate_v(env, cfg, &local_err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+return 0;
+}


This check is not necessary, we call this function only when we enable v 
by write_misa, which also have a prerequisite:


V is enabled at the very first. So this check will always be true, since 
the parameter for vector cannot be changed dynamically.


Similar to following check.


+
+if (cpu->cfg.ext_zfinx) {
+error_setg(errp, "Unable to enable V: Zfinx is enabled, "
+ "so F can not be enabled");
+return 0;
+}
+
+cfg->ext_f = true;
+env->misa_ext |= RVF;
+
+cfg->ext_d = true;
+env->misa_ext |= RVD;


We do check V against F/D at first. Why we do this when enable V?

And if we do this,  whether we should also enable F when enable D?



+
+/*
+ * The V vector extension depends on the
+ *  Zve32f, Zve64f and Zve64d extensions.
+ */
+cpu->cfg.ext_zve64d = true;
+cpu->cfg.ext_zve64f = true;
+cpu->cfg.ext_zve32f = true;


This is right, but not necessary in current implementation, since they 
will not be disabled when we disable V.


So we needn't enable them when we re-enable V.


+
+return env->misa_ext;
+}
+
  static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)
  {
  CPURISCVState *env = &cpu->env;
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 3ca1d4903c..45e801d926 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -600,6 +600,7 @@ void riscv_cpu_validate_extensions(RISCVCPU *cpu, uint32_t 
misa_ext,
  void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu, uint32_t misa_ext);
  
  target_ulong riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp);

+target_ulong riscv_cpu_enable_v(RISCVCPU *cpu, Error **errp);
  
  #define cpu_list riscv_cpu_list

  #define cpu_mmu_index riscv_cpu_mmu_index
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 4335398c19..e9e1afc57e 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1395,6 +1395,20 @@ static RISCVException write_misa(CPURISCVState *env, int 
csrno,
  goto commit;
  }
  
+if (val & RVV && !(env->misa_ext & RVV)) {

+/*
+ * If the write wants to enable RVV, only RVV and
+ * its dependencies will be updated in the CSR.
+ */
+val = riscv_cpu_enable_v(cpu, &local_err);
+if (local_err != NULL) {
+return RISCV_EXCP_NONE;
+}
+
+val |= RVV;
+goto commit;
+}
+


So, I think we can just treat V as common extension, and do nothing 
additionally for disabling/re-enabling it.


Regards,

Weiwei Li


  /*
   * This flow is similar to what riscv_cpu_realize() does,
   * with the difference that we will update env->misa_ext

RE: [PATCH 1/2] ui/gtk: use widget size for cursor motion event

[Kasireddy, Vivek]

Hi Erico,

> 
> The gd_motion_event size has some calculations for the cursor position,
> which also take into account things like different size of the
> framebuffer compared to the window size.
> The use of window size makes things more difficult though, as at least
> in the case of Wayland includes the size of ui elements like a menu bar
> at the top of the window. This leads to a wrong position calculation by
> a few pixels.
> Fix it by using the size of the widget, which already returns the size
> of the actual space to render the framebuffer.
> 
> Signed-off-by: Erico Nunes 
> ---
>  ui/gtk.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/ui/gtk.c b/ui/gtk.c
> index fd82e9b1ca..d1b2a80c2b 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -868,7 +868,6 @@ static gboolean gd_motion_event(GtkWidget *widget,
> GdkEventMotion *motion,
>  {
>  VirtualConsole *vc = opaque;
>  GtkDisplayState *s = vc->s;
> -GdkWindow *window;
>  int x, y;
>  int mx, my;
>  int fbh, fbw;
> @@ -881,10 +880,9 @@ static gboolean gd_motion_event(GtkWidget *widget,
> GdkEventMotion *motion,
>  fbw = surface_width(vc->gfx.ds) * vc->gfx.scale_x;
>  fbh = surface_height(vc->gfx.ds) * vc->gfx.scale_y;
> 
> -window = gtk_widget_get_window(vc->gfx.drawing_area);
> -ww = gdk_window_get_width(window);
> -wh = gdk_window_get_height(window);
> -ws = gdk_window_get_scale_factor(window);
> +ww = gtk_widget_get_allocated_width(widget);
> +wh = gtk_widget_get_allocated_height(widget);
[Kasireddy, Vivek] Could you please confirm if this works in X-based compositor
environments as well? Last time I checked (with Fedora 36 and Gnome + X), the
get_allocated_xxx APIs were not accurate in X-based environments. Therefore,
I restricted the above change to Wayland-based environments only:
https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03100.html


Thanks,
Vivek

> +ws = gtk_widget_get_scale_factor(widget);
> 
>  mx = my = 0;
>  if (ww > fbw) {
> --
> 2.39.2
> 

Re: [PATCH for-8.1 v3 25/26] target/riscv: allow write_misa() to enable RVG

[liweiwei]

On 2023/3/19 04:04, Daniel Henrique Barboza wrote:

Allow write_misa() to enable RVG by changing riscv_cpu_enable_g()
slighty: instead of returning void, return the current env->misa_ext
value. This is then retrieved by 'val', which will add the RVG flag
itself, and then we'll skip validation and go right into commiting the
changes.

The reason why it's ok to skip validation in this case is because we're
only allowing RVG (and its associated extensions/Z extensions) to be
enabled in the hart, and riscv_cpu_enable_g() already does its own
validation before enabling itself. Everything else is considered to be
already validated beforehand, so we don't need to repeat ourselves.

Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/cpu.c |  7 +--
  target/riscv/cpu.h |  2 ++
  target/riscv/csr.c | 15 +++
  3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 2d2a354af3..73a5fa46ee 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -281,7 +281,8 @@ static uint32_t 
riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
  return ext;
  }
  
-static void riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)

+
+target_ulong riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)
  {
  CPURISCVState *env = &cpu->env;
  RISCVCPUConfig *cfg = &cpu->cfg;
@@ -289,7 +290,7 @@ static void riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)
  if (cpu->cfg.ext_zfinx) {
  error_setg(errp, "Unable to enable G: Zfinx is enabled, "
   "so F can not be enabled");
-return;
+return 0;
  }
  
  if (!(cfg->ext_i && cfg->ext_m && cfg->ext_a &&

@@ -315,6 +316,8 @@ static void riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)
  cfg->ext_icsr = true;
  cfg->ext_ifencei = true;
  }
+
+return env->misa_ext;
  }
  
  static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index befc3b8fff..3ca1d4903c 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -599,6 +599,8 @@ void riscv_cpu_validate_extensions(RISCVCPU *cpu, uint32_t 
misa_ext,
 Error **errp);
  void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu, uint32_t misa_ext);
  
+target_ulong riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp);

+
  #define cpu_list riscv_cpu_list
  #define cpu_mmu_index riscv_cpu_mmu_index
  
diff --git a/target/riscv/csr.c b/target/riscv/csr.c

index 839862f1a8..4335398c19 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1381,6 +1381,20 @@ static RISCVException write_misa(CPURISCVState *env, int 
csrno,
  val &= RVE;
  }
  
+if (val & RVG && !(env->misa_ext & RVG)) {

+/*
+ * If the write wants to enable RVG, only RVG and
+ * its dependencies will be updated in the CSR.
+ */
+val = riscv_cpu_enable_g(cpu, &local_err);
+if (local_err != NULL) {
+return RISCV_EXCP_NONE;
+}
+
+val |= RVG;


This assignment is not necessary, since RVG is already set in val.

By the way, RVG is still not disabled if any some of included extensions 
are disabled by write_misa.


Regards,

Weiwei Li


+goto commit;
+}
+
  /*
   * This flow is similar to what riscv_cpu_realize() does,
   * with the difference that we will update env->misa_ext
@@ -1396,6 +1410,7 @@ static RISCVException write_misa(CPURISCVState *env, int 
csrno,
  return RISCV_EXCP_NONE;
  }
  
+commit:

  riscv_cpu_commit_cpu_cfg(cpu, val);
  
  if (!(val & RVF)) {

Re: [PATCH v14 4/4] vhost-vdpa: Add support for vIOMMU.

[Jason Wang]

On Tue, Mar 21, 2023 at 12:20 AM Cindy Lu  wrote:
>
> 1. The vIOMMU support will make vDPA can work in IOMMU mode. This
> will fix security issues while using the no-IOMMU mode.
> To support this feature we need to add new functions for IOMMU MR adds and
> deletes.
>
> Also since the SVQ does not support vIOMMU yet, add the check for IOMMU
> in vhost_vdpa_dev_start, if the SVQ and IOMMU enable at the same time
> the function will return fail.
>
> 2. Skip the iova_max check vhost_vdpa_listener_skipped_section(). While
> MR is IOMMU, move this check to  vhost_vdpa_iommu_map_notify()
>
> Verified in vp_vdpa and vdpa_sim_net driver
>
> Signed-off-by: Cindy Lu 
> ---
>  hw/virtio/vhost-vdpa.c | 149 +++--
>  include/hw/virtio/vhost-vdpa.h |  11 +++
>  2 files changed, 152 insertions(+), 8 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 0c8c37e786..b36922b365 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -26,6 +26,7 @@
>  #include "cpu.h"
>  #include "trace.h"
>  #include "qapi/error.h"
> +#include "hw/virtio/virtio-access.h"
>
>  /*
>   * Return one past the end of the end of section. Be careful with uint64_t
> @@ -60,15 +61,22 @@ static bool 
> vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
>   iova_min, section->offset_within_address_space);
>  return true;
>  }
> +/*
> + * While using vIOMMU, sometimes the section will be larger than 
> iova_max,
> + * but the memory that actually maps is smaller, so move the check to
> + * function vhost_vdpa_iommu_map_notify(). That function will use the 
> actual
> + * size that maps to the kernel
> + */
>
> -llend = vhost_vdpa_section_end(section);
> -if (int128_gt(llend, int128_make64(iova_max))) {
> -error_report("RAM section out of device range (max=0x%" PRIx64
> - ", end addr=0x%" PRIx64 ")",
> - iova_max, int128_get64(llend));
> -return true;
> +if (!memory_region_is_iommu(section->mr)) {
> +llend = vhost_vdpa_section_end(section);
> +if (int128_gt(llend, int128_make64(iova_max))) {
> +error_report("RAM section out of device range (max=0x%" PRIx64
> + ", end addr=0x%" PRIx64 ")",
> + iova_max, int128_get64(llend));
> +return true;
> +}
>  }
> -

Unnecessary changes.

>  return false;
>  }
>
> @@ -185,6 +193,118 @@ static void vhost_vdpa_listener_commit(MemoryListener 
> *listener)
>  v->iotlb_batch_begin_sent = false;
>  }
>
> +static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry 
> *iotlb)
> +{
> +struct vdpa_iommu *iommu = container_of(n, struct vdpa_iommu, n);
> +
> +hwaddr iova = iotlb->iova + iommu->iommu_offset;
> +struct vhost_vdpa *v = iommu->dev;
> +void *vaddr;
> +int ret;
> +Int128 llend;
> +
> +if (iotlb->target_as != &address_space_memory) {
> +error_report("Wrong target AS \"%s\", only system memory is allowed",
> + iotlb->target_as->name ? iotlb->target_as->name : 
> "none");
> +return;
> +}
> +RCU_READ_LOCK_GUARD();
> +/* check if RAM section out of device range */
> +llend = int128_add(int128_makes64(iotlb->addr_mask), 
> int128_makes64(iova));
> +if (int128_gt(llend, int128_make64(v->iova_range.last))) {
> +error_report("RAM section out of device range (max=0x%" PRIx64
> + ", end addr=0x%" PRIx64 ")",
> + v->iova_range.last, int128_get64(llend));
> +return;
> +}
> +
> +vhost_vdpa_iotlb_batch_begin_once(v);

Quoted from you answer in V1:

"
the VHOST_IOTLB_BATCH_END message was send by
vhost_vdpa_listener_commit, because we only use
one vhost_vdpa_memory_listener and no-iommu mode will also need to use
this listener, So we still need to add the batch begin here, based on
my testing after the notify function was called,  the listener_commit
function was also called .so it works well in this situation
"

This assumes the map_notify to be called within the memory
transactions which is not necessarily the case.

I think it could be triggered when guest tries to establish a new
mapping in the vIOMMU. In this case there's no memory transactions at
all?

Thanks

Re: [PATCH v14 3/4] vhost-vdpa: Add check for full 64-bit in region delete

[Jason Wang]

On Tue, Mar 21, 2023 at 12:20 AM Cindy Lu  wrote:
>
> The unmap ioctl doesn't accept a full 64-bit span. So need to
> add check for the section's size in vhost_vdpa_listener_region_del().
>
> Signed-off-by: Cindy Lu 

Acked-by: Jason Wang 

Thanks

> ---
>  hw/virtio/vhost-vdpa.c | 20 +++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 92c2413c76..0c8c37e786 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -316,10 +316,28 @@ static void 
> vhost_vdpa_listener_region_del(MemoryListener *listener,
>  vhost_iova_tree_remove(v->iova_tree, *result);
>  }
>  vhost_vdpa_iotlb_batch_begin_once(v);
> +/*
> + * The unmap ioctl doesn't accept a full 64-bit. need to check it
> + */
> +if (int128_eq(llsize, int128_2_64())) {
> +llsize = int128_rshift(llsize, 1);
> +ret = vhost_vdpa_dma_unmap(v, VHOST_VDPA_GUEST_PA_ASID, iova,
> +   int128_get64(llsize));
> +
> +if (ret) {
> +error_report("vhost_vdpa_dma_unmap(%p, 0x%" HWADDR_PRIx ", "
> + "0x%" HWADDR_PRIx ") = %d (%m)",
> + v, iova, int128_get64(llsize), ret);
> +}
> +iova += int128_get64(llsize);
> +}
>  ret = vhost_vdpa_dma_unmap(v, VHOST_VDPA_GUEST_PA_ASID, iova,
> int128_get64(llsize));
> +
>  if (ret) {
> -error_report("vhost_vdpa dma unmap error!");
> +error_report("vhost_vdpa_dma_unmap(%p, 0x%" HWADDR_PRIx ", "
> + "0x%" HWADDR_PRIx ") = %d (%m)",
> + v, iova, int128_get64(llsize), ret);
>  }
>
>  memory_region_unref(section->mr);
> --
> 2.34.3
>

Re: [PATCH for-8.1 v3 15/26] target/riscv/cpu.c: split RVG code from validate_set_extensions()

[liweiwei]

On 2023/3/19 04:04, Daniel Henrique Barboza wrote:

We can set all RVG related extensions during realize time, before
validate_set_extensions() itself. Put it in a separated function so the
validate function already uses the updated state.

Note that we're adding an extra constraint: ext_zfinx is a blocker for
F, which is a requirement to enable G. If zfinx is enabled we'll have to
error out.

Note that we're setting both cfg->ext_N and env->misa_ext bits, instead
of just setting cfg->ext_N. The intention here is to start syncing all
misa_ext operations with its cpu->cfg flags, in preparation to allow for
the validate function to operate using a misa_ext. This doesn't make any
difference for the current code state, but will be a requirement for
write_misa() later on.

Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/cpu.c | 66 +++---
  1 file changed, 51 insertions(+), 15 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 48ad7372b9..110b52712c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -281,6 +281,42 @@ static uint32_t 
riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
  return ext;
  }
  
+static void riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)

+{
+CPURISCVState *env = &cpu->env;
+RISCVCPUConfig *cfg = &cpu->cfg;
+
+if (cpu->cfg.ext_zfinx) {
+error_setg(errp, "Unable to enable G: Zfinx is enabled, "
+ "so F can not be enabled");
+return;
+}


This check is not very necessary here, since check Zfinx against F will 
be done in following code.


Regards,

Weiwei Li


+
+if (!(cfg->ext_i && cfg->ext_m && cfg->ext_a &&
+  cfg->ext_f && cfg->ext_d &&
+  cfg->ext_icsr && cfg->ext_ifencei)) {
+
+warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
+cfg->ext_i = true;
+env->misa_ext |= RVI;
+
+cfg->ext_m = true;
+env->misa_ext |= RVM;
+
+cfg->ext_a = true;
+env->misa_ext |= RVA;
+
+cfg->ext_f = true;
+env->misa_ext |= RVF;
+
+cfg->ext_d = true;
+env->misa_ext |= RVD;
+
+cfg->ext_icsr = true;
+cfg->ext_ifencei = true;
+}
+}
+
  static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
 uint32_t misa_ext)
  {
@@ -1036,21 +1072,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU 
*cpu, Error **errp)
  return;
  }
  
-/* Do some ISA extension error checking */

-if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m &&
-cpu->cfg.ext_a && cpu->cfg.ext_f &&
-cpu->cfg.ext_d &&
-cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
-warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
-cpu->cfg.ext_i = true;
-cpu->cfg.ext_m = true;
-cpu->cfg.ext_a = true;
-cpu->cfg.ext_f = true;
-cpu->cfg.ext_d = true;
-cpu->cfg.ext_icsr = true;
-cpu->cfg.ext_ifencei = true;
-}
-
  if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
  error_setg(errp,
 "I and E extensions are incompatible");
@@ -1293,6 +1314,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
  CPUState *cs = CPU(dev);
  RISCVCPU *cpu = RISCV_CPU(dev);
  RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
+CPURISCVState *env = &cpu->env;
  Error *local_err = NULL;
  
  cpu_exec_realizefn(cs, &local_err);

@@ -1313,6 +1335,20 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
  return;
  }
  
+if (cpu->cfg.ext_g) {

+riscv_cpu_enable_g(cpu, &local_err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+return;
+}
+
+/*
+ * Sync env->misa_ext_mask with the new
+ * env->misa_ext val.
+ */
+env->misa_ext_mask = env->misa_ext;
+}
+
  riscv_cpu_validate_set_extensions(cpu, &local_err);
  if (local_err != NULL) {
  error_propagate(errp, local_err);

Re: [PATCH v14 2/4] vhost_vdpa: fix the input in trace_vhost_vdpa_listener_region_del()

[Jason Wang]

On Tue, Mar 21, 2023 at 12:20 AM Cindy Lu  wrote:
>
> In trace_vhost_vdpa_listener_region_del, the value for llend
> should change to int128_get64(int128_sub(llend, int128_one()))
>
> Signed-off-by: Cindy Lu 

Acked-by: Jason Wang 

Thanks

> ---
>  hw/virtio/vhost-vdpa.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index bc6bad23d5..92c2413c76 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -288,7 +288,8 @@ static void vhost_vdpa_listener_region_del(MemoryListener 
> *listener,
>  iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
>  llend = vhost_vdpa_section_end(section);
>
> -trace_vhost_vdpa_listener_region_del(v, iova, int128_get64(llend));
> +trace_vhost_vdpa_listener_region_del(v, iova,
> +int128_get64(int128_sub(llend, int128_one(;
>
>  if (int128_ge(int128_make64(iova), llend)) {
>  return;
> --
> 2.34.3
>

Re: [PATCH v14 1/4] vhost: expose function vhost_dev_has_iommu()

[Jason Wang]

On Tue, Mar 21, 2023 at 12:20 AM Cindy Lu  wrote:
>
> To support vIOMMU in vdpa, need to exposed the function
> vhost_dev_has_iommu, vdpa will use this function to check
> if vIOMMU enable.
>
> Signed-off-by: Cindy Lu 

Acked-by: Jason Wang 

Thanks

> ---
>  hw/virtio/vhost.c | 2 +-
>  include/hw/virtio/vhost.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index a266396576..fd746b085b 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -107,7 +107,7 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
>  }
>  }
>
> -static bool vhost_dev_has_iommu(struct vhost_dev *dev)
> +bool vhost_dev_has_iommu(struct vhost_dev *dev)
>  {
>  VirtIODevice *vdev = dev->vdev;
>
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index a52f273347..f7f10c8fb7 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -336,4 +336,5 @@ int vhost_dev_set_inflight(struct vhost_dev *dev,
> struct vhost_inflight *inflight);
>  int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> struct vhost_inflight *inflight);
> +bool vhost_dev_has_iommu(struct vhost_dev *dev);
>  #endif
> --
> 2.34.3
>

Re: [PATCH for-8.1 v3 04/26] target/riscv: add PRIV_VERSION_LATEST

[LIU Zhiwei]

On 2023/3/19 4:04, Daniel Henrique Barboza wrote:

All these generic CPUs are using the latest priv available, at this
moment PRIV_VERSION_1_12_0:

- riscv_any_cpu_init()
- rv32_base_cpu_init()
- rv64_base_cpu_init()
- rv128_base_cpu_init()

Create a new PRIV_VERSION_LATEST enum and use it in those cases. I'll
make it easier to update everything at once when a new priv version is
available.

Reviewed-by: Richard Henderson 
Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/cpu.c | 8 
  target/riscv/cpu.h | 2 ++
  2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 18032dfd4e..1ee322001b 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -338,7 +338,7 @@ static void riscv_any_cpu_init(Object *obj)
  VM_1_10_SV32 : VM_1_10_SV57);
  #endif
  
-env->priv_ver = PRIV_VERSION_1_12_0;

+env->priv_ver = PRIV_VERSION_LATEST;
  register_cpu_props(obj);
  }
  
@@ -350,7 +350,7 @@ static void rv64_base_cpu_init(Object *obj)

  set_misa(env, MXL_RV64, 0);
  register_cpu_props(obj);
  /* Set latest version of privileged specification */
-env->priv_ver = PRIV_VERSION_1_12_0;
+env->priv_ver = PRIV_VERSION_LATEST;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
  #endif
@@ -426,7 +426,7 @@ static void rv128_base_cpu_init(Object *obj)
  set_misa(env, MXL_RV128, 0);
  register_cpu_props(obj);
  /* Set latest version of privileged specification */
-env->priv_ver = PRIV_VERSION_1_12_0;
+env->priv_ver = PRIV_VERSION_LATEST;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
  #endif
@@ -439,7 +439,7 @@ static void rv32_base_cpu_init(Object *obj)
  set_misa(env, MXL_RV32, 0);
  register_cpu_props(obj);
  /* Set latest version of privileged specification */
-env->priv_ver = PRIV_VERSION_1_12_0;
+env->priv_ver = PRIV_VERSION_LATEST;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
  #endif
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..76f81c6b68 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -88,6 +88,8 @@ enum {
  PRIV_VERSION_1_10_0 = 0,
  PRIV_VERSION_1_11_0,
  PRIV_VERSION_1_12_0,
+
+PRIV_VERSION_LATEST = PRIV_VERSION_1_12_0,

Reviewed-by: LIU Zhiwei 

Zhiwei

  };
  
  #define VEXT_VERSION_1_00_0 0x0001

Re: [PATCH for-8.1 v3 03/26] target/riscv/cpu.c: remove set_priv_version()

[LIU Zhiwei]

On 2023/3/19 4:04, Daniel Henrique Barboza wrote:

The setter is doing nothing special. Just set env->priv_ver directly.

Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/cpu.c | 30 +-
  1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 2752efe1eb..18032dfd4e 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -240,11 +240,6 @@ static void set_misa(CPURISCVState *env, RISCVMXL mxl, 
uint32_t ext)
  env->misa_ext_mask = env->misa_ext = ext;
  }
  
-static void set_priv_version(CPURISCVState *env, int priv_ver)

-{
-env->priv_ver = priv_ver;
-}
-
  #ifndef CONFIG_USER_ONLY
  static uint8_t satp_mode_from_str(const char *satp_mode_str)
  {
@@ -343,7 +338,7 @@ static void riscv_any_cpu_init(Object *obj)
  VM_1_10_SV32 : VM_1_10_SV57);
  #endif
  
-set_priv_version(env, PRIV_VERSION_1_12_0);

+env->priv_ver = PRIV_VERSION_1_12_0;
  register_cpu_props(obj);
  }
  
@@ -355,7 +350,7 @@ static void rv64_base_cpu_init(Object *obj)

  set_misa(env, MXL_RV64, 0);
  register_cpu_props(obj);
  /* Set latest version of privileged specification */
-set_priv_version(env, PRIV_VERSION_1_12_0);
+env->priv_ver = PRIV_VERSION_1_12_0;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
  #endif
@@ -366,7 +361,7 @@ static void rv64_sifive_u_cpu_init(Object *obj)
  CPURISCVState *env = &RISCV_CPU(obj)->env;
  set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
  register_cpu_props(obj);
-set_priv_version(env, PRIV_VERSION_1_10_0);
+env->priv_ver = PRIV_VERSION_1_10_0;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV39);
  #endif
@@ -379,7 +374,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
  
  set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);

  register_cpu_props(obj);
-set_priv_version(env, PRIV_VERSION_1_10_0);
+env->priv_ver = PRIV_VERSION_1_10_0;
  cpu->cfg.mmu = false;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -392,7 +387,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
  RISCVCPU *cpu = RISCV_CPU(obj);
  
  set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);

-set_priv_version(env, PRIV_VERSION_1_11_0);
+env->priv_ver = PRIV_VERSION_1_11_0;
  
  cpu->cfg.ext_g = true;

  cpu->cfg.ext_c = true;
@@ -431,7 +426,7 @@ static void rv128_base_cpu_init(Object *obj)
  set_misa(env, MXL_RV128, 0);
  register_cpu_props(obj);
  /* Set latest version of privileged specification */
-set_priv_version(env, PRIV_VERSION_1_12_0);
+env->priv_ver = PRIV_VERSION_1_12_0;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
  #endif
@@ -444,7 +439,7 @@ static void rv32_base_cpu_init(Object *obj)
  set_misa(env, MXL_RV32, 0);
  register_cpu_props(obj);
  /* Set latest version of privileged specification */
-set_priv_version(env, PRIV_VERSION_1_12_0);
+env->priv_ver = PRIV_VERSION_1_12_0;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
  #endif
@@ -454,8 +449,9 @@ static void rv32_sifive_u_cpu_init(Object *obj)
  {
  CPURISCVState *env = &RISCV_CPU(obj)->env;
  set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
+
  register_cpu_props(obj);
-set_priv_version(env, PRIV_VERSION_1_10_0);
+env->priv_ver = PRIV_VERSION_1_10_0;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
  #endif
@@ -468,7 +464,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
  
  set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);

  register_cpu_props(obj);
-set_priv_version(env, PRIV_VERSION_1_10_0);
+env->priv_ver = PRIV_VERSION_1_10_0;
  cpu->cfg.mmu = false;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -482,7 +478,7 @@ static void rv32_ibex_cpu_init(Object *obj)
  
  set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);

  register_cpu_props(obj);
-set_priv_version(env, PRIV_VERSION_1_11_0);
+env->priv_ver = PRIV_VERSION_1_11_0;
  cpu->cfg.mmu = false;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -497,7 +493,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
  
  set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);

  register_cpu_props(obj);
-set_priv_version(env, PRIV_VERSION_1_10_0);
+env->priv_ver = PRIV_VERSION_1_10_0;
  cpu->cfg.mmu = false;
  #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -1160,7 +1156,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
  }
  
  if (priv_version >= PRIV_VERSION_1_10_0) {

-set_priv_version(env, pr

Re: [PATCH for-8.1 v3 02/26] target/riscv/cpu.c: remove set_vext_version()

[LIU Zhiwei]

On 2023/3/19 4:04, Daniel Henrique Barboza wrote:

This setter is doing nothing else but setting env->vext_ver. Assign the
value directly.

Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/cpu.c | 7 +--
  1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 18591aa53a..2752efe1eb 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -245,11 +245,6 @@ static void set_priv_version(CPURISCVState *env, int 
priv_ver)
  env->priv_ver = priv_ver;
  }
  
-static void set_vext_version(CPURISCVState *env, int vext_ver)

-{
-env->vext_ver = vext_ver;
-}
-
  #ifndef CONFIG_USER_ONLY
  static uint8_t satp_mode_from_str(const char *satp_mode_str)
  {
@@ -839,7 +834,7 @@ static void riscv_cpu_validate_v(CPURISCVState *env, 
RISCVCPUConfig *cfg,
  qemu_log("vector version is not specified, "
   "use the default value v1.0\n");
  }
-set_vext_version(env, vext_version);
+env->vext_ver = vext_version;

Reviewed-by: LIU Zhiwei 

Zhiwei

  }
  
  /*

Re: [PATCH for-8.1 v3 01/26] target/riscv/cpu.c: add riscv_cpu_validate_v()

[LIU Zhiwei]

On 2023/3/19 4:04, Daniel Henrique Barboza wrote:

The RVV verification will error out if fails and it's being done at the
end of riscv_cpu_validate_set_extensions(). Let's put it in its own
function and do it earlier.

We'll move it out of riscv_cpu_validate_set_extensions() in the near future,
but for now this is enough to clean the code a bit.

Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/cpu.c | 86 ++
  1 file changed, 49 insertions(+), 37 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1e97473af2..18591aa53a 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -802,6 +802,46 @@ static void riscv_cpu_disas_set_info(CPUState *s, 
disassemble_info *info)
  }
  }
  
+static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,

+ Error **errp)
+{
+int vext_version = VEXT_VERSION_1_00_0;
+
+if (!is_power_of_2(cfg->vlen)) {
+error_setg(errp, "Vector extension VLEN must be power of 2");
+return;
+}
+if (cfg->vlen > RV_VLEN_MAX || cfg->vlen < 128) {
+error_setg(errp,
+   "Vector extension implementation only supports VLEN "
+   "in the range [128, %d]", RV_VLEN_MAX);
+return;
+}
+if (!is_power_of_2(cfg->elen)) {
+error_setg(errp, "Vector extension ELEN must be power of 2");
+return;
+}
+if (cfg->elen > 64 || cfg->elen < 8) {
+error_setg(errp,
+   "Vector extension implementation only supports ELEN "
+   "in the range [8, 64]");
+return;
+}
+if (cfg->vext_spec) {
+if (!g_strcmp0(cfg->vext_spec, "v1.0")) {
+vext_version = VEXT_VERSION_1_00_0;
+} else {
+error_setg(errp, "Unsupported vector spec version '%s'",
+   cfg->vext_spec);
+return;
+}
+} else {
+qemu_log("vector version is not specified, "
+ "use the default value v1.0\n");
+}
+set_vext_version(env, vext_version);
+}
+
  /*
   * Check consistency between chosen extensions while setting
   * cpu->cfg accordingly, doing a set_misa() in the end.
@@ -809,6 +849,7 @@ static void riscv_cpu_disas_set_info(CPUState *s, 
disassemble_info *info)
  static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
  {
  CPURISCVState *env = &cpu->env;
+Error *local_err = NULL;
  uint32_t ext = 0;
  
  /* Do some ISA extension error checking */

@@ -939,6 +980,14 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU 
*cpu, Error **errp)
  }
  }
  
+if (cpu->cfg.ext_v) {

+riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+return;
+}
+}
+
  if (cpu->cfg.ext_zk) {
  cpu->cfg.ext_zkn = true;
  cpu->cfg.ext_zkr = true;
@@ -993,44 +1042,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU 
*cpu, Error **errp)
  ext |= RVH;
  }
  if (cpu->cfg.ext_v) {
-int vext_version = VEXT_VERSION_1_00_0;
  ext |= RVV;
-if (!is_power_of_2(cpu->cfg.vlen)) {
-error_setg(errp,
-   "Vector extension VLEN must be power of 2");
-return;
-}
-if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
-error_setg(errp,
-   "Vector extension implementation only supports VLEN "
-   "in the range [128, %d]", RV_VLEN_MAX);
-return;
-}
-if (!is_power_of_2(cpu->cfg.elen)) {
-error_setg(errp,
-   "Vector extension ELEN must be power of 2");
-return;
-}
-if (cpu->cfg.elen > 64 || cpu->cfg.elen < 8) {
-error_setg(errp,
-   "Vector extension implementation only supports ELEN "
-   "in the range [8, 64]");
-return;
-}
-if (cpu->cfg.vext_spec) {
-if (!g_strcmp0(cpu->cfg.vext_spec, "v1.0")) {
-vext_version = VEXT_VERSION_1_00_0;
-} else {
-error_setg(errp,
-   "Unsupported vector spec version '%s'",
-   cpu->cfg.vext_spec);
-return;
-}
-} else {
-qemu_log("vector version is not specified, "
- "use the default value v1.0\n");
-}
-set_vext_version(env, vext_version);


Reviewed-by: LIU Zhiwei 

Zhiwei


  }
  if (cpu->cfg.ext_j) {
  ext |= RVJ;

Re: [PATCH] python: honour message limit when using pre-opened socket

[John Snow]

On Mon, Mar 20, 2023 at 8:20 AM Vladimir Sementsov-Ogievskiy
 wrote:
>
> On 20.03.23 13:54, Daniel P. Berrangé wrote:
> > The default message recv limit in asyncio is smaller than our needs, so
> > when opening connections we override it. This was done when opening a
> > connection using a socket address, but was missed when using a
> > pre-opened socket file descriptor.
> >
> > This latent bug was exposed when the QEMUMachine class was enhanced to
> > use socketpair() when no socket address was passed by:
> >
> >commit bd4c0ef409140bd1be393407c04005ac077d4574
> >Author: Marc-André Lureau
> >Date:   Wed Jan 11 12:01:01 2023 +0400
> >
> >  python/qemu/machine: use socketpair() for QMP by default
> >
> > Signed-off-by: Daniel P. Berrangé
>
> Tested-by: Vladimir Sementsov-Ogievskiy 
>
> Thanks!
>
> --
> Best regards,
> Vladimir
>

Thanks x3. Will stage in both places.

--js

Re: [PATCH 0/2] hw/i2c: Reset fixes

[Corey Minyard]

On Mon, Mar 20, 2023 at 10:14:17PM +, Joe Komlodi wrote:
> Hi all,
> 
> This series fixes some I2C state variables not being reset when a reset
> would happen.
> 
> These stale variables would infrequently cause issues, something around
> the order of 5/1000 runs, since the machine would have to be reset at a
> point where they would be in a state that would cause problems.

These look good to me.  Definitely a missing needed function.  Looking
through the way it's handled, I think the proper things are being reset
and the proper ones are being left alone.  There's no checking of the
reset type, but there's only one reset type right now, so I guess any
changes due to reset type will have to come when new types come.

Acked-by: Corey Minyard 

for another tree, or I can take them.

Thanks,

-corey

> 
> Thanks!
> Joe
> 
> Joe Komlodi (2):
>   hw/i2c: smbus_slave: Reset state on reset
>   hw/i2c: core: Add reset
> 
>  hw/i2c/core.c| 25 ++---
>  hw/i2c/smbus_slave.c |  9 +
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> -- 
> 2.40.0.rc2.332.ga46443480c-goog
> 


smime.p7s
Description: S/MIME cryptographic signature

Re: KVM call for agenda for 2023-03-21

[Bernhard Beschow]

Am 20. März 2023 15:47:33 UTC schrieb "Philippe Mathieu-Daudé" 
:
>Hi Juan,
>
>On 18/3/23 18:59, Juan Quintela wrote:
>> 
>> Hi
>> 
>> NOTE, NOTE, NOTE
>> 
>> Remember that we are back in that crazy part of the year when daylight
>> saving applies.  Call is done on US timezone.  If you are anything else,
>> just doublecheck that it is working for you properly.
>> 
>> NOTE, NOTE, NOTE
>> 
>> Topics in the backburner:
>> - single qemu binary
>>Philippe?
>
>Well we wanted a slot to discuss a bit the design problems we have
>around some PCI-to-ISA bridges like the PIIX and VIA south bridges.
>
>One of the main problem is figure how to instantiate circular IRQs
>with QOM. Ex:
>
>  devA exposes irqAo output
>   wires to irqAi input
>
>  devB exposes irqBo output
>   wires to irqBi input
>
>How to wire irqAo -> irqBi *AND* irqBo -> irqAi?
>
>However personally I was busy with debugging issues opened for the
>8.0 release, and it is probably very late to schedule with Mark and
>Bernhard for tomorrow...

Hmm, yeah, a few days notice in advance would help me to allocate the slot for 
QEMU rather than work... I'm very interested in the topic though.

Best regards,
Bernhard
>
>> - The future of icount.
>>Alex?  My understanding is that you are interested in
>>qemu 8.1 to open.
>> 
>> Anything else?
>> 
>> 
>> Please, send any topic that you are interested in covering.
>> 
>> At the end of Monday I will send an email with the agenda or the
>> cancellation of the call, so hurry up.

Re: [PATCH v2] ui/gtk: fix cursor moved to left corner

[Bernhard Beschow]

Am 20. März 2023 13:26:24 UTC schrieb marcandre.lur...@redhat.com:
>From: Marc-André Lureau 
>
>Do not attempt to move the pointer if the widget is not yet realized.
>The mouse cursor is placed to the corner of the screen, on X11 at least,
>as x_root and y_root are then miscalculated. (this is not reproducible
>on Wayland, because Gtk doesn't implement device warping there)
>
>This also fixes the following warning at start:
>qemu: Gdk: gdk_window_get_root_coords: assertion 'GDK_IS_WINDOW (window)' 
>failed
>
>Fixes: 6effaa16ac98 ("ui: set cursor position upon listener
>registration")

This particular issue gets fixed, so:
Tested-by: Bernhard Beschow 

However, when I perform a test like in 
https://gitlab.com/qemu-project/qemu/-/issues/1550 , the cursor gets placed 
into the QEMU window once the graphical environment is entered *in the guest*, 
regardless of whether QEMU has the focus or not. This seems quite strange 
because an application shouldn't "steal" the mouse from the active application. 
So perhaps this fix is just scratching the surface of a deeper underlying bug...

Best regards,
Bernhard

>Reported-by: Bernhard Beschow 
>Signed-off-by: Marc-André Lureau 
>---
> ui/gtk.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/ui/gtk.c b/ui/gtk.c
>index fd82e9b1ca..e9564f2baa 100644
>--- a/ui/gtk.c
>+++ b/ui/gtk.c
>@@ -450,7 +450,8 @@ static void gd_mouse_set(DisplayChangeListener *dcl,
> GdkDisplay *dpy;
> gint x_root, y_root;
> 
>-if (qemu_input_is_absolute()) {
>+if (!gtk_widget_get_realized(vc->gfx.drawing_area) ||
>+qemu_input_is_absolute()) {
> return;
> }
> 

[PATCH 1/2] hw/i2c: smbus_slave: Reset state on reset

[Joe Komlodi]

If a reset comes while the SMBus device is not in its idle state, it's
possible for it to get confused on valid transactions post-reset.

Signed-off-by: Joe Komlodi 
---
 hw/i2c/smbus_slave.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index feb3ec6333..7b9d8780ae 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -197,10 +197,19 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
 return 0;
 }
 
+static void smbus_device_enter_reset(Object *obj, ResetType type)
+{
+SMBusDevice *dev = SMBUS_DEVICE(obj);
+dev->mode = SMBUS_IDLE;
+dev->data_len = 0;
+}
+
 static void smbus_device_class_init(ObjectClass *klass, void *data)
 {
 I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+ResettableClass *rc = RESETTABLE_CLASS(klass);
 
+rc->phases.enter = smbus_device_enter_reset;
 sc->event = smbus_i2c_event;
 sc->recv = smbus_i2c_recv;
 sc->send = smbus_i2c_send;
-- 
2.40.0.rc2.332.ga46443480c-goog

[PATCH 2/2] hw/i2c: core: Add reset

[Joe Komlodi]

It's possible for a reset to come in the middle of a transaction, which
causes the bus to be in an old state when a new transaction comes in.

Signed-off-by: Joe Komlodi 
---
 hw/i2c/core.c | 25 ++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index bed594fe59..2aecbfb334 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -23,10 +23,29 @@ static Property i2c_props[] = {
 DEFINE_PROP_END_OF_LIST(),
 };
 
+static void i2c_bus_enter_reset(Object *obj, ResetType type)
+{
+I2CBus *bus = I2C_BUS(obj);
+I2CNode *node, *next;
+
+bus->broadcast = false;
+QLIST_FOREACH_SAFE(node, &bus->current_devs, next, next) {
+QLIST_REMOVE(node, next);
+g_free(node);
+}
+}
+
+static void i2c_bus_class_init(ObjectClass *klass, void *data)
+{
+ResettableClass *rc = RESETTABLE_CLASS(klass);
+rc->phases.enter = i2c_bus_enter_reset;
+}
+
 static const TypeInfo i2c_bus_info = {
-.name = TYPE_I2C_BUS,
-.parent = TYPE_BUS,
-.instance_size = sizeof(I2CBus),
+   .name = TYPE_I2C_BUS,
+   .parent = TYPE_BUS,
+   .instance_size = sizeof(I2CBus),
+   .class_init = i2c_bus_class_init,
 };
 
 static int i2c_bus_pre_save(void *opaque)
-- 
2.40.0.rc2.332.ga46443480c-goog

[PATCH 0/2] hw/i2c: Reset fixes

[Joe Komlodi]

Hi all,

This series fixes some I2C state variables not being reset when a reset
would happen.

These stale variables would infrequently cause issues, something around
the order of 5/1000 runs, since the machine would have to be reset at a
point where they would be in a state that would cause problems.

Thanks!
Joe

Joe Komlodi (2):
  hw/i2c: smbus_slave: Reset state on reset
  hw/i2c: core: Add reset

 hw/i2c/core.c| 25 ++---
 hw/i2c/smbus_slave.c |  9 +
 2 files changed, 31 insertions(+), 3 deletions(-)

-- 
2.40.0.rc2.332.ga46443480c-goog

[PATCH v3 1/5] bitops.h: add deposit16 function

[Titus Rwantare]

Makes it more explicit that 16 bit values are being used

Signed-off-by: Titus Rwantare 
---
 include/qemu/bitops.h | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 03213ce952..887b8f8ce8 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -446,6 +446,32 @@ static inline int64_t sextract64(uint64_t value, int 
start, int length)
 return ((int64_t)(value << (64 - length - start))) >> (64 - length);
 }
 
+/**
+ * deposit16:
+ * @value: initial value to insert bit field into
+ * @start: the lowest bit in the bit field (numbered from 0)
+ * @length: the length of the bit field
+ * @fieldval: the value to insert into the bit field
+ *
+ * Deposit @fieldval into the 16 bit @value at the bit field specified
+ * by the @start and @length parameters, and return the modified
+ * @value. Bits of @value outside the bit field are not modified.
+ * Bits of @fieldval above the least significant @length bits are
+ * ignored. The bit field must lie entirely within the 16 bit word.
+ * It is valid to request that all 16 bits are modified (ie @length
+ * 16 and @start 0).
+ *
+ * Returns: the modified @value.
+ */
+static inline uint16_t deposit16(uint16_t value, int start, int length,
+ uint16_t fieldval)
+{
+uint16_t mask;
+assert(start >= 0 && length > 0 && length <= 16 - start);
+mask = (~0U >> (16 - length)) << start;
+return (value & ~mask) | ((fieldval << start) & mask);
+}
+
 /**
  * deposit32:
  * @value: initial value to insert bit field into
-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH v3 3/5] hw/gpio: add PCA9536 i2c gpio expander

[Titus Rwantare]

This device has the same register layout as the pca9538, but 4 fewer
gpio pins. This commit lowers the number of pins initialised, and reuses
the pca9538 logic.

Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Hao Wu 
Signed-off-by: Titus Rwantare 
---
 hw/gpio/pca_i2c_gpio.c | 18 ++
 include/hw/gpio/pca_i2c_gpio.h |  2 ++
 tests/lcitool/libvirt-ci   |  2 +-
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/hw/gpio/pca_i2c_gpio.c b/hw/gpio/pca_i2c_gpio.c
index 00ba343f95..14da58e5c4 100644
--- a/hw/gpio/pca_i2c_gpio.c
+++ b/hw/gpio/pca_i2c_gpio.c
@@ -337,6 +337,19 @@ static void pca9538_gpio_class_init(ObjectClass *klass, 
void *data)
 k->send = pca953x_send;
 }
 
+static void pca9536_gpio_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+PCAGPIOClass *pc = PCA_I2C_GPIO_CLASS(klass);
+
+dc->desc = "PCA9536 4-bit I/O expander";
+pc->num_pins = PCA9536_NUM_PINS;
+
+k->recv = pca953x_recv;
+k->send = pca953x_send;
+}
+
 static void pca_i2c_gpio_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
@@ -387,6 +400,11 @@ static const TypeInfo pca_gpio_types[] = {
 .parent = TYPE_PCA_I2C_GPIO,
 .class_init = pca9538_gpio_class_init,
 },
+{
+.name = TYPE_PCA9536_GPIO,
+.parent = TYPE_PCA_I2C_GPIO,
+.class_init = pca9536_gpio_class_init,
+},
 };
 
 DEFINE_TYPES(pca_gpio_types);
diff --git a/include/hw/gpio/pca_i2c_gpio.h b/include/hw/gpio/pca_i2c_gpio.h
index a4220105e8..deb4528065 100644
--- a/include/hw/gpio/pca_i2c_gpio.h
+++ b/include/hw/gpio/pca_i2c_gpio.h
@@ -20,6 +20,7 @@
 #define PCA_I2C_MAX_PINS 16
 #define PCA6416_NUM_PINS 16
 #define PCA9538_NUM_PINS 8
+#define PCA9536_NUM_PINS 4
 
 typedef struct PCAGPIOClass {
 I2CSlaveClass parent;
@@ -63,5 +64,6 @@ OBJECT_DECLARE_TYPE(PCAGPIOState, PCAGPIOClass, PCA_I2C_GPIO)
 
 #define TYPE_PCA6416_GPIO "pca6416"
 #define TYPE_PCA9538_GPIO "pca9538"
+#define TYPE_PCA9536_GPIO "pca9536"
 
 #endif
diff --git a/tests/lcitool/libvirt-ci b/tests/lcitool/libvirt-ci
index e3eb28cf2e..232f41f160 16
--- a/tests/lcitool/libvirt-ci
+++ b/tests/lcitool/libvirt-ci
@@ -1 +1 @@
-Subproject commit e3eb28cf2e17fbcf7fe7e19505ee432b8ec5bbb5
+Subproject commit 232f41f160d4567b8c82dd52aa96c2bc3a5b75c1
-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH v3 0/5] PCA I2C GPIO expanders

[Titus Rwantare]

This patch series contains a set of i2c GPIO expanders,
with support for 4, 8, and 16 GPIO connections.

The devices are configured as GPIO inputs by default, but can have pins
configured to be inputs with qmp commands.

For example, the following snippet in a board file for a system,
configures a 16 bit pca6416 to have pins 8-11 as inputs, then asserts
them.

dev = DEVICE(i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 3), 
"pca6416", 0x72));
object_property_set_uint(OBJECT(dev), "gpio_config", 0x0F00, &error_abort);
object_property_set_uint(OBJECT(dev), "gpio_input", 0x0F00, &error_abort);

We currently use these to test hardware presence and LEDs in simulation.

Thanks

Since v2:
- switched to extract / deposit API
- added deposit16 to bitops.h
- squashed PCA9538 patch into PCA6416 to use the same send and recv
  functions
- updated unit tests use asymmetric 16-bit test values
- add patch to imply I2C devices on NPCM7xx boards

Since v1:
- addressed comments
- fixed typos in commit messages

Titus Rwantare (5):
  bitops.h: add deposit16 function
  hw/gpio: add PCA953x i2c GPIO expanders
  hw/gpio: add PCA9536 i2c gpio expander
  hw/i2c: add canonical path to i2c event traces
  hw/arm: imply I2C_DEVICES on NPCM7xx

 hw/arm/Kconfig  |   1 +
 hw/gpio/Kconfig |   5 +
 hw/gpio/meson.build |   1 +
 hw/gpio/pca_i2c_gpio.c  | 410 
 hw/gpio/trace-events|   5 +
 hw/i2c/core.c   |   8 +-
 hw/i2c/trace-events |   2 +-
 include/hw/gpio/pca_i2c_gpio.h  |  69 ++
 include/qemu/bitops.h   |  26 ++
 roms/edk2   |   2 +-
 roms/openbios   |   2 +-
 roms/opensbi|   2 +-
 roms/seabios|   2 +-
 tests/qtest/meson.build |   1 +
 tests/qtest/pca_i2c_gpio-test.c | 188 +++
 15 files changed, 716 insertions(+), 8 deletions(-)
 create mode 100644 hw/gpio/pca_i2c_gpio.c
 create mode 100644 include/hw/gpio/pca_i2c_gpio.h
 create mode 100644 tests/qtest/pca_i2c_gpio-test.c

-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH v3 2/5] hw/gpio: add PCA953x i2c GPIO expanders

[Titus Rwantare]

The PCA6416 is an i2c device with 16 GPIO pins, the PCA9538 has 8 pins.

Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Hao Wu 
Signed-off-by: Titus Rwantare 
---
 hw/gpio/Kconfig |   5 +
 hw/gpio/meson.build |   1 +
 hw/gpio/pca_i2c_gpio.c  | 392 
 hw/gpio/trace-events|   5 +
 include/hw/gpio/pca_i2c_gpio.h  |  67 ++
 roms/edk2   |   2 +-
 roms/openbios   |   2 +-
 roms/opensbi|   2 +-
 roms/seabios|   2 +-
 tests/lcitool/libvirt-ci|   2 +-
 tests/qtest/meson.build |   1 +
 tests/qtest/pca_i2c_gpio-test.c | 188 +++
 12 files changed, 664 insertions(+), 5 deletions(-)
 create mode 100644 hw/gpio/pca_i2c_gpio.c
 create mode 100644 include/hw/gpio/pca_i2c_gpio.h
 create mode 100644 tests/qtest/pca_i2c_gpio-test.c

diff --git a/hw/gpio/Kconfig b/hw/gpio/Kconfig
index d2cf3accc8..80395af197 100644
--- a/hw/gpio/Kconfig
+++ b/hw/gpio/Kconfig
@@ -16,3 +16,8 @@ config GPIO_PWR
 
 config SIFIVE_GPIO
 bool
+
+config PCA_I2C_GPIO
+bool
+depends on I2C
+default y if I2C_DEVICES
diff --git a/hw/gpio/meson.build b/hw/gpio/meson.build
index b726e6d27a..1e5b602002 100644
--- a/hw/gpio/meson.build
+++ b/hw/gpio/meson.build
@@ -12,3 +12,4 @@ softmmu_ss.add(when: 'CONFIG_OMAP', if_true: 
files('omap_gpio.c'))
 softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files('bcm2835_gpio.c'))
 softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_gpio.c'))
 softmmu_ss.add(when: 'CONFIG_SIFIVE_GPIO', if_true: files('sifive_gpio.c'))
+softmmu_ss.add(when: 'CONFIG_PCA_I2C_GPIO', if_true: files('pca_i2c_gpio.c'))
diff --git a/hw/gpio/pca_i2c_gpio.c b/hw/gpio/pca_i2c_gpio.c
new file mode 100644
index 00..00ba343f95
--- /dev/null
+++ b/hw/gpio/pca_i2c_gpio.c
@@ -0,0 +1,392 @@
+/*
+ * NXP PCA I2C GPIO Expanders
+ *
+ * Low-voltage translating 16-bit I2C/SMBus GPIO expander with interrupt 
output,
+ * reset, and configuration registers
+ *
+ * Datasheet: https://www.nxp.com/docs/en/data-sheet/PCA6416A.pdf
+ *
+ * Copyright 2023 Google LLC
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * To assert some input pins before boot, use the following in the board file 
of
+ * the machine:
+ *  object_property_set_uint(Object *obj, const char *name,
+ *   uint64_t value, Error **errp);
+ * specifying name as "gpio_config" and the value as a bitfield of the inputs
+ * e.g. for the pca6416, a value of 0xFFF0, configures pins 0-3 as outputs and
+ * 4-15 as inputs.
+ * Then using name "gpio_input" with value "0x0F00" would raise GPIOs 8-11.
+ *
+ * This value can also be set at runtime through qmp externally, or by
+ * writing to the config register using i2c. The guest driver should generally
+ * control the config register, but exposing it via qmp allows external 
testing.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "hw/gpio/pca_i2c_gpio.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "qapi/visitor.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/bitops.h"
+#include "trace.h"
+
+/*
+ * compare new_output to curr_output and update irq to match new_output
+ *
+ * The Input port registers (registers 0 and 1) reflect the incoming logic
+ * levels of the pins, regardless of whether the pin is defined as an input or
+ * an output by the Configuration register.
+ */
+static void pca_i2c_update_irqs(PCAGPIOState *ps)
+{
+PCAGPIOClass *pc = PCA_I2C_GPIO_GET_CLASS(ps);
+uint16_t out_diff = ps->new_output ^ ps->curr_output;
+uint16_t in_diff = ps->new_input ^ ps->curr_input;
+uint16_t mask, pin_i;
+
+if (in_diff || out_diff) {
+for (int i = 0; i < pc->num_pins; i++) {
+mask = BIT(i);
+/* pin must be configured as an output to be set here */
+if (out_diff & ~ps->config & mask) {
+pin_i = mask & ps->new_output;
+qemu_set_irq(ps->output[i], pin_i > 0);
+ps->curr_output &= ~mask;
+ps->curr_output |= pin_i;
+}
+
+if (in_diff & mask) {
+ps->curr_input &= ~mask;
+ps->curr_input |= mask & ps->new_input;
+}
+}
+/* make diff = 0 */
+ps->new_input = ps->curr_input;
+}
+}
+
+static void pca_i2c_irq_handler(void *opaque, int n, int level)
+{
+PCAGPIOState *ps = opaque;
+PCAGPIOClass *pc = PCA_I2C_GPIO_GET_CLASS(opaque);
+uint16_t mask = BIT(n);
+
+g_assert(n < pc->num_pins);
+g_assert(n >= 0);
+
+ps->new_input &= ~mask;
+
+if (level > 0) {
+ps->new_input |= BIT(n);
+}
+
+pca_i2c_update_irqs(ps);
+}
+
+/* slave to master */
+static uint8_t _pca953x_recv(I2CSlave *i2c, uint32_t shift)
+{
+PCAGPIOState *ps = PCA_I2C_GPIO(i2c);
+uint8_t data;
+
+switch (ps->command) {
+ca

[PATCH v3 5/5] hw/arm: imply I2C_DEVICES on NPCM7xx

[Titus Rwantare]

Signed-off-by: Titus Rwantare 
---
 hw/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index b5aed4aff5..548c10d7fc 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -406,6 +406,7 @@ config XLNX_VERSAL
 
 config NPCM7XX
 bool
+imply I2C_DEVICES
 select A9MPCORE
 select ADM1272
 select ARM_GIC
-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH v3 4/5] hw/i2c: add canonical path to i2c event traces

[Titus Rwantare]

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Titus Rwantare 
---
 hw/i2c/core.c   | 8 +---
 hw/i2c/trace-events | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index bed594fe59..896da359f5 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -161,7 +161,8 @@ static int i2c_do_start_transfer(I2CBus *bus, uint8_t 
address,
start condition.  */
 
 if (sc->event) {
-trace_i2c_event(event == I2C_START_SEND ? "start" : "start_async",
+trace_i2c_event(DEVICE(s)->canonical_path,
+event == I2C_START_SEND ? "start" : "start_async",
 s->address);
 rv = sc->event(s, event);
 if (rv && !bus->broadcast) {
@@ -244,7 +245,7 @@ void i2c_end_transfer(I2CBus *bus)
 I2CSlave *s = node->elt;
 sc = I2C_SLAVE_GET_CLASS(s);
 if (sc->event) {
-trace_i2c_event("finish", s->address);
+trace_i2c_event(DEVICE(s)->canonical_path, "finish", s->address);
 sc->event(s, I2C_FINISH);
 }
 QLIST_REMOVE(node, next);
@@ -321,7 +322,8 @@ void i2c_nack(I2CBus *bus)
 QLIST_FOREACH(node, &bus->current_devs, next) {
 sc = I2C_SLAVE_GET_CLASS(node->elt);
 if (sc->event) {
-trace_i2c_event("nack", node->elt->address);
+trace_i2c_event(DEVICE(node->elt)->canonical_path,
+"nack", node->elt->address);
 sc->event(node->elt, I2C_NACK);
 }
 }
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 8e88aa24c1..f42a1ff539 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -9,7 +9,7 @@ bitbang_i2c_data(unsigned dat, unsigned clk, unsigned old_out, 
unsigned new_out)
 
 # core.c
 
-i2c_event(const char *event, uint8_t address) "%s(addr:0x%02x)"
+i2c_event(const char *id, const char *event, uint8_t address) "%s: 
%s(addr:0x%02x)"
 i2c_send(uint8_t address, uint8_t data) "send(addr:0x%02x) data:0x%02x"
 i2c_send_async(uint8_t address, uint8_t data) "send_async(addr:0x%02x) 
data:0x%02x"
 i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
-- 
2.40.0.rc1.284.g88254d51c5-goog

Re: KVM call for agenda for 2023-03-21

[Mark Cave-Ayland]

On 20/03/2023 15:47, Philippe Mathieu-Daudé wrote:


Hi Juan,

On 18/3/23 18:59, Juan Quintela wrote:


Hi

NOTE, NOTE, NOTE

Remember that we are back in that crazy part of the year when daylight
saving applies.  Call is done on US timezone.  If you are anything else,
just doublecheck that it is working for you properly.

NOTE, NOTE, NOTE

Topics in the backburner:
- single qemu binary
   Philippe?


Well we wanted a slot to discuss a bit the design problems we have
around some PCI-to-ISA bridges like the PIIX and VIA south bridges.

One of the main problem is figure how to instantiate circular IRQs
with QOM. Ex:

   devA exposes irqAo output
    wires to irqAi input

   devB exposes irqBo output
    wires to irqBi input

How to wire irqAo -> irqBi *AND* irqBo -> irqAi?

However personally I was busy with debugging issues opened for the
8.0 release, and it is probably very late to schedule with Mark and
Bernhard for tomorrow...


Yeah unfortunately it's impossible for me to guarantee I'll be around for the call on 
Tuesdays, but then I've also had an idea that models the hardware in a different way 
so that circular IRQs aren't needed. I'm a bit backlogged with QEMU bits and pieces 
this week, so it will take a little time to come up with a suitable proposal.



ATB,

Mark.

Re: [PATCH v2 8/9] accel/tcg: Pass last not end to tb_invalidate_phys_page_range__locked

[Philippe Mathieu-Daudé]

On 17/3/23 16:54, Richard Henderson wrote:

Pass the address of the last byte to be changed, rather than
the first address past the last byte.  This avoids overflow
when the last page of the address space is involved.

Properly truncate tb_last to the end of the page; the comment about
tb_end being past the end of the page being ok is not correct,
considering overflow.

Signed-off-by: Richard Henderson 
---
  accel/tcg/tb-maint.c | 26 --
  1 file changed, 12 insertions(+), 14 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 3/9] include/exec: Replace reserved_va with max_reserved_va

[Philippe Mathieu-Daudé]

On 17/3/23 16:54, Richard Henderson wrote:

In addition to the rename, change the semantics to be the
last byte of the guest va, rather than the following byte.
This avoids some overflow conditions.

Signed-off-by: Richard Henderson 
---
  include/exec/cpu-all.h  | 11 ++-
  linux-user/arm/target_cpu.h |  2 +-
  bsd-user/main.c | 10 +++---
  bsd-user/mmap.c |  4 ++--
  linux-user/elfload.c| 21 +++--
  linux-user/main.c   | 27 +--
  linux-user/mmap.c   |  4 ++--
  7 files changed, 42 insertions(+), 37 deletions(-)




diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 28135c9e6a..cf14930c30 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -283,7 +283,7 @@ static abi_ulong mmap_find_vma_reserved(abi_ulong start, 
abi_ulong size,
  end_addr = start + size;
  if (start > reserved_va - size) {
  /* Start at the top of the address space.  */
-end_addr = ((reserved_va - size) & -align) + size;
+end_addr = ((reserved_va + 1 - size) & -align) + size;
  looped = true;
  }
  
@@ -297,7 +297,7 @@ static abi_ulong mmap_find_vma_reserved(abi_ulong start, abi_ulong size,

  return (abi_ulong)-1;
  }
  /* Re-start at the top of the address space.  */
-addr = end_addr = ((reserved_va - size) & -align) + size;
+addr = end_addr = ((reserved_va + 1 - size) & -align) + size;


Possible follow-up cleanup:

  addr = end_addr = ROUND_DOWN(reserved_va + 1 - size, align) + size;

Reviewed-by: Philippe Mathieu-Daudé 

Better with another R-b on top ;)

Re: [PATCH v2 1/9] linux-user: Diagnose misaligned -R size

[Philippe Mathieu-Daudé]

On 17/3/23 16:54, Richard Henderson wrote:

We have been enforcing host page alignment for the non-R
fallback of MAX_RESERVED_VA, but failing to enforce for -R.

Signed-off-by: Richard Henderson 
---
  linux-user/main.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/linux-user/main.c b/linux-user/main.c
index 4b18461969..39d9bd4d7a 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -793,6 +793,12 @@ int main(int argc, char **argv, char **envp)
   */
  max_reserved_va = MAX_RESERVED_VA(cpu);
  if (reserved_va != 0) {
+if (reserved_va % qemu_host_page_size) {
+char *s = size_to_str(qemu_host_page_size);
+fprintf(stderr, "Reserved virtual address not aligned mod %s\n", 
s);
+g_free(s);
+exit(EXIT_FAILURE);
+}
  if (max_reserved_va && reserved_va > max_reserved_va) {
  fprintf(stderr, "Reserved virtual address too big\n");
  exit(EXIT_FAILURE);


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] xen: Fix host pci for stubdom

[Jason Andryuk]

On Mon, Mar 20, 2023 at 2:41 PM Bernhard Beschow  wrote:
>
>
>
> Am 20. März 2023 00:05:54 UTC schrieb Jason Andryuk :
> >PCI passthrough for an HVM with a stubdom is PV PCI passthrough from
> >dom0 to the stubdom, and then QEMU passthrough of the PCI device inside
> >the stubdom.  xen-pciback has boolean module param passthrough which
> >controls "how to export PCI topology to guest".  If passthrough=1, the
> >frontend shows a PCI SBDF matching the backend host device.  When
> >passthough=0, the frontend will get a sequentially allocated SBDF.
> >
> >libxl passes the host SBDF over QMP to QEMU.  For non-stubdom or stubdom
> >with passthrough=1, this works fine.  However, it fails for
> >passthrough=0 when QEMU can't find the sysfs node for the host SBDF.
> >
> >Handle all these cases.  Look for the xenstore frontend nodes.  If they
> >are missing, then default to using the QMP command provided SBDF.  This
> >is the non-stubdom case.  If xenstore nodes are found, then read the
> >local SBDF from the xenstore nodes.  This will handle either
> >passthrough=0/1 case.
> >
> >Based on a stubdom-specific patch originally by Marek
> >Marczykowski-Górecki , which is based
> >on earlier work by HW42 
> >
> >Signed-off-by: Jason Andryuk 
> >---
> > hw/xen/xen-host-pci-device.c | 96 +++-
> > hw/xen/xen-host-pci-device.h |  6 +++
> > 2 files changed, 101 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> >index 8c6e9a1716..51a72b432d 100644
> >--- a/hw/xen/xen-host-pci-device.c
> >+++ b/hw/xen/xen-host-pci-device.c
> >@@ -9,6 +9,7 @@
> > #include "qemu/osdep.h"
> > #include "qapi/error.h"
> > #include "qemu/cutils.h"
> >+#include "hw/xen/xen-legacy-backend.h"
> > #include "xen-host-pci-device.h"
> >
> > #define XEN_HOST_PCI_MAX_EXT_CAP \
> >@@ -33,13 +34,101 @@
> > #define IORESOURCE_PREFETCH 0x1000  /* No side effects */
> > #define IORESOURCE_MEM_64   0x0010
> >
> >+/*
> >+ * Non-passthrough (dom0) accesses are local PCI devices and use the given 
> >BDF
> >+ * Passthough (stubdom) accesses are through PV frontend PCI device.  Those
>
> I'm unable to parse this sentence, which may be due to my unfamiliarity with 
> Xen terminology.

It's two sentences, but it's missing a period.
"Non-passthrough (dom0) accesses are local PCI devices and use the
given BDF."  and "Passthough (stubdom) accesses are through PV
frontend PCI device."

> There is also an extra space before "Those".

It's two spaces between sentences, which visually separates the
sentences.  It's a common formatting, so I think it's okay.

Thanks for taking a look.

> >+ * either have a BDF identical to the backend's BFD 
> >(xen-backend.passthrough=1)

(And a typo here: s/BFD/BDF/)

> >+ * or a local virtual BDF (xen-backend.passthrough=0)
> >+ *
> >+ * We are always given the backend's BDF and need to lookup the appropriate
> >+ * local BDF for sysfs access.
> >+ */

Regards,
Jason

Re: [RFC PATCH v2 08/11] hw/arm/smmuv3: Add CMDs related to stage-2

[Mostafa Saleh]

Hi Eric,

On Mon, Mar 20, 2023 at 05:51:07PM +0100, Eric Auger wrote:
> Hi Mostafa,
> 
> On 2/26/23 23:06, Mostafa Saleh wrote:
> > CMD_TLBI_S2_IPA: As S1+S2 is not enabled, for now this can be the
> > same as CMD_TLBI_NH_VAA.
> >
> > CMD_TLBI_S12_VMALL: Added new function to invalidate TLB by VMID.
> >
> > For stage-1 only commands, add a check to to throw CERROR_ILL if used
> s/to to/to
Will do.

> > @@ -1211,12 +1211,22 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> >  {
> >  uint16_t asid = CMD_ASID(&cmd);
> >  
> > +if (!STAGE1_SUPPORTED(s)) {
> > +cmd_error = SMMU_CERROR_ILL;
> Well looking further this is not said explicitly this should return
> 
> SMMU_CERROR_ILL. Maybe you should mark it as a guest error because we do not 
> expect a guest to send such command when S1 is not supported?
> 
I can add a check after the switch for SMMU_CERROR_ILL to log a guest
error. 
> > +break;
> > +}
> > +
> >  trace_smmuv3_cmdq_tlbi_nh_asid(asid);
> >  smmu_inv_notifiers_all(&s->smmu_state);
> >  smmu_iotlb_inv_asid(bs, asid);
> >  break;
> >  }
> >  case SMMU_CMD_TLBI_NH_ALL:
> > +if (!STAGE1_SUPPORTED(s)) {
> > +cmd_error = SMMU_CERROR_ILL;
> > +break;
> 
> there is a VMID field. Can't this be used in S2 mode as well?
According to the user manual "4.4.2 TLB invalidation of stage 1"
CMD_TLBI_NH_ALL causes CERROR_ILL if stage-1 is not supported.

Thanks,
Mostafa

Re: [PULL 2/3] edk2: replace build scripts

[Simon Glass]

Hi Gerd,

On Thu, 16 Mar 2023 at 04:49, Gerd Hoffmann  wrote:
>
>   Hi,
>
> > The README should mention that you need to use
> >
> > . edk2setup.sh
> >
> > first.
>
> The script will do that if needed.
>
> > Also you need to be in the edk2 directory, I think.
>
> Or use the --core switch, or place the location in the config file in
> the [global] section.

That seems to be resolved with the latest script.

>
> > It would be good if the edk2-clone.sh script could deal with updating
> > an existing checkout so I don't need to remove the old ones each time.
>
> Updating is just "git pull && git submodules update".

Even more reason to put it in the script :-)

>
> > edk2-build.py -c ../edk2-build-config/kraxel/x64.platforms -j30 --silent
>
> That config file expects cwd being edk2-platforms and edk2 being placed
> next to it (../ekd2).  edk2-non-osi too.  See the [global] section at
> the start of the file.

OK I see.

>
> > BaseTools/BuildEnv: 160: Bad substitution
> > Using Pip Basetools
> > BaseTools/BuildEnv: 184: Bad substitution
> > BaseTools/BuildEnv: 202: -c: not found
>
> Ok, tried updated the script to use bash not sh for that.  Does this
> work better for you?

Yes, fixed.

>
> > Do I need to make -C BaseTools first?
>
> No, the script will do that.

Seems to work fine now.

>
> > > +import optparse
> >
> > I think this is obsolete and argparse should be used for new things.
> > The conversion is pretty easy.
>
> Done.
>

OK

> > Silent mode still produces output. Can you add a -s alias and also
> > make it fully silent?
>
> Well, silent means no output from the "build" command, so the console is
> not flooded with build logs (unless there are build errors), output is
> written to logfiles instead.

We have a different understanding of 'silent' ::-)

>
> > If the config file is not found, it seems to say nothing, but just
> > does not work. It should give an error.
>
> Fixed.

Works fine.

Thanks for the great script and the fixes.

Regards,
Simon

Re: [RFC PATCH v2 06/11] hw/arm/smmuv3: Make TLB lookup work for stage-2

[Mostafa Saleh]

Hi Eric,

On Mon, Mar 20, 2023 at 05:05:31PM +0100, Eric Auger wrote:
> > +/*
> > + * TLB lookup looks for granule and input size for a translation stage,
> > + * as only one stage is supported right now, choose the right values
> > + * from the configuration.
> > + */
> > +page_mask = (1ULL << granule_sz) - 1;
> >  aligned_addr = addr & ~page_mask;
> >  
> > -cached_entry = smmu_iotlb_lookup(bs, cfg, tt, aligned_addr);
> > +SMMUTransTableInfo temp = {
> Move the declaration at the top. Also rename temp into tt to be more
> explicit about what it is?
I will move it to the top and remove granule_sz and tsz and just assign
values to this struct.
There is a pointer already called tt, I can call it tt_combined as
ideally this will hold the combined attributes for the TLB lookup.

Thanks,
Mostafa

Re: [PATCH v10 1/9] mm: Introduce memfd_restricted system call to create restricted user memory

[Michael Roth]

On Thu, Feb 16, 2023 at 03:21:21PM +0530, Nikunj A. Dadhania wrote:
> 
> > +static struct file *restrictedmem_file_create(struct file *memfd)
> > +{
> > +   struct restrictedmem_data *data;
> > +   struct address_space *mapping;
> > +   struct inode *inode;
> > +   struct file *file;
> > +
> > +   data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +   if (!data)
> > +   return ERR_PTR(-ENOMEM);
> > +
> > +   data->memfd = memfd;
> > +   mutex_init(&data->lock);
> > +   INIT_LIST_HEAD(&data->notifiers);
> > +
> > +   inode = alloc_anon_inode(restrictedmem_mnt->mnt_sb);
> > +   if (IS_ERR(inode)) {
> > +   kfree(data);
> > +   return ERR_CAST(inode);
> > +   }
> 
> alloc_anon_inode() uses new_pseudo_inode() to get the inode. As per the 
> comment, new inode 
> is not added to the superblock s_inodes list.

Another issue somewhat related to alloc_anon_inode() is that the shmem code
in some cases assumes the inode struct was allocated via shmem_alloc_inode(),
which allocates a struct shmem_inode_info, which is a superset of struct inode
with additional fields for things like spinlocks.

These additional fields don't get allocated/ininitialized in the case of
restrictedmem, so when restrictedmem_getattr() tries to pass the inode on to
shmem handler, it can cause a crash.

For instance, the following trace was seen when executing 'sudo lsof' while a
process/guest was running with an open memfd FD:

[24393.121409] general protection fault, probably for non-canonical address 
0xfe9fb182fea3f077:  [#1] PREEMPT SMP NOPTI
[24393.133546] CPU: 2 PID: 590073 Comm: lsof Tainted: GE  
6.1.0-rc4-upm10b-host-snp-v8b+ #4
[24393.144125] Hardware name: AMD Corporation ETHANOL_X/ETHANOL_X, BIOS 
RXM1009B 05/14/2022
[24393.153150] RIP: 0010:native_queued_spin_lock_slowpath+0x3a3/0x3e0
[24393.160049] Code: f3 90 41 8b 04 24 85 c0 74 ea eb f4 c1 ea 12 83 e0 03 
83 ea 01 48 c1 e0 05 48 63 d2 48 05 00 41 04 00 48 03 04 d5 e0 ea 8b 82 <48> 89 
18 8b 43 08 85 c0 75 09 f3 90 8b 43 08 85 c0 74 f7 48 8b 13
[24393.181004] RSP: 0018:c9006b6a3cf8 EFLAGS: 00010086
[24393.186832] RAX: fe9fb182fea3f077 RBX: 889fcc144100 RCX: 

[24393.194793] RDX: 3ffe RSI: 827acde9 RDI: 
c9006b6a3cdf
[24393.202751] RBP: c9006b6a3d20 R08: 0001 R09: 

[24393.210710] R10:  R11:  R12: 
888179fa50e0
[24393.218670] R13: 889fcc144100 R14: 000c R15: 
000c
[24393.226629] FS:  7f9440f45400() GS:889fcc10() 
knlGS:
[24393.235692] CS:  0010 DS:  ES:  CR0: 80050033
[24393.242101] CR2: 55c55a9cf088 CR3: 0008000220e9c003 CR4: 
00770ee0
[24393.250059] PKRU: 5554
[24393.253073] Call Trace:
[24393.255797]  
[24393.258133]  do_raw_spin_lock+0xc4/0xd0
[24393.262410]  _raw_spin_lock_irq+0x50/0x70
[24393.266880]  ? shmem_getattr+0x4c/0xf0
[24393.271060]  shmem_getattr+0x4c/0xf0
[24393.275044]  restrictedmem_getattr+0x34/0x40
[24393.279805]  vfs_getattr_nosec+0xbd/0xe0
[24393.284178]  vfs_getattr+0x37/0x50
[24393.287971]  vfs_statx+0xa0/0x150
[24393.291668]  vfs_fstatat+0x59/0x80
[24393.295462]  __do_sys_newstat+0x35/0x70
[24393.299739]  __x64_sys_newstat+0x16/0x20
[24393.304111]  do_syscall_64+0x3b/0x90
[24393.308098]  entry_SYSCALL_64_after_hwframe+0x63/0xcd

As a workaround we've been doing the following, but it's probably not the
proper fix:

  
https://github.com/AMDESE/linux/commit/0378116b5c4e373295c9101727f2cb5112d6b1f4

-Mike

Re: [RFC PATCH v2 05/11] hw/arm/smmuv3: Parse STE config for stage-2

[Mostafa Saleh]

Hi Eric,

On Mon, Mar 20, 2023 at 04:14:48PM +0100, Eric Auger wrote:
> Hi Mostafa,
> 
> On 2/26/23 23:06, Mostafa Saleh wrote:
> > Parse stage-2 configuration from STE and populate it in SMMUS2Cfg.
> > Validity of these value are checked when possible.
> s/these value/field values
Will do.

> >
> > Only AA64 tables are supported and STT is not supported.
> Small Translation Tables (STT)
Will do.
> >
> > According to SMMUv3 user manual "5.2 Stream Table Entry": All fields
> it is not a user manual but rather an IP architecture spec. put the full
> ref?
This is mentioned in the SMMU manual with the same wording “All fields
with an S2 prefix (with the exception of S2VMID) are IGNORED when stage
2 bypasses translation (Config[1] == 0)” in “5.2 Stream Table Entry”
in page 179, ARM IHI0070.E.a

> >  3 files changed, 145 insertions(+)
> >
> > diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> > index 183d5ac8dc..3388e1a5f8 100644
> > --- a/hw/arm/smmuv3-internal.h
> > +++ b/hw/arm/smmuv3-internal.h
> > @@ -526,9 +526,13 @@ typedef struct CD {
> >  #define STE_S2TG(x)extract32((x)->word[5], 14, 2)
> >  #define STE_S2PS(x)extract32((x)->word[5], 16, 3)
> >  #define STE_S2AA64(x)  extract32((x)->word[5], 19, 1)
> > +#define STE_S2ENDI(x)  extract32((x)->word[5], 20, 1)
> > +#define STE_S2AFFD(x)  extract32((x)->word[5], 21, 1)
> I am afraid there is a shift in the fields below. S2HD should be 23
> (problem in the original code) and everything is shifted.
Oh, yes, I should have checked that, I just relied they are next to it.
I will fix them.

> > +/*
> > + * Max valid value is 39 when SMMU_IDR3.STT == 0.
> > + * In architectures after SMMUv3.0:
> > + * - If STE.S2TG selects a 4KB or 16KB granule, the minimum valid value 
> > for this
> > + * field is MAX(16, 64-IAS)
> > + * - If STE.S2TG selects a 64KB granule, the minimum valid value for this 
> > field
> > + * is (64-IAS).
> > + * As we only support AA64, IAS = OAS.
> OK this comes from STE.S2T0SZ description on the SMMU arch spec. You can
> add this in previous patch too.
> > + */
> > +static bool t0sz_valid(SMMUTransCfg *cfg)
> use S2t0sz to avoid confusion with S1 stuff
Will do.

> > +{
> > +if (cfg->s2cfg.tsz > 39) {
> > +return false;
> > +}
> > +
> > +if (cfg->s2cfg.granule_sz == 16) {
> > +return (cfg->s2cfg.tsz >= 64 - cfg->s2cfg.oas);
> > +}
> > +
> > +return (cfg->s2cfg.tsz >= MAX(64 - cfg->s2cfg.oas, 16));
> > +}
> 
> this chapter also states:
> "The usable range of values is further constrained by a function of the
> starting level set by S2SL0 and, if S2AA64 == 1, granule size set by
> S2TG as described by the Armv8 translation system. Use of a value of
> S2T0SZ that is inconsistent with the permitted range (given S2SL0 and
> S2TG) is ILLEGAL."
Yes, my understanding is that with some configurations the values of
S2SL0 and S2T0SZ are correct but the final configuration will lead to
input address that needs more than 16 concatenated tables which is
invalid, this is checked in s2_pgtable_config_valid()

For example:
A configuration with granularity 4K (granule_sz = 12)
SL0 = 1 (start level = 1)
S2T0SZ = 16 (48 bits)
This means that the 3 levels would cover 3*9 = 27 + 12 = 39 bits
So there are extra 48-39 = 9 bits which requires 512 concatenated
tables. This is invalid.

> > +
> > +/*
> > + * Return true if s2 page table config is valid.
> > + * This checks with the configured start level, ias_bits and granularity 
> > we can
> > + * have a valid page table as described in ARM ARM D8.2 Translation 
> > process.
> > + * The idea here is to see for the highest possible number of IPA bits, how
> > + * many concatenated tables we would need, if it is more than 16, then 
> > this is
> > + * not possible.
> why? in that case doesn't it mean that we can't use concatanated tables?
Yes, that means that we would need more than 16 tables, as mentioned
above this is illegal.

> > +goto bad_ste;
> > +}
> > +
> > +if (STAGE2_SUPPORTED(s)) {
> > +/* VMID is considered even if s2 is disabled. */
> > +cfg->s2cfg.vmid = STE_S2VMID(ste);
> > +} else {
> > +/* Default to -1 */
> > +cfg->s2cfg.vmid = -1;
> > +}
> > +
> >  if (STE_CFG_S2_ENABLED(config)) {
> I think it would improve the readability to introduce a separate
> function decode_ste_s2_cftg() taking the s2cfg to be populated
Yes, will do.

> > +
> > +if (!s2_pgtable_config_valid(cfg->s2cfg.sl0, cfg->s2cfg.tsz,
> > + cfg->s2cfg.granule_sz)) {
> > +qemu_log_mask(LOG_GUEST_ERROR,
> > +  "SMMUv3 STE stage 2 config not valid!\n");
> > +goto bad_ste;
> > +}
> > +
> > +/* Only LE supported(IDR0.TTENDIAN). */
> > +if (STE_S2ENDI(ste)) {
> qemu_log
Will do.

Thanks,
Mostafa

Re: [RFC PATCH v2 04/11] hw/arm/smmuv3: Add page table walk for stage-2

[Mostafa Saleh]

Hi Eric,

On Mon, Mar 20, 2023 at 03:56:26PM +0100, Eric Auger wrote:
> Hi Mostafa,
> 
> On 2/26/23 23:06, Mostafa Saleh wrote:
> > In preparation for adding stage-2 support, add Stage-2 PTW code.
> > Only Aarch64 format is supported as stage-1.
> >
> > Nesting stage-1 and stage-2 is not supported right now.
> >
> > HTTU is not supported, SW is expected to maintain the Access flag.
> > This is described in the SMMUv3 manual "5.2. Stream Table Entry" in
> > "[181] S2AFFD".
> > This flag determines the behavior on access of a stage-2 page whose
> > descriptor has AF == 0:
> > - 0b0: An Access flag fault occurs (stall not supported).
> > - 0b1: An Access flag fault never occurs.
> > An Access fault takes priority over a Permission fault.
> >
> > Checks for IPA and output PA are done according to the user manual
> > "3.4 Address sizes".
> replace user manual by the exact ref of the doc. I guess this is IHI0070E
Will do.

> > + * Return 0 on success, < 0 on error. In case of error, @info is filled
> > + * and tlbe->perm is set to IOMMU_NONE.
> > + * Upon success, @tlbe is filled with translated_addr and entry
> > + * permission rights.
> > + */
> > +static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
> > +  dma_addr_t iova, IOMMUAccessFlags perm,
> Rename iova into ipa?
Will do.

> > +  SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> > +{
> > +const int stage = 2;
> > +int granule_sz = cfg->s2cfg.granule_sz;
> > +/* ARM ARM: Table D8-7. */
> You may refer the full reference
> in  DDI0487I.a as the chapter/table may vary. For instance in
> ARM DDI 0487F.c D8 corresponds to the activity monitor extensions
Will do.

> > +int inputsize = 64 - cfg->s2cfg.tsz;
> > +int level = get_start_level(cfg->s2cfg.sl0, granule_sz);
> > +int stride = SMMU_STRIDE(granule_sz);
> > +int idx = pgd_idx(level, granule_sz, iova);
> > +/*
> > + * Get the ttb from concatenated structure.
> > + * The offset is the idx * size of each ttb(number of ptes * 
> > (sizeof(pte))
> > + */
> what I don't get is the spec that concatenated tables are used if the
> initial lookup level would require less or equal than 16 entries. I
> don't see such kind of check or is done implicitly somewhere else?
Yes, this is checked in the STE patch in function
s2_pgtable_config_valid, where it checks that the max input will not
need more than 16 tables which means that the pagetable config is
inconsistent, which means the STE is ILLEGAL.

> > +uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
> > +  idx * sizeof(uint64_t);
> > +dma_addr_t indexmask = SMMU_IDXMSK(inputsize, stride, level);
> > +
> > +baseaddr &= ~indexmask;
> > +
> > +/*
> > + * If the input address of a transaction exceeds the size of the IAS, a
> > + * stage 1 Address Size fault occurs.
> > + * For AA64, IAS = OAS
> then you may use a local variable max_as = cfg->s2cfg.oas used below and
> in 
> 
> if (gpa >= (1ULL << cfg->s2cfg.oas)) {
> this is not obvious though that the ias = oas. Where does it come from?
> 
> In implementations of SMMUv3.1 and later, this value is Reserved and S2PS 
> behaves as 0b110 (52 bits).
> Effective_S2PS = MIN(STE.S2PS, SMMU_IDR5.OAS);
> OAS is a maximum of 52 bits in SMMUv3.1 and later
IAS = OAS for AA64. Described in "3.4 Address sizes".
The first check is actually not correct, as input address should be
compared to IAS(or OAS) while s2cfg.oas is effective PS.
I will rename s2cfg.oas to s2cfg.eff_ps to avoid confusion.
I will change the check here to be against s2t0sz and in this case it
causes stage-2 addr size fault.

I think the check for the IAS shouldn't be done here.

> > + */
> > +if (iova >= (1ULL << cfg->s2cfg.oas)) {
> > +info->type = SMMU_PTW_ERR_ADDR_SIZE;
> > +info->stage = 1;
> > +goto error_no_stage;
> > +}
> > +
> > +while (level < SMMU_LEVELS) {
> I would rename SMMU_LEVELs
You mean replace SMMU_LEVELS with SMMU_LEVELs?

> >  
> > +#define PTE_AF(pte) \
> > +(extract64(pte, 10, 1))
> >  /*
> >   * TODO: At the moment all transactions are considered as privileged (EL1)
> >   * as IOMMU translation callback does not pass user/priv attributes.
> > @@ -73,6 +75,9 @@
> >  #define is_permission_fault(ap, perm) \
> >  (((perm) & IOMMU_WO) && ((ap) & 0x2))
> >  
> > +#define is_permission_fault_s2(ap, perm) \
> I would rename ap param into s2ap. This is the name of the field in the spec
Will do.

> > +(!((ap & perm) == perm))
> > +
> >  #define PTE_AP_TO_PERM(ap) \
> >  (IOMMU_ACCESS_FLAG(true, !((ap) & 0x2)))
> >  
> > @@ -96,6 +101,40 @@ uint64_t iova_level_offset(uint64_t iova, int inputsize,
> >  MAKE_64BIT_MASK(0, gsz - 3);
> >  }
> >  
> > +#define SMMU_MAX_S2_CONCAT16
> it is not really an SMMU property (same as SMMU_LEVELS by the way). This
> is rather something related to VMSA spec, no?.
Yes, they are part of VMS

Re: [PATCH] xen: Fix host pci for stubdom

[Bernhard Beschow]

Am 20. März 2023 00:05:54 UTC schrieb Jason Andryuk :
>PCI passthrough for an HVM with a stubdom is PV PCI passthrough from
>dom0 to the stubdom, and then QEMU passthrough of the PCI device inside
>the stubdom.  xen-pciback has boolean module param passthrough which
>controls "how to export PCI topology to guest".  If passthrough=1, the
>frontend shows a PCI SBDF matching the backend host device.  When
>passthough=0, the frontend will get a sequentially allocated SBDF.
>
>libxl passes the host SBDF over QMP to QEMU.  For non-stubdom or stubdom
>with passthrough=1, this works fine.  However, it fails for
>passthrough=0 when QEMU can't find the sysfs node for the host SBDF.
>
>Handle all these cases.  Look for the xenstore frontend nodes.  If they
>are missing, then default to using the QMP command provided SBDF.  This
>is the non-stubdom case.  If xenstore nodes are found, then read the
>local SBDF from the xenstore nodes.  This will handle either
>passthrough=0/1 case.
>
>Based on a stubdom-specific patch originally by Marek
>Marczykowski-Górecki , which is based
>on earlier work by HW42 
>
>Signed-off-by: Jason Andryuk 
>---
> hw/xen/xen-host-pci-device.c | 96 +++-
> hw/xen/xen-host-pci-device.h |  6 +++
> 2 files changed, 101 insertions(+), 1 deletion(-)
>
>diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
>index 8c6e9a1716..51a72b432d 100644
>--- a/hw/xen/xen-host-pci-device.c
>+++ b/hw/xen/xen-host-pci-device.c
>@@ -9,6 +9,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "qemu/cutils.h"
>+#include "hw/xen/xen-legacy-backend.h"
> #include "xen-host-pci-device.h"
> 
> #define XEN_HOST_PCI_MAX_EXT_CAP \
>@@ -33,13 +34,101 @@
> #define IORESOURCE_PREFETCH 0x1000  /* No side effects */
> #define IORESOURCE_MEM_64   0x0010
> 
>+/*
>+ * Non-passthrough (dom0) accesses are local PCI devices and use the given BDF
>+ * Passthough (stubdom) accesses are through PV frontend PCI device.  Those

I'm unable to parse this sentence, which may be due to my unfamiliarity with 
Xen terminology.

There is also an extra space before "Those".

Best regards,
Bernhard

>+ * either have a BDF identical to the backend's BFD 
>(xen-backend.passthrough=1)
>+ * or a local virtual BDF (xen-backend.passthrough=0)
>+ *
>+ * We are always given the backend's BDF and need to lookup the appropriate
>+ * local BDF for sysfs access.
>+ */
>+static void xen_host_pci_fill_local_addr(XenHostPCIDevice *d, Error **errp)
>+{
>+unsigned int num_devs, len, i;
>+unsigned int domain, bus, dev, func;
>+char *be_path;
>+char path[80];
>+char *msg;
>+
>+be_path = qemu_xen_xs_read(xenstore, 0, "device/pci/0/backend", &len);
>+if (!be_path) {
>+/*
>+ * be_path doesn't exist, so we are dealing with a local
>+ * (non-passthough) device.
>+ */
>+d->local_domain = d->domain;
>+d->local_bus = d->bus;
>+d->local_dev = d->dev;
>+d->local_func = d->func;
>+
>+return;
>+}
>+
>+snprintf(path, sizeof(path), "%s/num_devs", be_path);
>+msg = qemu_xen_xs_read(xenstore, 0, path, &len);
>+if (!msg) {
>+goto err_out;
>+}
>+
>+if (sscanf(msg, "%u", &num_devs) != 1) {
>+error_setg(errp, "Failed to parse %s (%s)", msg, path);
>+goto err_out;
>+}
>+free(msg);
>+
>+for (i = 0; i < num_devs; i++) {
>+snprintf(path, sizeof(path), "%s/dev-%u", be_path, i);
>+msg = qemu_xen_xs_read(xenstore, 0, path, &len);
>+if (!msg) {
>+error_setg(errp, "Failed to read %s", path);
>+goto err_out;
>+}
>+if (sscanf(msg, "%x:%x:%x.%x", &domain, &bus, &dev, &func) != 4) {
>+error_setg(errp, "Failed to parse %s (%s)", msg, path);
>+goto err_out;
>+}
>+free(msg);
>+if (domain != d->domain ||
>+bus != d->bus ||
>+dev != d->dev ||
>+func != d->func)
>+continue;
>+snprintf(path, sizeof(path), "%s/vdev-%u", be_path, i);
>+msg = qemu_xen_xs_read(xenstore, 0, path, &len);
>+if (!msg) {
>+error_setg(errp, "Failed to read %s", path);
>+goto out;
>+}
>+if (sscanf(msg, "%x:%x:%x.%x", &domain, &bus, &dev, &func) != 4) {
>+error_setg(errp, "Failed to parse %s (%s)", msg, path);
>+goto err_out;
>+}
>+free(msg);
>+d->local_domain = domain;
>+d->local_bus = bus;
>+d->local_dev = dev;
>+d->local_func = func;
>+goto out;
>+}
>+
>+error_setg(errp, "Failed to find local %x:%x:%x.%x", d->domain, d->bus,
>+   d->dev, d->func);
>+
>+err_out:
>+free(msg);
>+out:
>+free(be_path);
>+}
>+
> static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
> const char *name, char *buf, ssize_t size)
> {
> int r

Re: Using QEMU how to redirect serial /dev/ttyS2 output of guest machine to host machine.

[Abhishek Singh Dagur]

Thanks a lot. It's working fine 😁 now.

Abhishek.

On Sun, 19 Mar, 2023, 11:27 pm Cédric Le Goater,  wrote:

> Hello Abhishek,
>
> On 3/18/23 18:55, Abhishek Singh Dagur wrote:
> > Hi Alex , Cédric,
> > we have tried this *option:-serial *pty* -serial *pty* -serial *pty
> *-serial *pty *-serial *pty to check the serial output on screen.
> >
> > Here we are providing 2 scenarios without and with the changes suggested
> by @Cédric Le Goater  .
> > In first we are able to get /dev/ttyS4 serial port not /dev/ttyS2
> > In second we are able to get /dev/ttyS2 not /dev/ttyS4
> >
> > *Scenario1:previously with normal qemu build i have tried emulation
> as:*
> >
> > qemu-system-arm -m 512 -M
> ast2500-evb,fmc-model=mx66u51235f,spi-model=mx66u51235f -nographic -drive
> file=./pru-1.2.4_dev-rc1.static.mtd,format=raw,if=mtd -serial pty -serial
> pty -serial pty -serial pty
> >
> > which gives us output as.
> >
> > QEMU 7.2.0 monitor - type 'help' for more information
> > (qemu) char device redirected to /dev/pts/15 (label serial0)
> > char device redirected to /dev/pts/16 (label serial1)
> > char device redirected to /dev/pts/17 (label serial2)
> > char device redirected to /dev/pts/18 (label serial3)
> >
> > so we can use *screen *to interact with the ttyS4 serial port like :
> > screen /dev/pts/15
> > In which we have our boot process and shell prompt .
> >
> > *Scenario2:* *when I am using the newly build image with the changes
> *@Cédric Le Goater *  provided*
> >
> > With the below command:
> > ./qemu-system-arm -m 512 -M
> ast2500-evb,uart-default=uart2,fmc-model=mx66u51235f,spi-model=mx66u51235f
> -nographic -drive file=./pru-1.2.4_dev-rc1.static.mtd,format=raw,if=mtd
> -serial pty -serial pty -serial pty -serial pty
>
> Since the machine expect /dev/ttyS2 to be the boot console, the SoC device
> to attach to the first serial is UART3. This command line should output
> the console logs in the same terminal :
>
>./qemu-system-arm -m 512 -M
> ast2500-evb,uart-default=uart3,fmc-model=mx66u51235f,spi-model=mx66u51235f
> -net user -nographic -drive
> file=./pru-1.2.4_dev-rc1.static.mtd,format=raw,if=mtd -serial mon:stdio
>
> I have updated the patch in my aspeed-8.0 tree :
>
>
> https://github.com/legoater/qemu/commit/20fe705361dd7ed1d9129747a8a0b643410869e3
>
> Please note that in this last version, the machine option is simply "uart".
>
> Thanks,
>
> C.
>

Re: [PULL 00/24] s390x and misc patches for 8.0-rc1

[Peter Maydell]

On Mon, 20 Mar 2023 at 13:03, Thomas Huth  wrote:
>
>  Hi Peter!
>
> The following changes since commit 74c581b6452394e591f13beba9fea2ec0688e2f5:
>
>   Merge tag 'trivial-branch-for-8.0-pull-request' of 
> https://gitlab.com/laurent_vivier/qemu into staging (2023-03-17 14:22:01 
> +)
>
> are available in the Git repository at:
>
>   https://gitlab.com/thuth/qemu.git tags/pull-request-2023-03-20
>
> for you to fetch changes up to 48805df9c22a0700fba4b3b548fafaa21726ca68:
>
>   replace TABs with spaces (2023-03-20 12:43:50 +0100)
>
> 
> * Mark Nios II as orphan
> * Many s390x emulation fixes
> * Disable flaky complete_in_standby blockjob unit test
> * White space cleanups in various files
>
> 


Applied, thanks.

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

-- PMM

Re: [PULL 0/6] Edk2 stable202302 20230320 patches

[Peter Maydell]

On Mon, 20 Mar 2023 at 09:39, Gerd Hoffmann  wrote:
>
> The following changes since commit 74c581b6452394e591f13beba9fea2ec0688e2f5:
>
>   Merge tag 'trivial-branch-for-8.0-pull-request' of 
> https://gitlab.com/laurent_vivier/qemu into staging (2023-03-17 14:22:01 
> +)
>
> are available in the Git repository at:
>
>   https://gitlab.com/kraxel/qemu.git 
> tags/edk2-stable202302-20230320-pull-request
>
> for you to fetch changes up to 86305e864191123dcf87c3af639fddfc59352ac6:
>
>   edk2: update firmware binaries (2023-03-20 10:36:31 +0100)
>
> 
> update edk2 to 202302 stable tag
>


Applied, thanks.

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

-- PMM

Re: [PATCH v2 0/7] plugin: fix clearing of plugin_mem_cbs on TB exit

[Alex Bennée]

Emilio Cota  writes:

> On Fri, Mar 10, 2023 at 11:52:45 -0800, Richard Henderson wrote:
>> Changes for v2:
> (snip)
>> Richard Henderson (7):
>>   tcg: Clear plugin_mem_cbs on TB exit
>>   tcg: Drop plugin_gen_disable_mem_helpers from tcg_gen_exit_tb
>>   include/qemu/plugin: Remove QEMU_PLUGIN_ASSERT
>>   *: Add missing includes of qemu/error-report.h
>>   *: Add missing includes of qemu/plugin.h
>>   include/qemu: Split out plugin-event.h
>>   include/qemu/plugin: Inline qemu_plugin_disable_mem_helpers
>
> Reviewed-by: Emilio Cota 
>
> Thanks, Richard!
>
> Alex: is it too late to add my R-b tags to the series?

I have to re-roll so I'll do that now.

>
>   Emilio


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Alex Bennée]

Claudio Fontana  writes:

> Hi Alex, all,
>
> again, this moves TCG-only code to common code, no?
>
> Even if this happens to work, the idea is to avoid adding unneeded accel TCG 
> code to a KVM-only binary.
>
> We need to keep in mind all dimensions when we do refactorings:
>
> user-mode vs sysemu,
> the architecture,
> the accel, in particular tcg, non-tcg (which could be not compiled in,
> built-in, or loaded as separate module).
>
> In many cases, testing with --disable-tcg --enable-kvm helps to avoid 
> breakages,
> but it is possible also to move in unneeded code in a way that does
> not generate compile or link-time errors, so we need to be a bit alert
> to that.

You are right of course, it should be kept tcg only. I guess using
tcgops instead of sysemu_ops.

>
> Ciao,
>
> C 
>
>
> On 3/20/23 11:10, Alex Bennée wrote:
>> We don't want to be polluting the core run loop code with target
>> specific handling, punt it to sysemu_ops where it belongs.
>> 
>> Signed-off-by: Alex Bennée 
>> ---
>>  include/hw/core/sysemu-cpu-ops.h |  5 +
>>  target/i386/cpu-internal.h   |  1 +
>>  accel/tcg/cpu-exec.c | 14 +++---
>>  target/i386/cpu-sysemu.c | 12 
>>  target/i386/cpu.c|  1 +
>>  5 files changed, 22 insertions(+), 11 deletions(-)
>> 
>> diff --git a/include/hw/core/sysemu-cpu-ops.h 
>> b/include/hw/core/sysemu-cpu-ops.h
>> index ee169b872c..c9d30172c4 100644
>> --- a/include/hw/core/sysemu-cpu-ops.h
>> +++ b/include/hw/core/sysemu-cpu-ops.h
>> @@ -48,6 +48,11 @@ typedef struct SysemuCPUOps {
>>   * GUEST_PANICKED events.
>>   */
>>  GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>> +/**
>> + * @handle_cpu_halt: Callback for special handling during 
>> cpu_handle_halt()
>> + * @cs: The CPUState
>> + */
>> +void (*handle_cpu_halt)(CPUState *cpu);
>>  /**
>>   * @write_elf32_note: Callback for writing a CPU-specific ELF note to a
>>   * 32-bit VM coredump.
>> diff --git a/target/i386/cpu-internal.h b/target/i386/cpu-internal.h
>> index 9baac5c0b4..75b302fb33 100644
>> --- a/target/i386/cpu-internal.h
>> +++ b/target/i386/cpu-internal.h
>> @@ -65,6 +65,7 @@ void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
>>  void x86_cpu_apic_create(X86CPU *cpu, Error **errp);
>>  void x86_cpu_apic_realize(X86CPU *cpu, Error **errp);
>>  void x86_cpu_machine_reset_cb(void *opaque);
>> +void x86_cpu_handle_halt(CPUState *cs);
>>  #endif /* !CONFIG_USER_ONLY */
>>  
>>  #endif /* I386_CPU_INTERNAL_H */
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index c815f2dbfd..5e5906e199 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -22,6 +22,7 @@
>>  #include "qapi/error.h"
>>  #include "qapi/type-helpers.h"
>>  #include "hw/core/tcg-cpu-ops.h"
>> +#include "hw/core/sysemu-cpu-ops.h"
>>  #include "trace.h"
>>  #include "disas/disas.h"
>>  #include "exec/exec-all.h"
>> @@ -30,9 +31,6 @@
>>  #include "qemu/rcu.h"
>>  #include "exec/log.h"
>>  #include "qemu/main-loop.h"
>> -#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
>> -#include "hw/i386/apic.h"
>> -#endif
>>  #include "sysemu/cpus.h"
>>  #include "exec/cpu-all.h"
>>  #include "sysemu/cpu-timers.h"
>> @@ -650,15 +648,9 @@ static inline bool cpu_handle_halt(CPUState *cpu)
>>  {
>>  #ifndef CONFIG_USER_ONLY
>>  if (cpu->halted) {
>> -#if defined(TARGET_I386)
>> -if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
>> -X86CPU *x86_cpu = X86_CPU(cpu);
>> -qemu_mutex_lock_iothread();
>> -apic_poll_irq(x86_cpu->apic_state);
>> -cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
>> -qemu_mutex_unlock_iothread();
>> +if (cpu->cc->sysemu_ops->handle_cpu_halt) {
>> +cpu->cc->sysemu_ops->handle_cpu_halt(cpu);
>>  }
>> -#endif /* TARGET_I386 */
>>  if (!cpu_has_work(cpu)) {
>>  return true;
>>  }
>> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
>> index 28115edf44..e545bf7590 100644
>> --- a/target/i386/cpu-sysemu.c
>> +++ b/target/i386/cpu-sysemu.c
>> @@ -18,6 +18,7 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu/main-loop.h"
>>  #include "cpu.h"
>>  #include "sysemu/xen.h"
>>  #include "sysemu/whpx.h"
>> @@ -310,6 +311,17 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>>   }
>>  }
>>  
>> +void x86_cpu_handle_halt(CPUState *cpu)
>> +{
>> +if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
>> +X86CPU *x86_cpu = X86_CPU(cpu);
>> +qemu_mutex_lock_iothread();
>> +apic_poll_irq(x86_cpu->apic_state);
>> +cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
>> +qemu_mutex_unlock_iothread();
>> +}
>> +}
>> +
>>  GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs)
>>  {
>>  X86CPU *cpu = X86_CPU(cs);
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 6576287e5b..67027d28b0 100644
>> --- a/target/i386

Re: [PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> On 20/3/23 11:10, Alex Bennée wrote:
>> We don't want to be polluting the core run loop code with target
>> specific handling, punt it to sysemu_ops where it belongs.
>> Signed-off-by: Alex Bennée 
>> ---
>>   include/hw/core/sysemu-cpu-ops.h |  5 +
>>   target/i386/cpu-internal.h   |  1 +
>>   accel/tcg/cpu-exec.c | 14 +++---
>>   target/i386/cpu-sysemu.c | 12 
>>   target/i386/cpu.c|  1 +
>>   5 files changed, 22 insertions(+), 11 deletions(-)
>
>
>> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
>> index 28115edf44..e545bf7590 100644
>> --- a/target/i386/cpu-sysemu.c
>> +++ b/target/i386/cpu-sysemu.c
>> @@ -18,6 +18,7 @@
>>*/
>> #include "qemu/osdep.h"
>> +#include "qemu/main-loop.h"
>>   #include "cpu.h"
>>   #include "sysemu/xen.h"
>>   #include "sysemu/whpx.h"
>
> Missing "hw/i386/apic.h" which declares apic_poll_irq() ...

I really need to get to the bottom of why some build hosts get away
without the include. Some additional path through the include maze
driven by config-host.h?
>
>
>> @@ -310,6 +311,17 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>>}
>>   }
>>   +void x86_cpu_handle_halt(CPUState *cpu)
>> +{
>> +if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
>> +X86CPU *x86_cpu = X86_CPU(cpu);
>> +qemu_mutex_lock_iothread();
>> +apic_poll_irq(x86_cpu->apic_state);
>
> ... used here.
>
>> +cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
>> +qemu_mutex_unlock_iothread();
>> +}
>> +}
>
> Otherwise,
>
> Reviewed-by: Philippe Mathieu-Daudé 


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 08/10] accel/tcg: push i386 specific hacks into handle_cpu_interrupt callback

[Alex Bennée]

Richard Henderson  writes:

> On 3/20/23 03:10, Alex Bennée wrote:
>> Signed-off-by: Alex Bennée 
>> ---
>>   include/hw/core/sysemu-cpu-ops.h | 11 +++
>>   target/i386/cpu-internal.h   |  1 +
>>   accel/tcg/cpu-exec-softmmu.c | 16 
>>   accel/tcg/cpu-exec.c | 31 ++-
>>   target/i386/cpu-sysemu.c | 17 +
>>   target/i386/cpu.c|  1 +
>>   6 files changed, 56 insertions(+), 21 deletions(-)
>> diff --git a/include/hw/core/sysemu-cpu-ops.h
>> b/include/hw/core/sysemu-cpu-ops.h
>> index c9d30172c4..d53907b517 100644
>> --- a/include/hw/core/sysemu-cpu-ops.h
>> +++ b/include/hw/core/sysemu-cpu-ops.h
>> @@ -53,6 +53,15 @@ typedef struct SysemuCPUOps {
>>* @cs: The CPUState
>>*/
>>   void (*handle_cpu_halt)(CPUState *cpu);
>> +/**
>> + * @handle_cpu_interrupt: handle init/reset interrupts
>> + * @cs: The CPUState
>> + * @irq_request: the interrupt request
>> + *
>> + * Most architectures share a common handler. Returns true if the
>> + * handler did indeed handle and interrupt.
>> + */
>
> and -> the? or any?
>
> This should be a tcg hook, not a sysemu hook, per the previous one.
> I would very much like it to never be NULL, but instead your new
> common_cpu_handle_interrupt function.

I was trying to figure out how to instantiate a default but ran into
const problems eventually forcing me to give up.

Why a TCG hook? Do we not process any interrupts for KVM or HVF?

>
>> -#if defined(TARGET_I386)
>> -else if (interrupt_request & CPU_INTERRUPT_INIT) {
>> -X86CPU *x86_cpu = X86_CPU(cpu);
>> -CPUArchState *env = &x86_cpu->env;
>> -replay_interrupt();
>> -cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
>> -do_cpu_init(x86_cpu);
>> -cpu->exception_index = EXCP_HALTED;
>> -return true;
>> -}
>> -#else
>> -else if (interrupt_request & CPU_INTERRUPT_RESET) {
>> -replay_interrupt();
>> -cpu_reset(cpu);
>> +else if (cpu->cc->sysemu_ops->handle_cpu_interrupt &&
>> + cpu->cc->sysemu_ops->handle_cpu_interrupt(cpu, 
>> interrupt_request)) {
>> +return true;
>> +} else if (common_cpu_handle_interrupt(cpu, interrupt_request)) {
>>   return true;
>
> ... because this is pretty ugly, and incorrectly indented.
>
>
> r~


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PULL 05/31] gitlab: update centos-8-stream job

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> On 18/3/23 12:46, Alex Bennée wrote:
>> A couple of clean-ups here:
>>- inherit from the custom runners job for artefacts
>
> "artifacts"
>
>>- call check-avocado directly
>>- add some comments to the top about setup
>> Signed-off-by: Alex Bennée 
>> Reviewed-by: Thomas Huth 
>> Message-Id: <20230315174331.2959-6-alex.ben...@linaro.org>
>> diff --git a/.gitlab-ci.d/custom-runners/centos-stream-8-x86_64.yml
>> b/.gitlab-ci.d/custom-runners/centos-stream-8-x86_64.yml
>> index 068b0c4335..367424db78 100644
>> --- a/.gitlab-ci.d/custom-runners/centos-stream-8-x86_64.yml
>> +++ b/.gitlab-ci.d/custom-runners/centos-stream-8-x86_64.yml
>> @@ -1,4 +1,9 @@
>> +# All centos-stream-8 jobs should run successfully in an environment
>> +# setup by the scripts/ci/setup/stream/8/build-environment.yml task
>> +# "Installation of extra packages to build QEMU"
>> +
>>   centos-stream-8-x86_64:
>> + extends: .custom_runner_template
>>allow_failure: true
>>needs: []
>>stage: build
>> @@ -8,15 +13,6 @@ centos-stream-8-x86_64:
>>rules:
>>- if: '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH =~ 
>> /^staging/'
>>- if: "$CENTOS_STREAM_8_x86_64_RUNNER_AVAILABLE"
>> - artifacts:
>> -   name: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG"
>> -   when: on_failure
>> -   expire_in: 7 days
>> -   paths:
>> - - build/tests/results/latest/results.xml
>> - - build/tests/results/latest/test-results
>> -   reports:
>> - junit: build/tests/results/latest/results.xml
>>before_script:
>>- JOBS=$(expr $(nproc) + 1)
>>script:
>> @@ -25,6 +21,4 @@ centos-stream-8-x86_64:
>>- ../scripts/ci/org.centos/stream/8/x86_64/configure
>>  || { cat config.log meson-logs/meson-log.txt; exit 1; }
>>- make -j"$JOBS"
>> - - make NINJA=":" check
>> -   || { cat meson-logs/testlog.txt; exit 1; } ;
>> - - ../scripts/ci/org.centos/stream/8/x86_64/test-avocado
>> + - make NINJA=":" check check-avocado
>
> Missing removing scripts/ci/org.centos/stream/8/x86_64/test-avocado
> along with this patch.

Given the failure running check-avocado maybe I should drop this part of
the change. I was aiming for increasing coverage but perhaps we should
stick with the limited cross section of tests?

Anyway I'm re-building my Centos8 Stream box now with more disk space to
see if I can see whats going wrong.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

[Eric Auger]

Hi Mostafa,

On 2/26/23 23:06, Mostafa Saleh wrote:
> OAS used to be hardcoded to 44 bits, however according to SMMU manual
> 6.3.6 SMMU_IDR5, OAS must match the system physical address size, so
> we read it from CPU PARANGE.
>
> Remove PA_MAX and pa_range as they were not used.
>
> Add SMMUv3State as an argument to decode_cd, so it can read the SMMU
> OAS.
>
> As CPU can use PARANGE with 52 bits, add 52 bits check to oas2bits,
> and cap OAS to 48 bits for stage-1 and stage-2 if granule is not 64KB
> as specified for SMMUv3.1 and later.
>
> Signed-off-by: Mostafa Saleh 
> ---
>  hw/arm/smmu-common.c | 13 +
>  hw/arm/smmuv3-internal.h | 15 ++-
>  hw/arm/smmuv3.c  | 41 ++--
>  3 files changed, 46 insertions(+), 23 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index e4b477af10..3a2b06fd7f 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -307,7 +307,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
>  dma_addr_t baseaddr, indexmask;
>  int stage = cfg->stage;
>  SMMUTransTableInfo *tt = select_tt(cfg, iova);
> -uint8_t level, granule_sz, inputsize, stride;
> +uint8_t level, granule_sz, inputsize, stride, oas;
>  
>  if (!tt || tt->disabled) {
>  info->type = SMMU_PTW_ERR_TRANSLATION;
> @@ -319,7 +319,12 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
>  inputsize = 64 - tt->tsz;
>  level = 4 - (inputsize - 4) / stride;
>  indexmask = SMMU_IDXMSK(inputsize, stride, level);
> -baseaddr = extract64(tt->ttb, 0, 48);
> +oas = cfg->oas;
> +if (tt->granule_sz != 16) {
> +oas = MIN(oas, 48);
> +}
> +
> +baseaddr = extract64(tt->ttb, 0, oas);
>  baseaddr &= ~indexmask;
>  
>  while (level < SMMU_LEVELS) {
> @@ -416,8 +421,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
>   * Get the ttb from concatenated structure.
>   * The offset is the idx * size of each ttb(number of ptes * 
> (sizeof(pte))
>   */
> -uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
> -  idx * sizeof(uint64_t);
> +uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, cfg->s2cfg.oas) +
> +  (1 << stride) * idx * sizeof(uint64_t);
>  dma_addr_t indexmask = SMMU_IDXMSK(inputsize, stride, level);
>  
>  baseaddr &= ~indexmask;
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 3388e1a5f8..25ae12fb5c 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -564,23 +564,12 @@ static inline int oas2bits(int oas_field)
>  return 44;
>  case 5:
>  return 48;
> +case 6:
> +return 52;
>  }
>  return -1;
>  }
>  
> -static inline int pa_range(STE *ste)
> -{
> -int oas_field = MIN(STE_S2PS(ste), SMMU_IDR5_OAS);
> -
> -if (!STE_S2AA64(ste)) {
> -return 40;
> -}
> -
> -return oas2bits(oas_field);
> -}
> -
> -#define MAX_PA(ste) ((1 << pa_range(ste)) - 1)
> -
>  /* CD fields */
>  
>  #define CD_VALID(x)   extract32((x)->word[0], 31, 1)
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 7297f6adc1..bc4ec202f4 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -238,6 +238,13 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo 
> *info)
>  
>  static void smmuv3_init_regs(SMMUv3State *s)
>  {
> +/*
> + * According to 6.3.6 SMMU_IDR5, OAS must match the system physical 
> address
> + * size.
> + */
> +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> +uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, 
> PARANGE);
is this working in accelerated mode?
> +
>  /**
>   * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
>   *   multi-level stream table
> @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> -s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits 
> */
> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
I am not sure you can change that easily. In case of migration this is
going to change the behavior of the device, no?

Thanks

Eric
>  
>  s->cmdq.base = deposit64(s->cmdq.base, 0, 5, SMMU_CMDQS);
>  s->cmdq.prod = 0;
> @@ -374,6 +381,7 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>STE *ste, SMMUEventInfo *event)
>  {
>  uint32_t config;
> +uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
>  
>  if (!STE_VALID(ste)) {
>  if (!event->inval_ste_allowed) {
> @@ -450,7 +458,16 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>  }
>  
>  
> -cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
> +cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), o

Re: [RFC PATCH v2 09/11] hw/arm/smmuv3: Add stage-2 support in iova notifier

[Eric Auger]

Hi,

On 2/26/23 23:06, Mostafa Saleh wrote:
> In smmuv3_notify_iova, read the granule based on translation stage
> and use VMID if valid value is sent.
>
> Signed-off-by: Mostafa Saleh 
> ---
>  hw/arm/smmuv3.c | 39 ++-
>  hw/arm/trace-events |  2 +-
>  2 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 8c76a48c8d..7297f6adc1 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -971,18 +971,21 @@ epilogue:
>   * @mr: IOMMU mr region handle
>   * @n: notifier to be called
>   * @asid: address space ID or negative value if we don't care
> + * @vmid: virtual machine ID or negative value if we don't care
>   * @iova: iova
>   * @tg: translation granule (if communicated through range invalidation)
>   * @num_pages: number of @granule sized pages (if tg != 0), otherwise 1
>   */
>  static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
> IOMMUNotifier *n,
> -   int asid, dma_addr_t iova,
> -   uint8_t tg, uint64_t num_pages)
> +   int asid, int vmid,
> +   dma_addr_t iova, uint8_t tg,
> +   uint64_t num_pages)
>  {
>  SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
>  IOMMUTLBEvent event;
>  uint8_t granule;
> +SMMUv3State *s = sdev->smmu;

I am not sure notifiers are likely to be registered in the S2 case
because it is mostly meant to integrate with vhost or VFIO. The code in
this patch would rather prepare for nested stage support I guess, I
don't think it can harm.

Reviewed-by: Eric Auger 

Eric

>  
>  if (!tg) {
>  SMMUEventInfo event = {.inval_ste_allowed = true};
> @@ -997,11 +1000,20 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
>  return;
>  }
>  
> -tt = select_tt(cfg, iova);
> -if (!tt) {
> +if (vmid >= 0 && cfg->s2cfg.vmid != vmid) {
>  return;
>  }
> -granule = tt->granule_sz;
> +
> +if (STAGE1_SUPPORTED(s)) {
> +tt = select_tt(cfg, iova);
> +if (!tt) {
> +return;
> +}
> +granule = tt->granule_sz;
> +} else {
> +granule = cfg->s2cfg.granule_sz;
> +}
> +
>  } else {
>  granule = tg * 2 + 10;
>  }
> @@ -1015,9 +1027,10 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
>  memory_region_notify_iommu_one(n, &event);
>  }
>  
> -/* invalidate an asid/iova range tuple in all mr's */
> -static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, dma_addr_t 
> iova,
> -  uint8_t tg, uint64_t num_pages)
> +/* invalidate an asid/vmid/iova range tuple in all mr's */
> +static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid,
> +  dma_addr_t iova, uint8_t tg,
> +  uint64_t num_pages)
>  {
>  SMMUDevice *sdev;
>  
> @@ -1025,11 +1038,11 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, 
> int asid, dma_addr_t iova,
>  IOMMUMemoryRegion *mr = &sdev->iommu;
>  IOMMUNotifier *n;
>  
> -trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, iova,
> -tg, num_pages);
> +trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, vmid,
> +iova, tg, num_pages);
>  
>  IOMMU_NOTIFIER_FOREACH(n, mr) {
> -smmuv3_notify_iova(mr, n, asid, iova, tg, num_pages);
> +smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages);
>  }
>  }
>  }
> @@ -1060,7 +1073,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
>  
>  if (!tg) {
>  trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
> -smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1);
> +smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1);
>  smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
>  return;
>  }
> @@ -1078,7 +1091,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
>  
>  num_pages = (mask + 1) >> granule;
>  trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
> -smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
> +smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages);
>  smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
>  addr += mask + 1;
>  }
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index f8fdf1ca9f..cdc1ea06a8 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -53,5 +53,5 @@ smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
>  smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
>  smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifi

Re: [PATCH] hw/mips/gt64xxx_pci: Don't endian-swap GT_PCI0_CFGADDR

[Nathan Chancellor]

On Wed, Mar 08, 2023 at 12:33:38AM +0100, Philippe Mathieu-Daudé wrote:
> On 23/2/23 17:19, Jiaxun Yang wrote:
> > 145e2198d749 ("hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE
> > MemoryRegionOps") converted CFGADDR/CFGDATA registers to use 
> > PCI_HOST_BRIDGE's
> > accessor facility and enabled byte swap for both CFGADDR/CFGDATA register.
> > 
> > However CFGADDR as a ISD internal register is not controled by MByteSwap
> > bit, it follows endian of all other ISD register, which means it ties to
> > little endian.
> > 
> > Move mapping of CFGADDR out of gt64120_update_pci_cfgdata_mapping to disable
> > endian-swapping.
> > 
> > This should fix some recent reports about poweroff hang.
> > 
> > Fixes: 145e2198d749 ("hw/mips/gt64xxx_pci: Endian-swap using 
> > PCI_HOST_BRIDGE MemoryRegionOps")
> > Signed-off-by: Jiaxun Yang 
> > ---
> >   hw/pci-host/gt64120.c | 18 ++
> >   1 file changed, 6 insertions(+), 12 deletions(-)
> 
> So this works on little-endian hosts, but fails on
> big-endian ones :(
> 
> I.e. on Linux we have early_console_write() -> prom_putchar()
> looping:
> 
> IN: prom_putchar
> 0x8010fab8:  lbu  v0,0(v1)
> 0x8010fabc:  andi v0,v0,0x20
> 0x8010fac0:  beqz v0,0x8010fab8
> 0x8010fac4:  andi v0,a0,0xff
> 
> gt64120: Illegal register read reg:0x3fc size:4 value:0x
> gt64120: Illegal register read reg:0x3fc size:4 value:0x
> gt64120: Illegal register read reg:0x3fc size:4 value:0x
> gt64120: Illegal register read reg:0x3fc size:4 value:0x
> gt64120: Illegal register read reg:0x3fc size:4 value:0x
> ...
> 

Is there going to be a new version of this patch or a different solution
to the poweroff hang then? I am still seeing that with tip of tree QEMU
and I see 8.0.0-rc0 has been tagged; I would hate for this to end up in
a release version.

Cheers,
Nathan

Re: [RFC PATCH v2 08/11] hw/arm/smmuv3: Add CMDs related to stage-2

[Eric Auger]

Hi Mostafa,

On 2/26/23 23:06, Mostafa Saleh wrote:
> CMD_TLBI_S2_IPA: As S1+S2 is not enabled, for now this can be the
> same as CMD_TLBI_NH_VAA.
>
> CMD_TLBI_S12_VMALL: Added new function to invalidate TLB by VMID.
>
> For stage-1 only commands, add a check to to throw CERROR_ILL if used
s/to to/to
> when stage-1 is not supported.
>
> Signed-off-by: Mostafa Saleh 
> ---
> Changes in v2:
> - Add checks for stage-1 only commands
> - Rename smmuv3_s1_range_inval to smmuv3_range_inval
> ---
>  hw/arm/smmu-common.c | 16 
>  hw/arm/smmuv3.c  | 47 +++-
>  hw/arm/trace-events  |  4 ++-
>  include/hw/arm/smmu-common.h |  1 +
>  4 files changed, 61 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 3191a008c6..e4b477af10 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -135,6 +135,16 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, 
> gpointer value,
>  
>  return SMMU_IOTLB_ASID(*iotlb_key) == asid;
>  }
> +
> +static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
> + gpointer user_data)
> +{
> +uint16_t vmid = *(uint16_t *)user_data;
> +SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
> +
> +return SMMU_IOTLB_VMID(*iotlb_key) == vmid;
> +}
> +
>  static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer 
> value,
>gpointer user_data)
>  {
> @@ -187,6 +197,12 @@ void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid)
>  g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
>  }
>  
> +inline void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid)
> +{
> +trace_smmu_iotlb_inv_vmid(vmid);
> +g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
> +}
> +
>  /* VMSAv8-64 Translation */
>  
>  /**
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index f9c06723f9..8c76a48c8d 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1034,7 +1034,7 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int 
> asid, dma_addr_t iova,
>  }
>  }
>  
> -static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
> +static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
>  {
>  dma_addr_t end, addr = CMD_ADDR(cmd);
>  uint8_t type = CMD_TYPE(cmd);
> @@ -1059,7 +1059,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd 
> *cmd)
>  }
>  
>  if (!tg) {
> -trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
> +trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
>  smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1);
>  smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
>  return;
> @@ -1077,7 +1077,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd 
> *cmd)
>  uint64_t mask = dma_aligned_pow2_mask(addr, end, 64);
>  
>  num_pages = (mask + 1) >> granule;
> -trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, num_pages, ttl, 
> leaf);
> +trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
>  smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
>  smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
>  addr += mask + 1;
> @@ -1211,12 +1211,22 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
>  {
>  uint16_t asid = CMD_ASID(&cmd);
>  
> +if (!STAGE1_SUPPORTED(s)) {
> +cmd_error = SMMU_CERROR_ILL;
Well looking further this is not said explicitly this should return

SMMU_CERROR_ILL. Maybe you should mark it as a guest error because we do not 
expect a guest to send such command when S1 is not supported?


> +break;
> +}
> +
>  trace_smmuv3_cmdq_tlbi_nh_asid(asid);
>  smmu_inv_notifiers_all(&s->smmu_state);
>  smmu_iotlb_inv_asid(bs, asid);
>  break;
>  }
>  case SMMU_CMD_TLBI_NH_ALL:
> +if (!STAGE1_SUPPORTED(s)) {
> +cmd_error = SMMU_CERROR_ILL;
> +break;

there is a VMID field. Can't this be used in S2 mode as well?

> +}
> +QEMU_FALLTHROUGH;
>  case SMMU_CMD_TLBI_NSNH_ALL:
>  trace_smmuv3_cmdq_tlbi_nh();
>  smmu_inv_notifiers_all(&s->smmu_state);
> @@ -1224,7 +1234,34 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
>  break;
>  case SMMU_CMD_TLBI_NH_VAA:
>  case SMMU_CMD_TLBI_NH_VA:
> -smmuv3_s1_range_inval(bs, &cmd);
> +if (!STAGE1_SUPPORTED(s)) {
> +cmd_error = SMMU_CERROR_ILL;
> +break;
> +}
> +smmuv3_range_inval(bs, &cmd);
> +break;
> +case SMMU_CMD_TLBI_S12_VMALL:
> +uint16_t vmid = CMD_VMID(&cmd);
> +
> +if (!STAGE2_SUPPORTED(s)) {
> +  

Re: [PATCH 10/10] accel/tcg: remove unused includes

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:

Signed-off-by: Alex Bennée
---
  accel/tcg/cpu-exec.c | 4 
  1 file changed, 4 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH 09/10] accel/tcg: re-inline the filtering of virtual IRQs but data driven

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:

Although only I386 currently uses it it is not inconceivable that
other arches might find this facility useful.

Signed-off-by: Alex Bennée
---
  include/hw/core/tcg-cpu-ops.h |  5 +
  accel/tcg/cpu-exec.c  | 29 +
  target/i386/tcg/tcg-cpu.c |  1 +
  3 files changed, 15 insertions(+), 20 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH 08/10] accel/tcg: push i386 specific hacks into handle_cpu_interrupt callback

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:

Signed-off-by: Alex Bennée 
---
  include/hw/core/sysemu-cpu-ops.h | 11 +++
  target/i386/cpu-internal.h   |  1 +
  accel/tcg/cpu-exec-softmmu.c | 16 
  accel/tcg/cpu-exec.c | 31 ++-
  target/i386/cpu-sysemu.c | 17 +
  target/i386/cpu.c|  1 +
  6 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
index c9d30172c4..d53907b517 100644
--- a/include/hw/core/sysemu-cpu-ops.h
+++ b/include/hw/core/sysemu-cpu-ops.h
@@ -53,6 +53,15 @@ typedef struct SysemuCPUOps {
   * @cs: The CPUState
   */
  void (*handle_cpu_halt)(CPUState *cpu);
+/**
+ * @handle_cpu_interrupt: handle init/reset interrupts
+ * @cs: The CPUState
+ * @irq_request: the interrupt request
+ *
+ * Most architectures share a common handler. Returns true if the
+ * handler did indeed handle and interrupt.
+ */


and -> the? or any?

This should be a tcg hook, not a sysemu hook, per the previous one.
I would very much like it to never be NULL, but instead your new 
common_cpu_handle_interrupt function.



-#if defined(TARGET_I386)
-else if (interrupt_request & CPU_INTERRUPT_INIT) {
-X86CPU *x86_cpu = X86_CPU(cpu);
-CPUArchState *env = &x86_cpu->env;
-replay_interrupt();
-cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
-do_cpu_init(x86_cpu);
-cpu->exception_index = EXCP_HALTED;
-return true;
-}
-#else
-else if (interrupt_request & CPU_INTERRUPT_RESET) {
-replay_interrupt();
-cpu_reset(cpu);
+else if (cpu->cc->sysemu_ops->handle_cpu_interrupt &&
+ cpu->cc->sysemu_ops->handle_cpu_interrupt(cpu, 
interrupt_request)) {
+return true;
+} else if (common_cpu_handle_interrupt(cpu, interrupt_request)) {
  return true;


... because this is pretty ugly, and incorrectly indented.


r~

Re: [PATCH 07/10] accel/tcg: use QEMU_IOTHREAD_LOCK_GUARD to cover the exit

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:

This avoids us having to make sure each exit path does an unlock.

Signed-off-by: Alex Bennée
---
  accel/tcg/cpu-exec.c | 12 +++-
  1 file changed, 3 insertions(+), 9 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Claudio Imbrenda]

On Mon, 20 Mar 2023 17:05:07 +0100
Markus Armbruster  wrote:

> Thomas Huth  writes:
> 
> > On 20/03/2023 16.31, Markus Armbruster wrote:  
> >> Claudio Imbrenda  writes:
> >>   
> >>> The recently introduced -async-teardown commandline option was not
> >>> wired up properly and did not show up in the output of the QMP command
> >>> query-command-line-options. This means that libvirt will have no way to
> >>> discover whether the feature is supported.  
> >> 
> >> There was nothing improper in its wiring.  The issue is that
> >> query-command-line-options is junk.  See my recent post
> >> 
> >>  Subject: query-command-line-options (was: [PATCH 1/7] qemu: 
> >> capabilities: Introduce QEMU_CAPS_MACHINE_ACPI)
> >>  Date: Tue, 07 Mar 2023 10:40:23 +0100
> >>  Message-ID: <87jzzsc320.fsf...@pond.sub.org>
> >>   
> >>> This patch fixes the issue by correctly wiring up the commandline
> >>> option so that it appears in the output of query-command-line-options.
> >>>
> >>> Reported-by: Boris Fiuczynski 
> >>> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on 
> >>> Linux")
> >>> Signed-off-by: Claudio Imbrenda 
> >>> ---
> >>>   util/async-teardown.c | 17 +
> >>>   1 file changed, 17 insertions(+)
> >>>
> >>> diff --git a/util/async-teardown.c b/util/async-teardown.c
> >>> index 62cdeb0f20..c9b9a3cdb2 100644
> >>> --- a/util/async-teardown.c
> >>> +++ b/util/async-teardown.c
> >>> @@ -12,6 +12,9 @@
> >>>*/
> >>>   
> >>>   #include "qemu/osdep.h"
> >>> +#include "qemu/config-file.h"
> >>> +#include "qemu/option.h"
> >>> +#include "qemu/module.h"
> >>>   #include 
> >>>   #include 
> >>>   #include 
> >>> @@ -144,3 +147,17 @@ void init_async_teardown(void)
> >>>   clone(async_teardown_fn, new_stack_for_clone(), CLONE_VM, NULL);
> >>>   sigprocmask(SIG_SETMASK, &old_signals, NULL);
> >>>   }
> >>> +
> >>> +static QemuOptsList qemu_async_teardown_opts = {
> >>> +.name = "async-teardown",
> >>> +.head = QTAILQ_HEAD_INITIALIZER(qemu_async_teardown_opts.head),
> >>> +.desc = {
> >>> +{ /* end of list */ }
> >>> +},
> >>> +};
> >>> +
> >>> +static void register_async_teardown(void)
> >>> +{
> >>> +qemu_add_opts(&qemu_async_teardown_opts);
> >>> +}
> >>> +opts_init(register_async_teardown);  
> >> 
> >> Now it *is* improperly wired up :)
> >> 
> >> You're defining new QemuOpts config group "async-teardown" with
> >> arbitrary option parameters, but don't actually use it for parsing or
> >> recording the option.  I figure because you can't: there is no option
> >> argument to parse and record, which is what QemuOpts is designed to do.
> >> 
> >> If you need the feature to be visible in query-command-line-options, you
> >> should make it an option parameter (a KEY, not a GROUP), preferably of
> >> an existing group / option.  
> >
> > Would it make sense to add it e.g. to "-action" instead, i.e. something 
> > like 
> > "-action teardown=async" ?  
> 
> I believe the new parameter "teardown" would be visible in
> query-command-line-options.
> 
> How well does it fit -action?

I guess it can be shoehorned in. generally action is about stuff that
happens in/to the guest, while in this case it's about how qemu will
perform the teardown of its address space once it terminates.

the important parts are: this is an OS-specific option (Linux), and it
needs to be parsed and enabled before sandboxing (otherwise clone(2)
might not work)

Re: [PATCH 06/10] includes: move irq definitions out of cpu-all.h

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:

These are common across all versions of the system so it would help if
we could use them for common code.

Signed-off-by: Alex Bennée
---
  include/exec/cpu-all.h | 52 +-
  include/exec/cpu-irq.h | 83 ++
  include/exec/poison.h  | 13 ---
  3 files changed, 84 insertions(+), 64 deletions(-)
  create mode 100644 include/exec/cpu-irq.h


Reviewed-by: Richard Henderson 

r~

[PATCH v14 1/4] vhost: expose function vhost_dev_has_iommu()

[Cindy Lu]

To support vIOMMU in vdpa, need to exposed the function
vhost_dev_has_iommu, vdpa will use this function to check
if vIOMMU enable.

Signed-off-by: Cindy Lu 
---
 hw/virtio/vhost.c | 2 +-
 include/hw/virtio/vhost.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index a266396576..fd746b085b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -107,7 +107,7 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
 }
 }
 
-static bool vhost_dev_has_iommu(struct vhost_dev *dev)
+bool vhost_dev_has_iommu(struct vhost_dev *dev)
 {
 VirtIODevice *vdev = dev->vdev;
 
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a52f273347..f7f10c8fb7 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -336,4 +336,5 @@ int vhost_dev_set_inflight(struct vhost_dev *dev,
struct vhost_inflight *inflight);
 int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
struct vhost_inflight *inflight);
+bool vhost_dev_has_iommu(struct vhost_dev *dev);
 #endif
-- 
2.34.3

[PATCH v14 2/4] vhost_vdpa: fix the input in trace_vhost_vdpa_listener_region_del()

[Cindy Lu]

In trace_vhost_vdpa_listener_region_del, the value for llend
should change to int128_get64(int128_sub(llend, int128_one()))

Signed-off-by: Cindy Lu 
---
 hw/virtio/vhost-vdpa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index bc6bad23d5..92c2413c76 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -288,7 +288,8 @@ static void vhost_vdpa_listener_region_del(MemoryListener 
*listener,
 iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
 llend = vhost_vdpa_section_end(section);
 
-trace_vhost_vdpa_listener_region_del(v, iova, int128_get64(llend));
+trace_vhost_vdpa_listener_region_del(v, iova,
+int128_get64(int128_sub(llend, int128_one(;
 
 if (int128_ge(int128_make64(iova), llend)) {
 return;
-- 
2.34.3

[PATCH v14 4/4] vhost-vdpa: Add support for vIOMMU.

[Cindy Lu]

1. The vIOMMU support will make vDPA can work in IOMMU mode. This
will fix security issues while using the no-IOMMU mode.
To support this feature we need to add new functions for IOMMU MR adds and
deletes.

Also since the SVQ does not support vIOMMU yet, add the check for IOMMU
in vhost_vdpa_dev_start, if the SVQ and IOMMU enable at the same time
the function will return fail.

2. Skip the iova_max check vhost_vdpa_listener_skipped_section(). While
MR is IOMMU, move this check to  vhost_vdpa_iommu_map_notify()

Verified in vp_vdpa and vdpa_sim_net driver

Signed-off-by: Cindy Lu 
---
 hw/virtio/vhost-vdpa.c | 149 +++--
 include/hw/virtio/vhost-vdpa.h |  11 +++
 2 files changed, 152 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 0c8c37e786..b36922b365 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -26,6 +26,7 @@
 #include "cpu.h"
 #include "trace.h"
 #include "qapi/error.h"
+#include "hw/virtio/virtio-access.h"
 
 /*
  * Return one past the end of the end of section. Be careful with uint64_t
@@ -60,15 +61,22 @@ static bool 
vhost_vdpa_listener_skipped_section(MemoryRegionSection *section,
  iova_min, section->offset_within_address_space);
 return true;
 }
+/*
+ * While using vIOMMU, sometimes the section will be larger than iova_max,
+ * but the memory that actually maps is smaller, so move the check to
+ * function vhost_vdpa_iommu_map_notify(). That function will use the 
actual
+ * size that maps to the kernel
+ */
 
-llend = vhost_vdpa_section_end(section);
-if (int128_gt(llend, int128_make64(iova_max))) {
-error_report("RAM section out of device range (max=0x%" PRIx64
- ", end addr=0x%" PRIx64 ")",
- iova_max, int128_get64(llend));
-return true;
+if (!memory_region_is_iommu(section->mr)) {
+llend = vhost_vdpa_section_end(section);
+if (int128_gt(llend, int128_make64(iova_max))) {
+error_report("RAM section out of device range (max=0x%" PRIx64
+ ", end addr=0x%" PRIx64 ")",
+ iova_max, int128_get64(llend));
+return true;
+}
 }
-
 return false;
 }
 
@@ -185,6 +193,118 @@ static void vhost_vdpa_listener_commit(MemoryListener 
*listener)
 v->iotlb_batch_begin_sent = false;
 }
 
+static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
+{
+struct vdpa_iommu *iommu = container_of(n, struct vdpa_iommu, n);
+
+hwaddr iova = iotlb->iova + iommu->iommu_offset;
+struct vhost_vdpa *v = iommu->dev;
+void *vaddr;
+int ret;
+Int128 llend;
+
+if (iotlb->target_as != &address_space_memory) {
+error_report("Wrong target AS \"%s\", only system memory is allowed",
+ iotlb->target_as->name ? iotlb->target_as->name : "none");
+return;
+}
+RCU_READ_LOCK_GUARD();
+/* check if RAM section out of device range */
+llend = int128_add(int128_makes64(iotlb->addr_mask), int128_makes64(iova));
+if (int128_gt(llend, int128_make64(v->iova_range.last))) {
+error_report("RAM section out of device range (max=0x%" PRIx64
+ ", end addr=0x%" PRIx64 ")",
+ v->iova_range.last, int128_get64(llend));
+return;
+}
+
+vhost_vdpa_iotlb_batch_begin_once(v);
+
+if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
+bool read_only;
+
+if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL)) {
+return;
+}
+
+ret = vhost_vdpa_dma_map(v, VHOST_VDPA_GUEST_PA_ASID, iova,
+ iotlb->addr_mask + 1, vaddr, read_only);
+if (ret) {
+error_report("vhost_vdpa_dma_map(%p, 0x%" HWADDR_PRIx ", "
+ "0x%" HWADDR_PRIx ", %p) = %d (%m)",
+ v, iova, iotlb->addr_mask + 1, vaddr, ret);
+}
+} else {
+ret = vhost_vdpa_dma_unmap(v, VHOST_VDPA_GUEST_PA_ASID, iova,
+   iotlb->addr_mask + 1);
+if (ret) {
+error_report("vhost_vdpa_dma_unmap(%p, 0x%" HWADDR_PRIx ", "
+ "0x%" HWADDR_PRIx ") = %d (%m)",
+ v, iova, iotlb->addr_mask + 1, ret);
+}
+}
+}
+
+static void vhost_vdpa_iommu_region_add(MemoryListener *listener,
+MemoryRegionSection *section)
+{
+struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener);
+
+struct vdpa_iommu *iommu;
+Int128 end;
+int iommu_idx;
+IOMMUMemoryRegion *iommu_mr;
+int ret;
+
+iommu_mr = IOMMU_MEMORY_REGION(section->mr);
+
+iommu = g_malloc0(sizeof(*iommu));
+end = int128_add(int128_make64(section->offset_within_region),
+ section->size);
+end = int128_sub(end, 

[PATCH v14 0/4] vhost-vdpa: add support for vIOMMU

[Cindy Lu]

These patches are to support vIOMMU in vdpa device

changes in V3
1. Move function vfio_get_xlat_addr to memory.c
2. Use the existing memory listener, while the MR is
iommu MR then call the function iommu_region_add/
iommu_region_del

changes in V4
1.make the comments in vfio_get_xlat_addr more general

changes in V5
1. Address the comments in the last version
2. Add a new arg in the function vfio_get_xlat_addr, which shows whether
the memory is backed by a discard manager. So the device can have its
own warning.

changes in V6
move the error_report for the unpopulated discard back to
memeory_get_xlat_addr

changes in V7
organize the error massage to avoid the duplicate information

changes in V8
Organize the code follow the comments in the last version

changes in V9
Organize the code follow the comments

changes in V10
Address the comments

changes in V11
Address the comments
fix the crash found in test

changes in V12
Address the comments, squash patch 1 into the next patch
improve the code style issue

changes in V13
fail to start if IOMMU and svq enable at same time
improve the code style issue

changes in V14
Address the comments

Cindy Lu (4):
  vhost: expose function vhost_dev_has_iommu()
  vhost_vdpa: fix the input in trace_vhost_vdpa_listener_region_del()
  vhost-vdpa: Add check for full 64-bit in region delete
  vhost-vdpa: Add support for vIOMMU.

 hw/virtio/vhost-vdpa.c | 172 +++--
 hw/virtio/vhost.c  |   2 +-
 include/hw/virtio/vhost-vdpa.h |  11 +++
 include/hw/virtio/vhost.h  |   1 +
 4 files changed, 175 insertions(+), 11 deletions(-)

-- 
2.34.3

[PATCH v14 3/4] vhost-vdpa: Add check for full 64-bit in region delete

[Cindy Lu]

The unmap ioctl doesn't accept a full 64-bit span. So need to
add check for the section's size in vhost_vdpa_listener_region_del().

Signed-off-by: Cindy Lu 
---
 hw/virtio/vhost-vdpa.c | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 92c2413c76..0c8c37e786 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -316,10 +316,28 @@ static void vhost_vdpa_listener_region_del(MemoryListener 
*listener,
 vhost_iova_tree_remove(v->iova_tree, *result);
 }
 vhost_vdpa_iotlb_batch_begin_once(v);
+/*
+ * The unmap ioctl doesn't accept a full 64-bit. need to check it
+ */
+if (int128_eq(llsize, int128_2_64())) {
+llsize = int128_rshift(llsize, 1);
+ret = vhost_vdpa_dma_unmap(v, VHOST_VDPA_GUEST_PA_ASID, iova,
+   int128_get64(llsize));
+
+if (ret) {
+error_report("vhost_vdpa_dma_unmap(%p, 0x%" HWADDR_PRIx ", "
+ "0x%" HWADDR_PRIx ") = %d (%m)",
+ v, iova, int128_get64(llsize), ret);
+}
+iova += int128_get64(llsize);
+}
 ret = vhost_vdpa_dma_unmap(v, VHOST_VDPA_GUEST_PA_ASID, iova,
int128_get64(llsize));
+
 if (ret) {
-error_report("vhost_vdpa dma unmap error!");
+error_report("vhost_vdpa_dma_unmap(%p, 0x%" HWADDR_PRIx ", "
+ "0x%" HWADDR_PRIx ") = %d (%m)",
+ v, iova, int128_get64(llsize), ret);
 }
 
 memory_region_unref(section->mr);
-- 
2.34.3

Re: [PATCH 05/10] accel/tcg: remove the fake_user_interrupt guards

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:

At the cost of an empty tcg_ops field for most targets we can avoid
target specific hacks in cpu-exec.c

Signed-off-by: Alex Bennée
---
  include/hw/core/tcg-cpu-ops.h |  2 +-
  accel/tcg/cpu-exec.c  | 14 +++---
  2 files changed, 8 insertions(+), 8 deletions(-)


Reviewed-by: Richard Henderson 


r~

Re: [PATCH 04/10] accel/tcg: don't bother with ifdef for CPU_DUMP_CCOP

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:

+int flags = CPU_DUMP_CCOP;;


Actually, since you can't turn it off, we should just remove it and the test in 
i386.


r~

Re: [RFC PATCH v2 07/11] hw/arm/smmuv3: Add VMID to tlb tagging

[Eric Auger]

Hi Mostafa,

On 2/26/23 23:06, Mostafa Saleh wrote:
> Allow TLB to be tagged with VMID.

s/tlb/TLB in the commit msg.
>
> If stage-1 is only supported, VMID is set to -1 and ignored from STE
> and CMD_TLBI_NH* cmds.
>
> Update smmu_iotlb_insert trace event to have vmid.
>
> Signed-off-by: Mostafa Saleh 
> ---
> Changes in v2:
> -Fix TLB aliasing issue from missing check in smmu_iotlb_key_equal.
> -Add vmid to traces smmu_iotlb_insert and smmu_iotlb_lookup_hit/miss.
> -Add vmid to hash function.
> ---
>  hw/arm/smmu-common.c | 36 ++--
>  hw/arm/smmu-internal.h   |  2 ++
>  hw/arm/smmuv3.c  | 12 +---
>  hw/arm/trace-events  |  6 +++---
>  include/hw/arm/smmu-common.h |  5 +++--
>  5 files changed, 39 insertions(+), 22 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 3f448bc82e..3191a008c6 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -38,7 +38,7 @@ static guint smmu_iotlb_key_hash(gconstpointer v)
>  
>  /* Jenkins hash */
>  a = b = c = JHASH_INITVAL + sizeof(*key);
> -a += key->asid + key->level + key->tg;
> +a += key->asid + key->vmid + key->level + key->tg;
>  b += extract64(key->iova, 0, 32);
>  c += extract64(key->iova, 32, 32);
>  
> @@ -53,13 +53,15 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, 
> gconstpointer v2)
>  SMMUIOTLBKey *k1 = (SMMUIOTLBKey *)v1, *k2 = (SMMUIOTLBKey *)v2;
>  
>  return (k1->asid == k2->asid) && (k1->iova == k2->iova) &&
> -   (k1->level == k2->level) && (k1->tg == k2->tg);
> +   (k1->level == k2->level) && (k1->tg == k2->tg) &&
> +   (k1->vmid == k2->vmid);
>  }
>  
> -SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova,
> +SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
>  uint8_t tg, uint8_t level)
>  {
> -SMMUIOTLBKey key = {.asid = asid, .iova = iova, .tg = tg, .level = 
> level};
> +SMMUIOTLBKey key = {.asid = asid, .vmid = vmid, .iova = iova,
> +.tg = tg, .level = level};
>  
>  return key;
>  }
> @@ -78,7 +80,8 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg 
> *cfg,
>  uint64_t mask = subpage_size - 1;
>  SMMUIOTLBKey key;
>  
> -key = smmu_get_iotlb_key(cfg->asid, iova & ~mask, tg, level);
> +key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid,
> + iova & ~mask, tg, level);
>  entry = g_hash_table_lookup(bs->iotlb, &key);
>  if (entry) {
>  break;
> @@ -88,13 +91,13 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, 
> SMMUTransCfg *cfg,
>  
>  if (entry) {
>  cfg->iotlb_hits++;
> -trace_smmu_iotlb_lookup_hit(cfg->asid, iova,
> +trace_smmu_iotlb_lookup_hit(cfg->asid, cfg->s2cfg.vmid, iova,
>  cfg->iotlb_hits, cfg->iotlb_misses,
>  100 * cfg->iotlb_hits /
>  (cfg->iotlb_hits + cfg->iotlb_misses));
>  } else {
>  cfg->iotlb_misses++;
> -trace_smmu_iotlb_lookup_miss(cfg->asid, iova,
> +trace_smmu_iotlb_lookup_miss(cfg->asid, cfg->s2cfg.vmid, iova,
>   cfg->iotlb_hits, cfg->iotlb_misses,
>   100 * cfg->iotlb_hits /
>   (cfg->iotlb_hits + cfg->iotlb_misses));
> @@ -111,8 +114,10 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, 
> SMMUTLBEntry *new)
>  smmu_iotlb_inv_all(bs);
>  }
>  
> -*key = smmu_get_iotlb_key(cfg->asid, new->entry.iova, tg, new->level);
> -trace_smmu_iotlb_insert(cfg->asid, new->entry.iova, tg, new->level);
> +*key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
> +  tg, new->level);
> +trace_smmu_iotlb_insert(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
> +tg, new->level);
>  g_hash_table_insert(bs->iotlb, key, new);
>  }
>  
> @@ -130,8 +135,7 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, 
> gpointer value,
>  
>  return SMMU_IOTLB_ASID(*iotlb_key) == asid;
>  }
> -
> -static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value,
> +static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer 
> value,
>gpointer user_data)
>  {
>  SMMUTLBEntry *iter = (SMMUTLBEntry *)value;
> @@ -142,18 +146,21 @@ static gboolean smmu_hash_remove_by_asid_iova(gpointer 
> key, gpointer value,
>  if (info->asid >= 0 && info->asid != SMMU_IOTLB_ASID(iotlb_key)) {
>  return false;
>  }
> +if (info->vmid >= 0 && info->vmid != SMMU_IOTLB_VMID(iotlb_key)) {
> +return false;
> +}
>  return ((info->iova & ~entry->addr_mask) == entry->iova) ||
> ((entr

Re: [PATCH 04/10] accel/tcg: don't bother with ifdef for CPU_DUMP_CCOP

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:

+int flags = CPU_DUMP_CCOP;;


two ;

Reviewed-by: Richard Henderson 


r~

Re: [PATCH 02/10] accel/tcg: move cpu_reloading_memory_map into cpu-exec-softmmu

[Richard Henderson]

On 3/20/23 07:33, Alex Bennée wrote:


Claudio Fontana  writes:


On 3/20/23 14:32, Alex Bennée wrote:


Claudio Fontana  writes:


How is this conditional on CONFIG_TCG? To me it looks like this breaks 
!CONFIG_TCG.
Careful, the meson.build in accel/tcg/meson.build is always recursed.


Surely it shouldn't be in accel/tcg then?



Hi Alex,

maybe we did not understand each other.

What I mean is that accel/tcg/meson.build is not conditionally read, it is 
_always_ read.

Therefore TCG-specific code needs to be conditionally included using
the CONFIG_TCG.


Ahh I see now, right I can fix that up next revision if there is
interest in this approach.


Yes, that seems fine.  With either when: or subdir_done(),

Reviewed-by: Richard Henderson 


r~

[PATCH 2/2] ui/gtk-egl: fix scaling for cursor position in scanout mode

[Erico Nunes]

vc->gfx.w and vc->gfx.h are not updated appropriately in this code path,
which leads to a different scaling factor for rendering the cursor on
some edge cases (e.g. the focus has left and re-entered the gtk window).
This can be reproduced using vhost-user-gpu with the gtk ui on the x11
backend.
Use the surface dimensions which are already updated accordingly.

Signed-off-by: Erico Nunes 
---
 ui/gtk-egl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index e84431790c..a4422be837 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -88,8 +88,8 @@ void gd_egl_draw(VirtualConsole *vc)
 #endif
 gd_egl_scanout_flush(&vc->gfx.dcl, 0, 0, vc->gfx.w, vc->gfx.h);
 
-vc->gfx.scale_x = (double)ww / vc->gfx.w;
-vc->gfx.scale_y = (double)wh / vc->gfx.h;
+vc->gfx.scale_x = (double)ww / surface_width(vc->gfx.ds);
+vc->gfx.scale_y = (double)wh / surface_height(vc->gfx.ds);
 
 glFlush();
 #ifdef CONFIG_GBM
-- 
2.39.2

[PATCH 1/2] ui/gtk: use widget size for cursor motion event

[Erico Nunes]

The gd_motion_event size has some calculations for the cursor position,
which also take into account things like different size of the
framebuffer compared to the window size.
The use of window size makes things more difficult though, as at least
in the case of Wayland includes the size of ui elements like a menu bar
at the top of the window. This leads to a wrong position calculation by
a few pixels.
Fix it by using the size of the widget, which already returns the size
of the actual space to render the framebuffer.

Signed-off-by: Erico Nunes 
---
 ui/gtk.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index fd82e9b1ca..d1b2a80c2b 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -868,7 +868,6 @@ static gboolean gd_motion_event(GtkWidget *widget, 
GdkEventMotion *motion,
 {
 VirtualConsole *vc = opaque;
 GtkDisplayState *s = vc->s;
-GdkWindow *window;
 int x, y;
 int mx, my;
 int fbh, fbw;
@@ -881,10 +880,9 @@ static gboolean gd_motion_event(GtkWidget *widget, 
GdkEventMotion *motion,
 fbw = surface_width(vc->gfx.ds) * vc->gfx.scale_x;
 fbh = surface_height(vc->gfx.ds) * vc->gfx.scale_y;
 
-window = gtk_widget_get_window(vc->gfx.drawing_area);
-ww = gdk_window_get_width(window);
-wh = gdk_window_get_height(window);
-ws = gdk_window_get_scale_factor(window);
+ww = gtk_widget_get_allocated_width(widget);
+wh = gtk_widget_get_allocated_height(widget);
+ws = gtk_widget_get_scale_factor(widget);
 
 mx = my = 0;
 if (ww > fbw) {
-- 
2.39.2

Re: [RFC PATCH v2 06/11] hw/arm/smmuv3: Make TLB lookup work for stage-2

[Eric Auger]

Hi Mostafa,

On 2/26/23 23:06, Mostafa Saleh wrote:
> Right now, either stage-1 or stage-2 are supported, this simplifies
> how we can deal with TLBs.
> This patch makes TLB lookup work if stage-2 is enabled instead of
> stage-1.
> TLB lookup is done before a PTW, if a valid entry is found we won't
> do the PTW.
> To be able to do TLB lookup, we need the correct tagging info, as
> granularity and input size, so we get this based on the supported
> translation stage. The TLB entries are added correctly from each
> stage PTW.
>
> When nested translation is supported, this would need to change, for
> example if we go with a combined TLB implementation, we would need to
> use the min of the granularities in TLB.
>
> As stage-2 shouldn't be tagged by ASID, it will be set to -1 if S1P
> is not enabled.
>
> Signed-off-by: Mostafa Saleh 
> ---
> Changes in v2:
> - check if S1 is enabled(not supported) when reading S1 TT.
> ---
>  hw/arm/smmuv3.c | 45 ++---
>  1 file changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index dc74a5442d..ce193e9598 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -697,6 +697,9 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, 
> SMMUTransCfg *cfg,
>  STE ste;
>  CD cd;
>  
> +/* ASID defaults to -1 (if s1 is not supported). */
> +cfg->asid = -1;
> +
>  ret = smmu_find_ste(s, sid, &ste, event);
>  if (ret) {
>  return ret;
> @@ -787,6 +790,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion 
> *mr, hwaddr addr,
>  SMMUTLBEntry *cached_entry = NULL;
>  SMMUTransTableInfo *tt;
>  SMMUTransCfg *cfg = NULL;
> +uint8_t granule_sz, tsz;
>  IOMMUTLBEntry entry = {
>  .target_as = &address_space_memory,
>  .iova = addr,
> @@ -822,21 +826,40 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion 
> *mr, hwaddr addr,
>  goto epilogue;
>  }
>  
> -tt = select_tt(cfg, addr);
> -if (!tt) {
> -if (cfg->record_faults) {
> -event.type = SMMU_EVT_F_TRANSLATION;
> -event.u.f_translation.addr = addr;
> -event.u.f_translation.rnw = flag & 0x1;
> +if (cfg->stage == 1) {
> +/* Select stage1 translation table. */
> +tt = select_tt(cfg, addr);
> +if (!tt) {
> +if (cfg->record_faults) {
> +event.type = SMMU_EVT_F_TRANSLATION;
> +event.u.f_translation.addr = addr;
> +event.u.f_translation.rnw = flag & 0x1;
> +}
> +status = SMMU_TRANS_ERROR;
> +goto epilogue;
>  }
> -status = SMMU_TRANS_ERROR;
> -goto epilogue;
> -}
> +granule_sz = tt->granule_sz;
> +tsz = tt->tsz;
>  
> -page_mask = (1ULL << (tt->granule_sz)) - 1;
> +} else {
> +/* Stage2. */
> +granule_sz = cfg->s2cfg.granule_sz;
> +tsz = cfg->s2cfg.tsz;
> +}
> +/*
> + * TLB lookup looks for granule and input size for a translation stage,
> + * as only one stage is supported right now, choose the right values
> + * from the configuration.
> + */
> +page_mask = (1ULL << granule_sz) - 1;
>  aligned_addr = addr & ~page_mask;
>  
> -cached_entry = smmu_iotlb_lookup(bs, cfg, tt, aligned_addr);
> +SMMUTransTableInfo temp = {
Move the declaration at the top. Also rename temp into tt to be more
explicit about what it is?
> +.granule_sz = granule_sz,
> +.tsz = tsz,
> +};
> +
> +cached_entry = smmu_iotlb_lookup(bs, cfg, &temp, aligned_addr);
>  if (cached_entry) {
>  if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
>  status = SMMU_TRANS_ERROR;
Besides, looks good to me

Reviewed-by: Eric Auger 

Thanks

Eric

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Markus Armbruster]

Thomas Huth  writes:

> On 20/03/2023 16.31, Markus Armbruster wrote:
>> Claudio Imbrenda  writes:
>> 
>>> The recently introduced -async-teardown commandline option was not
>>> wired up properly and did not show up in the output of the QMP command
>>> query-command-line-options. This means that libvirt will have no way to
>>> discover whether the feature is supported.
>> 
>> There was nothing improper in its wiring.  The issue is that
>> query-command-line-options is junk.  See my recent post
>> 
>>  Subject: query-command-line-options (was: [PATCH 1/7] qemu: 
>> capabilities: Introduce QEMU_CAPS_MACHINE_ACPI)
>>  Date: Tue, 07 Mar 2023 10:40:23 +0100
>>  Message-ID: <87jzzsc320.fsf...@pond.sub.org>
>> 
>>> This patch fixes the issue by correctly wiring up the commandline
>>> option so that it appears in the output of query-command-line-options.
>>>
>>> Reported-by: Boris Fiuczynski 
>>> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux")
>>> Signed-off-by: Claudio Imbrenda 
>>> ---
>>>   util/async-teardown.c | 17 +
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/util/async-teardown.c b/util/async-teardown.c
>>> index 62cdeb0f20..c9b9a3cdb2 100644
>>> --- a/util/async-teardown.c
>>> +++ b/util/async-teardown.c
>>> @@ -12,6 +12,9 @@
>>>*/
>>>   
>>>   #include "qemu/osdep.h"
>>> +#include "qemu/config-file.h"
>>> +#include "qemu/option.h"
>>> +#include "qemu/module.h"
>>>   #include 
>>>   #include 
>>>   #include 
>>> @@ -144,3 +147,17 @@ void init_async_teardown(void)
>>>   clone(async_teardown_fn, new_stack_for_clone(), CLONE_VM, NULL);
>>>   sigprocmask(SIG_SETMASK, &old_signals, NULL);
>>>   }
>>> +
>>> +static QemuOptsList qemu_async_teardown_opts = {
>>> +.name = "async-teardown",
>>> +.head = QTAILQ_HEAD_INITIALIZER(qemu_async_teardown_opts.head),
>>> +.desc = {
>>> +{ /* end of list */ }
>>> +},
>>> +};
>>> +
>>> +static void register_async_teardown(void)
>>> +{
>>> +qemu_add_opts(&qemu_async_teardown_opts);
>>> +}
>>> +opts_init(register_async_teardown);
>> 
>> Now it *is* improperly wired up :)
>> 
>> You're defining new QemuOpts config group "async-teardown" with
>> arbitrary option parameters, but don't actually use it for parsing or
>> recording the option.  I figure because you can't: there is no option
>> argument to parse and record, which is what QemuOpts is designed to do.
>> 
>> If you need the feature to be visible in query-command-line-options, you
>> should make it an option parameter (a KEY, not a GROUP), preferably of
>> an existing group / option.
>
> Would it make sense to add it e.g. to "-action" instead, i.e. something like 
> "-action teardown=async" ?

I believe the new parameter "teardown" would be visible in
query-command-line-options.

How well does it fit -action?

Re: [PATCH-for-8.1] block/dmg: Declare a type definition for DMG uncompress function

[Philippe Mathieu-Daudé]

On 20/3/23 16:56, Philippe Mathieu-Daudé wrote:

On 20/3/23 16:26, Philippe Mathieu-Daudé wrote:

Introduce the BdrvDmgUncompressFunc type defintion. To emphasis
dmg_uncompress_bz2 and dmg_uncompress_lzfse are pointer to functions,
declare them using this new typedef.

Signed-off-by: Philippe Mathieu-Daudé 
---
  block/dmg.h | 8 
  block/dmg.c | 7 ++-
  2 files changed, 6 insertions(+), 9 deletions(-)




diff --git a/block/dmg.c b/block/dmg.c
index e10b9a2ba5..2769900359 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -31,11 +31,8 @@
  #include "qemu/memalign.h"
  #include "dmg.h"
-int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in,
-  char *next_out, unsigned int avail_out);
-
-int (*dmg_uncompress_lzfse)(char *next_in, unsigned int avail_in,
-    char *next_out, unsigned int avail_out);
+BdrvDmgUncompressFunc *dmg_uncompress_bz2;
+BdrvDmgUncompressFunc *dmg_uncompress_lzfse;


Unrelated, but since DMG maintainers are Cc'ed, upstream lzfse warning:

In file included from ../../block/dmg-lzfse.c:26:
/opt/homebrew/Cellar/lzfse/1.0/include/lzfse.h:56:43: warning: this 
function declaration is not a prototype [-Wstrict-prototypes]

LZFSE_API size_t lzfse_encode_scratch_size();
   ^
    void


Reported in 2016:
https://github.com/lzfse/lzfse/issues/3#issuecomment-226574719


Unfortunately last upstream activity was 6 years ago...
https://github.com/lzfse/lzfse/pulls

Re: [PATCH-for-8.1] block/dmg: Declare a type definition for DMG uncompress function

[Philippe Mathieu-Daudé]

On 20/3/23 16:26, Philippe Mathieu-Daudé wrote:

Introduce the BdrvDmgUncompressFunc type defintion. To emphasis


Typo "definition".


dmg_uncompress_bz2 and dmg_uncompress_lzfse are pointer to functions,
declare them using this new typedef.

Signed-off-by: Philippe Mathieu-Daudé 
---
  block/dmg.h | 8 
  block/dmg.c | 7 ++-
  2 files changed, 6 insertions(+), 9 deletions(-)

Re: [PATCH-for-8.1] block/dmg: Declare a type definition for DMG uncompress function

[Philippe Mathieu-Daudé]

On 20/3/23 16:26, Philippe Mathieu-Daudé wrote:

Introduce the BdrvDmgUncompressFunc type defintion. To emphasis
dmg_uncompress_bz2 and dmg_uncompress_lzfse are pointer to functions,
declare them using this new typedef.

Signed-off-by: Philippe Mathieu-Daudé 
---
  block/dmg.h | 8 
  block/dmg.c | 7 ++-
  2 files changed, 6 insertions(+), 9 deletions(-)




diff --git a/block/dmg.c b/block/dmg.c
index e10b9a2ba5..2769900359 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -31,11 +31,8 @@
  #include "qemu/memalign.h"
  #include "dmg.h"
  
-int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in,

-  char *next_out, unsigned int avail_out);
-
-int (*dmg_uncompress_lzfse)(char *next_in, unsigned int avail_in,
-char *next_out, unsigned int avail_out);
+BdrvDmgUncompressFunc *dmg_uncompress_bz2;
+BdrvDmgUncompressFunc *dmg_uncompress_lzfse;


Unrelated, but since DMG maintainers are Cc'ed, upstream lzfse warning:

In file included from ../../block/dmg-lzfse.c:26:
/opt/homebrew/Cellar/lzfse/1.0/include/lzfse.h:56:43: warning: this 
function declaration is not a prototype [-Wstrict-prototypes]

LZFSE_API size_t lzfse_encode_scratch_size();
  ^
   void

Unfortunately last upstream activity was 6 years ago...
https://github.com/lzfse/lzfse/pulls

Re: KVM call for agenda for 2023-03-21

[Philippe Mathieu-Daudé]

Hi Juan,

On 18/3/23 18:59, Juan Quintela wrote:


Hi

NOTE, NOTE, NOTE

Remember that we are back in that crazy part of the year when daylight
saving applies.  Call is done on US timezone.  If you are anything else,
just doublecheck that it is working for you properly.

NOTE, NOTE, NOTE

Topics in the backburner:
- single qemu binary
   Philippe?


Well we wanted a slot to discuss a bit the design problems we have
around some PCI-to-ISA bridges like the PIIX and VIA south bridges.

One of the main problem is figure how to instantiate circular IRQs
with QOM. Ex:

  devA exposes irqAo output
   wires to irqAi input

  devB exposes irqBo output
   wires to irqBi input

How to wire irqAo -> irqBi *AND* irqBo -> irqAi?

However personally I was busy with debugging issues opened for the
8.0 release, and it is probably very late to schedule with Mark and
Bernhard for tomorrow...


- The future of icount.
   Alex?  My understanding is that you are interested in
   qemu 8.1 to open.

Anything else?


Please, send any topic that you are interested in covering.

At the end of Monday I will send an email with the agenda or the
cancellation of the call, so hurry up.

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Thomas Huth]

On 20/03/2023 16.31, Markus Armbruster wrote:

Claudio Imbrenda  writes:


The recently introduced -async-teardown commandline option was not
wired up properly and did not show up in the output of the QMP command
query-command-line-options. This means that libvirt will have no way to
discover whether the feature is supported.


There was nothing improper in its wiring.  The issue is that
query-command-line-options is junk.  See my recent post

 Subject: query-command-line-options (was: [PATCH 1/7] qemu: capabilities: 
Introduce QEMU_CAPS_MACHINE_ACPI)
 Date: Tue, 07 Mar 2023 10:40:23 +0100
 Message-ID: <87jzzsc320.fsf...@pond.sub.org>


This patch fixes the issue by correctly wiring up the commandline
option so that it appears in the output of query-command-line-options.

Reported-by: Boris Fiuczynski 
Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux")
Signed-off-by: Claudio Imbrenda 
---
  util/async-teardown.c | 17 +
  1 file changed, 17 insertions(+)

diff --git a/util/async-teardown.c b/util/async-teardown.c
index 62cdeb0f20..c9b9a3cdb2 100644
--- a/util/async-teardown.c
+++ b/util/async-teardown.c
@@ -12,6 +12,9 @@
   */
  
  #include "qemu/osdep.h"

+#include "qemu/config-file.h"
+#include "qemu/option.h"
+#include "qemu/module.h"
  #include 
  #include 
  #include 
@@ -144,3 +147,17 @@ void init_async_teardown(void)
  clone(async_teardown_fn, new_stack_for_clone(), CLONE_VM, NULL);
  sigprocmask(SIG_SETMASK, &old_signals, NULL);
  }
+
+static QemuOptsList qemu_async_teardown_opts = {
+.name = "async-teardown",
+.head = QTAILQ_HEAD_INITIALIZER(qemu_async_teardown_opts.head),
+.desc = {
+{ /* end of list */ }
+},
+};
+
+static void register_async_teardown(void)
+{
+qemu_add_opts(&qemu_async_teardown_opts);
+}
+opts_init(register_async_teardown);


Now it *is* improperly wired up :)

You're defining new QemuOpts config group "async-teardown" with
arbitrary option parameters, but don't actually use it for parsing or
recording the option.  I figure because you can't: there is no option
argument to parse and record, which is what QemuOpts is designed to do.

If you need the feature to be visible in query-command-line-options, you
should make it an option parameter (a KEY, not a GROUP), preferably of
an existing group / option.


Would it make sense to add it e.g. to "-action" instead, i.e. something like 
"-action teardown=async" ?


 Thomas

Re: [PATCH] vfio/pci: add support for VF token

[Alex Williamson]

On Mon, 20 Mar 2023 11:03:40 +0100
Cédric Le Goater  wrote:

> On 3/20/23 08:35, Minwoo Im wrote:
> > VF token was introduced [1] to kernel vfio-pci along with SR-IOV
> > support [2].  This patch adds support VF token among PF and VF(s). To
> > passthu PCIe VF to a VM, kernel >= v5.7 needs this.
> > 
> > It can be configured with UUID like:
> > 
> >-device vfio-pci,host=:BB:DD:F,vf-token=,...
> > 
> > [1] 
> > https://lore.kernel.org/linux-pci/158396393244.5601.10297430724964025753.st...@gimli.home/
> > [2] 
> > https://lore.kernel.org/linux-pci/158396044753.5601.14804870681174789709.st...@gimli.home/
> > 
> > Cc: Alex Williamson 
> > Signed-off-by: Minwoo Im 
> > Reviewed-by: Klaus Jensen 
> > ---
> >   hw/vfio/pci.c | 13 -
> >   hw/vfio/pci.h |  1 +
> >   2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index ec9a854361..cf27f28936 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error 
> > **errp)
> >   int groupid;
> >   int i, ret;
> >   bool is_mdev;
> > +char uuid[UUID_FMT_LEN];
> > +char *name;
> >   
> >   if (!vbasedev->sysfsdev) {
> >   if (!(~vdev->host.domain || ~vdev->host.bus ||
> > @@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error 
> > **errp)
> >   goto error;
> >   }
> >   
> > -ret = vfio_get_device(group, vbasedev->name, vbasedev, errp);
> > +if (!qemu_uuid_is_null(&vdev->vf_token)) {
> > +qemu_uuid_unparse(&vdev->vf_token, uuid);
> > +name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid);
> > +} else {
> > +name = vbasedev->name;
> > +}
> > +
> > +ret = vfio_get_device(group, name, vbasedev, errp);
> > +g_free(name);
> >   if (ret) {
> >   vfio_put_group(group);
> >   goto error;  
> 
> Shouldn't we set the VF token in the kernel also ? See this QEMU 
> implementation
> 
>https://lore.kernel.org/lkml/20200204161737.34696...@w520.home/
> 
> May be I misunderstood.
> 

I think you're referring to the part there that calls
VFIO_DEVICE_FEATURE in order to set a VF token.  I don't think that's
necessarily applicable here.  I believe this patch is only trying to
make it so that QEMU can consume a VF associated with a PF owned by a
userspace vfio driver, ie. not QEMU.

Setting the VF token is only relevant to PFs, which would require
significantly more SR-IOV infrastructure in QEMU than sketched out in
that proof-of-concept patch.  Even if we did have a QEMU owned PF where
we wanted to generate VFs, the token we use in that case would likely
need to be kept private to QEMU, not passed on the command line.
Thanks,

Alex

> > @@ -3268,6 +3278,7 @@ static void vfio_instance_init(Object *obj)
> >   
> >   static Property vfio_pci_dev_properties[] = {
> >   DEFINE_PROP_PCI_HOST_DEVADDR("host", VFIOPCIDevice, host),
> > +DEFINE_PROP_UUID_NODEFAULT("vf-token", VFIOPCIDevice, vf_token),
> >   DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
> >   DEFINE_PROP_ON_OFF_AUTO("x-pre-copy-dirty-page-tracking", 
> > VFIOPCIDevice,
> >   vbasedev.pre_copy_dirty_page_tracking,
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index 177abcc8fb..2674476d6c 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -137,6 +137,7 @@ struct VFIOPCIDevice {
> >   VFIOVGA *vga; /* 0xa, 0x3b0, 0x3c0 */
> >   void *igd_opregion;
> >   PCIHostDeviceAddress host;
> > +QemuUUID vf_token;
> >   EventNotifier err_notifier;
> >   EventNotifier req_notifier;
> >   int (*resetfn)(struct VFIOPCIDevice *);  
> 
> 

Re: [PULL 00/24] s390x and misc patches for 8.0-rc1

[Thomas Huth]

On 20/03/2023 16.27, Philippe Mathieu-Daudé wrote:

On 20/3/23 16:10, Peter Maydell wrote:

On Mon, 20 Mar 2023 at 14:02, Thomas Huth  wrote:


On 20/03/2023 14.03, Thomas Huth wrote:

   Hi Peter!

The following changes since commit 
74c581b6452394e591f13beba9fea2ec0688e2f5:


    Merge tag 'trivial-branch-for-8.0-pull-request' of 
https://gitlab.com/laurent_vivier/qemu into staging (2023-03-17 14:22:01 
+)


are available in the Git repository at:

    https://gitlab.com/thuth/qemu.git tags/pull-request-2023-03-20

for you to fetch changes up to 48805df9c22a0700fba4b3b548fafaa21726ca68:

    replace TABs with spaces (2023-03-20 12:43:50 +0100)


   Hi Peter,

FYI, since you likely did not put this into your CI branch yet, I did a
small fix on top: I replaced the patch that fixes osdep.h with the one by
Philippe, since it was slightly better (removing the "extern" keyword
instead of swapping it).

New commit ID for the tag is now: c29e73f7e65299ed9261abce3950710d89c64724

I hope that's ok, if not, please let me know.


Ah, I've already merged the old tag into staging for the CI
run. I could drop it and re-do, but we'd burn another lot of
CI minutes on it. Is that worth doing?


Nah, what is worth is getting CI green and saving minutes :)


Agreed, it was a rather cosmetic change - and I'm pretty sure Philippe will 
fix it in the 8.1 cycle instead :-)


 Thomas

Re: [PATCH v2 0/2] fix for #285

[Daniel P . Berrangé]

On Mon, Mar 20, 2023 at 04:05:01PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Emilio,
> 
> On 19/3/23 15:15, Emilio Cota wrote:
> > Ping. Any feedback on these two patches?
> > 
> > https://patchew.org/QEMU/20230205163758.416992-1-c...@braap.org/
> > https://lore.kernel.org/qemu-devel/20230205163758.416992-1-c...@braap.org/
> > 
> > Happy to resend if needed.
> 
> Since we are past hard-freeze, this series likely missed the v8.0
> release IMO. Note that doesn't mean maintainers can't queue it and
> send the pull request later when the next development window opens.

This series is a bug fix though, the freeze only applies to
features. For rc0/1 most bug fixes are still permissible.

I'd really recommend we take this series, as it is a clear
bug in linux-user that can be fairly easily reproduced

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Philippe Mathieu-Daudé]

On 20/3/23 16:23, Claudio Fontana wrote:

Hi Alex, all,

again, this moves TCG-only code to common code, no?


Oh, good point.


Even if this happens to work, the idea is to avoid adding unneeded accel TCG 
code to a KVM-only binary.


Could yet another AccelSysemuCPUOps *accel struct in SysemuCPUOps
help being stricter? ...


We need to keep in mind all dimensions when we do refactorings:

user-mode vs sysemu,
the architecture,
the accel, in particular tcg, non-tcg (which could be not compiled in, 
built-in, or loaded as separate module).

In many cases, testing with --disable-tcg --enable-kvm helps to avoid breakages,
but it is possible also to move in unneeded code in a way that does not 
generate compile or link-time errors, so we need to be a bit alert to that.

Ciao,

C


On 3/20/23 11:10, Alex Bennée wrote:

We don't want to be polluting the core run loop code with target
specific handling, punt it to sysemu_ops where it belongs.

Signed-off-by: Alex Bennée 
---
  include/hw/core/sysemu-cpu-ops.h |  5 +
  target/i386/cpu-internal.h   |  1 +
  accel/tcg/cpu-exec.c | 14 +++---
  target/i386/cpu-sysemu.c | 12 
  target/i386/cpu.c|  1 +
  5 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
index ee169b872c..c9d30172c4 100644
--- a/include/hw/core/sysemu-cpu-ops.h
+++ b/include/hw/core/sysemu-cpu-ops.h
@@ -48,6 +48,11 @@ typedef struct SysemuCPUOps {
   * GUEST_PANICKED events.
   */
  GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
+/**
+ * @handle_cpu_halt: Callback for special handling during cpu_handle_halt()
+ * @cs: The CPUState
+ */


Perhaps insert within a 'tcg' structure for now.

#ifdef CONFIG_TCG
struct {


+void (*handle_cpu_halt)(CPUState *cpu);


} tcg;
#endif

Then we could extract as accel.


  /**
   * @write_elf32_note: Callback for writing a CPU-specific ELF note to a
   * 32-bit VM coredump.

Re: [PATCH v2 1/1] util/async-teardown: wire up query-command-line-options

[Markus Armbruster]

Claudio Imbrenda  writes:

> The recently introduced -async-teardown commandline option was not
> wired up properly and did not show up in the output of the QMP command
> query-command-line-options. This means that libvirt will have no way to
> discover whether the feature is supported.

There was nothing improper in its wiring.  The issue is that
query-command-line-options is junk.  See my recent post

Subject: query-command-line-options (was: [PATCH 1/7] qemu: capabilities: 
Introduce QEMU_CAPS_MACHINE_ACPI)
Date: Tue, 07 Mar 2023 10:40:23 +0100
Message-ID: <87jzzsc320.fsf...@pond.sub.org>

> This patch fixes the issue by correctly wiring up the commandline
> option so that it appears in the output of query-command-line-options.
>
> Reported-by: Boris Fiuczynski 
> Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux")
> Signed-off-by: Claudio Imbrenda 
> ---
>  util/async-teardown.c | 17 +
>  1 file changed, 17 insertions(+)
>
> diff --git a/util/async-teardown.c b/util/async-teardown.c
> index 62cdeb0f20..c9b9a3cdb2 100644
> --- a/util/async-teardown.c
> +++ b/util/async-teardown.c
> @@ -12,6 +12,9 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/config-file.h"
> +#include "qemu/option.h"
> +#include "qemu/module.h"
>  #include 
>  #include 
>  #include 
> @@ -144,3 +147,17 @@ void init_async_teardown(void)
>  clone(async_teardown_fn, new_stack_for_clone(), CLONE_VM, NULL);
>  sigprocmask(SIG_SETMASK, &old_signals, NULL);
>  }
> +
> +static QemuOptsList qemu_async_teardown_opts = {
> +.name = "async-teardown",
> +.head = QTAILQ_HEAD_INITIALIZER(qemu_async_teardown_opts.head),
> +.desc = {
> +{ /* end of list */ }
> +},
> +};
> +
> +static void register_async_teardown(void)
> +{
> +qemu_add_opts(&qemu_async_teardown_opts);
> +}
> +opts_init(register_async_teardown);

Now it *is* improperly wired up :)

You're defining new QemuOpts config group "async-teardown" with
arbitrary option parameters, but don't actually use it for parsing or
recording the option.  I figure because you can't: there is no option
argument to parse and record, which is what QemuOpts is designed to do.

If you need the feature to be visible in query-command-line-options, you
should make it an option parameter (a KEY, not a GROUP), preferably of
an existing group / option.

Re: [PULL 00/24] s390x and misc patches for 8.0-rc1

[Philippe Mathieu-Daudé]

On 20/3/23 16:10, Peter Maydell wrote:

On Mon, 20 Mar 2023 at 14:02, Thomas Huth  wrote:


On 20/03/2023 14.03, Thomas Huth wrote:

   Hi Peter!

The following changes since commit 74c581b6452394e591f13beba9fea2ec0688e2f5:

Merge tag 'trivial-branch-for-8.0-pull-request' of 
https://gitlab.com/laurent_vivier/qemu into staging (2023-03-17 14:22:01 +)

are available in the Git repository at:

https://gitlab.com/thuth/qemu.git tags/pull-request-2023-03-20

for you to fetch changes up to 48805df9c22a0700fba4b3b548fafaa21726ca68:

replace TABs with spaces (2023-03-20 12:43:50 +0100)


   Hi Peter,

FYI, since you likely did not put this into your CI branch yet, I did a
small fix on top: I replaced the patch that fixes osdep.h with the one by
Philippe, since it was slightly better (removing the "extern" keyword
instead of swapping it).

New commit ID for the tag is now: c29e73f7e65299ed9261abce3950710d89c64724

I hope that's ok, if not, please let me know.


Ah, I've already merged the old tag into staging for the CI
run. I could drop it and re-do, but we'd burn another lot of
CI minutes on it. Is that worth doing?


Nah, what is worth is getting CI green and saving minutes :)

[PATCH-for-8.1] block/dmg: Declare a type definition for DMG uncompress function

[Philippe Mathieu-Daudé]

Introduce the BdrvDmgUncompressFunc type defintion. To emphasis
dmg_uncompress_bz2 and dmg_uncompress_lzfse are pointer to functions,
declare them using this new typedef.

Signed-off-by: Philippe Mathieu-Daudé 
---
 block/dmg.h | 8 
 block/dmg.c | 7 ++-
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/block/dmg.h b/block/dmg.h
index e488601b62..dcd6165e63 100644
--- a/block/dmg.h
+++ b/block/dmg.h
@@ -51,10 +51,10 @@ typedef struct BDRVDMGState {
 z_stream zstream;
 } BDRVDMGState;
 
-extern int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in,
- char *next_out, unsigned int avail_out);
+typedef int BdrvDmgUncompressFunc(char *next_in, unsigned int avail_in,
+  char *next_out, unsigned int avail_out);
 
-extern int (*dmg_uncompress_lzfse)(char *next_in, unsigned int avail_in,
-   char *next_out, unsigned int avail_out);
+extern BdrvDmgUncompressFunc *dmg_uncompress_bz2;
+extern BdrvDmgUncompressFunc *dmg_uncompress_lzfse;
 
 #endif
diff --git a/block/dmg.c b/block/dmg.c
index e10b9a2ba5..2769900359 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -31,11 +31,8 @@
 #include "qemu/memalign.h"
 #include "dmg.h"
 
-int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in,
-  char *next_out, unsigned int avail_out);
-
-int (*dmg_uncompress_lzfse)(char *next_in, unsigned int avail_in,
-char *next_out, unsigned int avail_out);
+BdrvDmgUncompressFunc *dmg_uncompress_bz2;
+BdrvDmgUncompressFunc *dmg_uncompress_lzfse;
 
 enum {
 /* Limit chunk sizes to prevent unreasonable amounts of memory being used
-- 
2.38.1

[PATCH v2] add option for a multislot usb ccid device

[Ripke, Klaus]

Signed-off-by: Klaus Ripke 

hw/usb/dev-smartcard-reader.c:
add option for a multislot usb ccid device, similar to audio multi.

(v2 with slight formatting fix on " + ")

---
 hw/usb/dev-smartcard-reader.c | 106 +-
 1 file changed, 103 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-
reader.c
index be0a4fc3bc..30d8892b4e 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -90,10 +90,13 @@ OBJECT_DECLARE_SIMPLE_TYPE(USBCCIDState,
USB_CCID_DEV)
  *  usbccid.sys (winxp, others untested) is a class driver so it
doesn't care.
  *  linux has a number of class drivers, but openct filters based on
  *   vendor/product (/etc/openct.conf under fedora), hence Gemplus.
+ * Use a Omnikey/HID 3121 with multislot for distinction.
  */
 #define CCID_VENDOR_ID  0x08e6
 #define CCID_PRODUCT_ID 0x4433
 #define CCID_DEVICE_VERSION 0x
+#define CCID_VENDOR_ID_MULTI    0x076b
+#define CCID_PRODUCT_ID_MULTI   0x3021
 
 /*
  * BULK_OUT messages from PC to Reader
@@ -312,7 +315,9 @@ struct USBCCIDState {
 uint8_t  bmSlotICCState;
 uint8_t  powered;
 uint8_t  notify_slot_change;
+    /* properties */
 uint8_t  debug;
+    bool multi;
 };
 
 /*
@@ -411,6 +416,34 @@ static const uint8_t qemu_ccid_descriptor[] = {
 0x01,   /* u8  bMaxCCIDBusySlots; */
 };
 
+static const uint8_t qemu_ccid_descriptor_multi[] = {
+    /* Smart Card Device Class Descriptor */
+    0x36,   /* u8  bLength; */
+    0x21,   /* u8  bDescriptorType; Functional */
+    0x10, 0x01, /* u16 bcdCCID; CCID Specification Release Number.
*/
+    0x0e,   /* u8  bMaxSlotIndex; 14, as 16 slots can cause
trouble. */
+    0x07,   /* u8  bVoltageSupport; 01h - 5.0v, 02h - 3.0, 03
- 1.8 */
+
+    0x01, 0x00, /* u32 dwProtocols;  .  = h.*/
+    0x00, 0x00, /* : see above */
+    0xa0, 0x0f, 0x00, 0x00, /* u32 dwMaximumClock; */
+    0x00, 0x00, 0x01, 0x00,
+    0x00, /* u8 bNumClockSupported; see above */
+    0x80, 0x25, 0x00, 0x00, /* u32 dwMaxDataRate ; see above */
+    0x00, 0xC2, 0x01, 0x00,
+    0x00,   /* u8  bNumDataRatesSupported; see above */
+    0xfe, 0x00, 0x00, 0x00, /* u32 dwMaxIFSD; see above */
+    0x00, 0x00, 0x00, 0x00, /* u32 dwSyncProtocols; see above */
+    0x00, 0x00, 0x00, 0x00, /* u32 dwMechanical; see above */
+    0xfe, 0x04, 0x04, 0x00, /* u32 dwFeatures; 400 for better
compat. */
+    0x12, 0x00, 0x01, 0x00, /* u32 dwMaxCCIDMessageLength; see
above */
+    0xFF,   /* u8  bClassGetResponse; see above */
+    0xFF,   /* u8  bClassEnvelope; see above */
+    0x00, 0x00, /* u16 wLcdLayout; see above */
+    0x01,   /* u8  bPINSupport; see above */
+    0x0f,   /* u8  bMaxCCIDBusySlots; modified from 1 */
+};
+
 enum {
 STR_MANUFACTURER = 1,
 STR_PRODUCT,
@@ -457,6 +490,38 @@ static const USBDescIface desc_iface0 = {
 }
 };
 
+static const USBDescIface desc_iface0_multi = {
+    .bInterfaceNumber  = 0,
+    .bNumEndpoints = 3,
+    .bInterfaceClass   = USB_CLASS_CSCID,
+    .bInterfaceSubClass    = USB_SUBCLASS_UNDEFINED,
+    .bInterfaceProtocol    = 0x00,
+    .iInterface    = STR_INTERFACE,
+    .ndesc = 1,
+    .descs = (USBDescOther[]) {
+    {
+    /* smartcard descriptor */
+    .data = qemu_ccid_descriptor_multi,
+    },
+    },
+    .eps = (USBDescEndpoint[]) {
+    {
+    .bEndpointAddress  = USB_DIR_IN | CCID_INT_IN_EP,
+    .bmAttributes  = USB_ENDPOINT_XFER_INT,
+    .bInterval = 255,
+    .wMaxPacketSize    = 64,
+    },{
+    .bEndpointAddress  = USB_DIR_IN | CCID_BULK_IN_EP,
+    .bmAttributes  = USB_ENDPOINT_XFER_BULK,
+    .wMaxPacketSize    = 64,
+    },{
+    .bEndpointAddress  = USB_DIR_OUT | CCID_BULK_OUT_EP,
+    .bmAttributes  = USB_ENDPOINT_XFER_BULK,
+    .wMaxPacketSize    = 64,
+    },
+    }
+};
+
 static const USBDescDevice desc_device = {
 .bcdUSB    = 0x0110,
 .bMaxPacketSize0   = 64,
@@ -474,6 +539,23 @@ static const USBDescDevice desc_device = {
 },
 };
 
+static const USBDescDevice desc_device_multi = {
+    .bcdUSB    = 0x0110,
+    .bMaxPacketSize0   = 64,
+    .bNumConfigurations    = 1,
+    .confs = (USBDescConfig[]) {
+    {
+    .bNumInterfaces    = 1,
+    .bConfigurationValue   = 1,
+    .bmAttributes  = USB_CFG_ATT_ONE |
USB_CFG_ATT_SELFPOWER |
+ USB_CFG_ATT_WAKEUP,
+    .bMaxPower = 50,
+  

Re: [PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Claudio Fontana]

Hi Alex, all,

again, this moves TCG-only code to common code, no?

Even if this happens to work, the idea is to avoid adding unneeded accel TCG 
code to a KVM-only binary.

We need to keep in mind all dimensions when we do refactorings:

user-mode vs sysemu,
the architecture,
the accel, in particular tcg, non-tcg (which could be not compiled in, 
built-in, or loaded as separate module).

In many cases, testing with --disable-tcg --enable-kvm helps to avoid breakages,
but it is possible also to move in unneeded code in a way that does not 
generate compile or link-time errors, so we need to be a bit alert to that.

Ciao,

C 


On 3/20/23 11:10, Alex Bennée wrote:
> We don't want to be polluting the core run loop code with target
> specific handling, punt it to sysemu_ops where it belongs.
> 
> Signed-off-by: Alex Bennée 
> ---
>  include/hw/core/sysemu-cpu-ops.h |  5 +
>  target/i386/cpu-internal.h   |  1 +
>  accel/tcg/cpu-exec.c | 14 +++---
>  target/i386/cpu-sysemu.c | 12 
>  target/i386/cpu.c|  1 +
>  5 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/core/sysemu-cpu-ops.h 
> b/include/hw/core/sysemu-cpu-ops.h
> index ee169b872c..c9d30172c4 100644
> --- a/include/hw/core/sysemu-cpu-ops.h
> +++ b/include/hw/core/sysemu-cpu-ops.h
> @@ -48,6 +48,11 @@ typedef struct SysemuCPUOps {
>   * GUEST_PANICKED events.
>   */
>  GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
> +/**
> + * @handle_cpu_halt: Callback for special handling during 
> cpu_handle_halt()
> + * @cs: The CPUState
> + */
> +void (*handle_cpu_halt)(CPUState *cpu);
>  /**
>   * @write_elf32_note: Callback for writing a CPU-specific ELF note to a
>   * 32-bit VM coredump.
> diff --git a/target/i386/cpu-internal.h b/target/i386/cpu-internal.h
> index 9baac5c0b4..75b302fb33 100644
> --- a/target/i386/cpu-internal.h
> +++ b/target/i386/cpu-internal.h
> @@ -65,6 +65,7 @@ void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
>  void x86_cpu_apic_create(X86CPU *cpu, Error **errp);
>  void x86_cpu_apic_realize(X86CPU *cpu, Error **errp);
>  void x86_cpu_machine_reset_cb(void *opaque);
> +void x86_cpu_handle_halt(CPUState *cs);
>  #endif /* !CONFIG_USER_ONLY */
>  
>  #endif /* I386_CPU_INTERNAL_H */
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index c815f2dbfd..5e5906e199 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -22,6 +22,7 @@
>  #include "qapi/error.h"
>  #include "qapi/type-helpers.h"
>  #include "hw/core/tcg-cpu-ops.h"
> +#include "hw/core/sysemu-cpu-ops.h"
>  #include "trace.h"
>  #include "disas/disas.h"
>  #include "exec/exec-all.h"
> @@ -30,9 +31,6 @@
>  #include "qemu/rcu.h"
>  #include "exec/log.h"
>  #include "qemu/main-loop.h"
> -#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
> -#include "hw/i386/apic.h"
> -#endif
>  #include "sysemu/cpus.h"
>  #include "exec/cpu-all.h"
>  #include "sysemu/cpu-timers.h"
> @@ -650,15 +648,9 @@ static inline bool cpu_handle_halt(CPUState *cpu)
>  {
>  #ifndef CONFIG_USER_ONLY
>  if (cpu->halted) {
> -#if defined(TARGET_I386)
> -if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
> -X86CPU *x86_cpu = X86_CPU(cpu);
> -qemu_mutex_lock_iothread();
> -apic_poll_irq(x86_cpu->apic_state);
> -cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
> -qemu_mutex_unlock_iothread();
> +if (cpu->cc->sysemu_ops->handle_cpu_halt) {
> +cpu->cc->sysemu_ops->handle_cpu_halt(cpu);
>  }
> -#endif /* TARGET_I386 */
>  if (!cpu_has_work(cpu)) {
>  return true;
>  }
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 28115edf44..e545bf7590 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
>  #include "cpu.h"
>  #include "sysemu/xen.h"
>  #include "sysemu/whpx.h"
> @@ -310,6 +311,17 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>   }
>  }
>  
> +void x86_cpu_handle_halt(CPUState *cpu)
> +{
> +if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
> +X86CPU *x86_cpu = X86_CPU(cpu);
> +qemu_mutex_lock_iothread();
> +apic_poll_irq(x86_cpu->apic_state);
> +cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
> +qemu_mutex_unlock_iothread();
> +}
> +}
> +
>  GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs)
>  {
>  X86CPU *cpu = X86_CPU(cs);
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 6576287e5b..67027d28b0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7241,6 +7241,7 @@ static const struct SysemuCPUOps i386_sysemu_ops = {
>  .get_phys_page_attrs_debug = x86_cpu_get_phys_page_attrs_debug,
>  .asidx_from_attrs = x86_asidx_from_attrs,
>  .get_crash_info = x86_cpu_get_crash_info,
> +  

Re: [RFC PATCH v2 05/11] hw/arm/smmuv3: Parse STE config for stage-2

[Eric Auger]

Hi Mostafa,

On 2/26/23 23:06, Mostafa Saleh wrote:
> Parse stage-2 configuration from STE and populate it in SMMUS2Cfg.
> Validity of these value are checked when possible.
s/these value/field values
>
> Only AA64 tables are supported and STT is not supported.
Small Translation Tables (STT)
>
> According to SMMUv3 user manual "5.2 Stream Table Entry": All fields
it is not a user manual but rather an IP architecture spec. put the full
ref?
> with an S2 prefix (with the exception of S2VMID) are IGNORED when
> stage-2 bypasses translation (Config[1] == 0).
>
> Which means that VMID can be used(for TLB tagging) even if stage-2 is
> bypassed, so we parse it unconditionally when S2P exists. Otherwise
> it is set to -1.(only S1P)
>
> As stall is not supported, if S2S is set the translation would abort.
> For S2R, we reuse the same code used for stage-1 with flag
> record_faults. However when nested translation is supported we would
> need to separate stage-1 and stage-2 faults.
>
> Signed-off-by: Mostafa Saleh 
> ---
> Changes in V2:
> - Parse S2PS and S2ENDI
> - Squash with S2VMID parsing patch
> - Squash with S2AFF parsing
> - Squash with fault reporting patch
> - Add check for S2T0SZ
> - Renaming and refactoring code
> ---
>  hw/arm/smmuv3-internal.h |   4 ++
>  hw/arm/smmuv3.c  | 138 +++
>  include/hw/arm/smmuv3.h  |   3 +
>  3 files changed, 145 insertions(+)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 183d5ac8dc..3388e1a5f8 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -526,9 +526,13 @@ typedef struct CD {
>  #define STE_S2TG(x)extract32((x)->word[5], 14, 2)
>  #define STE_S2PS(x)extract32((x)->word[5], 16, 3)
>  #define STE_S2AA64(x)  extract32((x)->word[5], 19, 1)
> +#define STE_S2ENDI(x)  extract32((x)->word[5], 20, 1)
> +#define STE_S2AFFD(x)  extract32((x)->word[5], 21, 1)
I am afraid there is a shift in the fields below. S2HD should be 23
(problem in the original code) and everything is shifted.
>  #define STE_S2HD(x)extract32((x)->word[5], 24, 1)
>  #define STE_S2HA(x)extract32((x)->word[5], 25, 1)
>  #define STE_S2S(x) extract32((x)->word[5], 26, 1)
> +#define STE_S2R(x) extract32((x)->word[5], 27, 1)
> +
>  #define STE_CTXPTR(x)   \
>  ({  \
>  unsigned long addr; \
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 4e90343996..dc74a5442d 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -329,6 +329,46 @@ static int smmu_get_cd(SMMUv3State *s, STE *ste, 
> uint32_t ssid,
>  return 0;
>  }
>  
> +/*
> + * Max valid value is 39 when SMMU_IDR3.STT == 0.
> + * In architectures after SMMUv3.0:
> + * - If STE.S2TG selects a 4KB or 16KB granule, the minimum valid value for 
> this
> + * field is MAX(16, 64-IAS)
> + * - If STE.S2TG selects a 64KB granule, the minimum valid value for this 
> field
> + * is (64-IAS).
> + * As we only support AA64, IAS = OAS.
OK this comes from STE.S2T0SZ description on the SMMU arch spec. You can
add this in previous patch too.
> + */
> +static bool t0sz_valid(SMMUTransCfg *cfg)
use S2t0sz to avoid confusion with S1 stuff
> +{
> +if (cfg->s2cfg.tsz > 39) {
> +return false;
> +}
> +
> +if (cfg->s2cfg.granule_sz == 16) {
> +return (cfg->s2cfg.tsz >= 64 - cfg->s2cfg.oas);
> +}
> +
> +return (cfg->s2cfg.tsz >= MAX(64 - cfg->s2cfg.oas, 16));
> +}

this chapter also states:
"The usable range of values is further constrained by a function of the
starting level set by S2SL0 and, if S2AA64 == 1, granule size set by
S2TG as described by the Armv8 translation system. Use of a value of
S2T0SZ that is inconsistent with the permitted range (given S2SL0 and
S2TG) is ILLEGAL."
> +
> +/*
> + * Return true if s2 page table config is valid.
> + * This checks with the configured start level, ias_bits and granularity we 
> can
> + * have a valid page table as described in ARM ARM D8.2 Translation process.
> + * The idea here is to see for the highest possible number of IPA bits, how
> + * many concatenated tables we would need, if it is more than 16, then this 
> is
> + * not possible.
why? in that case doesn't it mean that we can't use concatanated tables?
> + */
> +static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
> +{
> +int level = get_start_level(sl0, gran);
> +uint64_t ipa_bits = 64 - t0sz;
> +uint64_t max_ipa = (1ULL << ipa_bits) - 1;
> +int nr_concat = pgd_idx(level, gran, max_ipa) + 1;
> +
> +return nr_concat <= SMMU_MAX_S2_CONCAT;
> +}
> +
>  /* Returns < 0 in case of invalid STE, 0 otherwise */
>  static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>STE *ste, SMMUEventInfo *event)
> @@ -354,7 +394,105 @@ static int decode_ste(SMMUv3State *s, SMMUT

Re: [PULL 00/24] s390x and misc patches for 8.0-rc1

[Peter Maydell]

On Mon, 20 Mar 2023 at 14:02, Thomas Huth  wrote:
>
> On 20/03/2023 14.03, Thomas Huth wrote:
> >   Hi Peter!
> >
> > The following changes since commit 74c581b6452394e591f13beba9fea2ec0688e2f5:
> >
> >Merge tag 'trivial-branch-for-8.0-pull-request' of 
> > https://gitlab.com/laurent_vivier/qemu into staging (2023-03-17 14:22:01 
> > +)
> >
> > are available in the Git repository at:
> >
> >https://gitlab.com/thuth/qemu.git tags/pull-request-2023-03-20
> >
> > for you to fetch changes up to 48805df9c22a0700fba4b3b548fafaa21726ca68:
> >
> >replace TABs with spaces (2023-03-20 12:43:50 +0100)
>
>   Hi Peter,
>
> FYI, since you likely did not put this into your CI branch yet, I did a
> small fix on top: I replaced the patch that fixes osdep.h with the one by
> Philippe, since it was slightly better (removing the "extern" keyword
> instead of swapping it).
>
> New commit ID for the tag is now: c29e73f7e65299ed9261abce3950710d89c64724
>
> I hope that's ok, if not, please let me know.

Ah, I've already merged the old tag into staging for the CI
run. I could drop it and re-do, but we'd burn another lot of
CI minutes on it. Is that worth doing?

thanks
-- PMM

Re: [PATCH v2 0/2] fix for #285

[Philippe Mathieu-Daudé]

Hi Emilio,

On 19/3/23 15:15, Emilio Cota wrote:

Ping. Any feedback on these two patches?

https://patchew.org/QEMU/20230205163758.416992-1-c...@braap.org/
https://lore.kernel.org/qemu-devel/20230205163758.416992-1-c...@braap.org/

Happy to resend if needed.


Since we are past hard-freeze, this series likely missed the v8.0
release IMO. Note that doesn't mean maintainers can't queue it and
send the pull request later when the next development window opens.


On Fri, Feb 17, 2023 at 07:44:38 -0500, Emilio Cota wrote:

Ping.

This fixes a bug (admittedly with a big hammer) that affects
users with heavily multi-threaded user-mode workloads.

Thanks,
Emilio

On Sun, Feb 05, 2023 at 11:37:56 -0500, Emilio Cota wrote:

Changes since v1:

- Add configure check to only use QTree if Glib still implements gslice.
   If Glib doesn't, then we call Glib directly with inline functions.
- Add TODO's so that in the future (i.e. when the minimum version of
   Glib that we use doesn't implement gslice) we remove QTree.
- Add comment to the top of qtree.h.
- Make qtree-bench results more robust by running longer or more times.
- Drop deprecated API calls (they're unused in QEMU).
- Drop API calls that are too recent (they're unused in QEMU).
- Drop macro benchmark results from the TCG patch since they're too noisy.
- Add test program to the commit log so that we don't lose it in the future
   even if the bug tracker goes away.

Thanks,
Emilio

Re: [PULL 00/31] various fixes (testing, plugins, gitdm)

[Philippe Mathieu-Daudé]

On 20/3/23 15:56, Daniel P. Berrangé wrote:

On Mon, Mar 20, 2023 at 03:43:07PM +0100, Philippe Mathieu-Daudé wrote:

On 20/3/23 15:15, Daniel P. Berrangé wrote:

On Mon, Mar 20, 2023 at 01:42:46PM +, Peter Maydell wrote:

On Sat, 18 Mar 2023 at 11:46, Alex Bennée  wrote:


The following changes since commit 74c581b6452394e591f13beba9fea2ec0688e2f5:

Merge tag 'trivial-branch-for-8.0-pull-request' of 
https://gitlab.com/laurent_vivier/qemu into staging (2023-03-17 14:22:01 +)

are available in the Git repository at:

https://gitlab.com/stsquad/qemu.git tags/pull-for-8.0-170323-4

for you to fetch changes up to 4f2c431acd43d0aa505494229d05fa343762f272:

qtests: avoid printing comments before g_test_init() (2023-03-17
17:50:19 +)

You can see my CI run on the branch here:

https://gitlab.com/stsquad/qemu/-/pipelines/810271620

The failures:

FreeBSD's time out on a migration test
Centos8 Stream because my private runner needs more disk space


Misc fixes for 8.0 (testing, plugins, gitdm)

- update Alpine image used for testing images
- include libslirp in custom runner build env
- update gitlab-runner recipe for CentOS
- update docker calls for better caching behaviour
- document some plugin callbacks
- don't use tags to define drives for lkft baseline tests
- fix missing clear of plugin_mem_cbs
- fix iotests to report individual results
- update the gitdm metadata for contributors
- avoid printing comments before g_test_init()



This seems to consistently fail an avocado test on the
centos-stream-8-x86_64 job:
(21/51) tests/avocado/multiprocess.py:Multiprocess.test_multiprocess_x86_64:
ERROR: ConnectError: Failed to establish session: EOFError\n Exit
code: 1\n Command: ./qemu-system-x86_64 -display none -vga none
-chardev socket,id=mon,fd=17 -mon chardev=mon,mode=control -machine
x-remote -nodefaults -device lsi53c895a,id=lsi1 -object x-remote-o...
(0.10 s)

https://gitlab.com/qemu-project/qemu/-/jobs/3962028269
https://gitlab.com/qemu-project/qemu/-/jobs/3965134190



The iotests also don't seem to pass on the OpenBSD VM after this;
which test fails varies from run to run but the common factor
is a complaint about running out of disk space:


This must be caused by the change in the way we register the
iotests with meson, as I don't see any other interesting changes
in this series.


See "05/31 gitlab: update centos-8-stream job", now we call
'make check-avocado' instead of
scripts/ci/org.centos/stream/8/x86_64/test-avocado.


I was referring to Peter's comment about the OpenBSD Vms showing
failures wrt out of disk space. That won't be connected to any
change to the centos 8 job


): I really need more coffee

Re: [PATCH 09/10] accel/tcg: re-inline the filtering of virtual IRQs but data driven

[Philippe Mathieu-Daudé]

On 20/3/23 11:10, Alex Bennée wrote:

Although only I386 currently uses it it is not inconceivable that
other arches might find this facility useful.

Signed-off-by: Alex Bennée 
---
  include/hw/core/tcg-cpu-ops.h |  5 +
  accel/tcg/cpu-exec.c  | 29 +
  target/i386/tcg/tcg-cpu.c |  1 +
  3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
index 66c0cecdde..8e8df8c330 100644
--- a/include/hw/core/tcg-cpu-ops.h
+++ b/include/hw/core/tcg-cpu-ops.h
@@ -121,6 +121,11 @@ struct TCGCPUOps {
   */
  bool (*io_recompile_replay_branch)(CPUState *cpu,
 const TranslationBlock *tb);
+/**
+ * @virtual_interrupts: IRQs that can be ignored for replay purposes
+ */
+int virtual_interrupts;


Maybe rename as "virtual_interrupts_mask" and use 'unsigned' type?

Re: [RFC PATCH v2 04/11] hw/arm/smmuv3: Add page table walk for stage-2

[Eric Auger]

Hi Mostafa,

On 2/26/23 23:06, Mostafa Saleh wrote:
> In preparation for adding stage-2 support, add Stage-2 PTW code.
> Only Aarch64 format is supported as stage-1.
>
> Nesting stage-1 and stage-2 is not supported right now.
>
> HTTU is not supported, SW is expected to maintain the Access flag.
> This is described in the SMMUv3 manual "5.2. Stream Table Entry" in
> "[181] S2AFFD".
> This flag determines the behavior on access of a stage-2 page whose
> descriptor has AF == 0:
> - 0b0: An Access flag fault occurs (stall not supported).
> - 0b1: An Access flag fault never occurs.
> An Access fault takes priority over a Permission fault.
>
> Checks for IPA and output PA are done according to the user manual
> "3.4 Address sizes".
replace user manual by the exact ref of the doc. I guess this is IHI0070E
>
> Signed-off-by: Mostafa Saleh 
> ---
> Changes in v2:
> - Squash S2AFF PTW code.
> - Use common functions between stage-1 and stage-2.
> - Add checks for IPA and out PA.
> ---
>  hw/arm/smmu-common.c   | 132 -
>  hw/arm/smmu-internal.h |  39 
>  2 files changed, 170 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index b49c1affdb..3f448bc82e 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -363,6 +363,130 @@ error:
>  return -EINVAL;
>  }
>  
> +/**
> + * smmu_ptw_64_s2 - VMSAv8-64 Walk of the page tables for a given IOVA
> + * for stage-2.
> + * @cfg: translation config
> + * @iova: iova to translate
> + * @perm: access type
> + * @tlbe: SMMUTLBEntry (out)
> + * @info: handle to an error info
> + *
> + * Return 0 on success, < 0 on error. In case of error, @info is filled
> + * and tlbe->perm is set to IOMMU_NONE.
> + * Upon success, @tlbe is filled with translated_addr and entry
> + * permission rights.
> + */
> +static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
> +  dma_addr_t iova, IOMMUAccessFlags perm,
Rename iova into ipa?
> +  SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> +{
> +const int stage = 2;
> +int granule_sz = cfg->s2cfg.granule_sz;
> +/* ARM ARM: Table D8-7. */
You may refer the full reference
in  DDI0487I.a as the chapter/table may vary. For instance in
ARM DDI 0487F.c D8 corresponds to the activity monitor extensions
> +int inputsize = 64 - cfg->s2cfg.tsz;
> +int level = get_start_level(cfg->s2cfg.sl0, granule_sz);
> +int stride = SMMU_STRIDE(granule_sz);
> +int idx = pgd_idx(level, granule_sz, iova);
> +/*
> + * Get the ttb from concatenated structure.
> + * The offset is the idx * size of each ttb(number of ptes * 
> (sizeof(pte))
> + */
what I don't get is the spec that concatenated tables are used if the
initial lookup level would require less or equal than 16 entries. I
don't see such kind of check or is done implicitly somewhere else?
> +uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
> +  idx * sizeof(uint64_t);
> +dma_addr_t indexmask = SMMU_IDXMSK(inputsize, stride, level);
> +
> +baseaddr &= ~indexmask;
> +
> +/*
> + * If the input address of a transaction exceeds the size of the IAS, a
> + * stage 1 Address Size fault occurs.
> + * For AA64, IAS = OAS
then you may use a local variable max_as = cfg->s2cfg.oas used below and
in 

if (gpa >= (1ULL << cfg->s2cfg.oas)) {
this is not obvious though that the ias = oas. Where does it come from?

In implementations of SMMUv3.1 and later, this value is Reserved and S2PS 
behaves as 0b110 (52 bits).
Effective_S2PS = MIN(STE.S2PS, SMMU_IDR5.OAS);
OAS is a maximum of 52 bits in SMMUv3.1 and later

> + */
> +if (iova >= (1ULL << cfg->s2cfg.oas)) {
> +info->type = SMMU_PTW_ERR_ADDR_SIZE;
> +info->stage = 1;
> +goto error_no_stage;
> +}
> +
> +while (level < SMMU_LEVELS) {
I would rename SMMU_LEVELs
> +uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
> +uint64_t mask = subpage_size - 1;
> +uint32_t offset = iova_level_offset(iova, inputsize, level, 
> granule_sz);
> +uint64_t pte, gpa;
> +dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
> +uint8_t ap;
> +
> +if (get_pte(baseaddr, offset, &pte, info)) {
> +goto error;
> +}
> +trace_smmu_ptw_level(stage, level, iova, subpage_size,
> + baseaddr, offset, pte);
> +if (is_invalid_pte(pte) || is_reserved_pte(pte, level)) {
> +trace_smmu_ptw_invalid_pte(stage, level, baseaddr,
> +   pte_addr, offset, pte);
> +break;
> +}
> +
> +if (is_table_pte(pte, level)) {
> +baseaddr = get_table_pte_address(pte, granule_sz);
> +level++;
> +continue;
> +} else if (is_page_pte(pte, level)) {
> +gpa = get_page_pte_addres

Re: [PULL 00/31] various fixes (testing, plugins, gitdm)

[Daniel P . Berrangé]

On Mon, Mar 20, 2023 at 03:43:07PM +0100, Philippe Mathieu-Daudé wrote:
> On 20/3/23 15:15, Daniel P. Berrangé wrote:
> > On Mon, Mar 20, 2023 at 01:42:46PM +, Peter Maydell wrote:
> > > On Sat, 18 Mar 2023 at 11:46, Alex Bennée  wrote:
> > > > 
> > > > The following changes since commit 
> > > > 74c581b6452394e591f13beba9fea2ec0688e2f5:
> > > > 
> > > >Merge tag 'trivial-branch-for-8.0-pull-request' of 
> > > > https://gitlab.com/laurent_vivier/qemu into staging (2023-03-17 
> > > > 14:22:01 +)
> > > > 
> > > > are available in the Git repository at:
> > > > 
> > > >https://gitlab.com/stsquad/qemu.git tags/pull-for-8.0-170323-4
> > > > 
> > > > for you to fetch changes up to 4f2c431acd43d0aa505494229d05fa343762f272:
> > > > 
> > > >qtests: avoid printing comments before g_test_init() (2023-03-17
> > > >17:50:19 +)
> > > > 
> > > > You can see my CI run on the branch here:
> > > > 
> > > >https://gitlab.com/stsquad/qemu/-/pipelines/810271620
> > > > 
> > > > The failures:
> > > > 
> > > >FreeBSD's time out on a migration test
> > > >Centos8 Stream because my private runner needs more disk space
> > > > 
> > > > 
> > > > Misc fixes for 8.0 (testing, plugins, gitdm)
> > > > 
> > > >- update Alpine image used for testing images
> > > >- include libslirp in custom runner build env
> > > >- update gitlab-runner recipe for CentOS
> > > >- update docker calls for better caching behaviour
> > > >- document some plugin callbacks
> > > >- don't use tags to define drives for lkft baseline tests
> > > >- fix missing clear of plugin_mem_cbs
> > > >- fix iotests to report individual results
> > > >- update the gitdm metadata for contributors
> > > >- avoid printing comments before g_test_init()
> > > > 
> > > 
> > > This seems to consistently fail an avocado test on the
> > > centos-stream-8-x86_64 job:
> > > (21/51) 
> > > tests/avocado/multiprocess.py:Multiprocess.test_multiprocess_x86_64:
> > > ERROR: ConnectError: Failed to establish session: EOFError\n Exit
> > > code: 1\n Command: ./qemu-system-x86_64 -display none -vga none
> > > -chardev socket,id=mon,fd=17 -mon chardev=mon,mode=control -machine
> > > x-remote -nodefaults -device lsi53c895a,id=lsi1 -object x-remote-o...
> > > (0.10 s)
> > > 
> > > https://gitlab.com/qemu-project/qemu/-/jobs/3962028269
> > > https://gitlab.com/qemu-project/qemu/-/jobs/3965134190
> > > 
> > > 
> > > 
> > > The iotests also don't seem to pass on the OpenBSD VM after this;
> > > which test fails varies from run to run but the common factor
> > > is a complaint about running out of disk space:
> > 
> > This must be caused by the change in the way we register the
> > iotests with meson, as I don't see any other interesting changes
> > in this series.
> 
> See "05/31 gitlab: update centos-8-stream job", now we call
> 'make check-avocado' instead of
> scripts/ci/org.centos/stream/8/x86_64/test-avocado.

I was referring to Peter's comment about the OpenBSD Vms showing
failures wrt out of disk space. That won't be connected to any
change to the centos 8 job

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|