| Message ID | 1416593261-13751-2-git-send-email-a.rigo@virtualopensystems.com |
|---|---|
| State | New |
| Headers | show |
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 = { >
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
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
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
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
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
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 >
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 >> >
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
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
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 >
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 = {
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(-)