Re: [PATCH V4 24/24] [RFC] libxl: Add support for virtio-disk configuration

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

Oleksandr Tyshchenko writes ("[PATCH V4 24/24] [RFC] libxl: Add support for 
virtio-disk configuration"):
> This patch adds basic support for configuring and assisting virtio-disk
> backend (emualator) which is intended to run out of Qemu and could be run
> in any domain.

Thanks.  I think this is a very important feature.  But I think this
part at least needs some work.  (That's not inappropriate for an RFC
patch - so please don't feel you have done anything wrong.  I hope you
will find my comments constructive.)


> An example of domain configuration (two disks are assigned to the guest,
> the latter is in readonly mode):
> 
> vdisk = [ 'backend=DomD, disks=rw:/dev/mmcblk0p3;ro:/dev/mmcblk1p3' ]

I can see why you have done it like this but I am concerned that this
is not well-integrated with the existing disk configuration system.

As a result not only is your new feature lacking support for many
existing libxl features (block backend scripts, cdroms tagged as such,
non-raw formats) that could otherwise be made available, but I think
adding them later would be quite awkward.

I it would be better to reuse (and, if necessary, adapt) the existing
disk parsing logic in libxl, so that the syntax for your new vdisks =
[...] parameter is the same as for the existing disks.  Or even
better, simply make your new kind of disk a new flag on the existing
disk structure.

> Also there is one suggestion from Wei Chen regarding a parameter for
> domain config file which I haven't addressed yet.
> [Just copy here what Wei said in V2 thread]
> Can we keep use the same 'disk' parameter for virtio-disk, but add
> an option like "model=virtio-disk"?
> For example:
> disk = [ 'backend=DomD, disks=rw:/dev/mmcblk0p3,model=virtio-disk' ]
> Just like what Xen has done for x86 virtio-net.

This is the same suggestion I make above, basically.  It would be much
better, yes.


> Xenstore was chosen as a communication interface for the emulator
> running in non-toolstack domain to be able to get configuration
> either by reading Xenstore directly or by receiving command line
> parameters (an updated 'xl devd' running in the same domain would
> read Xenstore beforehand and call backend executable with the
> required arguments).

I was surprised to read this because I would expect that qemu upstream
would be resistant to this approach.  As far as the Xen Project's
point of view goes, I think using xenstore for this is fine, but we
would definitely want the support in upstream qemu.

Can you please explain the status of the corresponding qemu feature ?
(Ideally, in a formal way in the commit message.)

> Please note, there is a real concern about VirtIO interrupts allocation.
> [Just copy here what Stefano said in RFC thread]
> 
> So, if we end up allocating let's say 6 virtio interrupts for a
> domain, the chance of a clash with a physical interrupt of a
> passthrough device is real.
> 
> I am not entirely sure how to solve it, but these are a few ideas:
> - choosing virtio interrupts that are less likely to conflict (maybe > 1000)
> - make the virtio irq (optionally) configurable so that a user could
>   override the default irq and specify one that doesn't conflict
> - implementing support for virq != pirq (even the xl interface doesn't
>   allow to specify the virq number for passthrough devices, see "irqs")

I think here you have chosen to make the interupt configurable ?

The implications are that a someone using this with passthrough would
have to choose non-clashing IRQs ?  In the non-passthrough case (ie, a
guest with no passthrough devices), can your code choose an
appropriate IRQ, if the user doesn't specify one ?


I don't see any changes to the xl documentation in this patch.  That
would be the place to explain the irq stuff, and would be needed
anyway.  Indeed with anything substantial like your proposal, it is
often a good idea to write (at least a sketch of) the documentation
*first*, and then you know what you're aiming to implement.


I have some comments on the code details but I think you will probably
want to focus on the overall approach, first:

> +#ifndef container_of
> +#define container_of(ptr, type, member) ({                   \
> +        typeof( ((type *)0)->member ) *__mptr = (ptr);       \
> +        (type *)( (char *)__mptr - offsetof(type,member) );})
> +#endif

Please use the existing CONTAINER_OF which we have already.

>  static const char *gicv_to_string(libxl_gic_version gic_version)
>  {
>      switch (gic_version) {
> @@ -39,14 +45,32 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>          vuart_enabled = true;
>      }
>  
> -    /*
> -     * XXX: Handle properly virtio
> -     * A proper solution would be the toolstack to allocate the interrupts
> -     * used by each virtio backend and let the backend now which one is used
> -     */
>      if (libxl_defbool_val(d_config->b_info.arch_arm.virtio)) {
> -        nr_spis += (GUEST_VIRTIO_MMIO_SPI - 32) + 1;
> +        uint64_t virtio_base;
> +        libxl_device_virtio_disk *virtio_disk;
> +
> +        virtio_base = GUEST_VIRTIO_MMIO_BASE;
>          virtio_irq = GUEST_VIRTIO_MMIO_SPI;

I would like to see a review of these changes to virtio handling by
someone who understands virtio.

> +static int libxl__device_virtio_disk_setdefault(libxl__gc *gc, uint32_t 
> domid,
> +                                                libxl_device_virtio_disk 
> *virtio_disk,
> +                                                bool hotplug)
> +{
> +    return libxl__resolve_domid(gc, virtio_disk->backend_domname,
> +                                &virtio_disk->backend_domid);

There are some line length problems here.

I haven't reviewed your parsing code because I think this ought to be
done as an option or addition to with the existing disk spec parsing.

> diff --git a/tools/xl/xl_virtio_disk.c b/tools/xl/xl_virtio_disk.c
> new file mode 100644
> index 0000000..808a7da
> --- /dev/null
> +++ b/tools/xl/xl_virtio_disk.c
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright (C) 2020 EPAM Systems Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include <stdlib.h>
> +
> +#include <libxl.h>
> +#include <libxl_utils.h>
> +#include <libxlutil.h>
> +
> +#include "xl.h"
> +#include "xl_utils.h"
> +#include "xl_parse.h"
> +
> +int main_virtio_diskattach(int argc, char **argv)
> +{
> +    return 0;
> +}
> +
> +int main_virtio_disklist(int argc, char **argv)
> +{
> +   return 0;
> +}
> +
> +int main_virtio_diskdetach(int argc, char **argv)
> +{
> +    return 0;
> +}

This seems to be a stray early test file left over in the patch ?


Thanks,
Ian.