Re: [Qemu-devel] [PATCH v3 1/3] spapr: introduce a fixed IRQ number space

[Greg Kurz <groug@kaod.org>]

On Tue, 3 Jul 2018 17:19:56 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 07/02/2018 01:11 PM, Cédric Le Goater wrote:
> > On 07/02/2018 12:03 PM, Cédric Le Goater wrote:  
> >>> --- a/hw/ppc/spapr_vio.c
> >>> +++ b/hw/ppc/spapr_vio.c
> >>> @@ -436,6 +436,9 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
> >>>      }
> >>>  }
> >>>  
> >>> +/* TODO : poor VIO device indexing ... */
> >>> +static uint32_t vio_index;  
> >>
> >> I think we could also use (dev->reg & 0xff) as an index for 
> >> the VIO devices.
> >>
> >> The unit address of the virtual IOA is simply allocated using 
> >> an increment of bus->next_reg, next_reg being initialized at
> >> 0x71000000.
> >>
> >> I did not see any restrictions in the PAPR specs or in QEMU 
> >> that would break the above.  
> > 
> > That was until I discovered this macro : 
> > 
> >   #define DEFINE_SPAPR_PROPERTIES(type, field)           \
> >         DEFINE_PROP_UINT32("reg", type, field.reg, -1)
> >  
> > so 'reg' could have any value. We can not use it ...  
> 
> Would moving vio_index under the bus and incrementing it each time
> a VIO device is created be acceptable ? 
> 
> It does look like an allocator but I really don't know what else to 
> propose :/ See below.
> 
> Thanks,
> 
> C.
> 
> 
> From 35d206c8768498538ebc74764c65f7a8d59caa33 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
> Date: Tue, 3 Jul 2018 17:17:23 +0200
> Subject: [PATCH] spapr/vio: introduce a device index
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> To introduce a static layout of IRQ numbers, we need a clear identifier
> for all devices a sPAPR machine can use. The VIO devices have a "reg"
> property which can be set to any value by the user (or libvirt) and we
> can not rely on it. Add an "index" attribute defined by the bus when
> the device is created.
> 
> The number of VIO devices is limited to 100 to fit the IRQ range.
                                         0x100

> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

The patch looks ok to me.

>  include/hw/ppc/spapr_vio.h |  2 ++
>  hw/ppc/spapr_vio.c         | 13 ++++++++++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index e8b006d18f4e..d95abeffe963 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -60,6 +60,7 @@ typedef struct VIOsPAPRDeviceClass {
>  
>  struct VIOsPAPRDevice {
>      DeviceState qdev;
> +    uint32_t index;
>      uint32_t reg;
>      uint32_t irq;
>      uint64_t signal_state;
> @@ -75,6 +76,7 @@ struct VIOsPAPRDevice {
>  
>  struct VIOsPAPRBus {
>      BusState bus;
> +    uint32_t next_index;
>      uint32_t next_reg;
>  };
>  
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index be9af71437cc..4665422c11d2 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -445,14 +445,23 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>      }
>  }
>  
> +#define SPAPR_VIO_MAX_DEVICES 0x100
> +
>  static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> -    VIOsPAPRDevice *dev = (VIOsPAPRDevice *)qdev;
> +    VIOsPAPRBus *bus = SPAPR_VIO_BUS(qdev_get_parent_bus(qdev));
> +    VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(qdev);
>      VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
>      char *id;
>      Error *local_err = NULL;
>  
> +    if (bus->next_index == SPAPR_VIO_MAX_DEVICES) {
> +        error_setg(errp, "Maximum number of VIO devices reached");
> +        return;
> +    }
> +    dev->index = bus->next_index++;
> +
>      if (dev->reg != -1) {
>          /*
>           * Explicitly assigned address, just verify that no-one else
> @@ -471,8 +480,6 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>          }
>      } else {
>          /* Need to assign an address */
> -        VIOsPAPRBus *bus = SPAPR_VIO_BUS(dev->qdev.parent_bus);
> -
>          do {
>              dev->reg = bus->next_reg++;
>          } while (reg_conflict(dev));