Re: [Qemu-devel] [PATCH 4/6] prep: QOM'ify System I/O

[Hervé Poussineau]

Le 05/01/2017 à 01:41, David Gibson a écrit :

On Wed, Jan 04, 2017 at 10:17:10PM +0100, Hervé Poussineau wrote:

Le 03/01/2017 à 05:45, David Gibson a écrit :

On Thu, Dec 29, 2016 at 11:12:14PM +0100, Hervé Poussineau wrote:

Part of the functionality is copied from hw/ppc/prep.c.
Also add support for board identification/equipment registers.

Signed-off-by: Hervé Poussineau <hpous...@reactos.org>
---
 hw/ppc/Makefile.objs   |   1 +
 hw/ppc/prep_systemio.c | 302 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/trace-events    |   4 +
 3 files changed, 307 insertions(+)
 create mode 100644 hw/ppc/prep_systemio.c

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 8025129..db72297 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -16,6 +16,7 @@ obj-y += ppc405_boards.o ppc4xx_devs.o ppc405_uc.o 
ppc440_bamboo.o
 obj-y += ppc4xx_pci.o
 # PReP
 obj-$(CONFIG_PREP) += prep.o
+obj-$(CONFIG_PREP) += prep_systemio.o
 # OldWorld PowerMac
 obj-$(CONFIG_MAC) += mac_oldworld.o
 # NewWorld PowerMac
