On Tue, 3 Jul 2018 17:19:56 +0200
Cédric Le Goater <c...@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?= <c...@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 <c...@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));
