From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40320) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YmjJf-00067P-GO for qemu-devel@nongnu.org; Mon, 27 Apr 2015 09:43:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YmjJa-0000CO-Le for qemu-devel@nongnu.org; Mon, 27 Apr 2015 09:43:19 -0400 Received: from cantor2.suse.de ([195.135.220.15]:55930 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YmjJa-00009Y-4b for qemu-devel@nongnu.org; Mon, 27 Apr 2015 09:43:14 -0400 Message-ID: <553E3C71.1040001@suse.de> Date: Mon, 27 Apr 2015 15:41:05 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1429888773-11730-1-git-send-email-eric.auger@linaro.org> <1429888773-11730-4-git-send-email-eric.auger@linaro.org> In-Reply-To: <1429888773-11730-4-git-send-email-eric.auger@linaro.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v12 3/4] hw/arm/virt: add dynamic sysbus device support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Auger , eric.auger@st.com, qemu-devel@nongnu.org, peter.maydell@linaro.org, pbonzini@redhat.com Cc: b.reynal@virtualopensystems.com, patches@linaro.org, Bharat.Bhushan@freescale.com, alex.williamson@redhat.com, alex.bennee@linaro.org, kvmarm@lists.cs.columbia.edu, christoffer.dall@linaro.org 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 Hrm, are you sure this code is from me? :) > Signed-off-by: Eric Auger > Reviewed-by: Alex Benn=C3=A9e > > --- > 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 correc= t > 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" > =20 > #define NUM_VIRTIO_TRANSPORTS 32 > =20 > /* Number of external interrupt lines to configure the GIC with */ > -#define NUM_IRQS 128 > +#define NUM_IRQS 256 > =20 > #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 > =20 > +#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, > }; > =20 > +static ARMPlatformBusSystemParams platform_bus_params; > + > typedef struct MemMapEntry { > hwaddr base; > hwaddr size; > @@ -131,6 +138,7 @@ static const MemMapEntry a15memmap[] =3D { > [VIRT_FW_CFG] =3D { 0x09020000, 0x0000000a }, > [VIRT_MMIO] =3D { 0x0a000000, 0x00000200 }, > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of tha= t size */ > + [VIRT_PLATFORM_BUS] =3D { 0x0c000000, 0x02000000 }, Peter, would you have a hard time if we just get rid of VIRT_MMIO=20 completely and allow users to create the mmio-virtio bridges using=20 -device for -M virt-2.4 and above? At the end of the day, I'm fairly sure people will end up virtio-pci=20 anyway and it's just a big waste of address space to keep VIRT_MMIO=20 around, no? That said, if you want to keep virtio-mmio support in the way it is=20 today, I won't object. The rest of the patch looks good to me. Alex