Re: [PATCH] icount: don't adjust virtual time backwards after warp

[Paolo Bonzini]

On 6/27/23 08:14, Nicholas Piggin wrote:

The icount-based QEMU_CLOCK_VIRTUAL runs ahead of the RT clock at times.
When warping, it is possible it is still ahead at the end of the warp,
which causes icount adaptive mode to adjust it backward. This can result
in the machine observing time going backwards.

Prevent this by clamping adaptive adjustment to 0 at minimum.


Queued, thanks.

Paolo


Signed-off-by: Nicholas Piggin 
---
  softmmu/icount.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/softmmu/icount.c b/softmmu/icount.c
index 4504433e16..486ea7ef41 100644
--- a/softmmu/icount.c
+++ b/softmmu/icount.c
@@ -259,11 +259,16 @@ static void icount_warp_rt(void)
  warp_delta = clock - timers_state.vm_clock_warp_start;
  if (icount_enabled() == 2) {
  /*
- * In adaptive mode, do not let QEMU_CLOCK_VIRTUAL run too
- * far ahead of real time.
+ * In adaptive mode, do not let QEMU_CLOCK_VIRTUAL run too far
+ * ahead of real time (it might already be ahead so careful not
+ * to go backwards).
   */
  int64_t cur_icount = icount_get_locked();
  int64_t delta = clock - cur_icount;
+
+if (delta < 0) {
+delta = 0;
+}
  warp_delta = MIN(warp_delta, delta);
  }
  qatomic_set_i64(&timers_state.qemu_icount_bias,

Re: [PATCH v2] piix: fix regression during unplug in Xen HVM domUs

[Paolo Bonzini]

On 6/26/23 23:19, Olaf Hering wrote:

I need advice on how to debug this.

One thing that stands out is uhci_irq().
It reads a u16 from the USBSTS register.

On the qemu side, this read is served from bmdma_read. Since the read
size is 2, the result is ~0, and uhci_irq() turns the controller off.
In other words, memory_region_ops_read from addr=0xc102 is served from 
"piix-bmdma"

If the pci_set_word calls in piix_ide_reset are skipped, the read is
served from uhci_port_write. This is the expected behavior.
In other words, memory_region_ops_read from addr=0xc102 is served from "uhci".


I think what's happening is that

pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */

is setting the BAR to 0xC100, therefore overlapping the UHCI device's 
region.  In principle this line shouldn't be necessary at all though; 
it's enough to clear the COMMAND register.


Can you check the value of the COMMAND register (pci_conf + 0x04, 16 
bits, little endian)?  Something might be causing the register to be set 
back to a nonzero value, therefore re-enabling the I/O at the address 
that overlaps the UHCI device.


Paolo

Re: [PATCH 0/4] fpu: Add float64_to_int{32,64}_modulo

[Richard Henderson]

On 6/21/23 11:12, Richard Henderson wrote:

On 5/27/23 16:19, Richard Henderson wrote:

Extract some common code from Alpha and Arm, and which will
shortly also be required by the RISC-V Zfa extension.
Added a new test for Alpha; I already had a RISU test for Arm.


r~


Richard Henderson (4):
   fpu: Add float64_to_int{32,64}_modulo
   tests/tcg/alpha: Add test for cvttq
   target/alpha: Use float64_to_int64_modulo for CVTTQ
   target/arm: Use float64_to_int32_modulo for FJCVTZS

  include/fpu/softfloat.h |  3 ++
  fpu/softfloat.c | 31 
  target/alpha/fpu_helper.c   | 85 +++--
  target/arm/vfp_helper.c | 71 +--
  tests/tcg/alpha/test-cvttq.c    | 78 ++
  fpu/softfloat-parts.c.inc   | 78 ++
  tests/tcg/alpha/Makefile.target |  2 +-
  7 files changed, 221 insertions(+), 127 deletions(-)
  create mode 100644 tests/tcg/alpha/test-cvttq.c


ping.


ping 2.

r~

[PATCH v2 2/5] test-throttle: use enum ThrottleType

[zhenwei pi]

Use enum ThrottleType instead in the throttle test codes.

Signed-off-by: zhenwei pi 
---
 tests/unit/test-throttle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c
index 7adb5e6652..a60b5fe22e 100644
--- a/tests/unit/test-throttle.c
+++ b/tests/unit/test-throttle.c
@@ -169,8 +169,8 @@ static void test_init(void)
 
 /* check initialized fields */
 g_assert(tt->clock_type == QEMU_CLOCK_VIRTUAL);
-g_assert(tt->timers[0]);
-g_assert(tt->timers[1]);
+g_assert(tt->timers[THROTTLE_READ]);
+g_assert(tt->timers[THROTTLE_WRITE]);
 
 /* check other fields where cleared */
 g_assert(!ts.previous_leak);
@@ -191,7 +191,7 @@ static void test_destroy(void)
 throttle_timers_init(tt, ctx, QEMU_CLOCK_VIRTUAL,
  read_timer_cb, write_timer_cb, &ts);
 throttle_timers_destroy(tt);
-for (i = 0; i < 2; i++) {
+for (i = 0; i < THROTTLE_MAX; i++) {
 g_assert(!tt->timers[i]);
 }
 }
-- 
2.34.1

[PATCH v2 3/5] throttle: support read-only and write-only

[zhenwei pi]

Only one direction is necessary in several scenarios:
- a read-only disk
- operations on a device are considered as *write* only. For example,
  encrypt/decrypt/sign/verify operations on a cryptodev use a single
  *write* timer(read timer callback is defined, but never invoked).

Allow a single direction in throttle, this reduces memory, and uplayer
does not need a dummy callback any more.

Signed-off-by: zhenwei pi 
---
 util/throttle.c | 32 ++--
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/util/throttle.c b/util/throttle.c
index 5642e61763..c0bd0c26c3 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -199,12 +199,17 @@ static bool throttle_compute_timer(ThrottleState *ts,
 void throttle_timers_attach_aio_context(ThrottleTimers *tt,
 AioContext *new_context)
 {
-tt->timers[THROTTLE_READ] =
-aio_timer_new(new_context, tt->clock_type, SCALE_NS,
-  tt->timer_cb[THROTTLE_READ], tt->timer_opaque);
-tt->timers[THROTTLE_WRITE] =
-aio_timer_new(new_context, tt->clock_type, SCALE_NS,
-  tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);
+if (tt->timer_cb[THROTTLE_READ]) {
+tt->timers[THROTTLE_READ] =
+aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+  tt->timer_cb[THROTTLE_READ], tt->timer_opaque);
+}
+
+if (tt->timer_cb[THROTTLE_WRITE]) {
+tt->timers[THROTTLE_WRITE] =
+aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+  tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);
+}
 }
 
 /*
@@ -235,6 +240,7 @@ void throttle_timers_init(ThrottleTimers *tt,
   QEMUTimerCB *write_timer_cb,
   void *timer_opaque)
 {
+assert(read_timer_cb || write_timer_cb);
 memset(tt, 0, sizeof(ThrottleTimers));
 
 tt->clock_type = clock_type;
@@ -247,7 +253,9 @@ void throttle_timers_init(ThrottleTimers *tt,
 /* destroy a timer */
 static void throttle_timer_destroy(QEMUTimer **timer)
 {
-assert(*timer != NULL);
+if (*timer == NULL) {
+return;
+}
 
 timer_free(*timer);
 *timer = NULL;
@@ -272,7 +280,7 @@ void throttle_timers_destroy(ThrottleTimers *tt)
 /* is any throttling timer configured */
 bool throttle_timers_are_initialized(ThrottleTimers *tt)
 {
-if (tt->timers[0]) {
+if (tt->timers[THROTTLE_READ] || tt->timers[THROTTLE_WRITE]) {
 return true;
 }
 
@@ -424,8 +432,12 @@ bool throttle_schedule_timer(ThrottleState *ts,
 {
 int64_t now = qemu_clock_get_ns(tt->clock_type);
 int64_t next_timestamp;
+QEMUTimer *timer;
 bool must_wait;
 
+timer = is_write ? tt->timers[THROTTLE_WRITE] : tt->timers[THROTTLE_READ];
+assert(timer);
+
 must_wait = throttle_compute_timer(ts,
is_write,
now,
@@ -437,12 +449,12 @@ bool throttle_schedule_timer(ThrottleState *ts,
 }
 
 /* request throttled and timer pending -> do nothing */
-if (timer_pending(tt->timers[is_write])) {
+if (timer_pending(timer)) {
 return true;
 }
 
 /* request throttled and timer not pending -> arm timer */
-timer_mod(tt->timers[is_write], next_timestamp);
+timer_mod(timer, next_timestamp);
 return true;
 }
 
-- 
2.34.1

[PATCH v2 1/5] throttle: introduce enum ThrottleType

[zhenwei pi]

Use enum ThrottleType instead of number index.

Signed-off-by: zhenwei pi 
---
 include/qemu/throttle.h | 11 ---
 util/throttle.c | 16 +---
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 05f6346137..ba6293eeef 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -99,13 +99,18 @@ typedef struct ThrottleState {
 int64_t previous_leak;/* timestamp of the last leak done */
 } ThrottleState;
 
+typedef enum {
+THROTTLE_READ = 0,
+THROTTLE_WRITE,
+THROTTLE_MAX
+} ThrottleType;
+
 typedef struct ThrottleTimers {
-QEMUTimer *timers[2]; /* timers used to do the throttling */
+QEMUTimer *timers[THROTTLE_MAX];/* timers used to do the throttling */
 QEMUClockType clock_type; /* the clock used */
 
 /* Callbacks */
-QEMUTimerCB *read_timer_cb;
-QEMUTimerCB *write_timer_cb;
+QEMUTimerCB *timer_cb[THROTTLE_MAX];
 void *timer_opaque;
 } ThrottleTimers;
 
diff --git a/util/throttle.c b/util/throttle.c
index 81f247a8d1..5642e61763 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -199,10 +199,12 @@ static bool throttle_compute_timer(ThrottleState *ts,
 void throttle_timers_attach_aio_context(ThrottleTimers *tt,
 AioContext *new_context)
 {
-tt->timers[0] = aio_timer_new(new_context, tt->clock_type, SCALE_NS,
-  tt->read_timer_cb, tt->timer_opaque);
-tt->timers[1] = aio_timer_new(new_context, tt->clock_type, SCALE_NS,
-  tt->write_timer_cb, tt->timer_opaque);
+tt->timers[THROTTLE_READ] =
+aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+  tt->timer_cb[THROTTLE_READ], tt->timer_opaque);
+tt->timers[THROTTLE_WRITE] =
+aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+  tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);
 }
 
 /*
@@ -236,8 +238,8 @@ void throttle_timers_init(ThrottleTimers *tt,
 memset(tt, 0, sizeof(ThrottleTimers));
 
 tt->clock_type = clock_type;
-tt->read_timer_cb = read_timer_cb;
-tt->write_timer_cb = write_timer_cb;
+tt->timer_cb[THROTTLE_READ] = read_timer_cb;
+tt->timer_cb[THROTTLE_WRITE] = write_timer_cb;
 tt->timer_opaque = timer_opaque;
 throttle_timers_attach_aio_context(tt, aio_context);
 }
@@ -256,7 +258,7 @@ void throttle_timers_detach_aio_context(ThrottleTimers *tt)
 {
 int i;
 
-for (i = 0; i < 2; i++) {
+for (i = 0; i < THROTTLE_MAX; i++) {
 throttle_timer_destroy(&tt->timers[i]);
 }
 }
-- 
2.34.1

[PATCH v2 0/5] Misc fixes for throttle

[zhenwei pi]

v1 -> v2:
- rename 'ThrottleTimerType' to 'ThrottleType'
- add assertion to throttle_schedule_timer

Something remained:
- 'bool is_write' is no longer appropriate, the related functions
  need to use 'ThrottleType throttle' instead. To avoid changes from
  other subsystems in this series, do this work in a followup series
  after there patches apply.


v1:
- introduce enum ThrottleTimerType instead of timers[0], timer[1]...
- support read-only and write-only for throttle
- adapt related test codes
- cryptodev uses a write-only throttle timer

Zhenwei Pi (5):
  throttle: introduce enum ThrottleType
  test-throttle: use enum ThrottleType
  throttle: support read-only and write-only
  test-throttle: test read only and write only
  cryptodev: use NULL throttle timer cb for read direction

 backends/cryptodev.c   |  3 +-
 include/qemu/throttle.h| 11 --
 tests/unit/test-throttle.c | 72 --
 util/throttle.c| 36 +--
 4 files changed, 103 insertions(+), 19 deletions(-)

-- 
2.34.1

[PATCH v2 4/5] test-throttle: test read only and write only

[zhenwei pi]

Signed-off-by: zhenwei pi 
---
 tests/unit/test-throttle.c | 66 ++
 1 file changed, 66 insertions(+)

diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c
index a60b5fe22e..5547837a58 100644
--- a/tests/unit/test-throttle.c
+++ b/tests/unit/test-throttle.c
@@ -184,6 +184,70 @@ static void test_init(void)
 throttle_timers_destroy(tt);
 }
 
+static void test_init_readonly(void)
+{
+int i;
+
+tt = &tgm.throttle_timers;
+
+/* fill the structures with crap */
+memset(&ts, 1, sizeof(ts));
+memset(tt, 1, sizeof(*tt));
+
+/* init structures */
+throttle_init(&ts);
+throttle_timers_init(tt, ctx, QEMU_CLOCK_VIRTUAL,
+ read_timer_cb, NULL, &ts);
+
+/* check initialized fields */
+g_assert(tt->clock_type == QEMU_CLOCK_VIRTUAL);
+g_assert(tt->timers[THROTTLE_READ]);
+g_assert(!tt->timers[THROTTLE_WRITE]);
+
+/* check other fields where cleared */
+g_assert(!ts.previous_leak);
+g_assert(!ts.cfg.op_size);
+for (i = 0; i < BUCKETS_COUNT; i++) {
+g_assert(!ts.cfg.buckets[i].avg);
+g_assert(!ts.cfg.buckets[i].max);
+g_assert(!ts.cfg.buckets[i].level);
+}
+
+throttle_timers_destroy(tt);
+}
+
+static void test_init_writeonly(void)
+{
+int i;
+
+tt = &tgm.throttle_timers;
+
+/* fill the structures with crap */
+memset(&ts, 1, sizeof(ts));
+memset(tt, 1, sizeof(*tt));
+
+/* init structures */
+throttle_init(&ts);
+throttle_timers_init(tt, ctx, QEMU_CLOCK_VIRTUAL,
+ NULL, write_timer_cb, &ts);
+
+/* check initialized fields */
+g_assert(tt->clock_type == QEMU_CLOCK_VIRTUAL);
+g_assert(!tt->timers[THROTTLE_READ]);
+g_assert(tt->timers[THROTTLE_WRITE]);
+
+/* check other fields where cleared */
+g_assert(!ts.previous_leak);
+g_assert(!ts.cfg.op_size);
+for (i = 0; i < BUCKETS_COUNT; i++) {
+g_assert(!ts.cfg.buckets[i].avg);
+g_assert(!ts.cfg.buckets[i].max);
+g_assert(!ts.cfg.buckets[i].level);
+}
+
+throttle_timers_destroy(tt);
+}
+
 static void test_destroy(void)
 {
 int i;
@@ -752,6 +816,8 @@ int main(int argc, char **argv)
 g_test_add_func("/throttle/leak_bucket",test_leak_bucket);
 g_test_add_func("/throttle/compute_wait",   test_compute_wait);
 g_test_add_func("/throttle/init",   test_init);
+g_test_add_func("/throttle/init_readonly",  test_init_readonly);
+g_test_add_func("/throttle/init_writeonly", test_init_writeonly);
 g_test_add_func("/throttle/destroy",test_destroy);
 g_test_add_func("/throttle/have_timer", test_have_timer);
 g_test_add_func("/throttle/detach_attach",  test_detach_attach);
-- 
2.34.1

[PATCH v2 5/5] cryptodev: use NULL throttle timer cb for read direction

[zhenwei pi]

Operations on a crytpodev are considered as *write* only, the callback
of read direction is never invoked. Use NULL instead of an unreachable
path(cryptodev_backend_throttle_timer_cb on read direction).

Signed-off-by: zhenwei pi 
---
 backends/cryptodev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/backends/cryptodev.c b/backends/cryptodev.c
index 7d29517843..5cfa25c61c 100644
--- a/backends/cryptodev.c
+++ b/backends/cryptodev.c
@@ -331,8 +331,7 @@ static void cryptodev_backend_set_throttle(CryptoDevBackend 
*backend, int field,
 if (!enabled) {
 throttle_init(&backend->ts);
 throttle_timers_init(&backend->tt, qemu_get_aio_context(),
- QEMU_CLOCK_REALTIME,
- cryptodev_backend_throttle_timer_cb, /* FIXME */
+ QEMU_CLOCK_REALTIME, NULL,
  cryptodev_backend_throttle_timer_cb, backend);
 }
 
-- 
2.34.1

Re: [PATCH v2 6/6] qemu-keymap: properly check return from xkb_keymap_mod_get_index

[Juan Quintela]

Alex Bennée  wrote:
> We can return XKB_MOD_INVALID which rightly gets flagged by sanitisers
> as an overly wide shift attempt.
>
> Signed-off-by: Alex Bennée 

Reviewed-by: Juan Quintela 

Re: [PATCH v2 08/26] tests/qtests: clean-up and fix leak in generic_fuzz

[Juan Quintela]

Alex Bennée  wrote:
> An update to the clang tooling detects more issues with the code
> including a memory leak from the g_string_new() allocation. Clean up
> the code with g_autoptr and use ARRAY_SIZE while we are at it.
>
> Signed-off-by: Alex Bennée 
> ---
>  tests/qtest/fuzz/generic_fuzz.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
> index c525d22951..a4841181cc 100644
> --- a/tests/qtest/fuzz/generic_fuzz.c
> +++ b/tests/qtest/fuzz/generic_fuzz.c
> @@ -954,17 +954,14 @@ static void register_generic_fuzz_targets(void)
>  .crossover = generic_fuzz_crossover
>  });
>  
> -GString *name;
> +g_autoptr(GString) name = g_string_new("");
>  const generic_fuzz_config *config;
>  
> -for (int i = 0;
> - i < sizeof(predefined_configs) / sizeof(generic_fuzz_config);
> - i++) {
> +for (int i = 0; i < ARRAY_SIZE(predefined_configs); i++) {
>  config = predefined_configs + i;
> -name = g_string_new("generic-fuzz");
> -g_string_append_printf(name, "-%s", config->name);
> +g_string_printf(name, "generic-fuzz-%s", config->name);
>  fuzz_add_target(&(FuzzTarget){
> -.name = name->str,
> +.name = g_strdup(name->str),
>  .description = "Predefined generic-fuzz config.",
>  .get_init_cmdline = generic_fuzz_predefined_config_cmdline,
>  .pre_fuzz = generic_pre_fuzz,

Once that you are here, what about?
(Yes, I didn't care about the ARRAY_SIZE) but you got the idea.

Reviewed-by: Juan Quintela 

To your proposal with/without the change that I proposse.

modified   tests/qtest/fuzz/generic_fuzz.c
@@ -954,17 +954,14 @@ static void register_generic_fuzz_targets(void)
 .crossover = generic_fuzz_crossover
 });
 
-GString *name;
 const generic_fuzz_config *config;
 
 for (int i = 0;
  i < sizeof(predefined_configs) / sizeof(generic_fuzz_config);
  i++) {
 config = predefined_configs + i;
-name = g_string_new("generic-fuzz");
-g_string_append_printf(name, "-%s", config->name);
 fuzz_add_target(&(FuzzTarget){
-.name = name->str,
+.name = g_strdup_printf("generic-fuzz-%s", config->name),
 .description = "Predefined generic-fuzz config.",
 .get_init_cmdline = generic_fuzz_predefined_config_cmdline,
 .pre_fuzz = generic_pre_fuzz,

[PATCH v2 1/4] pc-bios/s390-ccw: Fix indentation in start.S

[Thomas Huth]

start.S is currently indented with a mixture of spaces and tabs, which
is quite ugly. QEMU coding style says indentation should be 4 spaces,
and this is also what we are using in the assembler files in the
tests/tcg/s390x/ folder already, so let's adjust start.S accordingly.

Reviewed-by: Cédric Le Goater 
Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/start.S | 136 +++
 1 file changed, 68 insertions(+), 68 deletions(-)

diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 6072906df4..d29de09cc6 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -10,37 +10,37 @@
  * directory.
  */
 
-.globl _start
+.globl _start
 _start:
 
-   larl   %r15, stack + 0x8000 /* Set up stack */
+larl%r15,stack + 0x8000 /* Set up stack */
 
-   /* clear bss */
-   larl %r2, __bss_start
-   larl %r3, _end
-   slgr %r3, %r2   /* get sizeof bss */
-   ltgr%r3,%r3 /* bss empty? */
-   jz  done
-   aghi%r3,-1
-   srlg%r4,%r3,8   /* how many 256 byte chunks? */
-   ltgr%r4,%r4
-   lgr %r1,%r2
-   jz  remainder
+/* clear bss */
+larl%r2,__bss_start
+larl%r3,_end
+slgr%r3,%r2/* get sizeof bss */
+ltgr%r3,%r3/* bss empty? */
+jz  done
+aghi%r3,-1
+srlg%r4,%r3,8  /* how many 256 byte chunks? */
+ltgr%r4,%r4
+lgr %r1,%r2
+jz  remainder
 loop:
-   xc  0(256,%r1),0(%r1)
-   la  %r1,256(%r1)
-   brctg   %r4,loop
+xc  0(256,%r1),0(%r1)
+la  %r1,256(%r1)
+brctg   %r4,loop
 remainder:
-   larl%r2,memsetxc
-   ex  %r3,0(%r2)
+larl%r2,memsetxc
+ex  %r3,0(%r2)
 done:
-/* set up a pgm exception disabled wait psw */
-larl   %r2, disabled_wait_psw
-mvc0x01d0(16), 0(%r2)
-j  main/* And call C */
+/* set up a pgm exception disabled wait psw */
+larl%r2,disabled_wait_psw
+mvc 0x01d0(16),0(%r2)
+j   main   /* And call C */
 
 memsetxc:
-   xc  0(1,%r1),0(%r1)
+xc  0(1,%r1),0(%r1)
 
 
 /*
@@ -48,11 +48,11 @@ memsetxc:
  *
  * stops the current guest cpu.
  */
-   .globl disabled_wait
+.globl disabled_wait
 disabled_wait:
-   larl%r1,disabled_wait_psw
-   lpswe   0(%r1)
-1: j   1b
+larl%r1,disabled_wait_psw
+lpswe   0(%r1)
+1:  j   1b
 
 
 /*
@@ -60,61 +60,61 @@ disabled_wait:
  *
  * eats one sclp interrupt
  */
-.globl consume_sclp_int
+.globl consume_sclp_int
 consume_sclp_int:
-/* enable service interrupts in cr0 */
-stctg   %c0,%c0,0(%r15)
-oi  6(%r15),0x2
-lctlg   %c0,%c0,0(%r15)
-/* prepare external call handler */
-larl %r1, external_new_code
-stg %r1, 0x1b8
-larl %r1, external_new_mask
-mvc 0x1b0(8),0(%r1)
-/* load enabled wait PSW */
-larl %r1, enabled_wait_psw
-lpswe 0(%r1)
+/* enable service interrupts in cr0 */
+stctg   %c0,%c0,0(%r15)
+oi  6(%r15),0x2
+lctlg   %c0,%c0,0(%r15)
+/* prepare external call handler */
+larl%r1,external_new_code
+stg %r1,0x1b8
+larl%r1,external_new_mask
+mvc 0x1b0(8),0(%r1)
+/* load enabled wait PSW */
+larl%r1,enabled_wait_psw
+lpswe   0(%r1)
 
 /*
  * void consume_io_int(void)
  *
  * eats one I/O interrupt
  */
-.globl consume_io_int
+.globl consume_io_int
 consume_io_int:
-/* enable I/O interrupts in cr6 */
-stctg %c6,%c6,0(%r15)
-oi4(%r15), 0xff
-lctlg %c6,%c6,0(%r15)
-/* prepare i/o call handler */
-larl  %r1, io_new_code
-stg   %r1, 0x1f8
-larl  %r1, io_new_mask
-mvc   0x1f0(8),0(%r1)
-/* load enabled wait PSW */
-larl  %r1, enabled_wait_psw
-lpswe 0(%r1)
+/* enable I/O interrupts in cr6 */
+stctg   %c6,%c6,0(%r15)
+oi  4(%r15), 0xff
+lctlg   %c6,%c6,0(%r15)
+/* prepare i/o call handler */
+larl%r1,io_new_code
+stg %r1,0x1f8
+larl%r1,io_new_mask
+mvc 0x1f0(8),0(%r1)
+/* load enabled wait PSW */
+larl%r1,enabled_wait_psw
+lpswe   0(%r1)
 
 external_new_code:
-/* disable service interrupts in cr0 */
-stctg   %c0,%c0,0(%r15)
-ni  6(%r15),0xfd
-lctlg   %c0,%c0,0(%r15)
-br  %r14
+/* disable service interrupts in cr0 */
+stctg   %c0,%c0,0(%r15)
+ni  6(%r15),0xfd
+lctlg   %c0,%c0,0(%r15)
+br  %r14
 
 io_new_code:
-/* disable I/O interrupts in cr6 */
-stctg %c6,%c6,0(%r15)
-ni4(%r15), 0x00
-lctlg %c6,%c6,0(%r15)
-br%r14
+/* disable I/O interrupts in cr6 */
+stctg   %c6,%c6,0(%r15)
+ni  4(%r15),0x00
+lctlg   %c6,%c6

[PATCH v2 2/4] pc-bios/s390-ccw: Provide space for initial stack frame in start.S

[Thomas Huth]

Providing the space of a stack frame is the duty of the caller,
so we should reserve 160 bytes before jumping into the main function.
Otherwise the main() function might write past the stack array.

While we're at it, add a proper STACK_SIZE macro for the stack size
instead of using magic numbers (this is also required for the following
patch).

Reviewed-by: Christian Borntraeger 
Reviewed-by: Cédric Le Goater 
Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/start.S | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index d29de09cc6..29b0a9ece0 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -10,10 +10,12 @@
  * directory.
  */
 
+#define STACK_SIZE 0x8000
+
 .globl _start
 _start:
 
-larl%r15,stack + 0x8000 /* Set up stack */
+larl%r15,stack + STACK_SIZE - 160   /* Set up stack */
 
 /* clear bss */
 larl%r2,__bss_start
-- 
2.39.3

[PATCH v2 0/4] pc-bios/s390-ccw: Fixes and improvements for start.S

[Thomas Huth]

Main motivation of this series was a bug that showed up when compiling
with Clang 16 and binutils 2.40 (which has been reported in Fedora ELN, see
https://bugzilla.redhat.com/show_bug.cgi?id=2216662). This is fixed in
the fourth patch. I checked with "objdump" that the change is fine, indeed.

While working on this issue, I came accross some other issues which I
address in the first three patches:

- Indentation is a mixture between tabs and spaces in start.S (patch 1)
- We do not set up a stack frame for the main() function, which could
  cause memory corruption (patch 2)
- The stack is declared in multiple places, though it's only needed
  in start.S (patch 3)

v2:
- Use ".space" instead of ".lcomm" in the third patch to make sure
  that the alignment is really taken into consideration (thanks Richard)
- Alignment of 8 should be enough in the third patch (thank Christian)
- Added Reviewed-bys from v1

Thomas Huth (4):
  pc-bios/s390-ccw: Fix indentation in start.S
  pc-bios/s390-ccw: Provide space for initial stack frame in start.S
  pc-bios/s390-ccw: Move the stack array into start.S
  pc-bios/s390-ccw: Don't use __bss_start with the "larl" instruction

 pc-bios/s390-ccw/s390-ccw.h |   1 -
 pc-bios/s390-ccw/main.c |   1 -
 pc-bios/s390-ccw/netmain.c  |   1 -
 pc-bios/s390-ccw/start.S| 144 +++-
 4 files changed, 76 insertions(+), 71 deletions(-)

-- 
2.39.3

[PATCH v2 4/4] pc-bios/s390-ccw: Don't use __bss_start with the "larl" instruction

[Thomas Huth]

start.S currently cannot be compiled with Clang 16 and binutils 2.40:

 ld: start.o(.text+0x8): misaligned symbol `__bss_start' (0xc1e5) for
 relocation R_390_PC32DBL

According to the built-in linker script of ld, the symbol __bss_start
can actually point *before* the .bss section and does not need to have
any alignment, so in certain situations (like when using the internal
assembler of Clang), the __bss_start symbol can indeed be unaligned
and thus it is not suitable for being used with the "larl" instruction
that needs an address that is at least aligned to halfwords.
The problem went unnoticed so far since binutils <= 2.39 did not
check the alignment, but starting with binutils 2.40, such unaligned
addresses are now refused.

Fix it by using the real start address of the .bss section instead.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2216662
Reported-by: Miroslav Rezanina 
Suggested-by: Nick Clifton 
Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/start.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 111dea261b..a63c4e3ff2 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -18,7 +18,7 @@ _start:
 larl%r15,stack + STACK_SIZE - 160   /* Set up stack */
 
 /* clear bss */
-larl%r2,__bss_start
+larl%r2,.bss
 larl%r3,_end
 slgr%r3,%r2/* get sizeof bss */
 ltgr%r3,%r3/* bss empty? */
-- 
2.39.3

[PATCH v2 3/4] pc-bios/s390-ccw: Move the stack array into start.S

[Thomas Huth]

The stack array is only referenced from the start-up code (which is
shared between the s390-ccw.img and the s390-netboot.img), but it is
currently declared twice, once in main.c and once in netmain.c.
It makes more sense to declare this in start.S instead - which will
also be helpful in the next patch, since we need to mention the .bss
section in start.S in that patch.

While we're at it, let's also drop the huge alignment of the stack,
since there is no technical requirement for aligning it to page
boundaries.

Signed-off-by: Thomas Huth 
---
 pc-bios/s390-ccw/s390-ccw.h | 1 -
 pc-bios/s390-ccw/main.c | 1 -
 pc-bios/s390-ccw/netmain.c  | 1 -
 pc-bios/s390-ccw/start.S| 6 ++
 4 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index b88e0550ab..91afcbbca9 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -55,7 +55,6 @@ void consume_io_int(void);
 /* main.c */
 void write_subsystem_identification(void);
 void write_iplb_location(void);
-extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 unsigned int get_loadparm_index(void);
 void main(void);
 
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index a2def83e82..5506798098 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -17,7 +17,6 @@
 #include "virtio-scsi.h"
 #include "dasd-ipl.h"
 
-char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 static SubChannelId blk_schid = { .one = 1 };
 static char loadparm_str[LOADPARM_LEN + 1];
 QemuIplParameters qipl;
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index 056e93a818..5cd619b2d6 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -50,7 +50,6 @@ void write_iplb_location(void) {}
 /* STSI 3.2.2 offset of first vmdb + offset of uuid inside vmdb */
 #define STSI322_VMDB_UUID_OFFSET ((8 + 12) * 4)
 
-char stack[PAGE_SIZE * 8] __attribute__((aligned(PAGE_SIZE)));
 IplParameterBlock iplb __attribute__((aligned(PAGE_SIZE)));
 static char cfgbuf[2048];
 
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 29b0a9ece0..111dea261b 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -120,3 +120,9 @@ external_new_mask:
 .quad   0x00018000
 io_new_mask:
 .quad   0x00018000
+
+.bss
+.align  8
+stack:
+.space  STACK_SIZE
+.size   stack,STACK_SIZE
-- 
2.39.3

Re: [PATCH v2 08/26] tests/qtests: clean-up and fix leak in generic_fuzz

[Alexander Bulekov]

On 230626 2259, Alex Bennée wrote:
> An update to the clang tooling detects more issues with the code
> including a memory leak from the g_string_new() allocation. Clean up
> the code with g_autoptr and use ARRAY_SIZE while we are at it.
> 
> Signed-off-by: Alex Bennée 

Reviewed-by: Alexander Bulekov 

Thank you

> ---
>  tests/qtest/fuzz/generic_fuzz.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
> index c525d22951..a4841181cc 100644
> --- a/tests/qtest/fuzz/generic_fuzz.c
> +++ b/tests/qtest/fuzz/generic_fuzz.c
> @@ -954,17 +954,14 @@ static void register_generic_fuzz_targets(void)
>  .crossover = generic_fuzz_crossover
>  });
>  
> -GString *name;
> +g_autoptr(GString) name = g_string_new("");
>  const generic_fuzz_config *config;
>  
> -for (int i = 0;
> - i < sizeof(predefined_configs) / sizeof(generic_fuzz_config);
> - i++) {
> +for (int i = 0; i < ARRAY_SIZE(predefined_configs); i++) {
>  config = predefined_configs + i;
> -name = g_string_new("generic-fuzz");
> -g_string_append_printf(name, "-%s", config->name);
> +g_string_printf(name, "generic-fuzz-%s", config->name);
>  fuzz_add_target(&(FuzzTarget){
> -.name = name->str,
> +.name = g_strdup(name->str),
>  .description = "Predefined generic-fuzz config.",
>  .get_init_cmdline = generic_fuzz_predefined_config_cmdline,
>  .pre_fuzz = generic_pre_fuzz,
> -- 
> 2.39.2
> 

Re: [PATCH 3/3] vfio/migration: Make VFIO migration non-experimental

[Avihai Horon]

On 26/06/2023 20:27, Alex Williamson wrote:

External email: Use caution opening links or attachments


On Mon, 26 Jun 2023 17:26:42 +0200
Cédric Le Goater  wrote:


On 6/26/23 15:40, Joao Martins wrote:

On 26/06/2023 14:20, Cédric Le Goater wrote:

Hello Avihai,

On 6/26/23 10:23, Avihai Horon wrote:

The major parts of VFIO migration are supported today in QEMU. This
includes basic VFIO migration, device dirty page tracking and precopy
support.

Thus, at this point in time, it seems appropriate to make VFIO migration
non-experimental: remove the x prefix from enable_migration property,
change it to ON_OFF_AUTO and let the default value be AUTO.

In addition, make the following adjustments:
1. Require device dirty tracking support when enable_migration is AUTO
  (i.e., not explicitly enabled). This is because device dirty tracking
  is currently the only method to do dirty page tracking, which is
  essential for migrating in a reasonable downtime.

hmm, I don't think QEMU should decide to disable a feature for all
devices supposedly because it could be slow for some. That's too
restrictive. What about devices with have small states ? for which
the downtime would be reasonable even without device dirty tracking
support.


device dirty tracking refers to the ability to tracking dirty IOVA used by the
device which will DMA into RAM. It is required because the
consequence/alternative is to transfer all RAM in stop copy phase. Device state
size at that point is the least of our problems downtime wise.

Arg. thanks for reminding me. I tend to take this for granted ...

My initial reaction was the same, for instance we could have a non-DMA
device (ex. PCI serial card) that supports migration w/o dirty
tracking, but QEMU cannot assume the device doesn't do DMA, nor is it
worth our time to monitor whether bus-master is ever enabled for the
device, so QEMU needs to assume all memory that's DMA accessible for
the device is perpetually dirty.  Also, if such a corner case existed
for a non-DMA migratable device, the easier path is simply to require
dirty tracking stubs in the variant driver to report no memory dirtied.


I can imagine that allowing without dirty tracking is useful for developer
testing of the suspend/device-state flows, but as real default (auto) is very
questionable to let it go through without dirty tracking. When we have IOMMUFD
dirty tracking that's when we can relieve this restriction as a default.

But then note that (...)


Setting
  enable_migration to ON will not require device dirty tracking.

(...) this lets it ignore dirty tracking as you would like.



2. Make migration blocker messages more elaborate.
3. Remove error prints in vfio_migration_query_flags().
4. Remove a redundant assignment in vfio_migration_realize().

Signed-off-by: Avihai Horon 
---
include/hw/vfio/vfio-common.h |  2 +-
hw/vfio/migration.c   | 29 -
hw/vfio/pci.c |  4 ++--
3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index b4c28f318f..387eabde60 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -139,7 +139,7 @@ typedef struct VFIODevice {
bool needs_reset;
bool no_mmap;
bool ram_block_discard_allowed;
-bool enable_migration;
+OnOffAuto enable_migration;
VFIODeviceOps *ops;
unsigned int num_irqs;
unsigned int num_regions;
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 79eb81dfd7..d8e0848635 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -731,14 +731,6 @@ static int vfio_migration_query_flags(VFIODevice
*vbasedev, uint64_t *mig_flags)
feature->argsz = sizeof(buf);
feature->flags = VFIO_DEVICE_FEATURE_GET | 
VFIO_DEVICE_FEATURE_MIGRATION;
if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
-if (errno == ENOTTY) {
-error_report("%s: VFIO migration is not supported in kernel",
- vbasedev->name);
-} else {
-error_report("%s: Failed to query VFIO migration support, err: %s",
- vbasedev->name, strerror(errno));
-}
-
return -errno;
}
@@ -831,14 +823,28 @@ void vfio_reset_bytes_transferred(void)
  int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
{
-int ret = -ENOTSUP;
+int ret;
-if (!vbasedev->enable_migration) {
+if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
+error_setg(&vbasedev->migration_blocker,
+   "%s: Migration is disabled for VFIO device", 
vbasedev->name);
goto add_blocker;
}
  ret = vfio_migration_init(vbasedev);
if (ret) {

It would be good to keep the message for 'errno == ENOTTY' as it was in
vfio_migration_query_flags(). When migration fails, it is an important
information to know 

Re: [PATCH v4 16/19] target/riscv/cpu.c: create KVM mock properties

[Andrew Jones]

On Mon, Jun 26, 2023 at 07:02:06PM -0300, Daniel Henrique Barboza wrote:
> KVM-specific properties are being created inside target/riscv/kvm.c. But
> at this moment we're gathering all the remaining properties from TCG and
> adding them as is when running KVM. This creates a situation where
> non-KVM properties are setting flags to 'true' due to its default
> settings (e.g.  Zawrs). Users can also freely enable them via command
> line.
> 
> This doesn't impact runtime per se because KVM doesn't care about these
> flags, but code such as riscv_isa_string_ext() take those flags into
> account. The result is that, for a KVM guest, setting non-KVM properties
> will make them appear in the riscv,isa DT.
> 
> We want to keep the same API for both TCG and KVM and at the same time,
> when running KVM, forbid non-KVM extensions to be enabled internally. We
> accomplish both by changing riscv_cpu_add_user_properties() to add a
> mock/no-op boolean property for every non-KVM extension in
> riscv_cpu_extensions[]. Then, when running KVM, users are still free to
> set extensions at will, we'll treat non-KVM extensions as a no-op, and
> riscv_isa_string_ext() will not report bogus extensions in the DT.

We're no longer treating these as no-ops. We're now intercepting attempts
to set unsupported extensions and erroring out.

> 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/riscv/cpu.c | 40 +---
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b65db165cc..ad4b0e3490 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1720,6 +1720,22 @@ static Property riscv_cpu_extensions[] = {
>  DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +
> +static void cpu_set_cfg_noop(Object *obj, Visitor *v,

As stated above, I don't think 'noop' conveys the right message.

> + const char *name,
> + void *opaque, Error **errp)
> +{
> +const char *propname = opaque;
> +bool value;
> +
> +if (!visit_type_bool(v, name, &value, errp)) {
> +return;
> +}
> +

We should only error out when value == true. Just like the other
KVM-unsupported extensions, we don't mind if the user explicitly
disables something we can't handle.

> +error_setg(errp, "extension %s is not available with KVM",
> +   propname);
> +}
> +
>  /*
>   * Add CPU properties with user-facing flags.
>   *
> @@ -1738,9 +1754,27 @@ static void riscv_cpu_add_user_properties(Object *obj)
>  riscv_cpu_add_misa_properties(obj);
>  
>  for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
> -/* Check if KVM didn't create the property already */
> -if (object_property_find(obj, prop->name)) {
> -continue;
> +if (riscv_running_kvm()) {
> +/* Check if KVM didn't create the property already */

The check is for the positive, so I think the comment would read better as

 "Check if KVM created the property already."

> +if (object_property_find(obj, prop->name)) {
> +continue;
> +}
> +
> +/*
> + * Set every multi-letter extension that KVM doesn't
> + * know as a no-op. This will allow users to set values

How about "Set the default to disabled for every extension unknown to
KVM and error out if the user attempts to enable any of them."


> + * to them while keeping their internal state to 'false'.
> + *
> + * We're giving a pass for non-bool properties since they're
> + * not related to the availability of extensions and can be
> + * safely ignored as is.

I guess that's OK for now. Ideally, we wouldn't have any non-booleans as
that will complicate QMP cpu model expansion. Instead, we'd have booleans
for valid values. For example, for vlen, we'd have vlen128, vlen256,
vlen512, etc.. Figuring out how to do that best is for another series
though.

> + */
> +if (prop->info == &qdev_prop_bool) {
> +object_property_add(obj, prop->name, "bool",
> +NULL, cpu_set_cfg_noop,
> +NULL, (void *)prop->name);
> +continue;
> +}
>  }
>  
>  qdev_property_add_static(dev, prop);
> -- 
> 2.41.0
>

Thanks,
drew

Re: [PATCH 0/4] target/ppc: Catch invalid real address accesses

[Mark Cave-Ayland]

On 26/06/2023 14:35, Cédric Le Goater wrote:


On 6/23/23 14:37, Cédric Le Goater wrote:

On 6/23/23 11:10, Peter Maydell wrote:

On Fri, 23 Jun 2023 at 09:21, Nicholas Piggin  wrote:


ppc has always silently ignored access to real (physical) addresses
with nothing behind it, which can make debugging difficult at times.

It looks like the way to handle this is implement the transaction
failed call, which most target architectures do. Notably not x86
though, I wonder why?


Much of this is historical legacy. QEMU originally had no
concept of "the system outside the CPU returns some kind
of bus error and the CPU raises an exception for it".
This is turn is (I think) because the x86 PC doesn't do
that: you always get back some kind of response, I think
-1 on reads and writes ignored. We added the do_transaction_failed
hook largely because we wanted it to give more accurate
emulation of this kind of thing on Arm, but as usual with new
facilities we left the other architectures to do it themselves
if they wanted -- by default the behaviour remained the same.
Some architectures have picked it up; some haven't.

The main reason it's a bit of a pain to turn the correct
handling on is because often boards don't actually implement
all the devices they're supposed to. For a pile of legacy Arm
boards, especially where we didn't have good test images,
we use the machine flag ignore_memory_transaction_failures to
retain the legacy behaviour. (This isn't great because it's
pretty much going to mean we have that flag set on those
boards forever because nobody is going to care enough to
investigate and test.)


Other question is, sometimes I guess it's nice to avoid crashing in
order to try to quickly get past some unimplemented MMIO. Maybe a
command line option or something could turn it off? It should
probably be a QEMU-wide option if so, so that shouldn't hold this
series up, I can propose a option for that if anybody is worried
about it.


I would not recommend going any further than maybe setting the
ignore_memory_transaction_failures flag for boards you don't
care about. (But in an ideal world, don't set it and deal with
any bug reports by implementing stub versions of missing devices.
Depends how confident you are in your test coverage.)


It seems it broke the "mac99" and  powernv10 machines, using the
qemu-ppc-boot images which are mostly buildroot. See below for logs.

Adding Mark for further testing on Mac OS.



Mac OS 9.2 fails to boot with a popup saying :
     Sorry, a system error occured.
     "Sound Manager"
   address error
     To temporarily turn off extensions, restart and
     hold down the shift key


Darwin and Mac OSX look OK.


My guess would be that MacOS 9.2 is trying to access the sound chip registers which 
isn't implemented in QEMU for the moment (I have a separate screamer branch 
available, but it's not ready for primetime yet). In theory they shouldn't be 
accessed at all because the sound device isn't present in the OpenBIOS device tree, 
but this is all fairly old stuff.


Does implementing the sound registers using a dummy device help at all?


diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 265c0bbd8d..e55f938da7 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "hw/misc/unimp.h"
 #include "hw/misc/macio/cuda.h"
 #include "hw/pci/pci.h"
 #include "hw/ppc/mac_dbdma.h"
@@ -94,6 +95,7 @@ static bool macio_common_realize(PCIDevice *d, Error **errp)
 {
 MacIOState *s = MACIO(d);
 SysBusDevice *sbd;
+DeviceState *dev;

 if (!qdev_realize(DEVICE(&s->dbdma), BUS(&s->macio_bus), errp)) {
 return false;
@@ -102,6 +104,14 @@ static bool macio_common_realize(PCIDevice *d, Error 
**errp)
 memory_region_add_subregion(&s->bar, 0x08000,
 sysbus_mmio_get_region(sbd, 0));

+dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
+qdev_prop_set_string(dev, "name", "screamer");
+qdev_prop_set_uint64(dev, "size", 0x1000);
+sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+sbd = SYS_BUS_DEVICE(dev);
+memory_region_add_subregion(&s->bar, 0x14000,
+sysbus_mmio_get_region(sbd, 0));
+
 qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
 qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
 qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
index 86df2c2b60..1894178a68 100644
--- a/include/hw/misc/macio/macio.h
+++ b/include/hw/misc/macio/macio.h
@@ -109,6 +109,7 @@ struct MacIOState {
 PMUState pmu;
 DBDMAState dbdma;
 ESCCState escc;
+MemoryRegion screamer;
 uint64_t frequency;
 };



ATB,

Mark.

Re: [PATCH v2 2/4] pc-bios/s390-ccw: Provide space for initial stack frame in start.S

[Marc Hartmayer]

Thomas Huth  writes:

> Providing the space of a stack frame is the duty of the caller,
> so we should reserve 160 bytes before jumping into the main function.
> Otherwise the main() function might write past the stack array.
>
> While we're at it, add a proper STACK_SIZE macro for the stack size
> instead of using magic numbers (this is also required for the following
> patch).
>
> Reviewed-by: Christian Borntraeger 
> Reviewed-by: Cédric Le Goater 
> Signed-off-by: Thomas Huth 
> ---
>  pc-bios/s390-ccw/start.S | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index d29de09cc6..29b0a9ece0 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -10,10 +10,12 @@
>   * directory.
>   */
>  
> +#define STACK_SIZE 0x8000
> +
>  .globl _start
>  _start:
>  
> -larl%r15,stack + 0x8000 /* Set up stack */
> +larl%r15,stack + STACK_SIZE - 160   /* Set up stack */
 ^^^
 You can also add a macro for this
 - e.g. STACK_FRAME_SIZE.

Besides that,
Reviewed-by: Marc Hartmayer 

>  
>  /* clear bss */
>  larl%r2,__bss_start
> -- 
> 2.39.3
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 1/7] target/i386: Add FEAT_7_1_EDX to adjust feature level

[Igor Mammedov]

On Tue, 27 Jun 2023 12:27:19 +0800
Tao Su  wrote:

> On Mon, Jun 26, 2023 at 02:39:15PM +0200, Igor Mammedov wrote:
> > On Fri, 16 Jun 2023 11:23:05 +0800
> > Tao Su  wrote:
> >   
> > > Considering the case of FEAT_7_1_EAX being 0 and FEAT_7_1_EDX being
> > > non-zero,  
> > Can you clarify when/why that happens?  
> 
> When start a VM on GraniteRapids using '-cpu host', we can see two leafs 
> CPUID_7_0
> and CPUID_7_1 in VM, because both CPUID_7_1_EAX and CPUID_7_1_EDX have 
> non-zero value:
> 0x0007 0x01: eax=0x00201c30 edx=0x4000
> 
> But if we minus all FEAT_7_1_EAX features using
> '-cpu host,-avx-vnni,-avx512-bf16,-fzrm,-fsrs,-fsrc,-amx-fp16', we can't get 
> CPUID_7_1
> leaf even though CPUID_7_1_EDX has non-zero value, so it is necessary to 
> update
> cpuid_level_func7 by CPUID_7_1_EDX.

Pls, explain that in commit message.
 
> Thanks,
> Tao
> 
> >   
> > > guest may report wrong maximum number sub-leaves in leaf
> > > 07H. So add FEAT_7_1_EDX to adjust feature level.
> > > 
> > > Fixes: eaaa197d5b11 ("target/i386: Add support for AVX-VNNI-INT8 in CPUID
> > > enumeration")
> > > 
> > > Signed-off-by: Tao Su 
> > > Reviewed-by: Xiaoyao Li 
> > > ---
> > >  target/i386/cpu.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 1242bd541a..e8a70c35d2 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -6778,6 +6778,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error 
> > > **errp)
> > >  x86_cpu_adjust_feat_level(cpu, FEAT_6_EAX);
> > >  x86_cpu_adjust_feat_level(cpu, FEAT_7_0_ECX);
> > >  x86_cpu_adjust_feat_level(cpu, FEAT_7_1_EAX);
> > > +x86_cpu_adjust_feat_level(cpu, FEAT_7_1_EDX);
> > >  x86_cpu_adjust_feat_level(cpu, FEAT_8000_0001_EDX);
> > >  x86_cpu_adjust_feat_level(cpu, FEAT_8000_0001_ECX);
> > >  x86_cpu_adjust_feat_level(cpu, FEAT_8000_0007_EDX);  
> > 
> >   
> 

Re: [PATCH 5/7] target/i386: Add few security fix bits in ARCH_CAPABILITIES into SapphireRapids CPU model

[Igor Mammedov]

On Tue, 27 Jun 2023 14:10:17 +0800
Xiaoyao Li  wrote:

> On 6/26/2023 9:15 PM, Igor Mammedov wrote:
> > On Fri, 16 Jun 2023 11:23:09 +0800
> > Tao Su  wrote:
> >   
> >> From: Lei Wang 
> >>
> >> Latest stepping (8) of SapphireRapids has bit 13, 14 and 15 of
> >> MSR_IA32_ARCH_CAPABILITIES enabled, which are related to some security
> >> fixes.
> >>
> >> Add version 2 of SapphireRapids CPU model with those bits enabled also.  
> > 
> > don't we need to update stepping value to 8 as well?  
> 
> No need.
> 
> The commit message is misleading. There 3 bits and some other bits in 
> MSR_IA32_ARCH_CAPABILITIES are not tied to CPU stepping. Instead, they 
> are enumerated with newer microcode.

It that case fix commit message please.

> 
> >>
> >> Signed-off-by: Lei Wang 
> >> Signed-off-by: Tao Su 
> >> ---
> >>   target/i386/cpu.c | 13 +++--
> >>   1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> >> index b5321240c6..f84fd20bb1 100644
> >> --- a/target/i386/cpu.c
> >> +++ b/target/i386/cpu.c
> >> @@ -3854,8 +3854,17 @@ static const X86CPUDefinition builtin_x86_defs[] = {
> >>   .model_id = "Intel Xeon Processor (SapphireRapids)",
> >>   .versions = (X86CPUVersionDefinition[]) {
> >>   { .version = 1 },
> >> -{ /* end of list */ },
> >> -},
> >> +{
> >> +.version = 2,
> >> +.props = (PropValue[]) {
> >> +{ "sbdr-ssdp-no", "on" },
> >> +{ "fbsdp-no", "on" },
> >> +{ "psdp-no", "on" },
> >> +{ /* end of list */ }
> >> +}
> >> +},
> >> +{ /* end of list */ }
> >> +}
> >>   },
> >>   {
> >>   .name = "Denverton",  
> >   
> 

Re: [PATCH 0/2] i386: Fix Hyper-V Gen1 guests stuck on boot with 'hv-passthrough'

[Vitaly Kuznetsov]

Vitaly Kuznetsov  writes:

> Hyper-V Gen1 guests are getting stuck on boot when 'hv-passthrough' is
> used. While 'hv-passthrough' is a debug only feature, this significantly
> limit its usefullness. While debugging the problem, I found that there are
> two loosely connected issues:
> - 'hv-passthrough' enables 'hv-syndbg' and this is undesired.
> - 'hv-syndbg's support by KVM is detected incorrectly when !CONFIG_SYNDBG.
>
> Fix both issues; exclude 'hv-syndbg' from 'hv-passthrough' and don't allow
> to turn on 'hv-syndbg' for !CONFIG_SYNDBG builds. 
>
> Vitaly Kuznetsov (2):
>   i386: Fix conditional CONFIG_SYNDBG enablement
>   i386: Exclude 'hv-syndbg' from 'hv-passthrough'
>
>  docs/system/i386/hyperv.rst | 13 +
>  target/i386/cpu.c   |  2 ++
>  target/i386/kvm/kvm.c   | 18 --
>  3 files changed, 23 insertions(+), 10 deletions(-)

Ping)

-- 
Vitaly

Re: [PATCH 6/7] target/i386: Add new CPU model EmeraldRapids

[Igor Mammedov]

On Tue, 27 Jun 2023 13:54:23 +0800
Xiaoyao Li  wrote:

> On 6/26/2023 8:56 PM, Igor Mammedov wrote:
> > On Fri, 16 Jun 2023 11:23:10 +0800
> > Tao Su  wrote:
> >   
> >> From: Qian Wen
> >>
> >> Emerald Rapids (EMR) is the next generation of Xeon server processor
> >> after Sapphire Rapids (SPR).
> >>
> >> Currently, regarding the feature set that can be exposed to guest, there
> >> isn't any one new comparing with SPR cpu model, except that EMR has a
> >> different model number.
> >>
> >> Though it's practicable to define EMR as an alias of a new version of
> >> SPR by only updating the model number and model name, it loses the
> >> flexibility when new version of EMR cpu model are needed for adding new
> >> features (that hasn't virtalized/supported by KVM yet).  
> > Which begs a question, why do we need EMR model (or alias) at all
> > if it's the same as SPR at the moment.
> > 
> > Make new features supported 1st and only then introduce a new CPU model.
> >   
> 
> Even if no new feature (that can be virtualized and exposed to guest) in 
> EMR compared to SPR in the end, I think it still makes sense to provide 
> a dedicated EMR CPU model in QEMU. Because
> 1) User will know EMR, Intel's next generation of Xeon after SRP, is 
> supported by QEMU, via -cpu ?/ -cpu help;

I don't see any benefits in misleading user by showing EMR model which is
nothing else but SPR one.
On negative side you would increase maintenance burden by introducing
extra versions of CPU model later. Which by itself is abusing versioning,
mainly used for fixing CPU bugs, by using it for adding new features.

> 2) It's convenient for user to create an EMR VM. People may not care 
> that much what the difference between "-cpu SapphireRapids" with "-cpu 
> EmeraldRapids", while they do want to create an VM which shows the CPU 
> is EmeraldRapids.
>
My guess would be is that you need guest to show EMR for developing EMR
features/guest bringup, in that case do it in your fork, and once
support is actually ready publish completed patches for it.

To exaggerate you reasoning further, we should create CPU models for
all future planned Intel/AMD CPU as a one of currently existing in
QEMU right now and then sometime in future implement features that
actually make those models what they should be.

It's downright confusing for user, so I'd object to this approach.

Re: [PATCH] vdpa: Increase out buffer size for CVQ commands

[Hawkins Jiawei]

On 2023/6/26 17:08, Eugenio Perez Martin wrote:
> On Mon, Jun 26, 2023 at 10:26 AM Hawkins Jiawei  wrote:
>>
>> On 2023/6/25 18:48, Eugenio Perez Martin wrote:
>>> On Thu, Jun 22, 2023 at 3:07 AM Hawkins Jiawei  wrote:

 According to the VirtIO standard, "Since there are no guarantees,
 it can use a hash filter or silently switch to
 allmulti or promiscuous mode if it is given too many addresses."
 To achive this, QEMU ignores MAC addresses and marks `mac_table.x_overflow`
 in the device internal state in virtio_net_handle_mac()
 if the guest sets more than `MAC_TABLE_ENTRIES` MAC addresses
 for the filter table.

 However, the problem is that QEMU never marks the `mac_table.x_overflow`
 for the vdpa device internal state when the guest sets more than
 `MAC_TABLE_ENTRIES` MAC addresses.

 To be more specific, currently QEMU offers a buffer size of
 vhost_vdpa_net_cvq_cmd_len() for CVQ commands, which represents the size of
 VIRTIO_NET_CTRL_MAC_TABLE_SET command with a maximum `MAC_TABLE_ENTRIES`
 MAC addresses.

 Consequently, if the guest sets more than `MAC_TABLE_ENTRIES` MAC 
 addresses,
 QEMU truncates the CVQ command data and copies this incomplete command
 into the out buffer. In this situation, virtio_net_handle_mac() fails the
 integrity check and returns VIRTIO_NET_ERR instead of marking
 `mac_table.x_overflow` and returning VIRTIO_NET_OK, since the copied
 CVQ command in the buffer is incomplete and flawed.

 This patch solves this problem by increasing the buffer size to
 vhost_vdpa_net_cvq_cmd_page_len(), which represents the size of the buffer
 that is allocated and mmaped. Therefore, everything should work correctly
 as long as the guest sets fewer than `(vhost_vdpa_net_cvq_cmd_page_len() -
 sizeof(struct virtio_net_ctrl_hdr)
 - 2 * sizeof(struct virtio_net_ctrl_mac)) / ETH_ALEN` MAC addresses.

 Considering the highly unlikely scenario for the guest setting more than
 that number of MAC addresses for the filter table, this patch should
 work fine for the majority of cases. If there is a need for more than thoes
 entries, we can increase the value for vhost_vdpa_net_cvq_cmd_page_len()
 in the future, mapping more than one page for command output.

 Fixes: 7a7f87e94c ("vdpa: Move command buffers map to start of net device")
 Signed-off-by: Hawkins Jiawei 
 ---
net/vhost-vdpa.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)

 diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
 index 5a72204899..ecfa8852b5 100644
 --- a/net/vhost-vdpa.c
 +++ b/net/vhost-vdpa.c
 @@ -784,9 +784,18 @@ static int 
 vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
};
ssize_t dev_written = -EINVAL;

 +/*
 + * This code truncates the VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ command
 + * and prevents QEMU from marking `mac_table.x_overflow` in the device
 + * internal state in virtio_net_handle_mac() if the guest sets more 
 than
 + * `(vhost_vdpa_net_cvq_cmd_page_len() - sizeof(struct 
 virtio_net_ctrl_hdr)
 + * - 2 * sizeof(struct virtio_net_ctrl_mac)) / ETH_ALEN` MAC 
 addresses for
 + * filter table.
 + * However, this situation is considered rare, so it is acceptable.
>>>
>>> I think we can also fix this situation. If it fits in one page, we can
>>> still send the same page to the device and virtio_net_handle_ctrl_iov.
>>> If it does not fit in one page, we can clear all mac filters with
>>> VIRTIO_NET_CTRL_RX_ALLUNI and / or VIRTIO_NET_CTRL_RX_ALLMULTI.
>>
>> Hi Eugenio,
>>
>> Thank you for your review.
>>
>> However, it appears that the approach may not resolve the situation.
>>
>> When the CVQ command exceeds one page,
>> vhost_vdpa_net_handle_ctrl_avail() truncates the command, resulting in a
>> malformed SVQ command being received by the hardware, which in turn
>> triggers an error acknowledgement to QEMU.
>>
>
> If that happens we can sanitize the copied cmd buffer. Let's start
> with an overflowed unicast table first.
>
> To configure the device we need to transform the command to
> VIRTIO_NET_CTRL_RX_ALLUNI, as we cannot copy all the table to the out
> cmd page. If that succeeds, we can continue to register that in the
> VirtIONet struct.
>
>   We can copy the first MAC_TABLE_ENTRIES + 1 entries and set entries =
> (MAC_TABLE_ENTRIES + 1), and then use that buffer to call
> virtio_net_handle_ctrl_iov. That sets VirtIONet.uni_overflow = true
> and VirtIONet.all_uni = false.
>
> We need to handle the multicast addresses in a similar way, but we can
> find cases where only multicast addresses overflow.
>
> In future series we can improve the situation:
> * Allocating bigger out buffers so more mac addresses fit in it.
> * Mapping also guest pages in CVQ, so the real device is able

Re: [PATCH v2 08/26] tests/qtests: clean-up and fix leak in generic_fuzz

[Philippe Mathieu-Daudé]

On 27/6/23 09:43, Juan Quintela wrote:

Alex Bennée  wrote:

An update to the clang tooling detects more issues with the code
including a memory leak from the g_string_new() allocation. Clean up
the code with g_autoptr and use ARRAY_SIZE while we are at it.

Signed-off-by: Alex Bennée 
---
  tests/qtest/fuzz/generic_fuzz.c | 11 ---
  1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index c525d22951..a4841181cc 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -954,17 +954,14 @@ static void register_generic_fuzz_targets(void)
  .crossover = generic_fuzz_crossover
  });
  
-GString *name;

+g_autoptr(GString) name = g_string_new("");
  const generic_fuzz_config *config;
  
-for (int i = 0;

- i < sizeof(predefined_configs) / sizeof(generic_fuzz_config);
- i++) {
+for (int i = 0; i < ARRAY_SIZE(predefined_configs); i++) {
  config = predefined_configs + i;
-name = g_string_new("generic-fuzz");
-g_string_append_printf(name, "-%s", config->name);
+g_string_printf(name, "generic-fuzz-%s", config->name);
  fuzz_add_target(&(FuzzTarget){
-.name = name->str,
+.name = g_strdup(name->str),
  .description = "Predefined generic-fuzz config.",
  .get_init_cmdline = generic_fuzz_predefined_config_cmdline,
  .pre_fuzz = generic_pre_fuzz,


Once that you are here, what about?
(Yes, I didn't care about the ARRAY_SIZE) but you got the idea.

Reviewed-by: Juan Quintela 

To your proposal with/without the change that I proposse.

modified   tests/qtest/fuzz/generic_fuzz.c
@@ -954,17 +954,14 @@ static void register_generic_fuzz_targets(void)
  .crossover = generic_fuzz_crossover
  });
  
-GString *name;

  const generic_fuzz_config *config;
  
  for (int i = 0;

   i < sizeof(predefined_configs) / sizeof(generic_fuzz_config);
   i++) {
  config = predefined_configs + i;
-name = g_string_new("generic-fuzz");
-g_string_append_printf(name, "-%s", config->name);
  fuzz_add_target(&(FuzzTarget){
-.name = name->str,
+.name = g_strdup_printf("generic-fuzz-%s", config->name),


Even simpler is g_strconcat() suggested by Richard in v1:
https://lore.kernel.org/qemu-devel/42b497a0-e234-64db-e845-1c37b6783...@linaro.org/

-- >8 --
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -956,13 +956,8 @@ static void register_generic_fuzz_targets(void)

-GString *name;
 const generic_fuzz_config *config;

-for (int i = 0;
- i < sizeof(predefined_configs) / sizeof(generic_fuzz_config);
- i++) {
-config = predefined_configs + i;
-name = g_string_new("generic-fuzz");
-g_string_append_printf(name, "-%s", config->name);
+for (int i = 0; i < ARRAY_SIZE(predefined_configs); i++) {
+config = &predefined_configs[i];
 fuzz_add_target(&(FuzzTarget){
-.name = name->str,
+.name = g_strconcat("generic-fuzz-", config->name, NULL),
 .description = "Predefined generic-fuzz config.",
---

Re: [RESEND PATCH v5 4/5] tests/qtest/hd-geo-test: fix incorrect pcie-root-port usage and simplify test

[Igor Mammedov]

On Mon, 26 Jun 2023 21:42:43 +0530
Ani Sinha  wrote:

> The test attaches a SCSI controller to a non-zero slot and a pcie-to-pci 
> bridge
> on slot 0 on the same pcie-root-port. Since a downstream device can be 
> attached
> to a pcie-root-port only on slot 0, the above test configuration is not 
> allowed.

> Additionally using pcie.0 as id for pcie-root-port is incorrect as that id is
   shouldn't it be pcie-to-pci ?
> reserved only for the root bus.

> 
> In the test scenario, there is no need to attach a pcie-root-port to the
> root complex. A SCSI controller can be attached to a pcie-to-pci bridge
> which can then be directly attached to the root bus (pcie.0).
> 
> Fix the test and simplify it.
> 
> CC: m...@redhat.com
> CC: imamm...@redhat.com
> CC: Michael Labiuk 
> 
> Signed-off-by: Ani Sinha 
> ---
>  tests/qtest/hd-geo-test.c | 18 --
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c
> index 5aa258a2b3..d08bffad91 100644
> --- a/tests/qtest/hd-geo-test.c
> +++ b/tests/qtest/hd-geo-test.c
> @@ -784,14 +784,12 @@ static void test_override_scsi(void)
>  test_override(args, "pc", expected);
>  }
>  
> -static void setup_pci_bridge(TestArgs *args, const char *id, const char 
> *rootid)
> +static void setup_pci_bridge(TestArgs *args, const char *id)
>  {
>  
> -char *root, *br;
> -root = g_strdup_printf("-device pcie-root-port,id=%s", rootid);
> -br = g_strdup_printf("-device pcie-pci-bridge,bus=%s,id=%s", rootid, id);
> +char *br;
> +br = g_strdup_printf("-device pcie-pci-bridge,bus=pcie.0,id=%s", id);
>  
> -args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, root);
>  args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, br);
>  }
>  
> @@ -811,8 +809,8 @@ static void test_override_scsi_q35(void)
>  add_drive_with_mbr(args, empty_mbr, 1);
>  add_drive_with_mbr(args, empty_mbr, 1);
>  add_drive_with_mbr(args, empty_mbr, 1);
> -setup_pci_bridge(args, "pcie.0", "br");
> -add_scsi_controller(args, "lsi53c895a", "br", 3);
> +setup_pci_bridge(args, "pcie-pci-br");
> +add_scsi_controller(args, "lsi53c895a", "pcie-pci-br", 3);
>  add_scsi_disk(args, 0, 0, 0, 0, 0, 1, 120, 30);
>  add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30);
>  add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0);
> @@ -868,9 +866,9 @@ static void test_override_virtio_blk_q35(void)
>  };
>  add_drive_with_mbr(args, empty_mbr, 1);
>  add_drive_with_mbr(args, empty_mbr, 1);
> -setup_pci_bridge(args, "pcie.0", "br");
> -add_virtio_disk(args, 0, "br", 3, 1, 120, 30);
> -add_virtio_disk(args, 1, "br", 4, 9000, 120, 30);
> +setup_pci_bridge(args, "pcie-pci-br");
> +add_virtio_disk(args, 0, "pcie-pci-br", 3, 1, 120, 30);
> +add_virtio_disk(args, 1, "pcie-pci-br", 4, 9000, 120, 30);
>  test_override(args, "q35", expected);
>  }
>  

Re: [PATCH] net: add initial support for AF_XDP network backend

[Stefan Hajnoczi]

Can multiple VMs share a host netdev by filtering incoming traffic
based on each VM's MAC address and directing it to the appropriate
XSK? If yes, then I think AF_XDP is interesting when SR-IOV or similar
hardware features are not available.

The idea of an AF_XDP passthrough device seems interesting because it
would minimize the overhead and avoid some of the existing software
limitations (mostly in QEMU's networking subsystem) that you
described. I don't know whether the AF_XDP API is suitable or can be
extended to build a hardware emulation interface, but it seems
plausible. When Stefano Garzarella played with io_uring passthrough
into the guest, one of the issues was guest memory translation (since
the guest doesn't use host userspace virtual addresses). I guess
AF_XDP would need an API for adding/removing memory translations or
operate in a mode where addresses are relative offsets from the start
of the umem regions (but this may be impractical if it limits where
the guest can allocate packet payload buffers).

Whether you pursue the passthrough approach or not, making -netdev
af-xdp work in an environment where QEMU runs unprivileged seems like
the most important practical issue to solve.

Stefan

Re: [PATCH v2 3/8] target/sparc: Drop inline markers from translate.c

[Philippe Mathieu-Daudé]

On 21/6/23 20:06, Richard Henderson wrote:

Let the compiler decide about inlining.


Should we clean that automatically with scripts and forbid via
checkpatch?

Reviewed-by: Philippe Mathieu-Daudé 


Signed-off-by: Richard Henderson 
---
  target/sparc/translate.c | 237 ++-
  1 file changed, 111 insertions(+), 126 deletions(-)

Re: [RESEND PATCH v5 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Igor Mammedov]

On Mon, 26 Jun 2023 21:42:44 +0530
Ani Sinha  wrote:

> PCI Express ports only have one slot, so PCI Express devices can only be
> plugged into slot 0 on a PCIE port. Enforce it.

btw, previously you mentioned ARI.
So if we turn it on, wouldn't this patch actually become regression?

> 
> CC: jus...@redhat.com
> CC: imamm...@redhat.com
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
> Signed-off-by: Ani Sinha 
> Reviewed-by: Julia Suvorova 
> ---
>  hw/pci/pci.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bf38905b7d..426af133b0 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -64,6 +64,7 @@ bool pci_available = true;
>  static char *pcibus_get_dev_path(DeviceState *dev);
>  static char *pcibus_get_fw_dev_path(DeviceState *dev);
>  static void pcibus_reset(BusState *qbus);
> +static bool pcie_has_upstream_port(PCIDevice *dev);
>  
>  static Property pci_props[] = {
>  DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> *pci_dev,
> name);
>  
> return NULL;
> +} else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> +error_setg(errp, "PCI: slot %d is not valid for %s,"
> +   " parent device only allows plugging into slot 0.",
> +   PCI_SLOT(devfn), name);
> +return NULL;
>  }
>  
>  pci_dev->devfn = devfn;

Re: [PATCH 6/6] tests/qtest: migration-test: Add tests for file-based migration

[Daniel P . Berrangé]

On Mon, Jun 26, 2023 at 03:22:10PM -0300, Fabiano Rosas wrote:
> From: Nikolay Borisov 
> 
> Add basic tests for file-based migration.
> 
> Signed-off-by: Nikolay Borisov 
> Signed-off-by: Fabiano Rosas 
> ---
>  tests/qtest/migration-test.c | 66 
>  1 file changed, 66 insertions(+)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index acb778a8cd..5a77257a53 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -763,6 +763,7 @@ static void test_migrate_end(QTestState *from, QTestState 
> *to, bool test_dest)
>  cleanup("migsocket");
>  cleanup("src_serial");
>  cleanup("dest_serial");
> +cleanup("migfile");
>  }
>  
>  #ifdef CONFIG_GNUTLS
> @@ -1460,11 +1461,28 @@ static void test_precopy_common(MigrateCommon *args)
>   */
>  wait_for_migration_complete(from);
>  
> +/*
> + * For file based migration the target must begin its
> + * migration after the source has finished.
> + */
> +if (strstr(connect_uri, "file:")) {
> +migrate_incoming_qmp(to, connect_uri, "{}");
> +}
> +
>  if (!got_src_stop) {
>  qtest_qmp_eventwait(from, "STOP");
>  }
>  } else {
>  wait_for_migration_complete(from);
> +
> +/*
> + * For file based migration the target must begin its
> + * migration after the source has finished.
> + */
> +if (strstr(connect_uri, "file:")) {
> +migrate_incoming_qmp(to, connect_uri, "{}");
> +}
> +
>  /*
>   * Must wait for dst to finish reading all incoming
>   * data on the socket before issuing 'cont' otherwise
> @@ -1682,6 +1700,46 @@ static void test_precopy_unix_compress_nowait(void)
>  test_precopy_common(&args);
>  }
>  
> +static void test_precopy_file(void)
> +{
> +g_autofree char *uri = g_strdup_printf("file:%s/migfile", tmpfs);

There's no unlink(), so I presume you're relying on the entire 'tmpfs'
being recursively deleted ?

> +MigrateCommon args = {
> +.connect_uri = uri,
> +.listen_uri = "defer",
> +};
> +
> +test_precopy_common(&args);
> +}
> +
> +static void test_precopy_file_offset(void)
> +{
> +g_autofree char *uri = g_strdup_printf("file:%s/migfile,offset=0x1000",
> +   tmpfs);
> +MigrateCommon args = {
> +.connect_uri = uri,
> +.listen_uri = "defer",
> +};
> +
> +test_precopy_common(&args);

There ought to be something here that values 0->0x1000 bytes are
all zeroes in the file.


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v2 4/8] target/sparc: Introduce DYNAMIC_PC_LOOKUP

[Philippe Mathieu-Daudé]

On 21/6/23 20:06, Richard Henderson wrote:

Create a new artificial "next pc" which also indicates
that nothing has changed within the cpu state which
requires returning to the main loop.

Pipe this new value though all pc/npc checks.
Do not produce this new value yet.

Signed-off-by: Richard Henderson 
---
  target/sparc/translate.c | 155 ---
  1 file changed, 111 insertions(+), 44 deletions(-)




@@ -5608,20 +5649,46 @@ static void sparc_tr_translate_insn(DisasContextBase 
*dcbase, CPUState *cs)
  static void sparc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
  {
  DisasContext *dc = container_of(dcbase, DisasContext, base);
+bool may_lookup;
  
  switch (dc->base.is_jmp) {

  case DISAS_NEXT:
  case DISAS_TOO_MANY:
-if (dc->pc != DYNAMIC_PC &&
-(dc->npc != DYNAMIC_PC && dc->npc != JUMP_PC)) {
+if (((dc->pc | dc->npc) & 3) == 0) {
  /* static PC and NPC: we can use direct chaining */
  gen_goto_tb(dc, 0, dc->pc, dc->npc);
-} else {
-if (dc->pc != DYNAMIC_PC) {
-tcg_gen_movi_tl(cpu_pc, dc->pc);
+break;
+}
+
+if (dc->pc & 3) {
+switch (dc->pc) {
+case DYNAMIC_PC_LOOKUP:
+may_lookup = true;
+break;
+case DYNAMIC_PC:
+may_lookup = false;
+break;
+default:
+g_assert_not_reached();
  }
-save_npc(dc);
+} else {
+tcg_gen_movi_tl(cpu_pc, dc->pc);
+may_lookup = true;
+}
+
+save_npc(dc);
+switch (dc->npc) {


switch (dc->npc & 3)?


+case DYNAMIC_PC_LOOKUP:
+if (may_lookup) {
+tcg_gen_lookup_and_goto_ptr();
+break;
+}
+/* fall through */
+case DYNAMIC_PC:
  tcg_gen_exit_tb(NULL, 0);
+break;
+default:
+g_assert_not_reached();
  }
  break;
  

Re: [PATCH v2 5/8] target/sparc: Use DYNAMIC_PC_LOOKUP for conditional branches

[Philippe Mathieu-Daudé]

On 21/6/23 20:06, Richard Henderson wrote:

When resolving JUMP_PC, we know this is for a plain branch
with no other side effects.

Signed-off-by: Richard Henderson 
---
  target/sparc/translate.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 7/8] target/sparc: Use DYNAMIC_PC_LOOKUP for v9 RETURN

[Philippe Mathieu-Daudé]

On 21/6/23 20:06, Richard Henderson wrote:

After the register window unwind, this is for a plain indirect
branch with no further side effects.

Signed-off-by: Richard Henderson 
---
  target/sparc/translate.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 1/6] migration: Set migration status early in incoming side

[Juan Quintela]

Fabiano Rosas  wrote:
> We are sending a migration event of MIGRATION_STATUS_SETUP at
> qemu_start_incoming_migration but never actually setting the state.
>
> This creates a window between qmp_migrate_incoming and
> process_incoming_migration_co where the migration status is still
> MIGRATION_STATUS_NONE. Calling query-migrate during this time will
> return an empty response even though the incoming migration command
> has already been issued.
>
> Commit 7cf1fe6d68 ("migration: Add migration events on target side")
> has added support to the 'events' capability to the incoming part of
> migration, but chose to send the SETUP event without setting the
> state. I'm assuming this was a mistake.
>
> To avoid introducing a change in behavior, we need to keep sending the
> SETUP event, even if the 'events' capability is not set.
>
> Signed-off-by: Fabiano Rosas 
> ---
>  migration/migration.c | 17 +++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 7c8292d4d4..562b78261d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -424,13 +424,26 @@ void migrate_add_address(SocketAddress *address)
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>  const char *p = NULL;
> +MigrationIncomingState *mis = migration_incoming_get_current();
>  
>  /* URI is not suitable for migration? */
>  if (!migration_channels_and_uri_compatible(uri, errp)) {
>  return;
>  }
>  
> -qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> +migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
> +  MIGRATION_STATUS_SETUP);
> +/*
> + * QMP clients should have set the 'events' migration capability
> + * if they want to receive this event, in which case the
> + * migrate_set_state() call above will have already sent the
> + * event. We still need to send the event for compatibility even
> + * if migration events are disabled.
> + */
> +if (!migrate_events()) {
> +qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> +}

Can we add and test for a property here, so we can drop this at some
point in the future?

> +
>  if (strstart(uri, "tcp:", &p) ||
>  strstart(uri, "unix:", NULL) ||
>  strstart(uri, "vsock:", NULL)) {
> @@ -524,7 +537,7 @@ process_incoming_migration_co(void *opaque)
>  
>  mis->largest_page_size = qemu_ram_pagesize_largest();
>  postcopy_state_set(POSTCOPY_INCOMING_NONE);
> -migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
> +migrate_set_state(&mis->state, MIGRATION_STATUS_SETUP,
>MIGRATION_STATUS_ACTIVE);
>  
>  mis->loadvm_co = qemu_coroutine_self();

Re: [PATCH v2 2/8] target/sparc: Fix npc comparison in sparc_tr_insn_start

[Philippe Mathieu-Daudé]

On 21/6/23 20:06, Richard Henderson wrote:

During translation, npc == address, DYNAMIC_PC, or JUMP_PC.
It is only the encoding between here and sparc_restore_state_to_opc
that considers JUMP_PC to be a bit within a larger value.

Signed-off-by: Richard Henderson 
---
  target/sparc/translate.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 6/8] target/sparc: Use DYNAMIC_PC_LOOKUP for JMPL

[Philippe Mathieu-Daudé]

On 21/6/23 20:06, Richard Henderson wrote:

This is for a plain indirect branch with no other side effects.

Signed-off-by: Richard Henderson 
---
  target/sparc/translate.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PULL 00/30] Next patches

[Juan Quintela]

Richard Henderson  wrote:
> On 6/22/23 18:54, Juan Quintela wrote:
>> The following changes since commit b455ce4c2f300c8ba47cba7232dd03261368a4cb:
>>Merge tag 'q800-for-8.1-pull-request'
>> ofhttps://github.com/vivier/qemu-m68k  into staging (2023-06-22
>> 10:18:32 +0200)
>> are available in the Git repository at:
>>https://gitlab.com/juan.quintela/qemu.git  tags/next-pull-request
>> for you to fetch changes up to
>> 23e4307eadc1497bd0a11ca91041768f15963b68:
>>migration/rdma: Split qemu_fopen_rdma() into input/output
>> functions (2023-06-22 18:11:58 +0200)
>> 
>> Migration Pull request (20230621) take 2
>> In this pull request the only change is fixing 32 bits complitaion
>> issue.
>> Please apply.
>> [take 1]
>> - fix for multifd thread creation (fabiano)
>> - dirtylimity (hyman)
>>* migration-test will go on next PULL request, as it has failures.
>> - Improve error description (tejus)
>> - improve -incoming and set parameters before calling incoming (wei)
>> - migration atomic counters reviewed patches (quintela)
>> - migration-test refacttoring reviewed (quintela)
>
> New failure with check-cfi-x86_64:
>
> https://gitlab.com/qemu-project/qemu/-/jobs/4527202764#L188
>
> /builds/qemu-project/qemu/build/pyvenv/bin/meson test  --no-rebuild -t
> 0  --num-processes 1 --print-errorlogs
>   1/350 qemu:qtest+qtest-x86_64 / qtest-x86_64/qom-test
>   OK 6.55s   8 subtests passed
> ▶   2/350 ERROR:../tests/qtest/migration-test.c:320:check_guests_ram:
> assertion failed: (bad == 0) ERROR
>   2/350 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test
>   ERROR 151.99s   killed by signal 6 SIGABRT

> G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.sh
> MALLOC_PERTURB_=3 QTEST_QEMU_IMG=./qemu-img
> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
> QTEST_QEMU_BINARY=./qemu-system-x86_64
> /builds/qemu-project/qemu/build/tests/qtest/migration-test --tap
>-k
> ― ✀  ―
> stderr:
> qemu-system-x86_64: Unable to read from socket: Connection reset by peer
> Memory content inconsistency at 4f65000 first_byte = 30 last_byte = 2f
> current = 88 hit_edge = 1
> **
> ERROR:../tests/qtest/migration-test.c:320:check_guests_ram: assertion failed: 
> (bad == 0)
>
> (test program exited with status code -6)
> ――

Still running in bisect mode (this takes forever).

[cc'ing stefan and kevin]

And now I get this problem with gcov:

https://gitlab.com/juan.quintela/qemu/-/jobs/4546094720

357/423 qemu:block / io-qcow2-copy-before-write
ERROR   6.23s   exit status 1
>>> PYTHON=/builds/juan.quintela/qemu/build/pyvenv/bin/python3 
>>> MALLOC_PERTURB_=154 /builds/juan.quintela/qemu/build/pyvenv/bin/python3 
>>> /builds/juan.quintela/qemu/build/../tests/qemu-iotests/check -tap -qcow2 
>>> copy-before-write --source-dir 
>>> /builds/juan.quintela/qemu/tests/qemu-iotests --build-dir 
>>> /builds/juan.quintela/qemu/build/tests/qemu-iotests
― ✀  ―
stderr:
--- /builds/juan.quintela/qemu/tests/qemu-iotests/tests/copy-before-write.out
+++ 
/builds/juan.quintela/qemu/build/scratch/qcow2-file-copy-before-write/copy-before-write.out.bad
@@ -1,5 +1,21 @@
-
+...F
+==
+FAIL: test_timeout_break_snapshot (__main__.TestCbwError)
+--
+Traceback (most recent call last):
+  File 
"/builds/juan.quintela/qemu/tests/qemu-iotests/tests/copy-before-write", line 
210, in test_timeout_break_snapshot
+self.assertEqual(log, """\
+AssertionError: 'wrot[195 chars]read 1048576/1048576 bytes at offset 0\n1 
MiB,[46 chars]c)\n' != 'wrot[195 chars]read failed: Permission denied\n'
+  wrote 524288/524288 bytes at offset 0
+  512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+  wrote 524288/524288 bytes at offset 524288
+  512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
++ read failed: Permission denied
+- read 1048576/1048576 bytes at offset 0
+- 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
 --
 Ran 4 tests
-OK
+FAILED (failures=1)
(test program exited with status code 1)
――

I have no clue how I can make qtests to fail with my changes.

Specially with a read permission.

Any clue?

Later, Juan.

PD. Yeap, continuing the bisect.

Re: [PATCH 2/6] tests/qtest: migration: Expose migrate_set_capability

[Juan Quintela]

Fabiano Rosas  wrote:
> The following patch will make use of this function from within
> migrate-helpers.c, so move it there.
>
> Signed-off-by: Fabiano Rosas 

Reviewed-by: Juan Quintela 

Re: [PATCH v2 2/4] pc-bios/s390-ccw: Provide space for initial stack frame in start.S

[Philippe Mathieu-Daudé]

On 27/6/23 10:26, Marc Hartmayer wrote:

Thomas Huth  writes:


Providing the space of a stack frame is the duty of the caller,
so we should reserve 160 bytes before jumping into the main function.
Otherwise the main() function might write past the stack array.

While we're at it, add a proper STACK_SIZE macro for the stack size
instead of using magic numbers (this is also required for the following
patch).

Reviewed-by: Christian Borntraeger 
Reviewed-by: Cédric Le Goater 
Signed-off-by: Thomas Huth 
---
  pc-bios/s390-ccw/start.S | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)




+#define STACK_SIZE 0x8000
+
  .globl _start
  _start:
  
-larl%r15,stack + 0x8000 /* Set up stack */

+larl%r15,stack + STACK_SIZE - 160   /* Set up stack */

  ^^^
  You can also add a macro for this
  - e.g. STACK_FRAME_SIZE.


Yes please :) No need to respin.

Reviewed-by: Philippe Mathieu-Daudé 


Besides that,
Reviewed-by: Marc Hartmayer 

=

Re: [PATCH 4/6] tests/qtest: migration: Use migrate_incoming_qmp where appropriate

[Juan Quintela]

Fabiano Rosas  wrote:
> Use the new migrate_incoming_qmp helper in the places that currently
> open-code calling migrate-incoming.
>
> Signed-off-by: Fabiano Rosas 

Reviewed-by: Juan Quintela 

Re: [PATCH v2 1/4] pc-bios/s390-ccw: Fix indentation in start.S

[Philippe Mathieu-Daudé]

On 27/6/23 09:47, Thomas Huth wrote:

start.S is currently indented with a mixture of spaces and tabs, which
is quite ugly. QEMU coding style says indentation should be 4 spaces,
and this is also what we are using in the assembler files in the
tests/tcg/s390x/ folder already, so let's adjust start.S accordingly.

Reviewed-by: Cédric Le Goater 
Signed-off-by: Thomas Huth 
---
  pc-bios/s390-ccw/start.S | 136 +++
  1 file changed, 68 insertions(+), 68 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 09/26] tests/docker: add test-fuzz

[Alexander Bulekov]

On 230626 2259, Alex Bennée wrote:
> Running the fuzzer requires some hoop jumping and some problems only
> show up in containers. This basically replicates the build-oss-fuzz
> job from our CI so we can run in the same containers we use in CI.
> 
> Signed-off-by: Alex Bennée 

Reviewed-by: Alexander Bulekov 

Thanks

> ---
>  tests/docker/test-fuzz | 28 
>  1 file changed, 28 insertions(+)
>  create mode 100755 tests/docker/test-fuzz
> 
> diff --git a/tests/docker/test-fuzz b/tests/docker/test-fuzz
> new file mode 100755
> index 00..7e506ae1f6
> --- /dev/null
> +++ b/tests/docker/test-fuzz
> @@ -0,0 +1,28 @@
> +#!/bin/bash -e
> +#
> +# Compile and check with oss-fuzz.
> +#
> +# Copyright (c) 2023 Linaro Ltd.
> +#
> +# Authors:
> +#  Alex Bennée 
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +. common.rc
> +
> +requires_binary clang
> +
> +# the build script runs out of $src so we need to copy across
> +cd "$BUILD_DIR"
> +cp -a $QEMU_SRC .
> +cd src
> +mkdir build-oss-fuzz
> +export LSAN_OPTIONS=suppressions=scripts/oss-fuzz/lsan_suppressions.txt
> +env CC="clang" CXX="clang++" CFLAGS="-fsanitize=address" 
> ./scripts/oss-fuzz/build.sh
> +export ASAN_OPTIONS="fast_unwind_on_malloc=0"
> +for fuzzer in $(find ./build-oss-fuzz/DEST_DIR/ -executable -type f | grep 
> -v slirp); do
> +grep "LLVMFuzzerTestOneInput" ${fuzzer} > /dev/null 2>&1 || continue 
> ;
> +echo Testing ${fuzzer} ... ;
> +"${fuzzer}" -runs=1 -seed=1 || exit 1 ;
> +done
> -- 
> 2.39.2
> 

Re: [RESEND PATCH v5 4/5] tests/qtest/hd-geo-test: fix incorrect pcie-root-port usage and simplify test

[Ani Sinha]

> On 27-Jun-2023, at 2:24 PM, Igor Mammedov  wrote:
> 
> On Mon, 26 Jun 2023 21:42:43 +0530
> Ani Sinha  wrote:
> 
>> The test attaches a SCSI controller to a non-zero slot and a pcie-to-pci 
>> bridge
>> on slot 0 on the same pcie-root-port. Since a downstream device can be 
>> attached
>> to a pcie-root-port only on slot 0, the above test configuration is not 
>> allowed.
> 
>> Additionally using pcie.0 as id for pcie-root-port is incorrect as that id is
>   shouldn't it be pcie-to-pci ?

Yes you are right. Pcie-root-port is “br” in the test. Its so confusing!

>> reserved only for the root bus.
> 
>> 
>> In the test scenario, there is no need to attach a pcie-root-port to the
>> root complex. A SCSI controller can be attached to a pcie-to-pci bridge
>> which can then be directly attached to the root bus (pcie.0).
>> 
>> Fix the test and simplify it.
>> 
>> CC: m...@redhat.com
>> CC: imamm...@redhat.com
>> CC: Michael Labiuk 
>> 
>> Signed-off-by: Ani Sinha 
>> ---
>> tests/qtest/hd-geo-test.c | 18 --
>> 1 file changed, 8 insertions(+), 10 deletions(-)
>> 
>> diff --git a/tests/qtest/hd-geo-test.c b/tests/qtest/hd-geo-test.c
>> index 5aa258a2b3..d08bffad91 100644
>> --- a/tests/qtest/hd-geo-test.c
>> +++ b/tests/qtest/hd-geo-test.c
>> @@ -784,14 +784,12 @@ static void test_override_scsi(void)
>> test_override(args, "pc", expected);
>> }
>> 
>> -static void setup_pci_bridge(TestArgs *args, const char *id, const char 
>> *rootid)
>> +static void setup_pci_bridge(TestArgs *args, const char *id)
>> {
>> 
>> -char *root, *br;
>> -root = g_strdup_printf("-device pcie-root-port,id=%s", rootid);
>> -br = g_strdup_printf("-device pcie-pci-bridge,bus=%s,id=%s", rootid, 
>> id);
>> +char *br;
>> +br = g_strdup_printf("-device pcie-pci-bridge,bus=pcie.0,id=%s", id);
>> 
>> -args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, root);
>> args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, br);
>> }
>> 
>> @@ -811,8 +809,8 @@ static void test_override_scsi_q35(void)
>> add_drive_with_mbr(args, empty_mbr, 1);
>> add_drive_with_mbr(args, empty_mbr, 1);
>> add_drive_with_mbr(args, empty_mbr, 1);
>> -setup_pci_bridge(args, "pcie.0", "br");
>> -add_scsi_controller(args, "lsi53c895a", "br", 3);
>> +setup_pci_bridge(args, "pcie-pci-br");
>> +add_scsi_controller(args, "lsi53c895a", "pcie-pci-br", 3);
>> add_scsi_disk(args, 0, 0, 0, 0, 0, 1, 120, 30);
>> add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30);
>> add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0);
>> @@ -868,9 +866,9 @@ static void test_override_virtio_blk_q35(void)
>> };
>> add_drive_with_mbr(args, empty_mbr, 1);
>> add_drive_with_mbr(args, empty_mbr, 1);
>> -setup_pci_bridge(args, "pcie.0", "br");
>> -add_virtio_disk(args, 0, "br", 3, 1, 120, 30);
>> -add_virtio_disk(args, 1, "br", 4, 9000, 120, 30);
>> +setup_pci_bridge(args, "pcie-pci-br");
>> +add_virtio_disk(args, 0, "pcie-pci-br", 3, 1, 120, 30);
>> +add_virtio_disk(args, 1, "pcie-pci-br", 4, 9000, 120, 30);
>> test_override(args, "q35", expected);
>> }
>> 
> 

Re: [PATCH 3/4] target/alpha: Use float64_to_int64_modulo for CVTTQ

[Philippe Mathieu-Daudé]

On 27/5/23 16:19, Richard Henderson wrote:

For the most part we can use the new generic routine,
though exceptions need some post-processing to sort
invalid from integer overflow.

Signed-off-by: Richard Henderson 
---
  target/alpha/fpu_helper.c | 85 +--
  1 file changed, 18 insertions(+), 67 deletions(-)


To the best of my knowledge...
Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 1/3] i386/cpuid: Decrease cpuid_i when skipping CPUID leaf 1F

[Yang, Weijiang]

On 6/13/2023 9:19 PM, Xiaoyao Li wrote:

Decrease array index cpuid_i when CPUID leaf 1F is skipped, otherwise it
will get an all zero'ed CPUID entry with leaf 0 and subleaf 0. It
conflicts with correct leaf 0.


Maybe change the commit log like this:

Exiting code misses a decrement of cpuid_i when skip left 0x1F, so 
there's a blank CPUID


entry(with all fields stuffed 0s) left in the CPUID array.  Fix the 
issue to avoid the blank slot.


Reviewed-by:Yang Weijiang 



Signed-off-by: Xiaoyao Li 
---
  target/i386/kvm/kvm.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index de531842f6b1..afa97799d89a 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1956,6 +1956,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
  }
  case 0x1f:
  if (env->nr_dies < 2) {
+cpuid_i--;
  break;
  }
  /* fallthrough */

Re: [PATCH v2 2/3] i386/cpuid: Remove subleaf constraint on CPUID leaf 1F

[Yang, Weijiang]

On 6/13/2023 9:19 PM, Xiaoyao Li wrote:


No such constraint that subleaf index needs to be less than 64.

Signed-off-by: Xiaoyao Li 
---
  target/i386/kvm/kvm.c | 4 
  1 file changed, 4 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index afa97799d89a..d7e235ce35a6 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1968,10 +1968,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
  break;
  }
  
-if (i == 0x1f && j == 64) {

-break;
-}
-
  c->function = i;
  c->flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
  c->index = j;

Reviewed-by:Yang Weijiang 

Re: [PATCH v2 2/4] pc-bios/s390-ccw: Provide space for initial stack frame in start.S

[Claudio Imbrenda]

On Tue, 27 Jun 2023 09:47:01 +0200
Thomas Huth  wrote:

> Providing the space of a stack frame is the duty of the caller,
> so we should reserve 160 bytes before jumping into the main function.
> Otherwise the main() function might write past the stack array.
> 
> While we're at it, add a proper STACK_SIZE macro for the stack size
> instead of using magic numbers (this is also required for the following
> patch).
> 
> Reviewed-by: Christian Borntraeger 
> Reviewed-by: Cédric Le Goater 
> Signed-off-by: Thomas Huth 


with Marc's suggestion applied:

Reviewed-by: Claudio Imbrenda 

> ---
>  pc-bios/s390-ccw/start.S | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index d29de09cc6..29b0a9ece0 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -10,10 +10,12 @@
>   * directory.
>   */
>  
> +#define STACK_SIZE 0x8000
> +
>  .globl _start
>  _start:
>  
> -larl%r15,stack + 0x8000 /* Set up stack */
> +larl%r15,stack + STACK_SIZE - 160   /* Set up stack */
>  
>  /* clear bss */
>  larl%r2,__bss_start

Re: [PATCH v2 3/3] i386/cpuid: Move leaf 7 to correct group

[Yang, Weijiang]

On 6/13/2023 9:19 PM, Xiaoyao Li wrote:

CPUID leaf 7 was grouped together with SGX leaf 0x12 by commit
b9edbadefb9e ("i386: Propagate SGX CPUID sub-leafs to KVM") by mistake.

SGX leaf 0x12 has its specific logic to check if subleaf (starting from 2)
is valid or not by checking the bit 0:3 of corresponding EAX is 1 or
not.

Leaf 7 follows the logic that EAX of subleaf 0 enumerates the maximum
valid subleaf.

Fixes: b9edbadefb9e ("i386: Propagate SGX CPUID sub-leafs to KVM")
Signed-off-by: Xiaoyao Li 
---
  target/i386/kvm/kvm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index d7e235ce35a6..86aab9ca4ba2 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1993,7 +1993,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
  c = &cpuid_data.entries[cpuid_i++];
  }
  break;
-case 0x7:
  case 0x12:
  for (j = 0; ; j++) {
  c->function = i;
@@ -2013,6 +2012,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
  c = &cpuid_data.entries[cpuid_i++];
  }
  break;
+case 0x7:
  case 0x14:
  case 0x1d:
  case 0x1e: {


Reviewed-by:Yang Weijiang 

Re: [PATCH v2 4/4] pc-bios/s390-ccw: Don't use __bss_start with the "larl" instruction

[Claudio Imbrenda]

On Tue, 27 Jun 2023 09:47:03 +0200
Thomas Huth  wrote:

> start.S currently cannot be compiled with Clang 16 and binutils 2.40:
> 
>  ld: start.o(.text+0x8): misaligned symbol `__bss_start' (0xc1e5) for
>  relocation R_390_PC32DBL
> 
> According to the built-in linker script of ld, the symbol __bss_start
> can actually point *before* the .bss section and does not need to have
> any alignment, so in certain situations (like when using the internal
> assembler of Clang), the __bss_start symbol can indeed be unaligned
> and thus it is not suitable for being used with the "larl" instruction
> that needs an address that is at least aligned to halfwords.
> The problem went unnoticed so far since binutils <= 2.39 did not
> check the alignment, but starting with binutils 2.40, such unaligned
> addresses are now refused.
> 
> Fix it by using the real start address of the .bss section instead.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2216662
> Reported-by: Miroslav Rezanina 
> Suggested-by: Nick Clifton 
> Signed-off-by: Thomas Huth 
> ---
>  pc-bios/s390-ccw/start.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index 111dea261b..a63c4e3ff2 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -18,7 +18,7 @@ _start:
>  larl%r15,stack + STACK_SIZE - 160   /* Set up stack */
>  
>  /* clear bss */
> -larl%r2,__bss_start
> +larl%r2,.bss
>  larl%r3,_end

since we are here, do you have guarantees that _end is always correctly
aligned?

if so:

Reviewed-by: Claudio Imbrenda 

>  slgr%r3,%r2/* get sizeof bss */
>  ltgr%r3,%r3/* bss empty? */

Re: [PATCH v2 1/4] pc-bios/s390-ccw: Fix indentation in start.S

[Claudio Imbrenda]

On Tue, 27 Jun 2023 09:47:00 +0200
Thomas Huth  wrote:

> start.S is currently indented with a mixture of spaces and tabs, which
> is quite ugly. QEMU coding style says indentation should be 4 spaces,
> and this is also what we are using in the assembler files in the
> tests/tcg/s390x/ folder already, so let's adjust start.S accordingly.
> 
> Reviewed-by: Cédric Le Goater 
> Signed-off-by: Thomas Huth 

Reviewed-by: Claudio Imbrenda 

> ---
>  pc-bios/s390-ccw/start.S | 136 +++
>  1 file changed, 68 insertions(+), 68 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index 6072906df4..d29de09cc6 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -10,37 +10,37 @@
>   * directory.
>   */
>  
> -.globl _start
> +.globl _start
>  _start:
>  
> - larl   %r15, stack + 0x8000 /* Set up stack */
> +larl%r15,stack + 0x8000 /* Set up stack */
>  
> - /* clear bss */
> - larl %r2, __bss_start
> - larl %r3, _end
> - slgr %r3, %r2   /* get sizeof bss */
> - ltgr%r3,%r3 /* bss empty? */
> - jz  done
> - aghi%r3,-1
> - srlg%r4,%r3,8   /* how many 256 byte chunks? */
> - ltgr%r4,%r4
> - lgr %r1,%r2
> - jz  remainder
> +/* clear bss */
> +larl%r2,__bss_start
> +larl%r3,_end
> +slgr%r3,%r2/* get sizeof bss */
> +ltgr%r3,%r3/* bss empty? */
> +jz  done
> +aghi%r3,-1
> +srlg%r4,%r3,8  /* how many 256 byte chunks? */
> +ltgr%r4,%r4
> +lgr %r1,%r2
> +jz  remainder
>  loop:
> - xc  0(256,%r1),0(%r1)
> - la  %r1,256(%r1)
> - brctg   %r4,loop
> +xc  0(256,%r1),0(%r1)
> +la  %r1,256(%r1)
> +brctg   %r4,loop
>  remainder:
> - larl%r2,memsetxc
> - ex  %r3,0(%r2)
> +larl%r2,memsetxc
> +ex  %r3,0(%r2)
>  done:
> -/* set up a pgm exception disabled wait psw */
> -larl %r2, disabled_wait_psw
> -mvc  0x01d0(16), 0(%r2)
> -j  main  /* And call C */
> +/* set up a pgm exception disabled wait psw */
> +larl%r2,disabled_wait_psw
> +mvc 0x01d0(16),0(%r2)
> +j   main   /* And call C */
>  
>  memsetxc:
> - xc  0(1,%r1),0(%r1)
> +xc  0(1,%r1),0(%r1)
>  
>  
>  /*
> @@ -48,11 +48,11 @@ memsetxc:
>   *
>   * stops the current guest cpu.
>   */
> - .globl disabled_wait
> +.globl disabled_wait
>  disabled_wait:
> - larl%r1,disabled_wait_psw
> - lpswe   0(%r1)
> -1:   j   1b
> +larl%r1,disabled_wait_psw
> +lpswe   0(%r1)
> +1:  j   1b
>  
>  
>  /*
> @@ -60,61 +60,61 @@ disabled_wait:
>   *
>   * eats one sclp interrupt
>   */
> -.globl consume_sclp_int
> +.globl consume_sclp_int
>  consume_sclp_int:
> -/* enable service interrupts in cr0 */
> -stctg   %c0,%c0,0(%r15)
> -oi  6(%r15),0x2
> -lctlg   %c0,%c0,0(%r15)
> -/* prepare external call handler */
> -larl %r1, external_new_code
> -stg %r1, 0x1b8
> -larl %r1, external_new_mask
> -mvc 0x1b0(8),0(%r1)
> -/* load enabled wait PSW */
> -larl %r1, enabled_wait_psw
> -lpswe 0(%r1)
> +/* enable service interrupts in cr0 */
> +stctg   %c0,%c0,0(%r15)
> +oi  6(%r15),0x2
> +lctlg   %c0,%c0,0(%r15)
> +/* prepare external call handler */
> +larl%r1,external_new_code
> +stg %r1,0x1b8
> +larl%r1,external_new_mask
> +mvc 0x1b0(8),0(%r1)
> +/* load enabled wait PSW */
> +larl%r1,enabled_wait_psw
> +lpswe   0(%r1)
>  
>  /*
>   * void consume_io_int(void)
>   *
>   * eats one I/O interrupt
>   */
> -.globl consume_io_int
> +.globl consume_io_int
>  consume_io_int:
> -/* enable I/O interrupts in cr6 */
> -stctg %c6,%c6,0(%r15)
> -oi4(%r15), 0xff
> -lctlg %c6,%c6,0(%r15)
> -/* prepare i/o call handler */
> -larl  %r1, io_new_code
> -stg   %r1, 0x1f8
> -larl  %r1, io_new_mask
> -mvc   0x1f0(8),0(%r1)
> -/* load enabled wait PSW */
> -larl  %r1, enabled_wait_psw
> -lpswe 0(%r1)
> +/* enable I/O interrupts in cr6 */
> +stctg   %c6,%c6,0(%r15)
> +oi  4(%r15), 0xff
> +lctlg   %c6,%c6,0(%r15)
> +/* prepare i/o call handler */
> +larl%r1,io_new_code
> +stg %r1,0x1f8
> +larl%r1,io_new_mask
> +mvc 0x1f0(8),0(%r1)
> +/* load enabled wait PSW */
> +larl%r1,enabled_wait_psw
> +lpswe   0(%r1)
>  
>  external_new_code:
> -/* disable service interrupts in cr0 */
> -stctg   %c0,%c0,0(%r15)
> -ni  6(%r15),0xfd
> -lctlg   %c0,%c0,0(%r15)
> -br  %r14
> +/* disable service interrupts in cr0 */
> +stctg   %c0,%c0,

Re: [PATCH v2 3/4] pc-bios/s390-ccw: Move the stack array into start.S

[Claudio Imbrenda]

On Tue, 27 Jun 2023 09:47:02 +0200
Thomas Huth  wrote:

> The stack array is only referenced from the start-up code (which is
> shared between the s390-ccw.img and the s390-netboot.img), but it is
> currently declared twice, once in main.c and once in netmain.c.
> It makes more sense to declare this in start.S instead - which will
> also be helpful in the next patch, since we need to mention the .bss
> section in start.S in that patch.
> 
> While we're at it, let's also drop the huge alignment of the stack,
> since there is no technical requirement for aligning it to page
> boundaries.
> 
> Signed-off-by: Thomas Huth 

Reviewed-by: Claudio Imbrenda 

> ---
>  pc-bios/s390-ccw/s390-ccw.h | 1 -
>  pc-bios/s390-ccw/main.c | 1 -
>  pc-bios/s390-ccw/netmain.c  | 1 -
>  pc-bios/s390-ccw/start.S| 6 ++
>  4 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index b88e0550ab..91afcbbca9 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -55,7 +55,6 @@ void consume_io_int(void);
>  /* main.c */
>  void write_subsystem_identification(void);
>  void write_iplb_location(void);
> -extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>  unsigned int get_loadparm_index(void);
>  void main(void);
>  
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index a2def83e82..5506798098 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -17,7 +17,6 @@
>  #include "virtio-scsi.h"
>  #include "dasd-ipl.h"
>  
> -char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>  static SubChannelId blk_schid = { .one = 1 };
>  static char loadparm_str[LOADPARM_LEN + 1];
>  QemuIplParameters qipl;
> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
> index 056e93a818..5cd619b2d6 100644
> --- a/pc-bios/s390-ccw/netmain.c
> +++ b/pc-bios/s390-ccw/netmain.c
> @@ -50,7 +50,6 @@ void write_iplb_location(void) {}
>  /* STSI 3.2.2 offset of first vmdb + offset of uuid inside vmdb */
>  #define STSI322_VMDB_UUID_OFFSET ((8 + 12) * 4)
>  
> -char stack[PAGE_SIZE * 8] __attribute__((aligned(PAGE_SIZE)));
>  IplParameterBlock iplb __attribute__((aligned(PAGE_SIZE)));
>  static char cfgbuf[2048];
>  
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index 29b0a9ece0..111dea261b 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -120,3 +120,9 @@ external_new_mask:
>  .quad   0x00018000
>  io_new_mask:
>  .quad   0x00018000
> +
> +.bss
> +.align  8
> +stack:
> +.space  STACK_SIZE
> +.size   stack,STACK_SIZE

Re: [PATCH v2 0/8] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Philippe Mathieu-Daudé]

On 27/6/23 08:46, Mark Cave-Ayland wrote:

On 22/06/2023 13:26, Mark Cave-Ayland wrote:


On 21/06/2023 19:05, Richard Henderson wrote:


Changes from v1:
   * Split into teeny weeny pieces.

   * It turns out the sparc_tr_tb_stop hunk of v1 was buggy,
 in that things that are not simple branches use DYNAMIC_PC,
 e.g. the RETT (return from trap) instruction.

 Introduce DYNAMIC_PC_LOOKUP to distinguish the couple of
 places where we have a dynamic pc, but no other change
 of state (conditional branches, JMPL, RETURN).

   * Drop the change for WRFPRS, because it's too infrequent.
 The WRASI change affects memcpy/memset, so that's more important.

Boots Mark's sol8 install cdrom.  :-)

Top of the profile changes from

 41.55%  qemu-system-sparc  [.] cpu_exec_loop
 14.02%  qemu-system-sparc  [.] cpu_tb_exec
  8.74%  qemu-system-sparc  [.] tb_lookup
  2.11%  qemu-system-sparc  [.] tcg_splitwx_to_rw
  1.63%  memfd:tcg-jit (deleted)    [.] 0x0004

to

 31.59%  qemu-system-sparc  [.] helper_lookup_tb_ptr
 17.79%  qemu-system-sparc  [.] tb_lookup
  5.38%  qemu-system-sparc  [.] compute_all_sub
  2.38%  qemu-system-sparc  [.] helper_compute_psr
  2.36%  qemu-system-sparc  [.] helper_check_align
  1.79%  memfd:tcg-jit (deleted)    [.] 0x0063fc8e

This probably indicates that cpu_get_tb_cpu_state could be
improved to not consume so much overhead.


Nice! I've just run this through all of my sun4m/sun4u/sun4v test 
images and I don't see any regressions with v2. The guests feel 
noticeably more responsive too :)


Tested-by: Mark Cave-Ayland 

I've skimmed the patches and without looking in too much detail they 
seem to be okay so I'm happy to give:


Acked-by: Mark Cave-Ayland 

Side note: the niagara tests require the patch at 
https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03537.html 
which still hasn't been merged yet.



Richard Henderson (8):
   target/sparc: Use tcg_gen_lookup_and_goto_ptr in gen_goto_tb
   target/sparc: Fix npc comparison in sparc_tr_insn_start
   target/sparc: Drop inline markers from translate.c
   target/sparc: Introduce DYNAMIC_PC_LOOKUP
   target/sparc: Use DYNAMIC_PC_LOOKUP for conditional branches
   target/sparc: Use DYNAMIC_PC_LOOKUP for JMPL
   target/sparc: Use DYNAMIC_PC_LOOKUP for v9 RETURN
   target/sparc: Use tcg_gen_lookup_and_goto_ptr for v9 WRASI

  target/sparc/translate.c | 410 ++-
  1 file changed, 233 insertions(+), 177 deletions(-)


I've just noticed during testing there is an issue with this series when 
used with a real SS-5 PROM image (I was using OpenBIOS for my previous 
tests) which causes it to assert() almost immediately on startup:


$ ./qemu-system-sparc -bios ss5.bin
ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code should not 
be reached
Bail out! ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code 
should not be reached

Aborted


Could you try this fix:

-- >8 --
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -5682,5 +5682,5 @@ static void sparc_tr_tb_stop(DisasContextBase 
*dcbase, CPUState *cs)


 save_npc(dc);
-switch (dc->npc) {
+switch (dc->npc & 3) {
 case DYNAMIC_PC_LOOKUP:
 if (may_lookup) {
---

?

Re: [RESEND PATCH v5 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Ani Sinha]

> On 27-Jun-2023, at 2:32 PM, Igor Mammedov  wrote:
> 
> On Mon, 26 Jun 2023 21:42:44 +0530
> Ani Sinha  wrote:
> 
>> PCI Express ports only have one slot, so PCI Express devices can only be
>> plugged into slot 0 on a PCIE port. Enforce it.
> 
> btw, previously you mentioned ARI.
> So if we turn it on, wouldn't this patch actually become regression?

If ARI breaks this, it will break other areas in QEMU too, ex anywhere 
pci_get_function_0() is used.
Regardless, I think at least the tests are worth fixing, particularly the mess 
with hd-geo-test.

> 
>> 
>> CC: jus...@redhat.com
>> CC: imamm...@redhat.com
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
>> Signed-off-by: Ani Sinha 
>> Reviewed-by: Julia Suvorova 
>> ---
>> hw/pci/pci.c | 6 ++
>> 1 file changed, 6 insertions(+)
>> 
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index bf38905b7d..426af133b0 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -64,6 +64,7 @@ bool pci_available = true;
>> static char *pcibus_get_dev_path(DeviceState *dev);
>> static char *pcibus_get_fw_dev_path(DeviceState *dev);
>> static void pcibus_reset(BusState *qbus);
>> +static bool pcie_has_upstream_port(PCIDevice *dev);
>> 
>> static Property pci_props[] = {
>> DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>> @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice 
>> *pci_dev,
>>name);
>> 
>>return NULL;
>> +} else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>> +error_setg(errp, "PCI: slot %d is not valid for %s,"
>> +   " parent device only allows plugging into slot 0.",
>> +   PCI_SLOT(devfn), name);
>> +return NULL;
>> }
>> 
>> pci_dev->devfn = devfn;
> 

Re: [PATCH v2 06/26] qemu-keymap: properly check return from xkb_keymap_mod_get_index

[Alex Bennée]

Richard Henderson  writes:

> On 6/26/23 23:59, Alex Bennée wrote:
>> We can return XKB_MOD_INVALID which rightly gets flagged by sanitisers
>> as an overly wide shift attempt.
>> Signed-off-by: Alex Bennée 
>> ---
>>   qemu-keymap.c | 24 
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>> diff --git a/qemu-keymap.c b/qemu-keymap.c
>> index 229866e004..8c80f7a4ed 100644
>> --- a/qemu-keymap.c
>> +++ b/qemu-keymap.c
>> @@ -140,6 +140,18 @@ static void usage(FILE *out)
>>   names.options ?: "-");
>>   }
>>   +static xkb_mod_mask_t get_mod(struct xkb_keymap *map, const char
>> *name)
>> +{
>> +xkb_mod_index_t mod;
>> +xkb_mod_mask_t mask = 0;
>> +
>> +mod = xkb_keymap_mod_get_index(map, name);
>> +if (mod != XKB_MOD_INVALID) {
>> +mask = (1 << mod);
>> +}
>> +return mask;
>> +}
>
> You have yet to answer Peter's question -- asked twice -- about what
> changes in the keymaps with this. If nothing, should it in fact be an
> assert instead?

Ahh in the other thread. No change, it looks like AltGr just doesn't
exist for some keymaps:

  🕙21:20:36 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?] 
  ➜  diff -ub gb.before gb.after
  🕙21:20:43 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?] 
  ➜  diff -ub ara.before ara.after
  🕙21:20:50 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?] 
  ➜  ag "Alt" ara.after 
  21:# 9: Alt
  23:#11: LAlt
  24:#12: RAlt
  29:#17: AltGr
  294:Alt_L 0x38
  1711:Alt_R 0xb8
  🕙21:22:14 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?] 
  ➜  ag "Alt" gb.after 
  21:# 9: Alt
  23:#11: LAlt
  24:#12: RAlt
  29:#17: AltGr
  338:Alt_L 0x38
  1757:Alt_R 0xb8


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 08/26] tests/qtests: clean-up and fix leak in generic_fuzz

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> On 27/6/23 09:43, Juan Quintela wrote:
>> Alex Bennée  wrote:
>>> An update to the clang tooling detects more issues with the code
>>> including a memory leak from the g_string_new() allocation. Clean up
>>> the code with g_autoptr and use ARRAY_SIZE while we are at it.
>>>
>>> Signed-off-by: Alex Bennée 
>>> ---
>>>   tests/qtest/fuzz/generic_fuzz.c | 11 ---
>>>   1 file changed, 4 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tests/qtest/fuzz/generic_fuzz.c 
>>> b/tests/qtest/fuzz/generic_fuzz.c
>>> index c525d22951..a4841181cc 100644
>>> --- a/tests/qtest/fuzz/generic_fuzz.c
>>> +++ b/tests/qtest/fuzz/generic_fuzz.c
>>> @@ -954,17 +954,14 @@ static void register_generic_fuzz_targets(void)
>>>   .crossover = generic_fuzz_crossover
>>>   });
>>>   -GString *name;
>>> +g_autoptr(GString) name = g_string_new("");
>>>   const generic_fuzz_config *config;
>>>   -for (int i = 0;
>>> - i < sizeof(predefined_configs) / sizeof(generic_fuzz_config);
>>> - i++) {
>>> +for (int i = 0; i < ARRAY_SIZE(predefined_configs); i++) {
>>>   config = predefined_configs + i;
>>> -name = g_string_new("generic-fuzz");
>>> -g_string_append_printf(name, "-%s", config->name);
>>> +g_string_printf(name, "generic-fuzz-%s", config->name);
>>>   fuzz_add_target(&(FuzzTarget){
>>> -.name = name->str,
>>> +.name = g_strdup(name->str),
>>>   .description = "Predefined generic-fuzz config.",
>>>   .get_init_cmdline = 
>>> generic_fuzz_predefined_config_cmdline,
>>>   .pre_fuzz = generic_pre_fuzz,
>> Once that you are here, what about?
>> (Yes, I didn't care about the ARRAY_SIZE) but you got the idea.
>> Reviewed-by: Juan Quintela 
>> To your proposal with/without the change that I proposse.
>> modified   tests/qtest/fuzz/generic_fuzz.c
>> @@ -954,17 +954,14 @@ static void register_generic_fuzz_targets(void)
>>   .crossover = generic_fuzz_crossover
>>   });
>>   -GString *name;
>>   const generic_fuzz_config *config;
>> for (int i = 0;
>>i < sizeof(predefined_configs) / sizeof(generic_fuzz_config);
>>i++) {
>>   config = predefined_configs + i;
>> -name = g_string_new("generic-fuzz");
>> -g_string_append_printf(name, "-%s", config->name);
>>   fuzz_add_target(&(FuzzTarget){
>> -.name = name->str,
>> +.name = g_strdup_printf("generic-fuzz-%s", config->name),
>
> Even simpler is g_strconcat() suggested by Richard in v1:
> https://lore.kernel.org/qemu-devel/42b497a0-e234-64db-e845-1c37b6783...@linaro.org/

Doh - missed that will fix up.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

RE: [PATCH 1/8] colo: Only support the same qemu version on source and destination

[Zhang, Chen]

> -Original Message-
> From: Dong, Eddie 
> Sent: Friday, June 23, 2023 1:17 AM
> To: Lukas Straub ; qemu-devel  de...@nongnu.org>
> Cc: Zhang, Hailiang ; Juan Quintela
> ; Peter Xu ; Leonardo Bras
> ; Zhang, Chen 
> Subject: RE: [PATCH 1/8] colo: Only support the same qemu version on
> source and destination
> 
> 
> 
> > -Original Message-
> > From: qemu-devel-bounces+eddie.dong=intel@nongnu.org  > devel-bounces+eddie.dong=intel@nongnu.org> On Behalf Of Lukas
> > Straub
> > Sent: Thursday, June 22, 2023 5:15 AM
> > To: qemu-devel 
> > Cc: Zhang, Hailiang ; Juan Quintela
> > ; Peter Xu ; Leonardo Bras
> > ; Zhang, Chen 
> > Subject: [PATCH 1/8] colo: Only support the same qemu version on
> > source and destination
> >
> > Signed-off-by: Lukas Straub 
> > ---
> >  docs/COLO-FT.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt index
> > 2e760a4aee..8e64480dbd 100644
> > --- a/docs/COLO-FT.txt
> > +++ b/docs/COLO-FT.txt
> > @@ -148,6 +148,8 @@ in test procedure.
> >  Note: Here we are running both instances on the same host for
> > testing, change the IP Addresses if you want to run it on two hosts.
> > Initially
> >  127.0.0.1 is the Primary Host and 127.0.0.2 is the Secondary Host.
> > +Note: COLO is a experimental feature,
> an experimental feature
> 
> >so currently is should only be
> it should ...
> 
> > +used with the same qemu version on sourcee and target.

S/sourcee/source

Thanks
Chen

> >
> >  == Startup qemu ==
> >  1. Primary:
> > --
> > 2.39.2

Re: [PATCH v2 1/4] pc-bios/s390-ccw: Fix indentation in start.S

[Eric Farman]

On Tue, 2023-06-27 at 09:47 +0200, Thomas Huth wrote:
> start.S is currently indented with a mixture of spaces and tabs,
> which
> is quite ugly. QEMU coding style says indentation should be 4 spaces,
> and this is also what we are using in the assembler files in the
> tests/tcg/s390x/ folder already, so let's adjust start.S accordingly.
> 
> Reviewed-by: Cédric Le Goater 
> Signed-off-by: Thomas Huth 
> ---
>  pc-bios/s390-ccw/start.S | 136 +++--
> --
>  1 file changed, 68 insertions(+), 68 deletions(-)

Oh, that looks nice. Thank you.

Reviewed-by: Eric Farman 

Re: [PATCH 1/4] fpu: Add float64_to_int{32,64}_modulo

[Alex Bennée]

Richard Henderson  writes:

> Add versions of float64_to_int* which do not saturate the result.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 2/4] pc-bios/s390-ccw: Provide space for initial stack frame in start.S

[Eric Farman]

On Tue, 2023-06-27 at 11:14 +0200, Philippe Mathieu-Daudé wrote:
> On 27/6/23 10:26, Marc Hartmayer wrote:
> > Thomas Huth  writes:
> > 
> > > Providing the space of a stack frame is the duty of the caller,
> > > so we should reserve 160 bytes before jumping into the main
> > > function.
> > > Otherwise the main() function might write past the stack array.
> > > 
> > > While we're at it, add a proper STACK_SIZE macro for the stack
> > > size
> > > instead of using magic numbers (this is also required for the
> > > following
> > > patch).
> > > 
> > > Reviewed-by: Christian Borntraeger 
> > > Reviewed-by: Cédric Le Goater 
> > > Signed-off-by: Thomas Huth 
> > > ---
> > >   pc-bios/s390-ccw/start.S | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 
> > > +#define STACK_SIZE 0x8000
> > > +
> > >   .globl _start
> > >   _start:
> > >   
> > > -    larl    %r15,stack + 0x8000 /* Set up stack */
> > > +    larl    %r15,stack + STACK_SIZE - 160   /* Set up stack */
> >   ^^^
> >   You can also add a macro
> > for this
> >   - e.g. STACK_FRAME_SIZE.
> 
> Yes please :) No need to respin.
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> 
> > Besides that,
> > Reviewed-by: Marc Hartmayer 
> =

Ditto :)

Reviewed-by: Eric Farman 

Re: [PATCH 2/4] tests/tcg/alpha: Add test for cvttq

[Alex Bennée]

Richard Henderson  writes:

> Test for invalid, integer overflow, and inexact.
> Test for proper result, modulo 2**64.
>
> Signed-off-by: Richard Henderson 

Acked-by: Alex Bennée 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 0/8] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Richard Henderson]

On 6/27/23 11:37, Philippe Mathieu-Daudé wrote:

On 27/6/23 08:46, Mark Cave-Ayland wrote:

On 22/06/2023 13:26, Mark Cave-Ayland wrote:


On 21/06/2023 19:05, Richard Henderson wrote:


Changes from v1:
   * Split into teeny weeny pieces.

   * It turns out the sparc_tr_tb_stop hunk of v1 was buggy,
 in that things that are not simple branches use DYNAMIC_PC,
 e.g. the RETT (return from trap) instruction.

 Introduce DYNAMIC_PC_LOOKUP to distinguish the couple of
 places where we have a dynamic pc, but no other change
 of state (conditional branches, JMPL, RETURN).

   * Drop the change for WRFPRS, because it's too infrequent.
 The WRASI change affects memcpy/memset, so that's more important.

Boots Mark's sol8 install cdrom.  :-)

Top of the profile changes from

 41.55%  qemu-system-sparc  [.] cpu_exec_loop
 14.02%  qemu-system-sparc  [.] cpu_tb_exec
  8.74%  qemu-system-sparc  [.] tb_lookup
  2.11%  qemu-system-sparc  [.] tcg_splitwx_to_rw
  1.63%  memfd:tcg-jit (deleted)    [.] 0x0004

to

 31.59%  qemu-system-sparc  [.] helper_lookup_tb_ptr
 17.79%  qemu-system-sparc  [.] tb_lookup
  5.38%  qemu-system-sparc  [.] compute_all_sub
  2.38%  qemu-system-sparc  [.] helper_compute_psr
  2.36%  qemu-system-sparc  [.] helper_check_align
  1.79%  memfd:tcg-jit (deleted)    [.] 0x0063fc8e

This probably indicates that cpu_get_tb_cpu_state could be
improved to not consume so much overhead.


Nice! I've just run this through all of my sun4m/sun4u/sun4v test images and I don't 
see any regressions with v2. The guests feel noticeably more responsive too :)


Tested-by: Mark Cave-Ayland 

I've skimmed the patches and without looking in too much detail they seem to be okay so 
I'm happy to give:


Acked-by: Mark Cave-Ayland 

Side note: the niagara tests require the patch at 
https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03537.html which still hasn't 
been merged yet.



Richard Henderson (8):
   target/sparc: Use tcg_gen_lookup_and_goto_ptr in gen_goto_tb
   target/sparc: Fix npc comparison in sparc_tr_insn_start
   target/sparc: Drop inline markers from translate.c
   target/sparc: Introduce DYNAMIC_PC_LOOKUP
   target/sparc: Use DYNAMIC_PC_LOOKUP for conditional branches
   target/sparc: Use DYNAMIC_PC_LOOKUP for JMPL
   target/sparc: Use DYNAMIC_PC_LOOKUP for v9 RETURN
   target/sparc: Use tcg_gen_lookup_and_goto_ptr for v9 WRASI

  target/sparc/translate.c | 410 ++-
  1 file changed, 233 insertions(+), 177 deletions(-)


I've just noticed during testing there is an issue with this series when used with a 
real SS-5 PROM image (I was using OpenBIOS for my previous tests) which causes it to 
assert() almost immediately on startup:


$ ./qemu-system-sparc -bios ss5.bin
ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code should not be 
reached
Bail out! ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code should not be 
reached

Aborted


Could you try this fix:

-- >8 --
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -5682,5 +5682,5 @@ static void sparc_tr_tb_stop(DisasContextBase *dcbase, 
CPUState *cs)

  save_npc(dc);
-    switch (dc->npc) {
+    switch (dc->npc & 3) {
  case DYNAMIC_PC_LOOKUP:
  if (may_lookup) {
---

?


No, we should have exact equality on those two values.


r~

Re: [PATCH v2 3/4] pc-bios/s390-ccw: Move the stack array into start.S

[Eric Farman]

On Tue, 2023-06-27 at 09:47 +0200, Thomas Huth wrote:
> The stack array is only referenced from the start-up code (which is
> shared between the s390-ccw.img and the s390-netboot.img), but it is
> currently declared twice, once in main.c and once in netmain.c.
> It makes more sense to declare this in start.S instead - which will
> also be helpful in the next patch, since we need to mention the .bss
> section in start.S in that patch.
> 
> While we're at it, let's also drop the huge alignment of the stack,
> since there is no technical requirement for aligning it to page
> boundaries.
> 
> Signed-off-by: Thomas Huth 
> ---
>  pc-bios/s390-ccw/s390-ccw.h | 1 -
>  pc-bios/s390-ccw/main.c | 1 -
>  pc-bios/s390-ccw/netmain.c  | 1 -
>  pc-bios/s390-ccw/start.S    | 6 ++
>  4 files changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Eric Farman 

Re: [PATCH v3 1/3] vfio/pci: Fix resource leak in vfio_realize

[Joao Martins]

>>>  out_deregister:
>>>  pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>>  if (vdev->irqchip_change_notifier.notify) {
>>>  kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
>>>  }
>>> +vfio_disable_interrupts(vdev);
>>> +if (vdev->intx.mmap_timer) {
>>> +timer_free(vdev->intx.mmap_timer);
>>> +}
>>
>> But this one suggests another one as it looks a pre-existing issue?
> Yes, it's another resource leak I just found.
> Not sure if it's better to also merge above change to this patch which is 
> targeting resource leak issues,
> or to PATCH2 which is targeting out_deregister path, or to create a new one.
> Any suggestion?

In general they are all bugs in the same deregistration path, but each resource
is not being teardown correctly. I tend to prefer 'logical change' per commit,
so there's would be a fix the irqchip_change notifier and another one for
mmap_timer teardown. Both with the Fixes tags that introduced each bug. Unless
everything was introduced by the same change in which case you would do
everything in one patch.

Re: [PATCH v2 06/26] qemu-keymap: properly check return from xkb_keymap_mod_get_index

[Peter Maydell]

On Tue, 27 Jun 2023 at 10:57, Alex Bennée  wrote:
> Ahh in the other thread. No change, it looks like AltGr just doesn't
> exist for some keymaps:
>
>   🕙21:20:36 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>   ➜  diff -ub gb.before gb.after
>   🕙21:20:43 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>   ➜  diff -ub ara.before ara.after
>   🕙21:20:50 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>   ➜  ag "Alt" ara.after
>   21:# 9: Alt
>   23:#11: LAlt
>   24:#12: RAlt
>   29:#17: AltGr
>   294:Alt_L 0x38
>   1711:Alt_R 0xb8
>   🕙21:22:14 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>   ➜  ag "Alt" gb.after
>   21:# 9: Alt
>   23:#11: LAlt
>   24:#12: RAlt
>   29:#17: AltGr
>   338:Alt_L 0x38
>   1757:Alt_R 0xb8

I'm having some difficulty interpreting this output. It
seems to show that there is an AltGr modifier in both
mappings (that's why it appears in the modifier listing).
And for me (xkeyboard-config 2.33) in the gb mapping it's
used too:

# evdev 2 (0x2), QKeyCode "1", number 0x2
1 0x02
exclam 0x02 shift
onesuperior 0x02 altgr
exclamdown 0x02 shift altgr

(i.e. the '1' key is 1 with no modifiers, ! with shift,
superscript-1 with altgr, and inverted exclamation mark
with shift-altgr).

The 'ara' keymap likewise has and uses altgr:
# evdev 2 (0x2), QKeyCode "1", number 0x2
1 0x02
exclam 0x02 shift
Arabic_1 0x02 altgr

So on the machines where we were running into this,
what's the version of xkeyboard-config and do we
output the same mapping as we do on machines with
the older xkeyboard-config ?

thanks
-- PMM

Re: [PATCH 0/4] target/ppc: Catch invalid real address accesses

[Howard Spoelstra]

On Tue, Jun 27, 2023 at 10:15 AM Mark Cave-Ayland <
mark.cave-ayl...@ilande.co.uk> wrote:

> On 26/06/2023 14:35, Cédric Le Goater wrote:
>
> > On 6/23/23 14:37, Cédric Le Goater wrote:
> >> On 6/23/23 11:10, Peter Maydell wrote:
> >>> On Fri, 23 Jun 2023 at 09:21, Nicholas Piggin 
> wrote:
> 
>  ppc has always silently ignored access to real (physical) addresses
>  with nothing behind it, which can make debugging difficult at times.
> 
>  It looks like the way to handle this is implement the transaction
>  failed call, which most target architectures do. Notably not x86
>  though, I wonder why?
> >>>
> >>> Much of this is historical legacy. QEMU originally had no
> >>> concept of "the system outside the CPU returns some kind
> >>> of bus error and the CPU raises an exception for it".
> >>> This is turn is (I think) because the x86 PC doesn't do
> >>> that: you always get back some kind of response, I think
> >>> -1 on reads and writes ignored. We added the do_transaction_failed
> >>> hook largely because we wanted it to give more accurate
> >>> emulation of this kind of thing on Arm, but as usual with new
> >>> facilities we left the other architectures to do it themselves
> >>> if they wanted -- by default the behaviour remained the same.
> >>> Some architectures have picked it up; some haven't.
> >>>
> >>> The main reason it's a bit of a pain to turn the correct
> >>> handling on is because often boards don't actually implement
> >>> all the devices they're supposed to. For a pile of legacy Arm
> >>> boards, especially where we didn't have good test images,
> >>> we use the machine flag ignore_memory_transaction_failures to
> >>> retain the legacy behaviour. (This isn't great because it's
> >>> pretty much going to mean we have that flag set on those
> >>> boards forever because nobody is going to care enough to
> >>> investigate and test.)
> >>>
>  Other question is, sometimes I guess it's nice to avoid crashing in
>  order to try to quickly get past some unimplemented MMIO. Maybe a
>  command line option or something could turn it off? It should
>  probably be a QEMU-wide option if so, so that shouldn't hold this
>  series up, I can propose a option for that if anybody is worried
>  about it.
> >>>
> >>> I would not recommend going any further than maybe setting the
> >>> ignore_memory_transaction_failures flag for boards you don't
> >>> care about. (But in an ideal world, don't set it and deal with
> >>> any bug reports by implementing stub versions of missing devices.
> >>> Depends how confident you are in your test coverage.)
> >>
> >> It seems it broke the "mac99" and  powernv10 machines, using the
> >> qemu-ppc-boot images which are mostly buildroot. See below for logs.
> >>
> >> Adding Mark for further testing on Mac OS.
> >
> >
> > Mac OS 9.2 fails to boot with a popup saying :
> >  Sorry, a system error occured.
> >  "Sound Manager"
> >address error
> >  To temporarily turn off extensions, restart and
> >  hold down the shift key
> >
> >
> > Darwin and Mac OSX look OK.
>
> My guess would be that MacOS 9.2 is trying to access the sound chip
> registers which
> isn't implemented in QEMU for the moment (I have a separate screamer
> branch
> available, but it's not ready for primetime yet). In theory they shouldn't
> be
> accessed at all because the sound device isn't present in the OpenBIOS
> device tree,
> but this is all fairly old stuff.
>
> Does implementing the sound registers using a dummy device help at all?
>
>
My uneducated guess is that you stumbled on a longstanding, but
intermittently occurring, issue specific to Mac OS 9.2 related to sound
support over USB in Apple monitors.
I believe It is not fixed by the patch set from the 23 of june, I still get
system errors when running Mac OS 9.2 with the mac99 machine after applying
them.
Mac OS 9.2 has required mac99,via=pmu for a long time now to always boot
successfully. (while 9.0.4 requires mac99 to boot, due to an undiagnosed
OHCI USB problem with the specific drivers that ship with it.)  ;-)

Best,
Howard


>
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 265c0bbd8d..e55f938da7 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -26,6 +26,7 @@
>   #include "qemu/osdep.h"
>   #include "qapi/error.h"
>   #include "qemu/module.h"
> +#include "hw/misc/unimp.h"
>   #include "hw/misc/macio/cuda.h"
>   #include "hw/pci/pci.h"
>   #include "hw/ppc/mac_dbdma.h"
> @@ -94,6 +95,7 @@ static bool macio_common_realize(PCIDevice *d, Error
> **errp)
>   {
>   MacIOState *s = MACIO(d);
>   SysBusDevice *sbd;
> +DeviceState *dev;
>
>   if (!qdev_realize(DEVICE(&s->dbdma), BUS(&s->macio_bus), errp)) {
>   return false;
> @@ -102,6 +104,14 @@ static bool macio_common_realize(PCIDevice *d, Error
> **errp)
>   memory_region_add_subregion(&s->bar, 0x08000,
>   sys

RE: [PATCH v3 1/3] vfio/pci: Fix resource leak in vfio_realize

[Duan, Zhenzhong]

>-Original Message-
>From: Joao Martins 
>Sent: Tuesday, June 27, 2023 6:22 PM
>To: Duan, Zhenzhong 
>Cc: alex.william...@redhat.com; c...@redhat.com; qemu-devel@nongnu.org;
>avih...@nvidia.com; Peng, Chao P 
>Subject: Re: [PATCH v3 1/3] vfio/pci: Fix resource leak in vfio_realize
>
  out_deregister:
  pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
  if (vdev->irqchip_change_notifier.notify) {
  kvm_irqchip_remove_change_notifier(&vdev-
>>irqchip_change_notifier);
  }
 +vfio_disable_interrupts(vdev);
 +if (vdev->intx.mmap_timer) {
 +timer_free(vdev->intx.mmap_timer);
 +}
>>>
>>> But this one suggests another one as it looks a pre-existing issue?
>> Yes, it's another resource leak I just found.
>> Not sure if it's better to also merge above change to this patch which
>> is targeting resource leak issues, or to PATCH2 which is targeting
>out_deregister path, or to create a new one.
>> Any suggestion?
>
>In general they are all bugs in the same deregistration path, but each resource
>is not being teardown correctly. I tend to prefer 'logical change' per commit,
>so there's would be a fix the irqchip_change notifier and another one for
>mmap_timer teardown. Both with the Fixes tags that introduced each bug.
>Unless everything was introduced by the same change in which case you
>would do everything in one patch.
Clear.

Thanks
Zhenzhong

Re: [PATCH 4/4] target/arm: Use float64_to_int32_modulo for FJCVTZS

[Alex Bennée]

Richard Henderson  writes:

> The standard floating point results are provided by the generic routine.
> We only need handle the extra Z flag result afterward.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] gdbstub: Permit reverse step/break to provide stop response

[Alex Bennée]

Nicholas Piggin  writes:

> The final part of the reverse step and break handling is to bring
> the machine back to a debug stop state. gdb expects a response.
>
> A gdb 'rsi' command hangs forever because the gdbstub filters out
> the response (also observable with reverse_debugging.py avocado
> tests).
>
> Fix by setting allow_stop_reply for the gdb backward packets.
>
> Fixes: 758370052fb ("gdbstub: only send stop-reply packets when allowed to")
> Cc: qemu-sta...@nongnu.org
> Cc: Matheus Tavares Bernardino 
> Cc: Alex Bennée 
> Cc: Taylor Simpson 
> Signed-off-by: Nicholas Piggin 

Queued to gdbstub/next, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 4/4] pc-bios/s390-ccw: Don't use __bss_start with the "larl" instruction

[Eric Farman]

On Tue, 2023-06-27 at 09:47 +0200, Thomas Huth wrote:
> start.S currently cannot be compiled with Clang 16 and binutils 2.40:
> 
>  ld: start.o(.text+0x8): misaligned symbol `__bss_start' (0xc1e5) for
>  relocation R_390_PC32DBL
> 
> According to the built-in linker script of ld, the symbol __bss_start
> can actually point *before* the .bss section and does not need to
> have
> any alignment, so in certain situations (like when using the internal
> assembler of Clang), the __bss_start symbol can indeed be unaligned
> and thus it is not suitable for being used with the "larl"
> instruction
> that needs an address that is at least aligned to halfwords.
> The problem went unnoticed so far since binutils <= 2.39 did not
> check the alignment, but starting with binutils 2.40, such unaligned
> addresses are now refused.
> 
> Fix it by using the real start address of the .bss section instead.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2216662
> Reported-by: Miroslav Rezanina 
> Suggested-by: Nick Clifton 
> Signed-off-by: Thomas Huth 
> ---
>  pc-bios/s390-ccw/start.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Farman 

Re: [PATCH] gdbstub: Permit reverse step/break to provide stop response

[Peter Maydell]

On Fri, 23 Jun 2023 at 13:19, Matheus Tavares Bernardino
 wrote:
>
> > Nicholas Piggin  wrote:
> >
> > The final part of the reverse step and break handling is to bring
> > the machine back to a debug stop state. gdb expects a response.
> >
> > A gdb 'rsi' command hangs forever because the gdbstub filters out
> > the response (also observable with reverse_debugging.py avocado
> > tests).
> >
> > Fix by setting allow_stop_reply for the gdb backward packets.
>
> Ah, it's interesting that [1] doesn't include 'bc' and 'bs' in the list
> of cmds that may respond with a stop-reply packet:
>
> "The 'C', 'c', 'S', 's', 'vCont', 'vAttach', 'vRun', 'vStopped', and
> '?' packets can receive any of the below as a reply."
>
> But their definitions at [2] do say the following:
>
> 'bc' (and 'bc')
> [...]
> Reply: See Stop Reply Packets, for the reply specifications.
>
> So I guess the list from [1] is not exhaustive. Anyway, thanks for the
> fix!

That looks like it's probably a gdb docs bug (forgetting to
update that list when the bc/bs packets were added); we
should probably report that to upstream gdb.

thanks
-- PMM

Re: [PATCH v2 4/4] pc-bios/s390-ccw: Don't use __bss_start with the "larl" instruction

[Thomas Huth]

On 27/06/2023 11.29, Claudio Imbrenda wrote:

On Tue, 27 Jun 2023 09:47:03 +0200
Thomas Huth  wrote:


start.S currently cannot be compiled with Clang 16 and binutils 2.40:

  ld: start.o(.text+0x8): misaligned symbol `__bss_start' (0xc1e5) for
  relocation R_390_PC32DBL

According to the built-in linker script of ld, the symbol __bss_start
can actually point *before* the .bss section and does not need to have
any alignment, so in certain situations (like when using the internal
assembler of Clang), the __bss_start symbol can indeed be unaligned
and thus it is not suitable for being used with the "larl" instruction
that needs an address that is at least aligned to halfwords.
The problem went unnoticed so far since binutils <= 2.39 did not
check the alignment, but starting with binutils 2.40, such unaligned
addresses are now refused.

Fix it by using the real start address of the .bss section instead.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2216662
Reported-by: Miroslav Rezanina 
Suggested-by: Nick Clifton 
Signed-off-by: Thomas Huth 
---
  pc-bios/s390-ccw/start.S | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 111dea261b..a63c4e3ff2 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -18,7 +18,7 @@ _start:
  larl%r15,stack + STACK_SIZE - 160   /* Set up stack */
  
  /* clear bss */

-larl%r2,__bss_start
+larl%r2,.bss
  larl%r3,_end


since we are here, do you have guarantees that _end is always correctly
aligned?


Yes, you can see the internal linker script by running
"ld --verbose", and there I get:

  ...
  .data1  : { *(.data1) }
  _edata = .; PROVIDE (edata = .);
  . = .;
  __bss_start = .;
  .bss:
  {
   *(.dynbss)
   *(.bss .bss.* .gnu.linkonce.b.*)
   *(COMMON)
   /* Align here to ensure that the .bss section occupies space up to
  _end.  Align after .bss to ensure correct alignment even if the
  .bss section disappears because there are no input sections.
  FIXME: Why do we need it? When there is no .bss section, we do not
  pad the .data section.  */
   . = ALIGN(. != 0 ? 64 / 8 : 1);
  }
  . = ALIGN(64 / 8);
  . = SEGMENT_START("ldata-segment", .);
  . = ALIGN(64 / 8);
  _end = .; PROVIDE (end = .);
  . = DATA_SEGMENT_END (.);
  ...

As you can see, there is no alignment in front of
__bss_start, but there is alignment in front of
__end.


if so:

Reviewed-by: Claudio Imbrenda 


Thanks!

 Thomas

Re: [PATCH v3 3/3] vfio/migration: vfio/migration: Refactor and fix print of "Migration disabled"

[Joao Martins]

On 27/06/2023 03:55, Duan, Zhenzhong wrote:
>> I guess it makes sense -- the thing that was tieing him was the global 
>> migration
>> blocker, which is now consolidated into the main migration blocker.
>>
>> My vIOMMU series will relax this condition yes (still same per-device scope).
>> And I will need to check the maximum IOVA in the vIOMMU. But it's new code
>> I can adjust and Zhenzhong shouldn't worry about the vIOMMU future stuff
> A bit confused, you agreed to move vfio_block_giommu_migration into 
> migration.c
> 
>> but I don't expect to influence location, say if it gets moved into 
>> migration.c
> and final decision is no move for influencing location reason? Do I 
> misunderstand?

Sorry for the confusion.

The thing is that I will need 'similar code' to test if a vIOMMU is enabled or
not. The reason being that dirty tracking will need this to understand what to
track meaning to decide whether we track the whole address space or just the
linear map[0]. And all that code is in common, not migration.c and where I use
it will have to look at all address spaces (because dirty tracking is started
for all devices, so there's no vbasedev to look at).

Eventually after the vIOMMU stuff, the migration blocker condition will look
more or less like this:

return (!vfio_viommu_preset(vbasedev) ||
(vfio_viommu_preset(vbasedev) &&
 !vfio_viommu_get_max_iova(&max)))

Whereby vfio_viommu_preset(vbasedev) is what you currently call
vfio_block_giommu_migration(vbasedev) in current patch. So perhaps I would say
to leave it in common.c and rename it to vfio_viommu_preset(vbasedev) with a
comment on top for /* Block migration with a vIOMMU */

Then when the time comes I can introduce in my vIOMMU series a
vfio_viommu_possible() [or some other name] when starting dirty tracking which
looks all VFIOAddressSpaces and reuses your vfio_viommu_preset(vbasedev). This
should cover current and future needs, without going to great extents beyond the
purpose of this patch?

[0]
https://lore.kernel.org/qemu-devel/20230622214845.3980-13-joao.m.mart...@oracle.com/#iZ31hw:vfio:common.c

[RFC PATCH] gdbstub: clean-up vcont handling to avoid goto

[Alex Bennée]

We can handle all the error exit cases by using g_autofree() for the
one thing that needs cleaning up on the exit.

Signed-off-by: Alex Bennée 
---
 gdbstub/gdbstub.c | 28 +---
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 9496d7b175..49143c7d83 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -573,7 +573,6 @@ static int gdb_handle_vcont(const char *p)
 {
 int res, signal = 0;
 char cur_action;
-char *newstates;
 unsigned long tmp;
 uint32_t pid, tid;
 GDBProcess *process;
@@ -581,7 +580,7 @@ static int gdb_handle_vcont(const char *p)
 GDBThreadIdKind kind;
 unsigned int max_cpus = gdb_get_max_cpus();
 /* uninitialised CPUs stay 0 */
-newstates = g_new0(char, max_cpus);
+g_autofree char *newstates = g_new0(char, max_cpus);
 
 /* mark valid CPUs with 1 */
 CPU_FOREACH(cpu) {
@@ -597,8 +596,7 @@ static int gdb_handle_vcont(const char *p)
 res = 0;
 while (*p) {
 if (*p++ != ';') {
-res = -ENOTSUP;
-goto out;
+return -ENOTSUP;
 }
 
 cur_action = *p++;
@@ -606,13 +604,12 @@ static int gdb_handle_vcont(const char *p)
 cur_action = qemu_tolower(cur_action);
 res = qemu_strtoul(p, &p, 16, &tmp);
 if (res) {
-goto out;
+return res;
 }
 signal = gdb_signal_to_target(tmp);
 } else if (cur_action != 'c' && cur_action != 's') {
 /* unknown/invalid/unsupported command */
-res = -ENOTSUP;
-goto out;
+return -ENOTSUP;
 }
 
 if (*p == '\0' || *p == ';') {
@@ -625,14 +622,12 @@ static int gdb_handle_vcont(const char *p)
 } else if (*p++ == ':') {
 kind = read_thread_id(p, &p, &pid, &tid);
 } else {
-res = -ENOTSUP;
-goto out;
+return -ENOTSUP;
 }
 
 switch (kind) {
 case GDB_READ_THREAD_ERR:
-res = -EINVAL;
-goto out;
+return -EINVAL;
 
 case GDB_ALL_PROCESSES:
 cpu = gdb_first_attached_cpu();
@@ -649,8 +644,7 @@ static int gdb_handle_vcont(const char *p)
 process = gdb_get_process(pid);
 
 if (!process->attached) {
-res = -EINVAL;
-goto out;
+return -EINVAL;
 }
 
 cpu = get_first_cpu_in_process(process);
@@ -668,8 +662,7 @@ static int gdb_handle_vcont(const char *p)
 
 /* invalid CPU/thread specified */
 if (!cpu) {
-res = -EINVAL;
-goto out;
+return -EINVAL;
 }
 
 /* only use if no previous match occourred */
@@ -679,12 +672,9 @@ static int gdb_handle_vcont(const char *p)
 break;
 }
 }
+
 gdbserver_state.signal = signal;
 gdb_continue_partial(newstates);
-
-out:
-g_free(newstates);
-
 return res;
 }
 
-- 
2.39.2

Re: [PATCH] gdbstub: fixes cases where wrong threads were reported to GDB on SIGINT

[Alex Bennée]

Matheus Branco Borella  writes:

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1725
>
> This fix is implemented by having the vCont handler set the value of
> `gdbserver_state.c_cpu` if any threads are to be resumed. The specific CPU
> is picked arbitrarily from the ones to be resumed, but it should be okay, as 
> all
> GDB cares about is that it is a resumed thread.
>
> Keep in mind that because this patch overwrites `c_cpu`, it breaks cases where
> $vCont is used together with $Hc, so there might be more work to be
> done here.

That doesn't sound good. Is that a possible case or an invalid one
because we shouldn't see gdbs using both?

> It might also be the case that it breaking this, specifically, isn't of
> consequence, seeing as single stepping with $vCont already overwrites `c_cpu`
> anyway, so you could say the implementation already behaves oddly as far as
> mixing $vCont and $Hc is concerned.

It would be nice to have some unit tests for this behaviour to defend
it. See the various tests in tests/tcg that call $(GDB_SCRIPT) for
examples.

BTW you are missing a Signed-off-by: tag which we will need to take a
patch submission. See:

  https://qemu.readthedocs.io/en/latest/devel/submitting-a-patch.html


> ---
>  gdbstub/gdbstub.c | 31 ++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index be18568d0a..4f7ac5ddfe 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -595,6 +595,15 @@ static int gdb_handle_vcont(const char *p)
>   *  or incorrect parameters passed.
>   */
>  res = 0;
> +
> +/* 
> + * target_count and last_target keep track of how many CPUs we are going 
> to
> + * step or resume, and a pointer to the state structure of one of them, 
> + * respectivelly
> + */
> +int target_count = 0;
> +CPUState *last_target = NULL;
> +
>  while (*p) {
>  if (*p++ != ';') {
>  res = -ENOTSUP;
> @@ -639,8 +648,10 @@ static int gdb_handle_vcont(const char *p)
>  while (cpu) {
>  if (newstates[cpu->cpu_index] == 1) {
>  newstates[cpu->cpu_index] = cur_action;
> -}
>  
> +target_count++;
> +last_target = cpu;
> +}
>  cpu = gdb_next_attached_cpu(cpu);
>  }
>  break;
> @@ -657,6 +668,9 @@ static int gdb_handle_vcont(const char *p)
>  while (cpu) {
>  if (newstates[cpu->cpu_index] == 1) {
>  newstates[cpu->cpu_index] = cur_action;
> +
> +target_count++;
> +last_target = cpu;
>  }
>  
>  cpu = gdb_next_cpu_in_process(cpu);
> @@ -675,10 +689,25 @@ static int gdb_handle_vcont(const char *p)
>  /* only use if no previous match occourred */
>  if (newstates[cpu->cpu_index] == 1) {
>  newstates[cpu->cpu_index] = cur_action;
> +
> +target_count++;
> +last_target = cpu;
>  }
>  break;
>  }
>  }
> +
> +/* 
> + * if we're about to resume a specific set of CPUs/threads, make it so 
> that 
> + * in case execution gets interrupted, we can send GDB a stop reply with 
> a
> + * correct value. it doesn't really matter which CPU we tell GDB the 
> signal 
> + * happened in (VM pauses stop all of them anyway), so long as it is one 
> of
> + * the ones we resumed/single stepped here.
> + */
> +if (target_count > 0) {
> +gdbserver_state.c_cpu = last_target;
> +}
> +
>  gdbserver_state.signal = signal;
>  gdb_continue_partial(newstates);

Looks reasonable at first glance but I would like some tests.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2] piix: fix regression during unplug in Xen HVM domUs

[Bernhard Beschow]

Am 27. Juni 2023 07:11:33 UTC schrieb Paolo Bonzini :
>On 6/26/23 23:19, Olaf Hering wrote:
>> I need advice on how to debug this.
>> 
>> One thing that stands out is uhci_irq().
>> It reads a u16 from the USBSTS register.
>> 
>> On the qemu side, this read is served from bmdma_read. Since the read
>> size is 2, the result is ~0, and uhci_irq() turns the controller off.
>> In other words, memory_region_ops_read from addr=0xc102 is served from 
>> "piix-bmdma"
>> 
>> If the pci_set_word calls in piix_ide_reset are skipped, the read is
>> served from uhci_port_write. This is the expected behavior.
>> In other words, memory_region_ops_read from addr=0xc102 is served from 
>> "uhci".
>
>I think what's happening is that
>
>pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
>
>is setting the BAR to 0xC100, therefore overlapping the UHCI device's region.  
>In principle this line shouldn't be necessary at all though; it's enough to 
>clear the COMMAND register.

Interesting. The BAR is a 32 bit register whose default value is 0x0001. I 
think what's supposed to happen here is a pci_set_long() rather than a 
pci_set_byte().

Bits 4..15 represent the BAR address, and pci_set_byte() only clears bits 4..7, 
leaving bits 8..15 unchanged. Perhaps this causes the BAR to be moved into the 
UHCI region? Does changing the call to pci_set_long() fix the problem?

Best regards,
Bernhard

>
>Can you check the value of the COMMAND register (pci_conf + 0x04, 16 bits, 
>little endian)?  Something might be causing the register to be set back to a 
>nonzero value, therefore re-enabling the I/O at the address that overlaps the 
>UHCI device.
>
>Paolo
>
>

Re: [RFC PATCH] gdbstub: clean-up vcont handling to avoid goto

[Philippe Mathieu-Daudé]

On 27/6/23 13:05, Alex Bennée wrote:

We can handle all the error exit cases by using g_autofree() for the
one thing that needs cleaning up on the exit.

Signed-off-by: Alex Bennée 
---
  gdbstub/gdbstub.c | 28 +---
  1 file changed, 9 insertions(+), 19 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 06/26] qemu-keymap: properly check return from xkb_keymap_mod_get_index

[Alex Bennée]

Peter Maydell  writes:

> On Tue, 27 Jun 2023 at 10:57, Alex Bennée  wrote:
>> Ahh in the other thread. No change, it looks like AltGr just doesn't
>> exist for some keymaps:
>>
>>   🕙21:20:36 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>>   ➜  diff -ub gb.before gb.after
>>   🕙21:20:43 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>>   ➜  diff -ub ara.before ara.after
>>   🕙21:20:50 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>>   ➜  ag "Alt" ara.after
>>   21:# 9: Alt
>>   23:#11: LAlt
>>   24:#12: RAlt
>>   29:#17: AltGr
>>   294:Alt_L 0x38
>>   1711:Alt_R 0xb8
>>   🕙21:22:14 alex@zen:qemu.git/builds/all  (399fc0c) (REBASING 2/22) [$!?]
>>   ➜  ag "Alt" gb.after
>>   21:# 9: Alt
>>   23:#11: LAlt
>>   24:#12: RAlt
>>   29:#17: AltGr
>>   338:Alt_L 0x38
>>   1757:Alt_R 0xb8
>
> I'm having some difficulty interpreting this output. It
> seems to show that there is an AltGr modifier in both
> mappings (that's why it appears in the modifier listing).
> And for me (xkeyboard-config 2.33) in the gb mapping it's
> used too:
>
> # evdev 2 (0x2), QKeyCode "1", number 0x2
> 1 0x02
> exclam 0x02 shift
> onesuperior 0x02 altgr
> exclamdown 0x02 shift altgr
>
> (i.e. the '1' key is 1 with no modifiers, ! with shift,
> superscript-1 with altgr, and inverted exclamation mark
> with shift-altgr).
>
> The 'ara' keymap likewise has and uses altgr:
> # evdev 2 (0x2), QKeyCode "1", number 0x2
> 1 0x02
> exclam 0x02 shift
> Arabic_1 0x02 altgr

Ahh right I see those too.

>
> So on the machines where we were running into this,
> what's the version of xkeyboard-config and do we
> output the same mapping as we do on machines with
> the older xkeyboard-config ?

2.35 I think:

ii  xkb-data   2.35.1-1all  X 
Keyboard Extension (XKB) configuration data

>
> thanks
> -- PMM


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 0/8] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Mark Cave-Ayland]

On 27/06/2023 10:37, Philippe Mathieu-Daudé wrote:


On 27/6/23 08:46, Mark Cave-Ayland wrote:

On 22/06/2023 13:26, Mark Cave-Ayland wrote:


On 21/06/2023 19:05, Richard Henderson wrote:


Changes from v1:
   * Split into teeny weeny pieces.

   * It turns out the sparc_tr_tb_stop hunk of v1 was buggy,
 in that things that are not simple branches use DYNAMIC_PC,
 e.g. the RETT (return from trap) instruction.

 Introduce DYNAMIC_PC_LOOKUP to distinguish the couple of
 places where we have a dynamic pc, but no other change
 of state (conditional branches, JMPL, RETURN).

   * Drop the change for WRFPRS, because it's too infrequent.
 The WRASI change affects memcpy/memset, so that's more important.

Boots Mark's sol8 install cdrom.  :-)

Top of the profile changes from

 41.55%  qemu-system-sparc  [.] cpu_exec_loop
 14.02%  qemu-system-sparc  [.] cpu_tb_exec
  8.74%  qemu-system-sparc  [.] tb_lookup
  2.11%  qemu-system-sparc  [.] tcg_splitwx_to_rw
  1.63%  memfd:tcg-jit (deleted)    [.] 0x0004

to

 31.59%  qemu-system-sparc  [.] helper_lookup_tb_ptr
 17.79%  qemu-system-sparc  [.] tb_lookup
  5.38%  qemu-system-sparc  [.] compute_all_sub
  2.38%  qemu-system-sparc  [.] helper_compute_psr
  2.36%  qemu-system-sparc  [.] helper_check_align
  1.79%  memfd:tcg-jit (deleted)    [.] 0x0063fc8e

This probably indicates that cpu_get_tb_cpu_state could be
improved to not consume so much overhead.


Nice! I've just run this through all of my sun4m/sun4u/sun4v test images and I 
don't see any regressions with v2. The guests feel noticeably more responsive too :)


Tested-by: Mark Cave-Ayland 

I've skimmed the patches and without looking in too much detail they seem to be 
okay so I'm happy to give:


Acked-by: Mark Cave-Ayland 

Side note: the niagara tests require the patch at 
https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03537.html which still 
hasn't been merged yet.



Richard Henderson (8):
   target/sparc: Use tcg_gen_lookup_and_goto_ptr in gen_goto_tb
   target/sparc: Fix npc comparison in sparc_tr_insn_start
   target/sparc: Drop inline markers from translate.c
   target/sparc: Introduce DYNAMIC_PC_LOOKUP
   target/sparc: Use DYNAMIC_PC_LOOKUP for conditional branches
   target/sparc: Use DYNAMIC_PC_LOOKUP for JMPL
   target/sparc: Use DYNAMIC_PC_LOOKUP for v9 RETURN
   target/sparc: Use tcg_gen_lookup_and_goto_ptr for v9 WRASI

  target/sparc/translate.c | 410 ++-
  1 file changed, 233 insertions(+), 177 deletions(-)


I've just noticed during testing there is an issue with this series when used with 
a real SS-5 PROM image (I was using OpenBIOS for my previous tests) which causes it 
to assert() almost immediately on startup:


$ ./qemu-system-sparc -bios ss5.bin
ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code should not be 
reached
Bail out! ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code should not 
be reached

Aborted


Could you try this fix:

-- >8 --
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -5682,5 +5682,5 @@ static void sparc_tr_tb_stop(DisasContextBase *dcbase, CPUState 
*cs)


  save_npc(dc);
-    switch (dc->npc) {
+    switch (dc->npc & 3) {
  case DYNAMIC_PC_LOOKUP:
  if (may_lookup) {
---

?


Unfortunately that doesn't fix the issue. A quick lunchtime debugging session with 
printf() shows this just before the assert() fires:


### dc->pc: 0x3
### dc->npc: 0xffd26c70
**
ERROR:../target/sparc/translate.c:5699:sparc_tr_tb_stop: code should not be 
reached
Bail out! ERROR:../target/sparc/translate.c:5699:sparc_tr_tb_stop: code should not be 
reached

Aborted


ATB,

Mark.

Re: [PATCH v2 1/2] qmp: remove virtio_list, search QOM tree instead

[Jonah Palmer]

On 6/26/23 08:16, Michael S. Tsirkin wrote:


On Mon, Jun 26, 2023 at 08:08:28AM -0400, Jonah Palmer wrote:

On 6/23/23 01:47, Michael S. Tsirkin wrote:

 On Fri, Jun 09, 2023 at 09:20:39AM -0400, Jonah Palmer wrote:

 The virtio_list duplicates information about virtio devices that 
already
 exist in the QOM composition tree. Instead of creating this list of
 realized virtio devices, search the QOM composition tree instead.

 This patch modifies the QMP command qmp_x_query_virtio to instead 
search
 the partial paths of '/machine/peripheral/' &
 '/machine/peripheral-anon/' in the QOM composition tree for virtio
 devices.

 A device is found to be a valid virtio device if (1) its canonical path
 is of 'TYPE_VIRTIO_DEVICE' and (2) the device has been realized.

 [Jonah: In the previous commit I had written that a device is found to
  be a valid virtio device if (1) it has a canonical path ending with
  'virtio-backend'.

  The code now determines if it's a virtio device by appending
  'virtio-backend' (if needed) to a given canonical path and then
  checking that path to see if the device is of type
 'TYPE_VIRTIO_DEVICE'.

  The patch also instead now checks to make sure it's a virtio device
  before attempting to check whether the device is realized or not.]

 Signed-off-by: Jonah Palmer


 Could one of QMP maintainers comment on this please?


 ---
  hw/virtio/virtio-qmp.c | 128 ++---
  hw/virtio/virtio-qmp.h |   8 +--
  hw/virtio/virtio.c |   6 --
  3 files changed, 82 insertions(+), 60 deletions(-)

 diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
 index b5e1835299..e936cc8ce5 100644
 --- a/hw/virtio/virtio-qmp.c
 +++ b/hw/virtio/virtio-qmp.c
 @@ -668,67 +668,101 @@ VirtioDeviceFeatures 
*qmp_decode_features(uint16_t device_id, uint64_t bitmap)
  VirtioInfoList *qmp_x_query_virtio(Error **errp)
  {
  VirtioInfoList *list = NULL;
 -VirtioInfo *node;
 -VirtIODevice *vdev;

 -QTAILQ_FOREACH(vdev, &virtio_list, next) {
 -DeviceState *dev = DEVICE(vdev);
 -Error *err = NULL;
 -QObject *obj = qmp_qom_get(dev->canonical_path, "realized", 
&err);
 -
 -if (err == NULL) {
 -GString *is_realized = qobject_to_json_pretty(obj, true);
 -/* virtio device is NOT realized, remove it from list */
 -if (!strncmp(is_realized->str, "false", 4)) {
 -QTAILQ_REMOVE(&virtio_list, vdev, next);
 -} else {
 -node = g_new(VirtioInfo, 1);
 -node->path = g_strdup(dev->canonical_path);
 -node->name = g_strdup(vdev->name);
 -QAPI_LIST_PREPEND(list, node);
 +/* Query the QOM composition tree for virtio devices */
 +qmp_set_virtio_device_list("/machine/peripheral/", &list);
 +qmp_set_virtio_device_list("/machine/peripheral-anon/", &list);

 How sure are we these will forever be the only two places where virtio
 can live?

A virtio device will always be considered a peripheral device, right?
Since peripheral devices are input and/or output devices by definition.

 +if (list == NULL) {
 +error_setg(errp, "No virtio devices found");
 +return NULL;
 +}
 +return list;
 +}
 +
 +/* qmp_set_virtio_device_list:
 + * @ppath: An incomplete peripheral path to search from.
 + * @list: A list of realized virtio devices.
 + * Searches a given incomplete peripheral path (e.g. 
'/machine/peripheral/'
 + * or '/machine/peripheral-anon/') for realized virtio devices and 
adds them
 + * to a given list of virtio devices.
 + */
 +void qmp_set_virtio_device_list(const char *ppath, VirtioInfoList 
**list)
 +{
 +ObjectPropertyInfoList *plist;
 +VirtioInfoList *node;
 +Error *err = NULL;
 +
 +/* Search an incomplete path for virtio devices */
 +plist = qmp_qom_list(ppath, &err);
 +if (err == NULL) {
 +ObjectPropertyInfoList *start = plist;
 +while (plist != NULL) {
 +ObjectPropertyInfo *value = plist->value;
 +GString *path = g_string_new(ppath);
 +g_string_append(path, value->name);
 +g_string_append(path, "/virtio-backend");
 +
 +/* Determine if full path is a realized virtio device */
 +VirtIODevice *vdev = q

Re: [PATCH 0/4] target/ppc: Catch invalid real address accesses

[Mark Cave-Ayland]

On 27/06/2023 11:28, Howard Spoelstra wrote:

On Tue, Jun 27, 2023 at 10:15 AM Mark Cave-Ayland > wrote:


On 26/06/2023 14:35, Cédric Le Goater wrote:

 > On 6/23/23 14:37, Cédric Le Goater wrote:
 >> On 6/23/23 11:10, Peter Maydell wrote:
 >>> On Fri, 23 Jun 2023 at 09:21, Nicholas Piggin mailto:npig...@gmail.com>> wrote:
 
  ppc has always silently ignored access to real (physical) addresses
  with nothing behind it, which can make debugging difficult at times.
 
  It looks like the way to handle this is implement the transaction
  failed call, which most target architectures do. Notably not x86
  though, I wonder why?
 >>>
 >>> Much of this is historical legacy. QEMU originally had no
 >>> concept of "the system outside the CPU returns some kind
 >>> of bus error and the CPU raises an exception for it".
 >>> This is turn is (I think) because the x86 PC doesn't do
 >>> that: you always get back some kind of response, I think
 >>> -1 on reads and writes ignored. We added the do_transaction_failed
 >>> hook largely because we wanted it to give more accurate
 >>> emulation of this kind of thing on Arm, but as usual with new
 >>> facilities we left the other architectures to do it themselves
 >>> if they wanted -- by default the behaviour remained the same.
 >>> Some architectures have picked it up; some haven't.
 >>>
 >>> The main reason it's a bit of a pain to turn the correct
 >>> handling on is because often boards don't actually implement
 >>> all the devices they're supposed to. For a pile of legacy Arm
 >>> boards, especially where we didn't have good test images,
 >>> we use the machine flag ignore_memory_transaction_failures to
 >>> retain the legacy behaviour. (This isn't great because it's
 >>> pretty much going to mean we have that flag set on those
 >>> boards forever because nobody is going to care enough to
 >>> investigate and test.)
 >>>
  Other question is, sometimes I guess it's nice to avoid crashing in
  order to try to quickly get past some unimplemented MMIO. Maybe a
  command line option or something could turn it off? It should
  probably be a QEMU-wide option if so, so that shouldn't hold this
  series up, I can propose a option for that if anybody is worried
  about it.
 >>>
 >>> I would not recommend going any further than maybe setting the
 >>> ignore_memory_transaction_failures flag for boards you don't
 >>> care about. (But in an ideal world, don't set it and deal with
 >>> any bug reports by implementing stub versions of missing devices.
 >>> Depends how confident you are in your test coverage.)
 >>
 >> It seems it broke the "mac99" and  powernv10 machines, using the
 >> qemu-ppc-boot images which are mostly buildroot. See below for logs.
 >>
 >> Adding Mark for further testing on Mac OS.
 >
 >
 > Mac OS 9.2 fails to boot with a popup saying :
 >      Sorry, a system error occured.
 >      "Sound Manager"
 >    address error
 >      To temporarily turn off extensions, restart and
 >      hold down the shift key
 >
 >
 > Darwin and Mac OSX look OK.

My guess would be that MacOS 9.2 is trying to access the sound chip 
registers which
isn't implemented in QEMU for the moment (I have a separate screamer branch
available, but it's not ready for primetime yet). In theory they shouldn't 
be
accessed at all because the sound device isn't present in the OpenBIOS 
device tree,
but this is all fairly old stuff.

Does implementing the sound registers using a dummy device help at all?


My uneducated guess is that you stumbled on a longstanding, but intermittently 
occurring, issue specific to Mac OS 9.2 related to sound support over USB in Apple 
monitors.


I'm not sure I understand this: are there non-standard command line options being 
used here other than "qemu-system-ppc -M mac99 -cdrom macos92.iso -boot d"?


I believe It is not fixed by the patch set from the 23 of june, I still get system 
errors when running Mac OS 9.2 with the mac99 machine after applying them.
Mac OS 9.2 has required mac99,via=pmu for a long time now to always boot 
successfully. (while 9.0.4 requires mac99 to boot, due to an undiagnosed OHCI USB 
problem with the specific drivers that ship with it.)  ;-)


I always test MacOS 9.2 boot both with and without via=pmu for my OpenBIOS tests, so 
I'd expect this to work unless a regression has slipped in?



ATB,

Mark.

Re: [PATCH 6/7] target/i386: Add new CPU model EmeraldRapids

[Xiaoyao Li]

On 6/27/2023 4:49 PM, Igor Mammedov wrote:

On Tue, 27 Jun 2023 13:54:23 +0800
Xiaoyao Li  wrote:


On 6/26/2023 8:56 PM, Igor Mammedov wrote:

On Fri, 16 Jun 2023 11:23:10 +0800
Tao Su  wrote:
   

From: Qian Wen

Emerald Rapids (EMR) is the next generation of Xeon server processor
after Sapphire Rapids (SPR).

Currently, regarding the feature set that can be exposed to guest, there
isn't any one new comparing with SPR cpu model, except that EMR has a
different model number.

Though it's practicable to define EMR as an alias of a new version of
SPR by only updating the model number and model name, it loses the
flexibility when new version of EMR cpu model are needed for adding new
features (that hasn't virtalized/supported by KVM yet).

Which begs a question, why do we need EMR model (or alias) at all
if it's the same as SPR at the moment.

Make new features supported 1st and only then introduce a new CPU model.
   


Even if no new feature (that can be virtualized and exposed to guest) in
EMR compared to SPR in the end, I think it still makes sense to provide
a dedicated EMR CPU model in QEMU. Because
1) User will know EMR, Intel's next generation of Xeon after SRP, is
supported by QEMU, via -cpu ?/ -cpu help;


I don't see any benefits in misleading user by showing EMR model which is
nothing else but SPR one.
On negative side you would increase maintenance burden by introducing
extra versions of CPU model later. Which by itself is abusing versioning,
mainly used for fixing CPU bugs, by using it for adding new features.


2) It's convenient for user to create an EMR VM. People may not care
that much what the difference between "-cpu SapphireRapids" with "-cpu
EmeraldRapids", while they do want to create an VM which shows the CPU
is EmeraldRapids.


My guess would be is that you need guest to show EMR for developing EMR
features/guest bringup, in that case do it in your fork, and once
support is actually ready publish completed patches for it.


No. I meant for CSPs who want to provide an EMR virtual machine to their 
customers, or lab admin provides an EMR (virtual) machine to its user.


Without a dedicated EmeraldRapids cpu model provided by QEMU, they need 
to use something like


  -cpu SapphireRapids,model=207,model-id="Intel Xeon Processor 
(EmeraldRapids)"


It's likely that no difference in supported features between SPR cpu 
model and EMR cpu model in the end. If so, will QEMU choose to provide a 
dedicated CPU model for EMR? or just document somewhere to QEMU users 
that "if you want to create an virtual machine with EMR cpu model, 
please go with SPR cpu model while changing it's model number to EMR's 
207 and changing model-id to tell EmeraldRapids" ?



To exaggerate you reasoning further, we should create CPU models for
all future planned Intel/AMD CPU as a one of currently existing in
QEMU right now and then sometime in future implement features that
actually make those models what they should be.


No, it's not the purpose. In fact, we're not adding an temporary EMR cpu 
model while planing to complement it in the future. Instead, we are 
adding an official EMR cpu model. The fact is, in terms of the features 
that are virtualizable and can be exposed to guest, there is no 
difference between SPR and EMR.


This comes to a basic question：Will QEMU provide a EMR cpu model even if 
no difference to SPR cpu model except the model number?



It's downright confusing for user, so I'd object to this approach.

Re: [PULL 28/53] vdpa: move CVQ isolation check to net_init_vhost_vdpa

[Peter Maydell]

On Mon, 26 Jun 2023 at 13:29, Michael S. Tsirkin  wrote:
>
> From: Eugenio Pérez 
>
> Evaluating it at start time instead of initialization time may make the
> guest capable of dynamically adding or removing migration blockers.
>
> Also, moving to initialization reduces the number of ioctls in the
> migration, reducing failure possibilities.
>
> As a drawback we need to check for CVQ isolation twice: one time with no
> MQ negotiated and another one acking it, as long as the device supports
> it.  This is because Vring ASID / group management is based on vq
> indexes, but we don't know the index of CVQ before negotiating MQ.

I was looking at this code because of a Coverity report.
That turned out to be a false positive, but I did notice
something here that looks like it might be wrong:

>
> +/**
> + * Probe if CVQ is isolated
> + *
> + * @device_fd The vdpa device fd
> + * @features  Features offered by the device.
> + * @cvq_index The control vq pair index
> + *
> + * Returns <0 in case of failure, 0 if false and 1 if true.
> + */
> +static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
> +  int cvq_index, Error **errp)
> +{
> +uint64_t backend_features;
> +int64_t cvq_group;
> +uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE |
> + VIRTIO_CONFIG_S_DRIVER |
> + VIRTIO_CONFIG_S_FEATURES_OK;
> +int r;
> +
> +ERRP_GUARD();
> +
> +r = ioctl(device_fd, VHOST_GET_BACKEND_FEATURES, &backend_features);
> +if (unlikely(r < 0)) {
> +error_setg_errno(errp, errno, "Cannot get vdpa backend_features");
> +return r;
> +}
> +
> +if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID))) {
> +return 0;
> +}
> +
> +r = ioctl(device_fd, VHOST_SET_FEATURES, &features);
> +if (unlikely(r)) {
> +error_setg_errno(errp, errno, "Cannot set features");

Shouldn't we have a 'return r' (or maybe a 'goto out') here ?
Otherwise we'll just plough onward and attempt to continue
execution...

> +}
> +
> +r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> +if (unlikely(r)) {
> +error_setg_errno(errp, -r, "Cannot set device features");

Isn't this trying to set device status, not features ?

> +goto out;
> +}

thanks
-- PMM

Re: [PATCH v2] piix: fix regression during unplug in Xen HVM domUs

[Olaf Hering]

Mon, 26 Jun 2023 23:19:01 +0200 Olaf Hering :

> So far I was unable to decipher how the pci_set_word calls can
> possibly affect the outcome and the owner of memory_region_ops_read.

It is enough to return from piix_ide_reset right after
pci_set_word(pci_conf + PCI_COMMAND, 0) to trigger the issue.


One thing which was not mentioned yet: the order in which kernel drivers
are loaded matters. Usually it is xen-platform-pci/uhci-hcd/ata_piix.

When uhci loads, it scans the USB bus, finds the tablet, loads usbhid.
While this happens, ata_piix loads. It finds the PCI device in state
disabled. The PCI code enables the device. On the qemu side this ends
up in pci_default_write_config for PCI device "piix3-ide" with addr=4,
val=1, len=2. This calls pci_update_mappings, which for region #4
changes the addr from 0xc120 to 0xc100. This causes the issue. Now
usbhid tries to use the USB bus, but uhci_irq fails.

If ata_piix is not loaded, uhci works.
If ata_piix is loaded before uhci-hcd, the USB bus can not be scanned,
udev is killed after a timeout and boot proceeds.
If usbhid is loaded before ata_piix, USB bus discovery usually finishes
before ata_piix enables its PCI device, boot proceeds.


Olaf


pgpAUwVLa6uWc.pgp
Description: Digitale Signatur von OpenPGP

Re: [PATCH 6/7] target/i386: Add new CPU model EmeraldRapids

[Daniel P . Berrangé]

On Tue, Jun 27, 2023 at 07:25:21PM +0800, Xiaoyao Li wrote:
> On 6/27/2023 4:49 PM, Igor Mammedov wrote:
> > On Tue, 27 Jun 2023 13:54:23 +0800
> > Xiaoyao Li  wrote:
> > 
> > > On 6/26/2023 8:56 PM, Igor Mammedov wrote:
> > > > On Fri, 16 Jun 2023 11:23:10 +0800
> > > > Tao Su  wrote:
> > > > > From: Qian Wen
> > > > > 
> > > > > Emerald Rapids (EMR) is the next generation of Xeon server processor
> > > > > after Sapphire Rapids (SPR).
> > > > > 
> > > > > Currently, regarding the feature set that can be exposed to guest, 
> > > > > there
> > > > > isn't any one new comparing with SPR cpu model, except that EMR has a
> > > > > different model number.
> > > > > 
> > > > > Though it's practicable to define EMR as an alias of a new version of
> > > > > SPR by only updating the model number and model name, it loses the
> > > > > flexibility when new version of EMR cpu model are needed for adding 
> > > > > new
> > > > > features (that hasn't virtalized/supported by KVM yet).
> > > > Which begs a question, why do we need EMR model (or alias) at all
> > > > if it's the same as SPR at the moment.
> > > > 
> > > > Make new features supported 1st and only then introduce a new CPU model.
> > > 
> > > Even if no new feature (that can be virtualized and exposed to guest) in
> > > EMR compared to SPR in the end, I think it still makes sense to provide
> > > a dedicated EMR CPU model in QEMU. Because
> > > 1) User will know EMR, Intel's next generation of Xeon after SRP, is
> > > supported by QEMU, via -cpu ?/ -cpu help;
> > 
> > I don't see any benefits in misleading user by showing EMR model which is
> > nothing else but SPR one.
> > On negative side you would increase maintenance burden by introducing
> > extra versions of CPU model later. Which by itself is abusing versioning,
> > mainly used for fixing CPU bugs, by using it for adding new features.
> > 
> > > 2) It's convenient for user to create an EMR VM. People may not care
> > > that much what the difference between "-cpu SapphireRapids" with "-cpu
> > > EmeraldRapids", while they do want to create an VM which shows the CPU
> > > is EmeraldRapids.
> > > 
> > My guess would be is that you need guest to show EMR for developing EMR
> > features/guest bringup, in that case do it in your fork, and once
> > support is actually ready publish completed patches for it.
> 
> No. I meant for CSPs who want to provide an EMR virtual machine to their
> customers, or lab admin provides an EMR (virtual) machine to its user.
> 
> Without a dedicated EmeraldRapids cpu model provided by QEMU, they need to
> use something like
> 
>   -cpu SapphireRapids,model=207,model-id="Intel Xeon Processor
> (EmeraldRapids)"
>
> It's likely that no difference in supported features between SPR cpu model
> and EMR cpu model in the end. If so, will QEMU choose to provide a dedicated
> CPU model for EMR? or just document somewhere to QEMU users that "if you
> want to create an virtual machine with EMR cpu model, please go with SPR cpu
> model while changing it's model number to EMR's 207 and changing model-id to
> tell EmeraldRapids" ?

I think QEMU's answer would be to not bother trying todo this at all,
just expose '-cpu SapphireRapids', because there's no functional benefit
to overriding the model ID, when all the CPUID features are identical.

Those who have the guest to see the *perfect* functional match of the
host still have '-cpu host' available.

The named CPU models are for the case where we want a rough approximation
for a CPU generation available, to easy migration across mixed CPU clusters.
Given the intent is to have a rough approximation, there's no compelling
reason to add an exact EmeraldRapids named CPU.

> > To exaggerate you reasoning further, we should create CPU models for
> > all future planned Intel/AMD CPU as a one of currently existing in
> > QEMU right now and then sometime in future implement features that
> > actually make those models what they should be.
> 
> No, it's not the purpose. In fact, we're not adding an temporary EMR cpu
> model while planing to complement it in the future. Instead, we are adding
> an official EMR cpu model. The fact is, in terms of the features that are
> virtualizable and can be exposed to guest, there is no difference between
> SPR and EMR.
> 
> This comes to a basic question：Will QEMU provide a EMR cpu model even if no
> difference to SPR cpu model except the model number?

Historically we have generally only added new CPU models if there was
a feature difference. We've skipped adding many of the Intel models
that didn't add bring new features, on the basis that there is no
compelling functional need to have them.

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v2] piix: fix regression during unplug in Xen HVM domUs

[Olaf Hering]

Tue, 27 Jun 2023 10:12:50 + Bernhard Beschow :

> Bits 4..15 represent the BAR address, and pci_set_byte() only clears bits 
> 4..7, leaving bits 8..15 unchanged. Perhaps this causes the BAR to be moved 
> into the UHCI region? Does changing the call to pci_set_long() fix the 
> problem?

Thanks, it seems this fixes the issue. Let me verify this with a clean build.


Olaf


pgpWRfMj07Puc.pgp
Description: Digitale Signatur von OpenPGP

[PATCH] pc-bios/s390-ccw: Get rid of the the __u* types

[Thomas Huth]

Using types starting with double underscores should be avoided since these
names are marked as reserved by the C standard. The corresponding Linux
kernel header file has also been changed accordingly a long time ago:

 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/drivers/s390/cio/cio.h?id=cd6b4f27b9bb2a

So we should get rid of the __u* in the s390-ccw bios now finally, too.

Signed-off-by: Thomas Huth 
---
 Based-on: <20230510143925.4094-4-quint...@redhat.com>

 pc-bios/s390-ccw/cio.h  | 232 ++--
 pc-bios/s390-ccw/s390-ccw.h |   4 -
 2 files changed, 116 insertions(+), 120 deletions(-)

diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index efeb449572..c977a52b50 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -17,10 +17,6 @@ typedef unsigned char  u8;
 typedef unsigned short u16;
 typedef unsigned int   u32;
 typedef unsigned long long u64;
-typedef unsigned char  __u8;
-typedef unsigned short __u16;
-typedef unsigned int   __u32;
-typedef unsigned long long __u64;
 
 #define true 1
 #define false 0
diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
index 88a88adfd2..8b18153deb 100644
--- a/pc-bios/s390-ccw/cio.h
+++ b/pc-bios/s390-ccw/cio.h
@@ -17,32 +17,32 @@
  * path management control word
  */
 struct pmcw {
-__u32 intparm;  /* interruption parameter */
-__u32 qf:1; /* qdio facility */
-__u32 w:1;
-__u32 isc:3;/* interruption subclass */
-__u32 res5:3;   /* reserved zeros */
-__u32 ena:1;/* enabled */
-__u32 lm:2; /* limit mode */
-__u32 mme:2;/* measurement-mode enable */
-__u32 mp:1; /* multipath mode */
-__u32 tf:1; /* timing facility */
-__u32 dnv:1;/* device number valid */
-__u32 dev:16;   /* device number */
-__u8  lpm;  /* logical path mask */
-__u8  pnom; /* path not operational mask */
-__u8  lpum; /* last path used mask */
-__u8  pim;  /* path installed mask */
-__u16 mbi;  /* measurement-block index */
-__u8  pom;  /* path operational mask */
-__u8  pam;  /* path available mask */
-__u8  chpid[8]; /* CHPID 0-7 (if available) */
-__u32 unused1:8;/* reserved zeros */
-__u32 st:3; /* subchannel type */
-__u32 unused2:18;   /* reserved zeros */
-__u32 mbfc:1;   /* measurement block format control */
-__u32 xmwme:1;  /* extended measurement word mode enable */
-__u32 csense:1; /* concurrent sense; can be enabled ...*/
+u32 intparm;/* interruption parameter */
+u32 qf:1;   /* qdio facility */
+u32 w:1;
+u32 isc:3;  /* interruption subclass */
+u32 res5:3; /* reserved zeros */
+u32 ena:1;  /* enabled */
+u32 lm:2;   /* limit mode */
+u32 mme:2;  /* measurement-mode enable */
+u32 mp:1;   /* multipath mode */
+u32 tf:1;   /* timing facility */
+u32 dnv:1;  /* device number valid */
+u32 dev:16; /* device number */
+u8  lpm;/* logical path mask */
+u8  pnom;   /* path not operational mask */
+u8  lpum;   /* last path used mask */
+u8  pim;/* path installed mask */
+u16 mbi;/* measurement-block index */
+u8  pom;/* path operational mask */
+u8  pam;/* path available mask */
+u8  chpid[8];   /* CHPID 0-7 (if available) */
+u32 unused1:8;  /* reserved zeros */
+u32 st:3;   /* subchannel type */
+u32 unused2:18; /* reserved zeros */
+u32 mbfc:1; /* measurement block format control */
+u32 xmwme:1;/* extended measurement word mode enable */
+u32 csense:1;   /* concurrent sense; can be enabled ...*/
 /*  ... per MSCH, however, if facility */
 /*  ... is not installed, this results */
 /*  ... in an operand exception.   */
@@ -50,24 +50,24 @@ struct pmcw {
 
 /* Target SCHIB configuration. */
 struct schib_config {
-__u64 mba;
-__u32 intparm;
-__u16 mbi;
-__u32 isc:3;
-__u32 ena:1;
-__u32 mme:2;
-__u32 mp:1;
-__u32 csense:1;
-__u32 mbfc:1;
+u64 mba;
+u32 intparm;
+u16 mbi;
+u32 isc:3;
+u32 ena:1;
+u32 mme:2;
+u32 mp:1;
+u32 csense:1;
+u32 mbfc:1;
 } __attribute__ ((packed));
 
 struct scsw {
-__u16 flags;
-__u16 ctrl;
-__u32 cpa;
-__u8 dstat;
-__u8 cstat;
-__u16 count;
+u16 flags;
+u16 ctrl;
+u32 cpa;
+u8 dstat;
+u8 cstat;
+u16 count;
 } __attribute__ ((packed));
 
 /* Function Control */
@@ -117,42 +117,42 @@ struct scsw {
 typedef struct schib {
 struct pmcw pmcw; /* path management control wo

[PATCH v3 1/6] target/ppc: Have 'kvm_ppc.h' include 'sysemu/kvm.h'

[Philippe Mathieu-Daudé]

"kvm_ppc.h" declares:

  int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run);

'struct kvm_run' is declared in "sysemu/kvm.h", include it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/ppc/kvm_ppc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 611debc3ce..2e395416f0 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -9,6 +9,7 @@
 #ifndef KVM_PPC_H
 #define KVM_PPC_H
 
+#include "sysemu/kvm.h"
 #include "exec/hwaddr.h"
 #include "cpu.h"
 
-- 
2.38.1

[PATCH v3 2/6] target/ppc: Reorder #ifdef'ry in kvm_ppc.h

[Philippe Mathieu-Daudé]

Keep a single if/else/endif block checking CONFIG_KVM.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/ppc/kvm_ppc.h | 62 
 1 file changed, 28 insertions(+), 34 deletions(-)

diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 2e395416f0..49954a300b 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -93,7 +93,34 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t 
tb_offset);
 
 int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run);
 
-#else
+#define kvmppc_eieio() \
+do {  \
+if (kvm_enabled()) {  \
+asm volatile("eieio" : : : "memory"); \
+} \
+} while (0)
+
+/* Store data cache blocks back to memory */
+static inline void kvmppc_dcbst_range(PowerPCCPU *cpu, uint8_t *addr, int len)
+{
+uint8_t *p;
+
+for (p = addr; p < addr + len; p += cpu->env.dcache_line_size) {
+asm volatile("dcbst 0,%0" : : "r"(p) : "memory");
+}
+}
+
+/* Invalidate instruction cache blocks */
+static inline void kvmppc_icbi_range(PowerPCCPU *cpu, uint8_t *addr, int len)
+{
+uint8_t *p;
+
+for (p = addr; p < addr + len; p += cpu->env.icache_line_size) {
+asm volatile("icbi 0,%0" : : "r"(p));
+}
+}
+
+#else /* !CONFIG_KVM */
 
 static inline uint32_t kvmppc_get_tbfreq(void)
 {
@@ -440,10 +467,6 @@ static inline bool 
kvmppc_pvr_workaround_required(PowerPCCPU *cpu)
 return false;
 }
 
-#endif
-
-#ifndef CONFIG_KVM
-
 #define kvmppc_eieio() do { } while (0)
 
 static inline void kvmppc_dcbst_range(PowerPCCPU *cpu, uint8_t *addr, int len)
@@ -454,35 +477,6 @@ static inline void kvmppc_icbi_range(PowerPCCPU *cpu, 
uint8_t *addr, int len)
 {
 }
 
-#else   /* CONFIG_KVM */
-
-#define kvmppc_eieio() \
-do {  \
-if (kvm_enabled()) {  \
-asm volatile("eieio" : : : "memory"); \
-} \
-} while (0)
-
-/* Store data cache blocks back to memory */
-static inline void kvmppc_dcbst_range(PowerPCCPU *cpu, uint8_t *addr, int len)
-{
-uint8_t *p;
-
-for (p = addr; p < addr + len; p += cpu->env.dcache_line_size) {
-asm volatile("dcbst 0,%0" : : "r"(p) : "memory");
-}
-}
-
-/* Invalidate instruction cache blocks */
-static inline void kvmppc_icbi_range(PowerPCCPU *cpu, uint8_t *addr, int len)
-{
-uint8_t *p;
-
-for (p = addr; p < addr + len; p += cpu->env.icache_line_size) {
-asm volatile("icbi 0,%0" : : "r"(p));
-}
-}
-
 #endif  /* CONFIG_KVM */
 
 #endif /* KVM_PPC_H */
-- 
2.38.1

[PATCH v3 0/6] target/ppc: Few cleanups in kvm_ppc.h

[Philippe Mathieu-Daudé]

PPC specific changes of a bigger KVM cleanup, remove "kvm_ppc.h"
from user emulation. Mostly trivial IMO.

Philippe Mathieu-Daudé (6):
  target/ppc: Have 'kvm_ppc.h' include 'sysemu/kvm.h'
  target/ppc: Reorder #ifdef'ry in kvm_ppc.h
  target/ppc: Move CPU QOM definitions to cpu-qom.h
  target/ppc: Define TYPE_HOST_POWERPC_CPU in cpu-qom.h
  target/ppc: Restrict 'kvm_ppc.h' to sysemu in cpu_init.c
  target/ppc: Remove pointless checks of CONFIG_USER_ONLY in 'kvm_ppc.h'

 target/ppc/cpu-qom.h  |  7 +
 target/ppc/cpu.h  |  6 
 target/ppc/kvm_ppc.h  | 70 ++-
 target/ppc/cpu_init.c |  2 +-
 4 files changed, 37 insertions(+), 48 deletions(-)

-- 
2.38.1

[PATCH v3 3/6] target/ppc: Move CPU QOM definitions to cpu-qom.h

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/ppc/cpu-qom.h | 5 +
 target/ppc/cpu.h | 6 --
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
index 9666f54f65..c2bff349cc 100644
--- a/target/ppc/cpu-qom.h
+++ b/target/ppc/cpu-qom.h
@@ -31,6 +31,11 @@
 
 OBJECT_DECLARE_CPU_TYPE(PowerPCCPU, PowerPCCPUClass, POWERPC_CPU)
 
+#define POWERPC_CPU_TYPE_SUFFIX "-" TYPE_POWERPC_CPU
+#define POWERPC_CPU_TYPE_NAME(model) model POWERPC_CPU_TYPE_SUFFIX
+#define CPU_RESOLVING_TYPE TYPE_POWERPC_CPU
+#define cpu_list ppc_cpu_list
+
 ObjectClass *ppc_cpu_class_by_name(const char *name);
 
 typedef struct CPUArchState CPUPPCState;
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index af12c93ebc..e91e1774e5 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1468,12 +1468,6 @@ static inline uint64_t ppc_dump_gpr(CPUPPCState *env, 
int gprn)
 int ppc_dcr_read(ppc_dcr_t *dcr_env, int dcrn, uint32_t *valp);
 int ppc_dcr_write(ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
 
-#define POWERPC_CPU_TYPE_SUFFIX "-" TYPE_POWERPC_CPU
-#define POWERPC_CPU_TYPE_NAME(model) model POWERPC_CPU_TYPE_SUFFIX
-#define CPU_RESOLVING_TYPE TYPE_POWERPC_CPU
-
-#define cpu_list ppc_cpu_list
-
 /* MMU modes definitions */
 #define MMU_USER_IDX 0
 static inline int cpu_mmu_index(CPUPPCState *env, bool ifetch)
-- 
2.38.1

[PATCH v3 4/6] target/ppc: Define TYPE_HOST_POWERPC_CPU in cpu-qom.h

[Philippe Mathieu-Daudé]

TYPE_HOST_POWERPC_CPU is used in various places of cpu_init.c,
in order to restrict "kvm_ppc.h" to sysemu, move this QOM-related
definition to cpu-qom.h.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/ppc/cpu-qom.h | 2 ++
 target/ppc/kvm_ppc.h | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
index c2bff349cc..4e4061068e 100644
--- a/target/ppc/cpu-qom.h
+++ b/target/ppc/cpu-qom.h
@@ -36,6 +36,8 @@ OBJECT_DECLARE_CPU_TYPE(PowerPCCPU, PowerPCCPUClass, 
POWERPC_CPU)
 #define CPU_RESOLVING_TYPE TYPE_POWERPC_CPU
 #define cpu_list ppc_cpu_list
 
+#define TYPE_HOST_POWERPC_CPU POWERPC_CPU_TYPE_NAME("host")
+
 ObjectClass *ppc_cpu_class_by_name(const char *name);
 
 typedef struct CPUArchState CPUPPCState;
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 49954a300b..901e188c9a 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -13,8 +13,6 @@
 #include "exec/hwaddr.h"
 #include "cpu.h"
 
-#define TYPE_HOST_POWERPC_CPU POWERPC_CPU_TYPE_NAME("host")
-
 #ifdef CONFIG_KVM
 
 uint32_t kvmppc_get_tbfreq(void);
-- 
2.38.1

[PATCH v3 6/6] target/ppc: Remove pointless checks of CONFIG_USER_ONLY in 'kvm_ppc.h'

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/ppc/kvm_ppc.h | 5 -
 1 file changed, 5 deletions(-)

diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 901e188c9a..6a4dd9c560 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -42,7 +42,6 @@ int kvmppc_booke_watchdog_enable(PowerPCCPU *cpu);
 target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
  bool radix, bool gtse,
  uint64_t proc_tbl);
-#ifndef CONFIG_USER_ONLY
 bool kvmppc_spapr_use_multitce(void);
 int kvmppc_spapr_enable_inkernel_multitce(void);
 void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t page_shift,
@@ -52,7 +51,6 @@ int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t 
window_size);
 int kvmppc_reset_htab(int shift_hint);
 uint64_t kvmppc_vrma_limit(unsigned int hash_shift);
 bool kvmppc_has_cap_spapr_vfio(void);
-#endif /* !CONFIG_USER_ONLY */
 bool kvmppc_has_cap_epr(void);
 int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);
 int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp);
@@ -262,7 +260,6 @@ static inline void kvmppc_set_reg_tb_offset(PowerPCCPU 
*cpu, int64_t tb_offset)
 {
 }
 
-#ifndef CONFIG_USER_ONLY
 static inline bool kvmppc_spapr_use_multitce(void)
 {
 return false;
@@ -322,8 +319,6 @@ static inline void kvmppc_write_hpte(hwaddr ptex, uint64_t 
pte0, uint64_t pte1)
 abort();
 }
 
-#endif /* !CONFIG_USER_ONLY */
-
 static inline bool kvmppc_has_cap_epr(void)
 {
 return false;
-- 
2.38.1

[PATCH v3 5/6] target/ppc: Restrict 'kvm_ppc.h' to sysemu in cpu_init.c

[Philippe Mathieu-Daudé]

User emulation shouldn't need any of the KVM prototypes
declared in "kvm_ppc.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/ppc/cpu_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index aeff71d063..f2afb539eb 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -21,7 +21,6 @@
 #include "qemu/osdep.h"
 #include "disas/dis-asm.h"
 #include "gdbstub/helpers.h"
-#include "kvm_ppc.h"
 #include "sysemu/cpus.h"
 #include "sysemu/hw_accel.h"
 #include "sysemu/tcg.h"
@@ -49,6 +48,7 @@
 #ifndef CONFIG_USER_ONLY
 #include "hw/boards.h"
 #include "hw/intc/intc.h"
+#include "kvm_ppc.h"
 #endif
 
 /* #define PPC_DEBUG_SPR */
-- 
2.38.1

Re: [PATCH v2 2/4] pc-bios/s390-ccw: Provide space for initial stack frame in start.S

[Thomas Huth]

On 27/06/2023 11.14, Philippe Mathieu-Daudé wrote:

On 27/6/23 10:26, Marc Hartmayer wrote:

Thomas Huth  writes:


Providing the space of a stack frame is the duty of the caller,
so we should reserve 160 bytes before jumping into the main function.
Otherwise the main() function might write past the stack array.

While we're at it, add a proper STACK_SIZE macro for the stack size
instead of using magic numbers (this is also required for the following
patch).

Reviewed-by: Christian Borntraeger 
Reviewed-by: Cédric Le Goater 
Signed-off-by: Thomas Huth 
---
  pc-bios/s390-ccw/start.S | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)




+#define STACK_SIZE 0x8000
+
  .globl _start
  _start:
-    larl    %r15,stack + 0x8000 /* Set up stack */
+    larl    %r15,stack + STACK_SIZE - 160   /* Set up stack */

  ^^^
  You can also add a macro for this
  - e.g. STACK_FRAME_SIZE.


Yes please :) No need to respin.


Ok, I'll add:

diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -10,12 +10,13 @@
  * directory.
  */
 
-#define STACK_SIZE 0x8000

+#define STACK_SIZE0x8000
+#define STACK_FRAME_SIZE  160
 
 .globl _start

 _start:
 
-larl%r15,stack + STACK_SIZE - 160   /* Set up stack */

+larl%r15,stack + STACK_SIZE - STACK_FRAME_SIZE   /* Set up stack */
 
 /* clear bss */

 larl%r2,.bss

Thanks,
 Thomas

Re: [RESEND PATCH v5 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Michael S. Tsirkin]

On Tue, Jun 27, 2023 at 03:23:04PM +0530, Ani Sinha wrote:
> 
> 
> > On 27-Jun-2023, at 2:32 PM, Igor Mammedov  wrote:
> > 
> > On Mon, 26 Jun 2023 21:42:44 +0530
> > Ani Sinha  wrote:
> > 
> >> PCI Express ports only have one slot, so PCI Express devices can only be
> >> plugged into slot 0 on a PCIE port. Enforce it.
> > 
> > btw, previously you mentioned ARI.
> > So if we turn it on, wouldn't this patch actually become regression?
> 
> If ARI breaks this, it will break other areas in QEMU too, ex anywhere 
> pci_get_function_0() is used.

We will just fix pci_get_function_0.

> Regardless, I think at least the tests are worth fixing, particularly the 
> mess with hd-geo-test.

ok

> > 
> >> 
> >> CC: jus...@redhat.com
> >> CC: imamm...@redhat.com
> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
> >> Signed-off-by: Ani Sinha 
> >> Reviewed-by: Julia Suvorova 
> >> ---
> >> hw/pci/pci.c | 6 ++
> >> 1 file changed, 6 insertions(+)
> >> 
> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> index bf38905b7d..426af133b0 100644
> >> --- a/hw/pci/pci.c
> >> +++ b/hw/pci/pci.c
> >> @@ -64,6 +64,7 @@ bool pci_available = true;
> >> static char *pcibus_get_dev_path(DeviceState *dev);
> >> static char *pcibus_get_fw_dev_path(DeviceState *dev);
> >> static void pcibus_reset(BusState *qbus);
> >> +static bool pcie_has_upstream_port(PCIDevice *dev);
> >> 
> >> static Property pci_props[] = {
> >> DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> >> @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> >> *pci_dev,
> >>name);
> >> 
> >>return NULL;
> >> +} else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> >> +error_setg(errp, "PCI: slot %d is not valid for %s,"
> >> +   " parent device only allows plugging into slot 0.",
> >> +   PCI_SLOT(devfn), name);
> >> +return NULL;
> >> }
> >> 
> >> pci_dev->devfn = devfn;
> > 

Re: [PATCH 7/7] target/i386: Add new CPU model GraniteRapids

[Igor Mammedov]

On Fri, 16 Jun 2023 11:23:11 +0800
Tao Su  wrote:

> The GraniteRapids CPU model mainly adds the following new features based
> on SapphireRapids:
> 
> - PREFETCHITI CPUID.(EAX=7,ECX=1):EDX[bit 14]
> - AMX-FP16 CPUID.(EAX=7,ECX=1):EAX[bit 21]
> - MCDT_NO CPUID.(EAX=7,ECX=2):EDX[bit 5]
> - SBDR_SSDP_NO MSR_IA32_ARCH_CAPABILITIES[bit 13]
> - FBSDP_NO MSR_IA32_ARCH_CAPABILITIES[bit 14]
> - PSDP_NO MSR_IA32_ARCH_CAPABILITIES[bit 15]
> - PBRSB_NO MSR_IA32_ARCH_CAPABILITIES[bit 24]

Can you point me to a some doc where above features
are are documented as being introduced by GraniteRapids?

 
> Signed-off-by: Tao Su 
> Tested-by: Xuelian Guo 
> Reviewed-by: Xiaoyao Li 
> ---
>  target/i386/cpu.c | 136 ++
>  1 file changed, 136 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 7faf6dfaee..860106fc24 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3993,6 +3993,142 @@ static const X86CPUDefinition builtin_x86_defs[] = {
>  { /* end of list */ },
>  },
>  },
> +{
> +.name = "GraniteRapids",
> +.level = 0x20,
> +.vendor = CPUID_VENDOR_INTEL,
> +.family = 6,
> +.model = 173,
> +.stepping = 0,
> +/*
> + * please keep the ascending order so that we can have a clear view 
> of
> + * bit position of each feature.
> + */
> +.features[FEAT_1_EDX] =
> +CPUID_FP87 | CPUID_VME | CPUID_DE | CPUID_PSE | CPUID_TSC |
> +CPUID_MSR | CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC |
> +CPUID_SEP | CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV |
> +CPUID_PAT | CPUID_PSE36 | CPUID_CLFLUSH | CPUID_MMX | CPUID_FXSR 
> |
> +CPUID_SSE | CPUID_SSE2,
> +.features[FEAT_1_ECX] =
> +CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSSE3 |
> +CPUID_EXT_FMA | CPUID_EXT_CX16 | CPUID_EXT_PCID | 
> CPUID_EXT_SSE41 |
> +CPUID_EXT_SSE42 | CPUID_EXT_X2APIC | CPUID_EXT_MOVBE |
> +CPUID_EXT_POPCNT | CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_AES |
> +CPUID_EXT_XSAVE | CPUID_EXT_AVX | CPUID_EXT_F16C | 
> CPUID_EXT_RDRAND,
> +.features[FEAT_8000_0001_EDX] =
> +CPUID_EXT2_SYSCALL | CPUID_EXT2_NX | CPUID_EXT2_PDPE1GB |
> +CPUID_EXT2_RDTSCP | CPUID_EXT2_LM,
> +.features[FEAT_8000_0001_ECX] =
> +CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM | CPUID_EXT3_3DNOWPREFETCH,
> +.features[FEAT_8000_0008_EBX] =
> +CPUID_8000_0008_EBX_WBNOINVD,
> +.features[FEAT_7_0_EBX] =
> +CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_HLE |
> +CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 |
> +CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID | CPUID_7_0_EBX_RTM |
> +CPUID_7_0_EBX_AVX512F | CPUID_7_0_EBX_AVX512DQ |
> +CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX | CPUID_7_0_EBX_SMAP |
> +CPUID_7_0_EBX_AVX512IFMA | CPUID_7_0_EBX_CLFLUSHOPT |
> +CPUID_7_0_EBX_CLWB | CPUID_7_0_EBX_AVX512CD | 
> CPUID_7_0_EBX_SHA_NI |
> +CPUID_7_0_EBX_AVX512BW | CPUID_7_0_EBX_AVX512VL,
> +.features[FEAT_7_0_ECX] =
> +CPUID_7_0_ECX_AVX512_VBMI | CPUID_7_0_ECX_UMIP | 
> CPUID_7_0_ECX_PKU |
> +CPUID_7_0_ECX_AVX512_VBMI2 | CPUID_7_0_ECX_GFNI |
> +CPUID_7_0_ECX_VAES | CPUID_7_0_ECX_VPCLMULQDQ |
> +CPUID_7_0_ECX_AVX512VNNI | CPUID_7_0_ECX_AVX512BITALG |
> +CPUID_7_0_ECX_AVX512_VPOPCNTDQ | CPUID_7_0_ECX_LA57 |
> +CPUID_7_0_ECX_RDPID | CPUID_7_0_ECX_BUS_LOCK_DETECT,
> +.features[FEAT_7_0_EDX] =
> +CPUID_7_0_EDX_FSRM | CPUID_7_0_EDX_SERIALIZE |
> +CPUID_7_0_EDX_TSX_LDTRK | CPUID_7_0_EDX_AMX_BF16 |
> +CPUID_7_0_EDX_AVX512_FP16 | CPUID_7_0_EDX_AMX_TILE |
> +CPUID_7_0_EDX_AMX_INT8 | CPUID_7_0_EDX_SPEC_CTRL |
> +CPUID_7_0_EDX_ARCH_CAPABILITIES | CPUID_7_0_EDX_SPEC_CTRL_SSBD,
> +.features[FEAT_ARCH_CAPABILITIES] =
> +MSR_ARCH_CAP_RDCL_NO | MSR_ARCH_CAP_IBRS_ALL |
> +MSR_ARCH_CAP_SKIP_L1DFL_VMENTRY | MSR_ARCH_CAP_MDS_NO |
> +MSR_ARCH_CAP_PSCHANGE_MC_NO | MSR_ARCH_CAP_TAA_NO |
> +MSR_ARCH_CAP_SBDR_SSDP_NO | MSR_ARCH_CAP_FBSDP_NO |
> +MSR_ARCH_CAP_PSDP_NO | MSR_ARCH_CAP_PBRSB_NO,
> +.features[FEAT_XSAVE] =
> +CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
> +CPUID_XSAVE_XGETBV1 | CPUID_XSAVE_XSAVES | CPUID_D_1_EAX_XFD,
> +.features[FEAT_6_EAX] =
> +CPUID_6_EAX_ARAT,
> +.features[FEAT_7_1_EAX] =
> +CPUID_7_1_EAX_AVX_VNNI | CPUID_7_1_EAX_AVX512_BF16 |
> +CPUID_7_1_EAX_FZRM | CPUID_7_1_EAX_FSRS | CPUID_7_1_EAX_FSRC |
> +CPUID_7_1_EAX_AMX_FP16,
> +.features[FEAT_7_1_EDX] =
> +CPUID_7_1_EDX_PREFE

Re: [RESEND PATCH v5 5/5] hw/pci: ensure PCIE devices are plugged into only slot 0 of PCIE port

[Ani Sinha]

> On 27-Jun-2023, at 3:23 PM, Ani Sinha  wrote:
> 
> 
> 
>> On 27-Jun-2023, at 2:32 PM, Igor Mammedov  wrote:
>> 
>> On Mon, 26 Jun 2023 21:42:44 +0530
>> Ani Sinha  wrote:
>> 
>>> PCI Express ports only have one slot, so PCI Express devices can only be
>>> plugged into slot 0 on a PCIE port. Enforce it.
>> 
>> btw, previously you mentioned ARI.
>> So if we turn it on, wouldn't this patch actually become regression?

Looking at 
https://pcisig.com/sites/default/files/specification_documents/ECN-alt-rid-interpretation-070604.pdf,
 section 7.23, seems a root port does not support ARI but it can support ARI 
forwarding capability (section 7.8.5).
Also with ARI enabled, the device cannot have a non-zero device number. Also, 
shouldn't any code path that uses PCI_SLOT() should probably also check for ARI 
if it wants to be ARI complaint?

Anyways these are just facts I could find but I am not sure if this would 
answer your above question.

> 
> If ARI breaks this, it will break other areas in QEMU too, ex anywhere 
> pci_get_function_0() is used.
> Regardless, I think at least the tests are worth fixing, particularly the 
> mess with hd-geo-test.
> 
>> 
>>> 
>>> CC: jus...@redhat.com
>>> CC: imamm...@redhat.com
>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
>>> Signed-off-by: Ani Sinha 
>>> Reviewed-by: Julia Suvorova 
>>> ---
>>> hw/pci/pci.c | 6 ++
>>> 1 file changed, 6 insertions(+)
>>> 
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index bf38905b7d..426af133b0 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -64,6 +64,7 @@ bool pci_available = true;
>>> static char *pcibus_get_dev_path(DeviceState *dev);
>>> static char *pcibus_get_fw_dev_path(DeviceState *dev);
>>> static void pcibus_reset(BusState *qbus);
>>> +static bool pcie_has_upstream_port(PCIDevice *dev);
>>> 
>>> static Property pci_props[] = {
>>>DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>>> @@ -1189,6 +1190,11 @@ static PCIDevice *do_pci_register_device(PCIDevice 
>>> *pci_dev,
>>>   name);
>>> 
>>>   return NULL;
>>> +} else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
>>> +error_setg(errp, "PCI: slot %d is not valid for %s,"
>>> +   " parent device only allows plugging into slot 0.",
>>> +   PCI_SLOT(devfn), name);
>>> +return NULL;
>>>}
>>> 
>>>pci_dev->devfn = devfn;
>> 
> 

Re: [PATCH v2 0/8] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Richard Henderson]

On 6/27/23 13:19, Mark Cave-Ayland wrote:

On 27/06/2023 10:37, Philippe Mathieu-Daudé wrote:


On 27/6/23 08:46, Mark Cave-Ayland wrote:

On 22/06/2023 13:26, Mark Cave-Ayland wrote:


On 21/06/2023 19:05, Richard Henderson wrote:


Changes from v1:
   * Split into teeny weeny pieces.

   * It turns out the sparc_tr_tb_stop hunk of v1 was buggy,
 in that things that are not simple branches use DYNAMIC_PC,
 e.g. the RETT (return from trap) instruction.

 Introduce DYNAMIC_PC_LOOKUP to distinguish the couple of
 places where we have a dynamic pc, but no other change
 of state (conditional branches, JMPL, RETURN).

   * Drop the change for WRFPRS, because it's too infrequent.
 The WRASI change affects memcpy/memset, so that's more important.

Boots Mark's sol8 install cdrom.  :-)

Top of the profile changes from

 41.55%  qemu-system-sparc  [.] cpu_exec_loop
 14.02%  qemu-system-sparc  [.] cpu_tb_exec
  8.74%  qemu-system-sparc  [.] tb_lookup
  2.11%  qemu-system-sparc  [.] tcg_splitwx_to_rw
  1.63%  memfd:tcg-jit (deleted)    [.] 0x0004

to

 31.59%  qemu-system-sparc  [.] helper_lookup_tb_ptr
 17.79%  qemu-system-sparc  [.] tb_lookup
  5.38%  qemu-system-sparc  [.] compute_all_sub
  2.38%  qemu-system-sparc  [.] helper_compute_psr
  2.36%  qemu-system-sparc  [.] helper_check_align
  1.79%  memfd:tcg-jit (deleted)    [.] 0x0063fc8e

This probably indicates that cpu_get_tb_cpu_state could be
improved to not consume so much overhead.


Nice! I've just run this through all of my sun4m/sun4u/sun4v test images and I don't 
see any regressions with v2. The guests feel noticeably more responsive too :)


Tested-by: Mark Cave-Ayland 

I've skimmed the patches and without looking in too much detail they seem to be okay 
so I'm happy to give:


Acked-by: Mark Cave-Ayland 

Side note: the niagara tests require the patch at 
https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03537.html which still hasn't 
been merged yet.



Richard Henderson (8):
   target/sparc: Use tcg_gen_lookup_and_goto_ptr in gen_goto_tb
   target/sparc: Fix npc comparison in sparc_tr_insn_start
   target/sparc: Drop inline markers from translate.c
   target/sparc: Introduce DYNAMIC_PC_LOOKUP
   target/sparc: Use DYNAMIC_PC_LOOKUP for conditional branches
   target/sparc: Use DYNAMIC_PC_LOOKUP for JMPL
   target/sparc: Use DYNAMIC_PC_LOOKUP for v9 RETURN
   target/sparc: Use tcg_gen_lookup_and_goto_ptr for v9 WRASI

  target/sparc/translate.c | 410 ++-
  1 file changed, 233 insertions(+), 177 deletions(-)


I've just noticed during testing there is an issue with this series when used with a 
real SS-5 PROM image (I was using OpenBIOS for my previous tests) which causes it to 
assert() almost immediately on startup:


$ ./qemu-system-sparc -bios ss5.bin
ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code should not be 
reached
Bail out! ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code should not be 
reached

Aborted


Could you try this fix:

-- >8 --
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -5682,5 +5682,5 @@ static void sparc_tr_tb_stop(DisasContextBase *dcbase, 
CPUState *cs)

  save_npc(dc);
-    switch (dc->npc) {
+    switch (dc->npc & 3) {
  case DYNAMIC_PC_LOOKUP:
  if (may_lookup) {
---

?


Unfortunately that doesn't fix the issue. A quick lunchtime debugging session with 
printf() shows this just before the assert() fires:


### dc->pc: 0x3
### dc->npc: 0xffd26c70
**
ERROR:../target/sparc/translate.c:5699:sparc_tr_tb_stop: code should not be 
reached
Bail out! ERROR:../target/sparc/translate.c:5699:sparc_tr_tb_stop: code should 
not be reached
Aborted


That makes no sense -- dynamic lookup pc with static npc?

Of course this happens before in_asm dump, so can you use -singlestep to figure out what 
pc, and thence instruction for which this is happening?



r~