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

[Cédric Le Goater]

On 07/06/2018 09:40 AM, Cédric Le Goater wrote:
> 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/address@hidden
>   drwxr-xr-x. 2 root root 0 Jul  2 04:22 
> /proc/device-tree/vdevice/address@hidden
> 
> which is going to have collisions.

maybe we could split the VIO index number space and use [0x0 - 0x7f] for 
the user defined  "reg" values and [0x80-0xff] the values allocated by the
QEMU VIO model, 0x71000000 and above ? 

C. 

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