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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 14996 bytes --]

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 <hpoussin@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?

> Note that we do nothing with the value, except giving it back in prep_port081c_read()
> 
> > 
> > > +}
> > > +
> > > +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 ? 1 : 0);
> > > +    s->iomap_type = val;
> > 
> > Again, should this be masked?
> 
> Again some reserved bits. Let's take them unconditionally.
> 
> > 
> > > +}
> > > +
> > > +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 = 0; /* contiguous mode XXX 0x1? */
> > > +    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(system_control, PrepSystemIoState),
> > > +        VMSTATE_UINT8(iomap_type, PrepSystemIoState),
> > > +        VMSTATE_END_OF_LIST()
> > 
> > I'm not sure how useful this will be given how much other stuff in
> > PReP won't migrate properly.
> 
> PReP is mostly a PC with a PowerPC CPU, so most devices are already migration-safe.
> 
> > 
> > > +    },
> > > +};
> > > +
> > > +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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]