Re: [PATCH 4/6] migration: Add ram-only capability

2022-01-14 Thread Daniel P . Berrangé
On Fri, Jan 14, 2022 at 12:22:13PM +0100, Markus Armbruster wrote:
> Nikita Lapshin  writes:
> 
> > If this capability is enabled migration stream
> > will have RAM section only.
> >
> > Signed-off-by: Nikita Lapshin 
> 
> [...]
> 
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index d53956852c..626fc59d14 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -454,6 +454,8 @@
> >  #
> >  # @no-ram: If enabled, migration stream won't contain any ram in it. 
> > (since 7.0)
> >  #
> > +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0)
> > +#
> 
> What happens when I ask for 'no-ram': true, 'ram-only': true?

So IIUC

  no-ram=false, ram-only=false =>  RAM + vmstate
  no-ram=true, ram-only=false => vmstate
  no-ram=false, ram-only=true =>  RAM
  no-ram=true, ram-only=true => nothing to send ?

I find that the fact that one flag is a negative request and
the other flag is a positive request to be confusing.

If we must have two flags then could we at least use the same
style for both. ie either

  @no-ram
  @no-vmstate

Or

  @ram-only
  @vmstate-only

Since the code enforces these flags are mutually exclusive
though, it might point towards being handled by a enum

  { 'enum': 'MigrationStreamContent',
'data': ['both', 'ram', 'vmstate'] }

none of these approaches are especially future proof if we ever
need fine grained control over sending a sub-set of the non-RAM
vmstate. Not sure if that matters in the end.


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 :|




[PATCH 2/2] hw/i386: support loading OVMF using -bios too

2022-01-13 Thread Daniel P . Berrangé
Traditionally the OVMF firmware has been loaded using the pflash
mechanism. This is because it is usually provided as a pair of
files, one read-only containing the code and one writable to
provided persistence of non-volatile firmware variables.

The AMD SEV build of EDK2, however, is provided as a single
file that contains only the code. This is intended to be used
read-only and explicitly does not provide any ability for
persistance of non-volatile firmware variables. While it is
possible to configure this with the pflash mechanism, by only
providing one of the 2 pflash blobs, conceptually it is a
little strange to use pflash if there won't be any persistent
data.

A stateless OVMF build can be loaded with -bios, however, QEMU
does not currently initialize SEV in that scenario. This patch
introduces the call needed for SEV initialization of the
firmware.

Signed-off-by: Daniel P. Berrangé 
---
 hw/i386/x86.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index b84840a1bb..c79d84936f 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -45,6 +45,7 @@
 #include "target/i386/cpu.h"
 #include "hw/i386/topology.h"
 #include "hw/i386/fw_cfg.h"
+#include "hw/i386/pc.h"
 #include "hw/intc/i8259.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "target/i386/sev.h"
@@ -1157,6 +1158,10 @@ void x86_bios_rom_init(MachineState *ms, const char 
*default_firmware,
 memory_region_add_subregion(rom_memory,
 (uint32_t)(-bios_size),
 bios);
+
+pc_system_ovmf_initialize_sev(
+rom_ptr((uint32_t)-bios_size, bios_size),
+bios_size);
 }
 
 bool x86_machine_is_smm_enabled(const X86MachineState *x86ms)
-- 
2.33.1




[PATCH 1/2] hw/i386: refactor logic for setting up SEV firmware

2022-01-13 Thread Daniel P . Berrangé
Currently this logic is only run in the pflash codepath
but we want to run it in other scenarios too.

Signed-off-by: Daniel P. Berrangé 
---
 hw/i386/pc_sysfw.c| 24 +++-
 hw/i386/pc_sysfw_ovmf-stubs.c | 10 ++
 hw/i386/pc_sysfw_ovmf.c   | 27 +++
 include/hw/i386/pc.h  |  1 +
 4 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index c8b17af953..b056526077 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -146,9 +146,6 @@ static void pc_system_flash_map(PCMachineState *pcms,
 int64_t size;
 PFlashCFI01 *system_flash;
 MemoryRegion *flash_mem;
-void *flash_ptr;
-int flash_size;
-int ret;
 
 assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
 
@@ -192,24 +189,9 @@ static void pc_system_flash_map(PCMachineState *pcms,
 flash_mem = pflash_cfi01_get_memory(system_flash);
 pc_isa_bios_init(rom_memory, flash_mem, size);
 
-/* Encrypt the pflash boot ROM */
-if (sev_enabled()) {
-flash_ptr = memory_region_get_ram_ptr(flash_mem);
-flash_size = memory_region_size(flash_mem);
-/*
- * OVMF places a GUIDed structures in the flash, so
- * search for them
- */
-pc_system_parse_ovmf_flash(flash_ptr, flash_size);
-
-ret = sev_es_save_reset_vector(flash_ptr, flash_size);
-if (ret) {
-error_report("failed to locate and/or save reset vector");
-exit(1);
-}
-
-sev_encrypt_flash(flash_ptr, flash_size, _fatal);
-}
+pc_system_ovmf_initialize_sev(
+memory_region_get_ram_ptr(flash_mem),
+memory_region_size(flash_mem));
 }
 }
 }
diff --git a/hw/i386/pc_sysfw_ovmf-stubs.c b/hw/i386/pc_sysfw_ovmf-stubs.c
index aabe78b271..d88970af88 100644
--- a/hw/i386/pc_sysfw_ovmf-stubs.c
+++ b/hw/i386/pc_sysfw_ovmf-stubs.c
@@ -14,6 +14,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/i386/pc.h"
+#include "sev.h"
 
 bool pc_system_ovmf_table_find(const char *entry, uint8_t **data, int 
*data_len)
 {
@@ -24,3 +25,12 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t 
flash_size)
 {
 g_assert_not_reached();
 }
+
+void pc_system_ovmf_initialize_sev(void *ptr, size_t size)
+{
+if (!sev_enabled()) {
+return;
+}
+
+g_assert_not_reached();
+}
diff --git a/hw/i386/pc_sysfw_ovmf.c b/hw/i386/pc_sysfw_ovmf.c
index f4dd92c588..180500a035 100644
--- a/hw/i386/pc_sysfw_ovmf.c
+++ b/hw/i386/pc_sysfw_ovmf.c
@@ -24,8 +24,10 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/i386/pc.h"
 #include "cpu.h"
+#include "sev.h"
 
 #define OVMF_TABLE_FOOTER_GUID "96b582de-1fb2-45f7-baea-a366c55a082d"
 
@@ -149,3 +151,28 @@ bool pc_system_ovmf_table_find(const char *entry, uint8_t 
**data,
 }
 return false;
 }
+
+
+void pc_system_ovmf_initialize_sev(void *ptr, size_t size)
+{
+int ret;
+
+/* Encrypt the boot ROM */
+if (!sev_enabled()) {
+return;
+}
+
+/*
+ * OVMF places a GUIDed structures in the flash, so
+ * search for them
+ */
+pc_system_parse_ovmf_flash(ptr, size);
+
+ret = sev_es_save_reset_vector(ptr, size);
+if (ret) {
+error_report("failed to locate and/or save reset vector");
+exit(1);
+}
+
+sev_encrypt_flash(ptr, size, _fatal);
+}
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9c9f4ac748..47f5bc82ba 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -191,6 +191,7 @@ void pc_system_firmware_init(PCMachineState *pcms, 
MemoryRegion *rom_memory);
 bool pc_system_ovmf_table_find(const char *entry, uint8_t **data,
int *data_len);
 void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size);
+void pc_system_ovmf_initialize_sev(void *ptr, size_t size);
 
 /* hw/i386/acpi-common.c */
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-- 
2.33.1




[PATCH 0/2] Improved support for AMD SEV firmware loading

2022-01-13 Thread Daniel P . Berrangé
The AMD SEV build of EDK2 only emits a single file, intended to be
mapped readonly. There is explicitly no separate writable VARS
store for persisting non-volatile firmware variables.

This can be used with QEMU's traditional pflash configuration
mechanism by only populating pflash0, leaving pflash1 unconfigured.
Conceptually, however, it is odd to be using pflash at all when we
have no intention of supporting any writable variables. The -bios
option should be sufficient for any firmware that is exclusively
readonly code.


A second issue is that the firmware descriptor schema does not allow
for describing a firmware that uses pflash, without any associated
non-volatile storage.

In docs/interop/firmware.json

 'struct' : 'FirmwareMappingFlash',
  'data'   : { 'executable' : 'FirmwareFlashFile',
   'nvram-template' : 'FirmwareFlashFile' } }

Notice that nvram-template is mandatory, and when consuming these
files libvirt will thus complain if the nvram-template field is
missing.

We could in theory make nvram-template optional in the schema and
then update libvirt to take account of it, but this feels dubious
when we have a perfectly good way of describing a firmware without
NVRAM, using 'FirmwareMappingMemory' which is intended to be used
with QEMU's -bios option.


A third issue is in libvirt, where again the code handling the
configuration of pflash supports two scenarios

 - A single pflash image, which is writable
 - A pair of pflash images, one writable one readonly

There is no support for a single read-only pflash image in libvirt
today.


This all points towards the fact that we should be using -bios
to load the AMD SEV firmware build of EDK.

The only thing preventing us doing that is that QEMU does not
initialize the SEV firmware when using -bios. That is fairly
easily solved, as done in this patch series.

For testing I've launched QEMU in in these scenarios

  - SEV guest using -bios and boot from HD
  - SEV guest using pflash and boot from HD
  - SEV-ES guest using -bios and direct kernel boot
  - SEV-ES guest using pflash and direct kernel boot

In all these cases I was able to validate the reported SEV
guest measurement.

Daniel P. Berrangé (2):
  hw/i386: refactor logic for setting up SEV firmware
  hw/i386: support loading OVMF using -bios too

 hw/i386/pc_sysfw.c| 24 +++-
 hw/i386/pc_sysfw_ovmf-stubs.c | 10 ++
 hw/i386/pc_sysfw_ovmf.c   | 27 +++
 hw/i386/x86.c |  5 +
 include/hw/i386/pc.h  |  1 +
 5 files changed, 46 insertions(+), 21 deletions(-)

-- 
2.33.1





Re: [PATCH 2/2] qapi/ui: introduce change-vnc-listen

