On 16 November 2018 at 10:50, Peng Hao <peng.h...@zte.com.cn> wrote: > Add pvpanic new type "TYPE_PVPANIC_MMIO" > > Signed-off-by: Peng Hao <peng.h...@zte.com.cn> > Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
I'm not entirely sure I understand why we have two signed-off-by lines here -- who is the author of this patch? > --- > hw/misc/pvpanic.c | 81 > +++++++++++++++++++++++++++++++++++++---------- > include/hw/misc/pvpanic.h | 1 + > 2 files changed, 65 insertions(+), 17 deletions(-) > > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c > index dd3aef2..5d0fbc6 100644 > --- a/hw/misc/pvpanic.c > +++ b/hw/misc/pvpanic.c > @@ -2,10 +2,12 @@ > * QEMU simulated pvpanic device. > * > * Copyright Fujitsu, Corp. 2013 > + * Copyright (c) 2018 ZTE Ltd. > * > * Authors: > * Wen Congyang <we...@cn.fujitsu.com> > * Hu Tao <hu...@cn.fujitsu.com> > + * Peng Hao <peng.h...@zte.com.cn> > * > * This work is licensed under the terms of the GNU GPL, version 2 or later. > * See the COPYING file in the top-level directory. > @@ -25,9 +27,6 @@ > /* The pv event value */ > #define PVPANIC_PANICKED (1 << PVPANIC_F_PANICKED) > > -#define PVPANIC(obj) \ > - OBJECT_CHECK(PVPanicState, (obj), TYPE_PVPANIC) > - We just added (renamed) this macro in the previous patch: has something gone wrong in a rebase ? > static void handle_event(int event) > { > static bool logged; > @@ -45,30 +44,50 @@ static void handle_event(int event) > > #include "hw/isa/isa.h" > > -typedef struct PVPanicState { > - /* private */ > - ISADevice isadev; > +/* PVPanicISAState for ISA device and > + * use ioport. > + */ > +typedef struct PVPanicISAState { > + /* private */ > + ISADevice isadev; This should be "ISADevice parent_obj;" and go above the /* private */ comment, because a PVPanicISAState is-a ISADevice (this is not a has-a relation). Again, this seems to be undoing changes from the previous patch somewhat ? > + uint16_t ioport; > > /* public */ > MemoryRegion mr; > - uint16_t ioport; > -} PVPanicState; > +} PVPanicISAState; > + > +/* PVPanicMMIOState for sysbus device and > + * use mmio. > + */ > +typedef struct PVPanicMMIOState { > + /* private */ > + SysBusDevice busdev; > + > + /* public */ > + MemoryRegion mr; > +} PVPanicMMIOState; > + > +#define PVPANIC_ISA(obj) \ > + OBJECT_CHECK(PVPanicISAState, (obj), TYPE_PVPANIC) > + > +#define PVPANIC_MMIO(obj) \ > + OBJECT_CHECK(PVPanicMMIOState, (obj), TYPE_PVPANIC_MMIO) > > /* return supported events on read */ > -static uint64_t pvpanic_ioport_read(void *opaque, hwaddr addr, unsigned size) > +static uint64_t pvpanic_read(void *opaque, hwaddr addr, unsigned size) > { > return PVPANIC_PANICKED; > } > > -static void pvpanic_ioport_write(void *opaque, hwaddr addr, uint64_t val, > +static void pvpanic_write(void *opaque, hwaddr addr, uint64_t val, > unsigned size) > { > handle_event(val); > } > > static const MemoryRegionOps pvpanic_ops = { > - .read = pvpanic_ioport_read, > - .write = pvpanic_ioport_write, > + .read = pvpanic_read, > + .write = pvpanic_write, > .impl = { > .min_access_size = 1, > .max_access_size = 1, > @@ -77,15 +96,16 @@ static const MemoryRegionOps pvpanic_ops = { > > static void pvpanic_isa_initfn(Object *obj) > { > - PVPanicState *s = PVPANIC(obj); > + PVPanicISAState *s = PVPANIC_ISA(obj); > > - memory_region_init_io(&s->mr, OBJECT(s), &pvpanic_ops, s, "pvpanic", 1); > + memory_region_init_io(&s->mr, OBJECT(s), &pvpanic_ops, s, > + TYPE_PVPANIC, 1); > } > > static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) > { > ISADevice *d = ISA_DEVICE(dev); > - PVPanicState *s = PVPANIC(dev); > + PVPanicISAState *s = PVPANIC_ISA(dev); > FWCfgState *fw_cfg = fw_cfg_find(); > uint16_t *pvpanic_port; > > @@ -102,7 +122,7 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error > **errp) > } > > static Property pvpanic_isa_properties[] = { > - DEFINE_PROP_UINT16(PVPANIC_IOPORT_PROP, PVPanicState, ioport, 0x505), > + DEFINE_PROP_UINT16(PVPANIC_IOPORT_PROP, PVPanicISAState, ioport, 0x505), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -118,14 +138,41 @@ static void pvpanic_isa_class_init(ObjectClass *klass, > void *data) > static TypeInfo pvpanic_isa_info = { > .name = TYPE_PVPANIC, > .parent = TYPE_ISA_DEVICE, > - .instance_size = sizeof(PVPanicState), > + .instance_size = sizeof(PVPanicISAState), > .instance_init = pvpanic_isa_initfn, > .class_init = pvpanic_isa_class_init, > }; > > + > +static void pvpanic_mmio_initfn(Object *obj) > +{ > + PVPanicMMIOState *s = PVPANIC_MMIO(obj); > + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > + > + memory_region_init_io(&s->mr, OBJECT(s), &pvpanic_ops, s, > + TYPE_PVPANIC_MMIO, 2); > + sysbus_init_mmio(sbd, &s->mr); > +} > + > +static void pvpanic_mmio_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + set_bit(DEVICE_CATEGORY_MISC, dc->categories); > +} > + > +static TypeInfo pvpanic_mmio_info = { > + .name = TYPE_PVPANIC_MMIO, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(PVPanicMMIOState), > + .instance_init = pvpanic_mmio_initfn, > + .class_init = pvpanic_mmio_class_init, > +}; > + > static void pvpanic_register_types(void) > { > type_register_static(&pvpanic_isa_info); > + type_register_static(&pvpanic_mmio_info); > } I would suggest refactoring the ISA device in one patch, and then adding the MMIO device in a second patch. Currently this patch is mixing the two. thanks -- PMM