Re: [PATCH] tests/qtest: Plug memory leaks in qtest_get_machines
On 20/01/2023 20.44, Fabiano Rosas wrote:
These leaks can be avoided:
759 bytes in 61 blocks are still reachable in loss record 56 of 60
at 0x4034744: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x4A88518: g_malloc (in /usr/lib64/libglib-2.0.so.0.7000.5)
by 0x4AA313E: g_strdup (in /usr/lib64/libglib-2.0.so.0.7000.5)
by 0x12083E: qtest_get_machines (libqtest.c:1323)
by 0x12098C: qtest_cb_for_every_machine (libqtest.c:1348)
by 0x11556C: main (test-hmp.c:160)
992 bytes in 1 blocks are still reachable in loss record 57 of 60
at 0x4034744: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x4A88518: g_malloc (in /usr/lib64/libglib-2.0.so.0.7000.5)
by 0x120725: qtest_get_machines (libqtest.c:1313)
by 0x12098C: qtest_cb_for_every_machine (libqtest.c:1348)
by 0x11556C: main (test-hmp.c:160)
Signed-off-by: Fabiano Rosas
---
tests/qtest/libqtest.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 6b2216cb20..65abac5029 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1285,6 +1285,18 @@ struct MachInfo {
char *alias;
};
+static void qtest_free_machine_info(gpointer data)
+{
+struct MachInfo *machines = data;
+int i;
+
+for (i = 0; machines[i].name != NULL; i++) {
+g_free((void *)machines[i].name); > +g_free((void
*)machines[i].alias);
I'd suggest setting .name and .alias to NULL after freeing them, to avoid
that danling pointers are left behind.
+}
+g_free(machines);
+}
+
/*
* Returns an array with pointers to the available machine names.
* The terminating entry has the name set to NULL.
@@ -1336,6 +1348,8 @@ static struct MachInfo *qtest_get_machines(void)
qobject_unref(response);
memset(&machines[idx], 0, sizeof(struct MachInfo)); /* Terminating entry */
+g_test_queue_destroy(qtest_free_machine_info, machines);
So this frees the machines structure...
return machines;
... but here it gets returned, too? ... that looks wrong. Did you maybe
rather want to free it at the end of qtest_cb_for_every_machine() and
qtest_has_machine ?
Thomas
}
Re: [PATCH v2 11/11] alsaaudio: reintroduce default recording settings
On 21/1/23 10:47, Volker Rümelin wrote:
Audio recording with ALSA default settings currently doesn't
work. The debug log shows updates every 0.75s and 1.5s.
audio: Elapsed since last alsa run (running): 0.743030
audio: Elapsed since last alsa run (running): 1.486048
audio: Elapsed since last alsa run (running): 0.743008
audio: Elapsed since last alsa run (running): 1.485878
audio: Elapsed since last alsa run (running): 1.486040
audio: Elapsed since last alsa run (running): 1.485886
The time between updates should be in the 10ms range. Audio
recording with ALSA has the same timing contraints as playback.
Reintroduce the default recording settings and use the same
default settings for recording as for playback.
The term "reintroduce" is correct because commit a93f328177
("alsaaudio: port to -audiodev config") removed the default
settings for recording.
Signed-off-by: Volker Rümelin
---
audio/alsaaudio.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
-/*
- * OptsVisitor sets unspecified optional fields to zero, but do not depend
- * on it...
- */
if (!dev->u.alsa.in->has_period_length) {
-dev->u.alsa.in->period_length = 0;
+/* 256 frames assuming 44100Hz */
+dev->u.alsa.in->period_length = 5805;
}
if (!dev->u.alsa.in->has_buffer_length) {
-dev->u.alsa.in->buffer_length = 0;
+/* 4096 frames assuming 44100Hz */
+dev->u.alsa.in->buffer_length = 92880;
}
Please use DIV_ROUND_UP(). Maybe worth adding definitions?
Re: [PATCH v2 10/11] alsaaudio: change default playback settings
On 21/1/23 10:47, Volker Rümelin wrote:
The currently used default playback settings in the ALSA audio
backend are a bit unfortunate. With a few emulated audio devices,
audio playback does not work properly. Here is a short part of
the debug log while audio is playing (elapsed time in seconds).
audio: Elapsed since last alsa run (running): 0.046244
audio: Elapsed since last alsa run (running): 0.023137
audio: Elapsed since last alsa run (running): 0.023170
audio: Elapsed since last alsa run (running): 0.023650
audio: Elapsed since last alsa run (running): 0.060802
audio: Elapsed since last alsa run (running): 0.031931
For some audio devices the time of more than 23ms between updates
is too long.
Set the period time to 5.8ms so that the maximum time between
two updates typically does not exceed 11ms. This roughly matches
the 10ms period time when doing playback with the audio timer.
After this patch the debug log looks like this.
audio: Elapsed since last alsa run (running): 0.011919
audio: Elapsed since last alsa run (running): 0.005788
audio: Elapsed since last alsa run (running): 0.005995
audio: Elapsed since last alsa run (running): 0.011069
audio: Elapsed since last alsa run (running): 0.005901
audio: Elapsed since last alsa run (running): 0.006084
Acked-by: Christian Schoenebeck
Signed-off-by: Volker Rümelin
---
audio/alsaaudio.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c
index 5f50dfa0bf..0cc982e61f 100644
--- a/audio/alsaaudio.c
+++ b/audio/alsaaudio.c
@@ -913,17 +913,14 @@ static void *alsa_audio_init(Audiodev *dev)
alsa_init_per_direction(aopts->in);
alsa_init_per_direction(aopts->out);
-/*
- * need to define them, as otherwise alsa produces no sound
- * doesn't set has_* so alsa_open can identify it wasn't set by the user
- */
+/* don't set has_* so alsa_open can identify it wasn't set by the user */
if (!dev->u.alsa.out->has_period_length) {
-/* 1024 frames assuming 44100Hz */
-dev->u.alsa.out->period_length = 1024 * 100 / 44100;
+/* 256 frames assuming 44100Hz */
+dev->u.alsa.out->period_length = 5805;
Please use DIV_ROUND_UP():
DIV_ROUND_UP(100ul << 8, 44100);
Or
DIV_ROUND_UP(512 * 100ul, 44100);
}
if (!dev->u.alsa.out->has_buffer_length) {
/* 4096 frames assuming 44100Hz */
-dev->u.alsa.out->buffer_length = 4096ll * 100 / 44100;
+dev->u.alsa.out->buffer_length = 92880;
Ditto:
DIV_ROUND_UP(100ul << 12, 44100);
}
/*
Re: [PATCH v2 03/11] audio: rename hardware store to backend
On 21/1/23 10:47, Volker Rümelin wrote: Use a consistent friendly name for the HWVoiceOut and HWVoiceIn structures. Reviewed-by: Thomas Huth Signed-off-by: Volker Rümelin --- audio/audio_template.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v3 1/5] hw/char/pl011: refactor FIFO depth handling code
On 20/1/23 16:54, Evgeny Iakovlev wrote: PL011 can be in either of 2 modes depending guest config: FIFO and single register. The last mode could be viewed as a 1-element-deep FIFO. Current code open-codes a bunch of depth-dependent logic. Refactor FIFO depth handling code to isolate calculating current FIFO depth. One functional (albeit guest-invisible) side-effect of this change is that previously we would always increment s->read_pos in UARTDR read handler even if FIFO was disabled, now we are limiting read_pos to not exceed FIFO depth (read_pos itself is reset to 0 if user disables FIFO). Signed-off-by: Evgeny Iakovlev --- hw/char/pl011.c | 30 ++ include/hw/char/pl011.h | 5 - 2 files changed, 22 insertions(+), 13 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v3 3/5] hw/char/pl011: implement a reset method
On 20/1/23 16:54, Evgeny Iakovlev wrote:
PL011 currently lacks a reset method. Implement it.
Signed-off-by: Evgeny Iakovlev
---
hw/char/pl011.c | 26 +-
1 file changed, 21 insertions(+), 5 deletions(-)
static void pl011_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = pl011_realize;
+dc->reset = pl011_reset;
Maybe directly implement as ResettableHoldPhase from "hw/resettable.h".
Reviewed-by: Philippe Mathieu-Daudé
[PATCH v2] include/hw/riscv/opentitan: update opentitan IRQs
From: Wilfred Mallawa
Updates the opentitan IRQs to match the latest supported commit of
Opentitan from TockOS.
OPENTITAN_SUPPORTED_SHA := 565e4af39760a123c59a184aa2f5812a961fde47
Memory layout as per [1]
[1]
https://github.com/lowRISC/opentitan/blob/565e4af39760a123c59a184aa2f5812a961fde47/hw/top_earlgrey/sw/autogen/top_earlgrey_memory.h
Signed-off-by: Wilfred Mallawa
---
Changes in v2:
- Updated the MMIO register layout/size
- Bumped the supported commit sha
- Added link to OT register layout for reference in the commit
msg
hw/riscv/opentitan.c | 80 ++--
include/hw/riscv/opentitan.h | 14 +++
2 files changed, 47 insertions(+), 47 deletions(-)
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index 64d5d435b9..353f030d80 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -31,47 +31,47 @@
/*
* This version of the OpenTitan machine currently supports
* OpenTitan RTL version:
- *
+ *
*
* MMIO mapping as per (specified commit):
* lowRISC/opentitan: hw/top_earlgrey/sw/autogen/top_earlgrey_memory.h
*/
static const MemMapEntry ibex_memmap[] = {
-[IBEX_DEV_ROM] ={ 0x8000, 0x8000 },
-[IBEX_DEV_RAM] ={ 0x1000, 0x2 },
-[IBEX_DEV_FLASH] = { 0x2000, 0x10 },
-[IBEX_DEV_UART] = { 0x4000, 0x1000 },
-[IBEX_DEV_GPIO] = { 0x4004, 0x1000 },
-[IBEX_DEV_SPI_DEVICE] = { 0x4005, 0x1000 },
-[IBEX_DEV_I2C] ={ 0x4008, 0x1000 },
-[IBEX_DEV_PATTGEN] ={ 0x400e, 0x1000 },
-[IBEX_DEV_TIMER] = { 0x4010, 0x1000 },
-[IBEX_DEV_OTP_CTRL] = { 0x4013, 0x4000 },
-[IBEX_DEV_LC_CTRL] ={ 0x4014, 0x1000 },
-[IBEX_DEV_ALERT_HANDLER] = { 0x4015, 0x1000 },
-[IBEX_DEV_SPI_HOST0] = { 0x4030, 0x1000 },
-[IBEX_DEV_SPI_HOST1] = { 0x4031, 0x1000 },
-[IBEX_DEV_USBDEV] = { 0x4032, 0x1000 },
-[IBEX_DEV_PWRMGR] = { 0x4040, 0x1000 },
-[IBEX_DEV_RSTMGR] = { 0x4041, 0x1000 },
-[IBEX_DEV_CLKMGR] = { 0x4042, 0x1000 },
-[IBEX_DEV_PINMUX] = { 0x4046, 0x1000 },
-[IBEX_DEV_AON_TIMER] = { 0x4047, 0x1000 },
-[IBEX_DEV_SENSOR_CTRL] ={ 0x4049, 0x1000 },
-[IBEX_DEV_FLASH_CTRL] = { 0x4100, 0x1000 },
-[IBEX_DEV_AES] ={ 0x4110, 0x1000 },
-[IBEX_DEV_HMAC] = { 0x4111, 0x1000 },
-[IBEX_DEV_KMAC] = { 0x4112, 0x1000 },
-[IBEX_DEV_OTBN] = { 0x4113, 0x1 },
-[IBEX_DEV_KEYMGR] = { 0x4114, 0x1000 },
-[IBEX_DEV_CSRNG] = { 0x4115, 0x1000 },
-[IBEX_DEV_ENTROPY] ={ 0x4116, 0x1000 },
-[IBEX_DEV_EDNO] = { 0x4117, 0x1000 },
-[IBEX_DEV_EDN1] = { 0x4118, 0x1000 },
-[IBEX_DEV_NMI_GEN] ={ 0x411c, 0x1000 },
-[IBEX_DEV_PERI] = { 0x411f, 0x1 },
-[IBEX_DEV_PLIC] = { 0x4800, 0x4005000 },
-[IBEX_DEV_FLASH_VIRTUAL] = { 0x8000, 0x8 },
+[IBEX_DEV_ROM] ={ 0x8000, 0x8000 },
+[IBEX_DEV_RAM] ={ 0x1000, 0x2 },
+[IBEX_DEV_FLASH] = { 0x2000, 0x10},
+[IBEX_DEV_UART] = { 0x4000, 0x40},
+[IBEX_DEV_GPIO] = { 0x4004, 0x40},
+[IBEX_DEV_SPI_DEVICE] = { 0x4005, 0x2000 },
+[IBEX_DEV_I2C] ={ 0x4008, 0x80},
+[IBEX_DEV_PATTGEN] ={ 0x400e, 0x40},
+[IBEX_DEV_TIMER] = { 0x4010, 0x200 },
+[IBEX_DEV_OTP_CTRL] = { 0x4013, 0x2000 },
+[IBEX_DEV_LC_CTRL] ={ 0x4014, 0x100 },
+[IBEX_DEV_ALERT_HANDLER] = { 0x4015, 0x800 },
+[IBEX_DEV_SPI_HOST0] = { 0x4030, 0x40},
+[IBEX_DEV_SPI_HOST1] = { 0x4031, 0x40},
+[IBEX_DEV_USBDEV] = { 0x4032, 0x1000 },
+[IBEX_DEV_PWRMGR] = { 0x4040, 0x80},
+[IBEX_DEV_RSTMGR] = { 0x4041, 0x80},
+[IBEX_DEV_CLKMGR] = { 0x4042, 0x80},
+[IBEX_DEV_PINMUX] = { 0x4046, 0x1000 },
+[IBEX_DEV_AON_TIMER] = { 0x4047, 0x40},
+[IBEX_DEV_SENSOR_CTRL] ={ 0x4049, 0x40},
+[IBEX_DEV_FLASH_CTRL] = { 0x4100, 0x200 },
+[IBEX_DEV_AES] ={ 0x4110, 0x100 },
+[IBEX_DEV_HMAC] = { 0x4111, 0x1000 },
+[IBEX_DEV_KMAC] = { 0x4112, 0x1000 },
+[IBEX_DEV_OTBN] = { 0x4113, 0x1 },
+[IBEX_DEV_KEYMGR] = { 0x4114, 0x1
Re: [PATCH qemu v3] x86: don't let decompressed kernel image clobber setup_data
Hi Michael, On Tue, Jan 10, 2023 at 12:50:42PM -0500, Michael S. Tsirkin wrote: > On Tue, Jan 10, 2023 at 04:34:49PM +0100, Jason A. Donenfeld wrote: > > Hi Michael, > > > > Could you queue up this patch and mark it as a fix for 7.2.1? It is a > > straight-up bug fix for a 7.2 regression that's now affected several > > users. > > OK. In the future pls cc me if you want me to merge a patch. Thanks! > > > - It has two Tested-by tags on the thread. > > - hpa, the maintainer of the kernel side of this, confirmed on one of > > the various tributary threads that this approach is a correct one. > > - It doesn't introduce any new functionality. > > > > For your convenience, you can grab this out of lore here: > > > > https://lore.kernel.org/lkml/20221230220725.618763-1-ja...@zx2c4.com/ > > > > Or if you want to yolo it: > > > > curl > > https://lore.kernel.org/lkml/20221230220725.618763-1-ja...@zx2c4.com/raw | > > git am -s > > > > It's now sat silent on the mailing list for a while. So let's please get > > this committed and backported so that the bug reports stop coming in. > > This patch still isn't on QEMU's master branch. What happened to it? - Eric
[PATCH] hw/riscv: boot: Don't use CSRs if they are disabled
From: Alistair Francis
If the CSRs and CSR instructions are disabled because the Zicsr
extension isn't enabled then we want to make sure we don't run any CSR
instructions in the boot ROM.
This patches removes the CSR instructions from the reset-vec if the
extension isn't enabled. We replace the instruction with a NOP instead.
Note that we don't do this for the SiFive U machine, as we are modelling
the hardware in that case.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1447
Signed-off-by: Alistair Francis
---
hw/riscv/boot.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 2594276223..cb27798a25 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -356,6 +356,15 @@ void riscv_setup_rom_reset_vec(MachineState *machine,
RISCVHartArrayState *harts
reset_vec[4] = 0x0182b283; /* ld t0, 24(t0) */
}
+if (!harts->harts[0].cfg.ext_icsr) {
+/*
+ * The Zicsr extension has been disabled, so let's ensure we don't
+ * run the CSR instruction. Let's fill the address with a non
+ * compressed nop.
+ */
+reset_vec[2] = 0x0013; /* addi x0, x0, 0 */
+}
+
/* copy in the reset vector in little_endian byte order */
for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
reset_vec[i] = cpu_to_le32(reset_vec[i]);
--
2.39.0
Re: [PATCH] include/hw/riscv/opentitan: update opentitan IRQs
On Mon, Jan 23, 2023 at 10:06 AM Wilfred Mallawa
wrote:
>
> From: Wilfred Mallawa
>
> Updates the opentitan IRQs to match the latest supported commit of
> Opentitan from TockOS.
>
> OPENTITAN_SUPPORTED_SHA := 565e4af39760a123c59a184aa2f5812a961fde47
>
> Signed-off-by: Wilfred Mallawa
Reviewed-by: Alistair Francis
Alistair
> ---
> include/hw/riscv/opentitan.h | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h
> index 7659d1bc5b..235728b9cc 100644
> --- a/include/hw/riscv/opentitan.h
> +++ b/include/hw/riscv/opentitan.h
> @@ -108,11 +108,11 @@ enum {
> IBEX_UART0_RX_BREAK_ERR_IRQ = 6,
> IBEX_UART0_RX_TIMEOUT_IRQ = 7,
> IBEX_UART0_RX_PARITY_ERR_IRQ = 8,
> -IBEX_TIMER_TIMEREXPIRED0_0= 127,
> -IBEX_SPI_HOST0_ERR_IRQ= 134,
> -IBEX_SPI_HOST0_SPI_EVENT_IRQ = 135,
> -IBEX_SPI_HOST1_ERR_IRQ= 136,
> -IBEX_SPI_HOST1_SPI_EVENT_IRQ = 137,
> +IBEX_TIMER_TIMEREXPIRED0_0= 124,
> +IBEX_SPI_HOST0_ERR_IRQ= 131,
> +IBEX_SPI_HOST0_SPI_EVENT_IRQ = 132,
> +IBEX_SPI_HOST1_ERR_IRQ= 133,
> +IBEX_SPI_HOST1_SPI_EVENT_IRQ = 134,
> };
>
> #endif
> --
> 2.39.0
>
>
Re: [PATCH v2 2/2] target/riscv: redirect XVentanaCondOps to use the Zicond functions
On Mon, 23 Jan 2023 at 02:29, Alistair Francis wrote:
>
> On Sat, Jan 21, 2023 at 12:36 PM Philipp Tomsich
> wrote:
> >
> > The Zicond standard extension implements the same instruction
> > semantics as XVentanaCondOps, although using different mnemonics and
> > opcodes.
> >
> > Point XVentanaCondOps to the (newly implemented) Zicond implementation
> > to reduce the future maintenance burden.
> >
> > Also updating MAINTAINERS as trans_xventanacondops.c.inc will not see
> > active maintenance from here forward.
> >
> > Signed-off-by: Philipp Tomsich
> > ---
> >
> > Changes in v2:
> > - Calls into the gen_czero_{eqz,nez} helpers instead of calling
> > trans_czero_{eqz,nez} to bypass the require-check and ensure that
> > XVentanaCondOps can be enabled/disabled independently of Zicond.
> >
> > MAINTAINERS| 2 +-
> > .../insn_trans/trans_xventanacondops.c.inc | 18 +++---
> > 2 files changed, 4 insertions(+), 16 deletions(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ca914c42fa..293a9d1c8c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -305,7 +305,7 @@ F: target/riscv/insn_trans/trans_zicond.c.inc
> > RISC-V XVentanaCondOps extension
> > M: Philipp Tomsich
> > L: qemu-ri...@nongnu.org
> > -S: Supported
> > +S: Odd Fixes
>
> Should this extension be deprecated then?
The extension is out in the wild (as the Ventana Veyron V1 core
implements it), so we shouldn't deprecate it.
However, this now is the thinnest possible layer of implementation
(and will pick up any fixes/updates from Zicond).
I felt that downgrading it to "Odd Fixes" was the right way to
indicate this. Let me know if you would like to handle it
differently.
Philipp.
Re: [PATCH v2 1/2] target/riscv: add Zicond as an experimental extension
On Mon, 23 Jan 2023 at 02:28, Alistair Francis wrote:
>
> On Sat, Jan 21, 2023 at 12:36 PM Philipp Tomsich
> wrote:
> >
> > This implements the Zicond (conditional integer operations) extension,
> > as of version 1.0-draft-20230120 as an experimental extension in QEMU
> > ("x-zicond").
> >
> > The Zicond extension acts as a building block for branchless sequences
> > including conditional-{arithmetic,logic,select,move}. Refer to the
> > specification for usage scenarios and application guidance.
> >
> > The following instructions constitute Zicond:
> > - czero.eqz rd, rs1, rs2 => rd = (rs2 == 0) ? 0 : rs1
> > - czero.nez rd, rs1, rs2 => rd = (rs2 != 0) ? 0 : rs1
> >
> > See
> >
> > https://github.com/riscv/riscv-zicond/releases/download/v1.0-draft-20230120/riscv-zicond_1.0-draft-20230120.pdf
> > for the (current version of the) Zicond specification and usage details.
> >
> > Signed-off-by: Philipp Tomsich
> > ---
> >
> > Changes in v2:
> > - gates availability of the instructions through a REQUIRE_ZICOND
> > macro (these were previously always enabled)
> >
> > MAINTAINERS | 7 +++
> > target/riscv/cpu.c | 4 ++
> > target/riscv/cpu.h | 1 +
> > target/riscv/insn32.decode | 4 ++
> > target/riscv/insn_trans/trans_rvzicond.c.inc | 54
> > target/riscv/translate.c | 1 +
> > 6 files changed, 71 insertions(+)
> > create mode 100644 target/riscv/insn_trans/trans_rvzicond.c.inc
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 08ad1e5341..ca914c42fa 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -295,6 +295,13 @@ F: include/hw/riscv/
> > F: linux-user/host/riscv32/
> > F: linux-user/host/riscv64/
> >
> > +RISC-V Zicond extension
> > +M: Philipp Tomsich
> > +M: Christoph Muellner
> > +L: qemu-ri...@nongnu.org
> > +S: Supported
> > +F: target/riscv/insn_trans/trans_zicond.c.inc
>
> I don't think we need a maintainers entry for official extensions,
> that seems like it's going to get out of hand very quickly.
I'll drop on v3 — or feel free to edit it out when merging.
I put this in, as Zicond is unratified and I put it in as a commitment
to handle this until ratified.
Cheers,
Philipp.
>
> > +
> > RISC-V XVentanaCondOps extension
> > M: Philipp Tomsich
> > L: qemu-ri...@nongnu.org
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index cc75ca7667..f214b03e95 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -73,6 +73,7 @@ struct isa_ext_data {
> > static const struct isa_ext_data isa_edata_arr[] = {
> > ISA_EXT_DATA_ENTRY(h, false, PRIV_VERSION_1_12_0, ext_h),
> > ISA_EXT_DATA_ENTRY(v, false, PRIV_VERSION_1_12_0, ext_v),
> > +ISA_EXT_DATA_ENTRY(zicond, true, PRIV_VERSION_1_12_0, ext_zicond),
> > ISA_EXT_DATA_ENTRY(zicsr, true, PRIV_VERSION_1_10_0, ext_icsr),
> > ISA_EXT_DATA_ENTRY(zifencei, true, PRIV_VERSION_1_10_0, ext_ifencei),
> > ISA_EXT_DATA_ENTRY(zihintpause, true, PRIV_VERSION_1_10_0,
> > ext_zihintpause),
> > @@ -1080,6 +1081,9 @@ static Property riscv_cpu_extensions[] = {
> > DEFINE_PROP_BOOL("x-smaia", RISCVCPU, cfg.ext_smaia, false),
> > DEFINE_PROP_BOOL("x-ssaia", RISCVCPU, cfg.ext_ssaia, false),
> >
> > +/* Zicond 1.0-draft-20230120 */
> > +DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
> > +
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index f5609b62a2..4b6ff800d3 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -446,6 +446,7 @@ struct RISCVCPUConfig {
> > bool ext_zkt;
> > bool ext_ifencei;
> > bool ext_icsr;
> > +bool ext_zicond;
> > bool ext_zihintpause;
> > bool ext_smstateen;
> > bool ext_sstc;
> > diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> > index b7e7613ea2..ca812c2f7a 100644
> > --- a/target/riscv/insn32.decode
> > +++ b/target/riscv/insn32.decode
> > @@ -890,3 +890,7 @@ sm3p1 00 01000 01001 . 001 . 0010011 @r2
> > # *** RV32 Zksed Standard Extension ***
> > sm4ed .. 11000 . . 000 . 0110011 @k_aes
> > sm4ks .. 11010 . . 000 . 0110011 @k_aes
> > +
> > +# *** Zicond Standard Extension ***
> > +czero_eqz 111 . . 101 . 0110011 @r
> > +czero_nez 111 . . 111 . 0110011 @r
> > diff --git a/target/riscv/insn_trans/trans_rvzicond.c.inc
> > b/target/riscv/insn_trans/trans_rvzicond.c.inc
> > new file mode 100644
> > index 00..20e9694a2c
> > --- /dev/null
> > +++ b/target/riscv/insn_trans/trans_rvzicond.c.inc
> > @@ -0,0 +1,54 @@
> > +/*
> > + * RISC-V translation routines for the XVentanaCondOps extension.
> > + *
> > + * Copyright (c) 2022 VRULL GmbH.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and condit
Re: [PATCH v2 2/2] target/riscv: redirect XVentanaCondOps to use the Zicond functions
On Sat, Jan 21, 2023 at 12:36 PM Philipp Tomsich
wrote:
>
> The Zicond standard extension implements the same instruction
> semantics as XVentanaCondOps, although using different mnemonics and
> opcodes.
>
> Point XVentanaCondOps to the (newly implemented) Zicond implementation
> to reduce the future maintenance burden.
>
> Also updating MAINTAINERS as trans_xventanacondops.c.inc will not see
> active maintenance from here forward.
>
> Signed-off-by: Philipp Tomsich
> ---
>
> Changes in v2:
> - Calls into the gen_czero_{eqz,nez} helpers instead of calling
> trans_czero_{eqz,nez} to bypass the require-check and ensure that
> XVentanaCondOps can be enabled/disabled independently of Zicond.
>
> MAINTAINERS| 2 +-
> .../insn_trans/trans_xventanacondops.c.inc | 18 +++---
> 2 files changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ca914c42fa..293a9d1c8c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -305,7 +305,7 @@ F: target/riscv/insn_trans/trans_zicond.c.inc
> RISC-V XVentanaCondOps extension
> M: Philipp Tomsich
> L: qemu-ri...@nongnu.org
> -S: Supported
> +S: Odd Fixes
Should this extension be deprecated then?
Alistair
Re: [PATCH v2 1/2] target/riscv: add Zicond as an experimental extension
On Sat, Jan 21, 2023 at 12:36 PM Philipp Tomsich
wrote:
>
> This implements the Zicond (conditional integer operations) extension,
> as of version 1.0-draft-20230120 as an experimental extension in QEMU
> ("x-zicond").
>
> The Zicond extension acts as a building block for branchless sequences
> including conditional-{arithmetic,logic,select,move}. Refer to the
> specification for usage scenarios and application guidance.
>
> The following instructions constitute Zicond:
> - czero.eqz rd, rs1, rs2 => rd = (rs2 == 0) ? 0 : rs1
> - czero.nez rd, rs1, rs2 => rd = (rs2 != 0) ? 0 : rs1
>
> See
>
> https://github.com/riscv/riscv-zicond/releases/download/v1.0-draft-20230120/riscv-zicond_1.0-draft-20230120.pdf
> for the (current version of the) Zicond specification and usage details.
>
> Signed-off-by: Philipp Tomsich
> ---
>
> Changes in v2:
> - gates availability of the instructions through a REQUIRE_ZICOND
> macro (these were previously always enabled)
>
> MAINTAINERS | 7 +++
> target/riscv/cpu.c | 4 ++
> target/riscv/cpu.h | 1 +
> target/riscv/insn32.decode | 4 ++
> target/riscv/insn_trans/trans_rvzicond.c.inc | 54
> target/riscv/translate.c | 1 +
> 6 files changed, 71 insertions(+)
> create mode 100644 target/riscv/insn_trans/trans_rvzicond.c.inc
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 08ad1e5341..ca914c42fa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -295,6 +295,13 @@ F: include/hw/riscv/
> F: linux-user/host/riscv32/
> F: linux-user/host/riscv64/
>
> +RISC-V Zicond extension
> +M: Philipp Tomsich
> +M: Christoph Muellner
> +L: qemu-ri...@nongnu.org
> +S: Supported
> +F: target/riscv/insn_trans/trans_zicond.c.inc
I don't think we need a maintainers entry for official extensions,
that seems like it's going to get out of hand very quickly.
Alistair
> +
> RISC-V XVentanaCondOps extension
> M: Philipp Tomsich
> L: qemu-ri...@nongnu.org
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index cc75ca7667..f214b03e95 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -73,6 +73,7 @@ struct isa_ext_data {
> static const struct isa_ext_data isa_edata_arr[] = {
> ISA_EXT_DATA_ENTRY(h, false, PRIV_VERSION_1_12_0, ext_h),
> ISA_EXT_DATA_ENTRY(v, false, PRIV_VERSION_1_12_0, ext_v),
> +ISA_EXT_DATA_ENTRY(zicond, true, PRIV_VERSION_1_12_0, ext_zicond),
> ISA_EXT_DATA_ENTRY(zicsr, true, PRIV_VERSION_1_10_0, ext_icsr),
> ISA_EXT_DATA_ENTRY(zifencei, true, PRIV_VERSION_1_10_0, ext_ifencei),
> ISA_EXT_DATA_ENTRY(zihintpause, true, PRIV_VERSION_1_10_0,
> ext_zihintpause),
> @@ -1080,6 +1081,9 @@ static Property riscv_cpu_extensions[] = {
> DEFINE_PROP_BOOL("x-smaia", RISCVCPU, cfg.ext_smaia, false),
> DEFINE_PROP_BOOL("x-ssaia", RISCVCPU, cfg.ext_ssaia, false),
>
> +/* Zicond 1.0-draft-20230120 */
> +DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
> +
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index f5609b62a2..4b6ff800d3 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -446,6 +446,7 @@ struct RISCVCPUConfig {
> bool ext_zkt;
> bool ext_ifencei;
> bool ext_icsr;
> +bool ext_zicond;
> bool ext_zihintpause;
> bool ext_smstateen;
> bool ext_sstc;
> diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> index b7e7613ea2..ca812c2f7a 100644
> --- a/target/riscv/insn32.decode
> +++ b/target/riscv/insn32.decode
> @@ -890,3 +890,7 @@ sm3p1 00 01000 01001 . 001 . 0010011 @r2
> # *** RV32 Zksed Standard Extension ***
> sm4ed .. 11000 . . 000 . 0110011 @k_aes
> sm4ks .. 11010 . . 000 . 0110011 @k_aes
> +
> +# *** Zicond Standard Extension ***
> +czero_eqz 111 . . 101 . 0110011 @r
> +czero_nez 111 . . 111 . 0110011 @r
> diff --git a/target/riscv/insn_trans/trans_rvzicond.c.inc
> b/target/riscv/insn_trans/trans_rvzicond.c.inc
> new file mode 100644
> index 00..20e9694a2c
> --- /dev/null
> +++ b/target/riscv/insn_trans/trans_rvzicond.c.inc
> @@ -0,0 +1,54 @@
> +/*
> + * RISC-V translation routines for the XVentanaCondOps extension.
> + *
> + * Copyright (c) 2022 VRULL GmbH.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> with
> +
Re: [PATCH v7 4/7] mac_newworld: Add machine types for different mac99 configs
On Jan 22, 2023, at 4:48 PM, BALATON Zoltan wrote:
> It would be tough to come up with a name for the powerbook3_2 though as these
> were called Early 2001 Titanium PowerBook G4 or code name Mercury but even
> Mac fanatics probably couldn't tell that was a powerbook if you call it
> g4mercury so I'm open to votes on naming but hard to be convinced there's
> anything simpler and more straightforward than using machine id which is
> usually also listed everywhere for these.
g4tibook1stgen?
("TiBook" being an affectionate abbreviation for "Titanium PowerBook")
Cheers,
Josh
Re: [PATCH v9 0/3] hw/riscv: clear kernel_entry high bits with 32bit CPUs
On Fri, Jan 20, 2023 at 7:38 AM Daniel Henrique Barboza wrote: > > Hi, > > In this version I changed the patch order to avoid having a patch that > would trigger the 32 bit regression Alistair observed. Patch 3 is now > the first patch. > > I've also addressed the comments from Bin and Phil. > > Patches based on riscv-to-apply.next. > > Changes from v8: > - patch 1 (former 3): > - comment changes > - now open code '32' instead of using a macro > - v8 link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg03254.html > > Daniel Henrique Barboza (3): > hw/riscv: clear kernel_entry higher bits from load_elf_ram_sym() > hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel() > hw/riscv/boot.c: make riscv_load_initrd() static Thanks! Applied to riscv-to-apply.next Alistair > > hw/riscv/boot.c| 96 ++ > hw/riscv/microchip_pfsoc.c | 12 + > hw/riscv/opentitan.c | 4 +- > hw/riscv/sifive_e.c| 4 +- > hw/riscv/sifive_u.c| 12 + > hw/riscv/spike.c | 14 ++ > hw/riscv/virt.c| 12 + > include/hw/riscv/boot.h| 3 +- > 8 files changed, 82 insertions(+), 75 deletions(-) > > -- > 2.39.0 > >
Re: [PATCH v1] target/riscv: update disas.c for xnor/orn/andn and slli.uw
On Sat, Jan 21, 2023 at 1:16 AM Philipp Tomsich
wrote:
>
> The decoding of the following instructions from Zb[abcs] currently
> contains decoding/printing errors:
> * xnor,orn,andn: the rs2 operand is not being printed
> * slli.uw: decodes and prints the immediate shift-amount as a
> register (e.g. 'shift-by-2' becomes 'sp') instead of
> interpreting this as an immediate
>
> This commit updates the instruction descriptions to use the
> appropriate decoding/printing formats.
>
> Signed-off-by: Philipp Tomsich
Thanks!
Applied to riscv-to-apply.next
Alistair
>
> ---
>
> disas/riscv.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/disas/riscv.c b/disas/riscv.c
> index d216b9c39b..ddda687c13 100644
> --- a/disas/riscv.c
> +++ b/disas/riscv.c
> @@ -1626,9 +1626,9 @@ const rv_opcode_data opcode_data[] = {
> { "cpop", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> { "sext.h", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> { "sext.b", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> -{ "xnor", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> -{ "orn", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> -{ "andn", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "xnor", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "orn", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "andn", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> { "rol", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> { "ror", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> { "sh1add", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> @@ -1647,7 +1647,7 @@ const rv_opcode_data opcode_data[] = {
> { "clzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> { "clzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> { "cpopw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> -{ "slli.uw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "slli.uw", rv_codec_i_sh5, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },
> { "add.uw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> { "rolw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> { "rorw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> --
> 2.34.1
>
>
[PATCH] include/hw/riscv/opentitan: update opentitan IRQs
From: Wilfred Mallawa
Updates the opentitan IRQs to match the latest supported commit of
Opentitan from TockOS.
OPENTITAN_SUPPORTED_SHA := 565e4af39760a123c59a184aa2f5812a961fde47
Signed-off-by: Wilfred Mallawa
---
include/hw/riscv/opentitan.h | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h
index 7659d1bc5b..235728b9cc 100644
--- a/include/hw/riscv/opentitan.h
+++ b/include/hw/riscv/opentitan.h
@@ -108,11 +108,11 @@ enum {
IBEX_UART0_RX_BREAK_ERR_IRQ = 6,
IBEX_UART0_RX_TIMEOUT_IRQ = 7,
IBEX_UART0_RX_PARITY_ERR_IRQ = 8,
-IBEX_TIMER_TIMEREXPIRED0_0= 127,
-IBEX_SPI_HOST0_ERR_IRQ= 134,
-IBEX_SPI_HOST0_SPI_EVENT_IRQ = 135,
-IBEX_SPI_HOST1_ERR_IRQ= 136,
-IBEX_SPI_HOST1_SPI_EVENT_IRQ = 137,
+IBEX_TIMER_TIMEREXPIRED0_0= 124,
+IBEX_SPI_HOST0_ERR_IRQ= 131,
+IBEX_SPI_HOST0_SPI_EVENT_IRQ = 132,
+IBEX_SPI_HOST1_ERR_IRQ= 133,
+IBEX_SPI_HOST1_SPI_EVENT_IRQ = 134,
};
#endif
--
2.39.0
Re: [PATCH v9 1/3] hw/riscv: clear kernel_entry higher bits from load_elf_ram_sym()
On Fri, Jan 20, 2023 at 7:38 AM Daniel Henrique Barboza
wrote:
>
> load_elf_ram_sym() will sign-extend 32 bit addresses. If a 32 bit
> QEMU guest happens to be running in a hypervisor that are using 64
> bits to encode its address, kernel_entry can be padded with '1's
> and create problems [1].
>
> Use a translate_fn() callback to be called by load_elf_ram_sym() and
> return only the 32 bits address if we're running a 32 bit CPU.
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg02281.html
>
> Suggested-by: Philippe Mathieu-Daudé
> Suggested-by: Bin Meng
> Reviewed-by: Philippe Mathieu-Daudé
> Signed-off-by: Daniel Henrique Barboza
Reviewed-by: Alistair Francis
Alistair
> ---
> hw/riscv/boot.c| 20 +++-
> hw/riscv/microchip_pfsoc.c | 3 ++-
> hw/riscv/opentitan.c | 3 ++-
> hw/riscv/sifive_e.c| 3 ++-
> hw/riscv/sifive_u.c| 3 ++-
> hw/riscv/spike.c | 3 ++-
> hw/riscv/virt.c| 3 ++-
> include/hw/riscv/boot.h| 1 +
> 8 files changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 2594276223..46fc7adccf 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -173,7 +173,24 @@ target_ulong riscv_load_firmware(const char
> *firmware_filename,
> exit(1);
> }
>
> +static uint64_t translate_kernel_address(void *opaque, uint64_t addr)
> +{
> +RISCVHartArrayState *harts = opaque;
> +
> +if (riscv_is_32bit(harts)) {
> +/*
> + * For 32 bit CPUs, kernel_load_base is sign-extended
> + * (i.e. it can be padded with '1's) by load_elf().
> + * Remove the sign extension by truncating to 32-bit.
> + */
> +return extract64(addr, 0, 32);
> +}
> +
> +return addr;
> +}
> +
> target_ulong riscv_load_kernel(MachineState *machine,
> + RISCVHartArrayState *harts,
> target_ulong kernel_start_addr,
> symbol_fn_t sym_cb)
> {
> @@ -189,7 +206,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
> * the (expected) load address load address. This allows kernels to have
> * separate SBI and ELF entry points (used by FreeBSD, for example).
> */
> -if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
> +if (load_elf_ram_sym(kernel_filename, NULL,
> + translate_kernel_address, harts,
> NULL, &kernel_load_base, NULL, NULL, 0,
> EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> return kernel_load_base;
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index 82ae5e7023..bdefeb3cbb 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -629,7 +629,8 @@ static void
> microchip_icicle_kit_machine_init(MachineState *machine)
> kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
> firmware_end_addr);
>
> -kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
> +kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
> + kernel_start_addr, NULL);
>
> if (machine->initrd_filename) {
> riscv_load_initrd(machine, kernel_entry);
> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> index 64d5d435b9..2731138c41 100644
> --- a/hw/riscv/opentitan.c
> +++ b/hw/riscv/opentitan.c
> @@ -101,7 +101,8 @@ static void opentitan_board_init(MachineState *machine)
> }
>
> if (machine->kernel_filename) {
> -riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, NULL);
> +riscv_load_kernel(machine, &s->soc.cpus,
> + memmap[IBEX_DEV_RAM].base, NULL);
> }
> }
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 3e3f4b0088..1a7d381514 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -114,7 +114,8 @@ static void sifive_e_machine_init(MachineState *machine)
>memmap[SIFIVE_E_DEV_MROM].base,
> &address_space_memory);
>
> if (machine->kernel_filename) {
> -riscv_load_kernel(machine, memmap[SIFIVE_E_DEV_DTIM].base, NULL);
> +riscv_load_kernel(machine, &s->soc.cpus,
> + memmap[SIFIVE_E_DEV_DTIM].base, NULL);
> }
> }
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 2fb6ee231f..83dfe09877 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -598,7 +598,8 @@ static void sifive_u_machine_init(MachineState *machine)
> kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
> firmware_end_addr);
>
> -kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
> +kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
> +
Re: [PATCH v1] target/riscv: update disas.c for xnor/orn/andn and slli.uw
On Sat, Jan 21, 2023 at 1:16 AM Philipp Tomsich
wrote:
>
> The decoding of the following instructions from Zb[abcs] currently
> contains decoding/printing errors:
> * xnor,orn,andn: the rs2 operand is not being printed
> * slli.uw: decodes and prints the immediate shift-amount as a
> register (e.g. 'shift-by-2' becomes 'sp') instead of
> interpreting this as an immediate
>
> This commit updates the instruction descriptions to use the
> appropriate decoding/printing formats.
>
> Signed-off-by: Philipp Tomsich
Reviewed-by: Alistair Francis
Alistair
>
> ---
>
> disas/riscv.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/disas/riscv.c b/disas/riscv.c
> index d216b9c39b..ddda687c13 100644
> --- a/disas/riscv.c
> +++ b/disas/riscv.c
> @@ -1626,9 +1626,9 @@ const rv_opcode_data opcode_data[] = {
> { "cpop", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> { "sext.h", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> { "sext.b", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> -{ "xnor", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> -{ "orn", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> -{ "andn", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> +{ "xnor", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "orn", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "andn", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> { "rol", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> { "ror", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> { "sh1add", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> @@ -1647,7 +1647,7 @@ const rv_opcode_data opcode_data[] = {
> { "clzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> { "clzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> { "cpopw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> -{ "slli.uw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +{ "slli.uw", rv_codec_i_sh5, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },
> { "add.uw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> { "rolw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> { "rorw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> --
> 2.34.1
>
>
Re: Display update issue on M1 Macs
On Thu, 19 Jan 2023, Akihiko Odaki wrote: On 2023/01/15 3:11, BALATON Zoltan wrote: On Sat, 14 Jan 2023, Akihiko Odaki wrote: On 2023/01/13 22:43, BALATON Zoltan wrote: On Thu, 5 Jan 2023, BALATON Zoltan wrote: Hello, I got reports from several users trying to run AmigaOS4 on sam460ex on Apple silicon Macs that they get missing graphics that I can't reproduce on x86_64. With help from the users who get the problem we've narrowed it down to the following: It looks like that data written to the sm501's ram in qemu/hw/display/sm501.c::sm501_2d_operation() is then not seen from sm501_update_display() in the same file. The sm501_2d_operation() function is called when the guest accesses the emulated card so it may run in a different thread than sm501_update_display() which is called by the ui backend but I'm not sure how QEMU calls these. Is device code running in iothread and display update in main thread? The problem is also independent of the display backend and was reproduced with both -display cocoa and -display sdl. We have confirmed it's not the pixman routines that sm501_2d_operation() uses as the same issue is seen also with QEMU 4.x where pixman wasn't used and with all versions up to 7.2 so it's also not some bisectable change in QEMU. It also happens with --enable-debug so it doesn't seem to be related to optimisation either and I don't get it on x86_64 but even x86_64 QEMU builds run on Apple M1 with Rosetta 2 show the problem. It also only seems to affect graphics written from sm501_2d_operation() which AmigaOS4 uses extensively but other OSes don't and just render graphics with the vcpu which work without problem also on the M1 Macs that show this problem with AmigaOS4. Theoretically this could be some missing syncronisation which is something ARM and PPC may need while x86 doesn't but I don't know if this is really the reason and if so where and how to fix it). Any idea what may cause this and what could be a fix to try? Any idea anyone? At least some explanation if the above is plausible or if there's an option to disable the iothread and run everyting in a single thread to verify the theory could help. I've got reports from at least 3 people getting this problem but I can't do much to fix it without some help. (Info on how to run it is here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#amigaos but AmigaOS4 is not freely distributable so it's a bit hard to reproduce. Some Linux X servers that support sm501/sm502 may also use the card's 2d engine but I don't know about any live CDs that readily run on sam460ex.) Thank you, BALATON Zoltan Sorry, I missed the email. Indeed the ui backend should call sm501_update_display() in the main thread, which should be different from the thread calling sm501_2d_operation(). However, if I understand it correctly, both of the functions should be called with iothread lock held so there should be no race condition in theory. But there is an exception: memory_region_snapshot_and_clear_dirty() releases iothread lock, and that broke raspi3b display device: https://lore.kernel.org/qemu-devel/CAFEAcA9odnPo2LPip295Uztri7JfoVnQbkJ=wn+k8dqneb_...@mail.gmail.com/T/ It is unexpected that gfx_update() callback releases iothread lock so it may break things in peculiar ways. Peter, is there any change in the situation regarding the race introduced by memory_region_snapshot_and_clear_dirty()? For now, to workaround the issue, I think you can create another mutex and make the entire sm501_2d_engine_write() and sm501_update_display() critical sections. Interesting thread but not sure it's the same problem so this workaround may not be enough to fix my issue. Here's a video posted by one of the people who reported it showing the problem on M1 Mac: https://www.youtube.com/watch?v=FDqoNbp6PQs and here's how it looks like on other machines: https://www.youtube.com/watch?v=ML7-F4HNFKQ There are also videos showing it running on RPi 4 and G5 Mac without this issue so it seems to only happen on Apple Silicon M1 Macs. What's strange is that graphics elements are not just delayed which I think should happen with missing thread synchronisation where the update callback would miss some pixels rendered during it's running but subsequent update callbacks would eventually draw those, woudn't they? Also setting full_update to 1 in sm501_update_display() callback to disable dirty tracking does not fix the problem. So it looks like as if sm501_2d_operation() running on one CPU core only writes data to the local cache of that core which sm501_update_display() running on other core can't see, so maybe some cache synchronisation is needed in memory_region_set_dirty() or if that's already there maybe I should call it for all changes not only those in the visible display area? I'm still not sure I understand the problem and don't know what could be a fix for it so anything to test to identify the issue better mi
Re: [PATCH v3 3/7] hw/riscv/microchip_pfsoc.c: add an Icicle Kit fdt address function
On Sun, Jan 22, 2023 at 5:16 AM Daniel Henrique Barboza wrote: > > Conor, > > Thanks for the Icicle-kit walk-through! I'll not claim that I fully > understood it, > but I understood enough to handle the situation ATM. > > Without this change, this is where the FDT is being installed in the board > when > I start it with 8Gb of RAM (retrieved via 'info roms'): > > addr=bfe0 size=0x00a720 mem=ram name="fdt" > > Which surprised me at first because this is almost at the end of the LO area > which has > 1Gb and I figured it would be in the middle of another RAM area. I took > another read > at what we're doing in riscv_load_fdt(): > > --- > temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end; > fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB); > --- > > This code can be read as "if the starting address of the RAM is lower than > 3Gb, put > the FDT no further than 3Gb (0xc000). Otherwise, put it at the end of > dram", > where "dram_base" is the starting address of the RAM block that the function > receives. > > For icicle-kit, this is being passed as memmap[MICROCHIP_PFSOC_DRAM_LO].base, > 0x8000, which is 2Gb. > > So, regardless of how much RAM we have (dram_end), the FDT will always be > capped at > 3Gb. At this moment, this fits exactly at the end of the LO area for the > Icicle Kit. > Which is funny because this 3Gb restriction was added by commit 1a475d39ef54 > to fix > 32 bit guest boot and it happened to also work for the Microchip SoC. > > So yeah, I thought that I was fixing a bug and in the end I caused one. This > patch > needs to go. > > > Alistair, I believe I should re-send v2, this time explaining why the > existing function > will not break the Microchip board because we'll never put the FDT out of the > LO area > of the board. Does this work for you? I think that's fine. My only worry is that we are losing some flexibility that some future board might want. Alistair
Re: [PATCH v7 3/7] mac_{old,new}world: Pass MacOS VGA NDRV in card ROM instead of fw_cfg
On Sun, 22 Jan 2023, Mark Cave-Ayland wrote:
On 11/01/2023 00:54, BALATON Zoltan wrote:
On Tue, 10 Jan 2023, Mark Cave-Ayland wrote:
On 04/01/2023 21:59, BALATON Zoltan wrote:
OpenBIOS cannot run FCode ROMs yet but it can detect NDRV in VGA card
ROM and add it to the device tree for MacOS. Pass the NDRV this way
instead of via fw_cfg. This solves the problem with OpenBIOS also
adding the NDRV to ati-vga which it does not work with. This does not
need any changes to OpenBIOS as this NDRV ROM handling is already
there but this patch also allows simplifying OpenBIOS later to remove
the fw_cfg ndrv handling from the vga FCode and also drop the
vga-ndrv? option which is not needed any more as users can disable the
ndrv with -device VGA,romfile="" (or override it with their own NDRV
or ROM). Once FCode support is implemented in OpenBIOS, the proper
FCode ROM can be set the same way so this paves the way to remove some
hacks.
Signed-off-by: BALATON Zoltan
---
hw/ppc/mac_newworld.c | 18 ++
hw/ppc/mac_oldworld.c | 18 ++
2 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 460c14b5e3..60c9c27986 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -510,18 +510,6 @@ static void ppc_core99_init(MachineState *machine)
fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_BUSFREQ, BUSFREQ);
fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_NVRAM_ADDR, nvram_addr);
- /* MacOS NDRV VGA driver */
- filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, NDRV_VGA_FILENAME);
- if (filename) {
- gchar *ndrv_file;
- gsize ndrv_size;
-
- if (g_file_get_contents(filename, &ndrv_file, &ndrv_size, NULL))
{
- fw_cfg_add_file(fw_cfg, "ndrv/qemu_vga.ndrv", ndrv_file,
ndrv_size);
- }
- g_free(filename);
- }
-
qemu_register_boot_set(fw_cfg_boot_set, fw_cfg);
}
@@ -565,6 +553,11 @@ static int core99_kvm_type(MachineState *machine,
const char *arg)
return 2;
}
+static GlobalProperty props[] = {
+ /* MacOS NDRV VGA driver */
+ { "VGA", "romfile", NDRV_VGA_FILENAME },
+};
+
static void core99_machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -585,6 +578,7 @@ static void core99_machine_class_init(ObjectClass
*oc, void *data)
#endif
mc->default_ram_id = "ppc_core99.ram";
mc->ignore_boot_device_suffixes = true;
+ compat_props_add(mc->compat_props, props, G_N_ELEMENTS(props));
fwc->get_dev_path = core99_fw_dev_path;
}
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 5a7b25a4a8..6a1b1ad47a 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -344,18 +344,6 @@ static void ppc_heathrow_init(MachineState *machine)
fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_CLOCKFREQ, CLOCKFREQ);
fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_BUSFREQ, BUSFREQ);
- /* MacOS NDRV VGA driver */
- filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, NDRV_VGA_FILENAME);
- if (filename) {
- gchar *ndrv_file;
- gsize ndrv_size;
-
- if (g_file_get_contents(filename, &ndrv_file, &ndrv_size, NULL))
{
- fw_cfg_add_file(fw_cfg, "ndrv/qemu_vga.ndrv", ndrv_file,
ndrv_size);
- }
- g_free(filename);
- }
-
qemu_register_boot_set(fw_cfg_boot_set, fw_cfg);
}
@@ -400,6 +388,11 @@ static int heathrow_kvm_type(MachineState
*machine, const char *arg)
return 2;
}
+static GlobalProperty props[] = {
+ /* MacOS NDRV VGA driver */
+ { "VGA", "romfile", NDRV_VGA_FILENAME },
+};
+
static void heathrow_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -420,6 +413,7 @@ static void heathrow_class_init(ObjectClass *oc, void
*data)
mc->default_display = "std";
mc->ignore_boot_device_suffixes = true;
mc->default_ram_id = "ppc_heathrow.ram";
+ compat_props_add(mc->compat_props, props, G_N_ELEMENTS(props));
fwc->get_dev_path = heathrow_fw_dev_path;
}
The qemu_vga.ndrv is deliberately kept separate from the PCI option ROM
because it is a binary generated by a separate project: otherwise you'd
end up creating a dependency between OpenBIOS and QemuMacDrivers, which is
almost impossible to achieve since qemu_vga.ndrv can only (currently) be
built in an emulated MacOS 9 guest.
I don't get this. The dependency is already there as qemu_vga.ndrv ships
with QEMU such as all the vgabios-*.bin and SeaBIOS binaries which are also
built from different projects. The qemu_vga.ndrv would also still be part
of an FCode ROM together with vga.fs if OpenBIOS could run that so this
patch solely changes the way of passing the ROM binary to OpenBIOS from
fw_cfg to the card ROM which is closer to how it should be and can direcly
be replaced with the FCode ROM later after OpenBIOS will be advanced to
that point.
Even if OpenBIOS were able to execute PCI option ROMs, the problem is
Re: [PATCH v7 6/7] mac_newworld: Deprecate mac99 "via" option
On Sun, 22 Jan 2023, Mark Cave-Ayland wrote: On 12/01/2023 23:51, BALATON Zoltan wrote: On Thu, 12 Jan 2023, Howard Spoelstra wrote: On Wed, Jan 11, 2023 at 1:15 AM BALATON Zoltan wrote: On Tue, 10 Jan 2023, Mark Cave-Ayland wrote: On 04/01/2023 21:59, BALATON Zoltan wrote: Setting emulated machine type with a property called "via" is confusing users so deprecate the "via" option in favour of newly added explicit machine types. The default via=cuda option is not a valid config (no real Mac has this combination of hardware) so no machine type could be defined for that therefore it is kept for backwards compatibility with older QEMU versions for now but other options resembling real machines are deprecated. Signed-off-by: BALATON Zoltan I believe that people do use -M mac99,via=cuda to run some rare versions of MacOS in QEMU (I think possibly OS X DP and Workgroup Server?), so we would want to keep this option somewhere. The idea is that after previous patches we now have machine types for all other via option values (that also match real Mac machines) other than via=cude but that is the default for mac99 so after the reprecation period when the via option is removed mac99 (which is the same as mac99,via=cuda) can remain for this use case (and for backward compatibility) until the other machines are fixed to not need this any more. So all via options are still available but as different machine types. My 2 cents about naming: It seems less important how the machines are named when their name is not covering their definition. F.i. the powermac3,1 never had adb, could not be equipped with a G3 cpu, did not run at 900Mhz. The closest possible qemu-options based definition of a powermac3,1 (via=pmu) will not run Mac OS 9.0.4 ;-) due to the 2 USB devices problem. To run that via=cuda is already needed. What does that mean? Should we aim to emulate real Macs or are we happy with the Franken-Mac we have now? The names also show what we intend to emulate even though the emulation may not be complete or have bugs (this is also true for other machines in QEMU where a lot of them are not fully emulated, only well enough to boot guest OSes). Looks like everybody has forgotten the previous discussion and not read the docs and deprecation patches where this is explained so I summarise the proposed change here again: - qemu-system-ppc -M mac99 is unchanged and works like before it just warns for the via option and when using it in qemu-system-ppc64 suggesting using new machines instead so these could evetually be removed next year. mac99,via=cuda is just mac99 so you can continue to use that, mac99 is not deprecated and don't want to remove it. - qemu-system-ppc64 -M mac99 -> powermac7_3 - qemu-system-ppc -M mac99,via=pmu -> powermac3,1 - qemu-system-ppc64 -M mac99,via=pmu-adb -> powerbook3_2 The last one is one of the rare Macs that had adb and pmu, all others with pmu usually have USB. The PowerMac1,2 (G4 PCI) had CUDA but not with mac99 hardware but more similar to g3beige and no ADB ports according to https://en.wikipedia.org/wiki/Power_Mac_G4#1st_generation:_Graphite https://en.wikipedia.org/wiki/Power_Macintosh_G3_(Blue_and_White)#Hardware The PowerMac7,3 seems to be matching the PCI device listing in the comment at the beginning of mac_newworld.c and also this article: https://www.informit.com/articles/article.aspx?p=606582 What is the 2 USB devices problem? Is it the one we've debugged before and found that it's noted in a comment marked with ??? in hw/usb/hcd-ohci.c? That could be fixed if there was somebody interested enough to provide a patch. But this series does not remove the mac99 and does not even deprecate it. What it deprecates are the via option to select different machine types and the automatic detection of ppc64 to emulate something different which are hard to understand for users and caused several misunderstandings. It's much more clear to have a separate machine type for each machine we emulate even when they aren't yet complete but at least we know which way to go and can compare to real hardware and fix the missing parts later. Also introducing powermac7_3 to split the ppc64 mac99 would allow to remove qemu-system-ppc if we wanted and only have one executable for all machines but even without this it's clearer to have separate machnies for G5 and G4 macs than mac99 silently behaving differently. Ultimately the issue you are trying to solve is this, which is that -M mac99 is different for qemu-system-ppc and qemu-system-ppc64. Perhaps the best way to start is to create a new "g5niagara" machine type (including OpenBIOS) and make it a clone of mac_newworld.c, and then issue a warning on qemu-system-ppc64 for -M mac99. I don't get what you mean. Patch 4 introduces a new machine type called powermac7_3 (or g5niagara if you want) which is a clone of mac99 and then issues the warning to deprecate qemu-system-ppc64 -M mac99
Re: [PATCH v7 4/7] mac_newworld: Add machine types for different mac99 configs
On Sun, 22 Jan 2023, Mark Cave-Ayland wrote: On 11/01/2023 00:36, BALATON Zoltan wrote: On Tue, 10 Jan 2023, Mark Cave-Ayland wrote: On 04/01/2023 21:59, BALATON Zoltan wrote: The mac99 machine emulates different machines depending on machine properties or even if it is run as qemu-system-ppc64 or qemu-system-ppc. This is very confusing for users and many hours were lost trying to explain it or finding out why commands users came up with are not working as expected. (E.g. Windows users might think qemu-system-ppc64 is just the 64 bit version of qemu-system-ppc and then fail to boot a 32 bit OS with -M mac99 trying to follow an example that had qemu-system-ppc.) To avoid such confusion, add explicit machine types for the different configs which will work the same with both qemu-system-ppc and qemu-system-ppc64 and also make the command line clearer for new users. Signed-off-by: BALATON Zoltan Some thoughts on this: the first is that not everyone agrees that for qemu-system-X that X represents the target. There were previous discussion where some KVM people assumed X represented the host, i.e. ppc64 was the binary that ran all PPC guests but with hardware acceleration for ppc64 guests on ppc64 hosts. This was a while ago, so it may be worth starting a thread on qemu-devel to see what the current consensus is. I don't see how this is relevant to this series, Also likely not the case any more as qemu-system-ppc and qemu-system-ppc64 share most of the code since a while with ppc64 including the config of ppc and adding more machines. Well the patch defines the powermac 7.3 machine just for TARGET_PPC64, no? So you're making the assumption qemu-system-ppc64 represents a 64-bit target rather than a 64-bit host. I'm not making that assumption, it's already there: $ qemu-system-ppc -machine help Supported machines are: 40p IBM RS/6000 7020 (40p) bamboo bamboo g3beige Heathrow based PowerMAC (default) mac99Mac99 based PowerMAC mpc8544dsmpc8544ds none empty machine pegasos2 Genesi/bPlan Pegasos II ppce500 generic paravirt e500 platform ref405ep ref405ep sam460ex aCube Sam460ex virtex-ml507 Xilinx Virtex ML507 reference design $ qemu-system-ppc64 -machine help Supported machines are: 40p IBM RS/6000 7020 (40p) bamboo bamboo g3beige Heathrow based PowerMAC mac99Mac99 based PowerMAC mpc8544dsmpc8544ds none empty machine pegasos2 Genesi/bPlan Pegasos II powernv10IBM PowerNV (Non-Virtualized) POWER10 powernv8 IBM PowerNV (Non-Virtualized) POWER8 powernv IBM PowerNV (Non-Virtualized) POWER9 (alias of powernv9) powernv9 IBM PowerNV (Non-Virtualized) POWER9 ppce500 generic paravirt e500 platform pseries-2.1 pSeries Logical Partition (PAPR compliant) [lots of different pseries versions omitted here] pseries pSeries Logical Partition (PAPR compliant) (alias of pseries-8.0) pseries-8.0 pSeries Logical Partition (PAPR compliant) (default) ref405ep ref405ep sam460ex aCube Sam460ex virtex-ml507 Xilinx Virtex ML507 reference design It makes no sense to define it for qemu-system-ppc as that version does not have G5 and 64 bit CPUs compiled in. Cf. qemu-system-ppc -cpu help and qemu-system-ppc64 -cpu help or target/ppc/cpu-models.c so I don't know what you're talking about. Secondly it's not clear to me why you've chosen names like "powermac_3_1" instead of "g4agp"? Does powermac_3_1 uniquely identify the G4 AGP Sawtooth model? For QEMU it is always best to emulate real machines, and whilst I understand you want to separate out the two versions of the mac99 machine, having "powermac_X_Y" seems less clear to me. These machine model identifiers are used by Apple to uniquely identify (all of) their machines since new-world Macs (even modern iPads and Macs have them) so for Mac people this should be clearer than the informal names that could get a bit long and confusing as there may be slight differences within a family. In any case, qemu-system-ppc -M mac99 is not corresponding to any real Mac so I'd like the options which do emulate real Macs to be called in a name that show which Mac is that. For the PPC Macs there's some info here for example: https://en.wikipedia.org/wiki/Power_Mac_G4 And everymac.com also has info on all Macs. There were actually more than one G4 PowerMac with AGP but the other one was informally called gigabit ethernet. So the model ID is a shorter and better way to clearly identify which hardware is it (and it's also referenced in the device-tree of these Macs). Are you planning to work on different types of G4 Mac where this could be confusing? Even to me "PowerMac 3.1" doesn't really tell me
Re: [PATCH 3/4] hw/misc/macio: Remove some single use local variables
On 18/01/2023 00:32, BALATON Zoltan wrote:
Drop some local variables that could just be substituted at the single
place they were used. This makes the code shorter and simpler.
Signed-off-by: BALATON Zoltan
---
hw/misc/macio/macio.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 4d7223cc85..ae2a9a960d 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -53,10 +53,8 @@
*/
static void macio_escc_legacy_setup(MacIOState *s)
{
-ESCCState *escc = ESCC(&s->escc);
-SysBusDevice *sbd = SYS_BUS_DEVICE(escc);
+SysBusDevice *sbd = SYS_BUS_DEVICE(&s->escc);
MemoryRegion *escc_legacy = g_new(MemoryRegion, 1);
-MemoryRegion *bar = &s->bar;
int i;
static const int maps[] = {
0x00, 0x00, /* Command B */
@@ -80,16 +78,15 @@ static void macio_escc_legacy_setup(MacIOState *s)
memory_region_add_subregion(escc_legacy, maps[i], port);
}
-memory_region_add_subregion(bar, 0x12000, escc_legacy);
+memory_region_add_subregion(&s->bar, 0x12000, escc_legacy);
}
static void macio_bar_setup(MacIOState *s)
{
-ESCCState *escc = ESCC(&s->escc);
-SysBusDevice *sbd = SYS_BUS_DEVICE(escc);
-MemoryRegion *bar = &s->bar;
+SysBusDevice *sbd = SYS_BUS_DEVICE(&s->escc);
+MemoryRegion *bar = sysbus_mmio_get_region(sbd, 0);
-memory_region_add_subregion(bar, 0x13000, sysbus_mmio_get_region(sbd, 0));
+memory_region_add_subregion(&s->bar, 0x13000, bar);
macio_escc_legacy_setup(s);
}
Reviewed-by: Mark Cave-Ayland
ATB,
Mark.
Re: [PATCH 2/4] hw/misc/macio: Rename sysbus_dev to sbd for consistency and brevity
On 18/01/2023 00:32, BALATON Zoltan wrote:
Some functions use sysbus_dev while others sbd name for local variable
storing a sysbus device pointer. Standardise on the shorter name to be
consistent and make the code easier to read as short name is less
distracting and needs less line breaks.
Signed-off-by: BALATON Zoltan
---
hw/misc/macio/macio.c | 78 +++
1 file changed, 35 insertions(+), 43 deletions(-)
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 0dfe372965..4d7223cc85 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -96,14 +96,14 @@ static void macio_bar_setup(MacIOState *s)
static void macio_common_realize(PCIDevice *d, Error **errp)
{
MacIOState *s = MACIO(d);
-SysBusDevice *sysbus_dev;
+SysBusDevice *sbd;
if (!qdev_realize(DEVICE(&s->dbdma), BUS(&s->macio_bus), errp)) {
return;
}
-sysbus_dev = SYS_BUS_DEVICE(&s->dbdma);
+sbd = SYS_BUS_DEVICE(&s->dbdma);
memory_region_add_subregion(&s->bar, 0x08000,
-sysbus_mmio_get_region(sysbus_dev, 0));
+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);
@@ -122,11 +122,10 @@ static void macio_realize_ide(MacIOState *s,
MACIOIDEState *ide,
qemu_irq irq0, qemu_irq irq1, int dmaid,
Error **errp)
{
-SysBusDevice *sysbus_dev;
+SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
-sysbus_dev = SYS_BUS_DEVICE(ide);
-sysbus_connect_irq(sysbus_dev, 0, irq0);
-sysbus_connect_irq(sysbus_dev, 1, irq1);
+sysbus_connect_irq(sbd, 0, irq0);
+sysbus_connect_irq(sbd, 1, irq1);
qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
object_property_set_link(OBJECT(ide), "dbdma", OBJECT(&s->dbdma),
&error_abort);
@@ -141,7 +140,7 @@ static void macio_oldworld_realize(PCIDevice *d, Error
**errp)
OldWorldMacIOState *os = OLDWORLD_MACIO(d);
DeviceState *pic_dev = DEVICE(&os->pic);
Error *err = NULL;
-SysBusDevice *sysbus_dev;
+SysBusDevice *sbd;
macio_common_realize(d, &err);
if (err) {
@@ -153,33 +152,30 @@ static void macio_oldworld_realize(PCIDevice *d, Error
**errp)
if (!qdev_realize(DEVICE(&os->pic), BUS(&s->macio_bus), errp)) {
return;
}
-sysbus_dev = SYS_BUS_DEVICE(&os->pic);
+sbd = SYS_BUS_DEVICE(&os->pic);
memory_region_add_subregion(&s->bar, 0x0,
-sysbus_mmio_get_region(sysbus_dev, 0));
+sysbus_mmio_get_region(sbd, 0));
qdev_prop_set_uint64(DEVICE(&s->cuda), "timebase-frequency",
s->frequency);
if (!qdev_realize(DEVICE(&s->cuda), BUS(&s->macio_bus), errp)) {
return;
}
-sysbus_dev = SYS_BUS_DEVICE(&s->cuda);
+sbd = SYS_BUS_DEVICE(&s->cuda);
memory_region_add_subregion(&s->bar, 0x16000,
-sysbus_mmio_get_region(sysbus_dev, 0));
-sysbus_connect_irq(sysbus_dev, 0, qdev_get_gpio_in(pic_dev,
- OLDWORLD_CUDA_IRQ));
+sysbus_mmio_get_region(sbd, 0));
+sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(pic_dev, OLDWORLD_CUDA_IRQ));
-sysbus_dev = SYS_BUS_DEVICE(&s->escc);
-sysbus_connect_irq(sysbus_dev, 0, qdev_get_gpio_in(pic_dev,
- OLDWORLD_ESCCB_IRQ));
-sysbus_connect_irq(sysbus_dev, 1, qdev_get_gpio_in(pic_dev,
- OLDWORLD_ESCCA_IRQ));
+sbd = SYS_BUS_DEVICE(&s->escc);
+sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(pic_dev, OLDWORLD_ESCCB_IRQ));
+sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(pic_dev, OLDWORLD_ESCCA_IRQ));
if (!qdev_realize(DEVICE(&os->nvram), BUS(&s->macio_bus), errp)) {
return;
}
-sysbus_dev = SYS_BUS_DEVICE(&os->nvram);
+sbd = SYS_BUS_DEVICE(&os->nvram);
memory_region_add_subregion(&s->bar, 0x6,
-sysbus_mmio_get_region(sysbus_dev, 0));
+sysbus_mmio_get_region(sbd, 0));
pmac_format_nvram_partition(&os->nvram, os->nvram.size);
/* IDE buses */
@@ -274,7 +270,7 @@ static void macio_newworld_realize(PCIDevice *d, Error
**errp)
NewWorldMacIOState *ns = NEWWORLD_MACIO(d);
DeviceState *pic_dev = DEVICE(&ns->pic);
Error *err = NULL;
-SysBusDevice *sysbus_dev;
+SysBusDevice *sbd;
MemoryRegion *timer_memory = NULL;
macio_common_realize(d, &err);
@@ -285,16 +281,14 @@ static void macio_newworld_realize(PCIDevice *d, Error
**errp)
/* OpenPIC */
qdev_prop_set_uint32(pic_dev, "model", OPENPIC_MODEL_K
Re: [PATCH 1/4] hw/misc/macio: Avoid some QOM casts
On 18/01/2023 00:32, BALATON Zoltan wrote:
At several places we already have the object pointer with the right
type so we don't need to cast it back and forth. Avoiding these casts
improves readability.
Signed-off-by: BALATON Zoltan
---
hw/misc/macio/macio.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 08dbdd7fc0..0dfe372965 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -220,11 +220,11 @@ static void macio_oldworld_init(Object *obj)
DeviceState *dev;
int i;
-object_initialize_child(OBJECT(s), "pic", &os->pic, TYPE_HEATHROW);
+object_initialize_child(obj, "pic", &os->pic, TYPE_HEATHROW);
-object_initialize_child(OBJECT(s), "cuda", &s->cuda, TYPE_CUDA);
+object_initialize_child(obj, "cuda", &s->cuda, TYPE_CUDA);
-object_initialize_child(OBJECT(s), "nvram", &os->nvram, TYPE_MACIO_NVRAM);
+object_initialize_child(obj, "nvram", &os->nvram, TYPE_MACIO_NVRAM);
dev = DEVICE(&os->nvram);
qdev_prop_set_uint32(dev, "size", MACIO_NVRAM_SIZE);
qdev_prop_set_uint32(dev, "it_shift", 4);
@@ -372,9 +372,9 @@ static void macio_newworld_init(Object *obj)
NewWorldMacIOState *ns = NEWWORLD_MACIO(obj);
int i;
-object_initialize_child(OBJECT(s), "pic", &ns->pic, TYPE_OPENPIC);
+object_initialize_child(obj, "pic", &ns->pic, TYPE_OPENPIC);
-object_initialize_child(OBJECT(s), "gpio", &ns->gpio, TYPE_MACIO_GPIO);
+object_initialize_child(obj, "gpio", &ns->gpio, TYPE_MACIO_GPIO);
for (i = 0; i < 2; i++) {
macio_init_ide(s, &ns->ide[i], i);
@@ -390,9 +390,9 @@ static void macio_instance_init(Object *obj)
qbus_init(&s->macio_bus, sizeof(s->macio_bus), TYPE_MACIO_BUS,
DEVICE(obj), "macio.0");
-object_initialize_child(OBJECT(s), "dbdma", &s->dbdma, TYPE_MAC_DBDMA);
+object_initialize_child(obj, "dbdma", &s->dbdma, TYPE_MAC_DBDMA);
-object_initialize_child(OBJECT(s), "escc", &s->escc, TYPE_ESCC);
+object_initialize_child(obj, "escc", &s->escc, TYPE_ESCC);
}
static const VMStateDescription vmstate_macio_oldworld = {
Reviewed-by: Mark Cave-Ayland
ATB,
Mark.
Re: [PATCH 00/17] audio: improve callback interface for audio frontends
On 15/01/2023 13:45, Volker Rümelin wrote: Am 15.01.23 um 14:08 schrieb Volker Rümelin: Ccing a few more people who might be interested in this patch series. @Mark: After this patch series, the code in your out of tree ASC audio device (and a few in tree audio devices) could be simplified. write_audio() and the loops calling write_audio() could be removed. Hi Volker, I know we have discussed this in a separate thread off-list, but this is fantastic! Just out of interest, if the available bytes wraps the circular buffer will the audio core call the audio callback twice to maximise the ability of the guest to generate samples before the next audio timer? Or does that not make much difference in practice? I'm not too familiar with the audio subsystem, but a quick skim of the series looks good (and being able to remove the write_audio() loops is a big plus). So I would certainly give this a thumbs up: Acked-by: Mark Cave-Ayland ATB, Mark. With best regards, Volker Based-on: <3b1404eb-a7c5-f64c-3e47-1397c54c4...@t-online.de> ([PATCH 00/11] audio: more improvements) The callback interface for emulated audio devices is strange. The callback function has an 'avail' parameter that passes the number of bytes that can be written or read. Unfortunately, this value sometimes is only an imprecise estimate and the callback functions must check the actual bytes written or read. For playback devices, this means that they either need a ring buffer or have to write the unwritten bytes again the next time. For recording devices, things are a bit easier. They only need to continue with the actual number of bytes read. After this patch series, the 'avail' argument for the -audiodev out.mixing-engine=on and in.mixing-engine=on cases is exact. Audio frontends only need a linear frame buffer and there's a guarantee they can write or read 'avail' bytes. The -audiodev out.mixing-engine=off case is also mostly accurate. Only the D-Bus audio backend is still missing a required function. The -audiodev in.mixing-engine=off case always passes a much too large 'avail' value. I haven't worked on this yet, because there was no reason for it so far. The following logs show the improvements. Not only the audio frontends can write or read all needed or available bytes. The same is true for the audio backends. For playback, the first six lines in the logs are expected. Here you can see how quickly the guest fills the empty downstream buffers after playback starts. QEMU was started with -device ich9-intel-hda,addr=0x1b -device hda-duplex,audiodev=audio0 -audiodev pa,out.frequency=96000,in.frequency=96000,id=audio0 playback guest 44100Hz => host 96000Hz unpatched version: hda_audio_output_cb: to write 8188, written 1704 audio_run_out: free 4458, played 926 hda_audio_output_cb: to write 6488, written 2384 audio_run_out: free 3532, played 1297 hda_audio_output_cb: to write 4104, written 2648 audio_run_out: free 2235, played 1441 audio_run_out: free 794, played 793 audio_run_out: free 897, played 896 audio_run_out: free 831, played 829 ... hda_audio_output_cb: could not write 4 bytes hda_audio_output_cb: to write 1764, written 1760 audio_run_out: free 960, played 958 ... patched version: hda_audio_output_cb: to write 8192, written 1620 audio_run_out: free 4458, played 880 hda_audio_output_cb: to write 6576, written 2508 audio_run_out: free 3578, played 1365 hda_audio_output_cb: to write 4068, written 2500 audio_run_out: free 2213, played 1360 record host 96000Hz => guest 44100Hz unpatched version: audio_run_in: avail 4458, acquired 4454 audio_run_in: avail 1574, acquired 1572 audio_run_in: avail 766, acquired 764 audio_run_in: avail 1052, acquired 1051 audio_run_in: avail 761, acquired 760 audio_run_in: avail 1123, acquired 1121 ... hda_audio_input_cb: could not read 4 bytes hda_audio_input_cb: to read 1988, read 1984 audio_run_in: avail 1082, acquired 1080 ... patched version: (no output) QEMU was started with -device ich9-intel-hda,addr=0x1b -device hda-duplex,audiodev=audio0 -audiodev pa,out.frequency=32000,in.frequency=32000,id=audio0 playback guest 44100Hz => host 32000Hz unpatched version: hda_audio_output_cb: to write 8188, written 1620 audio_run_out: free 1486, played 294 hda_audio_output_cb: to write 6568, written 2512 audio_run_out: free 1192, played 455 hda_audio_output_cb: to write 4060, written 2504 audio_run_out: free 737, played 455 audio_run_out: free 282, played 281 audio_run_out: free 357, played 356 audio_run_out: free 314, played 313 ... hda_audio_output_cb: could not write 4 bytes hda_audio_output_cb: to write 1416, written 1412 audio_run_out: free 257, played 256 ... patched version: hda_audio_output_cb: to write 8192, written 1656 audio_run_out: free 1486, played 300 hda_audio_output_cb: to write 6536, written 2516 audio_run_out: free 1186, played 457 hda_audio_output_cb: to write 4020, written 2540 audio_run_out: free 729, played 460 record host 32000Hz => g
Re: [PATCH v5] Emulate dip switch language layout settings on SUN keyboard
On 14/01/2023 14:38, Henrik Carlqvist wrote:
https://patchew.org/QEMU/20230114125029.7395a547.hc...@poolhem.se/
complains that "patch is empty", so here is my fifth attempt...
regards Henrik
SUN Type 4, 5 and 5c keyboards have dip switches to choose the language
layout of the keyboard. Solaris makes an ioctl to query the value of the
dipswitches and uses that value to select keyboard layout. Also the SUN
bios like the one in the file ss5.bin uses this value to support at least
some keyboard layouts. However, the OpenBIOS provided with qemu is
hardcoded to always use an US keyboard layout.
Before this patch, qemu allways gave dip switch value 0x21 (US keyboard),
this patch uses the command line switch "-k" (keyboard layout) to select
dip switch value. A table is used to lookup values from arguments like:
-k fr
-k es
But the patch also accepts numeric dip switch values directly to the -k
switch:
-k 0x2b
-k 43
Both values above are the same and select swedish keyboard as explained in
table 3-15 at
https://docs.oracle.com/cd/E19683-01/806-6642/new-43/index.html
Unless you want to do a full Solaris installation but happen to have
access to a bios file, the easiest way to test that the patch works is to:
qemu-system-sparc -k sv -bios /path/to/ss5.bin
If you already happen to have a Solaris installation in a qemu disk image
file you can easily try different keyboard layouts after this patch is
applied.
Signed-off-by: Henrik Carlqvist
---
hw/char/escc.c | 74 +-
1 file changed, 73 insertions(+), 1 deletion(-)
diff --git a/hw/char/escc.c b/hw/char/escc.c
index 17a908c59b..53022ccf39 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -31,6 +31,8 @@
#include "qemu/module.h"
#include "hw/char/escc.h"
#include "ui/console.h"
+#include "sysemu/sysemu.h"
+#include "qemu/cutils.h"
#include "trace.h"
/*
@@ -190,6 +192,7 @@
#define R_MISC1I 14
#define R_EXTINT 15
+static unsigned char sun_keyboard_layout_dip_switch(void);
static void handle_kbd_command(ESCCChannelState *s, int val);
static int serial_can_receive(void *opaque);
static void serial_receive_byte(ESCCChannelState *s, int ch);
@@ -846,6 +849,75 @@ static QemuInputHandler sunkbd_handler = {
.event = sunkbd_handle_event,
};
+static unsigned char sun_keyboard_layout_dip_switch(void)
+{
+/* Return the value of the dip-switches in a SUN Type 5 keyboard */
+static unsigned char ret = 0xff;
+
+if ((ret == 0xff) && keyboard_layout) {
+int i;
+struct layout_values {
+const char *lang;
+unsigned char dip;
+} languages[] =
+/* Dip values from table 3-16 Layouts for Type 4, 5, and 5c Keyboards */
+{
+{"en-us", 0x21}, /* U.S.A. (US5.kt) */
+ /* 0x22 is some other US (US_UNIX5.kt)*/
+{"fr",0x23}, /* France (France5.kt) */
+{"da",0x24}, /* Denmark (Denmark5.kt) */
+{"de",0x25}, /* Germany (Germany5.kt) */
+{"it",0x26}, /* Italy (Italy5.kt) */
+{"nl",0x27}, /* The Netherlands (Netherland5.kt) */
+{"no",0x28}, /* Norway (Norway.kt) */
+{"pt",0x29}, /* Portugal (Portugal5.kt) */
+{"es",0x2a}, /* Spain (Spain5.kt) */
+{"sv",0x2b}, /* Sweden (Sweden5.kt) */
+{"fr-ch", 0x2c}, /* Switzerland/French (Switzer_Fr5.kt) */
+{"de-ch", 0x2d}, /* Switzerland/German (Switzer_Ge5.kt) */
+{"en-gb", 0x2e}, /* Great Britain (UK5.kt) */
+{"ko",0x2f}, /* Korea (Korea5.kt) */
+{"tw",0x30}, /* Taiwan (Taiwan5.kt) */
+{"ja",0x31}, /* Japan (Japan5.kt) */
+{"fr-ca", 0x32}, /* Canada/French (Canada_Fr5.kt) */
+{"hu",0x33}, /* Hungary (Hungary5.kt) */
+{"pl",0x34}, /* Poland (Poland5.kt) */
+{"cz",0x35}, /* Czech (Czech5.kt) */
+{"ru",0x36}, /* Russia (Russia5.kt) */
+{"lv",0x37}, /* Latvia (Latvia5.kt) */
+{"tr",0x38}, /* Turkey-Q5 (TurkeyQ5.kt) */
+{"gr",0x39}, /* Greece (Greece5.kt) */
+{"ar",0x3a}, /* Arabic (Arabic5.kt) */
+{"lt",0x3b}, /* Lithuania (Lithuania5.kt) */
+{"nl-be", 0x3c}, /* Belgium (Belgian5.kt) */
+{"be",0x3c}, /* Belgium (Belgian5.kt) */
+};
+
+for (i = 0;
+ i < sizeof(languages) / sizeof(struct layout_values);
+ i++) {
+if (!strcmp(keyboard_layout, languages[i].lang)) {
+ret = languages[i].dip;
+return ret;
+}
+}
+/* Found no known language code */
+
+if ((keyboard_layout[0] >= '0') && (keyboard_layout[0] <= '9')) {
+
Re: [PATCH v7 6/7] mac_newworld: Deprecate mac99 "via" option
On 12/01/2023 23:51, BALATON Zoltan wrote: On Thu, 12 Jan 2023, Howard Spoelstra wrote: On Wed, Jan 11, 2023 at 1:15 AM BALATON Zoltan wrote: On Tue, 10 Jan 2023, Mark Cave-Ayland wrote: On 04/01/2023 21:59, BALATON Zoltan wrote: Setting emulated machine type with a property called "via" is confusing users so deprecate the "via" option in favour of newly added explicit machine types. The default via=cuda option is not a valid config (no real Mac has this combination of hardware) so no machine type could be defined for that therefore it is kept for backwards compatibility with older QEMU versions for now but other options resembling real machines are deprecated. Signed-off-by: BALATON Zoltan I believe that people do use -M mac99,via=cuda to run some rare versions of MacOS in QEMU (I think possibly OS X DP and Workgroup Server?), so we would want to keep this option somewhere. The idea is that after previous patches we now have machine types for all other via option values (that also match real Mac machines) other than via=cude but that is the default for mac99 so after the reprecation period when the via option is removed mac99 (which is the same as mac99,via=cuda) can remain for this use case (and for backward compatibility) until the other machines are fixed to not need this any more. So all via options are still available but as different machine types. My 2 cents about naming: It seems less important how the machines are named when their name is not covering their definition. F.i. the powermac3,1 never had adb, could not be equipped with a G3 cpu, did not run at 900Mhz. The closest possible qemu-options based definition of a powermac3,1 (via=pmu) will not run Mac OS 9.0.4 ;-) due to the 2 USB devices problem. To run that via=cuda is already needed. What does that mean? Should we aim to emulate real Macs or are we happy with the Franken-Mac we have now? The names also show what we intend to emulate even though the emulation may not be complete or have bugs (this is also true for other machines in QEMU where a lot of them are not fully emulated, only well enough to boot guest OSes). Looks like everybody has forgotten the previous discussion and not read the docs and deprecation patches where this is explained so I summarise the proposed change here again: - qemu-system-ppc -M mac99 is unchanged and works like before it just warns for the via option and when using it in qemu-system-ppc64 suggesting using new machines instead so these could evetually be removed next year. mac99,via=cuda is just mac99 so you can continue to use that, mac99 is not deprecated and don't want to remove it. - qemu-system-ppc64 -M mac99 -> powermac7_3 - qemu-system-ppc -M mac99,via=pmu -> powermac3,1 - qemu-system-ppc64 -M mac99,via=pmu-adb -> powerbook3_2 The last one is one of the rare Macs that had adb and pmu, all others with pmu usually have USB. The PowerMac1,2 (G4 PCI) had CUDA but not with mac99 hardware but more similar to g3beige and no ADB ports according to https://en.wikipedia.org/wiki/Power_Mac_G4#1st_generation:_Graphite https://en.wikipedia.org/wiki/Power_Macintosh_G3_(Blue_and_White)#Hardware The PowerMac7,3 seems to be matching the PCI device listing in the comment at the beginning of mac_newworld.c and also this article: https://www.informit.com/articles/article.aspx?p=606582 What is the 2 USB devices problem? Is it the one we've debugged before and found that it's noted in a comment marked with ??? in hw/usb/hcd-ohci.c? That could be fixed if there was somebody interested enough to provide a patch. But this series does not remove the mac99 and does not even deprecate it. What it deprecates are the via option to select different machine types and the automatic detection of ppc64 to emulate something different which are hard to understand for users and caused several misunderstandings. It's much more clear to have a separate machine type for each machine we emulate even when they aren't yet complete but at least we know which way to go and can compare to real hardware and fix the missing parts later. Also introducing powermac7_3 to split the ppc64 mac99 would allow to remove qemu-system-ppc if we wanted and only have one executable for all machines but even without this it's clearer to have separate machnies for G5 and G4 macs than mac99 silently behaving differently. Ultimately the issue you are trying to solve is this, which is that -M mac99 is different for qemu-system-ppc and qemu-system-ppc64. Perhaps the best way to start is to create a new "g5niagara" machine type (including OpenBIOS) and make it a clone of mac_newworld.c, and then issue a warning on qemu-system-ppc64 for -M mac99. The reason for suggesting this is that the number of users of qemu-system-ppc64 -M mac99 will be much smaller than those using qemu-system-ppc, which means there will be a lot less breakage for users. In the meantime we don't need to make a final decision r
Re: [PATCH v7 3/7] mac_{old,new}world: Pass MacOS VGA NDRV in card ROM instead of fw_cfg
On 11/01/2023 00:54, BALATON Zoltan wrote:
On Tue, 10 Jan 2023, Mark Cave-Ayland wrote:
On 04/01/2023 21:59, BALATON Zoltan wrote:
OpenBIOS cannot run FCode ROMs yet but it can detect NDRV in VGA card
ROM and add it to the device tree for MacOS. Pass the NDRV this way
instead of via fw_cfg. This solves the problem with OpenBIOS also
adding the NDRV to ati-vga which it does not work with. This does not
need any changes to OpenBIOS as this NDRV ROM handling is already
there but this patch also allows simplifying OpenBIOS later to remove
the fw_cfg ndrv handling from the vga FCode and also drop the
vga-ndrv? option which is not needed any more as users can disable the
ndrv with -device VGA,romfile="" (or override it with their own NDRV
or ROM). Once FCode support is implemented in OpenBIOS, the proper
FCode ROM can be set the same way so this paves the way to remove some
hacks.
Signed-off-by: BALATON Zoltan
---
hw/ppc/mac_newworld.c | 18 ++
hw/ppc/mac_oldworld.c | 18 ++
2 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 460c14b5e3..60c9c27986 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -510,18 +510,6 @@ static void ppc_core99_init(MachineState *machine)
fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_BUSFREQ, BUSFREQ);
fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_NVRAM_ADDR, nvram_addr);
- /* MacOS NDRV VGA driver */
- filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, NDRV_VGA_FILENAME);
- if (filename) {
- gchar *ndrv_file;
- gsize ndrv_size;
-
- if (g_file_get_contents(filename, &ndrv_file, &ndrv_size, NULL)) {
- fw_cfg_add_file(fw_cfg, "ndrv/qemu_vga.ndrv", ndrv_file,
ndrv_size);
- }
- g_free(filename);
- }
-
qemu_register_boot_set(fw_cfg_boot_set, fw_cfg);
}
@@ -565,6 +553,11 @@ static int core99_kvm_type(MachineState *machine, const
char *arg)
return 2;
}
+static GlobalProperty props[] = {
+ /* MacOS NDRV VGA driver */
+ { "VGA", "romfile", NDRV_VGA_FILENAME },
+};
+
static void core99_machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -585,6 +578,7 @@ static void core99_machine_class_init(ObjectClass *oc, void
*data)
#endif
mc->default_ram_id = "ppc_core99.ram";
mc->ignore_boot_device_suffixes = true;
+ compat_props_add(mc->compat_props, props, G_N_ELEMENTS(props));
fwc->get_dev_path = core99_fw_dev_path;
}
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 5a7b25a4a8..6a1b1ad47a 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -344,18 +344,6 @@ static void ppc_heathrow_init(MachineState *machine)
fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_CLOCKFREQ, CLOCKFREQ);
fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_BUSFREQ, BUSFREQ);
- /* MacOS NDRV VGA driver */
- filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, NDRV_VGA_FILENAME);
- if (filename) {
- gchar *ndrv_file;
- gsize ndrv_size;
-
- if (g_file_get_contents(filename, &ndrv_file, &ndrv_size, NULL)) {
- fw_cfg_add_file(fw_cfg, "ndrv/qemu_vga.ndrv", ndrv_file,
ndrv_size);
- }
- g_free(filename);
- }
-
qemu_register_boot_set(fw_cfg_boot_set, fw_cfg);
}
@@ -400,6 +388,11 @@ static int heathrow_kvm_type(MachineState *machine, const
char *arg)
return 2;
}
+static GlobalProperty props[] = {
+ /* MacOS NDRV VGA driver */
+ { "VGA", "romfile", NDRV_VGA_FILENAME },
+};
+
static void heathrow_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -420,6 +413,7 @@ static void heathrow_class_init(ObjectClass *oc, void *data)
mc->default_display = "std";
mc->ignore_boot_device_suffixes = true;
mc->default_ram_id = "ppc_heathrow.ram";
+ compat_props_add(mc->compat_props, props, G_N_ELEMENTS(props));
fwc->get_dev_path = heathrow_fw_dev_path;
}
The qemu_vga.ndrv is deliberately kept separate from the PCI option ROM because it
is a binary generated by a separate project: otherwise you'd end up creating a
dependency between OpenBIOS and QemuMacDrivers, which is almost impossible to
achieve since qemu_vga.ndrv can only (currently) be built in an emulated MacOS 9
guest.
I don't get this. The dependency is already there as qemu_vga.ndrv ships with QEMU
such as all the vgabios-*.bin and SeaBIOS binaries which are also built from
different projects. The qemu_vga.ndrv would also still be part of an FCode ROM
together with vga.fs if OpenBIOS could run that so this patch solely changes the way
of passing the ROM binary to OpenBIOS from fw_cfg to the card ROM which is closer to
how it should be and can direcly be replaced with the FCode ROM later after OpenBIOS
will be advanced to that point.
Even if OpenBIOS were able to execute PCI option ROMs, the problem is that OpenBIOS
cannot generate the qemu_vga.ndrv bin
Re: [PATCH v7 4/7] mac_newworld: Add machine types for different mac99 configs
On 11/01/2023 00:36, BALATON Zoltan wrote: On Tue, 10 Jan 2023, Mark Cave-Ayland wrote: On 04/01/2023 21:59, BALATON Zoltan wrote: The mac99 machine emulates different machines depending on machine properties or even if it is run as qemu-system-ppc64 or qemu-system-ppc. This is very confusing for users and many hours were lost trying to explain it or finding out why commands users came up with are not working as expected. (E.g. Windows users might think qemu-system-ppc64 is just the 64 bit version of qemu-system-ppc and then fail to boot a 32 bit OS with -M mac99 trying to follow an example that had qemu-system-ppc.) To avoid such confusion, add explicit machine types for the different configs which will work the same with both qemu-system-ppc and qemu-system-ppc64 and also make the command line clearer for new users. Signed-off-by: BALATON Zoltan Some thoughts on this: the first is that not everyone agrees that for qemu-system-X that X represents the target. There were previous discussion where some KVM people assumed X represented the host, i.e. ppc64 was the binary that ran all PPC guests but with hardware acceleration for ppc64 guests on ppc64 hosts. This was a while ago, so it may be worth starting a thread on qemu-devel to see what the current consensus is. I don't see how this is relevant to this series, Also likely not the case any more as qemu-system-ppc and qemu-system-ppc64 share most of the code since a while with ppc64 including the config of ppc and adding more machines. Well the patch defines the powermac 7.3 machine just for TARGET_PPC64, no? So you're making the assumption qemu-system-ppc64 represents a 64-bit target rather than a 64-bit host. Secondly it's not clear to me why you've chosen names like "powermac_3_1" instead of "g4agp"? Does powermac_3_1 uniquely identify the G4 AGP Sawtooth model? For QEMU it is always best to emulate real machines, and whilst I understand you want to separate out the two versions of the mac99 machine, having "powermac_X_Y" seems less clear to me. These machine model identifiers are used by Apple to uniquely identify (all of) their machines since new-world Macs (even modern iPads and Macs have them) so for Mac people this should be clearer than the informal names that could get a bit long and confusing as there may be slight differences within a family. In any case, qemu-system-ppc -M mac99 is not corresponding to any real Mac so I'd like the options which do emulate real Macs to be called in a name that show which Mac is that. For the PPC Macs there's some info here for example: https://en.wikipedia.org/wiki/Power_Mac_G4 And everymac.com also has info on all Macs. There were actually more than one G4 PowerMac with AGP but the other one was informally called gigabit ethernet. So the model ID is a shorter and better way to clearly identify which hardware is it (and it's also referenced in the device-tree of these Macs). Are you planning to work on different types of G4 Mac where this could be confusing? Even to me "PowerMac 3.1" doesn't really tell me what model of Mac is being emulated, whereas "g4agp" (much in the same way as g3beige) is much more friendlier to people interested in using QEMU for Mac emulation. Finally can you post links to the device trees that you are using for each of the new machine types so that we have a clear reference point for future changes to the QEMU Mac machines? Even better include the links in the comments for each machine so that the information is easily visible for developers. I still have those I've posted over the past 8 years when I made changes to OpenBIOS to make the device-tree closer to real machine. I've downloaded it back then, don't know where to find it now but searching for e.g. "PowerMac3,1" "device-tree" should get some results. Nothing shows up for me, I'm afraid (remember that Google searches are unique to each user). If you want argue for changing the QEMU machines, then we should agree on the reference device model for future changes. ATB, Mark.
[PATCH 6/7] hw/acpi: Trace GPE access in all device models, not just PIIX4
Signed-off-by: Bernhard Beschow
---
hw/acpi/core.c | 5 +
hw/acpi/piix4.c | 3 ---
hw/acpi/trace-events | 8
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 6da275c599..a33e410e69 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -32,6 +32,7 @@
#include "qemu/module.h"
#include "qemu/option.h"
#include "sysemu/runstate.h"
+#include "trace.h"
struct acpi_table_header {
uint16_t _length; /* our length, not actual part of the hdr */
@@ -686,6 +687,8 @@ void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr,
uint32_t val)
{
uint8_t *cur;
+trace_acpi_gpe_ioport_writeb(addr, val);
+
cur = acpi_gpe_ioport_get_ptr(ar, addr);
if (addr < ar->gpe.len / 2) {
/* GPE_STS */
@@ -709,6 +712,8 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
val = *cur;
}
+trace_acpi_gpe_ioport_readb(addr, val);
+
return val;
}
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 836f9026b1..b65ddb8e44 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -45,7 +45,6 @@
#include "hw/acpi/acpi_dev_interface.h"
#include "migration/vmstate.h"
#include "hw/core/cpu.h"
-#include "trace.h"
#include "qom/object.h"
#define GPE_BASE 0xafe0
@@ -510,7 +509,6 @@ static uint64_t gpe_readb(void *opaque, hwaddr addr,
unsigned width)
PIIX4PMState *s = opaque;
uint32_t val = acpi_gpe_ioport_readb(&s->ar, addr);
-trace_piix4_gpe_readb(addr, width, val);
return val;
}
@@ -519,7 +517,6 @@ static void gpe_writeb(void *opaque, hwaddr addr, uint64_t
val,
{
PIIX4PMState *s = opaque;
-trace_piix4_gpe_writeb(addr, width, val);
acpi_gpe_ioport_writeb(&s->ar, addr, val);
acpi_update_sci(&s->ar, s->irq);
}
diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
index 78e0a8670e..159937ddb9 100644
--- a/hw/acpi/trace-events
+++ b/hw/acpi/trace-events
@@ -17,6 +17,10 @@ mhp_acpi_clear_remove_evt(uint32_t slot) "slot[0x%"PRIx32"]
clear remove event"
mhp_acpi_pc_dimm_deleted(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm deleted"
mhp_acpi_pc_dimm_delete_failed(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm
delete failed"
+# core.c
+acpi_gpe_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==>
0x%" PRIx8
+acpi_gpe_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " <==
0x%" PRIx8
+
# cpu.c
cpuhp_acpi_invalid_idx_selected(uint32_t idx) "0x%"PRIx32
cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) "idx[0x%"PRIx32"] flags:
0x%"PRIx8
@@ -48,10 +52,6 @@ acpi_pci_sel_read(uint32_t val) "%" PRIu32
acpi_pci_ej_write(uint64_t addr, uint64_t data) "0x%" PRIx64 " <== %" PRIu64
acpi_pci_sel_write(uint64_t addr, uint64_t data) "0x%" PRIx64 " <== %" PRIu64
-# piix4.c
-piix4_gpe_readb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%"
PRIx64 " width: %d ==> 0x%" PRIx64
-piix4_gpe_writeb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%"
PRIx64 " width: %d <== 0x%" PRIx64
-
# tco.c
tco_timer_reload(int ticks, int msec) "ticks=%d (%d ms)"
tco_timer_expired(int timeouts_no, bool strap, bool no_reboot) "timeouts_no=%d
no_reboot=%d/%d"
--
2.39.1
[PATCH 5/7] hw/acpi/piix4: Fix offset of GPE0 registers
The PIIX4 datasheet defines the GPSTS register to be at offset 0x0c of the
power management I/O register block. This register block is represented
in the device model by the io attribute. So make io_gpe a child memory
region of io at offset 0x0c.
Note that SeaBIOS sets the base address of the register block to 0x600,
resulting in the io_gpe block to start at 0x60c. GPE_BASE is defined as
0xafe0 which is 0xa9d4 bytes off. In order to preserve compatibilty,
create an io_gpe_qemu memory region alias at GPE_BASE.
Signed-off-by: Bernhard Beschow
---
include/hw/acpi/piix4.h | 1 +
hw/acpi/piix4.c | 9 +++--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
index 62e1925a1f..4e6cad9e8c 100644
--- a/include/hw/acpi/piix4.h
+++ b/include/hw/acpi/piix4.h
@@ -40,6 +40,7 @@ struct PIIX4PMState {
MemoryRegion io;
MemoryRegion io_gpe;
+MemoryRegion io_gpe_qemu;
ACPIREGS ar;
APMState apm;
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2e9bc63fca..836f9026b1 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -49,6 +49,7 @@
#include "qom/object.h"
#define GPE_BASE 0xafe0
+#define GPE_OFS 0xc
#define GPE_LEN 4
#define ACPI_PCIHP_ADDR_PIIX4 0xae00
@@ -429,7 +430,7 @@ static void piix4_pm_add_properties(PIIX4PMState *s)
object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
&acpi_disable_cmd, OBJ_PROP_FLAG_READ);
object_property_add_uint64_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
- &s->io_gpe.addr, OBJ_PROP_FLAG_READ);
+ &s->io_gpe_qemu.addr, OBJ_PROP_FLAG_READ);
object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
&s->ar.gpe.len, OBJ_PROP_FLAG_READ);
object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
@@ -558,7 +559,11 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion
*parent,
{
memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
"acpi-gpe0", GPE_LEN);
-memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
+memory_region_add_subregion(&s->io, GPE_OFS, &s->io_gpe);
+
+memory_region_init_alias(&s->io_gpe_qemu, OBJECT(s), "acpi-gpe0-qemu",
+ &s->io_gpe, 0, memory_region_size(&s->io_gpe));
+memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe_qemu);
if (s->use_acpi_hotplug_bridge || s->use_acpi_root_pci_hotplug) {
acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
--
2.39.1
[PATCH 2/7] hw/acpi/ich9: Remove unneeded assignments
The first thing ich9_pm_iospace_update() does is to set pm->pm_io_base to
the pm_io_base parameter. The pm_io_base parameter's value is the old
one of pm->pm_io_base.
Signed-off-by: Bernhard Beschow
---
hw/acpi/ich9.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 2050af67b9..0313e71e74 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -136,9 +136,7 @@ void ich9_pm_iospace_update(ICH9LPCPMRegs *pm, uint32_t
pm_io_base)
static int ich9_pm_post_load(void *opaque, int version_id)
{
ICH9LPCPMRegs *pm = opaque;
-uint32_t pm_io_base = pm->pm_io_base;
-pm->pm_io_base = 0;
-ich9_pm_iospace_update(pm, pm_io_base);
+ich9_pm_iospace_update(pm, pm->pm_io_base);
return 0;
}
--
2.39.1
[PATCH 3/7] hw/acpi/{ich9, piix4}: Resolve redundant io_base address attributes
A MemoryRegion has an addr attribute which gets set to the same values
as the redundant io_addr attributes.
Signed-off-by: Bernhard Beschow
---
include/hw/acpi/ich9.h | 1 -
include/hw/acpi/piix4.h | 2 --
hw/acpi/ich9.c | 17 -
hw/acpi/piix4.c | 11 ++-
4 files changed, 14 insertions(+), 17 deletions(-)
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index d41866a229..22471a1b9d 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -49,7 +49,6 @@ typedef struct ICH9LPCPMRegs {
qemu_irq irq; /* SCI */
-uint32_t pm_io_base;
Notifier powerdown_notifier;
bool cpu_hotplug_legacy;
diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
index be1f8ea80e..62e1925a1f 100644
--- a/include/hw/acpi/piix4.h
+++ b/include/hw/acpi/piix4.h
@@ -39,8 +39,6 @@ struct PIIX4PMState {
/*< public >*/
MemoryRegion io;
-uint32_t io_base;
-
MemoryRegion io_gpe;
ACPIREGS ar;
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 0313e71e74..f8af238974 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -126,17 +126,16 @@ void ich9_pm_iospace_update(ICH9LPCPMRegs *pm, uint32_t
pm_io_base)
assert((pm_io_base & ICH9_PMIO_MASK) == 0);
-pm->pm_io_base = pm_io_base;
memory_region_transaction_begin();
-memory_region_set_enabled(&pm->io, pm->pm_io_base != 0);
-memory_region_set_address(&pm->io, pm->pm_io_base);
+memory_region_set_enabled(&pm->io, pm_io_base != 0);
+memory_region_set_address(&pm->io, pm_io_base);
memory_region_transaction_commit();
}
static int ich9_pm_post_load(void *opaque, int version_id)
{
ICH9LPCPMRegs *pm = opaque;
-ich9_pm_iospace_update(pm, pm->pm_io_base);
+ich9_pm_iospace_update(pm, pm->io.addr);
return 0;
}
@@ -349,9 +348,9 @@ static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
const char *name,
void *opaque, Error **errp)
{
ICH9LPCPMRegs *pm = opaque;
-uint32_t value = pm->pm_io_base + ICH9_PMIO_GPE0_STS;
+uint64_t value = pm->io.addr + ICH9_PMIO_GPE0_STS;
-visit_type_uint32(v, name, &value, errp);
+visit_type_uint64(v, name, &value, errp);
}
static bool ich9_pm_get_memory_hotplug_support(Object *obj, Error **errp)
@@ -440,9 +439,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
pm->keep_pci_slot_hpc = true;
pm->enable_tco = true;
-object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
- &pm->pm_io_base, OBJ_PROP_FLAG_READ);
-object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
+object_property_add_uint64_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
+ &pm->io.addr, OBJ_PROP_FLAG_READ);
+object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint64",
ich9_pm_get_gpe0_blk,
NULL, NULL, pm);
object_property_add_uint8_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 370b34eacf..2e9bc63fca 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -91,13 +91,14 @@ static void apm_ctrl_changed(uint32_t val, void *arg)
static void pm_io_space_update(PIIX4PMState *s)
{
PCIDevice *d = PCI_DEVICE(s);
+uint32_t io_base;
-s->io_base = le32_to_cpu(*(uint32_t *)(d->config + 0x40));
-s->io_base &= 0xffc0;
+io_base = le32_to_cpu(*(uint32_t *)(d->config + 0x40));
+io_base &= 0xffc0;
memory_region_transaction_begin();
memory_region_set_enabled(&s->io, d->config[0x80] & 1);
-memory_region_set_address(&s->io, s->io_base);
+memory_region_set_address(&s->io, io_base);
memory_region_transaction_commit();
}
@@ -433,8 +434,8 @@ static void piix4_pm_add_properties(PIIX4PMState *s)
&s->ar.gpe.len, OBJ_PROP_FLAG_READ);
object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
&sci_int, OBJ_PROP_FLAG_READ);
-object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
- &s->io_base, OBJ_PROP_FLAG_READ);
+object_property_add_uint64_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
+ &s->io.addr, OBJ_PROP_FLAG_READ);
}
static void piix4_pm_realize(PCIDevice *dev, Error **errp)
--
2.39.1
[PATCH 1/7] hw/acpi/{ich9, piix4}: Reuse existing attributes for QOM properties
The QOM properties are accessed after the device models have been
realized. This means that the constants are redundant. Remove them.
Signed-off-by: Bernhard Beschow
---
hw/acpi/ich9.c | 5 ++---
hw/acpi/piix4.c | 10 --
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index a93c470e9d..2050af67b9 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -433,7 +433,6 @@ static void ich9_pm_set_keep_pci_slot_hpc(Object *obj, bool
value, Error **errp)
void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
{
-static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
pm->acpi_memory_hotplug.is_enabled = true;
pm->cpu_hotplug_legacy = true;
pm->disable_s3 = 0;
@@ -448,8 +447,8 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
object_property_add(obj, ACPI_PM_PROP_GPE0_BLK, "uint32",
ich9_pm_get_gpe0_blk,
NULL, NULL, pm);
-object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
- &gpe0_len, OBJ_PROP_FLAG_READ);
+object_property_add_uint8_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
+ &pm->acpi_regs.gpe.len, OBJ_PROP_FLAG_READ);
object_property_add_bool(obj, "memory-hotplug-support",
ich9_pm_get_memory_hotplug_support,
ich9_pm_set_memory_hotplug_support);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 0a81f1ad93..370b34eacf 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -421,18 +421,16 @@ static void piix4_pm_add_properties(PIIX4PMState *s)
{
static const uint8_t acpi_enable_cmd = ACPI_ENABLE;
static const uint8_t acpi_disable_cmd = ACPI_DISABLE;
-static const uint32_t gpe0_blk = GPE_BASE;
-static const uint32_t gpe0_blk_len = GPE_LEN;
static const uint16_t sci_int = 9;
object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_ENABLE_CMD,
&acpi_enable_cmd, OBJ_PROP_FLAG_READ);
object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_ACPI_DISABLE_CMD,
&acpi_disable_cmd, OBJ_PROP_FLAG_READ);
-object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
- &gpe0_blk, OBJ_PROP_FLAG_READ);
-object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
- &gpe0_blk_len, OBJ_PROP_FLAG_READ);
+object_property_add_uint64_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK,
+ &s->io_gpe.addr, OBJ_PROP_FLAG_READ);
+object_property_add_uint8_ptr(OBJECT(s), ACPI_PM_PROP_GPE0_BLK_LEN,
+ &s->ar.gpe.len, OBJ_PROP_FLAG_READ);
object_property_add_uint16_ptr(OBJECT(s), ACPI_PM_PROP_SCI_INT,
&sci_int, OBJ_PROP_FLAG_READ);
object_property_add_uint32_ptr(OBJECT(s), ACPI_PM_PROP_PM_IO_BASE,
--
2.39.1
[PATCH 4/7] hw/acpi/ich9: Use ICH9_PMIO_GPE0_STS just once
Cosmetic change emphasizing that always the actual address of the gpe0
block is returned.
Signed-off-by: Bernhard Beschow
---
hw/acpi/ich9.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index f8af238974..40a20e01ea 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -348,7 +348,9 @@ static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
const char *name,
void *opaque, Error **errp)
{
ICH9LPCPMRegs *pm = opaque;
-uint64_t value = pm->io.addr + ICH9_PMIO_GPE0_STS;
+uint64_t value = pm->io.addr + pm->io_gpe.addr;
+
+assert(&pm->io == pm->io_gpe.container);
visit_type_uint64(v, name, &value, errp);
}
--
2.39.1
[PATCH 7/7] hw/acpi/core: Trace enable and status registers of GPE separately
The bit positions of both registers are related. Tracing the registers
independently results in the same offsets across these registers which
eases debugging.
Signed-off-by: Bernhard Beschow
---
hw/acpi/core.c | 10 +++---
hw/acpi/trace-events | 6 --
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index a33e410e69..cc33605d61 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -687,13 +687,13 @@ void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr,
uint32_t val)
{
uint8_t *cur;
-trace_acpi_gpe_ioport_writeb(addr, val);
-
cur = acpi_gpe_ioport_get_ptr(ar, addr);
if (addr < ar->gpe.len / 2) {
+trace_acpi_gpe_sts_ioport_writeb(addr, val);
/* GPE_STS */
*cur = (*cur) & ~val;
} else if (addr < ar->gpe.len) {
+trace_acpi_gpe_en_ioport_writeb(addr - (ar->gpe.len / 2), val);
/* GPE_EN */
*cur = val;
} else {
@@ -712,7 +712,11 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr)
val = *cur;
}
-trace_acpi_gpe_ioport_readb(addr, val);
+if (addr < ar->gpe.len / 2) {
+trace_acpi_gpe_sts_ioport_readb(addr, val);
+} else {
+trace_acpi_gpe_en_ioport_readb(addr - (ar->gpe.len / 2), val);
+}
return val;
}
diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
index 159937ddb9..d387adfb0b 100644
--- a/hw/acpi/trace-events
+++ b/hw/acpi/trace-events
@@ -18,8 +18,10 @@ mhp_acpi_pc_dimm_deleted(uint32_t slot) "slot[0x%"PRIx32"]
pc-dimm deleted"
mhp_acpi_pc_dimm_delete_failed(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm
delete failed"
# core.c
-acpi_gpe_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==>
0x%" PRIx8
-acpi_gpe_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " <==
0x%" PRIx8
+acpi_gpe_sts_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==>
0x%" PRIx8
+acpi_gpe_en_ioport_readb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " ==>
0x%" PRIx8
+acpi_gpe_sts_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 "
<== 0x%" PRIx8
+acpi_gpe_en_ioport_writeb(uint32_t addr, uint8_t val) "addr: 0x%" PRIx32 " <==
0x%" PRIx8
# cpu.c
cpuhp_acpi_invalid_idx_selected(uint32_t idx) "0x%"PRIx32
--
2.39.1
[PATCH 0/7] ACPI controller cleanup
This series brings the PIIX4 PM device closer to reality and resolves some
redundant code along the way.
Testing done:
- `make check`
- Starting a live CD under pc and q35 machines and check that the GPE accesses
are traced
Bernhard Beschow (7):
hw/acpi/{ich9,piix4}: Reuse existing attributes for QOM properties
hw/acpi/ich9: Remove unneeded assignments
hw/acpi/{ich9,piix4}: Resolve redundant io_base address attributes
hw/acpi/ich9: Use ICH9_PMIO_GPE0_STS just once
hw/acpi/piix4: Fix offset of GPE0 registers
hw/acpi: Trace GPE access in all device models, not just PIIX4
hw/acpi/core: Trace enable and status registers of GPE separately
include/hw/acpi/ich9.h | 1 -
include/hw/acpi/piix4.h | 3 +--
hw/acpi/core.c | 9 +
hw/acpi/ich9.c | 26 --
hw/acpi/piix4.c | 31 ---
hw/acpi/trace-events| 10 ++
6 files changed, 44 insertions(+), 36 deletions(-)
--
2.39.1
Re: [PATCH] vhost-user-fs: add capability to allow migration
On Sun, Jan 22, 2023 at 06:09:40PM +0200, Anton Kuchin wrote: > > On 22/01/2023 16:46, Michael S. Tsirkin wrote: > > On Sun, Jan 22, 2023 at 02:36:04PM +0200, Anton Kuchin wrote: > > > > > This flag should be set when qemu don't need to worry about any > > > > > external state stored in vhost-user daemons during migration: > > > > > don't fail migration, just pack generic virtio device states to > > > > > migration stream and orchestrator guarantees that the rest of the > > > > > state will be present at the destination to restore full context and > > > > > continue running. > > > > Sorry I still do not get it. So fundamentally, why do we need this > > > > property? > > > > vhost-user-fs is not created by default that we'd then need opt-in to > > > > the special "migrateable" case. > > > > That's why I said it might make some sense as a device property as qemu > > > > tracks whether device is unplugged for us. > > > > > > > > But as written, if you are going to teach the orchestrator about > > > > vhost-user-fs and its special needs, just teach it when to migrate and > > > > where not to migrate. > > > > > > > > Either we describe the special situation to qemu and let qemu > > > > make an intelligent decision whether to allow migration, > > > > or we trust the orchestrator. And if it's the latter, then 'migrate' > > > > already says orchestrator decided to migrate. > > > The problem I'm trying to solve is that most of vhost-user devices > > > now block migration in qemu. And this makes sense since qemu can't > > > extract and transfer backend daemon state. But this prevents us from > > > updating qemu executable via local migration. So this flag is > > > intended more as a safety check that says "I know what I'm doing". > > > > > > I agree that it is not really necessary if we trust the orchestrator > > > to request migration only when the migration can be performed in a > > > safe way. But changing the current behavior of vhost-user-fs from > > > "always blocks migration" to "migrates partial state whenever > > > orchestrator requests it" seems a little dangerous and can be > > > misinterpreted as full support for migration in all cases. > > It's not really different from block is it? orchestrator has to arrange > > for backend migration. I think we just assumed there's no use-case where > > this is practical for vhost-user-fs so we blocked it. > > But in any case it's orchestrator's responsibility. > > Yes, you are right. So do you think we should just drop the blocker > without adding a new flag? I'd be inclined to. I am curious what do dgilbert and stefanha think though. -- MST
Re: [PATCH] vhost-user-fs: add capability to allow migration
On 22/01/2023 16:46, Michael S. Tsirkin wrote: On Sun, Jan 22, 2023 at 02:36:04PM +0200, Anton Kuchin wrote: This flag should be set when qemu don't need to worry about any external state stored in vhost-user daemons during migration: don't fail migration, just pack generic virtio device states to migration stream and orchestrator guarantees that the rest of the state will be present at the destination to restore full context and continue running. Sorry I still do not get it. So fundamentally, why do we need this property? vhost-user-fs is not created by default that we'd then need opt-in to the special "migrateable" case. That's why I said it might make some sense as a device property as qemu tracks whether device is unplugged for us. But as written, if you are going to teach the orchestrator about vhost-user-fs and its special needs, just teach it when to migrate and where not to migrate. Either we describe the special situation to qemu and let qemu make an intelligent decision whether to allow migration, or we trust the orchestrator. And if it's the latter, then 'migrate' already says orchestrator decided to migrate. The problem I'm trying to solve is that most of vhost-user devices now block migration in qemu. And this makes sense since qemu can't extract and transfer backend daemon state. But this prevents us from updating qemu executable via local migration. So this flag is intended more as a safety check that says "I know what I'm doing". I agree that it is not really necessary if we trust the orchestrator to request migration only when the migration can be performed in a safe way. But changing the current behavior of vhost-user-fs from "always blocks migration" to "migrates partial state whenever orchestrator requests it" seems a little dangerous and can be misinterpreted as full support for migration in all cases. It's not really different from block is it? orchestrator has to arrange for backend migration. I think we just assumed there's no use-case where this is practical for vhost-user-fs so we blocked it. But in any case it's orchestrator's responsibility. Yes, you are right. So do you think we should just drop the blocker without adding a new flag?
Re: [PATCH v7 0/8] Mac Old World ROM experiment
On Sun, 22 Jan 2023, BALATON Zoltan wrote: On Mon, 29 Jun 2020, BALATON Zoltan wrote: This is now a minimal set of patches needed to make it possible to experiment with a firmware ROM from real hardware. After finding out that the board firmware does not probe PCI devices but expects them at known fixed addresses we only need to change the address of the macio device to get the firmware correctly map it. This allows dropping workarounds in previous versions for this and now only the minimal set of patches are included to get the firmware loaded and do something. (Also excluded the grackle revision and machine ID pathes for now that may be needed as the firmware accesses these but seems to go further without them so until we hit a problem we can live without it, although I wonder if this causes us unnecessary debugging later so unless they cause regressions they could be merged). I still don't get video output but at least it talks to the GPU chip now so it can be debugged and improved (this will either need emulating the correct chip the firmware has a driver for or an OF compliant ROM for the emulated card). As before the I2C part (patches 6-8) is RFC and unfinished but the first 5 patches should be good enough now. I hope someone can take care of I2C, I can look at the ati-vga side later. Regards, BALATON Zoltan BALATON Zoltan (8): mac_oldworld: Allow loading binary ROM image mac_newworld: Allow loading binary ROM image mac_oldworld: Drop a variable, use get_system_memory() directly mac_oldworld: Drop some variables mac_oldworld: Change PCI address of macio to match real hardware i2c: Match parameters of i2c_start_transfer and i2c_send_recv Continuing experimenting with getting g3beige work with the original firmware ROM here's the current status. The above patches were already merged. WIP macio/cuda: Attempt to add i2c support mac_oldworld: Add SPD data to cover RAM Adding these last two patches on top of Mark's screamer branch and increasing SCREAMER_BUFFER_SIZE define to 0x8000 in include/hw/audio/screamer.h to avoid a crash and using -memory 256 (as RAM size detection seems to be broken maybe due to imcomplete I2C emulation) I get the ROM to start but it cannot yet boot. We're past the initial OpenFirmware setup, can get a Forth prompt and explore the device tree and run Forth and also can start the Toolbox ROM from /AAPL,ROM but then it stops somewhere in there without giving any log or diag output. I think it may be waiting for some interrupt or missing some of the Heathrow emulation but I'm not really sure. I can get these traces: heathrow_write 0x28 1: 0x8000 heathrow_read 0x24 1: 0x8000 heathrow_read 0x2c 1: 0x0 heathrow_write 0x18 0: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_read 0x1c 0: 0x0 heathrow_write 0x28 1: 0x8000 heathrow_read 0x24 1: 0x8000 heathrow_read 0x2c 1: 0x0 heathrow_write 0x18 0: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_read 0x1c 0: 0x0 portA_write unimplemented portA_write unimplemented heathrow_read 0x24 1: 0x8000 heathrow_write 0x24 1: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_write 0x14 0: 0x0 heathrow_read 0x24 1: 0x8000 heathrow_write 0x24 1: 0x8004 heathrow_set_irq set_irq: num=0x12 level=1 heathrow_write 0x28 1: 0x8000 heathrow_read 0x24 1: 0x8004 heathrow_read 0x2c 1: 0x4 heathrow_write 0x18 0: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_read 0x1c 0: 0x0 heathrow_write 0x28 1: 0x8000 heathrow_read 0x24 1: 0x8004 heathrow_read 0x2c 1: 0x4 heathrow_write 0x18 0: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_read 0x1c 0: 0x0 heathrow_write 0x28 1: 0x8000 heathrow_read 0x24 1: 0x8004 heathrow_read 0x2c 1: 0x4 heathrow_write 0x18 0: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_read 0x1c 0: 0x0 Adding some cuda* traces and info via output in case that helps to tell what's happening: portA_write unimplemented cuda_delay_set_sr_int cuda_delay_set_sr_int cuda_packet_send length 5 cuda_packet_send_data [0] 0x00 cuda_packet_send_data [1] 0x40 cuda_packet_send_data [2] 0x2c cuda_packet_send_data [3] 0xa4 cuda_packet_send_data [4] 0xff cuda_delay_set_sr_int portA_write unimplemented heathrow_set_irq set_irq: num=0x12 level=1 (qemu) info via mos6522-cuda: Registers: ORB :0x30 ORA :0x10 DDRB:0x30 DDRA:0x58 T1CL:0x11 T1CH:0x14 T1LL:0xff T1LH:0xff T2CL:0x1b T2CH:0x88 SR :0xff ACR :0x1c PCR :0x0 IFR :0x60 IER :0x20 Timers: Using current time now(ns)=33052813591 T1 freq(hz)=78 mode=one-shot counter=0x1411 latch=0x load_time(ns)=0 next_irq_time(ns)=33131055374 T2 freq(hz)=1276 mode=one-shot counter=0x881b latch=0x30c load_time(ns)=257468378 next_irq_time(ns)=33349161167 then the last 6 lines are repeating endlessly. Does anybody have an idea what these registers are doing and where they are implemented in QEMU? Th
Re: [PATCH] mac_nvram: Add block backend to persist NVRAM contents
On Fri, 20 Jan 2023, Cédric Le Goater wrote:
On 1/19/23 23:28, BALATON Zoltan wrote:
Add a way to set a backing store for the mac_nvram similar to what
spapr_nvram or mac_via PRAM already does to allow to save its contents
between runs. Use -drive file=nvram.img,format=raw,if=mtd to specify
backing file where nvram.img must be MACIO_NVRAM_SIZE which is 8192
bytes. It is only wired for mac_oldworld for now but could be used by
mac_newworld in the future too.
Signed-off-by: BALATON Zoltan
---
hw/nvram/mac_nvram.c | 28
hw/ppc/mac_oldworld.c| 8 +++-
include/hw/nvram/mac_nvram.h | 1 +
3 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c
index 3d9ddda217..810e84f07e 100644
--- a/hw/nvram/mac_nvram.c
+++ b/hw/nvram/mac_nvram.c
@@ -24,9 +24,12 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "hw/nvram/chrp_nvram.h"
#include "hw/nvram/mac_nvram.h"
#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
+#include "sysemu/block-backend.h"
#include "migration/vmstate.h"
#include "qemu/cutils.h"
#include "qemu/module.h"
@@ -44,6 +47,9 @@ static void macio_nvram_writeb(void *opaque, hwaddr addr,
addr = (addr >> s->it_shift) & (s->size - 1);
trace_macio_nvram_write(addr, value);
s->data[addr] = value;
+if (s->blk) {
+blk_pwrite(s->blk, addr, 1, &s->data[addr], 0);
+}
}
static uint64_t macio_nvram_readb(void *opaque, hwaddr addr,
@@ -91,6 +97,27 @@ static void macio_nvram_realizefn(DeviceState *dev,
Error **errp)
s->data = g_malloc0(s->size);
+if (s->blk) {
+int64_t len = blk_getlength(s->blk);
+if (len < 0) {
+error_setg_errno(errp, -len,
+ "could not get length of nvram backing
image");
+return;
+} else if (len != s->size) {
+error_setg_errno(errp, -len,
+ "invalid size nvram backing image");
+return;
+}
+if (blk_set_perm(s->blk, BLK_PERM_CONSISTENT_READ |
BLK_PERM_WRITE,
+ BLK_PERM_ALL, errp) < 0) {
+return;
+}
+if (blk_pread(s->blk, 0, s->size, s->data, 0) < 0) {
+error_setg(errp, "can't read-nvram contents");
+return;
+}
This could use blk_check_size_and_read_all() instead ?
Good idea, except that comments in that function say its error handling is
not very good and tends to give unuseful messages to users so maybe until
that's sorted out I'd stay with open coding it here.
Regards,
BALATON Zoltan
Re: [PATCH v7 0/8] Mac Old World ROM experiment
On Mon, 29 Jun 2020, BALATON Zoltan wrote: This is now a minimal set of patches needed to make it possible to experiment with a firmware ROM from real hardware. After finding out that the board firmware does not probe PCI devices but expects them at known fixed addresses we only need to change the address of the macio device to get the firmware correctly map it. This allows dropping workarounds in previous versions for this and now only the minimal set of patches are included to get the firmware loaded and do something. (Also excluded the grackle revision and machine ID pathes for now that may be needed as the firmware accesses these but seems to go further without them so until we hit a problem we can live without it, although I wonder if this causes us unnecessary debugging later so unless they cause regressions they could be merged). I still don't get video output but at least it talks to the GPU chip now so it can be debugged and improved (this will either need emulating the correct chip the firmware has a driver for or an OF compliant ROM for the emulated card). As before the I2C part (patches 6-8) is RFC and unfinished but the first 5 patches should be good enough now. I hope someone can take care of I2C, I can look at the ati-vga side later. Regards, BALATON Zoltan BALATON Zoltan (8): mac_oldworld: Allow loading binary ROM image mac_newworld: Allow loading binary ROM image mac_oldworld: Drop a variable, use get_system_memory() directly mac_oldworld: Drop some variables mac_oldworld: Change PCI address of macio to match real hardware i2c: Match parameters of i2c_start_transfer and i2c_send_recv Continuing experimenting with getting g3beige work with the original firmware ROM here's the current status. The above patches were already merged. WIP macio/cuda: Attempt to add i2c support mac_oldworld: Add SPD data to cover RAM Adding these last two patches on top of Mark's screamer branch and increasing SCREAMER_BUFFER_SIZE define to 0x8000 in include/hw/audio/screamer.h to avoid a crash and using -memory 256 (as RAM size detection seems to be broken maybe due to imcomplete I2C emulation) I get the ROM to start but it cannot yet boot. We're past the initial OpenFirmware setup, can get a Forth prompt and explore the device tree and run Forth and also can start the Toolbox ROM from /AAPL,ROM but then it stops somewhere in there without giving any log or diag output. I think it may be waiting for some interrupt or missing some of the Heathrow emulation but I'm not really sure. I can get these traces: heathrow_write 0x28 1: 0x8000 heathrow_read 0x24 1: 0x8000 heathrow_read 0x2c 1: 0x0 heathrow_write 0x18 0: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_read 0x1c 0: 0x0 heathrow_write 0x28 1: 0x8000 heathrow_read 0x24 1: 0x8000 heathrow_read 0x2c 1: 0x0 heathrow_write 0x18 0: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_read 0x1c 0: 0x0 portA_write unimplemented portA_write unimplemented heathrow_read 0x24 1: 0x8000 heathrow_write 0x24 1: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_write 0x14 0: 0x0 heathrow_read 0x24 1: 0x8000 heathrow_write 0x24 1: 0x8004 heathrow_set_irq set_irq: num=0x12 level=1 heathrow_write 0x28 1: 0x8000 heathrow_read 0x24 1: 0x8004 heathrow_read 0x2c 1: 0x4 heathrow_write 0x18 0: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_read 0x1c 0: 0x0 heathrow_write 0x28 1: 0x8000 heathrow_read 0x24 1: 0x8004 heathrow_read 0x2c 1: 0x4 heathrow_write 0x18 0: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_read 0x1c 0: 0x0 heathrow_write 0x28 1: 0x8000 heathrow_read 0x24 1: 0x8004 heathrow_read 0x2c 1: 0x4 heathrow_write 0x18 0: 0x8000 heathrow_read 0x14 0: 0x0 heathrow_read 0x1c 0: 0x0 then the last 6 lines are repeating endlessly. Does anybody have an idea what these registers are doing and where they are implemented in QEMU? The model in hw/intc/heathrow_pic.c seems to be very simple but I'm not sure how are these connected to the rest of the mac_oldworld g3beige machine. I've checked that the IRQ numbers defined in include/hw/misc/macio/macio.h seems to match what's in the device tree generated by the ROM but there are some missing devices we don't emulate (such as SWIM floppy, PMAC ethernet and MESH SCSI). Yet the above looks like IRQ 0x12 is raised which should be CUDA and there were some portA write errors before that so maybe something with VIA or CUDA emulation? Any insight on this anyone? Regards, BALATON Zoltan hw/display/sm501.c | 2 +- hw/i2c/core.c| 34 +++--- hw/i2c/ppc4xx_i2c.c | 2 +- hw/misc/macio/cuda.c | 76 ++- hw/ppc/mac.h | 2 - hw/ppc/mac_newworld.c| 22 + hw/ppc/mac_oldworld.c| 86 +++- include/hw/i2c/i2c.h | 4 +- include/hw/misc/macio/cuda.h | 1 + 9 files changed, 167 insertions(+), 62 deletions(-)
Re: [PATCH v11] xen/pt: reserve PCI slot 2 for Intel igd-passthru
On 1/22/23 3:40 AM, Michael S. Tsirkin wrote:
> On Sat, Jan 21, 2023 at 07:57:02PM -0500, Chuck Zmudzinski wrote:
>> Intel specifies that the Intel IGD must occupy slot 2 on the PCI bus,
>> as noted in docs/igd-assign.txt in the Qemu source code.
>>
>> Currently, when the xl toolstack is used to configure a Xen HVM guest with
>> Intel IGD passthrough to the guest with the Qemu upstream device model,
>> a Qemu emulated PCI device will occupy slot 2 and the Intel IGD will occupy
>> a different slot. This problem often prevents the guest from booting.
>>
>> The only available workarounds are not good: Configure Xen HVM guests to
>> use the old and no longer maintained Qemu traditional device model
>> available from xenbits.xen.org which does reserve slot 2 for the Intel
>> IGD or use the "pc" machine type instead of the "xenfv" machine type and
>> add the xen platform device at slot 3 using a command line option
>> instead of patching qemu to fix the "xenfv" machine type directly. The
>> second workaround causes some degredation in startup performance such as
>> a longer boot time and reduced resolution of the grub menu that is
>> displayed on the monitor. This patch avoids that reduced startup
>> performance when using the Qemu upstream device model for Xen HVM guests
>> configured with the igd-passthru=on option.
>>
>> To implement this feature in the Qemu upstream device model for Xen HVM
>> guests, introduce the following new functions, types, and macros:
>>
>> * XEN_PT_DEVICE_CLASS declaration, based on the existing TYPE_XEN_PT_DEVICE
>> * XEN_PT_DEVICE_GET_CLASS macro helper function for XEN_PT_DEVICE_CLASS
>> * typedef XenPTQdevRealize function pointer
>> * XEN_PCI_IGD_SLOT_MASK, the value of slot_reserved_mask to reserve slot 2
>> * xen_igd_reserve_slot and xen_igd_clear_slot functions
>>
>> Michael Tsirkin:
>> * Introduce XEN_PCI_IGD_DOMAIN, XEN_PCI_IGD_BUS, XEN_PCI_IGD_DEV, and
>> XEN_PCI_IGD_FN - use them to compute the value of XEN_PCI_IGD_SLOT_MASK
>>
>> The new xen_igd_reserve_slot function uses the existing slot_reserved_mask
>> member of PCIBus to reserve PCI slot 2 for Xen HVM guests configured using
>> the xl toolstack with the gfx_passthru option enabled, which sets the
>> igd-passthru=on option to Qemu for the Xen HVM machine type.
>>
>> The new xen_igd_reserve_slot function also needs to be implemented in
>> hw/xen/xen_pt_stub.c to prevent FTBFS during the link stage for the case
>> when Qemu is configured with --enable-xen and --disable-xen-pci-passthrough,
>> in which case it does nothing.
>>
>> The new xen_igd_clear_slot function overrides qdev->realize of the parent
>> PCI device class to enable the Intel IGD to occupy slot 2 on the PCI bus
>> since slot 2 was reserved by xen_igd_reserve_slot when the PCI bus was
>> created in hw/i386/pc_piix.c for the case when igd-passthru=on.
>>
>> Move the call to xen_host_pci_device_get, and the associated error
>> handling, from xen_pt_realize to the new xen_igd_clear_slot function to
>> initialize the device class and vendor values which enables the checks for
>> the Intel IGD to succeed. The verification that the host device is an
>> Intel IGD to be passed through is done by checking the domain, bus, slot,
>> and function values as well as by checking that gfx_passthru is enabled,
>> the device class is VGA, and the device vendor in Intel.
>>
>> Signed-off-by: Chuck Zmudzinski
>> ---
>> Notes that might be helpful to reviewers of patched code in hw/xen:
>>
>> The new functions and types are based on recommendations from Qemu docs:
>> https://qemu.readthedocs.io/en/latest/devel/qom.html
>>
>> Notes that might be helpful to reviewers of patched code in hw/i386:
>>
>> The small patch to hw/i386/pc_piix.c is protected by CONFIG_XEN so it does
>> not affect builds that do not have CONFIG_XEN defined.
>>
>> xen_igd_gfx_pt_enabled() in the patched hw/i386/pc_piix.c file is an
>> existing function that is only true when Qemu is built with
>> xen-pci-passthrough enabled and the administrator has configured the Xen
>> HVM guest with Qemu's igd-passthru=on option.
>>
>> v2: Remove From: tag at top of commit message
>>
>> v3: Changed the test for the Intel IGD in xen_igd_clear_slot:
>>
>> if (is_igd_vga_passthrough(&s->real_device) &&
>> (s->real_device.vendor_id == PCI_VENDOR_ID_INTEL)) {
>>
>> is changed to
>>
>> if (xen_igd_gfx_pt_enabled() && (s->hostaddr.slot == 2)
>> && (s->hostaddr.function == 0)) {
>>
>> I hoped that I could use the test in v2, since it matches the
>> other tests for the Intel IGD in Qemu and Xen, but those tests
>> do not work because the necessary data structures are not set with
>> their values yet. So instead use the test that the administrator
>> has enabled gfx_passthru and the device address on the host is
>> 02.0. This test does detect the Intel IGD correctly.
>>
>> v4: Use brchu...@aol.com instead of brchu...@netscape.net for the author's
>>
[PATCH] qga/linux: add usb support to guest-get-fsinfo
---
qga/commands-posix.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index ebd33a643c..aab9d3bd50 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -880,7 +880,9 @@ static bool build_guest_fsinfo_for_pci_dev(char const
*syspath,
g_str_equal(driver, "sym53c8xx") ||
g_str_equal(driver, "virtio-pci") ||
g_str_equal(driver, "ahci") ||
- g_str_equal(driver, "nvme"))) {
+ g_str_equal(driver, "nvme") ||
+ g_str_equal(driver, "xhci_hcd") ||
+ g_str_equal(driver, "ehci-pci"))) {
break;
}
@@ -977,6 +979,8 @@ static bool build_guest_fsinfo_for_pci_dev(char const
*syspath,
}
} else if (strcmp(driver, "nvme") == 0) {
disk->bus_type = GUEST_DISK_BUS_TYPE_NVME;
+} else if (strcmp(driver, "ehci-pci") == 0 || strcmp(driver, "xhci_hcd")
== 0) {
+disk->bus_type = GUEST_DISK_BUS_TYPE_USB;
} else {
g_debug("unknown driver '%s' (sysfs path '%s')", driver, syspath);
goto cleanup;
--
2.38.1
Re: [PATCH] vhost-user-fs: add capability to allow migration
On Sun, Jan 22, 2023 at 02:36:04PM +0200, Anton Kuchin wrote: > > > This flag should be set when qemu don't need to worry about any > > > external state stored in vhost-user daemons during migration: > > > don't fail migration, just pack generic virtio device states to > > > migration stream and orchestrator guarantees that the rest of the > > > state will be present at the destination to restore full context and > > > continue running. > > Sorry I still do not get it. So fundamentally, why do we need this > > property? > > vhost-user-fs is not created by default that we'd then need opt-in to > > the special "migrateable" case. > > That's why I said it might make some sense as a device property as qemu > > tracks whether device is unplugged for us. > > > > But as written, if you are going to teach the orchestrator about > > vhost-user-fs and its special needs, just teach it when to migrate and > > where not to migrate. > > > > Either we describe the special situation to qemu and let qemu > > make an intelligent decision whether to allow migration, > > or we trust the orchestrator. And if it's the latter, then 'migrate' > > already says orchestrator decided to migrate. > > The problem I'm trying to solve is that most of vhost-user devices > now block migration in qemu. And this makes sense since qemu can't > extract and transfer backend daemon state. But this prevents us from > updating qemu executable via local migration. So this flag is > intended more as a safety check that says "I know what I'm doing". > > I agree that it is not really necessary if we trust the orchestrator > to request migration only when the migration can be performed in a > safe way. But changing the current behavior of vhost-user-fs from > "always blocks migration" to "migrates partial state whenever > orchestrator requests it" seems a little dangerous and can be > misinterpreted as full support for migration in all cases. It's not really different from block is it? orchestrator has to arrange for backend migration. I think we just assumed there's no use-case where this is practical for vhost-user-fs so we blocked it. But in any case it's orchestrator's responsibility. -- MST
Re: [PATCH] vhost-user-fs: add capability to allow migration
On 22/01/2023 10:16, Michael S. Tsirkin wrote:
On Fri, Jan 20, 2023 at 07:37:18PM +0200, Anton Kuchin wrote:
On 20/01/2023 15:58, Michael S. Tsirkin wrote:
On Thu, Jan 19, 2023 at 03:45:06PM +0200, Anton Kuchin wrote:
On 19/01/2023 14:51, Michael S. Tsirkin wrote:
On Sun, Jan 15, 2023 at 07:09:03PM +0200, Anton Kuchin wrote:
Now any vhost-user-fs device makes VM unmigratable, that also prevents
qemu update without stopping the VM. In most cases that makes sense
because qemu has no way to transfer FUSE session state.
But we can give an option to orchestrator to override this if it can
guarantee that state will be preserved (e.g. it uses migration to
update qemu and dst will run on the same host as src and use the same
socket endpoints).
This patch keeps default behavior that prevents migration with such devices
but adds migration capability 'vhost-user-fs' to explicitly allow migration.
Signed-off-by: Anton Kuchin
---
hw/virtio/vhost-user-fs.c | 25 -
qapi/migration.json | 7 ++-
2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index f5049735ac..13d920423e 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -24,6 +24,7 @@
#include "hw/virtio/vhost-user-fs.h"
#include "monitor/monitor.h"
#include "sysemu/sysemu.h"
+#include "migration/migration.h"
static const int user_feature_bits[] = {
VIRTIO_F_VERSION_1,
@@ -298,9 +299,31 @@ static struct vhost_dev *vuf_get_vhost(VirtIODevice *vdev)
return &fs->vhost_dev;
}
+static int vhost_user_fs_pre_save(void *opaque)
+{
+MigrationState *s = migrate_get_current();
+
+if (!s->enabled_capabilities[MIGRATION_CAPABILITY_VHOST_USER_FS]) {
+error_report("Migration of vhost-user-fs devices requires internal FUSE
"
+ "state of backend to be preserved. If orchestrator can "
+ "guarantee this (e.g. dst connects to the same backend "
+ "instance or backend state is migrated) set 'vhost-user-fs'
"
+ "migration capability to true to enable migration.");
Isn't it possible that some backends are same and some are not?
Shouldn't this be a device property then?
If some are not the same it is not guaranteed that correct FUSE
state is present there, so orchestrator shouldn't set the capability
because this can result in destination devices being broken (they'll
be fine after the remount in guest, but this is guest visible and is
not acceptable).
I can imagine smart orchestrator and backend that can transfer
internal FUSE state, but we are not there yet, and this would be
their responsibility then to ensure endpoint compatibility between src
and dst and set the capability (that's why I put "e.g." and "or" in
the error description).
So instead of relying on the orchestrator how about making it a device
property?
We have to rely on the orchestrator here and I can't see how a property
can help us with this: same device can be migratable or not depending
on if the destination is the same host, what features backend supports,
how management software works and other factors of environment that
are not accessible from qemu or backend daemon.
So in the end we'll end up with orchestrator having to setup flags for
each device before each migration based on information only it can
have - in my opinion this is worse than just giving the orchestrator
a single flag that it can set after calculating the decision for
the particular migration that it organizes.
+return -1;
+}
+
+return 0;
+}
+
static const VMStateDescription vuf_vmstate = {
.name = "vhost-user-fs",
-.unmigratable = 1,
+.minimum_version_id = 0,
+.version_id = 0,
+.fields = (VMStateField[]) {
+VMSTATE_VIRTIO_DEVICE,
+VMSTATE_END_OF_LIST()
+},
+ .pre_save = vhost_user_fs_pre_save,
};
static Property vuf_properties[] = {
diff --git a/qapi/migration.json b/qapi/migration.json
index 88ecf86ac8..9a229ea884 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -477,6 +477,11 @@
#will be handled faster. This is a performance feature
and
#should not affect the correctness of postcopy
migration.
#(since 7.1)
+# @vhost-user-fs: If enabled, the migration process will allow migration of
+# vhost-user-fs devices, this should be enabled only when
+# backend can preserve local FUSE state e.g. for qemu update
+# when dst reconects to the same endpoints after migration.
+# (since 8.0)
#
# Features:
# @unstable: Members @x-colo and @x-ignore-shared are experimental.
@@ -492,7 +497,7 @@
'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
{ 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
Re: [PATCH v8 09/13] vfio/migration: Implement VFIO migration protocol v2
On 21/01/2023 1:07, Alex Williamson wrote:
External email: Use caution opening links or attachments
On Mon, 16 Jan 2023 16:11:31 +0200
Avihai Horon wrote:
Implement the basic mandatory part of VFIO migration protocol v2.
This includes all functionality that is necessary to support
VFIO_MIGRATION_STOP_COPY part of the v2 protocol.
The two protocols, v1 and v2, will co-exist and in the following patches
v1 protocol code will be removed.
There are several main differences between v1 and v2 protocols:
- VFIO device state is now represented as a finite state machine instead
of a bitmap.
- Migration interface with kernel is now done using VFIO_DEVICE_FEATURE
ioctl and normal read() and write() instead of the migration region.
- Pre-copy is made optional in v2 protocol. Support for pre-copy will be
added later on.
Detailed information about VFIO migration protocol v2 and its difference
compared to v1 protocol can be found here [1].
[1]
https://lore.kernel.org/all/20220224142024.147653-10-yish...@nvidia.com/
Signed-off-by: Avihai Horon
Reviewed-by: Cédric Le Goater
---
include/hw/vfio/vfio-common.h | 5 +
hw/vfio/common.c | 19 +-
hw/vfio/migration.c | 455 +++---
hw/vfio/trace-events | 7 +
4 files changed, 447 insertions(+), 39 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index bbaf72ba00..6d7d850bfe 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -66,6 +66,11 @@ typedef struct VFIOMigration {
int vm_running;
Notifier migration_state;
uint64_t pending_bytes;
+uint32_t device_state;
+int data_fd;
+void *data_buffer;
+size_t data_buffer_size;
+bool v2;
} VFIOMigration;
typedef struct VFIOAddressSpace {
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 550b2d7ded..dcaa77d2a8 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -355,10 +355,18 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainer
*container)
return false;
}
-if ((vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) &&
+if (!migration->v2 &&
+(vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) &&
(migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING)) {
return false;
}
+
+if (migration->v2 &&
+(vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) &&
+(migration->device_state == VFIO_DEVICE_STATE_RUNNING ||
+ migration->device_state == VFIO_DEVICE_STATE_RUNNING_P2P)) {
+return false;
+}
}
}
return true;
@@ -385,7 +393,14 @@ static bool
vfio_devices_all_running_and_mig_active(VFIOContainer *container)
return false;
}
-if (migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) {
+if (!migration->v2 &&
+migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) {
+continue;
+}
+
+if (migration->v2 &&
+(migration->device_state == VFIO_DEVICE_STATE_RUNNING ||
+ migration->device_state == VFIO_DEVICE_STATE_RUNNING_P2P)) {
continue;
} else {
return false;
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 9df859f4d3..f19ada0f4f 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -10,6 +10,7 @@
#include "qemu/osdep.h"
#include "qemu/main-loop.h"
#include "qemu/cutils.h"
+#include "qemu/units.h"
#include
#include
@@ -44,8 +45,103 @@
#define VFIO_MIG_FLAG_DEV_SETUP_STATE (0xef13ULL)
#define VFIO_MIG_FLAG_DEV_DATA_STATE(0xef14ULL)
+/*
+ * This is an arbitrary size based on migration of mlx5 devices, where
typically
+ * total device migration size is on the order of 100s of MB. Testing with
+ * larger values, e.g. 128MB and 1GB, did not show a performance improvement.
+ */
+#define VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE (1 * MiB)
+
static int64_t bytes_transferred;
+static const char *mig_state_to_str(enum vfio_device_mig_state state)
+{
+switch (state) {
+case VFIO_DEVICE_STATE_ERROR:
+return "ERROR";
+case VFIO_DEVICE_STATE_STOP:
+return "STOP";
+case VFIO_DEVICE_STATE_RUNNING:
+return "RUNNING";
+case VFIO_DEVICE_STATE_STOP_COPY:
+return "STOP_COPY";
+case VFIO_DEVICE_STATE_RESUMING:
+return "RESUMING";
+case VFIO_DEVICE_STATE_RUNNING_P2P:
+return "RUNNING_P2P";
+default:
+return "UNKNOWN STATE";
+}
+}
+
+static int vfio_migration_set_state(VFIODevice *vbasedev,
+enum vfio_device_mig_state new_state,
+enum vfio_device_mig_state recover_state)
+{
+VFI
Re: [PATCH v8 04/13] vfio/migration: Allow migration without VFIO IOMMU dirty tracking support
On 21/01/2023 1:06, Alex Williamson wrote:
External email: Use caution opening links or attachments
On Mon, 16 Jan 2023 16:11:26 +0200
Avihai Horon wrote:
Currently, if IOMMU of a VFIO container doesn't support dirty page
tracking, migration is blocked. This is because a DMA-able VFIO device
can dirty RAM pages without updating QEMU about it, thus breaking the
migration.
However, this doesn't mean that migration can't be done at all.
In such case, allow migration and let QEMU VFIO code mark all pages
dirty.
This guarantees that all pages that might have gotten dirty are reported
back, and thus guarantees a valid migration even without VFIO IOMMU
dirty tracking support.
The motivation for this patch is the introduction of iommufd [1].
iommufd can directly implement the /dev/vfio/vfio container IOCTLs by
mapping them into its internal ops, allowing the usage of these IOCTLs
over iommufd. However, VFIO IOMMU dirty tracking is not supported by
this VFIO compatibility API.
This patch will allow migration by hosts that use the VFIO compatibility
API and prevent migration regressions caused by the lack of VFIO IOMMU
dirty tracking support.
[1]
https://lore.kernel.org/kvm/0-v6-a196d26f289e+11787-iommufd_...@nvidia.com/
Signed-off-by: Avihai Horon
---
hw/vfio/common.c| 20 ++--
hw/vfio/migration.c | 3 +--
2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 130e5d1dc7..f6dd571549 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -488,6 +488,12 @@ static int vfio_dma_unmap(VFIOContainer *container,
return -errno;
}
+if (iotlb && vfio_devices_all_running_and_saving(container)) {
+cpu_physical_memory_set_dirty_range(iotlb->translated_addr, size,
+tcg_enabled() ? DIRTY_CLIENTS_ALL :
+DIRTY_CLIENTS_NOCODE);
I take it this is an attempt to decipher the mask arg based on its use
in cpu_physical_memory_set_dirty_lebitmap().
Right.
I'm attempting to do the
same. It seems like it must logically be the case that
global_dirty_tracking is set to pass the running-and-saving test, but I
can't connect the pieces. Is this your understanding as well and the
reason we don't also need to optionally exclude DIRTY_MEMORY_MIGRATION?
Yes, this is how I understood it.
Running-and-saving test is passed only if migration has started, and if
migration has started, global_dirty_tracking is set.
So global_dirty_tracking should be set and we don't need to check for
DIRTY_MEMORY_MIGRATION exclusion.
Thanks,
Alex
+}
+
return 0;
}
@@ -1201,6 +1207,10 @@ static void vfio_set_dirty_page_tracking(VFIOContainer
*container, bool start)
.argsz = sizeof(dirty),
};
+if (!container->dirty_pages_supported) {
+return;
+}
+
if (start) {
dirty.flags = VFIO_IOMMU_DIRTY_PAGES_FLAG_START;
} else {
@@ -1236,6 +1246,13 @@ static int vfio_get_dirty_bitmap(VFIOContainer
*container, uint64_t iova,
uint64_t pages;
int ret;
+if (!container->dirty_pages_supported) {
+cpu_physical_memory_set_dirty_range(ram_addr, size,
+tcg_enabled() ? DIRTY_CLIENTS_ALL :
+DIRTY_CLIENTS_NOCODE);
+return 0;
+}
+
dbitmap = g_malloc0(sizeof(*dbitmap) + sizeof(*range));
dbitmap->argsz = sizeof(*dbitmap) + sizeof(*range);
@@ -1409,8 +1426,7 @@ static void vfio_listener_log_sync(MemoryListener
*listener,
{
VFIOContainer *container = container_of(listener, VFIOContainer,
listener);
-if (vfio_listener_skipped_section(section) ||
-!container->dirty_pages_supported) {
+if (vfio_listener_skipped_section(section)) {
return;
}
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 09fe7c1de2..552c2313b2 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -860,11 +860,10 @@ int64_t vfio_mig_bytes_transferred(void)
int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
{
-VFIOContainer *container = vbasedev->group->container;
struct vfio_region_info *info = NULL;
int ret = -ENOTSUP;
-if (!vbasedev->enable_migration || !container->dirty_pages_supported) {
+if (!vbasedev->enable_migration) {
goto add_blocker;
}
Re: [PATCH v11] xen/pt: reserve PCI slot 2 for Intel igd-passthru
On Sat, Jan 21, 2023 at 07:57:02PM -0500, Chuck Zmudzinski wrote:
> Intel specifies that the Intel IGD must occupy slot 2 on the PCI bus,
> as noted in docs/igd-assign.txt in the Qemu source code.
>
> Currently, when the xl toolstack is used to configure a Xen HVM guest with
> Intel IGD passthrough to the guest with the Qemu upstream device model,
> a Qemu emulated PCI device will occupy slot 2 and the Intel IGD will occupy
> a different slot. This problem often prevents the guest from booting.
>
> The only available workarounds are not good: Configure Xen HVM guests to
> use the old and no longer maintained Qemu traditional device model
> available from xenbits.xen.org which does reserve slot 2 for the Intel
> IGD or use the "pc" machine type instead of the "xenfv" machine type and
> add the xen platform device at slot 3 using a command line option
> instead of patching qemu to fix the "xenfv" machine type directly. The
> second workaround causes some degredation in startup performance such as
> a longer boot time and reduced resolution of the grub menu that is
> displayed on the monitor. This patch avoids that reduced startup
> performance when using the Qemu upstream device model for Xen HVM guests
> configured with the igd-passthru=on option.
>
> To implement this feature in the Qemu upstream device model for Xen HVM
> guests, introduce the following new functions, types, and macros:
>
> * XEN_PT_DEVICE_CLASS declaration, based on the existing TYPE_XEN_PT_DEVICE
> * XEN_PT_DEVICE_GET_CLASS macro helper function for XEN_PT_DEVICE_CLASS
> * typedef XenPTQdevRealize function pointer
> * XEN_PCI_IGD_SLOT_MASK, the value of slot_reserved_mask to reserve slot 2
> * xen_igd_reserve_slot and xen_igd_clear_slot functions
>
> Michael Tsirkin:
> * Introduce XEN_PCI_IGD_DOMAIN, XEN_PCI_IGD_BUS, XEN_PCI_IGD_DEV, and
> XEN_PCI_IGD_FN - use them to compute the value of XEN_PCI_IGD_SLOT_MASK
>
> The new xen_igd_reserve_slot function uses the existing slot_reserved_mask
> member of PCIBus to reserve PCI slot 2 for Xen HVM guests configured using
> the xl toolstack with the gfx_passthru option enabled, which sets the
> igd-passthru=on option to Qemu for the Xen HVM machine type.
>
> The new xen_igd_reserve_slot function also needs to be implemented in
> hw/xen/xen_pt_stub.c to prevent FTBFS during the link stage for the case
> when Qemu is configured with --enable-xen and --disable-xen-pci-passthrough,
> in which case it does nothing.
>
> The new xen_igd_clear_slot function overrides qdev->realize of the parent
> PCI device class to enable the Intel IGD to occupy slot 2 on the PCI bus
> since slot 2 was reserved by xen_igd_reserve_slot when the PCI bus was
> created in hw/i386/pc_piix.c for the case when igd-passthru=on.
>
> Move the call to xen_host_pci_device_get, and the associated error
> handling, from xen_pt_realize to the new xen_igd_clear_slot function to
> initialize the device class and vendor values which enables the checks for
> the Intel IGD to succeed. The verification that the host device is an
> Intel IGD to be passed through is done by checking the domain, bus, slot,
> and function values as well as by checking that gfx_passthru is enabled,
> the device class is VGA, and the device vendor in Intel.
>
> Signed-off-by: Chuck Zmudzinski
> ---
> Notes that might be helpful to reviewers of patched code in hw/xen:
>
> The new functions and types are based on recommendations from Qemu docs:
> https://qemu.readthedocs.io/en/latest/devel/qom.html
>
> Notes that might be helpful to reviewers of patched code in hw/i386:
>
> The small patch to hw/i386/pc_piix.c is protected by CONFIG_XEN so it does
> not affect builds that do not have CONFIG_XEN defined.
>
> xen_igd_gfx_pt_enabled() in the patched hw/i386/pc_piix.c file is an
> existing function that is only true when Qemu is built with
> xen-pci-passthrough enabled and the administrator has configured the Xen
> HVM guest with Qemu's igd-passthru=on option.
>
> v2: Remove From: tag at top of commit message
>
> v3: Changed the test for the Intel IGD in xen_igd_clear_slot:
>
> if (is_igd_vga_passthrough(&s->real_device) &&
> (s->real_device.vendor_id == PCI_VENDOR_ID_INTEL)) {
>
> is changed to
>
> if (xen_igd_gfx_pt_enabled() && (s->hostaddr.slot == 2)
> && (s->hostaddr.function == 0)) {
>
> I hoped that I could use the test in v2, since it matches the
> other tests for the Intel IGD in Qemu and Xen, but those tests
> do not work because the necessary data structures are not set with
> their values yet. So instead use the test that the administrator
> has enabled gfx_passthru and the device address on the host is
> 02.0. This test does detect the Intel IGD correctly.
>
> v4: Use brchu...@aol.com instead of brchu...@netscape.net for the author's
> email address to match the address used by the same author in commits
> be9c61da and c0e86b76
>
> Change variable for XEN_PT_DEVI
Re: [PATCH v2 01/10] target/loongarch: Enable the disassembler for host tcg
On 1/18/23 09:11, Richard Henderson wrote:
Reuse the decodetree based disassembler from
target/loongarch/ for tcg/loongarch64/.
The generation of decode-insns.c.inc into ./libcommon.fa.p/ could
eventually result in conflict, if any other host requires the same
trick, but this is good enough for now.
Signed-off-by: Richard Henderson
---
disas.c | 2 ++
target/loongarch/meson.build | 3 ++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/disas.c b/disas.c
index 3b31315f40..c9fa38e6d7 100644
--- a/disas.c
+++ b/disas.c
@@ -198,6 +198,8 @@ static void initialize_debug_host(CPUDebug *s)
s->info.cap_insn_split = 6;
#elif defined(__hppa__)
s->info.print_insn = print_insn_hppa;
+#elif defined(__loongarch64)
This could just be `__loongarch__` because both LA32 and LA64 share the
same encoding, so although LA32 userland isn't quite there yet it
wouldn't do any harm.
+s->info.print_insn = print_insn_loongarch;
#endif
}
diff --git a/target/loongarch/meson.build b/target/loongarch/meson.build
index 6376f9e84b..690633969f 100644
--- a/target/loongarch/meson.build
+++ b/target/loongarch/meson.build
@@ -3,7 +3,6 @@ gen = decodetree.process('insns.decode')
loongarch_ss = ss.source_set()
loongarch_ss.add(files(
'cpu.c',
- 'disas.c',
))
loongarch_tcg_ss = ss.source_set()
loongarch_tcg_ss.add(gen)
@@ -24,6 +23,8 @@ loongarch_softmmu_ss.add(files(
'iocsr_helper.c',
))
+common_ss.add(when: 'CONFIG_LOONGARCH_DIS', if_true: [files('disas.c'), gen])
+
loongarch_ss.add_all(when: 'CONFIG_TCG', if_true: [loongarch_tcg_ss])
target_arch += {'loongarch': loongarch_ss}
Apart from the minor suggestion above,
Reviewed-by: WANG Xuerui
Thanks!
Re: [PATCH v2 00/10] tcg/loongarch64: Reorg goto_tb and cleanups
Hi,
On 1/18/23 09:11, Richard Henderson wrote:
Based-on: 20230117231051.35-1-richard.hender...@linaro.org
("[PULL 00/22] tcg patch queue")
Includes:
* Disassembler from target/loongarch/.
* Improvements to movi by Rui Wang, with minor tweaks.
* Improvements to setcond.
* Implement movcond.
* Fix the same goto_tb bug that affected some others.
r~
Richard Henderson (9):
target/loongarch: Enable the disassembler for host tcg
target/loongarch: Disassemble jirl properly
target/loongarch: Disassemble pcadd* addresses
tcg/loongarch64: Update tcg-insn-defs.c.inc
tcg/loongarch64: Introduce tcg_out_addi
tcg/loongarch64: Improve setcond expansion
tcg/loongarch64: Implement movcond
tcg/loongarch64: Use tcg_pcrel_diff in tcg_out_ldst
tcg/loongarch64: Reorg goto_tb implementation
Rui Wang (1):
tcg/loongarch64: Optimize immediate loading
tcg/loongarch64/tcg-target-con-set.h | 5 +-
tcg/loongarch64/tcg-target-con-str.h | 2 +-
tcg/loongarch64/tcg-target.h | 11 +-
disas.c | 2 +
target/loongarch/disas.c | 39 +-
.../loongarch/insn_trans/trans_branch.c.inc | 2 +-
target/loongarch/insns.decode | 3 +-
target/loongarch/meson.build | 3 +-
tcg/loongarch64/tcg-insn-defs.c.inc | 10 +-
tcg/loongarch64/tcg-target.c.inc | 364 --
10 files changed, 300 insertions(+), 141 deletions(-)
mode change 100644 => 100755 tcg/loongarch64/tcg-insn-defs.c.inc
Sorry for the late review; I was focusing more on LLVM and day job these
days. I've reviewed some of these and will take a look at the rest (and
test all of them on native HW) tonight. Thanks very much for all the
refactoring!
Re: [PATCH v2 05/10] tcg/loongarch64: Update tcg-insn-defs.c.inc
On 1/18/23 09:11, Richard Henderson wrote:
Regenerate with ADDU16I included:
$ cd loongarch-opcodes/scripts/go
$ go run ./genqemutcgdefs > $QEMU/tcg/loongarch64/tcg-insn-defs.c.inc
Signed-off-by: Richard Henderson
---
tcg/loongarch64/tcg-insn-defs.c.inc | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
mode change 100644 => 100755 tcg/loongarch64/tcg-insn-defs.c.inc
diff --git a/tcg/loongarch64/tcg-insn-defs.c.inc
b/tcg/loongarch64/tcg-insn-defs.c.inc
old mode 100644
new mode 100755
index d162571856..b5bb0c5e73
--- a/tcg/loongarch64/tcg-insn-defs.c.inc
+++ b/tcg/loongarch64/tcg-insn-defs.c.inc
@@ -4,7 +4,7 @@
*
* This file is auto-generated by genqemutcgdefs from
* https://github.com/loongson-community/loongarch-opcodes,
- * from commit 961f0c60f5b63e574d785995600c71ad5413fdc4.
+ * from commit 25ca7effe9d88101c1cf96c4005423643386d81f.
* DO NOT EDIT.
*/
@@ -74,6 +74,7 @@ typedef enum {
OPC_ANDI = 0x0340,
OPC_ORI = 0x0380,
OPC_XORI = 0x03c0,
+OPC_ADDU16I_D = 0x1000,
OPC_LU12I_W = 0x1400,
OPC_CU32I_D = 0x1600,
OPC_PCADDU2I = 0x1800,
@@ -710,6 +711,13 @@ tcg_out_opc_xori(TCGContext *s, TCGReg d, TCGReg j,
uint32_t uk12)
tcg_out32(s, encode_djuk12_insn(OPC_XORI, d, j, uk12));
}
+/* Emits the `addu16i.d d, j, sk16` instruction. */
+static void __attribute__((unused))
+tcg_out_opc_addu16i_d(TCGContext *s, TCGReg d, TCGReg j, int32_t sk16)
+{
+tcg_out32(s, encode_djsk16_insn(OPC_ADDU16I_D, d, j, sk16));
+}
+
/* Emits the `lu12i.w d, sj20` instruction. */
static void __attribute__((unused))
tcg_out_opc_lu12i_w(TCGContext *s, TCGReg d, int32_t sj20)
Reviewed-by: WANG Xuerui
Re: [PATCH v2 08/10] tcg/loongarch64: Implement movcond
On 1/18/23 09:11, Richard Henderson wrote: Signed-off-by: Richard Henderson --- tcg/loongarch64/tcg-target-con-set.h | 1 + tcg/loongarch64/tcg-target.h | 4 ++-- tcg/loongarch64/tcg-target.c.inc | 33 3 files changed, 36 insertions(+), 2 deletions(-) Reviewed-by: WANG Xuerui
Re: [PATCH v2 04/10] tcg/loongarch64: Optimize immediate loading
On 1/18/23 09:11, Richard Henderson wrote: From: Rui Wang diff: Imm Before After addi.w rd, zero, 0 addi.w rd, zero, 0 lu52i.d rd, zero, 0 f800lu12i.w rd, -1 addi.w rd, zero, -2048 ori rd, rd, 2048lu32i.d rd, 0 lu32i.d rd, 0 ... Signed-off-by: Rui Wang Message-Id: <20221107144713.845550-1-wang...@loongson.cn> Signed-off-by: Richard Henderson --- tcg/loongarch64/tcg-target.c.inc | 35 +++- 1 file changed, 12 insertions(+), 23 deletions(-) Reviewed-by: WANG Xuerui Thanks!
Re: [PATCH v2 02/10] target/loongarch: Disassemble jirl properly
On 1/18/23 09:11, Richard Henderson wrote: While jirl shares the same instruction format as bne etc, it is not assembled the same. In particular, rd is printed first not second and the immediate is not pc-relative. Decode into the arg_rr_i structure, which prints correctly. This changes the "offs" member to "imm", to update translate. Signed-off-by: Richard Henderson --- target/loongarch/disas.c | 2 +- target/loongarch/insn_trans/trans_branch.c.inc | 2 +- target/loongarch/insns.decode | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/target/loongarch/disas.c b/target/loongarch/disas.c index 858dfcc53a..7cffd853ec 100644 --- a/target/loongarch/disas.c +++ b/target/loongarch/disas.c @@ -628,7 +628,7 @@ INSN(beqz, r_offs) INSN(bnez, r_offs) INSN(bceqz,c_offs) INSN(bcnez,c_offs) -INSN(jirl, rr_offs) +INSN(jirl, rr_i) INSN(b,offs) INSN(bl, offs) INSN(beq, rr_offs) diff --git a/target/loongarch/insn_trans/trans_branch.c.inc b/target/loongarch/insn_trans/trans_branch.c.inc index 65dbdff41e..a860f7e733 100644 --- a/target/loongarch/insn_trans/trans_branch.c.inc +++ b/target/loongarch/insn_trans/trans_branch.c.inc @@ -23,7 +23,7 @@ static bool trans_jirl(DisasContext *ctx, arg_jirl *a) TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE); TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE); -tcg_gen_addi_tl(cpu_pc, src1, a->offs); +tcg_gen_addi_tl(cpu_pc, src1, a->imm); tcg_gen_movi_tl(dest, ctx->base.pc_next + 4); gen_set_gpr(a->rd, dest, EXT_NONE); tcg_gen_lookup_and_goto_ptr(); diff --git a/target/loongarch/insns.decode b/target/loongarch/insns.decode index 3fdc6e148c..de7b8f0f3c 100644 --- a/target/loongarch/insns.decode +++ b/target/loongarch/insns.decode @@ -67,6 +67,7 @@ @rr_ui12 .. imm:12 rj:5 rd:5&rr_i @rr_i14s2 .. rj:5 rd:5&rr_i imm=%i14s2 @rr_i16 .. imm:s16 rj:5 rd:5&rr_i +@rr_i16s2 .. rj:5 rd:5&rr_i imm=%offs16 @hint_r_i12 .. imm:s12 rj:5 hint:5&hint_r_i @rrr_sa2p1 ... .. rk:5 rj:5 rd:5&rrr_sa sa=%sa2p1 @rrr_sa2 ... sa:2 rk:5 rj:5 rd:5&rrr_sa @@ -444,7 +445,7 @@ beqz0100 00 . . @r_offs21 bnez0100 01 . . @r_offs21 bceqz 0100 10 00 ... .@c_offs21 bcnez 0100 10 01 ... .@c_offs21 -jirl0100 11 . . @rr_offs16 +jirl0100 11 . . @rr_i16s2 b 0101 00 .. @offs26 bl 0101 01 .. @offs26 beq 0101 10 . . @rr_offs16 Reviewed-by: WANG Xuerui Thanks for the catch!
Re: [PATCH v2 09/10] tcg/loongarch64: Use tcg_pcrel_diff in tcg_out_ldst
On 1/18/23 09:11, Richard Henderson wrote:
Take the w^x split into account when computing the
pc-relative distance to an absolute pointer.
Signed-off-by: Richard Henderson
---
tcg/loongarch64/tcg-target.c.inc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tcg/loongarch64/tcg-target.c.inc b/tcg/loongarch64/tcg-target.c.inc
index 29d75c80eb..d6926bdb83 100644
--- a/tcg/loongarch64/tcg-target.c.inc
+++ b/tcg/loongarch64/tcg-target.c.inc
@@ -702,7 +702,7 @@ static void tcg_out_ldst(TCGContext *s, LoongArchInsn opc,
TCGReg data,
intptr_t imm12 = sextreg(offset, 0, 12);
if (offset != imm12) {
-intptr_t diff = offset - (uintptr_t)s->code_ptr;
+intptr_t diff = tcg_pcrel_diff(s, (void *)offset);
if (addr == TCG_REG_ZERO && diff == (int32_t)diff) {
imm12 = sextreg(diff, 0, 12);
Reviewed-by: WANG Xuerui
Thanks for the catch!
Re: [PATCH v2 03/10] target/loongarch: Disassemble pcadd* addresses
On 1/18/23 09:11, Richard Henderson wrote: Print both the raw field and the resolved pc-relative address, as we do for branches. Signed-off-by: Richard Henderson --- target/loongarch/disas.c | 37 + 1 file changed, 33 insertions(+), 4 deletions(-) Reviewed-by: WANG Xuerui Thanks!
Re: [PATCH] vhost-user-fs: add capability to allow migration
On Fri, Jan 20, 2023 at 07:37:18PM +0200, Anton Kuchin wrote:
> On 20/01/2023 15:58, Michael S. Tsirkin wrote:
> > On Thu, Jan 19, 2023 at 03:45:06PM +0200, Anton Kuchin wrote:
> > > On 19/01/2023 14:51, Michael S. Tsirkin wrote:
> > > > On Sun, Jan 15, 2023 at 07:09:03PM +0200, Anton Kuchin wrote:
> > > > > Now any vhost-user-fs device makes VM unmigratable, that also prevents
> > > > > qemu update without stopping the VM. In most cases that makes sense
> > > > > because qemu has no way to transfer FUSE session state.
> > > > >
> > > > > But we can give an option to orchestrator to override this if it can
> > > > > guarantee that state will be preserved (e.g. it uses migration to
> > > > > update qemu and dst will run on the same host as src and use the same
> > > > > socket endpoints).
> > > > >
> > > > > This patch keeps default behavior that prevents migration with such
> > > > > devices
> > > > > but adds migration capability 'vhost-user-fs' to explicitly allow
> > > > > migration.
> > > > >
> > > > > Signed-off-by: Anton Kuchin
> > > > > ---
> > > > >hw/virtio/vhost-user-fs.c | 25 -
> > > > >qapi/migration.json | 7 ++-
> > > > >2 files changed, 30 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > > > > index f5049735ac..13d920423e 100644
> > > > > --- a/hw/virtio/vhost-user-fs.c
> > > > > +++ b/hw/virtio/vhost-user-fs.c
> > > > > @@ -24,6 +24,7 @@
> > > > >#include "hw/virtio/vhost-user-fs.h"
> > > > >#include "monitor/monitor.h"
> > > > >#include "sysemu/sysemu.h"
> > > > > +#include "migration/migration.h"
> > > > >static const int user_feature_bits[] = {
> > > > >VIRTIO_F_VERSION_1,
> > > > > @@ -298,9 +299,31 @@ static struct vhost_dev
> > > > > *vuf_get_vhost(VirtIODevice *vdev)
> > > > >return &fs->vhost_dev;
> > > > >}
> > > > > +static int vhost_user_fs_pre_save(void *opaque)
> > > > > +{
> > > > > +MigrationState *s = migrate_get_current();
> > > > > +
> > > > > +if
> > > > > (!s->enabled_capabilities[MIGRATION_CAPABILITY_VHOST_USER_FS]) {
> > > > > +error_report("Migration of vhost-user-fs devices requires
> > > > > internal FUSE "
> > > > > + "state of backend to be preserved. If
> > > > > orchestrator can "
> > > > > + "guarantee this (e.g. dst connects to the same
> > > > > backend "
> > > > > + "instance or backend state is migrated) set
> > > > > 'vhost-user-fs' "
> > > > > + "migration capability to true to enable
> > > > > migration.");
> > > > Isn't it possible that some backends are same and some are not?
> > > > Shouldn't this be a device property then?
> > > If some are not the same it is not guaranteed that correct FUSE
> > > state is present there, so orchestrator shouldn't set the capability
> > > because this can result in destination devices being broken (they'll
> > > be fine after the remount in guest, but this is guest visible and is
> > > not acceptable).
> > >
> > > I can imagine smart orchestrator and backend that can transfer
> > > internal FUSE state, but we are not there yet, and this would be
> > > their responsibility then to ensure endpoint compatibility between src
> > > and dst and set the capability (that's why I put "e.g." and "or" in
> > > the error description).
> > So instead of relying on the orchestrator how about making it a device
> > property?
>
> We have to rely on the orchestrator here and I can't see how a property
> can help us with this: same device can be migratable or not depending
> on if the destination is the same host, what features backend supports,
> how management software works and other factors of environment that
> are not accessible from qemu or backend daemon.
> So in the end we'll end up with orchestrator having to setup flags for
> each device before each migration based on information only it can
> have - in my opinion this is worse than just giving the orchestrator
> a single flag that it can set after calculating the decision for
> the particular migration that it organizes.
>
> >
> >
> > > >
> > > >
> > > > > +return -1;
> > > > > +}
> > > > > +
> > > > > +return 0;
> > > > > +}
> > > > > +
> > > > >static const VMStateDescription vuf_vmstate = {
> > > > >.name = "vhost-user-fs",
> > > > > -.unmigratable = 1,
> > > > > +.minimum_version_id = 0,
> > > > > +.version_id = 0,
> > > > > +.fields = (VMStateField[]) {
> > > > > +VMSTATE_VIRTIO_DEVICE,
> > > > > +VMSTATE_END_OF_LIST()
> > > > > +},
> > > > > + .pre_save = vhost_user_fs_pre_save,
> > > > >};
> > > > >static Property vuf_properties[] = {
> > > > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > > > index 88ecf86ac8..9a229ea884 100644
> > > > > --- a/qapi/migration.json
> > > > > +++ b/qapi/migration.json
> > > > > @@ -
