Re: [RFC PATCH v5 16/16] hw/char/pl011: Implement TX FIFO
On 19/07/2024 19:10, Philippe Mathieu-Daudé wrote:
If the UART back-end chardev doesn't drain data as fast as stdout
does or blocks, buffer in the TX FIFO to try again later.
This avoids having the IO-thread busy waiting on chardev back-ends,
reported recently when testing the Trusted Reference Stack and
using the socket backend.
Implement registering a front-end 'watch' callback on back-end
events, so we can resume transmitting when the back-end is writable
again, not blocking the main loop.
Similarly to the RX FIFO path, FIFO level selection is not
implemented (interrupt is triggered when a single byte is available
in the FIFO).
Reported-by: Mikko Rapeli
Suggested-by: Alex Bennée
Signed-off-by: Philippe Mathieu-Daudé
---
RFC: Something is still broken, some characters are emitted async...
---
hw/char/pl011.c | 60
hw/char/trace-events | 1 +
2 files changed, 51 insertions(+), 10 deletions(-)
diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index cfa3fd3da4..9f72b6a765 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -240,7 +240,9 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition
cond, void *opaque)
{
PL011State *s = opaque;
int bytes_consumed;
-uint8_t data;
+const uint8_t *buf;
+uint32_t buflen;
+uint32_t count;
if (!(s->cr & CR_UARTEN)) {
qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled
UART\n");
@@ -249,25 +251,40 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition
cond, void *opaque)
qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled TX
UART\n");
}
+count = fifo8_num_used(>xmit_fifo);
+if (count < 1) {
+/* FIFO empty */
+return G_SOURCE_REMOVE;
+}
+
if (!qemu_chr_fe_backend_connected(>chr)) {
/* Instant drain the fifo when there's no back-end. */
pl011_drain_tx(s);
return G_SOURCE_REMOVE;
}
-data = fifo8_pop(>xmit_fifo);
-bytes_consumed = 1;
+buf = fifo8_peek_buf(>xmit_fifo, count, );
-/*
- * XXX this blocks entire thread. Rewrite to use
- * qemu_chr_fe_write and background I/O callbacks
- */
-qemu_chr_fe_write_all(>chr, , bytes_consumed);
+/* Transmit as much data as we can. */
+bytes_consumed = qemu_chr_fe_write(>chr, buf, buflen);
trace_pl011_fifo_tx_xmit(bytes_consumed);
+if (bytes_consumed < 0) {
+/* Error in back-end: drain the fifo. */
+pl011_drain_tx(s);
+return G_SOURCE_REMOVE;
+}
+
+/* Pop the data we could transmit. */
+fifo8_pop_buf(>xmit_fifo, bytes_consumed, NULL);
s->int_level |= INT_TX;
pl011_update(s);
One of the gotchas with Fifo8 is that whilst fifo8_push(), fifo8_pop() and
fifo8_push_all() will wrap the FIFO buffer, fifo8_{peek,pop}_buf() do not. For
example fifo8_num_used() could return 15, but if xmit_fifo->head is set to 15 then
fifo8_{peek_pop}_buf() would return 1 leaving 14 characters in the FIFO at the end of
pl011_xmit().
Possible solutions could be to use a loop to send one character at a time
similar to:
while (fifo8_num_used(>xmit_fifo)) {
uint8_t c = fifo8_pop(>xmit_fifo);
if (qemu_chr_fe_write(>chr, , 1) == -1) {
fifo8_push(>xmit_fifo, c);
break;
}
}
Or else use a solution similar to the one I used for ESP at
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/scsi/esp.c?ref_type=heads#L200.
I did think about whether it was worth adding a function similar to the one used for
ESP to the Fifo8 API, but wasn't sure it was worth it at the time.
+if (!fifo8_is_empty(>xmit_fifo)) {
+/* Reschedule another transmission if we couldn't transmit all. */
+return G_SOURCE_CONTINUE;
+}
+
return G_SOURCE_REMOVE;
}
@@ -290,6 +307,10 @@ static void pl011_write_txdata(PL011State *s, uint8_t data)
trace_pl011_fifo_tx_put(data);
pl011_loopback_tx(s, data);
fifo8_push(>xmit_fifo, data);
+if (fifo8_is_full(>xmit_fifo)) {
+s->flags |= PL011_FLAG_TXFF;
+}
+
pl011_xmit(NULL, G_IO_OUT, s);
}
@@ -488,10 +509,24 @@ static void pl011_write(void *opaque, hwaddr offset,
pl011_trace_baudrate_change(s);
break;
case 11: /* UARTLCR_H */
-/* Reset the FIFO state on FIFO enable or disable */
if ((s->lcr ^ value) & LCR_FEN) {
-pl011_reset_rx_fifo(s);
+bool fifo_enabled = value & LCR_FEN;
+
+trace_pl011_fifo_enable(fifo_enabled);
+if (fifo_enabled) {
+/* Transmit and receive FIFO buffers are enabled (FIFO mode).
*/
+fifo8_change_capacity(>xmit_fifo, PL011_FIFO_DEPTH);
+} else {
+/*
+ * FIFOs are disabled (character mode) that is, the FIFOs
+ * become 1-byte-deep holding registers.
+ */
+
Re: [PATCH 3/3] util/fifo8: Introduce fifo8_change_capacity()
On 19/07/2024 16:16, Philippe Mathieu-Daudé wrote:
FIFOs can be resized at runtime. Introduce the
fifo8_change_capacity() method to do that.
When capacity is changed, the FIFO must be reset.
Signed-off-by: Philippe Mathieu-Daudé
---
include/qemu/fifo8.h | 10 ++
util/fifo8.c | 7 +++
2 files changed, 17 insertions(+)
diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
index c6295c6ff0..9fe0555a24 100644
--- a/include/qemu/fifo8.h
+++ b/include/qemu/fifo8.h
@@ -31,6 +31,16 @@ void fifo8_create(Fifo8 *fifo, uint32_t capacity);
void fifo8_destroy(Fifo8 *fifo);
+/**
+ * fifo8_change_capacity:
+ * @fifo: struct Fifo8 to change the capacity
+ * @capacity: new capacity of the FIFO
+ *
+ * Change a FIFO capacity to the specified size. The FIFO is reset.
+ */
+
+void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity);
+
/**
* fifo8_push:
* @fifo: FIFO to push to
diff --git a/util/fifo8.c b/util/fifo8.c
index 2925fe5611..c453afd774 100644
--- a/util/fifo8.c
+++ b/util/fifo8.c
@@ -34,6 +34,13 @@ void fifo8_destroy(Fifo8 *fifo)
g_free(fifo->data);
}
+void fifo8_change_capacity(Fifo8 *fifo, uint32_t capacity)
+{
+fifo->data = g_renew(uint8_t, fifo->data, capacity);
+fifo->capacity = capacity;
+fifo8_reset(fifo);
+}
+
void fifo8_push(Fifo8 *fifo, uint8_t data)
{
assert(fifo->num < fifo->capacity);
The changes look okay, however I'm a little confused as to why this is needed as
generally hardware FIFOs are a fixed size? Presumably this is related to the PL011
series?
ATB,
Mark.
Re: [PATCH 2/3] util/fifo8: Use fifo8_reset() in fifo8_create()
On 19/07/2024 16:16, Philippe Mathieu-Daudé wrote:
Avoid open-coding fifo8_reset() in fifo8_create().
Signed-off-by: Philippe Mathieu-Daudé
---
util/fifo8.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/util/fifo8.c b/util/fifo8.c
index 4e01b532d9..2925fe5611 100644
--- a/util/fifo8.c
+++ b/util/fifo8.c
@@ -16,12 +16,17 @@
#include "migration/vmstate.h"
#include "qemu/fifo8.h"
+void fifo8_reset(Fifo8 *fifo)
+{
+fifo->num = 0;
+fifo->head = 0;
+}
+
void fifo8_create(Fifo8 *fifo, uint32_t capacity)
{
fifo->data = g_new(uint8_t, capacity);
fifo->capacity = capacity;
-fifo->head = 0;
-fifo->num = 0;
+fifo8_reset(fifo);
}
void fifo8_destroy(Fifo8 *fifo)
@@ -97,12 +102,6 @@ const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max,
uint32_t *numptr)
return fifo8_peekpop_buf(fifo, max, numptr, true);
}
-void fifo8_reset(Fifo8 *fifo)
-{
-fifo->num = 0;
-fifo->head = 0;
-}
-
bool fifo8_is_empty(Fifo8 *fifo)
{
return (fifo->num == 0);
Reviewed-by: Mark Cave-Ayland
ATB,
Mark.
Re: [PATCH 1/3] chardev/char-fe: Document returned value on error
On 19/07/2024 16:16, Philippe Mathieu-Daudé wrote: qemu_chr_fe_add_watch() and qemu_chr_fe_write[_all]() return -1 on error. Mention it in the documentation. Signed-off-by: Philippe Mathieu-Daudé --- include/chardev/char-fe.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h index ecef182835..3310449eaf 100644 --- a/include/chardev/char-fe.h +++ b/include/chardev/char-fe.h @@ -228,6 +228,7 @@ guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond, * is thread-safe. * * Returns: the number of bytes consumed (0 if no associated Chardev) + * or -1 on error. */ int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len); @@ -242,6 +243,7 @@ int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len); * attempted to be written. This function is thread-safe. * * Returns: the number of bytes consumed (0 if no associated Chardev) + * or -1 on error. */ int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len); @@ -253,6 +255,7 @@ int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len); * Read data to a buffer from the back end. * * Returns: the number of bytes read (0 if no associated Chardev) + * or -1 on error. */ int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len); Reviewed-by: Mark Cave-Ayland ATB, Mark.
Re: [PATCH] esp.c: remove transfer size check from DMA DATA IN and DATA OUT transfers
On 15/07/2024 07:48, Philippe Mathieu-Daudé wrote: On 14/7/24 00:42, Mark Cave-Ayland wrote: The transfer size check was originally added to prevent consecutive DMA TI commands from causing an assert() due to an existing SCSI request being in progress, but since the last set of updates [*] this is no longer required. Remove the transfer size check from DMA DATA IN and DATA OUT transfers so that issuing a DMA TI command when there is no data left to transfer does not cause an assert() due to an existing SCSI request being in progress. [*] See commits f3ace75be8..78d68f312a Signed-off-by: Mark Cave-Ayland Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2415 --- hw/scsi/esp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé Queued adding [*], thanks. Awesome, thanks Phil! ATB, Mark.
[PATCH] esp.c: remove transfer size check from DMA DATA IN and DATA OUT transfers
The transfer size check was originally added to prevent consecutive DMA TI
commands from causing an assert() due to an existing SCSI request being in
progress, but since the last set of updates this is no longer required.
Remove the transfer size check from DMA DATA IN and DATA OUT transfers so
that issuing a DMA TI command when there is no data left to transfer does
not cause an assert() due to an existing SCSI request being in progress.
Signed-off-by: Mark Cave-Ayland
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2415
---
hw/scsi/esp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 5d9b52632e..8504dd30a0 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -594,7 +594,7 @@ static void esp_do_dma(ESPState *s)
if (!s->current_req) {
return;
}
-if (s->async_len == 0 && esp_get_tc(s) && s->ti_size) {
+if (s->async_len == 0 && esp_get_tc(s)) {
/* Defer until data is available. */
return;
}
@@ -647,7 +647,7 @@ static void esp_do_dma(ESPState *s)
if (!s->current_req) {
return;
}
-if (s->async_len == 0 && esp_get_tc(s) && s->ti_size) {
+if (s->async_len == 0 && esp_get_tc(s)) {
/* Defer until data is available. */
return;
}
--
2.39.2
Re: [PATCH 2/2] hw/isa/vt82c686.c: Embed i8259 irq in device state instead of allocating
On 01/07/2024 13:58, Peter Maydell wrote:
On Sat, 29 Jun 2024 at 21:01, BALATON Zoltan wrote:
To avoid a warning about unfreed qemu_irq embed the i8259 irq in the
device state instead of allocating it.
Signed-off-by: BALATON Zoltan
---
hw/isa/vt82c686.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 8582ac0322..834051abeb 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -592,6 +592,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
struct ViaISAState {
PCIDevice dev;
+
+IRQState i8259_irq;
qemu_irq cpu_intr;
qemu_irq *isa_irqs_in;
uint16_t irq_state[ISA_NUM_IRQS];
@@ -715,13 +717,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
ViaISAState *s = VIA_ISA(d);
DeviceState *dev = DEVICE(d);
PCIBus *pci_bus = pci_get_bus(d);
-qemu_irq *isa_irq;
ISABus *isa_bus;
int i;
qdev_init_gpio_out(dev, >cpu_intr, 1);
qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);
-isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
+qemu_init_irq(>i8259_irq, via_isa_request_i8259_irq, s, 0);
isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
errp);
So if I understand correctly, this IRQ line isn't visible
from outside this chip, we're just trying to wire together
two internal components of the chip? If so, I agree that
this seems a better way than creating a named GPIO that
we then have to document as a "not really an external
connection, don't try to use this" line. (We've done that
before I think in other devices, and it works but it's
a bit odd-looking.)
It's basically wiring up the 8259 PIC (ISA) IRQs which aren't implemented using qdev
gpios to an internal IRQ handler.
If the 8259 PIC (ISA) IRQs were already converted to qdev gpios, then presumably
using a qdev gpio would be the correct solution for this? I'm conscious that if we do
decide to introduce another method for wiring IRQs then there should be clear
criteria for developers and reviewers as to which method is appropriate for each use
case.
ATB,
Mark.
Re: [PATCH] hw/display/tcx: Fix out-of-bounds access in tcx_blit_writel
On 30/06/2024 14:04, Zheyu Ma wrote:
This patch addresses a potential out-of-bounds memory access issue in the
tcx_blit_writel function. It adds bounds checking to ensure that memory
accesses do not exceed the allocated VRAM size. If an out-of-bounds access
is detected, an error is logged using qemu_log_mask.
ASAN log:
==2960379==ERROR: AddressSanitizer: SEGV on unknown address 0x7f524752fd01 (pc
0x7f525c2c4881 bp 0x7ffdaf87bfd0 sp 0x7ffdaf87b788 T0)
==2960379==The signal is caused by a READ memory access.
#0 0x7f525c2c4881 in memcpy
string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:222
#1 0x55aa782bd5b1 in __asan_memcpy
llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3
#2 0x55aa7854dedd in tcx_blit_writel hw/display/tcx.c:590:13
Reproducer:
cat << EOF | qemu-system-sparc -display none \
-machine accel=qtest, -m 512M -machine LX -m 256 -qtest stdio
writel 0x562e98c4 0x3d92fd01
EOF
Signed-off-by: Zheyu Ma
---
hw/display/tcx.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 99507e7638..af43bea7f2 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -33,6 +33,7 @@
#include "migration/vmstate.h"
#include "qemu/error-report.h"
#include "qemu/module.h"
+#include "qemu/log.h"
#include "qom/object.h"
#define TCX_ROM_FILE "QEMU,tcx.bin"
@@ -577,6 +578,14 @@ static void tcx_blit_writel(void *opaque, hwaddr addr,
addr = (addr >> 3) & 0xf;
adsr = val & 0xff;
len = ((val >> 24) & 0x1f) + 1;
+
+if (addr + len > s->vram_size || adsr + len > s->vram_size) {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: VRAM access out of bounds. addr: 0x%lx, adsr: 0x%x,
len: %u\n",
+ __func__, addr, adsr, len);
+return;
+}
+
if (adsr == 0xff) {
memset(>vram[addr], s->tmpblit, len);
if (s->depth == 24) {
What would happen if the source data plus length goes beyond the end of the
framebuffer but the destination lies completely within it? Presumably the length of
the data copy should be restricted to the length of the source data rather than the
entire copy being ignored?
ATB,
Mark.
[PATCH v2] hw/ide/macio.c: switch from using qemu_allocate_irq() to qdev input GPIOs
This prevents the IRQs from being leaked when the macio IDE device is used.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Peter Maydell
---
hw/ide/macio.c| 10 ++
include/hw/misc/macio/macio.h | 7 +--
2 files changed, 11 insertions(+), 6 deletions(-)
v2:
- Delete dma_irq and ide_irq from MACIOIDEState
- Add R-B tag from Peter
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index aca90d04f0..e84bf2c9f6 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -420,7 +420,8 @@ static void macio_ide_realizefn(DeviceState *dev, Error
**errp)
{
MACIOIDEState *s = MACIO_IDE(dev);
-ide_bus_init_output_irq(>bus, s->ide_irq);
+ide_bus_init_output_irq(>bus,
+qdev_get_gpio_in(dev, MACIO_IDE_PMAC_IDE_IRQ));
/* Register DMA callbacks */
s->dma.ops = _ops;
@@ -456,8 +457,8 @@ static void macio_ide_initfn(Object *obj)
sysbus_init_mmio(d, >mem);
sysbus_init_irq(d, >real_ide_irq);
sysbus_init_irq(d, >real_dma_irq);
-s->dma_irq = qemu_allocate_irq(pmac_ide_irq, s, 0);
-s->ide_irq = qemu_allocate_irq(pmac_ide_irq, s, 1);
+
+qdev_init_gpio_in(DEVICE(obj), pmac_ide_irq, MACIO_IDE_PMAC_NIRQS);
object_property_add_link(obj, "dbdma", TYPE_MAC_DBDMA,
(Object **) >dbdma,
@@ -508,7 +509,8 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo
**hd_table)
void macio_ide_register_dma(MACIOIDEState *s)
{
-DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq,
+DBDMA_register_channel(s->dbdma, s->channel,
+ qdev_get_gpio_in(DEVICE(s), MACIO_IDE_PMAC_DMA_IRQ),
pmac_ide_transfer, pmac_ide_flush, s);
}
diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
index 2b54da6b31..16aa95b876 100644
--- a/include/hw/misc/macio/macio.h
+++ b/include/hw/misc/macio/macio.h
@@ -80,8 +80,6 @@ struct MACIOIDEState {
uint32_t channel;
qemu_irq real_ide_irq;
qemu_irq real_dma_irq;
-qemu_irq ide_irq;
-qemu_irq dma_irq;
MemoryRegion mem;
IDEBus bus;
@@ -92,6 +90,11 @@ struct MACIOIDEState {
uint32_t irq_reg;
};
+#define MACIO_IDE_PMAC_NIRQS 2
+
+#define MACIO_IDE_PMAC_DMA_IRQ 0
+#define MACIO_IDE_PMAC_IDE_IRQ 1
+
void macio_ide_init_drives(MACIOIDEState *ide, DriveInfo **hd_table);
void macio_ide_register_dma(MACIOIDEState *ide);
--
2.39.2
Re: [PATCH] hw/ide/macio.c: switch from using qemu_allocate_irq() to qdev input GPIOs
On 28/06/2024 16:28, Peter Maydell wrote:
On Fri, 28 Jun 2024 at 11:55, Mark Cave-Ayland
wrote:
This prevents the IRQs from being leaked when the macio IDE device is used.
Signed-off-by: Mark Cave-Ayland
---
hw/ide/macio.c| 10 ++
include/hw/misc/macio/macio.h | 5 +
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index aca90d04f0..e84bf2c9f6 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -420,7 +420,8 @@ static void macio_ide_realizefn(DeviceState *dev, Error
**errp)
{
MACIOIDEState *s = MACIO_IDE(dev);
-ide_bus_init_output_irq(>bus, s->ide_irq);
+ide_bus_init_output_irq(>bus,
+qdev_get_gpio_in(dev, MACIO_IDE_PMAC_IDE_IRQ));
/* Register DMA callbacks */
s->dma.ops = _ops;
@@ -456,8 +457,8 @@ static void macio_ide_initfn(Object *obj)
sysbus_init_mmio(d, >mem);
sysbus_init_irq(d, >real_ide_irq);
sysbus_init_irq(d, >real_dma_irq);
-s->dma_irq = qemu_allocate_irq(pmac_ide_irq, s, 0);
-s->ide_irq = qemu_allocate_irq(pmac_ide_irq, s, 1);
+
+qdev_init_gpio_in(DEVICE(obj), pmac_ide_irq, MACIO_IDE_PMAC_NIRQS);
object_property_add_link(obj, "dbdma", TYPE_MAC_DBDMA,
(Object **) >dbdma,
@@ -508,7 +509,8 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo
**hd_table)
void macio_ide_register_dma(MACIOIDEState *s)
{
-DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq,
+DBDMA_register_channel(s->dbdma, s->channel,
+ qdev_get_gpio_in(DEVICE(s), MACIO_IDE_PMAC_DMA_IRQ),
pmac_ide_transfer, pmac_ide_flush, s);
}
diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
index 2b54da6b31..869b66055b 100644
--- a/include/hw/misc/macio/macio.h
+++ b/include/hw/misc/macio/macio.h
@@ -92,6 +92,11 @@ struct MACIOIDEState {
uint32_t irq_reg;
};
+#define MACIO_IDE_PMAC_NIRQS 2
+
+#define MACIO_IDE_PMAC_DMA_IRQ 0
+#define MACIO_IDE_PMAC_IDE_IRQ 1
+
void macio_ide_init_drives(MACIOIDEState *ide, DriveInfo **hd_table);
void macio_ide_register_dma(MACIOIDEState *ide);
Can we also now delete the dma_irq and ide_irq fields from the
MACIOIDEState struct?
Otherwise
Reviewed-by: Peter Maydell
Ooops, yes. I'll update and send a v2 including your Reviewed-by tag.
ATB,
Mark.
[PATCH] hw/ide/macio.c: switch from using qemu_allocate_irq() to qdev input GPIOs
This prevents the IRQs from being leaked when the macio IDE device is used.
Signed-off-by: Mark Cave-Ayland
---
hw/ide/macio.c| 10 ++
include/hw/misc/macio/macio.h | 5 +
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index aca90d04f0..e84bf2c9f6 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -420,7 +420,8 @@ static void macio_ide_realizefn(DeviceState *dev, Error
**errp)
{
MACIOIDEState *s = MACIO_IDE(dev);
-ide_bus_init_output_irq(>bus, s->ide_irq);
+ide_bus_init_output_irq(>bus,
+qdev_get_gpio_in(dev, MACIO_IDE_PMAC_IDE_IRQ));
/* Register DMA callbacks */
s->dma.ops = _ops;
@@ -456,8 +457,8 @@ static void macio_ide_initfn(Object *obj)
sysbus_init_mmio(d, >mem);
sysbus_init_irq(d, >real_ide_irq);
sysbus_init_irq(d, >real_dma_irq);
-s->dma_irq = qemu_allocate_irq(pmac_ide_irq, s, 0);
-s->ide_irq = qemu_allocate_irq(pmac_ide_irq, s, 1);
+
+qdev_init_gpio_in(DEVICE(obj), pmac_ide_irq, MACIO_IDE_PMAC_NIRQS);
object_property_add_link(obj, "dbdma", TYPE_MAC_DBDMA,
(Object **) >dbdma,
@@ -508,7 +509,8 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo
**hd_table)
void macio_ide_register_dma(MACIOIDEState *s)
{
-DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq,
+DBDMA_register_channel(s->dbdma, s->channel,
+ qdev_get_gpio_in(DEVICE(s), MACIO_IDE_PMAC_DMA_IRQ),
pmac_ide_transfer, pmac_ide_flush, s);
}
diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
index 2b54da6b31..869b66055b 100644
--- a/include/hw/misc/macio/macio.h
+++ b/include/hw/misc/macio/macio.h
@@ -92,6 +92,11 @@ struct MACIOIDEState {
uint32_t irq_reg;
};
+#define MACIO_IDE_PMAC_NIRQS 2
+
+#define MACIO_IDE_PMAC_DMA_IRQ 0
+#define MACIO_IDE_PMAC_IDE_IRQ 1
+
void macio_ide_init_drives(MACIOIDEState *ide, DriveInfo **hd_table);
void macio_ide_register_dma(MACIOIDEState *ide);
--
2.39.2
Re: [PATCH v2 04/15] hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259
On 27/06/2024 14:37, Akihiko Odaki wrote: This fixes qemu_irq array leak. Signed-off-by: Akihiko Odaki --- hw/isa/vt82c686.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 8582ac0322eb..629d2d568137 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -715,13 +715,14 @@ static void via_isa_realize(PCIDevice *d, Error **errp) ViaISAState *s = VIA_ISA(d); DeviceState *dev = DEVICE(d); PCIBus *pci_bus = pci_get_bus(d); -qemu_irq *isa_irq; +qemu_irq isa_irq; ISABus *isa_bus; int i; qdev_init_gpio_out(dev, >cpu_intr, 1); qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS); -isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); +qdev_init_gpio_in_named(dev, via_isa_request_i8259_irq, "i8259", 1); +isa_irq = qdev_get_gpio_in_named(dev, "i8259", 0); isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d), errp); @@ -729,7 +730,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp) return; } -s->isa_irqs_in = i8259_init(isa_bus, *isa_irq); +s->isa_irqs_in = i8259_init(isa_bus, isa_irq); isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in); i8254_pit_init(isa_bus, 0x40, 0, NULL); i8257_dma_init(OBJECT(d), isa_bus, 0); Have you confirmed that the machines using the VIA can still boot correctly with this change? I have a vague memory that Phil tried something like this, but due to legacy code poking around directly in the ISA IRQ array after realize it caused some things to break. ATB, Mark.
Re: [PATCH v2 03/15] hw/ide: Remove internal DMA qemu_irq
On 27/06/2024 14:37, Akihiko Odaki wrote:
A function pointer is sufficient for internal usage. Replacing qemu_irq
with one fixes the leak of qemu_irq.
Signed-off-by: Akihiko Odaki
---
include/hw/ppc/mac_dbdma.h | 5 +++--
hw/ide/macio.c | 11 +++
hw/misc/macio/mac_dbdma.c | 10 +-
3 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index 4a3f644516b3..1e1973c39580 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -32,6 +32,7 @@
typedef struct DBDMA_io DBDMA_io;
typedef void (*DBDMA_flush)(DBDMA_io *io);
+typedef void (*DBDMA_irq)(DBDMA_io *io);
typedef void (*DBDMA_rw)(DBDMA_io *io);
typedef void (*DBDMA_end)(DBDMA_io *io);
struct DBDMA_io {
@@ -154,7 +155,7 @@ typedef struct dbdma_cmd {
typedef struct DBDMA_channel {
int channel;
uint32_t regs[DBDMA_REGS];
-qemu_irq irq;
+DBDMA_irq irq;
DBDMA_io io;
DBDMA_rw rw;
DBDMA_flush flush;
@@ -172,7 +173,7 @@ typedef struct DBDMAState DBDMAState;
/* Externally callable functions */
-void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
+void DBDMA_register_channel(void *dbdma, int nchan, DBDMA_irq irq,
DBDMA_rw rw, DBDMA_flush flush,
void *opaque);
void DBDMA_kick(DBDMAState *dbdma);
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 9c96a857a7c1..425b670a52a9 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -427,9 +427,8 @@ static void macio_ide_realizefn(DeviceState *dev, Error
**errp)
s->bus.dma = >dma;
}
-static void pmac_irq(void *opaque, int n, int level)
+static void pmac_irq(MACIOIDEState *s, int n, int level)
{
-MACIOIDEState *s = opaque;
uint32_t mask = 0x8000u >> n;
/* We need to reflect the IRQ state in the irq register */
@@ -446,6 +445,11 @@ static void pmac_irq(void *opaque, int n, int level)
}
}
+static void pmac_dma_irq(DBDMA_io *io)
+{
+pmac_irq(io->opaque, 0, 1);
+}
+
static void pmac_ide_irq(void *opaque, int n, int level)
{
pmac_irq(opaque, 1, level);
@@ -461,7 +465,6 @@ static void macio_ide_initfn(Object *obj)
sysbus_init_mmio(d, >mem);
sysbus_init_irq(d, >real_ide_irq);
sysbus_init_irq(d, >real_dma_irq);
-s->dma_irq = qemu_allocate_irq(pmac_irq, s, 0);
qdev_init_gpio_in_named_with_opaque(DEVICE(obj), pmac_ide_irq, s, NULL,
1);
object_property_add_link(obj, "dbdma", TYPE_MAC_DBDMA,
@@ -513,7 +516,7 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo
**hd_table)
void macio_ide_register_dma(MACIOIDEState *s)
{
-DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq,
+DBDMA_register_channel(s->dbdma, s->channel, pmac_dma_irq,
pmac_ide_transfer, pmac_ide_flush, s);
}
diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 2a528ea08caf..3450105ad851 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -114,7 +114,7 @@ static void kill_channel(DBDMA_channel *ch)
ch->regs[DBDMA_STATUS] |= DEAD;
ch->regs[DBDMA_STATUS] &= ~ACTIVE;
-qemu_irq_raise(ch->irq);
+ch->irq(>io);
}
static void conditional_interrupt(DBDMA_channel *ch)
@@ -133,7 +133,7 @@ static void conditional_interrupt(DBDMA_channel *ch)
case INTR_NEVER: /* don't interrupt */
return;
case INTR_ALWAYS: /* always interrupt */
-qemu_irq_raise(ch->irq);
+ch->irq(>io);
DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
return;
}
@@ -148,13 +148,13 @@ static void conditional_interrupt(DBDMA_channel *ch)
switch(intr) {
case INTR_IFSET: /* intr if condition bit is 1 */
if (cond) {
-qemu_irq_raise(ch->irq);
+ch->irq(>io);
DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
}
return;
case INTR_IFCLR: /* intr if condition bit is 0 */
if (!cond) {
-qemu_irq_raise(ch->irq);
+ch->irq(>io);
DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
}
return;
@@ -562,7 +562,7 @@ void DBDMA_kick(DBDMAState *dbdma)
qemu_bh_schedule(dbdma->bh);
}
-void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
+void DBDMA_register_channel(void *dbdma, int nchan, DBDMA_irq irq,
DBDMA_rw rw, DBDMA_flush flush,
void *opaque)
{
At first glance I can't say I'm keen on this: in general we should be moving towards
standardising on QEMU irqs or qdev gpios rather than introducing a custom function.
As per my previous email I suspect this is another symptom that something is wrong
with the modelling, so I will take a look.
ATB,
Mark.
Re: [PATCH v2 02/15] hw/ide: Convert macio ide_irq into GPIO line
On 27/06/2024 14:37, Akihiko Odaki wrote:
macio ide_irq is connected to the IDE bus. This fixes the leak of
ide_irq.
Signed-off-by: Akihiko Odaki
---
hw/ide/macio.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index aca90d04f0e8..9c96a857a7c1 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -427,7 +427,7 @@ static void macio_ide_realizefn(DeviceState *dev, Error
**errp)
s->bus.dma = >dma;
}
-static void pmac_ide_irq(void *opaque, int n, int level)
+static void pmac_irq(void *opaque, int n, int level)
{
MACIOIDEState *s = opaque;
uint32_t mask = 0x8000u >> n;
@@ -446,6 +446,11 @@ static void pmac_ide_irq(void *opaque, int n, int level)
}
}
+static void pmac_ide_irq(void *opaque, int n, int level)
+{
+pmac_irq(opaque, 1, level);
+}
+
static void macio_ide_initfn(Object *obj)
{
SysBusDevice *d = SYS_BUS_DEVICE(obj);
@@ -456,8 +461,8 @@ static void macio_ide_initfn(Object *obj)
sysbus_init_mmio(d, >mem);
sysbus_init_irq(d, >real_ide_irq);
sysbus_init_irq(d, >real_dma_irq);
-s->dma_irq = qemu_allocate_irq(pmac_ide_irq, s, 0);
-s->ide_irq = qemu_allocate_irq(pmac_ide_irq, s, 1);
+s->dma_irq = qemu_allocate_irq(pmac_irq, s, 0);
+qdev_init_gpio_in_named_with_opaque(DEVICE(obj), pmac_ide_irq, s, NULL, 1);
object_property_add_link(obj, "dbdma", TYPE_MAC_DBDMA,
(Object **) >dbdma,
This doesn't feel quite right: generally I consider the use of
qdev_init_gpio_in_named_with_opaque() to indicate that the underlying modelling is
incorrect. Let me have a look and see if I can figure out what's supposed to be
happening.
I guess I should probably be marked as maintainer of hw/ide/macio.c as it is part of
the macio device, but it looks as if this is missing from MAINTAINERS.
ATB,
Mark.
Re: [PATCH 02/14] hw/ide: Free macio-ide IRQs
On 26/06/2024 13:59, Peter Maydell wrote:
On Wed, 26 Jun 2024 at 12:09, Akihiko Odaki wrote:
This suppresses LeakSanitizer warnings.
Signed-off-by: Akihiko Odaki
---
hw/ide/macio.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index aca90d04f0e8..d8fbc1a17ba6 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -464,6 +464,14 @@ static void macio_ide_initfn(Object *obj)
qdev_prop_allow_set_link_before_realize, 0);
}
+static void macio_ide_finalize(Object *obj)
+{
+MACIOIDEState *s = MACIO_IDE(obj);
+
+qemu_free_irq(s->dma_irq);
+qemu_free_irq(s->ide_irq);
+}
+
static Property macio_ide_properties[] = {
DEFINE_PROP_UINT32("channel", MACIOIDEState, channel, 0),
DEFINE_PROP_UINT32("addr", MACIOIDEState, addr, -1),
@@ -486,6 +494,7 @@ static const TypeInfo macio_ide_type_info = {
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(MACIOIDEState),
.instance_init = macio_ide_initfn,
+.instance_finalize = macio_ide_finalize,
.class_init = macio_ide_class_init,
};
Rather than this, I suspect macio_ide_initfn() should not
be using qemu_allocate_irq() in the first place. Looks like
maybe a QOM conversion that left a loose end un-tidied-up.
This is definitely old code: there used to be problems interfacing the IDE code with
qdev due to the hard-coded bus IRQs but I think this may is now possible with the
advent of ide_bus_init_output_irq().
I'll have a quick look and see what has changed in this area.
ATB,
Mark.
Re: [PATCH 1/2] target/m68k: implement do_unaligned_access callback for m68k CPUs
On 23/06/2024 20:47, Richard Henderson wrote:
On 6/23/24 04:57, Mark Cave-Ayland wrote:
+G_NORETURN void m68k_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
+ MMUAccessType access_type,
+ int mmu_idx, uintptr_t retaddr)
+{
+ CPUM68KState *env = cpu_env(cs);
+
+ raise_exception(env, EXCP_ADDRESS);
+}
Surely the address gets stored in the exception frame somewhere?
r~
Well. There is already some logic there for EXCP_ADDRESS in m68k_interrupt_all() so I
thought this would just work as-is, but upon closer reading of older CPU datasheets
it appears there are at least 2 different stack frame formats for CPUs before the 68040.
It looks like I'll have to do a bit more research before posting a v2.
ATB,
Mark.
Re: [PATCH 2/2] target/m68k: pass alignment into TCG memory load/store routines
On 23/06/2024 16:23, BALATON Zoltan wrote:
On Sun, 23 Jun 2024, Mark Cave-Ayland wrote:
Now that do_unaligned_access has been implemented for 68k CPUs, pass the
required
alignment into the TCG memory load/store routines. This allows the TCG memory
core
to generate an Address Error exception for unaligned memory accesses if
required.
Suggested-by: Laurent Vivier
Signed-off-by: Mark Cave-Ayland
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2165
---
target/m68k/translate.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 445966fb6a..661a7b4def 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -303,13 +303,18 @@ static inline TCGv gen_load(DisasContext *s, int opsize, TCGv
addr,
int sign, int index)
{
TCGv tmp = tcg_temp_new_i32();
+ MemOp memop = opsize | (sign ? MO_SIGN : 0) | MO_TE;
switch (opsize) {
case OS_BYTE:
+ tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
+ break;
case OS_WORD:
case OS_LONG:
- tcg_gen_qemu_ld_tl(tmp, addr, index,
- opsize | (sign ? MO_SIGN : 0) | MO_TE);
+ if (!m68k_feature(s->env, M68K_FEATURE_UNALIGNED_DATA)) {
+ memop |= MO_ALIGN_2;
+ }
+ tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
You could swap the order of these so byte comes last and fall through to it from
word/long to avoid duplicated line.
Maybe this answers my question about where it's restriced by CPU type. I wonder if
this check for M68K_FEATURE_UNALIGNED_DATA could be avoded here and done by checking
it in init and only set the unaligned method for CPUs that need it to not add
overhead for most CPUs that don't need it.
I don't think that it matters too much if the method isn't implemented as the logic
surrounding when to call do_unaligned_access is contained within the TCG core.
I'll have a go at updating the ordering and send a v2 if it looks good.
ATB,
Mark.
Re: [PATCH 1/2] target/m68k: implement do_unaligned_access callback for m68k CPUs
On 23/06/2024 16:11, BALATON Zoltan wrote:
On Sun, 23 Jun 2024, Mark Cave-Ayland wrote:
For m68k CPUs that do not support unaligned accesses, any such access should
cause the CPU to raise an Address Error exception.
Signed-off-by: Mark Cave-Ayland
---
target/m68k/cpu.c | 1 +
target/m68k/cpu.h | 4
target/m68k/op_helper.c | 11 +++
3 files changed, 16 insertions(+)
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index efd6bbded8..25e95f9f68 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -538,6 +538,7 @@ static const TCGCPUOps m68k_tcg_ops = {
.cpu_exec_interrupt = m68k_cpu_exec_interrupt,
.do_interrupt = m68k_cpu_do_interrupt,
.do_transaction_failed = m68k_cpu_transaction_failed,
+ .do_unaligned_access = m68k_cpu_do_unaligned_access,
#endif /* !CONFIG_USER_ONLY */
Why is it sysemu only? Shouldn't user mode cpu only emulation do the same? I also
don't get how this is restricted to pre 68020 CPUs or account for differences between
data and inst fetch on 20+ but I may be missing somerhing as I don't know this code
or 68k behaviour well. So this is just a question, I'm not saying it's wrong but I
don't understand why it's right.
I'm not exactly sure, but I'm guessing that this is handled by the host user code
since all CPUs that implement do_unaligned_access do so in a block contained within
#ifndef CONFIG_USER_ONLY ... #endif.
ATB,
Mark.
[PATCH 1/2] target/m68k: implement do_unaligned_access callback for m68k CPUs
For m68k CPUs that do not support unaligned accesses, any such access should
cause the CPU to raise an Address Error exception.
Signed-off-by: Mark Cave-Ayland
---
target/m68k/cpu.c | 1 +
target/m68k/cpu.h | 4
target/m68k/op_helper.c | 11 +++
3 files changed, 16 insertions(+)
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index efd6bbded8..25e95f9f68 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -538,6 +538,7 @@ static const TCGCPUOps m68k_tcg_ops = {
.cpu_exec_interrupt = m68k_cpu_exec_interrupt,
.do_interrupt = m68k_cpu_do_interrupt,
.do_transaction_failed = m68k_cpu_transaction_failed,
+.do_unaligned_access = m68k_cpu_do_unaligned_access,
#endif /* !CONFIG_USER_ONLY */
};
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index b5bbeedb7a..d4c9531b1c 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -590,6 +590,10 @@ void m68k_cpu_transaction_failed(CPUState *cs, hwaddr
physaddr, vaddr addr,
unsigned size, MMUAccessType access_type,
int mmu_idx, MemTxAttrs attrs,
MemTxResult response, uintptr_t retaddr);
+G_NORETURN void m68k_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
+ MMUAccessType access_type,
+ int mmu_idx,
+ uintptr_t retaddr);
#endif
#include "exec/cpu-all.h"
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 15bad5dd46..417b691d8d 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -558,6 +558,17 @@ raise_exception_format2(CPUM68KState *env, int tt, int
ilen, uintptr_t raddr)
cpu_loop_exit(cs);
}
+#if !defined(CONFIG_USER_ONLY)
+G_NORETURN void m68k_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
+ MMUAccessType access_type,
+ int mmu_idx, uintptr_t retaddr)
+{
+CPUM68KState *env = cpu_env(cs);
+
+raise_exception(env, EXCP_ADDRESS);
+}
+#endif
+
void HELPER(divuw)(CPUM68KState *env, int destr, uint32_t den, int ilen)
{
uint32_t num = env->dregs[destr];
--
2.39.2
[PATCH 0/2] target/m68k: implement unaligned accesses for m68k CPUs
This series implements unaligned accesses for early m68k CPUs that do not support word/long accesses at byte granularity. Patch 1 implements the do_unaligned_access function for m68k CPUs, whilst patch 2 is based upon a prototype patch developed by Laurent as part of Gitlab Issue #2165. Signed-off-by: Mark Cave-Ayland Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2165 Mark Cave-Ayland (2): target/m68k: implement do_unaligned_access callback for m68k CPUs target/m68k: pass alignment into TCG memory load/store routines target/m68k/cpu.c | 1 + target/m68k/cpu.h | 4 target/m68k/op_helper.c | 11 +++ target/m68k/translate.c | 18 +++--- 4 files changed, 31 insertions(+), 3 deletions(-) -- 2.39.2
[PATCH 2/2] target/m68k: pass alignment into TCG memory load/store routines
Now that do_unaligned_access has been implemented for 68k CPUs, pass the
required
alignment into the TCG memory load/store routines. This allows the TCG memory
core
to generate an Address Error exception for unaligned memory accesses if
required.
Suggested-by: Laurent Vivier
Signed-off-by: Mark Cave-Ayland
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2165
---
target/m68k/translate.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 445966fb6a..661a7b4def 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -303,13 +303,18 @@ static inline TCGv gen_load(DisasContext *s, int opsize,
TCGv addr,
int sign, int index)
{
TCGv tmp = tcg_temp_new_i32();
+MemOp memop = opsize | (sign ? MO_SIGN : 0) | MO_TE;
switch (opsize) {
case OS_BYTE:
+tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
+break;
case OS_WORD:
case OS_LONG:
-tcg_gen_qemu_ld_tl(tmp, addr, index,
- opsize | (sign ? MO_SIGN : 0) | MO_TE);
+if (!m68k_feature(s->env, M68K_FEATURE_UNALIGNED_DATA)) {
+memop |= MO_ALIGN_2;
+}
+tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
break;
default:
g_assert_not_reached();
@@ -321,11 +326,18 @@ static inline TCGv gen_load(DisasContext *s, int opsize,
TCGv addr,
static inline void gen_store(DisasContext *s, int opsize, TCGv addr, TCGv val,
int index)
{
+MemOp memop = opsize | MO_TE;
+
switch (opsize) {
case OS_BYTE:
+tcg_gen_qemu_st_tl(val, addr, index, memop);
+break;
case OS_WORD:
case OS_LONG:
-tcg_gen_qemu_st_tl(val, addr, index, opsize | MO_TE);
+if (!m68k_feature(s->env, M68K_FEATURE_UNALIGNED_DATA)) {
+memop |= MO_ALIGN_2;
+}
+tcg_gen_qemu_st_tl(val, addr, index, memop);
break;
default:
g_assert_not_reached();
--
2.39.2
Re: Examining device state via monitor for debugging
On 11/06/2024 08:57, Daniel P. Berrangé wrote: On Tue, Jun 11, 2024 at 07:49:12AM +0200, Markus Armbruster wrote: Philippe Mathieu-Daudé writes: Officialise the QMP command, use the existing hmp_info_human_readable_text() helper. I'm not sure "officialise" is a word :) Taking a step back... "info via" and its new QMP counterpart x-query-mos6522-devices dump device state. I understand why examining device state via monitor can be useful for debugging. However, we have more than 2000 devices in the tree. Clearly, we don't want 2000 device state queries. Not even 100. Could we have more generic means instead? We could use QOM (read-only) properties to expose device state. If we use one QOM property per "thing", examining device state becomes quite tedious. Also, you'd have to stop the guest to get a consistent view, and adding lots of QOM properties bloats the code. If we use a single, object-valued property for the entire state, we get to define the objects in QAPI. Differently tedious, and bloats the generated code. We could use a single string-valued property. Too much of an abuse of QOM? Yeah, I'd suggest we just keep it dumb and free form, adding a callback like this to the QOM base class: HumanReadableText (*debug_state)(Error **errp); I think that's a little bit too restrictive: certainly I can see the need for allowing parameters for the output to be customised, and there may also be cases where a device may want to register more than one debug (or monitor) command. Currently I quite like Manos' solution since it allows a MonitorInterface to be defined, and that could have separate methods for registering both "info" and "debug" commands. Longer term this could allow for e.g. TYPE_TCG_ACCEL to register "info jit" and devices can add whatever debug monitor commands they need. ATB, Mark.
Re: Examining device state via monitor for debugging
On 11/06/2024 07:58, Manos Pitsidianakis wrote: On Tue, 11 Jun 2024 at 09:11, Mark Cave-Ayland wrote: On 11/06/2024 06:49, Markus Armbruster wrote: Philippe Mathieu-Daudé writes: Officialise the QMP command, use the existing hmp_info_human_readable_text() helper. I'm not sure "officialise" is a word :) Taking a step back... "info via" and its new QMP counterpart x-query-mos6522-devices dump device state. I understand why examining device state via monitor can be useful for debugging. However, we have more than 2000 devices in the tree. Clearly, we don't want 2000 device state queries. Not even 100. Could we have more generic means instead? We could use QOM (read-only) properties to expose device state. If we use one QOM property per "thing", examining device state becomes quite tedious. Also, you'd have to stop the guest to get a consistent view, and adding lots of QOM properties bloats the code. If we use a single, object-valued property for the entire state, we get to define the objects in QAPI. Differently tedious, and bloats the generated code. We could use a single string-valued property. Too much of an abuse of QOM? We could add an optional "dump state for debugging" method to QOM, and have a single query command that calls it if present. Thoughts? I agree that there should be a better way of doing things here. The aim of the original "info via" series was to allow the command to be contained completely within mos6522.c, but unfortunately due to the way that qemu-options.hx works then you end up with #ifdef-fery or stubs to make all configuration combinations work. As you point out ideally there should be a way for a QOM object to dynamically register its own monitor commands, which I think should help with this. IIRC in the original thread Daniel or David proposed a new "debug" monitor command such that a device could register its own debug commands either via DeviceClass or a function called during realize that would return a HumanReadableText via QMP. This is starting to sound like OOP: A Monitor interface defines monitor commands, and QOM type classes can implement/define their own. A Debug interface would do this. I like this idea: having a QOM interface that objects can implement to register their own monitor commands feels like a good solution. ATB, Mark.
Re: Examining device state via monitor for debugging
On 11/06/2024 06:49, Markus Armbruster wrote: Philippe Mathieu-Daudé writes: Officialise the QMP command, use the existing hmp_info_human_readable_text() helper. I'm not sure "officialise" is a word :) Taking a step back... "info via" and its new QMP counterpart x-query-mos6522-devices dump device state. I understand why examining device state via monitor can be useful for debugging. However, we have more than 2000 devices in the tree. Clearly, we don't want 2000 device state queries. Not even 100. Could we have more generic means instead? We could use QOM (read-only) properties to expose device state. If we use one QOM property per "thing", examining device state becomes quite tedious. Also, you'd have to stop the guest to get a consistent view, and adding lots of QOM properties bloats the code. If we use a single, object-valued property for the entire state, we get to define the objects in QAPI. Differently tedious, and bloats the generated code. We could use a single string-valued property. Too much of an abuse of QOM? We could add an optional "dump state for debugging" method to QOM, and have a single query command that calls it if present. Thoughts? I agree that there should be a better way of doing things here. The aim of the original "info via" series was to allow the command to be contained completely within mos6522.c, but unfortunately due to the way that qemu-options.hx works then you end up with #ifdef-fery or stubs to make all configuration combinations work. As you point out ideally there should be a way for a QOM object to dynamically register its own monitor commands, which I think should help with this. IIRC in the original thread Daniel or David proposed a new "debug" monitor command such that a device could register its own debug commands either via DeviceClass or a function called during realize that would return a HumanReadableText via QMP. In terms of "info via" it is only used by developers for the 68k and PPC Mac machines so if it were to change from "info via" to "debug via" I don't see there would be a big problem with this. ATB, Mark.
Re: [PATCH 2/4] target/i386: use gen_writeback() within gen_POP()
On 06/06/2024 10:53, Mark Cave-Ayland wrote:
Instead of directly implementing the writeback using gen_op_st_v(), use the
existing gen_writeback() function.
Suggested-by: Paolo Bonzini
Signed-off-by: Mark Cave-Ayland
---
target/i386/tcg/emit.c.inc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index a89f8e0ebb..2d5dc11548 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -2569,7 +2569,7 @@ static void gen_POP(DisasContext *s, CPUX86State *env,
X86DecodedInsn *decode)
if (op->has_ea) {
/* NOTE: order is important for MMU exceptions */
-gen_op_st_v(s, ot, s->T0, s->A0);
+gen_writeback(s, decode, 0, s->T0);
op->unit = X86_OP_SKIP;
}
/* NOTE: writing back registers after update is important for pop %sp */
Hi Paolo,
I've just noticed that gen_writeback() also sets op->unit to X86_OP_SKIP at the end,
so in fact the line below it explicitly setting op->unit can also be removed. Do you
need me to send a v2 with this change included, or is it possible to touch it up
before commit?
ATB,
Mark.
Re: [PATCH] target/sparc: use signed denominator in sdiv helper
On 06/06/2024 15:43, Clément Chigot wrote:
The result has to be done with the signed denominator (b32) instead of
the unsigned value passed in argument (b).
Fixes: 1326010322d6 ("target/sparc: Remove CC_OP_DIV")
Signed-off-by: Clément Chigot
---
target/sparc/helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/sparc/helper.c b/target/sparc/helper.c
index 2247e243b5..7846ddd6f6 100644
--- a/target/sparc/helper.c
+++ b/target/sparc/helper.c
@@ -121,7 +121,7 @@ uint64_t helper_sdiv(CPUSPARCState *env, target_ulong a,
target_ulong b)
return (uint32_t)(b32 < 0 ? INT32_MAX : INT32_MIN) | (-1ull << 32);
}
-a64 /= b;
+a64 /= b32;
r = a64;
if (unlikely(r != a64)) {
return (uint32_t)(a64 < 0 ? INT32_MIN : INT32_MAX) | (-1ull << 32);
Thanks for the patch! I think this might also be:
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2319
ATB,
Mark.
[PATCH 0/4] target/i386: fixes for OS/2 Warp
This series contains two fixes which allow booting OS/2 Warp in QEMU with TCG (currently it only boots with KVM). Patches 1 and 2 are tidy-ups which prepare for the POP SP fix in patch 3, whilst patch 4 is the final fix for ENTER that allows my test image to boot successfully. Signed-off-by: Mark Cave-Ayland Mark Cave-Ayland (4): target/i386: use local X86DecodedOp in gen_POP() target/i386: use gen_writeback() within gen_POP() target/i386: fix SP when taking a memory fault during POP target/i386: fix size of EBP writeback in gen_enter() target/i386/tcg/emit.c.inc | 8 +--- target/i386/tcg/translate.c | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) -- 2.39.2
[PATCH 4/4] target/i386: fix size of EBP writeback in gen_enter()
The calculation of FrameTemp is done using the size indicated by mo_pushpop() before being written back to EBP, but the final writeback to EBP is done using the size indicated by mo_stacksize(). In the case where mo_pushpop() is MO_32 and mo_stacksize() is MO_16 then the final writeback to EBP is done using MO_16 which can leave junk in the top 16-bits of EBP after executing ENTER. Change the writeback of EBP to use the same size indicated by mo_pushpop() to ensure that the full value is written back. Signed-off-by: Mark Cave-Ayland Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2198 --- target/i386/tcg/translate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c index 0486ab6911..0716ca35d5 100644 --- a/target/i386/tcg/translate.c +++ b/target/i386/tcg/translate.c @@ -2125,7 +2125,7 @@ static void gen_enter(DisasContext *s, int esp_addend, int level) } /* Copy the FrameTemp value to EBP. */ -gen_op_mov_reg_v(s, a_ot, R_EBP, s->T1); +gen_op_mov_reg_v(s, d_ot, R_EBP, s->T1); /* Compute the final value of ESP. */ tcg_gen_subi_tl(s->T1, s->T1, esp_addend + size * level); -- 2.39.2
[PATCH 2/4] target/i386: use gen_writeback() within gen_POP()
Instead of directly implementing the writeback using gen_op_st_v(), use the
existing gen_writeback() function.
Suggested-by: Paolo Bonzini
Signed-off-by: Mark Cave-Ayland
---
target/i386/tcg/emit.c.inc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index a89f8e0ebb..2d5dc11548 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -2569,7 +2569,7 @@ static void gen_POP(DisasContext *s, CPUX86State *env,
X86DecodedInsn *decode)
if (op->has_ea) {
/* NOTE: order is important for MMU exceptions */
-gen_op_st_v(s, ot, s->T0, s->A0);
+gen_writeback(s, decode, 0, s->T0);
op->unit = X86_OP_SKIP;
}
/* NOTE: writing back registers after update is important for pop %sp */
--
2.39.2
[PATCH 3/4] target/i386: fix SP when taking a memory fault during POP
When OS/2 Warp configures its segment descriptors, many of them are configured
with
the P flag clear to allow for a fault-on-demand implementation. In the case
where
the stack value is POPped into the segment registers, the SP is incremented
before
calling gen_helper_load_seg() to validate the segment descriptor:
IN:
0xffef2c0c: 66 07popl %es
OP:
ld_i32 loc9,env,$0xfff8
sub_i32 loc9,loc9,$0x1
brcond_i32 loc9,$0x0,lt,$L0
st16_i32 loc9,env,$0xfff8
st8_i32 $0x1,env,$0xfffc
0c0c
ext16u_i64 loc0,rsp
add_i64 loc0,loc0,ss_base
ext32u_i64 loc0,loc0
qemu_ld_a64_i64 loc0,loc0,noat+un+leul,5
add_i64 loc3,rsp,$0x4
deposit_i64 rsp,rsp,loc3,$0x0,$0x10
extrl_i64_i32 loc5,loc0
call load_seg,$0x0,$0,env,$0x0,loc5
add_i64 rip,rip,$0x2
ext16u_i64 rip,rip
exit_tb $0x0
set_label $L0
exit_tb $0x7fff5843
If helper_load_seg() generates a fault when validating the segment descriptor
then as
the SP has already been incremented, the topmost word of the stack is
overwritten by
the arguments pushed onto the stack by the CPU before taking the fault handler.
As a
consequence things rapidly go wrong upon return from the fault handler due to
the
corrupted stack.
Update the logic for the existing writeback condition so that a POP into the
segment
registers also calls helper_load_seg() first before incrementing the SP, so
that if a
fault occurs the SP remains unaltered.
Signed-off-by: Mark Cave-Ayland
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2198
---
target/i386/tcg/emit.c.inc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index 2d5dc11548..f905a67380 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -2567,7 +2567,7 @@ static void gen_POP(DisasContext *s, CPUX86State *env,
X86DecodedInsn *decode)
X86DecodedOp *op = >op[0];
MemOp ot = gen_pop_T0(s);
-if (op->has_ea) {
+if (op->has_ea || op->unit == X86_OP_SEG) {
/* NOTE: order is important for MMU exceptions */
gen_writeback(s, decode, 0, s->T0);
op->unit = X86_OP_SKIP;
--
2.39.2
[PATCH 1/4] target/i386: use local X86DecodedOp in gen_POP()
This will make subsequent changes a little easier to read.
Signed-off-by: Mark Cave-Ayland
---
target/i386/tcg/emit.c.inc | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index e990141454..a89f8e0ebb 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -2564,11 +2564,13 @@ static void gen_PMOVMSKB(DisasContext *s, CPUX86State
*env, X86DecodedInsn *deco
static void gen_POP(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
{
+X86DecodedOp *op = >op[0];
MemOp ot = gen_pop_T0(s);
-if (decode->op[0].has_ea) {
+
+if (op->has_ea) {
/* NOTE: order is important for MMU exceptions */
gen_op_st_v(s, ot, s->T0, s->A0);
-decode->op[0].unit = X86_OP_SKIP;
+op->unit = X86_OP_SKIP;
}
/* NOTE: writing back registers after update is important for pop %sp */
gen_pop_update(s, ot);
--
2.39.2
Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default
On 03/06/2024 12:40, Daniel P. Berrangé wrote:
On Thu, May 30, 2024 at 01:22:11PM +0100, Mark Cave-Ayland wrote:
On 30/05/2024 12:40, BALATON Zoltan wrote:
On Thu, 30 May 2024, Gerd Hoffmann wrote:
stdvga is the much better option.
Signed-off-by: Gerd Hoffmann
---
hw/display/cirrus_vga.c | 1 +
hw/display/cirrus_vga_isa.c | 1 +
hw/display/Kconfig | 1 -
3 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 150883a97166..81421be1f89d 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -3007,6 +3007,7 @@ static void cirrus_vga_class_init(ObjectClass
*klass, void *data)
dc->vmsd = _pci_cirrus_vga;
device_class_set_props(dc, pci_vga_cirrus_properties);
dc->hotpluggable = false;
+ klass->deprecation_note = "use stdvga instead";
}
static const TypeInfo cirrus_vga_info = {
diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
index 84be51670ed8..3abbf490 100644
--- a/hw/display/cirrus_vga_isa.c
+++ b/hw/display/cirrus_vga_isa.c
@@ -85,6 +85,7 @@ static void isa_cirrus_vga_class_init(ObjectClass
*klass, void *data)
dc->realize = isa_cirrus_vga_realizefn;
device_class_set_props(dc, isa_cirrus_vga_properties);
set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+ klass->deprecation_note = "use stdvga instead";
Excepr some old OSes work better with this than stdvga so could this be
left and not removed? Does it cause a lot of work to keep this device? I
thought it's stable already and were not many changes for it lately. If
something works why drop it?
Seconded: whilst stdvga is preferred, there are a lot of older OSs that work
well in QEMU using the Cirrus emulation. I appreciate that the code could do
with a bit of work, but is there a more specific reason that it should be
deprecated?
I think there's different answers here for upstream vs downstream.
Upstream QEMU's scope is to emulate pretty much arbitrary hardware that
may have existed at any point in time. Emulating Cirrus is very much
in scope upstream, and even if there are other better VGA devices, that
doesn't make emulation of Cirrus redundant.
Downstream is a different matter - if a downstream vendor wants to be
opinionated and limit the scope of devices they ship to customers, it
is totally valid to cull Cirrus.
The concern for me is that if someone such as RedHat decided not to ship Cirrus then
we'd end up in a place where command lines for some legacy OSs would work on an
upstream build, but if someone was using RHEL then they would fail due to the device
not being present. I can see this causing confusion for users since they would report
that a command line doesn't work whilst others would shrug and report back that it
works for them.
I do wonder if a solution for this would be to add a blocklist file in /etc that
prevents the listed QOM types from being instantiated. The file could contain also
contain a machine regex to match and a reason that can be reported to the user e.g.
# QEMU QOM type blocklist
#
# QOM type regex, machine regex list, reason
"cirrus*","pc*,machine*","contains insecure blitter routines"
"fdc","pc*","may not be completely secure"
This feels like a better solution because:
- It's easy to add a message that reports "The requested QOM type cannot be
instantiated because " for specific machine types. The machine regex also
fixes the problem where usb-ohci should be allowed for PPC machines, but not
generally for PC machines.
- Downstream can ship with a secure policy for production environments but also a
less restrictive policy for more casual users
- If someone really needs a device that is not enabled by default, a privileged user
can simply edit the blocklist file and allow it
- It should work both with or without modules
ATB,
Mark.
Re: [PATCH v2 00/37] target/sparc: Implement VIS4
On 28/05/2024 22:29, Mark Cave-Ayland wrote: On 26/05/2024 20:42, Richard Henderson wrote: Now tested with RISU, using a Solaris M8 host as reference. This exposed a few bugs in the existing VIS1 support as well, so fix those before anything else. It also exposed a few bugs in the implementation of VIS3, so fixes squashed there as well. r~ Richard Henderson (37): target/sparc: Fix ARRAY8 target/sparc: Rewrite gen_edge target/sparc: Fix do_dc target/sparc: Fix helper_fmul8ulx16 target/sparc: Perform DFPREG/QFPREG in decodetree target/sparc: Remove gen_dest_fpr_D target/sparc: Remove cpu_fpr[] target/sparc: Use gvec for VIS1 parallel add/sub target/sparc: Implement FMAf extension target/sparc: Add feature bits for VIS 3 target/sparc: Implement ADDXC, ADDXCcc target/sparc: Implement CMASK instructions target/sparc: Implement FCHKSM16 target/sparc: Implement FHADD, FHSUB, FNHADD, FNADD, FNMUL target/sparc: Implement FLCMP target/sparc: Implement FMEAN16 target/sparc: Implement FPADD64, FPSUB64 target/sparc: Implement FPADDS, FPSUBS target/sparc: Implement FPCMPEQ8, FPCMPNE8, FPCMPULE8, FPCMPUGT8 target/sparc: Implement FSLL, FSRL, FSRA, FSLAS target/sparc: Implement LDXEFSR target/sparc: Implement LZCNT target/sparc: Implement MOVsTOw, MOVdTOx, MOVwTOs, MOVxTOd target/sparc: Implement PDISTN target/sparc: Implement UMULXHI target/sparc: Implement XMULX target/sparc: Enable VIS3 feature bit target/sparc: Implement IMA extension target/sparc: Add feature bit for VIS4 target/sparc: Implement FALIGNDATAi target/sparc: Implement 8-bit FPADD, FPADDS, and FPADDUS target/sparc: Implement VIS4 comparisons target/sparc: Implement FPMIN, FPMAX target/sparc: Implement SUBXC, SUBXCcc target/sparc: Implement MWAIT target/sparc: Implement monitor ASIs target/sparc: Enable VIS4 feature bit target/sparc/asi.h | 4 + target/sparc/helper.h | 27 +- target/sparc/cpu-feature.h.inc | 4 + target/sparc/insns.decode | 338 linux-user/elfload.c | 3 + target/sparc/cpu.c | 12 + target/sparc/fop_helper.c | 136 + target/sparc/ldst_helper.c | 4 + target/sparc/translate.c | 938 ++--- target/sparc/vis_helper.c | 392 +++--- fpu/softfloat-specialize.c.inc | 31 ++ 11 files changed, 1558 insertions(+), 331 deletions(-) Thanks - I'll give this series a quick run over my test images over the next few days just to make sure there are no regressions (unlikely as I don't have much in the way of current VIS test cases) and report back. This series passes all my sun4u boot tests so: Tested-by: Mark Cave-Ayland I don't have any other pending patches so feel free to take this via your TCG branch if you would like it merged in the near future. ATB, Mark.
Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default
On 30/05/2024 12:40, BALATON Zoltan wrote:
On Thu, 30 May 2024, Gerd Hoffmann wrote:
stdvga is the much better option.
Signed-off-by: Gerd Hoffmann
---
hw/display/cirrus_vga.c | 1 +
hw/display/cirrus_vga_isa.c | 1 +
hw/display/Kconfig | 1 -
3 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 150883a97166..81421be1f89d 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -3007,6 +3007,7 @@ static void cirrus_vga_class_init(ObjectClass *klass, void
*data)
dc->vmsd = _pci_cirrus_vga;
device_class_set_props(dc, pci_vga_cirrus_properties);
dc->hotpluggable = false;
+ klass->deprecation_note = "use stdvga instead";
}
static const TypeInfo cirrus_vga_info = {
diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
index 84be51670ed8..3abbf490 100644
--- a/hw/display/cirrus_vga_isa.c
+++ b/hw/display/cirrus_vga_isa.c
@@ -85,6 +85,7 @@ static void isa_cirrus_vga_class_init(ObjectClass *klass, void
*data)
dc->realize = isa_cirrus_vga_realizefn;
device_class_set_props(dc, isa_cirrus_vga_properties);
set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+ klass->deprecation_note = "use stdvga instead";
Excepr some old OSes work better with this than stdvga so could this be left and not
removed? Does it cause a lot of work to keep this device? I thought it's stable
already and were not many changes for it lately. If something works why drop it?
Seconded: whilst stdvga is preferred, there are a lot of older OSs that work well in
QEMU using the Cirrus emulation. I appreciate that the code could do with a bit of
work, but is there a more specific reason that it should be deprecated?
ATB,
Mark.
Re: [PATCH 3/4] usb/ohci-pci: deprecate, don't build by default
On 28/05/2024 11:35, Thomas Huth wrote:
On 28/05/2024 11.54, Gerd Hoffmann wrote:
The xhci host adapter is the much better choice.
Signed-off-by: Gerd Hoffmann
---
hw/usb/hcd-ohci-pci.c | 1 +
hw/usb/Kconfig | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/usb/hcd-ohci-pci.c b/hw/usb/hcd-ohci-pci.c
index 33ed9b6f5a52..88de657def71 100644
--- a/hw/usb/hcd-ohci-pci.c
+++ b/hw/usb/hcd-ohci-pci.c
@@ -143,6 +143,7 @@ static void ohci_pci_class_init(ObjectClass *klass, void
*data)
dc->hotpluggable = false;
dc->vmsd = _ohci;
dc->reset = usb_ohci_reset_pci;
+ klass->deprecation_note = "use qemu-xhci instead";
}
static const TypeInfo ohci_pci_info = {
diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index 84bc7fbe36cd..c4a6ea5a687f 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -17,7 +17,6 @@ config USB_OHCI_SYSBUS
config USB_OHCI_PCI
bool
- default y if PCI_DEVICES
depends on PCI
select USB_OHCI
Not sure whether we should disable it by default just because it is deprecated. We
don't do that for any other devices as far as I know.
Anyway, you should add the device to docs/about/deprecated.rst to really mark it as
deprecated, since that's our official list (AFAIK).
Also, there are still some machines that use this device:
$ grep -r USB_OHCI_PCI *
hw/hppa/Kconfig: imply USB_OHCI_PCI
hw/mips/Kconfig: imply USB_OHCI_PCI
hw/ppc/Kconfig: imply USB_OHCI_PCI
hw/ppc/Kconfig: imply USB_OHCI_PCI
pseries could certainly continue without OHCI AFAICT, but the others? Maybe this
needs some discussion first... (thus putting some more people on CC:)
Thomas
The mac99 machine has an in-built OHCI PCI interface so I don't think this device
should be marked as deprecated. Normally in these cases isn't it just a matter of
updating documentation to recommend XHCI over OHCI for particular uses?
ATB,
Mark.
Re: [PATCH v2 00/37] target/sparc: Implement VIS4
On 26/05/2024 20:42, Richard Henderson wrote: Now tested with RISU, using a Solaris M8 host as reference. This exposed a few bugs in the existing VIS1 support as well, so fix those before anything else. It also exposed a few bugs in the implementation of VIS3, so fixes squashed there as well. r~ Richard Henderson (37): target/sparc: Fix ARRAY8 target/sparc: Rewrite gen_edge target/sparc: Fix do_dc target/sparc: Fix helper_fmul8ulx16 target/sparc: Perform DFPREG/QFPREG in decodetree target/sparc: Remove gen_dest_fpr_D target/sparc: Remove cpu_fpr[] target/sparc: Use gvec for VIS1 parallel add/sub target/sparc: Implement FMAf extension target/sparc: Add feature bits for VIS 3 target/sparc: Implement ADDXC, ADDXCcc target/sparc: Implement CMASK instructions target/sparc: Implement FCHKSM16 target/sparc: Implement FHADD, FHSUB, FNHADD, FNADD, FNMUL target/sparc: Implement FLCMP target/sparc: Implement FMEAN16 target/sparc: Implement FPADD64, FPSUB64 target/sparc: Implement FPADDS, FPSUBS target/sparc: Implement FPCMPEQ8, FPCMPNE8, FPCMPULE8, FPCMPUGT8 target/sparc: Implement FSLL, FSRL, FSRA, FSLAS target/sparc: Implement LDXEFSR target/sparc: Implement LZCNT target/sparc: Implement MOVsTOw, MOVdTOx, MOVwTOs, MOVxTOd target/sparc: Implement PDISTN target/sparc: Implement UMULXHI target/sparc: Implement XMULX target/sparc: Enable VIS3 feature bit target/sparc: Implement IMA extension target/sparc: Add feature bit for VIS4 target/sparc: Implement FALIGNDATAi target/sparc: Implement 8-bit FPADD, FPADDS, and FPADDUS target/sparc: Implement VIS4 comparisons target/sparc: Implement FPMIN, FPMAX target/sparc: Implement SUBXC, SUBXCcc target/sparc: Implement MWAIT target/sparc: Implement monitor ASIs target/sparc: Enable VIS4 feature bit target/sparc/asi.h | 4 + target/sparc/helper.h | 27 +- target/sparc/cpu-feature.h.inc | 4 + target/sparc/insns.decode | 338 linux-user/elfload.c | 3 + target/sparc/cpu.c | 12 + target/sparc/fop_helper.c | 136 + target/sparc/ldst_helper.c | 4 + target/sparc/translate.c | 938 ++--- target/sparc/vis_helper.c | 392 +++--- fpu/softfloat-specialize.c.inc | 31 ++ 11 files changed, 1558 insertions(+), 331 deletions(-) Thanks - I'll give this series a quick run over my test images over the next few days just to make sure there are no regressions (unlikely as I don't have much in the way of current VIS test cases) and report back. ATB, Mark.
Re: [PATCH RISU v2 00/13] ELF and Sparc64 support
On 26/05/2024 20:36, Richard Henderson wrote: Let risu accept elf test files, adjusted from v1. Adjust risugen to invoke the assembler and linker, with a cross-compiler prefix if needed. Add some sparc64 testing which utilizes this. Changes for v2: - Implement VIS2 through VIS4. There's something odd going on with the Sparc M8 Solaris host where the values recorded via RISU for some floating-point operations are incorrectly rounded, but performing the same operations with the same inputs in a standalone test program produces correct results. I wonder if there's some unfinished_FPop exception being generated and the operating system emulation is producing incorrect results. I'd be much happier if I could test this on Linux... r~ Richard Henderson (13): risu: Allow use of ELF test files Build elf test cases instead of raw binaries Introduce host_context_t risu: Add initial sparc64 support risugen: Be explicit about print destinations risugen: Add sparc64 support contrib/generate_all: Do not rely on ag sparc64: Add a few logical insns sparc64: Add VIS1 instructions sparc64: Add VIS2 and FMAF insns sparc64: Add VIS3 instructions sparc64: Add IMA instructions sparc64: Add VIS4 instructions Makefile | 22 ++- risu.h | 16 +- risu_reginfo_aarch64.h | 2 + risu_reginfo_arm.h | 2 + risu_reginfo_i386.h| 2 + risu_reginfo_loongarch64.h | 3 + risu_reginfo_m68k.h| 2 + risu_reginfo_ppc64.h | 2 + risu_reginfo_s390x.h | 2 + risu_reginfo_sparc64.h | 36 risu.c | 59 +- risu_aarch64.c | 6 +- risu_arm.c | 7 +- risu_i386.c| 7 +- risu_loongarch64.c | 6 +- risu_m68k.c| 6 +- risu_ppc64.c | 6 +- risu_reginfo_loongarch64.c | 3 +- risu_reginfo_sparc64.c | 186 ++ risu_s390x.c | 5 +- risu_sparc64.c | 52 + configure | 2 + contrib/generate_all.sh| 4 +- risugen| 10 +- risugen_common.pm | 68 ++- risugen_sparc64.pm | 385 + sparc64.risu | 298 test.ld| 12 ++ test_aarch64.s | 4 +- test_arm.s | 16 +- test_i386.S| 4 +- test_sparc64.s | 137 + 32 files changed, 1298 insertions(+), 74 deletions(-) create mode 100644 risu_reginfo_sparc64.h create mode 100644 risu_reginfo_sparc64.c create mode 100644 risu_sparc64.c create mode 100644 risugen_sparc64.pm create mode 100644 sparc64.risu create mode 100644 test.ld create mode 100644 test_sparc64.s Nice! I don't have any experience with RISU so I don't feel too qualified to review the series, but obviously there are clear benefits to having SPARC support included :) ATB, Mark.
Re: [PATCH 00/41] target/sparc: Implement VIS4
On 15/05/2024 16:30, Richard Henderson wrote: On 4/29/24 23:02, Richard Henderson wrote: On 4/29/24 13:52, Mark Cave-Ayland wrote: No objections here about the remainder of the series, other than that I don't have an easy/obvious way to test the new instructions... I was thinking about adding support to RISU, but the gcc compile farm sparc machines have been down for ages, so no way to generate the reference traces. Update: I have successfully ported RISU to Sparc64, Solaris and Linux. There is a limitation in that I cannot find how to extract %gsr from the signal frame, which is unfortunate, but I can work around that for now. I have added descriptions of VIS1 instructions to RISU, and it turns out we have failures relative to a Sparc M8. I have not yet analyzed these failures, but it proves the effort was not wasted. :-) I'll clean up these patches and post them here when I next get some downtime. r~ That's great news, thanks for the update. I've had confirmation that there is work underway to repair the SPARC hardware hosting Linux for the gcc buildfarm, so hopefully it will be back in service soon. ATB, Mark.
[PULL 10/12] target/sparc: Fix FMULD8*X16
From: Richard Henderson
Not only do these instructions have f32 inputs, they also do not
perform rounding. Since these are relatively simple, implement
them properly inline.
Signed-off-by: Richard Henderson
Message-Id: <20240502165528.244004-6-richard.hender...@linaro.org>
Signed-off-by: Mark Cave-Ayland
---
target/sparc/helper.h | 2 --
target/sparc/translate.c | 48 +++
target/sparc/vis_helper.c | 46 -
3 files changed, 44 insertions(+), 52 deletions(-)
diff --git a/target/sparc/helper.h b/target/sparc/helper.h
index 9cde2b69a5..fcb9c617b7 100644
--- a/target/sparc/helper.h
+++ b/target/sparc/helper.h
@@ -99,8 +99,6 @@ DEF_HELPER_FLAGS_2(fmul8x16, TCG_CALL_NO_RWG_SE, i64, i32,
i64)
DEF_HELPER_FLAGS_2(fmul8x16a, TCG_CALL_NO_RWG_SE, i64, i32, s32)
DEF_HELPER_FLAGS_2(fmul8sux16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_2(fmul8ulx16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
-DEF_HELPER_FLAGS_2(fmuld8sux16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
-DEF_HELPER_FLAGS_2(fmuld8ulx16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_1(fexpand, TCG_CALL_NO_RWG_SE, i64, i32)
DEF_HELPER_FLAGS_3(pdist, TCG_CALL_NO_RWG_SE, i64, i64, i64, i64)
DEF_HELPER_FLAGS_2(fpack16, TCG_CALL_NO_RWG_SE, i32, i64, i64)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index a8ada6934a..8a2894bb9f 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -74,8 +74,6 @@
# define gen_helper_fmul8sux16 ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmul8ulx16 ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmul8x16 ({ qemu_build_not_reached(); NULL; })
-# define gen_helper_fmuld8sux16 ({ qemu_build_not_reached(); NULL; })
-# define gen_helper_fmuld8ulx16 ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fpmerge ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fqtox({ qemu_build_not_reached(); NULL; })
# define gen_helper_fstox({ qemu_build_not_reached(); NULL; })
@@ -730,6 +728,48 @@ static void gen_op_fmul8x16au(TCGv_i64 dst, TCGv_i32 src1,
TCGv_i32 src2)
gen_helper_fmul8x16a(dst, src1, src2);
}
+static void gen_op_fmuld8ulx16(TCGv_i64 dst, TCGv_i32 src1, TCGv_i32 src2)
+{
+TCGv_i32 t0 = tcg_temp_new_i32();
+TCGv_i32 t1 = tcg_temp_new_i32();
+TCGv_i32 t2 = tcg_temp_new_i32();
+
+tcg_gen_ext8u_i32(t0, src1);
+tcg_gen_ext16s_i32(t1, src2);
+tcg_gen_mul_i32(t0, t0, t1);
+
+tcg_gen_extract_i32(t1, src1, 16, 8);
+tcg_gen_sextract_i32(t2, src2, 16, 16);
+tcg_gen_mul_i32(t1, t1, t2);
+
+tcg_gen_concat_i32_i64(dst, t0, t1);
+}
+
+static void gen_op_fmuld8sux16(TCGv_i64 dst, TCGv_i32 src1, TCGv_i32 src2)
+{
+TCGv_i32 t0 = tcg_temp_new_i32();
+TCGv_i32 t1 = tcg_temp_new_i32();
+TCGv_i32 t2 = tcg_temp_new_i32();
+
+/*
+ * The insn description talks about extracting the upper 8 bits
+ * of the signed 16-bit input rs1, performing the multiply, then
+ * shifting left by 8 bits. Instead, zap the lower 8 bits of
+ * the rs1 input, which avoids the need for two shifts.
+ */
+tcg_gen_ext16s_i32(t0, src1);
+tcg_gen_andi_i32(t0, t0, ~0xff);
+tcg_gen_ext16s_i32(t1, src2);
+tcg_gen_mul_i32(t0, t0, t1);
+
+tcg_gen_sextract_i32(t1, src1, 16, 16);
+tcg_gen_andi_i32(t1, t1, ~0xff);
+tcg_gen_sextract_i32(t2, src2, 16, 16);
+tcg_gen_mul_i32(t1, t1, t2);
+
+tcg_gen_concat_i32_i64(dst, t0, t1);
+}
+
static void finishing_insn(DisasContext *dc)
{
/*
@@ -4614,6 +4654,8 @@ static bool do_dff(DisasContext *dc, arg_r_r_r *a,
TRANS(FMUL8x16AU, VIS1, do_dff, a, gen_op_fmul8x16au)
TRANS(FMUL8x16AL, VIS1, do_dff, a, gen_op_fmul8x16al)
+TRANS(FMULD8SUx16, VIS1, do_dff, a, gen_op_fmuld8sux16)
+TRANS(FMULD8ULx16, VIS1, do_dff, a, gen_op_fmuld8ulx16)
static bool do_dfd(DisasContext *dc, arg_r_r_r *a,
void (*func)(TCGv_i64, TCGv_i32, TCGv_i64))
@@ -4654,8 +4696,6 @@ static bool do_ddd(DisasContext *dc, arg_r_r_r *a,
TRANS(FMUL8SUx16, VIS1, do_ddd, a, gen_helper_fmul8sux16)
TRANS(FMUL8ULx16, VIS1, do_ddd, a, gen_helper_fmul8ulx16)
-TRANS(FMULD8SUx16, VIS1, do_ddd, a, gen_helper_fmuld8sux16)
-TRANS(FMULD8ULx16, VIS1, do_ddd, a, gen_helper_fmuld8ulx16)
TRANS(FPMERGE, VIS1, do_ddd, a, gen_helper_fpmerge)
TRANS(FPADD16, VIS1, do_ddd, a, tcg_gen_vec_add16_i64)
diff --git a/target/sparc/vis_helper.c b/target/sparc/vis_helper.c
index ff2f43c23f..61c61c7fea 100644
--- a/target/sparc/vis_helper.c
+++ b/target/sparc/vis_helper.c
@@ -194,52 +194,6 @@ uint64_t helper_fmul8ulx16(uint64_t src1, uint64_t src2)
return d.ll;
}
-uint64_t helper_fmuld8sux16(uint64_t src1, uint64_t src2)
-{
-VIS64 s, d;
-uint32_t tmp;
-
-s.ll = src1;
-d.ll = src2;
-
-#define PMUL(r) \
-tmp = (int32_t)d.VIS_
[PULL 12/12] target/sparc: Split out do_ms16b
From: Richard Henderson
The unit operation for fmul8x16 and friends is described in the
manual as "MS16b". Split that out for clarity. Improve rounding
with an unconditional addition of 0.5 as a fixed-point integer.
Signed-off-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20240502165528.244004-8-richard.hender...@linaro.org>
Signed-off-by: Mark Cave-Ayland
---
target/sparc/vis_helper.c | 78 ---
1 file changed, 24 insertions(+), 54 deletions(-)
diff --git a/target/sparc/vis_helper.c b/target/sparc/vis_helper.c
index 14c665cad6..e15c6bb34e 100644
--- a/target/sparc/vis_helper.c
+++ b/target/sparc/vis_helper.c
@@ -44,6 +44,7 @@ target_ulong helper_array8(target_ulong pixel_addr,
target_ulong cubesize)
#if HOST_BIG_ENDIAN
#define VIS_B64(n) b[7 - (n)]
+#define VIS_SB64(n) sb[7 - (n)]
#define VIS_W64(n) w[3 - (n)]
#define VIS_SW64(n) sw[3 - (n)]
#define VIS_L64(n) l[1 - (n)]
@@ -51,6 +52,7 @@ target_ulong helper_array8(target_ulong pixel_addr,
target_ulong cubesize)
#define VIS_W32(n) w[1 - (n)]
#else
#define VIS_B64(n) b[n]
+#define VIS_SB64(n) sb[n]
#define VIS_W64(n) w[n]
#define VIS_SW64(n) sw[n]
#define VIS_L64(n) l[n]
@@ -60,6 +62,7 @@ target_ulong helper_array8(target_ulong pixel_addr,
target_ulong cubesize)
typedef union {
uint8_t b[8];
+int8_t sb[8];
uint16_t w[4];
int16_t sw[4];
uint32_t l[2];
@@ -95,27 +98,23 @@ uint64_t helper_fpmerge(uint32_t src1, uint32_t src2)
return d.ll;
}
+static inline int do_ms16b(int x, int y)
+{
+return ((x * y) + 0x80) >> 8;
+}
+
uint64_t helper_fmul8x16(uint32_t src1, uint64_t src2)
{
VIS64 d;
VIS32 s;
-uint32_t tmp;
s.l = src1;
d.ll = src2;
-#define PMUL(r) \
-tmp = (int32_t)d.VIS_SW64(r) * (int32_t)s.VIS_B32(r); \
-if ((tmp & 0xff) > 0x7f) { \
-tmp += 0x100; \
-} \
-d.VIS_W64(r) = tmp >> 8;
-
-PMUL(0);
-PMUL(1);
-PMUL(2);
-PMUL(3);
-#undef PMUL
+d.VIS_W64(0) = do_ms16b(s.VIS_B32(0), d.VIS_SW64(0));
+d.VIS_W64(1) = do_ms16b(s.VIS_B32(1), d.VIS_SW64(1));
+d.VIS_W64(2) = do_ms16b(s.VIS_B32(2), d.VIS_SW64(2));
+d.VIS_W64(3) = do_ms16b(s.VIS_B32(3), d.VIS_SW64(3));
return d.ll;
}
@@ -124,25 +123,14 @@ uint64_t helper_fmul8x16a(uint32_t src1, int32_t src2)
{
VIS32 s;
VIS64 d;
-uint32_t tmp;
s.l = src1;
d.ll = 0;
-#define PMUL(r)\
-do { \
-tmp = src2 * (int32_t)s.VIS_B32(r);\
-if ((tmp & 0xff) > 0x7f) { \
-tmp += 0x100; \
-} \
-d.VIS_W64(r) = tmp >> 8; \
-} while (0)
-
-PMUL(0);
-PMUL(1);
-PMUL(2);
-PMUL(3);
-#undef PMUL
+d.VIS_W64(0) = do_ms16b(s.VIS_B32(0), src2);
+d.VIS_W64(1) = do_ms16b(s.VIS_B32(1), src2);
+d.VIS_W64(2) = do_ms16b(s.VIS_B32(2), src2);
+d.VIS_W64(3) = do_ms16b(s.VIS_B32(3), src2);
return d.ll;
}
@@ -150,23 +138,14 @@ uint64_t helper_fmul8x16a(uint32_t src1, int32_t src2)
uint64_t helper_fmul8sux16(uint64_t src1, uint64_t src2)
{
VIS64 s, d;
-uint32_t tmp;
s.ll = src1;
d.ll = src2;
-#define PMUL(r) \
-tmp = (int32_t)d.VIS_SW64(r) * ((int32_t)s.VIS_SW64(r) >> 8); \
-if ((tmp & 0xff) > 0x7f) { \
-tmp += 0x100; \
-} \
-d.VIS_W64(r) = tmp >> 8;
-
-PMUL(0);
-PMUL(1);
-PMUL(2);
-PMUL(3);
-#undef PMUL
+d.VIS_W64(0) = do_ms16b(s.VIS_SB64(1), d.VIS_SW64(0));
+d.VIS_W64(1) = do_ms16b(s.VIS_SB64(3), d.VIS_SW64(1));
+d.VIS_W64(2) = do_ms16b(s.VIS_SB64(5), d.VIS_SW64(2));
+d.VIS_W64(3) = do_ms16b(s.VIS_SB64(7), d.VIS_SW64(3));
return d.ll;
}
@@ -174,23 +153,14 @@ uint64_t helper_fmul8sux16(uint64_t src1, uint64_t src2)
uint64_t helper_fmul8ulx16(uint64_t src1, uint64_t src2)
{
VIS64 s, d;
-uint32_t tmp;
s.ll = src1;
d.ll = src2;
-#define PMUL(r) \
-tmp = (int32_t)d.VIS_SW64(r) * ((uint32_t)s.VIS_B64(r * 2));\
-if ((tmp & 0xff) > 0x7f) { \
-tmp += 0x100; \
-} \
-d.VIS_W64(r) = tmp >> 8;
-
-PMUL(0);
-PMUL(1);
-PMUL(2);
-
[PULL 11/12] target/sparc: Fix FPMERGE
From: Richard Henderson
This instruction has f32 inputs, which changes the decode
of the register numbers.
Signed-off-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20240502165528.244004-7-richard.hender...@linaro.org>
Signed-off-by: Mark Cave-Ayland
---
target/sparc/helper.h | 2 +-
target/sparc/translate.c | 2 +-
target/sparc/vis_helper.c | 27 ++-
3 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/target/sparc/helper.h b/target/sparc/helper.h
index fcb9c617b7..97fbf6f66c 100644
--- a/target/sparc/helper.h
+++ b/target/sparc/helper.h
@@ -94,7 +94,7 @@ DEF_HELPER_FLAGS_2(fstox, TCG_CALL_NO_WG, s64, env, f32)
DEF_HELPER_FLAGS_2(fdtox, TCG_CALL_NO_WG, s64, env, f64)
DEF_HELPER_FLAGS_2(fqtox, TCG_CALL_NO_WG, s64, env, i128)
-DEF_HELPER_FLAGS_2(fpmerge, TCG_CALL_NO_RWG_SE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(fpmerge, TCG_CALL_NO_RWG_SE, i64, i32, i32)
DEF_HELPER_FLAGS_2(fmul8x16, TCG_CALL_NO_RWG_SE, i64, i32, i64)
DEF_HELPER_FLAGS_2(fmul8x16a, TCG_CALL_NO_RWG_SE, i64, i32, s32)
DEF_HELPER_FLAGS_2(fmul8sux16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 8a2894bb9f..99c6f3cc72 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -4656,6 +4656,7 @@ TRANS(FMUL8x16AU, VIS1, do_dff, a, gen_op_fmul8x16au)
TRANS(FMUL8x16AL, VIS1, do_dff, a, gen_op_fmul8x16al)
TRANS(FMULD8SUx16, VIS1, do_dff, a, gen_op_fmuld8sux16)
TRANS(FMULD8ULx16, VIS1, do_dff, a, gen_op_fmuld8ulx16)
+TRANS(FPMERGE, VIS1, do_dff, a, gen_helper_fpmerge)
static bool do_dfd(DisasContext *dc, arg_r_r_r *a,
void (*func)(TCGv_i64, TCGv_i32, TCGv_i64))
@@ -4696,7 +4697,6 @@ static bool do_ddd(DisasContext *dc, arg_r_r_r *a,
TRANS(FMUL8SUx16, VIS1, do_ddd, a, gen_helper_fmul8sux16)
TRANS(FMUL8ULx16, VIS1, do_ddd, a, gen_helper_fmul8ulx16)
-TRANS(FPMERGE, VIS1, do_ddd, a, gen_helper_fpmerge)
TRANS(FPADD16, VIS1, do_ddd, a, tcg_gen_vec_add16_i64)
TRANS(FPADD32, VIS1, do_ddd, a, tcg_gen_vec_add32_i64)
diff --git a/target/sparc/vis_helper.c b/target/sparc/vis_helper.c
index 61c61c7fea..14c665cad6 100644
--- a/target/sparc/vis_helper.c
+++ b/target/sparc/vis_helper.c
@@ -74,22 +74,23 @@ typedef union {
float32 f;
} VIS32;
-uint64_t helper_fpmerge(uint64_t src1, uint64_t src2)
+uint64_t helper_fpmerge(uint32_t src1, uint32_t src2)
{
-VIS64 s, d;
+VIS32 s1, s2;
+VIS64 d;
-s.ll = src1;
-d.ll = src2;
+s1.l = src1;
+s2.l = src2;
+d.ll = 0;
-/* Reverse calculation order to handle overlap */
-d.VIS_B64(7) = s.VIS_B64(3);
-d.VIS_B64(6) = d.VIS_B64(3);
-d.VIS_B64(5) = s.VIS_B64(2);
-d.VIS_B64(4) = d.VIS_B64(2);
-d.VIS_B64(3) = s.VIS_B64(1);
-d.VIS_B64(2) = d.VIS_B64(1);
-d.VIS_B64(1) = s.VIS_B64(0);
-/* d.VIS_B64(0) = d.VIS_B64(0); */
+d.VIS_B64(7) = s1.VIS_B32(3);
+d.VIS_B64(6) = s2.VIS_B32(3);
+d.VIS_B64(5) = s1.VIS_B32(2);
+d.VIS_B64(4) = s2.VIS_B32(2);
+d.VIS_B64(3) = s1.VIS_B32(1);
+d.VIS_B64(2) = s2.VIS_B32(1);
+d.VIS_B64(1) = s1.VIS_B32(0);
+d.VIS_B64(0) = s2.VIS_B32(0);
return d.ll;
}
--
2.39.2
[PULL 08/12] target/sparc: Fix FMUL8x16
From: Richard Henderson
This instruction has f32 as source1, which alters the
decoding of the register number, which means we've been
passing the wrong data for odd register numbers.
Signed-off-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20240502165528.244004-4-richard.hender...@linaro.org>
Signed-off-by: Mark Cave-Ayland
---
target/sparc/helper.h | 2 +-
target/sparc/translate.c | 21 -
target/sparc/vis_helper.c | 9 +
3 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/target/sparc/helper.h b/target/sparc/helper.h
index 57ab755ffd..27dc604cac 100644
--- a/target/sparc/helper.h
+++ b/target/sparc/helper.h
@@ -95,7 +95,7 @@ DEF_HELPER_FLAGS_2(fdtox, TCG_CALL_NO_WG, s64, env, f64)
DEF_HELPER_FLAGS_2(fqtox, TCG_CALL_NO_WG, s64, env, i128)
DEF_HELPER_FLAGS_2(fpmerge, TCG_CALL_NO_RWG_SE, i64, i64, i64)
-DEF_HELPER_FLAGS_2(fmul8x16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(fmul8x16, TCG_CALL_NO_RWG_SE, i64, i32, i64)
DEF_HELPER_FLAGS_2(fmul8x16al, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_2(fmul8x16au, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_2(fmul8sux16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index dfcfe855a1..c4adc148d2 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -4583,6 +4583,26 @@ TRANS(FSUBs, ALL, do_env_fff, a, gen_helper_fsubs)
TRANS(FMULs, ALL, do_env_fff, a, gen_helper_fmuls)
TRANS(FDIVs, ALL, do_env_fff, a, gen_helper_fdivs)
+static bool do_dfd(DisasContext *dc, arg_r_r_r *a,
+ void (*func)(TCGv_i64, TCGv_i32, TCGv_i64))
+{
+TCGv_i64 dst, src2;
+TCGv_i32 src1;
+
+if (gen_trap_ifnofpu(dc)) {
+return true;
+}
+
+dst = gen_dest_fpr_D(dc, a->rd);
+src1 = gen_load_fpr_F(dc, a->rs1);
+src2 = gen_load_fpr_D(dc, a->rs2);
+func(dst, src1, src2);
+gen_store_fpr_D(dc, a->rd, dst);
+return advance_pc(dc);
+}
+
+TRANS(FMUL8x16, VIS1, do_dfd, a, gen_helper_fmul8x16)
+
static bool do_ddd(DisasContext *dc, arg_r_r_r *a,
void (*func)(TCGv_i64, TCGv_i64, TCGv_i64))
{
@@ -4600,7 +4620,6 @@ static bool do_ddd(DisasContext *dc, arg_r_r_r *a,
return advance_pc(dc);
}
-TRANS(FMUL8x16, VIS1, do_ddd, a, gen_helper_fmul8x16)
TRANS(FMUL8x16AU, VIS1, do_ddd, a, gen_helper_fmul8x16au)
TRANS(FMUL8x16AL, VIS1, do_ddd, a, gen_helper_fmul8x16al)
TRANS(FMUL8SUx16, VIS1, do_ddd, a, gen_helper_fmul8sux16)
diff --git a/target/sparc/vis_helper.c b/target/sparc/vis_helper.c
index db2e6dd6c1..7728ffe9c6 100644
--- a/target/sparc/vis_helper.c
+++ b/target/sparc/vis_helper.c
@@ -94,16 +94,17 @@ uint64_t helper_fpmerge(uint64_t src1, uint64_t src2)
return d.ll;
}
-uint64_t helper_fmul8x16(uint64_t src1, uint64_t src2)
+uint64_t helper_fmul8x16(uint32_t src1, uint64_t src2)
{
-VIS64 s, d;
+VIS64 d;
+VIS32 s;
uint32_t tmp;
-s.ll = src1;
+s.l = src1;
d.ll = src2;
#define PMUL(r) \
-tmp = (int32_t)d.VIS_SW64(r) * (int32_t)s.VIS_B64(r); \
+tmp = (int32_t)d.VIS_SW64(r) * (int32_t)s.VIS_B32(r); \
if ((tmp & 0xff) > 0x7f) { \
tmp += 0x100; \
} \
--
2.39.2
[PULL 03/12] docs/system/target-sparc: Improve the Sparc documentation
From: Thomas Huth Add some words about how to enable or disable boolean features, and remove the note about a Linux kernel being available on the QEMU website (they have been removed long ago already), and the note about NetBSD and OpenBSD still having issues (they should work fine nowadays). Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2141 Signed-off-by: Thomas Huth Reviewed-by: Mark Cave-Ayland Reviewed-by: Richard Henderson Message-Id: <20240419084812.504779-4-th...@redhat.com> Signed-off-by: Mark Cave-Ayland --- docs/system/target-sparc.rst | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/system/target-sparc.rst b/docs/system/target-sparc.rst index 9ec8c90c14..4116cac493 100644 --- a/docs/system/target-sparc.rst +++ b/docs/system/target-sparc.rst @@ -27,6 +27,11 @@ architecture machines: The emulation is somewhat complete. SMP up to 16 CPUs is supported, but Linux limits the number of usable CPUs to 4. +The list of available CPUs can be viewed by starting QEMU with ``-cpu help``. +Optional boolean features can be added with a "+" in front of the feature name, +or disabled with a "-" in front of the name, for example +``-cpu TI-SuperSparc-II,+float128``. + QEMU emulates the following sun4m peripherals: - IOMMU @@ -55,8 +60,5 @@ OpenBIOS is a free (GPL v2) portable firmware implementation. The goal is to implement a 100% IEEE 1275-1994 (referred to as Open Firmware) compliant firmware. -A sample Linux 2.6 series kernel and ram disk image are available on the -QEMU web site. There are still issues with NetBSD and OpenBSD, but most -kernel versions work. Please note that currently older Solaris kernels -don't work probably due to interface issues between OpenBIOS and -Solaris. +Please note that currently older Solaris kernels don't work; this is probably +due to interface issues between OpenBIOS and Solaris. -- 2.39.2
[PULL 05/12] hw/sparc64: set iommu_platform=on for virtio devices attached to the sun4u machine
The sun4u machine has an IOMMU and therefore it is possible to program it such
that the virtio-device IOVA does not map directly to the CPU physical address.
This is not a problem with Linux which always maps the IOVA directly to the CPU
physical address, however it is required for the NetBSD virtio driver where this
is not the case.
Set the sun4u machine defaults for all virtio devices so that disable-legacy=on
and iommu_platform=on to ensure a default configuration will allow virtio
devices to function correctly on both Linux and NetBSD.
Signed-off-by: Mark Cave-Ayland
Message-Id: <20240418205730.31396-1-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/sparc64/sun4u.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index cff6d5abaf..4ece1ac1ff 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -793,6 +793,12 @@ static void sun4v_init(MachineState *machine)
sun4uv_init(get_system_memory(), machine, [1]);
}
+static GlobalProperty hw_compat_sparc64[] = {
+{ "virtio-pci", "disable-legacy", "on", .optional = true },
+{ "virtio-device", "iommu_platform", "on" },
+};
+static const size_t hw_compat_sparc64_len = G_N_ELEMENTS(hw_compat_sparc64);
+
static void sun4u_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -810,6 +816,7 @@ static void sun4u_class_init(ObjectClass *oc, void *data)
mc->default_nic = "sunhme";
mc->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
fwc->get_dev_path = sun4u_fw_dev_path;
+compat_props_add(mc->compat_props, hw_compat_sparc64,
hw_compat_sparc64_len);
}
static const TypeInfo sun4u_type = {
--
2.39.2
[PULL 09/12] target/sparc: Fix FMUL8x16A{U,L}
From: Richard Henderson
These instructions have f32 inputs, which changes the decode
of the register numbers. While we're fixing things, use a
common helper for both insns, extracting the 16-bit scalar
in tcg beforehand.
Signed-off-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20240502165528.244004-5-richard.hender...@linaro.org>
Signed-off-by: Mark Cave-Ayland
---
target/sparc/helper.h | 3 +--
target/sparc/translate.c | 38 +++
target/sparc/vis_helper.c | 47 +++
3 files changed, 48 insertions(+), 40 deletions(-)
diff --git a/target/sparc/helper.h b/target/sparc/helper.h
index 27dc604cac..9cde2b69a5 100644
--- a/target/sparc/helper.h
+++ b/target/sparc/helper.h
@@ -96,8 +96,7 @@ DEF_HELPER_FLAGS_2(fqtox, TCG_CALL_NO_WG, s64, env, i128)
DEF_HELPER_FLAGS_2(fpmerge, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_2(fmul8x16, TCG_CALL_NO_RWG_SE, i64, i32, i64)
-DEF_HELPER_FLAGS_2(fmul8x16al, TCG_CALL_NO_RWG_SE, i64, i64, i64)
-DEF_HELPER_FLAGS_2(fmul8x16au, TCG_CALL_NO_RWG_SE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(fmul8x16a, TCG_CALL_NO_RWG_SE, i64, i32, s32)
DEF_HELPER_FLAGS_2(fmul8sux16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_2(fmul8ulx16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_2(fmuld8sux16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index c4adc148d2..a8ada6934a 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -45,6 +45,7 @@
# define gen_helper_clear_softint(E, S) qemu_build_not_reached()
# define gen_helper_done(E) qemu_build_not_reached()
# define gen_helper_flushw(E) qemu_build_not_reached()
+# define gen_helper_fmul8x16a(D, S1, S2)qemu_build_not_reached()
# define gen_helper_rdccr(D, E) qemu_build_not_reached()
# define gen_helper_rdcwp(D, E) qemu_build_not_reached()
# define gen_helper_restored(E) qemu_build_not_reached()
@@ -72,8 +73,6 @@
# define gen_helper_fexpand ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmul8sux16 ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmul8ulx16 ({ qemu_build_not_reached(); NULL; })
-# define gen_helper_fmul8x16al ({ qemu_build_not_reached(); NULL; })
-# define gen_helper_fmul8x16au ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmul8x16 ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmuld8sux16 ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmuld8ulx16 ({ qemu_build_not_reached(); NULL; })
@@ -719,6 +718,18 @@ static void gen_op_bshuffle(TCGv_i64 dst, TCGv_i64 src1,
TCGv_i64 src2)
#endif
}
+static void gen_op_fmul8x16al(TCGv_i64 dst, TCGv_i32 src1, TCGv_i32 src2)
+{
+tcg_gen_ext16s_i32(src2, src2);
+gen_helper_fmul8x16a(dst, src1, src2);
+}
+
+static void gen_op_fmul8x16au(TCGv_i64 dst, TCGv_i32 src1, TCGv_i32 src2)
+{
+tcg_gen_sari_i32(src2, src2, 16);
+gen_helper_fmul8x16a(dst, src1, src2);
+}
+
static void finishing_insn(DisasContext *dc)
{
/*
@@ -4583,6 +4594,27 @@ TRANS(FSUBs, ALL, do_env_fff, a, gen_helper_fsubs)
TRANS(FMULs, ALL, do_env_fff, a, gen_helper_fmuls)
TRANS(FDIVs, ALL, do_env_fff, a, gen_helper_fdivs)
+static bool do_dff(DisasContext *dc, arg_r_r_r *a,
+ void (*func)(TCGv_i64, TCGv_i32, TCGv_i32))
+{
+TCGv_i64 dst;
+TCGv_i32 src1, src2;
+
+if (gen_trap_ifnofpu(dc)) {
+return true;
+}
+
+dst = gen_dest_fpr_D(dc, a->rd);
+src1 = gen_load_fpr_F(dc, a->rs1);
+src2 = gen_load_fpr_F(dc, a->rs2);
+func(dst, src1, src2);
+gen_store_fpr_D(dc, a->rd, dst);
+return advance_pc(dc);
+}
+
+TRANS(FMUL8x16AU, VIS1, do_dff, a, gen_op_fmul8x16au)
+TRANS(FMUL8x16AL, VIS1, do_dff, a, gen_op_fmul8x16al)
+
static bool do_dfd(DisasContext *dc, arg_r_r_r *a,
void (*func)(TCGv_i64, TCGv_i32, TCGv_i64))
{
@@ -4620,8 +4652,6 @@ static bool do_ddd(DisasContext *dc, arg_r_r_r *a,
return advance_pc(dc);
}
-TRANS(FMUL8x16AU, VIS1, do_ddd, a, gen_helper_fmul8x16au)
-TRANS(FMUL8x16AL, VIS1, do_ddd, a, gen_helper_fmul8x16al)
TRANS(FMUL8SUx16, VIS1, do_ddd, a, gen_helper_fmul8sux16)
TRANS(FMUL8ULx16, VIS1, do_ddd, a, gen_helper_fmul8ulx16)
TRANS(FMULD8SUx16, VIS1, do_ddd, a, gen_helper_fmuld8sux16)
diff --git a/target/sparc/vis_helper.c b/target/sparc/vis_helper.c
index 7728ffe9c6..ff2f43c23f 100644
--- a/target/sparc/vis_helper.c
+++ b/target/sparc/vis_helper.c
@@ -119,44 +119,23 @@ uint64_t helper_fmul8x16(uint32_t src1, uint64_t src2)
return d.ll;
}
-uint64_t helper_fmul8x16al(uint64_t src1, uint64_t src2)
+uint64_t helper_fmul8x16a(uint32_t src1, int32_t src2)
{
-VIS64 s, d;
-uint32_t tmp;
-
-s.ll = src1;
-d.ll = src2;
[PULL 06/12] linux-user/sparc: Add more hwcap bits for sparc64
From: Richard Henderson
Supply HWCAP_SPARC_V8PLUS, HWCAP_SPARC_MUL32, HWCAP_SPARC_DIV32,
HWCAP_SPARC_POPC, HWCAP_SPARC_FSMULD, HWCAP_SPARC_VIS, HWCAP_SPARC_VIS2.
Signed-off-by: Richard Henderson
Message-Id: <20240502165528.244004-2-richard.hender...@linaro.org>
Signed-off-by: Mark Cave-Ayland
---
linux-user/elfload.c | 48 +++-
1 file changed, 34 insertions(+), 14 deletions(-)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index f9461d2844..14f08b64a1 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -968,24 +968,44 @@ const char *elf_hwcap2_str(uint32_t bit)
#endif /* TARGET_ARM */
#ifdef TARGET_SPARC
-#ifdef TARGET_SPARC64
-#define ELF_HWCAP (HWCAP_SPARC_FLUSH | HWCAP_SPARC_STBAR | HWCAP_SPARC_SWAP \
-| HWCAP_SPARC_MULDIV | HWCAP_SPARC_V9)
-#ifndef TARGET_ABI32
-#define elf_check_arch(x) ( (x) == EM_SPARCV9 || (x) == EM_SPARC32PLUS )
+#ifndef TARGET_SPARC64
+# define ELF_CLASS ELFCLASS32
+# define ELF_ARCH EM_SPARC
+#elif defined(TARGET_ABI32)
+# define ELF_CLASS ELFCLASS32
+# define elf_check_arch(x) ((x) == EM_SPARC32PLUS || (x) == EM_SPARC)
#else
-#define elf_check_arch(x) ( (x) == EM_SPARC32PLUS || (x) == EM_SPARC )
+# define ELF_CLASS ELFCLASS64
+# define ELF_ARCH EM_SPARCV9
#endif
-#define ELF_CLASS ELFCLASS64
-#define ELF_ARCHEM_SPARCV9
-#else
-#define ELF_HWCAP (HWCAP_SPARC_FLUSH | HWCAP_SPARC_STBAR | HWCAP_SPARC_SWAP \
-| HWCAP_SPARC_MULDIV)
-#define ELF_CLASS ELFCLASS32
-#define ELF_ARCHEM_SPARC
-#endif /* TARGET_SPARC64 */
+#include "elf.h"
+
+#define ELF_HWCAP get_elf_hwcap()
+
+static uint32_t get_elf_hwcap(void)
+{
+/* There are not many sparc32 hwcap bits -- we have all of them. */
+uint32_t r = HWCAP_SPARC_FLUSH | HWCAP_SPARC_STBAR |
+ HWCAP_SPARC_SWAP | HWCAP_SPARC_MULDIV;
+
+#ifdef TARGET_SPARC64
+CPUSPARCState *env = cpu_env(thread_cpu);
+uint32_t features = env->def.features;
+
+r |= HWCAP_SPARC_V9 | HWCAP_SPARC_V8PLUS;
+/* 32x32 multiply and divide are efficient. */
+r |= HWCAP_SPARC_MUL32 | HWCAP_SPARC_DIV32;
+/* We don't have an internal feature bit for this. */
+r |= HWCAP_SPARC_POPC;
+r |= features & CPU_FEATURE_FSMULD ? HWCAP_SPARC_FSMULD : 0;
+r |= features & CPU_FEATURE_VIS1 ? HWCAP_SPARC_VIS : 0;
+r |= features & CPU_FEATURE_VIS2 ? HWCAP_SPARC_VIS2 : 0;
+#endif
+
+return r;
+}
static inline void init_thread(struct target_pt_regs *regs,
struct image_info *infop)
--
2.39.2
[PULL 07/12] target/sparc: Fix FEXPAND
From: Richard Henderson
This is a 2-operand instruction, not 3-operand.
Worse, we took the source from the wrong operand.
Signed-off-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20240502165528.244004-3-richard.hender...@linaro.org>
Signed-off-by: Mark Cave-Ayland
---
target/sparc/helper.h | 2 +-
target/sparc/insns.decode | 2 +-
target/sparc/translate.c | 20 +++-
target/sparc/vis_helper.c | 6 +++---
4 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/target/sparc/helper.h b/target/sparc/helper.h
index b8087d0d2b..57ab755ffd 100644
--- a/target/sparc/helper.h
+++ b/target/sparc/helper.h
@@ -102,7 +102,7 @@ DEF_HELPER_FLAGS_2(fmul8sux16, TCG_CALL_NO_RWG_SE, i64,
i64, i64)
DEF_HELPER_FLAGS_2(fmul8ulx16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_2(fmuld8sux16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_2(fmuld8ulx16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
-DEF_HELPER_FLAGS_2(fexpand, TCG_CALL_NO_RWG_SE, i64, i64, i64)
+DEF_HELPER_FLAGS_1(fexpand, TCG_CALL_NO_RWG_SE, i64, i32)
DEF_HELPER_FLAGS_3(pdist, TCG_CALL_NO_RWG_SE, i64, i64, i64, i64)
DEF_HELPER_FLAGS_2(fpack16, TCG_CALL_NO_RWG_SE, i32, i64, i64)
DEF_HELPER_FLAGS_3(fpack32, TCG_CALL_NO_RWG_SE, i64, i64, i64, i64)
diff --git a/target/sparc/insns.decode b/target/sparc/insns.decode
index 2d26404cb2..e2d8a07dc4 100644
--- a/target/sparc/insns.decode
+++ b/target/sparc/insns.decode
@@ -352,7 +352,7 @@ FCMPEq 10 000 cc:2 110101 rs1:5 0 0101 0111 rs2:5
FALIGNDATAg 10 . 110110 . 0 0100 1000 .@r_r_r
FPMERGE 10 . 110110 . 0 0100 1011 .@r_r_r
BSHUFFLE10 . 110110 . 0 0100 1100 .@r_r_r
-FEXPAND 10 . 110110 . 0 0100 1101 .@r_r_r
+FEXPAND 10 . 110110 0 0 0100 1101 .@r_r2
FSRCd 10 . 110110 . 0 0111 0100 0@r_r1 # FSRC1d
FSRCs 10 . 110110 . 0 0111 0101 0@r_r1 # FSRC1s
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 571b3e3f03..dfcfe855a1 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -4358,6 +4358,25 @@ TRANS(FSQRTd, ALL, do_env_dd, a, gen_helper_fsqrtd)
TRANS(FxTOd, 64, do_env_dd, a, gen_helper_fxtod)
TRANS(FdTOx, 64, do_env_dd, a, gen_helper_fdtox)
+static bool do_df(DisasContext *dc, arg_r_r *a,
+ void (*func)(TCGv_i64, TCGv_i32))
+{
+TCGv_i64 dst;
+TCGv_i32 src;
+
+if (gen_trap_ifnofpu(dc)) {
+return true;
+}
+
+dst = tcg_temp_new_i64();
+src = gen_load_fpr_F(dc, a->rs);
+func(dst, src);
+gen_store_fpr_D(dc, a->rd, dst);
+return advance_pc(dc);
+}
+
+TRANS(FEXPAND, VIS1, do_df, a, gen_helper_fexpand)
+
static bool do_env_df(DisasContext *dc, arg_r_r *a,
void (*func)(TCGv_i64, TCGv_env, TCGv_i32))
{
@@ -4589,7 +4608,6 @@ TRANS(FMUL8ULx16, VIS1, do_ddd, a, gen_helper_fmul8ulx16)
TRANS(FMULD8SUx16, VIS1, do_ddd, a, gen_helper_fmuld8sux16)
TRANS(FMULD8ULx16, VIS1, do_ddd, a, gen_helper_fmuld8ulx16)
TRANS(FPMERGE, VIS1, do_ddd, a, gen_helper_fpmerge)
-TRANS(FEXPAND, VIS1, do_ddd, a, gen_helper_fexpand)
TRANS(FPADD16, VIS1, do_ddd, a, tcg_gen_vec_add16_i64)
TRANS(FPADD32, VIS1, do_ddd, a, tcg_gen_vec_add32_i64)
diff --git a/target/sparc/vis_helper.c b/target/sparc/vis_helper.c
index 7763b16c24..db2e6dd6c1 100644
--- a/target/sparc/vis_helper.c
+++ b/target/sparc/vis_helper.c
@@ -260,13 +260,13 @@ uint64_t helper_fmuld8ulx16(uint64_t src1, uint64_t src2)
return d.ll;
}
-uint64_t helper_fexpand(uint64_t src1, uint64_t src2)
+uint64_t helper_fexpand(uint32_t src2)
{
VIS32 s;
VIS64 d;
-s.l = (uint32_t)src1;
-d.ll = src2;
+s.l = src2;
+d.ll = 0;
d.VIS_W64(0) = s.VIS_B32(0) << 4;
d.VIS_W64(1) = s.VIS_B32(1) << 4;
d.VIS_W64(2) = s.VIS_B32(2) << 4;
--
2.39.2
[PULL 04/12] docs/about: Deprecate the old "UltraSparc" CPU names that contain a "+"
From: Thomas Huth For consistency we should drop the names with a "+" in it in the long run. Reviewed-by: Mark Cave-Ayland Signed-off-by: Thomas Huth Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson Message-Id: <20240419084812.504779-5-th...@redhat.com> Signed-off-by: Mark Cave-Ayland --- docs/about/deprecated.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 7b8aafa15b..03f8b1b655 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -194,6 +194,15 @@ in the QEMU object model anymore. ``power5+``, ``power5+_v2.1``, an alias, but for consistency these will get removed in a future release, too. Use ``power5p_v2.1`` and ``power7p_v2.1`` instead. +``Sun-UltraSparc-IIIi+`` and ``Sun-UltraSparc-IV+`` CPU names (since 9.1) +' + +The character "+" in device (and thus also CPU) names is not allowed +in the QEMU object model anymore. ``Sun-UltraSparc-IIIi+`` and +``Sun-UltraSparc-IV+`` are currently still supported via a workaround, +but for consistency these will get removed in a future release, too. +Use ``Sun-UltraSparc-IIIi-plus`` and ``Sun-UltraSparc-IV-plus`` instead. + CRIS CPU architecture (since 9.0) ' -- 2.39.2
[PULL 01/12] target/sparc/cpu: Rename the CPU models with a "+" in their names
From: Thomas Huth
Commit b447378e12 ("qom/object: Limit type names to alphanumerical ...")
cut down the amount of allowed characters for QOM types to a saner set.
The "+" character was meant to be included in this set, so we had to
add a hack there to still allow the legacy names of POWER and Sparc64
CPUs. However, instead of putting such a hack in the common QOM code,
there is a much better place to do this: The sparc_cpu_class_by_name()
function which is used to look up the names of all Sparc CPUs.
Thus let's finally get rid of the "+" in the Sparc CPU names, and provide
backward compatibility for the old names via some simple checks in the
sparc_cpu_class_by_name() function.
Reviewed-by: Mark Cave-Ayland
Signed-off-by: Thomas Huth
Reviewed-by: Richard Henderson
Message-Id: <20240419084812.504779-2-th...@redhat.com>
Signed-off-by: Mark Cave-Ayland
---
qom/object.c | 8
target/sparc/cpu.c | 14 --
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index 44ec8f6460..157a45c5f8 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -157,14 +157,6 @@ static bool type_name_is_valid(const char *name)
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
"0123456789-_.");
-/* Allow some legacy names with '+' in it for compatibility reasons */
-if (name[plen] == '+') {
-if (plen >= 17 && g_str_has_prefix(name, "Sun-UltraSparc-I")) {
-/* Allow "Sun-UltraSparc-IV+" and "Sun-UltraSparc-IIIi+" */
-return true;
-}
-}
-
return plen == slen;
}
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 485d416925..7487ae034d 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -314,7 +314,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "Sun UltraSparc IV+",
+.name = "Sun UltraSparc IV plus",
.iu_version = ((0x3eULL << 48) | (0x19ULL << 32) | (0x22ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_12,
@@ -323,7 +323,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES | CPU_FEATURE_CMT,
},
{
-.name = "Sun UltraSparc IIIi+",
+.name = "Sun UltraSparc IIIi plus",
.iu_version = ((0x3eULL << 48) | (0x22ULL << 32) | (0ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_3,
@@ -762,6 +762,16 @@ static ObjectClass *sparc_cpu_class_by_name(const char
*cpu_model)
char *typename;
typename = sparc_cpu_type_name(cpu_model);
+
+/* Fix up legacy names with '+' in it */
+if (g_str_equal(typename, SPARC_CPU_TYPE_NAME("Sun-UltraSparc-IV+"))) {
+g_free(typename);
+typename = g_strdup(SPARC_CPU_TYPE_NAME("Sun-UltraSparc-IV-plus"));
+} else if (g_str_equal(typename,
SPARC_CPU_TYPE_NAME("Sun-UltraSparc-IIIi+"))) {
+g_free(typename);
+typename = g_strdup(SPARC_CPU_TYPE_NAME("Sun-UltraSparc-IIIi-plus"));
+}
+
oc = object_class_by_name(typename);
g_free(typename);
return oc;
--
2.39.2
[PULL 02/12] target/sparc/cpu: Avoid spaces by default in the CPU names
From: Thomas Huth
The output of "-cpu help" is currently rather confusing to the users:
It might not be fully clear which part of the output defines the CPU
names since the CPU names contain white spaces (which we later have to
convert into dashes internally). At best it's at least a nuisance since
the users might need to specify the CPU names with quoting on the command
line if they are not aware of the fact that the CPU names could be written
with dashes instead. So let's finally clean up this mess by using dashes
instead of white spaces for the CPU names, like we're doing it internally
later (and like we're doing it in most other targets of QEMU).
Note that it is still possible to pass the CPU names with spaces to the
"-cpu" option, since sparc_cpu_type_name() still translates those to "-".
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2141
Reviewed-by: Richard Henderson
Reviewed-by: Mark Cave-Ayland
Signed-off-by: Thomas Huth
Message-Id: <20240419084812.504779-3-th...@redhat.com>
Signed-off-by: Mark Cave-Ayland
---
target/sparc/cpu.c | 56 +++---
1 file changed, 28 insertions(+), 28 deletions(-)
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 7487ae034d..c2be4a00b6 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -206,7 +206,7 @@ void cpu_sparc_set_id(CPUSPARCState *env, unsigned int cpu)
static const sparc_def_t sparc_defs[] = {
#ifdef TARGET_SPARC64
{
-.name = "Fujitsu Sparc64",
+.name = "Fujitsu-Sparc64",
.iu_version = ((0x04ULL << 48) | (0x02ULL << 32) | (0ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_12,
@@ -215,7 +215,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "Fujitsu Sparc64 III",
+.name = "Fujitsu-Sparc64-III",
.iu_version = ((0x04ULL << 48) | (0x03ULL << 32) | (0ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_12,
@@ -224,7 +224,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "Fujitsu Sparc64 IV",
+.name = "Fujitsu-Sparc64-IV",
.iu_version = ((0x04ULL << 48) | (0x04ULL << 32) | (0ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_12,
@@ -233,7 +233,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "Fujitsu Sparc64 V",
+.name = "Fujitsu-Sparc64-V",
.iu_version = ((0x04ULL << 48) | (0x05ULL << 32) | (0x51ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_12,
@@ -242,7 +242,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "TI UltraSparc I",
+.name = "TI-UltraSparc-I",
.iu_version = ((0x17ULL << 48) | (0x10ULL << 32) | (0x40ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_12,
@@ -251,7 +251,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "TI UltraSparc II",
+.name = "TI-UltraSparc-II",
.iu_version = ((0x17ULL << 48) | (0x11ULL << 32) | (0x20ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_12,
@@ -260,7 +260,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "TI UltraSparc IIi",
+.name = "TI-UltraSparc-IIi",
.iu_version = ((0x17ULL << 48) | (0x12ULL << 32) | (0x91ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_12,
@@ -269,7 +269,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "TI UltraSparc IIe",
+.name = "TI-UltraSparc-IIe",
.iu_version = ((0x17ULL << 48) | (0x13ULL << 32) | (0x14ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_12,
@@ -278,7 +278,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "Sun UltraSparc III",
+.name = "Sun-UltraSparc-III",
.iu_version = ((0x3eULL << 48) | (0x14ULL << 32) | (0x34ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_12,
@@ -287,7 +287,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "Sun UltraSparc III Cu",
[PULL 00/12] qemu-sparc queue 20240506
The following changes since commit 248f6f62df073a3b4158fd0093863ab885feabb5:
Merge tag 'pull-axp-20240504' of https://gitlab.com/rth7680/qemu into staging
(2024-05-04 08:39:46 -0700)
are available in the Git repository at:
https://github.com/mcayland/qemu.git tags/qemu-sparc-20240506
for you to fetch changes up to d6f898cf85c92389182d22f0bcc3a11d7194fc94:
target/sparc: Split out do_ms16b (2024-05-05 21:02:48 +0100)
qemu-sparc queue
- Default to modern virtio with iommu_platform enabled for sun4u
- Fixes for various VIS instructions from Richard
- CPU name updates from Thomas
Mark Cave-Ayland (1):
hw/sparc64: set iommu_platform=on for virtio devices attached to the
sun4u machine
Richard Henderson (7):
linux-user/sparc: Add more hwcap bits for sparc64
target/sparc: Fix FEXPAND
target/sparc: Fix FMUL8x16
target/sparc: Fix FMUL8x16A{U,L}
target/sparc: Fix FMULD8*X16
target/sparc: Fix FPMERGE
target/sparc: Split out do_ms16b
Thomas Huth (4):
target/sparc/cpu: Rename the CPU models with a "+" in their names
target/sparc/cpu: Avoid spaces by default in the CPU names
docs/system/target-sparc: Improve the Sparc documentation
docs/about: Deprecate the old "UltraSparc" CPU names that contain a "+"
docs/about/deprecated.rst| 9 +++
docs/system/target-sparc.rst | 12 +--
hw/sparc64/sun4u.c | 7 ++
linux-user/elfload.c | 48 +++
qom/object.c | 8 --
target/sparc/cpu.c | 66 ---
target/sparc/helper.h| 11 +--
target/sparc/insns.decode| 2 +-
target/sparc/translate.c | 129 ++---
target/sparc/vis_helper.c| 189 +++
10 files changed, 265 insertions(+), 216 deletions(-)
Re: [PATCH v2 0/7] target/sparc: vis fixes
On 03/05/2024 19:18, Philippe Mathieu-Daudé wrote:
On 2/5/24 18:55, Richard Henderson wrote:
Split out from my vis4 patch set, with just the bug fixes.
I've fixed the issue in patch 6, as noticed by Mark, but
include the follow-up that cleans up all of the macros by
removing them.
r~
Richard Henderson (7):
linux-user/sparc: Add more hwcap bits for sparc64
target/sparc: Fix FEXPAND
target/sparc: Fix FMUL8x16
target/sparc: Fix FMUL8x16A{U,L}
target/sparc: Fix FMULD8*X16
target/sparc: Fix FPMERGE
target/sparc: Split out do_ms16b
I'm wondering about the coverage here, since various patches
fix bugs since VIS intro in commit e9ebed4d41 from 2007, so
being broken for 17 years.
I've definitely seen the VIS instructions in use in my test images, however I can't
recall exactly whether it was the particular ones fixed in this series. I'm certainly
keen for some more VIS instruction coverage though, especially for VIS2 and later
which I'm unlikely to come across in my day-to-day testing.
ATB,
Mark.
Re: [PATCH v2 0/7] target/sparc: vis fixes
On 02/05/2024 17:55, Richard Henderson wrote:
Split out from my vis4 patch set, with just the bug fixes.
I've fixed the issue in patch 6, as noticed by Mark, but
include the follow-up that cleans up all of the macros by
removing them.
r~
Richard Henderson (7):
linux-user/sparc: Add more hwcap bits for sparc64
target/sparc: Fix FEXPAND
target/sparc: Fix FMUL8x16
target/sparc: Fix FMUL8x16A{U,L}
target/sparc: Fix FMULD8*X16
target/sparc: Fix FPMERGE
target/sparc: Split out do_ms16b
target/sparc/helper.h | 11 +--
target/sparc/insns.decode | 2 +-
linux-user/elfload.c | 48 +++---
target/sparc/translate.c | 129 ++---
target/sparc/vis_helper.c | 195 ++
5 files changed, 207 insertions(+), 178 deletions(-)
Thanks Richard. I've applied v2 to my qmeu-sparc branch, and will send a PR assuming
everything passes in GitLab.
ATB,
Mark.
Re: [PATCH 04/41] target/sparc: Fix FMUL8x16A{U,L}
On 02/03/2024 05:15, Richard Henderson wrote:
These instructions have f32 inputs, which changes the decode
of the register numbers. While we're fixing things, use a
common helper for both insns, extracting the 16-bit scalar
in tcg beforehand.
Signed-off-by: Richard Henderson
---
target/sparc/helper.h | 3 +--
target/sparc/translate.c | 38 ++
target/sparc/vis_helper.c | 43 +--
3 files changed, 45 insertions(+), 39 deletions(-)
diff --git a/target/sparc/helper.h b/target/sparc/helper.h
index adc1b87319..9e0b8b463e 100644
--- a/target/sparc/helper.h
+++ b/target/sparc/helper.h
@@ -93,8 +93,7 @@ DEF_HELPER_FLAGS_2(fqtox, TCG_CALL_NO_WG, s64, env, i128)
DEF_HELPER_FLAGS_2(fpmerge, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_2(fmul8x16, TCG_CALL_NO_RWG_SE, i64, i32, i64)
-DEF_HELPER_FLAGS_2(fmul8x16al, TCG_CALL_NO_RWG_SE, i64, i64, i64)
-DEF_HELPER_FLAGS_2(fmul8x16au, TCG_CALL_NO_RWG_SE, i64, i64, i64)
+DEF_HELPER_FLAGS_2(fmul8x16a, TCG_CALL_NO_RWG_SE, i64, i32, s32)
DEF_HELPER_FLAGS_2(fmul8sux16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_2(fmul8ulx16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
DEF_HELPER_FLAGS_2(fmuld8sux16, TCG_CALL_NO_RWG_SE, i64, i64, i64)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 5144fe4ed9..598cfcf0ac 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -45,6 +45,7 @@
# define gen_helper_clear_softint(E, S) qemu_build_not_reached()
# define gen_helper_done(E) qemu_build_not_reached()
# define gen_helper_flushw(E) qemu_build_not_reached()
+# define gen_helper_fmul8x16a(D, S1, S2)qemu_build_not_reached()
# define gen_helper_rdccr(D, E) qemu_build_not_reached()
# define gen_helper_rdcwp(D, E) qemu_build_not_reached()
# define gen_helper_restored(E) qemu_build_not_reached()
@@ -72,8 +73,6 @@
# define gen_helper_fexpand ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmul8sux16 ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmul8ulx16 ({ qemu_build_not_reached(); NULL; })
-# define gen_helper_fmul8x16al ({ qemu_build_not_reached(); NULL; })
-# define gen_helper_fmul8x16au ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmul8x16 ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmuld8sux16 ({ qemu_build_not_reached(); NULL; })
# define gen_helper_fmuld8ulx16 ({ qemu_build_not_reached(); NULL; })
@@ -719,6 +718,18 @@ static void gen_op_bshuffle(TCGv_i64 dst, TCGv_i64 src1,
TCGv_i64 src2)
#endif
}
+static void gen_op_fmul8x16al(TCGv_i64 dst, TCGv_i32 src1, TCGv_i32 src2)
+{
+tcg_gen_ext16s_i32(src2, src2);
+gen_helper_fmul8x16a(dst, src1, src2);
+}
+
+static void gen_op_fmul8x16au(TCGv_i64 dst, TCGv_i32 src1, TCGv_i32 src2)
+{
+tcg_gen_sari_i32(src2, src2, 16);
+gen_helper_fmul8x16a(dst, src1, src2);
+}
+
static void finishing_insn(DisasContext *dc)
{
/*
@@ -4539,6 +4550,27 @@ TRANS(FSUBs, ALL, do_env_fff, a, gen_helper_fsubs)
TRANS(FMULs, ALL, do_env_fff, a, gen_helper_fmuls)
TRANS(FDIVs, ALL, do_env_fff, a, gen_helper_fdivs)
+static bool do_dff(DisasContext *dc, arg_r_r_r *a,
+ void (*func)(TCGv_i64, TCGv_i32, TCGv_i32))
+{
+TCGv_i64 dst;
+TCGv_i32 src1, src2;
+
+if (gen_trap_ifnofpu(dc)) {
+return true;
+}
+
+dst = gen_dest_fpr_D(dc, a->rd);
+src1 = gen_load_fpr_F(dc, a->rs1);
+src2 = gen_load_fpr_F(dc, a->rs2);
+func(dst, src1, src2);
+gen_store_fpr_D(dc, a->rd, dst);
+return advance_pc(dc);
+}
+
+TRANS(FMUL8x16AU, VIS1, do_dff, a, gen_op_fmul8x16au)
+TRANS(FMUL8x16AL, VIS1, do_dff, a, gen_op_fmul8x16al)
+
static bool do_dfd(DisasContext *dc, arg_r_r_r *a,
void (*func)(TCGv_i64, TCGv_i32, TCGv_i64))
{
@@ -4576,8 +4608,6 @@ static bool do_ddd(DisasContext *dc, arg_r_r_r *a,
return advance_pc(dc);
}
-TRANS(FMUL8x16AU, VIS1, do_ddd, a, gen_helper_fmul8x16au)
-TRANS(FMUL8x16AL, VIS1, do_ddd, a, gen_helper_fmul8x16al)
TRANS(FMUL8SUx16, VIS1, do_ddd, a, gen_helper_fmul8sux16)
TRANS(FMUL8ULx16, VIS1, do_ddd, a, gen_helper_fmul8ulx16)
TRANS(FMULD8SUx16, VIS1, do_ddd, a, gen_helper_fmuld8sux16)
diff --git a/target/sparc/vis_helper.c b/target/sparc/vis_helper.c
index 7728ffe9c6..5c7f5536bc 100644
--- a/target/sparc/vis_helper.c
+++ b/target/sparc/vis_helper.c
@@ -119,43 +119,20 @@ uint64_t helper_fmul8x16(uint32_t src1, uint64_t src2)
return d.ll;
}
-uint64_t helper_fmul8x16al(uint64_t src1, uint64_t src2)
+uint64_t helper_fmul8x16a(uint32_t src1, int32_t src2)
{
-VIS64 s, d;
+VIS32 s;
+VIS64 d;
uint32_t tmp;
-s.ll = src1;
-d.ll = src2;
+s.l = src1;
+d.ll = 0;
-#define PMUL(r)
Re: [PULL 0/1] target/sparc late fix
On 28/04/2024 04:10, M Bazz wrote: Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/9.0 for any user-visible changes. The 9.0 Changelog was never updated. Could someone with the permissions please add the following to the SPARC section: sparc32: Fixed a longstanding softmmu bug that caused kernel panics when the UserTxt ASI was accessed. Appreciated, -- Bazz I've just updated the 9.0 changelog with an improved version of the above wording as requested. ATB, Mark.
Re: [PATCH 00/41] target/sparc: Implement VIS4
On 29/04/2024 22:02, Richard Henderson wrote: On 4/29/24 13:52, Mark Cave-Ayland wrote: No objections here about the remainder of the series, other than that I don't have an easy/obvious way to test the new instructions... I was thinking about adding support to RISU, but the gcc compile farm sparc machines have been down for ages, so no way to generate the reference traces. Ah that's frustrating. I've just pinged the debian-sparc folk to see if anyone there can offer any insight or an alternative. ATB, Mark.
Re: [PATCH v2 0/4] Sparc CPU naming and help text improvements
On 19/04/2024 09:48, Thomas Huth wrote: The Sparc CPU naming and the corresponding help text is somewhat confusing for the users. We should avoid spaces in the Names and provide clear information to the users what can be passed to the "-cpu" option. While we're at it, also remove the "+" from two of the CPU names since this character is now not allowed in device names anymore (and was worked around with an ugly hack in qom/object.c so far). v2: - Use "Sun-UltraSparc-IIIi-plus" and "Sun-UltraSparc-IV-plus" instead of just adding a "p" at the end - Drop the sentence about NetBSD and OpenBSD in the docs since these problems are likely fixed since a long time already - Added Reviewed-bys from earlier series and updated the patch descriptions a little bit Thomas Huth (4): target/sparc/cpu: Rename the CPU models with a "+" in their names target/sparc/cpu: Avoid spaces by default in the CPU names docs/system/target-sparc: Improve the Sparc documentation docs/about: Deprecate the old "UltraSparc" CPU names that contain a "+" docs/about/deprecated.rst| 9 + docs/system/target-sparc.rst | 12 --- qom/object.c | 8 - target/sparc/cpu.c | 66 +--- 4 files changed, 54 insertions(+), 41 deletions(-) Thanks! I've applied this to my qemu-sparc branch, along with Peter's suggested tweak to the grammar in patch 3. ATB, Mark.
Re: [PATCH 00/41] target/sparc: Implement VIS4
On 02/03/2024 05:15, Richard Henderson wrote:
I whipped this up over the Christmas break, but I'm just now
getting around to posting. I have not attempted to model the
newer cpus that have these features, but it is possible to
enable the features manually via -cpu properties.
Possibly the first 6 or 7 patches should be taken sooner than
later because they fix bugs in existing VIS[12] code.
I remove cpu_fpr[], so that we can use gvec on the same memory.
r~
Richard Henderson (41):
linux-user/sparc: Add more hwcap bits for sparc64
target/sparc: Fix FEXPAND
target/sparc: Fix FMUL8x16
target/sparc: Fix FMUL8x16A{U,L}
target/sparc: Fix FMULD8*X16
target/sparc: Fix FPMERGE
target/sparc: Split out do_ms16b
target/sparc: Perform DFPREG/QFPREG in decodetree
target/sparc: Remove gen_dest_fpr_D
target/sparc: Remove cpu_fpr[]
target/sparc: Use gvec for VIS1 parallel add/sub
target/sparc: Implement FMAf extension
target/sparc: Add feature bits for VIS 3
target/sparc: Implement ADDXC, ADDXCcc
target/sparc: Implement CMASK instructions
target/sparc: Implement FCHKSM16
target/sparc: Implement FHADD, FHSUB, FNHADD, FNADD
target/sparc: Implement FNMUL
target/sparc: Implement FLCMP
target/sparc: Implement FMEAN16
target/sparc: Implement FPADD64 FPSUB64
target/sparc: Implement FPADDS, FPSUBS
target/sparc: Implement FPCMPEQ8, FPCMPNE8, FPCMPULE8, FPCMPUGT8
target/sparc: Implement FSLL, FSRL, FSRA, FSLAS
target/sparc: Implement LDXEFSR
target/sparc: Implement LZCNT
target/sparc: Implement MOVsTOw, MOVdTOx, MOVwTOs, MOVxTOd
target/sparc: Implement PDISTN
target/sparc: Implement UMULXHI
target/sparc: Implement XMULX
target/sparc: Enable VIS3 feature bit
target/sparc: Implement IMA extension
target/sparc: Add feature bit for VIS4
target/sparc: Implement FALIGNDATAi
target/sparc: Implement 8-bit FPADD, FPADDS, and FPADDUS
target/sparc: Implement VIS4 comparisons
target/sparc: Implement FPMIN, FPMAX
target/sparc: Implement SUBXC, SUBXCcc
target/sparc: Implement MWAIT
target/sparc: Implement monitor asis
target/sparc: Enable VIS4 feature bit
target/sparc/asi.h | 4 +
target/sparc/helper.h | 36 +-
linux-user/elfload.c | 51 +-
target/sparc/cpu.c | 12 +
target/sparc/fop_helper.c | 104
target/sparc/ldst_helper.c | 4 +
target/sparc/translate.c | 960 +
target/sparc/vis_helper.c | 526 +++---
target/sparc/cpu-feature.h.inc | 4 +
target/sparc/insns.decode | 338 +---
10 files changed, 1626 insertions(+), 413 deletions(-)
I've applied the first 6 patches to my qemu-sparc branch, since I believe these are
the up-to-date version of the patches posted for
https://gitlab.com/qemu-project/qemu/-/issues/1901.
No objections here about the remainder of the series, other than that I don't have an
easy/obvious way to test the new instructions...
ATB,
Mark.
Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one
On 25/04/2024 11:26, Manos Pitsidianakis wrote:
On Thu, 25 Apr 2024 at 13:24, Michael S. Tsirkin wrote:
On Thu, Apr 25, 2024 at 01:04:31PM +0300, Manos Pitsidianakis wrote:
On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
wrote:
On 25/04/2024 07:30, Manos Pitsidianakis wrote:
On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
wrote:
On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
On 23/4/24 11:18, Manos Pitsidianakis wrote:
On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
wrote:
On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
On 22/4/24 23:02, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.
Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé
This is all completely bogus. Virtio SND is from Virtio 1.0 only.
It is unconditionally little endian.
This part of the code is for PCM frames (raw bytes), not virtio spec
fields (which indeed must be LE in modern VIRTIO).
Thought a little more about it. We should keep the target's endianness
here, if it's mutable then we should query the machine the device is
attached to somehow. the virtio device should never change endianness
like Michael says since it's not legacy.
Grr. So as Richard suggested, this need to be pass as a device
property then.
(https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09...@linaro.org/)
It feels to me that the endianness is something that should be negotiated as
part of
the frame format, since the endianness of the audio hardware can be different
from
that of the CPU (think PReP machines where it was common that a big endian CPU
is
driving little endian hardware as found on x86).
But that is the job of the hardware drivers, isn't it? Here we are
taking frames passed from the guest to its virtio driver in the format
specified in the target cpu's endianness and QEMU as the device passes
it to host ALSA/Pipewire/etc which in turn passes it to the actual
audio hardware driver..
The problem is that the notion of target CPU endian is not fixed. For example
the
PowerPC CPU starts off in big-endian mode, but these days most systems will
switch
the CPU to little-endian mode on startup to run ppc64le. There's also the ILE
bit
which can be configured so that a big-endian PowerPC CPU can dynamically switch
to
little-endian mode when processing an interrupt, so you could potentially end
up with
either depending upon the current mode of the CPU.
These are the kinds of issues that led to the later virtio specifications simply
using little-endian for everything, since then there is zero ambiguity over what
endian is required for the virtio configuration space accesses.
It feels to me that assuming a target CPU endian is fixed for the PCM frame
formats
is simply repeating the mistakes of the past - and even the fact that we are
discussing this within this thread suggests that at a very minimum the
virtio-snd
specification needs to be updated to clarify the byte ordering of the PCM frame
formats.
ATB,
Mark.
Agreed, I think we are saying approximately the same thing here.
We need a mechanism to retrieve the vCPUs endianness and a way to
notify subscribed devices when it changes.
I don't think I agree, it's not the same thing.
Guest should just convert and send data in LE format.
Host should then convert from LE format.
Target endian-ness does not come into it.
That's not in the VIRTIO 1.2 spec. We are talking about supporting
things as they currently stand, not as they could have been.
Can you also clarify the particular case that you're trying to fix - is it big-endian
on ARM, or something else?
ATB,
Mark.
Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one
On 25/04/2024 11:04, Manos Pitsidianakis wrote:
On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
wrote:
On 25/04/2024 07:30, Manos Pitsidianakis wrote:
On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
wrote:
On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
On 23/4/24 11:18, Manos Pitsidianakis wrote:
On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
wrote:
On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
On 22/4/24 23:02, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.
Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé
This is all completely bogus. Virtio SND is from Virtio 1.0 only.
It is unconditionally little endian.
This part of the code is for PCM frames (raw bytes), not virtio spec
fields (which indeed must be LE in modern VIRTIO).
Thought a little more about it. We should keep the target's endianness
here, if it's mutable then we should query the machine the device is
attached to somehow. the virtio device should never change endianness
like Michael says since it's not legacy.
Grr. So as Richard suggested, this need to be pass as a device
property then.
(https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09...@linaro.org/)
It feels to me that the endianness is something that should be negotiated as
part of
the frame format, since the endianness of the audio hardware can be different
from
that of the CPU (think PReP machines where it was common that a big endian CPU
is
driving little endian hardware as found on x86).
But that is the job of the hardware drivers, isn't it? Here we are
taking frames passed from the guest to its virtio driver in the format
specified in the target cpu's endianness and QEMU as the device passes
it to host ALSA/Pipewire/etc which in turn passes it to the actual
audio hardware driver..
The problem is that the notion of target CPU endian is not fixed. For example
the
PowerPC CPU starts off in big-endian mode, but these days most systems will
switch
the CPU to little-endian mode on startup to run ppc64le. There's also the ILE
bit
which can be configured so that a big-endian PowerPC CPU can dynamically switch
to
little-endian mode when processing an interrupt, so you could potentially end
up with
either depending upon the current mode of the CPU.
These are the kinds of issues that led to the later virtio specifications simply
using little-endian for everything, since then there is zero ambiguity over what
endian is required for the virtio configuration space accesses.
It feels to me that assuming a target CPU endian is fixed for the PCM frame
formats
is simply repeating the mistakes of the past - and even the fact that we are
discussing this within this thread suggests that at a very minimum the
virtio-snd
specification needs to be updated to clarify the byte ordering of the PCM frame
formats.
Agreed, I think we are saying approximately the same thing here.
We need a mechanism to retrieve the vCPUs endianness and a way to
notify subscribed devices when it changes.
I think that then, since the virtio device is mostly certain of the
correct target endianness and completely certain of the host
endianness, it can perform the necessary conversions.
I don't recall seeing a restriction on the byte ordering of PCM
formats other than the CPU order; except for the ones which have
explicit endianness in their definitions. Please correct me if I am
wrong!
A straightforward solution would be to set an endianness change notify
callback in DeviceClass. What do you think?
Well, I guess... but why propose such a complex solution when you can simply specify
the endianness of the PCM frame? If you think back to pre-1.0 virtio where everything
was done using target vCPU endianness, why did they not implement this solution back
then to solve the problem instead of mandating that virtio-1.0 onwards should simply
use little-endian? Clearly specifying that all implementations should use
little-endian was the deemed to be the best way to solve all these issues.
Have you raised this issue on the virtio-dev mailing list? It's worth explaining the
problem you're trying to solve there, since hopefully the person or persons who
designed and proposed the virtio-snd specification will be able to give you the
correct advice.
ATB,
Mark.
Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one
On 25/04/2024 07:30, Manos Pitsidianakis wrote:
On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
wrote:
On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
On 23/4/24 11:18, Manos Pitsidianakis wrote:
On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
wrote:
On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
On 22/4/24 23:02, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.
Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé
This is all completely bogus. Virtio SND is from Virtio 1.0 only.
It is unconditionally little endian.
This part of the code is for PCM frames (raw bytes), not virtio spec
fields (which indeed must be LE in modern VIRTIO).
Thought a little more about it. We should keep the target's endianness
here, if it's mutable then we should query the machine the device is
attached to somehow. the virtio device should never change endianness
like Michael says since it's not legacy.
Grr. So as Richard suggested, this need to be pass as a device
property then.
(https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09...@linaro.org/)
It feels to me that the endianness is something that should be negotiated as
part of
the frame format, since the endianness of the audio hardware can be different
from
that of the CPU (think PReP machines where it was common that a big endian CPU
is
driving little endian hardware as found on x86).
But that is the job of the hardware drivers, isn't it? Here we are
taking frames passed from the guest to its virtio driver in the format
specified in the target cpu's endianness and QEMU as the device passes
it to host ALSA/Pipewire/etc which in turn passes it to the actual
audio hardware driver..
The problem is that the notion of target CPU endian is not fixed. For example the
PowerPC CPU starts off in big-endian mode, but these days most systems will switch
the CPU to little-endian mode on startup to run ppc64le. There's also the ILE bit
which can be configured so that a big-endian PowerPC CPU can dynamically switch to
little-endian mode when processing an interrupt, so you could potentially end up with
either depending upon the current mode of the CPU.
These are the kinds of issues that led to the later virtio specifications simply
using little-endian for everything, since then there is zero ambiguity over what
endian is required for the virtio configuration space accesses.
It feels to me that assuming a target CPU endian is fixed for the PCM frame formats
is simply repeating the mistakes of the past - and even the fact that we are
discussing this within this thread suggests that at a very minimum the virtio-snd
specification needs to be updated to clarify the byte ordering of the PCM frame formats.
ATB,
Mark.
Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one
On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
On 23/4/24 11:18, Manos Pitsidianakis wrote:
On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
wrote:
On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
On 22/4/24 23:02, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.
Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé
This is all completely bogus. Virtio SND is from Virtio 1.0 only.
It is unconditionally little endian.
This part of the code is for PCM frames (raw bytes), not virtio spec
fields (which indeed must be LE in modern VIRTIO).
Thought a little more about it. We should keep the target's endianness
here, if it's mutable then we should query the machine the device is
attached to somehow. the virtio device should never change endianness
like Michael says since it's not legacy.
Grr. So as Richard suggested, this need to be pass as a device
property then.
(https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09...@linaro.org/)
It feels to me that the endianness is something that should be negotiated as part of
the frame format, since the endianness of the audio hardware can be different from
that of the CPU (think PReP machines where it was common that a big endian CPU is
driving little endian hardware as found on x86).
My feeling is that either the VIRTIO_SND_PCM_FMT_* constants should be extended to
have both _LE and _BE variants, or all frame formats are defined to always be little
endian.
ATB,
Mark.
Re: [PATCH] target/i386/translate.c: always write 32-bits for SGDT and SIDT
On 23/04/2024 10:18, Paolo Bonzini wrote: On Mon, Apr 22, 2024 at 9:10 PM Volker Rümelin wrote: Am 20.04.24 um 07:40 schrieb Mark Cave-Ayland: Current documentation agrees that all 32 bits are written, so I don't think you need this comment: Ah that's good to know the docs are now correct. I added the comment as there was a lot of conflicting information around for older CPUs so I thought it was worth an explicit mention. Quote from the Intel® 64 and IA-32 Architectures Software Developer’s Manual Volume 2B: Instruction Set Reference, M-U March 2024: IA-32 Architecture Compatibility The 16-bit form of SGDT is compatible with the Intel 286 processor if the upper 8 bits are not referenced. The Intel 286 processor fills these bits with 1s; processor generations later than the Intel 286 processor fill these bits with 0s. Intel still claims the upper 8 bits are filled with 0s, but the Operation pseudo code below is correct. The same is true for SIDT. I think the claim is that it fills with 0s when the software is compatible with the 286, i.e. never uses a 32-bit LIDT or LGDT instruction. Software written to target specifically older processors typically used the undocumented LOADALL instruction to exit protected mode or to set 4GB segment limits, so it won't run on QEMU. You can read about the usage here: https://www.os2museum.com/wp/more-on-loadall-and-os2/ (286) https://www.os2museum.com/wp/386-loadall/ (386) and about how it worked here: https://www.pcjs.org/documents/manuals/intel/80286/loadall/ https://www.pcjs.org/documents/manuals/intel/80386/loadall/ Interestingly, byte 3 of the GDTR or IDTR on the 286 are documented as "should be zeroes" for LOADALL, not all ones. Let's change "Despite claims to the contrary" with "Despite a confusing description". Thanks for sorting this, Paolo. I suspect that KVM needs a similar patch as per https://gitlab.com/qemu-project/qemu/-/issues/2198#note_1815726425 however the Win32s and OS/2 Warp 4 tests seem to work fine on KVM. Maybe it's because the SGDT and SIDT instructions run natively and don't need to be emulated for these cases? ATB, Mark.
Re: [PATCH 4/5] docs/system/target-sparc: Improve the Sparc documentation
On 20/04/2024 00:14, Brad Smith wrote: On 2024-04-18 4:27 p.m., Mark Cave-Ayland wrote: On 07/03/2024 17:43, Thomas Huth wrote: Add some words about how to enable or disable boolean features, and remove the note about a Linux kernel being available on the QEMU website (they have been removed long ago already). Signed-off-by: Thomas Huth --- docs/system/target-sparc.rst | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/system/target-sparc.rst b/docs/system/target-sparc.rst index 9ec8c90c14..9f418b9d3e 100644 --- a/docs/system/target-sparc.rst +++ b/docs/system/target-sparc.rst @@ -27,6 +27,11 @@ architecture machines: The emulation is somewhat complete. SMP up to 16 CPUs is supported, but Linux limits the number of usable CPUs to 4. +The list of available CPUs can be viewed by starting QEMU with ``-cpu help``. +Optional boolean features can be added with a "+" in front of the feature name, +or disabled with a "-" in front of the name, for example +``-cpu TI-SuperSparc-II,+float128``. + QEMU emulates the following sun4m peripherals: - IOMMU @@ -55,8 +60,7 @@ OpenBIOS is a free (GPL v2) portable firmware implementation. The goal is to implement a 100% IEEE 1275-1994 (referred to as Open Firmware) compliant firmware. -A sample Linux 2.6 series kernel and ram disk image are available on the -QEMU web site. There are still issues with NetBSD and OpenBSD, but most +There are still issues with NetBSD and OpenBSD, but most kernel versions work. Please note that currently older Solaris kernels don't work probably due to interface issues between OpenBIOS and Solaris. Just curious as to what current issues exist with NetBSD and OpenBSD? At least both my NetBSD and OpenBSD test images survive a casual boot test here with latest git. I was just trying OpenBSD/sparc64 with 8.2 recently and found hme(4) does not work. I tried with the NE2k driver as I remember adding the driver to the OpenBSD kernel before an hme driver existed and it sort of worked, but there were still issues. I'll re-test with 9 now and see what happens. Thanks for the update: my local tests for SPARC changes are boot tests, so it's entirely possible I could miss an issue with the hme device. Feel free to open a GitLab issue with the relevant information and I'll take a look as time allows. ATB, Mark.
Re: [PATCH] target/i386/translate.c: always write 32-bits for SGDT and SIDT
On 20/04/2024 02:21, Richard Henderson wrote:
On 4/19/24 12:51, Mark Cave-Ayland wrote:
The various Intel CPU manuals claim that SGDT and SIDT can write either 24-bits
or 32-bits depending upon the operand size, but this is incorrect. Not only do
the Intel CPU manuals give contradictory information between processor
revisions, but this information doesn't even match real-life behaviour.
In fact, tests on real hardware show that the CPU always writes 32-bits for SGDT
and SIDT, and this behaviour is required for at least OS/2 Warp and WFW 3.11
with
Win32s to function correctly. Remove the masking applied due to the operand size
for SGDT and SIDT so that the TCG behaviour matches the behaviour on real
hardware.
Signed-off-by: Mark Cave-Ayland
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2198
--
MCA: Whilst I don't have a copy of OS/2 Warp handy, I've confirmed that this
patch fixes the issue in WFW 3.11 with Win32s. For more technical information I
highly recommend the excellent write-up at
https://www.os2museum.com/wp/sgdtsidt-fiction-and-reality/.
---
target/i386/tcg/translate.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 76a42c679c..3026eb6774 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -5846,9 +5846,10 @@ static bool disas_insn(DisasContext *s, CPUState *cpu)
gen_op_st_v(s, MO_16, s->T0, s->A0);
gen_add_A0_im(s, 2);
tcg_gen_ld_tl(s->T0, tcg_env, offsetof(CPUX86State, gdt.base));
- if (dflag == MO_16) {
- tcg_gen_andi_tl(s->T0, s->T0, 0xff);
- }
+ /*
+ * NB: Despite claims to the contrary in Intel CPU documentation,
+ * all 32-bits are written regardless of operand size.
+ */
Current documentation agrees that all 32 bits are written, so I don't think you need
this comment:
Ah that's good to know the docs are now correct. I added the comment as there was a
lot of conflicting information around for older CPUs so I thought it was worth an
explicit mention.
If everyone agrees a version without comments is preferable, I can re-send an updated
version without them included.
IF OperandSize =16 or OperandSize = 32 (* Legacy or Compatibility Mode *)
THEN
DEST[0:15] := GDTR(Limit);
DEST[16:47] := GDTR(Base); (* Full 32-bit base address stored *)
FI;
Anyway,
Reviewed-by: Richard Henderson
Thanks!
ATB,
Mark.
[PATCH] target/i386/translate.c: always write 32-bits for SGDT and SIDT
The various Intel CPU manuals claim that SGDT and SIDT can write either 24-bits
or 32-bits depending upon the operand size, but this is incorrect. Not only do
the Intel CPU manuals give contradictory information between processor
revisions, but this information doesn't even match real-life behaviour.
In fact, tests on real hardware show that the CPU always writes 32-bits for SGDT
and SIDT, and this behaviour is required for at least OS/2 Warp and WFW 3.11
with
Win32s to function correctly. Remove the masking applied due to the operand size
for SGDT and SIDT so that the TCG behaviour matches the behaviour on real
hardware.
Signed-off-by: Mark Cave-Ayland
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2198
--
MCA: Whilst I don't have a copy of OS/2 Warp handy, I've confirmed that this
patch fixes the issue in WFW 3.11 with Win32s. For more technical information I
highly recommend the excellent write-up at
https://www.os2museum.com/wp/sgdtsidt-fiction-and-reality/.
---
target/i386/tcg/translate.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 76a42c679c..3026eb6774 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -5846,9 +5846,10 @@ static bool disas_insn(DisasContext *s, CPUState *cpu)
gen_op_st_v(s, MO_16, s->T0, s->A0);
gen_add_A0_im(s, 2);
tcg_gen_ld_tl(s->T0, tcg_env, offsetof(CPUX86State, gdt.base));
-if (dflag == MO_16) {
-tcg_gen_andi_tl(s->T0, s->T0, 0xff);
-}
+/*
+ * NB: Despite claims to the contrary in Intel CPU documentation,
+ * all 32-bits are written regardless of operand size.
+ */
gen_op_st_v(s, CODE64(s) + MO_32, s->T0, s->A0);
break;
@@ -5901,9 +5902,10 @@ static bool disas_insn(DisasContext *s, CPUState *cpu)
gen_op_st_v(s, MO_16, s->T0, s->A0);
gen_add_A0_im(s, 2);
tcg_gen_ld_tl(s->T0, tcg_env, offsetof(CPUX86State, idt.base));
-if (dflag == MO_16) {
-tcg_gen_andi_tl(s->T0, s->T0, 0xff);
-}
+/*
+ * NB: Despite claims to the contrary in Intel CPU documentation,
+ * all 32-bits are written regardless of operand size.
+ */
gen_op_st_v(s, CODE64(s) + MO_32, s->T0, s->A0);
break;
--
2.39.2
Re: [PATCH v2 3/4] docs/system/target-sparc: Improve the Sparc documentation
On 19/04/2024 09:48, Thomas Huth wrote: Add some words about how to enable or disable boolean features, and remove the note about a Linux kernel being available on the QEMU website (they have been removed long ago already), and the note about NetBSD and OpenBSD still having issues (they should work fine nowadays). Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2141 Signed-off-by: Thomas Huth --- docs/system/target-sparc.rst | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/system/target-sparc.rst b/docs/system/target-sparc.rst index 9ec8c90c14..54bd8b6ead 100644 --- a/docs/system/target-sparc.rst +++ b/docs/system/target-sparc.rst @@ -27,6 +27,11 @@ architecture machines: The emulation is somewhat complete. SMP up to 16 CPUs is supported, but Linux limits the number of usable CPUs to 4. +The list of available CPUs can be viewed by starting QEMU with ``-cpu help``. +Optional boolean features can be added with a "+" in front of the feature name, +or disabled with a "-" in front of the name, for example +``-cpu TI-SuperSparc-II,+float128``. + QEMU emulates the following sun4m peripherals: - IOMMU @@ -55,8 +60,5 @@ OpenBIOS is a free (GPL v2) portable firmware implementation. The goal is to implement a 100% IEEE 1275-1994 (referred to as Open Firmware) compliant firmware. -A sample Linux 2.6 series kernel and ram disk image are available on the -QEMU web site. There are still issues with NetBSD and OpenBSD, but most -kernel versions work. Please note that currently older Solaris kernels -don't work probably due to interface issues between OpenBIOS and -Solaris. +Please note that currently older Solaris kernels don't work probably due +to interface issues between OpenBIOS and Solaris. Reviewed-by: Mark Cave-Ayland ATB, Mark.
Re: [PATCH 4/5] docs/system/target-sparc: Improve the Sparc documentation
On 19/04/2024 05:59, Thomas Huth wrote: On 18/04/2024 22.27, Mark Cave-Ayland wrote: On 07/03/2024 17:43, Thomas Huth wrote: Add some words about how to enable or disable boolean features, and remove the note about a Linux kernel being available on the QEMU website (they have been removed long ago already). Signed-off-by: Thomas Huth --- docs/system/target-sparc.rst | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/system/target-sparc.rst b/docs/system/target-sparc.rst index 9ec8c90c14..9f418b9d3e 100644 --- a/docs/system/target-sparc.rst +++ b/docs/system/target-sparc.rst @@ -27,6 +27,11 @@ architecture machines: The emulation is somewhat complete. SMP up to 16 CPUs is supported, but Linux limits the number of usable CPUs to 4. +The list of available CPUs can be viewed by starting QEMU with ``-cpu help``. +Optional boolean features can be added with a "+" in front of the feature name, +or disabled with a "-" in front of the name, for example +``-cpu TI-SuperSparc-II,+float128``. + QEMU emulates the following sun4m peripherals: - IOMMU @@ -55,8 +60,7 @@ OpenBIOS is a free (GPL v2) portable firmware implementation. The goal is to implement a 100% IEEE 1275-1994 (referred to as Open Firmware) compliant firmware. -A sample Linux 2.6 series kernel and ram disk image are available on the -QEMU web site. There are still issues with NetBSD and OpenBSD, but most +There are still issues with NetBSD and OpenBSD, but most kernel versions work. Please note that currently older Solaris kernels don't work probably due to interface issues between OpenBIOS and Solaris. Just curious as to what current issues exist with NetBSD and OpenBSD? At least both my NetBSD and OpenBSD test images survive a casual boot test here with latest git. Maybe it's also about a long fixed bug ... shall I remove that sentence while I'm at it? Yeah, I don't believe that has been true about NetBSD/OpenBSD for quite some time :) Please do keep the wording about Solaris since I get asked that often (and it's a task that will be quite difficult and time-consuming to do). ATB, Mark.
[PATCH] hw/sparc64: set iommu_platform=on for virtio devices attached to the sun4u machine
The sun4u machine has an IOMMU and therefore it is possible to program it such
that the virtio-device IOVA does not map directly to the CPU physical address.
This is not a problem with Linux which always maps the IOVA directly to the CPU
physical address, however it is required for the NetBSD virtio driver where this
is not the case.
Set the sun4u machine defaults for all virtio devices so that disable-legacy=on
and iommu_platform=on to ensure a default configuration will allow virtio
devices to function correctly on both Linux and NetBSD.
Signed-off-by: Mark Cave-Ayland
---
hw/sparc64/sun4u.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index cff6d5abaf..4ece1ac1ff 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -793,6 +793,12 @@ static void sun4v_init(MachineState *machine)
sun4uv_init(get_system_memory(), machine, [1]);
}
+static GlobalProperty hw_compat_sparc64[] = {
+{ "virtio-pci", "disable-legacy", "on", .optional = true },
+{ "virtio-device", "iommu_platform", "on" },
+};
+static const size_t hw_compat_sparc64_len = G_N_ELEMENTS(hw_compat_sparc64);
+
static void sun4u_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -810,6 +816,7 @@ static void sun4u_class_init(ObjectClass *oc, void *data)
mc->default_nic = "sunhme";
mc->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
fwc->get_dev_path = sun4u_fw_dev_path;
+compat_props_add(mc->compat_props, hw_compat_sparc64,
hw_compat_sparc64_len);
}
static const TypeInfo sun4u_type = {
--
2.39.2
Re: [PATCH 0/5] Sparc CPU naming and help text improvements
On 18/04/2024 21:08, Mark Cave-Ayland wrote: On 15/04/2024 08:26, Thomas Huth wrote: On 07/03/2024 18.43, Thomas Huth wrote: The Sparc CPU naming and the corresponding help text is somewhat confusing for the users. We should avoid spaces in the Names and provide clear information to the users what can be passed to the "-cpu" option. While we're at it, also remove the "+" from two of the CPU names since this character is now not allowed in device names anymore (and was worked around with an ugly hack in qom/object.c so far). Thomas Huth (5): target/sparc/cpu: Rename the CPU models with a "+" in their names target/sparc/cpu: Avoid spaces by default in the CPU names target/sparc/cpu: Improve the CPU help text docs/system/target-sparc: Improve the Sparc documentation docs/about: Deprecate the old "UltraSparc" CPU names that contain a "+" Ping! Mark, Aryom, could you please comment on this patch series, too? Thanks, Thomas Done! I didn't realise that it was only patches 1 and 2 that were still outstanding until I tested the series, so I've replied to those separately. Oops actually that's not quite right: looks like my git-am failed on patch 3 because it was already applied, but the remaining 2 patches are still relevant. I've just replied to those too. ATB, Mark.
Re: [PATCH 5/5] docs/about: Deprecate the old "UltraSparc" CPU names that contain a "+"
On 07/03/2024 17:43, Thomas Huth wrote: For consistency we should drop the names with a "+" in it in the long run. Signed-off-by: Thomas Huth --- docs/about/deprecated.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 8565644da6..7058341f8f 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -202,6 +202,15 @@ in the QEMU object model anymore. ``power5+``, ``power5+_v2.1``, an alias, but for consistency these will get removed in a future release, too. Use ``power5p_v2.1`` and ``power7p_v2.1`` instead. +``Sun-UltraSparc-IIIi+`` and ``Sun-UltraSparc-IV+`` CPU names (since 9.0) +' + +The character "+" in device (and thus also CPU) names is not allowed +in the QEMU object model anymore. ``Sun-UltraSparc-IIIi+`` and +``Sun-UltraSparc-IV+`` are currently still supported via a workaround, +but for consistency these will get removed in a future release, too. +Use ``Sun-UltraSparc-IIIip`` and ``Sun-UltraSparc-IVp`` instead. + CRIS CPU architecture (since 9.0) ' See my previous comment about the CPU names, otherwise: Reviewed-by: Mark Cave-Ayland ATB, Mark.
Re: [PATCH 4/5] docs/system/target-sparc: Improve the Sparc documentation
On 07/03/2024 17:43, Thomas Huth wrote: Add some words about how to enable or disable boolean features, and remove the note about a Linux kernel being available on the QEMU website (they have been removed long ago already). Signed-off-by: Thomas Huth --- docs/system/target-sparc.rst | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/system/target-sparc.rst b/docs/system/target-sparc.rst index 9ec8c90c14..9f418b9d3e 100644 --- a/docs/system/target-sparc.rst +++ b/docs/system/target-sparc.rst @@ -27,6 +27,11 @@ architecture machines: The emulation is somewhat complete. SMP up to 16 CPUs is supported, but Linux limits the number of usable CPUs to 4. +The list of available CPUs can be viewed by starting QEMU with ``-cpu help``. +Optional boolean features can be added with a "+" in front of the feature name, +or disabled with a "-" in front of the name, for example +``-cpu TI-SuperSparc-II,+float128``. + QEMU emulates the following sun4m peripherals: - IOMMU @@ -55,8 +60,7 @@ OpenBIOS is a free (GPL v2) portable firmware implementation. The goal is to implement a 100% IEEE 1275-1994 (referred to as Open Firmware) compliant firmware. -A sample Linux 2.6 series kernel and ram disk image are available on the -QEMU web site. There are still issues with NetBSD and OpenBSD, but most +There are still issues with NetBSD and OpenBSD, but most kernel versions work. Please note that currently older Solaris kernels don't work probably due to interface issues between OpenBIOS and Solaris. Just curious as to what current issues exist with NetBSD and OpenBSD? At least both my NetBSD and OpenBSD test images survive a casual boot test here with latest git. ATB, Mark.
Re: [PATCH 0/5] Sparc CPU naming and help text improvements
On 15/04/2024 08:26, Thomas Huth wrote: On 07/03/2024 18.43, Thomas Huth wrote: The Sparc CPU naming and the corresponding help text is somewhat confusing for the users. We should avoid spaces in the Names and provide clear information to the users what can be passed to the "-cpu" option. While we're at it, also remove the "+" from two of the CPU names since this character is now not allowed in device names anymore (and was worked around with an ugly hack in qom/object.c so far). Thomas Huth (5): target/sparc/cpu: Rename the CPU models with a "+" in their names target/sparc/cpu: Avoid spaces by default in the CPU names target/sparc/cpu: Improve the CPU help text docs/system/target-sparc: Improve the Sparc documentation docs/about: Deprecate the old "UltraSparc" CPU names that contain a "+" Ping! Mark, Aryom, could you please comment on this patch series, too? Thanks, Thomas Done! I didn't realise that it was only patches 1 and 2 that were still outstanding until I tested the series, so I've replied to those separately. ATB, Mark.
Re: [PATCH 1/5] target/sparc/cpu: Rename the CPU models with a "+" in their names
On 07/03/2024 17:43, Thomas Huth wrote:
Commit b447378e12 ("qom/object: Limit type names to alphanumerical ...")
cut down the amount of allowed characters for QOM types to a saner set.
The "+" character was not meant to be included in this set, so we had
to add a hack there to still allow the legacy names of POWER and Sparc64
CPUs. However, instead of putting such a hack in the common QOM code,
there is a much better place to do this: The sparc_cpu_class_by_name()
function which is used to look up the names of all Sparc CPUs.
Thus let's finally get rid of the "+" in the Sparc CPU names, and provide
backward compatibility for the old names via some simple checks in the
sparc_cpu_class_by_name() function.
Signed-off-by: Thomas Huth
---
qom/object.c | 8
target/sparc/cpu.c | 14 --
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index d4a001cf41..759e194972 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -158,14 +158,6 @@ static bool type_name_is_valid(const char *name)
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
"0123456789-_.");
-/* Allow some legacy names with '+' in it for compatibility reasons */
-if (name[plen] == '+') {
-if (plen >= 17 && g_str_has_prefix(name, "Sun-UltraSparc-I")) {
-/* Allow "Sun-UltraSparc-IV+" and "Sun-UltraSparc-IIIi+" */
-return true;
-}
-}
-
return plen == slen;
}
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 313ebc4c11..651e49bfeb 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -316,7 +316,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "Sun UltraSparc IV+",
+.name = "Sun UltraSparc IVp",
.iu_version = ((0x3eULL << 48) | (0x19ULL << 32) | (0x22ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_12,
@@ -325,7 +325,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES | CPU_FEATURE_CMT,
},
{
-.name = "Sun UltraSparc IIIi+",
+.name = "Sun UltraSparc IIIip",
.iu_version = ((0x3eULL << 48) | (0x22ULL << 32) | (0ULL << 24)),
.fpu_version = 0x,
.mmu_version = mmu_us_3,
@@ -767,6 +767,16 @@ static ObjectClass *sparc_cpu_class_by_name(const char
*cpu_model)
char *typename;
typename = sparc_cpu_type_name(cpu_model);
+
+/* Fix up legacy names with '+' in it */
+if (g_str_equal(typename, SPARC_CPU_TYPE_NAME("Sun-UltraSparc-IV+"))) {
+g_free(typename);
+typename = g_strdup(SPARC_CPU_TYPE_NAME("Sun-UltraSparc-IVp"));
+} else if (g_str_equal(typename,
SPARC_CPU_TYPE_NAME("Sun-UltraSparc-IIIi+"))) {
+g_free(typename);
+typename = g_strdup(SPARC_CPU_TYPE_NAME("Sun-UltraSparc-IIIip"));
+}
+
oc = object_class_by_name(typename);
g_free(typename);
return oc;
I've seen some references in Sun documentation to processors in the form "UltraSparc
IIIi plus" so I'd be inclined to use that form for the new type names e.g.
"UltraSparc-IIIi-plus".
Otherwise looks good to me, thanks for having a look at this!
Reviewed-by: Mark Cave-Ayland
ATB,
Mark.
Re: [PATCH 2/5] target/sparc/cpu: Avoid spaces by default in the CPU names
.name = "TI-SuperSparc-40", /* STP1020NPGA */
.iu_version = 0x4100, /* SuperSPARC 2.x */
.fpu_version = 0 << FSR_VER_SHIFT,
.mmu_version = 0x0800, /* SuperSPARC 2.x, no MXCC */
@@ -444,7 +444,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "TI SuperSparc 50", /* STP1020PGA */
+.name = "TI-SuperSparc-50", /* STP1020PGA */
.iu_version = 0x4000, /* SuperSPARC 3.x */
.fpu_version = 0 << FSR_VER_SHIFT,
.mmu_version = 0x01000800, /* SuperSPARC 3.x, no MXCC */
@@ -457,7 +457,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "TI SuperSparc 51",
+.name = "TI-SuperSparc-51",
.iu_version = 0x4000, /* SuperSPARC 3.x */
.fpu_version = 0 << FSR_VER_SHIFT,
.mmu_version = 0x0100, /* SuperSPARC 3.x, MXCC */
@@ -471,7 +471,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "TI SuperSparc 60", /* STP1020APGA */
+.name = "TI-SuperSparc-60", /* STP1020APGA */
.iu_version = 0x4000, /* SuperSPARC 3.x */
.fpu_version = 0 << FSR_VER_SHIFT,
.mmu_version = 0x01000800, /* SuperSPARC 3.x, no MXCC */
@@ -484,7 +484,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "TI SuperSparc 61",
+.name = "TI-SuperSparc-61",
.iu_version = 0x4400, /* SuperSPARC 3.x */
.fpu_version = 0 << FSR_VER_SHIFT,
.mmu_version = 0x0100, /* SuperSPARC 3.x, MXCC */
@@ -498,7 +498,7 @@ static const sparc_def_t sparc_defs[] = {
.features = CPU_DEFAULT_FEATURES,
},
{
-.name = "TI SuperSparc II",
+.name = "TI-SuperSparc-II",
.iu_version = 0x4000, /* SuperSPARC II 1.x */
.fpu_version = 0 << FSR_VER_SHIFT,
.mmu_version = 0x0800, /* SuperSPARC II 1.x, MXCC */
Thanks Thomas, this looks much better!
Reviewed-by: Mark Cave-Ayland
ATB,
Mark.
Re: [PATCH] target/sparc: Use GET_ASI_CODE for ASI_KERNELTXT and ASI_USERTXT
default:
+g_assert_not_reached();
+}
+return ret;
+}
+
#endif /* CONFIG_USER_ONLY */
#else /* TARGET_SPARC64 */
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 319934d9bd..c9b9b047df 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -1117,6 +1117,7 @@ typedef enum {
GET_ASI_EXCP,
GET_ASI_DIRECT,
GET_ASI_DTWINX,
+GET_ASI_CODE,
GET_ASI_BLOCK,
GET_ASI_SHORT,
GET_ASI_BCOPY,
@@ -1159,14 +1160,22 @@ static DisasASI resolve_asi(DisasContext *dc, int asi,
MemOp memop)
|| (asi == ASI_USERDATA
&& (dc->def->features & CPU_FEATURE_CASA))) {
switch (asi) {
-case ASI_USERDATA: /* User data access */
+case ASI_USERDATA:/* User data access */
mem_idx = MMU_USER_IDX;
type = GET_ASI_DIRECT;
break;
-case ASI_KERNELDATA: /* Supervisor data access */
+case ASI_KERNELDATA: /* Supervisor data access */
mem_idx = MMU_KERNEL_IDX;
type = GET_ASI_DIRECT;
break;
+case ASI_USERTXT: /* User text access */
+mem_idx = MMU_USER_IDX;
+type = GET_ASI_CODE;
+break;
+case ASI_KERNELTXT: /* Supervisor text access */
+mem_idx = MMU_KERNEL_IDX;
+type = GET_ASI_CODE;
+break;
case ASI_M_BYPASS:/* MMU passthrough */
case ASI_LEON_BYPASS: /* LEON MMU passthrough */
mem_idx = MMU_PHYS_IDX;
@@ -1379,6 +1388,22 @@ static void gen_ld_asi(DisasContext *dc, DisasASI *da,
TCGv dst, TCGv addr)
case GET_ASI_DIRECT:
tcg_gen_qemu_ld_tl(dst, addr, da->mem_idx, da->memop | MO_ALIGN);
break;
+
+case GET_ASI_CODE:
+#if !defined(CONFIG_USER_ONLY) && !defined(TARGET_SPARC64)
+{
+MemOpIdx oi = make_memop_idx(da->memop, da->mem_idx);
+TCGv_i32 r_oi = tcg_constant_i32(oi);
+TCGv_i64 t64 = tcg_temp_new_i64();
+
+gen_helper_ld_code(t64, tcg_env, addr, r_oi);
+tcg_gen_trunc_i64_tl(dst, t64);
+}
+break;
+#else
+g_assert_not_reached();
+#endif
+
default:
{
TCGv_i32 r_asi = tcg_constant_i32(da->asi);
@@ -1791,6 +1816,26 @@ static void gen_ldda_asi(DisasContext *dc, DisasASI *da,
TCGv addr, int rd)
}
break;
+case GET_ASI_CODE:
+#if !defined(CONFIG_USER_ONLY) && !defined(TARGET_SPARC64)
+{
+TCGv_i64 tmp = tcg_temp_new_i64();
+MemOpIdx oi = make_memop_idx(da->memop, da->mem_idx);
+
+gen_helper_ld_code(tmp, tcg_env, addr, tcg_constant_i32(oi));
+
+/* See above. */
+if ((da->memop & MO_BSWAP) == MO_TE) {
+tcg_gen_extr_i64_tl(lo, hi, tmp);
+} else {
+tcg_gen_extr_i64_tl(hi, lo, tmp);
+}
+}
+break;
+#else
+g_assert_not_reached();
+#endif
+
default:
/* ??? In theory we've handled all of the ASIs that are valid
for ldda, and this should raise DAE_invalid_asi. However,
Thanks for the excellent analysis, and also thanks to Richard for improving the
correctness of the patch:
Acked-by: Mark Cave-Ayland
ATB,
Mark.
[PULL 07/17] esp.c: use esp_fifo_push() instead of fifo8_push()
There are still a few places that use fifo8_push() instead of esp_fifo_push() in
order to push a value into the FIFO. Update those places to use esp_fifo_push()
instead.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paolo Bonzini
Message-Id: <20240324191707.623175-8-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index d474268438..8d2d36d56c 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -858,7 +858,7 @@ static void esp_do_nodma(ESPState *s)
return;
}
if (fifo8_is_empty(>fifo)) {
-fifo8_push(>fifo, s->async_buf[0]);
+esp_fifo_push(s, s->async_buf[0]);
s->async_buf++;
s->async_len--;
s->ti_size--;
@@ -881,7 +881,7 @@ static void esp_do_nodma(ESPState *s)
case STAT_ST:
switch (s->rregs[ESP_CMD]) {
case CMD_ICCS:
-fifo8_push(>fifo, s->status);
+esp_fifo_push(s, s->status);
esp_set_phase(s, STAT_MI);
/* Process any message in phase data */
@@ -893,7 +893,7 @@ static void esp_do_nodma(ESPState *s)
case STAT_MI:
switch (s->rregs[ESP_CMD]) {
case CMD_ICCS:
-fifo8_push(>fifo, 0);
+esp_fifo_push(s, 0);
/* Raise end of command interrupt */
s->rregs[ESP_RINTR] |= INTR_FC;
--
2.39.2
[PULL 08/17] esp.c: change esp_fifo_pop_buf() to take ESPState
Now that all users of esp_fifo_pop_buf() operate on the main FIFO there is no
need to pass the FIFO explicitly.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paolo Bonzini
Message-Id: <20240324191707.623175-9-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 8d2d36d56c..83b621ee0f 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -155,9 +155,9 @@ static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t
*dest, int maxlen)
return n;
}
-static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
+static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen)
{
-return esp_fifo8_pop_buf(fifo, dest, maxlen);
+return esp_fifo8_pop_buf(>fifo, dest, maxlen);
}
static uint32_t esp_get_tc(ESPState *s)
@@ -459,7 +459,7 @@ static void esp_do_dma(ESPState *s)
s->dma_memory_read(s->dma_opaque, buf, len);
esp_set_tc(s, esp_get_tc(s) - len);
} else {
-len = esp_fifo_pop_buf(>fifo, buf, fifo8_num_used(>fifo));
+len = esp_fifo_pop_buf(s, buf, fifo8_num_used(>fifo));
len = MIN(fifo8_num_free(>cmdfifo), len);
esp_raise_drq(s);
}
@@ -515,7 +515,7 @@ static void esp_do_dma(ESPState *s)
fifo8_push_all(>cmdfifo, buf, len);
esp_set_tc(s, esp_get_tc(s) - len);
} else {
-len = esp_fifo_pop_buf(>fifo, buf, fifo8_num_used(>fifo));
+len = esp_fifo_pop_buf(s, buf, fifo8_num_used(>fifo));
len = MIN(fifo8_num_free(>cmdfifo), len);
fifo8_push_all(>cmdfifo, buf, len);
esp_raise_drq(s);
@@ -549,7 +549,7 @@ static void esp_do_dma(ESPState *s)
/* Copy FIFO data to device */
len = MIN(s->async_len, ESP_FIFO_SZ);
len = MIN(len, fifo8_num_used(>fifo));
-len = esp_fifo_pop_buf(>fifo, s->async_buf, len);
+len = esp_fifo_pop_buf(s, s->async_buf, len);
esp_raise_drq(s);
}
@@ -713,7 +713,7 @@ static void esp_nodma_ti_dataout(ESPState *s)
}
len = MIN(s->async_len, ESP_FIFO_SZ);
len = MIN(len, fifo8_num_used(>fifo));
-esp_fifo_pop_buf(>fifo, s->async_buf, len);
+esp_fifo_pop_buf(s, s->async_buf, len);
s->async_buf += len;
s->async_len -= len;
s->ti_size += len;
@@ -738,7 +738,7 @@ static void esp_do_nodma(ESPState *s)
switch (s->rregs[ESP_CMD]) {
case CMD_SELATN:
/* Copy FIFO into cmdfifo */
-len = esp_fifo_pop_buf(>fifo, buf, fifo8_num_used(>fifo));
+len = esp_fifo_pop_buf(s, buf, fifo8_num_used(>fifo));
len = MIN(fifo8_num_free(>cmdfifo), len);
fifo8_push_all(>cmdfifo, buf, len);
@@ -757,7 +757,7 @@ static void esp_do_nodma(ESPState *s)
case CMD_SELATNS:
/* Copy one byte from FIFO into cmdfifo */
-len = esp_fifo_pop_buf(>fifo, buf, 1);
+len = esp_fifo_pop_buf(s, buf, 1);
len = MIN(fifo8_num_free(>cmdfifo), len);
fifo8_push_all(>cmdfifo, buf, len);
@@ -774,7 +774,7 @@ static void esp_do_nodma(ESPState *s)
case CMD_TI:
/* Copy FIFO into cmdfifo */
-len = esp_fifo_pop_buf(>fifo, buf, fifo8_num_used(>fifo));
+len = esp_fifo_pop_buf(s, buf, fifo8_num_used(>fifo));
len = MIN(fifo8_num_free(>cmdfifo), len);
fifo8_push_all(>cmdfifo, buf, len);
@@ -792,7 +792,7 @@ static void esp_do_nodma(ESPState *s)
switch (s->rregs[ESP_CMD]) {
case CMD_TI:
/* Copy FIFO into cmdfifo */
-len = esp_fifo_pop_buf(>fifo, buf, fifo8_num_used(>fifo));
+len = esp_fifo_pop_buf(s, buf, fifo8_num_used(>fifo));
len = MIN(fifo8_num_free(>cmdfifo), len);
fifo8_push_all(>cmdfifo, buf, len);
@@ -821,7 +821,7 @@ static void esp_do_nodma(ESPState *s)
case CMD_SEL | CMD_DMA:
case CMD_SELATN | CMD_DMA:
/* Copy FIFO into cmdfifo */
-len = esp_fifo_pop_buf(>fifo, buf, fifo8_num_used(>fifo));
+len = esp_fifo_pop_buf(s, buf, fifo8_num_used(>fifo));
len = MIN(fifo8_num_free(>cmdfifo), len);
fifo8_push_all(>cmdfifo, buf, len);
@@ -836,7 +836,7 @@ static void esp_do_nodma(ESPState *s)
case CMD_SEL:
case CMD_SELATN:
/* FIFO already contain entire CDB: copy to cmdfifo and execute */
-len = esp_fifo_pop_buf(>fifo, buf, fifo8_num_used(>fifo));
+len = esp_fifo_pop_buf(s, buf, fifo8_num_used(>fifo));
len = MIN(fifo8_num_free(>cmdfifo), len);
fifo8_push_all(>cmdfifo, buf, len);
--
2.39.2
[PULL 13/17] esp.c: move esp_set_phase() and esp_get_phase() towards the beginning of the file
This allows these functions to be used earlier in the file without needing a
separate forward declaration.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paolo Bonzini
Message-Id: <20240324191707.623175-14-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index d8db33b921..9e35c00927 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -79,6 +79,24 @@ static void esp_lower_drq(ESPState *s)
}
}
+static const char *esp_phase_names[8] = {
+"DATA OUT", "DATA IN", "COMMAND", "STATUS",
+"(reserved)", "(reserved)", "MESSAGE OUT", "MESSAGE IN"
+};
+
+static void esp_set_phase(ESPState *s, uint8_t phase)
+{
+s->rregs[ESP_RSTAT] &= ~7;
+s->rregs[ESP_RSTAT] |= phase;
+
+trace_esp_set_phase(esp_phase_names[phase]);
+}
+
+static uint8_t esp_get_phase(ESPState *s)
+{
+return s->rregs[ESP_RSTAT] & 7;
+}
+
void esp_dma_enable(ESPState *s, int irq, int level)
{
if (level) {
@@ -200,24 +218,6 @@ static uint32_t esp_get_stc(ESPState *s)
return dmalen;
}
-static const char *esp_phase_names[8] = {
-"DATA OUT", "DATA IN", "COMMAND", "STATUS",
-"(reserved)", "(reserved)", "MESSAGE OUT", "MESSAGE IN"
-};
-
-static void esp_set_phase(ESPState *s, uint8_t phase)
-{
-s->rregs[ESP_RSTAT] &= ~7;
-s->rregs[ESP_RSTAT] |= phase;
-
-trace_esp_set_phase(esp_phase_names[phase]);
-}
-
-static uint8_t esp_get_phase(ESPState *s)
-{
-return s->rregs[ESP_RSTAT] & 7;
-}
-
static uint8_t esp_pdma_read(ESPState *s)
{
uint8_t val;
--
2.39.2
[PULL 15/17] esp.c: update esp_fifo_{push, pop}() to call esp_update_drq()
This ensures that the DRQ line is always set correctly when reading/writing
single bytes to/from the FIFO.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paolo Bonzini
Message-Id: <20240324191707.623175-16-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 6fd1a12f23..4895181ec1 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -170,10 +170,11 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
{
if (fifo8_num_used(>fifo) == s->fifo.capacity) {
trace_esp_error_fifo_overrun();
-return;
+} else {
+fifo8_push(>fifo, val);
}
-fifo8_push(>fifo, val);
+esp_update_drq(s);
}
static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len)
@@ -184,11 +185,16 @@ static void esp_fifo_push_buf(ESPState *s, uint8_t *buf,
int len)
static uint8_t esp_fifo_pop(ESPState *s)
{
+uint8_t val;
+
if (fifo8_is_empty(>fifo)) {
-return 0;
+val = 0;
+} else {
+val = fifo8_pop(>fifo);
}
-return fifo8_pop(>fifo);
+esp_update_drq(s);
+return val;
}
static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
--
2.39.2
[PULL 17/17] esp.c: remove explicit setting of DRQ within ESP state machine
Now the esp_update_drq() is called for all reads/writes to the FIFO, there is
no need to manually raise and lower the DRQ signal.
Signed-off-by: Mark Cave-Ayland
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/611
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1831
Reviewed-by: Paolo Bonzini
Message-Id: <20240324191707.623175-18-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 9 -
1 file changed, 9 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 04dfd90090..5d9b52632e 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -506,7 +506,6 @@ static void esp_dma_ti_check(ESPState *s)
if (esp_get_tc(s) == 0 && fifo8_num_used(>fifo) < 2) {
s->rregs[ESP_RINTR] |= INTR_BS;
esp_raise_irq(s);
-esp_lower_drq(s);
}
}
@@ -526,7 +525,6 @@ static void esp_do_dma(ESPState *s)
} else {
len = esp_fifo_pop_buf(s, buf, fifo8_num_used(>fifo));
len = MIN(fifo8_num_free(>cmdfifo), len);
-esp_raise_drq(s);
}
fifo8_push_all(>cmdfifo, buf, len);
@@ -583,7 +581,6 @@ static void esp_do_dma(ESPState *s)
len = esp_fifo_pop_buf(s, buf, fifo8_num_used(>fifo));
len = MIN(fifo8_num_free(>cmdfifo), len);
fifo8_push_all(>cmdfifo, buf, len);
-esp_raise_drq(s);
}
trace_esp_handle_ti_cmd(cmdlen);
s->ti_size = 0;
@@ -615,7 +612,6 @@ static void esp_do_dma(ESPState *s)
len = MIN(s->async_len, ESP_FIFO_SZ);
len = MIN(len, fifo8_num_used(>fifo));
len = esp_fifo_pop_buf(s, s->async_buf, len);
-esp_raise_drq(s);
}
s->async_buf += len;
@@ -667,7 +663,6 @@ static void esp_do_dma(ESPState *s)
/* Copy device data to FIFO */
len = MIN(len, fifo8_num_free(>fifo));
esp_fifo_push_buf(s, s->async_buf, len);
-esp_raise_drq(s);
}
s->async_buf += len;
@@ -733,7 +728,6 @@ static void esp_do_dma(ESPState *s)
if (fifo8_num_used(>fifo) < 2) {
s->rregs[ESP_RINTR] |= INTR_BS;
esp_raise_irq(s);
-esp_lower_drq(s);
}
break;
}
@@ -1021,9 +1015,6 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
s->rregs[ESP_RINTR] |= INTR_BS;
esp_raise_irq(s);
-/* Ensure DRQ is set correctly for TC underflow or normal completion */
-esp_dma_ti_check(s);
-
if (s->current_req) {
scsi_req_unref(s->current_req);
s->current_req = NULL;
--
2.39.2
[PULL 10/17] esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS
The current logic assumes that at least 1 byte is present in the FIFO when executing a non-DMA SELATNS command, but this may not be the case if the guest executes an invalid ESP command sequence. Reported-by: Chuhong Yuan Signed-off-by: Mark Cave-Ayland Reviewed-by: Paolo Bonzini Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20240324191707.623175-11-mark.cave-ayl...@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 1aac8f5564..f3aa5364cf 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -762,7 +762,8 @@ static void esp_do_nodma(ESPState *s) case CMD_SELATNS: /* Copy one byte from FIFO into cmdfifo */ -len = esp_fifo_pop_buf(s, buf, 1); +len = esp_fifo_pop_buf(s, buf, + MIN(fifo8_num_used(>fifo), 1)); len = MIN(fifo8_num_free(>cmdfifo), len); fifo8_push_all(>cmdfifo, buf, len); -- 2.39.2
[PULL 03/17] esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in do_message_phase()
The aim is to restrict the esp_fifo_*() functions so that they only operate on
the hardware FIFO. When reading from cmdfifo in do_message_phase() use the
underlying esp_fifo8_pop_buf() function directly.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Paolo Bonzini
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20240324191707.623175-4-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ff51145da7..9386704a58 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -325,7 +325,7 @@ static void do_message_phase(ESPState *s)
/* Ignore extended messages for now */
if (s->cmdfifo_cdb_offset) {
int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(>cmdfifo));
-esp_fifo_pop_buf(>cmdfifo, NULL, len);
+esp_fifo8_pop_buf(>cmdfifo, NULL, len);
s->cmdfifo_cdb_offset = 0;
}
}
--
2.39.2
[PULL 16/17] esp.c: ensure esp_pdma_write() always calls esp_fifo_push()
This ensures that esp_update_drq() is called via esp_fifo_push() whenever the
host uses PDMA to transfer data to a SCSI device.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paolo Bonzini
Message-Id: <20240324191707.623175-17-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 4895181ec1..04dfd90090 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -282,14 +282,12 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
{
uint32_t dmalen = esp_get_tc(s);
-if (dmalen == 0) {
-return;
-}
-
esp_fifo_push(s, val);
-dmalen--;
-esp_set_tc(s, dmalen);
+if (dmalen && s->drq_state) {
+dmalen--;
+esp_set_tc(s, dmalen);
+}
}
static int esp_select(ESPState *s)
--
2.39.2
[PULL 02/17] esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in do_command_phase()
The aim is to restrict the esp_fifo_*() functions so that they only operate on
the hardware FIFO. When reading from cmdfifo in do_command_phase() use the
underlying esp_fifo8_pop_buf() function directly.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Paolo Bonzini
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20240324191707.623175-3-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 1b7b118a0b..ff51145da7 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -280,7 +280,7 @@ static void do_command_phase(ESPState *s)
if (!cmdlen || !s->current_dev) {
return;
}
-esp_fifo_pop_buf(>cmdfifo, buf, cmdlen);
+esp_fifo8_pop_buf(>cmdfifo, buf, cmdlen);
current_lun = scsi_device_find(>bus, 0, s->current_dev->id, s->lun);
if (!current_lun) {
--
2.39.2
[PULL 14/17] esp.c: introduce esp_update_drq() and update esp_fifo_{push, pop}_buf() to use it
This new function sets the DRQ line correctly according to the current transfer
mode, direction and FIFO contents. Update esp_fifo_push_buf() and
esp_fifo_pop_buf()
to use it so that DRQ is always set correctly when reading/writing multiple
bytes
to/from the FIFO.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paolo Bonzini
Message-Id: <20240324191707.623175-15-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 48 +++-
1 file changed, 47 insertions(+), 1 deletion(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 9e35c00927..6fd1a12f23 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -124,6 +124,48 @@ void esp_request_cancelled(SCSIRequest *req)
}
}
+static void esp_update_drq(ESPState *s)
+{
+bool to_device;
+
+switch (esp_get_phase(s)) {
+case STAT_MO:
+case STAT_CD:
+case STAT_DO:
+to_device = true;
+break;
+
+case STAT_DI:
+case STAT_ST:
+case STAT_MI:
+to_device = false;
+break;
+
+default:
+return;
+}
+
+if (s->dma) {
+/* DMA request so update DRQ according to transfer direction */
+if (to_device) {
+if (fifo8_num_free(>fifo) < 2) {
+esp_lower_drq(s);
+} else {
+esp_raise_drq(s);
+}
+} else {
+if (fifo8_num_used(>fifo) < 2) {
+esp_lower_drq(s);
+} else {
+esp_raise_drq(s);
+}
+}
+} else {
+/* Not a DMA request */
+esp_lower_drq(s);
+}
+}
+
static void esp_fifo_push(ESPState *s, uint8_t val)
{
if (fifo8_num_used(>fifo) == s->fifo.capacity) {
@@ -137,6 +179,7 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len)
{
fifo8_push_all(>fifo, buf, len);
+esp_update_drq(s);
}
static uint8_t esp_fifo_pop(ESPState *s)
@@ -180,7 +223,10 @@ static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t
*dest, int maxlen)
static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen)
{
-return esp_fifo8_pop_buf(>fifo, dest, maxlen);
+uint32_t len = esp_fifo8_pop_buf(>fifo, dest, maxlen);
+
+esp_update_drq(s);
+return len;
}
static uint32_t esp_get_tc(ESPState *s)
--
2.39.2
[PULL 05/17] esp.c: change esp_fifo_push() to take ESPState
Now that all users of esp_fifo_push() operate on the main FIFO there is no need
to pass the FIFO explicitly.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paolo Bonzini
Message-Id: <20240324191707.623175-6-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 5b169b3720..7e3338815b 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -106,14 +106,14 @@ void esp_request_cancelled(SCSIRequest *req)
}
}
-static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
+static void esp_fifo_push(ESPState *s, uint8_t val)
{
-if (fifo8_num_used(fifo) == fifo->capacity) {
+if (fifo8_num_used(>fifo) == s->fifo.capacity) {
trace_esp_error_fifo_overrun();
return;
}
-fifo8_push(fifo, val);
+fifo8_push(>fifo, val);
}
static uint8_t esp_fifo_pop(Fifo8 *fifo)
@@ -229,7 +229,7 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
return;
}
-esp_fifo_push(>fifo, val);
+esp_fifo_push(s, val);
dmalen--;
esp_set_tc(s, dmalen);
@@ -1240,7 +1240,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t
val)
break;
case ESP_FIFO:
if (!fifo8_is_full(>fifo)) {
-esp_fifo_push(>fifo, val);
+esp_fifo_push(s, val);
}
esp_do_nodma(s);
break;
--
2.39.2
[PULL 11/17] esp.c: rework esp_cdb_length() into esp_cdb_ready()
The esp_cdb_length() function is only used as part of a calculation to determine
whether the cmdfifo contains an entire SCSI CDB. Rework esp_cdb_length() into a
new esp_cdb_ready() function which both enables us to handle the case where
scsi_cdb_length() returns -1, plus simplify the logic for its callers.
Suggested-by: Paolo Bonzini
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Paolo Bonzini
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20240324191707.623175-12-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 30 ++
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f3aa5364cf..f47abc36d6 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -425,20 +425,20 @@ static void write_response(ESPState *s)
}
}
-static int esp_cdb_length(ESPState *s)
+static bool esp_cdb_ready(ESPState *s)
{
+int len = fifo8_num_used(>cmdfifo) - s->cmdfifo_cdb_offset;
const uint8_t *pbuf;
-int cmdlen, len;
+int cdblen;
-cmdlen = fifo8_num_used(>cmdfifo);
-if (cmdlen < s->cmdfifo_cdb_offset) {
-return 0;
+if (len <= 0) {
+return false;
}
-pbuf = fifo8_peek_buf(>cmdfifo, cmdlen, NULL);
-len = scsi_cdb_length((uint8_t *)[s->cmdfifo_cdb_offset]);
+pbuf = fifo8_peek_buf(>cmdfifo, len, NULL);
+cdblen = scsi_cdb_length((uint8_t *)[s->cmdfifo_cdb_offset]);
-return len;
+return cdblen < 0 ? false : (len >= cdblen);
}
static void esp_dma_ti_check(ESPState *s)
@@ -806,10 +806,9 @@ static void esp_do_nodma(ESPState *s)
trace_esp_handle_ti_cmd(cmdlen);
/* CDB may be transferred in one or more TI commands */
-if (esp_cdb_length(s) && esp_cdb_length(s) ==
-fifo8_num_used(>cmdfifo) - s->cmdfifo_cdb_offset) {
-/* Command has been received */
-do_cmd(s);
+if (esp_cdb_ready(s)) {
+/* Command has been received */
+do_cmd(s);
} else {
/*
* If data was transferred from the FIFO then raise bus
@@ -832,10 +831,9 @@ static void esp_do_nodma(ESPState *s)
fifo8_push_all(>cmdfifo, buf, len);
/* Handle when DMA transfer is terminated by non-DMA FIFO write */
-if (esp_cdb_length(s) && esp_cdb_length(s) ==
-fifo8_num_used(>cmdfifo) - s->cmdfifo_cdb_offset) {
-/* Command has been received */
-do_cmd(s);
+if (esp_cdb_ready(s)) {
+/* Command has been received */
+do_cmd(s);
}
break;
--
2.39.2
[PULL 09/17] esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO
Instead of pushing data into the FIFO directly with fifo8_push_all(), add a new
esp_fifo_push_buf() function and use it accordingly.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paolo Bonzini
Message-Id: <20240324191707.623175-10-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 83b621ee0f..1aac8f5564 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -116,6 +116,11 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
fifo8_push(>fifo, val);
}
+static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len)
+{
+fifo8_push_all(>fifo, buf, len);
+}
+
static uint8_t esp_fifo_pop(ESPState *s)
{
if (fifo8_is_empty(>fifo)) {
@@ -601,7 +606,7 @@ static void esp_do_dma(ESPState *s)
} else {
/* Copy device data to FIFO */
len = MIN(len, fifo8_num_free(>fifo));
-fifo8_push_all(>fifo, s->async_buf, len);
+esp_fifo_push_buf(s, s->async_buf, len);
esp_raise_drq(s);
}
@@ -650,7 +655,7 @@ static void esp_do_dma(ESPState *s)
if (s->dma_memory_write) {
s->dma_memory_write(s->dma_opaque, buf, len);
} else {
-fifo8_push_all(>fifo, buf, len);
+esp_fifo_push_buf(s, buf, len);
}
esp_set_tc(s, esp_get_tc(s) - len);
@@ -685,7 +690,7 @@ static void esp_do_dma(ESPState *s)
if (s->dma_memory_write) {
s->dma_memory_write(s->dma_opaque, buf, len);
} else {
-fifo8_push_all(>fifo, buf, len);
+esp_fifo_push_buf(s, buf, len);
}
esp_set_tc(s, esp_get_tc(s) - len);
--
2.39.2
[PULL 01/17] esp.c: move esp_fifo_pop_buf() internals to new esp_fifo8_pop_buf() function
Update esp_fifo_pop_buf() to be a simple wrapper onto the new
esp_fifo8_pop_buf()
function.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Paolo Bonzini
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20240324191707.623175-2-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 590ff99744..1b7b118a0b 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -125,7 +125,7 @@ static uint8_t esp_fifo_pop(Fifo8 *fifo)
return fifo8_pop(fifo);
}
-static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
+static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
{
const uint8_t *buf;
uint32_t n, n2;
@@ -155,6 +155,11 @@ static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t
*dest, int maxlen)
return n;
}
+static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
+{
+return esp_fifo8_pop_buf(fifo, dest, maxlen);
+}
+
static uint32_t esp_get_tc(ESPState *s)
{
uint32_t dmalen;
--
2.39.2
[PULL 12/17] esp.c: prevent cmdfifo overflow in esp_cdb_ready()
During normal use the cmdfifo will never wrap internally and cmdfifo_cdb_offset
will always indicate the start of the SCSI CDB. However it is possible that a
malicious guest could issue an invalid ESP command sequence such that cmdfifo
wraps internally and cmdfifo_cdb_offset could point beyond the end of the FIFO
data buffer.
Add an extra check to fifo8_peek_buf() to ensure that if the cmdfifo has wrapped
internally then esp_cdb_ready() will exit rather than allow scsi_cdb_length() to
access data outside the cmdfifo data buffer.
Reported-by: Chuhong Yuan
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Paolo Bonzini
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20240324191707.623175-13-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f47abc36d6..d8db33b921 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -429,13 +429,23 @@ static bool esp_cdb_ready(ESPState *s)
{
int len = fifo8_num_used(>cmdfifo) - s->cmdfifo_cdb_offset;
const uint8_t *pbuf;
+uint32_t n;
int cdblen;
if (len <= 0) {
return false;
}
-pbuf = fifo8_peek_buf(>cmdfifo, len, NULL);
+pbuf = fifo8_peek_buf(>cmdfifo, len, );
+if (n < len) {
+/*
+ * In normal use the cmdfifo should never wrap, but include this check
+ * to prevent a malicious guest from reading past the end of the
+ * cmdfifo data buffer below
+ */
+return false;
+}
+
cdblen = scsi_cdb_length((uint8_t *)[s->cmdfifo_cdb_offset]);
return cdblen < 0 ? false : (len >= cdblen);
--
2.39.2
[PULL 00/17] qemu-sparc queue 20240404
The following changes since commit 786fd793b81410fb2a28914315e2f05d2ff6733b:
Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging
(2024-04-03 12:52:03 +0100)
are available in the Git repository at:
https://github.com/mcayland/qemu.git tags/qemu-sparc-20240404
for you to fetch changes up to d7fe931818d5e9aa70d08056c43b496ce789ba64:
esp.c: remove explicit setting of DRQ within ESP state machine (2024-04-04
15:17:53 +0100)
qemu-sparc queue
- This contains fixes for the ESP emulation discovered by fuzzing (with thanks
to
Chuhong Yuan )
Mark Cave-Ayland (17):
esp.c: move esp_fifo_pop_buf() internals to new esp_fifo8_pop_buf()
function
esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
do_command_phase()
esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
do_message_phase()
esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()
esp.c: change esp_fifo_push() to take ESPState
esp.c: change esp_fifo_pop() to take ESPState
esp.c: use esp_fifo_push() instead of fifo8_push()
esp.c: change esp_fifo_pop_buf() to take ESPState
esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO
esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS
esp.c: rework esp_cdb_length() into esp_cdb_ready()
esp.c: prevent cmdfifo overflow in esp_cdb_ready()
esp.c: move esp_set_phase() and esp_get_phase() towards the beginning of
the file
esp.c: introduce esp_update_drq() and update esp_fifo_{push, pop}_buf()
to use it
esp.c: update esp_fifo_{push, pop}() to call esp_update_drq()
esp.c: ensure esp_pdma_write() always calls esp_fifo_push()
esp.c: remove explicit setting of DRQ within ESP state machine
hw/scsi/esp.c | 223 +-
1 file changed, 142 insertions(+), 81 deletions(-)
[PULL 06/17] esp.c: change esp_fifo_pop() to take ESPState
Now that all users of esp_fifo_pop() operate on the main FIFO there is no need
to pass the FIFO explicitly.
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paolo Bonzini
Message-Id: <20240324191707.623175-7-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 7e3338815b..d474268438 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -116,13 +116,13 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
fifo8_push(>fifo, val);
}
-static uint8_t esp_fifo_pop(Fifo8 *fifo)
+static uint8_t esp_fifo_pop(ESPState *s)
{
-if (fifo8_is_empty(fifo)) {
+if (fifo8_is_empty(>fifo)) {
return 0;
}
-return fifo8_pop(fifo);
+return fifo8_pop(>fifo);
}
static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
@@ -217,7 +217,7 @@ static uint8_t esp_pdma_read(ESPState *s)
{
uint8_t val;
-val = esp_fifo_pop(>fifo);
+val = esp_fifo_pop(s);
return val;
}
@@ -1184,7 +1184,7 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
switch (saddr) {
case ESP_FIFO:
-s->rregs[ESP_FIFO] = esp_fifo_pop(>fifo);
+s->rregs[ESP_FIFO] = esp_fifo_pop(s);
val = s->rregs[ESP_FIFO];
break;
case ESP_RINTR:
--
2.39.2
[PULL 04/17] esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()
Signed-off-by: Mark Cave-Ayland
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paolo Bonzini
Message-Id: <20240324191707.623175-5-mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 9386704a58..5b169b3720 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -315,7 +315,8 @@ static void do_command_phase(ESPState *s)
static void do_message_phase(ESPState *s)
{
if (s->cmdfifo_cdb_offset) {
-uint8_t message = esp_fifo_pop(>cmdfifo);
+uint8_t message = fifo8_is_empty(>cmdfifo) ? 0 :
+ fifo8_pop(>cmdfifo);
trace_esp_do_identify(message);
s->lun = message & 7;
--
2.39.2
Re: [PATCH v3 00/17] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine
On 04/04/2024 11:28, Philippe Mathieu-Daudé wrote:
Hi Mark,
On 24/3/24 20:16, Mark Cave-Ayland wrote:
Mark Cave-Ayland (17):
esp.c: move esp_fifo_pop_buf() internals to new esp_fifo8_pop_buf()
function
esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
do_command_phase()
esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
do_message_phase()
esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()
esp.c: change esp_fifo_push() to take ESPState
esp.c: change esp_fifo_pop() to take ESPState
esp.c: use esp_fifo_push() instead of fifo8_push()
esp.c: change esp_fifo_pop_buf() to take ESPState
esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO
esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS
esp.c: rework esp_cdb_length() into esp_cdb_ready()
esp.c: prevent cmdfifo overflow in esp_cdb_ready()
esp.c: move esp_set_phase() and esp_get_phase() towards the beginning
of the file
esp.c: introduce esp_update_drq() and update esp_fifo_{push,pop}_buf()
to use it
esp.c: update esp_fifo_{push,pop}() to call esp_update_drq()
esp.c: ensure esp_pdma_write() always calls esp_fifo_push()
esp.c: remove explicit setting of DRQ within ESP state machine
I have to prepare another PR for rc3, do you want me to take
this series?
Hi Phil,
Thanks for the offer, but amazingly I have a little bit of time this afternoon(!), so
I'll try and send a PR later today.
ATB,
Mark.
Re: [PATCH-for-9.0 v2] hw/i386/pc: Deprecate 64-bit CPUs on ISA-only PC machine
On 27/03/2024 16:54, Philippe Mathieu-Daudé wrote:
Per Daniel suggestion [*]:
> isapc could arguably be restricted to just 32-bit CPU models,
> because we should not need it to support any feature that didn't
> exist prior to circa 1995. eg refuse to start with isapc, if 'lm'
> is present in the CPU model for example.
Display a warning when such CPU is used:
$ qemu-system-x86_64 -monitor stdio -S -M isapc -cpu Westmere
qemu-system-x86_64: warning: Use of 64-bit CPU 'Westmere' is deprecated on
the ISA-only PC machine
QEMU 8.2.91 monitor - type 'help' for more information
(qemu) q
$ qemu-system-x86_64 -monitor stdio -S -M isapc -cpu athlon
QEMU 8.2.91 monitor - type 'help' for more information
(qemu) q
[*] https://lore.kernel.org/qemu-devel/zgqks4rpmst5x...@redhat.com/
Suggested-by: Daniel P. Berrangé
Signed-off-by: Philippe Mathieu-Daudé
---
docs/about/deprecated.rst | 7 +++
include/hw/i386/pc.h | 1 +
hw/i386/pc_piix.c | 14 ++
3 files changed, 22 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 7b548519b5..345c35507f 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -208,6 +208,13 @@ is no longer packaged in any distro making it harder to
run the
``check-tcg`` tests. Unless we can improve the testing situation there
is a chance the code will bitrot without anyone noticing.
+64-bit (x86_64) CPUs on the ``isapc`` machine (since 9.0)
+'
+
+The ``isapc`` machine aims to emulate old PC machine without PCI was
+generalized, so hardware available around 1995, before 64-bit intel
+CPUs were produced.
+
System emulator machines
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 27a68071d7..2d202b9549 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -96,6 +96,7 @@ struct PCMachineClass {
const char *default_south_bridge;
/* Compat options: */
+bool deprecate_64bit_cpu; /* Specific to the 'isapc' machine */
/* Default CPU model version. See x86_cpu_set_default_version(). */
int default_cpu_version;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 18ba076609..2e5b2efc33 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -182,7 +182,20 @@ static void pc_init1(MachineState *machine, const char
*pci_type)
}
pc_machine_init_sgx_epc(pcms);
+
x86_cpus_init(x86ms, pcmc->default_cpu_version);
+if (pcmc->deprecate_64bit_cpu) {
+X86CPU *cpu = X86_CPU(first_cpu);
+
+if (cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
+const char *cpu_type = object_get_typename(OBJECT(first_cpu));
+int cpu_len = strlen(cpu_type) - strlen(X86_CPU_TYPE_SUFFIX);
+
+warn_report("Use of 64-bit CPU '%.*s' is deprecated"
+" on the ISA-only PC machine",
+cpu_len, cpu_type);
+}
+}
if (kvm_enabled()) {
kvmclock_create(pcmc->kvmclock_create_always);
@@ -918,6 +931,7 @@ static void isapc_machine_options(MachineClass *m)
pcmc->gigabyte_align = false;
pcmc->smbios_legacy_mode = true;
pcmc->has_reserved_memory = false;
+pcmc->deprecate_64bit_cpu = true;
m->default_nic = "ne2k_isa";
m->default_cpu_type = X86_CPU_TYPE_NAME("486");
m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
The logic around checking CPUID_EXT2_LM looks good to me. Slightly curious as to
whether people feel updating PCMachineClass is necessary, or you can simply do
qdev_get_machine() and use object_dynamic_cast() to see if the machine matches
MACHINE_NAME("isapc") and warn that way?
FWIW I'd be amazed if anyone were actually overriding the default and trying to do
this, but I guess that's what the warn_report() is for anyhow:
Reviewed-by: Mark Cave-Ayland
ATB,
Mark.
Re: [PATCH for-9.0] docs/about: Mark the iaspc machine type as deprecated
On 27/03/2024 07:09, Gerd Hoffmann wrote: On Tue, Mar 26, 2024 at 01:30:48PM +, Mark Cave-Ayland wrote: Heh I've actually been using isapc over the past couple of weeks to fire up some old programs in a Windows 3 VM :) I'm wondering why these use cases can't simply use the 'pc' machine type? The early pci chipsets of the 90-ies have been designed in a backward-compatible manner, with devices such as the IDE controller being mapped to the standard ISA ioports. So even an historic OS which does not know what PCI is can run on that hardware, by simply talking to devices using the standard ISA io ports ... Hmmm that's a fair point: I think the pc machine has a PCI-ISA bridge included, so ISA devices can be plugged in as needed. The reason I ended up on that configuration was because I ended up chasing down a regression, and wanted to quickly eliminate things such as ACPI. ATB, Mark.
Re: [PATCH for-9.0] docs/about: Mark the iaspc machine type as deprecated
On 26/03/2024 12:51, Igor Mammedov wrote:
ISAPC machine was introduced 25 years ago and it's a lot of time since
such machine was around with real ISA only PC hardware practically defunct.
Also it's slowly bit-rots (for example: I was able to boot RHEL6 on RHEL9 host
in only TCG mode, while in KVM mode it hung in the middle of boot)
Rather than spending time on fixing 'the oldest' no longer tested machine type,
deprecate it so we can clean up QEMU code from legacy fixups and hopefully
make it easier to follow.
Folks who have to use ancient guest that requires ISAPC can still
use older QEMU to play with it.
Heh I've actually been using isapc over the past couple of weeks to fire up some old
programs in a Windows 3 VM :)
I'd really hate to see isapc disappear as there are a number of people from the retro
crowd (such as myself) who fire up QEMU/KVM on various historical images, and whilst
there are alternatives, there isn't really anything that touches QEMU performance-wise.
This leads into the question as to how QEMU should handle less recent machines: I
appreciate that from an enterprise perspective there is little interest, but there
are plenty of hobbyists and historians who are.
From my personal experience with SPARC/macppc machines I accept that they are not
first-class citizens, and so my approach here is that I don't mind if patches break
migration or command-line compatibility as long as I can still fire up the VM.
Regressions do occur, but fortunately they don't tend to occur that often.
I can see how there is a lot of legacy cruft around handling legacy command line
options that could be improved by removing isapc, and I think that a lot of this is
around preserving historical behaviour.
How about splitting the isapc machine out of the generic PC init so that it can be
used going forward with less command-line/migration compatibility guarantees, but
also won't prevent subsequent tidy-ups/changes to the main PC machines going forward?
Signed-off-by: Igor Mammedov
---
docs/about/deprecated.rst | 7 +++
hw/i386/pc_piix.c | 1 +
2 files changed, 8 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 7b548519b5..5708296991 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -226,6 +226,13 @@ These old machine types are quite neglected nowadays and
thus might have
various pitfalls with regards to live migration. Use a newer machine type
instead.
+``isapc`` (since 9.0)
+'
+
+These old machine type are quite neglected nowadays and thus might have
+various pitfalls with regards to live migration. Use a newer machine type
+instead.
+
Nios II ``10m50-ghrd`` and ``nios2-generic-nommu`` machines (since 8.2)
'''
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 18ba076609..96f72384dd 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -921,6 +921,7 @@ static void isapc_machine_options(MachineClass *m)
m->default_nic = "ne2k_isa";
m->default_cpu_type = X86_CPU_TYPE_NAME("486");
m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
+m->deprecation_reason = "old and unattended - use a newer version instead";
}
DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
ATB,
Mark.
Re: [PATCH v3 10/17] esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS
On 25/03/2024 10:49, Philippe Mathieu-Daudé wrote:
On 24/3/24 20:16, Mark Cave-Ayland wrote:
The current logic assumes that at least 1 byte is present in the FIFO when
executing a non-DMA SELATNS command, but this may not be the case if the
guest executes an invalid ESP command sequence.
What is real hardware behavior here?
I don't know for sure, but my guess is that if you ask to transfer a single byte from
the FIFO to the SCSI bus and the FIFO is empty, you'll either end up with all zeros
or a NOOP.
Reported-by: Chuhong Yuan
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 1aac8f5564..f3aa5364cf 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -762,7 +762,8 @@ static void esp_do_nodma(ESPState *s)
case CMD_SELATNS:
Alternatively logging the guest abuse:
len = fifo8_num_used(>fifo);
if (len < 1) {
qemu_log_mask(LOG_GUEST_ERROR, ...
break;
}
/* Copy one byte from FIFO into cmdfifo */
- len = esp_fifo_pop_buf(s, buf, 1);
+ len = esp_fifo_pop_buf(s, buf,
+ MIN(fifo8_num_used(>fifo), 1));
This is similar to your previous comment in that it's an artifact of the
implementation: when popping data using esp_fifo_pop_buf() I've always allowed the
internal Fifo8 assert() if too much data is requested. This was a deliberate design
choice that allowed me to catch several memory issues when working on the ESP
emulation: it just so happened I missed a case in the last big ESP rework that was
found by fuzzing.
It's also worth noting that it's a Fifo8 internal protective assert() that fires here
which is different from the previous case whereby an overflow of the internal Fifo8
data buffer actually did occur.
len = MIN(fifo8_num_free(>cmdfifo), len);
fifo8_push_all(>cmdfifo, buf, len);
ATB,
Mark.
Re: [PATCH v3 12/17] esp.c: prevent cmdfifo overflow in esp_cdb_ready()
On 25/03/2024 10:26, Philippe Mathieu-Daudé wrote:
On 24/3/24 20:17, Mark Cave-Ayland wrote:
During normal use the cmdfifo will never wrap internally and cmdfifo_cdb_offset
will always indicate the start of the SCSI CDB. However it is possible that a
malicious guest could issue an invalid ESP command sequence such that cmdfifo
wraps internally and cmdfifo_cdb_offset could point beyond the end of the FIFO
data buffer.
Add an extra check to fifo8_peek_buf() to ensure that if the cmdfifo has wrapped
internally then esp_cdb_ready() will exit rather than allow scsi_cdb_length() to
access data outside the cmdfifo data buffer.
Reported-by: Chuhong Yuan
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f47abc36d6..d8db33b921 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -429,13 +429,23 @@ static bool esp_cdb_ready(ESPState *s)
{
int len = fifo8_num_used(>cmdfifo) - s->cmdfifo_cdb_offset;
const uint8_t *pbuf;
+ uint32_t n;
int cdblen;
if (len <= 0) {
return false;
}
- pbuf = fifo8_peek_buf(>cmdfifo, len, NULL);
+ pbuf = fifo8_peek_buf(>cmdfifo, len, );
+ if (n < len) {
+ /*
+ * In normal use the cmdfifo should never wrap, but include this check
+ * to prevent a malicious guest from reading past the end of the
+ * cmdfifo data buffer below
+ */
Can we qemu_log_mask(LOG_GUEST_ERROR) something here?
I'm not sure that this makes sense here? The cmdfifo wrapping is internal artifact of
the Fifo8 implementation rather than being directly affected by writes to the ESP
hardware FIFO (i.e. this is not the same as the ESP hardware FIFO overflow).
+ return false;
+ }
+
cdblen = scsi_cdb_length((uint8_t *)[s->cmdfifo_cdb_offset]);
return cdblen < 0 ? false : (len >= cdblen);
ATB,
Mark.
