[PATCH v6 11/40] accel/hax: Implement AccelOpsClass::has_work()

[Philippe Mathieu-Daudé]

Since there is no specific HAX handling for cpu_has_work() in
cpu_thread_is_idle(), implement HAX has_work() handler as a
simple 'return false' code.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/hax/hax-accel-ops.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/target/i386/hax/hax-accel-ops.c b/target/i386/hax/hax-accel-ops.c
index 136630e9b23..5407ba17eaf 100644
--- a/target/i386/hax/hax-accel-ops.c
+++ b/target/i386/hax/hax-accel-ops.c
@@ -74,6 +74,11 @@ static void hax_start_vcpu_thread(CPUState *cpu)
 #endif
 }
 
+static bool hax_cpu_has_work(CPUState *cpu)
+{
+return false;
+}
+
 static void hax_accel_ops_class_init(ObjectClass *oc, void *data)
 {
 AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
@@ -85,6 +90,7 @@ static void hax_accel_ops_class_init(ObjectClass *oc, void 
*data)
 ops->synchronize_post_init = hax_cpu_synchronize_post_init;
 ops->synchronize_state = hax_cpu_synchronize_state;
 ops->synchronize_pre_loadvm = hax_cpu_synchronize_pre_loadvm;
+ops->has_work = hax_cpu_has_work;
 }
 
 static const TypeInfo hax_accel_ops_type = {
-- 
2.31.1

[PATCH v6 22/40] target/hppa: Restrict has_work() handler to sysemu

[Philippe Mathieu-Daudé]

Restrict has_work() to sysemu.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/hppa/cpu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index e8edd189bfc..be940ae2246 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -60,10 +60,12 @@ static void hppa_cpu_synchronize_from_tb(CPUState *cs,
 cpu->env.psw_n = (tb->flags & PSW_N) != 0;
 }
 
+#if !defined(CONFIG_USER_ONLY)
 static bool hppa_cpu_has_work(CPUState *cs)
 {
 return cs->interrupt_request & CPU_INTERRUPT_HARD;
 }
+#endif /* !CONFIG_USER_ONLY */
 
 static void hppa_cpu_disas_set_info(CPUState *cs, disassemble_info *info)
 {
@@ -147,6 +149,7 @@ static const struct TCGCPUOps hppa_tcg_ops = {
 .tlb_fill = hppa_cpu_tlb_fill,
 
 #ifndef CONFIG_USER_ONLY
+.has_work = hppa_cpu_has_work,
 .cpu_exec_interrupt = hppa_cpu_exec_interrupt,
 .do_interrupt = hppa_cpu_do_interrupt,
 .do_unaligned_access = hppa_cpu_do_unaligned_access,
@@ -163,7 +166,6 @@ static void hppa_cpu_class_init(ObjectClass *oc, void *data)
 >parent_realize);
 
 cc->class_by_name = hppa_cpu_class_by_name;
-cc->has_work = hppa_cpu_has_work;
 cc->dump_state = hppa_cpu_dump_state;
 cc->set_pc = hppa_cpu_set_pc;
 cc->gdb_read_register = hppa_cpu_gdb_read_register;
-- 
2.31.1

[PATCH v6 20/40] target/cris: Restrict has_work() handler to sysemu

[Philippe Mathieu-Daudé]

Restrict has_work() to sysemu.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/cris/cpu.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index c2e7483f5bd..b2761f8b110 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -35,10 +35,12 @@ static void cris_cpu_set_pc(CPUState *cs, vaddr value)
 cpu->env.pc = value;
 }
 
+#if !defined(CONFIG_USER_ONLY)
 static bool cris_cpu_has_work(CPUState *cs)
 {
 return cs->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI);
 }
+#endif /* !CONFIG_USER_ONLY */
 
 static void cris_cpu_reset(DeviceState *dev)
 {
@@ -208,6 +210,7 @@ static const struct TCGCPUOps crisv10_tcg_ops = {
 .tlb_fill = cris_cpu_tlb_fill,
 
 #ifndef CONFIG_USER_ONLY
+.has_work = cris_cpu_has_work,
 .cpu_exec_interrupt = cris_cpu_exec_interrupt,
 .do_interrupt = crisv10_cpu_do_interrupt,
 #endif /* !CONFIG_USER_ONLY */
@@ -218,6 +221,7 @@ static const struct TCGCPUOps crisv32_tcg_ops = {
 .tlb_fill = cris_cpu_tlb_fill,
 
 #ifndef CONFIG_USER_ONLY
+.has_work = cris_cpu_has_work,
 .cpu_exec_interrupt = cris_cpu_exec_interrupt,
 .do_interrupt = cris_cpu_do_interrupt,
 #endif /* !CONFIG_USER_ONLY */
@@ -294,7 +298,6 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
 device_class_set_parent_reset(dc, cris_cpu_reset, >parent_reset);
 
 cc->class_by_name = cris_cpu_class_by_name;
-cc->has_work = cris_cpu_has_work;
 cc->dump_state = cris_cpu_dump_state;
 cc->set_pc = cris_cpu_set_pc;
 cc->gdb_read_register = cris_cpu_gdb_read_register;
-- 
2.31.1

[PATCH v6 18/40] target/arm: Restrict has_work() handler to sysemu and TCG

[Philippe Mathieu-Daudé]

Restrict arm_cpu_has_work() and has_work() handler to TCG sysemu.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/arm/internals.h | 4 +++-
 target/arm/cpu.c   | 7 +--
 target/arm/cpu_tcg.c   | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 864b5ad4cdf..29bb815100d 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -175,9 +175,11 @@ void arm_translate_init(void);
 
 #ifdef CONFIG_TCG
 void arm_cpu_synchronize_from_tb(CPUState *cs, const TranslationBlock *tb);
-#endif /* CONFIG_TCG */
 
+#if !defined(CONFIG_USER_ONLY)
 bool arm_cpu_has_work(CPUState *cs);
+#endif /* !CONFIG_USER_ONLY */
+#endif /* CONFIG_TCG */
 
 /**
  * aarch64_sve_zcr_get_valid_len:
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 4b08f717f64..53c478171ac 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -74,8 +74,8 @@ void arm_cpu_synchronize_from_tb(CPUState *cs,
 env->regs[15] = tb->pc;
 }
 }
-#endif /* CONFIG_TCG */
 
+#ifndef CONFIG_USER_ONLY
 bool arm_cpu_has_work(CPUState *cs)
 {
 ARMCPU *cpu = ARM_CPU(cs);
@@ -86,6 +86,9 @@ bool arm_cpu_has_work(CPUState *cs)
  | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ
  | CPU_INTERRUPT_EXITTB);
 }
+#endif /* !CONFIG_USER_ONLY */
+
+#endif /* CONFIG_TCG */
 
 void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
  void *opaque)
@@ -2035,6 +2038,7 @@ static const struct TCGCPUOps arm_tcg_ops = {
 .debug_excp_handler = arm_debug_excp_handler,
 
 #if !defined(CONFIG_USER_ONLY)
+.has_work = arm_cpu_has_work,
 .cpu_exec_interrupt = arm_cpu_exec_interrupt,
 .do_interrupt = arm_cpu_do_interrupt,
 .do_transaction_failed = arm_cpu_do_transaction_failed,
@@ -2059,7 +2063,6 @@ static void arm_cpu_class_init(ObjectClass *oc, void 
*data)
 device_class_set_parent_reset(dc, arm_cpu_reset, >parent_reset);
 
 cc->class_by_name = arm_cpu_class_by_name;
-cc->has_work = arm_cpu_has_work;
 cc->dump_state = arm_cpu_dump_state;
 cc->set_pc = arm_cpu_set_pc;
 cc->gdb_read_register = arm_cpu_gdb_read_register;
diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
index 9a0927ad5d0..7d0d9fcbc79 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/cpu_tcg.c
@@ -902,6 +902,7 @@ static const struct TCGCPUOps arm_v7m_tcg_ops = {
 .debug_excp_handler = arm_debug_excp_handler,
 
 #if !defined(CONFIG_USER_ONLY)
+.has_work = arm_cpu_has_work,
 .cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt,
 .do_interrupt = arm_v7m_cpu_do_interrupt,
 .do_transaction_failed = arm_cpu_do_transaction_failed,
@@ -920,7 +921,6 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
 
 acc->info = data;
 #ifdef CONFIG_TCG
-cc->has_work = arm_cpu_has_work;
 cc->tcg_ops = _v7m_tcg_ops;
 #endif /* CONFIG_TCG */
 
-- 
2.31.1

[PATCH v6 10/40] accel/xen: Implement AccelOpsClass::has_work()

[Philippe Mathieu-Daudé]

Since there is no specific Xen handling for cpu_has_work() in
cpu_thread_is_idle(), implement Xen has_work() handler as a
simple 'return false' code.

Acked-by: Paul Durrant 
Signed-off-by: Philippe Mathieu-Daudé 
---
 accel/xen/xen-all.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 69aa7d018b2..fe5a37fa2e6 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -215,11 +215,17 @@ static const TypeInfo xen_accel_type = {
 .class_init = xen_accel_class_init,
 };
 
+static bool xen_cpu_has_work(CPUState *cpu)
+{
+return false;
+}
+
 static void xen_accel_ops_class_init(ObjectClass *oc, void *data)
 {
 AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
 ops->create_vcpu_thread = dummy_start_vcpu_thread;
+ops->has_work = xen_cpu_has_work;
 }
 
 static const TypeInfo xen_accel_ops_type = {
-- 
2.31.1

[PATCH v6 09/40] accel/hvf: Implement AccelOpsClass::has_work()

[Philippe Mathieu-Daudé]

Since there is no specific HVF handling for cpu_has_work() in
cpu_thread_is_idle(), implement HVF has_work() handler as a
simple 'return false' code.

Signed-off-by: Philippe Mathieu-Daudé 
---
 accel/hvf/hvf-accel-ops.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
index 93976f4ecea..ba7d4f20a35 100644
--- a/accel/hvf/hvf-accel-ops.c
+++ b/accel/hvf/hvf-accel-ops.c
@@ -454,6 +454,11 @@ static void hvf_start_vcpu_thread(CPUState *cpu)
cpu, QEMU_THREAD_JOINABLE);
 }
 
+static bool hvf_cpu_has_work(CPUState *cpu)
+{
+return false;
+}
+
 static void hvf_accel_ops_class_init(ObjectClass *oc, void *data)
 {
 AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
@@ -465,6 +470,7 @@ static void hvf_accel_ops_class_init(ObjectClass *oc, void 
*data)
 ops->synchronize_post_init = hvf_cpu_synchronize_post_init;
 ops->synchronize_state = hvf_cpu_synchronize_state;
 ops->synchronize_pre_loadvm = hvf_cpu_synchronize_pre_loadvm;
+ops->has_work = hvf_cpu_has_work;
 };
 static const TypeInfo hvf_accel_ops_type = {
 .name = ACCEL_OPS_NAME("hvf"),
-- 
2.31.1

[PATCH v6 19/40] target/avr: Restrict has_work() handler to sysemu

[Philippe Mathieu-Daudé]

Restrict has_work() to sysemu.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/avr/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 5d70e34dd54..6d51f91ca2c 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -195,6 +195,7 @@ static const struct SysemuCPUOps avr_sysemu_ops = {
 static const struct TCGCPUOps avr_tcg_ops = {
 .initialize = avr_cpu_tcg_init,
 .synchronize_from_tb = avr_cpu_synchronize_from_tb,
+.has_work = avr_cpu_has_work,
 .cpu_exec_interrupt = avr_cpu_exec_interrupt,
 .tlb_fill = avr_cpu_tlb_fill,
 .do_interrupt = avr_cpu_do_interrupt,
@@ -211,7 +212,6 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
 
 cc->class_by_name = avr_cpu_class_by_name;
 
-cc->has_work = avr_cpu_has_work;
 cc->dump_state = avr_cpu_dump_state;
 cc->set_pc = avr_cpu_set_pc;
 cc->memory_rw_debug = avr_cpu_memory_rw_debug;
-- 
2.31.1

[PATCH v6 15/40] accel: Simplify cpu_has_work()

[Philippe Mathieu-Daudé]

Now that all accelerators implement a has_work() handler, we can
simplify cpu_has_work() by removing the non-NULL handler check.

Add an assertion in cpus_register_accel() for future accelerators.

Suggested-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 softmmu/cpus.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index bb16a25bcef..bbb83d5982a 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -251,10 +251,7 @@ void cpu_interrupt(CPUState *cpu, int mask)
 
 bool cpu_has_work(CPUState *cpu)
 {
-if (cpus_accel->has_work && cpus_accel->has_work(cpu)) {
-return true;
-}
-return false;
+return cpus_accel->has_work(cpu);
 }
 
 static int do_vm_stop(RunState state, bool send_stop)
@@ -613,6 +610,7 @@ void cpus_register_accel(const AccelOpsClass *ops)
 
 /* Mandatory non-NULL handlers */
 assert(ops->create_vcpu_thread != NULL);
+assert(ops->has_work != NULL);
 
 cpus_accel = ops;
 }
-- 
2.31.1

[PATCH v6 07/40] accel/kvm: Implement AccelOpsClass::has_work()

[Philippe Mathieu-Daudé]

Implement KVM has_work() handler in AccelOpsClass and
remove it from cpu_thread_is_idle() since cpu_has_work()
is already called.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 accel/kvm/kvm-accel-ops.c | 6 ++
 softmmu/cpus.c| 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c
index 7516c67a3f5..6f4d5df3a0d 100644
--- a/accel/kvm/kvm-accel-ops.c
+++ b/accel/kvm/kvm-accel-ops.c
@@ -74,6 +74,11 @@ static void kvm_start_vcpu_thread(CPUState *cpu)
cpu, QEMU_THREAD_JOINABLE);
 }
 
+static bool kvm_cpu_has_work(CPUState *cpu)
+{
+return kvm_halt_in_kernel();
+}
+
 static void kvm_accel_ops_class_init(ObjectClass *oc, void *data)
 {
 AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
@@ -83,6 +88,7 @@ static void kvm_accel_ops_class_init(ObjectClass *oc, void 
*data)
 ops->synchronize_post_init = kvm_cpu_synchronize_post_init;
 ops->synchronize_state = kvm_cpu_synchronize_state;
 ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm;
+ops->has_work = kvm_cpu_has_work;
 }
 
 static const TypeInfo kvm_accel_ops_type = {
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 85b06d3e685..c9f54a09989 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -90,7 +90,7 @@ bool cpu_thread_is_idle(CPUState *cpu)
 return true;
 }
 if (!cpu->halted || cpu_has_work(cpu) ||
-kvm_halt_in_kernel() || whpx_apic_in_platform()) {
+whpx_apic_in_platform()) {
 return false;
 }
 return true;
-- 
2.31.1

[PATCH v6 04/40] hw/core: Un-inline cpu_has_work()

[Philippe Mathieu-Daudé]

We want to make cpu_has_work() per-accelerator. Only declare its
prototype and move its definition to softmmu/cpus.c.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/core/cpu.h | 8 +---
 softmmu/cpus.c| 8 
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 2bd563e221f..e2dd171a13f 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -546,13 +546,7 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
  *
  * Returns: %true if the CPU has work, %false otherwise.
  */
-static inline bool cpu_has_work(CPUState *cpu)
-{
-CPUClass *cc = CPU_GET_CLASS(cpu);
-
-g_assert(cc->has_work);
-return cc->has_work(cpu);
-}
+bool cpu_has_work(CPUState *cpu);
 
 /**
  * cpu_get_phys_page_attrs_debug:
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 646326b24fd..6a05860f7fe 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -251,6 +251,14 @@ void cpu_interrupt(CPUState *cpu, int mask)
 }
 }
 
+bool cpu_has_work(CPUState *cpu)
+{
+CPUClass *cc = CPU_GET_CLASS(cpu);
+
+g_assert(cc->has_work);
+return cc->has_work(cpu);
+}
+
 static int do_vm_stop(RunState state, bool send_stop)
 {
 int ret = 0;
-- 
2.31.1

[PATCH v6 12/40] accel/nvmm: Implement AccelOpsClass::has_work()

[Philippe Mathieu-Daudé]

Since there is no specific NVMM handling for cpu_has_work() in
cpu_thread_is_idle(), implement NVMM has_work() handler as a
simple 'return false' code.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/i386/nvmm/nvmm-accel-ops.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/target/i386/nvmm/nvmm-accel-ops.c 
b/target/i386/nvmm/nvmm-accel-ops.c
index f788f75289f..36296f79ff8 100644
--- a/target/i386/nvmm/nvmm-accel-ops.c
+++ b/target/i386/nvmm/nvmm-accel-ops.c
@@ -83,6 +83,11 @@ static void nvmm_kick_vcpu_thread(CPUState *cpu)
 cpus_kick_thread(cpu);
 }
 
+static bool nvmm_cpu_has_work(CPUState *cpu)
+{
+return false;
+}
+
 static void nvmm_accel_ops_class_init(ObjectClass *oc, void *data)
 {
 AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
@@ -94,6 +99,7 @@ static void nvmm_accel_ops_class_init(ObjectClass *oc, void 
*data)
 ops->synchronize_post_init = nvmm_cpu_synchronize_post_init;
 ops->synchronize_state = nvmm_cpu_synchronize_state;
 ops->synchronize_pre_loadvm = nvmm_cpu_synchronize_pre_loadvm;
+ops->has_work = nvmm_cpu_has_work;
 }
 
 static const TypeInfo nvmm_accel_ops_type = {
-- 
2.31.1

[PATCH v6 05/40] hw/core: Move cpu_common_has_work() to cpu_has_work()

[Philippe Mathieu-Daudé]

cpu_class_init() always register cpu_common_has_work() as
CPUClass::has_work() handler, so the assertion check in
cpu_has_work() is pointless.
Since cpu_common_has_work() simply returns 'false', we can
inline it in cpu_has_work(), improving the function readability.

Suggested-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/core/cpu-common.c | 6 --
 softmmu/cpus.c   | 6 --
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index e2f5a646046..5ed1ccdfdd5 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -143,11 +143,6 @@ static void cpu_common_reset(DeviceState *dev)
 }
 }
 
-static bool cpu_common_has_work(CPUState *cs)
-{
-return false;
-}
-
 ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
 {
 CPUClass *cc = CPU_CLASS(object_class_by_name(typename));
@@ -279,7 +274,6 @@ static void cpu_class_init(ObjectClass *klass, void *data)
 
 k->parse_features = cpu_common_parse_features;
 k->get_arch_id = cpu_common_get_arch_id;
-k->has_work = cpu_common_has_work;
 k->gdb_read_register = cpu_common_gdb_read_register;
 k->gdb_write_register = cpu_common_gdb_write_register;
 set_bit(DEVICE_CATEGORY_CPU, dc->categories);
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 6a05860f7fe..accb20f0589 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -255,8 +255,10 @@ bool cpu_has_work(CPUState *cpu)
 {
 CPUClass *cc = CPU_GET_CLASS(cpu);
 
-g_assert(cc->has_work);
-return cc->has_work(cpu);
+if (cc->has_work && cc->has_work(cpu)) {
+return true;
+}
+return false;
 }
 
 static int do_vm_stop(RunState state, bool send_stop)
-- 
2.31.1

[PATCH v6 03/40] hw/core: Restrict cpu_has_work() to sysemu

[Philippe Mathieu-Daudé]

cpu_has_work() is only called from system emulation code.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/core/cpu.h | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index bc864564cee..2bd563e221f 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -538,6 +538,22 @@ enum CPUDumpFlags {
 void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 
 #ifndef CONFIG_USER_ONLY
+/**
+ * cpu_has_work:
+ * @cpu: The vCPU to check.
+ *
+ * Checks whether the CPU has work to do.
+ *
+ * Returns: %true if the CPU has work, %false otherwise.
+ */
+static inline bool cpu_has_work(CPUState *cpu)
+{
+CPUClass *cc = CPU_GET_CLASS(cpu);
+
+g_assert(cc->has_work);
+return cc->has_work(cpu);
+}
+
 /**
  * cpu_get_phys_page_attrs_debug:
  * @cpu: The CPU to obtain the physical page address for.
@@ -636,22 +652,6 @@ CPUState *cpu_create(const char *typename);
  */
 const char *parse_cpu_option(const char *cpu_option);
 
-/**
- * cpu_has_work:
- * @cpu: The vCPU to check.
- *
- * Checks whether the CPU has work to do.
- *
- * Returns: %true if the CPU has work, %false otherwise.
- */
-static inline bool cpu_has_work(CPUState *cpu)
-{
-CPUClass *cc = CPU_GET_CLASS(cpu);
-
-g_assert(cc->has_work);
-return cc->has_work(cpu);
-}
-
 /**
  * qemu_cpu_is_self:
  * @cpu: The vCPU to check against.
-- 
2.31.1

[PATCH v6 08/40] accel/whpx: Implement AccelOpsClass::has_work()

[Philippe Mathieu-Daudé]

Implement WHPX has_work() handler in AccelOpsClass and
remove it from cpu_thread_is_idle() since cpu_has_work()
is already called.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 softmmu/cpus.c| 4 +---
 target/i386/whpx/whpx-accel-ops.c | 6 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index c9f54a09989..5ffa02f9cef 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -41,7 +41,6 @@
 #include "sysemu/replay.h"
 #include "sysemu/runstate.h"
 #include "sysemu/cpu-timers.h"
-#include "sysemu/whpx.h"
 #include "hw/boards.h"
 #include "hw/hw.h"
 #include "trace.h"
@@ -89,8 +88,7 @@ bool cpu_thread_is_idle(CPUState *cpu)
 if (cpu_is_stopped(cpu)) {
 return true;
 }
-if (!cpu->halted || cpu_has_work(cpu) ||
-whpx_apic_in_platform()) {
+if (!cpu->halted || cpu_has_work(cpu)) {
 return false;
 }
 return true;
diff --git a/target/i386/whpx/whpx-accel-ops.c 
b/target/i386/whpx/whpx-accel-ops.c
index 6bc47c53098..1f9c6d52c27 100644
--- a/target/i386/whpx/whpx-accel-ops.c
+++ b/target/i386/whpx/whpx-accel-ops.c
@@ -83,6 +83,11 @@ static void whpx_kick_vcpu_thread(CPUState *cpu)
 }
 }
 
+static bool whpx_cpu_has_work(CPUState *cpu)
+{
+return whpx_apic_in_platform();
+}
+
 static void whpx_accel_ops_class_init(ObjectClass *oc, void *data)
 {
 AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
@@ -94,6 +99,7 @@ static void whpx_accel_ops_class_init(ObjectClass *oc, void 
*data)
 ops->synchronize_post_init = whpx_cpu_synchronize_post_init;
 ops->synchronize_state = whpx_cpu_synchronize_state;
 ops->synchronize_pre_loadvm = whpx_cpu_synchronize_pre_loadvm;
+ops->has_work = whpx_cpu_has_work;
 }
 
 static const TypeInfo whpx_accel_ops_type = {
-- 
2.31.1

[PATCH v6 06/40] accel: Introduce AccelOpsClass::has_work()

[Philippe Mathieu-Daudé]

Introduce an accelerator-specific has_work() handler.
Eventually call it from cpu_has_work().

Suggested-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/sysemu/accel-ops.h | 5 +
 softmmu/cpus.c | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h
index 032f6979d76..de83f095f20 100644
--- a/include/sysemu/accel-ops.h
+++ b/include/sysemu/accel-ops.h
@@ -31,6 +31,11 @@ struct AccelOpsClass {
 void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY NON-NULL */
 void (*kick_vcpu_thread)(CPUState *cpu);
 
+/**
+ * @has_work: Callback for checking if there is work to do.
+ */
+bool (*has_work)(CPUState *cpu);
+
 void (*synchronize_post_reset)(CPUState *cpu);
 void (*synchronize_post_init)(CPUState *cpu);
 void (*synchronize_state)(CPUState *cpu);
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index accb20f0589..85b06d3e685 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -258,6 +258,9 @@ bool cpu_has_work(CPUState *cpu)
 if (cc->has_work && cc->has_work(cpu)) {
 return true;
 }
+if (cpus_accel->has_work && cpus_accel->has_work(cpu)) {
+return true;
+}
 return false;
 }
 
-- 
2.31.1

[PATCH v6 02/40] accel/tcg: Restrict cpu_handle_halt() to sysemu

[Philippe Mathieu-Daudé]

Commit 372579427a5 ("tcg: enable thread-per-vCPU") added the following
comment describing EXCP_HALTED in qemu_tcg_cpu_thread_fn():

case EXCP_HALTED:
 /* during start-up the vCPU is reset and the thread is
  * kicked several times. If we don't ensure we go back
  * to sleep in the halted state we won't cleanly
  * start-up when the vCPU is enabled.
  *
  * cpu->halted should ensure we sleep in wait_io_event
  */
 g_assert(cpu->halted);
 break;

qemu_wait_io_event() is sysemu-specific, so we can restrict the
cpu_handle_halt() call in cpu_exec() to system emulation.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 accel/tcg/cpu-exec.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 75dbc1e4e33..5fd1ed34222 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -588,8 +588,9 @@ static inline void tb_add_jump(TranslationBlock *tb, int n,
 
 static inline bool cpu_handle_halt(CPUState *cpu)
 {
+#ifndef CONFIG_USER_ONLY
 if (cpu->halted) {
-#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
+#if defined(TARGET_I386)
 if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
 X86CPU *x86_cpu = X86_CPU(cpu);
 qemu_mutex_lock_iothread();
@@ -597,13 +598,14 @@ static inline bool cpu_handle_halt(CPUState *cpu)
 cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
 qemu_mutex_unlock_iothread();
 }
-#endif
+#endif /* TARGET_I386 */
 if (!cpu_has_work(cpu)) {
 return true;
 }
 
 cpu->halted = 0;
 }
+#endif /* !CONFIG_USER_ONLY */
 
 return false;
 }
-- 
2.31.1

[PATCH v6 01/40] accel: Simplify qemu_init_vcpu()

[Philippe Mathieu-Daudé]

cpus_register_accel() already checks for ops->create_vcpu_thread
being non-NULL, so it is pointless to re-check for it in
qemu_init_vcpu().

Signed-off-by: Philippe Mathieu-Daudé 
---
 softmmu/cpus.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 071085f840b..646326b24fd 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -604,7 +604,10 @@ void cpu_remove_sync(CPUState *cpu)
 void cpus_register_accel(const AccelOpsClass *ops)
 {
 assert(ops != NULL);
-assert(ops->create_vcpu_thread != NULL); /* mandatory */
+
+/* Mandatory non-NULL handlers */
+assert(ops->create_vcpu_thread != NULL);
+
 cpus_accel = ops;
 }
 
@@ -626,7 +629,7 @@ void qemu_init_vcpu(CPUState *cpu)
 }
 
 /* accelerators all implement the AccelOpsClass */
