Re: [PATCH v4 06/10] migration: Introduce dirty-limit capability

[Hyman Huang]

在 2023/3/24 22:32, Markus Armbruster 写道:

Hyman Huang  writes:


在 2023/3/24 20:11, Markus Armbruster 写道:

huang...@chinatelecom.cn writes:


From: Hyman Huang(黄勇) 

Introduce migration dirty-limit capability, which can
be turned on before live migration and limit dirty
page rate durty live migration.

Introduce migrate_dirty_limit function to help check
if dirty-limit capability enabled during live migration.

Meanwhile, refactor vcpu_dirty_rate_stat_collect
so that period can be configured instead of hardcoded.

dirty-limit capability is kind of like auto-converge
but using dirty limit instead of traditional cpu-throttle
to throttle guest down. To enable this feature, turn on
the dirty-limit capability before live migration using
migrate-set-capabilities, and set the parameters
"x-vcpu-dirty-limit-period", "vcpu-dirty-limit" suitably
to speed up convergence.

Signed-off-by: Hyman Huang(黄勇) 
Acked-by: Peter Xu 

[...]


diff --git a/qapi/migration.json b/qapi/migration.json
index d33cc2d582..b7a92be055 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -477,6 +477,8 @@
   #will be handled faster.  This is a performance feature 
and
   #should not affect the correctness of postcopy migration.
   #(since 7.1)
+# @dirty-limit: Use dirty-limit to throttle down guest if enabled.
+#   (since 8.0)


Feels too terse.  What exactly is used and how?  It's not the capability
itself (although the text sure sounds like it).  I guess it's the thing
you set with command set-vcpu-dirty-limit.

Is that used only when the capability is set?


Yes, live migration set "dirty-limit" value when that capability is set,
the comment changes to "Apply the algorithm of dirty page rate limit to throttle 
down guest if capability is set, rather than auto-converge".

Please continue to polish the doc if needed. Thanks.


Let's see whether I understand.

Throttling happens only during migration.

There are two throttling algorithms: "auto-converge" (default) and
"dirty page rate limit".

The latter can be tuned with set-vcpu-dirty-limit.

Correct?

Yes


What happens when migration capability dirty-limit is enabled, but the
user hasn't set a limit with set-vcpu-dirty-limit, or canceled it with
cancel-vcpu-dirty-limit?
dirty-limit capability use the default value if user hasn't set. In the 
path of cancel-vcpu-dirty-limit, canceling should be check and not be 
allowed if migration is in process.


see the following code in commit:
[PATCH v4 08/10] migration: Implement dirty-limit convergence algo

--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -438,6 +438,8 @@ void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index,
  int64_t cpu_index,
  Error **errp)
 {
+MigrationState *ms = migrate_get_current();
+
 if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
 return;
 }
@@ -451,6 +453,15 @@ void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index,
 return;
 }

+if (migration_is_running(ms->state) &&
+(!qemu_thread_is_self(&ms->thread)) &&
+migrate_dirty_limit() &&
+dirtylimit_in_service()) {
+error_setg(errp, "can't cancel dirty page limit while"
+   " migration is running");
+return;
+}
+
 dirtylimit_state_lock();

 if (has_cpu_index) {
@@ -486,6 +497,8 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index,
   uint64_t dirty_rate,
   Error **errp)
 {
+MigrationState *ms = migrate_get_current();
+
 if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
 error_setg(errp, "dirty page limit feature requires KVM with"
" accelerator property 'dirty-ring-size' set'");
@@ -502,6 +515,15 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index,
 return;
 }

+if (migration_is_running(ms->state) &&
+(!qemu_thread_is_self(&ms->thread)) &&
+migrate_dirty_limit() &&
+dirtylimit_in_service()) {
+error_setg(errp, "can't cancel dirty page limit while"
+   " migration is running");
+return;
+}
+
 dirtylimit_state_lock();

 if (!dirtylimit_in_service()) {
--




   #
   # Features:
   # @unstable: Members @x-colo and @x-ignore-shared are experimental.
@@ -492,7 +494,7 @@
  'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
  { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
  'validate-uuid', 'background-snapshot',
-   'zero-copy-send', 'postcopy-preempt'] }
+   'zero-copy-send', 'postcopy-preempt', 'dirty-limit'] }
 ##
   # @MigrationCapabilityStatus:

[...]





--
Best regard

Hyman Huang(黄勇)

Re: [PATCH 6/8] target/riscv: Fix format for indentation

[liweiwei]

On 2023/3/25 22:23, LIU Zhiwei wrote:


On 2023/3/24 20:38, Weiwei Li wrote:

Fix identation problems, and try to use the same indentation strategy
in the same file.

Signed-off-by: Weiwei Li 
Signed-off-by: Junqiang Wang 
---
  target/riscv/arch_dump.c    |   4 +-
  target/riscv/cpu.c  |   4 +-
  target/riscv/cpu_helper.c   |  15 +--
  target/riscv/insn_trans/trans_rvv.c.inc |  46 
  target/riscv/op_helper.c    |   4 +-
  target/riscv/pmp.c  |  19 ++--
  target/riscv/pmp.h  |   9 +-
  target/riscv/vector_helper.c    | 134 +---
  8 files changed, 122 insertions(+), 113 deletions(-)

diff --git a/target/riscv/arch_dump.c b/target/riscv/arch_dump.c
index 736a232956..573587810e 100644
--- a/target/riscv/arch_dump.c
+++ b/target/riscv/arch_dump.c
@@ -180,8 +180,8 @@ int cpu_get_dump_info(ArchDumpInfo *info,
  info->d_class = ELFCLASS32;
  #endif
  -    info->d_endian = (env->mstatus & MSTATUS_UBE) != 0
- ? ELFDATA2MSB : ELFDATA2LSB;
+    info->d_endian = (env->mstatus & MSTATUS_UBE) != 0 ?
+ ELFDATA2MSB : ELFDATA2LSB;
    return 0;
  }
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 16e465a0ab..75dab70ba7 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -344,8 +344,8 @@ static void riscv_any_cpu_init(Object *obj)
    #ifndef CONFIG_USER_ONLY
  set_satp_mode_max_supported(RISCV_CPU(obj),
-    riscv_cpu_mxl(&RISCV_CPU(obj)->env) == MXL_RV32 ?
-    VM_1_10_SV32 : VM_1_10_SV57);
+    riscv_cpu_mxl(&RISCV_CPU(obj)->env) == MXL_RV32 ?
+    VM_1_10_SV32 : VM_1_10_SV57);
  #endif
    set_priv_version(env, PRIV_VERSION_1_12_0);
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index de2d4a8c1d..08689ee3f6 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -68,12 +68,12 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, 
target_ulong *pc,

  flags = FIELD_DP32(flags, TB_FLAGS, VILL, env->vill);
  flags = FIELD_DP32(flags, TB_FLAGS, SEW, sew);
  flags = FIELD_DP32(flags, TB_FLAGS, LMUL,
-    FIELD_EX64(env->vtype, VTYPE, VLMUL));
+   FIELD_EX64(env->vtype, VTYPE, VLMUL));
  flags = FIELD_DP32(flags, TB_FLAGS, VL_EQ_VLMAX, vl_eq_vlmax);
  flags = FIELD_DP32(flags, TB_FLAGS, VTA,
-    FIELD_EX64(env->vtype, VTYPE, VTA));
+   FIELD_EX64(env->vtype, VTYPE, VTA));
  flags = FIELD_DP32(flags, TB_FLAGS, VMA,
-    FIELD_EX64(env->vtype, VTYPE, VMA));
+   FIELD_EX64(env->vtype, VTYPE, VMA));
  } else {
  flags = FIELD_DP32(flags, TB_FLAGS, VILL, 1);
  }
@@ -1056,7 +1056,7 @@ restart:
  /* add write permission on stores or if the page is 
already dirty,
 so that we TLB miss on later writes to update the 
dirty bit */

  if ((pte & PTE_W) &&
-    (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
+    (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
  *prot |= PAGE_WRITE;
  }
  return TRANSLATE_SUCCESS;
@@ -1285,9 +1285,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr 
address, int size,

 false);
    qemu_log_mask(CPU_LOG_MMU,
-    "%s 2nd-stage address=%" VADDR_PRIx " ret %d 
physical "

-    HWADDR_FMT_plx " prot %d\n",
-    __func__, im_address, ret, pa, prot2);
+  "%s 2nd-stage address=%" VADDR_PRIx
+  " ret %d physical "
+  HWADDR_FMT_plx " prot %d\n",
+  __func__, im_address, ret, pa, prot2);
    prot &= prot2;
  diff --git a/target/riscv/insn_trans/trans_rvv.c.inc 
b/target/riscv/insn_trans/trans_rvv.c.inc

