Re: [Qemu-ppc] [PATCH v2 2/4] prep: add PReP System I/O

[David Gibson]

On Thu, Jan 05, 2017 at 10:56:55PM +0100, Hervé Poussineau wrote:
> This device is a partial duplicate of System I/O device available in 
> hw/ppc/prep.c
> This new one doesn't have all the Motorola-specific registers.
> The old one should be deprecated and removed with the 'prep' machine.
> 
> Partial documentation available at
> ftp://ftp.software.ibm.com/rs6000/technology/spec/srp1_1.exe
> section 6.1.5 (I/O Device Mapping)
> 
> Signed-off-by: Hervé Poussineau <address@hidden>

With one exception noted below,

Reviewed-by: David Gibson <address@hidden>

> ---
>  hw/ppc/Makefile.objs   |   1 +
>  hw/ppc/prep_systemio.c | 309 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/ppc/trace-events    |   4 +
>  3 files changed, 314 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..fa05cb0
> --- /dev/null
> +++ b/hw/ppc/prep_systemio.c
> @@ -0,0 +1,309 @@
> +/*
> + * QEMU PReP System I/O emulation
> + *
> + * Copyright (c) 2017 Hervé 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);
> +
> +    s->sreset = val & PORT0092_SOFTRESET;
> +    qemu_set_irq(s->softreset_irq, s->sreset);
> +
> +    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;
> +    uint8_t ret = 0;
> +
> +    /* XXX LE mode unsupported */
> +    if (s->sreset) {
> +        ret |= PORT0092_SOFTRESET;
> +    }

So, I asked you to change this to make read and write match logically,
but now you've changed both sides so they still don't match.

Now that write sets s->sreset directly to (masked) val, read should
just return it plain as well.

Basically it needs to be clear whether s->sreset represents the
register value itself, or is just a boolean flag.  In the previous
version, write treated it like a boolean and read treated it like a
register value.  Now write treats it like a register value and read
treats it like a boolean.  Pick one.