2022-01-13 Thread Daniel P . Berrangé
On Thu, Jan 13, 2022 at 05:27:08PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Mon, Dec 20, 2021 at 04:44:18PM +0100, Vladimir Sementsov-Ogievskiy 
> > wrote:
> >> Add command that can change addresses where VNC server listens for new
> >> connections. Prior to 6.0 this functionality was available through
> >> 'change' qmp command which was deleted.
> >> 
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> >> ---
> >>  docs/about/removed-features.rst |  3 ++-
> >>  qapi/ui.json| 12 
> >>  ui/vnc.c| 26 ++
> >>  3 files changed, 40 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/docs/about/removed-features.rst 
> >> b/docs/about/removed-features.rst
> >> index d42c3341de..20e6901a82 100644
> >> --- a/docs/about/removed-features.rst
> >> +++ b/docs/about/removed-features.rst
> >> @@ -348,7 +348,8 @@ documentation of ``query-hotpluggable-cpus`` for 
> >> additional details.
> >>  ``change`` (removed in 6.0)
> >>  '''
> >>  
> >> -Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
> >> +Use ``blockdev-change-medium`` or ``change-vnc-password`` or
> >> +``change-vnc-listen`` instead.
> >>  
> >>  ``query-events`` (removed in 6.0)
> >>  '
> >> diff --git a/qapi/ui.json b/qapi/ui.json
> >> index d7567ac866..14e6fe0b4c 100644
> >> --- a/qapi/ui.json
> >> +++ b/qapi/ui.json
> >> @@ -1304,3 +1304,15 @@
> >>  { 'command': 'display-reload',
> >>'data': 'DisplayReloadOptions',
> >>'boxed' : true }
> >> +
> >> +##
> >> +# @change-vnc-listen:
> >> +#
> >> +# Change set of addresses to listen for connections.
> >> +#
> >> +# Since: 7.0
> >> +#
> >> +##
> >> +{ 'command': 'change-vnc-listen',
> >> +  'data': { 'id': 'str', 'addresses': ['SocketAddress'],
> >> +'*websockets': ['SocketAddress'] } }
> >
> > We already have a general purpose command above 'display-reload'
> > for doing live changes to the display backends.
> >
> > THis should instead be
> >
> > { 'struct': 'DisplayReloadOptionsVNC',
> >   'data': { '*tls-certs': 'bool',
> > '*addresses': ['SocketAddress'],
> > '*websockets': ['SocketAddress'] } }
> >
> > if 'addresses' is non-null then the listener can be updated.
> 
> Good point.  Gerd, what do you think?

I guess you could argue that 'display-reload' is more about reloading
state, rather than changing configuration.  If we want to make such a
distinction, then we could have a very similar 'display-update' command
for configuration changes.

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: [PULL 10/13] numa: Enable numa for SGX EPC sections

2022-01-13 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 09:25:12PM +0100, Paolo Bonzini wrote:
> From: Yang Zhong 
> 
> The basic SGX did not enable numa for SGX EPC sections, which
> result in all EPC sections located in numa node 0. This patch
> enable SGX numa function in the guest and the EPC section can
> work with RAM as one numa node.
> 
> The Guest kernel related log:
> [0.009981] ACPI: SRAT: Node 0 PXM 0 [mem 0x18000-0x183ff]
> [0.009982] ACPI: SRAT: Node 1 PXM 1 [mem 0x18400-0x185bf]
> The SRAT table can normally show SGX EPC sections menory info in different
> numa nodes.
> 
> The SGX EPC numa related command:
>  ..
>  -m 4G,maxmem=20G \
>  -smp sockets=2,cores=2 \
>  -cpu host,+sgx-provisionkey \
>  -object memory-backend-ram,size=2G,host-nodes=0,policy=bind,id=node0 \
>  -object 
> memory-backend-epc,id=mem0,size=64M,prealloc=on,host-nodes=0,policy=bind \
>  -numa node,nodeid=0,cpus=0-1,memdev=node0 \
>  -object memory-backend-ram,size=2G,host-nodes=1,policy=bind,id=node1 \
>  -object 
> memory-backend-epc,id=mem1,size=28M,prealloc=on,host-nodes=1,policy=bind \
>  -numa node,nodeid=1,cpus=2-3,memdev=node1 \
>  -M 
> sgx-epc.0.memdev=mem0,sgx-epc.0.node=0,sgx-epc.1.memdev=mem1,sgx-epc.1.node=1 
> \
>  ..
> 
> Signed-off-by: Yang Zhong 
> Message-Id: <20211101162009.62161-2-yang.zh...@intel.com>
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/core/numa.c|  5 ++---
>  hw/i386/acpi-build.c  |  2 ++
>  hw/i386/sgx-epc.c |  3 +++
>  hw/i386/sgx-stub.c|  4 
>  hw/i386/sgx.c | 44 +++
>  include/hw/i386/sgx-epc.h |  3 +++
>  monitor/hmp-cmds.c|  1 +
>  qapi/machine.json | 10 -
>  qemu-options.hx   |  4 ++--
>  9 files changed, 70 insertions(+), 6 deletions(-)

> diff --git a/qapi/machine.json b/qapi/machine.json
> index f1839acf20..edeab6084b 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1207,12 +1207,15 @@
>  #
>  # @memdev: memory backend linked with device
>  #
> +# @node: the numa node

This needs a "(Since: 7.0)" annotation

> +#
>  # Since: 6.2
>  ##
>  { 'struct': 'SgxEPCDeviceInfo',
>'data': { '*id': 'str',
>  'memaddr': 'size',
>  'size': 'size',
> +'node': 'int',
>  'memdev': 'str'
>}
>  }
> @@ -1285,10 +1288,15 @@
>  #
>  # @memdev: memory backend linked with device
>  #
> +# @node: the numa node

Likewise

> +#
>  # Since: 6.2
>  ##
>  { 'struct': 'SgxEPC',
> -  'data': { 'memdev': 'str' } }
> +  'data': { 'memdev': 'str',
> +'node': 'int'
> +  }
> +}
>  

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: [PULL 11/13] numa: Support SGX numa in the monitor and Libvirt interfaces

2022-01-13 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 09:25:13PM +0100, Paolo Bonzini wrote:
> From: Yang Zhong 
> 
> Add the SGXEPCSection list into SGXInfo to show the multiple
> SGX EPC sections detailed info, not the total size like before.
> This patch can enable numa support for 'info sgx' command and
> QMP interfaces. The new interfaces show each EPC section info
> in one numa node. Libvirt can use QMP interface to get the
> detailed host SGX EPC capabilities to decide how to allocate
> host EPC sections to guest.



> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 5aa2b95b7d..1022aa0184 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -337,6 +337,21 @@
>'if': 'TARGET_ARM' }
>  
>  
> +##
> +# @SGXEPCSection:
> +#
> +# Information about intel SGX EPC section info
> +#
> +# @node: the numa node
> +#
> +# @size: the size of epc section
> +#
> +# Since: 6.2

This is wrong because it was merged for 7.0 not 6.2

> +##
> +{ 'struct': 'SGXEPCSection',
> +  'data': { 'node': 'int',
> +'size': 'uint64'}}
> +
>  ##
>  # @SGXInfo:
>  #
> @@ -350,7 +365,7 @@
>  #
>  # @flc: true if FLC is supported
>  #
> -# @section-size: The EPC section size for guest
> +# @sections: The EPC sections info for guest

This is a non-backwards compatible schema change.

"@section-size" must not be removed without going
through a deprecation period, so this needs to be
re-instated.

The "@sections" addition needs a "Since 7.0" annotation too.


Yong, can you submit a followup patch to correct these mistakes


With 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 v7 4/5] migration: Add migrate_use_tls() helper

2022-01-13 Thread Daniel P . Berrangé
On Thu, Jan 06, 2022 at 07:13:41PM -0300, Leonardo Bras wrote:
> A lot of places check parameters.tls_creds in order to evaluate if TLS is
> in use, and sometimes call migrate_get_current() just for that test.
> 
> Add new helper function migrate_use_tls() in order to simplify testing
> for TLS usage.
> 
> Signed-off-by: Leonardo Bras 
> Reviewed-by: Juan Quintela 
> ---
>  migration/migration.h | 1 +
>  migration/channel.c   | 6 +++---
>  migration/migration.c | 9 +
>  migration/multifd.c   | 5 +
>  4 files changed, 14 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé 

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




Re: [PATCH v7 3/5] migration: Add zero-copy parameter for QMP/HMP for Linux

2022-01-13 Thread Daniel P . Berrangé
On Thu, Jan 06, 2022 at 07:13:40PM -0300, Leonardo Bras wrote:
> Add property that allows zero-copy migration of memory pages,
> and also includes a helper function migrate_use_zero_copy() to check
> if it's enabled.
> 
> No code is introduced to actually do the migration, but it allow
> future implementations to enable/disable this feature.
> 
> On non-Linux builds this parameter is compiled-out.
> 
> Signed-off-by: Leonardo Bras 
> ---
>  qapi/migration.json   | 24 
>  migration/migration.h |  5 +
>  migration/migration.c | 32 
>  migration/socket.c|  5 +
>  monitor/hmp-cmds.c|  6 ++
>  5 files changed, 72 insertions(+)

Reviewed-by: Daniel P. Berrangé 

> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index bbfd48cf0b..2e62ea6ebd 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -730,6 +730,13 @@
>  #  will consume more CPU.
>  #  Defaults to 1. (Since 5.0)
>  #
> +# @zero-copy: Controls behavior on sending memory pages on migration.
> +# When true, enables a zero-copy mechanism for sending memory
> +# pages, if host supports it.
> +# Requires that QEMU be permitted to use locked memory for guest
> +# RAM pages.
> +# Defaults to false. (Since 7.0)
> +#
>  # @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>  #aliases for the purpose of dirty bitmap migration.  
> Such
>  #aliases may for example be the corresponding names 
> on the
> @@ -769,6 +776,7 @@
> 'xbzrle-cache-size', 'max-postcopy-bandwidth',
> 'max-cpu-throttle', 'multifd-compression',
> 'multifd-zlib-level' ,'multifd-zstd-level',
> +   { 'name': 'zero-copy', 'if' : 'CONFIG_LINUX'},
> 'block-bitmap-mapping' ] }
>  
>  ##
> @@ -895,6 +903,13 @@
>  #  will consume more CPU.
>  #  Defaults to 1. (Since 5.0)
>  #
> +# @zero-copy: Controls behavior on sending memory pages on migration.
> +# When true, enables a zero-copy mechanism for sending memory
> +# pages, if host supports it.
> +# Requires that QEMU be permitted to use locked memory for guest
> +# RAM pages.
> +# Defaults to false. (Since 7.0)
> +#
>  # @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>  #aliases for the purpose of dirty bitmap migration.  
> Such
>  #aliases may for example be the corresponding names 
> on the
> @@ -949,6 +964,7 @@
>  '*multifd-compression': 'MultiFDCompression',
>  '*multifd-zlib-level': 'uint8',
>  '*multifd-zstd-level': 'uint8',
> +'*zero-copy': { 'type': 'bool', 'if': 'CONFIG_LINUX' },
>  '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }

The current zerocopy impl is for the send path.

Do you expect we might get zerocopy in the receive path
later ?

If so then either call this 'send-zero-copy', or change it
from a bool to an enum taking '["send", "recv", "both"]'.

I'd probably take the former and just rename it.


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 v7 2/5] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX

2022-01-13 Thread Daniel P . Berrangé
On Thu, Jan 06, 2022 at 07:13:39PM -0300, Leonardo Bras wrote:
> For CONFIG_LINUX, implement the new zero copy flag and the optional callback
> io_flush on QIOChannelSocket, but enables it only when MSG_ZEROCOPY
> feature is available in the host kernel, which is checked on
> qio_channel_socket_connect_sync()
> 
> qio_channel_socket_flush() was implemented by counting how many times
> sendmsg(...,MSG_ZEROCOPY) was successfully called, and then reading the
> socket's error queue, in order to find how many of them finished sending.
> Flush will loop until those counters are the same, or until some error occurs.
> 
> Notes on using writev() with QIO_CHANNEL_WRITE_FLAG_ZERO_COPY:
> 1: Buffer
> - As MSG_ZEROCOPY tells the kernel to use the same user buffer to avoid 
> copying,
> some caution is necessary to avoid overwriting any buffer before it's sent.
> If something like this happen, a newer version of the buffer may be sent 
> instead.
> - If this is a problem, it's recommended to call qio_channel_flush() before 
> freeing
> or re-using the buffer.
> 
> 2: Locked memory
> - When using MSG_ZERCOCOPY, the buffer memory will be locked after queued, and
> unlocked after it's sent.
> - Depending on the size of each buffer, and how often it's sent, it may 
> require
> a larger amount of locked memory than usually available to non-root user.
> - If the required amount of locked memory is not available, writev_zero_copy
> will return an error, which can abort an operation like migration,
> - Because of this, when an user code wants to add zero copy as a feature, it
> requires a mechanism to disable it, so it can still be accessible to less
> privileged users.
> 
> Signed-off-by: Leonardo Bras 
> ---
>  include/io/channel-socket.h |   2 +
>  io/channel-socket.c | 107 ++++++--
>  2 files changed, 105 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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




Re: [PATCH v7 1/5] QIOChannel: Add flags on io_writev and introduce io_flush callback

2022-01-13 Thread Daniel P . Berrangé
On Thu, Jan 06, 2022 at 07:13:38PM -0300, Leonardo Bras wrote:
> Add flags to io_writev and introduce io_flush as optional callback to
> QIOChannelClass, allowing the implementation of zero copy writes by
> subclasses.
> 
> How to use them:
> - Write data using qio_channel_writev(...,QIO_CHANNEL_WRITE_FLAG_ZERO_COPY),
> - Wait write completion with qio_channel_flush().
> 
> Notes:
> As some zero copy implementations work asynchronously, it's
> recommended to keep the write buffer untouched until the return of
> qio_channel_flush(), to avoid the risk of sending an updated buffer
> instead of the buffer state during write.
> 
> As io_flush callback is optional, if a subclass does not implement it, then:
> - io_flush will return 0 without changing anything.
> 
> Also, some functions like qio_channel_writev_full_all() were adapted to
> receive a flag parameter. That allows shared code between zero copy and
> non-zero copy writev, and also an easier implementation on new flags.
> 
> Signed-off-by: Leonardo Bras 
> ---
>  include/io/channel.h | 67 +++-
>  io/channel-buffer.c  |  1 +
>  io/channel-command.c |  1 +
>  io/channel-file.c|  1 +
>  io/channel-socket.c  |  2 ++
>  io/channel-tls.c |  1 +
>  io/channel-websock.c |  1 +
>  io/channel.c | 51 +++--
>  migration/rdma.c |  1 +
>  9 files changed, 98 insertions(+), 28 deletions(-)
> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 88988979f8..343766ce5b 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -32,12 +32,15 @@ OBJECT_DECLARE_TYPE(QIOChannel, QIOChannelClass,
>  
>  #define QIO_CHANNEL_ERR_BLOCK -2
>  
> +#define QIO_CHANNEL_WRITE_FLAG_ZERO_COPY 0x1
> +
>  typedef enum QIOChannelFeature QIOChannelFeature;
>  
>  enum QIOChannelFeature {
>  QIO_CHANNEL_FEATURE_FD_PASS,
>  QIO_CHANNEL_FEATURE_SHUTDOWN,
>  QIO_CHANNEL_FEATURE_LISTEN,
> +QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY,
>  };
>  
>  
> @@ -104,6 +107,7 @@ struct QIOChannelClass {
>   size_t niov,
>   int *fds,
>   size_t nfds,
> + int flags,
>   Error **errp);
>  ssize_t (*io_readv)(QIOChannel *ioc,
>  const struct iovec *iov,
> @@ -136,6 +140,8 @@ struct QIOChannelClass {
>IOHandler *io_read,
>IOHandler *io_write,
>void *opaque);
> +int (*io_flush)(QIOChannel *ioc,
> +Error **errp);
>  };
>  
>  /* General I/O handling functions */
> @@ -222,12 +228,13 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
>  
>  
>  /**
> - * qio_channel_writev_full:
> + * qio_channel_writev_full_flags:
>   * @ioc: the channel object
>   * @iov: the array of memory regions to write data from
>   * @niov: the length of the @iov array
>   * @fds: an array of file handles to send
>   * @nfds: number of file handles in @fds
> + * @flags: write flags (QIO_CHANNEL_WRITE_FLAG_*)
>   * @errp: pointer to a NULL-initialized error object
>   *
>   * Write data to the IO channel, reading it from the
> @@ -255,12 +262,16 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
>   * or QIO_CHANNEL_ERR_BLOCK if no data is can be sent
>   * and the channel is non-blocking
>   */
> -ssize_t qio_channel_writev_full(QIOChannel *ioc,
> -const struct iovec *iov,
> -size_t niov,
> -int *fds,
> -size_t nfds,
> -Error **errp);
> +ssize_t qio_channel_writev_full_flags(QIOChannel *ioc,
> +  const struct iovec *iov,
> +  size_t niov,
> +  int *fds,
> +  size_t nfds,
> +  int flags,
> +  Error **errp);
> +
> +#define qio_channel_writev_full(ioc, iov, niov, fds, nfds, errp) \
> +qio_channel_writev_full_flags(ioc, iov, niov, fds, nfds, 0, errp)

Don't introduce yet another API variant here. Just add flags to
all the existing write APIs with "full" in their name. The word
"full" in their name was intended to indicate that they are
accepting all possible parameters, so it doesn't mean sense to
add APIs which take even more possible parameters.

> +int qio_channel_writev_full_all_flags(QIOChannel *ioc,
> +  const struct iovec *iov,
> +  size_t niov,
> +  int *fds, size_t nfds,
> +  int flags, Error **errp);
> +#define qio_channel_writev_full_all(ioc, iov, niov, fds, nfds, errp) \
> +qio_channel_writev_full_all_flags(ioc, 

Re: [PATCH v7 2/5] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX

2022-01-13 Thread Daniel P . Berrangé
On Thu, Jan 13, 2022 at 06:34:12PM +0800, Peter Xu wrote:
> On Thu, Jan 13, 2022 at 10:06:14AM +0000, Daniel P. Berrangé wrote:
> > On Thu, Jan 13, 2022 at 02:48:15PM +0800, Peter Xu wrote:
> > > On Thu, Jan 06, 2022 at 07:13:39PM -0300, Leonardo Bras wrote:
> > > > @@ -558,15 +575,26 @@ static ssize_t 
> > > > qio_channel_socket_writev(QIOChannel *ioc,
> > > >  memcpy(CMSG_DATA(cmsg), fds, fdsize);
> > > >  }
> > > >  
> > > > +if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> > > > +sflags = MSG_ZEROCOPY;
> > > > +}
> > > > +
> > > >   retry:
> > > > -ret = sendmsg(sioc->fd, , 0);
> > > > +ret = sendmsg(sioc->fd, , sflags);
> > > >  if (ret <= 0) {
> > > > -if (errno == EAGAIN) {
> > > > +switch (errno) {
> > > > +case EAGAIN:
> > > >  return QIO_CHANNEL_ERR_BLOCK;
> > > > -}
> > > > -if (errno == EINTR) {
> > > > +case EINTR:
> > > >  goto retry;
> > > > +case ENOBUFS:
> > > > +if (sflags & MSG_ZEROCOPY) {
> > > > +error_setg_errno(errp, errno,
> > > > + "Process can't lock enough memory for 
> > > > using MSG_ZEROCOPY");
> > > > +return -1;
> > > > +}
> > > 
> > > I have no idea whether it'll make a real differnece, but - should we 
> > > better add
> > > a "break" here?  If you agree and with that fixed, feel free to add:
> > > 
> > > Reviewed-by: Peter Xu 
> > > 
> > > I also wonder whether you hit ENOBUFS in any of the environments.  On 
> > > Fedora
> > > here it's by default unlimited, but just curious when we should keep an 
> > > eye.
> > 
> > Fedora doesn't allow unlimited locked memory by default
> > 
> > $ grep "locked memory" /proc/self/limits 
> > Max locked memory 6553665536bytes   
> >   
> > 
> > And  regardless of Fedora defaults, libvirt will set a limit
> > for the guest. It will only be unlimited if requiring certain
> > things like VFIO.
> 
> Thanks, I obviously checked up the wrong host..
> 
> Leo, do you know how much locked memory will be needed by zero copy?  Will
> there be a limit?  Is it linear to the number of sockets/channels?

IIRC we decided it would be limited by the socket send buffer size, rather
than guest RAM, because writes will block once the send buffer is full.

This has a default global setting, with per-socket override. On one box I
have it is 200 Kb. With multifd you'll need  "num-sockets * send buffer".

> It'll be better if we can fail at enabling the feature when we detected that
> the specified locked memory limit may not be suffice.

Checking this value against available locked memory though will always
have an error margin because other things in QEMU can use locked memory
too


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 v7 2/5] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX

2022-01-13 Thread Daniel P . Berrangé
On Thu, Jan 13, 2022 at 02:48:15PM +0800, Peter Xu wrote:
> On Thu, Jan 06, 2022 at 07:13:39PM -0300, Leonardo Bras wrote:
> > @@ -558,15 +575,26 @@ static ssize_t qio_channel_socket_writev(QIOChannel 
> > *ioc,
> >  memcpy(CMSG_DATA(cmsg), fds, fdsize);
> >  }
> >  
> > +if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> > +sflags = MSG_ZEROCOPY;
> > +}
> > +
> >   retry:
> > -ret = sendmsg(sioc->fd, , 0);
> > +ret = sendmsg(sioc->fd, , sflags);
> >  if (ret <= 0) {
> > -if (errno == EAGAIN) {
> > +switch (errno) {
> > +case EAGAIN:
> >  return QIO_CHANNEL_ERR_BLOCK;
> > -}
> > -if (errno == EINTR) {
> > +case EINTR:
> >  goto retry;
> > +case ENOBUFS:
> > +if (sflags & MSG_ZEROCOPY) {
> > +error_setg_errno(errp, errno,
> > + "Process can't lock enough memory for 
> > using MSG_ZEROCOPY");
> > +return -1;
> > +}
> 
> I have no idea whether it'll make a real differnece, but - should we better 
> add
> a "break" here?  If you agree and with that fixed, feel free to add:
> 
> Reviewed-by: Peter Xu 
> 
> I also wonder whether you hit ENOBUFS in any of the environments.  On Fedora
> here it's by default unlimited, but just curious when we should keep an eye.

Fedora doesn't allow unlimited locked memory by default

$ grep "locked memory" /proc/self/limits 
Max locked memory 6553665536bytes 

And  regardless of Fedora defaults, libvirt will set a limit
for the guest. It will only be unlimited if requiring certain
things like VFIO.

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: PyPI account

2022-01-12 Thread Daniel P . Berrangé
On Wed, Jan 12, 2022 at 12:47:01PM -0500, John Snow wrote:
> On Wed, Jan 12, 2022 at 12:34 PM Daniel P. Berrangé  
> wrote:
> >
> > On Wed, Jan 12, 2022 at 12:25:16PM -0500, John Snow wrote:
> > > On Wed, Jan 12, 2022 at 5:56 AM Stefan Hajnoczi  
> > > wrote:
> > > >
> > > > [Context: John created a PyPI QEMU user in order to publish the qemu.qmp
> > > > package. If anyone wants to publish additional Python packages from
> > > > qemu.git, please contact him for PyPI access.]
> > > >
> > > > On Tue, Jan 11, 2022 at 03:42:23PM -0500, John Snow wrote:
> > > > > Account made: https://pypi.org/user/QEMU/
> > > > >
> > > > > I can't update the wiki, I lack admin perms to edit
> > > > > https://wiki.qemu.org/AdminContacts
> > > > >
> > > > > I assume in the event that I fall into a black hole or get launched
> > > > > out of a cannon into the sun, any mails sent to js...@redhat.com can
> > > > > be recovered by Red Hat in general, so there's a sufficient recourse
> > > > > for recovering the account in that circumstance.
> > > >
> > > > Thanks, I have added the PyPI QEMU user and added you as the admin
> > > > contact:
> > > > https://wiki.qemu.org/AdminContacts#Other_resources
> > > >
> > > > Stefan
> > >
> > > Thanks, Stefan!
> > >
> > > As additional context, there is currently a single package that
> > > belongs to that user, "qemu.qmp" [1]. I published it "in advance" to
> > > be able to test integration in an RFC patch set I posted to the list
> > > just before the winter break [2]. The package is an "alpha prerelease"
> > > and is at a very low-risk to be installed by accident. The version
> > > chosen here will be considered "less than" any other valid version
> > > string chosen, and can be deleted permanently from PyPI after
> > > consensus on list review. Please forgive the mid-review publishing.
> > > The exact metadata, wording of the README, etc is still under review
> > > here [3].
> > >
> > > As for the PyPI account itself, I have volunteered to administer it.
> > > If anyone wants access (esp. a leadership committee member from
> > > another employer), please contact me - I'm happy to share.
> >
> > As a general rule we should ensure we always have 2 nominated people
> > with access to any account held on behalf of the QEMU project, so we
> > have some redundancy in case of unexpected events. So we definitely
> > ought to have a 2nd person with access to PyPI even if they do nothing
> > with it.
> >
> 
> I'm perfectly fine with that. It'd increase the fault tolerance to
> have someone from outside of RH be the paper-admin there. Any
> volunteers?
> 
> (I can add your email as a secondary contact to the account such that
> you would be able to use that email to initiate an account recovery,
> but you wouldn't receive email from PyPI concerning the comings and
> goings of the account. Your name and email would not show up on any
> PyPI package pages, so it should very hopefully not involve really any
> actual maintainership on your part.)

In my case I expect it could complain that my email(s) are already
in use for other PyPI accounts of my own.

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: PyPI account

2022-01-12 Thread Daniel P . Berrangé
On Wed, Jan 12, 2022 at 12:25:16PM -0500, John Snow wrote:
> On Wed, Jan 12, 2022 at 5:56 AM Stefan Hajnoczi  wrote:
> >
> > [Context: John created a PyPI QEMU user in order to publish the qemu.qmp
> > package. If anyone wants to publish additional Python packages from
> > qemu.git, please contact him for PyPI access.]
> >
> > On Tue, Jan 11, 2022 at 03:42:23PM -0500, John Snow wrote:
> > > Account made: https://pypi.org/user/QEMU/
> > >
> > > I can't update the wiki, I lack admin perms to edit
> > > https://wiki.qemu.org/AdminContacts
> > >
> > > I assume in the event that I fall into a black hole or get launched
> > > out of a cannon into the sun, any mails sent to js...@redhat.com can
> > > be recovered by Red Hat in general, so there's a sufficient recourse
> > > for recovering the account in that circumstance.
> >
> > Thanks, I have added the PyPI QEMU user and added you as the admin
> > contact:
> > https://wiki.qemu.org/AdminContacts#Other_resources
> >
> > Stefan
> 
> Thanks, Stefan!
> 
> As additional context, there is currently a single package that
> belongs to that user, "qemu.qmp" [1]. I published it "in advance" to
> be able to test integration in an RFC patch set I posted to the list
> just before the winter break [2]. The package is an "alpha prerelease"
> and is at a very low-risk to be installed by accident. The version
> chosen here will be considered "less than" any other valid version
> string chosen, and can be deleted permanently from PyPI after
> consensus on list review. Please forgive the mid-review publishing.
> The exact metadata, wording of the README, etc is still under review
> here [3].
> 
> As for the PyPI account itself, I have volunteered to administer it.
> If anyone wants access (esp. a leadership committee member from
> another employer), please contact me - I'm happy to share.

As a general rule we should ensure we always have 2 nominated people
with access to any account held on behalf of the QEMU project, so we
have some redundancy in case of unexpected events. So we definitely
ought to have a 2nd person with access to PyPI even if they do nothing
with it.

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] sgx: Move sgx object from /machine/unattached to /machine

2022-01-12 Thread Daniel P . Berrangé
On Wed, Jan 12, 2022 at 10:16:33AM +, Peter Maydell wrote:
> On Wed, 12 Jan 2022 at 10:14, Daniel P. Berrangé  wrote:
> >
> > On Wed, Jan 12, 2022 at 11:55:17AM -0500, Yang Zhong wrote:
> > > When Libvirt start, it get the vcpu's unavailable-features from
> > > /machine/unattached/device[0] path by qom-get command, but in SGX
> > > guest, since the sgx-epc virtual device is initialized before VCPU
> > > creation(virtual sgx need set the virtual EPC info in the cpuid). This
> > > /machine/unattached/device[0] is occupied by sgx-epc device, which
> > > fail to get the unvailable-features from /machine/unattached/device[0].
> >
> > If libvirt decides to enable SGX in a VM, then surely it knows
> > that it should just query /machine/unattached/device[1] to get
> > the CPU features instead. Why do we need to do anything in QEMU ?
> 
> libvirt having to know it needs to look at /machine/unattached/device[n]
> for anything is a bit fragile, really... it's effectively encoding
> knowledge about what order things happen to get created inside QEMU.

So how do CPUs and other devices end up being under /unattached/ ?
Can we ensure that *all* QEMU devices have a well defined attachment
point ?

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] sgx: Move sgx object from /machine/unattached to /machine

2022-01-12 Thread Daniel P . Berrangé
On Wed, Jan 12, 2022 at 11:55:17AM -0500, Yang Zhong wrote:
> When Libvirt start, it get the vcpu's unavailable-features from
> /machine/unattached/device[0] path by qom-get command, but in SGX
> guest, since the sgx-epc virtual device is initialized before VCPU
> creation(virtual sgx need set the virtual EPC info in the cpuid). This
> /machine/unattached/device[0] is occupied by sgx-epc device, which
> fail to get the unvailable-features from /machine/unattached/device[0].

If libvirt decides to enable SGX in a VM, then surely it knows
that it should just query /machine/unattached/device[1] to get
the CPU features instead. Why do we need to do anything in QEMU ?

> 
> This patch make one new /machine/sgx object to avoid this issue.
> (qemu) qom-list /machine/unattached/
> device[0] (child)
> 
> (qemu) qom-list /machine/sgx
> device[0] (child)
> 
> Signed-off-by: Yang Zhong 
> ---
>  hw/core/qdev.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 84f3019440..4154eef0d8 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -497,7 +497,7 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>  NamedClockList *ncl;
>  Error *local_err = NULL;
>  bool unattached_parent = false;
> -static int unattached_count;
> +static int unattached_count, sgx_count;
>  
>  if (dev->hotplugged && !dc->hotpluggable) {
>  error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> @@ -509,7 +509,15 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>  goto fail;
>  }
>  
> -if (!obj->parent) {
> +if (!obj->parent && !strcmp(object_get_typename(obj), "sgx-epc")) {
> +gchar *name = g_strdup_printf("device[%d]", sgx_count++);
> +
> +object_property_add_child(container_get(qdev_get_machine(),
> +"/sgx"),
> +  name, obj);
> +unattached_parent = true;
> +g_free(name);

The qdev.c file is part of our generic object code. It should not
contain any code that is tied to very specific object types like
this.

> +} else if (!obj->parent) {
>  gchar *name = g_strdup_printf("device[%d]", unattached_count++);
>  
>  object_property_add_child(container_get(qdev_get_machine(),

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: [RFC qemu.qmp PATCH 17/24] Makefile: add build and publish targets

2022-01-12 Thread Daniel P . Berrangé
On Tue, Jan 11, 2022 at 02:48:55PM -0500, John Snow wrote:
> On Fri, Dec 17, 2021 at 8:46 AM Daniel P. Berrangé 
> wrote:
> 
> > On Thu, Dec 16, 2021 at 06:35:23PM -0500, John Snow wrote:
> > > On Thu, Dec 16, 2021 at 5:48 AM Daniel P. Berrangé 
> > > wrote:
> > >
> > > > On Wed, Dec 15, 2021 at 04:06:27PM -0500, John Snow wrote:
> > > > > Signed-off-by: John Snow 
> > > > > ---
> > > > >  Makefile | 32 
> > > > >  1 file changed, 32 insertions(+)
> > > > >
> > > > > diff --git a/Makefile b/Makefile
> > > > > index 97d737a..81bfca8 100644
> > > > > --- a/Makefile
> > > > > +++ b/Makefile
> > > > > @@ -110,3 +110,35 @@ distclean: clean
> > > > >   rm -f .coverage .coverage.*
> > > > >   rm -rf htmlcov/
> > > > >   rm -rf test-results/
> > > > > +
> > > > > +.PHONY: pristine
> > > > > +pristine:
> > > > > + @git diff-files --quiet --ignore-submodules -- || \
> > > > > + (echo "You have unstaged changes."; exit 1)
> > > > > + @git diff-index --cached --quiet HEAD --ignore-submodules --
> > || \
> > > > > + (echo "Your index contains uncommitted changes."; exit
> > 1)
> > > > > + @[ -z "$(shell git ls-files -o)" ] || \
> > > > > + (echo "You have untracked files: $(shell git ls-files
> > > > -o)"; exit 1)
> > > > > +
> > > > > +dist: setup.cfg setup.py Makefile README.rst
> > > > > + python3 -m build
> > > > > + @touch dist
> > > > > +
> > > > > +.PHONY: pre-publish
> > > > > +pre-publish: pristine dist
> > > > > + @git describe --exact-match 2>/dev/null || \
> > > > > + (echo -e "\033[0;31mThere is no annotated tag for this
> > > > commit.\033[0m"; exit 1)
> > > > > + python3 -m twine check --strict dist/*
> > > > > + git push -v --atomic --follow-tags --dry-run
> > > > > +
> > > > > +.PHONY: publish
> > > > > +publish: pre-publish
> > > > > + # Set the username via TWINE_USERNAME.
> > > > > + # Set the password via TWINE_PASSWORD.
> > > > > + # Set the pkg repository via TWINE_REPOSITORY.
> > > > > + python3 -m twine upload --verbose dist/*
> > > > > + git push -v --atomic --follow-tags
> > > > > +
> > > > > +.PHONY: publish-test
> > > > > +publish-test: pre-publish
> > > > > + python3 -m twine upload --verbose -r testpypi dist/*
> > > >
> > > > It doesn't feel very pythonic to have a makefile in the project.
> > > >
> > > > If we want some helpers for publishing releases, I would have
> > > > expected to see a python script  eg scripts/publish.py
> > > >
> > > >
> > > Eh, Python folks use Makefiles too. I've been using these little Makefile
> > > targets for hobby things for a while and I had them laying around and
> > ready
> > > to go. I have no strong need to "upgrade" to python scripts for these
> > right
> > > now, unless there's some extra features you want to see.
> >
> > Using make means you have to worry about portability across different
> > impls of make and different impls of shell. Using python means your
> > python project is portable to anywhere that python runs.
> 
> 
> I still like the idea of using a Makefile as a "canonical menu of things
> you can do in this directory", but there's probably room for interactive
> error checking and so on with the TWINE_USERNAME / TWINE_PASSWORD /
> TWINE_REPOSITORY environment variables in a python script. I'll look into
> it as a follow-up, if that's fine. (I'm worried it's a lot of polish and
> effort on a maintainers-only interface that only I will likely use for at
> least the next year or two.)
> 
> Ultimately, what's likely to happen here is that I will generate some oauth
> tokens with publish permissions and a hypothetical user would set e.g.
> TWINE_USERNAME to "__token__", and the password would be
> "pypi-tokengoeshere". Using the "keyring" python package, we could attempt
> to fetch stored values from a session keyring, falling back to an
> interactive prompt if they're unset.

FWIW, don't consider this original comment of mine to be a technical
blocker, rather it is more of a conceptual observation.  If you don't
think it matters, I won't mind.

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: cirrus-ci: FreeBSD failure (lttng-ust package not found)

2022-01-11 Thread Daniel P . Berrangé
On Tue, Jan 11, 2022 at 02:11:14PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Alex,
> 
> I am getting this failure for the x64-freebsd-*-build jobs [1, 2]:
> 
> pkg: No packages available to install matching 'lttng-ust' have been
> found in the repositories

It was being installed fine on FreeBSD 13 as recently as 5
days ago

  https://gitlab.com/qemu-project/qemu/-/jobs/1944746050

but then disappeared from FreeBSD 13 four days ago

  https://gitlab.com/qemu-project/qemu/-/jobs/1949284897

and then disappeared from FreeBSD 12 yesterday

  https://gitlab.com/qemu-project/qemu/-/jobs/1958443678

Looking back at the older working builds I see

[quote]
==>   NOTICE:

The lttng-ust port currently does not have a maintainer. As a result, it is
more likely to have unresolved issues, not be up-to-date, or even be removed in
the future. To volunteer to maintain this port, please create an issue at:

https://bugs.freebsd.org/bugzilla
[/quote]

I notice last year they marked it broken in >= 13:

  
https://cgit.freebsd.org/ports/commit/?id=abe5fefed2e24ef43a890f4d50ba712c581c7cfe

Overall it feels like we probably just need to drop this package
from the build on FreeBSD

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: Re: [PATCH] usb: allow max 8192 bytes for desc

2022-01-11 Thread Daniel P . Berrangé
On Tue, Jan 11, 2022 at 08:27:35PM +0800, zhenwei pi wrote:
> 
> 
> On 1/11/22 8:25 PM, Daniel P. Berrangé wrote:
> > On Tue, Jan 11, 2022 at 12:21:42PM +, Peter Maydell wrote:
> > > On Tue, 11 Jan 2022 at 10:54, zhenwei pi  wrote:
> > > > 
> > > > A device of USB video class usually uses larger desc structure, so
> > > > use larger buffer to avoid failure. (dev-video.c is ready)
> > > > 
> > > > Allocating memory dynamically by g_malloc of the orignal version of
> > > > this change, Philippe suggested just using the stack. Test the two
> > > > versions of qemu binary, the size of stack gets no change.
> > > > 
> > > > CC: Philippe Mathieu-Daudé 
> > > > Signed-off-by: zhenwei pi 
> > > > ---
> > > >   hw/usb/desc.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/usb/desc.c b/hw/usb/desc.c
> > > > index 8b6eaea407..57d2aedba1 100644
> > > > --- a/hw/usb/desc.c
> > > > +++ b/hw/usb/desc.c
> > > > @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, 
> > > > USBPacket *p,
> > > >   bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
> > > >   const USBDesc *desc = usb_device_get_usb_desc(dev);
> > > >   const USBDescDevice *other_dev;
> > > > -uint8_t buf[256];
> > > > +uint8_t buf[8192];
> > > >   uint8_t type = value >> 8;
> > > >   uint8_t index = value & 0xff;
> > > >   int flags, ret = -1;
> > > 
> > > I think 8K is too large to be allocating as an array on
> > > the stack, so if we need this buffer to be larger we should
> > > switch to some other allocation strategy for it.
> > 
> > IIUC, querying USB device descriptors is not a hot path, so using
> > heap allocation feels sufficient.
> > 
> Yes, I tested this a lot, and found that it's an unlikely code path:
> 1, during guest startup, guest tries to probe device.
> 2, run 'lsusb' command in guest(or other similar commands).
> 
> The original patch and context link:
> https://patchwork.kernel.org/project/qemu-devel/patch/20211227142734.691900-5-pizhen...@bytedance.com/

Yes, the orignal patch is better I think.



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 4/5] usb: allow max 8192 bytes for desc

2022-01-11 Thread Daniel P . Berrangé
On Mon, Dec 27, 2021 at 10:27:33PM +0800, zhenwei pi wrote:
> A device of USB video class usually uses larger desc structure, so
> use larger buffer to avoid failure.
> 
> Signed-off-by: zhenwei pi 
> ---
>  hw/usb/desc.c | 15 ---
>  hw/usb/desc.h |  1 +
>  2 files changed, 9 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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




Re: [PATCH] usb: allow max 8192 bytes for desc

2022-01-11 Thread Daniel P . Berrangé
On Tue, Jan 11, 2022 at 12:21:42PM +, Peter Maydell wrote:
> On Tue, 11 Jan 2022 at 10:54, zhenwei pi  wrote:
> >
> > A device of USB video class usually uses larger desc structure, so
> > use larger buffer to avoid failure. (dev-video.c is ready)
> >
> > Allocating memory dynamically by g_malloc of the orignal version of
> > this change, Philippe suggested just using the stack. Test the two
> > versions of qemu binary, the size of stack gets no change.
> >
> > CC: Philippe Mathieu-Daudé 
> > Signed-off-by: zhenwei pi 
> > ---
> >  hw/usb/desc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/usb/desc.c b/hw/usb/desc.c
> > index 8b6eaea407..57d2aedba1 100644
> > --- a/hw/usb/desc.c
> > +++ b/hw/usb/desc.c
> > @@ -632,7 +632,7 @@ int usb_desc_get_descriptor(USBDevice *dev, USBPacket 
> > *p,
> >  bool msos = (dev->flags & (1 << USB_DEV_FLAG_MSOS_DESC_IN_USE));
> >  const USBDesc *desc = usb_device_get_usb_desc(dev);
> >  const USBDescDevice *other_dev;
> > -uint8_t buf[256];
> > +uint8_t buf[8192];
> >  uint8_t type = value >> 8;
> >  uint8_t index = value & 0xff;
> >  int flags, ret = -1;
> 
> I think 8K is too large to be allocating as an array on
> the stack, so if we need this buffer to be larger we should
> switch to some other allocation strategy for it.

IIUC, querying USB device descriptors is not a hot path, so using
heap allocation feels sufficient.


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: "Startup" meeting (was Re: Meeting today?)

2022-01-11 Thread Daniel P . Berrangé
On Tue, Jan 11, 2022 at 11:20:54AM +0100, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> Just checking in, this call is scheduled for today, 3pm CEST, right?
> 
> Here is the KVM call calendar:
> https://www.google.com/calendar/embed?src=dG9iMXRqcXAzN3Y4ZXZwNzRoMHE4a3BqcXNAZ3JvdXAuY2FsZW5kYXIuZ29vZ2xlLmNvbQ

Hmm, no dial-in details in that calender, so where are we
holding the call.

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: [RFC PATCH v2 06/44] hw/i386: Introduce kvm-type for TDX guest

2022-01-10 Thread Daniel P . Berrangé
On Mon, Jan 10, 2022 at 08:01:33PM +0800, Xiaoyao Li wrote:
> On 1/10/2022 7:18 PM, Daniel P. Berrangé wrote:
> > On Wed, Nov 24, 2021 at 03:31:13PM +0800, Xiaoyao Li wrote:
> > > On 8/26/2021 6:22 PM, Gerd Hoffmann wrote:
> > > > On Wed, Jul 07, 2021 at 05:54:36PM -0700, isaku.yamah...@gmail.com 
> > > > wrote:
> > > > > From: Xiaoyao Li 
> > > > > 
> > > > > Introduce a machine property, kvm-type, to allow the user to create a
> > > > > Trusted Domain eXtensions (TDX) VM, a.k.a. a Trusted Domain (TD), 
> > > > > e.g.:
> > > > > 
> > > > ># $QEMU \
> > > > >   -machine ...,kvm-type=tdx \
> > > > >   ...
> > > 
> > > Sorry for the very late reply.
> > > 
> > > > Can we align sev and tdx better than that?
> > > > 
> > > > SEV is enabled this way:
> > > > 
> > > > qemu -machine ...,confidential-guest-support=sev0 \
> > > >-object sev-guest,id=sev0,...
> > > > 
> > > > (see docs/amd-memory-encryption.txt for details).
> > > > 
> > > > tdx could likewise use a tdx-guest object (and both sev-guest and
> > > > tdx-guest should probably have a common parent object type) to enable
> > > > and configure tdx support.
> > > 
> > > yes, sev only introduced a new object and passed it to
> > > confidential-guest-support. This is because SEV doesn't require the new 
> > > type
> > > of VM.
> > > However, TDX does require a new type of VM.
> > > 
> > > If we read KVM code, there is a parameter of CREATE_VM to pass the 
> > > vm_type,
> > > though x86 doesn't use this field so far. On QEMU side, it also has the
> > > codes to pass/configure vm-type in command line. Of cousre, x86 arch 
> > > doesn't
> > > implement it. With upcoming TDX, it will implement and use vm type for 
> > > TDX.
> > > That's the reason we wrote this patch to implement kvm-type for x86, 
> > > similar
> > > to other arches.
> > > 
> > > yes, of course we can infer the vm_type from "-object tdx-guest". But I
> > > prefer to just use vm_type. Let's see others opinion.
> > 
> > It isn't just SEV that is using the confidential-guest-support approach.
> > This was done for PPC64 and S390x too.  This gives QEMU a standard
> > internal interface to declare that a confidential guest is being used /
> > configured. IMHO, TDX needs to use this too, unless there's a compelling
> > technical reason why it is a bad approach & needs to diverge from every
> > other confidential guest impl in QEMU.
> > 
> 
> Forgot to tell the update that we went the direction to identify the TDX
> vm_type based on confidential-guest_support like below:
> 
> 
> if (ms->cgs && object_dynamic_cast(OBJECT(ms->cgs), TYPE_TDX_GUEST)) {
> kvm_type = KVM_X86_TDX_VM;
> } else {
> kvm_type = KVM_X86_DEFAULT_VM;
> }
> 
> 
> I think it's what you want, right?

Yes, that seems reasonable

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] docs: Add measurement calculation details to amd-memory-encryption.txt

2022-01-10 Thread Daniel P . Berrangé
On Mon, Jan 10, 2022 at 01:17:02PM +0200, Dov Murik wrote:
> 
> 
> On 07/01/2022 22:18, Daniel P. Berrangé wrote:
> > On Thu, Dec 16, 2021 at 11:41:27PM +0200, Dov Murik wrote:
> >>
> >>
> >> On 16/12/2021 18:09, Daniel P. Berrangé wrote:
> >>> On Thu, Dec 16, 2021 at 12:38:34PM +0200, Dov Murik wrote:
> >>>>
> >>>>
> >>>> On 14/12/2021 20:39, Daniel P. Berrangé wrote:
> >>>>> Is there any practical guidance we can give apps on the way the VMSAs
> >>>>> can be expected to be initialized ? eg can they assume essentially
> >>>>> all fields in vmcb_save_area are 0 initialized except for certain
> >>>>> ones ? Is initialization likely to vary at all across KVM or EDK2
> >>>>> vesions or something ?
> >>>>
> >>>> From my own experience, the VMSA of vcpu0 doesn't change; it is 
> >>>> basically what QEMU
> >>>> sets up in x86_cpu_reset() (which is mostly zeros but not all).  I don't 
> >>>> know if it
> >>>> may change in newer QEMU (machine types?) or kvm.  As for vcpu1+, in 
> >>>> SEV-ES the
> >>>> CS:EIP for the APs is taken from a GUIDed table at the end of the OVMF 
> >>>> image, and has
> >>>> actually changed a few months ago when the memory layout changed to 
> >>>> support both TDX
> >>>> and SEV.
> >>>
> >>> That is an unplesantly large number of moving parts that could
> >>> potentially impact the expected state :-(  I think we need to
> >>> be careful to avoid gratuitous changes, to avoid creating a
> >>> combinatorial expansion in the number of possibly valid VMSA
> >>> blocks.
> >>>
> >>> It makes me wonder if we need to think about defining some
> >>> standard approach for distro vendors (and/or cloud vendors)
> >>> to publish the expected contents for various combinations
> >>> of their software pieces.
> >>>
> >>>>
> >>>>
> >>>> Here are the VMSAs for my 2-vcpu SEV-ES VM:
> >>>>
> >>>>
> >>>> $ hd vmsa/vmsa_cpu0.bin
> >>>
> >>> ...snipp...
> >>>
> >>> was there a nice approach / tool you used to capture
> >>> this initial state ?
> >>>
> >>
> >> I wouldn't qualify this as nice: I ended up modifying my
> >> host kernel's kvm (see patch below).  Later I wrote a
> >> script to parse that hex dump from the kernel log into
> >> proper 4096-byte binary VMSA files.
> >>
> >>
> >>
> >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> >> index 7fbce342eec4..4e45fe37b93d 100644
> >> --- a/arch/x86/kvm/svm/sev.c
> >> +++ b/arch/x86/kvm/svm/sev.c
> >> @@ -624,6 +624,12 @@ static int sev_launch_update_vmsa(struct kvm *kvm, 
> >> struct kvm_sev_cmd *argp)
> >>  */
> >> clflush_cache_range(svm->vmsa, PAGE_SIZE);
> >>
> >> +/* dubek */
> >> +pr_info("DEBUG_VMSA - cpu %d START ---\n", i);
> >> +print_hex_dump(KERN_INFO, "DEBUG_VMSA", 
> >> DUMP_PREFIX_OFFSET, 16, 1, svm->vmsa, PAGE_SIZE, true);
> >> +pr_info("DEBUG_VMSA - cpu %d END ---\n", i);
> >> +/* - */
> >> +
> >> vmsa.handle = sev->handle;
> >> vmsa.address = __sme_pa(svm->vmsa);
> >> vmsa.len = PAGE_SIZE;
> > 
> > FWIW, I made a 1% less hacky solution by writing a systemtap
> > script. It will require changing to set the line number for
> > every single kernel version, but at least it doesn't require
> > building a custom kernel
> 
> Thanks, we'll check it out.  It does require a kernel compiled with
> debug info (I assume) to be able to hook the exact line number.

On RHEL / Fedora, you should merely need to install the corresponding
-debuginfo RPM to match your running kernel.

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: [RFC PATCH v2 06/44] hw/i386: Introduce kvm-type for TDX guest

2022-01-10 Thread Daniel P . Berrangé
On Wed, Nov 24, 2021 at 03:31:13PM +0800, Xiaoyao Li wrote:
> On 8/26/2021 6:22 PM, Gerd Hoffmann wrote:
> > On Wed, Jul 07, 2021 at 05:54:36PM -0700, isaku.yamah...@gmail.com wrote:
> > > From: Xiaoyao Li 
> > > 
> > > Introduce a machine property, kvm-type, to allow the user to create a
> > > Trusted Domain eXtensions (TDX) VM, a.k.a. a Trusted Domain (TD), e.g.:
> > > 
> > >   # $QEMU \
> > >   -machine ...,kvm-type=tdx \
> > >   ...
> 
> Sorry for the very late reply.
> 
> > Can we align sev and tdx better than that?
> > 
> > SEV is enabled this way:
> > 
> > qemu -machine ...,confidential-guest-support=sev0 \
> >   -object sev-guest,id=sev0,...
> > 
> > (see docs/amd-memory-encryption.txt for details).
> > 
> > tdx could likewise use a tdx-guest object (and both sev-guest and
> > tdx-guest should probably have a common parent object type) to enable
> > and configure tdx support.
> 
> yes, sev only introduced a new object and passed it to
> confidential-guest-support. This is because SEV doesn't require the new type
> of VM.
> However, TDX does require a new type of VM.
> 
> If we read KVM code, there is a parameter of CREATE_VM to pass the vm_type,
> though x86 doesn't use this field so far. On QEMU side, it also has the
> codes to pass/configure vm-type in command line. Of cousre, x86 arch doesn't
> implement it. With upcoming TDX, it will implement and use vm type for TDX.
> That's the reason we wrote this patch to implement kvm-type for x86, similar
> to other arches.
> 
> yes, of course we can infer the vm_type from "-object tdx-guest". But I
> prefer to just use vm_type. Let's see others opinion.

It isn't just SEV that is using the confidential-guest-support approach.
This was done for PPC64 and S390x too.  This gives QEMU a standard
internal interface to declare that a confidential guest is being used /
configured. IMHO, TDX needs to use this too, unless there's a compelling
technical reason why it is a bad approach & needs to diverge from every
other confidential guest impl in QEMU.

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 v2 6/6] gitlab-ci: Support macOS 12 via cirrus-run

2022-01-10 Thread Daniel P . Berrangé
On Sun, Jan 09, 2022 at 06:06:12PM +0100, Philippe Mathieu-Daudé wrote:
> Add support for macOS 12 build on Cirrus-CI, similarly to commit
> 0e103a65ba1 ("gitlab: support for ... macOS 11 via cirrus-run").
> 
> Disable deprecation warnings on Objective C to avoid:
> 
>   [2789/6622] Compiling Objective-C object libcommon.fa.p/ui_cocoa.m.o
>   ui/cocoa.m:1411:16: error: 'setAllowedFileTypes:' is deprecated: first 
> deprecated in macOS 12.0 - Use -allowedContentTypes instead 
> [-Werror,-Wdeprecated-declarations]
>   [openPanel setAllowedFileTypes: supportedImageFileTypes];
>  ^
>   
> /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSSavePanel.h:215:49:
>  note: property 'allowedFileTypes' is declared deprecated here
>   @property (nullable, copy) NSArray *allowedFileTypes 
> API_DEPRECATED("Use -allowedContentTypes instead", macos(10.3,12.0));
>   ^
>   
> /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSSavePanel.h:215:49:
>  note: 'setAllowedFileTypes:' has been explicitly marked deprecated here
>   FAILED: libcommon.fa.p/ui_cocoa.m.o
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Generated using lcitool from:
> https://gitlab.com/libvirt/libvirt-ci/-/merge_requests/210
> ---
>  .gitlab-ci.d/cirrus.yml   | 16 
>  .gitlab-ci.d/cirrus/macos-12.vars | 16 
>  2 files changed, 32 insertions(+)
>  create mode 100644 .gitlab-ci.d/cirrus/macos-12.vars

There's going to be a minor interaction with my patches that integrate
lcitool more formally in QEMU. Alex has them queued here:

  https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg00558.html

could you rebase on top of that series - all it will really mean
in this case is including the git submodule update on top, and
updating the refresh script to include macos-12 too

 https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg00571.html

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] docs: Add measurement calculation details to amd-memory-encryption.txt

2022-01-07 Thread Daniel P . Berrangé
On Thu, Dec 16, 2021 at 11:41:27PM +0200, Dov Murik wrote:
> 
> 
> On 16/12/2021 18:09, Daniel P. Berrangé wrote:
> > On Thu, Dec 16, 2021 at 12:38:34PM +0200, Dov Murik wrote:
> >>
> >>
> >> On 14/12/2021 20:39, Daniel P. Berrangé wrote:
> >>> Is there any practical guidance we can give apps on the way the VMSAs
> >>> can be expected to be initialized ? eg can they assume essentially
> >>> all fields in vmcb_save_area are 0 initialized except for certain
> >>> ones ? Is initialization likely to vary at all across KVM or EDK2
> >>> vesions or something ?
> >>
> >> From my own experience, the VMSA of vcpu0 doesn't change; it is basically 
> >> what QEMU
> >> sets up in x86_cpu_reset() (which is mostly zeros but not all).  I don't 
> >> know if it
> >> may change in newer QEMU (machine types?) or kvm.  As for vcpu1+, in 
> >> SEV-ES the
> >> CS:EIP for the APs is taken from a GUIDed table at the end of the OVMF 
> >> image, and has
> >> actually changed a few months ago when the memory layout changed to 
> >> support both TDX
> >> and SEV.
> > 
> > That is an unplesantly large number of moving parts that could
> > potentially impact the expected state :-(  I think we need to
> > be careful to avoid gratuitous changes, to avoid creating a
> > combinatorial expansion in the number of possibly valid VMSA
> > blocks.
> > 
> > It makes me wonder if we need to think about defining some
> > standard approach for distro vendors (and/or cloud vendors)
> > to publish the expected contents for various combinations
> > of their software pieces.
> > 
> >>
> >>
> >> Here are the VMSAs for my 2-vcpu SEV-ES VM:
> >>
> >>
> >> $ hd vmsa/vmsa_cpu0.bin
> > 
> > ...snipp...
> > 
> > was there a nice approach / tool you used to capture
> > this initial state ?
> > 
> 
> I wouldn't qualify this as nice: I ended up modifying my
> host kernel's kvm (see patch below).  Later I wrote a
> script to parse that hex dump from the kernel log into
> proper 4096-byte binary VMSA files.
> 
> 
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 7fbce342eec4..4e45fe37b93d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -624,6 +624,12 @@ static int sev_launch_update_vmsa(struct kvm *kvm, 
> struct kvm_sev_cmd *argp)
>  */
> clflush_cache_range(svm->vmsa, PAGE_SIZE);
> 
> +/* dubek */
> +pr_info("DEBUG_VMSA - cpu %d START ---\n", i);
> +print_hex_dump(KERN_INFO, "DEBUG_VMSA", DUMP_PREFIX_OFFSET, 
> 16, 1, svm->vmsa, PAGE_SIZE, true);
> +pr_info("DEBUG_VMSA - cpu %d END ---\n", i);
> +/* - */
> +
> vmsa.handle = sev->handle;
> vmsa.address = __sme_pa(svm->vmsa);
> vmsa.len = PAGE_SIZE;

FWIW, I made a 1% less hacky solution by writing a systemtap
script. It will require changing to set the line number for
every single kernel version, but at least it doesn't require
building a custom kernel

$ cat sev-vmsa.stp 
function dump_vmsa(addr:long) {
  printf("VMSA\n")
  for (i = 0; i < 4096 ; i+= 64) {
printf("%.64M\n", addr + i);
  }
}

probe 
module("kvm_amd").statement("__sev_launch_update_vmsa@arch/x86/kvm/svm/sev.c:618")
 {
  dump_vmsa($svm->vmsa)
}

the line number is that of the 'vmsa.handle = sev->handle' assignment

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: [Qemu-devel] [PATCH] configure: Add pkg-config handling for libgcrypt

2022-01-07 Thread Daniel P . Berrangé
On Fri, Jan 07, 2022 at 12:55:42PM +0100, Thomas Huth wrote:
> On 07/01/2022 12.43, Andrea Bolognani wrote:
> > On Thu, Aug 29, 2019 at 10:15:05AM +0100, Daniel P. Berrangé wrote:
> > > On Thu, Aug 29, 2019 at 04:53:02PM +0800, zhe...@windriver.com wrote:
> > > > libgcrypt may also be controlled by pkg-config, this patch adds 
> > > > pkg-config
> > > > handling for libgcrypt.
> > > 
> > > Where are you seeing pkg-config files for libgcrypt ?
> > > 
> > > The upstream project has (frustratingly) been hostile to any proposal to
> > > add pkg-config support saying people should stick with their custom
> > > libgcrypt-config tool
> > > 
> > > https://dev.gnupg.org/T2037
> > > 
> > > Even if this is something added by some distro downstream, what is the
> > > benefit in using it, compared with libgcrypt-confg which should already
> > > work & is portable.
> > 
> > Resurrecting an old thread to point out that the upstream stance
> > seems to have changed since that discussion:
> > 
> >
> > https://git.gnupg.org/cgi-bin/gitweb.cgi?p=libgcrypt.git;a=commit;h=97194b422bc89a6137f4e218d4cdee118c63e96e
> > 
> > libgcrypt 1.9.0, released almost exactly a year ago, comes with a
> > pkg-config file out of the box. With that in mind, I think it would
> > make sense to re-evaluate this patch for inclusion.
> 
> Maybe ... but we switched to Meson in between, so the patch needs to be
> rewritten to use meson.build instead, I think. Also it seems like version
> 1.9 is not available in all distros yet, so someone needs to do an
> assessment whether the distros that use an older version of libgrypt provide
> a .pc file or not...

Comes back to my question of what is the benefit of using the .pc file
when what we have already works and is known to be portable.

When using meson there is essentially zero burden to using the
libgcrypt-config approach, because that's all handled transparently
by meson.  This is different from the situation with configure,
where libgcrypt-config required extra work on our side.

Overall I don't see any need to change it.

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: "Startup" meeting (was Re: Meeting today?)

2022-01-06 Thread Daniel P . Berrangé
No one objected, so I think we can go for the 11th.

On Thu, Jan 06, 2022 at 12:21:56PM +0100, Mark Burton wrote:
> Can we confirm the 11th for this meeting?
> 
> Cheers
> Mark.
> 
> 
> > On 4 Jan 2022, at 10:29, Edgar E. Iglesias  wrote:
> > 
> > 
> > 
> > On Tue, Dec 14, 2021 at 3:49 PM Markus Armbruster  > <mailto:arm...@redhat.com>> wrote:
> > Daniel P. Berrangé mailto:berra...@redhat.com>> 
> > writes:
> > 
> > > On Tue, Dec 14, 2021 at 12:37:43PM +0100, Markus Armbruster wrote:
> > >> Mark Burton  > >> <mailto:mark.bur...@greensocs.com>> writes:
> > >> 
> > >> > I realise it’s very short notice, but what about having a discussion 
> > >> > today at 15:00 ?
> > >> 
> > >> I have a conflict today.  I could try to reschedule, but I'd prefer to
> > >> talk next week instead.  Less stress, better prep.
> > >
> > > I fear we've run out of time for this year if we want all interested
> > > parties to be able to attend.  I'll be off on PTO from end of this
> > > week until the new year, and I know alot of folk are doing similar.
> > 
> > Right.  I'll be off from Dec 23 to Jan 9.  Can we all make Jan 11?
> > 
> > Jan 11th works for me!
> > 
> > Thanks,
> > Edgar
> 

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 1/1] softmmu: fix device deletion events with -device JSON syntax

2022-01-05 Thread Daniel P . Berrangé
On Wed, Jan 05, 2022 at 04:00:54PM +0100, Laurent Vivier wrote:
> On 05/01/2022 15:55, Daniel P. Berrangé wrote:
> > On Wed, Jan 05, 2022 at 03:49:12PM +0100, Laurent Vivier wrote:
> > > On 05/01/2022 13:38, Daniel P. Berrangé wrote:
> > > > The -device JSON syntax impl leaks a reference on the created
> > > > DeviceState instance. As a result when you hot-unplug the
> > > > device, the device_finalize method won't be called and thus
> > > > it will fail to emit the required DEVICE_DELETED event.
> > > > 
> > > > A 'json-cli' feature was previously added against the
> > > > 'device_add' QMP command QAPI schema to indicated to mgmt
> > > > apps that -device supported JSON syntax. Given the hotplug
> > > > bug that feature flag is no unusable for its purpose, so
> > > 
> > > Not sure to understand: do you mean "now unusable"?
> > 
> > An application wants to known whether QEMU can support JSON
> > syntax with -device. If they look for the 'json-cli' feature
> > as a witness, they'll end up using JSON with QEMU 6.2 which
> > is giving them broken hotplug. This is unusable for any
> > non-trivial use cases. So we need a new witness to indicate
> > whether JSON is viable with -device, that only the newly
> > fixed QEMU will report.
> 
> I understand that, my problem was with your sentence:
> 
> "Given the hotplug bug that feature flag is no unusable for its purpose"

What's the problem with that ? It is reasonabled to say a -device impl
which is broken for hotplug is not usable for non-toy use cases.

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 1/1] softmmu: fix device deletion events with -device JSON syntax

2022-01-05 Thread Daniel P . Berrangé
On Wed, Jan 05, 2022 at 03:49:12PM +0100, Laurent Vivier wrote:
> On 05/01/2022 13:38, Daniel P. Berrangé wrote:
> > The -device JSON syntax impl leaks a reference on the created
> > DeviceState instance. As a result when you hot-unplug the
> > device, the device_finalize method won't be called and thus
> > it will fail to emit the required DEVICE_DELETED event.
> > 
> > A 'json-cli' feature was previously added against the
> > 'device_add' QMP command QAPI schema to indicated to mgmt
> > apps that -device supported JSON syntax. Given the hotplug
> > bug that feature flag is no unusable for its purpose, so
> 
> Not sure to understand: do you mean "now unusable"?

An application wants to known whether QEMU can support JSON
syntax with -device. If they look for the 'json-cli' feature
as a witness, they'll end up using JSON with QEMU 6.2 which
is giving them broken hotplug. This is unusable for any
non-trivial use cases. So we need a new witness to indicate
whether JSON is viable with -device, that only the newly
fixed QEMU will report.

> 
> > we add a new 'json-cli-hotplug' feature to indicate the
> > -device supports JSON without breaking hotplug.
> > 
> > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/802
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >   qapi/qdev.json |  5 -
> >   softmmu/vl.c   |  4 +++-
> >   tests/qtest/device-plug-test.c | 19 +++
> >   3 files changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qapi/qdev.json b/qapi/qdev.json
> > index 69656b14df..26cd10106b 100644
> > --- a/qapi/qdev.json
> > +++ b/qapi/qdev.json
> > @@ -44,6 +44,9 @@
> >   # @json-cli: If present, the "-device" command line option supports JSON
> >   #syntax with a structure identical to the arguments of this
> >   #command.
> > +# @json-cli-hotplug: If present, the "-device" command line option 
> > supports JSON
> > +#syntax without the reference counting leak that broke
> > +#hot-unplug
> >   #
> >   # Notes:
> >   #
> > @@ -74,7 +77,7 @@
> >   { 'command': 'device_add',
> > 'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
> > 'gen': false, # so we can get the additional arguments
> > -  'features': ['json-cli'] }
> > +  'features': ['json-cli', 'json-cli-hotplug'] }
> >   ##
> >   # @device_del:
> > diff --git a/softmmu/vl.c b/softmmu/vl.c
> > index d9e4c619d3..b1fc7da104 100644
> > --- a/softmmu/vl.c
> > +++ b/softmmu/vl.c
> > @@ -2684,6 +2684,7 @@ static void qemu_create_cli_devices(void)
> >   qemu_opts_foreach(qemu_find_opts("device"),
> > device_init_func, NULL, _fatal);
> >   QTAILQ_FOREACH(opt, _opts, next) {
> > +DeviceState *dev;
> >   loc_push_restore(>loc);
> >   /*
> >* TODO Eventually we should call qmp_device_add() here to make 
> > sure it
> > @@ -2692,7 +2693,8 @@ static void qemu_create_cli_devices(void)
> >* from the start, so call qdev_device_add_from_qdict() directly 
> > for
> >* now.
> >*/
> > -qdev_device_add_from_qdict(opt->opts, true, _fatal);
> > +dev = qdev_device_add_from_qdict(opt->opts, true, _fatal);
> > +object_unref(OBJECT(dev));
> >   loc_pop(>loc);
> >   }
> >   rom_reset_order_override();
> > diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
> > index 559d47727a..ad79bd4c14 100644
> > --- a/tests/qtest/device-plug-test.c
> > +++ b/tests/qtest/device-plug-test.c
> > @@ -77,6 +77,23 @@ static void test_pci_unplug_request(void)
> >   qtest_quit(qtest);
> >   }
> > +static void test_pci_unplug_json_request(void)
> > +{
> > +QTestState *qtest = qtest_initf(
> > +"-device '{\"driver\": \"virtio-mouse-pci\", \"id\": \"dev0\"}'");
> > +
> > +/*
> > + * Request device removal. As the guest is not running, the request 
> > won't
> > + * be processed. However during system reset, the removal will be
> > + * handled, removing the device.
> > + */
> > +device_del(qtest, "dev0");
> > +system_reset(qtest);
> 
> You can use qpci_unplug_acpi_device_test() to process the request... but I
> see this is done like that too in test_pci_unplug_request()
> 
> > +wait_device_dele

Re: [PATCH v1 20/34] tests/docker: add libfuse3 development headers

2022-01-05 Thread Daniel P . Berrangé
On Wed, Jan 05, 2022 at 02:26:55PM +, Richard W.M. Jones wrote:
> On Wed, Jan 05, 2022 at 01:49:55PM +, Alex Bennée wrote:
> > From: Stefan Hajnoczi 
> > 
> > The FUSE exports feature is not built because most container images do
> > not have libfuse3 development headers installed. Add the necessary
> > packages to the Dockerfiles.
> > 
> > Cc: Hanna Reitz 
> > Cc: Richard W.M. Jones 
> > Signed-off-by: Stefan Hajnoczi 
> > Acked-by: Richard W.M. Jones 
> > Reviewed-by: Beraldo Leal 
> > Tested-by: Beraldo Leal 
> > Message-Id: <20211207160025.52466-1-stefa...@redhat.com>
> > [AJB: migrate to lcitool qemu.yml and regenerate]
> > Signed-off-by: Alex Bennée 
> 
> 
> I checked all the package names and they look good, so:

FYI, the libvirt-ci  CI pipelines validate that all package
names are correct by creating a job for each distro and
checking that every package known to libvirt-ci can be
installed. This catches issues where distros rename or
drop packages. These are some of the reasons why we'll
benefit from using libvirt-ci / lcitool for auto-generating
these dockerfiles in QEMU.

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 :|




[PATCH 0/1] Fix -device JSON support wrt hotplug

2022-01-05 Thread Daniel P . Berrangé
Libvirt switched to using -device JSON support, but we discovered in
testing that it is broken for hotplug, never sending DEVICE_DELETED
events. This is caused by a subtle refcount leak.

Daniel P. Berrangé (1):
  softmmu: fix device deletion events with -device JSON syntax

 qapi/qdev.json |  5 -
 softmmu/vl.c   |  4 +++-
 tests/qtest/device-plug-test.c | 19 +++
 3 files changed, 26 insertions(+), 2 deletions(-)

-- 
2.33.1





[PATCH 1/1] softmmu: fix device deletion events with -device JSON syntax

2022-01-05 Thread Daniel P . Berrangé
The -device JSON syntax impl leaks a reference on the created
DeviceState instance. As a result when you hot-unplug the
device, the device_finalize method won't be called and thus
it will fail to emit the required DEVICE_DELETED event.

A 'json-cli' feature was previously added against the
'device_add' QMP command QAPI schema to indicated to mgmt
apps that -device supported JSON syntax. Given the hotplug
bug that feature flag is no unusable for its purpose, so
we add a new 'json-cli-hotplug' feature to indicate the
-device supports JSON without breaking hotplug.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/802
Signed-off-by: Daniel P. Berrangé 
---
 qapi/qdev.json |  5 -
 softmmu/vl.c   |  4 +++-
 tests/qtest/device-plug-test.c | 19 +++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 69656b14df..26cd10106b 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -44,6 +44,9 @@
 # @json-cli: If present, the "-device" command line option supports JSON
 #syntax with a structure identical to the arguments of this
 #command.
+# @json-cli-hotplug: If present, the "-device" command line option supports 
JSON
+#syntax without the reference counting leak that broke
+#hot-unplug
 #
 # Notes:
 #
@@ -74,7 +77,7 @@
 { 'command': 'device_add',
   'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
   'gen': false, # so we can get the additional arguments
-  'features': ['json-cli'] }
+  'features': ['json-cli', 'json-cli-hotplug'] }
 
 ##
 # @device_del:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index d9e4c619d3..b1fc7da104 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2684,6 +2684,7 @@ static void qemu_create_cli_devices(void)
 qemu_opts_foreach(qemu_find_opts("device"),
   device_init_func, NULL, _fatal);
 QTAILQ_FOREACH(opt, _opts, next) {
+DeviceState *dev;
 loc_push_restore(>loc);
 /*
  * TODO Eventually we should call qmp_device_add() here to make sure it
@@ -2692,7 +2693,8 @@ static void qemu_create_cli_devices(void)
  * from the start, so call qdev_device_add_from_qdict() directly for
  * now.
  */
-qdev_device_add_from_qdict(opt->opts, true, _fatal);
+dev = qdev_device_add_from_qdict(opt->opts, true, _fatal);
+object_unref(OBJECT(dev));
 loc_pop(>loc);
 }
 rom_reset_order_override();
diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
index 559d47727a..ad79bd4c14 100644
--- a/tests/qtest/device-plug-test.c
+++ b/tests/qtest/device-plug-test.c
@@ -77,6 +77,23 @@ static void test_pci_unplug_request(void)
 qtest_quit(qtest);
 }
 
+static void test_pci_unplug_json_request(void)
+{
+QTestState *qtest = qtest_initf(
+"-device '{\"driver\": \"virtio-mouse-pci\", \"id\": \"dev0\"}'");
+
+/*
+ * Request device removal. As the guest is not running, the request won't
+ * be processed. However during system reset, the removal will be
+ * handled, removing the device.
+ */
+device_del(qtest, "dev0");
+system_reset(qtest);
+wait_device_deleted_event(qtest, "dev0");
+
+qtest_quit(qtest);
+}
+
 static void test_ccw_unplug(void)
 {
 QTestState *qtest = qtest_initf("-device virtio-balloon-ccw,id=dev0");
@@ -145,6 +162,8 @@ int main(int argc, char **argv)
  */
 qtest_add_func("/device-plug/pci-unplug-request",
test_pci_unplug_request);
+qtest_add_func("/device-plug/pci-unplug-json-request",
+   test_pci_unplug_json_request);
 
 if (!strcmp(arch, "s390x")) {
 qtest_add_func("/device-plug/ccw-unplug",
-- 
2.33.1




Re: [PATCH v2] FreeBSD: Upgrade to 12.3 release

2022-01-05 Thread Daniel P . Berrangé
On Tue, Jan 04, 2022 at 09:28:16PM -0500, Brad Smith wrote:
> FreeBSD: Upgrade to 12.3 release
> 
> Note, since libtasn1 was fixed in 12.3 [*], this commit re-enables GnuTLS.
> 
> [*] https://gitlab.com/gnutls/libtasn1/-/merge_requests/71
> 
> 
> Signed-off-by: Brad Smith 
> Tested-by: Thomas Huth 
> Reviewed-by: Warner Losh 
> ---
>  .gitlab-ci.d/cirrus.yml | 5 +
>  tests/vm/freebsd| 8 +++-
>  2 files changed, 4 insertions(+), 9 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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




Re: [PATCH 2/2] [crypto] Only verify CA certs in chain of trust

2022-01-04 Thread Daniel P . Berrangé
On Wed, Dec 22, 2021 at 03:06:00PM +, Henry Kleynhans wrote:
> From: Henry Kleynhans 
> 
> The CA file provided to qemu may contain CA certificates which do not
> form part of the chain of trust for the specific certificate we are
> sanity checking.
> 
> This patch changes the sanity checking from validating every CA
> certificate to only checking the CA certificates which are part of the
> chain of trust (issuer chain).  Other certificates are ignored.
> 
> Signed-off-by: Henry Kleynhans 
> ---
>  crypto/tlscredsx509.c | 50 +++
>  tests/unit/test-crypto-tlscredsx509.c | 25 +-
>  2 files changed, 68 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé 

> 
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index d061c68253..841f80aac6 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -315,6 +315,44 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds,
>  return 0;
>  }
>  
> +static int
> +qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
> +gnutls_x509_crt_t cert,
> +gnutls_x509_crt_t *cacerts,
> +unsigned int ncacerts,
> +const char *cacertFile,
> +bool isServer,
> +bool isCA,
> +Error **errp)
> +{
> +gnutls_x509_crt_t *cert_to_check = 
> +int checking_issuer = 1;
> +int retval = 0;
> +
> +while (checking_issuer) {
> +checking_issuer = 0;
> +
> +if (gnutls_x509_crt_check_issuer(*cert_to_check, *cert_to_check)) {
> +/* The cert is self-signed indicating we have reached the root 
> of trust. */
> +return qcrypto_tls_creds_check_cert(creds, *cert_to_check, 
> cacertFile,
> +isServer, isCA, errp);
> +}
> +for (int i = 0; i < ncacerts; i++) {
> +if (gnutls_x509_crt_check_issuer(*cert_to_check, cacerts[i])) {
> +retval = qcrypto_tls_creds_check_cert(creds, cacerts[i], 
> cacertFile,
> +  isServer, isCA, errp);
> +if (retval < 0) {
> +return retval;
> +}
> +cert_to_check = [i];
> +checking_issuer = 1;
> +break;
> +}
> +}
> +}
> +
> +return -1;

I have a feeling this should be 'return 0'.  eg consider the case
where the cacert.pem does not contain the issuer of clientcert.pem.
we will only do one iteration of the while(checking_issuer) loop,
not hitting any of the 'return' statements. This is ok, so should
report success I think.

> +}
>  
>  static int
>  qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
> @@ -500,12 +538,12 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 
> *creds,
>  goto cleanup;
>  }
>  
> -for (i = 0; i < ncacerts; i++) {
> -if (qcrypto_tls_creds_check_cert(creds,
> - cacerts[i], cacertFile,
> - isServer, true, errp) < 0) {
> -goto cleanup;
> -}
> +if (cert && 
> +qcrypto_tls_creds_check_authority_chain(creds, cert, 
> +cacerts, ncacerts,
> +cacertFile, isServer,
> +true, errp) < 0) {
> +goto cleanup;
>  }
>  
>  if (cert && ncacerts &&
> diff --git a/tests/unit/test-crypto-tlscredsx509.c 
> b/tests/unit/test-crypto-tlscredsx509.c
> index aab4149b56..e4d657ba61 100644
> --- a/tests/unit/test-crypto-tlscredsx509.c
> +++ b/tests/unit/test-crypto-tlscredsx509.c
> @@ -589,6 +589,12 @@ int main(int argc, char **argv)
>   true, true, GNUTLS_KEY_KEY_CERT_SIGN,
>   false, false, NULL, NULL,
>   0, 0);
> +TLS_CERT_REQ(cacertlevel1creq_invalid, cacertrootreq,
> + "UK", "qemu level 1c invalid", NULL, NULL, NULL, NULL,
> + true, true, true,
> + true, true, GNUTLS_KEY_KEY_CERT_SIGN,
> + false, false, NULL, NULL,
> + 360, 400);
>  TLS_CERT_REQ(cacertlevel2areq, cacertlevel1areq,
>   "UK", "qemu level 2a", NULL, NULL, NULL, NULL,
>

Re: [PATCH 2/2] [crypto] Only verify CA certs in chain of trust

2022-01-04 Thread Daniel P . Berrangé
On Wed, Dec 22, 2021 at 03:54:08PM +, Henry Kleynhans wrote:
> Hi Daniel,
> 
> This patch tightens the CA verification code to only check the
> issuer chain of the client cert.  I think this will still not
> catch expired/invalid certs if the client and server certs have
> different issuer chains; so maybe this too is not quite the
> correct fix.  Let me know what you think.

Different issuer chains is not going to be very common/typical.
So what you've done in this patch is at least pretty decent for
the common case, so will catch most user's mistakes. Let me have
a think about whether we can do anything better without making
the code too painful


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 1/2] [crypto] Load all certificates in X509 CA file

2022-01-04 Thread Daniel P . Berrangé
On Wed, Dec 22, 2021 at 03:05:59PM +, Henry Kleynhans wrote:
> From: Henry Kleynhans 
> 
> Some CA files may contain multiple intermediaries and roots of trust.
> These may not fit into the hard-coded limit of 16.
> 
> Extend the validation code to allocate enough space to load all of the
> certificates present in the CA file and ensure they are cleaned up.
> 
> Signed-off-by: Henry Kleynhans 
> ---
>  crypto/tlscredsx509.c | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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




Re: [PATCH v5 0/3] tpm: Add missing ACPI device identification objects

2022-01-04 Thread Daniel P . Berrangé
On Tue, Jan 04, 2022 at 12:58:03PM -0500, Stefan Berger wrote:
> This series of patches adds missing ACPI device identification objects _STR
> and _UID to TPM 1.2 and TPM 2 ACPI tables.

What was the practical impact on guests (if any) from these ACPI
objects being missing ?

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] [RESEND] docs: Add spec of OVMF GUIDed table for SEV guests

2022-01-04 Thread Daniel P . Berrangé
On Mon, Jan 03, 2022 at 11:14:13AM +0200, Dov Murik wrote:
> Add docs/specs/sev-guest-firmware.rst which describes the GUIDed table
> in the end of OVMF's image which is parsed by QEMU, and currently used
> to describe some values for SEV and SEV-ES guests.
> 
> Signed-off-by: Dov Murik 
> ---
>  docs/specs/index.rst  |   1 +
>  docs/specs/sev-guest-firmware.rst | 125 ++
>  2 files changed, 126 insertions(+)
>  create mode 100644 docs/specs/sev-guest-firmware.rst

Reviewed-by: Daniel P. Berrangé 


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




Re: [PATCH v2] docs: Add measurement calculation details to amd-memory-encryption.txt

2022-01-04 Thread Daniel P . Berrangé
On Mon, Dec 20, 2021 at 10:42:24AM +, Dov Murik wrote:
> Add a section explaining how the Guest Owner should calculate the
> expected guest launch measurement for SEV and SEV-ES.
> 
> Also update the name and link to the SEV API Spec document.
> 
> Signed-off-by: Dov Murik 
> Suggested-by: Daniel P. Berrangé 
> 
> ---
> 
> v2:
> - Explain that firmware must be built without NVRAM store.
> ---
>  docs/amd-memory-encryption.txt | 52 +++---
>  1 file changed, 48 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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




Re: [PATCH 0/5] Introduce camera subsystem and USB video device

2022-01-04 Thread Daniel P . Berrangé
On Mon, Dec 27, 2021 at 10:27:29PM +0800, zhenwei pi wrote:
> 1, The full picture of this patch set:
>+-+   ++ +---+
>|UVC(done)|   |virtio(TODO)| |other HW device|
>+-+   ++ +---+
>  | | |
>  |++ |
>++camera(done)+-+
>   ++---+
>|
>  +-+-+
>  | | |
>   +--+--+ ++-++--+--+
>   |builtin(done)| |v4l2(done)||other drivers|
>   +-+ +--++-+
> 
> With this patch set, We can run a desktop VM (Ex Ubuntu-2004), several camera
> APPs(cheese, kamoso, guvcview and qcam) work fine.
> 
> Some works still in working:
>   1, hot-plug
>   2, compat with live migration
>   3, several actions defined in UVC SPEC
> 
> Zhenwei Pi (5):
>   camera: Introduce camera subsystem and builtin driver
>   camera: v4l2: Introduce v4l2 camera driver
>   usb: Introduce video
>   usb: allow max 8192 bytes for desc
>   usb-video: Introduce USB video class
> 
>  camera/builtin.c|  717 
>  camera/camera-int.h |   19 +
>  camera/camera.c |  522 +++
>  camera/meson.build  |   20 +
>  camera/trace-events |   28 +
>  camera/trace.h  |1 +
>  camera/v4l2.c   |  637 ++
>  hw/usb/Kconfig  |5 +
>  hw/usb/desc.c   |   15 +-
>  hw/usb/desc.h   |1 +
>  hw/usb/dev-video.c  | 1395 +++
>  hw/usb/meson.build  |1 +
>  hw/usb/trace-events |   11 +
>  include/camera/camera.h |  238 +++
>  include/hw/usb.h|2 +
>  include/hw/usb/video.h  |  303 +
>  meson.build |   20 +-
>  meson_options.txt   |3 +
>  qapi/camera.json|  101 +++
>  qapi/meson.build|1 +
>  qapi/qapi-schema.json   |1 +
>  qemu-options.hx |   13 +
>  softmmu/vl.c|4 +

There's no MAINTAINERS file update here.

As a general rule, if you are introducing an entire new subsystem
into the QEMU codebase, it is expected someone will be nominated
as the maintainer for the new subsystem. Usually the person adding
it will themselves volunteer to be the maintainer.

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 v2 2/3] qapi/ui: introduce change-vnc-listen

2022-01-04 Thread Daniel P . Berrangé
On Wed, Dec 22, 2021 at 08:17:30PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Add command that can change addresses where VNC server listens for new
> connections. Prior to 6.0 this functionality was available through
> 'change' qmp command which was deleted.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Marc-André Lureau 
> ---
>  docs/about/removed-features.rst |  3 ++-
>  qapi/ui.json| 19 +++
>  ui/vnc.c| 26 ++
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
> index d42c3341de..20e6901a82 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -348,7 +348,8 @@ documentation of ``query-hotpluggable-cpus`` for 
> additional details.
>  ``change`` (removed in 6.0)
>  '''
>  
> -Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
> +Use ``blockdev-change-medium`` or ``change-vnc-password`` or
> +``change-vnc-listen`` instead.
>  
>  ``query-events`` (removed in 6.0)
>  '
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 2b4371da37..6a586edff1 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1334,3 +1334,22 @@
>  { 'command': 'display-reload',
>'data': 'DisplayReloadOptions',
>'boxed' : true }
> +
> +##
> +# @change-vnc-listen:
> +#
> +# Change set of addresses to listen for connections.
> +#
> +# @id: vnc display identifier
> +#
> +# @addresses: list of addresses for listen at
> +#
> +# @websockets: list of addresses to listen with websockets
> +#
> +# Since: 7.0
> +#
> +##
> +{ 'command': 'change-vnc-listen',
> +  'data': { 'id': 'str', 'addresses': ['SocketAddress'],
> +'*websockets': ['SocketAddress'] },
> +  'if': 'CONFIG_VNC' }