index f2e3d38515..2aed66934a 100644
--- a/target/riscv/insn_trans/trans_rvv.c.inc
+++ b/target/riscv/insn_trans/trans_rvv.c.inc
@@ -238,8 +238,8 @@ static bool vext_check_store(DisasContext *s, int 
vd, int nf, uint8_t eew)

  {
  int8_t emul = eew - s->sew + s->lmul;
  return (emul >= -3 && emul <= 3) &&
-    require_align(vd, emul) &&
-    require_nf(vd, nf, emul);
+   require_align(vd, emul) &&
+   require_nf(vd, nf, emul);
  }
    /*
@@ -315,7 +315,7 @@ static bool vext_check_ld_index(DisasContext *s, 
int vd, int vs2,

  int8_t seg_vd;
  int8_t emul = eew - s->sew + s->lmul;
  bool ret = vext_check_st_index(s, vd, vs2, nf, eew) &&
-    require_vm(vm, vd);
+   require_vm(vm, vd);
    /* Each segment register group has to follow overlap rules. */
  for (int i = 0; i < nf; ++i) {
@@ -345,8 +345,8 @@ static bool vext_check_ld_index(DisasContext *s, 
int vd,

Re: [PATCH v6 02/25] target/riscv: Add a general status enum for extensions

[liweiwei]

On 2023/3/25 18:54, Richard Henderson wrote:

From: LIU Zhiwei 

The pointer masking is the only extension that directly use status.
The vector or float extension uses the status in an indirect way.

Replace the pointer masking extension special status fields with
the general status.

Reviewed-by: Richard Henderson 
Signed-off-by: LIU Zhiwei 
Message-Id: <20230324143031.1093-3-zhiwei_...@linux.alibaba.com>
[rth: Add a typedef for the enum]
Signed-off-by: Richard Henderson 
---


Reviewed-by: Weiwei Li 

Weiwei Li

  target/riscv/cpu.h  |  8 
  target/riscv/cpu_bits.h | 12 
  target/riscv/cpu.c  |  2 +-
  target/riscv/csr.c  | 14 +++---
  4 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 12fe8d8546..30d9828d59 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -99,6 +99,14 @@ enum {
  TRANSLATE_G_STAGE_FAIL
  };
  
+/* Extension context status */

+typedef enum {
+EXT_STATUS_DISABLED = 0,
+EXT_STATUS_INITIAL,
+EXT_STATUS_CLEAN,
+EXT_STATUS_DIRTY,
+} RISCVExtStatus;
+
  #define MMU_USER_IDX 3
  
  #define MAX_RISCV_PMPS (16)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index fca7ef0cef..b84f62f8d6 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -9,6 +9,9 @@
   (((uint64_t)(val) * ((mask) & ~((mask) << 1))) & \
   (uint64_t)(mask)))
  
+/* Extension context status mask */

+#define EXT_STATUS_MASK 0x3ULL
+
  /* Floating point round mode */
  #define FSR_RD_SHIFT5
  #define FSR_RD  (0x7 << FSR_RD_SHIFT)
@@ -734,13 +737,6 @@ typedef enum RISCVException {
  #define PM_ENABLE   0x0001ULL
  #define PM_CURRENT  0x0002ULL
  #define PM_INSN 0x0004ULL
-#define PM_XS_MASK  0x0003ULL
-
-/* PointerMasking XS bits values */
-#define PM_EXT_DISABLE  0xULL
-#define PM_EXT_INITIAL  0x0001ULL
-#define PM_EXT_CLEAN0x0002ULL
-#define PM_EXT_DIRTY0x0003ULL
  
  /* Execution enviornment configuration bits */

  #define MENVCFG_FIOM   BIT(0)
@@ -780,7 +776,7 @@ typedef enum RISCVException {
  #define S_OFFSET 5ULL
  #define M_OFFSET 8ULL
  
-#define PM_XS_BITS   (PM_XS_MASK << XS_OFFSET)

+#define PM_XS_BITS   (EXT_STATUS_MASK << XS_OFFSET)
  #define U_PM_ENABLE  (PM_ENABLE  << U_OFFSET)
  #define U_PM_CURRENT (PM_CURRENT << U_OFFSET)
  #define U_PM_INSN(PM_INSN<< U_OFFSET)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1e97473af2..1135106b3e 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -764,7 +764,7 @@ static void riscv_cpu_reset_hold(Object *obj)
  i++;
  }
  /* mmte is supposed to have pm.current hardwired to 1 */
-env->mmte |= (PM_EXT_INITIAL | MMTE_M_PM_CURRENT);
+env->mmte |= (EXT_STATUS_INITIAL | MMTE_M_PM_CURRENT);
  #endif
  env->xl = riscv_cpu_mxl(env);
  riscv_cpu_update_mask(env);
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d522efc0b6..abea7b749e 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3513,7 +3513,7 @@ static RISCVException write_mmte(CPURISCVState *env, int 
csrno,
  
  /* hardwiring pm.instruction bit to 0, since it's not supported yet */

  wpri_val &= ~(MMTE_M_PM_INSN | MMTE_S_PM_INSN | MMTE_U_PM_INSN);
-env->mmte = wpri_val | PM_EXT_DIRTY;
+env->mmte = wpri_val | EXT_STATUS_DIRTY;
  riscv_cpu_update_mask(env);
  
  /* Set XS and SD bits, since PM CSRs are dirty */

@@ -3593,7 +3593,7 @@ static RISCVException write_mpmmask(CPURISCVState *env, 
int csrno,
  if ((env->priv == PRV_M) && (env->mmte & M_PM_ENABLE)) {
  env->cur_pmmask = val;
  }
-env->mmte |= PM_EXT_DIRTY;
+env->mmte |= EXT_STATUS_DIRTY;
  
  /* Set XS and SD bits, since PM CSRs are dirty */

  mstatus = env->mstatus | MSTATUS_XS;
@@ -3621,7 +3621,7 @@ static RISCVException write_spmmask(CPURISCVState *env, 
int csrno,
  if ((env->priv == PRV_S) && (env->mmte & S_PM_ENABLE)) {
  env->cur_pmmask = val;
  }
-env->mmte |= PM_EXT_DIRTY;
+env->mmte |= EXT_STATUS_DIRTY;
  
  /* Set XS and SD bits, since PM CSRs are dirty */

  mstatus = env->mstatus | MSTATUS_XS;
@@ -3649,7 +3649,7 @@ static RISCVException write_upmmask(CPURISCVState *env, 
int csrno,
  if ((env->priv == PRV_U) && (env->mmte & U_PM_ENABLE)) {
  env->cur_pmmask = val;
  }
-env->mmte |= PM_EXT_DIRTY;
+env->mmte |= EXT_STATUS_DIRTY;
  
  /* Set XS and SD bits, since PM CSRs are dirty */

  mstatus = env->mstatus | MSTATUS_XS;
@@ -3673,7 +3673,7 @@ static RISCVException write_mpmbase(CPURISCVState *env, 
int csrno,
  if ((env->priv == PRV_M) && (env->mmte & M_PM_ENABLE)) {
  env->cur_pmbase = val;
  }
-env->mmte |= PM_EXT_DIRTY;
+env->mmte |= EXT_STATUS_DIRTY;
  
  /* Set XS and SD bits, since PM CSRs are dirty */

  mstatus = en

Re: [PATCH v6 3/3] tpm: Add support for TPM device over I2C bus

[Stefan Berger]

On 3/25/23 22:38, Ninad Palsule wrote:

Qemu already supports devices attached to ISA and sysbus. This drop adds
support for the I2C bus attached TPM devices. I2C model only supports
TPM2 protocol.

This commit includes changes for the common code.
- Added I2C emulation model. Logic was added in the model to temporarily
   cache the data as I2C interface works per byte basis.
- New tpm type "tpm-tis-i2c" added for I2C support. The user has to
   provide this string on command line.

Testing:
   TPM I2C device module is tested using SWTPM (software based TPM
   package). Qemu uses the rainier machine and is connected to swtpm over
   the socket interface.

   The command to start swtpm is as follows:
   $ swtpm socket --tpmstate dir=/tmp/mytpm1\
  --ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock  \
  --tpm2 --log level=100

   The command to start qemu is as follows:
   $ qemu-system-arm -M rainier-bmc -nographic \
 -kernel ${IMAGEPATH}/fitImage-linux.bin \
 -dtb ${IMAGEPATH}/aspeed-bmc-ibm-rainier.dtb \
 -initrd ${IMAGEPATH}/obmc-phosphor-initramfs.rootfs.cpio.xz \
 -drive 
file=${IMAGEPATH}/obmc-phosphor-image.rootfs.wic.qcow2,if=sd,index=2 \
 -net nic -net 
user,hostfwd=:127.0.0.1:-:22,hostfwd=:127.0.0.1:2443-:443 \
 -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
 -tpmdev emulator,id=tpm0,chardev=chrtpm \
 -device tpm-tis-i2c,tpmdev=tpm0,bus=aspeed.i2c.bus.12,address=0x2e

Signed-off-by: Ninad Palsule 
---
V2:
Incorporated Stephen's review comments.
- Handled checksum related register in I2C layer
- Defined I2C interface capabilities and return those instead of
   capabilities from TPM TIS. Add required capabilities from TIS.
- Do not cache FIFO data in the I2C layer.
- Make sure that Device address change register is not passed to I2C
   layer as capability indicate that it is not supported.
- Added boundary checks.
- Make sure that bits 26-31 are zeroed for the TPM_STS register on read
- Updated Kconfig files for new define.

---
V3:
- Moved processing of register TPM_I2C_LOC_SEL in the I2C. So I2C layer
   remembers the locality and pass it to TIS on each read/write.
- The write data is no more cached in the I2C layer so the buffer size
   is reduced to 16 bytes.
- Checksum registers are now managed by the I2C layer. Added new
   function in TIS layer to return the checksum and used that to process
   the request.
- Now 2-4 byte register value will be passed to TIS layer in a single
   write call instead of 1 byte at a time. Added functions to convert
   between little endian stream of bytes to single 32 bit unsigned
   integer. Similarly 32  bit integer to stream of bytes.
- Added restriction on device change register.
- Replace few if-else statement with switch statement for clarity.
- Log warning when unknown register is received.
- Moved all register definations to acpi/tmp.h

---
V4:
Incorporated review comments from Cedric and Stefan.
- Reduced data[] size from 16 byte to 5 bytes.
- Added register name in the mapping table which can be used for
   tracing.
- Removed the endian conversion functions instead used simple logic
   provided by Stefan.
- Rename I2C registers to reduce the length.
- Added traces for send, recv and event functions. You can turn on trace
   on command line by using "-trace "tpm_tis_i2c*" option.

---
V5:
Fixed issues reported by Stefan's test.
- Added mask for the INT_ENABLE register.
- Use correct TIS register for reading interrupt capability.
- Cleanup how register is converted from I2C to TIS and also saved
   information like tis_addr and register name in the i2cst so that we
   can only convert it once on i2c_send.
- Trace register number for unknown registers.

---
V6:
Fixed review comments from Stefan.
- Fixed some variable size.
- Removed unused variables.
- Added vmstat backin to handle migration.
- Added post load phase to reload tis address and register name.
---
  hw/arm/Kconfig   |   1 +
  hw/tpm/Kconfig   |   7 +
  hw/tpm/meson.build   |   1 +
  hw/tpm/tpm_tis_i2c.c | 533 +++
  hw/tpm/trace-events  |   6 +
  include/sysemu/tpm.h |   3 +
  6 files changed, 551 insertions(+)
  create mode 100644 hw/tpm/tpm_tis_i2c.c

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index b5aed4aff5..05d6ef1a31 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -6,6 +6,7 @@ config ARM_VIRT
  imply VFIO_PLATFORM
  imply VFIO_XGMAC
  imply TPM_TIS_SYSBUS
+imply TPM_TIS_I2C
  imply NVDIMM
  select ARM_GIC
  select ACPI
diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
index 29e82f3c92..a46663288c 100644
--- a/hw/tpm/Kconfig
+++ b/hw/tpm/Kconfig
@@ -1,3 +1,10 @@
+config TPM_TIS_I2C
+bool
+depends on TPM
+select TPM_BACKEND
+select I2C
+select TPM_TIS
+
  config TPM_TIS_ISA
  bool
  depends on TPM && ISA_BUS
diff --git a/hw/tpm/meson.build b/hw

Re: [PATCH v6 00/25] target/riscv: MSTATUS_SUM + cleanups

[liweiwei]

On 2023/3/25 18:54, Richard Henderson wrote:

This builds on Fei and Zhiwei's SUM and TB_FLAGS changes.

   * Reclaim 5 TB_FLAGS bits, since we nearly ran out.

   * Using cpu_mmu_index(env, true) is insufficient to implement
 HLVX properly.  While that chooses the correct mmu_idx, it
 does not perform the read with execute permission.
 I add a new tcg interface to perform a read-for-execute with
 an arbitrary mmu_idx.  This is still not 100% compliant, but
 it's closer.

   * Handle mstatus.MPV in cpu_mmu_index.
   * Use vsstatus.SUM when required for MMUIdx_S_SUM.
   * Cleanups for get_physical_address.

While this passes check-avocado, I'm sure that's insufficient.
Please have a close look.


r~


Fei Wu (2):
   target/riscv: Separate priv from mmu_idx
   target/riscv: Reduce overhead of MSTATUS_SUM change

LIU Zhiwei (4):
   target/riscv: Extract virt enabled state from tb flags
   target/riscv: Add a general status enum for extensions
   target/riscv: Encode the FS and VS on a normal way for tb flags
   target/riscv: Add a tb flags field for vstart

Richard Henderson (19):
   target/riscv: Remove mstatus_hs_{fs,vs} from tb_flags
   accel/tcg: Add cpu_ld*_code_mmu
   target/riscv: Use cpu_ld*_code_mmu for HLVX
   target/riscv: Handle HLV, HSV via helpers
   target/riscv: Rename MMU_HYP_ACCESS_BIT to MMU_2STAGE_BIT
   target/riscv: Introduce mmuidx_sum
   target/riscv: Introduce mmuidx_priv
   target/riscv: Introduce mmuidx_2stage
   target/riscv: Move hstatus.spvp check to check_access_hlsv
   target/riscv: Set MMU_2STAGE_BIT in riscv_cpu_mmu_index
   target/riscv: Check SUM in the correct register
   target/riscv: Hoist second stage mode change to callers
   target/riscv: Hoist pbmte and hade out of the level loop
   target/riscv: Move leaf pte processing out of level loop
   target/riscv: Suppress pte update with is_debug
   target/riscv: Don't modify SUM with is_debug
   target/riscv: Merge checks for reserved pte flags
   target/riscv: Reorg access check in get_physical_address
   target/riscv: Reorg sum check in get_physical_address

  include/exec/cpu_ldst.h   |   9 +
  target/riscv/cpu.h|  47 ++-
  target/riscv/cpu_bits.h   |  12 +-
  target/riscv/helper.h |  12 +-
  target/riscv/internals.h  |  35 ++
  accel/tcg/cputlb.c|  48 +++
  accel/tcg/user-exec.c |  58 +++
  target/riscv/cpu.c|   2 +-
  target/riscv/cpu_helper.c | 393 +-
  target/riscv/csr.c|  21 +-
  target/riscv/op_helper.c  | 113 -
  target/riscv/translate.c  |  72 ++--
  .../riscv/insn_trans/trans_privileged.c.inc   |   2 +-
  target/riscv/insn_trans/trans_rvf.c.inc   |   2 +-
  target/riscv/insn_trans/trans_rvh.c.inc   | 135 +++---
  target/riscv/insn_trans/trans_rvv.c.inc   |  22 +-
  target/riscv/insn_trans/trans_xthead.c.inc|   7 +-
  17 files changed, 595 insertions(+), 395 deletions(-)


This patchset is LGTM.  Patch 16 seems fix a bug in the two_stage 
parameter of raise_mmu_exception


called when translation fails,  it didn't take MPV into consideration in 
original implementation.


Reviewed-by: Weiwei Li 

Weiwei Li

Re: [PATCH 1/1] target/riscv: Convert env->virt to a bool env->virt_enabled

[liweiwei]

On 2023/3/25 22:53, LIU Zhiwei wrote:

Currently we only use the env->virt to encode the virtual mode enabled
status. Let's make it a bool type.

Signed-off-by: LIU Zhiwei 
---


I'm not quite sure the original reason to use a int for virt.

However, this change is acceptable to me.

Reviewed-by: Weiwei Li 

Weiwei Li


  target/riscv/cpu.h| 2 +-
  target/riscv/cpu_bits.h   | 3 ---
  target/riscv/cpu_helper.c | 6 +++---
  target/riscv/machine.c| 6 +++---
  target/riscv/translate.c  | 4 ++--
  5 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..3c8041c5a4 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -183,7 +183,7 @@ struct CPUArchState {
  #ifndef CONFIG_USER_ONLY
  target_ulong priv;
  /* This contains QEMU specific information about the virt state. */
-target_ulong virt;
+bool virt_enabled;
  target_ulong geilen;
  uint64_t resetvec;
  
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h

index fca7ef0cef..45ddb00aa5 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -607,9 +607,6 @@ typedef enum {
  #define PRV_H 2 /* Reserved */
  #define PRV_M 3
  
-/* Virtulisation Register Fields */

-#define VIRT_ONOFF  1
-
  /* RV32 satp CSR field masks */
  #define SATP32_MODE 0x8000
  #define SATP32_ASID 0x7fc0
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f88c503cf4..56f4ff9ccc 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -564,7 +564,7 @@ bool riscv_cpu_virt_enabled(CPURISCVState *env)
  return false;
  }
  
-return get_field(env->virt, VIRT_ONOFF);

+return env->virt_enabled;
  }
  
  void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable)

@@ -574,11 +574,11 @@ void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool 
enable)
  }
  
  /* Flush the TLB on all virt mode changes. */

-if (get_field(env->virt, VIRT_ONOFF) != enable) {
+if (env->virt_enabled != enable) {
  tlb_flush(env_cpu(env));
  }
  
-env->virt = set_field(env->virt, VIRT_ONOFF, enable);

+env->virt_enabled = enable;
  
  if (enable) {

  /*
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 9c455931d8..0fb3ddda06 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -331,8 +331,8 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
  
  const VMStateDescription vmstate_riscv_cpu = {

  .name = "cpu",
-.version_id = 7,
-.minimum_version_id = 7,
+.version_id = 8,
+.minimum_version_id = 8,
  .post_load = riscv_cpu_post_load,
  .fields = (VMStateField[]) {
  VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
@@ -352,7 +352,7 @@ const VMStateDescription vmstate_riscv_cpu = {
  VMSTATE_UINT32(env.misa_mxl_max, RISCVCPU),
  VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU),
  VMSTATE_UINTTL(env.priv, RISCVCPU),
-VMSTATE_UINTTL(env.virt, RISCVCPU),
+VMSTATE_BOOL(env.virt_enabled, RISCVCPU),
  VMSTATE_UINT64(env.resetvec, RISCVCPU),
  VMSTATE_UINTTL(env.mhartid, RISCVCPU),
  VMSTATE_UINT64(env.mstatus, RISCVCPU),
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 0ee8ee147d..c3adf30b54 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1255,8 +1255,8 @@ static void riscv_tr_disas_log(const DisasContextBase 
*dcbase,
  
  fprintf(logfile, "IN: %s\n", lookup_symbol(dcbase->pc_first));

  #ifndef CONFIG_USER_ONLY
-fprintf(logfile, "Priv: "TARGET_FMT_ld"; Virt: "TARGET_FMT_ld"\n",
-env->priv, env->virt);
+fprintf(logfile, "Priv: "TARGET_FMT_ld"; Virt: %d\n",
+env->priv, env->virt_enabled);
  #endif
  target_disas(logfile, cpu, dcbase->pc_first, dcbase->tb->size);
  }

Re: s390 migration crash

[Peter Xu]

On Wed, Mar 22, 2023 at 03:16:23PM -0400, Peter Xu wrote:
> On Wed, Mar 22, 2023 at 06:13:43PM +, Dr. David Alan Gilbert wrote:
> > * Peter Xu (pet...@redhat.com) wrote:
> > > On Wed, Mar 22, 2023 at 02:05:06PM +, Dr. David Alan Gilbert wrote:
> > > > * Peter Xu (pet...@redhat.com) wrote:
> > > > > On Tue, Mar 21, 2023 at 08:24:37PM +, Dr. David Alan Gilbert 
> > > > > wrote:
> > > > > > Hi Peter's,
> > > > > >   Peter M pointed me to a seg in a migration test in CI; I can 
> > > > > > reproduce
> > > > > > it:
> > > > > >   * On an s390 host
> > > > > 
> > > > > How easy to reproduce?
> > > > 
> > > > Pretty much every time when run as:
> > > > make check -j 4
> > > > 
> > > > > >   * only as part of a make check - running migration-test by itself
> > > > > > doesn't trigger for me.
> > > > > >   * It looks like it's postcopy preempt
> > > > > > 
> > > > > > (gdb) bt full
> > > > > > #0  iov_size (iov=iov@entry=0x2aa00e60670, iov_cnt=) 
> > > > > > at ../util/iov.c:88
> > > > > > len = 13517923312037845750
> > > > > > i = 17305
> > > > > > #1  0x02aa004d068c in qemu_fflush (f=0x2aa00e58630) at 
> > > > > > ../migration/qemu-file.c:307
> > > > > > local_error = 0x0
> > > > > > #2  0x02aa004d0e04 in qemu_fflush (f=) at 
> > > > > > ../migration/qemu-file.c:297
> > > > > > #3  0x02aa00613962 in postcopy_preempt_shutdown_file 
> > > > > > (s=s@entry=0x2aa00d1b4e0) at ../migration/ram.c:4657
> > > > > > #4  0x02aa004e12b4 in migration_completion (s=0x2aa00d1b4e0) at 
> > > > > > ../migration/migration.c:3469
> > > > > > ret = 
> > > > > > current_active_state = 5
> > > > > > must_precopy = 0
> > > > > > can_postcopy = 0
> > > > > > in_postcopy = true
> > > > > > pending_size = 0
> > > > > > __func__ = "migration_iteration_run"
> > > > > > iter_state = 
> > > > > > s = 0x2aa00d1b4e0
> > > > > > thread = 
> > > > > > setup_start = 
> > > > > > thr_error = 
> > > > > > urgent = 
> > > > > > #5  migration_iteration_run (s=0x2aa00d1b4e0) at 
> > > > > > ../migration/migration.c:3882
> > > > > > must_precopy = 0
> > > > > > can_postcopy = 0
> > > > > > in_postcopy = true
> > > > > > pending_size = 0
> > > > > > __func__ = "migration_iteration_run"
> > > > > > iter_state = 
> > > > > > s = 0x2aa00d1b4e0
> > > > > > thread = 
> > > > > > setup_start = 
> > > > > > thr_error = 
> > > > > > urgent = 
> > > > > > #6  migration_thread (opaque=opaque@entry=0x2aa00d1b4e0) at 
> > > > > > ../migration/migration.c:4124
> > > > > > iter_state = 
> > > > > > s = 0x2aa00d1b4e0
> > > > > > --Type  for more, q to quit, c to continue without paging--
> > > > > > thread = 
> > > > > > setup_start = 
> > > > > > thr_error = 
> > > > > > urgent = 
> > > > > > #7  0x02aa00819b8c in qemu_thread_start (args=) 
> > > > > > at ../util/qemu-thread-posix.c:541
> > > > > > __cancel_buf = 
> > > > > > {__cancel_jmp_buf = {{__cancel_jmp_buf = {{__gregs = 
> > > > > > {4396782422080, 4393751543808, 4397299389454, 4396844235904, 
> > > > > > 2929182727824, 2929182933488, 4396843986792, 4397299389455, 
> > > > > > 33679382915066768, 33678512846981306}, __fpregs = {4396774031360, 
> > > > > > 8392704, 2929182933488, 0, 4396782422272, 2929172491858, 
> > > > > > 4396774031360, 1}}}, __mask_was_saved = 0}}, __pad = 
> > > > > > {0x3ffb4a77a60, 0x0, 0x0, 0x0}}
> > > > > > __cancel_routine = 0x2aa00819bf0 
> > > > > > __not_first_call = 
> > > > > > start_routine = 0x2aa004e08f0 
> > > > > > arg = 0x2aa00d1b4e0
> > > > > > r = 
> > > > > > #8  0x03ffb7b1e2e6 in start_thread () at /lib64/libc.so.6
> > > > > > #9  0x03ffb7aafdbe in thread_start () at /lib64/libc.so.6
> > > > > > 
> > > > > > It looks like it's in the preempt test:
> > > > > > 
> > > > > > (gdb) where
> > > > > > #0  0x03ffb17a0126 in __pthread_kill_implementation () from 
> > > > > > /lib64/libc.so.6
> > > > > > #1  0x03ffb1750890 in raise () from /lib64/libc.so.6
> > > > > > #2  0x03ffb172a340 in abort () from /lib64/libc.so.6
> > > > > > #3  0x02aa0041c130 in qtest_check_status (s=) at 
> > > > > > ../tests/qtest/libqtest.c:194
> > > > > > #4  0x03ffb1a3b5de in g_hook_list_invoke () from 
> > > > > > /lib64/libglib-2.0.so.0
> > > > > > #5  
> > > > > > #6  0x03ffb17a0126 in __pthread_kill_implementation () from 
> > > > > > /lib64/libc.so.6
> > > > > > #7  0x03ffb1750890 in raise () from /lib64/libc.so.6
> > > > > > #8  0x03ffb172a340 in abort () from /lib64/libc.so.6
> > > > > > #9  0x02aa00420318 in qmp_fd_receive (fd=) at 
> > > > > > ../tests/qtest/libqmp.c:80
> > > > > > #10 0x02aa0041d5ee in qtest_qmp_receive_dict (s=0x2aa01eb2700) 
> > > > > > at ../tests/qtest/libqtest.c:713
> > > 

[PATCH for-8.0 1/3] io: tls: Inherit QIO_CHANNEL_FEATURE_SHUTDOWN on server side

[Peter Xu]

TLS iochannel will inherit io_shutdown() from the master ioc, however we
missed to do that on the server side.

This will e.g. allow qemu_file_shutdown() to work on dest QEMU too for
migration.

Acked-by: Daniel P. Berrangé 
Signed-off-by: Peter Xu 
---
 io/channel-tls.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/io/channel-tls.c b/io/channel-tls.c
index 5a7a3d48d6..9805dd0a3f 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -74,6 +74,9 @@ qio_channel_tls_new_server(QIOChannel *master,
 ioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
 
 ioc->master = master;
+if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
+qio_channel_set_feature(QIO_CHANNEL(ioc), 
QIO_CHANNEL_FEATURE_SHUTDOWN);
+}
 object_ref(OBJECT(master));
 
 ioc->session = qcrypto_tls_session_new(
-- 
2.39.1

[PATCH for-8.0 0/3] migration: Preempt mode fixes for 8.0

[Peter Xu]

I'm proposing this patchset for current 8.0 rc material.  If we decide to
merge this we should merge this sooner the better..  If we leave this for
next release then 8.0 release will break with preempt migrations to 7.2-
binaries, then also we need to copy stable when merged.  Currently no
stable backport needed if this can land 8.0 (I still attached Fixes for
each of them just to mark out the issue commits, though).

This patchset target to fix two issues for preempt mode (both of them
should be introduced during 8.0 dev cycle):

(1) The rare s390 crash reported by Peter Maydell and later on reproduced
by Dave (thanks!), see  on qemu-devel.

(2) Preempt mode migration breakage from 8.0 -> pre-7.2 binaries

The 2nd issue was what I found when testing (1), so I think (2) is even
more severe because it constantly breaks migration from new->old binaries,
depending on how much we care about that.

Patch 1 was a pre-requisite of patch 2 on enabling shutdown() to work on
TLS sockets even on the server side.  Should be something we just
overlooked when having the tls code merged but just never bother because we
never used the server side shutdowns.

Patch 2 targets to fix issue (1).  Dave, I didn't yet attach your T-b due
to the flag change.  I think it'll work the same as old, though.

Patch 3 fixes issue (2) which I checked myself along with patch 2.

Logically patch 1 is not bugfix but still a dependency and I see it low
risk to merge even during rc release cycles.  But please reviewers justify
in case for whatever reason this set is not suitable for 8.0 anymore.

Tests I've done with the whole set applied:

- qtests
- different binary tests for preempt, majorly:
  - 8.0 -> 8.0
  - 7.2 -> 8.0 (mach=pc-q35-7.2)
  - 8.0 -> 7.2 (mach=pc-q35-7.2)

Thanks,

Peter Xu (3):
  io: tls: Inherit QIO_CHANNEL_FEATURE_SHUTDOWN on server side
  migration: Fix potential race on postcopy_qemufile_src
  migration: Recover behavior of preempt channel creation for pre-7.2

 hw/core/machine.c|  1 +
 io/channel-tls.c |  3 +++
 migration/migration.c| 19 +--
 migration/migration.h| 41 +++-
 migration/postcopy-ram.c | 30 ++---
 5 files changed, 84 insertions(+), 10 deletions(-)

-- 
2.39.1

[PATCH for-8.0 3/3] migration: Recover behavior of preempt channel creation for pre-7.2

[Peter Xu]

In 8.0 devel window we reworked preempt channel creation, so that there'll
be no race condition when the migration channel and preempt channel got
established in the wrong order in commit 5655aab079.

However no one noticed that the change will also be not compatible with
older qemus, majorly 7.1/7.2 versions where preempt mode started to be
supported.

Leverage the same pre-7.2 flag introduced in the previous patch to recover
the behavior hopefully before 8.0 releases, so we don't break migration
when we migrate from 8.0 to older qemu binaries.

Fixes: 5655aab079 ("migration: Postpone postcopy preempt channel to be after 
main")
Signed-off-by: Peter Xu 
---
 migration/migration.c|  9 +
 migration/migration.h|  7 +++
 migration/postcopy-ram.c | 10 --
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 37fc4fb3e2..bda4789193 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -4388,6 +4388,15 @@ void migrate_fd_connect(MigrationState *s, Error 
*error_in)
 }
 }
 
+/*
+ * This needs to be done before resuming a postcopy.  Note: for newer
+ * QEMUs we will delay the channel creation until postcopy_start(), to
+ * avoid disorder of channel creations.
+ */
+if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
+postcopy_preempt_setup(s);
+}
+
 if (resume) {
 /* Wakeup the main migration thread to do the recovery */
 migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
diff --git a/migration/migration.h b/migration/migration.h
index 67baba2184..310ae8901b 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -384,12 +384,19 @@ struct MigrationState {
  * - postcopy preempt src QEMU instance will generate an EOS message at
  *   the end of migration to shut the preempt channel on dest side.
  *
+ * - postcopy preempt channel will be created at the setup phase on src
+ QEMU.
+ *
  * When clear:
  *
  * - postcopy preempt src QEMU instance will _not_ generate an EOS
  *   message at the end of migration, the dest qemu will shutdown the
  *   channel itself.
  *
+ * - postcopy preempt channel will be created at the switching phase
+ *   from precopy -> postcopy (to avoid race condtion of misordered
+ *   creation of channels).
+ *
  * NOTE: See message-id  on qemu-devel
  * mailing list for more information on the possible race.  Everyone
  * should probably just keep this value untouched after set by the
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 263bab75ec..93f39f8e06 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1630,8 +1630,14 @@ int postcopy_preempt_establish_channel(MigrationState *s)
 return 0;
 }
 
-/* Kick off async task to establish preempt channel */
-postcopy_preempt_setup(s);
+/*
+ * Kick off async task to establish preempt channel.  Only do so with
+ * 8.0+ machines, because 7.1/7.2 require the channel to be created in
+ * setup phase of migration (even if racy in an unreliable network).
+ */
+if (!s->preempt_pre_7_2) {
+postcopy_preempt_setup(s);
+}
 
 /*
  * We need the postcopy preempt channel to be established before
-- 
2.39.1

[PATCH for-8.0 2/3] migration: Fix potential race on postcopy_qemufile_src

[Peter Xu]

postcopy_qemufile_src object should be owned by one thread, either the main
thread (e.g. when at the beginning, or at the end of migration), or by the
return path thread (when during a preempt enabled postcopy migration).  If
that's not the case the access to the object might be racy.

postcopy_preempt_shutdown_file() can be potentially racy, because it's
called at the end phase of migration on the main thread, however during
which the return path thread hasn't yet been recycled; the recycle happens
in await_return_path_close_on_source() which is after this point.

It means, logically it's posslbe the main thread and the return path thread
are both operating on the same qemufile.  While I don't think qemufile is
thread safe at all.

postcopy_preempt_shutdown_file() used to be needed because that's where we
send EOS to dest so that dest can safely shutdown the preempt thread.

To avoid the possible race, remove this only place that a race can happen.
Instead we figure out another way to safely close the preempt thread on
dest.

The core idea during postcopy on deciding "when to stop" is that dest will
send a postcopy SHUT message to src, telling src that all data is there.
Hence to shut the dest preempt thread maybe better to do it directly on
dest node.

This patch proposed such a way that we change postcopy_prio_thread_created
into PreemptThreadStatus, so that we kick the preempt thread on dest qemu
by a sequence of:

  mis->preempt_thread_status = PREEMPT_THREAD_QUIT;
  qemu_file_shutdown(mis->postcopy_qemufile_dst);

While here shutdown() is probably so far the easiest way to kick preempt
thread from a blocked qemu_get_be64().  Then it reads preempt_thread_status
to make sure it's not a network failure but a willingness to quit the
thread.

We could have avoided that extra status but just rely on migration status.
The problem is postcopy_ram_incoming_cleanup() is just called early enough
so we're still during POSTCOPY_ACTIVE no matter what.. So just make it
simple to have the status introduced.

One flag x-preempt-pre-7-2 is added to keep old pre-7.2 behaviors of
postcopy preempt.

Fixes: 9358982744 ("migration: Send requested page directly in rp-return 
thread")
Signed-off-by: Peter Xu 
---
 hw/core/machine.c|  1 +
 migration/migration.c| 10 --
 migration/migration.h| 34 +-
 migration/postcopy-ram.c | 20 +++-
 4 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 45e3d24fdc..cd13b8b0a3 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -42,6 +42,7 @@
 GlobalProperty hw_compat_7_2[] = {
 { "e1000e", "migrate-timadj", "off" },
 { "virtio-mem", "x-early-migration", "false" },
+{ "migration", "x-preempt-pre-7-2", "true" },
 };
 const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
 
diff --git a/migration/migration.c b/migration/migration.c
index ae2025d9d8..37fc4fb3e2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3464,8 +3464,12 @@ static void migration_completion(MigrationState *s)
 qemu_savevm_state_complete_postcopy(s->to_dst_file);
 qemu_mutex_unlock_iothread();
 
-/* Shutdown the postcopy fast path thread */
-if (migrate_postcopy_preempt()) {
+/*
+ * Shutdown the postcopy fast path thread.  This is only needed
+ * when dest QEMU binary is old (7.1/7.2).  QEMU 8.0+ doesn't need
+ * this.
+ */
+if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
 postcopy_preempt_shutdown_file(s);
 }
 
@@ -4443,6 +4447,8 @@ static Property migration_properties[] = {
   decompress_error_check, true),
 DEFINE_PROP_UINT8("x-clear-bitmap-shift", MigrationState,
   clear_bitmap_shift, CLEAR_BITMAP_SHIFT_DEFAULT),
+DEFINE_PROP_BOOL("x-preempt-pre-7-2", MigrationState,
+ preempt_pre_7_2, false),
 
 /* Migration parameters */
 DEFINE_PROP_UINT8("x-compress-level", MigrationState,
diff --git a/migration/migration.h b/migration/migration.h
index 2da2f8a164..67baba2184 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -65,6 +65,12 @@ typedef struct {
 bool all_zero;
 } PostcopyTmpPage;
 
+typedef enum {
+PREEMPT_THREAD_NONE = 0,
+PREEMPT_THREAD_CREATED,
+PREEMPT_THREAD_QUIT,
+} PreemptThreadStatus;
+
 /* State for the incoming migration */
 struct MigrationIncomingState {
 QEMUFile *from_src_file;
@@ -124,7 +130,12 @@ struct MigrationIncomingState {
 QemuSemaphore postcopy_qemufile_dst_done;
 /* Postcopy priority thread is used to receive postcopy requested pages */
 QemuThread postcopy_prio_thread;
-bool postcopy_prio_thread_created;
+/*
+ * Always set by the main vm load thread only, but can be read by the
+ * postcopy preempt thread.  "volatile" makes sure all reads will be
+ * uptodate across cores.
+ */
+  

Re: Audio playback speed issue on sam460ex and pegasos2

[Volker Rümelin]

Am 19.03.23 um 21:03 schrieb Volker Rümelin:

Am 19.03.23 um 18:49 schrieb BALATON Zoltan:


Not sure this helps but I get these with DEBUG enabled in qemu/audio 
on Linux host with alsa set to 44100 Hz dmix rate with default 
settings without any -audiodev options with AmigaOS guest.

With pegasos2:

audio: open via-ac97.out, freq 44100, nchannels 1, fmt 
1##] 100 %

audio: open via-ac97.out, freq 44100, nchannels 2, fmt 3
alsa: enabling voice
alsa: disabling voice
alsa: alsa_fini

or pegasos2 with ES1370:

audio: open via-ac97.out, freq 44100, nchannels 2, fmt 3
alsa: enabling voice
alsa: disabling voice
alsa: alsa_fini

this does not play as slow as with sam460ex below but maybe a bit 
slow which seems to improve with try-poll=off so this may be because 
of the alsa backend issue. It's a bit faster with sdl backend, not 
sure if that's the right speed or too fast but at least the backend 
seems to influence playback speed.




Hi,

I still don't understand how the playback speed can slow down without 
changing the pitch. Is it possible that the guest can't provide the 
audio frames fast enough and there are buffer underruns in the ALSA 
backend? But I think you would hear buffer underruns.



With sam460ex and ES1370:

audio: open es1370.dac2, freq 44100, nchannels 1, fmt 0
audio: open es1370.adc, freq 44100, nchannels 1, fmt 0
audio: open es1370.dac2, freq 48662, nchannels 1, fmt 0
audio: open es1370.adc, freq 48662, nchannels 1, fmt 0
audio: open es1370.dac2, freq 48662, nchannels 2, fmt 3
alsa: enabling voice
alsa: disabling voice
alsa: alsa_fini

this plays definitely slow and the freq also seems to be off. I may 
have different AmigaOS versions on pegasos2 and sam460ex but I they 
seem to use the same driver as there were no updates to that part. 
I'm not sure what the driver in AmigaOS looks like but it may be 
similar to the AROS AHI SB128 one. I don't know if higher level parts 
in AHI may try to measure something like you mentioned but at least 
the card driver does not seem to do that.


I had a look at the AROS SB128 driver and the AHI Preferences code. 
There is no code to measure the audio clock frequency. The frequency 
selection of 48662Hz seems to be a AROS/AmigaOS bug. This log is from 
a AROS x86 guest. I hear some faint static noise but the playback 
speed is correct.


open pcspk, freq 32000, nchannels 1, fmt 0
open es1370.dac2, freq 44100, nchannels 1, fmt 0
open es1370.adc, freq 44100, nchannels 1, fmt 0
open es1370.dac2, freq 48662, nchannels 1, fmt 0
open es1370.adc, freq 48662, nchannels 1, fmt 0
open es1370.dac2, freq 48662, nchannels 2, fmt 3
open es1370.dac2, freq 44100, nchannels 2, fmt 3
open es1370.adc, freq 44100, nchannels 1, fmt 0



There seems to be two independent problems, one is the bug in alsa 
backend that you mentioned and something else only affecting sam460ex 
which causes the wrong freq to be selected but I have no idea why or 
what to check further to find out. 


I'm not so sure if your analysis is correct. The ALSA error I 
mentioned can significantly increase the processor load, sometimes to 
the point of stopping the guest. But it can't directly change the 
playback speed.


I will write a patch with a few tracepoints for the audio system. With 
your help it should be possible to find the cause of the problem. It 
might take me a few days to write and test the patch.


With best regards,
Volker


Hi Zoltan,

I have written a few patches to add audio tracepoints. I pushed my 
audio-tracepoints branch to https://gitlab.com/volker.ruemelin/qemu.git.


For useful audio trace files QEMU has to be compiled with 
--enable-trace-backends=simple to enable the "simple" trace backend. The 
command line options  -trace "audio_open*_out" -trace 
"audio_*_frames_out" -trace file=/tmp/qemu-trace enable audio playback 
tracing. To view the trace start ./scripts/simpletrace.py 
trace/trace-events-all /tmp/qemu-trace | less in the QEMU build directory.


Here is an example of audio playback of a mp3 file with mplayer in a x86 
AROS guest. I used -device ES1370,audiodev=audio0 -audiodev 
alsa,id=audio0,out.try-poll=off,out.dev=hw:PCH,,0,in.try-poll=off,in.dev=hw:PCH,,0


audio_open_out 0.000 pid=8798 card=b'es1370' name=b'es1370.dac2' 
freq=0xac44 fmt=b'u8' ch=0x1
audio_open_info_out 249624.597 pid=8798 end=b'sw' card=b'es1370' 
name=b'es1370.dac2' freq=0xac44 ch=0x1 bits=0x8 is_signed=0x0 is_fl

oat=0x0
audio_open_info_out 0.853 pid=8798 end=b'hw' card=b'es1370' 
name=b'es1370.dac2' freq=0xac44 ch=0x2 bits=0x10 is_signed=0x1 is_float=

0x0
audio_open_out 52404.954 pid=8798 card=b'es1370' name=b'es1370.dac2' 
freq=0xac44 fmt=b's16' ch=0x2
audio_open_info_out 10.985 pid=8798 end=b'sw' card=b'es1370' 
name=b'es1370.dac2' freq=0xac44 ch=0x2 bits=0x10 is_signed=0x1 is_float

=0x0
audio_open_info_out 0.606 pid=8798 end=b'hw' card=b'es1370' 
name=b'es1370.dac2' freq=0xac44 ch=0x2 bits=0x10 is_signed=0x1 is_float=

0x0
audio_fe_frames_out 10145.216 pid=8798 fe_free=0

Re: [PATCH v2 1/4] hw/watchdog: Allwinner WDT emulation for system reset

[Niek Linnenbank]

Hi Strahinja,




On Fri, Mar 17, 2023 at 1:13 AM Strahinja Jankovic <
strahinjapjanko...@gmail.com> wrote:

> This patch adds basic support for Allwinner WDT.
> Both sun4i and sun6i variants are supported.
> However, interrupt generation is not supported, so WDT can be used only to
> trigger system reset.
>
> Signed-off-by: Strahinja Jankovic 
>
> ---
>  hw/watchdog/Kconfig |   4 +
>  hw/watchdog/allwinner-wdt.c | 416 
>  hw/watchdog/meson.build |   1 +
>  hw/watchdog/trace-events|   7 +
>  include/hw/watchdog/allwinner-wdt.h | 123 
>  5 files changed, 551 insertions(+)
>  create mode 100644 hw/watchdog/allwinner-wdt.c
>  create mode 100644 include/hw/watchdog/allwinner-wdt.h
>
> diff --git a/hw/watchdog/Kconfig b/hw/watchdog/Kconfig
> index 66e1d029e3..861fd00334 100644
> --- a/hw/watchdog/Kconfig
> +++ b/hw/watchdog/Kconfig
> @@ -20,3 +20,7 @@ config WDT_IMX2
>
>  config WDT_SBSA
>  bool
> +
> +config ALLWINNER_WDT
> +bool
> +select PTIMER
> diff --git a/hw/watchdog/allwinner-wdt.c b/hw/watchdog/allwinner-wdt.c
> new file mode 100644
> index 00..45a4a36ba7
> --- /dev/null
> +++ b/hw/watchdog/allwinner-wdt.c
> @@ -0,0 +1,416 @@
> +/*
> + * Allwinner Watchdog emulation
> + *
> + * Copyright (C) 2023 Strahinja Jankovic 
> + *
> + *  This file is derived from Allwinner RTC,
> + *  by Niek Linnenbank.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/units.h"
> +#include "qemu/module.h"
> +#include "trace.h"
> +#include "hw/sysbus.h"
> +#include "hw/registerfields.h"
> +#include "hw/watchdog/allwinner-wdt.h"
> +#include "sysemu/watchdog.h"
> +#include "migration/vmstate.h"
> +
> +/* WDT registers */
> +enum {
> +REG_IRQ_EN = 0, /* Watchdog interrupt enable */
> +REG_IRQ_STA,/* Watchdog interrupt status */
> +REG_CTRL,   /* Watchdog control register */
> +REG_CFG,/* Watchdog configuration register */
> +REG_MODE,   /* Watchdog mode register */
> +};
> +
> +/* Universal WDT register flags */
> +#define WDT_RESTART_MASK(1 << 0)
> +#define WDT_EN_MASK (1 << 0)
> +
> +/* sun4i specific WDT register flags */
> +#define RST_EN_SUN4I_MASK   (1 << 1)
> +#define INTV_VALUE_SUN4I_SHIFT  (3)
> +#define INTV_VALUE_SUN4I_MASK   (0xfu << INTV_VALUE_SUN4I_SHIFT)
> +
> +/* sun6i specific WDT register flags */
> +#define RST_EN_SUN6I_MASK   (1 << 0)
> +#define KEY_FIELD_SUN6I_SHIFT   (1)
> +#define KEY_FIELD_SUN6I_MASK(0xfffu << KEY_FIELD_SUN6I_SHIFT)
> +#define KEY_FIELD_SUN6I (0xA57u)
> +#define INTV_VALUE_SUN6I_SHIFT  (4)
> +#define INTV_VALUE_SUN6I_MASK   (0xfu << INTV_VALUE_SUN6I_SHIFT)
> +
> +/* Map of INTV_VALUE to 0.5s units. */
> +static const uint8_t allwinner_wdt_count_map[] = {
> +1,
> +2,
> +4,
> +6,
> +8,
> +10,
> +12,
> +16,
> +20,
> +24,
> +28,
> +32
> +};
> +
> +/* WDT sun4i register map (offset to name) */
> +const uint8_t allwinner_wdt_sun4i_regmap[] = {
> +[0x] = REG_CTRL,
> +[0x0004] = REG_MODE,
> +};
> +
> +/* WDT sun6i register map (offset to name) */
> +const uint8_t allwinner_wdt_sun6i_regmap[] = {
> +[0x] = REG_IRQ_EN,
> +[0x0004] = REG_IRQ_STA,
> +[0x0010] = REG_CTRL,
> +[0x0014] = REG_CFG,
> +[0x0018] = REG_MODE,
> +};
> +
> +static bool allwinner_wdt_sun4i_read(AwWdtState *s, uint32_t offset)
> +{
> +/* no sun4i specific registers currently implemented */
> +return false;
> +}
> +
> +static bool allwinner_wdt_sun4i_write(AwWdtState *s, uint32_t offset,
> +  uint32_t data)
> +{
> +/* no sun4i specific registers currently implemented */
> +return false;
> +}
> +
> +static bool allwinner_wdt_sun4i_can_reset_system(AwWdtState *s)
> +{
> +if (s->regs[REG_MODE] & RST_EN_SUN6I_MASK) {
>

Should this function use the RST_EN_SUN4I_MASK instead?


> +return true;
> +} else {
> +return false;
> +}
> +}
> +
> +static bool allwinner_wdt_sun4i_is_key_valid(AwWdtState *s, uint32_t val)
> +{
> +/* sun4i has no key */
> +return true;
> +}
> +
> +static uint8_t allwinner_wdt_sun4i_get_intv_value(AwWdtState *s)
> +{
> +return ((s->regs[REG_MODE] 

Re: [PATCH v2 1/4] hw/watchdog: Allwinner WDT emulation for system reset

[Strahinja Jankovic]

Hi Niek,

On Sun, Mar 26, 2023 at 9:04 PM Niek Linnenbank
 wrote:
>
> Hi Strahinja,
>
>
>
>
> On Fri, Mar 17, 2023 at 1:13 AM Strahinja Jankovic 
>  wrote:
>>
>> This patch adds basic support for Allwinner WDT.
>> Both sun4i and sun6i variants are supported.
>> However, interrupt generation is not supported, so WDT can be used only to 
>> trigger system reset.
>>
>> Signed-off-by: Strahinja Jankovic 
>>
>> ---
>>  hw/watchdog/Kconfig |   4 +
>>  hw/watchdog/allwinner-wdt.c | 416 
>>  hw/watchdog/meson.build |   1 +
>>  hw/watchdog/trace-events|   7 +
>>  include/hw/watchdog/allwinner-wdt.h | 123 
>>  5 files changed, 551 insertions(+)
>>  create mode 100644 hw/watchdog/allwinner-wdt.c
>>  create mode 100644 include/hw/watchdog/allwinner-wdt.h
>>
>> diff --git a/hw/watchdog/Kconfig b/hw/watchdog/Kconfig
>> index 66e1d029e3..861fd00334 100644
>> --- a/hw/watchdog/Kconfig
>> +++ b/hw/watchdog/Kconfig
>> @@ -20,3 +20,7 @@ config WDT_IMX2
>>
>>  config WDT_SBSA
>>  bool
>> +
>> +config ALLWINNER_WDT
>> +bool
>> +select PTIMER
>> diff --git a/hw/watchdog/allwinner-wdt.c b/hw/watchdog/allwinner-wdt.c
>> new file mode 100644
>> index 00..45a4a36ba7
>> --- /dev/null
>> +++ b/hw/watchdog/allwinner-wdt.c
>> @@ -0,0 +1,416 @@
>> +/*
>> + * Allwinner Watchdog emulation
>> + *
>> + * Copyright (C) 2023 Strahinja Jankovic 
>> + *
>> + *  This file is derived from Allwinner RTC,
>> + *  by Niek Linnenbank.
>> + *
>> + * This program is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qemu/units.h"
>> +#include "qemu/module.h"
>> +#include "trace.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/registerfields.h"
>> +#include "hw/watchdog/allwinner-wdt.h"
>> +#include "sysemu/watchdog.h"
>> +#include "migration/vmstate.h"
>> +
>> +/* WDT registers */
>> +enum {
>> +REG_IRQ_EN = 0, /* Watchdog interrupt enable */
>> +REG_IRQ_STA,/* Watchdog interrupt status */
>> +REG_CTRL,   /* Watchdog control register */
>> +REG_CFG,/* Watchdog configuration register */
>> +REG_MODE,   /* Watchdog mode register */
>> +};
>> +
>> +/* Universal WDT register flags */
>> +#define WDT_RESTART_MASK(1 << 0)
>> +#define WDT_EN_MASK (1 << 0)
>> +
>> +/* sun4i specific WDT register flags */
>> +#define RST_EN_SUN4I_MASK   (1 << 1)
>> +#define INTV_VALUE_SUN4I_SHIFT  (3)
>> +#define INTV_VALUE_SUN4I_MASK   (0xfu << INTV_VALUE_SUN4I_SHIFT)
>> +
>> +/* sun6i specific WDT register flags */
>> +#define RST_EN_SUN6I_MASK   (1 << 0)
>> +#define KEY_FIELD_SUN6I_SHIFT   (1)
>> +#define KEY_FIELD_SUN6I_MASK(0xfffu << KEY_FIELD_SUN6I_SHIFT)
>> +#define KEY_FIELD_SUN6I (0xA57u)
>> +#define INTV_VALUE_SUN6I_SHIFT  (4)
>> +#define INTV_VALUE_SUN6I_MASK   (0xfu << INTV_VALUE_SUN6I_SHIFT)
>> +
>> +/* Map of INTV_VALUE to 0.5s units. */
>> +static const uint8_t allwinner_wdt_count_map[] = {
>> +1,
>> +2,
>> +4,
>> +6,
>> +8,
>> +10,
>> +12,
>> +16,
>> +20,
>> +24,
>> +28,
>> +32
>> +};
>> +
>> +/* WDT sun4i register map (offset to name) */
>> +const uint8_t allwinner_wdt_sun4i_regmap[] = {
>> +[0x] = REG_CTRL,
>> +[0x0004] = REG_MODE,
>> +};
>> +
>> +/* WDT sun6i register map (offset to name) */
>> +const uint8_t allwinner_wdt_sun6i_regmap[] = {
>> +[0x] = REG_IRQ_EN,
>> +[0x0004] = REG_IRQ_STA,
>> +[0x0010] = REG_CTRL,
>> +[0x0014] = REG_CFG,
>> +[0x0018] = REG_MODE,
>> +};
>> +
>> +static bool allwinner_wdt_sun4i_read(AwWdtState *s, uint32_t offset)
>> +{
>> +/* no sun4i specific registers currently implemented */
>> +return false;
>> +}
>> +
>> +static bool allwinner_wdt_sun4i_write(AwWdtState *s, uint32_t offset,
>> +  uint32_t data)
>> +{
>> +/* no sun4i specific registers currently implemented */
>> +return false;
>> +}
>> +
>> +static bool allwinner_wdt_sun4i_can_reset_system(AwWdtState *s)
>> +{
>> +if (s->regs[REG_MODE] & RST_EN_SUN6I_MASK) {
>
>
> Should this function use the RST_EN_SUN4I_MASK instead?

Thanks for reviewing this. You are correct, it should be the SUN4I_MASK.
I will update this and send v3.

>
>>
>> +return true;
>

[PATCH v3 2/4] hw/arm: Add WDT to Allwinner-A10 and Cubieboard

[Strahinja Jankovic]

This patch adds WDT to Allwinner-A10 and Cubieboard.
WDT is added as an overlay to the Timer module memory map.

Signed-off-by: Strahinja Jankovic 

Reviewed-by: Niek Linnenbank 
---
 docs/system/arm/cubieboard.rst | 1 +
 hw/arm/Kconfig | 1 +
 hw/arm/allwinner-a10.c | 7 +++
 include/hw/arm/allwinner-a10.h | 2 ++
 4 files changed, 11 insertions(+)

diff --git a/docs/system/arm/cubieboard.rst b/docs/system/arm/cubieboard.rst
index 8d485f5435..58c4a2d3ea 100644
--- a/docs/system/arm/cubieboard.rst
+++ b/docs/system/arm/cubieboard.rst
@@ -15,3 +15,4 @@ Emulated devices:
 - USB controller
 - SATA controller
 - TWI (I2C) controller
+- Watchdog timer
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index b5aed4aff5..ec15248536 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -325,6 +325,7 @@ config ALLWINNER_A10
 select ALLWINNER_A10_PIC
 select ALLWINNER_A10_CCM
 select ALLWINNER_A10_DRAMC
+select ALLWINNER_WDT
 select ALLWINNER_EMAC
 select ALLWINNER_I2C
 select AXP209_PMU
diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index b7ca795c71..b0ea3f7f66 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -38,6 +38,7 @@
 #define AW_A10_EHCI_BASE0x01c14000
 #define AW_A10_OHCI_BASE0x01c14400
 #define AW_A10_SATA_BASE0x01c18000
+#define AW_A10_WDT_BASE 0x01c20c90
 #define AW_A10_RTC_BASE 0x01c20d00
 #define AW_A10_I2C0_BASE0x01c2ac00
 
@@ -92,6 +93,8 @@ static void aw_a10_init(Object *obj)
 object_initialize_child(obj, "mmc0", &s->mmc0, TYPE_AW_SDHOST_SUN4I);
 
 object_initialize_child(obj, "rtc", &s->rtc, TYPE_AW_RTC_SUN4I);
+
+object_initialize_child(obj, "wdt", &s->wdt, TYPE_AW_WDT_SUN4I);
 }
 
 static void aw_a10_realize(DeviceState *dev, Error **errp)
@@ -203,6 +206,10 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
 sysbus_realize(SYS_BUS_DEVICE(&s->i2c0), &error_fatal);
 sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c0), 0, AW_A10_I2C0_BASE);
 sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c0), 0, qdev_get_gpio_in(dev, 7));
+
+/* WDT */
+sysbus_realize(SYS_BUS_DEVICE(&s->wdt), &error_fatal);
+sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->wdt), 0, AW_A10_WDT_BASE, 1);
 }
 
 static void aw_a10_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
index 095afb225d..cd1465c613 100644
--- a/include/hw/arm/allwinner-a10.h
+++ b/include/hw/arm/allwinner-a10.h
@@ -13,6 +13,7 @@
 #include "hw/misc/allwinner-a10-ccm.h"
 #include "hw/misc/allwinner-a10-dramc.h"
 #include "hw/i2c/allwinner-i2c.h"
+#include "hw/watchdog/allwinner-wdt.h"
 #include "sysemu/block-backend.h"
 
 #include "target/arm/cpu.h"
@@ -41,6 +42,7 @@ struct AwA10State {
 AwSdHostState mmc0;
 AWI2CState i2c0;
 AwRtcState rtc;
+AwWdtState wdt;
 MemoryRegion sram_a;
 EHCISysBusState ehci[AW_A10_NUM_USB];
 OHCISysBusState ohci[AW_A10_NUM_USB];
-- 
2.30.2

[PATCH v3 1/4] hw/watchdog: Allwinner WDT emulation for system reset

[Strahinja Jankovic]

This patch adds basic support for Allwinner WDT.
Both sun4i and sun6i variants are supported.
However, interrupt generation is not supported, so WDT can be used only to 
trigger system reset.

Signed-off-by: Strahinja Jankovic 

Reviewed-by: Niek Linnenbank 
Tested-by: Niek Linnenbank 
---
 hw/watchdog/Kconfig |   4 +
 hw/watchdog/allwinner-wdt.c | 416 
 hw/watchdog/meson.build |   1 +
 hw/watchdog/trace-events|   7 +
 include/hw/watchdog/allwinner-wdt.h | 123 
 5 files changed, 551 insertions(+)
 create mode 100644 hw/watchdog/allwinner-wdt.c
 create mode 100644 include/hw/watchdog/allwinner-wdt.h

diff --git a/hw/watchdog/Kconfig b/hw/watchdog/Kconfig
index 66e1d029e3..861fd00334 100644
--- a/hw/watchdog/Kconfig
+++ b/hw/watchdog/Kconfig
@@ -20,3 +20,7 @@ config WDT_IMX2
 
 config WDT_SBSA
 bool
+
+config ALLWINNER_WDT
+bool
+select PTIMER
diff --git a/hw/watchdog/allwinner-wdt.c b/hw/watchdog/allwinner-wdt.c
new file mode 100644
index 00..6205765efe
--- /dev/null
+++ b/hw/watchdog/allwinner-wdt.c
@@ -0,0 +1,416 @@
+/*
+ * Allwinner Watchdog emulation
+ *
+ * Copyright (C) 2023 Strahinja Jankovic 
+ *
+ *  This file is derived from Allwinner RTC,
+ *  by Niek Linnenbank.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/units.h"
+#include "qemu/module.h"
+#include "trace.h"
+#include "hw/sysbus.h"
+#include "hw/registerfields.h"
+#include "hw/watchdog/allwinner-wdt.h"
+#include "sysemu/watchdog.h"
+#include "migration/vmstate.h"
+
+/* WDT registers */
+enum {
+REG_IRQ_EN = 0, /* Watchdog interrupt enable */
+REG_IRQ_STA,/* Watchdog interrupt status */
+REG_CTRL,   /* Watchdog control register */
+REG_CFG,/* Watchdog configuration register */
+REG_MODE,   /* Watchdog mode register */
+};
+
+/* Universal WDT register flags */
+#define WDT_RESTART_MASK(1 << 0)
+#define WDT_EN_MASK (1 << 0)
+
+/* sun4i specific WDT register flags */
+#define RST_EN_SUN4I_MASK   (1 << 1)
+#define INTV_VALUE_SUN4I_SHIFT  (3)
+#define INTV_VALUE_SUN4I_MASK   (0xfu << INTV_VALUE_SUN4I_SHIFT)
+
+/* sun6i specific WDT register flags */
+#define RST_EN_SUN6I_MASK   (1 << 0)
+#define KEY_FIELD_SUN6I_SHIFT   (1)
+#define KEY_FIELD_SUN6I_MASK(0xfffu << KEY_FIELD_SUN6I_SHIFT)
+#define KEY_FIELD_SUN6I (0xA57u)
+#define INTV_VALUE_SUN6I_SHIFT  (4)
+#define INTV_VALUE_SUN6I_MASK   (0xfu << INTV_VALUE_SUN6I_SHIFT)
+
+/* Map of INTV_VALUE to 0.5s units. */
+static const uint8_t allwinner_wdt_count_map[] = {
+1,
+2,
+4,
+6,
+8,
+10,
+12,
+16,
+20,
+24,
+28,
+32
+};
+
+/* WDT sun4i register map (offset to name) */
+const uint8_t allwinner_wdt_sun4i_regmap[] = {
+[0x] = REG_CTRL,
+[0x0004] = REG_MODE,
+};
+
+/* WDT sun6i register map (offset to name) */
+const uint8_t allwinner_wdt_sun6i_regmap[] = {
+[0x] = REG_IRQ_EN,
+[0x0004] = REG_IRQ_STA,
+[0x0010] = REG_CTRL,
+[0x0014] = REG_CFG,
+[0x0018] = REG_MODE,
+};
+
+static bool allwinner_wdt_sun4i_read(AwWdtState *s, uint32_t offset)
+{
+/* no sun4i specific registers currently implemented */
+return false;
+}
+
+static bool allwinner_wdt_sun4i_write(AwWdtState *s, uint32_t offset,
+  uint32_t data)
+{
+/* no sun4i specific registers currently implemented */
+return false;
+}
+
+static bool allwinner_wdt_sun4i_can_reset_system(AwWdtState *s)
+{
+if (s->regs[REG_MODE] & RST_EN_SUN4I_MASK) {
+return true;
+} else {
+return false;
+}
+}
+
+static bool allwinner_wdt_sun4i_is_key_valid(AwWdtState *s, uint32_t val)
+{
+/* sun4i has no key */
+return true;
+}
+
+static uint8_t allwinner_wdt_sun4i_get_intv_value(AwWdtState *s)
+{
+return ((s->regs[REG_MODE] & INTV_VALUE_SUN4I_MASK) >>
+INTV_VALUE_SUN4I_SHIFT);
+}
+
+static bool allwinner_wdt_sun6i_read(AwWdtState *s, uint32_t offset)
+{
+const AwWdtClass *c = AW_WDT_GET_CLASS(s);
+
+switch (c->regmap[offset]) {
+case REG_IRQ_EN:
+case REG_IRQ_STA:
+case REG_CFG:
+return true;
+default:
+break;
+}
+return false;
+}
+
+static bool allwinner_wdt_sun6i_write(AwWd

[PATCH v3 4/4] tests/avocado: Add reboot tests to Cubieboard

[Strahinja Jankovic]

Cubieboard tests end with comment "reboot not functioning; omit test".
Fix this so reboot is done at the end of each test.

Signed-off-by: Strahinja Jankovic 

Reviewed-by: Niek Linnenbank 
Tested-by: Niek Linnenbank 
---
 tests/avocado/boot_linux_console.py | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tests/avocado/boot_linux_console.py 
b/tests/avocado/boot_linux_console.py
index 574609bf43..c0675809e6 100644
--- a/tests/avocado/boot_linux_console.py
+++ b/tests/avocado/boot_linux_console.py
@@ -581,7 +581,10 @@ def test_arm_cubieboard_initrd(self):
 'Allwinner sun4i/sun5i')
 exec_command_and_wait_for_pattern(self, 'cat /proc/iomem',
 'system-control@1c0')
-# cubieboard's reboot is not functioning; omit reboot test.
+exec_command_and_wait_for_pattern(self, 'reboot',
+'reboot: Restarting system')
+# Wait for VM to shut down gracefully
+self.vm.wait()
 
 def test_arm_cubieboard_sata(self):
 """
@@ -625,7 +628,10 @@ def test_arm_cubieboard_sata(self):
 'Allwinner sun4i/sun5i')
 exec_command_and_wait_for_pattern(self, 'cat /proc/partitions',
 'sda')
-# cubieboard's reboot is not functioning; omit reboot test.
+exec_command_and_wait_for_pattern(self, 'reboot',
+'reboot: Restarting system')
+# Wait for VM to shut down gracefully
+self.vm.wait()
 
 @skipUnless(os.getenv('AVOCADO_ALLOW_LARGE_STORAGE'), 'storage limited')
 def test_arm_cubieboard_openwrt_22_03_2(self):
@@ -672,7 +678,10 @@ def test_arm_cubieboard_openwrt_22_03_2(self):
 
 exec_command_and_wait_for_pattern(self, 'cat /proc/cpuinfo',
 'Allwinner sun4i/sun5i')
-# cubieboard's reboot is not functioning; omit reboot test.
+exec_command_and_wait_for_pattern(self, 'reboot',
+'reboot: Restarting system')
+# Wait for VM to shut down gracefully
+self.vm.wait()
 
 @skipUnless(os.getenv('AVOCADO_TIMEOUT_EXPECTED'), 'Test might timeout')
 def test_arm_quanta_gsj(self):
-- 
2.30.2

[PATCH v3 0/4] Basic Allwinner WDT emulation

[Strahinja Jankovic]

This patch set introduces basic emulation of Allwinner WDT.
Since WDT in both A10 and H3 is part of Timer module, the WDT
functionality is added as an overlay in the memory map.

The focus was to enable reboot functionality, so WDT interrupt handling
is not covered in this patch set.

With these patches the `reboot` command can be used for both Cubieboard
and Orangepi-PC in order to restart the system.

Also, Cubieboard avocado tests have been improved to include reboot
steps as well.

v3:
- Fixed allwinner_wdt_sun4i_can_reset_system function to use RST_EN_SUN4I_MASK

v2:
- Cleaned up WDT implementation (changes only in patch 01/04)
- Removed unnecessary checks - instead of changing enum to start from 1,
  removed if (!c->regmap[offset]) since it was conflicting enum values
- Reorganized comments

Strahinja Jankovic (4):
  hw/watchdog: Allwinner WDT emulation for system reset
  hw/arm: Add WDT to Allwinner-A10 and Cubieboard
  hw/arm: Add WDT to Allwinner-H3 and Orangepi-PC
  tests/avocado: Add reboot tests to Cubieboard

 docs/system/arm/cubieboard.rst  |   1 +
 docs/system/arm/orangepi.rst|   1 +
 hw/arm/Kconfig  |   2 +
 hw/arm/allwinner-a10.c  |   7 +
 hw/arm/allwinner-h3.c   |   8 +
 hw/watchdog/Kconfig |   4 +
 hw/watchdog/allwinner-wdt.c | 416 
 hw/watchdog/meson.build |   1 +
 hw/watchdog/trace-events|   7 +
 include/hw/arm/allwinner-a10.h  |   2 +
 include/hw/arm/allwinner-h3.h   |   5 +-
 include/hw/watchdog/allwinner-wdt.h | 123 
 tests/avocado/boot_linux_console.py |  15 +-
 13 files changed, 588 insertions(+), 4 deletions(-)
 create mode 100644 hw/watchdog/allwinner-wdt.c
 create mode 100644 include/hw/watchdog/allwinner-wdt.h

-- 
2.30.2

[PATCH v3 3/4] hw/arm: Add WDT to Allwinner-H3 and Orangepi-PC

[Strahinja Jankovic]

This patch adds WDT to Allwinner-H3 and Orangepi-PC.
WDT is added as an overlay to the Timer module memory area.

Signed-off-by: Strahinja Jankovic 

Reviewed-by: Niek Linnenbank 
---
 docs/system/arm/orangepi.rst  | 1 +
 hw/arm/Kconfig| 1 +
 hw/arm/allwinner-h3.c | 8 
 include/hw/arm/allwinner-h3.h | 5 -
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/docs/system/arm/orangepi.rst b/docs/system/arm/orangepi.rst
index e5973600a1..9afa54213b 100644
--- a/docs/system/arm/orangepi.rst
+++ b/docs/system/arm/orangepi.rst
@@ -26,6 +26,7 @@ The Orange Pi PC machine supports the following devices:
  * System Control module
  * Security Identifier device
  * TWI (I2C)
+ * Watchdog timer
 
 Limitations
 """
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index ec15248536..7d916f5450 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -337,6 +337,7 @@ config ALLWINNER_H3
 select ALLWINNER_A10_PIT
 select ALLWINNER_SUN8I_EMAC
 select ALLWINNER_I2C
+select ALLWINNER_WDT
 select SERIAL
 select ARM_TIMER
 select ARM_GIC
diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index 69d0ad6f50..f05afddf7e 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -49,6 +49,7 @@ const hwaddr allwinner_h3_memmap[] = {
 [AW_H3_DEV_OHCI3]  = 0x01c1d400,
 [AW_H3_DEV_CCU]= 0x01c2,
 [AW_H3_DEV_PIT]= 0x01c20c00,
+[AW_H3_DEV_WDT]= 0x01c20ca0,
 [AW_H3_DEV_UART0]  = 0x01c28000,
 [AW_H3_DEV_UART1]  = 0x01c28400,
 [AW_H3_DEV_UART2]  = 0x01c28800,
@@ -234,6 +235,8 @@ static void allwinner_h3_init(Object *obj)
 object_initialize_child(obj, "twi1",  &s->i2c1,  TYPE_AW_I2C_SUN6I);
 object_initialize_child(obj, "twi2",  &s->i2c2,  TYPE_AW_I2C_SUN6I);
 object_initialize_child(obj, "r_twi", &s->r_twi, TYPE_AW_I2C_SUN6I);
+
+object_initialize_child(obj, "wdt", &s->wdt, TYPE_AW_WDT_SUN6I);
 }
 
 static void allwinner_h3_realize(DeviceState *dev, Error **errp)
@@ -453,6 +456,11 @@ static void allwinner_h3_realize(DeviceState *dev, Error 
**errp)
 sysbus_connect_irq(SYS_BUS_DEVICE(&s->r_twi), 0,
qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_R_TWI));
 
+/* WDT */
+sysbus_realize(SYS_BUS_DEVICE(&s->wdt), &error_fatal);
+sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->wdt), 0,
+s->memmap[AW_H3_DEV_WDT], 1);
+
 /* Unimplemented devices */
 for (i = 0; i < ARRAY_SIZE(unimplemented); i++) {
 create_unimplemented_device(unimplemented[i].device_name,
diff --git a/include/hw/arm/allwinner-h3.h b/include/hw/arm/allwinner-h3.h
index 59e0f822d2..f15d6d7cc7 100644
--- a/include/hw/arm/allwinner-h3.h
+++ b/include/hw/arm/allwinner-h3.h
@@ -48,6 +48,7 @@
 #include "hw/net/allwinner-sun8i-emac.h"
 #include "hw/rtc/allwinner-rtc.h"
 #include "hw/i2c/allwinner-i2c.h"
+#include "hw/watchdog/allwinner-wdt.h"
 #include "target/arm/cpu.h"
 #include "sysemu/block-backend.h"
 
@@ -96,7 +97,8 @@ enum {
 AW_H3_DEV_RTC,
 AW_H3_DEV_CPUCFG,
 AW_H3_DEV_R_TWI,
-AW_H3_DEV_SDRAM
+AW_H3_DEV_SDRAM,
+AW_H3_DEV_WDT
 };
 
 /** Total number of CPU cores in the H3 SoC */
@@ -141,6 +143,7 @@ struct AwH3State {
 AWI2CState r_twi;
 AwSun8iEmacState emac;
 AwRtcState rtc;
+AwWdtState wdt;
 GICState gic;
 MemoryRegion sram_a1;
 MemoryRegion sram_a2;
-- 
2.30.2

[PATCH] tracing: install trace events file only if necessary

[casantos]

From: Carlos Santos 

It is required only if linux-user, bsd-user or system emulator is built.

Signed-off-by: Carlos Santos 
---
 trace/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/trace/meson.build b/trace/meson.build
index 8e80be895c..3fb41c97a4 100644
--- a/trace/meson.build
+++ b/trace/meson.build
@@ -64,7 +64,7 @@ trace_events_all = custom_target('trace-events-all',
  input: trace_events_files,
  command: [ 'cat', '@INPUT@' ],
  capture: true,
- install: true,
+ install: have_linux_user or have_bsd_user or 
have_system,
  install_dir: qemu_datadir)
 
 if 'ust' in get_option('trace_backends')
-- 
2.31.1

[PATCH] meson: install keyboard maps only if necessary

[casantos]

From: Carlos Santos 

They are required only for system emulation (i.e. have_system is true).

Signed-off-by: Carlos Santos 
---
 pc-bios/keymaps/meson.build   | 6 --
 scripts/meson-buildoptions.sh | 2 ++
 tests/fp/berkeley-testfloat-3 | 2 +-
 ui/keycodemapdb   | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/pc-bios/keymaps/meson.build b/pc-bios/keymaps/meson.build
index 158a3b410c..bff3083313 100644
--- a/pc-bios/keymaps/meson.build
+++ b/pc-bios/keymaps/meson.build
@@ -47,7 +47,7 @@ if native_qemu_keymap.found()
build_by_default: true,
output: km,
command: [native_qemu_keymap, '-f', '@OUTPUT@', 
args.split()],
-   install: true,
+   install: have_system,
install_dir: qemu_datadir / 'keymaps')
   endforeach
 
@@ -56,4 +56,6 @@ else
   install_data(keymaps.keys(), install_dir: qemu_datadir / 'keymaps')
 endif
 
-install_data(['sl', 'sv'], install_dir: qemu_datadir / 'keymaps')
+if have_system
+  install_data(['sl', 'sv'], install_dir: qemu_datadir / 'keymaps')
+endif
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 009fab1515..6eec7bc57f 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -301,6 +301,8 @@ _meson_option_parse() {
 --includedir=*) quote_sh "-Dincludedir=$2" ;;
 --enable-install-blobs) printf "%s" -Dinstall_blobs=true ;;
 --disable-install-blobs) printf "%s" -Dinstall_blobs=false ;;
+--enable-install-keymaps) printf "%s" -Dinstall_keymaps=true ;;
+--disable-install-keymaps) printf "%s" -Dinstall_keymaps=false ;;
 --interp-prefix=*) quote_sh "-Dinterp_prefix=$2" ;;
 --enable-jack) printf "%s" -Djack=enabled ;;
 --disable-jack) printf "%s" -Djack=disabled ;;
