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