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

[Laurent Vivier <laurent@vivier.eu>]

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 {
>