-g_assert(cpus_accel != NULL && cpus_accel->create_vcpu_thread != NULL);
+g_assert(cpus_accel != NULL);
 cpus_accel->create_vcpu_thread(cpu);
 
 while (!cpu->created) {
-- 
2.31.1

[PATCH v6 00/40] accel: Move has_work() from CPUClass to AccelOpsClass

[Philippe Mathieu-Daudé]

Hi,

CPU has_work() is a per-accelerator handler. This series is organized
in 2 parts:
- Patches 1-15: Move has_work() from CPUClass to AccelOpsClass
- Patches 16-40: Move remainging has_work() to TCGCPUOps

I prefer to send as a single big series to be sure it is merged
at once, since the 2nd part logic (TCGCPUOps) is related to the
end of the first part (we proved remaining CPUClass::has_work
handlers are TCG specific, thus can be moved to TCGCPUOps).

Missing review:
0001-accel-Simplify-qemu_init_vcpu.patch
0005-hw-core-Move-cpu_common_has_work-to-cpu_has_work.patch
0006-accel-Introduce-AccelOpsClass-has_work.patch
0009-accel-hvf-Implement-AccelOpsClass-has_work.patch
0010-accel-xen-Implement-AccelOpsClass-has_work.patch
0011-accel-hax-Implement-AccelOpsClass-has_work.patch
0012-accel-nvmm-Implement-AccelOpsClass-has_work.patch
0013-accel-qtest-Implement-AccelOpsClass-has_work.patch
0014-accel-tcg-Implement-AccelOpsClass-has_work.patch
0015-accel-Simplify-cpu_has_work.patch
0016-accel-tcg-Introduce-TCGCPUOps-has_work.patch
0017-target-arm-Explicit-v7M-cores-use-arm_cpu_has_work-a.patch
0018-target-arm-Restrict-has_work-handler-to-sysemu-and-T.patch
0039-accel-tcg-Remove-CPUClass-has_work.patch
0040-accel-tcg-Simplify-tcg_cpu_has_work.patch

Since v5:
- Rework ARM v7M case (pm215)
- Reorder patch logic (rth)

Since v4:
- Implement arm_v7m_cpu_has_work() (new patch)
- Assert has_work() handlers are set, don't use default value
- Fix ARM v7M and cris CPUs
- Reset R-b tags on modified patches

Since v3:
- Remove pointless CONFIG_TCG uses (rth)
- Rework PPC patches, still using indirection

Since v2:
- Full rewrite, no more RFC.

Supersedes: <20210920214447.2998623-1-f4...@amsat.org>

Philippe Mathieu-Daudé (40):
  accel: Simplify qemu_init_vcpu()
  accel/tcg: Restrict cpu_handle_halt() to sysemu
  hw/core: Restrict cpu_has_work() to sysemu
  hw/core: Un-inline cpu_has_work()
  hw/core: Move cpu_common_has_work() to cpu_has_work()
  accel: Introduce AccelOpsClass::has_work()
  accel/kvm: Implement AccelOpsClass::has_work()
  accel/whpx: Implement AccelOpsClass::has_work()
  accel/hvf: Implement AccelOpsClass::has_work()
  accel/xen: Implement AccelOpsClass::has_work()
  accel/hax: Implement AccelOpsClass::has_work()
  accel/nvmm: Implement AccelOpsClass::has_work()
  accel/qtest: Implement AccelOpsClass::has_work()
  accel/tcg: Implement AccelOpsClass::has_work()
  accel: Simplify cpu_has_work()
  accel/tcg: Introduce TCGCPUOps::has_work()
  target/arm: Explicit v7M cores use arm_cpu_has_work as
CPUClass:has_work
  target/arm: Restrict has_work() handler to sysemu and TCG
  target/avr: Restrict has_work() handler to sysemu
  target/cris: Restrict has_work() handler to sysemu
  target/hexagon: Remove unused has_work() handler
  target/hppa: Restrict has_work() handler to sysemu
  target/i386: Restrict has_work() handler to sysemu and TCG
  target/m68k: Restrict has_work() handler to sysemu
  target/microblaze: Restrict has_work() handler to sysemu
  target/mips: Restrict has_work() handler to sysemu and TCG
  target/nios2: Restrict has_work() handler to sysemu
  target/openrisc: Restrict has_work() handler to sysemu
  target/ppc: Introduce PowerPCCPUClass::has_work()
  target/ppc: Restrict has_work() handlers to sysemu and TCG
  target/riscv: Restrict has_work() handler to sysemu and TCG
  target/rx: Restrict has_work() handler to sysemu
  target/s390x: Restrict has_work() handler to sysemu and TCG
  target/sh4: Restrict has_work() handler to sysemu
  target/sparc: Remove pointless use of CONFIG_TCG definition
  target/sparc: Restrict has_work() handler to sysemu
  target/tricore: Restrict has_work() handler to sysemu
  target/xtensa: Restrict has_work() handler to sysemu
  accel/tcg: Remove CPUClass::has_work()
  accel/tcg: Simplify tcg_cpu_has_work()

 include/hw/core/cpu.h | 28 +--
 include/hw/core/tcg-cpu-ops.h |  4 
 include/sysemu/accel-ops.h|  5 +
 target/arm/internals.h|  4 
 target/ppc/cpu-qom.h  |  3 +++
 accel/hvf/hvf-accel-ops.c |  6 +
 accel/kvm/kvm-accel-ops.c |  6 +
 accel/qtest/qtest.c   |  6 +
 accel/tcg/cpu-exec.c  | 10 +++--
 accel/tcg/tcg-accel-ops.c |  9 
 accel/xen/xen-all.c   |  6 +
 hw/core/cpu-common.c  |  6 -
 softmmu/cpus.c| 17 +-
 target/arm/cpu.c  |  9 +---
 target/arm/cpu_tcg.c  |  1 +
 target/avr/cpu.c  |  2 +-
 target/cris/cpu.c |  5 -
 target/hexagon/cpu.c  |  6 -
 target/hppa/cpu.c |  4 +++-
 target/i386/cpu.c |  6 -
 target/i386/hax/hax-accel-ops.c   |  6 +
 target/i386/nvmm/nvmm-accel-ops.c |  6 +
 target/i386/tcg/tcg-cpu.c |  8 ++-
 target/i386/whpx/whpx-accel-ops.c |  6 +
 target/m68k/cpu.c |  4 

Re: [question] VFIO Device Migration: The vCPU may be paused during vfio device DMA in iommu nested stage mode && vSVA

[Kirti Wankhede]

On 9/24/2021 12:17 PM, Tian, Kevin wrote:

From: Kunkun Jiang 
Sent: Friday, September 24, 2021 2:19 PM

Hi all,

I encountered a problem in vfio device migration test. The
vCPU may be paused during vfio-pci DMA in iommu nested
stage mode && vSVA. This may lead to migration fail and
other problems related to device hardware and driver
implementation.

It may be a bit early to discuss this issue, after all, the iommu
nested stage mode and vSVA are not yet mature. But judging
from the current implementation, we will definitely encounter
this problem in the future.


Yes, this is a known limitation to support migration with vSVA.



This is the current process of vSVA processing translation fault
in iommu nested stage mode (take SMMU as an example):

guest os            4.handle translation fault 5.send CMD_RESUME to vSMMU


qemu                3.inject fault into guest os 6.deliver response to
host os
(vfio/vsmmu)


host os          2.notify the qemu 7.send CMD_RESUME to SMMU
(vfio/smmu)


SMMU          1.address translation fault          8.retry or
terminate

The order is 1--->8.

Currently, qemu may pause vCPU at any step. It is possible to
pause vCPU at step 1-5, that is, in a DMA. This may lead to
migration fail and other problems related to device hardware
and driver implementation. For example, the device status
cannot be changed from RUNNING && SAVING to SAVING,
because the device DMA is not over.

As far as i can see, vCPU should not be paused during a device
IO process, such as DMA. However, currently live migration
does not pay attention to the state of vfio device when pausing
the vCPU. And if the vCPU is not paused, the vfio device is
always running. This looks like a *deadlock*.


Basically this requires:

1) stopping vCPU after stopping device (could selectively enable
this sequence for vSVA);



I don't think this is change is required. When vCPUs are at halt vCPU 
states are already saved, step 4 or 5 will be taken care by that. Then 
when device is transitioned in SAVING state, save qemu and host os state 
in the migration stream, i.e. state at step 2 and 3, depending on that 
take action while resuming, about step 6 or 7 to run.


Thanks,
Kirti


2) when stopping device, the driver should block new requests
from vCPU (queued to a pending list) and then drain all in-fly
requests including faults;
 * to block this further requires switching from fast-path to
slow trap-emulation path for the cmd portal before stopping
the device;

3) save the pending requests in the vm image and replay them
after the vm is resumed;
 * finally disable blocking by switching back to the fast-path for
the cmd portal;



Do you have any ideas to solve this problem?
Looking forward to your replay.



We verified above flow can work in our internal POC.

Thanks
Kevin

Re: Ping: [PATCH 0/2] cocoa.m: keyboard quality of life reborn

[Peter Maydell]

On Fri, 24 Sept 2021 at 00:08, Programmingkid  wrote:
>
> Hi Peter, are you reviewing cocoa patches? Should someone else see these 
> patches?

Gerd sent out a message a while back suggesting that people interested
in the cocoa UI (we have had several people recently submit patches)
ought to start reviewing each others' patches. I would certainly
prefer it if those people who are actively using and working on
the cocoa UI could take on more of this review work.

-- PMM

Re: [PATCH v3 2/2] docs/sphinx: change default role to "any"

[Peter Maydell]

On Thu, 23 Sept 2021 at 20:14, John Snow  wrote:
>
> This interprets single-backtick syntax in all of our Sphinx docs as a
> cross-reference to *something*, including Python symbols.
>
> From here on out, new uses of `backticks` will cause a build failure if
> the target cannot be referenced.
>
> Signed-off-by: John Snow 
> ---
>  docs/conf.py | 5 +
Reviewed-by: Peter Maydell 

thanks
-- PMM

[PATCH 08/11] qdev: Base object creation on QDict rather than QemuOpts

[Kevin Wolf]

QDicts are both what QMP natively uses and what the keyval parser
produces. Going through QemuOpts isn't useful for either one, so switch
the main device creation function to QDicts. By sharing more code with
the -object/object-add code path, we can even reduce the code size a
bit.

This commit doesn't remove the detour through QemuOpts from any code
path yet, but it allows the following commits to do so.

Signed-off-by: Kevin Wolf 
---
 include/hw/qdev-core.h |  8 ++---
 hw/core/qdev.c |  5 ++--
 hw/net/virtio-net.c|  4 +--
 hw/vfio/pci.c  |  4 +--
 softmmu/qdev-monitor.c | 67 +++---
 5 files changed, 41 insertions(+), 47 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1857d9698e..5b3d4704a5 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -180,7 +180,7 @@ struct DeviceState {
 char *canonical_path;
 bool realized;
 bool pending_deleted_event;
-QemuOpts *opts;
+QDict *opts;
 int hotplugged;
 bool allow_unplug_during_migration;
 BusState *parent_bus;
@@ -202,7 +202,7 @@ struct DeviceListener {
  * hide a failover device depending for example on the device
  * opts.
  */
-bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts);
+bool (*hide_device)(DeviceListener *listener, const QDict *device_opts);
 QTAILQ_ENTRY(DeviceListener) link;
 };
 
@@ -831,13 +831,13 @@ void device_listener_unregister(DeviceListener *listener);
 
 /**
  * @qdev_should_hide_device:
- * @opts: QemuOpts as passed on cmdline.
+ * @opts: options QDict
  *
  * Check if a device should be added.
  * When a device is added via qdev_device_add() this will be called,
  * and return if the device should be added now or not.
  */
-bool qdev_should_hide_device(QemuOpts *opts);
+bool qdev_should_hide_device(const QDict *opts);
 
 typedef enum MachineInitPhase {
 /* current_machine is NULL.  */
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d918b50a1d..5b889866c5 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -28,6 +28,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-qdev.h"
+#include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/visitor.h"
 #include "qemu/error-report.h"
@@ -211,7 +212,7 @@ void device_listener_unregister(DeviceListener *listener)
 QTAILQ_REMOVE(_listeners, listener, link);
 }
 
-bool qdev_should_hide_device(QemuOpts *opts)
+bool qdev_should_hide_device(const QDict *opts)
 {
 DeviceListener *listener;
 
@@ -955,7 +956,7 @@ static void device_finalize(Object *obj)
 dev->canonical_path = NULL;
 }
 
-qemu_opts_del(dev->opts);
+qobject_unref(dev->opts);
 g_free(dev->id);
 }
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f205331dcf..5684c2b2b7 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3304,7 +3304,7 @@ static void virtio_net_migration_state_notifier(Notifier 
*notifier, void *data)
 }
 
 static bool failover_hide_primary_device(DeviceListener *listener,
- QemuOpts *device_opts)
+ const QDict *device_opts)
 {
 VirtIONet *n = container_of(listener, VirtIONet, primary_listener);
 const char *standby_id;
@@ -3312,7 +3312,7 @@ static bool failover_hide_primary_device(DeviceListener 
*listener,
 if (!device_opts) {
 return false;
 }
-standby_id = qemu_opt_get(device_opts, "failover_pair_id");
+standby_id = qdict_get_try_str(device_opts, "failover_pair_id");
 if (g_strcmp0(standby_id, n->netclient_name) != 0) {
 return false;
 }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4feaa1cb68..5cdf1d4298 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -29,10 +29,10 @@
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 #include "migration/vmstate.h"
+#include "qapi/qmp/qdict.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
-#include "qemu/option.h"
 #include "qemu/range.h"
 #include "qemu/units.h"
 #include "sysemu/kvm.h"
@@ -941,7 +941,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
 }
 
 if (vfio_opt_rom_in_denylist(vdev)) {
-if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
+if (dev->opts && qdict_haskey(dev->opts, "rombar")) {
 warn_report("Device at %s is known to cause system instability"
 " issues during option rom execution",
 vdev->vbasedev.name);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index c2af906df0..c09b7430eb 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -196,34 +196,6 @@ static void qdev_print_devinfos(bool show_no_user)
 g_slist_free(list);
 }
 
-static int set_property(void *opaque, const char *name, const char *value,
-Error **errp)
-{
-Object *obj = opaque;
-   

[PATCH 07/11] qemu-option: Allow deleting opts during qemu_opts_foreach()

[Kevin Wolf]

Use QTAILQ_FOREACH_SAFE() so that the current QemuOpts can be deleted
while iterating through the whole list.

Signed-off-by: Kevin Wolf 
---
 util/qemu-option.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 61cb4a97bd..eedd08929b 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -1126,11 +1126,11 @@ int qemu_opts_foreach(QemuOptsList *list, 
qemu_opts_loopfunc func,
   void *opaque, Error **errp)
 {
 Location loc;
-QemuOpts *opts;
+QemuOpts *opts, *next;
 int rc = 0;
 
 loc_push_none();
-QTAILQ_FOREACH(opts, >head, next) {
+QTAILQ_FOREACH_SAFE(opts, >head, next, next) {
 loc_restore(>loc);
 rc = func(opaque, opts, errp);
 if (rc) {
-- 
2.31.1

[PATCH 10/11] vl: Enable JSON syntax for -device

[Kevin Wolf]

Like we already do for -object, introduce support for JSON syntax in
-device, which can be kept stable in the long term and guarantees that a
single code path with identical behaviour is used for both QMP and the
command line. Compared to the QemuOpts based code, the parser contains
less surprises and has support for non-scalar options (lists and
structs). Switching management tools to JSON means that we can more
easily change the "human" CLI syntax from QemuOpts to the keyval parser
later.

In the QAPI schema, a feature flag is added to the device-add command to
allow management tools to detect support for this.

Signed-off-by: Kevin Wolf 
---
 qapi/qdev.json | 15 +
 softmmu/vl.c   | 58 --
 2 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index b83178220b..cdc8f911b5 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -32,17 +32,23 @@
 ##
 # @device_add:
 #
+# Add a device.
+#
 # @driver: the name of the new device's driver
 #
 # @bus: the device's parent bus (device tree path)
 #
 # @id: the device's ID, must be unique
 #
-# Additional arguments depend on the type.
-#
-# Add a device.
+# Features:
+# @json-cli: If present, the "-device" command line option supports JSON
+#syntax with a structure identical to the arguments of this
+#command.
 #
 # Notes:
+#
+# Additional arguments depend on the type.
+#
 # 1. For detailed information about this command, please refer to the
 #'docs/qdev-device-use.txt' file.
 #
@@ -67,7 +73,8 @@
 ##
 { 'command': 'device_add',
   'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
-  'gen': false } # so we can get the additional arguments
+  'gen': false, # so we can get the additional arguments
+  'features': ['json-cli'] }
 
 ##
 # @device_del:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 55ab70eb97..7596d9da06 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -144,6 +144,12 @@ typedef struct ObjectOption {
 QTAILQ_ENTRY(ObjectOption) next;
 } ObjectOption;
 
+typedef struct DeviceOption {
+QDict *opts;
+Location loc;
+QTAILQ_ENTRY(DeviceOption) next;
+} DeviceOption;
+
 static const char *cpu_option;
 static const char *mem_path;
 static const char *incoming;
@@ -151,6 +157,7 @@ static const char *loadvm;
 static const char *accelerators;
 static QDict *machine_opts_dict;
 static QTAILQ_HEAD(, ObjectOption) object_opts = 
QTAILQ_HEAD_INITIALIZER(object_opts);
+static QTAILQ_HEAD(, DeviceOption) device_opts = 
QTAILQ_HEAD_INITIALIZER(device_opts);
 static ram_addr_t maxram_size;
 static uint64_t ram_slots;
 static int display_remote;
@@ -494,21 +501,39 @@ const char *qemu_get_vm_name(void)
 return qemu_name;
 }
 
-static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
+static void default_driver_disable(const char *driver)
 {
-const char *driver = qemu_opt_get(opts, "driver");
 int i;
 
-if (!driver)
-return 0;
+if (!driver) {
+return;
+}
+
 for (i = 0; i < ARRAY_SIZE(default_list); i++) {
 if (strcmp(default_list[i].driver, driver) != 0)
 continue;
 *(default_list[i].flag) = 0;
 }
+}
+
+static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
+{
+const char *driver = qemu_opt_get(opts, "driver");
+
+default_driver_disable(driver);
 return 0;
 }
 
+static void default_driver_check_json(void)
+{
+DeviceOption *opt;
+
+QTAILQ_FOREACH(opt, _opts, next) {
+const char *driver = qdict_get_try_str(opt->opts, "driver");
+default_driver_disable(driver);
+}
+}
+
 static int parse_name(void *opaque, QemuOpts *opts, Error **errp)
 {
 const char *proc_name;
@@ -1311,6 +1336,7 @@ static void qemu_disable_default_devices(void)
 {
 MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
 
+default_driver_check_json();
 qemu_opts_foreach(qemu_find_opts("device"),
   default_driver_check, NULL, NULL);
 qemu_opts_foreach(qemu_find_opts("global"),
@@ -2637,6 +2663,8 @@ static void qemu_init_board(void)
 
 static void qemu_create_cli_devices(void)
 {
+DeviceOption *opt;
+
 soundhw_init();
 
 qemu_opts_foreach(qemu_find_opts("fw_cfg"),
@@ -2652,6 +2680,13 @@ static void qemu_create_cli_devices(void)
 rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
 qemu_opts_foreach(qemu_find_opts("device"),
   device_init_func, NULL, _fatal);
+QTAILQ_FOREACH(opt, _opts, next) {
+QObject *ret_data;
+
+loc_push_restore(>loc);
+qmp_device_add(opt->opts, _data, _fatal);
+loc_pop(>loc);
+}
 rom_reset_order_override();
 }
 
@@ -3352,9 +3387,18 @@ void qemu_init(int argc, char **argv, char **envp)
 add_device_config(DEV_USB, optarg);
 break;
 case QEMU_OPTION_device:
-if 

[PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

[Kevin Wolf]

Directly call qdev_device_add_from_qdict() for QMP device_add instead of
first going through QemuOpts and converting back to QDict.

Note that this changes the behaviour of device_add, though in ways that
should be considered bug fixes:

QemuOpts ignores differences between data types, so you could
successfully pass a string "123" for an integer property, or a string
"on" for a boolean property (and vice versa).  After this change, the
correct data type for the property must be used in the JSON input.

qemu_opts_from_qdict() also silently ignores any options whose value is
a QDict, QList or QNull.

To illustrate, the following QMP command was accepted before and is now
rejected for both reasons:

{ "execute": "device_add",
  "arguments": { "driver": "scsi-cd",
 "drive": { "completely": "invalid" },
 "physical_block_size": "4096" } }

Signed-off-by: Kevin Wolf 
---
 softmmu/qdev-monitor.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index c09b7430eb..8622ccade6 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
 qdev_print_devinfos(true);
 }
 
-void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
+static void monitor_device_add(QDict *qdict, QObject **ret_data,
+   bool from_json, Error **errp)
 {
 QemuOpts *opts;
 DeviceState *dev;
@@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error 
**errp)
 qemu_opts_del(opts);
 return;
 }
-dev = qdev_device_add(opts, errp);
+qemu_opts_del(opts);
+
+dev = qdev_device_add_from_qdict(qdict, from_json, errp);
 
 /*
  * Drain all pending RCU callbacks. This is done because
@@ -838,13 +841,14 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, 
Error **errp)
  */
 drain_call_rcu();
 
-if (!dev) {
-qemu_opts_del(opts);
-return;
-}
 object_unref(OBJECT(dev));
 }
 
+void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
+{
+monitor_device_add(qdict, ret_data, true, errp);
+}
+
 static DeviceState *find_device_state(const char *id, Error **errp)
 {
 Object *obj;
@@ -936,7 +940,7 @@ void hmp_device_add(Monitor *mon, const QDict *qdict)
 {
 Error *err = NULL;
 
-qmp_device_add((QDict *)qdict, NULL, );
+monitor_device_add((QDict *)qdict, NULL, false, );
 hmp_handle_error(mon, err);
 }
 
-- 
2.31.1

Re: [PATCH v5 18/20] nubus: add support for slot IRQs

[Philippe Mathieu-Daudé]

On 9/24/21 11:01, Philippe Mathieu-Daudé wrote:

On 9/24/21 09:06, Mark Cave-Ayland wrote:

On 23/09/2021 10:49, Philippe Mathieu-Daudé wrote:


On 9/23/21 11:13, Mark Cave-Ayland wrote:
Each Nubus slot has an IRQ line that can be used to request service 
from the
CPU. Connect the IRQs to the Nubus bridge so that they can be wired 
up using qdev
gpios accordingly, and introduce a new nubus_set_irq() function that 
can be used

by Nubus devices to control the slot IRQ.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
  hw/nubus/nubus-bridge.c  | 2 ++
  hw/nubus/nubus-device.c  | 8 
  include/hw/nubus/nubus.h | 6 ++
  3 files changed, 16 insertions(+)



  static Property nubus_bridge_properties[] = {
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 280f40e88a..0f1852f671 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -10,12 +10,20 @@
  #include "qemu/osdep.h"
  #include "qemu/datadir.h"
+#include "hw/irq.h"
  #include "hw/loader.h"
  #include "hw/nubus/nubus.h"
  #include "qapi/error.h"
  #include "qemu/error-report.h"
+void nubus_set_irq(NubusDevice *nd, int level)
+{
+    NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd)));
+


A trace-event could be helpful here, otherwise:

Reviewed-by: Philippe Mathieu-Daudé 


+    qemu_set_irq(nubus->irqs[nd->slot], level);
+}


I think adding a trace event here would just be too verbose 
(particularly if you have more than one nubus device) and not 
particularly helpful: normally the part you want to debug is the in 
the device itself looking at which constituent flags combine to 
raise/lower the interrupt line. And once you've done that then you've 
already got a suitable trace-event in place...


But devices accessing the bus are not aware of which devices are plugged
onto it. Wait, what is suppose to call nubus_set_irq()? Devices on the
bus, to propagate the interrupt up to the CPU? OK so then the trace
event is irrelevant indeed. I understood this API as any external device
could trigger an IRQ to device on the bus. I wonder if renaming as
nubus_device_set_irq() could make it clearer. Or even simpler, add
a comment in "hw/nubus/nubus.h" explaining what nubus_set_irq() is for
to avoid any confusion, and we are good.


Just noticed v6 was sent, so the function description could either
- sent as reply to v6 patch and squashed by Laurent when applying
- sent later as a new cleanup patch on top
- never added

Up to you, I don't mind mind much the outcome.

[PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts

[Kevin Wolf]

DeviceState.id is a pointer to a string that is stored in the QemuOpts
object DeviceState.opts and freed together with it. We want to create
devices without going through QemuOpts in the future, so make this a
separately allocated string.

Signed-off-by: Kevin Wolf 
---
 include/hw/qdev-core.h  | 2 +-
 include/monitor/qdev.h  | 2 +-
 hw/arm/virt.c   | 2 +-
 hw/core/qdev.c  | 1 +
 hw/pci-bridge/pci_expander_bridge.c | 2 +-
 hw/ppc/e500.c   | 2 +-
 softmmu/qdev-monitor.c  | 5 +++--
 7 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 34c8a7506a..1857d9698e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -176,7 +176,7 @@ struct DeviceState {
 Object parent_obj;
 /*< public >*/
 
-const char *id;
+char *id;
 char *canonical_path;
 bool realized;
 bool pending_deleted_event;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index eaa947d73a..389287eb44 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error 
**errp);
 
 int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, const char *id);
+void qdev_set_id(DeviceState *dev, char *id);
 
 #endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1d59f0e59f..f933d48d3b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
 MemoryRegion *sysmem = get_system_memory();
 
 dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
 qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
 qdev_prop_set_uint32(dev, "mmio_size", 
vms->memmap[VIRT_PLATFORM_BUS].size);
 sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a..d918b50a1d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
 }
 
 qemu_opts_del(dev->opts);
+g_free(dev->id);
 }
 
 static void device_class_base_init(ObjectClass *class, void *data)
diff --git a/hw/pci-bridge/pci_expander_bridge.c 
b/hw/pci-bridge/pci_expander_bridge.c
index 7112dc3062..10e6e7c2ab 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool 
pcie, Error **errp)
 } else {
 bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, 
TYPE_PXB_BUS);
 bds = qdev_new("pci-bridge");
-bds->id = dev_name;
+bds->id = g_strdup(dev_name);
 qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
 qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
 }
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 95451414dd..960e7efcd3 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
 /* Platform Bus Device */
 if (pmc->has_platform_bus) {
 dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
-dev->id = TYPE_PLATFORM_BUS_DEVICE;
+dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
 qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
 qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
 sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 034b999401..1207e57a46 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp)
 return bus;
 }
 
-void qdev_set_id(DeviceState *dev, const char *id)
+/* Takes ownership of @id, will be freed when deleting the device */
+void qdev_set_id(DeviceState *dev, char *id)
 {
 if (id) {
 dev->id = id;
@@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 }
 }
 
-qdev_set_id(dev, qemu_opts_id(opts));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
 
 /* set properties */
 if (qemu_opt_foreach(opts, set_property, dev, errp)) {
-- 
2.31.1

[PATCH 11/11] Deprecate stable non-JSON -device and -object

[Kevin Wolf]

We want to switch both from QemuOpts to the keyval parser in the future,
which results in some incompatibilities, mainly around list handling.
Mark the non-JSON version of both as unstable syntax so that management
tools switch to JSON and we can later make the change without breaking
things.

Signed-off-by: Kevin Wolf 
---
 docs/about/deprecated.rst | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 3c2be84d80..42f6a478fb 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -160,6 +160,17 @@ Use ``-display sdl`` instead.
 
 Use ``-display curses`` instead.
 
+Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2)
+''
+
+If you rely on a stable interface for ``-device`` and ``-object`` that doesn't
+change incompatibly between QEMU versions (e.g. because you are using the QEMU
+command line as a machine interface in scripts rather than interactively), use
+JSON syntax for these options instead.
+
+There is no intention to remove support for non-JSON syntax entirely, but
+future versions may change the way to spell some options.
+
 
 Plugin argument passing through ``arg=`` (since 6.1)
 
-- 
2.31.1

[PATCH 04/11] qdev: Avoid using string visitor for properties

[Kevin Wolf]

The only thing the string visitor adds compared to a keyval visitor is
list support. git grep for 'visit_start_list' and 'visit.*List' shows
that devices don't make use of this.

In a world with a QAPIfied command line interface, the keyval visitor is
used to parse the command line. In order to make sure that no devices
start using this feature that would make backwards compatibility harder,
just switch away from object_property_parse(), which internally uses the
string visitor, to a keyval visitor and object_property_set().

Signed-off-by: Kevin Wolf 
---
 softmmu/qdev-monitor.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 0705f00846..034b999401 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -28,6 +28,8 @@
 #include "qapi/qmp/dispatch.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "qemu/help_option.h"
@@ -198,16 +200,28 @@ static int set_property(void *opaque, const char *name, 
const char *value,
 Error **errp)
 {
 Object *obj = opaque;
+QString *val;
+Visitor *v;
+int ret;
 
 if (strcmp(name, "driver") == 0)
 return 0;
 if (strcmp(name, "bus") == 0)
 return 0;
 
-if (!object_property_parse(obj, name, value, errp)) {
-return -1;
+val = qstring_from_str(value);
+v = qobject_input_visitor_new_keyval(QOBJECT(val));
+
+if (!object_property_set(obj, name, v, errp)) {
+ret = -1;
+goto out;
 }
-return 0;
+
+ret = 0;
+out:
+visit_free(v);
+qobject_unref(val);
+return ret;
 }
 
 static const char *find_typename_by_alias(const char *alias)
-- 
2.31.1

[PATCH 02/11] iotests/245: Fix type for iothread property

[Kevin Wolf]

iothread is a string property, so None (= JSON null) is not a valid
value for it. Pass the empty string instead to get the default iothread.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/245 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index bf8261eec0..9b12b42eed 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1189,10 +1189,10 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 self.run_test_iothreads('iothread0', 'iothread0')
 
 def test_iothreads_switch_backing(self):
-self.run_test_iothreads('iothread0', None)
+self.run_test_iothreads('iothread0', '')
 
 def test_iothreads_switch_overlay(self):
-self.run_test_iothreads(None, 'iothread0')
+self.run_test_iothreads('', 'iothread0')
 
 if __name__ == '__main__':
 iotests.activate_logging()
-- 
2.31.1

[PATCH 03/11] iotests/051: Fix typo

[Kevin Wolf]

The iothread isn't called 'iothread0', but 'thread0'. Depending on the
order that properties are parsed, the error message may change from the
expected one to another one saying that the iothread doesn't exist.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/051| 2 +-
 tests/qemu-iotests/051.pc.out | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 7bf29343d7..1d2fa93a11 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -199,7 +199,7 @@ case "$QEMU_DEFAULT_MACHINE" in
 # virtio-blk enables the iothread only when the driver initialises the
 # device, so a second virtio-blk device can't be added even with the
 # same iothread. virtio-scsi allows this.
-run_qemu $iothread -device 
virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on
+run_qemu $iothread -device 
virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on
 run_qemu $iothread -device 
virtio-scsi,id=virtio-scsi1,iothread=thread0 -device 
scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on
 ;;
  *)
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index afe7632964..063e4fc584 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -183,9 +183,9 @@ Testing: -drive 
file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) QEMU_PROG: -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on: 
Cannot change iothread of active block backend
 
-Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object 
iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 
-device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device 
virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on
+Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object 
iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 
-device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device 
virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device 
virtio-blk-pci,drive=disk,iothread=iothread0,share-rw=on: Cannot change 
iothread of active block backend
+(qemu) QEMU_PROG: -device 
virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread 
of active block backend
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object 
iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 
-device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device 
virtio-scsi,id=virtio-scsi1,iothread=thread0 -device 
scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
-- 
2.31.1

[PATCH 06/11] qdev: Add Error parameter to qdev_set_id()

[Kevin Wolf]

object_property_add_child() fails (with _abort) if an object with
the same name already exists. As long as QemuOpts is in use for -device
and device_add, it catches duplicate IDs before qdev_set_id() is even
called. However, for enabling non-QemuOpts code paths, we need to make
sure that the condition doesn't cause a crash, but fails gracefully.

Signed-off-by: Kevin Wolf 
---
 include/monitor/qdev.h  |  2 +-
 hw/xen/xen-legacy-backend.c |  3 ++-
 softmmu/qdev-monitor.c  | 16 ++--
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..7961308c75 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error 
**errp);
 
 int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, char *id);
+void qdev_set_id(DeviceState *dev, char *id, Error **errp);
 
 #endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index dd8ae1452d..17aca85ddc 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char 
*type, int dom,
 xendev = g_malloc0(ops->size);
 object_initialize(>qdev, ops->size, TYPE_XENBACKEND);
 OBJECT(xendev)->free = g_free;
-qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev));
+qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+_abort);
 qdev_realize(DEVICE(xendev), xen_sysbus, _fatal);
 object_unref(OBJECT(xendev));
 
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1207e57a46..c2af906df0 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path, Error **errp)
 }
 
 /* Takes ownership of @id, will be freed when deleting the device */
-void qdev_set_id(DeviceState *dev, char *id)
+void qdev_set_id(DeviceState *dev, char *id, Error **errp)
 {
 if (id) {
 dev->id = id;
 }
 
 if (dev->id) {
-object_property_add_child(qdev_get_peripheral(), dev->id,
-  OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral(), dev->id,
+  OBJECT(dev), errp);
 } else {
 static int anon_count;
 gchar *name = g_strdup_printf("device[%d]", anon_count++);
-object_property_add_child(qdev_get_peripheral_anon(), name,
-  OBJECT(dev));
+object_property_try_add_child(qdev_get_peripheral_anon(), name,
+  OBJECT(dev), errp);
 g_free(name);
 }
 }
 
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 {
+ERRP_GUARD();
 DeviceClass *dc;
 const char *driver, *path;
 DeviceState *dev = NULL;
@@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 }
 }
 
-qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp);
+if (*errp) {
+goto err_del_dev;
+}
 
 /* set properties */
 if (qemu_opt_foreach(opts, set_property, dev, errp)) {
-- 
2.31.1

[PATCH 00/11] qdev: Add JSON -device and fix QMP device_add

[Kevin Wolf]

It's still a long way until we'll have QAPIfied devices, but there are
some improvements that we can already make now to make the future switch
easier.

One important part of this is having code paths without QemuOpts, which
we want to get rid of and replace with the keyval parser in the long
run. This series adds support for JSON syntax to -device, which bypasses
QemuOpts.

While we're not using QAPI yet, devices are based on QOM, so we already
do have type checks and an implied schema. JSON syntax supported now can
be supported by QAPI later and regarding command line compatibility,
actually switching to it becomes an implementation detail this way (of
course, it will still add valuable user-visible features like
introspection and documentation).

Apart from making things more future proof, this also immediately adds
a way to do non-scalar properties on the command line. nvme could have
used list support recently, and the lack of it in -device led to some
rather unnatural solution in the first version (doing the relationship
between a device and objects backwards) and loss of features in the
following. With this series, using a list as a device property should be
possible without any weird tricks.

Unfortunately, even QMP device_add goes through QemuOpts before this
series, which destroys any type safety QOM provides and also can't
support non-scalar properties. This is a bug that is fixed during this
series, but that is technically an incompatible change. A similar change
was made for netdev_add in commit db2a380c84.

libvirt needs to make sure that it actually always passes the right
type, because passing a wrong type will start to fail after this
series. I hope that libvirt already does things correctly, so this
won't cause any trouble, but if it does, there is the option of using
the QemuOpts-less code path only for JSON -device and going through a
deprecation period for QMP device_add.

Kevin Wolf (11):
  qom: Reduce use of error_propagate()
  iotests/245: Fix type for iothread property
  iotests/051: Fix typo
  qdev: Avoid using string visitor for properties
  qdev: Make DeviceState.id independent of QemuOpts
  qdev: Add Error parameter to qdev_set_id()
  qemu-option: Allow deleting opts during qemu_opts_foreach()
  qdev: Base object creation on QDict rather than QemuOpts
  qdev: Avoid QemuOpts in QMP device_add
  vl: Enable JSON syntax for -device
  Deprecate stable non-JSON -device and -object

 qapi/qdev.json  | 15 +++--
 docs/about/deprecated.rst   | 11 
 include/hw/qdev-core.h  | 10 ++--
 include/monitor/qdev.h  |  2 +-
 hw/arm/virt.c   |  2 +-
 hw/core/qdev.c  |  6 +-
 hw/net/virtio-net.c |  4 +-
 hw/pci-bridge/pci_expander_bridge.c |  2 +-
 hw/ppc/e500.c   |  2 +-
 hw/vfio/pci.c   |  4 +-
 hw/xen/xen-legacy-backend.c |  3 +-
 qom/object.c|  7 +--
 qom/object_interfaces.c | 17 ++
 softmmu/qdev-monitor.c  | 90 +
 softmmu/vl.c| 58 ---
 util/qemu-option.c  |  4 +-
 tests/qemu-iotests/051  |  2 +-
 tests/qemu-iotests/051.pc.out   |  4 +-
 tests/qemu-iotests/245  |  4 +-
 19 files changed, 161 insertions(+), 86 deletions(-)

-- 
2.31.1

[PATCH 01/11] qom: Reduce use of error_propagate()

[Kevin Wolf]

ERRP_GUARD() makes debugging easier by making sure that _abort
still fails at the real origin of the error instead of
error_propagate().

Signed-off-by: Kevin Wolf 
---
 qom/object.c|  7 +++
 qom/object_interfaces.c | 17 ++---
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index e86cb05b84..6be710bc40 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1389,7 +1389,7 @@ bool object_property_get(Object *obj, const char *name, 
Visitor *v,
 bool object_property_set(Object *obj, const char *name, Visitor *v,
  Error **errp)
 {
-Error *err = NULL;
+ERRP_GUARD();
 ObjectProperty *prop = object_property_find_err(obj, name, errp);
 
 if (prop == NULL) {
@@ -1400,9 +1400,8 @@ bool object_property_set(Object *obj, const char *name, 
Visitor *v,
 error_setg(errp, QERR_PERMISSION_DENIED);
 return false;
 }
-prop->set(obj, v, name, prop->opaque, );
-error_propagate(errp, err);
-return !err;
+prop->set(obj, v, name, prop->opaque, errp);
+return !*errp;
 }
 
 bool object_property_set_str(Object *obj, const char *name,
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ad9b56b59a..80691e88cd 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -45,26 +45,21 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
 static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
  Visitor *v, Error **errp)
 {
+ERRP_GUARD();
 const QDictEntry *e;
-Error *local_err = NULL;
 
-if (!visit_start_struct(v, NULL, NULL, 0, _err)) {
-goto out;
+if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
+return;
 }
 for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-if (!object_property_set(obj, e->key, v, _err)) {
+if (!object_property_set(obj, e->key, v, errp)) {
 break;
 }
 }
-if (!local_err) {
-visit_check_struct(v, _err);
+if (!*errp) {
+visit_check_struct(v, errp);
 }
 visit_end_struct(v, NULL);
-
-out:
-if (local_err) {
-error_propagate(errp, local_err);
-}
 }
 
 void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
-- 
2.31.1

Re: [PATCH v6 05/20] nubus: move slot bitmap checks from NubusDevice realize() to BusClass check_address()

[Philippe Mathieu-Daudé]

On 9/24/21 09:37, Mark Cave-Ayland wrote:

Allow Nubus to manage the slot allocations itself using the BusClass 
check_address()
virtual function rather than managing this during NubusDevice realize().

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
  hw/nubus/nubus-bus.c| 29 +
  hw/nubus/nubus-device.c | 21 -
  2 files changed, 29 insertions(+), 21 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v6 04/20] nubus: use bitmap to manage available slots

[Philippe Mathieu-Daudé]

On 9/24/21 09:37, Mark Cave-Ayland wrote:

Convert nubus_device_realize() to use a bitmap to manage available slots to 
allow
for future Nubus devices to be plugged into arbitrary slots from the command 
line
using a new qdev "slot" parameter for nubus devices.

Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on Macintosh 
machines
as documented in "Designing Cards and Drivers for the Macintosh Family".

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
  hw/nubus/mac-nubus-bridge.c |  4 
  hw/nubus/nubus-bus.c|  5 +++--
  hw/nubus/nubus-device.c | 29 -
  include/hw/nubus/mac-nubus-bridge.h |  4 
  include/hw/nubus/nubus.h| 13 ++---
  5 files changed, 41 insertions(+), 14 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v5 18/20] nubus: add support for slot IRQs

[Philippe Mathieu-Daudé]

On 9/24/21 09:06, Mark Cave-Ayland wrote:

On 23/09/2021 10:49, Philippe Mathieu-Daudé wrote:


On 9/23/21 11:13, Mark Cave-Ayland wrote:
Each Nubus slot has an IRQ line that can be used to request service 
from the
CPU. Connect the IRQs to the Nubus bridge so that they can be wired 
up using qdev
gpios accordingly, and introduce a new nubus_set_irq() function that 
can be used

by Nubus devices to control the slot IRQ.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
  hw/nubus/nubus-bridge.c  | 2 ++
  hw/nubus/nubus-device.c  | 8 
  include/hw/nubus/nubus.h | 6 ++
  3 files changed, 16 insertions(+)



  static Property nubus_bridge_properties[] = {
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 280f40e88a..0f1852f671 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -10,12 +10,20 @@
  #include "qemu/osdep.h"
  #include "qemu/datadir.h"
+#include "hw/irq.h"
  #include "hw/loader.h"
  #include "hw/nubus/nubus.h"
  #include "qapi/error.h"
  #include "qemu/error-report.h"
+void nubus_set_irq(NubusDevice *nd, int level)
+{
+    NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd)));
+


A trace-event could be helpful here, otherwise:

Reviewed-by: Philippe Mathieu-Daudé 


+    qemu_set_irq(nubus->irqs[nd->slot], level);
+}


I think adding a trace event here would just be too verbose 
(particularly if you have more than one nubus device) and not 
particularly helpful: normally the part you want to debug is the in the 
device itself looking at which constituent flags combine to raise/lower 
the interrupt line. And once you've done that then you've already got a 
suitable trace-event in place...


But devices accessing the bus are not aware of which devices are plugged
onto it. Wait, what is suppose to call nubus_set_irq()? Devices on the
bus, to propagate the interrupt up to the CPU? OK so then the trace
event is irrelevant indeed. I understood this API as any external device
could trigger an IRQ to device on the bus. I wonder if renaming as
nubus_device_set_irq() could make it clearer. Or even simpler, add
a comment in "hw/nubus/nubus.h" explaining what nubus_set_irq() is for
to avoid any confusion, and we are good.

Re: [PATCH v5 04/20] nubus: use bitmap to manage available slots

[Philippe Mathieu-Daudé]

On 9/24/21 09:16, Mark Cave-Ayland wrote:

On 23/09/2021 15:16, BALATON Zoltan wrote:


On Thu, 23 Sep 2021, Mark Cave-Ayland wrote:
Convert nubus_device_realize() to use a bitmap to manage available 
slots to allow
for future Nubus devices to be plugged into arbitrary slots from the 
command line

using a new qdev "slot" parameter for nubus devices.

Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a 
Macintosh
machines as documented in "Desigining Cards and Drivers for the 
Macintosh Family".


Small typo: "a Macintosh machnies", either a or s is not needed.


Thanks - I've updated this for v6.


Signed-off-by: Mark Cave-Ayland 
---
hw/nubus/mac-nubus-bridge.c |  4 
hw/nubus/nubus-bus.c    |  5 +++--
hw/nubus/nubus-device.c | 32 +++--
include/hw/nubus/mac-nubus-bridge.h |  4 
include/hw/nubus/nubus.h    | 13 ++--
5 files changed, 43 insertions(+), 15 deletions(-)



struct NubusDevice {
    DeviceState qdev;

-    int slot;
+    int32_t slot;


Why uint32_t? Considering its max value even uint8_t would be enough 
although maybe invalid value would be 255 instead of -1 then. As this 
was added in previous patch you could avoid churn here by introducing 
it with the right type in that patch already. (But feel free to ignore 
it if you have no time for more changes, the current version works so 
if you don't do another version for other reasons this probably don't 
worth the effort alone.)


I think it makes sense to keep this signed since -1 is used for other 
bus implementations to indicate that an explicit slot hasn't been 
assigned. Potentially the slot number could be represented by an 8-bit 
value, however it seems there is no DEFINE_PROP_INT8 or 
DEFINE_PROP_INT16. Fortunately the slot number is restricted by the 
available slots bitmask anyhow, so this shouldn't be an issue.


I wondered the same and noticed there is no DEFINE_PROP_INT8, so didn't
want to bother Mark furthermore :) Adding & using DEFINE_PROP_INT8 seems
a good idea, but to be fair with the repository we'd need to audit the
other places where DEFINE_PROP_INT32 isn't justified and update. Extra
work for not much gain, so I'm find with this patch. Can be improved on
top.

Re: [PATCH] hw/misc: Add an iBT device model

[Philippe Mathieu-Daudé]

On 9/24/21 01:48, Titus Rwantare wrote:

Hello all,

I'd like some clarification on how the following code transfers irqs
back and forth:


b/hw/arm/aspeed_soc.c
+/* iBT */
+if (!sysbus_realize(SYS_BUS_DEVICE(>ibt), errp)) {
+return;
+}
+memory_region_add_subregion(>lpc.iomem,
+   sc->memmap[ASPEED_DEV_IBT] - sc->memmap[ASPEED_DEV_LPC],
+   >ibt.iomem);
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_ibt,


Code smell indeed, likely:

sysbus_connect_irq(SYS_BUS_DEVICE(>ibt), 1 + aspeed_lpc_ibt,


+   qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_ibt));
}


and


