Re: [PATCH v2 04/20] nubus: use bitmap to manage available slots

[Mark Cave-Ayland]

On 15/09/2021 11:39, Philippe Mathieu-Daudé wrote:

> On 9/15/21 12:10 PM, 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.
> >
> > 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     | 33 +++++++++++++++++++++++++++------
> >   include/hw/nubus/nubus.h    |  4 ++--
> >   4 files changed, 33 insertions(+), 9 deletions(-)
>
> >   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..f9f614cc01 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;
> > -
> > -    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;
>
> Can't we have s >= NUBUS_LAST_SLOT here?
>
> > +        } else {
> > +            error_setg(errp, "Cannot register nubus card, no free slot "
> > +                             "available");
> > +            return;
> > +        }
> > +    } else {
>
> If so, maybe better to move this code <...
>
> > +        /* Slot specified, make sure the slot is available */
> > +        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;
> > +        }
>
> ...> ...
>
> > +
> > +        if (!(nubus->slot_available_mask & (1UL << nd->slot))) {
> > +            error_setg(errp, "Cannot register nubus card, slot %d is "
> > +                             "unavailable or already occupied", nd->slot);
> > +            return;
> > +        }
> >       }
>
> ... here?

Functionally it shouldn't make a difference since the default mask is 0xffff, but it 
might just help the unwary programmer low on caffeine ;)

I'll test it later and make sure everything still works as intended.

> > -    nd->slot = nubus->current_slot++;
> > +    nubus->slot_available_mask &= ~(1UL << 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.