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, thanks for the review, please see my replies inline

> -----Original Message-----
> From: Julien Grall <Julien.Grall@xxxxxxx>
> Sent: Wednesday, October 23, 2019 3:54 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 Jia,
> 
> On 23/10/2019 04:51, Jia He wrote:
> > +static int pf_probe(void)
> > +{
> > +   struct pf_device_id devid;
> > +   struct pf_driver *drv;
> > +   int i;
> > +   int end_offset = -1;
> > +   int ret = -ENODEV;
> 
> IHMO, this is valid to have no devices under a bus and therefore not
> return an error. Looking at other usage, I think you want to return 0 here.

Ok
> 
> > +   const fdt32_t *prop;
> > +   int type, hwirq, prop_len;
> > +   __u64 reg_base;
> > +   __phys_addr dev_base;
> > +   int dev_irq;
> > +   const void *fdt = _libkvmplat_cfg.dtb;
> > +
> > +   uk_pr_info("Probe PF\n");
> > +
> > +   /* We only support virtio_mmio as a platform device here.
> > +    * A loop here is needed for finding drivers if more devices
> > +    */
> > +   devid.device_id = VIRTIO_MMIO_ID;
> 
> This feels a bit strange to have virtio specific code in a file called
> "platform_bus.c". Don't you want to abstract this a bit further?

Yes, let me consider it more clearly
> 
> > +
> > +   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.

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

--
Cheers,
Justin (Jia He)


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