hw/misc/aspeed_ibt.c
+static void aspeed_ibt_realize(DeviceState *dev, Error **errp)
+{
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+AspeedIBTState *ibt = ASPEED_IBT(dev);

...

+
+sysbus_init_irq(sbd, >irq);


I ask because the code in aspeed_soc.c seems to connect to the
lpc->subdevice_irqs[aspeed_lpc_ibt], initialised on
hw/misc/aspeed_lpc.c:408.
I noticed that bmc firmware running in qemu was checking the BT_CTRL
register less frequently than I'd like while editing this patch to use
the IPMIInterface.

Thanks,
Titus

[PATCH v6 20/20] q800: configure nubus available slots for Quadra 800

[Mark Cave-Ayland]

Slot 0x9 is reserved for use by the in-built framebuffer whilst only slots
0xc, 0xd and 0xe physically exist on the Quadra 800.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/m68k/q800.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 5bc9df58a0..09b3366024 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -78,6 +78,13 @@
 
 #define MAC_CLOCK  3686418
 
+/*
+ * Slot 0x9 is reserved for use by the in-built framebuffer whilst only
+ * slots 0xc, 0xd and 0xe physically exist on the Quadra 800
+ */
+#define Q800_NUBUS_SLOTS_AVAILABLE(BIT(0x9) | BIT(0xc) | BIT(0xd) | \
+   BIT(0xe))
+
 /*
  * The GLUE (General Logic Unit) is an Apple custom integrated circuit chip
  * that performs a variety of functions (RAM management, clock generation, 
...).
@@ -392,6 +399,8 @@ static void q800_init(MachineState *machine)
 /* NuBus */
 
 dev = qdev_new(TYPE_MAC_NUBUS_BRIDGE);
+qdev_prop_set_uint32(dev, "slot-available-mask",
+ Q800_NUBUS_SLOTS_AVAILABLE);
 sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
 MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE);
-- 
2.20.1

[PATCH v6 11/20] nubus-device: add romfile property for loading declaration ROMs

[Mark Cave-Ayland]

The declaration ROM is located at the top-most address of the standard slot
space.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-device.c  | 44 +++-
 include/hw/nubus/nubus.h |  6 ++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index d4932d64a2..280f40e88a 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -9,16 +9,21 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/datadir.h"
+#include "hw/loader.h"
 #include "hw/nubus/nubus.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 
 
 static void nubus_device_realize(DeviceState *dev, Error **errp)
 {
 NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(dev));
 NubusDevice *nd = NUBUS_DEVICE(dev);
-char *name;
+char *name, *path;
 hwaddr slot_offset;
+int64_t size;
+int ret;
 
 /* Super */
 slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
@@ -38,10 +43,47 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 memory_region_add_subregion(>slot_io, slot_offset,
 >slot_mem);
 g_free(name);
+
+/* Declaration ROM */
+if (nd->romfile != NULL) {
+path = qemu_find_file(QEMU_FILE_TYPE_BIOS, nd->romfile);
+if (path == NULL) {
+path = g_strdup(nd->romfile);
+}
+
+size = get_image_size(path);
+if (size < 0) {
+error_setg(errp, "failed to find romfile \"%s\"", nd->romfile);
+g_free(path);
+return;
+} else if (size == 0) {
+error_setg(errp, "romfile \"%s\" is empty", nd->romfile);
+g_free(path);
+return;
+} else if (size > NUBUS_DECL_ROM_MAX_SIZE) {
+error_setg(errp, "romfile \"%s\" too large (maximum size 128K)",
+   nd->romfile);
+g_free(path);
+return;
+}
+
+name = g_strdup_printf("nubus-slot-%x-declaration-rom", nd->slot);
+memory_region_init_rom(>decl_rom, OBJECT(dev), name, size,
+   _abort);
+ret = load_image_mr(path, >decl_rom);
+g_free(path);
+if (ret < 0) {
+error_setg(errp, "could not load romfile \"%s\"", nd->romfile);
+return;
+}
+memory_region_add_subregion(>slot_mem, NUBUS_SLOT_SIZE - size,
+>decl_rom);
+}
 }
 
 static Property nubus_device_properties[] = {
 DEFINE_PROP_INT32("slot", NubusDevice, slot, -1),
+DEFINE_PROP_STRING("romfile", NubusDevice, romfile),
 DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 187ecc00a5..343be95841 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -12,6 +12,7 @@
 #include "hw/qdev-properties.h"
 #include "exec/address-spaces.h"
 #include "qom/object.h"
+#include "qemu/units.h"
 
 #define NUBUS_SUPER_SLOT_SIZE 0x1000U
 #define NUBUS_SUPER_SLOT_NB   0xe
@@ -38,12 +39,17 @@ struct NubusBus {
 uint16_t slot_available_mask;
 };
 
+#define NUBUS_DECL_ROM_MAX_SIZE(128 * KiB)
+
 struct NubusDevice {
 DeviceState qdev;
 
 int32_t slot;
 MemoryRegion super_slot_mem;
 MemoryRegion slot_mem;
+
+char *romfile;
+MemoryRegion decl_rom;
 };
 
 #endif
-- 
2.20.1

[PATCH v6 04/20] nubus: use bitmap to manage available slots

[Mark Cave-Ayland]

Convert nubus_device_realize() to use a bitmap to manage available slots to 
allow
for future Nubus devices to be plugged into arbitrary slots from the command 
line
using a new qdev "slot" parameter for nubus devices.

Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on Macintosh 
machines
as documented in "Designing Cards and Drivers for the Macintosh Family".

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/mac-nubus-bridge.c |  4 
 hw/nubus/nubus-bus.c|  5 +++--
 hw/nubus/nubus-device.c | 29 -
 include/hw/nubus/mac-nubus-bridge.h |  4 
 include/hw/nubus/nubus.h| 13 ++---
 5 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index 7c329300b8..3f075789e9 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -18,6 +18,10 @@ static void mac_nubus_bridge_init(Object *obj)
 
 s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL));
 
+/* Macintosh only has slots 0x9 to 0xe available */
+s->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
+  MAC_NUBUS_SLOT_NB);
+
 sysbus_init_mmio(sbd, >bus->super_slot_io);
 sysbus_init_mmio(sbd, >bus->slot_io);
 }
diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index f4410803ff..3cd7534864 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -86,13 +86,14 @@ static void nubus_init(Object *obj)
 
 memory_region_init_io(>super_slot_io, obj, _super_slot_ops,
   nubus, "nubus-super-slots",
-  NUBUS_SUPER_SLOT_NB * NUBUS_SUPER_SLOT_SIZE);
+  (NUBUS_SUPER_SLOT_NB + 1) * NUBUS_SUPER_SLOT_SIZE);
 
 memory_region_init_io(>slot_io, obj, _slot_ops,
   nubus, "nubus-slots",
   NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);
 
-nubus->current_slot = NUBUS_FIRST_SLOT;
+nubus->slot_available_mask = MAKE_64BIT_MASK(NUBUS_FIRST_SLOT,
+ NUBUS_SLOT_NB);
 }
 
 static void nubus_class_init(ObjectClass *oc, void *data)
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 4e23df1280..2e96d6b4fc 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -161,13 +161,26 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 char *name;
 hwaddr slot_offset;
 
-if (nubus->current_slot < NUBUS_FIRST_SLOT ||
-nubus->current_slot > NUBUS_LAST_SLOT) {
-error_setg(errp, "Cannot register nubus card, not enough slots");
-return;
+if (nd->slot == -1) {
+/* No slot specified, find first available free slot */
+int s = ctz32(nubus->slot_available_mask);
+if (s != 32) {
+nd->slot = s;
+} else {
+error_setg(errp, "Cannot register nubus card, no free slot "
+ "available");
+return;
+}
+} else {
+/* Slot specified, make sure the slot is available */
+if (!(nubus->slot_available_mask & BIT(nd->slot))) {
+error_setg(errp, "Cannot register nubus card, slot %d is "
+ "unavailable or already occupied", nd->slot);
+return;
+}
 }
 
-nd->slot = nubus->current_slot++;
+nubus->slot_available_mask &= ~BIT(nd->slot);
 
 /* Super */
 slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
@@ -191,12 +204,18 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 nubus_register_format_block(nd);
 }
 
+static Property nubus_device_properties[] = {
+DEFINE_PROP_INT32("slot", NubusDevice, slot, -1),
+DEFINE_PROP_END_OF_LIST()
+};
+
 static void nubus_device_class_init(ObjectClass *oc, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(oc);
 
 dc->realize = nubus_device_realize;
 dc->bus_type = TYPE_NUBUS_BUS;
+device_class_set_props(dc, nubus_device_properties);
 }
 
 static const TypeInfo nubus_device_type_info = {
diff --git a/include/hw/nubus/mac-nubus-bridge.h 
b/include/hw/nubus/mac-nubus-bridge.h
index 36aa098dd4..118d67267d 100644
--- a/include/hw/nubus/mac-nubus-bridge.h
+++ b/include/hw/nubus/mac-nubus-bridge.h
@@ -12,6 +12,10 @@
 #include "hw/nubus/nubus.h"
 #include "qom/object.h"
 
+#define MAC_NUBUS_FIRST_SLOT 0x9
+#define MAC_NUBUS_LAST_SLOT  0xe
+#define MAC_NUBUS_SLOT_NB(MAC_NUBUS_LAST_SLOT - MAC_NUBUS_FIRST_SLOT + 1)
+
 #define TYPE_MAC_NUBUS_BRIDGE "mac-nubus-bridge"
 OBJECT_DECLARE_SIMPLE_TYPE(MacNubusState, MAC_NUBUS_BRIDGE)
 
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 89b0976aaa..3eea2952d5 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -14,13 +14,12 @@
 #include "qom/object.h"
 
 #define NUBUS_SUPER_SLOT_SIZE 0x1000U
-#define 

[PATCH v6 19/20] q800: wire up nubus IRQs

[Mark Cave-Ayland]

Nubus IRQs are routed to the CPU through the VIA2 device so wire up the IRQs
using gpios accordingly.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/m68k/q800.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 074acf4fdc..5bc9df58a0 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -398,6 +398,12 @@ static void q800_init(MachineState *machine)
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
 MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE);
 
+for (i = 0; i < VIA2_NUBUS_IRQ_NB; i++) {
+qdev_connect_gpio_out(dev, 9 + i,
+  qdev_get_gpio_in_named(via2_dev, "nubus-irq",
+ VIA2_NUBUS_IRQ_9 + i));
+}
+
 nubus = _BRIDGE(dev)->bus;
 
 /* framebuffer in nubus slot #9 */
-- 
2.20.1

[PATCH v6 16/20] nubus-bridge: embed the NubusBus object directly within nubus-bridge

[Mark Cave-Ayland]

Since nubus-bridge is a container for NubusBus then it should be embedded
directly within the bridge device using qbus_create_inplace().

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/m68k/q800.c  | 2 +-
 hw/nubus/mac-nubus-bridge.c | 9 +
 hw/nubus/nubus-bridge.c | 3 ++-
 include/hw/nubus/nubus.h| 2 +-
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 9bdea1a362..074acf4fdc 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -398,7 +398,7 @@ static void q800_init(MachineState *machine)
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
 MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE);
 
-nubus = NUBUS_BRIDGE(dev)->bus;
+nubus = _BRIDGE(dev)->bus;
 
 /* framebuffer in nubus slot #9 */
 
diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index db8640eed2..a0da5a8b2f 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -18,19 +18,20 @@ static void mac_nubus_bridge_init(Object *obj)
 MacNubusBridge *s = MAC_NUBUS_BRIDGE(obj);
 NubusBridge *nb = NUBUS_BRIDGE(obj);
 SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+NubusBus *bus = >bus;
 
 /* Macintosh only has slots 0x9 to 0xe available */
-nb->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
-   MAC_NUBUS_SLOT_NB);
+bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
+   MAC_NUBUS_SLOT_NB);
 
 /* Aliases for slots 0x9 to 0xe */
 memory_region_init_alias(>super_slot_alias, obj, "super-slot-alias",
- >bus->nubus_mr,
+ >nubus_mr,
  MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE,
  MAC_NUBUS_SLOT_NB * NUBUS_SUPER_SLOT_SIZE);
 
 memory_region_init_alias(>slot_alias, obj, "slot-alias",
- >bus->nubus_mr,
+ >nubus_mr,
  NUBUS_SLOT_BASE +
  MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE,
  MAC_NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);
diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c
index 3b68d4435c..1adda7f5a6 100644
--- a/hw/nubus/nubus-bridge.c
+++ b/hw/nubus/nubus-bridge.c
@@ -16,8 +16,9 @@
 static void nubus_bridge_init(Object *obj)
 {
 NubusBridge *s = NUBUS_BRIDGE(obj);
+NubusBus *bus = >bus;
 
-s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL));
+qbus_create_inplace(bus, sizeof(s->bus), TYPE_NUBUS_BUS, DEVICE(s), NULL);
 }
 
 static void nubus_bridge_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 2d00d18150..63c69a7586 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -63,7 +63,7 @@ struct NubusDevice {
 struct NubusBridge {
 SysBusDevice parent_obj;
 
-NubusBus *bus;
+NubusBus bus;
 };
 
 #endif
-- 
2.20.1

[PATCH v6 18/20] nubus: add support for slot IRQs

[Mark Cave-Ayland]

Each Nubus slot has an IRQ line that can be used to request service from the
CPU. Connect the IRQs to the Nubus bridge so that they can be wired up using 
qdev
gpios accordingly, and introduce a new nubus_set_irq() function that can be used
by Nubus devices to control the slot IRQ.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/nubus/nubus-bridge.c  | 2 ++
 hw/nubus/nubus-device.c  | 8 
 include/hw/nubus/nubus.h | 6 ++
 3 files changed, 16 insertions(+)

diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c
index 7b51722f66..c517a8a704 100644
--- a/hw/nubus/nubus-bridge.c
+++ b/hw/nubus/nubus-bridge.c
@@ -19,6 +19,8 @@ static void nubus_bridge_init(Object *obj)
 NubusBus *bus = >bus;
 
 qbus_create_inplace(bus, sizeof(s->bus), TYPE_NUBUS_BUS, DEVICE(s), NULL);
+
+qdev_init_gpio_out(DEVICE(s), bus->irqs, NUBUS_IRQS);
 }
 
 static Property nubus_bridge_properties[] = {
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 280f40e88a..0f1852f671 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -10,12 +10,20 @@
 
 #include "qemu/osdep.h"
 #include "qemu/datadir.h"
+#include "hw/irq.h"
 #include "hw/loader.h"
 #include "hw/nubus/nubus.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 
 
+void nubus_set_irq(NubusDevice *nd, int level)
+{
+NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd)));
+
+qemu_set_irq(nubus->irqs[nd->slot], level);
+}
+
 static void nubus_device_realize(DeviceState *dev, Error **errp)
 {
 NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(dev));
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 63c69a7586..b3b4d2eadb 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -26,6 +26,8 @@
 #define NUBUS_LAST_SLOT   0xf
 #define NUBUS_SLOT_NB (NUBUS_LAST_SLOT - NUBUS_FIRST_SLOT + 1)
 
+#define NUBUS_IRQS16
+
 #define TYPE_NUBUS_DEVICE "nubus-device"
 OBJECT_DECLARE_SIMPLE_TYPE(NubusDevice, NUBUS_DEVICE)
 
@@ -45,6 +47,8 @@ struct NubusBus {
 MemoryRegion slot_io;
 
 uint16_t slot_available_mask;
+
+qemu_irq irqs[NUBUS_IRQS];
 };
 
 #define NUBUS_DECL_ROM_MAX_SIZE(128 * KiB)
@@ -60,6 +64,8 @@ struct NubusDevice {
 MemoryRegion decl_rom;
 };
 
+void nubus_set_irq(NubusDevice *nd, int level);
+
 struct NubusBridge {
 SysBusDevice parent_obj;
 
-- 
2.20.1

Re: [PATCH] hw/nvme: reattach subsystem namespaces on hotplug

[Klaus Jensen]

On Sep 24 08:05, Hannes Reinecke wrote:
> On 9/23/21 10:09 PM, Klaus Jensen wrote:
> > On Sep  9 13:37, Hannes Reinecke wrote:
> > > On 9/9/21 12:47 PM, Klaus Jensen wrote:
> > > > On Sep  9 11:43, Hannes Reinecke wrote:
> > > > > With commit 5ffbaeed16 ("hw/nvme: fix controller hot unplugging")
> > > > > namespaces get moved from the controller to the subsystem if one
> > > > > is specified.
> > > > > That keeps the namespaces alive after a controller hot-unplug, but
> > > > > after a controller hotplug we have to reconnect the namespaces
> > > > > from the subsystem to the controller.
> > > > > 
> > > > > Fixes: 5ffbaeed16 ("hw/nvme: fix controller hot unplugging")
> > > > > Cc: Klaus Jensen 
> > > > > Signed-off-by: Hannes Reinecke 
> > > > > ---
> > > > >   hw/nvme/subsys.c | 8 +++-
> > > > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c
> > > > > index 93c35950d6..a9404f2b5e 100644
> > > > > --- a/hw/nvme/subsys.c
> > > > > +++ b/hw/nvme/subsys.c
> > > > > @@ -14,7 +14,7 @@
> > > > >   int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
> > > > >   {
> > > > >   NvmeSubsystem *subsys = n->subsys;
> > > > > -int cntlid;
> > > > > +int cntlid, nsid;
> > > > >   for (cntlid = 0; cntlid < ARRAY_SIZE(subsys->ctrls); cntlid++) {
> > > > >   if (!subsys->ctrls[cntlid]) {
> > > > > @@ -29,12 +29,18 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error 
> > > > > **errp)
> > > > >   subsys->ctrls[cntlid] = n;
> > > > > +for (nsid = 0; nsid < ARRAY_SIZE(subsys->namespaces); nsid++) {
> > > > > +if (subsys->namespaces[nsid]) {
> > > > > +nvme_attach_ns(n, subsys->namespaces[nsid]);
> > > > > +}
> > > > 
> > > > Thanks Hannes! I like it, keeping things simple.
> > > > 
> > > > But we should only attach namespaces that have the shared property or
> > > > have ns->attached == 0. Non-shared namespaces may already be attached to
> > > > another controller in the subsystem.
> > > > 
> > > 
> > > Well ... I tried to avoid that subject, but as you brought it up:
> > > There is a slightly tricky issue in fabrics, in that the 'controller' is
> > > independent from the 'connection'.
> > > The 'shared' bit in the CMIC setting indicates that the subsystem may
> > > have more than one _controller_. It doesn't talk about how many
> > > _connections_ a controller may support; that then is the realm of
> > > dynamic or static controllers, which we don't talk about :-).
> > > Sufficient to say, linux only implements the dynamic controller model,
> > > so every connection will be to a different controller.
> > > But you have been warned :-)
> > > 
> > > However, the 'CMIC' setting is independent on the 'NMIC' setting (ie the
> > > 'shared' bit in the namespace).
> > > Which leads to the interesting question on how exactly should one handle
> > > non-shared namespaces in subsystems for which there are multiple
> > > controllers. Especially as the NSID space is per subsystem, so each
> > > controller will be able to figure out if there are blanked-out namespaces.
> > > So to make this a sane setup I would propose to set the 'shared' option
> > > automatically whenever the controller has the 'subsys' option set.
> > > And actually, I would ditch the 'shared' option completely, and make it
> > > dependent on the 'subsys' setting for the controller.
> > > Much like we treat the 'CMIC' setting nowadays.
> > > That avoids lots of confusions, and also make the implementation _way_
> > > easier.
> > > 
> > 
> > I see your point. Unfortunately, there is no easy way to ditch shared=
> > now. But I think it'd be good enough to attach to shared automatically,
> > but not to "shared=off".
> > 
> > I've already ditched the shared parameter on my RFC refactor series and
> > always having the namespaces shared.
> > 
> 
> Okay.
> 
> > > > However...
> > > > 
> > > > The spec says that "The attach and detach operations are persistent
> > > > across all reset events.". This means that we should track those events
> > > > in the subsystem and only reattach namespaces that were attached at the
> > > > time of the "reset" event. Currently, we don't have anything mapping
> > > > that state. But the device already has to take some liberties with
> > > > regard to stuff that is considered persistent by the spec (SMART log
> > > > etc.) since we do not have any way to store stuff persistently across
> > > > qemu invocations, so I think the above is an acceptable compromise.
> > > > 
> > > Careful. 'attach' and 'detach' are MI (management interface) operations.
> > > If and how many namespaces are visible to any given controllers is
> > > actually independent on that; and, in fact, controllers might not even
> > > implement 'attach' or 'detach'.
> > > But I do agree that we don't handle the 'reset' state properly.
> > > 
> > 
> > The Namespace Attachment command has nothing to do with MI? And the QEMU
> > controller 

[PATCH v6 13/20] nubus-bridge: introduce separate NubusBridge structure

[Mark Cave-Ayland]

This is to allow the Nubus bridge to store its own additional state. Also update
the comment in the file header to reflect that nubus-bridge is not specific to
the Macintosh.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bridge.c | 4 ++--
 include/hw/nubus/mac-nubus-bridge.h | 2 +-
 include/hw/nubus/nubus.h| 6 ++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c
index cd8c6a91eb..95662568c5 100644
--- a/hw/nubus/nubus-bridge.c
+++ b/hw/nubus/nubus-bridge.c
@@ -1,5 +1,5 @@
 /*
- * QEMU Macintosh Nubus
+ * QEMU Nubus
  *
  * Copyright (c) 2013-2018 Laurent Vivier 
  *
@@ -22,7 +22,7 @@ static void nubus_bridge_class_init(ObjectClass *klass, void 
*data)
 static const TypeInfo nubus_bridge_info = {
 .name  = TYPE_NUBUS_BRIDGE,
 .parent= TYPE_SYS_BUS_DEVICE,
-.instance_size = sizeof(SysBusDevice),
+.instance_size = sizeof(NubusBridge),
 .class_init= nubus_bridge_class_init,
 };
 
diff --git a/include/hw/nubus/mac-nubus-bridge.h 
b/include/hw/nubus/mac-nubus-bridge.h
index 04451d357c..fa454f5fbe 100644
--- a/include/hw/nubus/mac-nubus-bridge.h
+++ b/include/hw/nubus/mac-nubus-bridge.h
@@ -20,7 +20,7 @@
 OBJECT_DECLARE_SIMPLE_TYPE(MacNubusState, MAC_NUBUS_BRIDGE)
 
 struct MacNubusState {
-SysBusDevice sysbus_dev;
+NubusBridge parent_obj;
 
 NubusBus *bus;
 MemoryRegion super_slot_alias;
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 9f9386afed..11bcc9bb36 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -10,6 +10,7 @@
 #define HW_NUBUS_NUBUS_H
 
 #include "hw/qdev-properties.h"
+#include "hw/sysbus.h"
 #include "exec/address-spaces.h"
 #include "qom/object.h"
 #include "qemu/units.h"
@@ -32,6 +33,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(NubusDevice, NUBUS_DEVICE)
 OBJECT_DECLARE_SIMPLE_TYPE(NubusBus, NUBUS_BUS)
 
 #define TYPE_NUBUS_BRIDGE "nubus-bridge"
+OBJECT_DECLARE_SIMPLE_TYPE(NubusBridge, NUBUS_BRIDGE);
 
 struct NubusBus {
 BusState qbus;
@@ -58,4 +60,8 @@ struct NubusDevice {
 MemoryRegion decl_rom;
 };
 
+struct NubusBridge {
+SysBusDevice parent_obj;
+};
+
 #endif
-- 
2.20.1

[PATCH v6 15/20] nubus: move NubusBus from mac-nubus-bridge to nubus-bridge

[Mark Cave-Ayland]

Now that Nubus has its own address space rather than mapping directly into the
system bus, move the Nubus reference from MacNubusBridge to NubusBridge.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/m68k/q800.c  |  2 +-
 hw/nubus/mac-nubus-bridge.c | 11 +--
 hw/nubus/nubus-bridge.c |  9 +
 include/hw/nubus/mac-nubus-bridge.h |  1 -
 include/hw/nubus/nubus.h|  2 ++
 5 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index a07912b87c..9bdea1a362 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -398,7 +398,7 @@ static void q800_init(MachineState *machine)
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
 MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE);
 
-nubus = MAC_NUBUS_BRIDGE(dev)->bus;
+nubus = NUBUS_BRIDGE(dev)->bus;
 
 /* framebuffer in nubus slot #9 */
 
diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index e241c581b5..db8640eed2 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -16,22 +16,21 @@
 static void mac_nubus_bridge_init(Object *obj)
 {
 MacNubusBridge *s = MAC_NUBUS_BRIDGE(obj);
+NubusBridge *nb = NUBUS_BRIDGE(obj);
 SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL));
-
 /* Macintosh only has slots 0x9 to 0xe available */
-s->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
-  MAC_NUBUS_SLOT_NB);
+nb->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
+   MAC_NUBUS_SLOT_NB);
 
 /* Aliases for slots 0x9 to 0xe */
 memory_region_init_alias(>super_slot_alias, obj, "super-slot-alias",
- >bus->nubus_mr,
+ >bus->nubus_mr,
  MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE,
  MAC_NUBUS_SLOT_NB * NUBUS_SUPER_SLOT_SIZE);
 
 memory_region_init_alias(>slot_alias, obj, "slot-alias",
- >bus->nubus_mr,
+ >bus->nubus_mr,
  NUBUS_SLOT_BASE +
  MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE,
  MAC_NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);
diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c
index 95662568c5..3b68d4435c 100644
--- a/hw/nubus/nubus-bridge.c
+++ b/hw/nubus/nubus-bridge.c
@@ -12,6 +12,14 @@
 #include "hw/sysbus.h"
 #include "hw/nubus/nubus.h"
 
+
+static void nubus_bridge_init(Object *obj)
+{
+NubusBridge *s = NUBUS_BRIDGE(obj);
+
+s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL));
+}
+
 static void nubus_bridge_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
@@ -22,6 +30,7 @@ static void nubus_bridge_class_init(ObjectClass *klass, void 
*data)
 static const TypeInfo nubus_bridge_info = {
 .name  = TYPE_NUBUS_BRIDGE,
 .parent= TYPE_SYS_BUS_DEVICE,
+.instance_init = nubus_bridge_init,
 .instance_size = sizeof(NubusBridge),
 .class_init= nubus_bridge_class_init,
 };
diff --git a/include/hw/nubus/mac-nubus-bridge.h 
b/include/hw/nubus/mac-nubus-bridge.h
index b595e1b7ef..70ab50ab2d 100644
--- a/include/hw/nubus/mac-nubus-bridge.h
+++ b/include/hw/nubus/mac-nubus-bridge.h
@@ -22,7 +22,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(MacNubusBridge, MAC_NUBUS_BRIDGE)
 struct MacNubusBridge {
 NubusBridge parent_obj;
 
-NubusBus *bus;
 MemoryRegion super_slot_alias;
 MemoryRegion slot_alias;
 };
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 11bcc9bb36..2d00d18150 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -62,6 +62,8 @@ struct NubusDevice {
 
 struct NubusBridge {
 SysBusDevice parent_obj;
+
+NubusBus *bus;
 };
 
 #endif
-- 
2.20.1

[PATCH v6 17/20] nubus-bridge: make slot_available_mask a qdev property

[Mark Cave-Ayland]

This is to allow Macintosh machines to further specify which slots are available
since the number of addressable slots may not match the number of physical slots
present in the machine.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/nubus/nubus-bridge.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c
index 1adda7f5a6..7b51722f66 100644
--- a/hw/nubus/nubus-bridge.c
+++ b/hw/nubus/nubus-bridge.c
@@ -21,11 +21,18 @@ static void nubus_bridge_init(Object *obj)
 qbus_create_inplace(bus, sizeof(s->bus), TYPE_NUBUS_BUS, DEVICE(s), NULL);
 }
 
+static Property nubus_bridge_properties[] = {
+DEFINE_PROP_UINT16("slot-available-mask", NubusBridge,
+   bus.slot_available_mask, 0x),
+DEFINE_PROP_END_OF_LIST()
+};
+
 static void nubus_bridge_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->fw_name = "nubus";
+device_class_set_props(dc, nubus_bridge_properties);
 }
 
 static const TypeInfo nubus_bridge_info = {
-- 
2.20.1

[PATCH v6 09/20] macfb: don't register declaration ROM

[Mark Cave-Ayland]

The macfb device is an on-board framebuffer and so is initialised by the
system declaration ROM included within the MacOS toolbox ROM.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
 hw/display/macfb.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index d8183b9bbd..76808b69cc 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -383,10 +383,6 @@ static void macfb_sysbus_realize(DeviceState *dev, Error 
**errp)
 sysbus_init_mmio(SYS_BUS_DEVICE(s), >mem_vram);
 }
 
-const uint8_t macfb_rom[] = {
-255, 0, 0, 0,
-};
-
 static void macfb_nubus_realize(DeviceState *dev, Error **errp)
 {
 NubusDevice *nd = NUBUS_DEVICE(dev);
@@ -399,8 +395,6 @@ static void macfb_nubus_realize(DeviceState *dev, Error 
**errp)
 macfb_common_realize(dev, ms, errp);
 memory_region_add_subregion(>slot_mem, DAFB_BASE, >mem_ctrl);
 memory_region_add_subregion(>slot_mem, VIDEO_BASE, >mem_vram);
-
-nubus_register_rom(nd, macfb_rom, sizeof(macfb_rom), 1, 9, 0xf);
 }
 
 static void macfb_sysbus_reset(DeviceState *d)
-- 
2.20.1

[PATCH v6 14/20] mac-nubus-bridge: rename MacNubusState to MacNubusBridge

[Mark Cave-Ayland]

This better reflects that the mac-nubus-bridge device is derived from the
nubus-bridge device, and that the structure represents the state of the bridge
device and not the Nubus itself. Also update the comment in the file header to
reflect that mac-nubus-bridge is specific to the Macintosh.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/mac-nubus-bridge.c | 8 +---
 include/hw/nubus/mac-nubus-bridge.h | 4 ++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index 3af4f5d396..e241c581b5 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -1,5 +1,7 @@
 /*
- *  Copyright (c) 2013-2018 Laurent Vivier 
+ * QEMU Macintosh Nubus
+ *
+ * Copyright (c) 2013-2018 Laurent Vivier 
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -13,7 +15,7 @@
 
 static void mac_nubus_bridge_init(Object *obj)
 {
-MacNubusState *s = MAC_NUBUS_BRIDGE(obj);
+MacNubusBridge *s = MAC_NUBUS_BRIDGE(obj);
 SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
 s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL));
@@ -49,7 +51,7 @@ static const TypeInfo mac_nubus_bridge_info = {
 .name  = TYPE_MAC_NUBUS_BRIDGE,
 .parent= TYPE_NUBUS_BRIDGE,
 .instance_init = mac_nubus_bridge_init,
-.instance_size = sizeof(MacNubusState),
+.instance_size = sizeof(MacNubusBridge),
 .class_init= mac_nubus_bridge_class_init,
 };
 
diff --git a/include/hw/nubus/mac-nubus-bridge.h 
b/include/hw/nubus/mac-nubus-bridge.h
index fa454f5fbe..b595e1b7ef 100644
--- a/include/hw/nubus/mac-nubus-bridge.h
+++ b/include/hw/nubus/mac-nubus-bridge.h
@@ -17,9 +17,9 @@
 #define MAC_NUBUS_SLOT_NB(MAC_NUBUS_LAST_SLOT - MAC_NUBUS_FIRST_SLOT + 1)
 
 #define TYPE_MAC_NUBUS_BRIDGE "mac-nubus-bridge"
-OBJECT_DECLARE_SIMPLE_TYPE(MacNubusState, MAC_NUBUS_BRIDGE)
+OBJECT_DECLARE_SIMPLE_TYPE(MacNubusBridge, MAC_NUBUS_BRIDGE)
 
-struct MacNubusState {
+struct MacNubusBridge {
 NubusBridge parent_obj;
 
 NubusBus *bus;
-- 
2.20.1

[PATCH v6 10/20] nubus-device: remove nubus_register_rom() and nubus_register_format_block()

[Mark Cave-Ayland]

Since there is no need to generate a dummy declaration ROM, remove both
nubus_register_rom() and nubus_register_format_block(). These will shortly be
replaced with a mechanism to optionally load a declaration ROM from disk to
allow real images to be used within QEMU.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-device.c  | 143 ---
 include/hw/nubus/nubus.h |  19 --
 2 files changed, 162 deletions(-)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 516b13d2d5..d4932d64a2 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -13,147 +13,6 @@
 #include "qapi/error.h"
 
 
-/* The Format Block Structure */
-
-#define FBLOCK_DIRECTORY_OFFSET 0
-#define FBLOCK_LENGTH   4
-#define FBLOCK_CRC  8
-#define FBLOCK_REVISION_LEVEL   12
-#define FBLOCK_FORMAT   13
-#define FBLOCK_TEST_PATTERN 14
-#define FBLOCK_RESERVED 18
-#define FBLOCK_BYTE_LANES   19
-
-#define FBLOCK_SIZE 20
-#define FBLOCK_PATTERN_VAL  0x5a932bc7
-
-static uint64_t nubus_fblock_read(void *opaque, hwaddr addr, unsigned int size)
-{
-NubusDevice *dev = opaque;
-uint64_t val;
-
-#define BYTE(v, b) (((v) >> (24 - 8 * (b))) & 0xff)
-switch (addr) {
-case FBLOCK_BYTE_LANES:
-val = dev->byte_lanes;
-val |= (val ^ 0xf) << 4;
-break;
-case FBLOCK_RESERVED:
-val = 0x00;
-break;
-case FBLOCK_TEST_PATTERN...FBLOCK_TEST_PATTERN + 3:
-val = BYTE(FBLOCK_PATTERN_VAL, addr - FBLOCK_TEST_PATTERN);
-break;
-case FBLOCK_FORMAT:
-val = dev->rom_format;
-break;
-case FBLOCK_REVISION_LEVEL:
-val = dev->rom_rev;
-break;
-case FBLOCK_CRC...FBLOCK_CRC + 3:
-val = BYTE(dev->rom_crc, addr - FBLOCK_CRC);
-break;
-case FBLOCK_LENGTH...FBLOCK_LENGTH + 3:
-val = BYTE(dev->rom_length, addr - FBLOCK_LENGTH);
-break;
-case FBLOCK_DIRECTORY_OFFSET...FBLOCK_DIRECTORY_OFFSET + 3:
-val = BYTE(dev->directory_offset, addr - FBLOCK_DIRECTORY_OFFSET);
-break;
-default:
-val = 0;
-break;
-}
-return val;
-}
-
-static void nubus_fblock_write(void *opaque, hwaddr addr, uint64_t val,
-   unsigned int size)
-{
-/* read only */
-}
-
-static const MemoryRegionOps nubus_format_block_ops = {
-.read = nubus_fblock_read,
-.write = nubus_fblock_write,
-.endianness = DEVICE_BIG_ENDIAN,
-.valid = {
-.min_access_size = 1,
-.max_access_size = 1,
-}
-};
-
-static void nubus_register_format_block(NubusDevice *dev)
-{
-char *fblock_name;
-
-fblock_name = g_strdup_printf("nubus-slot-%d-format-block",
-  dev->slot);
-
-hwaddr fblock_offset = memory_region_size(>slot_mem) - FBLOCK_SIZE;
-memory_region_init_io(>fblock_io, NULL, _format_block_ops,
-  dev, fblock_name, FBLOCK_SIZE);
-memory_region_add_subregion(>slot_mem, fblock_offset,
->fblock_io);
-
-g_free(fblock_name);
-}
-
-static void mac_nubus_rom_write(void *opaque, hwaddr addr, uint64_t val,
-   unsigned int size)
-{
-/* read only */
-}
-
-static uint64_t mac_nubus_rom_read(void *opaque, hwaddr addr,
-unsigned int size)
-{
-NubusDevice *dev = opaque;
-
-return dev->rom[addr];
-}
-
-static const MemoryRegionOps mac_nubus_rom_ops = {
-.read  = mac_nubus_rom_read,
-.write = mac_nubus_rom_write,
-.endianness = DEVICE_BIG_ENDIAN,
-.valid = {
-.min_access_size = 1,
-.max_access_size = 1,
-},
-};
-
-
-void nubus_register_rom(NubusDevice *dev, const uint8_t *rom, uint32_t size,
-int revision, int format, uint8_t byte_lanes)
-{
-hwaddr rom_offset;
-char *rom_name;
-
-/* FIXME : really compute CRC */
-dev->rom_length = 0;
-dev->rom_crc = 0;
-
-dev->rom_rev = revision;
-dev->rom_format = format;
-
-dev->byte_lanes = byte_lanes;
-dev->directory_offset = -size;
-
-/* ROM */
-
-dev->rom = rom;
-rom_name = g_strdup_printf("nubus-slot-%d-rom", dev->slot);
-memory_region_init_io(>rom_io, NULL, _nubus_rom_ops,
-  dev, rom_name, size);
-memory_region_set_readonly(>rom_io, true);
-
-rom_offset = memory_region_size(>slot_mem) - FBLOCK_SIZE +
- dev->directory_offset;
-memory_region_add_subregion(>slot_mem, rom_offset, >rom_io);
-
-g_free(rom_name);
-}
-
 static void nubus_device_realize(DeviceState *dev, Error **errp)
 {
 NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(dev));
@@ -179,8 +38,6 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 memory_region_add_subregion(>slot_io, slot_offset,
 >slot_mem);
 

[PATCH v6 08/20] nubus: generate bus error when attempting to access empty slots

[Mark Cave-Ayland]

According to "Designing Cards and Drivers for the Macintosh Family" any attempt
to access an unimplemented address location on Nubus generates a bus error. 
MacOS
uses a custom bus error handler to detect empty Nubus slots, and with the 
current
implementation assumes that all slots are occupied as the Nubus transactions
never fail.

Switch nubus_slot_ops and nubus_super_slot_ops over to use 
{read,write}_with_attrs
and hard-code them to return MEMTX_DECODE_ERROR so that unoccupied Nubus slots
will generate the expected bus error.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bus.c | 34 ++
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index a9fb6ded9e..215fdb6b4e 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -28,23 +28,23 @@ static NubusBus *nubus_find(void)
 return NUBUS_BUS(object_resolve_path_type("", TYPE_NUBUS_BUS, NULL));
 }
 
-static void nubus_slot_write(void *opaque, hwaddr addr, uint64_t val,
- unsigned int size)
+static MemTxResult nubus_slot_write(void *opaque, hwaddr addr, uint64_t val,
+unsigned size, MemTxAttrs attrs)
 {
-/* read only */
 trace_nubus_slot_write(addr, val, size);
+return MEMTX_DECODE_ERROR;
 }
 
-static uint64_t nubus_slot_read(void *opaque, hwaddr addr,
-unsigned int size)
+static MemTxResult nubus_slot_read(void *opaque, hwaddr addr, uint64_t *data,
+   unsigned size, MemTxAttrs attrs)
 {
 trace_nubus_slot_read(addr, size);
-return 0;
+return MEMTX_DECODE_ERROR;
 }
 
 static const MemoryRegionOps nubus_slot_ops = {
-.read  = nubus_slot_read,
-.write = nubus_slot_write,
+.read_with_attrs  = nubus_slot_read,
+.write_with_attrs = nubus_slot_write,
 .endianness = DEVICE_BIG_ENDIAN,
 .valid = {
 .min_access_size = 1,
@@ -52,23 +52,25 @@ static const MemoryRegionOps nubus_slot_ops = {
 },
 };
 
-static void nubus_super_slot_write(void *opaque, hwaddr addr, uint64_t val,
-   unsigned int size)
+static MemTxResult nubus_super_slot_write(void *opaque, hwaddr addr,
+  uint64_t val, unsigned size,
+  MemTxAttrs attrs)
 {
-/* read only */
 trace_nubus_super_slot_write(addr, val, size);
+return MEMTX_DECODE_ERROR;
 }
 
-static uint64_t nubus_super_slot_read(void *opaque, hwaddr addr,
-  unsigned int size)
+static MemTxResult nubus_super_slot_read(void *opaque, hwaddr addr,
+ uint64_t *data, unsigned size,
+ MemTxAttrs attrs)
 {
 trace_nubus_super_slot_read(addr, size);
-return 0;
+return MEMTX_DECODE_ERROR;
 }
 
 static const MemoryRegionOps nubus_super_slot_ops = {
-.read  = nubus_super_slot_read,
-.write = nubus_super_slot_write,
+.read_with_attrs = nubus_super_slot_read,
+.write_with_attrs = nubus_super_slot_write,
 .endianness = DEVICE_BIG_ENDIAN,
 .valid = {
 .min_access_size = 1,
-- 
2.20.1

[PATCH v6 12/20] nubus: move nubus to its own 32-bit address space

[Mark Cave-Ayland]

According to "Designing Cards and Drivers for the Macintosh Family" the Nubus
has its own 32-bit address space based upon physical slot addressing.

Move Nubus to its own 32-bit address space and then use memory region aliases
to map available slot and super slot ranges into the q800 system address
space via the Macintosh Nubus bridge.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/m68k/q800.c  |  9 -
 hw/nubus/mac-nubus-bridge.c | 16 ++--
 hw/nubus/nubus-bus.c| 18 ++
 include/hw/nubus/mac-nubus-bridge.h |  2 ++
 include/hw/nubus/nubus.h|  6 ++
 5 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 5ba87f789c..a07912b87c 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -67,9 +67,6 @@
 #define ASC_BASE  (IO_BASE + 0x14000)
 #define SWIM_BASE (IO_BASE + 0x1E000)
 
-#define NUBUS_SUPER_SLOT_BASE 0x6000
-#define NUBUS_SLOT_BASE   0xf000
-
 #define SONIC_PROM_SIZE   0x1000
 
 /*
@@ -396,8 +393,10 @@ static void q800_init(MachineState *machine)
 
 dev = qdev_new(TYPE_MAC_NUBUS_BRIDGE);
 sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
-sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, NUBUS_SUPER_SLOT_BASE);
-sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
+MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
+MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE);
 
 nubus = MAC_NUBUS_BRIDGE(dev)->bus;
 
diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index 3f075789e9..3af4f5d396 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -22,8 +22,20 @@ static void mac_nubus_bridge_init(Object *obj)
 s->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
   MAC_NUBUS_SLOT_NB);
 
-sysbus_init_mmio(sbd, >bus->super_slot_io);
-sysbus_init_mmio(sbd, >bus->slot_io);
+/* Aliases for slots 0x9 to 0xe */
+memory_region_init_alias(>super_slot_alias, obj, "super-slot-alias",
+ >bus->nubus_mr,
+ MAC_NUBUS_FIRST_SLOT * NUBUS_SUPER_SLOT_SIZE,
+ MAC_NUBUS_SLOT_NB * NUBUS_SUPER_SLOT_SIZE);
+
+memory_region_init_alias(>slot_alias, obj, "slot-alias",
+ >bus->nubus_mr,
+ NUBUS_SLOT_BASE +
+ MAC_NUBUS_FIRST_SLOT * NUBUS_SLOT_SIZE,
+ MAC_NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);
+
+sysbus_init_mmio(sbd, >super_slot_alias);
+sysbus_init_mmio(sbd, >slot_alias);
 }
 
 static void mac_nubus_bridge_class_init(ObjectClass *klass, void *data)
diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index 215fdb6b4e..07c279bde5 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -78,25 +78,42 @@ static const MemoryRegionOps nubus_super_slot_ops = {
 },
 };
 
+static void nubus_unrealize(BusState *bus)
+{
+NubusBus *nubus = NUBUS_BUS(bus);
+
+address_space_destroy(>nubus_as);
+}
+
 static void nubus_realize(BusState *bus, Error **errp)
 {
+NubusBus *nubus = NUBUS_BUS(bus);
+
 if (!nubus_find()) {
 error_setg(errp, "at most one %s device is permitted", TYPE_NUBUS_BUS);
 return;
 }
+
+address_space_init(>nubus_as, >nubus_mr, "nubus");
 }
 
 static void nubus_init(Object *obj)
 {
 NubusBus *nubus = NUBUS_BUS(obj);
 
+memory_region_init(>nubus_mr, obj, "nubus", 0x1);
+
 memory_region_init_io(>super_slot_io, obj, _super_slot_ops,
   nubus, "nubus-super-slots",
   (NUBUS_SUPER_SLOT_NB + 1) * NUBUS_SUPER_SLOT_SIZE);
+memory_region_add_subregion(>nubus_mr, 0x0, >super_slot_io);
 
 memory_region_init_io(>slot_io, obj, _slot_ops,
   nubus, "nubus-slots",
   NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);
+memory_region_add_subregion(>nubus_mr,
+(NUBUS_SUPER_SLOT_NB + 1) *
+NUBUS_SUPER_SLOT_SIZE, >slot_io);
 
 nubus->slot_available_mask = MAKE_64BIT_MASK(NUBUS_FIRST_SLOT,
  NUBUS_SLOT_NB);
@@ -150,6 +167,7 @@ static void nubus_class_init(ObjectClass *oc, void *data)
 BusClass *bc = BUS_CLASS(oc);
 
 bc->realize = nubus_realize;
+bc->unrealize = nubus_unrealize;
 bc->check_address = nubus_check_address;
 bc->get_dev_path = nubus_get_dev_path;
 }
diff --git a/include/hw/nubus/mac-nubus-bridge.h 
b/include/hw/nubus/mac-nubus-bridge.h
index 118d67267d..04451d357c 100644
--- a/include/hw/nubus/mac-nubus-bridge.h
+++ 

[PATCH v6 03/20] nubus-device: expose separate super slot memory region

[Mark Cave-Ayland]

According to "Designing Cards and Drivers for the Macintosh Family" each 
physical
nubus slot can access 2 separate address ranges: a super slot memory region 
which
is 256MB and a standard slot memory region which is 16MB.

Currently a Nubus device uses the physical slot number to determine whether it 
is
using a standard slot memory region or a super slot memory region rather than
exposing both memory regions for use as required.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-device.c  | 36 ++--
 include/hw/nubus/nubus.h |  1 +
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index be01269563..4e23df1280 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -168,26 +168,26 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 }
 
 nd->slot = nubus->current_slot++;
-name = g_strdup_printf("nubus-slot-%d", nd->slot);
-
-if (nd->slot < NUBUS_FIRST_SLOT) {
-/* Super */
-slot_offset = (nd->slot - 6) * NUBUS_SUPER_SLOT_SIZE;
-
-memory_region_init(>slot_mem, OBJECT(dev), name,
-   NUBUS_SUPER_SLOT_SIZE);
-memory_region_add_subregion(>super_slot_io, slot_offset,
->slot_mem);
-} else {
-/* Normal */
-slot_offset = nd->slot * NUBUS_SLOT_SIZE;
-
-memory_region_init(>slot_mem, OBJECT(dev), name, NUBUS_SLOT_SIZE);
-memory_region_add_subregion(>slot_io, slot_offset,
->slot_mem);
-}
 
+/* Super */
+slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
+
+name = g_strdup_printf("nubus-super-slot-%x", nd->slot);
+memory_region_init(>super_slot_mem, OBJECT(dev), name,
+   NUBUS_SUPER_SLOT_SIZE);
+memory_region_add_subregion(>super_slot_io, slot_offset,
+>super_slot_mem);
+g_free(name);
+
+/* Normal */
+slot_offset = nd->slot * NUBUS_SLOT_SIZE;
+
+name = g_strdup_printf("nubus-slot-%x", nd->slot);
+memory_region_init(>slot_mem, OBJECT(dev), name, NUBUS_SLOT_SIZE);
+memory_region_add_subregion(>slot_io, slot_offset,
+>slot_mem);
 g_free(name);
