Re: [Qemu-devel] [PATCH v8 3/3] hw/arm/virt: add dynamic sysbus device support

[Eric Auger <eric.auger@linaro.org>]

Hi Peter,
On 01/06/2015 07:57 PM, Peter Maydell wrote:
> On 5 January 2015 at 16:14, Eric Auger <eric.auger@linaro.org> wrote:
>> Allows sysbus devices to be instantiated from command line by
>> using -device option. Machvirt creates a platform bus at init.
>> The dynamic sysbus devices are attached to this platform bus device.
> 
>> @@ -59,6 +61,8 @@
>>  #define GIC_FDT_IRQ_PPI_CPU_START 8
>>  #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
>>
>> +#define PLATFORM_BUS_NUM_IRQS     20
>> +
>>  enum {
>>      VIRT_FLASH,
>>      VIRT_MEM,
>> @@ -69,8 +73,11 @@ enum {
>>      VIRT_MMIO,
>>      VIRT_RTC,
>>      VIRT_FW_CFG,
>> +    VIRT_PLATFORM_BUS,
>>  };
>>
>> +static ARMPlatformBusSystemParams platform_bus_params;
>> +
>>  typedef struct MemMapEntry {
>>      hwaddr base;
>>      hwaddr size;
>> @@ -119,24 +126,26 @@ typedef struct {
>>   */
>>  static const MemMapEntry a15memmap[] = {
>>      /* Space up to 0x8000000 is reserved for a boot ROM */
>> -    [VIRT_FLASH] =      {          0, 0x08000000 },
>> -    [VIRT_CPUPERIPHS] = { 0x08000000, 0x00020000 },
>> +    [VIRT_FLASH] =          {          0, 0x08000000 },
>> +    [VIRT_CPUPERIPHS] =     { 0x08000000, 0x00020000 },
>>      /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */
>> -    [VIRT_GIC_DIST] =   { 0x08000000, 0x00010000 },
>> -    [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
>> -    [VIRT_UART] =       { 0x09000000, 0x00001000 },
>> -    [VIRT_RTC] =        { 0x09010000, 0x00001000 },
>> -    [VIRT_FW_CFG] =     { 0x09020000, 0x0000000a },
>> -    [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>> +    [VIRT_GIC_DIST] =       { 0x08000000, 0x00010000 },
>> +    [VIRT_GIC_CPU] =        { 0x08010000, 0x00010000 },
>> +    [VIRT_UART] =           { 0x09000000, 0x00001000 },
>> +    [VIRT_RTC] =            { 0x09010000, 0x00001000 },
>> +    [VIRT_FW_CFG] =         { 0x09020000, 0x0000000a },
> 
> Please don't re-indent unrelated lines in the same patch: it makes
> it very hard to tell whether any of them have actually changed.
sure I will create another patch file dedicated to that
> 
>> +    [VIRT_PLATFORM_BUS] =   { 0x09400000, 0x00400000 },
>> +    [VIRT_MMIO] =           { 0x0a000000, 0x00000200 },
>>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>>      /* 0x10000000 .. 0x40000000 reserved for PCI */
>> -    [VIRT_MEM] =        { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
>> +    [VIRT_MEM] =            { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
>>  };
>>
>>  static const int a15irqmap[] = {
>>      [VIRT_UART] = 1,
>>      [VIRT_RTC] = 2,
>>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>> +    [VIRT_PLATFORM_BUS] = 48, /* .. to 48 + PLATFORM_BUS_NUM_IRQS -1*/
> 
> Missing spaces again (also use '...to' for consistency with line above).
> 
>>  };
> 
> This patch generally looks OK, so I think the major question is:
> are we happy with this memory and IRQ usage?
> 
> Why 20 for PLATFORM_BUS_NUM_IRQS? It seems a funny number.
> Starting the platform bus IRQs at 48 means there is no scope
> at all for raising NUM_VIRTIO_TRANSPORTS later, which is
> not really what I had in mind, though in fact it seems
> unlikely that we'll ever really want to do that given the
> imminent advent of PCI. Still, I don't think it would
> hurt to start at 64.
Then I will start at 64.

About PLATFORM_BUS_NUM_IRQS= 20, I must acknowledge it is an arbitrary
figure. Alex used 10 in e500 as default value.

If I am not wrong a DMA330 can have up to 32 IRQ. It is another
candidate for VFIO. Would it be OK to use 32 instead of 20?

About memory, here we have space for 64 64kB pages. any other suggestions?

>  we have
>>  static VirtBoardInfo machines[] = {
>> @@ -556,6 +565,47 @@ static void create_fw_cfg(const VirtBoardInfo *vbi)
>>      g_free(nodename);
>>  }
>>
>> +static void create_platform_bus(VirtBoardInfo *vbi, qemu_irq *pic)
>> +{
>> +    DeviceState *dev;
>> +    SysBusDevice *s;
>> +    int i;
>> +    ARMPlatformBusFdtParams *fdt_params = g_new(ARMPlatformBusFdtParams, 1);
> 
> ...does the arm_register_platform_bus_fdt_creator() function
> implicitly agree to g_free() the pointer it's passed when it's
> done with it, or are we just leaking this?
Well I could deallocate in platform_bus_fdt_notify routine if you
recommend to do so.

Thanks for the review.

Eric
> 
> thanks
> -- PMM
>