diff --git a/tests/fp/berkeley-testfloat-3 b/tests/fp/berkeley-testfloat-3
index 40619cbb3b..5a59dcec19 16
--- a/tests/fp/berkeley-testfloat-3
+++ b/tests/fp/berkeley-testfloat-3
@@ -1 +1 @@
-Subproject commit 40619cbb3bf32872df8c53cc457039229428a263
+Subproject commit 5a59dcec19327396a011a17fd924aed4fec416b3
diff --git a/ui/keycodemapdb b/ui/keycodemapdb
index f5772a62ec..d21009b1c9 16
--- a/ui/keycodemapdb
+++ b/ui/keycodemapdb
@@ -1 +1 @@
-Subproject commit f5772a62ec52591ff6870b7e8ef32482371f22c6
+Subproject commit d21009b1c9f94b740ea66be8e48a1d8ad8124023
-- 
2.31.1

Re: [PATCH] meson: allow disablind the installation of keymaps

[Carlos Santos]

On Mon, Jan 2, 2023 at 1:19 PM  wrote:
>
> From: Carlos Santos 
>
> There are situatuions in which the keyboard maps are not necessary (e.g.
> when building only tools or linux-user emulator). Add an option to avoid
> installing them, as already possible to do with firmware blobs.
>
> Signed-off-by: Carlos Santos 
> ---
>  configure | 2 ++
>  meson_options.txt | 2 ++
>  pc-bios/keymaps/meson.build   | 6 --
>  scripts/meson-buildoptions.sh | 4 
>  4 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/configure b/configure
> index 789a4f6cc9..c6ed6a23d0 100755
> --- a/configure
> +++ b/configure
> @@ -889,6 +889,8 @@ for opt do
>;;
>--disable-blobs) meson_option_parse --disable-install-blobs ""
>;;
> +  --disable-keymaps) meson_option_parse --disable-install-keymaps ""
> +  ;;
>--enable-vfio-user-server) vfio_user_server="enabled"
>;;
>--disable-vfio-user-server) vfio_user_server="disabled"
> diff --git a/meson_options.txt b/meson_options.txt
> index 559a571b6b..be27137e98 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -48,6 +48,8 @@ option('module_upgrades', type : 'boolean', value : false,
> description: 'try to load modules from alternate paths for upgrades')
>  option('install_blobs', type : 'boolean', value : true,
> description: 'install provided firmware blobs')
> +option('install_keymaps', type : 'boolean', value : true,
> +   description: 'install provided keyboard maps')
>  option('sparse', type : 'feature', value : 'auto',
> description: 'sparse checker')
>  option('guest_agent', type : 'feature', value : 'auto',
> diff --git a/pc-bios/keymaps/meson.build b/pc-bios/keymaps/meson.build
> index 06c75e646b..7d80c23005 100644
> --- a/pc-bios/keymaps/meson.build
> +++ b/pc-bios/keymaps/meson.build
> @@ -47,7 +47,7 @@ if native_qemu_keymap.found()
> build_by_default: true,
> output: km,
> command: [native_qemu_keymap, '-f', '@OUTPUT@', 
> args.split()],
> -   install: true,
> +   install: get_option('install_keymaps'),
> install_dir: qemu_datadir / 'keymaps')
>endforeach
>
> @@ -56,4 +56,6 @@ else
>install_data(keymaps.keys(), install_dir: qemu_datadir / 'keymaps')
>  endif
>
> -install_data(['sl', 'sv'], install_dir: qemu_datadir / 'keymaps')
> +if get_option('install_keymaps')
> +  install_data(['sl', 'sv'], install_dir: qemu_datadir / 'keymaps')
> +endif
> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
> index aa6e30ea91..f17d9c196e 100644
> --- a/scripts/meson-buildoptions.sh
> +++ b/scripts/meson-buildoptions.sh
> @@ -11,6 +11,8 @@ meson_options_help() {
>printf "%s\n" '  --datadir=VALUE  Data file directory [share]'
>printf "%s\n" '  --disable-coroutine-pool coroutine freelist (better 
> performance)'
>printf "%s\n" '  --disable-install-blobs  install provided firmware blobs'
> +  printf "%s\n" '  --disable-install-keymaps'
> +  printf "%s\n" '   install provided keyboard maps'
>printf "%s\n" '  --docdir=VALUE   Base directory for documentation 
> installation'
>printf "%s\n" '   (can be empty) [share/doc]'
>printf "%s\n" '  --enable-block-drv-whitelist-in-tools'
> @@ -291,6 +293,8 @@ _meson_option_parse() {
>  --includedir=*) quote_sh "-Dincludedir=$2" ;;
>  --enable-install-blobs) printf "%s" -Dinstall_blobs=true ;;
>  --disable-install-blobs) printf "%s" -Dinstall_blobs=false ;;
> +--enable-install-keymaps) printf "%s" -Dinstall_keymaps=true ;;
> +--disable-install-keymaps) printf "%s" -Dinstall_keymaps=false ;;
>  --interp-prefix=*) quote_sh "-Dinterp_prefix=$2" ;;
>  --enable-jack) printf "%s" -Djack=enabled ;;
>  --disable-jack) printf "%s" -Djack=disabled ;;
> --
> 2.31.1
>

