date:20240111

We can simply use the g_ascii_isxdigit() from the glib instead.

Signed-off-by: Thomas Huth 
---
 util/uri.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/util/uri.c b/util/uri.c
index dcb3305236..7411c5ba14 100644
--- a/util/uri.c
+++ b/util/uri.c
@@ -1561,15 +1561,6 @@ done_cd:
 return 0;
 }
 
-static int is_hex(char c)
-{
-if (((c >= '0') && (c <= '9')) || ((c >= 'a') && (c <= 'f')) ||
-((c >= 'A') && (c <= 'F'))) {
-return 1;
-}
-return 0;
-}
-
 /**
  * uri_string_unescape:
  * @str:  the string to unescape
@@ -1607,7 +1598,7 @@ char *uri_string_unescape(const char *str, int len, char 
*target)
 in = str;
 out = ret;
 while (len > 0) {
-if ((len > 2) && (*in == '%') && (is_hex(in[1])) && (is_hex(in[2]))) {
+if (len > 2 && *in == '%' && g_ascii_isxdigit(in[1]) && 
g_ascii_isxdigit(in[2])) {
 in++;
 if ((*in >= '0') && (*in <= '9')) {
 *out = (*in - '0');
-- 
2.43.0

Re: [PATCH 1/2] target/ppc/cpu-models: Rename power5+ and power7+ for new QOM naming rules


On 12/01/2024 05.57, Harsh Prateek Bora wrote:



On 1/11/24 22:16, Thomas Huth wrote:

The character "+" is now forbidden in QOM device names (see commit
b447378e1217 - "Limit type names to alphanumerical and some few special
characters"). For the "power5+" and "power7+" CPU names, there is
currently a hack in type_name_is_valid() to still allow them for
compatibility reasons. However, there is a much nicer solution for this:
Simply use aliases! This way we can still support the old names without
the need for the ugly hack in type_name_is_valid().

Signed-off-by: Thomas Huth 
---
  hw/ppc/spapr_cpu_core.c |  4 ++--
  qom/object.c    |  4 
  target/ppc/cpu-models.c | 10 ++
  3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 5aa1ed474a..214b7a03d8 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -389,9 +389,9 @@ static const TypeInfo spapr_cpu_core_type_infos[] = {
  DEFINE_SPAPR_CPU_CORE_TYPE("970_v2.2"),
  DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.0"),
  DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.1"),
-    DEFINE_SPAPR_CPU_CORE_TYPE("power5+_v2.1"),
+    DEFINE_SPAPR_CPU_CORE_TYPE("power5plus_v2.1"),
  DEFINE_SPAPR_CPU_CORE_TYPE("power7_v2.3"),
-    DEFINE_SPAPR_CPU_CORE_TYPE("power7+_v2.1"),
+    DEFINE_SPAPR_CPU_CORE_TYPE("power7plus_v2.1"),


Will using Power5x, Power7x be a better naming than using 'plus' suffix ?


The "x" looks like a placeholder to me, so it could be confused with 
power50, power51, power52, etc. ...?
But actually, I was thinking about using "power5p" and "power7p" first, so 
if the whole "plus" looks too long for you, would "p" be an option instead?



Otherwise,
Reviewed-by: Harsh Prateek Bora 


Thanks!

 Thomas

Re: [PATCH v2 2/9] hw/hppa/machine: Disable default devices with --nodefaults option


On 11/01/2024 23.28, Helge Deller wrote:

On 1/9/24 17:01, Richard Henderson wrote:

On 1/9/24 22:16, Helge Deller wrote:

On 1/9/24 10:57, Richard Henderson wrote:

On 1/8/24 00:22, del...@kernel.org wrote:

From: Helge Deller 

Add support for the qemu --nodefaults option, which will disable the
following default devices:
- lsi53c895a SCSI controller,
- artist graphics card,
- LASI 82596 NIC,
- tulip PCI NIC,
- second serial PCI card,
- USB OHCI controller.

Adding this option is very useful to allow manual testing and
debugging of the other possible devices on the command line.

Signed-off-by: Helge Deller 
---
  hw/hppa/machine.c | 15 +--
  1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index b11907617e..8017002a2a 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -346,11 +346,14 @@ static void 
machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,

  SysBusDevice *s;
  /* SCSI disk setup. */
-    dev = DEVICE(pci_create_simple(pci_bus, -1, "lsi53c895a"));
-    lsi53c8xx_handle_legacy_cmdline(dev);
+    if (defaults_enabled()) {
+    dev = DEVICE(pci_create_simple(pci_bus, -1, "lsi53c895a"));
+    lsi53c8xx_handle_legacy_cmdline(dev);
+    }
  /* Graphics setup. */
-    if (machine->enable_graphics && vga_interface_type != VGA_NONE) {
+    if (defaults_enabled() && machine->enable_graphics &&
+    vga_interface_type != VGA_NONE) {
  vga_interface_created = true;
  dev = qdev_new("artist");
  s = SYS_BUS_DEVICE(dev);
@@ -360,7 +363,7 @@ static void 
machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,

  }
  /* Network setup. */
-    if (enable_lasi_lan()) {
+    if (defaults_enabled() && enable_lasi_lan()) {
  lasi_82596_init(addr_space, translate(NULL, LASI_LAN_HPA),
  qdev_get_gpio_in(lasi_dev, LASI_IRQ_LAN_HPA));
  }
@@ -385,7 +388,7 @@ static void 
machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
  pci_set_word(_dev->config[PCI_SUBSYSTEM_ID], 0x1227); /* 
Powerbar */

  /* create a second serial PCI card when running Astro */
-    if (!lasi_dev) {
+    if (defaults_enabled() && !lasi_dev) {
  pci_dev = pci_new(-1, "pci-serial-4x");
  qdev_prop_set_chr(DEVICE(pci_dev), "chardev1", serial_hd(1));
  qdev_prop_set_chr(DEVICE(pci_dev), "chardev2", serial_hd(2));
@@ -395,7 +398,7 @@ static void 
machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,

  }
  /* create USB OHCI controller for USB keyboard & mouse on Astro 
machines */

-    if (!lasi_dev && machine->enable_graphics) {
+    if (defaults_enabled() && !lasi_dev && machine->enable_graphics) {
  pci_create_simple(pci_bus, -1, "pci-ohci");
  usb_create_simple(usb_bus_find(-1), "usb-kbd");
  usb_create_simple(usb_bus_find(-1), "usb-mouse");


This almost doubles the uses of default_enabled in the entire tree.
I wonder if some of them are redundant or should be using a different
test.


Any proposal?
Maybe introduce a local variable hppa_bare_metal = !defaults_enabled();
and use that instead?


No, not like that.


Ok.


In casual review I am surprised that !defaults_enabled() does not
already imply !enable_graphics, unless the command-line goes on to
explicitly add a graphics device.
Am I missing something?


Will check that tommorow. If it does I'll remove that additional check.

But what other do you suggest in general how I should address your
concerns here?


IIRC enable_graphics is not influenced by --nodefaults, but it should be 
possible to simply check vga_interface_type only - that should get set to 
VGA_NONE when the user started QEMU with --nodefaults.


For networking, other boards normally check nd_table[0]. And for serial, you 
can check whether serial_hd(0) returns a non-NULL value.


For checking whether you have to create SCSI devices by default, you can 
check drive_get_max_bus(IF_SCSI), I think.


 HTH,
  Thomas

Re: [PATCH 1/2] target/ppc/cpu-models: Rename power5+ and power7+ for new QOM naming rules

2024-01-11 Thread Harsh Prateek Bora





On 1/11/24 22:16, Thomas Huth wrote:

The character "+" is now forbidden in QOM device names (see commit
b447378e1217 - "Limit type names to alphanumerical and some few special
characters"). For the "power5+" and "power7+" CPU names, there is
currently a hack in type_name_is_valid() to still allow them for
compatibility reasons. However, there is a much nicer solution for this:
Simply use aliases! This way we can still support the old names without
the need for the ugly hack in type_name_is_valid().

Signed-off-by: Thomas Huth 
---
  hw/ppc/spapr_cpu_core.c |  4 ++--
  qom/object.c|  4 
  target/ppc/cpu-models.c | 10 ++
  3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 5aa1ed474a..214b7a03d8 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -389,9 +389,9 @@ static const TypeInfo spapr_cpu_core_type_infos[] = {
  DEFINE_SPAPR_CPU_CORE_TYPE("970_v2.2"),
  DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.0"),
  DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.1"),
-DEFINE_SPAPR_CPU_CORE_TYPE("power5+_v2.1"),
+DEFINE_SPAPR_CPU_CORE_TYPE("power5plus_v2.1"),
  DEFINE_SPAPR_CPU_CORE_TYPE("power7_v2.3"),
-DEFINE_SPAPR_CPU_CORE_TYPE("power7+_v2.1"),
+DEFINE_SPAPR_CPU_CORE_TYPE("power7plus_v2.1"),


Will using Power5x, Power7x be a better naming than using 'plus' suffix ?

Otherwise,
Reviewed-by: Harsh Prateek Bora 


  DEFINE_SPAPR_CPU_CORE_TYPE("power8_v2.0"),
  DEFINE_SPAPR_CPU_CORE_TYPE("power8e_v2.1"),
  DEFINE_SPAPR_CPU_CORE_TYPE("power8nvl_v1.0"),
diff --git a/qom/object.c b/qom/object.c
index 654e1afaf2..2c4c64d2b6 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -160,10 +160,6 @@ static bool type_name_is_valid(const char *name)
  
  /* Allow some legacy names with '+' in it for compatibility reasons */

  if (name[plen] == '+') {
-if (plen == 6 && g_str_has_prefix(name, "power")) {
-/* Allow "power5+" and "power7+" CPU names*/
-return true;
-}
  if (plen >= 17 && g_str_has_prefix(name, "Sun-UltraSparc-I")) {
  /* Allow "Sun-UltraSparc-IV+" and "Sun-UltraSparc-IIIi+" */
  return true;
diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
index 7dbb47de64..6d854bb023 100644
--- a/target/ppc/cpu-models.c
+++ b/target/ppc/cpu-models.c
@@ -716,11 +716,11 @@
  "PowerPC 970MP v1.0")
  POWERPC_DEF("970mp_v1.1",CPU_POWERPC_970MP_v11,  970,
  "PowerPC 970MP v1.1")
-POWERPC_DEF("power5+_v2.1",  CPU_POWERPC_POWER5P_v21,POWER5P,
+POWERPC_DEF("power5plus_v2.1", CPU_POWERPC_POWER5P_v21,  POWER5P,
  "POWER5+ v2.1")
  POWERPC_DEF("power7_v2.3",   CPU_POWERPC_POWER7_v23, POWER7,
  "POWER7 v2.3")
-POWERPC_DEF("power7+_v2.1",  CPU_POWERPC_POWER7P_v21,POWER7,
+POWERPC_DEF("power7plus_v2.1", CPU_POWERPC_POWER7P_v21,  POWER7,
  "POWER7+ v2.1")
  POWERPC_DEF("power8e_v2.1",  CPU_POWERPC_POWER8E_v21,POWER8,
  "POWER8E v2.1")
@@ -902,10 +902,12 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
  { "970", "970_v2.2" },
  { "970fx", "970fx_v3.1" },
  { "970mp", "970mp_v1.1" },
-{ "power5+", "power5+_v2.1" },
+{ "power5+", "power5plus_v2.1" },
+{ "power5+_v2.1", "power5plus_v2.1" },
  { "power5gs", "power5+_v2.1" },
  { "power7", "power7_v2.3" },
-{ "power7+", "power7+_v2.1" },
+{ "power7+", "power7plus_v2.1" },
+{ "power7+_v2.1", "power7plus_v2.1" },
  { "power8e", "power8e_v2.1" },
  { "power8", "power8_v2.0" },
  { "power8nvl", "power8nvl_v1.0" },

[PATCH 0/4] target/xtensa: provide configuration with MPU

Hello,

this series adds xtensa core 'sample_controller32' with 32 foreground
entry MPU, adds missing translation for the 'wsr.mpucfg' opcode and
makes xtensa/tcg/tests work with the new core.

Max Filippov (4):
  target/xtensa: add translation for wsr.mpucfg
  target/xtensa: import sample_controller32 core
  tests/tcg/xtensa: tidy test linker script
  tests/tcg/xtensa: fix SR test for configs with MPU

 target/xtensa/core-sample_controller32.c  |52 +
 .../core-sample_controller32/core-isa.h   |   739 +
 .../core-sample_controller32/core-matmap.h|   106 +
 .../core-sample_controller32/gdb-config.c.inc |   144 +
 .../xtensa-modules.c.inc  | 11845 
 target/xtensa/cores.list  | 1 +
 target/xtensa/translate.c | 9 +
 tests/tcg/xtensa/linker.ld.S  |34 +-
 tests/tcg/xtensa/test_sr.S|16 +-
 9 files changed, 12921 insertions(+), 25 deletions(-)
 create mode 100644 target/xtensa/core-sample_controller32.c
 create mode 100644 target/xtensa/core-sample_controller32/core-isa.h
 create mode 100644 target/xtensa/core-sample_controller32/core-matmap.h
 create mode 100644 target/xtensa/core-sample_controller32/gdb-config.c.inc
 create mode 100644 target/xtensa/core-sample_controller32/xtensa-modules.c.inc

-- 
2.39.2

[PATCH 4/4] tests/tcg/xtensa: fix SR test for configs with MPU

- atomctl is available not only in the presence of s32c1i, but also with
  the exclusive access option
- cacheadrdis SR has the same number as cacheattr, mpuenb SR has the
  same number as rasid and mpucfg SR has the same number as dtlbcfg,
  add MPU case to the tests of these SR numbers

Signed-off-by: Max Filippov 
---
 tests/tcg/xtensa/test_sr.S | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tests/tcg/xtensa/test_sr.S b/tests/tcg/xtensa/test_sr.S
index 34441c7afff7..661ef6c66ed1 100644
--- a/tests/tcg/xtensa/test_sr.S
+++ b/tests/tcg/xtensa/test_sr.S
@@ -62,7 +62,7 @@ test_sr_mask /*acchi*/17, 0, 0
 test_sr_mask /*acclo*/16, 0, 0
 #endif
 
-#if XCHAL_HAVE_S32C1I && XCHAL_HW_VERSION >= 23
+#if XCHAL_HAVE_S32C1I && XCHAL_HW_VERSION >= 23 || XCHAL_HAVE_EXCLUSIVE
 test_sr atomctl, 1
 #else
 test_sr_mask /*atomctl*/99, 0, 0
@@ -74,7 +74,11 @@ test_sr br, 1
 test_sr_mask /*br*/4, 0, 0
 #endif
 
+#if XCHAL_HAVE_MPU
+test_sr cacheadrdis, 1
+#else
 test_sr_mask /*cacheattr*/98, 0, 0
+#endif
 
 #if XCHAL_HAVE_CCOUNT
 test_sr ccompare0, 1
@@ -106,6 +110,8 @@ test_sr depc, 1
 
 #if XCHAL_HAVE_PTP_MMU
 test_sr dtlbcfg, 1
+#elif XCHAL_HAVE_MPU
+test_sr_mask /*mpucfg*/92, 0, 3
 #else
 test_sr_mask /*dtlbcfg*/92, 0, 0
 #endif
@@ -205,9 +211,15 @@ test_sr ps, 1
 
 #if XCHAL_HAVE_PTP_MMU
 test_sr ptevaddr, 1
-test_sr rasid, 1
 #else
 test_sr_mask /*ptevaddr*/83, 0, 0
+#endif
+
+#if XCHAL_HAVE_PTP_MMU
+test_sr rasid, 1
+#elif XCHAL_HAVE_MPU
+test_sr mpuenb, 1
+#else
 test_sr_mask /*rasid*/90, 0, 0
 #endif
 
-- 
2.39.2

[PATCH 3/4] tests/tcg/xtensa: tidy test linker script

Drop MEMORY clause and related size definitions and output section
region specifications. Drop .rodata output section as the tests don't
use it. Add DATA_SEGMENT_ALIGN/DATA_SEGMENT_END around .data and .bss to
let the linker make an RW segment for data. Reserve 1M for stack instead
of almost 128M.

Signed-off-by: Max Filippov 
---
 tests/tcg/xtensa/linker.ld.S | 34 +++---
 1 file changed, 11 insertions(+), 23 deletions(-)

diff --git a/tests/tcg/xtensa/linker.ld.S b/tests/tcg/xtensa/linker.ld.S
index ac89b0054ee4..0e21eee31ccc 100644
--- a/tests/tcg/xtensa/linker.ld.S
+++ b/tests/tcg/xtensa/linker.ld.S
@@ -10,9 +10,8 @@
 #define XCHAL_WINDOW_UF12_VECOFS  0x0140
 #endif
 
-#define RAM_SIZE 0x0800  /* 128M */
-#define ROM_SIZE 0x1000  /* 4k */
 #define VECTORS_RESERVED_SIZE 0x1000
+#define STACK_SIZE 0x0010
 
 #if XCHAL_HAVE_BE
 OUTPUT_FORMAT("elf32-xtensa-be")
@@ -21,18 +20,13 @@ OUTPUT_FORMAT("elf32-xtensa-le")
 #endif
 ENTRY(_start)
 
-MEMORY {
-ram : ORIGIN = XCHAL_VECBASE_RESET_VADDR, LENGTH = RAM_SIZE
-rom : ORIGIN = XCHAL_RESET_VECTOR_VADDR, LENGTH = ROM_SIZE
-}
-
 SECTIONS
 {
-.init :
+.init XCHAL_RESET_VECTOR_VADDR :
 {
 *(.init)
 *(.init.*)
-} > rom
+}
 
 #if XCHAL_HAVE_WINDOWED
 .vector.window XCHAL_WINDOW_VECTORS_VADDR :
@@ -119,23 +113,16 @@ SECTIONS
 *(.vector.kernel.*)
 *(.vector.user.*)
 *(.vector.double.*)
-} > ram
+}
 
 .text :
 {
 _ftext = .;
 *(.text .stub .text.* .gnu.linkonce.t.* .literal .literal.*)
 _etext = .;
-} > ram
+}
 
-.rodata :
-{
-. = ALIGN(4);
-_frodata = .;
-*(.rodata .rodata.* .gnu.linkonce.r.*)
-*(.rodata1)
-_erodata = .;
-} > ram
+. = DATA_SEGMENT_ALIGN(0x1000, 0x1000);
 
 .data :
 {
@@ -146,7 +133,7 @@ SECTIONS
 _gp = ALIGN(16);
 *(.sdata .sdata.* .gnu.linkonce.s.*)
 _edata = .;
-} > ram
+}
 
 .bss :
 {
@@ -160,7 +147,8 @@ SECTIONS
 *(COMMON)
 _ebss = .;
 _end = .;
-} > ram
-}
+}
 
-PROVIDE(_fstack = (ORIGIN(ram) & 0xf000) + LENGTH(ram) - 16);
+. = DATA_SEGMENT_END(.);
+_fstack = ALIGN(. + STACK_SIZE, 16);
+}
-- 
2.39.2

[PATCH 1/4] target/xtensa: add translation for wsr.mpucfg

Although MPUCFG is not writable, the opcode wsr.mpucfg is defined and it
just does nothing. Define wsr.mpucfg as nop.

Signed-off-by: Max Filippov 
---
 target/xtensa/translate.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index de899405994e..6a2f5d308e6a 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -5332,6 +5332,15 @@ static const XtensaOpcodeOps core_ops[] = {
 XTENSA_OPTION_TRACE_PORT,
 },
 .op_flags = XTENSA_OP_PRIVILEGED,
+}, {
+.name = "wsr.mpucfg",
+.translate = translate_nop,
+.test_exceptions = test_exceptions_sr,
+.par = (const uint32_t[]){
+MPUCFG,
+XTENSA_OPTION_MPU,
+},
+.op_flags = XTENSA_OP_PRIVILEGED,
 }, {
 .name = "wsr.mpuenb",
 .translate = translate_wsr_mpuenb,
-- 
2.39.2

Re: [PATCH 0/2] Export debug triggers as an extension

On Thu, Jan 11, 2024 at 5:20 AM Daniel Henrique Barboza
 wrote:
>
> Himanshu,
>
> We spoke offline but let's make everyone aware:
>
> - 'sdtrig' should be marked with 'x-' and be an experimental extension since
> the spec isn't yet frozen;
>
> - Alvin sent a patch to the ML adding the 'mcontext' CSR for 'sdtrig' some 
> time
> ago:
>
> "[PATCH v2] target/riscv: Implement optional CSR mcontext of debug Sdtrig 
> extension"
>
> It would be good to put his patch on top of this series to ease the review 
> for everyone.
> The changes done in patch 2 would also be applicable to the mcontext CSR;
>
>
> - last but probably the most important: the existing 'debug' flag seems to be 
> acting as
> the actual 'sdtrig' extension due to how the flag is gating trigger code, 
> e.g.:
>
>if (cpu->cfg.debug) {
>  riscv_trigger_realize(>env);
>  }
>
> and
>
>  if (cpu->cfg.debug) {
>  riscv_trigger_reset_hold(env);
>  }
>
>
> If that's really the case, all the checks with cpu->cfg.debug will need to 
> also include
> cpu->cfg.ext_sdtrig (one or the other). And now we'll have to make an option: 
> do we leave
> the debug triggers (i.e. the 'debug' flag) as always enabled?

>From memory the "debug" property is for the original debug spec:
https://github.com/riscv/riscv-debug-spec/releases/tag/task_group_vote

That was ratified and is an official extension. AFAIK this is what is
in physical hardware as well.

The actual PDF says draft though, I'm not sure what's going on there.

The debug spec doesn't have a Z* name, so it's just "debug", at least AFAIK.

"sdtrig" seems to be a new backwards-incompatible extension doing
basically the same thing. What a mess

>
> If it's up to me I would make 'debug' as default 'false' and deprecate it. 
> Users will need

I don't think that's the right approach. It's a ratified extension
that we are supporting and is in hardware. I think we are stuck
supporting it

> to enable the debug triggers via x-sdtrig=true from now on. This will break 
> existing behavior,
> but the way it is now we're always enabling an extension (via the debug flag) 
> that isn't even
> frozen, so we're already in the wrong.

Maybe the debug group can chime in and say what they expect users to
do? It seems like there are conflicting specs

Alistair

Re: [PATCH 04/12] tests/plugin/inline: migrate to new per_vcpu API

2024-01-11 Thread Pierrick Bouvier


On 1/12/24 02:10, Richard Henderson wrote:

On 1/12/24 01:23, Pierrick Bouvier wrote:

Signed-off-by: Pierrick Bouvier 
---
   tests/plugin/inline.c | 17 -
   1 file changed, 17 deletions(-)


Was this supposed to be together with patch 6?



My goal was to have a version that still uses original API.
If you prefer this to be squashed, no problem to do it.


r~



diff --git a/tests/plugin/inline.c b/tests/plugin/inline.c
index 6114ebca545..ae59f7af7a7 100644
--- a/tests/plugin/inline.c
+++ b/tests/plugin/inline.c
@@ -18,15 +18,12 @@
   static uint64_t count_tb;
   static uint64_t count_tb_per_vcpu[MAX_CPUS];
   static uint64_t count_tb_inline_per_vcpu[MAX_CPUS];
-static uint64_t count_tb_inline_racy;
   static uint64_t count_insn;
   static uint64_t count_insn_per_vcpu[MAX_CPUS];
   static uint64_t count_insn_inline_per_vcpu[MAX_CPUS];
-static uint64_t count_insn_inline_racy;
   static uint64_t count_mem;
   static uint64_t count_mem_per_vcpu[MAX_CPUS];
   static uint64_t count_mem_inline_per_vcpu[MAX_CPUS];
-static uint64_t count_mem_inline_racy;
   static GMutex tb_lock;
   static GMutex insn_lock;
   static GMutex mem_lock;
@@ -50,11 +47,9 @@ static void stats_insn(void)
   printf("insn: %" PRIu64 "\n", expected);
   printf("insn: %" PRIu64 " (per vcpu)\n", per_vcpu);
   printf("insn: %" PRIu64 " (per vcpu inline)\n", inl_per_vcpu);
-printf("insn: %" PRIu64 " (inline racy)\n", count_insn_inline_racy);
   g_assert(expected > 0);
   g_assert(per_vcpu == expected);
   g_assert(inl_per_vcpu == expected);
-g_assert(count_insn_inline_racy <= expected);
   }
   
   static void stats_tb(void)

@@ -65,11 +60,9 @@ static void stats_tb(void)
   printf("tb: %" PRIu64 "\n", expected);
   printf("tb: %" PRIu64 " (per vcpu)\n", per_vcpu);
   printf("tb: %" PRIu64 " (per vcpu inline)\n", inl_per_vcpu);
-printf("tb: %" PRIu64 " (inline racy)\n", count_tb_inline_racy);
   g_assert(expected > 0);
   g_assert(per_vcpu == expected);
   g_assert(inl_per_vcpu == expected);
-g_assert(count_tb_inline_racy <= expected);
   }
   
   static void stats_mem(void)

@@ -80,11 +73,9 @@ static void stats_mem(void)
   printf("mem: %" PRIu64 "\n", expected);
   printf("mem: %" PRIu64 " (per vcpu)\n", per_vcpu);
   printf("mem: %" PRIu64 " (per vcpu inline)\n", inl_per_vcpu);
-printf("mem: %" PRIu64 " (inline racy)\n", count_mem_inline_racy);
   g_assert(expected > 0);
   g_assert(per_vcpu == expected);
   g_assert(inl_per_vcpu == expected);
-g_assert(count_mem_inline_racy <= expected);
   }
   
   static void plugin_exit(qemu_plugin_id_t id, void *udata)

@@ -142,8 +133,6 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct 
qemu_plugin_tb *tb)
   {
   qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
QEMU_PLUGIN_CB_NO_REGS, 0);
-qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
- _tb_inline_racy, 1);
   qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
   tb, QEMU_PLUGIN_INLINE_ADD_U64,
   count_tb_inline_per_vcpu, sizeof(uint64_t), 1);
@@ -152,18 +141,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct 
qemu_plugin_tb *tb)
   struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, idx);
   qemu_plugin_register_vcpu_insn_exec_cb(insn, vcpu_insn_exec,
  QEMU_PLUGIN_CB_NO_REGS, 0);
