Re: [PATCH v7 0/2] remove sysconfig files

[Olaf Hering]

Wed, 5 Jan 2022 12:13:28 -0700 Jim Fehlig :

> Thanks for reviewing v6! It looks like Olaf has addressed all of your 
> comments. 

I plan to provide a man page for libvirt-guests, just did not get around to 
actually write it down.


Olaf


pgpcPVCn1OI00.pgp
Description: Digitale Signatur von OpenPGP

Re: [PATCH v7 0/2] remove sysconfig files

[Andrea Bolognani]

On Fri, Jan 07, 2022 at 11:20:54AM -0700, Jim Fehlig wrote:
> On 1/7/22 09:55, Andrea Bolognani wrote:
> > The concerns mentioned here have not been addressed:
> >
> >
> > https://listman.redhat.com/archives/libvir-list/2022-January/msg00021.html
>
> I looked at that thread again and want to verify what is missing. IIUC, you
> agreed the '--listen' comment is not needed in the service file since it is
> well documented in the manpage, etc.

Yes.

> WRT libvirt-guests, Olaf added the comments for each env setting to the top
> of the script. I guess what is missing is your request for a comment in the
> service file telling where to look? I.e. the following part?
>
> [Service]
> # To learn what configuration knobs are available for this
> # service, check out the top of the libvirt-guest.sh script
> ...

That's the main part, yes. I also suggested that the configuration
bits be moved to the very top of the libvirt-guest.sh script, right
after the license blurb, so that they're more visible, but that's not
quite as important as having a well-documented path from the service
file, where settings have to be applied, to the script, which is the
only source of documentation for them.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v7 0/2] remove sysconfig files

[Andrea Bolognani]

On Wed, Jan 05, 2022 at 12:13:28PM -0700, Jim Fehlig wrote:
> On 1/3/22 03:49, Olaf Hering wrote:
> > style issues in docs/ and libvirt.spec.in (abologna)
>
> Hi Andrea,
>
> Thanks for reviewing v6! It looks like Olaf has addressed all of your
> comments. Any additional issues with this version?

The concerns mentioned here have not been addressed:

  https://listman.redhat.com/archives/libvir-list/2022-January/msg00021.html

I don't plan to give the patch my R-b until they have.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 1/1] virt-ssh-helper: Add manual page

[Andrea Bolognani]

On Fri, Jan 07, 2022 at 04:30:33PM +0100, Michal Prívozník wrote:
> On 12/13/21 14:44, Andrea Bolognani wrote:
> > We don't usually provide manual pages for internal tools,
> > but in the case of virt-ssh-helper the command is installed
> > inside the default $PATH and so it's likely that the user
> > will stumble upon it by using the shell's completion feature
> > when invoking another virt-* command, which makes it a good
> > idea to provide at least a minimal manual page.
> >
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  docs/manpages/meson.build |  1 +
> >  docs/manpages/virt-ssh-helper.rst | 95 +++
> >  libvirt.spec.in   |  1 +
> >  3 files changed, 97 insertions(+)
> >  create mode 100644 docs/manpages/virt-ssh-helper.rst
>
> Reviewed-by: Michal Privoznik 

I just realized that there is a small bug in the patch:
virt-ssh-helper is only built and installed when the daemon is, and
obviously the same should happen to its manual page. I have squashed
in the diff below before pushing.

Thanks for the review! :)


diff --git a/docs/manpages/meson.build b/docs/manpages/meson.build
index bf6fc730e0..6763d19af8 100644
--- a/docs/manpages/meson.build
+++ b/docs/manpages/meson.build
@@ -19,7 +19,7 @@ docs_man_files = [
   { 'name': 'virt-pki-query-dn', 'section': '1', 'install': true },
   { 'name': 'virt-pki-validate', 'section': '1', 'install': true },
   { 'name': 'virt-qemu-run', 'section': '1', 'install':
conf.has('WITH_QEMU') },
-  { 'name': 'virt-ssh-helper', 'section': '1', 'install': true },
+  { 'name': 'virt-ssh-helper', 'section': '1', 'install':
conf.has('WITH_LIBVIRTD') },
   { 'name': 'virt-xml-validate', 'section': '1', 'install': true },

   { 'name': 'libvirtd', 'section': '8', 'install': conf.has('WITH_LIBVIRTD') },
-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] do not report generic OPERATION_FAILED error when calling virConnectOpenAuth()

[Michal Prívozník]

On 1/6/22 18:07, Ani Sinha wrote:
> virConnectOpenAuth() calls virConnectOpenInternal(). This later function
> generates fine grained errors arising from various failure conditions that are
> more accurate than a "catch all" broader VIR_ERR_OPERATION_FAILED error that
> the callers of this function generates. Remove the broader error so that more
> specific errors can be caught and processed.
> 
> Signed-off-by: Ani Sinha 
> ---
>  src/libxl/libxl_migration.c | 3 ---
>  src/qemu/qemu_migration.c   | 3 ---
>  2 files changed, 6 deletions(-)

Reviewed-by: Michal Privoznik 

and pushed.

Michal

Re: [libvirt PATCH v2 1/1] virt-ssh-helper: Add manual page

[Michal Prívozník]

On 12/13/21 14:44, Andrea Bolognani wrote:
> We don't usually provide manual pages for internal tools,
> but in the case of virt-ssh-helper the command is installed
> inside the default $PATH and so it's likely that the user
> will stumble upon it by using the shell's completion feature
> when invoking another virt-* command, which makes it a good
> idea to provide at least a minimal manual page.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/manpages/meson.build |  1 +
>  docs/manpages/virt-ssh-helper.rst | 95 +++
>  libvirt.spec.in   |  1 +
>  3 files changed, 97 insertions(+)
>  create mode 100644 docs/manpages/virt-ssh-helper.rst


Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] src: Don't check for retval of g_strsplit()

[Erik Skultety]

On Fri, Jan 07, 2022 at 02:39:17PM +0100, Michal Privoznik wrote:
> The g_strsplit() function can return NULL if and only if either
> the input string is NULL or delimiter is NULL or an empty string.
> In neither of places we call it any of the conditions is true and
> thus we don't need to check for the return value.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libxl/xen_xl.c | 2 --
>  src/util/vircgroupv2.c | 2 --
>  src/util/virprocess.c  | 2 --
>  src/util/virresctrl.c  | 2 --
>  4 files changed, 8 deletions(-)
> 
> diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
> index 7604e3d534..869083a1d1 100644
> --- a/src/libxl/xen_xl.c
> +++ b/src/libxl/xen_xl.c
> @@ -265,8 +265,6 @@ xenParseXLCPUID(virConf *conf, virDomainDef *def)
>  }
>  
>  cpuid_pairs = g_strsplit(cpuid_str, ",", 0);
> -if (!cpuid_pairs)
> -return -1;
>  
>  if (!cpuid_pairs[0])
>  return 0;

But the following pattern:

if (!(identifier = g_strsplit(instr, delim, N)))
return -1;

can be found in many more places and it is semantically equivalent to what
you're removing in your patch, so all of those should be adjusted. While it
is true that NULL is returned only under those 3 conditions you mentioned,
I don't think this patch is really desirable as a simple typo like "" for the
delimiter which can go unnoticed easily in a large patch series is a problem
which may or may not pop up immediately depending on what tests have been
performed that would exercise the code.

So, unless you have a very compelling point on why we'd benefit from this patch
it's a NACK from me.

Erik

Re: [libvirt PATCH 2/2] docs: Replace node.gif with node.png

[Andrea Bolognani]

On Fri, Jan 07, 2022 at 03:49:25PM +0100, Ján Tomko wrote:
> On a Friday in 2022, Andrea Bolognani wrote:
> > This eliminates the only .gif file in our repository.
>
> What are the advantages of such change, apart from unambiguous
> pronunciation and bigger file size?

How is the first thing you mentioned not a good enough justification
on its own? :)

I'm personally just mildly annoyed by this one file breaking what
would otherwise be perfect consistency among image formats. I think
Dan suggested[1] getting rid of it because of the patent-encumbered
nature of the format.

Note that I'm absolutely open to addressing the situation by simply
axing the file, and I don't think anything of value would be lost if
we did that :)


[1] https://listman.redhat.com/archives/libvir-list/2022-January/msg00116.html
-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt][PATCH v9 2/5] Transfer Qemu SGX Capabilities to XML

[Michal Prívozník]

On 12/15/21 04:40, Haibin Huang wrote:
> Convert qemu sgx capabilities:
> {"sgx": true, "section-size": 0, "flc": false}
> 
> to XML format:
>  
> no
> 1
>  
> 

This should make it obvious which XML you are talking about. It is
domain capabilities XML, or domcaps XML for short. For commit message
either is usable.

> Signed-off-by: Haibin Huang 
> ---


>  99 files changed, 146 insertions(+), 1 deletion(-)
> 

As I said in previous patch, parts of the patch should be moved into
this one, because domcaps is semantically different change than QEMU
querying.

Also, please don't forget to update documentation:
docs/formatdomaincaps.html.in

You want probably reference new memory model there, so this patch could
be moved towards the end, after the new memory model was introduced and
documented.

Michal

Re: [libvirt][PATCH v9 3/5] conf: Introduce SGX EPC element into device memory xml

[Michal Prívozník]

On 12/15/21 04:40, Haibin Huang wrote:
> From: Lin Yang 
> 
> 
>   ...
>   
> 
>   512
> 
>   
>   ...
> 
> 
> Signed-off-by: Lin Yang 
> ---
>  docs/schemas/domaincommon.rng| 1 +
>  src/conf/domain_conf.c   | 6 ++
>  src/conf/domain_conf.h   | 1 +
>  src/conf/domain_validate.c   | 1 +
>  src/qemu/qemu_alias.c| 3 +++
>  src/qemu/qemu_command.c  | 1 +
>  src/qemu/qemu_domain.c   | 2 ++
>  src/qemu/qemu_domain_address.c   | 6 ++
>  src/qemu/qemu_driver.c   | 1 +
>  src/qemu/qemu_process.c  | 2 ++
>  src/qemu/qemu_validate.c | 8 
>  src/security/security_apparmor.c | 1 +
>  src/security/security_dac.c  | 2 ++
>  src/security/security_selinux.c  | 2 ++
>  14 files changed, 37 insertions(+)

Any domain XML change/extention has to be coupled with documentation
(docs/formatdomain.rst). How would an user know there's a new memory
model and what does its XML look like?

> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 26990c4d6d..39b02d1cb7 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -6616,6 +6616,7 @@
>nvdimm
>virtio-pmem
>virtio-mem
> +  sgx-epc
>  
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6fcf86ba58..c892865da4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1399,6 +1399,7 @@ VIR_ENUM_IMPL(virDomainMemoryModel,
>"nvdimm",
>"virtio-pmem",
>"virtio-mem",
> +  "sgx-epc",
>  );
>  
>  VIR_ENUM_IMPL(virDomainShmemModel,
> @@ -5508,6 +5509,7 @@ virDomainMemoryDefPostParse(virDomainMemoryDef *mem,
>  
>  case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
>  case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> +case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
>  case VIR_DOMAIN_MEMORY_MODEL_NONE:
>  case VIR_DOMAIN_MEMORY_MODEL_LAST:
>  break;
> @@ -14696,6 +14698,7 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node,
>  def->nvdimmPath = virXPathString("string(./path)", ctxt);
>  break;
>  
> +case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
>  case VIR_DOMAIN_MEMORY_MODEL_NONE:
>  case VIR_DOMAIN_MEMORY_MODEL_LAST:
>  break;
> @@ -14764,6 +14767,7 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
>  case VIR_DOMAIN_MEMORY_MODEL_NONE:
>  case VIR_DOMAIN_MEMORY_MODEL_DIMM:
>  case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM:
> +case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
>  case VIR_DOMAIN_MEMORY_MODEL_LAST:
>  break;
>  }
> @@ -16548,6 +16552,7 @@ virDomainMemoryFindByDefInternal(virDomainDef *def,
>  continue;
>  break;
>  
> +case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
>  case VIR_DOMAIN_MEMORY_MODEL_NONE:
>  case VIR_DOMAIN_MEMORY_MODEL_LAST:
>  break;
> @@ -25997,6 +26002,7 @@ virDomainMemorySourceDefFormat(virBuffer *buf,
>  virBufferEscapeString(, "%s\n", 
> def->nvdimmPath);
>  break;
>  
> +case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
>  case VIR_DOMAIN_MEMORY_MODEL_NONE:
>  case VIR_DOMAIN_MEMORY_MODEL_LAST:
>  break;
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 1ac802feca..58b6ff8355 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2482,6 +2482,7 @@ typedef enum {
>  VIR_DOMAIN_MEMORY_MODEL_NVDIMM, /* nvdimm memory device */
>  VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM, /* virtio-pmem memory device */
>  VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM, /* virtio-mem memory device */
> +VIR_DOMAIN_MEMORY_MODEL_SGX_EPC, /* SGX enclave page cache */
>  
>  VIR_DOMAIN_MEMORY_MODEL_LAST
>  } virDomainMemoryModel;
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 80401cf8c7..982ecc60d0 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -2066,6 +2066,7 @@ virDomainMemoryDefValidate(const virDomainMemoryDef 
> *mem,
>  break;
>  
>  case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> +case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
>  break;
>  
>  case VIR_DOMAIN_MEMORY_MODEL_NONE:
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index 276a03cb56..5795924754 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -538,6 +538,9 @@ qemuAssignDeviceMemoryAlias(virDomainDef *def,
>  case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
>  prefix = "virtiomem";
>  break;
> +case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
> +prefix = "epc";
> +break;
>  case VIR_DOMAIN_MEMORY_MODEL_NONE:
>  case VIR_DOMAIN_MEMORY_MODEL_LAST:
>  default:
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index dba877a740..36281a69e2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3768,6 +3768,7 @@ 

Re: [libvirt][PATCH v9 1/5] Get SGX Capabilities from QEMU

[Michal Prívozník]

On 12/15/21 04:40, Haibin Huang wrote:
> The Qemu QMP provide the command "query-sgx-capabilities"
> libvirt call the command to get sgx capabilities
> 
> {"execute":"query-sgx-capabilities"}
> {"return":
>   {"sgx": true, "sgx1": true, "sgx2": false, "section-size": 0, \
>"flc": false}}
> 
> Signed-off-by: Haibin Huang 
> ---
>  src/conf/domain_capabilities.c|  10 ++
>  src/conf/domain_capabilities.h|  13 ++
>  src/libvirt_private.syms  |   1 +
>  src/qemu/qemu_capabilities.c  | 143 +-
>  src/qemu/qemu_capabilities.h  |   4 +
>  src/qemu/qemu_monitor.c   |  10 ++
>  src/qemu/qemu_monitor.h   |   3 +
>  src/qemu/qemu_monitor_json.c  |  83 ++
>  src/qemu/qemu_monitor_json.h  |   3 +
>  .../caps_6.2.0.x86_64.replies |  22 ++-
>  .../caps_6.2.0.x86_64.xml |   5 +
>  11 files changed, 292 insertions(+), 5 deletions(-)


There's too much going on in this patch. You are querying qemu for SGX
support and filling domain caps. At least the domain caps should go into
the next patch.

Secondly, you are using SEV functions as an placeholder. I mean, where
you see SEV you put corresponding SGX function. There is nothing wrong
with that, but either put pick a placement (after/before SEV code) and
stick to it.

More comments below.
> 
> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
> index 22f0963326..d39be55f6a 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c
> @@ -78,6 +78,16 @@ virSEVCapabilitiesFree(virSEVCapability *cap)
>  }
>  
>  
> +void
> +virSGXCapabilitiesFree(virSGXCapability *cap)
> +{
> +if (!cap)
> +return;
> +
> +VIR_FREE(cap);
> +}
> +
> +
>  static void
>  virDomainCapsDispose(void *obj)
>  {
> diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
> index d44acdcd01..b647ff8107 100644
> --- a/src/conf/domain_capabilities.h
> +++ b/src/conf/domain_capabilities.h
> @@ -172,6 +172,13 @@ struct _virDomainCapsCPU {
>  virDomainCapsCPUModels *custom;
>  };
>  
> +typedef struct _virSGXCapability virSGXCapability;
> +typedef virSGXCapability *virSGXCapabilityPtr;

Even in 7.8.0 which you implemented your patches on top of had virXXXPtr
typedefs dropped. Please do not introduce new ones.

> +struct _virSGXCapability {
> +bool flc;
> +unsigned int epc_size;
> +};
> +
>  typedef struct _virSEVCapability virSEVCapability;
>  struct _virSEVCapability {
>  char *pdh;
> @@ -215,6 +222,7 @@ struct _virDomainCaps {
>  
>  virDomainCapsFeatureGIC gic;
>  virSEVCapability *sev;
> +virSGXCapability *sgx;
>  /* add new domain features here */
>  
>  virTristateBool features[VIR_DOMAIN_CAPS_FEATURE_LAST];
> @@ -262,4 +270,9 @@ char * virDomainCapsFormat(const virDomainCaps *caps);
>  void
>  virSEVCapabilitiesFree(virSEVCapability *capabilities);
>  
> +void
> +virSGXCapabilitiesFree(virSGXCapability *capabilities);
> +
>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSEVCapability, virSEVCapabilitiesFree);
> +
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSGXCapability, virSGXCapabilitiesFree);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c5d788285e..d90d4ee6e1 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -219,6 +219,7 @@ virDomainCapsEnumSet;
>  virDomainCapsFormat;
>  virDomainCapsNew;
>  virSEVCapabilitiesFree;
> +virSGXCapabilitiesFree;
>  
>  
>  # conf/domain_conf.h
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index a607f5ea5f..8ce184ce35 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -651,6 +651,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
>"chardev.json", /* QEMU_CAPS_CHARDEV_JSON */
>"device.json", /* QEMU_CAPS_DEVICE_JSON */
>"query-dirty-rate", /* QEMU_CAPS_QUERY_DIRTY_RATE */
> +  "sgx-epc", /* QEMU_CAPS_SGX_EPC */
>  );
>  
>  
> @@ -731,11 +732,14 @@ struct _virQEMUCaps {
>  
>  virSEVCapability *sevCapabilities;
>  
> +virSGXCapability *sgxCapabilities;
> +
>  /* Capabilities which may differ depending on the accelerator. */
>  virQEMUCapsAccel kvm;
>  virQEMUCapsAccel tcg;
>  };
>  
> +
>  struct virQEMUCapsSearchData {
>  virArch arch;
>  const char *binaryFilter;
> @@ -1367,6 +1371,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] 
> = {
>  { "virtio-vga-gl", QEMU_CAPS_VIRTIO_VGA_GL },
>  { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
>  { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
> +{ "sgx-epc", QEMU_CAPS_SGX_EPC },
>  };
>  
>  
> @@ -1918,6 +1923,22 @@ virQEMUCapsSEVInfoCopy(virSEVCapability **dst,
>  }
>  
>  
> +static int
> +virQEMUCapsSGXInfoCopy(virSGXCapabilityPtr *dst,
> +   virSGXCapabilityPtr 

Re: [libvirt][PATCH v9 0/5] Support query and use SGX

[Michal Prívozník]

On 12/15/21 04:40, Haibin Huang wrote:
> This patch series provides support for enabling Intel's Software Guard
> Extensions (SGX) feature in guest VM.
> Giving the SGX support in QEMU be accepted and will be merged in two
> days Intel SGX is a set of instructions that increases the security
> of application code and data, giving them more protection from disclosure
> or modification.
> Developers can partition sensitive information into enclaves, which are
> areas of execution in memory with more security protection.
> 
> The typical flow looks below at very high level:
> 
> 1. Calls virConnectGetDomainCapabilities API to domain capabilities that
> includes the following SGX information.
> 
> 
> ...
>   
> N
>   
> 
> 
> 2. User requests to start a guest calling virCreateXML() with SGX requirement.
> It should contain
> 
>  
>   ...
>   
> 
>   N
> 
>   
>   ...
>   
> 
> Haibin Huang (2):
>   Get SGX Capabilities from QEMU
>   Transfer Qemu SGX Capabilities to XML
> 
> Lin Yang (3):
>   conf: Introduce SGX EPC element into device memory xml
>   qemu: Add command-line to generate SGX EPC memory backend
>   Add unit tests for guest VM creation command with SGX EPC

Next time please make sure that any patch you send is rebased onto the
master branch that's at least somewhat current. I had to go all the way
down to 7.8.0 to apply these (somewhere mid October). I believe libvirt
is not the only project that mandates this.

I've uploaded these patches to my gitlab:

https://gitlab.com/MichalPrivoznik/libvirt/-/tree/sgx

You'll find 'fixup' commits there which cover some of the points I am
raising. Might be worth looking at it. I'll keep the branch there for a
while.

Michal

Re: [libvirt][PATCH v9 4/5] qemu: Add command-line to generate SGX EPC memory backend

[Michal Prívozník]

On 12/15/21 04:40, Haibin Huang wrote:
> From: Lin Yang 
> 
> According to the result parsing from xml, add the argument of
> SGX EPC memory backend into QEMU command line:
> 
> #qemu-system-x86_64 \
> .. \
> -object memory-backend-epc,id=mem1,size=64M,prealloc=on \
> -object memory-backend-epc,id=mem2,size=28M \
> -M sgx-epc.0.memdev=mem1,sgx-epc.1.memdev=mem2
> 
> Signed-off-by: Lin Yang 
> ---
>  src/qemu/qemu_alias.c   |  3 ++-
>  src/qemu/qemu_command.c | 40 
>  src/qemu/qemu_domain.c  | 10 +-
>  3 files changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index 5795924754..89afea8778 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -489,7 +489,8 @@ qemuDeviceMemoryGetAliasID(virDomainDef *def,
>   * valid */
>  if (!oldAlias &&
>  mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM &&
> -mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM)
> +mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM &&
> +mem->model != VIR_DOMAIN_MEMORY_MODEL_SGX_EPC)
>  return mem->info.addr.dimm.slot;
>  
>  for (i = 0; i < def->nmems; i++) {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 36281a69e2..ebb3aa1023 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3555,6 +3555,10 @@ qemuBuildMemoryBackendProps(virJSONValue 
> **backendProps,
>  if (systemMemory)
>  disableCanonicalPath = true;
>  
> +} else if (mem->model == VIR_DOMAIN_MEMORY_MODEL_SGX_EPC) {
> +backendType = "memory-backend-epc";
> +if (!priv->memPrealloc)
> +prealloc = true;
>  } else {
>  backendType = "memory-backend-ram";
>  }
> @@ -7838,6 +7842,8 @@ qemuBuildMemoryDeviceCommandLine(virCommand *cmd,
>   qemuDomainObjPrivate *priv)
>  {
>  size_t i;
> +g_auto(virBuffer) epcBuf = VIR_BUFFER_INITIALIZER;
> +int epcNum = 0;
>  
>  /* memory hotplug requires NUMA to be enabled - we already checked
>   * that memory devices are present only when NUMA is */
> @@ -7847,11 +7853,37 @@ qemuBuildMemoryDeviceCommandLine(virCommand *cmd,
>  if (qemuBuildMemoryDimmBackendStr(cmd, def->mems[i], def, cfg, priv) 
> < 0)
>  return -1;
>  
> -if (!(props = qemuBuildMemoryDeviceProps(def, def->mems[i])))
> -return -1;
> +switch ((virDomainMemoryModel) def->mems[i]->model) {
> +case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> +case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM:
> +case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> +if (!(props = qemuBuildMemoryDeviceProps(def, def->mems[i])))
> +return -1;
>  
> -if (qemuBuildDeviceCommandlineFromJSON(cmd, props, priv->qemuCaps) < 
> 0)
> -return -1;
> +if (qemuBuildDeviceCommandlineFromJSON(cmd, props, 
> priv->qemuCaps) < 0)
> +return -1;
> +
> +break;
> +
> +case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC:
> +if (virBufferUse() > 0)
> +virBufferAddChar(, ',');
> +
> +virBufferAsprintf(, "sgx-epc.%d.memdev=%s", epcNum++,
> +  g_strdup_printf("mem%s", 
> def->mems[i]->info.alias));

IIUC, there's also .node= attribute which tells QEMU which NUMA node
should the memory be at. Should this be also reflected? The NUMA node is
stored in to def->mems[i]->targetNode. Mind you, this is guest NUMA node
I'm talking about. Does the attribute refer to the host NUMA node?

Also, virBufferAsprintf() hold its promise and behaves like asprintf().
There's no need for additional g_strdup_printf(), more so when it's leaked.

Finally, there are multiple ways that hugepages can sneak in. For
instance the following input generates memory-backend-file instead of
memory-backend-epc:


  QEMUGuest1
  c7a5fdbd-edaf-9455-926a-d65c16db1809
  219100
  219100
  

  

  
  1
  ...
  
/usr/bin/qemu-system-x86_64
...

  
65536
  
  

  



Michal

Re: [libvirt PATCH 2/2] docs: Replace node.gif with node.png

[Ján Tomko]

On a Friday in 2022, Andrea Bolognani wrote:

This eliminates the only .gif file in our repository.



What are the advantages of such change, apart from unambiguous
pronunciation and bigger file size?

Jano


Signed-off-by: Andrea Bolognani 
---
docs/goals.html.in  |   2 +-
docs/images/meson.build |   2 +-
docs/images/node.gif| Bin 1397 -> 0 bytes
docs/images/node.png| Bin 0 -> 5532 bytes
4 files changed, 2 insertions(+), 2 deletions(-)
delete mode 100644 docs/images/node.gif
create mode 100644 docs/images/node.png


signature.asc
Description: PGP signature

[libvirt PATCH 2/2] docs: Replace node.gif with node.png

[Andrea Bolognani]

This eliminates the only .gif file in our repository.

Signed-off-by: Andrea Bolognani 
---
 docs/goals.html.in  |   2 +-
 docs/images/meson.build |   2 +-
 docs/images/node.gif| Bin 1397 -> 0 bytes
 docs/images/node.png| Bin 0 -> 5532 bytes
 4 files changed, 2 insertions(+), 2 deletions(-)
 delete mode 100644 docs/images/node.gif
 create mode 100644 docs/images/node.png

diff --git a/docs/goals.html.in b/docs/goals.html.in
index d205bf4f42..a85a97d437 100644
--- a/docs/goals.html.in
+++ b/docs/goals.html.in
@@ -15,7 +15,7 @@
 virtualized machine provided by the hypervisor
 
 
-  
+  
 
 Now we can define the goal of libvirt:  to provide a common and
 stable layer sufficient to securely manage domains on a node, possibly
diff --git a/docs/images/meson.build b/docs/images/meson.build
index 85a35da4aa..778ac9bfad 100644
--- a/docs/images/meson.build
+++ b/docs/images/meson.build
@@ -10,7 +10,7 @@ docs_image_files = [
   'migration-native.png',
   'migration-tunnel.png',
   'migration-unmanaged-direct.png',
-  'node.gif',
+  'node.png',
 ]
 
 install_data(docs_image_files, install_dir: docs_html_dir / 'images')
diff --git a/docs/images/node.gif b/docs/images/node.gif
deleted file mode 100644
index 
16a5fe9a3157944769354918aff7f28d3faaf073..
GIT binary patch
literal 0
HcmV?d1

literal 1397
zcmV-*1%w1VWI%B0Du4h0002)>*gW(1OWg50RSuj0001@0I>i70{)DTsmtvT
zqnxxhfV1Ab`wxcVNS5a51%So2?hD7zy2|#A=P<4J{{Q0vhr}X9XiO@V$ffhCYeJ{g
zmb8k^Ib8^E(&`ipMQSMd7$M|g*#xTvVc
zw}4@oP*e6eDG6z2c|eJIDYj|Q>4`9!6^ck&3Yt1$s@i%Q8}Z8UaaL=~n!B|(nvoRg
zy34`~X$o9PY+Dupu*-^4{?`Ys&^
zn7?w;f`#*A(W8>K4l#$7+!wBy!Uoi8#q3J7#nh6e_BLx+uV3TJg+&(#sO(~xLpy76T~5_ZQCO`YL24UKDc#0@l~WpieUpYc``m=FesT{n3)rCFgR
zjHr>H{zUi?W0l=yUWANdGw7oX{udL1iq_d>Z6nR+lb?8Yx+IdRh>6{)8-iwNtBY`!
z2x+OAN$L}|j>x5_Joc!oh=}g^T14=PDusV_0WDtBRdsT+b$^tEZNqx#n
zwY*;YshHiU*VIBQnOf;;7;;Nqf8UCFU_OxMSgcO>(7EP;ZGwoSm*>h`AfTYGXWqRk
zm5alG63y$ciJB^`V!jY5dGJV#Dm>x84HG=azQAFoCx8@toD#sd4g)cfFa3*hiYnua
z@oX9s`66a9Iz{tBGaifN$jg@8*RDVg+hDvyY8w*J@CuEW!|CD{MKs^X)
zQt!M}cUAeA)EdrV?TMF0JWV>wk*}jdfwoz%f?Si9lPyP1XxY}KZ+;|iA7TA9O
z?e|qu?j2f>RT5rTkVPY2xGucZeReKrK>kj)>J
zI8K|RZkp$(XARY{V_b@aEJME?ycl*FJjOxN{Et>%H4am6|!nXr!F^(8)a4
zi(g`U*1q$kor0S_g*`(dmZ#y`uJaCi%q6MBy@cm`@10EjH8=jd!e5Ub`ZT-CVV?=3
zEPj%~TQR+3#}0+aLHS)<4z#PfPSeh{qOnfdV2gC=-;PwG?PP>rGHx2&(0O2ov
zZ0Lh>w?R#5PkjT!(|=B)n!eWD`fYLaNR{4sPY!Mkmj8QCXR$pGogio
z(LkdGF@s<6iv)8)w>Q#2L3T6i
z*1Jdo0Kn%@wY1FiwY2zs{JdSBdN>0BqL~qyDtbL;%n8?J2Cz!HKnC9dvjPYz2|9C|
zxmdG;#Wp~?b%Xkq+$}N5v{?79E{zq%T0(99VADaih<9}x+3fOycWUPev
zcrw|B-yqH?c6jF%9jAq;*6Wyxn4kOJ+8pI>`{xgEBHz!|z*VFA#w}z>LtX6~eKXFr
z+M{k1rH`JC|BVNy{uQ96Xnyd`CiydcV;K1wkerSg(v~xk1OPDQ>1%6P1kdaggkUUI
zv-_huKFCYNQs<*8o<3zM$rK_>$?Oy2Ol2p}Wd|{pFnvV%0(HVt_%)HQweOd_(dltb
z(P8uvmQj{u*YM@{A`#R^05r6%Y}z)xU<-7~Y?7Xs`~Tko4KjwT-erRjrras
z_OM_td`}Fpwz=rDLM1|M(aOWE)ZeSw98;o%L$CV?Ds!7?Co+vL-dL|ubpKk~&!9J!
z$lIQOEf7yyg{aA)KPk=(L1IQd-YRDP;h2*!g~NNLeL+tXV*sk;Ddd%NDPqe$1TumI
zK}HcjUykhEb4KQqn!k8Qdr%i*K@DY{-fSA8FeM6$?(#}nkIViTZG%%#
zw6pS2mmVBuC_p_TyV@2W;yeKu#>{!a_`i8Ok8)7*0j&$p+B$!hfq-
zi_}=#@CO2ZQ?s^aONtM_m$aZe_uvFsHEUu%qIbAJf;@o(;ZL_SN(KkcQX2V&{xi$E
z?+Njc;ST0w2P3aEe_m1AztRfZW~IDd8h8C6A=tZ=(XLste;Vyg6a=7mUtmSv4
zoYSo)_9B~1%77I|hyWHyv8l#)5`5WNmp!QMiR?*VZZ#K>*;8|86YeY!)sm#8{0tJ6
z7CpjNeKn`DrwJ!tCyYP+5tm(n`zWaOu4#@moW+pwj9i+p#q3W4P>NU`=tVLZUDZI7
zYgd0O+y^-S~MXABWFhH<|_A99@%ME{dT!-7vL#l+4PwlB$?D11_(ZPVT)+Tj|
z?GaZfj^f!E~8XuQ!KnTML
zLpmGcc*)%XGFctYFRk6){Js;=mtSv_=RFX#MQ<@s$MwcP#VjaH?6;^7cAkEMBCPFi
zFR9?AJpq4FTW~KZKA8+qv>w1luz<{m2rK;Y7h^p<0vhI+Wp~TKaHWt&9LzV}!giI2
zRzZ2(ANxj#rbuNpY{_pl=pOBs`w|)k(+g%cS{y!EBkLi*a4sp^Q^-O>s?7yFK40)M6$NfV9DUf3xj3N^^Z}4!_y;Rsr
zT17mh{Yx$^O`s^g-QDmGC-oz2t`5vO<0RAjVwSnc5oy8aOVniHU-(nfGd!s#6$_yD
zQ4yrGx(7k~GZSHt+?#E2|^~YlcH_t$^58#zPBwEXmQi+FMp~x
zJ=u?%uGA1gCgIO#lKM4hu_R!>scj!Q}*+s$}N#w@3ddL)fvL=3ejur!Z#6gP`;m
z3wf{t8OIn|i09r$%OyCvWa7IgPA>W_uurCI@0E-PBlDU9?hZ#*D=^K3{9xW>YT8Ii37JhR$lvR_$Gk2~bUnSz5
zKGH??J%;(Mexx<0@q6Ez`lyXJi>+Q=|`Kts)dL4SJqa*UVWu8KZ(9GwlJ_@
zvOENzjrv;~2`Svu4bTvxw8h?4ej>>Atn%02jzd%cIyT#C1Uusl4WRms^aB
z_15CheGJM?_O4~c*4M4BK~6%awB=OvvtJ0LJ`2X=OZc0*6WYE>Qq!I?zNq)7%MF%(
zr^>O1O+yNARL$IzZ=b%cRe=A@kEyY*hv!7Rr%^Ul!gnwe{s
zMi(v=`WA3D(Yco7cHKv9EUvDd_@?o=%R%kL8|^4oy>yptpJnpJp(gTM=!pEBIT
zzvz$`NtNi9yEEQ)Z6C;9eyXLZXb}{!*r8d}LCR#b1F@4LO{3l^B7=->`9Yw34BpXL
z_s2<7N)1Z=^;DV;cxOR}7K)K)mmxhd>YFXuy@M{*V@j-v;qT^T58T^;C*Nh_#y#WP
zm(G5a%n8mLbX=E{$;FpS-Oi@Ams8@A({hsv!`cHU4F>t#z$^5oJX%7IABqvj^zW7H0XZQ^)zD0zL^+locM55
z$2D12s_d0RZQ^?VXIM+D_u9eb9{B3hHbn4!WooJ-SO%sO2(3+w
z;fmY$=7NKqoRx_FmJ1RAa|ZUXHg(pYb3d7#gz$(w%D<}P<;+|t$CD6Le;jL7
zw^pC-m)>et_RWfIg(0+C7boo^3m)0m~Q
zRr{{sbpNZ>_Flax<~B_9<7;r_^Hpm4f+H8RxrJ2s$etDUi1{nxMIh51E_W1rU|7%M
zCFzze0Ky~0Ru${_+Tgl9P>>x*Fca+E$^jq!Zfhp{Y`RB-WeRvpk+a59cKS_
zxW7o>htG@T6AMAPq1PPA`QWz`8XblYiT0Aaq`pD7TiLLDX!_E0Y
zMDOFc!VpZ^Fb*24Ag{0__E!Q6^Ps5P^p%ljH7r{v{VT4(^P~{Er)`%@-lIk|y-)5xR7M9FGwQK{TC
z@cvCXU@Ybko_G?aPn_>Ot$9S~4*_8$qCTn#=bbdAN%gx#Svh^?5W*oEyLICu_hv`-
zW}?xuEXZz{I9i`A7ddECC7oT>gh>l*svK!QNiDW*;@)#T%>L~#w%GpQj|

Re: [libvirt PATCH v3 0/4] docs: Unclutter top-level directory

[Andrea Bolognani]

On Tue, Jan 04, 2022 at 09:45:16AM -0500, Andrea Bolognani wrote:
> On Tue, Jan 04, 2022 at 02:25:14PM +, Daniel P. Berrangé wrote:
> > On Tue, Jan 04, 2022 at 02:55:58PM +0100, Andrea Bolognani wrote:
> > >  docs/{ => images}/node.gif| Bin
> >
> > Ew, just noticed "gif" file, how did we let that get in here.
> > We should burn it [1] !
> >
> > But seriously, pre-existing + tangential problem so not actually
> > asking you to fix it in this series, unless you fancy it.
>
> Yeah, I noticed that as well while preparing these patches and had
> the very same reaction :) Sort of forgot about it in the meantime,
> but now that you've reminded me I'll look into addressing it with a
> follow-up patch.

Done.

  https://listman.redhat.com/archives/libvir-list/2022-January/msg00317.html

-- 
Andrea Bolognani / Red Hat / Virtualization

[libvirt PATCH 1/2] docs: Update node.svg

[Andrea Bolognani]

Specifically, resize the document so that it's as big as the
contents and re-align some of the elements.

Signed-off-by: Andrea Bolognani 
---
 docs/images/node.svg | 174 +++
 1 file changed, 143 insertions(+), 31 deletions(-)

diff --git a/docs/images/node.svg b/docs/images/node.svg
index 4069d43d7f..a4db123ad6 100644
--- a/docs/images/node.svg
+++ b/docs/images/node.svg
@@ -1,36 +1,148 @@
 
 
+
 
+
 
-http://www.w3.org/2000/svg;
-   xmlns:xlink="http://www.w3.org/1999/xlink;
-   width="150pt" height="159pt"
-   viewBox="963 1488 2490 2649">
-
-
-
-
-Domain
-
-
-
-Domain
-
-
-
-Domain
-
-
-
-
-
-Hypervisor
-
-Node
-
+
+http://www.inkscape.org/namespaces/inkscape;
+   xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd;
+   xmlns="http://www.w3.org/2000/svg;
+   xmlns:svg="http://www.w3.org/2000/svg;>
+  
+  
+  
+
+Domain
+
+Domain
+
+Domain
+
+
+Hypervisor
+Node
+  
 
-- 
2.31.1

[libvirt PATCH 0/2] docs: Replace node.gif with node.png

[Andrea Bolognani]

Andrea Bolognani (2):
  docs: Update node.svg
  docs: Replace node.gif with node.png

 docs/goals.html.in  |   2 +-
 docs/images/meson.build |   2 +-
 docs/images/node.gif| Bin 1397 -> 0 bytes
 docs/images/node.png| Bin 0 -> 5532 bytes
 docs/images/node.svg| 174 +---
 5 files changed, 145 insertions(+), 33 deletions(-)
 delete mode 100644 docs/images/node.gif
 create mode 100644 docs/images/node.png

-- 
2.31.1

Re: [libvirt PATCH v2 0/1] Improve usage information and manual pages

[Andrea Bolognani]

On Mon, Dec 13, 2021 at 02:44:50PM +0100, Andrea Bolognani wrote:
> Changes from [v1]:
>
>   * the commit message has been updated to include the rationale
> for adding a manual page for virt-ssh-helper.
>
> [v1] 
> https://listman.redhat.com/archives/libvir-list/2021-December/msg00466.html
>
> Andrea Bolognani (1):
>   virt-ssh-helper: Add manual page

ping

-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH] src: Don't check for retval of g_strsplit()

[Michal Privoznik]

The g_strsplit() function can return NULL if and only if either
the input string is NULL or delimiter is NULL or an empty string.
In neither of places we call it any of the conditions is true and
thus we don't need to check for the return value.

Signed-off-by: Michal Privoznik 
---
 src/libxl/xen_xl.c | 2 --
 src/util/vircgroupv2.c | 2 --
 src/util/virprocess.c  | 2 --
 src/util/virresctrl.c  | 2 --
 4 files changed, 8 deletions(-)

diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 7604e3d534..869083a1d1 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -265,8 +265,6 @@ xenParseXLCPUID(virConf *conf, virDomainDef *def)
 }
 
 cpuid_pairs = g_strsplit(cpuid_str, ",", 0);
-if (!cpuid_pairs)
-return -1;
 
 if (!cpuid_pairs[0])
 return 0;
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 4c110940cf..f00a8f154b 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -286,8 +286,6 @@ virCgroupV2ParseControllersFile(virCgroup *group,
 virTrimSpaces(contStr, NULL);
 
 contList = g_strsplit(contStr, " ", 20);
-if (!contList)
-return -1;
 
 tmp = contList;
 
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index b559a4257e..06767dbf51 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1837,8 +1837,6 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
 return -1;
 
 lines = g_strsplit(data, "\n", 0);
-if (!lines)
-return -1;
 
 for (i = 0; lines[i] != NULL; i++) {
 const char *line = lines[i];
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index edbf078654..fe45ad3c64 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -1646,8 +1646,6 @@ virResctrlAllocParseCacheLine(virResctrlInfo *resctrl,
 }
 
 caches = g_strsplit(tmp, ";", 0);
-if (!caches)
-return 0;
 
 for (next = caches; *next; next++) {
 if (virResctrlAllocParseProcessCache(resctrl, alloc, level, type, 
*next) < 0)
-- 
2.34.1

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Ani Sinha]

On Fri, 7 Jan 2022, Michal Prívozník wrote:

> On 1/7/22 13:38, Ani Sinha wrote:
> >
>
> >
> > Ok fine but still, life is not ideal ... libraries do have bugs.
>
> In that case, where do we draw the line? Say pthread has a bug and it
> doesn't spawn threads. Worse, it doesn't even return any error value.
> Should we mitigate that too?
>
> I'd say stick with what's documented (abort on OOM and/or !NULL
> returned) and make our lives simpler. In fact, if we'd crash because we
> accessed NULL we will immediately see where and why and can report the
> bug to glib for benefit of us and others.

Yes, so if I were you, I would not simply remove the check. I would
replace it with assert().

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Michal Prívozník]

On 1/7/22 13:38, Ani Sinha wrote:
> 

> 
> Ok fine but still, life is not ideal ... libraries do have bugs.

In that case, where do we draw the line? Say pthread has a bug and it
doesn't spawn threads. Worse, it doesn't even return any error value.
Should we mitigate that too?

I'd say stick with what's documented (abort on OOM and/or !NULL
returned) and make our lives simpler. In fact, if we'd crash because we
accessed NULL we will immediately see where and why and can report the
bug to glib for benefit of us and others.

Michal

Re: [PATCH] do not report generic OPERATION_FAILED error when calling virConnectOpenAuth()

[Ani Sinha]

Ping ...

On Thu, 6 Jan 2022, Ani Sinha wrote:

> virConnectOpenAuth() calls virConnectOpenInternal(). This later function
> generates fine grained errors arising from various failure conditions that are
> more accurate than a "catch all" broader VIR_ERR_OPERATION_FAILED error that
> the callers of this function generates. Remove the broader error so that more
> specific errors can be caught and processed.
>
> Signed-off-by: Ani Sinha 
> ---
>  src/libxl/libxl_migration.c | 3 ---
>  src/qemu/qemu_migration.c   | 3 ---
>  2 files changed, 6 deletions(-)
>
> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index 6d0ab4ee28..bc2b5401da 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -1134,9 +1134,6 @@ libxlDomainMigrationSrcPerformP2P(libxlDriverPrivate 
> *driver,
>  virObjectLock(vm);
>
>  if (dconn == NULL) {
> -virReportError(VIR_ERR_OPERATION_FAILED,
> -   _("Failed to connect to remote libvirt URI %s: %s"),
> -   dconnuri, virGetLastErrorMessage());
>  return ret;
>  }
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index b9d7d582f5..2635ef1162 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5145,9 +5145,6 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver *driver,
>  goto cleanup;
>
>  if (dconn == NULL) {
> -virReportError(VIR_ERR_OPERATION_FAILED,
> -   _("Failed to connect to remote libvirt URI %s: %s"),
> -   dconnuri, virGetLastErrorMessage());
>  return -1;
>  }
>
> --
> 2.25.1
>
>

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Ani Sinha]

On Fri, 7 Jan 2022, Daniel P. Berrangé wrote:

> On Fri, Jan 07, 2022 at 06:04:04PM +0530, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Daniel P. Berrangé wrote:
> >
> > > On Fri, Jan 07, 2022 at 05:24:18PM +0530, Ani Sinha wrote:
> > > >
> > > >
> > > > On Fri, 7 Jan 2022, Ján Tomko wrote:
> > > >
> > > > > On a Friday in 2022, Ani Sinha wrote:
> > > > > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > > > > I don't think so. Just like we've discussed under one patch of 
> > > > > > > yours, a
> > > > > > > function should either report error in all cases or none. And in 
> > > > > > > case of
> > > > > > > virProcessGetSchedInfo() the linux version does report error
> > > > > >
> > > > > > I see your point but there is also a bug in that function - not all 
> > > > > > error
> > > > > > paths report errors. For example, !proc and !lines cases. We need 
> > > > > > to fix
> > > > > > that.
> > > > > >
> > > > >
> > > > > I don't see a !proc error path in virProcessGetSchedInfo.
> > > > >
> > > >
> > > >if (tid)
> > > > proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, 
> > > > (int)
> > > > tid);
> > > > else
> > > > proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> > > > if (!proc)
> > > > return -1; <=== not reported
> > >
> > > g_strdup_printf can't fail unless we mangled the printf format string
> > > (which we havent), so that 'if (!proc)' check is redundant / unreachable
> > >
> >
> > I am ok with Michal's patch that removes this check. However, such
> > assumptutions does makes me nervous. Since we do not really have
> > control over the library that implements g_strdup_printf, what if the
> > "can't fail" logic changes one day? This can happen deliberately or due to
> > some bug in the library. Does it not make sense to be defensive as the
> > consumer of this function and write code that is future proof?
>
> Their API spec guarantees we are safe
>
> https://docs.gtk.org/glib/func.strdup_printf.html
>
>   "The returned string is guaranteed to be non-NULL, unless format contains 
> %lc or %ls conversions, which can fail if no multibyte representation is 
> available for the given character."
>

Ok fine but still, life is not ideal ... libraries do have bugs.

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Daniel P . Berrangé]

On Fri, Jan 07, 2022 at 06:04:04PM +0530, Ani Sinha wrote:
> 
> 
> On Fri, 7 Jan 2022, Daniel P. Berrangé wrote:
> 
> > On Fri, Jan 07, 2022 at 05:24:18PM +0530, Ani Sinha wrote:
> > >
> > >
> > > On Fri, 7 Jan 2022, Ján Tomko wrote:
> > >
> > > > On a Friday in 2022, Ani Sinha wrote:
> > > > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > > > I don't think so. Just like we've discussed under one patch of 
> > > > > > yours, a
> > > > > > function should either report error in all cases or none. And in 
> > > > > > case of
> > > > > > virProcessGetSchedInfo() the linux version does report error
> > > > >
> > > > > I see your point but there is also a bug in that function - not all 
> > > > > error
> > > > > paths report errors. For example, !proc and !lines cases. We need to 
> > > > > fix
> > > > > that.
> > > > >
> > > >
> > > > I don't see a !proc error path in virProcessGetSchedInfo.
> > > >
> > >
> > >if (tid)
> > > proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
> > > tid);
> > > else
> > > proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> > > if (!proc)
> > > return -1; <=== not reported
> >
> > g_strdup_printf can't fail unless we mangled the printf format string
> > (which we havent), so that 'if (!proc)' check is redundant / unreachable
> >
> 
> I am ok with Michal's patch that removes this check. However, such
> assumptutions does makes me nervous. Since we do not really have
> control over the library that implements g_strdup_printf, what if the
> "can't fail" logic changes one day? This can happen deliberately or due to
> some bug in the library. Does it not make sense to be defensive as the
> consumer of this function and write code that is future proof?

Their API spec guarantees we are safe

https://docs.gtk.org/glib/func.strdup_printf.html

  "The returned string is guaranteed to be non-NULL, unless format contains %lc 
or %ls conversions, which can fail if no multibyte representation is available 
for the given character."

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: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Ani Sinha]

On Fri, 7 Jan 2022, Daniel P. Berrangé wrote:

> On Fri, Jan 07, 2022 at 05:24:18PM +0530, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Ján Tomko wrote:
> >
> > > On a Friday in 2022, Ani Sinha wrote:
> > > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > > I don't think so. Just like we've discussed under one patch of yours, 
> > > > > a
> > > > > function should either report error in all cases or none. And in case 
> > > > > of
> > > > > virProcessGetSchedInfo() the linux version does report error
> > > >
> > > > I see your point but there is also a bug in that function - not all 
> > > > error
> > > > paths report errors. For example, !proc and !lines cases. We need to fix
> > > > that.
> > > >
> > >
> > > I don't see a !proc error path in virProcessGetSchedInfo.
> > >
> >
> >if (tid)
> > proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
> > tid);
> > else
> > proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> > if (!proc)
> > return -1; <=== not reported
>
> g_strdup_printf can't fail unless we mangled the printf format string
> (which we havent), so that 'if (!proc)' check is redundant / unreachable
>

I am ok with Michal's patch that removes this check. However, such
assumptutions does makes me nervous. Since we do not really have
control over the library that implements g_strdup_printf, what if the
"can't fail" logic changes one day? This can happen deliberately or due to
some bug in the library. Does it not make sense to be defensive as the
consumer of this function and write code that is future proof?

Re: [PATCH] report error when virProcessGetStatInfo() is unable to parse data

[Michal Prívozník]

On 1/7/22 12:55, Ani Sinha wrote:
> 
> 
> On Fri, 7 Jan 2022, Michal Prívozník wrote:
> 
>> On 1/7/22 10:09, Ani Sinha wrote:
>>> Currently virProcessGetStatInfo() always returns success and only logs error
>>> when it is unable to parse the data. Make this function actually report the
>>> error and return a negative value in this error scenario.
>>>
>>> Signed-off-by: Ani Sinha 
>>> ---
>>>  src/util/virprocess.c | 6 +-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
>>> index c74bd16fe6..b9f498d5d8 100644
>>> --- a/src/util/virprocess.c
>>> +++ b/src/util/virprocess.c
>>> @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
>>>  virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, 
>>> ) < 0 ||
>>>  virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 
>>> 0 ||
>>>  virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, 
>>> ) < 0) {
>>> -VIR_WARN("cannot parse process status data");
>>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +   _("cannot parse process status data for pid 
>>> '%d/%d'"),
>>> +   (int) pid, (int) tid);
>>> +
>>> +return -1;
>>>  }
>>>
>>>  /* We got jiffies
>>
>> Couple of problems with this patch as is. I'm not against the idea of
>> reporting an error here.
> 
> Good. now we are moving in the right direction.
> 
> But couple of things needs to be addressed first:
>>
>> 1) Currently, all callers check for retval and report an error if -1 was
>> returned. This means, that even though this new message is reported it
>> is immediately overwritten in caller.
> 
> Let me fix the callers and send an updated patch. Meanwhile ...
> 
>>
>> 2) The non-linux implementation now has to report error too. I believe
>> it's obvious why from our previous discussion this morning.
> 
> Maybe you can fix your patch.

There is nothing to fix. With current master nor Linux nor non-Linux
variant reports any error, i.e. are consistent. This patch introduced
inconsistency and in my review I have pointed it out. Looking forward to v2.

Michal

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Michal Prívozník]

On 1/7/22 09:34, Ani Sinha wrote:
> 
> 
> On Fri, 7 Jan 2022, Michal Prívozník wrote:
> 
>> On 1/7/22 09:05, Ani Sinha wrote:
>>>
>>>
>>> On Fri, 7 Jan 2022, Michal Privoznik wrote:
>>>
 Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
 Linux centric. Provide stubs for non-Linux platforms.

 Signed-off-by: Michal Privoznik 
 ---
  src/util/virprocess.c | 24 
  1 file changed, 24 insertions(+)

 diff --git a/src/util/virprocess.c b/src/util/virprocess.c
 index c74bd16fe6..5788faea9c 100644
 --- a/src/util/virprocess.c
 +++ b/src/util/virprocess.c
 @@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
  }


 +#ifdef __linux__
  int
  virProcessGetStatInfo(unsigned long long *cpuTime,
int *lastCpu,
 @@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,

  return 0;
  }
 +
 +#else
 +int
 +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
 +  int *lastCpu G_GNUC_UNUSED,
 +  long *vm_rss G_GNUC_UNUSED,
 +  pid_t pid G_GNUC_UNUSED,
 +  pid_t tid G_GNUC_UNUSED)
 +{
 +errno = ENOSYS;
 +return -1;
 +}
 +
 +int
 +virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
 +   pid_t pid G_GNUC_UNUSED,
 +   pid_t tid G_GNUC_UNUSED)
 +{
 +virReportSystemError(ENOSYS, "%s",
 + _("scheduler information is not supported on 
 this platform"));
>>>
>>> We should do this for both functions for non-linux platforms or not do it
>>> at all. Like it should be consistent IMHO.
>>
>> I don't think so. Just like we've discussed under one patch of yours, a
>> function should either report error in all cases or none. And in case of
>> virProcessGetSchedInfo() the linux version does report error
> 
> I see your point but there is also a bug in that function - not all error
> paths report errors. For example, !proc and !lines cases. We need to fix
> that.
> 

D'oh! I knew I forgot something in the other patch I've sent today:

https://listman.redhat.com/archives/libvir-list/2022-January/msg00270.html

g_strsplit() won't return NULL on sensible inputs (i.e. !NULL). Let me
rerun my spatch and post another patch.

> hence the
>> non-linux variant should report an error too. And in case of
>> virProcessGetStatInfo() no error is reported for linux version thus
>> non-linux version shouldn't report an error.
>>
> 
> No this is not my understanding from the discussion. What I understood is
> that the lowest level of functions should always report error when an
> error path is encountered. For example virFileReadAll() does this nicely.
> Currently there is no  error path in virProcessGetStatInfo() and it
> uncondiotionally returns 0. For non-linux variant, I think it would be
> correct to report an error.
> 

Yes and no. Reporting error where it happens is okay most of the time
[1]. But having a function that's consistent in reporting or not
reporting an error in all cases is above that. I'm failing to see how
one can write effective code if a function reports error in some cases
but it doesn't in others. Especially if you want to report less generic
errors and more specific ones.

1: In a few cases it may be worth reporting more generic error because
it may be more user friendly and thus shed more light into what's going
on. But this situation is very rare.

> Now having done that, we should also fix the callers so that the callers
> are not overriding the narrower errors with broader ones.

Agreed, see my reply to the patch you posted.

Michal

[PATCH v2] report error when virProcessGetStatInfo() is unable to parse data

[Ani Sinha]

Currently virProcessGetStatInfo() always returns success and only logs error
when it is unable to parse the data. Make this function actually report the
error and return a negative value in this error scenario.

Fix the callers so that they do not override the error generated.

Signed-off-by: Ani Sinha 
---
 src/ch/ch_driver.c | 2 --
 src/qemu/qemu_driver.c | 7 +--
 src/util/virprocess.c  | 6 +-
 3 files changed, 6 insertions(+), 9 deletions(-)

changelog:
v2: fixed the callers

diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 53e0872207..3cbc668489 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -1073,8 +1073,6 @@ chDomainHelperGetVcpus(virDomainObj *vm,
 if (virProcessGetStatInfo(>cpuTime,
   >cpu, NULL,
   vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
-  _("cannot get vCPU placement & pCPU 
time"));
 return -1;
 }
 }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4974450333..015ffb2ce7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1359,8 +1359,6 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
 if (virProcessGetStatInfo(>cpuTime,
   >cpu, NULL,
   vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU 
time"));
 return -1;
 }
 }
@@ -2521,8 +2519,6 @@ qemuDomainGetInfo(virDomainPtr dom,
 if (virDomainObjIsActive(vm)) {
 if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL,
   vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-   _("cannot read cputime for domain"));
 goto cleanup;
 }
 }
@@ -10530,8 +10526,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriver *driver,
 }
 
 if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-   _("cannot get RSS for domain"));
+return -1;
 } else {
 stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
 stats[ret].val = rss;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index c74bd16fe6..b9f498d5d8 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
 virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, 
) < 0 ||
 virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
 virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, ) 
< 0) {
-VIR_WARN("cannot parse process status data");
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("cannot parse process status data for pid '%d/%d'"),
+   (int) pid, (int) tid);
+
+return -1;
 }
 
 /* We got jiffies
-- 
2.25.1

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Ani Sinha]

On Fri, 7 Jan 2022, Ján Tomko wrote:

> On a Friday in 2022, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Ján Tomko wrote:
> >
> > > On a Friday in 2022, Ani Sinha wrote:
> > > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > > I don't think so. Just like we've discussed under one patch of yours,
> > > a
> > > > > function should either report error in all cases or none. And in case
> > > of
> > > > > virProcessGetSchedInfo() the linux version does report error
> > > >
> > > > I see your point but there is also a bug in that function - not all
> > > error
> > > > paths report errors. For example, !proc and !lines cases. We need to fix
> > > > that.
> > > >
> > >
> > > I don't see a !proc error path in virProcessGetSchedInfo.
> > >
> >
> >   if (tid)
> >proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
> > tid);
> >else
> >proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> >if (!proc)
> >return -1; <=== not reported
> >
>
> Oh, I did not realize that I had Michal's patch that removes it applied
> locally:
> https://listman.redhat.com/archives/libvir-list/2022-January/msg00270.html

Ah ok, all makes sense now.

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Ján Tomko]

On a Friday in 2022, Ani Sinha wrote:



On Fri, 7 Jan 2022, Ján Tomko wrote:


On a Friday in 2022, Ani Sinha wrote:
> On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > I don't think so. Just like we've discussed under one patch of yours, a
> > function should either report error in all cases or none. And in case of
> > virProcessGetSchedInfo() the linux version does report error
>
> I see your point but there is also a bug in that function - not all error
> paths report errors. For example, !proc and !lines cases. We need to fix
> that.
>

I don't see a !proc error path in virProcessGetSchedInfo.



  if (tid)
   proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
tid);
   else
   proc = g_strdup_printf("/proc/%d/sched", (int) pid);
   if (!proc)
   return -1; <=== not reported



Oh, I did not realize that I had Michal's patch that removes it applied locally:
https://listman.redhat.com/archives/libvir-list/2022-January/msg00270.html

Jano


signature.asc
Description: PGP signature

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Daniel P . Berrangé]

On Fri, Jan 07, 2022 at 05:24:18PM +0530, Ani Sinha wrote:
> 
> 
> On Fri, 7 Jan 2022, Ján Tomko wrote:
> 
> > On a Friday in 2022, Ani Sinha wrote:
> > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > I don't think so. Just like we've discussed under one patch of yours, a
> > > > function should either report error in all cases or none. And in case of
> > > > virProcessGetSchedInfo() the linux version does report error
> > >
> > > I see your point but there is also a bug in that function - not all error
> > > paths report errors. For example, !proc and !lines cases. We need to fix
> > > that.
> > >
> >
> > I don't see a !proc error path in virProcessGetSchedInfo.
> >
> 
>if (tid)
> proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
> tid);
> else
> proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> if (!proc)
> return -1; <=== not reported

g_strdup_printf can't fail unless we mangled the printf format string
(which we havent), so that 'if (!proc)' check is redundant / unreachable

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: [PATCH] report error when virProcessGetStatInfo() is unable to parse data

[Ani Sinha]

On Fri, 7 Jan 2022, Michal Prívozník wrote:

> On 1/7/22 10:09, Ani Sinha wrote:
> > Currently virProcessGetStatInfo() always returns success and only logs error
> > when it is unable to parse the data. Make this function actually report the
> > error and return a negative value in this error scenario.
> >
> > Signed-off-by: Ani Sinha 
> > ---
> >  src/util/virprocess.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index c74bd16fe6..b9f498d5d8 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> >  virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, 
> > ) < 0 ||
> >  virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 
> > 0 ||
> >  virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, 
> > ) < 0) {
> > -VIR_WARN("cannot parse process status data");
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("cannot parse process status data for pid 
> > '%d/%d'"),
> > +   (int) pid, (int) tid);
> > +
> > +return -1;
> >  }
> >
> >  /* We got jiffies
>
> Couple of problems with this patch as is. I'm not against the idea of
> reporting an error here.

Good. now we are moving in the right direction.

But couple of things needs to be addressed first:
>
> 1) Currently, all callers check for retval and report an error if -1 was
> returned. This means, that even though this new message is reported it
> is immediately overwritten in caller.

Let me fix the callers and send an updated patch. Meanwhile ...

>
> 2) The non-linux implementation now has to report error too. I believe
> it's obvious why from our previous discussion this morning.

Maybe you can fix your patch.

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Ani Sinha]

On Fri, 7 Jan 2022, Ján Tomko wrote:

> On a Friday in 2022, Ani Sinha wrote:
> > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > I don't think so. Just like we've discussed under one patch of yours, a
> > > function should either report error in all cases or none. And in case of
> > > virProcessGetSchedInfo() the linux version does report error
> >
> > I see your point but there is also a bug in that function - not all error
> > paths report errors. For example, !proc and !lines cases. We need to fix
> > that.
> >
>
> I don't see a !proc error path in virProcessGetSchedInfo.
>

   if (tid)
proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
tid);
else
proc = g_strdup_printf("/proc/%d/sched", (int) pid);
if (!proc)
return -1; <=== not reported


> The !lines case is inconsistent but thankfully it can only happen
> if /proc/%d/sched is empty.
>
> > hence the
> > > non-linux variant should report an error too. And in case of
> > > virProcessGetStatInfo() no error is reported for linux version thus
> > > non-linux version shouldn't report an error.
> > >
> >
> > No this is not my understanding from the discussion. What I understood is
> > that the lowest level of functions should always report error when an
> > error path is encountered.
>
> Errors should be reported by the function that has the context to
> provide a helpful error message.

Correct. Most often it is the lowest level functions.

For some low-level helpers, we rely
> on errno's and let the caller report a less-specific error - either
> out of laziness because (like above) there would have to be a buggy
> kernel, or because most of the code paths are syscalls which set errno.
>
> Jano
>
> > For example virFileReadAll() does this nicely.
> > Currently there is no  error path in virProcessGetStatInfo() and it
> > uncondiotionally returns 0. For non-linux variant, I think it would be
> > correct to report an error.
> >
> > Now having done that, we should also fix the callers so that the callers
> > are not overriding the narrower errors with broader ones.
>

Re: [PATCH] report error when virProcessGetStatInfo() is unable to parse data

[Michal Prívozník]

On 1/7/22 10:09, Ani Sinha wrote:
> Currently virProcessGetStatInfo() always returns success and only logs error
> when it is unable to parse the data. Make this function actually report the
> error and return a negative value in this error scenario.
> 
> Signed-off-by: Ani Sinha 
> ---
>  src/util/virprocess.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index c74bd16fe6..b9f498d5d8 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
>  virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, 
> ) < 0 ||
>  virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 
> ||
>  virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, 
> ) < 0) {
> -VIR_WARN("cannot parse process status data");
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("cannot parse process status data for pid '%d/%d'"),
> +   (int) pid, (int) tid);
> +
> +return -1;
>  }
>  
>  /* We got jiffies

Couple of problems with this patch as is. I'm not against the idea of
reporting an error here. But couple of things needs to be addressed first:

1) Currently, all callers check for retval and report an error if -1 was
returned. This means, that even though this new message is reported it
is immediately overwritten in caller.

2) The non-linux implementation now has to report error too. I believe
it's obvious why from our previous discussion this morning.

Michal

[libvirt PATCH v4 15/19] docs: Add hvf on QEMU driver page

[Andrea Bolognani]

From: Roman Bolshakov 

It's worth to make the domain type a little bit more visible than a row
in news. An example of hvf domain is available on QEMU driver page.

While at it, mention Hypervisor.framework on index page.

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 docs/drvqemu.rst   | 48 +++---
 docs/index.html.in |  1 +
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/docs/drvqemu.rst b/docs/drvqemu.rst
index e18075d865..2611194fe6 100644
--- a/docs/drvqemu.rst
+++ b/docs/drvqemu.rst
@@ -1,13 +1,18 @@
 .. role:: since
 .. role:: removed
 
-==
-KVM/QEMU hypervisor driver
-==
+==
+QEMU/KVM/HVF hypervisor driver
+==
 
 The libvirt KVM/QEMU driver can manage any QEMU emulator from version 2.11.0 or
 later.
 
+It supports multiple QEMU accelerators: software
+emulation also known as TCG, hardware-assisted virtualization on Linux
+with KVM and hardware-assisted virtualization on macOS with
+Hypervisor.framework (:since:`since 8.0.0`).
+
 .. contents::
 
 Project Links
@@ -15,6 +20,7 @@ Project Links
 
 -  The `KVM `__ Linux hypervisor
 -  The `QEMU `__ emulator
+-  
`Hypervisor.framework`__` 
reference
 
 Deployment pre-requisites
 -
@@ -27,6 +33,9 @@ Deployment pre-requisites
 -  **KVM hypervisor**: The driver will probe ``/usr/bin`` for the presence of
``qemu-kvm`` and ``/dev/kvm`` device node. If both are found, then KVM fully
virtualized, hardware accelerated guests will be available.
+-  **Hypervisor.framework (HVF)**: The driver will probe ``sysctl`` for the
+   presence of ``Hypervisor.framework``. If it is found and QEMU is newer than
+   2.12, then it will be possible to create hardware accelerated guests.
 
 Connections to QEMU driver
 --
@@ -634,3 +643,36 @@ KVM hardware accelerated guest on i686

  

+
+HVF hardware accelerated guest on x86_64
+
+
+::
+
+   
+ hvf-demo
+ 4dea24b3-1d52-d8f3-2516-782e98a23fa0
+ 131072
+ 1
+ 
+   hvm
+ 
+ 
+   
+ 
+ 
+ 
+   /usr/local/bin/qemu-system-x86_64
+   
+   
+ 
+ 
+ 
+   
+   
+ 
+ 
+   
+   
+ 
+   
diff --git a/docs/index.html.in b/docs/index.html.in
index bf164edb58..2c4aa7c6d0 100644
--- a/docs/index.html.in
+++ b/docs/index.html.in
@@ -21,6 +21,7 @@
 is accessible from C, Python, Perl, Go and more
 is licensed under open source licenses
 supports KVM,
+  Hypervisor.framework,
   QEMU, Xen,
   Virtuozzo,
   VMWare ESX,
-- 
2.31.1

[libvirt PATCH v4 19/19] fixup! NEWS: Mention Apple Silicon support for HVF

[Andrea Bolognani]

And simplify the entry by removing mentions of QEMU 2.12 and
macOS 10.10, which are both ancient at this point, and of
specific hardware features that are going to be present in any
recent Apple machine.

Signed-off-by: Andrea Bolognani 
---
 NEWS.rst | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/NEWS.rst b/NEWS.rst
index 56efaa9f72..85cbdd154f 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -55,11 +55,8 @@ v8.0.0 (unreleased)
 
   * qemu: Add hvf domain type for Hypervisor.framework
 
-QEMU introduced experimental support of Hypervisor.framework
-since 2.12.
-
-It's supported on machines with Intel VT-x feature set that includes
-Extended Page Tables (EPT) and Unrestricted Mode since macOS 10.10.
+It works on Intel machines as well as recent machines powered by Apple
+Silicon. QEMU 6.2.0 is needed for Apple Silicon support.
 
 
 * **Improvements**
-- 
2.31.1

[libvirt PATCH v4 18/19] news: Mention hvf domain type

[Andrea Bolognani]

From: Roman Bolshakov 

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 NEWS.rst | 9 +
 1 file changed, 9 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 7ee03fde0e..56efaa9f72 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -53,6 +53,15 @@ v8.0.0 (unreleased)
 ``domsetlaunchsecstate`` are added to support injecting a launch secret
 in a domain's memory.
 
+  * qemu: Add hvf domain type for Hypervisor.framework
+
+QEMU introduced experimental support of Hypervisor.framework
+since 2.12.
+
+It's supported on machines with Intel VT-x feature set that includes
+Extended Page Tables (EPT) and Unrestricted Mode since macOS 10.10.
+
+
 * **Improvements**
 
   * libxl: Implement the virDomainGetMessages API
-- 
2.31.1

[libvirt PATCH v4 17/19] docs: Add support page for libvirt on macOS

[Andrea Bolognani]

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.rst | 44 
 docs/meson.build   |  1 +
 4 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 docs/macos.rst

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.rst b/docs/macos.rst
new file mode 100644
index 00..7792ad8819
--- /dev/null
+++ b/docs/macos.rst
@@ -0,0 +1,44 @@
+.. role:: since
+
+=
+macOS support
+=
+
+.. contents::
+
+Libvirt works both as client (for most drivers) and server (for the
+`QEMU driver `__) on macOS.
+
+:since:`Since 8.0.0`, the "hvf" domain type can be used to run
+hardware-accelerated VMs on macOS via
+`Hypervisor.framework 
`__.
+QEMU version 2.12 or newer is needed for this to work.
+
+
+Installation
+
+
+libvirt client (virsh), server (libvirtd) and development headers can be
+installed from `Homebrew `__:
+
+::
+
+   brew install libvirt
+
+
+Running libvirtd locally
+
+
+The server can be started manually:
+
+::
+
+   $ libvirtd
+
+or on system boot:
+
+::
+
+   $ brew services start libvirt
+
+Once started, you can use virsh as you would on Linux.
diff --git a/docs/meson.build b/docs/meson.build
index 3e912f21ad..717d3c025c 100644
--- a/docs/meson.build
+++ b/docs/meson.build
@@ -106,6 +106,7 @@ docs_rst_files = [
   'hacking',
   'libvirt-go',
   'libvirt-go-xml',
+  'macos',
   'migration',
   'newreposetup',
   'pci-addresses',
-- 
2.31.1

[libvirt PATCH v4 16/19] docs: Note hvf support for domain elements

[Andrea Bolognani]

From: Roman Bolshakov 

Many domain elements have "QEMU and KVM only" or "QEMU/KVM since x.y.z"
remarks. Most of the elements work for HVF domain, so it makes sense to
add respective notices for HVF domain.

All the elements have been manually tested.

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 docs/formatdomain.rst | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 3e9de05249..e27fb23119 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1426,7 +1426,8 @@ In case no restrictions need to be put on CPU model and 
its features, a simpler
   :since:`Since 7.1.0` with the QEMU driver.
 
Both ``host-model`` and ``host-passthrough`` modes make sense when a domain
-   can run directly on the host CPUs (for example, domains with type ``kvm``).
+   can run directly on the host CPUs (for example, domains with type ``kvm``
+   or ``hvf``).
The actual host CPU is irrelevant for domains with emulated virtual CPUs
(such as domains with type ``qemu``). However, for backward compatibility
``host-model`` may be implemented even for domains running on emulated CPUs
@@ -1750,7 +1751,7 @@ Each of these states allow for the same four possible 
actions.
The domain will be terminated and then restarted with a new name. (Only
supported by the libxl hypervisor driver.)
 
-QEMU/KVM supports the ``on_poweroff`` and ``on_reboot`` events handling the
+QEMU/KVM/HVF supports the ``on_poweroff`` and ``on_reboot`` events handling the
 ``destroy`` and ``restart`` actions, but the combination of ``on_poweroff`` set
 to ``restart`` and ``on_reboot`` set to ``destroy`` is forbidden.
 
@@ -1885,8 +1886,8 @@ are:
Physical address extension mode allows 32-bit guests to address more than 4
GB of memory.
 ``acpi``
-   ACPI is useful for power management, for example, with KVM guests it is
-   required for graceful shutdown to work.
+   ACPI is useful for power management, for example, with KVM or HVF guests it
+   is required for graceful shutdown to work.
 ``apic``
APIC allows the use of programmable IRQ management. :since:`Since 0.10.2
(QEMU only)` there is an optional attribute ``eoi`` with values ``on`` and
@@ -6195,14 +6196,16 @@ A video device.
 
You can provide the amount of video memory in kibibytes (blocks of 1024
bytes) using ``vram``. This is supported only for guest type of "vz", 
"qemu",
-   "vbox", "vmx" and "xen". If no value is provided the default is used. If the
+   "kvm", "hvf", "vbox", "vmx" and "xen".
+   If no value is provided the default is used. If the
size is not a power of two it will be rounded to closest one.
 
The number of screen can be set using ``heads``. This is supported only for
-   guests type of "vz", "kvm", "vbox" and "vmx".
+   guests type of "vz", "kvm", "hvf", "vbox" and "vmx".
 
-   For guest type of "kvm" or "qemu" and model type "qxl" there are optional
-   attributes. Attribute ``ram`` ( :since:`since 1.0.2` ) specifies the size of
+   For guest type of "kvm", "hvf" or "qemu" and model type "qxl" there are
+   optional attributes.
+   Attribute ``ram`` ( :since:`since 1.0.2` ) specifies the size of
the primary bar, while the attribute ``vram`` specifies the secondary bar
size. If ``ram`` or ``vram`` are not supplied a default value is used. The
``ram`` should also be rounded to power of two as ``vram``. There is also
-- 
2.31.1

[libvirt PATCH v4 14/19] tests: Add HVF test cases

[Andrea Bolognani]

We need to use a hardcoded list of capabilities because we don't
yet have proper replies files obtained from QEMU running on actual
macOS machines.

Signed-off-by: Andrea Bolognani 
---
 .../hvf-aarch64-virt-headless.args| 48 +
 .../hvf-aarch64-virt-headless.xml | 45 +
 .../hvf-x86_64-q35-headless.args  | 47 +
 .../hvf-x86_64-q35-headless.x86_64-latest.err |  1 +
 .../hvf-x86_64-q35-headless.xml   | 44 +
 tests/qemuxml2argvtest.c  | 18 
 .../hvf-aarch64-virt-headless.xml | 94 ++
 .../hvf-x86_64-q35-headless.xml   | 97 +++
 tests/qemuxml2xmltest.c   | 16 +++
 9 files changed, 410 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/hvf-aarch64-virt-headless.args
 create mode 100644 tests/qemuxml2argvdata/hvf-aarch64-virt-headless.xml
 create mode 100644 tests/qemuxml2argvdata/hvf-x86_64-q35-headless.args
 create mode 100644 
tests/qemuxml2argvdata/hvf-x86_64-q35-headless.x86_64-latest.err
 create mode 100644 tests/qemuxml2argvdata/hvf-x86_64-q35-headless.xml
 create mode 100644 tests/qemuxml2xmloutdata/hvf-aarch64-virt-headless.xml
 create mode 100644 tests/qemuxml2xmloutdata/hvf-x86_64-q35-headless.xml

diff --git a/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.args 
b/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.args
new file mode 100644
index 00..0f1eed66c2
--- /dev/null
+++ b/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.args
@@ -0,0 +1,48 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-test \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-test/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-test/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-test/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-aarch64 \
+-name guest=test,debug-threads=on \
+-S \
+-object 
secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-test/master-key.aes \
+-machine virt,usb=off,dump-guest-core=off,gic-version=2 \
+-accel hvf \
+-drive 
file=/usr/share/edk2/aarch64/QEMU_EFI-silent-pflash.raw,if=pflash,format=raw,unit=0,readonly=on
 \
+-drive 
file=/var/lib/libvirt/qemu/nvram/test_VARS.fd,if=pflash,format=raw,unit=1 \
+-m 4096 \
+-realtime mlock=off \
+-smp 2,sockets=2,cores=1,threads=1 \
+-uuid 1b826c23-8767-47ad-a6b5-c83a88277f71 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-test/monitor.sock,server=on,wait=off
 \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc,driftfix=slew \
+-no-shutdown \
+-boot strict=on \
+-device 
pcie-root-port,port=8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x1 \
+-device pcie-root-port,port=9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \
+-device pcie-root-port,port=10,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x2 \
+-device pcie-root-port,port=11,chassis=4,id=pci.4,bus=pcie.0,addr=0x1.0x3 \
+-device pcie-root-port,port=12,chassis=5,id=pci.5,bus=pcie.0,addr=0x1.0x4 \
+-device pcie-root-port,port=13,chassis=6,id=pci.6,bus=pcie.0,addr=0x1.0x5 \
+-device virtio-serial-pci,id=virtio-serial0,bus=pci.2,addr=0x0 \
+-drive 
file=/var/lib/libvirt/images/test.qcow2,format=qcow2,if=none,id=drive-virtio-disk0
 \
+-device 
virtio-blk-pci,bus=pci.3,addr=0x0,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
 \
+-netdev user,id=hostnet0 \
+-device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:9a:e6:c6,bus=pci.1,addr=0x0 
\
+-chardev pty,id=charserial0 \
+-serial chardev:charserial0 \
+-chardev 
socket,id=charchannel0,path=/tmp/channel/domain--1-test/org.qemu.guest_agent.0,server=on,wait=off
 \
+-device 
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0
 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.4,addr=0x0 \
+-object rng-random,id=objrng0,filename=/dev/urandom \
+-device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.5,addr=0x0 \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.xml 
b/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.xml
new file mode 100644
index 00..ef13820e17
--- /dev/null
+++ b/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.xml
@@ -0,0 +1,45 @@
+
+  test
+  1b826c23-8767-47ad-a6b5-c83a88277f71
+  4194304
+  4194304
+  2
+  
+hvm
+/usr/share/edk2/aarch64/QEMU_EFI-silent-pflash.raw
+/var/lib/libvirt/qemu/nvram/test_VARS.fd
+
+  
+  
+
+  
+  
+
+
+
+  
+  destroy
+  restart
+  restart
+  
+/usr/bin/qemu-system-aarch64
+
+  
+  
+  
+
+
+
+  
+  
+
+
+
+  
+
+
+
+  /dev/urandom
+
+  
+
diff --git a/tests/qemuxml2argvdata/hvf-x86_64-q35-headless.args 
b/tests/qemuxml2argvdata/hvf-x86_64-q35-headless.args
new file mode 100644
index 00..b3358e3d59
--- /dev/null
+++ b/tests/qemuxml2argvdata/hvf-x86_64-q35-headless.args
@@ -0,0 +1,47 @@
+LC_ALL=C \
+PATH=/bin \

[libvirt PATCH v4 11/19] tests: Introduce testQemuHostOS

[Andrea Bolognani]

This new enumeration provides a way to specify the host OS
that a specific test case expects. The default is Linux, which
has been the implicit host OS until now; when Linux is selected
as the host OS, KVM support is advertised in capabilies data
exposed to test cases.

This commit doesn't result in any functional change, and simply
sets the stage for introducing macOS host OS support later.

Signed-off-by: Andrea Bolognani 
---
 tests/testutilsqemu.c | 87 +++
 tests/testutilsqemu.h |  4 ++
 2 files changed, 58 insertions(+), 33 deletions(-)

diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index cb665e501b..b9835782da 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -160,7 +160,8 @@ virHostCPUX86GetCPUID(uint32_t leaf,
 
 static int
 testQemuAddGuest(virCaps *caps,
- virArch arch)
+ virArch arch,
+ testQemuHostOS hostOS)
 {
 size_t nmachines;
 virCapsGuestMachine **machines = NULL;
@@ -193,16 +194,18 @@ testQemuAddGuest(virCaps *caps,
 virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU,
   NULL, NULL, 0, NULL);
 
-if (kvm_machines[emu_arch] != NULL) {
-nmachines = g_strv_length((char **)kvm_machines[emu_arch]);
-machines = virCapabilitiesAllocMachines(kvm_machines[emu_arch],
-nmachines);
-if (machines == NULL)
-goto error;
-
-virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM,
-  qemu_emulators[emu_arch],
-  NULL, nmachines, machines);
+if (hostOS == HOST_OS_LINUX) {
+if (kvm_machines[emu_arch] != NULL) {
+nmachines = g_strv_length((char **)kvm_machines[emu_arch]);
+machines = virCapabilitiesAllocMachines(kvm_machines[emu_arch],
+nmachines);
+if (machines == NULL)
+goto error;
+
+virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM,
+  qemu_emulators[emu_arch],
+  NULL, nmachines, machines);
+}
 }
 
 return 0;
@@ -213,7 +216,8 @@ testQemuAddGuest(virCaps *caps,
 }
 
 
-virCaps *testQemuCapsInit(void)
+static virCaps*
+testQemuCapsInitImpl(testQemuHostOS hostOS)
 {
 virCaps *caps;
 size_t i;
@@ -233,7 +237,7 @@ virCaps *testQemuCapsInit(void)
 goto cleanup;
 
 for (i = 0; i < VIR_ARCH_LAST; i++) {
-if (testQemuAddGuest(caps, i) < 0)
+if (testQemuAddGuest(caps, i, hostOS) < 0)
 goto cleanup;
 }
 
@@ -255,6 +259,12 @@ virCaps *testQemuCapsInit(void)
 return NULL;
 }
 
+virCaps*
+testQemuCapsInit(void)
+{
+return testQemuCapsInitImpl(HOST_OS_LINUX);
+}
+
 
 void
 qemuTestSetHostArch(virQEMUDriver *driver,
@@ -335,10 +345,10 @@ void qemuTestDriverFree(virQEMUDriver *driver)
 virCPUDefFree(cpuPower9);
 }
 
-
 static void
 qemuTestCapsPopulateFakeMachines(virQEMUCaps *caps,
- virArch arch)
+ virArch arch,
+ testQemuHostOS hostOS)
 {
 size_t i;
 const char *defaultRAMid = NULL;
@@ -366,20 +376,22 @@ qemuTestCapsPopulateFakeMachines(virQEMUCaps *caps,
 virQEMUCapsSet(caps, QEMU_CAPS_TCG);
 }
 
-if (kvm_machines[arch] != NULL) {
-for (i = 0; kvm_machines[arch][i] != NULL; i++) {
-virQEMUCapsAddMachine(caps,
-  VIR_DOMAIN_VIRT_KVM,
-  kvm_machines[arch][i],
-  NULL,
-  NULL,
-  0,
-  false,
-  false,
-  true,
-  defaultRAMid,
-  false);
-virQEMUCapsSet(caps, QEMU_CAPS_KVM);
+if (hostOS == HOST_OS_LINUX) {
+if (kvm_machines[arch] != NULL) {
+for (i = 0; kvm_machines[arch][i] != NULL; i++) {
+virQEMUCapsAddMachine(caps,
+  VIR_DOMAIN_VIRT_KVM,
+  kvm_machines[arch][i],
+  NULL,
+  NULL,
+  0,
+  false,
+  false,
+  true,
+  defaultRAMid,
+  false);
+virQEMUCapsSet(caps, QEMU_CAPS_KVM);
+}
 }
 }
 }
@@ -399,8 +411,10 @@ qemuTestCapsCacheInsertData(virFileCache *cache,
 }
 
 
-int 

[libvirt PATCH v4 13/19] tests: Add macOS support to qemuxml2*test

[Andrea Bolognani]

The new DO_TEST_MACOS() macro makes it possible to create test
cases that verify the behavior of libvirt on a macOS machine
with HVF support available.

Signed-off-by: Andrea Bolognani 
---
 tests/qemuxml2argvtest.c | 25 -
 tests/qemuxml2xmltest.c  | 27 ++-
 tests/testutilsqemu.c|  4 
 tests/testutilsqemu.h|  2 ++
 4 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index cc67d806e4..6bb330e27e 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -39,6 +39,8 @@
 # define VIR_FROM_THIS VIR_FROM_QEMU
 
 static virQEMUDriver driver;
+static virCaps *linuxCaps;
+static virCaps *macOSCaps;
 
 static unsigned char *
 fakeSecretGetValue(virSecretPtr obj G_GNUC_UNUSED,
@@ -716,12 +718,18 @@ testCompareXMLToArgv(const void *data)
 g_autofree char *archstr = NULL;
 virArch arch = VIR_ARCH_NONE;
 g_autoptr(virIdentity) sysident = virIdentityGetSystem();
+int rc;
 
 memset(_chr, 0, sizeof(monitor_chr));
 
 if (testQemuInfoInitArgs((struct testQemuInfo *) info) < 0)
 goto cleanup;
 
+if (info->args.hostOS == HOST_OS_MACOS)
+driver.caps = macOSCaps;
+else
+driver.caps = linuxCaps;
+
 if (info->arch != VIR_ARCH_NONE && info->arch != VIR_ARCH_X86_64)
 qemuTestSetHostArch(, info->arch);
 
@@ -771,7 +779,11 @@ testCompareXMLToArgv(const void *data)
 goto cleanup;
 }
 
-if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps) < 0)
+if (info->args.hostOS == HOST_OS_MACOS)
+rc = qemuTestCapsCacheInsertMacOS(driver.qemuCapsCache, 
info->qemuCaps);
+else
+rc = qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps);
+if (rc < 0)
 goto cleanup;
 
 if (info->migrateFrom &&
@@ -934,6 +946,13 @@ mymain(void)
 if (qemuTestDriverInit() < 0)
 return EXIT_FAILURE;
 
+/* By default, the driver gets a virCaps instance that's suitable for
+ * tests that expect Linux as the host OS. We create another one for
+ * macOS, and keep around pointers to both so that we can pick the
+ * best one for each test case */
+linuxCaps = driver.caps;
+macOSCaps = testQemuCapsInitMacOS();
+
 driver.privileged = true;
 
 VIR_FREE(driver.config->defaultTLSx509certdir);
@@ -1074,6 +1093,10 @@ mymain(void)
 DO_TEST_FULL(name, "", \
  ARG_GIC, gic, \
  ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST, ARG_END)
+# define DO_TEST_MACOS(name, ...) \
+DO_TEST_FULL(name, "", \
+ ARG_HOST_OS, HOST_OS_MACOS, \
+ ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST, ARG_END)
 
 # define DO_TEST_FAILURE(name, ...) \
 DO_TEST_FULL(name, "", \
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index fb438269b9..90047197b4 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -21,6 +21,8 @@
 #define VIR_FROM_THIS VIR_FROM_NONE
 
 static virQEMUDriver driver;
+static virCaps *linuxCaps;
+static virCaps *macOSCaps;
 
 enum {
 WHEN_INACTIVE = 1,
@@ -32,13 +34,24 @@ enum {
 static int
 testXML2XMLCommon(const struct testQemuInfo *info)
 {
+int rc;
+
 if (testQemuInfoInitArgs((struct testQemuInfo *) info) < 0)
 return -1;
 
+if (info->args.hostOS == HOST_OS_MACOS)
+driver.caps = macOSCaps;
+else
+driver.caps = linuxCaps;
+
 if (!(info->flags & FLAG_REAL_CAPS))
 virQEMUCapsInitQMPBasicArch(info->qemuCaps);
 
-if (qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps) < 0)
+if (info->args.hostOS == HOST_OS_MACOS)
+rc = qemuTestCapsCacheInsertMacOS(driver.qemuCapsCache, 
info->qemuCaps);
+else
+rc = qemuTestCapsCacheInsert(driver.qemuCapsCache, info->qemuCaps);
+if (rc < 0)
 return -1;
 
 return 0;
@@ -143,6 +156,13 @@ mymain(void)
 if (qemuTestDriverInit() < 0)
 return EXIT_FAILURE;
 
+/* By default, the driver gets a virCaps instance that's suitable for
+ * tests that expect Linux as the host OS. We create another one for
+ * macOS, and keep around pointers to both so that we can pick the
+ * best one for each test case */
+linuxCaps = driver.caps;
+macOSCaps = testQemuCapsInitMacOS();
+
 cfg = virQEMUDriverGetConfig();
 driver.privileged = true;
 
@@ -206,6 +226,11 @@ mymain(void)
 #define DO_TEST_NOCAPS(name) \
 DO_TEST_FULL(name, "", WHEN_BOTH, ARG_END)
 
+#define DO_TEST_MACOS(name, ...) \
+DO_TEST_FULL(name, "", WHEN_BOTH, \
+ ARG_HOST_OS, HOST_OS_MACOS, \
+ ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST, ARG_END)
+
 /* Unset or set all envvars here that are copied in qemudBuildCommandLine
  * using ADD_ENV_COPY, otherwise these tests may fail due to unexpected
  * values for these envvars */
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 

[libvirt PATCH v4 12/19] tests: Add macOS support to testutilsqemu

[Andrea Bolognani]

This exposes a couple of macOS-specific variants of existing
APIs, which can be used when implementing test programs and
result in HVF support being advertised.

Signed-off-by: Andrea Bolognani 
---
 tests/testutilsqemu.c | 58 +++
 tests/testutilsqemu.h |  4 +++
 2 files changed, 62 insertions(+)

diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index b9835782da..64c57b0073 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -97,6 +97,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",
@@ -208,6 +220,20 @@ testQemuAddGuest(virCaps *caps,
 }
 }
 
+if (hostOS == HOST_OS_MACOS) {
+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:
@@ -265,6 +291,12 @@ testQemuCapsInit(void)
 return testQemuCapsInitImpl(HOST_OS_LINUX);
 }
 
+virCaps*
+testQemuCapsInitMacOS(void)
+{
+return testQemuCapsInitImpl(HOST_OS_MACOS);
+}
+
 
 void
 qemuTestSetHostArch(virQEMUDriver *driver,
@@ -394,6 +426,25 @@ qemuTestCapsPopulateFakeMachines(virQEMUCaps *caps,
 }
 }
 }
+
+if (hostOS == HOST_OS_MACOS) {
+if (hvf_machines[arch] != NULL) {
+for (i = 0; hvf_machines[arch][i] != NULL; i++) {
+virQEMUCapsAddMachine(caps,
+VIR_DOMAIN_VIRT_HVF,
+hvf_machines[arch][i],
+NULL,
+NULL,
+0,
+false,
+false,
+true,
+defaultRAMid,
+false);
+virQEMUCapsSet(caps, QEMU_CAPS_HVF);
+}
+}
+}
 }
 
 
@@ -491,6 +542,13 @@ qemuTestCapsCacheInsert(virFileCache *cache,
 return qemuTestCapsCacheInsertImpl(cache, caps, HOST_OS_LINUX);
 }
 
+int
+qemuTestCapsCacheInsertMacOS(virFileCache *cache,
+ virQEMUCaps *caps)
+{
+return qemuTestCapsCacheInsertImpl(cache, caps, HOST_OS_MACOS);
+}
+
 
 # define STATEDIRTEMPLATE abs_builddir "/qemustatedir-XX"
 # define CONFIGDIRTEMPLATE abs_builddir "/qemuconfigdir-XX"
diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h
index a8de6eb52b..a9202d2ae6 100644
--- a/tests/testutilsqemu.h
+++ b/tests/testutilsqemu.h
@@ -35,6 +35,7 @@ enum {
 
 typedef enum {
 HOST_OS_LINUX = 0,
+HOST_OS_MACOS,
 } testQemuHostOS;
 
 typedef enum {
@@ -92,6 +93,7 @@ struct testQemuInfo {
 };
 
 virCaps *testQemuCapsInit(void);
+virCaps *testQemuCapsInitMacOS(void);
 virDomainXMLOption *testQemuXMLConfInit(void);
 
 
@@ -113,6 +115,8 @@ int qemuTestDriverInit(virQEMUDriver *driver);
 void qemuTestDriverFree(virQEMUDriver *driver);
 int qemuTestCapsCacheInsert(virFileCache *cache,
 virQEMUCaps *caps);
+int qemuTestCapsCacheInsertMacOS(virFileCache *cache,
+ virQEMUCaps *caps);
 
 int testQemuCapsSetGIC(virQEMUCaps *qemuCaps,
int gic);
-- 
2.31.1

[libvirt PATCH v4 10/19] qemu: Correct CPU capabilities probing for hvf

[Andrea Bolognani]

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 | 32 +---
 src/qemu/qemu_process.c  |  2 ++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 8d0a700544..dac8af38e6 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -744,6 +744,7 @@ struct _virQEMUCaps {
 
 /* Capabilities which may differ depending on the accelerator. */
 virQEMUCapsAccel kvm;
+virQEMUCapsAccel hvf;
 virQEMUCapsAccel tcg;
 };
 
@@ -805,14 +806,16 @@ virQEMUCapsAccelStr(virDomainVirtType type)
 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);
 }
 
 
@@ -868,6 +871,8 @@ virQEMUCapsGetAccel(virQEMUCaps *qemuCaps,
 {
 if (type == VIR_DOMAIN_VIRT_KVM)
 return >kvm;
+else if (type == VIR_DOMAIN_VIRT_HVF)
+return >hvf;
 
 return >tcg;
 }
@@ -998,6 +1003,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;
 
@@ -2016,6 +2023,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;
 
@@ -2069,6 +2077,7 @@ void virQEMUCapsDispose(void *obj)
 virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
 
 virQEMUCapsAccelClear(>kvm);
+virQEMUCapsAccelClear(>hvf);
 virQEMUCapsAccelClear(>tcg);
 }
 
@@ -2320,6 +2329,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;
@@ -2797,7 +2810,9 @@ bool
 virQEMUCapsHasMachines(virQEMUCaps *qemuCaps)
 {
 
-return !!qemuCaps->kvm.nmachineTypes || !!qemuCaps->tcg.nmachineTypes;
+return !!qemuCaps->kvm.nmachineTypes ||
+   !!qemuCaps->hvf.nmachineTypes ||
+   !!qemuCaps->tcg.nmachineTypes;
 }
 
 
@@ -4486,6 +4501,10 @@ virQEMUCapsLoadCache(virArch hostArch,
 virQEMUCapsLoadAccel(qemuCaps, ctxt, VIR_DOMAIN_VIRT_KVM) < 0) {
 return -1;
 }
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF) &&
+virQEMUCapsLoadAccel(qemuCaps, ctxt, VIR_DOMAIN_VIRT_HVF) < 0) {
+return -1;
+}
 if (virQEMUCapsLoadAccel(qemuCaps, ctxt, VIR_DOMAIN_VIRT_QEMU) < 0)
 return -1;
 
@@ -4497,6 +4516,8 @@ virQEMUCapsLoadCache(virArch hostArch,
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF))
+virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_HVF);
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
 
 if (virXPathBoolean("boolean(./kvmSupportsNesting)", ctxt) > 0)
@@ -4731,6 +4752,8 @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps)
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
 virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_KVM);
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF))
+virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_HVF);
 virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_QEMU);
 
 for (i = 0; i < qemuCaps->ngicCapabilities; i++) {
@@ -5591,6 +5614,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF))
+virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_HVF);
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
 
 if (virQEMUCapsHaveAccel(qemuCaps)) {
@@ -6585,5 +6610,6 @@ void
 virQEMUCapsStripMachineAliases(virQEMUCaps *qemuCaps)
 {
 virQEMUCapsStripMachineAliasesForVirtType(qemuCaps, VIR_DOMAIN_VIRT_KVM);
+virQEMUCapsStripMachineAliasesForVirtType(qemuCaps, VIR_DOMAIN_VIRT_HVF);
 

[libvirt PATCH v4 09/19] qemu: Introduce virQEMUCapsHaveAccel

[Andrea Bolognani]

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 
Reviewed-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c | 17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 8e127c9041..8d0a700544 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -809,6 +809,13 @@ virQEMUCapsTypeIsAccelerated(virDomainVirtType type)
 }
 
 
+static bool
+virQEMUCapsHaveAccel(virQEMUCaps *qemuCaps)
+{
+return virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM);
+}
+
+
 /* Checks whether a domain with @guest arch can run natively on @host.
  */
 bool
@@ -5021,7 +5028,7 @@ virQEMUCapsIsValid(void *data,
 return false;
 }
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
+if (virQEMUCapsHaveAccel(qemuCaps)) {
 if (STRNEQ_NULLABLE(priv->hostCPUSignature, 
qemuCaps->hostCPUSignature)) {
 VIR_DEBUG("Outdated capabilities for '%s': host CPU changed "
   "('%s' vs '%s')",
@@ -5055,7 +5062,9 @@ virQEMUCapsIsValid(void *data,
   qemuCaps->binary);
 return false;
 }
+}
 
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
 kvmSupportsNesting = virQEMUCapsKVMSupportsNesting();
 if (kvmSupportsNesting != qemuCaps->kvmSupportsNesting) {
 VIR_DEBUG("Outdated capabilities for '%s': kvm kernel nested "
@@ -5520,7 +5529,7 @@ virQEMUCapsInitQMP(virQEMUCaps *qemuCaps,
  * for TCG capabilities by asking the same binary again and turning KVM
  * off.
  */
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
+if (virQEMUCapsHaveAccel(qemuCaps) &&
 virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG) &&
 virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, true) < 0)
 return -1;
@@ -5584,13 +5593,15 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
+if (virQEMUCapsHaveAccel(qemuCaps)) {
 qemuCaps->hostCPUSignature = g_strdup(hostCPUSignature);
 qemuCaps->microcodeVersion = microcodeVersion;
 qemuCaps->cpuData = virCPUDataNewCopy(cpuData);
 
 qemuCaps->kernelVersion = g_strdup(kernelVersion);
+}
 
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
 qemuCaps->kvmSupportsNesting = virQEMUCapsKVMSupportsNesting();
 
 qemuCaps->kvmSupportsSecureGuest = virQEMUCapsKVMSupportsSecureGuest();
-- 
2.31.1

[libvirt PATCH v4 08/19] qemu: Introduce virQEMUCapsTypeIsAccelerated

[Andrea Bolognani]

From: Roman Bolshakov 

It replaces hardcoded checks for KVM. It'll be cleaner to use
the function once multiple accelerators are supported in the
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 | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c246e1b6ce..8e127c9041 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -802,6 +802,13 @@ virQEMUCapsAccelStr(virDomainVirtType type)
 }
 
 
+static bool
+virQEMUCapsTypeIsAccelerated(virDomainVirtType type)
+{
+return type == VIR_DOMAIN_VIRT_KVM;
+}
+
+
 /* Checks whether a domain with @guest arch can run natively on @host.
  */
 bool
@@ -2342,7 +2349,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:
@@ -3004,7 +3011,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;
@@ -3715,7 +3722,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;
@@ -5848,10 +5855,10 @@ 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);
+   _("the accel '%s' is not supported by '%s' on this 
host"),
+   virQEMUCapsAccelStr(virttype), binary);
 return NULL;
 }
 
-- 
2.31.1

[libvirt PATCH v4 06/19] qemu: Expose hvf domain type if hvf is supported

[Andrea Bolognani]

From: Roman Bolshakov 

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
Reviewed-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 56325d1762..081c43e728 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1084,6 +1084,10 @@ virQEMUCapsInitGuestFromBinary(virCaps *caps,
 virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM,
   NULL, NULL, 0, NULL);
 }
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF)) {
+virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HVF,
+  NULL, NULL, 0, NULL);
+}
 
 if ((ARCH_IS_X86(guestarch) || guestarch == VIR_ARCH_AARCH64))
 virCapabilitiesAddGuestFeatureWithToggle(guest, 
VIR_CAPS_GUEST_FEATURE_TYPE_ACPI,
-- 
2.31.1

[libvirt PATCH v4 07/19] qemu: Introduce virQEMUCapsAccelStr

[Andrea Bolognani]

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 081c43e728..c246e1b6ce 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -792,6 +792,16 @@ const char *virQEMUCapsArchToString(virArch arch)
 }
 
 
+static const char *
+virQEMUCapsAccelStr(virDomainVirtType type)
+{
+if (type == VIR_DOMAIN_VIRT_QEMU)
+return "tcg";
+
+return virDomainVirtTypeToString(type);
+}
+
+
 /* Checks whether a domain with @guest arch can run natively on @host.
  */
 bool
@@ -4069,7 +4079,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;
@@ -4622,7 +4632,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

[libvirt PATCH v4 05/19] qemu: Query hvf capability on macOS

[Andrea Bolognani]

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 
Reviewed-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 6b7e295f68..56325d1762 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -56,6 +56,10 @@
 #include 
 #include 
 #include 
+#ifdef __APPLE__
+# include 
+# include 
+#endif
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
@@ -3195,6 +3199,33 @@ virQEMUCapsProbeQMPKVMState(virQEMUCaps *qemuCaps,
 return 0;
 }
 
+#ifdef __APPLE__
+static int
+virQEMUCapsProbeHVF(virQEMUCaps *qemuCaps)
+{
+int hv_support;
+size_t len = sizeof(hv_support);
+
+if (sysctlbyname("kern.hv_support", _support, , NULL, 0) < 0)
+hv_support = 0;
+
+if (qemuCaps->version >= 2012000 &&
+(qemuCaps->arch == VIR_ARCH_X86_64 ||
+ qemuCaps->arch == VIR_ARCH_AARCH64) &&
+hv_support) {
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_HVF);
+}
+
+return 0;
+}
+#else
+static int
+virQEMUCapsProbeHVF(virQEMUCaps *qemuCaps G_GNUC_UNUSED)
+{
+  return 0;
+}
+#endif
+
 struct virQEMUCapsCommandLineProps {
 const char *option;
 const char *param;
@@ -5344,6 +5375,9 @@ virQEMUCapsInitQMPMonitor(virQEMUCaps *qemuCaps,
 if (virQEMUCapsProbeQMPKVMState(qemuCaps, mon) < 0)
 return -1;
 
+if (virQEMUCapsProbeHVF(qemuCaps) < 0)
+return -1;
+
 type = virQEMUCapsGetVirtType(qemuCaps);
 accel = virQEMUCapsGetAccel(qemuCaps, type);
 
-- 
2.31.1

[libvirt PATCH v4 04/19] qemu: Define hvf capability

[Andrea Bolognani]

From: Roman Bolshakov 

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
Reviewed-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c | 3 +++
 src/qemu/qemu_capabilities.h | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index fd001100a1..6b7e295f68 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -654,6 +654,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "rbd-encryption", /* QEMU_CAPS_RBD_ENCRYPTION */
   "sev-guest-kernel-hashes", /* QEMU_CAPS_SEV_GUEST_KERNEL_HASHES 
*/
   "sev-inject-launch-secret", /* 
QEMU_CAPS_SEV_INJECT_LAUNCH_SECRET */
+
+  /* 420 */
+  "hvf", /* QEMU_CAPS_HVF */
 );
 
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 63ac24314f..4a9a40a8b9 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -634,6 +634,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_SEV_GUEST_KERNEL_HASHES, /* sev-guest.kernel-hashes= */
 QEMU_CAPS_SEV_INJECT_LAUNCH_SECRET, /* 'sev-inject-launch-secret' qmp 
command present */
 
+/* 420 */
+QEMU_CAPS_HVF, /* Whether Hypervisor.framework is available */
+
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
 
-- 
2.31.1

[libvirt PATCH v4 01/19] qemu: Only probe KVM on Linux

[Andrea Bolognani]

We already know it's not going to be available on other
platforms.

Suggested-by: Daniel P. Berrangé 
Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_process.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 5c9ca0fe4f..c2c10d282a 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -9269,6 +9269,12 @@ qemuProcessQMPInit(qemuProcessQMP *proc)
 }
 
 
+#if defined(__linux__)
+# define hwaccel "kvm:tcg"
+#else
+# define hwaccel "tcg"
+#endif
+
 static int
 qemuProcessQMPLaunch(qemuProcessQMP *proc)
 {
@@ -9279,7 +9285,7 @@ qemuProcessQMPLaunch(qemuProcessQMP *proc)
 if (proc->forceTCG)
 machine = "none,accel=tcg";
 else
-machine = "none,accel=kvm:tcg";
+machine = "none,accel=" hwaccel;
 
 VIR_DEBUG("Try to probe capabilities of '%s' via QMP, machine %s",
   proc->binary, machine);
-- 
2.31.1

[libvirt PATCH v4 02/19] qemu: Add KVM CPUs into cache only if KVM is present

[Andrea Bolognani]

From: Roman Bolshakov 

virQEMUCapsFormatCache/virQEMUCapsLoadCache adds/reads KVM CPUs to/from
capabilities cache regardless of QEMU_CAPS_KVM. That can cause undesired
side-effects when KVM CPUs are present in the cache on a platform that
doesn't support it, e.g. macOS or Linux without KVM support.

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2d72132410..fd001100a1 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4420,8 +4420,11 @@ virQEMUCapsLoadCache(virArch hostArch,
 return -1;
 }
 
-if (virQEMUCapsLoadAccel(qemuCaps, ctxt, VIR_DOMAIN_VIRT_KVM) < 0 ||
-virQEMUCapsLoadAccel(qemuCaps, ctxt, VIR_DOMAIN_VIRT_QEMU) < 0)
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
+virQEMUCapsLoadAccel(qemuCaps, ctxt, VIR_DOMAIN_VIRT_KVM) < 0) {
+return -1;
+}
+if (virQEMUCapsLoadAccel(qemuCaps, ctxt, VIR_DOMAIN_VIRT_QEMU) < 0)
 return -1;
 
 if (virQEMUCapsParseGIC(qemuCaps, ctxt) < 0)
@@ -4430,7 +4433,8 @@ virQEMUCapsLoadCache(virArch hostArch,
 if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0)
 return -1;
 
-virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
+virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
 
 if (virXPathBoolean("boolean(./kvmSupportsNesting)", ctxt) > 0)
@@ -4663,7 +4667,8 @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps)
 virBufferAsprintf(, "%s\n",
   virArchToString(qemuCaps->arch));
 
-virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_KVM);
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
+virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_KVM);
 virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_QEMU);
 
 for (i = 0; i < qemuCaps->ngicCapabilities; i++) {
@@ -5517,7 +5522,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
 qemuCaps->libvirtCtime = virGetSelfLastChanged();
 qemuCaps->libvirtVersion = LIBVIR_VERSION_NUMBER;
 
-virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
+virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
-- 
2.31.1

[libvirt PATCH v4 03/19] conf: Add hvf domain type

[Andrea Bolognani]

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 
Reviewed-by: Daniel P. Berrangé 
---
 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(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index d4f30bb8af..3e9de05249 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -20,7 +20,8 @@ Element and attribute overview
 
 The root element required for all virtual machines is named ``domain``. It has
 two attributes, the ``type`` specifies the hypervisor used for running the
-domain. The allowed values are driver specific, but include "xen", "kvm", 
"qemu"
+domain. The allowed values are driver specific, but include "xen", "kvm",
+"hvf" (:since:`since 8.0.0 and QEMU 2.12`), "qemu"
 and "lxc". The second attribute is ``id`` which is a unique integer identifier
 for the running guest machine. Inactive machines have no id value.
 
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 7fa5c2b8b5..bf9c12397f 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -230,6 +230,7 @@
 phyp 
 vz
 bhyve
+hvf
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5691b8d2d5..0faecf2bb4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -125,6 +125,7 @@ VIR_ENUM_IMPL(virDomainVirt,
   "parallels",
   "bhyve",
   "vz",
+  "hvf",
 );
 
 VIR_ENUM_IMPL(virDomainOS,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 144ba4dd12..d4d8aa7e23 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -139,6 +139,7 @@ typedef enum {
 VIR_DOMAIN_VIRT_PARALLELS,
 VIR_DOMAIN_VIRT_BHYVE,
 VIR_DOMAIN_VIRT_VZ,
+VIR_DOMAIN_VIRT_HVF,
 
 VIR_DOMAIN_VIRT_LAST
 } virDomainVirtType;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d822533ccb..3bacf3edf2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7229,6 +7229,10 @@ qemuBuildAccelCommandLine(virCommand *cmd,
 }
 break;
 
+case VIR_DOMAIN_VIRT_HVF:
+virBufferAddLit(, "hvf");
+break;
+
 case VIR_DOMAIN_VIRT_KQEMU:
 case VIR_DOMAIN_VIRT_XEN:
 case VIR_DOMAIN_VIRT_LXC:
-- 
2.31.1

[libvirt PATCH v4 00/19] qemu: Introduce hvf domain type for Hypervisor.framework

[Andrea Bolognani]

In order to hopefully address [libvirt#147] at long last, I've picked
up Roman's patches from 2018 and attempted to forward-port them.

More specifically, I've used the [roolebo/hvf-domain] branch as a
starting point, since it seems to contain a few improvements over
[v2] and was just easier to pick up.

The code is mostly his own, so I've retained the existing authorship
information, but I've dropped Reviewed-by tags for commits that have
been modified in non-trivial ways. I've applied very minimal style
tweaks along the way, but overall I've tried to modify the existing
patches as little as possible.

I've added a few changes of my own, which I've marked as "fixup!"
when I felt that they should be squashed into the previous patch
rather than existing as separate commits.

The new test cases, such as they are, pass, and no regressions to KVM
support appear to have been introduced in the process. I don't
currently have access to a machine running macOS, so I can't verify
that it's actually possible to start a hardware-accelerated VM.

Changes from [v3]:

  * reintroduced the patch that was missing in the initial version
of the forward-port;
  * converted the documentation to reStructuredText and trimmed it
significantly;
  * reworked virQEMUCapsAccelStr() based on Dan's suggestions;
  * reworked macOS support in the test suite based on Dan's
suggestions;
  * fixed a few minor issues found while doing the above.

Changes from [v2]:

  * rebased on top of master;
  * added a couple of simple test cases.

Useful links:

  * GitLab: [abologna/hvf]
  * CI: [pipeline]

[libvirt#147] https://gitlab.com/libvirt/libvirt/-/issues/147
[roolebo/hvf-domain] https://github.com/roolebo/libvirt/tree/hvf-domain
[abologna/hvf] https://gitlab.com/abologna/libvirt/-/commits/hvf
[pipeline] https://gitlab.com/abologna/libvirt/-/pipelines/443320533
[v3] https://listman.redhat.com/archives/libvir-list/2022-January/msg00131.html
[v2] https://listman.redhat.com/archives/libvir-list/2018-November/msg00802.html

Andrea Bolognani (6):
  qemu: Only probe KVM on Linux
  tests: Introduce testQemuHostOS
  tests: Add macOS support to testutilsqemu
  tests: Add macOS support to qemuxml2*test
  tests: Add HVF test cases
  fixup! NEWS: Mention Apple Silicon support for HVF

Roman Bolshakov (13):
  qemu: Add KVM CPUs into cache only if KVM is present
  conf: Add hvf domain type
  qemu: Define hvf capability
  qemu: Query hvf capability on macOS
  qemu: Expose hvf domain type if hvf is supported
  qemu: Introduce virQEMUCapsAccelStr
  qemu: Introduce virQEMUCapsTypeIsAccelerated
  qemu: Introduce virQEMUCapsHaveAccel
  qemu: Correct CPU capabilities probing for hvf
  docs: Add hvf on QEMU driver page
  docs: Note hvf support for domain elements
  docs: Add support page for libvirt on macOS
  news: Mention hvf domain type

 NEWS.rst  |   6 +
 docs/docs.html.in |   3 +
 docs/drvqemu.rst  |  48 +-
 docs/formatdomain.rst |  22 +--
 docs/index.html.in|   4 +-
 docs/macos.rst|  44 ++
 docs/meson.build  |   1 +
 docs/schemas/domaincommon.rng |   1 +
 src/conf/domain_conf.c|   1 +
 src/conf/domain_conf.h|   1 +
 src/qemu/qemu_capabilities.c  | 135 ++--
 src/qemu/qemu_capabilities.h  |   3 +
 src/qemu/qemu_command.c   |   4 +
 src/qemu/qemu_process.c   |  10 +-
 .../hvf-aarch64-virt-headless.args|  48 ++
 .../hvf-aarch64-virt-headless.xml |  45 ++
 .../hvf-x86_64-q35-headless.args  |  47 ++
 .../hvf-x86_64-q35-headless.x86_64-latest.err |   1 +
 .../hvf-x86_64-q35-headless.xml   |  44 ++
 tests/qemuxml2argvtest.c  |  43 -
 .../hvf-aarch64-virt-headless.xml |  94 +++
 .../hvf-x86_64-q35-headless.xml   |  97 
 tests/qemuxml2xmltest.c   |  43 -
 tests/testutilsqemu.c | 147 ++
 tests/testutilsqemu.h |  10 ++
 25 files changed, 837 insertions(+), 65 deletions(-)
 create mode 100644 docs/macos.rst
 create mode 100644 tests/qemuxml2argvdata/hvf-aarch64-virt-headless.args
 create mode 100644 tests/qemuxml2argvdata/hvf-aarch64-virt-headless.xml
 create mode 100644 tests/qemuxml2argvdata/hvf-x86_64-q35-headless.args
 create mode 100644 
tests/qemuxml2argvdata/hvf-x86_64-q35-headless.x86_64-latest.err
 create mode 100644 tests/qemuxml2argvdata/hvf-x86_64-q35-headless.xml
 create mode 100644 tests/qemuxml2xmloutdata/hvf-aarch64-virt-headless.xml
 create mode 100644 tests/qemuxml2xmloutdata/hvf-x86_64-q35-headless.xml

-- 
2.31.1

Re: .conf file setting(s) for packet filtering backend(s)

[Daniel P . Berrangé]

On Thu, Jan 06, 2022 at 02:24:18PM -0500, Laine Stump wrote:
> On 1/4/22 7:57 AM, Daniel P. Berrangé wrote:
> > On Sun, Jan 02, 2022 at 09:41:37PM -0500, Laine Stump wrote:
> > > I'm currently working on switching the backend of the network driver from
> > > using iptables to using nftables. Due to some functionality that is not
> > > available with nftables (the rule that fixes up the checksum of DHCP 
> > > packets
> > > which, btw, is only relevant for *very* old guests, e.g. RHEL5), this 
> > > needs
> > > to be opt-in via a config file setting. In the meantime, in order to make
> > > this doable in a reasonable amount of time, I am *not* converting the
> > > nwfilter driver right away, and when I do it will need its own config file
> > > setting for opt-in.
> > > 
> > > I've never before looked at the code for the .conf file settings at all. I
> > > had assumed there would be some sort of "pull" API, where code in the
> > > drivers could call, e.g. virConfGetString("filter_backend") and it would
> > > return the config setting to the caller. But when I look at it, I see that
> > > all daemons use the same daemonConfigLoadFile() called from
> > > remote_daemon.c:main() (which is shared by all the daemons) and the
> > > daemonConfig object that is created to hold the config settings that are
> > > read is only visible within main() - the only way that a config setting is
> > > used is by main() "pushing" it out to a static variable somewhere else 
> > > where
> > > it is later retrieved by the interested party, e.g. the way that main()
> > > calls daemonSetupNetDevOpenvswitch(config), which then sets the static
> > > virNetDevOpenvswitchTimeout in util/virnetdevopenvswitch.c.
> > 
> > I'd consider the OVS approach to be a bad example
> > 
> > Global state needing configurable parameters for stuff in util/ should
> > generally be considered a design flaw.
> 
> Okay, so then the setting of the host uuid is also a bad example (set into a
> static in util/viruuid.c), as is all the config for logging (set in statics
> in util/virlog.c by calling a function in util/virdaemon.c) :-)

Ok true, there are reasonable exceptions like logging / uuid.

> > ie  ovs_timeout should have been in qemu.conf (any other drivers' config
> > files if appropriate).
> 
> Somehow I had always considered qemu.conf to be specifically for things
> related to starting the qemu process *only* (and not necessarily pertaining
> to the entire qemu driver), although even with that interpretation I guess
> ovs_timeout could be considered to be in that group as well (since it's used
> when running ovs-vsctl as part of preparing the network connection for a
> qemu process that will soon be started). I see now I've just been too narrow
> minded all this time.

I think I'd describe 'libvirtd.conf' as being for settings related to
operation of the daemon / RPC system, while 'qemu.conf' is for settings
related to operation of the QEMU driver or any features it uses.

With this view point logging / host uuid is appropriate for libvirtd.conf
but OVS timeouts, firewall settings are driver conf things


> > This stuff even gets into the libvirt.so that's used client side,
> > so the argument that we had a single monolithic libvirtd didn't
> > apply either.
> 
> Really? I have always just assumed that if nothing in a particular .o was
> referenced, then that .o wouldn't show up in the binary. And even if that
> isn't the case, then we could tailor the build to only include those sources
> that are actually used (although that would be cumbersome to maintain).

What you describe is true if your linking to a static library.

The src/util stuff does indeed get into a static libvirt_util.a library,
but this is then built into a dynamic library libvirt.so and all the
APIs in src/util are exported (with @LIBVIRT_PRIVATE version tag).
As a result no .o files in src/util will ever get dropped.

> > > If I could count on all builds using split daemons (i.e. separate
> > > virtnetworkd and virtnwfilterd) then I could add a similar API in
> > > virfirewall.c that remote_daemon.c:main() could use to set 
> > > "filter_backend"
> > > into a static in virfirewall.c (which is used by both drivers) and
> > > everything would just happily work:
> > > 
> > > virtnetworkd.conf:
> > >filter_backend = nftables
> > > 
> > > virtnwfilterd.conf
> > >filter_backend = iptables
> > 
> > Putting these settings into virtnetworkd.conf and virtnwfilterd.conf
> > certainly makes conceptual sense.
> 
> Or maybe, based on what I say about "virtqemud.conf vs. qemu.conf" (and thus
> "virtnetworkd.conf vs. network.conf") above, they should be put in
> networkd.conf and nwfilter.conf. (again, I'm loathe to create "yet another"
> config file, but that seems the most logical thing to do).

Sorry, yes, I mis-typed. virtnetworkd.conf / virtnwfilterd.conf are
the direct equivalent of libvirtd.conf so I shouldn't have written
that. I meand  network.conf / nwfilter.conf 

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Ján Tomko]

On a Friday in 2022, Ani Sinha wrote:

On Fri, 7 Jan 2022, Michal Prívozník wrote:

I don't think so. Just like we've discussed under one patch of yours, a
function should either report error in all cases or none. And in case of
virProcessGetSchedInfo() the linux version does report error


I see your point but there is also a bug in that function - not all error
paths report errors. For example, !proc and !lines cases. We need to fix
that.



I don't see a !proc error path in virProcessGetSchedInfo.

The !lines case is inconsistent but thankfully it can only happen
if /proc/%d/sched is empty.


hence the

non-linux variant should report an error too. And in case of
virProcessGetStatInfo() no error is reported for linux version thus
non-linux version shouldn't report an error.



No this is not my understanding from the discussion. What I understood is
that the lowest level of functions should always report error when an
error path is encountered.


Errors should be reported by the function that has the context to
provide a helpful error message. For some low-level helpers, we rely
on errno's and let the caller report a less-specific error - either
out of laziness because (like above) there would have to be a buggy
kernel, or because most of the code paths are syscalls which set errno.

Jano


For example virFileReadAll() does this nicely.
Currently there is no  error path in virProcessGetStatInfo() and it
uncondiotionally returns 0. For non-linux variant, I think it would be
correct to report an error.

Now having done that, we should also fix the callers so that the callers
are not overriding the narrower errors with broader ones.


signature.asc
Description: PGP signature

[PATCH] report error when virProcessGetStatInfo() is unable to parse data

[Ani Sinha]

Currently virProcessGetStatInfo() always returns success and only logs error
when it is unable to parse the data. Make this function actually report the
error and return a negative value in this error scenario.

Signed-off-by: Ani Sinha 
---
 src/util/virprocess.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index c74bd16fe6..b9f498d5d8 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
 virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, 
) < 0 ||
 virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
 virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, ) 
< 0) {
-VIR_WARN("cannot parse process status data");
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("cannot parse process status data for pid '%d/%d'"),
+   (int) pid, (int) tid);
+
+return -1;
 }
 
 /* We got jiffies
-- 
2.25.1

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Ján Tomko]

On a Friday in 2022, Michal Privoznik wrote:

Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
Linux centric. Provide stubs for non-Linux platforms.

Signed-off-by: Michal Privoznik 
---
src/util/virprocess.c | 24 
1 file changed, 24 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] src: Don't check for retval of some glib functions

[Ján Tomko]

On a Friday in 2022, Michal Privoznik wrote:

There are a few glib functions that abort on OOM and thus there's
no point in checking their retval against NULL. Nevertheless, we
do have those checks in a few places. Remove them.

Generated using the following spatch:



[...]


Signed-off-by: Michal Privoznik 
---
src/hyperv/hyperv_driver.c | 14 --
src/util/virprocess.c  |  2 --
2 files changed, 16 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Peter Krempa]

On Fri, Jan 07, 2022 at 08:55:16 +0100, Michal Privoznik wrote:
> Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
> Linux centric. Provide stubs for non-Linux platforms.

Fixes: d73852c49962fbfe652fc7bec595a3f242faf847

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virprocess.c | 24 
>  1 file changed, 24 insertions(+)

IMO qualifies for build-breaker rule:

https://gitlab.com/libvirt/libvirt/-/jobs/1950755381


Reviewed-by: Peter Krempa 

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Ani Sinha]

On Fri, 7 Jan 2022, Michal Prívozník wrote:

> On 1/7/22 09:05, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Michal Privoznik wrote:
> >
> >> Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
> >> Linux centric. Provide stubs for non-Linux platforms.
> >>
> >> Signed-off-by: Michal Privoznik 
> >> ---
> >>  src/util/virprocess.c | 24 
> >>  1 file changed, 24 insertions(+)
> >>
> >> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> >> index c74bd16fe6..5788faea9c 100644
> >> --- a/src/util/virprocess.c
> >> +++ b/src/util/virprocess.c
> >> @@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
> >>  }
> >>
> >>
> >> +#ifdef __linux__
> >>  int
> >>  virProcessGetStatInfo(unsigned long long *cpuTime,
> >>int *lastCpu,
> >> @@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
> >>
> >>  return 0;
> >>  }
> >> +
> >> +#else
> >> +int
> >> +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
> >> +  int *lastCpu G_GNUC_UNUSED,
> >> +  long *vm_rss G_GNUC_UNUSED,
> >> +  pid_t pid G_GNUC_UNUSED,
> >> +  pid_t tid G_GNUC_UNUSED)
> >> +{
> >> +errno = ENOSYS;
> >> +return -1;
> >> +}
> >> +
> >> +int
> >> +virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
> >> +   pid_t pid G_GNUC_UNUSED,
> >> +   pid_t tid G_GNUC_UNUSED)
> >> +{
> >> +virReportSystemError(ENOSYS, "%s",
> >> + _("scheduler information is not supported on 
> >> this platform"));
> >
> > We should do this for both functions for non-linux platforms or not do it
> > at all. Like it should be consistent IMHO.
>
> I don't think so. Just like we've discussed under one patch of yours, a
> function should either report error in all cases or none. And in case of
> virProcessGetSchedInfo() the linux version does report error

I see your point but there is also a bug in that function - not all error
paths report errors. For example, !proc and !lines cases. We need to fix
that.

hence the
> non-linux variant should report an error too. And in case of
> virProcessGetStatInfo() no error is reported for linux version thus
> non-linux version shouldn't report an error.
>

No this is not my understanding from the discussion. What I understood is
that the lowest level of functions should always report error when an
error path is encountered. For example virFileReadAll() does this nicely.
Currently there is no  error path in virProcessGetStatInfo() and it
uncondiotionally returns 0. For non-linux variant, I think it would be
correct to report an error.

Now having done that, we should also fix the callers so that the callers
are not overriding the narrower errors with broader ones.

> Think of it like this. You have a function that's called from somewhere.
> However, the function has two (or more) implementations (e.g. one for
> Linux, the other for FreeBSD, another one for MinGW, and so on). And you
> call the function like this:
>
>   var = func();
>   if (!var) {
> ...
>
> now should there be an error reported inside the if()? If all
> implementations of the func() are consistent then there's a clear answer.
>
> Hope this sheds more light.
>
> Michal
>
>

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Michal Prívozník]

On 1/7/22 09:05, Ani Sinha wrote:
> 
> 
> On Fri, 7 Jan 2022, Michal Privoznik wrote:
> 
>> Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
>> Linux centric. Provide stubs for non-Linux platforms.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/util/virprocess.c | 24 
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
>> index c74bd16fe6..5788faea9c 100644
>> --- a/src/util/virprocess.c
>> +++ b/src/util/virprocess.c
>> @@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
>>  }
>>
>>
>> +#ifdef __linux__
>>  int
>>  virProcessGetStatInfo(unsigned long long *cpuTime,
>>int *lastCpu,
>> @@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
>>
>>  return 0;
>>  }
>> +
>> +#else
>> +int
>> +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
>> +  int *lastCpu G_GNUC_UNUSED,
>> +  long *vm_rss G_GNUC_UNUSED,
>> +  pid_t pid G_GNUC_UNUSED,
>> +  pid_t tid G_GNUC_UNUSED)
>> +{
>> +errno = ENOSYS;
>> +return -1;
>> +}
>> +
>> +int
>> +virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
>> +   pid_t pid G_GNUC_UNUSED,
>> +   pid_t tid G_GNUC_UNUSED)
>> +{
>> +virReportSystemError(ENOSYS, "%s",
>> + _("scheduler information is not supported on this 
>> platform"));
> 
> We should do this for both functions for non-linux platforms or not do it
> at all. Like it should be consistent IMHO.

I don't think so. Just like we've discussed under one patch of yours, a
function should either report error in all cases or none. And in case of
virProcessGetSchedInfo() the linux version does report error hence the
non-linux variant should report an error too. And in case of
virProcessGetStatInfo() no error is reported for linux version thus
non-linux version shouldn't report an error.

Think of it like this. You have a function that's called from somewhere.
However, the function has two (or more) implementations (e.g. one for
Linux, the other for FreeBSD, another one for MinGW, and so on). And you
call the function like this:

  var = func();
  if (!var) {
...

now should there be an error reported inside the if()? If all
implementations of the func() are consistent then there's a clear answer.

Hope this sheds more light.

Michal

Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info

[Ani Sinha]

On Fri, 7 Jan 2022, Michal Privoznik wrote:

> Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
> Linux centric. Provide stubs for non-Linux platforms.
>
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virprocess.c | 24 
>  1 file changed, 24 insertions(+)
>
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index c74bd16fe6..5788faea9c 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
>  }
>
>
> +#ifdef __linux__
>  int
>  virProcessGetStatInfo(unsigned long long *cpuTime,
>int *lastCpu,
> @@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
>
>  return 0;
>  }
> +
> +#else
> +int
> +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
> +  int *lastCpu G_GNUC_UNUSED,
> +  long *vm_rss G_GNUC_UNUSED,
> +  pid_t pid G_GNUC_UNUSED,
> +  pid_t tid G_GNUC_UNUSED)
> +{
> +errno = ENOSYS;
> +return -1;
> +}
> +
> +int
> +virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
> +   pid_t pid G_GNUC_UNUSED,
> +   pid_t tid G_GNUC_UNUSED)
> +{
> +virReportSystemError(ENOSYS, "%s",
> + _("scheduler information is not supported on this 
> platform"));

We should do this for both functions for non-linux platforms or not do it
at all. Like it should be consistent IMHO.

> +return -1;
> +}
> +#endif /* __linux__ */
> --
> 2.34.1
>
>

[PATCH] src: Don't check for retval of some glib functions

[Michal Privoznik]

There are a few glib functions that abort on OOM and thus there's
no point in checking their retval against NULL. Nevertheless, we
do have those checks in a few places. Remove them.

Generated using the following spatch:

  @@
  expression x;
  identifier n;
  expression r;
  @@
  (
x = g_strdup_printf(...);
  | x = g_strdup_vprintf(...);
  | x = g_strdup(...);
  | x = g_strndup(...);
  | x = g_new0(...);
  | x = g_realloc(...);
  )
... when != x
  - if(!x)
  (
  -   return r;
  |
  -   goto n;
  )

Signed-off-by: Michal Privoznik 
---
 src/hyperv/hyperv_driver.c | 14 --
 src/util/virprocess.c  |  2 --
 2 files changed, 16 deletions(-)

diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 3327f0132e..7b684e04ba 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -482,8 +482,6 @@ hypervDomainAddVirtualDiskParent(virDomainPtr domain,
 parent__PATH = g_strdup_printf("%s\\Root\\Virtualization\\V2:"

"Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"",
hostname, parentInstanceIDEscaped);
-if (!parent__PATH)
-return -1;
 
 if (hypervSetEmbeddedProperty(controllerResource, "Parent", parent__PATH) 
< 0)
 return -1;
@@ -529,9 +527,6 @@ hypervDomainAddVirtualHardDisk(virDomainPtr domain,
 
"Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"",
 hostname, vhdInstanceIdEscaped);
 
-if (!vhd__PATH)
-return -1;
-
 if (hypervSetEmbeddedProperty(volumeResource, "Parent", vhd__PATH) < 0)
 return -1;
 
@@ -660,8 +655,6 @@ hypervDomainAttachPhysicalDisk(virDomainPtr domain,
 controller__PATH = g_strdup_printf("%s\\Root\\Virtualization\\V2:"

"Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"",
hostname, controllerInstanceIdEscaped);
-if (!controller__PATH)
-return -1;
 
 if (hypervSetEmbeddedProperty(diskResource, "Parent", controller__PATH) < 
0)
 return -1;
@@ -711,8 +704,6 @@ hypervDomainAddOpticalDrive(virDomainPtr domain,
 parent__PATH = g_strdup_printf("%s\\Root\\Virtualization\\V2:"

"Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"",
hostname, parentInstanceIDEscaped);
-if (!parent__PATH)
-return -1;
 
 if (hypervSetEmbeddedProperty(driveResource, "Parent", parent__PATH) < 0)
 return -1;
@@ -756,8 +747,6 @@ hypervDomainAddOpticalDisk(virDomainPtr domain,
 vhd__PATH = g_strdup_printf("%s\\Root\\Virtualization\\V2:"
 
"Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"",
 hostname, vhdInstanceIdEscaped);
-if (!vhd__PATH)
-return -1;
 
 if (hypervSetEmbeddedProperty(volumeResource, "Parent", vhd__PATH) < 0)
 return -1;
@@ -829,9 +818,6 @@ hypervDomainAttachFloppy(virDomainPtr domain,
 
"Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"",
 hostname, vhdInstanceIdEscaped);
 
-if (!vfd__PATH)
-return -1;
-
 if (hypervSetEmbeddedProperty(volumeResource, "Parent", vfd__PATH) < 0)
 return -1;
 
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index c74bd16fe6..66b20b0a33 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1826,8 +1826,6 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
 proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int) tid);
 else
 proc = g_strdup_printf("/proc/%d/sched", (int) pid);
-if (!proc)
-return -1;
 
 /* The file is not guaranteed to exist (needs CONFIG_SCHED_DEBUG) */
 if (access(proc, R_OK) < 0) {
-- 
2.34.1