diff --git a/hw/ppc/prep_systemio.c b/hw/ppc/prep_systemio.c
new file mode 100644
index 0000000..449056c
--- /dev/null
+++ b/hw/ppc/prep_systemio.c
@@ -0,0 +1,302 @@
+/*
+ * QEMU PReP System I/O emulation
+ *
+ * Copyright (c) 2016 Herve Poussineau
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/isa/isa.h"
+#include "exec/address-spaces.h"
+#include "qemu/error-report.h" /* for error_report() */
+#include "sysemu/sysemu.h" /* for vm_stop() */
+#include "cpu.h"
+#include "trace.h"
+
+#define TYPE_PREP_SYSTEMIO "prep-systemio"
+#define PREP_SYSTEMIO(obj) \
+    OBJECT_CHECK(PrepSystemIoState, (obj), TYPE_PREP_SYSTEMIO)
+
+/* Bit as defined in PowerPC Reference Plaform v1.1, sect. 6.1.5, p. 132 */
+#define PREP_BIT(n) (1 << (7 - (n)))
+
+typedef struct PrepSystemIoState {
+    ISADevice parent_obj;
+    MemoryRegion ppc_parity_mem;
+
+    qemu_irq non_contiguous_io_map_irq;
+    uint8_t sreset; /* 0x0092 */
+    uint8_t equipment; /* 0x080c */
+    uint8_t system_control; /* 0x081c */
+    uint8_t iomap_type; /* 0x0850 */
+    uint8_t ibm_planar_id; /* 0x0852 */
+    qemu_irq softreset_irq;
+    PortioList portio;
+} PrepSystemIoState;
+
+/* PORT 0092 -- Special Port 92 (Read/Write) */
+
+enum {
+    PORT0092_SOFTRESET  = PREP_BIT(7),
+    PORT0092_LE_MODE    = PREP_BIT(6),
+};
+
+static void prep_port0092_write(void *opaque, uint32_t addr, uint32_t val)
+{
+    PrepSystemIoState *s = opaque;
+
+    trace_prep_systemio_write(addr, val);
+
+    if ((val & PORT0092_SOFTRESET) != 0) {
+        qemu_irq_raise(s->softreset_irq);
+        s->sreset = 1;
+    } else {
+        qemu_irq_lower(s->softreset_irq);
+        s->sreset = 0;

You could just use
        s->sreset = val & PORT0092_SOFTRESET;
        qemu_set_irq(irq, s->sreset);
here, rather than the if.

OK

+    }
+
+    if ((val & PORT0092_LE_MODE) != 0) {
+        /* XXX Not supported yet */
+        error_report("little-endian mode not supported");
+        vm_stop(RUN_STATE_PAUSED);
+    } else {
+        /* Nothing to do */
+    }
+}
+
+static uint32_t prep_port0092_read(void *opaque, uint32_t addr)
+{
+    PrepSystemIoState *s = opaque;
+    /* XXX LE mode unsupported */
+    trace_prep_systemio_read(addr, 0);
+    return s->sreset;

This doesn't seem to quite logically match the write side.  On the
write side you convert the PORT0092_SOFTRESET bit into an explicit
0/1, here you return the raw 0/1.  It ends up being the same thing
because PORT0092_SOFTRESET == 0x01, but that's not terribly obvious.

OK, will change that.

+}
+
+/* PORT 0808 -- Hardfile Light Register (Write Only) */
+
+enum {
+    PORT0808_HARDFILE_LIGHT_ON  = PREP_BIT(7),
+};
+
+static void prep_port0808_write(void *opaque, uint32_t addr, uint32_t val)
+{
+    trace_prep_systemio_write(addr, val);
+}
+
+/* PORT 0810 -- Password Protect 1 Register (Write Only) */
+
+/* reset by port 0x4D in the SIO */
+static void prep_port0810_write(void *opaque, uint32_t addr, uint32_t val)
+{
+    trace_prep_systemio_write(addr, val);
+}
+
+/* PORT 0812 -- Password Protect 2 Register (Write Only) */
+
+/* reset by port 0x4D in the SIO */
+static void prep_port0812_write(void *opaque, uint32_t addr, uint32_t val)
+{
+    trace_prep_systemio_write(addr, val);
+}
+
+/* PORT 0814 -- L2 Invalidate Register (Write Only) */
+
+static void prep_port0814_write(void *opaque, uint32_t addr, uint32_t val)
+{
+    trace_prep_systemio_write(addr, val);
+}
+
+/* PORT 0818 -- Reserved for Keylock (Read Only) */
+
+enum {
+    PORT0818_KEYLOCK_SIGNAL_HIGH    = PREP_BIT(7),
+};
+
+static uint32_t prep_port0818_read(void *opaque, uint32_t addr)
+{
+    uint32_t val = 0;
+    trace_prep_systemio_read(addr, val);
+    return val;
+}
+
+/* PORT 080C -- Equipment */
+
+enum {
+    PORT080C_SCSIFUSE               = PREP_BIT(1),
+    PORT080C_L2_COPYBACK            = PREP_BIT(4),
+    PORT080C_L2_256                 = PREP_BIT(5),
+    PORT080C_UPGRADE_CPU            = PREP_BIT(6),
+    PORT080C_L2                     = PREP_BIT(7),
+};
+
+static uint32_t prep_port080c_read(void *opaque, uint32_t addr)
+{
+    PrepSystemIoState *s = opaque;
+    trace_prep_systemio_read(addr, s->equipment);
+    return s->equipment;
+}
+
+/* PORT 081C -- System Control Register (Read/Write) */
+
+enum {
+    PORT081C_FLOPPY_MOTOR_INHIBIT   = PREP_BIT(3),
+    PORT081C_MASK_TEA               = PREP_BIT(2),
+    PORT081C_L2_UPDATE_INHIBIT      = PREP_BIT(1),
+    PORT081C_L2_CACHEMISS_INHIBIT   = PREP_BIT(0),
+};
+
+static void prep_port081c_write(void *opaque, uint32_t addr, uint32_t val)
+{
+    PrepSystemIoState *s = opaque;
+    trace_prep_systemio_write(addr, val);
+    s->system_control = val;

Should this value be masked to restrict it to the valid bits?

Some bits are reserved according to specification, and must be
written as 0.

Right, so I assumed.

I think we can still take them unconditionally.

Hrm.. allowing reserved bits to be set and retrieved is usually a bad
idea.  Do you have a particular reason to allow this?

Not much. I've changed it to ignore reserved bits in write handler, so they are 
read back as 0.

Hervé