| Message ID | 20181018130434.23237-4-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/misc: Add a MMIO interface to the pvpanic device | expand |
On 18 October 2018 at 14:04, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Signed-off-by: Peng Hao <peng.hao2@zte.com.cn> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > [PMD: Use TYPE_PVPANIC definition, split in 2 patches] > --- > Peng: I hope this is now more obvious how you could reuse the pvpanic device. > > hw/misc/pvpanic.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c > index 4f552e1533..b81c5fa633 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 <wency@cn.fujitsu.com> > * Hu Tao <hutao@cn.fujitsu.com> > + * Peng Hao <peng.hao2@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. > @@ -47,7 +49,10 @@ static void handle_event(int event) > > typedef struct PVPanicState { > /*< private >*/ > - ISADevice isadev; > + union { > + ISADevice isadev; > + SysBusDevice busdev; > + }; This field is the parent-type for the QOM object, so I don't think it makes sense for it to be a union. Any one QOM object should have a single distinct parent type. (There are other examples of "some more or less similar device has ISA and SysBus or PCI and SysBus variations" which should provide some models to copy from. thanks -- PMM
On 18/10/2018 15:08, Peter Maydell wrote: > On 18 October 2018 at 14:04, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> [PMD: Use TYPE_PVPANIC definition, split in 2 patches] >> --- >> Peng: I hope this is now more obvious how you could reuse the pvpanic device. >> >> hw/misc/pvpanic.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 51 insertions(+), 1 deletion(-) >> >> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c >> index 4f552e1533..b81c5fa633 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 <wency@cn.fujitsu.com> >> * Hu Tao <hutao@cn.fujitsu.com> >> + * Peng Hao <peng.hao2@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. >> @@ -47,7 +49,10 @@ static void handle_event(int event) >> >> typedef struct PVPanicState { >> /*< private >*/ >> - ISADevice isadev; >> + union { >> + ISADevice isadev; >> + SysBusDevice busdev; >> + }; > > This field is the parent-type for the QOM object, so I don't > think it makes sense for it to be a union. Any one QOM object > should have a single distinct parent type. (There are other > examples of "some more or less similar device has ISA and > SysBus or PCI and SysBus variations" which should provide > some models to copy from. OK, I understand as I can use "Device parent_obj" instead of that union? But then we want: pvpanic_isa_info.instance_size = sizeof(PVPanic_Common_State) + sizeof(ISADevice) pvpanic_mmio_info.instance_size = sizeof(PVPanic_Common_State) + sizeof(SysBusDevice) So to avoid union, I have to use: typedef struct PVPanicCommonState { MemoryRegion mr; uint16_t ioport; } PVPanicCommonState; typedef struct PVPanicISAState { /*< private >*/ ISADevice isadev; /*< public >*/ PVPanicCommonState common; } PVPanicISAState; #define PVPANIC_ISA(obj) \ OBJECT_CHECK(PVPanicISAState, (obj), TYPE_PVPANIC) typedef struct PVPanicMMIOState { /*< private >*/ SysBusDevice busdev; /*< public >*/ PVPanicCommonState common; } PVPanicMMIOState; #define PVPANIC_MMIO(obj) \ OBJECT_CHECK(PVPanicMMIOState, (obj), TYPE_PVPANIC) And later: -->8-- @@ -92,3 +104,3 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) ISADevice *d = ISA_DEVICE(dev); - PVPanicState *s = PVPANIC(dev); + PVPanicState *s = PVPANIC_ISA(dev); FWCfgState *fw_cfg = fw_cfg_find(); @@ -125,3 +137,3 @@ static TypeInfo pvpanic_isa_info = { .parent = TYPE_ISA_DEVICE, - .instance_size = sizeof(PVPanicState), + .instance_size = sizeof(PVPanicISAState), .instance_init = pvpanic_isa_initfn, @@ -151,3 +163,3 @@ static void pvpanic_mmio_initfn(Object *obj) { - PVPanicState *s = PVPANIC(obj); + PVPanicState *s = PVPANIC_MMIO(obj); SysBusDevice *sbd = SYS_BUS_DEVICE(obj); @@ -169,3 +181,3 @@ static TypeInfo pvpanic_mmio_info = { .parent = TYPE_SYS_BUS_DEVICE, - .instance_size = sizeof(PVPanicState), + .instance_size = sizeof(PVPanicMMIOState), .instance_init = pvpanic_mmio_initfn, --- Am I correct? Thanks, Phil.
On 18 October 2018 at 14:19, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 18/10/2018 15:08, Peter Maydell wrote: >> This field is the parent-type for the QOM object, so I don't >> think it makes sense for it to be a union. Any one QOM object >> should have a single distinct parent type. (There are other >> examples of "some more or less similar device has ISA and >> SysBus or PCI and SysBus variations" which should provide >> some models to copy from. > So to avoid union, I have to use: > > typedef struct PVPanicCommonState { > MemoryRegion mr; > uint16_t ioport; > } PVPanicCommonState; > > typedef struct PVPanicISAState { > /*< private >*/ > ISADevice isadev; > > /*< public >*/ > PVPanicCommonState common; > } PVPanicISAState; > > #define PVPANIC_ISA(obj) \ > OBJECT_CHECK(PVPanicISAState, (obj), TYPE_PVPANIC) > > typedef struct PVPanicMMIOState { > /*< private >*/ > SysBusDevice busdev; > > /*< public >*/ > PVPanicCommonState common; > } PVPanicMMIOState; > > #define PVPANIC_MMIO(obj) \ > OBJECT_CHECK(PVPanicMMIOState, (obj), TYPE_PVPANIC) > Something like that, I think. Basically you have two distinct types, an ISA PVPanic and a SysBus PVPanic, which have straightforward inheritance (the ISA PVPanic is-a ISADevice, the SysBus PVPanic is-a SysBusdevice). Both have-a PVPanic common state. See eg hw/display/sm501.c, or hw/net/ne2000{,-isa}.c. thanks -- PMM
> >Signed-off-by: Peng Hao <peng.hao2@zte.com.cn> >Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >[PMD: Use TYPE_PVPANIC definition, split in 2 patches] >--- >Peng: I hope this is now more obvious how you could reuse the pvpanic device. > Thanks. I will think about your suggestions and patches. >hw/misc/pvpanic.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++- >1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c index 4f552e1533..b81c5fa633 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 <wency@cn.fujitsu.com> * Hu Tao <hutao@cn.fujitsu.com> + * Peng Hao <peng.hao2@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. @@ -47,7 +49,10 @@ static void handle_event(int event) typedef struct PVPanicState { /*< private >*/ - ISADevice isadev; + union { + ISADevice isadev; + SysBusDevice busdev; + }; /*< public >*/ MemoryRegion mr; @@ -123,9 +128,54 @@ static TypeInfo pvpanic_isa_info = { .class_init = pvpanic_isa_class_init, }; +static uint64_t pvpanic_mmio_read(void *opaque, hwaddr addr, unsigned size) +{ + return -1; +} + +static void pvpanic_mmio_write(void *opaque, hwaddr addr, uint64_t value, + unsigned size) +{ + handle_event(value); +} + +static const MemoryRegionOps pvpanic_mmio_ops = { + .read = pvpanic_mmio_read, + .write = pvpanic_mmio_write, + .impl = { + .max_access_size = 1, + }, +}; + +static void pvpanic_mmio_initfn(Object *obj) +{ + PVPanicState *s = PVPANIC(obj); + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); + + memory_region_init_io(&s->mr, OBJECT(s), &pvpanic_mmio_ops, s, + TYPE_PVPANIC, 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, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(PVPanicState), + .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); } type_init(pvpanic_register_types)