-qemu_plugin_register_vcpu_insn_exec_inline(
-insn, QEMU_PLUGIN_INLINE_ADD_U64,
-_insn_inline_racy, 1);
   qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(
   insn, QEMU_PLUGIN_INLINE_ADD_U64,
   count_insn_inline_per_vcpu, sizeof(uint64_t), 1);
   qemu_plugin_register_vcpu_mem_cb(insn, _mem_access,
QEMU_PLUGIN_CB_NO_REGS,
QEMU_PLUGIN_MEM_RW, 0);
-qemu_plugin_register_vcpu_mem_inline(insn, QEMU_PLUGIN_MEM_RW,
- QEMU_PLUGIN_INLINE_ADD_U64,
- _mem_inline_racy, 1);
   qemu_plugin_register_vcpu_mem_inline_per_vcpu(
   insn, QEMU_PLUGIN_MEM_RW,
   QEMU_PLUGIN_INLINE_ADD_U64,

Re: [PATCH v2 38/43] plugins: add an API to read registers

2024-01-11 Thread Pierrick Bouvier


On 1/3/24 21:33, Alex Bennée wrote:

We can only request a list of registers once the vCPU has been
initialised so the user needs to use either call the get function on
vCPU initialisation or during the translation phase.

We don't expose the reg number to the plugin instead hiding it behind
an opaque handle. This allows for a bit of future proofing should the
internals need to be changed while also being hashed against the
CPUClass so we can handle different register sets per-vCPU in
hetrogenous situations.

Having an internal state within the plugins also allows us to expand
the interface in future (for example providing callbacks on register
change if the translator can track changes).

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706
Cc: Akihiko Odaki 
Based-on: <20231025093128.33116-18-akihiko.od...@daynix.com>
Signed-off-by: Alex Bennée 

---
v3
   - also g_intern_string the register name
   - make get_registers documentation a bit less verbose
v2
   - use new get whole list api, and expose upwards

vAJB:

The main difference to Akikio's version is hiding the gdb register
detail from the plugin for the reasons described above.
---
  include/qemu/qemu-plugin.h   |  51 +-
  plugins/api.c| 102 +++
  plugins/qemu-plugins.symbols |   2 +
  3 files changed, 153 insertions(+), 2 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 4daab6efd29..95380895f81 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -11,6 +11,7 @@
  #ifndef QEMU_QEMU_PLUGIN_H
  #define QEMU_QEMU_PLUGIN_H
  
+#include 

  #include 
  #include 
  #include 
@@ -227,8 +228,8 @@ struct qemu_plugin_insn;
   * @QEMU_PLUGIN_CB_R_REGS: callback reads the CPU's regs
   * @QEMU_PLUGIN_CB_RW_REGS: callback reads and writes the CPU's regs
   *
- * Note: currently unused, plugins cannot read or change system
- * register state.
+ * Note: currently QEMU_PLUGIN_CB_RW_REGS is unused, plugins cannot change
+ * system register state.
   */
  enum qemu_plugin_cb_flags {
  QEMU_PLUGIN_CB_NO_REGS,
@@ -708,4 +709,50 @@ uint64_t qemu_plugin_end_code(void);
  QEMU_PLUGIN_API
  uint64_t qemu_plugin_entry_code(void);
  
+/** struct qemu_plugin_register - Opaque handle for register access */

+struct qemu_plugin_register;
+
+/**
+ * typedef qemu_plugin_reg_descriptor - register descriptions
+ *
+ * @handle: opaque handle for retrieving value with qemu_plugin_read_register
+ * @name: register name
+ * @feature: optional feature descriptor, can be NULL
+ */
+typedef struct {
+struct qemu_plugin_register *handle;
+const char *name;
+const char *feature;
+} qemu_plugin_reg_descriptor;
+
+/**
+ * qemu_plugin_get_registers() - return register list for vCPU
+ * @vcpu_index: vcpu to query
+ *
+ * Returns a GArray of qemu_plugin_reg_descriptor or NULL. Caller
+ * frees the array (but not the const strings).
+ *
+ * Should be used from a qemu_plugin_register_vcpu_init_cb() callback
+ * after the vCPU is initialised.
+ */
+GArray * qemu_plugin_get_registers(unsigned int vcpu_index);
+
+/**
+ * qemu_plugin_read_register() - read register
+ *
+ * @vcpu: vcpu index
+ * @handle: a @qemu_plugin_reg_handle handle
+ * @buf: A GByteArray for the data owned by the plugin
+ *
+ * This function is only available in a context that register read access is
+ * explicitly requested.
+ *
+ * Returns the size of the read register. The content of @buf is in target byte
+ * order. On failure returns -1
+ */
+int qemu_plugin_read_register(unsigned int vcpu,
+  struct qemu_plugin_register *handle,
+  GByteArray *buf);
+
+
  #endif /* QEMU_QEMU_PLUGIN_H */
diff --git a/plugins/api.c b/plugins/api.c
index ac39cdea0b3..f8905325c43 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -8,6 +8,7 @@
   *
   *  qemu_plugin_tb
   *  qemu_plugin_insn
+ *  qemu_plugin_register
   *
   * Which can then be passed back into the API to do additional things.
   * As such all the public functions in here are exported in
@@ -35,10 +36,12 @@
   */
  
  #include "qemu/osdep.h"

+#include "qemu/main-loop.h"
  #include "qemu/plugin.h"
  #include "qemu/log.h"
  #include "tcg/tcg.h"
  #include "exec/exec-all.h"
+#include "exec/gdbstub.h"
  #include "exec/ram_addr.h"
  #include "disas/disas.h"
  #include "plugin.h"
@@ -435,3 +438,102 @@ uint64_t qemu_plugin_entry_code(void)
  #endif
  return entry;
  }
+
+/*
+ * Register handles
+ *
+ * The plugin infrastructure keeps hold of these internal data
+ * structures which are presented to plugins as opaque handles. They
+ * are global to the system and therefor additions to the hash table
+ * must be protected by the @reg_handle_lock.
+ *
+ * In order to future proof for up-coming heterogeneous work we want
+ * different entries for each CPU type while sharing them in the
+ * common case of multiple cores of the same type.
+ */
+
+static QemuMutex reg_handle_lock;
+

Re: [PATCH 1/2] target/riscv: Export sdtrig as an extension and ISA string

On Wed, Jan 10, 2024 at 2:03 PM Himanshu Chauhan
 wrote:
>
> This patch makes the debug trigger (sdtrig) capability
> as an extension and exports it as an ISA string. The sdtrig
> extension may or may not be implemented in a system. The
> -cpu rv64,sdtrig=
> option can be used to dynamicaly turn sdtrig extension
> on or off.
>
> Signed-off-by: Himanshu Chauhan 
> ---
>  target/riscv/cpu.c | 2 ++
>  target/riscv/cpu_cfg.h | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b07a76ef6b..aaa2d4ff1d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -143,6 +143,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>  ISA_EXT_DATA_ENTRY(zvkt, PRIV_VERSION_1_12_0, ext_zvkt),
>  ISA_EXT_DATA_ENTRY(zhinx, PRIV_VERSION_1_12_0, ext_zhinx),
>  ISA_EXT_DATA_ENTRY(zhinxmin, PRIV_VERSION_1_12_0, ext_zhinxmin),
> +ISA_EXT_DATA_ENTRY(sdtrig, PRIV_VERSION_1_12_0, ext_sdtrig),
>  ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
>  ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
>  ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
> @@ -1306,6 +1307,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>  MULTI_EXT_CFG_BOOL("zve64d", ext_zve64d, false),
>  MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
>
> +MULTI_EXT_CFG_BOOL("sdtrig", ext_sdtrig, true),

This exposes the property, but doesn't wire it up. Can you swap the
order of these patches?

>  MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
>  MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
>  MULTI_EXT_CFG_BOOL("svadu", ext_svadu, true),
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index f4605fb190..3d3acc7f90 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -113,6 +113,7 @@ struct RISCVCPUConfig {
>  bool ext_ssaia;
>  bool ext_sscofpmf;
>  bool ext_smepmp;
> +bool ext_sdtrig;

and include this change in the other patch

Alistair

>  bool rvv_ta_all_1s;
>  bool rvv_ma_all_1s;
>
> --
> 2.34.1
>
>

Re: [PATCH 2/2] target/riscv: Raise an exception when sdtrig is turned off

On Wed, Jan 10, 2024 at 2:03 PM Himanshu Chauhan
 wrote:
>
> When sdtrig is turned off by "sdtrig=false" option, raise
> and illegal instruction exception on any read/write to
> sdtrig CSRs.
>
> Signed-off-by: Himanshu Chauhan 
> ---
>  target/riscv/csr.c | 20 
>  1 file changed, 20 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index c50a33397c..b9ca016ef2 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3854,6 +3854,10 @@ static RISCVException write_pmpaddr(CPURISCVState 
> *env, int csrno,
>  static RISCVException read_tselect(CPURISCVState *env, int csrno,
> target_ulong *val)
>  {
> +if (!riscv_cpu_cfg(env)->ext_sdtrig) {
> +return RISCV_EXCP_ILLEGAL_INST;
> +}

Thanks for the patch!

You should be able to add this check to the

static RISCVException debug(CPURISCVState *env, int csrno)

function instead

Alistair

> +
>  *val = tselect_csr_read(env);
>  return RISCV_EXCP_NONE;
>  }
> @@ -3861,6 +3865,10 @@ static RISCVException read_tselect(CPURISCVState *env, 
> int csrno,
>  static RISCVException write_tselect(CPURISCVState *env, int csrno,
>  target_ulong val)
>  {
> +if (!riscv_cpu_cfg(env)->ext_sdtrig) {
> +return RISCV_EXCP_ILLEGAL_INST;
> +}
> +
>  tselect_csr_write(env, val);
>  return RISCV_EXCP_NONE;
>  }
> @@ -3868,6 +3876,10 @@ static RISCVException write_tselect(CPURISCVState 
> *env, int csrno,
>  static RISCVException read_tdata(CPURISCVState *env, int csrno,
>   target_ulong *val)
>  {
> +if (!riscv_cpu_cfg(env)->ext_sdtrig) {
> +return RISCV_EXCP_ILLEGAL_INST;
> +}
> +
>  /* return 0 in tdata1 to end the trigger enumeration */
>  if (env->trigger_cur >= RV_MAX_TRIGGERS && csrno == CSR_TDATA1) {
>  *val = 0;
> @@ -3885,6 +3897,10 @@ static RISCVException read_tdata(CPURISCVState *env, 
> int csrno,
>  static RISCVException write_tdata(CPURISCVState *env, int csrno,
>target_ulong val)
>  {
> +if (!riscv_cpu_cfg(env)->ext_sdtrig) {
> +return RISCV_EXCP_ILLEGAL_INST;
> +}
> +
>  if (!tdata_available(env, csrno - CSR_TDATA1)) {
>  return RISCV_EXCP_ILLEGAL_INST;
>  }
> @@ -3896,6 +3912,10 @@ static RISCVException write_tdata(CPURISCVState *env, 
> int csrno,
>  static RISCVException read_tinfo(CPURISCVState *env, int csrno,
>   target_ulong *val)
>  {
> +if (!riscv_cpu_cfg(env)->ext_sdtrig) {
> +return RISCV_EXCP_ILLEGAL_INST;
> +}
> +
>  *val = tinfo_csr_read(env);
>  return RISCV_EXCP_NONE;
>  }
> --
> 2.34.1
>
>

Re: [PATCH v4 00/17] target/riscv: deprecate riscv_cpu_options[]

On Sat, Jan 6, 2024 at 9:07 AM Daniel Henrique Barboza
 wrote:
>
> Hi,
>
> This new version contains changes due to a rebase with current
> riscv-to-apply.next, after "[PATCH v13 00/26] riscv: RVA22 profiles
> support" was queued.
>
> Most notable change is a new patch (12) that was added to handle
> 'cbop_blocksize' - zicbop was added by the profile work that just got
> queued and was missing from v3.
>
> A wrong 'cbom_blocksize' reference in patch 10 was also fixed.
>
> Patches based on Alistair's riscv-to-apply.next.
>
> Patches missing acks: 10, 12, 15, 16, 17
>
> Changes from v3:
> - patch 10:
>   - changed wrong cbom_blocksize ref to cboz_blocksize
> - patch 12 (new):
>   - move cbop_blocksize to riscv_cpu_properties[]
> - v3 link: 
> https://lore.kernel.org/qemu-riscv/20240103174013.147279-1-dbarb...@ventanamicro.com/
>
>
> Daniel Henrique Barboza (17):
>   target/riscv/cpu_cfg.h: remove unused fields
>   target/riscv: make riscv_cpu_is_vendor() public
>   target/riscv: move 'pmu-mask' and 'pmu-num' to riscv_cpu_properties[]
>   target/riscv: move 'mmu' to riscv_cpu_properties[]
>   target/riscv: move 'pmp' to riscv_cpu_properties[]
>   target/riscv: rework 'priv_spec'
>   target/riscv: rework 'vext_spec'
>   target/riscv: move 'vlen' to riscv_cpu_properties[]
>   target/riscv: move 'elen' to riscv_cpu_properties[]

I've applied the first few patches to the RISC-V tree, the others
don't apply. Do you mind rebasing them and sending them again?

Alistair

>   target/riscv: create finalize_features() for KVM
>   target/riscv: move 'cbom_blocksize' to riscv_cpu_properties[]
>   target/riscv: move 'cbop_blocksize' to riscv_cpu_properties[]
>   target/riscv: move 'cboz_blocksize' to riscv_cpu_properties[]
>   target/riscv: remove riscv_cpu_options[]
>   target/riscv/cpu.c: move 'mvendorid' to riscv_cpu_properties[]
>   target/riscv/cpu.c: move 'mimpid' to riscv_cpu_properties[]
>   target/riscv/cpu.c: move 'marchid' to riscv_cpu_properties[]
>
>  target/riscv/cpu.c   | 755 ---
>  target/riscv/cpu.h   |   8 +-
>  target/riscv/cpu_cfg.h   |   4 -
>  target/riscv/kvm/kvm-cpu.c   |  94 +++--
>  target/riscv/kvm/kvm_riscv.h |   1 +
>  target/riscv/tcg/tcg-cpu.c   |  63 ---
>  6 files changed, 676 insertions(+), 249 deletions(-)
>
> --
> 2.43.0
>
>

[PATCH] vhost-user.rst: Fix vring address description

2024-01-11 Thread Andrey Ignatov

There is no "size" field in vring address structure. Remove it.

Fixes: 5fc0e00291 ("Add vhost-user protocol documentation")
Signed-off-by: Andrey Ignatov 
---
 docs/interop/vhost-user.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 9f1103f85a..ad6e142f23 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -148,9 +148,9 @@ Vring descriptor indices for packed virtqueues
 A vring address description
 ^^^
 
-+---+---+--++--+---+-+
-| index | flags | size | descriptor | used | available | log |
-+---+---+--++--+---+-+
++---+---++--+---+-+
+| index | flags | descriptor | used | available | log |
++---+---++--+---+-+
 
 :index: a 32-bit vring index
 
-- 
2.41.0

Re: [PATCH] hw/core: Handle cpu_model_from_type() returning NULL value

2024-01-11 Thread Gavin Shan


Hi Phil,

On 1/11/24 18:21, Philippe Mathieu-Daudé wrote:

On 11/1/24 08:30, Gavin Shan wrote:

On 1/11/24 16:47, Philippe Mathieu-Daudé wrote:

Per cpu_model_from_type() docstring (added in commit 445946f4dd):

   * Returns: CPU model name or NULL if the CPU class doesn't exist

We must check the return value in order to avoid surprises, i.e.:

  $ qemu-system-arm -machine virt -cpu cortex-a9
   qemu-system-arm: Invalid CPU model: cortex-a9
   The valid models are: cortex-a7, cortex-a15, (null), (null), (null), (null), 
(null), (null), (null), (null), (null), (null), (null), max

Add assertions when the call can not fail (because the CPU type
must be registered).

Fixes: 5422d2a8fa ("machine: Print CPU model name instead of CPU type")
Reported-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
  cpu-target.c  | 1 +
  hw/core/machine.c | 5 +
  target/ppc/cpu_init.c | 1 +
  3 files changed, 7 insertions(+)

diff --git a/cpu-target.c b/cpu-target.c
index 5eecd7ea2d..b0f6deb13b 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -291,6 +291,7 @@ static void cpu_list_entry(gpointer data, gpointer 
user_data)
  const char *typename = object_class_get_name(OBJECT_CLASS(data));
  g_autofree char *model = cpu_model_from_type(typename);
+    assert(model);
  if (cc->deprecation_note) {
  qemu_printf("  %s (deprecated)\n", model);
  } else {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index fc239101f9..730ec10328 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1422,16 +1422,21 @@ static bool is_cpu_type_supported(const MachineState 
*machine, Error **errp)
  /* The user specified CPU type isn't valid */
  if (!mc->valid_cpu_types[i]) {
  g_autofree char *requested = 
cpu_model_from_type(machine->cpu_type);
+    assert(requested);
  error_setg(errp, "Invalid CPU model: %s", requested);
  if (!mc->valid_cpu_types[1]) {
  g_autofree char *model = cpu_model_from_type(
mc->valid_cpu_types[0]);
+    assert(model);
  error_append_hint(errp, "The only valid type is: %s\n", 
model);
  } else {
  error_append_hint(errp, "The valid models are: ");
  for (i = 0; mc->valid_cpu_types[i]; i++) {
  g_autofree char *model = cpu_model_from_type(
mc->valid_cpu_types[i]);
+    if (!model) {
+    continue;
+    }


Shall we assert(model) for this case, to be consistent with other cases? :)


No, this is the "(null)" cases displayed in the example.

IOW, mc->valid_cpu_types[] contains a CPU type which isn't registered,
so we just skip it.



I thought this should be fixed by correcting mc->valid_cpu_types[] in
hw/arm/virt.c. It means the consistent mc->valid_cpu_types[] needs to
be  provided by the specific board. Otherwise, the logic is incorrect from
the code level at least. For example, "cortex-a9" isn't available to
qemu-system-arm but it has been wrongly declared as supported in hw/arm/virt.c

I've posted one patch against it:

https://lists.nongnu.org/archive/html/qemu-arm/2024-01/msg00531.html





  error_append_hint(errp, "%s%s",
    model,
    mc->valid_cpu_types[i + 1] ? ", " : "");


Otherwise, the separator here need to be adjusted because it's uncertain that
mc->valid_cpu_types[i+1] ... mc->valid_cpu_types[END] are valid.


Here we know mc->valid_cpu_types[i] is *not* NULL, but
mc->valid_cpu_types[i + 1] might be (signaling the end
of the array).

This seems correct to me, but I might be missing something.



When the class for mc->valid_cpu_types[i + 1] isn't registered, we will
skip the entry. it's possible that the class of mc->valid_cpu_types[i + 2]
isn't registered either. mc->valid_cpu_types[i + 3] to mc->valid_cpu_types[END 
- 1]
have the similar situations.

In order to correct the separator, we need to invalidate the return value
from cpu_model_from_type(mc->valid_cpu_types[i + 1]) ... 
cpu_model_from_type(mc->valid_cpu_types[END - 1]).
Too much complex for that and it's another reason why I suggested assert(model) 
as above




diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 344196a8ce..58f0c1e30e 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7037,6 +7037,7 @@ static void ppc_cpu_list_entry(gpointer data, gpointer 
user_data)
  }
  name = cpu_model_from_type(typename);
+    assert(name);
  qemu_printf("PowerPC %-16s PVR %08x\n", name, pcc->pvr);
  for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
  PowerPCCPUAlias *alias = _cpu_aliases[i];




Thanks,
Gavin

Re: [PATCH v4 17/17] target/riscv/cpu.c: move 'marchid' to riscv_cpu_properties[]

On Sat, Jan 6, 2024 at 9:09 AM Daniel Henrique Barboza
 wrote:
>
> Keep all class properties in riscv_cpu_properties[].
>
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 110 +++--
>  1 file changed, 57 insertions(+), 53 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 6149f5960e..3870c3a433 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -2044,6 +2044,62 @@ static const PropertyInfo prop_mimpid = {
>  .set = prop_mimpid_set,
>  };
>
> +static void prop_marchid_set(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> +bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
> +RISCVCPU *cpu = RISCV_CPU(obj);
> +uint64_t prev_val = cpu->cfg.marchid;
> +uint64_t value, invalid_val;
> +uint32_t mxlen = 0;
> +
> +if (!visit_type_uint64(v, name, , errp)) {
> +return;
> +}
> +
> +if (!dynamic_cpu && prev_val != value) {
> +error_setg(errp, "Unable to change %s marchid (0x%" PRIu64 ")",
> +   object_get_typename(obj), prev_val);
> +return;
> +}
> +
> +switch (riscv_cpu_mxl(>env)) {
> +case MXL_RV32:
> +mxlen = 32;
> +break;
> +case MXL_RV64:
> +case MXL_RV128:
> +mxlen = 64;
> +break;
> +default:
> +g_assert_not_reached();
> +}
> +
> +invalid_val = 1LL << (mxlen - 1);
> +
> +if (value == invalid_val) {
> +error_setg(errp, "Unable to set marchid with MSB (%u) bit set "
> + "and the remaining bits zero", mxlen);
> +return;
> +}
> +
> +cpu->cfg.marchid = value;
> +}
> +
> +static void prop_marchid_get(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> +uint64_t value = RISCV_CPU(obj)->cfg.marchid;
> +
> +visit_type_uint64(v, name, , errp);
> +}
> +
> +static const PropertyInfo prop_marchid = {
> +.name = "marchid",
> +.get = prop_marchid_get,
> +.set = prop_marchid_set,
> +};
> +
>  /*
>   * RVA22U64 defines some 'named features' or 'synthetic extensions'
>   * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
> @@ -2132,6 +2188,7 @@ static Property riscv_cpu_properties[] = {
>
>   {.name = "mvendorid", .info = _mvendorid},
>   {.name = "mimpid", .info = _mimpid},
> + {.name = "marchid", .info = _marchid},
>
>  #ifndef CONFIG_USER_ONLY
>  DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
> @@ -2213,56 +2270,6 @@ static const struct SysemuCPUOps riscv_sysemu_ops = {
>  };
>  #endif
>
> -static void cpu_set_marchid(Object *obj, Visitor *v, const char *name,
> -void *opaque, Error **errp)
> -{
> -bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
> -RISCVCPU *cpu = RISCV_CPU(obj);
> -uint64_t prev_val = cpu->cfg.marchid;
> -uint64_t value, invalid_val;
> -uint32_t mxlen = 0;
> -
> -if (!visit_type_uint64(v, name, , errp)) {
> -return;
> -}
> -
> -if (!dynamic_cpu && prev_val != value) {
> -error_setg(errp, "Unable to change %s marchid (0x%" PRIu64 ")",
> -   object_get_typename(obj), prev_val);
> -return;
> -}
> -
> -switch (riscv_cpu_mxl(>env)) {
> -case MXL_RV32:
> -mxlen = 32;
> -break;
> -case MXL_RV64:
> -case MXL_RV128:
> -mxlen = 64;
> -break;
> -default:
> -g_assert_not_reached();
> -}
> -
> -invalid_val = 1LL << (mxlen - 1);
> -
> -if (value == invalid_val) {
> -error_setg(errp, "Unable to set marchid with MSB (%u) bit set "
> - "and the remaining bits zero", mxlen);
> -return;
> -}
> -
> -cpu->cfg.marchid = value;
> -}
> -
> -static void cpu_get_marchid(Object *obj, Visitor *v, const char *name,
> -   void *opaque, Error **errp)
> -{
> -uint64_t value = RISCV_CPU(obj)->cfg.marchid;
> -
> -visit_type_uint64(v, name, , errp);
> -}
> -
>  static void riscv_cpu_class_init(ObjectClass *c, void *data)
>  {
>  RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> @@ -2293,9 +2300,6 @@ static void riscv_cpu_class_init(ObjectClass *c, void 
> *data)
>  cc->gdb_arch_name = riscv_gdb_arch_name;
>  cc->gdb_get_dynamic_xml = riscv_gdb_get_dynamic_xml;
>
> -object_class_property_add(c, "marchid", "uint64", cpu_get_marchid,
> -  cpu_set_marchid, NULL, NULL);
> -
>  device_class_set_props(dc, riscv_cpu_properties);
>  }
>
> --
> 2.43.0
>
>

Re: [PATCH v4 16/17] target/riscv/cpu.c: move 'mimpid' to riscv_cpu_properties[]

On Sat, Jan 6, 2024 at 9:07 AM Daniel Henrique Barboza
 wrote:
>
> Keep all class properties in riscv_cpu_properties[].
>
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 68 --
>  1 file changed, 36 insertions(+), 32 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index c725a4839d..6149f5960e 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -2009,6 +2009,41 @@ static const PropertyInfo prop_mvendorid = {
>  .set = prop_mvendorid_set,
>  };
>
> +static void prop_mimpid_set(Object *obj, Visitor *v, const char *name,
> +void *opaque, Error **errp)
> +{
> +bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
> +RISCVCPU *cpu = RISCV_CPU(obj);
> +uint64_t prev_val = cpu->cfg.mimpid;
> +uint64_t value;
> +
> +if (!visit_type_uint64(v, name, , errp)) {
> +return;
> +}
> +
> +if (!dynamic_cpu && prev_val != value) {
> +error_setg(errp, "Unable to change %s mimpid (0x%" PRIu64 ")",
> +   object_get_typename(obj), prev_val);
> +return;
> +}
> +
> +cpu->cfg.mimpid = value;
> +}
> +
> +static void prop_mimpid_get(Object *obj, Visitor *v, const char *name,
> +void *opaque, Error **errp)
> +{
> +uint64_t value = RISCV_CPU(obj)->cfg.mimpid;
> +
> +visit_type_uint64(v, name, , errp);
> +}
> +
> +static const PropertyInfo prop_mimpid = {
> +.name = "mimpid",
> +.get = prop_mimpid_get,
> +.set = prop_mimpid_set,
> +};
> +
>  /*
>   * RVA22U64 defines some 'named features' or 'synthetic extensions'
>   * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
> @@ -2096,6 +2131,7 @@ static Property riscv_cpu_properties[] = {
>  {.name = "cboz_blocksize", .info = _cboz_blksize},
>
>   {.name = "mvendorid", .info = _mvendorid},
> + {.name = "mimpid", .info = _mimpid},
>
>  #ifndef CONFIG_USER_ONLY
>  DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
> @@ -2177,35 +2213,6 @@ static const struct SysemuCPUOps riscv_sysemu_ops = {
>  };
>  #endif
>
> -static void cpu_set_mimpid(Object *obj, Visitor *v, const char *name,
> -   void *opaque, Error **errp)
> -{
> -bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
> -RISCVCPU *cpu = RISCV_CPU(obj);
> -uint64_t prev_val = cpu->cfg.mimpid;
> -uint64_t value;
> -
> -if (!visit_type_uint64(v, name, , errp)) {
> -return;
> -}
> -
> -if (!dynamic_cpu && prev_val != value) {
> -error_setg(errp, "Unable to change %s mimpid (0x%" PRIu64 ")",
> -   object_get_typename(obj), prev_val);
> -return;
> -}
> -
> -cpu->cfg.mimpid = value;
> -}
> -
> -static void cpu_get_mimpid(Object *obj, Visitor *v, const char *name,
> -   void *opaque, Error **errp)
> -{
> -uint64_t value = RISCV_CPU(obj)->cfg.mimpid;
> -
> -visit_type_uint64(v, name, , errp);
> -}
> -
>  static void cpu_set_marchid(Object *obj, Visitor *v, const char *name,
>  void *opaque, Error **errp)
>  {
> @@ -2286,9 +2293,6 @@ static void riscv_cpu_class_init(ObjectClass *c, void 
> *data)
>  cc->gdb_arch_name = riscv_gdb_arch_name;
>  cc->gdb_get_dynamic_xml = riscv_gdb_get_dynamic_xml;
>
> -object_class_property_add(c, "mimpid", "uint64", cpu_get_mimpid,
> -  cpu_set_mimpid, NULL, NULL);
> -
>  object_class_property_add(c, "marchid", "uint64", cpu_get_marchid,
>cpu_set_marchid, NULL, NULL);
>
> --
> 2.43.0
>
>

Re: [PATCH v4 15/17] target/riscv/cpu.c: move 'mvendorid' to riscv_cpu_properties[]

On Sat, Jan 6, 2024 at 9:09 AM Daniel Henrique Barboza
 wrote:
>
> Keep all class properties in riscv_cpu_properties[].
>
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 69 +-
>  1 file changed, 37 insertions(+), 32 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 9d4243891c..c725a4839d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1974,6 +1974,41 @@ static const PropertyInfo prop_cboz_blksize = {
>  .set = prop_cboz_blksize_set,
>  };
>
> +static void prop_mvendorid_set(Object *obj, Visitor *v, const char *name,
> +   void *opaque, Error **errp)
> +{
> +bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
> +RISCVCPU *cpu = RISCV_CPU(obj);
> +uint32_t prev_val = cpu->cfg.mvendorid;
> +uint32_t value;
> +
> +if (!visit_type_uint32(v, name, , errp)) {
> +return;
> +}
> +
> +if (!dynamic_cpu && prev_val != value) {
> +error_setg(errp, "Unable to change %s mvendorid (0x%x)",
> +   object_get_typename(obj), prev_val);
> +return;
> +}
> +
> +cpu->cfg.mvendorid = value;
> +}
> +
> +static void prop_mvendorid_get(Object *obj, Visitor *v, const char *name,
> +   void *opaque, Error **errp)
> +{
> +uint32_t value = RISCV_CPU(obj)->cfg.mvendorid;
> +
> +visit_type_uint32(v, name, , errp);
> +}
> +
> +static const PropertyInfo prop_mvendorid = {
> +.name = "mvendorid",
> +.get = prop_mvendorid_get,
> +.set = prop_mvendorid_set,
> +};
> +
>  /*
>   * RVA22U64 defines some 'named features' or 'synthetic extensions'
>   * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa
> @@ -2060,6 +2095,8 @@ static Property riscv_cpu_properties[] = {
>  {.name = "cbop_blocksize", .info = _cbop_blksize},
>  {.name = "cboz_blocksize", .info = _cboz_blksize},
>
> + {.name = "mvendorid", .info = _mvendorid},
> +
>  #ifndef CONFIG_USER_ONLY
>  DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> @@ -2140,35 +2177,6 @@ static const struct SysemuCPUOps riscv_sysemu_ops = {
>  };
>  #endif
>
> -static void cpu_set_mvendorid(Object *obj, Visitor *v, const char *name,
> -  void *opaque, Error **errp)
> -{
> -bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
> -RISCVCPU *cpu = RISCV_CPU(obj);
> -uint32_t prev_val = cpu->cfg.mvendorid;
> -uint32_t value;
> -
> -if (!visit_type_uint32(v, name, , errp)) {
> -return;
> -}
> -
> -if (!dynamic_cpu && prev_val != value) {
> -error_setg(errp, "Unable to change %s mvendorid (0x%x)",
> -   object_get_typename(obj), prev_val);
> -return;
> -}
> -
> -cpu->cfg.mvendorid = value;
> -}
> -
> -static void cpu_get_mvendorid(Object *obj, Visitor *v, const char *name,
> -  void *opaque, Error **errp)
> -{
> -uint32_t value = RISCV_CPU(obj)->cfg.mvendorid;
> -
> -visit_type_uint32(v, name, , errp);
> -}
> -
>  static void cpu_set_mimpid(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
>  {
> @@ -2278,9 +2286,6 @@ static void riscv_cpu_class_init(ObjectClass *c, void 
> *data)
>  cc->gdb_arch_name = riscv_gdb_arch_name;
>  cc->gdb_get_dynamic_xml = riscv_gdb_get_dynamic_xml;
>
> -object_class_property_add(c, "mvendorid", "uint32", cpu_get_mvendorid,
> -  cpu_set_mvendorid, NULL, NULL);
> -
>  object_class_property_add(c, "mimpid", "uint64", cpu_get_mimpid,
>cpu_set_mimpid, NULL, NULL);
>
> --
> 2.43.0
>
>

Re: [PATCH v2 0/2] target/riscv: Add support for 'B' extension

On Fri, Jan 12, 2024 at 2:17 AM Rob Bradford  wrote:
>
> Add support for the new (fast track) 'B' extension [1] this extension
> uses the misa.B bit to indicate that the Zba, Zbb and Zbs extensions are
> present.
>
> Since this extension is not yet frozen it is exposed via the 'x-b' cpu
> option. The validation logic is based on the new approach taken for the
> 'G' extension.
>
> The specification handles backward compatability: The misa.B bit may be
> set if Zba, Zbb and Zbs are present but in order to not break existing
> systems the bit is not required to be set if they are present. As such
> even though Zba, Zbb and Zbs default to on in QEMU this extension is not
> enabled by default in any cpu.
>
> Cheers,
>
> Rob
>
> [1] - https://github.com/riscv/riscv-b
>
> Changes since V1:
> - Rebased on master after latest riscv updates
> - All patches have R-B tags
> - Array formatting fix to make future diffs clean (Daniel)
> - Dropped enabling for max CPU variant as misa.B is reserved until
>   spec is at least frozen (Daniel & Drew)
>
> Rob Bradford (2):
>   target/riscv: Add infrastructure for 'B' MISA extension
>   target/riscv: Add step to validate 'B' extension

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  target/riscv/cpu.c |  5 +++--
>  target/riscv/cpu.h |  1 +
>  target/riscv/tcg/tcg-cpu.c | 33 +
>  3 files changed, 37 insertions(+), 2 deletions(-)
>
> --
> 2.43.0
>
>

Re: [RFC PATCH v3 04/30] io: fsync before closing a file channel

2024-01-11 Thread Peter Xu

On Thu, Jan 11, 2024 at 03:46:02PM -0300, Fabiano Rosas wrote:
> > (1) Does this apply to all io channel users, or only migration?
> 
> All file channel users.

I meant the whole idea of flushing on close, on whether there will be
iochannel users that will prefer not do so?  It's a matter of where to put
this best.

-- 
Peter Xu

Re: [PATCH v4 12/17] target/riscv: move 'cbop_blocksize' to riscv_cpu_properties[]

On Sat, Jan 6, 2024 at 9:09 AM Daniel Henrique Barboza
 wrote:
>
> Do the same we did with 'cbom_blocksize' in the previous patch.
>
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 38 +-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b77d26231c..e3cbe9b1b6 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1317,6 +1317,7 @@ static void riscv_cpu_init(Object *obj)
>  cpu->cfg.vlen = 128;
>  cpu->cfg.elen = 64;
>  cpu->cfg.cbom_blocksize = 64;
> +cpu->cfg.cbop_blocksize = 64;
>  cpu->env.vext_ver = VEXT_VERSION_1_00_0;
>  }
>
> @@ -1902,8 +1903,42 @@ static const PropertyInfo prop_cbom_blksize = {
>  .set = prop_cbom_blksize_set,
>  };
>
> +static void prop_cbop_blksize_set(Object *obj, Visitor *v, const char *name,
> +  void *opaque, Error **errp)
> +{
> +RISCVCPU *cpu = RISCV_CPU(obj);
> +uint16_t value;
> +
> +if (!visit_type_uint16(v, name, , errp)) {
> +return;
> +}
> +
> +if (value != cpu->cfg.cbop_blocksize && riscv_cpu_is_vendor(obj)) {
> +cpu_set_prop_err(cpu, name, errp);
> +error_append_hint(errp, "Current '%s' val: %u\n",
> +  name, cpu->cfg.cbop_blocksize);
> +return;
> +}
> +
> +cpu_option_add_user_setting(name, value);
> +cpu->cfg.cbop_blocksize = value;
> +}
> +
> +static void prop_cbop_blksize_get(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> +uint16_t value = RISCV_CPU(obj)->cfg.cbop_blocksize;
> +
> +visit_type_uint16(v, name, , errp);
> +}
> +
> +static const PropertyInfo prop_cbop_blksize = {
> +.name = "cbop_blocksize",
> +.get = prop_cbop_blksize_get,
> +.set = prop_cbop_blksize_set,
> +};
> +
>  Property riscv_cpu_options[] = {
> -DEFINE_PROP_UINT16("cbop_blocksize", RISCVCPU, cfg.cbop_blocksize, 64),
>  DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
>
>  DEFINE_PROP_END_OF_LIST(),
> @@ -1992,6 +2027,7 @@ static Property riscv_cpu_properties[] = {
>  {.name = "elen", .info = _elen},
>
>  {.name = "cbom_blocksize", .info = _cbom_blksize},
> +{.name = "cbop_blocksize", .info = _cbop_blksize},
>
>  #ifndef CONFIG_USER_ONLY
>  DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
> --
> 2.43.0
>
>

Re: [PATCH v4 10/17] target/riscv: create finalize_features() for KVM

On Sat, Jan 6, 2024 at 10:09 AM Daniel Henrique Barboza
 wrote:
>
> To turn cbom_blocksize and cboz_blocksize into class properties we need
> KVM specific changes.
>
> KVM is creating its own version of these options with a customized
> setter() that prevents users from picking an invalid value during init()
> time. This comes at the cost of duplicating each option that KVM
> supports. This will keep happening for each new shared option KVM
> implements in the future.
>
> We can avoid that by using the same property TCG uses and adding
> specific KVM handling during finalize() time, like TCG already does with
> riscv_tcg_cpu_finalize_features(). To do that, the common CPU property
> offers a way of knowing if an option was user set or not, sparing us
> from doing unneeded syscalls.
>
> riscv_kvm_cpu_finalize_features() is then created using the same
> KVMScratch CPU we already use during init() time, since finalize() time
> is still too early to use the official KVM CPU for it. cbom_blocksize
> and cboz_blocksize are then handled during finalize() in the same way
> they're handled by their KVM specific setter.
>
> With this change we can proceed with the blocksize changes in the common
> code without breaking the KVM driver.
>
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c   | 16 +++---
>  target/riscv/cpu.h   |  1 +
>  target/riscv/kvm/kvm-cpu.c   | 59 
>  target/riscv/kvm/kvm_riscv.h |  1 +
>  4 files changed, 72 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 2bb4828324..cd91966ea7 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -68,6 +68,11 @@ static void cpu_option_add_user_setting(const char 
> *optname, uint32_t value)
>  GUINT_TO_POINTER(value));
>  }
>
> +bool riscv_cpu_option_set(const char *optname)
> +{
> +return g_hash_table_contains(general_user_opts, optname);
> +}
> +
>  #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
>  {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}
>
> @@ -1104,17 +1109,18 @@ void riscv_cpu_finalize_features(RISCVCPU *cpu, Error 
> **errp)
>  }
>  #endif
>
> -/*
> - * KVM accel does not have a specialized finalize()
> - * callback because its extensions are validated
> - * in the get()/set() callbacks of each property.
> - */
>  if (tcg_enabled()) {
>  riscv_tcg_cpu_finalize_features(cpu, _err);
>  if (local_err != NULL) {
>  error_propagate(errp, local_err);
>  return;
>  }
> +} else if (kvm_enabled()) {
> +riscv_kvm_cpu_finalize_features(cpu, _err);
> +if (local_err != NULL) {
> +error_propagate(errp, local_err);
> +return;
> +}
>  }
>  }
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 3cec85069f..1c19fa84bb 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -510,6 +510,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int 
> size,
>  bool probe, uintptr_t retaddr);
>  char *riscv_isa_string(RISCVCPU *cpu);
>  void riscv_cpu_list(void);
> +bool riscv_cpu_option_set(const char *optname);
>
>  #define cpu_list riscv_cpu_list
>  #define cpu_mmu_index riscv_cpu_mmu_index
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 2c5217102c..2713f4b2ba 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1493,6 +1493,65 @@ static void kvm_cpu_instance_init(CPUState *cs)
>  }
>  }
>
> +void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
> +{
> +CPURISCVState *env = >env;
> +KVMScratchCPU kvmcpu;
> +struct kvm_one_reg reg;
> +uint64_t val;
> +int ret;
> +
> +/* short-circuit without spinning the scratch CPU */
> +if (!cpu->cfg.ext_zicbom && !cpu->cfg.ext_zicboz) {
> +return;
> +}
> +
> +if (!kvm_riscv_create_scratch_vcpu()) {
> +error_setg(errp, "Unable to create scratch KVM cpu");
> +return;
> +}
> +
> +if (cpu->cfg.ext_zicbom &&
> +riscv_cpu_option_set(kvm_cbom_blocksize.name)) {
> +
> +reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
> +kvm_cbom_blocksize.kvm_reg_id);
> +reg.addr = (uint64_t)
> +ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, );
> +if (ret != 0) {
> +error_setg(errp, "Unable to read cbom_blocksize, error %d", 
> errno);
> +return;
> +}
> +
> +if (cpu->cfg.cbom_blocksize != val) {
> +error_setg(errp, "Unable to set cbom_blocksize to a different "
> +   "value than the host (%lu)", val);
> +return;
> +}
> +}
> +
> +if (cpu->cfg.ext_zicboz &&
> +riscv_cpu_option_set(kvm_cboz_blocksize.name)) {
> +
> +reg.id =

Re: [PATCH v4 2/3] hw/arm: Connect STM32L4x5 SYSCFG to STM32L4x5 SoC

On Wed, Jan 10, 2024 at 5:47 AM Inès Varhol
 wrote:
>
> The SYSCFG input GPIOs aren't connected yet. When the STM32L4x5 GPIO
> device will be implemented, its output GPIOs will be connected to the
> SYSCFG input GPIOs.
>
> Tested-by: Philippe Mathieu-Daudé 
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Arnaud Minier 
> Signed-off-by: Inès Varhol 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/arm/Kconfig |  1 +
>  hw/arm/stm32l4x5_soc.c | 21 -
>  include/hw/arm/stm32l4x5_soc.h |  2 ++
>  3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 8c8488a70a..bb4693bfbb 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -459,6 +459,7 @@ config STM32L4X5_SOC
>  bool
>  select ARM_V7M
>  select OR_IRQ
> +select STM32L4X5_SYSCFG
>  select STM32L4X5_EXTI
>
>  config XLNX_ZYNQMP_ARM
> diff --git a/hw/arm/stm32l4x5_soc.c b/hw/arm/stm32l4x5_soc.c
> index fe46b7c6c0..431f982caf 100644
> --- a/hw/arm/stm32l4x5_soc.c
> +++ b/hw/arm/stm32l4x5_soc.c
> @@ -37,6 +37,7 @@
>  #define SRAM2_SIZE (32 * KiB)
>
>  #define EXTI_ADDR 0x40010400
> +#define SYSCFG_ADDR 0x4001
>
>  #define NUM_EXTI_IRQ 40
>  /* Match exti line connections with their CPU IRQ number */
> @@ -80,6 +81,7 @@ static void stm32l4x5_soc_initfn(Object *obj)
>  Stm32l4x5SocState *s = STM32L4X5_SOC(obj);
>
>  object_initialize_child(obj, "exti", >exti, TYPE_STM32L4X5_EXTI);
> +object_initialize_child(obj, "syscfg", >syscfg, 
> TYPE_STM32L4X5_SYSCFG);
>
>  s->sysclk = qdev_init_clock_in(DEVICE(s), "sysclk", NULL, NULL, 0);
>  s->refclk = qdev_init_clock_in(DEVICE(s), "refclk", NULL, NULL, 0);
> @@ -154,6 +156,19 @@ static void stm32l4x5_soc_realize(DeviceState *dev_soc, 
> Error **errp)
>  return;
>  }
>
> +/* System configuration controller */
> +busdev = SYS_BUS_DEVICE(>syscfg);
> +if (!sysbus_realize(busdev, errp)) {
> +return;
> +}
> +sysbus_mmio_map(busdev, 0, SYSCFG_ADDR);
> +/*
> + * TODO: when the GPIO device is implemented, connect it
> + * to SYCFG using `qdev_connect_gpio_out`, NUM_GPIOS and
> + * GPIO_NUM_PINS.
> + */
> +
> +/* EXTI device */
>  busdev = SYS_BUS_DEVICE(>exti);
>  if (!sysbus_realize(busdev, errp)) {
>  return;
> @@ -163,6 +178,11 @@ static void stm32l4x5_soc_realize(DeviceState *dev_soc, 
> Error **errp)
>  sysbus_connect_irq(busdev, i, qdev_get_gpio_in(armv7m, exti_irq[i]));
>  }
>
> +for (unsigned i = 0; i < 16; i++) {
> +qdev_connect_gpio_out(DEVICE(>syscfg), i,
> +  qdev_get_gpio_in(DEVICE(>exti), i));
> +}
> +
>  /* APB1 BUS */
>  create_unimplemented_device("TIM2",  0x4000, 0x400);
>  create_unimplemented_device("TIM3",  0x4400, 0x400);
> @@ -200,7 +220,6 @@ static void stm32l4x5_soc_realize(DeviceState *dev_soc, 
> Error **errp)
>  /* RESERVED:0x40009800, 0x6800 */
>
>  /* APB2 BUS */
> -create_unimplemented_device("SYSCFG",0x4001, 0x30);
>  create_unimplemented_device("VREFBUF",   0x40010030, 0x1D0);
>  create_unimplemented_device("COMP",  0x40010200, 0x200);
>  /* RESERVED:0x40010800, 0x1400 */
> diff --git a/include/hw/arm/stm32l4x5_soc.h b/include/hw/arm/stm32l4x5_soc.h
> index f7305568dc..baf70410b5 100644
> --- a/include/hw/arm/stm32l4x5_soc.h
> +++ b/include/hw/arm/stm32l4x5_soc.h
> @@ -26,6 +26,7 @@
>
>  #include "exec/memory.h"
>  #include "hw/arm/armv7m.h"
> +#include "hw/misc/stm32l4x5_syscfg.h"
>  #include "hw/misc/stm32l4x5_exti.h"
>  #include "qom/object.h"
>
> @@ -41,6 +42,7 @@ struct Stm32l4x5SocState {
>  ARMv7MState armv7m;
>
>  Stm32l4x5ExtiState exti;
> +Stm32l4x5SyscfgState syscfg;
>
>  MemoryRegion sram1;
>  MemoryRegion sram2;
> --
> 2.43.0
>
>

Re: [PATCH v2 23/43] target/riscv: Remove misa_mxl validation

On Tue, Jan 9, 2024 at 1:43 AM Alex Bennée  wrote:
>
> Alex Bennée  writes:
>
> > From: Akihiko Odaki 
> >
> > It is initialized with a simple assignment and there is little room for
> > error. In fact, the validation is even more complex.
> >
> > Signed-off-by: Akihiko Odaki 
> > Acked-by: LIU Zhiwei 
> > Reviewed-by: Daniel Henrique Barboza 
> > Acked-by: Alistair Francis 
> > Message-Id: <20231213-riscv-v7-2-a760156a3...@daynix.com>
> > Signed-off-by: Alex Bennée 
>
> ping: along with this are the RiscV maintainers happy for me to take:
>
>   [PATCH v2 23/43] target/riscv: Remove misa_mxl validation
>   [PATCH v2 24/43] target/riscv: Move misa_mxl_max to class
>   [PATCH v2 25/43] target/riscv: Validate misa_mxl_max only once
>
> through my next pull request?

Yep! Go for it

Alistair

>
> 
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>

Re: [PATCH v2 25/43] target/riscv: Validate misa_mxl_max only once

On Thu, Jan 4, 2024 at 5:04 AM Alex Bennée  wrote:
>
> From: Akihiko Odaki 
>
> misa_mxl_max is now a class member and initialized only once for each
> class. This also moves the initialization of gdb_core_xml_file which
> will be referenced before realization in the future.
>
> Signed-off-by: Akihiko Odaki 
> Message-Id: <20231213-riscv-v7-4-a760156a3...@daynix.com>
> Signed-off-by: Alex Bennée 

Acked-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 21 +
>  target/riscv/tcg/tcg-cpu.c | 23 ---
>  2 files changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 2ab61df2217..b799f133604 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1247,6 +1247,26 @@ static const MISAExtInfo misa_ext_info_arr[] = {
>  MISA_EXT_INFO(RVG, "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
>  };
>
> +static void riscv_cpu_validate_misa_mxl(RISCVCPUClass *mcc)
> +{
> +CPUClass *cc = CPU_CLASS(mcc);
> +
> +/* Validate that MISA_MXL is set properly. */
> +switch (mcc->misa_mxl_max) {
> +#ifdef TARGET_RISCV64
> +case MXL_RV64:
> +case MXL_RV128:
> +cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
> +break;
> +#endif
> +case MXL_RV32:
> +cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
> +break;
> +default:
> +g_assert_not_reached();
> +}
> +}
> +
>  static int riscv_validate_misa_info_idx(uint32_t bit)
>  {
>  int idx;
> @@ -1695,6 +1715,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void 
> *data)
>  RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
>
>  mcc->misa_mxl_max = (uint32_t)(uintptr_t)data;
> +riscv_cpu_validate_misa_mxl(mcc);
>  }
>
>  static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 7f6712c81a4..eb243e011ca 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -148,27 +148,6 @@ static void riscv_cpu_validate_misa_priv(CPURISCVState 
> *env, Error **errp)
>  }
>  }
>
> -static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu)
> -{
> -RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> -CPUClass *cc = CPU_CLASS(mcc);
> -
> -/* Validate that MISA_MXL is set properly. */
> -switch (mcc->misa_mxl_max) {
> -#ifdef TARGET_RISCV64
> -case MXL_RV64:
> -case MXL_RV128:
> -cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
> -break;
> -#endif
> -case MXL_RV32:
> -cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
> -break;
> -default:
> -g_assert_not_reached();
> -}
> -}
> -
>  static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)
>  {
>  CPURISCVState *env = >env;
> @@ -676,8 +655,6 @@ static bool tcg_cpu_realize(CPUState *cs, Error **errp)
>  return false;
>  }
>
> -riscv_cpu_validate_misa_mxl(cpu);
> -
>  #ifndef CONFIG_USER_ONLY
>  CPURISCVState *env = >env;
>  Error *local_err = NULL;
> --
> 2.39.2
>
>

Re: [PATCH v2 24/43] target/riscv: Move misa_mxl_max to class

On Thu, Jan 4, 2024 at 3:44 AM Alex Bennée  wrote:
>
> From: Akihiko Odaki 
>
> misa_mxl_max is common for all instances of a RISC-V CPU class so they
> are better put into class.
>
> Signed-off-by: Akihiko Odaki 
> Message-Id: <20231213-riscv-v7-3-a760156a3...@daynix.com>
> Signed-off-by: Alex Bennée 

Acked-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.h |   4 +-
>  target/riscv/cpu.c | 118 +++--
>  target/riscv/gdbstub.c |  12 ++--
>  target/riscv/kvm/kvm-cpu.c |  10 ++--
>  target/riscv/machine.c |   7 +--
>  target/riscv/tcg/tcg-cpu.c |  12 ++--
>  target/riscv/translate.c   |   3 +-
>  7 files changed, 87 insertions(+), 79 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index d74b361be64..060b7f69a74 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -169,7 +169,6 @@ struct CPUArchState {
>
>  /* RISCVMXL, but uint32_t for vmstate migration */
>  uint32_t misa_mxl;  /* current mxl */
> -uint32_t misa_mxl_max;  /* max mxl for this cpu */
>  uint32_t misa_ext;  /* current extensions */
>  uint32_t misa_ext_mask; /* max ext for this cpu */
>  uint32_t xl;/* current xlen */
> @@ -450,6 +449,7 @@ struct RISCVCPUClass {
>
>  DeviceRealize parent_realize;
>  ResettablePhases parent_phases;
> +uint32_t misa_mxl_max;  /* max mxl for this cpu */
>  };
>
>  static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
> @@ -756,7 +756,7 @@ enum riscv_pmu_event_idx {
>  /* used by tcg/tcg-cpu.c*/
>  void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, bool en);
>  bool isa_ext_is_enabled(RISCVCPU *cpu, uint32_t ext_offset);
> -void riscv_cpu_set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext);
> +void riscv_cpu_set_misa_ext(CPURISCVState *env, uint32_t ext);
>
>  typedef struct RISCVCPUMultiExtConfig {
>  const char *name;
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 83c7c0cf07b..2ab61df2217 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -274,9 +274,8 @@ const char *riscv_cpu_get_trap_name(target_ulong cause, 
> bool async)
>  }
>  }
>
> -void riscv_cpu_set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
> +void riscv_cpu_set_misa_ext(CPURISCVState *env, uint32_t ext)
>  {
> -env->misa_mxl_max = env->misa_mxl = mxl;
>  env->misa_ext_mask = env->misa_ext = ext;
>  }
>
> @@ -378,11 +377,7 @@ static void riscv_any_cpu_init(Object *obj)
>  {
>  RISCVCPU *cpu = RISCV_CPU(obj);
>  CPURISCVState *env = >env;
> -#if defined(TARGET_RISCV32)
> -riscv_cpu_set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | 
> RVU);
> -#elif defined(TARGET_RISCV64)
> -riscv_cpu_set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | 
> RVU);
> -#endif
> +riscv_cpu_set_misa_ext(env, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
>
>  #ifndef CONFIG_USER_ONLY
>  set_satp_mode_max_supported(RISCV_CPU(obj),
> @@ -403,16 +398,14 @@ static void riscv_max_cpu_init(Object *obj)
>  {
>  RISCVCPU *cpu = RISCV_CPU(obj);
>  CPURISCVState *env = >env;
> -RISCVMXL mlx = MXL_RV64;
>
> -#ifdef TARGET_RISCV32
> -mlx = MXL_RV32;
> -#endif
> -riscv_cpu_set_misa(env, mlx, 0);
>  env->priv_ver = PRIV_VERSION_LATEST;
>  #ifndef CONFIG_USER_ONLY
> -set_satp_mode_max_supported(RISCV_CPU(obj), mlx == MXL_RV32 ?
> -VM_1_10_SV32 : VM_1_10_SV57);
> +#ifdef TARGET_RISCV32
> +set_satp_mode_max_supported(cpu, VM_1_10_SV32);
> +#else
> +set_satp_mode_max_supported(cpu, VM_1_10_SV57);
> +#endif
>  #endif
>  }
>
> @@ -420,8 +413,6 @@ static void riscv_max_cpu_init(Object *obj)
>  static void rv64_base_cpu_init(Object *obj)
>  {
>  CPURISCVState *env = _CPU(obj)->env;
> -/* We set this in the realise function */
> -riscv_cpu_set_misa(env, MXL_RV64, 0);
>  /* Set latest version of privileged specification */
>  env->priv_ver = PRIV_VERSION_LATEST;
>  #ifndef CONFIG_USER_ONLY
> @@ -433,8 +424,7 @@ static void rv64_sifive_u_cpu_init(Object *obj)
>  {
>  RISCVCPU *cpu = RISCV_CPU(obj);
>  CPURISCVState *env = >env;
> -riscv_cpu_set_misa(env, MXL_RV64,
> -   RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> +riscv_cpu_set_misa_ext(env, RVI | RVM | RVA | RVF | RVD | RVC | RVS | 
> RVU);
>  env->priv_ver = PRIV_VERSION_1_10_0;
>  #ifndef CONFIG_USER_ONLY
>  set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV39);
> @@ -452,7 +442,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
>  CPURISCVState *env = _CPU(obj)->env;
>  RISCVCPU *cpu = RISCV_CPU(obj);
>
> -riscv_cpu_set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
> +riscv_cpu_set_misa_ext(env, RVI | RVM | RVA | RVC | RVU);
>  env->priv_ver = PRIV_VERSION_1_10_0;
>  #ifndef CONFIG_USER_ONLY
>  set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
> @@ -469,7 +459,7 @@ static void

Re: [PATCH v2 qemu] hw/cxl/device: read from register values in mdev_reg_read()

2024-01-11 Thread fan

On Thu, Jan 11, 2024 at 02:59:05PM +, Jonathan Cameron wrote:
> From: Hyeonggon Yoo <42.hye...@gmail.com>
> 
> In the current mdev_reg_read() implementation, it consistently returns
> that the Media Status is Ready (01b). This was fine until commit
> 25a52959f99d ("hw/cxl: Add support for device sanitation") because the
> media was presumed to be ready.
> 
> However, as per the CXL 3.0 spec "8.2.9.8.5.1 Sanitize (Opcode 4400h)",
> during sanitation, the Media State should be set to Disabled (11b). The
> mentioned commit correctly sets it to Disabled, but mdev_reg_read()
> still returns Media Status as Ready.
> 
> To address this, update mdev_reg_read() to read register values instead
> of returning dummy values.
> 
> Note __toggle_media() was overwriting the mailbox capability
> register, but nothing was reading that after this so that bug had no
> obvious effect unless the driver was reloaded.
> 
> Fixes: commit 25a52959f99d ("hw/cxl: Add support for device sanitation")
> Signed-off-by: Hyeonggon Yoo <42.hye...@gmail.com>
> Link: https://lore.kernel.org/r/20231222090051.3265307-3-42.hye...@gmail.com
> Signed-off-by: Jonathan Cameron 
> 

Reviewed-by: Fan Ni 

> --
> 
> Hyeonggon - I've kept your sign-off. Let me know if this is ok.
> Dropped RBs etc as this has changed quite a bit.
> 
> I plan to send out a group of fixes including this soon, but given
> I've been pointing out the original fix didn't work thought I'd send
> this one out for early review!
> 
> ---
>  include/hw/cxl/cxl_device.h |  9 +++--
>  hw/cxl/cxl-device-utils.c   | 17 +++--
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index befb5f884b..31d2afcd3d 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -202,6 +202,9 @@ typedef struct cxl_device_state {
>  };
>  };
>  
> +/* Stash the memory device status value */
> +uint64_t memdev_status;
> +
>  struct {
>  bool set;
>  uint64_t last_set;
> @@ -353,8 +356,10 @@ static inline void __toggle_media(CXLDeviceState 
> *cxl_dstate, int val)
>  {
>  uint64_t dev_status_reg;
>  
> -dev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val);
> -cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = dev_status_reg;
> +dev_status_reg = cxl_dstate->memdev_status;
> +dev_status_reg = FIELD_DP64(dev_status_reg, CXL_MEM_DEV_STS, 
> MEDIA_STATUS,
> +val);
> +cxl_dstate->memdev_status = dev_status_reg;
>  }
>  #define cxl_dev_disable_media(cxlds)\
>  do { __toggle_media((cxlds), 0x3); } while (0)
> diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> index 61a3c4dc2e..40b619ffd9 100644
> --- a/hw/cxl/cxl-device-utils.c
> +++ b/hw/cxl/cxl-device-utils.c
> @@ -229,12 +229,9 @@ static void mailbox_reg_write(void *opaque, hwaddr 
> offset, uint64_t value,
>  
>  static uint64_t mdev_reg_read(void *opaque, hwaddr offset, unsigned size)
>  {
> -uint64_t retval = 0;
> -
> -retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MEDIA_STATUS, 1);
> -retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MBOX_READY, 1);
> +CXLDeviceState *cxl_dstate = opaque;
>  
> -return retval;
> +return cxl_dstate->memdev_status;
>  }
>  
>  static void ro_reg_write(void *opaque, hwaddr offset, uint64_t value,
> @@ -371,7 +368,15 @@ static void mailbox_reg_init_common(CXLDeviceState 
> *cxl_dstate)
>  cxl_dstate->mbox_msi_n = msi_n;
>  }
>  
> -static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) { }
> +static void memdev_reg_init_common(CXLDeviceState *cxl_dstate)
> +{
> +uint64_t memdev_status_reg;
> +
> +memdev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, 1);
> +memdev_status_reg = FIELD_DP64(memdev_status_reg, CXL_MEM_DEV_STS,
> +   MBOX_READY, 1);
> +cxl_dstate->memdev_status = memdev_status_reg;
> +}
>  
>  void cxl_device_register_init_t3(CXLType3Dev *ct3d)
>  {
> -- 
> 2.39.2
>

Re: [PATCH 5/5] target/riscv: Rename tcg_cpu_FOO() to include 'riscv'

On Thu, Jan 11, 2024 at 11:05 PM Philippe Mathieu-Daudé
 wrote:
>
> The tcg_cpu_FOO() names are riscv specific, so rename
> them as riscv_tcg_cpu_FOO() (as other names in this file)
> to ease navigating the code.
>
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/tcg/tcg-cpu.c | 28 ++--
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 14133ff665..994ca1cdf9 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -929,7 +929,7 @@ static bool riscv_cpu_is_vendor(Object *cpu_obj)
>   *   -> cpu_exec_realizefn()
>   *  -> tcg_cpu_realize() (via accel_cpu_common_realize())
>   */
> -static bool tcg_cpu_realize(CPUState *cs, Error **errp)
> +static bool riscv_tcg_cpu_realize(CPUState *cs, Error **errp)
>  {
>  RISCVCPU *cpu = RISCV_CPU(cs);
>  Error *local_err = NULL;
> @@ -1372,7 +1372,7 @@ static bool riscv_cpu_has_max_extensions(Object 
> *cpu_obj)
>  return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL;
>  }
>
> -static void tcg_cpu_instance_init(CPUState *cs)
> +static void riscv_tcg_cpu_instance_init(CPUState *cs)
>  {
>  RISCVCPU *cpu = RISCV_CPU(cs);
>  Object *obj = OBJECT(cpu);
> @@ -1386,7 +1386,7 @@ static void tcg_cpu_instance_init(CPUState *cs)
>  }
>  }
>
> -static void tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
> +static void riscv_tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
>  {
>  /*
>   * All cpus use the same set of operations.
> @@ -1394,30 +1394,30 @@ static void tcg_cpu_init_ops(AccelCPUClass 
> *accel_cpu, CPUClass *cc)
>  cc->tcg_ops = _tcg_ops;
>  }
>
> -static void tcg_cpu_class_init(CPUClass *cc)
> +static void riscv_tcg_cpu_class_init(CPUClass *cc)
>  {
> -cc->init_accel_cpu = tcg_cpu_init_ops;
> +cc->init_accel_cpu = riscv_tcg_cpu_init_ops;
>  }
>
> -static void tcg_cpu_accel_class_init(ObjectClass *oc, void *data)
> +static void riscv_tcg_cpu_accel_class_init(ObjectClass *oc, void *data)
>  {
>  AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
>
> -acc->cpu_class_init = tcg_cpu_class_init;
> -acc->cpu_instance_init = tcg_cpu_instance_init;
> -acc->cpu_target_realize = tcg_cpu_realize;
> +acc->cpu_class_init = riscv_tcg_cpu_class_init;
> +acc->cpu_instance_init = riscv_tcg_cpu_instance_init;
> +acc->cpu_target_realize = riscv_tcg_cpu_realize;
>  }
>
> -static const TypeInfo tcg_cpu_accel_type_info = {
> +static const TypeInfo riscv_tcg_cpu_accel_type_info = {
>  .name = ACCEL_CPU_NAME("tcg"),
>
>  .parent = TYPE_ACCEL_CPU,
> -.class_init = tcg_cpu_accel_class_init,
> +.class_init = riscv_tcg_cpu_accel_class_init,
>  .abstract = true,
>  };
>
> -static void tcg_cpu_accel_register_types(void)
> +static void riscv_tcg_cpu_accel_register_types(void)
>  {
> -type_register_static(_cpu_accel_type_info);
> +type_register_static(_tcg_cpu_accel_type_info);
>  }
> -type_init(tcg_cpu_accel_register_types);
> +type_init(riscv_tcg_cpu_accel_register_types);
> --
> 2.41.0
>
>

Re: [PATCH 1/5] accel: Rename accel_init_ops_interfaces() to include 'system'

On Thu, Jan 11, 2024 at 10:05 PM Philippe Mathieu-Daudé
 wrote:
>
> accel_init_ops_interfaces() is system specific, so
> rename it as accel_system_init_ops_interfaces() to
> ease navigating the code.
>
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  accel/accel-system.h | 2 +-
>  accel/accel-system.c | 2 +-
>  accel/accel-target.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/accel/accel-system.h b/accel/accel-system.h
> index d41c62f21b..2d37c73c97 100644
> --- a/accel/accel-system.h
> +++ b/accel/accel-system.h
> @@ -10,6 +10,6 @@
>  #ifndef ACCEL_SYSTEM_H
>  #define ACCEL_SYSTEM_H
>
> -void accel_init_ops_interfaces(AccelClass *ac);
> +void accel_system_init_ops_interfaces(AccelClass *ac);
>
>  #endif /* ACCEL_SYSTEM_H */
> diff --git a/accel/accel-system.c b/accel/accel-system.c
> index fa8f43757c..f6c947dd82 100644
> --- a/accel/accel-system.c
> +++ b/accel/accel-system.c
> @@ -62,7 +62,7 @@ void accel_setup_post(MachineState *ms)
>  }
>
>  /* initialize the arch-independent accel operation interfaces */
> -void accel_init_ops_interfaces(AccelClass *ac)
> +void accel_system_init_ops_interfaces(AccelClass *ac)
>  {
>  const char *ac_name;
>  char *ops_name;
> diff --git a/accel/accel-target.c b/accel/accel-target.c
> index 7e3cbde5df..08626c00c2 100644
> --- a/accel/accel-target.c
> +++ b/accel/accel-target.c
> @@ -104,7 +104,7 @@ static void accel_init_cpu_interfaces(AccelClass *ac)
>  void accel_init_interfaces(AccelClass *ac)
>  {
>  #ifndef CONFIG_USER_ONLY
> -accel_init_ops_interfaces(ac);
> +accel_system_init_ops_interfaces(ac);
>  #endif /* !CONFIG_USER_ONLY */
>
>  accel_init_cpu_interfaces(ac);
> --
> 2.41.0
>
>

Re: [PATCH v2 2/2] target/riscv: Add step to validate 'B' extension

On Fri, Jan 12, 2024 at 2:17 AM Rob Bradford  wrote:
>
> If the B extension is enabled warn if the user has disabled any of the
> required extensions that are part of the 'B' extension. Conversely
> enable the extensions that make up the 'B' extension if it is enabled.
>
> Signed-off-by: Rob Bradford 
> Reviewed-by: Daniel Henrique Barboza 
> Reviewed-by: Andrew Jones 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/tcg/tcg-cpu.c | 33 +
>  1 file changed, 33 insertions(+)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 5396c6c3eb..b5ba78240e 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -442,6 +442,35 @@ static void riscv_cpu_validate_g(RISCVCPU *cpu)
>  }
>  }
>
> +static void riscv_cpu_validate_b(RISCVCPU *cpu)
> +{
> +const char *warn_msg = "RVB mandates disabled extension %s";
> +
> +if (!cpu->cfg.ext_zba) {
> +if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zba))) {
> +cpu->cfg.ext_zba = true;
> +} else {
> +warn_report(warn_msg, "zba");
> +}
> +}
> +
> +if (!cpu->cfg.ext_zbb) {
> +if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zbb))) {
> +cpu->cfg.ext_zbb = true;
> +} else {
> +warn_report(warn_msg, "zbb");
> +}
> +}
> +
> +if (!cpu->cfg.ext_zbs) {
> +if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zbs))) {
> +cpu->cfg.ext_zbs = true;
> +} else {
> +warn_report(warn_msg, "zbs");
> +}
> +}
> +}
> +
>  /*
>   * Check consistency between chosen extensions while setting
>   * cpu->cfg accordingly.
> @@ -455,6 +484,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>  riscv_cpu_validate_g(cpu);
>  }
>
> +if (riscv_has_ext(env, RVB)) {
> +riscv_cpu_validate_b(cpu);
> +}
> +
>  if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
>  error_setg(errp,
> "I and E extensions are incompatible");
> --
> 2.43.0
>
>

Re: [PATCH v2 1/2] target/riscv: Add infrastructure for 'B' MISA extension

On Fri, Jan 12, 2024 at 3:38 AM Rob Bradford  wrote:
>
> Add the infrastructure for the 'B' extension which is the union of the
> Zba, Zbb and Zbs instructions.
>
> Signed-off-by: Rob Bradford 
> Reviewed-by: Daniel Henrique Barboza 
> Reviewed-by: Andrew Jones 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 5 +++--
>  target/riscv/cpu.h | 1 +
>  target/riscv/tcg/tcg-cpu.c | 1 +
>  3 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 8cbfc7e781..fc01c10e24 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -38,9 +38,9 @@
>  #include "tcg/tcg.h"
>
>  /* RISC-V CPU definitions */
> -static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
> +static const char riscv_single_letter_exts[] = "IEMAFDQCBPVH";
>  const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, RVV,
> -  RVC, RVS, RVU, RVH, RVJ, RVG, 0};
> +  RVC, RVS, RVU, RVH, RVJ, RVG, RVB, 0};
>
>  /*
>   * From vector_helper.c
> @@ -1299,6 +1299,7 @@ static const MISAExtInfo misa_ext_info_arr[] = {
>  MISA_EXT_INFO(RVJ, "x-j", "Dynamic translated languages"),
>  MISA_EXT_INFO(RVV, "v", "Vector operations"),
>  MISA_EXT_INFO(RVG, "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
> +MISA_EXT_INFO(RVB, "x-b", "Bit manipulation (Zba_Zbb_Zbs)")
>  };
>
>  static int riscv_validate_misa_info_idx(uint32_t bit)
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 5f3955c38d..3843d44fc9 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -69,6 +69,7 @@ typedef struct CPUArchState CPURISCVState;
>  #define RVH RV('H')
>  #define RVJ RV('J')
>  #define RVG RV('G')
> +#define RVB RV('B')
>
>  extern const uint32_t misa_bits[];
>  const char *riscv_get_misa_ext_name(uint32_t bit);
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 14133ff665..5396c6c3eb 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -1056,6 +1056,7 @@ static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>  MISA_CFG(RVJ, false),
>  MISA_CFG(RVV, false),
>  MISA_CFG(RVG, false),
> +MISA_CFG(RVB, false),
>  };
>
>  /*
> --
> 2.43.0
>
>

Re: [PATCH v2 2/9] hw/hppa/machine: Disable default devices with --nodefaults option

2024-01-11 Thread Helge Deller


On 1/9/24 17:01, Richard Henderson wrote:

On 1/9/24 22:16, Helge Deller wrote:

On 1/9/24 10:57, Richard Henderson wrote:

On 1/8/24 00:22, del...@kernel.org wrote:

From: Helge Deller 

Add support for the qemu --nodefaults option, which will disable the
following default devices:
- lsi53c895a SCSI controller,
- artist graphics card,
- LASI 82596 NIC,
- tulip PCI NIC,
- second serial PCI card,
- USB OHCI controller.

Adding this option is very useful to allow manual testing and
debugging of the other possible devices on the command line.

Signed-off-by: Helge Deller 
---
  hw/hppa/machine.c | 15 +--
  1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index b11907617e..8017002a2a 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -346,11 +346,14 @@ static void machine_HP_common_init_tail(MachineState 
*machine, PCIBus *pci_bus,
  SysBusDevice *s;
  /* SCSI disk setup. */