This patch can be ignored. I submitted a better solution, which make
the installation depend on have_linux_user or have_bsd_user or
have_system.
-- 
Carlos Santos
Senior Software Maintenance Engineer
Red Hat
casan...@redhat.comT: +55-11-3534-6186

[PATCH V2] meson: install keyboard maps only if necessary

[casantos]

From: Carlos Santos 

They are required only for system emulation (i.e. have_system is true).

Signed-off-by: Carlos Santos 
---
 pc-bios/keymaps/meson.build   | 6 --
 tests/fp/berkeley-testfloat-3 | 2 +-
 ui/keycodemapdb   | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/pc-bios/keymaps/meson.build b/pc-bios/keymaps/meson.build
index 158a3b410c..bff3083313 100644
--- a/pc-bios/keymaps/meson.build
+++ b/pc-bios/keymaps/meson.build
@@ -47,7 +47,7 @@ if native_qemu_keymap.found()
build_by_default: true,
output: km,
command: [native_qemu_keymap, '-f', '@OUTPUT@', 
args.split()],
-   install: true,
+   install: have_system,
install_dir: qemu_datadir / 'keymaps')
   endforeach
 
@@ -56,4 +56,6 @@ else
   install_data(keymaps.keys(), install_dir: qemu_datadir / 'keymaps')
 endif
 
