* [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update @ 2014-11-21 18:07 Alvise Rigo 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt Alvise Rigo ` (5 more replies) 0 siblings, 6 replies; 29+ messages in thread From: Alvise Rigo @ 2014-11-21 18:07 UTC (permalink / raw) To: qemu-devel; +Cc: rob.herring, tech, claudio.fontana, Alvise Rigo, peter.maydell This patch series is based on the previous work [1] and [2] by Rob Herring and on [3] by myself. For sake of readability and since this is still a RFC, these patches come as a stand alone work, so there's no need to apply first [1][2][3]. it tries to enhance this work on these points: Improvements from v1: - The code should be general enough to allow the use of the controller with other platforms, not only with mach-virt. The only assumption made is that a GIC v2 is used at guest side (the interrupt-map property describes the parent interrupts using the three cells format). - The interrupt-map node generation has been enhanced in the following aspects: - support of multi-function PCI device has been added - a PCI device can now use an interrupt pin different from #INTA Since some other works like [4] require to modify the device tree only when all the devices have been instantiated, the PATCH[1/4] proposes a solution for mach-virt to allow multiple agents (e.g., generic-pci, VFIO) to modify the device tree. The approach in simple: a global list is kept to store all the routines that perform the modification of the device tree. Eventually, when the machine is completed, all these routines are sequentially executed and the kernel is loaded to the guest by mean of a machine_init_done_notifier. In the context of this patch, here are some questions: Rather than postponing the arm_load_kernel call as this patch does, should we use instead the modify_dtb call provided by arm_boot_info to modify the device tree? If so, shouldn't modify_dtb be used to modify only *user* provided device trees? This work has been tested attaching several PCI devices to the mach-virt platform using an ARMv7 CPU. The tested devices are: virtio-blk-pci, virtio-net-pci, lsi53c895a and pci-ohci (all attached at the same time). TODO: - Add MSI, MSI-X support - PCI-E support. Due to a lack of devices, this part is a bit hard to accomplish at the moment. Thank you, alvise [1] "[Qemu-devel] [RFC PATCH 1/2] hw/pci-host: add a generic PCI host" http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html [2] "[Qemu-devel] [RFC PATCH 2/2] hw/arm/virt: Add generic PCI host device" http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03483.html [3] "[Qemu-devel] [RFC PATCH 0/8] Add Generic PCI host device update" https://lists.gnu.org/archive/html/qemu-devel/2014-07/msg01957.html [4] http://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg03816.html Alvise Rigo (4): hw/arm/virt: Allow multiple agents to modify dt hw/arm/virt: find_machine_info: handle NULL value hw/pci-host: Add a generic PCI host controller for virtual platforms hw/arm/virt: Add generic-pci device support hw/arm/virt.c | 114 +++++++++++++++- hw/pci-host/Makefile.objs | 2 +- hw/pci-host/generic-pci.c | 280 ++++++++++++++++++++++++++++++++++++++ include/hw/pci-host/generic-pci.h | 74 ++++++++++ 4 files changed, 468 insertions(+), 2 deletions(-) create mode 100644 hw/pci-host/generic-pci.c create mode 100644 include/hw/pci-host/generic-pci.h -- 2.1.0 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2014-11-21 18:07 [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update Alvise Rigo @ 2014-11-21 18:07 ` Alvise Rigo 2014-11-24 11:47 ` Claudio Fontana 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 2/4] hw/arm/virt: find_machine_info: handle NULL value Alvise Rigo ` (4 subsequent siblings) 5 siblings, 1 reply; 29+ messages in thread From: Alvise Rigo @ 2014-11-21 18:07 UTC (permalink / raw) To: qemu-devel; +Cc: rob.herring, tech, claudio.fontana, Alvise Rigo, peter.maydell Keep a global list with all the functions that need to modify the device tree. Using qemu_add_machine_init_done_notifier we register a notifier that executes all the functions on the list and loads the kernel. Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> --- hw/arm/virt.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 314e55b..e8d527d 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -70,6 +70,16 @@ enum { VIRT_RTC, }; +typedef void (*modify_dtb_func)(void *fdt, DeviceState *dev); +typedef struct DTModifier { + QLIST_ENTRY(DTModifier) entry; + modify_dtb_func modify_dtb; + DeviceState *dev; +} DTModifier; + +static QLIST_HEAD(, DTModifier) dtb_modifiers = + QLIST_HEAD_INITIALIZER(dtb_modifiers); + typedef struct MemMapEntry { hwaddr base; hwaddr size; @@ -149,6 +159,23 @@ static VirtBoardInfo *find_machine_info(const char *cpu) return NULL; } +static void add_dtb_modifier(modify_dtb_func func, DeviceState *dev) +{ + DTModifier *mod_entry = g_new(DTModifier, 1); + mod_entry->modify_dtb = func; + mod_entry->dev = dev; + QLIST_INSERT_HEAD(&dtb_modifiers, mod_entry, entry); +} + +static void free_dtb_modifiers(void) +{ + while (!QLIST_EMPTY(&dtb_modifiers)) { + DTModifier *modifier = QLIST_FIRST(&dtb_modifiers); + QLIST_REMOVE(modifier, entry); + g_free(modifier); + } +} + static void create_fdt(VirtBoardInfo *vbi) { void *fdt = create_device_tree(&vbi->fdt_size); @@ -527,6 +554,29 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) return board->fdt; } +static void machvirt_finalize_dt(Notifier *notify, void *data) +{ + VirtBoardInfo *vbi; + MachineState *machine; + + machine = MACHINE(qdev_get_machine()); + + vbi = find_machine_info(machine->cpu_model); + if (!vbi) { + vbi = find_machine_info("cortex-a15"); + } + + struct DTModifier *modifier, *next; + QLIST_FOREACH_SAFE(modifier, &dtb_modifiers, entry, next) { + modifier->modify_dtb(vbi->fdt, modifier->dev); + } + + free_dtb_modifiers(); + + /* Load the kernel only after that the device tree has been modified. */ + arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo); +} + static void machvirt_init(MachineState *machine) { qemu_irq pic[NUM_IRQS]; @@ -604,6 +654,10 @@ static void machvirt_init(MachineState *machine) */ create_virtio_devices(vbi, pic); + Notifier *finalize_dtb_notifier = g_new(Notifier, 1); + finalize_dtb_notifier->notify = machvirt_finalize_dt; + qemu_add_machine_init_done_notifier(finalize_dtb_notifier); + vbi->bootinfo.ram_size = machine->ram_size; vbi->bootinfo.kernel_filename = machine->kernel_filename; vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline; @@ -612,7 +666,6 @@ static void machvirt_init(MachineState *machine) vbi->bootinfo.board_id = -1; vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base; vbi->bootinfo.get_dtb = machvirt_dtb; - arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo); } static QEMUMachine machvirt_a15_machine = { -- 2.1.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt Alvise Rigo @ 2014-11-24 11:47 ` Claudio Fontana 2015-01-05 15:36 ` Peter Maydell 0 siblings, 1 reply; 29+ messages in thread From: Claudio Fontana @ 2014-11-24 11:47 UTC (permalink / raw) To: Alvise Rigo, qemu-devel, peter.maydell; +Cc: rob.herring, tech On 21.11.2014 19:07, Alvise Rigo wrote: > Keep a global list with all the functions that need to modify the device > tree. Using qemu_add_machine_init_done_notifier we register a notifier > that executes all the functions on the list and loads the kernel. > > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> Peter, could you weigh in about whether this is a good idea or not? > --- > hw/arm/virt.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 54 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 314e55b..e8d527d 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -70,6 +70,16 @@ enum { > VIRT_RTC, > }; > > +typedef void (*modify_dtb_func)(void *fdt, DeviceState *dev); > +typedef struct DTModifier { > + QLIST_ENTRY(DTModifier) entry; > + modify_dtb_func modify_dtb; > + DeviceState *dev; > +} DTModifier; > + > +static QLIST_HEAD(, DTModifier) dtb_modifiers = > + QLIST_HEAD_INITIALIZER(dtb_modifiers); > + > typedef struct MemMapEntry { > hwaddr base; > hwaddr size; > @@ -149,6 +159,23 @@ static VirtBoardInfo *find_machine_info(const char *cpu) > return NULL; > } > > +static void add_dtb_modifier(modify_dtb_func func, DeviceState *dev) > +{ > + DTModifier *mod_entry = g_new(DTModifier, 1); > + mod_entry->modify_dtb = func; > + mod_entry->dev = dev; > + QLIST_INSERT_HEAD(&dtb_modifiers, mod_entry, entry); > +} > + > +static void free_dtb_modifiers(void) > +{ > + while (!QLIST_EMPTY(&dtb_modifiers)) { > + DTModifier *modifier = QLIST_FIRST(&dtb_modifiers); > + QLIST_REMOVE(modifier, entry); > + g_free(modifier); > + } > +} > + > static void create_fdt(VirtBoardInfo *vbi) > { > void *fdt = create_device_tree(&vbi->fdt_size); > @@ -527,6 +554,29 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) > return board->fdt; > } > > +static void machvirt_finalize_dt(Notifier *notify, void *data) > +{ > + VirtBoardInfo *vbi; > + MachineState *machine; > + > + machine = MACHINE(qdev_get_machine()); > + > + vbi = find_machine_info(machine->cpu_model); > + if (!vbi) { > + vbi = find_machine_info("cortex-a15"); > + } > + > + struct DTModifier *modifier, *next; > + QLIST_FOREACH_SAFE(modifier, &dtb_modifiers, entry, next) { > + modifier->modify_dtb(vbi->fdt, modifier->dev); > + } > + > + free_dtb_modifiers(); > + > + /* Load the kernel only after that the device tree has been modified. */ > + arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo); > +} > + > static void machvirt_init(MachineState *machine) > { > qemu_irq pic[NUM_IRQS]; > @@ -604,6 +654,10 @@ static void machvirt_init(MachineState *machine) > */ > create_virtio_devices(vbi, pic); > > + Notifier *finalize_dtb_notifier = g_new(Notifier, 1); > + finalize_dtb_notifier->notify = machvirt_finalize_dt; > + qemu_add_machine_init_done_notifier(finalize_dtb_notifier); > + > vbi->bootinfo.ram_size = machine->ram_size; > vbi->bootinfo.kernel_filename = machine->kernel_filename; > vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline; > @@ -612,7 +666,6 @@ static void machvirt_init(MachineState *machine) > vbi->bootinfo.board_id = -1; > vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base; > vbi->bootinfo.get_dtb = machvirt_dtb; > - arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo); > } > > static QEMUMachine machvirt_a15_machine = { > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2014-11-24 11:47 ` Claudio Fontana @ 2015-01-05 15:36 ` Peter Maydell 2015-01-05 16:14 ` alvise rigo 0 siblings, 1 reply; 29+ messages in thread From: Peter Maydell @ 2015-01-05 15:36 UTC (permalink / raw) To: Claudio Fontana; +Cc: Rob Herring, tech, Alvise Rigo, QEMU Developers On 24 November 2014 at 11:47, Claudio Fontana <claudio.fontana@huawei.com> wrote: > On 21.11.2014 19:07, Alvise Rigo wrote: >> Keep a global list with all the functions that need to modify the device >> tree. Using qemu_add_machine_init_done_notifier we register a notifier >> that executes all the functions on the list and loads the kernel. >> >> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> > > Peter, could you weigh in about whether this is a good idea or not? Sorry, I think I must have missed this series first time around. I'm not convinced -- I don't see any reason why we should treat the PCI host controller differently from other devices in the virt board: just generate an appropriate dtb fragment in virt.c. thanks -- PMM ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2015-01-05 15:36 ` Peter Maydell @ 2015-01-05 16:14 ` alvise rigo 2015-01-05 16:41 ` Peter Maydell 2015-01-06 9:18 ` Eric Auger 0 siblings, 2 replies; 29+ messages in thread From: alvise rigo @ 2015-01-05 16:14 UTC (permalink / raw) To: Peter Maydell; +Cc: Rob Herring, tech, Claudio Fontana, QEMU Developers Hi, On Mon, Jan 5, 2015 at 4:36 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 24 November 2014 at 11:47, Claudio Fontana > <claudio.fontana@huawei.com> wrote: >> On 21.11.2014 19:07, Alvise Rigo wrote: >>> Keep a global list with all the functions that need to modify the device >>> tree. Using qemu_add_machine_init_done_notifier we register a notifier >>> that executes all the functions on the list and loads the kernel. >>> >>> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> >> >> Peter, could you weigh in about whether this is a good idea or not? > > Sorry, I think I must have missed this series first time around. > I'm not convinced -- I don't see any reason why we should treat > the PCI host controller differently from other devices in the The reason for this is that the PCI host controller needs to generate its device node after all the PCI devices have been added to the bus (also those specified with the -device option). This is required by the interrupt-map node property, that specifies for each PCI device an interrupt map entry. Since we have one device requiring this 'postponed' node generation, this patch allows also other devices to do the same. Recently I was thinking to another approach, which is to have multiple dtb modifiers called by arm_boot_info.modify_dtb(). Is this reasonable? > virt board: just generate an appropriate dtb fragment in virt.c. Of course, the method that generates the device node fragment can be moved to virt.c, but the requirement of postponing its call after the machine has been created still applies. Thank you for your feedback, alvise > > thanks > -- PMM ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2015-01-05 16:14 ` alvise rigo @ 2015-01-05 16:41 ` Peter Maydell 2015-01-05 17:35 ` alvise rigo 2015-01-06 9:18 ` Eric Auger 1 sibling, 1 reply; 29+ messages in thread From: Peter Maydell @ 2015-01-05 16:41 UTC (permalink / raw) To: alvise rigo; +Cc: Rob Herring, tech, Claudio Fontana, QEMU Developers On 5 January 2015 at 16:14, alvise rigo <a.rigo@virtualopensystems.com> wrote: > On Mon, Jan 5, 2015 at 4:36 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> Sorry, I think I must have missed this series first time around. >> I'm not convinced -- I don't see any reason why we should treat >> the PCI host controller differently from other devices in the > > The reason for this is that the PCI host controller needs to generate > its device node after all the PCI devices have been added to the bus > (also those specified with the -device option). > This is required by the interrupt-map node property, that specifies > for each PCI device an interrupt map entry. Since we have one device > requiring this 'postponed' node generation, this patch allows also > other devices to do the same. What? This doesn't sound right -- you can have hot-plugged PCI devices, for a start. Device tree is only supposed to be needed for the bits of hardware that can't be probed, and we can rely on PCI itself to probe the other devices. interrupt-map as far as I can tell just specifies how the interrupt lines are mapped for each PCI slot; it won't change based on whether devices are present or not. The example in the wiki: http://devicetree.org/Device_Tree_Usage#Advanced_Interrupt_Mapping cares about number of slots, but that's all. thanks -- PMM ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2015-01-05 16:41 ` Peter Maydell @ 2015-01-05 17:35 ` alvise rigo 2015-01-05 18:07 ` Peter Maydell 0 siblings, 1 reply; 29+ messages in thread From: alvise rigo @ 2015-01-05 17:35 UTC (permalink / raw) To: Peter Maydell; +Cc: Rob Herring, tech, Claudio Fontana, QEMU Developers On Mon, Jan 5, 2015 at 5:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 January 2015 at 16:14, alvise rigo <a.rigo@virtualopensystems.com> wrote: >> On Mon, Jan 5, 2015 at 4:36 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> Sorry, I think I must have missed this series first time around. >>> I'm not convinced -- I don't see any reason why we should treat >>> the PCI host controller differently from other devices in the >> >> The reason for this is that the PCI host controller needs to generate >> its device node after all the PCI devices have been added to the bus >> (also those specified with the -device option). >> This is required by the interrupt-map node property, that specifies >> for each PCI device an interrupt map entry. Since we have one device >> requiring this 'postponed' node generation, this patch allows also >> other devices to do the same. > > What? This doesn't sound right -- you can have hot-plugged PCI > devices, for a start. Device tree is only supposed to be > needed for the bits of hardware that can't be probed, and > we can rely on PCI itself to probe the other devices. OK, I see. > > interrupt-map as far as I can tell just specifies how the > interrupt lines are mapped for each PCI slot; it won't > change based on whether devices are present or not. The > example in the wiki: > http://devicetree.org/Device_Tree_Usage#Advanced_Interrupt_Mapping > cares about number of slots, but that's all. So I suppose we need to define a fixed number of PCI slots according to the number of interrupts available in mach-virt. In this regard, should this number be a qdev property? Thank you, alvise > > thanks > -- PMM ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2015-01-05 17:35 ` alvise rigo @ 2015-01-05 18:07 ` Peter Maydell 2015-01-06 9:56 ` alvise rigo 0 siblings, 1 reply; 29+ messages in thread From: Peter Maydell @ 2015-01-05 18:07 UTC (permalink / raw) To: alvise rigo; +Cc: Rob Herring, tech, Claudio Fontana, QEMU Developers On 5 January 2015 at 17:35, alvise rigo <a.rigo@virtualopensystems.com> wrote: > So I suppose we need to define a fixed number of PCI slots according > to the number of interrupts available in mach-virt. In this regard, > should this number be a qdev property? The PCI spec means that a bus has an implicit maximum of 32 slots (the devfn in a PCI address is only 5 bits). Note that this doesn't imply having 32 interrupt lines. What you can do is something like this (which is what the versatilepb device-tree-binding will have): + interrupt-map-mask = <0x1800 0 0 7>; + interrupt-map = <0x1800 0 0 1 &sic 28 + 0x1800 0 0 2 &sic 29 + 0x1800 0 0 3 &sic 30 + 0x1800 0 0 4 &sic 27 + + 0x1000 0 0 1 &sic 27 + 0x1000 0 0 2 &sic 28 + 0x1000 0 0 3 &sic 29 + 0x1000 0 0 4 &sic 30 + + 0x0800 0 0 1 &sic 30 + 0x0800 0 0 2 &sic 27 + 0x0800 0 0 3 &sic 28 + 0x0800 0 0 4 &sic 29 + + 0x0000 0 0 1 &sic 29 + 0x0000 0 0 2 &sic 30 + 0x0000 0 0 3 &sic 27 + 0x0000 0 0 4 &sic 28>; That says "we have four interrupts, which are swizzled in the usual way between slots", and doesn't impose any particular maximum number of slots. (Notice that the mask value is 0x1800 0 0 0 7, which means we only match on the low two bits of the devfn, so a unit-interrupt-specifier of <0x2000 0x0 0x0 1> for slot 4 matches the entry <0x0000 0x0 0x0 1> in the map table, as required.) (You'll want to do something more sensible than 27..30, which is just what the interrupt lines on the versatilepb happen to be.) -- PMM ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2015-01-05 18:07 ` Peter Maydell @ 2015-01-06 9:56 ` alvise rigo 0 siblings, 0 replies; 29+ messages in thread From: alvise rigo @ 2015-01-06 9:56 UTC (permalink / raw) To: Peter Maydell; +Cc: Rob Herring, tech, Claudio Fontana, QEMU Developers Thank you. I will keep this in mind for the next spin of the patches. Regards, alvise On Mon, Jan 5, 2015 at 7:07 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 January 2015 at 17:35, alvise rigo <a.rigo@virtualopensystems.com> wrote: >> So I suppose we need to define a fixed number of PCI slots according >> to the number of interrupts available in mach-virt. In this regard, >> should this number be a qdev property? > > The PCI spec means that a bus has an implicit maximum of > 32 slots (the devfn in a PCI address is only 5 bits). > Note that this doesn't imply having 32 interrupt lines. > > What you can do is something like this (which is what the > versatilepb device-tree-binding will have): > > + interrupt-map-mask = <0x1800 0 0 7>; > + interrupt-map = <0x1800 0 0 1 &sic 28 > + 0x1800 0 0 2 &sic 29 > + 0x1800 0 0 3 &sic 30 > + 0x1800 0 0 4 &sic 27 > + > + 0x1000 0 0 1 &sic 27 > + 0x1000 0 0 2 &sic 28 > + 0x1000 0 0 3 &sic 29 > + 0x1000 0 0 4 &sic 30 > + > + 0x0800 0 0 1 &sic 30 > + 0x0800 0 0 2 &sic 27 > + 0x0800 0 0 3 &sic 28 > + 0x0800 0 0 4 &sic 29 > + > + 0x0000 0 0 1 &sic 29 > + 0x0000 0 0 2 &sic 30 > + 0x0000 0 0 3 &sic 27 > + 0x0000 0 0 4 &sic 28>; > > That says "we have four interrupts, which are swizzled in > the usual way between slots", and doesn't impose any > particular maximum number of slots. (Notice that the > mask value is 0x1800 0 0 0 7, which means we only match > on the low two bits of the devfn, so a unit-interrupt-specifier > of <0x2000 0x0 0x0 1> for slot 4 matches the entry > <0x0000 0x0 0x0 1> in the map table, as required.) > > (You'll want to do something more sensible than 27..30, > which is just what the interrupt lines on the versatilepb > happen to be.) > > -- PMM ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2015-01-05 16:14 ` alvise rigo 2015-01-05 16:41 ` Peter Maydell @ 2015-01-06 9:18 ` Eric Auger 2015-01-06 9:29 ` alvise rigo 2015-01-06 9:51 ` Peter Maydell 1 sibling, 2 replies; 29+ messages in thread From: Eric Auger @ 2015-01-06 9:18 UTC (permalink / raw) To: alvise rigo, Peter Maydell Cc: Rob Herring, tech, Claudio Fontana, QEMU Developers On 01/05/2015 05:14 PM, alvise rigo wrote: > Hi, > > On Mon, Jan 5, 2015 at 4:36 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 24 November 2014 at 11:47, Claudio Fontana >> <claudio.fontana@huawei.com> wrote: >>> On 21.11.2014 19:07, Alvise Rigo wrote: >>>> Keep a global list with all the functions that need to modify the device >>>> tree. Using qemu_add_machine_init_done_notifier we register a notifier >>>> that executes all the functions on the list and loads the kernel. >>>> >>>> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> >>> >>> Peter, could you weigh in about whether this is a good idea or not? >> >> Sorry, I think I must have missed this series first time around. >> I'm not convinced -- I don't see any reason why we should treat >> the PCI host controller differently from other devices in the > > The reason for this is that the PCI host controller needs to generate > its device node after all the PCI devices have been added to the bus > (also those specified with the -device option). > This is required by the interrupt-map node property, that specifies > for each PCI device an interrupt map entry. Since we have one device > requiring this 'postponed' node generation, this patch allows also > other devices to do the same. > Recently I was thinking to another approach, which is to have multiple > dtb modifiers called by arm_boot_info.modify_dtb(). Is this > reasonable? > >> virt board: just generate an appropriate dtb fragment in virt.c. > > Of course, the method that generates the device node fragment can be > moved to virt.c, but the requirement of postponing its call after the > machine has been created still applies. Hi Alvise, Peter, Besides the PCI aspects, the dt generation problem that is addressed here is identical to the one related to VFIO platform device dt node generation that also needs to happen after machine init. In "machvirt dynamic sysbus device instantiation" (https://www.mail-archive.com/qemu-devel@nongnu.org/msg272506.html), arm_load_kernel is proposed to be executed in a machine init done notifier and VFIO node creation happens in another machine init done notifier registered after this latter. Best Regards Eric > > Thank you for your feedback, > alvise > >> >> thanks >> -- PMM > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2015-01-06 9:18 ` Eric Auger @ 2015-01-06 9:29 ` alvise rigo 2015-01-06 9:51 ` Peter Maydell 1 sibling, 0 replies; 29+ messages in thread From: alvise rigo @ 2015-01-06 9:29 UTC (permalink / raw) To: Eric Auger Cc: Peter Maydell, tech, Claudio Fontana, QEMU Developers, Rob Herring Hi Eric, You are right. In fact, I've also spent some time to see if it was possible to use the code you mentioned. However, it's not needed anymore: the node generation will happen at machine init for the reasons discussed in this thread. Regards, alvise On Tue, Jan 6, 2015 at 10:18 AM, Eric Auger <eric.auger@linaro.org> wrote: > On 01/05/2015 05:14 PM, alvise rigo wrote: >> Hi, >> >> On Mon, Jan 5, 2015 at 4:36 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 24 November 2014 at 11:47, Claudio Fontana >>> <claudio.fontana@huawei.com> wrote: >>>> On 21.11.2014 19:07, Alvise Rigo wrote: >>>>> Keep a global list with all the functions that need to modify the device >>>>> tree. Using qemu_add_machine_init_done_notifier we register a notifier >>>>> that executes all the functions on the list and loads the kernel. >>>>> >>>>> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> >>>> >>>> Peter, could you weigh in about whether this is a good idea or not? >>> >>> Sorry, I think I must have missed this series first time around. >>> I'm not convinced -- I don't see any reason why we should treat >>> the PCI host controller differently from other devices in the >> >> The reason for this is that the PCI host controller needs to generate >> its device node after all the PCI devices have been added to the bus >> (also those specified with the -device option). >> This is required by the interrupt-map node property, that specifies >> for each PCI device an interrupt map entry. Since we have one device >> requiring this 'postponed' node generation, this patch allows also >> other devices to do the same. >> Recently I was thinking to another approach, which is to have multiple >> dtb modifiers called by arm_boot_info.modify_dtb(). Is this >> reasonable? >> >>> virt board: just generate an appropriate dtb fragment in virt.c. >> >> Of course, the method that generates the device node fragment can be >> moved to virt.c, but the requirement of postponing its call after the >> machine has been created still applies. > > Hi Alvise, Peter, > > Besides the PCI aspects, the dt generation problem that is addressed > here is identical to the one related to VFIO platform device dt node > generation that also needs to happen after machine init. > > In "machvirt dynamic sysbus device instantiation" > (https://www.mail-archive.com/qemu-devel@nongnu.org/msg272506.html), > arm_load_kernel is proposed to be executed in a machine init done > notifier and VFIO node creation happens in another machine init done > notifier registered after this latter. > > Best Regards > > Eric >> >> Thank you for your feedback, >> alvise >> >>> >>> thanks >>> -- PMM >> > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2015-01-06 9:18 ` Eric Auger 2015-01-06 9:29 ` alvise rigo @ 2015-01-06 9:51 ` Peter Maydell 2015-01-06 10:05 ` Eric Auger 1 sibling, 1 reply; 29+ messages in thread From: Peter Maydell @ 2015-01-06 9:51 UTC (permalink / raw) To: Eric Auger Cc: Rob Herring, tech, Claudio Fontana, alvise rigo, QEMU Developers On 6 January 2015 at 09:18, Eric Auger <eric.auger@linaro.org> wrote: > Besides the PCI aspects, the dt generation problem that is addressed > here is identical to the one related to VFIO platform device dt node > generation that also needs to happen after machine init. Right, for VFIO we need it; but for PCI we don't, so we shouldn't tangle the two up together unnecessarily. -- PMM ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt 2015-01-06 9:51 ` Peter Maydell @ 2015-01-06 10:05 ` Eric Auger 0 siblings, 0 replies; 29+ messages in thread From: Eric Auger @ 2015-01-06 10:05 UTC (permalink / raw) To: Peter Maydell Cc: Rob Herring, tech, Claudio Fontana, alvise rigo, QEMU Developers On 01/06/2015 10:51 AM, Peter Maydell wrote: > On 6 January 2015 at 09:18, Eric Auger <eric.auger@linaro.org> wrote: >> Besides the PCI aspects, the dt generation problem that is addressed >> here is identical to the one related to VFIO platform device dt node >> generation that also needs to happen after machine init. > > Right, for VFIO we need it; but for PCI we don't, so we shouldn't > tangle the two up together unnecessarily. OK understood Eric > > -- PMM > ^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [RFC v2 2/4] hw/arm/virt: find_machine_info: handle NULL value 2014-11-21 18:07 [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update Alvise Rigo 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt Alvise Rigo @ 2014-11-21 18:07 ` Alvise Rigo 2015-01-05 15:36 ` Peter Maydell 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms Alvise Rigo ` (3 subsequent siblings) 5 siblings, 1 reply; 29+ messages in thread From: Alvise Rigo @ 2014-11-21 18:07 UTC (permalink / raw) To: qemu-devel; +Cc: rob.herring, tech, claudio.fontana, Alvise Rigo, peter.maydell Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> --- hw/arm/virt.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index e8d527d..4e7b869 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -151,6 +151,10 @@ static VirtBoardInfo *find_machine_info(const char *cpu) { int i; + if (!cpu) { + return NULL; + } + for (i = 0; i < ARRAY_SIZE(machines); i++) { if (strcmp(cpu, machines[i].cpu_model) == 0) { return &machines[i]; -- 2.1.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 2/4] hw/arm/virt: find_machine_info: handle NULL value 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 2/4] hw/arm/virt: find_machine_info: handle NULL value Alvise Rigo @ 2015-01-05 15:36 ` Peter Maydell 2015-01-05 16:31 ` alvise rigo 0 siblings, 1 reply; 29+ messages in thread From: Peter Maydell @ 2015-01-05 15:36 UTC (permalink / raw) To: Alvise Rigo; +Cc: Rob Herring, tech, Claudio Fontana, QEMU Developers On 21 November 2014 at 18:07, Alvise Rigo <a.rigo@virtualopensystems.com> wrote: > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> > --- > hw/arm/virt.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index e8d527d..4e7b869 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -151,6 +151,10 @@ static VirtBoardInfo *find_machine_info(const char *cpu) > { > int i; > > + if (!cpu) { > + return NULL; > + } > + > for (i = 0; i < ARRAY_SIZE(machines); i++) { > if (strcmp(cpu, machines[i].cpu_model) == 0) { > return &machines[i]; What's the motivation for this change? We can never call this function with a NULL pointer at the moment... thanks -- PMM ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 2/4] hw/arm/virt: find_machine_info: handle NULL value 2015-01-05 15:36 ` Peter Maydell @ 2015-01-05 16:31 ` alvise rigo 2015-01-05 16:42 ` Peter Maydell 0 siblings, 1 reply; 29+ messages in thread From: alvise rigo @ 2015-01-05 16:31 UTC (permalink / raw) To: Peter Maydell; +Cc: Rob Herring, tech, Claudio Fontana, QEMU Developers It's just because in patch 1/4 of this series we use find_machine_info(machine->cpu_model), which could be a NULL pointer. Indeed this patch can be avoided reworking a bit the calling function code. Regards, alvise On Mon, Jan 5, 2015 at 4:36 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 21 November 2014 at 18:07, Alvise Rigo <a.rigo@virtualopensystems.com> wrote: >> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> >> --- >> hw/arm/virt.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index e8d527d..4e7b869 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -151,6 +151,10 @@ static VirtBoardInfo *find_machine_info(const char *cpu) >> { >> int i; >> >> + if (!cpu) { >> + return NULL; >> + } >> + >> for (i = 0; i < ARRAY_SIZE(machines); i++) { >> if (strcmp(cpu, machines[i].cpu_model) == 0) { >> return &machines[i]; > > What's the motivation for this change? We can never call this > function with a NULL pointer at the moment... > > thanks > -- PMM ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 2/4] hw/arm/virt: find_machine_info: handle NULL value 2015-01-05 16:31 ` alvise rigo @ 2015-01-05 16:42 ` Peter Maydell 0 siblings, 0 replies; 29+ messages in thread From: Peter Maydell @ 2015-01-05 16:42 UTC (permalink / raw) To: alvise rigo; +Cc: Rob Herring, tech, Claudio Fontana, QEMU Developers On 5 January 2015 at 16:31, alvise rigo <a.rigo@virtualopensystems.com> wrote: > It's just because in patch 1/4 of this series we use > find_machine_info(machine->cpu_model), which could be a NULL pointer. Well, don't do that, because in that case you'll do the wrong thing if we take the default value of the cpu model. But I don't think patch 1 is the right approach anyway. (Also, if you have a series where patch B requires a change or fix that is put in patch A, then A should come before B in the series.) thanks -- PMM ^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms 2014-11-21 18:07 [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update Alvise Rigo 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt Alvise Rigo 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 2/4] hw/arm/virt: find_machine_info: handle NULL value Alvise Rigo @ 2014-11-21 18:07 ` Alvise Rigo 2014-11-24 10:34 ` Claudio Fontana 2015-01-05 17:13 ` Alexander Graf [not found] ` <1416593261-13751-5-git-send-email-a.rigo@virtualopensystems.com> ` (2 subsequent siblings) 5 siblings, 2 replies; 29+ messages in thread From: Alvise Rigo @ 2014-11-21 18:07 UTC (permalink / raw) To: qemu-devel; +Cc: rob.herring, tech, claudio.fontana, Alvise Rigo, peter.maydell Add a generic PCI host controller for virtual platforms, based on the previous work by Rob Herring: http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html The controller creates a PCI bus for PCI devices; it is intended to receive from the platform all the needed information to initiate the memory regions expected by the PCI system. Due to this dependence, a header file is used to define the structure that the platform has to fill with the data needed by the controller. The device provides also a routine for the device tree node generation, which mostly has to take care of the interrupt-map property generation. This property is fetched by the guest operating system to map any PCI interrupt to the interrupt controller. For the time being, the device expects a GIC v2 to be used by the guest. Only mach-virt has been used to test the controller. Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> --- hw/pci-host/Makefile.objs | 2 +- hw/pci-host/generic-pci.c | 280 ++++++++++++++++++++++++++++++++++++++ include/hw/pci-host/generic-pci.h | 74 ++++++++++ 3 files changed, 355 insertions(+), 1 deletion(-) create mode 100644 hw/pci-host/generic-pci.c create mode 100644 include/hw/pci-host/generic-pci.h diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs index bb65f9c..8ef9fac 100644 --- a/hw/pci-host/Makefile.objs +++ b/hw/pci-host/Makefile.objs @@ -1,4 +1,4 @@ -common-obj-y += pam.o +common-obj-y += pam.o generic-pci.o # PPC devices common-obj-$(CONFIG_PREP_PCI) += prep.o diff --git a/hw/pci-host/generic-pci.c b/hw/pci-host/generic-pci.c new file mode 100644 index 0000000..9c90263 --- /dev/null +++ b/hw/pci-host/generic-pci.c @@ -0,0 +1,280 @@ +/* + * Generic PCI host controller + * + * Copyright (c) 2014 Linaro, Ltd. + * Author: Rob Herring <rob.herring@linaro.org> + * + * Based on ARM Versatile PCI controller (hw/pci-host/versatile.c): + * Copyright (c) 2006-2009 CodeSourcery. + * Written by Paul Brook + * + * This code is licensed under the LGPL. + */ + +#include "hw/sysbus.h" +#include "hw/pci-host/generic-pci.h" +#include "exec/address-spaces.h" +#include "sysemu/device_tree.h" + +static const VMStateDescription pci_generic_host_vmstate = { + .name = "generic-host-pci", + .version_id = 1, + .minimum_version_id = 1, +}; + +static void pci_cam_config_write(void *opaque, hwaddr addr, + uint64_t val, unsigned size) +{ + PCIGenState *s = opaque; + pci_data_write(&s->pci_bus, addr, val, size); +} + +static uint64_t pci_cam_config_read(void *opaque, hwaddr addr, unsigned size) +{ + PCIGenState *s = opaque; + uint32_t val; + val = pci_data_read(&s->pci_bus, addr, size); + return val; +} + +static const MemoryRegionOps pci_vpb_config_ops = { + .read = pci_cam_config_read, + .write = pci_cam_config_write, + .endianness = DEVICE_NATIVE_ENDIAN, +}; + +static void pci_generic_set_irq(void *opaque, int irq_num, int level) +{ + qemu_irq *pic = opaque; + qemu_set_irq(pic[irq_num], level); +} + +static void pci_generic_host_init(Object *obj) +{ + PCIHostState *h = PCI_HOST_BRIDGE(obj); + PCIGenState *s = PCI_GEN(obj); + GenericPCIHostState *gps = &s->pci_gen; + + memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32); + memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32); + + pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci", + &s->pci_mem_space, &s->pci_io_space, + PCI_DEVFN(0, 0), TYPE_PCIE_BUS); + h->bus = &s->pci_bus; + + object_initialize(gps, sizeof(*gps), TYPE_GENERIC_PCI_HOST); + qdev_set_parent_bus(DEVICE(gps), BUS(&s->pci_bus)); + gps->devfns = 0; +} + +static int generic_pci_map_irq_fn(PCIDevice *pci_dev, int pin) +{ + BusState *bus = qdev_get_parent_bus(&pci_dev->qdev); + PCIBus *pci_bus = PCI_BUS(bus); + PCIDevice *pdev = pci_bus->devices[PCI_DEVFN(0, 0)]; + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); + + return gps->irqmap.devfn_idx_map[PCI_SLOT(pci_dev->devfn)] + [PCI_FUNC(pci_dev->devfn)]; +} + +static void pci_generic_host_realize(DeviceState *dev, Error **errp) +{ + PCIGenState *s = PCI_GEN(dev); + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); + GenericPCIPlatformData *pdata = &s->plat_data; + int i; + + if (!pdata->addr_map[PCI_CFG].size || !pdata->addr_map[PCI_IO].size || + !pdata->addr_map[PCI_MEM].size || !pdata->irqs) { + hw_error("generic_pci: PCI controller not fully configured."); + } + + for (i = 0; i < pdata->irqs; i++) { + sysbus_init_irq(sbd, &s->irq[i]); + } + + pci_bus_irqs(&s->pci_bus, pci_generic_set_irq, generic_pci_map_irq_fn, + s->irq, pdata->irqs); + + memory_region_init_io(&s->mem_config, OBJECT(s), &pci_vpb_config_ops, s, + "pci-config", pdata->addr_map[PCI_CFG].size); + sysbus_init_mmio(sbd, &s->mem_config); + + /* Depending on the available memory of the board using the controller, we + create a window on both the I/O and mememory space. */ + memory_region_init_alias(&s->pci_io_window, OBJECT(s), "pci-io-win", + &s->pci_io_space, 0, + pdata->addr_map[PCI_IO].size); + sysbus_init_mmio(sbd, &s->pci_io_window); + + memory_region_init_alias(&s->pci_mem_window, OBJECT(s), "pci-mem-win", + &s->pci_mem_space, + pdata->addr_map[PCI_MEM].addr, + pdata->addr_map[PCI_MEM].size); + sysbus_init_mmio(sbd, &s->pci_mem_window); + + /* TODO Remove once realize propagates to child devices. */ + object_property_set_bool(OBJECT(&s->pci_gen), true, "realized", errp); +} + +static void pci_generic_host_class_init(ObjectClass *klass, void *data) +{ + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); + + k->vendor_id = PCI_VENDOR_ID_REDHAT; + k->device_id = 0x1234; + k->class_id = PCI_CLASS_PROCESSOR_CO; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; +} + +struct dt_irq_mapping { + int irqs; + uint32_t gic_phandle; + int base_irq_num; + uint64_t *data; +}; + +/* */ +#define IRQ_MAP_ENTRY_DESC_SZ 14 +static void gen_int_mapping_fn(PCIBus *b, PCIDevice *d, void *opaque) +{ + struct dt_irq_mapping *map_data; + int pin; + + PCIDevice *pdev = b->devices[PCI_DEVFN(0, 0)]; + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); + map_data = (struct dt_irq_mapping *)opaque; + + /* Check if the platform has enough IRQs available. */ + if (gps->devfns > map_data->irqs) { + hw_error("generic_pci: too many PCI devices."); + } + + pin = pci_get_byte(d->config + PCI_INTERRUPT_PIN); + if (pin) { + uint64_t *data_ptr = map_data->data + (IRQ_MAP_ENTRY_DESC_SZ * gps->devfns); + uint8_t pci_slot = PCI_SLOT(d->devfn); + uint8_t pci_func = PCI_FUNC(d->devfn); + uint8_t *devfn_idx = &gps->irqmap.devfn_idx_map[pci_slot][0]; + uint8_t *devfn_irq = gps->irqmap.devfn_irq_map; + + devfn_idx[pci_func] = gps->devfns; + devfn_irq[gps->devfns] = map_data->base_irq_num + gps->devfns; + + uint64_t buffer[IRQ_MAP_ENTRY_DESC_SZ] = + {1, (pci_slot << 11)|(pci_func << 8), 2, 0x00000000, 1, pin, + 1, map_data->gic_phandle, 1, 0x0, 1, devfn_irq[gps->devfns], + 1, 0x1}; + + memcpy(data_ptr, buffer, IRQ_MAP_ENTRY_DESC_SZ * sizeof(*buffer)); + gps->devfns++; + } +} + +/* Generate the "interrup-map" node's data and store it in map_data */ +static void generate_int_mapping(struct dt_irq_mapping *map_data, + PCIGenState *s) +{ + pci_for_each_device(s->parent_obj.bus, 0, gen_int_mapping_fn, map_data); +} + +static void generate_dt_node(void *fdt, DeviceState *dev) +{ + PCIGenState *s = PCI_GEN(dev); + char *nodename; + uint32_t gic_phandle; + uint32_t plat_acells; + uint32_t plat_scells; + + SysBusDevice *busdev; + busdev = SYS_BUS_DEVICE(dev); + MemoryRegion *cfg = sysbus_mmio_get_region(busdev, 0); + MemoryRegion *io = sysbus_mmio_get_region(busdev, 1); + MemoryRegion *mem = sysbus_mmio_get_region(busdev, 2); + + nodename = g_strdup_printf("/pci@%" PRIx64, cfg->addr); + qemu_fdt_add_subnode(fdt, nodename); + qemu_fdt_setprop_string(fdt, nodename, "compatible", + "pci-host-cam-generic"); + qemu_fdt_setprop_string(fdt, nodename, "device_type", "pci"); + qemu_fdt_setprop_cell(fdt, nodename, "#address-cells", 0x3); + qemu_fdt_setprop_cell(fdt, nodename, "#size-cells", 0x2); + qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 0x1); + + plat_acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells"); + plat_scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells"); + qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", plat_acells, cfg->addr, + plat_scells, memory_region_size(cfg)); + + qemu_fdt_setprop_sized_cells(fdt, nodename, "ranges", + 1, 0x01000000, 2, 0x00000000, /* I/O space */ + 2, io->addr, 2, memory_region_size(io), + 1, 0x02000000, 2, mem->addr, /* 32bit memory space */ + 2, mem->addr, 2, memory_region_size(mem)); + + gic_phandle = qemu_fdt_get_phandle(fdt, s->plat_data.gic_node_name); + /* A mask covering bits [15,8] of phys.high allows to honor the + function number when resolving a triggered PCI interrupt. */ + qemu_fdt_setprop_sized_cells(fdt, nodename, "interrupt-map-mask", + 1, 0xff00, 1, 0x0, 1, 0x0, 1, 0x7); + + uint64_t *int_mapping_data = g_malloc0(IRQ_MAP_ENTRY_DESC_SZ * + sizeof(uint64_t) * s->plat_data.irqs); + struct dt_irq_mapping dt_map_data = { + .irqs = s->plat_data.irqs, + .gic_phandle = gic_phandle, + .base_irq_num = s->plat_data.base_irq, + .data = int_mapping_data + }; + /* Generate the interrupt mapping according to the devices attached + * to the PCI bus of the controller. */ + generate_int_mapping(&dt_map_data, s); + qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "interrupt-map", + (s->pci_gen.devfns * IRQ_MAP_ENTRY_DESC_SZ)/2, int_mapping_data); + + g_free(int_mapping_data); + g_free(nodename); +} + +void pci_controller_build_dt_node(void *fdt, DeviceState *dev) +{ + generate_dt_node(fdt, dev); +} + +static const TypeInfo pci_generic_host_info = { + .name = TYPE_GENERIC_PCI_HOST, + .parent = TYPE_PCI_DEVICE, + .instance_size = sizeof(GenericPCIHostState), + .class_init = pci_generic_host_class_init, +}; + +static void pci_generic_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->realize = pci_generic_host_realize; + dc->vmsd = &pci_generic_host_vmstate; +} + +static const TypeInfo pci_generic_info = { + .name = TYPE_GENERIC_PCI, + .parent = TYPE_PCI_HOST_BRIDGE, + .instance_size = sizeof(PCIGenState), + .instance_init = pci_generic_host_init, + .class_init = pci_generic_class_init, +}; + +static void generic_pci_host_register_types(void) +{ + type_register_static(&pci_generic_info); + type_register_static(&pci_generic_host_info); +} + +type_init(generic_pci_host_register_types) \ No newline at end of file diff --git a/include/hw/pci-host/generic-pci.h b/include/hw/pci-host/generic-pci.h new file mode 100644 index 0000000..43c7a0f --- /dev/null +++ b/include/hw/pci-host/generic-pci.h @@ -0,0 +1,74 @@ +#ifndef QEMU_GENERIC_PCI_H +#define QEMU_GENERIC_PCI_H + +#include "hw/pci/pci.h" +#include "hw/pci/pci_bus.h" +#include "hw/pci/pci_host.h" + +#define MAX_PCI_DEVICES (PCI_SLOT_MAX * PCI_FUNC_MAX) + +enum { + PCI_CFG = 0, + PCI_IO, + PCI_MEM, +}; + +typedef struct { + /* addr_map[PCI_CFG].addr = base address of configuration memory + addr_map[PCI_CFG].size = configuration memory size + ... */ + struct addr_map { + hwaddr addr; + hwaddr size; + } addr_map[3]; + + const char *gic_node_name; + uint32_t base_irq; + uint32_t irqs; +} GenericPCIPlatformData; + +typedef struct { + PCIDevice parent_obj; + + struct irqmap { + /* devfn_idx_map[i][j] = index inside slot_irq_map for device at slot i, + fn j */ + uint8_t devfn_idx_map[PCI_SLOT_MAX][PCI_FUNC_MAX]; + /* devfn_irq_map[i] = IRQ num. of the i-th device (devfn) attached to + the bus */ + uint8_t devfn_irq_map[MAX_PCI_DEVICES]; + } irqmap; + /* number of devices attached to the bus */ + uint32_t devfns; +} GenericPCIHostState; + +typedef struct { + PCIHostState parent_obj; + + qemu_irq irq[MAX_PCI_DEVICES]; + MemoryRegion mem_config; + /* Containers representing the PCI address spaces */ + MemoryRegion pci_io_space; + MemoryRegion pci_mem_space; + /* Alias regions into PCI address spaces which we expose as sysbus regions. + * The offsets into pci_mem_space is fixed. */ + MemoryRegion pci_io_window; + MemoryRegion pci_mem_window; + PCIBus pci_bus; + GenericPCIHostState pci_gen; + + /* Platform dependent data */ + GenericPCIPlatformData plat_data; +} PCIGenState; + +#define TYPE_GENERIC_PCI "generic_pci" +#define PCI_GEN(obj) \ + OBJECT_CHECK(PCIGenState, (obj), TYPE_GENERIC_PCI) + +#define TYPE_GENERIC_PCI_HOST "generic_pci_host" +#define PCI_GEN_HOST(obj) \ + OBJECT_CHECK(GenericPCIHostState, (obj), TYPE_GENERIC_PCI_HOST) + +void pci_controller_build_dt_node(void *fdt, DeviceState *dev); + +#endif \ No newline at end of file -- 2.1.0 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms Alvise Rigo @ 2014-11-24 10:34 ` Claudio Fontana 2014-11-24 14:57 ` alvise rigo 2015-01-05 17:13 ` Alexander Graf 1 sibling, 1 reply; 29+ messages in thread From: Claudio Fontana @ 2014-11-24 10:34 UTC (permalink / raw) To: Alvise Rigo, qemu-devel; +Cc: rob.herring, tech, peter.maydell On 21.11.2014 19:07, Alvise Rigo wrote: > Add a generic PCI host controller for virtual platforms, based on the > previous work by Rob Herring: > http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html Would it not be better to add rob herring's signoff, and then the explanation of what you changed from his patch, followed by your signoff? Or did you change so much that you had to redo the original work by Rob Herring from scratch? > > The controller creates a PCI bus for PCI devices; it is intended to > receive from the platform all the needed information to initiate the > memory regions expected by the PCI system. Due to this dependence, a > header file is used to define the structure that the platform has to > fill with the data needed by the controller. The device provides also a > routine for the device tree node generation, which mostly has to take > care of the interrupt-map property generation. This property is fetched > by the guest operating system to map any PCI interrupt to the interrupt > controller. > For the time being, the device expects a GIC v2 to be used > by the guest. That's a pretty big limitation for a "generic" controller. If it's only about the size of a parent interrupt cell or something like that, why don't we provide it as part of the platform description that you pass to the machinery anyway? > Only mach-virt has been used to test the controller. > > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> > --- > hw/pci-host/Makefile.objs | 2 +- > hw/pci-host/generic-pci.c | 280 ++++++++++++++++++++++++++++++++++++++ > include/hw/pci-host/generic-pci.h | 74 ++++++++++ > 3 files changed, 355 insertions(+), 1 deletion(-) > create mode 100644 hw/pci-host/generic-pci.c > create mode 100644 include/hw/pci-host/generic-pci.h > > diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs > index bb65f9c..8ef9fac 100644 > --- a/hw/pci-host/Makefile.objs > +++ b/hw/pci-host/Makefile.objs > @@ -1,4 +1,4 @@ > -common-obj-y += pam.o > +common-obj-y += pam.o generic-pci.o > > # PPC devices > common-obj-$(CONFIG_PREP_PCI) += prep.o > diff --git a/hw/pci-host/generic-pci.c b/hw/pci-host/generic-pci.c > new file mode 100644 > index 0000000..9c90263 > --- /dev/null > +++ b/hw/pci-host/generic-pci.c > @@ -0,0 +1,280 @@ > +/* > + * Generic PCI host controller > + * > + * Copyright (c) 2014 Linaro, Ltd. > + * Author: Rob Herring <rob.herring@linaro.org> > + * > + * Based on ARM Versatile PCI controller (hw/pci-host/versatile.c): > + * Copyright (c) 2006-2009 CodeSourcery. > + * Written by Paul Brook > + * > + * This code is licensed under the LGPL. > + */ > + > +#include "hw/sysbus.h" > +#include "hw/pci-host/generic-pci.h" > +#include "exec/address-spaces.h" > +#include "sysemu/device_tree.h" > + > +static const VMStateDescription pci_generic_host_vmstate = { > + .name = "generic-host-pci", > + .version_id = 1, > + .minimum_version_id = 1, > +}; > + > +static void pci_cam_config_write(void *opaque, hwaddr addr, > + uint64_t val, unsigned size) > +{ I would add a comment: /* the MemoryRegionOps require uint64_t val, but we can only do 32bit */ there are already asserts in pci_host.c, so that should be sufficient. > + PCIGenState *s = opaque; > + pci_data_write(&s->pci_bus, addr, val, size); > +} > + > +static uint64_t pci_cam_config_read(void *opaque, hwaddr addr, unsigned size) /* same here */ > +{ > + PCIGenState *s = opaque; > + uint32_t val; > + val = pci_data_read(&s->pci_bus, addr, size); > + return val; > +} > + > +static const MemoryRegionOps pci_vpb_config_ops = { > + .read = pci_cam_config_read, > + .write = pci_cam_config_write, > + .endianness = DEVICE_NATIVE_ENDIAN, > +}; > + > +static void pci_generic_set_irq(void *opaque, int irq_num, int level) > +{ > + qemu_irq *pic = opaque; > + qemu_set_irq(pic[irq_num], level); > +} > + > +static void pci_generic_host_init(Object *obj) > +{ > + PCIHostState *h = PCI_HOST_BRIDGE(obj); > + PCIGenState *s = PCI_GEN(obj); > + GenericPCIHostState *gps = &s->pci_gen; > + > + memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32); > + memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32); > + > + pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci", > + &s->pci_mem_space, &s->pci_io_space, > + PCI_DEVFN(0, 0), TYPE_PCIE_BUS); TYPE_PCI_BUS, until we actually support PCIE. > + h->bus = &s->pci_bus; > + > + object_initialize(gps, sizeof(*gps), TYPE_GENERIC_PCI_HOST); > + qdev_set_parent_bus(DEVICE(gps), BUS(&s->pci_bus)); > + gps->devfns = 0; > +} > + > +static int generic_pci_map_irq_fn(PCIDevice *pci_dev, int pin) > +{ > + BusState *bus = qdev_get_parent_bus(&pci_dev->qdev); > + PCIBus *pci_bus = PCI_BUS(bus); > + PCIDevice *pdev = pci_bus->devices[PCI_DEVFN(0, 0)]; > + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); > + > + return gps->irqmap.devfn_idx_map[PCI_SLOT(pci_dev->devfn)] > + [PCI_FUNC(pci_dev->devfn)]; > +} > + > +static void pci_generic_host_realize(DeviceState *dev, Error **errp) > +{ > + PCIGenState *s = PCI_GEN(dev); > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > + GenericPCIPlatformData *pdata = &s->plat_data; > + int i; > + > + if (!pdata->addr_map[PCI_CFG].size || !pdata->addr_map[PCI_IO].size || > + !pdata->addr_map[PCI_MEM].size || !pdata->irqs) { > + hw_error("generic_pci: PCI controller not fully configured."); > + } > + > + for (i = 0; i < pdata->irqs; i++) { > + sysbus_init_irq(sbd, &s->irq[i]); > + } > + > + pci_bus_irqs(&s->pci_bus, pci_generic_set_irq, generic_pci_map_irq_fn, > + s->irq, pdata->irqs); > + > + memory_region_init_io(&s->mem_config, OBJECT(s), &pci_vpb_config_ops, s, > + "pci-config", pdata->addr_map[PCI_CFG].size); > + sysbus_init_mmio(sbd, &s->mem_config); > + > + /* Depending on the available memory of the board using the controller, we > + create a window on both the I/O and mememory space. */ s/mememory/memory/ > + memory_region_init_alias(&s->pci_io_window, OBJECT(s), "pci-io-win", > + &s->pci_io_space, 0, > + pdata->addr_map[PCI_IO].size); > + sysbus_init_mmio(sbd, &s->pci_io_window); > + > + memory_region_init_alias(&s->pci_mem_window, OBJECT(s), "pci-mem-win", > + &s->pci_mem_space, > + pdata->addr_map[PCI_MEM].addr, > + pdata->addr_map[PCI_MEM].size); > + sysbus_init_mmio(sbd, &s->pci_mem_window); > + > + /* TODO Remove once realize propagates to child devices. */ > + object_property_set_bool(OBJECT(&s->pci_gen), true, "realized", errp); > +} > + > +static void pci_generic_host_class_init(ObjectClass *klass, void *data) > +{ > + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + k->vendor_id = PCI_VENDOR_ID_REDHAT; > + k->device_id = 0x1234; > + k->class_id = PCI_CLASS_PROCESSOR_CO; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > +} > + > +struct dt_irq_mapping { > + int irqs; > + uint32_t gic_phandle; cannot this be generic enough that we don't talk about gic here? > + int base_irq_num; > + uint64_t *data; > +}; > + > +/* */ ? Can you remove this empty comment ? > +#define IRQ_MAP_ENTRY_DESC_SZ 14 > +static void gen_int_mapping_fn(PCIBus *b, PCIDevice *d, void *opaque) > +{ > + struct dt_irq_mapping *map_data; > + int pin; > + > + PCIDevice *pdev = b->devices[PCI_DEVFN(0, 0)]; > + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); > + map_data = (struct dt_irq_mapping *)opaque; > + > + /* Check if the platform has enough IRQs available. */ > + if (gps->devfns > map_data->irqs) { > + hw_error("generic_pci: too many PCI devices."); > + } > + > + pin = pci_get_byte(d->config + PCI_INTERRUPT_PIN); > + if (pin) { > + uint64_t *data_ptr = map_data->data + (IRQ_MAP_ENTRY_DESC_SZ * gps->devfns); > + uint8_t pci_slot = PCI_SLOT(d->devfn); > + uint8_t pci_func = PCI_FUNC(d->devfn); > + uint8_t *devfn_idx = &gps->irqmap.devfn_idx_map[pci_slot][0]; > + uint8_t *devfn_irq = gps->irqmap.devfn_irq_map; > + > + devfn_idx[pci_func] = gps->devfns; > + devfn_irq[gps->devfns] = map_data->base_irq_num + gps->devfns; > + > + uint64_t buffer[IRQ_MAP_ENTRY_DESC_SZ] = > + {1, (pci_slot << 11)|(pci_func << 8), 2, 0x00000000, 1, pin, > + 1, map_data->gic_phandle, 1, 0x0, 1, devfn_irq[gps->devfns], > + 1, 0x1}; ah now I see, I think you forgot a comment above. But maybe here is a better place. The above needs to be commented heavily, to make it obvious what each field is, and which "spec" this is following. You have mentioned already in the previous discussion, but it needs to be in the code. > + > + memcpy(data_ptr, buffer, IRQ_MAP_ENTRY_DESC_SZ * sizeof(*buffer)); > + gps->devfns++; > + } > +} > + > +/* Generate the "interrup-map" node's data and store it in map_data */ interrupt-map > +static void generate_int_mapping(struct dt_irq_mapping *map_data, > + PCIGenState *s) > +{ > + pci_for_each_device(s->parent_obj.bus, 0, gen_int_mapping_fn, map_data); > +} > + > +static void generate_dt_node(void *fdt, DeviceState *dev) > +{ > + PCIGenState *s = PCI_GEN(dev); > + char *nodename; > + uint32_t gic_phandle; > + uint32_t plat_acells; > + uint32_t plat_scells; > + > + SysBusDevice *busdev; > + busdev = SYS_BUS_DEVICE(dev); > + MemoryRegion *cfg = sysbus_mmio_get_region(busdev, 0); > + MemoryRegion *io = sysbus_mmio_get_region(busdev, 1); > + MemoryRegion *mem = sysbus_mmio_get_region(busdev, 2); > + > + nodename = g_strdup_printf("/pci@%" PRIx64, cfg->addr); > + qemu_fdt_add_subnode(fdt, nodename); > + qemu_fdt_setprop_string(fdt, nodename, "compatible", > + "pci-host-cam-generic"); > + qemu_fdt_setprop_string(fdt, nodename, "device_type", "pci"); > + qemu_fdt_setprop_cell(fdt, nodename, "#address-cells", 0x3); > + qemu_fdt_setprop_cell(fdt, nodename, "#size-cells", 0x2); > + qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 0x1); > + > + plat_acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells"); > + plat_scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells"); > + qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", plat_acells, cfg->addr, > + plat_scells, memory_region_size(cfg)); > + > + qemu_fdt_setprop_sized_cells(fdt, nodename, "ranges", > + 1, 0x01000000, 2, 0x00000000, /* I/O space */ > + 2, io->addr, 2, memory_region_size(io), > + 1, 0x02000000, 2, mem->addr, /* 32bit memory space */ Is it commented somewhere sensible that the generic pci has this layout: cfg = whatever base address io = base + 0x1000000 mem = base + 0x2000000 > + 2, mem->addr, 2, memory_region_size(mem)); > + > + gic_phandle = qemu_fdt_get_phandle(fdt, s->plat_data.gic_node_name); hmm could it be gic-independent? > + /* A mask covering bits [15,8] of phys.high allows to honor the > + function number when resolving a triggered PCI interrupt. */ > + qemu_fdt_setprop_sized_cells(fdt, nodename, "interrupt-map-mask", > + 1, 0xff00, 1, 0x0, 1, 0x0, 1, 0x7); and what about 0, 0, 7, what do they mean? How does the mapping work, what do all the fields mean, and which paper/spec are you implementing?.. > + > + uint64_t *int_mapping_data = g_malloc0(IRQ_MAP_ENTRY_DESC_SZ * > + sizeof(uint64_t) * s->plat_data.irqs); > + struct dt_irq_mapping dt_map_data = { > + .irqs = s->plat_data.irqs, > + .gic_phandle = gic_phandle, could this be gic independent? > + .base_irq_num = s->plat_data.base_irq, > + .data = int_mapping_data > + }; > + /* Generate the interrupt mapping according to the devices attached > + * to the PCI bus of the controller. */ > + generate_int_mapping(&dt_map_data, s); > + qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "interrupt-map", > + (s->pci_gen.devfns * IRQ_MAP_ENTRY_DESC_SZ)/2, int_mapping_data); > + > + g_free(int_mapping_data); > + g_free(nodename); > +} > + > +void pci_controller_build_dt_node(void *fdt, DeviceState *dev) > +{ > + generate_dt_node(fdt, dev); > +} > + > +static const TypeInfo pci_generic_host_info = { > + .name = TYPE_GENERIC_PCI_HOST, > + .parent = TYPE_PCI_DEVICE, > + .instance_size = sizeof(GenericPCIHostState), > + .class_init = pci_generic_host_class_init, > +}; > + > +static void pci_generic_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->realize = pci_generic_host_realize; > + dc->vmsd = &pci_generic_host_vmstate; > +} > + > +static const TypeInfo pci_generic_info = { > + .name = TYPE_GENERIC_PCI, > + .parent = TYPE_PCI_HOST_BRIDGE, > + .instance_size = sizeof(PCIGenState), > + .instance_init = pci_generic_host_init, > + .class_init = pci_generic_class_init, > +}; > + > +static void generic_pci_host_register_types(void) > +{ > + type_register_static(&pci_generic_info); > + type_register_static(&pci_generic_host_info); > +} > + > +type_init(generic_pci_host_register_types) > \ No newline at end of file > diff --git a/include/hw/pci-host/generic-pci.h b/include/hw/pci-host/generic-pci.h > new file mode 100644 > index 0000000..43c7a0f > --- /dev/null > +++ b/include/hw/pci-host/generic-pci.h > @@ -0,0 +1,74 @@ > +#ifndef QEMU_GENERIC_PCI_H > +#define QEMU_GENERIC_PCI_H > + > +#include "hw/pci/pci.h" > +#include "hw/pci/pci_bus.h" > +#include "hw/pci/pci_host.h" > + > +#define MAX_PCI_DEVICES (PCI_SLOT_MAX * PCI_FUNC_MAX) > + maybe this would be a good place to talk about what the address space layout assumptions of generic pci are, like /* The Configuration space starts at the PCI base address passed in the virtual platform * information, * The I/O space starts at an offset of 0x1000000 from the PCI base address * The Mem space starts at an offset of 0x2000000 from the PCI base address */ these the assumptions right? > +enum { > + PCI_CFG = 0, > + PCI_IO, > + PCI_MEM, > +}; > + > +typedef struct { > + /* addr_map[PCI_CFG].addr = base address of configuration memory > + addr_map[PCI_CFG].size = configuration memory size > + ... */ > + struct addr_map { > + hwaddr addr; > + hwaddr size; > + } addr_map[3]; > + > + const char *gic_node_name; > + uint32_t base_irq; > + uint32_t irqs; > +} GenericPCIPlatformData; > + > +typedef struct { > + PCIDevice parent_obj; > + > + struct irqmap { > + /* devfn_idx_map[i][j] = index inside slot_irq_map for device at slot i, > + fn j */ > + uint8_t devfn_idx_map[PCI_SLOT_MAX][PCI_FUNC_MAX]; > + /* devfn_irq_map[i] = IRQ num. of the i-th device (devfn) attached to > + the bus */ > + uint8_t devfn_irq_map[MAX_PCI_DEVICES]; > + } irqmap; > + /* number of devices attached to the bus */ > + uint32_t devfns; > +} GenericPCIHostState; > + > +typedef struct { > + PCIHostState parent_obj; > + > + qemu_irq irq[MAX_PCI_DEVICES]; > + MemoryRegion mem_config; > + /* Containers representing the PCI address spaces */ > + MemoryRegion pci_io_space; > + MemoryRegion pci_mem_space; > + /* Alias regions into PCI address spaces which we expose as sysbus regions. > + * The offsets into pci_mem_space is fixed. */ > + MemoryRegion pci_io_window; > + MemoryRegion pci_mem_window; > + PCIBus pci_bus; > + GenericPCIHostState pci_gen; > + > + /* Platform dependent data */ > + GenericPCIPlatformData plat_data; > +} PCIGenState; > + > +#define TYPE_GENERIC_PCI "generic_pci" > +#define PCI_GEN(obj) \ > + OBJECT_CHECK(PCIGenState, (obj), TYPE_GENERIC_PCI) > + > +#define TYPE_GENERIC_PCI_HOST "generic_pci_host" > +#define PCI_GEN_HOST(obj) \ > + OBJECT_CHECK(GenericPCIHostState, (obj), TYPE_GENERIC_PCI_HOST) > + > +void pci_controller_build_dt_node(void *fdt, DeviceState *dev); > + > +#endif > \ No newline at end of file > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms 2014-11-24 10:34 ` Claudio Fontana @ 2014-11-24 14:57 ` alvise rigo 2014-11-25 10:20 ` Claudio Fontana 0 siblings, 1 reply; 29+ messages in thread From: alvise rigo @ 2014-11-24 14:57 UTC (permalink / raw) To: Claudio Fontana Cc: Rob Herring, VirtualOpenSystems Technical Team, QEMU Developers, Peter Maydell Hi Claudio, Thank you for your review. Please see my in-line comments. On Mon, Nov 24, 2014 at 11:34 AM, Claudio Fontana <claudio.fontana@huawei.com> wrote: > On 21.11.2014 19:07, Alvise Rigo wrote: >> Add a generic PCI host controller for virtual platforms, based on the >> previous work by Rob Herring: >> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html > > Would it not be better to add rob herring's signoff, and then the explanation > of what you changed from his patch, followed by your signoff? > Or did you change so much that you had to redo the original work by Rob Herring > from scratch? It was actually difficult to create some meaningful patches over the initial ones. It's sure that for a final version these details will be properly addressed (signoff and diff over the original patches if possible). > >> >> The controller creates a PCI bus for PCI devices; it is intended to >> receive from the platform all the needed information to initiate the >> memory regions expected by the PCI system. Due to this dependence, a >> header file is used to define the structure that the platform has to >> fill with the data needed by the controller. The device provides also a >> routine for the device tree node generation, which mostly has to take >> care of the interrupt-map property generation. This property is fetched >> by the guest operating system to map any PCI interrupt to the interrupt >> controller. > >> For the time being, the device expects a GIC v2 to be used >> by the guest. > > That's a pretty big limitation for a "generic" controller. > If it's only about the size of a parent interrupt cell or something like > that, why don't we provide it as part of the platform description that > you pass to the machinery anyway? Yes we can, however being totally interrupt controller independent means that the platform has to fully provide the way to describe a parent interrupt. For the time being, we use a description compatible with GICv2 (and v3), to cover the use case mach-virt + generic-pci (this is why I also called the interrupt specific structures "gic*"). If we really need to be more general, I can see two solutions: the first one is to find a way to pass all the necessary interrupt controller information to the device machinery (phandle, #cells, interrupt number, etc.). I don't know how such a solution could get complicated in the future, with some other virt-platform that requires a complicated interrupt mapping for example. The second would be to move all the device node generation back to the platform, making its code even more crowded. Are there other solutions that I'm missing? > >> Only mach-virt has been used to test the controller. >> >> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> >> --- >> hw/pci-host/Makefile.objs | 2 +- >> hw/pci-host/generic-pci.c | 280 ++++++++++++++++++++++++++++++++++++++ >> include/hw/pci-host/generic-pci.h | 74 ++++++++++ >> 3 files changed, 355 insertions(+), 1 deletion(-) >> create mode 100644 hw/pci-host/generic-pci.c >> create mode 100644 include/hw/pci-host/generic-pci.h >> >> diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs >> index bb65f9c..8ef9fac 100644 >> --- a/hw/pci-host/Makefile.objs >> +++ b/hw/pci-host/Makefile.objs >> @@ -1,4 +1,4 @@ >> -common-obj-y += pam.o >> +common-obj-y += pam.o generic-pci.o >> >> # PPC devices >> common-obj-$(CONFIG_PREP_PCI) += prep.o >> diff --git a/hw/pci-host/generic-pci.c b/hw/pci-host/generic-pci.c >> new file mode 100644 >> index 0000000..9c90263 >> --- /dev/null >> +++ b/hw/pci-host/generic-pci.c >> @@ -0,0 +1,280 @@ >> +/* >> + * Generic PCI host controller >> + * >> + * Copyright (c) 2014 Linaro, Ltd. >> + * Author: Rob Herring <rob.herring@linaro.org> >> + * >> + * Based on ARM Versatile PCI controller (hw/pci-host/versatile.c): >> + * Copyright (c) 2006-2009 CodeSourcery. >> + * Written by Paul Brook >> + * >> + * This code is licensed under the LGPL. >> + */ >> + >> +#include "hw/sysbus.h" >> +#include "hw/pci-host/generic-pci.h" >> +#include "exec/address-spaces.h" >> +#include "sysemu/device_tree.h" >> + >> +static const VMStateDescription pci_generic_host_vmstate = { >> + .name = "generic-host-pci", >> + .version_id = 1, >> + .minimum_version_id = 1, >> +}; >> + >> +static void pci_cam_config_write(void *opaque, hwaddr addr, >> + uint64_t val, unsigned size) >> +{ > > I would add a comment: > /* the MemoryRegionOps require uint64_t val, but we can only do 32bit */ > > there are already asserts in pci_host.c, so that should be sufficient. Agreed. > >> + PCIGenState *s = opaque; >> + pci_data_write(&s->pci_bus, addr, val, size); >> +} >> + >> +static uint64_t pci_cam_config_read(void *opaque, hwaddr addr, unsigned size) > > /* same here */ > >> +{ >> + PCIGenState *s = opaque; >> + uint32_t val; >> + val = pci_data_read(&s->pci_bus, addr, size); >> + return val; >> +} >> + >> +static const MemoryRegionOps pci_vpb_config_ops = { >> + .read = pci_cam_config_read, >> + .write = pci_cam_config_write, >> + .endianness = DEVICE_NATIVE_ENDIAN, >> +}; >> + >> +static void pci_generic_set_irq(void *opaque, int irq_num, int level) >> +{ >> + qemu_irq *pic = opaque; >> + qemu_set_irq(pic[irq_num], level); >> +} >> + >> +static void pci_generic_host_init(Object *obj) >> +{ >> + PCIHostState *h = PCI_HOST_BRIDGE(obj); >> + PCIGenState *s = PCI_GEN(obj); >> + GenericPCIHostState *gps = &s->pci_gen; >> + >> + memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32); >> + memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32); >> + >> + pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci", >> + &s->pci_mem_space, &s->pci_io_space, >> + PCI_DEVFN(0, 0), TYPE_PCIE_BUS); > > TYPE_PCI_BUS, until we actually support PCIE. This was present in the first RFC as well since harmless, but I will make it PCI only. > >> + h->bus = &s->pci_bus; >> + >> + object_initialize(gps, sizeof(*gps), TYPE_GENERIC_PCI_HOST); >> + qdev_set_parent_bus(DEVICE(gps), BUS(&s->pci_bus)); >> + gps->devfns = 0; >> +} >> + >> +static int generic_pci_map_irq_fn(PCIDevice *pci_dev, int pin) >> +{ >> + BusState *bus = qdev_get_parent_bus(&pci_dev->qdev); >> + PCIBus *pci_bus = PCI_BUS(bus); >> + PCIDevice *pdev = pci_bus->devices[PCI_DEVFN(0, 0)]; >> + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); >> + >> + return gps->irqmap.devfn_idx_map[PCI_SLOT(pci_dev->devfn)] >> + [PCI_FUNC(pci_dev->devfn)]; >> +} >> + >> +static void pci_generic_host_realize(DeviceState *dev, Error **errp) >> +{ >> + PCIGenState *s = PCI_GEN(dev); >> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >> + GenericPCIPlatformData *pdata = &s->plat_data; >> + int i; >> + >> + if (!pdata->addr_map[PCI_CFG].size || !pdata->addr_map[PCI_IO].size || >> + !pdata->addr_map[PCI_MEM].size || !pdata->irqs) { >> + hw_error("generic_pci: PCI controller not fully configured."); >> + } >> + >> + for (i = 0; i < pdata->irqs; i++) { >> + sysbus_init_irq(sbd, &s->irq[i]); >> + } >> + >> + pci_bus_irqs(&s->pci_bus, pci_generic_set_irq, generic_pci_map_irq_fn, >> + s->irq, pdata->irqs); >> + >> + memory_region_init_io(&s->mem_config, OBJECT(s), &pci_vpb_config_ops, s, >> + "pci-config", pdata->addr_map[PCI_CFG].size); >> + sysbus_init_mmio(sbd, &s->mem_config); >> + >> + /* Depending on the available memory of the board using the controller, we >> + create a window on both the I/O and mememory space. */ > > s/mememory/memory/ ACK. > >> + memory_region_init_alias(&s->pci_io_window, OBJECT(s), "pci-io-win", >> + &s->pci_io_space, 0, >> + pdata->addr_map[PCI_IO].size); >> + sysbus_init_mmio(sbd, &s->pci_io_window); >> + >> + memory_region_init_alias(&s->pci_mem_window, OBJECT(s), "pci-mem-win", >> + &s->pci_mem_space, >> + pdata->addr_map[PCI_MEM].addr, >> + pdata->addr_map[PCI_MEM].size); >> + sysbus_init_mmio(sbd, &s->pci_mem_window); >> + >> + /* TODO Remove once realize propagates to child devices. */ >> + object_property_set_bool(OBJECT(&s->pci_gen), true, "realized", errp); >> +} >> + >> +static void pci_generic_host_class_init(ObjectClass *klass, void *data) >> +{ >> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + k->vendor_id = PCI_VENDOR_ID_REDHAT; >> + k->device_id = 0x1234; >> + k->class_id = PCI_CLASS_PROCESSOR_CO; >> + /* >> + * PCI-facing part of the host bridge, not usable without the >> + * host-facing part, which can't be device_add'ed, yet. >> + */ >> + dc->cannot_instantiate_with_device_add_yet = true; >> +} >> + >> +struct dt_irq_mapping { >> + int irqs; >> + uint32_t gic_phandle; > > cannot this be generic enough that we don't talk about gic here? Yes. I will change it. > >> + int base_irq_num; >> + uint64_t *data; >> +}; >> + >> +/* */ > > ? Can you remove this empty comment ? Of course, I missed it. > > >> +#define IRQ_MAP_ENTRY_DESC_SZ 14 >> +static void gen_int_mapping_fn(PCIBus *b, PCIDevice *d, void *opaque) >> +{ >> + struct dt_irq_mapping *map_data; >> + int pin; >> + >> + PCIDevice *pdev = b->devices[PCI_DEVFN(0, 0)]; >> + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); >> + map_data = (struct dt_irq_mapping *)opaque; >> + >> + /* Check if the platform has enough IRQs available. */ >> + if (gps->devfns > map_data->irqs) { >> + hw_error("generic_pci: too many PCI devices."); >> + } >> + >> + pin = pci_get_byte(d->config + PCI_INTERRUPT_PIN); >> + if (pin) { >> + uint64_t *data_ptr = map_data->data + (IRQ_MAP_ENTRY_DESC_SZ * gps->devfns); >> + uint8_t pci_slot = PCI_SLOT(d->devfn); >> + uint8_t pci_func = PCI_FUNC(d->devfn); >> + uint8_t *devfn_idx = &gps->irqmap.devfn_idx_map[pci_slot][0]; >> + uint8_t *devfn_irq = gps->irqmap.devfn_irq_map; >> + >> + devfn_idx[pci_func] = gps->devfns; >> + devfn_irq[gps->devfns] = map_data->base_irq_num + gps->devfns; >> + >> + uint64_t buffer[IRQ_MAP_ENTRY_DESC_SZ] = >> + {1, (pci_slot << 11)|(pci_func << 8), 2, 0x00000000, 1, pin, >> + 1, map_data->gic_phandle, 1, 0x0, 1, devfn_irq[gps->devfns], >> + 1, 0x1}; > > ah now I see, I think you forgot a comment above. But maybe here is a better place. > The above needs to be commented heavily, to make it obvious what each field is, > and which "spec" this is following. You have mentioned already in the previous discussion, > but it needs to be in the code. I will do it. > >> + >> + memcpy(data_ptr, buffer, IRQ_MAP_ENTRY_DESC_SZ * sizeof(*buffer)); >> + gps->devfns++; >> + } >> +} >> + >> +/* Generate the "interrup-map" node's data and store it in map_data */ > > interrupt-map ACK. > >> +static void generate_int_mapping(struct dt_irq_mapping *map_data, >> + PCIGenState *s) >> +{ >> + pci_for_each_device(s->parent_obj.bus, 0, gen_int_mapping_fn, map_data); >> +} >> + >> +static void generate_dt_node(void *fdt, DeviceState *dev) >> +{ >> + PCIGenState *s = PCI_GEN(dev); >> + char *nodename; >> + uint32_t gic_phandle; >> + uint32_t plat_acells; >> + uint32_t plat_scells; >> + >> + SysBusDevice *busdev; >> + busdev = SYS_BUS_DEVICE(dev); >> + MemoryRegion *cfg = sysbus_mmio_get_region(busdev, 0); >> + MemoryRegion *io = sysbus_mmio_get_region(busdev, 1); >> + MemoryRegion *mem = sysbus_mmio_get_region(busdev, 2); >> + >> + nodename = g_strdup_printf("/pci@%" PRIx64, cfg->addr); >> + qemu_fdt_add_subnode(fdt, nodename); >> + qemu_fdt_setprop_string(fdt, nodename, "compatible", >> + "pci-host-cam-generic"); >> + qemu_fdt_setprop_string(fdt, nodename, "device_type", "pci"); >> + qemu_fdt_setprop_cell(fdt, nodename, "#address-cells", 0x3); >> + qemu_fdt_setprop_cell(fdt, nodename, "#size-cells", 0x2); >> + qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 0x1); >> + >> + plat_acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells"); >> + plat_scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells"); >> + qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", plat_acells, cfg->addr, >> + plat_scells, memory_region_size(cfg)); >> + >> + qemu_fdt_setprop_sized_cells(fdt, nodename, "ranges", >> + 1, 0x01000000, 2, 0x00000000, /* I/O space */ >> + 2, io->addr, 2, memory_region_size(io), >> + 1, 0x02000000, 2, mem->addr, /* 32bit memory space */ > > Is it commented somewhere sensible that the generic pci has this layout: > > cfg = whatever base address > io = base + 0x1000000 > mem = base + 0x2000000 The value 0x1000000 actually means that the following address has to be interpreted as I/O address, while 0x2000000 means that it's a 32bit address. I will add some more documentation here, for now I point to page 4 of the following document where the whole encoding is explained: http://www.o3one.org/hwdocs/openfirmware/pci_supplement_2_1.pdf All the documentation should also be available at the website http://www.openfirmware.org (as pointed also by the kernel documentation file Documentation/devicetree/bindings/pci/pci.txt), however the domain does not seem active. > >> + 2, mem->addr, 2, memory_region_size(mem)); >> + >> + gic_phandle = qemu_fdt_get_phandle(fdt, s->plat_data.gic_node_name); > > hmm could it be gic-independent? > >> + /* A mask covering bits [15,8] of phys.high allows to honor the >> + function number when resolving a triggered PCI interrupt. */ >> + qemu_fdt_setprop_sized_cells(fdt, nodename, "interrupt-map-mask", >> + 1, 0xff00, 1, 0x0, 1, 0x0, 1, 0x7); > > and what about 0, 0, 7, what do they mean? How does the mapping work, > what do all the fields mean, and which paper/spec are you implementing?.. 0, 0 are needed to mask out the phys.mid and hys.low bits of the PCI address (since are not needed to resolve the interrupt). I will add a more detailed documentation in the next version. > >> + >> + uint64_t *int_mapping_data = g_malloc0(IRQ_MAP_ENTRY_DESC_SZ * >> + sizeof(uint64_t) * s->plat_data.irqs); >> + struct dt_irq_mapping dt_map_data = { >> + .irqs = s->plat_data.irqs, >> + .gic_phandle = gic_phandle, > > could this be gic independent? Yes, but still we need a field necessary to store the interrupt controller phandle. > >> + .base_irq_num = s->plat_data.base_irq, >> + .data = int_mapping_data >> + }; >> + /* Generate the interrupt mapping according to the devices attached >> + * to the PCI bus of the controller. */ >> + generate_int_mapping(&dt_map_data, s); >> + qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "interrupt-map", >> + (s->pci_gen.devfns * IRQ_MAP_ENTRY_DESC_SZ)/2, int_mapping_data); >> + >> + g_free(int_mapping_data); >> + g_free(nodename); >> +} >> + >> +void pci_controller_build_dt_node(void *fdt, DeviceState *dev) >> +{ >> + generate_dt_node(fdt, dev); >> +} >> + >> +static const TypeInfo pci_generic_host_info = { >> + .name = TYPE_GENERIC_PCI_HOST, >> + .parent = TYPE_PCI_DEVICE, >> + .instance_size = sizeof(GenericPCIHostState), >> + .class_init = pci_generic_host_class_init, >> +}; >> + >> +static void pci_generic_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->realize = pci_generic_host_realize; >> + dc->vmsd = &pci_generic_host_vmstate; >> +} >> + >> +static const TypeInfo pci_generic_info = { >> + .name = TYPE_GENERIC_PCI, >> + .parent = TYPE_PCI_HOST_BRIDGE, >> + .instance_size = sizeof(PCIGenState), >> + .instance_init = pci_generic_host_init, >> + .class_init = pci_generic_class_init, >> +}; >> + >> +static void generic_pci_host_register_types(void) >> +{ >> + type_register_static(&pci_generic_info); >> + type_register_static(&pci_generic_host_info); >> +} >> + >> +type_init(generic_pci_host_register_types) >> \ No newline at end of file >> diff --git a/include/hw/pci-host/generic-pci.h b/include/hw/pci-host/generic-pci.h >> new file mode 100644 >> index 0000000..43c7a0f >> --- /dev/null >> +++ b/include/hw/pci-host/generic-pci.h >> @@ -0,0 +1,74 @@ >> +#ifndef QEMU_GENERIC_PCI_H >> +#define QEMU_GENERIC_PCI_H >> + >> +#include "hw/pci/pci.h" >> +#include "hw/pci/pci_bus.h" >> +#include "hw/pci/pci_host.h" >> + >> +#define MAX_PCI_DEVICES (PCI_SLOT_MAX * PCI_FUNC_MAX) >> + > > maybe this would be a good place to talk about what the address space > layout assumptions of generic pci are, like > > /* The Configuration space starts at the PCI base address passed in the virtual platform > * information, > * The I/O space starts at an offset of 0x1000000 from the PCI base address > * The Mem space starts at an offset of 0x2000000 from the PCI base address > */ > > these the assumptions right? Actually this is not completely true. As explained before, 0x01000000 and 0x02000000 indicate an I/O region and a 32bit addressable region. The I/O window (pci_io_window) starts at the beginning of the I/O address space (pci_io_space), no matter where it is mapped to by the platform. For example, mach-virt maps it to the CPU address 0x11000000 but from the PCI bus point of view, it starts at address 0x0. For the memory window (pci_mem_window) instead we need to place it at the same address in the memory space (mem_io_space) to which it has been mapped by the platform. In mach-virt, this region starts at CPU (and also PCI) address 0x12000000. I hope to have been enough clear. Regards, alvise > >> +enum { >> + PCI_CFG = 0, >> + PCI_IO, >> + PCI_MEM, >> +}; >> + >> +typedef struct { >> + /* addr_map[PCI_CFG].addr = base address of configuration memory >> + addr_map[PCI_CFG].size = configuration memory size >> + ... */ >> + struct addr_map { >> + hwaddr addr; >> + hwaddr size; >> + } addr_map[3]; >> + >> + const char *gic_node_name; >> + uint32_t base_irq; >> + uint32_t irqs; >> +} GenericPCIPlatformData; >> + >> +typedef struct { >> + PCIDevice parent_obj; >> + >> + struct irqmap { >> + /* devfn_idx_map[i][j] = index inside slot_irq_map for device at slot i, >> + fn j */ >> + uint8_t devfn_idx_map[PCI_SLOT_MAX][PCI_FUNC_MAX]; >> + /* devfn_irq_map[i] = IRQ num. of the i-th device (devfn) attached to >> + the bus */ >> + uint8_t devfn_irq_map[MAX_PCI_DEVICES]; >> + } irqmap; >> + /* number of devices attached to the bus */ >> + uint32_t devfns; >> +} GenericPCIHostState; >> + >> +typedef struct { >> + PCIHostState parent_obj; >> + >> + qemu_irq irq[MAX_PCI_DEVICES]; >> + MemoryRegion mem_config; >> + /* Containers representing the PCI address spaces */ >> + MemoryRegion pci_io_space; >> + MemoryRegion pci_mem_space; >> + /* Alias regions into PCI address spaces which we expose as sysbus regions. >> + * The offsets into pci_mem_space is fixed. */ >> + MemoryRegion pci_io_window; >> + MemoryRegion pci_mem_window; >> + PCIBus pci_bus; >> + GenericPCIHostState pci_gen; >> + >> + /* Platform dependent data */ >> + GenericPCIPlatformData plat_data; >> +} PCIGenState; >> + >> +#define TYPE_GENERIC_PCI "generic_pci" >> +#define PCI_GEN(obj) \ >> + OBJECT_CHECK(PCIGenState, (obj), TYPE_GENERIC_PCI) >> + >> +#define TYPE_GENERIC_PCI_HOST "generic_pci_host" >> +#define PCI_GEN_HOST(obj) \ >> + OBJECT_CHECK(GenericPCIHostState, (obj), TYPE_GENERIC_PCI_HOST) >> + >> +void pci_controller_build_dt_node(void *fdt, DeviceState *dev); >> + >> +#endif >> \ No newline at end of file >> > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms 2014-11-24 14:57 ` alvise rigo @ 2014-11-25 10:20 ` Claudio Fontana 0 siblings, 0 replies; 29+ messages in thread From: Claudio Fontana @ 2014-11-25 10:20 UTC (permalink / raw) To: alvise rigo Cc: Rob Herring, VirtualOpenSystems Technical Team, QEMU Developers, Peter Maydell On 24.11.2014 15:57, alvise rigo wrote: > Hi Claudio, > > Thank you for your review. Please see my in-line comments. > > On Mon, Nov 24, 2014 at 11:34 AM, Claudio Fontana > <claudio.fontana@huawei.com> wrote: >> On 21.11.2014 19:07, Alvise Rigo wrote: >>> Add a generic PCI host controller for virtual platforms, based on the >>> previous work by Rob Herring: >>> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html >> >> Would it not be better to add rob herring's signoff, and then the explanation >> of what you changed from his patch, followed by your signoff? >> Or did you change so much that you had to redo the original work by Rob Herring >> from scratch? > > It was actually difficult to create some meaningful patches over the > initial ones. > It's sure that for a final version these details will be properly > addressed (signoff and diff over the original patches if possible). > >> >>> >>> The controller creates a PCI bus for PCI devices; it is intended to >>> receive from the platform all the needed information to initiate the >>> memory regions expected by the PCI system. Due to this dependence, a >>> header file is used to define the structure that the platform has to >>> fill with the data needed by the controller. The device provides also a >>> routine for the device tree node generation, which mostly has to take >>> care of the interrupt-map property generation. This property is fetched >>> by the guest operating system to map any PCI interrupt to the interrupt >>> controller. >> >>> For the time being, the device expects a GIC v2 to be used >>> by the guest. >> >> That's a pretty big limitation for a "generic" controller. >> If it's only about the size of a parent interrupt cell or something like >> that, why don't we provide it as part of the platform description that >> you pass to the machinery anyway? > > Yes we can, however being totally interrupt controller independent > means that the platform has to fully provide the way to describe a > parent interrupt. > For the time being, we use a description compatible with GICv2 (and > v3), to cover the use case mach-virt + generic-pci (this is why I also > called the interrupt specific structures "gic*"). > If we really need to be more general, I can see two solutions: the > first one is to find a way to pass all the necessary interrupt > controller information to the device machinery (phandle, #cells, > interrupt number, etc.). > I don't know how such a solution could get complicated in the future, > with some other virt-platform that requires a complicated interrupt > mapping for example. > The second would be to move all the device node generation back to the > platform, making its code even more crowded. > Are there other solutions that I'm missing? > >> >>> Only mach-virt has been used to test the controller. >>> >>> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> >>> --- >>> hw/pci-host/Makefile.objs | 2 +- >>> hw/pci-host/generic-pci.c | 280 ++++++++++++++++++++++++++++++++++++++ >>> include/hw/pci-host/generic-pci.h | 74 ++++++++++ >>> 3 files changed, 355 insertions(+), 1 deletion(-) >>> create mode 100644 hw/pci-host/generic-pci.c >>> create mode 100644 include/hw/pci-host/generic-pci.h >>> >>> diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs >>> index bb65f9c..8ef9fac 100644 >>> --- a/hw/pci-host/Makefile.objs >>> +++ b/hw/pci-host/Makefile.objs >>> @@ -1,4 +1,4 @@ >>> -common-obj-y += pam.o >>> +common-obj-y += pam.o generic-pci.o >>> >>> # PPC devices >>> common-obj-$(CONFIG_PREP_PCI) += prep.o >>> diff --git a/hw/pci-host/generic-pci.c b/hw/pci-host/generic-pci.c >>> new file mode 100644 >>> index 0000000..9c90263 >>> --- /dev/null >>> +++ b/hw/pci-host/generic-pci.c >>> @@ -0,0 +1,280 @@ >>> +/* >>> + * Generic PCI host controller >>> + * >>> + * Copyright (c) 2014 Linaro, Ltd. >>> + * Author: Rob Herring <rob.herring@linaro.org> >>> + * >>> + * Based on ARM Versatile PCI controller (hw/pci-host/versatile.c): >>> + * Copyright (c) 2006-2009 CodeSourcery. >>> + * Written by Paul Brook >>> + * >>> + * This code is licensed under the LGPL. >>> + */ >>> + >>> +#include "hw/sysbus.h" >>> +#include "hw/pci-host/generic-pci.h" >>> +#include "exec/address-spaces.h" >>> +#include "sysemu/device_tree.h" >>> + >>> +static const VMStateDescription pci_generic_host_vmstate = { >>> + .name = "generic-host-pci", >>> + .version_id = 1, >>> + .minimum_version_id = 1, >>> +}; >>> + >>> +static void pci_cam_config_write(void *opaque, hwaddr addr, >>> + uint64_t val, unsigned size) >>> +{ >> >> I would add a comment: >> /* the MemoryRegionOps require uint64_t val, but we can only do 32bit */ >> >> there are already asserts in pci_host.c, so that should be sufficient. > > Agreed. > >> >>> + PCIGenState *s = opaque; >>> + pci_data_write(&s->pci_bus, addr, val, size); >>> +} >>> + >>> +static uint64_t pci_cam_config_read(void *opaque, hwaddr addr, unsigned size) >> >> /* same here */ >> >>> +{ >>> + PCIGenState *s = opaque; >>> + uint32_t val; >>> + val = pci_data_read(&s->pci_bus, addr, size); >>> + return val; >>> +} >>> + >>> +static const MemoryRegionOps pci_vpb_config_ops = { >>> + .read = pci_cam_config_read, >>> + .write = pci_cam_config_write, >>> + .endianness = DEVICE_NATIVE_ENDIAN, >>> +}; >>> + >>> +static void pci_generic_set_irq(void *opaque, int irq_num, int level) >>> +{ >>> + qemu_irq *pic = opaque; >>> + qemu_set_irq(pic[irq_num], level); >>> +} >>> + >>> +static void pci_generic_host_init(Object *obj) >>> +{ >>> + PCIHostState *h = PCI_HOST_BRIDGE(obj); >>> + PCIGenState *s = PCI_GEN(obj); >>> + GenericPCIHostState *gps = &s->pci_gen; >>> + >>> + memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32); >>> + memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32); >>> + >>> + pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci", >>> + &s->pci_mem_space, &s->pci_io_space, >>> + PCI_DEVFN(0, 0), TYPE_PCIE_BUS); >> >> TYPE_PCI_BUS, until we actually support PCIE. > > This was present in the first RFC as well since harmless, but I will > make it PCI only. > >> >>> + h->bus = &s->pci_bus; >>> + >>> + object_initialize(gps, sizeof(*gps), TYPE_GENERIC_PCI_HOST); >>> + qdev_set_parent_bus(DEVICE(gps), BUS(&s->pci_bus)); >>> + gps->devfns = 0; >>> +} >>> + >>> +static int generic_pci_map_irq_fn(PCIDevice *pci_dev, int pin) >>> +{ >>> + BusState *bus = qdev_get_parent_bus(&pci_dev->qdev); >>> + PCIBus *pci_bus = PCI_BUS(bus); >>> + PCIDevice *pdev = pci_bus->devices[PCI_DEVFN(0, 0)]; >>> + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); >>> + >>> + return gps->irqmap.devfn_idx_map[PCI_SLOT(pci_dev->devfn)] >>> + [PCI_FUNC(pci_dev->devfn)]; >>> +} >>> + >>> +static void pci_generic_host_realize(DeviceState *dev, Error **errp) >>> +{ >>> + PCIGenState *s = PCI_GEN(dev); >>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >>> + GenericPCIPlatformData *pdata = &s->plat_data; >>> + int i; >>> + >>> + if (!pdata->addr_map[PCI_CFG].size || !pdata->addr_map[PCI_IO].size || >>> + !pdata->addr_map[PCI_MEM].size || !pdata->irqs) { >>> + hw_error("generic_pci: PCI controller not fully configured."); >>> + } >>> + >>> + for (i = 0; i < pdata->irqs; i++) { >>> + sysbus_init_irq(sbd, &s->irq[i]); >>> + } >>> + >>> + pci_bus_irqs(&s->pci_bus, pci_generic_set_irq, generic_pci_map_irq_fn, >>> + s->irq, pdata->irqs); >>> + >>> + memory_region_init_io(&s->mem_config, OBJECT(s), &pci_vpb_config_ops, s, >>> + "pci-config", pdata->addr_map[PCI_CFG].size); >>> + sysbus_init_mmio(sbd, &s->mem_config); >>> + >>> + /* Depending on the available memory of the board using the controller, we >>> + create a window on both the I/O and mememory space. */ >> >> s/mememory/memory/ > > ACK. > >> >>> + memory_region_init_alias(&s->pci_io_window, OBJECT(s), "pci-io-win", >>> + &s->pci_io_space, 0, >>> + pdata->addr_map[PCI_IO].size); >>> + sysbus_init_mmio(sbd, &s->pci_io_window); >>> + >>> + memory_region_init_alias(&s->pci_mem_window, OBJECT(s), "pci-mem-win", >>> + &s->pci_mem_space, >>> + pdata->addr_map[PCI_MEM].addr, >>> + pdata->addr_map[PCI_MEM].size); >>> + sysbus_init_mmio(sbd, &s->pci_mem_window); >>> + >>> + /* TODO Remove once realize propagates to child devices. */ >>> + object_property_set_bool(OBJECT(&s->pci_gen), true, "realized", errp); >>> +} >>> + >>> +static void pci_generic_host_class_init(ObjectClass *klass, void *data) >>> +{ >>> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>> + DeviceClass *dc = DEVICE_CLASS(klass); >>> + >>> + k->vendor_id = PCI_VENDOR_ID_REDHAT; >>> + k->device_id = 0x1234; >>> + k->class_id = PCI_CLASS_PROCESSOR_CO; >>> + /* >>> + * PCI-facing part of the host bridge, not usable without the >>> + * host-facing part, which can't be device_add'ed, yet. >>> + */ >>> + dc->cannot_instantiate_with_device_add_yet = true; >>> +} >>> + >>> +struct dt_irq_mapping { >>> + int irqs; >>> + uint32_t gic_phandle; >> >> cannot this be generic enough that we don't talk about gic here? > > Yes. I will change it. > >> >>> + int base_irq_num; >>> + uint64_t *data; >>> +}; >>> + >>> +/* */ >> >> ? Can you remove this empty comment ? > > Of course, I missed it. > >> >> >>> +#define IRQ_MAP_ENTRY_DESC_SZ 14 >>> +static void gen_int_mapping_fn(PCIBus *b, PCIDevice *d, void *opaque) >>> +{ >>> + struct dt_irq_mapping *map_data; >>> + int pin; >>> + >>> + PCIDevice *pdev = b->devices[PCI_DEVFN(0, 0)]; >>> + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); >>> + map_data = (struct dt_irq_mapping *)opaque; >>> + >>> + /* Check if the platform has enough IRQs available. */ >>> + if (gps->devfns > map_data->irqs) { >>> + hw_error("generic_pci: too many PCI devices."); >>> + } >>> + >>> + pin = pci_get_byte(d->config + PCI_INTERRUPT_PIN); >>> + if (pin) { >>> + uint64_t *data_ptr = map_data->data + (IRQ_MAP_ENTRY_DESC_SZ * gps->devfns); >>> + uint8_t pci_slot = PCI_SLOT(d->devfn); >>> + uint8_t pci_func = PCI_FUNC(d->devfn); >>> + uint8_t *devfn_idx = &gps->irqmap.devfn_idx_map[pci_slot][0]; >>> + uint8_t *devfn_irq = gps->irqmap.devfn_irq_map; >>> + >>> + devfn_idx[pci_func] = gps->devfns; >>> + devfn_irq[gps->devfns] = map_data->base_irq_num + gps->devfns; >>> + >>> + uint64_t buffer[IRQ_MAP_ENTRY_DESC_SZ] = >>> + {1, (pci_slot << 11)|(pci_func << 8), 2, 0x00000000, 1, pin, >>> + 1, map_data->gic_phandle, 1, 0x0, 1, devfn_irq[gps->devfns], >>> + 1, 0x1}; >> >> ah now I see, I think you forgot a comment above. But maybe here is a better place. >> The above needs to be commented heavily, to make it obvious what each field is, >> and which "spec" this is following. You have mentioned already in the previous discussion, >> but it needs to be in the code. > > I will do it. > >> >>> + >>> + memcpy(data_ptr, buffer, IRQ_MAP_ENTRY_DESC_SZ * sizeof(*buffer)); >>> + gps->devfns++; >>> + } >>> +} >>> + >>> +/* Generate the "interrup-map" node's data and store it in map_data */ >> >> interrupt-map > > ACK. > >> >>> +static void generate_int_mapping(struct dt_irq_mapping *map_data, >>> + PCIGenState *s) >>> +{ >>> + pci_for_each_device(s->parent_obj.bus, 0, gen_int_mapping_fn, map_data); >>> +} >>> + >>> +static void generate_dt_node(void *fdt, DeviceState *dev) >>> +{ >>> + PCIGenState *s = PCI_GEN(dev); >>> + char *nodename; >>> + uint32_t gic_phandle; >>> + uint32_t plat_acells; >>> + uint32_t plat_scells; >>> + >>> + SysBusDevice *busdev; >>> + busdev = SYS_BUS_DEVICE(dev); >>> + MemoryRegion *cfg = sysbus_mmio_get_region(busdev, 0); >>> + MemoryRegion *io = sysbus_mmio_get_region(busdev, 1); >>> + MemoryRegion *mem = sysbus_mmio_get_region(busdev, 2); >>> + >>> + nodename = g_strdup_printf("/pci@%" PRIx64, cfg->addr); >>> + qemu_fdt_add_subnode(fdt, nodename); >>> + qemu_fdt_setprop_string(fdt, nodename, "compatible", >>> + "pci-host-cam-generic"); >>> + qemu_fdt_setprop_string(fdt, nodename, "device_type", "pci"); >>> + qemu_fdt_setprop_cell(fdt, nodename, "#address-cells", 0x3); >>> + qemu_fdt_setprop_cell(fdt, nodename, "#size-cells", 0x2); >>> + qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 0x1); >>> + >>> + plat_acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells"); >>> + plat_scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells"); >>> + qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", plat_acells, cfg->addr, >>> + plat_scells, memory_region_size(cfg)); >>> + >>> + qemu_fdt_setprop_sized_cells(fdt, nodename, "ranges", >>> + 1, 0x01000000, 2, 0x00000000, /* I/O space */ >>> + 2, io->addr, 2, memory_region_size(io), >>> + 1, 0x02000000, 2, mem->addr, /* 32bit memory space */ >> >> Is it commented somewhere sensible that the generic pci has this layout: >> >> cfg = whatever base address >> io = base + 0x1000000 >> mem = base + 0x2000000 > > The value 0x1000000 actually means that the following address has to > be interpreted as I/O address, while 0x2000000 means that it's a 32bit > address. > I will add some more documentation here, for now I point to page 4 of > the following document where the whole encoding is explained: > http://www.o3one.org/hwdocs/openfirmware/pci_supplement_2_1.pdf > All the documentation should also be available at the website > http://www.openfirmware.org (as pointed also by the kernel > documentation file Documentation/devicetree/bindings/pci/pci.txt), > however the domain does not seem active. > >> >>> + 2, mem->addr, 2, memory_region_size(mem)); >>> + >>> + gic_phandle = qemu_fdt_get_phandle(fdt, s->plat_data.gic_node_name); >> >> hmm could it be gic-independent? >> >>> + /* A mask covering bits [15,8] of phys.high allows to honor the >>> + function number when resolving a triggered PCI interrupt. */ >>> + qemu_fdt_setprop_sized_cells(fdt, nodename, "interrupt-map-mask", >>> + 1, 0xff00, 1, 0x0, 1, 0x0, 1, 0x7); >> >> and what about 0, 0, 7, what do they mean? How does the mapping work, >> what do all the fields mean, and which paper/spec are you implementing?.. > > 0, 0 are needed to mask out the phys.mid and hys.low bits of the PCI > address (since are not needed to resolve the interrupt). > I will add a more detailed documentation in the next version. > >> >>> + >>> + uint64_t *int_mapping_data = g_malloc0(IRQ_MAP_ENTRY_DESC_SZ * >>> + sizeof(uint64_t) * s->plat_data.irqs); >>> + struct dt_irq_mapping dt_map_data = { >>> + .irqs = s->plat_data.irqs, >>> + .gic_phandle = gic_phandle, >> >> could this be gic independent? > > Yes, but still we need a field necessary to store the interrupt > controller phandle. > >> >>> + .base_irq_num = s->plat_data.base_irq, >>> + .data = int_mapping_data >>> + }; >>> + /* Generate the interrupt mapping according to the devices attached >>> + * to the PCI bus of the controller. */ >>> + generate_int_mapping(&dt_map_data, s); >>> + qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "interrupt-map", >>> + (s->pci_gen.devfns * IRQ_MAP_ENTRY_DESC_SZ)/2, int_mapping_data); >>> + >>> + g_free(int_mapping_data); >>> + g_free(nodename); >>> +} >>> + >>> +void pci_controller_build_dt_node(void *fdt, DeviceState *dev) >>> +{ >>> + generate_dt_node(fdt, dev); >>> +} >>> + >>> +static const TypeInfo pci_generic_host_info = { >>> + .name = TYPE_GENERIC_PCI_HOST, >>> + .parent = TYPE_PCI_DEVICE, >>> + .instance_size = sizeof(GenericPCIHostState), >>> + .class_init = pci_generic_host_class_init, >>> +}; >>> + >>> +static void pci_generic_class_init(ObjectClass *klass, void *data) >>> +{ >>> + DeviceClass *dc = DEVICE_CLASS(klass); >>> + >>> + dc->realize = pci_generic_host_realize; >>> + dc->vmsd = &pci_generic_host_vmstate; >>> +} >>> + >>> +static const TypeInfo pci_generic_info = { >>> + .name = TYPE_GENERIC_PCI, >>> + .parent = TYPE_PCI_HOST_BRIDGE, >>> + .instance_size = sizeof(PCIGenState), >>> + .instance_init = pci_generic_host_init, >>> + .class_init = pci_generic_class_init, >>> +}; >>> + >>> +static void generic_pci_host_register_types(void) >>> +{ >>> + type_register_static(&pci_generic_info); >>> + type_register_static(&pci_generic_host_info); >>> +} >>> + >>> +type_init(generic_pci_host_register_types) >>> \ No newline at end of file >>> diff --git a/include/hw/pci-host/generic-pci.h b/include/hw/pci-host/generic-pci.h >>> new file mode 100644 >>> index 0000000..43c7a0f >>> --- /dev/null >>> +++ b/include/hw/pci-host/generic-pci.h >>> @@ -0,0 +1,74 @@ >>> +#ifndef QEMU_GENERIC_PCI_H >>> +#define QEMU_GENERIC_PCI_H >>> + >>> +#include "hw/pci/pci.h" >>> +#include "hw/pci/pci_bus.h" >>> +#include "hw/pci/pci_host.h" >>> + >>> +#define MAX_PCI_DEVICES (PCI_SLOT_MAX * PCI_FUNC_MAX) >>> + >> >> maybe this would be a good place to talk about what the address space >> layout assumptions of generic pci are, like >> >> /* The Configuration space starts at the PCI base address passed in the virtual platform >> * information, >> * The I/O space starts at an offset of 0x1000000 from the PCI base address >> * The Mem space starts at an offset of 0x2000000 from the PCI base address >> */ >> >> these the assumptions right? > > Actually this is not completely true. As explained before, 0x01000000 > and 0x02000000 indicate an I/O region and a 32bit addressable region. > The I/O window (pci_io_window) starts at the beginning of the I/O > address space (pci_io_space), no matter where it is mapped to by the > platform. > For example, mach-virt maps it to the CPU address 0x11000000 but from > the PCI bus point of view, it starts at address 0x0. > For the memory window (pci_mem_window) instead we need to place it at > the same address in the memory space (mem_io_space) to which it has > been mapped by the platform. > In mach-virt, this region starts at CPU (and also PCI) address 0x12000000. > I hope to have been enough clear. Well yes, but the problem is that using those magic numbers (0x1000000 and 0x2000000) instead of properly named macros, along with the lack of documentation about what is being done, causes exactly the kind of confusion for the reader which I incurred into. > > Regards, > alvise > >> >>> +enum { >>> + PCI_CFG = 0, >>> + PCI_IO, >>> + PCI_MEM, >>> +}; >>> + >>> +typedef struct { >>> + /* addr_map[PCI_CFG].addr = base address of configuration memory >>> + addr_map[PCI_CFG].size = configuration memory size >>> + ... */ >>> + struct addr_map { >>> + hwaddr addr; >>> + hwaddr size; >>> + } addr_map[3]; >>> + >>> + const char *gic_node_name; >>> + uint32_t base_irq; >>> + uint32_t irqs; >>> +} GenericPCIPlatformData; >>> + >>> +typedef struct { >>> + PCIDevice parent_obj; >>> + >>> + struct irqmap { >>> + /* devfn_idx_map[i][j] = index inside slot_irq_map for device at slot i, >>> + fn j */ >>> + uint8_t devfn_idx_map[PCI_SLOT_MAX][PCI_FUNC_MAX]; >>> + /* devfn_irq_map[i] = IRQ num. of the i-th device (devfn) attached to >>> + the bus */ >>> + uint8_t devfn_irq_map[MAX_PCI_DEVICES]; >>> + } irqmap; >>> + /* number of devices attached to the bus */ >>> + uint32_t devfns; >>> +} GenericPCIHostState; >>> + >>> +typedef struct { >>> + PCIHostState parent_obj; >>> + >>> + qemu_irq irq[MAX_PCI_DEVICES]; >>> + MemoryRegion mem_config; >>> + /* Containers representing the PCI address spaces */ >>> + MemoryRegion pci_io_space; >>> + MemoryRegion pci_mem_space; >>> + /* Alias regions into PCI address spaces which we expose as sysbus regions. >>> + * The offsets into pci_mem_space is fixed. */ >>> + MemoryRegion pci_io_window; >>> + MemoryRegion pci_mem_window; >>> + PCIBus pci_bus; >>> + GenericPCIHostState pci_gen; >>> + >>> + /* Platform dependent data */ >>> + GenericPCIPlatformData plat_data; >>> +} PCIGenState; >>> + >>> +#define TYPE_GENERIC_PCI "generic_pci" >>> +#define PCI_GEN(obj) \ >>> + OBJECT_CHECK(PCIGenState, (obj), TYPE_GENERIC_PCI) >>> + >>> +#define TYPE_GENERIC_PCI_HOST "generic_pci_host" >>> +#define PCI_GEN_HOST(obj) \ >>> + OBJECT_CHECK(GenericPCIHostState, (obj), TYPE_GENERIC_PCI_HOST) >>> + >>> +void pci_controller_build_dt_node(void *fdt, DeviceState *dev); >>> + >>> +#endif >>> \ No newline at end of file >>> >> >> -- Claudio Fontana Server Virtualization Architect Huawei Technologies Duesseldorf GmbH Riesstraße 25 - 80992 München office: +49 89 158834 4135 mobile: +49 15253060158 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms Alvise Rigo 2014-11-24 10:34 ` Claudio Fontana @ 2015-01-05 17:13 ` Alexander Graf 2015-01-06 8:47 ` alvise rigo 1 sibling, 1 reply; 29+ messages in thread From: Alexander Graf @ 2015-01-05 17:13 UTC (permalink / raw) To: Alvise Rigo, qemu-devel; +Cc: rob.herring, tech, claudio.fontana, peter.maydell On 21.11.14 19:07, Alvise Rigo wrote: > Add a generic PCI host controller for virtual platforms, based on the > previous work by Rob Herring: > http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html > > The controller creates a PCI bus for PCI devices; it is intended to > receive from the platform all the needed information to initiate the > memory regions expected by the PCI system. Due to this dependence, a > header file is used to define the structure that the platform has to > fill with the data needed by the controller. The device provides also a > routine for the device tree node generation, which mostly has to take > care of the interrupt-map property generation. This property is fetched > by the guest operating system to map any PCI interrupt to the interrupt > controller. For the time being, the device expects a GIC v2 to be used > by the guest. > Only mach-virt has been used to test the controller. > > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> > --- > hw/pci-host/Makefile.objs | 2 +- > hw/pci-host/generic-pci.c | 280 ++++++++++++++++++++++++++++++++++++++ > include/hw/pci-host/generic-pci.h | 74 ++++++++++ > 3 files changed, 355 insertions(+), 1 deletion(-) > create mode 100644 hw/pci-host/generic-pci.c > create mode 100644 include/hw/pci-host/generic-pci.h > > diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs > index bb65f9c..8ef9fac 100644 > --- a/hw/pci-host/Makefile.objs > +++ b/hw/pci-host/Makefile.objs > @@ -1,4 +1,4 @@ > -common-obj-y += pam.o > +common-obj-y += pam.o generic-pci.o > > # PPC devices > common-obj-$(CONFIG_PREP_PCI) += prep.o > diff --git a/hw/pci-host/generic-pci.c b/hw/pci-host/generic-pci.c > new file mode 100644 > index 0000000..9c90263 > --- /dev/null > +++ b/hw/pci-host/generic-pci.c > @@ -0,0 +1,280 @@ > +/* > + * Generic PCI host controller > + * > + * Copyright (c) 2014 Linaro, Ltd. > + * Author: Rob Herring <rob.herring@linaro.org> > + * > + * Based on ARM Versatile PCI controller (hw/pci-host/versatile.c): > + * Copyright (c) 2006-2009 CodeSourcery. > + * Written by Paul Brook > + * > + * This code is licensed under the LGPL. > + */ > + > +#include "hw/sysbus.h" > +#include "hw/pci-host/generic-pci.h" > +#include "exec/address-spaces.h" > +#include "sysemu/device_tree.h" > + > +static const VMStateDescription pci_generic_host_vmstate = { > + .name = "generic-host-pci", > + .version_id = 1, > + .minimum_version_id = 1, > +}; > + > +static void pci_cam_config_write(void *opaque, hwaddr addr, > + uint64_t val, unsigned size) > +{ > + PCIGenState *s = opaque; > + pci_data_write(&s->pci_bus, addr, val, size); > +} > + > +static uint64_t pci_cam_config_read(void *opaque, hwaddr addr, unsigned size) > +{ > + PCIGenState *s = opaque; > + uint32_t val; > + val = pci_data_read(&s->pci_bus, addr, size); > + return val; > +} > + > +static const MemoryRegionOps pci_vpb_config_ops = { > + .read = pci_cam_config_read, > + .write = pci_cam_config_write, > + .endianness = DEVICE_NATIVE_ENDIAN, Never use NATIVE_ENDIAN unless you're 100% sure it's correct. In this case, please make it LITTLE_ENDIAN. > +}; > + > +static void pci_generic_set_irq(void *opaque, int irq_num, int level) > +{ > + qemu_irq *pic = opaque; > + qemu_set_irq(pic[irq_num], level); > +} > + > +static void pci_generic_host_init(Object *obj) > +{ > + PCIHostState *h = PCI_HOST_BRIDGE(obj); > + PCIGenState *s = PCI_GEN(obj); > + GenericPCIHostState *gps = &s->pci_gen; > + > + memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32); Why is IO space that big? It's only 64k usually, no? > + memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32); > + > + pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci", > + &s->pci_mem_space, &s->pci_io_space, > + PCI_DEVFN(0, 0), TYPE_PCIE_BUS); > + h->bus = &s->pci_bus; > + > + object_initialize(gps, sizeof(*gps), TYPE_GENERIC_PCI_HOST); > + qdev_set_parent_bus(DEVICE(gps), BUS(&s->pci_bus)); > + gps->devfns = 0; > +} > + > +static int generic_pci_map_irq_fn(PCIDevice *pci_dev, int pin) > +{ > + BusState *bus = qdev_get_parent_bus(&pci_dev->qdev); > + PCIBus *pci_bus = PCI_BUS(bus); > + PCIDevice *pdev = pci_bus->devices[PCI_DEVFN(0, 0)]; > + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); > + > + return gps->irqmap.devfn_idx_map[PCI_SLOT(pci_dev->devfn)] > + [PCI_FUNC(pci_dev->devfn)]; > +} > + > +static void pci_generic_host_realize(DeviceState *dev, Error **errp) > +{ > + PCIGenState *s = PCI_GEN(dev); > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > + GenericPCIPlatformData *pdata = &s->plat_data; Where does this come from? Why isn't it exposed as qdev parameters? > + int i; > + > + if (!pdata->addr_map[PCI_CFG].size || !pdata->addr_map[PCI_IO].size || > + !pdata->addr_map[PCI_MEM].size || !pdata->irqs) { > + hw_error("generic_pci: PCI controller not fully configured."); > + } > + > + for (i = 0; i < pdata->irqs; i++) { > + sysbus_init_irq(sbd, &s->irq[i]); > + } > + > + pci_bus_irqs(&s->pci_bus, pci_generic_set_irq, generic_pci_map_irq_fn, > + s->irq, pdata->irqs); > + > + memory_region_init_io(&s->mem_config, OBJECT(s), &pci_vpb_config_ops, s, > + "pci-config", pdata->addr_map[PCI_CFG].size); > + sysbus_init_mmio(sbd, &s->mem_config); > + > + /* Depending on the available memory of the board using the controller, we > + create a window on both the I/O and mememory space. */ meme? > + memory_region_init_alias(&s->pci_io_window, OBJECT(s), "pci-io-win", > + &s->pci_io_space, 0, > + pdata->addr_map[PCI_IO].size); > + sysbus_init_mmio(sbd, &s->pci_io_window); > + > + memory_region_init_alias(&s->pci_mem_window, OBJECT(s), "pci-mem-win", > + &s->pci_mem_space, > + pdata->addr_map[PCI_MEM].addr, > + pdata->addr_map[PCI_MEM].size); > + sysbus_init_mmio(sbd, &s->pci_mem_window); What if we simply postpone the creation of those regions and the creation of the PCIBus to the realize function? At that point we know the size of the regions. > + > + /* TODO Remove once realize propagates to child devices. */ > + object_property_set_bool(OBJECT(&s->pci_gen), true, "realized", errp); Is this still necessary? In fact, wouldn't the realize of our PHB and the creation of child devices happen consecutively usually? > +} > + > +static void pci_generic_host_class_init(ObjectClass *klass, void *data) > +{ > + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + k->vendor_id = PCI_VENDOR_ID_REDHAT; > + k->device_id = 0x1234; Is this a reserved ID? > + k->class_id = PCI_CLASS_PROCESSOR_CO; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > +} > + > +struct dt_irq_mapping { > + int irqs; > + uint32_t gic_phandle; > + int base_irq_num; > + uint64_t *data; > +}; > + > +/* */ > +#define IRQ_MAP_ENTRY_DESC_SZ 14 > +static void gen_int_mapping_fn(PCIBus *b, PCIDevice *d, void *opaque) > +{ > + struct dt_irq_mapping *map_data; > + int pin; > + > + PCIDevice *pdev = b->devices[PCI_DEVFN(0, 0)]; > + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); > + map_data = (struct dt_irq_mapping *)opaque; > + > + /* Check if the platform has enough IRQs available. */ > + if (gps->devfns > map_data->irqs) { > + hw_error("generic_pci: too many PCI devices."); > + } > + > + pin = pci_get_byte(d->config + PCI_INTERRUPT_PIN); > + if (pin) { > + uint64_t *data_ptr = map_data->data + (IRQ_MAP_ENTRY_DESC_SZ * gps->devfns); > + uint8_t pci_slot = PCI_SLOT(d->devfn); > + uint8_t pci_func = PCI_FUNC(d->devfn); > + uint8_t *devfn_idx = &gps->irqmap.devfn_idx_map[pci_slot][0]; > + uint8_t *devfn_irq = gps->irqmap.devfn_irq_map; > + > + devfn_idx[pci_func] = gps->devfns; > + devfn_irq[gps->devfns] = map_data->base_irq_num + gps->devfns; > + > + uint64_t buffer[IRQ_MAP_ENTRY_DESC_SZ] = > + {1, (pci_slot << 11)|(pci_func << 8), 2, 0x00000000, 1, pin, > + 1, map_data->gic_phandle, 1, 0x0, 1, devfn_irq[gps->devfns], > + 1, 0x1}; > + > + memcpy(data_ptr, buffer, IRQ_MAP_ENTRY_DESC_SZ * sizeof(*buffer)); > + gps->devfns++; > + } > +} > + > +/* Generate the "interrup-map" node's data and store it in map_data */ > +static void generate_int_mapping(struct dt_irq_mapping *map_data, > + PCIGenState *s) > +{ > + pci_for_each_device(s->parent_obj.bus, 0, gen_int_mapping_fn, map_data); The interrupt-map should describe interrupt mappings for every slot, not every device. If you only describe the current state of affairs, hotplug won't work. > +} > + > +static void generate_dt_node(void *fdt, DeviceState *dev) > +{ > + PCIGenState *s = PCI_GEN(dev); > + char *nodename; > + uint32_t gic_phandle; > + uint32_t plat_acells; > + uint32_t plat_scells; > + > + SysBusDevice *busdev; > + busdev = SYS_BUS_DEVICE(dev); > + MemoryRegion *cfg = sysbus_mmio_get_region(busdev, 0); > + MemoryRegion *io = sysbus_mmio_get_region(busdev, 1); > + MemoryRegion *mem = sysbus_mmio_get_region(busdev, 2); > + > + nodename = g_strdup_printf("/pci@%" PRIx64, cfg->addr); > + qemu_fdt_add_subnode(fdt, nodename); > + qemu_fdt_setprop_string(fdt, nodename, "compatible", > + "pci-host-cam-generic"); > + qemu_fdt_setprop_string(fdt, nodename, "device_type", "pci"); > + qemu_fdt_setprop_cell(fdt, nodename, "#address-cells", 0x3); > + qemu_fdt_setprop_cell(fdt, nodename, "#size-cells", 0x2); > + qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 0x1); > + > + plat_acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells"); > + plat_scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells"); > + qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", plat_acells, cfg->addr, > + plat_scells, memory_region_size(cfg)); > + > + qemu_fdt_setprop_sized_cells(fdt, nodename, "ranges", > + 1, 0x01000000, 2, 0x00000000, /* I/O space */ > + 2, io->addr, 2, memory_region_size(io), > + 1, 0x02000000, 2, mem->addr, /* 32bit memory space */ > + 2, mem->addr, 2, memory_region_size(mem)); > + > + gic_phandle = qemu_fdt_get_phandle(fdt, s->plat_data.gic_node_name); > + /* A mask covering bits [15,8] of phys.high allows to honor the > + function number when resolving a triggered PCI interrupt. */ > + qemu_fdt_setprop_sized_cells(fdt, nodename, "interrupt-map-mask", > + 1, 0xff00, 1, 0x0, 1, 0x0, 1, 0x7); > + > + uint64_t *int_mapping_data = g_malloc0(IRQ_MAP_ENTRY_DESC_SZ * > + sizeof(uint64_t) * s->plat_data.irqs); > + struct dt_irq_mapping dt_map_data = { > + .irqs = s->plat_data.irqs, > + .gic_phandle = gic_phandle, > + .base_irq_num = s->plat_data.base_irq, > + .data = int_mapping_data > + }; > + /* Generate the interrupt mapping according to the devices attached > + * to the PCI bus of the controller. */ > + generate_int_mapping(&dt_map_data, s); > + qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "interrupt-map", > + (s->pci_gen.devfns * IRQ_MAP_ENTRY_DESC_SZ)/2, int_mapping_data); > + > + g_free(int_mapping_data); > + g_free(nodename); > +} > + > +void pci_controller_build_dt_node(void *fdt, DeviceState *dev) > +{ > + generate_dt_node(fdt, dev); Device tree generation should *never* be part of device code. It's machine / board specific. If one day someone can convince me that it's safe to share device tree bits of one device with multiple boards I'm happy to work with them to achieve that goal then, but for now, please leave device tree bits in the machine logic. Alex > +} > + > +static const TypeInfo pci_generic_host_info = { > + .name = TYPE_GENERIC_PCI_HOST, > + .parent = TYPE_PCI_DEVICE, > + .instance_size = sizeof(GenericPCIHostState), > + .class_init = pci_generic_host_class_init, > +}; > + > +static void pci_generic_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->realize = pci_generic_host_realize; > + dc->vmsd = &pci_generic_host_vmstate; > +} > + > +static const TypeInfo pci_generic_info = { > + .name = TYPE_GENERIC_PCI, > + .parent = TYPE_PCI_HOST_BRIDGE, > + .instance_size = sizeof(PCIGenState), > + .instance_init = pci_generic_host_init, > + .class_init = pci_generic_class_init, > +}; > + > +static void generic_pci_host_register_types(void) > +{ > + type_register_static(&pci_generic_info); > + type_register_static(&pci_generic_host_info); > +} > + > +type_init(generic_pci_host_register_types) > \ No newline at end of file > diff --git a/include/hw/pci-host/generic-pci.h b/include/hw/pci-host/generic-pci.h > new file mode 100644 > index 0000000..43c7a0f > --- /dev/null > +++ b/include/hw/pci-host/generic-pci.h > @@ -0,0 +1,74 @@ > +#ifndef QEMU_GENERIC_PCI_H > +#define QEMU_GENERIC_PCI_H > + > +#include "hw/pci/pci.h" > +#include "hw/pci/pci_bus.h" > +#include "hw/pci/pci_host.h" > + > +#define MAX_PCI_DEVICES (PCI_SLOT_MAX * PCI_FUNC_MAX) > + > +enum { > + PCI_CFG = 0, > + PCI_IO, > + PCI_MEM, > +}; > + > +typedef struct { > + /* addr_map[PCI_CFG].addr = base address of configuration memory > + addr_map[PCI_CFG].size = configuration memory size > + ... */ > + struct addr_map { > + hwaddr addr; > + hwaddr size; > + } addr_map[3]; > + > + const char *gic_node_name; > + uint32_t base_irq; > + uint32_t irqs; > +} GenericPCIPlatformData; > + > +typedef struct { > + PCIDevice parent_obj; > + > + struct irqmap { > + /* devfn_idx_map[i][j] = index inside slot_irq_map for device at slot i, > + fn j */ > + uint8_t devfn_idx_map[PCI_SLOT_MAX][PCI_FUNC_MAX]; > + /* devfn_irq_map[i] = IRQ num. of the i-th device (devfn) attached to > + the bus */ > + uint8_t devfn_irq_map[MAX_PCI_DEVICES]; > + } irqmap; > + /* number of devices attached to the bus */ > + uint32_t devfns; > +} GenericPCIHostState; > + > +typedef struct { > + PCIHostState parent_obj; > + > + qemu_irq irq[MAX_PCI_DEVICES]; > + MemoryRegion mem_config; > + /* Containers representing the PCI address spaces */ > + MemoryRegion pci_io_space; > + MemoryRegion pci_mem_space; > + /* Alias regions into PCI address spaces which we expose as sysbus regions. > + * The offsets into pci_mem_space is fixed. */ > + MemoryRegion pci_io_window; > + MemoryRegion pci_mem_window; > + PCIBus pci_bus; > + GenericPCIHostState pci_gen; > + > + /* Platform dependent data */ > + GenericPCIPlatformData plat_data; > +} PCIGenState; > + > +#define TYPE_GENERIC_PCI "generic_pci" > +#define PCI_GEN(obj) \ > + OBJECT_CHECK(PCIGenState, (obj), TYPE_GENERIC_PCI) > + > +#define TYPE_GENERIC_PCI_HOST "generic_pci_host" > +#define PCI_GEN_HOST(obj) \ > + OBJECT_CHECK(GenericPCIHostState, (obj), TYPE_GENERIC_PCI_HOST) > + > +void pci_controller_build_dt_node(void *fdt, DeviceState *dev); > + > +#endif > \ No newline at end of file > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms 2015-01-05 17:13 ` Alexander Graf @ 2015-01-06 8:47 ` alvise rigo 2015-01-06 11:18 ` Alexander Graf 0 siblings, 1 reply; 29+ messages in thread From: alvise rigo @ 2015-01-06 8:47 UTC (permalink / raw) To: Alexander Graf Cc: Rob Herring, VirtualOpenSystems Technical Team, Claudio Fontana, QEMU Developers, Peter Maydell Hi, On Mon, Jan 5, 2015 at 6:13 PM, Alexander Graf <agraf@suse.de> wrote: > > > On 21.11.14 19:07, Alvise Rigo wrote: >> Add a generic PCI host controller for virtual platforms, based on the >> previous work by Rob Herring: >> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html >> >> The controller creates a PCI bus for PCI devices; it is intended to >> receive from the platform all the needed information to initiate the >> memory regions expected by the PCI system. Due to this dependence, a >> header file is used to define the structure that the platform has to >> fill with the data needed by the controller. The device provides also a >> routine for the device tree node generation, which mostly has to take >> care of the interrupt-map property generation. This property is fetched >> by the guest operating system to map any PCI interrupt to the interrupt >> controller. For the time being, the device expects a GIC v2 to be used >> by the guest. >> Only mach-virt has been used to test the controller. >> >> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> >> --- >> hw/pci-host/Makefile.objs | 2 +- >> hw/pci-host/generic-pci.c | 280 ++++++++++++++++++++++++++++++++++++++ >> include/hw/pci-host/generic-pci.h | 74 ++++++++++ >> 3 files changed, 355 insertions(+), 1 deletion(-) >> create mode 100644 hw/pci-host/generic-pci.c >> create mode 100644 include/hw/pci-host/generic-pci.h >> >> diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs >> index bb65f9c..8ef9fac 100644 >> --- a/hw/pci-host/Makefile.objs >> +++ b/hw/pci-host/Makefile.objs >> @@ -1,4 +1,4 @@ >> -common-obj-y += pam.o >> +common-obj-y += pam.o generic-pci.o >> >> # PPC devices >> common-obj-$(CONFIG_PREP_PCI) += prep.o >> diff --git a/hw/pci-host/generic-pci.c b/hw/pci-host/generic-pci.c >> new file mode 100644 >> index 0000000..9c90263 >> --- /dev/null >> +++ b/hw/pci-host/generic-pci.c >> @@ -0,0 +1,280 @@ >> +/* >> + * Generic PCI host controller >> + * >> + * Copyright (c) 2014 Linaro, Ltd. >> + * Author: Rob Herring <rob.herring@linaro.org> >> + * >> + * Based on ARM Versatile PCI controller (hw/pci-host/versatile.c): >> + * Copyright (c) 2006-2009 CodeSourcery. >> + * Written by Paul Brook >> + * >> + * This code is licensed under the LGPL. >> + */ >> + >> +#include "hw/sysbus.h" >> +#include "hw/pci-host/generic-pci.h" >> +#include "exec/address-spaces.h" >> +#include "sysemu/device_tree.h" >> + >> +static const VMStateDescription pci_generic_host_vmstate = { >> + .name = "generic-host-pci", >> + .version_id = 1, >> + .minimum_version_id = 1, >> +}; >> + >> +static void pci_cam_config_write(void *opaque, hwaddr addr, >> + uint64_t val, unsigned size) >> +{ >> + PCIGenState *s = opaque; >> + pci_data_write(&s->pci_bus, addr, val, size); >> +} >> + >> +static uint64_t pci_cam_config_read(void *opaque, hwaddr addr, unsigned size) >> +{ >> + PCIGenState *s = opaque; >> + uint32_t val; >> + val = pci_data_read(&s->pci_bus, addr, size); >> + return val; >> +} >> + >> +static const MemoryRegionOps pci_vpb_config_ops = { >> + .read = pci_cam_config_read, >> + .write = pci_cam_config_write, >> + .endianness = DEVICE_NATIVE_ENDIAN, > > Never use NATIVE_ENDIAN unless you're 100% sure it's correct. In this > case, please make it LITTLE_ENDIAN. Agreed. > >> +}; >> + >> +static void pci_generic_set_irq(void *opaque, int irq_num, int level) >> +{ >> + qemu_irq *pic = opaque; >> + qemu_set_irq(pic[irq_num], level); >> +} >> + >> +static void pci_generic_host_init(Object *obj) >> +{ >> + PCIHostState *h = PCI_HOST_BRIDGE(obj); >> + PCIGenState *s = PCI_GEN(obj); >> + GenericPCIHostState *gps = &s->pci_gen; >> + >> + memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32); > > Why is IO space that big? It's only 64k usually, no? This was just to prevent a definition of the io space smaller than the actual size of the memory region. This could be avoided as you suggested later, by postponing the creation of the PCIBus. > >> + memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32); >> + >> + pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci", >> + &s->pci_mem_space, &s->pci_io_space, >> + PCI_DEVFN(0, 0), TYPE_PCIE_BUS); >> + h->bus = &s->pci_bus; >> + >> + object_initialize(gps, sizeof(*gps), TYPE_GENERIC_PCI_HOST); >> + qdev_set_parent_bus(DEVICE(gps), BUS(&s->pci_bus)); >> + gps->devfns = 0; >> +} >> + >> +static int generic_pci_map_irq_fn(PCIDevice *pci_dev, int pin) >> +{ >> + BusState *bus = qdev_get_parent_bus(&pci_dev->qdev); >> + PCIBus *pci_bus = PCI_BUS(bus); >> + PCIDevice *pdev = pci_bus->devices[PCI_DEVFN(0, 0)]; >> + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); >> + >> + return gps->irqmap.devfn_idx_map[PCI_SLOT(pci_dev->devfn)] >> + [PCI_FUNC(pci_dev->devfn)]; >> +} >> + >> +static void pci_generic_host_realize(DeviceState *dev, Error **errp) >> +{ >> + PCIGenState *s = PCI_GEN(dev); >> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >> + GenericPCIPlatformData *pdata = &s->plat_data; > > Where does this come from? Why isn't it exposed as qdev parameters? It comes from the platform (e.g. mach-virt). Indeed, I will expose them as qdev properties. > >> + int i; >> + >> + if (!pdata->addr_map[PCI_CFG].size || !pdata->addr_map[PCI_IO].size || >> + !pdata->addr_map[PCI_MEM].size || !pdata->irqs) { >> + hw_error("generic_pci: PCI controller not fully configured."); >> + } >> + >> + for (i = 0; i < pdata->irqs; i++) { >> + sysbus_init_irq(sbd, &s->irq[i]); >> + } >> + >> + pci_bus_irqs(&s->pci_bus, pci_generic_set_irq, generic_pci_map_irq_fn, >> + s->irq, pdata->irqs); >> + >> + memory_region_init_io(&s->mem_config, OBJECT(s), &pci_vpb_config_ops, s, >> + "pci-config", pdata->addr_map[PCI_CFG].size); >> + sysbus_init_mmio(sbd, &s->mem_config); >> + >> + /* Depending on the available memory of the board using the controller, we >> + create a window on both the I/O and mememory space. */ > > meme? Ack. > >> + memory_region_init_alias(&s->pci_io_window, OBJECT(s), "pci-io-win", >> + &s->pci_io_space, 0, >> + pdata->addr_map[PCI_IO].size); >> + sysbus_init_mmio(sbd, &s->pci_io_window); >> + >> + memory_region_init_alias(&s->pci_mem_window, OBJECT(s), "pci-mem-win", >> + &s->pci_mem_space, >> + pdata->addr_map[PCI_MEM].addr, >> + pdata->addr_map[PCI_MEM].size); >> + sysbus_init_mmio(sbd, &s->pci_mem_window); > > What if we simply postpone the creation of those regions and the > creation of the PCIBus to the realize function? At that point we know > the size of the regions. I'm not sure but I think I've already tried this path and for some reason I've abandoned it. I will explore it again and I'll let you know. > >> + >> + /* TODO Remove once realize propagates to child devices. */ >> + object_property_set_bool(OBJECT(&s->pci_gen), true, "realized", errp); > > Is this still necessary? In fact, wouldn't the realize of our PHB and > the creation of child devices happen consecutively usually? Sure, I will fix it. > >> +} >> + >> +static void pci_generic_host_class_init(ObjectClass *klass, void *data) >> +{ >> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + k->vendor_id = PCI_VENDOR_ID_REDHAT; >> + k->device_id = 0x1234; > > Is this a reserved ID? Not at all. I see now from the documentation (docs/specs/pci-ids.txt) that a value within the range 0000 -> 00ff should be used. I suppose that eventually we will pick up one of the available? > >> + k->class_id = PCI_CLASS_PROCESSOR_CO; >> + /* >> + * PCI-facing part of the host bridge, not usable without the >> + * host-facing part, which can't be device_add'ed, yet. >> + */ >> + dc->cannot_instantiate_with_device_add_yet = true; >> +} >> + >> +struct dt_irq_mapping { >> + int irqs; >> + uint32_t gic_phandle; >> + int base_irq_num; >> + uint64_t *data; >> +}; >> + >> +/* */ >> +#define IRQ_MAP_ENTRY_DESC_SZ 14 >> +static void gen_int_mapping_fn(PCIBus *b, PCIDevice *d, void *opaque) >> +{ >> + struct dt_irq_mapping *map_data; >> + int pin; >> + >> + PCIDevice *pdev = b->devices[PCI_DEVFN(0, 0)]; >> + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); >> + map_data = (struct dt_irq_mapping *)opaque; >> + >> + /* Check if the platform has enough IRQs available. */ >> + if (gps->devfns > map_data->irqs) { >> + hw_error("generic_pci: too many PCI devices."); >> + } >> + >> + pin = pci_get_byte(d->config + PCI_INTERRUPT_PIN); >> + if (pin) { >> + uint64_t *data_ptr = map_data->data + (IRQ_MAP_ENTRY_DESC_SZ * gps->devfns); >> + uint8_t pci_slot = PCI_SLOT(d->devfn); >> + uint8_t pci_func = PCI_FUNC(d->devfn); >> + uint8_t *devfn_idx = &gps->irqmap.devfn_idx_map[pci_slot][0]; >> + uint8_t *devfn_irq = gps->irqmap.devfn_irq_map; >> + >> + devfn_idx[pci_func] = gps->devfns; >> + devfn_irq[gps->devfns] = map_data->base_irq_num + gps->devfns; >> + >> + uint64_t buffer[IRQ_MAP_ENTRY_DESC_SZ] = >> + {1, (pci_slot << 11)|(pci_func << 8), 2, 0x00000000, 1, pin, >> + 1, map_data->gic_phandle, 1, 0x0, 1, devfn_irq[gps->devfns], >> + 1, 0x1}; >> + >> + memcpy(data_ptr, buffer, IRQ_MAP_ENTRY_DESC_SZ * sizeof(*buffer)); >> + gps->devfns++; >> + } >> +} >> + >> +/* Generate the "interrup-map" node's data and store it in map_data */ >> +static void generate_int_mapping(struct dt_irq_mapping *map_data, >> + PCIGenState *s) >> +{ >> + pci_for_each_device(s->parent_obj.bus, 0, gen_int_mapping_fn, map_data); > > The interrupt-map should describe interrupt mappings for every slot, not > every device. If you only describe the current state of affairs, hotplug > won't work. Ack, as agreed also with Peter in the other thread. > >> +} >> + >> +static void generate_dt_node(void *fdt, DeviceState *dev) >> +{ >> + PCIGenState *s = PCI_GEN(dev); >> + char *nodename; >> + uint32_t gic_phandle; >> + uint32_t plat_acells; >> + uint32_t plat_scells; >> + >> + SysBusDevice *busdev; >> + busdev = SYS_BUS_DEVICE(dev); >> + MemoryRegion *cfg = sysbus_mmio_get_region(busdev, 0); >> + MemoryRegion *io = sysbus_mmio_get_region(busdev, 1); >> + MemoryRegion *mem = sysbus_mmio_get_region(busdev, 2); >> + >> + nodename = g_strdup_printf("/pci@%" PRIx64, cfg->addr); >> + qemu_fdt_add_subnode(fdt, nodename); >> + qemu_fdt_setprop_string(fdt, nodename, "compatible", >> + "pci-host-cam-generic"); >> + qemu_fdt_setprop_string(fdt, nodename, "device_type", "pci"); >> + qemu_fdt_setprop_cell(fdt, nodename, "#address-cells", 0x3); >> + qemu_fdt_setprop_cell(fdt, nodename, "#size-cells", 0x2); >> + qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 0x1); >> + >> + plat_acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells"); >> + plat_scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells"); >> + qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", plat_acells, cfg->addr, >> + plat_scells, memory_region_size(cfg)); >> + >> + qemu_fdt_setprop_sized_cells(fdt, nodename, "ranges", >> + 1, 0x01000000, 2, 0x00000000, /* I/O space */ >> + 2, io->addr, 2, memory_region_size(io), >> + 1, 0x02000000, 2, mem->addr, /* 32bit memory space */ >> + 2, mem->addr, 2, memory_region_size(mem)); >> + >> + gic_phandle = qemu_fdt_get_phandle(fdt, s->plat_data.gic_node_name); >> + /* A mask covering bits [15,8] of phys.high allows to honor the >> + function number when resolving a triggered PCI interrupt. */ >> + qemu_fdt_setprop_sized_cells(fdt, nodename, "interrupt-map-mask", >> + 1, 0xff00, 1, 0x0, 1, 0x0, 1, 0x7); >> + >> + uint64_t *int_mapping_data = g_malloc0(IRQ_MAP_ENTRY_DESC_SZ * >> + sizeof(uint64_t) * s->plat_data.irqs); >> + struct dt_irq_mapping dt_map_data = { >> + .irqs = s->plat_data.irqs, >> + .gic_phandle = gic_phandle, >> + .base_irq_num = s->plat_data.base_irq, >> + .data = int_mapping_data >> + }; >> + /* Generate the interrupt mapping according to the devices attached >> + * to the PCI bus of the controller. */ >> + generate_int_mapping(&dt_map_data, s); >> + qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "interrupt-map", >> + (s->pci_gen.devfns * IRQ_MAP_ENTRY_DESC_SZ)/2, int_mapping_data); >> + >> + g_free(int_mapping_data); >> + g_free(nodename); >> +} >> + >> +void pci_controller_build_dt_node(void *fdt, DeviceState *dev) >> +{ >> + generate_dt_node(fdt, dev); > > Device tree generation should *never* be part of device code. It's > machine / board specific. If one day someone can convince me that it's Actually you already convinced me that the device tree generation should stay in the board code :) I will rework the code accordingly. Thank you for your feedback, alvise > safe to share device tree bits of one device with multiple boards I'm > happy to work with them to achieve that goal then, but for now, please > leave device tree bits in the machine logic. > > > Alex > >> +} >> + >> +static const TypeInfo pci_generic_host_info = { >> + .name = TYPE_GENERIC_PCI_HOST, >> + .parent = TYPE_PCI_DEVICE, >> + .instance_size = sizeof(GenericPCIHostState), >> + .class_init = pci_generic_host_class_init, >> +}; >> + >> +static void pci_generic_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->realize = pci_generic_host_realize; >> + dc->vmsd = &pci_generic_host_vmstate; >> +} >> + >> +static const TypeInfo pci_generic_info = { >> + .name = TYPE_GENERIC_PCI, >> + .parent = TYPE_PCI_HOST_BRIDGE, >> + .instance_size = sizeof(PCIGenState), >> + .instance_init = pci_generic_host_init, >> + .class_init = pci_generic_class_init, >> +}; >> + >> +static void generic_pci_host_register_types(void) >> +{ >> + type_register_static(&pci_generic_info); >> + type_register_static(&pci_generic_host_info); >> +} >> + >> +type_init(generic_pci_host_register_types) >> \ No newline at end of file >> diff --git a/include/hw/pci-host/generic-pci.h b/include/hw/pci-host/generic-pci.h >> new file mode 100644 >> index 0000000..43c7a0f >> --- /dev/null >> +++ b/include/hw/pci-host/generic-pci.h >> @@ -0,0 +1,74 @@ >> +#ifndef QEMU_GENERIC_PCI_H >> +#define QEMU_GENERIC_PCI_H >> + >> +#include "hw/pci/pci.h" >> +#include "hw/pci/pci_bus.h" >> +#include "hw/pci/pci_host.h" >> + >> +#define MAX_PCI_DEVICES (PCI_SLOT_MAX * PCI_FUNC_MAX) >> + >> +enum { >> + PCI_CFG = 0, >> + PCI_IO, >> + PCI_MEM, >> +}; >> + >> +typedef struct { >> + /* addr_map[PCI_CFG].addr = base address of configuration memory >> + addr_map[PCI_CFG].size = configuration memory size >> + ... */ >> + struct addr_map { >> + hwaddr addr; >> + hwaddr size; >> + } addr_map[3]; >> + >> + const char *gic_node_name; >> + uint32_t base_irq; >> + uint32_t irqs; >> +} GenericPCIPlatformData; >> + >> +typedef struct { >> + PCIDevice parent_obj; >> + >> + struct irqmap { >> + /* devfn_idx_map[i][j] = index inside slot_irq_map for device at slot i, >> + fn j */ >> + uint8_t devfn_idx_map[PCI_SLOT_MAX][PCI_FUNC_MAX]; >> + /* devfn_irq_map[i] = IRQ num. of the i-th device (devfn) attached to >> + the bus */ >> + uint8_t devfn_irq_map[MAX_PCI_DEVICES]; >> + } irqmap; >> + /* number of devices attached to the bus */ >> + uint32_t devfns; >> +} GenericPCIHostState; >> + >> +typedef struct { >> + PCIHostState parent_obj; >> + >> + qemu_irq irq[MAX_PCI_DEVICES]; >> + MemoryRegion mem_config; >> + /* Containers representing the PCI address spaces */ >> + MemoryRegion pci_io_space; >> + MemoryRegion pci_mem_space; >> + /* Alias regions into PCI address spaces which we expose as sysbus regions. >> + * The offsets into pci_mem_space is fixed. */ >> + MemoryRegion pci_io_window; >> + MemoryRegion pci_mem_window; >> + PCIBus pci_bus; >> + GenericPCIHostState pci_gen; >> + >> + /* Platform dependent data */ >> + GenericPCIPlatformData plat_data; >> +} PCIGenState; >> + >> +#define TYPE_GENERIC_PCI "generic_pci" >> +#define PCI_GEN(obj) \ >> + OBJECT_CHECK(PCIGenState, (obj), TYPE_GENERIC_PCI) >> + >> +#define TYPE_GENERIC_PCI_HOST "generic_pci_host" >> +#define PCI_GEN_HOST(obj) \ >> + OBJECT_CHECK(GenericPCIHostState, (obj), TYPE_GENERIC_PCI_HOST) >> + >> +void pci_controller_build_dt_node(void *fdt, DeviceState *dev); >> + >> +#endif >> \ No newline at end of file >> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms 2015-01-06 8:47 ` alvise rigo @ 2015-01-06 11:18 ` Alexander Graf 0 siblings, 0 replies; 29+ messages in thread From: Alexander Graf @ 2015-01-06 11:18 UTC (permalink / raw) To: alvise rigo Cc: Rob Herring, VirtualOpenSystems Technical Team, Claudio Fontana, QEMU Developers, Peter Maydell On 06.01.15 09:47, alvise rigo wrote: > Hi, > > On Mon, Jan 5, 2015 at 6:13 PM, Alexander Graf <agraf@suse.de> wrote: >> >> >> On 21.11.14 19:07, Alvise Rigo wrote: >>> Add a generic PCI host controller for virtual platforms, based on the >>> previous work by Rob Herring: >>> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html >>> >>> The controller creates a PCI bus for PCI devices; it is intended to >>> receive from the platform all the needed information to initiate the >>> memory regions expected by the PCI system. Due to this dependence, a >>> header file is used to define the structure that the platform has to >>> fill with the data needed by the controller. The device provides also a >>> routine for the device tree node generation, which mostly has to take >>> care of the interrupt-map property generation. This property is fetched >>> by the guest operating system to map any PCI interrupt to the interrupt >>> controller. For the time being, the device expects a GIC v2 to be used >>> by the guest. >>> Only mach-virt has been used to test the controller. >>> >>> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> >>> --- >>> hw/pci-host/Makefile.objs | 2 +- >>> hw/pci-host/generic-pci.c | 280 ++++++++++++++++++++++++++++++++++++++ >>> include/hw/pci-host/generic-pci.h | 74 ++++++++++ >>> 3 files changed, 355 insertions(+), 1 deletion(-) >>> create mode 100644 hw/pci-host/generic-pci.c >>> create mode 100644 include/hw/pci-host/generic-pci.h >>> >>> diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs >>> index bb65f9c..8ef9fac 100644 >>> --- a/hw/pci-host/Makefile.objs >>> +++ b/hw/pci-host/Makefile.objs >>> @@ -1,4 +1,4 @@ >>> -common-obj-y += pam.o >>> +common-obj-y += pam.o generic-pci.o >>> >>> # PPC devices >>> common-obj-$(CONFIG_PREP_PCI) += prep.o >>> diff --git a/hw/pci-host/generic-pci.c b/hw/pci-host/generic-pci.c >>> new file mode 100644 >>> index 0000000..9c90263 >>> --- /dev/null >>> +++ b/hw/pci-host/generic-pci.c >>> @@ -0,0 +1,280 @@ >>> +/* >>> + * Generic PCI host controller >>> + * >>> + * Copyright (c) 2014 Linaro, Ltd. >>> + * Author: Rob Herring <rob.herring@linaro.org> >>> + * >>> + * Based on ARM Versatile PCI controller (hw/pci-host/versatile.c): >>> + * Copyright (c) 2006-2009 CodeSourcery. >>> + * Written by Paul Brook >>> + * >>> + * This code is licensed under the LGPL. >>> + */ >>> + >>> +#include "hw/sysbus.h" >>> +#include "hw/pci-host/generic-pci.h" >>> +#include "exec/address-spaces.h" >>> +#include "sysemu/device_tree.h" >>> + >>> +static const VMStateDescription pci_generic_host_vmstate = { >>> + .name = "generic-host-pci", >>> + .version_id = 1, >>> + .minimum_version_id = 1, >>> +}; >>> + >>> +static void pci_cam_config_write(void *opaque, hwaddr addr, >>> + uint64_t val, unsigned size) >>> +{ >>> + PCIGenState *s = opaque; >>> + pci_data_write(&s->pci_bus, addr, val, size); >>> +} >>> + >>> +static uint64_t pci_cam_config_read(void *opaque, hwaddr addr, unsigned size) >>> +{ >>> + PCIGenState *s = opaque; >>> + uint32_t val; >>> + val = pci_data_read(&s->pci_bus, addr, size); >>> + return val; >>> +} >>> + >>> +static const MemoryRegionOps pci_vpb_config_ops = { >>> + .read = pci_cam_config_read, >>> + .write = pci_cam_config_write, >>> + .endianness = DEVICE_NATIVE_ENDIAN, >> >> Never use NATIVE_ENDIAN unless you're 100% sure it's correct. In this >> case, please make it LITTLE_ENDIAN. > > Agreed. > >> >>> +}; >>> + >>> +static void pci_generic_set_irq(void *opaque, int irq_num, int level) >>> +{ >>> + qemu_irq *pic = opaque; >>> + qemu_set_irq(pic[irq_num], level); >>> +} >>> + >>> +static void pci_generic_host_init(Object *obj) >>> +{ >>> + PCIHostState *h = PCI_HOST_BRIDGE(obj); >>> + PCIGenState *s = PCI_GEN(obj); >>> + GenericPCIHostState *gps = &s->pci_gen; >>> + >>> + memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32); >> >> Why is IO space that big? It's only 64k usually, no? > > This was just to prevent a definition of the io space smaller than the > actual size of the memory region. > This could be avoided as you suggested later, by postponing the > creation of the PCIBus. > >> >>> + memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32); >>> + >>> + pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci", >>> + &s->pci_mem_space, &s->pci_io_space, >>> + PCI_DEVFN(0, 0), TYPE_PCIE_BUS); >>> + h->bus = &s->pci_bus; >>> + >>> + object_initialize(gps, sizeof(*gps), TYPE_GENERIC_PCI_HOST); >>> + qdev_set_parent_bus(DEVICE(gps), BUS(&s->pci_bus)); >>> + gps->devfns = 0; >>> +} >>> + >>> +static int generic_pci_map_irq_fn(PCIDevice *pci_dev, int pin) >>> +{ >>> + BusState *bus = qdev_get_parent_bus(&pci_dev->qdev); >>> + PCIBus *pci_bus = PCI_BUS(bus); >>> + PCIDevice *pdev = pci_bus->devices[PCI_DEVFN(0, 0)]; >>> + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); >>> + >>> + return gps->irqmap.devfn_idx_map[PCI_SLOT(pci_dev->devfn)] >>> + [PCI_FUNC(pci_dev->devfn)]; >>> +} >>> + >>> +static void pci_generic_host_realize(DeviceState *dev, Error **errp) >>> +{ >>> + PCIGenState *s = PCI_GEN(dev); >>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >>> + GenericPCIPlatformData *pdata = &s->plat_data; >> >> Where does this come from? Why isn't it exposed as qdev parameters? > > It comes from the platform (e.g. mach-virt). > Indeed, I will expose them as qdev properties. > >> >>> + int i; >>> + >>> + if (!pdata->addr_map[PCI_CFG].size || !pdata->addr_map[PCI_IO].size || >>> + !pdata->addr_map[PCI_MEM].size || !pdata->irqs) { >>> + hw_error("generic_pci: PCI controller not fully configured."); >>> + } >>> + >>> + for (i = 0; i < pdata->irqs; i++) { >>> + sysbus_init_irq(sbd, &s->irq[i]); >>> + } >>> + >>> + pci_bus_irqs(&s->pci_bus, pci_generic_set_irq, generic_pci_map_irq_fn, >>> + s->irq, pdata->irqs); >>> + >>> + memory_region_init_io(&s->mem_config, OBJECT(s), &pci_vpb_config_ops, s, >>> + "pci-config", pdata->addr_map[PCI_CFG].size); >>> + sysbus_init_mmio(sbd, &s->mem_config); >>> + >>> + /* Depending on the available memory of the board using the controller, we >>> + create a window on both the I/O and mememory space. */ >> >> meme? > > Ack. > >> >>> + memory_region_init_alias(&s->pci_io_window, OBJECT(s), "pci-io-win", >>> + &s->pci_io_space, 0, >>> + pdata->addr_map[PCI_IO].size); >>> + sysbus_init_mmio(sbd, &s->pci_io_window); >>> + >>> + memory_region_init_alias(&s->pci_mem_window, OBJECT(s), "pci-mem-win", >>> + &s->pci_mem_space, >>> + pdata->addr_map[PCI_MEM].addr, >>> + pdata->addr_map[PCI_MEM].size); >>> + sysbus_init_mmio(sbd, &s->pci_mem_window); >> >> What if we simply postpone the creation of those regions and the >> creation of the PCIBus to the realize function? At that point we know >> the size of the regions. > > I'm not sure but I think I've already tried this path and for some > reason I've abandoned it. > I will explore it again and I'll let you know. > >> >>> + >>> + /* TODO Remove once realize propagates to child devices. */ >>> + object_property_set_bool(OBJECT(&s->pci_gen), true, "realized", errp); >> >> Is this still necessary? In fact, wouldn't the realize of our PHB and >> the creation of child devices happen consecutively usually? > > Sure, I will fix it. > >> >>> +} >>> + >>> +static void pci_generic_host_class_init(ObjectClass *klass, void *data) >>> +{ >>> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>> + DeviceClass *dc = DEVICE_CLASS(klass); >>> + >>> + k->vendor_id = PCI_VENDOR_ID_REDHAT; >>> + k->device_id = 0x1234; >> >> Is this a reserved ID? > > Not at all. I see now from the documentation (docs/specs/pci-ids.txt) > that a value within the range 0000 -> 00ff should be used. > I suppose that eventually we will pick up one of the available? > >> >>> + k->class_id = PCI_CLASS_PROCESSOR_CO; >>> + /* >>> + * PCI-facing part of the host bridge, not usable without the >>> + * host-facing part, which can't be device_add'ed, yet. >>> + */ >>> + dc->cannot_instantiate_with_device_add_yet = true; >>> +} >>> + >>> +struct dt_irq_mapping { >>> + int irqs; >>> + uint32_t gic_phandle; >>> + int base_irq_num; >>> + uint64_t *data; >>> +}; >>> + >>> +/* */ >>> +#define IRQ_MAP_ENTRY_DESC_SZ 14 >>> +static void gen_int_mapping_fn(PCIBus *b, PCIDevice *d, void *opaque) >>> +{ >>> + struct dt_irq_mapping *map_data; >>> + int pin; >>> + >>> + PCIDevice *pdev = b->devices[PCI_DEVFN(0, 0)]; >>> + GenericPCIHostState *gps = PCI_GEN_HOST(pdev); >>> + map_data = (struct dt_irq_mapping *)opaque; >>> + >>> + /* Check if the platform has enough IRQs available. */ >>> + if (gps->devfns > map_data->irqs) { >>> + hw_error("generic_pci: too many PCI devices."); >>> + } >>> + >>> + pin = pci_get_byte(d->config + PCI_INTERRUPT_PIN); >>> + if (pin) { >>> + uint64_t *data_ptr = map_data->data + (IRQ_MAP_ENTRY_DESC_SZ * gps->devfns); >>> + uint8_t pci_slot = PCI_SLOT(d->devfn); >>> + uint8_t pci_func = PCI_FUNC(d->devfn); >>> + uint8_t *devfn_idx = &gps->irqmap.devfn_idx_map[pci_slot][0]; >>> + uint8_t *devfn_irq = gps->irqmap.devfn_irq_map; >>> + >>> + devfn_idx[pci_func] = gps->devfns; >>> + devfn_irq[gps->devfns] = map_data->base_irq_num + gps->devfns; >>> + >>> + uint64_t buffer[IRQ_MAP_ENTRY_DESC_SZ] = >>> + {1, (pci_slot << 11)|(pci_func << 8), 2, 0x00000000, 1, pin, >>> + 1, map_data->gic_phandle, 1, 0x0, 1, devfn_irq[gps->devfns], >>> + 1, 0x1}; >>> + >>> + memcpy(data_ptr, buffer, IRQ_MAP_ENTRY_DESC_SZ * sizeof(*buffer)); >>> + gps->devfns++; >>> + } >>> +} >>> + >>> +/* Generate the "interrup-map" node's data and store it in map_data */ >>> +static void generate_int_mapping(struct dt_irq_mapping *map_data, >>> + PCIGenState *s) >>> +{ >>> + pci_for_each_device(s->parent_obj.bus, 0, gen_int_mapping_fn, map_data); >> >> The interrupt-map should describe interrupt mappings for every slot, not >> every device. If you only describe the current state of affairs, hotplug >> won't work. > > Ack, as agreed also with Peter in the other thread. > >> >>> +} >>> + >>> +static void generate_dt_node(void *fdt, DeviceState *dev) >>> +{ >>> + PCIGenState *s = PCI_GEN(dev); >>> + char *nodename; >>> + uint32_t gic_phandle; >>> + uint32_t plat_acells; >>> + uint32_t plat_scells; >>> + >>> + SysBusDevice *busdev; >>> + busdev = SYS_BUS_DEVICE(dev); >>> + MemoryRegion *cfg = sysbus_mmio_get_region(busdev, 0); >>> + MemoryRegion *io = sysbus_mmio_get_region(busdev, 1); >>> + MemoryRegion *mem = sysbus_mmio_get_region(busdev, 2); >>> + >>> + nodename = g_strdup_printf("/pci@%" PRIx64, cfg->addr); >>> + qemu_fdt_add_subnode(fdt, nodename); >>> + qemu_fdt_setprop_string(fdt, nodename, "compatible", >>> + "pci-host-cam-generic"); >>> + qemu_fdt_setprop_string(fdt, nodename, "device_type", "pci"); >>> + qemu_fdt_setprop_cell(fdt, nodename, "#address-cells", 0x3); >>> + qemu_fdt_setprop_cell(fdt, nodename, "#size-cells", 0x2); >>> + qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 0x1); >>> + >>> + plat_acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells"); >>> + plat_scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells"); >>> + qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", plat_acells, cfg->addr, >>> + plat_scells, memory_region_size(cfg)); >>> + >>> + qemu_fdt_setprop_sized_cells(fdt, nodename, "ranges", >>> + 1, 0x01000000, 2, 0x00000000, /* I/O space */ >>> + 2, io->addr, 2, memory_region_size(io), >>> + 1, 0x02000000, 2, mem->addr, /* 32bit memory space */ >>> + 2, mem->addr, 2, memory_region_size(mem)); >>> + >>> + gic_phandle = qemu_fdt_get_phandle(fdt, s->plat_data.gic_node_name); >>> + /* A mask covering bits [15,8] of phys.high allows to honor the >>> + function number when resolving a triggered PCI interrupt. */ >>> + qemu_fdt_setprop_sized_cells(fdt, nodename, "interrupt-map-mask", >>> + 1, 0xff00, 1, 0x0, 1, 0x0, 1, 0x7); >>> + >>> + uint64_t *int_mapping_data = g_malloc0(IRQ_MAP_ENTRY_DESC_SZ * >>> + sizeof(uint64_t) * s->plat_data.irqs); >>> + struct dt_irq_mapping dt_map_data = { >>> + .irqs = s->plat_data.irqs, >>> + .gic_phandle = gic_phandle, >>> + .base_irq_num = s->plat_data.base_irq, >>> + .data = int_mapping_data >>> + }; >>> + /* Generate the interrupt mapping according to the devices attached >>> + * to the PCI bus of the controller. */ >>> + generate_int_mapping(&dt_map_data, s); >>> + qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "interrupt-map", >>> + (s->pci_gen.devfns * IRQ_MAP_ENTRY_DESC_SZ)/2, int_mapping_data); >>> + >>> + g_free(int_mapping_data); >>> + g_free(nodename); >>> +} >>> + >>> +void pci_controller_build_dt_node(void *fdt, DeviceState *dev) >>> +{ >>> + generate_dt_node(fdt, dev); >> >> Device tree generation should *never* be part of device code. It's >> machine / board specific. If one day someone can convince me that it's > > Actually you already convinced me that the device tree generation > should stay in the board code :) > I will rework the code accordingly. Meanwhile I've taken a stab at implementing a generic PCIe PHB instead. I'm not sure we should really bother with legacy PCI at this point. Please give it a try and see whether all of the devices that worked for you with this patch also work with the PCIe version: https://github.com/agraf/qemu/commit/ff56c2b2f8f50fe165b3febee0ab2d773b26040b Alex ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <1416593261-13751-5-git-send-email-a.rigo@virtualopensystems.com>]
* Re: [Qemu-devel] [RFC v2 4/4] hw/arm/virt: Add generic-pci device support [not found] ` <1416593261-13751-5-git-send-email-a.rigo@virtualopensystems.com> @ 2014-11-24 10:38 ` Claudio Fontana 2014-11-24 10:47 ` alvise rigo 0 siblings, 1 reply; 29+ messages in thread From: Claudio Fontana @ 2014-11-24 10:38 UTC (permalink / raw) To: Alvise Rigo, qemu-devel; +Cc: rob.herring, tech, peter.maydell On 21.11.2014 19:07, Alvise Rigo wrote: > Instantiate a generic-pci PCI controller to add a PCI bus to the > mach-virt platform. The platform memory map has now three more memory > ranges to map the device's memory regions (Configuration region, I/O > region and Memory region). Now that a PCI bus is provided, the machine > could be launched with multiple PCI devices through the -device option > (e.g., -device virtio-blk-pci). > > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> > --- > hw/arm/virt.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 4e7b869..74e6838 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -32,6 +32,7 @@ > #include "hw/arm/arm.h" > #include "hw/arm/primecell.h" > #include "hw/devices.h" > +#include "hw/pci-host/generic-pci.h" > #include "net/net.h" > #include "sysemu/block-backend.h" > #include "sysemu/device_tree.h" > @@ -44,6 +45,7 @@ > #include "qemu/error-report.h" > > #define NUM_VIRTIO_TRANSPORTS 32 > +#define NUM_PCI_IRQS 20 > > /* Number of external interrupt lines to configure the GIC with */ > #define NUM_IRQS 128 > @@ -68,6 +70,9 @@ enum { > VIRT_UART, > VIRT_MMIO, > VIRT_RTC, > + VIRT_PCI_CFG, > + VIRT_PCI_IO, > + VIRT_PCI_MEM, > }; > > typedef void (*modify_dtb_func)(void *fdt, DeviceState *dev); > @@ -120,6 +125,9 @@ static const MemMapEntry a15memmap[] = { > [VIRT_MMIO] = { 0x0a000000, 0x00000200 }, > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */ > /* 0x10000000 .. 0x40000000 reserved for PCI */ > + [VIRT_PCI_CFG] = { 0x10000000, 0x01000000 }, > + [VIRT_PCI_IO] = { 0x11000000, 0x00010000 }, > + [VIRT_PCI_MEM] = { 0x12000000, 0x2e000000 }, > [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 }, > }; > > @@ -127,6 +135,7 @@ static const int a15irqmap[] = { > [VIRT_UART] = 1, > [VIRT_RTC] = 2, > [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ > + [VIRT_PCI_CFG] = 47, why not say [VIRT_PCI_CFG] = 16 + NUM_VIRTIO_TRANSPORTS instead of "47"? By the way isn't 47 wrong anyway? From 16 to 47 it's virtio transports no? Should it not be 48? > }; > > static VirtBoardInfo machines[] = { > @@ -550,6 +559,50 @@ static void create_flash(const VirtBoardInfo *vbi) > g_free(nodename); > } > > +static void create_pci_host(const VirtBoardInfo *vbi, qemu_irq *pic) > +{ > + PCIBus *pci_bus; > + DeviceState *dev; > + SysBusDevice *busdev; > + PCIGenState *pgdev; > + GenericPCIPlatformData *pdata; > + int i; > + const MemMapEntry *mme = NULL; > + > + dev = qdev_create(NULL, "generic_pci"); > + busdev = SYS_BUS_DEVICE(dev); > + pgdev = PCI_GEN(dev); > + > + mme = vbi->memmap; > + > + /* Pass platform dependent data to the controller. */ > + pdata = &pgdev->plat_data; > + pdata->addr_map[PCI_CFG].addr = mme[VIRT_PCI_CFG].base; > + pdata->addr_map[PCI_CFG].size = mme[VIRT_PCI_CFG].size; > + pdata->addr_map[PCI_IO].addr = mme[VIRT_PCI_IO].base; > + pdata->addr_map[PCI_IO].size = mme[VIRT_PCI_IO].size; > + pdata->addr_map[PCI_MEM].addr = mme[VIRT_PCI_MEM].base; > + pdata->addr_map[PCI_MEM].size = mme[VIRT_PCI_MEM].size; > + pdata->gic_node_name = "/intc"; > + pdata->base_irq = vbi->irqmap[VIRT_PCI_CFG]; > + pdata->irqs = NUM_PCI_IRQS; > + qdev_init_nofail(dev); > + > + sysbus_mmio_map(busdev, 0, mme[VIRT_PCI_CFG].base); /* PCI config */ > + sysbus_mmio_map(busdev, 1, mme[VIRT_PCI_IO].base); /* PCI I/O */ > + sysbus_mmio_map(busdev, 2, mme[VIRT_PCI_MEM].base); /* PCI memory window */ > + > + for ( i = 0; i < NUM_PCI_IRQS; i++) { > + sysbus_connect_irq(busdev, i, pic[vbi->irqmap[VIRT_PCI_CFG] + i]); > + } > + > + pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci"); > + pci_create_simple(pci_bus, -1, "pci-ohci"); > + pci_create_simple(pci_bus, -1, "lsi53c895a"); > + > + add_dtb_modifier(pci_controller_build_dt_node, dev); > +} > + > static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) > { > const VirtBoardInfo *board = (const VirtBoardInfo *)binfo; > @@ -658,6 +711,8 @@ static void machvirt_init(MachineState *machine) > */ > create_virtio_devices(vbi, pic); > > + create_pci_host(vbi, pic); > + > Notifier *finalize_dtb_notifier = g_new(Notifier, 1); > finalize_dtb_notifier->notify = machvirt_finalize_dt; > qemu_add_machine_init_done_notifier(finalize_dtb_notifier); > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 4/4] hw/arm/virt: Add generic-pci device support 2014-11-24 10:38 ` [Qemu-devel] [RFC v2 4/4] hw/arm/virt: Add generic-pci device support Claudio Fontana @ 2014-11-24 10:47 ` alvise rigo 0 siblings, 0 replies; 29+ messages in thread From: alvise rigo @ 2014-11-24 10:47 UTC (permalink / raw) To: Claudio Fontana Cc: Rob Herring, VirtualOpenSystems Technical Team, QEMU Developers, Peter Maydell [-- Attachment #1: Type: text/plain, Size: 5048 bytes --] Yes, I forgot to remove this hard-coded (wrong) value. I will fix it in the next release. On Mon, Nov 24, 2014 at 11:38 AM, Claudio Fontana < claudio.fontana@huawei.com> wrote: > On 21.11.2014 19:07, Alvise Rigo wrote: > > Instantiate a generic-pci PCI controller to add a PCI bus to the > > mach-virt platform. The platform memory map has now three more memory > > ranges to map the device's memory regions (Configuration region, I/O > > region and Memory region). Now that a PCI bus is provided, the machine > > could be launched with multiple PCI devices through the -device option > > (e.g., -device virtio-blk-pci). > > > > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> > > --- > > hw/arm/virt.c | 55 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index 4e7b869..74e6838 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -32,6 +32,7 @@ > > #include "hw/arm/arm.h" > > #include "hw/arm/primecell.h" > > #include "hw/devices.h" > > +#include "hw/pci-host/generic-pci.h" > > #include "net/net.h" > > #include "sysemu/block-backend.h" > > #include "sysemu/device_tree.h" > > @@ -44,6 +45,7 @@ > > #include "qemu/error-report.h" > > > > #define NUM_VIRTIO_TRANSPORTS 32 > > +#define NUM_PCI_IRQS 20 > > > > /* Number of external interrupt lines to configure the GIC with */ > > #define NUM_IRQS 128 > > @@ -68,6 +70,9 @@ enum { > > VIRT_UART, > > VIRT_MMIO, > > VIRT_RTC, > > + VIRT_PCI_CFG, > > + VIRT_PCI_IO, > > + VIRT_PCI_MEM, > > }; > > > > typedef void (*modify_dtb_func)(void *fdt, DeviceState *dev); > > @@ -120,6 +125,9 @@ static const MemMapEntry a15memmap[] = { > > [VIRT_MMIO] = { 0x0a000000, 0x00000200 }, > > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that > size */ > > /* 0x10000000 .. 0x40000000 reserved for PCI */ > > + [VIRT_PCI_CFG] = { 0x10000000, 0x01000000 }, > > + [VIRT_PCI_IO] = { 0x11000000, 0x00010000 }, > > + [VIRT_PCI_MEM] = { 0x12000000, 0x2e000000 }, > > [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 }, > > }; > > > > @@ -127,6 +135,7 @@ static const int a15irqmap[] = { > > [VIRT_UART] = 1, > > [VIRT_RTC] = 2, > > [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ > > + [VIRT_PCI_CFG] = 47, > > why not say > [VIRT_PCI_CFG] = 16 + NUM_VIRTIO_TRANSPORTS > > instead of "47"? > > By the way isn't 47 wrong anyway? From 16 to 47 it's virtio transports no? > Should it not be 48? > > > }; > > > > static VirtBoardInfo machines[] = { > > @@ -550,6 +559,50 @@ static void create_flash(const VirtBoardInfo *vbi) > > g_free(nodename); > > } > > > > +static void create_pci_host(const VirtBoardInfo *vbi, qemu_irq *pic) > > +{ > > + PCIBus *pci_bus; > > + DeviceState *dev; > > + SysBusDevice *busdev; > > + PCIGenState *pgdev; > > + GenericPCIPlatformData *pdata; > > + int i; > > + const MemMapEntry *mme = NULL; > > + > > + dev = qdev_create(NULL, "generic_pci"); > > + busdev = SYS_BUS_DEVICE(dev); > > + pgdev = PCI_GEN(dev); > > + > > + mme = vbi->memmap; > > + > > + /* Pass platform dependent data to the controller. */ > > + pdata = &pgdev->plat_data; > > + pdata->addr_map[PCI_CFG].addr = mme[VIRT_PCI_CFG].base; > > + pdata->addr_map[PCI_CFG].size = mme[VIRT_PCI_CFG].size; > > + pdata->addr_map[PCI_IO].addr = mme[VIRT_PCI_IO].base; > > + pdata->addr_map[PCI_IO].size = mme[VIRT_PCI_IO].size; > > + pdata->addr_map[PCI_MEM].addr = mme[VIRT_PCI_MEM].base; > > + pdata->addr_map[PCI_MEM].size = mme[VIRT_PCI_MEM].size; > > + pdata->gic_node_name = "/intc"; > > + pdata->base_irq = vbi->irqmap[VIRT_PCI_CFG]; > > + pdata->irqs = NUM_PCI_IRQS; > > + qdev_init_nofail(dev); > > + > > + sysbus_mmio_map(busdev, 0, mme[VIRT_PCI_CFG].base); /* PCI config */ > > + sysbus_mmio_map(busdev, 1, mme[VIRT_PCI_IO].base); /* PCI I/O */ > > + sysbus_mmio_map(busdev, 2, mme[VIRT_PCI_MEM].base); /* PCI memory > window */ > > + > > + for ( i = 0; i < NUM_PCI_IRQS; i++) { > > + sysbus_connect_irq(busdev, i, pic[vbi->irqmap[VIRT_PCI_CFG] + > i]); > > + } > > + > > + pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci"); > > + pci_create_simple(pci_bus, -1, "pci-ohci"); > > + pci_create_simple(pci_bus, -1, "lsi53c895a"); > > + > > + add_dtb_modifier(pci_controller_build_dt_node, dev); > > +} > > + > > static void *machvirt_dtb(const struct arm_boot_info *binfo, int > *fdt_size) > > { > > const VirtBoardInfo *board = (const VirtBoardInfo *)binfo; > > @@ -658,6 +711,8 @@ static void machvirt_init(MachineState *machine) > > */ > > create_virtio_devices(vbi, pic); > > > > + create_pci_host(vbi, pic); > > + > > Notifier *finalize_dtb_notifier = g_new(Notifier, 1); > > finalize_dtb_notifier->notify = machvirt_finalize_dt; > > qemu_add_machine_init_done_notifier(finalize_dtb_notifier); > > > > > [-- Attachment #2: Type: text/html, Size: 6645 bytes --] ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update 2014-11-21 18:07 [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update Alvise Rigo ` (3 preceding siblings ...) [not found] ` <1416593261-13751-5-git-send-email-a.rigo@virtualopensystems.com> @ 2014-11-24 15:50 ` Claudio Fontana 2014-11-25 10:28 ` alvise rigo 2015-01-12 16:26 ` Claudio Fontana 5 siblings, 1 reply; 29+ messages in thread From: Claudio Fontana @ 2014-11-24 15:50 UTC (permalink / raw) To: Alvise Rigo, qemu-devel; +Cc: rob.herring, tech, peter.maydell Another general question about this series use: why do all these other devices that are unrelated to the virt platform show up? Here I am running on the guest with just virtio-net, virtio-blk and virtio-rng: (qemu) info pci Bus 0, device 0, function 0: Class 2880: PCI device 1b36:1234 id "" Bus 0, device 1, function 0: USB controller: PCI device 106b:003f IRQ 0. BAR0: 32 bit memory at 0xffffffffffffffff [0x000000fe]. id "" Bus 0, device 2, function 0: SCSI controller: PCI device 1000:0012 IRQ 0. BAR0: I/O at 0xffffffffffffffff [0x00fe]. BAR1: 32 bit memory at 0xffffffffffffffff [0x000003fe]. BAR2: 32 bit memory at 0xffffffffffffffff [0x00001ffe]. id "" Bus 0, device 3, function 0: SCSI controller: PCI device 1af4:1001 IRQ 0. BAR0: I/O at 0x0100 [0x013f]. id "blk0" Bus 0, device 4, function 0: Class 0255: PCI device 1af4:1005 IRQ 0. BAR0: I/O at 0x0140 [0x015f]. id "" Bus 0, device 5, function 0: Ethernet controller: PCI device 1af4:1000 IRQ 0. BAR0: I/O at 0x0160 [0x017f]. BAR6: 32 bit memory at 0xffffffffffffffff [0x0003fffe]. id "" Also what is the BAR6 for the virtio-net device? I am struggling to understand where it is coming from... Thanks, Claudio On 21.11.2014 19:07, Alvise Rigo wrote: > This patch series is based on the previous work [1] and [2] by Rob > Herring and on [3] by myself. For sake of readability and since this is > still a RFC, these patches come as a stand alone work, so there's no > need to apply first [1][2][3]. it tries to enhance this work on these > points: > > Improvements from v1: > > - The code should be general enough to allow the use of the controller > with other platforms, not only with mach-virt. The only assumption > made is that a GIC v2 is used at guest side (the interrupt-map > property describes the parent interrupts using the three cells > format). > - The interrupt-map node generation has been enhanced in the following > aspects: > - support of multi-function PCI device has been added > - a PCI device can now use an interrupt pin different from #INTA > > Since some other works like [4] require to modify the device tree only > when all the devices have been instantiated, the PATCH[1/4] proposes a > solution for mach-virt to allow multiple agents (e.g., generic-pci, > VFIO) to modify the device tree. The approach in simple: a global list > is kept to store all the routines that perform the modification of the > device tree. Eventually, when the machine is completed, all these > routines are sequentially executed and the kernel is loaded to the guest > by mean of a machine_init_done_notifier. > In the context of this patch, here are some questions: > Rather than postponing the arm_load_kernel call as this patch does, > should we use instead the modify_dtb call provided by arm_boot_info to > modify the device tree? > If so, shouldn't modify_dtb be used to modify only *user* provided > device trees? > > This work has been tested attaching several PCI devices to the mach-virt > platform using an ARMv7 CPU. The tested devices are: virtio-blk-pci, > virtio-net-pci, lsi53c895a and pci-ohci (all attached at the same time). > > TODO: > - Add MSI, MSI-X support > - PCI-E support. Due to a lack of devices, this part is a bit hard to > accomplish at the moment. > > Thank you, alvise > > [1] > "[Qemu-devel] [RFC PATCH 1/2] hw/pci-host: add a generic PCI host" > http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html > [2] > "[Qemu-devel] [RFC PATCH 2/2] hw/arm/virt: Add generic PCI host device" > http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03483.html > [3] > "[Qemu-devel] [RFC PATCH 0/8] Add Generic PCI host device update" > https://lists.gnu.org/archive/html/qemu-devel/2014-07/msg01957.html > [4] > http://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg03816.html > > Alvise Rigo (4): > hw/arm/virt: Allow multiple agents to modify dt > hw/arm/virt: find_machine_info: handle NULL value > hw/pci-host: Add a generic PCI host controller for virtual platforms > hw/arm/virt: Add generic-pci device support > > hw/arm/virt.c | 114 +++++++++++++++- > hw/pci-host/Makefile.objs | 2 +- > hw/pci-host/generic-pci.c | 280 ++++++++++++++++++++++++++++++++++++++ > include/hw/pci-host/generic-pci.h | 74 ++++++++++ > 4 files changed, 468 insertions(+), 2 deletions(-) > create mode 100644 hw/pci-host/generic-pci.c > create mode 100644 include/hw/pci-host/generic-pci.h > -- Claudio Fontana Server Virtualization Architect Huawei Technologies Duesseldorf GmbH Riesstraße 25 - 80992 München office: +49 89 158834 4135 mobile: +49 15253060158 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update 2014-11-24 15:50 ` [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update Claudio Fontana @ 2014-11-25 10:28 ` alvise rigo 0 siblings, 0 replies; 29+ messages in thread From: alvise rigo @ 2014-11-25 10:28 UTC (permalink / raw) To: Claudio Fontana Cc: Rob Herring, VirtualOpenSystems Technical Team, QEMU Developers, Peter Maydell On Mon, Nov 24, 2014 at 4:50 PM, Claudio Fontana <claudio.fontana@huawei.com> wrote: > Another general question about this series use: > > why do all these other devices that are unrelated to the virt platform show up? There are two devices added to the platform by default in mach-virt. Look at the end of create_pci_host() in virt.c, you will find that a pci-ohci and a lsi53c895a device are created. > Here I am running on the guest with just virtio-net, virtio-blk and virtio-rng: > > (qemu) info pci > Bus 0, device 0, function 0: > Class 2880: PCI device 1b36:1234 > id "" > Bus 0, device 1, function 0: > USB controller: PCI device 106b:003f > IRQ 0. > BAR0: 32 bit memory at 0xffffffffffffffff [0x000000fe]. > id "" > Bus 0, device 2, function 0: > SCSI controller: PCI device 1000:0012 > IRQ 0. > BAR0: I/O at 0xffffffffffffffff [0x00fe]. > BAR1: 32 bit memory at 0xffffffffffffffff [0x000003fe]. > BAR2: 32 bit memory at 0xffffffffffffffff [0x00001ffe]. > id "" > Bus 0, device 3, function 0: > SCSI controller: PCI device 1af4:1001 > IRQ 0. > BAR0: I/O at 0x0100 [0x013f]. > id "blk0" > Bus 0, device 4, function 0: > Class 0255: PCI device 1af4:1005 > IRQ 0. > BAR0: I/O at 0x0140 [0x015f]. > id "" > Bus 0, device 5, function 0: > Ethernet controller: PCI device 1af4:1000 > IRQ 0. > BAR0: I/O at 0x0160 [0x017f]. > BAR6: 32 bit memory at 0xffffffffffffffff [0x0003fffe]. > id "" > > Also what is the BAR6 for the virtio-net device? I am struggling to understand where it is coming from... I think it has to do with the efi-virtio.rom which is supplied to the virtio-net-pci device. At the end of pci_add_option_rom() in hw/pci/pci.c you will see the sixth bar registered. alvise > > Thanks, > > Claudio > > On 21.11.2014 19:07, Alvise Rigo wrote: >> This patch series is based on the previous work [1] and [2] by Rob >> Herring and on [3] by myself. For sake of readability and since this is >> still a RFC, these patches come as a stand alone work, so there's no >> need to apply first [1][2][3]. it tries to enhance this work on these >> points: >> >> Improvements from v1: >> >> - The code should be general enough to allow the use of the controller >> with other platforms, not only with mach-virt. The only assumption >> made is that a GIC v2 is used at guest side (the interrupt-map >> property describes the parent interrupts using the three cells >> format). >> - The interrupt-map node generation has been enhanced in the following >> aspects: >> - support of multi-function PCI device has been added >> - a PCI device can now use an interrupt pin different from #INTA >> >> Since some other works like [4] require to modify the device tree only >> when all the devices have been instantiated, the PATCH[1/4] proposes a >> solution for mach-virt to allow multiple agents (e.g., generic-pci, >> VFIO) to modify the device tree. The approach in simple: a global list >> is kept to store all the routines that perform the modification of the >> device tree. Eventually, when the machine is completed, all these >> routines are sequentially executed and the kernel is loaded to the guest >> by mean of a machine_init_done_notifier. >> In the context of this patch, here are some questions: >> Rather than postponing the arm_load_kernel call as this patch does, >> should we use instead the modify_dtb call provided by arm_boot_info to >> modify the device tree? >> If so, shouldn't modify_dtb be used to modify only *user* provided >> device trees? >> >> This work has been tested attaching several PCI devices to the mach-virt >> platform using an ARMv7 CPU. The tested devices are: virtio-blk-pci, >> virtio-net-pci, lsi53c895a and pci-ohci (all attached at the same time). >> >> TODO: >> - Add MSI, MSI-X support >> - PCI-E support. Due to a lack of devices, this part is a bit hard to >> accomplish at the moment. >> >> Thank you, alvise >> >> [1] >> "[Qemu-devel] [RFC PATCH 1/2] hw/pci-host: add a generic PCI host" >> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html >> [2] >> "[Qemu-devel] [RFC PATCH 2/2] hw/arm/virt: Add generic PCI host device" >> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03483.html >> [3] >> "[Qemu-devel] [RFC PATCH 0/8] Add Generic PCI host device update" >> https://lists.gnu.org/archive/html/qemu-devel/2014-07/msg01957.html >> [4] >> http://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg03816.html >> >> Alvise Rigo (4): >> hw/arm/virt: Allow multiple agents to modify dt >> hw/arm/virt: find_machine_info: handle NULL value >> hw/pci-host: Add a generic PCI host controller for virtual platforms >> hw/arm/virt: Add generic-pci device support >> >> hw/arm/virt.c | 114 +++++++++++++++- >> hw/pci-host/Makefile.objs | 2 +- >> hw/pci-host/generic-pci.c | 280 ++++++++++++++++++++++++++++++++++++++ >> include/hw/pci-host/generic-pci.h | 74 ++++++++++ >> 4 files changed, 468 insertions(+), 2 deletions(-) >> create mode 100644 hw/pci-host/generic-pci.c >> create mode 100644 include/hw/pci-host/generic-pci.h >> > > > -- > Claudio Fontana > Server Virtualization Architect > Huawei Technologies Duesseldorf GmbH > Riesstraße 25 - 80992 München > > office: +49 89 158834 4135 > mobile: +49 15253060158 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update 2014-11-21 18:07 [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update Alvise Rigo ` (4 preceding siblings ...) 2014-11-24 15:50 ` [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update Claudio Fontana @ 2015-01-12 16:26 ` Claudio Fontana 5 siblings, 0 replies; 29+ messages in thread From: Claudio Fontana @ 2015-01-12 16:26 UTC (permalink / raw) To: Alvise Rigo, qemu-devel; +Cc: rob.herring, tech, peter.maydell On 21.11.2014 19:07, Alvise Rigo wrote: > This patch series is based on the previous work [1] and [2] by Rob > Herring and on [3] by myself. For sake of readability and since this is > still a RFC, these patches come as a stand alone work, so there's no > need to apply first [1][2][3]. it tries to enhance this work on these > points: > > Improvements from v1: > > - The code should be general enough to allow the use of the controller > with other platforms, not only with mach-virt. The only assumption > made is that a GIC v2 is used at guest side (the interrupt-map > property describes the parent interrupts using the three cells > format). > - The interrupt-map node generation has been enhanced in the following > aspects: > - support of multi-function PCI device has been added > - a PCI device can now use an interrupt pin different from #INTA > > Since some other works like [4] require to modify the device tree only > when all the devices have been instantiated, the PATCH[1/4] proposes a > solution for mach-virt to allow multiple agents (e.g., generic-pci, > VFIO) to modify the device tree. The approach in simple: a global list > is kept to store all the routines that perform the modification of the > device tree. Eventually, when the machine is completed, all these > routines are sequentially executed and the kernel is loaded to the guest > by mean of a machine_init_done_notifier. > In the context of this patch, here are some questions: > Rather than postponing the arm_load_kernel call as this patch does, > should we use instead the modify_dtb call provided by arm_boot_info to > modify the device tree? > If so, shouldn't modify_dtb be used to modify only *user* provided > device trees? > > This work has been tested attaching several PCI devices to the mach-virt > platform using an ARMv7 CPU. The tested devices are: virtio-blk-pci, > virtio-net-pci, lsi53c895a and pci-ohci (all attached at the same time). > > TODO: > - Add MSI, MSI-X support > - PCI-E support. Due to a lack of devices, this part is a bit hard to > accomplish at the moment. > > Thank you, alvise > > [1] > "[Qemu-devel] [RFC PATCH 1/2] hw/pci-host: add a generic PCI host" > http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html > [2] > "[Qemu-devel] [RFC PATCH 2/2] hw/arm/virt: Add generic PCI host device" > http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03483.html > [3] > "[Qemu-devel] [RFC PATCH 0/8] Add Generic PCI host device update" > https://lists.gnu.org/archive/html/qemu-devel/2014-07/msg01957.html > [4] > http://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg03816.html > > Alvise Rigo (4): > hw/arm/virt: Allow multiple agents to modify dt > hw/arm/virt: find_machine_info: handle NULL value > hw/pci-host: Add a generic PCI host controller for virtual platforms > hw/arm/virt: Add generic-pci device support > > hw/arm/virt.c | 114 +++++++++++++++- > hw/pci-host/Makefile.objs | 2 +- > hw/pci-host/generic-pci.c | 280 ++++++++++++++++++++++++++++++++++++++ > include/hw/pci-host/generic-pci.h | 74 ++++++++++ > 4 files changed, 468 insertions(+), 2 deletions(-) > create mode 100644 hw/pci-host/generic-pci.c > create mode 100644 include/hw/pci-host/generic-pci.h > Tested with modified OSv guest on AArch64. Tested-by: Claudio Fontana <claudio.fontana@huawei.com> ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2015-01-12 16:27 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-11-21 18:07 [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update Alvise Rigo 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 1/4] hw/arm/virt: Allow multiple agents to modify dt Alvise Rigo 2014-11-24 11:47 ` Claudio Fontana 2015-01-05 15:36 ` Peter Maydell 2015-01-05 16:14 ` alvise rigo 2015-01-05 16:41 ` Peter Maydell 2015-01-05 17:35 ` alvise rigo 2015-01-05 18:07 ` Peter Maydell 2015-01-06 9:56 ` alvise rigo 2015-01-06 9:18 ` Eric Auger 2015-01-06 9:29 ` alvise rigo 2015-01-06 9:51 ` Peter Maydell 2015-01-06 10:05 ` Eric Auger 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 2/4] hw/arm/virt: find_machine_info: handle NULL value Alvise Rigo 2015-01-05 15:36 ` Peter Maydell 2015-01-05 16:31 ` alvise rigo 2015-01-05 16:42 ` Peter Maydell 2014-11-21 18:07 ` [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms Alvise Rigo 2014-11-24 10:34 ` Claudio Fontana 2014-11-24 14:57 ` alvise rigo 2014-11-25 10:20 ` Claudio Fontana 2015-01-05 17:13 ` Alexander Graf 2015-01-06 8:47 ` alvise rigo 2015-01-06 11:18 ` Alexander Graf [not found] ` <1416593261-13751-5-git-send-email-a.rigo@virtualopensystems.com> 2014-11-24 10:38 ` [Qemu-devel] [RFC v2 4/4] hw/arm/virt: Add generic-pci device support Claudio Fontana 2014-11-24 10:47 ` alvise rigo 2014-11-24 15:50 ` [Qemu-devel] [RFC v2 0/4] Add Generic PCI host device update Claudio Fontana 2014-11-25 10:28 ` alvise rigo 2015-01-12 16:26 ` Claudio Fontana
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.