Re: [Qemu-devel] [RFC v5 00/29] vSMMUv3/pSMMUv3 2 stage VFIO integration

[Auger Eric]

Hi,

On 7/12/19 12:26 AM, no-re...@patchew.org wrote:
> Patchew URL: 
> https://patchew.org/QEMU/20190711173933.31203-1-eric.au...@redhat.com/
> 
> 
> 
> Hi,
> 
> This series failed build test on s390x host. Please find the details below.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> # Testing script will be invoked under the git checkout with
> # HEAD pointing to a commit that has the patches applied on top of "base"
> # branch
> set -e
> 
> echo
> echo "=== ENV ==="
> env
> 
> echo
> echo "=== PACKAGES ==="
> rpm -qa
> 
> echo
> echo "=== UNAME ==="
> uname -a
> 
> CC=$HOME/bin/cc
> INSTALL=$PWD/install
> BUILD=$PWD/build
> mkdir -p $BUILD $INSTALL
> SRC=$PWD
> cd $BUILD
> $SRC/configure --cc=$CC --prefix=$INSTALL
> make -j4
> # XXX: we need reliable clean up
> # make check -j4 V=1
> make install
> === TEST SCRIPT END ===
> 
>   CC  i386-softmmu/hw/virtio/virtio-crypto.o
>   CC  i386-softmmu/hw/virtio/virtio-crypto-pci.o
>   CC  i386-softmmu/hw/virtio/virtio-pmem.o
> /var/tmp/patchew-tester-tmp-u52ljjk0/src/hw/virtio/virtio-pmem.c:21:10: fatal 
> error: standard-headers/linux/virtio_pmem.h: No such file or directory
>21 | #include "standard-headers/linux/virtio_pmem.h"
>   |  ^~
> compilation terminated.
In the kernel branch I used along with ./scripts/update-linux-headers.sh
the header was not yet there. As a consequence the
standard-headers/linux/virtio_pmem.h was removed. I will fix that soon.

Does not block testing on ARM where the VIRTIO_PMEM device is not
configured.

Thanks

Eric
> 
> 
> The full log is available at
> http://patchew.org/logs/20190711173933.31203-1-eric.au...@redhat.com/testing.s390x/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-de...@redhat.com
> 

[Qemu-devel] [PATCH 1/3] virtio pmem: fix wrong mem region condition

[Pankaj Gupta]

Coverity reported memory region returns zero
for non-null value. This is because of wrong
arguments to '?:' , fixing this.

Signed-off-by: Pankaj Gupta 
---
 hw/virtio/virtio-pmem-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
index 8b2d0dbccc..0da6627469 100644
--- a/hw/virtio/virtio-pmem-pci.c
+++ b/hw/virtio/virtio-pmem-pci.c
@@ -57,7 +57,7 @@ static uint64_t virtio_pmem_pci_get_plugged_size(const 
MemoryDeviceState *md,
 MemoryRegion *mr = vpc->get_memory_region(pmem, errp);
 
 /* the plugged size corresponds to the region size */
-return mr ? 0 : memory_region_size(mr);
+return mr ? memory_region_size(mr) : 0;
 }
 
 static void virtio_pmem_pci_fill_device_info(const MemoryDeviceState *md,
-- 
2.14.5

[Qemu-devel] [PATCH 0/3] virtio pmem: coverity fixes

[Pankaj Gupta]

This patch series two fixes for coverity and a 
transactional info removal patch.

Pankaj Gupta (3):
  virtio pmem: fix wrong mem region condition
  virtio pmem: remove memdev null check
  virtio pmem: remove transational device info

 hw/virtio/virtio-pmem-pci.c | 4 +---
 hw/virtio/virtio-pmem.c | 4 ++--
 2 files changed, 3 insertions(+), 5 deletions(-)

-- 
2.14.5

[Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info

[Pankaj Gupta]

Remove transactional & non transactional device info
for virtio pmem. 

Signed-off-by: Pankaj Gupta 
---
 hw/virtio/virtio-pmem-pci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
index 0da6627469..fe2af00fa1 100644
--- a/hw/virtio/virtio-pmem-pci.c
+++ b/hw/virtio/virtio-pmem-pci.c
@@ -113,8 +113,6 @@ static void virtio_pmem_pci_instance_init(Object *obj)
 static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = {
 .base_name = TYPE_VIRTIO_PMEM_PCI,
 .generic_name  = "virtio-pmem-pci",
-.transitional_name = "virtio-pmem-pci-transitional",
-.non_transitional_name = "virtio-pmem-pci-non-transitional",
 .instance_size = sizeof(VirtIOPMEMPCI),
 .instance_init = virtio_pmem_pci_instance_init,
 .class_init= virtio_pmem_pci_class_init,
-- 
2.14.5

[Qemu-devel] [PATCH 2/3] virtio pmem: remove memdev null check

[Pankaj Gupta]

Coverity reports that when we're assigning vi->size we handle the 
"pmem->memdev is NULL" case; but we then pass it into 
object_get_canonical_path(), which unconditionally dereferences it
and will crash if it is NULL. If this pointer can be NULL then we
need to do something else here.

We are removing 'pmem->memdev' null check here as memdev will never
be null in this function.

Signed-off-by: Pankaj Gupta 
---
 hw/virtio/virtio-pmem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
index adbfb603ab..17c196d107 100644
--- a/hw/virtio/virtio-pmem.c
+++ b/hw/virtio/virtio-pmem.c
@@ -134,8 +134,8 @@ static void virtio_pmem_fill_device_info(const VirtIOPMEM 
*pmem,
  VirtioPMEMDeviceInfo *vi)
 {
 vi->memaddr = pmem->start;
-vi->size = pmem->memdev ? memory_region_size(&pmem->memdev->mr) : 0;
-vi->memdev = object_get_canonical_path(OBJECT(pmem->memdev));
+vi->size= memory_region_size(&pmem->memdev->mr);
+vi->memdev  = object_get_canonical_path(OBJECT(pmem->memdev));
 }
 
 static MemoryRegion *virtio_pmem_get_memory_region(VirtIOPMEM *pmem,
-- 
2.14.5

Re: [Qemu-devel] [PATCH v2] pcie: consistent names for function args

[Marcel Apfelbaum]

On 7/11/19 11:28 PM, Michael S. Tsirkin wrote:

The function declarations for pci_cap_slot_get and
pci_cap_slot_write_config call the argument "slot_ctl", but the function
definitions and all the call sites drop the 'o' and call it "slt_ctl".
Let's be consistent.

Reported-by: Peter Maydell 
Signed-off-by: Michael S. Tsirkin 
---

Fix pcie_cap_slot_write_config too.

  include/hw/pci/pcie.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 34f277735c..8cf3361fc4 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -107,9 +107,9 @@ void pcie_cap_lnkctl_reset(PCIDevice *dev);
  
  void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot);

  void pcie_cap_slot_reset(PCIDevice *dev);
-void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slot_ctl, uint16_t *slt_sta);
+void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta);
  void pcie_cap_slot_write_config(PCIDevice *dev,
-uint16_t old_slot_ctl, uint16_t old_slt_sta,
+uint16_t old_slt_ctl, uint16_t old_slt_sta,
  uint32_t addr, uint32_t val, int len);
  int pcie_cap_slot_post_load(void *opaque, int version_id);
  void pcie_cap_slot_push_attention_button(PCIDevice *dev);


Reviewed-by: Marcel Apfelbaum 

Thanks,
Marcel

[Qemu-devel] [PATCH] hw/arm/virt: Fix non-secure flash mode

[David Engraf]

Using the whole 128 MiB flash in non-secure mode is not working because
virt_flash_fdt() expects the same address for secure_sysmem and sysmem.
This is not correctly handled by caller because it forwards NULL for
secure_sysmem in non-secure flash mode.

Fixed by using sysmem when secure_sysmem is NULL.

Signed-off-by: David Engraf 
---
 hw/arm/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 0b5138cb22..d9496c9363 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1674,7 +1674,7 @@ static void machvirt_init(MachineState *machine)
 &machine->device_memory->mr);
 }
 
-virt_flash_fdt(vms, sysmem, secure_sysmem);
+virt_flash_fdt(vms, sysmem, secure_sysmem ?: sysmem);
 
 create_gic(vms, pic);
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH] xio3130_downstream: typo fix

[Marcel Apfelbaum]

On 7/11/19 10:25 PM, Michael S. Tsirkin wrote:

slt ctl/status are passed in incorrect order.
Fix this up.

Signed-off-by: Michael S. Tsirkin 
Reported-by: Peter Maydell 
---
  hw/pci-bridge/xio3130_downstream.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci-bridge/xio3130_downstream.c 
b/hw/pci-bridge/xio3130_downstream.c
index 899b0fd6c9..182e164f74 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -43,7 +43,7 @@ static void xio3130_downstream_write_config(PCIDevice *d, 
uint32_t address,
  {
  uint16_t slt_ctl, slt_sta;
  
-pcie_cap_slot_get(d, &slt_sta, &slt_ctl);

+pcie_cap_slot_get(d, &slt_ctl, &slt_sta);
  pci_bridge_write_config(d, address, val, len);
  pcie_cap_flr_write_config(d, address, val, len);
  pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len);


Reviewed-by: Marcel Apfelbaum 

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2] migration: Do not re-read the clock on pre_save in case of paused guest

[David Gibson]

On Thu, Jul 11, 2019 at 04:47:02PM -0300, Maxiwell S. Garcia wrote:
> Re-read the timebase before migrate was ported from x86 commit:
>6053a86fe7bd: kvmclock: reduce kvmclock difference on migration
> 
> The clock move makes the guest knows about the paused time between
> the stop and migrate commands. This is an issue in an already-paused
> VM because some side effects, like process stalls, could happen
> after migration.
> 
> So, this patch checks the runstate of guest in the pre_save handler and
> do not re-reads the timebase in case of paused state (cold migration).
> 
> Signed-off-by: Maxiwell S. Garcia 

I've applied this to ppc-for-4.2.  I think it probably is a correct
fix, but this could have subtle impacts on things that are mostly
working at the moment, so I don't want to risk it during hard freeze.

> ---
>  hw/ppc/ppc.c | 13 +
>  target/ppc/cpu-qom.h |  1 +
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index a9e508c496..8572e45274 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -1008,6 +1008,8 @@ static void timebase_save(PPCTimebase *tb)
>   * there is no need to update it from KVM here
>   */
>  tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
> +
> +tb->runstate_paused = runstate_check(RUN_STATE_PAUSED);
>  }
>  
>  static void timebase_load(PPCTimebase *tb)
> @@ -1051,9 +1053,9 @@ void cpu_ppc_clock_vm_state_change(void *opaque, int 
> running,
>  }
>  
>  /*
> - * When migrating, read the clock just before migration,
> - * so that the guest clock counts during the events
> - * between:
> + * When migrating a running guest, read the clock just
> + * before migration, so that the guest clock counts
> + * during the events between:
>   *
>   *  * vm_stop()
>   *  *
> @@ -1068,7 +1070,10 @@ static int timebase_pre_save(void *opaque)
>  {
>  PPCTimebase *tb = opaque;
>  
> -timebase_save(tb);
> +/* guest_timebase won't be overridden in case of paused guest */
> +if (!tb->runstate_paused) {
> +timebase_save(tb);
> +}
>  
>  return 0;
>  }
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index be9b4c30c3..5fbcdee9c9 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -201,6 +201,7 @@ typedef struct PowerPCCPUClass {
>  typedef struct PPCTimebase {
>  uint64_t guest_timebase;
>  int64_t time_of_the_day_ns;
> +bool runstate_paused;
>  } PPCTimebase;
>  
>  extern const struct VMStateDescription vmstate_ppc_timebase;

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/1] Don't obey the kernel block device max transfer len / max segments for raw block devices

[Stefan Hajnoczi]

On Thu, Jul 04, 2019 at 03:43:41PM +0300, Maxim Levitsky wrote:
> Linux block devices, even in O_DIRECT mode don't have any user visible
> limit on transfer size / number of segments, which underlying kernel block 
> device can have.
> The kernel block layer takes care of enforcing these limits by splitting the 
> bios.
> 
> By limiting the transfer sizes, we force qemu to do the splitting itself which
> introduces various overheads.
> It is especially visible in nbd server, where the low max transfer size of the
> underlying device forces us to advertise this over NBD, thus increasing the
> traffic overhead in case of image conversion which benefits from large blocks.
> 
> More information can be found here:
> https://bugzilla.redhat.com/show_bug.cgi?id=1647104
> 
> Tested this with qemu-img convert over nbd and natively and to my surprise,
> even native IO performance improved a bit.
> 
> (The device on which it was tested is Intel Optane DC P4800X,
> which has 128k max transfer size reported by the kernel)
> 
> The benchmark:
> 
> Images were created using:
> 
> Sparse image:  qemu-img create -f qcow2 /dev/nvme0n1p3 1G / 10G / 100G
> Allocated image: qemu-img create -f qcow2 /dev/nvme0n1p3 -o 
> preallocation=metadata  1G / 10G / 100G
> 
> The test was:
> 
>  echo "convert native:"
>  rm -rf /dev/shm/disk.img
>  time qemu-img convert -p -f qcow2 -O raw -T none $FILE /dev/shm/disk.img > 
> /dev/zero
> 
>  echo "convert via nbd:"
>  qemu-nbd -k /tmp/nbd.sock -v  -f qcow2 $FILE -x export --cache=none 
> --aio=native --fork
>  rm -rf /dev/shm/disk.img
>  time qemu-img convert -p -f raw -O raw 
> nbd:unix:/tmp/nbd.sock:exportname=export /dev/shm/disk.img > /dev/zero
> 
> The results:
> 
> =
> 1G sparse image:
>  native:
>   before: 0.027s
>   after: 0.027s
>  nbd:
>   before: 0.287s
>   after: 0.035s
> 
> =
> 100G sparse image:
>  native:
>   before: 0.028s
>   after: 0.028s
>  nbd:
>   before: 23.796s
>   after: 0.109s
> 
> =
> 1G preallocated image:
>  native:
>before: 0.454s
>after: 0.427s
>  nbd:
>before: 0.649s
>after: 0.546s
> 
> The block limits of max transfer size/max segment size are retained
> for the SCSI passthrough because in this case the kernel passes the userspace 
> request
> directly to the kernel scsi driver, bypassing the block layer, and thus there 
> is no code to split
> such requests.
> 
> Fam, since you was the original author of the code that added
> these limits, could you share your opinion on that?
> What was the reason besides SCSI passthrough?
> 
> V2:
> 
> *  Manually tested to not break the scsi passthrough with a nested VM
> *  As Eric suggested, refactored the area around the fstat.
> *  Spelling/grammar fixes
> 
> Best regards,
>   Maxim Levitsky
> 
> Maxim Levitsky (1):
>   raw-posix.c - use max transfer length / max segement count only for
> SCSI passthrough
> 
>  block/file-posix.c | 54 --
>  1 file changed, 28 insertions(+), 26 deletions(-)
> 
> -- 
> 2.17.2
> 
> 

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v3] doc: document that the monitor console is a privileged control interface

[Stefan Hajnoczi]

On Fri, Jul 05, 2019 at 04:41:54PM +0100, Daniel P. Berrangé wrote:
> A supposed exploit of QEMU was recently announced as CVE-2019-12928
> claiming that the monitor console was insecure because the "migrate"
> command enabled arbitrary command execution for a remote attacker.
> 
> To be a security risk the user launching QEMU must have configured
> the monitor in a way that allows for other users to access it. The
> exploit report quoted use of the "tcp" character device backend for
> QMP.
> 
> This would indeed allow any network user to connect to QEMU and
> execute arbitrary commands, however, this is not a flaw in QEMU.
> It is the normal expected behaviour of the monitor console and the
> commands it supports. Given a monitor connection, there are many
> ways to access host file system content besides the migrate command.
> 
> The reality is that the monitor console (whether QMP or HMP) is
> considered a privileged interface to QEMU and as such must only
> be made available to trusted users. IOW, making it available with
> no authentication over TCP is simply a, very serious, user
> configuration error not a security flaw in QEMU itself.
> 
> The one thing this bogus security report highlights though is that
> we have not clearly documented the security implications around the
> use of the monitor. Add a few paragraphs of text to the security
> docs explaining why the monitor is a privileged interface and making
> a recommendation to only use the UNIX socket character device backend.
> 
> Reviewed-by: Alex Bennée 
> Reviewed-by: Markus Armbruster 
> Reviewed-by: Prasad J Pandit 
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Daniel P. Berrangé 
> ---
> 
> Changed in v3:
> 
>  - More copy editing from review feedback (Markus, PJP, Alex)
> 
> Changed in v2:
> 
>  - Addressed misc typos (Eric / Philippe)
> 
>  docs/security.texi | 36 
>  1 file changed, 36 insertions(+)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-block] [PATCH] Fix Guest VM crash due to iSCSI Sense Key error

[Stefan Hajnoczi]

On Sun, Jul 07, 2019 at 07:55:03PM -0700, shaju.abra...@nutanix.com wrote:

Reviewed-by: Stefan Hajnoczi 

CCing John Snow, IDE maintainer.

You can use scripts/get_maintainer.pl -f hw/ide/core.c to find out who
to send patches to.

Stefan

> From: Shaju Abraham 
> 
> During the  IDE DMA transfer for a ISCSI target,when libiscsi encounters
> a SENSE KEY error, it sets the task->sense to  the value "COMMAND ABORTED".
> The function iscsi_translate_sense() later translaters this error to 
> -ECANCELED
> and this value is passed to the callback function. In the case of  IDE DMA 
> read
> or write, the callback function returns immediately if the value of the ret
> argument is -ECANCELED.
> Later when ide_cancel_dma_sync() function is invoked  the assertion
> "s->bus->dma->aiocb == ((void *)0)" fails and the qemu process gets 
> terminated.
> Fix the issue by making the value of s->bus->dma->aiocb = NULL when
> -ECANCELED is passed to the callback.
> 
> Signed-off-by: Shaju Abraham 
> ---
>  hw/ide/core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 6afadf8..78ea357 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -841,6 +841,7 @@ static void ide_dma_cb(void *opaque, int ret)
>  bool stay_active = false;
>  
>  if (ret == -ECANCELED) {
> +s->bus->dma->aiocb = NULL;
>  return;
>  }
>  
> -- 
> 1.9.4
> 
> 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] configure: fix sdl detection using sdl2-config

[Thomas Huth]

On 11/07/2019 00.55, Carlo Marcelo Arenas Belón wrote:
> If SDL2 is requested but pkg-config doesn't have a module for it
> configure should fallback to use sdl*-config, but wasn't able to
> because and old variable (from SDL) was being used by mistake.
> 
> Correct the variable name and complete other related changes so
> there are no more references to the old SDL.
> 
> Fixes: 0015ca5cbabe ("ui: remove support for SDL1.2 in favour of SDL2")
> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  configure | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/configure b/configure
> index 4983c8b533..0f88ba98a6 100755
> --- a/configure
> +++ b/configure
> @@ -3016,15 +3016,15 @@ fi
>  ##
>  # SDL probe
>  
> -# Look for sdl configuration program (pkg-config or sdl-config).  Try
> -# sdl-config even without cross prefix, and favour pkg-config over 
> sdl-config.
> +# Look for sdl configuration program (pkg-config or sdl2-config).  Try
> +# sdl2-config even without cross prefix, and favour pkg-config over 
> sdl2-config.
>  
>  sdl_probe ()
>  {
>if $pkg_config sdl2 --exists; then
>  sdlconfig="$pkg_config sdl2"
>  sdlversion=$($sdlconfig --modversion 2>/dev/null)
> -  elif has ${sdl_config}; then
> +  elif has "$sdl2_config"; then
>  sdlconfig="$sdl2_config"
>  sdlversion=$($sdlconfig --version)
>else
> @@ -3035,7 +3035,7 @@ sdl_probe ()
>  # no need to do the rest
>  return
>fi
> -  if test -n "$cross_prefix" && test "$(basename "$sdlconfig")" = 
> sdl-config; then
> +  if test -n "$cross_prefix" && test "$(basename "$sdlconfig")" = 
> sdl2-config; then
>  echo warning: using "\"$sdlconfig\"" to detect cross-compiled sdl >&2
>fi
>  
> @@ -8019,7 +8019,6 @@ preserve_env PKG_CONFIG
>  preserve_env PKG_CONFIG_LIBDIR
>  preserve_env PKG_CONFIG_PATH
>  preserve_env PYTHON
> -preserve_env SDL_CONFIG
>  preserve_env SDL2_CONFIG
>  preserve_env SMBD
>  preserve_env STRIP
> 

Reviewed-by: Thomas Huth 

[Qemu-devel] [PATCH] libvhost-user: Add missing GCC_FMT_ATTR and fix three format errors

[Stefan Weil]

Signed-off-by: Stefan Weil 
---
 contrib/libvhost-user/libvhost-user.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/libvhost-user/libvhost-user.c 
b/contrib/libvhost-user/libvhost-user.c
index 4b36e35a82..59b3202979 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -142,7 +142,7 @@ vu_request_to_string(unsigned int req)
 }
 }
 
