Message ID | 1429888773-11730-4-git-send-email-eric.auger@linaro.org |
---|---|
State | New |
Headers | show |
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
Hi Alex On 04/27/2015 03:41 PM, Alexander Graf wrote: > 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? :) Well I just wanted to emphasize this originates from your e500.c etsec integration. through this is true most of it now is in sysbus-fdt.c. I will remove your Signed-off-by if this is your will ;-) Best Regards Eric > >> 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 >
On 27 April 2015 at 14:41, Alexander Graf <agraf@suse.de> wrote: > 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? I would strongly prefer not -- it breaks existing working command lines. I agree that we are going to want to move to virtio-pci and virtio-mmio is just legacy, but retaining legacy is what makes QEMU not kvmtool, right? :-) -- PMM
On 04/27/2015 03:56 PM, Peter Maydell wrote: > On 27 April 2015 at 14:41, Alexander Graf <agraf@suse.de> wrote: >> 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? > I would strongly prefer not -- it breaks existing working command > lines. I agree that we are going to want to move to virtio-pci > and virtio-mmio is just legacy, but retaining legacy is what makes > QEMU not kvmtool, right? :-) Heh ;). Oh well, I guess the series is ready to go in then. I'd still like to see the virtio-mmio bridge get enabled for dynamic platform bus support soon, so that we can eventually phase out the preallocated region. Alex
Hi Alex, On 04/27/2015 09:41 AM, Alexander Graf wrote: >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -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? I'm not sure I have an opinion one way or the other, but I would like to understand the "big waste" argument. Is there something that users are eager to reuse this address space for, like more RAM? Thanks, Chris
On 04/27/2015 06:58 PM, Christopher Covington wrote: > Hi Alex, > > On 04/27/2015 09:41 AM, Alexander Graf wrote: > >>> --- a/hw/arm/virt.c >>> +++ b/hw/arm/virt.c >>> @@ -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? > I'm not sure I have an opinion one way or the other, but I would like to > understand the "big waste" argument. Is there something that users are eager > to reuse this address space for, like more RAM? It will get used for platform device assignment. If we only reserve 32MB we will quickly run out of address space to map devices into. Alex
On 27 April 2015 at 18:04, Alexander Graf <agraf@suse.de> wrote: > On 04/27/2015 06:58 PM, Christopher Covington wrote: >> I'm not sure I have an opinion one way or the other, but I would like to >> understand the "big waste" argument. Is there something that users are >> eager >> to reuse this address space for, like more RAM? > > > It will get used for platform device assignment. If we only reserve 32MB we > will quickly run out of address space to map devices into. Personally I am hoping that platform device assignment will also turn out to be mostly for legacy reasons and anybody wanting to do serious device passthrough will be doing it with PCI... -- PMM
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 }, /* * PCIE verbose map: * @@ -147,6 +155,7 @@ static const int a15irqmap[] = { [VIRT_RTC] = 2, [VIRT_PCIE] = 3, /* ... to 6 */ [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ + [VIRT_PLATFORM_BUS] = 64, /* ... to 64 + PLATFORM_BUS_NUM_IRQS -1 */ }; static VirtBoardInfo machines[] = { @@ -717,6 +726,47 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic, 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); + MemoryRegion *sysmem = get_system_memory(); + + platform_bus_params.platform_bus_base = vbi->memmap[VIRT_PLATFORM_BUS].base; + platform_bus_params.platform_bus_size = vbi->memmap[VIRT_PLATFORM_BUS].size; + platform_bus_params.platform_bus_first_irq = vbi->irqmap[VIRT_PLATFORM_BUS]; + platform_bus_params.platform_bus_num_irqs = PLATFORM_BUS_NUM_IRQS; + + fdt_params->system_params = &platform_bus_params; + fdt_params->binfo = &vbi->bootinfo; + fdt_params->intc = "/intc"; + /* + * register a machine init done notifier that creates the device tree + * nodes of the platform bus and its children dynamic sysbus devices + */ + arm_register_platform_bus_fdt_creator(fdt_params); + + dev = qdev_create(NULL, TYPE_PLATFORM_BUS_DEVICE); + dev->id = TYPE_PLATFORM_BUS_DEVICE; + qdev_prop_set_uint32(dev, "num_irqs", + platform_bus_params.platform_bus_num_irqs); + qdev_prop_set_uint32(dev, "mmio_size", + platform_bus_params.platform_bus_size); + qdev_init_nofail(dev); + s = SYS_BUS_DEVICE(dev); + + for (i = 0; i < platform_bus_params.platform_bus_num_irqs; i++) { + int irqn = platform_bus_params.platform_bus_first_irq + i; + sysbus_connect_irq(s, i, pic[irqn]); + } + + memory_region_add_subregion(sysmem, + platform_bus_params.platform_bus_base, + sysbus_mmio_get_region(s, 0)); +} + static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) { const VirtBoardInfo *board = (const VirtBoardInfo *)binfo; @@ -837,6 +887,14 @@ static void machvirt_init(MachineState *machine) vbi->bootinfo.get_dtb = machvirt_dtb; vbi->bootinfo.firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo); + + /* + * arm_load_kernel machine init done notifier registration must + * happen before the platform_bus_create call. In this latter, + * another notifier is registered which adds platform bus nodes. + * Notifiers are executed in registration reverse order. + */ + create_platform_bus(vbi, pic); } static bool virt_get_secure(Object *obj, Error **errp) @@ -875,6 +933,7 @@ static void virt_class_init(ObjectClass *oc, void *data) mc->desc = "ARM Virtual Machine", mc->init = machvirt_init; mc->max_cpus = 8; + mc->has_dynamic_sysbus = true; } static const TypeInfo machvirt_info = {