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

[Alexander Graf <agraf@suse.de>]

On 04/24/2015 05:19 PM, Eric Auger 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.
>
> The platform bus device registers a machine init done notifier
> whose role will be to bind the dynamic sysbus devices. Indeed
> dynamic sysbus devices are created after machine init.
>
> machvirt also registers a notifier that will build the device
> tree nodes for the platform bus and its children dynamic sysbus
> devices.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Hrm, are you sure this code is from me? :)

> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v11 -> v12:
> - resize PLATFORM_BUS_NUM_IRQS to 64 instead of 32
> - overall NUM_IRQS changed to 256. This leaves space for MSI
>    looming addition
> - VIRT_PLATFORM_BUS size changed from 4MB to 32MB and base set at
>    0xc000000
> - Add Alex R-b
>
> v8 -> v9:
> - PLATFORM_BUS_NUM_IRQS set to 32 instead of 20
> - platform bus irq now start at 64 instead of 48
> - remove change of indentation in a15memmap
> - correct misc style issues
>
> v7 -> v8:
> - rebase on 2.2.0
> - in machvirt_init, create_platform_bus simply is added
>    after the arm_load_kernel call instead of moving this latter.
>    Related comment slighly reworded.
> - Due to those changes I dropped Alex and Shannon's Reviewed-by
>
> v6 -> v7:
> Take into account Shannon comments:
> - remove PLATFORM_BUS_FIRST_IRQ macro
> - correct platform bus size to 0x400000
> - add an additional comment in a15irqmap related to
>    PLATFORM_BUS_NUM_IRQS
>
> v5 -> v6:
> - Take into account Peter's comments:
>    - platform_bus_params initialized from vbi->memmap and vbi->irqmap.
>      As a consequence, const is removed. Also alignment in a15memmap
>      is slightly changed.
>    - ARMPlatformBusSystemParams handle removed from create_platform_bus
>      prototype
> - arm_load_kernel has become a machine init done notifier registration.
>    It must be called before platform_bus_create to guarantee the correct
>    notifier execution sequence
>
> v4 -> v5:
> - platform_bus_params becomes static const
> - reword comment in create_platform_bus
> - reword the commit message
>
> v3 -> v4:
> - use platform bus object, instantiated in create_platform_bus
> - device tree generation for platform bus and children dynamic
>    sysbus devices is no more handled at reset but in a
>    machine_init_done_notifier (due to the change in implementaion
>    of ARM load dtb using rom_add_blob_fixed).
> - device tree enhancement now takes into account the case of
>    user provided dtb. Before the user dtb was overwritten which
>    was wrong. However in case the dtb is provided by the user,
>    dynamic sysbus nodes are not added there.
> - renaming of MACHVIRT_PLATFORM defines
> - MACHVIRT_PLATFORM_PAGE_SHIFT and SIZE_PAGES not needed anymore,
>    hence removed.
> - DynSysbusParams struct renamed into ARMPlatformBusSystemParams
>    and above params removed.
> - separation of dt creation and QEMU binding is not mandated anymore
>    since the device tree is not created from scratch anymore. Instead
>    the modify_dtb function is used.
> - create_platform_bus registers another machine init done notifier
>    to start VFIO IRQ handling. This latter executes after the
>    dynamic sysbus device binding.
>
> v2 -> v3:
> - renaming of arm_platform_bus_create_devtree and arm_load_dtb
> - add copyright in hw/arm/dyn_sysbus_devtree.c
>
> v1 -> v2:
> - remove useless vfio-platform.h include file
> - s/MACHVIRT_PLATFORM_HOLE/MACHVIRT_PLATFORM_SIZE
> - use dyn_sysbus_binding and dyn_sysbus_devtree
> - dynamic sysbus platform buse size shrinked to 4MB and
>    moved between RTC and MMIO
>
> v1:
>
> Inspired from what Alex Graf did in ppc e500
> https://lists.gnu.org/archive/html/qemu-ppc/2014-07/msg00012.html
> ---
>   hw/arm/virt.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 565f573..efa3216 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -43,11 +43,13 @@
>   #include "qemu/bitops.h"
>   #include "qemu/error-report.h"
>   #include "hw/pci-host/gpex.h"
> +#include "hw/arm/sysbus-fdt.h"
> +#include "hw/platform-bus.h"
>   
>   #define NUM_VIRTIO_TRANSPORTS 32
>   
>   /* Number of external interrupt lines to configure the GIC with */
> -#define NUM_IRQS 128
> +#define NUM_IRQS 256
>   
>   #define GIC_FDT_IRQ_TYPE_SPI 0
>   #define GIC_FDT_IRQ_TYPE_PPI 1
> @@ -60,6 +62,8 @@
>   #define GIC_FDT_IRQ_PPI_CPU_START 8
>   #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
>   
> +#define PLATFORM_BUS_NUM_IRQS 64
> +
>   enum {
>       VIRT_FLASH,
>       VIRT_MEM,
> @@ -71,8 +75,11 @@ enum {
>       VIRT_RTC,
>       VIRT_FW_CFG,
>       VIRT_PCIE,
> +    VIRT_PLATFORM_BUS,
>   };
>   
> +static ARMPlatformBusSystemParams platform_bus_params;
> +
>   typedef struct MemMapEntry {
>       hwaddr base;
>       hwaddr size;
> @@ -131,6 +138,7 @@ static const MemMapEntry a15memmap[] = {
>       [VIRT_FW_CFG] =     { 0x09020000, 0x0000000a },
>       [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>       /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
> +    [VIRT_PLATFORM_BUS] =   { 0x0c000000, 0x02000000 },

Peter, would you have a hard time if we just get rid of VIRT_MMIO 
completely and allow users to create the mmio-virtio bridges using 
-device for -M virt-2.4 and above?

At the end of the day, I'm fairly sure people will end up virtio-pci 
anyway and it's just a big waste of address space to keep VIRT_MMIO 
around, no?

That said, if you want to keep virtio-mmio support in the way it is 
today, I won't object. The rest of the patch looks good to me.


Alex