-static void
+static void GCC_FMT_ATTR(2, 3)
 vu_panic(VuDev *dev, const char *msg, ...)
 {
 char *buf = NULL;
@@ -661,7 +661,7 @@ vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg 
*vmsg)
 
 if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, ®_struct)) {
 vu_panic(dev, "%s: Failed to userfault region %d "
-  "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
+  "@%" PRIx64 " + size:%zx offset: %zx: (ufd=%d)%s\n",
  __func__, i,
  dev_region->mmap_addr,
  dev_region->size, dev_region->mmap_offset,
@@ -1753,7 +1753,7 @@ virtqueue_get_head(VuDev *dev, VuVirtq *vq,
 
 /* If their number is silly, that's a fatal mistake. */
 if (*head >= vq->vring.num) {
-vu_panic(dev, "Guest says index %u is available", head);
+vu_panic(dev, "Guest says index %u is available", idx);
 return false;
 }
 
@@ -1812,7 +1812,7 @@ virtqueue_read_next_desc(VuDev *dev, struct vring_desc 
*desc,
 smp_wmb();
 
 if (*next >= max) {
-vu_panic(dev, "Desc next is %u", next);
+vu_panic(dev, "Desc next is %u", *next);
 return VIRTQUEUE_READ_DESC_ERROR;
 }
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH for-4.1] Makefile: Fix "make install" when "make all" needs work

[Daniel P . Berrangé]

On Fri, Jul 12, 2019 at 07:59:35AM +0200, Markus Armbruster wrote:
> Until recently, target install used to recurse into target directories
> in its recipe: it ran make install in a for-loop.  Since target
> install depends on target all, this trivially ensured we run the
> sub-make install only after completing target all.
> 
> Commit 1338a4b "Makefile: Reuse all's recursion machinery for clean
> and install" moved the target recursion to dependencies.  That's good
> (the commit message explains why), but I forgot to add dependencies to
> ensure make runs the sub-make install only after completing target
> all.  Do that now.
> 
> Fixes: 1338a4b72659ce08eacb9de0205fe16202a22d9c
> Reported-by: Mark Cave-Ayland 
> Reported-by: Guenter Roeck 
> Tested-by: Guenter Roeck 
> Signed-off-by: Markus Armbruster 
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)

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: [Qemu-devel] [PATCH-for-4.1] tests/docker: Install Sphinx in the Ubuntu images

[Stefano Garzarella]

On Thu, Jul 11, 2019 at 02:06:09PM +0200, Philippe Mathieu-Daudé wrote:
> Since commit 5f71eac06e the Sphinx tool is required
> to build the rST documentation.
> 
> This fixes:
> 
>  $ ./configure --enable-docs
> 
>  ERROR: User requested feature docs
> configure was not able to find it.
> Install texinfo, Perl/perl-podlators and python-sphinx
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/docker/dockerfiles/ubuntu.docker | 1 +
>  tests/docker/dockerfiles/ubuntu1804.docker | 1 +
>  2 files changed, 2 insertions(+)

Reviewed-by: Stefano Garzarella 

Re: [Qemu-devel] [PATCH v2 0/1] Don't obey the kernel block device max transfer len / max segments for raw block devices

[Pankaj Gupta]

> Linux block devices, even in O_DIRECT mode don't have any user visible
> limit on transfer size / number of segments, which underlying kernel block
> device can have.
> The kernel block layer takes care of enforcing these limits by splitting the
> bios.
> 
> By limiting the transfer sizes, we force qemu to do the splitting itself
> which
> introduces various overheads.
> It is especially visible in nbd server, where the low max transfer size of
> the
> underlying device forces us to advertise this over NBD, thus increasing the
> traffic overhead in case of image conversion which benefits from large
> blocks.
> 
> More information can be found here:
> https://bugzilla.redhat.com/show_bug.cgi?id=1647104
> 
> Tested this with qemu-img convert over nbd and natively and to my 
> surprise,Reviewed-by: Stefano Garzarella 
> even native IO performance improved a bit.
> 
> (The device on which it was tested is Intel Optane DC P4800X,
> which has 128k max transfer size reported by the kernel)
> 
> The benchmark:
> 
> Images were created using:
> 
> Sparse image:  qemu-img create -f qcow2 /dev/nvme0n1p3 1G / 10G / 100G
> Allocated image: qemu-img create -f qcow2 /dev/nvme0n1p3 -o
> preallocation=metadata  1G / 10G / 100G
> 
> The test was:
> 
>  echo "convert native:"
>  rm -rf /dev/shm/disk.img
>  time qemu-img convert -p -f qcow2 -O raw -T none $FILE /dev/shm/disk.img >
>  /dev/zero
> 
>  echo "convert via nbd:"
>  qemu-nbd -k /tmp/nbd.sock -v  -f qcow2 $FILE -x export --cache=none
>  --aio=native --fork
>  rm -rf /dev/shm/disk.img
>  time qemu-img convert -p -f raw -O raw
>  nbd:unix:/tmp/nbd.sock:exportname=export /dev/shm/disk.img > /dev/zero
> 
> The results:
> 
> =
> 1G sparse image:
>  native:
>   before: 0.027s
>   after: 0.027s
>  nbd:
>   before: 0.287s
>   after: 0.035s
> 
> =
> 100G sparse image:
>  native:
>   before: 0.028s
>   after: 0.028s
>  nbd:
>   before: 23.796s
>   after: 0.109s
> 
> =
> 1G preallocated image:
>  native:
>before: 0.454s
>after: 0.427s
>  nbd:
>before: 0.649s
>after: 0.546s
> 
> The block limits of max transfer size/max segment size are retained
> for the SCSI passthrough because in this case the kernel passes the userspace
> request
> directly to the kernel scsi driver, bypassing the block layer, and thus there
> is no code to split
> such requests.
> 
> Fam, since you was the original author of the code that added
> these limits, could you share your opinion on that?
> What was the reason besides SCSI passthrough?
> 
> V2:
> 
> *  Manually tested to not break the scsi passthrough with a nested VM
> *  As Eric suggested, refactored the area around the fstat.
> *  Spelling/grammar fixes
> 
> Best regards,
>   Maxim Levitsky
> 
> Maxim Levitsky (1):
>   raw-posix.c - use max transfer length / max segement count only for
> SCSI passthrough
> 
>  block/file-posix.c | 54 --
>  1 file changed, 28 insertions(+), 26 deletions(-)
> 
> --

I am not familiar with SCSI passthrough special case. But overall looks good to 
me.

Feel free to add:
Reviewed-by: Pankaj Gupta 

> 2.17.2
> 
> 
> 

Re: [Qemu-devel] [PATCH] libvhost-user: Add missing GCC_FMT_ATTR and fix three format errors

[Marc-André Lureau]

On Fri, Jul 12, 2019 at 12:25 PM Stefan Weil  wrote:
>
> Signed-off-by: Stefan Weil 
> ---
>  contrib/libvhost-user/libvhost-user.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/libvhost-user/libvhost-user.c 
> b/contrib/libvhost-user/libvhost-user.c
> index 4b36e35a82..59b3202979 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -142,7 +142,7 @@ vu_request_to_string(unsigned int req)
>  }
>  }
>
> -static void
> +static void GCC_FMT_ATTR(2, 3)
>  vu_panic(VuDev *dev, const char *msg, ...)
>  {
>  char *buf = NULL;
> @@ -661,7 +661,7 @@ vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg 
> *vmsg)
>
>  if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, ®_struct)) {
>  vu_panic(dev, "%s: Failed to userfault region %d "
> -  "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
> +  "@%" PRIx64 " + size:%zx offset: %zx: 
> (ufd=%d)%s\n",
>   __func__, i,
>   dev_region->mmap_addr,
>   dev_region->size, dev_region->mmap_offset,
> @@ -1753,7 +1753,7 @@ virtqueue_get_head(VuDev *dev, VuVirtq *vq,
>
>  /* If their number is silly, that's a fatal mistake. */
>  if (*head >= vq->vring.num) {
> -vu_panic(dev, "Guest says index %u is available", head);
> +vu_panic(dev, "Guest says index %u is available", idx);

The original version (hw/virtio/virtio.c) print *head. The message
isn't great, but I would rather stay consistent for now.

other than that,
Reviewed-by: Marc-André Lureau 


>  return false;
>  }
>
> @@ -1812,7 +1812,7 @@ virtqueue_read_next_desc(VuDev *dev, struct vring_desc 
> *desc,
>  smp_wmb();
>
>  if (*next >= max) {
> -vu_panic(dev, "Desc next is %u", next);
> +vu_panic(dev, "Desc next is %u", *next);
>  return VIRTQUEUE_READ_DESC_ERROR;
>  }
>
> --
> 2.20.1
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/3] virtio pmem: fix wrong mem region condition

[Stefano Garzarella]

On Fri, Jul 12, 2019 at 01:05:52PM +0530, Pankaj Gupta wrote:
> Coverity reported memory region returns zero
> for non-null value. This is because of wrong
> arguments to '?:' , fixing this.
> 
> Signed-off-by: Pankaj Gupta 
> ---
>  hw/virtio/virtio-pmem-pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefano Garzarella 

> 
> diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
> index 8b2d0dbccc..0da6627469 100644
> --- a/hw/virtio/virtio-pmem-pci.c
> +++ b/hw/virtio/virtio-pmem-pci.c
> @@ -57,7 +57,7 @@ static uint64_t virtio_pmem_pci_get_plugged_size(const 
> MemoryDeviceState *md,
>  MemoryRegion *mr = vpc->get_memory_region(pmem, errp);
>  
>  /* the plugged size corresponds to the region size */
> -return mr ? 0 : memory_region_size(mr);
> +return mr ? memory_region_size(mr) : 0;
>  }
>  
>  static void virtio_pmem_pci_fill_device_info(const MemoryDeviceState *md,
> -- 
> 2.14.5

Re: [Qemu-devel] [PATCH for-4.1] Makefile: Fix "make install" when "make all" needs work

[Stefano Garzarella]

On Fri, Jul 12, 2019 at 07:59:35AM +0200, Markus Armbruster wrote:
> Until recently, target install used to recurse into target directories
> in its recipe: it ran make install in a for-loop.  Since target
> install depends on target all, this trivially ensured we run the
> sub-make install only after completing target all.
> 
> Commit 1338a4b "Makefile: Reuse all's recursion machinery for clean
> and install" moved the target recursion to dependencies.  That's good
> (the commit message explains why), but I forgot to add dependencies to
> ensure make runs the sub-make install only after completing target
> all.  Do that now.
> 
> Fixes: 1338a4b72659ce08eacb9de0205fe16202a22d9c
> Reported-by: Mark Cave-Ayland 
> Reported-by: Guenter Roeck 
> Tested-by: Guenter Roeck 
> Signed-off-by: Markus Armbruster 
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Makefile b/Makefile
> index 1fcbaed62c..09b77e8a7b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -522,6 +522,7 @@ $(ROM_DIRS_RULES):
>  recurse-all: $(addsuffix /all, $(TARGET_DIRS) $(ROM_DIRS))
>  recurse-clean: $(addsuffix /clean, $(TARGET_DIRS) $(ROM_DIRS))
>  recurse-install: $(addsuffix /install, $(TARGET_DIRS))
> +$(addsuffix /install, $(TARGET_DIRS)): all
>  
>  $(BUILD_DIR)/version.o: $(SRC_PATH)/version.rc config-host.h
>   $(call quiet-command,$(WINDRES) -I$(BUILD_DIR) -o $@ 
> $<,"RC","version.o")

Reviewed-by: Stefano Garzarella 

Re: [Qemu-devel] [PATCH v2 0/1] Don't obey the kernel block device max transfer len / max segments for raw block devices

[Kevin Wolf]

Am 04.07.2019 um 14:43 hat Maxim Levitsky geschrieben:
> Linux block devices, even in O_DIRECT mode don't have any user visible
> limit on transfer size / number of segments, which underlying kernel block 
> device can have.
> The kernel block layer takes care of enforcing these limits by splitting the 
> bios.
> 
> By limiting the transfer sizes, we force qemu to do the splitting itself which
> introduces various overheads.
> It is especially visible in nbd server, where the low max transfer size of the
> underlying device forces us to advertise this over NBD, thus increasing the
> traffic overhead in case of image conversion which benefits from large blocks.
> 
> More information can be found here:
> https://bugzilla.redhat.com/show_bug.cgi?id=1647104
> 
> Tested this with qemu-img convert over nbd and natively and to my surprise,
> even native IO performance improved a bit.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close()

[Kevin Wolf]

Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
> When nbd_close() is called from a coroutine, the connection_co never
> gets to run, and thus nbd_teardown_connection() hangs.
> 
> This is because aio_co_enter() only puts the connection_co into the main
> coroutine's wake-up queue, so this main coroutine needs to yield and
> reschedule itself to let the connection_co run.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/nbd.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 81edabbf35..b83b6cd43e 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -135,7 +135,17 @@ static void nbd_teardown_connection(BlockDriverState *bs)
>  qio_channel_shutdown(s->ioc,
>   QIO_CHANNEL_SHUTDOWN_BOTH,
>   NULL);
> -BDRV_POLL_WHILE(bs, s->connection_co);
> +
> +if (qemu_in_coroutine()) {
> +/* Let our caller poll and just yield until connection_co is done */
> +while (s->connection_co) {
> +aio_co_schedule(qemu_get_current_aio_context(),
> +qemu_coroutine_self());
> +qemu_coroutine_yield();
> +}

Isn't this busy waiting? Why not let s->connection_co wake us up when
it's about to terminate instead of immediately rescheduling ourselves?

> +} else {
> +BDRV_POLL_WHILE(bs, s->connection_co);
> +}
>  
>  nbd_client_detach_aio_context(bs);
>  object_unref(OBJECT(s->sioc));

Kevin

Re: [Qemu-devel] [PATCH v2 03/13] migration/ram: add support to send encrypted pages

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.si...@amd.com) wrote:
> 
> 
> On 7/11/19 12:34 PM, Dr. David Alan Gilbert wrote:
> > * Singh, Brijesh (brijesh.si...@amd.com) wrote:
> >> When memory encryption is enabled, the guest memory will be encrypted with
> >> the guest specific key. The patch introduces RAM_SAVE_FLAG_ENCRYPTED_PAGE
> >> flag to distinguish the encrypted data from plaintext. Encrypted pages
> >> may need special handling. The kvm_memcrypt_save_outgoing_page() is used
> >> by the sender to write the encrypted pages onto the socket, similarly the
> >> kvm_memcrypt_load_incoming_page() is used by the target to read the
> >> encrypted pages from the socket and load into the guest memory.
> >>
> >> Signed-off-by: Brijesh Singh 
> >> ---
> >>   migration/ram.c | 54 -
> >>   1 file changed, 53 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 908517fc2b..3c8977d508 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -57,6 +57,7 @@
> >>   #include "qemu/uuid.h"
> >>   #include "savevm.h"
> >>   #include "qemu/iov.h"
> >> +#include "sysemu/kvm.h"
> >>   
> >>   /***/
> >>   /* ram save/restore */
> >> @@ -76,6 +77,7 @@
> >>   #define RAM_SAVE_FLAG_XBZRLE   0x40
> >>   /* 0x80 is reserved in migration.h start with 0x100 next */
> >>   #define RAM_SAVE_FLAG_COMPRESS_PAGE0x100
> >> +#define RAM_SAVE_FLAG_ENCRYPTED_PAGE   0x200
> > 
> > OK, that's our very last usable flag!  Use it wisely!
> > 
> 
> Hmm, maybe then I missed something. I thought the flag is 64-bit and
> we have more room. Did I miss something ?

The 64bit value written in the stream is 
  (the address of the page) | (the set of flags)

so the set of usable flags depends on the minimum page alignment
of which the worst case is ARM with a TARGET_PAGE_BITS_MIN of 10
(most others are 4k at least but that's still only 2 left).

> 
> >>   static inline bool is_zero_range(uint8_t *p, uint64_t size)
> >>   {
> >> @@ -460,6 +462,9 @@ static QemuCond decomp_done_cond;
> >>   static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock 
> >> *block,
> >>ram_addr_t offset, uint8_t *source_buf);
> >>   
> >> +static int ram_save_encrypted_page(RAMState *rs, PageSearchStatus *pss,
> >> +   bool last_stage);
> >> +
> >>   static void *do_data_compress(void *opaque)
> >>   {
> >>   CompressParam *param = opaque;
> >> @@ -2006,6 +2011,36 @@ static int ram_save_multifd_page(RAMState *rs, 
> >> RAMBlock *block,
> >>   return 1;
> >>   }
> >>   
> >> +/**
> >> + * ram_save_encrypted_page - send the given encrypted page to the stream
> >> + */
> >> +static int ram_save_encrypted_page(RAMState *rs, PageSearchStatus *pss,
> >> +   bool last_stage)
> >> +{
> >> +int ret;
> >> +uint8_t *p;
> >> +RAMBlock *block = pss->block;
> >> +ram_addr_t offset = pss->page << TARGET_PAGE_BITS;
> >> +uint64_t bytes_xmit;
> >> +
> >> +p = block->host + offset;
> >> +
> >> +ram_counters.transferred +=
> >> +save_page_header(rs, rs->f, block,
> >> +offset | RAM_SAVE_FLAG_ENCRYPTED_PAGE);
> >> +
> >> +ret = kvm_memcrypt_save_outgoing_page(rs->f, p,
> > 
> > I think you need to somehow abstract the kvm_memcrypt stuff; nothing
> > else in migration actually knows it's dealing with kvm.  So there
> > should be some indirection - probably through the cpu or the machine
> > type or something.
> > 
> 
> Currently, there are two interfaces by which we can know if we
> are dealing with encrypted guest. kvm_memcrypt_enabled() or
> MachineState->memory_encryption pointer. I did realized that
> migration code have not dealt with kvm so far.
> 
> How about target/i386/sev.c exporting the migration functions and
> based on state of MachineState->memory_encryption we call the
> SEV migration routines for the encrypted pages?

I'm migration not machine; so from my point of view the thing that's
important is making sure we've not got KVM direct dependencies in here;
if you've already got a MachineState->memory_encryption pointer then I'd
hope you could do something like:

ret = MachineState->memory_encryption->ops->save()

> > Also, this isn't bisectable - you can't make this call in this patch
> > because you don't define/declare this function until a later patch.
> > 
> > 
> >> +TARGET_PAGE_SIZE, &bytes_xmit);
> >> +if (ret) {
> >> +return -1;
> >> +}
> >> +
> >> +ram_counters.transferred += bytes_xmit;
> >> +ram_counters.normal++;
> >> +
> >> +return 1;
> >> +}
> >> +
> >>   static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock 
> >> *block,
> >>ram_addr_t offset, uint8_t *source_buf)
> >>   {
> >> @@ -2450,6 +2485,16 @@ static int ram_save_target_pa

Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

[Bartosz Golaszewski]

wt., 9 lip 2019 o 17:59 Geert Uytterhoeven  napisał(a):
>
> Hi Bartosz,
>
> On Tue, Jul 9, 2019 at 4:59 PM Bartosz Golaszewski
>  wrote:
> > pon., 8 lip 2019 o 12:24 Geert Uytterhoeven  
> > napisał(a):
> > > On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski
> > >  wrote:
> > > > pt., 5 lip 2019 o 18:05 Geert Uytterhoeven  
> > > > napisał(a):
> > > > > GPIO controllers are exported to userspace using /dev/gpiochip*
> > > > > character devices.  Access control to these devices is provided by
> > > > > standard UNIX file system permissions, on an all-or-nothing basis:
> > > > > either a GPIO controller is accessible for a user, or it is not.
> > > > > Currently no mechanism exists to control access to individual GPIOs.
> > > > >
> > > > > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 
> > > > > 32),
> > > > > and expose them as a new gpiochip.  This is useful for implementing
> > > > > access control, and assigning a set of GPIOs to a specific user.
> > > > > Furthermore, it would simplify and harden exporting GPIOs to a virtual
> > > > > machine, as the VM can just grab the full virtual GPIO controller, and
> > > > > no longer needs to care about which GPIOs to grab and which not,
> > > > > reducing the attack surface.
> > > > >
> > > > > Virtual GPIO controllers are instantiated by writing to the 
> > > > > "new_device"
> > > > > attribute file in sysfs:
> > > > >
> > > > > $ echo "  [ ...]"
> > > > >"[,   [ ...]] ...]"
> > > > > > /sys/bus/platform/drivers/gpio-virt-agg/new_device
> > > > >
> > > > > Likewise, virtual GPIO controllers can be destroyed after use:
> > > > >
> > > > > $ echo gpio-virt-agg. \
> > > > > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>
> > Am I doing it right? I'm trying to create a device and am only getting this:
> >
> > # echo gpiochip2 23 > new_device
> > [  707.507039] gpio-virt-agg gpio-virt-agg.0: Cannot find gpiochip gpiochip2
> >
> > gpiochip2 *does* exist in the system.
>
> Please try the name of the platform device instead.
> I.e. for my koelsch (R-Car M2-W), it needs "e6052000.gpio" instead
> of "gpiochip2".
>
> Probably the driver should match on both.
>
> > I see. I'll try to review it more thoroughly once I get to play with
> > it. So far I'm stuck on creating the virtual chip.
>
> Thanks, good luck!
>

This is not a show-stopper but one thing that's bothering me in this
is that lines used by the aggregator are considered 'used' in regard
to the original chip. I'm wondering how much effort would it take to
have them be 'muxed' into two (real and virtual) chips at once.

Other than that - seems to works pretty nice other than the matching
by chip name and by line names.

Bart

> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

Re: [Qemu-devel] [RFC 2/5] block: Generic truncation fallback

[Kevin Wolf]

Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
> If a protocol driver does not support truncation, we call fall back to
> effectively not doing anything if the new size is less than the actual
> file size.  This is what we have been doing for some host device drivers
> already.

Specifically, we're doing it for drivers that access a fixed-size image,
i.e. block devices rather than regular files. We don't want to do this
for drivers where the file size could be changed, but just didn't
implement it.

So I would suggest calling the function more specifically something like
bdrv_co_truncate_blockdev(), and not using it as an automatic fallback
in bdrv_co_truncate(), but just make it the BlockDriver.bdrv_co_truncate
implementation for those drivers where it makes sense.

And of course, we only need these fake implementations because qemu-img
(or .bdrv_co_create_opts) always wants to create the protocol level. If
we could avoid this, then we wouldn't need any of this.

Kevin

Re: [Qemu-devel] [PATCH v2 07/13] target/i386: sev: do not create launch context for an incoming guest

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.si...@amd.com) wrote:
> The LAUNCH_START is used for creating an encryption context to encrypt
> newly created guest, for an incoming guest the RECEIVE_START should be
> used.
> 
> Signed-off-by: Brijesh Singh 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  target/i386/sev.c | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 6dbdc3cdf1..49baf8fef0 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -789,10 +789,16 @@ sev_guest_init(const char *id)
>  goto err;
>  }
>  
> -ret = sev_launch_start(s);
> -if (ret) {
> -error_report("%s: failed to create encryption context", __func__);
> -goto err;
> +/*
> + * The LAUNCH context is used for new guest, if its an incoming guest
> + * then RECEIVE context will be created after the connection is 
> established.
> + */
> +if (!runstate_check(RUN_STATE_INMIGRATE)) {
> +ret = sev_launch_start(s);
> +if (ret) {
> +error_report("%s: failed to create encryption context", 
> __func__);
> +goto err;
> +}
>  }
>  
>  ram_block_notifier_add(&sev_ram_notifier);
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] virtio pmem: fix wrong mem region condition

[Cornelia Huck]

On Fri, 12 Jul 2019 13:05:52 +0530
Pankaj Gupta  wrote:

> Coverity reported memory region returns zero
> for non-null value. This is because of wrong
> arguments to '?:' , fixing this.
> 
> Signed-off-by: Pankaj Gupta 
> ---
>  hw/virtio/virtio-pmem-pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Cornelia Huck 

Re: [Qemu-devel] [PATCH v2 08/13] misc.json: add migrate-set-sev-info command

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.si...@amd.com) wrote:
> The command can be used by the hypervisor to specify the target Platform
> Diffie-Hellman key (PDH) and certificate chain before starting the SEV
> guest migration. The values passed through the command will be used while
> creating the outgoing encryption context.
> 
> Signed-off-by: Brijesh Singh 

I'm wondering if it would make sense to have these as migration
parameters rather than using a new command.
You could just use string parameters.
(cc'ing Eric and Daniel for interface suggestions)

> ---
>  qapi/misc-target.json  | 18 ++
>  target/i386/monitor.c  | 10 ++
>  target/i386/sev-stub.c |  5 +
>  target/i386/sev.c  | 11 +++
>  target/i386/sev_i386.h |  9 -
>  5 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index a00fd821eb..938dcaea14 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -266,3 +266,21 @@
>  ##
>  { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
>'if': 'defined(TARGET_ARM)' }
> +
> +##
> +# @migrate-set-sev-info:
> +#
> +# The command is used to provide the target host information used during the
> +# SEV guest.
> +#
> +# @pdh the target host platform diffie-hellman key encoded in base64
> +#
> +# @plat-cert the target host platform certificate chain encoded in base64
> +#
> +# @amd-cert AMD certificate chain which include ASK and OCA encoded in base64
> +#
> +# Since 4.2
> +#
> +##
> +{ 'command': 'migrate-set-sev-info',
> +  'data': { 'pdh': 'str', 'plat-cert': 'str', 'amd-cert' : 'str' }}
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 1f3b532fc2..4a5f50fb45 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -736,3 +736,13 @@ SevCapability *qmp_query_sev_capabilities(Error **errp)
>  
>  return data;
>  }
> +
> +void qmp_migrate_set_sev_info(const char *pdh, const char *plat_cert,
> +  const char *amd_cert, Error **errp)
> +{
> +if (sev_enabled()) {
> +sev_set_migrate_info(pdh, plat_cert, amd_cert);
> +} else {
> +error_setg(errp, "SEV is not enabled");
> +}
> +}
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index e5ee13309c..173bfa6374 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -48,3 +48,8 @@ SevCapability *sev_get_capabilities(void)
>  {
>  return NULL;
>  }
> +
> +void sev_set_migrate_info(const char *pdh, const char *plat_cert,
> +  const char *amd_cert)
> +{
> +}
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 49baf8fef0..6c902d0be8 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -825,6 +825,17 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t 
> len)
>  return 0;
>  }
>  
> +void sev_set_migrate_info(const char *pdh, const char *plat_cert,
> +  const char *amd_cert)
> +{
> +SEVState *s = sev_state;
> +
> +s->remote_pdh = g_base64_decode(pdh, &s->remote_pdh_len);
> +s->remote_plat_cert = g_base64_decode(plat_cert,
> +  &s->remote_plat_cert_len);
> +s->amd_cert = g_base64_decode(amd_cert, &s->amd_cert_len);

What sanity checking is there that it's a valid base64 string of sane
length?

> +}
> +
>  static void
>  sev_register_types(void)
>  {
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index 55313441ae..3f3449b346 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -39,7 +39,8 @@ extern uint32_t sev_get_cbit_position(void);
>  extern uint32_t sev_get_reduced_phys_bits(void);
>  extern char *sev_get_launch_measurement(void);
>  extern SevCapability *sev_get_capabilities(void);
> -
> +extern void sev_set_migrate_info(const char *pdh, const char *plat_cert,
> + const char *amd_cert);
>  typedef struct QSevGuestInfo QSevGuestInfo;
>  typedef struct QSevGuestInfoClass QSevGuestInfoClass;
>  
> @@ -81,6 +82,12 @@ struct SEVState {
>  int sev_fd;
>  SevState state;
>  gchar *measurement;
> +guchar *remote_pdh;
> +size_t remote_pdh_len;
> +guchar *remote_plat_cert;
> +size_t remote_plat_cert_len;
> +guchar *amd_cert;
> +size_t amd_cert_len;
>  };
>  
>  typedef struct SEVState SEVState;
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] virtio pmem: remove memdev null check

[Cornelia Huck]

On Fri, 12 Jul 2019 13:05:53 +0530
Pankaj Gupta  wrote:

> Coverity reports that when we're assigning vi->size we handle the 
> "pmem->memdev is NULL" case; but we then pass it into 
> object_get_canonical_path(), which unconditionally dereferences it
> and will crash if it is NULL. If this pointer can be NULL then we
> need to do something else here.
> 
> We are removing 'pmem->memdev' null check here as memdev will never
> be null in this function.

Indeed, we'll fail to realize the device if it is NULL.

> 
> Signed-off-by: Pankaj Gupta 
> ---
>  hw/virtio/virtio-pmem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info

[Cornelia Huck]

On Fri, 12 Jul 2019 13:05:54 +0530
Pankaj Gupta  wrote:

> Remove transactional & non transactional device info
> for virtio pmem. 

s/device info/names/ ?

> 
> Signed-off-by: Pankaj Gupta 
> ---
>  hw/virtio/virtio-pmem-pci.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [Qemu-devel] [PATCH 0/3] virtio pmem: coverity fixes

[Cornelia Huck]

On Fri, 12 Jul 2019 13:05:51 +0530
Pankaj Gupta  wrote:

> This patch series two fixes for coverity and a 
> transactional info removal patch.
> 
> Pankaj Gupta (3):
>   virtio pmem: fix wrong mem region condition
>   virtio pmem: remove memdev null check
>   virtio pmem: remove transational device info
> 
>  hw/virtio/virtio-pmem-pci.c | 4 +---
>  hw/virtio/virtio-pmem.c | 4 ++--
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 

I think all of this is 4.1 material.

Re: [Qemu-devel] [RFC 3/5] block: Fall back to fallback truncate function

[Kevin Wolf]

Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
> file-posix does not need to basically duplicate our fallback truncate
> implementation; and sheepdog can fall back to it for "shrinking" files.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/file-posix.c | 21 +
>  block/sheepdog.c   |  2 +-
>  2 files changed, 2 insertions(+), 21 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index ab05b51a66..bcddfc7fbe 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2031,23 +2031,7 @@ static int coroutine_fn 
> raw_co_truncate(BlockDriverState *bs, int64_t offset,
>  return raw_regular_truncate(bs, s->fd, offset, prealloc, errp);
>  }

The only thing that is left here is a fstat() check to see that we
really have a regular file here. But since this function is for the
'file' driver, we can just assume this and the function can go away
altogether.

In fact, 'file' with block/character devices has been deprecated since
3.0, so we can just remove support for it now and make it more than just
an assumption.

> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 6f402e5d4d..4af4961cb7 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2301,7 +2301,7 @@ static int coroutine_fn sd_co_truncate(BlockDriverState 
> *bs, int64_t offset,
>  max_vdi_size = (UINT64_C(1) << s->inode.block_size_shift) * 
> MAX_DATA_OBJS;
>  if (offset < old_size) {
>  error_setg(errp, "shrinking is not supported");
> -return -EINVAL;
> +return -ENOTSUP;
>  } else if (offset > max_vdi_size) {
>  error_setg(errp, "too big image size");
>  return -EINVAL;

The image will be unchanged and the guest will keep seeing the old image
size, so is there any use case where having the fallback here makes
sense? The only expected case where an image is shrunk is that the user
explicitly sent block_resize - and in that case returning success, but
doing nothing achieves nothing except confusing the user.

file-posix has the same confusing case, but at least it also has cases
where the fake truncate actually achieves something (such a allowing to
create images on block devices).

Kevin

Re: [Qemu-devel] [PATCH 0/3] virtio pmem: coverity fixes

[Pankaj Gupta]

> 
> On Fri, 12 Jul 2019 13:05:51 +0530
> Pankaj Gupta  wrote:
> 
> > This patch series two fixes for coverity and a
> > transactional info removal patch.
> > 
> > Pankaj Gupta (3):
> >   virtio pmem: fix wrong mem region condition
> >   virtio pmem: remove memdev null check
> >   virtio pmem: remove transational device info
> > 
> >  hw/virtio/virtio-pmem-pci.c | 4 +---
> >  hw/virtio/virtio-pmem.c | 4 ++--
> >  2 files changed, 3 insertions(+), 5 deletions(-)
> > 
> 
> I think all of this is 4.1 material.

Yes, forgot to add in the subject.

Thank you for the review.

Best regards,
Pankaj

> 
> 

Re: [Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info

[Pankaj Gupta]

> 
> On Fri, 12 Jul 2019 13:05:54 +0530
> Pankaj Gupta  wrote:
> 
> > Remove transactional & non transactional device info
> > for virtio pmem.
> 
> s/device info/names/ ?

yes.

> 
> > 
> > Signed-off-by: Pankaj Gupta 
> > ---
> >  hw/virtio/virtio-pmem-pci.c | 2 --
> >  1 file changed, 2 deletions(-)
> 
> Reviewed-by: Cornelia Huck 
> 

Re: [Qemu-devel] [PATCH v2 08/13] misc.json: add migrate-set-sev-info command

[Daniel P . Berrangé]

On Fri, Jul 12, 2019 at 11:00:22AM +0100, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.si...@amd.com) wrote:
> > The command can be used by the hypervisor to specify the target Platform
> > Diffie-Hellman key (PDH) and certificate chain before starting the SEV
> > guest migration. The values passed through the command will be used while
> > creating the outgoing encryption context.
> > 
> > Signed-off-by: Brijesh Singh 
> 
> I'm wondering if it would make sense to have these as migration
> parameters rather than using a new command.
> You could just use string parameters.
> (cc'ing Eric and Daniel for interface suggestions)

Either option would be fine from libvirt's POV I believe. On balance it is
probably slightly easier to deal with migration parameters, since libvirt
already has code for setting many such params.

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] libvhost-user: Add missing GCC_FMT_ATTR and fix three format errors

[Philippe Mathieu-Daudé]

On 7/12/19 10:19 AM, Stefan Weil wrote:
> Signed-off-by: Stefan Weil 
> ---
>  contrib/libvhost-user/libvhost-user.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/libvhost-user/libvhost-user.c 
> b/contrib/libvhost-user/libvhost-user.c
> index 4b36e35a82..59b3202979 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -142,7 +142,7 @@ vu_request_to_string(unsigned int req)
>  }
>  }
>  
> -static void
> +static void GCC_FMT_ATTR(2, 3)
>  vu_panic(VuDev *dev, const char *msg, ...)
>  {
>  char *buf = NULL;
> @@ -661,7 +661,7 @@ vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg 
> *vmsg)
>  
>  if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, ®_struct)) {
>  vu_panic(dev, "%s: Failed to userfault region %d "
> -  "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
> +  "@%" PRIx64 " + size:%zx offset: %zx: 
> (ufd=%d)%s\n",
>   __func__, i,
>   dev_region->mmap_addr,
>   dev_region->size, dev_region->mmap_offset,
> @@ -1753,7 +1753,7 @@ virtqueue_get_head(VuDev *dev, VuVirtq *vq,
>  
>  /* If their number is silly, that's a fatal mistake. */
>  if (*head >= vq->vring.num) {
> -vu_panic(dev, "Guest says index %u is available", head);
> +vu_panic(dev, "Guest says index %u is available", idx);
>  return false;
>  }
>  
> @@ -1812,7 +1812,7 @@ virtqueue_read_next_desc(VuDev *dev, struct vring_desc 
> *desc,
>  smp_wmb();
>  
>  if (*next >= max) {
> -vu_panic(dev, "Desc next is %u", next);
> +vu_panic(dev, "Desc next is %u", *next);
>  return VIRTQUEUE_READ_DESC_ERROR;
>  }
>  
> 

This fixes:

  CC  contrib/libvhost-user/libvhost-user.o
contrib/libvhost-user/libvhost-user.c: In function
'vu_set_mem_table_exec_postcopy':
contrib/libvhost-user/libvhost-user.c:663:27: error: format '%p'
expects argument of type 'void *', but argument 5 has type 'uint64_t
{aka long long unsigned int}' [-Werror=format=]
 vu_panic(dev, "%s: Failed to userfault region %d "
   ^
contrib/libvhost-user/libvhost-user.c: In function 'virtqueue_get_head':
contrib/libvhost-user/libvhost-user.c:1756:42: error: format '%u'
expects argument of type 'unsigned int', but argument 3 has type
'unsigned int *' [-Werror=format=]
 vu_panic(dev, "Guest says index %u is available", head);
  ^
contrib/libvhost-user/libvhost-user.c: In function
'virtqueue_read_next_desc':
contrib/libvhost-user/libvhost-user.c:1815:38: error: format '%u'
expects argument of type 'unsigned int', but argument 3 has type
'unsigned int *' [-Werror=format=]
 vu_panic(dev, "Desc next is %u", next);
  ^

However with your patch applied I still have:

contrib/libvhost-user/libvhost-user.c: In function
'vu_set_mem_table_exec_postcopy':
contrib/libvhost-user/libvhost-user.c:663:27: error: format '%zx'
expects argument of type 'size_t', but argument 6 has type 'uint64_t
{aka long long unsigned int}' [-Werror=format=]
 vu_panic(dev, "%s: Failed to userfault region %d "
   ^
contrib/libvhost-user/libvhost-user.c:663:27: error: format '%zx'
expects argument of type 'size_t', but argument 7 has type 'uint64_t
{aka long long unsigned int}' [-Werror=format=]
cc1: all warnings being treated as errors

Which is right:

typedef struct VuDevRegion {
/* Guest Physical address. */
uint64_t gpa;
/* Memory region size. */
uint64_t size;
/* QEMU virtual address (userspace). */
uint64_t qva;
/* Starting offset in our mmaped space. */
uint64_t mmap_offset;
/* Start address of mmaped space. */
uint64_t mmap_addr;
} VuDevRegion;

Build succeed applying this on top of your patch:

-- >8 --
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -661,7 +661,8 @@ vu_set_mem_table_exec_postcopy(VuDev *dev,
VhostUserMsg *vmsg)

 if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, ®_struct)) {
 vu_panic(dev, "%s: Failed to userfault region %d "
-  "@%" PRIx64 " + size:%zx offset: %zx:
(ufd=%d)%s\n",
+  "@%" PRIx64 " + size:%" PRIx64 " offset: %"
PRIx64
+  ": (ufd=%d)%s\n",
  __func__, i,
  dev_region->mmap_addr,
  dev_region->size, dev_region->mmap_offset,
---

With this snippet amended:
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

Thanks,

Phil.

[Qemu-devel] [PATCH for-4.1 0/2] Compatibility fixes for nettle 2.7 vs 3.0 vs 3.5

[Daniel P . Berrangé]

This short series fixes a few compatibility issues around different
nettle versions.

Daniel P. Berrangé (2):
  crypto: switch to modern nettle AES APIs
  crypto: fix function signatures for nettle 2.7 vs 3

 crypto/cipher-nettle.c | 218 ++---
 crypto/hash-nettle.c   |  12 ++-
 crypto/hmac-nettle.c   |  17 +++-
 3 files changed, 205 insertions(+), 42 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH for-4.1 1/2] crypto: switch to modern nettle AES APIs

[Daniel P . Berrangé]

The aes_ctx struct and aes_* functions have been deprecated in nettle
3.5, in favour of keysize specific functions which were introduced
first in nettle 3.0.

Switch QEMU code to use the new APIs and add some backcompat defines
such that it still builds on nettle 2.7

Signed-off-by: Daniel P. Berrangé 
---
 crypto/cipher-nettle.c | 218 ++---
 1 file changed, 183 insertions(+), 35 deletions(-)

diff --git a/crypto/cipher-nettle.c b/crypto/cipher-nettle.c
index 3848cb3b3a..115d16dd7b 100644
--- a/crypto/cipher-nettle.c
+++ b/crypto/cipher-nettle.c
@@ -42,29 +42,89 @@ typedef void *   cipher_ctx_t;
 typedef unsigned cipher_length_t;
 
 #define cast5_set_key cast128_set_key
+
+#define aes128_ctx aes_ctx
+#define aes192_ctx aes_ctx
+#define aes256_ctx aes_ctx
+#define aes128_set_encrypt_key(c, k) \
+aes_set_encrypt_key(c, 16, k)
+#define aes192_set_encrypt_key(c, k) \
+aes_set_encrypt_key(c, 24, k)
+#define aes256_set_encrypt_key(c, k) \
+aes_set_encrypt_key(c, 32, k)
+#define aes128_set_decrypt_key(c, k) \
+aes_set_decrypt_key(c, 16, k)
+#define aes192_set_decrypt_key(c, k) \
+aes_set_decrypt_key(c, 24, k)
+#define aes256_set_decrypt_key(c, k) \
+aes_set_decrypt_key(c, 32, k)
+#define aes128_encrypt aes_encrypt
+#define aes192_encrypt aes_encrypt
+#define aes256_encrypt aes_encrypt
+#define aes128_decrypt aes_decrypt
+#define aes192_decrypt aes_decrypt
+#define aes256_decrypt aes_decrypt
 #else
 typedef nettle_cipher_func * QCryptoCipherNettleFuncNative;
 typedef const void * cipher_ctx_t;
 typedef size_t   cipher_length_t;
 #endif
 
-typedef struct QCryptoNettleAES {
-struct aes_ctx enc;
-struct aes_ctx dec;
-} QCryptoNettleAES;
+typedef struct QCryptoNettleAES128 {
+struct aes128_ctx enc;
+struct aes128_ctx dec;
+} QCryptoNettleAES128;
+
+typedef struct QCryptoNettleAES192 {
+struct aes192_ctx enc;
+struct aes192_ctx dec;
+} QCryptoNettleAES192;
+
+typedef struct QCryptoNettleAES256 {
+struct aes256_ctx enc;
+struct aes256_ctx dec;
+} QCryptoNettleAES256;
+
+static void aes128_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
+  uint8_t *dst, const uint8_t *src)
+{
+const QCryptoNettleAES128 *aesctx = ctx;
+aes128_encrypt(&aesctx->enc, length, dst, src);
+}
+
+static void aes128_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
+  uint8_t *dst, const uint8_t *src)
+{
+const QCryptoNettleAES128 *aesctx = ctx;
+aes128_decrypt(&aesctx->dec, length, dst, src);
+}
+
+static void aes192_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
+   uint8_t *dst, const uint8_t *src)
+{
+const QCryptoNettleAES192 *aesctx = ctx;
+aes192_encrypt(&aesctx->enc, length, dst, src);
+}
+
+static void aes192_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
+   uint8_t *dst, const uint8_t *src)
+{
+const QCryptoNettleAES192 *aesctx = ctx;
+aes192_decrypt(&aesctx->dec, length, dst, src);
+}
 
-static void aes_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
+static void aes256_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
uint8_t *dst, const uint8_t *src)
 {
-const QCryptoNettleAES *aesctx = ctx;
-aes_encrypt(&aesctx->enc, length, dst, src);
+const QCryptoNettleAES256 *aesctx = ctx;
+aes256_encrypt(&aesctx->enc, length, dst, src);
 }
 
-static void aes_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
+static void aes256_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
uint8_t *dst, const uint8_t *src)
 {
-const QCryptoNettleAES *aesctx = ctx;
-aes_decrypt(&aesctx->dec, length, dst, src);
+const QCryptoNettleAES256 *aesctx = ctx;
+aes256_decrypt(&aesctx->dec, length, dst, src);
 }
 
 static void des_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
@@ -127,18 +187,46 @@ static void twofish_decrypt_native(cipher_ctx_t ctx, 
cipher_length_t length,
 twofish_decrypt(ctx, length, dst, src);
 }
 
-static void aes_encrypt_wrapper(const void *ctx, size_t length,
+static void aes128_encrypt_wrapper(const void *ctx, size_t length,
+uint8_t *dst, const uint8_t *src)
+{
+const QCryptoNettleAES128 *aesctx = ctx;
+aes128_encrypt(&aesctx->enc, length, dst, src);
+}
+
+static void aes128_decrypt_wrapper(const void *ctx, size_t length,
 uint8_t *dst, const uint8_t *src)
 {
-const QCryptoNettleAES *aesctx = ctx;
-aes_encrypt(&aesctx->enc, length, dst, src);
+const QCryptoNettleAES128 *aesctx = ctx;
+aes128_decrypt(&aesctx->dec, length, dst, src);
 }
 
-static void aes_decrypt_wrapper(const void *ctx, size_t length,
+static void aes192_encrypt_wrapper(const void *ctx, size_t length,
 uint8_t *dst, const uint8_t *src)

[Qemu-devel] [PATCH for-4.1 2/2] crypto: fix function signatures for nettle 2.7 vs 3

[Daniel P . Berrangé]

Nettle version 2.7.x used 'unsigned int' instead of 'size_t' for length
parameters in functions. Use a local typedef so that we can build with
the correct signature depending on nettle version, as we already do in
the cipher code.

Signed-off-by: Daniel P. Berrangé 
---
 crypto/hash-nettle.c | 12 +---
 crypto/hmac-nettle.c | 17 +
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
index 96f186f442..6ffb9c3db7 100644
--- a/crypto/hash-nettle.c
+++ b/crypto/hash-nettle.c
@@ -26,12 +26,18 @@
 #include 
 #include 
 
+#if CONFIG_NETTLE_VERSION_MAJOR < 3
+typedef unsigned int hash_length_t;
+#else
+typedef size_t   hash_length_t;
+#endif
+
 typedef void (*qcrypto_nettle_init)(void *ctx);
 typedef void (*qcrypto_nettle_write)(void *ctx,
- unsigned int len,
+ hash_length_t len,
  const uint8_t *buf);
 typedef void (*qcrypto_nettle_result)(void *ctx,
-  unsigned int len,
+  hash_length_t len,
   uint8_t *buf);
 
 union qcrypto_hash_ctx {
@@ -112,7 +118,7 @@ qcrypto_nettle_hash_bytesv(QCryptoHashAlgorithm alg,
size_t *resultlen,
Error **errp)
 {
-int i;
+size_t i;
 union qcrypto_hash_ctx ctx;
 
 if (!qcrypto_hash_supports(alg)) {
diff --git a/crypto/hmac-nettle.c b/crypto/hmac-nettle.c
index ec2d61bdde..1152b741fd 100644
--- a/crypto/hmac-nettle.c
+++ b/crypto/hmac-nettle.c
@@ -18,14 +18,23 @@
 #include "hmacpriv.h"
 #include 
 
+#if CONFIG_NETTLE_VERSION_MAJOR < 3
+typedef unsigned int hmac_length_t;
+#else
+typedef size_t hmac_length_t;
+#endif
+
 typedef void (*qcrypto_nettle_hmac_setkey)(void *ctx,
-  size_t key_length, const uint8_t *key);
+   hmac_length_t key_length,
+   const uint8_t *key);
 
 typedef void (*qcrypto_nettle_hmac_update)(void *ctx,
-  size_t length, const uint8_t *data);
+   hmac_length_t length,
+   const uint8_t *data);
 
 typedef void (*qcrypto_nettle_hmac_digest)(void *ctx,
-  size_t length, uint8_t *digest);
+   hmac_length_t length,
+   uint8_t *digest);
 
 typedef struct QCryptoHmacNettle QCryptoHmacNettle;
 struct QCryptoHmacNettle {
@@ -135,7 +144,7 @@ qcrypto_nettle_hmac_bytesv(QCryptoHmac *hmac,
Error **errp)
 {
 QCryptoHmacNettle *ctx;
-int i;
+size_t i;
 
 ctx = (QCryptoHmacNettle *)hmac->opaque;
 
-- 
2.21.0

Re: [Qemu-devel] [Qemu-block] [PATCH] doc: Preallocation does not require writing zeroes

[Stefano Garzarella]

On Thu, Jul 11, 2019 at 03:29:35PM +0200, Max Reitz wrote:
> When preallocating an encrypted qcow2 image, it just lets the protocol
> driver write data and then does not mark the clusters as zero.
> Therefore, reading this image will yield effectively random data.
> 
> As such, we have not fulfilled the promise of always writing zeroes when
> preallocating an image in a while.  It seems that nobody has really
> cared, so change the documentation to conform to qemu's actual behavior.
> 
> Signed-off-by: Max Reitz 
> ---
>  qapi/block-core.json | 9 +
>  docs/qemu-block-drivers.texi | 4 ++--
>  qemu-img.texi| 4 ++--
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0d43d4f37c..a4363b84d2 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5167,10 +5167,11 @@
>  # @off: no preallocation
>  # @metadata: preallocate only for metadata
>  # @falloc: like @full preallocation but allocate disk space by
> -#  posix_fallocate() rather than writing zeros.
> -# @full: preallocate all data by writing zeros to device to ensure disk
> -#space is really available. @full preallocation also sets up
> -#metadata correctly.
> +#  posix_fallocate() rather than writing data.
> +# @full: preallocate all data by writing it to the device to ensure
> +#disk space is really available. This data may or may not be
> +#zero, depending on the image format and storage.
> +#@full preallocation also sets up metadata correctly.
>  #
>  # Since: 2.2
>  ##
> diff --git a/docs/qemu-block-drivers.texi b/docs/qemu-block-drivers.texi
> index 91ab0eceae..c02547e28c 100644
> --- a/docs/qemu-block-drivers.texi
> +++ b/docs/qemu-block-drivers.texi
> @@ -31,8 +31,8 @@ Supported options:
>  @item preallocation
>  Preallocation mode (allowed values: @code{off}, @code{falloc}, @code{full}).
>  @code{falloc} mode preallocates space for image by calling posix_fallocate().
> -@code{full} mode preallocates space for image by writing zeros to underlying
> -storage.
> +@code{full} mode preallocates space for image by writing data to underlying
> +storage.  This data may or may not be zero, depending on the storage 
> location.
>  @end table
>  
>  @item qcow2
> diff --git a/qemu-img.texi b/qemu-img.texi
> index c8e9bba515..b5156d6316 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -666,8 +666,8 @@ Supported options:
>  @item preallocation
>  Preallocation mode (allowed values: @code{off}, @code{falloc}, @code{full}).
>  @code{falloc} mode preallocates space for image by calling posix_fallocate().
> -@code{full} mode preallocates space for image by writing zeros to underlying
> -storage.
> +@code{full} mode preallocates space for image by writing data to underlying
> +storage.  This data may or may not be zero, depending on the storage 
> location.
>  @end table
>  
>  @item qcow2

Just a question:
if a protocol driver returns 1 with the .bdrv_has_zero_init callback, is it
expected that the preallocation will fill the image with zeroes?

IIUC, for example, the qcow2 returns 1 with the .bdrv_has_zero_init. but during
the qcow2_co_truncate() it calls bdrv_co_truncate() and, depending on the
backend driver, it should fill the image with zeroes (or not?).

Maybe I miss something related to the metadata...


Thanks,
Stefano

Re: [Qemu-devel] [PATCH v2 09/13] target/i386: sev: add support to encrypt the outgoing page

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.si...@amd.com) wrote:
> The sev_save_outgoing_page() provide the implementation to encrypt the
> guest private pages during the transit. The routines uses the SEND_START
> command to create the outgoing encryption context on the first call then
> uses the SEND_UPDATE_DATA command to encrypt the data before writing it
> to the socket. While encrypting the data SEND_UPDATE_DATA produces some
> metadata (e.g MAC, IV). The metadata is also sent to the target machine.
> After migration is completed, we issue the SEND_FINISH command to transition
> the SEV guest state from sending to unrunnable state.
> 
> Signed-off-by: Brijesh Singh 
> ---
>  accel/kvm/kvm-all.c  |   1 +
>  target/i386/sev.c| 229 +++
>  target/i386/sev_i386.h   |   2 +
>  target/i386/trace-events |   3 +
>  4 files changed, 235 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c935e9366c..a9fb447248 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1792,6 +1792,7 @@ static int kvm_init(MachineState *ms)
>  }
>  
>  kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
> +kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
>  }
>  
>  ret = kvm_arch_init(ms, s);
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 6c902d0be8..28b36c8035 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -27,6 +27,8 @@
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
>  #include "migration/blocker.h"
> +#include "migration/qemu-file.h"
> +#include "migration/misc.h"
>  
>  #define DEFAULT_GUEST_POLICY0x1 /* disable debug */
>  #define DEFAULT_SEV_DEVICE  "/dev/sev"
> @@ -718,6 +720,39 @@ sev_vm_state_change(void *opaque, int running, RunState 
> state)
>  }
>  }
>  
> +static void
> +sev_send_finish(void)
> +{
> +int ret, error;
> +
> +trace_kvm_sev_send_finish();
> +ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_SEND_FINISH, 0, &error);
> +if (ret) {
> +error_report("%s: LAUNCH_FINISH ret=%d fw_error=%d '%s'",

why LAUNCH?

> + __func__, ret, error, fw_error_to_str(error));
> +}
> +
> +sev_set_guest_state(SEV_STATE_RUNNING);
> +}
> +
> +static void
> +sev_migration_state_notifier(Notifier *notifier, void *data)
> +{
> +MigrationState *s = data;
> +
> +if (migration_has_finished(s) ||
> +migration_in_postcopy_after_devices(s) ||
> +migration_has_failed(s)) {
> +if (sev_check_state(SEV_STATE_SEND_UPDATE)) {
> +sev_send_finish();
> +}

I don't quite understand SEV_SEND_FINISH; is it just terminating the
migration process or is it actually making the VM unrunnable?
I'm interested in what the behaviour is on a failed migration - do
we lose both VMs or do we potentialyl have a memory clone?
(Neither are pretty!)

> +}
> +}
> +
> +static Notifier sev_migration_state_notify = {
> +.notify = sev_migration_state_notifier,
> +};
> +
>  void *
>  sev_guest_init(const char *id)
>  {
> @@ -804,6 +839,7 @@ sev_guest_init(const char *id)
>  ram_block_notifier_add(&sev_ram_notifier);
>  qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
>  qemu_add_vm_change_state_handler(sev_vm_state_change, s);
> +add_migration_state_change_notifier(&sev_migration_state_notify);
>  
>  return s;
>  err:
> @@ -836,6 +872,199 @@ void sev_set_migrate_info(const char *pdh, const char 
> *plat_cert,
>  s->amd_cert = g_base64_decode(amd_cert, &s->amd_cert_len);
>  }
>  
> +static int
> +sev_get_send_session_length(void)
> +{
> +int ret, fw_err = 0;
> +struct kvm_sev_send_start *start;
> +
> +start = g_new0(struct kvm_sev_send_start, 1);

These are tiny structures; they may as well be on the stack rather than
allocating/freeing them.

> +ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_SEND_START, start, &fw_err);
> +if (fw_err != SEV_RET_INVALID_LEN) {
> +ret = -1;
> +error_report("%s: failed to get session length ret=%d fw_error=%d 
> '%s'",
> + __func__, ret, fw_err, fw_error_to_str(fw_err));
> +goto err;
> +}
> +
> +ret = start->session_len;
> +err:
> +g_free(start);
> +return ret;
> +}
> +
> +static int
> +sev_send_start(SEVState *s, QEMUFile *f, uint64_t *bytes_sent)
> +{
> +gsize pdh_len = 0, plat_cert_len;
> +int session_len, ret, fw_error;
> +struct kvm_sev_send_start *start;
> +guchar *pdh = NULL, *plat_cert = NULL, *session = NULL;
> +
> +if (!s->remote_pdh || !s->remote_plat_cert) {
> +error_report("%s: missing remote PDH or PLAT_CERT", __func__);
> +return 1;
> +}
> +
> +start = g_new0(struct kvm_sev_send_start, 1);
> +
> +start->pdh_cert_uaddr = (unsigned long) s->remote_pdh;
> +start->pdh_cert_len = s->remote_pdh_len;
> +
> +start->plat_cert_uaddr = (unsigned long)s->remote_plat_cert;
> +start->plat_cert_len = 

[Qemu-devel] [PATCH] gluster: fix .bdrv_reopen_prepare when backing file is a JSON object

[Stefano Garzarella]

When the backing_file is specified as a JSON object, the
qemu_gluster_reopen_prepare() fails with this message:
invalid URI json:{"server.0.host": ...}

In this case, we should call qemu_gluster_init() using the QDict
'state->options' that contains the parameters already parsed.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1542445
Signed-off-by: Stefano Garzarella 
---
 block/gluster.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/gluster.c b/block/gluster.c
index 62f8ff2147..26971db1ea 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -931,7 +931,16 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState 
*state,
 gconf->has_debug = true;
 gconf->logfile = g_strdup(s->logfile);
 gconf->has_logfile = true;
-reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp);
+/*
+ * If 'bs->filename' starts with "json:", then 'state->options' will
+ * contain the parameters already parsed.
+ */
+if (state->bs->filename && !strstart(state->bs->filename, "json:", NULL)) {
+reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL,
+ errp);
+} else {
+reop_s->glfs = qemu_gluster_init(gconf, NULL, state->options, errp);
+}
 if (reop_s->glfs == NULL) {
 ret = -errno;
 goto exit;
-- 
2.20.1

Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close()

[Max Reitz]

On 12.07.19 11:24, Kevin Wolf wrote:
> Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
>> When nbd_close() is called from a coroutine, the connection_co never
>> gets to run, and thus nbd_teardown_connection() hangs.
>>
>> This is because aio_co_enter() only puts the connection_co into the main
>> coroutine's wake-up queue, so this main coroutine needs to yield and
>> reschedule itself to let the connection_co run.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  block/nbd.c | 12 +++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 81edabbf35..b83b6cd43e 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -135,7 +135,17 @@ static void nbd_teardown_connection(BlockDriverState 
>> *bs)
>>  qio_channel_shutdown(s->ioc,
>>   QIO_CHANNEL_SHUTDOWN_BOTH,
>>   NULL);
>> -BDRV_POLL_WHILE(bs, s->connection_co);
>> +
>> +if (qemu_in_coroutine()) {
>> +/* Let our caller poll and just yield until connection_co is done */
>> +while (s->connection_co) {
>> +aio_co_schedule(qemu_get_current_aio_context(),
>> +qemu_coroutine_self());
>> +qemu_coroutine_yield();
>> +}
> 
> Isn't this busy waiting? Why not let s->connection_co wake us up when
> it's about to terminate instead of immediately rescheduling ourselves?

Yes, it is busy waiting, but I didn’t find that bad.  The connection_co
will be invoked in basically every iteration, and once there is no
pending data, it will quit.

The answer to “why not...” of course is because it’d be more complicated.

But anyway.

Adding a new function qemu_coroutine_run_after(target) that adds
qemu_coroutine_self() to the given @target coroutine’s wake-up queue and
then using that instead of scheduling works, too, yes.

I don’t really like being responsible for coroutine code, though...

(And maybe it’d be better to make it qemu_coroutine_yield_for(target),
which does the above and then yields?)

Max

>> +} else {
>> +BDRV_POLL_WHILE(bs, s->connection_co);
>> +}
>>  
>>  nbd_client_detach_aio_context(bs);
>>  object_unref(OBJECT(s->sioc));
> 
> Kevin
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC 2/5] block: Generic truncation fallback

