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

[Julien Grall <Julien.Grall@xxxxxxx>]

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.

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

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

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

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


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

[...]

Cheers,

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