Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Stefan Berger]

On 01/20/2018 07:54 AM, Philippe Mathieu-Daudé wrote:

On 01/19/2018 11:11 AM, Marc-André Lureau wrote:

tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB)
Interface as defined in TCG PC Client Platform TPM Profile (PTP)
Specification Family “2.0” Level 00 Revision 01.03 v22.

The PTP allows device implementation to switch between TIS and CRB
model at run time, but given that CRB is a simpler device to
implement, I chose to implement it as a different device.

The device doesn't implement other locality than 0 for now (my laptop
TPM doesn't either, so I assume this isn't so bad)

The command/reply memory region is statically allocated after the CRB
registers address TPM_CRB_ADDR_BASE + sizeof(struct crb_regs) (I
wonder if the BIOS could or should allocate it instead, or what size
to use, again this seems to fit well expectations)

The PTP doesn't specify a particular bus to put the device. So I added
it on the system bus directly, so it could hopefully be used easily on
a different platform than x86. Currently, it fails to init on piix,
because error_on_sysbus_device() check. The check may be changed in a
near future, see discussion on the qemu-devel ML.

Tested with some success with Linux upstream and Windows 10, seabios &
modified ovmf. The device is recognized and correctly transmit
command/response with passthrough & emu. However, we are missing PPI
ACPI part atm.

Signed-off-by: Marc-André Lureau 
Signed-off-by: Stefan Berger 
---
  qapi/tpm.json  |   5 +-
  include/hw/acpi/tpm.h  |  72 
  include/sysemu/tpm.h   |   3 +
  hw/i386/acpi-build.c   |  34 +++-
  hw/tpm/tpm_crb.c   | 327 +
  default-configs/i386-softmmu.mak   |   1 +
  default-configs/x86_64-softmmu.mak |   1 +
  hw/tpm/Makefile.objs   |   1 +
  8 files changed, 434 insertions(+), 10 deletions(-)
  create mode 100644 hw/tpm/tpm_crb.c

diff --git a/qapi/tpm.json b/qapi/tpm.json
index 7093f268fb..d50deef5e9 100644
--- a/qapi/tpm.json
+++ b/qapi/tpm.json
@@ -11,10 +11,11 @@
  # An enumeration of TPM models
  #
  # @tpm-tis: TPM TIS model
+# @tpm-crb: TPM CRB model (since 2.12)
  #
  # Since: 1.5
  ##
-{ 'enum': 'TpmModel', 'data': [ 'tpm-tis' ] }
+{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb' ] }
  
  ##

  # @query-tpm-models:
@@ -28,7 +29,7 @@
  # Example:
  #
  # -> { "execute": "query-tpm-models" }
-# <- { "return": [ "tpm-tis" ] }
+# <- { "return": [ "tpm-tis", "tpm-crb" ] }
  #
  ##
  { 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index 6d516c6a7f..b0048515fa 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -16,11 +16,82 @@
  #ifndef HW_ACPI_TPM_H
  #define HW_ACPI_TPM_H
  
+#include "qemu/osdep.h"

+
  #define TPM_TIS_ADDR_BASE   0xFED4
  #define TPM_TIS_ADDR_SIZE   0x5000
  
  #define TPM_TIS_IRQ 5
  
+struct crb_regs {

+union {
+uint32_t reg;
+struct {
+unsigned tpm_established:1;
+unsigned loc_assigned:1;
+unsigned active_locality:3;
+unsigned reserved:2;
+unsigned tpm_reg_valid_sts:1;
+} bits;

I suppose this is little-endian layout, so this won't work on big-endian
hosts.

Can you add a qtest?


+} loc_state;
+uint32_t reserved1;
+uint32_t loc_ctrl;
+union {
+uint32_t reg;
+struct {
+unsigned granted:1;
+unsigned been_seized:1;
+} bits;


This is unclear where you expect those bits (right/left aligned).

Can you add an unnamed field to be more explicit?

i.e. without using struct if left alignment expected:

unsigned granted:1, been_seized:1, :30;



I got rid of all the bitfields and this patch here makes it work on a 
ppc64 big endian and also x86_64 host:


https://github.com/stefanberger/qemu-tpm/commit/28fc07f0d9314168986190effd6d72d9fd3972dd

Regards,
Stefan





+} loc_sts;
+uint8_t reserved2[32];
+union {
+uint64_t reg;
+struct {
+unsigned type:4;
+unsigned version:4;
+unsigned cap_locality:1;
+unsigned cap_crb_idle_bypass:1;
+unsigned reserved1:1;
+unsigned cap_data_xfer_size_support:2;
+unsigned cap_fifo:1;
+unsigned cap_crb:1;
+unsigned cap_if_res:2;
+unsigned if_selector:2;
+unsigned if_selector_lock:1;
+unsigned reserved2:4;
+unsigned rid:8;
+unsigned vid:16;
+unsigned did:16;
+} bits;
+} intf_id;
+uint64_t ctrl_ext;
+
+uint32_t ctrl_req;
+union {
+uint32_t reg;
+struct {
+unsigned tpm_sts:1;
+unsigned tpm_idle:1;
+unsigned reserved:30;

Oh here you 

[Qemu-devel] [PULL 07/12] spapr: fix device tree properties when using compatibility mode

[David Gibson]

From: Greg Kurz 

Commit 51f84465dd98 changed the compatility mode setting logic:
- machine reset only sets compatibility mode for the boot CPU
- compatibility mode is set for other CPUs when they are put online
  by the guest with the "start-cpu" RTAS call

This causes a regression for machines started with max-compat-cpu:
the device tree nodes related to secondary CPU cores contain wrong
"cpu-version" and "ibm,pa-features" values, as shown below.

Guest started on a POWER8 host with:
 -smp cores=2 -machine pseries,max-cpu-compat=compat7

ibm,pa-features = [18 00 f6 3f c7 c0 80 f0 80 00
 00 00 00 00 00 00 00 00 80 00 80 00 80 00 00 00];
cpu-version = <0x4d0200>;

   ^^^
second CPU core

ibm,pa-features = <0x600f63f 0xc70080c0>;
cpu-version = <0xf03>;

   ^^^
  boot CPU core

The second core is advertised in raw POWER8 mode. This happens because
CAS assumes all CPUs to have the same compatibility mode. Since the
boot CPU already has the requested compatibility mode, the CAS code
does not set it for the secondary one, and exposes the bogus device
tree properties in in the CAS response to the guest.

A similar situation is observed when hot-plugging a CPU core. The
related device tree properties are generated and exposed to guest
with the "ibm,configure-connector" RTAS before "start-cpu" is called.
The CPU core is advertised to the guest in raw mode as well.

It both cases, it boils down to the fact that "start-cpu" happens too
late. This can be fixed globally by propagating the compatibility mode
of the boot CPU to the other CPUs during reset.  For this to work, the
compatibility mode of the boot CPU must be set before the machine code
actually resets all CPUs.

It is not needed to set the compatibility mode in "start-cpu" anymore,
so the code is dropped.

Fixes: 51f84465dd98
Signed-off-by: Greg Kurz 
Signed-off-by: David Gibson 
---
 hw/ppc/spapr.c  | 18 +-
 hw/ppc/spapr_cpu_core.c |  7 +++
 hw/ppc/spapr_rtas.c |  9 -
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fe38c56ff3..88a78d31eb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1484,6 +1484,15 @@ static void spapr_machine_reset(void)
 spapr_setup_hpt_and_vrma(spapr);
 }
 
+/* if this reset wasn't generated by CAS, we should reset our
+ * negotiated options and start from scratch */
+if (!spapr->cas_reboot) {
+spapr_ovec_cleanup(spapr->ov5_cas);
+spapr->ov5_cas = spapr_ovec_new();
+
+ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, _fatal);
+}
+
 qemu_devices_reset();
 
 /* DRC reset may cause a device to be unplugged. This will cause troubles
@@ -1504,15 +1513,6 @@ static void spapr_machine_reset(void)
 rtas_addr = rtas_limit - RTAS_MAX_SIZE;
 fdt_addr = rtas_addr - FDT_MAX_SIZE;
 
-/* if this reset wasn't generated by CAS, we should reset our
- * negotiated options and start from scratch */
-if (!spapr->cas_reboot) {
-spapr_ovec_cleanup(spapr->ov5_cas);
-spapr->ov5_cas = spapr_ovec_new();
-
-ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, _fatal);
-}
-
 fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
 
 spapr_load_rtas(spapr, fdt, rtas_addr);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index ac19b2e0b7..590d167b04 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -44,6 +44,13 @@ static void spapr_cpu_reset(void *opaque)
 if (cs != first_cpu) {
 env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
 }
+
+/* Set compatibility mode to match the boot CPU, which was either set
+ * by the machine reset code or by CAS. This should never fail.
+ */
+if (cs != first_cpu) {
+ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, _abort);
+}
 }
 
 static void spapr_cpu_destroy(PowerPCCPU *cpu)
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 2b89e1d448..4bb939d3d1 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -163,7 +163,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, 
sPAPRMachineState *spapr,
 CPUState *cs = CPU(cpu);
 CPUPPCState *env = >env;
 PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
-Error *local_err = NULL;
 
 if (!cs->halted) {
 rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
@@ -175,14 +174,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, 
sPAPRMachineState *spapr,
  * new cpu enters */
 kvm_cpu_synchronize_state(cs);
 
-/* Set compatibility mode to match existing cpus */
-ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, _err);
-if (local_err) {
-

[Qemu-devel] [PULL 05/12] target/ppc: msgsnd and msgclr instructions need hypervisor privilege

[David Gibson]

From: Cédric Le Goater 

Signed-off-by: Cédric Le Goater 
Signed-off-by: David Gibson 
---
 target/ppc/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 0ef21cce33..396f422cf4 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6174,7 +6174,7 @@ static void gen_msgclr(DisasContext *ctx)
 #if defined(CONFIG_USER_ONLY)
 GEN_PRIV;
 #else
-CHK_SV;
+CHK_HV;
 gen_helper_msgclr(cpu_env, cpu_gpr[rB(ctx->opcode)]);
 #endif /* defined(CONFIG_USER_ONLY) */
 }
@@ -6184,7 +6184,7 @@ static void gen_msgsnd(DisasContext *ctx)
 #if defined(CONFIG_USER_ONLY)
 GEN_PRIV;
 #else
-CHK_SV;
+CHK_HV;
 gen_helper_msgsnd(cpu_gpr[rB(ctx->opcode)]);
 #endif /* defined(CONFIG_USER_ONLY) */
 }
-- 
2.14.3

[Qemu-devel] [PULL 08/12] target-ppc: optimize cmp translation

[David Gibson]

From: "pbonz...@redhat.com" 

We know that only one bit (in addition to SO) is going to be set in
the condition register, so do two movconds instead of three setconds,
three shifts and two ORs.

For ppc64-linux-user, the code size reduction is around 5% and the
performance improvement slightly less than 10%.  For softmmu, the
improvement is around 5%.