> +    trace_prep_systemio_read(addr, ret);
> +    return ret;
> +}
> +
> +/* 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)
> +{
> +    static const uint8_t mask = PORT081C_FLOPPY_MOTOR_INHIBIT |
> +                                PORT081C_MASK_TEA |
> +                                PORT081C_L2_UPDATE_INHIBIT |
> +                                PORT081C_L2_CACHEMISS_INHIBIT;
> +    PrepSystemIoState *s = opaque;
> +    trace_prep_systemio_write(addr, val);
> +    s->system_control = val & mask;
> +}
> +
> +static uint32_t prep_port081c_read(void *opaque, uint32_t addr)
> +{
> +    PrepSystemIoState *s = opaque;
> +    trace_prep_systemio_read(addr, s->system_control);
> +    return s->system_control;
> +}
> +
> +/* System Board Identification */
> +
> +static uint32_t prep_port0852_read(void *opaque, uint32_t addr)
> +{
> +    PrepSystemIoState *s = opaque;
> +    trace_prep_systemio_read(addr, s->ibm_planar_id);
> +    return s->ibm_planar_id;
> +}
> +
> +/* PORT 0850 -- I/O Map Type Register (Read/Write) */
> +
> +enum {
> +    PORT0850_IOMAP_NONCONTIGUOUS    = PREP_BIT(7),
> +};
> +
> +static uint32_t prep_port0850_read(void *opaque, uint32_t addr)
> +{
> +    PrepSystemIoState *s = opaque;
> +    trace_prep_systemio_read(addr, s->iomap_type);
> +    return s->iomap_type;
> +}
> +
> +static void prep_port0850_write(void *opaque, uint32_t addr, uint32_t val)
> +{
> +    PrepSystemIoState *s = opaque;
> +
> +    trace_prep_systemio_write(addr, val);
> +    qemu_set_irq(s->non_contiguous_io_map_irq,
> +                 val & PORT0850_IOMAP_NONCONTIGUOUS);
> +    s->iomap_type = val & PORT0850_IOMAP_NONCONTIGUOUS;
> +}
> +
> +static const MemoryRegionPortio ppc_io800_port_list[] = {
> +    { 0x092, 1, 1, .read = prep_port0092_read,
> +                   .write = prep_port0092_write, },
> +    { 0x808, 1, 1, .write = prep_port0808_write, },
> +    { 0x80c, 1, 1, .read = prep_port080c_read, },
> +    { 0x810, 1, 1, .write = prep_port0810_write, },
> +    { 0x812, 1, 1, .write = prep_port0812_write, },
> +    { 0x814, 1, 1, .write = prep_port0814_write, },
> +    { 0x818, 1, 1, .read = prep_port0818_read },
> +    { 0x81c, 1, 1, .read = prep_port081c_read,
> +                   .write = prep_port081c_write, },
> +    { 0x850, 1, 1, .read = prep_port0850_read,
> +                   .write = prep_port0850_write, },
> +    { 0x852, 1, 1, .read = prep_port0852_read, },
> +    PORTIO_END_OF_LIST()
> +};
> +
> +static uint64_t ppc_parity_error_readl(void *opaque, hwaddr addr,
> +                                       unsigned int size)
> +{
> +    uint32_t val = 0;
> +    trace_prep_systemio_read((unsigned int)addr, val);
> +    return val;
> +}
> +
> +static const MemoryRegionOps ppc_parity_error_ops = {
> +    .read = ppc_parity_error_readl,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +static void prep_systemio_realize(DeviceState *dev, Error **errp)
> +{
> +    ISADevice *isa = ISA_DEVICE(dev);
> +    PrepSystemIoState *s = PREP_SYSTEMIO(dev);
> +    PowerPCCPU *cpu;
> +
> +    qdev_init_gpio_out(dev, &s->non_contiguous_io_map_irq, 1);
> +    s->iomap_type = PORT0850_IOMAP_NONCONTIGUOUS;
> +    qemu_set_irq(s->non_contiguous_io_map_irq,
> +                 s->iomap_type & PORT0850_IOMAP_NONCONTIGUOUS);
> +    cpu = POWERPC_CPU(first_cpu);
> +    s->softreset_irq = cpu->env.irq_inputs[PPC6xx_INPUT_HRESET];
> +
> +    isa_register_portio_list(isa, &s->portio, 0x0, ppc_io800_port_list, s,
> +                             "systemio800");
> +
> +    memory_region_init_io(&s->ppc_parity_mem, OBJECT(dev),
> +                          &ppc_parity_error_ops, s, "ppc-parity", 0x4);
> +    memory_region_add_subregion(get_system_memory(), 0xbfffeff0,
> +                                &s->ppc_parity_mem);
> +}
> +
> +static const VMStateDescription vmstate_prep_systemio = {
> +    .name = "prep_systemio",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(sreset, PrepSystemIoState),
> +        VMSTATE_UINT8(system_control, PrepSystemIoState),
> +        VMSTATE_UINT8(iomap_type, PrepSystemIoState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static Property prep_systemio_properties[] = {
> +    DEFINE_PROP_UINT8("ibm-planar-id", PrepSystemIoState, ibm_planar_id, 0),
> +    DEFINE_PROP_UINT8("equipment", PrepSystemIoState, equipment, 0),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void prep_systemio_class_initfn(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = prep_systemio_realize;
> +    dc->vmsd = &vmstate_prep_systemio;
> +    dc->props = prep_systemio_properties;
> +}
> +
> +static TypeInfo prep_systemio800_info = {
> +    .name          = TYPE_PREP_SYSTEMIO,
> +    .parent        = TYPE_ISA_DEVICE,
> +    .instance_size = sizeof(PrepSystemIoState),
> +    .class_init    = prep_systemio_class_initfn,
> +};
> +
> +static void prep_systemio_register_types(void)
> +{
> +    type_register_static(&prep_systemio800_info);
> +}
> +
> +type_init(prep_systemio_register_types)
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 2297ead..2ba6166 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -74,3 +74,7 @@ ppc_tb_adjust(uint64_t offs1, uint64_t offs2, int64_t diff, 
> int64_t seconds) "ad
>  # hw/ppc/prep.c
>  prep_io_800_writeb(uint32_t addr, uint32_t val) "0x%08" PRIx32 " => 0x%02" 
> PRIx32
>  prep_io_800_readb(uint32_t addr, uint32_t retval) "0x%08" PRIx32 " <= 0x%02" 
> PRIx32
> +
> +# hw/ppc/prep_systemio.c
> +prep_systemio_read(uint32_t addr, uint32_t val) "read addr=%x val=%x"
> +prep_systemio_write(uint32_t addr, uint32_t val) "write addr=%x val=%x"

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature