Re: [PATCH v2 2/6] accel: accel_available() function
On Thu, Nov 26, 2020 at 10:14:31AM +0100, Claudio Fontana wrote:
> Hi Eduardo,
>
> On 11/25/20 9:56 PM, Eduardo Habkost wrote:
> > This function will be used to replace the xen_available() and
> > kvm_available() functions from arch_init.c.
> >
> > Signed-off-by: Eduardo Habkost
> > ---
> > Cc: Richard Henderson
> > Cc: Paolo Bonzini
> > Cc: Claudio Fontana
> > Cc: Roman Bolshakov
> > ---
> > include/sysemu/accel.h | 1 +
> > accel/accel.c | 5 +
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> > index e08b8ab8fa..a4a00c75c8 100644
> > --- a/include/sysemu/accel.h
> > +++ b/include/sysemu/accel.h
> > @@ -67,6 +67,7 @@ typedef struct AccelClass {
> > OBJECT_GET_CLASS(AccelClass, (obj), TYPE_ACCEL)
> >
> > AccelClass *accel_find(const char *opt_name);
> > +bool accel_available(const char *name);
> > int accel_init_machine(AccelState *accel, MachineState *ms);
> >
> > /* Called just before os_setup_post (ie just before drop OS privs) */
> > diff --git a/accel/accel.c b/accel/accel.c
> > index cb555e3b06..4a64a2b38a 100644
> > --- a/accel/accel.c
> > +++ b/accel/accel.c
> > @@ -46,6 +46,11 @@ AccelClass *accel_find(const char *opt_name)
> > return ac;
> > }
> >
> > +bool accel_available(const char *name)
> > +{
> > +return accel_find(name) != NULL;
>
>
> accel_find() in its implementation allocates and then frees memory to
> generate the string,
> the user of accel_available() might be unaware and overuse leading to
> fragmentation/performance issues?
Is that a real issue? We had only 3 users of kvm_available() and
xen_available() since those functions were added 10 years ago.
Do you have any suggestions on what we should do?
--
Eduardo
[PATCH v2 5/6] Remove unnecessary usage of arch_init.h
The only declarations in arch_init.h are the `arch_type` variable and the QEMU_ARCH_* constants. Stop including arch_init.h from code that don't use neither. Patch generated automatically using the command: $ sed -i -e '/#include "sysemu.arch_init.h"/d' \ $(comm -23 \ <(git grep -l arch_init.h | sort) \ <((git grep -l -w 'arch_type'; g grep -l QEMU_ARCH;) | sort -u)) Signed-off-by: Eduardo Habkost --- Cc: Richard Henderson Cc: Paolo Bonzini Cc: Eduardo Habkost Cc: "Michael S. Tsirkin" Cc: Marcel Apfelbaum Cc: "Hervé Poussineau" Cc: Aleksandar Rikalo Cc: "Philippe Mathieu-Daudé" Cc: Aurelien Jarno Cc: Jiaxun Yang Cc: David Gibson Cc: Palmer Dabbelt Cc: Alistair Francis Cc: Sagar Karandikar Cc: Bastian Koppelmann Cc: Markus Armbruster Cc: David Hildenbrand Cc: Cornelia Huck Cc: Thomas Huth Cc: Halil Pasic Cc: Christian Borntraeger Cc: qemu-devel@nongnu.org Cc: qemu-...@nongnu.org Cc: qemu-ri...@nongnu.org Cc: qemu-s3...@nongnu.org --- accel/accel.c | 1 - hw/i386/pc.c| 1 - hw/i386/pc_piix.c | 1 - hw/i386/pc_q35.c| 1 - hw/mips/jazz.c | 1 - hw/mips/malta.c | 1 - hw/ppc/prep.c | 1 - hw/riscv/sifive_e.c | 1 - hw/riscv/sifive_u.c | 1 - hw/riscv/spike.c| 1 - hw/riscv/virt.c | 1 - monitor/qmp-cmds.c | 1 - target/i386/cpu.c | 1 - target/s390x/cpu.c | 1 - target/s390x/cpu_models.c | 1 - target/ppc/translate_init.c.inc | 1 - 16 files changed, 16 deletions(-) diff --git a/accel/accel.c b/accel/accel.c index 4a64a2b38a..3e567a001f 100644 --- a/accel/accel.c +++ b/accel/accel.c @@ -26,7 +26,6 @@ #include "qemu/osdep.h" #include "sysemu/accel.h" #include "hw/boards.h" -#include "sysemu/arch_init.h" #include "sysemu/sysemu.h" #include "qom/object.h" diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 17b514d1da..48a5fb0798 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -67,7 +67,6 @@ #include "ui/qemu-spice.h" #include "exec/memory.h" #include "exec/address-spaces.h" -#include "sysemu/arch_init.h" #include "qemu/bitmap.h" #include "qemu/config-file.h" #include "qemu/error-report.h" diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 13d1628f13..95ee1f39d8 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -44,7 +44,6 @@ #include "hw/kvm/clock.h" #include "sysemu/sysemu.h" #include "hw/sysbus.h" -#include "sysemu/arch_init.h" #include "hw/i2c/smbus_eeprom.h" #include "hw/xen/xen-x86.h" #include "exec/memory.h" diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index a3f4959c43..cf85b240bc 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -31,7 +31,6 @@ #include "qemu/osdep.h" #include "qemu/units.h" #include "hw/loader.h" -#include "sysemu/arch_init.h" #include "hw/i2c/smbus_eeprom.h" #include "hw/rtc/mc146818rtc.h" #include "sysemu/kvm.h" diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c index 71448f72ac..6fee2a4ec0 100644 --- a/hw/mips/jazz.c +++ b/hw/mips/jazz.c @@ -34,7 +34,6 @@ #include "hw/isa/isa.h" #include "hw/block/fdc.h" #include "sysemu/sysemu.h" -#include "sysemu/arch_init.h" #include "hw/boards.h" #include "net/net.h" #include "hw/scsi/esp.h" diff --git a/hw/mips/malta.c b/hw/mips/malta.c index 9d1a3b50b7..6b4387c179 100644 --- a/hw/mips/malta.c +++ b/hw/mips/malta.c @@ -38,7 +38,6 @@ #include "hw/mips/cpudevs.h" #include "hw/pci/pci.h" #include "sysemu/sysemu.h" -#include "sysemu/arch_init.h" #include "qemu/log.h" #include "hw/mips/bios.h" #include "hw/ide.h" diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c index 4a0cb434a6..5c9ec45749 100644 --- a/hw/ppc/prep.c +++ b/hw/ppc/prep.c @@ -43,7 +43,6 @@ #include "hw/rtc/mc146818rtc.h" #include "hw/isa/pc87312.h" #include "hw/qdev-properties.h" -#include "sysemu/arch_init.h" #include "sysemu/kvm.h" #include "sysemu/qtest.h" #include "sysemu/reset.h" diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c index 59bac4cc9a..6185872127 100644 --- a/hw/riscv/sifive_e.c +++ b/hw/riscv/sifive_e.c @@ -46,7 +46,6 @@ #include "hw/intc/sifive_plic.h" #include "hw/misc/sifive_e_prci.h" #include "chardev/char.h" -#include "sysemu/arch_init.h" #include "sysemu/sysemu.h" #include "exec/address-spaces.h" diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sif
[PATCH v2 4/6] xen: Delete xen_available() function
The function can be replaced with accel_available("xen").
Signed-off-by: Eduardo Habkost
---
Cc: Paolo Bonzini
Cc: qemu-devel@nongnu.org
Cc: Stefano Stabellini
Cc: Anthony Perard
Cc: Paul Durrant
Cc: xen-de...@lists.xenproject.org
Cc: Richard Henderson
Cc: Claudio Fontana
Cc: Roman Bolshakov
---
include/sysemu/arch_init.h | 2 --
softmmu/arch_init.c| 9 -
softmmu/vl.c | 6 +++---
3 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index b32ce1afa9..40ac8052b7 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -32,6 +32,4 @@ enum {
extern const uint32_t arch_type;
-int xen_available(void);
-
#endif
diff --git a/softmmu/arch_init.c b/softmmu/arch_init.c
index 79383c8db8..f4770931f5 100644
--- a/softmmu/arch_init.c
+++ b/softmmu/arch_init.c
@@ -49,12 +49,3 @@ int graphic_depth = 32;
const uint32_t arch_type = QEMU_ARCH;
-
-int xen_available(void)
-{
-#ifdef CONFIG_XEN
-return 1;
-#else
-return 0;
-#endif
-}
diff --git a/softmmu/vl.c b/softmmu/vl.c
index e6e0ad5a92..74b6ebf1e4 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -3667,21 +3667,21 @@ void qemu_init(int argc, char **argv, char **envp)
has_defaults = 0;
break;
case QEMU_OPTION_xen_domid:
-if (!(xen_available())) {
+if (!(accel_available("xen"))) {
error_report("Option not supported for this target");
exit(1);
}
xen_domid = atoi(optarg);
break;
case QEMU_OPTION_xen_attach:
-if (!(xen_available())) {
+if (!(accel_available("xen"))) {
error_report("Option not supported for this target");
exit(1);
}
xen_mode = XEN_ATTACH;
break;
case QEMU_OPTION_xen_domid_restrict:
-if (!(xen_available())) {
+if (!(accel_available("xen"))) {
error_report("Option not supported for this target");
exit(1);
}
--
2.28.0
[PATCH v2 3/6] kvm: Remove kvm_available() function
The only caller can use accel_available("kvm") instead.
Signed-off-by: Eduardo Habkost
---
Cc: Markus Armbruster
Cc: qemu-devel@nongnu.org
Cc: Paolo Bonzini
Cc: k...@vger.kernel.org
Cc: Richard Henderson
Cc: Paolo Bonzini
Cc: Claudio Fontana
Cc: Roman Bolshakov
---
include/sysemu/arch_init.h | 1 -
monitor/qmp-cmds.c | 2 +-
softmmu/arch_init.c| 9 -
3 files changed, 1 insertion(+), 11 deletions(-)
diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index 54f069d491..b32ce1afa9 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -32,7 +32,6 @@ enum {
extern const uint32_t arch_type;
-int kvm_available(void);
int xen_available(void);
#endif
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index a08143b323..ac5b8a97d7 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -57,7 +57,7 @@ KvmInfo *qmp_query_kvm(Error **errp)
KvmInfo *info = g_malloc0(sizeof(*info));
info->enabled = kvm_enabled();
-info->present = kvm_available();
+info->present = accel_available("kvm");
return info;
}
diff --git a/softmmu/arch_init.c b/softmmu/arch_init.c
index 7ef32a98b9..79383c8db8 100644
--- a/softmmu/arch_init.c
+++ b/softmmu/arch_init.c
@@ -50,15 +50,6 @@ int graphic_depth = 32;
const uint32_t arch_type = QEMU_ARCH;
-int kvm_available(void)
-{
-#ifdef CONFIG_KVM
-return 1;
-#else
-return 0;
-#endif
-}
-
int xen_available(void)
{
#ifdef CONFIG_XEN
--
2.28.0
[PATCH v2 6/6] Rename arch_init.h to arch_type.h
The only declarations in arch_init.h are related to the arch_type
variable (which is a useful feature that allows us to simplify
command line option handling). Rename the header to reflect its
purpose.
Signed-off-by: Eduardo Habkost
---
Cc: Markus Armbruster
Cc: Kevin Wolf
Cc: Max Reitz
Cc: Paolo Bonzini
Cc: "Daniel P. Berrangé"
Cc: Eduardo Habkost
Cc: qemu-bl...@nongnu.org
Cc: qemu-devel@nongnu.org
---
include/sysemu/{arch_init.h => arch_type.h} | 0
blockdev.c | 2 +-
softmmu/arch_init.c | 2 +-
softmmu/qdev-monitor.c | 2 +-
softmmu/vl.c| 2 +-
stubs/arch_type.c | 2 +-
6 files changed, 5 insertions(+), 5 deletions(-)
rename include/sysemu/{arch_init.h => arch_type.h} (100%)
diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_type.h
similarity index 100%
rename from include/sysemu/arch_init.h
rename to include/sysemu/arch_type.h
diff --git a/blockdev.c b/blockdev.c
index fe6fb5dc1d..46c10b2609 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -56,7 +56,7 @@
#include "sysemu/iothread.h"
#include "block/block_int.h"
#include "block/trace.h"
-#include "sysemu/arch_init.h"
+#include "sysemu/arch_type.h"
#include "sysemu/qtest.h"
#include "sysemu/runstate.h"
#include "sysemu/replay.h"
diff --git a/softmmu/arch_init.c b/softmmu/arch_init.c
index f4770931f5..5a9bc56387 100644
--- a/softmmu/arch_init.c
+++ b/softmmu/arch_init.c
@@ -24,7 +24,7 @@
#include "qemu/osdep.h"
#include "cpu.h"
#include "sysemu/sysemu.h"
-#include "sysemu/arch_init.h"
+#include "sysemu/arch_type.h"
#include "hw/pci/pci.h"
#include "hw/audio/soundhw.h"
#include "qapi/error.h"
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index bf79d0bbcd..c8b7fb27dc 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -22,7 +22,7 @@
#include "monitor/hmp.h"
#include "monitor/monitor.h"
#include "monitor/qdev.h"
-#include "sysemu/arch_init.h"
+#include "sysemu/arch_type.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-qdev.h"
#include "qapi/qmp/qdict.h"
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 74b6ebf1e4..1dd63b2782 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -95,7 +95,7 @@
#include "trace/control.h"
#include "qemu/plugin.h"
#include "qemu/queue.h"
-#include "sysemu/arch_init.h"
+#include "sysemu/arch_type.h"
#include "ui/qemu-spice.h"
#include "qapi/string-input-visitor.h"
diff --git a/stubs/arch_type.c b/stubs/arch_type.c
index fc5423bc98..603a49deec 100644
--- a/stubs/arch_type.c
+++ b/stubs/arch_type.c
@@ -1,4 +1,4 @@
#include "qemu/osdep.h"
-#include "sysemu/arch_init.h"
+#include "sysemu/arch_type.h"
const uint32_t arch_type = QEMU_ARCH_NONE;
--
2.28.0
[PATCH v2 2/6] accel: accel_available() function
This function will be used to replace the xen_available() and
kvm_available() functions from arch_init.c.
Signed-off-by: Eduardo Habkost
---
Cc: Richard Henderson
Cc: Paolo Bonzini
Cc: Claudio Fontana
Cc: Roman Bolshakov
---
include/sysemu/accel.h | 1 +
accel/accel.c | 5 +
2 files changed, 6 insertions(+)
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index e08b8ab8fa..a4a00c75c8 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -67,6 +67,7 @@ typedef struct AccelClass {
OBJECT_GET_CLASS(AccelClass, (obj), TYPE_ACCEL)
AccelClass *accel_find(const char *opt_name);
+bool accel_available(const char *name);
int accel_init_machine(AccelState *accel, MachineState *ms);
/* Called just before os_setup_post (ie just before drop OS privs) */
diff --git a/accel/accel.c b/accel/accel.c
index cb555e3b06..4a64a2b38a 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -46,6 +46,11 @@ AccelClass *accel_find(const char *opt_name)
return ac;
}
+bool accel_available(const char *name)
+{
+return accel_find(name) != NULL;
+}
+
int accel_init_machine(AccelState *accel, MachineState *ms)
{
AccelClass *acc = ACCEL_GET_CLASS(accel);
--
2.28.0
[PATCH v2 0/6] arch_init.c cleanup
This series gets rid of most of the code in arch_init.c. It
moves the QEMU_ARCH macro definitions to corresponding cpu.h
files, and gets rid of kvm_available() and xen_available().
After this series, only two things remain in arch_init.c:
- the arch_type variable, which seems to be a useful feature; and
- the initialization of graphic_width/graphic_height/graphic_depth,
which is a hack we must eventually get rid of.
Gerd got rid of the graphic_* initialization hack once (in 2017),
but the series was never merged:
https://lore.kernel.org/qemu-devel/1487715299-21102-5-git-send-email-kra...@redhat.com
Eduardo Habkost (6):
arch_init: Move QEMU_ARCH definitions to cpu.h
accel: accel_available() function
kvm: Remove kvm_available() function
xen: Delete xen_available() function
Remove unnecessary usage of arch_init.h
Rename arch_init.h to arch_type.h
include/sysemu/accel.h | 1 +
include/sysemu/{arch_init.h => arch_type.h} | 3 -
target/alpha/cpu.h | 1 +
target/arm/cpu.h| 1 +
target/avr/cpu.h| 1 +
target/cris/cpu.h | 1 +
target/hppa/cpu.h | 1 +
target/i386/cpu.h | 1 +
target/lm32/cpu.h | 1 +
target/m68k/cpu.h | 1 +
target/microblaze/cpu.h | 1 +
target/mips/cpu.h | 1 +
target/moxie/cpu.h | 1 +
target/nios2/cpu.h | 1 +
target/openrisc/cpu.h | 1 +
target/ppc/cpu.h| 1 +
target/riscv/cpu.h | 1 +
target/rx/cpu.h | 1 +
target/s390x/cpu.h | 1 +
target/sh4/cpu.h| 1 +
target/sparc/cpu.h | 1 +
target/tricore/cpu.h| 1 +
target/unicore32/cpu.h | 1 +
target/xtensa/cpu.h | 1 +
accel/accel.c | 6 +-
blockdev.c | 2 +-
hw/i386/pc.c| 1 -
hw/i386/pc_piix.c | 1 -
hw/i386/pc_q35.c| 1 -
hw/mips/jazz.c | 1 -
hw/mips/malta.c | 1 -
hw/ppc/prep.c | 1 -
hw/riscv/sifive_e.c | 1 -
hw/riscv/sifive_u.c | 1 -
hw/riscv/spike.c| 1 -
hw/riscv/virt.c | 1 -
monitor/qmp-cmds.c | 3 +-
softmmu/arch_init.c | 66 +
softmmu/qdev-monitor.c | 2 +-
softmmu/vl.c| 8 +--
stubs/arch_type.c | 2 +-
target/i386/cpu.c | 1 -
target/s390x/cpu.c | 1 -
target/s390x/cpu_models.c | 1 -
target/ppc/translate_init.c.inc | 1 -
45 files changed, 37 insertions(+), 92 deletions(-)
rename include/sysemu/{arch_init.h => arch_type.h} (94%)
--
2.28.0
[PATCH v2 1/6] arch_init: Move QEMU_ARCH definitions to cpu.h
Instead of a collection of #ifdefs on arch_init.c, define
QEMU_ARCH inside each cpu.h file.
Signed-off-by: Eduardo Habkost
---
Cc: Richard Henderson
Cc: Peter Maydell
Cc: Michael Rolnik
Cc: Sarah Harris
Cc: "Edgar E. Iglesias"
Cc: Paolo Bonzini
Cc: Eduardo Habkost
Cc: Michael Walle
Cc: Laurent Vivier
Cc: "Philippe Mathieu-Daudé"
Cc: Aurelien Jarno
Cc: Jiaxun Yang
Cc: Aleksandar Rikalo
Cc: Anthony Green
Cc: Chris Wulff
Cc: Marek Vasut
Cc: Stafford Horne
Cc: David Gibson
Cc: Palmer Dabbelt
Cc: Alistair Francis
Cc: Sagar Karandikar
Cc: Bastian Koppelmann
Cc: Yoshinori Sato
Cc: David Hildenbrand
Cc: Cornelia Huck
Cc: Thomas Huth
Cc: Mark Cave-Ayland
Cc: Artyom Tarasenko
Cc: Guan Xuetao
Cc: Max Filippov
Cc: qemu-devel@nongnu.org
Cc: qemu-...@nongnu.org
Cc: qemu-...@nongnu.org
Cc: qemu-ri...@nongnu.org
Cc: qemu-s3...@nongnu.org
---
target/alpha/cpu.h | 1 +
target/arm/cpu.h| 1 +
target/avr/cpu.h| 1 +
target/cris/cpu.h | 1 +
target/hppa/cpu.h | 1 +
target/i386/cpu.h | 1 +
target/lm32/cpu.h | 1 +
target/m68k/cpu.h | 1 +
target/microblaze/cpu.h | 1 +
target/mips/cpu.h | 1 +
target/moxie/cpu.h | 1 +
target/nios2/cpu.h | 1 +
target/openrisc/cpu.h | 1 +
target/ppc/cpu.h| 1 +
target/riscv/cpu.h | 1 +
target/rx/cpu.h | 1 +
target/s390x/cpu.h | 1 +
target/sh4/cpu.h| 1 +
target/sparc/cpu.h | 1 +
target/tricore/cpu.h| 1 +
target/unicore32/cpu.h | 1 +
target/xtensa/cpu.h | 1 +
softmmu/arch_init.c | 46 -
23 files changed, 22 insertions(+), 46 deletions(-)
diff --git a/target/alpha/cpu.h b/target/alpha/cpu.h
index 82df108967..313a4e456e 100644
--- a/target/alpha/cpu.h
+++ b/target/alpha/cpu.h
@@ -438,6 +438,7 @@ void alpha_translate_init(void);
#define ALPHA_CPU_TYPE_SUFFIX "-" TYPE_ALPHA_CPU
#define ALPHA_CPU_TYPE_NAME(model) model ALPHA_CPU_TYPE_SUFFIX
#define CPU_RESOLVING_TYPE TYPE_ALPHA_CPU
+#define QEMU_ARCH QEMU_ARCH_ALPHA
void alpha_cpu_list(void);
/* you can call this signal handler from your SIGBUS and SIGSEGV
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e5514c8286..3f469a6485 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2822,6 +2822,7 @@ bool write_cpustate_to_list(ARMCPU *cpu, bool kvm_sync);
#define ARM_CPU_TYPE_SUFFIX "-" TYPE_ARM_CPU
#define ARM_CPU_TYPE_NAME(name) (name ARM_CPU_TYPE_SUFFIX)
#define CPU_RESOLVING_TYPE TYPE_ARM_CPU
+#define QEMU_ARCH QEMU_ARCH_ARM
#define cpu_signal_handler cpu_arm_signal_handler
#define cpu_list arm_cpu_list
diff --git a/target/avr/cpu.h b/target/avr/cpu.h
index d148e8c75a..98f5df0ad7 100644
--- a/target/avr/cpu.h
+++ b/target/avr/cpu.h
@@ -31,6 +31,7 @@
#define AVR_CPU_TYPE_SUFFIX "-" TYPE_AVR_CPU
#define AVR_CPU_TYPE_NAME(name) (name AVR_CPU_TYPE_SUFFIX)
#define CPU_RESOLVING_TYPE TYPE_AVR_CPU
+#define QEMU_ARCH QEMU_ARCH_AVR
#define TCG_GUEST_DEFAULT_MO 0
diff --git a/target/cris/cpu.h b/target/cris/cpu.h
index d3b6492909..2482915699 100644
--- a/target/cris/cpu.h
+++ b/target/cris/cpu.h
@@ -249,6 +249,7 @@ enum {
#define CRIS_CPU_TYPE_SUFFIX "-" TYPE_CRIS_CPU
#define CRIS_CPU_TYPE_NAME(name) (name CRIS_CPU_TYPE_SUFFIX)
#define CPU_RESOLVING_TYPE TYPE_CRIS_CPU
+#define QEMU_ARCH QEMU_ARCH_CRIS
#define cpu_signal_handler cpu_cris_signal_handler
diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
index 61178fa6a2..74d813289b 100644
--- a/target/hppa/cpu.h
+++ b/target/hppa/cpu.h
@@ -242,6 +242,7 @@ static inline int cpu_mmu_index(CPUHPPAState *env, bool
ifetch)
void hppa_translate_init(void);
#define CPU_RESOLVING_TYPE TYPE_HPPA_CPU
+#define QEMU_ARCH QEMU_ARCH_HPPA
static inline target_ulong hppa_form_gva_psw(target_ureg psw, uint64_t spc,
target_ureg off)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 88e8586f8f..03202f610c 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1971,6 +1971,7 @@ uint64_t cpu_get_tsc(CPUX86State *env);
#define X86_CPU_TYPE_SUFFIX "-" TYPE_X86_CPU
#define X86_CPU_TYPE_NAME(name) (name X86_CPU_TYPE_SUFFIX)
#define CPU_RESOLVING_TYPE TYPE_X86_CPU
+#define QEMU_ARCH QEMU_ARCH_I386
#ifdef TARGET_X86_64
#define TARGET_DEFAULT_CPU_TYPE X86_CPU_TYPE_NAME("qemu64")
diff --git a/target/lm32/cpu.h b/target/lm32/cpu.h
index ea7c01ca8b..169c0ff19d 100644
--- a/target/lm32/cpu.h
+++ b/target/lm32/cpu.h
@@ -238,6 +238,7 @@ bool lm32_cpu_do_semihosting(CPUState *cs);
#define LM32_CPU_TYPE_SUFFIX "-" TYPE_LM32_CPU
#define LM32_CPU_TYPE_NAME(model) model LM32_CPU_TYPE_SUFFIX
#define CPU_RESOLVING_TYPE TYPE_LM32_CPU
+#define QEMU_ARCH QEMU_ARCH_LM32
#define cpu_list lm32_cpu_list
#define cpu_signal_handler cpu_lm32_signal_handler
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 521ac67cdd..
Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
On Wed, Nov 25, 2020 at 07:40:48AM +0100, Markus Armbruster wrote: > Eduardo Habkost writes: > > > On Tue, Nov 24, 2020 at 04:20:37PM +0100, Markus Armbruster wrote: > >> Eduardo Habkost writes: > >> > >> > On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote: > >> >> Eduardo Habkost writes: > >> >> > >> >> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote: > >> >> >> Eduardo Habkost writes: > >> >> >> > >> >> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote: > >> >> >> [...] > >> >> >> >> When the structure of a data type is to be kept away from its > >> >> >> >> users, I > >> >> >> >> prefer to keep it out of the public header, so the compiler > >> >> >> >> enforces the > >> >> >> >> encapsulation. > >> >> >> > > >> >> >> > I prefer that too, except that it is impossible when users of the > >> >> >> > API need the compiler to know the struct size. > >> >> >> > >> >> >> There are cases where the structure of a data type should be > >> >> >> encapsulated, yet its size must be made known for performance (avoid > >> >> >> dynamic memory allocation and pointer chasing). > >> >> >> > >> >> >> Need for encapsulation correlates with complex algorithms and data > >> >> >> structures. The cost of dynamic allocation is often in the noise > >> >> >> then. > >> >> > > >> >> > I don't know what we are talking about anymore. None of this > >> >> > applies to the QNum API, right? > >> >> > > >> >> > QNum/QNumValue are not complex data structures, and the reason we > >> >> > need the compiler to know the size of QNumValue is not related to > >> >> > performance at all. > >> >> > >> >> We started with the question whether to make QNumValue's members > >> >> private. We digressed to the question when to make members private. > >> >> So back to the original question. > >> >> > >> >> > We might still want to discourage users of the QNum API from > >> >> > accessing QNum.u/QNumValue.u directly. Documenting the field as > >> >> > private is a very easy way to do it. > >> >> > >> >> It's a complete non-issue. QNum has been around for years, and we > >> >> haven't had any issues that could've been plausibly avoided by asking > >> >> people to refrain from accessing its members. > >> >> > >> >> If there was an actual need to keep the members private, I'd move the > >> >> struct out of the header, so the compiler enforces privacy. > >> > > >> > Understood. There's still a question I'd like to answer, to > >> > decide how the API documentation should look like: > >> > > >> > Is QNum.u/QNumValue.u required to be part of the API > >> > documentation? > >> > > >> > If accessing that field directly is not necessary for using the > >> > API, I don't think it should appear in the documentation (because > >> > it would be just noise). > >> > >> The current patch's comment on QNumValue looks good to me. > >> > >> Does this answer your question? > > > > The current patch (v3) doesn't address the question. It doesn't > > include documentation for the field, but doesn't hide it. > > kernel-doc will print a warning on that case. > > Do we care? Yes. Peter will reject pull requests if it generates kernel-doc warnings. > How many such warnings exist before the patch? Zero. > Does this series add just this one, or more? The current series (v3) doesn't add any, because I dropped the patch that added QObject and QNum documentation to docs/devel. I still want to resubmit that patch later, though. > > Use your judgement, then be ready to explain it :) OK! -- Eduardo
Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
On Wed, Nov 25, 2020 at 12:48:22PM +0100, Claudio Fontana wrote: > On 11/25/20 10:32 AM, Claudio Fontana wrote: > > On 11/24/20 9:34 PM, Eduardo Habkost wrote: > >> On Tue, Nov 24, 2020 at 08:39:33PM +0100, Claudio Fontana wrote: > >>> On 11/24/20 8:27 PM, Eduardo Habkost wrote: > >>>> On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote: > >>>> [...] > >>>>>>> +} > >>>>>> > >>>>>> Additionally, if you call arch_cpu_accel_init() here, you won't > >>>>>> need MODULE_INIT_ACCEL_CPU anymore. The > >>>>>> > >>>>>> module_call_init(MODULE_INIT_ACCEL_CPU) > >>>>>> > >>>>>> call with implicit dependencies on runtime state inside vl.c and > >>>>>> *-user/main.c becomes a trivial: > >>>>>> > >>>>>> accel_init(accel) > >>>>>> > >>>>>> call in accel_init_machine() and *-user:main(). > > > On this one I see an issue: > > the *-user_main() would still need an ac->machine_init() call to initialize > tcg itself, > currently the accelerator initialization is put into ac->machine_init > > (tcg_init, kvm_init, xen_init, etc). > > Or are you proposing to move tcg initialization away from the current > ->machine_init(), > into the new ac->init called by accel_init()? Yes. Anything that requires MachineState (and is softmmu-specific) would go to ->machine_init(). Anything that is not softmmu-specific would go to ->init(). > > This would make tcg even more different from the other accelerators. That's true, but isn't this only because TCG is the only one that really needs it? > > Or are you proposing for all accelerators to separate the initialization of > the accelerator itself > from the machine state input, leading to, for example, separating kvm-all.c > kvm_init() into two > functions, one which takes the input from MachineState and puts it into the > AccelState, and > another one which actually then initializes kvm proper? And same for all > accels? That would be possible (and maybe a good idea), but not necessary to make it work. > > In my view we could still do in *-user main.c, > > ac = ACCEL_GET_CLASS(current_accel()) > ac->machine_init(NULL); > ac->init_cpu_interfaces(ac); That would work too. I would implement it as an accel_init(NULL) call, however, to avoid duplicating the code from accel_init_machine(). Calling ->machine_init(NULL) is just a bit surprising because of the name (calling machine_init() when there's no machine), and because we know most accelerators will crash if getting a NULL argument. Anyway, the split between ->machine_init() and ->init() is just a suggestion. Keeping a single init method that accepts a NULL MachineState* as argument is not my favourite option, but it works. Whatever you choose, my only ask is to document clearly the expectations and requirements of the AccelClass methods you are using. > > to solve this, or something like that, but also the option of fixing all > accelerators to separate > the gathering of the input from the MachineState to the actual accelerator > initialization is > a possibility to me. > > Ciao, > > Claudio Thank you very much for your patience! I think we're going on the right direction. -- Eduardo
Re: [RFC v5 11/12] i386: centralize initialization of cpu accel interfaces
On Tue, Nov 24, 2020 at 09:13:13PM +0100, Paolo Bonzini wrote:
> On 24/11/20 17:22, Claudio Fontana wrote:
> > +static void x86_cpu_accel_init(void)
> > {
> > -X86CPUAccelClass *acc;
> > +const char *ac_name;
> > +ObjectClass *ac;
> > +char *xac_name;
> > +ObjectClass *xac;
> > -acc = X86_CPU_ACCEL_CLASS(object_class_by_name(accel_name));
> > -g_assert(acc != NULL);
> > +ac = object_get_class(OBJECT(current_accel()));
> > +g_assert(ac != NULL);
> > +ac_name = object_class_get_name(ac);
> > +g_assert(ac_name != NULL);
> > -object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false,
> > &acc);
> > +xac_name = g_strdup_printf("%s-%s", ac_name, TYPE_X86_CPU);
> > +xac = object_class_by_name(xac_name);
> > +g_free(xac_name);
> > +
> > +if (xac) {
> > +object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false,
> > xac);
> > +}
> > }
> > +
> > +accel_cpu_init(x86_cpu_accel_init);
>
> If this and cpus_accel_ops_init are the only call to accel_cpu_init, I'd
> rather make them functions in CPUClass (which you find and call via
> CPU_RESOLVING_TYPE) and AccelClass respectively.
Making x86_cpu_accel_init() be a CPUClass method sounds like a
good idea. This way we won't need a arch_cpu_accel_init() stub
for non-x86.
accel.c can't use cpu.h, correct? We can add a:
CPUClass *arch_base_cpu_type(void)
{
return object_class_by_name(CPU_RESOLVING_TYPE);
}
function to arch_init.c, to allow target-independent code call
target-specific code.
--
Eduardo
Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
On Tue, Nov 24, 2020 at 08:39:33PM +0100, Claudio Fontana wrote: > On 11/24/20 8:27 PM, Eduardo Habkost wrote: > > On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote: > > [...] > >>>> +} > >>> > >>> Additionally, if you call arch_cpu_accel_init() here, you won't > >>> need MODULE_INIT_ACCEL_CPU anymore. The > >>> > >>> module_call_init(MODULE_INIT_ACCEL_CPU) > >>> > >>> call with implicit dependencies on runtime state inside vl.c and > >>> *-user/main.c becomes a trivial: > >>> > >>> accel_init(accel) > >>> > >>> call in accel_init_machine() and *-user:main(). > >> > >> > >> > >> I do need a separate thing for the arch cpu accel specialization though, > >> without MODULE_INIT_ACCEL_CPU that link between all operations done at > >> accel-chosen time is missing.. > >> > > > > I think this is a key point we need to sort out. > > > > What do you mean by "link between all operations done at > > accel-chosen time" and why that's important? > > > For understanding by a reader that tries to figure this out, > (see the gid MODULE_INIT_ACCEL_CPU comment elsewhere in the thread). Right, but how does the module_call_init(MODULE_INIT_ACCEL_CPU) indirection makes this easier to figure out than just looking at a accel_init() function that makes regular function calls? > > And it could be that the high level plan to make accelerators fully > dynamically loadable plugins in the future > also conditioned me to want to have a very clear demarcation line around the > choice of the accelerator. We have dynamically loadable modules for other QOM types, already, and they just use type_init(). I don't see why an extra module_init() type makes this easier. > > > > > > accel_init_machine() has 2-3 lines of code with side effects. It > > calls AccelClass.init_machine(), which may may have hundreds of > > > could we initialize also all arch-dependent stuff in here? You can, if you use a wrapper + stub, like arch_cpu_accel_init(). > > > > lines of code. accel_setup_post() has one additional method > > call, which is triggered at a slightly different moment. > > > > You are using MODULE_INIT_ACCEL for 2 additional lines of code: > > - the cpus_register_accel() call > > - the x86_cpu_accel_init() call > > > > What makes those 2 lines of code so special, to make them deserve > > a completely new mechanism to trigger them, instead of using > > trivial function calls inside a accel_init() function? > > > > ...can we do also the x86_cpu_accel_init inside accel_init()? > > > In any case I'll try also the alternative, it would be nice if I could bring > everything together under the same roof, > and easily discoverable, both the arch-specific steps that we need to do at > accel-chosen time and the non-arch-specific steps. One way to bring everything together under the same roof is to call everything inside a accel_init() function. > > Hope this helps clarifying where I am coming from, > but I am open to have my mind changed, also trying the alternatives you > propose here could help me see first hand how things play out. Thanks! -- Eduardo
Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote: [...] > >> +} > > > > Additionally, if you call arch_cpu_accel_init() here, you won't > > need MODULE_INIT_ACCEL_CPU anymore. The > > > > module_call_init(MODULE_INIT_ACCEL_CPU) > > > > call with implicit dependencies on runtime state inside vl.c and > > *-user/main.c becomes a trivial: > > > > accel_init(accel) > > > > call in accel_init_machine() and *-user:main(). > > > > I do need a separate thing for the arch cpu accel specialization though, > without MODULE_INIT_ACCEL_CPU that link between all operations done at > accel-chosen time is missing.. > I think this is a key point we need to sort out. What do you mean by "link between all operations done at accel-chosen time" and why that's important? accel_init_machine() has 2-3 lines of code with side effects. It calls AccelClass.init_machine(), which may may have hundreds of lines of code. accel_setup_post() has one additional method call, which is triggered at a slightly different moment. You are using MODULE_INIT_ACCEL for 2 additional lines of code: - the cpus_register_accel() call - the x86_cpu_accel_init() call What makes those 2 lines of code so special, to make them deserve a completely new mechanism to trigger them, instead of using trivial function calls inside a accel_init() function? -- Eduardo
Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On Tue, Nov 24, 2020 at 07:29:50PM +0100, Claudio Fontana wrote:
> On 11/24/20 6:08 PM, Eduardo Habkost wrote:
> > On Tue, Nov 24, 2020 at 05:22:07PM +0100, Claudio Fontana wrote:
> >> apply this to the registration of the cpus accel interfaces,
> >>
> >> but this will be also in preparation for later use of this
> >> new module init step to also register per-accel x86 cpu type
> >> interfaces.
> >>
> >> Signed-off-by: Claudio Fontana
> >> ---
> > [...]
> >> diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
> >> index b4e731cb2b..482f89729f 100644
> >> --- a/accel/qtest/qtest.c
> >> +++ b/accel/qtest/qtest.c
> >> @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = {
> >>
> >> static int qtest_init_accel(MachineState *ms)
> >> {
> >> -cpus_register_accel(&qtest_cpus);
> >> return 0;
> >> }
> >>
> >> @@ -58,3 +57,12 @@ static void qtest_type_init(void)
> >> }
> >>
> >> type_init(qtest_type_init);
> >> +
> >> +static void qtest_accel_cpu_init(void)
> >> +{
> >> +if (qtest_enabled()) {
> >> +cpus_register_accel(&qtest_cpus);
> >> +}
> >> +}
> >> +
> >> +accel_cpu_init(qtest_accel_cpu_init);
> >
> > I don't understand why this (and the similar changes on other
> > accelerators) is an improvement.
> >
> > You are replacing a trivial AccelClass-specific init method with
> > a module_init() function that has a hidden dependency on runtime
> > state.
> >
>
> Not a big advantage I agree,
> I think however there is one, in using the existing framework that exists,
> for the purposes that it was built for.
>
> As I understand it, the global module init framework is supposed to mark the
> major initialization steps,
> and this seems to fit the bill.
That seems to be the main source of disagreement. I don't agree
that's the purpose of module_init().
The module init framework is used to unconditionally register
module-provided entities like option names, QOM types, block
drivers, trace events, etc. The entities registered by module
init functions represent a passive dynamically loadable piece of
code.
module_init() was never used for initialization of machines,
devices, CPUs, or other runtime state. We don't have
MODULE_INIT_MONITOR, MODULE_INIT_OBJECTS, MODULE_INIT_MACHINE,
MODULE_INIT_CPUS, MODULE_INIT_DEVICES, etc.
And I'm not convinced we should, because it would hide
dependencies between initialization steps. It would force us to
make initialization functions affect and depend on global state.
I believe this:
int main()
{
result_of_A = init_A(input_for_A);
result_of_B = init_B(input_for_B);
result_of_C = init_C(input_for_C);
}
is clearer and more maintainable than:
int main()
{
module_init_call(MODULE_INIT_A); /* result_of_A hidden in global state */
module_init_call(MODULE_INIT_B); /* result_of_B hidden in global state */
module_init_call(MODULE_INIT_C); /* result_of_C hidden in global state */
}
>
> The "hidden" dependency on the fact that accels need to be initialized at
> that time, is not hidden at all I think,
> it is what this module init step is all about.
>
> It is explicitly meaning, "_now that the current accelerator is chosen_,
> perform these initializations".
>
> But, as you mentioned elsewhere, I will in the meantime anyway squash these
> things so they do not start fragmented at all, and centralize immediately.
Agreed. We still need to sort out the disagreement above, or
we'll spend a lot of energy arguing about code.
--
Eduardo
Re: [RFC v5 08/12] accel: extend AccelState and AccelClass to user-mode
On Tue, Nov 24, 2020 at 05:22:06PM +0100, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana
> ---
[...]
> @@ -908,8 +909,12 @@ int main(int argc, char **argv)
> }
>
> /* init tcg before creating CPUs and to get qemu_host_page_size */
> -tcg_exec_init(0);
> +{
> +AccelClass *ac = accel_find("tcg");
>
> +g_assert(ac != NULL);
> +ac->init_machine(NULL);
Most init_machine() methods will crash if you call them with a
NULL argument.
This looks like another reason for having a
void accel_init(AccelState*)
function and a
void (*init)(AccelState*)
method in AccelClass.
Then the whole code block above would be as trivial as:
accel_init(current_accel());
> +}
[...]
>
--
Eduardo
Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps
On Tue, Nov 24, 2020 at 05:22:10PM +0100, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana
> ---
> accel/kvm/kvm-all.c | 9 ---
> accel/kvm/kvm-cpus.c | 26 +-
> accel/kvm/kvm-cpus.h | 2 --
> accel/qtest/qtest.c | 31 --
> accel/tcg/tcg-cpus-icount.c | 11 +---
> accel/tcg/tcg-cpus-icount.h | 2 ++
> accel/tcg/tcg-cpus-mttcg.c | 12 +++--
> accel/tcg/tcg-cpus-mttcg.h | 19 ++
> accel/tcg/tcg-cpus-rr.c | 7 -
> accel/tcg/tcg-cpus.c | 48 ++---
> accel/tcg/tcg-cpus.h | 4 ---
> accel/xen/xen-all.c | 29 ++--
> include/sysemu/cpus.h| 39 ---
> softmmu/cpus.c | 51 +---
> target/i386/hax/hax-all.c| 9 ---
> target/i386/hax/hax-cpus.c | 29 +++-
> target/i386/hax/hax-cpus.h | 2 --
> target/i386/hvf/hvf-cpus.c | 27 ++-
> target/i386/hvf/hvf-cpus.h | 2 --
> target/i386/hvf/hvf.c| 9 ---
> target/i386/whpx/whpx-all.c | 9 ---
> target/i386/whpx/whpx-cpus.c | 29 +++-
> target/i386/whpx/whpx-cpus.h | 2 --
> 23 files changed, 251 insertions(+), 157 deletions(-)
> create mode 100644 accel/tcg/tcg-cpus-mttcg.h
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 509b249f52..33156cc4c7 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3234,12 +3234,3 @@ static void kvm_type_init(void)
> }
>
> type_init(kvm_type_init);
> -
> -static void kvm_accel_cpu_init(void)
> -{
> -if (kvm_enabled()) {
> -cpus_register_accel(&kvm_cpus);
> -}
> -}
> -
> -accel_cpu_init(kvm_accel_cpu_init);
> diff --git a/accel/kvm/kvm-cpus.c b/accel/kvm/kvm-cpus.c
> index d809b1e74c..33dc8e737a 100644
> --- a/accel/kvm/kvm-cpus.c
> +++ b/accel/kvm/kvm-cpus.c
> @@ -74,11 +74,25 @@ static void kvm_start_vcpu_thread(CPUState *cpu)
> cpu, QEMU_THREAD_JOINABLE);
> }
>
> -const CpusAccel kvm_cpus = {
> -.create_vcpu_thread = kvm_start_vcpu_thread,
> +static void kvm_cpus_class_init(ObjectClass *oc, void *data)
> +{
> +CpusAccelOps *ops = CPUS_ACCEL_OPS_CLASS(oc);
Why do you need a separate QOM type hierarchy instead of just
doing this inside AccelClass methods and/or existing accel
class_init functions?
>
> -.synchronize_post_reset = kvm_cpu_synchronize_post_reset,
> -.synchronize_post_init = kvm_cpu_synchronize_post_init,
> -.synchronize_state = kvm_cpu_synchronize_state,
> -.synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm,
> +ops->create_vcpu_thread = kvm_start_vcpu_thread;
> +ops->synchronize_post_reset = kvm_cpu_synchronize_post_reset;
> +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;
All of these could be AccelClass fields.
TCG makes it a bit more complicated because there's a different
set of methods chosen for TYPE_TCG_ACCEL depending on the
configuration. This could be solved by patching AccelClass at
init time, or by moving the method pointers to AccelState.
Alternatively, if you still want to keep the
CpusAccel/CpusAccelOps struct, that's OK. You can just add a
`CpusAccel *cpu_accel_ops` field to AccelClass or AccelState. No
need for a separate QOM hierarchy, either.
If you _really_ want a separate TYPE_CPU_ACCEL_OPS QOM type, you
can still have it. But it can be just an interface implemented
by each accel subclass, instead of requiring a separate
CPUS_ACCEL_TYPE_NAME(...) type to be registered for each
accelerator. (I don't see why you would want it, though.)
> };
> +static const TypeInfo kvm_cpus_type_info = {
> +.name = CPUS_ACCEL_TYPE_NAME("kvm"),
> +
> +.parent = TYPE_CPUS_ACCEL_OPS,
> +.class_init = kvm_cpus_class_init,
> +.abstract = true,
> +};
> +static void kvm_cpus_register_types(void)
> +{
> +type_register_static(&kvm_cpus_type_info);
> +}
> +type_init(kvm_cpus_register_types);
[...]
> -typedef struct CpusAccel {
> -void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY */
> +typedef struct CpusAccelOps CpusAccelOps;
> +DECLARE_CLASS_CHECKERS(CpusAccelOps, CPUS_ACCEL_OPS, TYPE_CPUS_ACCEL_OPS)
> +
> +struct CpusAccelOps {
> +ObjectClass parent_class;
> +
> +/* initialization function called when accel is chosen */
> +void (*accel_chosen_init)(CpusAccelOps *ops);
This can be an AccelClass method too. What about just naming it
AccelClass.init?
> +
> +void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY NON-NULL */
> void (*kick_vcpu_thread)(CPUState *cpu);
>
> void (*synchronize_post_reset)(CPUState *cpu);
> @@ -20,13 +45,7 @@ typedef struct CpusAccel {
>
> int64_t (*get_virtual_clock)(void);
> int64_t (*get_elapsed_ticks)(void);
> -}
Re: [RFC v5 09/12] module: introduce MODULE_INIT_ACCEL_CPU
On Tue, Nov 24, 2020 at 05:22:07PM +0100, Claudio Fontana wrote:
> apply this to the registration of the cpus accel interfaces,
>
> but this will be also in preparation for later use of this
> new module init step to also register per-accel x86 cpu type
> interfaces.
>
> Signed-off-by: Claudio Fontana
> ---
[...]
> diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
> index b4e731cb2b..482f89729f 100644
> --- a/accel/qtest/qtest.c
> +++ b/accel/qtest/qtest.c
> @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = {
>
> static int qtest_init_accel(MachineState *ms)
> {
> -cpus_register_accel(&qtest_cpus);
> return 0;
> }
>
> @@ -58,3 +57,12 @@ static void qtest_type_init(void)
> }
>
> type_init(qtest_type_init);
> +
> +static void qtest_accel_cpu_init(void)
> +{
> +if (qtest_enabled()) {
> +cpus_register_accel(&qtest_cpus);
> +}
> +}
> +
> +accel_cpu_init(qtest_accel_cpu_init);
I don't understand why this (and the similar changes on other
accelerators) is an improvement.
You are replacing a trivial AccelClass-specific init method with
a module_init() function that has a hidden dependency on runtime
state.
--
Eduardo
Re: [RFC v5 11/12] i386: centralize initialization of cpu accel interfaces
On Tue, Nov 24, 2020 at 05:22:09PM +0100, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana
Probably this can be squashed into patch 10/12.
> ---
> target/i386/cpu-qom.h | 2 --
> target/i386/cpu.c | 27 ---
> target/i386/hvf/cpu.c | 9 -
> target/i386/kvm/cpu.c | 8
> target/i386/tcg/cpu.c | 9 -
> 5 files changed, 20 insertions(+), 35 deletions(-)
>
> diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h
> index 9316e78e71..2cea5394c6 100644
> --- a/target/i386/cpu-qom.h
> +++ b/target/i386/cpu-qom.h
> @@ -98,6 +98,4 @@ struct X86CPUAccelClass {
> void (*cpu_realizefn)(X86CPU *cpu, Error **errp);
> };
>
> -void x86_cpu_accel_init(const char *accel_name);
> -
> #endif
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b799723e53..f6fd055046 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7066,18 +7066,31 @@ type_init(x86_cpu_register_types)
> static void x86_cpu_accel_init_aux(ObjectClass *klass, void *opaque)
> {
> X86CPUClass *xcc = X86_CPU_CLASS(klass);
> -const X86CPUAccelClass **accel = opaque;
> +X86CPUAccelClass *accel = opaque;
>
> -xcc->accel = *accel;
> +xcc->accel = accel;
> xcc->accel->cpu_common_class_init(xcc);
> }
>
> -void x86_cpu_accel_init(const char *accel_name)
> +static void x86_cpu_accel_init(void)
> {
> -X86CPUAccelClass *acc;
> +const char *ac_name;
> +ObjectClass *ac;
> +char *xac_name;
> +ObjectClass *xac;
>
> -acc = X86_CPU_ACCEL_CLASS(object_class_by_name(accel_name));
> -g_assert(acc != NULL);
> +ac = object_get_class(OBJECT(current_accel()));
> +g_assert(ac != NULL);
> +ac_name = object_class_get_name(ac);
> +g_assert(ac_name != NULL);
>
> -object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false, &acc);
> +xac_name = g_strdup_printf("%s-%s", ac_name, TYPE_X86_CPU);
> +xac = object_class_by_name(xac_name);
> +g_free(xac_name);
> +
> +if (xac) {
> +object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false,
> xac);
> +}
> }
> +
> +accel_cpu_init(x86_cpu_accel_init);
This keeps the hidden initialization ordering dependency between
MODULE_INIT_ACCEL_CPU and current_accel(). I thought we were
going to get rid of module init functions that depend on runtime
state.
This is an improvement to the code in patch 10/12, though. If
others believe it is an acceptable (temporary) solution, I won't
block it.
I would still prefer to have a
void arch_accel_cpu_init(AccelState*)
function which would call a
void x86_cpu_accel_init(AccelState*)
function. That would make the dependency between
x86_cpu_accel_init() and accelerator creation explicit.
> diff --git a/target/i386/hvf/cpu.c b/target/i386/hvf/cpu.c
> index 7e7dc044d3..70b6dbfc10 100644
> --- a/target/i386/hvf/cpu.c
> +++ b/target/i386/hvf/cpu.c
> @@ -65,12 +65,3 @@ static void hvf_cpu_accel_register_types(void)
> type_register_static(&hvf_cpu_accel_type_info);
> }
> type_init(hvf_cpu_accel_register_types);
> -
> -static void hvf_cpu_accel_init(void)
> -{
> -if (hvf_enabled()) {
> -x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("hvf"));
> -}
> -}
> -
> -accel_cpu_init(hvf_cpu_accel_init);
> diff --git a/target/i386/kvm/cpu.c b/target/i386/kvm/cpu.c
> index bc5f519479..c17ed5a3f2 100644
> --- a/target/i386/kvm/cpu.c
> +++ b/target/i386/kvm/cpu.c
> @@ -147,11 +147,3 @@ static void kvm_cpu_accel_register_types(void)
> type_register_static(&kvm_cpu_accel_type_info);
> }
> type_init(kvm_cpu_accel_register_types);
> -
> -static void kvm_cpu_accel_init(void)
> -{
> -if (kvm_enabled()) {
> -x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("kvm"));
> -}
> -}
> -accel_cpu_init(kvm_cpu_accel_init);
> diff --git a/target/i386/tcg/cpu.c b/target/i386/tcg/cpu.c
> index e7d4effdd0..00166c36e9 100644
> --- a/target/i386/tcg/cpu.c
> +++ b/target/i386/tcg/cpu.c
> @@ -170,12 +170,3 @@ static void tcg_cpu_accel_register_types(void)
> type_register_static(&tcg_cpu_accel_type_info);
> }
> type_init(tcg_cpu_accel_register_types);
> -
> -static void tcg_cpu_accel_init(void)
> -{
> -if (tcg_enabled()) {
> -x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("tcg"));
> -}
> -}
> -
> -accel_cpu_init(tcg_cpu_accel_init);
> --
> 2.26.2
>
--
Eduardo
Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
On Tue, Nov 24, 2020 at 04:20:37PM +0100, Markus Armbruster wrote: > Eduardo Habkost writes: > > > On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote: > >> Eduardo Habkost writes: > >> > >> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote: > >> >> Eduardo Habkost writes: > >> >> > >> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote: > >> >> [...] > >> >> >> When the structure of a data type is to be kept away from its users, > >> >> >> I > >> >> >> prefer to keep it out of the public header, so the compiler enforces > >> >> >> the > >> >> >> encapsulation. > >> >> > > >> >> > I prefer that too, except that it is impossible when users of the > >> >> > API need the compiler to know the struct size. > >> >> > >> >> There are cases where the structure of a data type should be > >> >> encapsulated, yet its size must be made known for performance (avoid > >> >> dynamic memory allocation and pointer chasing). > >> >> > >> >> Need for encapsulation correlates with complex algorithms and data > >> >> structures. The cost of dynamic allocation is often in the noise then. > >> > > >> > I don't know what we are talking about anymore. None of this > >> > applies to the QNum API, right? > >> > > >> > QNum/QNumValue are not complex data structures, and the reason we > >> > need the compiler to know the size of QNumValue is not related to > >> > performance at all. > >> > >> We started with the question whether to make QNumValue's members > >> private. We digressed to the question when to make members private. > >> So back to the original question. > >> > >> > We might still want to discourage users of the QNum API from > >> > accessing QNum.u/QNumValue.u directly. Documenting the field as > >> > private is a very easy way to do it. > >> > >> It's a complete non-issue. QNum has been around for years, and we > >> haven't had any issues that could've been plausibly avoided by asking > >> people to refrain from accessing its members. > >> > >> If there was an actual need to keep the members private, I'd move the > >> struct out of the header, so the compiler enforces privacy. > > > > Understood. There's still a question I'd like to answer, to > > decide how the API documentation should look like: > > > > Is QNum.u/QNumValue.u required to be part of the API > > documentation? > > > > If accessing that field directly is not necessary for using the > > API, I don't think it should appear in the documentation (because > > it would be just noise). > > The current patch's comment on QNumValue looks good to me. > > Does this answer your question? The current patch (v3) doesn't address the question. It doesn't include documentation for the field, but doesn't hide it. kernel-doc will print a warning on that case. -- Eduardo
[PULL 1/1] Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"
From: "Michael S. Tsirkin"
This reverts commit bccb20c49df1bd683248a366021973901c11982f as it
introduced a regression blocking bus addresses > 0x1f or higher.
Legal bus numbers go up to 0xff.
Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
Reported-by: Klaus Herman
Reported-by: Geoffrey McRae
Signed-off-by: Michael S. Tsirkin
Reviewed-by: Philippe Mathieu-Daudé
Cc: "Philippe Mathieu-Daudé"
Message-Id: <20201120130409.956956-1-...@redhat.com>
Signed-off-by: Eduardo Habkost
---
hw/core/qdev-properties-system.c | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index b81a4e8d14..9d80a07d26 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -858,7 +858,7 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v,
const char *name,
Property *prop = opaque;
PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
char *str, *p;
-const char *e;
+char *e;
unsigned long val;
unsigned long dom = 0, bus = 0;
unsigned int slot = 0, func = 0;
@@ -873,23 +873,23 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v,
const char *name,
}
p = str;
-if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x || e == p) {
-goto inval;
-}
-if (*e != ':') {
+val = strtoul(p, &e, 16);
+if (e == p || *e != ':') {
goto inval;
}
bus = val;
-p = (char *)e + 1;
-if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {
+p = e + 1;
+val = strtoul(p, &e, 16);
+if (e == p) {
goto inval;
}
if (*e == ':') {
dom = bus;
bus = val;
-p = (char *)e + 1;
-if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {
+p = e + 1;
+val = strtoul(p, &e, 16);
+if (e == p) {
goto inval;
}
}
@@ -898,13 +898,14 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v,
const char *name,
if (*e != '.') {
goto inval;
}
-p = (char *)e + 1;
-if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {
+p = e + 1;
+val = strtoul(p, &e, 10);
+if (e == p) {
goto inval;
}
func = val;
-if (bus > 0xff) {
+if (dom > 0x || bus > 0xff || slot > 0x1f || func > 7) {
goto inval;
}
--
2.28.0
[PULL 0/1] PCI host devaddr property fix for 5.2
The following changes since commit d536d9578ec3ac5029a70b8126cb84bb6f2124a4: Merge remote-tracking branch 'remotes/jasowang/tags/net-pull-request' into staging (2020-11-24 10:59:12 +) are available in the Git repository at: git://github.com/ehabkost/qemu.git tags/machine-next-for-5.2-pull-request for you to fetch changes up to 28afbc1f11f5ae33b69deb162a551110717eec94: Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()" (2020-11-24 10:06:54 -0500) PCI host devaddr property fix for 5.2 Michael S. Tsirkin (1): Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()" hw/core/qdev-properties-system.c | 25 + 1 file changed, 13 insertions(+), 12 deletions(-) -- 2.28.0
Re: [PATCH for-5.2] Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"
On Tue, Nov 24, 2020 at 03:13:14PM +0100, Philippe Mathieu-Daudé wrote:
> On 11/20/20 2:10 PM, Paolo Bonzini wrote:
> > On 20/11/20 14:04, Michael S. Tsirkin wrote:
> >> This reverts commit bccb20c49df1bd683248a366021973901c11982f as it
> >> introduced a regression blocking bus addresses > 0x1f or higher.
> >> Legal bus numbers go up to 0xff.
> >>
> >> Cc: "Philippe Mathieu-Daudé"
> >> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
> >> Reported-by: Klaus Herman
> >> Reported-by: Geoffrey McRae
> >> Signed-off-by: Michael S. Tsirkin
> >> ---
> >>
> >> checkpatch will complain since it does not like strtoul but
> >> we had it for years so should be ok for 5.2, right?
> >
> > Yes, of course.
> >
> > Paolo
>
> Which tree is going to merge this patch?
Sorry, I was expecting Michael to merge it, as it's PCI-specific
but it looks like people were waiting for me to merge it.
I'll merge it and submit a pull request ASAP.
--
Eduardo
Re: [PATCH v3 10/19] qlit: Support all types of QNums
On Tue, Nov 24, 2020 at 01:22:02PM +0100, Markus Armbruster wrote: > Paolo Bonzini writes: > > > On 24/11/20 10:55, Markus Armbruster wrote: > >>> +/* Larger than UINT64_MAX: */ > >>> +QLIT_QNUM_DOUBLE(18446744073709552e3), > >>> +/* Smaller than INT64_MIN: */ > >>> +QLIT_QNUM_DOUBLE(-92233720368547758e2), > >> Why "larger than UINT64_MAX" and "smaller than INT64_MIN"? > >> > > > > I guess the point is to test values that are only representable as a > > double, so (double)((uint64_t)INT64_MAX+1) wouldn't be very useful for > > that: as the expression shows, it would not be a QNUM_VAL_INT but it > > would be representable as QNUM_VAL_UINT. > > > > So these are the cases that matter the most, even though -1, 0 and > > INT64_MAX+1 could be nice to have. > > qnum_is_equal()'s contract: > > * Doubles are never considered equal to integers. If that's part of the contract, it would be OK to include 0.0, -1.0, 1.0, INT64_MAX+1 in the list. I incorrectly assumed qnum_is_equal(qnum_from_int(0), qnum_from_double(0.0)) was undefined. However, if we really care about test coverage of qnum_is_equal(), we probably should be extending check-qnum.c, not check-qlit.c. -- Eduardo
Re: [PATCH v3 08/19] qlit: Move qlit_equal_qobject() reference values to array
On Tue, Nov 24, 2020 at 10:51:34AM +0100, Markus Armbruster wrote:
> Eduardo Habkost writes:
>
> > Add an array of values to qlit_equal_qobject_test(), so we can
> > extend the test case to compare multiple literals, not just the
> > ones at the existing `qlit` and `qlit_foo` variables.
> >
> > Signed-off-by: Eduardo Habkost
> > ---
> > This is a new patch added in v3 of this series.
> > ---
> > tests/check-qlit.c | 26 +++---
> > 1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/tests/check-qlit.c b/tests/check-qlit.c
> > index 24ac21395c..b1cfbddb17 100644
> > --- a/tests/check-qlit.c
> > +++ b/tests/check-qlit.c
> > @@ -29,11 +29,6 @@ static QLitObject qlit = QLIT_QDICT(((QLitDictEntry[]) {
> > { },
> > }));
> >
> > -static QLitObject qlit_foo = QLIT_QDICT(((QLitDictEntry[]) {
> > -{ "foo", QLIT_QNUM_INT(42) },
> > -{ },
> > -}));
> > -
> > static QObject *make_qobject(void)
> > {
> > QDict *qdict = qdict_new();
> > @@ -53,16 +48,33 @@ static QObject *make_qobject(void)
> >
> > static void qlit_equal_qobject_test(void)
> > {
> > +/* Each entry in the values[] array should be different from the
> > others */
> > +QLitObject values[] = {
> > +qlit,
> > +QLIT_QDICT(((QLitDictEntry[]) {
> > +{ "foo", QLIT_QNUM_INT(42) },
> > +{ },
> > +})),
> > +};
> > +int i;
> > QObject *qobj = make_qobject();
> >
> > g_assert(qlit_equal_qobject(&qlit, qobj));
> >
> > -g_assert(!qlit_equal_qobject(&qlit_foo, qobj));
> > -
> > qdict_put(qobject_to(QDict, qobj), "bee", qlist_new());
> > g_assert(!qlit_equal_qobject(&qlit, qobj));
> >
> > qobject_unref(qobj);
> > +
> > +for (i = 0; i < ARRAY_SIZE(values); i++) {
> > +int j;
>
> I'd prefer to declare this one together with @i.
>
> > +QObject *o = qobject_from_qlit(&values[i]);
>
> Blank line, if you don't mind.
I will surely do it if there's a v4, but I hope you don't make me
submit v4 just to change these.
>
> > +for (j = 0; j < ARRAY_SIZE(values); j++) {
> > +g_assert(qlit_equal_qobject(&values[j], o) == (i == j));
> > +}
> > +qobject_unref(o);
> > +}
> > +
> > }
> >
> > static void qlit_equal_large_qnum_test(void)
--
Eduardo
Re: [PATCH v3 11/19] qom: field_prop_set_default_value() helper
On Tue, Nov 24, 2020 at 10:58:27AM +0100, Markus Armbruster wrote:
> Eduardo Habkost writes:
>
> > Move code that sets the property default value to a separate
> > function, to reduce duplication and make refactoring easier.
> >
> > Signed-off-by: Eduardo Habkost
> > ---
> > This is a new patch added in v3 of this series.
> > Hopefully, this will make the series easier to review.
> >
> > The field_prop_set_default_value() was added in v2 at
> > "qom: Use qlit to represent property defaults".
> > ---
> > qom/field-property.c | 24
> > 1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/qom/field-property.c b/qom/field-property.c
> > index cb729626ce..6a0df7c6ea 100644
> > --- a/qom/field-property.c
> > +++ b/qom/field-property.c
> > @@ -62,6 +62,17 @@ static void static_prop_release_dynamic_prop(Object
> > *obj, const char *name,
> > g_free(prop);
> > }
> >
> > +static void field_prop_set_default_value(ObjectProperty *op,
> > + Property *prop)
> > +{
> > +if (!prop->set_default) {
> > +return;
> > +}
> > +
> > +assert(prop->info->set_default_value);
> > +prop->info->set_default_value(op, prop);
> > +}
> > +
> > ObjectProperty *
> > object_property_add_field(Object *obj, const char *name,
> >Property *prop,
> > @@ -83,11 +94,9 @@ object_property_add_field(Object *obj, const char *name,
> > object_property_set_description(obj, name,
> > newprop->info->description);
> >
> > -if (newprop->set_default) {
> > -newprop->info->set_default_value(op, newprop);
> > -if (op->init) {
> > -op->init(obj, op);
> > -}
> > +field_prop_set_default_value(op, newprop);
> > +if (op->init) {
> > +op->init(obj, op);
> > }
>
> op->init() is now called even when !newprop->set_default. Why is that
> okay?
op->init() will be NULL if object_property_set_default() was not
called.
It's subtle, and worth mentioning in the commit message. I think
we should encapsulate op->init() behind a
object_property_reset_to_default_value() function.
>
> >
> > op->allow_set = allow_set;
> > @@ -113,9 +122,8 @@ object_class_property_add_field_static(ObjectClass *oc,
> > const char *name,
> > prop->info->release,
> > prop);
> > }
> > -if (prop->set_default) {
> > -prop->info->set_default_value(op, prop);
> > -}
> > +
> > +field_prop_set_default_value(op, prop);
> > if (prop->info->description) {
> > object_class_property_set_description(oc, name,
> >prop->info->description);
--
Eduardo
Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote: > Eduardo Habkost writes: > > > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote: > >> Eduardo Habkost writes: > >> > >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote: > >> [...] > >> >> When the structure of a data type is to be kept away from its users, I > >> >> prefer to keep it out of the public header, so the compiler enforces the > >> >> encapsulation. > >> > > >> > I prefer that too, except that it is impossible when users of the > >> > API need the compiler to know the struct size. > >> > >> There are cases where the structure of a data type should be > >> encapsulated, yet its size must be made known for performance (avoid > >> dynamic memory allocation and pointer chasing). > >> > >> Need for encapsulation correlates with complex algorithms and data > >> structures. The cost of dynamic allocation is often in the noise then. > > > > I don't know what we are talking about anymore. None of this > > applies to the QNum API, right? > > > > QNum/QNumValue are not complex data structures, and the reason we > > need the compiler to know the size of QNumValue is not related to > > performance at all. > > We started with the question whether to make QNumValue's members > private. We digressed to the question when to make members private. > So back to the original question. > > > We might still want to discourage users of the QNum API from > > accessing QNum.u/QNumValue.u directly. Documenting the field as > > private is a very easy way to do it. > > It's a complete non-issue. QNum has been around for years, and we > haven't had any issues that could've been plausibly avoided by asking > people to refrain from accessing its members. > > If there was an actual need to keep the members private, I'd move the > struct out of the header, so the compiler enforces privacy. Understood. There's still a question I'd like to answer, to decide how the API documentation should look like: Is QNum.u/QNumValue.u required to be part of the API documentation? If accessing that field directly is not necessary for using the API, I don't think it should appear in the documentation (because it would be just noise). -- Eduardo
[PATCH v3 16/19] qom: Make PropertyInfo.set_default_value optional
If .set_default_value is not set, call
object_property_set_default(). This will let us delete most of
the .set_default_value functions later.
Signed-off-by: Eduardo Habkost
---
This is a new patch in v3 of this series.
In v2 of the series, equivalent functionality was implemented by
"qom: Use qlit to represent property defaults".
---
include/qom/field-property.h | 3 +++
qom/field-property.c | 12 ++--
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/qom/field-property.h b/include/qom/field-property.h
index 951cec2fb0..00b83ee9ba 100644
--- a/include/qom/field-property.h
+++ b/include/qom/field-property.h
@@ -53,6 +53,9 @@ struct PropertyInfo {
* @set_default_value: Callback for initializing the default value
*
* @defval is a weak reference.
+ *
+ * Optional. If not set and Property.defval is not QTYPE_NONE,
+ * object_property_set_default() will be called.
*/
void (*set_default_value)(ObjectProperty *op, const Property *prop,
const QObject *defval);
diff --git a/qom/field-property.c b/qom/field-property.c
index 593ffb53e9..d21ff98862 100644
--- a/qom/field-property.c
+++ b/qom/field-property.c
@@ -72,8 +72,16 @@ static void field_prop_set_default_value(ObjectProperty *op,
}
defval = qobject_from_qlit(&prop->defval);
-assert(prop->info->set_default_value);
-prop->info->set_default_value(op, prop, defval);
+if (prop->info->set_default_value) {
+/* .set_default_value() gets a weak reference */
+prop->info->set_default_value(op, prop, defval);
+} else {
+/*
+ * object_property_set_default() takes ownership,
+ * so qobject_ref() is needed.
+ */
+object_property_set_default(op, qobject_ref(defval));
+}
qobject_unref(defval);
}
--
2.28.0
[PATCH v3 15/19] qom: Make object_property_set_default() public
The function will be used outside qom/object.c, to simplify the
field property code that sets the property default value.
Reviewed-by: Marc-André Lureau
Signed-off-by: Eduardo Habkost
---
include/qom/object.h | 11 +++
qom/object.c | 2 +-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/include/qom/object.h b/include/qom/object.h
index 2ab124b8f0..4234cc9b66 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1090,6 +1090,17 @@ ObjectProperty *object_class_property_add(ObjectClass
*klass, const char *name,
ObjectPropertyRelease *release,
void *opaque);
+/**
+ * object_property_set_default:
+ * @prop: the property to set
+ * @value: the value to be written to the property
+ *
+ * Set the property default value.
+ *
+ * Ownership of @value is transferred to the property.
+ */
+void object_property_set_default(ObjectProperty *prop, QObject *value);
+
/**
* object_property_set_default_bool:
* @prop: the property to set
diff --git a/qom/object.c b/qom/object.c
index 7c11bcd3b1..6b0d9d8c79 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1547,7 +1547,7 @@ static void object_property_init_defval(Object *obj,
ObjectProperty *prop)
visit_free(v);
}
-static void object_property_set_default(ObjectProperty *prop, QObject *defval)
+void object_property_set_default(ObjectProperty *prop, QObject *defval)
{
assert(!prop->defval);
assert(!prop->init);
--
2.28.0
[PATCH v3 13/19] qom: Fix documentation of UUID property type
On some cases, the documentation for UUID properties is lying:
properties defined using DEFINE_PROP_UUID_NODEFAULT are not set
to "auto" by default. It's better to omit this instead of
providing incorrect information.
Signed-off-by: Eduardo Habkost
---
This is a new patch addeed in v3 of this series.
---
hw/core/qdev-properties-system.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 217e041152..6071f672a4 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -1089,7 +1089,7 @@ static void set_default_uuid_auto(ObjectProperty *op,
const Property *prop,
const PropertyInfo qdev_prop_uuid = {
.name = "str",
.description = "UUID (aka GUID) or \"" UUID_VALUE_AUTO
-"\" for random value (default)",
+"\" for random value",
.get = get_uuid,
.set = set_uuid,
.set_default_value = set_default_uuid_auto,
--
2.28.0
[PATCH v3 11/19] qom: field_prop_set_default_value() helper
Move code that sets the property default value to a separate
function, to reduce duplication and make refactoring easier.
Signed-off-by: Eduardo Habkost
---
This is a new patch added in v3 of this series.
Hopefully, this will make the series easier to review.
The field_prop_set_default_value() was added in v2 at
"qom: Use qlit to represent property defaults".
---
qom/field-property.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/qom/field-property.c b/qom/field-property.c
index cb729626ce..6a0df7c6ea 100644
--- a/qom/field-property.c
+++ b/qom/field-property.c
@@ -62,6 +62,17 @@ static void static_prop_release_dynamic_prop(Object *obj,
const char *name,
g_free(prop);
}
+static void field_prop_set_default_value(ObjectProperty *op,
+ Property *prop)
+{
+if (!prop->set_default) {
+return;
+}
+
+assert(prop->info->set_default_value);
+prop->info->set_default_value(op, prop);
+}
+
ObjectProperty *
object_property_add_field(Object *obj, const char *name,
Property *prop,
@@ -83,11 +94,9 @@ object_property_add_field(Object *obj, const char *name,
object_property_set_description(obj, name,
newprop->info->description);
-if (newprop->set_default) {
-newprop->info->set_default_value(op, newprop);
-if (op->init) {
-op->init(obj, op);
-}
+field_prop_set_default_value(op, newprop);
+if (op->init) {
+op->init(obj, op);
}
op->allow_set = allow_set;
@@ -113,9 +122,8 @@ object_class_property_add_field_static(ObjectClass *oc,
const char *name,
prop->info->release,
prop);
}
-if (prop->set_default) {
-prop->info->set_default_value(op, prop);
-}
+
+field_prop_set_default_value(op, prop);
if (prop->info->description) {
object_class_property_set_description(oc, name,
prop->info->description);
--
2.28.0
[PATCH v3 17/19] qom: Delete field_prop_set_default_value_*int()
The field_prop_set_default_value_*int() functions can be
completely replaced by object_propert_set_default(),
we don't need them anymore.
Signed-off-by: Eduardo Habkost
---
This is a new patch in v3 of this series.
In v2 of the series, equivalent changes were part of "qom: Use
qlit to represent property defaults".
---
include/qom/field-property-internal.h | 6 --
hw/core/qdev-properties-system.c | 2 --
qom/property-types.c | 25 -
3 files changed, 33 deletions(-)
diff --git a/include/qom/field-property-internal.h
b/include/qom/field-property-internal.h
index 7ed0d8d160..4bcf5b45f3 100644
--- a/include/qom/field-property-internal.h
+++ b/include/qom/field-property-internal.h
@@ -16,12 +16,6 @@ void field_prop_set_enum(Object *obj, Visitor *v, const char
*name,
void field_prop_set_default_value_enum(ObjectProperty *op,
const Property *prop,
const QObject *defval);
-void field_prop_set_default_value_int(ObjectProperty *op,
- const Property *prop,
- const QObject *defval);
-void field_prop_set_default_value_uint(ObjectProperty *op,
- const Property *prop,
- const QObject *defval);
void field_prop_get_int32(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp);
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 117c540254..b2df955f2a 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -571,7 +571,6 @@ const PropertyInfo qdev_prop_blocksize = {
" and " MAX_BLOCK_SIZE_STR,
.get = field_prop_get_size32,
.set = set_blocksize,
-.set_default_value = field_prop_set_default_value_uint,
};
/* --- Block device error handling policy --- */
@@ -769,7 +768,6 @@ const PropertyInfo qdev_prop_pci_devfn = {
.print = print_pci_devfn,
.get = field_prop_get_int32,
.set = set_pci_devfn,
-.set_default_value = field_prop_set_default_value_int,
};
/* --- pci host address --- */
diff --git a/qom/property-types.c b/qom/property-types.c
index 0fc24b3687..399e36c29e 100644
--- a/qom/property-types.c
+++ b/qom/property-types.c
@@ -195,27 +195,10 @@ static void set_uint8(Object *obj, Visitor *v, const char
*name, void *opaque,
visit_type_uint8(v, name, ptr, errp);
}
-void field_prop_set_default_value_int(ObjectProperty *op,
- const Property *prop,
- const QObject *defval)
-{
-QNum *qn = qobject_to(QNum, defval);
-object_property_set_default_int(op, qnum_get_int(qn));
-}
-
-void field_prop_set_default_value_uint(ObjectProperty *op,
- const Property *prop,
- const QObject *defval)
-{
-QNum *qn = qobject_to(QNum, defval);
-object_property_set_default_uint(op, qnum_get_uint(qn));
-}
-
const PropertyInfo prop_info_uint8 = {
.name = "uint8",
.get = get_uint8,
.set = set_uint8,
-.set_default_value = field_prop_set_default_value_uint,
};
/* --- 16bit integer --- */
@@ -242,7 +225,6 @@ const PropertyInfo prop_info_uint16 = {
.name = "uint16",
.get = get_uint16,
.set = set_uint16,
-.set_default_value = field_prop_set_default_value_uint,
};
/* --- 32bit integer --- */
@@ -287,14 +269,12 @@ const PropertyInfo prop_info_uint32 = {
.name = "uint32",
.get = get_uint32,
.set = set_uint32,
-.set_default_value = field_prop_set_default_value_uint,
};
const PropertyInfo prop_info_int32 = {
.name = "int32",
.get = field_prop_get_int32,
.set = set_int32,
-.set_default_value = field_prop_set_default_value_int,
};
/* --- 64bit integer --- */
@@ -339,14 +319,12 @@ const PropertyInfo prop_info_uint64 = {
.name = "uint64",
.get = get_uint64,
.set = set_uint64,
-.set_default_value = field_prop_set_default_value_uint,
};
const PropertyInfo prop_info_int64 = {
.name = "int64",
.get = get_int64,
.set = set_int64,
-.set_default_value = field_prop_set_default_value_int,
};
/* --- string --- */
@@ -441,7 +419,6 @@ const PropertyInfo prop_info_size32 = {
.name = "size",
.get = field_prop_get_size32,
.set = set_size32,
-.set_default_value = field_prop_set_default_value_uint,
};
/* --- support for array properties --- */
@@ -505,7 +482,6 @@ const PropertyInfo prop_info_arraylen = {
.name = "uint32",
.get = get_uint32,
.set = set_prop_arraylen,
-.set_default_value = field_prop_set_default_value_uint,
};
/* --- 6
[PATCH v3 14/19] qom: Don't ignore defval on UUID property
UUID properties were weird because the value of .defval was
completely ignored. Fix this by setting the default value to
QLIT_QSTR("auto") on the default case, and actually using .defval
inside .set_default_value.
Signed-off-by: Eduardo Habkost
---
This is a new patch added in v3 of this series.
This is similar (but not completely the same) to changes that
were submitted as part of
"[PATCH v2 8/8] qom: Use qlit to represent property defaults".
---
include/hw/qdev-properties-system.h | 9 +++--
hw/core/qdev-properties-system.c| 10 +-
2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/include/hw/qdev-properties-system.h
b/include/hw/qdev-properties-system.h
index 6221da704e..834ca84904 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -63,14 +63,11 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_width, \
PCIExpLinkWidth)
+#define UUID_VALUE_AUTO"auto"
+
#define DEFINE_PROP_UUID(_name, _state, _field) \
DEFINE_PROP(_name, _state, _field, qdev_prop_uuid, QemuUUID, \
-/* \
- * Note that set_default_uuid_auto() currently \
- * ignores the actual value value of .defval,\
- * we just need it to not be not QTYPE_NONE \
- */ \
-.defval = QLIT_QNULL)
+.defval = QLIT_QSTR(UUID_VALUE_AUTO))
#define DEFINE_PROP_AUDIODEV(_n, _s, _f) \
DEFINE_PROP(_n, _s, _f, qdev_prop_audiodev, QEMUSoundCard)
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 6071f672a4..117c540254 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -15,6 +15,7 @@
#include "hw/qdev-properties-system.h"
#include "qapi/error.h"
#include "qapi/visitor.h"
+#include "qapi/qmp/qstring.h"
#include "qapi/qapi-types-block.h"
#include "qapi/qapi-types-machine.h"
#include "qapi/qapi-types-migration.h"
@@ -1059,8 +1060,6 @@ static void get_uuid(Object *obj, Visitor *v, const char
*name, void *opaque,
visit_type_str(v, name, &p, errp);
}
-#define UUID_VALUE_AUTO"auto"
-
static void set_uuid(Object *obj, Visitor *v, const char *name, void *opaque,
Error **errp)
{
@@ -1080,10 +1079,11 @@ static void set_uuid(Object *obj, Visitor *v, const
char *name, void *opaque,
g_free(str);
}
-static void set_default_uuid_auto(ObjectProperty *op, const Property *prop,
+static void set_default_uuid(ObjectProperty *op, const Property *prop,
const QObject *defval)
{
-object_property_set_default_str(op, UUID_VALUE_AUTO);
+QString *qs = qobject_to(QString, defval);
+object_property_set_default_str(op, qstring_get_str(qs));
}
const PropertyInfo qdev_prop_uuid = {
@@ -1092,5 +1092,5 @@ const PropertyInfo qdev_prop_uuid = {
"\" for random value",
.get = get_uuid,
.set = set_uuid,
-.set_default_value = set_default_uuid_auto,
+.set_default_value = set_default_uuid,
};
--
2.28.0
[PATCH v3 03/19] qnum: QNumValue type for QNum value literals
Provide a separate QNumValue type that can be used for QNum value
literals without the referencing counting and memory allocation
features provided by QObject.
Signed-off-by: Eduardo Habkost
---
Changes v2 -> v3:
* Fixed copy-pasta at qnum_from_value() documentation
* Removed qnum_get_value() function
* Moved doc comment of qnum_from_value() to .c file, for
consistency with other functions.
* Removed "private:" doc comment at QNumValue.
* Removed unnecessary kernel-doc noise (obvious
parameter descriptions).
* Removed space after type cast in qnum_from_*().
* qnum_is_equal() variable const-ness & renames:
* Renamed new QNumValue variables to val_x/val_y.
* Keep existing QNum num_x/num_y variable names.
* const-ness change of num_x/num_y was moved to a separate
patch.
Changes v1 -> v2:
* Fix "make check" failure, by updating check-qnum unit test to
use the new struct fields
---
include/qapi/qmp/qnum.h | 23 ++-
qobject/qnum.c | 91 ++---
tests/check-qnum.c | 14 +++
3 files changed, 76 insertions(+), 52 deletions(-)
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index 3e9ecd324e..03193dca20 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -44,16 +44,35 @@ typedef enum {
* in range: qnum_get_try_int() / qnum_get_try_uint() check range and
* convert under the hood.
*/
-struct QNum {
-struct QObjectBase_ base;
+
+/**
+ * struct QNumValue: the value of a QNum
+ *
+ * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
+ * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
+ */
+typedef struct QNumValue {
QNumKind kind;
union {
int64_t i64;
uint64_t u64;
double dbl;
} u;
+} QNumValue;
+
+#define QNUM_VAL_INT(value) \
+{ .kind = QNUM_I64, .u.i64 = value }
+#define QNUM_VAL_UINT(value) \
+{ .kind = QNUM_U64, .u.u64 = value }
+#define QNUM_VAL_DOUBLE(value) \
+{ .kind = QNUM_DOUBLE, .u.dbl = value }
+
+struct QNum {
+struct QObjectBase_ base;
+QNumValue value;
};
+QNum *qnum_from_value(QNumValue value);
QNum *qnum_from_int(int64_t value);
QNum *qnum_from_uint(uint64_t value);
QNum *qnum_from_double(double value);
diff --git a/qobject/qnum.c b/qobject/qnum.c
index e5ea728638..94e668db60 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -16,21 +16,29 @@
#include "qapi/qmp/qnum.h"
/**
- * qnum_from_int(): Create a new QNum from an int64_t
+ * qnum_from_value(): Create a new QNum from a QNumValue
*
* Return strong reference.
*/
-QNum *qnum_from_int(int64_t value)
+QNum *qnum_from_value(QNumValue value)
{
QNum *qn = g_new(QNum, 1);
qobject_init(QOBJECT(qn), QTYPE_QNUM);
-qn->kind = QNUM_I64;
-qn->u.i64 = value;
-
+qn->value = value;
return qn;
}
+/**
+ * qnum_from_int(): Create a new QNum from an int64_t
+ *
+ * Return strong reference.
+ */
+QNum *qnum_from_int(int64_t value)
+{
+return qnum_from_value((QNumValue)QNUM_VAL_INT(value));
+}
+
/**
* qnum_from_uint(): Create a new QNum from an uint64_t
*
@@ -38,13 +46,7 @@ QNum *qnum_from_int(int64_t value)
*/
QNum *qnum_from_uint(uint64_t value)
{
-QNum *qn = g_new(QNum, 1);
-
-qobject_init(QOBJECT(qn), QTYPE_QNUM);
-qn->kind = QNUM_U64;
-qn->u.u64 = value;
-
-return qn;
+return qnum_from_value((QNumValue)QNUM_VAL_UINT(value));
}
/**
@@ -54,13 +56,7 @@ QNum *qnum_from_uint(uint64_t value)
*/
QNum *qnum_from_double(double value)
{
-QNum *qn = g_new(QNum, 1);
-
-qobject_init(QOBJECT(qn), QTYPE_QNUM);
-qn->kind = QNUM_DOUBLE;
-qn->u.dbl = value;
-
-return qn;
+return qnum_from_value((QNumValue)QNUM_VAL_DOUBLE(value));
}
/**
@@ -70,15 +66,17 @@ QNum *qnum_from_double(double value)
*/
bool qnum_get_try_int(const QNum *qn, int64_t *val)
{
-switch (qn->kind) {
+const QNumValue *qv = &qn->value;
+
+switch (qv->kind) {
case QNUM_I64:
-*val = qn->u.i64;
+*val = qv->u.i64;
return true;
case QNUM_U64:
-if (qn->u.u64 > INT64_MAX) {
+if (qv->u.u64 > INT64_MAX) {
return false;
}
-*val = qn->u.u64;
+*val = qv->u.u64;
return true;
case QNUM_DOUBLE:
return false;
@@ -108,15 +106,17 @@ int64_t qnum_get_int(const QNum *qn)
*/
bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
{
-switch (qn->kind) {
+const QNumValue *qv = &qn->value;
+
+switch (qv->kind) {
case QNUM_I64:
-if (qn->u.i64 < 0) {
+if (qv->u.i64 < 0) {
return false;
}
-*val = qn->u.i64;
+*val = qv->u.i64;
return true;
case QNUM_U64:
-*val = qn->u.u64;
+*val = qv->u.u64;
return true;
case QNUM_DOUBLE:
return false;
@@ -146,1
[PATCH v3 04/19] qnum: qnum_value_is_equal() function
Extract the QNum value comparison logic to a function that takes
QNumValue* as argument.
Signed-off-by: Eduardo Habkost
---
Changes v2 -> v3:
* Rename function parameters to val_x/val_y
* Insert blank line after variable declarations at
qnum_is_equal()
---
include/qapi/qmp/qnum.h | 1 +
qobject/qnum.c | 24
2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index 03193dca20..3dcb020689 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -87,6 +87,7 @@ double qnum_get_double(const QNum *qn);
char *qnum_to_string(QNum *qn);
+bool qnum_value_is_equal(const QNumValue *val_x, const QNumValue *val_y);
bool qnum_is_equal(const QObject *x, const QObject *y);
void qnum_destroy_obj(QObject *obj);
diff --git a/qobject/qnum.c b/qobject/qnum.c
index 94e668db60..53c637f46d 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -202,7 +202,7 @@ char *qnum_to_string(QNum *qn)
}
/**
- * qnum_is_equal(): Test whether the two QNums are equal
+ * qnum_value_is_equal(): Test whether two QNumValues are equal
*
* Negative integers are never considered equal to unsigned integers,
* but positive integers in the range [0, INT64_MAX] are considered
@@ -210,13 +210,8 @@ char *qnum_to_string(QNum *qn)
*
* Doubles are never considered equal to integers.
*/
-bool qnum_is_equal(const QObject *x, const QObject *y)
+bool qnum_value_is_equal(const QNumValue *val_x, const QNumValue *val_y)
{
-const QNum *num_x = qobject_to(QNum, x);
-const QNum *num_y = qobject_to(QNum, y);
-const QNumValue *val_x = &num_x->value;
-const QNumValue *val_y = &num_y->value;
-
switch (val_x->kind) {
case QNUM_I64:
switch (val_y->kind) {
@@ -234,7 +229,7 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
case QNUM_U64:
switch (val_y->kind) {
case QNUM_I64:
-return qnum_is_equal(y, x);
+return qnum_value_is_equal(val_y, val_x);
case QNUM_U64:
/* Comparison in native uint64_t type */
return val_x->u.u64 == val_y->u.u64;
@@ -257,6 +252,19 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
abort();
}
+/**
+ * qnum_is_equal(): Test whether the two QNums are equal
+ *
+ * See qnum_value_is_equal() for details on the comparison rules.
+ */
+bool qnum_is_equal(const QObject *x, const QObject *y)
+{
+const QNum *qnum_x = qobject_to(QNum, x);
+const QNum *qnum_y = qobject_to(QNum, y);
+
+return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
+}
+
/**
* qnum_destroy_obj(): Free all memory allocated by a
* QNum object
--
2.28.0
[PATCH v3 19/19] qom: Delete set_default_value_bool()
set_default_value_bool() can be completely replaced by
object_property_set_default(), we don't need that function
anymore.
Signed-off-by: Eduardo Habkost
---
This is a new patch in v3 of this series.
In v2 of the series, equivalent changes were part of "qom: Use
qlit to represent property defaults".
---
qom/property-types.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/qom/property-types.c b/qom/property-types.c
index 399e36c29e..cb7ba2a229 100644
--- a/qom/property-types.c
+++ b/qom/property-types.c
@@ -84,19 +84,11 @@ static void prop_set_bit(Object *obj, Visitor *v, const
char *name,
bit_prop_set(obj, prop, value);
}
-static void set_default_value_bool(ObjectProperty *op, const Property *prop,
- const QObject *defval)
-{
-QBool *qb = qobject_to(QBool, defval);
-object_property_set_default_bool(op, qbool_get_bool(qb));
-}
-
const PropertyInfo prop_info_bit = {
.name = "bool",
.description = "on/off",
.get = prop_get_bit,
.set = prop_set_bit,
-.set_default_value = set_default_value_bool,
};
/* Bit64 */
@@ -145,7 +137,6 @@ const PropertyInfo prop_info_bit64 = {
.description = "on/off",
.get = prop_get_bit64,
.set = prop_set_bit64,
-.set_default_value = set_default_value_bool,
};
/* --- bool --- */
@@ -172,7 +163,6 @@ const PropertyInfo prop_info_bool = {
.name = "bool",
.get = get_bool,
.set = set_bool,
-.set_default_value = set_default_value_bool,
};
/* --- 8bit integer --- */
--
2.28.0
[PATCH v3 12/19] qom: Replace defval value in Property with QLitObject
QLitObject is capable of representing a wider range of values,
and it will allow us to simplify a lot of the existing code
setting property defaults, later. Replace the bool and union
fields with QLitObject.
In short, this is just a direct translation from:
prop->set_default
to
prop->defval.type != QTYPE_NONE
from
prop->defval.i
to
qnum_get_int(qobject_to(QNum, prop->defval)
and from
prop->defval.u
to
qnum_get_uint(qobject_to(QNum, prop->defval)
Note that this doesn't replace any of the default property
setters (yet), but just make them safer. Now the actual type of
.defval is validated before it is used.
Also note that set_default_uuid_auto() is a bit weird: it ignores
.defval completely. This patch keeps the existing behavior, and
set_default_uuid_auto() weirdness will be addressed later.
Signed-off-by: Eduardo Habkost
---
This is a new patch added in v3 of this series.
I am splitting the changes and making them in smaller steps to
make them easier to understand and review. With this, I intend
to demonstrate that the conversion from bool+union to QLitObject
is an improvement even if the removal of custom .set_defaul_value
functions isn't 100% finished yet.
---
include/hw/qdev-properties-system.h | 7 ++-
include/qom/field-property-internal.h | 9 ++---
include/qom/field-property.h | 27 ++-
include/qom/property-types.h | 18 ++
hw/core/qdev-properties-system.c | 3 ++-
qom/field-property.c | 8 ++--
qom/property-types.c | 26 ++
7 files changed, 54 insertions(+), 44 deletions(-)
diff --git a/include/hw/qdev-properties-system.h
b/include/hw/qdev-properties-system.h
index 0ac327ae60..6221da704e 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -65,7 +65,12 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
#define DEFINE_PROP_UUID(_name, _state, _field) \
DEFINE_PROP(_name, _state, _field, qdev_prop_uuid, QemuUUID, \
-.set_default = true)
+/* \
+ * Note that set_default_uuid_auto() currently \
+ * ignores the actual value value of .defval,\
+ * we just need it to not be not QTYPE_NONE \
+ */ \
+.defval = QLIT_QNULL)
#define DEFINE_PROP_AUDIODEV(_n, _s, _f) \
DEFINE_PROP(_n, _s, _f, qdev_prop_audiodev, QEMUSoundCard)
diff --git a/include/qom/field-property-internal.h
b/include/qom/field-property-internal.h
index a7b7e2b69d..7ed0d8d160 100644
--- a/include/qom/field-property-internal.h
+++ b/include/qom/field-property-internal.h
@@ -14,11 +14,14 @@ void field_prop_set_enum(Object *obj, Visitor *v, const
char *name,
void *opaque, Error **errp);
void field_prop_set_default_value_enum(ObjectProperty *op,
- const Property *prop);
+ const Property *prop,
+ const QObject *defval);
void field_prop_set_default_value_int(ObjectProperty *op,
- const Property *prop);
+ const Property *prop,
+ const QObject *defval);
void field_prop_set_default_value_uint(ObjectProperty *op,
- const Property *prop);
+ const Property *prop,
+ const QObject *defval);
void field_prop_get_int32(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp);
diff --git a/include/qom/field-property.h b/include/qom/field-property.h
index 0cb1fe2217..951cec2fb0 100644
--- a/include/qom/field-property.h
+++ b/include/qom/field-property.h
@@ -6,6 +6,7 @@
#include "qom/object.h"
#include "qapi/util.h"
+#include "qapi/qmp/qlit.h"
/**
* struct Property: definition of a field property
@@ -27,21 +28,8 @@ struct Property {
const PropertyInfo *info;
ptrdiff_toffset;
uint8_t bitnr;
-/**
- * @set_default: true if the default value should be set from @defval,
- *in which case @info->set_default_value must not be NULL
- *(if false then no default value is set by the property system
- * and the field retains whatever value it was given by instance_init).
- */
-bool set_default;
-/**
- * @defval: default value for the property. This is used only if
@set_default
- * is true.
- */
-union {
-int64_t i;
-uint64_t u;
-} defval;
+/** @defval: If not QTYPE_NONE, the default value for the property */
+QLitObject defval;
/* priva
[PATCH v3 09/19] qlit: Add more test literals to qlit_equal_qobject() test case
Add a few examples of each qlit type, to make sure each one
compare as equal to itself, but not equal to the other values.
Signed-off-by: Eduardo Habkost
---
This is a new patch added in v3 of this series.
---
tests/check-qlit.c | 16
1 file changed, 16 insertions(+)
diff --git a/tests/check-qlit.c b/tests/check-qlit.c
index b1cfbddb17..5a9260b93f 100644
--- a/tests/check-qlit.c
+++ b/tests/check-qlit.c
@@ -50,11 +50,27 @@ static void qlit_equal_qobject_test(void)
{
/* Each entry in the values[] array should be different from the others */
QLitObject values[] = {
+QLIT_QNULL,
+QLIT_QBOOL(false),
+QLIT_QBOOL(true),
+QLIT_QNUM_INT(-1),
+QLIT_QNUM_INT(0),
+QLIT_QNUM_INT(1),
+QLIT_QNUM_INT(INT64_MIN),
+QLIT_QNUM_INT(INT64_MAX),
+QLIT_QSTR(""),
+QLIT_QSTR("foo"),
qlit,
QLIT_QDICT(((QLitDictEntry[]) {
{ "foo", QLIT_QNUM_INT(42) },
{ },
})),
+QLIT_QLIST(((QLitObject[]){
+QLIT_QNUM_INT(-1),
+QLIT_QNUM_INT(0),
+QLIT_QNUM_INT(1),
+{ },
+})),
};
int i;
QObject *qobj = make_qobject();
--
2.28.0
[PATCH v3 02/19] qnum: Make num_x/num_y variables at qnum_is_equal() const
qobject_to() drops const-ness by accident, but our function
arguments (x, y) are const. Make num_x/num_y const too.
Signed-off-by: Eduardo Habkost
---
This is a new patch added in v3 of the series.
---
qobject/qnum.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/qobject/qnum.c b/qobject/qnum.c
index d328d91fcb..e5ea728638 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -209,8 +209,8 @@ char *qnum_to_string(QNum *qn)
*/
bool qnum_is_equal(const QObject *x, const QObject *y)
{
-QNum *num_x = qobject_to(QNum, x);
-QNum *num_y = qobject_to(QNum, y);
+const QNum *num_x = qobject_to(QNum, x);
+const QNum *num_y = qobject_to(QNum, y);
switch (num_x->kind) {
case QNUM_I64:
--
2.28.0
[PATCH v3 18/19] qom: Delete set_default_uuid()
The function can be completely replaced by
object_property_set_default(), we don't need it anymore.
Signed-off-by: Eduardo Habkost
---
This is a new patch in v3 of this series.
In v2 of the series, equivalent changes were part of "qom: Use
qlit to represent property defaults".
---
hw/core/qdev-properties-system.c | 8
1 file changed, 8 deletions(-)
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index b2df955f2a..07945ea1c0 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -1077,18 +1077,10 @@ static void set_uuid(Object *obj, Visitor *v, const
char *name, void *opaque,
g_free(str);
}
-static void set_default_uuid(ObjectProperty *op, const Property *prop,
- const QObject *defval)
-{
-QString *qs = qobject_to(QString, defval);
-object_property_set_default_str(op, qstring_get_str(qs));
-}
-
const PropertyInfo qdev_prop_uuid = {
.name = "str",
.description = "UUID (aka GUID) or \"" UUID_VALUE_AUTO
"\" for random value",
.get = get_uuid,
.set = set_uuid,
-.set_default_value = set_default_uuid,
};
--
2.28.0
[PATCH v3 06/19] qlit: Rename QLIT_QNUM to QLIT_QNUM_INT
Rename the existing QLIT_QNUM macro to indicate it only supports
signed int values. We're going to add support to other types of
QNums later.
Signed-off-by: Eduardo Habkost
---
This is a new patch added in v3 of this series.
In v2, the existing QLIT_QNUM() macro was being kept, now it is
replaced by QLIT_QNUM_INT().
---
include/qapi/qmp/qlit.h | 2 +-
tests/check-qjson.c | 30 +++---
tests/check-qlit.c | 10 +-
3 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
index c0676d5daf..2fc2db282e 100644
--- a/include/qapi/qmp/qlit.h
+++ b/include/qapi/qmp/qlit.h
@@ -39,7 +39,7 @@ struct QLitDictEntry {
{ .type = QTYPE_QNULL }
#define QLIT_QBOOL(val) \
{ .type = QTYPE_QBOOL, .value.qbool = (val) }
-#define QLIT_QNUM(val) \
+#define QLIT_QNUM_INT(val) \
{ .type = QTYPE_QNUM, .value.qnum = (val) }
#define QLIT_QSTR(val) \
{ .type = QTYPE_QSTRING, .value.qstr = (val) }
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 07a773e653..bc5b7ebdf3 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1062,7 +1062,7 @@ static void simple_dict(void)
{
.encoded = "{\"foo\": 42, \"bar\": \"hello world\"}",
.decoded = QLIT_QDICT(((QLitDictEntry[]){
-{ "foo", QLIT_QNUM(42) },
+{ "foo", QLIT_QNUM_INT(42) },
{ "bar", QLIT_QSTR("hello world") },
{ }
})),
@@ -1074,7 +1074,7 @@ static void simple_dict(void)
}, {
.encoded = "{\"foo\": 43}",
.decoded = QLIT_QDICT(((QLitDictEntry[]){
-{ "foo", QLIT_QNUM(43) },
+{ "foo", QLIT_QNUM_INT(43) },
{ }
})),
},
@@ -1160,15 +1160,15 @@ static void simple_list(void)
{
.encoded = "[43,42]",
.decoded = QLIT_QLIST(((QLitObject[]){
-QLIT_QNUM(43),
-QLIT_QNUM(42),
+QLIT_QNUM_INT(43),
+QLIT_QNUM_INT(42),
{ }
})),
},
{
.encoded = "[43]",
.decoded = QLIT_QLIST(((QLitObject[]){
-QLIT_QNUM(43),
+QLIT_QNUM_INT(43),
{ }
})),
},
@@ -1217,35 +1217,35 @@ static void simple_whitespace(void)
{
.encoded = " [ 43 , 42 ]",
.decoded = QLIT_QLIST(((QLitObject[]){
-QLIT_QNUM(43),
-QLIT_QNUM(42),
+QLIT_QNUM_INT(43),
+QLIT_QNUM_INT(42),
{ }
})),
},
{
.encoded = "\t[ 43 , { 'h' : 'b' },\r\n\t[ ], 42 ]\n",
.decoded = QLIT_QLIST(((QLitObject[]){
-QLIT_QNUM(43),
+QLIT_QNUM_INT(43),
QLIT_QDICT(((QLitDictEntry[]){
{ "h", QLIT_QSTR("b") },
{ }})),
QLIT_QLIST(((QLitObject[]){
{ }})),
-QLIT_QNUM(42),
+QLIT_QNUM_INT(42),
{ }
})),
},
{
.encoded = " [ 43 , { 'h' : 'b' , 'a' : 32 }, [ ], 42 ]",
.decoded = QLIT_QLIST(((QLitObject[]){
-QLIT_QNUM(43),
+QLIT_QNUM_INT(43),
QLIT_QDICT(((QLitDictEntry[]){
{ "h", QLIT_QSTR("b") },
-{ "a", QLIT_QNUM(32) },
+{ "a", QLIT_QNUM_INT(32) },
{ }})),
QLIT_QLIST(((QLitObject[]){
{ }})),
-QLIT_QNUM(42),
+QLIT_QNUM_INT(42),
{ }
})),
},
@@ -1275,11 +1275,11 @@ static void simple_interpolation(void)
QObject *embedded_obj;
QObject *obj;
QLitObject decoded = QLIT_QLIST(((QLitObject[]){
-QLIT_QNUM(1),
+QLIT_QNUM_INT(1),
QLIT_QSTR("100%"),
QLIT_QLIST(((QLitObject[]){
-QLIT_QNUM(32),
-QLIT_QNUM(42),
+
[PATCH v3 08/19] qlit: Move qlit_equal_qobject() reference values to array
Add an array of values to qlit_equal_qobject_test(), so we can
extend the test case to compare multiple literals, not just the
ones at the existing `qlit` and `qlit_foo` variables.
Signed-off-by: Eduardo Habkost
---
This is a new patch added in v3 of this series.
---
tests/check-qlit.c | 26 +++---
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/tests/check-qlit.c b/tests/check-qlit.c
index 24ac21395c..b1cfbddb17 100644
--- a/tests/check-qlit.c
+++ b/tests/check-qlit.c
@@ -29,11 +29,6 @@ static QLitObject qlit = QLIT_QDICT(((QLitDictEntry[]) {
{ },
}));
-static QLitObject qlit_foo = QLIT_QDICT(((QLitDictEntry[]) {
-{ "foo", QLIT_QNUM_INT(42) },
-{ },
-}));
-
static QObject *make_qobject(void)
{
QDict *qdict = qdict_new();
@@ -53,16 +48,33 @@ static QObject *make_qobject(void)
static void qlit_equal_qobject_test(void)
{
+/* Each entry in the values[] array should be different from the others */
+QLitObject values[] = {
+qlit,
+QLIT_QDICT(((QLitDictEntry[]) {
+{ "foo", QLIT_QNUM_INT(42) },
+{ },
+})),
+};
+int i;
QObject *qobj = make_qobject();
g_assert(qlit_equal_qobject(&qlit, qobj));
-g_assert(!qlit_equal_qobject(&qlit_foo, qobj));
-
qdict_put(qobject_to(QDict, qobj), "bee", qlist_new());
g_assert(!qlit_equal_qobject(&qlit, qobj));
qobject_unref(qobj);
+
+for (i = 0; i < ARRAY_SIZE(values); i++) {
+int j;
+QObject *o = qobject_from_qlit(&values[i]);
+for (j = 0; j < ARRAY_SIZE(values); j++) {
+g_assert(qlit_equal_qobject(&values[j], o) == (i == j));
+}
+qobject_unref(o);
+}
+
}
static void qlit_equal_large_qnum_test(void)
--
2.28.0
[PATCH v3 10/19] qlit: Support all types of QNums
Add two new macros to support other types of QNums:
QLIT_QNUM_UINT, and QLIT_QNUM_DOUBLE, and include them
in the qlit_equal_qobject_test() test case.
Signed-off-by: Eduardo Habkost
---
Changes v2 -> v3:
* QLIT_QNUM macro doesn't exist anymore
* Addition of the QNumValue field to QLitObject is
now in a separate patch ("qlit: Use QNumValue to represent QNums")
* check-qjson test case changes dropped.
Instead, I'm only extending the qlit_equal_qobject_test() test
case.
Changes v1 -> v2:
* Coding style fix at qlit_equal_qobject()
---
include/qapi/qmp/qlit.h | 4
tests/check-qlit.c | 5 +
2 files changed, 9 insertions(+)
diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
index a240cdd299..a2881b7f42 100644
--- a/include/qapi/qmp/qlit.h
+++ b/include/qapi/qmp/qlit.h
@@ -42,6 +42,10 @@ struct QLitDictEntry {
{ .type = QTYPE_QBOOL, .value.qbool = (val) }
#define QLIT_QNUM_INT(val) \
{ .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_INT(val) }
+#define QLIT_QNUM_UINT(val) \
+{ .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_UINT(val) }
+#define QLIT_QNUM_DOUBLE(val) \
+{ .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_DOUBLE(val) }
#define QLIT_QSTR(val) \
{ .type = QTYPE_QSTRING, .value.qstr = (val) }
#define QLIT_QDICT(val) \
diff --git a/tests/check-qlit.c b/tests/check-qlit.c
index 5a9260b93f..31e90f8965 100644
--- a/tests/check-qlit.c
+++ b/tests/check-qlit.c
@@ -58,6 +58,11 @@ static void qlit_equal_qobject_test(void)
QLIT_QNUM_INT(1),
QLIT_QNUM_INT(INT64_MIN),
QLIT_QNUM_INT(INT64_MAX),
+QLIT_QNUM_UINT(UINT64_MAX),
+/* Larger than UINT64_MAX: */
+QLIT_QNUM_DOUBLE(18446744073709552e3),
+/* Smaller than INT64_MIN: */
+QLIT_QNUM_DOUBLE(-92233720368547758e2),
QLIT_QSTR(""),
QLIT_QSTR("foo"),
qlit,
--
2.28.0
[PATCH v3 01/19] qnum: Make qnum_get_double() get const pointer
qnum_get_double() won't change the object, the argument can be
const.
Reviewed-by: Marc-André Lureau
Signed-off-by: Eduardo Habkost
---
include/qapi/qmp/qnum.h | 2 +-
qobject/qnum.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index bbae0a5ec8..3e9ecd324e 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -64,7 +64,7 @@ int64_t qnum_get_int(const QNum *qn);
bool qnum_get_try_uint(const QNum *qn, uint64_t *val);
uint64_t qnum_get_uint(const QNum *qn);
-double qnum_get_double(QNum *qn);
+double qnum_get_double(const QNum *qn);
char *qnum_to_string(QNum *qn);
diff --git a/qobject/qnum.c b/qobject/qnum.c
index 7012fc57f2..d328d91fcb 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -144,7 +144,7 @@ uint64_t qnum_get_uint(const QNum *qn)
*
* qnum_get_double() loses precision for integers beyond 53 bits.
*/
-double qnum_get_double(QNum *qn)
+double qnum_get_double(const QNum *qn)
{
switch (qn->kind) {
case QNUM_I64:
--
2.28.0
[PATCH v3 05/19] qlit: Use qnum_value_is_equal() when comparing QNums
Currently, qlit_equal_qobject() crashes if getting a QNum that
can't be represented as int64. Fix this by using
qnum_value_is_equal().
Signed-off-by: Eduardo Habkost
---
This is a new patch added in v3 of the series.
---
qobject/qlit.c | 3 ++-
tests/check-qlit.c | 19 +++
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/qobject/qlit.c b/qobject/qlit.c
index be8332136c..67126b25d5 100644
--- a/qobject/qlit.c
+++ b/qobject/qlit.c
@@ -71,7 +71,8 @@ bool qlit_equal_qobject(const QLitObject *lhs, const QObject
*rhs)
case QTYPE_QBOOL:
return lhs->value.qbool == qbool_get_bool(qobject_to(QBool, rhs));
case QTYPE_QNUM:
-return lhs->value.qnum == qnum_get_int(qobject_to(QNum, rhs));
+return qnum_value_is_equal(&(QNumValue)QNUM_VAL_INT(lhs->value.qnum),
+ &qobject_to(QNum, rhs)->value);
case QTYPE_QSTRING:
return (strcmp(lhs->value.qstr,
qstring_get_str(qobject_to(QString, rhs))) == 0);
diff --git a/tests/check-qlit.c b/tests/check-qlit.c
index bd6798d912..58ceaae5a3 100644
--- a/tests/check-qlit.c
+++ b/tests/check-qlit.c
@@ -65,6 +65,24 @@ static void qlit_equal_qobject_test(void)
qobject_unref(qobj);
}
+static void qlit_equal_large_qnum_test(void)
+{
+/* 2^32-1 */
+QNum *large = qnum_from_uint(9223372036854775807LL);
+/* 2^32 */
+QNum *too_large = qnum_from_uint(9223372036854775808ULL);
+QNum *dbl = qnum_from_double(9223372036854775808.0);
+QLitObject qlit_large = QLIT_QNUM(9223372036854775807LL);
+
+g_assert(qlit_equal_qobject(&qlit_large, QOBJECT(large)));
+g_assert(!qlit_equal_qobject(&qlit_large, QOBJECT(too_large)));
+g_assert(!qlit_equal_qobject(&qlit_large, QOBJECT(dbl)));
+
+qobject_unref(dbl);
+qobject_unref(large);
+qobject_unref(too_large);
+}
+
static void qobject_from_qlit_test(void)
{
QObject *obj, *qobj = qobject_from_qlit(&qlit);
@@ -95,6 +113,7 @@ int main(int argc, char **argv)
g_test_init(&argc, &argv, NULL);
g_test_add_func("/qlit/equal_qobject", qlit_equal_qobject_test);
+g_test_add_func("/qlit/equal_large_qnum", qlit_equal_large_qnum_test);
g_test_add_func("/qlit/qobject_from_qlit", qobject_from_qlit_test);
return g_test_run();
--
2.28.0
[PATCH v3 07/19] qlit: Use QNumValue to represent QNums
Replace the existing int64_t field in QLitObject with QNumValue,
so we can get support for other QNum types for free.
Signed-off-by: Eduardo Habkost
---
This is a new patch added in v3 of the series. It includes
portions of a previous patch from v2:
"qlit: Support all types of QNums".
---
include/qapi/qmp/qlit.h | 5 +++--
qobject/qlit.c | 4 ++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
index 2fc2db282e..a240cdd299 100644
--- a/include/qapi/qmp/qlit.h
+++ b/include/qapi/qmp/qlit.h
@@ -15,6 +15,7 @@
#define QLIT_H
#include "qobject.h"
+#include "qnum.h"
typedef struct QLitDictEntry QLitDictEntry;
typedef struct QLitObject QLitObject;
@@ -23,7 +24,7 @@ struct QLitObject {
QType type;
union {
bool qbool;
-int64_t qnum;
+QNumValue qnum;
const char *qstr;
QLitDictEntry *qdict;
QLitObject *qlist;
@@ -40,7 +41,7 @@ struct QLitDictEntry {
#define QLIT_QBOOL(val) \
{ .type = QTYPE_QBOOL, .value.qbool = (val) }
#define QLIT_QNUM_INT(val) \
-{ .type = QTYPE_QNUM, .value.qnum = (val) }
+{ .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_INT(val) }
#define QLIT_QSTR(val) \
{ .type = QTYPE_QSTRING, .value.qstr = (val) }
#define QLIT_QDICT(val) \
diff --git a/qobject/qlit.c b/qobject/qlit.c
index 67126b25d5..b7af81cefb 100644
--- a/qobject/qlit.c
+++ b/qobject/qlit.c
@@ -71,7 +71,7 @@ bool qlit_equal_qobject(const QLitObject *lhs, const QObject
*rhs)
case QTYPE_QBOOL:
return lhs->value.qbool == qbool_get_bool(qobject_to(QBool, rhs));
case QTYPE_QNUM:
-return qnum_value_is_equal(&(QNumValue)QNUM_VAL_INT(lhs->value.qnum),
+return qnum_value_is_equal(&lhs->value.qnum,
&qobject_to(QNum, rhs)->value);
case QTYPE_QSTRING:
return (strcmp(lhs->value.qstr,
@@ -95,7 +95,7 @@ QObject *qobject_from_qlit(const QLitObject *qlit)
case QTYPE_QNULL:
return QOBJECT(qnull());
case QTYPE_QNUM:
-return QOBJECT(qnum_from_int(qlit->value.qnum));
+return QOBJECT(qnum_from_value(qlit->value.qnum));
case QTYPE_QSTRING:
return QOBJECT(qstring_from_str(qlit->value.qstr));
case QTYPE_QDICT: {
--
2.28.0
[PATCH v3 00/19] qom: Use qlit to represent property defaults
Based-on: 20201104160021.2342108-1-ehabk...@redhat.com Git branch: https://gitlab.com/ehabkost/qemu/-/commits/work/qdev-qlit-defaults This extend qlit.h to support all QNum types (signed int, unsigned int, and double), and use QLitObject to represent field property defaults. It allows us to get rid of most type-specific .set_default_value functions for QOM property types. The only remaining .set_default_value function in the code is field_prop_set_default_value_enum(). This will take a bit more work because the default is currently stored as int, but parsed as string, so it will be addressed in a separate series. Changes v2 -> v3: * qnum/qlit patches: * Variable naming in qnum code * Coding style changes in qnum code * Split "qlit: Support all types of QNums" in smaller patches * Replace QLIT_QNUM with QLIT_QNUM_INT * Extend test cases, move most of the new test case code to check-qlit.c * Remove qnum_get_value() and qlit_get_type() function * Removed kernel-doc conversion patch, to reduce noise in series review * qom patches: * Split "qom: Use qlit to represent property defaults" in smaller patches, hopefully making review easier * Small changes in UUID property code (See "qom: Don't ignore defval on UUID property" and "qom: Fix documentation of UUID property type") Changes v1 -> v2: * Rebase to latest version of field properties series * Fix unit test failure * Coding style changes Eduardo Habkost (19): qnum: Make qnum_get_double() get const pointer qnum: Make num_x/num_y variables at qnum_is_equal() const qnum: QNumValue type for QNum value literals qnum: qnum_value_is_equal() function qlit: Use qnum_value_is_equal() when comparing QNums qlit: Rename QLIT_QNUM to QLIT_QNUM_INT qlit: Use QNumValue to represent QNums qlit: Move qlit_equal_qobject() reference values to array qlit: Add more test literals to qlit_equal_qobject() test case qlit: Support all types of QNums qom: field_prop_set_default_value() helper qom: Replace defval value in Property with QLitObject qom: Fix documentation of UUID property type qom: Don't ignore defval on UUID property qom: Make object_property_set_default() public qom: Make PropertyInfo.set_default_value optional qom: Delete field_prop_set_default_value_*int() qom: Delete set_default_uuid() qom: Delete set_default_value_bool() include/hw/qdev-properties-system.h | 4 +- include/qapi/qmp/qlit.h | 11 ++- include/qapi/qmp/qnum.h | 26 +- include/qom/field-property-internal.h | 7 +- include/qom/field-property.h | 30 +++ include/qom/object.h | 11 +++ include/qom/property-types.h | 18 ++-- hw/core/qdev-properties-system.c | 13 +-- qobject/qlit.c| 5 +- qobject/qnum.c| 113 ++ qom/field-property.c | 36 ++-- qom/object.c | 2 +- qom/property-types.c | 37 ++--- tests/check-qjson.c | 30 +++ tests/check-qlit.c| 72 +--- tests/check-qnum.c| 14 ++-- 16 files changed, 253 insertions(+), 176 deletions(-) -- 2.28.0
Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote: > Eduardo Habkost writes: > > > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote: > [...] > >> When the structure of a data type is to be kept away from its users, I > >> prefer to keep it out of the public header, so the compiler enforces the > >> encapsulation. > > > > I prefer that too, except that it is impossible when users of the > > API need the compiler to know the struct size. > > There are cases where the structure of a data type should be > encapsulated, yet its size must be made known for performance (avoid > dynamic memory allocation and pointer chasing). > > Need for encapsulation correlates with complex algorithms and data > structures. The cost of dynamic allocation is often in the noise then. I don't know what we are talking about anymore. None of this applies to the QNum API, right? QNum/QNumValue are not complex data structures, and the reason we need the compiler to know the size of QNumValue is not related to performance at all. We might still want to discourage users of the QNum API from accessing QNum.u/QNumValue.u directly. Documenting the field as private is a very easy way to do it. -- Eduardo
Re: [RFC v3 9/9] i386: split cpu accelerators from cpu.c
On Fri, Nov 20, 2020 at 10:08:46AM +0100, Claudio Fontana wrote:
> On 11/19/20 8:23 PM, Eduardo Habkost wrote:
> > On Thu, Nov 19, 2020 at 09:53:09AM +0100, Claudio Fontana wrote:
> >> Hi,
> >>
> >> On 11/18/20 7:28 PM, Eduardo Habkost wrote:
> >>> On Wed, Nov 18, 2020 at 11:29:36AM +0100, Claudio Fontana wrote:
> >>>> split cpu.c into:
> >>>>
> >>>> cpu.ccpuid and common x86 cpu functionality
> >>>> host-cpu.c host x86 cpu functions and "host" cpu type
> >>>> kvm/cpu.cKVM x86 cpu type
> >>>> hvf/cpu.cHVF x86 cpu type
> >>>> tcg/cpu.cTCG x86 cpu type
> >>>>
> >>>> The accel interface of the X86CPUClass is set at MODULE_INIT_ACCEL_CPU
> >>>> time, when the accelerator is known.
> >>>>
> >>>> Signed-off-by: Claudio Fontana
> >>>> ---
> >>> [...]
> >>>> +/**
> >>>> + * X86CPUAccel:
> >>>> + * @name: string name of the X86 CPU Accelerator
> >>>> + *
> >>>> + * @common_class_init: initializer for the common cpu
> >>>
> >>> So this will be called for every single CPU class.
> >>
> >> Not really, it's called for every TYPE_X86_CPU cpu class (if an accel
> >> interface is registered).
> >
> > This means every single non-abstract CPU class in
> > qemu-system-x86_64, correct?
> >
> >>
> >> This function extends the existing x86_cpu_common_class_init
> >> (target/i386/cpu.c),
> >> where some methods of the base class CPUClass are set.
> >>
> >>>
> >>>> + * @instance_init: cpu instance initialization
> >>>> + * @realizefn: realize function, called first in x86 cpu realize
> >>>> + *
> >>>> + * X86 CPU accelerator-specific CPU initializations
> >>>> + */
> >>>> +
> >>>> +struct X86CPUAccel {
> >>>> +const char *name;
> >>>> +
> >>>> +void (*common_class_init)(X86CPUClass *xcc);
> >>>> +void (*instance_init)(X86CPU *cpu);
> >>>> +void (*realizefn)(X86CPU *cpu, Error **errp);
> >>>> };
> >>>>
> >>>> +void x86_cpu_accel_init(const X86CPUAccel *accel);
> >>> [...]
> >>>> +static void x86_cpu_accel_init_aux(ObjectClass *klass, void *opaque)
> >>>> +{
> >>>> +X86CPUClass *xcc = X86_CPU_CLASS(klass);
> >>>> +const X86CPUAccel **accel = opaque;
> >>>> +
> >>>> +xcc->accel = *accel;
> >>>> +xcc->accel->common_class_init(xcc);
> >>>> +}
> >>>> +
> >>>> +void x86_cpu_accel_init(const X86CPUAccel *accel)
> >>>> +{
> >>>> +object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false,
> >>>> &accel);
> >>>> +}
> >>>
> >>> This matches the documented behavior.
> >>>
> >>> [...]
> >>>> +void host_cpu_class_init(X86CPUClass *xcc)
> >>>> +{
> >>>> +xcc->host_cpuid_required = true;
> >>>> +xcc->ordering = 8;
> >>>> +xcc->model_description =
> >>>> +g_strdup_printf("%s processor with all supported host features
> >>>> ",
> >>>> +xcc->accel->name);
> >>>> +}
> >>> [...]
> >>>> +static void hvf_cpu_common_class_init(X86CPUClass *xcc)
> >>>> +{
> >>>> +host_cpu_class_init(xcc);
> >>>
> >>> Why are you calling host_cpu_class_init() for all CPU types?
> >>
> >> I am not..
> >
> > I don't get it. You are calling host_cpu_class_init() for every
> > single non-abstract TYPE_X86_CPU subclass (which includes all CPU
> > models in qemu-system-x86_64), and I don't understand why, or if
> > this is really intentional.
>
> It is really intentional what is done here,
>
> when HVF accelerator is enabled, and only when the HVF accelerator is enabled,
>
> all X86 CPU classes and subclasses (cpu models, which have been
> implemented as subclasses of TYPE_X86_CPU), are updated with a
> link to the accelerator-specific HVF interface.
I'm confused. That (updating the class with a link) is not what
host_cpu_class_init() does.
This is what host_cpu_class_init() does:
void host_cpu_class_init(X86CPUClass *xcc)
{
xcc->host_cpuid_required = true;
xcc->ordering = 8;
xcc->model_description =
g_strdup_printf("%s processor with all supported host features ",
xcc->accel->name);
}
Why do you want to call host_cpu_class_init() for every non-abstract
TYPE_X86_CPU subclass?
--
Eduardo
Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU
On Mon, Nov 23, 2020 at 04:02:24PM +0100, Claudio Fontana wrote:
> On 11/23/20 2:18 PM, Paolo Bonzini wrote:
> > On 23/11/20 10:55, Claudio Fontana wrote:
> >> One idea that came to mind is, why not extend accel.h to user mode?
> >>
> >> It already contains
> >>
> >> #ifndef CONFIG_USER_ONLY
> >>
> >> parts, so maybe it was meant to be used by both, and just happened to
> >> end up confined to include/softmmu ?
> >>
> >> Basically I was thinking, we could have an AccelState and an
> >> AccelClass for user mode as well (without bringing in the whole
> >> machine thing), and from there we could use current_accel() to build
> >> up the right name for the chosen accelerator?
> >
> > Yes, extending the accelerator class to usermode emulation is certainly
> > a good idea.
> >
> > Paolo
> >
>
> Thanks, I'll work on this option.
>
> Btw considering that CpusAccel for tcg is actually three different interfaces
> (for mttcg, for icount, and plain RR),
> it will be tough to, in the stated objective, "remove all conditionals", even
> after removing the tcg_enabled().
The main issue were the conditionals inside module init function.
They are completely OK inside accel-specific methods.
>
> I wonder how you see this issue (patches for 3 TCG split are in Richard's
> queue atm).
>
> static void tcg_accel_cpu_init(void)
> {
> if (tcg_enabled()) {
> TCGState *s = TCG_STATE(current_accel());
>
> if (s->mttcg_enabled) {
> cpus_register_accel(&tcg_cpus_mttcg);
> } else if (icount_enabled()) {
> cpus_register_accel(&tcg_cpus_icount);
> } else {
> cpus_register_accel(&tcg_cpus_rr);
> }
> }
> }
Probably what we are missing here is a non-softmmu-specific
AccelClass.init() method?
--
Eduardo
Re: [RFC v4 9/9] i386: split cpu accelerators from cpu.c
On Fri, Nov 20, 2020 at 07:47:11PM +0100, Claudio Fontana wrote:
> On 11/20/20 6:44 PM, Eduardo Habkost wrote:
> > On Fri, Nov 20, 2020 at 03:49:09PM +0100, Claudio Fontana wrote:
> >> split cpu.c into:
> >>
> >> cpu.ccpuid and common x86 cpu functionality
> >> host-cpu.c host x86 cpu functions and "host" cpu type
> >> kvm/cpu.cKVM x86 cpu type
> >> hvf/cpu.cHVF x86 cpu type
> >> tcg/cpu.cTCG x86 cpu type
> >>
> >> The link to the accel class is set in the X86CPUClass classes
> >> at MODULE_INIT_ACCEL_CPU time, when the accelerator is known.
> >>
> >> Signed-off-by: Claudio Fontana
> > [...]
> >> +static void hvf_cpu_accel_class_init(ObjectClass *oc, void *data)
> >> +{
> >> +X86CPUAccelClass *acc = X86_CPU_ACCEL_CLASS(oc);
> >> +
> >> +acc->cpu_realizefn = host_cpu_realizefn;
> >> +acc->cpu_common_class_init = hvf_cpu_common_class_init;
> >> +acc->cpu_instance_init = hvf_cpu_instance_init;
> >> +};
> >> +static const TypeInfo hvf_cpu_accel_type_info = {
> >> +.name = X86_CPU_ACCEL_TYPE_NAME("hvf"),
> >> +
> >> +.parent = TYPE_X86_CPU_ACCEL,
> >> +.class_init = hvf_cpu_accel_class_init,
> >> +};
> >> +static void hvf_cpu_accel_register_types(void)
> >> +{
> >> +type_register_static(&hvf_cpu_accel_type_info);
> >> +}
> >> +type_init(hvf_cpu_accel_register_types);
> >> +
> >> +static void hvf_cpu_accel_init(void)
> >> +{
> >> +if (hvf_enabled()) {
> >> +x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("hvf"));
> >> +}
> >> +}
> >> +
> >> +accel_cpu_init(hvf_cpu_accel_init);
> >
> > The point of my suggestion of using QOM is to not require
> > separate accel_cpu_init() functions and (hvf|tcg|kvm)_enabled()
> > checks.
> >
> > If we still have separate accel_cpu_init() functions for calling
> > x86_cpu_accel_init() with the right argument, using a pointer to
> > static variables like &hvf_cpu_accel (like you did before) was
> > simpler and required less boilerplate code.
>
>
>
> Yes I agree.
>
>
>
>
> >
> > However, the difference is that with the X86_CPU_ACCEL_TYPE_NAME
> > macro + object_class_by_name(), you don't need the separate
> > accel_cpu_init() functions for each accelerator.
> >
> > All you need is a single:
> >
> > x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME(chosen_accel_name));
> >
> > call somewhere in the initialization path.
>
>
> Makes sense. The problem is just determining chosen_accel_name.
Yeah, that was a challenge. do_configure_accelerator() knows
what's the chosen accel name, though.
We can also do it inside accel_init_machine(), if we can
determine the correct accel name from the AccelState object.
>
>
> >
> > A good place for the x86_cpu_accel_init() call would be
> > do_configure_accelerator(), but the function is arch-specific.
> > That's why I suggested a cpu_accel_arch_init() function at
> > https://lore.kernel.org/qemu-devel/20201118220750.gp1509...@habkost.net
> >
>
>
> Fine by me. I'd use a specific init step for this, but that also works.
A separate module init function has no easy access to the accel
name, but in this case I'd say it's on purpose: the intended use
case for module init functions is to unconditionally register
features provided by a code module. They shouldn't look at any
runtime configuration or runtime state.
--
Eduardo
Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
> Eduardo Habkost writes:
>
> > On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
> >> Marc-André Lureau writes:
> >>
> >> > On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost
> >> > wrote:
> >> >
> >> >> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> >> >> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost
> >> >> wrote:
> >> >> >
> >> >> > > Provide a separate QNumValue type that can be used for QNum value
> >> >> > > literals without the referencing counting and memory allocation
> >> >> > > features provided by QObject.
> >> >> > >
> >> >> > > Signed-off-by: Eduardo Habkost
> >> >> > > ---
> >> >> > > Changes v1 -> v2:
> >> >> > > * Fix "make check" failure, by updating check-qnum unit test to
> >> >> > > use the new struct fields
> >> >> > > ---
> >> >> > > include/qapi/qmp/qnum.h | 40 +++--
> >> >> > > qobject/qnum.c | 78
> >> >> > > -
> >> >> > > tests/check-qnum.c | 14
> >> >> > > 3 files changed, 84 insertions(+), 48 deletions(-)
> >> >> > >
> >> >> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> >> >> > > index 55c27b1c24..62fbdfda68 100644
> >> >> > > --- a/include/qapi/qmp/qnum.h
> >> >> > > +++ b/include/qapi/qmp/qnum.h
> >> >> > > @@ -46,20 +46,56 @@ typedef enum {
> >> >> > > * in range: qnum_get_try_int() / qnum_get_try_uint() check range
> >> >> > > and
> >> >> > > * convert under the hood.
> >> >> > > */
> >> >> > > -struct QNum {
> >> >> > > -struct QObjectBase_ base;
> >> >> > > +
> >> >> > > +/**
> >> >> > > + * struct QNumValue: the value of a QNum
> >> >> > > + *
> >> >> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> >> >> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> >> >> > > + */
> >> >> > > +typedef struct QNumValue {
> >> >> > > +/* private: */
> >>
> >> Do we care?
> >
> > Are you asking if we want to make them private, or if we want to
> > document them as private (assuming they are).
> >
> > The answer to the latter is yes ("private:" is an indication to
> > kernel-doc). The answer to the former seems to be "no", based on
> > your other comments.
> >
> > Or maybe `kind` should be public and `u` should be private?
>
> You're factoring out a part of struct QNum into new struct QNumValue.
> struct QNum is not private before the patch. I see no need to start
> making it or parts of it private now.
I don't want to change the rules, just to document the existing
implicit rules. If you say QNum.u was never private, I won't
argue.
>
> When the structure of a data type is to be kept away from its users, I
> prefer to keep it out of the public header, so the compiler enforces the
> encapsulation.
I prefer that too, except that it is impossible when users of the
API need the compiler to know the struct size.
--
Eduardo
Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
On Fri, Nov 20, 2020 at 07:52:31AM +0100, Markus Armbruster wrote:
> Eduardo Habkost writes:
>
> > On Thu, Nov 19, 2020 at 11:27:40AM +0100, Markus Armbruster wrote:
> > [...]
> >> > +bool qnum_is_equal(const QObject *x, const QObject *y)
> >> > +{
> >> > +const QNum *qnum_x = qobject_to(QNum, x);
> >> > +const QNum *qnum_y = qobject_to(QNum, y);
> >>
> >> Humor me: blank line between declarations and statements, please.
> >
> > I can do it. But why do you prefer it that way?
>
> Habit borne out of C lacking other visual cues to distinguish
> declarations and statements.
Why is the distinction important, when many variable declarations
also include initializer expressions that can be as complex as
other statements?
(The qobject_to() calls above are an example).
>
> Declaration or statement? Tell me quick, don't analyze!
>
> mumble(*mutter)();
>
> This "obviously" declares @mutter as pointer to function returning
> mumble.
>
> Except when @mumble isn't a typedef name, but a function taking one
> argument and returning a function that takes no argument. Then it
> passes *mutter to mumble(), and calls its return value.
>
> The whole point of coding style is to help readers along. Two stylistic
> conventions that can help here:
>
> 1. In a function call, no space between the expression denoting the
>called function and the (parenthesized) argument list. Elsewhere,
>space.
>
>So, when the example above is indeed a declaration, write it as
>
> mumble (*mutter)();
>
>If it's function calls, write it as
>
> mumble(*mutter)();
This makes lots of sense. Starting with a word followed by space
is what makes declarations visually distinguishable.
>
> 2. Separate declarations from statements with a blank line. Do not mix
>them.
I'm not sure about this one, and I'm actually glad it is not part
of CODING_STYLE. :)
(I'll still follow your advice as maintainer of that piece of
code, of course)
--
Eduardo
Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU
On Fri, Nov 20, 2020 at 06:41:35PM +0100, Claudio Fontana wrote: > On 11/20/20 6:19 PM, Eduardo Habkost wrote: > > On Fri, Nov 20, 2020 at 01:13:33PM +0100, Claudio Fontana wrote: > >> On 11/18/20 11:07 PM, Eduardo Habkost wrote: > >>> On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote: > >>>> On 18/11/20 18:30, Eduardo Habkost wrote: > >>>>>> Adding a layer of indirect calls is not very different from monkey > >>>>>> patching > >>>>>> though. > >>>>> > >>>>> I'm a little bothered by monkey patching, but I'm more > >>>>> bothered by having to: > >>>>> > >>>>> (1) register (module_init()) a function (kvm_cpu_accel_register()) that > >>>>>(2) register (accel_register_call()) a function > >>>>> (kvm_cpu_accel_init()) that > >>>>> (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel > >>>>> kvm_cpu_accel) that > >>>>>(4) will be saved in multiple QOM classes, so that > >>>>> (5) we will call the right X86CPUClass.accel method at the > >>>>> right moment > >>>>> (common_class_init(), instance_init(), realizefn()), > >>>>> where: > >>>>>step 4 must be done before any CPU object is created > >>>>> (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn > >>>>> will be silently ignored), and > >>>>>step 3 must be done after all QOM types were registered. > >>>>> > >>>>>> You also have to consider that accel currently does not exist in > >>>>>> usermode > >>>>>> emulators, so that's an issue too. I would rather get a simple change > >>>>>> in > >>>>>> quickly, instead of designing the perfect class hierarchy. > >>>>> > >>>>> It doesn't have to be perfect. I agree that simple is better. > >>>>> > >>>>> To me, registering a QOM type and looking it up when necessary is > >>>>> simpler than the above. Even if it's a weird class having no > >>>>> object instances. It probably could be an interface type. > >>>> > >>>> Registering a QOM type still has quite some boilerplate. [...] > >>> > >>> We're working on that. :) > >>> > >>>>[...] Also > >>>> registering a > >>>> QOM type has a public side effect (shows up in qom-list-types). In > >>>> general > >>>> I don't look at QOM unless I want its property mechanism, but maybe > >>>> that's > >>>> just me. > >>> > >>> We have lots of internal-use-only types returned by > >>> qom-list-types, I don't think it's a big deal. > >>> > >>>> > >>>>>> Perhaps another idea would be to allow adding interfaces to classes > >>>>>> *separately from the registration of the types*. Then we can use it to > >>>>>> add > >>>>>> SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, > >>>>>> and > >>>>>> add the accel object to usermode emulators. > >>>>> > >>>>> I'm not sure I follow. What do you mean by bare bones accel > >>>>> class, and when exactly would you add the new interfaces to the > >>>>> classes? > >>>> > >>>> A bare bones accel class would not have init_machine and setup_post > >>>> methods; > >>>> those would be in a TYPE_SOFTMMU_ACCEL interface. It would still have > >>>> properties (such as tb-size for TCG) and would be able to register compat > >>>> properties. > > > > [1] > > > >>> > >>> Oh, I think I see. This could save us having a lot of parallel type > >>> hierarchies. > >>> > >>>> > >>>> Where would I add it, I don't know. It could be a simple public wrapper > >>>> around type_initialize_interface() if we add a new MODULE_INIT_* phase > >>>> after > >>>> QOM. > >>>> > >>>> Or without adding
Re: [RFC v4 9/9] i386: split cpu accelerators from cpu.c
On Fri, Nov 20, 2020 at 03:49:09PM +0100, Claudio Fontana wrote:
> split cpu.c into:
>
> cpu.ccpuid and common x86 cpu functionality
> host-cpu.c host x86 cpu functions and "host" cpu type
> kvm/cpu.cKVM x86 cpu type
> hvf/cpu.cHVF x86 cpu type
> tcg/cpu.cTCG x86 cpu type
>
> The link to the accel class is set in the X86CPUClass classes
> at MODULE_INIT_ACCEL_CPU time, when the accelerator is known.
>
> Signed-off-by: Claudio Fontana
[...]
> +static void hvf_cpu_accel_class_init(ObjectClass *oc, void *data)
> +{
> +X86CPUAccelClass *acc = X86_CPU_ACCEL_CLASS(oc);
> +
> +acc->cpu_realizefn = host_cpu_realizefn;
> +acc->cpu_common_class_init = hvf_cpu_common_class_init;
> +acc->cpu_instance_init = hvf_cpu_instance_init;
> +};
> +static const TypeInfo hvf_cpu_accel_type_info = {
> +.name = X86_CPU_ACCEL_TYPE_NAME("hvf"),
> +
> +.parent = TYPE_X86_CPU_ACCEL,
> +.class_init = hvf_cpu_accel_class_init,
> +};
> +static void hvf_cpu_accel_register_types(void)
> +{
> +type_register_static(&hvf_cpu_accel_type_info);
> +}
> +type_init(hvf_cpu_accel_register_types);
> +
> +static void hvf_cpu_accel_init(void)
> +{
> +if (hvf_enabled()) {
> +x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("hvf"));
> +}
> +}
> +
> +accel_cpu_init(hvf_cpu_accel_init);
The point of my suggestion of using QOM is to not require
separate accel_cpu_init() functions and (hvf|tcg|kvm)_enabled()
checks.
If we still have separate accel_cpu_init() functions for calling
x86_cpu_accel_init() with the right argument, using a pointer to
static variables like &hvf_cpu_accel (like you did before) was
simpler and required less boilerplate code.
However, the difference is that with the X86_CPU_ACCEL_TYPE_NAME
macro + object_class_by_name(), you don't need the separate
accel_cpu_init() functions for each accelerator.
All you need is a single:
x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME(chosen_accel_name));
call somewhere in the initialization path.
A good place for the x86_cpu_accel_init() call would be
do_configure_accelerator(), but the function is arch-specific.
That's why I suggested a cpu_accel_arch_init() function at
https://lore.kernel.org/qemu-devel/20201118220750.gp1509...@habkost.net
--
Eduardo
Re: [RFC v4 9/9] i386: split cpu accelerators from cpu.c
On Fri, Nov 20, 2020 at 04:34:47PM +0100, Claudio Fontana wrote: > On 11/20/20 3:49 PM, Claudio Fontana wrote: > > split cpu.c into: > > > > cpu.ccpuid and common x86 cpu functionality > > host-cpu.c host x86 cpu functions and "host" cpu type > > kvm/cpu.cKVM x86 cpu type > > hvf/cpu.cHVF x86 cpu type > > tcg/cpu.cTCG x86 cpu type > > > > The link to the accel class is set in the X86CPUClass classes > > at MODULE_INIT_ACCEL_CPU time, when the accelerator is known. > > > > Signed-off-by: Claudio Fontana > > --- [...] > > +#define TYPE_X86_CPU_ACCEL TYPE_X86_CPU "-accel" > > +#define X86_CPU_ACCEL_TYPE_NAME(name) (name "-" TYPE_X86_CPU_ACCEL) > > + > > +OBJECT_DECLARE_TYPE(X86CPUAccel, X86CPUAccelClass, > > +X86_CPU_ACCEL) > > > Instead of OBJECT_DECLARE_TYPE, since this is never instantiated, this should > probably be: > > +typedef struct X86CPUAccelClass X86CPUAccelClass; > +DECLARE_CLASS_CHECKERS(X86CPUAccelClass, X86_CPU_ACCEL, TYPE_X86_CPU_ACCEL) Yes, and this way we get rid of the only difference between OBJECT_DECLARE_TYPE and OBJECT_DECLARE_INTERFACE: the instance type cast macro is a bit different (it uses INTERFACE_CHECK). -- Eduardo
Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU
On Fri, Nov 20, 2020 at 01:13:33PM +0100, Claudio Fontana wrote: > On 11/18/20 11:07 PM, Eduardo Habkost wrote: > > On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote: > >> On 18/11/20 18:30, Eduardo Habkost wrote: > >>>> Adding a layer of indirect calls is not very different from monkey > >>>> patching > >>>> though. > >>> > >>> I'm a little bothered by monkey patching, but I'm more > >>> bothered by having to: > >>> > >>> (1) register (module_init()) a function (kvm_cpu_accel_register()) that > >>>(2) register (accel_register_call()) a function (kvm_cpu_accel_init()) > >>> that > >>> (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel > >>> kvm_cpu_accel) that > >>>(4) will be saved in multiple QOM classes, so that > >>> (5) we will call the right X86CPUClass.accel method at the right > >>> moment > >>> (common_class_init(), instance_init(), realizefn()), > >>> where: > >>>step 4 must be done before any CPU object is created > >>> (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn > >>> will be silently ignored), and > >>>step 3 must be done after all QOM types were registered. > >>> > >>>> You also have to consider that accel currently does not exist in usermode > >>>> emulators, so that's an issue too. I would rather get a simple change in > >>>> quickly, instead of designing the perfect class hierarchy. > >>> > >>> It doesn't have to be perfect. I agree that simple is better. > >>> > >>> To me, registering a QOM type and looking it up when necessary is > >>> simpler than the above. Even if it's a weird class having no > >>> object instances. It probably could be an interface type. > >> > >> Registering a QOM type still has quite some boilerplate. [...] > > > > We're working on that. :) > > > >>[...] Also registering > >> a > >> QOM type has a public side effect (shows up in qom-list-types). In general > >> I don't look at QOM unless I want its property mechanism, but maybe that's > >> just me. > > > > We have lots of internal-use-only types returned by > > qom-list-types, I don't think it's a big deal. > > > >> > >>>> Perhaps another idea would be to allow adding interfaces to classes > >>>> *separately from the registration of the types*. Then we can use it to > >>>> add > >>>> SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, and > >>>> add the accel object to usermode emulators. > >>> > >>> I'm not sure I follow. What do you mean by bare bones accel > >>> class, and when exactly would you add the new interfaces to the > >>> classes? > >> > >> A bare bones accel class would not have init_machine and setup_post > >> methods; > >> those would be in a TYPE_SOFTMMU_ACCEL interface. It would still have > >> properties (such as tb-size for TCG) and would be able to register compat > >> properties. [1] > > > > Oh, I think I see. This could save us having a lot of parallel type > > hierarchies. > > > >> > >> Where would I add it, I don't know. It could be a simple public wrapper > >> around type_initialize_interface() if we add a new MODULE_INIT_* phase > >> after > >> QOM. > >> > >> Or without adding a new phase, it could be a class_type->array of > >> (interface_type, init_fn) hash table. type_initialize would look up the > >> class_type by name, add the interfaces would to the class with > >> type_initialize_interface, and then call the init_fn to fill in the vtable. > > > > That sounds nice. I don't think Claudio's cleanup should be > > blocked until this new mechanism is ready, though. > > > > We don't really need the type representing X86CPUAccel to be a > > subtype of TYPE_ACCEL or an interface implemented by > > current_machine->accelerator, in the first version. We just need > > a simple way for the CPU initialization code to find the correct > > X86CPUAccel struct. > > > > While we don't have the new mechanism, it can be just a: > &
Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
On Thu, Nov 19, 2020 at 01:21:58PM -0500, Eduardo Habkost wrote:
> On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
[...]
> > >> > > +return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
> >
> > No space between between (type) and its operand, please.
> >
> > Could we lift the cast into the macro somehow?
>
> I think we can. I had thought the cast in the macro would break
> usage as static variable initializers. I was wrong.
Actually, including the cast in the macro breaks QLIT_QDICT
initializers (which use (QLitDictEntry[]) compound literals), and
I don't know why.
Compound literals in initializers of static variables is a GCC
extension. I don't understand why it doesn't work inside array
compound literals, though.
Any language lawyers around?
This works:
typedef struct QLit {
int x, y;
} QLit;
typedef struct Entry {
int key;
QLit value;
} Entry;
Entry e = { .key = 0, .value = (QLit) { 1, 2 } };
This works:
Entry *es1 = (Entry[]) {
{ .key = 0, .value = { 1, 2 } },
};
But this doesn't:
Entry *es2 = (Entry[]) {
{ .key = 0, .value = (QLit) { 1, 2 } },
};
dict.c:16:24: error: initializer element is not constant
16 | Entry *es2 = (Entry[]) {
|^
dict.c:16:24: note: (near initialization for ‘es2’)
(gcc (GCC) 10.2.1 20201005 (Red Hat 10.2.1-5))
--
Eduardo
Re: [RFC v3 9/9] i386: split cpu accelerators from cpu.c
On Thu, Nov 19, 2020 at 09:53:09AM +0100, Claudio Fontana wrote:
> Hi,
>
> On 11/18/20 7:28 PM, Eduardo Habkost wrote:
> > On Wed, Nov 18, 2020 at 11:29:36AM +0100, Claudio Fontana wrote:
> >> split cpu.c into:
> >>
> >> cpu.ccpuid and common x86 cpu functionality
> >> host-cpu.c host x86 cpu functions and "host" cpu type
> >> kvm/cpu.cKVM x86 cpu type
> >> hvf/cpu.cHVF x86 cpu type
> >> tcg/cpu.cTCG x86 cpu type
> >>
> >> The accel interface of the X86CPUClass is set at MODULE_INIT_ACCEL_CPU
> >> time, when the accelerator is known.
> >>
> >> Signed-off-by: Claudio Fontana
> >> ---
> > [...]
> >> +/**
> >> + * X86CPUAccel:
> >> + * @name: string name of the X86 CPU Accelerator
> >> + *
> >> + * @common_class_init: initializer for the common cpu
> >
> > So this will be called for every single CPU class.
>
> Not really, it's called for every TYPE_X86_CPU cpu class (if an accel
> interface is registered).
This means every single non-abstract CPU class in
qemu-system-x86_64, correct?
>
> This function extends the existing x86_cpu_common_class_init
> (target/i386/cpu.c),
> where some methods of the base class CPUClass are set.
>
> >
> >> + * @instance_init: cpu instance initialization
> >> + * @realizefn: realize function, called first in x86 cpu realize
> >> + *
> >> + * X86 CPU accelerator-specific CPU initializations
> >> + */
> >> +
> >> +struct X86CPUAccel {
> >> +const char *name;
> >> +
> >> +void (*common_class_init)(X86CPUClass *xcc);
> >> +void (*instance_init)(X86CPU *cpu);
> >> +void (*realizefn)(X86CPU *cpu, Error **errp);
> >> };
> >>
> >> +void x86_cpu_accel_init(const X86CPUAccel *accel);
> > [...]
> >> +static void x86_cpu_accel_init_aux(ObjectClass *klass, void *opaque)
> >> +{
> >> +X86CPUClass *xcc = X86_CPU_CLASS(klass);
> >> +const X86CPUAccel **accel = opaque;
> >> +
> >> +xcc->accel = *accel;
> >> +xcc->accel->common_class_init(xcc);
> >> +}
> >> +
> >> +void x86_cpu_accel_init(const X86CPUAccel *accel)
> >> +{
> >> +object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false,
> >> &accel);
> >> +}
> >
> > This matches the documented behavior.
> >
> > [...]
> >> +void host_cpu_class_init(X86CPUClass *xcc)
> >> +{
> >> +xcc->host_cpuid_required = true;
> >> +xcc->ordering = 8;
> >> +xcc->model_description =
> >> +g_strdup_printf("%s processor with all supported host features ",
> >> +xcc->accel->name);
> >> +}
> > [...]
> >> +static void hvf_cpu_common_class_init(X86CPUClass *xcc)
> >> +{
> >> +host_cpu_class_init(xcc);
> >
> > Why are you calling host_cpu_class_init() for all CPU types?
>
> I am not..
I don't get it. You are calling host_cpu_class_init() for every
single non-abstract TYPE_X86_CPU subclass (which includes all CPU
models in qemu-system-x86_64), and I don't understand why, or if
this is really intentional.
>
> >
> >> +}
> > [...]
> >> +static void kvm_cpu_common_class_init(X86CPUClass *xcc)
> >> +{
> >> +host_cpu_class_init(xcc);
> >> +}
> >
> > Same question as above.
> >
>
> Ciao,
>
> Claudio
>
--
Eduardo
Re: [PATCH v2 0/8] qom: Use qlit to represent property defaults
On Thu, Nov 19, 2020 at 07:25:15PM +0100, Paolo Bonzini wrote: > On 19/11/20 18:55, Eduardo Habkost wrote: > > On Thu, Nov 19, 2020 at 06:23:30PM +0100, Paolo Bonzini wrote: > > > On 19/11/20 18:13, Eduardo Habkost wrote: > > > > > What's left? > > > > Enums. Enums properties are a mess to implement, and I plan to > > > > tackle them later. > > > > > > > > On all other cases, the external representation of the property > > > > value is similar to the internal representation. In the case of > > > > enums, the external representation is a string, but the internal > > > > representation is an integer. > > > > > > > > > > I would have expected a string QLit to work with enums, is there a reason > > > why it doesn't? > > > > It would work, but it would be more inconvenient for callers. > > Right now they use the C enum constant instead of a string. > > It matches what you have to do already for compat props, so it's not a big > deal. I would say just use strings. No problem to me. I'll do. -- Eduardo
Re: [PATCH v2 5/8] qlit: Support all types of QNums
On Thu, Nov 19, 2020 at 11:39:14AM +0100, Markus Armbruster wrote:
> Marc-André Lureau writes:
>
> > On Tue, Nov 17, 2020 at 2:48 AM Eduardo Habkost wrote:
> >
> >> Use QNumValue to represent QNums, so we can also support uint64_t
> >> and double QNum values. Add new QLIT_QNUM_(INT|UINT|DOUBLE)
> >> macros for each case.
> >>
> >> The QLIT_QNUM() macro is being kept for compatibility with
> >> existing code, but becomes just a wrapper for QLIT_QNUM_INT().
> >>
> >
> > I am not sure it's worth to keep. (furthermore, it's only used in tests
> > afaics)
>
> Seconded.
Understood, I will remove it. I thought the QAPI code generator
was using it.
[...]
> >> diff --git a/qobject/qlit.c b/qobject/qlit.c
> >> index be8332136c..b23cdc4532 100644
> >> --- a/qobject/qlit.c
> >> +++ b/qobject/qlit.c
> >> @@ -71,7 +71,8 @@ bool qlit_equal_qobject(const QLitObject *lhs, const
> >> QObject *rhs)
> >> case QTYPE_QBOOL:
> >> return lhs->value.qbool == qbool_get_bool(qobject_to(QBool, rhs));
> >> case QTYPE_QNUM:
> >> -return lhs->value.qnum == qnum_get_int(qobject_to(QNum, rhs));
> >> +return qnum_value_is_equal(&lhs->value.qnum,
> >> + qnum_get_value(qobject_to(QNum, rhs)));
>
> Before the patch, we crash when @rhs can't be represented as int64_t.
I thought it was expected behavior? QLit never supported
QNUM_U64 or QNUM_DOUBLE as input.
>
> Afterwards, we return false (I think).
>
> Please note this in the commit message. A separate fix preceding this
> patch would be even better, but may not be worth the trouble. Up to
> you.
The fix would be 3 or 4 extra lines of code that would be
immediately deleted. I'll just mention it as a side effect of
the new feature.
>
> >> case QTYPE_QSTRING:
> >> return (strcmp(lhs->value.qstr,
> >> qstring_get_str(qobject_to(QString, rhs))) == 0);
> >> @@ -94,7 +95,7 @@ QObject *qobject_from_qlit(const QLitObject *qlit)
> >> case QTYPE_QNULL:
> >> return QOBJECT(qnull());
> >> case QTYPE_QNUM:
> >> -return QOBJECT(qnum_from_int(qlit->value.qnum));
> >> +return QOBJECT(qnum_from_value(qlit->value.qnum));
> >> case QTYPE_QSTRING:
> >> return QOBJECT(qstring_from_str(qlit->value.qstr));
> >> case QTYPE_QDICT: {
> >> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> >> index 07a773e653..711030cffd 100644
> >> --- a/tests/check-qjson.c
> >> +++ b/tests/check-qjson.c
> >> @@ -796,20 +796,23 @@ static void simple_number(void)
> >> int i;
> >> struct {
> >> const char *encoded;
> >> +QLitObject qlit;
> >> int64_t decoded;
> >> int skip;
> >> } test_cases[] = {
> >> -{ "0", 0 },
> >> -{ "1234", 1234 },
> >> -{ "1", 1 },
> >> -{ "-32", -32 },
> >> -{ "-0", 0, .skip = 1 },
> >> +{ "0",QLIT_QNUM(0),0, },
> >> +{ "1234", QLIT_QNUM(1234), 1234, },
> >> +{ "1",QLIT_QNUM(1),1, },
> >> +{ "-32", QLIT_QNUM(-32), -32, },
> >> +{ "-0", QLIT_QNUM(0),0, .skip = 1 },
>
> Note .qlit is always QLIT_QNUM(.decoded). Would doing without .qlit
> result in a simpler patch?
Good point. When I wrote this, I mistakenly thought we would end
up having different types of qlits in the array.
I still want to test multiple types of QNums here, not just
QNUM_I64. I will try to get something that is simple but also
gets us more coverage. Maybe as a separate test function and/or
a separate patch.
>
> >> { },
> >> };
> >>
> >> for (i = 0; test_cases[i].encoded; i++) {
> >> QNum *qnum;
> >> int64_t val;
> >> +QNum *qlit_num;
> >> +int64_t qlit_val;
> >>
> >> qnum = qobject_to(QNum,
> >>qobject_from_json(test_cases[i].encoded,
> >> @@ -817,6 +820,7 @@ static void simple_number(void)
> >> g_assert(qnum);
> >> g_assert(qnum_get_try_int(qnum, &val));
> >> g_assert_cmpint(val, ==, test_cases[i].decoded);
> >> +
> >> if (test_cases[i].skip == 0) {
> >> QString *str;
> >>
> >> @@ -826,9 +830,66 @@ static void simple_number(void)
> >> }
> >>
> >> qobject_unref(qnum);
> >> +
> >> +qlit_num = qobject_to(QNum,
> >> + qobject_from_qlit(&test_cases[i].qlit));
> >> +g_assert(qlit_num);
> >> +g_assert(qnum_get_try_int(qlit_num, &qlit_val));
> >> +g_assert_cmpint(qlit_val, ==, test_cases[i].decoded);
> >> +
> >> +qobject_unref(qlit_num);
> >> }
> >> }
> >>
[...]
--
Eduardo
Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
On Thu, Nov 19, 2020 at 11:24:52AM +0100, Markus Armbruster wrote:
> Marc-André Lureau writes:
>
> > On Tue, Nov 17, 2020 at 6:42 PM Eduardo Habkost wrote:
> >
> >> On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> >> > On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost
> >> wrote:
> >> >
> >> > > Provide a separate QNumValue type that can be used for QNum value
> >> > > literals without the referencing counting and memory allocation
> >> > > features provided by QObject.
> >> > >
> >> > > Signed-off-by: Eduardo Habkost
> >> > > ---
> >> > > Changes v1 -> v2:
> >> > > * Fix "make check" failure, by updating check-qnum unit test to
> >> > > use the new struct fields
> >> > > ---
> >> > > include/qapi/qmp/qnum.h | 40 +++--
> >> > > qobject/qnum.c | 78 -
> >> > > tests/check-qnum.c | 14
> >> > > 3 files changed, 84 insertions(+), 48 deletions(-)
> >> > >
> >> > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> >> > > index 55c27b1c24..62fbdfda68 100644
> >> > > --- a/include/qapi/qmp/qnum.h
> >> > > +++ b/include/qapi/qmp/qnum.h
> >> > > @@ -46,20 +46,56 @@ typedef enum {
> >> > > * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> >> > > * convert under the hood.
> >> > > */
> >> > > -struct QNum {
> >> > > -struct QObjectBase_ base;
> >> > > +
> >> > > +/**
> >> > > + * struct QNumValue: the value of a QNum
> >> > > + *
> >> > > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> >> > > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> >> > > + */
> >> > > +typedef struct QNumValue {
> >> > > +/* private: */
>
> Do we care?
Are you asking if we want to make them private, or if we want to
document them as private (assuming they are).
The answer to the latter is yes ("private:" is an indication to
kernel-doc). The answer to the former seems to be "no", based on
your other comments.
Or maybe `kind` should be public and `u` should be private?
>
> >> > > QNumKind kind;
> >> > > union {
> >> > > int64_t i64;
> >> > > uint64_t u64;
> >> > > double dbl;
> >> > > } u;
> >> > > +} QNumValue;
> >> > > +
> >> > > +#define QNUM_VAL_INT(value) \
> >> > > +{ .kind = QNUM_I64, .u.i64 = value }
> >> > > +#define QNUM_VAL_UINT(value) \
> >> > > +{ .kind = QNUM_U64, .u.u64 = value }
> >> > > +#define QNUM_VAL_DOUBLE(value) \
> >> > > +{ .kind = QNUM_DOUBLE, .u.dbl = value }
> >> > > +
> >> > > +struct QNum {
> >> > > +struct QObjectBase_ base;
> >> > > +QNumValue value;
> >> > > };
> >> > >
> >> > > +/**
> >> > > + * qnum_from_int(): Create a new QNum from a QNumValue
> >> > >
> >> >
> >> > Copy-pasta qnum_from_int() -> qnum_from_value()
> >>
> >> Oops! I will fix it in v3, or submit a fixup patch if that's the
> >> only needed change.
> >>
> >> >
> >> > I wonder if there is a check for that kind of mistake, as it may be
> >> common.
> >>
> >> It looks like kernel-doc ignores what's before the colon in the
> >> summary line. It probably shouldn't.
> >>
> >> >
> >> > + * @value: QNumValue
> >> > > + *
> >> > > + * Return strong reference.
> >> > > + */
> >> > > +QNum *qnum_from_value(QNumValue value);
> >> > >
> >> > +
> >> > > QNum *qnum_from_int(int64_t value);
> >> > > QNum *qnum_from_uint(uint64_t value);
> >> > > QNum *qnum_from_double(double value);
> >> > >
> >> > > +/**
> >> > > + * qnum_get_value(): Get QNumValue from QNum
> >> > > + * @qn: QNum object
> >> > > + */
> >> > > +static inline const QNumValue *qnum_get_value(const QNum *qn)
&
Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
On Thu, Nov 19, 2020 at 11:27:40AM +0100, Markus Armbruster wrote:
[...]
> > +bool qnum_is_equal(const QObject *x, const QObject *y)
> > +{
> > +const QNum *qnum_x = qobject_to(QNum, x);
> > +const QNum *qnum_y = qobject_to(QNum, y);
>
> Humor me: blank line between declarations and statements, please.
I can do it. But why do you prefer it that way?
>
> > +return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
> > +}
> > +
> > /**
> > * qnum_destroy_obj(): Free all memory allocated by a QNum object
> > *
--
Eduardo
Re: [PATCH v2 1/8] qobject: Include API docs in docs/devel/qobject.html
CCing Peter, Sphinx documentation machinery maintainer.
On Thu, Nov 19, 2020 at 10:37:22AM +0100, Markus Armbruster wrote:
> Eduardo Habkost writes:
>
> > Render existing doc comments at docs/devel/qobject.html.
> >
> > Signed-off-by: Eduardo Habkost
> > ---
[...]
> > /**
> > - * qnum_destroy_obj(): Free all memory allocated by a
> > - * QNum object
> > + * qnum_destroy_obj(): Free all memory allocated by a QNum object
> > + *
> > + * @obj: QNum object to be destroyed
> > */
> > void qnum_destroy_obj(QObject *obj)
> > {
>
> Many lines of the form
>
> + * @foo: a foo
I hate them too.
>
> One of my reasons to dislike GTK-Doc. Oh well, it's what we're using.
It's not gtk-doc. It's kernel-doc who generates warnings
if some parameters are not documented, and maybe we should
silence them.
--
Eduardo
Re: [PATCH v2 6/8] qlit: qlit_type() function
On Thu, Nov 19, 2020 at 11:41:05AM +0100, Markus Armbruster wrote:
> Eduardo Habkost writes:
>
> > Useful function where we need to check for the qlit type before
> > converting it to an actual QObject.
> >
> > Signed-off-by: Eduardo Habkost
> > ---
> > include/qapi/qmp/qlit.h | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
> > index f9e356d31e..acddb80831 100644
> > --- a/include/qapi/qmp/qlit.h
> > +++ b/include/qapi/qmp/qlit.h
> > @@ -59,4 +59,9 @@ bool qlit_equal_qobject(const QLitObject *lhs, const
> > QObject *rhs);
> >
> > QObject *qobject_from_qlit(const QLitObject *qlit);
> >
> > +static inline QType qlit_type(const QLitObject *qlit)
> > +{
> > +return qlit->type;
> > +}
> > +
> > #endif /* QLIT_H */
>
> Hiding qlit->type behind a function makes sense only when the structure
> of QLitObject is an implementation secret. It don't think it is.
I thought all QLitObject fields were considered private. If they
are not, I can happily get rid of that function.
--
Eduardo
Re: [PATCH v2 0/8] qom: Use qlit to represent property defaults
On Thu, Nov 19, 2020 at 06:23:30PM +0100, Paolo Bonzini wrote: > On 19/11/20 18:13, Eduardo Habkost wrote: > > > What's left? > > Enums. Enums properties are a mess to implement, and I plan to > > tackle them later. > > > > On all other cases, the external representation of the property > > value is similar to the internal representation. In the case of > > enums, the external representation is a string, but the internal > > representation is an integer. > > > > I would have expected a string QLit to work with enums, is there a reason > why it doesn't? It would work, but it would be more inconvenient for callers. Right now they use the C enum constant instead of a string. -- Eduardo
Re: [PATCH v2 0/8] qom: Use qlit to represent property defaults
On Thu, Nov 19, 2020 at 01:39:50PM +0100, Markus Armbruster wrote: > Eduardo Habkost writes: > > > Based-on: 20201104160021.2342108-1-ehabk...@redhat.com > > Git branch: > > https://gitlab.com/ehabkost/qemu/-/commits/work/qdev-qlit-defaults > > > > This extend qlit.h to support all QNum types (signed int, > > unsigned int, and double), and use QLitObject to represent field > > property defaults. > > > > It allows us to get rid of most type-specific .set_default_value > > functions for QOM property types. > > What's left? Enums. Enums properties are a mess to implement, and I plan to tackle them later. On all other cases, the external representation of the property value is similar to the internal representation. In the case of enums, the external representation is a string, but the internal representation is an integer. > > I'm asking because if you create a new way to get rid of most of an old > way, you're still left with two ways, which may or may not be an > improvement. I don't think this is an accurate description. We had five different ways of doing this[1]. I replaced four of them with the new qlit-based mechanism, and now we have two. [1] The five different ways were: qdev_propinfo_set_default_value_enum, qdev_propinfo_set_default_value_int, qdev_propinfo_set_default_value_uint, set_default_uuid_auto, and set_default_value_bool. > > Moving defaults from code to data sounds attractive to me. Data is > easier to reason about than code. For QAPI, we've been talking about > defining defaults in the schema for a long time, but nobody has gotten > around to finish an implementation. -- Eduardo
Re: [PATCH RFC v3 23/23] i386: provide simple 'hyperv=on' option to x86 machine types
On Fri, Oct 09, 2020 at 02:18:42PM +0200, Vitaly Kuznetsov wrote:
> Enabling Hyper-V emulation for a Windows VM is a tiring experience as it
> requires listing all currently supported enlightenments ("hv_*" CPU
> features) explicitly. We do have a 'hv_passthrough' mode enabling
> everything but it can't be used in production as it prevents migration.
>
> Introduce a simple 'hyperv=on' option for all x86 machine types enabling
> all currently supported Hyper-V enlightenments. Later, when new
> enlightenments get implemented, we will be adding them to newer machine
> types only (by disabling them for legacy machine types) thus preserving
> migration.
>
> Signed-off-by: Vitaly Kuznetsov
This patch seems to be especially useful, and I wonder if we
could make sure this is included before all the rest of the
series. Especially considering that the KVM changes might take a
while to be merged.
Would you be able to submit this independently from the rest of
the series? You can add my:
Reviewed-by: Eduardo Habkost
My only (minor) suggestion is to rename
X86MachineClass.hyperv_features to
X86MachineClass.default_hyperv_features.
> ---
> docs/hyperv.txt | 8
> hw/i386/x86.c | 30 ++
> include/hw/i386/x86.h | 7 +++
> target/i386/cpu.c | 14 ++
> 4 files changed, 59 insertions(+)
>
> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> index 5df00da54fc4..1a76a07f8417 100644
> --- a/docs/hyperv.txt
> +++ b/docs/hyperv.txt
> @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled,
> QEMU changes hypervisor
> identification (CPUID 0x4000..0x400A) to Hyper-V. KVM identification
> and features are kept in leaves 0x4100..0x4101.
>
> +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an
> +x86 machine type:
> +
> + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on
> ...
> +
> +Note, new enlightenments are only added to the latest (in-develompent)
> machine
> +type, older machine types keep the list of the supported features intact to
> +safeguard migration.
>
> 3. Existing enlightenments
> ===
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 3137a2008588..eeecd4e3322f 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor
> *v, const char *name,
> visit_type_OnOffAuto(v, name, &x86ms->acpi, errp);
> }
>
> +static bool x86_machine_get_hyperv(Object *obj, Error **errp)
> +{
> +X86MachineState *x86ms = X86_MACHINE(obj);
> +
> +return x86ms->hyperv_enabled;
> +}
> +
> +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp)
> +{
> +X86MachineState *x86ms = X86_MACHINE(obj);
> +
> +x86ms->hyperv_enabled = value;
> +}
> +
> static void x86_machine_initfn(Object *obj)
> {
> X86MachineState *x86ms = X86_MACHINE(obj);
> @@ -1193,6 +1207,16 @@ static void x86_machine_class_init(ObjectClass *oc,
> void *data)
> x86mc->save_tsc_khz = true;
> nc->nmi_monitor_handler = x86_nmi;
>
> +/* Hyper-V features enabled with 'hyperv=on' */
> +x86mc->hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> BIT(HYPERV_FEAT_VAPIC) |
> +BIT(HYPERV_FEAT_TIME) | BIT(HYPERV_FEAT_CRASH) |
> +BIT(HYPERV_FEAT_RESET) | BIT(HYPERV_FEAT_VPINDEX) |
> +BIT(HYPERV_FEAT_RUNTIME) | BIT(HYPERV_FEAT_SYNIC) |
> +BIT(HYPERV_FEAT_STIMER) | BIT(HYPERV_FEAT_FREQUENCIES) |
> +BIT(HYPERV_FEAT_REENLIGHTENMENT) | BIT(HYPERV_FEAT_TLBFLUSH) |
> +BIT(HYPERV_FEAT_EVMCS) | BIT(HYPERV_FEAT_IPI) |
> +BIT(HYPERV_FEAT_STIMER_DIRECT);
> +
> object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto",
> x86_machine_get_smm, x86_machine_set_smm,
> NULL, NULL);
> @@ -1204,6 +1228,12 @@ static void x86_machine_class_init(ObjectClass *oc,
> void *data)
> NULL, NULL);
> object_class_property_set_description(oc, X86_MACHINE_ACPI,
> "Enable ACPI");
> +
> +object_class_property_add_bool(oc, X86_MACHINE_HYPERV,
> +x86_machine_get_hyperv, x86_machine_set_hyperv);
> +
> +object_class_property_set_description(oc, X86_MACHINE_HYPERV,
> +"Enable Hyper-V enlightenments");
> }
>
> static const TypeInfo x86_machine_info = {
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index d5dcf7a07fdc..2b989e5fc49d 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -38,6 +38,9 @@ struct X86MachineClass {
Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU
On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote:
> On 18/11/20 18:30, Eduardo Habkost wrote:
> > > Adding a layer of indirect calls is not very different from monkey
> > > patching
> > > though.
> >
> > I'm a little bothered by monkey patching, but I'm more
> > bothered by having to:
> >
> > (1) register (module_init()) a function (kvm_cpu_accel_register()) that
> >(2) register (accel_register_call()) a function (kvm_cpu_accel_init())
> > that
> > (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel
> > kvm_cpu_accel) that
> >(4) will be saved in multiple QOM classes, so that
> > (5) we will call the right X86CPUClass.accel method at the right
> > moment
> > (common_class_init(), instance_init(), realizefn()),
> > where:
> >step 4 must be done before any CPU object is created
> > (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn
> > will be silently ignored), and
> >step 3 must be done after all QOM types were registered.
> >
> > > You also have to consider that accel currently does not exist in usermode
> > > emulators, so that's an issue too. I would rather get a simple change in
> > > quickly, instead of designing the perfect class hierarchy.
> >
> > It doesn't have to be perfect. I agree that simple is better.
> >
> > To me, registering a QOM type and looking it up when necessary is
> > simpler than the above. Even if it's a weird class having no
> > object instances. It probably could be an interface type.
>
> Registering a QOM type still has quite some boilerplate. [...]
We're working on that. :)
>[...] Also registering a
> QOM type has a public side effect (shows up in qom-list-types). In general
> I don't look at QOM unless I want its property mechanism, but maybe that's
> just me.
We have lots of internal-use-only types returned by
qom-list-types, I don't think it's a big deal.
>
> > > Perhaps another idea would be to allow adding interfaces to classes
> > > *separately from the registration of the types*. Then we can use it to add
> > > SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, and
> > > add the accel object to usermode emulators.
> >
> > I'm not sure I follow. What do you mean by bare bones accel
> > class, and when exactly would you add the new interfaces to the
> > classes?
>
> A bare bones accel class would not have init_machine and setup_post methods;
> those would be in a TYPE_SOFTMMU_ACCEL interface. It would still have
> properties (such as tb-size for TCG) and would be able to register compat
> properties.
Oh, I think I see. This could save us having a lot of parallel type
hierarchies.
>
> Where would I add it, I don't know. It could be a simple public wrapper
> around type_initialize_interface() if we add a new MODULE_INIT_* phase after
> QOM.
>
> Or without adding a new phase, it could be a class_type->array of
> (interface_type, init_fn) hash table. type_initialize would look up the
> class_type by name, add the interfaces would to the class with
> type_initialize_interface, and then call the init_fn to fill in the vtable.
That sounds nice. I don't think Claudio's cleanup should be
blocked until this new mechanism is ready, though.
We don't really need the type representing X86CPUAccel to be a
subtype of TYPE_ACCEL or an interface implemented by
current_machine->accelerator, in the first version. We just need
a simple way for the CPU initialization code to find the correct
X86CPUAccel struct.
While we don't have the new mechanism, it can be just a:
object_class_by_name("%s-x86-cpu-accel" % (accel->name))
call.
Below is a rough draft of what I mean. There's still lots of
room for cleaning it up (especially getting rid of the
X86CPUClass.common_class_init and X86CPUClass.accel fields).
git tree at
https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel
Signed-off-by: Eduardo Habkost
---
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 485eda986a..944d403cbd 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -44,7 +44,6 @@ typedef enum {
MODULE_INIT_BLOCK,
MODULE_INIT_OPTS,
MODULE_INIT_QOM,
-MODULE_INIT_ACCEL_CPU,
MODULE_INIT_TRACE,
MODULE_INIT_XEN_BACKEND,
MODULE_INIT_LIBQOS,
@@ -55,7 +54,6 @@ typedef enum {
#define block_init(function) module_init(function, MODULE_INIT_BLOCK)
#define opts_init(function) module_init(function, MODULE_INIT_OPTS)
#define type
Re: [RFC v3 9/9] i386: split cpu accelerators from cpu.c
On Wed, Nov 18, 2020 at 11:29:36AM +0100, Claudio Fontana wrote:
> split cpu.c into:
>
> cpu.ccpuid and common x86 cpu functionality
> host-cpu.c host x86 cpu functions and "host" cpu type
> kvm/cpu.cKVM x86 cpu type
> hvf/cpu.cHVF x86 cpu type
> tcg/cpu.cTCG x86 cpu type
>
> The accel interface of the X86CPUClass is set at MODULE_INIT_ACCEL_CPU
> time, when the accelerator is known.
>
> Signed-off-by: Claudio Fontana
> ---
[...]
> +/**
> + * X86CPUAccel:
> + * @name: string name of the X86 CPU Accelerator
> + *
> + * @common_class_init: initializer for the common cpu
So this will be called for every single CPU class.
> + * @instance_init: cpu instance initialization
> + * @realizefn: realize function, called first in x86 cpu realize
> + *
> + * X86 CPU accelerator-specific CPU initializations
> + */
> +
> +struct X86CPUAccel {
> +const char *name;
> +
> +void (*common_class_init)(X86CPUClass *xcc);
> +void (*instance_init)(X86CPU *cpu);
> +void (*realizefn)(X86CPU *cpu, Error **errp);
> };
>
> +void x86_cpu_accel_init(const X86CPUAccel *accel);
[...]
> +static void x86_cpu_accel_init_aux(ObjectClass *klass, void *opaque)
> +{
> +X86CPUClass *xcc = X86_CPU_CLASS(klass);
> +const X86CPUAccel **accel = opaque;
> +
> +xcc->accel = *accel;
> +xcc->accel->common_class_init(xcc);
> +}
> +
> +void x86_cpu_accel_init(const X86CPUAccel *accel)
> +{
> +object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false,
> &accel);
> +}
This matches the documented behavior.
[...]
> +void host_cpu_class_init(X86CPUClass *xcc)
> +{
> +xcc->host_cpuid_required = true;
> +xcc->ordering = 8;
> +xcc->model_description =
> +g_strdup_printf("%s processor with all supported host features ",
> +xcc->accel->name);
> +}
[...]
> +static void hvf_cpu_common_class_init(X86CPUClass *xcc)
> +{
> +host_cpu_class_init(xcc);
Why are you calling host_cpu_class_init() for all CPU types?
> +}
[...]
> +static void kvm_cpu_common_class_init(X86CPUClass *xcc)
> +{
> +host_cpu_class_init(xcc);
> +}
Same question as above.
--
Eduardo
Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU
On Wed, Nov 18, 2020 at 05:22:46PM +0100, Paolo Bonzini wrote:
> Il mer 18 nov 2020, 17:11 Eduardo Habkost ha scritto:
>
> > On Wed, Nov 18, 2020 at 04:43:19PM +0100, Paolo Bonzini wrote:
> > > Il mer 18 nov 2020, 16:26 Eduardo Habkost ha
> > scritto:
> > >
> > > >
> > > > > The alternative is to store the (type, function) tuple directly,
> > with the
> > > > > type as a string. Then you can just use type_init.
> > > >
> > > > Right. Let's build on top of that:
> > > >
> > > > Another alternative would be to store a (type, X86CPUAccel) tuple
> > > > directly, with the type as string. This would save the extra
> > > > indirection of the x86_cpu_accel_init() call.
> > > >
> > > > It turns out we already have a mechanism to register and store
> > > > (type, StructContainingFunctionPointers) tuples at initialization
> > > > time: QOM.
> > > >
> > > > X86CPUAccel can become X86CPUAccelClass, and be registered as a
> > > > QOM type. It could be a subtype of TYPE_ACCEL or not, it
> > > > shouldn't matter.
> > > >
> > >
> > > It would be a weird type that isn't instantiated, and/or that does
> > nothing
> > > but monkey patching other classes. I don't think it's a good fit.
> >
> > The whole point of this would be to avoid monkey patching other
> > classes.
> >
>
> Adding a layer of indirect calls is not very different from monkey patching
> though.
I'm a little bothered by monkey patching, but I'm more
bothered by having to:
(1) register (module_init()) a function (kvm_cpu_accel_register()) that
(2) register (accel_register_call()) a function (kvm_cpu_accel_init()) that
(3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel
kvm_cpu_accel) that
(4) will be saved in multiple QOM classes, so that
(5) we will call the right X86CPUClass.accel method at the right moment
(common_class_init(), instance_init(), realizefn()),
where:
step 4 must be done before any CPU object is created
(otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn
will be silently ignored), and
step 3 must be done after all QOM types were registered.
>
> You also have to consider that accel currently does not exist in usermode
> emulators, so that's an issue too. I would rather get a simple change in
> quickly, instead of designing the perfect class hierarchy.
It doesn't have to be perfect. I agree that simple is better.
To me, registering a QOM type and looking it up when necessary is
simpler than the above. Even if it's a weird class having no
object instances. It probably could be an interface type.
>
> Perhaps another idea would be to allow adding interfaces to classes
> *separately from the registration of the types*. Then we can use it to add
> SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, and
> add the accel object to usermode emulators.
I'm not sure I follow. What do you mean by bare bones accel
class, and when exactly would you add the new interfaces to the
classes?
>
> Why wouldn't we instantiate it? There's a huge number of static
> > variables in target/i386/kvm.c that could be moved to that
> > object. Sounds like a perfect fit for me.
> >
>
> Most of those are properties of the running kernel so there's no need to
> move them inside an object.
There's no need, correct. Some consistency would be nice,
though. All kernel capabilities in kvm-all.c are saved in
KVMState.
>
> Paolo
>
> I won't try to stop you if you really want to invent a brand new
> > (name => CollectionOfFunctionPointers) registry, but it seems
> > unnecessary.
> >
> > >
> > > Yet another possibility is to use GHashTable. It is limited to one value
> > > per key, but it's enough if everything is kept local to {hw,target}/i386.
> > > If needed a new function pointer can be added to MachineClass,
> > implemented
> > > in X86MachineState (where the GHashTable would also be) and called in
> > > accel.c.
> > >
> > > Paolo
> > >
> > > Paolo
> > >
> > >
> > > > I remember this was suggested in a previous thread, but I don't
> > > > remember if there were any objections.
> > > >
> > > > >
> > > > > > Making sure module_call_init() is called at the correct moment is
> > > > > > not easier or safer than just making sure accel_init_machine()
>
Re: [PATCH for-6.0 1/6] qapi: Add query-accel command
On Wed, Nov 18, 2020 at 09:56:28AM -0600, Eric Blake wrote:
> On 11/18/20 9:45 AM, Eduardo Habkost wrote:
> > On Wed, Nov 18, 2020 at 02:53:26PM +0100, Markus Armbruster wrote:
> > [...]
> >> Another way to skin this cat:
> >>
> >> {"available": {"kvm": { extra properties... },
> >> "tcg": ...,
> >> "xen": ...},
> >>"active": "kvm"}
> >
> > How would this structure be represented in the QAPI schema?
> >
> > In other words, how do I say "Dict[str, AccelInfo]" in QAPIese?
>
> {'type':'AvailAccel', 'data': {
> '*kvm': 'KvmExtraProps',
> '*tcg': 'TcgExtraProps',
> '*xen': 'XenExtraProps',
> '*hax': 'HaxExtraProps' } }
> {'command':'query-accel', 'returns': {
>'available': 'AvailAccel', 'active': 'strOrEnum' } }
>
> where adding a new accelerator then adds a new optional member to
> AvailAccel as well as possibly a new enum member if 'active' is driving
> by an enum instead of 'str'.
Is it possible to represent this if we don't enumerate all
possible dictionary keys in advance? (I'm not suggesting we
should/shouldn't do that, just wondering if it's possible)
--
Eduardo
Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU
On Wed, Nov 18, 2020 at 04:43:19PM +0100, Paolo Bonzini wrote:
> Il mer 18 nov 2020, 16:26 Eduardo Habkost ha scritto:
>
> >
> > > The alternative is to store the (type, function) tuple directly, with the
> > > type as a string. Then you can just use type_init.
> >
> > Right. Let's build on top of that:
> >
> > Another alternative would be to store a (type, X86CPUAccel) tuple
> > directly, with the type as string. This would save the extra
> > indirection of the x86_cpu_accel_init() call.
> >
> > It turns out we already have a mechanism to register and store
> > (type, StructContainingFunctionPointers) tuples at initialization
> > time: QOM.
> >
> > X86CPUAccel can become X86CPUAccelClass, and be registered as a
> > QOM type. It could be a subtype of TYPE_ACCEL or not, it
> > shouldn't matter.
> >
>
> It would be a weird type that isn't instantiated, and/or that does nothing
> but monkey patching other classes. I don't think it's a good fit.
The whole point of this would be to avoid monkey patching other
classes.
Why wouldn't we instantiate it? There's a huge number of static
variables in target/i386/kvm.c that could be moved to that
object. Sounds like a perfect fit for me.
I won't try to stop you if you really want to invent a brand new
(name => CollectionOfFunctionPointers) registry, but it seems
unnecessary.
>
> Yet another possibility is to use GHashTable. It is limited to one value
> per key, but it's enough if everything is kept local to {hw,target}/i386.
> If needed a new function pointer can be added to MachineClass, implemented
> in X86MachineState (where the GHashTable would also be) and called in
> accel.c.
>
> Paolo
>
> Paolo
>
>
> > I remember this was suggested in a previous thread, but I don't
> > remember if there were any objections.
> >
> > >
> > > > Making sure module_call_init() is called at the correct moment is
> > > > not easier or safer than just making sure accel_init_machine()
> > > > (or another init function you create) is called at the correct
> > > > moment.
> > >
> > > Since there is a way to do it without a new level, that would of course
> > be
> > > fine for me too. Let me explain however why I think Claudio's design had
> > > module_call_init() misplaced and what the fundamental difference is. The
> > > basic phases in qemu_init() are:
> > >
> > > - initialize stuff
> > > - parse command line
> > > - create machine
> > > - create accelerator
> > > - initialize machine
> > > - create devices
> > > - start
> > >
> > > with a mess of other object creation sprinkled between the various phases
> > > (but we don't care about those).
> > >
> > > What I object to, is calling module_call_init() after the "initialize
> > stuff"
> > > phase. Claudio was using it to call the function directly, so it had to
> > be
> > > exactly at "create accelerator". This is different from all other
> > > module_call_init() calls, which are done very early.
> >
> > I agree.
> >
> > >
> > > With the implementation I sketched, accel_register_call must still be
> > done
> > > after type_init, so there's still an ordering constraint, but all it's
> > doing
> > > is registering a callback in the "initialize stuff" phase.
> >
> > Makes sense, if we really want to introduce a new accel_register_call()
> > abstraction. I don't think we need it, though.
> >
> > --
> > Eduardo
> >
> >
--
Eduardo
Re: [PATCH for-6.0 1/6] qapi: Add query-accel command
On Wed, Nov 18, 2020 at 02:53:26PM +0100, Markus Armbruster wrote:
[...]
> Another way to skin this cat:
>
> {"available": {"kvm": { extra properties... },
> "tcg": ...,
> "xen": ...},
>"active": "kvm"}
How would this structure be represented in the QAPI schema?
In other words, how do I say "Dict[str, AccelInfo]" in QAPIese?
>
> No need for unions then. "No dupes" is enforced.
>
> We could inline "available":
>
> {"kvm": { extra properties... },
>"tcg": ...,
>"xen": ...,
>"active": "kvm"}
>
> Future accelerators can't be named "active" then.
>
> > I guess this can be extended with a union to report extra props for the
> > accelerator, discriminated on the 'active' field eg
> >
> > { 'available': [ 'kvm', 'tcg', 'xen' ],
> >'active': 'kvm',
> >'data': {
> >"allow-nested": true,
> >}
> > }
>
> Correct.
--
Eduardo
Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU
On Wed, Nov 18, 2020 at 03:51:44PM +0100, Paolo Bonzini wrote:
> On 18/11/20 15:36, Eduardo Habkost wrote:
> > On Wed, Nov 18, 2020 at 03:05:42PM +0100, Paolo Bonzini wrote:
> > > On 18/11/20 14:48, Claudio Fontana wrote:
> > > > On 11/18/20 1:48 PM, Eduardo Habkost wrote:
> > > > > I don't get why we would use a new module initialization level
> > > >
> > > > To have a clear point in time after which all accelerator interface
> > > > initialization is done.
> > > > It avoids to have to hunt down the registration points spread around
> > > > the code base.
> > > > I'd turn it around, why not?
> > >
> > > I see two disadvantages:
> > >
> > > 1) you have to hunt down accel_cpu_inits instead of looking at accelerator
> > > classes. :)
> > >
> > > 2) all callbacks have an "if (*_enabled())" around the actual meat.
> > > Another
> > > related issue is that usually the module_call_init are unconditional.
> > >
> > > I think the idea of using module_call_init is good however. What about:
> > >
> > > static void kvm_cpu_accel_init(void)
> > > {
> > > x86_cpu_accel_init(&kvm_cpu_accel);
> >
> > What do you expect x86_cpu_accel_init() to do?
>
> I don't know, the same that it was doing in Claudio's patches. :)
>
> He had
>
> if (kvm_enabled()) {
> x86_cpu_accel_init(&kvm_cpu_accel);
> }
>
> and I'm calling only the function that is registered on the enabled
> accelerator.
>
> > I don't understand why a separate module init level is necessary
> > here.
>
> Because you must call accel_register_call after the TYPE_KVM type has been
> registered, or object_class_by_name fails:
>
> void
> accel_register_call(const char *qom_type, void (*fn)(void))
> {
> AccelClass *acc = ACCEL_CLASS(object_class_by_name(qom_type));
>
> acc->setup_calls = g_slist_append(acc->setup_calls, (void *)fn);
> }
>
> The alternative is to store the (type, function) tuple directly, with the
> type as a string. Then you can just use type_init.
Right. Let's build on top of that:
Another alternative would be to store a (type, X86CPUAccel) tuple
directly, with the type as string. This would save the extra
indirection of the x86_cpu_accel_init() call.
It turns out we already have a mechanism to register and store
(type, StructContainingFunctionPointers) tuples at initialization
time: QOM.
X86CPUAccel can become X86CPUAccelClass, and be registered as a
QOM type. It could be a subtype of TYPE_ACCEL or not, it
shouldn't matter.
I remember this was suggested in a previous thread, but I don't
remember if there were any objections.
>
> > Making sure module_call_init() is called at the correct moment is
> > not easier or safer than just making sure accel_init_machine()
> > (or another init function you create) is called at the correct
> > moment.
>
> Since there is a way to do it without a new level, that would of course be
> fine for me too. Let me explain however why I think Claudio's design had
> module_call_init() misplaced and what the fundamental difference is. The
> basic phases in qemu_init() are:
>
> - initialize stuff
> - parse command line
> - create machine
> - create accelerator
> - initialize machine
> - create devices
> - start
>
> with a mess of other object creation sprinkled between the various phases
> (but we don't care about those).
>
> What I object to, is calling module_call_init() after the "initialize stuff"
> phase. Claudio was using it to call the function directly, so it had to be
> exactly at "create accelerator". This is different from all other
> module_call_init() calls, which are done very early.
I agree.
>
> With the implementation I sketched, accel_register_call must still be done
> after type_init, so there's still an ordering constraint, but all it's doing
> is registering a callback in the "initialize stuff" phase.
Makes sense, if we really want to introduce a new accel_register_call()
abstraction. I don't think we need it, though.
--
Eduardo
Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU
On Wed, Nov 18, 2020 at 03:05:42PM +0100, Paolo Bonzini wrote:
> On 18/11/20 14:48, Claudio Fontana wrote:
> > On 11/18/20 1:48 PM, Eduardo Habkost wrote:
> > > On Wed, Nov 18, 2020 at 11:29:35AM +0100, Claudio Fontana wrote:
> > > > apply this to the registration of the cpus accel interfaces,
> > > >
> > > > but this will be also in preparation for later use of this
> > > > new module init step to also defer the registration of the cpu models,
> > > > in order to make them subclasses of a per-accel cpu type.
> > > >
> > > > Signed-off-by: Claudio Fontana
> > > > ---
> > > [...]
> > > > +/*
> > > > + * accelerator has been chosen and initialized, now it is time to
> > > > + * register the cpu accel interface.
> > > > + */
> > > > +module_call_init(MODULE_INIT_ACCEL_CPU);
> > >
> > > I don't get why we would use a new module initialization level
> >
> > To have a clear point in time after which all accelerator interface
> > initialization is done.
> > It avoids to have to hunt down the registration points spread around the
> > code base.
> > I'd turn it around, why not?
>
> I see two disadvantages:
>
> 1) you have to hunt down accel_cpu_inits instead of looking at accelerator
> classes. :)
>
> 2) all callbacks have an "if (*_enabled())" around the actual meat. Another
> related issue is that usually the module_call_init are unconditional.
>
> I think the idea of using module_call_init is good however. What about:
>
> static void kvm_cpu_accel_init(void)
> {
> x86_cpu_accel_init(&kvm_cpu_accel);
What do you expect x86_cpu_accel_init() to do?
> }
>
> static void kvm_cpu_accel_register(void)
> {
> accel_register_call(TYPE_KVM, kvm_cpu_accel_init);
> }
> accel_cpu_init(kvm_cpu_accel_register);
>
> ...
>
> void
> accel_register_call(const char *qom_type, void (*fn)(void))
> {
> AccelClass *acc = ACCEL_CLASS(object_class_by_name(qom_type));
>
> acc->setup_calls = g_slist_append(acc->setup_calls, (void *)fn);
> }
>
> void
> accel_do_call(void *data, void *unused)
> {
> void (*fn)(void) = data;
>
> data();
> }
>
> int accel_init_machine(AccelState *accel, MachineState *ms)
> {
> ...
> if (ret < 0) {
> ms->accelerator = NULL;
> *(acc->allowed) = false;
> object_unref(OBJECT(accel));
> } else {
> object_set_accelerator_compat_props(acc->compat_props);
> g_slist_foreach(acc->setup_calls, accel_do_call, NULL);
Why all this extra complexity if you can simply do:
ACCEL_GET_CLASS(acc)->finish_arch_specific_init();
?
> }
> return ret;
> }
>
> where the module_call_init would be right after MODULE_INIT_QOM
>
> Paolo
>
> > > for this. If the accelerator object was already created, we can
> > > just ask the existing accel object to do whatever initialization
> > > step is necessary.
> > >
> > > e.g. we can add a AccelClass.cpu_accel_ops field, and call:
> > >
> > > cpus_register_accel(current_machine->accelerator->cpu_accel_ops);
> > >
> >
> > _When_ this is done is the question, in my view, where the call to the
> > registration is placed.
> >
> > After adding additonal operations that have to be done at
> > "accelerator-chosen" time, it becomes more and more difficult
> > to trace them around the codebase.
I don't understand why a separate module init level is necessary
here.
Making sure module_call_init() is called at the correct moment is
not easier or safer than just making sure accel_init_machine()
(or another init function you create) is called at the correct
moment.
--
Eduardo
Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU
On Wed, Nov 18, 2020 at 11:29:35AM +0100, Claudio Fontana wrote: > apply this to the registration of the cpus accel interfaces, > > but this will be also in preparation for later use of this > new module init step to also defer the registration of the cpu models, > in order to make them subclasses of a per-accel cpu type. > > Signed-off-by: Claudio Fontana > --- [...] > +/* > + * accelerator has been chosen and initialized, now it is time to > + * register the cpu accel interface. > + */ > +module_call_init(MODULE_INIT_ACCEL_CPU); I don't get why we would use a new module initialization level for this. If the accelerator object was already created, we can just ask the existing accel object to do whatever initialization step is necessary. e.g. we can add a AccelClass.cpu_accel_ops field, and call: cpus_register_accel(current_machine->accelerator->cpu_accel_ops); -- Eduardo
Re: Property '.hmat' not found
On Wed, Nov 18, 2020 at 06:16:36PM +0800, 郭俊甫 (Jack Kuo) wrote: > > > > Could you check what happens if you use > > "-machine pc-i440fx-5.2,hmat=on" instead of "-machine hmat=on"? > > > It failed, but `-machine pc-i440fx-4.2` worked without `hmat` option. > So, the problem could be that I didn't fully compile QEMU, which specified > `--target-list=x86_64-softmmu` in the configure command. > > Thus, I re-compile QEMU without any additional arg, then the problem is > solved! > It's strange that `-machine pc,help` showed me `pc-i440fx-5.2` but actually > loaded the `pc-i440fx-4.2`. Yeah, it is strange. Were the "-version" and "-machine pc,help" commands executed on a different shell process? If you have installed a new qemu-system-x86_64 binary in a separate location in $PATH (e.g. /usr/local/bin), existing shell processes might still run the old binary if the old path was already in their hash table. > > BTW, originally, I thought that `-hmat-cache` is the arg for customizing > CPU cache, and now I find that I'm wrong... It's for memory side cache. Correct. Unfortunately, there are not many CPU cache configuration options in qemu-system-x86_64 today, except for "l3-cache" and "host-cache-info" -cpu options. > > Thank you very much, really help me a lot. No problem! > > Sincerely, > Jack > > Eduardo Habkost 於 2020年11月17日 週二 下午11:09寫道： > > > On Tue, Nov 17, 2020 at 11:49:38AM +0800, 郭俊甫 (Jack Kuo) wrote: > > > > > > > > Do you have the output of `qemu-system-x86_64 -version`, and more > > > > information on how your binary was built? > > > > > > > > > I have 2 machines, both of them encounter the .hmat problem. > > > > > > (1) *5.1.90 (v5.2.0-rc0*) > > > - follow this wiki [1], and configure cmd: `./configure > > > --target-list=x86_64-softmmu` > > > - i5-8400, Ubuntu 20.04.1 LTS, kernel: 5.4.0-53-generic > > > > This one is supposed to work, and I see the pc-i440fx-5.2.hmat > > property below. > > > > > > > > > > (2) *4.2.1 (Debian 1:4.2-3ubuntu6.8)* > > > > This one isn't expect to have "hmat", as the feature was > > introduced in v5.0.0. > > > > > - use apt install qemu > > > - E5-2670 * 2, Ubuntu 20.04.1 LTS, kernel: 5.4.0-53-generic, Dell > > > PowerEdge-R720 > > > > > > I can't reproduce it here. I've tested both qemu.git master > > > > > > > > > That’s strange, also I try the command you (Eduardo) provided, '.hmat' > > not > > > found, too. > > > Does the kernel module matter? or PC architecture? > > > > Kernel or host architecture shouldn't matter. The choice of > > machine type matters, but the default is supposed to be "pc". > > The output of "-machine help" might be useful to debug it. > > > > Could you check what happens if you use > > "-machine pc-i440fx-5.2,hmat=on" instead of "-machine hmat=on"? > > > > > > > > > > Output of `-machine help` and `-machine pc,help` would be useful > > > > to debug it, too. > > > > > > > > > (1) *5.1.90 (i5-8400)* > > > *```* > > > $ qemu-system-x86_64 -machine pc,help > > > pc-i440fx-5.2.nvdimm-persistence=string (Set NVDIMM persistenceValid > > values > > > are cpu, mem-ctrl) > > > pc-i440fx-5.2.hmat=bool (Set on/off to enable/disable ACPI Heterogeneous > > > Memory Attribute Table (HMAT)) > > > pc-i440fx-5.2.nvdimm=bool (Set on/off to enable/disable NVDIMM > > > instantiation) > > > pc-i440fx-5.2.pcspk-audiodev=str (ID of an audiodev to use as a backend) > > > pc-i440fx-5.2.pflash1=str (Node name or ID of a block device to use as a > > > backend) > > > pc-i440fx-5.2.pflash0=str (Node name or ID of a block device to use as a > > > backend) > > > pc-i440fx-5.2.hpet=bool > > > pc-i440fx-5.2.vmport=OnOffAuto (Enable vmport (pc & q35)) > > > pc-i440fx-5.2.sata=bool > > > pc-i440fx-5.2.pit=bool > > > pc-i440fx-5.2.max-ram-below-4g=size (Maximum ram below the 4G boundary > > > (32bit boundary)) > > > pc-i440fx-5.2.smbus=bool > > > pc-i440fx-5.2.acpi=OnOffAuto (Enable ACPI) > > > pc-i440fx-5.2.smm=OnOffAuto (Enable SMM) > > > pc-i440fx-5.2.memory-backend=string (Set RAM backendValid value is ID of > > > hostmem based backend) > > > pc-i440fx-5.2.firmware=string (Firmware image) &g
Re: [PATCH v2 02/16] qapi/expr.py: Check for dict instead of OrderedDict
On Tue, Nov 17, 2020 at 05:30:04PM +0100, Markus Armbruster wrote: > John Snow writes: > > > OrderedDict is a subtype of dict, so we can check for a more general form. > > > > Signed-off-by: John Snow > > Reviewed-by: Eduardo Habkost > > Reviewed-by: Cleber Rosa > > --- > > scripts/qapi/expr.py | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > > index 35695c4c653b..5694c501fa38 100644 > > --- a/scripts/qapi/expr.py > > +++ b/scripts/qapi/expr.py > > @@ -14,7 +14,6 @@ > > # This work is licensed under the terms of the GNU GPL, version 2. > > # See the COPYING file in the top-level directory. > > > > -from collections import OrderedDict > > import re > > > > from .common import c_name > > @@ -131,7 +130,7 @@ def check_if_str(ifcond): > > > > > > def normalize_members(members): > > -if isinstance(members, OrderedDict): > > +if isinstance(members, dict): > > for key, arg in members.items(): > > if isinstance(arg, dict): > > continue > > @@ -162,7 +161,7 @@ def check_type(value, info, source, > > if not allow_dict: > > raise QAPISemError(info, "%s should be a type name" % source) > > > > -if not isinstance(value, OrderedDict): > > +if not isinstance(value, dict): > > raise QAPISemError(info, > > "%s should be an object or type name" % source) > > Plain dict remembers insertion order since Python 3.6, but it wasn't > formally promised until 3.7. > > Can we simply ditch OrderedDict entirely? In theory, our build requirement is "Python >= 3.6", not "CPython >= 3.6". In practice, I don't expect anybody to ever use any other Python implementation except CPython to build QEMU. I think we can get rid of OrderedDict if you really want to. -- Eduardo
Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
On Tue, Nov 17, 2020 at 08:53:19PM +0400, Marc-André Lureau wrote: > On Tue, Nov 17, 2020 at 7:49 PM Eduardo Habkost wrote: > > > On Tue, Nov 17, 2020 at 12:42:47PM +0400, Marc-André Lureau wrote: > > > On Tue, Nov 17, 2020 at 2:42 AM Eduardo Habkost > > wrote: > > > > > > > Extract the QNum value comparison logic to a function that takes > > > > QNumValue* as argument. > > > > > > > > Signed-off-by: Eduardo Habkost > > > > --- > > > > include/qapi/qmp/qnum.h | 1 + > > > > qobject/qnum.c | 29 +++-- > > > > 2 files changed, 20 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h > > > > index 62fbdfda68..0327ecd0f0 100644 > > > > --- a/include/qapi/qmp/qnum.h > > > > +++ b/include/qapi/qmp/qnum.h > > > > @@ -106,6 +106,7 @@ double qnum_get_double(const QNum *qn); > > > > > > > > char *qnum_to_string(QNum *qn); > > > > > > > > +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue > > *num_y); > > > > bool qnum_is_equal(const QObject *x, const QObject *y); > > > > void qnum_destroy_obj(QObject *obj); > > > > > > > > diff --git a/qobject/qnum.c b/qobject/qnum.c > > > > index f80d4efd76..6a0f948b16 100644 > > > > --- a/qobject/qnum.c > > > > +++ b/qobject/qnum.c > > > > @@ -207,9 +207,9 @@ char *qnum_to_string(QNum *qn) > > > > } > > > > > > > > /** > > > > - * qnum_is_equal(): Test whether the two QNums are equal > > > > - * @x: QNum object > > > > - * @y: QNum object > > > > + * qnum_value_is_equal(): Test whether two QNumValues are equal > > > > + * @num_x: QNum value > > > > + * @num_y: QNum value > > > > > > > > > > val_x: a QNumValue ? > > > > Do you mean: > > @num_x: a QNumValue > > > ? > > > > I was not planning to rename the existing num_x/num_y variables. > > > > > Not renaming because that would make some churn? But this is already quite > confusing, so it's better to use "val" for QNumVal and "num" for QNum I > guess. > > If you don't want to rename it in the code, may I suggest doing it at the > declaration side? Not sure it's a better idea. Yeah, I was not renaming them just to avoid churn. However, I'm already replacing `qn` variables with `qv` at patch 3/8. Replacing num_x/num_y with val_x/val_y at qnum_is_equal() (at patch 3/8 as well) wouldn't hurt too much. -- Eduardo
Re: [PATCH v2 4/8] qnum: qnum_value_is_equal() function
On Tue, Nov 17, 2020 at 12:42:47PM +0400, Marc-André Lureau wrote:
> On Tue, Nov 17, 2020 at 2:42 AM Eduardo Habkost wrote:
>
> > Extract the QNum value comparison logic to a function that takes
> > QNumValue* as argument.
> >
> > Signed-off-by: Eduardo Habkost
> > ---
> > include/qapi/qmp/qnum.h | 1 +
> > qobject/qnum.c | 29 +++--
> > 2 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> > index 62fbdfda68..0327ecd0f0 100644
> > --- a/include/qapi/qmp/qnum.h
> > +++ b/include/qapi/qmp/qnum.h
> > @@ -106,6 +106,7 @@ double qnum_get_double(const QNum *qn);
> >
> > char *qnum_to_string(QNum *qn);
> >
> > +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y);
> > bool qnum_is_equal(const QObject *x, const QObject *y);
> > void qnum_destroy_obj(QObject *obj);
> >
> > diff --git a/qobject/qnum.c b/qobject/qnum.c
> > index f80d4efd76..6a0f948b16 100644
> > --- a/qobject/qnum.c
> > +++ b/qobject/qnum.c
> > @@ -207,9 +207,9 @@ char *qnum_to_string(QNum *qn)
> > }
> >
> > /**
> > - * qnum_is_equal(): Test whether the two QNums are equal
> > - * @x: QNum object
> > - * @y: QNum object
> > + * qnum_value_is_equal(): Test whether two QNumValues are equal
> > + * @num_x: QNum value
> > + * @num_y: QNum value
> >
>
> val_x: a QNumValue ?
Do you mean:
@num_x: a QNumValue
?
I was not planning to rename the existing num_x/num_y variables.
>
> *
> > * Negative integers are never considered equal to unsigned integers,
> > * but positive integers in the range [0, INT64_MAX] are considered
> > @@ -217,13 +217,8 @@ char *qnum_to_string(QNum *qn)
> > *
> > * Doubles are never considered equal to integers.
> > */
> > -bool qnum_is_equal(const QObject *x, const QObject *y)
> > +bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y)
> > {
> > -const QNum *qnum_x = qobject_to(QNum, x);
> > -const QNum *qnum_y = qobject_to(QNum, y);
> > -const QNumValue *num_x = &qnum_x->value;
> > -const QNumValue *num_y = &qnum_y->value;
> > -
> > switch (num_x->kind) {
> > case QNUM_I64:
> > switch (num_y->kind) {
> > @@ -241,7 +236,7 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
> > case QNUM_U64:
> > switch (num_y->kind) {
> > case QNUM_I64:
> > -return qnum_is_equal(y, x);
> > +return qnum_value_is_equal(num_y, num_x);
> > case QNUM_U64:
> > /* Comparison in native uint64_t type */
> > return num_x->u.u64 == num_y->u.u64;
> > @@ -264,6 +259,20 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
> > abort();
> > }
> >
> > +/**
> > + * qnum_is_equal(): Test whether the two QNums are equal
> > + * @x: QNum object
> > + * @y: QNum object
> > + *
> > + * See qnum_value_is_equal() for details on the comparison rules.
> > + */
> > +bool qnum_is_equal(const QObject *x, const QObject *y)
> > +{
> > +const QNum *qnum_x = qobject_to(QNum, x);
> > +const QNum *qnum_y = qobject_to(QNum, y);
> > +return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
> > +}
> > +
> > /**
> > * qnum_destroy_obj(): Free all memory allocated by a QNum object
> > *
> > --
> > 2.28.0
> >
> >
> >
> beside that,
> Reviewed-by: Marc-André Lureau
Thanks!
--
Eduardo
Re: Property '.hmat' not found
te > dynamically) > pc-i440fx-4.2.igd-passthru=bool (Set on/off to enable/disable igd passthrou) > pc-i440fx-4.2.kvm-shadow-mem=int (KVM shadow MMU size) > pc-i440fx-4.2.dump-guest-core=bool (Include guest memory in a core dump) > pc-i440fx-4.2.memory-encryption=string (Set memory encryption object to use) > pc-i440fx-4.2.firmware=string (Firmware image) > pc-i440fx-4.2.usb=bool (Set on/off to enable/disable usb) > pc-i440fx-4.2.kernel-irqchip=on|off|split (Configure KVM in-kernel irqchip) > pc-i440fx-4.2.graphics=bool (Set on/off to enable/disable graphics > emulation) > pc-i440fx-4.2.initrd=string (Linux initial ramdisk file) > pc-i440fx-4.2.dt-compatible=string (Overrides the "compatible" property of > the dt root node) > pc-i440fx-4.2.kernel=string (Linux kernel image file) > pc-i440fx-4.2.enforce-config-section=bool (Set on to enforce configuration > section migration) > ``` > > [1] https://wiki.qemu.org/Hosts/Linux#Building_QEMU_for_Linux > > Sincerely, > Jack > > Eduardo Habkost 於 2020年11月17日 週二 上午3:02寫道： > > > On Mon, Nov 16, 2020 at 01:51:37PM +0100, Philippe Mathieu-Daudé wrote: > > > Cc'ing Igor & Eduardo. > > > > Thanks! > > > > > > > > On 11/13/20 10:17 AM, Jack wrote: > > > > Hi all, > > > > > > > > As I follow the document[1] to enable hmat, it fails and shows the > > message: > > > > qemu-system-x86_64: Property '.hmat' not found > > > > > > > > My QEMU version is 5.1.90 > > > > Do you have the output of `qemu-system-x86_64 -version`, and more > > information on how your binary was built? > > > > Output of `-machine help` and `-machine pc,help` would be useful > > to debug it, too. > > > > I can't reproduce it here. I've tested both qemu.git master > > (commit cb5ed407a1dd) and v5.2.0-rc (commit 3d6e32347a3b). > > > > [build/(cb5ed407a1...)]$ ./qemu-system-x86_64 -version > > QEMU emulator version 5.1.91 (v5.2.0-rc1-107-gcb5ed407a1) > > Copyright (c) 2003-2020 Fabrice Bellard and the QEMU Project developers > > [build/(cb5ed407a1...)]$ ./qemu-system-x86_64 -machine hmat=on -m 2G > > -object memory-backend-ram,size=1G,id=m0 -object > > memory-backend-ram,size=1G,id=m1 -smp 2 -numa node,nodeid=0,memdev=m0 -numa > > node,nodeid=1,memdev=m1,initiator=0 -numa cpu ,node-id=0,socket-id=0 -numa > > cpu,node-id=0,socket-id=1 -numa > > hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-latency,latency=5 > > -numa > > hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=200M > > -numa > > hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-latency,latency=10 > > -numa > > hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=100M > > -numa > > hmat-cache,node-id=0,size=10K,level=1,associativity=direct,policy=write-back,line=8 > > -numa > > hmat-cache,node-id=1,size=10K,level=1,associativity=direct,policy=write-back,line=8 > > -cpu host -vga virtio -accel kvm > > [VM boots] > > > > > > > > > > > > Does anyone know why? > > > > > > > > Here is my command: > > > > > > > > ``` > > > > $ sudo qemu-system-x86_64 \ > > > > -machine hmat=on \ > > > > -m 2G \ > > > > -object memory-backend-ram,size=1G,id=m0 \ > > > > -object memory-backend-ram,size=1G,id=m1 \ > > > > -smp 2 \ > > > > -numa node,nodeid=0,memdev=m0 \ > > > > -numa node,nodeid=1,memdev=m1,initiator=0 \ > > > > -numa cpu,node-id=0,socket-id=0 \ > > > > -numa cpu,node-id=0,socket-id=1 \ > > > > -numa > > > > > > hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-latency,latency=5 > > > > \ > > > > -numa > > > > > > hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=200M > > > > \ > > > > -numa > > > > > > hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-latency,latency=10 > > > > \ > > > > -numa > > > > > > hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=100M > > > > \ > > > > -numa > > > > > > hmat-cache,node-id=0,size=10K,level=1,associativity=direct,policy=write-back,line=8 > > > > \ > > > > -numa > > > > > > hmat-cache,node-id=1,size=10K,level=1,associativity=direct,policy=write-back,line=8 > > > > \ > > > > -cpu host \ > > > > -m 4g -vga virtio -hda ubuntu20.04.qcow2 \ > > > > -nic user,model=virtio \ > > > > -nic tap,model=e1000 \ > > > > -nic tap,model=e1000 \ > > > > -accel kvm > > > > ``` > > > > > > > > Thanks. > > > > > > > > [1] > > > > > > https://www.qemu.org/docs/master/system/qemu-manpage.html?highlight=numa > > > > > > > > > > > > Sincerely, > > > > Jack > > > > > > > > > > > -- > > Eduardo > > > > -- Eduardo
Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals
On Tue, Nov 17, 2020 at 12:37:56PM +0400, Marc-André Lureau wrote:
> On Tue, Nov 17, 2020 at 2:43 AM Eduardo Habkost wrote:
>
> > Provide a separate QNumValue type that can be used for QNum value
> > literals without the referencing counting and memory allocation
> > features provided by QObject.
> >
> > Signed-off-by: Eduardo Habkost
> > ---
> > Changes v1 -> v2:
> > * Fix "make check" failure, by updating check-qnum unit test to
> > use the new struct fields
> > ---
> > include/qapi/qmp/qnum.h | 40 +++--
> > qobject/qnum.c | 78 -
> > tests/check-qnum.c | 14
> > 3 files changed, 84 insertions(+), 48 deletions(-)
> >
> > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> > index 55c27b1c24..62fbdfda68 100644
> > --- a/include/qapi/qmp/qnum.h
> > +++ b/include/qapi/qmp/qnum.h
> > @@ -46,20 +46,56 @@ typedef enum {
> > * in range: qnum_get_try_int() / qnum_get_try_uint() check range and
> > * convert under the hood.
> > */
> > -struct QNum {
> > -struct QObjectBase_ base;
> > +
> > +/**
> > + * struct QNumValue: the value of a QNum
> > + *
> > + * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
> > + * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
> > + */
> > +typedef struct QNumValue {
> > +/* private: */
> > QNumKind kind;
> > union {
> > int64_t i64;
> > uint64_t u64;
> > double dbl;
> > } u;
> > +} QNumValue;
> > +
> > +#define QNUM_VAL_INT(value) \
> > +{ .kind = QNUM_I64, .u.i64 = value }
> > +#define QNUM_VAL_UINT(value) \
> > +{ .kind = QNUM_U64, .u.u64 = value }
> > +#define QNUM_VAL_DOUBLE(value) \
> > +{ .kind = QNUM_DOUBLE, .u.dbl = value }
> > +
> > +struct QNum {
> > +struct QObjectBase_ base;
> > +QNumValue value;
> > };
> >
> > +/**
> > + * qnum_from_int(): Create a new QNum from a QNumValue
> >
>
> Copy-pasta qnum_from_int() -> qnum_from_value()
Oops! I will fix it in v3, or submit a fixup patch if that's the
only needed change.
>
> I wonder if there is a check for that kind of mistake, as it may be common.
It looks like kernel-doc ignores what's before the colon in the
summary line. It probably shouldn't.
>
> + * @value: QNumValue
> > + *
> > + * Return strong reference.
> > + */
> > +QNum *qnum_from_value(QNumValue value);
> >
> +
> > QNum *qnum_from_int(int64_t value);
> > QNum *qnum_from_uint(uint64_t value);
> > QNum *qnum_from_double(double value);
> >
> > +/**
> > + * qnum_get_value(): Get QNumValue from QNum
> > + * @qn: QNum object
> > + */
> > +static inline const QNumValue *qnum_get_value(const QNum *qn)
> > +{
> > +return &qn->value;
> > +}
> > +
> >
>
> Nothing uses that function in this patch. Should be put into use.
It is used in patch 5/8. Why do you think it's necessary to use
it in internal code too?
>
> bool qnum_get_try_int(const QNum *qn, int64_t *val);
> > int64_t qnum_get_int(const QNum *qn);
> >
> > diff --git a/qobject/qnum.c b/qobject/qnum.c
> > index 69fd9a82d9..f80d4efd76 100644
> > --- a/qobject/qnum.c
> > +++ b/qobject/qnum.c
> > @@ -15,6 +15,15 @@
> > #include "qemu/osdep.h"
> > #include "qapi/qmp/qnum.h"
> >
> > +QNum *qnum_from_value(QNumValue value)
> >
>
> I wonder if it shouldn't be made "inline" in header too (if that can help
> avoid argument copy).
I'm unsure. I would make it inline (in a separate patch) if
there's evidence it's worth it. I expect the g_new() call to be
more expensive than any argument copying, though.
>
> +{
> > +QNum *qn = g_new(QNum, 1);
> > +
> > +qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > +qn->value = value;
> > +return qn;
> > +}
> > +
> > /**
> > * qnum_from_int(): Create a new QNum from an int64_t
> > * @value: int64_t value
> > @@ -23,13 +32,7 @@
> > */
> > QNum *qnum_from_int(int64_t value)
> > {
> > -QNum *qn = g_new(QNum, 1);
> > -
> > -qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > -qn->kind = QNUM_I64;
> > -qn->u.i64 = value;
> > -
> > -return qn;
> > +return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
> > }
> >
> > /**
> &
[PATCH v2 6/8] qlit: qlit_type() function
Useful function where we need to check for the qlit type before
converting it to an actual QObject.
Signed-off-by: Eduardo Habkost
---
include/qapi/qmp/qlit.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
index f9e356d31e..acddb80831 100644
--- a/include/qapi/qmp/qlit.h
+++ b/include/qapi/qmp/qlit.h
@@ -59,4 +59,9 @@ bool qlit_equal_qobject(const QLitObject *lhs, const QObject
*rhs);
QObject *qobject_from_qlit(const QLitObject *qlit);
+static inline QType qlit_type(const QLitObject *qlit)
+{
+return qlit->type;
+}
+
#endif /* QLIT_H */
--
2.28.0
[PATCH v2 5/8] qlit: Support all types of QNums
Use QNumValue to represent QNums, so we can also support uint64_t
and double QNum values. Add new QLIT_QNUM_(INT|UINT|DOUBLE)
macros for each case.
The QLIT_QNUM() macro is being kept for compatibility with
existing code, but becomes just a wrapper for QLIT_QNUM_INT().
Signed-off-by: Eduardo Habkost
---
Changes v1 -> v2:
* Coding style fix at qlit_equal_qobject()
---
include/qapi/qmp/qlit.h | 11 +--
qobject/qlit.c | 5 +--
tests/check-qjson.c | 72 ++---
3 files changed, 79 insertions(+), 9 deletions(-)
diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
index c0676d5daf..f9e356d31e 100644
--- a/include/qapi/qmp/qlit.h
+++ b/include/qapi/qmp/qlit.h
@@ -15,6 +15,7 @@
#define QLIT_H
#include "qobject.h"
+#include "qnum.h"
typedef struct QLitDictEntry QLitDictEntry;
typedef struct QLitObject QLitObject;
@@ -23,7 +24,7 @@ struct QLitObject {
QType type;
union {
bool qbool;
-int64_t qnum;
+QNumValue qnum;
const char *qstr;
QLitDictEntry *qdict;
QLitObject *qlist;
@@ -39,8 +40,14 @@ struct QLitDictEntry {
{ .type = QTYPE_QNULL }
#define QLIT_QBOOL(val) \
{ .type = QTYPE_QBOOL, .value.qbool = (val) }
+#define QLIT_QNUM_INT(val) \
+{ .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_INT(val) }
+#define QLIT_QNUM_UINT(val) \
+{ .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_UINT(val) }
+#define QLIT_QNUM_DOUBLE(val) \
+{ .type = QTYPE_QNUM, .value.qnum = QNUM_VAL_DOUBLE(val) }
#define QLIT_QNUM(val) \
-{ .type = QTYPE_QNUM, .value.qnum = (val) }
+QLIT_QNUM_INT(val)
#define QLIT_QSTR(val) \
{ .type = QTYPE_QSTRING, .value.qstr = (val) }
#define QLIT_QDICT(val) \
diff --git a/qobject/qlit.c b/qobject/qlit.c
index be8332136c..b23cdc4532 100644
--- a/qobject/qlit.c
+++ b/qobject/qlit.c
@@ -71,7 +71,8 @@ bool qlit_equal_qobject(const QLitObject *lhs, const QObject
*rhs)
case QTYPE_QBOOL:
return lhs->value.qbool == qbool_get_bool(qobject_to(QBool, rhs));
case QTYPE_QNUM:
-return lhs->value.qnum == qnum_get_int(qobject_to(QNum, rhs));
+return qnum_value_is_equal(&lhs->value.qnum,
+ qnum_get_value(qobject_to(QNum, rhs)));
case QTYPE_QSTRING:
return (strcmp(lhs->value.qstr,
qstring_get_str(qobject_to(QString, rhs))) == 0);
@@ -94,7 +95,7 @@ QObject *qobject_from_qlit(const QLitObject *qlit)
case QTYPE_QNULL:
return QOBJECT(qnull());
case QTYPE_QNUM:
-return QOBJECT(qnum_from_int(qlit->value.qnum));
+return QOBJECT(qnum_from_value(qlit->value.qnum));
case QTYPE_QSTRING:
return QOBJECT(qstring_from_str(qlit->value.qstr));
case QTYPE_QDICT: {
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 07a773e653..711030cffd 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -796,20 +796,23 @@ static void simple_number(void)
int i;
struct {
const char *encoded;
+QLitObject qlit;
int64_t decoded;
int skip;
} test_cases[] = {
-{ "0", 0 },
-{ "1234", 1234 },
-{ "1", 1 },
-{ "-32", -32 },
-{ "-0", 0, .skip = 1 },
+{ "0",QLIT_QNUM(0),0, },
+{ "1234", QLIT_QNUM(1234), 1234, },
+{ "1",QLIT_QNUM(1),1, },
+{ "-32", QLIT_QNUM(-32), -32, },
+{ "-0", QLIT_QNUM(0),0, .skip = 1 },
{ },
};
for (i = 0; test_cases[i].encoded; i++) {
QNum *qnum;
int64_t val;
+QNum *qlit_num;
+int64_t qlit_val;
qnum = qobject_to(QNum,
qobject_from_json(test_cases[i].encoded,
@@ -817,6 +820,7 @@ static void simple_number(void)
g_assert(qnum);
g_assert(qnum_get_try_int(qnum, &val));
g_assert_cmpint(val, ==, test_cases[i].decoded);
+
if (test_cases[i].skip == 0) {
QString *str;
@@ -826,9 +830,66 @@ static void simple_number(void)
}
qobject_unref(qnum);
+
+qlit_num = qobject_to(QNum,
+ qobject_from_qlit(&test_cases[i].qlit));
+g_assert(qlit_num);
+g_assert(qnum_get_try_int(qlit_num, &qlit_val));
+g_assert_cmpint(qlit_val, ==, test_cases[i].decoded);
+
+qobject_unref(qlit_num);
}
}
+static void qlit_large_number(void)
+{
+QLitObject maxu64 = QLIT_QNUM_UINT(UINT64_MAX);
+QLitObject maxi64 = QLIT_QNUM(INT64_MAX);
+QLitObject mini64 = QLIT_QNUM(INT64_MIN);
+QLitObject gtu64 = QLIT_QNUM_DOUBLE(18446744073709552e3);
+QLitObject lti64 = QLIT_QNUM_DOUBLE(-92233720368547758e2);
+QNum *qnum;
+uint64_t val;
+int64_t ival;
+
+qnum = qobject_to
[PATCH v2 8/8] qom: Use qlit to represent property defaults
Using QLitObject lets us get rid of most of the
.set_default_value functions, and just use
object_property_set_default() directly.
Signed-off-by: Eduardo Habkost
---
Changes v1 -> v2:
* Instead of initializing defval to QLIT_QNULL
by default, just check for QTYPE_NONE, to find out if .defval
was explicitly set. This avoids extra complexity at
set_prop_arraylen().
---
include/hw/qdev-properties-system.h | 2 +-
include/qom/field-property-internal.h | 4 ---
include/qom/field-property.h | 26 ---
include/qom/property-types.h | 19 ++
hw/core/qdev-properties-system.c | 8 --
qom/field-property.c | 27 ++--
qom/property-types.c | 36 ---
7 files changed, 42 insertions(+), 80 deletions(-)
diff --git a/include/hw/qdev-properties-system.h
b/include/hw/qdev-properties-system.h
index 0ac327ae60..a586424a33 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -65,7 +65,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
#define DEFINE_PROP_UUID(_name, _state, _field) \
DEFINE_PROP(_name, _state, _field, qdev_prop_uuid, QemuUUID, \
-.set_default = true)
+.defval = QLIT_QSTR("auto"))
#define DEFINE_PROP_AUDIODEV(_n, _s, _f) \
DEFINE_PROP(_n, _s, _f, qdev_prop_audiodev, QEMUSoundCard)
diff --git a/include/qom/field-property-internal.h
b/include/qom/field-property-internal.h
index a7b7e2b69d..9bc29e9b67 100644
--- a/include/qom/field-property-internal.h
+++ b/include/qom/field-property-internal.h
@@ -15,10 +15,6 @@ void field_prop_set_enum(Object *obj, Visitor *v, const char
*name,
void field_prop_set_default_value_enum(ObjectProperty *op,
const Property *prop);
-void field_prop_set_default_value_int(ObjectProperty *op,
- const Property *prop);
-void field_prop_set_default_value_uint(ObjectProperty *op,
- const Property *prop);
void field_prop_get_int32(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp);
diff --git a/include/qom/field-property.h b/include/qom/field-property.h
index 0cb1fe2217..3cfd19cc14 100644
--- a/include/qom/field-property.h
+++ b/include/qom/field-property.h
@@ -6,6 +6,7 @@
#include "qom/object.h"
#include "qapi/util.h"
+#include "qapi/qmp/qlit.h"
/**
* struct Property: definition of a field property
@@ -27,21 +28,8 @@ struct Property {
const PropertyInfo *info;
ptrdiff_toffset;
uint8_t bitnr;
-/**
- * @set_default: true if the default value should be set from @defval,
- *in which case @info->set_default_value must not be NULL
- *(if false then no default value is set by the property system
- * and the field retains whatever value it was given by instance_init).
- */
-bool set_default;
-/**
- * @defval: default value for the property. This is used only if
@set_default
- * is true.
- */
-union {
-int64_t i;
-uint64_t u;
-} defval;
+/** @defval: If not QTYPE_NONE, the default value for the property */
+QLitObject defval;
/* private: */
int arrayoffset;
const PropertyInfo *arrayinfo;
@@ -61,7 +49,13 @@ struct PropertyInfo {
const QEnumLookup *enum_table;
/** @print: String formatting function, for the human monitor */
int (*print)(Object *obj, Property *prop, char *dest, size_t len);
-/** @set_default_value: Callback for initializing the default value */
+/**
+ * @set_default_value: Optional callback for initializing the default value
+ *
+ * Most property types don't need to set this, as by default
+ * object_property_set_default() is called with the value at
+ * Property.defval.
+ */
void (*set_default_value)(ObjectProperty *op, const Property *prop);
/** @create: Optional callback for creation of property */
ObjectProperty *(*create)(ObjectClass *oc, const char *name,
diff --git a/include/qom/property-types.h b/include/qom/property-types.h
index 3132ddafd9..869d1a993a 100644
--- a/include/qom/property-types.h
+++ b/include/qom/property-types.h
@@ -5,6 +5,7 @@
#define QOM_PROPERTY_TYPES_H
#include "qom/field-property.h"
+#include "qapi/qmp/qlit.h"
extern const PropertyInfo prop_info_bit;
extern const PropertyInfo prop_info_bit64;
@@ -25,34 +26,29 @@ extern const PropertyInfo prop_info_link;
#define PROP_SIGNED(_state, _field, _defval, _prop, _type, ...) \
FIELD_PROP(_state, _field, _prop, _type,\
- .set_default = true, \
- .defval.i= (_type)_defval, \
+ .defval = QLIT_QNUM_INT((_
[PATCH v2 0/8] qom: Use qlit to represent property defaults
Based-on: 20201104160021.2342108-1-ehabk...@redhat.com Git branch: https://gitlab.com/ehabkost/qemu/-/commits/work/qdev-qlit-defaults This extend qlit.h to support all QNum types (signed int, unsigned int, and double), and use QLitObject to represent field property defaults. It allows us to get rid of most type-specific .set_default_value functions for QOM property types. Changes v1 -> v2: * Rebase to latest version of field properties series * Fix unit test failure * Coding style changes Eduardo Habkost (8): qobject: Include API docs in docs/devel/qobject.html qnum: Make qnum_get_double() get const pointer qnum: QNumValue type for QNum value literals qnum: qnum_value_is_equal() function qlit: Support all types of QNums qlit: qlit_type() function qom: Make object_property_set_default() public qom: Use qlit to represent property defaults docs/devel/index.rst | 1 + docs/devel/qobject.rst| 11 +++ include/hw/qdev-properties-system.h | 2 +- include/qapi/qmp/qlit.h | 16 +++- include/qapi/qmp/qnum.h | 47 ++- include/qapi/qmp/qobject.h| 48 +++ include/qom/field-property-internal.h | 4 - include/qom/field-property.h | 26 +++--- include/qom/object.h | 11 +++ include/qom/property-types.h | 19 ++--- hw/core/qdev-properties-system.c | 8 -- qobject/qlit.c| 5 +- qobject/qnum.c| 116 +++--- qom/field-property.c | 27 -- qom/object.c | 2 +- qom/property-types.c | 36 ++-- tests/check-qjson.c | 72 ++-- tests/check-qnum.c| 14 ++-- 18 files changed, 301 insertions(+), 164 deletions(-) create mode 100644 docs/devel/qobject.rst -- 2.28.0
[PATCH v2 7/8] qom: Make object_property_set_default() public
The function will be used outside qom/object.c, to simplify the
field property code that sets the property default value.
Signed-off-by: Eduardo Habkost
---
include/qom/object.h | 11 +++
qom/object.c | 2 +-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/include/qom/object.h b/include/qom/object.h
index 2ab124b8f0..4234cc9b66 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1090,6 +1090,17 @@ ObjectProperty *object_class_property_add(ObjectClass
*klass, const char *name,
ObjectPropertyRelease *release,
void *opaque);
+/**
+ * object_property_set_default:
+ * @prop: the property to set
+ * @value: the value to be written to the property
+ *
+ * Set the property default value.
+ *
+ * Ownership of @value is transferred to the property.
+ */
+void object_property_set_default(ObjectProperty *prop, QObject *value);
+
/**
* object_property_set_default_bool:
* @prop: the property to set
diff --git a/qom/object.c b/qom/object.c
index 7c11bcd3b1..6b0d9d8c79 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1547,7 +1547,7 @@ static void object_property_init_defval(Object *obj,
ObjectProperty *prop)
visit_free(v);
}
-static void object_property_set_default(ObjectProperty *prop, QObject *defval)
+void object_property_set_default(ObjectProperty *prop, QObject *defval)
{
assert(!prop->defval);
assert(!prop->init);
--
2.28.0
[PATCH v2 1/8] qobject: Include API docs in docs/devel/qobject.html
Render existing doc comments at docs/devel/qobject.html.
Signed-off-by: Eduardo Habkost
---
docs/devel/index.rst | 1 +
docs/devel/qobject.rst | 11 +
include/qapi/qmp/qnum.h| 4 +++-
include/qapi/qmp/qobject.h | 48 +-
qobject/qnum.c | 19 ---
5 files changed, 63 insertions(+), 20 deletions(-)
create mode 100644 docs/devel/qobject.rst
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index f10ed77e4c..1cb39a9384 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -35,3 +35,4 @@ Contents:
clocks
qom
block-coroutine-wrapper
+ qobject
diff --git a/docs/devel/qobject.rst b/docs/devel/qobject.rst
new file mode 100644
index 00..4f192ced7c
--- /dev/null
+++ b/docs/devel/qobject.rst
@@ -0,0 +1,11 @@
+QObject API
+===
+
+.. kernel-doc:: include/qapi/qmp/qobject.h
+
+QNum module
+---
+
+.. kernel-doc:: include/qapi/qmp/qnum.h
+
+.. kernel-doc:: qobject/qnum.c
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index bbae0a5ec8..25f4733efc 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -23,7 +23,9 @@ typedef enum {
QNUM_DOUBLE
} QNumKind;
-/*
+/**
+ * DOC:
+ *
* QNum encapsulates how our dialect of JSON fills in the blanks left
* by the JSON specification (RFC 8259) regarding numbers.
*
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index fcfd549220..bdc33bdb65 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -1,5 +1,5 @@
/*
- * QEMU Object Model.
+ * QObject API
*
* Based on ideas by Avi Kivity
*
@@ -10,24 +10,31 @@
*
* This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
* See the COPYING.LIB file in the top-level directory.
+ */
+
+/**
+ * DOC: QObject Reference Counts Terminology
*
- * QObject Reference Counts Terminology
- *
+ * Returning references
+ *
*
- * - Returning references: A function that returns an object may
- * return it as either a weak or a strong reference. If the
- * reference is strong, you are responsible for calling
- * qobject_unref() on the reference when you are done.
+ * A function that returns an object may return it as either a
+ * weak or a strong reference. If the reference is strong, you
+ * are responsible for calling qobject_unref() on the reference
+ * when you are done.
*
- * If the reference is weak, the owner of the reference may free it at
- * any time in the future. Before storing the reference anywhere, you
- * should call qobject_ref() to make the reference strong.
+ * If the reference is weak, the owner of the reference may free it at
+ * any time in the future. Before storing the reference anywhere, you
+ * should call qobject_ref() to make the reference strong.
*
- * - Transferring ownership: when you transfer ownership of a reference
- * by calling a function, you are no longer responsible for calling
- * qobject_unref() when the reference is no longer needed. In other words,
- * when the function returns you must behave as if the reference to the
- * passed object was weak.
+ * Transferring ownership
+ * --
+ *
+ * When you transfer ownership of a reference by calling a
+ * function, you are no longer responsible for calling
+ * qobject_unref() when the reference is no longer needed. In
+ * other words, when the function returns you must behave as if
+ * the reference to the passed object was weak.
*/
#ifndef QOBJECT_H
#define QOBJECT_H
@@ -81,6 +88,8 @@ static inline void qobject_ref_impl(QObject *obj)
/**
* qobject_is_equal(): Return whether the two objects are equal.
+ * @x: QObject pointer
+ * @y: QObject pointer
*
* Any of the pointers may be NULL; return true if both are. Always
* return false if only one is (therefore a QNull object is not
@@ -90,6 +99,7 @@ bool qobject_is_equal(const QObject *x, const QObject *y);
/**
* qobject_destroy(): Free resources used by the object
+ * @obj: QObject pointer
*/
void qobject_destroy(QObject *obj);
@@ -103,6 +113,7 @@ static inline void qobject_unref_impl(QObject *obj)
/**
* qobject_ref(): Increment QObject's reference count
+ * @obj: QObject pointer
*
* Returns: the same @obj. The type of @obj will be propagated to the
* return type.
@@ -115,12 +126,14 @@ static inline void qobject_unref_impl(QObject *obj)
/**
* qobject_unref(): Decrement QObject's reference count, deallocate
- * when it reaches zero
+ * when it reaches zero
+ * @obj: QObject pointer
*/
#define qobject_unref(obj) qobject_unref_impl(QOBJECT(obj))
/**
* qobject_type(): Return the QObject's type
+ * @obj: QObject pointer
*/
static inline QType qobject_type(const QObject *obj)
{
@@ -130,6 +143,9 @@ static inline QType qobject_type(const QObject *obj)
/**
* qobject_check_type(): Helper f
[PATCH v2 2/8] qnum: Make qnum_get_double() get const pointer
qnum_get_double() won't change the object, the argument can be
const.
Signed-off-by: Eduardo Habkost
---
include/qapi/qmp/qnum.h | 2 +-
qobject/qnum.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index 25f4733efc..55c27b1c24 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -66,7 +66,7 @@ int64_t qnum_get_int(const QNum *qn);
bool qnum_get_try_uint(const QNum *qn, uint64_t *val);
uint64_t qnum_get_uint(const QNum *qn);
-double qnum_get_double(QNum *qn);
+double qnum_get_double(const QNum *qn);
char *qnum_to_string(QNum *qn);
diff --git a/qobject/qnum.c b/qobject/qnum.c
index 017c8aa739..69fd9a82d9 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -154,7 +154,7 @@ uint64_t qnum_get_uint(const QNum *qn)
*
* qnum_get_double() loses precision for integers beyond 53 bits.
*/
-double qnum_get_double(QNum *qn)
+double qnum_get_double(const QNum *qn)
{
switch (qn->kind) {
case QNUM_I64:
--
2.28.0
[PATCH v2 3/8] qnum: QNumValue type for QNum value literals
Provide a separate QNumValue type that can be used for QNum value
literals without the referencing counting and memory allocation
features provided by QObject.
Signed-off-by: Eduardo Habkost
---
Changes v1 -> v2:
* Fix "make check" failure, by updating check-qnum unit test to
use the new struct fields
---
include/qapi/qmp/qnum.h | 40 +++--
qobject/qnum.c | 78 -
tests/check-qnum.c | 14
3 files changed, 84 insertions(+), 48 deletions(-)
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index 55c27b1c24..62fbdfda68 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -46,20 +46,56 @@ typedef enum {
* in range: qnum_get_try_int() / qnum_get_try_uint() check range and
* convert under the hood.
*/
-struct QNum {
-struct QObjectBase_ base;
+
+/**
+ * struct QNumValue: the value of a QNum
+ *
+ * QNumValue literals can be constructed using the `QNUM_VAL_INT`,
+ * `QNUM_VAL_UINT`, and `QNUM_VAL_DOUBLE` macros.
+ */
+typedef struct QNumValue {
+/* private: */
QNumKind kind;
union {
int64_t i64;
uint64_t u64;
double dbl;
} u;
+} QNumValue;
+
+#define QNUM_VAL_INT(value) \
+{ .kind = QNUM_I64, .u.i64 = value }
+#define QNUM_VAL_UINT(value) \
+{ .kind = QNUM_U64, .u.u64 = value }
+#define QNUM_VAL_DOUBLE(value) \
+{ .kind = QNUM_DOUBLE, .u.dbl = value }
+
+struct QNum {
+struct QObjectBase_ base;
+QNumValue value;
};
+/**
+ * qnum_from_int(): Create a new QNum from a QNumValue
+ * @value: QNumValue
+ *
+ * Return strong reference.
+ */
+QNum *qnum_from_value(QNumValue value);
+
QNum *qnum_from_int(int64_t value);
QNum *qnum_from_uint(uint64_t value);
QNum *qnum_from_double(double value);
+/**
+ * qnum_get_value(): Get QNumValue from QNum
+ * @qn: QNum object
+ */
+static inline const QNumValue *qnum_get_value(const QNum *qn)
+{
+return &qn->value;
+}
+
bool qnum_get_try_int(const QNum *qn, int64_t *val);
int64_t qnum_get_int(const QNum *qn);
diff --git a/qobject/qnum.c b/qobject/qnum.c
index 69fd9a82d9..f80d4efd76 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -15,6 +15,15 @@
#include "qemu/osdep.h"
#include "qapi/qmp/qnum.h"
+QNum *qnum_from_value(QNumValue value)
+{
+QNum *qn = g_new(QNum, 1);
+
+qobject_init(QOBJECT(qn), QTYPE_QNUM);
+qn->value = value;
+return qn;
+}
+
/**
* qnum_from_int(): Create a new QNum from an int64_t
* @value: int64_t value
@@ -23,13 +32,7 @@
*/
QNum *qnum_from_int(int64_t value)
{
-QNum *qn = g_new(QNum, 1);
-
-qobject_init(QOBJECT(qn), QTYPE_QNUM);
-qn->kind = QNUM_I64;
-qn->u.i64 = value;
-
-return qn;
+return qnum_from_value((QNumValue) QNUM_VAL_INT(value));
}
/**
@@ -40,13 +43,7 @@ QNum *qnum_from_int(int64_t value)
*/
QNum *qnum_from_uint(uint64_t value)
{
-QNum *qn = g_new(QNum, 1);
-
-qobject_init(QOBJECT(qn), QTYPE_QNUM);
-qn->kind = QNUM_U64;
-qn->u.u64 = value;
-
-return qn;
+return qnum_from_value((QNumValue) QNUM_VAL_UINT(value));
}
/**
@@ -57,13 +54,7 @@ QNum *qnum_from_uint(uint64_t value)
*/
QNum *qnum_from_double(double value)
{
-QNum *qn = g_new(QNum, 1);
-
-qobject_init(QOBJECT(qn), QTYPE_QNUM);
-qn->kind = QNUM_DOUBLE;
-qn->u.dbl = value;
-
-return qn;
+return qnum_from_value((QNumValue) QNUM_VAL_DOUBLE(value));
}
/**
@@ -75,15 +66,17 @@ QNum *qnum_from_double(double value)
*/
bool qnum_get_try_int(const QNum *qn, int64_t *val)
{
-switch (qn->kind) {
+const QNumValue *qv = &qn->value;
+
+switch (qv->kind) {
case QNUM_I64:
-*val = qn->u.i64;
+*val = qv->u.i64;
return true;
case QNUM_U64:
-if (qn->u.u64 > INT64_MAX) {
+if (qv->u.u64 > INT64_MAX) {
return false;
}
-*val = qn->u.u64;
+*val = qv->u.u64;
return true;
case QNUM_DOUBLE:
return false;
@@ -116,15 +109,17 @@ int64_t qnum_get_int(const QNum *qn)
*/
bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
{
-switch (qn->kind) {
+const QNumValue *qv = &qn->value;
+
+switch (qv->kind) {
case QNUM_I64:
-if (qn->u.i64 < 0) {
+if (qv->u.i64 < 0) {
return false;
}
-*val = qn->u.i64;
+*val = qv->u.i64;
return true;
case QNUM_U64:
-*val = qn->u.u64;
+*val = qv->u.u64;
return true;
case QNUM_DOUBLE:
return false;
@@ -156,13 +151,15 @@ uint64_t qnum_get_uint(const QNum *qn)
*/
double qnum_get_double(const QNum *qn)
{
-switch (qn->kind) {
+const QNumValue *qv = &qn->value;
+
+switch (qv->kind) {
case QNUM_I64:
-return qn->u.i64;
+return qv->u.
[PATCH v2 4/8] qnum: qnum_value_is_equal() function
Extract the QNum value comparison logic to a function that takes
QNumValue* as argument.
Signed-off-by: Eduardo Habkost
---
include/qapi/qmp/qnum.h | 1 +
qobject/qnum.c | 29 +++--
2 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index 62fbdfda68..0327ecd0f0 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -106,6 +106,7 @@ double qnum_get_double(const QNum *qn);
char *qnum_to_string(QNum *qn);
+bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y);
bool qnum_is_equal(const QObject *x, const QObject *y);
void qnum_destroy_obj(QObject *obj);
diff --git a/qobject/qnum.c b/qobject/qnum.c
index f80d4efd76..6a0f948b16 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -207,9 +207,9 @@ char *qnum_to_string(QNum *qn)
}
/**
- * qnum_is_equal(): Test whether the two QNums are equal
- * @x: QNum object
- * @y: QNum object
+ * qnum_value_is_equal(): Test whether two QNumValues are equal
+ * @num_x: QNum value
+ * @num_y: QNum value
*
* Negative integers are never considered equal to unsigned integers,
* but positive integers in the range [0, INT64_MAX] are considered
@@ -217,13 +217,8 @@ char *qnum_to_string(QNum *qn)
*
* Doubles are never considered equal to integers.
*/
-bool qnum_is_equal(const QObject *x, const QObject *y)
+bool qnum_value_is_equal(const QNumValue *num_x, const QNumValue *num_y)
{
-const QNum *qnum_x = qobject_to(QNum, x);
-const QNum *qnum_y = qobject_to(QNum, y);
-const QNumValue *num_x = &qnum_x->value;
-const QNumValue *num_y = &qnum_y->value;
-
switch (num_x->kind) {
case QNUM_I64:
switch (num_y->kind) {
@@ -241,7 +236,7 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
case QNUM_U64:
switch (num_y->kind) {
case QNUM_I64:
-return qnum_is_equal(y, x);
+return qnum_value_is_equal(num_y, num_x);
case QNUM_U64:
/* Comparison in native uint64_t type */
return num_x->u.u64 == num_y->u.u64;
@@ -264,6 +259,20 @@ bool qnum_is_equal(const QObject *x, const QObject *y)
abort();
}
+/**
+ * qnum_is_equal(): Test whether the two QNums are equal
+ * @x: QNum object
+ * @y: QNum object
+ *
+ * See qnum_value_is_equal() for details on the comparison rules.
+ */
+bool qnum_is_equal(const QObject *x, const QObject *y)
+{
+const QNum *qnum_x = qobject_to(QNum, x);
+const QNum *qnum_y = qobject_to(QNum, y);
+return qnum_value_is_equal(&qnum_x->value, &qnum_y->value);
+}
+
/**
* qnum_destroy_obj(): Free all memory allocated by a QNum object
*
--
2.28.0
Re: [PATCH for-6.0 1/6] qapi: Add query-accel command
On Mon, Nov 16, 2020 at 10:20:04AM -0600, Eric Blake wrote:
> On 11/16/20 7:10 AM, Roman Bolshakov wrote:
> > There's a problem for management applications to determine if certain
> > accelerators available. Generic QMP command should help with that.
> >
> > Signed-off-by: Roman Bolshakov
> > ---
> > monitor/qmp-cmds.c | 15 +++
> > qapi/machine.json | 19 +++
> > 2 files changed, 34 insertions(+)
> >
>
> > +++ b/qapi/machine.json
> > @@ -591,6 +591,25 @@
> > ##
> > { 'command': 'query-kvm', 'returns': 'KvmInfo' }
> >
> > +##
> > +# @query-accel:
> > +#
> > +# Returns information about an accelerator
> > +#
> > +# Returns: @KvmInfo
> > +#
> > +# Since: 6.0.0
>
> We're inconsistent on whether we have 'Since: x.y' or 'Since: x.y.z',
> although I prefer the shorter form. Maybe Markus has an opnion on that.
>
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "query-accel", "arguments": { "name": "kvm" } }
> > +# <- { "return": { "enabled": true, "present": true } }
> > +#
> > +##
> > +{ 'command': 'query-accel',
> > + 'data': { 'name': 'str' },
> > + 'returns': 'KvmInfo' }
>
> '@name' is undocumented and an open-coded string. Better would be
> requiring 'name' to be one of an enum type. [...]
This seem similar to CPU models, machine types, device types, and
backend object types: the set of valid values is derived from the
list of subtypes of a QOM type. We don't duplicate lists of QOM
types in the QAPI schema, today.
Do we want to duplicate the list of accelerators in the QAPI
schema, or should we wait for a generic solution that works for
any QOM type?
> [...] Even better would be
> returning an array of KvmInfo with information on all supported
> accelerators at once, rather than making the user call this command once
> per name.
Maybe. It would save us the work of answering the question
above, but is this (querying information for all accelerators at
once) going to be a common use case?
--
Eduardo
Re: Property '.hmat' not found
On Mon, Nov 16, 2020 at 01:51:37PM +0100, Philippe Mathieu-Daudé wrote: > Cc'ing Igor & Eduardo. Thanks! > > On 11/13/20 10:17 AM, Jack wrote: > > Hi all, > > > > As I follow the document[1] to enable hmat, it fails and shows the message: > > qemu-system-x86_64: Property '.hmat' not found > > > > My QEMU version is 5.1.90 Do you have the output of `qemu-system-x86_64 -version`, and more information on how your binary was built? Output of `-machine help` and `-machine pc,help` would be useful to debug it, too. I can't reproduce it here. I've tested both qemu.git master (commit cb5ed407a1dd) and v5.2.0-rc (commit 3d6e32347a3b). [build/(cb5ed407a1...)]$ ./qemu-system-x86_64 -version QEMU emulator version 5.1.91 (v5.2.0-rc1-107-gcb5ed407a1) Copyright (c) 2003-2020 Fabrice Bellard and the QEMU Project developers [build/(cb5ed407a1...)]$ ./qemu-system-x86_64 -machine hmat=on -m 2G -object memory-backend-ram,size=1G,id=m0 -object memory-backend-ram,size=1G,id=m1 -smp 2 -numa node,nodeid=0,memdev=m0 -numa node,nodeid=1,memdev=m1,initiator=0 -numa cpu ,node-id=0,socket-id=0 -numa cpu,node-id=0,socket-id=1 -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-latency,latency=5 -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=200M -numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-latency,latency=10 -numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=100M -numa hmat-cache,node-id=0,size=10K,level=1,associativity=direct,policy=write-back,line=8 -numa hmat-cache,node-id=1,size=10K,level=1,associativity=direct,policy=write-back,line=8 -cpu host -vga virtio -accel kvm [VM boots] > > > > Does anyone know why? > > > > Here is my command: > > > > ``` > > $ sudo qemu-system-x86_64 \ > > -machine hmat=on \ > > -m 2G \ > > -object memory-backend-ram,size=1G,id=m0 \ > > -object memory-backend-ram,size=1G,id=m1 \ > > -smp 2 \ > > -numa node,nodeid=0,memdev=m0 \ > > -numa node,nodeid=1,memdev=m1,initiator=0 \ > > -numa cpu,node-id=0,socket-id=0 \ > > -numa cpu,node-id=0,socket-id=1 \ > > -numa > > hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-latency,latency=5 > > \ > > -numa > > hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=200M > > \ > > -numa > > hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-latency,latency=10 > > \ > > -numa > > hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=100M > > \ > > -numa > > hmat-cache,node-id=0,size=10K,level=1,associativity=direct,policy=write-back,line=8 > > \ > > -numa > > hmat-cache,node-id=1,size=10K,level=1,associativity=direct,policy=write-back,line=8 > > \ > > -cpu host \ > > -m 4g -vga virtio -hda ubuntu20.04.qcow2 \ > > -nic user,model=virtio \ > > -nic tap,model=e1000 \ > > -nic tap,model=e1000 \ > > -accel kvm > > ``` > > > > Thanks. > > > > [1] > > https://www.qemu.org/docs/master/system/qemu-manpage.html?highlight=numa > > > > > > Sincerely, > > Jack > > > -- Eduardo
[PATCH v3 53/53] sev: Use class properties
Instance properties make introspection hard and are not shown by "-object ...,help". Convert them to class properties. Signed-off-by: Eduardo Habkost --- This is a new patch added in v3 of this series. There was another version of this patch inside series: Subject: [PATCH 00/12] qom: Make all -object types use only class properties Message-Id: <20201009160122.1662082-1-ehabk...@redhat.com> https://lore.kernel.org/qemu-devel/20201009160122.1662082-1-ehabk...@redhat.com Changes from v1 of the patch: * Use new object_class_property_add_field() API to replace object_property_add_uint32_ptr(). (v1 used object_class_property_add_uint32_ptr(), which won't exist anymore) --- Cc: Paolo Bonzini Cc: Richard Henderson Cc: Eduardo Habkost Cc: qemu-devel@nongnu.org --- target/i386/sev.c | 25 ++--- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/target/i386/sev.c b/target/i386/sev.c index 93c4d60b82..264ea5a7fb 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -28,7 +28,7 @@ #include "sysemu/runstate.h" #include "trace.h" #include "migration/blocker.h" -#include "qom/object.h" +#include "qom/qom.h" #define TYPE_SEV_GUEST "sev-guest" OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST) @@ -298,6 +298,19 @@ sev_guest_class_init(ObjectClass *oc, void *data) sev_guest_set_session_file); object_class_property_set_description(oc, "session-file", "guest owners session parameters (encoded with base64)"); + +object_class_property_add_field(oc, "policy", +PROP_UINT32(SevGuestState, policy, DEFAULT_GUEST_POLICY), +prop_allow_set_always); +object_class_property_add_field(oc, "handle", +PROP_UINT32(SevGuestState, handle, 0), +prop_allow_set_always); +object_class_property_add_field(oc, "cbitpos", +PROP_UINT32(SevGuestState, cbitpos, 0), +prop_allow_set_always); +object_class_property_add_field(oc, "reduced-phys-bits", +PROP_UINT32(SevGuestState, reduced_phys_bits, 0), +prop_allow_set_always); } static void @@ -306,16 +319,6 @@ sev_guest_instance_init(Object *obj) SevGuestState *sev = SEV_GUEST(obj); sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE); -sev->policy = DEFAULT_GUEST_POLICY; -object_property_add_uint32_ptr(obj, "policy", &sev->policy, - OBJ_PROP_FLAG_READWRITE); -object_property_add_uint32_ptr(obj, "handle", &sev->handle, - OBJ_PROP_FLAG_READWRITE); -object_property_add_uint32_ptr(obj, "cbitpos", &sev->cbitpos, - OBJ_PROP_FLAG_READWRITE); -object_property_add_uint32_ptr(obj, "reduced-phys-bits", - &sev->reduced_phys_bits, - OBJ_PROP_FLAG_READWRITE); } /* sev guest info */ -- 2.28.0
[PATCH v3 51/53] qom: PROP_* macros
The new helper macros are similar to the old DEFINE_PROP_* macros, but don't take a name argument. They can be used directly as argument to object_class_property_add_field(). The DEFINE_PROP_* macros were redefined to just be wrappers to PROP_*. Signed-off-by: Eduardo Habkost --- Changes v2 -> v3: * Now the DEFINE_PROP_* macros are defined using PROP_*, not the other way around * Remove unused macros (PROP_UNSIGNED*, PROP_SIGNED*, PROP_UUID) * Removed PROP_ARRAY because it isn't as trivial as the others * Now PROP_* won't return a pointer to a static variable anymore, but just a compound literal for a Property struct. This is a new patch added in v2 of the series --- Cc: Paolo Bonzini Cc: "Daniel P. Berrangé" Cc: Eduardo Habkost Cc: qemu-devel@nongnu.org --- include/qom/property-types.h | 141 ++- 1 file changed, 90 insertions(+), 51 deletions(-) diff --git a/include/qom/property-types.h b/include/qom/property-types.h index 91b166badf..3132ddafd9 100644 --- a/include/qom/property-types.h +++ b/include/qom/property-types.h @@ -23,6 +23,64 @@ extern const PropertyInfo prop_info_size32; extern const PropertyInfo prop_info_arraylen; extern const PropertyInfo prop_info_link; +#define PROP_SIGNED(_state, _field, _defval, _prop, _type, ...) \ +FIELD_PROP(_state, _field, _prop, _type,\ + .set_default = true, \ + .defval.i= (_type)_defval, \ + __VA_ARGS__) + +#define PROP_UNSIGNED(_state, _field, _defval, _prop, _type, ...) \ +FIELD_PROP(_state, _field, _prop, _type,\ + .set_default = true, \ + .defval.u = (_type)_defval, \ + __VA_ARGS__) + +#define PROP_BIT(_state, _field, _bit, _defval, ...) \ +FIELD_PROP(_state, _field, prop_info_bit, uint32_t, \ + .bitnr = (_bit), \ + .set_default = true, \ + .defval.u= (bool)_defval,\ + __VA_ARGS__) + +#define PROP_BIT64(_state, _field, _bit, _defval, ...) \ +FIELD_PROP(_state, _field, prop_info_bit64, uint64_t, \ + .bitnr= (_bit), \ + .set_default = true, \ + .defval.u = (bool)_defval, \ + __VA_ARGS__) + +#define PROP_BOOL(_state, _field, _defval, ...) \ +FIELD_PROP(_state, _field, prop_info_bool, bool,\ + .set_default = true, \ + .defval.u= (bool)_defval,\ + __VA_ARGS__) + +#define PROP_LINK(_state, _field, _type, _ptr_type, ...) \ +FIELD_PROP(_state, _field, prop_info_link, _ptr_type, \ + .link_type = _type, \ + __VA_ARGS__) + +#define PROP_UINT8(_s, _f, _d, ...) \ +PROP_UNSIGNED(_s, _f, _d, prop_info_uint8, uint8_t, __VA_ARGS__) +#define PROP_UINT16(_s, _f, _d, ...) \ +PROP_UNSIGNED(_s, _f, _d, prop_info_uint16, uint16_t, __VA_ARGS__) +#define PROP_UINT32(_s, _f, _d, ...) \ +PROP_UNSIGNED(_s, _f, _d, prop_info_uint32, uint32_t, __VA_ARGS__) +#define PROP_INT32(_s, _f, _d, ...) \ +PROP_SIGNED(_s, _f, _d, prop_info_int32, int32_t, __VA_ARGS__) +#define PROP_UINT64(_s, _f, _d, ...) \ +PROP_UNSIGNED(_s, _f, _d, prop_info_uint64, uint64_t, __VA_ARGS__) +#define PROP_INT64(_s, _f, _d, ...) \ +PROP_SIGNED(_s, _f, _d, prop_info_int64, int64_t, __VA_ARGS__) +#define PROP_SIZE(_s, _f, _d, ...) \ +PROP_UNSIGNED(_s, _f, _d, prop_info_size, uint64_t, __VA_ARGS__) +#define PROP_STRING(_s, _f, ...) \ +FIELD_PROP(_s, _f, prop_info_string, char*, __VA_ARGS__) +#define PROP_ON_OFF_AUTO(_s, _f, _d, ...) \ +PROP_SIGNED(_s, _f, _d, prop_info_on_off_auto, OnOffAuto, __VA_ARGS__) +#define PROP_SIZE32(_s, _f, _d, ...) \ +PROP_UNSIGNED(_s, _f, _d, prop_info_size32, uint32_t, __VA_ARGS__) + /** * DEFINE_PROP: Define a #Property struct, including a property name * @@ -43,33 +101,6 @@ extern const PropertyInfo prop_info_link; .name_template = (_name), \ __VA_ARGS__) -#define DEFINE_PROP_SIGNED(_name, _state, _field, _defval, _prop, _type) \ -DEFINE_PROP(_name, _state, _field, _prop, _type, \ -.set_default = true, \ -.defval.i= (_type)_defval) - -#define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) \ -DEFINE_PROP(_name, _state, _field, prop_info_bit, uint32_t, \ -.bitnr = (_bit), \ -.set_default = true,\ -
[PATCH v3 50/53] qom: Delete DEFINE_PROP_*SIGNED_NODEFAULT macro
Those macros are exactly the same as DEFINE_PROP, we can use
DEFINE_PROP directly.
Signed-off-by: Eduardo Habkost
---
This is a new patch added in v3 of this series.
---
Cc: Alex Williamson
Cc: Paolo Bonzini
Cc: "Daniel P. Berrangé"
Cc: Eduardo Habkost
Cc: Peter Maydell
Cc: qemu-devel@nongnu.org
Cc: qemu-...@nongnu.org
---
include/qom/property-types.h | 6 --
hw/vfio/pci.c| 6 +++---
target/arm/cpu.c | 6 +++---
3 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/include/qom/property-types.h b/include/qom/property-types.h
index 62551c77e0..91b166badf 100644
--- a/include/qom/property-types.h
+++ b/include/qom/property-types.h
@@ -48,9 +48,6 @@ extern const PropertyInfo prop_info_link;
.set_default = true, \
.defval.i= (_type)_defval)
-#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) \
-DEFINE_PROP(_name, _state, _field, _prop, _type)
-
#define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) \
DEFINE_PROP(_name, _state, _field, prop_info_bit, uint32_t, \
.bitnr = (_bit), \
@@ -62,9 +59,6 @@ extern const PropertyInfo prop_info_link;
.set_default = true, \
.defval.u = (_type)_defval)
-#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type) \
-DEFINE_PROP(_name, _state, _field, _prop, _type)
-
#define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) \
DEFINE_PROP(_name, _state, _field, prop_info_bit64, uint64_t, \
.bitnr= (_bit), \
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index edfaed8c9a..f15abacbdf 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3214,9 +3214,9 @@ static Property vfio_pci_dev_properties[] = {
DEFINE_PROP_UINT32("x-pci-sub-device-id", VFIOPCIDevice,
sub_device_id, PCI_ANY_ID),
DEFINE_PROP_UINT32("x-igd-gms", VFIOPCIDevice, igd_gms, 0),
-DEFINE_PROP_UNSIGNED_NODEFAULT("x-nv-gpudirect-clique", VFIOPCIDevice,
- nv_gpudirect_clique,
- qdev_prop_nv_gpudirect_clique, uint8_t),
+DEFINE_PROP("x-nv-gpudirect-clique", VFIOPCIDevice,
+nv_gpudirect_clique,
+qdev_prop_nv_gpudirect_clique, uint8_t),
DEFINE_PROP_OFF_AUTO_PCIBAR("x-msix-relocation", VFIOPCIDevice, msix_relo,
OFF_AUTOPCIBAR_OFF),
/*
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index cef92879b0..7cf2078622 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1106,9 +1106,9 @@ static Property arm_cpu_has_mpu_property =
* to override that with an incorrect constant value.
*/
static Property arm_cpu_pmsav7_dregion_property =
-DEFINE_PROP_UNSIGNED_NODEFAULT("pmsav7-dregion", ARMCPU,
- pmsav7_dregion,
- prop_info_uint32, uint32_t);
+DEFINE_PROP("pmsav7-dregion", ARMCPU,
+pmsav7_dregion,
+prop_info_uint32, uint32_t);
static bool arm_get_pmu(Object *obj, Error **errp)
{
--
2.28.0
[PATCH v3 46/53] qdev: Move base property types to qom/property-types.c
Move all property types from qdev-properties.c to
qom/property-types.c. Declarations are moved from
hw/qdev-properties.h to qom/property-types.h.
Signed-off-by: Eduardo Habkost
---
Changes v2 -> v3:
* Redone after changes in previous patches in this series
* Reordered patch, tried to put it a bit earlier in the series
Changes v1 -> v2:
* Rebased after changes in previous patches in the series
---
Cc: Paolo Bonzini
Cc: "Daniel P. Berrangé"
Cc: Eduardo Habkost
Cc: qemu-devel@nongnu.org
---
include/hw/qdev-properties.h | 125 +---
include/qom/property-types.h | 131 +
include/qom/qom.h| 1 +
hw/core/qdev-properties.c| 538 --
qom/property-types.c | 546 +++
qom/meson.build | 1 +
6 files changed, 680 insertions(+), 662 deletions(-)
create mode 100644 include/qom/property-types.h
create mode 100644 qom/property-types.c
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 6585a8e693..939b6dbf4e 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -3,130 +3,7 @@
#include "hw/qdev-core.h"
#include "qom/field-property.h"
-
-/*** qdev-properties.c ***/
-
-extern const PropertyInfo prop_info_bit;
-extern const PropertyInfo prop_info_bit64;
-extern const PropertyInfo prop_info_bool;
-extern const PropertyInfo prop_info_enum;
-extern const PropertyInfo prop_info_uint8;
-extern const PropertyInfo prop_info_uint16;
-extern const PropertyInfo prop_info_uint32;
-extern const PropertyInfo prop_info_int32;
-extern const PropertyInfo prop_info_uint64;
-extern const PropertyInfo prop_info_int64;
-extern const PropertyInfo prop_info_size;
-extern const PropertyInfo prop_info_string;
-extern const PropertyInfo prop_info_on_off_auto;
-extern const PropertyInfo prop_info_size32;
-extern const PropertyInfo prop_info_arraylen;
-extern const PropertyInfo prop_info_link;
-
-#define DEFINE_PROP(_name, _state, _field, _prop, _type, ...) { \
-.name_template = (_name), \
-.info = &(_prop), \
-.offset= offsetof(_state, _field)\
-+ type_check(_type, typeof_field(_state, _field)), \
-__VA_ARGS__ \
-}
-
-#define DEFINE_PROP_SIGNED(_name, _state, _field, _defval, _prop, _type) \
-DEFINE_PROP(_name, _state, _field, _prop, _type, \
-.set_default = true, \
-.defval.i= (_type)_defval)
-
-#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) \
-DEFINE_PROP(_name, _state, _field, _prop, _type)
-
-#define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) \
-DEFINE_PROP(_name, _state, _field, prop_info_bit, uint32_t, \
-.bitnr = (_bit), \
-.set_default = true,\
-.defval.u= (bool)_defval)
-
-#define DEFINE_PROP_UNSIGNED(_name, _state, _field, _defval, _prop, _type) \
-DEFINE_PROP(_name, _state, _field, _prop, _type, \
-.set_default = true, \
-.defval.u = (_type)_defval)
-
-#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type) \
-DEFINE_PROP(_name, _state, _field, _prop, _type)
-
-#define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) \
-DEFINE_PROP(_name, _state, _field, prop_info_bit64, uint64_t, \
-.bitnr= (_bit), \
-.set_default = true, \
-.defval.u = (bool)_defval)
-
-#define DEFINE_PROP_BOOL(_name, _state, _field, _defval) \
-DEFINE_PROP(_name, _state, _field, prop_info_bool, bool, \
-.set_default = true, \
-.defval.u= (bool)_defval)
-
-#define PROP_ARRAY_LEN_PREFIX "len-"
-
-/**
- * DEFINE_PROP_ARRAY:
- * @_name: name of the array
- * @_state: name of the device state structure type
- * @_field: uint32_t field in @_state to hold the array length
- * @_arrayfield: field in @_state (of type '@_arraytype *') which
- * will point to the array
- * @_arrayprop: PropertyInfo defining what property the array elements have
- * @_arraytype: C type of the array elements
- *
- * Define device properties for a variable-length array _name. A
- * static property "len-arrayname" is defined. When the device creator
- * sets this property to the desired length of array, further dynamic
- * properties "arrayname[0]", "arrayname[1]", ... are defined so the
- * device creator can set the array element values. Setting the
- * "len-arrayname
[PATCH v3 52/53] tests: Use field property at check-qom-proplist test case
Use the field property system for the "sv" property used at
check-qom-proplist.
Signed-off-by: Eduardo Habkost
---
Changes v2 -> v3:
* Don't register a instance field property, as
object_property_add_field() is an internal API
Changes v1 -> v2:
* Use PROP_* and object_class_property_add_field() interface
---
Cc: Paolo Bonzini
Cc: "Daniel P. Berrangé"
Cc: Eduardo Habkost
Cc: qemu-devel@nongnu.org
---
tests/check-qom-proplist.c | 39 --
1 file changed, 4 insertions(+), 35 deletions(-)
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 1b76581980..2ded6203fb 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -25,7 +25,7 @@
#include "qemu/module.h"
#include "qemu/option.h"
#include "qemu/config-file.h"
-#include "qom/object_interfaces.h"
+#include "qom/qom.h"
#define TYPE_DUMMY "qemu-dummy"
@@ -103,26 +103,6 @@ static int dummy_get_av(Object *obj,
return dobj->av;
}
-
-static void dummy_set_sv(Object *obj,
- const char *value,
- Error **errp)
-{
-DummyObject *dobj = DUMMY_OBJECT(obj);
-
-g_free(dobj->sv);
-dobj->sv = g_strdup(value);
-}
-
-static char *dummy_get_sv(Object *obj,
- Error **errp)
-{
-DummyObject *dobj = DUMMY_OBJECT(obj);
-
-return g_strdup(dobj->sv);
-}
-
-
static void dummy_init(Object *obj)
{
object_property_add_bool(obj, "bv",
@@ -130,12 +110,11 @@ static void dummy_init(Object *obj)
dummy_set_bv);
}
-
static void dummy_class_init(ObjectClass *cls, void *data)
{
-object_class_property_add_str(cls, "sv",
- dummy_get_sv,
- dummy_set_sv);
+object_class_property_add_field(cls, "sv",
+PROP_STRING(DummyObject, sv),
+prop_allow_set_always);
object_class_property_add_enum(cls, "av",
"DummyAnimal",
&dummy_animal_map,
@@ -143,21 +122,11 @@ static void dummy_class_init(ObjectClass *cls, void *data)
dummy_set_av);
}
-
-static void dummy_finalize(Object *obj)
-{
-DummyObject *dobj = DUMMY_OBJECT(obj);
-
-g_free(dobj->sv);
-}
-
-
static const TypeInfo dummy_info = {
.name = TYPE_DUMMY,
.parent= TYPE_OBJECT,
.instance_size = sizeof(DummyObject),
.instance_init = dummy_init,
-.instance_finalize = dummy_finalize,
.class_size = sizeof(DummyObjectClass),
.class_init = dummy_class_init,
.interfaces = (InterfaceInfo[]) {
--
2.28.0
[PATCH v3 44/53] qom: Add new qom.h header
Multiple QOM features will be provided by different headers. Add a qom/qom.h header to make it easier to include all of them at once. Signed-off-by: Eduardo Habkost --- This is a new patch added in v3 of the series. --- Cc: Paolo Bonzini Cc: "Daniel P. Berrangé" Cc: Eduardo Habkost Cc: qemu-devel@nongnu.org --- include/qom/qom.h | 10 ++ 1 file changed, 10 insertions(+) create mode 100644 include/qom/qom.h diff --git a/include/qom/qom.h b/include/qom/qom.h new file mode 100644 index 00..3286605083 --- /dev/null +++ b/include/qom/qom.h @@ -0,0 +1,10 @@ +/* + * QEMU Object Model convenience header + * + * This header is not supposed to be included by other headers + * but by .c files that use QOM. + */ + +#include "qom/object.h" +#include "qom/object_interfaces.h" +#include "qom/qom-qobject.h" -- 2.28.0
[PATCH v3 42/53] qdev: Reuse object_property_add_field() when adding array elements
Now that we call object_property_add() with exactly the same
arguments as object_property_add_field() does, we can just reuse
the function. We can now use a stack variable for the new
Property struct, because object_property_add_field() will copy
the struct.
Signed-off-by: Eduardo Habkost
---
Changes v2 -> v3:
* Fix memory leak from v2, after making
object_property_add_field() copy the Property struct
Changes v1 -> v2:
* Now we don't need to hack ObjectProperty.release anymore,
patch became trivial
---
Cc: Paolo Bonzini
Cc: "Daniel P. Berrangé"
Cc: Eduardo Habkost
Cc: qemu-devel@nongnu.org
---
hw/core/qdev-properties.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 451bb54cf6..83fd45add0 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -576,23 +576,17 @@ static void set_prop_arraylen(Object *obj, Visitor *v,
const char *name,
*arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
g_autofree char *propname = g_strdup_printf("%s[%d]", arrayname, i);
-Property *arrayprop = g_new0(Property, 1);
-ObjectProperty *elmop;
-arrayprop->info = prop->arrayinfo;
+Property arrayprop = { };
+arrayprop.info = prop->arrayinfo;
/* This ugly piece of pointer arithmetic sets up the offset so
* that when the underlying get/set hooks call qdev_get_prop_ptr
* they get the right answer despite the array element not actually
* being inside the device struct.
*/
-arrayprop->offset = eltptr - (void *)obj;
-assert(object_field_prop_ptr(obj, arrayprop) == eltptr);
-elmop = object_property_add(obj, propname,
-arrayprop->info->name,
-field_prop_getter(arrayprop->info),
-field_prop_setter(arrayprop->info),
-static_prop_release_dynamic_prop,
-arrayprop);
-elmop->allow_set = op->allow_set;
+arrayprop.offset = eltptr - (void *)obj;
+assert(object_field_prop_ptr(obj, &arrayprop) == eltptr);
+object_property_add_field(obj, propname, &arrayprop,
+ op->allow_set);
}
}
--
2.28.0