Signed-off-by: Paolo Bonzini 
Signed-off-by: David Gibson 
---
 target/ppc/translate.c | 29 -
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 396f422cf4..bcd36d5353 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -605,27 +605,22 @@ static opc_handler_t invalid_handler = {
 static inline void gen_op_cmp(TCGv arg0, TCGv arg1, int s, int crf)
 {
 TCGv t0 = tcg_temp_new();
-TCGv_i32 t1 = tcg_temp_new_i32();
-
-tcg_gen_trunc_tl_i32(cpu_crf[crf], cpu_so);
-
-tcg_gen_setcond_tl((s ? TCG_COND_LT: TCG_COND_LTU), t0, arg0, arg1);
-tcg_gen_trunc_tl_i32(t1, t0);
-tcg_gen_shli_i32(t1, t1, CRF_LT_BIT);
-tcg_gen_or_i32(cpu_crf[crf], cpu_crf[crf], t1);
+TCGv t1 = tcg_temp_new();
+TCGv_i32 t = tcg_temp_new_i32();
 
-tcg_gen_setcond_tl((s ? TCG_COND_GT: TCG_COND_GTU), t0, arg0, arg1);
-tcg_gen_trunc_tl_i32(t1, t0);
-tcg_gen_shli_i32(t1, t1, CRF_GT_BIT);
-tcg_gen_or_i32(cpu_crf[crf], cpu_crf[crf], t1);
+tcg_gen_movi_tl(t0, CRF_EQ);
+tcg_gen_movi_tl(t1, CRF_LT);
+tcg_gen_movcond_tl((s ? TCG_COND_LT : TCG_COND_LTU), t0, arg0, arg1, t1, 
t0);
+tcg_gen_movi_tl(t1, CRF_GT);
+tcg_gen_movcond_tl((s ? TCG_COND_GT : TCG_COND_GTU), t0, arg0, arg1, t1, 
t0);
 
-tcg_gen_setcond_tl(TCG_COND_EQ, t0, arg0, arg1);
-tcg_gen_trunc_tl_i32(t1, t0);
-tcg_gen_shli_i32(t1, t1, CRF_EQ_BIT);
-tcg_gen_or_i32(cpu_crf[crf], cpu_crf[crf], t1);
+tcg_gen_trunc_tl_i32(t, t0);
+tcg_gen_trunc_tl_i32(cpu_crf[crf], cpu_so);
+tcg_gen_or_i32(cpu_crf[crf], cpu_crf[crf], t);
 
 tcg_temp_free(t0);
-tcg_temp_free_i32(t1);
+tcg_temp_free(t1);
+tcg_temp_free_i32(t);
 }
 
 static inline void gen_op_cmpi(TCGv arg0, target_ulong arg1, int s, int crf)
-- 
2.14.3

[Qemu-devel] [PULL 11/12] target/ppc: add support for hypervisor doorbells on book3s CPUs

[David Gibson]

From: Cédric Le Goater 

The hypervisor doorbells are used by skiboot and Linux on POWER9
processors to wake up secondaries.

This adds processor control support to the Server architecture by
reusing the Embedded support. They are very similar, only the bits
definition of the CPU identifier differ.

Still to be done is message broadcast to all threads of the same
processor.

Signed-off-by: Cédric Le Goater 
Signed-off-by: David Gibson 
---
 target/ppc/cpu.h|  8 +--
 target/ppc/excp_helper.c| 52 +
 target/ppc/helper.h |  2 ++
 target/ppc/translate.c  | 25 --
 target/ppc/translate_init.c |  2 +-
 5 files changed, 84 insertions(+), 5 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index b8f4dfc108..603a38cae8 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -930,7 +930,7 @@ enum {
 #define BOOKE206_MAX_TLBN  4
 
 /*/
-/* Embedded.Processor Control */
+/* Server and Embedded Processor Control */
 
 #define DBELL_TYPE_SHIFT   27
 #define DBELL_TYPE_MASK(0x1f << DBELL_TYPE_SHIFT)
@@ -940,11 +940,15 @@ enum {
 #define DBELL_TYPE_G_DBELL_CRIT(0x03 << DBELL_TYPE_SHIFT)
 #define DBELL_TYPE_G_DBELL_MC  (0x04 << DBELL_TYPE_SHIFT)
 
-#define DBELL_BRDCAST  (1 << 26)
+#define DBELL_TYPE_DBELL_SERVER(0x05 << DBELL_TYPE_SHIFT)
+
+#define DBELL_BRDCAST  PPC_BIT(37)
 #define DBELL_LPIDTAG_SHIFT14
 #define DBELL_LPIDTAG_MASK (0xfff << DBELL_LPIDTAG_SHIFT)
 #define DBELL_PIRTAG_MASK  0x3fff
 
+#define DBELL_PROCIDTAG_MASK   PPC_BITMASK(44, 63)
+
 /*/
 /* Segment page size information, used by recent hash MMUs
  * The format of this structure mirrors kvm_ppc_smmu_info
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 4e548a4487..c092fbead0 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -417,6 +417,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
excp_model, int excp)
 case POWERPC_EXCP_HISI:  /* Hypervisor instruction storage exception */
 case POWERPC_EXCP_HDSEG: /* Hypervisor data segment exception*/
 case POWERPC_EXCP_HISEG: /* Hypervisor instruction segment exception */
+case POWERPC_EXCP_SDOOR_HV:  /* Hypervisor Doorbell interrupt*/
 case POWERPC_EXCP_HV_EMU:
 srr0 = SPR_HSRR0;
 srr1 = SPR_HSRR1;
@@ -846,6 +847,11 @@ static void ppc_hw_interrupt(CPUPPCState *env)
 powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
 return;
 }
+if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDOORBELL)) {
+env->pending_interrupts &= ~(1 << PPC_INTERRUPT_HDOORBELL);
+powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_SDOOR_HV);
+return;
+}
 if (env->pending_interrupts & (1 << PPC_INTERRUPT_PERFM)) {
 env->pending_interrupts &= ~(1 << PPC_INTERRUPT_PERFM);
 powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_PERFM);
@@ -1145,4 +1151,50 @@ void helper_msgsnd(target_ulong rb)
 }
 qemu_mutex_unlock_iothread();
 }
+
+/* Server Processor Control */
+static int book3s_dbell2irq(target_ulong rb)
+{
+int msg = rb & DBELL_TYPE_MASK;
+
+/* A Directed Hypervisor Doorbell message is sent only if the
+ * message type is 5. All other types are reserved and the
+ * instruction is a no-op */
+return msg == DBELL_TYPE_DBELL_SERVER ? PPC_INTERRUPT_HDOORBELL : -1;
+}
+
+void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
+{
+int irq = book3s_dbell2irq(rb);
+
+if (irq < 0) {
+return;
+}
+
+env->pending_interrupts &= ~(1 << irq);
+}
+
+void helper_book3s_msgsnd(target_ulong rb)
+{
+int irq = book3s_dbell2irq(rb);
+int pir = rb & DBELL_PROCIDTAG_MASK;
+CPUState *cs;
+
+if (irq < 0) {
+return;
+}
+
+qemu_mutex_lock_iothread();
+CPU_FOREACH(cs) {
+PowerPCCPU *cpu = POWERPC_CPU(cs);
+CPUPPCState *cenv = >env;
+
+/* TODO: broadcast message to all threads of the same  processor */
+if (cenv->spr_cb[SPR_PIR].default_value == pir) {
+cenv->pending_interrupts |= 1 << irq;
+cpu_interrupt(cs, CPU_INTERRUPT_HARD);
+}
+}
+qemu_mutex_unlock_iothread();
+}
 #endif
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index bb6a94a8b3..5b739179b8 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -679,6 +679,8 @@ DEF_HELPER_FLAGS_3(store_sr, TCG_CALL_NO_RWG, void, env, 
tl, tl)
 DEF_HELPER_FLAGS_1(602_mfrom, TCG_CALL_NO_RWG_SE, tl, tl)
 DEF_HELPER_1(msgsnd, void, tl)
 DEF_HELPER_2(msgclr, void, env, tl)

[Qemu-devel] [PULL 12/12] target/ppc/spapr_caps: Add macro to generate spapr_caps migration vmstate

[David Gibson]

From: Suraj Jitindar Singh 

The vmstate description and the contained needed function for migration
of spapr_caps is the same for each cap, with the name of the cap
substituted. As such introduce a macro to allow for easier generation of
these.

Convert the three existing spapr_caps (htm, vsx, and dfp) to use this
macro.

Signed-off-by: Suraj Jitindar Singh 
Signed-off-by: David Gibson 
---
 hw/ppc/spapr_caps.c | 78 +
 1 file changed, 24 insertions(+), 54 deletions(-)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index d5c9ce774a..5d52969bd5 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -228,62 +228,32 @@ int spapr_caps_post_migration(sPAPRMachineState *spapr)
 return ok ? 0 : -EINVAL;
 }
 