-    dev = DEVICE(pci_create_simple(pci_bus, -1, "lsi53c895a"));
-    lsi53c8xx_handle_legacy_cmdline(dev);
+    if (defaults_enabled()) {
+    dev = DEVICE(pci_create_simple(pci_bus, -1, "lsi53c895a"));
+    lsi53c8xx_handle_legacy_cmdline(dev);
+    }
  /* Graphics setup. */
-    if (machine->enable_graphics && vga_interface_type != VGA_NONE) {
+    if (defaults_enabled() && machine->enable_graphics &&
+    vga_interface_type != VGA_NONE) {
  vga_interface_created = true;
  dev = qdev_new("artist");
  s = SYS_BUS_DEVICE(dev);
@@ -360,7 +363,7 @@ static void machine_HP_common_init_tail(MachineState 
*machine, PCIBus *pci_bus,
  }
  /* Network setup. */
-    if (enable_lasi_lan()) {
+    if (defaults_enabled() && enable_lasi_lan()) {
  lasi_82596_init(addr_space, translate(NULL, LASI_LAN_HPA),
  qdev_get_gpio_in(lasi_dev, LASI_IRQ_LAN_HPA));
  }
@@ -385,7 +388,7 @@ static void machine_HP_common_init_tail(MachineState 
*machine, PCIBus *pci_bus,
  pci_set_word(_dev->config[PCI_SUBSYSTEM_ID], 0x1227); /* Powerbar */
  /* create a second serial PCI card when running Astro */
-    if (!lasi_dev) {
+    if (defaults_enabled() && !lasi_dev) {
  pci_dev = pci_new(-1, "pci-serial-4x");
  qdev_prop_set_chr(DEVICE(pci_dev), "chardev1", serial_hd(1));
  qdev_prop_set_chr(DEVICE(pci_dev), "chardev2", serial_hd(2));
@@ -395,7 +398,7 @@ static void machine_HP_common_init_tail(MachineState 
*machine, PCIBus *pci_bus,
  }
  /* create USB OHCI controller for USB keyboard & mouse on Astro machines 
*/
-    if (!lasi_dev && machine->enable_graphics) {
+    if (defaults_enabled() && !lasi_dev && machine->enable_graphics) {
  pci_create_simple(pci_bus, -1, "pci-ohci");
  usb_create_simple(usb_bus_find(-1), "usb-kbd");
  usb_create_simple(usb_bus_find(-1), "usb-mouse");


This almost doubles the uses of default_enabled in the entire tree.
I wonder if some of them are redundant or should be using a different
test.


Any proposal?
Maybe introduce a local variable hppa_bare_metal = !defaults_enabled();
and use that instead?


No, not like that.


Ok.


In casual review I am surprised that !defaults_enabled() does not
already imply !enable_graphics, unless the command-line goes on to
explicitly add a graphics device.
Am I missing something?


Will check that tommorow. If it does I'll remove that additional check.

But what other do you suggest in general how I should address your
concerns here?

Helge

Re: [PATCH 11/19] qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type

2024-01-11 Thread John Snow

On Thu, Jan 11, 2024 at 4:33 AM Markus Armbruster  wrote:
>
> John Snow  writes:
>
> > On Thu, Nov 23, 2023, 8:03 AM Markus Armbruster  wrote:
> >
> >> John Snow  writes:
> >>
> >> > On Wed, Nov 22, 2023 at 7:59 AM Markus Armbruster  
> >> > wrote:
> >> >>
> >> >> John Snow  writes:
> >> >>
> >> >> > There's more conditionals in here than we can reasonably pack into a
> >> >> > terse little statement, so break it apart into something more> 
> >> >> > explicit.
> >> >> >
> >> >> > (When would a built-in array ever cause a QAPISemError? I don't know,
> >> >> > maybe never - but the type system wasn't happy all the same.)
> >> >> >
> >> >> > Signed-off-by: John Snow 
> >> >> > ---
> >> >> >  scripts/qapi/schema.py | 11 +--
> >> >> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >> >> >
> >> >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> >> >> > index 462acb2bb61..164d86c4064 100644
> >> >> > --- a/scripts/qapi/schema.py
> >> >> > +++ b/scripts/qapi/schema.py
> >> >> > @@ -384,9 +384,16 @@ def need_has_if_optional(self):
> >> >> >
> >> >> >  def check(self, schema):
> >> >> >  super().check(schema)
> >> >> > +
> >> >> > +if self.info:
> >> >> > +assert self.info.defn_meta  # guaranteed to be set by> 
> >> >> > expr.py
> >> >> > +what = self.info.defn_meta
> >> >> > +else:
> >> >> > +what = 'built-in array'
> >> >> > +
> >> >> >  self._element_type = schema.resolve_type(
> >> >> > -self._element_type_name, self.info,
> >> >> > -self.info and self.info.defn_meta)
> >> >> > +self._element_type_name, self.info, what
> >> >> > +)
> >> 0>> >  assert not isinstance(self.element_type, 
> >> QAPISchemaArrayType)
> >> >> >
> >> >> >  def set_module(self, schema):
> >> >>
> >> >> What problem are you solving here?
> >> >>
> >> >
> >> > 1. "self.info and self.info.defn_meta" is the wrong type ifn't self.info
> >>
> >> self.info is Optional[QAPISourceInfo].
> >>
> >> When self.info, then self.info.defn_meta is is Optional[str].
> >>
> >> Naive me expects self.info and self.info.defn_meta to be Optional[str].
> >> Playing with mypy...  it seems to be Union[QAPISourceInfo, None, str].
> >> Type inference too weak.
> >>
> >
> > I think my expectations match yours: "x and y" should return either x or y,
> > so the resulting type would naively be Union[X | Y], which would indeed be
> > Union[QAPISourceInfo | None | str], but:
> >
> > If QAPISourceInfo is *false-y*, but not None, it'd be possible for the
> > expression to yield a QAPISourceInfo. mypy does not understand that
> > QAPISourceInfo can never be false-y.
> >
> > (That I know of. Maybe there's a trick to annotate it. I like your solution
> > below better anyway, just curious about the exact nature of this
> > limitation.)
> >
> >
> >> > 2. self.info.defn_meta is *also* not guaranteed by static types
> >>
> >> Yes.  We know it's not None ("guaranteed to be set by expr.py"), but the
> >> type system doesn't.
> >>
> >
> > Mmhmm.
> >
> >
> >> > ultimately: we need to assert self.info and self.info.defn_meta both;
> >> > but it's possible (?) that we don't have self.info in the case that
> >> > we're a built-in array, so I handle that.
> >>
> >> This bring us back to the question in your commit message: "When would a
> >> built-in array ever cause a QAPISemError?"  Short answer: never.
> >
> > Right, okay. I just couldn't guarantee it statically. I knew this patch was
> > a little bananas, sorry for tossing you the stinkbomb.
>
> No need to be sorry!  Feels like an efficient way to collaborate with
> me.
>
> >> Long answer.  We're dealing with a *specific* QAPISemError here, namely
> >> .resolve_type()'s "uses unknown type".  If this happens for a built-in
> >> array, it's a programming error.
> >>
> >> Let's commit such an error to see what happens: stick
> >>
> >> self._make_array_type('xxx', None)
> >>
> >> Dies like this:
> >>
> >> Traceback (most recent call last):
> >>   File "/work/armbru/qemu/scripts/qapi/main.py", line 94, in main
> >> generate(args.schema,
> >>   File "/work/armbru/qemu/scripts/qapi/main.py", line 50, in generate
> >> schema = QAPISchema(schema_file)
> >>  ^^^
> >>   File "/work/armbru/qemu/scripts/qapi/schema.py", line 938, in
> >> __init__
> >> self.check()
> >>   File "/work/armbru/qemu/scripts/qapi/schema.py", line 1225, in check
> >> ent.check(self)
> >>   File "/work/armbru/qemu/scripts/qapi/schema.py", line 373, in check
> >> self.element_type = schema.resolve_type(
> >> 
> >>   File "/work/armbru/qemu/scripts/qapi/schema.py", line 973, in
> >> resolve_type
> >> raise QAPISemError(
> >> qapi.error.QAPISemError: 
> >>
> >> During handling of the above exception, another exception occurred:
> >>
> >>

Re: [PATCH 08/12] tests/plugin/bb: migrate to new per_vcpu API


On 1/12/24 01:23, Pierrick Bouvier wrote:

Since we need a fixed offset between count memory location, we now need
a contiguous array of CPUCount (instead of array of pointers).

Signed-off-by: Pierrick Bouvier
---
  tests/plugin/bb.c | 54 +++
  1 file changed, 27 insertions(+), 27 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH 07/12] tests/plugin/insn: migrate to new per_vcpu API


On 1/12/24 01:23, Pierrick Bouvier wrote:

Signed-off-by: Pierrick Bouvier 
---
  tests/plugin/insn.c | 14 ++
  1 file changed, 10 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson 


@@ -195,6 +200,7 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t 
id,
  fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
  return -1;
  }
+g_assert(info->system.smp_vcpus <= MAX_CPUS);


Alex, can we do better than aborting here?


r~

Re: [PATCH 05/12] tests/plugin/mem: fix race condition with callbacks


On 1/12/24 01:23, Pierrick Bouvier wrote:

Introduce a lock so global count is correct.
This was found by comparing with new inline per_vcpu inline op.

Signed-off-by: Pierrick Bouvier 
---
  tests/plugin/mem.c | 3 +++
  1 file changed, 3 insertions(+)


Reviewed-by: Richard Henderson 


r~

Re: [PATCH 04/12] tests/plugin/inline: migrate to new per_vcpu API


On 1/12/24 01:23, Pierrick Bouvier wrote:

Signed-off-by: Pierrick Bouvier 
---
  tests/plugin/inline.c | 17 -
  1 file changed, 17 deletions(-)


Was this supposed to be together with patch 6?

r~



diff --git a/tests/plugin/inline.c b/tests/plugin/inline.c
index 6114ebca545..ae59f7af7a7 100644
--- a/tests/plugin/inline.c
+++ b/tests/plugin/inline.c
@@ -18,15 +18,12 @@
  static uint64_t count_tb;
  static uint64_t count_tb_per_vcpu[MAX_CPUS];
  static uint64_t count_tb_inline_per_vcpu[MAX_CPUS];
-static uint64_t count_tb_inline_racy;
  static uint64_t count_insn;
  static uint64_t count_insn_per_vcpu[MAX_CPUS];
  static uint64_t count_insn_inline_per_vcpu[MAX_CPUS];
-static uint64_t count_insn_inline_racy;
  static uint64_t count_mem;
  static uint64_t count_mem_per_vcpu[MAX_CPUS];
  static uint64_t count_mem_inline_per_vcpu[MAX_CPUS];
-static uint64_t count_mem_inline_racy;
  static GMutex tb_lock;
  static GMutex insn_lock;
  static GMutex mem_lock;
@@ -50,11 +47,9 @@ static void stats_insn(void)
  printf("insn: %" PRIu64 "\n", expected);
  printf("insn: %" PRIu64 " (per vcpu)\n", per_vcpu);
  printf("insn: %" PRIu64 " (per vcpu inline)\n", inl_per_vcpu);
-printf("insn: %" PRIu64 " (inline racy)\n", count_insn_inline_racy);
  g_assert(expected > 0);
  g_assert(per_vcpu == expected);
  g_assert(inl_per_vcpu == expected);
-g_assert(count_insn_inline_racy <= expected);
  }
  
  static void stats_tb(void)

@@ -65,11 +60,9 @@ static void stats_tb(void)
  printf("tb: %" PRIu64 "\n", expected);
  printf("tb: %" PRIu64 " (per vcpu)\n", per_vcpu);
  printf("tb: %" PRIu64 " (per vcpu inline)\n", inl_per_vcpu);
-printf("tb: %" PRIu64 " (inline racy)\n", count_tb_inline_racy);
  g_assert(expected > 0);
  g_assert(per_vcpu == expected);
  g_assert(inl_per_vcpu == expected);
-g_assert(count_tb_inline_racy <= expected);
  }
  
  static void stats_mem(void)

@@ -80,11 +73,9 @@ static void stats_mem(void)
  printf("mem: %" PRIu64 "\n", expected);
  printf("mem: %" PRIu64 " (per vcpu)\n", per_vcpu);
  printf("mem: %" PRIu64 " (per vcpu inline)\n", inl_per_vcpu);
-printf("mem: %" PRIu64 " (inline racy)\n", count_mem_inline_racy);
  g_assert(expected > 0);
  g_assert(per_vcpu == expected);
  g_assert(inl_per_vcpu == expected);
-g_assert(count_mem_inline_racy <= expected);
  }
  
  static void plugin_exit(qemu_plugin_id_t id, void *udata)

@@ -142,8 +133,6 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct 
qemu_plugin_tb *tb)
  {
  qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
   QEMU_PLUGIN_CB_NO_REGS, 0);
-qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
- _tb_inline_racy, 1);
  qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
  tb, QEMU_PLUGIN_INLINE_ADD_U64,
  count_tb_inline_per_vcpu, sizeof(uint64_t), 1);
@@ -152,18 +141,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct 
qemu_plugin_tb *tb)
  struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, idx);
  qemu_plugin_register_vcpu_insn_exec_cb(insn, vcpu_insn_exec,
 QEMU_PLUGIN_CB_NO_REGS, 0);
-qemu_plugin_register_vcpu_insn_exec_inline(
-insn, QEMU_PLUGIN_INLINE_ADD_U64,
-_insn_inline_racy, 1);
  qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(
  insn, QEMU_PLUGIN_INLINE_ADD_U64,
  count_insn_inline_per_vcpu, sizeof(uint64_t), 1);
  qemu_plugin_register_vcpu_mem_cb(insn, _mem_access,
   QEMU_PLUGIN_CB_NO_REGS,
   QEMU_PLUGIN_MEM_RW, 0);
-qemu_plugin_register_vcpu_mem_inline(insn, QEMU_PLUGIN_MEM_RW,
- QEMU_PLUGIN_INLINE_ADD_U64,
- _mem_inline_racy, 1);
  qemu_plugin_register_vcpu_mem_inline_per_vcpu(
  insn, QEMU_PLUGIN_MEM_RW,
  QEMU_PLUGIN_INLINE_ADD_U64,

Re: [PATCH 02/12] plugins: add inline operation per vcpu


On 1/12/24 01:23, Pierrick Bouvier wrote:

Extends API with three new functions:
qemu_plugin_register_vcpu_{tb, insn, mem}_exec_inline_per_vcpu().

Compared to non per_vcpu versions, ptr is now a base, and current
cpu_index and an offset are used to compute memory location on which
operation happens (ptr + cpu_index * offset).

This allows to have a thread-safe version of inline operations.

Having a flexible offset is useful in case a user wants to target a
memory location embedded into a struct. In this case, the offset between
two memory locations will be bigger than sizeof(uint64_t).

Signed-off-by: Pierrick Bouvier
---
  include/qemu/qemu-plugin.h   | 56 +++-
  plugins/api.c| 36 ---
  plugins/qemu-plugins.symbols |  3 ++
  3 files changed, 90 insertions(+), 5 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH 01/12] plugins: implement inline operation with cpu_index offset


On 1/12/24 01:23, Pierrick Bouvier wrote:

Instead of working on a fixed memory location, allow to index it based
on cpu_index and a given offset (ptr + cpu_index * offset).
Current semantic is not modified as we use a 0 offset, thus inline
operation still targets always the same memory location.

Signed-off-by: Pierrick Bouvier 
---
  accel/tcg/plugin-gen.c | 60 +++---
  include/qemu/plugin.h  |  1 +
  plugins/api.c  |  7 ++---
  plugins/core.c | 11 +---
  plugins/plugin.h   |  5 ++--
  5 files changed, 65 insertions(+), 19 deletions(-)


Reviewed-by: Richard Henderson 

For the to-do list: add mul -> shl strength reduction in fold_mul().


r~

Re: [PATCH] accel/tcg: Include missing headers to 'tb-jmp-cache.h'


On 1/12/24 03:24, Philippe Mathieu-Daudé wrote:

Due to missing headers, when including "tb-jmp-cache.h" we might get:

   accel/tcg/tb-jmp-cache.h:21:21: error: field ‘rcu’ has incomplete type
  21 | struct rcu_head rcu;
 | ^~~
   accel/tcg/tb-jmp-cache.h:24:9: error: unknown type name ‘vaddr’
  24 | vaddr pc;
 | ^

Add the missing "qemu/rcu.h" and "exec/cpu-common.h" headers.

Signed-off-by: Philippe Mathieu-Daudé 


Acked-by: Richard Henderson 


r~


---
  accel/tcg/tb-jmp-cache.h | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/accel/tcg/tb-jmp-cache.h b/accel/tcg/tb-jmp-cache.h
index bb424c8a05..b13a02e45d 100644
--- a/accel/tcg/tb-jmp-cache.h
+++ b/accel/tcg/tb-jmp-cache.h
@@ -9,6 +9,9 @@
  #ifndef ACCEL_TCG_TB_JMP_CACHE_H
  #define ACCEL_TCG_TB_JMP_CACHE_H
  
+#include "qemu/rcu.h"

+#include "exec/cpu-common.h"
+
  #define TB_JMP_CACHE_BITS 12
  #define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)

Re: [PULL 15/21] linux-user/riscv: Add vdso

2024-01-11 Thread Vineet Gupta

Hi Richard, Alistair

On 10/30/23 14:17, Richard Henderson wrote:
> diff --git a/linux-user/riscv/Makefile.vdso b/linux-user/riscv/Makefile.vdso
> new file mode 100644
> index 00..2c257dbfda
> --- /dev/null
> +++ b/linux-user/riscv/Makefile.vdso
> @@ -0,0 +1,15 @@
> +include $(BUILD_DIR)/tests/tcg/riscv64-linux-user/config-target.mak
> +
> +SUBDIR = $(SRC_PATH)/linux-user/riscv
> +VPATH += $(SUBDIR)
> +
> +all: $(SUBDIR)/vdso-32.so $(SUBDIR)/vdso-64.so
> +
> +LDFLAGS = -nostdlib -shared -fpic -Wl,-h,linux-vdso.so.1 -Wl,--build-id=sha1 
> \
> +   -Wl,--hash-style=both -Wl,-T,$(SUBDIR)/vdso.ld
> +
> +$(SUBDIR)/vdso-32.so: vdso.S vdso.ld vdso-asmoffset.h
> + $(CC) -o $@ $(LDFLAGS) -mabi=ilp32d -march=rv32g $<
> +
> +$(SUBDIR)/vdso-64.so: vdso.S vdso.ld vdso-asmoffset.h
> + $(CC) -o $@ $(LDFLAGS) -mabi=lp64d -march=rv64g $<

So by default qemu ships the vdso binary. How can one rebuild it ?

>From skimming the build files it seems following ought to do it
    make update-linux-vdso

with a prior configure cmd like below with PATH pointing to the cross
compiler.
../configure  --target-list=riscv64-linux-user
--cross-cc-riscv64=riscv64-unknown-linux-gnu-gcc

But it doesn't, I'm sure we are missing something basis here.

The reason for trying to rebuild is, Edwin ran into an issue running gcc
testsuite for RISC-V.

TEst such as gcc/testsuite/gcc.dg/cleanup-10.c segv with new QEMU (with
exact same test binary)

With QEMU 8.1.2 it runs fine (exit code 0)
but with 8.2 it segv (exit code 134).

This is because it can't unwind out of signal stack and force_unwind
abort()s.

Edwin bisected this to following QEMU commit,

    2021-07-06 468c1bb5cac9 linux-user/riscv: Add vdso  

which kind of makes sense since this changes the rt_sigreturn trampoline
and associated unwinding stuff.

For starters we saw something that seems like a thinko in

diff --git a/linux-user/riscv/vdso.S b/linux-user/riscv/vdso.S
-#define sizeof_reg (__riscv_xlen / 4)
+#define sizeof_reg (__riscv_xlen / 8)

And wanted to see if that fixes it but can't really coax the build
system to rebuild the vdso as described above.

As as aside, we also see that rt_sigreturn in kernel vdso elides the
explicit the call frame information. Again we naively don't know if that
is required in qemu.

    .text
ENTRY(__vdso_rt_sigreturn)
    .cfi_startproc
    .cfi_signal_frame
    li a7, __NR_rt_sigreturn
    ecall
    .cfi_endproc
ENDPROC(__vdso_rt_sigreturn)


Would appreicate any answers and pointers.

Thx,


> diff --git a/linux-user/riscv/meson.build b/linux-user/riscv/meson.build
> new file mode 100644
> index 00..beb989a7ca
> --- /dev/null
> +++ b/linux-user/riscv/meson.build
> @@ -0,0 +1,7 @@
> +vdso_32_inc = gen_vdso.process('vdso-32.so',
> +   extra_args: ['-r', '__vdso_rt_sigreturn'])
> +vdso_64_inc = gen_vdso.process('vdso-64.so',
> +   extra_args: ['-r', '__vdso_rt_sigreturn'])
> +
> +linux_user_ss.add(when: 'TARGET_RISCV32', if_true: vdso_32_inc)
> +linux_user_ss.add(when: 'TARGET_RISCV64', if_true: vdso_64_inc)
> diff --git a/linux-user/riscv/vdso-32.so b/linux-user/riscv/vdso-32.so
> new file mode 100755
> index 
> ..1ad1e5c8b1fe36b0fe4bcb6c06fab8219ecd
> GIT binary patch
> literal 2900
> zcmb_eTWl0n7(TPZVyUHWDVSm#Q)tBoakE`3X|QQ_x7~7aX*Y!`UgC5+yX~&9yUp%w
> zLmR@yBHoaTR0Kq>L6L=zLABtH4zgAYa%jnRm{h}7?!nX^o_4?g&}-~Q*n
> z%s=PM`Oi1AEgb1m6h%l;#cx7dEpVPL6Ji#0i>Mc~MU$u!9%$NEaRFn3d4wv|^w
> zTRLpb7;DS=wp%clxP}go5H6@1BuN~CP00Gu?~M2%+(e=gF+#?vFF7z%d_LkRAy#(x
> ziH@zeoN%Z87eg1Tzxkm)cKE@*AHS_c%m7%c6BGLV%2aj>G?#S_n$Mt!IhI531+E1!
> zb!!dKcx z_D*OiRpQsFei%N)IbX)BH+hNx@;@l%HgX^T`9`e~Z1?i^n9( z1g}x5<}?R|$3vTWW zq|%A(U_3S|efp-XzEMX0<;*4W(ua%n;exaVHx=?pEv3iwWT|}4(DJFFT*4^iGGbFI
> zQ`mefZ|lW>vxuzj?%SZXc$;s>gdwhJf9!SZZ}R-lbJ=rs0Q-uj8Vu zlv?$Udf0qM9wfFxC!wRz6VR9G2Po=TuTZuqO6^>iV4WJ^OMpEfqLuY=G&6I30BbFy
> zA2MMnUcU|n0!nR#tLSzI;yVT&>K}h#=lu`gyKBQEcRjlPvEIj{Pjv5^7is=ZzN
> zI`@a33LI#kXnT6a!PaM%A8I-5KjM4Vdvw`zOP_CgVac)1$ zT3+$L>U+(5dfDqs-)MSsNwM**#c$W2x$#F=KbqGn16QOhn|lr}{z=*S
> z+2_AN!< zm+o=9s@>5)^*R-~LR437Z$I z`hy|- zWcyUJJ1h>QOEpi4#W|L(nwbF}76{Kig zuIGuiIM(}DX+aWbC8HPmWpr)bfhYp=kvXMt zJZSr2lk+Vp7hw$K`We4ZhM>u9gV0Xy8iVcR#yzl|T*jVra$ym+lRH0y?c_$G>(TsZ
> zB(~9;6^)yP)_G5NaP8_2p+$RpI>McOy zt2%pywldNa3P!Y^uC8_A^)k^xD>li^#7r|0-*i5k#$9%$5;Zzlu;e3=3gOL zw2`xPQjRiNBO$!;(M&9z(#-*Kp<_l!x3tZ!(roT7DyEc}5bsd@7rnW@vHYO(eC!
> zTr8c?l5u28OL ztdn8AgZR$kJCxnddVJ>-uHggF2
> z{; zrr`NvZC>O27K3SuT)P2F=8->$pX?kenYY!> vc~2SIg1ifGl4tmx!+P8|`R#(CDQ#Hj*V2HN^{Oz_
> literal 0
> HcmV?d1
>
> diff --git a/linux-user/riscv/vdso-64.so b/linux-user/riscv/vdso-64.so
> new file mode 100755
> index 
> ..83992bebe6d0182f24edfffc531015fd2f4e1cfb
> GIT binary patch
> literal 3856
> zcmc&%>u*#=6hC(_g{2g1p%}2$6tfXRaI z(h#Xy-@LSlh@eFjU#RsBzS<8a68*wQ{R51AFh(W%#Ya77?>Woe=_Y*egQvasH*+2{
> zbI;8DW_Eul-0jdb5YgcZT&)54*l?-dDl9

