| Message ID | 20210923091308.13832-5-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | nubus: bus, device, bridge, IRQ and address space improvements | expand |
On 9/23/21 11:12, Mark Cave-Ayland wrote: > Convert nubus_device_realize() to use a bitmap to manage available slots to allow > for future Nubus devices to be plugged into arbitrary slots from the command line > using a new qdev "slot" parameter for nubus devices. > > Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh > machines as documented in "Desigining Cards and Drivers for the Macintosh Family". > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/nubus/mac-nubus-bridge.c | 4 ++++ > hw/nubus/nubus-bus.c | 5 +++-- > hw/nubus/nubus-device.c | 32 +++++++++++++++++++++++------ > include/hw/nubus/mac-nubus-bridge.h | 4 ++++ > include/hw/nubus/nubus.h | 13 ++++++------ > 5 files changed, 43 insertions(+), 15 deletions(-) > diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c > index 4e23df1280..562650a05b 100644 > --- a/hw/nubus/nubus-device.c > +++ b/hw/nubus/nubus-device.c > @@ -160,14 +160,28 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) > NubusDevice *nd = NUBUS_DEVICE(dev); > char *name; > hwaddr slot_offset; > - > - if (nubus->current_slot < NUBUS_FIRST_SLOT || > - nubus->current_slot > NUBUS_LAST_SLOT) { > - error_setg(errp, "Cannot register nubus card, not enough slots"); > - return; > + uint16_t s; > + > + if (nd->slot == -1) { > + /* No slot specified, find first available free slot */ > + s = ctz32(nubus->slot_available_mask); Nitpicking: int s = ctz32(nubus->slot_available_mask); Otherwise, Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > + if (s != 32) { > + nd->slot = s; > + } else { > + error_setg(errp, "Cannot register nubus card, no free slot " > + "available"); > + return; > + } > + } else { > + /* Slot specified, make sure the slot is available */ > + if (!(nubus->slot_available_mask & BIT(nd->slot))) { > + error_setg(errp, "Cannot register nubus card, slot %d is " > + "unavailable or already occupied", nd->slot); > + return; > + } > }
Le 23/09/2021 à 11:12, Mark Cave-Ayland a écrit : > Convert nubus_device_realize() to use a bitmap to manage available slots to allow > for future Nubus devices to be plugged into arbitrary slots from the command line > using a new qdev "slot" parameter for nubus devices. > > Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh > machines as documented in "Desigining Cards and Drivers for the Macintosh Family". > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/nubus/mac-nubus-bridge.c | 4 ++++ > hw/nubus/nubus-bus.c | 5 +++-- > hw/nubus/nubus-device.c | 32 +++++++++++++++++++++++------ > include/hw/nubus/mac-nubus-bridge.h | 4 ++++ > include/hw/nubus/nubus.h | 13 ++++++------ > 5 files changed, 43 insertions(+), 15 deletions(-) > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
On Thu, 23 Sep 2021, Mark Cave-Ayland wrote: > Convert nubus_device_realize() to use a bitmap to manage available slots to allow > for future Nubus devices to be plugged into arbitrary slots from the command line > using a new qdev "slot" parameter for nubus devices. > > Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh > machines as documented in "Desigining Cards and Drivers for the Macintosh Family". Small typo: "a Macintosh machnies", either a or s is not needed. > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/nubus/mac-nubus-bridge.c | 4 ++++ > hw/nubus/nubus-bus.c | 5 +++-- > hw/nubus/nubus-device.c | 32 +++++++++++++++++++++++------ > include/hw/nubus/mac-nubus-bridge.h | 4 ++++ > include/hw/nubus/nubus.h | 13 ++++++------ > 5 files changed, 43 insertions(+), 15 deletions(-) > > diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c > index 7c329300b8..3f075789e9 100644 > --- a/hw/nubus/mac-nubus-bridge.c > +++ b/hw/nubus/mac-nubus-bridge.c > @@ -18,6 +18,10 @@ static void mac_nubus_bridge_init(Object *obj) > > s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL)); > > + /* Macintosh only has slots 0x9 to 0xe available */ > + s->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT, > + MAC_NUBUS_SLOT_NB); > + > sysbus_init_mmio(sbd, &s->bus->super_slot_io); > sysbus_init_mmio(sbd, &s->bus->slot_io); > } > diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c > index f4410803ff..3cd7534864 100644 > --- a/hw/nubus/nubus-bus.c > +++ b/hw/nubus/nubus-bus.c > @@ -86,13 +86,14 @@ static void nubus_init(Object *obj) > > memory_region_init_io(&nubus->super_slot_io, obj, &nubus_super_slot_ops, > nubus, "nubus-super-slots", > - NUBUS_SUPER_SLOT_NB * NUBUS_SUPER_SLOT_SIZE); > + (NUBUS_SUPER_SLOT_NB + 1) * NUBUS_SUPER_SLOT_SIZE); > > memory_region_init_io(&nubus->slot_io, obj, &nubus_slot_ops, > nubus, "nubus-slots", > NUBUS_SLOT_NB * NUBUS_SLOT_SIZE); > > - nubus->current_slot = NUBUS_FIRST_SLOT; > + nubus->slot_available_mask = MAKE_64BIT_MASK(NUBUS_FIRST_SLOT, > + NUBUS_SLOT_NB); > } > > static void nubus_class_init(ObjectClass *oc, void *data) > diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c > index 4e23df1280..562650a05b 100644 > --- a/hw/nubus/nubus-device.c > +++ b/hw/nubus/nubus-device.c > @@ -160,14 +160,28 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) > NubusDevice *nd = NUBUS_DEVICE(dev); > char *name; > hwaddr slot_offset; > - > - if (nubus->current_slot < NUBUS_FIRST_SLOT || > - nubus->current_slot > NUBUS_LAST_SLOT) { > - error_setg(errp, "Cannot register nubus card, not enough slots"); > - return; > + uint16_t s; > + > + if (nd->slot == -1) { > + /* No slot specified, find first available free slot */ > + s = ctz32(nubus->slot_available_mask); > + if (s != 32) { > + nd->slot = s; > + } else { > + error_setg(errp, "Cannot register nubus card, no free slot " > + "available"); > + return; > + } > + } else { > + /* Slot specified, make sure the slot is available */ > + if (!(nubus->slot_available_mask & BIT(nd->slot))) { > + error_setg(errp, "Cannot register nubus card, slot %d is " > + "unavailable or already occupied", nd->slot); > + return; > + } > } > > - nd->slot = nubus->current_slot++; > + nubus->slot_available_mask &= ~BIT(nd->slot); > > /* Super */ > slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE; > @@ -191,12 +205,18 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) > nubus_register_format_block(nd); > } > > +static Property nubus_device_properties[] = { > + DEFINE_PROP_INT32("slot", NubusDevice, slot, -1), > + DEFINE_PROP_END_OF_LIST() > +}; > + > static void nubus_device_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > > dc->realize = nubus_device_realize; > dc->bus_type = TYPE_NUBUS_BUS; > + device_class_set_props(dc, nubus_device_properties); > } > > static const TypeInfo nubus_device_type_info = { > diff --git a/include/hw/nubus/mac-nubus-bridge.h b/include/hw/nubus/mac-nubus-bridge.h > index 36aa098dd4..118d67267d 100644 > --- a/include/hw/nubus/mac-nubus-bridge.h > +++ b/include/hw/nubus/mac-nubus-bridge.h > @@ -12,6 +12,10 @@ > #include "hw/nubus/nubus.h" > #include "qom/object.h" > > +#define MAC_NUBUS_FIRST_SLOT 0x9 > +#define MAC_NUBUS_LAST_SLOT 0xe > +#define MAC_NUBUS_SLOT_NB (MAC_NUBUS_LAST_SLOT - MAC_NUBUS_FIRST_SLOT + 1) > + > #define TYPE_MAC_NUBUS_BRIDGE "mac-nubus-bridge" > OBJECT_DECLARE_SIMPLE_TYPE(MacNubusState, MAC_NUBUS_BRIDGE) > > diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h > index 89b0976aaa..988e4a2361 100644 > --- a/include/hw/nubus/nubus.h > +++ b/include/hw/nubus/nubus.h > @@ -14,13 +14,12 @@ > #include "qom/object.h" > > #define NUBUS_SUPER_SLOT_SIZE 0x10000000U > -#define NUBUS_SUPER_SLOT_NB 0x9 > +#define NUBUS_SUPER_SLOT_NB 0xe > > #define NUBUS_SLOT_SIZE 0x01000000 > -#define NUBUS_SLOT_NB 0xF > - > -#define NUBUS_FIRST_SLOT 0x9 > -#define NUBUS_LAST_SLOT 0xF > +#define NUBUS_FIRST_SLOT 0x0 > +#define NUBUS_LAST_SLOT 0xf > +#define NUBUS_SLOT_NB (NUBUS_LAST_SLOT - NUBUS_FIRST_SLOT + 1) > > #define TYPE_NUBUS_DEVICE "nubus-device" > OBJECT_DECLARE_SIMPLE_TYPE(NubusDevice, NUBUS_DEVICE) > @@ -36,13 +35,13 @@ struct NubusBus { > MemoryRegion super_slot_io; > MemoryRegion slot_io; > > - int current_slot; > + uint32_t slot_available_mask; > }; > > struct NubusDevice { > DeviceState qdev; > > - int slot; > + int32_t slot; Why uint32_t? Considering its max value even uint8_t would be enough although maybe invalid value would be 255 instead of -1 then. As this was added in previous patch you could avoid churn here by introducing it with the right type in that patch already. (But feel free to ignore it if you have no time for more changes, the current version works so if you don't do another version for other reasons this probably don't worth the effort alone.) Regards, BALATON Zoltan
On 23/09/2021 10:42, Philippe Mathieu-Daudé wrote: > On 9/23/21 11:12, Mark Cave-Ayland wrote: >> Convert nubus_device_realize() to use a bitmap to manage available slots to allow >> for future Nubus devices to be plugged into arbitrary slots from the command line >> using a new qdev "slot" parameter for nubus devices. >> >> Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh >> machines as documented in "Desigining Cards and Drivers for the Macintosh Family". >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/nubus/mac-nubus-bridge.c | 4 ++++ >> hw/nubus/nubus-bus.c | 5 +++-- >> hw/nubus/nubus-device.c | 32 +++++++++++++++++++++++------ >> include/hw/nubus/mac-nubus-bridge.h | 4 ++++ >> include/hw/nubus/nubus.h | 13 ++++++------ >> 5 files changed, 43 insertions(+), 15 deletions(-) > >> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c >> index 4e23df1280..562650a05b 100644 >> --- a/hw/nubus/nubus-device.c >> +++ b/hw/nubus/nubus-device.c >> @@ -160,14 +160,28 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) >> NubusDevice *nd = NUBUS_DEVICE(dev); >> char *name; >> hwaddr slot_offset; >> - >> - if (nubus->current_slot < NUBUS_FIRST_SLOT || >> - nubus->current_slot > NUBUS_LAST_SLOT) { >> - error_setg(errp, "Cannot register nubus card, not enough slots"); >> - return; >> + uint16_t s; >> + >> + if (nd->slot == -1) { >> + /* No slot specified, find first available free slot */ >> + s = ctz32(nubus->slot_available_mask); > > Nitpicking: > > int s = ctz32(nubus->slot_available_mask); > > Otherwise, > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> I'll make sure this is included in v6. >> + if (s != 32) { >> + nd->slot = s; >> + } else { >> + error_setg(errp, "Cannot register nubus card, no free slot " >> + "available"); >> + return; >> + } >> + } else { >> + /* Slot specified, make sure the slot is available */ >> + if (!(nubus->slot_available_mask & BIT(nd->slot))) { >> + error_setg(errp, "Cannot register nubus card, slot %d is " >> + "unavailable or already occupied", nd->slot); >> + return; >> + } >> } ATB, Mark.
On 23/09/2021 15:16, BALATON Zoltan wrote: > On Thu, 23 Sep 2021, Mark Cave-Ayland wrote: >> Convert nubus_device_realize() to use a bitmap to manage available slots to allow >> for future Nubus devices to be plugged into arbitrary slots from the command line >> using a new qdev "slot" parameter for nubus devices. >> >> Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh >> machines as documented in "Desigining Cards and Drivers for the Macintosh Family". > > Small typo: "a Macintosh machnies", either a or s is not needed. Thanks - I've updated this for v6. >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/nubus/mac-nubus-bridge.c | 4 ++++ >> hw/nubus/nubus-bus.c | 5 +++-- >> hw/nubus/nubus-device.c | 32 +++++++++++++++++++++++------ >> include/hw/nubus/mac-nubus-bridge.h | 4 ++++ >> include/hw/nubus/nubus.h | 13 ++++++------ >> 5 files changed, 43 insertions(+), 15 deletions(-) >> >> diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c >> index 7c329300b8..3f075789e9 100644 >> --- a/hw/nubus/mac-nubus-bridge.c >> +++ b/hw/nubus/mac-nubus-bridge.c >> @@ -18,6 +18,10 @@ static void mac_nubus_bridge_init(Object *obj) >> >> s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL)); >> >> + /* Macintosh only has slots 0x9 to 0xe available */ >> + s->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT, >> + MAC_NUBUS_SLOT_NB); >> + >> sysbus_init_mmio(sbd, &s->bus->super_slot_io); >> sysbus_init_mmio(sbd, &s->bus->slot_io); >> } >> diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c >> index f4410803ff..3cd7534864 100644 >> --- a/hw/nubus/nubus-bus.c >> +++ b/hw/nubus/nubus-bus.c >> @@ -86,13 +86,14 @@ static void nubus_init(Object *obj) >> >> memory_region_init_io(&nubus->super_slot_io, obj, &nubus_super_slot_ops, >> nubus, "nubus-super-slots", >> - NUBUS_SUPER_SLOT_NB * NUBUS_SUPER_SLOT_SIZE); >> + (NUBUS_SUPER_SLOT_NB + 1) * NUBUS_SUPER_SLOT_SIZE); >> >> memory_region_init_io(&nubus->slot_io, obj, &nubus_slot_ops, >> nubus, "nubus-slots", >> NUBUS_SLOT_NB * NUBUS_SLOT_SIZE); >> >> - nubus->current_slot = NUBUS_FIRST_SLOT; >> + nubus->slot_available_mask = MAKE_64BIT_MASK(NUBUS_FIRST_SLOT, >> + NUBUS_SLOT_NB); >> } >> >> static void nubus_class_init(ObjectClass *oc, void *data) >> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c >> index 4e23df1280..562650a05b 100644 >> --- a/hw/nubus/nubus-device.c >> +++ b/hw/nubus/nubus-device.c >> @@ -160,14 +160,28 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) >> NubusDevice *nd = NUBUS_DEVICE(dev); >> char *name; >> hwaddr slot_offset; >> - >> - if (nubus->current_slot < NUBUS_FIRST_SLOT || >> - nubus->current_slot > NUBUS_LAST_SLOT) { >> - error_setg(errp, "Cannot register nubus card, not enough slots"); >> - return; >> + uint16_t s; >> + >> + if (nd->slot == -1) { >> + /* No slot specified, find first available free slot */ >> + s = ctz32(nubus->slot_available_mask); >> + if (s != 32) { >> + nd->slot = s; >> + } else { >> + error_setg(errp, "Cannot register nubus card, no free slot " >> + "available"); >> + return; >> + } >> + } else { >> + /* Slot specified, make sure the slot is available */ >> + if (!(nubus->slot_available_mask & BIT(nd->slot))) { >> + error_setg(errp, "Cannot register nubus card, slot %d is " >> + "unavailable or already occupied", nd->slot); >> + return; >> + } >> } >> >> - nd->slot = nubus->current_slot++; >> + nubus->slot_available_mask &= ~BIT(nd->slot); >> >> /* Super */ >> slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE; >> @@ -191,12 +205,18 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) >> nubus_register_format_block(nd); >> } >> >> +static Property nubus_device_properties[] = { >> + DEFINE_PROP_INT32("slot", NubusDevice, slot, -1), >> + DEFINE_PROP_END_OF_LIST() >> +}; >> + >> static void nubus_device_class_init(ObjectClass *oc, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(oc); >> >> dc->realize = nubus_device_realize; >> dc->bus_type = TYPE_NUBUS_BUS; >> + device_class_set_props(dc, nubus_device_properties); >> } >> >> static const TypeInfo nubus_device_type_info = { >> diff --git a/include/hw/nubus/mac-nubus-bridge.h b/include/hw/nubus/mac-nubus-bridge.h >> index 36aa098dd4..118d67267d 100644 >> --- a/include/hw/nubus/mac-nubus-bridge.h >> +++ b/include/hw/nubus/mac-nubus-bridge.h >> @@ -12,6 +12,10 @@ >> #include "hw/nubus/nubus.h" >> #include "qom/object.h" >> >> +#define MAC_NUBUS_FIRST_SLOT 0x9 >> +#define MAC_NUBUS_LAST_SLOT 0xe >> +#define MAC_NUBUS_SLOT_NB (MAC_NUBUS_LAST_SLOT - MAC_NUBUS_FIRST_SLOT + 1) >> + >> #define TYPE_MAC_NUBUS_BRIDGE "mac-nubus-bridge" >> OBJECT_DECLARE_SIMPLE_TYPE(MacNubusState, MAC_NUBUS_BRIDGE) >> >> diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h >> index 89b0976aaa..988e4a2361 100644 >> --- a/include/hw/nubus/nubus.h >> +++ b/include/hw/nubus/nubus.h >> @@ -14,13 +14,12 @@ >> #include "qom/object.h" >> >> #define NUBUS_SUPER_SLOT_SIZE 0x10000000U >> -#define NUBUS_SUPER_SLOT_NB 0x9 >> +#define NUBUS_SUPER_SLOT_NB 0xe >> >> #define NUBUS_SLOT_SIZE 0x01000000 >> -#define NUBUS_SLOT_NB 0xF >> - >> -#define NUBUS_FIRST_SLOT 0x9 >> -#define NUBUS_LAST_SLOT 0xF >> +#define NUBUS_FIRST_SLOT 0x0 >> +#define NUBUS_LAST_SLOT 0xf >> +#define NUBUS_SLOT_NB (NUBUS_LAST_SLOT - NUBUS_FIRST_SLOT + 1) >> >> #define TYPE_NUBUS_DEVICE "nubus-device" >> OBJECT_DECLARE_SIMPLE_TYPE(NubusDevice, NUBUS_DEVICE) >> @@ -36,13 +35,13 @@ struct NubusBus { >> MemoryRegion super_slot_io; >> MemoryRegion slot_io; >> >> - int current_slot; >> + uint32_t slot_available_mask; >> }; >> >> struct NubusDevice { >> DeviceState qdev; >> >> - int slot; >> + int32_t slot; > > Why uint32_t? Considering its max value even uint8_t would be enough although maybe > invalid value would be 255 instead of -1 then. As this was added in previous patch > you could avoid churn here by introducing it with the right type in that patch > already. (But feel free to ignore it if you have no time for more changes, the > current version works so if you don't do another version for other reasons this > probably don't worth the effort alone.) I think it makes sense to keep this signed since -1 is used for other bus implementations to indicate that an explicit slot hasn't been assigned. Potentially the slot number could be represented by an 8-bit value, however it seems there is no DEFINE_PROP_INT8 or DEFINE_PROP_INT16. Fortunately the slot number is restricted by the available slots bitmask anyhow, so this shouldn't be an issue. ATB, Mark.
On 9/24/21 09:16, Mark Cave-Ayland wrote: > On 23/09/2021 15:16, BALATON Zoltan wrote: > >> On Thu, 23 Sep 2021, Mark Cave-Ayland wrote: >>> Convert nubus_device_realize() to use a bitmap to manage available >>> slots to allow >>> for future Nubus devices to be plugged into arbitrary slots from the >>> command line >>> using a new qdev "slot" parameter for nubus devices. >>> >>> Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a >>> Macintosh >>> machines as documented in "Desigining Cards and Drivers for the >>> Macintosh Family". >> >> Small typo: "a Macintosh machnies", either a or s is not needed. > > Thanks - I've updated this for v6. > >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> --- >>> hw/nubus/mac-nubus-bridge.c | 4 ++++ >>> hw/nubus/nubus-bus.c | 5 +++-- >>> hw/nubus/nubus-device.c | 32 +++++++++++++++++++++++------ >>> include/hw/nubus/mac-nubus-bridge.h | 4 ++++ >>> include/hw/nubus/nubus.h | 13 ++++++------ >>> 5 files changed, 43 insertions(+), 15 deletions(-) >>> struct NubusDevice { >>> DeviceState qdev; >>> >>> - int slot; >>> + int32_t slot; >> >> Why uint32_t? Considering its max value even uint8_t would be enough >> although maybe invalid value would be 255 instead of -1 then. As this >> was added in previous patch you could avoid churn here by introducing >> it with the right type in that patch already. (But feel free to ignore >> it if you have no time for more changes, the current version works so >> if you don't do another version for other reasons this probably don't >> worth the effort alone.) > > I think it makes sense to keep this signed since -1 is used for other > bus implementations to indicate that an explicit slot hasn't been > assigned. Potentially the slot number could be represented by an 8-bit > value, however it seems there is no DEFINE_PROP_INT8 or > DEFINE_PROP_INT16. Fortunately the slot number is restricted by the > available slots bitmask anyhow, so this shouldn't be an issue. I wondered the same and noticed there is no DEFINE_PROP_INT8, so didn't want to bother Mark furthermore :) Adding & using DEFINE_PROP_INT8 seems a good idea, but to be fair with the repository we'd need to audit the other places where DEFINE_PROP_INT32 isn't justified and update. Extra work for not much gain, so I'm find with this patch. Can be improved on top.
On Fri, 24 Sep 2021, Philippe Mathieu-Daudé wrote: > On 9/24/21 09:16, Mark Cave-Ayland wrote: >> On 23/09/2021 15:16, BALATON Zoltan wrote: >> >>> On Thu, 23 Sep 2021, Mark Cave-Ayland wrote: >>>> Convert nubus_device_realize() to use a bitmap to manage available slots >>>> to allow >>>> for future Nubus devices to be plugged into arbitrary slots from the >>>> command line >>>> using a new qdev "slot" parameter for nubus devices. >>>> >>>> Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a >>>> Macintosh >>>> machines as documented in "Desigining Cards and Drivers for the Macintosh >>>> Family". >>> >>> Small typo: "a Macintosh machnies", either a or s is not needed. >> >> Thanks - I've updated this for v6. >> >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>> --- >>>> hw/nubus/mac-nubus-bridge.c | 4 ++++ >>>> hw/nubus/nubus-bus.c | 5 +++-- >>>> hw/nubus/nubus-device.c | 32 +++++++++++++++++++++++------ >>>> include/hw/nubus/mac-nubus-bridge.h | 4 ++++ >>>> include/hw/nubus/nubus.h | 13 ++++++------ >>>> 5 files changed, 43 insertions(+), 15 deletions(-) > >>>> struct NubusDevice { >>>> DeviceState qdev; >>>> >>>> - int slot; >>>> + int32_t slot; >>> >>> Why uint32_t? Considering its max value even uint8_t would be enough >>> although maybe invalid value would be 255 instead of -1 then. As this was >>> added in previous patch you could avoid churn here by introducing it with >>> the right type in that patch already. (But feel free to ignore it if you >>> have no time for more changes, the current version works so if you don't >>> do another version for other reasons this probably don't worth the effort >>> alone.) >> >> I think it makes sense to keep this signed since -1 is used for other bus >> implementations to indicate that an explicit slot hasn't been assigned. >> Potentially the slot number could be represented by an 8-bit value, however >> it seems there is no DEFINE_PROP_INT8 or DEFINE_PROP_INT16. Fortunately the >> slot number is restricted by the available slots bitmask anyhow, so this >> shouldn't be an issue. > > I wondered the same and noticed there is no DEFINE_PROP_INT8, so didn't > want to bother Mark furthermore :) Adding & using DEFINE_PROP_INT8 seems > a good idea, but to be fair with the repository we'd need to audit the > other places where DEFINE_PROP_INT32 isn't justified and update. Extra > work for not much gain, so I'm find with this patch. Can be improved on > top. That's why I said UINT8 for prop and treat -1 as 0xff but I agree this is not a big deal so I've also said I'm OK with the current version. If it would be more effort than Mark is willing to put in this now I can understand that and not pushing it. It's not something that's wrong or worth holding the series back for just a possible minor improvement to avoid some code churn. Regards, BALATON Zoltan
diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c index 7c329300b8..3f075789e9 100644 --- a/hw/nubus/mac-nubus-bridge.c +++ b/hw/nubus/mac-nubus-bridge.c @@ -18,6 +18,10 @@ static void mac_nubus_bridge_init(Object *obj) s->bus = NUBUS_BUS(qbus_create(TYPE_NUBUS_BUS, DEVICE(s), NULL)); + /* Macintosh only has slots 0x9 to 0xe available */ + s->bus->slot_available_mask = MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT, + MAC_NUBUS_SLOT_NB); + sysbus_init_mmio(sbd, &s->bus->super_slot_io); sysbus_init_mmio(sbd, &s->bus->slot_io); } diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c index f4410803ff..3cd7534864 100644 --- a/hw/nubus/nubus-bus.c +++ b/hw/nubus/nubus-bus.c @@ -86,13 +86,14 @@ static void nubus_init(Object *obj) memory_region_init_io(&nubus->super_slot_io, obj, &nubus_super_slot_ops, nubus, "nubus-super-slots", - NUBUS_SUPER_SLOT_NB * NUBUS_SUPER_SLOT_SIZE); + (NUBUS_SUPER_SLOT_NB + 1) * NUBUS_SUPER_SLOT_SIZE); memory_region_init_io(&nubus->slot_io, obj, &nubus_slot_ops, nubus, "nubus-slots", NUBUS_SLOT_NB * NUBUS_SLOT_SIZE); - nubus->current_slot = NUBUS_FIRST_SLOT; + nubus->slot_available_mask = MAKE_64BIT_MASK(NUBUS_FIRST_SLOT, + NUBUS_SLOT_NB); } static void nubus_class_init(ObjectClass *oc, void *data) diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c index 4e23df1280..562650a05b 100644 --- a/hw/nubus/nubus-device.c +++ b/hw/nubus/nubus-device.c @@ -160,14 +160,28 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) NubusDevice *nd = NUBUS_DEVICE(dev); char *name; hwaddr slot_offset; - - if (nubus->current_slot < NUBUS_FIRST_SLOT || - nubus->current_slot > NUBUS_LAST_SLOT) { - error_setg(errp, "Cannot register nubus card, not enough slots"); - return; + uint16_t s; + + if (nd->slot == -1) { + /* No slot specified, find first available free slot */ + s = ctz32(nubus->slot_available_mask); + if (s != 32) { + nd->slot = s; + } else { + error_setg(errp, "Cannot register nubus card, no free slot " + "available"); + return; + } + } else { + /* Slot specified, make sure the slot is available */ + if (!(nubus->slot_available_mask & BIT(nd->slot))) { + error_setg(errp, "Cannot register nubus card, slot %d is " + "unavailable or already occupied", nd->slot); + return; + } } - nd->slot = nubus->current_slot++; + nubus->slot_available_mask &= ~BIT(nd->slot); /* Super */ slot_offset = nd->slot * NUBUS_SUPER_SLOT_SIZE; @@ -191,12 +205,18 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) nubus_register_format_block(nd); } +static Property nubus_device_properties[] = { + DEFINE_PROP_INT32("slot", NubusDevice, slot, -1), + DEFINE_PROP_END_OF_LIST() +}; + static void nubus_device_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); dc->realize = nubus_device_realize; dc->bus_type = TYPE_NUBUS_BUS; + device_class_set_props(dc, nubus_device_properties); } static const TypeInfo nubus_device_type_info = { diff --git a/include/hw/nubus/mac-nubus-bridge.h b/include/hw/nubus/mac-nubus-bridge.h index 36aa098dd4..118d67267d 100644 --- a/include/hw/nubus/mac-nubus-bridge.h +++ b/include/hw/nubus/mac-nubus-bridge.h @@ -12,6 +12,10 @@ #include "hw/nubus/nubus.h" #include "qom/object.h" +#define MAC_NUBUS_FIRST_SLOT 0x9 +#define MAC_NUBUS_LAST_SLOT 0xe +#define MAC_NUBUS_SLOT_NB (MAC_NUBUS_LAST_SLOT - MAC_NUBUS_FIRST_SLOT + 1) + #define TYPE_MAC_NUBUS_BRIDGE "mac-nubus-bridge" OBJECT_DECLARE_SIMPLE_TYPE(MacNubusState, MAC_NUBUS_BRIDGE) diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h index 89b0976aaa..988e4a2361 100644 --- a/include/hw/nubus/nubus.h +++ b/include/hw/nubus/nubus.h @@ -14,13 +14,12 @@ #include "qom/object.h" #define NUBUS_SUPER_SLOT_SIZE 0x10000000U -#define NUBUS_SUPER_SLOT_NB 0x9 +#define NUBUS_SUPER_SLOT_NB 0xe #define NUBUS_SLOT_SIZE 0x01000000 -#define NUBUS_SLOT_NB 0xF - -#define NUBUS_FIRST_SLOT 0x9 -#define NUBUS_LAST_SLOT 0xF +#define NUBUS_FIRST_SLOT 0x0 +#define NUBUS_LAST_SLOT 0xf +#define NUBUS_SLOT_NB (NUBUS_LAST_SLOT - NUBUS_FIRST_SLOT + 1) #define TYPE_NUBUS_DEVICE "nubus-device" OBJECT_DECLARE_SIMPLE_TYPE(NubusDevice, NUBUS_DEVICE) @@ -36,13 +35,13 @@ struct NubusBus { MemoryRegion super_slot_io; MemoryRegion slot_io; - int current_slot; + uint32_t slot_available_mask; }; struct NubusDevice { DeviceState qdev; - int slot; + int32_t slot; MemoryRegion super_slot_mem; MemoryRegion slot_mem;
Convert nubus_device_realize() to use a bitmap to manage available slots to allow for future Nubus devices to be plugged into arbitrary slots from the command line using a new qdev "slot" parameter for nubus devices. Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh machines as documented in "Desigining Cards and Drivers for the Macintosh Family". Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/nubus/mac-nubus-bridge.c | 4 ++++ hw/nubus/nubus-bus.c | 5 +++-- hw/nubus/nubus-device.c | 32 +++++++++++++++++++++++------ include/hw/nubus/mac-nubus-bridge.h | 4 ++++ include/hw/nubus/nubus.h | 13 ++++++------ 5 files changed, 43 insertions(+), 15 deletions(-)