[Max Reitz]

On 12.07.19 11:49, Kevin Wolf wrote:
> Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
>> If a protocol driver does not support truncation, we call fall back to
>> effectively not doing anything if the new size is less than the actual
>> file size.  This is what we have been doing for some host device drivers
>> already.
> 
> Specifically, we're doing it for drivers that access a fixed-size image,
> i.e. block devices rather than regular files. We don't want to do this
> for drivers where the file size could be changed, but just didn't
> implement it.
> 
> So I would suggest calling the function more specifically something like
> bdrv_co_truncate_blockdev(), and not using it as an automatic fallback
> in bdrv_co_truncate(), but just make it the BlockDriver.bdrv_co_truncate
> implementation for those drivers where it makes sense.

I was thinking about this, but the problem is that .bdrv_co_truncate()
does not get a BdrvChild, so an implementation for it cannot generically
zero the first sector (without bypassing the permission system, which
would be wrong).

So the function pointer would actually need to be set to something like
(int (*)(BlockDriverState *, int64_t, PreallocMode, Error **))42ul, or a
dummy function that just aborts, and then bdrv_co_truncate() would
recognize this magic constant.  But that seemed so weird to me that I
decided just not to do it, mostly because I was wondering what would be
so bad about treating images whose size we cannot change because we
haven’t implemented it exactly like fixed-size images.

(Also, “fixed-size” is up to interpretation.  You can change an LVM
volume’s size.  qemu doesn’t do it, obviously.  But that is the reason
for the warning qemu-img resize emits when it sees that the file size
did not change.)

> And of course, we only need these fake implementations because qemu-img
> (or .bdrv_co_create_opts) always wants to create the protocol level. If
> we could avoid this, then we wouldn't need any of this.

It’s trivial to avoid this.  I mean, patch 4 does exactly that.

So it isn’t about avoiding creating the protocol level, it’s about
avoiding the truncation there.  But why would you do that?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close()

[Kevin Wolf]

Am 12.07.2019 um 12:47 hat Max Reitz geschrieben:
> On 12.07.19 11:24, Kevin Wolf wrote:
> > Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
> >> When nbd_close() is called from a coroutine, the connection_co never
> >> gets to run, and thus nbd_teardown_connection() hangs.
> >>
> >> This is because aio_co_enter() only puts the connection_co into the main
> >> coroutine's wake-up queue, so this main coroutine needs to yield and
> >> reschedule itself to let the connection_co run.
> >>
> >> Signed-off-by: Max Reitz 
> >> ---
> >>  block/nbd.c | 12 +++-
> >>  1 file changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/block/nbd.c b/block/nbd.c
> >> index 81edabbf35..b83b6cd43e 100644
> >> --- a/block/nbd.c
> >> +++ b/block/nbd.c
> >> @@ -135,7 +135,17 @@ static void nbd_teardown_connection(BlockDriverState 
> >> *bs)
> >>  qio_channel_shutdown(s->ioc,
> >>   QIO_CHANNEL_SHUTDOWN_BOTH,
> >>   NULL);
> >> -BDRV_POLL_WHILE(bs, s->connection_co);
> >> +
> >> +if (qemu_in_coroutine()) {
> >> +/* Let our caller poll and just yield until connection_co is done 
> >> */
> >> +while (s->connection_co) {
> >> +aio_co_schedule(qemu_get_current_aio_context(),
> >> +qemu_coroutine_self());
> >> +qemu_coroutine_yield();
> >> +}
> > 
> > Isn't this busy waiting? Why not let s->connection_co wake us up when
> > it's about to terminate instead of immediately rescheduling ourselves?
> 
> Yes, it is busy waiting, but I didn’t find that bad.  The connection_co
> will be invoked in basically every iteration, and once there is no
> pending data, it will quit.
> 
> The answer to “why not...” of course is because it’d be more complicated.
> 
> But anyway.
> 
> Adding a new function qemu_coroutine_run_after(target) that adds
> qemu_coroutine_self() to the given @target coroutine’s wake-up queue and
> then using that instead of scheduling works, too, yes.
> 
> I don’t really like being responsible for coroutine code, though...
> 
> (And maybe it’d be better to make it qemu_coroutine_yield_for(target),
> which does the above and then yields?)

Or just do something like this, which is arguably not only a fix for the
busy wait, but also a code simplification:

diff --git a/block/nbd.c b/block/nbd.c
index b83b6cd43e..c061bd1bfc 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -61,6 +61,7 @@ typedef struct BDRVNBDState {
 CoMutex send_mutex;
 CoQueue free_sema;
 Coroutine *connection_co;
+Coroutine *teardown_co;
 int in_flight;

 NBDClientRequest requests[MAX_NBD_REQUESTS];
@@ -137,12 +138,9 @@ static void nbd_teardown_connection(BlockDriverState *bs)
  NULL);

 if (qemu_in_coroutine()) {
-/* Let our caller poll and just yield until connection_co is done */
-while (s->connection_co) {
-aio_co_schedule(qemu_get_current_aio_context(),
-qemu_coroutine_self());
-qemu_coroutine_yield();
-}
+/* just yield until connection_co is done */
+s->teardown_co = qemu_coroutine_self();
+qemu_coroutine_yield();
 } else {
 BDRV_POLL_WHILE(bs, s->connection_co);
 }
@@ -217,6 +215,9 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
 bdrv_dec_in_flight(s->bs);

 s->connection_co = NULL;
+if (s->teardown_co) {
+aio_co_wake(s->teardown_co);
+}
 aio_wait_kick();
 }


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 10/13] target/i386: sev: add support to load incoming encrypted page

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.si...@amd.com) wrote:
> The sev_load_incoming_page() provide the implementation to read the
> incoming guest private pages from the socket and load it into the guest
> memory. The routines uses the RECEIVE_START command to create the
> incoming encryption context on the first call then uses the
> RECEIEVE_UPDATE_DATA command to load the encrypted pages into the guest
> memory. After migration is completed, we issue the RECEIVE_FINISH command
> to transition the SEV guest to the runnable state so that it can be
> executed.
> 
> Signed-off-by: Brijesh Singh 
> ---
>  accel/kvm/kvm-all.c  |   1 +
>  target/i386/sev.c| 126 ++-
>  target/i386/trace-events |   3 +
>  3 files changed, 129 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index a9fb447248..7f94dba6f9 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1793,6 +1793,7 @@ static int kvm_init(MachineState *ms)
>  
>  kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
>  kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
> +kvm_state->memcrypt_load_incoming_page = sev_load_incoming_page;
>  }
>  
>  ret = kvm_arch_init(ms, s);
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 28b36c8035..09a62d6f88 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -708,13 +708,34 @@ sev_launch_finish(SEVState *s)
>  }
>  }
>  
> +static int
> +sev_receive_finish(SEVState *s)
> +{
> +int error, ret = 1;
> +
> +trace_kvm_sev_receive_finish();
> +ret = sev_ioctl(s->sev_fd, KVM_SEV_RECEIVE_FINISH, 0, &error);
> +if (ret) {
> +error_report("%s: RECEIVE_FINISH ret=%d fw_error=%d '%s'",
> +__func__, ret, error, fw_error_to_str(error));
> +goto err;
> +}
> +
> +sev_set_guest_state(SEV_STATE_RUNNING);
> +err:
> +return ret;
> +}
> +
> +
>  static void
>  sev_vm_state_change(void *opaque, int running, RunState state)
>  {
>  SEVState *s = opaque;
>  
>  if (running) {
> -if (!sev_check_state(SEV_STATE_RUNNING)) {
> +if (sev_check_state(SEV_STATE_RECEIVE_UPDATE)) {
> +sev_receive_finish(s);
> +} else if (!sev_check_state(SEV_STATE_RUNNING)) {
>  sev_launch_finish(s);
>  }
>  }
> @@ -1065,6 +1086,109 @@ int sev_save_outgoing_page(void *handle, QEMUFile *f, 
> uint8_t *ptr,
>  return sev_send_update_data(s, f, ptr, sz, bytes_sent);
>  }
>  
> +static int
> +sev_receive_start(QSevGuestInfo *sev, QEMUFile *f)
> +{
> +int ret = 1;
> +int fw_error;
> +struct kvm_sev_receive_start *start;
> +gchar *session = NULL, *pdh_cert = NULL;
> +
> +start = g_new0(struct kvm_sev_receive_start, 1);

Same as the send patch; these are tiny so may as well be on the stack

> +/* get SEV guest handle */
> +start->handle = object_property_get_int(OBJECT(sev), "handle",
> +&error_abort);
> +
> +/* get the source policy */
> +start->policy = qemu_get_be32(f);
> +
> +/* get source PDH key */
> +start->pdh_len = qemu_get_be32(f);

You might want to bound the sizes of pdh_len and session_len
on reading; if the migration stream is badly corrupt you could
end up allocating and then trying to read a few GB ofjunk off the wire.

> +pdh_cert = g_new(gchar, start->pdh_len);
> +qemu_get_buffer(f, (uint8_t *)pdh_cert, start->pdh_len);
> +start->pdh_uaddr = (unsigned long)pdh_cert;
> +
> +/* get source session data */
> +start->session_len = qemu_get_be32(f);
> +session = g_new(gchar, start->session_len);
> +qemu_get_buffer(f, (uint8_t *)session, start->session_len);
> +start->session_uaddr = (unsigned long)session;
> +
> +trace_kvm_sev_receive_start(start->policy, session, pdh_cert);
> +
> +ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_RECEIVE_START, start, 
> &fw_error);
> +if (ret < 0) {
> +error_report("Error RECEIVE_START ret=%d fw_error=%d '%s'",
> +ret, fw_error, fw_error_to_str(fw_error));
> +goto err;
> +}
> +
> +object_property_set_int(OBJECT(sev), start->handle, "handle", 
> &error_abort);
> +sev_set_guest_state(SEV_STATE_RECEIVE_UPDATE);
> +err:
> +g_free(start);
> +g_free(session);
> +g_free(pdh_cert);
> +
> +return ret;
> +}
> +
> +static int sev_receive_update_data(QEMUFile *f, uint8_t *ptr)
> +{
> +int ret = 1, fw_error = 0;
> +gchar *hdr = NULL, *trans = NULL;
> +struct kvm_sev_receive_update_data *update;
> +
> +update = g_new0(struct kvm_sev_receive_update_data, 1);

Similar comments to the _start function

> +/* get packet header */
> +update->hdr_len = qemu_get_be32(f);
> +hdr = g_new(gchar, update->hdr_len);
> +qemu_get_buffer(f, (uint8_t *)hdr, update->hdr_len);
> +update->hdr_uaddr = (unsigned long)hdr;
> +
> +/* get transport buffer */
> +update->trans_len = qem

Re: [Qemu-devel] [RFC 3/5] block: Fall back to fallback truncate function

[Max Reitz]

On 12.07.19 12:04, Kevin Wolf wrote:
> Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
>> file-posix does not need to basically duplicate our fallback truncate
>> implementation; and sheepdog can fall back to it for "shrinking" files.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  block/file-posix.c | 21 +
>>  block/sheepdog.c   |  2 +-
>>  2 files changed, 2 insertions(+), 21 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index ab05b51a66..bcddfc7fbe 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -2031,23 +2031,7 @@ static int coroutine_fn 
>> raw_co_truncate(BlockDriverState *bs, int64_t offset,
>>  return raw_regular_truncate(bs, s->fd, offset, prealloc, errp);
>>  }
> 
> The only thing that is left here is a fstat() check to see that we
> really have a regular file here. But since this function is for the
> 'file' driver, we can just assume this and the function can go away
> altogether.
> 
> In fact, 'file' with block/character devices has been deprecated since
> 3.0, so we can just remove support for it now and make it more than just
> an assumption.
> 
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index 6f402e5d4d..4af4961cb7 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -2301,7 +2301,7 @@ static int coroutine_fn 
>> sd_co_truncate(BlockDriverState *bs, int64_t offset,
>>  max_vdi_size = (UINT64_C(1) << s->inode.block_size_shift) * 
>> MAX_DATA_OBJS;
>>  if (offset < old_size) {
>>  error_setg(errp, "shrinking is not supported");
>> -return -EINVAL;
>> +return -ENOTSUP;
>>  } else if (offset > max_vdi_size) {
>>  error_setg(errp, "too big image size");
>>  return -EINVAL;
> 
> The image will be unchanged and the guest will keep seeing the old image
> size, so is there any use case where having the fallback here makes
> sense? The only expected case where an image is shrunk is that the user
> explicitly sent block_resize - and in that case returning success, but
> doing nothing achieves nothing except confusing the user.
> 
> file-posix has the same confusing case, but at least it also has cases
> where the fake truncate actually achieves something (such a allowing to
> create images on block devices).

Hm, yes, that’s right.  It is as confusing for block devices, but that
at least gives something in return.  For sheepdog (and others, like
ssh), there is nothing in return.

Explaining for every block driver why it needs to be a bit confusing and
fall back to the fixed-size device implementation (because it doesn’t
implement creation) seems a bit off, too.

Hm.  Maybe the protocol creation fallback should just ignore failures
when truncating an image and in such a case zero the first sector of the
image?  Maybe it should just ignore ENOTSUP errors?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close()

[Max Reitz]

On 12.07.19 13:01, Kevin Wolf wrote:
> Am 12.07.2019 um 12:47 hat Max Reitz geschrieben:
>> On 12.07.19 11:24, Kevin Wolf wrote:
>>> Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
 When nbd_close() is called from a coroutine, the connection_co never
 gets to run, and thus nbd_teardown_connection() hangs.

 This is because aio_co_enter() only puts the connection_co into the main
 coroutine's wake-up queue, so this main coroutine needs to yield and
 reschedule itself to let the connection_co run.

 Signed-off-by: Max Reitz 
 ---
  block/nbd.c | 12 +++-
  1 file changed, 11 insertions(+), 1 deletion(-)

 diff --git a/block/nbd.c b/block/nbd.c
 index 81edabbf35..b83b6cd43e 100644
 --- a/block/nbd.c
 +++ b/block/nbd.c
 @@ -135,7 +135,17 @@ static void nbd_teardown_connection(BlockDriverState 
 *bs)
  qio_channel_shutdown(s->ioc,
   QIO_CHANNEL_SHUTDOWN_BOTH,
   NULL);
 -BDRV_POLL_WHILE(bs, s->connection_co);
 +
 +if (qemu_in_coroutine()) {
 +/* Let our caller poll and just yield until connection_co is done 
 */
 +while (s->connection_co) {
 +aio_co_schedule(qemu_get_current_aio_context(),
 +qemu_coroutine_self());
 +qemu_coroutine_yield();
 +}
>>>
>>> Isn't this busy waiting? Why not let s->connection_co wake us up when
>>> it's about to terminate instead of immediately rescheduling ourselves?
>>
>> Yes, it is busy waiting, but I didn’t find that bad.  The connection_co
>> will be invoked in basically every iteration, and once there is no
>> pending data, it will quit.
>>
>> The answer to “why not...” of course is because it’d be more complicated.
>>
>> But anyway.
>>
>> Adding a new function qemu_coroutine_run_after(target) that adds
>> qemu_coroutine_self() to the given @target coroutine’s wake-up queue and
>> then using that instead of scheduling works, too, yes.
>>
>> I don’t really like being responsible for coroutine code, though...
>>
>> (And maybe it’d be better to make it qemu_coroutine_yield_for(target),
>> which does the above and then yields?)
> 
> Or just do something like this, which is arguably not only a fix for the
> busy wait, but also a code simplification:

1. Is that guaranteed to work?  What if data sneaks in, the
connection_co handles that, and doesn’t wake up the teardown_co?  Will
it be re-scheduled?

2. I precisely didn’t want to do this because we have this functionality
already in the form of Coroutine.co_queue_wakeup.  Why duplicate it here?

Max

> diff --git a/block/nbd.c b/block/nbd.c
> index b83b6cd43e..c061bd1bfc 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -61,6 +61,7 @@ typedef struct BDRVNBDState {
>  CoMutex send_mutex;
>  CoQueue free_sema;
>  Coroutine *connection_co;
> +Coroutine *teardown_co;
>  int in_flight;
> 
>  NBDClientRequest requests[MAX_NBD_REQUESTS];
> @@ -137,12 +138,9 @@ static void nbd_teardown_connection(BlockDriverState *bs)
>   NULL);
> 
>  if (qemu_in_coroutine()) {
> -/* Let our caller poll and just yield until connection_co is done */
> -while (s->connection_co) {
> -aio_co_schedule(qemu_get_current_aio_context(),
> -qemu_coroutine_self());
> -qemu_coroutine_yield();
> -}
> +/* just yield until connection_co is done */
> +s->teardown_co = qemu_coroutine_self();
> +qemu_coroutine_yield();
>  } else {
>  BDRV_POLL_WHILE(bs, s->connection_co);
>  }
> @@ -217,6 +215,9 @@ static coroutine_fn void nbd_connection_entry(void 
> *opaque)
>  bdrv_dec_in_flight(s->bs);
> 
>  s->connection_co = NULL;
> +if (s->teardown_co) {
> +aio_co_wake(s->teardown_co);
> +}
>  aio_wait_kick();
>  }
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH RFC v5 1/3] tests: New make target check-source

[Markus Armbruster]

Richard Henderson  writes:

> On 7/11/19 2:28 PM, Markus Armbruster wrote:
>>  include/exec/cpu_ldst_template.h  |  3 +
>>  include/exec/cpu_ldst_useronly_template.h |  3 +
>>  include/exec/cputlb.h |  3 +
>>  include/exec/exec-all.h   |  3 +
>>  include/exec/gen-icount.h |  2 +
>>  include/exec/helper-gen.h |  2 +
>>  include/exec/helper-proto.h   |  2 +
>>  include/exec/helper-tcg.h |  2 +
>>  include/exec/ioport.h |  2 +
>>  include/exec/memory-internal.h|  2 +
>>  include/exec/memory_ldst.inc.h|  2 +
>>  include/exec/memory_ldst_cached.inc.h |  2 +
>>  include/exec/memory_ldst_phys.inc.h   |  2 +
>
> The pattern used should not match all *.h, but exclude *_template.h (older
> naming style) and *.inc.h (newer naming style; we really should finish the
> conversion).
>
> These headers are not standalone, and will be included multiple times by one 
> of
> the other headers.

Yes.

I excluded them the stupid way in this RFC, in part because I was unsure
about the naming convention for "special .h meant to be included
multiple times".  My cover letter should have mentioned this under
"Known issues".

For my series to shed the RFC tag, I need to eliminate the FIXMEs in
headers that aren't actually broken.  This includes the FIXMEs marking
"special .h meant to be included multiple times".

Perhaps I can finish the conversion to the .inc.h convention in v6.

Re: [Qemu-devel] [PULL 0/2] ppc-for-4.1 queue 20190712

[Peter Maydell]

On Fri, 12 Jul 2019 at 07:25, David Gibson  wrote:
>
> The following changes since commit 9411db8f37c64b9adb3e4b393c623a5760bcb847:
>
>   Merge remote-tracking branch 'remotes/jnsnow/tags/bitmaps-pull-request' 
> into staging (2019-07-11 11:58:14 +0100)
>
> are available in the Git repository at:
>
>   git://github.com/dgibson/qemu.git tags/ppc-for-4.1-20190712
>
> for you to fetch changes up to 38298611d5a87d2739d0a21d5f9e47ba43570c22:
>
>   xics/kvm: Always set the MASKED bit if interrupt is masked (2019-07-12 
> 15:50:00 +1000)
>
> 
> ppc patch queue for 2019-07-12
>
> First 4.1 hard freeze pull request.  Not much here, just a bug fix for
> the XICS interrupt controller and a SLOF firmware update to fix a bug
> with IP discovery when there are multiple NICs.
>
> 
> Alexey Kardashevskiy (1):
>   pseries: Update SLOF firmware image
>
> Greg Kurz (1):
>   xics/kvm: Always set the MASKED bit if interrupt is masked


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.1
for any user-visible changes.

-- PMM

Re: [Qemu-devel] [RFC 2/5] block: Generic truncation fallback

[Kevin Wolf]

Am 12.07.2019 um 12:58 hat Max Reitz geschrieben:
> On 12.07.19 11:49, Kevin Wolf wrote:
> > Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
> >> If a protocol driver does not support truncation, we call fall back to
> >> effectively not doing anything if the new size is less than the actual
> >> file size.  This is what we have been doing for some host device drivers
> >> already.
> > 
> > Specifically, we're doing it for drivers that access a fixed-size image,
> > i.e. block devices rather than regular files. We don't want to do this
> > for drivers where the file size could be changed, but just didn't
> > implement it.
> > 
> > So I would suggest calling the function more specifically something like
> > bdrv_co_truncate_blockdev(), and not using it as an automatic fallback
> > in bdrv_co_truncate(), but just make it the BlockDriver.bdrv_co_truncate
> > implementation for those drivers where it makes sense.
> 
> I was thinking about this, but the problem is that .bdrv_co_truncate()
> does not get a BdrvChild, so an implementation for it cannot generically
> zero the first sector (without bypassing the permission system, which
> would be wrong).

Hm, I see. The next best thing would then probably having a bool in
BlockDriver that enables the fallback.

> So the function pointer would actually need to be set to something like
> (int (*)(BlockDriverState *, int64_t, PreallocMode, Error **))42ul, or a
> dummy function that just aborts, and then bdrv_co_truncate() would
> recognize this magic constant.  But that seemed so weird to me that I
> decided just not to do it, mostly because I was wondering what would be
> so bad about treating images whose size we cannot change because we
> haven’t implemented it exactly like fixed-size images.
> 
> (Also, “fixed-size” is up to interpretation.  You can change an LVM
> volume’s size.  qemu doesn’t do it, obviously.  But that is the reason
> for the warning qemu-img resize emits when it sees that the file size
> did not change.)
> 
> > And of course, we only need these fake implementations because qemu-img
> > (or .bdrv_co_create_opts) always wants to create the protocol level. If
> > we could avoid this, then we wouldn't need any of this.
> 
> It’s trivial to avoid this.  I mean, patch 4 does exactly that.
> 
> So it isn’t about avoiding creating the protocol level, it’s about
> avoiding the truncation there.  But why would you do that?

Because we can't actually truncate there. We can only do the fake thing
and claim we have truncated even though the size didn't change.

But actually, while avoiding the fake truncate outside of image creation
would avoid confusing situations where the user requested image
shrinking, gets success, but nothing happened, it would also break
actual resizes where the volume is resized externally and block_resize
is only called to notify qemu to pick up the size change.

So after all, we probably do need to keep the hack in bdrv_co_truncate()
instead of moving it to image creation only.

Kevin


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH for 4.1? v1 0/7] testing/next (docker, win-cross)

[Alex Bennée]

Hi,

This is my current queue for testing/next. I think they are all worth
it for 4.1 but it would be nice to get the Windows cross builds up and
running again.

The following patches need review
 patch 0001/tests docker add test misc for building tools doc.patch
 patch 0005/tests migration test don t spam the logs when we .patch
 patch 0006/tests dockerfiles update the win cross builds to .patch
 patch 0007/shippable re enable the windows cross builds.patch

Alex Bennée (4):
  tests/docker: add test-misc for building tools & docs
  tests/migration-test: don't spam the logs when we fail
  tests/dockerfiles: update the win cross builds to stretch
  shippable: re-enable the windows cross builds

Philippe Mathieu-Daudé (3):
  tests/docker: Install Sphinx in the Ubuntu images
  tests/docker: Install Sphinx in the Fedora image
  tests/docker: Install Ubuntu images noninteractively

 .shippable.yml|  9 
 tests/docker/Makefile.include |  6 ++---
 .../dockerfiles/debian-win32-cross.docker |  4 ++--
 .../dockerfiles/debian-win64-cross.docker |  4 ++--
 ...{debian8-mxe.docker => debian9-mxe.docker} | 11 ++
 tests/docker/dockerfiles/fedora.docker|  1 +
 tests/docker/dockerfiles/ubuntu.docker|  3 ++-
 tests/docker/dockerfiles/ubuntu1804.docker|  3 ++-
 tests/docker/test-misc| 22 +++
 tests/migration-test.c| 19 ++--
 10 files changed, 57 insertions(+), 25 deletions(-)
 rename tests/docker/dockerfiles/{debian8-mxe.docker => debian9-mxe.docker} 
(56%)
 create mode 100755 tests/docker/test-misc

-- 
2.20.1

[Qemu-devel] [PATCH v1 4/7] tests/docker: Install Ubuntu images noninteractively

[Alex Bennée]

From: Philippe Mathieu-Daudé 

We correctly use the DEBIAN_FRONTEND environment variable on
the Debian images, but forgot the Ubuntu ones are based on it.

Since building docker images is not interactive, we need to
inform the APT tools about it using the DEBIAN_FRONTEND
environment variable (we already use it on our Debian images).

This fixes:

  $ make docker-image-ubuntu V=1
  [...]
  Setting up tzdata (2019b-0ubuntu0.19.04) ...
  debconf: unable to initialize frontend: Dialog
  debconf: (TERM is not set, so the dialog frontend is not usable.)
  debconf: falling back to frontend: Readline
  Configuring tzdata
  --

  Please select the geographic area in which you live. Subsequent configuration
  questions will narrow this down by presenting a list of cities, representing
  the time zones in which they are located.

1. Africa  4. Australia  7. Atlantic  10. Pacific  13. Etc
2. America 5. Arctic 8. Europe11. SystemV
3. Antarctica  6. Asia   9. Indian12. US
  Geographic area: 12
  [HANG]

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
Message-Id: <20190711124805.26476-1-phi...@redhat.com>
Signed-off-by: Alex Bennée 
---
 tests/docker/dockerfiles/ubuntu.docker | 2 +-
 tests/docker/dockerfiles/ubuntu1804.docker | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/docker/dockerfiles/ubuntu.docker 
b/tests/docker/dockerfiles/ubuntu.docker
index 2500ec84b6f..a4f601395c8 100644
--- a/tests/docker/dockerfiles/ubuntu.docker
+++ b/tests/docker/dockerfiles/ubuntu.docker
@@ -66,6 +66,6 @@ ENV PACKAGES flex bison \
 texinfo \
 xfslibs-dev
 RUN apt-get update && \
-apt-get -y install $PACKAGES
+DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
 RUN dpkg -l $PACKAGES | sort > /packages.txt
 ENV FEATURES clang pyyaml sdl2
diff --git a/tests/docker/dockerfiles/ubuntu1804.docker 
b/tests/docker/dockerfiles/ubuntu1804.docker
index 0bb8088658d..44bbf0f77ae 100644
--- a/tests/docker/dockerfiles/ubuntu1804.docker
+++ b/tests/docker/dockerfiles/ubuntu1804.docker
@@ -53,6 +53,6 @@ ENV PACKAGES flex bison \
 texinfo \
 xfslibs-dev
 RUN apt-get update && \
-apt-get -y install $PACKAGES
+DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
 RUN dpkg -l $PACKAGES | sort > /packages.txt
 ENV FEATURES clang pyyaml sdl2
-- 
2.20.1

[Qemu-devel] [PATCH v1 3/7] tests/docker: Install Sphinx in the Fedora image

[Alex Bennée]

From: Philippe Mathieu-Daudé 

Since commit 5f71eac06e the Sphinx tool is required
to build the rST documentation.

This fixes:

 $ ./configure --enable-docs

 ERROR: User requested feature docs
configure was not able to find it.
Install texinfo, Perl/perl-podlators and python-sphinx

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
Message-Id: <20190711102710.2263-1-phi...@redhat.com>
Signed-off-by: Alex Bennée 
---
 tests/docker/dockerfiles/fedora.docker | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/docker/dockerfiles/fedora.docker 
b/tests/docker/dockerfiles/fedora.docker
index 619d1b5656d..e6d39e14cb1 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -76,6 +76,7 @@ ENV PACKAGES \
 perl-Test-Harness \
 pixman-devel \
 python3 \
+python3-sphinx \
 PyYAML \
 rdma-core-devel \
 SDL2-devel \
-- 
2.20.1

[Qemu-devel] [PATCH v1 1/7] tests/docker: add test-misc for building tools & docs

[Alex Bennée]

Add yet another test type so we cna quickly exercise the miscellaneous
build products of the build system under various docer configurations.

Signed-off-by: Alex Bennée 
---
 tests/docker/test-misc | 22 ++
 1 file changed, 22 insertions(+)
 create mode 100755 tests/docker/test-misc

diff --git a/tests/docker/test-misc b/tests/docker/test-misc
new file mode 100755
index 000..d480afedca7
--- /dev/null
+++ b/tests/docker/test-misc
@@ -0,0 +1,22 @@
+#!/bin/bash -e
+#
+# Build the miscellaneous components
+#
+# Copyright (c) 2019 Linaro Ltd.
+#
+# Authors:
+#  Alex Bennée 
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+. common.rc
+
+cd "$BUILD_DIR"
+
+# build everything else but QEMU
+configure_qemu --disable-user --disable-system --enable-docs --enable-tools
+build_qemu
+install_qemu
-- 
2.20.1

[Qemu-devel] [PATCH v1 2/7] tests/docker: Install Sphinx in the Ubuntu images

[Alex Bennée]

From: Philippe Mathieu-Daudé 

Since commit 5f71eac06e the Sphinx tool is required
to build the rST documentation.

This fixes:

 $ ./configure --enable-docs

 ERROR: User requested feature docs
configure was not able to find it.
Install texinfo, Perl/perl-podlators and python-sphinx

Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20190711120609.12773-1-phi...@redhat.com>
Signed-off-by: Alex Bennée 
---
 tests/docker/dockerfiles/ubuntu.docker | 1 +
 tests/docker/dockerfiles/ubuntu1804.docker | 1 +
 2 files changed, 2 insertions(+)

diff --git a/tests/docker/dockerfiles/ubuntu.docker 
b/tests/docker/dockerfiles/ubuntu.docker
index d3b72209c85..2500ec84b6f 100644
--- a/tests/docker/dockerfiles/ubuntu.docker
+++ b/tests/docker/dockerfiles/ubuntu.docker
@@ -61,6 +61,7 @@ ENV PACKAGES flex bison \
 libxen-dev \
 make \
 python-yaml \
+python3-sphinx \
 sparse \
 texinfo \
 xfslibs-dev
diff --git a/tests/docker/dockerfiles/ubuntu1804.docker 
b/tests/docker/dockerfiles/ubuntu1804.docker
index 9d80b115009..0bb8088658d 100644
--- a/tests/docker/dockerfiles/ubuntu1804.docker
+++ b/tests/docker/dockerfiles/ubuntu1804.docker
@@ -48,6 +48,7 @@ ENV PACKAGES flex bison \
 libxen-dev \
 make \
 python-yaml \
+python3-sphinx \
 sparse \
 texinfo \
 xfslibs-dev
-- 
2.20.1

[Qemu-devel] [PATCH v1 5/7] tests/migration-test: don't spam the logs when we fail

[Alex Bennée]

Quite often the information about which test failed is hidden by the
wall of repeated failures for each page. Stop outputting the error
after 10 bad pages and just summarise the total damage at the end.

Signed-off-by: Alex Bennée 
---
 tests/migration-test.c | 19 ---
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index b6434628e1c..ce041f80c2a 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -308,7 +308,7 @@ static void check_guests_ram(QTestState *who)
 uint8_t first_byte;
 uint8_t last_byte;
 bool hit_edge = false;
-bool bad = false;
+int bad = 0;
 
 qtest_memread(who, start_address, &first_byte, 1);
 last_byte = first_byte;
@@ -327,15 +327,20 @@ static void check_guests_ram(QTestState *who)
 hit_edge = true;
 last_byte = b;
 } else {
-fprintf(stderr, "Memory content inconsistency at %x"
-" first_byte = %x last_byte = %x current = %x"
-" hit_edge = %x\n",
-address, first_byte, last_byte, b, hit_edge);
-bad = true;
+bad++;
+if (bad <= 10) {
+fprintf(stderr, "Memory content inconsistency at %x"
+" first_byte = %x last_byte = %x current = %x"
+" hit_edge = %x\n",
+address, first_byte, last_byte, b, hit_edge);
+}
 }
 }
 }
-g_assert_false(bad);
+if (bad >= 10) {
+fprintf(stderr, "and in another %d pages", bad);
+}
+g_assert(bad == 0);
 }
 
 static void cleanup(const char *filename)
-- 
2.20.1

[Qemu-devel] [PATCH v1 7/7] shippable: re-enable the windows cross builds

[Alex Bennée]

The pkg.mxe.cc repo has been restored.

Signed-off-by: Alex Bennée 
---
 .shippable.yml | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/.shippable.yml b/.shippable.yml
index f2ffef21d11..f74a3de3ffd 100644
--- a/.shippable.yml
+++ b/.shippable.yml
@@ -7,11 +7,10 @@ env:
   matrix:
 - IMAGE=debian-amd64
   TARGET_LIST=x86_64-softmmu,x86_64-linux-user
-# currently disabled as the mxe.cc repos are down
-# - IMAGE=debian-win32-cross
-#   TARGET_LIST=arm-softmmu,i386-softmmu,lm32-softmmu
-# - IMAGE=debian-win64-cross
-#   TARGET_LIST=aarch64-softmmu,sparc64-softmmu,x86_64-softmmu
+- IMAGE=debian-win32-cross
+  TARGET_LIST=arm-softmmu,i386-softmmu,lm32-softmmu
+- IMAGE=debian-win64-cross
+  TARGET_LIST=aarch64-softmmu,sparc64-softmmu,x86_64-softmmu
 - IMAGE=debian-armel-cross
   TARGET_LIST=arm-softmmu,arm-linux-user,armeb-linux-user
 - IMAGE=debian-armhf-cross
-- 
2.20.1

[Qemu-devel] [PATCH v1 6/7] tests/dockerfiles: update the win cross builds to stretch

[Alex Bennée]

While fixing up pkg.mxe.cc they move the URLs around a bit and dropped
Jessie support in favour of Stretch. We also need to update the keys
used to verify the packages.

Signed-off-by: Alex Bennée 
---
 tests/docker/Makefile.include |  6 +++---
 tests/docker/dockerfiles/debian-win32-cross.docker|  4 ++--
 tests/docker/dockerfiles/debian-win64-cross.docker|  4 ++--
 .../{debian8-mxe.docker => debian9-mxe.docker}| 11 +++
 4 files changed, 14 insertions(+), 11 deletions(-)
 rename tests/docker/dockerfiles/{debian8-mxe.docker => debian9-mxe.docker} 
(56%)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index aaf5396b85d..dbd58e548c1 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -85,7 +85,7 @@ endif
 
 # Enforce dependencies for composite images
 docker-image-debian: docker-image-debian9
-docker-image-debian8-mxe: docker-image-debian8
+docker-image-debian9-mxe: docker-image-debian9
 docker-image-debian-amd64: docker-image-debian9
 docker-image-debian-armel-cross: docker-image-debian9
 docker-image-debian-armhf-cross: docker-image-debian9
@@ -96,8 +96,8 @@ docker-image-debian-mipsel-cross: docker-image-debian9
 docker-image-debian-mips64el-cross: docker-image-debian9
 docker-image-debian-ppc64el-cross: docker-image-debian9
 docker-image-debian-s390x-cross: docker-image-debian9
-docker-image-debian-win32-cross: docker-image-debian8-mxe
-docker-image-debian-win64-cross: docker-image-debian8-mxe
+docker-image-debian-win32-cross: docker-image-debian9-mxe
+docker-image-debian-win64-cross: docker-image-debian9-mxe
 
 docker-image-debian-alpha-cross: docker-image-debian-sid
 docker-image-debian-hppa-cross: docker-image-debian-sid
diff --git a/tests/docker/dockerfiles/debian-win32-cross.docker 
b/tests/docker/dockerfiles/debian-win32-cross.docker
index 0a4970c0683..c787e432454 100644
--- a/tests/docker/dockerfiles/debian-win32-cross.docker
+++ b/tests/docker/dockerfiles/debian-win32-cross.docker
@@ -1,9 +1,9 @@
 #
 # Docker mingw32 cross-compiler target
 #
-# This docker target builds on the debian Jessie MXE base image.
+# This docker target builds on the debian Stretch MXE base image.
 #
-FROM qemu:debian8-mxe
+FROM qemu:debian9-mxe
 
 MAINTAINER Philippe Mathieu-Daudé 
 
diff --git a/tests/docker/dockerfiles/debian-win64-cross.docker 
b/tests/docker/dockerfiles/debian-win64-cross.docker
index b27985b1b1f..a7068ed6ac6 100644
--- a/tests/docker/dockerfiles/debian-win64-cross.docker
+++ b/tests/docker/dockerfiles/debian-win64-cross.docker
@@ -1,9 +1,9 @@
 #
 # Docker mingw64 cross-compiler target
 #
-# This docker target builds on the debian Jessie MXE base image.
+# This docker target builds on the debian Stretch MXE base image.
 #
-FROM qemu:debian8-mxe
+FROM qemu:debian9-mxe
 
 MAINTAINER Philippe Mathieu-Daudé 
 
diff --git a/tests/docker/dockerfiles/debian8-mxe.docker 
b/tests/docker/dockerfiles/debian9-mxe.docker
similarity index 56%
rename from tests/docker/dockerfiles/debian8-mxe.docker
rename to tests/docker/dockerfiles/debian9-mxe.docker
index 2df4cc8c5c9..5bc8a6d5c36 100644
--- a/tests/docker/dockerfiles/debian8-mxe.docker
+++ b/tests/docker/dockerfiles/debian9-mxe.docker
@@ -1,15 +1,18 @@
 #
 # Docker mingw cross-compiler target
 #
-# This docker target builds on the debian Jessie base image.
+# This docker target builds on the debian Stretch base image.
 #
-FROM qemu:debian8
+FROM qemu:debian9
 
 MAINTAINER Philippe Mathieu-Daudé 
 
+RUN DEBIAN_FRONTEND=noninteractive eatmydata \
+apt install -y --no-install-recommends gnupg dirmngr
+
 # Add the foreign architecture we want and install dependencies
-RUN apt-key adv --keyserver keyserver.ubuntu.com --recv-keys 
D43A795B73B16ABE9643FE1AFD8FFF16DB45C6AB && \
-echo "deb http://pkg.mxe.cc/repos/apt/debian jessie main" > 
/etc/apt/sources.list.d/mxeapt.list
+RUN apt-key adv --keyserver keyserver.ubuntu.com --recv-keys C6BF758A33A3A276 
&& \
+echo "deb http://pkg.mxe.cc/repos/apt stretch main" > 
/etc/apt/sources.list.d/mxeapt.list
 RUN apt-get update
 RUN DEBIAN_FRONTEND=noninteractive eatmydata \
 apt-get install -y --no-install-recommends \
-- 
2.20.1

Re: [Qemu-devel] [PATCH v1 5/7] tests/migration-test: don't spam the logs when we fail

[Laurent Vivier]

On 12/07/2019 13:18, Alex Bennée wrote:
> Quite often the information about which test failed is hidden by the
> wall of repeated failures for each page. Stop outputting the error
> after 10 bad pages and just summarise the total damage at the end.
> 
> Signed-off-by: Alex Bennée 
> ---
>  tests/migration-test.c | 19 ---
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index b6434628e1c..ce041f80c2a 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -308,7 +308,7 @@ static void check_guests_ram(QTestState *who)
>  uint8_t first_byte;
>  uint8_t last_byte;
>  bool hit_edge = false;
> -bool bad = false;
> +int bad = 0;
>  
>  qtest_memread(who, start_address, &first_byte, 1);
>  last_byte = first_byte;
> @@ -327,15 +327,20 @@ static void check_guests_ram(QTestState *who)
>  hit_edge = true;
>  last_byte = b;
>  } else {
> -fprintf(stderr, "Memory content inconsistency at %x"
> -" first_byte = %x last_byte = %x current = 
> %x"
> -" hit_edge = %x\n",
> -address, first_byte, last_byte, b, hit_edge);
> -bad = true;
> +bad++;
> +if (bad <= 10) {
> +fprintf(stderr, "Memory content inconsistency at %x"
> +" first_byte = %x last_byte = %x current = %x"
> +" hit_edge = %x\n",
> +address, first_byte, last_byte, b, hit_edge);
> +}
>  }
>  }
>  }
> -g_assert_false(bad);
> +if (bad >= 10) {
> +fprintf(stderr, "and in another %d pages", bad);

"bad - 10" as you have already displayed 10 errors.

Thanks,
Laurent

Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close()

[Kevin Wolf]

Am 12.07.2019 um 13:09 hat Max Reitz geschrieben:
> On 12.07.19 13:01, Kevin Wolf wrote:
> > Am 12.07.2019 um 12:47 hat Max Reitz geschrieben:
> >> On 12.07.19 11:24, Kevin Wolf wrote:
> >>> Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
>  When nbd_close() is called from a coroutine, the connection_co never
>  gets to run, and thus nbd_teardown_connection() hangs.
> 
>  This is because aio_co_enter() only puts the connection_co into the main
>  coroutine's wake-up queue, so this main coroutine needs to yield and
>  reschedule itself to let the connection_co run.
> 
>  Signed-off-by: Max Reitz 
>  ---
>   block/nbd.c | 12 +++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
>  diff --git a/block/nbd.c b/block/nbd.c
>  index 81edabbf35..b83b6cd43e 100644
>  --- a/block/nbd.c
>  +++ b/block/nbd.c
>  @@ -135,7 +135,17 @@ static void 
>  nbd_teardown_connection(BlockDriverState *bs)
>   qio_channel_shutdown(s->ioc,
>    QIO_CHANNEL_SHUTDOWN_BOTH,
>    NULL);
>  -BDRV_POLL_WHILE(bs, s->connection_co);
>  +
>  +if (qemu_in_coroutine()) {
>  +/* Let our caller poll and just yield until connection_co is 
>  done */
>  +while (s->connection_co) {
>  +aio_co_schedule(qemu_get_current_aio_context(),
>  +qemu_coroutine_self());
>  +qemu_coroutine_yield();
>  +}
> >>>
> >>> Isn't this busy waiting? Why not let s->connection_co wake us up when
> >>> it's about to terminate instead of immediately rescheduling ourselves?
> >>
> >> Yes, it is busy waiting, but I didn’t find that bad.  The connection_co
> >> will be invoked in basically every iteration, and once there is no
> >> pending data, it will quit.
> >>
> >> The answer to “why not...” of course is because it’d be more complicated.
> >>
> >> But anyway.
> >>
> >> Adding a new function qemu_coroutine_run_after(target) that adds
> >> qemu_coroutine_self() to the given @target coroutine’s wake-up queue and
> >> then using that instead of scheduling works, too, yes.
> >>
> >> I don’t really like being responsible for coroutine code, though...
> >>
> >> (And maybe it’d be better to make it qemu_coroutine_yield_for(target),
> >> which does the above and then yields?)
> > 
> > Or just do something like this, which is arguably not only a fix for the
> > busy wait, but also a code simplification:
> 
> 1. Is that guaranteed to work?  What if data sneaks in, the
> connection_co handles that, and doesn’t wake up the teardown_co?  Will
> it be re-scheduled?

Then connection_co is buggy because we clearly requested that it
terminate. It is possible that it does so only after handling another
request, but this wouldn't be a problem. teardown_co would then just
sleep for a few cycles more until connection_co is done and reaches the
aio_co_wake() call.

> 2. I precisely didn’t want to do this because we have this functionality
> already in the form of Coroutine.co_queue_wakeup.  Why duplicate it here?

co_queue_wakeup contains coroutines to be run at the next yield point
(or termination), which may be when connection_co is actually done, but
it might also be earlier. My explicit aio_co_wake() at the end of
connection_co is guaranteed to run only when connection_co is done.

Kevin

> > diff --git a/block/nbd.c b/block/nbd.c
> > index b83b6cd43e..c061bd1bfc 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -61,6 +61,7 @@ typedef struct BDRVNBDState {
> >  CoMutex send_mutex;
> >  CoQueue free_sema;
> >  Coroutine *connection_co;
> > +Coroutine *teardown_co;
> >  int in_flight;
> > 
> >  NBDClientRequest requests[MAX_NBD_REQUESTS];
> > @@ -137,12 +138,9 @@ static void nbd_teardown_connection(BlockDriverState 
> > *bs)
> >   NULL);
> > 
> >  if (qemu_in_coroutine()) {
> > -/* Let our caller poll and just yield until connection_co is done 
> > */
> > -while (s->connection_co) {
> > -aio_co_schedule(qemu_get_current_aio_context(),
> > -qemu_coroutine_self());
> > -qemu_coroutine_yield();
> > -}
> > +/* just yield until connection_co is done */
> > +s->teardown_co = qemu_coroutine_self();
> > +qemu_coroutine_yield();
> >  } else {
> >  BDRV_POLL_WHILE(bs, s->connection_co);
> >  }
> > @@ -217,6 +215,9 @@ static coroutine_fn void nbd_connection_entry(void 
> > *opaque)
> >  bdrv_dec_in_flight(s->bs);
> > 
> >  s->connection_co = NULL;
> > +if (s->teardown_co) {
> > +aio_co_wake(s->teardown_co);
> > +}
> >  aio_wait_kick();
> >  }
> > 
> 
> 





signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v1 5/7] tests/migration-test: don't spam the logs when we fail

[Thomas Huth]

On 12/07/2019 13.18, Alex Bennée wrote:
> Quite often the information about which test failed is hidden by the
> wall of repeated failures for each page. Stop outputting the error
> after 10 bad pages and just summarise the total damage at the end.
> 
> Signed-off-by: Alex Bennée 
> ---
>  tests/migration-test.c | 19 ---
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index b6434628e1c..ce041f80c2a 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -308,7 +308,7 @@ static void check_guests_ram(QTestState *who)
>  uint8_t first_byte;
>  uint8_t last_byte;
>  bool hit_edge = false;
> -bool bad = false;
> +int bad = 0;
>  
>  qtest_memread(who, start_address, &first_byte, 1);
>  last_byte = first_byte;
> @@ -327,15 +327,20 @@ static void check_guests_ram(QTestState *who)
>  hit_edge = true;
>  last_byte = b;
>  } else {
> -fprintf(stderr, "Memory content inconsistency at %x"
> -" first_byte = %x last_byte = %x current = 
> %x"
> -" hit_edge = %x\n",
> -address, first_byte, last_byte, b, hit_edge);
> -bad = true;
> +bad++;
> +if (bad <= 10) {
> +fprintf(stderr, "Memory content inconsistency at %x"
> +" first_byte = %x last_byte = %x current = %x"
> +" hit_edge = %x\n",
> +address, first_byte, last_byte, b, hit_edge);
> +}
>  }
>  }
>  }
> -g_assert_false(bad);
> +if (bad >= 10) {
> +fprintf(stderr, "and in another %d pages", bad);
> +}
> +g_assert(bad == 0);
>  }
>  
>  static void cleanup(const char *filename)

Good idea.

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [PATCH v2 12/13] migration: add support to migrate page encryption bitmap

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.si...@amd.com) wrote:
> When memory encryption is enabled, the hypervisor maintains a page
> encryption bitmap which is referred by hypervisor during migratoin to check
> if page is private or shared. The bitmap is built during the VM bootup and
> must be migrated to the target host so that hypervisor on target host can
> use it for future migration. The KVM_{SET,GET}_PAGE_ENC_BITMAP can be used
> to get and set the bitmap for a given gfn range.
> 
> Signed-off-by: Brijesh Singh 
> ---
>  accel/kvm/kvm-all.c  |  4 +++
>  migration/ram.c  | 11 +++
>  target/i386/sev.c| 67 
>  target/i386/trace-events |  2 ++
>  4 files changed, 84 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 442b1af36e..9e23088a94 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1831,6 +1831,10 @@ static int kvm_init(MachineState *ms)
>  kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
>  kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
>  kvm_state->memcrypt_load_incoming_page = sev_load_incoming_page;
> +kvm_state->memcrypt_load_incoming_page_enc_bitmap =
> +sev_load_incoming_page_enc_bitmap;
> +kvm_state->memcrypt_save_outgoing_page_enc_bitmap =
> +sev_save_outgoing_page_enc_bitmap;
>  }
>  
>  ret = kvm_arch_init(ms, s);
> diff --git a/migration/ram.c b/migration/ram.c
> index d179867e1b..3a4bdf3c03 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -78,6 +78,7 @@
>  /* 0x80 is reserved in migration.h start with 0x100 next */
>  #define RAM_SAVE_FLAG_COMPRESS_PAGE0x100
>  #define RAM_SAVE_FLAG_ENCRYPTED_PAGE   0x200
> +#define RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP   0x400 /* used in 
> target/i386/sev.c */

OK, you see now we're toast!  We can't use that bit.

I see two ways around this:

  a) Define a flag to mean 'more flags'  - we can reuse the old
RAM_SAVE_FLAG_FULL to mean more-flags, and then send a second 64bit word
that actually contains the flags

  b) Do something clever where RAM_SAVE_FLAG_ENCRYPTED_PAGE | something
is your bitmap.  But then you need to be careful in any confusion during
the decoding.

>  static inline bool is_zero_range(uint8_t *p, uint64_t size)
>  {
> @@ -3595,6 +3596,10 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>  flush_compressed_data(rs);
>  ram_control_after_iterate(f, RAM_CONTROL_FINISH);
>  
> +if (kvm_memcrypt_enabled()) {
> +ret = kvm_memcrypt_save_outgoing_page_enc_bitmap(f);
> +}
> +
>  rcu_read_unlock();
>  
>  multifd_send_sync_main();
> @@ -4469,6 +4474,12 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>  ret = -EINVAL;
>  }
>  break;
> +case RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP:
> +if (kvm_memcrypt_load_incoming_page_enc_bitmap(f)) {
> +error_report("Failed to load page enc bitmap");
> +ret = -EINVAL;
> +}
> +break;
>  case RAM_SAVE_FLAG_EOS:
>  /* normal exit */
>  multifd_recv_sync_main();
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 09a62d6f88..93c6a90806 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -63,6 +63,7 @@ static const char *const sev_fw_errlist[] = {
>  };
>  
>  #define SEV_FW_MAX_ERROR  ARRAY_SIZE(sev_fw_errlist)
> +#define RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP   0x400
>  
>  static int
>  sev_ioctl(int fd, int cmd, void *data, int *error)
> @@ -1189,6 +1190,72 @@ int sev_load_incoming_page(void *handle, QEMUFile *f, 
> uint8_t *ptr)
>  return sev_receive_update_data(f, ptr);
>  }
>  
> +#define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
> +
> +int sev_load_incoming_page_enc_bitmap(void *handle, QEMUFile *f)
> +{
> +void *bmap;
> +unsigned long npages;
> +unsigned long bmap_size, base_gpa;
> +struct kvm_page_enc_bitmap e = {};
> +
> +base_gpa = qemu_get_be64(f);
> +npages = qemu_get_be64(f);
> +bmap_size = qemu_get_be64(f);
> +
> +bmap = g_malloc0(bmap_size);
> +qemu_get_buffer(f, (uint8_t *)bmap, bmap_size);

You should validate npages against bmap_size and validate bmap_size
for being huge if possible (although huge VMs are legal).
You could also sanity check base_gpa for alignment.

Treat data coming off the wire as hostile.

> +if (kvm_vm_ioctl(kvm_state, KVM_SET_PAGE_ENC_BITMAP, &e) == -1) {
> +
> +trace_kvm_sev_load_page_enc_bitmap(base_gpa, npages << TARGET_PAGE_BITS);
> +
> +e.start_gfn = base_gpa >> TARGET_PAGE_BITS;
> +e.num_pages = npages;
> +e.enc_bitmap = bmap;

> +error_report("KVM_SET_PAGE_ENC_BITMAP ioctl failed %d", errno);
> +g_free(bmap);
> +return 1;
> +}
> +
> +g_free(bmap);
> +
> +return 0;
> +}
> +
> +int sev_save_outgoing_page_enc_bitmap(void *handle, QEMUFil

Re: [Qemu-devel] [PATCH v2 13/13] target/i386: sev: remove migration blocker

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.si...@amd.com) wrote:
> Signed-off-by: Brijesh Singh 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  target/i386/sev.c | 12 
>  1 file changed, 12 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 93c6a90806..48336515a2 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -34,7 +34,6 @@
>  #define DEFAULT_SEV_DEVICE  "/dev/sev"
>  
>  static SEVState *sev_state;
> -static Error *sev_mig_blocker;
>  
>  static const char *const sev_fw_errlist[] = {
>  "",
> @@ -686,7 +685,6 @@ static void
>  sev_launch_finish(SEVState *s)
>  {
>  int ret, error;
> -Error *local_err = NULL;
>  
>  trace_kvm_sev_launch_finish();
>  ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_LAUNCH_FINISH, 0, &error);
> @@ -697,16 +695,6 @@ sev_launch_finish(SEVState *s)
>  }
>  
>  sev_set_guest_state(SEV_STATE_RUNNING);
> -
> -/* add migration blocker */
> -error_setg(&sev_mig_blocker,
> -   "SEV: Migration is not implemented");
> -ret = migrate_add_blocker(sev_mig_blocker, &local_err);
> -if (local_err) {
> -error_report_err(local_err);
> -error_free(sev_mig_blocker);
> -exit(1);
> -}
>  }
>  
>  static int
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [PATCH for 4.1 0/4] target/mips: Fixes for 4.1