-static bool spapr_cap_htm_needed(void *opaque)
-{
-sPAPRMachineState *spapr = opaque;
-
-return spapr->cmd_line_caps[SPAPR_CAP_HTM] &&
-   (spapr->eff.caps[SPAPR_CAP_HTM] != spapr->def.caps[SPAPR_CAP_HTM]);
-}
-
-const VMStateDescription vmstate_spapr_cap_htm = {
-.name = "spapr/cap/htm",
-.version_id = 1,
-.minimum_version_id = 1,
-.needed = spapr_cap_htm_needed,
-.fields = (VMStateField[]) {
-VMSTATE_UINT8(mig.caps[SPAPR_CAP_HTM], sPAPRMachineState),
-VMSTATE_END_OF_LIST()
-},
-};
-
-static bool spapr_cap_vsx_needed(void *opaque)
-{
-sPAPRMachineState *spapr = opaque;
-
-return spapr->cmd_line_caps[SPAPR_CAP_VSX] &&
-   (spapr->eff.caps[SPAPR_CAP_VSX] != spapr->def.caps[SPAPR_CAP_VSX]);
+/* Used to generate the migration field and needed function for a spapr cap */
+#define SPAPR_CAP_MIG_STATE(cap, ccap)  \
+static bool spapr_cap_##cap##_needed(void *opaque)  \
+{   \
+sPAPRMachineState *spapr = opaque;  \
+\
+return spapr->cmd_line_caps[SPAPR_CAP_##ccap] &&\
+   (spapr->eff.caps[SPAPR_CAP_##ccap] !=\
+spapr->def.caps[SPAPR_CAP_##ccap]); \
+}   \
+\
+const VMStateDescription vmstate_spapr_cap_##cap = {\
+.name = "spapr/cap/" #cap,  \
+.version_id = 1,\
+.minimum_version_id = 1,\
+.needed = spapr_cap_##cap##_needed, \
+.fields = (VMStateField[]) {\
+VMSTATE_UINT8(mig.caps[SPAPR_CAP_##ccap],   \
+  sPAPRMachineState),   \
+VMSTATE_END_OF_LIST()   \
+},  \
 }
 
-const VMStateDescription vmstate_spapr_cap_vsx = {
-.name = "spapr/cap/vsx",
-.version_id = 1,
-.minimum_version_id = 1,
-.needed = spapr_cap_vsx_needed,
-.fields = (VMStateField[]) {
-VMSTATE_UINT8(mig.caps[SPAPR_CAP_VSX], sPAPRMachineState),
-VMSTATE_END_OF_LIST()
-},
-};
-
-static bool spapr_cap_dfp_needed(void *opaque)
-{
-sPAPRMachineState *spapr = opaque;
-
-return spapr->cmd_line_caps[SPAPR_CAP_DFP] &&
-   (spapr->eff.caps[SPAPR_CAP_DFP] != spapr->def.caps[SPAPR_CAP_DFP]);
-}
-
-const VMStateDescription vmstate_spapr_cap_dfp = {
-.name = "spapr/cap/dfp",
-.version_id = 1,
-.minimum_version_id = 1,
-.needed = spapr_cap_dfp_needed,
-.fields = (VMStateField[]) {
-VMSTATE_UINT8(mig.caps[SPAPR_CAP_DFP], sPAPRMachineState),
-VMSTATE_END_OF_LIST()
-},
-};
+SPAPR_CAP_MIG_STATE(htm, HTM);
+SPAPR_CAP_MIG_STATE(vsx, VSX);
+SPAPR_CAP_MIG_STATE(dfp, DFP);
 
 void spapr_caps_reset(sPAPRMachineState *spapr)
 {
-- 
2.14.3

[Qemu-devel] [PULL 00/12] ppc-for-2.12 queue 20180121

[David Gibson]

The following changes since commit b384cd95eb9c6f73ad84ed1bb0717a26e29cc78f:

  Merge remote-tracking branch 
'remotes/ehabkost/tags/machine-next-pull-request' into staging (2018-01-19 
16:35:25 +)

are available in the Git repository at:

  git://github.com/dgibson/qemu.git tags/ppc-for-2.12-20180121

for you to fetch changes up to 1f63ebaa91f73f469c8f107dbd266cabdbea3a40:

  target/ppc/spapr_caps: Add macro to generate spapr_caps migration vmstate 
(2018-01-20 17:15:05 +1100)

Peter, since I'm on my way to linux.conf.au, this hasn't been through
my full usual test regimen, just a quick sanity check.  I'm hoping
this is ok, since the only changes since the superseded pull request
(which did pass the full set of tests) is a clean rebase and removal
of Thomas' patch deprecating ppcemb.


ppc patch queue 2018-01-21

This request supersedes the one from 2018-01-19.  The only difference
is that the patch deprecating ppcemb-softmmu, and thereby creating
many annying warnings from make check has been removed.

Highlights are:
  * Significant TCG speedup by optimizing cmp generation
  * Fix a regression caused by recent change to set compat mode on
hotplugged cpus
  * Cleanup of default configs
  * Some implementation of msgsnd/msgrcv instructions for server chips


BALATON Zoltan (2):
  sm501: Add missing break to case
  sii3112: Add explicit type casts to avoid unintended sign extension

Cédric Le Goater (3):
  target/ppc: fix doorbell and hypervisor doorbell definitions
  target/ppc: msgsnd and msgclr instructions need hypervisor privilege
  target/ppc: add support for hypervisor doorbells on book3s CPUs

Greg Kurz (2):
  spapr: drop duplicate variable in spapr_core_plug()
  spapr: fix device tree properties when using compatibility mode

Suraj Jitindar Singh (1):
  target/ppc/spapr_caps: Add macro to generate spapr_caps migration vmstate

Thomas Huth (3):
  default-configs/ppc64-softmmu: Include 32-bit configs instead of copying 
them
  default-configs/ppc-softmmu: Restructure the switches according to the 
machines
  hw/ppc/Makefile: Add a way to disable the PPC4xx boards

pbonz...@redhat.com (1):
  target-ppc: optimize cmp translation

 default-configs/ppc-softmmu.mak   | 59 +++--
 default-configs/ppc64-softmmu.mak | 61 --
 hw/display/sm501.c|  1 +
 hw/ide/sii3112.c  | 10 ++---
 hw/ppc/Makefile.objs  |  4 +-
 hw/ppc/spapr.c| 22 +--
 hw/ppc/spapr_caps.c   | 78 ---
 hw/ppc/spapr_cpu_core.c   |  7 
 hw/ppc/spapr_rtas.c   |  9 -
 target/ppc/cpu.h  | 16 +---
 target/ppc/excp_helper.c  | 52 ++
 target/ppc/helper.h   |  2 +
 target/ppc/translate.c| 58 ++---
 target/ppc/translate_init.c   |  2 +-
 14 files changed, 191 insertions(+), 190 deletions(-)

[Qemu-devel] [PULL 02/12] default-configs/ppc-softmmu: Restructure the switches according to the machines

[David Gibson]

From: Thomas Huth 

Order the CONFIG switches in ppc-softmmu.mak according to the machine
classes where they are used (embedded, Mac or PReP), so that it is
easier for the users to disable a set of switches completely if they
are not needed.

Also add the missing CONFIG_IDE_SII3112 switch to the embedded section
which was previously only added to ppcemb-softmmu.mak.

And while we're at it, also remove the CONFIG_IDE_CMD646 switch since
this controller does not seem to be used by any ppc machine in QEMU.

Signed-off-by: Thomas Huth 
Signed-off-by: David Gibson 
---
 default-configs/ppc-softmmu.mak | 59 ++---
 1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index bb225c6e46..3baed6a8fd 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -3,52 +3,57 @@
 include pci.mak
 include sound.mak
 include usb.mak
+
+# For embedded PPCs:
 CONFIG_PPC4XX=y
-CONFIG_ESCC=y
 CONFIG_M48T59=y
 CONFIG_SERIAL=y
-CONFIG_PARALLEL=y
-CONFIG_I8254=y
-CONFIG_PCKBD=y
-CONFIG_FDC=y
 CONFIG_I8257=y
-CONFIG_I82374=y
 CONFIG_OPENPIC=y
-CONFIG_PREP_PCI=y
-CONFIG_I82378=y
-CONFIG_PC87312=y
-CONFIG_MACIO=y
-CONFIG_SUNGEM=y
-CONFIG_PCSPK=y
-CONFIG_CS4231A=y
-CONFIG_CUDA=y
-CONFIG_ADB=y
-CONFIG_MAC_NVRAM=y
-CONFIG_MAC_DBDMA=y
-CONFIG_HEATHROW_PIC=y
-CONFIG_GRACKLE_PCI=y
-CONFIG_UNIN_PCI=y
-CONFIG_DEC_PCI=y
 CONFIG_PPCE500_PCI=y
-CONFIG_IDE_ISA=y
-CONFIG_IDE_CMD646=y
-CONFIG_IDE_MACIO=y
-CONFIG_NE2000_ISA=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_PFLASH_CFI02=y
 CONFIG_PTIMER=y
 CONFIG_I8259=y
 CONFIG_XILINX=y
 CONFIG_XILINX_ETHLITE=y
-CONFIG_PREP=y
-CONFIG_MAC=y
 CONFIG_E500=y
 CONFIG_OPENPIC_KVM=$(call land,$(CONFIG_E500),$(CONFIG_KVM))
 CONFIG_PLATFORM_BUS=y
 CONFIG_ETSEC=y
 CONFIG_SM501=y
+CONFIG_IDE_SII3112=y
+
+# For Macs
+CONFIG_MAC=y
+CONFIG_ESCC=y
+CONFIG_MACIO=y
+CONFIG_SUNGEM=y
+CONFIG_CUDA=y
+CONFIG_ADB=y
+CONFIG_MAC_NVRAM=y
+CONFIG_MAC_DBDMA=y
+CONFIG_HEATHROW_PIC=y
+CONFIG_GRACKLE_PCI=y
+CONFIG_UNIN_PCI=y
+CONFIG_DEC_PCI=y
+CONFIG_IDE_MACIO=y
+
 # For PReP
+CONFIG_PREP=y
+CONFIG_PREP_PCI=y
 CONFIG_SERIAL_ISA=y
 CONFIG_MC146818RTC=y
 CONFIG_ISA_TESTDEV=y
 CONFIG_RS6000_MC=y
+CONFIG_PARALLEL=y
+CONFIG_I82374=y
+CONFIG_I82378=y
+CONFIG_I8254=y
+CONFIG_PCKBD=y
+CONFIG_FDC=y
+CONFIG_NE2000_ISA=y
+CONFIG_PC87312=y
+CONFIG_PCSPK=y
+CONFIG_IDE_ISA=y
+CONFIG_CS4231A=y
-- 
2.14.3

[Qemu-devel] [PULL 06/12] spapr: drop duplicate variable in spapr_core_plug()

[David Gibson]

From: Greg Kurz 

A variable is already defined at the begining of the function to
hold a pointer to the CPU core object:

sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));

No need to define it again in the pre-2.10 compatibility code snipplet.

Signed-off-by: Greg Kurz 
Signed-off-by: David Gibson 
---
 hw/ppc/spapr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a781dd22e7..fe38c56ff3 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3357,9 +3357,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, 
DeviceState *dev,
 int i;
 
 for (i = 0; i < cc->nr_threads; i++) {
-sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
-
-cs = CPU(sc->threads[i]);
+cs = CPU(core->threads[i]);
 pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
 }
 }
-- 
2.14.3

[Qemu-devel] [PULL 10/12] sii3112: Add explicit type casts to avoid unintended sign extension

[David Gibson]

From: BALATON Zoltan 

Noticed by Coverity

Reported-by: Peter Maydell 
Signed-off-by: BALATON Zoltan 
Signed-off-by: David Gibson 
---
 hw/ide/sii3112.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index e2f5562bb7..17aa930e39 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -79,13 +79,13 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
 val |= (d->regs[0].confstat & (1UL << 11) ? (1 << 4) : 0); /*SATAINT0*/
 val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 6) : 0); /*SATAINT1*/
 val |= (d->i.bmdma[1].status & BM_STATUS_INT ? (1 << 14) : 0);
-val |= d->i.bmdma[0].status << 16;
-val |= d->i.bmdma[1].status << 24;
+val |= (uint32_t)d->i.bmdma[0].status << 16;
+val |= (uint32_t)d->i.bmdma[1].status << 24;
 break;
 case 0x18:
 val = d->i.bmdma[1].cmd;
 val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
-val |= d->i.bmdma[1].status << 16;
+val |= (uint32_t)d->i.bmdma[1].status << 16;
 break;
 case 0x80 ... 0x87:
 if (size == 1) {
@@ -128,7 +128,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
 val = (d->i.bus[0].ifs[0].blk) ? 0x113 : 0;
 break;
 case 0x148:
-val = d->regs[0].sien << 16;
+val = (uint32_t)d->regs[0].sien << 16;
 break;
 case 0x180:
 val = d->regs[1].scontrol;
@@ -137,7 +137,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
 val = (d->i.bus[1].ifs[0].blk) ? 0x113 : 0;
 break;
 case 0x1c8:
-val = d->regs[1].sien << 16;
+val = (uint32_t)d->regs[1].sien << 16;
 break;
 default:
 val = 0;
-- 
2.14.3

[Qemu-devel] [PULL 03/12] hw/ppc/Makefile: Add a way to disable the PPC4xx boards

[David Gibson]

From: Thomas Huth 

We've got the config switch CONFIG_PPC4XX, so we should use it
in the Makefile accordingly and only include the PPC4xx boards
if this switch has been enabled. (Note: Unfortunately, the files
ppc4xx_devs.c and ppc405_uc.c still have to be included in the
build anyway to fulfil some complicated linker dependencies ...
so these are subject to a more thourough clean-up later)

Signed-off-by: Thomas Huth 
Signed-off-by: David Gibson 
---
 hw/ppc/Makefile.objs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 1faff853b7..ad1928c5d8 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -12,8 +12,8 @@ obj-y += spapr_pci_vfio.o
 endif
 obj-$(CONFIG_PSERIES) += spapr_rtas_ddw.o
 # PowerPC 4xx boards
-obj-y += ppc405_boards.o ppc4xx_devs.o ppc405_uc.o ppc440_bamboo.o
-obj-y += ppc4xx_pci.o
+obj-y += ppc4xx_devs.o ppc405_uc.o
+obj-$(CONFIG_PPC4XX) += ppc4xx_pci.o ppc405_boards.o ppc440_bamboo.o
 # PReP
 obj-$(CONFIG_PREP) += prep.o
 obj-$(CONFIG_PREP) += prep_systemio.o
-- 
2.14.3

[Qemu-devel] [PULL 01/12] default-configs/ppc64-softmmu: Include 32-bit configs instead of copying them

[David Gibson]

From: Thomas Huth 

qemu-softmmu-ppc64 is supposed to be a superset of qemu-softmmu-ppc.
However, instead of simply including the 32-bit config file, we've
duplicated all CONFIG_xxx settings there instead. This way, we've missed
some CONFIG switches in ppc64-softmmu.mak which were only added to the
32-bit config file (e.g. CONFIG_SUNGEM). Let's fix this problem by
including the 32-bit config file into the 64-bit config file instead
of duplicating all the CONFIG switches there.

Signed-off-by: Thomas Huth 
Signed-off-by: David Gibson 
---
 default-configs/ppc64-softmmu.mak | 61 +--
 1 file changed, 8 insertions(+), 53 deletions(-)

diff --git a/default-configs/ppc64-softmmu.mak 
b/default-configs/ppc64-softmmu.mak
index d1b3a6dd50..b94af6c7c6 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -1,64 +1,19 @@
 # Default configuration for ppc64-softmmu
 
-include pci.mak
-include sound.mak
-include usb.mak
-CONFIG_PPC4XX=y
-CONFIG_VIRTIO_VGA=y
-CONFIG_ESCC=y
-CONFIG_M48T59=y
+# Include all 32-bit boards
+include ppc-softmmu.mak
+
+# For PowerNV
+CONFIG_POWERNV=y
 CONFIG_IPMI=y
 CONFIG_IPMI_LOCAL=y
 CONFIG_IPMI_EXTERN=y
 CONFIG_ISA_IPMI_BT=y
-CONFIG_SERIAL=y
-CONFIG_PARALLEL=y
-CONFIG_I8254=y
-CONFIG_PCKBD=y
-CONFIG_FDC=y
-CONFIG_I8257=y
-CONFIG_I82374=y
-CONFIG_OPENPIC=y
-CONFIG_PREP_PCI=y
-CONFIG_I82378=y
-CONFIG_PC87312=y
-CONFIG_MACIO=y
-CONFIG_PCSPK=y
-CONFIG_CUDA=y
-CONFIG_ADB=y
-CONFIG_MAC_NVRAM=y
-CONFIG_MAC_DBDMA=y
-CONFIG_HEATHROW_PIC=y
-CONFIG_GRACKLE_PCI=y
-CONFIG_UNIN_PCI=y
-CONFIG_DEC_PCI=y
-CONFIG_PPCE500_PCI=y
-CONFIG_IDE_ISA=y
-CONFIG_IDE_CMD646=y
-CONFIG_IDE_MACIO=y
-CONFIG_NE2000_ISA=y
-CONFIG_PFLASH_CFI01=y
-CONFIG_PFLASH_CFI02=y
-CONFIG_PTIMER=y
-CONFIG_I8259=y
-CONFIG_XILINX=y
-CONFIG_XILINX_ETHLITE=y
-CONFIG_PSERIES=y
-CONFIG_POWERNV=y
-CONFIG_PREP=y
-CONFIG_MAC=y
-CONFIG_E500=y
-CONFIG_OPENPIC_KVM=$(call land,$(CONFIG_E500),$(CONFIG_KVM))
-CONFIG_PLATFORM_BUS=y
-CONFIG_ETSEC=y
-CONFIG_SM501=y
+
 # For pSeries
+CONFIG_PSERIES=y
+CONFIG_VIRTIO_VGA=y
 CONFIG_XICS=$(CONFIG_PSERIES)
 CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
 CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
-# For PReP
-CONFIG_SERIAL_ISA=y
-CONFIG_MC146818RTC=y
-CONFIG_ISA_TESTDEV=y
 CONFIG_MEM_HOTPLUG=y
-CONFIG_RS6000_MC=y
-- 
2.14.3

[Qemu-devel] [PULL 09/12] sm501: Add missing break to case

[David Gibson]

From: BALATON Zoltan 

Noticed by Coverity, forgotten in 5690d9ece

Reported-by: Peter Maydell 
Signed-off-by: BALATON Zoltan 
Signed-off-by: David Gibson 
---
 hw/display/sm501.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 4f7dc59b25..134cbed607 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -797,6 +797,7 @@ static uint64_t sm501_system_config_read(void *opaque, 
hwaddr addr,
 break;
 case SM501_COMMAND_LIST_STATUS:
 ret = 0x00180002; /* FIFOs are empty, everything idle */
+break;
 case SM501_IRQ_MASK:
 ret = s->irq_mask;
 break;
-- 
2.14.3

[Qemu-devel] [PULL 04/12] target/ppc: fix doorbell and hypervisor doorbell definitions

[David Gibson]

From: Cédric Le Goater 

commit f03a1af581b9 ("ppc: Fix POWER7 and POWER8 exception definitions")
introduced definitions for the server doorbell exceptions by reusing
the embedded definitions but this adds complexity in the powerpc_excp()
routine. Let's introduce specific definitions for the Server doorbells
exception.

Signed-off-by: Cédric Le Goater 
Signed-off-by: David Gibson 
---
 target/ppc/cpu.h | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 14aaa87fe8..b8f4dfc108 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -140,9 +140,6 @@ enum {
 POWERPC_EXCP_HYPPRIV  = 41, /* Embedded hypervisor priv instruction  */
 /* Vectors 42 to 63 are reserved */
 /* Exceptions defined in the PowerPC server specification*/
-/* Server doorbell variants */
-#define POWERPC_EXCP_SDOOR  POWERPC_EXCP_GDOORI
-#define POWERPC_EXCP_SDOOR_HV   POWERPC_EXCP_DOORI
 POWERPC_EXCP_RESET= 64, /* System reset exception*/
 POWERPC_EXCP_DSEG = 65, /* Data segment exception*/
 POWERPC_EXCP_ISEG = 66, /* Instruction segment exception */
@@ -189,8 +186,11 @@ enum {
 POWERPC_EXCP_HV_EMU   = 96, /* HV emulation assistance   */
 POWERPC_EXCP_HV_MAINT = 97, /* HMI   */
 POWERPC_EXCP_HV_FU= 98, /* Hypervisor Facility unavailable   */
+/* Server doorbell variants */
+POWERPC_EXCP_SDOOR= 99,
+POWERPC_EXCP_SDOOR_HV = 100,
 /* EOL   */
-POWERPC_EXCP_NB   = 99,
+POWERPC_EXCP_NB   = 101,
 /* QEMU exceptions: used internally during code translation  */
 POWERPC_EXCP_STOP = 0x200, /* stop translation   */
 POWERPC_EXCP_BRANCH   = 0x201, /* branch instruction */
-- 
2.14.3

Re: [Qemu-devel] [PULL 00/27] Migration pull

[Philippe Mathieu-Daudé]

Hi Juan,

On 01/20/2018 08:36 PM, Juan Quintela wrote:
> Peter Maydell  wrote:
>> On 19 January 2018 at 16:43, Alexey Perevalov  
>> wrote:
>>> As I remember, I tested build in QEMU's docker build system,
>>> but now I checked it on i386 Ubuntu, and yes linker says about unresolved
>>> atomic symbols. Next week, I'll have a time to investigate it deeper.
>>
>> This sounds like exactly the problem I pointed out in a previous
>> round of this patchset :-(
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg02103.html
>>
>> Ignoring comments and sending patches anyway makes me grumpy,
>> especially when the result is exactly "fails obscurely on
>> some architectures only"...
> 
> It compiles for me.  F25 i686 gcc.  I did change it to use intptr_t
> instead of uint64_t.  So, I don't know what is going on here.
> 
> Then, I have a report that clang -m32 didn't work (Mat).  I haven't been
> able yet to reproduce.  I don't have 32bits libraries installed on my
> x86_64 systems.  clang compiles for me on both 64 bits (f27) and 32bits
> native (f25, it is a long history that it is not f27).
> 
> So, I can agree that we have to fix anything that don't work, but I
> can't agree that I didn't care about comments, at least I tried to fix
> the problems you pointed me to.
> 
> What I don't have is an easier way of compiling on arm and ppc32, but I
> will search a way to do that for my pull requsets.

You can use docker to cross compile (supposed to work on any Linux):

$ make docker-test-build@debian-powerpc-cross

If you don't want to rebuild all the images the following commands are
faster to modify the codebase and rebuild more frequently:

$ mkdir -p build_ppc32

$ docker run --rm -it -v `pwd`:`pwd` -w `pwd`/build_ppc32 -u `id -u`
qemu:debian-powerpc-cross sh -c '../configure $QEMU_CONFIGURE_OPTS'
[...]

$ docker run --rm -it -v `pwd`:`pwd` -w `pwd`/build_ppc32 -u `id -u`
qemu:debian-powerpc-cross make -j4 subdir-ppc-softmmu
[...]
  CC  ppc-softmmu/gdbstub-xml.o
  CC  ppc-softmmu/trace/generated-helpers.o
  LINKppc-softmmu/qemu-system-ppc
../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_end':
/source/qemu/migration/postcopy-ram.c:717: undefined reference to
`__atomic_fetch_add_8'
/source/qemu/migration/postcopy-ram.c:738: undefined reference to
`__atomic_fetch_add_8'
../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_begin':
/source/qemu/migration/postcopy-ram.c:651: undefined reference to
`__atomic_exchange_8'
/source/qemu/migration/postcopy-ram.c:652: undefined reference to
`__atomic_exchange_8'
/source/qemu/migration/postcopy-ram.c:661: undefined reference to
`__atomic_exchange_8'
collect2: error: ld returned 1 exit status
Makefile:193: recipe for target 'qemu-system-ppc' failed
make[1]: *** [qemu-system-ppc] Error 1
Makefile:391: recipe for target 'subdir-ppc-softmmu' failed
make: *** [subdir-ppc-softmmu] Error 2



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PULL 00/27] Migration pull

[Juan Quintela]

Peter Maydell  wrote:
> On 19 January 2018 at 16:43, Alexey Perevalov  wrote:
>> As I remember, I tested build in QEMU's docker build system,
>> but now I checked it on i386 Ubuntu, and yes linker says about unresolved
>> atomic symbols. Next week, I'll have a time to investigate it deeper.
>
> This sounds like exactly the problem I pointed out in a previous
> round of this patchset :-(
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg02103.html
>
> Ignoring comments and sending patches anyway makes me grumpy,
> especially when the result is exactly "fails obscurely on
> some architectures only"...

It compiles for me.  F25 i686 gcc.  I did change it to use intptr_t
instead of uint64_t.  So, I don't know what is going on here.

Then, I have a report that clang -m32 didn't work (Mat).  I haven't been
able yet to reproduce.  I don't have 32bits libraries installed on my
x86_64 systems.  clang compiles for me on both 64 bits (f27) and 32bits
native (f25, it is a long history that it is not f27).

So, I can agree that we have to fix anything that don't work, but I
can't agree that I didn't care about comments, at least I tried to fix
the problems you pointed me to.

What I don't have is an easier way of compiling on arm and ppc32, but I
will search a way to do that for my pull requsets.

Sorry about the inconveniences.

Later, Juan.

Re: [Qemu-devel] [RFC] qid path collision issues in 9pfs

[Emilio G. Cota]

On Fri, Jan 19, 2018 at 19:05:06 -0500, Emilio G. Cota wrote:
> > > > On Fri, 12 Jan 2018 19:32:10 +0800
> > > > Antonios Motakis  wrote:
> > > Since inodes are not completely random, and we usually have a handful of 
> > > device IDs,
> > > we get a much smaller number of entries to track in the hash table.
> > > 
> > > So what this would give:
> > > (1)   Would be faster and take less memory than mapping the full 
> > > inode_nr,devi_id
> > > tuple to unique QID paths
> > > (2)   Guaranteed not to run out of bits when inode numbers stay below 
> > > the lowest
> > > 54 bits and we have less than 1024 devices.
> > > (3)   When we get beyond this this limit, there is a chance we run 
> > > out of bits to
> > > allocate new QID paths, but we can detect this and refuse to serve the 
> > > offending
> > > files instead of allowing a collision.
> > > 
> > > We could tweak the prefix size to match the scenarios that we consider 
> > > more likely,
> > > but I think close to 10-16 bits sounds reasonable enough. What do you 
> > > think?
> 
> Assuming assumption (2) is very likely to be true, I'd suggest
> dropping the intermediate hash table altogether, and simply refuse
> to work with any files that do not meet (2).
> 
> That said, the naive solution of having a large hash table with all entries
> in it might be worth a shot.

hmm but that would still take a lot of memory.

Given assumption (2), a good compromise would be the following,
taking into account that the number of total gids is unlikely to
reach even close to 2**64:
- bit 63: 0/1 determines "fast" or "slow" encoding
- 62-0:
  - fast (trivial) encoding: when assumption (2) is met
- 62-53: device id (it fits because of (2))
- 52-0: inode (it fits because of (2))
  - slow path: assumption (2) isn't met. Then, assign incremental
IDs in the [0,2**63-1] range and track them in a hash table.

Choosing 10 or whatever else bits for the device id is of course TBD,
as Antonios you pointed out.

Something like this will give you great performance and 0 memory
overhead for the majority of cases if (2) indeed holds.

Emilio

Re: [Qemu-devel] [PATCH 00/11] sun4u: APB tidy-up/rename and tracepoint conversions

[Artyom Tarasenko]

On Mon, Jan 15, 2018 at 7:38 PM, Mark Cave-Ayland
 wrote:
> On 14/01/18 13:25, Philippe Mathieu-Daudé wrote:
>
>> On 01/14/2018 08:21 AM, Mark Cave-Ayland wrote:
>>>
>>> On 14/01/18 11:15, no-re...@patchew.org wrote:

 Hi,

 This series seems to have some coding style problems. See output below
 for
 more information:

 Type: series
 Message-id: 20180114104751.21965-1-mark.cave-ayl...@ilande.co.uk
 Subject: [Qemu-devel] [PATCH 00/11] sun4u: APB tidy-up/rename and
 tracepoint conversions
>>>
>>>
>>> (lots cut)
>>>
 === OUTPUT BEGIN ===
 Checking PATCH 1/11: apb: split simba PCI bridge into
 hw/pci-bridge/simba.c...
 Checking PATCH 2/11: simba: rename PBMPCIBridge and QOM types to
 reflect simba naming...
 Checking PATCH 3/11: apb: rename APB functions to use sabre prefix...
 Checking PATCH 4/11: apb: change pbm_pci_host prefix functions to use
 sabre_pci prefix...
 Checking PATCH 5/11: apb: QOMify sabre PCI host bridge...
 Checking PATCH 6/11: apb: rename QOM type from TYPE_APB to TYPE_SABRE...
 Checking PATCH 7/11: sun4u: rename apb variables and constants...
 Checking PATCH 8/11: apb: rename apb.c to sabre.c...
 ERROR: do not use C99 // comments
 #86: FILE: hw/pci-host/sabre.c:41:
 +//#define DEBUG_SABRE

 total: 1 errors, 0 warnings, 188 lines checked

 Your patch has style problems, please review.  If any of these errors
 are false positives report them to the maintainer, see
 CHECKPATCH in MAINTAINERS.

 Checking PATCH 9/11: pci: add trace-events support for hw/pci-host...
 Checking PATCH 10/11: sabre: convert from SABRE_DPRINTF macro to
 trace-events...
 Checking PATCH 11/11: sparc64: convert hw/sparc64/sparc64.c from
 DPRINTF macros to trace events...
 === OUTPUT END ===

 Test command exited with code: 1
>>>
>>>
>>> This is fine - it's just a side-effect of renaming DEBUG_APB to
>>> DEBUG_SABRE as part of the APB to sabre rename, and in fact this code is
>>> completely removed in patch 10 with the conversion to tracepoints.
>>
>>
>> This can be avoided moving patch #8 after #10, although not worthy IMHO.
>
>
> Thanks once again for the review. It was quite a tricky patchset to order in
> the end because of the rename, and there wasn't really one ordering which I
> found completely satisfactory.
>
> Given that you're generally happy with the patches, I'm inclined to keep the
> patchset in its current form, pending any further comments from Artyom.
>

The whole series does look good to me. The new naming conventions are
really a lot cleaner than what we used to have. Well done!

Acked-by: Artyom Tarasenko 


-- 
Regards,
Artyom Tarasenko

SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu

Re: [Qemu-devel] [PATCH] sun4u: implement power device

[Artyom Tarasenko]

Hi Mark,


On Mon, Jan 15, 2018 at 9:58 PM, Mark Cave-Ayland
 wrote:
> This inbuilt device contains a single 4-byte register, of which bit 24 is used
> to power down the machine on a real Ultra 5.
>
> The power device exists at offset 0x724000 on a real machine, but due to the
> current configuration of the BARs in QEMU it must be located lower in PCI IO
> space.
>
> For the moment we place the power device at offset 0x7240 as a reminder of its
> original location and raise the base PCI IO address from 0x4000 to 0x8000.

I think it's ok to have it there for now. The guests should get the
info from the device tree.

> Signed-off-by: Mark Cave-Ayland 

Reviewed-by: Artyom Tarasenko 

> ---
>  hw/sparc64/sun4u.c | 64 
> +-
>  1 file changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index ec45ec2801..26ab6e9a9c 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -205,6 +205,59 @@ typedef struct ResetData {
>  uint64_t prom_addr;
>  } ResetData;
>
> +#define TYPE_SUN4U_POWER "power"
> +#define SUN4U_POWER(obj) OBJECT_CHECK(PowerDevice, (obj), TYPE_SUN4U_POWER)
> +
> +typedef struct PowerDevice {
> +SysBusDevice parent_obj;
> +
> +MemoryRegion power_mmio;
> +} PowerDevice;
> +
> +/* Power */
> +static void power_mem_write(void *opaque, hwaddr addr,
> +uint64_t val, unsigned size)
> +{
> +/* According to a real Ultra 5, bit 24 controls the power */
> +if (val & 0x100) {
> +qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +}
> +}
> +
> +static const MemoryRegionOps power_mem_ops = {
> +.write = power_mem_write,
> +.endianness = DEVICE_NATIVE_ENDIAN,
> +.valid = {
> +.min_access_size = 4,
> +.max_access_size = 4,
> +},
> +};
> +
> +static void power_realize(DeviceState *dev, Error **errp)
> +{
> +PowerDevice *d = SUN4U_POWER(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +memory_region_init_io(>power_mmio, OBJECT(dev), _mem_ops, d,
> +  "power", sizeof(uint32_t));
> +
> +sysbus_init_mmio(sbd, >power_mmio);
> +}
> +
> +static void power_class_init(ObjectClass *klass, void *data)
> +{
> +DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +dc->realize = power_realize;
> +}
> +
> +static const TypeInfo power_info = {
> +.name  = TYPE_SUN4U_POWER,
> +.parent= TYPE_SYS_BUS_DEVICE,
> +.instance_size = sizeof(PowerDevice),
> +.class_init= power_class_init,
> +};
> +
>  static void ebus_isa_irq_handler(void *opaque, int n, int level)
>  {
>  EbusState *s = EBUS(opaque);
> @@ -221,6 +274,7 @@ static void ebus_isa_irq_handler(void *opaque, int n, int 
> level)
>  static void ebus_realize(PCIDevice *pci_dev, Error **errp)
>  {
>  EbusState *s = EBUS(pci_dev);
> +SysBusDevice *sbd;
>  DeviceState *dev;
>  qemu_irq *isa_irq;
>  DriveInfo *fd[MAX_FD];
> @@ -270,6 +324,13 @@ static void ebus_realize(PCIDevice *pci_dev, Error 
> **errp)
>  qdev_prop_set_uint32(dev, "dma", -1);
>  qdev_init_nofail(dev);
>
> +/* Power */
> +dev = qdev_create(NULL, TYPE_SUN4U_POWER);
> +qdev_init_nofail(dev);
> +sbd = SYS_BUS_DEVICE(dev);
> +memory_region_add_subregion(pci_address_space_io(pci_dev), 0x7240,
> +sysbus_mmio_get_region(sbd, 0));
> +
>  /* PCI */
>  pci_dev->config[0x04] = 0x06; // command = bus master, pci mem
>  pci_dev->config[0x05] = 0x00;
> @@ -282,7 +343,7 @@ static void ebus_realize(PCIDevice *pci_dev, Error **errp)
>   0, 0x100);
>  pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, >bar0);
>  memory_region_init_alias(>bar1, OBJECT(s), "bar1", get_system_io(),
> - 0, 0x4000);
> + 0, 0x8000);
>  pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >bar1);
>  }
>
> @@ -693,6 +754,7 @@ static const TypeInfo sun4v_type = {
>
>  static void sun4u_register_types(void)
>  {
> +type_register_static(_info);
>  type_register_static(_info);
>  type_register_static(_info);
>  type_register_static(_info);
> --
> 2.11.0
>



-- 
Regards,
Artyom Tarasenko

SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu

[Qemu-devel] [PULL 0/1] Dump patches

[Marc-André Lureau]

The following changes since commit b384cd95eb9c6f73ad84ed1bb0717a26e29cc78f:

  Merge remote-tracking branch 
'remotes/ehabkost/tags/machine-next-pull-request' into staging (2018-01-19 
16:35:25 +)

are available in the Git repository at:

  https://github.com/elmarco/qemu.git tags/dump-pull-request

for you to fetch changes up to 6f49ec4034e55dfb675a56a62c9579384f7fb8cc:

  dump-guest-memory.py: fix python 2 support (2018-01-20 20:59:00 +0100)





Marc-André Lureau (1):
  dump-guest-memory.py: fix python 2 support

 scripts/dump-guest-memory.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.16.0.rc1.1.gef27df75a1

[Qemu-devel] [PULL 1/1] dump-guest-memory.py: fix python 2 support

[Marc-André Lureau]

Python GDB support may use Python 2 or 3.

Inferior.read_memory() may return a 'buffer' with Python 2 or a
'memoryview' with Python 3 (see also
https://sourceware.org/gdb/onlinedocs/gdb/Inferiors-In-Python.html)

The elf.add_vmcoreinfo_note() method expects a "bytes" object. Wrap
the returned memory with bytes(), which works with both 'memoryview'
and 'buffer'.

Fixes a regression introduced with commit
d23bfa91b7789534d16ede6cb7d925bfac3f3c4c ("add vmcoreinfo").

Suggested-by: Peter Maydell 
Signed-off-by: Marc-André Lureau 
Acked-by: Laszlo Ersek 
Reviewed-by: Eric Blake 
---
 scripts/dump-guest-memory.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
index 09bec92b50..03fbf69f8a 100644
--- a/scripts/dump-guest-memory.py
+++ b/scripts/dump-guest-memory.py
@@ -564,7 +564,7 @@ shape and this command should mostly work."""
 
 vmcoreinfo = self.phys_memory_read(addr, size)
 if vmcoreinfo:
-self.elf.add_vmcoreinfo_note(vmcoreinfo.tobytes())
+self.elf.add_vmcoreinfo_note(bytes(vmcoreinfo))
 
 def invoke(self, args, from_tty):
 """Handles command invocation from gdb."""
-- 
2.16.0.rc1.1.gef27df75a1

[Qemu-devel] [PATCH v2 2/6] qapi: Replace qobject_to_X(o) by qobject_to(o, X)

[Max Reitz]

This patch was generated using the following Coccinelle script:

@@
expression Obj;
@@
(
- qobject_to_qnum(Obj)
+ qobject_to(Obj, QNum)
|
- qobject_to_qstring(Obj)
+ qobject_to(Obj, QString)
|
- qobject_to_qdict(Obj)
+ qobject_to(Obj, QDict)
|
- qobject_to_qlist(Obj)
+ qobject_to(Obj, QList)
|
- qobject_to_qbool(Obj)
+ qobject_to(Obj, QBool)
)

and a bit of manual fix-up for overly long lines and three places in
tests/check-qjson.c that Coccinelle did not find.

Signed-off-by: Max Reitz 
---
 block.c |  2 +-
 block/qapi.c| 12 -
 block/rbd.c |  8 +++---
 blockdev.c  |  7 ++---
 hw/i386/acpi-build.c| 14 +-
 monitor.c   |  8 +++---
 qapi/qmp-dispatch.c |  2 +-
 qapi/qobject-input-visitor.c| 20 +++---
 qapi/qobject-output-visitor.c   |  4 +--
 qga/main.c  |  2 +-
 qmp.c   |  2 +-
 qobject/json-parser.c   |  2 +-
 qobject/qbool.c |  4 +--
 qobject/qdict.c | 38 +-
 qobject/qjson.c | 10 +++
 qobject/qlist.c |  6 ++---
 qobject/qlit.c  | 10 +++
 qobject/qnum.c  |  6 ++---
 qobject/qstring.c   |  6 ++---
 qom/object.c|  8 +++---
 target/i386/cpu.c   |  2 +-
 target/s390x/cpu_models.c   |  2 +-
 tests/check-qdict.c | 20 +++---
 tests/check-qjson.c | 41 ++--
 tests/check-qlist.c |  4 +--
 tests/check-qlit.c  |  2 +-
 tests/check-qnum.c  |  4 +--
 tests/check-qobject.c   |  2 +-
 tests/check-qstring.c   |  2 +-
 tests/device-introspect-test.c  | 14 +-
 tests/libqtest.c|  6 ++---
 tests/numa-test.c   |  8 +++---
 tests/qom-test.c|  4 +--
 tests/test-char.c   |  2 +-
 tests/test-keyval.c |  8 +++---
 tests/test-qga.c| 19 ++---
 tests/test-qmp-commands.c   | 12 -
 tests/test-qmp-event.c  | 16 +--
 tests/test-qobject-input-visitor.c  | 10 +++
 tests/test-qobject-output-visitor.c | 54 ++---
 tests/test-x86-cpuid-compat.c   | 17 ++--
 util/keyval.c   |  4 +--
 util/qemu-config.c  |  2 +-
 util/qemu-option.c  |  6 ++---
 44 files changed, 218 insertions(+), 214 deletions(-)

diff --git a/block.c b/block.c
index a8da4f2b25..909cf076b7 100644
--- a/block.c
+++ b/block.c
@@ -1454,7 +1454,7 @@ static QDict *parse_json_filename(const char *filename, 
Error **errp)
 return NULL;
 }
 
-options = qobject_to_qdict(options_obj);
+options = qobject_to(options_obj, QDict);
 if (!options) {
 qobject_decref(options_obj);
 error_setg(errp, "Invalid JSON object given");
diff --git a/block/qapi.c b/block/qapi.c
index fc10f0a565..3d0e11ccb3 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -642,29 +642,29 @@ static void dump_qobject(fprintf_function func_fprintf, 
void *f,
 {
 switch (qobject_type(obj)) {
 case QTYPE_QNUM: {
-QNum *value = qobject_to_qnum(obj);
+QNum *value = qobject_to(obj, QNum);
 char *tmp = qnum_to_string(value);
 func_fprintf(f, "%s", tmp);
 g_free(tmp);
 break;
 }
 case QTYPE_QSTRING: {
-QString *value = qobject_to_qstring(obj);
+QString *value = qobject_to(obj, QString);
 func_fprintf(f, "%s", qstring_get_str(value));
 break;
 }
 case QTYPE_QDICT: {
-QDict *value = qobject_to_qdict(obj);
+QDict *value = qobject_to(obj, QDict);
 dump_qdict(func_fprintf, f, comp_indent, value);
 break;
 }
 case QTYPE_QLIST: {
-QList *value = qobject_to_qlist(obj);
+QList *value = qobject_to(obj, QList);
 dump_qlist(func_fprintf, f, comp_indent, value);
 break;
 }
 case QTYPE_QBOOL: {
-QBool *value = qobject_to_qbool(obj);
+QBool *value = qobject_to(obj, QBool);
 func_fprintf(f, "%s", qbool_get_bool(value) ? "true" : "false");
 break;
 }
@@ -725,7 +725,7 @@ void bdrv_image_info_specific_dump(fprintf_function 
func_fprintf, void *f,
 
 visit_type_ImageInfoSpecific(v, NULL, _spec, _abort);
 visit_complete(v, );
-data = qdict_get(qobject_to_qdict(obj), "data");
+data = qdict_get(qobject_to(obj, QDict), "data");
 dump_qobject(func_fprintf, f, 1, data);
 

[Qemu-devel] [PATCH v2 5/6] block: Handle null backing link

[Max Reitz]

Instead of converting all "backing": null instances into "backing": "",
handle a null value directly in bdrv_open_inherit().

This enables explicitly null backing links for json:{} filenames.

Signed-off-by: Max Reitz 
---
 block.c|  4 +++-
 blockdev.c | 14 --
 tests/qemu-iotests/089 | 20 
 tests/qemu-iotests/089.out |  8 
 4 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index 909cf076b7..32b7887651 100644
--- a/block.c
+++ b/block.c
@@ -2597,7 +2597,9 @@ static BlockDriverState *bdrv_open_inherit(const char 
*filename,
 
 /* See cautionary note on accessing @options above */
 backing = qdict_get_try_str(options, "backing");
-if (backing && *backing == '\0') {
+if (qobject_to(qdict_get(options, "backing"), QNull) != NULL ||
+(backing && *backing == '\0'))
+{
 flags |= BDRV_O_NO_BACKING;
 qdict_del(options, "backing");
 }
diff --git a/blockdev.c b/blockdev.c
index 2dce231c98..5aac920eae 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3969,7 +3969,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
**errp)
 QObject *obj;
 Visitor *v = qobject_output_visitor_new();
 QDict *qdict;
-const QDictEntry *ent;
 Error *local_err = NULL;
 
 visit_type_BlockdevOptions(v, NULL, , _err);
@@ -3983,19 +3982,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
**errp)
 
 qdict_flatten(qdict);
 
-/*
- * Rewrite "backing": null to "backing": ""
- * TODO Rewrite "" to null instead, and perhaps not even here
- */
-for (ent = qdict_first(qdict); ent; ent = qdict_next(qdict, ent)) {
-char *dot = strrchr(ent->key, '.');
-
-if (!strcmp(dot ? dot + 1 : ent->key, "backing")
-&& qobject_type(ent->value) == QTYPE_QNULL) {
-qdict_put(qdict, ent->key, qstring_new());
-}
-}
-
 if (!qdict_get_try_str(qdict, "node-name")) {
 error_setg(errp, "'node-name' must be specified for the root node");
 goto fail;
diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
index 0b059aba90..aa1ba4a98e 100755
--- a/tests/qemu-iotests/089
+++ b/tests/qemu-iotests/089
@@ -82,6 +82,26 @@ $QEMU_IO_PROG --cache $CACHEMODE \
 $QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
 
 
+echo
+echo "=== Testing correct handling of 'backing':null ==="
+echo
+
+_make_test_img -b "$TEST_IMG.base" $IMG_SIZE
+
+# This should read 42
+$QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
+
+# This should read 0
+$QEMU_IO -c 'read -P 0 0 512' "json:{\
+'driver': '$IMGFMT',
+'file': {
+'driver': 'file',
+'filename': '$TEST_IMG'
+},
+'backing': null
+}" | _filter_qemu_io
+
+
 # Taken from test 071
 echo
 echo "=== Testing blkdebug ==="
diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
index 0bf5a13ec1..89e3e4340a 100644
--- a/tests/qemu-iotests/089.out
+++ b/tests/qemu-iotests/089.out
@@ -19,6 +19,14 @@ Pattern verification failed at offset 0, 512 bytes
 read 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
+=== Testing correct handling of 'backing':null ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
backing_file=TEST_DIR/t.IMGFMT.base
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
 === Testing blkdebug ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-- 
2.14.3

[Qemu-devel] [PATCH v2 4/6] qapi: Make more of qobject_to()

[Max Reitz]

This patch reworks some places which use either qobject_type() checks
plus qobject_to(), where the latter alone is sufficient, or NULL checks
plus qobject_type() checks where we can simply do a qobject_to() != NULL
check.

Signed-off-by: Max Reitz 
---
 qapi/qobject-input-visitor.c |  4 ++--
 qobject/json-parser.c| 13 +++--
 qobject/qdict.c  | 20 +++-
 3 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 45e6160b21..fa6b92d9f6 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -334,7 +334,7 @@ static GenericList *qobject_input_next_list(Visitor *v, 
GenericList *tail,
 QObjectInputVisitor *qiv = to_qiv(v);
 StackObject *tos = QSLIST_FIRST(>stack);
 
-assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST);
+assert(tos && qobject_to(tos->obj, QList));
 
 if (!tos->entry) {
 return NULL;
@@ -348,7 +348,7 @@ static void qobject_input_check_list(Visitor *v, Error 
**errp)
 QObjectInputVisitor *qiv = to_qiv(v);
 StackObject *tos = QSLIST_FIRST(>stack);
 
-assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST);
+assert(tos && qobject_to(tos->obj, QList));
 
 if (tos->entry) {
 error_setg(errp, "Only %u list elements expected in %s",
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 885039e94b..c4d1f0d8de 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -271,7 +271,8 @@ static void parser_context_free(JSONParserContext *ctxt)
  */
 static int parse_pair(JSONParserContext *ctxt, QDict *dict, va_list *ap)
 {
-QObject *key = NULL, *value;
+QObject *value;
+QString *key = NULL;
 JSONToken *peek, *token;
 
 peek = parser_context_peek_token(ctxt);
@@ -280,8 +281,8 @@ static int parse_pair(JSONParserContext *ctxt, QDict *dict, 
va_list *ap)
 goto out;
 }
 
-key = parse_value(ctxt, ap);
-if (!key || qobject_type(key) != QTYPE_QSTRING) {
+key = qobject_to(parse_value(ctxt, ap), QString);
+if (!key) {
 parse_error(ctxt, peek, "key is not a string in object");
 goto out;
 }
@@ -303,14 +304,14 @@ static int parse_pair(JSONParserContext *ctxt, QDict 
*dict, va_list *ap)
 goto out;
 }
 
-qdict_put_obj(dict, qstring_get_str(qobject_to(key, QString)), value);
+qdict_put_obj(dict, qstring_get_str(key), value);
 
-qobject_decref(key);
+QDECREF(key);
 
 return 0;
 
 out:
-qobject_decref(key);
+QDECREF(key);
 
 return -1;
 }
diff --git a/qobject/qdict.c b/qobject/qdict.c
index ca84455883..3e97c2d3bc 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -861,18 +861,20 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
 
 child = qdict_get(two_level, prefix);
 if (suffix) {
-if (child) {
-if (qobject_type(child) != QTYPE_QDICT) {
+QDict *child_dict = qobject_to(child, QDict);
+if (!child_dict) {
+if (child) {
 error_setg(errp, "Key %s prefix is already set as a 
scalar",
prefix);
 goto error;
 }
-} else {
-child = QOBJECT(qdict_new());
-qdict_put_obj(two_level, prefix, child);
+
+child_dict = qdict_new();
+qdict_put_obj(two_level, prefix, QOBJECT(child_dict));
 }
+
 qobject_incref(ent->value);
-qdict_put_obj(qobject_to(child, QDict), suffix, ent->value);
+qdict_put_obj(child_dict, suffix, ent->value);
 } else {
 if (child) {
 error_setg(errp, "Key %s prefix is already set as a dict",
@@ -892,9 +894,9 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
 multi_level = qdict_new();
 for (ent = qdict_first(two_level); ent != NULL;
  ent = qdict_next(two_level, ent)) {
-
-if (qobject_type(ent->value) == QTYPE_QDICT) {
-child = qdict_crumple(qobject_to(ent->value, QDict), errp);
+QDict *dict = qobject_to(ent->value, QDict);
+if (dict) {
+child = qdict_crumple(dict, errp);
 if (!child) {
 goto error;
 }
-- 
2.14.3

[Qemu-devel] [PATCH v2 3/6] qapi: Remove qobject_to_X() functions

[Max Reitz]

They are no longer needed now.

Signed-off-by: Max Reitz 
---
 include/qapi/qmp/qbool.h   |  1 -
 include/qapi/qmp/qdict.h   |  1 -
 include/qapi/qmp/qlist.h   |  1 -
 include/qapi/qmp/qnum.h|  1 -
 include/qapi/qmp/qstring.h |  1 -
 qobject/qbool.c| 11 ---
 qobject/qdict.c| 11 ---
 qobject/qlist.c| 11 ---
 qobject/qnum.c | 11 ---
 qobject/qstring.c  | 11 ---
 10 files changed, 60 deletions(-)

diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
index f77ea86c4e..63e2918041 100644
--- a/include/qapi/qmp/qbool.h
+++ b/include/qapi/qmp/qbool.h
@@ -23,7 +23,6 @@ typedef struct QBool {
 
 QBool *qbool_from_bool(bool value);
 bool qbool_get_bool(const QBool *qb);
-QBool *qobject_to_qbool(const QObject *obj);
 bool qbool_is_equal(const QObject *x, const QObject *y);
 void qbool_destroy_obj(QObject *obj);
 
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index fc218e7be6..7dbe07dd85 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -42,7 +42,6 @@ void qdict_put_obj(QDict *qdict, const char *key, QObject 
*value);
 void qdict_del(QDict *qdict, const char *key);
 int qdict_haskey(const QDict *qdict, const char *key);
 QObject *qdict_get(const QDict *qdict, const char *key);
-QDict *qobject_to_qdict(const QObject *obj);
 bool qdict_is_equal(const QObject *x, const QObject *y);
 void qdict_iter(const QDict *qdict,
 void (*iter)(const char *key, QObject *obj, void *opaque),
diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index ec3fcc1a4c..979da8bc14 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -60,7 +60,6 @@ QObject *qlist_pop(QList *qlist);
 QObject *qlist_peek(QList *qlist);
 int qlist_empty(const QList *qlist);
 size_t qlist_size(const QList *qlist);
-QList *qobject_to_qlist(const QObject *obj);
 bool qlist_is_equal(const QObject *x, const QObject *y);
 void qlist_destroy_obj(QObject *obj);
 
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index c3d86794bb..56ca3c4658 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -68,7 +68,6 @@ double qnum_get_double(QNum *qn);
 
 char *qnum_to_string(QNum *qn);
 
-QNum *qobject_to_qnum(const QObject *obj);
 bool qnum_is_equal(const QObject *x, const QObject *y);
 void qnum_destroy_obj(QObject *obj);
 
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 65c05a9be5..9293af9ac6 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -30,7 +30,6 @@ const char *qstring_get_str(const QString *qstring);
 void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
-QString *qobject_to_qstring(const QObject *obj);
 bool qstring_is_equal(const QObject *x, const QObject *y);
 void qstring_destroy_obj(QObject *obj);
 
diff --git a/qobject/qbool.c b/qobject/qbool.c
index 683d2b9b3f..91fc2ddcbb 100644
--- a/qobject/qbool.c
+++ b/qobject/qbool.c
@@ -40,17 +40,6 @@ bool qbool_get_bool(const QBool *qb)
 return qb->value;
 }
 
-/**
- * qobject_to_qbool(): Convert a QObject into a QBool
- */
-QBool *qobject_to_qbool(const QObject *obj)
-{
-if (!obj || qobject_type(obj) != QTYPE_QBOOL) {
-return NULL;
-}
-return container_of(obj, QBool, base);
-}
-
 /**
  * qbool_is_equal(): Test whether the two QBools are equal
  */
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 472d989a67..ca84455883 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -36,17 +36,6 @@ QDict *qdict_new(void)
 return qdict;
 }
 
-/**
- * qobject_to_qdict(): Convert a QObject into a QDict
- */
-QDict *qobject_to_qdict(const QObject *obj)
-{
-if (!obj || qobject_type(obj) != QTYPE_QDICT) {
-return NULL;
-}
-return container_of(obj, QDict, base);
-}
-
 /**
  * tdb_hash(): based on the hash agorithm from gdbm, via tdb
  * (from module-init-tools)
diff --git a/qobject/qlist.c b/qobject/qlist.c
index a8bd42e3de..b13a392fd5 100644
--- a/qobject/qlist.c
+++ b/qobject/qlist.c
@@ -128,17 +128,6 @@ size_t qlist_size(const QList *qlist)
 return count;
 }
 
-/**
- * qobject_to_qlist(): Convert a QObject into a QList
- */
-QList *qobject_to_qlist(const QObject *obj)
-{
-if (!obj || qobject_type(obj) != QTYPE_QLIST) {
-return NULL;
-}
-return container_of(obj, QList, base);
-}
-
 /**
  * qlist_is_equal(): Test whether the two QLists are equal
  *
diff --git a/qobject/qnum.c b/qobject/qnum.c
index 18dcf27582..98893e06fb 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -201,17 +201,6 @@ char *qnum_to_string(QNum *qn)
 return NULL;
 }
 
-/**
- * qobject_to_qnum(): Convert a QObject into a QNum
- */
-QNum *qobject_to_qnum(const QObject *obj)
-{
-if (!obj || qobject_type(obj) != QTYPE_QNUM) {
-return NULL;
-}
-return container_of(obj, 

[Qemu-devel] [PATCH v2 1/6] qapi: Add qobject_to()

[Max Reitz]

This is a dynamic casting macro that, given a QObject type, returns an
object as that type or NULL if the object is of a different type (or
NULL itself).

The macro uses lower-case letters because:
1. There does not seem to be a hard rule on whether qemu macros have to
   be upper-cased,
2. The current situation in qapi/qmp is inconsistent (compare e.g.
   QINCREF() vs. qdict_put()),
3. qobject_to() will evaluate its @obj parameter only once, thus it is
   generally not important to the caller whether it is a macro or not,
4. I prefer it aesthetically.

Signed-off-by: Max Reitz 
---
You're more than welcome to convince me to call it QOBJECT_TO()!
---
 include/qapi/qmp/qobject.h | 32 
 1 file changed, 32 insertions(+)

diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 38ac68845c..1211989ca0 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -50,6 +50,24 @@ struct QObject {
 #define QDECREF(obj)  \
 qobject_decref(obj ? QOBJECT(obj) : NULL)
 
+/* Required for qobject_to() */
+#define QTYPE_CAST_TO_QNull QTYPE_QNULL
+#define QTYPE_CAST_TO_QNum  QTYPE_QNUM
+#define QTYPE_CAST_TO_QString   QTYPE_QSTRING
+#define QTYPE_CAST_TO_QDict QTYPE_QDICT
+#define QTYPE_CAST_TO_QList QTYPE_QLIST
+#define QTYPE_CAST_TO_QBool QTYPE_QBOOL
+
+#ifdef CONFIG_STATIC_ASSERT
+_Static_assert(QTYPE__MAX == 7,
+   "The QTYPE_CAST_TO_* list needs to be extended");
+#endif
+
+#define qobject_to(obj, type) \
+container_of(qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)) ?: \
+ QOBJECT((type *)NULL), \
+ type, base)
+
 /* Initialize an object to default values */
 static inline void qobject_init(QObject *obj, QType type)
 {
@@ -102,4 +120,18 @@ static inline QType qobject_type(const QObject *obj)
 return obj->type;
 }
 
+/**
+ * qobject_check_type(): Helper function for the qobject_to() macro.
+ * Return @obj, but only if @obj is not NULL and @type is equal to
+ * @obj's type.  Return NULL otherwise.
+ */
+static inline QObject *qobject_check_type(const QObject *obj, QType type)
+{
+if (obj && qobject_type(obj) == type) {
+return (QObject *)obj;
+} else {
+return NULL;
+}
+}
+
 #endif /* QOBJECT_H */
-- 
2.14.3

[Qemu-devel] [PATCH v2 6/6] block: Deprecate "backing": ""

[Max Reitz]

We have a clear replacement, so let's deprecate it.

Signed-off-by: Max Reitz 
---
 qapi/block-core.json | 4 ++--
 block.c  | 4 
 qemu-doc.texi| 7 +++
 qemu-options.hx  | 4 ++--
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 89ed2bc6a4..78de8eff63 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1090,7 +1090,7 @@
 # @overlay: reference to the existing block device that will become
 #   the overlay of @node, as part of creating the snapshot.
 #   It must not have a current backing file (this can be
-#   achieved by passing "backing": "" to blockdev-add).
+#   achieved by passing "backing": null to blockdev-add).
 #
 # Since: 2.5
 ##
@@ -1238,7 +1238,7 @@
 # "node-name": "node1534",
 # "file": { "driver": "file",
 #   "filename": "hd1.qcow2" },
-# "backing": "" } }
+# "backing": null } }
 #
 # <- { "return": {} }
 #
diff --git a/block.c b/block.c
index 32b7887651..5ac0aba638 100644
--- a/block.c
+++ b/block.c
@@ -2600,6 +2600,10 @@ static BlockDriverState *bdrv_open_inherit(const char 
*filename,
 if (qobject_to(qdict_get(options, "backing"), QNull) != NULL ||
 (backing && *backing == '\0'))
 {
+if (backing) {
+warn_report("Use of \"backing\": \"\" is deprecated; "
+"use \"backing\": null instead");
+}
 flags |= BDRV_O_NO_BACKING;
 qdict_del(options, "backing");
 }
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 3e9eb819a6..d321908da8 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2773,6 +2773,13 @@ or ``ivshmem-doorbell`` device types.
 The ``xlnx-ep108'' machine has been replaced by the ``xlnx-zcu102'' machine.
 The ``xlnx-zcu102'' machine has the same features and capabilites in QEMU.
 
+@section Block device options
+
+@subsection "backing": "" (since 2.12.0)
+
+In order to prevent QEMU from automatically opening an image's backing
+chain, use ``"backing": null'' instead.
+
 @node License
 @appendix License
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 5ff741a4af..76e9a5b06b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -743,8 +743,8 @@ Reference to or definition of the data source block driver 
node
 
 @item backing
 Reference to or definition of the backing file block device (default is taken
-from the image file). It is allowed to pass an empty string here in order to
-disable the default backing file.
+from the image file). It is allowed to pass @code{null} here in order to 
disable
+the default backing file.
 
 @item lazy-refcounts
 Whether to enable the lazy refcounts feature (on/off; default is taken from the
-- 
2.14.3

[Qemu-devel] [PATCH v2 0/6] block: Handle null backing link

[Max Reitz]

Currently, we try to rewrite every occurrence of "backing": null into
"backing": "" in qmp_blockdev_add().  However, that breaks using the
same "backing": null construction in json:{} file names (which do not go
through qmp_blockdev_add()).  Currently, these then just behave as if
the option has not been specified.

Since there is actually only one place where we evaluate the @backing
option to find out whether not to use a backing file, we can instead
just check for null there.  It doesn't matter that this changes the
runtime state of the option from "" to null, because nobody really does
anything with that runtime state anyway (except put it into qemu again,
but qemu doesn't care whether it's "" or null).

And in the future, it's much better if we get it to be null in that
runtime state sooner than later -- see patch 6.


v2: Add qobject_to() macro instead of qdict_is_null() (and then we can
do qobject_to(qdict_get(...), QNull) != NULL instead) [Markus]
- I'm not sure what I came up with is what Markus intended me to,
  and if it is not, I'm inclined to go back to qdict_is_null() (or
  qdict_entry_is_null(), as Berto has proposed);
  or, alternatively, add a qobject_to_qnull() function and drop
  patches 2 through 4.


git-backport-diff against v1:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/6:[down] 'qapi: Add qobject_to()'
002/6:[down] 'qapi: Replace qobject_to_X(o) by qobject_to(o, X)'
003/6:[down] 'qapi: Remove qobject_to_X() functions'
004/6:[down] 'qapi: Make more of qobject_to()'
005/6:[0004] [FC] 'block: Handle null backing link'
006/6:[] [-C] 'block: Deprecate "backing": ""'


Max Reitz (6):
  qapi: Add qobject_to()
  qapi: Replace qobject_to_X(o) by qobject_to(o, X)
  qapi: Remove qobject_to_X() functions
  qapi: Make more of qobject_to()
  block: Handle null backing link
  block: Deprecate "backing": ""

 qapi/block-core.json|  4 +--
 include/qapi/qmp/qbool.h|  1 -
 include/qapi/qmp/qdict.h|  1 -
 include/qapi/qmp/qlist.h|  1 -
 include/qapi/qmp/qnum.h |  1 -
 include/qapi/qmp/qobject.h  | 32 ++
 include/qapi/qmp/qstring.h  |  1 -
 block.c | 10 --
 block/qapi.c| 12 +++
 block/rbd.c |  8 ++---
 blockdev.c  | 21 +++-
 hw/i386/acpi-build.c| 14 
 monitor.c   |  8 ++---
 qapi/qmp-dispatch.c |  2 +-
 qapi/qobject-input-visitor.c| 24 +++---
 qapi/qobject-output-visitor.c   |  4 +--
 qga/main.c  |  2 +-
 qmp.c   |  2 +-
 qobject/json-parser.c   | 13 
 qobject/qbool.c | 15 ++---
 qobject/qdict.c | 65 -
 qobject/qjson.c | 10 +++---
 qobject/qlist.c | 17 ++
 qobject/qlit.c  | 10 +++---
 qobject/qnum.c  | 17 ++
 qobject/qstring.c   | 17 ++
 qom/object.c|  8 ++---
 target/i386/cpu.c   |  2 +-
 target/s390x/cpu_models.c   |  2 +-
 tests/check-qdict.c | 20 ++--
 tests/check-qjson.c | 41 +++
 tests/check-qlist.c |  4 +--
 tests/check-qlit.c  |  2 +-
 tests/check-qnum.c  |  4 +--
 tests/check-qobject.c   |  2 +-
 tests/check-qstring.c   |  2 +-
 tests/device-introspect-test.c  | 14 
 tests/libqtest.c|  6 ++--
 tests/numa-test.c   |  8 ++---
 tests/qom-test.c|  4 +--
 tests/test-char.c   |  2 +-
 tests/test-keyval.c |  8 ++---
 tests/test-qga.c| 19 ++-
 tests/test-qmp-commands.c   | 12 +++
 tests/test-qmp-event.c  | 16 -
 tests/test-qobject-input-visitor.c  | 10 +++---
 tests/test-qobject-output-visitor.c | 54 +++---
 tests/test-x86-cpuid-compat.c   | 17 +-
 util/keyval.c   |  4 +--
 util/qemu-config.c  |  2 +-
 util/qemu-option.c  |  6 ++--
 qemu-doc.texi   |  7 
 qemu-options.hx |  4 +--
 tests/qemu-iotests/089  | 20 
 tests/qemu-iotests/089.out  |  8 +
 55 files changed, 313 insertions(+), 307 deletions(-)

-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Philippe Mathieu-Daudé]

On 01/19/2018 11:11 AM, Marc-André Lureau wrote:
> tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB)
> Interface as defined in TCG PC Client Platform TPM Profile (PTP)
> Specification Family “2.0” Level 00 Revision 01.03 v22.
> 
> The PTP allows device implementation to switch between TIS and CRB
> model at run time, but given that CRB is a simpler device to
> implement, I chose to implement it as a different device.
> 
> The device doesn't implement other locality than 0 for now (my laptop
> TPM doesn't either, so I assume this isn't so bad)
> 
> The command/reply memory region is statically allocated after the CRB
> registers address TPM_CRB_ADDR_BASE + sizeof(struct crb_regs) (I
> wonder if the BIOS could or should allocate it instead, or what size
> to use, again this seems to fit well expectations)
> 
> The PTP doesn't specify a particular bus to put the device. So I added
> it on the system bus directly, so it could hopefully be used easily on
> a different platform than x86. Currently, it fails to init on piix,
> because error_on_sysbus_device() check. The check may be changed in a
> near future, see discussion on the qemu-devel ML.
> 
> Tested with some success with Linux upstream and Windows 10, seabios &
> modified ovmf. The device is recognized and correctly transmit
> command/response with passthrough & emu. However, we are missing PPI
> ACPI part atm.
> 
> Signed-off-by: Marc-André Lureau 
> Signed-off-by: Stefan Berger 
> ---
>  qapi/tpm.json  |   5 +-
>  include/hw/acpi/tpm.h  |  72 
>  include/sysemu/tpm.h   |   3 +
>  hw/i386/acpi-build.c   |  34 +++-
>  hw/tpm/tpm_crb.c   | 327 
> +
>  default-configs/i386-softmmu.mak   |   1 +
>  default-configs/x86_64-softmmu.mak |   1 +
>  hw/tpm/Makefile.objs   |   1 +
>  8 files changed, 434 insertions(+), 10 deletions(-)
>  create mode 100644 hw/tpm/tpm_crb.c
> 
> diff --git a/qapi/tpm.json b/qapi/tpm.json
> index 7093f268fb..d50deef5e9 100644
> --- a/qapi/tpm.json
> +++ b/qapi/tpm.json
> @@ -11,10 +11,11 @@
>  # An enumeration of TPM models
>  #
>  # @tpm-tis: TPM TIS model
> +# @tpm-crb: TPM CRB model (since 2.12)
>  #
>  # Since: 1.5
>  ##
> -{ 'enum': 'TpmModel', 'data': [ 'tpm-tis' ] }
> +{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb' ] }
>  
>  ##
>  # @query-tpm-models:
> @@ -28,7 +29,7 @@
>  # Example:
>  #
>  # -> { "execute": "query-tpm-models" }
> -# <- { "return": [ "tpm-tis" ] }
> +# <- { "return": [ "tpm-tis", "tpm-crb" ] }
>  #
>  ##
>  { 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> index 6d516c6a7f..b0048515fa 100644
> --- a/include/hw/acpi/tpm.h
> +++ b/include/hw/acpi/tpm.h
> @@ -16,11 +16,82 @@
>  #ifndef HW_ACPI_TPM_H
>  #define HW_ACPI_TPM_H
>  
> +#include "qemu/osdep.h"
> +
>  #define TPM_TIS_ADDR_BASE   0xFED4
>  #define TPM_TIS_ADDR_SIZE   0x5000
>  
>  #define TPM_TIS_IRQ 5
>  
> +struct crb_regs {
> +union {
> +uint32_t reg;
> +struct {
> +unsigned tpm_established:1;
> +unsigned loc_assigned:1;
> +unsigned active_locality:3;
> +unsigned reserved:2;
> +unsigned tpm_reg_valid_sts:1;
> +} bits;

I suppose this is little-endian layout, so this won't work on big-endian
hosts.

Can you add a qtest?

> +} loc_state;
> +uint32_t reserved1;
> +uint32_t loc_ctrl;
> +union {
> +uint32_t reg;
> +struct {
> +unsigned granted:1;
> +unsigned been_seized:1;
> +} bits;


This is unclear where you expect those bits (right/left aligned).

Can you add an unnamed field to be more explicit?

i.e. without using struct if left alignment expected:

   unsigned granted:1, been_seized:1, :30;

> +} loc_sts;
> +uint8_t reserved2[32];
> +union {
> +uint64_t reg;
> +struct {
> +unsigned type:4;
> +unsigned version:4;
> +unsigned cap_locality:1;
> +unsigned cap_crb_idle_bypass:1;
> +unsigned reserved1:1;
> +unsigned cap_data_xfer_size_support:2;
> +unsigned cap_fifo:1;
> +unsigned cap_crb:1;
> +unsigned cap_if_res:2;
> +unsigned if_selector:2;
> +unsigned if_selector_lock:1;
> +unsigned reserved2:4;
> +unsigned rid:8;
> +unsigned vid:16;
> +unsigned did:16;
> +} bits;
> +} intf_id;
> +uint64_t ctrl_ext;
> +
> +uint32_t ctrl_req;
> +union {
> +uint32_t reg;
> +struct {
> +unsigned tpm_sts:1;
> +unsigned tpm_idle:1;
> +unsigned reserved:30;

Oh here you use 'reserved' to left align.

> +} bits;
> +} 

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Eduardo Habkost]

On Fri, Jan 19, 2018 at 04:56:31PM -0500, Stefan Berger wrote:
> On 01/19/2018 01:42 PM, Eduardo Habkost wrote:
> > On Fri, Jan 19, 2018 at 12:10:03PM -0500, Stefan Berger wrote:
> > > On 01/19/2018 09:11 AM, Marc-André Lureau wrote:
> > > > tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB)
> > > > Interface as defined in TCG PC Client Platform TPM Profile (PTP)
> > > > Specification Family “2.0” Level 00 Revision 01.03 v22.
> > > > 
> > > > The PTP allows device implementation to switch between TIS and CRB
> > > > model at run time, but given that CRB is a simpler device to
> > > > implement, I chose to implement it as a different device.
> > > > 
> > > > The device doesn't implement other locality than 0 for now (my laptop
> > > > TPM doesn't either, so I assume this isn't so bad)
> > > > 
> > > > The command/reply memory region is statically allocated after the CRB
> > > > registers address TPM_CRB_ADDR_BASE + sizeof(struct crb_regs) (I
> > > > wonder if the BIOS could or should allocate it instead, or what size
> > > > to use, again this seems to fit well expectations)
> > > I removed this last sentence now. It's at the right location.
> > > 
> > > > The PTP doesn't specify a particular bus to put the device. So I added
> > > > it on the system bus directly, so it could hopefully be used easily on
> > > > a different platform than x86. Currently, it fails to init on piix,
> > > > because error_on_sysbus_device() check. The check may be changed in a
> > > > near future, see discussion on the qemu-devel ML.
> > > I think this has to be solved. So I remove these last 2 sentences. I'll 
> > > have
> > > to wait until that other patch series from Eduard is merged since it 
> > > doesn't
> > > start yet.
> > The series was just merged to master.  It's possible to make a
> > machine accept the new device using
> > machine_class_allow_dynamic_sysbus_dev(), now.
> 
> I saw that.
> > 
> > However, is it really necessary to make it a sysbus device?
> > Having bus-less devices was not possible in the past, but it is
> > possible today.
> > 
> What I don't like about it is the fact that I would have to use q35 if we
> only extend that machine type to allow this sysbus device. What is the
> reason that dynamic sysbus devices have to explicitly be allowed? If we
> don't need to limit this device to a certain machine type that may be more
> user friendly.

Most sysbus devices don't work unless they have additional
supporting machine code to wire them at the right addresses.
Devices that work without extra machine code (like *-iommu) are
the exception.

I think the last time I saw an explanation for this was at:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg439549.html

-- 
Eduardo