Re: [PATCH V9 1/2] libxl: Add support for Virtio disk configuration

[Oleksandr <olekstysh@xxxxxxxxx>]

On 10.06.22 20:18, Oleksandr wrote:

Hello Anthony

> On 10.06.22 18:12, Anthony PERARD wrote:
>
> Hello Anthony
>
> > On Wed, Jun 01, 2022 at 08:57:40PM +0300, Oleksandr Tyshchenko wrote:
> > > diff --git a/tools/libs/light/libxl_disk.c 
> > > b/tools/libs/light/libxl_disk.c
> > > index a5ca778..e90bc25 100644
> > > --- a/tools/libs/light/libxl_disk.c
> > > +++ b/tools/libs/light/libxl_disk.c
> > > @@ -163,6 +163,25 @@ static int 
> > > libxl__device_disk_setdefault(libxl__gc *gc, uint32_t domid,
> > >       rc = libxl__resolve_domid(gc, disk->backend_domname, 
> > > &disk->backend_domid);
> > >       if (rc < 0) return rc;
> > >   +    if (disk->specification == LIBXL_DISK_SPECIFICATION_UNKNOWN)
> > > +        disk->specification = LIBXL_DISK_SPECIFICATION_XEN;
> > > +
> > > +    if (disk->specification == LIBXL_DISK_SPECIFICATION_XEN &&
> > > +        disk->transport != LIBXL_DISK_TRANSPORT_UNKNOWN) {
> > > +        LOGD(ERROR, domid, "Transport is only supported for 
> > > specification virtio");
> > > +        return ERROR_FAIL;
> > Could you return ERROR_INVAL instead?
> yes
>
>
> > > +    }
> > > +
> > > +    /* Force transport mmio for specification virtio for now */
> > > +    if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
> > > +        if (!(disk->transport == LIBXL_DISK_TRANSPORT_UNKNOWN ||
> > > +              disk->transport == LIBXL_DISK_TRANSPORT_MMIO)) {
> > > +            LOGD(ERROR, domid, "Unsupported transport for 
> > > specification virtio");
> > > +            return ERROR_FAIL;
> > Same here.
> yes
>
>
> > > +        }
> > > +        disk->transport = LIBXL_DISK_TRANSPORT_MMIO;
> > > +    }
> > > +
> > >       /* Force Qdisk backend for CDROM devices of guests with a 
> > > device model. */
> > >       if (disk->is_cdrom != 0 &&
> > >           libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
> > > @@ -575,6 +660,41 @@ cleanup:
> > >       return rc;
> > >   }
> > >   +static int libxl__device_disk_get_path(libxl__gc *gc, uint32_t 
> > > domid,
> > > +                                       char **path)
> > > +{
> > > +    const char *dir;
> > > +    int rc;
> > > +
> > > +    /*
> > > +     * As we don't know exactly what device kind to be used here, 
> > > guess it
> > > +     * by checking the presence of the corresponding path in Xenstore.
> > > +     * First, try to read path for vbd device (default) and if not 
> > > exists
> > > +     * read path for virtio_disk device. This will work as long as 
> > > both Xen PV
> > > +     * and Virtio disk devices are not assigned to the same guest.
> > > +     */
> > > +    *path = GCSPRINTF("%s/device/%s",
> > > +                      libxl__xs_libxl_path(gc, domid),
> > > + libxl__device_kind_to_string(LIBXL__DEVICE_KIND_VBD));
> > > +
> > > +    rc = libxl__xs_read_checked(gc, XBT_NULL, *path, &dir);
> > > +    if (rc)
> > > +        return rc;
> > > +
> > > +    if (dir)
> > > +        return 0;
> > > +
> > > +    *path = GCSPRINTF("%s/device/%s",
> > > +                      libxl__xs_libxl_path(gc, domid),
> > > + libxl__device_kind_to_string(LIBXL__DEVICE_KIND_VIRTIO_DISK));
> > > +
> > > +    rc = libxl__xs_read_checked(gc, XBT_NULL, *path, &dir);
> > > +    if (rc)
> > > +        return rc;
> > > +
> > > +    return 0;
> > I still don't like this implementation of get_path() which return a
> > different answer depending on the environment which can change from one
> > call to the next. I think get_path() was introduced when the path for
> > the kind of device didn't correspond to the common path which other kind
> > of devices uses. And when get_path() is implemented, it always returns
> > the same answer (see libxl_pci.c for the only implementation).
> >
> > I don't really know how to deal with a type of device that have two
> > different frontend kind at the moment. (But maybe there's something in
> > libxl_usb.c which could be useful as a potential example, but one of the
> > kind doesn't use xenstore so is probably easier to deal with.). So I
> > guess we are stuck with an implementation of get_path() which may return
> > a different answer depending on thing outside of libxl's full control.
> >
> > So, could you at least make it much harder to have libxl's view of a
> > guest in a weird state? I mean:
> > - always check both xenstore paths
> >    -> if both exist, return an error
> >    -> if only one exist, return that one.
> >    -> default to the "vbd" kind, otherwise.
> I think I got your idea, will try to do this way
I hope that this (it seems working) function follows your suggestion. If 
no objections, I will use it for V10.

static int libxl__device_disk_get_path(libxl__gc *gc, uint32_t domid,
                                       char **path)
{
    const char *xen_dir, *virtio_dir;
    char *xen_path, *virtio_path;
    int rc;

    xen_path = GCSPRINTF("%s/device/%s",
                         libxl__xs_libxl_path(gc, domid),
libxl__device_kind_to_string(LIBXL__DEVICE_KIND_VBD));

    rc = libxl__xs_read_checked(gc, XBT_NULL, xen_path, &xen_dir);
    if (rc)
        return rc;

    virtio_path = GCSPRINTF("%s/device/%s",
                            libxl__xs_libxl_path(gc, domid),
libxl__device_kind_to_string(LIBXL__DEVICE_KIND_VIRTIO_DISK));

    rc = libxl__xs_read_checked(gc, XBT_NULL, virtio_path, &virtio_dir);
    if (rc)
        return rc;

    if (xen_dir && virtio_dir) {
        LOGD(ERROR, domid, "Invalid configuration, both xen and virtio 
paths are present");
        return ERROR_INVAL;
    } else if (virtio_dir)
        *path = virtio_path;
    else
        *path = xen_path;

    return 0;
}


> > That would be better than the current implementation which returns the
> > "virtio" path by default.
> agree,
>
> making virtio by default wasn't the original intention, it was 
> entirely involuntarily))
>
> > Thanks,
--
Regards,

Oleksandr Tyshchenko