Re: [PATCH] cpus: Restrict 'start-powered-off' property to system emulation


On 1/12/24 03:18, Philippe Mathieu-Daudé wrote:

Since the CPUState::start-powered-off property is irrelevant
to user emulation, restrict it to system emulation.

Signed-off-by: Philippe Mathieu-Daudé 
---
  cpu-target.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)


Reviewed-by: Richard Henderson 


r~

Re: [PATCH] system/watchpoint: Move TCG specific code to accel/tcg/


On 1/12/24 03:20, Philippe Mathieu-Daudé wrote:

Keep system/watchpoint.c accelerator-agnostic by moving
TCG specific code to accel/tcg/watchpoint.c. Update meson.

Signed-off-by: Philippe Mathieu-Daudé
---
  accel/tcg/watchpoint.c | 143 +
  system/watchpoint.c| 124 ---
  accel/tcg/meson.build  |   1 +
  3 files changed, 144 insertions(+), 124 deletions(-)
  create mode 100644 accel/tcg/watchpoint.c


Reviewed-by: Richard Henderson 

r~

[PATCH 2/2] qga: Solaris has net/if_arp.h and netinet/if_ether.h but not ETHER_ADDR_LEN

2024-01-11 Thread Nick Briggs

Solaris has net/if_arp.h and netinet/if_ether.h rather than net/ethernet.h,
but does not define ETHER_ADDR_LEN, instead providing ETHERADDRL.

Signed-off-by: Nick Briggs 
---
 qga/commands-posix.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 6169bbf7a0..26008db497 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -45,9 +45,12 @@
 #include 
 #include 
 #include 
-#if defined(__NetBSD__) || defined(__OpenBSD__)
+#if defined(__NetBSD__) || defined(__OpenBSD__) || defined(CONFIG_SOLARIS)
 #include 
 #include 
+#if !defined(ETHER_ADDR_LEN) && defined(ETHERADDRL)
+#define ETHER_ADDR_LEN ETHERADDRL
+#endif
 #else
 #include 
 #endif
-- 
2.31.1

[PATCH 1/2] migration/rdma: define htonll/ntohll only if not predefined

2024-01-11 Thread Nick Briggs

Solaris has #defines for htonll and ntohll which cause syntax errors
when compiling code that attempts to (re)define these functions..

Signed-off-by: Nick Briggs 
---
 migration/rdma.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/migration/rdma.c b/migration/rdma.c
index 94c0f871f0..a355dcea89 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -238,6 +238,7 @@ static const char *control_desc(unsigned int rdma_control)
 return strs[rdma_control];
 }
 
+#if !defined(htonll)
 static uint64_t htonll(uint64_t v)
 {
 union { uint32_t lv[2]; uint64_t llv; } u;
@@ -245,13 +246,16 @@ static uint64_t htonll(uint64_t v)
 u.lv[1] = htonl(v & 0xULL);
 return u.llv;
 }
+#endif
 
+#if !defined(ntohll)
 static uint64_t ntohll(uint64_t v)
 {
 union { uint32_t lv[2]; uint64_t llv; } u;
 u.llv = v;
 return ((uint64_t)ntohl(u.lv[0]) << 32) | (uint64_t) ntohl(u.lv[1]);
 }