[Aleksandar Markovic]

From: Aleksandar Markovic 

At the moment, this includes fixes for problems in switch statements
found by GCC 8.3 improved code analysis features.

Aleksandar Markovic (4):
  target/mips: Add 'fall through' comments for handling nanoMips' SHXS,
SWXS
  target/mips: Add missing 'break' for a case of MTHC0 handling
  target/mips: Add missing 'break' for certain cases of MFTR handling
  target/mips: Add missing 'break' for certain cases of MTTR handling

 target/mips/translate.c | 7 +++
 1 file changed, 7 insertions(+)

-- 
2.7.4

[Qemu-devel] [PATCH for 4.1 2/4] target/mips: Add missing 'break' for a case of MTHC0 handling

[Aleksandar Markovic]

From: Aleksandar Markovic 

This was found by GCC 8.3 static analysis.

Signed-off-by: Aleksandar Markovic 
---
 target/mips/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 2be5e2d..59d4acd 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -6745,6 +6745,7 @@ static void gen_mthc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
 default:
 goto cp0_unimplemented;
 }
+break;
 case CP0_REGISTER_17:
 switch (sel) {
 case 0:
-- 
2.7.4

[Qemu-devel] [PATCH for 4.1 3/4] target/mips: Add missing 'break' for certain cases of MFTR handling

[Aleksandar Markovic]

From: Aleksandar Markovic 

This was found by GCC 8.3 static analysis.

Signed-off-by: Aleksandar Markovic 
---
 target/mips/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 59d4acd..b1cf5f0 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -9826,6 +9826,7 @@ static void gen_mftr(CPUMIPSState *env, DisasContext 
*ctx, int rt, int rd,
 gen_mfc0(ctx, t0, rt, sel);
 break;
 }
+break;
 case 12:
 switch (sel) {
 case 0:
@@ -9835,6 +9836,7 @@ static void gen_mftr(CPUMIPSState *env, DisasContext 
*ctx, int rt, int rd,
 gen_mfc0(ctx, t0, rt, sel);
 break;
 }
+break;
 case 13:
 switch (sel) {
 case 0:
-- 
2.7.4

[Qemu-devel] [PATCH for 4.1 1/4] target/mips: Add 'fall through' comments for handling nanoMips' SHXS, SWXS

[Aleksandar Markovic]

From: Aleksandar Markovic 

This was found by GCC 8.3 static analysis.

Signed-off-by: Aleksandar Markovic 
---
 target/mips/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index f96f141..2be5e2d 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -20136,12 +20136,14 @@ static void gen_p_lsx(DisasContext *ctx, int rd, int 
rs, int rt)
 switch (extract32(ctx->opcode, 7, 4)) {
 case NM_SHXS:
 check_nms(ctx);
+/* fall through */
 case NM_LHXS:
 case NM_LHUXS:
 tcg_gen_shli_tl(t0, t0, 1);
 break;
 case NM_SWXS:
 check_nms(ctx);
+/* fall through */
 case NM_LWXS:
 case NM_LWC1XS:
 case NM_SWC1XS:
-- 
2.7.4

[Qemu-devel] [PATCH for 4.1 v2] linux-user: Fix structure target_ucontext for MIPS

[Aleksandar Markovic]

From: Aleksandar Markovic 

Structure ucontext for MIPS is defined in the following way in
Linux kernel:

(arch/mips/include/uapi/asm/ucontext.h, lines 54-64)

struct ucontext {
/* Historic fields matching asm-generic */
unsigned long   uc_flags;
struct ucontext *uc_link;
stack_t uc_stack;
struct sigcontext   uc_mcontext;
sigset_tuc_sigmask;

/* Extended context structures may follow ucontext */
unsigned long long  uc_extcontext[0];
};

Fix the structure target_ucontext for MIPS to reflect the definition
above, except the correction for field uc_extcontext, which will
follow at some later time.

Fixes: 94c5495d

Reported-by: Dragan Mladjenovic 
Signed-off-by: Aleksandar Markovic 
Reviewed-by: Laurent Vivier 

---

v2: rectified a commit message mistake with
s/target_sigset_t/target_ucontext
---
 linux-user/mips/signal.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/linux-user/mips/signal.c b/linux-user/mips/signal.c
index 6aa303e..455a8a2 100644
--- a/linux-user/mips/signal.c
+++ b/linux-user/mips/signal.c
@@ -71,10 +71,9 @@ struct sigframe {
 };
 
 struct target_ucontext {
-target_ulong tuc_flags;
-target_ulong tuc_link;
+abi_ulong tuc_flags;
+abi_ulong tuc_link;
 target_stack_t tuc_stack;
-target_ulong pad0;
 struct target_sigcontext tuc_mcontext;
 target_sigset_t tuc_sigmask;
 };
-- 
2.7.4

[Qemu-devel] [PATCH for 4.1 4/4] target/mips: Add missing 'break' for certain cases of MTTR handling

[Aleksandar Markovic]

From: Aleksandar Markovic 

This was found by GCC 8.3 static analysis.

Signed-off-by: Aleksandar Markovic 
---
 target/mips/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index b1cf5f0..ca62800 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -10055,6 +10055,7 @@ static void gen_mttr(CPUMIPSState *env, DisasContext 
*ctx, int rd, int rt,
 gen_mtc0(ctx, t0, rd, sel);
 break;
 }
+break;
 case 12:
 switch (sel) {
 case 0:
@@ -10064,6 +10065,7 @@ static void gen_mttr(CPUMIPSState *env, DisasContext 
*ctx, int rd, int rt,
 gen_mtc0(ctx, t0, rd, sel);
 break;
 }
+break;
 case 13:
 switch (sel) {
 case 0:
-- 
2.7.4

Re: [Qemu-devel] [PATCH v1 5/7] tests/migration-test: don't spam the logs when we fail

[Dr. David Alan Gilbert]

* Alex Bennée (alex.ben...@linaro.org) wrote:
> Quite often the information about which test failed is hidden by the
> wall of repeated failures for each page. Stop outputting the error
> after 10 bad pages and just summarise the total damage at the end.
> 
> Signed-off-by: Alex Bennée 

Yep, occasionally you do find you do want the full set to see what's
going on, but this is true most of the time.

With 10 you should be able to see if it's a single page that's wrong or
the flip gone wrong.

> ---
>  tests/migration-test.c | 19 ---
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index b6434628e1c..ce041f80c2a 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -308,7 +308,7 @@ static void check_guests_ram(QTestState *who)
>  uint8_t first_byte;
>  uint8_t last_byte;
>  bool hit_edge = false;
> -bool bad = false;
> +int bad = 0;
>  
>  qtest_memread(who, start_address, &first_byte, 1);
>  last_byte = first_byte;
> @@ -327,15 +327,20 @@ static void check_guests_ram(QTestState *who)
>  hit_edge = true;
>  last_byte = b;
>  } else {
> -fprintf(stderr, "Memory content inconsistency at %x"
> -" first_byte = %x last_byte = %x current = 
> %x"
> -" hit_edge = %x\n",
> -address, first_byte, last_byte, b, hit_edge);
> -bad = true;
> +bad++;
> +if (bad <= 10) {
> +fprintf(stderr, "Memory content inconsistency at %x"
> +" first_byte = %x last_byte = %x current = %x"
> +" hit_edge = %x\n",
> +address, first_byte, last_byte, b, hit_edge);
> +}
>  }
>  }
>  }
> -g_assert_false(bad);
> +if (bad >= 10) {
> +fprintf(stderr, "and in another %d pages", bad);
> +}

as Laurent says, the 'another' would need -10 but other than that:


Reviewed-by: Dr. David Alan Gilbert 

> +g_assert(bad == 0);
>  }
>  
>  static void cleanup(const char *filename)
> -- 
> 2.20.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close()

[Max Reitz]

On 12.07.19 13:23, Kevin Wolf wrote:
> Am 12.07.2019 um 13:09 hat Max Reitz geschrieben:
>> On 12.07.19 13:01, Kevin Wolf wrote:
>>> Am 12.07.2019 um 12:47 hat Max Reitz geschrieben:
 On 12.07.19 11:24, Kevin Wolf wrote:
> Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
>> When nbd_close() is called from a coroutine, the connection_co never
>> gets to run, and thus nbd_teardown_connection() hangs.
>>
>> This is because aio_co_enter() only puts the connection_co into the main
>> coroutine's wake-up queue, so this main coroutine needs to yield and
>> reschedule itself to let the connection_co run.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  block/nbd.c | 12 +++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 81edabbf35..b83b6cd43e 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -135,7 +135,17 @@ static void 
>> nbd_teardown_connection(BlockDriverState *bs)
>>  qio_channel_shutdown(s->ioc,
>>   QIO_CHANNEL_SHUTDOWN_BOTH,
>>   NULL);
>> -BDRV_POLL_WHILE(bs, s->connection_co);
>> +
>> +if (qemu_in_coroutine()) {
>> +/* Let our caller poll and just yield until connection_co is 
>> done */
>> +while (s->connection_co) {
>> +aio_co_schedule(qemu_get_current_aio_context(),
>> +qemu_coroutine_self());
>> +qemu_coroutine_yield();
>> +}
>
> Isn't this busy waiting? Why not let s->connection_co wake us up when
> it's about to terminate instead of immediately rescheduling ourselves?

 Yes, it is busy waiting, but I didn’t find that bad.  The connection_co
 will be invoked in basically every iteration, and once there is no
 pending data, it will quit.

 The answer to “why not...” of course is because it’d be more complicated.

 But anyway.

 Adding a new function qemu_coroutine_run_after(target) that adds
 qemu_coroutine_self() to the given @target coroutine’s wake-up queue and
 then using that instead of scheduling works, too, yes.

 I don’t really like being responsible for coroutine code, though...

 (And maybe it’d be better to make it qemu_coroutine_yield_for(target),
 which does the above and then yields?)
>>>
>>> Or just do something like this, which is arguably not only a fix for the
>>> busy wait, but also a code simplification:
>>
>> 1. Is that guaranteed to work?  What if data sneaks in, the
>> connection_co handles that, and doesn’t wake up the teardown_co?  Will
>> it be re-scheduled?
> 
> Then connection_co is buggy because we clearly requested that it
> terminate.

Did we?  This would be done by setting s->quit to true, which isn’t
explicitly done here.

I thought it worked by just waking up the coroutine until it doesn’t
receive anything anymore, because the connection is closed.  Now I don’t
know whether QIO_CHANNEL_SHUTDOWN_BOTH discards data that has been
received before the channel is closed.  I don’t expect it to.

>It is possible that it does so only after handling another
> request, but this wouldn't be a problem. teardown_co would then just
> sleep for a few cycles more until connection_co is done and reaches the
> aio_co_wake() call.

I don’t quite understand, because the fact how connection_co would
proceed after handling another request was exactly my question.  If it
were to yield and not to wake up, it would never be done.

But I’ve followed nbd_receive_reply() now, and I suppose nbd_read_eof()
will simply never yield after we have invoked qio_channel_shutdown().

>> 2. I precisely didn’t want to do this because we have this functionality
>> already in the form of Coroutine.co_queue_wakeup.  Why duplicate it here?
> 
> co_queue_wakeup contains coroutines to be run at the next yield point
> (or termination), which may be when connection_co is actually done, but
> it might also be earlier.

Yes, if it handles another request, that would be the yield point at the
end of its main loop.

But that would be solved by simply putting the yield_for() in a loop,
like the one that already exists for the non-coroutine case with
BDRV_POLL_WHILE().

>   My explicit aio_co_wake() at the end of
> connection_co is guaranteed to run only when connection_co is done.

I can’t explain it, it feels off to me to add this field to BDRVNBDState
and let connection_co handle it just for this special case.  It seems to
me that if we had a yield_for() function, we could simply use it instead
-- and I’d prefer a generic function over a special-case implementation.

I do understand that “it feels off” is not a reasonable justification
for doing anything.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC 2/5] block: Generic truncation fallback

[Max Reitz]

On 12.07.19 13:17, Kevin Wolf wrote:
> Am 12.07.2019 um 12:58 hat Max Reitz geschrieben:
>> On 12.07.19 11:49, Kevin Wolf wrote:
>>> Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
 If a protocol driver does not support truncation, we call fall back to
 effectively not doing anything if the new size is less than the actual
 file size.  This is what we have been doing for some host device drivers
 already.
>>>
>>> Specifically, we're doing it for drivers that access a fixed-size image,
>>> i.e. block devices rather than regular files. We don't want to do this
>>> for drivers where the file size could be changed, but just didn't
>>> implement it.
>>>
>>> So I would suggest calling the function more specifically something like
>>> bdrv_co_truncate_blockdev(), and not using it as an automatic fallback
>>> in bdrv_co_truncate(), but just make it the BlockDriver.bdrv_co_truncate
>>> implementation for those drivers where it makes sense.
>>
>> I was thinking about this, but the problem is that .bdrv_co_truncate()
>> does not get a BdrvChild, so an implementation for it cannot generically
>> zero the first sector (without bypassing the permission system, which
>> would be wrong).
> 
> Hm, I see. The next best thing would then probably having a bool in
> BlockDriver that enables the fallback.
> 
>> So the function pointer would actually need to be set to something like
>> (int (*)(BlockDriverState *, int64_t, PreallocMode, Error **))42ul, or a
>> dummy function that just aborts, and then bdrv_co_truncate() would
>> recognize this magic constant.  But that seemed so weird to me that I
>> decided just not to do it, mostly because I was wondering what would be
>> so bad about treating images whose size we cannot change because we
>> haven’t implemented it exactly like fixed-size images.
>>
>> (Also, “fixed-size” is up to interpretation.  You can change an LVM
>> volume’s size.  qemu doesn’t do it, obviously.  But that is the reason
>> for the warning qemu-img resize emits when it sees that the file size
>> did not change.)
>>
>>> And of course, we only need these fake implementations because qemu-img
>>> (or .bdrv_co_create_opts) always wants to create the protocol level. If
>>> we could avoid this, then we wouldn't need any of this.
>>
>> It’s trivial to avoid this.  I mean, patch 4 does exactly that.
>>
>> So it isn’t about avoiding creating the protocol level, it’s about
>> avoiding the truncation there.  But why would you do that?
> 
> Because we can't actually truncate there. We can only do the fake thing
> and claim we have truncated even though the size didn't change.

You’re right.  I actually didn’t realize that we have no drivers that
support truncation, but not image creation.

Yes, then it’s stupid.

I thought it was a reasonable thing to do for such drivers.

So I suppose the best thing is to do what I hinted at in my reply to
your reply to patch 3, which is to drop patches 2 and 3 and instead make
the creation fallback work around blk_truncate() failures.

Max

> But actually, while avoiding the fake truncate outside of image creation
> would avoid confusing situations where the user requested image
> shrinking, gets success, but nothing happened, it would also break
> actual resizes where the volume is resized externally and block_resize
> is only called to notify qemu to pick up the size change.
> 
> So after all, we probably do need to keep the hack in bdrv_co_truncate()
> instead of moving it to image creation only.
> 
> Kevin
> 




signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH for-4.1?] hw/arm/fsl-imx6ul.c: Remove dead SMP-related code

[Peter Maydell]

The i.MX6UL always has a single Cortex-A7 CPU (we set FSL_IMX6UL_NUM_CPUS
to 1 in line with this). This means that all the code in fsl-imx6ul.c to
handle multiple CPUs is dead code, and Coverity is now complaining that
it is unreachable (CID 1403008, 1403011).

Remove the unreachable code and the only-executes-once loops,
and replace the single-entry cpu[] array in the FSLIMX6ULState
with a simple cpu member.

Signed-off-by: Peter Maydell 
---
The only real reason to put this into 4.1 is because it fixes
some Coverity issues, and it would be nice to be able to get
down to no Coverity issues for the release. I think that pre-rc1
that's a reasonable reason to put this in.

Disclaimer: tested with "make check" as I have no test image for
this board.

 include/hw/arm/fsl-imx6ul.h |  2 +-
 hw/arm/fsl-imx6ul.c | 62 +++--
 hw/arm/mcimx6ul-evk.c   |  2 +-
 3 files changed, 20 insertions(+), 46 deletions(-)

diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
index 9e94e98f8ee..eda389aec7d 100644
--- a/include/hw/arm/fsl-imx6ul.h
+++ b/include/hw/arm/fsl-imx6ul.h
@@ -61,7 +61,7 @@ typedef struct FslIMX6ULState {
 DeviceStateparent_obj;
 
 /*< public >*/
-ARMCPU cpu[FSL_IMX6UL_NUM_CPUS];
+ARMCPU cpu;
 A15MPPrivState a7mpcore;
 IMXGPTStategpt[FSL_IMX6UL_NUM_GPTS];
 IMXEPITState   epit[FSL_IMX6UL_NUM_EPITS];
diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
index f8601654388..b074177a71d 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -29,16 +29,12 @@
 
 static void fsl_imx6ul_init(Object *obj)
 {
-MachineState *ms = MACHINE(qdev_get_machine());
 FslIMX6ULState *s = FSL_IMX6UL(obj);
 char name[NAME_SIZE];
 int i;
 
-for (i = 0; i < MIN(ms->smp.cpus, FSL_IMX6UL_NUM_CPUS); i++) {
-snprintf(name, NAME_SIZE, "cpu%d", i);
-object_initialize_child(obj, name, &s->cpu[i], sizeof(s->cpu[i]),
-"cortex-a7-" TYPE_ARM_CPU, &error_abort, NULL);
-}
+object_initialize_child(obj, "cpu0", &s->cpu, sizeof(s->cpu),
+"cortex-a7-" TYPE_ARM_CPU, &error_abort, NULL);
 
 /*
  * A7MPCORE
@@ -161,42 +157,25 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error 
**errp)
 MachineState *ms = MACHINE(qdev_get_machine());
 FslIMX6ULState *s = FSL_IMX6UL(dev);
 int i;
-qemu_irq irq;
 char name[NAME_SIZE];
-unsigned int smp_cpus = ms->smp.cpus;
+SysBusDevice *sbd;
+DeviceState *d;
 
-if (smp_cpus > FSL_IMX6UL_NUM_CPUS) {
-error_setg(errp, "%s: Only %d CPUs are supported (%d requested)",
-   TYPE_FSL_IMX6UL, FSL_IMX6UL_NUM_CPUS, smp_cpus);
+if (ms->smp.cpus > 1) {
+error_setg(errp, "%s: Only a single CPU is supported (%d requested)",
+   TYPE_FSL_IMX6UL, ms->smp.cpus);
 return;
 }
 
-for (i = 0; i < smp_cpus; i++) {
-Object *o = OBJECT(&s->cpu[i]);
-
-object_property_set_int(o, QEMU_PSCI_CONDUIT_SMC,
-"psci-conduit", &error_abort);
-
-/* On uniprocessor, the CBAR is set to 0 */
-if (smp_cpus > 1) {
-object_property_set_int(o, FSL_IMX6UL_A7MPCORE_ADDR,
-"reset-cbar", &error_abort);
-}
-
-if (i) {
-/* Secondary CPUs start in PSCI powered-down state */
-object_property_set_bool(o, true,
- "start-powered-off", &error_abort);
-}
-
-object_property_set_bool(o, true, "realized", &error_abort);
-}
+object_property_set_int(OBJECT(&s->cpu), QEMU_PSCI_CONDUIT_SMC,
+"psci-conduit", &error_abort);
+object_property_set_bool(OBJECT(&s->cpu), true,
+ "realized", &error_abort);
 
 /*
  * A7MPCORE
  */
-object_property_set_int(OBJECT(&s->a7mpcore), smp_cpus, "num-cpu",
-&error_abort);
+object_property_set_int(OBJECT(&s->a7mpcore), 1, "num-cpu", &error_abort);
 object_property_set_int(OBJECT(&s->a7mpcore),
 FSL_IMX6UL_MAX_IRQ + GIC_INTERNAL,
 "num-irq", &error_abort);
@@ -204,18 +183,13 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error 
**errp)
  &error_abort);
 sysbus_mmio_map(SYS_BUS_DEVICE(&s->a7mpcore), 0, FSL_IMX6UL_A7MPCORE_ADDR);
 
-for (i = 0; i < smp_cpus; i++) {
-SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
-DeviceState  *d   = DEVICE(qemu_get_cpu(i));
+sbd = SYS_BUS_DEVICE(&s->a7mpcore);
+d = DEVICE(&s->cpu);
 
-irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
-sysbus_connect_irq(sbd, i, irq);
-sysbus_connect_irq(sbd, i + smp_cpus, qdev_get_gpio_in(d, 
ARM_CPU_FIQ));
-sysbus_connect_irq(sbd, i + 2 * smp_cpus,
-

[Qemu-devel] [PATCH v2] libvhost-user: Add missing GCC_FMT_ATTR and fix three format errors

[Stefan Weil]

Reviewed-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Signed-off-by: Stefan Weil 
---

v2:
- Show different value in "Guest says [...]" (suggested by Marc-André Lureau)
- Fix more format errors for 32 bit builds (reported by Philippe Mathieu-Daudé)

Philippe, I did not get the additional errors on x86_64.

Regards
Stefan


 contrib/libvhost-user/libvhost-user.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/contrib/libvhost-user/libvhost-user.c 
b/contrib/libvhost-user/libvhost-user.c
index 4b36e35a82..3b5520a77f 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -142,7 +142,7 @@ vu_request_to_string(unsigned int req)
 }
 }
 
