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: Friday, October 25, 2019 5:12 PM
> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx
> 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>;
> Santiago.Pagani@xxxxxxxxx
> Subject: Re: [UNIKRAFT PATCH RFC v2 3/7] plat/common: Introduce new
> platform bus
> 
> Hi,
> 
> On 25/10/2019 01:12, Justin He (Arm Technology China) wrote:
> >> -----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
> 
> QEMU actually gives a good example why relying on ordering may lead to
> unwanted
> behavior:
> 
>      /* We create the transports in forwards order. Since qbus_realize()
>       * prepends (not appends) new child buses, the incrementing loop below
> will
>       * create a list of virtio-mmio buses with decreasing base addresses.
>       *
>       * When a -device option is processed from the command line,
>       * qbus_find_recursive() picks the next free virtio-mmio bus in forwards
>       * order. The upshot is that -device options in increasing command line
>       * order are mapped to virtio-mmio buses with decreasing base addresses.
>       *
>       * When this code was originally written, that arrangement ensured that
> the
>       * guest Linux kernel would give the lowest "name" (/dev/vda, eth0, etc)
> to
>       * the first -device on the command line. (The end-to-end order is a
>       * function of this loop, qbus_realize(), qbus_find_recursive(), and the
>       * guest kernel's name-to-address assignment strategy.)
>       *
>       * Meanwhile, the kernel's traversal seems to have been reversed; see eg.
>       * the message, if not necessarily the code, of commit 70161ff336.
>       * Therefore the loop now establishes the inverse of the original intent.
>       *
>       * Unfortunately, we can't counteract the kernel change by reversing the
>       * loop; it would break existing command lines.
>       *
>       * In any case, the kernel makes no guarantee about the stability of
>       * enumeration order of virtio devices (as demonstrated by it changing
>       * between kernel versions). For reliable and stable identification
>       * of disks users must use UUIDs or similar mechanisms.
>       */
> 
> More generally, Device-Tree does not guarantee that devices will be laid out
> in
> the correct order. For instance, if you are using a non-QEMU backend you
> may
> have a different order in the device-tree.
> 
> As this is common code, you definitely can't rely on ordering. But it would
> also
> clearly be wrong if you implement the probe ordering based on your
> backend.
> 
> In your solution, can you explain how you expect Unikraft to find a virtio
> device?

Thanks, yes you are right. I read the platform device probing process in kernel.
It uses VIRTIO_MMIO_DEVICE_ID to judge whether it is a existed or dummy device.
readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);

I will consider to use that similar logic( in normal order).

--
Cheers,
Justin (Jia He)


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