+#endif
 
 static void dest_block_to_network(RDMADestBlock *db)
 {
-- 
2.31.1

RE: [PATCH v2 8/9] Hexagon (target/hexagon) Remove unused WRITES_PRED_REG attribute



> -Original Message-
> From: Taylor Simpson 
> Sent: Sunday, December 10, 2023 4:07 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain ; Matheus Bernardino (QUIC)
> ; Sid Manning ; Marco
> Liebel (QUIC) ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng; ltaylorsimp...@gmail.com
> Subject: [PATCH v2 8/9] Hexagon (target/hexagon) Remove unused
> WRITES_PRED_REG attribute
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> This is the only remaining use of the is_written function.  We will
> remove it in the subsequent commit.
> 
> Signed-off-by: Taylor Simpson 
> ---
>  target/hexagon/attribs_def.h.inc |  1 -
>  target/hexagon/hex_common.py | 11 ---
>  2 files changed, 12 deletions(-)
> 
> diff --git a/target/hexagon/attribs_def.h.inc 
> b/target/hexagon/attribs_def.h.inc
> index 21d457fa4a..87942d46f4 100644
> --- a/target/hexagon/attribs_def.h.inc
> +++ b/target/hexagon/attribs_def.h.inc
> @@ -117,7 +117,6 @@ DEF_ATTRIB(IMPLICIT_READS_P1, "Reads the P1
> register", "", "")
>  DEF_ATTRIB(IMPLICIT_READS_P2, "Reads the P2 register", "", "")
>  DEF_ATTRIB(IMPLICIT_READS_P3, "Reads the P3 register", "", "")
>  DEF_ATTRIB(IMPLICIT_WRITES_USR, "May write USR", "", "")
> -DEF_ATTRIB(WRITES_PRED_REG, "Writes a predicate register", "", "")
>  DEF_ATTRIB(COMMUTES, "The operation is communitive", "", "")
>  DEF_ATTRIB(DEALLOCRET, "dealloc_return", "", "")
>  DEF_ATTRIB(DEALLOCFRAME, "deallocframe", "", "")
> diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py
> index 4565dd1953..ca5e9630c1 100755
> --- a/target/hexagon/hex_common.py
> +++ b/target/hexagon/hex_common.py
> @@ -94,10 +94,6 @@ def is_cond_call(tag):
>  def calculate_attribs():
>  add_qemu_macro_attrib("fREAD_PC", "A_IMPLICIT_READS_PC")
>  add_qemu_macro_attrib("fTRAP", "A_IMPLICIT_READS_PC")
> -add_qemu_macro_attrib("fWRITE_P0", "A_WRITES_PRED_REG")
> -add_qemu_macro_attrib("fWRITE_P1", "A_WRITES_PRED_REG")
> -add_qemu_macro_attrib("fWRITE_P2", "A_WRITES_PRED_REG")
> -add_qemu_macro_attrib("fWRITE_P3", "A_WRITES_PRED_REG")
>  add_qemu_macro_attrib("fSET_OVERFLOW", "A_IMPLICIT_WRITES_USR")
>  add_qemu_macro_attrib("fSET_LPCFG", "A_IMPLICIT_WRITES_USR")
>  add_qemu_macro_attrib("fLOAD", "A_SCALAR_LOAD")
> @@ -122,13 +118,6 @@ def calculate_attribs():
>  continue
>  macro = macros[macname]
>  attribdict[tag] |= set(macro.attribs)
> -# Figure out which instructions write predicate registers
> -tagregs = get_tagregs()
> -for tag in tags:
> -regs = tagregs[tag]
> -for regtype, regid in regs:
> -if regtype == "P" and is_written(regid):
> -attribdict[tag].add("A_WRITES_PRED_REG")
>  # Mark conditional jumps and calls
>  # Not all instructions are properly marked with A_CONDEXEC
>  for tag in tags:
> --
> 2.34.1


Reviewed-by: Brian Cain

RE: [PATCH v2 9/9] Hexagon (target/hexagon) Remove dead functions from hex_common.py



> -Original Message-
> From: Taylor Simpson 
> Sent: Sunday, December 10, 2023 4:07 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain ; Matheus Bernardino (QUIC)
> ; Sid Manning ; Marco
> Liebel (QUIC) ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng; ltaylorsimp...@gmail.com
> Subject: [PATCH v2 9/9] Hexagon (target/hexagon) Remove dead functions
> from hex_common.py
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> These functions are no longer used after making the generators
> object oriented.
> 
> Signed-off-by: Taylor Simpson 
> ---
>  target/hexagon/hex_common.py | 51 
>  1 file changed, 51 deletions(-)
> 
> diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py
> index ca5e9630c1..195620c7ec 100755
> --- a/target/hexagon/hex_common.py
> +++ b/target/hexagon/hex_common.py
> @@ -33,9 +33,6 @@
>  overrides = {}  # tags with helper overrides
>  idef_parser_enabled = {}  # tags enabled for idef-parser
> 
> -def bad_register(regtype, regid):
> -raise Exception(f"Bad register parse: regtype '{regtype}' regid 
> '{regid}'")
> -
>  # We should do this as a hash for performance,
>  # but to keep order let's keep it as a list.
>  def uniquify(seq):
> @@ -200,46 +197,6 @@ def get_tagimms():
>  return dict(zip(tags, list(map(compute_tag_immediates, tags
> 
> 
> -def is_pair(regid):
> -return len(regid) == 2
> -
> -
> -def is_single(regid):
> -return len(regid) == 1
> -
> -
> -def is_written(regid):
> -return regid[0] in "dexy"
> -
> -
> -def is_writeonly(regid):
> -return regid[0] in "de"
> -
> -
> -def is_read(regid):
> -return regid[0] in "stuvwxy"
> -
> -
> -def is_readwrite(regid):
> -return regid[0] in "xy"
> -
> -
> -def is_scalar_reg(regtype):
> -return regtype in "RPC"
> -
> -
> -def is_hvx_reg(regtype):
> -return regtype in "VQ"
> -
> -
> -def is_old_val(regtype, regid, tag):
> -return regtype + regid + "V" in semdict[tag]
> -
> -
> -def is_new_val(regtype, regid, tag):
> -return regtype + regid + "N" in semdict[tag]
> -
> -
>  def need_slot(tag):
>  if (
>  "A_CVI_SCATTER" not in attribdict[tag]
> @@ -280,14 +237,6 @@ def skip_qemu_helper(tag):
>  return tag in overrides.keys()
> 
> 
> -def is_tmp_result(tag):
> -return "A_CVI_TMP" in attribdict[tag] or "A_CVI_TMP_DST" in
> attribdict[tag]
> -
> -
> -def is_new_result(tag):
> -return "A_CVI_NEW" in attribdict[tag]
> -
> -
>  def is_idef_parser_enabled(tag):
>  return tag in idef_parser_enabled
> 
> --
> 2.34.1


Reviewed-by: Brian Cain

RE: [PATCH v2 6/9] Hexagon (target/hexagon) Make generators object oriented - gen_op_regs



> -Original Message-
> From: Taylor Simpson 
> Sent: Sunday, December 10, 2023 4:07 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain ; Matheus Bernardino (QUIC)
> ; Sid Manning ; Marco
> Liebel (QUIC) ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng; ltaylorsimp...@gmail.com
> Subject: [PATCH v2 6/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_op_regs
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> Reviewed-by: Brian Cain 
> Signed-off-by: Taylor Simpson 
> ---
>  target/hexagon/gen_op_regs.py | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/hexagon/gen_op_regs.py b/target/hexagon/gen_op_regs.py
> index a8a7712129..7b7b33895a 100755
> --- a/target/hexagon/gen_op_regs.py
> +++ b/target/hexagon/gen_op_regs.py
> @@ -70,6 +70,7 @@ def strip_reg_prefix(x):
>  def main():
>  hex_common.read_semantics_file(sys.argv[1])
>  hex_common.read_attribs_file(sys.argv[2])
> +hex_common.init_registers()
>  tagregs = hex_common.get_tagregs(full=True)
>  tagimms = hex_common.get_tagimms()
> 
> @@ -80,11 +81,12 @@ def main():
>  wregs = []
>  regids = ""
>  for regtype, regid, _, numregs in regs:
> -if hex_common.is_read(regid):
> +reg = hex_common.get_register(tag, regtype, regid)
> +if reg.is_read():
>  if regid[0] not in regids:
>  regids += regid[0]
>  rregs.append(regtype + regid + numregs)
> -if hex_common.is_written(regid):
> +if reg.is_written():
>  wregs.append(regtype + regid + numregs)
>  if regid[0] not in regids:
>  regids += regid[0]
> --
> 2.34.1

Reviewed-by: Brian Cain

RE: [PATCH v2 4/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_funcs



> -Original Message-
> From: Taylor Simpson 
> Sent: Sunday, December 10, 2023 4:07 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain ; Matheus Bernardino (QUIC)
> ; Sid Manning ; Marco
> Liebel (QUIC) ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng; ltaylorsimp...@gmail.com
> Subject: [PATCH v2 4/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_helper_funcs
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> Signed-off-by: Taylor Simpson 
> ---
>  target/hexagon/gen_helper_funcs.py | 368 +
>  target/hexagon/hex_common.py   |  48 +++-
>  2 files changed, 103 insertions(+), 313 deletions(-)
> 
> diff --git a/target/hexagon/gen_helper_funcs.py
> b/target/hexagon/gen_helper_funcs.py
> index ce21d3b688..9cc3d69c49 100755
> --- a/target/hexagon/gen_helper_funcs.py
> +++ b/target/hexagon/gen_helper_funcs.py
> @@ -23,181 +23,14 @@
>  import hex_common
> 
> 
> -##
> -## Helpers for gen_helper_function
> -##
> -def gen_decl_ea(f):
> -f.write("uint32_t EA;\n")
> -
> -
> -def gen_helper_return_type(f, regtype, regid, regno):
> -if regno > 1:
> -f.write(", ")
> -f.write("int32_t")
> -
> -
> -def gen_helper_return_type_pair(f, regtype, regid, regno):
> -if regno > 1:
> -f.write(", ")
> -f.write("int64_t")
> -
> -
> -def gen_helper_arg(f, regtype, regid, regno):
> -if regno > 0:
> -f.write(", ")
> -f.write(f"int32_t {regtype}{regid}V")
> -
> -
> -def gen_helper_arg_new(f, regtype, regid, regno):
> -if regno >= 0:
> -f.write(", ")
> -f.write(f"int32_t {regtype}{regid}N")
> -
> -
> -def gen_helper_arg_pair(f, regtype, regid, regno):
> -if regno >= 0:
> -f.write(", ")
> -f.write(f"int64_t {regtype}{regid}V")
> -
> -
> -def gen_helper_arg_ext(f, regtype, regid, regno):
> -if regno > 0:
> -f.write(", ")
> -f.write(f"void *{regtype}{regid}V_void")
> -
> -
> -def gen_helper_arg_ext_pair(f, regtype, regid, regno):
> -if regno > 0:
> -f.write(", ")
> -f.write(f"void *{regtype}{regid}V_void")
> -
> -
> -def gen_helper_arg_opn(f, regtype, regid, i, tag):
> -if hex_common.is_pair(regid):
> -if hex_common.is_hvx_reg(regtype):
> -gen_helper_arg_ext_pair(f, regtype, regid, i)
> -else:
> -gen_helper_arg_pair(f, regtype, regid, i)
> -elif hex_common.is_single(regid):
> -if hex_common.is_old_val(regtype, regid, tag):
> -if hex_common.is_hvx_reg(regtype):
> -gen_helper_arg_ext(f, regtype, regid, i)
> -else:
> -gen_helper_arg(f, regtype, regid, i)
> -elif hex_common.is_new_val(regtype, regid, tag):
> -gen_helper_arg_new(f, regtype, regid, i)
> -else:
> -hex_common.bad_register(regtype, regid)
> -else:
> -hex_common.bad_register(regtype, regid)
> -
> -
> -def gen_helper_arg_imm(f, immlett):
> -f.write(f", int32_t {hex_common.imm_name(immlett)}")
> -
> -
> -def gen_helper_dest_decl(f, regtype, regid, regno, subfield=""):
> -f.write(f"int32_t {regtype}{regid}V{subfield} = 0;\n")
> -
> -
> -def gen_helper_dest_decl_pair(f, regtype, regid, regno, subfield=""):
> -f.write(f"int64_t {regtype}{regid}V{subfield} = 0;\n")
> -
> -
> -def gen_helper_dest_decl_ext(f, regtype, regid):
> -if regtype == "Q":
> -f.write(
> -f"/* {regtype}{regid}V is *(MMQReg *)" 
> f"({regtype}{regid}V_void)
> */\n"
> -)
> -else:
> -f.write(
> -f"/* {regtype}{regid}V is *(MMVector *)"
> -f"({regtype}{regid}V_void) */\n"
> -)
> -
> -
> -def gen_helper_dest_decl_ext_pair(f, regtype, regid, regno):
> -f.write(
> -f"/* {regtype}{regid}V is *(MMVectorPair *))"
> -f"{regtype}{regid}V_void) */\n"
> -)
> -
> -
> -def gen_helper_dest_decl_opn(f, regtype, regid, i):
> -if hex_common.is_pair(regid):
> -if hex_common.is_hvx_reg(regtype):
> -gen_helper_dest_decl_ext_pair(f, regtype, regid, i)
> -else:
> -gen_helper_dest_decl_pair(f, regtype, regid, i)
> -elif hex_common.is_single(regid):
> -if hex_common.is_hvx_reg(regtype):
> -gen_helper_dest_decl_ext(f, regtype, regid)
> -else:
> -gen_helper_dest_decl(f, regtype, regid, i)
> -else:
> -hex_common.bad_register(regtype, regid)
> -
> -
> -def gen_helper_src_var_ext(f, regtype, regid):
> -if regtype == "Q":
> -f.write(
> -f"/* {regtype}{regid}V is *(MMQReg *)" 
> f"({regtype}{regid}V_void)
> */\n"
> -)
> -else:
> -f.write(
> -f"/* {regtype}{regid}V is *(MMVector *)"
> -f"({regtype}{regid}V_void) */\n"
> -)
> -
> -
> -def gen_helper_src_var_ext_pair(f, regtype, regid, regno):
>

RE: [PATCH v2 7/9] Hexagon (target/hexagon) Make generators object oriented - gen_analyze_funcs



> -Original Message-
> From: Taylor Simpson 
> Sent: Sunday, December 10, 2023 4:07 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain ; Matheus Bernardino (QUIC)
> ; Sid Manning ; Marco
> Liebel (QUIC) ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng; ltaylorsimp...@gmail.com
> Subject: [PATCH v2 7/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_analyze_funcs
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> This patch conflicts with
> https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg00729.html
> If that series goes in first, we'll rework this patch and vice versa.
> 
> Signed-off-by: Taylor Simpson 
> ---
>  target/hexagon/gen_analyze_funcs.py | 163 +---
>  target/hexagon/hex_common.py| 151 ++
>  2 files changed, 157 insertions(+), 157 deletions(-)
> 
> diff --git a/target/hexagon/gen_analyze_funcs.py
> b/target/hexagon/gen_analyze_funcs.py
> index c3b521abef..a9af666cef 100755
> --- a/target/hexagon/gen_analyze_funcs.py
> +++ b/target/hexagon/gen_analyze_funcs.py
> @@ -23,162 +23,6 @@
>  import hex_common
> 
> 
> -##
> -## Helpers for gen_analyze_func
> -##
> -def is_predicated(tag):
> -return "A_CONDEXEC" in hex_common.attribdict[tag]
> -
> -
> -def analyze_opn_old(f, tag, regtype, regid, regno):
> -regN = f"{regtype}{regid}N"
> -predicated = "true" if is_predicated(tag) else "false"
> -if regtype == "R":
> -if regid in {"ss", "tt"}:
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(f"ctx_log_reg_read_pair(ctx, {regN});\n")
> -elif regid in {"dd", "ee", "xx", "yy"}:
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(f"ctx_log_reg_write_pair(ctx, {regN}, 
> {predicated});\n")
> -elif regid in {"s", "t", "u", "v"}:
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(f"ctx_log_reg_read(ctx, {regN});\n")
> -elif regid in {"d", "e", "x", "y"}:
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(f"ctx_log_reg_write(ctx, {regN}, {predicated});\n")
> -else:
> -hex_common.bad_register(regtype, regid)
> -elif regtype == "P":
> -if regid in {"s", "t", "u", "v"}:
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(f"ctx_log_pred_read(ctx, {regN});\n")
> -elif regid in {"d", "e", "x"}:
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(f"ctx_log_pred_write(ctx, {regN});\n")
> -else:
> -hex_common.bad_register(regtype, regid)
> -elif regtype == "C":
> -if regid == "ss":
> -f.write(
> -f"const int {regN} = insn->regno[{regno}] "
> -"+ HEX_REG_SA0;\n"
> -)
> -f.write(f"ctx_log_reg_read_pair(ctx, {regN});\n")
> -elif regid == "dd":
> -f.write(f"const int {regN} = insn->regno[{regno}] " "+
> HEX_REG_SA0;\n")
> -f.write(f"ctx_log_reg_write_pair(ctx, {regN}, 
> {predicated});\n")
> -elif regid == "s":
> -f.write(
> -f"const int {regN} = insn->regno[{regno}] "
> -"+ HEX_REG_SA0;\n"
> -)
> -f.write(f"ctx_log_reg_read(ctx, {regN});\n")
> -elif regid == "d":
> -f.write(f"const int {regN} = insn->regno[{regno}] " "+
> HEX_REG_SA0;\n")
> -f.write(f"ctx_log_reg_write(ctx, {regN}, {predicated});\n")
> -else:
> -hex_common.bad_register(regtype, regid)
> -elif regtype == "M":
> -if regid == "u":
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(f"ctx_log_reg_read(ctx, {regN});\n")
> -else:
> -hex_common.bad_register(regtype, regid)
> -elif regtype == "V":
> -newv = "EXT_DFL"
> -if hex_common.is_new_result(tag):
> -newv = "EXT_NEW"
> -elif hex_common.is_tmp_result(tag):
> -newv = "EXT_TMP"
> -if regid in {"dd", "xx"}:
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(
> -f"ctx_log_vreg_write_pair(ctx, {regN}, {newv}, " 
> f"{predicated});\n"
> -)
> -elif regid in {"uu", "vv"}:
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(f"ctx_log_vreg_read_pair(ctx, {regN});\n")
> -elif regid in {"s", "u", "v", "w"}:
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(f"ctx_log_vreg_read(ctx, {regN});\n")
> -elif regid in {"d", "x",

RE: [PATCH v2 2/9] Hexagon (target/hexagon) Make generators object oriented - gen_tcg_funcs



> -Original Message-
> From: Taylor Simpson 
> Sent: Sunday, December 10, 2023 4:07 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain ; Matheus Bernardino (QUIC)
> ; Sid Manning ; Marco
> Liebel (QUIC) ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng; ltaylorsimp...@gmail.com
> Subject: [PATCH v2 2/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_tcg_funcs
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> The generators are generally a bunch of Python if-then-else
> statements based on the regtype and regid.  Encapsulate regtype/regid
> into a class hierarchy.  Clients lookup the register and invoke
> methods.
> 
> This has several advantages for making the code easier to read,
> understand, and maintain
> - The class name makes it more clear what the operand does
> - All the methods for a given type of operand are together
> - Don't need hex_common.bad_register
>   If a regtype/regid is missing, the lookup in hex_common.get_register
>   will fail
> - We can remove the functions in hex_common that use regtype/regid
>   (e.g., is_read)
> 
> This patch creates the class hierarchy in hex_common and converts
> gen_tcg_funcs.py.  The other scripts will be converted in subsequent
> patches in this series.
> 
> Signed-off-by: Taylor Simpson 
> ---
>  target/hexagon/gen_tcg_funcs.py | 571 ++-
>  target/hexagon/hex_common.py| 659
> 
>  2 files changed, 683 insertions(+), 547 deletions(-)
> 
> diff --git a/target/hexagon/gen_tcg_funcs.py
> b/target/hexagon/gen_tcg_funcs.py
> index 02d93bc5ce..3d8e3cb6a2 100755
> --- a/target/hexagon/gen_tcg_funcs.py
> +++ b/target/hexagon/gen_tcg_funcs.py
> @@ -23,466 +23,13 @@
>  import hex_common
> 
> 
> -##
> -## Helpers for gen_tcg_func
> -##
> -def gen_decl_ea_tcg(f, tag):
> -f.write("TCGv EA G_GNUC_UNUSED = tcg_temp_new();\n")
> -
> -
> -def genptr_decl_pair_writable(f, tag, regtype, regid, regno):
> -regN = f"{regtype}{regid}N"
> -if regtype == "R":
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -elif regtype == "C":
> -f.write(f"const int {regN} = insn->regno[{regno}] + 
> HEX_REG_SA0;\n")
> -else:
> -hex_common.bad_register(regtype, regid)
> -f.write(f"TCGv_i64 {regtype}{regid}V = " f"get_result_gpr_pair(ctx,
> {regN});\n")
> -
> -
> -def genptr_decl_writable(f, tag, regtype, regid, regno):
> -regN = f"{regtype}{regid}N"
> -if regtype == "R":
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(f"TCGv {regtype}{regid}V = get_result_gpr(ctx, 
> {regN});\n")
> -elif regtype == "C":
> -f.write(f"const int {regN} = insn->regno[{regno}] + 
> HEX_REG_SA0;\n")
> -f.write(f"TCGv {regtype}{regid}V = get_result_gpr(ctx, 
> {regN});\n")
> -elif regtype == "P":
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -f.write(f"TCGv {regtype}{regid}V = tcg_temp_new();\n")
> -else:
> -hex_common.bad_register(regtype, regid)
> -
> -
> -def genptr_decl(f, tag, regtype, regid, regno):
> -regN = f"{regtype}{regid}N"
> -if regtype == "R":
> -if regid in {"ss", "tt"}:
> -f.write(f"TCGv_i64 {regtype}{regid}V = 
> tcg_temp_new_i64();\n")
> -f.write(f"const int {regN} = insn->regno[{regno}];\n")
> -elif regid in {"dd", "ee", "xx", "yy"}:
> -genptr_decl_pair_writable(f, tag, regtype, regid, regno)
> -elif regid in {"s", "t", "u", "v"}:
> -f.write(
> -f"TCGv {regtype}{regid}V = " 
> f"hex_gpr[insn->regno[{regno}]];\n"
> -)
> -elif regid in {"d", "e", "x", "y"}:
> -genptr_decl_writable(f, tag, regtype, regid, regno)
> -else:
> -hex_common.bad_register(regtype, regid)
> -elif regtype == "P":
> -if regid in {"s", "t", "u", "v"}:
> -f.write(
> -f"TCGv {regtype}{regid}V = " 
> f"hex_pred[insn->regno[{regno}]];\n"
> -)
> -elif regid in {"d", "e", "x"}:
> -genptr_decl_writable(f, tag, regtype, regid, regno)
> -else:
> -hex_common.bad_register(regtype, regid)
> -elif regtype == "C":
> -if regid == "ss":
> -f.write(f"TCGv_i64 {regtype}{regid}V = " 
> f"tcg_temp_new_i64();\n")
> -f.write(f"const int {regN} = insn->regno[{regno}] + "
> "HEX_REG_SA0;\n")
> -elif regid == "dd":
> -genptr_decl_pair_writable(f, tag, regtype, regid, regno)
> -elif regid == "s":
> -f.write(f"TCGv {regtype}{regid}V = tcg_temp_new();\n")
> -f.write(
> -f"const int {regtype}{regid}N = insn->regno[{regno}] + "
> -"HEX_REG_SA0;\n"
> -)
> -

RE: [PATCH v2 3/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_protos



> -Original Message-
> From: Taylor Simpson 
> Sent: Sunday, December 10, 2023 4:07 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain ; Matheus Bernardino (QUIC)
> ; Sid Manning ; Marco
> Liebel (QUIC) ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng; ltaylorsimp...@gmail.com
> Subject: [PATCH v2 3/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_helper_protos
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> Signed-off-by: Taylor Simpson 
> ---
>  target/hexagon/gen_helper_protos.py | 149 ++--
>  target/hexagon/hex_common.py|   7 --
>  2 files changed, 8 insertions(+), 148 deletions(-)
> 
> diff --git a/target/hexagon/gen_helper_protos.py
> b/target/hexagon/gen_helper_protos.py
> index 131043795a..c82b0f54e4 100755
> --- a/target/hexagon/gen_helper_protos.py
> +++ b/target/hexagon/gen_helper_protos.py
> @@ -22,39 +22,6 @@
>  import string
>  import hex_common
> 
> -##
> -## Helpers for gen_helper_prototype
> -##
> -def_helper_types = {
> -"N": "s32",
> -"O": "s32",
> -"P": "s32",
> -"M": "s32",
> -"C": "s32",
> -"R": "s32",
> -"V": "ptr",
> -"Q": "ptr",
> -}
> -
> -def_helper_types_pair = {
> -"R": "s64",
> -"C": "s64",
> -"S": "s64",
> -"G": "s64",
> -"V": "ptr",
> -"Q": "ptr",
> -}
> -
> -
> -def gen_def_helper_opn(f, tag, regtype, regid, i):
> -if hex_common.is_pair(regid):
> -f.write(f", {def_helper_types_pair[regtype]}")
> -elif hex_common.is_single(regid):
> -f.write(f", {def_helper_types[regtype]}")
> -else:
> -hex_common.bad_register(regtype, regid)
> -
> -
>  ##
>  ## Generate the DEF_HELPER prototype for an instruction
>  ## For A2_add: Rd32=add(Rs32,Rt32)
> @@ -65,116 +32,15 @@ def gen_helper_prototype(f, tag, tagregs, tagimms):
>  regs = tagregs[tag]
>  imms = tagimms[tag]
> 
> -numresults = 0
> -numscalarresults = 0
> -numscalarreadwrite = 0
> -for regtype, regid in regs:
> -if hex_common.is_written(regid):
> -numresults += 1
> -if hex_common.is_scalar_reg(regtype):
> -numscalarresults += 1
> -if hex_common.is_readwrite(regid):
> -if hex_common.is_scalar_reg(regtype):
> -numscalarreadwrite += 1
> -
> -if numscalarresults > 1:
> -## The helper is bogus when there is more than one result
> -f.write(f"DEF_HELPER_1({tag}, void, env)\n")
> -else:
> -## Figure out how many arguments the helper will take
> -if numscalarresults == 0:
> -def_helper_size = len(regs) + len(imms) + numscalarreadwrite + 1
> -if hex_common.need_pkt_has_multi_cof(tag):
> -def_helper_size += 1
> -if hex_common.need_pkt_need_commit(tag):
> -def_helper_size += 1
> -if hex_common.need_part1(tag):
> -def_helper_size += 1
> -if hex_common.need_slot(tag):
> -def_helper_size += 1
> -if hex_common.need_PC(tag):
> -def_helper_size += 1
> -if hex_common.helper_needs_next_PC(tag):
> -def_helper_size += 1
> -if hex_common.need_condexec_reg(tag, regs):
> -def_helper_size += 1
> -f.write(f"DEF_HELPER_{def_helper_size}({tag}")
> -## The return type is void
> -f.write(", void")
> -else:
> -def_helper_size = len(regs) + len(imms) + numscalarreadwrite
> -if hex_common.need_pkt_has_multi_cof(tag):
> -def_helper_size += 1
> -if hex_common.need_pkt_need_commit(tag):
> -def_helper_size += 1
> -if hex_common.need_part1(tag):
> -def_helper_size += 1
> -if hex_common.need_slot(tag):
> -def_helper_size += 1
> -if hex_common.need_PC(tag):
> -def_helper_size += 1
> -if hex_common.need_condexec_reg(tag, regs):
> -def_helper_size += 1
> -if hex_common.helper_needs_next_PC(tag):
> -def_helper_size += 1
> -f.write(f"DEF_HELPER_{def_helper_size}({tag}")
> -
> -## Generate the qemu DEF_HELPER type for each result
> -## Iterate over this list twice
> -## - Emit the scalar result
> -## - Emit the vector result
> -i = 0
> -for regtype, regid in regs:
> -if hex_common.is_written(regid):
> -if not hex_common.is_hvx_reg(regtype):
> -gen_def_helper_opn(f, tag, regtype, regid, i)
> -i += 1
> -
> -## Put the env between the outputs and inputs
> -f.write(", env")
> -i += 1
> -
> -# Second pass
> -for regtype, regid in regs:
> -if

RE: [PATCH v2 5/9] Hexagon (target/hexagon) Make generators object oriented - gen_idef_parser_funcs



> -Original Message-
> From: Taylor Simpson 
> Sent: Sunday, December 10, 2023 4:07 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain ; Matheus Bernardino (QUIC)
> ; Sid Manning ; Marco
> Liebel (QUIC) ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng; ltaylorsimp...@gmail.com
> Subject: [PATCH v2 5/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_idef_parser_funcs
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> Signed-off-by: Taylor Simpson 
> ---
>  target/hexagon/gen_idef_parser_funcs.py | 20 
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/target/hexagon/gen_idef_parser_funcs.py
> b/target/hexagon/gen_idef_parser_funcs.py
> index f4518e653f..550a48cb7b 100644
> --- a/target/hexagon/gen_idef_parser_funcs.py
> +++ b/target/hexagon/gen_idef_parser_funcs.py
> @@ -46,6 +46,7 @@ def main():
>  hex_common.read_semantics_file(sys.argv[1])
>  hex_common.read_attribs_file(sys.argv[2])
>  hex_common.calculate_attribs()
> +hex_common.init_registers()
>  tagregs = hex_common.get_tagregs()
>  tagimms = hex_common.get_tagimms()
> 
> @@ -132,22 +133,9 @@ def main():
> 
>  arguments = []
>  for regtype, regid in regs:
> -prefix = "in " if hex_common.is_read(regid) else ""
> -
> -is_pair = hex_common.is_pair(regid)
> -is_single_old = hex_common.is_single(regid) and
> hex_common.is_old_val(
> -regtype, regid, tag
> -)
> -is_single_new = hex_common.is_single(regid) and
> hex_common.is_new_val(
> -regtype, regid, tag
> -)
> -
> -if is_pair or is_single_old:
> -arguments.append(f"{prefix}{regtype}{regid}V")
> -elif is_single_new:
> -arguments.append(f"{prefix}{regtype}{regid}N")
> -else:
> -hex_common.bad_register(regtype, regid)
> +reg = hex_common.get_register(tag, regtype, regid)
> +prefix = "in " if reg.is_read() else ""
> +arguments.append(f"{prefix}{reg.reg_tcg()}")
> 
>  for immlett, bits, immshift in imms:
>  arguments.append(hex_common.imm_name(immlett))
> --
> 2.34.1

Reviewed-by: Brian Cain

Re: Possible race condition in aspeed ast2600 smp boot on TCG QEMU

2024-01-11 Thread Stephen Longfield

Sort of resolved: We were able to find a good-enough workaround. In case
anyone else is running into this, here's what we did:

By dropping to the uboot console and running the command

```
mw.l 0x1e6e2188 0xbabecafe
```

The magic number is set in the SCU regardless of how the race goes, and the
2nd core gets released from its mailbox polling loop.

On Thu, Jan 11, 2024 at 9:38 AM Stephen Longfield 
wrote:

> We’ve noticed inconsistent behavior when running a large number of aspeed
> ast2600 executions, that seems to be tied to a race condition in the smp
> boot when executing on TCG-QEMU, and were wondering what a good mediation
> strategy might be.
>
> The problem first shows up as part of SMP boot. On a run that’s likely to
> later run into issues, we’ll see something like:
>
> ```
> [0.008350] smp: Bringing up secondary CPUs ...
> [1.168584] CPU1: failed to come online
> [1.187277] smp: Brought up 1 node, 1 CPU
> ```
>
> Compared to the more likely to succeed:
>
> ```
> [0.080313] smp: Bringing up secondary CPUs ...
> [0.093166] smp: Brought up 1 node, 2 CPUs
> [0.093345] SMP: Total of 2 processors activated (4800.00 BogoMIPS).
> ```
>
> It’s somewhat reliably reproducible by running the ast2600-evb with an
> OpenBMC image, using ‘-icount auto’ to slow execution and make the race
> condition more frequent (it happens without this, just easier to debug if
> we can reproduce):
>
>
> ```
> ./aarch64-softmmu/qemu-system-aarch64 -machine ast2600-evb -nographic
> -drive file=~/bmc-bin/image-obmc-ast2600,if=mtd,bus=0,unit=0,snapshot=on
> -nic user -icount auto
> ```
>
> Our current hypothesis is that the problem comes up in the platform
> uboot.  As part of the boot, the secondary core waits for the smp mailbox
> to get a magic number written by the primary core:
>
>
> https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/arch/arm/mach-aspeed/ast2600/platform.S#L168
>
> However, this memory address is cleared on boot:
>
>
> https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/arch/arm/mach-aspeed/ast2600/platform.S#L146
>
> The race condition occurs if the primary core runs far ahead of the
> secondary core: if the primary core gets to the point where it signals the
> secondary core’s mailbox before the secondary core gets past the point
> where it does the initial reset and starts waiting, the reset will clear
> the signal, and then the secondary core will never get past the point where
> it’s looping in `poll_smp_mbox_ready`.
>
> We’ve observed this race happening by dumping all SCU reads and writes,
> and validated that this is the problem by using a modified `platform.S`
> that doesn’t clear the =SCU_SMP_READY mailbox on reset, but would rather
> not have to use a modified version of SMP boot just for QEMU-TCG execution.
>
> Is there a way to have QEMU insert a barrier synchronization at some point
> in the bootloader?  I think getting both cores past the =SCU_SMP_READY
> reset would get rid of this race, but I’m not aware of a way to do that
> kind of thing in QEMU-TCG.
>
> Thanks for any insights!
>
> --Stephen
>
> ---
>
> P.S. Additional note about the aspeed platform.S:
>
> Clearing the mailbox was added in this patch:
>
>
> https://github.com/AspeedTech-BMC/u-boot/commit/55825c55d1dabc00e37999a38495ed05c901bec2
>
> At the time, the write to what was then known as
> `=AST_SMP_MBOX_FIELD_READY` (now `=SCU_SMP_READY`) happened after
> `scu_unlock`.  But, when the boot flow was revised in
>
>
> https://github.com/AspeedTech-BMC/u-boot/commit/46a48bbe56c1e790c9bd1794364db86ec609c48e
> the scu_unlock was moved to primary core boot, so, unless the primary core
> wins the race, it doesn’t seem like the mailbox ready clear actually will
> have any effect, since it’ll be writing while the SCU is locked.
>

Re: [PATCH 4/5] target/i386: Rename tcg_cpu_FOO() to include 'x86'

2024-01-11 Thread Michael Tokarev


11.01.2024 15:02, Philippe Mathieu-Daudé :

The tcg_cpu_FOO() names are x86 specific, so rename
them as x86_tcg_cpu_FOO() (as other names in this file)
to ease navigating the code.


Reviewed-by: Michael Tokarev 

Wow, that's a large list of recipients..


Signed-off-by: Philippe Mathieu-Daudé 
---
  target/i386/tcg/tcg-cpu.c | 32 
  1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index 6e881e9e27..e56489caea 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -114,18 +114,18 @@ static const struct TCGCPUOps x86_tcg_ops = {
  #endif /* !CONFIG_USER_ONLY */
  };
  
-static void tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)

+static void x86_tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
  {
  /* for x86, all cpus use the same set of operations */
  cc->tcg_ops = _tcg_ops;
  }
  
-static void tcg_cpu_class_init(CPUClass *cc)

+static void x86_tcg_cpu_class_init(CPUClass *cc)
  {
-cc->init_accel_cpu = tcg_cpu_init_ops;
+cc->init_accel_cpu = x86_tcg_cpu_init_ops;
  }
  
-static void tcg_cpu_xsave_init(void)

+static void x86_tcg_cpu_xsave_init(void)
  {
  #define XO(bit, field) \
  x86_ext_save_areas[bit].offset = offsetof(X86XSaveArea, field);
@@ -147,25 +147,25 @@ static void tcg_cpu_xsave_init(void)
   * TCG-specific defaults that override cpudef models when using TCG.
   * Only for builtin_x86_defs models initialized with 
x86_register_cpudef_types.
   */
-static PropValue tcg_default_props[] = {
+static PropValue x86_tcg_default_props[] = {
  { "vme", "off" },
  { NULL, NULL },
  };
  
-static void tcg_cpu_instance_init(CPUState *cs)

+static void x86_tcg_cpu_instance_init(CPUState *cs)
  {
  X86CPU *cpu = X86_CPU(cs);
  X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
  
  if (xcc->model) {

  /* Special cases not set in the X86CPUDefinition structs: */
-x86_cpu_apply_props(cpu, tcg_default_props);
+x86_cpu_apply_props(cpu, x86_tcg_default_props);
  }
  
-tcg_cpu_xsave_init();

+x86_tcg_cpu_xsave_init();
  }
  
-static void tcg_cpu_accel_class_init(ObjectClass *oc, void *data)

+static void x86_tcg_cpu_accel_class_init(ObjectClass *oc, void *data)
  {
  AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
  
@@ -173,18 +173,18 @@ static void tcg_cpu_accel_class_init(ObjectClass *oc, void *data)

  acc->cpu_target_realize = tcg_cpu_realizefn;
  #endif /* CONFIG_USER_ONLY */
  
-acc->cpu_class_init = tcg_cpu_class_init;

-acc->cpu_instance_init = tcg_cpu_instance_init;
+acc->cpu_class_init = x86_tcg_cpu_class_init;
+acc->cpu_instance_init = x86_tcg_cpu_instance_init;
  }
-static const TypeInfo tcg_cpu_accel_type_info = {
+static const TypeInfo x86_tcg_cpu_accel_type_info = {
  .name = ACCEL_CPU_NAME("tcg"),
  
  .parent = TYPE_ACCEL_CPU,

-.class_init = tcg_cpu_accel_class_init,
+.class_init = x86_tcg_cpu_accel_class_init,
  .abstract = true,
  };
-static void tcg_cpu_accel_register_types(void)
+static void x86_tcg_cpu_accel_register_types(void)
  {
-type_register_static(_cpu_accel_type_info);
+type_register_static(_tcg_cpu_accel_type_info);
  }
-type_init(tcg_cpu_accel_register_types);
+type_init(x86_tcg_cpu_accel_register_types);

[PATCH] MAINTAINERS: Update Raphael Norwitz email

2024-01-11 Thread Raphael Norwitz

I will be leaving Nutanix so updating my email in MAINTAINERS to my
personal email for now.

Signed-off-by: Raphael Norwitz 
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 00ec1f7eca..d7bb52bfd1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2540,7 +2540,7 @@ F: include/hw/virtio/virtio-gpu.h
 F: docs/system/devices/virtio-gpu.rst
 
 vhost-user-blk
-M: Raphael Norwitz 
+M: Raphael Norwitz 
 S: Maintained
 F: contrib/vhost-user-blk/
 F: contrib/vhost-user-scsi/
-- 
2.41.0

[PATCH 0/1] smbios_build_type_8_table should use T8_BASE.

2024-01-11 Thread Felix Wu

It should use T8_BASE instead of T0_BASE.

Felix Wu (1):
  SMBIOS type 8 should use T8_BASE.

 hw/smbios/smbios.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.43.0.275.g3460e3d667-goog

[PATCH 1/1] SMBIOS type 8 should use T8_BASE.

2024-01-11 Thread Felix Wu

---
 hw/smbios/smbios.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 2a90601ac5..7dda84b284 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -591,6 +591,7 @@ bool smbios_skip_table(uint8_t type, bool required_table)
 #define T2_BASE 0x200
 #define T3_BASE 0x300
 #define T4_BASE 0x400
+#define T8_BASE 0x800
 #define T11_BASE 0xe00
 
 #define T16_BASE 0x1000
@@ -775,7 +776,7 @@ static void smbios_build_type_8_table(void)
 struct type8_instance *t8;
 
 QTAILQ_FOREACH(t8, , next) {
-SMBIOS_BUILD_TABLE_PRE(8, T0_BASE + instance, true);
+SMBIOS_BUILD_TABLE_PRE(8, T8_BASE + instance, true);
 
 SMBIOS_TABLE_SET_STR(8, internal_reference_str, 
t8->internal_reference);
 SMBIOS_TABLE_SET_STR(8, external_reference_str, 
t8->external_reference);
-- 
2.43.0.275.g3460e3d667-goog

[PATCH 4/6] vdpa: add listener_registered

Check if the listener has been registered or not, so it needs to be
registered again at start.

Signed-off-by: Eugenio Pérez 
---
 include/hw/virtio/vhost-vdpa.h | 6 ++
 hw/virtio/vhost-vdpa.c | 7 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index 8f54e5edd4..03ed2f2be3 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -45,6 +45,12 @@ typedef struct vhost_vdpa_shared {
 
 bool iotlb_batch_begin_sent;
 
+/*
+ * The memory listener has been registered, so DMA maps have been sent to
+ * the device.
+ */
+bool listener_registered;
+
 /* Vdpa must send shadow addresses as IOTLB key for data queues, not GPA */
 bool shadow_data;
 } VhostVDPAShared;
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index bd4db7ba5f..b08349d57c 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1331,7 +1331,10 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, 
bool started)
  "IOMMU and try again");
 return -1;
 }
-memory_listener_register(>shared->listener, dev->vdev->dma_as);
+if (!v->shared->listener_registered) {
+memory_listener_register(>shared->listener, dev->vdev->dma_as);
+v->shared->listener_registered = true;
+}
 
 return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
 }
@@ -1351,6 +1354,8 @@ static void vhost_vdpa_reset_status(struct vhost_dev *dev)
 vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
VIRTIO_CONFIG_S_DRIVER);
 memory_listener_unregister(>shared->listener);
+v->shared->listener_registered = false;
+
 }
 
 static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
-- 
2.39.3

[PATCH 5/6] vdpa: reorder listener assignment

Since commit f6fe3e333f ("vdpa: move memory listener to
vhost_vdpa_shared") this piece of code repeatedly assign
shared->listener members.  This was not a problem as it was not used
until device start.

However next patches move the listener registration to this
vhost_vdpa_init function.  When the listener is registered it is added
to an embedded linked list, so setting its members again will cause
memory corruption to the linked list node.

Do the right thing and only set it in the first vdpa device.

Signed-off-by: Eugenio Pérez 
---
 hw/virtio/vhost-vdpa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index b08349d57c..521a889104 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -619,7 +619,6 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void 
*opaque, Error **errp)
 
 v->dev = dev;
 dev->opaque =  opaque ;
-v->shared->listener = vhost_vdpa_memory_listener;
 
 ret = vhost_vdpa_set_backend_cap(dev);
 if (unlikely(ret != 0)) {
@@ -661,6 +660,7 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void 
*opaque, Error **errp)
 vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
VIRTIO_CONFIG_S_DRIVER);
 
+v->shared->listener = vhost_vdpa_memory_listener;
 return 0;
 }
 
-- 
2.39.3

[PATCH 6/6] vdpa: move memory listener register to vhost_vdpa_init

Current memory operations like pinning may take a lot of time at the
destination.  Currently they are done after the source of the migration is
stopped, and before the workload is resumed at the destination.  This is a
period where neigher traffic can flow, nor the VM workload can continue
(downtime).

We can do better as we know the memory layout of the guest RAM at the
destination from the moment that all devices are initializaed.  So
moving that operation allows QEMU to communicate the kernel the maps
while the workload is still running in the source, so Linux can start
mapping them.

As a small drawback, there is a time in the initialization where QEMU
cannot respond to QMP etc.  By some testing, this time is about
0.2seconds.  This may be further reduced (or increased) depending on the
vdpa driver and the platform hardware, and it is dominated by the cost
of memory pinning.

This matches the time that we move out of the called downtime window.
The downtime is measured as checking the trace timestamp from the moment
the source suspend the device to the moment the destination starts the
eight and last virtqueue pair.  For a 39G guest, it goes from ~2.2526
secs to 2.0949.

Signed-off-by: Eugenio Pérez 
---
 hw/virtio/vhost-vdpa.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 521a889104..eae8b790dd 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -660,7 +660,13 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void 
*opaque, Error **errp)
 vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
VIRTIO_CONFIG_S_DRIVER);
 
+/*
+ * Being optimistic and listening address space memory. If the device
+ * uses vIOMMU, it is changed at vhost_vdpa_dev_start.
+ */
 v->shared->listener = vhost_vdpa_memory_listener;
+memory_listener_register(>shared->listener, _space_memory);
+v->shared->listener_registered = true;
 return 0;
 }
 
@@ -1331,6 +1337,11 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, 
bool started)
  "IOMMU and try again");
 return -1;
 }
+if (v->shared->listener_registered &&
+dev->vdev->dma_as != v->shared->listener.address_space) {
+memory_listener_unregister(>shared->listener);
+v->shared->listener_registered = false;
+}
 if (!v->shared->listener_registered) {
 memory_listener_register(>shared->listener, dev->vdev->dma_as);
 v->shared->listener_registered = true;
-- 
2.39.3

[PATCH 3/6] vdpa: set backend capabilities at vhost_vdpa_init

The backend does not reset them until the vdpa file descriptor is closed
so there is no harm in doing it only once.

This allows the destination of a live migration to premap memory in
batches, using VHOST_BACKEND_F_IOTLB_BATCH.

Signed-off-by: Eugenio Pérez 
---
 hw/virtio/vhost-vdpa.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 33ae285f87..bd4db7ba5f 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -620,6 +620,12 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void 
*opaque, Error **errp)
 v->dev = dev;
 dev->opaque =  opaque ;
 v->shared->listener = vhost_vdpa_memory_listener;
+
+ret = vhost_vdpa_set_backend_cap(dev);
+if (unlikely(ret != 0)) {
+return ret;
+}
+
 vhost_vdpa_init_svq(dev, v);
 
 error_propagate(>migration_blocker, v->migration_blocker);
@@ -1506,7 +1512,6 @@ const VhostOps vdpa_ops = {
 .vhost_set_vring_kick = vhost_vdpa_set_vring_kick,
 .vhost_set_vring_call = vhost_vdpa_set_vring_call,
 .vhost_get_features = vhost_vdpa_get_features,
-.vhost_set_backend_cap = vhost_vdpa_set_backend_cap,
 .vhost_set_owner = vhost_vdpa_set_owner,
 .vhost_set_vring_endian = NULL,
 .vhost_backend_memslots_limit = vhost_vdpa_memslots_limit,
-- 
2.39.3

[PATCH 1/6] vdpa: check for iova tree initialized at net_client_start

To map the guest memory while it is migrating we need to create the
iova_tree, as long as the destination uses x-svq=on. Checking to not
override it.

The function vhost_vdpa_net_client_stop clear it if the device is
stopped. If the guest starts the device again, the iova tree is
recreated by vhost_vdpa_net_data_start_first or vhost_vdpa_net_cvq_start
if needed, so old behavior is kept.

Signed-off-by: Eugenio Pérez 
---
 net/vhost-vdpa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 3726ee5d67..e11b390466 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -341,7 +341,9 @@ static void vhost_vdpa_net_data_start_first(VhostVDPAState 
*s)
 
 migration_add_notifier(>migration_state,
vdpa_net_migration_state_notifier);
-if (v->shadow_vqs_enabled) {
+
+/* iova_tree may be initialized by vhost_vdpa_net_load_setup */
+if (v->shadow_vqs_enabled && !v->shared->iova_tree) {
 v->shared->iova_tree = vhost_iova_tree_new(v->shared->iova_range.first,
v->shared->iova_range.last);
 }
-- 
2.39.3

[PATCH 2/6] vdpa: reorder vhost_vdpa_set_backend_cap

It will be used directly by vhost_vdpa_init.

Signed-off-by: Eugenio Pérez 
---
 hw/virtio/vhost-vdpa.c | 60 +-
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index ddae494ca8..33ae285f87 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -580,6 +580,36 @@ static void vhost_vdpa_init_svq(struct vhost_dev *hdev, 
struct vhost_vdpa *v)
 v->shadow_vqs = g_steal_pointer(_vqs);
 }
 
+static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
+{
+struct vhost_vdpa *v = dev->opaque;
+
+uint64_t features;
+uint64_t f = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
+0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
+0x1ULL << VHOST_BACKEND_F_IOTLB_ASID |
+0x1ULL << VHOST_BACKEND_F_SUSPEND;
+int r;
+
+if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, )) {
+return -EFAULT;
+}
+
+features &= f;
+
+if (vhost_vdpa_first_dev(dev)) {
+r = vhost_vdpa_call(dev, VHOST_SET_BACKEND_FEATURES, );
+if (r) {
+return -EFAULT;
+}
+}
+
+dev->backend_cap = features;
+v->shared->backend_cap = features;
+
+return 0;
+}
+
 static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
 {
 struct vhost_vdpa *v = opaque;
@@ -827,36 +857,6 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
 return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
 }
 
-static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
-{
-struct vhost_vdpa *v = dev->opaque;
-
-uint64_t features;
-uint64_t f = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
-0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
-0x1ULL << VHOST_BACKEND_F_IOTLB_ASID |
-0x1ULL << VHOST_BACKEND_F_SUSPEND;
-int r;
-
-if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, )) {
-return -EFAULT;
-}
-
-features &= f;
-
-if (vhost_vdpa_first_dev(dev)) {
-r = vhost_vdpa_call(dev, VHOST_SET_BACKEND_FEATURES, );
-if (r) {
-return -EFAULT;
-}
-}
-
-dev->backend_cap = features;
-v->shared->backend_cap = features;
-
-return 0;
-}
-
 static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
 uint32_t *device_id)
 {
-- 
2.39.3

[PATCH 0/6] Move memory listener register to vhost_vdpa_init

Current memory operations like pinning may take a lot of time at the
destination. Currently they are done after the source of the migration is
stopped, and before the workload is resumed at the destination. This is a
period where neigher traffic can flow, nor the VM workload can continue
(downtime).

We can do better as we know the memory layout of the guest RAM at the
destination from the moment that all devices are initializaed. So
moving that operation allows QEMU to communicate the kernel the maps
while the workload is still running in the source, so Linux can start
mapping them.

As a small drawback, there is a time in the initialization where QEMU
cannot respond to QMP etc. By some testing, this time is about
0.2seconds. This may be further reduced (or increased) depending on the
vdpa driver and the platform hardware, and it is dominated by the cost
of memory pinning.

This matches the time that we move out of the called downtime window.
The downtime is measured as checking the trace timestamp from the moment
the source suspend the device to the moment the destination starts the
eight and last virtqueue pair. For a 39G guest, it goes from ~2.2526
secs to 2.0949.

Future directions on top of this series may include to move more things ahead
of the migration time, like set DRIVER_OK or perform actual iterative migration
of virtio-net devices.

Comments are welcome.

This series is a different approach of series [1]. As the title does not
reflect the changes anymore, please refer to the previous one to know the
series history.

[1]
https://patchwork.kernel.org/project/qemu-devel/cover/20231215172830.2540987-1-epere...@redhat.com/

Eugenio Pérez (6):
vdpa: check for iova tree initialized at net_client_start
vdpa: reorder vhost_vdpa_set_backend_cap
vdpa: set backend capabilities at vhost_vdpa_init
vdpa: add listener_registered
vdpa: reorder listener assignment
vdpa: move memory listener register to vhost_vdpa_init

include/hw/virtio/vhost-vdpa.h | 6 +++
hw/virtio/vhost-vdpa.c | 87 +-
net/vhost-vdpa.c | 4 +-
3 files changed, 63 insertions(+), 34 deletions(-)

--
2.39.3

Re: Goldfish TTY enhancement

2024-01-11 Thread Jason Thorpe

> On Jan 10, 2024, at 8:01 AM, Philippe Mathieu-Daudé  wrote:
> 
> IIUC Goldfish virtual HW is maintained externally by Google
> https://android.googlesource.com/platform/external/qemu/+/master/docs/GOLDFISH-VIRTUAL-HARDWARE.TXT
> 
> I suppose the spec needs to be updated before the change can be
> accepted in mainstream QEMU, but since I'm not sure I Cc'ed Alex,
> David and Laurent.

Hey Philippe,

I have seen that document didn’t realize that it was the source of truth for 
the Goldfish devices in Qemu, as Qemu already has Goldfish devices that deviate 
in behavior from that document.  In particular:

1. There is no distinction between “rtc” and “timer” in Qemu.

2. The Goldfish “pic” device does not behave as that document describes.  In 
particular, the “NUMBER” register is described in that document as returning 
the lowest pending interrupt index or 0 for none (i.e. a number in the range 
0..32).  But Qemu returns a bitmask of pending interrupts when that register is 
read.  And despite the name “DISABLE_ALL” that document claims that writing to 
it merely clears the pending interrupts without disabling them (which would be 
quite the trick with level-triggered interrupt sources) whereas in Qemu, it 
does both clear and disable.

(I am not, in any way, advocating for a behavior change in Qemu, BTW… I just 
thought that referenced docuemnt was no longer relevant.)

-- thorpej

Re: [RFC PATCH v3 05/30] migration/qemu-file: add utility methods for working with seekable channels

2024-01-11 Thread Fabiano Rosas

Peter Xu  writes:

> On Mon, Nov 27, 2023 at 05:25:47PM -0300, Fabiano Rosas wrote:
>> From: Nikolay Borisov 
>> 
>> Add utility methods that will be needed when implementing 'fixed-ram'
>> migration capability.
>> 
>> qemu_file_is_seekable
>> qemu_put_buffer_at
>> qemu_get_buffer_at
>> qemu_set_offset
>> qemu_get_offset
>> 
>> Signed-off-by: Nikolay Borisov 
>> Signed-off-by: Fabiano Rosas 
>> Reviewed-by: Daniel P. Berrangé 
>> ---
>>  include/migration/qemu-file-types.h |  2 +
>>  migration/qemu-file.c   | 82 +
>>  migration/qemu-file.h   |  6 +++
>>  3 files changed, 90 insertions(+)
>> 
>> diff --git a/include/migration/qemu-file-types.h 
>> b/include/migration/qemu-file-types.h
>> index 9ba163f333..adec5abc07 100644
>> --- a/include/migration/qemu-file-types.h
>> +++ b/include/migration/qemu-file-types.h
>> @@ -50,6 +50,8 @@ unsigned int qemu_get_be16(QEMUFile *f);
>>  unsigned int qemu_get_be32(QEMUFile *f);
>>  uint64_t qemu_get_be64(QEMUFile *f);
>>  
>> +bool qemu_file_is_seekable(QEMUFile *f);
>> +
>>  static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
>>  {
>>  qemu_put_be64(f, *pv);
>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> index 94231ff295..faf6427b91 100644
>> --- a/migration/qemu-file.c
>> +++ b/migration/qemu-file.c
>> @@ -33,6 +33,7 @@
>>  #include "options.h"
>>  #include "qapi/error.h"
>>  #include "rdma.h"
>> +#include "io/channel-file.h"
>>  
>>  #define IO_BUF_SIZE 32768
>>  #define MAX_IOV_SIZE MIN_CONST(IOV_MAX, 64)
>> @@ -255,6 +256,10 @@ static void qemu_iovec_release_ram(QEMUFile *f)
>>  memset(f->may_free, 0, sizeof(f->may_free));
>>  }
>>  
>> +bool qemu_file_is_seekable(QEMUFile *f)
>> +{
>> +return qio_channel_has_feature(f->ioc, QIO_CHANNEL_FEATURE_SEEKABLE);
>> +}
>>  
>>  /**
>>   * Flushes QEMUFile buffer
>> @@ -447,6 +452,83 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, 
>> size_t size)
>>  }
>>  }
>>  
>> +void qemu_put_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
>> +off_t pos)
>> +{
>> +Error *err = NULL;
>> +
>> +if (f->last_error) {
>> +return;
>> +}
>> +
>> +qemu_fflush(f);
>> +qio_channel_pwrite(f->ioc, (char *)buf, buflen, pos, );
>
> Partial writes won't set err.  Do we want to check the retval here too and
> fail properly if detected partial writes?
>

Yep.

>> +
>> +if (err) {
>> +qemu_file_set_error_obj(f, -EIO, err);
>> +} else {
>> +stat64_add(_stats.qemu_file_transferred, buflen);
>
> buflen is only accurate if with above, iiuc.
>
>> +}
>> +
>> +return;
>> +}
>> +
>> +
>> +size_t qemu_get_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
>> +  off_t pos)
>> +{
>> +Error *err = NULL;
>> +ssize_t ret;
>> +
>> +if (f->last_error) {
>> +return 0;
>> +}
>> +
>> +ret = qio_channel_pread(f->ioc, (char *)buf, buflen, pos, );
>
> Same question here.
>
>> +if (ret == -1 || err) {
>> +goto error;
>> +}
>> +
>> +return (size_t)ret;
>> +
>> + error:
>> +qemu_file_set_error_obj(f, -EIO, err);
>> +return 0;
>> +}
>> +
>> +void qemu_set_offset(QEMUFile *f, off_t off, int whence)
>> +{
>> +Error *err = NULL;
>> +off_t ret;
>> +
>> +qemu_fflush(f);
>> +
>> +if (!qemu_file_is_writable(f)) {
>> +f->buf_index = 0;
>> +f->buf_size = 0;
>> +}
>
> There's the qemu_file_is_writable() check after all, then put qemu_fflush()
> into condition too?
>
>   if (qemu_file_is_writable(f)) {
>   qemu_fflush(f);
>   } else {
>  /* Drop all the cached buffers if existed; will trigger a re-fill later 
> */
>  f->buf_index = 0;
>  f->buf_size = 0;
>   }
>

Could be. I'll change it.

>> +
>> +ret = qio_channel_io_seek(f->ioc, off, whence, );
>> +if (ret == (off_t)-1) {
>> +qemu_file_set_error_obj(f, -EIO, err);
>> +}
>> +}
>> +
>> +off_t qemu_get_offset(QEMUFile *f)
>> +{
>> +Error *err = NULL;
>> +off_t ret;
>> +
>> +qemu_fflush(f);
>> +
>> +ret = qio_channel_io_seek(f->ioc, 0, SEEK_CUR, );
>> +if (ret == (off_t)-1) {
>> +qemu_file_set_error_obj(f, -EIO, err);
>> +}
>> +return ret;
>> +}
>> +
>> +
>>  void qemu_put_byte(QEMUFile *f, int v)
>>  {
>>  if (f->last_error) {
>> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
>> index 8aec9fabf7..32fd4a34fd 100644
>> --- a/migration/qemu-file.h
>> +++ b/migration/qemu-file.h
>> @@ -75,6 +75,12 @@ QEMUFile *qemu_file_get_return_path(QEMUFile *f);
>>  int qemu_fflush(QEMUFile *f);
>>  void qemu_file_set_blocking(QEMUFile *f, bool block);
>>  int qemu_file_get_to_fd(QEMUFile *f, int fd, size_t size);
>> +void qemu_set_offset(QEMUFile *f, off_t off, int whence);
>> +off_t qemu_get_offset(QEMUFile *f);
>> +void qemu_put_buffer_at(QEMUFile *f, const uint8_t *buf, size_t buflen,
>> +off_t pos);
>> +size_t

Re: [RFC PATCH v3 04/30] io: fsync before closing a file channel

2024-01-11 Thread Fabiano Rosas

Peter Xu  writes:

> On Mon, Nov 27, 2023 at 05:25:46PM -0300, Fabiano Rosas wrote:
>> Make sure the data is flushed to disk before closing file
>> channels. This will ensure data is on disk at the end of a migration
>> to file.
>
> Looks reasonable, but just two (possibly naive) questions:
>
> (1) Does this apply to all io channel users, or only migration?

All file channel users.

>
> (2) Why metadata doesn't matter (v.s. fsync(), when CONFIG_FDATASYNC=y)?

Syncing the inode information is not critical, it's mostly timestamp
information (man inode). And fdatasync makes sure to sync any metadata
that would be relevant for the retrieval of the data.

>
> Thanks,

Re: [RFC PATCH v3 00/30] migration: File based migration with multifd and fixed-ram

2024-01-11 Thread Fabiano Rosas

Peter Xu  writes:

> On Mon, Nov 27, 2023 at 05:25:42PM -0300, Fabiano Rosas wrote:
>> Hi,
>> 
>> In this v3:
>> 
>> Added support for the "file:/dev/fdset/" syntax to receive multiple
>> file descriptors. This allows the management layer to open the
>> migration file beforehand and pass the file descriptors to QEMU. We
>> need more than one fd to be able to use O_DIRECT concurrently with
>> unaligned writes.
>> 
>> Dropped the auto-pause capability. That discussion was kind of
>> stuck. We can revisit optimizations for non-live scenarios once the
>> series is more mature/merged.
>> 
>> Changed the multifd incoming side to use a more generic data structure
>> instead of MultiFDPages_t. This allows multifd to restore the ram
>> using larger chunks.
>> 
>> The rest are minor changes, I have noted them in the patches
>> themselves.
>
> Fabiano,
>
> Could you always keep a section around in the cover letter (and also in the
> upcoming doc file fixed-ram.rst) on the benefits of this feature?
>
> Please bare with me - I can start to ask silly questions.
>

That's fine. Ask away!

> I thought it was about "keeping the snapshot file small".  But then when I
> was thinking the use case, iiuc fixed-ram migration should always suggest
> the user to stop the VM first before migration starts, then if the VM is
> stopped the ultimate image shouldn't be large either.
>
> Or is it about performance only?  Where did I miss?

Performance is the main benefit because fixed-ram enables the use of
multifd for file migration which would otherwise not be
parallelizable. To use multifd has been the direction for a while as you
know, so it makes sense.

A fast file migration is desirable because it could be used for
snapshots with a stopped vm and also to replace the "exec:cat" hack
(this last one I found out about recently, Juan mentioned it in this
thread: https://lore.kernel.org/r/87cyx5ty26.fsf@secure.mitica).

The size aspect is just an interesting property, not necessarily a
reason. It's about having the file bounded to the RAM size. So a running
guest would not produce a continuously growing file. This is in contrast
with previous experiments (libvirt code) in using a proxy to put
multifd-produced data into a file.

I'll add this^ information in a more organized matter to the docs and
cover letter. Let me know what else I need to clarify.

Some notes about fixed-ram by itself:

This series also enables fixed-ram without multifd, which would only
take benefit of the size property. That is not part of our end goal
which is to have multifd + fixed-ram, but I kept it nonetheless because
it helps to debug/reason about the fixed-ram format without conflating
matters with multifd.

Fixed-ram without multifd also allows the file migration to take benefit
of direct io because the data portion of the file (pages) will be
written with alignment. This version of the series does not yet support
it, but I have a simple patch for the next version.

I also had a - perhaps naive - idea that we could merge the io code +
fixed-ram first, to expedite things and later bring in the multifd and
directio enhancements, but the review process ended up not being that
modular.

Re: [PATCH 3/5] hw/s390x: Rename cpu_class_init() to include 'sclp'


On 11/01/2024 13.02, Philippe Mathieu-Daudé wrote:

cpu_class_init() is specific to s390x SCLP, so rename
it as sclp_cpu_class_init() (as other names in this file)
to ease navigating the code.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/s390x/sclpcpu.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/sclpcpu.c b/hw/s390x/sclpcpu.c
index f2b1a4b037..fa79891f5a 100644
--- a/hw/s390x/sclpcpu.c
+++ b/hw/s390x/sclpcpu.c
@@ -73,7 +73,7 @@ static int read_event_data(SCLPEvent *event, 
EventBufferHeader *evt_buf_hdr,
  return 1;
  }
  
-static void cpu_class_init(ObjectClass *oc, void *data)

+static void sclp_cpu_class_init(ObjectClass *oc, void *data)
  {
  SCLPEventClass *k = SCLP_EVENT_CLASS(oc);
  DeviceClass *dc = DEVICE_CLASS(oc);
@@ -94,7 +94,7 @@ static const TypeInfo sclp_cpu_info = {
  .name  = TYPE_SCLP_CPU_HOTPLUG,
  .parent= TYPE_SCLP_EVENT,
  .instance_size = sizeof(SCLPEvent),
-.class_init= cpu_class_init,
+.class_init= sclp_cpu_class_init,
  .class_size= sizeof(SCLPEventClass),
  };
  


Reviewed-by: Thomas Huth

Possible race condition in aspeed ast2600 smp boot on TCG QEMU

2024-01-11 Thread Stephen Longfield

We’ve noticed inconsistent behavior when running a large number of aspeed
ast2600 executions, that seems to be tied to a race condition in the smp
boot when executing on TCG-QEMU, and were wondering what a good mediation
strategy might be.

The problem first shows up as part of SMP boot. On a run that’s likely to
later run into issues, we’ll see something like:

```
[0.008350] smp: Bringing up secondary CPUs ...
[1.168584] CPU1: failed to come online
[1.187277] smp: Brought up 1 node, 1 CPU
```

Compared to the more likely to succeed:

```
[0.080313] smp: Bringing up secondary CPUs ...
[0.093166] smp: Brought up 1 node, 2 CPUs
[0.093345] SMP: Total of 2 processors activated (4800.00 BogoMIPS).
```

It’s somewhat reliably reproducible by running the ast2600-evb with an
OpenBMC image, using ‘-icount auto’ to slow execution and make the race
condition more frequent (it happens without this, just easier to debug if
we can reproduce):


```
./aarch64-softmmu/qemu-system-aarch64 -machine ast2600-evb -nographic
-drive file=~/bmc-bin/image-obmc-ast2600,if=mtd,bus=0,unit=0,snapshot=on
-nic user -icount auto
```

Our current hypothesis is that the problem comes up in the platform uboot.
As part of the boot, the secondary core waits for the smp mailbox to get a
magic number written by the primary core:

https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/arch/arm/mach-aspeed/ast2600/platform.S#L168

However, this memory address is cleared on boot:

https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/arch/arm/mach-aspeed/ast2600/platform.S#L146

The race condition occurs if the primary core runs far ahead of the
secondary core: if the primary core gets to the point where it signals the
secondary core’s mailbox before the secondary core gets past the point
where it does the initial reset and starts waiting, the reset will clear
the signal, and then the secondary core will never get past the point where
it’s looping in `poll_smp_mbox_ready`.

We’ve observed this race happening by dumping all SCU reads and writes, and
validated that this is the problem by using a modified `platform.S` that
doesn’t clear the =SCU_SMP_READY mailbox on reset, but would rather not
have to use a modified version of SMP boot just for QEMU-TCG execution.

Is there a way to have QEMU insert a barrier synchronization at some point
in the bootloader?  I think getting both cores past the =SCU_SMP_READY
reset would get rid of this race, but I’m not aware of a way to do that
kind of thing in QEMU-TCG.

Thanks for any insights!

--Stephen

---

P.S. Additional note about the aspeed platform.S:

Clearing the mailbox was added in this patch:

https://github.com/AspeedTech-BMC/u-boot/commit/55825c55d1dabc00e37999a38495ed05c901bec2

At the time, the write to what was then known as
`=AST_SMP_MBOX_FIELD_READY` (now `=SCU_SMP_READY`) happened after
`scu_unlock`.  But, when the boot flow was revised in

https://github.com/AspeedTech-BMC/u-boot/commit/46a48bbe56c1e790c9bd1794364db86ec609c48e
the scu_unlock was moved to primary core boot, so, unless the primary core
wins the race, it doesn’t seem like the mailbox ready clear actually will
have any effect, since it’ll be writing while the SCU is locked.

[PATCH v3] qemu-img: Fix Column Width and Improve Formatting in snapshot list

2024-01-11 Thread Abhiram Tilak

When running the command `qemu-img snapshot -l SNAPSHOT` the output of
VM_CLOCK (measures the offset between host and VM clock) cannot to
accommodate values in the order of thousands (4-digit).

This line [1] hints on the problem. Additionally, the column width for
the VM_CLOCK field was reduced from 15 to 13 spaces in commit b39847a5
in line [2], resulting in a shortage of space.

[1]: 
https://gitlab.com/qemu-project/qemu/-/blob/master/block/qapi.c?ref_type=heads#L753
[2]: 
https://gitlab.com/qemu-project/qemu/-/blob/master/block/qapi.c?ref_type=heads#L763

This patch restores the column width to 15 spaces and makes adjustments
to the affected iotests accordingly. Furthermore, addresses a potential source
of confusion by removing whitespace in column headers. Example, VM CLOCK
is modified to VM_CLOCK. Additionally a '--' symbol is introduced when
ICOUNT returns no output for clarity.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2062
Fixes: b39847a50553 (migration: introduce icount field for snapshots )
Signed-off-by: Abhiram Tilak 
---
 v3:
* Make a patch by avoid changing the .patch file
 v2:
* Change email provider to 'gmail' to avoid auto-wrapping patches
* Modify iotests for file 'qcow2-internal-snapshots.out'

 block/qapi.c  | 10 ++--
 tests/qemu-iotests/267.out| 48 +--
 .../tests/qcow2-internal-snapshots.out| 10 ++--
 3 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 9e806fa230..ee066ee53c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -742,15 +742,15 @@ void bdrv_snapshot_dump(QEMUSnapshotInfo *sn)
 char *sizing = NULL;
 
 if (!sn) {
-qemu_printf("%-10s%-17s%8s%20s%13s%11s",
-"ID", "TAG", "VM SIZE", "DATE", "VM CLOCK", "ICOUNT");
+qemu_printf("%-10s%-17s%8s%20s%15s%11s",
+"ID", "TAG", "VM_SIZE", "DATE", "VM_CLOCK", "ICOUNT");
 } else {
 g_autoptr(GDateTime) date = 
g_date_time_new_from_unix_local(sn->date_sec);
 g_autofree char *date_buf = g_date_time_format(date, "%Y-%m-%d 
%H:%M:%S");
 
 secs = sn->vm_clock_nsec / 10;
 snprintf(clock_buf, sizeof(clock_buf),
- "%02d:%02d:%02d.%03d",
+ "%04d:%02d:%02d.%03d",
  (int)(secs / 3600),
  (int)((secs / 60) % 60),
  (int)(secs % 60),
@@ -759,8 +759,10 @@ void bdrv_snapshot_dump(QEMUSnapshotInfo *sn)
 if (sn->icount != -1ULL) {
 snprintf(icount_buf, sizeof(icount_buf),
 "%"PRId64, sn->icount);
+} else {
+snprintf(icount_buf, sizeof(icount_buf), "--");
 }
-qemu_printf("%-9s %-16s %8s%20s%13s%11s",
+qemu_printf("%-9s %-16s %8s%20s%15s%11s",
 sn->id_str, sn->name,
 sizing,
 date_buf,
diff --git a/tests/qemu-iotests/267.out b/tests/qemu-iotests/267.out
index 7176e376e1..21339e67ad 100644
--- a/tests/qemu-iotests/267.out
+++ b/tests/qemu-iotests/267.out
@@ -33,8 +33,8 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm snap0
 (qemu) info snapshots
 List of snapshots present on all disks:
-IDTAG   VM SIZEDATE VM CLOCK ICOUNT
---snap0SIZE -mm-dd hh:mm:ss 00:00:00.000   
+IDTAG   VM_SIZEDATE   VM_CLOCK 
ICOUNT
+--snap0SIZE -mm-dd hh:mm:ss :00:00.000 
--
 (qemu) loadvm snap0
 (qemu) quit
 
@@ -44,8 +44,8 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm snap0
 (qemu) info snapshots
 List of snapshots present on all disks:
-IDTAG   VM SIZEDATE VM CLOCK ICOUNT
---snap0SIZE -mm-dd hh:mm:ss 00:00:00.000   
+IDTAG   VM_SIZEDATE   VM_CLOCK 
ICOUNT
+--snap0SIZE -mm-dd hh:mm:ss :00:00.000 
--
 (qemu) loadvm snap0
 (qemu) quit
 
@@ -69,8 +69,8 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm snap0
 (qemu) info snapshots
 List of snapshots present on all disks:
-IDTAG   VM SIZEDATE VM CLOCK ICOUNT
---snap0SIZE -mm-dd hh:mm:ss 00:00:00.000   
+IDTAG   VM_SIZEDATE   VM_CLOCK 
ICOUNT
+--snap0SIZE -mm-dd hh:mm:ss :00:00.000 
--
 (qemu) loadvm snap0
 (qemu) quit
 
@@ -94,8 +94,8 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm snap0
 (qemu) info snapshots
 List of snapshots present on all disks:
-IDTAG   VM SIZEDATE VM CLOCK ICOUNT
---snap0SIZE -mm-dd hh:mm:ss

Re: [PATCH 2/2] docs/about: Deprecate the old "power5+" and "power7+" CPU names

2024-01-11 Thread Cédric Le Goater


On 1/11/24 17:46, Thomas Huth wrote:

For consistency we should drop the names with a "+" in it in the
long run.

Signed-off-by: Thomas Huth 
---
  docs/about/deprecated.rst | 9 +
  1 file changed, 9 insertions(+)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 2e15040246..7fdd2239b4 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -245,6 +245,15 @@ Nios II CPU (since 8.2)
  The Nios II architecture is orphan. The ``nios2`` guest CPU support is
  deprecated and will be removed in a future version of QEMU.
  
+``power5+`` and ``power7+`` CPU names (since 9.0)

+'
+
+The character "+" in device (and thus also CPU) names is not allowed
+in the QEMU object model anymore. ``power5+``, ``power5+_v2.1``,
+``power7+`` and ``power7+_v2.1`` are currently still supported via
+an alias, but for consistency these will get removed in a future
+release, too. Use ``power5plus_v2.1`` and ``power7plus_v2.1`` instead.
+
  
  System emulator machines

  



Reviewed-by: Cédric Le Goater 

Thanks,

C.

Re: [PATCH 1/2] target/ppc/cpu-models: Rename power5+ and power7+ for new QOM naming rules

2024-01-11 Thread Cédric Le Goater


On 1/11/24 17:46, Thomas Huth wrote:

The character "+" is now forbidden in QOM device names (see commit
b447378e1217 - "Limit type names to alphanumerical and some few special
characters"). For the "power5+" and "power7+" CPU names, there is
currently a hack in type_name_is_valid() to still allow them for
compatibility reasons. However, there is a much nicer solution for this:
Simply use aliases! This way we can still support the old names without
the need for the ugly hack in type_name_is_valid().

Signed-off-by: Thomas Huth 



Reviewed-by: Cédric Le Goater 

Thanks,

C.

Re: [PATCH 03/12] tests/plugin: add test plugin for inline operations

2024-01-11 Thread Pierrick Bouvier


On 1/11/24 19:57, Philippe Mathieu-Daudé wrote:

Hi Pierrick,

On 11/1/24 15:23, Pierrick Bouvier wrote:

For now, it simply performs instruction, bb and mem count, and ensure
that inline vs callback versions have the same result. Later, we'll
extend it when new inline operations are added.

Use existing plugins to test everything works is a bit cumbersome, as
different events are treated in different plugins. Thus, this new one.

Signed-off-by: Pierrick Bouvier 
---
   tests/plugin/inline.c| 183 +++
   tests/plugin/meson.build |   2 +-
   2 files changed, 184 insertions(+), 1 deletion(-)
   create mode 100644 tests/plugin/inline.c



+#define MAX_CPUS 8


Where does this value come from?



The plugin tests/plugin/insn.c had this constant, so I picked it up from 
here.



Should the pluggin API provide a helper to ask TCG how many
vCPUs are created?


In user mode, we can't know how many simultaneous threads (and thus 
vcpu) will be triggered by advance. I'm not sure if additional cpus can 
be added in system mode.


One problem though, is that when you register an inline op with a 
dynamic array, when you resize it (when detecting a new vcpu), you can't 
change it afterwards. So, you need a storage statically sized somewhere.


Your question is good, and maybe we should define a MAX constant that 
plugins should rely on, instead of a random amount.

Re: [PULL 13/13] tests/avocado: remove skips from replay_kernel


On 08/01/2024 16.13, Alex Bennée wrote:

With the latest fixes for #2010 and #2013 these tests look pretty
stable now. Of course the only way to be really sure is to run it in
the CI infrastructure and see what breaks.

Acked-by: Pavel Dovgalyuk 
Signed-off-by: Alex Bennée 
Message-Id: <20231211091346.14616-14-alex.ben...@linaro.org>


The replay tests seem still to be very flaky, I'm now getting:

 https://gitlab.com/thuth/qemu/-/jobs/5910241580#L227
 https://gitlab.com/thuth/qemu/-/jobs/5910241593#L396

I'd suggest to revert this patch to disable them in the CI again.

 Thomas

[PULL 17/17] .gitlab-ci.d/buildtest.yml: Work around htags bug when environment is large

From: Peter Maydell 

Sometimes the CI "pages" job fails with a message like this from
htags:

$ htags -anT --tree-view=filetree -m qemu_init -t "Welcome to the QEMU 
sourcecode"
htags: Negative exec line limit = -371

This is due to a bug in hflags where if the environment is too large it
falls over:
https://lists.gnu.org/archive/html/bug-global/2024-01/msg0.html

This happens to us because GitLab CI puts the commit message of the
commit under test into the CI_COMMIT_MESSAGE and/or CI_COMMIT_TAG_MESSAGE
environment variables, so the job will fail if the commit happens to
have a verbose commit message.

Work around the htags bug by unsetting these variables while running
htags.

Cc: qemu-sta...@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2080
Reviewed-by: Philippe Mathieu-Daudé 
Message-ID: <2024025543.1573473-1-peter.mayd...@linaro.org>
Signed-off-by: Thomas Huth 
---
 .gitlab-ci.d/buildtest.yml | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index cfe737aca2..9b4df24e9a 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -647,7 +647,10 @@ pages:
 - mkdir -p public
 # HTML-ised source tree
 - make gtags
-- htags -anT --tree-view=filetree -m qemu_init
+# We unset variables to work around a bug in some htags versions
+# which causes it to fail when the environment is large
+- CI_COMMIT_MESSAGE= CI_COMMIT_TAG_MESSAGE= htags
+-anT --tree-view=filetree -m qemu_init
 -t "Welcome to the QEMU sourcecode"
 - mv HTML public/src
 # Project documentation
-- 
2.43.0

[PULL 12/17] hw/s390x/ccw: Replace basename() with g_path_get_basename()

From: Zhao Liu 

g_path_get_basename() is a portable utility function that has the
advantage of not modifying the string argument, so it should be
preferred over basename().

And also to avoid potential compile breakage with the Musl C library
similar to [1], replace basename() with g_path_get_basename().

[1]: https://lore.kernel.org/all/20231212010228.2701544-1-raj.k...@gmail.com/

Suggested-by: Cédric Le Goater 
Signed-off-by: Zhao Liu 
Message-ID: <20231221171921.57784-2-zhao1@linux.intel.com>
Reviewed-by: Cédric Le Goater 
Reviewed-by: Eric Farman 
Signed-off-by: Thomas Huth 
---
 hw/s390x/s390-ccw.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index e2d86d96e7..ab7022a3ab 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -76,7 +76,8 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
   Error **errp)
 {
 unsigned int cssid, ssid, devid;
-char dev_path[PATH_MAX] = {0}, *tmp;
+char dev_path[PATH_MAX] = {0};
+g_autofree char *tmp = NULL;
 
 if (!sysfsdev) {
 error_setg(errp, "No host device provided");
@@ -92,7 +93,7 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
 
 cdev->mdevid = g_path_get_basename(dev_path);
 
-tmp = basename(dirname(dev_path));
+tmp = g_path_get_basename(dirname(dev_path));
 if (sscanf(tmp, "%2x.%1x.%4x", , , ) != 3) {
 error_setg_errno(errp, errno, "Failed to read %s", tmp);
 return;
-- 
2.43.0

[PULL 13/17] hw/s390x/ccw: Replace dirname() with g_path_get_dirname()

From: Zhao Liu 

As commit 3e015d815b3f ("use g_path_get_basename instead of basename")
said, g_path_get_dirname() should be preferred over dirname() since
the former is a portable utility function that has the advantage of not
modifying the string argument.

Replace dirname() with g_path_get_dirname().

Suggested-by: Cédric Le Goater 
Signed-off-by: Zhao Liu 
Message-ID: <20231221171921.57784-3-zhao1@linux.intel.com>
Reviewed-by: Cédric Le Goater 
Reviewed-by: Eric Farman 
Signed-off-by: Thomas Huth 
---
 hw/s390x/s390-ccw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index ab7022a3ab..5261e66724 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -77,6 +77,7 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
 {
 unsigned int cssid, ssid, devid;
 char dev_path[PATH_MAX] = {0};
+g_autofree char *tmp_dir = NULL;
 g_autofree char *tmp = NULL;
 
 if (!sysfsdev) {
@@ -93,7 +94,8 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
 
 cdev->mdevid = g_path_get_basename(dev_path);
 
-tmp = g_path_get_basename(dirname(dev_path));
+tmp_dir = g_path_get_dirname(dev_path);
+tmp = g_path_get_basename(tmp_dir);
 if (sscanf(tmp, "%2x.%1x.%4x", , , ) != 3) {
 error_setg_errno(errp, errno, "Failed to read %s", tmp);
 return;
-- 
2.43.0

[PULL 03/17] Revert "netdev: set timeout depending on loadavg"

From: Daniel P. Berrangé 

This reverts commit cadfc7293977ecadc2d6c48d7cffc553ed2f85f1.

The test was not timing out because of slow execution. It was
timing out due to a race condition leading to the client QEMU
attempting (and fatally failing) to connect before the server
QEMU was listening.

Signed-off-by: "Daniel P. Berrangé" 
Message-ID: <20240104162942.211458-2-berra...@redhat.com>
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Thomas Huth 
---
 tests/qtest/netdev-socket.c | 28 +---
 1 file changed, 1 insertion(+), 27 deletions(-)

diff --git a/tests/qtest/netdev-socket.c b/tests/qtest/netdev-socket.c
index bb99d08b5e..7ba1eff120 100644
--- a/tests/qtest/netdev-socket.c
+++ b/tests/qtest/netdev-socket.c
@@ -18,32 +18,6 @@
 
 #define CONNECTION_TIMEOUT120
 
-static double connection_timeout(void)
-{
-double load;
-int ret = getloadavg(, 1);
-
-/*
- * If we can't get load data, or load is low because we just started
- * running, assume load of 1 (we are alone in this system).
- */
-if (ret < 1 || load < 1.0) {
-load = 1.0;
-}
-/*
- * No one wants to wait more than 10 minutes for this test. Higher load?
- * Too bad.
- */
-if (load > 10.0) {
-fprintf(stderr, "Warning: load %f higher than 10 - test might 
timeout\n",
-load);
-load = 10.0;
-}
-
-/* if load is high increase timeout as we might not get a chance to run */
-return load * CONNECTION_TIMEOUT;
-}
-
 #define EXPECT_STATE(q, e, t) \
 do {  \
 char *resp = NULL;\
@@ -57,7 +31,7 @@ do {  \
 if (g_str_equal(resp, e)) {   \
 break;\
 } \
-} while (g_test_timer_elapsed() < connection_timeout()); \
+} while (g_test_timer_elapsed() < CONNECTION_TIMEOUT); \
 g_assert_cmpstr(resp, ==, e); \
 g_free(resp); \
 } while (0)
-- 
2.43.0

[PULL 09/17] tests/qtest/virtio-ccw: Fix device presence checking

From: Samuel Tardieu 

An apparent copy-paste error tests for the presence of the
virtio-rng-ccw device in order to perform tests on the virtio-scsi-ccw
device.

Signed-off-by: Samuel Tardieu 
Message-ID: <20240106130121.1244993-1-...@rfc1149.net>
Fixes: 65331bf5d1 ("tests/qtest: Check for virtio-ccw devices before  using 
them")
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Thomas Huth 
---
 tests/qtest/virtio-ccw-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/virtio-ccw-test.c b/tests/qtest/virtio-ccw-test.c
index f4f5858b84..7a5357c212 100644
--- a/tests/qtest/virtio-ccw-test.c
+++ b/tests/qtest/virtio-ccw-test.c
@@ -85,7 +85,7 @@ int main(int argc, char **argv)
 if (qtest_has_device("virtio-rng-ccw")) {
 qtest_add_func("/virtio/rng/nop", virtio_rng_nop);
 }
-if (qtest_has_device("virtio-rng-ccw")) {
+if (qtest_has_device("virtio-scsi-ccw")) {
 qtest_add_func("/virtio/scsi/nop", virtio_scsi_nop);
 qtest_add_func("/virtio/scsi/hotplug", virtio_scsi_hotplug);
 }
-- 
2.43.0

[PULL 15/17] target/s390x: Fix LAE setting a wrong access register

From: Ilya Leoshkevich 

LAE should set the access register corresponding to the first operand,
instead, it always modifies access register 1.

Co-developed-by: Ido Plat 
Cc: qemu-sta...@nongnu.org
Fixes: a1c7610a6879 ("target-s390x: implement LAY and LAEY instructions")
Reviewed-by: David Hildenbrand 
Signed-off-by: Ilya Leoshkevich 
Message-ID: <20240111092328.929421-2-...@linux.ibm.com>
Signed-off-by: Thomas Huth 
---
 target/s390x/tcg/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 62ab2be8b1..8df00b7df9 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -3221,6 +3221,7 @@ static DisasJumpType op_mov2e(DisasContext *s, DisasOps 
*o)
 {
 int b2 = get_field(s, b2);
 TCGv ar1 = tcg_temp_new_i64();
+int r1 = get_field(s, r1);
 
 o->out = o->in2;
 o->in2 = NULL;
@@ -3244,7 +3245,7 @@ static DisasJumpType op_mov2e(DisasContext *s, DisasOps 
*o)
 break;
 }
 
-tcg_gen_st32_i64(ar1, tcg_env, offsetof(CPUS390XState, aregs[1]));
+tcg_gen_st32_i64(ar1, tcg_env, offsetof(CPUS390XState, aregs[r1]));
 return DISAS_NEXT;
 }
 
-- 
2.43.0

[PULL 04/17] Revert "osdep: add getloadavg"

From: Daniel P. Berrangé 

This reverts commit dc864d3a3777424187280e50c9bfb84dced54f12.

This functionality is not required after the previous revert

Signed-off-by: "Daniel P. Berrangé" 
Message-ID: <20240104162942.211458-3-berra...@redhat.com>
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Thomas Huth 
---
 meson.build  |  1 -
 include/qemu/osdep.h | 10 --
 2 files changed, 11 deletions(-)

diff --git a/meson.build b/meson.build
index 371edafae6..8dd6347d1b 100644
--- a/meson.build
+++ b/meson.build
@@ -2341,7 +2341,6 @@ config_host_data.set('HAVE_GLIB_WITH_SLICE_ALLOCATOR', 
glib_has_gslice)
 config_host_data.set('HAVE_OPENPTY', cc.has_function('openpty', dependencies: 
util))
 config_host_data.set('HAVE_STRCHRNUL', cc.has_function('strchrnul'))
 config_host_data.set('HAVE_SYSTEM_FUNCTION', cc.has_function('system', prefix: 
'#include '))
-config_host_data.set('HAVE_GETLOADAVG_FUNCTION', cc.has_function('getloadavg', 
prefix: '#include '))
 if rbd.found()
   config_host_data.set('HAVE_RBD_NAMESPACE_EXISTS',
cc.has_function('rbd_namespace_exists',
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index db366d6796..9a405bed89 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -781,16 +781,6 @@ static inline int platform_does_not_support_system(const 
char *command)
 }
 #endif /* !HAVE_SYSTEM_FUNCTION */
 
-/**
- * If the load average was unobtainable, -1 is returned
- */
-#ifndef HAVE_GETLOADAVG_FUNCTION
-static inline int getloadavg(double loadavg[], int nelem)
-{
-return -1;
-}
-#endif /* !HAVE_GETLOADAVG_FUNCTION */
-
 #ifdef __cplusplus
 }
 #endif
-- 
2.43.0

[PULL 11/17] target/s390x/kvm/pv: Provide some more useful information if decryption fails

It's a common scenario to copy guest images from one host to another
to run the guest on the other machine. This (of course) does not work
with "secure execution" guests since they are encrypted with one certain
host key. However, if you still (accidentally) do it, you only get a
very user-unfriendly error message that looks like this:

 qemu-system-s390x: KVM PV command 2 (KVM_PV_SET_SEC_PARMS) failed:
  header rc 108 rrc 5 IOCTL rc: -22

Let's provide at least a somewhat nicer hint to the users so that they
are able to figure out what might have gone wrong.

Buglink: https://issues.redhat.com/browse/RHEL-18212
Message-ID: <20240110142916.850605-1-th...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Cédric Le Goater 
Reviewed-by: Claudio Imbrenda 
Signed-off-by: Thomas Huth 
---
 hw/s390x/ipl.h |  2 +-
 target/s390x/kvm/pv.h  |  5 +++--
 hw/s390x/ipl.c |  5 ++---
 hw/s390x/s390-virtio-ccw.c |  5 -
 target/s390x/kvm/pv.c  | 25 -
 5 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 7fc86e7905..57cd125769 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -107,7 +107,7 @@ typedef union IplParameterBlock IplParameterBlock;
 
 int s390_ipl_set_loadparm(uint8_t *loadparm);
 void s390_ipl_update_diag308(IplParameterBlock *iplb);
-int s390_ipl_prepare_pv_header(void);
+int s390_ipl_prepare_pv_header(Error **errp);
 int s390_ipl_pv_unpack(void);
 void s390_ipl_prepare_cpu(S390CPU *cpu);
 IplParameterBlock *s390_ipl_get_iplb(void);
diff --git a/target/s390x/kvm/pv.h b/target/s390x/kvm/pv.h
index 7b935e2246..5877d28ff1 100644
--- a/target/s390x/kvm/pv.h
+++ b/target/s390x/kvm/pv.h
@@ -42,7 +42,7 @@ int s390_pv_query_info(void);
 int s390_pv_vm_enable(void);
 void s390_pv_vm_disable(void);
 bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms);
-int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length, Error **errp);
 int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
 void s390_pv_prep_reset(void);
 int s390_pv_verify(void);
@@ -62,7 +62,8 @@ static inline int s390_pv_query_info(void) { return 0; }
 static inline int s390_pv_vm_enable(void) { return 0; }
 static inline void s390_pv_vm_disable(void) {}
 static inline bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms) { 
return false; }
-static inline int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) { 
return 0; }
+static inline int s390_pv_set_sec_parms(uint64_t origin, uint64_t length,
+Error **errp) { return 0; }
 static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) 
{ return 0; }
 static inline void s390_pv_prep_reset(void) {}
 static inline int s390_pv_verify(void) { return 0; }
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 76110e8f58..e934bf89d1 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -702,7 +702,7 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
 cpu_physical_memory_unmap(addr, len, 1, len);
 }
 
-int s390_ipl_prepare_pv_header(void)
+int s390_ipl_prepare_pv_header(Error **errp)
 {
 IplParameterBlock *ipib = s390_ipl_get_iplb_pv();
 IPLBlockPV *ipib_pv = >pv;
@@ -711,8 +711,7 @@ int s390_ipl_prepare_pv_header(void)
 
 cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
  ipib_pv->pv_header_len);
-rc = s390_pv_set_sec_parms((uintptr_t)hdr,
-   ipib_pv->pv_header_len);
+rc = s390_pv_set_sec_parms((uintptr_t)hdr, ipib_pv->pv_header_len, errp);
 g_free(hdr);
 return rc;
 }
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 1169e20b94..eaf61d3640 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -391,7 +391,7 @@ static int s390_machine_protect(S390CcwMachineState *ms)
 }
 
 /* Set SE header and unpack */
-rc = s390_ipl_prepare_pv_header();
+rc = s390_ipl_prepare_pv_header(_err);
 if (rc) {
 goto out_err;
 }
@@ -410,6 +410,9 @@ static int s390_machine_protect(S390CcwMachineState *ms)
 return rc;
 
 out_err:
+if (local_err) {
+error_report_err(local_err);
+}
 s390_machine_unprotect(ms);
 return rc;
 }
diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
index 6a69be7e5c..7ca7faec73 100644
--- a/target/s390x/kvm/pv.c
+++ b/target/s390x/kvm/pv.c
@@ -29,7 +29,8 @@ static bool info_valid;
 static struct kvm_s390_pv_info_vm info_vm;
 static struct kvm_s390_pv_info_dump info_dump;
 
-static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
+static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data,
+ int *pvrc)
 {
 struct kvm_pv_cmd pv_cmd = {
 .cmd = cmd,
@@ -46,6 +47,9 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, 
void *data)
  "IOCTL rc: %d", cmd, cmdname,

[PULL 06/17] net: add explicit info about connecting/listening state

From: Daniel P. Berrangé 

When running 'info network', if the stream backend is still in
the process of connecting, or waiting for an incoming connection,
no information is displayed.

There is also no way to distinguish whether the server is still
in the process of setting up the listener socket, or whether it
is ready to accept incoming client connections.

This leads to a race condition in the netdev-socket qtest which
launches a server process followed by a client process. Under
high load conditions it is possible for the client to attempt
to connect before the server is accepting clients. For the
scenarios which do not set the 'reconnect' option, this opens
up a race which can lead to the test scenario failing to reach
the expected state.

Now that 'info network' can distinguish between initialization
phase and the listening phase, the netdev-socket qtest will
correctly synchronize, such that the client QEMU is not spawned
until the server is ready.

This should solve the non-deterministic failures seen with the
netdev-socket qtest.

Signed-off-by: "Daniel P. Berrangé" 
Message-ID: <20240104162942.211458-5-berra...@redhat.com>
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Thomas Huth 
---
 net/stream.c|  5 -
 tests/qtest/netdev-socket.c | 10 +-
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/stream.c b/net/stream.c
index 9204b4c96e..0defb21d45 100644
--- a/net/stream.c
+++ b/net/stream.c
@@ -173,7 +173,7 @@ static gboolean net_stream_send(QIOChannel *ioc,
 
 net_socket_rs_init(>rs, net_stream_rs_finalize, false);
 s->nc.link_down = true;
-qemu_set_info_str(>nc, "%s", "");
+qemu_set_info_str(>nc, "listening");
 
 qapi_event_send_netdev_stream_disconnected(s->nc.name);
 net_stream_arm_reconnect(s);
@@ -292,6 +292,7 @@ static void net_stream_server_listening(QIOTask *task, 
gpointer opaque)
 s->nc.link_down = true;
 s->listener = qio_net_listener_new();
 
+qemu_set_info_str(>nc, "listening");
 net_socket_rs_init(>rs, net_stream_rs_finalize, false);
 qio_net_listener_set_client_func(s->listener, net_stream_listen, s, NULL);
 qio_net_listener_add(s->listener, listen_sioc);
@@ -309,6 +310,7 @@ static int net_stream_server_init(NetClientState *peer,
 
 nc = qemu_new_net_client(_stream_info, peer, model, name);
 s = DO_UPCAST(NetStreamState, nc, nc);
+qemu_set_info_str(>nc, "initializing");
 
 s->listen_ioc = QIO_CHANNEL(listen_sioc);
 qio_channel_socket_listen_async(listen_sioc, addr, 0,
@@ -400,6 +402,7 @@ static int net_stream_client_init(NetClientState *peer,
 
 nc = qemu_new_net_client(_stream_info, peer, model, name);
 s = DO_UPCAST(NetStreamState, nc, nc);
+qemu_set_info_str(>nc, "connecting");
 
 s->ioc = QIO_CHANNEL(sioc);
 s->nc.link_down = true;
diff --git a/tests/qtest/netdev-socket.c b/tests/qtest/netdev-socket.c
index 3fc2ac26d0..91441f7922 100644
--- a/tests/qtest/netdev-socket.c
+++ b/tests/qtest/netdev-socket.c
@@ -127,7 +127,7 @@ static void test_stream_inet_ipv4(void)
"addr.ipv4=on,addr.ipv6=off,"
"addr.host=127.0.0.1,addr.port=%d", port);
 
-EXPECT_STATE(qts0, "st0: index=0,type=stream,\r\n", 0);
+EXPECT_STATE(qts0, "st0: index=0,type=stream,listening\r\n", 0);
 
 qts1 = qtest_initf("-nodefaults -M none "
"-netdev stream,server=false,id=st0,addr.type=inet,"
@@ -200,7 +200,7 @@ static void test_stream_unix_reconnect(void)
"-netdev stream,id=st0,server=true,addr.type=unix,"
"addr.path=%s", path);
 
-EXPECT_STATE(qts0, "st0: index=0,type=stream,\r\n", 0);
+EXPECT_STATE(qts0, "st0: index=0,type=stream,listening\r\n", 0);
 
 qts1 = qtest_initf("-nodefaults -M none "
"-netdev stream,server=false,id=st0,addr.type=unix,"
@@ -250,7 +250,7 @@ static void test_stream_inet_ipv6(void)
"addr.ipv4=off,addr.ipv6=on,"
"addr.host=::1,addr.port=%d", port);
 
-EXPECT_STATE(qts0, "st0: index=0,type=stream,\r\n", 0);
+EXPECT_STATE(qts0, "st0: index=0,type=stream,listening\r\n", 0);
 
 qts1 = qtest_initf("-nodefaults -M none "
"-netdev stream,server=false,id=st0,addr.type=inet,"
@@ -282,7 +282,7 @@ static void test_stream_unix(void)
"addr.type=unix,addr.path=%s,",
path);
 
-EXPECT_STATE(qts0, "st0: index=0,type=stream,\r\n", 0);
+EXPECT_STATE(qts0, "st0: index=0,type=stream,listening\r\n", 0);
 
 qts1 = qtest_initf("-nodefaults -M none "
"-netdev stream,id=st0,server=false,"
@@ -314,7 +314,7 @@ static void test_stream_unix_abstract(void)
"addr.abstract=on",
path);
 
-EXPECT_STATE(qts0, "st0: index=0,type=stream,\r\n", 0);
+EXPECT_STATE(qts0, "st0:

[PULL 05/17] Revert "tests/qtest/netdev-socket: Raise connection timeout to 120 seconds"

From: Daniel P. Berrangé 

This reverts commit 0daaf2761f6d268ffaa2d01d450e202e127452b1.

The test was not timing out because of slow execution. It was
timing out due to a race condition leading to the client QEMU
attempting (and fatally failing) to connect before the server
QEMU was listening.

Signed-off-by: "Daniel P. Berrangé" 
Message-ID: <20240104162942.211458-4-berra...@redhat.com>
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Thomas Huth 
---
 tests/qtest/netdev-socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/netdev-socket.c b/tests/qtest/netdev-socket.c
index 7ba1eff120..3fc2ac26d0 100644
--- a/tests/qtest/netdev-socket.c
+++ b/tests/qtest/netdev-socket.c
@@ -16,7 +16,7 @@
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qapi-visit-sockets.h"
 
-#define CONNECTION_TIMEOUT120
+#define CONNECTION_TIMEOUT60
 
 #define EXPECT_STATE(q, e, t) \
 do {  \
-- 
2.43.0

[PULL 01/17] q800: move dp8393x_prom memory region to Q800MachineState

From: Mark Cave-Ayland 

There is no need to dynamically allocate the memory region from the heap.

Signed-off-by: Mark Cave-Ayland 
Message-ID: <20231227210212.245106-1-mark.cave-ayl...@ilande.co.uk>
Reviewed-by: Thomas Huth 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laurent Vivier 
Signed-off-by: Thomas Huth 
---
 include/hw/m68k/q800.h | 1 +
 hw/m68k/q800.c | 7 +++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/hw/m68k/q800.h b/include/hw/m68k/q800.h
index a9661f65f6..34365c9860 100644
--- a/include/hw/m68k/q800.h
+++ b/include/hw/m68k/q800.h
@@ -55,6 +55,7 @@ struct Q800MachineState {
 MOS6522Q800VIA1State via1;
 MOS6522Q800VIA2State via2;
 dp8393xState dp8393x;
+MemoryRegion dp8393x_prom;
 ESCCState escc;
 OrIRQState escc_orgate;
 SysBusESPState esp;
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 83d1571d02..b80a3b6d5f 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -253,7 +253,6 @@ static void q800_machine_init(MachineState *machine)
 int bios_size;
 ram_addr_t initrd_base;
 int32_t initrd_size;
-MemoryRegion *dp8393x_prom = g_new(MemoryRegion, 1);
 uint8_t *prom;
 int i, checksum;
 MacFbMode *macfb_mode;
@@ -406,13 +405,13 @@ static void q800_machine_init(MachineState *machine)
 sysbus_connect_irq(sysbus, 0,
qdev_get_gpio_in(DEVICE(>glue), GLUE_IRQ_IN_SONIC));
 
-memory_region_init_rom(dp8393x_prom, NULL, "dp8393x-q800.prom",
+memory_region_init_rom(>dp8393x_prom, NULL, "dp8393x-q800.prom",
SONIC_PROM_SIZE, _fatal);
 memory_region_add_subregion(get_system_memory(), SONIC_PROM_BASE,
-dp8393x_prom);
+>dp8393x_prom);
 
 /* Add MAC address with valid checksum to PROM */
-prom = memory_region_get_ram_ptr(dp8393x_prom);
+prom = memory_region_get_ram_ptr(>dp8393x_prom);
 checksum = 0;
 for (i = 0; i < 6; i++) {
 prom[i] = revbit8(nd_table[0].macaddr.a[i]);
-- 
2.43.0

[PULL 02/17] qtest: use correct boolean type for failover property

From: Daniel P. Berrangé 

QMP device_add does not historically validate the parameter types.
At some point it will likely change to enforce correct types, to
match behaviour of -device. The failover property is expected to
be a boolean in JSON.

Signed-off-by: "Daniel P. Berrangé" 
Message-ID: <20240103123005.2400437-1-berra...@redhat.com>
Reviewed-by: Zhao Liu 
Signed-off-by: Thomas Huth 
---
 tests/qtest/virtio-net-failover.c | 36 +++
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/tests/qtest/virtio-net-failover.c 
b/tests/qtest/virtio-net-failover.c
index 0d40bc1f2d..73dfabc272 100644
--- a/tests/qtest/virtio-net-failover.c
+++ b/tests/qtest/virtio-net-failover.c
@@ -486,7 +486,7 @@ static void test_hotplug_1_reverse(void)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",
  "{'bus': 'root0',"
- "'failover': 'on',"
+ "'failover': true,"
  "'netdev': 'hs0',"
  "'mac': '"MAC_STANDBY0"'}");
 
@@ -517,7 +517,7 @@ static void test_hotplug_2(void)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",
  "{'bus': 'root0',"
- "'failover': 'on',"
+ "'failover': true,"
  "'netdev': 'hs0',"
  "'mac': '"MAC_STANDBY0"'}");
 
@@ -566,7 +566,7 @@ static void test_hotplug_2_reverse(void)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",
  "{'bus': 'root0',"
- "'failover': 'on',"
+ "'failover': true,"
  "'netdev': 'hs0',"
  "'rombar': 0,"
  "'romfile': '',"
@@ -639,7 +639,7 @@ static void test_migrate_out(gconstpointer opaque)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",
  "{'bus': 'root0',"
- "'failover': 'on',"
+ "'failover': true,"
  "'netdev': 'hs0',"
  "'mac': '"MAC_STANDBY0"'}");
 
@@ -754,7 +754,7 @@ static void test_migrate_in(gconstpointer opaque)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",
  "{'bus': 'root0',"
- "'failover': 'on',"
+ "'failover': true,"
  "'netdev': 'hs0',"
  "'mac': '"MAC_STANDBY0"'}");
 
@@ -808,7 +808,7 @@ static void test_off_migrate_out(gconstpointer opaque)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",
  "{'bus': 'root0',"
- "'failover': 'off',"
+ "'failover': false,"
  "'netdev': 'hs0',"
  "'mac': '"MAC_STANDBY0"'}");
 
@@ -876,7 +876,7 @@ static void test_off_migrate_in(gconstpointer opaque)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",
  "{'bus': 'root0',"
- "'failover': 'off',"
+ "'failover': false,"
  "'netdev': 'hs0',"
  "'mac': '"MAC_STANDBY0"'}");
 
@@ -927,7 +927,7 @@ static void test_guest_off_migrate_out(gconstpointer opaque)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",
  "{'bus': 'root0',"
- "'failover': 'on',"
+ "'failover': true,"
  "'netdev': 'hs0',"
  "'mac': '"MAC_STANDBY0"'}");
 
@@ -1003,7 +1003,7 @@ static void test_guest_off_migrate_in(gconstpointer 
opaque)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",
  "{'bus': 'root0',"
- "'failover': 'on',"
+ "'failover': true,"
  "'netdev': 'hs0',"
  "'mac': '"MAC_STANDBY0"'}");
 
@@ -1054,7 +1054,7 @@ static void test_migrate_guest_off_abort(gconstpointer 
opaque)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",
  "{'bus': 'root0',"
- "'failover': 'on',"
+ "'failover': true,"
  "'netdev': 'hs0',"
  "'mac': '"MAC_STANDBY0"'}");
 
@@ -1154,7 +1154,7 @@ static void test_migrate_abort_wait_unplug(gconstpointer 
opaque)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",
  "{'bus': 'root0',"
- "'failover': 'on',"
+ "'failover': true,"
  "'netdev': 'hs0',"
  "'mac': '"MAC_STANDBY0"'}");
 
@@ -1243,7 +1243,7 @@ static void test_migrate_abort_active(gconstpointer 
opaque)
 
 qtest_qmp_device_add(qts, "virtio-net", "standby0",

[PULL 16/17] tests/tcg/s390x: Test LOAD ADDRESS EXTENDED

From: Ilya Leoshkevich 

Add a small test to prevent regressions. Userspace runs in primary
mode, so LAE should always set the access register to 0.

Signed-off-by: Ilya Leoshkevich 
Message-ID: <20240111092328.929421-3-...@linux.ibm.com>
Reviewed-by: Thomas Huth 
Signed-off-by: Thomas Huth 
---
 tests/tcg/s390x/lae.c   | 31 +++
 tests/tcg/s390x/Makefile.target |  1 +
 2 files changed, 32 insertions(+)
 create mode 100644 tests/tcg/s390x/lae.c

diff --git a/tests/tcg/s390x/lae.c b/tests/tcg/s390x/lae.c
new file mode 100644
index 00..59712b5e37
--- /dev/null
+++ b/tests/tcg/s390x/lae.c
@@ -0,0 +1,31 @@
+/*
+ * Test the LOAD ADDRESS EXTENDED instruction.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include 
+#include 
+
+int main(void)
+{
+unsigned long long ar = -1, b2 = 10, r, x2 = 500;
+/*
+ * Hardcode the register number, since clang does not allow using %rN in
+ * place of %aN.
+ */
+register unsigned long long r2 __asm__("2");
+int tmp;
+
+asm("ear %[tmp],%%a2\n"
+"lae %%r2,42(%[x2],%[b2])\n"
+"ear %[ar],%%a2\n"
+"sar %%a2,%[tmp]"
+: [tmp] "=" (tmp), "=" (r2), [ar] "+r" (ar)
+: [b2] "r" (b2), [x2] "r" (x2)
+: "memory");
+r = r2;
+assert(ar == 0xULL);
+assert(r == 100542);
+
+return EXIT_SUCCESS;
+}
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 0e670f3f8b..30994dcf9c 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -44,6 +44,7 @@ TESTS+=clgebr
 TESTS+=clc
 TESTS+=laalg
 TESTS+=add-logical-with-carry
+TESTS+=lae
 
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
-- 
2.43.0

[PULL 00/17] Misc patches (qtests, s390x, m68k, gitlab-ci)

 Hi Peter!

The following changes since commit 34eac35f893664eb8545b98142e23d9954722766:

  Merge tag 'pull-riscv-to-apply-20240110' of 
https://github.com/alistair23/qemu into staging (2024-01-10 11:41:56 +)

are available in the Git repository at:

  https://gitlab.com/thuth/qemu.git tags/pull-request-2024-01-11

for you to fetch changes up to 52a21689cd829c1cc931b59b5ee5bdb10dd578c1:

  .gitlab-ci.d/buildtest.yml: Work around htags bug when environment is large 
(2024-01-11 17:49:21 +0100)


* Fix non-deterministic failures of the 'netdev-socket' qtest
* Fix device presence checking in the virtio-ccw qtest
* Support codespell checking in checkpatch.pl
* Fix emulation of LAE s390x instruction
* Work around htags bug when environment is large
* Some other small clean-ups here and there


Daniel P. Berrangé (7):
  qtest: use correct boolean type for failover property
  Revert "netdev: set timeout depending on loadavg"
  Revert "osdep: add getloadavg"
  Revert "tests/qtest/netdev-socket: Raise connection timeout to 120 
seconds"
  net: add explicit info about connecting/listening state
  net: handle QIOTask completion to report useful error message
  qtest: ensure netdev-socket tests have non-overlapping names

Ilya Leoshkevich (2):
  target/s390x: Fix LAE setting a wrong access register
  tests/tcg/s390x: Test LOAD ADDRESS EXTENDED

Mark Cave-Ayland (1):
  q800: move dp8393x_prom memory region to Q800MachineState

Nicholas Piggin (1):
  gitlab: fix s390x tag for avocado-system-centos

Peter Maydell (1):
  .gitlab-ci.d/buildtest.yml: Work around htags bug when environment is 
large

Samuel Tardieu (1):
  tests/qtest/virtio-ccw: Fix device presence checking

Thomas Huth (1):
  target/s390x/kvm/pv: Provide some more useful information if decryption 
fails

Zhao Liu (3):
  hw/s390x/ccw: Replace basename() with g_path_get_basename()
  hw/s390x/ccw: Replace dirname() with g_path_get_dirname()
  scripts/checkpatch: Support codespell checking

 meson.build   |   1 -
 hw/s390x/ipl.h|   2 +-
 include/hw/m68k/q800.h|   1 +
 include/qemu/osdep.h  |  10 ---
 target/s390x/kvm/pv.h |   5 +-
 hw/m68k/q800.c|   7 +--
 hw/s390x/ipl.c|   5 +-
 hw/s390x/s390-ccw.c   |   7 ++-
 hw/s390x/s390-virtio-ccw.c|   5 +-
 net/stream.c  |  18 --
 target/s390x/kvm/pv.c |  25 ++--
 target/s390x/tcg/translate.c  |   3 +-
 tests/qtest/netdev-socket.c   |  42 +++--
 tests/qtest/virtio-ccw-test.c |   2 +-
 tests/qtest/virtio-net-failover.c |  36 +--
 tests/tcg/s390x/lae.c |  31 ++
 .gitlab-ci.d/buildtest.yml|   7 ++-
 scripts/checkpatch.pl | 125 --
 tests/tcg/s390x/Makefile.target   |   1 +
 19 files changed, 223 insertions(+), 110 deletions(-)
 create mode 100644 tests/tcg/s390x/lae.c

[PULL 08/17] qtest: ensure netdev-socket tests have non-overlapping names