Re: [PATCH] Account for fact that virDomainDeviceDefCopy() does an inactive copy

[Laine Stump]

On 1/4/22 11:59 AM, Michal Privoznik wrote:

In a few places (e.g. device attach/detach/update) we are given a
device XML, parse it but then need a copy of parsed data so that
the original can be passed to function handling the request over
inactive XML and the copy is then passed to function handling the
operation over live XML. Note, both functions consume passed
device on success, hence the need for copy.

The problem is in combination of how the copy is obtained and
where is passed. The copy is done by calling
virDomainDeviceDefCopy() which does only inactive copy, i.e. no
live information is copied over (e.g. no aliases).

Then, this copy (inactive XML effectively) is passed to function
handling live part of the operation (e.g.
qemuDomainUpdateDeviceLive()) and the definition containing all
the juicy, live bits is passed to function handling inactive part
of the operation (e.g. qemuDomainUpdateDeviceConfig()).

This is rather suboptimal, and XML copies should be passed to
their respective functions.


s/suboptimal/incorrect/   :-)

It's funny that the comment at the top of virDomainDeviceDefCopy 
explicitly says that it's a copy of the inactive/config object, and that 
every single place that function is called, its result is being put into 
the live domain :-P


At first I was concerned that there were no matching chunks for 
qemuDomainAttachDeviceFlags(), but then I took a look and realized that 
function parses the XML twice instead of parsing it once and making a 
copy (and I suppose I should mention the moment of cringe when, while 
looking at that function, I came across some piece of ugly treachery I 
had added to make sure the MAC addresses match between the copies. It 
seems like this cringe moment happens every time I open a file these 
days...)




Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2036895
Signed-off-by: Michal Privoznik 



Reviewed-by: Laine Stump 



---
  src/lxc/lxc_driver.c   | 10 +-
  src/qemu/qemu_driver.c |  8 
  2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index fe583ccb76..7bc39120ee 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -4308,17 +4308,17 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
   false) < 0)
  goto endjob;
  
-if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0)

+if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev_copy)) < 0)
  goto endjob;
  }
  
  if (flags & VIR_DOMAIN_AFFECT_LIVE) {

-if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
+if (virDomainDefCompatibleDevice(vm->def, dev, NULL,
   VIR_DOMAIN_DEVICE_ACTION_ATTACH,
   true) < 0)
  goto endjob;
  
-if ((ret = lxcDomainAttachDeviceLive(driver, vm, dev_copy)) < 0)

+if ((ret = lxcDomainAttachDeviceLive(driver, vm, dev)) < 0)
  goto endjob;
  /*
   * update domain status forcibly because the domain status may be
@@ -4475,12 +4475,12 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom,
  if (!vmdef)
  goto endjob;
  
-if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev)) < 0)

+if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev_copy)) < 0)
  goto endjob;
  }
  
  if (flags & VIR_DOMAIN_AFFECT_LIVE) {

-if ((ret = lxcDomainDetachDeviceLive(driver, vm, dev_copy)) < 0)
+if ((ret = lxcDomainDetachDeviceLive(driver, vm, dev)) < 0)
  goto endjob;
  /*
   * update domain status forcibly because the domain status may be
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b8537a4260..b1255da9f2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8020,7 +8020,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
  
  /* virDomainDefCompatibleDevice call is delayed until we know the

   * device we're going to update. */
-if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev, priv->qemuCaps,
+if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev_copy, 
priv->qemuCaps,
  parse_flags,
  driver->xmlopt)) < 0)
  goto endjob;
@@ -8029,7 +8029,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
  if (flags & VIR_DOMAIN_AFFECT_LIVE) {
  /* virDomainDefCompatibleDevice call is delayed until we know the
   * device we're going to update. */
-if ((ret = qemuDomainUpdateDeviceLive(vm, dev_copy, dom, force)) < 0)
+if ((ret = qemuDomainUpdateDeviceLive(vm, dev, dom, force)) < 0)
  goto endjob;
  
  qemuDomainSaveStatus(vm);

@@ -8100,7 +8100,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriver *driver,
  if 

Re: [PATCH v3 12/12] target/riscv: Support virtual time context synchronization

[Alistair Francis]

On Tue, Dec 21, 2021 at 2:44 AM Yifei Jiang via  wrote:
>
> Add virtual time context description to vmstate_kvmtimer. After cpu being
> loaded, virtual time context is updated to KVM.
>
> Signed-off-by: Yifei Jiang 
> Signed-off-by: Mingwang Li 
> Reviewed-by: Anup Patel 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/machine.c | 30 ++
>  1 file changed, 30 insertions(+)
>
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index ad8248ebfd..95eb82792a 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -164,6 +164,35 @@ static const VMStateDescription vmstate_pointermasking = 
> {
>  }
>  };
>
> +static bool kvmtimer_needed(void *opaque)
> +{
> +return kvm_enabled();
> +}
> +
> +static int cpu_post_load(void *opaque, int version_id)
> +{
> +RISCVCPU *cpu = opaque;
> +CPURISCVState *env = >env;
> +
> +env->kvm_timer_dirty = true;
> +return 0;
> +}
> +
> +static const VMStateDescription vmstate_kvmtimer = {
> +.name = "cpu/kvmtimer",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.needed = kvmtimer_needed,
> +.post_load = cpu_post_load,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT64(env.kvm_timer_time, RISCVCPU),
> +VMSTATE_UINT64(env.kvm_timer_compare, RISCVCPU),
> +VMSTATE_UINT64(env.kvm_timer_state, RISCVCPU),
> +
> +VMSTATE_END_OF_LIST()
> +}
> +};
> +
>  const VMStateDescription vmstate_riscv_cpu = {
>  .name = "cpu",
>  .version_id = 3,
> @@ -218,6 +247,7 @@ const VMStateDescription vmstate_riscv_cpu = {
>  _hyper,
>  _vector,
>  _pointermasking,
> +_kvmtimer,
>  NULL
>  }
>  };
> --
> 2.19.1
>
>

Re: [PATCH v3 11/12] target/riscv: Implement virtual time adjusting with vm state changing

[Alistair Francis]

On Tue, Dec 21, 2021 at 3:45 AM Yifei Jiang via  wrote:
>
> We hope that virtual time adjusts with vm state changing. When a vm
> is stopped, guest virtual time should stop counting and kvm_timer
> should be stopped. When the vm is resumed, guest virtual time should
> continue to count and kvm_timer should be restored.
>
> Signed-off-by: Yifei Jiang 
> Signed-off-by: Mingwang Li 
> Reviewed-by: Anup Patel 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/kvm.c | 14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 3c20ec5ad3..6c0306bd2b 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -42,6 +42,7 @@
>  #include "chardev/char-fe.h"
>  #include "semihosting/console.h"
>  #include "migration/migration.h"
> +#include "sysemu/runstate.h"
>
>  static uint64_t kvm_riscv_reg_id(CPURISCVState *env, uint64_t type, uint64_t 
> idx)
>  {
> @@ -378,6 +379,17 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu)
>  return cpu->cpu_index;
>  }
>
> +static void kvm_riscv_vm_state_change(void *opaque, bool running, RunState 
> state)
> +{
> +CPUState *cs = opaque;
> +
> +if (running) {
> +kvm_riscv_put_regs_timer(cs);
> +} else {
> +kvm_riscv_get_regs_timer(cs);
> +}
> +}
> +
>  void kvm_arch_init_irq_routing(KVMState *s)
>  {
>  }
> @@ -390,6 +402,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  CPURISCVState *env = >env;
>  uint64_t id;
>
> +qemu_add_vm_change_state_handler(kvm_riscv_vm_state_change, cs);
> +
>  id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG, 
> KVM_REG_RISCV_CONFIG_REG(isa));
>  ret = kvm_get_one_reg(cs, id, );
>  if (ret) {
> --
> 2.19.1
>
>

Re: [PATCH v3 08/12] target/riscv: Handle KVM_EXIT_RISCV_SBI exit

[Alistair Francis]

On Tue, Dec 21, 2021 at 3:41 AM Yifei Jiang via  wrote:
>
> Use char-fe to handle console sbi call, which implement early
> console io while apply 'earlycon=sbi' into kernel parameters.
>
> Signed-off-by: Yifei Jiang 
> Signed-off-by: Mingwang Li 
> Reviewed-by: Anup Patel 
> ---
>  target/riscv/kvm.c | 43 +-
>  target/riscv/sbi_ecall_interface.h | 72 ++
>  2 files changed, 114 insertions(+), 1 deletion(-)
>  create mode 100644 target/riscv/sbi_ecall_interface.h
>
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 0027f11f45..4d08669c81 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -38,6 +38,9 @@
>  #include "qemu/log.h"
>  #include "hw/loader.h"
>  #include "kvm_riscv.h"
> +#include "sbi_ecall_interface.h"
> +#include "chardev/char-fe.h"
> +#include "semihosting/console.h"
>
>  static uint64_t kvm_riscv_reg_id(CPURISCVState *env, uint64_t type, uint64_t 
> idx)
>  {
> @@ -365,9 +368,47 @@ bool kvm_arch_stop_on_emulation_error(CPUState *cs)
>  return true;
>  }
>
> +static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
> +{
> +int ret = 0;
> +unsigned char ch;
> +switch (run->riscv_sbi.extension_id) {
> +case SBI_EXT_0_1_CONSOLE_PUTCHAR:
> +ch = run->riscv_sbi.args[0];
> +qemu_semihosting_log_out((const char *), sizeof(ch));

Hmmm... We print to the semihosting

> +break;
> +case SBI_EXT_0_1_CONSOLE_GETCHAR:
> +ret = qemu_chr_fe_read_all(serial_hd(0)->be, , sizeof(ch));

but then read from the first serial device.

That seems a little strange. Would it be better to just print to serial as well?

Alistair

> +if (ret == sizeof(ch)) {
> +run->riscv_sbi.args[0] = ch;
> +} else {
> +run->riscv_sbi.args[0] = -1;
> +}
> +break;
> +default:
> +qemu_log_mask(LOG_UNIMP,
> +  "%s: un-handled SBI EXIT, specific reasons is %lu\n",
> +  __func__, run->riscv_sbi.extension_id);
> +ret = -1;
> +break;
> +}
> +return ret;
> +}
> +
>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>  {
> -return 0;
> +int ret = 0;
> +switch (run->exit_reason) {
> +case KVM_EXIT_RISCV_SBI:
> +ret = kvm_riscv_handle_sbi(cs, run);
> +break;
> +default:
> +qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
> +  __func__, run->exit_reason);
> +ret = -1;
> +break;
> +}
> +return ret;
>  }
>
>  void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
> diff --git a/target/riscv/sbi_ecall_interface.h 
> b/target/riscv/sbi_ecall_interface.h
> new file mode 100644
> index 00..fb1a3fa8f2
> --- /dev/null
> +++ b/target/riscv/sbi_ecall_interface.h
> @@ -0,0 +1,72 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + *
> + * Authors:
> + *   Anup Patel 
> + */
> +
> +#ifndef __SBI_ECALL_INTERFACE_H__
> +#define __SBI_ECALL_INTERFACE_H__
> +
> +/* clang-format off */
> +
> +/* SBI Extension IDs */
> +#define SBI_EXT_0_1_SET_TIMER   0x0
> +#define SBI_EXT_0_1_CONSOLE_PUTCHAR 0x1
> +#define SBI_EXT_0_1_CONSOLE_GETCHAR 0x2
> +#define SBI_EXT_0_1_CLEAR_IPI   0x3
> +#define SBI_EXT_0_1_SEND_IPI0x4
> +#define SBI_EXT_0_1_REMOTE_FENCE_I  0x5
> +#define SBI_EXT_0_1_REMOTE_SFENCE_VMA   0x6
> +#define SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID 0x7
> +#define SBI_EXT_0_1_SHUTDOWN0x8
> +#define SBI_EXT_BASE0x10
> +#define SBI_EXT_TIME0x54494D45
> +#define SBI_EXT_IPI 0x735049
> +#define SBI_EXT_RFENCE  0x52464E43
> +#define SBI_EXT_HSM 0x48534D
> +
> +/* SBI function IDs for BASE extension*/
> +#define SBI_EXT_BASE_GET_SPEC_VERSION   0x0
> +#define SBI_EXT_BASE_GET_IMP_ID 0x1
> +#define SBI_EXT_BASE_GET_IMP_VERSION0x2
> +#define SBI_EXT_BASE_PROBE_EXT  0x3
> +#define SBI_EXT_BASE_GET_MVENDORID  0x4
> +#define SBI_EXT_BASE_GET_MARCHID0x5
> +#define SBI_EXT_BASE_GET_MIMPID 0x6
> +
> +/* SBI function IDs for TIME extension*/
> +#define SBI_EXT_TIME_SET_TIMER  0x0
> +
> +/* SBI function IDs for IPI extension*/
> +#define SBI_EXT_IPI_SEND_IPI0x0
> +
> +/* SBI function IDs for RFENCE extension*/
> +#define SBI_EXT_RFENCE_REMOTE_FENCE_I   0x0
> +#define SBI_EXT_RFENCE_REMOTE_SFENCE_VMA0x1
> +#define SBI_EXT_RFENCE_REMOTE_SFENCE_VMA_ASID  0x2
> +#define SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA   0x3
> +#define SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA_VMID 0x4
> +#define SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA   0x5
> +#define SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA_ASID 0x6
> +
> +/* SBI function IDs for HSM extension */
> +#define SBI_EXT_HSM_HART_START  0x0
> +#define SBI_EXT_HSM_HART_STOP   0x1
> +#define 

Re: [libvirt PATCH v3 00/13] cgroup and thread management in ch driver.

[Praveen K Paladugu]

Folks,
I'd like to get this patch set in next release. Bubbling up this thread
as it was sent out before the holidays.

Regards,
Praveen K Paladugu

On 12/10/2021 2:34 PM, Praveen K Paladugu wrote:

This patchset adds support for cgroup management of ch threads. This version
correctly manages cgroups for vcpu and emulator threads created by ch. cgroup
management for iothreads is not yet supported.

Along with cgroup management, this patchset also enables support for pinning
vcpu and emulator threads to selected host cpus.

v3:
* addrressed all the formatting comments in v2 patch set
* dropped indentation patches are they do not adhere to libvirt coding style
* fixed build issue in qemu driver that was introduced in v2

Praveen K Paladugu (5):
   util: Helper functions to get process info
   ch_driver,ch_domain: vcpu info getter callbacks
   qemu,hypervisor: refactor some cgroup mgmt methods
   ch_process: Setup emulator and iothread settings
   ch_driver: emulator threadinfo & pinning callbacks

Vineeth Pillai (8):
   ch_domain: add virCHDomainGetMonitor helper method
   ch_domain: add methods to manage private vcpu data
   ch_driver: domainGetVcpuPinInfo and nodeGetCPUMap
   ch_monitor: Get nicindexes in prep for cgroup mgmt
   ch: methods for cgroup mgmt in ch driver
   ch_driver,ch_domain: vcpupin callback in ch driver
   ch_driver: enable typed param string for numatune
   ch_driver: add numatune callbacks for CH driver

  src/ch/ch_conf.c   |   2 +
  src/ch/ch_conf.h   |   6 +-
  src/ch/ch_domain.c | 172 ++-
  src/ch/ch_domain.h |  32 +-
  src/ch/ch_driver.c | 789 +
  src/ch/ch_monitor.c| 341 +++---
  src/ch/ch_monitor.h|  60 ++-
  src/ch/ch_process.c| 385 +++-
  src/ch/ch_process.h|   3 +
  src/ch/meson.build |   1 +
  src/hypervisor/domain_cgroup.c | 426 +-
  src/hypervisor/domain_cgroup.h |  52 +++
  src/libvirt_private.syms   |  15 +
  src/qemu/qemu_cgroup.c | 410 +
  src/qemu/qemu_cgroup.h |  11 -
  src/qemu/qemu_driver.c | 130 +-
  src/qemu/qemu_hotplug.c|   7 +-
  src/qemu/qemu_process.c|  20 +-
  src/util/virprocess.c  | 108 +
  src/util/virprocess.h  |   5 +
  20 files changed, 2357 insertions(+), 618 deletions(-)

[libvirt PATCH] util: fix prototype of virDaemonSetupLogging

[Ján Tomko]

The commit that added error checking to this function
forgot to adjust the WIN32 stub.

Fixes: a873924e36b28c5b125621e35b32beb6b077bcc8
Signed-off-by: Ján Tomko 
---
Pushed as a build fix.

 src/util/virdaemon.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/util/virdaemon.c b/src/util/virdaemon.c
index 5c0ca534cf..00bd7095f6 100644
--- a/src/util/virdaemon.c
+++ b/src/util/virdaemon.c
@@ -275,17 +275,17 @@ int virDaemonForkIntoBackground(const char *argv0 
G_GNUC_UNUSED)
 return -1;
 }
 
-void virDaemonSetupLogging(const char *daemon_name G_GNUC_UNUSED,
-   unsigned int log_level G_GNUC_UNUSED,
-   char *log_filters G_GNUC_UNUSED,
-   char *log_outputs G_GNUC_UNUSED,
-   bool privileged G_GNUC_UNUSED,
-   bool verbose G_GNUC_UNUSED,
-   bool godaemon G_GNUC_UNUSED)
+int virDaemonSetupLogging(const char *daemon_name G_GNUC_UNUSED,
+  unsigned int log_level G_GNUC_UNUSED,
+  char *log_filters G_GNUC_UNUSED,
+  char *log_outputs G_GNUC_UNUSED,
+  bool privileged G_GNUC_UNUSED,
+  bool verbose G_GNUC_UNUSED,
+  bool godaemon G_GNUC_UNUSED)
 {
 /* NOOP */
 errno = ENOTSUP;
-return;
+return -1;
 }
 
 int virDaemonUnixSocketPaths(const char *sock_prefix G_GNUC_UNUSED,
-- 
2.31.1

Re: [libvirt PATCH 1/2] conf: add killpriv v2 attribute for virtiofs

[Daniel P . Berrangé]

On Wed, Jan 05, 2022 at 04:06:49PM +0100, Ján Tomko wrote:
> Add a new attribute to control the killpriv feature:
> 
>   
> ...
> 
>   
> 
>   

Ewww, ewww, ewww.

This is a horrible element & attribute name. Even with the docs I have
little clue as to why we would ever need this or what its real effects
are. Having now read the libvirt and related QEMU bugzilla, I now see
that   killpriv v2 is enabled by default in virtiofs because it is
faster and more reliable. A limitation in NFS, however, means that
some specific syscalls don't work correctly with it, so QEMU folks
are saying killpriv v2 should not be used with NFS.

At this point I ask why doesn't virtiofsd just do the right thing.
It can statfs() the root of the export it is configured with and see
that it is NFS.  I'm not convinced by a need to expose this knob in
libvirt, especially with such an unintelligible name & difficult to
understand behavioural impact.

> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1972571
> 
> Signed-off-by: Ján Tomko 
> ---
>  docs/formatdomain.rst |  4 
>  docs/schemas/domaincommon.rng |  7 +++
>  src/conf/domain_conf.c| 15 +++
>  src/conf/domain_conf.h|  1 +
>  .../qemuxml2argvdata/vhost-user-fs-fd-memory.xml  |  1 +
>  5 files changed, 28 insertions(+)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index d4f30bb8af..73ff8bce51 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -3313,6 +3313,7 @@ A directory on the host that can be accessed directly 
> from the guest.
>   
>  
>  
> +
>  
>   
>   
> @@ -3447,6 +3448,9 @@ A directory on the host that can be accessed directly 
> from the guest.
> ``chroot``, see the
> `virtiofsd documentation 
> `__
> for more details. ( :since:`Since 7.2.0` )
> +   The ``killpriv`` element with the attribute ``v2`` (values: ``on`` or 
> ``off``)
> +   can be used to disable the killpriv capability which is used to improve 
> performance
> +   by expecting writes to reset some security attributes. ( :since:`Since 
> 8.0.0` )
>  ``source``
> The resource on the host that is being accessed in the guest. The ``name``
> attribute must be used with ``type='template'``, and the ``dir`` attribute
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 7fa5c2b8b5..5701bbe193 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3057,6 +3057,13 @@
>  
>
>  
> +
> +  
> +
> +  
> +
> +  
> +
>
>  
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5691b8d2d5..2a1802a3d5 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9874,6 +9874,7 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
>  g_autofree char *sandbox = 
> virXPathString("string(./binary/sandbox/@mode)", ctxt);
>  g_autofree char *posix_lock = 
> virXPathString("string(./binary/lock/@posix)", ctxt);
>  g_autofree char *flock = 
> virXPathString("string(./binary/lock/@flock)", ctxt);
> +g_autofree char *killpriv_v2 = 
> virXPathString("string(./binary/killpriv/@v2)", ctxt);
>  int val;
>  
>  if (queue_size && virStrToLong_ull(queue_size, NULL, 10, 
> >queue_size) < 0) {
> @@ -9932,6 +9933,15 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
>  }
>  def->flock = val;
>  }
> +
> +if (killpriv_v2) {
> +if ((val = virTristateSwitchTypeFromString(killpriv_v2)) <= 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("unknown killpriv_v2 value '%s'"), 
> killpriv_v2);
> +goto error;
> +}
> +def->killpriv_v2 = val;
> +}
>  }
>  
>  if (source == NULL && def->type != VIR_DOMAIN_FS_TYPE_RAM
> @@ -24197,6 +24207,11 @@ virDomainFSDefFormat(virBuffer *buf,
>virTristateSwitchTypeToString(def->flock));
>  }
>  
> +if (def->killpriv_v2 != VIR_TRISTATE_SWITCH_ABSENT) {
> +virBufferAsprintf(, "\n",
> +  
> virTristateSwitchTypeToString(def->killpriv_v2));
> +}
> +
>  virXMLFormatElement(, "lock", , NULL);
>  }
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 144ba4dd12..4619fcbfd1 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -892,6 +892,7 @@ struct _virDomainFSDef {
>  virTristateSwitch posix_lock;
>  virTristateSwitch flock;
>  virDomainFSSandboxMode sandbox;
> +virTristateSwitch killpriv_v2;
>  

[libvirt PATCH 1/2] conf: add killpriv v2 attribute for virtiofs

[Ján Tomko]

Add a new attribute to control the killpriv feature:

  
...

  

  

https://bugzilla.redhat.com/show_bug.cgi?id=1972571

Signed-off-by: Ján Tomko 
---
 docs/formatdomain.rst |  4 
 docs/schemas/domaincommon.rng |  7 +++
 src/conf/domain_conf.c| 15 +++
 src/conf/domain_conf.h|  1 +
 .../qemuxml2argvdata/vhost-user-fs-fd-memory.xml  |  1 +
 5 files changed, 28 insertions(+)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index d4f30bb8af..73ff8bce51 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3313,6 +3313,7 @@ A directory on the host that can be accessed directly 
from the guest.
  
 
 
+
 
  
  
@@ -3447,6 +3448,9 @@ A directory on the host that can be accessed directly 
from the guest.
``chroot``, see the
`virtiofsd documentation 
`__
for more details. ( :since:`Since 7.2.0` )
+   The ``killpriv`` element with the attribute ``v2`` (values: ``on`` or 
``off``)
+   can be used to disable the killpriv capability which is used to improve 
performance
+   by expecting writes to reset some security attributes. ( :since:`Since 
8.0.0` )
 ``source``
The resource on the host that is being accessed in the guest. The ``name``
attribute must be used with ``type='template'``, and the ``dir`` attribute
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 7fa5c2b8b5..5701bbe193 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3057,6 +3057,13 @@
 
   
 
+
+  
+
+  
+
+  
+
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5691b8d2d5..2a1802a3d5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9874,6 +9874,7 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
 g_autofree char *sandbox = 
virXPathString("string(./binary/sandbox/@mode)", ctxt);
 g_autofree char *posix_lock = 
virXPathString("string(./binary/lock/@posix)", ctxt);
 g_autofree char *flock = 
virXPathString("string(./binary/lock/@flock)", ctxt);
+g_autofree char *killpriv_v2 = 
virXPathString("string(./binary/killpriv/@v2)", ctxt);
 int val;
 
 if (queue_size && virStrToLong_ull(queue_size, NULL, 10, 
>queue_size) < 0) {
@@ -9932,6 +9933,15 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt,
 }
 def->flock = val;
 }
+
+if (killpriv_v2) {
+if ((val = virTristateSwitchTypeFromString(killpriv_v2)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown killpriv_v2 value '%s'"), 
killpriv_v2);
+goto error;
+}
+def->killpriv_v2 = val;
+}
 }
 
 if (source == NULL && def->type != VIR_DOMAIN_FS_TYPE_RAM
@@ -24197,6 +24207,11 @@ virDomainFSDefFormat(virBuffer *buf,
   virTristateSwitchTypeToString(def->flock));
 }
 
+if (def->killpriv_v2 != VIR_TRISTATE_SWITCH_ABSENT) {
+virBufferAsprintf(, "\n",
+  virTristateSwitchTypeToString(def->killpriv_v2));
+}
+
 virXMLFormatElement(, "lock", , NULL);
 }
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 144ba4dd12..4619fcbfd1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -892,6 +892,7 @@ struct _virDomainFSDef {
 virTristateSwitch posix_lock;
 virTristateSwitch flock;
 virDomainFSSandboxMode sandbox;
+virTristateSwitch killpriv_v2;
 virDomainVirtioOptions *virtio;
 virObject *privateData;
 };
diff --git a/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml 
b/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml
index abddf0870b..2f44a1593a 100644
--- a/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml
+++ b/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml
@@ -31,6 +31,7 @@
   
 
 
+
 
   
   
-- 
2.31.1

[libvirt PATCH 2/2] qemu: virtiofs: add (no_)killpriv_v2 support

[Ján Tomko]

https://bugzilla.redhat.com/show_bug.cgi?id=1972571

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_virtiofs.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index 1b853a5a59..c89da76c27 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -162,6 +162,11 @@ qemuVirtioFSBuildCommandLine(virQEMUDriverConfig *cfg,
 else if (fs->posix_lock == VIR_TRISTATE_SWITCH_OFF)
 virBufferAddLit(, ",no_posix_lock");
 
+if (fs->killpriv_v2 == VIR_TRISTATE_SWITCH_ON)
+virBufferAddLit(, ",killpriv_v2");
+else if (fs->killpriv_v2 == VIR_TRISTATE_SWITCH_OFF)
+virBufferAddLit(, ",no_killpriv_v2");
+
 virCommandAddArgBuffer(cmd, );
 if (cfg->virtiofsdDebug)
 virCommandAddArg(cmd, "-d");
-- 
2.31.1

[libvirt PATCH 0/2] qemu: virtiofs: add killpriv_v2 support (virtiofs epopee)

[Ján Tomko]

Ján Tomko (2):
  conf: add killpriv v2 attribute for virtiofs
  qemu: virtiofs: add (no_)killpriv_v2 support

 docs/formatdomain.rst |  4 
 docs/schemas/domaincommon.rng |  7 +++
 src/conf/domain_conf.c| 15 +++
 src/conf/domain_conf.h|  1 +
 src/qemu/qemu_virtiofs.c  |  5 +
 .../qemuxml2argvdata/vhost-user-fs-fd-memory.xml  |  1 +
 6 files changed, 33 insertions(+)

-- 
2.31.1

[PATCH v1 34/34] linux-user: Remove the deprecated ppc64abi32 target

[Alex Bennée]

From: Thomas Huth 

It's likely broken, and nobody cared for picking it up again
during the deprecation phase, so let's remove this now.

Since this is the last entry in deprecated_targets_list, remove
the related code in the configure script, too.

Signed-off-by: Thomas Huth 
Reviewed-by: Richard Henderson 
Acked-by: Cédric Le Goater 
Acked-by: Alex Bennée 
Message-Id: <20211215084958.185214-1-th...@redhat.com>
Signed-off-by: Alex Bennée 
---
 docs/about/deprecated.rst |  7 -
 docs/about/removed-features.rst   |  8 +
 docs/user/main.rst|  1 -
 configure | 29 +--
 configs/targets/ppc64abi32-linux-user.mak |  8 -
 linux-user/ppc/target_syscall.h   |  4 +--
 linux-user/syscall_defs.h |  6 ++--
 linux-user/elfload.c  |  4 +--
 linux-user/ppc/signal.c   | 11 ++-
 .gitlab-ci.d/buildtest.yml| 27 -
 .../dockerfiles/debian-ppc64el-cross.docker   |  2 +-
 tests/tcg/configure.sh|  2 +-
 12 files changed, 21 insertions(+), 88 deletions(-)
 delete mode 100644 configs/targets/ppc64abi32-linux-user.mak

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 5693abb663..7f12f53713 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -396,13 +396,6 @@ The above, converted to the current supported format::
 linux-user mode CPUs
 
 
-``ppc64abi32`` CPUs (since 5.2)
-'''
-
-The ``ppc64abi32`` architecture has a number of issues which regularly
-trip up our CI testing and is suspected to be quite broken. For that
-reason the maintainers strongly suspect no one actually uses it.
-
 MIPS ``I7200`` CPU (since 5.2)
 ''
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index d42c3341de..f92b8bd738 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -594,6 +594,14 @@ the upstream Linux kernel in 2018, and it has also been 
dropped from glibc, so
 there is no new Linux development taking place with this architecture. For
 running the old binaries, you can use older versions of QEMU.
 
+``ppc64abi32`` CPUs (removed in 7.0)
+
+
+The ``ppc64abi32`` architecture has a number of issues which regularly
+tripped up the CI testing and was suspected to be quite broken. For that
+reason the maintainers strongly suspected no one actually used it.
+
+
 System emulator devices
 ---
 
diff --git a/docs/user/main.rst b/docs/user/main.rst
index e08d4be63b..6f2ffa080f 100644
--- a/docs/user/main.rst
+++ b/docs/user/main.rst
@@ -166,7 +166,6 @@ Other binaries
 
 -  user mode (PowerPC)
 
-   * ``qemu-ppc64abi32`` TODO.
* ``qemu-ppc64`` TODO.
* ``qemu-ppc`` TODO.
 
diff --git a/configure b/configure
index 030728d11e..0c57a063c6 100755
--- a/configure
+++ b/configure
@@ -1273,8 +1273,6 @@ if [ "$ARCH" = "unknown" ]; then
 fi
 
 default_target_list=""
-deprecated_targets_list=ppc64abi32-linux-user
-deprecated_features=""
 mak_wilds=""
 
 if [ "$softmmu" = "yes" ]; then
@@ -1287,16 +1285,6 @@ if [ "$bsd_user" = "yes" ]; then
 mak_wilds="${mak_wilds} $source_path/configs/targets/*-bsd-user.mak"
 fi
 
-# If the user doesn't explicitly specify a deprecated target we will
-# skip it.
-if test -z "$target_list"; then
-if test -z "$target_list_exclude"; then
-target_list_exclude="$deprecated_targets_list"
-else
-target_list_exclude="$target_list_exclude,$deprecated_targets_list"
-fi
-fi
-
 for config in $mak_wilds; do
 target="$(basename "$config" .mak)"
 if echo "$target_list_exclude" | grep -vq "$target"; then
@@ -1315,11 +1303,9 @@ Standard options:
   --prefix=PREFIX  install in PREFIX [$prefix]
   --interp-prefix=PREFIX   where to find shared libraries, etc.
use %M for cpu name [$interp_prefix]
-  --target-list=LIST   set target list (default: build all non-deprecated)
+  --target-list=LIST   set target list (default: build all)
 $(echo Available targets: $default_target_list | \
   fold -s -w 53 | sed -e 's/^/   /')
-$(echo Deprecated targets: $deprecated_targets_list | \
-  fold -s -w 53 | sed -e 's/^/   /')
   --target-list-exclude=LIST exclude a set of targets from the default 
target-list
 
 Advanced options (experts only):
@@ -1804,13 +1790,6 @@ else
 done
 fi
 
-for target in $target_list; do
-# if a deprecated target is enabled we note it here
-if echo "$deprecated_targets_list" | grep -q "$target"; then
-add_to deprecated_features $target
-fi
-done
-
 # see if system emulation was really requested
 case " $target_list " in
   *"-softmmu "*) softmmu=yes
@@ -3950,12 +3929,6 @@ else
   fi
 fi
 

Re: [PATCH] util: Improve log out parsing errors

[Martin Kletzander]

Obviously s/out/output/ in $SUBJ

On Wed, Jan 05, 2022 at 02:31:37PM +0100, Martin Kletzander wrote:

Suggested-by: Erik Skultety 
Signed-off-by: Martin Kletzander 
---
src/util/virlog.c | 8 
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index 139dce8d0220..b44ad0ef6c47 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1487,21 +1487,21 @@ virLogParseOutput(const char *src)
if (!(tokens = g_strsplit(src, ":", 0)) ||
(count = g_strv_length(tokens)) < 2) {
virReportError(VIR_ERR_INVALID_ARG,
-   _("Malformed format for output '%s'"), src);
+   _("Malformed format for log output '%s'"), src);
return NULL;
}

if (virStrToLong_uip(tokens[0], NULL, 10, ) < 0 ||
(prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR)) {
virReportError(VIR_ERR_INVALID_ARG,
-   _("Invalid priority '%s' for output '%s'"),
+   _("Invalid log priority '%s' for log output '%s'"),
   tokens[0], src);
return NULL;
}

if ((dest = virLogDestinationTypeFromString(tokens[1])) < 0) {
virReportError(VIR_ERR_INVALID_ARG,
-   _("Invalid destination '%s' for output '%s'"),
+   _("Invalid log destination '%s' for log output '%s'"),
   tokens[1], src);
return NULL;
}
@@ -1511,7 +1511,7 @@ virLogParseOutput(const char *src)
((dest == VIR_LOG_TO_FILE ||
  dest == VIR_LOG_TO_SYSLOG) && count != 3)) {
virReportError(VIR_ERR_INVALID_ARG,
-   _("Output '%s' does not meet the format requirements "
+   _("Log output '%s' does not meet the format requirements 
"
 "for destination type '%s'"), src, tokens[1]);
return NULL;
}
--
2.34.1



signature.asc
Description: PGP signature

[PATCH] util: Improve log out parsing errors

[Martin Kletzander]

Suggested-by: Erik Skultety 
Signed-off-by: Martin Kletzander 
---
 src/util/virlog.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index 139dce8d0220..b44ad0ef6c47 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1487,21 +1487,21 @@ virLogParseOutput(const char *src)
 if (!(tokens = g_strsplit(src, ":", 0)) ||
 (count = g_strv_length(tokens)) < 2) {
 virReportError(VIR_ERR_INVALID_ARG,
-   _("Malformed format for output '%s'"), src);
+   _("Malformed format for log output '%s'"), src);
 return NULL;
 }
 
 if (virStrToLong_uip(tokens[0], NULL, 10, ) < 0 ||
 (prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR)) {
 virReportError(VIR_ERR_INVALID_ARG,
-   _("Invalid priority '%s' for output '%s'"),
+   _("Invalid log priority '%s' for log output '%s'"),
tokens[0], src);
 return NULL;
 }
 
 if ((dest = virLogDestinationTypeFromString(tokens[1])) < 0) {
 virReportError(VIR_ERR_INVALID_ARG,
-   _("Invalid destination '%s' for output '%s'"),
+   _("Invalid log destination '%s' for log output '%s'"),
tokens[1], src);
 return NULL;
 }
@@ -1511,7 +1511,7 @@ virLogParseOutput(const char *src)
 ((dest == VIR_LOG_TO_FILE ||
   dest == VIR_LOG_TO_SYSLOG) && count != 3)) {
 virReportError(VIR_ERR_INVALID_ARG,
-   _("Output '%s' does not meet the format requirements "
+   _("Log output '%s' does not meet the format 
requirements "
  "for destination type '%s'"), src, tokens[1]);
 return NULL;
 }
-- 
2.34.1

Re: [PATCH 6/8] util: Check for errors in virLogSetFromEnv

[Martin Kletzander]

On Wed, Jan 05, 2022 at 12:29:18PM +0100, Erik Skultety wrote:

On Tue, Jan 04, 2022 at 02:47:10PM +0100, Martin Kletzander wrote:

And make callers check the return value as well.  This helps error out early for
invalid environment variables.

That is desirable because it could lead to deadlocks.  This can happen when
resetting logging after fork() reports translated errors because gettext
functions are not reentrant.  Well, it is not limited to resetting logging after
fork(), it can be any translation at that phase, but parsing environment
variables is easy to make fail on purpose to show the result, it can also happen
just due to a typo.




Logging settings are also something that we want to report
errors on for example when it is being done over admin API.


True in general, but slightly off-topic wrt to the patch itself as
virLogSetFromEnv is irrelevant to admin API usage.



Yeah, this was supposed to be a part of another commit message, I just
wanted to guard against someone suggesting we do not report errors at
all (which would be another solution, but a wrong one I think).


...


-void
+int
 virLogSetFromEnv(void)
 {
 const char *debugEnv;

 if (virLogInitialize() < 0)
-return;
+return -1;

 debugEnv = getenv("LIBVIRT_DEBUG");
-if (debugEnv && *debugEnv)
-virLogSetDefaultPriority(virLogParseDefaultPriority(debugEnv));
+if (debugEnv && *debugEnv) {
+int priority = virLogParseDefaultPriority(debugEnv);
+if (priority < 0 ||
+virLogSetDefaultPriority(priority) < 0)
+return -1;


  ^^^ indentation



Thanks for catching that!


+}
 debugEnv = getenv("LIBVIRT_LOG_FILTERS");
-if (debugEnv && *debugEnv)
-virLogSetFilters(debugEnv);
+if (debugEnv && *debugEnv &&
+virLogSetFilters(debugEnv))
+return -1;
 debugEnv = getenv("LIBVIRT_LOG_OUTPUTS");
-if (debugEnv && *debugEnv)
-virLogSetOutputs(debugEnv);
+if (debugEnv && *debugEnv &&
+virLogSetOutputs(debugEnv))
+return -1;
+
+return 0;
 }


Reviewed-by: Erik Skultety 



signature.asc
Description: PGP signature

Re: [libvirt PATCH 11/17] tests: Add HVF support to testutilsqemu

[Andrea Bolognani]

On Wed, Jan 05, 2022 at 11:14:20AM +, Daniel P. Berrangé wrote:
> On Wed, Jan 05, 2022 at 03:02:07AM -0800, Andrea Bolognani wrote:
> > What if I changed things so that both the HVF test cases and the
> > testutilsqemu bit above are only built on macOS? We'd still have the
> > weird mix of capabilities on that platform, but at least Linux would
> > be unaffected. We run the test suite on macOS as part of our CI
> > pipeline, so coverage wouldn't be any worse.
>
> I was thinking more like
>
>  - testQemuCapsInit  only adds TCG+KVM emulators
>  - testQemuCapsInitMacOS only adds TCG+HVF emulators
>
> In  qemuxml2argvtest.c do
>
> virCapsPtr linuxCaps = driver->caps
> driver->caps = testQemuCapsInitMacOS();
> DO_TEST("hvf-blah",
> QEMU_CAPS_HVF,
> QEMU_CAPS_PIIX3_USB_UHCI,
>   
> QEMU_CAPS_USB_HUB);
>
> virObjectUnref(driver->caps);
> driver->caps = linuxCaps;
>
> feels like it ought to be reasonably simple to get working

Okay, I'll give it a try.

-- 
Andrea Bolognani / Red Hat / Virtualization

Plans for the next release

[Jiri Denemark]

We are getting close to the next release of libvirt. To aim for the
release on Jan 14 I suggest entering the freeze on Monday Jan 10 and
tagging RC2 on Wednesday Jan 12.

I hope this works for everyone.

Jirka

Re: [PATCH 0/8] Fix an unfortunate deadlock

[Erik Skultety]

On Tue, Jan 04, 2022 at 02:47:04PM +0100, Martin Kletzander wrote:
> Before this series:
> 
> # LIBVIRT_LOG_OUTPUTS=1:asdf:fdsa:meh libvirtd
> 
> 
> After this series:
> 
> # LIBVIRT_LOG_OUTPUTS=1:asdf:fdsa:meh libvirtd
> libvirt:  error : invalid argument: Invalid destination 'asdf' for output 
> '1:asdf:fdsa:meh'

Hmm, definitely an improvement, but it's not clear what the output is from the
message, I suggest tweaking the messages in virLogParseOutput in a follow-up
patch. Arguably, this would make even more of a difference :)

libvirt:  error : invalid argument: Invalid log destination 'asdf' for log 
output '1:asdf:fdsa:meh'

Erik

Re: [PATCH 4/8] util: Initialize virLogMutex statically

[Erik Skultety]

On Tue, Jan 04, 2022 at 02:47:08PM +0100, Martin Kletzander wrote:
> The only difference is that we are not going to be guaranteed that the mutex 
> is
> normal (as opposed to recursive, although there is no system known to me that
> would default to recursive mutexes), but that was done only to find occasional
> errors (during runtime, back in 2010, commit 336fd879c00b).  Functions using
> this mutex are mostly stable and unchanging, and it makes the virLogOnceInit()
> function only return 0 (or possibly abort in glib calls).  On top of that we 
> can
> assume that the virLogMutex is always initialized which enables us to be more
> consistent in some early error reporting.
> 
> Signed-off-by: Martin Kletzander 
> ---
Reviewed-by: Erik Skultety 

Re: [PATCH 8/8] Do not print error in remote_daemon.c:main

[Erik Skultety]

On Tue, Jan 04, 2022 at 02:47:12PM +0100, Martin Kletzander wrote:
> There is no need to do that since both fallible functions do that already.
> 
> Signed-off-by: Martin Kletzander 
> ---
Reviewed-by: Erik Skultety 

Re: [PATCH 7/8] Dispatch error in virInitialize

[Erik Skultety]

On Tue, Jan 04, 2022 at 02:47:11PM +0100, Martin Kletzander wrote:
> Callers that already do this anyway can be cleaned up thanks to this and the 
> one
> that does not (daemon startup) gains the benefit of the error being printed to
> standard error output changing:
> 
> LIBVIRT_LOG_OUTPUTS=1:invalid libvirtd
> /home/nert/dev/libvirt/upstream/build/src/libvirtd: initialisation failed
> 
> into:
> 
> LIBVIRT_LOG_OUTPUTS=1:invalid libvirtd
> libvirt:  error : invalid argument: Invalid destination 'invalid' for output 
> '1:invalid'
> /home/nert/dev/libvirt/upstream/build/src/libvirtd: initialisation failed
> 
> Signed-off-by: Martin Kletzander 
Reviewed-by: Erik Skultety 

Re: [PATCH 5/8] Exit on errors from virDaemonSetupLogging

[Erik Skultety]

On Tue, Jan 04, 2022 at 02:47:09PM +0100, Martin Kletzander wrote:
> This prevents starting any daemons with improper logging settings.  This is
> desirable on its own, but will be even more beneficial when more functions 
> start
> reporting errors and failing on them, coming up in following patches
> 
> Signed-off-by: Martin Kletzander 
> ---
Reviewed-by: Erik Skultety 

Re: [PATCH 3/8] util: Report error in virLogSetDefaultOutputToFile

[Erik Skultety]

On Tue, Jan 04, 2022 at 02:47:07PM +0100, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander 
> ---
Reviewed-by: Erik Skultety 

Re: [PATCH 2/8] util: Do not hide errors in virLogSetDefaultOutput

[Erik Skultety]

On Tue, Jan 04, 2022 at 02:47:06PM +0100, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander 
> ---
Reviewed-by: Erik Skultety 

Re: [PATCH 1/8] util: Report error in virLogParseDefaultPriority

[Erik Skultety]

On Tue, Jan 04, 2022 at 02:47:05PM +0100, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander 
> ---
Reviewed-by: Erik Skultety 

Re: [PATCH 6/8] util: Check for errors in virLogSetFromEnv

[Erik Skultety]

On Tue, Jan 04, 2022 at 02:47:10PM +0100, Martin Kletzander wrote:
> And make callers check the return value as well.  This helps error out early 
> for
> invalid environment variables.
> 
> That is desirable because it could lead to deadlocks.  This can happen when
> resetting logging after fork() reports translated errors because gettext
> functions are not reentrant.  Well, it is not limited to resetting logging 
> after
> fork(), it can be any translation at that phase, but parsing environment
> variables is easy to make fail on purpose to show the result, it can also 
> happen
> just due to a typo.


> Logging settings are also something that we want to report
> errors on for example when it is being done over admin API.

True in general, but slightly off-topic wrt to the patch itself as
virLogSetFromEnv is irrelevant to admin API usage.

...

> -void
> +int
>  virLogSetFromEnv(void)
>  {
>  const char *debugEnv;
>  
>  if (virLogInitialize() < 0)
> -return;
> +return -1;
>  
>  debugEnv = getenv("LIBVIRT_DEBUG");
> -if (debugEnv && *debugEnv)
> -virLogSetDefaultPriority(virLogParseDefaultPriority(debugEnv));
> +if (debugEnv && *debugEnv) {
> +int priority = virLogParseDefaultPriority(debugEnv);
> +if (priority < 0 ||
> +virLogSetDefaultPriority(priority) < 0)
> +return -1;

   ^^^ indentation

> +}
>  debugEnv = getenv("LIBVIRT_LOG_FILTERS");
> -if (debugEnv && *debugEnv)
> -virLogSetFilters(debugEnv);
> +if (debugEnv && *debugEnv &&
> +virLogSetFilters(debugEnv))
> +return -1;
>  debugEnv = getenv("LIBVIRT_LOG_OUTPUTS");
> -if (debugEnv && *debugEnv)
> -virLogSetOutputs(debugEnv);
> +if (debugEnv && *debugEnv &&
> +virLogSetOutputs(debugEnv))
> +return -1;
> +
> +return 0;
>  }

Reviewed-by: Erik Skultety 

Re: [libvirt PATCH 11/17] tests: Add HVF support to testutilsqemu

[Daniel P . Berrangé]

On Wed, Jan 05, 2022 at 03:02:07AM -0800, Andrea Bolognani wrote:
> On Wed, Jan 05, 2022 at 10:10:07AM +, Daniel P. Berrangé wrote:
> > On Tue, Jan 04, 2022 at 07:52:50PM +0100, Andrea Bolognani wrote:
> > > + if (hvf_machines[i] != NULL) {
> > > + for (j = 0; hvf_machines[i][j] != NULL; j++) {
> > > + virQEMUCapsAddMachine(tmpCaps,
> > > +   VIR_DOMAIN_VIRT_HVF,
> > > +   hvf_machines[i][j],
> > > +   NULL,
> > > +   NULL,
> > > +   0,
> > > +   false,
> > > +   false,
> > > +   true,
> > > +   defaultRAMid,
> > > +   false);
> > > + virQEMUCapsSet(tmpCaps, QEMU_CAPS_HVF);
> > > + }
> > > + }
> >
> > IIUC this means in tests we're going to build capabilities that
> > indicate support for KVM and HVF at the same time. This is not
> > a scenario that applies in the real world, so I'm a little
> > uncomfortable with this.  It is the simple option, though I
> > would prefer if the individual tests could express
> >
> >   "gimme capabilities for linux"
> >
> > vs
> >
> >   "gimme capabilities for macOS"
> >
> > Also relies on fact that we don't #ifdef any of the interesting
> > code in the QEMU driver related to KVM/HVF. Probably ok-ish
> > assumption in most cases, at least for unit tests.
> 
> Yeah, ideally we'd have that and also real replies files taken from
> QEMU running on macOS, but I don't currently have a way to generate
> the latter and the former would take more development time. In the
> interest of unblocking macOS users who are currently unable to run
> hardware accelerated VMs through libvirt at all, I'm personally okay
> with cutting some corners and improving things later.
> 
> What if I changed things so that both the HVF test cases and the
> testutilsqemu bit above are only built on macOS? We'd still have the
> weird mix of capabilities on that platform, but at least Linux would
> be unaffected. We run the test suite on macOS as part of our CI
> pipeline, so coverage wouldn't be any worse.

I was thinking more like
 
 - testQemuCapsInit  only adds TCG+KVM emulators
 - testQemuCapsInitMacOS only adds TCG+HVF emulators

In  qemuxml2argvtest.c do

virCapsPtr linuxCaps = driver->caps
driver->caps = testQemuCapsInitMacOS();
DO_TEST("hvf-blah",
QEMU_CAPS_HVF,
QEMU_CAPS_PIIX3_USB_UHCI,

QEMU_CAPS_USB_HUB);

virObjectUnref(driver->caps);
driver->caps = linuxCaps;

feels like it ought to be reasonably simple to get working

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 11/17] tests: Add HVF support to testutilsqemu

[Andrea Bolognani]

On Wed, Jan 05, 2022 at 10:10:07AM +, Daniel P. Berrangé wrote:
> On Tue, Jan 04, 2022 at 07:52:50PM +0100, Andrea Bolognani wrote:
> > + if (hvf_machines[i] != NULL) {
> > + for (j = 0; hvf_machines[i][j] != NULL; j++) {
> > + virQEMUCapsAddMachine(tmpCaps,
> > +   VIR_DOMAIN_VIRT_HVF,
> > +   hvf_machines[i][j],
> > +   NULL,
> > +   NULL,
> > +   0,
> > +   false,
> > +   false,
> > +   true,
> > +   defaultRAMid,
> > +   false);
> > + virQEMUCapsSet(tmpCaps, QEMU_CAPS_HVF);
> > + }
> > + }
>
> IIUC this means in tests we're going to build capabilities that
> indicate support for KVM and HVF at the same time. This is not
> a scenario that applies in the real world, so I'm a little
> uncomfortable with this.  It is the simple option, though I
> would prefer if the individual tests could express
>
>   "gimme capabilities for linux"
>
> vs
>
>   "gimme capabilities for macOS"
>
> Also relies on fact that we don't #ifdef any of the interesting
> code in the QEMU driver related to KVM/HVF. Probably ok-ish
> assumption in most cases, at least for unit tests.

Yeah, ideally we'd have that and also real replies files taken from
QEMU running on macOS, but I don't currently have a way to generate
the latter and the former would take more development time. In the
interest of unblocking macOS users who are currently unable to run
hardware accelerated VMs through libvirt at all, I'm personally okay
with cutting some corners and improving things later.

What if I changed things so that both the HVF test cases and the
testutilsqemu bit above are only built on macOS? We'd still have the
weird mix of capabilities on that platform, but at least Linux would
be unaffected. We run the test suite on macOS as part of our CI
pipeline, so coverage wouldn't be any worse.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 15/17] docs: Add support page for libvirt on macOS

[Andrea Bolognani]

On Wed, Jan 05, 2022 at 10:04:59AM +, Daniel P. Berrangé wrote:
> On Tue, Jan 04, 2022 at 07:52:54PM +0100, Andrea Bolognani wrote:
> > +
> > +  "hvf" domain type adds support of  > +  href="https://developer.apple.com/documentation/hypervisor;>
> > +  Hypervisor.framework since 4.10.0. To use "hvf" domain, QEMU must
> > +  be at least 2.12 and macOS must be no less than Yosemite (10.10). 
> > "hvf"
> > +  domain type is similar to "kvm" but it has less features.
> > +
>
> Since 8.0.0 not 4.10.0

Yeah, I basically didn't touch this at all because I wanted to
convert it to ReStructuredText first - we should really not add any
HTML documents to the repository at this point. In the respin, I'll
perform the conversion and apply your feedback.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 07/17] qemu: Introduce virQEMUCapsAccelStr

[Daniel P . Berrangé]

On Wed, Jan 05, 2022 at 01:36:23AM -0800, Andrea Bolognani wrote:
> On Wed, Jan 05, 2022 at 09:23:09AM +, Daniel P. Berrangé wrote:
> > On Tue, Jan 04, 2022 at 07:52:46PM +0100, Andrea Bolognani wrote:
> > > +static const char *
> > > +virQEMUCapsAccelStr(virDomainVirtType type)
> > > +{
> > > +if (type == VIR_DOMAIN_VIRT_KVM) {
> > > +return "kvm";
> > > +} else {
> > > +return "tcg";
> > > +}
> > > +}
> >
> > . VIR_ENUM_IMPL/DECL is a better long term plan. For enum
> > values that don't make sense for QEMU just return a placeholder
> > or something.
> 
> Agreed. Would you accept that as a separate patch that's still part
> of this series but applies after all the existing code changes? As I
> mention in the cover letter, I tried to keep Roman's original code
> intact as much as possible, and I would prefer to avoid making any
> non-trivial changes to it to keep the authorship story relatively
> straight.

Would still be bisectable, so that's ok.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 11/17] tests: Add HVF support to testutilsqemu

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 07:52:50PM +0100, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  tests/testutilsqemu.c | 40 
>  1 file changed, 40 insertions(+)
> 
> diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
> index 7fdb82daec..a75995c77a 100644
> --- a/tests/testutilsqemu.c
> +++ b/tests/testutilsqemu.c
> @@ -106,6 +106,18 @@ static const char *const *kvm_machines[VIR_ARCH_LAST] = {
>  [VIR_ARCH_S390X] = s390x_machines,
>  };
>  
> +static const char *const *hvf_machines[VIR_ARCH_LAST] = {
> +[VIR_ARCH_I686] = NULL,
> +[VIR_ARCH_X86_64] = x86_64_machines,
> +[VIR_ARCH_AARCH64] = aarch64_machines,
> +[VIR_ARCH_ARMV7L] = NULL,
> +[VIR_ARCH_PPC64] = NULL,
> +[VIR_ARCH_PPC] = NULL,
> +[VIR_ARCH_RISCV32] = NULL,
> +[VIR_ARCH_RISCV64] = NULL,
> +[VIR_ARCH_S390X] = NULL,
> +};
> +
>  static const char *qemu_default_ram_id[VIR_ARCH_LAST] = {
>  [VIR_ARCH_I686] = "pc.ram",
>  [VIR_ARCH_X86_64] = "pc.ram",
> @@ -214,6 +226,18 @@ testQemuAddGuest(virCaps *caps,
>NULL, nmachines, machines);
>  }
>  
> +if (hvf_machines[emu_arch] != NULL) {
> +nmachines = g_strv_length((char **)hvf_machines[emu_arch]);
> +machines = virCapabilitiesAllocMachines(hvf_machines[emu_arch],
> +nmachines);
> +if (machines == NULL)
> +goto error;
> +
> +virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HVF,
> +  qemu_emulators[emu_arch],
> +  NULL, nmachines, machines);
> +}
> +
>  return 0;
>  
>   error:
> @@ -403,6 +427,22 @@ int qemuTestCapsCacheInsert(virFileCache *cache,
>  virQEMUCapsSet(tmpCaps, QEMU_CAPS_KVM);
>  }
>  }
> +if (hvf_machines[i] != NULL) {
> +for (j = 0; hvf_machines[i][j] != NULL; j++) {
> +virQEMUCapsAddMachine(tmpCaps,
> +  VIR_DOMAIN_VIRT_HVF,
> +  hvf_machines[i][j],
> +  NULL,
> +  NULL,
> +  0,
> +  false,
> +  false,
> +  true,
> +  defaultRAMid,
> +  false);
> +virQEMUCapsSet(tmpCaps, QEMU_CAPS_HVF);
> +}
> +}
>  }

IIUC this means in tests we're going to build capabilities that
indicate support for KVM and HVF at the same time. This is not
a scenario that applies in the real world, so I'm a little
uncomfortable with this.  It is the simple option, though I
would prefer if the individual tests could express

  "gimme capabilities for linux"

vs

  "gimme capabilities for macOS"


Also relies on fact that we don't #ifdef any of the interesting
code in the QEMU driver related to KVM/HVF. Probably ok-ish
assumption in most cases, at least for unit tests.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 15/17] docs: Add support page for libvirt on macOS

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 07:52:54PM +0100, Andrea Bolognani wrote:
> From: Roman Bolshakov 
> 
> Signed-off-by: Roman Bolshakov 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/docs.html.in  |   3 +
>  docs/index.html.in |   3 +-
>  docs/macos.html.in | 229 +
>  3 files changed, 234 insertions(+), 1 deletion(-)
>  create mode 100644 docs/macos.html.in
> 
> diff --git a/docs/docs.html.in b/docs/docs.html.in
> index 8132090762..225827b693 100644
> --- a/docs/docs.html.in
> +++ b/docs/docs.html.in
> @@ -16,6 +16,9 @@
>  Windows
>  Downloads for Windows
>  
> +macOS
> +Working with libvirt on macOS
> +
>  Migration
>  Migrating guests between machines
>  
> diff --git a/docs/index.html.in b/docs/index.html.in
> index 2c4aa7c6d0..3c065badb7 100644
> --- a/docs/index.html.in
> +++ b/docs/index.html.in
> @@ -28,7 +28,8 @@
>LXC,
>BHyve and
>more
> -targets Linux, FreeBSD, Windows and 
> macOS
> +targets Linux, FreeBSD, Windows and
> +  macOS
>  is used by many applications
>
>Recent / forthcoming release changes
> diff --git a/docs/macos.html.in b/docs/macos.html.in
> new file mode 100644
> index 00..54c93ea2fb
> --- /dev/null
> +++ b/docs/macos.html.in
> @@ -0,0 +1,229 @@
> +
> +
> +http://www.w3.org/1999/xhtml;>
> +  
> +macOS support
> +
> +
> +
> +
> +  Libvirt works both as client and server (for 
> +  "qemu" domain) on macOS High Sierra (10.13) and macOS Mojave 
> (10.14)
> +  since 4.7.0. Other macOS variants likely work but we neither tested nor
> +  received reports for them.
> +

Per our platform support matrixs we only target 2 most recent versions
which will be Monteray and Big Sur.  I don't think we should mention
platforms at all here as it will be frequently getting out of date.

> +
> +  "hvf" domain type adds support of  +  href="https://developer.apple.com/documentation/hypervisor;>
> +  Hypervisor.framework since 4.10.0. To use "hvf" domain, QEMU must
> +  be at least 2.12 and macOS must be no less than Yosemite (10.10). "hvf"
> +  domain type is similar to "kvm" but it has less features.
> +

Since 8.0.0 not 4.10.0


> +
> +Installation
> +
> +
> +  libvirt client (virsh), server (libvirtd) and development headers can 
> be
> +  installed from https://brew.sh;>homebrew:
> +
> +  brew install libvirt
> +
> +  http://virt-manager.org;>virt-manager and virt-viewer can 
> be
> +  installed from source via  +  href="https://github.com/jeffreywildman/homebrew-virt-manager;>
> +  Jeffrey Wildman's tap:
> +
> +  brew tap jeffreywildman/homebrew-virt-manager
> +brew install virt-manager virt-viewer
> +

This hasn't been touched in 3 years, so I'm not convinced we should
be pointing people to it.

> +Running libvirtd locally
> +
> +
> +  The server can be started manually:
> +  libvirtd
> +  or on system boot:
> +  brew services start libvirt
> +
> +
> +  Once started, you can use virsh to work with libvirtd:

This doesn't distinguish between system and session mode at all.

> +  virsh define domain.xml
> +virsh start domain
> +virsh shutdown domain
> +
> +  For more details on virsh, please see virsh
> +  command reference or built-in help:
> +  virsh help
> +
> +
> +
> +  Domain XML examples can be found on  href="drvqemu.html#xmlconfig">QEMU
> +  driver page. Full reference is available on  +  href="formatdomain.html">domain XML format page.
> +
> +
> +
> +  You can use virt-manager to connect to libvirtd (connection URI must be
> +  specified on the first connection, then it'll be possible to omit it):
> +  virt-manager -c qemu:///session
> +  or, if you only need an access to the virtual display of a VM you can 
> use
> +  virt-viewer:
> +  virt-viewer -c qemu:///session
> +

Do we really want to start documenting apps here, we don't for any other
platform.

> +Working with external 
> hypervisors
> +
> +  Details on the example domain XML files, capabilities and connection
> +  string syntax used for connecting to external hypervisors can be found
> +  online on hypervisor specific driver
> +  pages.
> +
> +
> +TLS Certificates
> +
> +
> +  TLS certificates must be placed in the correct locations, before you 
> will
> +  be able to connect to QEMU servers over TLS.
> +
> +
> +
> +  Information on generating TLS certificates can be found here:
> +
> +
> + href="http://wiki.libvirt.org/page/TLSSetup;>http://wiki.libvirt.org/page/TLSSetup
> +
> +
> +  The Certificate Authority (CA) certificate file must be placed in:
> +
> +
> +
> +  ~/.cache/libvirt/pki/CA/cacert.pem
> +
> +
> +
> +  The Client certificate file must 

Re: [libvirt PATCH 10/17] fixup! qemu: Fix HVF architecture check

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 07:52:49PM +0100, Andrea Bolognani wrote:
> Apple Silicon (aarch64) has HVF support, but there is no
> 32-bit Intel hardware that is HVF-capable.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 09/17] qemu: Correct CPU capabilities probing for hvf

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 07:52:48PM +0100, Andrea Bolognani wrote:
> From: Roman Bolshakov 
> 
> With this change virsh domcapabilites shows:
>   
> 
> https://gitlab.com/libvirt/libvirt/-/issues/147
> 
> Signed-off-by: Roman Bolshakov 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c | 25 ++---
>  src/qemu/qemu_process.c  |  2 +-
>  2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 893af0b635..c2ae87d747 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -741,6 +741,7 @@ struct _virQEMUCaps {
>  
>  /* Capabilities which may differ depending on the accelerator. */
>  virQEMUCapsAccel kvm;
> +virQEMUCapsAccel hvf;
>  virQEMUCapsAccel tcg;
>  };
>  
> @@ -791,13 +792,15 @@ const char *virQEMUCapsArchToString(virArch arch)
>  static bool
>  virQEMUCapsTypeIsAccelerated(virDomainVirtType type)
>  {
> -return type == VIR_DOMAIN_VIRT_KVM;
> +return type == VIR_DOMAIN_VIRT_KVM ||
> +   type == VIR_DOMAIN_VIRT_HVF;
>  }
>  
>  static bool
>  virQEMUCapsHaveAccel(virQEMUCaps *qemuCaps)
>  {
> -return virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM);
> +return virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) ||
> +   virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF);
>  }
>  
>  static const char *
> @@ -805,6 +808,8 @@ virQEMUCapsAccelStr(virDomainVirtType type)
>  {
>  if (type == VIR_DOMAIN_VIRT_KVM) {
>  return "kvm";
> +} else if (type == VIR_DOMAIN_VIRT_HVF) {
> +return "hvf";
>  } else {
>  return "tcg";
>  }
> @@ -862,6 +867,8 @@ virQEMUCapsGetAccel(virQEMUCaps *qemuCaps,
>  {
>  if (type == VIR_DOMAIN_VIRT_KVM)
>  return >kvm;
> +else if (type == VIR_DOMAIN_VIRT_HVF)
> +return >hvf;
>  
>  return >tcg;
>  }
> @@ -992,6 +999,8 @@ virQEMUCapsGetMachineTypesCaps(virQEMUCaps *qemuCaps,
>   * take the set of machine types we probed first. */
>  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
>  accel = >kvm;
> +else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF))
> +accel = >hvf;
>  else
>  accel = >tcg;
>  
> @@ -2009,6 +2018,7 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps *qemuCaps)
>  ret->cpuData = virCPUDataNewCopy(qemuCaps->cpuData);
>  
>  if (virQEMUCapsAccelCopy(>kvm, >kvm) < 0 ||
> +virQEMUCapsAccelCopy(>hvf, >hvf) < 0 ||
>  virQEMUCapsAccelCopy(>tcg, >tcg) < 0)
>  return NULL;
>  
> @@ -2062,6 +2072,7 @@ void virQEMUCapsDispose(void *obj)
>  virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
>  
>  virQEMUCapsAccelClear(>kvm);
> +virQEMUCapsAccelClear(>hvf);
>  virQEMUCapsAccelClear(>tcg);
>  }
>  
> @@ -2313,6 +2324,10 @@ virQEMUCapsIsVirtTypeSupported(virQEMUCaps *qemuCaps,
>  virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG))
>  return true;
>  
> +if (virtType == VIR_DOMAIN_VIRT_HVF &&
> +virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF))
> +return true;
> +
>  if (virtType == VIR_DOMAIN_VIRT_KVM &&
>  virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
>  return true;
> @@ -2790,7 +2805,9 @@ bool
>  virQEMUCapsHasMachines(virQEMUCaps *qemuCaps)
>  {
>  
> -return !!qemuCaps->kvm.nmachineTypes || !!qemuCaps->tcg.nmachineTypes;
> +return !!qemuCaps->kvm.nmachineTypes ||
> +   !!qemuCaps->hvf.nmachineTypes ||
> +   !!qemuCaps->tcg.nmachineTypes;
>  }
>  
>  
> @@ -4718,6 +4735,7 @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps)
>virArchToString(qemuCaps->arch));
>  
>  virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_KVM);
> +virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_HVF);
>  virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_QEMU);
>  
>  for (i = 0; i < qemuCaps->ngicCapabilities; i++) {
> @@ -5577,6 +5595,7 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
>  qemuCaps->libvirtVersion = LIBVIR_VERSION_NUMBER;
>  
>  virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
> +virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_HVF);
>  virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
>  
>  if (virQEMUCapsHaveAccel(qemuCaps)) {
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index de1146251d..c80c9bae2d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -9288,7 +9288,7 @@ qemuProcessQMPLaunch(qemuProcessQMP *proc)
>  if (proc->forceTCG)
>  machine = "none,accel=tcg";
>  else
> -machine = "none,accel=kvm:tcg";
> +machine = "none,accel=kvm:hvf:tcg";

Probing kvm on macos or hvf on linux doesn't make much sense. For
that matter the existing code was already mistakenlyprobing kvm
on all platforms. IMHO this should be OS conditionalized

 #ifdef __linux__
 # define tryhwaccel kvm:tcg
 #elif __APPLE__
 # define 

Re: [libvirt PATCH 07/17] qemu: Introduce virQEMUCapsAccelStr

[Andrea Bolognani]

On Wed, Jan 05, 2022 at 09:23:09AM +, Daniel P. Berrangé wrote:
> On Tue, Jan 04, 2022 at 07:52:46PM +0100, Andrea Bolognani wrote:
> > +static const char *
> > +virQEMUCapsAccelStr(virDomainVirtType type)
> > +{
> > +if (type == VIR_DOMAIN_VIRT_KVM) {
> > +return "kvm";
> > +} else {
> > +return "tcg";
> > +}
> > +}
>
> . VIR_ENUM_IMPL/DECL is a better long term plan. For enum
> values that don't make sense for QEMU just return a placeholder
> or something.

Agreed. Would you accept that as a separate patch that's still part
of this series but applies after all the existing code changes? As I
mention in the cover letter, I tried to keep Roman's original code
intact as much as possible, and I would prefer to avoid making any
non-trivial changes to it to keep the authorship story relatively
straight.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 05/17] qemu: Introduce virQEMUCapsTypeIsAccelerated

[Andrea Bolognani]

On Wed, Jan 05, 2022 at 09:18:17AM +, Daniel P. Berrangé wrote:
> On Tue, Jan 04, 2022 at 07:52:44PM +0100, Andrea Bolognani wrote:
> > -if (virttype == VIR_DOMAIN_VIRT_KVM && capsType == 
> > VIR_DOMAIN_VIRT_QEMU) {
> > +if (virQEMUCapsTypeIsAccelerated(virttype) && capsType == 
> > VIR_DOMAIN_VIRT_QEMU) {
> >  virReportError(VIR_ERR_INVALID_ARG,
> > _("KVM is not supported by '%s' on this host"),
> > binary);
>
> s/KVM/Hardware acceleration/

This is changed in a later patch (8/17). Does that work for you, or
do you want me to try and shuffle things around?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 08/17] qemu: Make error message accel-agnostic

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 07:52:47PM +0100, Andrea Bolognani wrote:
> From: Roman Bolshakov 
> 
> With more acceleration types, KVM should be used only in error messages
> related to KVM.
> 
> Signed-off-by: Roman Bolshakov 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index eac1e65a39..893af0b635 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -5854,8 +5854,8 @@ virQEMUCapsCacheLookupDefault(virFileCache *cache,
>  
>  if (virQEMUCapsTypeIsAccelerated(virttype) && capsType == 
> VIR_DOMAIN_VIRT_QEMU) {
>  virReportError(VIR_ERR_INVALID_ARG,
> -   _("KVM is not supported by '%s' on this host"),
> -   binary);
> +   _("the accel '%s' is not supported by '%s' on this 
> host"),
> +   virQEMUCapsAccelStr(virttype), binary);
>  return NULL;
>  }

This should have been part of the earlier patch that changed the code
to use virQEMUCapsTypeIsAccelerated


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 07/17] qemu: Introduce virQEMUCapsAccelStr

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 07:52:46PM +0100, Andrea Bolognani wrote:
> From: Roman Bolshakov 
> 
> This makes possible to add more accelerators by touching less code and
> reduces code duplication.
> 
> Signed-off-by: Roman Bolshakov 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index b504963ddf..eac1e65a39 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -800,6 +800,16 @@ virQEMUCapsHaveAccel(virQEMUCaps *qemuCaps)
>  return virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM);
>  }
>  
> +static const char *
> +virQEMUCapsAccelStr(virDomainVirtType type)
> +{
> +if (type == VIR_DOMAIN_VIRT_KVM) {
> +return "kvm";
> +} else {
> +return "tcg";
> +}
> +}

. VIR_ENUM_IMPL/DECL is a better long term plan. For enum
values that don't make sense for QEMU just return a placeholder
or something.

> +
>  /* Checks whether a domain with @guest arch can run natively on @host.
>   */
>  bool
> @@ -4075,7 +4085,7 @@ virQEMUCapsLoadAccel(virQEMUCaps *qemuCaps,
>   virDomainVirtType type)
>  {
>  virQEMUCapsAccel *caps = virQEMUCapsGetAccel(qemuCaps, type);
> -const char *typeStr = type == VIR_DOMAIN_VIRT_KVM ? "kvm" : "tcg";
> +const char *typeStr = virQEMUCapsAccelStr(type);
>  
>  if (virQEMUCapsLoadHostCPUModelInfo(caps, ctxt, typeStr) < 0)
>  return -1;
> @@ -4624,7 +4634,7 @@ virQEMUCapsFormatAccel(virQEMUCaps *qemuCaps,
> virDomainVirtType type)
>  {
>  virQEMUCapsAccel *caps = virQEMUCapsGetAccel(qemuCaps, type);
> -const char *typeStr = type == VIR_DOMAIN_VIRT_KVM ? "kvm" : "tcg";
> +const char *typeStr = virQEMUCapsAccelStr(type);
>  
>  virQEMUCapsFormatHostCPUModelInfo(caps, buf, typeStr);
>  virQEMUCapsFormatCPUModels(caps, buf, typeStr);
> -- 
> 2.31.1
> 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 06/17] qemu: Introduce virQEMUCapsHaveAccel

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 07:52:45PM +0100, Andrea Bolognani wrote:
> From: Roman Bolshakov 
> 
> The function should be used to check if qemu capabilities include a
> hardware acceleration, i.e. accel is not TCG.
> 
> Signed-off-by: Roman Bolshakov 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 05/17] qemu: Introduce virQEMUCapsTypeIsAccelerated

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 07:52:44PM +0100, Andrea Bolognani wrote:
> From: Roman Bolshakov 
> 
> It replaces hardcoded checks that select accelCPU/accelCPUModels
> (formerly known as kvmCPU/kvmCPUModels) for KVM. It'll be cleaner to use
> the function when multiple accelerators are supported in qemu driver.
> 
> Explicit KVM domain checks should be done only when a feature is
> available only for KVM.
> 
> Signed-off-by: Roman Bolshakov 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index ca060485fa..e2ce2a5d07 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -788,6 +788,11 @@ const char *virQEMUCapsArchToString(virArch arch)
>  return virArchToString(arch);
>  }
>  
> +static bool
> +virQEMUCapsTypeIsAccelerated(virDomainVirtType type)
> +{
> +return type == VIR_DOMAIN_VIRT_KVM;
> +}
>  
>  /* Checks whether a domain with @guest arch can run natively on @host.
>   */
> @@ -2328,7 +2333,7 @@ virQEMUCapsIsCPUModeSupported(virQEMUCaps *qemuCaps,
>  
>  switch (mode) {
>  case VIR_CPU_MODE_HOST_PASSTHROUGH:
> -return type == VIR_DOMAIN_VIRT_KVM &&
> +return virQEMUCapsTypeIsAccelerated(type) &&
> virQEMUCapsGuestIsNative(hostarch, qemuCaps->arch);
>  
>  case VIR_CPU_MODE_HOST_MODEL:
> @@ -2990,7 +2995,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCaps *qemuCaps,
> qemuMonitor *mon,
> virDomainVirtType virtType)
>  {
> -const char *model = virtType == VIR_DOMAIN_VIRT_KVM ? "host" : "max";
> +const char *model = virQEMUCapsTypeIsAccelerated(virtType) ? "host" : 
> "max";
>  g_autoptr(qemuMonitorCPUModelInfo) modelInfo = NULL;
>  g_autoptr(qemuMonitorCPUModelInfo) nonMigratable = NULL;
>  g_autoptr(GHashTable) hash = NULL;
> @@ -3700,7 +3705,7 @@ virQEMUCapsInitHostCPUModel(virQEMUCaps *qemuCaps,
>virArchToString(qemuCaps->arch),
>virDomainVirtTypeToString(type));
>  goto error;
> -} else if (type == VIR_DOMAIN_VIRT_KVM &&
> +} else if (virQEMUCapsTypeIsAccelerated(type) &&
> virCPUGetHostIsSupported(qemuCaps->arch)) {
>  if (!(fullCPU = virQEMUCapsProbeHostCPU(qemuCaps->arch, NULL)))
>  goto error;
> @@ -5827,7 +5832,7 @@ virQEMUCapsCacheLookupDefault(virFileCache *cache,
>  if (virttype == VIR_DOMAIN_VIRT_NONE)
>  virttype = capsType;
>  
> -if (virttype == VIR_DOMAIN_VIRT_KVM && capsType == VIR_DOMAIN_VIRT_QEMU) 
> {
> +if (virQEMUCapsTypeIsAccelerated(virttype) && capsType == 
> VIR_DOMAIN_VIRT_QEMU) {
>  virReportError(VIR_ERR_INVALID_ARG,
> _("KVM is not supported by '%s' on this host"),
> binary);

s/KVM/Hardware acceleration/


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 03/17] qemu: Query hvf capability on macOS

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 07:52:42PM +0100, Andrea Bolognani wrote:
> From: Roman Bolshakov 
> 
> There's no QMP command for querying if hvf is supported, therefore we
> use sysctl interface that tells if Hypervisor.framework works/available
> on the host.
> 
> Signed-off-by: Roman Bolshakov 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c | 33 +
>  1 file changed, 33 insertions(+)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 02/17] qemu: Define hvf capability

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 07:52:41PM +0100, Andrea Bolognani wrote:
> From: Roman Bolshakov 
> 
> Signed-off-by: Roman Bolshakov 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c | 1 +
>  src/qemu/qemu_capabilities.h | 1 +
>  2 files changed, 2 insertions(+)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 01/17] conf: Add hvf domain type

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 07:52:40PM +0100, Andrea Bolognani wrote:
> From: Roman Bolshakov 
> 
> QEMU supports Hypervisor.framework since 2.12 as hvf accel.
> Hypervisor.framework provides a lightweight interface to run a virtual
> cpu on macOS without the need to install third-party kernel
> extensions (KEXTs).
> 
> It's supported since macOS 10.10 on machines with Intel VT-x feature
> set that includes Extended Page Tables (EPT) and Unrestricted Mode.
> 
> Signed-off-by: Roman Bolshakov 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/formatdomain.rst | 3 ++-
>  docs/schemas/domaincommon.rng | 1 +
>  src/conf/domain_conf.c| 1 +
>  src/conf/domain_conf.h| 1 +
>  src/qemu/qemu_command.c   | 4 
>  5 files changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|