-install_data(['sl', 'sv'], install_dir: qemu_datadir / 'keymaps')
+if have_system
+  install_data(['sl', 'sv'], install_dir: qemu_datadir / 'keymaps')
+endif
diff --git a/tests/fp/berkeley-testfloat-3 b/tests/fp/berkeley-testfloat-3
index 40619cbb3b..5a59dcec19 16
--- a/tests/fp/berkeley-testfloat-3
+++ b/tests/fp/berkeley-testfloat-3
@@ -1 +1 @@
-Subproject commit 40619cbb3bf32872df8c53cc457039229428a263
+Subproject commit 5a59dcec19327396a011a17fd924aed4fec416b3
diff --git a/ui/keycodemapdb b/ui/keycodemapdb
index f5772a62ec..d21009b1c9 16
--- a/ui/keycodemapdb
+++ b/ui/keycodemapdb
@@ -1 +1 @@
-Subproject commit f5772a62ec52591ff6870b7e8ef32482371f22c6
+Subproject commit d21009b1c9f94b740ea66be8e48a1d8ad8124023
-- 
2.31.1

Re: [PATCH v6 3/3] tpm: Add support for TPM device over I2C bus

[Ninad Palsule]

Hi Stephan,

On 3/26/23 8:00 AM, Stefan Berger wrote:



On 3/25/23 22:38, Ninad Palsule wrote:

Qemu already supports devices attached to ISA and sysbus. This drop adds
support for the I2C bus attached TPM devices. I2C model only supports
TPM2 protocol.

This commit includes changes for the common code.
- Added I2C emulation model. Logic was added in the model to temporarily
   cache the data as I2C interface works per byte basis.
- New tpm type "tpm-tis-i2c" added for I2C support. The user has to
   provide this string on command line.

Testing:
   TPM I2C device module is tested using SWTPM (software based TPM
   package). Qemu uses the rainier machine and is connected to swtpm 
over

   the socket interface.

   The command to start swtpm is as follows:
   $ swtpm socket --tpmstate dir=/tmp/mytpm1    \
  --ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock  \
  --tpm2 --log level=100

   The command to start qemu is as follows:
   $ qemu-system-arm -M rainier-bmc -nographic \
 -kernel ${IMAGEPATH}/fitImage-linux.bin \
 -dtb ${IMAGEPATH}/aspeed-bmc-ibm-rainier.dtb \
 -initrd 
${IMAGEPATH}/obmc-phosphor-initramfs.rootfs.cpio.xz \
 -drive 
file=${IMAGEPATH}/obmc-phosphor-image.rootfs.wic.qcow2,if=sd,index=2 \
 -net nic -net 
user,hostfwd=:127.0.0.1:-:22,hostfwd=:127.0.0.1:2443-:443 \

 -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
 -tpmdev emulator,id=tpm0,chardev=chrtpm \
 -device 
tpm-tis-i2c,tpmdev=tpm0,bus=aspeed.i2c.bus.12,address=0x2e


Signed-off-by: Ninad Palsule 
---
V2:
Incorporated Stephen's review comments.
- Handled checksum related register in I2C layer
- Defined I2C interface capabilities and return those instead of
   capabilities from TPM TIS. Add required capabilities from TIS.
- Do not cache FIFO data in the I2C layer.
- Make sure that Device address change register is not passed to I2C
   layer as capability indicate that it is not supported.
- Added boundary checks.
- Make sure that bits 26-31 are zeroed for the TPM_STS register on read
- Updated Kconfig files for new define.

---
V3:
- Moved processing of register TPM_I2C_LOC_SEL in the I2C. So I2C layer
   remembers the locality and pass it to TIS on each read/write.
- The write data is no more cached in the I2C layer so the buffer size
   is reduced to 16 bytes.
- Checksum registers are now managed by the I2C layer. Added new
   function in TIS layer to return the checksum and used that to process
   the request.
- Now 2-4 byte register value will be passed to TIS layer in a single
   write call instead of 1 byte at a time. Added functions to convert
   between little endian stream of bytes to single 32 bit unsigned
   integer. Similarly 32  bit integer to stream of bytes.
- Added restriction on device change register.
- Replace few if-else statement with switch statement for clarity.
- Log warning when unknown register is received.
- Moved all register definations to acpi/tmp.h

---
V4:
Incorporated review comments from Cedric and Stefan.
- Reduced data[] size from 16 byte to 5 bytes.
- Added register name in the mapping table which can be used for
   tracing.
- Removed the endian conversion functions instead used simple logic
   provided by Stefan.
- Rename I2C registers to reduce the length.
- Added traces for send, recv and event functions. You can turn on trace
   on command line by using "-trace "tpm_tis_i2c*" option.

---
V5:
Fixed issues reported by Stefan's test.
- Added mask for the INT_ENABLE register.
- Use correct TIS register for reading interrupt capability.
- Cleanup how register is converted from I2C to TIS and also saved
   information like tis_addr and register name in the i2cst so that we
   can only convert it once on i2c_send.
- Trace register number for unknown registers.

---
V6:
Fixed review comments from Stefan.
- Fixed some variable size.
- Removed unused variables.
- Added vmstat backin to handle migration.
- Added post load phase to reload tis address and register name.
---
  hw/arm/Kconfig   |   1 +
  hw/tpm/Kconfig   |   7 +
  hw/tpm/meson.build   |   1 +
  hw/tpm/tpm_tis_i2c.c | 533 +++
  hw/tpm/trace-events  |   6 +
  include/sysemu/tpm.h |   3 +
  6 files changed, 551 insertions(+)
  create mode 100644 hw/tpm/tpm_tis_i2c.c

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index b5aed4aff5..05d6ef1a31 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -6,6 +6,7 @@ config ARM_VIRT
  imply VFIO_PLATFORM
  imply VFIO_XGMAC
  imply TPM_TIS_SYSBUS
+    imply TPM_TIS_I2C
  imply NVDIMM
  select ARM_GIC
  select ACPI
diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
index 29e82f3c92..a46663288c 100644
--- a/hw/tpm/Kconfig
+++ b/hw/tpm/Kconfig
@@ -1,3 +1,10 @@
+config TPM_TIS_I2C
+    bool
+    depends on TPM
+    select TPM_BACKEND
+    select I2C
+    select TPM_TIS
+
  config TPM_TIS_ISA
  bool
  de

[PATCH v7 3/3] tpm: Add support for TPM device over I2C bus

[Ninad Palsule]

Qemu already supports devices attached to ISA and sysbus. This drop adds
support for the I2C bus attached TPM devices. I2C model only supports
TPM2 protocol.

This commit includes changes for the common code.
- Added I2C emulation model. Logic was added in the model to temporarily
  cache the data as I2C interface works per byte basis.
- New tpm type "tpm-tis-i2c" added for I2C support. The user has to
  provide this string on command line.

Testing:
  TPM I2C device module is tested using SWTPM (software based TPM
  package). Qemu uses the rainier machine and is connected to swtpm over
  the socket interface.

  The command to start swtpm is as follows:
  $ swtpm socket --tpmstate dir=/tmp/mytpm1\
 --ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock  \
 --tpm2 --log level=100

  The command to start qemu is as follows:
  $ qemu-system-arm -M rainier-bmc -nographic \
-kernel ${IMAGEPATH}/fitImage-linux.bin \
-dtb ${IMAGEPATH}/aspeed-bmc-ibm-rainier.dtb \
-initrd ${IMAGEPATH}/obmc-phosphor-initramfs.rootfs.cpio.xz \
-drive 
file=${IMAGEPATH}/obmc-phosphor-image.rootfs.wic.qcow2,if=sd,index=2 \
-net nic -net 
user,hostfwd=:127.0.0.1:-:22,hostfwd=:127.0.0.1:2443-:443 \
-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-tis-i2c,tpmdev=tpm0,bus=aspeed.i2c.bus.12,address=0x2e