I replied to your v1 before noticing this v2. So this is just to
point out that I suggested we could use 'display-reload' to update
the address rather than adding a new command.


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 v2 1/5] ps2: Initial horizontal scroll support

2022-01-04 Thread Daniel P . Berrangé
On Wed, Dec 22, 2021 at 02:06:43AM +0100, Dmitry Petrov wrote:
> v2:
>   - Patch is split into a sequence
>   - value is clamped to 31 for horizontal scroll
> 
> This patch introduces horizontal scroll support for the ps/2
> mouse.
> 
> The patch is based on the previous work
> by Brad Jorsch done in 2010
> but never merge, see
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=579968

The bugs attached to that ticket don't have any Signed-off-by
from Brad. Fortunately at the time he did re-submit them on
qemu-devel with Signed-off-by present.

Can you include this link to his patches in your commit
message to get the paper trail

  https://lists.gnu.org/archive/html/qemu-devel/2010-05/msg00223.html

> This change adds support for horizontal scroll to ps/2 mouse device
> code. The code is implemented to match the logic of linux kernel
> which is used as a reference.
> 
> Signed-off-by: Dmitry Petrov 
> ---
>  hw/input/ps2.c | 57 +++---
>  qapi/ui.json   |  2 +-
>  2 files changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index 9376a8f4ce..6236711e1b 100644
> --- a/hw/input/ps2.c
> +++ b/hw/input/ps2.c
> @@ -123,6 +123,7 @@ typedef struct {
>  int mouse_dx; /* current values, needed for 'poll' mode */
>  int mouse_dy;
>  int mouse_dz;
> +int mouse_dw;
>  uint8_t mouse_buttons;
>  } PS2MouseState;
>  
> @@ -715,7 +716,7 @@ static int ps2_mouse_send_packet(PS2MouseState *s)
>  /* IMPS/2 and IMEX send 4 bytes, PS2 sends 3 bytes */
>  const int needed = s->mouse_type ? 4 : 3;
>  unsigned int b;
> -int dx1, dy1, dz1;
> +int dx1, dy1, dz1, dw1;
>  
>  if (PS2_QUEUE_SIZE - s->common.queue.count < needed) {
>  return 0;
> @@ -724,6 +725,7 @@ static int ps2_mouse_send_packet(PS2MouseState *s)
>  dx1 = s->mouse_dx;
>  dy1 = s->mouse_dy;
>  dz1 = s->mouse_dz;
> +dw1 = s->mouse_dw;
>  /* XXX: increase range to 8 bits ? */
>  if (dx1 > 127)
>  dx1 = 127;
> @@ -740,6 +742,9 @@ static int ps2_mouse_send_packet(PS2MouseState *s)
>  /* extra byte for IMPS/2 or IMEX */
>  switch(s->mouse_type) {
>  default:
> +/* Just ignore the wheels if not supported */
> +s->mouse_dz = 0;
> +s->mouse_dw = 0;
>  break;
>  case 3:
>  if (dz1 > 127)
> @@ -747,13 +752,41 @@ static int ps2_mouse_send_packet(PS2MouseState *s)
>  else if (dz1 < -127)
>  dz1 = -127;
>  ps2_queue_noirq(>common, dz1 & 0xff);
> +s->mouse_dz -= dz1;
> +s->mouse_dw = 0;
>  break;
>  case 4:
> -if (dz1 > 7)
> -dz1 = 7;
> -else if (dz1 < -7)
> -dz1 = -7;
> -b = (dz1 & 0x0f) | ((s->mouse_buttons & 0x18) << 1);
> +/*
> + * This matches what the Linux kernel expects for exps/2 in
> + * drivers/input/mouse/psmouse-base.c. Note, if you happen to
> + * press/release the 4th or 5th buttons at the same moment as a
> + * horizontal wheel scroll, those button presses will get lost. I'm 
> not
> + * sure what to do about that, since by this point we don't know
> + * whether those buttons actually changed state.
> + */
> +if (dw1 != 0) {
> +if (dw1 > 31) {
> +dw1 = 31;
> +} else if (dw1 < -31) {
> +dw1 = -31;
> +}
> +
> +/*
> + * linux kernel expects first 6 bits to represent the value
> + * for horizontal scroll
> + */
> +b = (dw1 & 0x3f) | 0x40;
> +s->mouse_dw -= dw1;
> +} else {
> +if (dz1 > 7) {
> +dz1 = 7;
> +} else if (dz1 < -7) {
> +dz1 = -7;
> +}
> +
> +b = (dz1 & 0x0f) | ((s->mouse_buttons & 0x18) << 1);
> +s->mouse_dz -= dz1;
> +}
>  ps2_queue_noirq(>common, b);
>  break;
>  }
> @@ -764,7 +797,6 @@ static int ps2_mouse_send_packet(PS2MouseState *s)
>  /* update deltas */
>  s->mouse_dx -= dx1;
>  s->mouse_dy -= dy1;
> -s->mouse_dz -= dz1;
>  
>  return 1;
>  }
> @@ -806,6 +838,12 @@ static void ps2_mouse_event(DeviceState *dev, 
> QemuConsole *src,
>  } else if (btn->button == INPUT_BUTTON_WHEEL_DOWN) {
>  s->mouse_dz++;
>  }
> +
> +if (btn->button == INPUT_BUTTON_WHEEL_RIGHT) {
> +s->mouse_dw--;
> +} else if (btn->button == INPUT_BUTTON_WHEEL_LEFT) {
> +s->mouse_dw++;
> +}
>  } else {
>  s->mouse_buttons &= ~bmap[btn->button];
>  }
> @@ -833,8 +871,10 @@ static void ps2_mouse_sync(DeviceState *dev)
>  /* if not remote, send event. Multiple events are sent if
> too big deltas */
>  while (ps2_mouse_send_packet(s)) {
> -

Re: [PATCH 2/2] qapi/ui: introduce change-vnc-listen

2022-01-04 Thread Daniel P . Berrangé
On Mon, Dec 20, 2021 at 04:44:18PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Add command that can change addresses where VNC server listens for new
> connections. Prior to 6.0 this functionality was available through
> 'change' qmp command which was deleted.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  docs/about/removed-features.rst |  3 ++-
>  qapi/ui.json| 12 
>  ui/vnc.c| 26 ++
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
> index d42c3341de..20e6901a82 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -348,7 +348,8 @@ documentation of ``query-hotpluggable-cpus`` for 
> additional details.
>  ``change`` (removed in 6.0)
>  '''
>  
> -Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
> +Use ``blockdev-change-medium`` or ``change-vnc-password`` or
> +``change-vnc-listen`` instead.
>  
>  ``query-events`` (removed in 6.0)
>  '
> diff --git a/qapi/ui.json b/qapi/ui.json
> index d7567ac866..14e6fe0b4c 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1304,3 +1304,15 @@
>  { 'command': 'display-reload',
>'data': 'DisplayReloadOptions',
>'boxed' : true }
> +
> +##
> +# @change-vnc-listen:
> +#
> +# Change set of addresses to listen for connections.
> +#
> +# Since: 7.0
> +#
> +##
> +{ 'command': 'change-vnc-listen',
> +  'data': { 'id': 'str', 'addresses': ['SocketAddress'],
> +'*websockets': ['SocketAddress'] } }

We already have a general purpose command above 'display-reload'
for doing live changes to the display backends.

THis should instead be

{ 'struct': 'DisplayReloadOptionsVNC',
  'data': { '*tls-certs': 'bool',
'*addresses': ['SocketAddress'],
'*websockets': ['SocketAddress'] } }

if 'addresses' is non-null then the listener can be updated.


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] vl: Add -set options to device opts dict when using JSON syntax for -device

2022-01-04 Thread Daniel P . Berrangé
On Wed, Dec 22, 2021 at 09:22:47AM +0100, Gerd Hoffmann wrote:
> On Tue, Dec 21, 2021 at 04:40:28PM +0100, Markus Armbruster wrote:
> > Paolo Bonzini  writes:
> > 
> > > On 12/21/21 13:58, Markus Armbruster wrote:
> > >>> Is this a regression?  I suspect commit 5dacda5167 "vl: Enable JSON
> > >>> syntax for -device" (v6.2.0).
> > >> 
> > >> Obviously not a regression: everything that used to work still works.
> > >
> > > FWIW I think -set should be deprecated.  I'm not aware of any
> > > particularly useful use of it.  There are a couple in the QEMU tests
> > > (in vhost-user-test and in qemu-iotests 068), but in both cases the
> > > code would be easier to follow without; patches can be dusted off if
> > > desired.
> > 
> > -set has its uses, but they're kind of obscure.  When you want to use
> > some canned configuration with slight modifications, then -readconfig
> > canned.cfg -set ... is nicer than editing a copy of canned.cfg.
> 
> Simliar: configure stuff not supported by libvirt:
> 
>   
> 
> 
>   
> 
> There will always be a gap between qemu and libvirt, even if most of
> them are temporary only (while developing a new feature).  I think we
> need some way to deal with this kind of tweaks when moving to QAPI-based
> machine setup.  Possibly not in qemu, maybe it's easier to add new
> '' syntax to libvirt.

Yes, I'd suggest we get

   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: [RFC PATCH] docs/devel: more documentation on the use of suffixes

2021-12-17 Thread Daniel P . Berrangé
On Fri, Dec 17, 2021 at 05:19:02PM +, Alex Bennée wrote:
> Using _qemu is a little confusing. Let's use _compat for these sorts
> of things. We should also mention _impl which is another common suffix
> in the code base.
> 
> Signed-off-by: Alex Bennée 
> ---
>  docs/devel/style.rst  | 7 +++
>  include/glib-compat.h | 6 +++---
>  2 files changed, 10 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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




Re: [PATCH v2] hw: Add compat machines for 7.0

2021-12-17 Thread Daniel P . Berrangé
On Fri, Dec 17, 2021 at 03:39:48PM +0100, Cornelia Huck wrote:
> Add 7.0 machine types for arm/i440fx/q35/s390x/spapr.
> 
> Acked-by: Cédric Le Goater 
> Reviewed-by: Juan Quintela 
> Signed-off-by: Cornelia Huck 
> ---
> 
> v1->v2: fix typo in i386 function chaining (thanks danpb!)
> 
> ---
>  hw/arm/virt.c  |  9 -
>  hw/core/machine.c  |  3 +++
>  hw/i386/pc.c   |  3 +++
>  hw/i386/pc_piix.c  | 14 +-
>  hw/i386/pc_q35.c   | 13 -
>  hw/ppc/spapr.c | 15 +--
>  hw/s390x/s390-virtio-ccw.c | 14 +-
>  include/hw/boards.h|  3 +++
>  include/hw/i386/pc.h   |  3 +++
>  9 files changed, 71 insertions(+), 6 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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




Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper

2021-12-17 Thread Daniel P . Berrangé
On Fri, Dec 17, 2021 at 02:53:05PM +, Alex Bennée wrote:
> 
> Daniel P. Berrangé  writes:
> 
> > On Thu, Dec 16, 2021 at 02:11:37PM +, Alex Bennée wrote:
> >> 
> >> Philippe Mathieu-Daudé  writes:
> >> 
> >> > When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
> >> > (Fedora 34 provides GLib 2.68.1) we get:
> >> >
> >> >   hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use 
> >> > 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
> >> >   ...
> >> >
> >> > g_memdup() has been updated by g_memdup2() to fix eventual security
> >> > issues (size argument is 32-bit and could be truncated / wrapping).
> >> > GLib recommends to copy their static inline version of g_memdup2():
> >> > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
> >> >
> >> > Our glib-compat.h provides a comment explaining how to deal with
> >> > these deprecated declarations (see commit e71e8cc0355
> >> > "glib: enforce the minimum required version and warn about old APIs").
> >> >
> >> > Following this comment suggestion, implement the g_memdup2_qemu()
> >> > wrapper to g_memdup2(), and use the safer equivalent inlined when
> >> > we are using pre-2.68 GLib.
> >> >
> >> > Reported-by: Eric Blake 
> >> > Signed-off-by: Philippe Mathieu-Daudé 
> >> > ---
> >> >  include/glib-compat.h | 37 +
> >> >  1 file changed, 37 insertions(+)
> >> >
> >> > diff --git a/include/glib-compat.h b/include/glib-compat.h
> >> > index 9e95c888f54..8d01a8c01fb 100644
> >> > --- a/include/glib-compat.h
> >> > +++ b/include/glib-compat.h
> >> > @@ -68,6 +68,43 @@
> >> >   * without generating warnings.
> >> >   */
> >> >  
> >> > +/*
> >> > + * g_memdup2_qemu:
> >> > + * @mem: (nullable): the memory to copy.
> >> > + * @byte_size: the number of bytes to copy.
> >> > + *
> >> > + * Allocates @byte_size bytes of memory, and copies @byte_size bytes 
> >> > into it
> >> > + * from @mem. If @mem is %NULL it returns %NULL.
> >> > + *
> >> > + * This replaces g_memdup(), which was prone to integer overflows when
> >> > + * converting the argument from a #gsize to a #guint.
> >> > + *
> >> > + * This static inline version is a backport of the new public API from
> >> > + * GLib 2.68, kept internal to GLib for backport to older stable 
> >> > releases.
> >> > + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
> >> > + *
> >> > + * Returns: (nullable): a pointer to the newly-allocated copy of the 
> >> > memory,
> >> > + *  or %NULL if @mem is %NULL.
> >> > + */
> >> > +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize 
> >> > byte_size)
> >> > +{
> >> > +#if GLIB_CHECK_VERSION(2, 68, 0)
> >> > +return g_memdup2(mem, byte_size);
> >> > +#else
> >> > +gpointer new_mem;
> >> > +
> >> > +if (mem && byte_size != 0) {
> >> > +new_mem = g_malloc(byte_size);
> >> > +memcpy(new_mem, mem, byte_size);
> >> > +} else {
> >> > +new_mem = NULL;
> >> > +}
> >> > +
> >> > +return new_mem;
> >> > +#endif
> >> > +}
> >> > +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
> >> > +
> >> 
> >> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> >> s)?
> >
> > Not in this case. We use suffix as we don't want people calling this
> > directly with the suffix.
> >
> > In the glibcompat.h header we're attempting to transparently/secretly
> > replace/wrap standard glib APIs.  All the callers should remain using
> > the plain glib API name, never call the method with the suffix at
> > all. This lets us delete the wrapper later and not have to update
> > any callers. The suffix is basically just a hack of the impl we use
> > for transparent replacement.
> 
> Right - at the risk of bike shedding names maybe we should choose a
> suffix the better reflects the purpose like _alt or _internal rather
> than overloading qemu?

Sure, I'm fine with that.

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 v3 02/28] glib-compat: Introduce g_memdup2() wrapper

2021-12-17 Thread Daniel P . Berrangé
On Thu, Dec 16, 2021 at 02:11:37PM +, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé  writes:
> 
> > When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
> > (Fedora 34 provides GLib 2.68.1) we get:
> >
> >   hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use 
> > 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
> >   ...
> >
> > g_memdup() has been updated by g_memdup2() to fix eventual security
> > issues (size argument is 32-bit and could be truncated / wrapping).
> > GLib recommends to copy their static inline version of g_memdup2():
> > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
> >
> > Our glib-compat.h provides a comment explaining how to deal with
> > these deprecated declarations (see commit e71e8cc0355
> > "glib: enforce the minimum required version and warn about old APIs").
> >
> > Following this comment suggestion, implement the g_memdup2_qemu()
> > wrapper to g_memdup2(), and use the safer equivalent inlined when
> > we are using pre-2.68 GLib.
> >
> > Reported-by: Eric Blake 
> > Signed-off-by: Philippe Mathieu-Daudé 
> > ---
> >  include/glib-compat.h | 37 +
> >  1 file changed, 37 insertions(+)
> >
> > diff --git a/include/glib-compat.h b/include/glib-compat.h
> > index 9e95c888f54..8d01a8c01fb 100644
> > --- a/include/glib-compat.h
> > +++ b/include/glib-compat.h
> > @@ -68,6 +68,43 @@
> >   * without generating warnings.
> >   */
> >  
> > +/*
> > + * g_memdup2_qemu:
> > + * @mem: (nullable): the memory to copy.
> > + * @byte_size: the number of bytes to copy.
> > + *
> > + * Allocates @byte_size bytes of memory, and copies @byte_size bytes into 
> > it
> > + * from @mem. If @mem is %NULL it returns %NULL.
> > + *
> > + * This replaces g_memdup(), which was prone to integer overflows when
> > + * converting the argument from a #gsize to a #guint.
> > + *
> > + * This static inline version is a backport of the new public API from
> > + * GLib 2.68, kept internal to GLib for backport to older stable releases.
> > + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
> > + *
> > + * Returns: (nullable): a pointer to the newly-allocated copy of the 
> > memory,
> > + *  or %NULL if @mem is %NULL.
> > + */
> > +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
> > +{
> > +#if GLIB_CHECK_VERSION(2, 68, 0)
> > +return g_memdup2(mem, byte_size);
> > +#else
> > +gpointer new_mem;
> > +
> > +if (mem && byte_size != 0) {
> > +new_mem = g_malloc(byte_size);
> > +memcpy(new_mem, mem, byte_size);
> > +} else {
> > +new_mem = NULL;
> > +}
> > +
> > +return new_mem;
> > +#endif
> > +}
> > +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
> > +
> 
> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> s)?

Not in this case. We use suffix as we don't want people calling this
directly with the suffix.

In the glibcompat.h header we're attempting to transparently/secretly
replace/wrap standard glib APIs.  All the callers should remain using
the plain glib API name, never call the method with the suffix at
all. This lets us delete the wrapper later and not have to update
any callers. The suffix is basically just a hack of the impl we use
for transparent replacement. 

A method with a 'qemu_' prefix by constrast is something that callers
are explicitly expected to call directly.


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: [RFC qemu.qmp PATCH 17/24] Makefile: add build and publish targets

2021-12-17 Thread Daniel P . Berrangé
On Thu, Dec 16, 2021 at 06:35:23PM -0500, John Snow wrote:
> On Thu, Dec 16, 2021 at 5:48 AM Daniel P. Berrangé 
> wrote:
> 
> > On Wed, Dec 15, 2021 at 04:06:27PM -0500, John Snow wrote:
> > > Signed-off-by: John Snow 
> > > ---
> > >  Makefile | 32 
> > >  1 file changed, 32 insertions(+)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 97d737a..81bfca8 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -110,3 +110,35 @@ distclean: clean
> > >   rm -f .coverage .coverage.*
> > >   rm -rf htmlcov/
> > >   rm -rf test-results/
> > > +
> > > +.PHONY: pristine
> > > +pristine:
> > > + @git diff-files --quiet --ignore-submodules -- || \
> > > + (echo "You have unstaged changes."; exit 1)
> > > + @git diff-index --cached --quiet HEAD --ignore-submodules -- || \
> > > + (echo "Your index contains uncommitted changes."; exit 1)
> > > + @[ -z "$(shell git ls-files -o)" ] || \
> > > + (echo "You have untracked files: $(shell git ls-files
> > -o)"; exit 1)
> > > +
> > > +dist: setup.cfg setup.py Makefile README.rst
> > > + python3 -m build
> > > + @touch dist
> > > +
> > > +.PHONY: pre-publish
> > > +pre-publish: pristine dist
> > > + @git describe --exact-match 2>/dev/null || \
> > > + (echo -e "\033[0;31mThere is no annotated tag for this
> > commit.\033[0m"; exit 1)
> > > + python3 -m twine check --strict dist/*
> > > + git push -v --atomic --follow-tags --dry-run
> > > +
> > > +.PHONY: publish
> > > +publish: pre-publish
> > > + # Set the username via TWINE_USERNAME.
> > > + # Set the password via TWINE_PASSWORD.
> > > + # Set the pkg repository via TWINE_REPOSITORY.
> > > + python3 -m twine upload --verbose dist/*
> > > + git push -v --atomic --follow-tags
> > > +
> > > +.PHONY: publish-test
> > > +publish-test: pre-publish
> > > + python3 -m twine upload --verbose -r testpypi dist/*
> >
> > It doesn't feel very pythonic to have a makefile in the project.
> >
> > If we want some helpers for publishing releases, I would have
> > expected to see a python script  eg scripts/publish.py
> >
> >
> Eh, Python folks use Makefiles too. I've been using these little Makefile
> targets for hobby things for a while and I had them laying around and ready
> to go. I have no strong need to "upgrade" to python scripts for these right
> now, unless there's some extra features you want to see.

Using make means you have to worry about portability across different
impls of make and different impls of shell. Using python means your
python project is portable to anywhere that python runs.

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 v3 02/28] glib-compat: Introduce g_memdup2() wrapper

2021-12-17 Thread Daniel P . Berrangé
On Fri, Dec 17, 2021 at 11:10:31AM +, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé  writes:
> 
> > On 12/16/21 15:11, Alex Bennée wrote:
> >> Philippe Mathieu-Daudé  writes:
> >> 
> >>> When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
> >>> (Fedora 34 provides GLib 2.68.1) we get:
> >>>
> >>>   hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use 
> >>> 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
> >>>   ...
> >>>
> >>> g_memdup() has been updated by g_memdup2() to fix eventual security
> >>> issues (size argument is 32-bit and could be truncated / wrapping).
> >>> GLib recommends to copy their static inline version of g_memdup2():
> >>> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
> >>>
> >>> Our glib-compat.h provides a comment explaining how to deal with
> >>> these deprecated declarations (see commit e71e8cc0355
> >>> "glib: enforce the minimum required version and warn about old APIs").
> >>>
> 
> >>> +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
> >>> +
> >> 
> >> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> >> s)?
> >
> > I followed the documentation in include/glib-compat.h:
> >
> > /*
> >  * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
> > allowing
> >  * use of functions from newer GLib via this compat header needs a little
> >  * trickery to prevent warnings being emitted.
> >  *
> >  * Consider a function from newer glib-X.Y that we want to use
> >  *
> >  *int g_foo(const char *wibble)
> >  *
> >  * We must define a static inline function with the same signature that does
> >  * what we need, but with a "_qemu" suffix e.g.
> >  *
> >  * static inline void g_foo_qemu(const char *wibble)
> >  * {
> >  * #if GLIB_CHECK_VERSION(X, Y, 0)
> >  *g_foo(wibble)
> >  * #else
> >  *g_something_equivalent_in_older_glib(wibble);
> >  * #endif
> >  * }
> >  *
> >  * The #pragma at the top of this file turns off -Wdeprecated-declarations,
> >  * ensuring this wrapper function impl doesn't trigger the compiler warning
> >  * about using too new glib APIs. Finally we can do
> >  *
> >  *   #define g_foo(a) g_foo_qemu(a)
> >  *
> >  * So now the code elsewhere in QEMU, which *does* have the
> >  * -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
> >  * without generating warnings.
> >  */
> >
> > which is how g_unix_get_passwd_entry_qemu() is implemented.
> 
> Yet later we have qemu_g_test_slow following the style guide. Also I'm
> confused by the usage of g_unix_get_passwd_entry_qemu because the only
> place I see it in qga/commands-posix-ssh.c right before it does:
> 
> #define g_unix_get_passwd_entry_qemu(username, err) \
>test_get_passwd_entry(username, err)

The g_unix_get_passwd_entry method is a bad example as it is
not following the guide for transparent replacement.

 > 
> although I think that only hold when the file is built with
> QGA_BUILD_UNIT_TEST.
> 
> > Should we reword the documentation first?
> 
> The original wording in glib-compat.h was added by Daniel in 2018 but
> the commit that added the password function comments:
> 
>   Since the fallback version is still unsafe, I would rather keep the
>   _qemu postfix, to make sure it's not being misused by mistake. When/if
>   necessary, we can implement a safer fallback and drop the _qemu suffix.
> 
> So if we are going to make a distinction between a qemu prefix and
> suffix we should agree that and add it to the style document.

The glibcompat.h header should only be used for cases where
we are doing transparent replacement such that callers continue
using the normal glib API name, and the suffix is a hidden
impl detail only seen in the header.

I think in that particular case we should have just had
'qemu_unix_get_passwd_entry' defined in a completely separate
place like osdep.c.

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 for-7.0] hw: Add compat machines for 7.0

2021-12-17 Thread Daniel P . Berrangé
On Fri, Dec 17, 2021 at 09:13:55AM +0100, Cornelia Huck wrote:
> On Wed, Dec 08 2021, Cornelia Huck  wrote:
> 
> > Add 7.0 machine types for arm/i440fx/q35/s390x/spapr.
> >
> > Signed-off-by: Cornelia Huck 
> > ---
> >  hw/arm/virt.c  |  9 -
> >  hw/core/machine.c  |  3 +++
> >  hw/i386/pc.c   |  3 +++
> >  hw/i386/pc_piix.c  | 14 +-
> >  hw/i386/pc_q35.c   | 13 -
> >  hw/ppc/spapr.c | 15 +--
> >  hw/s390x/s390-virtio-ccw.c | 14 +-
> >  include/hw/boards.h|  3 +++
> >  include/hw/i386/pc.h   |  3 +++
> >  9 files changed, 71 insertions(+), 6 deletions(-)
> >
> 


> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 223dd3e05d15..b03026bf0648 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -413,7 +413,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
> >  machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
> >  }
> >  
> > -static void pc_i440fx_6_2_machine_options(MachineClass *m)
> > +static void pc_i440fx_7_0_machine_options(MachineClass *m)
> >  {
> >  PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> >  pc_i440fx_machine_options(m);
> > @@ -422,6 +422,18 @@ static void pc_i440fx_6_2_machine_options(MachineClass 
> > *m)
> >  pcmc->default_cpu_version = 1;
> >  }
> >  
> > +DEFINE_I440FX_MACHINE(v7_0, "pc-i440fx-7.0", NULL,
> > +  pc_i440fx_7_0_machine_options);
> > +
> > +static void pc_i440fx_6_2_machine_options(MachineClass *m)
> > +{
> > +pc_i440fx_machine_options(m);

Needs to be pc_i440fx_7_0_machine_options()

> > +m->alias = NULL;
> > +m->is_default = false;
> > +compat_props_add(m->compat_props, hw_compat_6_2, hw_compat_6_2_len);
> > +compat_props_add(m->compat_props, pc_compat_6_2, pc_compat_6_2_len);
> > +}
> > +
> >  DEFINE_I440FX_MACHINE(v6_2, "pc-i440fx-6.2", NULL,
> >pc_i440fx_6_2_machine_options);
> >  
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index e1e100316d93..6b66eb16bb64 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -360,7 +360,7 @@ static void pc_q35_machine_options(MachineClass *m)
> >  m->max_cpus = 288;
> >  }
> >  
> > -static void pc_q35_6_2_machine_options(MachineClass *m)
> > +static void pc_q35_7_0_machine_options(MachineClass *m)
> >  {
> >  PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> >  pc_q35_machine_options(m);
> > @@ -368,6 +368,17 @@ static void pc_q35_6_2_machine_options(MachineClass *m)
> >  pcmc->default_cpu_version = 1;
> >  }
> >  
> > +DEFINE_Q35_MACHINE(v7_0, "pc-q35-7.0", NULL,
> > +   pc_q35_7_0_machine_options);
> > +
> > +static void pc_q35_6_2_machine_options(MachineClass *m)
> > +{
> > +pc_q35_machine_options(m);

Needs to be pc_q35_7_0_machine_options()

> > +m->alias = NULL;
> > +compat_props_add(m->compat_props, hw_compat_6_2, hw_compat_6_2_len);
> > +compat_props_add(m->compat_props, pc_compat_6_2, pc_compat_6_2_len);
> > +}
> > +
> >  DEFINE_Q35_MACHINE(v6_2, "pc-q35-6.2", NULL,
> > pc_q35_6_2_machine_options);
> >  
> 
> So, this apparently causes some problems with one of the avocado tests:
> 
> 162-tests/avocado/x86_cpu_model_versions.py:X86CPUModelAliases.test_4_1_alias 
> -> AssertionError: None != 'Cascadelake-Server-v1' : Cascadelake-Server must 
> be an alias of Cascadelake-Server-v1
> 
> (full output at https://gitlab.com/qemu-project/qemu/-/jobs/1893456217)
> 
> I have looked at the patch again and do not see what might be wrong (has
> something changed with the cpu model versioning recently?)
> 
> Does anyone else (especially the x86 folks) have an idea?

AFAICT, just a typo in chaining up the methods I've pointed out inline.

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] docs: Add measurement calculation details to amd-memory-encryption.txt

2021-12-17 Thread Daniel P . Berrangé
On Thu, Dec 16, 2021 at 11:41:27PM +0200, Dov Murik wrote:
> 
> 
> On 16/12/2021 18:09, Daniel P. Berrangé wrote:
> > On Thu, Dec 16, 2021 at 12:38:34PM +0200, Dov Murik wrote:
> >>
> >>
> >> On 14/12/2021 20:39, Daniel P. Berrangé wrote:
> >>> On Tue, Dec 14, 2021 at 01:59:10PM +, Dov Murik wrote:
> >>>> Add a section explaining how the Guest Owner should calculate the
> >>>> expected guest launch measurement for SEV and SEV-ES.
> >>>>
> >>>> Also update the name and link to the SEV API Spec document.
> >>>>
> >>>> Signed-off-by: Dov Murik 
> >>>> Suggested-by: Daniel P. Berrangé 
> >>>> ---
> >>>>  docs/amd-memory-encryption.txt | 50 +++---
> >>>>  1 file changed, 46 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/docs/amd-memory-encryption.txt 
> >>>> b/docs/amd-memory-encryption.txt
> >>>> index ffca382b5f..f97727482f 100644
> >>>> --- a/docs/amd-memory-encryption.txt
> >>>> +++ b/docs/amd-memory-encryption.txt
> >>>> @@ -43,7 +43,7 @@ The guest policy is passed as plaintext. A hypervisor 
> >>>> may choose to read it,
> >>>>  but should not modify it (any modification of the policy bits will 
> >>>> result
> >>>>  in bad measurement). The guest policy is a 4-byte data structure 
> >>>> containing
> >>>>  several flags that restricts what can be done on a running SEV guest.
> >>>> -See KM Spec section 3 and 6.2 for more details.
> >>>> +See SEV API Spec [1] section 3 and 6.2 for more details.
> >>>>  
> >>>>  The guest policy can be provided via the 'policy' property (see below)
> >>>>  
> >>>> @@ -88,7 +88,7 @@ expects.
> >>>>  LAUNCH_FINISH finalizes the guest launch and destroys the cryptographic
> >>>>  context.
> >>>>  
> >>>> -See SEV KM API Spec [1] 'Launching a guest' usage flow (Appendix A) for 
> >>>> the
> >>>> +See SEV API Spec [1] 'Launching a guest' usage flow (Appendix A) for the
> >>>>  complete flow chart.
> >>>>  
> >>>>  To launch a SEV guest
> >>>> @@ -113,6 +113,45 @@ a SEV-ES guest:
> >>>>   - Requires in-kernel irqchip - the burden is placed on the hypervisor 
> >>>> to
> >>>> manage booting APs.
> >>>>  
> >>>> +Calculating expected guest launch measurement
> >>>> +-
> >>>> +In order to verify the guest launch measurement, The Guest Owner must 
> >>>> compute
> >>>> +it in the exact same way as it is calculated by the AMD-SP.  SEV API 
> >>>> Spec [1]
> >>>> +section 6.5.1 describes the AMD-SP operations:
> >>>> +
> >>>> +GCTX.LD is finalized, producing the hash digest of all plaintext 
> >>>> data
> >>>> +imported into the guest.
> >>>> +
> >>>> +The launch measurement is calculated as:
> >>>> +
> >>>> +HMAC(0x04 || API_MAJOR || API_MINOR || BUILD || GCTX.POLICY || 
> >>>> GCTX.LD || MNONCE; GCTX.TIK)
> >>>> +
> >>>> +where "||" represents concatenation.
> >>>> +
> >>>> +The values of API_MAJOR, API_MINOR, BUILD, and GCTX.POLICY can be 
> >>>> obtained
> >>>> +from the 'query-sev' qmp command.
> >>>> +
> >>>> +The value of MNONCE is part of the response of 
> >>>> 'query-sev-launch-measure': it
> >>>> +is the last 16 bytes of the base64-decoded data field (see SEV API Spec 
> >>>> [1]
> >>>> +section 6.5.2 Table 52: LAUNCH_MEASURE Measurement Buffer).
> >>>> +
> >>>> +The value of GCTX.LD is SHA256(firmware_blob || kernel_hashes_blob || 
> >>>> vmsas_blob),
> >>>> +where:
> >>>> +
> >>>> +* firmware_blob is the content of the entire firmware flash file (for 
> >>>> example,
> >>>> +  OVMF.fd).
> >>>
> >>> Lets add a caveat that the firmware flash should be built to be stateless
> >>> ie that it is not secure to attempt to measure a guest where the firmware
> >>> uses an NVRAM store.
> >

Re: Redesign of QEMU startup & initial configuration

2021-12-16 Thread Daniel P . Berrangé
On Thu, Dec 16, 2021 at 05:00:55PM +0100, Mark Burton wrote:
> 
> 
> > On 16 Dec 2021, at 16:40, Daniel P. Berrangé  wrote:
> > 
> > On Thu, Dec 16, 2021 at 04:28:29PM +0100, Paolo Bonzini wrote:
> >> On 12/16/21 11:24, Markus Armbruster wrote:
> >>>> Not really, in particular the startup has been mostly reworked already
> >>>> and I disagree that it is messy.  softmmu/vl.c is messy, sure: it has
> >>>> N different command line parser for command line options, magic
> >>>> related to default devices, and complicated ordering of -object
> >>>> creation.
> >>>> 
> >>>> But the building of emulator data structures is not messy; only the
> >>>> code that transforms the user's instructions into startup commands.
> >>>> The messy parts are almost entirely contained within softmmu/vl.c.
> >>> 
> >>> In my opinion, the worst mess is the reordering and the (commonly
> >>> unstated, sometimes unknown) dependencies that govern it.
> >>> The reordering is part of the stable interface.  Its finer points
> >>> basically nobody understands, at least not without staring at the code.
> >> 
> >> Then we agree, because that's what I meant by "the messy parts are almost
> >> entirely contained within softmmu/vl.c".
> >> 
> >>>> The one (and important, but fixable) exception is backends for
> >>>> on-board devices: serial_hd, drive_get, and nd_table.
> >>> 
> >>> Practical ideas on fixing it?
> >> 
> >> What you did with pflash, turned up to 11.
> >> 
> >>>>> * A new binary sidesteps the need to manage incompatible change.
> >>>> 
> >>>> More precisely, a new binary sidesteps the need to integrate an
> >>>> existing mechanism with a new transport, and deal with the
> >>>> incompatibilities that arise.
> >>> 
> >>> I'm not sure I got this.
> >> 
> >> Configuring the VM part in CLI and part in QMP is not something anybody
> >> needs nor should desire.  A new binary can use the CLI only for things that
> >> really have to be done before QMP is up, for example the configuration of
> >> sandboxing or tracing; and even that is only nice-to-have and not 
> >> absolutely
> >> necessary.
> > 
> > I wouldn't special case sandboxing at least. It should be something that
> > can be activated via a QMP command too. That way you can blockdev-add
> > a bunch of disks and other backends while you are still relatively
> > unconfined, and then lock it down with a sandbox before starting vCPUs.
> > 
> > Or you can perhaps start with a coarse sandbox and then switch to
> > a stronger sandbox part way through config (though can't remember
> > offhand if that's possible with seccomp, or whether the first
> > seccomp profile applied, prevents you adding stronger ones later).
> > 
> > Anyway sandboxing doesn't need to be active before QMP IMHO, because
> > our primary goal with it is to mitigate guest exploits from untrusted
> > code - the initial startup is largely trustworthy. Starting guest CPUs,
> > or reading VMState is where it becomes dangerous generally.
> > 
> > I think you can probably argue that even tracing doesn't hugely
> > need special casing if you can get QMP started early enough that
> > there's little else that can go wrong before you get a chance to
> > send a QMP 'trace' command.
> > 
> >> 
> >>>> The problem is that CLI and HMP, being targeted to humans (and as you
> >>>> say below humans matter), are not necessarily trivial transports.  If
> >>>> people find the trivial transport unusable, we will not be able to
> >>>> retire the old CLI.
> >>> 
> >>> Yes, a trivial CLI transport alone may not suffice to retire the old
> >>> CLI.  By itself, that doesn't mean trivial transports must be avoided.
> >>> 
> >>> Do I have to argue the benefits of a trivial configuration file
> >>> transport?
> >> 
> >> I think you do; I'm not sure that a trivial configuration file transport is
> >> useful.  It's a middle ground in sophistication that I'm not sure would
> >> serve many people.  The only source of bug reports for -readconfig has been
> >> lxd, and I think we agree that they would be served better by QMP.
> >> 
> >>> Do I have to argue the benefits of a trivial CLI transport for use by
> >&g

Re: [PATCH] docs: Add measurement calculation details to amd-memory-encryption.txt

2021-12-16 Thread Daniel P . Berrangé
On Thu, Dec 16, 2021 at 12:38:34PM +0200, Dov Murik wrote:
> 
> 
> On 14/12/2021 20:39, Daniel P. Berrangé wrote:
> > On Tue, Dec 14, 2021 at 01:59:10PM +, Dov Murik wrote:
> >> Add a section explaining how the Guest Owner should calculate the
> >> expected guest launch measurement for SEV and SEV-ES.
> >>
> >> Also update the name and link to the SEV API Spec document.
> >>
> >> Signed-off-by: Dov Murik 
> >> Suggested-by: Daniel P. Berrangé 
> >> ---
> >>  docs/amd-memory-encryption.txt | 50 +++---
> >>  1 file changed, 46 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/docs/amd-memory-encryption.txt 
> >> b/docs/amd-memory-encryption.txt
> >> index ffca382b5f..f97727482f 100644
> >> --- a/docs/amd-memory-encryption.txt
> >> +++ b/docs/amd-memory-encryption.txt
> >> @@ -43,7 +43,7 @@ The guest policy is passed as plaintext. A hypervisor 
> >> may choose to read it,
> >>  but should not modify it (any modification of the policy bits will result
> >>  in bad measurement). The guest policy is a 4-byte data structure 
> >> containing
> >>  several flags that restricts what can be done on a running SEV guest.
> >> -See KM Spec section 3 and 6.2 for more details.
> >> +See SEV API Spec [1] section 3 and 6.2 for more details.
> >>  
> >>  The guest policy can be provided via the 'policy' property (see below)
> >>  
> >> @@ -88,7 +88,7 @@ expects.
> >>  LAUNCH_FINISH finalizes the guest launch and destroys the cryptographic
> >>  context.
> >>  
> >> -See SEV KM API Spec [1] 'Launching a guest' usage flow (Appendix A) for 
> >> the
> >> +See SEV API Spec [1] 'Launching a guest' usage flow (Appendix A) for the
> >>  complete flow chart.
> >>  
> >>  To launch a SEV guest
> >> @@ -113,6 +113,45 @@ a SEV-ES guest:
> >>   - Requires in-kernel irqchip - the burden is placed on the hypervisor to
> >> manage booting APs.
> >>  
> >> +Calculating expected guest launch measurement
> >> +-
> >> +In order to verify the guest launch measurement, The Guest Owner must 
> >> compute
> >> +it in the exact same way as it is calculated by the AMD-SP.  SEV API Spec 
> >> [1]
> >> +section 6.5.1 describes the AMD-SP operations:
> >> +
> >> +GCTX.LD is finalized, producing the hash digest of all plaintext data
> >> +imported into the guest.
> >> +
> >> +The launch measurement is calculated as:
> >> +
> >> +HMAC(0x04 || API_MAJOR || API_MINOR || BUILD || GCTX.POLICY || 
> >> GCTX.LD || MNONCE; GCTX.TIK)
> >> +
> >> +where "||" represents concatenation.
> >> +
> >> +The values of API_MAJOR, API_MINOR, BUILD, and GCTX.POLICY can be obtained
> >> +from the 'query-sev' qmp command.
> >> +
> >> +The value of MNONCE is part of the response of 
> >> 'query-sev-launch-measure': it
> >> +is the last 16 bytes of the base64-decoded data field (see SEV API Spec 
> >> [1]
> >> +section 6.5.2 Table 52: LAUNCH_MEASURE Measurement Buffer).
> >> +
> >> +The value of GCTX.LD is SHA256(firmware_blob || kernel_hashes_blob || 
> >> vmsas_blob),
> >> +where:
> >> +
> >> +* firmware_blob is the content of the entire firmware flash file (for 
> >> example,
> >> +  OVMF.fd).
> > 
> > Lets add a caveat that the firmware flash should be built to be stateless
> > ie that it is not secure to attempt to measure a guest where the firmware
> > uses an NVRAM store.
> > 
> 
> * firmware_blob is the content of the entire firmware flash file (for   
>   example, OVMF.fd).  Note that you must build a stateless firmware file
>   which doesn't use an NVRAM store, because the NVRAM area is not
>   measured, and therefore it is not secure to use a firmware which uses 
>   state from an NVRAM store.

Looks good to me.

> >> +* if kernel is used, and kernel-hashes=on, then kernel_hashes_blob is the
> >> +  content of PaddedSevHashTable (including the zero padding), which itself
> >> +  includes the hashes of kernel, initrd, and cmdline that are passed to 
> >> the
> >> +  guest.  The PaddedSevHashTable struct is defined in target/i386/sev.c .
> >> +* if SEV-ES is enabled (policy & 0x4 != 0), vmsas_blob is the 
> >> concatenation of
> >> +  all VMSAs of the g

Re: Redesign of QEMU startup & initial configuration

2021-12-16 Thread Daniel P . Berrangé
On Thu, Dec 16, 2021 at 04:28:29PM +0100, Paolo Bonzini wrote:
> On 12/16/21 11:24, Markus Armbruster wrote:
> > > Not really, in particular the startup has been mostly reworked already
> > > and I disagree that it is messy.  softmmu/vl.c is messy, sure: it has
> > > N different command line parser for command line options, magic
> > > related to default devices, and complicated ordering of -object
> > > creation.
> > > 
> > > But the building of emulator data structures is not messy; only the
> > > code that transforms the user's instructions into startup commands.
> > > The messy parts are almost entirely contained within softmmu/vl.c.
> > 
> > In my opinion, the worst mess is the reordering and the (commonly
> > unstated, sometimes unknown) dependencies that govern it.
> > The reordering is part of the stable interface.  Its finer points
> > basically nobody understands, at least not without staring at the code.
> 
> Then we agree, because that's what I meant by "the messy parts are almost
> entirely contained within softmmu/vl.c".
> 
> > > The one (and important, but fixable) exception is backends for
> > > on-board devices: serial_hd, drive_get, and nd_table.
> > 
> > Practical ideas on fixing it?
> 
> What you did with pflash, turned up to 11.
> 
> > > > * A new binary sidesteps the need to manage incompatible change.
> > > 
> > > More precisely, a new binary sidesteps the need to integrate an
> > > existing mechanism with a new transport, and deal with the
> > > incompatibilities that arise.
> > 
> > I'm not sure I got this.
> 
> Configuring the VM part in CLI and part in QMP is not something anybody
> needs nor should desire.  A new binary can use the CLI only for things that
> really have to be done before QMP is up, for example the configuration of
> sandboxing or tracing; and even that is only nice-to-have and not absolutely
> necessary.

I wouldn't special case sandboxing at least. It should be something that
can be activated via a QMP command too. That way you can blockdev-add
a bunch of disks and other backends while you are still relatively
unconfined, and then lock it down with a sandbox before starting vCPUs.

Or you can perhaps start with a coarse sandbox and then switch to
a stronger sandbox part way through config (though can't remember
offhand if that's possible with seccomp, or whether the first
seccomp profile applied, prevents you adding stronger ones later).

Anyway sandboxing doesn't need to be active before QMP IMHO, because
our primary goal with it is to mitigate guest exploits from untrusted
code - the initial startup is largely trustworthy. Starting guest CPUs,
or reading VMState is where it becomes dangerous generally.

I think you can probably argue that even tracing doesn't hugely
need special casing if you can get QMP started early enough that
there's little else that can go wrong before you get a chance to
send a QMP 'trace' command.

> 
> > > The problem is that CLI and HMP, being targeted to humans (and as you
> > > say below humans matter), are not necessarily trivial transports.  If
> > > people find the trivial transport unusable, we will not be able to
> > > retire the old CLI.
> > 
> > Yes, a trivial CLI transport alone may not suffice to retire the old
> > CLI.  By itself, that doesn't mean trivial transports must be avoided.
> > 
> > Do I have to argue the benefits of a trivial configuration file
> > transport?
> 
> I think you do; I'm not sure that a trivial configuration file transport is
> useful.  It's a middle ground in sophistication that I'm not sure would
> serve many people.  The only source of bug reports for -readconfig has been
> lxd, and I think we agree that they would be served better by QMP.
> 
> > Do I have to argue the benefits of a trivial CLI transport for use by
> > relatively unsophisticated programs / relatively sophisticated humans
> > (such as developers)? Can we agree these benefits are not zero?
> 
> We can.  But again I think you're misunderstanding the pain that the
> existing CLI inflicts on users.  Most users do not find the ordering
> complicated in practice; they do not even know that the issue exists. The
> problem that users have is the 100+ character command lines, and that can be
> fixed in two ways:
> 
> - with a trivial configuration file transport
> 
> - with a management tool such as virt-manager or virsh.
> 
> And I maintain that most users would be better served by the latter.  In
> fact, I constantly wonder how much we're overestimating the amount of people
> that are using QEMU.  In every post (Reddit, HN, wherever) that mentions
> QEMU being too complex and not having a front-end like VirtualBox, there's
> always someone that mentions virt-manager.

An important thing to note is that while libvirt is theoretically
general purpose for any aspect of QEMU, practically speaking our
coverage of QEMU features is very much skewed to virtualization
use cases. The emulation use cases don't get anywher near as much
love & 

Re: [PATCH v2 00/25] Python: delete synchronous qemu.qmp package

2021-12-16 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 02:39:14PM -0500, John Snow wrote:
> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-qmp-legacy-switch
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/430491195
> 
> Hi, this series is part of an effort to publish the qemu.qmp package on
> PyPI. It is the first of three series to complete this work:
> 
> --> (1) Switch the new Async QMP library in to python/qemu/qmp
> (2) Fork python/qemu/qmp out into its own repository,
> with updated GitLab CI/CD targets to build packages.
> (3) Update qemu.git to install qemu.qmp from PyPI,
> and then delete python/qemu/qmp.

What timeframe are you suggesting step (3) for ?

In the series for (2) you're calling it version 0.0.1 indicating
it is liable to  have API incompatible changes.

For step (3), either we're going to have to fetch a precise
version number to avoid risk of API breakage, or we're going
to have to call it stable in (2) and commit to the API.


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: [RFC qemu.qmp PATCH 17/24] Makefile: add build and publish targets

2021-12-16 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 04:06:27PM -0500, John Snow wrote:
> Signed-off-by: John Snow 
> ---
>  Makefile | 32 
>  1 file changed, 32 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 97d737a..81bfca8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -110,3 +110,35 @@ distclean: clean
>   rm -f .coverage .coverage.*
>   rm -rf htmlcov/
>   rm -rf test-results/
> +
> +.PHONY: pristine
> +pristine:
> + @git diff-files --quiet --ignore-submodules -- || \
> + (echo "You have unstaged changes."; exit 1)
> + @git diff-index --cached --quiet HEAD --ignore-submodules -- || \
> + (echo "Your index contains uncommitted changes."; exit 1)
> + @[ -z "$(shell git ls-files -o)" ] || \
> + (echo "You have untracked files: $(shell git ls-files -o)"; 
> exit 1)
> +
> +dist: setup.cfg setup.py Makefile README.rst
> + python3 -m build
> + @touch dist
> +
> +.PHONY: pre-publish
> +pre-publish: pristine dist
> + @git describe --exact-match 2>/dev/null || \
> + (echo -e "\033[0;31mThere is no annotated tag for this 
> commit.\033[0m"; exit 1)
> + python3 -m twine check --strict dist/*
> + git push -v --atomic --follow-tags --dry-run
> +
> +.PHONY: publish
> +publish: pre-publish
> + # Set the username via TWINE_USERNAME.
> + # Set the password via TWINE_PASSWORD.
> + # Set the pkg repository via TWINE_REPOSITORY.
> + python3 -m twine upload --verbose dist/*
> + git push -v --atomic --follow-tags
> +
> +.PHONY: publish-test
> +publish-test: pre-publish
> + python3 -m twine upload --verbose -r testpypi dist/*

It doesn't feel very pythonic to have a makefile in the project.

If we want some helpers for publishing releases, I would have
expected to see a python script  eg scripts/publish.py 

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: [RFC qemu.qmp PATCH 03/24] Update maintainer metadata

2021-12-16 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 04:06:13PM -0500, John Snow wrote:
> I'm the primary author of this particular component; update the metadata
> accordingly.
> 
> Signed-off-by: John Snow 
> ---
>  setup.cfg | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/setup.cfg b/setup.cfg
> index bca..7cd8470 100644
> --- a/setup.cfg
> +++ b/setup.cfg
> @@ -1,7 +1,9 @@
>  [metadata]
>  name = qemu.qmp
>  version = file:VERSION
> -maintainer = QEMU Developer Team
> +author = John Snow
> +author_email = js...@redhat.com

Isn't the authorship of this more of a collaborative effort ?
IOW, shouldn't it just be "The QEMU Project" as for the
maintainer.

> +maintainer = QEMU Project
>  maintainer_email = qemu-devel@nongnu.org
>  url = https://www.qemu.org/
>  download_url = https://www.qemu.org/download/

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: [RFC qemu.qmp PATCH 00/24] Python: Fork qemu.qmp Python lib into independent repo

2021-12-16 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 04:06:10PM -0500, John Snow wrote:
> Hi, this series is part of an effort to publish the qemu.qmp package on
> PyPI. It is the second of three series to complete this work:
> 
> (1) Switch the new Async QMP library in to python/qemu/qmp
> --> (2) Fork python/qemu/qmp out into its own repository,
> with updated GitLab CI/CD targets to build packages.
> (3) Update qemu.git to install qemu.qmp from PyPI,
> and then delete python/qemu/qmp.
> 
> This series is not meant to apply to qemu.git, rather -- it's the series
> that performs the split and would apply to a brand new repository.
> 
> I am submitting it to the QEMU mailing list for these reasons:
> 
> (1) To more broadly announce my intentions, and as reference alongside
> series #1 and #3 detailed above.
> 
> (2) To ask for permission to become the maintainer of a
> 'qemu-project/qemu.qmp' repository, where I would like to host this
> subproject.

I'd say we need 3 designated maintainers as a minimum for redundancy.

> (3) To ask for review on the README.rst file which details my intended
> contribution guidelines for this subproject.
> 
> (4) To ask for review on the .gitlab-ci.d/ files and other repo-level
> CI/CD ephemera, including and especially the docs-building process.  I
> think the generated docs are still ugly, and I'd like to upload them to
> readthedocs, among other things -- hence the RFC quality of this series.

> Some review/RFC notes:
> 
> - I use jsnow/qemu.qmp as the repo name throughout the series; that will
>   have to be changed eventually, but for the purposes of prototyping, it
>   was nicer to have a fully working series.
> 
> - I'm planning on using gitlab issues and MRs for the subproject.

Great !

> - I plan to version this lib independently, starting at 0.0.1 for the
>   initial public release and bumping only the micro version for every
>   last release. I plan to bump the minor version once it hits a "beta"
>   state. There will be no cross-versioning against QEMU. I don't plan to
>   publish new releases during QEMU freezes.

IMHO if we're saying that QEMU is going to use this library straight
from PyPI from the start, then we're defacto considering it staable
from the start too. We can't accept changes published to PyPI that
are going to be incompatible with existing QEMU.

If that isn't acceptable, then QEMU is going to have to be pinned to
a very specific version from PyPi, and explicitly not pull the
latest.

> - Docs are not yet uploaded anywhere (GitLab pages, readthedocs?)

Since we're already using gitlab, personally I'd just setup a 'pages'
job and assign a qemu.org sub-domain to gitlab pages service.

> - Tags on a commit trigger two pipelines; this causes one of the package
>   builds to fail as the version number will be duplicated in this
>   case. Not entirely sure how I want to fix this yet ...

If you dont have any 'rules:' stanza gitlab creates a pipeline
for any 'push' event - this means pushes of branch commits or
pushes of tags.

To remove the duplicates we need to filter based on certain
standard variables - CI_COMMIT_BRANCH or CI_COMMIT_TAG

  rules:
- if '$CI_PIPELINE_SOURCE != "push"'
  when: never
- if '$CI_PIPELINE_SOURCE == "push" && $CI_COMMIT_BRANCH'
  when: never
- if '$CI_PIPELINE_SOURCE == "push" && $CI_COMMIT_BRANCH'
  when: on_success
- when: never


will cull jobs for pushes of branch commit, leaving pipelines
for tag pushes. It can get arbitrarily more complicated depending
on what you need to achieve.


Since we're going to use merge requests, we should be aiming to
*NOT* run pipelines on branch commit pushes for forks. We only
want pipelines attached to the merge request.

You'll need pipelines on pushes of tags for the post-merge publishing
jobs potentially, unless you want todo that on a nightly schedule

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 v2 03/25] python/aqmp: copy type definitions from qmp

2021-12-16 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 02:39:17PM -0500, John Snow wrote:
> Copy the remaining type definitions from QMP into the qemu.aqmp.legacy
> module. Now, most users don't need to import anything else but
> qemu.aqmp.legacy.

I'm probably missing the historical discussion but it feels very
wierd to be saying

   "most users don't need anything except   legacy"

Naively, I'd expect most users to want something *not* legacy.


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 v2 06/25] python/qemu-ga-client: update instructions to newer CLI syntax

2021-12-16 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 02:39:20PM -0500, John Snow wrote:
> Signed-off-by: John Snow 
> ---
>  python/qemu/qmp/qemu_ga_client.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 

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




Re: [PATCH v2 14/25] scripts/cpu-x86-uarch-abi: switch to AQMP

2021-12-16 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 02:39:28PM -0500, John Snow wrote:
> Signed-off-by: John Snow 
> ---
>  scripts/cpu-x86-uarch-abi.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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




Re: [PATCH v2 13/25] scripts/cpu-x86-uarch-abi: fix CLI parsing

2021-12-16 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 02:39:27PM -0500, John Snow wrote:
> Signed-off-by: John Snow 
> ---
>  scripts/cpu-x86-uarch-abi.py | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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




Re: Redesign of QEMU startup & initial configuration

2021-12-15 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 07:46:37PM +0100, Paolo Bonzini wrote:
> On 12/13/21 19:53, Daniel P. Berrangé wrote:
> > > Adding vhost-user backends and helper processes means one of two things:
> > > either you are not going to support hotplug, or you are going to redo
> > > libvirtd with a QMP-based RPC.
> > 
> > If it were possible to keep auto-spawning of helpers at the high level
> > that feels cleaner, so that the low level only has to worry about a
> > single way of doing things. If that is too hard for hotplug though,
> > so be it, leave auto-spawning in the low level.
> 
> OTOH, autospawning in the low-level saves hotplugging but it kills
> sandboxing; the seccomp filter prohibits forking.


I think the kind of users we expect to leverage the high level interface
don't especially need sandboxing. They're more the people doing adhoc
virtualization or emulation tasks, not production deployments of VMs.
If they need strong security they'd be better off using a layer like
libvirt. 

> The libvirt model is the only good one once you care about separation of
> privilege.  The idea of moving large parts of libvirt's domain driver into a
> new QEMU-level binary was floated around in the past by Andrea Bolognani,
> and I don't dislike it; but I don't believe anybody will have time to
> actually realize it, much less to bring it to feature parity.

Yep, lets not create masses more work for ourselves, by expanding the
scope of this new design. 

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: [RFC PATCH] docs/devel: update C standard to C11

2021-12-15 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 03:20:55PM +, Alex Bennée wrote:
> Since 8a9d3d5640 (configure: Use -std=gnu11) we have allowed C11 code
> so lets reflect that in the style guide.
> 
> Signed-off-by: Alex Bennée 
> ---
>  docs/devel/style.rst | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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




[PATCH v5 14/18] .gitlab-ci.d/cirrus: auto-generate variables with lcitool

2021-12-15 Thread Daniel P . Berrangé
The current Cirrus CI variables files were previously generated by using
lcitool. This change wires them up to the refresh script to make that
link explicit.

This changes the package list because libvirt-ci now knows about the
mapping for dtc on FreeBSD and macOS platforms.

The variables are also now emit in sorted order for stability across
runs.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.d/cirrus/freebsd-12.vars | 11 +++
 .gitlab-ci.d/cirrus/freebsd-13.vars | 11 +++
 .gitlab-ci.d/cirrus/macos-11.vars   | 11 ++-
 tests/lcitool/refresh   | 10 ++
 4 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/.gitlab-ci.d/cirrus/freebsd-12.vars 
b/.gitlab-ci.d/cirrus/freebsd-12.vars
index 2099b21354..9c52266811 100644
--- a/.gitlab-ci.d/cirrus/freebsd-12.vars
+++ b/.gitlab-ci.d/cirrus/freebsd-12.vars
@@ -2,12 +2,15 @@
 #
 #  $ lcitool variables freebsd-12 qemu
 #
-# 
https://gitlab.com/libvirt/libvirt-ci/-/commit/c7e275ab27ac0dcd09da290817b9adeea1fd1eb1
+# https://gitlab.com/libvirt/libvirt-ci
 
-PACKAGING_COMMAND='pkg'
 CCACHE='/usr/local/bin/ccache'
+CPAN_PKGS=''
+CROSS_PKGS=''
 MAKE='/usr/local/bin/gmake'
 NINJA='/usr/local/bin/ninja'
-PYTHON='/usr/local/bin/python3'
+PACKAGING_COMMAND='pkg'
 PIP3='/usr/local/bin/pip-3.8'
-PKGS='alsa-lib bash bzip2 ca_root_nss capstone4 ccache cdrkit-genisoimage 
ctags curl cyrus-sasl dbus diffutils gettext git glib gmake gnutls gsed gtk3 
libepoxy libffi libgcrypt libjpeg-turbo libnfs libspice-server libssh libtasn1 
libxml2 llvm lttng-ust lzo2 meson ncurses nettle ninja opencv p5-Test-Harness 
perl5 pixman pkgconf png py38-numpy py38-pillow py38-pip py38-sphinx 
py38-sphinx_rtd_theme py38-virtualenv py38-yaml python3 rpm2cpio sdl2 
sdl2_image snappy spice-protocol tesseract texinfo usbredir virglrenderer vte3 
zstd'
+PKGS='alsa-lib bash bzip2 ca_root_nss capstone4 ccache cdrkit-genisoimage 
ctags curl cyrus-sasl dbus diffutils dtc gettext git glib gmake gnutls gsed 
gtk3 libepoxy libffi libgcrypt libjpeg-turbo libnfs libspice-server libssh 
libtasn1 libxml2 llvm lttng-ust lzo2 meson ncurses nettle ninja opencv 
p5-Test-Harness perl5 pixman pkgconf png py38-numpy py38-pillow py38-pip 
py38-sphinx py38-sphinx_rtd_theme py38-virtualenv py38-yaml python3 rpm2cpio 
sdl2 sdl2_image snappy spice-protocol tesseract texinfo usbredir virglrenderer 
vte3 zstd'
+PYPI_PKGS=''
+PYTHON='/usr/local/bin/python3'
diff --git a/.gitlab-ci.d/cirrus/freebsd-13.vars 
b/.gitlab-ci.d/cirrus/freebsd-13.vars
index 323fe806d5..7b44dba324 100644
--- a/.gitlab-ci.d/cirrus/freebsd-13.vars
+++ b/.gitlab-ci.d/cirrus/freebsd-13.vars
@@ -2,12 +2,15 @@
 #
 #  $ lcitool variables freebsd-13 qemu
 #
-# 
https://gitlab.com/libvirt/libvirt-ci/-/commit/c7e275ab27ac0dcd09da290817b9adeea1fd1eb1
+# https://gitlab.com/libvirt/libvirt-ci
 
-PACKAGING_COMMAND='pkg'
 CCACHE='/usr/local/bin/ccache'
+CPAN_PKGS=''
+CROSS_PKGS=''
 MAKE='/usr/local/bin/gmake'
 NINJA='/usr/local/bin/ninja'
-PYTHON='/usr/local/bin/python3'
+PACKAGING_COMMAND='pkg'
 PIP3='/usr/local/bin/pip-3.8'
-PKGS='alsa-lib bash bzip2 ca_root_nss capstone4 ccache cdrkit-genisoimage 
ctags curl cyrus-sasl dbus diffutils gettext git glib gmake gnutls gsed gtk3 
libepoxy libffi libgcrypt libjpeg-turbo libnfs libspice-server libssh libtasn1 
libxml2 llvm lttng-ust lzo2 meson ncurses nettle ninja opencv p5-Test-Harness 
perl5 pixman pkgconf png py38-numpy py38-pillow py38-pip py38-sphinx 
py38-sphinx_rtd_theme py38-virtualenv py38-yaml python3 rpm2cpio sdl2 
sdl2_image snappy spice-protocol tesseract texinfo usbredir virglrenderer vte3 
zstd'
+PKGS='alsa-lib bash bzip2 ca_root_nss capstone4 ccache cdrkit-genisoimage 
ctags curl cyrus-sasl dbus diffutils dtc gettext git glib gmake gnutls gsed 
gtk3 libepoxy libffi libgcrypt libjpeg-turbo libnfs libspice-server libssh 
libtasn1 libxml2 llvm lttng-ust lzo2 meson ncurses nettle ninja opencv 
p5-Test-Harness perl5 pixman pkgconf png py38-numpy py38-pillow py38-pip 
py38-sphinx py38-sphinx_rtd_theme py38-virtualenv py38-yaml python3 rpm2cpio 
sdl2 sdl2_image snappy spice-protocol tesseract texinfo usbredir virglrenderer 
vte3 zstd'
+PYPI_PKGS=''
+PYTHON='/usr/local/bin/python3'
diff --git a/.gitlab-ci.d/cirrus/macos-11.vars 
b/.gitlab-ci.d/cirrus/macos-11.vars
index cbec8a44a3..613d1373c2 100644
--- a/.gitlab-ci.d/cirrus/macos-11.vars
+++ b/.gitlab-ci.d/cirrus/macos-11.vars
@@ -2,14 +2,15 @@
 #
 #  $ lcitool variables macos-11 qemu
 #
-# 
https://gitlab.com/libvirt/libvirt-ci/-/commit/c7e275ab27ac0dcd09da290817b9adeea1fd1eb1
+# https://gitlab.com/libvirt/libvirt-ci
 
-PACKAGING_COMMAND='brew'
 CCACHE='/usr/local/bin/ccache'
+CPAN_PKGS='Test::Harness'
+CROSS_PKGS=''
 MAKE='/usr/local/bin/gmake'
 NINJA='/usr/local/bin/ninja'
-PYTHON='/usr/local/bin/python3'
+PACKAGING_COMMAND='brew'
 PIP3='/usr/local/bin/pip3'
-PKGS='bash bc bzip2 capstone ccache cpanminus ctags curl dbus diffutils gcovr 
gettext git glib gnu-sed gnutls

[PATCH v5 11/18] tests/docker: auto-generate ubuntu2004.docker with lcitool

2021-12-15 Thread Daniel P . Berrangé
This commit is best examined using the "-b" option to diff.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/docker/dockerfiles/ubuntu2004.docker | 257 -
 tests/lcitool/refresh  |   9 +-
 2 files changed, 151 insertions(+), 115 deletions(-)

diff --git a/tests/docker/dockerfiles/ubuntu2004.docker 
b/tests/docker/dockerfiles/ubuntu2004.docker
index 15a026be09..40402b91fe 100644
--- a/tests/docker/dockerfiles/ubuntu2004.docker
+++ b/tests/docker/dockerfiles/ubuntu2004.docker
@@ -1,119 +1,148 @@
+# THIS FILE WAS AUTO-GENERATED
+#
+#  $ lcitool dockerfile ubuntu-2004 qemu
+#
+# https://gitlab.com/libvirt/libvirt-ci
+
 FROM docker.io/library/ubuntu:20.04
-ENV PACKAGES \
-bc \
-bsdmainutils \
-bzip2 \
-ca-certificates \
-ccache \
-clang \
-dbus \
-debianutils \
-diffutils \
-exuberant-ctags \
-findutils \
-g++ \
-gcc \
-gcovr \
-genisoimage \
-gettext \
-git \
-hostname \
-libaio-dev \
-libasan5 \
-libasound2-dev \
-libattr1-dev \
-libbrlapi-dev \
-libbz2-dev \
-libc6-dev \
-libcacard-dev \
-libcap-ng-dev \
-libcapstone-dev \
-libcurl4-gnutls-dev \
-libdaxctl-dev \
-libdrm-dev \
-libepoxy-dev \
-libfdt-dev \
-libffi-dev \
-libgbm-dev \
-libgcrypt20-dev \
-libglib2.0-dev \
-libglusterfs-dev \
-libgnutls28-dev \
-libgtk-3-dev \
-libibverbs-dev \
-libiscsi-dev \
-libjemalloc-dev \
-libjpeg-turbo8-dev \
-liblttng-ust-dev \
-liblzo2-dev \
-libncursesw5-dev \
-libnfs-dev \
-libnuma-dev \
-libpam0g-dev \
-libpixman-1-dev \
-libpmem-dev \
-libpng-dev \
-libpulse-dev \
-librbd-dev \
-librdmacm-dev \
-libsasl2-dev \
-libsdl2-dev \
-libsdl2-image-dev \
-libseccomp-dev \
-libselinux-dev \
-libslirp-dev \
-libsnappy-dev \
-libspice-protocol-dev \
-libspice-server-dev \
-libssh-dev \
-libsystemd-dev \
-libtasn1-6-dev \
-libtest-harness-perl \
-libubsan1 \
-libudev-dev \
-libusb-1.0-0-dev \
-libusbredirhost-dev \
-libvdeplug-dev \
-libvirglrenderer-dev \
-libvte-2.91-dev \
-libxen-dev \
-libxml2-dev \
-libzstd-dev \
-llvm \
-locales \
-make \
-multipath-tools \
-ncat \
-nettle-dev \
-ninja-build \
-openssh-client \
-perl-base \
-pkgconf \
-python3 \
-python3-numpy \
-python3-opencv \
-python3-pillow \
-python3-pip \
-python3-setuptools \
-python3-sphinx \
-python3-sphinx-rtd-theme \
-python3-venv \
-python3-wheel \
-python3-yaml \
-rpm2cpio \
-sed \
-sparse \
-systemtap-sdt-dev \
-tar \
-tesseract-ocr \
-tesseract-ocr-eng \
-texinfo \
-xfslibs-dev \
-zlib1g-dev
-RUN apt-get update && \
-DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
-RUN dpkg -l $PACKAGES | sort > /packages.txt
 
+RUN export DEBIAN_FRONTEND=noninteractive && \
+apt-get update && \
+apt-get install -y eatmydata && \
+eatmydata apt-get dist-upgrade -y && \
+eatmydata apt-get install --no-install-recommends -y \
+bash \
+bc \
+bsdmainutils \
+bzip2 \
+ca-certificates \
+ccache \
+clang \
+dbus \
+debianutils \
+diffutils \
+exuberant-ctags \
+findutils \
+g++ \
+gcc \
+gcovr \
+genisoimage \
+gettext \
+git \
+hostname \
+libaio-dev \
+libasan5 \
+libasound2-dev \
+libattr1-dev \
+libbrlapi-dev \
+libbz2-dev \
+libc6-dev \
+libcacard-dev \
+libcap-ng-dev \
+libcapstone-dev \
+libcurl4-gnutls-dev \
+libdaxctl-dev \
+libdrm-dev \
+libepoxy-dev \
+libfdt-dev \
+libffi-dev \
+libgbm-dev \
+libgcrypt20-dev \
+libglib2.0-dev \
+libglusterfs-dev \
+libgnutls28-dev \
+libgtk-3-dev \
+libibverbs-dev \
+libiscsi-dev \
+libjemalloc-dev \
+libjpeg-turbo8-dev \
+liblttng-ust-dev \
+liblzo2-dev \
+libncursesw5-dev \
+libnfs-dev \
+libnuma-dev \
+libpam0g-dev \
+libpcre2-dev \
+libpixman-1-dev \
+libpmem-dev \
+libpng-dev \
+libpulse-dev \
+librbd-dev \
+librdmacm-dev \
+libsasl2-dev \
+libsdl2-dev \
+libsdl2-image-dev \
+libseccomp-dev \
+  

[PATCH v5 07/18] tests: integrate lcitool for generating build env manifests

2021-12-15 Thread Daniel P . Berrangé
This introduces

  https://gitlab.com/libvirt/libvirt-ci

as a git submodule at tests/lcitool/libvirt-ci

The 'lcitool' program within this submodule will be used to
automatically generate build environment manifests from a definition
of requirements in tests/lcitool/projects/qemu.yml

It will ultimately be capable of generating

 - Dockerfiles
 - Package lists for installation in VMs
 - Variables for configuring Cirrus CI environments

When a new build pre-requisite is needed for QEMU, if this package
is not currently known to libvirt-ci, it must first be added to the
'mappings.yml' file in the above git repo.

Then the submodule can be updated and the build pre-requisite added
to the tests/lcitool/projects/qemu.yml file. Now all the build env
manifests can be re-generated using  'make lcitool-refresh'

This ensures that when a new build pre-requisite is introduced, it
is added to all the different OS containers, VMs and Cirrus CI
environments consistently.

It also facilitates the addition of containers targetting new distros
or updating existing containers to new versions of the same distro,
where packages might have been renamed.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 .gitmodules |   3 +
 Makefile|   2 +
 docs/devel/testing.rst  | 104 -
 tests/lcitool/Makefile.include  |  17 +
 tests/lcitool/libvirt-ci|   1 +
 tests/lcitool/projects/qemu.yml | 115 
 tests/lcitool/refresh   |  67 +++
 7 files changed, 306 insertions(+), 3 deletions(-)
 create mode 100644 tests/lcitool/Makefile.include
 create mode 16 tests/lcitool/libvirt-ci
 create mode 100644 tests/lcitool/projects/qemu.yml
 create mode 100755 tests/lcitool/refresh

diff --git a/.gitmodules b/.gitmodules
index 08b1b48a09..84425d87e2 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -64,3 +64,6 @@
 [submodule "roms/vbootrom"]
path = roms/vbootrom
url = https://gitlab.com/qemu-project/vbootrom.git
+[submodule "tests/lcitool/libvirt-ci"]
+   path = tests/lcitool/libvirt-ci
+   url = http://gitlab.com/libvirt/libvirt-ci
diff --git a/Makefile b/Makefile
index 74c5b46d38..aec4728240 100644
--- a/Makefile
+++ b/Makefile
@@ -287,6 +287,7 @@ cscope:
 # Needed by "meson install"
 export DESTDIR
 
+include $(SRC_PATH)/tests/lcitool/Makefile.include
 include $(SRC_PATH)/tests/docker/Makefile.include
 include $(SRC_PATH)/tests/vm/Makefile.include
 
@@ -316,6 +317,7 @@ endif
@echo  'Test targets:'
$(call print-help,check,Run all tests (check-help for details))
$(call print-help,bench,Run all benchmarks)
+   $(call print-help,lcitool-help,Help about targets for managing build 
environment manifests)
$(call print-help,docker-help,Help about targets running tests inside 
containers)
$(call print-help,vm-help,Help about targets running tests inside VM)
@echo  ''
diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 755343c7dd..d744b5909c 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -382,14 +382,112 @@ Along with many other images, the ``centos8`` image is 
defined in a Dockerfile
 in ``tests/docker/dockerfiles/``, called ``centos8.docker``. ``make 
docker-help``
 command will list all the available images.
 
-To add a new image, simply create a new ``.docker`` file under the
-``tests/docker/dockerfiles/`` directory.
-
 A ``.pre`` script can be added beside the ``.docker`` file, which will be
 executed before building the image under the build context directory. This is
 mainly used to do necessary host side setup. One such setup is ``binfmt_misc``,
 for example, to make qemu-user powered cross build containers work.
 
+Most of the existing Dockerfiles were written by hand, simply by creating a
+a new ``.docker`` file under the ``tests/docker/dockerfiles/`` directory.
+This has led to an inconsistent set of packages being present across the
+different containers.
+
+Thus going forward, QEMU is aiming to automatically generate the Dockerfiles
+using the ``lcitool`` program provided by the ``libvirt-ci`` project:
+
+  https://gitlab.com/libvirt/libvirt-ci
+
+In that project, there is a ``mappings.yml`` file defining the distro native
+package names for a wide variety of third party projects. This is processed
+in combination with a project defined list of build pre-requisites to determine
+the list of native packages to install on each distribution. This can be used
+to generate dockerfiles, VM package lists and Cirrus CI variables needed to
+setup build environments across OS distributions with a consistent set of
+packages present.
+
+When preparing a patch series that adds a new build pre-requisite to QEMU,
+updates to various lcitool data files may be required.
+
+
+Adding new build pre-requisites
+^^^
+
+In

[PATCH v5 02/18] spice: Update QXLInterface for spice >= 0.15.0

2021-12-15 Thread Daniel P . Berrangé
From: John Snow 

spice updated the spelling (and arguments) of "attache_worker" in
0.15.0. Update QEMU to match, preventing -Wdeprecated-declarations
compilations from reporting build errors.

See also:
https://gitlab.freedesktop.org/spice/spice/-/commit/974692bda1e77af92b71ed43b022439448492cb9

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Signed-off-by: John Snow 
Signed-off-by: Daniel P. Berrangé 
---
 hw/display/qxl.c| 14 +-
 include/ui/qemu-spice.h |  6 ++
 ui/spice-display.c  | 11 +++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 29c80b4289..1da6703e44 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -517,13 +517,20 @@ static int qxl_track_command(PCIQXLDevice *qxl, struct 
QXLCommandExt *ext)
 
 /* spice display interface callbacks */
 
-static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker)
+static void interface_attached_worker(QXLInstance *sin)
 {
 PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
 
 trace_qxl_interface_attach_worker(qxl->id);
 }
 
+#if !(SPICE_HAS_ATTACHED_WORKER)
+static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker)
+{
+interface_attached_worker(sin);
+}
+#endif
+
 static void interface_set_compression_level(QXLInstance *sin, int level)
 {
 PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
@@ -1131,7 +1138,12 @@ static const QXLInterface qxl_interface = {
 .base.major_version  = SPICE_INTERFACE_QXL_MAJOR,
 .base.minor_version  = SPICE_INTERFACE_QXL_MINOR,
 
+#if SPICE_HAS_ATTACHED_WORKER
+.attached_worker = interface_attached_worker,
+#else
 .attache_worker  = interface_attach_worker,
+#endif
+
 .set_compression_level   = interface_set_compression_level,
 #if SPICE_NEEDS_SET_MM_TIME
 .set_mm_time = interface_set_mm_time,
diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index 71ecd6cfd1..21fe195e18 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -40,6 +40,12 @@ int qemu_spice_migrate_info(const char *hostname, int port, 
int tls_port,
 #define SPICE_NEEDS_SET_MM_TIME 0
 #endif
 
+#if defined(SPICE_SERVER_VERSION) && (SPICE_SERVER_VERSION >= 0x000f00)
+#define SPICE_HAS_ATTACHED_WORKER 1
+#else
+#define SPICE_HAS_ATTACHED_WORKER 0
+#endif
+
 #else  /* CONFIG_SPICE */
 
 #include "qemu/error-report.h"
diff --git a/ui/spice-display.c b/ui/spice-display.c
index f59c69882d..1a60cebb7d 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -500,10 +500,17 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
 
 /* spice display interface callbacks */
 
+#if SPICE_HAS_ATTACHED_WORKER
+static void interface_attached_worker(QXLInstance *sin)
+{
+/* nothing to do */
+}
+#else
 static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker)
 {
 /* nothing to do */
 }
+#endif
 
 static void interface_set_compression_level(QXLInstance *sin, int level)
 {
@@ -702,7 +709,11 @@ static const QXLInterface dpy_interface = {
 .base.major_version  = SPICE_INTERFACE_QXL_MAJOR,
 .base.minor_version  = SPICE_INTERFACE_QXL_MINOR,
 
+#if SPICE_HAS_ATTACHED_WORKER
+.attached_worker = interface_attached_worker,
+#else
 .attache_worker  = interface_attach_worker,
+#endif
 .set_compression_level   = interface_set_compression_level,
 #if SPICE_NEEDS_SET_MM_TIME
 .set_mm_time = interface_set_mm_time,
-- 
2.33.1




[PATCH v5 03/18] meson: require liburing >= 0.3

2021-12-15 Thread Daniel P . Berrangé
openSUSE Leap 15.2 ships with liburing == 0.2 against which QEMU fails
to build.

../util/fdmon-io_uring.c: In function ‘fdmon_io_uring_need_wait’:
../util/fdmon-io_uring.c:305:9: error: implicit declaration of function 
‘io_uring_sq_ready’; did you mean ‘io_uring_cq_ready’? 
[-Werror=implicit-function-declaration]
 if (io_uring_sq_ready(>fdmon_io_uring)) {
 ^
 io_uring_cq_ready

This method was introduced in liburing 0.3, so set that as a minimum
requirement.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 meson.build | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 96de1a6ef9..3b77d7f5b6 100644
--- a/meson.build
+++ b/meson.build
@@ -425,7 +425,8 @@ if not get_option('linux_aio').auto() or have_block
 endif
 linux_io_uring = not_found
 if not get_option('linux_io_uring').auto() or have_block
-  linux_io_uring = dependency('liburing', required: 
get_option('linux_io_uring'),
+  linux_io_uring = dependency('liburing', version: '>=0.3',
+  required: get_option('linux_io_uring'),
   method: 'pkg-config', kwargs: static_kwargs)
 endif
 libxml2 = not_found
-- 
2.33.1




[PATCH v5 01/18] ui: avoid compiler warnings from unused clipboard info variable

2021-12-15 Thread Daniel P . Berrangé
With latest clang 13.0.0 we get

../ui/clipboard.c:47:34: error: variable 'old' set but not used 
[-Werror,-Wunused-but-set-variable]
g_autoptr(QemuClipboardInfo) old = NULL;
 ^

The compiler can't tell that we only declared this variable in
order to get the side effect of free'ing it when out of scope.

This pattern is a little dubious for a use of g_autoptr, so
rewrite the code to avoid it.

Reviewed-by: Richard Henderson 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 ui/clipboard.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ui/clipboard.c b/ui/clipboard.c
index d7b008d62a..7672058e84 100644
--- a/ui/clipboard.c
+++ b/ui/clipboard.c
@@ -44,12 +44,11 @@ void qemu_clipboard_peer_release(QemuClipboardPeer *peer,
 
 void qemu_clipboard_update(QemuClipboardInfo *info)
 {
-g_autoptr(QemuClipboardInfo) old = NULL;
 assert(info->selection < QEMU_CLIPBOARD_SELECTION__COUNT);
 
 notifier_list_notify(_notifiers, info);
 
-old = cbinfo[info->selection];
+qemu_clipboard_info_unref(cbinfo[info->selection]);
 cbinfo[info->selection] = qemu_clipboard_info_ref(info);
 }
 
-- 
2.33.1




[PATCH v5 18/18] tests/docker: auto-generate alpine.docker with lcitool

2021-12-15 Thread Daniel P . Berrangé
This commit is best examined using the "-b" option to diff.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/docker/dockerfiles/alpine.docker | 225 +
 tests/lcitool/refresh  |   1 +
 2 files changed, 120 insertions(+), 106 deletions(-)

diff --git a/tests/docker/dockerfiles/alpine.docker 
b/tests/docker/dockerfiles/alpine.docker
index 0ac30c8014..97c7a88d1f 100644
--- a/tests/docker/dockerfiles/alpine.docker
+++ b/tests/docker/dockerfiles/alpine.docker
@@ -1,109 +1,122 @@
+# THIS FILE WAS AUTO-GENERATED
+#
+#  $ lcitool dockerfile alpine-edge qemu
+#
+# https://gitlab.com/libvirt/libvirt-ci
 
-FROM alpine:edge
+FROM docker.io/library/alpine:edge
 
-RUN apk update
-RUN apk upgrade
+RUN apk update && \
+apk upgrade && \
+apk add \
+alsa-lib-dev \
+attr-dev \
+bash \
+bc \
+bzip2 \
+bzip2-dev \
+ca-certificates \
+capstone-dev \
+ccache \
+cdrkit \
+ceph-dev \
+clang \
+ctags \
+curl-dev \
+cyrus-sasl-dev \
+dbus \
+diffutils \
+dtc-dev \
+eudev-dev \
+findutils \
+g++ \
+gcc \
+gcovr \
+gettext \
+git \
+glib-dev \
+glib-static \
+gnutls-dev \
+gtk+3.0-dev \
+libaio-dev \
+libbpf-dev \
+libcap-ng-dev \
+libdrm-dev \
+libepoxy-dev \
+libffi-dev \
+libgcrypt-dev \
+libjpeg-turbo-dev \
+libnfs-dev \
+libpng-dev \
+libseccomp-dev \
+libselinux-dev \
+libslirp-dev \
+libssh-dev \
+libtasn1-dev \
+liburing-dev \
+libusb-dev \
+libxml2-dev \
+linux-pam-dev \
+llvm11 \
+lttng-ust-dev \
+lzo-dev \
+make \
+mesa-dev \
+meson \
+multipath-tools \
+ncurses-dev \
+ndctl-dev \
+net-tools \
+nettle-dev \
+nmap-ncat \
+numactl-dev \
+openssh-client \
+pcre-dev \
+perl \
+perl-test-harness \
+pixman-dev \
+pkgconf \
+pulseaudio-dev \
+py3-numpy \
+py3-pillow \
+py3-pip \
+py3-sphinx \
+py3-sphinx_rtd_theme \
+py3-virtualenv \
+py3-yaml \
+python3 \
+rpm2cpio \
+samurai \
+sdl2-dev \
+sdl2_image-dev \
+sed \
+snappy-dev \
+sparse \
+spice-dev \
+spice-protocol \
+tar \
+tesseract-ocr \
+texinfo \
+usbredir-dev \
+util-linux \
+vde2-dev \
+virglrenderer-dev \
+vte3-dev \
+which \
+xen-dev \
+xfsprogs-dev \
+zlib-dev \
+zlib-static \
+zstd-dev && \
+mkdir -p /usr/libexec/ccache-wrappers && \
+ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/c++ && \
+ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/cc && \
+ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/clang && \
+ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/g++ && \
+ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/gcc
 
-# Please keep this list sorted alphabetically
-ENV PACKAGES \
-   alsa-lib-dev \
-   attr-dev \
-   bash \
-   bc \
-   bzip2 \
-   bzip2-dev \
-   ca-certificates \
-   capstone-dev \
-   ccache \
-   cdrkit \
-   ceph-dev \
-   clang \
-   ctags \
-   curl-dev \
-   cyrus-sasl-dev \
-   dbus \
-   diffutils \
-   dtc-dev \
-   eudev-dev \
-   findutils \
-   g++ \
-   gcc \
-   gcovr \
-   gettext \
-   git \
-   glib-dev \
-   glib-static \
-   gnutls-dev \
-   gtk+3.0-dev \
-   libaio-dev \
-   libbpf-dev \
-   libcap-ng-dev \
-   libdrm-dev \
-   libepoxy-dev \
-   libffi-dev \
-   libgcrypt-dev \
-   libjpeg-turbo-dev \
-   libnfs-dev \
-   libpng-dev \
-   libseccomp-dev \
-   libselinux-dev \
-   libslirp-dev \
-   libssh-dev \
-   libtasn1-dev \
-   liburing-dev \
-   libusb-dev \
-   libxml2-dev \
-   linux-pam-dev \
-   llvm11 \
-   lttng-ust-dev \
-   lzo-dev \
-   make \
-   mesa-dev \
-   meson \
-   multipath-tools \
-   ncurses-dev \
-   ndctl-dev \
-   net-tools \
-   nettle-dev \
-   nmap-ncat \
-   numactl-dev \
-   openssh-client \
-   pcre-dev \
-   perl \
-   perl-test-harness \
-   pixman-dev \
-   pkgconf \
-   pulseaudio-dev \
-   py3-numpy \
-   py3-pillow \
-   py3-pip \
-   py3-sphinx \
-   py3-sphinx_rtd_theme \
-   py3-virtualenv \
-   py3-yaml

[PATCH v5 17/18] tests/docker: fully expand the alpine package list

2021-12-15 Thread Daniel P . Berrangé
Add many extra alpine packages to cover the various optional QEMU build
dependencies pulled in by other dockerfiles.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/docker/dockerfiles/alpine.docker | 58 +-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/tests/docker/dockerfiles/alpine.docker 
b/tests/docker/dockerfiles/alpine.docker
index ca4b3b58d2..0ac30c8014 100644
--- a/tests/docker/dockerfiles/alpine.docker
+++ b/tests/docker/dockerfiles/alpine.docker
@@ -7,11 +7,29 @@ RUN apk upgrade
 # Please keep this list sorted alphabetically
 ENV PACKAGES \
alsa-lib-dev \
+   attr-dev \
bash \
+   bc \
+   bzip2 \
+   bzip2-dev \
+   ca-certificates \
+   capstone-dev \
ccache \
+   cdrkit \
+   ceph-dev \
+   clang \
+   ctags \
curl-dev \
+   cyrus-sasl-dev \
+   dbus \
+   diffutils \
+   dtc-dev \
+   eudev-dev \
+   findutils \
g++ \
gcc \
+   gcovr \
+   gettext \
git \
glib-dev \
glib-static \
@@ -20,34 +38,72 @@ ENV PACKAGES \
libaio-dev \
libbpf-dev \
libcap-ng-dev \
+   libdrm-dev \
+   libepoxy-dev \
libffi-dev \
+   libgcrypt-dev \
libjpeg-turbo-dev \
libnfs-dev \
libpng-dev \
libseccomp-dev \
+   libselinux-dev \
+   libslirp-dev \
libssh-dev \
+   libtasn1-dev \
+   liburing-dev \
libusb-dev \
libxml2-dev \
+   linux-pam-dev \
+   llvm11 \
+   lttng-ust-dev \
lzo-dev \
make \
mesa-dev \
meson \
+   multipath-tools \
ncurses-dev \
+   ndctl-dev \
+   net-tools \
+   nettle-dev \
+   nmap-ncat \
+   numactl-dev \
+   openssh-client \
+   pcre-dev \
perl \
+   perl-test-harness \
+   pixman-dev \
+   pkgconf \
pulseaudio-dev \
+   py3-numpy \
+   py3-pillow \
+   py3-pip \
py3-sphinx \
py3-sphinx_rtd_theme \
+   py3-virtualenv \
+   py3-yaml \
python3 \
+   rpm2cpio \
samurai \
+   sdl2-dev \
+   sdl2_image-dev \
+   sed \
snappy-dev \
+   sparse \
spice-dev \
+   spice-protocol \
+   tar \
+   tesseract-ocr \
texinfo \
usbredir-dev \
+   util-linux \
vde2-dev \
virglrenderer-dev \
vte3-dev \
+   which \
+   xen-dev \
xfsprogs-dev \
zlib-dev \
-   zlib-static
+   zlib-static \
+   zstd-dev
 
 RUN apk add $PACKAGES
-- 
2.33.1




[PATCH v5 08/18] tests/docker: auto-generate centos8.docker with lcitool

2021-12-15 Thread Daniel P . Berrangé
This commit is best examined using the "-b" option to diff.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/docker/dockerfiles/centos8.docker | 243 +---
 tests/lcitool/refresh   |   2 +
 2 files changed, 135 insertions(+), 110 deletions(-)

diff --git a/tests/docker/dockerfiles/centos8.docker 
b/tests/docker/dockerfiles/centos8.docker
index 7f135f8e8c..3c62b62a99 100644
--- a/tests/docker/dockerfiles/centos8.docker
+++ b/tests/docker/dockerfiles/centos8.docker
@@ -1,112 +1,135 @@
-FROM docker.io/centos:8
+# THIS FILE WAS AUTO-GENERATED
+#
+#  $ lcitool dockerfile centos-8 qemu
+#
+# https://gitlab.com/libvirt/libvirt-ci
 
-RUN dnf -y update
-ENV PACKAGES \
-SDL2-devel \
-alsa-lib-devel \
-bc \
-brlapi-devel \
-bzip2 \
-bzip2-devel \
-ca-certificates \
-capstone-devel \
-ccache \
-clang \
-ctags \
-cyrus-sasl-devel \
-daxctl-devel \
-dbus-daemon \
-device-mapper-multipath-devel \
-diffutils \
-findutils \
-gcc \
-gcc-c++ \
-genisoimage \
-gettext \
-git \
-glib2-devel \
-glibc-langpack-en \
-glibc-static \
-glusterfs-api-devel \
-gnutls-devel \
-gtk3-devel \
-hostname \
-jemalloc-devel \
-libaio-devel \
-libasan \
-libattr-devel \
-libbpf-devel \
-libcacard-devel \
-libcap-ng-devel \
-libcurl-devel \
-libdrm-devel \
-libepoxy-devel \
-libfdt-devel \
-libffi-devel \
-libgcrypt-devel \
-libiscsi-devel \
-libjpeg-devel \
-libnfs-devel \
-libpmem-devel \
-libpng-devel \
-librbd-devel \
-libseccomp-devel \
-libselinux-devel \
-libslirp-devel \
-libssh-devel \
-libtasn1-devel \
-libubsan \
-libudev-devel \
-libusbx-devel \
-libxml2-devel \
-libzstd-devel \
-llvm \
-lzo-devel \
-make \
-mesa-libgbm-devel \
-ncurses-devel \
-nettle-devel \
-ninja-build \
-nmap-ncat \
-numactl-devel \
-openssh-clients \
-pam-devel \
-perl \
-perl-Test-Harness \
-pixman-devel \
-pkgconfig \
-pulseaudio-libs-devel \
-python3 \
-python3-PyYAML \
-python3-numpy \
-python3-pillow \
-python3-pip \
-python3-setuptools \
-python3-sphinx \
-python3-sphinx_rtd_theme \
-python3-virtualenv \
-python3-wheel \
-rdma-core-devel \
-rpm \
-sed \
-snappy-devel \
-spice-protocol \
-spice-server-devel \
-systemd-devel \
-systemtap-sdt-devel \
-tar \
-texinfo \
-usbredir-devel \
-util-linux \
-virglrenderer-devel \
-vte291-devel \
-which \
-xfsprogs-devel \
-zlib-devel
+FROM docker.io/library/centos:8
 
-RUN dnf install -y dnf-plugins-core && \
-  dnf config-manager --set-enabled powertools && \
-  dnf install -y centos-release-advanced-virtualization && \
-  dnf install -y epel-release && \
-  dnf install -y $PACKAGES
-RUN rpm -q $PACKAGES | sort > /packages.txt
+RUN dnf update -y && \
+dnf install 'dnf-command(config-manager)' -y && \
+dnf config-manager --set-enabled -y powertools && \
+dnf install -y centos-release-advanced-virtualization && \
+dnf install -y epel-release && \
+dnf install -y \
+SDL2-devel \
+alsa-lib-devel \
+bash \
+bc \
+brlapi-devel \
+bzip2 \
+bzip2-devel \
+ca-certificates \
+capstone-devel \
+ccache \
+clang \
+ctags \
+cyrus-sasl-devel \
+daxctl-devel \
+dbus-daemon \
+device-mapper-multipath-devel \
+diffutils \
+findutils \
+gcc \
+gcc-c++ \
+genisoimage \
+gettext \
+git \
+glib2-devel \
+glib2-static \
+glibc-langpack-en \
+glibc-static \
+glusterfs-api-devel \
+gnutls-devel \
+gtk3-devel \
+hostname \
+jemalloc-devel \
+libaio-devel \
+libasan \
+libattr-devel \
+libbpf-devel \
+libcacard-devel \
+libcap-ng-devel \
+libcurl-devel \
+libdrm-devel \
+libepoxy-devel \
+libfdt-devel \
+libffi-devel \
+libgcrypt-devel \
+libiscsi-devel \
+libjpeg-devel \
+libnfs-devel \
+libpmem-devel \
+libpng-devel \
+librbd-devel \
+libseccomp-devel \
+libselinux-devel \
+libslirp-devel \
+libssh-devel \
+libtasn1-devel \
+libubsan \
+libudev-devel \
+liburing-devel \
+libusbx-devel \
+libxml2-devel \
+libzstd-devel \
+llvm \
+lttng-ust-devel \
+lzo-devel \
+make \
+mesa-libgbm-devel \
+meson \
+ncurses-devel \
+

[PATCH v5 16/18] tests/docker: fix sorting of alpine image package lists

2021-12-15 Thread Daniel P . Berrangé
"python" sorts alphabetically after "py3-"

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/docker/dockerfiles/alpine.docker | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/dockerfiles/alpine.docker 
b/tests/docker/dockerfiles/alpine.docker
index 5a1808726e..ca4b3b58d2 100644
--- a/tests/docker/dockerfiles/alpine.docker
+++ b/tests/docker/dockerfiles/alpine.docker
@@ -35,9 +35,9 @@ ENV PACKAGES \
ncurses-dev \
perl \
pulseaudio-dev \
-   python3 \
py3-sphinx \
py3-sphinx_rtd_theme \
+   python3 \
samurai \
snappy-dev \
spice-dev \
-- 
2.33.1




[PATCH v5 00/18] tests/docker: start using libvirt-ci's "lcitool" for dockerfiles

2021-12-15 Thread Daniel P . Berrangé
Currently the tests/docker/dockerfiles/*Dockerfile recipes are all hand
written by contributors. There is a common design pattern, but the set
of packages listed for installation leaves alot to be desired

 - There is no consistency at all across distros
 - Many potential build deps are not listed in the containers
 - Some packages are not used by QEMU at all
 - Adding new distros is an error prone task

The same applies to package lists for VMs, Cirrus CI / Travis CI, and
probably more.

This problem is not unique to QEMU, libvirt faced the exact same issues
and developed a program called "lcitool" which is part of the libvirt-ci
git repository to reduce the burden in this area.

Despite its name, this repository is not tied to libvirt, and so as well
as the 40+ libvirt git repos, it is also used by the libosinfo and
virt-viewer projects for their CI needs.

lcitool is capable of automating the installation and updating of VM
images, creation of dockerfiles and creation of standalone package
lists.

In this series I'm taking the easy step which is the generation of
dockerfiles and Cirrus CI variables, since that is where the most
immediate value lies for QEMU.

The key concept in lcitool that brings a huge win in maintainability
is that there is a single file which defines a mapping between a
build pre-requisite and the native package on each targetted distro.

   
https://gitlab.com/libvirt/libvirt-ci/-/blob/master/guests/lcitool/lcitool/ansible/vars/mappings.yml

A project merely then to have its list of pre-requisites enumerated
For example:

   
https://gitlab.com/libvirt/libvirt-ci/-/blob/master/guests/lcitool/lcitool/ansible/vars/projects/qemu.yml

The combination of these two files is enough to generate accurate
package lists for any supported distro. Currently supported distro
targets are

 $ lcitool targets
  alpine-314
  alpine-edge
  centos-8
  centos-stream-8
  centos-stream-9
  debian-10
  debian-11
  debian-sid
  fedora-34
  fedora-35
  fedora-rawhide
  freebsd-12
  freebsd-13
  freebsd-current
  macos-11
  opensuse-leap-152
  opensuse-tumbleweed
  ubuntu-1804
  ubuntu-2004

At the end of this series, I have dockerfiles auto-generated for QEMU
covering Ubuntu 18.04 & 20.04, CentOS 8, Fedora 35 and OpenSUSE 15.2
and Alpine Edge

lcitool is also capable of generating dockerfiles for cross-compiled
non-x86 architectures for Debian, and for mingw32/64 for Fedora. This
is driven from the very same mapping.yml file listed above, which has
attributes to indicate whether a given dependancy should be pulled from
the native or cross build target. Again this means that we have strong
guarantee of consistent deps being used between cross containers.

I have not converted cross containers in this series though, because
the way lcitool generated cross dockerfiles is different from how QEMU
does it. lcitool will always generate fully self-contained dockerfiles,
but QEMU currently uses layered dockerfiles for cross-builds, so all
cross builds share a common intermediate container.

I've got a pending enhancement to lcitool to support split layers
for the cross-buld dockerfiles. An alternative is to just use fully
self-contained dockerfiles for cross builds too though.

There is also scope for auto-generating the package lists for tests/vm,
but I've not attempted that yet. The same general idea appies - we just
call lcitool to spit out a yml file containing a list of native packages
for each VM target.

If converting tests/vm, we would need to add more distros to lcitool
mappings.yml to convert openbsd, netbsd, haiku since libvirt does not
target those distros itself.

I have provided a 'make lcitool-refresh' target that needs to be
invoked whenever the local files need re-generating with updated
package lists. This can be either when adding a new distro target
or when some build pre-requisite is added. This make target will
checkout the libvirt-ci.git submodule to do its work.

Changed in v5:

 - Rebased to latest git master
 - Typos in commit messages
 - CI pipeline is now validated to pass

Changed in v4:

 - Re-introduce use of git submodule
 - Pull qemu.yml package list into qemu.git not libvirt-ci.git
 - Improved make trget integration for refreshing files
 - Also convert Alpine dockerfile
 - Also generate cirrus CI variables files
 - Rebase fedora dockerfile to F35, since F33 is no longer
   supported by libvirt-ci as it is end of life by Fedora.

Changed in v4:

 - Refresh to cope with new libbpf, libffi & rtd theme packages
 - Drop use of git submodule
 - Improve documentation

Changed in v3:

 - Drop changes for CentOS 7
 - Catch up with newly added build deps
 - Add git submodule and make target for refresh

Changed in v2:

 - Remove more travis stuff from tests/docker/Makefile.include
 - Convert opensuse image to be auto-generated
 - Add SDL2_image package
 - QEMU package manifest is now officially merged in libvirt-ci.git

Daniel P. Berrangé (17):
  ui: avoid compiler wa

[PATCH v5 13/18] tests/docker: remove ubuntu.docker container

2021-12-15 Thread Daniel P . Berrangé
This duplicates the ubuntu2004 container but with an inconsistent set of
packages.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.d/containers.yml|  5 --
 tests/docker/dockerfiles/ubuntu.docker | 71 --
 2 files changed, 76 deletions(-)
 delete mode 100644 tests/docker/dockerfiles/ubuntu.docker

diff --git a/.gitlab-ci.d/containers.yml b/.gitlab-ci.d/containers.yml
index cd06d3f5f4..b9b675fdcb 100644
--- a/.gitlab-ci.d/containers.yml
+++ b/.gitlab-ci.d/containers.yml
@@ -29,11 +29,6 @@ amd64-ubuntu2004-container:
   variables:
 NAME: ubuntu2004
 
-amd64-ubuntu-container:
-  extends: .container_job_template
-  variables:
-NAME: ubuntu
-
 amd64-opensuse-leap-container:
   extends: .container_job_template
   variables:
diff --git a/tests/docker/dockerfiles/ubuntu.docker 
b/tests/docker/dockerfiles/ubuntu.docker
deleted file mode 100644
index f0e0180d21..00
--- a/tests/docker/dockerfiles/ubuntu.docker
+++ /dev/null
@@ -1,71 +0,0 @@
-#
-# Latest Ubuntu Release
-#
-# Useful for testing against relatively bleeding edge libraries and
-# compilers. We also have seperate recipe for the most recent LTS
-# release.
-#
-# When updating use the full tag not :latest otherwise the build
-# system won't pick up that it has changed.
-#
-
-FROM docker.io/library/ubuntu:20.04
-ENV PACKAGES \
-ccache \
-clang \
-dbus \
-gcc \
-gettext \
-git \
-glusterfs-common \
-libaio-dev \
-libattr1-dev \
-libbrlapi-dev \
-libbz2-dev \
-libcacard-dev \
-libcap-ng-dev \
-libcurl4-gnutls-dev \
-libdrm-dev \
-libepoxy-dev \
-libfdt-dev \
-libffi-dev \
-libgbm-dev \
-libgnutls28-dev \
-libgtk-3-dev \
-libibverbs-dev \
-libiscsi-dev \
-libjemalloc-dev \
-libjpeg-turbo8-dev \
-liblzo2-dev \
-libncurses5-dev \
-libncursesw5-dev \
-libnfs-dev \
-libnuma-dev \
-libpixman-1-dev \
-libpng-dev \
-librados-dev \
-librbd-dev \
-librdmacm-dev \
-libsasl2-dev \
-libsdl2-dev \
-libseccomp-dev \
-libsnappy-dev \
-libspice-protocol-dev \
-libspice-server-dev \
-libssh-dev \
-libusb-1.0-0-dev \
-libusbredirhost-dev \
-libvdeplug-dev \
-libvte-2.91-dev \
-libxen-dev \
-libzstd-dev \
-make \
-ninja-build \
-python3-yaml \
-python3-sphinx \
-python3-sphinx-rtd-theme \
-sparse \
-xfslibs-dev
-RUN apt-get update && \
-DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
-RUN dpkg -l $PACKAGES | sort > /packages.txt
-- 
2.33.1




[PATCH v5 12/18] tests/docker: auto-generate opensuse-leap.docker with lcitool

2021-12-15 Thread Daniel P . Berrangé
This commit is best examined using the "-b" option to diff.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/docker/dockerfiles/opensuse-leap.docker | 245 ++
 tests/lcitool/refresh |   1 +
 2 files changed, 135 insertions(+), 111 deletions(-)

diff --git a/tests/docker/dockerfiles/opensuse-leap.docker 
b/tests/docker/dockerfiles/opensuse-leap.docker
index 3bbdb67f4f..5510bdf19c 100644
--- a/tests/docker/dockerfiles/opensuse-leap.docker
+++ b/tests/docker/dockerfiles/opensuse-leap.docker
@@ -1,114 +1,137 @@
+# THIS FILE WAS AUTO-GENERATED
+#
+#  $ lcitool dockerfile opensuse-leap-152 qemu
+#
+# https://gitlab.com/libvirt/libvirt-ci
+
 FROM registry.opensuse.org/opensuse/leap:15.2
 
-# Please keep this list sorted alphabetically
-ENV PACKAGES \
-Mesa-devel \
-alsa-lib-devel \
-bc \
-brlapi-devel \
-bzip2 \
-ca-certificates \
-ccache \
-clang \
-ctags \
-cyrus-sasl-devel \
-dbus-1 \
-diffutils \
-findutils \
-gcc \
-gcc-c++ \
-gcovr \
-gettext-runtime \
-git \
-glib2-devel \
-glibc-locale \
-glibc-static \
-glusterfs-devel \
-gtk3-devel \
-hostname \
-jemalloc-devel \
-libSDL2-devel \
-libSDL2_image-devel \
-libaio-devel \
-libasan6 \
-libattr-devel \
-libbpf-devel \
-libbz2-devel \
-libcacard-devel \
-libcap-ng-devel \
-libcurl-devel \
-libdrm-devel \
-libepoxy-devel \
-libfdt-devel \
-libffi-devel \
-libgcrypt-devel \
-libgnutls-devel \
-libiscsi-devel \
-libjpeg8-devel \
-libndctl-devel \
-libnettle-devel \
-libnfs-devel \
-libnuma-devel \
-libpixman-1-0-devel \
-libpmem-devel \
-libpng16-devel \
-libpulse-devel \
-librbd-devel \
-libseccomp-devel \
-libselinux-devel \
-libspice-server-devel \
-libssh-devel \
-libtasn1-devel \
-libubsan1 \
-libudev-devel \
-libusb-1_0-devel \
-libxml2-devel \
-libzstd-devel \
-llvm \
-lttng-ust-devel \
-lzo-devel \
-make \
-mkisofs \
-ncat \
-ncurses-devel \
-ninja \
-openssh \
-pam-devel \
-perl-Test-Harness \
-perl-base \
-pkgconfig \
-python3-Pillow \
-python3-PyYAML \
-python3-Sphinx \
-python3-base \
-python3-numpy \
-python3-opencv \
-python3-pip \
-python3-setuptools \
-python3-sphinx_rtd_theme \
-python3-virtualenv \
-python3-wheel \
-rdma-core-devel \
-rpm \
-sed \
-snappy-devel \
-sparse \
-spice-protocol-devel \
-systemd-devel \
-systemtap-sdt-devel \
-tar \
-tesseract-ocr \
-tesseract-ocr-traineddata-english \
-texinfo \
-usbredir-devel \
-util-linux \
-virglrenderer-devel \
-vte-devel \
-which \
-xen-devel \
-xfsprogs-devel \
-zlib-devel
-ENV QEMU_CONFIGURE_OPTS --python=/usr/bin/python3.6
+RUN zypper update -y && \
+zypper install -y \
+   Mesa-devel \
+   alsa-lib-devel \
+   bash \
+   bc \
+   brlapi-devel \
+   bzip2 \
+   ca-certificates \
+   ccache \
+   clang \
+   ctags \
+   cyrus-sasl-devel \
+   dbus-1 \
+   diffutils \
+   findutils \
+   gcc \
+   gcc-c++ \
+   gcovr \
+   gettext-runtime \
+   git \
+   glib2-devel \
+   glibc-locale \
+   glibc-static \
+   glusterfs-devel \
+   gtk3-devel \
+   hostname \
+   jemalloc-devel \
+   libSDL2-devel \
+   libSDL2_image-devel \
+   libaio-devel \
+   libasan6 \
+   libattr-devel \
+   libbpf-devel \
+   libbz2-devel \
+   libcacard-devel \
+   libcap-ng-devel \
+   libcurl-devel \
+   libdrm-devel \
+   libepoxy-devel \
+   libfdt-devel \
+   libffi-devel \
+   libgcrypt-devel \
+   libgnutls-devel \
+   libiscsi-devel \
+   libjpeg8-devel \
+   libndctl-devel \
+   libnettle-devel \
+   libnfs-devel \
+   libnuma-devel \
+   libpixman-1-0-devel \
+   libpmem-devel \
+   libpng16-devel \
+   libpulse-devel \
+   librbd-devel \
+   libseccomp-devel \
+   libselinux-devel \
+   libspice-server-devel \
+   libssh-devel \
+   libtasn1-devel \
+   libubsan1 \
+   libudev-devel \
+   liburing-devel \
+   libusb-1_0-devel \
+   libxml2-devel \
+   libzstd-devel \
+   llvm \
+   lttng-ust-devel \
+   lzo-devel \
+   make \
+   mkisofs \
+   ncat \
+   ncurses-devel \
+   ninja \
+   openssh \
+   pam-devel \
+   

[PATCH v5 10/18] tests/docker: auto-generate ubuntu1804.docker with lcitool

2021-12-15 Thread Daniel P . Berrangé
This commit is best examined using the "-b" option to diff.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/docker/dockerfiles/ubuntu1804.docker | 255 -
 tests/lcitool/refresh  |   7 +
 2 files changed, 149 insertions(+), 113 deletions(-)

diff --git a/tests/docker/dockerfiles/ubuntu1804.docker 
b/tests/docker/dockerfiles/ubuntu1804.docker
index 450fd06d0d..0ffa3c4d4b 100644
--- a/tests/docker/dockerfiles/ubuntu1804.docker
+++ b/tests/docker/dockerfiles/ubuntu1804.docker
@@ -1,117 +1,146 @@
+# THIS FILE WAS AUTO-GENERATED
+#
+#  $ lcitool dockerfile ubuntu-1804 qemu
+#
+# https://gitlab.com/libvirt/libvirt-ci
+
 FROM docker.io/library/ubuntu:18.04
-ENV PACKAGES \
-bc \
-bsdmainutils \
-bzip2 \
-ca-certificates \
-ccache \
-clang \
-dbus \
-debianutils \
-diffutils \
-exuberant-ctags \
-findutils \
-g++ \
-gcc \
-gcovr \
-genisoimage \
-gettext \
-git \
-glusterfs-common \
-hostname \
-libaio-dev \
-libasan5 \
-libasound2-dev \
-libattr1-dev \
-libbrlapi-dev \
-libbz2-dev \
-libc6-dev \
-libcacard-dev \
-libcap-ng-dev \
-libcapstone-dev \
-libcurl4-gnutls-dev \
-libdaxctl-dev \
-libdrm-dev \
-libepoxy-dev \
-libfdt-dev \
-libffi-dev \
-libgbm-dev \
-libgcrypt20-dev \
-libglib2.0-dev \
-libgnutls28-dev \
-libgtk-3-dev \
-libibverbs-dev \
-libiscsi-dev \
-libjemalloc-dev \
-libjpeg-turbo8-dev \
-liblttng-ust-dev \
-liblzo2-dev \
-libncursesw5-dev \
-libnfs-dev \
-libnuma-dev \
-libpam0g-dev \
-libpixman-1-dev \
-libpmem-dev \
-libpng-dev \
-libpulse-dev \
-librbd-dev \
-librdmacm-dev \
-libsasl2-dev \
-libsdl2-dev \
-libsdl2-image-dev \
-libseccomp-dev \
-libselinux-dev \
-libsnappy-dev \
-libspice-protocol-dev \
-libspice-server-dev \
-libssh-dev \
-libsystemd-dev \
-libtasn1-6-dev \
-libtest-harness-perl \
-libubsan1 \
-libudev-dev \
-libusb-1.0-0-dev \
-libusbredirhost-dev \
-libvdeplug-dev \
-libvirglrenderer-dev \
-libvte-2.91-dev \
-libxen-dev \
-libxml2-dev \
-libzstd-dev \
-llvm \
-locales \
-make \
-multipath-tools \
-netcat-openbsd \
-nettle-dev \
-ninja-build \
-openssh-client \
-perl-base \
-pkgconf \
-python3 \
-python3-numpy \
-python3-opencv \
-python3-pillow \
-python3-pip \
-python3-setuptools \
-python3-sphinx \
-python3-sphinx-rtd-theme \
-python3-venv \
-python3-wheel \
-python3-yaml \
-rpm2cpio \
-sed \
-sparse \
-systemtap-sdt-dev \
-tar \
-tesseract-ocr \
-tesseract-ocr-eng \
-texinfo \
-xfslibs-dev \
-zlib1g-dev
-RUN apt-get update && \
-DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
-RUN dpkg -l $PACKAGES | sort > /packages.txt
 
+RUN export DEBIAN_FRONTEND=noninteractive && \
+apt-get update && \
+apt-get install -y eatmydata && \
+eatmydata apt-get dist-upgrade -y && \
+eatmydata apt-get install --no-install-recommends -y \
+bash \
+bc \
+bsdmainutils \
+bzip2 \
+ca-certificates \
+ccache \
+clang \
+dbus \
+debianutils \
+diffutils \
+exuberant-ctags \
+findutils \
+g++ \
+gcc \
+gcovr \
+genisoimage \
+gettext \
+git \
+glusterfs-common \
+hostname \
+libaio-dev \
+libasan5 \
+libasound2-dev \
+libattr1-dev \
+libbrlapi-dev \
+libbz2-dev \
+libc6-dev \
+libcacard-dev \
+libcap-ng-dev \
+libcapstone-dev \
+libcurl4-gnutls-dev \
+libdaxctl-dev \
+libdrm-dev \
+libepoxy-dev \
+libfdt-dev \
+libffi-dev \
+libgbm-dev \
+libgcrypt20-dev \
+libglib2.0-dev \
+libgnutls28-dev \
+libgtk-3-dev \
+libibverbs-dev \
+libiscsi-dev \
+libjemalloc-dev \
+libjpeg-turbo8-dev \
+liblttng-ust-dev \
+liblzo2-dev \
+libncursesw5-dev \
+libnfs-dev \
+libnuma-dev \
+libpam0g-dev \
+libpcre2-dev \
+libpixman-1-dev \
+libpmem-dev \
+libpng-dev \
+libpulse-dev \
+librbd-dev \
+librdmacm-dev \
+libsasl2-dev \
+libsdl2-dev \
+libsdl2-image-dev \
+libseccomp-dev \
+libselinux1-dev

[PATCH v5 06/18] tests/docker: switch fedora image to release 35

2021-12-15 Thread Daniel P . Berrangé
The Fedora 33 release is shortly end of life. Switch to the newest
Fedora 35 to maximise lifespan until we need to update again.

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/docker/dockerfiles/fedora.docker | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/dockerfiles/fedora.docker 
b/tests/docker/dockerfiles/fedora.docker
index c6fd7e1113..855aefaac5 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -1,4 +1,4 @@
-FROM registry.fedoraproject.org/fedora:33
+FROM registry.fedoraproject.org/fedora:35
 
 # Please keep this list sorted alphabetically
 ENV PACKAGES \
-- 
2.33.1




[PATCH v5 15/18] tests/docker: updates to alpine package list

2021-12-15 Thread Daniel P . Berrangé
Cleanup the package lists by removing some entries that we don't need to
directly reference

  binutils: implied by the compiler toolchain
  coreutils: not required by QEMU build
  mesa-egl mesa-gbm: implied by mesa-dev
  ninja: alias for samurai package
  shadow: not required by QEMU build
  util-linux-dev: not directly required by QEMU build

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/docker/dockerfiles/alpine.docker | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/tests/docker/dockerfiles/alpine.docker 
b/tests/docker/dockerfiles/alpine.docker
index 7e6997e301..5a1808726e 100644
--- a/tests/docker/dockerfiles/alpine.docker
+++ b/tests/docker/dockerfiles/alpine.docker
@@ -8,9 +8,7 @@ RUN apk upgrade
 ENV PACKAGES \
alsa-lib-dev \
bash \
-   binutils \
ccache \
-   coreutils \
curl-dev \
g++ \
gcc \
@@ -33,22 +31,18 @@ ENV PACKAGES \
lzo-dev \
make \
mesa-dev \
-   mesa-egl \
-   mesa-gbm \
meson \
ncurses-dev \
-   ninja \
perl \
pulseaudio-dev \
python3 \
py3-sphinx \
py3-sphinx_rtd_theme \
-   shadow \
+   samurai \
snappy-dev \
spice-dev \
texinfo \
usbredir-dev \
-   util-linux-dev \
vde2-dev \
virglrenderer-dev \
vte3-dev \
-- 
2.33.1




[PATCH v5 04/18] ui: avoid warnings about directdb on Alpine / musl libc

2021-12-15 Thread Daniel P . Berrangé
On Alpine, SDL is built with directfb support and this triggers warnings
during QEMU build

In file included from /usr/include/directfb/direct/thread.h:38,
 from /usr/include/directfb/direct/debug.h:43,
 from /usr/include/directfb/direct/interface.h:36,
 from /usr/include/directfb/directfb.h:49,
 from /usr/include/SDL2/SDL_syswm.h:80,
 from /builds/berrange/qemu/include/ui/sdl2.h:8,
 from ../ui/sdl2-gl.c:31:
/usr/include/directfb/direct/os/waitqueue.h:41:25: error: redundant 
redeclaration of 'direct_waitqueue_init' [-Werror=redundant-decls]
   41 | DirectResult DIRECT_API direct_waitqueue_init( DirectWaitQueue 
*queue );
  | ^

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 include/ui/sdl2.h | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/include/ui/sdl2.h b/include/ui/sdl2.h
index f85c117a78..0330978df8 100644
--- a/include/ui/sdl2.h
+++ b/include/ui/sdl2.h
@@ -5,7 +5,18 @@
 #undef WIN32_LEAN_AND_MEAN
 
 #include 
+
+/* with Alpine / muslc SDL headers pull in directfb headers
+ * which in turn trigger warning about redundant decls for
+ * direct_waitqueue_deinit.
+ */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wredundant-decls"
+
 #include 
+
+#pragma GCC diagnostic pop
+
 #ifdef CONFIG_SDL_IMAGE
 # include 
 #endif
-- 
2.33.1




[PATCH v5 05/18] ci: explicitly skip I/O tests on alpine

2021-12-15 Thread Daniel P . Berrangé
The block I/O tests don't work on Alpine because their alternative libc
impl emits different strings for errnos, which breaks the expected
output matching. e.g.

=== IO: pattern 102
 wrote 512/512 bytes at offset 512
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-img: Error while reading offset 0 of 
blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT: Input/output error
+qemu-img: Error while reading offset 0 of 
blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT: I/O error
 4
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 Formatting 'TEST_DIR/t.IMGFMT.2', fmt=IMGFMT size=0

Currently the I/O tests are skipped as a side effect of the Alpine image
containing a minimal busybox 'sed' binary, rather than GNU sed. This is
a fragile assumption that will be invalidated when the dockerfile is
changed to be autogenerated from a standardized package list that
includes GNU sed.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.d/buildtest.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index 71d0f407ad..e1fe37e563 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -24,7 +24,7 @@ check-system-alpine:
   artifacts: true
   variables:
 IMAGE: alpine
-MAKE_CHECK_ARGS: check
+MAKE_CHECK_ARGS: check-unit check-qtest
 
 avocado-system-alpine:
   extends: .avocado_test_job_template
-- 
2.33.1




[PATCH v5 09/18] tests/docker: auto-generate fedora.docker with lcitool

2021-12-15 Thread Daniel P . Berrangé
This commit is best examined using the "-b" option to diff.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniel P. Berrangé 
---
 tests/docker/dockerfiles/fedora.docker | 260 ++---
 tests/lcitool/refresh  |   1 +
 2 files changed, 146 insertions(+), 115 deletions(-)

diff --git a/tests/docker/dockerfiles/fedora.docker 
b/tests/docker/dockerfiles/fedora.docker
index 855aefaac5..6784878b56 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -1,118 +1,148 @@
+# THIS FILE WAS AUTO-GENERATED
+#
+#  $ lcitool dockerfile fedora-35 qemu
+#
+# https://gitlab.com/libvirt/libvirt-ci
+
 FROM registry.fedoraproject.org/fedora:35
 
-# Please keep this list sorted alphabetically
-ENV PACKAGES \
-SDL2-devel \
-SDL2_image-devel \
-alsa-lib-devel \
-bc \
-brlapi-devel \
-bzip2 \
-bzip2-devel \
-ca-certificates \
-capstone-devel \
-ccache \
-clang \
-ctags \
-cyrus-sasl-devel \
-daxctl-devel \
-dbus-daemon \
-device-mapper-multipath-devel \
-diffutils \
-findutils \
-gcc \
-gcc-c++ \
-gcovr \
-genisoimage \
-gettext \
-git \
-glib2-devel \
-glibc-langpack-en \
-glibc-static \
-glusterfs-api-devel \
-gnutls-devel \
-gtk3-devel \
-hostname \
-jemalloc-devel \
-libaio-devel \
-libasan \
-libattr-devel \
-libbpf-devel \
-libcacard-devel \
-libcap-ng-devel \
-libcurl-devel \
-libdrm-devel \
-libepoxy-devel \
-libfdt-devel \
-libffi-devel \
-libgcrypt-devel \
-libiscsi-devel \
-libjpeg-devel \
-libnfs-devel \
-libpmem-devel \
-libpng-devel \
-librbd-devel \
-libseccomp-devel \
-libselinux-devel \
-libslirp-devel \
-libssh-devel \
-libtasn1-devel \
-libubsan \
-libudev-devel \
-liburing-devel \
-libusbx-devel \
-libxml2-devel \
-libzstd-devel \
-llvm \
-lttng-ust-devel \
-lzo-devel \
-make \
-mesa-libgbm-devel \
-meson \
-ncurses-devel \
-nettle-devel \
-ninja-build \
-nmap-ncat \
-numactl-devel \
-openssh-clients \
-pam-devel \
-perl-Test-Harness \
-perl-base \
-pixman-devel \
-pkgconfig \
-pulseaudio-libs-devel \
-python3 \
-python3-PyYAML \
-python3-numpy \
-python3-opencv \
-python3-pillow \
-python3-pip \
-python3-sphinx \
-python3-sphinx_rtd_theme \
-python3-virtualenv \
-rdma-core-devel \
-rpm \
-sed \
-snappy-devel \
-sparse \
-spice-protocol \
-spice-server-devel \
-systemd-devel \
-systemtap-sdt-devel \
-tar \
-tesseract \
-tesseract-langpack-eng \
-texinfo \
-usbredir-devel \
-util-linux \
-virglrenderer-devel \
-vte291-devel \
-which \
-xen-devel \
-xfsprogs-devel \
-zlib-devel
-ENV QEMU_CONFIGURE_OPTS --python=/usr/bin/python3
+RUN dnf install -y nosync && \
+echo -e '#!/bin/sh\n\
+if test -d /usr/lib64\n\
+then\n\
+export LD_PRELOAD=/usr/lib64/nosync/nosync.so\n\
+else\n\
+export LD_PRELOAD=/usr/lib/nosync/nosync.so\n\
+fi\n\
+exec "$@"' > /usr/bin/nosync && \
+chmod +x /usr/bin/nosync && \
+nosync dnf update -y && \
+nosync dnf install -y \
+SDL2-devel \
+SDL2_image-devel \
+alsa-lib-devel \
+bash \
+bc \
+brlapi-devel \
+bzip2 \
+bzip2-devel \
+ca-certificates \
+capstone-devel \
+ccache \
+clang \
+ctags \
+cyrus-sasl-devel \
+daxctl-devel \
+dbus-daemon \
+device-mapper-multipath-devel \
+diffutils \
+findutils \
+gcc \
+gcc-c++ \
+gcovr \
+genisoimage \
+gettext \
+git \
+glib2-devel \
+glib2-static \
+glibc-langpack-en \
+glibc-static \
+glusterfs-api-devel \
+gnutls-devel \
+gtk3-devel \
+hostname \
+jemalloc-devel \
+libaio-devel \
+libasan \
+libattr-devel \
+libbpf-devel \
+libcacard-devel \
+libcap-ng-devel \
+libcurl-devel \
+libdrm-devel \
+libepoxy-devel \
+libfdt-devel \
+libffi-devel \
+libgcrypt-devel \
+libiscsi-devel \
+libjpeg-devel \
+libnfs-devel \
+libpmem-devel \
+libpng-devel \
+librbd-devel \
+libseccomp-devel \
+libselinux-devel \
+libslirp-devel \
+libssh-devel \
+libtasn1-devel \
+libubsan \
+libudev-devel \
+liburing-devel \
+libusbx-devel \
+libxml2-devel \
+libzstd-devel \
+llvm \
+lttng-ust-devel \
+lzo-devel \
+make \
+mesa-libgbm-devel \
+   

Re: [PATCH v4 00/18] tests/docker: start using libvirt-ci's "lcitool" for dockerfiles

2021-12-15 Thread Daniel P . Berrangé
On Wed, Nov 24, 2021 at 01:01:32PM +, Daniel P. Berrangé wrote:
> One unsolved problem here is that the rebase to Fedora 35 has
> caused the oss-fuzz job in CI to reliably fail with a timeout.
> Running locally on F35 the fuzz script never finishes even after
> 24 hours. There's a genuine problem hiding in there in the
> tests or code itself, that newer clang has likely exposed.

Well something has changed in the last month - either Fedora 35
got some fixes or something in QEMU got fixed. The oss-fuzz
jobs now pass on Fedora 35 reliably both locally and in CI.


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 v4 15/18] tests/docker: updates to alpine package list

2021-12-15 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 12:31:33PM +0100, Philippe Mathieu-Daudé wrote:
> On 11/24/21 14:01, Daniel P. Berrangé wrote:
> > Cleanup the package lists by removing some entries that we don't need to
> > directly reference
> > 
> >   binutils: implied by the compiler toolchain
> >   coreutils: not required by QEMU build
> >   mesa-egl mesa-gbm: implied by mesa-dev
> >   ninja: alias for samurai package
> 
> I'd rather keep the alias to avoid looking for ninja or
> for what samurai is. Anyhow,

This change was done to align with the package lists that
lcitool will generate. In lcitool we always aim to use the
canonical package name, rather than any alias, so it is
explicit what impl we're using for something.

> Reviewed-by: Philippe Mathieu-Daudé 
> 
> >   shadow: not required by QEMU build
> >   util-linux-dev: not directly required by QEMU build
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  tests/docker/dockerfiles/alpine.docker | 8 +---
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> 

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: Building QEMU as a shared library

2021-12-15 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 09:45:56AM +, Stefan Hajnoczi wrote:
> On Wed, Dec 15, 2021 at 08:18:53AM +, Amir Gonnen wrote:
> > Before sending a patch, I would like to check if it's of interest to the 
> > community.
> > 
> > My goal is to simulate a mixed architecture system.
> > Today QEMU strongly assumes that the simulated system is a *single 
> > architecture*.
> > Changing this assumption and supporting mixed architecture in QEMU proved 
> > to be
> > non-trivial and may require significant development effort. Common code 
> > such as
> > TCG and others explicitly include architecture specific header files, for 
> > example.
> 
> Hi Amir,
> Simulating heterogenous machines comes up from periodically. So far no
> one has upstreamed a solution but there is definitely interest.
> 
> I suggest going ahead and posting the code even if it's not cleaned up.
> 
> > A possible solution, discussed on 
> > https://stackoverflow.com/q/63229262/619493 is to
> > separate the simulation to multiple processes (as done by Xilinx) and to 
> > use some form
> > of Interprocess Communication channel between them.
> > Such solution has several disadvantages:
> > 
> > - Harder to synchronize simulation between processes
> > - Performance impact of Interprocess Communication
> > - Harder to debug, profile and maintain
> > 
> > Instead, I would like to suggest a new approach we use at Neuroblade to 
> > achieve this:
> > Build QEMU as a shared library that can be loaded and used directly in a 
> > larger simulation.
> > Today we build qemu-system-nios2 shared library and load it from 
> > qemu-system-x86_64 in order
> > to simulate an x86_64 system that also consists of multiple nios2 cores.
> > In our simulation, two independent "main" functions are running on 
> > different threads, and
> > simulation synchronization is reduced to synchronizing threads.
> > 
> > To achieve this, I needed to do the following changes in QEMU:
> > 
> > 1. Avoid Glib global context. Use a different context (g_main_context_new) 
> > for each QEMU instance.
> > 2. Change meson.build to build QEMU as a shared library (with PIC enabled 
> > for static libraries)
> > 3. Define a C API for the library and export it (with a 
> > -Wl,--version-script)

I'm curious what kind of scope the C API has and whether it is likely to
conflict with any general plans we have for future evolution to QEMU's
design, especially around configuration / CLI.

In your example where the client loading the QEMU shared library is
QEMU itself, the licensing doesn't become an issue, as everything
is within the same project codebase.

If the shared library were exposed to consumers outside of the
QEMU project, then licensing is a significant stumbling block,
as any consumer has to be GPL-v2-only compatible. It is pretty
easy to get into a license incompatibility situation with this
requirement. I fear alot of people would just ignore this as an
"inconvenience" and knowingly use incompatible code.

Both of these issues could be avoided if we make any shuared
library a QEMU-internal thing, by not installing any headers
and doing something to ensure its ABI changes on every build.

> > These changes seem enough for simulating mixed architecture system on a 
> > single process.
> > 
> > If this approach sounds useful, I'll be happy to send patches.
> > I'd appreciate if you could provide your feedback!
> 
> I'm curious how much synchronization and IPC there is between the QEMU
> shared libraries? I would have guessed that the pain of making
> communication work efficiently between processes would be less than the
> pain of solving global state bugs related to shared libraries within a
> single process.

We've already got ability to have device backends run in separate
processes with vhost-user, and have various other efforts underway
to support more generalized out of tree device implementations.
With this direction in mind, it feels conceptually desirable  if
we are able to support heterogenous CPU emulation using co-operating
processes too, as opposed to a monolithic process architecture.

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: Building QEMU as a shared library

2021-12-15 Thread Daniel P . Berrangé
On Wed, Dec 15, 2021 at 10:10:35AM +, Peter Maydell wrote:
> On Wed, 15 Dec 2021 at 08:18, Amir Gonnen  wrote:
> > My goal is to simulate a mixed architecture system.
> >
> > Today QEMU strongly assumes that the simulated system is a *single 
> > architecture*.
> > Changing this assumption and supporting mixed architecture in QEMU proved 
> > to be
> > non-trivial and may require significant development effort. Common code 
> > such as
> > TCG and others explicitly include architecture specific header files, for 
> > example.
> 
> Yeah. This is definitely something we'd like to fix some day. It's
> the approach I would prefer for getting multi-architecture machines.
> 
> > Instead, I would like to suggest a new approach we use at Neuroblade to 
> > achieve this:
> > Build QEMU as a shared library that can be loaded and used directly in a 
> > larger simulation.
> > Today we build qemu-system-nios2 shared library and load it from 
> > qemu-system-x86_64 in order
> > to simulate an x86_64 system that also consists of multiple nios2 cores.
> > In our simulation, two independent "main" functions are running on 
> > different threads, and
> > simulation synchronization is reduced to synchronizing threads.
> 
> I agree with Stefan that you should go ahead and send the code as
> an RFC patchset, but I feel like there is a lot of work required
> to really get the codebase into a state where it is a clean
> shared library...

I expect there could end up being a big difference between

   "clean for use with QEMU CLI config X"

vs

   "clean for use with all possible QEMU CLI configs"


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 00/47] Patch Round-up for stable 6.1.1, freeze on 2021-12-21

2021-12-15 Thread Daniel P . Berrangé
On Tue, Dec 14, 2021 at 06:00:38PM -0600, Michael Roth wrote:
> Hi everyone,
> 
> The following new patches are queued for QEMU stable v6.1.1:
> 
>   https://gitlab.com/qemu-project/qemu/-/commits/stable-6.1-staging/

FYI, this branch doesn't appear to have been pushed.

> Patch freeze is 2021-12-21, and the release is planned for 2021-12-23:
> 
>   https://wiki.qemu.org/Planning/6.1
> 
> Please respond here or CC qemu-sta...@nongnu.org on any additional patches
> you think should (or shouldn't) be included in the release.

Based on critical fixes Fedora users hit in 6.1 we pulled in
the following fixes that you've not queued yet:

  eb94846280df3f1e2a91b6179fc05f9890b7e384 qxl: fix pre-save logic

  69e3895f9d37ca39536775b13ce63e8c291427ba target/i386: add missing bits to 
CR4_RESERVED_MASK

  b9537d5904f6e3df896264a6144883ab07db9608 tcg/arm: Reduce vector alignment 
requirement for NEON

  8e751e9c38e324737fd3d3aa0562f886313bba3c tests: tcg: Fix PVH test with 
binutils 2.36+

> 
> Ani Sinha (6):
>   bios-tables-test: allow changes in DSDT ACPI tables for q35
>   hw/i386/acpi: fix conflicting IO address range for acpi pci hotplug in 
> q35
>   bios-tables-test: Update ACPI DSDT table golden blobs for q35
>   tests/acpi/bios-tables-test: add and allow changes to a new q35 DSDT 
> table blob
>   tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges 
> for q35
>   tests/acpi/bios-tables-test: update DSDT blob for multifunction bridge 
> test
> 
> Ari Sundholm (1):
>   block/file-posix: Fix return value translation for AIO discards
> 
> Christian Schoenebeck (1):
>   9pfs: fix crash in v9fs_walk()
> 
> Daniil Tatianin (1):
>   chardev/wctable: don't free the instance in wctablet_chr_finalize
> 
> David Hildenbrand (3):
>   virtio-balloon: don't start free page hinting if postcopy is possible
>   virtio-mem-pci: Fix memory leak when creating MEMORY_DEVICE_SIZE_CHANGE 
> event
>   libvhost-user: fix VHOST_USER_REM_MEM_REG skipping mmap_addr
> 
> Eric Blake (1):
>   nbd/server: Don't complain on certain client disconnects
> 
> Gerd Hoffmann (1):
>   uas: add stream number sanity checks.
> 
> Greg Kurz (2):
>   rcu: Introduce force_rcu notifier
>   accel/tcg: Register a force_rcu notifier
> 
> Helge Deller (1):
>   hw/display/artist: Fix bug in coordinate extraction in 
> artist_vram_read() and artist_vram_write()
> 
> Igor Mammedov (1):
>   pcie: rename 'native-hotplug' to 'x-native-hotplug'
> 
> Jason Wang (3):
>   virtio-net: fix use after unmap/free for sg
>   virtio: use virtio accessor to access packed descriptor flags
>   virtio: use virtio accessor to access packed event
> 
> Jean-Philippe Brucker (2):
>   hw/arm/virt: Rename default_bus_bypass_iommu
>   hw/i386: Rename default_bus_bypass_iommu
> 
> Jessica Clarke (1):
>   Partially revert "build: -no-pie is no functional linker flag"
> 
> Jon Maloy (1):
>   e1000: fix tx re-entrancy problem
> 
> Klaus Jensen (1):
>   hw/nvme: fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)
> 
> Laurent Vivier (1):
>   hw: m68k: virt: Add compat machine for 6.1
> 
> Mahmoud Mandour (1):
>   plugins/execlog: removed unintended "s" at the end of log lines.
> 
> Mark Mielke (1):
>   virtio-blk: Fix clean up of host notifiers for single MR transaction.
> 
> Markus Armbruster (1):
>   hmp: Unbreak "change vnc"
> 
> Mauro Matteo Cascella (1):
>   hw/scsi/scsi-disk: MODE_PAGE_ALLS not allowed in MODE SELECT commands
> 
> Michael S. Tsirkin (1):
>   pci: fix PCI resource reserve capability on BE
> 
> Michael Tokarev (1):
>   qemu-sockets: fix unix socket path copy (again)
> 
> Nir Soffer (1):
>   qemu-nbd: Change default cache mode to writeback
> 
> Paolo Bonzini (4):
>   plugins: do not limit exported symbols if modules are active
>   block: introduce max_hw_iov for use in scsi-generic
>   target-i386: mmu: use pg_mode instead of HF_LMA_MASK
>   target-i386: mmu: fix handling of noncanonical virtual addresses
> 
> Peng Liang (1):
>   vfio: Fix memory leak of hostwin
> 
> Peter Maydell (1):
>   target/arm: Don't skip M-profile reset entirely in user mode
> 
> Philippe Mathieu-Daudé (3):
>   hw/block/fdc: Extract blk_create_empty_drive()
>   hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196
>   tests/qtest/fdc-test: Add a regression test for CVE-2021-20196
> 
> Prasad J Pandit (1):
>   net: vmxnet3: validate configuration values during activate 
> (CVE-2021-20203)
> 
> Stefano Garzarella (1):
>   vhost-vsock: fix migration issue when seqpacket is supported
> 
> Xueming Li (1):
>   vhost-user: fix duplicated notifier MR init
> 
> Yang Zhong (1):
>   i386/cpu: Remove AVX_VNNI feature from Cooperlake cpu model
> 
>  accel/tcg/tcg-accel-ops-mttcg.c   |  26 
>  

Re: [PATCH v2] monitor: move x-query-profile into accel/tcg to fix build

2021-12-15 Thread Daniel P . Berrangé
On Tue, Dec 14, 2021 at 07:50:48PM +, Alex Bennée wrote:
> As --enable-profiler isn't defended in CI we missed this breakage.
> Move the qmp handler into accel/tcg so we have access to the helpers
> we need. While we are at it ensure we gate the feature on CONFIG_TCG.
> 
> Signed-off-by: Alex Bennée 
> Suggested-by: Daniel P. Berrangé 
> Reported-by: Mark Cave-Ayland 
> Fixes: 37087fde0e ("qapi: introduce x-query-profile QMP command")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/773
> 
> ---
> v2
>   - enclosed in #ifndef CONFIG_USER_ONLY section
> ---
>  qapi/machine.json|  1 +
>  accel/tcg/cpu-exec.c | 31 +++
>  monitor/qmp-cmds.c   | 31 ---
>  3 files changed, 32 insertions(+), 31 deletions(-)

Reviewed-by: Daniel P. Berrangé 

Sorry about the mess up I introduced in refactoring, and thanks for
fixing it for me.

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] monitor: move x-query-profile into accel/tcg to fix build

2021-12-14 Thread Daniel P . Berrangé
On Tue, Dec 14, 2021 at 06:22:07PM +, Alex Bennée wrote:
> As --enable-profiler isn't defended in CI we missed this breakage.
> Move the qmp handler into accel/tcg so we have access to the helpers
> we need. While we are at it ensure we gate the feature on CONFIG_TCG.
> 
> Signed-off-by: Alex Bennée 
> Suggested-by: Daniel P. Berrangé 
> Reported-by: Mark Cave-Ayland 
> Fixes: 37087fde0e ("qapi: introduce x-query-profile QMP command")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/773
> ---
>  qapi/machine.json|  1 +
>  accel/tcg/cpu-exec.c | 31 +++
>  monitor/qmp-cmds.c   | 31 ---
>  3 files changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 067e3f5378..0c9f24a712 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1492,6 +1492,7 @@
>  ##
>  { 'command': 'x-query-profile',
>'returns': 'HumanReadableText',
> +  'if': 'CONFIG_TCG',
>'features': [ 'unstable' ] }
>  
>  ##
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 409ec8c38c..9498a16681 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -1091,3 +1091,34 @@ HumanReadableText *qmp_x_query_opcount(Error **errp)
>  }
>  
>  #endif /* !CONFIG_USER_ONLY */

I think this #endif probably needs to be after the qmp_x_query_profile
impl, as it is for the other TCG QMP cmds  ?

> +
> +#ifdef CONFIG_PROFILER
> +
> +int64_t dev_time;
> +
> +HumanReadableText *qmp_x_query_profile(Error **errp)
> +{
> +g_autoptr(GString) buf = g_string_new("");
> +static int64_t last_cpu_exec_time;
> +int64_t cpu_exec_time;
> +int64_t delta;
> +
> +cpu_exec_time = tcg_cpu_exec_time();
> +delta = cpu_exec_time - last_cpu_exec_time;
> +
> +g_string_append_printf(buf, "async time  %" PRId64 " (%0.3f)\n",
> +   dev_time, dev_time / 
> (double)NANOSECONDS_PER_SECOND);
> +g_string_append_printf(buf, "qemu time   %" PRId64 " (%0.3f)\n",
> +   delta, delta / (double)NANOSECONDS_PER_SECOND);
> +last_cpu_exec_time = cpu_exec_time;
> +dev_time = 0;
> +
> +return human_readable_text_from_str(buf);
> +}
> +#else
> +HumanReadableText *qmp_x_query_profile(Error **errp)
> +{
> +error_setg(errp, "Internal profiler not compiled");
> +return NULL;
> +}
> +#endif
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 343353e27a..be5e44c569 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -355,37 +355,6 @@ void qmp_display_reload(DisplayReloadOptions *arg, Error 
> **errp)
>  }
>  }
>  
> -#ifdef CONFIG_PROFILER
> -
> -int64_t dev_time;
> -
> -HumanReadableText *qmp_x_query_profile(Error **errp)
> -{
> -g_autoptr(GString) buf = g_string_new("");
> -static int64_t last_cpu_exec_time;
> -int64_t cpu_exec_time;
> -int64_t delta;
> -
> -cpu_exec_time = tcg_cpu_exec_time();
> -delta = cpu_exec_time - last_cpu_exec_time;
> -
> -g_string_append_printf(buf, "async time  %" PRId64 " (%0.3f)\n",
> -   dev_time, dev_time / 
> (double)NANOSECONDS_PER_SECOND);
> -g_string_append_printf(buf, "qemu time   %" PRId64 " (%0.3f)\n",
> -   delta, delta / (double)NANOSECONDS_PER_SECOND);
> -last_cpu_exec_time = cpu_exec_time;
> -dev_time = 0;
> -
> -return human_readable_text_from_str(buf);
> -}
> -#else
> -HumanReadableText *qmp_x_query_profile(Error **errp)
> -{
> -error_setg(errp, "Internal profiler not compiled");
> -return NULL;
> -}
> -#endif
> -
>  static int qmp_x_query_rdma_foreach(Object *obj, void *opaque)
>  {
>  RdmaProvider *rdma;
> -- 
> 2.30.2
> 

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] docs: Add measurement calculation details to amd-memory-encryption.txt

2021-12-14 Thread Daniel P . Berrangé
On Tue, Dec 14, 2021 at 01:59:10PM +, Dov Murik wrote:
> Add a section explaining how the Guest Owner should calculate the
> expected guest launch measurement for SEV and SEV-ES.
> 
> Also update the name and link to the SEV API Spec document.
> 
> Signed-off-by: Dov Murik 
> Suggested-by: Daniel P. Berrangé 
> ---
>  docs/amd-memory-encryption.txt | 50 +++---
>  1 file changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
> index ffca382b5f..f97727482f 100644
> --- a/docs/amd-memory-encryption.txt
> +++ b/docs/amd-memory-encryption.txt
> @@ -43,7 +43,7 @@ The guest policy is passed as plaintext. A hypervisor may 
> choose to read it,
>  but should not modify it (any modification of the policy bits will result
>  in bad measurement). The guest policy is a 4-byte data structure containing
>  several flags that restricts what can be done on a running SEV guest.
> -See KM Spec section 3 and 6.2 for more details.
> +See SEV API Spec [1] section 3 and 6.2 for more details.
>  
>  The guest policy can be provided via the 'policy' property (see below)
>  
> @@ -88,7 +88,7 @@ expects.
>  LAUNCH_FINISH finalizes the guest launch and destroys the cryptographic
>  context.
>  
> -See SEV KM API Spec [1] 'Launching a guest' usage flow (Appendix A) for the
> +See SEV API Spec [1] 'Launching a guest' usage flow (Appendix A) for the
>  complete flow chart.
>  
>  To launch a SEV guest
> @@ -113,6 +113,45 @@ a SEV-ES guest:
>   - Requires in-kernel irqchip - the burden is placed on the hypervisor to
> manage booting APs.
>  
> +Calculating expected guest launch measurement
> +-
> +In order to verify the guest launch measurement, The Guest Owner must compute
> +it in the exact same way as it is calculated by the AMD-SP.  SEV API Spec [1]
> +section 6.5.1 describes the AMD-SP operations:
> +
> +GCTX.LD is finalized, producing the hash digest of all plaintext data
> +imported into the guest.
> +
> +The launch measurement is calculated as:
> +
> +HMAC(0x04 || API_MAJOR || API_MINOR || BUILD || GCTX.POLICY || GCTX.LD 
> || MNONCE; GCTX.TIK)
> +
> +where "||" represents concatenation.
> +
> +The values of API_MAJOR, API_MINOR, BUILD, and GCTX.POLICY can be obtained
> +from the 'query-sev' qmp command.
> +
> +The value of MNONCE is part of the response of 'query-sev-launch-measure': it
> +is the last 16 bytes of the base64-decoded data field (see SEV API Spec [1]
> +section 6.5.2 Table 52: LAUNCH_MEASURE Measurement Buffer).
> +
> +The value of GCTX.LD is SHA256(firmware_blob || kernel_hashes_blob || 
> vmsas_blob),
> +where:
> +
> +* firmware_blob is the content of the entire firmware flash file (for 
> example,
> +  OVMF.fd).

Lets add a caveat that the firmware flash should be built to be stateless
ie that it is not secure to attempt to measure a guest where the firmware
uses an NVRAM store.

> +* if kernel is used, and kernel-hashes=on, then kernel_hashes_blob is the
> +  content of PaddedSevHashTable (including the zero padding), which itself
> +  includes the hashes of kernel, initrd, and cmdline that are passed to the
> +  guest.  The PaddedSevHashTable struct is defined in target/i386/sev.c .
> +* if SEV-ES is enabled (policy & 0x4 != 0), vmsas_blob is the concatenation 
> of
> +  all VMSAs of the guest vcpus.  Each VMSA is 4096 bytes long; its content is
> +  defined inside Linux kernel code as struct vmcb_save_area, or in AMD APM
> +  Volume 2 [2] Table B-2: VMCB Layout, State Save Area.

Is there any practical guidance we can give apps on the way the VMSAs
can be expected to be initialized ? eg can they assume essentially
all fields in vmcb_save_area are 0 initialized except for certain
ones ? Is initialization likely to vary at all across KVM or EDK2
vesions or something ?

> +
> +If kernel hashes are not used, or SEV-ES is disabled, use empty blobs for
> +kernel_hashes_blob and vmsas_blob as needed.


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] Relax X509 CA cert sanity checking

2021-12-14 Thread Daniel P . Berrangé
On Tue, Dec 14, 2021 at 02:55:02PM +, Henry Kleynhans wrote:
> Hi Daniel,
> 
> I agree that this would allow QEMU startup with a broken
> TLS setup.  Maybe the better solution is to only validate
> the chain of trust.  Would a patch that does that be acceptable?

Yes, that would be fine. It was only ever intended to validate
the chain of trust needed for QEMU's usage. It simply never
occurred to me that someone who have extra redundant certs in
their bundle, so I didn't do anything special to handle that.

BTW, there's a decent amount of unit test coverage for this code
in tests/unit/test-crypto-tlscredsx509.c which could be fairly
easily extended to cover the scenarios of extra certs outside
the required chain of trust.

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: Redesign of QEMU startup & initial configuration

2021-12-14 Thread Daniel P . Berrangé
On Tue, Dec 14, 2021 at 03:42:52PM +0100, Mark Burton wrote:
> I think we’re talking at cross purposes, and probably we agree (not sure). 
> I’ll top quote to try and explain my point of view.
> 
> I think there are two discussions being mixed:
> 1/ A discussion about improving the CLI. (Having a new one, etc etc)
> 2/ A discussion about supporting a low level, and complete, API that can be 
> used by “management layers” (QAPI).
> 
> I think this also gets mixed up with the discussion on migrating the CLI to 
> use the low level API.
> 
> I want to focus on the low level API. 
> 
> I dont see why we’re discussing a ‘high level’ thing when, for now, we have 
> to support the CLI, and we have work to do on QAPI.

We're discussing both because we're setting out what our end goal is
to be, and that end goal should be expected to cover both use cases.

> If somebody wants to build a new CLI, with a new ‘high level’
> interface, using QAPI - let them!

This is too weak of a statement, as it implies that a replacement
high level interface is optional and not important for the overall
project. I don't believe that to be the case, so I'm saying that
our design & impl plan has to demonstrate how we intend to cover
both deliverables or use cases. We can't simply ignore the high
level API saying it is someone else's problem to worry about.

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: Redesign of QEMU startup & initial configuration

2021-12-14 Thread Daniel P . Berrangé
On Tue, Dec 14, 2021 at 02:36:26PM +0100, Mark Burton wrote:
> 
> 
> > On 14 Dec 2021, at 14:21, Daniel P. Berrangé  wrote:
> > 
> > On Tue, Dec 14, 2021 at 02:11:11PM +0100, Mark Burton wrote:
> >> 
> >> 
> >>> On 14 Dec 2021, at 14:05, Daniel P. Berrangé  wrote:
> >>> 
> >>> On Mon, Dec 13, 2021 at 09:22:14PM +0100, Mark Burton wrote:
> >>>> 
> >>>> 
> >>>>> On 13 Dec 2021, at 18:59, Daniel P. Berrangé  
> >>>>> wrote:
> >>>>> 
> >>>>> …. we no longer have to solve everything
> >>>>> Ourselves. 
> >>>> 
> >>>> I support this sentiment.
> >>>> 
> >>>> Lets re-factor the code so people can build what they need using an API.
> >>>> Actually, ‘QEMU’ only need support the existing CLI, and provide a 
> >>>> suitable internal API.
> >>>> If that API was relatively stable, that would help those (few) who 
> >>>> maintain a different startup mechanism (but its only a ’nice to have’). 
> >>>> (Making that convenient, as Paolo has show, would also be ’nice to 
> >>>> have’).
> >>> 
> >>> To be clear I do strongly believe that the QEMU project needs
> >>> to deliver the higher level simplified interface too. I just
> >>> want that higher level interface to be flexible enough to
> >>> let end users expand on what it offers, without having to
> >>> write C code nor having to switch entirely to the low level
> >>> interface like we do today.
> >>> 
> >>> IOW, QEMU needs to deliver more than just a low level building
> >>> block API.
> >> 
> >> Why?
> >> Clearly it would be nice if “higher level” interfaceS existed in
> >> the world. Clearly QEMU could provide one, two, or many. But, why
> >> do you think QEMU ‘must’ provide them?
> > 
> > To serve our users who are not all wanting to be use a management
> > layer. They want to be using a simple binary to spin up adhoc
> > VMs. This is the reason why we've kept most of the short option
> > CLI args in the existing QEMU binaries, despite having more
> > comprehensive low level replacement args. 
> 
> So - there are
> a) uses today that use the CLI as it exists.
> b) users who might prefer a better interface if that was made available - 
> but, again, that doesn’t require QEMU itself to do anything. If we provide a 
> low-level API, and somebody else (you for instance) provides a ’nice’ 
> ‘friendly’ CLI or config file reader - those users would be happy.
> 
> I still dont see why QEMU itself needs to provide this ‘high level’ thing.

The QEMU project has provided this high level CLI itself historically.
In previous discussions about dropping the simplified options, leaving
only the QAPI based options, that idea has always been rejected by a
non-trivial number of QEMU maintainers who consider it a core part of
our deliverable as a project.

> 
> I think we all agree (correct me if I’m wrong) :
> * We all want a low level interface
> * We all want the current CLI to be supported (at least for now, though it 
> could change in time)
> * We all want the CLI to be based on the low level interface
> 
> I’m just asking why we ALSO want to support “yet another high level 
> interface”….

You're arguing for a significant change in the scope of what QEMU
delivers to its users, punting a bunch of functionality to be
someone else's problem.


> > If we just declare we're not going to provide this simple binary
> > any more, then we're just casting these users adrift. This in
> 
> “Any more” - Are you talking about the existing CLI users?

Yes.

> > effect means they'll just carry on existing the historical QEMU
> > binaries and we'll never be able to eliminate them, so we'll be
> > maintaining two things forever.
> 
> A CLI and the low level interface? - Yes? Can we remove the CLI
> and only support the low-level interface ? But here you seem to
> be arguing against yourself, so I guess I misunderstood.

The current qemu-system-$TARGET emulators have a variety of
CLI args, some low level QAPI driven, and some higher level
simplified args. Mgmt app consumers tend to use the former,
while our humans userbase tends to use the latter.  I'm
saying we need to carry on serving both userbases, unless
we get the QEMU maintainers in general to agree to a pretty
significant change in what the project delivers. Based on
previous discussions, I'm doubtful we'll get agreement to
do that.

So if we're going to successfully introduce replacement
for qemu-system-$TARGET, we need to cater for both userbases,
albeit we can do so with separate binaries, rather than trying
to cram low level and high level into the same.


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] Relax X509 CA cert sanity checking

2021-12-14 Thread Daniel P . Berrangé
On Tue, Dec 14, 2021 at 11:39:30AM +, Henry Kleynhans wrote:
> From: Henry Kleynhans 
> 
> The sanity checking function attempts to validate all the certificates
> in the provided CA file.  These checks are performed on certificates
> which may or may not be part of the signing chain and duplicates checks
> that should be performed by the TLS library.
> 
> In real life this causes a problem if the certificate chain I want to
> use is valid, but there exist another expired certificate in the CA
> file.
> 
> This patch relaxes the sanity checks to only ensure we have at least one
> valid certificate in the CA certificate file and leave the actual
> validation to the TLS library.

IIUC your scenario is that you have something like

   Root CA -> Sub CA1 ---> Server Cert
  \\-> Client Cert
   \---> Sub CA2

And the ca-cert.pem file given to QEMU contains all of 'Root CA',
'Sub CA1' and 'Sub CA2', despite 'Sub CA2' being irrelevant
from POV of QEMU's needs for the chain of trust.

Now 'Sub CA2' is expired so QEMU is rejecting the entire
'ca-cert.pem' at startup.

Your suggested change makes it so that we only validate that
one of these three certs is OK, so if 'Sub CA2' is expired
it dosn't block startup.

The trouble is that this equally ignores problems if 'Sub CA1'
is expired (or otherwise invalid), which is exactly the
scenario that we're aiming to detect.


TLS certificate config mistakes are an incredibly common
problem and the error reporting when this happens at time
of TLS session handshake is terrible. This leads to a 
stream of support requests from users wondering why their
TLS sessions won't establish, which are very hard for us
to debug as maintainers. The validation made a significant
difference to this by giving users clearer error reports
upfront instead of letting QEMU startup with a broken
TLS cert setup.

Thus I'm loathe to relax the validation in the way proposed
here.

Is there a reason you aren't able to just set the
property  'validate=off' on the tls-creds-x509 object
to skip the validation in the case where you know your
CA bundle is valid overall, but contains broken certs ?


> 
> Signed-off-by: Henry Kleynhans 
> ---
>  crypto/tlscredsx509.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 32948a6bdc..fb056f96a2 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -473,6 +473,7 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 
> *creds,
>  gnutls_x509_crt_t cert = NULL;
>  gnutls_x509_crt_t cacerts[MAX_CERTS];
>  size_t ncacerts = 0;
> +size_t nvalidca = 0;
>  size_t i;
>  int ret = -1;
>  
> @@ -505,11 +506,15 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 
> *creds,
>  for (i = 0; i < ncacerts; i++) {
>  if (qcrypto_tls_creds_check_cert(creds,
>   cacerts[i], cacertFile,
> - isServer, true, errp) < 0) {
> -goto cleanup;
> + isServer, true, errp) == 0) {
> +++nvalidca;
>  }
>  }
>  
> +if (nvalidca == 0) {
> +goto cleanup;
> +}
> +
>  if (cert && ncacerts &&
>  qcrypto_tls_creds_check_cert_pair(cert, certFile, cacerts,
>ncacerts, cacertFile,
> -- 
> 2.34.1
> 
> 

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




Re: Redesign of QEMU startup & initial configuration

2021-12-14 Thread Daniel P . Berrangé
On Tue, Dec 14, 2021 at 02:11:11PM +0100, Mark Burton wrote:
> 
> 
> > On 14 Dec 2021, at 14:05, Daniel P. Berrangé  wrote:
> > 
> > On Mon, Dec 13, 2021 at 09:22:14PM +0100, Mark Burton wrote:
> >> 
> >> 
> >>> On 13 Dec 2021, at 18:59, Daniel P. Berrangé  wrote:
> >>> 
> >>> …. we no longer have to solve everything
> >>> Ourselves. 
> >> 
> >> I support this sentiment.
> >> 
> >> Lets re-factor the code so people can build what they need using an API.
> >> Actually, ‘QEMU’ only need support the existing CLI, and provide a 
> >> suitable internal API.
> >> If that API was relatively stable, that would help those (few) who 
> >> maintain a different startup mechanism (but its only a ’nice to have’). 
> >> (Making that convenient, as Paolo has show, would also be ’nice to have’).
> > 
> > To be clear I do strongly believe that the QEMU project needs
> > to deliver the higher level simplified interface too. I just
> > want that higher level interface to be flexible enough to
> > let end users expand on what it offers, without having to
> > write C code nor having to switch entirely to the low level
> > interface like we do today.
> > 
> > IOW, QEMU needs to deliver more than just a low level building
> > block API.
> 
> Why?
> Clearly it would be nice if “higher level” interfaceS existed in
> the world. Clearly QEMU could provide one, two, or many. But, why
> do you think QEMU ‘must’ provide them?

To serve our users who are not all wanting to be use a management
layer. They want to be using a simple binary to spin up adhoc
VMs. This is the reason why we've kept most of the short option
CLI args in the existing QEMU binaries, despite having more
comprehensive low level replacement args. 

If we just declare we're not going to provide this simple binary
any more, then we're just casting these users adrift. This in
effect means they'll just carry on existing the historical QEMU
binaries and we'll never be able to eliminate them, so we'll be
maintaining two things forever.

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 :|




  1   2   3   4   5   6   7   8   9   10   >