| Message ID | 20210917075057.20924-5-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | nubus: bus, device, bridge, IRQ and address space improvements | expand |
Le 17/09/2021 à 09:50, 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. > > 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". Perhaps we can also add "NuBus Specification" for the non mac-nubus part? http://www.bitsavers.org/pdf/ti/nubus/2242825-0001_NuBus_Spec1983.pdf > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/nubus/mac-nubus-bridge.c | 3 +++ > hw/nubus/nubus-bus.c | 2 +- > hw/nubus/nubus-device.c | 29 +++++++++++++++++++++++++---- > include/hw/nubus/nubus.h | 4 ++-- > 4 files changed, 31 insertions(+), 7 deletions(-) > > diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c > index 7c329300b8..c1d77e2bc7 100644 > --- a/hw/nubus/mac-nubus-bridge.c > +++ b/hw/nubus/mac-nubus-bridge.c > @@ -18,6 +18,9 @@ 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(9, 6); Perhaps we can introduce MAC_NUBUS_FIRST_SLOT and MAC_NUBUS_LAST_SLOT #define MAC_NUBUS_FIRST_SLOT 0x9 #define MAC_NUBUS_LAST_SLOT 0xe MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT, MAC_NUBUS_LAST_SLOT - MAC_NUBUS_FIRST_SLOT + 1) > + > 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 5c13452308..404c1032e0 100644 > --- a/hw/nubus/nubus-bus.c > +++ b/hw/nubus/nubus-bus.c > @@ -84,7 +84,7 @@ static void nubus_init(Object *obj) > nubus, "nubus-slots", > NUBUS_SLOT_NB * NUBUS_SLOT_SIZE); > > - nubus->current_slot = NUBUS_FIRST_SLOT; > + nubus->slot_available_mask = MAKE_64BIT_MASK(0, 16); MAKE_64BIT_MASK(NUBUS_FIRST_SLOT, NUBUS_LAST_SLOT - NUBUS_FIRST_SLOT + 1) ? And we define 16 slots, but NUBUS_SLOT_NB (above) is 15. (I think it's the value for Mac as last slot is 0xe) > } > > static void nubus_class_init(ObjectClass *oc, void *data) > diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c > index c1832f73da..d91a1e4af3 100644 > --- a/hw/nubus/nubus-device.c > +++ b/hw/nubus/nubus-device.c > @@ -160,14 +160,35 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) > NubusDevice *nd = NUBUS_DEVICE(dev); > char *name; > hwaddr slot_offset; > + 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; > + } > + } > > - if (nubus->current_slot < NUBUS_FIRST_SLOT || > - nubus->current_slot > NUBUS_LAST_SLOT) { > - error_setg(errp, "Cannot register nubus card, not enough slots"); > + if (nd->slot < NUBUS_FIRST_SLOT || nd->slot > NUBUS_LAST_SLOT) { > + error_setg(errp, "Cannot register nubus card, slot must be " > + "between %d and %d", NUBUS_FIRST_SLOT, > + NUBUS_LAST_SLOT); Do we need this checking as we already checked the slot bit is available? Moreover it would be more accurate to rely on the bitmap as the first available slot differs between nubus and mac-nubus. > return; > } > > - nd->slot = nubus->current_slot++; > + nubus->slot_available_mask &= ~BIT(nd->slot); > > /* Super */ > slot_offset = (nd->slot - 6) * NUBUS_SUPER_SLOT_SIZE; > diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h > index 357f621d15..8ff4736259 100644 > --- a/include/hw/nubus/nubus.h > +++ b/include/hw/nubus/nubus.h > @@ -19,7 +19,7 @@ > #define NUBUS_SLOT_SIZE 0x01000000 > #define NUBUS_SLOT_NB 0xF > > -#define NUBUS_FIRST_SLOT 0x9 > +#define NUBUS_FIRST_SLOT 0x0 > #define NUBUS_LAST_SLOT 0xF > > #define TYPE_NUBUS_DEVICE "nubus-device" > @@ -36,7 +36,7 @@ struct NubusBus { > MemoryRegion super_slot_io; > MemoryRegion slot_io; > > - int current_slot; > + uint32_t slot_available_mask; > }; > > struct NubusDevice { >
On 20/09/2021 20:48, Laurent Vivier wrote: > Le 17/09/2021 à 09:50, 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. >> >> 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". > > Perhaps we can also add "NuBus Specification" for the non mac-nubus part? > > http://www.bitsavers.org/pdf/ti/nubus/2242825-0001_NuBus_Spec1983.pdf I can add that but I'm wondering if it would be better to do this in patch 13 ("nubus-bridge: introduce separate NubusBridge structure") where the comment is updated to reflect the difference between Nubus and Macintosh-specific Nubus? >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/nubus/mac-nubus-bridge.c | 3 +++ >> hw/nubus/nubus-bus.c | 2 +- >> hw/nubus/nubus-device.c | 29 +++++++++++++++++++++++++---- >> include/hw/nubus/nubus.h | 4 ++-- >> 4 files changed, 31 insertions(+), 7 deletions(-) >> >> diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c >> index 7c329300b8..c1d77e2bc7 100644 >> --- a/hw/nubus/mac-nubus-bridge.c >> +++ b/hw/nubus/mac-nubus-bridge.c >> @@ -18,6 +18,9 @@ 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(9, 6); > > Perhaps we can introduce MAC_NUBUS_FIRST_SLOT and MAC_NUBUS_LAST_SLOT > > #define MAC_NUBUS_FIRST_SLOT 0x9 > #define MAC_NUBUS_LAST_SLOT 0xe > > MAKE_64BIT_MASK(MAC_NUBUS_FIRST_SLOT, MAC_NUBUS_LAST_SLOT - MAC_NUBUS_FIRST_SLOT + 1) I wasn't so keen on the verbosity of the above approach, however since both yourself and Zoltan have suggested a similar thing then I will see how it looks for v5. >> + >> 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 5c13452308..404c1032e0 100644 >> --- a/hw/nubus/nubus-bus.c >> +++ b/hw/nubus/nubus-bus.c >> @@ -84,7 +84,7 @@ static void nubus_init(Object *obj) >> nubus, "nubus-slots", >> NUBUS_SLOT_NB * NUBUS_SLOT_SIZE); >> >> - nubus->current_slot = NUBUS_FIRST_SLOT; >> + nubus->slot_available_mask = MAKE_64BIT_MASK(0, 16); > > MAKE_64BIT_MASK(NUBUS_FIRST_SLOT, NUBUS_LAST_SLOT - NUBUS_FIRST_SLOT + 1) ? Same here. > And we define 16 slots, but NUBUS_SLOT_NB (above) is 15. (I think it's the value for Mac as last > slot is 0xe) If your suggested approach above works then I should be able to change NUBUS_SLOT_NB from 15 to 16 here with no other effect. >> } >> >> static void nubus_class_init(ObjectClass *oc, void *data) >> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c >> index c1832f73da..d91a1e4af3 100644 >> --- a/hw/nubus/nubus-device.c >> +++ b/hw/nubus/nubus-device.c >> @@ -160,14 +160,35 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) >> NubusDevice *nd = NUBUS_DEVICE(dev); >> char *name; >> hwaddr slot_offset; >> + 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; >> + } >> + } >> >> - if (nubus->current_slot < NUBUS_FIRST_SLOT || >> - nubus->current_slot > NUBUS_LAST_SLOT) { >> - error_setg(errp, "Cannot register nubus card, not enough slots"); >> + if (nd->slot < NUBUS_FIRST_SLOT || nd->slot > NUBUS_LAST_SLOT) { >> + error_setg(errp, "Cannot register nubus card, slot must be " >> + "between %d and %d", NUBUS_FIRST_SLOT, >> + NUBUS_LAST_SLOT); > > Do we need this checking as we already checked the slot bit is available? > Moreover it would be more accurate to rely on the bitmap as the first available slot differs between > nubus and mac-nubus. From the discussion of earlier versions of the patchset, the intention was really to keep the old restriction as a failsafe: I'm happy to remove this for v5. >> return; >> } >> >> - nd->slot = nubus->current_slot++; >> + nubus->slot_available_mask &= ~BIT(nd->slot); >> >> /* Super */ >> slot_offset = (nd->slot - 6) * NUBUS_SUPER_SLOT_SIZE; >> diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h >> index 357f621d15..8ff4736259 100644 >> --- a/include/hw/nubus/nubus.h >> +++ b/include/hw/nubus/nubus.h >> @@ -19,7 +19,7 @@ >> #define NUBUS_SLOT_SIZE 0x01000000 >> #define NUBUS_SLOT_NB 0xF >> >> -#define NUBUS_FIRST_SLOT 0x9 >> +#define NUBUS_FIRST_SLOT 0x0 >> #define NUBUS_LAST_SLOT 0xF >> >> #define TYPE_NUBUS_DEVICE "nubus-device" >> @@ -36,7 +36,7 @@ struct NubusBus { >> MemoryRegion super_slot_io; >> MemoryRegion slot_io; >> >> - int current_slot; >> + uint32_t slot_available_mask; >> }; >> >> struct NubusDevice { >> ATB, Mark.
diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c index 7c329300b8..c1d77e2bc7 100644 --- a/hw/nubus/mac-nubus-bridge.c +++ b/hw/nubus/mac-nubus-bridge.c @@ -18,6 +18,9 @@ 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(9, 6); + 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 5c13452308..404c1032e0 100644 --- a/hw/nubus/nubus-bus.c +++ b/hw/nubus/nubus-bus.c @@ -84,7 +84,7 @@ static void nubus_init(Object *obj) nubus, "nubus-slots", NUBUS_SLOT_NB * NUBUS_SLOT_SIZE); - nubus->current_slot = NUBUS_FIRST_SLOT; + nubus->slot_available_mask = MAKE_64BIT_MASK(0, 16); } static void nubus_class_init(ObjectClass *oc, void *data) diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c index c1832f73da..d91a1e4af3 100644 --- a/hw/nubus/nubus-device.c +++ b/hw/nubus/nubus-device.c @@ -160,14 +160,35 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) NubusDevice *nd = NUBUS_DEVICE(dev); char *name; hwaddr slot_offset; + 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; + } + } - if (nubus->current_slot < NUBUS_FIRST_SLOT || - nubus->current_slot > NUBUS_LAST_SLOT) { - error_setg(errp, "Cannot register nubus card, not enough slots"); + if (nd->slot < NUBUS_FIRST_SLOT || nd->slot > NUBUS_LAST_SLOT) { + error_setg(errp, "Cannot register nubus card, slot must be " + "between %d and %d", NUBUS_FIRST_SLOT, + NUBUS_LAST_SLOT); return; } - nd->slot = nubus->current_slot++; + nubus->slot_available_mask &= ~BIT(nd->slot); /* Super */ slot_offset = (nd->slot - 6) * NUBUS_SUPER_SLOT_SIZE; diff --git a/include/hw/nubus/nubus.h b/include/hw/nubus/nubus.h index 357f621d15..8ff4736259 100644 --- a/include/hw/nubus/nubus.h +++ b/include/hw/nubus/nubus.h @@ -19,7 +19,7 @@ #define NUBUS_SLOT_SIZE 0x01000000 #define NUBUS_SLOT_NB 0xF -#define NUBUS_FIRST_SLOT 0x9 +#define NUBUS_FIRST_SLOT 0x0 #define NUBUS_LAST_SLOT 0xF #define TYPE_NUBUS_DEVICE "nubus-device" @@ -36,7 +36,7 @@ struct NubusBus { MemoryRegion super_slot_io; MemoryRegion slot_io; - int current_slot; + uint32_t slot_available_mask; }; struct NubusDevice {
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. 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 | 3 +++ hw/nubus/nubus-bus.c | 2 +- hw/nubus/nubus-device.c | 29 +++++++++++++++++++++++++---- include/hw/nubus/nubus.h | 4 ++-- 4 files changed, 31 insertions(+), 7 deletions(-)