Signed-off-by: Ninad Palsule 
---
V2:
Incorporated Stephen's review comments.
- Handled checksum related register in I2C layer
- Defined I2C interface capabilities and return those instead of
  capabilities from TPM TIS. Add required capabilities from TIS.
- Do not cache FIFO data in the I2C layer.
- Make sure that Device address change register is not passed to I2C
  layer as capability indicate that it is not supported.
- Added boundary checks.
- Make sure that bits 26-31 are zeroed for the TPM_STS register on read
- Updated Kconfig files for new define.

---
V3:
- Moved processing of register TPM_I2C_LOC_SEL in the I2C. So I2C layer
  remembers the locality and pass it to TIS on each read/write.
- The write data is no more cached in the I2C layer so the buffer size
  is reduced to 16 bytes.
- Checksum registers are now managed by the I2C layer. Added new
  function in TIS layer to return the checksum and used that to process
  the request.
- Now 2-4 byte register value will be passed to TIS layer in a single
  write call instead of 1 byte at a time. Added functions to convert
  between little endian stream of bytes to single 32 bit unsigned
  integer. Similarly 32  bit integer to stream of bytes.
- Added restriction on device change register.
- Replace few if-else statement with switch statement for clarity.
- Log warning when unknown register is received.
- Moved all register definations to acpi/tmp.h

---
V4:
Incorporated review comments from Cedric and Stefan.
- Reduced data[] size from 16 byte to 5 bytes.
- Added register name in the mapping table which can be used for
  tracing.
- Removed the endian conversion functions instead used simple logic
  provided by Stefan.
- Rename I2C registers to reduce the length.
- Added traces for send, recv and event functions. You can turn on trace
  on command line by using "-trace "tpm_tis_i2c*" option.

---
V5:
Fixed issues reported by Stefan's test.
- Added mask for the INT_ENABLE register.
- Use correct TIS register for reading interrupt capability.
- Cleanup how register is converted from I2C to TIS and also saved
  information like tis_addr and register name in the i2cst so that we
  can only convert it once on i2c_send.
- Trace register number for unknown registers.

---
V6:
Fixed review comments from Stefan.
- Fixed some variable size.
- Removed unused variables.
- Added vmstat backin to handle migration.
- Added post load phase to reload tis address and register name.

---
V7:
Incorporated review comments from Stefan.
- Added tpm_tis_i2c_initfn function
- Set the device catagory DEVICE_CATEGORY_MISC.
- Corrected default locality selection.
- Other cleanup. Include file cleanup.
---
 hw/arm/Kconfig   |   1 +
 hw/tpm/Kconfig   |   7 +
 hw/tpm/meson.build   |   1 +
 hw/tpm/tpm_tis_i2c.c | 540 +++
 hw/tpm/trace-events  |   6 +
 include/sysemu/tpm.h |   3 +
 6 files changed, 558 insertions(+)
 create mode 100644 hw/tpm/tpm_tis_i2c.c

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index b5aed4aff5..05d6ef1a31 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -6,6 +6,7 @@ config ARM_VIRT
 imply VFIO_PLATFORM
 imply VFIO_XGMAC
 imply TPM_TIS_SYSBUS
+imply TPM_TIS_I2C
 imply NVDIMM
 select ARM_GIC
 select ACPI
diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
index 29e82f3c92..a46663288c 100644
--- a/hw/tpm/Kconfig
+++ b/hw/tpm/Kconfig
@@ -1,3 +1,10 @@
+config TPM_TIS_I2C
+bool
+depends on TPM
+select TPM_BACKEND
+select I2C

[PATCH v7 2/3] tpm: Extend common APIs to support TPM TIS I2C

[Ninad Palsule]

Qemu already supports devices attached to ISA and sysbus. This drop adds
support for the I2C bus attached TPM devices.

This commit includes changes for the common code.
- Added support for the new checksum registers which are required for
  the I2C support. The checksum calculation is handled in the qemu
  common code.
- Added wrapper function for read and write data so that I2C code can
  call it without MMIO interface.

The TPM TIS I2C spec describes in the table in section "Interface Locality
Usage per Register" that the TPM_INT_ENABLE and TPM_INT_STATUS registers
must be writable for any locality even if the locality is not the active
locality. Therefore, remove the checks whether the writing locality is the
active locality for these registers.

Signed-off-by: Ninad Palsule 
Signed-off-by: Stefan Berger 
---
V2:

Incorporated Stephen's comments.

- Removed checksum enable and checksum get registers.
- Added checksum calculation function which can be called from
  i2c layer.

---
V3:
Incorporated review comments from Cedric and Stefan.

- Pass locality to the checksum calculation function and cleanup
- Moved I2C related definations in the acpi/tpm.h

---
V4:

Incorporated review comments by Stefan

- Remove the check for locality while calculating checksum
- Use bswap16 instead of cpu_ti_be16.
- Rename TPM_I2C register by dropping _TIS_ from it.

---
V7:

Incorporated review comments from Stefan.

- Removed locality check from INT_ENABLE and INT_STATUS registers write
  path.
- Moved TPM_DATA_CSUM_ENABLED define in the tpm.h
---
 hw/tpm/tpm_tis.h|  3 +++
 hw/tpm/tpm_tis_common.c | 36 
 include/hw/acpi/tpm.h   | 31 +++
 3 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
index f6b5872ba6..6f29a508dd 100644
--- a/hw/tpm/tpm_tis.h
+++ b/hw/tpm/tpm_tis.h
@@ -86,5 +86,8 @@ int tpm_tis_pre_save(TPMState *s);
 void tpm_tis_reset(TPMState *s);
 enum TPMVersion tpm_tis_get_tpm_version(TPMState *s);
 void tpm_tis_request_completed(TPMState *s, int ret);
+uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size);
+void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, uint32_t size);
+uint16_t tpm_tis_get_checksum(TPMState *s);
 
 #endif /* TPM_TPM_TIS_H */
diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
index 503be2a541..c07c179dbc 100644
--- a/hw/tpm/tpm_tis_common.c
+++ b/hw/tpm/tpm_tis_common.c
@@ -26,6 +26,8 @@
 #include "hw/irq.h"
 #include "hw/isa/isa.h"
 #include "qapi/error.h"
+#include "qemu/bswap.h"
+#include "qemu/crc-ccitt.h"
 #include "qemu/module.h"
 
 #include "hw/acpi/tpm.h"
@@ -447,6 +449,23 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr 
addr,
 return val;
 }
 
+/*
+ * A wrapper read function so that it can be directly called without
+ * mmio.
+ */
+uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size)
+{
+return tpm_tis_mmio_read(s, addr, size);
+}
+
+/*
+ * Calculate current data buffer checksum
+ */
+uint16_t tpm_tis_get_checksum(TPMState *s)
+{
+return bswap16(crc_ccitt(0, s->buffer, s->rw_offset));
+}
+
 /*
  * Write a value to a register of the TIS interface
  * See specs pages 33-63 for description of the registers
@@ -588,10 +607,6 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
 
 break;
 case TPM_TIS_REG_INT_ENABLE:
-if (s->active_locty != locty) {
-break;
-}
-
 s->loc[locty].inte &= mask;
 s->loc[locty].inte |= (val & (TPM_TIS_INT_ENABLED |
 TPM_TIS_INT_POLARITY_MASK |
@@ -601,10 +616,6 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
 /* hard wired -- ignore */
 break;
 case TPM_TIS_REG_INT_STATUS:
-if (s->active_locty != locty) {
-break;
-}
-
 /* clearing of interrupt flags */
 if (((val & TPM_TIS_INTERRUPTS_SUPPORTED)) &&
 (s->loc[locty].ints & TPM_TIS_INTERRUPTS_SUPPORTED)) {
@@ -767,6 +778,15 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
 }
 }
 
+/*
+ * A wrapper write function so that it can be directly called without
+ * mmio.
+ */
+void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, uint32_t size)
+{
+tpm_tis_mmio_write(s, addr, val, size);
+}
+
 const MemoryRegionOps tpm_tis_memory_ops = {
 .read = tpm_tis_mmio_read,
 .write = tpm_tis_mmio_write,
diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index 559ba6906c..39f1300aa0 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -93,6 +93,7 @@
 #define TPM_TIS_CAP_DATA_TRANSFER_64B(3 << 9)
 #define TPM_TIS_CAP_DATA_TRANSFER_LEGACY (0 << 9)
 #define TPM_TIS_CAP_BURST_COUNT_DYNAMIC  (0 << 8)
+#define TPM_TIS_CAP_BURST_COUNT_STATIC   (1 << 8)
 #define TPM_TIS_CAP_INTERRUPT_LOW_LEVEL  (1 << 4) /* support is mandatory */
 #define TPM_TIS_CAPABILITIES_SUPPORTED1_3 \
 (TPM_TIS_CAP_I

[PATCH v7 1/3] docs: Add support for TPM devices over I2C bus

[Ninad Palsule]

This is a documentation change for I2C TPM device support.

Qemu already supports devices attached to ISA and sysbus.
This drop adds support for the I2C bus attached TPM devices.

Signed-off-by: Ninad Palsule 

---
V2:

Incorporated Stephen's review comments
- Added example in the document.

---
V4:
Incorporate Cedric & Stefan's comments

- Added example for ast2600-evb
- Corrected statement about arm virtual machine.

---
V6:
Incorporated review comments from Stefan.
---
 docs/specs/tpm.rst | 32 
 1 file changed, 32 insertions(+)

diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index 535912a92b..590e670a9a 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -21,12 +21,16 @@ QEMU files related to TPM TIS interface:
  - ``hw/tpm/tpm_tis_common.c``
  - ``hw/tpm/tpm_tis_isa.c``
  - ``hw/tpm/tpm_tis_sysbus.c``
+ - ``hw/tpm/tpm_tis_i2c.c``
  - ``hw/tpm/tpm_tis.h``
 
 Both an ISA device and a sysbus device are available. The former is
 used with pc/q35 machine while the latter can be instantiated in the
 Arm virt machine.
 
+An I2C device support is also provided which can be instantiated in the Arm
+based emulation machines. This device only supports the TPM 2 protocol.
+
 CRB interface
 -
 
@@ -348,6 +352,34 @@ In case an Arm virt machine is emulated, use the following 
command line:
 -drive if=pflash,format=raw,file=flash0.img,readonly=on \
 -drive if=pflash,format=raw,file=flash1.img
 
+In case a ast2600-evb bmc machine is emulated and want to use TPM device
+attached to I2C bus, use the following command line:
+
+.. code-block:: console
+
+  qemu-system-arm -M ast2600-evb -nographic \
+-kernel arch/arm/boot/zImage \
+-dtb arch/arm/boot/dts/aspeed-ast2600-evb.dtb \
+-initrd rootfs.cpio \
+-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
+-tpmdev emulator,id=tpm0,chardev=chrtpm \
+-device tpm-tis-i2c,tpmdev=tpm0,bus=aspeed.i2c.bus.12,address=0x2e
+
+In case a Rainier bmc machine is emulated and want to use TPM device
+attached to I2C bus, use the following command line:
+
+.. code-block:: console
+
+  qemu-system-arm -M rainier-bmc -nographic \
+-kernel ${IMAGEPATH}/fitImage-linux.bin \
+-dtb ${IMAGEPATH}/aspeed-bmc-ibm-rainier.dtb \
+-initrd ${IMAGEPATH}/obmc-phosphor-initramfs.rootfs.cpio.xz \
+-drive 
file=${IMAGEPATH}/obmc-phosphor-image.rootfs.wic.qcow2,if=sd,index=2\
+-net nic -net 
user,hostfwd=:127.0.0.1:-:22,hostfwd=:127.0.0.1:2443-:443\
+-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
+-tpmdev emulator,id=tpm0,chardev=chrtpm \
+-device tpm-tis-i2c,tpmdev=tpm0,bus=aspeed.i2c.bus.12,address=0x2e
+
 In case SeaBIOS is used as firmware, it should show the TPM menu item
 after entering the menu with 'ESC'.
 
-- 
2.37.2

[PATCH v7 0/3] Add support for TPM devices over I2C bus

[Ninad Palsule]

Hello,

I have incorporated review comments from Stefan. Please review.

This drop adds support for the TPM devices attached to the I2C bus. It
only supports the TPM2 protocol. You need to run it with the external
TPM emulator like swtpm. I have tested it with swtpm.

I have refered to the work done by zhdan...@meta.com but at the core
level out implementation is different.
https://github.com/theopolis/qemu/commit/2e2e57cde9e419c36af8071bb85392ad1ed70966

Based-on: $MESSAGE_ID


Ninad Palsule (3):
  docs: Add support for TPM devices over I2C bus
  tpm: Extend common APIs to support TPM TIS I2C
  tpm: Add support for TPM device over I2C bus

 docs/specs/tpm.rst  |  32 +++
 hw/arm/Kconfig  |   1 +
 hw/tpm/Kconfig  |   7 +
 hw/tpm/meson.build  |   1 +
 hw/tpm/tpm_tis.h|   3 +
 hw/tpm/tpm_tis_common.c |  36 ++-
 hw/tpm/tpm_tis_i2c.c| 540 
 hw/tpm/trace-events |   6 +
 include/hw/acpi/tpm.h   |  31 +++
 include/sysemu/tpm.h|   3 +
 10 files changed, 652 insertions(+), 8 deletions(-)
 create mode 100644 hw/tpm/tpm_tis_i2c.c

-- 
2.37.2

Re: [PATCH v7 2/3] tpm: Extend common APIs to support TPM TIS I2C

[Stefan Berger]

On 3/26/23 18:44, Ninad Palsule wrote:

Qemu already supports devices attached to ISA and sysbus. This drop adds
support for the I2C bus attached TPM devices.

This commit includes changes for the common code.
- Added support for the new checksum registers which are required for
   the I2C support. The checksum calculation is handled in the qemu
   common code.
- Added wrapper function for read and write data so that I2C code can
   call it without MMIO interface.

The TPM TIS I2C spec describes in the table in section "Interface Locality
Usage per Register" that the TPM_INT_ENABLE and TPM_INT_STATUS registers
must be writable for any locality even if the locality is not the active
locality. Therefore, remove the checks whether the writing locality is the
active locality for these registers.

Signed-off-by: Ninad Palsule 
Signed-off-by: Stefan Berger 
---
V2:

Incorporated Stephen's comments.

- Removed checksum enable and checksum get registers.
- Added checksum calculation function which can be called from
   i2c layer.

---
V3:
Incorporated review comments from Cedric and Stefan.

- Pass locality to the checksum calculation function and cleanup
- Moved I2C related definations in the acpi/tpm.h

---
V4:

Incorporated review comments by Stefan

- Remove the check for locality while calculating checksum
- Use bswap16 instead of cpu_ti_be16.
- Rename TPM_I2C register by dropping _TIS_ from it.

---
V7:

Incorporated review comments from Stefan.

- Removed locality check from INT_ENABLE and INT_STATUS registers write
   path.
- Moved TPM_DATA_CSUM_ENABLED define in the tpm.h
---
  hw/tpm/tpm_tis.h|  3 +++
  hw/tpm/tpm_tis_common.c | 36 
  include/hw/acpi/tpm.h   | 31 +++
  3 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
index f6b5872ba6..6f29a508dd 100644
--- a/hw/tpm/tpm_tis.h
+++ b/hw/tpm/tpm_tis.h
@@ -86,5 +86,8 @@ int tpm_tis_pre_save(TPMState *s);
  void tpm_tis_reset(TPMState *s);
  enum TPMVersion tpm_tis_get_tpm_version(TPMState *s);
  void tpm_tis_request_completed(TPMState *s, int ret);
+uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size);
+void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, uint32_t size);
+uint16_t tpm_tis_get_checksum(TPMState *s);
  
  #endif /* TPM_TPM_TIS_H */

diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
index 503be2a541..c07c179dbc 100644
--- a/hw/tpm/tpm_tis_common.c
+++ b/hw/tpm/tpm_tis_common.c
@@ -26,6 +26,8 @@
  #include "hw/irq.h"
  #include "hw/isa/isa.h"
  #include "qapi/error.h"
+#include "qemu/bswap.h"
+#include "qemu/crc-ccitt.h"
  #include "qemu/module.h"
  
  #include "hw/acpi/tpm.h"

@@ -447,6 +449,23 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr 
addr,
  return val;
  }
  
+/*

+ * A wrapper read function so that it can be directly called without
+ * mmio.
+ */
+uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size)
+{
+return tpm_tis_mmio_read(s, addr, size);
+}
+
+/*
+ * Calculate current data buffer checksum
+ */
+uint16_t tpm_tis_get_checksum(TPMState *s)
+{
+return bswap16(crc_ccitt(0, s->buffer, s->rw_offset));
+}
+
  /*
   * Write a value to a register of the TIS interface
   * See specs pages 33-63 for description of the registers
@@ -588,10 +607,6 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
  
  break;

  case TPM_TIS_REG_INT_ENABLE:
-if (s->active_locty != locty) {
-break;
-}
-
  s->loc[locty].inte &= mask;
  s->loc[locty].inte |= (val & (TPM_TIS_INT_ENABLED |
  TPM_TIS_INT_POLARITY_MASK |
@@ -601,10 +616,6 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
  /* hard wired -- ignore */
  break;
  case TPM_TIS_REG_INT_STATUS:
-if (s->active_locty != locty) {
-break;
-}
-
  /* clearing of interrupt flags */
  if (((val & TPM_TIS_INTERRUPTS_SUPPORTED)) &&
  (s->loc[locty].ints & TPM_TIS_INTERRUPTS_SUPPORTED)) {
@@ -767,6 +778,15 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
  }
  }
  
+/*

+ * A wrapper write function so that it can be directly called without
+ * mmio.
+ */
+void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, uint32_t size)
+{
+tpm_tis_mmio_write(s, addr, val, size);
+}
+
  const MemoryRegionOps tpm_tis_memory_ops = {
  .read = tpm_tis_mmio_read,
  .write = tpm_tis_mmio_write,
diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index 559ba6906c..39f1300aa0 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -93,6 +93,7 @@
  #define TPM_TIS_CAP_DATA_TRANSFER_64B(3 << 9)
  #define TPM_TIS_CAP_DATA_TRANSFER_LEGACY (0 << 9)
  #define TPM_TIS_CAP_BURST_COUNT_DYNAMIC  (0 << 8)
+#define TPM_TIS_CAP_BURST_COUNT_STATIC   (1 << 8)
  #define TPM_TIS_CAP_INTERRUPT_LOW_LEVEL  (1 

[PATCH 0/2] qtests: tpm: Add test cases for TPM TIS I2C device emulation

[Stefan Berger]

Refactor existing test code and move tpm_util_tis_transmit() into
tpm-tis-utils.c to avoid having to declare tpm_tis_base_addr in test
cases that have nothing to do with TIS and shouldn't need to provide
this variable.

Add test cases exercising much of the TPM TIS I2C device emulation
assuming that the device is connected to the Aspeed I2C controller.
Tests are passing on little and big endian hosts.

This series of patches builds on the following series of patches
providing the TPM TIS I2C device emulation:
https://lists.nongnu.org/archive/html/qemu-devel/2023-03/msg06253.html

Regards,
Stefan


Stefan Berger (2):
  qtest: Move tpm_util_tis_transmit() into tpm-tis-utils.c and rename it
  qtest: Add a test case for TPM TIS I2C connected to Aspeed I2C
controller

 tests/qtest/meson.build |   3 +
 tests/qtest/qtest_aspeed.c  | 117 +
 tests/qtest/qtest_aspeed.h  |  27 +
 tests/qtest/tpm-crb-swtpm-test.c|   3 -
 tests/qtest/tpm-crb-test.c  |   3 -
 tests/qtest/tpm-tis-device-swtpm-test.c |   5 +-
 tests/qtest/tpm-tis-i2c-test.c  | 628 
 tests/qtest/tpm-tis-swtpm-test.c|   5 +-
 tests/qtest/tpm-tis-util.c  |  47 +-
 tests/qtest/tpm-tis-util.h  |   4 +
 tests/qtest/tpm-util.c  |  45 --
 tests/qtest/tpm-util.h  |   3 -
 12 files changed, 831 insertions(+), 59 deletions(-)
 create mode 100644 tests/qtest/qtest_aspeed.c
 create mode 100644 tests/qtest/qtest_aspeed.h
 create mode 100644 tests/qtest/tpm-tis-i2c-test.c

-- 
2.39.2

[PATCH 1/2] qtest: Move tpm_util_tis_transmit() into tpm-tis-utils.c and rename it

[Stefan Berger]

To be able to remove tpm_tis_base_addr from test cases that do not really
need it move the tpm_util_tis_transmit() function into tpm-tis-utils.c and
rename it to tpm_tis_transmit().

Fix a locality parameter in a test case on the way.

Signed-off-by: Stefan Berger 
---
 tests/qtest/tpm-crb-swtpm-test.c|  3 --
 tests/qtest/tpm-crb-test.c  |  3 --
 tests/qtest/tpm-tis-device-swtpm-test.c |  5 +--
 tests/qtest/tpm-tis-swtpm-test.c|  5 +--
 tests/qtest/tpm-tis-util.c  | 47 -
 tests/qtest/tpm-tis-util.h  |  4 +++
 tests/qtest/tpm-util.c  | 45 ---
 tests/qtest/tpm-util.h  |  3 --
 8 files changed, 56 insertions(+), 59 deletions(-)

diff --git a/tests/qtest/tpm-crb-swtpm-test.c b/tests/qtest/tpm-crb-swtpm-test.c
index 40254f762f..ffeb1c396b 100644
--- a/tests/qtest/tpm-crb-swtpm-test.c
+++ b/tests/qtest/tpm-crb-swtpm-test.c
@@ -19,9 +19,6 @@
 #include "tpm-tests.h"
 #include "hw/acpi/tpm.h"
 
-/* Not used but needed for linking */
-uint64_t tpm_tis_base_addr = TPM_TIS_ADDR_BASE;
-
 typedef struct TestState {
 char *src_tpm_path;
 char *dst_tpm_path;
diff --git a/tests/qtest/tpm-crb-test.c b/tests/qtest/tpm-crb-test.c
index 7b94453390..396ae3f91c 100644
--- a/tests/qtest/tpm-crb-test.c
+++ b/tests/qtest/tpm-crb-test.c
@@ -19,9 +19,6 @@
 #include "qemu/module.h"
 #include "tpm-emu.h"
 
-/* Not used but needed for linking */
-uint64_t tpm_tis_base_addr = TPM_TIS_ADDR_BASE;
-
 #define TPM_CMD "\x80\x01\x00\x00\x00\x0c\x00\x00\x01\x44\x00\x00"
 
 static void tpm_crb_test(const void *data)
diff --git a/tests/qtest/tpm-tis-device-swtpm-test.c 
b/tests/qtest/tpm-tis-device-swtpm-test.c
index 8c067fddd4..517a077005 100644
--- a/tests/qtest/tpm-tis-device-swtpm-test.c
+++ b/tests/qtest/tpm-tis-device-swtpm-test.c
@@ -18,6 +18,7 @@
 #include "libqtest.h"
 #include "qemu/module.h"
 #include "tpm-tests.h"
+#include "tpm-tis-util.h"
 #include "hw/acpi/tpm.h"
 
 uint64_t tpm_tis_base_addr = 0xc00;
@@ -33,7 +34,7 @@ static void tpm_tis_swtpm_test(const void *data)
 {
 const TestState *ts = data;
 
-tpm_test_swtpm_test(ts->src_tpm_path, tpm_util_tis_transfer,
+tpm_test_swtpm_test(ts->src_tpm_path, tpm_tis_transfer,
 "tpm-tis-device", MACHINE_OPTIONS);
 }
 
@@ -42,7 +43,7 @@ static void tpm_tis_swtpm_migration_test(const void *data)
 const TestState *ts = data;
 
 tpm_test_swtpm_migration_test(ts->src_tpm_path, ts->dst_tpm_path, ts->uri,
-  tpm_util_tis_transfer, "tpm-tis-device",
+  tpm_tis_transfer, "tpm-tis-device",
   MACHINE_OPTIONS);
 }
 
diff --git a/tests/qtest/tpm-tis-swtpm-test.c b/tests/qtest/tpm-tis-swtpm-test.c
index 11539c0a52..105e42e21d 100644
--- a/tests/qtest/tpm-tis-swtpm-test.c
+++ b/tests/qtest/tpm-tis-swtpm-test.c
@@ -17,6 +17,7 @@
 #include "libqtest.h"
 #include "qemu/module.h"
 #include "tpm-tests.h"
+#include "tpm-tis-util.h"
 #include "hw/acpi/tpm.h"
 
 uint64_t tpm_tis_base_addr = TPM_TIS_ADDR_BASE;
@@ -31,7 +32,7 @@ static void tpm_tis_swtpm_test(const void *data)
 {
 const TestState *ts = data;
 
-tpm_test_swtpm_test(ts->src_tpm_path, tpm_util_tis_transfer,
+tpm_test_swtpm_test(ts->src_tpm_path, tpm_tis_transfer,
 "tpm-tis", NULL);
 }
 
@@ -40,7 +41,7 @@ static void tpm_tis_swtpm_migration_test(const void *data)
 const TestState *ts = data;
 
 tpm_test_swtpm_migration_test(ts->src_tpm_path, ts->dst_tpm_path, ts->uri,
-  tpm_util_tis_transfer, "tpm-tis", NULL);
+  tpm_tis_transfer, "tpm-tis", NULL);
 }
 
 int main(int argc, char **argv)
diff --git a/tests/qtest/tpm-tis-util.c b/tests/qtest/tpm-tis-util.c
index 939893bf01..728cd3e065 100644
--- a/tests/qtest/tpm-tis-util.c
+++ b/tests/qtest/tpm-tis-util.c
@@ -52,7 +52,7 @@ void tpm_tis_test_check_localities(const void *data)
 uint32_t rid;
 
 for (locty = 0; locty < TPM_TIS_NUM_LOCALITIES; locty++) {
-access = readb(TIS_REG(0, TPM_TIS_REG_ACCESS));
+access = readb(TIS_REG(locty, TPM_TIS_REG_ACCESS));
 g_assert_cmpint(access, ==, TPM_TIS_ACCESS_TPM_REG_VALID_STS |
 TPM_TIS_ACCESS_TPM_ESTABLISHMENT);
 
@@ -449,3 +449,48 @@ void tpm_tis_test_check_transmit(const void *data)
 writeb(TIS_REG(0, TPM_TIS_REG_ACCESS), TPM_TIS_ACCESS_ACTIVE_LOCALITY);
 access = readb(TIS_REG(0, TPM_TIS_REG_ACCESS));
 }
+
+void tpm_tis_transfer(QTestState *s,
+  const unsigned char *req, size_t req_size,
+  unsigned char *rsp, size_t rsp_size)
+{
+uint32_t sts;
+uint16_t bcount;
+size_t i;
+
+/* request use of locality 0 */
+qtest_writeb(s, TIS_REG(0, TPM_TIS_REG_ACCESS), 
TPM_TIS_ACCESS_REQUEST_USE);
+qtest_writel(s, TIS_REG(0, TPM_TIS_REG_STS), TPM_TIS_STS_COM

[PATCH 2/2] qtest: Add a test case for TPM TIS I2C connected to Aspeed I2C controller

[Stefan Berger]

Add a test case for the TPM TIS I2C device exercising most of its
functionality, including localities.

Add library functions for being able to read from and write to registers
of the TPM TIS I2C device connected to the Aspeed i2c controller.

Signed-off-by: Stefan Berger 
---
 tests/qtest/meson.build|   3 +
 tests/qtest/qtest_aspeed.c | 117 ++
 tests/qtest/qtest_aspeed.h |  27 ++
 tests/qtest/tpm-tis-i2c-test.c | 628 +
 4 files changed, 775 insertions(+)
 create mode 100644 tests/qtest/qtest_aspeed.c
 create mode 100644 tests/qtest/qtest_aspeed.h
 create mode 100644 tests/qtest/tpm-tis-i2c-test.c

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 29a4efb4c2..065a00d34d 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -200,6 +200,7 @@ qtests_arm = \
   (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed : []) + \
   (config_all_devices.has_key('CONFIG_NPCM7XX') ? qtests_npcm7xx : []) + \
   (config_all_devices.has_key('CONFIG_GENERIC_LOADER') ? ['hexloader-test'] : 
[]) + \
+  (config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : 
[]) + \
   ['arm-cpu-features',
'microbit-test',
'test-arm-mptimer',
@@ -212,6 +213,7 @@ qtests_aarch64 = \
 ['tpm-tis-device-test', 'tpm-tis-device-swtpm-test'] : []) +   
  \
   (config_all_devices.has_key('CONFIG_XLNX_ZYNQMP_ARM') ? ['xlnx-can-test', 
'fuzz-xlnx-dp-test'] : []) + \
   (config_all_devices.has_key('CONFIG_RASPI') ? ['bcm2835-dma-test'] : []) +  \
+  (config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : 
[]) + \
   ['arm-cpu-features',
'numa-test',
'boot-serial-test',
@@ -303,6 +305,7 @@ qtests = {
   'tpm-crb-test': [io, tpmemu_files],
   'tpm-tis-swtpm-test': [io, tpmemu_files, 'tpm-tis-util.c'],
   'tpm-tis-test': [io, tpmemu_files, 'tpm-tis-util.c'],
+  'tpm-tis-i2c-test': [io, tpmemu_files, 'qtest_aspeed.c'],
   'tpm-tis-device-swtpm-test': [io, tpmemu_files, 'tpm-tis-util.c'],
   'tpm-tis-device-test': [io, tpmemu_files, 'tpm-tis-util.c'],
   'vmgenid-test': files('boot-sector.c', 'acpi-utils.c'),
diff --git a/tests/qtest/qtest_aspeed.c b/tests/qtest/qtest_aspeed.c
new file mode 100644
index 00..2b316178e4
--- /dev/null
+++ b/tests/qtest/qtest_aspeed.c
@@ -0,0 +1,117 @@
+/*
+ * Aspeed i2c bus interface to reading and writing to i2c device registers
+ *
+ * Copyright (c) 2023 IBM Corporation
+ *
+ * Authors:
+ *   Stefan Berger 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "qtest_aspeed.h"
+
+#include "hw/i2c/aspeed_i2c.h"
+#include "libqtest-single.h"
+
+#define A_I2CD_M_STOP_CMD   BIT(5)
+#define A_I2CD_M_RX_CMD BIT(3)
+#define A_I2CD_M_TX_CMD BIT(1)
+#define A_I2CD_M_START_CMD  BIT(0)
+
+#define A_I2CD_MASTER_ENBIT(0)
+
+static void aspeed_i2c_startup(uint32_t baseaddr, uint8_t slave_addr,
+   uint8_t reg)
+{
+uint32_t v;
+static int once;
+
+if (!once) {
+/* one time: enable master */
+   writel(baseaddr + A_I2CC_FUN_CTRL, 0);
+   v = readl(baseaddr + A_I2CC_FUN_CTRL) | A_I2CD_MASTER_EN;
+   writel(baseaddr + A_I2CC_FUN_CTRL, v);
+   once = 1;
+}
+
+/* select device */
+writel(baseaddr + A_I2CD_BYTE_BUF, slave_addr << 1);
+writel(baseaddr + A_I2CD_CMD, A_I2CD_M_START_CMD | A_I2CD_M_RX_CMD);
+
+/* select the register to write to */
+writel(baseaddr + A_I2CD_BYTE_BUF, reg);
+writel(baseaddr + A_I2CD_CMD, A_I2CD_M_TX_CMD);
+}
+
+static uint32_t aspeed_i2c_read_n(uint32_t baseaddr, uint8_t slave_addr,
+  uint8_t reg, size_t nbytes)
+{
+uint32_t res = 0;
+uint32_t v;
+size_t i;
+
+aspeed_i2c_startup(baseaddr, slave_addr, reg);
+
+for (i = 0; i < nbytes; i++) {
+writel(baseaddr + A_I2CD_CMD, A_I2CD_M_RX_CMD);
+v = readl(baseaddr + A_I2CD_BYTE_BUF) >> 8;
+res |= (v & 0xff) << (i * 8);
+}
+
+writel(baseaddr + A_I2CD_CMD, A_I2CD_M_STOP_CMD);
+
+return res;
+}
+
+uint32_t aspeed_i2c_readl(uint32_t baseaddr, uint8_t slave_addr, uint8_t reg)
+{
+return aspeed_i2c_read_n(baseaddr, slave_addr, reg, sizeof(uint32_t));
+}
+
+uint16_t aspeed_i2c_readw(uint32_t baseaddr, uint8_t slave_addr, uint8_t reg)
+{
+return aspeed_i2c_read_n(baseaddr, slave_addr, reg, sizeof(uint16_t));
+}
+
+uint8_t aspeed_i2c_readb(uint32_t baseaddr, uint8_t slave_addr, uint8_t reg)
+{
+return aspeed_i2c_read_n(baseaddr, slave_addr, reg, sizeof(uint8_t));
+}
+
+static void aspeed_i2c_write_n(uint32_t baseaddr, uint8_t slave_addr,
+   uint8_t reg, uint32_t v, size_t nbytes)
+{
+size_t i;
+
+aspeed_i2c_startup(baseaddr, slave_addr, reg);
+
+for (i = 0; i < nbytes; i++) {
+writel(baseaddr + A_I

Re: [PATCH v7 0/3] Add support for TPM devices over I2C bus

[Joel Stanley]

Hi Ninad,

On Sun, 26 Mar 2023 at 22:44, Ninad Palsule  wrote:
>
> Hello,
>
> I have incorporated review comments from Stefan. Please review.
>
> This drop adds support for the TPM devices attached to the I2C bus. It
> only supports the TPM2 protocol. You need to run it with the external
> TPM emulator like swtpm. I have tested it with swtpm.

Nice work. I tested these stop cedric's aspeed-8.0 qemu tree, using
the rainier machine and the openbmc dev-6.1 kernel.

We get this message when booting from a kernel:

[0.582699] tpm_tis_i2c 12-002e: 2.0 TPM (device-id 0x1, rev-id 1)
[0.586361] tpm tpm0: A TPM error (256) occurred attempting the self test
[0.586623] tpm tpm0: starting up the TPM manually

Do we understand why the error appears?

# grep -r . /sys/class/tpm/tpm0/pcr-sha256/ | sort -n -k 7 -t /
/sys/class/tpm/tpm0/pcr-sha256/0:
/sys/class/tpm/tpm0/pcr-sha256/1:
/sys/class/tpm/tpm0/pcr-sha256/2:
/sys/class/tpm/tpm0/pcr-sha256/3:
/sys/class/tpm/tpm0/pcr-sha256/4:
/sys/class/tpm/tpm0/pcr-sha256/5:
/sys/class/tpm/tpm0/pcr-sha256/6:
/sys/class/tpm/tpm0/pcr-sha256/7:
/sys/class/tpm/tpm0/pcr-sha256/8:
/sys/class/tpm/tpm0/pcr-sha256/9:
/sys/class/tpm/tpm0/pcr-sha256/10:
/sys/class/tpm/tpm0/pcr-sha256/11:
/sys/class/tpm/tpm0/pcr-sha256/12:
/sys/class/tpm/tpm0/pcr-sha256/13:
/sys/class/tpm/tpm0/pcr-sha256/14:
/sys/class/tpm/tpm0/pcr-sha256/15:
/sys/class/tpm/tpm0/pcr-sha256/16:
/sys/class/tpm/tpm0/pcr-sha256/17:
/sys/class/tpm/tpm0/pcr-sha256/18:
/sys/class/tpm/tpm0/pcr-sha256/19:
/sys/class/tpm/tpm0/pcr-sha256/20:
/sys/class/tpm/tpm0/pcr-sha256/21:
/sys/class/tpm/tpm0/pcr-sha256/22:
/sys/class/tpm/tpm0/pcr-sha256/23:

If I boot through the openbmc u-boot for the p10bmc machine, which
measures things into the PCRs:

[0.556713] tpm_tis_i2c 12-002e: 2.0 TPM (device-id 0x1, rev-id 1)

/ # grep -r . /sys/class/tpm/tpm0/pcr-sha256/ | sort -n -k 7 -t /
/sys/class/tpm/tpm0/pcr-sha256/0:AFA13691EFC7BC6E189E92347F20676FB4523302CB957DA9A65C3430C45E8BCC
/sys/class/tpm/tpm0/pcr-sha256/1:37F0F710A5502FAE6DB7433B36001FEE1CBF15BA2A7D6923207FF56888584714
/sys/class/tpm/tpm0/pcr-sha256/2:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7B00A10214CA8FEA93
/sys/class/tpm/tpm0/pcr-sha256/3:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7B00A10214CA8FEA93
/sys/class/tpm/tpm0/pcr-sha256/4:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7B00A10214CA8FEA93
/sys/class/tpm/tpm0/pcr-sha256/5:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7B00A10214CA8FEA93
/sys/class/tpm/tpm0/pcr-sha256/6:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7B00A10214CA8FEA93
/sys/class/tpm/tpm0/pcr-sha256/7:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7B00A10214CA8FEA93
/sys/class/tpm/tpm0/pcr-sha256/8:AE67485BD01E8D6FE0208C46C473940173F66E9C6F43C75ABB404375787E9705
/sys/class/tpm/tpm0/pcr-sha256/9:DB99D92EADBB446894CB0C062AEB673F60DDAFBC62BC2A9CA561A13B31E5357C
/sys/class/tpm/tpm0/pcr-sha256/10:
/sys/class/tpm/tpm0/pcr-sha256/11:
/sys/class/tpm/tpm0/pcr-sha256/12:
/sys/class/tpm/tpm0/pcr-sha256/13:
/sys/class/tpm/tpm0/pcr-sha256/14:
/sys/class/tpm/tpm0/pcr-sha256/15:

Re: [PATCH v6 04/25] target/riscv: Remove mstatus_hs_{fs, vs} from tb_flags

[liweiwei]

On 2023/3/25 18:54, Richard Henderson wrote:

Merge with mstatus_{fs,vs}.  We might perform a redundant
assignment to one or the other field, but it's a trivial
and saves 4 bits from TB_FLAGS.

Signed-off-by: Richard Henderson 
---
  target/riscv/cpu.h| 16 +++-
  target/riscv/cpu_helper.c | 34 --
  target/riscv/translate.c  | 32 ++--
  3 files changed, 33 insertions(+), 49 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index f787145a21..d9e0eaaf9b 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -646,19 +646,17 @@ FIELD(TB_FLAGS, VL_EQ_VLMAX, 13, 1)
  FIELD(TB_FLAGS, VILL, 14, 1)
  /* Is a Hypervisor instruction load/store allowed? */
  FIELD(TB_FLAGS, HLSX, 15, 1)
-FIELD(TB_FLAGS, MSTATUS_HS_FS, 16, 2)
-FIELD(TB_FLAGS, MSTATUS_HS_VS, 18, 2)
  /* The combination of MXL/SXL/UXL that applies to the current cpu mode. */
-FIELD(TB_FLAGS, XL, 20, 2)
+FIELD(TB_FLAGS, XL, 16, 2)
  /* If PointerMasking should be applied */
-FIELD(TB_FLAGS, PM_MASK_ENABLED, 22, 1)
-FIELD(TB_FLAGS, PM_BASE_ENABLED, 23, 1)
-FIELD(TB_FLAGS, VTA, 24, 1)
-FIELD(TB_FLAGS, VMA, 25, 1)
+FIELD(TB_FLAGS, PM_MASK_ENABLED, 18, 1)
+FIELD(TB_FLAGS, PM_BASE_ENABLED, 19, 1)
+FIELD(TB_FLAGS, VTA, 20, 1)
+FIELD(TB_FLAGS, VMA, 21, 1)
  /* Native debug itrigger */
-FIELD(TB_FLAGS, ITRIGGER, 26, 1)
+FIELD(TB_FLAGS, ITRIGGER, 22, 1)
  /* Virtual mode enabled */
-FIELD(TB_FLAGS, VIRT_ENABLED, 27, 1)
+FIELD(TB_FLAGS, VIRT_ENABLED, 23, 1)
  
  #ifdef TARGET_RISCV32

  #define riscv_cpu_mxl(env)  ((void)(env), MXL_RV32)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 1e7ee9aa30..4fdd6fe021 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -45,7 +45,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong 
*pc,
  {
  CPUState *cs = env_cpu(env);
  RISCVCPU *cpu = RISCV_CPU(cs);
-
+RISCVExtStatus fs, vs;
  uint32_t flags = 0;
  
  *pc = env->xl == MXL_RV32 ? env->pc & UINT32_MAX : env->pc;

@@ -79,18 +79,12 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong 
*pc,
  }
  
  #ifdef CONFIG_USER_ONLY

-flags = FIELD_DP32(flags, TB_FLAGS, FS, EXT_STATUS_DIRTY);
-flags = FIELD_DP32(flags, TB_FLAGS, VS, EXT_STATUS_DIRTY);
+fs = EXT_STATUS_DIRTY;
+vs = EXT_STATUS_DIRTY;
  #else
  flags |= cpu_mmu_index(env, 0);
-if (riscv_cpu_fp_enabled(env)) {
-flags =  FIELD_DP32(flags, TB_FLAGS, FS,
-get_field(env->mstatus,  MSTATUS_FS));
-}
-if (riscv_cpu_vector_enabled(env)) {
-flags =  FIELD_DP32(flags, TB_FLAGS, VS,
-get_field(env->mstatus, MSTATUS_VS));
-}
+fs = get_field(env->mstatus, MSTATUS_FS);
+vs = get_field(env->mstatus, MSTATUS_VS);
  
  if (riscv_has_ext(env, RVH)) {

  if (env->priv == PRV_M ||
@@ -100,19 +94,23 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong 
*pc,
  flags = FIELD_DP32(flags, TB_FLAGS, HLSX, 1);
  }
  
-flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_FS,

-   get_field(env->mstatus_hs, MSTATUS_FS));
-
-flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_VS,
-   get_field(env->mstatus_hs, MSTATUS_VS));
-flags = FIELD_DP32(flags, TB_FLAGS, VIRT_ENABLED,
-   get_field(env->virt, VIRT_ONOFF));
+if (riscv_cpu_virt_enabled(env)) {
+flags = FIELD_DP32(flags, TB_FLAGS, VIRT_ENABLED, 1);
+/*
+ * Merge DISABLED and !DIRTY states using MIN.
+ * We will set both fields when dirtying.
+ */
+fs = MIN(fs, get_field(env->mstatus_hs, MSTATUS_FS));
+vs = MIN(vs, get_field(env->mstatus_hs, MSTATUS_VS));
+}
  }
  if (cpu->cfg.debug && !icount_enabled()) {
  flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
  }
  #endif
  
+flags = FIELD_DP32(flags, TB_FLAGS, FS, fs);

+flags = FIELD_DP32(flags, TB_FLAGS, VS, vs);
  flags = FIELD_DP32(flags, TB_FLAGS, XL, env->xl);
  if (env->cur_pmmask < (env->xl == MXL_RV32 ? UINT32_MAX : UINT64_MAX)) {
  flags = FIELD_DP32(flags, TB_FLAGS, PM_MASK_ENABLED, 1);
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index b897bf6006..74d0b9889d 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -66,8 +66,6 @@ typedef struct DisasContext {
  uint32_t opcode;
  RISCVExtStatus mstatus_fs;
  RISCVExtStatus mstatus_vs;
-RISCVExtStatus mstatus_hs_fs;
-RISCVExtStatus mstatus_hs_vs;
  uint32_t mem_idx;
  /* Remember the rounding mode encoded in the previous fp instruction,
 which we have already installed into env->fp_status.  Or -1 for
@@ -618,16 +616,12 @@ static void mark_fs_dirty(DisasContext *ctx)
  tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatu

Re: [PATCH v6 13/25] target/riscv: Introduce mmuidx_priv

[LIU Zhiwei]

On 2023/3/25 18:54, Richard Henderson wrote:

Use the priv level encoded into the mmu_idx, rather than
starting from env->priv.  We have already checked MPRV+MPP
in riscv_cpu_mmu_index -- no need to repeat that.

Signed-off-by: Richard Henderson 
---
  target/riscv/internals.h  | 9 +
  target/riscv/cpu_helper.c | 6 +-
  2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/target/riscv/internals.h b/target/riscv/internals.h
index 0b61f337dd..4aa1cb409f 100644
--- a/target/riscv/internals.h
+++ b/target/riscv/internals.h
@@ -37,6 +37,15 @@
  #define MMUIdx_M3
  #define MMU_2STAGE_BIT  (1 << 2)
  
+static inline int mmuidx_priv(int mmu_idx)

+{
+int ret = mmu_idx & 3;
+if (ret == MMUIdx_S_SUM) {
+ret = PRV_S;
+}
+return ret;
+}
+


Can we remove the PRIV from the tb flags after we have this function?

Best Regards,
Zhiwei


  static inline bool mmuidx_sum(int mmu_idx)
  {
  return (mmu_idx & 3) == MMUIdx_S_SUM;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 7e6cd8e0fd..cb260b88ea 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -771,7 +771,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr 
*physical,
   * (riscv_cpu_do_interrupt) is correct */
  MemTxResult res;
  MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
-int mode = env->priv;
+int mode = mmuidx_priv(mmu_idx);
  bool use_background = false;
  hwaddr ppn;
  RISCVCPU *cpu = env_archcpu(env);
@@ -793,10 +793,6 @@ static int get_physical_address(CPURISCVState *env, hwaddr 
*physical,
 instructions, HLV, HLVX, and HSV. */
  if (riscv_cpu_two_stage_lookup(mmu_idx)) {
  mode = get_field(env->hstatus, HSTATUS_SPVP);
-} else if (mode == PRV_M && access_type != MMU_INST_FETCH) {
-if (get_field(env->mstatus, MSTATUS_MPRV)) {
-mode = get_field(env->mstatus, MSTATUS_MPP);
-}
  }
  
  if (first_stage == false) {

Re: [PATCH v7 0/3] Add support for TPM devices over I2C bus

[Ninad Palsule]

Hi Joel,

On 3/26/23 8:05 PM, Joel Stanley wrote:

Hi Ninad,

On Sun, 26 Mar 2023 at 22:44, Ninad Palsule  wrote:

Hello,

I have incorporated review comments from Stefan. Please review.

This drop adds support for the TPM devices attached to the I2C bus. It
only supports the TPM2 protocol. You need to run it with the external
TPM emulator like swtpm. I have tested it with swtpm.

Nice work. I tested these stop cedric's aspeed-8.0 qemu tree, using
the rainier machine and the openbmc dev-6.1 kernel.

We get this message when booting from a kernel:

[0.582699] tpm_tis_i2c 12-002e: 2.0 TPM (device-id 0x1, rev-id 1)
[0.586361] tpm tpm0: A TPM error (256) occurred attempting the self test
[0.586623] tpm tpm0: starting up the TPM manually

Do we understand why the error appears?



Yes, As per kernel code this is an expected error for some emulators.

On swtpm emulator, It returns TPM2_RC_INITIALIZE if emulator is not 
initialized. I searched it in swtpm and it indicated that selftest 
requested before it is initialized. I meant to ask Stefan but busy with 
the review comments.


This function comment in the driver mentioned below indicate that this 
case possible with emulators.


/**
 * tpm2_startup - turn on the TPM
 * @chip: TPM chip to use
 *
 * Normally the firmware should start the TPM. This function is 
provided as a

 * workaround if this does not happen. A legal case for this could be for
 * example when a TPM emulator is used.
 *
 * Return: same as tpm_transmit_cmd()
 */

static int tpm2_startup(struct tpm_chip *chip)




# grep -r . /sys/class/tpm/tpm0/pcr-sha256/ | sort -n -k 7 -t /
/sys/class/tpm/tpm0/pcr-sha256/0:
/sys/class/tpm/tpm0/pcr-sha256/1:
/sys/class/tpm/tpm0/pcr-sha256/2:
/sys/class/tpm/tpm0/pcr-sha256/3:
/sys/class/tpm/tpm0/pcr-sha256/4:
/sys/class/tpm/tpm0/pcr-sha256/5:
/sys/class/tpm/tpm0/pcr-sha256/6:
/sys/class/tpm/tpm0/pcr-sha256/7:
/sys/class/tpm/tpm0/pcr-sha256/8:
/sys/class/tpm/tpm0/pcr-sha256/9:
/sys/class/tpm/tpm0/pcr-sha256/10:
/sys/class/tpm/tpm0/pcr-sha256/11:
/sys/class/tpm/tpm0/pcr-sha256/12:
/sys/class/tpm/tpm0/pcr-sha256/13:
/sys/class/tpm/tpm0/pcr-sha256/14:
/sys/class/tpm/tpm0/pcr-sha256/15:
/sys/class/tpm/tpm0/pcr-sha256/16:
/sys/class/tpm/tpm0/pcr-sha256/17:
/sys/class/tpm/tpm0/pcr-sha256/18:
/sys/class/tpm/tpm0/pcr-sha256/19:
/sys/class/tpm/tpm0/pcr-sha256/20:
/sys/class/tpm/tpm0/pcr-sha256/21:
/sys/class/tpm/tpm0/pcr-sha256/22:
/sys/class/tpm/tpm0/pcr-sha256/23:

If I boot through the openbmc u-boot for the p10bmc machine, which
measures things into the PCRs:

[0.556713] tpm_tis_i2c 12-002e: 2.0 TPM (device-id 0x1, rev-id 1)

/ # grep -r . /sys/class/tpm/tpm0/pcr-sha256/ | sort -n -k 7 -t /
/sys/class/tpm/tpm0/pcr-sha256/0:AFA13691EFC7BC6E189E92347F20676FB4523302CB957DA9A65C3430C45E8BCC
/sys/class/tpm/tpm0/pcr-sha256/1:37F0F710A5502FAE6DB7433B36001FEE1CBF15BA2A7D6923207FF56888584714
/sys/class/tpm/tpm0/pcr-sha256/2:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7B00A10214CA8FEA93
/sys/class/tpm/tpm0/pcr-sha256/3:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7B00A10214CA8FEA93
/sys/class/tpm/tpm0/pcr-sha256/4:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7B00A10214CA8FEA93
/sys/class/tpm/tpm0/pcr-sha256/5:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7B00A10214CA8FEA93
/sys/class/tpm/tpm0/pcr-sha256/6:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7B00A10214CA8FEA93
/sys/class/tpm/tpm0/pcr-sha256/7:E21B703EE69C77476BCCB43EC0336A9A1B2914B378944F7

Re: [PATCH 3/6] target/ppc: Fix instruction loading endianness in alignment interrupt

[Nicholas Piggin]

On Fri Mar 24, 2023 at 11:30 PM AEST, Fabiano Rosas wrote:
> Hi Nick,
>
> > powerpc ifetch endianness depends on MSR[LE] so it has to byteswap
> > after cpu_ldl_code(). This corrects DSISR bits in alignment
> > interrupts when running in little endian mode.
> >
>
> Just a thought, we have these tests that perhaps could have caught
> this:  https://github.com/legoater/pnv-test
>
> Despite the name they do have (some) support to pseries as well. Not
> sure how the P8 support is these days though.

Hey Fabiano,

Thanks for the review. Good thinking, and actually I did catch some
of these (the SPR size one) when running kvm-unit-tests with TCG. I
ported it to powernv too. I wonder if we should merge pnv-test into
kvm-unit-tests.

> > Signed-off-by: Nicholas Piggin 
> > ---
> >  target/ppc/excp_helper.c | 27 ++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 287659c74d..5f0e363363 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -133,6 +133,31 @@ static void dump_hcall(CPUPPCState *env)
> >env->nip);
> >  }
> >  
> > +/* Return true iff byteswap is needed in a scalar memop */
> > +static inline bool need_byteswap(CPUArchState *env)
> > +{
> > +#if TARGET_BIG_ENDIAN
>
> TARGET_BIG_ENDIAN is always set for softmmu mode. See
> configs/targets/ppc64-softmmu.mak

I see, I just took it from translate.c and actually stupidly forgot
to actually call it here :) I'll fix it up.

Thanks,
Nick

Re: [PATCH 4/6] target/ppc: Alignment faults do not set DSISR in ISA v3.0 onward

[Nicholas Piggin]

On Fri Mar 24, 2023 at 11:39 PM AEST, Fabiano Rosas wrote:
> Nicholas Piggin  writes:
>
> > This optional behavior was removed from the ISA in v3.0, see
> > Summary of Changes preface:
> >
> >   Data Storage Interrupt Status Register for Alignment Interrupt:
> >   Simplifies the Alignment interrupt by remov- ing the Data Storage
> >   Interrupt Status Register (DSISR) from the set of registers modified
> >   by the Alignment interrupt.
> >
> > Signed-off-by: Nicholas Piggin 
> > ---
> >  target/ppc/excp_helper.c | 23 ---
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 5f0e363363..c8b8eca3b1 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -1456,13 +1456,22 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int 
> > excp)
> >  break;
> >  }
> >  case POWERPC_EXCP_ALIGN: /* Alignment exception
> >   */
> > -/* Get rS/rD and rA from faulting opcode */
> > -/*
> > - * Note: the opcode fields will not be set properly for a
> > - * direct store load/store, but nobody cares as nobody
> > - * actually uses direct store segments.
> > - */
> > -env->spr[SPR_DSISR] |= (env->error_code & 0x03FF) >> 16;
> > +switch (env->excp_model) {
>
> Slightly better would be to check on (env->insn_flags2 & PPC2_ISA300).
> We were trying to phase out the usage of "exception models" wherever
> possible in favor of specific feature/isa level flags.

Oh good point, thanks for catching that. Will fix and resend the series
(I've done the same thing in a few other places too).

Thanks,
Nick

Re: [PATCH v4 06/10] migration: Introduce dirty-limit capability

[Markus Armbruster]

Hyman Huang  writes:

> 在 2023/3/24 22:32, Markus Armbruster 写道:
>> Hyman Huang  writes:
>> 
>>> 在 2023/3/24 20:11, Markus Armbruster 写道:
 huang...@chinatelecom.cn writes:

> From: Hyman Huang(黄勇) 
>
> Introduce migration dirty-limit capability, which can
> be turned on before live migration and limit dirty
> page rate durty live migration.
>
> Introduce migrate_dirty_limit function to help check
> if dirty-limit capability enabled during live migration.
>
> Meanwhile, refactor vcpu_dirty_rate_stat_collect
> so that period can be configured instead of hardcoded.
>
> dirty-limit capability is kind of like auto-converge
> but using dirty limit instead of traditional cpu-throttle
> to throttle guest down. To enable this feature, turn on
> the dirty-limit capability before live migration using
> migrate-set-capabilities, and set the parameters
> "x-vcpu-dirty-limit-period", "vcpu-dirty-limit" suitably
> to speed up convergence.
>
> Signed-off-by: Hyman Huang(黄勇) 
> Acked-by: Peter Xu 
 [...]

> diff --git a/qapi/migration.json b/qapi/migration.json
> index d33cc2d582..b7a92be055 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -477,6 +477,8 @@
>#will be handled faster.  This is a performance 
> feature and
>#should not affect the correctness of postcopy 
> migration.
>#(since 7.1)
> +# @dirty-limit: Use dirty-limit to throttle down guest if enabled.
> +#   (since 8.0)

 Feels too terse.  What exactly is used and how?  It's not the capability
 itself (although the text sure sounds like it).  I guess it's the thing
 you set with command set-vcpu-dirty-limit.

 Is that used only when the capability is set?
>>>
>>> Yes, live migration set "dirty-limit" value when that capability is set,
>>> the comment changes to "Apply the algorithm of dirty page rate limit to 
>>> throttle down guest if capability is set, rather than auto-converge".
>>>
>>> Please continue to polish the doc if needed. Thanks.
>>
>> Let's see whether I understand.
>>
>> Throttling happens only during migration.
>>
>> There are two throttling algorithms: "auto-converge" (default) and
>> "dirty page rate limit".
>>
>> The latter can be tuned with set-vcpu-dirty-limit.
>> Correct?
>
> Yes
>
>> What happens when migration capability dirty-limit is enabled, but the
>> user hasn't set a limit with set-vcpu-dirty-limit, or canceled it with
>> cancel-vcpu-dirty-limit?
>
> dirty-limit capability use the default value if user hasn't set.

What is the default value?  I can't find it in the doc comments.

> In the path of cancel-vcpu-dirty-limit, canceling should be check and not be 
> allowed if migration is in process.

Can you change the dirty limit with set-vcpu-dirty-limit while migration
is in progress?  Let's see...

Has the dirty limit any effect while migration is not in progress?

> see the following code in commit:
> [PATCH v4 08/10] migration: Implement dirty-limit convergence algo
>
> --- a/softmmu/dirtylimit.c
> +++ b/softmmu/dirtylimit.c
> @@ -438,6 +438,8 @@ void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index,
>   int64_t cpu_index,
>   Error **errp)
>  {
> +MigrationState *ms = migrate_get_current();
> +
>  if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
>  return;
>  }
> @@ -451,6 +453,15 @@ void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index,
>  return;
>  }
>
> +if (migration_is_running(ms->state) &&
> +(!qemu_thread_is_self(&ms->thread)) &&
> +migrate_dirty_limit() &&
> +dirtylimit_in_service()) {
> +error_setg(errp, "can't cancel dirty page limit while"
> +   " migration is running");
> +return;
> +}

We can get here even when migration_is_running() is true.  Seems to
contradict your claim "no cancel while migration is in progress".  Am I
confused?

Please drop the superfluous parenthesis around !qemu_thread_is_self().

> +
>  dirtylimit_state_lock();
>
>  if (has_cpu_index) {
> @@ -486,6 +497,8 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index,
>uint64_t dirty_rate,
>Error **errp)
>  {
> +MigrationState *ms = migrate_get_current();
> +
>  if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
>  error_setg(errp, "dirty page limit feature requires KVM with"
> " accelerator property 'dirty-ring-size' set'");
> @@ -502,6 +515,15 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index,
>  return;
>  }
>
> +if (migration_is_running(ms->state) &&
> +(!qemu_thread_is_self(&ms->thread)) &&
> +migrate_dirty_limit() &&
> +dirtylimit_in_service()) {
> +error_setg(er