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

["Justin He (Arm Technology China)" <Justin.He@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: Thursday, October 24, 2019 4:35 PM
> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; Santiago.Pagani@xxxxxxxxx
> Cc: nd <nd@xxxxxxx>; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>;
> Sharan.Santhanam@xxxxxxxxx; Felipe Huici <felipe.huici@xxxxxxxxx>; Kaly
> Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>
> Subject: Re: [UNIKRAFT PATCH RFC v2 3/7] plat/common: Introduce new
> platform bus
> 
> 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?

Sorry, I don't quite understand your concerns here.
If Unikraft didn’t search it in reverse order, how can it probe the correct 
Virtio-mmio device? Could you please elaborate it more, thanks

> >
> >>
> >>> +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.

Ok, I can provide a new helper to get the hwirq and type at the same time.

--
Cheers,
Justin (Jia He)


> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel