Re: [Minios-devel] [UNIKRAFT PATCH RFC v2 3/7] plat/common: Introduce new platform bus

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 10/24/19 5:55 AM, Justin He (Arm Technology China) wrote:
> > > +
> > > +drv = pf_find_driver(&devid);
> > > +if (!drv) {
> > > +uk_pr_info("<no driver>\n");
> > > +return -ENODEV;
> > > +}
> > > +
> > > +uk_pr_info("driver %p\n", drv);
> > > +
> > > +/* qemu creates virtio devices in reverse order */
> > Why the order matters here? What's going to happen if you are using a
> > different backend that will chose a different order?
> Please see the create_virtio_devices() in qemu codes
>      for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
>          int irq = vms->irqmap[VIRT_MMIO] + i;
>          hwaddr base = vms->memmap[VIRT_MMIO].base + i * size;
>
>          sysbus_create_simple("virtio-mmio", base, pic[irq]);
>      }
>
>      /* We add dtb nodes in reverse order so that they appear in the finished
>       * device tree lowest address first.
>       *
>       * Note that this mapping is independent of the loop above. The previous
>       * loop influences virtio device to virtio transport assignment, whereas
>       * this loop controls how virtio transports are laid out in the dtb.
>       */
>      for (i = NUM_VIRTIO_TRANSPORTS - 1; i >= 0; i--) {
>
> So qemu ^ will create max (32) virtio mmio devices and in reverse order.
This tells me what QEMU does *not* why Unikraft needs to do it.

AFAICT, both Linux and FreeBSD does need to search for virtio,mmio node 
in reverse order. So why does Unikraft need it? In other word, what will 
happen if you do the "wrong" way?
> > > +for (i = 0; i < UK_MAX_VIRTIO_MMIO_DEVICE; i++) {
> > So if you have more than UK_MAX_VIRTIO_MMIO_DEVICE, the rest will be
> > ignored. Don't you want to at least warn the users if there are more
> > than what we can support?
> Ok, I will add the warning if the number is larger than 32.
> > > +end_offset = fdt_get_last_node_by_compatible(fdt,
> > > +end_offset,
> > > +pf_device_list[0]);
> > > +if (end_offset == -FDT_ERR_NOTFOUND) {
> > > +uk_pr_info("device not found in fdt\n");
> > > +goto error_exit;
> > > +} else {
> > > +prop = fdt_getprop(fdt, end_offset, "interrupts",
> > > +   &prop_len);
> > > +if (!prop) {
> > > +uk_pr_err("irq of device not found in fdt\n");
> > > +goto error_exit;
> > > +}
> > > +
> > > +type = fdt32_to_cpu(prop[0]);
> > > +hwirq = fdt32_to_cpu(prop[1]);
> > > +
> > > +prop = fdt_getprop(fdt, end_offset, "reg", &prop_len);
> > > +if (!prop) {
> > > +uk_pr_err("reg of device not found in fdt\n");
> > > +goto error_exit;
> > > +}
> > > +
> > > +/* only care about base addr, ignore the size */
> > > +reg_base = fdt32_to_cpu(prop[0]);
> > > +reg_base = reg_base << 32 | fdt32_to_cpu(prop[1]);
> > What happen if the address is described as a 32-bit only value? I
> > thought we introduced helpers for that.
> It is problematic, thanks. I will consider to use fdt_get_address helper.
>
> > > +}
> > > +
> > > +dev_base = reg_base;
> > > +dev_irq = gic_irq_translate(type, hwirq);
> > I can't find this function in the main Unikraft repo. What is it based on?
> It is at http://xenbits.xenproject.org/gitweb/?p=unikraft/unikraft.git;a=blob;f=plat/drivers/gic/gic-v2.c;h=c01c92f6368c09ba8e405584bea44d66b0a1620d;hb=refs/heads/staging#l294 
Thanks, so Unikraft has no helper to get the interrupt and the type from 
at DT at the same time, right? If so, I think it would be worth 
considering it as you usually needs the type in hand to configure your 
interrupt.
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel