Re: [PATCH 05/21] q800: add IOSB subsystem

[Mark Cave-Ayland]

On 07/07/2023 09:25, Philippe Mathieu-Daudé wrote:

> On 2/7/23 17:48, Mark Cave-Ayland wrote:
> > It is needed because it defines the BIOSConfig area.
> >
> > Co-developed-by: Laurent Vivier <laurent@vivier.eu>
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   MAINTAINERS            |   2 +
> >   hw/m68k/Kconfig        |   1 +
> >   hw/m68k/q800.c         |   9 +++
> >   hw/misc/Kconfig        |   3 +
> >   hw/misc/iosb.c         | 137 +++++++++++++++++++++++++++++++++++++++++
> >   hw/misc/meson.build    |   1 +
> >   hw/misc/trace-events   |   4 ++
> >   include/hw/m68k/q800.h |   2 +
> >   include/hw/misc/iosb.h |  25 ++++++++
> >   9 files changed, 184 insertions(+)
> >   create mode 100644 hw/misc/iosb.c
> >   create mode 100644 include/hw/misc/iosb.h
>
>
> > diff --git a/hw/misc/iosb.c b/hw/misc/iosb.c
> > new file mode 100644
> > index 0000000000..4fc10bcf9f
> > --- /dev/null
> > +++ b/hw/misc/iosb.c
> > @@ -0,0 +1,137 @@
> > +/*
> > + * QEMU IOSB emulation
> > + *
> > + * Copyright (c) 2019 Laurent Vivier
> > + * Copyright (c) 2022 Mark Cave-Ayland
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/log.h"
> > +#include "migration/vmstate.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/misc/iosb.h"
> > +#include "trace.h"
> > +
> > +#define IOSB_SIZE          0x2000
> > +
> > +#define IOSB_CONFIG        0x0
> > +#define IOSB_CONFIG2       0x100
> > +#define IOSB_SONIC_SCSI    0x200
> > +#define IOSB_REVISION      0x300
> > +#define IOSB_SCSI_RESID    0x400
> > +#define IOSB_BRIGHTNESS    0x500
> > +#define IOSB_TIMEOUT       0x600
> > +
> > +
> > +static uint64_t iosb_read(void *opaque, hwaddr addr,
> > +                          unsigned size)
> > +{
> > +    IOSBState *s = IOSB(opaque);
> > +    uint64_t val = 0;
> > +
> > +    switch (addr) {
> > +    case IOSB_CONFIG:
> > +    case IOSB_CONFIG2:
> > +    case IOSB_SONIC_SCSI:
> > +    case IOSB_REVISION:
> > +    case IOSB_SCSI_RESID:
> > +    case IOSB_BRIGHTNESS:
> > +    case IOSB_TIMEOUT:
> > +        val = s->regs[addr >> 8];
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP, "IOSB: unimplemented read addr=0x%"PRIx64
> > +                                 " val=0x%"PRIx64 " size=%d\n",
> > +                                 addr, val, size);
> > +    }
> > +
> > +    trace_iosb_read(addr, val, size);
> > +    return val;
> > +}
> > +
> > +static void iosb_write(void *opaque, hwaddr addr, uint64_t val,
> > +                       unsigned size)
> > +{
> > +    IOSBState *s = IOSB(opaque);
> > +
> > +    switch (addr) {
> > +    case IOSB_CONFIG:
> > +    case IOSB_CONFIG2:
> > +    case IOSB_SONIC_SCSI:
> > +    case IOSB_REVISION:
> > +    case IOSB_SCSI_RESID:
> > +    case IOSB_BRIGHTNESS:
> > +    case IOSB_TIMEOUT:
> > +        s->regs[addr >> 8] = val;
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP, "IOSB: unimplemented write addr=0x%"PRIx64
> > +                                 " val=0x%"PRIx64 " size=%d\n",
> > +                                 addr, val, size);
> > +    }
> > +
> > +    trace_iosb_write(addr, val, size);
> > +}
> > +
> > +static const MemoryRegionOps iosb_mmio_ops = {
> > +    .read = iosb_read,
> > +    .write = iosb_write,
> > +    .endianness = DEVICE_BIG_ENDIAN,
> > +    .impl = {
> > +        .min_access_size = 1,
>
> IIUC .impl.min_access_size = 4.
>
> Do you mean .valid.min_access_size = 1?

Hmmm I can't remember the exact origin of this, but indeed looking at the stride this 
doesn't make much sense. I've done some tests with .impl removed completely and 
haven't seen any issues, so I'll drop this from v2.

> Otherwise,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> > +        .max_access_size = 4,
> > +    },
> > +};


ATB,

Mark.