| Message ID | 20230908222859.3381003-2-ninad@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce model for IBM's FSI | expand |
On 9/9/23 00:28, Ninad Palsule wrote: > This is a part of patchset where IBM's Flexible Service Interface is > introduced. > > The LBUS is modelled to maintain the qdev bus hierarchy and to take > advantage of the object model to automatically generate the CFAM > configuration block. The configuration block presents engines in the > order they are attached to the CFAM's LBUS. Engine implementations > should subclass the LBusDevice and set the 'config' member of > LBusDeviceClass to match the engine's type. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com> > --- > v2: > - Incorporated Joel's review comments. > --- It is nice to have the header files upfront. orderFile = /path/to/qemu/scripts/git.orderfile > hw/Kconfig | 1 + > hw/fsi/Kconfig | 2 + > hw/fsi/lbus.c | 94 +++++++++++++++++++++++++++++++++++++++++++ > hw/fsi/meson.build | 1 + > hw/meson.build | 1 + > include/hw/fsi/lbus.h | 48 ++++++++++++++++++++++ > include/qemu/bitops.h | 6 +++ > 7 files changed, 153 insertions(+) > create mode 100644 hw/fsi/Kconfig > create mode 100644 hw/fsi/lbus.c > create mode 100644 hw/fsi/meson.build > create mode 100644 include/hw/fsi/lbus.h > > diff --git a/hw/Kconfig b/hw/Kconfig > index ba62ff6417..2ccb73add5 100644 > --- a/hw/Kconfig > +++ b/hw/Kconfig > @@ -9,6 +9,7 @@ source core/Kconfig > source cxl/Kconfig > source display/Kconfig > source dma/Kconfig > +source fsi/Kconfig > source gpio/Kconfig > source hyperv/Kconfig > source i2c/Kconfig > diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig > new file mode 100644 > index 0000000000..687449e14e > --- /dev/null > +++ b/hw/fsi/Kconfig > @@ -0,0 +1,2 @@ > +config LBUS > + bool > diff --git a/hw/fsi/lbus.c b/hw/fsi/lbus.c > new file mode 100644 > index 0000000000..afb26ef7ea > --- /dev/null > +++ b/hw/fsi/lbus.c > @@ -0,0 +1,94 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0-or-later > + * Copyright (C) 2023 IBM Corp. > + * > + * IBM Local bus where FSI slaves are connected > + */ > + > +#include "qemu/osdep.h" > + > +#include "qapi/error.h" > +#include "qemu/log.h" > + > +#include "hw/fsi/lbus.h" > + > +#include "hw/qdev-properties.h" > + > +static void lbus_realize(BusState *bus, Error **errp) > +{ > + LBusNode *node; > + LBus *lbus = LBUS(bus); > + > + memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_LBUS, > + (2 * 1024 * 1024) - 0x400); please use some define > + > + QLIST_FOREACH(node, &lbus->devices, next) { > + memory_region_add_subregion(&lbus->mr, node->ldev->address, > + &node->ldev->iomem); > + } > +} > + > +static void lbus_init(Object *o) > +{ > +} > + please remove if unused > +static void lbus_class_init(ObjectClass *klass, void *data) > +{ > + BusClass *k = BUS_CLASS(klass); > + k->realize = lbus_realize; > +} > + > +static const TypeInfo lbus_info = { > + .name = TYPE_LBUS, > + .parent = TYPE_BUS, > + .instance_init = lbus_init, > + .instance_size = sizeof(LBus), > + .class_init = lbus_class_init, > +}; > + > +static Property lbus_device_props[] = { > + DEFINE_PROP_UINT32("address", LBusDevice, address, 0), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +DeviceState *lbus_create_device(LBus *bus, const char *type, uint32_t addr) > +{ > + DeviceState *dev; > + LBusNode *node; > + > + dev = qdev_new(type); > + qdev_prop_set_uint8(dev, "address", addr); > + qdev_realize_and_unref(dev, &bus->bus, &error_fatal); BUS(bus) ? > + > + /* Move to post_load */ > + node = g_malloc(sizeof(struct LBusNode)); > + node->ldev = LBUS_DEVICE(dev); > + QLIST_INSERT_HEAD(&bus->devices, node, next); This list looks useless to me. > + return dev; > +} > + > +static void lbus_device_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->bus_type = TYPE_LBUS; > + device_class_set_props(dc, lbus_device_props); > +} > + > +static const TypeInfo lbus_device_type_info = { > + .name = TYPE_LBUS_DEVICE, > + .parent = TYPE_DEVICE, > + .instance_size = sizeof(LBusDevice), > + .abstract = true, > + .class_init = lbus_device_class_init, > + .class_size = sizeof(LBusDeviceClass), > +}; > + > +static void lbus_register_types(void) > +{ > + type_register_static(&lbus_info); > + type_register_static(&lbus_device_type_info); > +} > + > +type_init(lbus_register_types); > diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build > new file mode 100644 > index 0000000000..e1007d5fea > --- /dev/null > +++ b/hw/fsi/meson.build > @@ -0,0 +1 @@ > +system_ss.add(when: 'CONFIG_LBUS', if_true: files('lbus.c')) > diff --git a/hw/meson.build b/hw/meson.build > index c7ac7d3d75..6c71ee9cfa 100644 > --- a/hw/meson.build > +++ b/hw/meson.build > @@ -43,6 +43,7 @@ subdir('virtio') > subdir('watchdog') > subdir('xen') > subdir('xenpv') > +subdir('fsi') > > subdir('alpha') > subdir('arm') > diff --git a/include/hw/fsi/lbus.h b/include/hw/fsi/lbus.h > new file mode 100644 > index 0000000000..fafc065178 > --- /dev/null > +++ b/include/hw/fsi/lbus.h > @@ -0,0 +1,48 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0-or-later > + * Copyright (C) 2023 IBM Corp. > + * > + * IBM Local bus and connected device structures. > + */ > +#ifndef FSI_LBUS_H > +#define FSI_LBUS_H > + > +#include "exec/memory.h" > +#include "hw/qdev-core.h" > + > +#define TYPE_LBUS_DEVICE "lbus.device" > +OBJECT_DECLARE_TYPE(LBusDevice, LBusDeviceClass, LBUS_DEVICE) > Using an "fsi/FSI" prefix for all definitions seems like a good idea to avoid namespace conflicts. > +typedef struct LBusDevice { > + DeviceState parent; > + > + MemoryRegion iomem; > + uint32_t address; > +} LBusDevice; > + > +typedef struct LBusDeviceClass { > + DeviceClass parent; > + > + uint32_t config; > +} LBusDeviceClass; > + > +typedef struct LBusNode { > + LBusDevice *ldev; > + > + QLIST_ENTRY(LBusNode) next; > +} LBusNode; > + > +#define TYPE_LBUS "lbus" > +OBJECT_DECLARE_SIMPLE_TYPE(LBus, LBUS) > + > +typedef struct LBus { > + BusState bus; > + > + MemoryRegion mr; > + > + QLIST_HEAD(, LBusNode) devices; BusState already has a list. I think this needs to be reworked. > +} LBus; > + > +DeviceState *lbus_create_device(LBus *bus, const char *type, uint32_t addr); > +int lbus_add_device(LBus *bus, LBusDevice *dev); > +#endif /* FSI_LBUS_H */ > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h > index cb3526d1f4..e12496f619 100644 > --- a/include/qemu/bitops.h > +++ b/include/qemu/bitops.h > @@ -618,4 +618,10 @@ static inline uint64_t half_unshuffle64(uint64_t x) > return x; > } > > +/* Bitwise operations at the word level. */ > +#define BE_BIT(x) BIT(31 - (x)) > +#define GENMASK(t, b) \ > + (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1)) > +#define BE_GENMASK(t, b) GENMASK(BE_BIT(t), BE_BIT(b)) I think we should rework the code to make these macro local to the fsi subsystem, in the fsi.h file, or even better get rid of them. Thanks, C. > #endif
Hello Cederic, On 9/9/23 03:25, Cédric Le Goater wrote: > On 9/9/23 00:28, Ninad Palsule wrote: >> This is a part of patchset where IBM's Flexible Service Interface is >> introduced. >> >> The LBUS is modelled to maintain the qdev bus hierarchy and to take >> advantage of the object model to automatically generate the CFAM >> configuration block. The configuration block presents engines in the >> order they are attached to the CFAM's LBUS. Engine implementations >> should subclass the LBusDevice and set the 'config' member of >> LBusDeviceClass to match the engine's type. >> >> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com> >> --- >> v2: >> - Incorporated Joel's review comments. >> --- > > > It is nice to have the header files upfront. > > orderFile = /path/to/qemu/scripts/git.orderfile ok, Done. Thanks. I wasn't aware of this. > > >> hw/Kconfig | 1 + >> hw/fsi/Kconfig | 2 + >> hw/fsi/lbus.c | 94 +++++++++++++++++++++++++++++++++++++++++++ >> hw/fsi/meson.build | 1 + >> hw/meson.build | 1 + >> include/hw/fsi/lbus.h | 48 ++++++++++++++++++++++ >> include/qemu/bitops.h | 6 +++ >> 7 files changed, 153 insertions(+) >> create mode 100644 hw/fsi/Kconfig >> create mode 100644 hw/fsi/lbus.c >> create mode 100644 hw/fsi/meson.build >> create mode 100644 include/hw/fsi/lbus.h >> >> diff --git a/hw/Kconfig b/hw/Kconfig >> index ba62ff6417..2ccb73add5 100644 >> --- a/hw/Kconfig >> +++ b/hw/Kconfig >> @@ -9,6 +9,7 @@ source core/Kconfig >> source cxl/Kconfig >> source display/Kconfig >> source dma/Kconfig >> +source fsi/Kconfig >> source gpio/Kconfig >> source hyperv/Kconfig >> source i2c/Kconfig >> diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig >> new file mode 100644 >> index 0000000000..687449e14e >> --- /dev/null >> +++ b/hw/fsi/Kconfig >> @@ -0,0 +1,2 @@ >> +config LBUS >> + bool >> diff --git a/hw/fsi/lbus.c b/hw/fsi/lbus.c >> new file mode 100644 >> index 0000000000..afb26ef7ea >> --- /dev/null >> +++ b/hw/fsi/lbus.c >> @@ -0,0 +1,94 @@ >> +/* >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + * Copyright (C) 2023 IBM Corp. >> + * >> + * IBM Local bus where FSI slaves are connected >> + */ >> + >> +#include "qemu/osdep.h" >> + >> +#include "qapi/error.h" >> +#include "qemu/log.h" >> + >> +#include "hw/fsi/lbus.h" >> + >> +#include "hw/qdev-properties.h" >> + >> +static void lbus_realize(BusState *bus, Error **errp) >> +{ >> + LBusNode *node; >> + LBus *lbus = LBUS(bus); >> + >> + memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_LBUS, >> + (2 * 1024 * 1024) - 0x400); > > please use some define Added defines. > >> + >> + QLIST_FOREACH(node, &lbus->devices, next) { >> + memory_region_add_subregion(&lbus->mr, node->ldev->address, >> + &node->ldev->iomem); >> + } >> +} >> + >> +static void lbus_init(Object *o) >> +{ >> +} >> + > > please remove if unused Removed. > > >> +static void lbus_class_init(ObjectClass *klass, void *data) >> +{ >> + BusClass *k = BUS_CLASS(klass); >> + k->realize = lbus_realize; >> +} >> + >> +static const TypeInfo lbus_info = { >> + .name = TYPE_LBUS, >> + .parent = TYPE_BUS, >> + .instance_init = lbus_init, >> + .instance_size = sizeof(LBus), >> + .class_init = lbus_class_init, >> +}; >> + >> +static Property lbus_device_props[] = { >> + DEFINE_PROP_UINT32("address", LBusDevice, address, 0), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> +DeviceState *lbus_create_device(LBus *bus, const char *type, >> uint32_t addr) >> +{ >> + DeviceState *dev; >> + LBusNode *node; >> + >> + dev = qdev_new(type); >> + qdev_prop_set_uint8(dev, "address", addr); >> + qdev_realize_and_unref(dev, &bus->bus, &error_fatal); > > BUS(bus) ? Fixed > >> + >> + /* Move to post_load */ >> + node = g_malloc(sizeof(struct LBusNode)); >> + node->ldev = LBUS_DEVICE(dev); >> + QLIST_INSERT_HEAD(&bus->devices, node, next); > > This list looks useless to me. This list is used to add new devices. Keeping it for now. > >> + return dev; >> +} >> + >> +static void lbus_device_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->bus_type = TYPE_LBUS; >> + device_class_set_props(dc, lbus_device_props); >> +} >> + >> +static const TypeInfo lbus_device_type_info = { >> + .name = TYPE_LBUS_DEVICE, >> + .parent = TYPE_DEVICE, >> + .instance_size = sizeof(LBusDevice), >> + .abstract = true, >> + .class_init = lbus_device_class_init, >> + .class_size = sizeof(LBusDeviceClass), >> +}; >> + >> +static void lbus_register_types(void) >> +{ >> + type_register_static(&lbus_info); >> + type_register_static(&lbus_device_type_info); >> +} >> + >> +type_init(lbus_register_types); >> diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build >> new file mode 100644 >> index 0000000000..e1007d5fea >> --- /dev/null >> +++ b/hw/fsi/meson.build >> @@ -0,0 +1 @@ >> +system_ss.add(when: 'CONFIG_LBUS', if_true: files('lbus.c')) >> diff --git a/hw/meson.build b/hw/meson.build >> index c7ac7d3d75..6c71ee9cfa 100644 >> --- a/hw/meson.build >> +++ b/hw/meson.build >> @@ -43,6 +43,7 @@ subdir('virtio') >> subdir('watchdog') >> subdir('xen') >> subdir('xenpv') >> +subdir('fsi') >> subdir('alpha') >> subdir('arm') >> diff --git a/include/hw/fsi/lbus.h b/include/hw/fsi/lbus.h >> new file mode 100644 >> index 0000000000..fafc065178 >> --- /dev/null >> +++ b/include/hw/fsi/lbus.h >> @@ -0,0 +1,48 @@ >> +/* >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + * Copyright (C) 2023 IBM Corp. >> + * >> + * IBM Local bus and connected device structures. >> + */ >> +#ifndef FSI_LBUS_H >> +#define FSI_LBUS_H >> + >> +#include "exec/memory.h" >> +#include "hw/qdev-core.h" >> + >> +#define TYPE_LBUS_DEVICE "lbus.device" >> +OBJECT_DECLARE_TYPE(LBusDevice, LBusDeviceClass, LBUS_DEVICE) >> > > Using an "fsi/FSI" prefix for all definitions seems like a good idea > to avoid namespace conflicts. Added FSI prefix. > >> +typedef struct LBusDevice { >> + DeviceState parent; >> + >> + MemoryRegion iomem; >> + uint32_t address; >> +} LBusDevice; >> + >> +typedef struct LBusDeviceClass { >> + DeviceClass parent; >> + >> + uint32_t config; >> +} LBusDeviceClass; >> + >> +typedef struct LBusNode { >> + LBusDevice *ldev; >> + >> + QLIST_ENTRY(LBusNode) next; >> +} LBusNode; >> + >> +#define TYPE_LBUS "lbus" >> +OBJECT_DECLARE_SIMPLE_TYPE(LBus, LBUS) >> + >> +typedef struct LBus { >> + BusState bus; >> + >> + MemoryRegion mr; >> + >> + QLIST_HEAD(, LBusNode) devices; > > BusState already has a list. I think this needs to be reworked. The list in BusState is for the buses. This list is for devices. > > >> +} LBus; >> + >> +DeviceState *lbus_create_device(LBus *bus, const char *type, >> uint32_t addr); >> +int lbus_add_device(LBus *bus, LBusDevice *dev); >> +#endif /* FSI_LBUS_H */ >> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h >> index cb3526d1f4..e12496f619 100644 >> --- a/include/qemu/bitops.h >> +++ b/include/qemu/bitops.h >> @@ -618,4 +618,10 @@ static inline uint64_t half_unshuffle64(uint64_t x) >> return x; >> } >> +/* Bitwise operations at the word level. */ >> +#define BE_BIT(x) BIT(31 - (x)) >> +#define GENMASK(t, b) \ >> + (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1)) >> +#define BE_GENMASK(t, b) GENMASK(BE_BIT(t), >> BE_BIT(b)) > > I think we should rework the code to make these macro local to the fsi > subsystem, in the fsi.h file, or even better get rid of them. OK, Removed it from bitops.h Thanks for the review. ~ Ninad > > Thanks, > > C. > > >> #endif >
diff --git a/hw/Kconfig b/hw/Kconfig index ba62ff6417..2ccb73add5 100644 --- a/hw/Kconfig +++ b/hw/Kconfig @@ -9,6 +9,7 @@ source core/Kconfig source cxl/Kconfig source display/Kconfig source dma/Kconfig +source fsi/Kconfig source gpio/Kconfig source hyperv/Kconfig source i2c/Kconfig diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig new file mode 100644 index 0000000000..687449e14e --- /dev/null +++ b/hw/fsi/Kconfig @@ -0,0 +1,2 @@ +config LBUS + bool diff --git a/hw/fsi/lbus.c b/hw/fsi/lbus.c new file mode 100644 index 0000000000..afb26ef7ea --- /dev/null +++ b/hw/fsi/lbus.c @@ -0,0 +1,94 @@ +/* + * SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (C) 2023 IBM Corp. + * + * IBM Local bus where FSI slaves are connected + */ + +#include "qemu/osdep.h" + +#include "qapi/error.h" +#include "qemu/log.h" + +#include "hw/fsi/lbus.h" + +#include "hw/qdev-properties.h" + +static void lbus_realize(BusState *bus, Error **errp) +{ + LBusNode *node; + LBus *lbus = LBUS(bus); + + memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_LBUS, + (2 * 1024 * 1024) - 0x400); + + QLIST_FOREACH(node, &lbus->devices, next) { + memory_region_add_subregion(&lbus->mr, node->ldev->address, + &node->ldev->iomem); + } +} + +static void lbus_init(Object *o) +{ +} + +static void lbus_class_init(ObjectClass *klass, void *data) +{ + BusClass *k = BUS_CLASS(klass); + k->realize = lbus_realize; +} + +static const TypeInfo lbus_info = { + .name = TYPE_LBUS, + .parent = TYPE_BUS, + .instance_init = lbus_init, + .instance_size = sizeof(LBus), + .class_init = lbus_class_init, +}; + +static Property lbus_device_props[] = { + DEFINE_PROP_UINT32("address", LBusDevice, address, 0), + DEFINE_PROP_END_OF_LIST(), +}; + +DeviceState *lbus_create_device(LBus *bus, const char *type, uint32_t addr) +{ + DeviceState *dev; + LBusNode *node; + + dev = qdev_new(type); + qdev_prop_set_uint8(dev, "address", addr); + qdev_realize_and_unref(dev, &bus->bus, &error_fatal); + + /* Move to post_load */ + node = g_malloc(sizeof(struct LBusNode)); + node->ldev = LBUS_DEVICE(dev); + QLIST_INSERT_HEAD(&bus->devices, node, next); + + return dev; +} + +static void lbus_device_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->bus_type = TYPE_LBUS; + device_class_set_props(dc, lbus_device_props); +} + +static const TypeInfo lbus_device_type_info = { + .name = TYPE_LBUS_DEVICE, + .parent = TYPE_DEVICE, + .instance_size = sizeof(LBusDevice), + .abstract = true, + .class_init = lbus_device_class_init, + .class_size = sizeof(LBusDeviceClass), +}; + +static void lbus_register_types(void) +{ + type_register_static(&lbus_info); + type_register_static(&lbus_device_type_info); +} + +type_init(lbus_register_types); diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build new file mode 100644 index 0000000000..e1007d5fea --- /dev/null +++ b/hw/fsi/meson.build @@ -0,0 +1 @@ +system_ss.add(when: 'CONFIG_LBUS', if_true: files('lbus.c')) diff --git a/hw/meson.build b/hw/meson.build index c7ac7d3d75..6c71ee9cfa 100644 --- a/hw/meson.build +++ b/hw/meson.build @@ -43,6 +43,7 @@ subdir('virtio') subdir('watchdog') subdir('xen') subdir('xenpv') +subdir('fsi') subdir('alpha') subdir('arm') diff --git a/include/hw/fsi/lbus.h b/include/hw/fsi/lbus.h new file mode 100644 index 0000000000..fafc065178 --- /dev/null +++ b/include/hw/fsi/lbus.h @@ -0,0 +1,48 @@ +/* + * SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (C) 2023 IBM Corp. + * + * IBM Local bus and connected device structures. + */ +#ifndef FSI_LBUS_H +#define FSI_LBUS_H + +#include "exec/memory.h" +#include "hw/qdev-core.h" + +#define TYPE_LBUS_DEVICE "lbus.device" +OBJECT_DECLARE_TYPE(LBusDevice, LBusDeviceClass, LBUS_DEVICE) + +typedef struct LBusDevice { + DeviceState parent; + + MemoryRegion iomem; + uint32_t address; +} LBusDevice; + +typedef struct LBusDeviceClass { + DeviceClass parent; + + uint32_t config; +} LBusDeviceClass; + +typedef struct LBusNode { + LBusDevice *ldev; + + QLIST_ENTRY(LBusNode) next; +} LBusNode; + +#define TYPE_LBUS "lbus" +OBJECT_DECLARE_SIMPLE_TYPE(LBus, LBUS) + +typedef struct LBus { + BusState bus; + + MemoryRegion mr; + + QLIST_HEAD(, LBusNode) devices; +} LBus; + +DeviceState *lbus_create_device(LBus *bus, const char *type, uint32_t addr); +int lbus_add_device(LBus *bus, LBusDevice *dev); +#endif /* FSI_LBUS_H */ diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h index cb3526d1f4..e12496f619 100644 --- a/include/qemu/bitops.h +++ b/include/qemu/bitops.h @@ -618,4 +618,10 @@ static inline uint64_t half_unshuffle64(uint64_t x) return x; } +/* Bitwise operations at the word level. */ +#define BE_BIT(x) BIT(31 - (x)) +#define GENMASK(t, b) \ + (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1)) +#define BE_GENMASK(t, b) GENMASK(BE_BIT(t), BE_BIT(b)) + #endif