-static void
+static void GCC_FMT_ATTR(2, 3)
 vu_panic(VuDev *dev, const char *msg, ...)
 {
 char *buf = NULL;
@@ -661,7 +661,8 @@ vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg 
*vmsg)
 
 if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, ®_struct)) {
 vu_panic(dev, "%s: Failed to userfault region %d "
-  "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
+  "@%" PRIx64 " + size:%" PRIx64
+  " offset: %" PRIx64 ": (ufd=%d)%s\n",
  __func__, i,
  dev_region->mmap_addr,
  dev_region->size, dev_region->mmap_offset,
@@ -1753,7 +1754,7 @@ virtqueue_get_head(VuDev *dev, VuVirtq *vq,
 
 /* If their number is silly, that's a fatal mistake. */
 if (*head >= vq->vring.num) {
-vu_panic(dev, "Guest says index %u is available", head);
+vu_panic(dev, "Guest says index %u is available", *head);
 return false;
 }
 
@@ -1812,7 +1813,7 @@ virtqueue_read_next_desc(VuDev *dev, struct vring_desc 
*desc,
 smp_wmb();
 
 if (*next >= max) {
-vu_panic(dev, "Desc next is %u", next);
+vu_panic(dev, "Desc next is %u", *next);
 return VIRTQUEUE_READ_DESC_ERROR;
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH for 4.1 v2 4/4] target/mips: Add missing 'break' for certain cases of MTTR handling

[Aleksandar Markovic]

From: Aleksandar Markovic 

This was found by GCC 8.3 static analysis.

Signed-off-by: Aleksandar Markovic 
---
 target/mips/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index b1cf5f0..ca62800 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -10055,6 +10055,7 @@ static void gen_mttr(CPUMIPSState *env, DisasContext 
*ctx, int rd, int rt,
 gen_mtc0(ctx, t0, rd, sel);
 break;
 }
+break;
 case 12:
 switch (sel) {
 case 0:
@@ -10064,6 +10065,7 @@ static void gen_mttr(CPUMIPSState *env, DisasContext 
*ctx, int rd, int rt,
 gen_mtc0(ctx, t0, rd, sel);
 break;
 }
+break;
 case 13:
 switch (sel) {
 case 0:
-- 
2.7.4

[Qemu-devel] [PATCH for 4.1 v2 1/4] target/mips: Add 'fall through' comments for handling nanoMips' SHXS, SWXS

[Aleksandar Markovic]

From: Aleksandar Markovic 

This was found by GCC 8.3 static analysis.

Signed-off-by: Aleksandar Markovic 
---
 target/mips/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index f96f141..2be5e2d 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -20136,12 +20136,14 @@ static void gen_p_lsx(DisasContext *ctx, int rd, int 
rs, int rt)
 switch (extract32(ctx->opcode, 7, 4)) {
 case NM_SHXS:
 check_nms(ctx);
+/* fall through */
 case NM_LHXS:
 case NM_LHUXS:
 tcg_gen_shli_tl(t0, t0, 1);
 break;
 case NM_SWXS:
 check_nms(ctx);
+/* fall through */
 case NM_LWXS:
 case NM_LWC1XS:
 case NM_SWC1XS:
-- 
2.7.4

[Qemu-devel] [PATCH for 4.1 v2 3/4] target/mips: Add missing 'break' for certain cases of MFTR handling

[Aleksandar Markovic]

From: Aleksandar Markovic 

This was found by GCC 8.3 static analysis.

Signed-off-by: Aleksandar Markovic 
---
 target/mips/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 59d4acd..b1cf5f0 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -9826,6 +9826,7 @@ static void gen_mftr(CPUMIPSState *env, DisasContext 
*ctx, int rt, int rd,
 gen_mfc0(ctx, t0, rt, sel);
 break;
 }
+break;
 case 12:
 switch (sel) {
 case 0:
@@ -9835,6 +9836,7 @@ static void gen_mftr(CPUMIPSState *env, DisasContext 
*ctx, int rt, int rd,
 gen_mfc0(ctx, t0, rt, sel);
 break;
 }
+break;
 case 13:
 switch (sel) {
 case 0:
-- 
2.7.4

[Qemu-devel] [PATCH for 4.1 v2 2/4] target/mips: Add missing 'break' for a case of MTHC0 handling

[Aleksandar Markovic]

From: Aleksandar Markovic 

This was found by GCC 8.3 static analysis.

Signed-off-by: Aleksandar Markovic 
---
 target/mips/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 2be5e2d..59d4acd 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -6745,6 +6745,7 @@ static void gen_mthc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
 default:
 goto cp0_unimplemented;
 }
+break;
 case CP0_REGISTER_17:
 switch (sel) {
 case 0:
-- 
2.7.4

[Qemu-devel] [PATCH for 4.1 v2 0/4] target/mips: Fixes for 4.1 rc1

[Aleksandar Markovic]

From: Aleksandar Markovic 

At the moment, this includes fixes for problems in switch statements
found by GCC 8.3 improved code analysis features.

v1->v2:

  - excluded the patch on "ucontext" that will go into linux user queue

Aleksandar Markovic (4):
  target/mips: Add 'fall through' comments for handling nanoMips' SHXS,
SWXS
  target/mips: Add missing 'break' for a case of MTHC0 handling
  target/mips: Add missing 'break' for certain cases of MFTR handling
  target/mips: Add missing 'break' for certain cases of MTTR handling

 target/mips/translate.c | 7 +++
 1 file changed, 7 insertions(+)

-- 
2.7.4

Re: [Qemu-devel] [PATCH v26 1/7] target/avr: Add outward facing interfaces and core CPU logic

[Igor Mammedov]

On Fri, 12 Jul 2019 08:36:58 +0300
Michael Rolnik  wrote:

> From: Sarah Harris 
> 
> This includes:
> - CPU data structures
> - object model classes and functions
> - migration functions
> - GDB hooks
> 
> Signed-off-by: Michael Rolnik 
looks fine to me from QOM point of view

Acked-by: Igor Mammedov 

> ---
>  gdb-xml/avr-cpu.xml|  49 
>  target/avr/cpu-param.h |  37 +++
>  target/avr/cpu.c   | 579 +
>  target/avr/cpu.h   | 280 
>  target/avr/gdbstub.c   |  85 ++
>  target/avr/machine.c   | 123 +
>  6 files changed, 1153 insertions(+)
>  create mode 100644 gdb-xml/avr-cpu.xml
>  create mode 100644 target/avr/cpu-param.h
>  create mode 100644 target/avr/cpu.c
>  create mode 100644 target/avr/cpu.h
>  create mode 100644 target/avr/gdbstub.c
>  create mode 100644 target/avr/machine.c
> 
> diff --git a/gdb-xml/avr-cpu.xml b/gdb-xml/avr-cpu.xml
> new file mode 100644
> index 00..c4747f5b40
> --- /dev/null
> +++ b/gdb-xml/avr-cpu.xml
> @@ -0,0 +1,49 @@
> +
> +
> +
> +
> +
> +
> +
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +
> diff --git a/target/avr/cpu-param.h b/target/avr/cpu-param.h
> new file mode 100644
> index 00..ccd1ea3429
> --- /dev/null
> +++ b/target/avr/cpu-param.h
> @@ -0,0 +1,37 @@
> +/*
> + * QEMU AVR CPU
> + *
> + * Copyright (c) 2019 Michael Rolnik
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see
> + * 
> + */
> +
> +#ifndef AVR_CPU_PARAM_H
> +#define AVR_CPU_PARAM_H 1
> +
> +#define TARGET_LONG_BITS 32
> +/*
> + * TARGET_PAGE_BITS cannot be more than 8 bits because
> + * 1.  all IO registers occupy [0x .. 0x00ff] address range, and they
> + * should be implemented as a device and not memory
> + * 2.  SRAM starts at the address 0x0100
> + */
> +#define TARGET_PAGE_BITS 8
> +#define TARGET_PHYS_ADDR_SPACE_BITS 24
> +#define TARGET_VIRT_ADDR_SPACE_BITS 24
> +#define NB_MMU_MODES 2
> +
> +
> +#endif
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> new file mode 100644
> index 00..c474526925
> --- /dev/null
> +++ b/target/avr/cpu.c
> @@ -0,0 +1,579 @@
> +/*
> + * QEMU AVR CPU
> + *
> + * Copyright (c) 2019 Michael Rolnik
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see
> + * 
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/qemu-print.h"
> +#include "qemu/log.h"
> +#include "cpu.h"
> +#include "exec/exec-all.h"
> +#include "qapi/error.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +
> +static void avr_cpu_set_pc(CPUState *cs, vaddr value)
> +{
> +AVRCPU *cpu = AVR_CPU(cs);
> +
> +cpu->env.pc_w = value / 2; /* internally PC points to words */
> +}
> +
> +static bool avr_cpu_has_work(CPUState *cs)
> +{
> +AVRCPU *cpu = AVR_CPU(cs);
> +CPUAVRState *env = &cpu->env;
> +
> +return (cs->interrupt_request & (CPU_INTERRUPT_HARD | 
> CPU_INTERRUPT_RESET))
> +&& cpu_interrupts_enabled(env);
> +}
> +
> +static void avr_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb)
> +{
> +AVRCPU *cpu = AVR_CPU(cs);
> +CPUAVRState *env = &cpu->env;
> +
> +env->pc_w = tb->pc / 2; /* internally PC points to words */
> +}
> +
> +static void avr_cpu_reset(CPUState *cs)
> +{
> +AVRCPU *cpu = AVR_CPU(cs);
> +AVRCPUClass *mcc = AVR_CPU_GET_CLASS(cpu);
> +CPUAVRState *env = &cpu->env;
> +
> +mcc->parent_reset(cs);
> +
> +env->pc_w = 0;
> +env->sregI = 1;
> +env->sregC = 0;
> +env->sregZ = 0;
> +env->sre

Re: [Qemu-devel] [PATCH for-4.1 1/2] crypto: switch to modern nettle AES APIs

[Alex Bennée]

Daniel P. Berrangé  writes:

> The aes_ctx struct and aes_* functions have been deprecated in nettle
> 3.5, in favour of keysize specific functions which were introduced
> first in nettle 3.0.
>
> Switch QEMU code to use the new APIs and add some backcompat defines
> such that it still builds on nettle 2.7
>
> Signed-off-by: Daniel P. Berrangé 

Reviewed-by: Alex Bennée 
Tested-by: Alex Bennée 

> ---
>  crypto/cipher-nettle.c | 218 ++---
>  1 file changed, 183 insertions(+), 35 deletions(-)
>
> diff --git a/crypto/cipher-nettle.c b/crypto/cipher-nettle.c
> index 3848cb3b3a..115d16dd7b 100644
> --- a/crypto/cipher-nettle.c
> +++ b/crypto/cipher-nettle.c
> @@ -42,29 +42,89 @@ typedef void *   cipher_ctx_t;
>  typedef unsigned cipher_length_t;
>
>  #define cast5_set_key cast128_set_key
> +
> +#define aes128_ctx aes_ctx
> +#define aes192_ctx aes_ctx
> +#define aes256_ctx aes_ctx
> +#define aes128_set_encrypt_key(c, k) \
> +aes_set_encrypt_key(c, 16, k)
> +#define aes192_set_encrypt_key(c, k) \
> +aes_set_encrypt_key(c, 24, k)
> +#define aes256_set_encrypt_key(c, k) \
> +aes_set_encrypt_key(c, 32, k)
> +#define aes128_set_decrypt_key(c, k) \
> +aes_set_decrypt_key(c, 16, k)
> +#define aes192_set_decrypt_key(c, k) \
> +aes_set_decrypt_key(c, 24, k)
> +#define aes256_set_decrypt_key(c, k) \
> +aes_set_decrypt_key(c, 32, k)
> +#define aes128_encrypt aes_encrypt
> +#define aes192_encrypt aes_encrypt
> +#define aes256_encrypt aes_encrypt
> +#define aes128_decrypt aes_decrypt
> +#define aes192_decrypt aes_decrypt
> +#define aes256_decrypt aes_decrypt
>  #else
>  typedef nettle_cipher_func * QCryptoCipherNettleFuncNative;
>  typedef const void * cipher_ctx_t;
>  typedef size_t   cipher_length_t;
>  #endif
>
> -typedef struct QCryptoNettleAES {
> -struct aes_ctx enc;
> -struct aes_ctx dec;
> -} QCryptoNettleAES;
> +typedef struct QCryptoNettleAES128 {
> +struct aes128_ctx enc;
> +struct aes128_ctx dec;
> +} QCryptoNettleAES128;
> +
> +typedef struct QCryptoNettleAES192 {
> +struct aes192_ctx enc;
> +struct aes192_ctx dec;
> +} QCryptoNettleAES192;
> +
> +typedef struct QCryptoNettleAES256 {
> +struct aes256_ctx enc;
> +struct aes256_ctx dec;
> +} QCryptoNettleAES256;
> +
> +static void aes128_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
> +  uint8_t *dst, const uint8_t *src)
> +{
> +const QCryptoNettleAES128 *aesctx = ctx;
> +aes128_encrypt(&aesctx->enc, length, dst, src);
> +}
> +
> +static void aes128_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
> +  uint8_t *dst, const uint8_t *src)
> +{
> +const QCryptoNettleAES128 *aesctx = ctx;
> +aes128_decrypt(&aesctx->dec, length, dst, src);
> +}
> +
> +static void aes192_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
> +   uint8_t *dst, const uint8_t *src)
> +{
> +const QCryptoNettleAES192 *aesctx = ctx;
> +aes192_encrypt(&aesctx->enc, length, dst, src);
> +}
> +
> +static void aes192_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
> +   uint8_t *dst, const uint8_t *src)
> +{
> +const QCryptoNettleAES192 *aesctx = ctx;
> +aes192_decrypt(&aesctx->dec, length, dst, src);
> +}
>
> -static void aes_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
> +static void aes256_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
> uint8_t *dst, const uint8_t *src)
>  {
> -const QCryptoNettleAES *aesctx = ctx;
> -aes_encrypt(&aesctx->enc, length, dst, src);
> +const QCryptoNettleAES256 *aesctx = ctx;
> +aes256_encrypt(&aesctx->enc, length, dst, src);
>  }
>
> -static void aes_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
> +static void aes256_decrypt_native(cipher_ctx_t ctx, cipher_length_t length,
> uint8_t *dst, const uint8_t *src)
>  {
> -const QCryptoNettleAES *aesctx = ctx;
> -aes_decrypt(&aesctx->dec, length, dst, src);
> +const QCryptoNettleAES256 *aesctx = ctx;
> +aes256_decrypt(&aesctx->dec, length, dst, src);
>  }
>
>  static void des_encrypt_native(cipher_ctx_t ctx, cipher_length_t length,
> @@ -127,18 +187,46 @@ static void twofish_decrypt_native(cipher_ctx_t ctx, 
> cipher_length_t length,
>  twofish_decrypt(ctx, length, dst, src);
>  }
>
> -static void aes_encrypt_wrapper(const void *ctx, size_t length,
> +static void aes128_encrypt_wrapper(const void *ctx, size_t length,
> +uint8_t *dst, const uint8_t *src)
> +{
> +const QCryptoNettleAES128 *aesctx = ctx;
> +aes128_encrypt(&aesctx->enc, length, dst, src);
> +}
> +
> +static void aes128_decrypt_wrapper(const void *ctx, size_t length,
>  uint8_t *dst, const uint8_t *src)
>  {
> -const QCryptoNettleAES *aesctx = ctx;
> -aes_encrypt(

Re: [Qemu-devel] [PATCH for-4.1 2/2] crypto: fix function signatures for nettle 2.7 vs 3

[Alex Bennée]

Daniel P. Berrangé  writes:

> Nettle version 2.7.x used 'unsigned int' instead of 'size_t' for length
> parameters in functions. Use a local typedef so that we can build with
> the correct signature depending on nettle version, as we already do in
> the cipher code.
>
> Signed-off-by: Daniel P. Berrangé 

Reviewed-by: Alex Bennée 
Tested-by: Alex Bennée 

> ---
>  crypto/hash-nettle.c | 12 +---
>  crypto/hmac-nettle.c | 17 +
>  2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
> index 96f186f442..6ffb9c3db7 100644
> --- a/crypto/hash-nettle.c
> +++ b/crypto/hash-nettle.c
> @@ -26,12 +26,18 @@
>  #include 
>  #include 
>
> +#if CONFIG_NETTLE_VERSION_MAJOR < 3
> +typedef unsigned int hash_length_t;
> +#else
> +typedef size_t   hash_length_t;
> +#endif
> +
>  typedef void (*qcrypto_nettle_init)(void *ctx);
>  typedef void (*qcrypto_nettle_write)(void *ctx,
> - unsigned int len,
> + hash_length_t len,
>   const uint8_t *buf);
>  typedef void (*qcrypto_nettle_result)(void *ctx,
> -  unsigned int len,
> +  hash_length_t len,
>uint8_t *buf);
>
>  union qcrypto_hash_ctx {
> @@ -112,7 +118,7 @@ qcrypto_nettle_hash_bytesv(QCryptoHashAlgorithm alg,
> size_t *resultlen,
> Error **errp)
>  {
> -int i;
> +size_t i;
>  union qcrypto_hash_ctx ctx;
>
>  if (!qcrypto_hash_supports(alg)) {
> diff --git a/crypto/hmac-nettle.c b/crypto/hmac-nettle.c
> index ec2d61bdde..1152b741fd 100644
> --- a/crypto/hmac-nettle.c
> +++ b/crypto/hmac-nettle.c
> @@ -18,14 +18,23 @@
>  #include "hmacpriv.h"
>  #include 
>
> +#if CONFIG_NETTLE_VERSION_MAJOR < 3
> +typedef unsigned int hmac_length_t;
> +#else
> +typedef size_t hmac_length_t;
> +#endif
> +
>  typedef void (*qcrypto_nettle_hmac_setkey)(void *ctx,
> -  size_t key_length, const uint8_t *key);
> +   hmac_length_t key_length,
> +   const uint8_t *key);
>
>  typedef void (*qcrypto_nettle_hmac_update)(void *ctx,
> -  size_t length, const uint8_t *data);
> +   hmac_length_t length,
> +   const uint8_t *data);
>
>  typedef void (*qcrypto_nettle_hmac_digest)(void *ctx,
> -  size_t length, uint8_t *digest);
> +   hmac_length_t length,
> +   uint8_t *digest);
>
>  typedef struct QCryptoHmacNettle QCryptoHmacNettle;
>  struct QCryptoHmacNettle {
> @@ -135,7 +144,7 @@ qcrypto_nettle_hmac_bytesv(QCryptoHmac *hmac,
> Error **errp)
>  {
>  QCryptoHmacNettle *ctx;
> -int i;
> +size_t i;
>
>  ctx = (QCryptoHmacNettle *)hmac->opaque;


--
Alex Bennée

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels

[Laurent Vivier]

Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
> On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier  wrote:
> 
>>
>> Notes:
>> v4: [lv] timeval64 and timespec64 are { long long , long }
> 
>>
>> +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG)
>> +
>> +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG)
>> +
> 
> This still doesn't look right, see my earlier comment about padding
> on big-endian architectures.
> 
> Note that the in-kernel 'timespec64' is different from the uapi
> '__kernel_timespec' exported by the kernel. I also still think you may
> need to convert between SIOCGSTAMP_NEW and SIOCGSTAMP_OLD,
> e.g. when emulating a 32-bit riscv process (which only use
> SIOCGSTAMP_NEW) on a kernel that only understands
> SIOCGSTAMP_OLD.

I agree.
I'm preparing a patch always using SIOCGSTAMP and SIOCGSTAMPNS on the
host (converting the structure when needed).

I've added the SH4 variant.
I've added the sparc64 variant too: does it means sparc64 use the same
structure internally for the OLD and NEW version?
What about sparc 32bit?

For big-endian, I didn't find in the kernel where the difference is
managed: a byte swapping of the 64bit value is not enough?

Thanks,
Laurent

Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close()

[Kevin Wolf]

Am 12.07.2019 um 13:44 hat Max Reitz geschrieben:
> On 12.07.19 13:23, Kevin Wolf wrote:
> > Am 12.07.2019 um 13:09 hat Max Reitz geschrieben:
> >> On 12.07.19 13:01, Kevin Wolf wrote:
> >>> Am 12.07.2019 um 12:47 hat Max Reitz geschrieben:
>  On 12.07.19 11:24, Kevin Wolf wrote:
> > Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
> >> When nbd_close() is called from a coroutine, the connection_co never
> >> gets to run, and thus nbd_teardown_connection() hangs.
> >>
> >> This is because aio_co_enter() only puts the connection_co into the 
> >> main
> >> coroutine's wake-up queue, so this main coroutine needs to yield and
> >> reschedule itself to let the connection_co run.
> >>
> >> Signed-off-by: Max Reitz 
> >> ---
> >>  block/nbd.c | 12 +++-
> >>  1 file changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/block/nbd.c b/block/nbd.c
> >> index 81edabbf35..b83b6cd43e 100644
> >> --- a/block/nbd.c
> >> +++ b/block/nbd.c
> >> @@ -135,7 +135,17 @@ static void 
> >> nbd_teardown_connection(BlockDriverState *bs)
> >>  qio_channel_shutdown(s->ioc,
> >>   QIO_CHANNEL_SHUTDOWN_BOTH,
> >>   NULL);
> >> -BDRV_POLL_WHILE(bs, s->connection_co);
> >> +
> >> +if (qemu_in_coroutine()) {
> >> +/* Let our caller poll and just yield until connection_co is 
> >> done */
> >> +while (s->connection_co) {
> >> +aio_co_schedule(qemu_get_current_aio_context(),
> >> +qemu_coroutine_self());
> >> +qemu_coroutine_yield();
> >> +}
> >
> > Isn't this busy waiting? Why not let s->connection_co wake us up when
> > it's about to terminate instead of immediately rescheduling ourselves?
> 
>  Yes, it is busy waiting, but I didn’t find that bad.  The connection_co
>  will be invoked in basically every iteration, and once there is no
>  pending data, it will quit.
> 
>  The answer to “why not...” of course is because it’d be more complicated.
> 
>  But anyway.
> 
>  Adding a new function qemu_coroutine_run_after(target) that adds
>  qemu_coroutine_self() to the given @target coroutine’s wake-up queue and
>  then using that instead of scheduling works, too, yes.
> 
>  I don’t really like being responsible for coroutine code, though...
> 
>  (And maybe it’d be better to make it qemu_coroutine_yield_for(target),
>  which does the above and then yields?)
> >>>
> >>> Or just do something like this, which is arguably not only a fix for the
> >>> busy wait, but also a code simplification:
> >>
> >> 1. Is that guaranteed to work?  What if data sneaks in, the
> >> connection_co handles that, and doesn’t wake up the teardown_co?  Will
> >> it be re-scheduled?
> > 
> > Then connection_co is buggy because we clearly requested that it
> > terminate.
> 
> Did we?  This would be done by setting s->quit to true, which isn’t
> explicitly done here.

*we clearly requested implicitly ;-)

> I thought it worked by just waking up the coroutine until it doesn’t
> receive anything anymore, because the connection is closed.  Now I don’t
> know whether QIO_CHANNEL_SHUTDOWN_BOTH discards data that has been
> received before the channel is closed.  I don’t expect it to.

It doesn't really matter, but I expect that we'll still read everything
that was buffered and receive EOF after everything is read.

> >It is possible that it does so only after handling another
> > request, but this wouldn't be a problem. teardown_co would then just
> > sleep for a few cycles more until connection_co is done and reaches the
> > aio_co_wake() call.
> 
> I don’t quite understand, because the fact how connection_co would
> proceed after handling another request was exactly my question.  If it
> were to yield and not to wake up, it would never be done.

But why would it not wake up? This would be a bug, every yield needs a
corresponding place from which the coroutine is reentered later.

If this were missing, it would already today mean that we hang during
shutdown because s->connection_co would never become NULL.

> But I’ve followed nbd_receive_reply() now, and I suppose nbd_read_eof()
> will simply never yield after we have invoked qio_channel_shutdown().

If my expectation above is right, this would probably be the case at
least for the "main" yield. Not sure if there aren't other yield points,
though. But as I said, it doesn't matter anyway how many times the
coroutine yields and is reentered before finally reaching the
aio_co_wake() and terminating.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-devel] 答复: migrate_set_speed has no effect if the guest is using hugepages.

[Dr. David Alan Gilbert]

* Lin Ma (l...@suse.com) wrote:
> 
> 
> > -邮件原件-
> > 发件人: Dr. David Alan Gilbert 
> > 发送时间: 2019年7月11日 18:24
> > 收件人: Lin Ma 
> > 抄送: qemu-devel@nongnu.org
> > 主题: Re: [Qemu-devel] migrate_set_speed has no effect if the guest is using
> > hugepages.
> > 
> > * Lin Ma (l...@suse.com) wrote:
> > > Hi all,
> > 
> > Hi Lin,
> 
> Hi Dave,
> > 
> > > When I live migrate a qemu/kvm guest, If the guest is using huge
> > > pages, I found that the migrate_set_speed command had no effect during
> > stage 2.
> > 
> > Can you explain what you mean by 'stage 2'?
> We know that the live migration contains 3 stages:
> Stage 1: Mark all of RAM dirty.
> Stage 2: Keep sending dirty RAM pages since last iteration
> Stage 3: Stop guest, transfer remaining dirty RAM, device state
> (Please refer to 
> https://developers.redhat.com/blog/2015/03/24/live-migrating-qemu-kvm-virtual-machines/#live-migration
>  for further details)

OK, yeh the numbering is pretty arbitrary so it's not something I
normally think about like that.

> 
> > > It was caused by commit 4c011c3 postcopy: Send whole huge pages
> > >
> > > I'm wondering that is it by design or is it a bug waiting for fix?
> > 
> > This is the first report I've seen for it.  How did you conclude that
> > 4c011c3 caused it?  While I can see it might have some effect on the
> > bandwidth management, I'm surprised it has this much effect.
> 
> While digging into the bandwidth issue, Git bisect shows that this commit was 
> the first bad commit.

OK.

> > What size huge pages are you using - 2MB or 1GB?
> 
> When I hit this issue I was using 1GB huge page size.
> I tested this issue with 2MB page size today On Gigabit LAN, Although the 
> bandwidth control looks
> a little better than using 1GB, But not too much. Please refer to the below 
> test result.

OK, I can certainly see why this might happen with 1GB huge pages; I
need to have a think about a fix.

> > I can imagine we might have a problem that since we only do the sleep 
> > between
> > the hugepages, if we were using 1GB hugepages then we'd see  > data>[sleep][sleep] which isn't as smooth as it used to 
> > be.
> > 
> > Can you give me some more details of your test?
> 
> Live migration bandwidth management testing with 2MB hugepage size:
> sles12sp4_i440fx is a qemu/kvm guest with 6GB memory size.
> Note: the throughput value is approximating value.
> 
> Terminal 1:
> virsh migrate-setspeed sles12sp4_i440fx $bandwidth && virsh migrate --live 
> sles12sp4_i440fx qemu+tcp://5810f/system
> 
> Terminal 2:
> virsh qemu-monitor-command sles12sp4_i440fx --hmp "info migrate"
> 
> bandwidth=5
> throughput: 160 mbps
> 
> bandwidth=10
> throughput: 167 mbps
> 
> bandwidth=15
> throughput: 168 mbps
> 
> bandwidth=20
> throughput: 168 mbps
> 
> bandwidth=21
> throughput: 336 mbps
> 
> bandwidth=22
> throughput: 336 mbps
> 
> bandwidth=25
> throughput: 335.87 mbps
> 
> bandwidth=30
> throughput: 335 mbps
> 
> bandwidth=35
> throughput: 335 mbps
> 
> bandwidth=40
> throughput: 335 mbps
> 
> bandwidth=45
> throughput: 504.00 mbps
> 
> bandwidth=50
> throughput: 500.00 mbps
> 
> bandwidth=55
> throughput: 500.00 mbps
> 
> bandwidth=60
> throughput: 500.00 mbps
> 
> bandwidth=65
> throughput: 650.00 mbps
> 
> bandwidth=70
> throughput: 660.00 mbps

OK, so migrate-setspeed takes a bandwidth in MBytes/sec and I guess
you're throughput is in MBit/sec - so at the higher end it's about
right, and at the lower end it's way off.

Let me think about a fix for this.

What are you using to measure throughput?

Dave

> 
> Thanks,
> Lin
> 
> 
> > Dave
> > 
> > >
> > > Thanks,
> > > Lin
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [RISU PATCH v3 01/18] risugen_common: add helper functions insnv, randint

[Alex Bennée]

Jan Bobek  writes:

> insnv allows emitting variable-length instructions in little-endian or
> big-endian byte order; it subsumes functionality of former insn16()
> and insn32() functions.
>
> randint can reliably generate signed or unsigned integers of arbitrary
> width.
>
> Signed-off-by: Jan Bobek 

Reviewed-by: Alex Bennée 

> ---
>  risugen_common.pm | 55 +--
>  1 file changed, 48 insertions(+), 7 deletions(-)
>
> diff --git a/risugen_common.pm b/risugen_common.pm
> index 71ee996..d63250a 100644
> --- a/risugen_common.pm
> +++ b/risugen_common.pm
> @@ -23,8 +23,9 @@ BEGIN {
>  require Exporter;
>
>  our @ISA = qw(Exporter);
> -our @EXPORT = qw(open_bin close_bin set_endian insn32 insn16 $bytecount
> -   progress_start progress_update progress_end
> +our @EXPORT = qw(open_bin close_bin set_endian insn32 insn16
> +   $bytecount insnv randint progress_start
> +   progress_update progress_end
> eval_with_fields is_pow_of_2 sextract ctz
> dump_insn_details);
>  }
> @@ -37,7 +38,7 @@ my $bigendian = 0;
>  # (default is little endian, 0).
>  sub set_endian
>  {
> -$bigendian = @_;
> +($bigendian) = @_;
>  }
>
>  sub open_bin
> @@ -52,18 +53,58 @@ sub close_bin
>  close(BIN) or die "can't close output file: $!";
>  }
>
> +sub insnv(%)
> +{
> +my (%args) = @_;
> +
> +# Default to big-endian order, so that the instruction bytes are
> +# emitted in the same order as they are written in the
> +# configuration file.
> +$args{bigendian} = 1 unless defined $args{bigendian};
> +
> +for (my $bitcur = 0; $bitcur < $args{width}; $bitcur += 8) {
> +my $value = $args{value} >> ($args{bigendian}
> + ? $args{width} - $bitcur - 8
> + : $bitcur);
> +
> +print BIN pack("C", $value & 0xff);
> +$bytecount += 1;
> +}
> +}
> +
>  sub insn32($)
>  {
>  my ($insn) = @_;
> -print BIN pack($bigendian ? "N" : "V", $insn);
> -$bytecount += 4;
> +insnv(value => $insn, width => 32, bigendian => $bigendian);
>  }
>
>  sub insn16($)
>  {
>  my ($insn) = @_;
> -print BIN pack($bigendian ? "n" : "v", $insn);
> -$bytecount += 2;
> +insnv(value => $insn, width => 16, bigendian => $bigendian);
> +}
> +
> +sub randint
> +{
> +my (%args) = @_;
> +my $width = $args{width};
> +
> +if ($width > 32) {
> +# Generate at most 32 bits at once; Perl's rand() does not
> +# behave well with ranges that are too large.
> +my $lower = randint(%args, width => 32);
> +my $upper = randint(%args, width => $args{width} - 32);
> +# Use arithmetic rather than bitwise operators, since bitwise
> +# ops turn signed integers into unsigned.
> +return $upper * (1 << 32) + $lower;
> +} elsif ($width > 0) {
> +my $halfrange = 1 << ($width - 1);
> +my $value = int(rand(2 * $halfrange));
> +$value -= $halfrange if defined $args{signed} && $args{signed};
> +return $value;
> +} else {
> +return 0;
> +}
>  }
>
>  # Progress bar implementation


--
Alex Bennée

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels

[Arnd Bergmann]

On Fri, Jul 12, 2019 at 2:17 PM Laurent Vivier  wrote:
>
> Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
> > On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier  wrote:
> >
> >>
> >> Notes:
> >> v4: [lv] timeval64 and timespec64 are { long long , long }
> >
> >>
> >> +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG)
> >> +
> >> +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG)
> >> +
> >
> > This still doesn't look right, see my earlier comment about padding
> > on big-endian architectures.
> >
> > Note that the in-kernel 'timespec64' is different from the uapi
> > '__kernel_timespec' exported by the kernel. I also still think you may
> > need to convert between SIOCGSTAMP_NEW and SIOCGSTAMP_OLD,
> > e.g. when emulating a 32-bit riscv process (which only use
> > SIOCGSTAMP_NEW) on a kernel that only understands
> > SIOCGSTAMP_OLD.
>
> I agree.
> I'm preparing a patch always using SIOCGSTAMP and SIOCGSTAMPNS on the
> host (converting the structure when needed).

That in turn would have the problem of breaking in 2038 when the
timestamp overflows.

> I've added the SH4 variant.

What is special about SH4?

> I've added the sparc64 variant too: does it means sparc64 use the same
> structure internally for the OLD and NEW version?

I had to look it up in the code. Yes, it does, timeval is 64/32/pad32,
timespec is 64/64 in both OLD and NEW for sparc.

> What about sparc 32bit?

sparc32 is like all other 32-bit targets: OLD uses 32/32, and
new uses 64/64.

> For big-endian, I didn't find in the kernel where the difference is
> managed: a byte swapping of the 64bit value is not enough?

No, you don't need to swap. The difference is only in the padding.
Since the kernel uses a 64/64 structure here, and user space
may have use 'long tv_nsec', you need to add the padding on
the correct side, like

struct timeval64 {
   long long tv_sec;
#if 32bit && big-endian
   long :32; /* anonymous padding */
#endif
   suseconds_t tv_usec;
#if (32bit && little-endian) || sparc64
   long :32;
#endif
};

 Arnd

Re: [Qemu-devel] [PATCH v2] libvhost-user: Add missing GCC_FMT_ATTR and fix three format errors

[Philippe Mathieu-Daudé]

On 7/12/19 1:50 PM, Stefan Weil wrote:
> Reviewed-by: Marc-André Lureau 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Signed-off-by: Stefan Weil 
> ---
> 
> v2:
> - Show different value in "Guest says [...]" (suggested by Marc-André Lureau)
> - Fix more format errors for 32 bit builds (reported by Philippe 
> Mathieu-Daudé)
> 
> Philippe, I did not get the additional errors on x86_64.

Works for me! I was testing on armhf host.

Thanks for the quick v2,

Phil.

> Regards
> Stefan
> 
> 
>  contrib/libvhost-user/libvhost-user.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/libvhost-user/libvhost-user.c 
> b/contrib/libvhost-user/libvhost-user.c
> index 4b36e35a82..3b5520a77f 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -142,7 +142,7 @@ vu_request_to_string(unsigned int req)
>  }
>  }
>  
> -static void
> +static void GCC_FMT_ATTR(2, 3)
>  vu_panic(VuDev *dev, const char *msg, ...)
>  {
>  char *buf = NULL;
> @@ -661,7 +661,8 @@ vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg 
> *vmsg)
>  
>  if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, ®_struct)) {
>  vu_panic(dev, "%s: Failed to userfault region %d "
> -  "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
> +  "@%" PRIx64 " + size:%" PRIx64
> +  " offset: %" PRIx64 ": (ufd=%d)%s\n",
>   __func__, i,
>   dev_region->mmap_addr,
>   dev_region->size, dev_region->mmap_offset,
> @@ -1753,7 +1754,7 @@ virtqueue_get_head(VuDev *dev, VuVirtq *vq,
>  
>  /* If their number is silly, that's a fatal mistake. */
>  if (*head >= vq->vring.num) {
> -vu_panic(dev, "Guest says index %u is available", head);
> +vu_panic(dev, "Guest says index %u is available", *head);
>  return false;
>  }
>  
> @@ -1812,7 +1813,7 @@ virtqueue_read_next_desc(VuDev *dev, struct vring_desc 
> *desc,
>  smp_wmb();
>  
>  if (*next >= max) {
> -vu_panic(dev, "Desc next is %u", next);
> +vu_panic(dev, "Desc next is %u", *next);
>  return VIRTQUEUE_READ_DESC_ERROR;
>  }
>  
> 

[Qemu-devel] [PATCH] Remove old global variable smp_cpus

[Stefan Weil]

Signed-off-by: Stefan Weil 
---
 include/sysemu/sysemu.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 984c439ac9..9b849cb770 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -103,7 +103,6 @@ extern const char *keyboard_layout;
 extern int win2k_install_hack;
 extern int alt_grab;
 extern int ctrl_grab;
-extern int smp_cpus;
 extern unsigned int max_cpus;
 extern int cursor_hide;
 extern int graphic_rotate;
-- 
2.20.1

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels

[Laurent Vivier]

Le 12/07/2019 à 14:47, Arnd Bergmann a écrit :
> On Fri, Jul 12, 2019 at 2:17 PM Laurent Vivier  wrote:
>>
>> Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
>>> On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier  wrote:
>>>

 Notes:
 v4: [lv] timeval64 and timespec64 are { long long , long }
>>>

 +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG)
 +
 +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG)
 +
>>>
>>> This still doesn't look right, see my earlier comment about padding
>>> on big-endian architectures.
>>>
>>> Note that the in-kernel 'timespec64' is different from the uapi
>>> '__kernel_timespec' exported by the kernel. I also still think you may
>>> need to convert between SIOCGSTAMP_NEW and SIOCGSTAMP_OLD,
>>> e.g. when emulating a 32-bit riscv process (which only use
>>> SIOCGSTAMP_NEW) on a kernel that only understands
>>> SIOCGSTAMP_OLD.
>>
>> I agree.
>> I'm preparing a patch always using SIOCGSTAMP and SIOCGSTAMPNS on the
>> host (converting the structure when needed).
> 
> That in turn would have the problem of breaking in 2038 when the
> timestamp overflows.

No, because SIOCGSTAMP and SIOCGSTAMPNS are aliased to the _NEW versions 
on system supporting them (yes, we need to rebuild the binary, but we have 19 
years to do that).

#define SIOCGSTAMP  ((sizeof(struct timeval))  == 8 ? \
 SIOCGSTAMP_OLD   : SIOCGSTAMP_NEW)
#define SIOCGSTAMPNS((sizeof(struct timespec)) == 8 ? \
 SIOCGSTAMPNS_OLD : SIOCGSTAMPNS_NEW)

