The SCSI CSRs are located within the SCSI subsystem of the NeXT PC (Peripheral
Contoller) which is now modelled as a separate QEMU device.
Add a new VMStateDescription for the next-scsi device to enable the SCSI CSRs
to be migrated.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/m68k/next-cube.c | 88 +++++++++++++++++++++++++++++++++++++++------
1 file changed, 78 insertions(+), 10 deletions(-)
diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 266f57ac63..32466a425f 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -93,6 +93,10 @@ struct NeXTSCSI {
MemoryRegion scsi_mem;
SysBusESPState sysbus_esp;
+
+ MemoryRegion scsi_csr_mem;
+ uint8_t scsi_csr_1;
+ uint8_t scsi_csr_2;
};
#define TYPE_NEXT_PC "next-pc"
@@ -115,8 +119,6 @@ struct NeXTPC {
uint32_t led;
NeXTSCSI next_scsi;
- uint8_t scsi_csr_1;
- uint8_t scsi_csr_2;
qemu_irq scsi_reset;
qemu_irq scsi_dma;
@@ -364,6 +366,7 @@ static const MemoryRegionOps next_mmio_ops = {
static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
{
NeXTPC *s = NEXT_PC(opaque);
+ NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
uint64_t val;
switch (addr) {
@@ -373,12 +376,12 @@ static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
break;
case 0x14020:
- DPRINTF("SCSI 4020 STATUS READ %X\n", s->scsi_csr_1);
- val = s->scsi_csr_1;
+ DPRINTF("SCSI 4020 STATUS READ %X\n", ns->scsi_csr_1);
+ val = ns->scsi_csr_1;
break;
case 0x14021:
- DPRINTF("SCSI 4021 STATUS READ %X\n", s->scsi_csr_2);
+ DPRINTF("SCSI 4021 STATUS READ %X\n", ns->scsi_csr_2);
val = 0x40;
break;
@@ -411,6 +414,7 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
unsigned size)
{
NeXTPC *s = NEXT_PC(opaque);
+ NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
switch (addr) {
case 0x14108:
@@ -445,7 +449,7 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
DPRINTF("SCSICSR Reset\n");
/* I think this should set DMADIR. CPUDMA and INTMASK to 0 */
qemu_irq_raise(s->scsi_reset);
- s->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
+ ns->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
qemu_irq_lower(s->scsi_reset);
}
if (val & SCSICSR_DMADIR) {
@@ -838,6 +842,54 @@ static void nextscsi_write(void *opaque, uint8_t *buf, int size)
nextdma_write(opaque, buf, size, NEXTDMA_SCSI);
}
+static void next_scsi_csr_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned size)
+{
+ NeXTSCSI *s = NEXT_SCSI(opaque);
+
+ switch (addr) {
+ case 0:
+ s->scsi_csr_1 = val;
+ break;
+
+ case 1:
+ s->scsi_csr_2 = val;
+ break;
+
+ default:
+ g_assert_not_reached();
+ }
+}
+
+static uint64_t next_scsi_csr_read(void *opaque, hwaddr addr, unsigned size)
+{
+ NeXTSCSI *s = NEXT_SCSI(opaque);
+ uint64_t val;
+
+ switch (addr) {
+ case 0:
+ val = s->scsi_csr_1;
+ break;
+
+ case 1:
+ val = s->scsi_csr_2;
+ break;
+
+ default:
+ g_assert_not_reached();
+ }
+
+ return val;
+}
+
+static const MemoryRegionOps next_scsi_csr_ops = {
+ .read = next_scsi_csr_read,
+ .write = next_scsi_csr_write,
+ .valid.min_access_size = 1,
+ .valid.max_access_size = 1,
+ .endianness = DEVICE_BIG_ENDIAN,
+};
+
static void next_scsi_init(Object *obj)
{
NeXTSCSI *s = NEXT_SCSI(obj);
@@ -845,6 +897,9 @@ static void next_scsi_init(Object *obj)
object_initialize_child(obj, "esp", &s->sysbus_esp, TYPE_SYSBUS_ESP);
+ memory_region_init_io(&s->scsi_csr_mem, obj, &next_scsi_csr_ops,
+ s, "csrs", 2);
+
memory_region_init(&s->scsi_mem, obj, "next.scsi", 0x40);
sysbus_init_mmio(sbd, &s->scsi_mem);
}
@@ -874,15 +929,30 @@ static void next_scsi_realize(DeviceState *dev, Error **errp)
memory_region_add_subregion(&s->scsi_mem, 0x0,
sysbus_mmio_get_region(sbd, 0));
+ /* SCSI CSRs */
+ memory_region_add_subregion(&s->scsi_mem, 0x20, &s->scsi_csr_mem);
+
scsi_bus_legacy_handle_cmdline(&s->sysbus_esp.esp.bus);
}
+static const VMStateDescription next_scsi_vmstate = {
+ .name = "next-scsi",
+ .version_id = 0,
+ .minimum_version_id = 0,
+ .fields = (const VMStateField[]) {
+ VMSTATE_UINT8(scsi_csr_1, NeXTSCSI),
+ VMSTATE_UINT8(scsi_csr_2, NeXTSCSI),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
static void next_scsi_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
dc->desc = "NeXT SCSI Controller";
dc->realize = next_scsi_realize;
+ dc->vmsd = &next_scsi_vmstate;
}
static const TypeInfo next_scsi_info = {
@@ -1000,8 +1070,8 @@ static const VMStateDescription next_rtc_vmstate = {
static const VMStateDescription next_pc_vmstate = {
.name = "next-pc",
- .version_id = 2,
- .minimum_version_id = 2,
+ .version_id = 3,
+ .minimum_version_id = 3,
.fields = (const VMStateField[]) {
VMSTATE_UINT32(scr1, NeXTPC),
VMSTATE_UINT32(scr2, NeXTPC),
@@ -1009,8 +1079,6 @@ static const VMStateDescription next_pc_vmstate = {
VMSTATE_UINT32(int_mask, NeXTPC),
VMSTATE_UINT32(int_status, NeXTPC),
VMSTATE_UINT32(led, NeXTPC),
- VMSTATE_UINT8(scsi_csr_1, NeXTPC),
- VMSTATE_UINT8(scsi_csr_2, NeXTPC),
VMSTATE_STRUCT(rtc, NeXTPC, 0, next_rtc_vmstate, NextRtc),
VMSTATE_END_OF_LIST()
},
--
2.39.5Am Wed, 23 Oct 2024 09:58:25 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
> The SCSI CSRs are located within the SCSI subsystem of the NeXT PC (Peripheral
> Contoller) which is now modelled as a separate QEMU device.
>
> Add a new VMStateDescription for the next-scsi device to enable the SCSI CSRs
> to be migrated.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/m68k/next-cube.c | 88 +++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 78 insertions(+), 10 deletions(-)
>
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index 266f57ac63..32466a425f 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -93,6 +93,10 @@ struct NeXTSCSI {
> MemoryRegion scsi_mem;
>
> SysBusESPState sysbus_esp;
> +
> + MemoryRegion scsi_csr_mem;
> + uint8_t scsi_csr_1;
> + uint8_t scsi_csr_2;
> };
>
> #define TYPE_NEXT_PC "next-pc"
> @@ -115,8 +119,6 @@ struct NeXTPC {
> uint32_t led;
>
> NeXTSCSI next_scsi;
> - uint8_t scsi_csr_1;
> - uint8_t scsi_csr_2;
>
> qemu_irq scsi_reset;
> qemu_irq scsi_dma;
> @@ -364,6 +366,7 @@ static const MemoryRegionOps next_mmio_ops = {
> static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
> {
> NeXTPC *s = NEXT_PC(opaque);
> + NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
> uint64_t val;
>
> switch (addr) {
> @@ -373,12 +376,12 @@ static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
> break;
>
> case 0x14020:
> - DPRINTF("SCSI 4020 STATUS READ %X\n", s->scsi_csr_1);
> - val = s->scsi_csr_1;
> + DPRINTF("SCSI 4020 STATUS READ %X\n", ns->scsi_csr_1);
> + val = ns->scsi_csr_1;
> break;
>
> case 0x14021:
> - DPRINTF("SCSI 4021 STATUS READ %X\n", s->scsi_csr_2);
> + DPRINTF("SCSI 4021 STATUS READ %X\n", ns->scsi_csr_2);
> val = 0x40;
> break;
>
> @@ -411,6 +414,7 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
> unsigned size)
> {
> NeXTPC *s = NEXT_PC(opaque);
> + NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
>
> switch (addr) {
> case 0x14108:
> @@ -445,7 +449,7 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
> DPRINTF("SCSICSR Reset\n");
> /* I think this should set DMADIR. CPUDMA and INTMASK to 0 */
> qemu_irq_raise(s->scsi_reset);
> - s->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
> + ns->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
> qemu_irq_lower(s->scsi_reset);
> }
> if (val & SCSICSR_DMADIR) {
> @@ -838,6 +842,54 @@ static void nextscsi_write(void *opaque, uint8_t *buf, int size)
> nextdma_write(opaque, buf, size, NEXTDMA_SCSI);
> }
>
> +static void next_scsi_csr_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned size)
> +{
> + NeXTSCSI *s = NEXT_SCSI(opaque);
> +
> + switch (addr) {
> + case 0:
> + s->scsi_csr_1 = val;
> + break;
> +
> + case 1:
> + s->scsi_csr_2 = val;
> + break;
The old code never set the scsi_csr_x directly like this, so I'm not sure
whether this is right?
Also, maybe best squash this patch together with the next patch, otherwise
this is temporary change in behaviour, isn't it?
Thomas
> + default:
> + g_assert_not_reached();
> + }
> +}
> +
> +static uint64_t next_scsi_csr_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + NeXTSCSI *s = NEXT_SCSI(opaque);
> + uint64_t val;
> +
> + switch (addr) {
> + case 0:
> + val = s->scsi_csr_1;
> + break;
> +
> + case 1:
> + val = s->scsi_csr_2;
> + break;
> +
> + default:
> + g_assert_not_reached();
> + }
> +
> + return val;
> +}
> +
> +static const MemoryRegionOps next_scsi_csr_ops = {
> + .read = next_scsi_csr_read,
> + .write = next_scsi_csr_write,
> + .valid.min_access_size = 1,
> + .valid.max_access_size = 1,
> + .endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> static void next_scsi_init(Object *obj)
> {
> NeXTSCSI *s = NEXT_SCSI(obj);
> @@ -845,6 +897,9 @@ static void next_scsi_init(Object *obj)
>
> object_initialize_child(obj, "esp", &s->sysbus_esp, TYPE_SYSBUS_ESP);
>
> + memory_region_init_io(&s->scsi_csr_mem, obj, &next_scsi_csr_ops,
> + s, "csrs", 2);
> +
> memory_region_init(&s->scsi_mem, obj, "next.scsi", 0x40);
> sysbus_init_mmio(sbd, &s->scsi_mem);
> }
> @@ -874,15 +929,30 @@ static void next_scsi_realize(DeviceState *dev, Error **errp)
> memory_region_add_subregion(&s->scsi_mem, 0x0,
> sysbus_mmio_get_region(sbd, 0));
>
> + /* SCSI CSRs */
> + memory_region_add_subregion(&s->scsi_mem, 0x20, &s->scsi_csr_mem);
> +
> scsi_bus_legacy_handle_cmdline(&s->sysbus_esp.esp.bus);
> }
>
> +static const VMStateDescription next_scsi_vmstate = {
> + .name = "next-scsi",
> + .version_id = 0,
> + .minimum_version_id = 0,
> + .fields = (const VMStateField[]) {
> + VMSTATE_UINT8(scsi_csr_1, NeXTSCSI),
> + VMSTATE_UINT8(scsi_csr_2, NeXTSCSI),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> static void next_scsi_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> dc->desc = "NeXT SCSI Controller";
> dc->realize = next_scsi_realize;
> + dc->vmsd = &next_scsi_vmstate;
> }
>
> static const TypeInfo next_scsi_info = {
> @@ -1000,8 +1070,8 @@ static const VMStateDescription next_rtc_vmstate = {
>
> static const VMStateDescription next_pc_vmstate = {
> .name = "next-pc",
> - .version_id = 2,
> - .minimum_version_id = 2,
> + .version_id = 3,
> + .minimum_version_id = 3,
> .fields = (const VMStateField[]) {
> VMSTATE_UINT32(scr1, NeXTPC),
> VMSTATE_UINT32(scr2, NeXTPC),
> @@ -1009,8 +1079,6 @@ static const VMStateDescription next_pc_vmstate = {
> VMSTATE_UINT32(int_mask, NeXTPC),
> VMSTATE_UINT32(int_status, NeXTPC),
> VMSTATE_UINT32(led, NeXTPC),
> - VMSTATE_UINT8(scsi_csr_1, NeXTPC),
> - VMSTATE_UINT8(scsi_csr_2, NeXTPC),
> VMSTATE_STRUCT(rtc, NeXTPC, 0, next_rtc_vmstate, NextRtc),
> VMSTATE_END_OF_LIST()
> },
On 28/10/2024 16:21, Thomas Huth wrote:
> Am Wed, 23 Oct 2024 09:58:25 +0100
> schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>
>> The SCSI CSRs are located within the SCSI subsystem of the NeXT PC (Peripheral
>> Contoller) which is now modelled as a separate QEMU device.
>>
>> Add a new VMStateDescription for the next-scsi device to enable the SCSI CSRs
>> to be migrated.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/m68k/next-cube.c | 88 +++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 78 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index 266f57ac63..32466a425f 100644
>> --- a/hw/m68k/next-cube.c
>> +++ b/hw/m68k/next-cube.c
>> @@ -93,6 +93,10 @@ struct NeXTSCSI {
>> MemoryRegion scsi_mem;
>>
>> SysBusESPState sysbus_esp;
>> +
>> + MemoryRegion scsi_csr_mem;
>> + uint8_t scsi_csr_1;
>> + uint8_t scsi_csr_2;
>> };
>>
>> #define TYPE_NEXT_PC "next-pc"
>> @@ -115,8 +119,6 @@ struct NeXTPC {
>> uint32_t led;
>>
>> NeXTSCSI next_scsi;
>> - uint8_t scsi_csr_1;
>> - uint8_t scsi_csr_2;
>>
>> qemu_irq scsi_reset;
>> qemu_irq scsi_dma;
>> @@ -364,6 +366,7 @@ static const MemoryRegionOps next_mmio_ops = {
>> static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
>> {
>> NeXTPC *s = NEXT_PC(opaque);
>> + NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
>> uint64_t val;
>>
>> switch (addr) {
>> @@ -373,12 +376,12 @@ static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
>> break;
>>
>> case 0x14020:
>> - DPRINTF("SCSI 4020 STATUS READ %X\n", s->scsi_csr_1);
>> - val = s->scsi_csr_1;
>> + DPRINTF("SCSI 4020 STATUS READ %X\n", ns->scsi_csr_1);
>> + val = ns->scsi_csr_1;
>> break;
>>
>> case 0x14021:
>> - DPRINTF("SCSI 4021 STATUS READ %X\n", s->scsi_csr_2);
>> + DPRINTF("SCSI 4021 STATUS READ %X\n", ns->scsi_csr_2);
>> val = 0x40;
>> break;
>>
>> @@ -411,6 +414,7 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
>> unsigned size)
>> {
>> NeXTPC *s = NEXT_PC(opaque);
>> + NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
>>
>> switch (addr) {
>> case 0x14108:
>> @@ -445,7 +449,7 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
>> DPRINTF("SCSICSR Reset\n");
>> /* I think this should set DMADIR. CPUDMA and INTMASK to 0 */
>> qemu_irq_raise(s->scsi_reset);
>> - s->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
>> + ns->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
>> qemu_irq_lower(s->scsi_reset);
>> }
>> if (val & SCSICSR_DMADIR) {
>> @@ -838,6 +842,54 @@ static void nextscsi_write(void *opaque, uint8_t *buf, int size)
>> nextdma_write(opaque, buf, size, NEXTDMA_SCSI);
>> }
>>
>> +static void next_scsi_csr_write(void *opaque, hwaddr addr, uint64_t val,
>> + unsigned size)
>> +{
>> + NeXTSCSI *s = NEXT_SCSI(opaque);
>> +
>> + switch (addr) {
>> + case 0:
>> + s->scsi_csr_1 = val;
>> + break;
>> +
>> + case 1:
>> + s->scsi_csr_2 = val;
>> + break;
>
> The old code never set the scsi_csr_x directly like this, so I'm not sure
> whether this is right?
Well I initially did this on a hunch that something had gone wrong with an earlier
refactoring, but I just did a quick check with Previous and it also treats them as
normal registers (see
https://sourceforge.net/p/previous/code/HEAD/tree/trunk/src/esp.c#l160). So I think
this should be fine for now?
> Also, maybe best squash this patch together with the next patch, otherwise
> this is temporary change in behaviour, isn't it?
If possible could I keep it as-is? It just means there is separation between the
change of the memory region topology and then consolidating the SCSI CSR access
routines in the following patch.
>> + default:
>> + g_assert_not_reached();
>> + }
>> +}
>> +
>> +static uint64_t next_scsi_csr_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> + NeXTSCSI *s = NEXT_SCSI(opaque);
>> + uint64_t val;
>> +
>> + switch (addr) {
>> + case 0:
>> + val = s->scsi_csr_1;
>> + break;
>> +
>> + case 1:
>> + val = s->scsi_csr_2;
>> + break;
>> +
>> + default:
>> + g_assert_not_reached();
>> + }
>> +
>> + return val;
>> +}
>> +
>> +static const MemoryRegionOps next_scsi_csr_ops = {
>> + .read = next_scsi_csr_read,
>> + .write = next_scsi_csr_write,
>> + .valid.min_access_size = 1,
>> + .valid.max_access_size = 1,
>> + .endianness = DEVICE_BIG_ENDIAN,
>> +};
>> +
>> static void next_scsi_init(Object *obj)
>> {
>> NeXTSCSI *s = NEXT_SCSI(obj);
>> @@ -845,6 +897,9 @@ static void next_scsi_init(Object *obj)
>>
>> object_initialize_child(obj, "esp", &s->sysbus_esp, TYPE_SYSBUS_ESP);
>>
>> + memory_region_init_io(&s->scsi_csr_mem, obj, &next_scsi_csr_ops,
>> + s, "csrs", 2);
>> +
>> memory_region_init(&s->scsi_mem, obj, "next.scsi", 0x40);
>> sysbus_init_mmio(sbd, &s->scsi_mem);
>> }
>> @@ -874,15 +929,30 @@ static void next_scsi_realize(DeviceState *dev, Error **errp)
>> memory_region_add_subregion(&s->scsi_mem, 0x0,
>> sysbus_mmio_get_region(sbd, 0));
>>
>> + /* SCSI CSRs */
>> + memory_region_add_subregion(&s->scsi_mem, 0x20, &s->scsi_csr_mem);
>> +
>> scsi_bus_legacy_handle_cmdline(&s->sysbus_esp.esp.bus);
>> }
>>
>> +static const VMStateDescription next_scsi_vmstate = {
>> + .name = "next-scsi",
>> + .version_id = 0,
>> + .minimum_version_id = 0,
>> + .fields = (const VMStateField[]) {
>> + VMSTATE_UINT8(scsi_csr_1, NeXTSCSI),
>> + VMSTATE_UINT8(scsi_csr_2, NeXTSCSI),
>> + VMSTATE_END_OF_LIST()
>> + },
>> +};
>> +
>> static void next_scsi_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>>
>> dc->desc = "NeXT SCSI Controller";
>> dc->realize = next_scsi_realize;
>> + dc->vmsd = &next_scsi_vmstate;
>> }
>>
>> static const TypeInfo next_scsi_info = {
>> @@ -1000,8 +1070,8 @@ static const VMStateDescription next_rtc_vmstate = {
>>
>> static const VMStateDescription next_pc_vmstate = {
>> .name = "next-pc",
>> - .version_id = 2,
>> - .minimum_version_id = 2,
>> + .version_id = 3,
>> + .minimum_version_id = 3,
>> .fields = (const VMStateField[]) {
>> VMSTATE_UINT32(scr1, NeXTPC),
>> VMSTATE_UINT32(scr2, NeXTPC),
>> @@ -1009,8 +1079,6 @@ static const VMStateDescription next_pc_vmstate = {
>> VMSTATE_UINT32(int_mask, NeXTPC),
>> VMSTATE_UINT32(int_status, NeXTPC),
>> VMSTATE_UINT32(led, NeXTPC),
>> - VMSTATE_UINT8(scsi_csr_1, NeXTPC),
>> - VMSTATE_UINT8(scsi_csr_2, NeXTPC),
>> VMSTATE_STRUCT(rtc, NeXTPC, 0, next_rtc_vmstate, NextRtc),
>> VMSTATE_END_OF_LIST()
>> },
ATB,
Mark.
Am Mon, 28 Oct 2024 22:21:20 +0000
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
> On 28/10/2024 16:21, Thomas Huth wrote:
>
> > Am Wed, 23 Oct 2024 09:58:25 +0100
> > schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
> >
> >> The SCSI CSRs are located within the SCSI subsystem of the NeXT PC (Peripheral
> >> Contoller) which is now modelled as a separate QEMU device.
> >>
> >> Add a new VMStateDescription for the next-scsi device to enable the SCSI CSRs
> >> to be migrated.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >> hw/m68k/next-cube.c | 88 +++++++++++++++++++++++++++++++++++++++------
> >> 1 file changed, 78 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> >> index 266f57ac63..32466a425f 100644
> >> --- a/hw/m68k/next-cube.c
> >> +++ b/hw/m68k/next-cube.c
> >> @@ -93,6 +93,10 @@ struct NeXTSCSI {
> >> MemoryRegion scsi_mem;
> >>
> >> SysBusESPState sysbus_esp;
> >> +
> >> + MemoryRegion scsi_csr_mem;
> >> + uint8_t scsi_csr_1;
> >> + uint8_t scsi_csr_2;
> >> };
> >>
> >> #define TYPE_NEXT_PC "next-pc"
> >> @@ -115,8 +119,6 @@ struct NeXTPC {
> >> uint32_t led;
> >>
> >> NeXTSCSI next_scsi;
> >> - uint8_t scsi_csr_1;
> >> - uint8_t scsi_csr_2;
> >>
> >> qemu_irq scsi_reset;
> >> qemu_irq scsi_dma;
> >> @@ -364,6 +366,7 @@ static const MemoryRegionOps next_mmio_ops = {
> >> static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
> >> {
> >> NeXTPC *s = NEXT_PC(opaque);
> >> + NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
> >> uint64_t val;
> >>
> >> switch (addr) {
> >> @@ -373,12 +376,12 @@ static uint64_t next_scr_readfn(void *opaque, hwaddr addr, unsigned size)
> >> break;
> >>
> >> case 0x14020:
> >> - DPRINTF("SCSI 4020 STATUS READ %X\n", s->scsi_csr_1);
> >> - val = s->scsi_csr_1;
> >> + DPRINTF("SCSI 4020 STATUS READ %X\n", ns->scsi_csr_1);
> >> + val = ns->scsi_csr_1;
> >> break;
> >>
> >> case 0x14021:
> >> - DPRINTF("SCSI 4021 STATUS READ %X\n", s->scsi_csr_2);
> >> + DPRINTF("SCSI 4021 STATUS READ %X\n", ns->scsi_csr_2);
> >> val = 0x40;
> >> break;
> >>
> >> @@ -411,6 +414,7 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
> >> unsigned size)
> >> {
> >> NeXTPC *s = NEXT_PC(opaque);
> >> + NeXTSCSI *ns = NEXT_SCSI(&s->next_scsi);
> >>
> >> switch (addr) {
> >> case 0x14108:
> >> @@ -445,7 +449,7 @@ static void next_scr_writefn(void *opaque, hwaddr addr, uint64_t val,
> >> DPRINTF("SCSICSR Reset\n");
> >> /* I think this should set DMADIR. CPUDMA and INTMASK to 0 */
> >> qemu_irq_raise(s->scsi_reset);
> >> - s->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
> >> + ns->scsi_csr_1 &= ~(SCSICSR_INTMASK | 0x80 | 0x1);
> >> qemu_irq_lower(s->scsi_reset);
> >> }
> >> if (val & SCSICSR_DMADIR) {
> >> @@ -838,6 +842,54 @@ static void nextscsi_write(void *opaque, uint8_t *buf, int size)
> >> nextdma_write(opaque, buf, size, NEXTDMA_SCSI);
> >> }
> >>
> >> +static void next_scsi_csr_write(void *opaque, hwaddr addr, uint64_t val,
> >> + unsigned size)
> >> +{
> >> + NeXTSCSI *s = NEXT_SCSI(opaque);
> >> +
> >> + switch (addr) {
> >> + case 0:
> >> + s->scsi_csr_1 = val;
> >> + break;
> >> +
> >> + case 1:
> >> + s->scsi_csr_2 = val;
> >> + break;
> >
> > The old code never set the scsi_csr_x directly like this, so I'm not sure
> > whether this is right?
>
> Well I initially did this on a hunch that something had gone wrong with an earlier
> refactoring, but I just did a quick check with Previous and it also treats them as
> normal registers (see
> https://sourceforge.net/p/previous/code/HEAD/tree/trunk/src/esp.c#l160). So I think
> this should be fine for now?
Ok, fine for me, but then please mention it in the commit description!
> > Also, maybe best squash this patch together with the next patch, otherwise
> > this is temporary change in behaviour, isn't it?
>
> If possible could I keep it as-is? It just means there is separation between the
> change of the memory region topology and then consolidating the SCSI CSR access
> routines in the following patch.
Sure, it was just a suggestion! Keep it separate if you prefer it that way.
Thomas
© 2016 - 2024 Red Hat, Inc.