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

[Cédric Le Goater]

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?= <address@hidden>
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.

Signed-off-by: Cédric Le Goater <address@hidden>
---
 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));
-- 
2.13.6