Sam Eiderman
2021-Mar-15 15:57 UTC
[Libguestfs] [PATCH libguestfs 1/1] launch: libvirt, direct: Add force_tcg backend setting.
Just noticed I replied off-list.
I will rework this commit to use the suggested enum, both in libvirt
and direct then.
Not sure if we need a single place to define it (since it's only
relevant for two of these backends).
Might just define it as:
enum { BEST_ACCEL, FORCE_TCG, FORCE_KVM } accel;
in both places as you suggested.
Do I need to worry about documentation in this commit? force_tcg is
pretty well documented.
Sam
On Mon, Mar 15, 2021 at 5:51 PM Richard W.M. Jones <rjones at redhat.com>
wrote:>
> On Mon, Mar 15, 2021 at 05:49:08PM +0200, Sam Eiderman wrote:
> > On Mon, Mar 15, 2021 at 12:38 PM Richard W.M. Jones <rjones at
redhat.com> wrote:
> > >
> > > On Sun, Mar 14, 2021 at 05:33:42PM +0200, Sam Eiderman wrote:
> > > > By using:
> > > >
> > > > export LIBGUESTFS_BACKEND_SETTINGS=force_kvm
> > > >
> > > > you can force the backend to use KVM and never fall back to
> > > > TCG (software emulation).
> > > > ---
> > > > lib/launch-direct.c | 22 +++++++++++++++++++---
> > > > lib/launch-libvirt.c | 12 +++++++++++-
> > > > 2 files changed, 30 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/lib/launch-direct.c b/lib/launch-direct.c
> > > > index b6ed9766f..79e6ef2fd 100644
> > > > --- a/lib/launch-direct.c
> > > > +++ b/lib/launch-direct.c
> > > > @@ -385,6 +385,7 @@ launch_direct (guestfs_h *g, void
*datav, const char *arg)
> > > > struct hv_param *hp;
> > > > bool has_kvm;
> > > > int force_tcg;
> > > > + int force_kvm;
> > >
> > > Wouldn't this be better as a tri-state setting? eg:
> > >
> > > enum { BEST_ACCEL, FORCE_TCG, FORCE_KVM } accel;
> > >
> > > It seems it would make the checks below simpler.
> > >
> >
> > Indeed this is the mindset, but not sure it will change much in code.
> > Parsing will have to populate that enum anyway so we will have to
> > check that both values do not collide.
>
> I don't necessarily have strong opinions one way or the other,
> but I guess one variable is better than two?
>
> Rich.
>
> >
> > > > const char *cpu_model;
> > > > CLEANUP_FREE char *append = NULL;
> > > > CLEANUP_FREE_STRING_LIST char **argv = NULL;
> > > > @@ -434,8 +435,22 @@ launch_direct (guestfs_h *g, void
*datav, const char *arg)
> > > > if (force_tcg == -1)
> > > > return -1;
> > > >
> > > > - if (!has_kvm && !force_tcg)
> > > > - debian_kvm_warning (g);
> > > > + force_kvm = guestfs_int_get_backend_setting_bool (g,
"force_kvm");
> > > > + if (force_kvm == -1)
> > > > + return -1;
> > > > +
> > > > + if (force_kvm && force_tcg) {
> > > > + error (g, "Both force_kvm and force_tcg backend
settings supplied.");
> > > > + return -1;
> > > > + }
> >
> > The following if statement will stay the same probably (just use the
> > enum values) - notice that we fallthrough in printing the warning.
> >
> > > > + if (!has_kvm) {
> > > > + if (!force_tcg)
> > > > + debian_kvm_warning (g);
> > > > + if (force_kvm) {
> > > > + error (g, "force_kvm supplied but kvm not
available.");
> > > > + return -1;
> > > > + }
> > > > + }
> > > >
> > > > /* Using virtio-serial, we need to create a local Unix
domain socket
> > > > * for qemu to connect to.
> > > > @@ -530,7 +545,8 @@ launch_direct (guestfs_h *g, void
*datav, const char *arg)
> > > > if (has_kvm && !force_tcg)
> > > > append_list ("gic-version=host");
> > > > #endif
> >
> > This might be slightly simplified - but not sure I would add an enum
> > for that, no strong opinions though.
> >
> > > > - append_list_format ("accel=%s", !force_tcg ?
"kvm:tcg" : "tcg");
> > > > + append_list_format ("accel=%s", force_kvm ?
"kvm" :
> > > > + (!force_tcg ?
"kvm:tcg" : "tcg"));
> > > > } end_list ();
> > > >
> > > > cpu_model = guestfs_int_get_cpu_model (has_kvm &&
!force_tcg);
> > > > diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c
> > > > index 6c0cfe937..a56afbdd4 100644
> > > > --- a/lib/launch-libvirt.c
> > > > +++ b/lib/launch-libvirt.c
> > > > @@ -773,6 +773,7 @@ parse_capabilities (guestfs_h *g, const
char *capabilities_xml,
> > > > xmlAttrPtr attr;
> > > > size_t seen_qemu, seen_kvm;
> > > > int force_tcg;
> > > > + int force_kvm;
> > > >
> > > > doc = xmlReadMemory (capabilities_xml, strlen
(capabilities_xml),
> > > > NULL, NULL, XML_PARSE_NONET);
> > > > @@ -820,11 +821,15 @@ parse_capabilities (guestfs_h *g,
const char *capabilities_xml,
> > > > }
> > > > }
> > > >
> > > > + force_kvm = guestfs_int_get_backend_setting_bool (g,
"force_kvm");
> > > > + if (force_kvm == -1)
> > > > + return -1;
> > > > +
> > > > /* This was RHBZ#886915: in that case the default libvirt
URI
> > > > * pointed to a Xen hypervisor, and so could not create
the
> > > > * appliance VM.
> > > > */
> > > > - if (!seen_qemu && !seen_kvm) {
> > > > + if ((!seen_qemu || force_kvm) && !seen_kvm) {
> >
> > If force_kvm is set to true, we enter this if "!seen_kvm" -
and fail
> >
> > > > CLEANUP_FREE char *backend = guestfs_get_backend (g);
> > > >
> > > > error (g,
> > > > @@ -846,6 +851,11 @@ parse_capabilities (guestfs_h *g, const
char *capabilities_xml,
> > > > if (force_tcg == -1)
> > > > return -1;
> > > >
> > > > + if (force_kvm && force_tcg) {
> > > > + error (g, "Both force_kvm and force_tcg backend
settings supplied.");
> > > > + return -1;
> > > > + }
> > > > +
> > > > if (!force_tcg)
> >
> > That means that seen_kvm = TRUE here
> > That means that data->is_kvm = TRUE here
> >
> > > > data->is_kvm = seen_kvm;
> > > > else
> > >
> > > Where does this actually set KVM only in the libvirt backend?
> >
> > Commented above
> >
> > >From what I understand this is later translated to "kvm"
under
> > domain->type in libvirt xml.
> >
> > I actually needed this only for direct, but thought that I should add
> > it to libvirt too since that what was done for force_tcg
> >
> > >
> > > Rich.
> > >
> > > --
> > > Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> > > Read my programming and virtualization blog:
http://rwmj.wordpress.com
> > > virt-df lists disk usage of guests without needing to install any
> > > software inside the virtual machine. Supports Linux and Windows.
> > > http://people.redhat.com/~rjones/virt-df/
> > >
>
> --
> Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-p2v converts physical machines to virtual machines. Boot with a
> live CD or over the network (PXE) and turn machines into KVM guests.
> http://libguestfs.org/virt-v2v
>
Richard W.M. Jones
2021-Mar-15 16:39 UTC
[Libguestfs] [PATCH libguestfs 1/1] launch: libvirt, direct: Add force_tcg backend setting.
On Mon, Mar 15, 2021 at 05:57:14PM +0200, Sam Eiderman wrote:> Just noticed I replied off-list. > > I will rework this commit to use the suggested enum, both in libvirt > and direct then. > Not sure if we need a single place to define it (since it's only > relevant for two of these backends). > Might just define it as: > enum { BEST_ACCEL, FORCE_TCG, FORCE_KVM } accel; > in both places as you suggested.Could it go in lib/guestfs-internal.h ?> Do I need to worry about documentation in this commit? force_tcg is > pretty well documented.Worth adding it to the man page I think (lib/guestfs.pod). RIch. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/