+
 nubus_register_format_block(nd);
 }
 
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 424309dd73..89b0976aaa 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -43,6 +43,7 @@ struct NubusDevice {
 DeviceState qdev;
 
 int slot;
+MemoryRegion super_slot_mem;
 MemoryRegion slot_mem;
 
 /* Format Block */
-- 
2.20.1

[PATCH v6 02/20] nubus-device: rename slot_nb variable to slot

[Mark Cave-Ayland]

This is in preparation for creating a qdev property of the same name.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-device.c  | 14 +++---
 include/hw/nubus/nubus.h |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index ffe78a8823..be01269563 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -87,7 +87,7 @@ static void nubus_register_format_block(NubusDevice *dev)
 char *fblock_name;
 
 fblock_name = g_strdup_printf("nubus-slot-%d-format-block",
-  dev->slot_nb);
+  dev->slot);
 
 hwaddr fblock_offset = memory_region_size(>slot_mem) - FBLOCK_SIZE;
 memory_region_init_io(>fblock_io, NULL, _format_block_ops,
@@ -142,7 +142,7 @@ void nubus_register_rom(NubusDevice *dev, const uint8_t 
*rom, uint32_t size,
 /* ROM */
 
 dev->rom = rom;
-rom_name = g_strdup_printf("nubus-slot-%d-rom", dev->slot_nb);
+rom_name = g_strdup_printf("nubus-slot-%d-rom", dev->slot);
 memory_region_init_io(>rom_io, NULL, _nubus_rom_ops,
   dev, rom_name, size);
 memory_region_set_readonly(>rom_io, true);
@@ -167,12 +167,12 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 return;
 }
 
-nd->slot_nb = nubus->current_slot++;
-name = g_strdup_printf("nubus-slot-%d", nd->slot_nb);
+nd->slot = nubus->current_slot++;
+name = g_strdup_printf("nubus-slot-%d", nd->slot);
 
-if (nd->slot_nb < NUBUS_FIRST_SLOT) {
+if (nd->slot < NUBUS_FIRST_SLOT) {
 /* Super */
-slot_offset = (nd->slot_nb - 6) * NUBUS_SUPER_SLOT_SIZE;
+slot_offset = (nd->slot - 6) * NUBUS_SUPER_SLOT_SIZE;
 
 memory_region_init(>slot_mem, OBJECT(dev), name,
NUBUS_SUPER_SLOT_SIZE);
@@ -180,7 +180,7 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 >slot_mem);
 } else {
 /* Normal */
-slot_offset = nd->slot_nb * NUBUS_SLOT_SIZE;
+slot_offset = nd->slot * NUBUS_SLOT_SIZE;
 
 memory_region_init(>slot_mem, OBJECT(dev), name, NUBUS_SLOT_SIZE);
 memory_region_add_subregion(>slot_io, slot_offset,
diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index e2b5cf260b..424309dd73 100644
--- a/include/hw/nubus/nubus.h
+++ b/include/hw/nubus/nubus.h
@@ -42,7 +42,7 @@ struct NubusBus {
 struct NubusDevice {
 DeviceState qdev;
 
-int slot_nb;
+int slot;
 MemoryRegion slot_mem;
 
 /* Format Block */
-- 
2.20.1

[PATCH v6 06/20] nubus: implement BusClass get_dev_path()

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bus.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index 96ef027bad..04f11edd24 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -96,6 +96,21 @@ static void nubus_init(Object *obj)
  NUBUS_SLOT_NB);
 }
 
+static char *nubus_get_dev_path(DeviceState *dev)
+{
+NubusDevice *nd = NUBUS_DEVICE(dev);
+BusState *bus = qdev_get_parent_bus(dev);
+char *p = qdev_get_dev_path(bus->parent);
+
+if (p) {
+char *ret = g_strdup_printf("%s/%s/%02x", p, bus->name, nd->slot);
+g_free(p);
+return ret;
+} else {
+return g_strdup_printf("%s/%02x", bus->name, nd->slot);
+}
+}
+
 static bool nubus_check_address(BusState *bus, DeviceState *dev, Error **errp)
 {
 NubusDevice *nd = NUBUS_DEVICE(dev);
@@ -130,6 +145,7 @@ static void nubus_class_init(ObjectClass *oc, void *data)
 
 bc->realize = nubus_realize;
 bc->check_address = nubus_check_address;
+bc->get_dev_path = nubus_get_dev_path;
 }
 
 static const TypeInfo nubus_bus_info = {
-- 
2.20.1

[PATCH v6 07/20] nubus: add trace-events for empty slot accesses

[Mark Cave-Ayland]

Increase the max_access_size to 4 bytes for empty Nubus slot and super slot
accesses to allow tracing of the Nubus enumeration process by the guest OS.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bus.c  | 10 +++---
 hw/nubus/trace-events |  7 +++
 hw/nubus/trace.h  |  1 +
 meson.build   |  1 +
 4 files changed, 16 insertions(+), 3 deletions(-)
 create mode 100644 hw/nubus/trace-events
 create mode 100644 hw/nubus/trace.h

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index 04f11edd24..a9fb6ded9e 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -19,6 +19,7 @@
 #include "qemu/osdep.h"
 #include "hw/nubus/nubus.h"
 #include "qapi/error.h"
+#include "trace.h"
 
 
 static NubusBus *nubus_find(void)
@@ -31,12 +32,13 @@ static void nubus_slot_write(void *opaque, hwaddr addr, 
uint64_t val,
  unsigned int size)
 {
 /* read only */
+trace_nubus_slot_write(addr, val, size);
 }
 
-
 static uint64_t nubus_slot_read(void *opaque, hwaddr addr,
 unsigned int size)
 {
+trace_nubus_slot_read(addr, size);
 return 0;
 }
 
@@ -46,7 +48,7 @@ static const MemoryRegionOps nubus_slot_ops = {
 .endianness = DEVICE_BIG_ENDIAN,
 .valid = {
 .min_access_size = 1,
-.max_access_size = 1,
+.max_access_size = 4,
 },
 };
 
@@ -54,11 +56,13 @@ static void nubus_super_slot_write(void *opaque, hwaddr 
addr, uint64_t val,
unsigned int size)
 {
 /* read only */
+trace_nubus_super_slot_write(addr, val, size);
 }
 
 static uint64_t nubus_super_slot_read(void *opaque, hwaddr addr,
   unsigned int size)
 {
+trace_nubus_super_slot_read(addr, size);
 return 0;
 }
 
@@ -68,7 +72,7 @@ static const MemoryRegionOps nubus_super_slot_ops = {
 .endianness = DEVICE_BIG_ENDIAN,
 .valid = {
 .min_access_size = 1,
-.max_access_size = 1,
+.max_access_size = 4,
 },
 };
 
diff --git a/hw/nubus/trace-events b/hw/nubus/trace-events
new file mode 100644
index 00..e31833d694
--- /dev/null
+++ b/hw/nubus/trace-events
@@ -0,0 +1,7 @@
+# See docs/devel/tracing.txt for syntax documentation.
+
+# nubus-bus.c
+nubus_slot_read(uint64_t addr, int size) "reading unassigned addr 0x%"PRIx64 " 
size %d"
+nubus_slot_write(uint64_t addr, uint64_t val, int size) "writing unassigned 
addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
+nubus_super_slot_read(uint64_t addr, int size) "reading unassigned addr 
0x%"PRIx64 " size %d"
+nubus_super_slot_write(uint64_t addr, uint64_t val, int size) "writing 
unassigned addr 0x%"PRIx64 " value 0x%"PRIx64 " size %d"
diff --git a/hw/nubus/trace.h b/hw/nubus/trace.h
new file mode 100644
index 00..3749420da1
--- /dev/null
+++ b/hw/nubus/trace.h
@@ -0,0 +1 @@
+#include "trace/trace-hw_nubus.h"
diff --git a/meson.build b/meson.build
index 15ef4d3c41..7bdbbbdf02 100644
--- a/meson.build
+++ b/meson.build
@@ -2142,6 +2142,7 @@ if have_system
 'hw/misc/macio',
 'hw/net',
 'hw/net/can',
+'hw/nubus',
 'hw/nvme',
 'hw/nvram',
 'hw/pci',
-- 
2.20.1

[PATCH v6 00/20] nubus: bus, device, bridge, IRQ and address space improvements

[Mark Cave-Ayland]

This patchset is the next set of changes required to boot MacOS on the q800 
machine. The
main aim of these patches is to improve the Nubus support so that devices can 
be plugged
into the Nubus from the command line i.e.

-device nubus-macfb[,slot=num][,romfile=decl.rom]

At the moment the only device that can be plugged into the Nubus is the macfb 
framebuffer
however with these changes it is possible to take a ROM from a real Nubus card 
and
attempt to use it in QEMU, and also allow for future interfaces such as virtio.

Patches 1 to 6 move the logic which manages bus addresses from the NubusDevice 
into
the NubusBus itself, including the introduction of a bitmap to manage available
slots on the bus.

Patches 7 and 8 change the handling for unassigned (empty) slots to generate a 
bus
fault and add trace events to allow logging of empty slot accesses during Nubus
enumeration.

Patches 9 to 11 remove the existing stubs for generating the format block (the 
epilogue
of the Nubus device embedded ROM consisting of metadata and a checksum) and 
replace them
with a romfile device property to allow the entire Nubus ROM to be loaded from 
a file
into the ROM area, similar to a PCI option ROM.

Patch 12 moves the Nubus into its own separate address space whilst patches 13 
to 17
update the NubusBridge (and MacNubusBridge) devices to allow machines to map the
required slots from the Nubus address space using sysbus_mmio_map().

Finally patches 18 to 20 add support for Nubus IRQs and wire them up 
appropriately for
the q800 machine through VIA2, which is required for the next set of macfb 
updates.

Signed-off-by: Mark Cave-Ayland 


v6:
- Rebase onto master
- Add extra R-B tags from Laurent and Philippe
- Use int for ctz32() result in patches 4 and 5
- Change slot_available_mask to uint16_t in patches 4 and 17
- Fix typo in commit message for patch 4

v5:
- Rebase onto master
- Add R-B tags from Laurent
- Introduce NUBUS_FIRST_SLOT/NUBUS_LAST_SLOT and 
MAC_NUBUS_FIRST_SLOT/MAC_NUBUS_LAST_SLOT
  and fix up NUBUS_SUPER_SLOT_NB/NUBUS_SLOT_NB in patch 4
- Fix super slot offset calculation in patch 4
- Squash original patch 3 ("nubus-device: add device slot parameter") into 
patch 4
  ("nubus: use bitmap to manage available slots")
- Add new patch 1 ("nubus: add comment indicating reference documents") 
containing
  documentation references
- Drop "nubus->slot_available_mask = MAKE_64BIT_MASK(0, 16);" from nubus_init() 
in patch 17
  
v4:
- Rebase onto master
- Pass _abort to memory_region_init_rom() in patch 11
- Change warn_error() to error_setg() and tweak message in patch 11

v3:
- Rebase onto master
- Add Phil's R-B for patch 7
- Move NUBUS_FIRST_SLOT/NUBUS_LAST_SLOT check to end of nubus_device_realize() 
in patch 4
- Use BIT() macro in patches 4 and 20

v2:
- Rebase onto master
- Tweak the cover letter by adding the optional slot parameter in the -device 
example
- Add R-B tags from Phil
- Document the increase in max_access_size in patch 7
- Change the maximum declaration ROM size to 128KiB using (128 * KiB) in patch 
11
- use MAKE_64BIT_MASK() in patches 4 and 16


Mark Cave-Ayland (20):
  nubus: add comment indicating reference documents
  nubus-device: rename slot_nb variable to slot
  nubus-device: expose separate super slot memory region
  nubus: use bitmap to manage available slots
  nubus: move slot bitmap checks from NubusDevice realize() to BusClass
check_address()
  nubus: implement BusClass get_dev_path()
  nubus: add trace-events for empty slot accesses
  nubus: generate bus error when attempting to access empty slots
  macfb: don't register declaration ROM
  nubus-device: remove nubus_register_rom() and
nubus_register_format_block()
  nubus-device: add romfile property for loading declaration ROMs
  nubus: move nubus to its own 32-bit address space
  nubus-bridge: introduce separate NubusBridge structure
  mac-nubus-bridge: rename MacNubusState to MacNubusBridge
  nubus: move NubusBus from mac-nubus-bridge to nubus-bridge
  nubus-bridge: embed the NubusBus object directly within nubus-bridge
  nubus-bridge: make slot_available_mask a qdev property
  nubus: add support for slot IRQs
  q800: wire up nubus IRQs
  q800: configure nubus available slots for Quadra 800

 hw/display/macfb.c  |   6 -
 hw/m68k/q800.c  |  26 +++-
 hw/nubus/mac-nubus-bridge.c |  34 -
 hw/nubus/nubus-bridge.c |  23 ++-
 hw/nubus/nubus-bus.c| 120 ---
 hw/nubus/nubus-device.c | 227 
 hw/nubus/trace-events   |   7 +
 hw/nubus/trace.h|   1 +
 include/hw/nubus/mac-nubus-bridge.h |  13 +-
 include/hw/nubus/nubus.h|  49 +++---
 meson.build |   1 +
 11 files changed, 278 insertions(+), 229 deletions(-)
 create mode 100644 hw/nubus/trace-events
 create mode 100644 hw/nubus/trace.h

-- 
2.20.1

[PATCH v6 05/20] nubus: move slot bitmap checks from NubusDevice realize() to BusClass check_address()

[Mark Cave-Ayland]

Allow Nubus to manage the slot allocations itself using the BusClass 
check_address()
virtual function rather than managing this during NubusDevice realize().

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bus.c| 29 +
 hw/nubus/nubus-device.c | 21 -
 2 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index 3cd7534864..96ef027bad 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -96,11 +96,40 @@ static void nubus_init(Object *obj)
  NUBUS_SLOT_NB);
 }
 
+static bool nubus_check_address(BusState *bus, DeviceState *dev, Error **errp)
+{
+NubusDevice *nd = NUBUS_DEVICE(dev);
+NubusBus *nubus = NUBUS_BUS(bus);
+
+if (nd->slot == -1) {
+/* No slot specified, find first available free slot */
+int s = ctz32(nubus->slot_available_mask);
+if (s != 32) {
+nd->slot = s;
+} else {
+error_setg(errp, "Cannot register nubus card, no free slot "
+ "available");
+return false;
+}
+} else {
+/* Slot specified, make sure the slot is available */
+if (!(nubus->slot_available_mask & BIT(nd->slot))) {
+error_setg(errp, "Cannot register nubus card, slot %d is "
+ "unavailable or already occupied", nd->slot);
+return false;
+}
+}
+
+nubus->slot_available_mask &= ~BIT(nd->slot);
+return true;
+}
+
 static void nubus_class_init(ObjectClass *oc, void *data)
 {
 BusClass *bc = BUS_CLASS(oc);
 
 bc->realize = nubus_realize;
+bc->check_address = nubus_check_address;
 }
 
 static const TypeInfo nubus_bus_info = {
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 2e96d6b4fc..516b13d2d5 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -161,27 +161,6 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 char *name;
 hwaddr slot_offset;
 
-if (nd->slot == -1) {
-/* No slot specified, find first available free slot */
-int s = ctz32(nubus->slot_available_mask);
-if (s != 32) {
-nd->slot = s;
-} else {
-error_setg(errp, "Cannot register nubus card, no free slot "
- "available");
-return;
-}
-} else {
-/* Slot specified, make sure the slot is available */
-if (!(nubus->slot_available_mask & BIT(nd->slot))) {
-error_setg(errp, "Cannot register nubus card, slot %d is "
- "unavailable or already occupied", nd->slot);
-return;
-}
-}
-
-nubus->slot_available_mask &= ~BIT(nd->slot);
-
 /* Super */
 slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
 
-- 
2.20.1

[PATCH v6 01/20] nubus: add comment indicating reference documents

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
---
 hw/nubus/nubus-bus.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index 5c13452308..f4410803ff 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -8,6 +8,14 @@
  *
  */
 
+/*
+ * References:
+ *   Nubus Specification (TI)
+ * http://www.bitsavers.org/pdf/ti/nubus/2242825-0001_NuBus_Spec1983.pdf
+ *
+ *   Designing Cards and Drivers for the Macintosh Family (Apple)
+ */
+
 #include "qemu/osdep.h"
 #include "hw/nubus/nubus.h"
 #include "qapi/error.h"
-- 
2.20.1

[PATCH 0/2] hw/nvme: fix namespace attachment on controller hotplug

[Klaus Jensen]

From: Klaus Jensen 

First patch from Hannes fixes the subsystem registration process such
that shared (but non-detached) namespaces are automatically attached to
hotplugged controllers.

The second patch changes the default for 'shared' such that namespaces
are shared by default and will thus by default be attached to hotplugged
controllers. This adds a compat property for older machine versions and
updates the documentation to reflect this.

Hannes Reinecke (1):
  hw/nvme: reattach subsystem namespaces on hotplug

Klaus Jensen (1):
  hw/nvme: change nvme-ns 'shared' default

 docs/system/devices/nvme.rst | 24 ++--
 hw/core/machine.c|  4 +++-
 hw/nvme/ns.c |  8 +---
 hw/nvme/subsys.c | 10 +-
 4 files changed, 27 insertions(+), 19 deletions(-)

-- 
2.33.0

[PATCH 1/2] hw/nvme: reattach subsystem namespaces on hotplug

[Klaus Jensen]

From: Hannes Reinecke 

With commit 5ffbaeed16 ("hw/nvme: fix controller hot unplugging")
namespaces get moved from the controller to the subsystem if one
is specified.
That keeps the namespaces alive after a controller hot-unplug, but
after a controller hotplug we have to reconnect the namespaces
from the subsystem to the controller.

Fixes: 5ffbaeed16 ("hw/nvme: fix controller hot unplugging")
Cc: Klaus Jensen 
Signed-off-by: Hannes Reinecke 
[k.jensen: only attach to shared and non-detached namespaces]
Signed-off-by: Klaus Jensen 
---
 hw/nvme/subsys.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c
index 93c35950d69d..6b2e4c975f5b 100644
--- a/hw/nvme/subsys.c
+++ b/hw/nvme/subsys.c
@@ -14,7 +14,7 @@
 int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
 {
 NvmeSubsystem *subsys = n->subsys;
-int cntlid;
+int cntlid, nsid;
 
 for (cntlid = 0; cntlid < ARRAY_SIZE(subsys->ctrls); cntlid++) {
 if (!subsys->ctrls[cntlid]) {
@@ -29,12 +29,20 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
 
 subsys->ctrls[cntlid] = n;
 
+for (nsid = 1; nsid < ARRAY_SIZE(subsys->namespaces); nsid++) {
+NvmeNamespace *ns = subsys->namespaces[nsid];
+if (ns && ns->params.shared && !ns->params.detached) {
+nvme_attach_ns(n, ns);
+}
+}
+
 return cntlid;
 }
 
 void nvme_subsys_unregister_ctrl(NvmeSubsystem *subsys, NvmeCtrl *n)
 {
 subsys->ctrls[n->cntlid] = NULL;
+n->cntlid = -1;
 }
 
 static void nvme_subsys_setup(NvmeSubsystem *subsys)
-- 
2.33.0

[PATCH 2/2] hw/nvme: change nvme-ns 'shared' default

[Klaus Jensen]

From: Klaus Jensen 

Change namespaces to be shared namespaces by default (parameter
shared=on). Keep shared=off for older machine types.

Signed-off-by: Klaus Jensen 
---
 docs/system/devices/nvme.rst | 24 ++--
 hw/core/machine.c|  4 +++-
 hw/nvme/ns.c |  8 +---
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/docs/system/devices/nvme.rst b/docs/system/devices/nvme.rst
index bff72d1c24d0..a1c0db01f6d5 100644
--- a/docs/system/devices/nvme.rst
+++ b/docs/system/devices/nvme.rst
@@ -110,28 +110,32 @@ multipath I/O.
 This will create an NVM subsystem with two controllers. Having controllers
 linked to an ``nvme-subsys`` device allows additional ``nvme-ns`` parameters:
 
-``shared`` (default: ``off``)
+``shared`` (default: ``on`` since 6.2)
   Specifies that the namespace will be attached to all controllers in the
-  subsystem. If set to ``off`` (the default), the namespace will remain a
-  private namespace and may only be attached to a single controller at a time.
+  subsystem. If set to ``off``, the namespace will remain a private namespace
+  and may only be attached to a single controller at a time. Shared namespaces
+  are always automatically attached to all controllers (also when controllers
+  are hotplugged).
 
 ``detached`` (default: ``off``)
   If set to ``on``, the namespace will be be available in the subsystem, but
-  not attached to any controllers initially.
+  not attached to any controllers initially. A shared namespace with this set
+  to ``on`` will never be automatically attached to controllers.
 
 Thus, adding
 
 .. code-block:: console
 
-drive file=nvm-1.img,if=none,id=nvm-1
-   -device nvme-ns,drive=nvm-1,nsid=1,shared=on
+   -device nvme-ns,drive=nvm-1,nsid=1
-drive file=nvm-2.img,if=none,id=nvm-2
-   -device nvme-ns,drive=nvm-2,nsid=3,detached=on
+   -device nvme-ns,drive=nvm-2,nsid=3,shared=off,detached=on
 
-will cause NSID 1 will be a shared namespace (due to ``shared=on``) that is
-initially attached to both controllers. NSID 3 will be a private namespace
-(i.e. only attachable to a single controller at a time) and will not be
-attached to any controller initially (due to ``detached=on``).
+will cause NSID 1 will be a shared namespace that is initially attached to both
+controllers. NSID 3 will be a private namespace due to ``shared=off`` and only
+attachable to a single controller at a time. Additionally it will not be
+attached to any controller initially (due to ``detached=on``) or to hotplugged
+controllers.
 
 Optional Features
 =
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 067f42b528fd..5e2fa3e392b9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -37,7 +37,9 @@
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-pci.h"
 
-GlobalProperty hw_compat_6_1[] = {};
+GlobalProperty hw_compat_6_1[] = {
+{ "nvme-ns", "shared", "off" },
+};
 const size_t hw_compat_6_1_len = G_N_ELEMENTS(hw_compat_6_1);
 
 GlobalProperty hw_compat_6_0[] = {
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index b7cf1494e75b..8b5f98c76180 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -465,12 +465,6 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
"linked to an nvme-subsys device");
 return;
 }
-
-if (ns->params.shared) {
-error_setg(errp, "shared requires that the nvme device is "
-   "linked to an nvme-subsys device");
-return;
-}
 } else {
 /*
  * If this namespace belongs to a subsystem (through a link on the
@@ -532,7 +526,7 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
 static Property nvme_ns_props[] = {
 DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf),
 DEFINE_PROP_BOOL("detached", NvmeNamespace, params.detached, false),
-DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false),
+DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, true),
 DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
 DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
 DEFINE_PROP_UINT64("eui64", NvmeNamespace, params.eui64, 0),
-- 
2.33.0

Re: [PATCH v5 04/20] nubus: use bitmap to manage available slots

[Mark Cave-Ayland]

On 23/09/2021 15:16, BALATON Zoltan wrote:


On Thu, 23 Sep 2021, Mark Cave-Ayland wrote:

Convert nubus_device_realize() to use a bitmap to manage available slots to 
allow
for future Nubus devices to be plugged into arbitrary slots from the command 
line
using a new qdev "slot" parameter for nubus devices.

Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh
machines as documented in "Desigining Cards and Drivers for the Macintosh 
Family".


Small typo: "a Macintosh machnies", either a or s is not needed.


Thanks - I've updated this for v6.


Signed-off-by: Mark Cave-Ayland 
---
hw/nubus/mac-nubus-bridge.c |  4 
hw/nubus/nubus-bus.c    |  5 +++--
hw/nubus/nubus-device.c | 32 +++--
include/hw/nubus/mac-nubus-bridge.h |  4 
include/hw/nubus/nubus.h    | 13 ++--
5 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
index 7c329300b8..3f075789e9 100644
--- a/hw/nubus/mac-nubus-bridge.c
+++ b/hw/nubus/mac-nubus-bridge.c
@@ -18,6 +18,10 @@ static void mac_nubus_bridge_init(Object *obj)

    s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL));

+    /* Macintosh only has slots 0x9 to 0xe available */
+    s->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT,
+  MAC_NUBUS_SLOT_NB);
+
    sysbus_init_mmio(sbd, >bus->super_slot_io);
    sysbus_init_mmio(sbd, >bus->slot_io);
}
diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index f4410803ff..3cd7534864 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -86,13 +86,14 @@ static void nubus_init(Object *obj)

    memory_region_init_io(>super_slot_io, obj, _super_slot_ops,
  nubus, "nubus-super-slots",
-  NUBUS_SUPER_SLOT_NB * NUBUS_SUPER_SLOT_SIZE);
+  (NUBUS_SUPER_SLOT_NB + 1) * NUBUS_SUPER_SLOT_SIZE);

    memory_region_init_io(>slot_io, obj, _slot_ops,
  nubus, "nubus-slots",
  NUBUS_SLOT_NB * NUBUS_SLOT_SIZE);

-    nubus->current_slot = NUBUS_FIRST_SLOT;
+    nubus->slot_available_mask = MAKE_64BIT_MASK(NUBUS_FIRST_SLOT,
+ NUBUS_SLOT_NB);
}

static void nubus_class_init(ObjectClass *oc, void *data)
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 4e23df1280..562650a05b 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -160,14 +160,28 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
    NubusDevice *nd = NUBUS_DEVICE(dev);
    char *name;
    hwaddr slot_offset;
-
-    if (nubus->current_slot < NUBUS_FIRST_SLOT ||
-    nubus->current_slot > NUBUS_LAST_SLOT) {
-    error_setg(errp, "Cannot register nubus card, not enough slots");
-    return;
+    uint16_t s;
+
+    if (nd->slot == -1) {
+    /* No slot specified, find first available free slot */
+    s = ctz32(nubus->slot_available_mask);
+    if (s != 32) {
+    nd->slot = s;
+    } else {
+    error_setg(errp, "Cannot register nubus card, no free slot "
+ "available");
+    return;
+    }
+    } else {
+    /* Slot specified, make sure the slot is available */
+    if (!(nubus->slot_available_mask & BIT(nd->slot))) {
+    error_setg(errp, "Cannot register nubus card, slot %d is "
+ "unavailable or already occupied", nd->slot);
+    return;
+    }
    }

-    nd->slot = nubus->current_slot++;
+    nubus->slot_available_mask &= ~BIT(nd->slot);

    /* Super */
    slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE;
@@ -191,12 +205,18 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
    nubus_register_format_block(nd);
}

