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

["Cédric Le Goater" <clg@kaod.org>]

On 07/06/2018 07:44 AM, David Gibson wrote:
> On Tue, Jul 03, 2018 at 05:19:56PM +0200, Cédric Le Goater 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 ? 
> 
> Not really, no.
> 
>> It does look like an allocator but I really don't know what else to 
>> propose :/ See below.
> 
> Not only is it a stealth allocator, it also means we have two
> different unique ids for VIO devices - the 'reg' and this new index.
> That sounds like a recipe for confusion.
> 
> I think we can do better.  I had a look at how these are allocated and
> it seems to be this:
> 
> In qemu:
> 	VIO devices start at reg=0x71000000, and just increment by one
> 	from there.
> 
> In libvirt:
> 	VIO net devices start at reg=0x1000
> 	VIO scsi devices start at reg=0x2000
> 	VIO nvram devices start at reg=0x3000

but a default VIO nvram device is always created by the machine. Here is 
a typical /vdevice layout :

  drwxr-xr-x. 2 root root 0 Jul  2 04:22 /proc/device-tree/vdevice/nvram@71000000
  drwxr-xr-x. 2 root root 0 Jul  2 04:22 /proc/device-tree/vdevice/vty@30000000

which is going to have collisions.

Should we set the "register" property of the defaut nvram device to some 
high value ? the sPAPR platform expects to always have a nvram device:

    R1–8.1–1.

      Platforms must implement at least 8 KB of Non-Volatile Memory. 
      The actual amount is platform dependent and must allow for 4 KB 
      for the OS. Platforms must provide an additional 4 KB for each 
      installed OS beyond the first.

So we can not remove it. 

The vty devices are dependent on the chardev backends. We are fine on that
side.

Thanks,

C.

> 	VIO vty devices start at reg=0x30000000
> 	    and increment by 0x1000 each type
> 
> So we could go for say:
> 	irq = (reg & 0xf) ^ ((reg >> 12) & 0xf);
> 
> Obviously it's easily to construct cases where that will result in
> collisions, but I don't think it'll happen for anyone not going out of
> there way to make it happen.
>