> 
>> I've added the SH4 variant.> What is special about SH4?

The definition of _OLD is different:

#define SIOCGSTAMP_OLD  _IOR('s', 100, struct timeval) /* Get stamp (timeval) */
#define SIOCGSTAMPNS_OLD _IOR('s', 101, struct timespec) /* Get stamp 
(timespec) */


> 
>> I've added the sparc64 variant too: does it means sparc64 use the same
>> structure internally for the OLD and NEW version?
> 
> I had to look it up in the code. Yes, it does, timeval is 64/32/pad32,
> timespec is 64/64 in both OLD and NEW for sparc.
> 
>> What about sparc 32bit?
> 
> sparc32 is like all other 32-bit targets: OLD uses 32/32, and
> new uses 64/64.
> 
>> For big-endian, I didn't find in the kernel where the difference is
>> managed: a byte swapping of the 64bit value is not enough?
> 
> No, you don't need to swap. The difference is only in the padding.
> Since the kernel uses a 64/64 structure here, and user space
> may have use 'long tv_nsec', you need to add the padding on
> the correct side, like
> 
> struct timeval64 {
>long long tv_sec;
> #if 32bit && big-endian
>long :32; /* anonymous padding */
> #endif
>suseconds_t tv_usec;
> #if (32bit && little-endian) || sparc64
>long :32;
> #endif
> };

We don't do memcopy() but we set each field one by one, so the padding doesn't 
seem needed if we define correctly the user structure:

struct target_timeval64 {
abi_llong tv_sec;
abi_long tv_usec;
};

and do something like:

struct target_timeval64 *target_tv;
struct timeval *host_tv;
...
__put_user(host_tv->tv_sec, &target_tv->tv_sec);
__put_user(host_tv->tv_usec, &target_tv->tv_usec);
...

Thanks,
Laurent

[Qemu-devel] [PATCH for 4.1] Fix broken build with WHPX enabled

[Stefan Weil]

Signed-off-by: Stefan Weil 
---
 target/i386/whpx-all.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
index 31d47320e4..ed95105eae 100644
--- a/target/i386/whpx-all.c
+++ b/target/i386/whpx-all.c
@@ -1396,7 +1396,7 @@ static int whpx_accel_init(MachineState *ms)
 }
 
 memset(&prop, 0, sizeof(WHV_PARTITION_PROPERTY));
-prop.ProcessorCount = smp_cpus;
+prop.ProcessorCount = ms->smp.cpus;
 hr = whp_dispatch.WHvSetPartitionProperty(
 whpx->partition,
 WHvPartitionPropertyCodeProcessorCount,
@@ -1405,7 +1405,7 @@ static int whpx_accel_init(MachineState *ms)
 
 if (FAILED(hr)) {
 error_report("WHPX: Failed to set partition core count to %d,"
- " hr=%08lx", smp_cores, hr);
+ " hr=%08lx", ms->smp.cores, hr);
 ret = -EINVAL;
 goto error;
 }
-- 
2.20.1

Re: [Qemu-devel] [PATCH for 4.1 v2] linux-user: Fix structure target_ucontext for MIPS

[Laurent Vivier]

Le 12/07/2019 à 13:37, Aleksandar Markovic a écrit :
> From: Aleksandar Markovic 
> 
> Structure ucontext for MIPS is defined in the following way in
> Linux kernel:
> 
> (arch/mips/include/uapi/asm/ucontext.h, lines 54-64)
> 
> struct ucontext {
> /* Historic fields matching asm-generic */
> unsigned long   uc_flags;
> struct ucontext *uc_link;
> stack_t uc_stack;
> struct sigcontext   uc_mcontext;
> sigset_tuc_sigmask;
> 
> /* Extended context structures may follow ucontext */
> unsigned long longuc_extcontext[0];
> };
> 
> Fix the structure target_ucontext for MIPS to reflect the definition
> above, except the correction for field uc_extcontext, which will
> follow at some later time.
> 
> Fixes: 94c5495d
> 
> Reported-by: Dragan Mladjenovic 
> Signed-off-by: Aleksandar Markovic 
> Reviewed-by: Laurent Vivier 
> 
> ---
> 
> v2: rectified a commit message mistake with
> s/target_sigset_t/target_ucontext
> ---
>  linux-user/mips/signal.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/linux-user/mips/signal.c b/linux-user/mips/signal.c
> index 6aa303e..455a8a2 100644
> --- a/linux-user/mips/signal.c
> +++ b/linux-user/mips/signal.c
> @@ -71,10 +71,9 @@ struct sigframe {
>  };
>  
>  struct target_ucontext {
> -target_ulong tuc_flags;
> -target_ulong tuc_link;
> +abi_ulong tuc_flags;
> +abi_ulong tuc_link;
>  target_stack_t tuc_stack;
> -target_ulong pad0;
>  struct target_sigcontext tuc_mcontext;
>  target_sigset_t tuc_sigmask;
>  };
> 

Applied to my linux-user branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v2 05/13] doc: update AMD SEV API spec web link

[Singh, Brijesh]

On 7/11/19 1:06 PM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.si...@amd.com) wrote:
>> Signed-off-by: Brijesh Singh 
>> ---
>>   docs/amd-memory-encryption.txt | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
>> index 43bf3ee6a5..abb9a976f5 100644
>> --- a/docs/amd-memory-encryption.txt
>> +++ b/docs/amd-memory-encryption.txt
>> @@ -98,7 +98,7 @@ AMD Memory Encryption whitepaper:
>>   
>> http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf
>>   
>>   Secure Encrypted Virtualization Key Management:
>> -[1] http://support.amd.com/TechDocs/55766_SEV-KM API_Specification.pdf
>> +[1] https://developer.amd.com/sev/ (Secure Encrypted Virtualization API)
> 
> No; that reference [1] is used a few lines hire up for:
> 
> See SEV KM API Spec [1] 'Launching a guest' usage flow (Appendix A) for the
> complete flow chart.
> 
> 
> so that needs fixing up to actually point to that flowchart or
> equivalent.
> 

OK, I will fix them in next rev.

> That site is useful to include, but I guess it also needs a pointer
> to the Volume2 section 15.34 or the like.
> 

Sure, I will add the Volume2 section note.

-Brijesh

Re: [Qemu-devel] [RISU PATCH v3 00/18] Support for generating x86 SIMD test images

[Alex Bennée]

Jan Bobek  writes:

> This is v3 of the patch series posted in [1] and [2]. Note that this
> is the first fully-featured patch series implementing all desired
> functionality, including (V)LDMXCSR and VSIB-based instructions like
> VGATHER*.
>
> While implementing the last bits required in order to support VGATHERx
> instructions, I ran into problems which required a larger redesign;
> namely, there are no more !emit blocks as their functionality is now
> implemented in regular !constraints blocks. Also, memory constraints
> are specified in !memory blocks, similarly to other architectures.
>
> I tested these changes on my machine; both master and slave modes work
> in both 32-bit and 64-bit modes.

Two things I've noticed:

  ./contrib/generate_all.sh -n 1 x86.risu testcases.x86

takes a very long time. I wonder if this is a consequence of constantly
needing to re-query the random number generator?

The other is:

  set -x RISU ./build/i686-linux-gnu/risu
  ./contrib/record_traces.sh testcases.x86/*.risu.bin

fails on the first trace when validating the playback. Might want to
check why that is.

>
> Cheers,
>  -Jan
>
> Changes since v2:
>   Too many to be listed individually; this patch series might be
>   better reviewed on its own.
>
> References:
>   1. https://lists.nongnu.org/archive/html/qemu-devel/2019-06/msg04123.html
>   2. https://lists.nongnu.org/archive/html/qemu-devel/2019-07/msg1.html
>
> Jan Bobek (18):
>   risugen_common: add helper functions insnv, randint
>   risugen_common: split eval_with_fields into extract_fields and
> eval_block
>   risugen_x86_asm: add module
>   risugen_x86_constraints: add module
>   risugen_x86_memory: add module
>   risugen_x86: add module
>   risugen: allow all byte-aligned instructions
>   risugen: add command-line flag --x86_64
>   risugen: add --xfeatures option for x86
>   x86.risu: add MMX instructions
>   x86.risu: add SSE instructions
>   x86.risu: add SSE2 instructions
>   x86.risu: add SSE3 instructions
>   x86.risu: add SSSE3 instructions
>   x86.risu: add SSE4.1 and SSE4.2 instructions
>   x86.risu: add AES and PCLMULQDQ instructions
>   x86.risu: add AVX instructions
>   x86.risu: add AVX2 instructions
>
>  risugen|   27 +-
>  risugen_arm.pm |6 +-
>  risugen_common.pm  |  117 +-
>  risugen_m68k.pm|3 +-
>  risugen_ppc64.pm   |6 +-
>  risugen_x86.pm |  518 +
>  risugen_x86_asm.pm |  918 
>  risugen_x86_constraints.pm |  154 ++
>  risugen_x86_memory.pm  |   87 +
>  x86.risu   | 4499 
>  10 files changed, 6293 insertions(+), 42 deletions(-)
>  create mode 100644 risugen_x86.pm
>  create mode 100644 risugen_x86_asm.pm
>  create mode 100644 risugen_x86_constraints.pm
>  create mode 100644 risugen_x86_memory.pm
>  create mode 100644 x86.risu


--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 5/7] tests/migration-test: don't spam the logs when we fail

[Alex Bennée]

Laurent Vivier  writes:

> On 12/07/2019 13:18, Alex Bennée wrote:
>> Quite often the information about which test failed is hidden by the
>> wall of repeated failures for each page. Stop outputting the error
>> after 10 bad pages and just summarise the total damage at the end.
>>
>> Signed-off-by: Alex Bennée 
>> ---
>>  tests/migration-test.c | 19 ---
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/migration-test.c b/tests/migration-test.c
>> index b6434628e1c..ce041f80c2a 100644
>> --- a/tests/migration-test.c
>> +++ b/tests/migration-test.c
>> @@ -308,7 +308,7 @@ static void check_guests_ram(QTestState *who)
>>  uint8_t first_byte;
>>  uint8_t last_byte;
>>  bool hit_edge = false;
>> -bool bad = false;
>> +int bad = 0;
>>
>>  qtest_memread(who, start_address, &first_byte, 1);
>>  last_byte = first_byte;
>> @@ -327,15 +327,20 @@ static void check_guests_ram(QTestState *who)
>>  hit_edge = true;
>>  last_byte = b;
>>  } else {
>> -fprintf(stderr, "Memory content inconsistency at %x"
>> -" first_byte = %x last_byte = %x current = 
>> %x"
>> -" hit_edge = %x\n",
>> -address, first_byte, last_byte, b, 
>> hit_edge);
>> -bad = true;
>> +bad++;
>> +if (bad <= 10) {
>> +fprintf(stderr, "Memory content inconsistency at %x"
>> +" first_byte = %x last_byte = %x current = %x"
>> +" hit_edge = %x\n",
>> +address, first_byte, last_byte, b, hit_edge);
>> +}
>>  }
>>  }
>>  }
>> -g_assert_false(bad);
>> +if (bad >= 10) {
>> +fprintf(stderr, "and in another %d pages", bad);
>
> "bad - 10" as you have already displayed 10 errors.

Will do.

>
> Thanks,
> Laurent


--
Alex Bennée

Re: [Qemu-devel] [Qemu-trivial] [PATCH] Remove old global variable smp_cpus

[Laurent Vivier]

Le 12/07/2019 à 15:19, Stefan Weil a écrit :
> Signed-off-by: Stefan Weil 
> ---
>  include/sysemu/sysemu.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 984c439ac9..9b849cb770 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -103,7 +103,6 @@ extern const char *keyboard_layout;
>  extern int win2k_install_hack;
>  extern int alt_grab;
>  extern int ctrl_grab;
> -extern int smp_cpus;
>  extern unsigned int max_cpus;
>  extern int cursor_hide;
>  extern int graphic_rotate;
> 

Fixes: a5e0b331193a ("vl.c: Replace smp global variables with smp
machine properties")

I thin you can also remove max_cpus.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels

[Arnd Bergmann]

On Fri, Jul 12, 2019 at 3:23 PM Laurent Vivier  wrote:
>
> Le 12/07/2019 à 14:47, Arnd Bergmann a écrit :
> > On Fri, Jul 12, 2019 at 2:17 PM Laurent Vivier  wrote:
> >>
> >> Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
> >>> On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier  wrote:
> >>>
> 
>  Notes:
>  v4: [lv] timeval64 and timespec64 are { long long , long }
> >>>
> 
>  +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG)
>  +
>  +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG)
>  +
> >>>
> >>> This still doesn't look right, see my earlier comment about padding
> >>> on big-endian architectures.
> >>>
> >>> Note that the in-kernel 'timespec64' is different from the uapi
> >>> '__kernel_timespec' exported by the kernel. I also still think you may
> >>> need to convert between SIOCGSTAMP_NEW and SIOCGSTAMP_OLD,
> >>> e.g. when emulating a 32-bit riscv process (which only use
> >>> SIOCGSTAMP_NEW) on a kernel that only understands
> >>> SIOCGSTAMP_OLD.
> >>
> >> I agree.
> >> I'm preparing a patch always using SIOCGSTAMP and SIOCGSTAMPNS on the
> >> host (converting the structure when needed).
> >
> > That in turn would have the problem of breaking in 2038 when the
> > timestamp overflows.
>
> No, because SIOCGSTAMP and SIOCGSTAMPNS are aliased to the _NEW versions
> on system supporting them (yes, we need to rebuild the binary, but we have 19
> years to do that).
>
> #define SIOCGSTAMP  ((sizeof(struct timeval))  == 8 ? \
>  SIOCGSTAMP_OLD   : SIOCGSTAMP_NEW)
> #define SIOCGSTAMPNS((sizeof(struct timespec)) == 8 ? \
>  SIOCGSTAMPNS_OLD : SIOCGSTAMPNS_NEW)

Right, makes sense.

> >
> >> I've added the SH4 variant.> What is special about SH4?
>
> The definition of _OLD is different:
>
> #define SIOCGSTAMP_OLD  _IOR('s', 100, struct timeval) /* Get stamp (timeval) 
> */
> #define SIOCGSTAMPNS_OLD _IOR('s', 101, struct timespec) /* Get stamp 
> (timespec) */

Ah, that one.


> > No, you don't need to swap. The difference is only in the padding.
> > Since the kernel uses a 64/64 structure here, and user space
> > may have use 'long tv_nsec', you need to add the padding on
> > the correct side, like
> >
> > struct timeval64 {
> >long long tv_sec;
> > #if 32bit && big-endian
> >long :32; /* anonymous padding */
> > #endif
> >suseconds_t tv_usec;
> > #if (32bit && little-endian) || sparc64
> >long :32;
> > #endif
> > };
>
> We don't do memcopy() but we set each field one by one, so the padding doesn't
> seem needed if we define correctly the user structure:
>
> struct target_timeval64 {
> abi_llong tv_sec;
> abi_long tv_usec;
> };
>
> and do something like:
>
> struct target_timeval64 *target_tv;
> struct timeval *host_tv;
> ...
> __put_user(host_tv->tv_sec, &target_tv->tv_sec);
> __put_user(host_tv->tv_usec, &target_tv->tv_usec);
> ...

That still seems wrong. The user application has a definition
of 'timeval' that contains the padding, so your definition has
to match that.

   Arnd

[Qemu-devel] [PATCH-for-4.1 2/7] hw/usb: Bluetooth HCI USB depends on USB & BLUETOOTH

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/usb/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index 564305e283..1b435ec002 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -82,7 +82,7 @@ config USB_NETWORK
 config USB_BLUETOOTH
 bool
 default y
-depends on USB
+depends on USB && BLUETOOTH
 
 config USB_SMARTCARD
 bool
-- 
2.20.1

[Qemu-devel] [PATCH-for-4.1? 0/7] vl: Allow building with CONFIG_BLUETOOTH disabled

[Philippe Mathieu-Daudé]

A series of obvious patches to build without the deprecated
bluetooth devices. Still worth for 4.1 or too late?
It is clearly not a bugfix.

Regards,

Phil.

Philippe Mathieu-Daudé (7):
  hw/arm: Nokia N-series tablet requires Bluetooth
  hw/usb: Bluetooth HCI USB depends on USB & BLUETOOTH
  MAINTAINERS: Add a Bluetooth entry
  vl: Fix 'braces' coding style issues
  vl: Use qemu_strtoi() instead of strtol()
  vl: Extract bt_parse() into its own file
  hw/bt: Allow building with CONFIG_BLUETOOTH disabled

 MAINTAINERS |   7 +++
 Makefile.objs   |   3 +-
 bt-opts.c   | 140 
 bt-stubs.c  |  18 ++
 hw/arm/Kconfig  |   1 +
 hw/bt/Makefile.objs |   4 +-
 hw/usb/Kconfig  |   2 +-
 include/sysemu/bt.h |   3 +
 vl.c| 121 --
 9 files changed, 174 insertions(+), 125 deletions(-)
 create mode 100644 bt-opts.c
 create mode 100644 bt-stubs.c

-- 
2.20.1