+static Property nubus_device_properties[] = {
+    DEFINE_PROP_INT32("slot", NubusDevice, slot, -1),
+    DEFINE_PROP_END_OF_LIST()
+};
+
static void nubus_device_class_init(ObjectClass *oc, void *data)
{
    DeviceClass *dc = DEVICE_CLASS(oc);

    dc->realize = nubus_device_realize;
    dc->bus_type = TYPE_NUBUS_BUS;
+    device_class_set_props(dc, nubus_device_properties);
}

static const TypeInfo nubus_device_type_info = {
diff --git a/include/hw/nubus/mac-nubus-bridge.h 
b/include/hw/nubus/mac-nubus-bridge.h
index 36aa098dd4..118d67267d 100644
--- a/include/hw/nubus/mac-nubus-bridge.h
+++ b/include/hw/nubus/mac-nubus-bridge.h
@@ -12,6 +12,10 @@
#include "hw/nubus/nubus.h"
#include "qom/object.h"

+#define MAC_NUBUS_FIRST_SLOT 0x9
+#define MAC_NUBUS_LAST_SLOT  0xe
+#define MAC_NUBUS_SLOT_NB    (MAC_NUBUS_LAST_SLOT - MAC_NUBUS_FIRST_SLOT + 1)
+
#define TYPE_MAC_NUBUS_BRIDGE "mac-nubus-bridge"
OBJECT_DECLARE_SIMPLE_TYPE(MacNubusState, MAC_NUBUS_BRIDGE)

diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h
index 89b0976aaa..988e4a2361 

Re: [PATCH v5 17/20] nubus-bridge: make slot_available_mask a qdev property

[Mark Cave-Ayland]

On 23/09/2021 10:52, Philippe Mathieu-Daudé wrote:


On 9/23/21 11:13, Mark Cave-Ayland wrote:

This is to allow Macintosh machines to further specify which slots are available
since the number of addressable slots may not match the number of physical slots
present in the machine.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
  hw/nubus/nubus-bridge.c | 7 +++
  1 file changed, 7 insertions(+)



+static Property nubus_bridge_properties[] = {
+    DEFINE_PROP_UINT32("slot-available-mask", NubusBridge,
+   bus.slot_available_mask, 0x),


What about using DEFINE_PROP_UINT16() here and uint16_t in
patch 04/20 "nubus: use bitmap to manage available slots"?

Regardless:
Reviewed-by: Philippe Mathieu-Daudé 


Yes, that seems to work fine so I'll make this change in v6.


+    DEFINE_PROP_END_OF_LIST()
+};



ATB,

Mark.

[PULL 2/3] hw/nvme: fix verification of select field in namespace attachment

[Klaus Jensen]

From: Naveen Nagar 

Fix is added to check for reserved value in select field for
namespace attachment

CC: Minwoo Im 
Signed-off-by: Naveen Nagar 
Signed-off-by: Klaus Jensen 
---
 hw/nvme/ctrl.c   | 15 ---
 include/block/nvme.h |  5 +
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index ff784851137e..dc0e7b00308e 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5191,7 +5191,7 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, 
NvmeRequest *req)
 uint16_t list[NVME_CONTROLLER_LIST_SIZE] = {};
 uint32_t nsid = le32_to_cpu(req->cmd.nsid);
 uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
-bool attach = !(dw10 & 0xf);
+uint8_t sel = dw10 & 0xf;
 uint16_t *nr_ids = [0];
 uint16_t *ids = [1];
 uint16_t ret;
@@ -5224,7 +5224,8 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, 
NvmeRequest *req)
 return NVME_NS_CTRL_LIST_INVALID | NVME_DNR;
 }
 
-if (attach) {
+switch (sel) {
+case NVME_NS_ATTACHMENT_ATTACH:
 if (nvme_ns(ctrl, nsid)) {
 return NVME_NS_ALREADY_ATTACHED | NVME_DNR;
 }
@@ -5235,7 +5236,10 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, 
NvmeRequest *req)
 
 nvme_attach_ns(ctrl, ns);
 nvme_select_iocs_ns(ctrl, ns);
-} else {
+
+break;
+
+case NVME_NS_ATTACHMENT_DETACH:
 if (!nvme_ns(ctrl, nsid)) {
 return NVME_NS_NOT_ATTACHED | NVME_DNR;
 }
@@ -5244,6 +5248,11 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, 
NvmeRequest *req)
 ns->attached--;
 
 nvme_update_dmrsl(ctrl);
+
+break;
+
+default:
+return NVME_INVALID_FIELD | NVME_DNR;
 }
 
 /*
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 77aae0117494..e3bd47bf76ab 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -1154,6 +1154,11 @@ enum NvmeIdCtrlCmic {
 NVME_CMIC_MULTI_CTRL= 1 << 1,
 };
 
+enum NvmeNsAttachmentOperation {
+NVME_NS_ATTACHMENT_ATTACH = 0x0,
+NVME_NS_ATTACHMENT_DETACH = 0x1,
+};
+
 #define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)
 #define NVME_CTRL_SQES_MAX(sqes) (((sqes) >> 4) & 0xf)
 #define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
-- 
2.33.0

Re: [PATCH v5 18/20] nubus: add support for slot IRQs

[Mark Cave-Ayland]

On 23/09/2021 10:49, Philippe Mathieu-Daudé wrote:


On 9/23/21 11:13, Mark Cave-Ayland wrote:

Each Nubus slot has an IRQ line that can be used to request service from the
CPU. Connect the IRQs to the Nubus bridge so that they can be wired up using 
qdev
gpios accordingly, and introduce a new nubus_set_irq() function that can be used
by Nubus devices to control the slot IRQ.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
  hw/nubus/nubus-bridge.c  | 2 ++
  hw/nubus/nubus-device.c  | 8 
  include/hw/nubus/nubus.h | 6 ++
  3 files changed, 16 insertions(+)



  static Property nubus_bridge_properties[] = {
diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 280f40e88a..0f1852f671 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -10,12 +10,20 @@
  #include "qemu/osdep.h"
  #include "qemu/datadir.h"
+#include "hw/irq.h"
  #include "hw/loader.h"
  #include "hw/nubus/nubus.h"
  #include "qapi/error.h"
  #include "qemu/error-report.h"
+void nubus_set_irq(NubusDevice *nd, int level)
+{
+    NubusBus *nubus = NUBUS_BUS(qdev_get_parent_bus(DEVICE(nd)));
+


A trace-event could be helpful here, otherwise:

Reviewed-by: Philippe Mathieu-Daudé 


+    qemu_set_irq(nubus->irqs[nd->slot], level);
+}


I think adding a trace event here would just be too verbose (particularly if you have 
more than one nubus device) and not particularly helpful: normally the part you want 
to debug is the in the device itself looking at which constituent flags combine to 
raise/lower the interrupt line. And once you've done that then you've already got a 
suitable trace-event in place...



ATB,

Mark.

Re: [PATCH v5 05/20] nubus: move slot bitmap checks from NubusDevice realize() to BusClass check_address()

[Mark Cave-Ayland]

On 23/09/2021 10:45, Philippe Mathieu-Daudé wrote:


On 9/23/21 11:12, Mark Cave-Ayland wrote:

Allow Nubus to manage the slot allocations itself using the BusClass 
check_address()
virtual function rather than managing this during NubusDevice realize().

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Laurent Vivier 
---
  hw/nubus/nubus-bus.c    | 30 ++
  hw/nubus/nubus-device.c | 22 --
  2 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
index 3cd7534864..d4daaa36f2 100644
--- a/hw/nubus/nubus-bus.c
+++ b/hw/nubus/nubus-bus.c
@@ -96,11 +96,41 @@ static void nubus_init(Object *obj)
   NUBUS_SLOT_NB);
  }
+static bool nubus_check_address(BusState *bus, DeviceState *dev, Error **errp)
+{
+    NubusDevice *nd = NUBUS_DEVICE(dev);
+    NubusBus *nubus = NUBUS_BUS(bus);
+    uint16_t s;
+
+    if (nd->slot == -1) {
+    /* No slot specified, find first available free slot */
+    s = ctz32(nubus->slot_available_mask);


Same comment than previous patch:

    int s = ctz32(nubus->slot_available_mask);


... and same here.


Otherwise:
Reviewed-by: Philippe Mathieu-Daudé 


+    if (s != 32) {
+    nd->slot = s;
+    } else {
+    error_setg(errp, "Cannot register nubus card, no free slot "
+ "available");
+    return false;
+    }
+    } else {
+    /* Slot specified, make sure the slot is available */
+    if (!(nubus->slot_available_mask & BIT(nd->slot))) {
+    error_setg(errp, "Cannot register nubus card, slot %d is "
+ "unavailable or already occupied", nd->slot);
+    return false;
+    }
+    }
+
+    nubus->slot_available_mask &= ~BIT(nd->slot);
+    return true;
+}



ATB,

Mark.

Re: [PATCH v5 04/20] nubus: use bitmap to manage available slots

[Mark Cave-Ayland]

On 23/09/2021 10:42, Philippe Mathieu-Daudé wrote:

On 9/23/21 11:12, Mark Cave-Ayland wrote:

Convert nubus_device_realize() to use a bitmap to manage available slots to 
allow
for future Nubus devices to be plugged into arbitrary slots from the command 
line
using a new qdev "slot" parameter for nubus devices.

Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh
machines as documented in "Desigining Cards and Drivers for the Macintosh 
Family".

Signed-off-by: Mark Cave-Ayland 
---
  hw/nubus/mac-nubus-bridge.c |  4 
  hw/nubus/nubus-bus.c    |  5 +++--
  hw/nubus/nubus-device.c | 32 +++--
  include/hw/nubus/mac-nubus-bridge.h |  4 
  include/hw/nubus/nubus.h    | 13 ++--
  5 files changed, 43 insertions(+), 15 deletions(-)



diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 4e23df1280..562650a05b 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -160,14 +160,28 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
  NubusDevice *nd = NUBUS_DEVICE(dev);
  char *name;
  hwaddr slot_offset;
-
-    if (nubus->current_slot < NUBUS_FIRST_SLOT ||
-    nubus->current_slot > NUBUS_LAST_SLOT) {
-    error_setg(errp, "Cannot register nubus card, not enough slots");
-    return;
+    uint16_t s;
+
+    if (nd->slot == -1) {
+    /* No slot specified, find first available free slot */
+    s = ctz32(nubus->slot_available_mask);


Nitpicking:

    int s = ctz32(nubus->slot_available_mask);

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé 


I'll make sure this is included in v6.


+    if (s != 32) {
+    nd->slot = s;
+    } else {
+    error_setg(errp, "Cannot register nubus card, no free slot "
+ "available");
+    return;
+    }
+    } else {
+    /* Slot specified, make sure the slot is available */
+    if (!(nubus->slot_available_mask & BIT(nd->slot))) {
+    error_setg(errp, "Cannot register nubus card, slot %d is "
+ "unavailable or already occupied", nd->slot);
+    return;
+    }
  }



ATB,

Mark.

[PULL 3/3] hw/nvme: Return error for fused operations

[Klaus Jensen]

From: Pankaj Raghav 

Currently, FUSED operations are not supported by QEMU. As per the 1.4 SPEC,
controller should abort the command that requested a fused operation with
an INVALID FIELD error code if they are not supported.

Changes from v1:
Added FUSE flag check also to the admin cmd processing as the FUSED
operations are mentioned in the general SQE section in the SPEC.

Signed-off-by: Pankaj Raghav 
Reviewed-by: Keith Busch 
Signed-off-by: Klaus Jensen 
---
 hw/nvme/ctrl.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index dc0e7b00308e..2f247a9275ca 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -3893,6 +3893,10 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest 
*req)
 return ns->status;
 }
 
+if (NVME_CMD_FLAGS_FUSE(req->cmd.flags)) {
+return NVME_INVALID_FIELD;
+}
+
 req->ns = ns;
 
 switch (req->cmd.opcode) {
@@ -5475,6 +5479,10 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest 
*req)
 return NVME_INVALID_FIELD | NVME_DNR;
 }
 
+if (NVME_CMD_FLAGS_FUSE(req->cmd.flags)) {
+return NVME_INVALID_FIELD;
+}
+
 switch (req->cmd.opcode) {
 case NVME_ADM_CMD_DELETE_SQ:
 return nvme_del_sq(n, req);
-- 
2.33.0

Re: [PATCH] block: introduce max_hw_iov for use in scsi-generic

[Christian Borntraeger]

Peter, Michael,

do we still do stable releases for QEMU or has this stopped?

Am 24.09.21 um 07:27 schrieb Paolo Bonzini:

Yes, the question is whether it still exists... Paolo El jue., 23 sept. 2021 16:48, 
Christian Borntraeger  escribió: Am 23.09.21 um 15:04 
schrieb Paolo Bonzini: > Linux limits the size of iovecs to 1024 (UIO_MAXIOV 
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
ZjQcmQRYFpfptBannerEnd
Yes, the question is whether it still exists...

Paolo

El jue., 23 sept. 2021 16:48, Christian Borntraeger mailto:borntrae...@de.ibm.com>> escribió:



Am 23.09.21 um 15:04 schrieb Paolo Bonzini:
 > Linux limits the size of iovecs to 1024 (UIO_MAXIOV in the kernel
 > sources, IOV_MAX in POSIX).  Because of this, on some host adapters
 > requests with many iovecs are rejected with -EINVAL by the
 > io_submit() or readv()/writev() system calls.
 >
 > In fact, the same limit applies to SG_IO as well.  To fix both the
 > EINVAL and the possible performance issues from using fewer iovecs
 > than allowed by Linux (some HBAs have max_segments as low as 128),
 > introduce a separate entry in BlockLimits to hold the max_segments
 > value from sysfs.  This new limit is used only for SG_IO and clamped
 > to bs->bl.max_iov anyway, just like max_hw_transfer is clamped to
 > bs->bl.max_transfer.
 >
 > Reported-by: Halil Pasic mailto:pa...@linux.ibm.com>>
 > Cc: Hanna Reitz mailto:hre...@redhat.com>>
 > Cc: Kevin Wolf mailto:kw...@redhat.com>>
 > Cc: qemu-bl...@nongnu.org 
 > Fixes: 18473467d5 ("file-posix: try BLKSECTGET on block devices too, do not 
round to power of 2", 2021-06-25)

This sneaked in shortly before the 6.1 release (between rc0 and rc1 I 
think).
Shouldnt that go to stable in cass this still exist?


 > Signed-off-by: Paolo Bonzini mailto:pbonz...@redhat.com>>
 > ---
 >   block/block-backend.c          | 6 ++
 >   block/file-posix.c             | 2 +-
 >   block/io.c                     | 1 +
 >   hw/scsi/scsi-generic.c         | 2 +-
 >   include/block/block_int.h      | 7 +++
 >   include/sysemu/block-backend.h | 1 +
 >   6 files changed, 17 insertions(+), 2 deletions(-)
 >
 > diff --git a/block/block-backend.c b/block/block-backend.c
 > index 6140d133e2..ba2b5ebb10 100644
 > --- a/block/block-backend.c
 > +++ b/block/block-backend.c
 > @@ -1986,6 +1986,12 @@ uint32_t blk_get_max_transfer(BlockBackend *blk)
 >       return ROUND_DOWN(max, blk_get_request_alignment(blk));
 >   }
 >
 > +int blk_get_max_hw_iov(BlockBackend *blk)
 > +{
 > +    return MIN_NON_ZERO(blk->root->bs->bl.max_hw_iov,
 > +                        blk->root->bs->bl.max_iov);
 > +}
 > +
 >   int blk_get_max_iov(BlockBackend *blk)
 >   {
 >       return blk->root->bs->bl.max_iov;
 > diff --git a/block/file-posix.c b/block/file-posix.c
 > index cb9bffe047..1567edb3d5 100644
 > --- a/block/file-posix.c
 > +++ b/block/file-posix.c
 > @@ -1273,7 +1273,7 @@ static void raw_refresh_limits(BlockDriverState 
*bs, Error **errp)
 >
 >           ret = hdev_get_max_segments(s->fd, );
 >           if (ret > 0) {
 > -            bs->bl.max_iov = ret;
 > +            bs->bl.max_hw_iov = ret;
 >           }
 >       }
 >   }
 > diff --git a/block/io.c b/block/io.c
 > index a19942718b..f38e7f81d8 100644
 > --- a/block/io.c
 > +++ b/block/io.c
 > @@ -136,6 +136,7 @@ static void bdrv_merge_limits(BlockLimits *dst, 
const BlockLimits *src)
 >       dst->min_mem_alignment = MAX(dst->min_mem_alignment,
 >                                    src->min_mem_alignment);
 >       dst->max_iov = MIN_NON_ZERO(dst->max_iov, src->max_iov);
 > +    dst->max_hw_iov = MIN_NON_ZERO(dst->max_hw_iov, src->max_hw_iov);
 >   }
 >
 >   typedef struct BdrvRefreshLimitsState {
 > diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
 > index 665baf900e..0306ccc7b1 100644
 > --- a/hw/scsi/scsi-generic.c
 > +++ b/hw/scsi/scsi-generic.c
 > @@ -180,7 +180,7 @@ static int scsi_handle_inquiry_reply(SCSIGenericReq 
*r, SCSIDevice *s, int len)
 >           page = r->req.cmd.buf[2];
 >           if (page == 0xb0) {
 >               uint64_t max_transfer = 
blk_get_max_hw_transfer(s->conf.blk);
 > -            uint32_t max_iov = blk_get_max_iov(s->conf.blk);
 > +            uint32_t max_iov = blk_get_max_hw_iov(s->conf.blk);
 >
 >               assert(max_transfer);
 >               max_transfer = MIN_NON_ZERO(max_transfer, max_iov * 
qemu_real_host_page_size)
 > diff --git a/include/block/block_int.h b/include/block/block_int.h
 > index f1a54db0f8..c31cbd034a 100644
 > --- a/include/block/block_int.h
 > +++ 

Re: [PATCH v3] target/riscv: Set mstatus_hs.[SD|FS] bits if Clean and V=1 in mark_fs_dirty()

[Alistair Francis]

On Tue, Sep 21, 2021 at 12:03 PM  wrote:
>
> From: Frank Chang 
>
> When V=1, both vsstauts.FS and HS-level sstatus.FS are in effect.
> Modifying the floating-point state when V=1 causes both fields to
> be set to 3 (Dirty).
>
> However, it's possible that HS-level sstatus.FS is Clean and VS-level
> vsstatus.FS is Dirty at the time mark_fs_dirty() is called when V=1.
> We can't early return for this case because we still need to set
> sstatus.FS to Dirty according to spec.
>
> Signed-off-by: Frank Chang 
> Reviewed-by: Vincent Chen 
> Tested-by: Vincent Chen 
> Reviewed-by: Richard Henderson 

Thanks!

Applied to riscv-to-apply.next

Alistair

Re: [PATCH v2 2/3] target/riscv: Implement the stval/mtval illegal instruction

[Alistair Francis]

On Wed, Sep 8, 2021 at 4:48 PM Richard Henderson
 wrote:
>
> On 9/8/21 6:54 AM, Alistair Francis wrote:
> > @@ -967,6 +967,16 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >   write_tval  = true;
> >   tval = env->badaddr;
> >   break;
> > +case RISCV_EXCP_ILLEGAL_INST:
> > +if (riscv_feature(env, RISCV_FEATURE_MTVAL_INST)) {
> > +/*
> > + * The stval/mtval register can optionally also be used to
> > + * return the faulting instruction bits on an illegal
> > + * instruction exception.
> > + */
> > +tval = env->bins;
> > +}
> > +break;
>
> I'll note that write_tval should probably be renamed, and/or eliminated, 
> because it looks
> like it's incorrectly unset here.  If you move the adjustment to cause above 
> this switch,
> then you can move the RVH code that needed write_tval into this switch (just 
> the
> HSTATUS_GVA update?).
>
> But... more specific to this case.  Prior to this, was the exception handler 
> allowed to
> assume anything about the contents of stval?  Should the value have been 
> zero?  Would it
> be wrong to write to stval unconditionally?  How does the guest OS know that 
> it can rely
> on stval being set?

As we didn't support writing the illegal instruction stval should be
zero before this patch.

>
> I simply wonder whether it's worthwhile to add the feature and feature test.

Do you just mean have it enabled all the time?

Alistair

>
>
> r~

[PULL 1/3] hw/nvme: fix validation of ASQ and ACQ

[Klaus Jensen]

From: Klaus Jensen 

Address 0x0 is a valid address. Fix the admin submission and completion
queue address validation to not error out on this.

Signed-off-by: Klaus Jensen 
Reviewed-by: Keith Busch 
---
 hw/nvme/ctrl.c   | 8 
 hw/nvme/trace-events | 2 --
 2 files changed, 10 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 6baf9e0420d5..ff784851137e 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5623,14 +5623,6 @@ static int nvme_start_ctrl(NvmeCtrl *n)
 trace_pci_nvme_err_startfail_sq();
 return -1;
 }
-if (unlikely(!asq)) {
-trace_pci_nvme_err_startfail_nbarasq();
-return -1;
-}
-if (unlikely(!acq)) {
-trace_pci_nvme_err_startfail_nbaracq();
-return -1;
-}
 if (unlikely(asq & (page_size - 1))) {
 trace_pci_nvme_err_startfail_asq_misaligned(asq);
 return -1;
diff --git a/hw/nvme/trace-events b/hw/nvme/trace-events
index 430eeb395b24..ff6cafd520df 100644
--- a/hw/nvme/trace-events
+++ b/hw/nvme/trace-events
@@ -159,8 +159,6 @@ pci_nvme_err_invalid_setfeat(uint32_t dw10) "invalid set 
features, dw10=0x%"PRIx
 pci_nvme_err_invalid_log_page(uint16_t cid, uint16_t lid) "cid %"PRIu16" lid 
0x%"PRIx16""
 pci_nvme_err_startfail_cq(void) "nvme_start_ctrl failed because there are 
non-admin completion queues"
 pci_nvme_err_startfail_sq(void) "nvme_start_ctrl failed because there are 
non-admin submission queues"
-pci_nvme_err_startfail_nbarasq(void) "nvme_start_ctrl failed because the admin 
submission queue address is null"
-pci_nvme_err_startfail_nbaracq(void) "nvme_start_ctrl failed because the admin 
completion queue address is null"
 pci_nvme_err_startfail_asq_misaligned(uint64_t addr) "nvme_start_ctrl failed 
because the admin submission queue address is misaligned: 0x%"PRIx64""
 pci_nvme_err_startfail_acq_misaligned(uint64_t addr) "nvme_start_ctrl failed 
because the admin completion queue address is misaligned: 0x%"PRIx64""
 pci_nvme_err_startfail_page_too_small(uint8_t log2ps, uint8_t maxlog2ps) 
"nvme_start_ctrl failed because the page size is too small: log2size=%u, min=%u"
-- 
2.33.0

RE: [question] VFIO Device Migration: The vCPU may be paused during vfio device DMA in iommu nested stage mode && vSVA

[Tian, Kevin]

> From: Kunkun Jiang 
> Sent: Friday, September 24, 2021 2:19 PM
> 
> Hi all,
> 
> I encountered a problem in vfio device migration test. The
> vCPU may be paused during vfio-pci DMA in iommu nested
> stage mode && vSVA. This may lead to migration fail and
> other problems related to device hardware and driver
> implementation.
> 
> It may be a bit early to discuss this issue, after all, the iommu
> nested stage mode and vSVA are not yet mature. But judging
> from the current implementation, we will definitely encounter
> this problem in the future.

Yes, this is a known limitation to support migration with vSVA.

> 
> This is the current process of vSVA processing translation fault
> in iommu nested stage mode (take SMMU as an example):
> 
> guest os            4.handle translation fault 5.send CMD_RESUME to vSMMU
> 
> 
> qemu                3.inject fault into guest os 6.deliver response to
> host os
> (vfio/vsmmu)
> 
> 
> host os          2.notify the qemu 7.send CMD_RESUME to SMMU
> (vfio/smmu)
> 
> 
> SMMU          1.address translation fault          8.retry or
> terminate
> 
> The order is 1--->8.
> 
> Currently, qemu may pause vCPU at any step. It is possible to
> pause vCPU at step 1-5, that is, in a DMA. This may lead to
> migration fail and other problems related to device hardware
> and driver implementation. For example, the device status
> cannot be changed from RUNNING && SAVING to SAVING,
> because the device DMA is not over.
> 
> As far as i can see, vCPU should not be paused during a device
> IO process, such as DMA. However, currently live migration
> does not pay attention to the state of vfio device when pausing
> the vCPU. And if the vCPU is not paused, the vfio device is
> always running. This looks like a *deadlock*.

Basically this requires:

1) stopping vCPU after stopping device (could selectively enable
this sequence for vSVA);

2) when stopping device, the driver should block new requests
from vCPU (queued to a pending list) and then drain all in-fly 
requests including faults;
* to block this further requires switching from fast-path to
slow trap-emulation path for the cmd portal before stopping
the device;

3) save the pending requests in the vm image and replay them 
after the vm is resumed;
* finally disable blocking by switching back to the fast-path for
the cmd portal;

> 
> Do you have any ideas to solve this problem?
> Looking forward to your replay.
> 

We verified above flow can work in our internal POC. 

Thanks
Kevin

[PULL 0/3] hw/nvme updates

[Klaus Jensen]

From: Klaus Jensen 

Hi Peter,

The following changes since commit 2c3e83f92d93fbab071b8a96b8ab769b01902475:

  Merge remote-tracking branch 
'remotes/alistair23/tags/pull-riscv-to-apply-20210921' into staging (2021-09-21 
10:57:48 -0700)

are available in the Git repository at:

  git://git.infradead.org/qemu-nvme.git tags/nvme-next-pull-request

for you to fetch changes up to c53a9a91021c2f57de9ab18393d0048bd0fe90c2:

  hw/nvme: Return error for fused operations (2021-09-24 08:43:58 +0200)


hw/nvme updates



Klaus Jensen (1):
  hw/nvme: fix validation of ASQ and ACQ

Naveen Nagar (1):
  hw/nvme: fix verification of select field in namespace attachment

Pankaj Raghav (1):
  hw/nvme: Return error for fused operations

 hw/nvme/ctrl.c   | 31 ---
 hw/nvme/trace-events |  2 --
 include/block/nvme.h |  5 +
 3 files changed, 25 insertions(+), 13 deletions(-)

-- 
2.33.0

Re: [PATCH v5 23/31] target/riscv: Restrict has_work() handler to sysemu and TCG

[Alistair Francis]

On Tue, Sep 21, 2021 at 8:09 AM Philippe Mathieu-Daudé  wrote:
>
> Restrict has_work() to TCG sysemu.
>
> Reviewed-by: Richard Henderson 
> Signed-off-by: Philippe Mathieu-Daudé 

Acked-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 13575c14085..abb555a8bdb 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -335,9 +335,9 @@ static void riscv_cpu_synchronize_from_tb(CPUState *cs,
>  env->pc = tb->pc;
>  }
>
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>  static bool riscv_cpu_has_work(CPUState *cs)
>  {
> -#ifndef CONFIG_USER_ONLY
>  RISCVCPU *cpu = RISCV_CPU(cs);
>  CPURISCVState *env = >env;
>  /*
> @@ -345,10 +345,8 @@ static bool riscv_cpu_has_work(CPUState *cs)
>   * mode and delegation registers, but respect individual enables
>   */
>  return (env->mip & env->mie) != 0;
> -#else
> -return true;
> -#endif
>  }
> +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */
>
>  void restore_state_to_opc(CPURISCVState *env, TranslationBlock *tb,
>target_ulong *data)
> @@ -647,6 +645,7 @@ static const struct TCGCPUOps riscv_tcg_ops = {
>  .tlb_fill = riscv_cpu_tlb_fill,
>
>  #ifndef CONFIG_USER_ONLY
> +.has_work = riscv_cpu_has_work,
>  .cpu_exec_interrupt = riscv_cpu_exec_interrupt,
>  .do_interrupt = riscv_cpu_do_interrupt,
>  .do_transaction_failed = riscv_cpu_do_transaction_failed,
> @@ -666,7 +665,6 @@ static void riscv_cpu_class_init(ObjectClass *c, void 
> *data)
>  device_class_set_parent_reset(dc, riscv_cpu_reset, >parent_reset);
>
>  cc->class_by_name = riscv_cpu_class_by_name;
> -cc->has_work = riscv_cpu_has_work;
>  cc->dump_state = riscv_cpu_dump_state;
>  cc->set_pc = riscv_cpu_set_pc;
>  cc->gdb_read_register = riscv_cpu_gdb_read_register;
> --
> 2.31.1
>
>

Re: [PATCH v3] target/riscv: Set mstatus_hs.[SD|FS] bits if Clean and V=1 in mark_fs_dirty()

[Alistair Francis]

On Tue, Sep 21, 2021 at 12:03 PM  wrote:
>
> From: Frank Chang 
>
> When V=1, both vsstauts.FS and HS-level sstatus.FS are in effect.
> Modifying the floating-point state when V=1 causes both fields to
> be set to 3 (Dirty).
>
> However, it's possible that HS-level sstatus.FS is Clean and VS-level
> vsstatus.FS is Dirty at the time mark_fs_dirty() is called when V=1.
> We can't early return for this case because we still need to set
> sstatus.FS to Dirty according to spec.
>
> Signed-off-by: Frank Chang 
> Reviewed-by: Vincent Chen 
> Tested-by: Vincent Chen 
> Reviewed-by: Richard Henderson 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.h   |  4 
>  target/riscv/translate.c | 30 +-
>  2 files changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index e735e53e26c..bef7c551646 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -394,6 +394,7 @@ FIELD(TB_FLAGS, SEW, 5, 3)
>  FIELD(TB_FLAGS, VILL, 8, 1)
>  /* Is a Hypervisor instruction load/store allowed? */
>  FIELD(TB_FLAGS, HLSX, 9, 1)
> +FIELD(TB_FLAGS, MSTATUS_HS_FS, 10, 2)
>
>  bool riscv_cpu_is_32bit(CPURISCVState *env);
>
> @@ -450,6 +451,9 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState 
> *env, target_ulong *pc,
>  get_field(env->hstatus, HSTATUS_HU))) {
>  flags = FIELD_DP32(flags, TB_FLAGS, HLSX, 1);
>  }
> +
> +flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_FS,
> +   get_field(env->mstatus_hs, MSTATUS_FS));
>  }
>  #endif
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 74b33fa3c90..6be22347426 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -58,6 +58,7 @@ typedef struct DisasContext {
>  target_ulong misa;
>  uint32_t opcode;
>  uint32_t mstatus_fs;
> +uint32_t mstatus_hs_fs;
>  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
> @@ -280,27 +281,29 @@ static void gen_jal(DisasContext *ctx, int rd, 
> target_ulong imm)
>  static void mark_fs_dirty(DisasContext *ctx)
>  {
>  TCGv tmp;
> -target_ulong sd;
> +target_ulong sd = is_32bit(ctx) ? MSTATUS32_SD : MSTATUS64_SD;
>
> -if (ctx->mstatus_fs == MSTATUS_FS) {
> -return;
> -}
> -/* Remember the state change for the rest of the TB.  */
> -ctx->mstatus_fs = MSTATUS_FS;
> +if (ctx->mstatus_fs != MSTATUS_FS) {
> +/* Remember the state change for the rest of the TB. */
> +ctx->mstatus_fs = MSTATUS_FS;
>
> -tmp = tcg_temp_new();
> -sd = is_32bit(ctx) ? MSTATUS32_SD : MSTATUS64_SD;
> +tmp = tcg_temp_new();
> +tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
> +tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd);
> +tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
> +tcg_temp_free(tmp);
> +}
>
> -tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
> -tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd);
> -tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
> +if (ctx->virt_enabled && ctx->mstatus_hs_fs != MSTATUS_FS) {
> +/* Remember the stage change for the rest of the TB. */
> +ctx->mstatus_hs_fs = MSTATUS_FS;
>
> -if (ctx->virt_enabled) {
> +tmp = tcg_temp_new();
>  tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs));
>  tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd);
>  tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs));
> +tcg_temp_free(tmp);
>  }
> -tcg_temp_free(tmp);
>  }
>  #else
>  static inline void mark_fs_dirty(DisasContext *ctx) { }
> @@ -533,6 +536,7 @@ static void riscv_tr_init_disas_context(DisasContextBase 
> *dcbase, CPUState *cs)
>  ctx->frm = -1;  /* unknown rounding mode */
>  ctx->ext_ifencei = cpu->cfg.ext_ifencei;
>  ctx->vlen = cpu->cfg.vlen;
> +ctx->mstatus_hs_fs = FIELD_EX32(tb_flags, TB_FLAGS, MSTATUS_HS_FS);
>  ctx->hlsx = FIELD_EX32(tb_flags, TB_FLAGS, HLSX);
>  ctx->vill = FIELD_EX32(tb_flags, TB_FLAGS, VILL);
>  ctx->sew = FIELD_EX32(tb_flags, TB_FLAGS, SEW);
> --
> 2.25.1
>
>

Re: [PATCH v5 14/20] mac-nubus-bridge: rename MacNubusState to MacNubusBridge

[Mark Cave-Ayland]

On 23/09/2021 17:03, Mark Cave-Ayland wrote:


On 23/09/2021 14:53, Laurent Vivier wrote:


Le 23/09/2021 à 14:50, Mark Cave-Ayland a écrit :

On 23/09/2021 11:35, Laurent Vivier wrote:


Le 23/09/2021 à 11:13, Mark Cave-Ayland a écrit :

This better reflects that the mac-nubus-bridge device is derived from the
nubus-bridge device, and that the structure represents the state of the bridge
device and not the Nubus itself. Also update the comment in the file header to
reflect that mac-nubus-bridge is specific to the Macintosh.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Philippe Mathieu-Daudé 
---
   hw/nubus/mac-nubus-bridge.c | 8 +---
   include/hw/nubus/mac-nubus-bridge.h | 4 ++--
   2 files changed, 7 insertions(+), 5 deletions(-)




Reviewed-by: Laurent Vivier 

(it could be merged with previous one)


I like to try and keep renames on a per-device basis if possible, even if it's 
just to help rebasing

during development.


Ok. No problem.

Other than that, is there anything else outstanding you think would require a v6 
series?


Except if you want to address comments from Philippe, I can collect this series and 
send a PR.


Okay I see there are a couple more comments on v5 - let me take a quick look.


There's probably just about enough minor comments to make it worth doing a v6, so I 
will update and resend.



ATB,

Mark.

[PATCH 0/1] hw: aspeed_gpio: Fix GPIO array indexing

[pdel]

From: Peter Delevoryas 

Hey everyone,

I think there might be a bug aspeed_gpio_update, when it's selecting a
GPIO IRQ to update. I was testing booting Facebook's OpenBMC platform
"YosemiteV2" (fby2), and I was hitting a segfault in QEMU:

qemu-system-arm -machine ast2500-evb \
-drive file=fby2.mtd,format=raw,if=mtd \
-serial stdio -display none
...
Setup Caching for Bridge IC info..done.
Setup Front Panel Daemon..done.
Setup fan speed...
FAN CONFIG : Single Rotor FAN
Unexpected 4 Servers config! Run FSC 4 TLs Config as default config
Setting Zone 0 speed to 70%
Setting Zone 1 speed to 70%
ok: run: fscd: (pid 1726) 0s
done.
Powering fru 1 to ON state...
Segmentation fault (core dumped)

In gdb:

Thread 3 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x720ee700 (LWP 1840353)]
qemu_set_irq (irq=0x, level=1) at ../hw/core/irq.c:45
45  irq->handler(irq->opaque, irq->n, level);
(gdb) p irq
$1 = (qemu_irq) 0x
(gdb) up
#1  0x558e36f5 in aspeed_gpio_update (s=0x77ecffb0, 
regs=0x77ed0c94, value=128) at ../hw/gpio/aspeed_gpio.c:287
287 qemu_set_irq(s->gpios[offset], !!(new & mask));
(gdb) p s->gpios
$2 = {0x0 }
(gdb) p offset
$3 = 231
(gdb) p set
$5 = 7
(gdb) p gpio
$4 = 7

The commit message for the fix has a little more info on the bug here,
see that for more info.

I tested this by verifying that after this diff, I can boot this fby2
platform. I don't see any unit or qtest's for aspeed gpio's, maybe I
could add one? I figured that, first, I could just put out an email to
let everyone know about it, and get the diff reviewed.

The image I was using is here:

https://github.com/peterdelevoryas/openbmc/releases/tag/fby2.debug.mtd

Peter Delevoryas (1):
  hw: aspeed_gpio: Fix GPIO array indexing

 hw/gpio/aspeed_gpio.c | 80 +++
 include/hw/gpio/aspeed_gpio.h |  5 +--
 2 files changed, 35 insertions(+), 50 deletions(-)

-- 
2.30.2

[PATCH 1/1] hw: aspeed_gpio: Fix GPIO array indexing

[pdel]

From: Peter Delevoryas 

The gpio array is declared as a dense array:

...
qemu_irq gpios[ASPEED_GPIO_NR_PINS];

(AST2500 has 228, AST2400 has 216, AST2600 has 208)

However, this array is used like a matrix of GPIO sets
(e.g. gpio[NR_SETS][NR_PINS_PER_SET] = gpio[8][32])

size_t offset = set * GPIOS_PER_SET + gpio;
qemu_set_irq(s->gpios[offset], !!(new & mask));

This can result in an out-of-bounds access to "s->gpios" because the
gpio sets do _not_ have the same length. Some of the groups (e.g.
GPIOAB) only have 4 pins. 228 != 8 * 32 == 256.

To fix this, I converted the gpio array from dense to sparse, to that
match both the hardware layout and this existing indexing code.

Also, I noticed that some of the property specifications looked wrong:
the lower 8 bits in the input and output u32's correspond to the first
group label, and the upper 8 bits correspond to the last group label.

I looked at the datasheet and several of these declarations seemed
incorrect to me (I was basing it off of the I/O column). If somebody
can double-check this, I'd really appreciate it!

Some were definitely wrong though, like "Y" and "Z" in the 2600.

@@ -796,7 +776,7 @@ static const GPIOSetProperties ast2500_set_props[] = {
 [3] = {0x,  0x,  {"M", "N", "O", "P"} },
 [4] = {0x,  0x,  {"Q", "R", "S", "T"} },
 [5] = {0x,  0x,  {"U", "V", "W", "X"} },
-[6] = {0xff0f,  0x0f0f,  {"Y", "Z", "AA", "AB"} },
+[6] = {0x0fff,  0x0fff,  {"Y", "Z", "AA", "AB"} },
 [7] = {0x00ff,  0x00ff,  {"AC"} },
 };

@@ -805,9 +785,9 @@ static GPIOSetProperties ast2600_3_3v_set_props[] = {
 [1] = {0x,  0x,  {"E", "F", "G", "H"} },
 [2] = {0x,  0x,  {"I", "J", "K", "L"} },
 [3] = {0x,  0x,  {"M", "N", "O", "P"} },
-[4] = {0x,  0x,  {"Q", "R", "S", "T"} },
-[5] = {0x,  0x,  {"U", "V", "W", "X"} },
-[6] = {0x,  0x0fff,  {"Y", "Z", "", ""} },
+[4] = {0x,  0x00ff,  {"Q", "R", "S", "T"} },
+[5] = {0x,  0xff00,  {"U", "V", "W", "X"} },
+[6] = {0x,  0x,  {"Y", "Z", "", ""} },
 };

Signed-off-by: Peter Delevoryas 
---
 hw/gpio/aspeed_gpio.c | 80 +++
 include/hw/gpio/aspeed_gpio.h |  5 +--
 2 files changed, 35 insertions(+), 50 deletions(-)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index dfa6d6cb40..8e3f7e4398 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -16,11 +16,7 @@
 #include "hw/irq.h"
 #include "migration/vmstate.h"
 
-#define GPIOS_PER_REG 32
-#define GPIOS_PER_SET GPIOS_PER_REG
-#define GPIO_PIN_GAP_SIZE 4
 #define GPIOS_PER_GROUP 8
-#define GPIO_GROUP_SHIFT 3
 
 /* GPIO Source Types */
 #define ASPEED_CMD_SRC_MASK 0x01010101
@@ -259,7 +255,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets 
*regs,
 
 diff = old ^ new;
 if (diff) {
-for (gpio = 0; gpio < GPIOS_PER_REG; gpio++) {
+for (gpio = 0; gpio < ASPEED_GPIOS_PER_SET; gpio++) {
 uint32_t mask = 1 << gpio;
 
 /* If the gpio needs to be updated... */
@@ -283,8 +279,7 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets 
*regs,
 if (direction & mask) {
 /* ...trigger the line-state IRQ */
 ptrdiff_t set = aspeed_gpio_set_idx(s, regs);
-size_t offset = set * GPIOS_PER_SET + gpio;
-qemu_set_irq(s->gpios[offset], !!(new & mask));
+qemu_set_irq(s->gpios[set][gpio], !!(new & mask));
 } else {
 /* ...otherwise if we meet the line's current IRQ policy... */
 if (aspeed_evaluate_irq(regs, old & mask, gpio)) {
@@ -297,21 +292,6 @@ static void aspeed_gpio_update(AspeedGPIOState *s, 
GPIOSets *regs,
 qemu_set_irq(s->irq, !!(s->pending));
 }
 
-static uint32_t aspeed_adjust_pin(AspeedGPIOState *s, uint32_t pin)
-{
-AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-/*
- * The 2500 has a 4 pin gap in group AB and the 2400 has a 4 pin
- * gap in group Y (and only four pins in AB but this is the last group so
- * it doesn't matter).
- */
-if (agc->gap && pin >= agc->gap) {
-pin += GPIO_PIN_GAP_SIZE;
-}
-
-return pin;
-}
-
 static bool aspeed_gpio_get_pin_level(AspeedGPIOState *s, uint32_t set_idx,
   uint32_t pin)
 {
@@ -367,7 +347,7 @@ static uint32_t update_value_control_source(GPIOSets *regs, 
uint32_t old_value,
 uint32_t new_value = 0;
 
 /* for each group in set */
-for (i = 0; i < GPIOS_PER_REG; i += GPIOS_PER_GROUP) {
+for (i = 0; i < ASPEED_GPIOS_PER_SET; i += GPIOS_PER_GROUP) {
 cmd_source = extract32(regs->cmd_source_0, i, 1)
 | (extract32(regs->cmd_source_1, i, 1) << 1);
 
@@ -637,7 +617,7 @@ static void aspeed_gpio_write(void 

[question] VFIO Device Migration: The vCPU may be paused during vfio device DMA in iommu nested stage mode && vSVA

[Kunkun Jiang]

Hi all,

I encountered a problem in vfio device migration test. The
vCPU may be paused during vfio-pci DMA in iommu nested
stage mode && vSVA. This may lead to migration fail and
other problems related to device hardware and driver
implementation.

It may be a bit early to discuss this issue, after all, the iommu
nested stage mode and vSVA are not yet mature. But judging
from the current implementation, we will definitely encounter
this problem in the future.

This is the current process of vSVA processing translation fault
in iommu nested stage mode (take SMMU as an example):

guest os            4.handle translation fault 5.send CMD_RESUME to vSMMU


qemu                3.inject fault into guest os 6.deliver response to 
host os

(vfio/vsmmu)


host os          2.notify the qemu 7.send CMD_RESUME to SMMU
(vfio/smmu)


SMMU          1.address translation fault          8.retry or 
terminate


The order is 1--->8.

Currently, qemu may pause vCPU at any step. It is possible to
pause vCPU at step 1-5, that is, in a DMA. This may lead to
migration fail and other problems related to device hardware
and driver implementation. For example, the device status
cannot be changed from RUNNING && SAVING to SAVING,
because the device DMA is not over.

As far as i can see, vCPU should not be paused during a device
IO process, such as DMA. However, currently live migration
does not pay attention to the state of vfio device when pausing
the vCPU. And if the vCPU is not paused, the vfio device is
always running. This looks like a *deadlock*.

Do you have any ideas to solve this problem?
Looking forward to your replay.

Thanks,
Kunkun Jiang

Re: [PATCH] hw/nvme: reattach subsystem namespaces on hotplug

[Hannes Reinecke]

On 9/23/21 10:09 PM, Klaus Jensen wrote:

On Sep  9 13:37, Hannes Reinecke wrote:

On 9/9/21 12:47 PM, Klaus Jensen wrote:

On Sep  9 11:43, Hannes Reinecke wrote:

With commit 5ffbaeed16 ("hw/nvme: fix controller hot unplugging")
namespaces get moved from the controller to the subsystem if one
is specified.
That keeps the namespaces alive after a controller hot-unplug, but
after a controller hotplug we have to reconnect the namespaces
from the subsystem to the controller.

Fixes: 5ffbaeed16 ("hw/nvme: fix controller hot unplugging")
Cc: Klaus Jensen 
Signed-off-by: Hannes Reinecke 
---
  hw/nvme/subsys.c | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c
index 93c35950d6..a9404f2b5e 100644
--- a/hw/nvme/subsys.c
+++ b/hw/nvme/subsys.c
@@ -14,7 +14,7 @@
  int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
  {
  NvmeSubsystem *subsys = n->subsys;
-int cntlid;
+int cntlid, nsid;
  
  for (cntlid = 0; cntlid < ARRAY_SIZE(subsys->ctrls); cntlid++) {

  if (!subsys->ctrls[cntlid]) {
@@ -29,12 +29,18 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
  
  subsys->ctrls[cntlid] = n;
  
+for (nsid = 0; nsid < ARRAY_SIZE(subsys->namespaces); nsid++) {

+if (subsys->namespaces[nsid]) {
+nvme_attach_ns(n, subsys->namespaces[nsid]);
+}


Thanks Hannes! I like it, keeping things simple.

But we should only attach namespaces that have the shared property or
have ns->attached == 0. Non-shared namespaces may already be attached to
another controller in the subsystem.



Well ... I tried to avoid that subject, but as you brought it up:
There is a slightly tricky issue in fabrics, in that the 'controller' is
independent from the 'connection'.
The 'shared' bit in the CMIC setting indicates that the subsystem may
have more than one _controller_. It doesn't talk about how many
_connections_ a controller may support; that then is the realm of
dynamic or static controllers, which we don't talk about :-).
Sufficient to say, linux only implements the dynamic controller model,
so every connection will be to a different controller.
But you have been warned :-)

However, the 'CMIC' setting is independent on the 'NMIC' setting (ie the
'shared' bit in the namespace).
Which leads to the interesting question on how exactly should one handle
non-shared namespaces in subsystems for which there are multiple
controllers. Especially as the NSID space is per subsystem, so each
controller will be able to figure out if there are blanked-out namespaces.
So to make this a sane setup I would propose to set the 'shared' option
automatically whenever the controller has the 'subsys' option set.
And actually, I would ditch the 'shared' option completely, and make it
dependent on the 'subsys' setting for the controller.
Much like we treat the 'CMIC' setting nowadays.
That avoids lots of confusions, and also make the implementation _way_
easier.



I see your point. Unfortunately, there is no easy way to ditch shared=
now. But I think it'd be good enough to attach to shared automatically,
but not to "shared=off".

I've already ditched the shared parameter on my RFC refactor series and
always having the namespaces shared.



Okay.


However...

The spec says that "The attach and detach operations are persistent
across all reset events.". This means that we should track those events
in the subsystem and only reattach namespaces that were attached at the
time of the "reset" event. Currently, we don't have anything mapping
that state. But the device already has to take some liberties with
regard to stuff that is considered persistent by the spec (SMART log
etc.) since we do not have any way to store stuff persistently across
qemu invocations, so I think the above is an acceptable compromise.


Careful. 'attach' and 'detach' are MI (management interface) operations.
If and how many namespaces are visible to any given controllers is
actually independent on that; and, in fact, controllers might not even
implement 'attach' or 'detach'.
But I do agree that we don't handle the 'reset' state properly.



The Namespace Attachment command has nothing to do with MI? And the QEMU
controller does support attaching and detaching namespaces.



No, you got me wrong. Whether a not a namespace is attached is 
independent of any 'Namespace attachment' command.
Case in point: the subsystem will be starting up with namespace already 
attached, even though no-one issued any namespace attachment command.



A potential (as good as it gets) fix would be to keep a map/list of
"persistently" attached controllers on the namespaces and re-attach
according to that when we see that controller joining the subsystem
again. CNTLID would be the obvious choice for the key here, but problem
is that we cant really use it since we assign it sequentially from the
subsystem, which now looks like a pretty bad choice. CNTLID should have
been a required property