Re: [PATCH 2/2] util/qemu-sockets: make keep-alive enabled by default

[Daniel P . Berrangé]

On Wed, Jul 08, 2020 at 10:15:39PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Keep-alive won't hurt, let's try to enable it even if not requested by
> user.

Keep-alive intentionally breaks TCP connections earlier than normal
in face of transient networking problems.

The question is more about which type of pain is more desirable. A
stall in the network connection (for a potentially very long time),
or an intentionally broken socket.

I'm not at all convinced it is a good idea to intentionally break
/all/ QEMU sockets in the face of transient problems, even if the
problems last for 2 hours or more. 

I could see keep-alives being ok on some QEMU socket. For example
VNC/SPICE clients, as there is no downside to proactively culling
them as they can trivially reconnect. Migration too is quite
reasonable to use keep alives, as you generally want migration to
run to completion in a short amount of time, and aborting migration
needs to be safe no matter what.

Breaking chardevs or block devices or network devices that use
QEMU sockets though will be disruptive. The only solution once
those backends have a dead socket is going to be to kill QEMU
and cold-boot the VM again.


> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  util/qemu-sockets.c | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index b961963472..f6851376f5 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -438,7 +438,8 @@ static struct addrinfo 
> *inet_parse_connect_saddr(InetSocketAddress *saddr,
>   *
>   * Handle keep_alive settings. If user specified settings explicitly, fail if
>   * can't set the settings. If user just enabled keep-alive, not specifying 
> the
> - * settings, try to set defaults but ignore failures.
> + * settings, try to set defaults but ignore failures. If keep-alive option is
> + * not specified, try to set it but ignore failures.
>   */
>  static int inet_set_keepalive(int sock, bool has_keep_alive,
>KeepAliveField *keep_alive, Error **errp)
> @@ -447,8 +448,8 @@ static int inet_set_keepalive(int sock, bool 
> has_keep_alive,
>  int val;
>  bool has_settings = has_keep_alive &&  keep_alive->type == QTYPE_QDICT;
>  
> -if (!has_keep_alive || (keep_alive->type == QTYPE_QBOOL &&
> -!keep_alive->u.enabled))
> +if (has_keep_alive &&
> +keep_alive->type == QTYPE_QBOOL && !keep_alive->u.enabled)
>  {
>  return 0;
>  }
> @@ -456,8 +457,12 @@ static int inet_set_keepalive(int sock, bool 
> has_keep_alive,
>  val = 1;
>  ret = qemu_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, , sizeof(val));
>  if (ret < 0) {
> -error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> -return -1;
> +if (has_keep_alive) {
> +error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> +return -1;
> +} else {
> +return 0;
> +}
>  }
>  
>  val = has_settings ? keep_alive->u.settings.idle : 30;
> -- 
> 2.21.0
> 

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

Re: [PATCH RFC 4/5] s390x: implement virtio-mem-ccw

[Cornelia Huck]

On Wed,  8 Jul 2020 20:51:34 +0200
David Hildenbrand  wrote:

> Add a proper CCW proxy device, similar to the PCI variant.
> 
> Signed-off-by: David Hildenbrand 
> ---
>  hw/s390x/virtio-ccw-mem.c | 165 ++
>  hw/s390x/virtio-ccw.h |  13 +++
>  2 files changed, 178 insertions(+)
>  create mode 100644 hw/s390x/virtio-ccw-mem.c

(...)

> +static void virtio_ccw_mem_instance_init(Object *obj)
> +{
> +VirtIOMEMCcw *ccw_mem = VIRTIO_MEM_CCW(obj);
> +VirtIOMEMClass *vmc;
> +VirtIOMEM *vmem;
> +

I think you want

ccw_dev->force_revision_1 = true;

here (similar to forcing virtio-pci to modern-only.)

> +virtio_instance_init_common(obj, _mem->vdev, sizeof(ccw_mem->vdev),
> +TYPE_VIRTIO_MEM);
> +
> +ccw_mem->size_change_notifier.notify = virtio_ccw_mem_size_change_notify;
> +vmem = VIRTIO_MEM(_mem->vdev);
> +vmc = VIRTIO_MEM_GET_CLASS(vmem);
> +/*
> + * We never remove the notifier again, as we expect both devices to
> + * disappear at the same time.
> + */
> +vmc->add_size_change_notifier(vmem, _mem->size_change_notifier);
> +
> +object_property_add_alias(obj, VIRTIO_MEM_BLOCK_SIZE_PROP,
> +  OBJECT(_mem->vdev),
> +  VIRTIO_MEM_BLOCK_SIZE_PROP);
> +object_property_add_alias(obj, VIRTIO_MEM_SIZE_PROP, 
> OBJECT(_mem->vdev),
> +  VIRTIO_MEM_SIZE_PROP);
> +object_property_add_alias(obj, VIRTIO_MEM_REQUESTED_SIZE_PROP,
> +  OBJECT(_mem->vdev),
> +  VIRTIO_MEM_REQUESTED_SIZE_PROP);
> +}

(...)

(have not looked at the rest yet)

Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it

[Paolo Bonzini]

On 09/07/20 11:55, Mohammed Gamal wrote:
>> Ideally we would simply outlaw (3), but it's hard for backward
>> compatibility reasons.  Second best solution is a flag somewhere
>> (msr, cpuid, ...) telling the guest firmware "you can use
>> GUEST_MAXPHYADDR, we guarantee it is <= HOST_MAXPHYADDR".
> Problem is GUEST_MAXPHYADDR > HOST_MAXPHYADDR is actually a supported
> configuration on some setups. Namely when memory encryption is enabled
> on AMD CPUs[1].
> 

It's not that bad since there's two MAXPHYADDRs, the one in CPUID and
the one computed internally by the kernel.  GUEST_MAXPHYADDR greater
than the host CPUID maxphyaddr is never supported.

Paolo

Re: [PATCH 1/7] migration/savevm: respect qemu_fclose() error code in save_snapshot()

[Juan Quintela]

"Denis V. Lunev"  wrote:
> qemu_fclose() could return error, f.e. if bdrv_co_flush() will return
> the error.
>
> This validation will become more important once we will start waiting of
> asynchronous IO operations, started from bdrv_write_vmstate(), which are
> coming soon.
>
> Signed-off-by: Denis V. Lunev 
> Reviewed-by: "Dr. David Alan Gilbert" 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> CC: Kevin Wolf 
> CC: Max Reitz 
> CC: Stefan Hajnoczi 
> CC: Fam Zheng 
> CC: Juan Quintela 
> CC: Denis Plotnikov 

Reviewed-by: Juan Quintela 

queued


> ---
>  migration/savevm.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index b979ea6e7f..da3dead4e9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2628,7 +2628,7 @@ int save_snapshot(const char *name, Error **errp)
>  {
>  BlockDriverState *bs, *bs1;
>  QEMUSnapshotInfo sn1, *sn = , old_sn1, *old_sn = _sn1;
> -int ret = -1;
> +int ret = -1, ret2;
>  QEMUFile *f;
>  int saved_vm_running;
>  uint64_t vm_state_size;
> @@ -2712,10 +2712,14 @@ int save_snapshot(const char *name, Error **errp)
>  }
>  ret = qemu_savevm_state(f, errp);
>  vm_state_size = qemu_ftell(f);
> -qemu_fclose(f);
> +ret2 = qemu_fclose(f);
>  if (ret < 0) {
>  goto the_end;
>  }
> +if (ret2 < 0) {
> +ret = ret2;
> +goto the_end;
> +}
>  
>  /* The bdrv_all_create_snapshot() call that follows acquires the 
> AioContext
>   * for itself.  BDRV_POLL_WHILE() does not support nested locking because

Re: [PULL 13/29] qapi: Flatten object-add

[Markus Armbruster]

Paolo Bonzini  writes:

> On 08/07/20 18:05, Kevin Wolf wrote:
>> Markus was going to introduce new QAPI schema syntax that would allow to
>> specify a few options explicitly and then one option for "the rest" that
>> would just be mapped to a QDict like "any" or "gen": false, and that
>> wouldn't require any nesting.
>
> Oh, I wasn't aware of that.  That would be something like 'properties':
> 'remainder' I guess.  That would be fine too.

Glad I'm spared a design argument ;)

>> I'm not sure if any progress was made there, but making things

Not yet; error handling ate me whole, and spit me out only the day
before yesterday or so.

>> consistent between device_add, netdev_add and object_add was a step in
>> this direction anyway.

Yes.

Permit me to digress.

We ran into the "and also arbitray properties" need repeatedly, and
tried several solutions over time.  QAPI/QMP is big, and the left hand
often wasn't too interested in what the right hand had been doing.

If memory and my git archaeology skills serve, the first instance was
device_add:

3418bd25e1 "qdev hotplug: infrastructure and monitor commands."
2009-10-05

Simple adaption of the QemuOpts-based -device for HMP.  Since QemuOpts
is flat, so is device_add.  QAPI didn't exist, yet.  A QMP version
followed:

8bc27249f0 "monitor: convert do_device_add() to QObject"
2010-03-16

Just as flat.

Next was netdev_add:

ae82d3242d "monitor: New commands netdev_add, netdev_del"
2010-04-18

Flat for the same reason.

When QAPI came along in

ebffe2afce "Merge remote-tracking branch 'qmp/queue/qmp' into
staging"
2011-10-10

these two commands were left unQAPIfied, as the schema language could
not express "and also arbitrary properties".  Soon "solved" with a cop
out:

5dbee474f3 "qapi: allow a 'gen' key to suppress code generation"
2011-12-15

A half-hearted QAPIfication of netdev_add followed:

928059a37b "qapi: convert netdev_add"
2012-06-04

QAPI schema:

{ 'command': 'netdev_add',
  'data': {'type': 'str', 'id': 'str', '*props': '**'},
  'gen': 'no' }

Note the bogus type '**', which only works because 'gen': 'no' also
bypasses type checking (which you wouldn't guess from the commit
message, documentation, or even the schema).  In fact, the whole 'props'
thing is a lie: there is no such parameter, the command is as flat as it
ever was.  Fixed in

b8a98326d5 "qapi-schema: Fix up misleading specification of
netdev_add"
2015-09-21

{ 'command': 'netdev_add',
  'data': {'type': 'str', 'id': 'str'},
  'gen': false }

but by then it had already spread to object-add, with an equally bogus
type 'dict':

cff8b2c6fc "monitor: add object-add (QMP) and object_add (HMP)
command"
2014-01-06

{ 'command': 'object-add',
  'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
  'gen': 'no' }

Only 'props' was real this time: you really had to wrap the properties
in "props": { ... }.  Non-flat.  Meh.

Eventually, even device_add made it into the schema:

94cfd07f26 "qapi-schema: add 'device_add'"
2016-09-19

{ 'command': 'device_add',
  'data': {'driver': 'str', 'id': 'str'},
  'gen': false } # so we can get the additional arguments

And netdev_add was finally done right:

db2a380c84 "net: Complete qapi-fication of netdev_add"
2020-03-17

{ 'command': 'netdev_add', 'data': 'Netdev', 'boxed': true }

Doing device_add and object-add right is harder (impractical?), because
their schema would be a union with one branch per device / object type.

End of digression.

>>> As an aside, it would have been nice to run this through Markus and me,
>>> though in all fairness I'm not sure I would have been responsive back
>>> in February.
>> It went through my tree because of the other patches in the series, but
>> I wrote this patch specifically at Markus's request.

Yes.  We discussed how to best satisfy Kevin's immediate needs without
making other problems harder.  Perhaps we should have posted a summary
to the list.

>>> I would like to un-deprecate this for 5.1, and revert it in either 5.1
>>> or 5.2.  (Also I will be away next week, so the decision would have to
>>> be taken quickly).
>> Please discuss it with Markus then.

Re: [PULL 00/12] Block layer patches

[Kevin Wolf]

Am 07.07.2020 um 18:34 hat Kevin Wolf geschrieben:
> The following changes since commit 7623b5ba017f61de5d7c2bba12c6feb3d55091b1:
> 
>   Merge remote-tracking branch 
> 'remotes/vivier2/tags/linux-user-for-5.1-pull-request' into staging 
> (2020-07-06 11:40:10 +0100)
> 
> are available in the Git repository at:
> 
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
> 
> for you to fetch changes up to 7bf114070834e1b0c947b7c2a1c96cb734eb6b86:
> 
>   qemu-img: Deprecate use of -b without -F (2020-07-07 18:18:06 +0200)
> 
> 
> Block layer patches:
> 
> - file-posix: Mitigate file fragmentation with extent size hints
> - Tighten qemu-img rules on missing backing format
> - qemu-img map: Don't limit block status request size

This has merge conflicts with Max's pull request that has been merged
since I sent this one, so I'll have to send a v2 once they are resolved
(unfortunately one of the conflicts is not completely trivial).

Kevin

Re: [PATCH] tests/qtest/fuzz: Add missing spaces in description

[Philippe Mathieu-Daudé]

On 7/9/20 10:37 AM, Thomas Huth wrote:
> There should be a space between "forking" and "for".
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/qtest/fuzz/virtio_scsi_fuzz.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qtest/fuzz/virtio_scsi_fuzz.c 
> b/tests/qtest/fuzz/virtio_scsi_fuzz.c
> index 51dce491ab..3a9ea13736 100644
> --- a/tests/qtest/fuzz/virtio_scsi_fuzz.c
> +++ b/tests/qtest/fuzz/virtio_scsi_fuzz.c
> @@ -191,7 +191,7 @@ static void register_virtio_scsi_fuzz_targets(void)
>  {
>  fuzz_add_qos_target(&(FuzzTarget){
>  .name = "virtio-scsi-fuzz",
> -.description = "Fuzz the virtio-scsi virtual queues, forking"
> +.description = "Fuzz the virtio-scsi virtual queues, forking 
> "
>  "for each fuzz run",
>  .pre_vm_init = _shm_init,
>  .pre_fuzz = _scsi_pre_fuzz,
> @@ -202,7 +202,7 @@ static void register_virtio_scsi_fuzz_targets(void)
>  
>  fuzz_add_qos_target(&(FuzzTarget){
>  .name = "virtio-scsi-flags-fuzz",
> -.description = "Fuzz the virtio-scsi virtual queues, forking"
> +.description = "Fuzz the virtio-scsi virtual queues, forking 
> "
>  "for each fuzz run (also fuzzes the virtio flags)",
>  .pre_vm_init = _shm_init,
>  .pre_fuzz = _scsi_pre_fuzz,
> 

Uh I thought we had fixed these already :/

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] cpu: Add starts_halted() method

[Greg Kurz]

On Thu, 9 Jul 2020 12:18:06 +0200
Philippe Mathieu-Daudé  wrote:

> On 7/9/20 11:54 AM, Greg Kurz wrote:
> > On Thu, 9 Jul 2020 07:11:09 +0200
> > Philippe Mathieu-Daudé  wrote:
> >> On 7/8/20 11:39 PM, Eduardo Habkost wrote:
> >>> On Wed, Jul 08, 2020 at 06:45:57PM +0200, Philippe Mathieu-Daudé wrote:
>  On 7/8/20 5:25 PM, Eduardo Habkost wrote:
> > On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
> >> On Wed, 8 Jul 2020 at 12:12, David Gibson 
> >>  wrote:
> >>> On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé 
> >>> wrote:
>  Class boolean field certainly sounds better, but I am not sure this
>  is a property of the machine. Rather the arch? So move the field
>  to CPUClass? Maybe not, let's discuss :)
> >>>
> >>> It is absolutely a property of the machine.  e.g. I don't think we
> >>> want this for powernv.  pseries is a bit of a special case since it is
> >>> explicitly a paravirt platform.  But even for emulated hardware, the
> >>> board can absolutely strap things so that cpus do or don't start
> >>> immediately.
> >>
> >> It's a property of the individual CPU, I think. One common setup
> >> for Arm systems is that the primary CPU starts powered up but
> >> the secondaries all start powered down.
> >
> > Both statements can be true.  It can be a property of the
> > individual CPU (although I'm not convinced it has to), but it
> > still needs to be controlled by the machine.
> 
>  From what said Peter, I understand this is a property of the
>  chipset. Chipsets are modelled unevenly.
> 
>  IIUC QEMU started with single-core CPUs.
>  CPUState had same meaning for 'core' or 'cpu', 1-1 mapping.
> 
>  Then multicore CPUs could be easily modelled using multiple
>  single-core CPUs, usually created in the machine code.
> 
>  Then we moved to SoC models, creating the cores in the SoC.
>  Some SoCs have array of cores, eventually heterogeneous
>  (see the ZynqMP). We have containers of CPUState.
> 
>  On an ARM-based SoC, you might have the first core started
>  (as said Peter) or not.
> 
>  BCM2836 / BCM2837 and ZynqMP start will all ARM cores off.
>  On the BCM chipsets, a DSP core will boot the ARM cores.
>  On the ZynqMP, a MicroBlaze core boots them.
>  As QEMU doesn't models heterogeneous architectures, we start
>  modelling after the unmodelled cores booted us, so either one
>  or all cores on.
> 
>  In this case, we narrowed down the 'start-powered-off' field
>  to the SoC, which happens to be how ARM SoCs are modelled.
> >>>
> >>> I was not aware of the start-powered-off property.  If we make it
> >>> generic, we can just let spapr use it.
> >>>
> 
> 
>  Chipsets providing a JTAG interface can have a SRST signal,
>  the "system reset". When a JTAG probe is attached, it can
>  keeps the whole chipset in a reset state. This is equivalent
>  to QEMU '-S' mode (single step mode).
> 
> 
>  I don't know about pseries hardware, but if this is 'explicit
>  to paravirt platform', then I expect this to be the same with
>  other accelerators/architectures.
> 
>  If paravirtualized -> cores start off by default. Let the
>  hypervisor start them. So still a property of the CPUState
>  depending on the accelerator used?
> >>>
> >>> I don't understand this part.  Why would this depend on the
> >>> accelerator?
> >>
> >> Because starting a virtualized machine with all cores powered-off
> >> with TCG accelerator should at least emit a warning? Or change
> >> the behavior and start them powered-on? This is machine-specific
> >> although.
> >>
> > 
> > FYI, PAPR requires all vCPUs to be "stopped" by default. It is up to the
> > guest to start them explicitly through an RTAS call. The hypervisor is
> > only responsible to start a single vCPU (see spapr_cpu_set_entry_state()
> > called from spapr_machine_reset()) to be able to boot the guest.
> > 
> > So I'm not sure to see how that would depend on the accelerator...
> 
> $ qemu-system-ppc64 -M pseries-5.0,accel=tcg -d in_asm
> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> cap-cfpc=workaround
> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> cap-sbbc=workaround
> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> cap-ibs=workaround
> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> cap-ccf-assist=on
> 
> IN:
> 0x0100:  48003f00  b0x4000
> 
> 
> IN:
> 0x4000:  7c7f1b78  mr   r31, r3
> 0x4004:  7d6000a6  mfmsrr11
> 0x4008:  3980a000  li   r12, 0xa000
> 0x400c:  798c83c6  sldi r12, r12, 0x30
> 0x4010:  7d6b6378  or   r11, r11, r12
> 0x4014:  7d600164  mtmsrd   r11
> ...
> 
> The vCPU doesn't seem stopped to 

Re: [PULL 00/53] Misc patches for QEMU 5.1 soft freeze

[Claudio Fontana]

On 7/8/20 8:41 PM, Paolo Bonzini wrote:
> 
> 
> Il mer 8 lug 2020, 20:25 Claudio Fontana  > ha scritto:
> 
> What I did notice is that all the code that directly or indirectly uses 
> the functions is under an
> 
> if (0) (
> )
> 
> since tcg_enabled is the constant 0.
> 
> By "indirectly" I mean that the static void qemu_tcg_cpu_thread_fn() 
> function that calls those is referenced only by static void 
> qemu_tcg_init_vcpu(), which is called only under an if (0),
> ie if (tcg_enabled()).
> 
> 
> Maybe my compiler is older.
> 
> I admit I am not familiar with the rationale of why the stubs are all 
> built regardless, could we have that icount.o from stubs/ is replacing 
> softmmu/icount.o to cause this?
> 
> 
> No, stubs are in a static library and therefore are always overridden by 
> symbols in the executable's .o files.
> 
> Paolo
> 
> 

Hi Paolo,

which compiler, linker, binutils etc are you using?

I am using

$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib64/gcc/x86_64-suse-linux/7/lto-wrapper
OFFLOAD_TARGET_NAMES=hsa:nvptx-none
Target: x86_64-suse-linux
Configured with: ../configure --prefix=/usr --infodir=/usr/share/info 
--mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64 
--enable-languages=c,c++,objc,fortran,obj-c++,ada,go 
--enable-offload-targets=hsa,nvptx-none=/usr/nvptx-none, --without-cuda-driver 
--enable-checking=release --disable-werror 
--with-gxx-include-dir=/usr/include/c++/7 --enable-ssp --disable-libssp 
--disable-libvtv --disable-libcc1 --disable-plugin 
--with-bugurl=https://bugs.opensuse.org/ --with-pkgversion='SUSE Linux' 
--with-slibdir=/lib64 --with-system-zlib --enable-libstdcxx-allocator=new 
--disable-libstdcxx-pch --enable-version-specific-runtime-libs 
--with-gcc-major-version-only --enable-linker-build-id --enable-linux-futex 
--enable-gnu-indirect-function --program-suffix=-7 --without-system-libunwind 
--enable-multilib --with-arch-32=x86-64 --with-tune=generic 
--build=x86_64-suse-linux --host=x86_64-suse-linux
Thread model: posix
gcc version 7.5.0 (SUSE Linux) 


$ ld -v
GNU ld (GNU Binutils; openSUSE Leap 15.1) 2.32.0.20190909-lp151.3.3

anything else that we could use to find the real problem?

Of course I can try a blind fix, where I suggest to explicitly provide the 
stubs,
or you can apply the workaround you already suggested if you want,
but currently I do not have any way to ensure that what I build is ok,
since apparently the local build or any of the CI (travis, cirrus) is not 
sufficient to capture this.

So getting to the bottom of the issue would be important going forward I think 
so I can ensure better input from my side.

In general, the way current code is relying on the compiler to (maybe) compile 
out code that calls undefined symbols,
using #define tcg_enabled 0 , and then having the undefined function calls in 
the code, seems less preferable than solving the problem explicitly
with proper refactoring or with explicit stubs until proper refactoring is 
complete..

Thanks,

Claudio

 

Re: [PATCH 1/2] sockets: keep-alive settings

[Daniel P . Berrangé]

On Wed, Jul 08, 2020 at 10:15:38PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Introduce keep-alive settings (TCP_KEEPCNT, TCP_KEEPIDLE,
> TCP_KEEPINTVL) and chose some defaults.
> 
> The linux default of 2 hours for /proc/tcp_keepalive_time
> (corresponding to TCP_KEEPIDLE) makes keep-alive option almost
> superfluous. Let's add a possibility to set the options by hand
> and specify some defaults resulting in smaller total time to terminate
> idle connection.

As you say, 2 hours just a default. The sysadmin can override that
as they wish to change the behaviour globally on the system, so using
the global settings is quite reasonable IMHO.

> Do not document the default values in QAPI as they may be altered in
> future (careful user will use explicit values).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
> 
> Suggested default numbers are RFC, any better suggestion is welcome.
> I just looked at /etc/libvirt/qemu.conf in my system and take values of
> keepalive_interval and keepalive_count.
> The only thing I'm sure in is that 2 hours is too long.
> 
>  qapi/sockets.json   | 33 +++-
>  util/qemu-sockets.c | 76 ++---
>  2 files changed, 97 insertions(+), 12 deletions(-)
> 
> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index cbd6ef35d0..73ff66a5d5 100644
> --- a/qapi/sockets.json
> +++ b/qapi/sockets.json
> @@ -37,6 +37,37 @@
>  'host': 'str',
>  'port': 'str' } }
>  
> +##
> +# @KeepAliveSettings:
> +#
> +# @idle: The time (in seconds) the connection needs to remain idle
> +#before TCP starts sending keepalive probes (sets TCP_KEEPIDLE).
> +# @interval: The time (in seconds) between individual keepalive probes
> +#(sets TCP_KEEPINTVL).
> +# @count: The maximum number of keepalive probes TCP should send before
> +# dropping the connection (sets TCP_KEEPCNT).
> +#
> +# Since: 5.2
> +##
> +{ 'struct': 'KeepAliveSettings',
> +  'data': {
> +'idle': 'int',
> +'interval': 'int',
> +'count': 'int' } }
> +
> +##
> +# @KeepAliveField:
> +#
> +# @enabled: If true, enable keep-alive with some default settings
> +# @settings: Enable keep-alive and use explicit settings
> +#
> +# Since: 5.2
> +##
> +{ 'alternate': 'KeepAliveField',
> +  'data': {
> +'enabled': 'bool',
> +'settings': 'KeepAliveSettings' } }
> +
>  ##
>  # @InetSocketAddress:
>  #
> @@ -65,7 +96,7 @@
>  '*to': 'uint16',
>  '*ipv4': 'bool',
>  '*ipv6': 'bool',
> -'*keep-alive': 'bool' } }
> +'*keep-alive': 'KeepAliveField' } }
>  
>  ##
>  # @UnixSocketAddress:
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index b37d288866..b961963472 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -433,6 +433,57 @@ static struct addrinfo 
> *inet_parse_connect_saddr(InetSocketAddress *saddr,
>  return res;
>  }
>  
> +/*
> + * inet_set_keepalive
> + *
> + * Handle keep_alive settings. If user specified settings explicitly, fail if
> + * can't set the settings. If user just enabled keep-alive, not specifying 
> the
> + * settings, try to set defaults but ignore failures.
> + */
> +static int inet_set_keepalive(int sock, bool has_keep_alive,
> +  KeepAliveField *keep_alive, Error **errp)
> +{
> +int ret;
> +int val;
> +bool has_settings = has_keep_alive &&  keep_alive->type == QTYPE_QDICT;
> +
> +if (!has_keep_alive || (keep_alive->type == QTYPE_QBOOL &&
> +!keep_alive->u.enabled))
> +{
> +return 0;
> +}
> +
> +val = 1;
> +ret = qemu_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, , sizeof(val));
> +if (ret < 0) {
> +error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> +return -1;
> +}
> +
> +val = has_settings ? keep_alive->u.settings.idle : 30;
> +ret = qemu_setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, , 
> sizeof(val));
> +if (has_settings && ret < 0) {
> +error_setg_errno(errp, errno, "Unable to set TCP_KEEPIDLE");
> +return -1;
> +}
> +
> +val = has_settings ? keep_alive->u.settings.interval : 30;
> +ret = qemu_setsockopt(sock, IPPROTO_TCP, TCP_KEEPINTVL, , 
> sizeof(val));
> +if (has_settings && ret < 0) {
> +error_setg_errno(errp, errno, "Unable to set TCP_KEEPINTVL");
> +return -1;
> +}
> +
> +val = has_settings ? keep_alive->u.settings.count : 20;
> +ret = qemu_setsockopt(sock, IPPROTO_TCP, TCP_KEEPCNT, , sizeof(val));
> +if (has_settings && ret < 0) {
> +error_setg_errno(errp, errno, "Unable to set TCP_KEEPCNT");
> +return -1;
> +}
> +
> +return 0;
> +}
> +
>  /**
>   * Create a socket and connect it to an address.
>   *
> @@ -468,16 +519,11 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error 
> **errp)
>  return sock;
>  }
>  
> -if (saddr->keep_alive) {
> -int val = 1;
> -int ret = qemu_setsockopt(sock, SOL_SOCKET, 

Re: [PATCH v7 00/47] block: Deal with filters

[Andrey Shinkevich]

On 09.07.2020 11:20, Max Reitz wrote:

On 08.07.20 22:47, Eric Blake wrote:

On 7/8/20 12:20 PM, Andrey Shinkevich wrote:

On 25.06.2020 18:21, Max Reitz wrote:

v6:
https://lists.nongnu.org/archive/html/qemu-devel/2019-08/msg01715.html

Branch: https://github.com/XanClic/qemu.git child-access-functions-v7
Branch: https://git.xanclic.moe/XanClic/qemu.git
child-access-functions-v7



I cloned the branch from the github and built successfully.

Running the iotests reports multiple errors of such a kind:

128: readarray -td '' formatting_line < <(sed -e 's/, fmt=/\x0/')

"./common.filter: line 128: readarray: -d: invalid option"

introduced with the commit

a7399eb iotests: Make _filter_img_create more active

You appear to be staging off an unreleased preliminary tree.  a7399eb is
not upstream; the upstream commit 'iotests: Make _filter_img_create more
active' is commit 57ee95ed, and while it uses readarray, it does not use
the problematic -d.  In other words, it looks like the problem was
caught and fixed in between the original patch creation and the pull
request.

Ah, sorry, my mail client’s threading layout hid this mail from me a bit.

Yes.  Well, no, it wasn’t fixed before the pull request, but it was
fixed in the second pull request.  But yes.

Max


I'm clear with it now. Thank you all for your explenations and time!

Andrey

Re: qemu-system-ppc64 abort()s with pcie bridges

[Greg Kurz]

On Wed, 8 Jul 2020 11:57:03 +0200
Greg Kurz  wrote:

> On Wed, 8 Jul 2020 10:03:47 +0200
> Thomas Huth  wrote:
> 
> > 
> >  Hi,
> > 
> > qemu-system-ppc64 currently abort()s when it is started with a pcie
> > bridge device:
> > 
> > $ qemu-system-ppc64 -M pseries-5.1 -device pcie-pci-bridge
> > Unexpected error in object_property_find() at qom/object.c:1240:
> > qemu-system-ppc64: -device pcie-pci-bridge: Property '.chassis_nr' not found
> > Aborted (core dumped)
> > 
> > or:
> > 
> > $ qemu-system-ppc64 -M pseries -device dec-21154-p2p-bridge
> > Unexpected error in object_property_find() at qom/object.c:1240:
> > qemu-system-ppc64: -device dec-21154-p2p-bridge: Property '.chassis_nr'
> > not found
> > Aborted (core dumped)
> > 
> > That's kind of ugly, and it shows up as error when running
> > scripts/device-crash-test. Is there an easy way to avoid the abort() and
> > fail more gracefully here?
> > 
> 
> And even worse, this can tear down a running guest with hotplug :\
> 
> (qemu) device_add pcie-pci-bridge 
> Unexpected error in object_property_find() at 
> /home/greg/Work/qemu/qemu-ppc/qom/object.c:1240:
> Property '.chassis_nr' not found
> Aborted (core dumped)
> 
> This is caused by recent commit:
> 
> commit 7ef1553dac8ef8dbe547b58d7420461a16be0eeb
> Author: Markus Armbruster 
> Date:   Tue May 5 17:29:25 2020 +0200
> 
> spapr_pci: Drop some dead error handling
> 
> chassis_from_bus() uses object_property_get_uint() to get property
> "chassis_nr" of the bridge device.  Failure would be a programming
> error.  Pass _abort, and simplify its callers.
> 
> Cc: David Gibson 
> Cc: qemu-...@nongnu.org
> Signed-off-by: Markus Armbruster 
> Acked-by: David Gibson 
> Reviewed-by: Greg Kurz 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Paolo Bonzini 
> Message-Id: <20200505152926.18877-18-arm...@redhat.com>
> 
> Before that, we would simply print the "chassir_nr not found" error,
> and in case of a cold plugged device exit.
> 
> The root cause is that the sPAPR PCI code assumes that a PCI bridge
> has a "chassir_nr" property, ie. it is a standard PCI bridge. Other
> PCI bridge types don't have that. Not sure yet why this information
> is required, I'll check LoPAPR.
> 

More on this side : each slot of a PCI bridge is associated a DRC (a
PAPR thingy to handle hot plug/unplug). Each DRC must have a unique
identifier system-wide. We used to use the bus number to compute
the DRC id but it was broken, so we now _hijack_ "chassis_nr" as an
alternative since this commit:

commit 05929a6c5dfe1028ef66250b7bbf11939f8e77cd
Author: David Gibson 
Date:   Wed Apr 10 11:49:28 2019 +1000

spapr: Don't use bus number for building DRC ids

This means that we only support the standard pci-bridge device,
and this relies on the availability of "chassis_nr". Failure
to find this property is then not a programming error, but
an expected case where we want to fail gracefully (ie. revert
Markus's commit mentioned above).

While reading code I realized that we have another problem : the
realization of the pci-bridge device does fail if "chassis_nr" is
zero, but I failed to find a uniqueness check. And we get:

$ qemu-system-ppc64 -device pci-bridge,chassis_nr=1 -device 
pci-bridge,chassis_nr=1
Unexpected error in object_property_try_add() at qom/object.c:1167:
qemu-system-ppc64: -device pci-bridge,chassis_nr=1: attempt to add duplicate 
property '4100' to object (type 'container')
Aborted (core dumped)

It is very confusing to see that we state that "chassis_nr" is unique
several times in slotid_cap_init() but it is never enforced anywhere.

if (!chassis) {
error_setg(errp, "Bridge chassis not specified. Each bridge is required"
   " to be assigned a unique chassis id > 0.");
return -EINVAL;
}

or

/* We make each chassis unique, this way each bridge is First in Chassis */


Michael, Marcel or anyone with PCI knowledge,

Can you shed some light on the semantics of "chassis_nr" ?

> In the meantime, since we're in soft freeze, I guess we should
> revert Markus's patch and add a big fat comment to explain
> what's going on and maybe change the error message to something
> more informative, eg. "PCIE-to-PCI bridges are not supported".
> 
> Thoughts ?
> 
> >  Thomas
> > 
> 

Re: [PATCH] migration: fix memory leak in qmp_migrate_set_parameters

[Juan Quintela]

Chuan Zheng  wrote:
> From: Zheng Chuan 
>
> "tmp.tls_hostname" and "tmp.tls_creds" allocated by 
> migrate_params_test_apply()
> is forgot to free at the end of qmp_migrate_set_parameters(). Fix that.
>
> The leak stack:
> Direct leak of 2 byte(s) in 2 object(s) allocated from:
>#0 0xb597c20b in __interceptor_malloc (/usr/lib64/libasan.so.4+0xd320b)
>#1 0xb52dcb1b in g_malloc (/usr/lib64/libglib-2.0.so.0+0x58b1b)
>#2 0xb52f8143 in g_strdup (/usr/lib64/libglib-2.0.so.0+0x74143)
>#3 0xc52447fb in migrate_params_test_apply 
> (/usr/src/debug/qemu-4.1.0/migration/migration.c:1377)
>#4 0xc52fdca7 in qmp_migrate_set_parameters 
> (/usr/src/debug/qemu-4.1.0/qapi/qapi-commands-migration.c:192)
>#5 0xc551d543 in qmp_dispatch 
> (/usr/src/debug/qemu-4.1.0/qapi/qmp-dispatch.c:165)
>#6 0xc52a0a8f in qmp_dispatch 
> (/usr/src/debug/qemu-4.1.0/monitor/qmp.c:125)
>#7 0xc52a1c7f in monitor_qmp_dispatch 
> (/usr/src/debug/qemu-4.1.0/monitor/qmp.c:214)
>#8 0xc55cb0cf in aio_bh_call 
> (/usr/src/debug/qemu-4.1.0/util/async.c:117)
>#9 0xc55d4543 in aio_bh_poll 
> (/usr/src/debug/qemu-4.1.0/util/aio-posix.c:459)
>#10 0xc55cae0f in aio_dispatch 
> (/usr/src/debug/qemu-4.1.0/util/async.c:268)
>#11 0xb52d6a7b in g_main_context_dispatch 
> (/usr/lib64/libglib-2.0.so.0+0x52a7b)
>#12 0xc55d1e3b(/usr/bin/qemu-kvm-4.1.0+0x1622e3b)
>#13 0xc4e314bb(/usr/bin/qemu-kvm-4.1.0+0xe824bb)
>#14 0xc47f45ef(/usr/bin/qemu-kvm-4.1.0+0x8455ef)
>#15 0xb4bfef3f in __libc_start_main (/usr/lib64/libc.so.6+0x23f3f)
>#16 0xc47ffacb(/usr/bin/qemu-kvm-4.1.0+0x850acb)
>
> Direct leak of 2 byte(s) in 2 object(s) allocated from:
>#0 0xb597c20b in __interceptor_malloc (/usr/lib64/libasan.so.4+0xd320b)
>#1 0xb52dcb1b in g_malloc (/usr/lib64/libglib-2.0.so.0+0x58b1b)
>#2 0xb52f8143 in g_strdup (/usr/lib64/libglib-2.0.so.0+0x74143)
>#3 0xc5244893 in migrate_params_test_apply 
> (/usr/src/debug/qemu-4.1.0/migration/migration.c:1382)
>#4 0xc52fdca7 in qmp_migrate_set_parameters 
> (/usr/src/debug/qemu-4.1.0/qapi/qapi-commands-migration.c:192)
>#5 0xc551d543 in qmp_dispatch 
> (/usr/src/debug/qemu-4.1.0/qapi/qmp-dispatch.c)
>#6 0xc52a0a8f in qmp_dispatch 
> (/usr/src/debug/qemu-4.1.0/monitor/qmp.c:125)
>#7 0xc52a1c7f in monitor_qmp_dispatch 
> (/usr/src/debug/qemu-4.1.0/monitor/qmp.c:214)
>#8 0xc55cb0cf in aio_bh_call 
> (/usr/src/debug/qemu-4.1.0/util/async.c:117)
>#9 0xc55d4543 in aio_bh_poll 
> (/usr/src/debug/qemu-4.1.0/util/aio-posix.c:459)
>#10 0xc55cae0f in in aio_dispatch 
> (/usr/src/debug/qemu-4.1.0/util/async.c:268)
>#11 0xb52d6a7b in g_main_context_dispatch 
> (/usr/lib64/libglib-2.0.so.0+0x52a7b)
>#12 0xc55d1e3b(/usr/bin/qemu-kvm-4.1.0+0x1622e3b)
>#13 0xc4e314bb(/usr/bin/qemu-kvm-4.1.0+0xe824bb)
>#14 0xc47f45ef (/usr/bin/qemu-kvm-4.1.0+0x8455ef)
>#15 0xb4bfef3f in __libc_start_main (/usr/lib64/libc.so.6+0x23f3f)
>#16 0xc47ffacb(/usr/bin/qemu-kvm-4.1.0+0x850acb)
>
> Signed-off-by: Chuan Zheng 
> Reviewed-by: KeQian Zhu 
> Reviewed-by: HaiLiang 

Nice catch.

Reviewed-by: Juan Quintela 

Queued.

> ---
>  migration/migration.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 92e44e0..045180c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1342,12 +1342,12 @@ static void 
> migrate_params_test_apply(MigrateSetParameters *params,
>  
>  if (params->has_tls_creds) {
>  assert(params->tls_creds->type == QTYPE_QSTRING);
> -dest->tls_creds = g_strdup(params->tls_creds->u.s);
> +dest->tls_creds = params->tls_creds->u.s;
>  }
>  
>  if (params->has_tls_hostname) {
>  assert(params->tls_hostname->type == QTYPE_QSTRING);
> -dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
> +dest->tls_hostname = params->tls_hostname->u.s;
>  }
>  
>  if (params->has_max_bandwidth) {

Re: [PATCH] cpu: Add starts_halted() method

[Greg Kurz]

On Thu, 9 Jul 2020 07:11:09 +0200
Philippe Mathieu-Daudé  wrote:

> On 7/8/20 11:39 PM, Eduardo Habkost wrote:
> > On Wed, Jul 08, 2020 at 06:45:57PM +0200, Philippe Mathieu-Daudé wrote:
> >> On 7/8/20 5:25 PM, Eduardo Habkost wrote:
> >>> On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
>  On Wed, 8 Jul 2020 at 12:12, David Gibson  
>  wrote:
> > On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé 
> > wrote:
> >> Class boolean field certainly sounds better, but I am not sure this
> >> is a property of the machine. Rather the arch? So move the field
> >> to CPUClass? Maybe not, let's discuss :)
> >
> > It is absolutely a property of the machine.  e.g. I don't think we
> > want this for powernv.  pseries is a bit of a special case since it is
> > explicitly a paravirt platform.  But even for emulated hardware, the
> > board can absolutely strap things so that cpus do or don't start
> > immediately.
> 
>  It's a property of the individual CPU, I think. One common setup
>  for Arm systems is that the primary CPU starts powered up but
>  the secondaries all start powered down.
> >>>
> >>> Both statements can be true.  It can be a property of the
> >>> individual CPU (although I'm not convinced it has to), but it
> >>> still needs to be controlled by the machine.
> >>
> >> From what said Peter, I understand this is a property of the
> >> chipset. Chipsets are modelled unevenly.
> >>
> >> IIUC QEMU started with single-core CPUs.
> >> CPUState had same meaning for 'core' or 'cpu', 1-1 mapping.
> >>
> >> Then multicore CPUs could be easily modelled using multiple
> >> single-core CPUs, usually created in the machine code.
> >>
> >> Then we moved to SoC models, creating the cores in the SoC.
> >> Some SoCs have array of cores, eventually heterogeneous
> >> (see the ZynqMP). We have containers of CPUState.
> >>
> >> On an ARM-based SoC, you might have the first core started
> >> (as said Peter) or not.
> >>
> >> BCM2836 / BCM2837 and ZynqMP start will all ARM cores off.
> >> On the BCM chipsets, a DSP core will boot the ARM cores.
> >> On the ZynqMP, a MicroBlaze core boots them.
> >> As QEMU doesn't models heterogeneous architectures, we start
> >> modelling after the unmodelled cores booted us, so either one
> >> or all cores on.
> >>
> >> In this case, we narrowed down the 'start-powered-off' field
> >> to the SoC, which happens to be how ARM SoCs are modelled.
> > 
> > I was not aware of the start-powered-off property.  If we make it
> > generic, we can just let spapr use it.
> > 
> >>
> >>
> >> Chipsets providing a JTAG interface can have a SRST signal,
> >> the "system reset". When a JTAG probe is attached, it can
> >> keeps the whole chipset in a reset state. This is equivalent
> >> to QEMU '-S' mode (single step mode).
> >>
> >>
> >> I don't know about pseries hardware, but if this is 'explicit
> >> to paravirt platform', then I expect this to be the same with
> >> other accelerators/architectures.
> >>
> >> If paravirtualized -> cores start off by default. Let the
> >> hypervisor start them. So still a property of the CPUState
> >> depending on the accelerator used?
> > 
> > I don't understand this part.  Why would this depend on the
> > accelerator?
> 
> Because starting a virtualized machine with all cores powered-off
> with TCG accelerator should at least emit a warning? Or change
> the behavior and start them powered-on? This is machine-specific
> although.
> 

FYI, PAPR requires all vCPUs to be "stopped" by default. It is up to the
guest to start them explicitly through an RTAS call. The hypervisor is
only responsible to start a single vCPU (see spapr_cpu_set_entry_state()
called from spapr_machine_reset()) to be able to boot the guest.

So I'm not sure to see how that would depend on the accelerator...

> 

Re: Migrating custom qemu.org infrastructure to GitLab

[Gerd Hoffmann]

  Hi,

> > 2. wiki.qemu.org is a MediaWiki instance. Account creation is a hurdle
> > to one-time or new contributors. It is unclear whether GitLab's wiki
> > is expressive enough for a lossless conversion of the existing QEMU
> > wiki. Any volunteers interested in evaluating the wiki migration would
> > be appreciated.
> 
> Yeah, this is a potentially big piece of work. We didn't finish this
> in libvirt either. Looking at the libvirt mediawiki though, I decided
> not todo a straight export/import of all content.

FYI: gitlab wiki is basically just a git repo with markdown pages +
renderer + gui editor.  You can also update the wiki using git clone +
edit + git commit + git push.

take care,
  Gerd

Re: [PATCH v2 2/2] GitLab Gating CI: initial set of jobs, documentation and scripts

[Daniel P . Berrangé]

On Wed, Jul 08, 2020 at 10:46:57PM -0400, Cleber Rosa wrote:
> This is a mapping of Peter's "remake-merge-builds" and
> "pull-buildtest" scripts, gone through some updates, adding some build
> option and removing others.
> 
> The jobs currently cover the machines that the QEMU project owns, and that
> are setup and ready to run jobs:
> 
>  - Ubuntu 18.04 on S390x
>  - Ubuntu 20.04 on aarch64
> 
> During the development of this set of jobs, the GitLab CI was tested
> with many other architectures, including ppc64, s390x and aarch64,
> along with the other OSs (not included here):
> 
>  - Fedora 30
>  - FreeBSD 12.1
> 
> More information can be found in the documentation itself.
> 
> Signed-off-by: Cleber Rosa 
> ---
>  .gitlab-ci.d/gating.yml| 146 +

AFAIK, the jobs in this file just augment what is already defined
in the main .gitlab-ci.yml. Also since we're providing setup info
for other people to configure custom runners, these jobs are usable
for non-gating CI scenarios too.

IOW, the jobs in this file happen to be usable for gating, but they
are not the only gating jobs, and can be used for non-gating reasons.

This is a complicated way of saying that gating.yml is not a desirable
filename, so I'd suggest splitting it in two and having these files
named based on what their contents is, rather than their use case:

   .gitlab-ci.d/runners-s390x.yml
   .gitlab-ci.d/runners-aarch64.yml

The existing jobs in .gitlab-ci.yml could possibly be moved into
a .gitlab-ci.d/runners-shared.yml file for consistency.

>  .gitlab-ci.yml |   1 +
>  docs/devel/testing.rst | 147 +
>  scripts/ci/setup/build-environment.yml | 217 +
>  scripts/ci/setup/gitlab-runner.yml |  72 
>  scripts/ci/setup/inventory |   2 +
>  scripts/ci/setup/vars.yml  |  13 ++
>  7 files changed, 598 insertions(+)
>  create mode 100644 .gitlab-ci.d/gating.yml
>  create mode 100644 scripts/ci/setup/build-environment.yml
>  create mode 100644 scripts/ci/setup/gitlab-runner.yml
>  create mode 100644 scripts/ci/setup/inventory
>  create mode 100644 scripts/ci/setup/vars.yml
> 
> diff --git a/.gitlab-ci.d/gating.yml b/.gitlab-ci.d/gating.yml
> new file mode 100644
> index 00..5562df5708
> --- /dev/null
> +++ b/.gitlab-ci.d/gating.yml
> @@ -0,0 +1,146 @@
> +variables:
> +  GIT_SUBMODULE_STRATEGY: recursive
> +
> +# All ubuntu-18.04 jobs should run successfully in an environment
> +# setup by the scripts/ci/setup/build-environment.yml task
> +# "Install basic packages to build QEMU on Ubuntu 18.04/20.04"
> +ubuntu-18.04-s390x-all-linux-static:
> + tags:
> + - ubuntu_18.04
> + - s390x
> + rules:
> + - if: '$CI_COMMIT_REF_NAME == "staging"'

I think I'd make it more flexible in particular to allow multiple
branches. For example I have multiple subsystems and have separate
branches for each.

This could be as simple as allowing a regex prefix

  - if: '$CI_COMMIT_REF_NAME =~ /^staging/'




> diff --git a/scripts/ci/setup/build-environment.yml 
> b/scripts/ci/setup/build-environment.yml
> new file mode 100644
> index 00..89b35386c7
> --- /dev/null
> +++ b/scripts/ci/setup/build-environment.yml
> @@ -0,0 +1,217 @@
> +---
> +- name: Installation of basic packages to build QEMU
> +  hosts: all
> +  vars_files:
> +- vars.yml
> +  tasks:
> +- name: Install basic packages to build QEMU on Ubuntu 18.04/20.04
> +  apt:
> +update_cache: yes
> +# This matches the packages on 
> tests/docker/Dockerfiles/ubuntu1804.docker

I'd be inclined to actually use docker on the custom runners.

eg. instead of having separate physical machines or VMs for each
(distro, arch) pair, have a single host distro for the arch. Then
use docker to provide the build environment against each distro.

IOW, a RHEL-8 aarch64 host, running docker for ubuntu18.04, fedora30
etc.

That way we don't end up duplicating all these packages, and instead
can use  tests/docker/Dockerfiles/ubuntu1804.docker.  This ensures
that if a user needs to reproduce a build failure on their own local
aarch64 machine, they can run docker and get the exact same build
architecture.

It also has the benefit that we don't need to worry about how to
setup gitlab runners for every distro we care about. We only need to
do gitlab runner for the standard host distro, which spawns a pristine
throwaway docker env.

I appreciate this is a big change from what you've done in this patch
though, so don't consider this comment a blocker for initial merge.
I think we should do this as the long term strategy though. Essentially
for Linux builds, everything should always be container based.

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

Re: [PULL v2 00/41] virtio,acpi: features, fixes, cleanups.

[Peter Maydell]

On Tue, 7 Jul 2020 at 18:50, Peter Maydell  wrote:
>
> On Tue, 7 Jul 2020 at 13:04, Michael S. Tsirkin  wrote:
> > Precisely. Sorry about missing this.
> > I made this change and pushed to the same tag - don't want to spam
> > the list for a small thing like this. Can you pick this up pls?
> > Commit 849c48004df0e123b53fe9888770cb4f6eb5e8ab now
>
> Sure. (You can always just resend a new v2 cover letter without
> all the patches; that's what most people do for minor respins.)
>
> Applied, thanks.

I've just noticed that the commit that got merged was not the
one you quote but 1e0a84ea49b68b7cf60e -- can you check whether
anything was missed or the wrong version ?

thanks
-- PMM

Re: [PATCH v4 05/12] hw/arm: Add NPCM730 and NPCM750 SoC models

[Havard Skinnemoen]

On Wed, Jul 8, 2020 at 10:34 PM Philippe Mathieu-Daudé  wrote:
>
> On 7/9/20 2:06 AM, Havard Skinnemoen wrote:
> > On Wed, Jul 8, 2020 at 11:13 AM Havard Skinnemoen
> >  wrote:
> >> On Wed, Jul 8, 2020 at 10:31 AM Philippe Mathieu-Daudé  
> >> wrote:
> >>> On 7/7/20 8:47 PM, Havard Skinnemoen wrote:
>  +typedef struct NPCM7xxClass {
>  +DeviceClass parent;
> >>>
> >>> Similar comment that elsewhere on this series, if NPCM7xxClass not used
> >>> outside of npcm7xx.c, keep it local.
> >>
> >> OK, will do.
> >
> > Turns out it is used in npcm7xx_boards.c, so it has to stay where it is.
>
> Indeed:
>
> static void npcm7xx_load_kernel(MachineState *machine,
> NPCM7xxState *soc)
> {
> NPCM7xxClass *sc = NPCM7XX_GET_CLASS(soc);
>
> npcm7xx_binfo.ram_size = machine->ram_size;
> npcm7xx_binfo.nb_cpus = sc->num_cpus;
>
> arm_load_kernel(>cpu[0], machine, _binfo);
> }
>
> This is fine.

It's also used here:

static void npcm7xx_set_soc_type(NPCM7xxMachineClass *nmc, const char *type)
{
NPCM7xxClass *sc = NPCM7XX_CLASS(object_class_by_name(type));
MachineClass *mc = MACHINE_CLASS(nmc);

nmc->soc_type = type;
mc->default_cpus = mc->min_cpus = mc->max_cpus = sc->num_cpus;
}

>
> Just thinking loudly, we traditionally add the load_kernel() code
> in the machine, because it is often specific to Linux guest, and
> the SoC doesn't need to know about the guest OS.
>
> hw/arm/boot.c contains helpers also useful for firmwares.
>
> The SoC has a link to the DRAM so can get its size.
> All the arm_boot_info fields are specific to this SoC.
> So we could move a lot of code to npcm7xx.c, only declaring:
>
>   void npcm7xx_load_kernel(MachineState *machine,
>NPCM7xxState *soc);

I can do that, but it doesn't completely get rid of all references to
NPCM7xxClass. I'm afraid there will always be some amount of coupling
between the machines and corresponding SoCs.

Havard

[PATCH 2/2] linux-user: add netlink RTM_SETLINK command

[Laurent Vivier]

This command is needed to be able to boot systemd in a container.

  $ sudo systemd-nspawn -D /chroot/armhf/sid/ -b
  Spawning container sid on /chroot/armhf/sid.
  Press ^] three times within 1s to kill container.
  systemd 245.6-2 running in system mode.
  Detected virtualization systemd-nspawn.
  Detected architecture arm.

  Welcome to Debian GNU/Linux bullseye/sid!

  Set hostname to .
  Failed to enqueue loopback interface start request: Operation not supported
  Caught , dumped core as pid 3.
  Exiting PID 1...
  Container sid failed with error code 255.

Signed-off-by: Laurent Vivier 
---
 linux-user/fd-trans.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
index 5d49a53552b2..1486c81aaa27 100644
--- a/linux-user/fd-trans.c
+++ b/linux-user/fd-trans.c
@@ -1204,6 +1204,7 @@ static abi_long target_to_host_data_route(struct nlmsghdr 
*nlh)
 break;
 case RTM_NEWLINK:
 case RTM_DELLINK:
+case RTM_SETLINK:
 if (nlh->nlmsg_len >= NLMSG_LENGTH(sizeof(*ifi))) {
 ifi = NLMSG_DATA(nlh);
 ifi->ifi_type = tswap16(ifi->ifi_type);
-- 
2.26.2

[PATCH 1/2] linux-user: add new netlink types

[Laurent Vivier]

Only implement IFLA_PERM_ADDRESS to fix the following error:

  Unknown host QEMU_IFLA type: 54

The couple of other ones, IFLA_PROP_LIST and IFLA_ALT_IFNAME, have
been introduced to be used with RTM_NEWLINKPROP, RTM_DELLINKPROP and
RTM_GETLINKPROP that are not implemented by QEMU.

Signed-off-by: Laurent Vivier 
---
 linux-user/fd-trans.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
index c0687c52e62b..5d49a53552b2 100644
--- a/linux-user/fd-trans.c
+++ b/linux-user/fd-trans.c
@@ -133,6 +133,9 @@ enum {
 QEMU_IFLA_NEW_IFINDEX,
 QEMU_IFLA_MIN_MTU,
 QEMU_IFLA_MAX_MTU,
+QEMU_IFLA_PROP_LIST,
+QEMU_IFLA_ALT_IFNAME,
+QEMU_IFLA_PERM_ADDRESS,
 QEMU___IFLA_MAX
 };
 
@@ -807,6 +810,7 @@ static abi_long host_to_target_data_link_rtattr(struct 
rtattr *rtattr)
 /* binary stream */
 case QEMU_IFLA_ADDRESS:
 case QEMU_IFLA_BROADCAST:
+case QEMU_IFLA_PERM_ADDRESS:
 /* string */
 case QEMU_IFLA_IFNAME:
 case QEMU_IFLA_QDISC:
-- 
2.26.2

Re: [PATCH v7 00/47] block: Deal with filters

[Max Reitz]

On 08.07.20 22:47, Eric Blake wrote:
> On 7/8/20 12:20 PM, Andrey Shinkevich wrote:
>> On 25.06.2020 18:21, Max Reitz wrote:
>>> v6:
>>> https://lists.nongnu.org/archive/html/qemu-devel/2019-08/msg01715.html
>>>
>>> Branch: https://github.com/XanClic/qemu.git child-access-functions-v7
>>> Branch: https://git.xanclic.moe/XanClic/qemu.git
>>> child-access-functions-v7
>>>
>>>
>> I cloned the branch from the github and built successfully.
>>
>> Running the iotests reports multiple errors of such a kind:
>>
>> 128: readarray -td '' formatting_line < <(sed -e 's/, fmt=/\x0/')
>>
>> "./common.filter: line 128: readarray: -d: invalid option"
>>
>> introduced with the commit
>>
>> a7399eb iotests: Make _filter_img_create more active
> 
> You appear to be staging off an unreleased preliminary tree.  a7399eb is
> not upstream; the upstream commit 'iotests: Make _filter_img_create more
> active' is commit 57ee95ed, and while it uses readarray, it does not use
> the problematic -d.  In other words, it looks like the problem was
> caught and fixed in between the original patch creation and the pull
> request.

Ah, sorry, my mail client’s threading layout hid this mail from me a bit.

Yes.  Well, no, it wasn’t fixed before the pull request, but it was
fixed in the second pull request.  But yes.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v7 00/47] block: Deal with filters

[Max Reitz]

On 08.07.20 22:37, Eric Blake wrote:
> On 7/8/20 2:46 PM, Andrey Shinkevich wrote:
>>
>> On 08.07.2020 20:32, Eric Blake wrote:
>>> On 7/8/20 12:20 PM, Andrey Shinkevich wrote:
 On 25.06.2020 18:21, Max Reitz wrote:
> v6:
> https://lists.nongnu.org/archive/html/qemu-devel/2019-08/msg01715.html
>
> Branch: https://github.com/XanClic/qemu.git child-access-functions-v7
> Branch: https://git.xanclic.moe/XanClic/qemu.git
> child-access-functions-v7
>
>
 I cloned the branch from the github and built successfully.

 Running the iotests reports multiple errors of such a kind:

 128: readarray -td '' formatting_line < <(sed -e 's/, fmt=/\x0/')

 "./common.filter: line 128: readarray: -d: invalid option"

>>>
>>> Arrgh. If I'm reading bash's changelog correctly, readarray -d was
>>> introduced in bash 4.4, so I'm guessing you're still on 4.3 or
>>> earlier? What bash version and platform are you using?
>>>
>> My bash version is 4.2.46.
>>
>> It is the latest in the virtuozzolinux-base repository. I should
>> install the 4.4 package manually.
> 
> Well, if bash 4.2 is the default installed version on any of our
> platforms that meet our supported criteria, then we should instead fix
> the patch in question to avoid non-portable use of readarray.
> 
> Per https://repology.org/project/bash/versions (hinted from
> docs/system/build-platforms.rst), at least CentOS 7 still ships bash
> 4.2, and per 'make docker', centos7 is still a viable build target.  So
> we do indeed need to fix our regression.

There is no regression.  It’s just that I based this series on an
earlier version of “Make _filter_img_create more active” – when I sent a
pull request for that version, Peter already reported to me that it
failed on some test environments, so I revised it.

You’ll find there is no a7399eb in master; it’s 57ee95ed4ee there and
doesn’t use -d.

(My branch on github/gitea is still based on that older version, though,
because that’s what I wrote it on.)

Max



signature.asc
Description: OpenPGP digital signature

[PATCH] tests/qtest/fuzz: Add missing spaces in description

[Thomas Huth]

There should be a space between "forking" and "for".

Signed-off-by: Thomas Huth 
---
 tests/qtest/fuzz/virtio_scsi_fuzz.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/fuzz/virtio_scsi_fuzz.c 
b/tests/qtest/fuzz/virtio_scsi_fuzz.c
index 51dce491ab..3a9ea13736 100644
--- a/tests/qtest/fuzz/virtio_scsi_fuzz.c
+++ b/tests/qtest/fuzz/virtio_scsi_fuzz.c
@@ -191,7 +191,7 @@ static void register_virtio_scsi_fuzz_targets(void)
 {
 fuzz_add_qos_target(&(FuzzTarget){
 .name = "virtio-scsi-fuzz",
-.description = "Fuzz the virtio-scsi virtual queues, forking"
+.description = "Fuzz the virtio-scsi virtual queues, forking "
 "for each fuzz run",
 .pre_vm_init = _shm_init,
 .pre_fuzz = _scsi_pre_fuzz,
@@ -202,7 +202,7 @@ static void register_virtio_scsi_fuzz_targets(void)
 
 fuzz_add_qos_target(&(FuzzTarget){
 .name = "virtio-scsi-flags-fuzz",
-.description = "Fuzz the virtio-scsi virtual queues, forking"
+.description = "Fuzz the virtio-scsi virtual queues, forking "
 "for each fuzz run (also fuzzes the virtio flags)",
 .pre_vm_init = _shm_init,
 .pre_fuzz = _scsi_pre_fuzz,
-- 
2.18.1

Re: [PATCH v7 02/47] block: Add chain helper functions

[Andrey Shinkevich]

On 09.07.2020 11:24, Max Reitz wrote:

On 08.07.20 19:20, Andrey Shinkevich wrote:

On 25.06.2020 18:21, Max Reitz wrote:

Add some helper functions for skipping filters in a chain of block
nodes.

Signed-off-by: Max Reitz 
---
   include/block/block_int.h |  3 +++
   block.c   | 55 +++
   2 files changed, 58 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index bb3457c5e8..5da793bfc3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1382,6 +1382,9 @@ BdrvChild *bdrv_cow_child(BlockDriverState *bs);
   BdrvChild *bdrv_filter_child(BlockDriverState *bs);
   BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs);
   BdrvChild *bdrv_primary_child(BlockDriverState *bs);
+BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs);
+BlockDriverState *bdrv_skip_filters(BlockDriverState *bs);
+BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs);
     static inline BlockDriverState *child_bs(BdrvChild *child)
   {
diff --git a/block.c b/block.c
index 5a42ef49fd..0a0b855261 100644
--- a/block.c
+++ b/block.c
@@ -7008,3 +7008,58 @@ BdrvChild *bdrv_primary_child(BlockDriverState
*bs)
     return NULL;
   }
+
+static BlockDriverState *bdrv_do_skip_filters(BlockDriverState *bs,
+  bool
stop_on_explicit_filter)
+{
+    BdrvChild *c;
+
+    if (!bs) {
+    return NULL;
+    }
+
+    while (!(stop_on_explicit_filter && !bs->implicit)) {
+    c = bdrv_filter_child(bs);
+    if (!c) {
+    break;
+    }
+    bs = c->bs;

Could it be child_bs(bs) ?

Well, in a sense, but not really.  We need to check whether there is a
child before overwriting @bs (because @bs must stay a non-NULL pointer),
so we wouldn’t have fewer lines of code if we replaced “BdrvChild *c” by
“BlockDriverState *child_bs”, and then used bdrv_child() to set child_bs.

(And because we have to check whether @c is NULL anyway, there is no
real reason to use child_bs(c) instead of c->bs afterwards.)


Got it, thanks.

Andrey


+    }

Reviewed-by: Andrey Shinkevich 

Thanks a lot for reviewing!


Pleasure!

Andrey

Re: [PATCH v2 2/7] Implement drain_call_rcu and use it in hmp_device_del

[Maxim Levitsky]

On Wed, 2020-05-27 at 14:11 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:46PM +0300, Maxim Levitsky wrote:
> >  /* The operands of the minus operator must have the same type,
> >   * which must be the one that we specify in the cast.
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 56cee1483f..70877840a2 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, 
> > Error **errp)
> >  return;
> >  }
> >  dev = qdev_device_add(opts, _err);
> > +drain_call_rcu();
> 
> Please include comments explaining what each drain waits for. Without
> comments we'll quickly lose track of why drain_call_rcu() calls are
> necessary (similar to documenting memory barrier or refcount inc/dec
> pairing).
> 
> > diff --git a/util/rcu.c b/util/rcu.c
> > index 60a37f72c3..e8b1c4d6c5 100644
> > --- a/util/rcu.c
> > +++ b/util/rcu.c
> > @@ -293,6 +293,39 @@ void call_rcu1(struct rcu_head *node, void 
> > (*func)(struct rcu_head *node))
> >  qemu_event_set(_call_ready_event);
> >  }
> >  
> > +
> > +struct rcu_drain {
> > +struct rcu_head rcu;
> > +QemuEvent drain_complete_event;
> > +};
> > +
> > +static void drain_rcu_callback(struct rcu_head *node)
> > +{
> > +struct rcu_drain *event = (struct rcu_drain *)node;
> > +qemu_event_set(>drain_complete_event);
> 
> A comment would be nice explaining that callbacks are invoked in
> sequence so we're sure that all previously scheduled callbacks have
> completed when drain_rcu_callback() is invoked.
> 
> > +}
> > +
> > +void drain_call_rcu(void)
> 
> Please document that the main loop mutex is dropped if it's held. This
> will prevent surprises and allow callers to think about thread-safety
> across this call.

Done.
> 
> Aside from the comment requests:
> Reviewed-by: Stefan Hajnoczi 


Best regards,
Maxim levitsky

Re: [PATCH v2 2/2] GitLab Gating CI: initial set of jobs, documentation and scripts

[Philippe Mathieu-Daudé]

On 7/9/20 4:46 AM, Cleber Rosa wrote:
> This is a mapping of Peter's "remake-merge-builds" and
> "pull-buildtest" scripts, gone through some updates, adding some build
> option and removing others.
> 
> The jobs currently cover the machines that the QEMU project owns, and that
> are setup and ready to run jobs:
> 
>  - Ubuntu 18.04 on S390x
>  - Ubuntu 20.04 on aarch64
> 
> During the development of this set of jobs, the GitLab CI was tested
> with many other architectures, including ppc64, s390x and aarch64,
> along with the other OSs (not included here):
> 
>  - Fedora 30
>  - FreeBSD 12.1
> 
> More information can be found in the documentation itself.
> 
> Signed-off-by: Cleber Rosa 
> ---
>  .gitlab-ci.d/gating.yml| 146 +
>  .gitlab-ci.yml |   1 +
>  docs/devel/testing.rst | 147 +

Time to consider moving the CI doc in a separate file...

>  scripts/ci/setup/build-environment.yml | 217 +
>  scripts/ci/setup/gitlab-runner.yml |  72 
>  scripts/ci/setup/inventory |   2 +
>  scripts/ci/setup/vars.yml  |  13 ++

Should we name these last two as inventory.template
and vars.yml.template?

Maybe you can add them with gitlab-runner.yml and a part of
the documentation in a first patch,

Then build-environment.yml and another part of the doc
in another patch,

Finally gating.yml and the related missing doc as the
last patch?

>  7 files changed, 598 insertions(+)
>  create mode 100644 .gitlab-ci.d/gating.yml
>  create mode 100644 scripts/ci/setup/build-environment.yml
>  create mode 100644 scripts/ci/setup/gitlab-runner.yml
>  create mode 100644 scripts/ci/setup/inventory
>  create mode 100644 scripts/ci/setup/vars.yml
[...]

[PATCH v2 2/2] hw/riscv: sifive_u: Provide a reliable way for bootloader to detect whether it is running in QEMU

[Bin Meng]

From: Bin Meng 

The reset vector codes are subject to change, e.g.: with recent
fw_dynamic type image support, it breaks oreboot again.

Add a subregion in the MROM, with the size of machine RAM stored,
so that we can provide a reliable way for bootloader to detect
whether it is running in QEMU.

Signed-off-by: Bin Meng 

---

Changes in v2:
- correctly populate the value in little-endian

 hw/riscv/sifive_u.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 3413369..79519d4 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -88,6 +88,7 @@ static const struct MemmapEntry {
 
 #define OTP_SERIAL  1
 #define GEM_REVISION0x10070109
+#define MROM_RAMSIZE_OFFSET 0xf8
 
 static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap,
 uint64_t mem_size, const char *cmdline)
@@ -496,6 +497,26 @@ static void sifive_u_machine_init(MachineState *machine)
 riscv_rom_copy_firmware_info(memmap[SIFIVE_U_MROM].base,
  memmap[SIFIVE_U_MROM].size,
  sizeof(reset_vec), kernel_entry);
+
+/*
+ * Tell guest the machine ram size at MROM_RAMSIZE_OFFSET.
+ * On real hardware, the 64-bit value from MROM_RAMSIZE_OFFSET is zero.
+ * QEMU aware bootloader (e.g.: oreboot, U-Boot) can check value stored
+ * here to determine whether it is running in QEMU.
+ */
+
+uint32_t ram_size[2] = {
+machine->ram_size,
+((uint64_t)(machine->ram_size)) >> 32
+};
+
+/* copy in the ram size in little_endian byte order */
+for (i = 0; i < ARRAY_SIZE(ram_size); i++) {
+ram_size[i] = cpu_to_le32(ram_size[i]);
+}
+rom_add_blob_fixed_as("mrom.ram_size", ram_size, sizeof(ram_size),
+  memmap[SIFIVE_U_MROM].base + MROM_RAMSIZE_OFFSET,
+  _space_memory);
 }
 
 static bool sifive_u_machine_get_start_in_flash(Object *obj, Error **errp)
-- 
2.7.4

Re: [PATCH RFC 2/5] s390x: implement diag260

[Christian Borntraeger]

On 08.07.20 20:51, David Hildenbrand wrote:
> Let's implement the "storage configuration" part of diag260. This diag
> is found under z/VM, to indicate usable chunks of memory tot he guest OS.
> As I don't have access to documentation, I have no clue what the actual
> error cases are, and which other stuff we could eventually query using this
> interface. Somebody with access to documentation should fix this. This
> implementation seems to work with Linux guests just fine.
> 
> The Linux kernel supports diag260 to query the available memory since
> v4.20. Older kernels / kvm-unit-tests will later fail to run in such a VM
> (with maxmem being defined and bigger than the memory size, e.g., "-m
>  2G,maxmem=4G"), just as if support for SCLP storage information is not
> implemented. They will fail to detect the actual initial memory size.
> 
> This interface allows us to expose the maximum ramsize via sclp
> and the initial ramsize via diag260 - without having to mess with the
> memory increment size and having to align the initial memory size to it.
> 
> This is a preparation for memory device support. We'll unlock the
> implementation with a new QEMU machine that supports memory devices.
> 
> Signed-off-by: David Hildenbrand 

I have not looked into this, so this is purely a question. 

Is there a way to hotplug virtio-mem memory beyond the initial size of 
the memory as specified by the  initial sclp)? then we could avoid doing
this platform specfic diag260?
the only issue I see is when we need to go beyond 4TB due to the page table
upgrade in the kernel. 

FWIW diag 260 is publicly documented. 

Re: [PATCH] Remove the CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE switch

[Paolo Bonzini]

On 09/07/20 07:34, Thomas Huth wrote:
> GCC supports "#pragma GCC diagnostic" since version 4.6, and
> Clang seems to support it, too, since its early versions 3.x.
> That means that our minimum required compiler versions all support
> this pragma already and we can remove the test from configure and
> all the related #ifdefs in the code.
> 
> Signed-off-by: Thomas Huth 
> ---
>  configure | 29 -
>  include/ui/gtk.h  |  4 
>  include/ui/qemu-pixman.h  |  4 
>  scripts/decodetree.py | 12 
>  ui/gtk.c  |  4 
>  util/coroutine-ucontext.c |  4 
>  6 files changed, 4 insertions(+), 53 deletions(-)

Cc: qemu-triv...@nongnu.org

Looks good, thanks!

Paolo

> diff --git a/configure b/configure
> index ee6c3c6792..fbf119bbc0 100755
> --- a/configure
> +++ b/configure
> @@ -5703,31 +5703,6 @@ if compile_prog "" "" ; then
>  linux_magic_h=yes
>  fi
>  
> -
> -# check whether we can disable warning option with a pragma (this is needed
> -# to silence warnings in the headers of some versions of external libraries).
> -# This test has to be compiled with -Werror as otherwise an unknown pragma is
> -# only a warning.
> -#
> -# If we can't selectively disable warning in the code, disable -Werror so 
> that
> -# the build doesn't fail anyway.
> -
> -pragma_disable_unused_but_set=no
> -cat > $TMPC << EOF
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wstrict-prototypes"
> -#pragma GCC diagnostic pop
> -
> -int main(void) {
> -return 0;
> -}
> -EOF
> -if compile_prog "-Werror" "" ; then
> -pragma_diagnostic_available=yes
> -else
> -werror=no
> -fi
> -
>  
>  # check if we have valgrind/valgrind.h
>  
> @@ -7661,10 +7636,6 @@ if test "$linux_magic_h" = "yes" ; then
>echo "CONFIG_LINUX_MAGIC_H=y" >> $config_host_mak
>  fi
>  
> -if test "$pragma_diagnostic_available" = "yes" ; then
> -  echo "CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE=y" >> $config_host_mak
> -fi
> -
>  if test "$valgrind_h" = "yes" ; then
>echo "CONFIG_VALGRIND_H=y" >> $config_host_mak
>  fi
> diff --git a/include/ui/gtk.h b/include/ui/gtk.h
> index d1b230848a..eaeb450f91 100644
> --- a/include/ui/gtk.h
> +++ b/include/ui/gtk.h
> @@ -1,15 +1,11 @@
>  #ifndef UI_GTK_H
>  #define UI_GTK_H
>  
> -#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
>  /* Work around an -Wstrict-prototypes warning in GTK headers */
>  #pragma GCC diagnostic push
>  #pragma GCC diagnostic ignored "-Wstrict-prototypes"
> -#endif
>  #include 
> -#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
>  #pragma GCC diagnostic pop
> -#endif
>  
>  #include 
>  
> diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
> index 3b7cf70157..87737a6f16 100644
> --- a/include/ui/qemu-pixman.h
> +++ b/include/ui/qemu-pixman.h
> @@ -7,14 +7,10 @@
>  #define QEMU_PIXMAN_H
>  
>  /* pixman-0.16.0 headers have a redundant declaration */
> -#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
>  #pragma GCC diagnostic push
>  #pragma GCC diagnostic ignored "-Wredundant-decls"
> -#endif
>  #include 
> -#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
>  #pragma GCC diagnostic pop
> -#endif
>  
>  /*
>   * pixman image formats are defined to be native endian,
> diff --git a/scripts/decodetree.py b/scripts/decodetree.py
> index 530d41ca62..694757b6c2 100755
> --- a/scripts/decodetree.py
> +++ b/scripts/decodetree.py
> @@ -1327,12 +1327,10 @@ def main():
>  # but we can't tell which ones.  Prevent issues from the compiler by
>  # suppressing redundant declaration warnings.
>  if anyextern:
> -output("#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE\n",
> -   "# pragma GCC diagnostic push\n",
> -   "# pragma GCC diagnostic ignored \"-Wredundant-decls\"\n",
> -   "# ifdef __clang__\n"
> +output("#pragma GCC diagnostic push\n",
> +   "#pragma GCC diagnostic ignored \"-Wredundant-decls\"\n",
> +   "#ifdef __clang__\n"
> "#  pragma GCC diagnostic ignored 
> \"-Wtypedef-redefinition\"\n",
> -   "# endif\n",
> "#endif\n\n")
>  
>  out_pats = {}
> @@ -1347,9 +1345,7 @@ def main():
>  output('\n')
>  
>  if anyextern:
> -output("#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE\n",
> -   "# pragma GCC diagnostic pop\n",
> -   "#endif\n\n")
> +output("#pragma GCC diagnostic pop\n\n")
>  
>  for n in sorted(formats.keys()):
>  f = formats[n]
> diff --git a/ui/gtk.c b/ui/gtk.c
> index d4b49bd7da..b0cc08ad6d 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -1996,14 +1996,10 @@ static GSList *gd_vc_gfx_init(GtkDisplayState *s, 
> VirtualConsole *vc,
>   * proper replacement (native opengl support) is only
>   * available in 3.16+.  Silence the warning if possible.
>   */
> -#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
>  

Re: [PULL 00/53] Misc patches for QEMU 5.1 soft freeze

[Paolo Bonzini]

On 09/07/20 08:59, Claudio Fontana wrote:
> anything else that we could use to find the real problem?

I'm using

$ gcc -v
...
gcc version 8.3.1 20191121 (Red Hat 8.3.1-5) (GCC)
$ ld -v
GNU ld version 2.30-68.el8

> Of course I can try a blind fix, where I suggest to explicitly provide the 
> stubs,
> or you can apply the workaround you already suggested if you want,
> but currently I do not have any way to ensure that what I build is ok,
> since apparently the local build or any of the CI (travis, cirrus) is not 
> sufficient to capture this.

No problem, I can test it myself on my machine when applying the patch.

Paolo

Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it

[Mohammed Gamal]

On Thu, 2020-07-09 at 11:44 +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > (CCing libvir-list, and people who were included in the OVMF
> > > thread[1])
> > > 
> > > [1] 
> > > https://lore.kernel.org/qemu-devel/99779e9c-f05f-501b-b4be-ff719f140...@canonical.com/
> > > Also, it's important that we work with libvirt and management
> > > software to ensure they have appropriate APIs to choose what to
> > > do when a cluster has hosts with different MAXPHYADDR.
> > 
> > There's been so many complex discussions that it is hard to have
> > any
> > understanding of what we should be doing going forward. There's
> > enough
> > problems wrt phys bits, that I think we would benefit from a doc
> > that
> > outlines the big picture expectation for how to handle this in the
> > virt stack.
> 
> Well, the fundamental issue is not that hard actually.  We have three
> cases:
> 
> (1) GUEST_MAXPHYADDR == HOST_MAXPHYADDR
> 
> Everything is fine ;)
> 
> (2) GUEST_MAXPHYADDR < HOST_MAXPHYADDR
> 
> Mostly fine.  Some edge cases, like different page fault errors
> for
> addresses above GUEST_MAXPHYADDR and below
> HOST_MAXPHYADDR.  Which I
> think Mohammed fixed in the kernel recently.
> 
> (3) GUEST_MAXPHYADDR > HOST_MAXPHYADDR
> 
> Broken.  If the guest uses addresses above HOST_MAXPHYADDR
> everything
> goes south.
> 
> The (2) case isn't much of a problem.  We only need to figure
> whenever
> we want qemu allow this unconditionally (current state) or only in
> case
> the kernel fixes are present (state with this patch applied if I read
> it
> correctly).
> 
> The (3) case is the reason why guest firmware never ever uses
> GUEST_MAXPHYADDR and goes with very conservative heuristics instead,
> which in turn leads to the consequences discussed at length in the
> OVMF thread linked above.
> 
> Ideally we would simply outlaw (3), but it's hard for backward
> compatibility reasons.  Second best solution is a flag somewhere
> (msr, cpuid, ...) telling the guest firmware "you can use
> GUEST_MAXPHYADDR, we guarantee it is <= HOST_MAXPHYADDR".

Problem is GUEST_MAXPHYADDR > HOST_MAXPHYADDR is actually a supported
configuration on some setups. Namely when memory encryption is enabled
on AMD CPUs[1].

> 
> > As mentioned in the thread quoted above, using host_phys_bits is a
> > obvious thing to do when the user requested "-cpu host".
> > 
> > The harder issue is how to handle other CPU models. I had suggested
> > we should try associating a phys bits value with them, which would
> > probably involve creating Client/Server variants for all our CPU
> > models which don't currently have it. I still think that's worth
> > exploring as a strategy and with versioned CPU models we should
> > be ok wrt back compatibility with that approach.
> 
> Yep, better defaults for GUEST_MAXPHYADDR would be good too, but that
> is a separate (although related) discussion.
> 
> take care,
>   Gerd
> 
[1] - https://lkml.org/lkml/2020/6/19/2371

Re: [PATCH] cpu: Add starts_halted() method

[Philippe Mathieu-Daudé]

On 7/9/20 11:54 AM, Greg Kurz wrote:
> On Thu, 9 Jul 2020 07:11:09 +0200
> Philippe Mathieu-Daudé  wrote:
>> On 7/8/20 11:39 PM, Eduardo Habkost wrote:
>>> On Wed, Jul 08, 2020 at 06:45:57PM +0200, Philippe Mathieu-Daudé wrote:
 On 7/8/20 5:25 PM, Eduardo Habkost wrote:
> On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
>> On Wed, 8 Jul 2020 at 12:12, David Gibson  
>> wrote:
>>> On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé 
>>> wrote:
 Class boolean field certainly sounds better, but I am not sure this
 is a property of the machine. Rather the arch? So move the field
 to CPUClass? Maybe not, let's discuss :)
>>>
>>> It is absolutely a property of the machine.  e.g. I don't think we
>>> want this for powernv.  pseries is a bit of a special case since it is
>>> explicitly a paravirt platform.  But even for emulated hardware, the
>>> board can absolutely strap things so that cpus do or don't start
>>> immediately.
>>
>> It's a property of the individual CPU, I think. One common setup
>> for Arm systems is that the primary CPU starts powered up but
>> the secondaries all start powered down.
>
> Both statements can be true.  It can be a property of the
> individual CPU (although I'm not convinced it has to), but it
> still needs to be controlled by the machine.

 From what said Peter, I understand this is a property of the
 chipset. Chipsets are modelled unevenly.

 IIUC QEMU started with single-core CPUs.
 CPUState had same meaning for 'core' or 'cpu', 1-1 mapping.

 Then multicore CPUs could be easily modelled using multiple
 single-core CPUs, usually created in the machine code.

 Then we moved to SoC models, creating the cores in the SoC.
 Some SoCs have array of cores, eventually heterogeneous
 (see the ZynqMP). We have containers of CPUState.

 On an ARM-based SoC, you might have the first core started
 (as said Peter) or not.

 BCM2836 / BCM2837 and ZynqMP start will all ARM cores off.
 On the BCM chipsets, a DSP core will boot the ARM cores.
 On the ZynqMP, a MicroBlaze core boots them.
 As QEMU doesn't models heterogeneous architectures, we start
 modelling after the unmodelled cores booted us, so either one
 or all cores on.

 In this case, we narrowed down the 'start-powered-off' field
 to the SoC, which happens to be how ARM SoCs are modelled.
>>>
>>> I was not aware of the start-powered-off property.  If we make it
>>> generic, we can just let spapr use it.
>>>


 Chipsets providing a JTAG interface can have a SRST signal,
 the "system reset". When a JTAG probe is attached, it can
 keeps the whole chipset in a reset state. This is equivalent
 to QEMU '-S' mode (single step mode).


 I don't know about pseries hardware, but if this is 'explicit
 to paravirt platform', then I expect this to be the same with
 other accelerators/architectures.

 If paravirtualized -> cores start off by default. Let the
 hypervisor start them. So still a property of the CPUState
 depending on the accelerator used?
>>>
>>> I don't understand this part.  Why would this depend on the
>>> accelerator?
>>
>> Because starting a virtualized machine with all cores powered-off
>> with TCG accelerator should at least emit a warning? Or change
>> the behavior and start them powered-on? This is machine-specific
>> although.
>>
> 
> FYI, PAPR requires all vCPUs to be "stopped" by default. It is up to the
> guest to start them explicitly through an RTAS call. The hypervisor is
> only responsible to start a single vCPU (see spapr_cpu_set_entry_state()
> called from spapr_machine_reset()) to be able to boot the guest.
> 
> So I'm not sure to see how that would depend on the accelerator...

$ qemu-system-ppc64 -M pseries-5.0,accel=tcg -d in_asm
qemu-system-ppc64: warning: TCG doesn't support requested feature,
cap-cfpc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature,
cap-sbbc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature,
cap-ibs=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature,
cap-ccf-assist=on

IN:
0x0100:  48003f00  b0x4000


IN:
0x4000:  7c7f1b78  mr   r31, r3
0x4004:  7d6000a6  mfmsrr11
0x4008:  3980a000  li   r12, 0xa000
0x400c:  798c83c6  sldi r12, r12, 0x30
0x4010:  7d6b6378  or   r11, r11, r12
0x4014:  7d600164  mtmsrd   r11
...

The vCPU doesn't seem stopped to me...

Am I missing something?

Re: [PATCH] Remove the CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE switch

[Stefan Hajnoczi]

On Thu, Jul 09, 2020 at 07:34:56AM +0200, Thomas Huth wrote:
> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
> index f0b66320e1..a4e6446ed9 100644
> --- a/util/coroutine-ucontext.c
> +++ b/util/coroutine-ucontext.c
> @@ -237,19 +237,15 @@ Coroutine *qemu_coroutine_new(void)
>  }
>  
>  #ifdef CONFIG_VALGRIND_H
> -#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)
>  /* Work around an unused variable in the valgrind.h macro... */
>  #pragma GCC diagnostic push
>  #pragma GCC diagnostic ignored "-Wunused-but-set-variable"
> -#endif

What about !defined(__clang__)? Looks like this will break clang builds:

  warning: unknown warning option '-Wunused-but-set-variable'; did you mean 
'-Wunused-const-variable'? [-Wunknown-warning-option]

Stefan


signature.asc
Description: PGP signature

[PATCH] migration: fix memory leak in qmp_migrate_set_parameters

[Chuan Zheng]

From: Zheng Chuan 

"tmp.tls_hostname" and "tmp.tls_creds" allocated by migrate_params_test_apply()
is forgot to free at the end of qmp_migrate_set_parameters(). Fix that.

The leak stack:
Direct leak of 2 byte(s) in 2 object(s) allocated from:
   #0 0xb597c20b in __interceptor_malloc (/usr/lib64/libasan.so.4+0xd320b)
   #1 0xb52dcb1b in g_malloc (/usr/lib64/libglib-2.0.so.0+0x58b1b)
   #2 0xb52f8143 in g_strdup (/usr/lib64/libglib-2.0.so.0+0x74143)
   #3 0xc52447fb in migrate_params_test_apply 
(/usr/src/debug/qemu-4.1.0/migration/migration.c:1377)
   #4 0xc52fdca7 in qmp_migrate_set_parameters 
(/usr/src/debug/qemu-4.1.0/qapi/qapi-commands-migration.c:192)
   #5 0xc551d543 in qmp_dispatch 
(/usr/src/debug/qemu-4.1.0/qapi/qmp-dispatch.c:165)
   #6 0xc52a0a8f in qmp_dispatch 
(/usr/src/debug/qemu-4.1.0/monitor/qmp.c:125)
   #7 0xc52a1c7f in monitor_qmp_dispatch 
(/usr/src/debug/qemu-4.1.0/monitor/qmp.c:214)
   #8 0xc55cb0cf in aio_bh_call (/usr/src/debug/qemu-4.1.0/util/async.c:117)
   #9 0xc55d4543 in aio_bh_poll 
(/usr/src/debug/qemu-4.1.0/util/aio-posix.c:459)
   #10 0xc55cae0f in aio_dispatch 
(/usr/src/debug/qemu-4.1.0/util/async.c:268)
   #11 0xb52d6a7b in g_main_context_dispatch 
(/usr/lib64/libglib-2.0.so.0+0x52a7b)
   #12 0xc55d1e3b(/usr/bin/qemu-kvm-4.1.0+0x1622e3b)
   #13 0xc4e314bb(/usr/bin/qemu-kvm-4.1.0+0xe824bb)
   #14 0xc47f45ef(/usr/bin/qemu-kvm-4.1.0+0x8455ef)
   #15 0xb4bfef3f in __libc_start_main (/usr/lib64/libc.so.6+0x23f3f)
   #16 0xc47ffacb(/usr/bin/qemu-kvm-4.1.0+0x850acb)

Direct leak of 2 byte(s) in 2 object(s) allocated from:
   #0 0xb597c20b in __interceptor_malloc (/usr/lib64/libasan.so.4+0xd320b)
   #1 0xb52dcb1b in g_malloc (/usr/lib64/libglib-2.0.so.0+0x58b1b)
   #2 0xb52f8143 in g_strdup (/usr/lib64/libglib-2.0.so.0+0x74143)
   #3 0xc5244893 in migrate_params_test_apply 
(/usr/src/debug/qemu-4.1.0/migration/migration.c:1382)
   #4 0xc52fdca7 in qmp_migrate_set_parameters 
(/usr/src/debug/qemu-4.1.0/qapi/qapi-commands-migration.c:192)
   #5 0xc551d543 in qmp_dispatch 
(/usr/src/debug/qemu-4.1.0/qapi/qmp-dispatch.c)
   #6 0xc52a0a8f in qmp_dispatch 
(/usr/src/debug/qemu-4.1.0/monitor/qmp.c:125)
   #7 0xc52a1c7f in monitor_qmp_dispatch 
(/usr/src/debug/qemu-4.1.0/monitor/qmp.c:214)
   #8 0xc55cb0cf in aio_bh_call (/usr/src/debug/qemu-4.1.0/util/async.c:117)
   #9 0xc55d4543 in aio_bh_poll 
(/usr/src/debug/qemu-4.1.0/util/aio-posix.c:459)
   #10 0xc55cae0f in in aio_dispatch 
(/usr/src/debug/qemu-4.1.0/util/async.c:268)
   #11 0xb52d6a7b in g_main_context_dispatch 
(/usr/lib64/libglib-2.0.so.0+0x52a7b)
   #12 0xc55d1e3b(/usr/bin/qemu-kvm-4.1.0+0x1622e3b)
   #13 0xc4e314bb(/usr/bin/qemu-kvm-4.1.0+0xe824bb)
   #14 0xc47f45ef (/usr/bin/qemu-kvm-4.1.0+0x8455ef)
   #15 0xb4bfef3f in __libc_start_main (/usr/lib64/libc.so.6+0x23f3f)
   #16 0xc47ffacb(/usr/bin/qemu-kvm-4.1.0+0x850acb)

Signed-off-by: Chuan Zheng 
Reviewed-by: KeQian Zhu 
Reviewed-by: HaiLiang 
---
 migration/migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 92e44e0..045180c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1342,12 +1342,12 @@ static void 
migrate_params_test_apply(MigrateSetParameters *params,
 
 if (params->has_tls_creds) {
 assert(params->tls_creds->type == QTYPE_QSTRING);
-dest->tls_creds = g_strdup(params->tls_creds->u.s);
+dest->tls_creds = params->tls_creds->u.s;
 }
 
 if (params->has_tls_hostname) {
 assert(params->tls_hostname->type == QTYPE_QSTRING);
-dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
+dest->tls_hostname = params->tls_hostname->u.s;
 }
 
 if (params->has_max_bandwidth) {
-- 
1.8.3.1

[Bug 1886811] Re: systemd complains Failed to enqueue loopback interface start request: Operation not supported

[Laurent Vivier]

** Changed in: qemu
   Status: New => In Progress

** Changed in: qemu
 Assignee: (unassigned) => Laurent Vivier (laurent-vivier)

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1886811

Title:
  systemd complains Failed to enqueue loopback interface start request:
  Operation not supported

Status in QEMU:
  In Progress
Status in qemu package in Debian:
  Unknown

Bug description:
  This symptom seems similar to
  https://bugs.launchpad.net/qemu/+bug/1823790

  Host Linux: Debian 11 Bullseye (testing) on x84-64 architecture
  qemu version: latest git of git commit hash 
eb2c66b10efd2b914b56b20ae90655914310c925
  compiled with "./configure --static --disable-system" 

  Down stream bug report at 
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=964289
  Bug report (closed) to systemd: 
https://github.com/systemd/systemd/issues/16359

  systemd in armhf and armel (both little endian 32-bit) containers fail to 
start with
  Failed to enqueue loopback interface start request: Operation not supported

  How to reproduce on Debian (and probably Ubuntu):
  mmdebstrap --components="main contrib non-free" --architectures=armhf 
--variant=important bullseye /var/lib/machines/armhf-bullseye
  systemd-nspawn -D /var/lib/machines/armhf-bullseye -b

  When "armhf" architecture is replaced with "mips" (32-bit big endian) or 
"ppc64"
  (64-bit big endian), the container starts up fine.

  The same symptom is also observed with "powerpc" (32-bit big endian)
  architecture.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1886811/+subscriptions

Re: [PATCH 2/2] util/qemu-sockets: make keep-alive enabled by default

[Vladimir Sementsov-Ogievskiy]

09.07.2020 11:29, Daniel P. Berrangé wrote:

On Wed, Jul 08, 2020 at 10:15:39PM +0300, Vladimir Sementsov-Ogievskiy wrote:

Keep-alive won't hurt, let's try to enable it even if not requested by
user.


Keep-alive intentionally breaks TCP connections earlier than normal
in face of transient networking problems.

The question is more about which type of pain is more desirable. A
stall in the network connection (for a potentially very long time),
or an intentionally broken socket.

I'm not at all convinced it is a good idea to intentionally break
/all/ QEMU sockets in the face of transient problems, even if the
problems last for 2 hours or more.

I could see keep-alives being ok on some QEMU socket. For example
VNC/SPICE clients, as there is no downside to proactively culling
them as they can trivially reconnect. Migration too is quite
reasonable to use keep alives, as you generally want migration to
run to completion in a short amount of time, and aborting migration
needs to be safe no matter what.

Breaking chardevs or block devices or network devices that use
QEMU sockets though will be disruptive. The only solution once
those backends have a dead socket is going to be to kill QEMU
and cold-boot the VM again.



Reasonable, thanks for explanation.

We are mostly interested in keep-alive for migration and NBD connections.
(NBD driver has ability to reconnect). What do you think about setting
keep-alive (with some KEEPIDLE smaller than 2 hours) by default for
migration and NBD (at least when NBD reconnect is enabled), would it be
valid?




Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  util/qemu-sockets.c | 15 ++-
  1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index b961963472..f6851376f5 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -438,7 +438,8 @@ static struct addrinfo 
*inet_parse_connect_saddr(InetSocketAddress *saddr,
   *
   * Handle keep_alive settings. If user specified settings explicitly, fail if
   * can't set the settings. If user just enabled keep-alive, not specifying the
- * settings, try to set defaults but ignore failures.
+ * settings, try to set defaults but ignore failures. If keep-alive option is
+ * not specified, try to set it but ignore failures.
   */
  static int inet_set_keepalive(int sock, bool has_keep_alive,
KeepAliveField *keep_alive, Error **errp)
@@ -447,8 +448,8 @@ static int inet_set_keepalive(int sock, bool has_keep_alive,
  int val;
  bool has_settings = has_keep_alive &&  keep_alive->type == QTYPE_QDICT;
  
-if (!has_keep_alive || (keep_alive->type == QTYPE_QBOOL &&

-!keep_alive->u.enabled))
+if (has_keep_alive &&
+keep_alive->type == QTYPE_QBOOL && !keep_alive->u.enabled)
  {
  return 0;
  }
@@ -456,8 +457,12 @@ static int inet_set_keepalive(int sock, bool 
has_keep_alive,
  val = 1;
  ret = qemu_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, , sizeof(val));
  if (ret < 0) {
-error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
-return -1;
+if (has_keep_alive) {
+error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
+return -1;
+} else {
+return 0;
+}
  }
  
  val = has_settings ? keep_alive->u.settings.idle : 30;

--
2.21.0



Regards,
Daniel




--
Best regards,
Vladimir

Re: [PATCH v2 1/2] GitLab Gating CI: introduce pipeline-status contrib script

[Erik Skultety]

On Wed, Jul 08, 2020 at 10:46:56PM -0400, Cleber Rosa wrote:
> This script is intended to be used right after a push to a branch.
>
> By default, it will look for the pipeline associated with the commit
> that is the HEAD of the *local* staging branch.  It can be used as a
> one time check, or with the `--wait` option to wait until the pipeline
> completes.
>
> If the pipeline is successful, then a merge of the staging branch into
> the master branch should be the next step.
>
> Signed-off-by: Cleber Rosa 
> ---
>  scripts/ci/gitlab-pipeline-status | 156 ++
>  1 file changed, 156 insertions(+)
>  create mode 100755 scripts/ci/gitlab-pipeline-status
>
> diff --git a/scripts/ci/gitlab-pipeline-status 
> b/scripts/ci/gitlab-pipeline-status
> new file mode 100755
> index 00..4a9de39872
> --- /dev/null
> +++ b/scripts/ci/gitlab-pipeline-status
> @@ -0,0 +1,156 @@
> +#!/usr/bin/env python3
> +#
> +# Copyright (c) 2019-2020 Red Hat, Inc.
> +#
> +# Author:
> +#  Cleber Rosa 
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +"""
> +Checks the GitLab pipeline status for a given commit commit

s/commit$/(hash|sha|ID|)

> +"""
> +
> +# pylint: disable=C0103
> +
> +import argparse
> +import http.client
> +import json
> +import os
> +import subprocess
> +import time
> +import sys
> +
> +
> +def get_local_staging_branch_commit():
> +"""
> +Returns the commit sha1 for the *local* branch named "staging"
> +"""
> +result = subprocess.run(['git', 'rev-parse', 'staging'],

If one day Peter decides that "staging" is not a cool name anymore and use a
different name for the branch :) we should account for that and make it a
variable, possibly even parametrize this function with it.

> +stdin=subprocess.DEVNULL,
> +stdout=subprocess.PIPE,
> +stderr=subprocess.DEVNULL,
> +cwd=os.path.dirname(__file__),
> +universal_newlines=True).stdout.strip()
> +if result == 'staging':
> +raise ValueError("There's no local staging branch")

"There's no local branch named 'staging'" would IMO be more descriptive, so as
not to confuse it with staging in git.

> +if len(result) != 40:
> +raise ValueError("Branch staging HEAD doesn't look like a sha1")
> +return result
> +
> +
> +def get_pipeline_status(project_id, commit_sha1):
> +"""
> +Returns the JSON content of the pipeline status API response
> +"""
> +url = '/api/v4/projects/{}/pipelines?sha={}'.format(project_id,
> +commit_sha1)
> +connection = http.client.HTTPSConnection('gitlab.com')
> +connection.request('GET', url=url)
> +response = connection.getresponse()
> +if response.code != http.HTTPStatus.OK:
> +raise ValueError("Failed to receive a successful response")
> +json_response = json.loads(response.read())

a blank line separating the commentary block would slightly help readability

> +# afaict, there should one one pipeline for the same project + commit

s/one one/be only one/

> +# if this assumption is false, we can add further filters to the
> +# url, such as username, and order_by.
> +if not json_response:
> +raise ValueError("No pipeline found")
> +return json_response[0]
> +
> +
> +def wait_on_pipeline_success(timeout, interval,
> + project_id, commit_sha):
> +"""
> +Waits for the pipeline to end up to the timeout given

"Waits for the pipeline to finish within the given timeout"

> +"""
> +start = time.time()
> +while True:
> +if time.time() >= (start + timeout):
> +print("Waiting on the pipeline success timed out")

s/success//
(the pipeline doesn't always have to finish with success)

> +return False
> +
> +status = get_pipeline_status(project_id, commit_sha)
> +if status['status'] == 'running':
> +time.sleep(interval)
> +print('running...')
> +continue
> +
> +if status['status'] == 'success':
> +return True
> +
> +msg = "Pipeline ended unsuccessfully, check: %s" % status['web_url']

I think more common expression is "Pipeline failed"

> +print(msg)
> +return False
> +
...

Code-wise looks OK to me, but since I don't know what Peter's requirements
on/expectations of this script are, I can't do a more thorough review.

Regards,
Erik

Re: [PATCH v7 12/47] block: Use bdrv_filter_(bs|child) where obvious

[Max Reitz]

On 08.07.20 20:24, Andrey Shinkevich wrote:
> On 25.06.2020 18:21, Max Reitz wrote:
>> Places that use patterns like
>>
>>  if (bs->drv->is_filter && bs->file) {
>>  ... something about bs->file->bs ...
>>  }
>>
>> should be
>>
>>  BlockDriverState *filtered = bdrv_filter_bs(bs);
>>  if (filtered) {
>>  ... something about @filtered ...
>>  }
>>
>> instead.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   block.c    | 31 ---
>>   block/io.c |  7 +--
>>   migration/block-dirty-bitmap.c |  8 +---
>>   3 files changed, 26 insertions(+), 20 deletions(-)
>>
> ...
>> diff --git a/block/io.c b/block/io.c
>> index df8f2a98d4..385176b331 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -3307,6 +3307,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
>> *child, int64_t offset, bool exact,
>>     Error **errp)
>>   {
>>   BlockDriverState *bs = child->bs;
>> +    BdrvChild *filtered;
>>   BlockDriver *drv = bs->drv;
>>   BdrvTrackedRequest req;
>>   int64_t old_size, new_bytes;
>> @@ -3358,6 +3359,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
>> *child, int64_t offset, bool exact,
>>   goto out;
>>   }
>>   +    filtered = bdrv_filter_child(bs);
>> +
> 
> Isn't better to have this initialization right before the relevant
> if/else block?

Hm, well, yes.  In this case, though, maybe not.  Patch 16 will add
another BdrvChild to be initialized here (@backing), and we need to
initialize that one here.  So I felt it made sense to group them together.

They got split up when I decided to put @filtered into this patch and
@backing into its own.  So now it may look a bit weird, but I feel like
after patch 16 it makes sense.

(I’m indifferent, basically.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH RFC 4/5] s390x: implement virtio-mem-ccw

[David Hildenbrand]

On 09.07.20 11:24, Cornelia Huck wrote:
> On Wed,  8 Jul 2020 20:51:34 +0200
> David Hildenbrand  wrote:
> 
>> Add a proper CCW proxy device, similar to the PCI variant.
>>
>> Signed-off-by: David Hildenbrand 
>> ---
>>  hw/s390x/virtio-ccw-mem.c | 165 ++
>>  hw/s390x/virtio-ccw.h |  13 +++
>>  2 files changed, 178 insertions(+)
>>  create mode 100644 hw/s390x/virtio-ccw-mem.c
> 
> (...)
> 
>> +static void virtio_ccw_mem_instance_init(Object *obj)
>> +{
>> +VirtIOMEMCcw *ccw_mem = VIRTIO_MEM_CCW(obj);
>> +VirtIOMEMClass *vmc;
>> +VirtIOMEM *vmem;
>> +
> 
> I think you want
> 
> ccw_dev->force_revision_1 = true;
> 
> here (similar to forcing virtio-pci to modern-only.)

Ah, that's the magic bit, was looking for that. Thanks!


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 3/7] device-core: use RCU for list of childs of a bus

[Maxim Levitsky]

On Wed, 2020-05-27 at 15:45 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:47PM +0300, Maxim Levitsky wrote:
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index d87d989e72..ef47cb2d9c 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -3,6 +3,8 @@
> >  
> >  #include "qemu/queue.h"
> >  #include "qemu/bitmap.h"
> > +#include "qemu/rcu.h"
> > +#include "qemu/rcu_queue.h"
> >  #include "qom/object.h"
> >  #include "hw/hotplug.h"
> >  #include "hw/resettable.h"
> > @@ -230,6 +232,7 @@ struct BusClass {
> >  };
> >  
> >  typedef struct BusChild {
> > +struct rcu_head rcu;
> >  DeviceState *child;
> >  int index;
> >  QTAILQ_ENTRY(BusChild) sibling;
> 
> Please add a doc comment to struct BusState saying the children field is
> an RCU QTAILQ and writers must hold the QEMU global mutex.
> 
> Stefan
Done.


Best regards,
Maxim Levitsky

[PATCH v2 1/2] hw/riscv: Modify MROM size to end at 0x10000

[Bin Meng]

From: Bin Meng 

At present the size of Mask ROM for sifive_u / spike / virt machines
is set to 0x11000, which ends at an unusual address. This changes the
size to 0xf000 so that it ends at 0x1.

Signed-off-by: Bin Meng 
Reviewed-by: Philippe Mathieu-Daudé 
---

(no changes since v1)

 hw/riscv/sifive_u.c | 2 +-
 hw/riscv/spike.c| 2 +-
 hw/riscv/virt.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index dc46f64..3413369 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -70,7 +70,7 @@ static const struct MemmapEntry {
 hwaddr size;
 } sifive_u_memmap[] = {
 [SIFIVE_U_DEBUG] ={0x0,  0x100 },
-[SIFIVE_U_MROM] = { 0x1000,0x11000 },
+[SIFIVE_U_MROM] = { 0x1000, 0xf000 },
 [SIFIVE_U_CLINT] ={  0x200,0x1 },
 [SIFIVE_U_L2LIM] ={  0x800,  0x200 },
 [SIFIVE_U_PLIC] = {  0xc00,  0x400 },
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index a187aa3..ea4be98 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -57,7 +57,7 @@ static const struct MemmapEntry {
 hwaddr base;
 hwaddr size;
 } spike_memmap[] = {
-[SPIKE_MROM] = { 0x1000,0x11000 },
+[SPIKE_MROM] = { 0x1000, 0xf000 },
 [SPIKE_CLINT] ={  0x200,0x1 },
 [SPIKE_DRAM] = { 0x8000,0x0 },
 };
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 5ca49c5..37b8c55 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -53,7 +53,7 @@ static const struct MemmapEntry {
 hwaddr size;
 } virt_memmap[] = {
 [VIRT_DEBUG] =   {0x0, 0x100 },
-[VIRT_MROM] ={ 0x1000,   0x11000 },
+[VIRT_MROM] ={ 0x1000,0xf000 },
 [VIRT_TEST] ={   0x10,0x1000 },
 [VIRT_RTC] = {   0x101000,0x1000 },
 [VIRT_CLINT] =   {  0x200,   0x1 },
-- 
2.7.4

Re: [PATCH v2 6/7] scsi: Add scsi_device_get

[Maxim Levitsky]

On Wed, 2020-05-27 at 16:27 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:50PM +0300, Maxim Levitsky wrote:
> > +/*
> > + * This function works like scsi_device_get but doesn't take a refernce
> 
> s/refernce/reference/
> 
> > + * to the returned object. Intended for legacy code
> 
> The following explains this in more detail. It's not necessarily legacy
> code but rather whether it runs under the QEMU global mutex or not:
> 
> Devices that run under the QEMU global mutex can use this function.
> Devices that run outside the QEMU global mutex must use
> scsi_device_get() instead.
Done.

Best regards,
Maxim Levitsky

Re: Migrating custom qemu.org infrastructure to GitLab

[Paolo Bonzini]

On 09/07/20 12:22, Thomas Huth wrote:
> FWIW, seems like we could use the "pandoc" tool to convert Mediawiki
> (our old Wiki) to Markdown (Gitlab wiki). I've done a quick test and
> converted https://wiki.qemu.org/Contribute/MailingLists into
> https://gitlab.com/huth/qemu/-/wikis/Contribute/MailingLists with some
> few clicks.
> 
> But the longer I look at most Wiki pages, the more I think that we
> should convert the important pages rather into a part of qemu-web
> instead. I'll have a closer look and will suggest some patches when time
> permits...

The wiki was cleaned up more or less at the same time as the
qemu-web.git repo was created (actually as a prerequisite), it's
actually not in a bad shape.  The idea was that the wiki kept:

- stuff that really belonged in documentation (such as completed
features and developer information)

- stuff that needs to be edited quickly (such as feature pages or or
internship ideas)

- developer-targeted information that doesn't belong in documentation
(such as CI status), even if it's linked from qemu.org (e.g.
https://www.qemu.org/contribute/

while qemu-web got the more user-targeted information.  This is because
updating qemu-web is a bit slower, requiring review and all that.

We can certainly move some wiki pages to qemu-web, like we did for
"report a bug" in the past and like Alex did recently for the
Conservancy page.  But I think there aren't that many left, most of them
are in the first category above and should be moved to docs/devel (for
example https://wiki.qemu.org/Contribute/SubmitAPatch).

Once we have docs CI on GitLab we can easily link to them from
qemu-web.git, so setting up docs CI is probably a good first step
towards relying more on GitLab and also cleaning up the wiki.

Paolo

Re: [PATCH 0/2] keepalive default

[Daniel P . Berrangé]

On Wed, Jul 08, 2020 at 10:15:37PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> We understood, that keepalive is almost superfluous with default 2 hours
> in /proc/tcp_keepalive_time. Forcing user to setup keepalive for the
> whole system doesn't seem right, better setup it per-socket.

Adding the ability to explicitly configure the keepalive settings makes
sense for QEMU. Completely ignoring system defaults when no explicit
settings are given though is not valid IMHO.


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

Re: [PATCH v2 4/7] device-core: use atomic_set on .realized property

[Maxim Levitsky]

On Wed, 2020-05-27 at 16:00 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:48PM +0300, Maxim Levitsky wrote:
> > Some code might race with placement of new devices on a bus.
> > We currently first place a (unrealized) device on the bus
> > and then realize it.
> > 
> > As a workaround, users that scan the child device list, can
> > check the realized property to see if it is safe to access such a device.
> > Use an atomic write here too to aid with this.
> > 
> > A separate discussion is what to do with devices that are unrealized:
> > It looks like for this case we only call the hotplug handler's unplug
> > callback and its up to it to unrealize the device.
> > An atomic operation doesn't cause harm for this code path though.
> > 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  hw/core/qdev.c | 15 ++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> Please add a comment to struct DeviceState saying the realized field
> must be accessed with atomic_load_acquire() when used outside the QEMU
> global mutex.
> 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 732789e2b7..d530c5922f 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -964,7 +964,20 @@ static void device_set_realized(Object *obj, bool 
> > value, Error **errp)
> >  }
> > }
> >  
> > +   atomic_store_release(>realized, value);
> > +
> >  } else if (!value && dev->realized) {
> > +
> > +/*
> > + * Change the value so that any concurrent users are aware
> > + * that the device is going to be unrealized
> > + *
> > + * TODO: change .realized property to enum that states
> > + * each phase of the device realization/unrealization
> > + */
> > +
> > +atomic_store_release(>realized, value);
> 
> I'm not sure if atomic_store_release() is strong enough in the true ->
> false case:
> 
>   Operations coming after ``atomic_store_release()`` can still be
>   reordered before it.
> 
> A reader may already seen changes made to unrealize the DeviceState even
> though realized still appears to be true. A full write memory barrier
> seems safer here.
Done.

Best regards,
Maxim Levitsky

Re: [PATCH] cpu: Add starts_halted() method

[Philippe Mathieu-Daudé]

On 7/9/20 5:26 AM, Thiago Jung Bauermann wrote:
> 
> Thiago Jung Bauermann  writes:
> 
>> I'm seeing the vcpu being KVM_RUN'd too early twice during hotplug.
>> Both of them are before cpu_reset() and ppc_cpu_reset().
> 
> Hm, rereading the message obviously the above is partially wrong. The
> second case happens during ppc_cpu_reset().
> 
>> Here's the second:
>>
>> #0  in qemu_cpu_kick_thread ()
>> #1  in qemu_cpu_kick ()
>> #2  in queue_work_on_cpu ()
>> #3  in async_run_on_cpu ()
>> #4  in tlb_flush_by_mmuidx ()
>> #5  in tlb_flush ()
>> #6  in ppc_tlb_invalidate_all ()
>> #7  in ppc_cpu_reset ()
>> #8  in device_transitional_reset ()
>> #9  in resettable_phase_hold ()
>> #10 in resettable_assert_reset ()
>> #11 in device_set_realized ()

Dunno if related, might be helpful:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg686477.html

>> #12 in property_set_bool ()
>> #13 in object_property_set ()
>> #14 in object_property_set_qobject ()
>> #15 in object_property_set_bool ()
>> #16 in qdev_realize ()
>> #17 in spapr_realize_vcpu ()
>> #18 in spapr_cpu_core_realize ()
>> #19 in device_set_realized ()
>> #20 in property_set_bool ()
>> #21 in object_property_set ()
>> #22 in object_property_set_qobject ()
>> #23 in object_property_set_bool ()
>> #24 in qdev_realize ()
>> #25 in qdev_device_add ()
>> #26 in qmp_device_add ()
> 

Re: [PATCH RFC 2/5] s390x: implement diag260

[Cornelia Huck]

On Wed,  8 Jul 2020 20:51:32 +0200
David Hildenbrand  wrote:

> Let's implement the "storage configuration" part of diag260. This diag
> is found under z/VM, to indicate usable chunks of memory tot he guest OS.
> As I don't have access to documentation, I have no clue what the actual
> error cases are, and which other stuff we could eventually query using this
> interface. Somebody with access to documentation should fix this. This
> implementation seems to work with Linux guests just fine.
> 
> The Linux kernel supports diag260 to query the available memory since
> v4.20. Older kernels / kvm-unit-tests will later fail to run in such a VM
> (with maxmem being defined and bigger than the memory size, e.g., "-m
>  2G,maxmem=4G"), just as if support for SCLP storage information is not
> implemented. They will fail to detect the actual initial memory size.
> 
> This interface allows us to expose the maximum ramsize via sclp
> and the initial ramsize via diag260 - without having to mess with the
> memory increment size and having to align the initial memory size to it.
> 
> This is a preparation for memory device support. We'll unlock the
> implementation with a new QEMU machine that supports memory devices.
> 
> Signed-off-by: David Hildenbrand 
> ---
>  target/s390x/diag.c| 57 ++
>  target/s390x/internal.h|  2 ++
>  target/s390x/kvm.c | 11 
>  target/s390x/misc_helper.c |  6 
>  target/s390x/translate.c   |  4 +++
>  5 files changed, 80 insertions(+)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index 1a48429564..c3b1e24b2c 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -23,6 +23,63 @@
>  #include "hw/s390x/pv.h"
>  #include "kvm_s390x.h"
>  
> +void handle_diag_260(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t 
> ra)
> +{
> +MachineState *ms = MACHINE(qdev_get_machine());
> +const ram_addr_t initial_ram_size = ms->ram_size;
> +const uint64_t subcode = env->regs[r3];
> +S390CPU *cpu = env_archcpu(env);
> +ram_addr_t addr, length;
> +uint64_t tmp;
> +
> +/* TODO: Unlock with new QEMU machine. */
> +if (false) {
> +s390_program_interrupt(env, PGM_OPERATION, ra);
> +return;
> +}
> +
> +/*
> + * There also seems to be subcode "0xc", which stores the size of the
> + * first chunk and the total size to r1/r2. It's only used by very old
> + * Linux, so don't implement it.

FWIW,
https://www-01.ibm.com/servers/resourcelink/svc0302a.nsf/pages/zVMV7R1sc246272/$file/hcpb4_v7r1.pdf
seems to list the available subcodes. Anything but 0xc and 0x10 is for
24/31 bit only, so we can safely ignore them. Not sure what we want to
do with 0xc: it is supposed to "Return the highest addressable byte of
virtual storage in the host-primary address space, including named
saved systems and saved segments", so returning the end of the address
space should be easy enough, but not very useful.

> + */
> +if ((r1 & 1) || subcode != 0x10) {
> +s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +return;
> +}
> +addr = env->regs[r1];
> +length = env->regs[r1 + 1];
> +
> +/* FIXME: Somebody with documentation should fix this. */

Doc mentioned above says for specification exception:

"For subcode X'10':
• Rx is not an even-numbered register.
• The address contained in Rx is not on a quadword boundary.
• The length contained in Rx+1 is not a positive multiple of 16."

> +if (!QEMU_IS_ALIGNED(addr, 16) || !QEMU_IS_ALIGNED(length, 16)) {
> +s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +return;
> +}
> +
> +/* FIXME: Somebody with documentation should fix this. */
> +if (!length) {

Probably specification exception as well?

> +setcc(cpu, 3);
> +return;
> +}
> +
> +/* FIXME: Somebody with documentation should fix this. */

For access exception:

"For subcode X'10', an error occurred trying to store the extent
information into the guest's output area."

> +if (!address_space_access_valid(_space_memory, addr, length, 
> true,
> +MEMTXATTRS_UNSPECIFIED)) {
> +s390_program_interrupt(env, PGM_ADDRESSING, ra);
> +return;
> +}
> +
> +/* Indicate our initial memory ([0 .. ram_size - 1]) */
> +tmp = cpu_to_be64(0);
> +cpu_physical_memory_write(addr, , sizeof(tmp));
> +tmp = cpu_to_be64(initial_ram_size - 1);
> +cpu_physical_memory_write(addr + sizeof(tmp), , sizeof(tmp));
> +
> +/* Exactly one entry was stored. */
> +env->regs[r3] = 1;
> +setcc(cpu, 0);
> +}
> +
>  int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
>  {
>  uint64_t func = env->regs[r1];

(...)

> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 58dbc023eb..d7274eb320 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -116,6 +116,12 @@ 

[PATCH] iotests: Simplify _filter_img_create() a bit

[Max Reitz]

Not only is it a bit stupid to try to filter multi-line "Formatting"
output (because we only need it for a single test, which can easily be
amended to no longer need it), it is also problematic when there can be
output after a "Formatting" line that we do not want to filter as if it
were part of it.

So rename _filter_img_create to _do_filter_img_create, let it filter
only a single line, and let _filter_img_create loop over all input
lines, calling _do_filter_img_create only on those that match
/^Formatting/ (basically, what _filter_img_create_in_qmp did already).
(And fix 020 to work with that.)

Reported-by: Kevin Wolf 
Signed-off-by: Max Reitz 
---
Kevin noted that the changes to _filter_img_create broke Eric's patch to
flush the Formatting line out before a potential error message.  This
patch should fix it (and the diff stat is negative, so that's nice).
---
 tests/qemu-iotests/020   | 29 ---
 tests/qemu-iotests/020.out   | 13 +--
 tests/qemu-iotests/141   |  2 +-
 tests/qemu-iotests/common.filter | 62 ++--
 4 files changed, 45 insertions(+), 61 deletions(-)

diff --git a/tests/qemu-iotests/020 b/tests/qemu-iotests/020
index 20f8f185d0..b488000cb9 100755
--- a/tests/qemu-iotests/020
+++ b/tests/qemu-iotests/020
@@ -115,18 +115,23 @@ TEST_IMG="$TEST_IMG.base" _make_test_img 1M
 # Create an image with a null backing file to which committing will fail (with
 # ENOSPC so we can distinguish the result from some generic EIO which may be
 # generated anywhere in the block layer)
-_make_test_img -b "json:{'driver': '$IMGFMT',
- 'file': {
- 'driver': 'blkdebug',
- 'inject-error': [{
- 'event': 'write_aio',
- 'errno': 28,
- 'once': true
- }],
- 'image': {
- 'driver': 'file',
- 'filename': '$TEST_IMG.base'
- }}}"
+backing="json:{'driver': '$IMGFMT',
+   'file': {
+   'driver': 'blkdebug',
+   'inject-error': [{
+   'event': 'write_aio',
+   'errno': 28,
+   'once': true
+   }],
+   'image': {
+   'driver': 'file',
+   'filename': '$TEST_IMG.base'
+   }}}"
+
+# Filter out newlines and collapse spaces
+backing=$(echo "$backing" | tr -d '\n' | tr -s ' ')
+
+_make_test_img -b "$backing"
 
 # Just write anything so committing will not be a no-op
 $QEMU_IO -c 'writev 0 64k' "$TEST_IMG" | _filter_qemu_io
diff --git a/tests/qemu-iotests/020.out b/tests/qemu-iotests/020.out
index 4b722b2dd0..4668ac59df 100644
--- a/tests/qemu-iotests/020.out
+++ b/tests/qemu-iotests/020.out
@@ -1079,18 +1079,7 @@ No errors were found on the image.
 Testing failing commit
 
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=json:{'driver': 'IMGFMT',,
- 'file': {
- 'driver': 'blkdebug',,
- 'inject-error': [{
- 'event': 'write_aio',,
- 'errno': 28,,
- 'once': true
- }],,
- 'image': {
- 'driver': 'file',,
- 'filename': 'TEST_DIR/t.IMGFMT.base'
- }}}
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=json:{'driver': 'IMGFMT',, 'file': { 'driver': 'blkdebug',, 
'inject-error': [{ 'event': 'write_aio',, 'errno': 28,, 'once': true }],, 
'image': { 'driver': 'file',, 'filename': 'TEST_DIR/t.IMGFMT.base' }}}
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qemu-img: Block job failed: No space left on device
diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
index 6d1b7b0d4c..5192d256e3 100755
--- a/tests/qemu-iotests/141
+++ b/tests/qemu-iotests/141
@@ -68,7 +68,7 @@ test_blockjob()
 _send_qemu_cmd $QEMU_HANDLE \
 "$1" \
 "$2" \
-| _filter_img_create_in_qmp | _filter_qmp_empty_return
+| _filter_img_create | _filter_qmp_empty_return
 
 # We want this to return an error because the block job is still running
 _send_qemu_cmd $QEMU_HANDLE \
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index d967adc59a..3833206327 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -119,8 +119,21 @@ _filter_actual_image_size()
 $SED -s 's/\("actual-size":\s*\)[0-9]\+/\1SIZE/g'
 }
 
+# Filename filters for 

[PATCH] docs/devel/fuzzing: Fix bugs in documentation

[Thomas Huth]

Fix typo - the option is called "--fuzz-target" and not "--fuzz_taget".
Also use a different fuzzer in the example, since "virtio-net-fork-fuzz"
does not seem to be a valid fuzzer target (anymore?).

Signed-off-by: Thomas Huth 
---
 docs/devel/fuzzing.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/devel/fuzzing.txt b/docs/devel/fuzzing.txt
index 324d2cd92b..db5641de74 100644
--- a/docs/devel/fuzzing.txt
+++ b/docs/devel/fuzzing.txt
@@ -33,11 +33,11 @@ Fuzz targets are built similarly to system/softmmu:
 
 This builds ./i386-softmmu/qemu-fuzz-i386
 
-The first option to this command is: --fuzz_taget=FUZZ_NAME
+The first option to this command is: --fuzz-target=FUZZ_NAME
 To list all of the available fuzzers run qemu-fuzz-i386 with no arguments.
 
-eg:
-./i386-softmmu/qemu-fuzz-i386 --fuzz-target=virtio-net-fork-fuzz
+For example:
+./i386-softmmu/qemu-fuzz-i386 --fuzz-target=virtio-scsi-fuzz
 
 Internally, libfuzzer parses all arguments that do not begin with "--".
 Information about these is available by passing -help=1
-- 
2.18.1

Re: [PATCH v7 12/47] block: Use bdrv_filter_(bs|child) where obvious

[Andrey Shinkevich]

On 09.07.2020 11:59, Max Reitz wrote:

On 08.07.20 20:24, Andrey Shinkevich wrote:

On 25.06.2020 18:21, Max Reitz wrote:

Places that use patterns like

  if (bs->drv->is_filter && bs->file) {
  ... something about bs->file->bs ...
  }

should be

  BlockDriverState *filtered = bdrv_filter_bs(bs);
  if (filtered) {
  ... something about @filtered ...
  }

instead.

Signed-off-by: Max Reitz 
---
   block.c    | 31 ---
   block/io.c |  7 +--
   migration/block-dirty-bitmap.c |  8 +---
   3 files changed, 26 insertions(+), 20 deletions(-)


...

diff --git a/block/io.c b/block/io.c
index df8f2a98d4..385176b331 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3307,6 +3307,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
*child, int64_t offset, bool exact,
     Error **errp)
   {
   BlockDriverState *bs = child->bs;
+    BdrvChild *filtered;
   BlockDriver *drv = bs->drv;
   BdrvTrackedRequest req;
   int64_t old_size, new_bytes;
@@ -3358,6 +3359,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
*child, int64_t offset, bool exact,
   goto out;
   }
   +    filtered = bdrv_filter_child(bs);
+

Isn't better to have this initialization right before the relevant
if/else block?

Hm, well, yes.  In this case, though, maybe not.  Patch 16 will add
another BdrvChild to be initialized here (@backing), and we need to
initialize that one here.  So I felt it made sense to group them together.

They got split up when I decided to put @filtered into this patch and
@backing into its own.  So now it may look a bit weird, but I feel like
after patch 16 it makes sense.

(I’m indifferent, basically.)

Max


Yes, it makes a sence. I am on the way to reviewing further and have not 
reached the 16th yet.


It is a minor thing anyway )) Thank you for your response.

Andrey

Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it

[Gerd Hoffmann]

  Hi,

> > (CCing libvir-list, and people who were included in the OVMF
> > thread[1])
> > 
> > [1] 
> > https://lore.kernel.org/qemu-devel/99779e9c-f05f-501b-b4be-ff719f140...@canonical.com/

> > Also, it's important that we work with libvirt and management
> > software to ensure they have appropriate APIs to choose what to
> > do when a cluster has hosts with different MAXPHYADDR.
> 
> There's been so many complex discussions that it is hard to have any
> understanding of what we should be doing going forward. There's enough
> problems wrt phys bits, that I think we would benefit from a doc that
> outlines the big picture expectation for how to handle this in the
> virt stack.

Well, the fundamental issue is not that hard actually.  We have three
cases:

(1) GUEST_MAXPHYADDR == HOST_MAXPHYADDR

Everything is fine ;)

(2) GUEST_MAXPHYADDR < HOST_MAXPHYADDR

Mostly fine.  Some edge cases, like different page fault errors for
addresses above GUEST_MAXPHYADDR and below HOST_MAXPHYADDR.  Which I
think Mohammed fixed in the kernel recently.

(3) GUEST_MAXPHYADDR > HOST_MAXPHYADDR

Broken.  If the guest uses addresses above HOST_MAXPHYADDR everything
goes south.

The (2) case isn't much of a problem.  We only need to figure whenever
we want qemu allow this unconditionally (current state) or only in case
the kernel fixes are present (state with this patch applied if I read it
correctly).

The (3) case is the reason why guest firmware never ever uses
GUEST_MAXPHYADDR and goes with very conservative heuristics instead,
which in turn leads to the consequences discussed at length in the
OVMF thread linked above.

Ideally we would simply outlaw (3), but it's hard for backward
compatibility reasons.  Second best solution is a flag somewhere
(msr, cpuid, ...) telling the guest firmware "you can use
GUEST_MAXPHYADDR, we guarantee it is <= HOST_MAXPHYADDR".

> As mentioned in the thread quoted above, using host_phys_bits is a
> obvious thing to do when the user requested "-cpu host".
> 
> The harder issue is how to handle other CPU models. I had suggested
> we should try associating a phys bits value with them, which would
> probably involve creating Client/Server variants for all our CPU
> models which don't currently have it. I still think that's worth
> exploring as a strategy and with versioned CPU models we should
> be ok wrt back compatibility with that approach.

Yep, better defaults for GUEST_MAXPHYADDR would be good too, but that
is a separate (although related) discussion.

take care,
  Gerd

Re: [PATCH 1/2] hw/riscv: Modify MROM size to end at 0x10000

[Bin Meng]

Hi Philippe,

On Thu, Jul 9, 2020 at 1:15 PM Philippe Mathieu-Daudé  wrote:
>
> On 7/9/20 3:09 AM, Bin Meng wrote:
> > From: Bin Meng 
> >
> > At present the size of Mask ROM for sifive_u / spike / virt machines
> > is set to 0x11000, which ends at an unusual address. This changes the
> > size to 0xf000 so that it ends at 0x1.
>
> Maybe the size is correct but the first 4K are shadowed by the DEBUG
> region?
>

The DEBUG region does not match what the SiFive FU540 manual says. But
we don't support DEBUG in QEMU anyway :)

> Anyway for QEMU this patch is an improvement, so:
> Reviewed-by: Philippe Mathieu-Daudé 
>

Thanks for the review!

Regards,
Bin

Re: Migrating custom qemu.org infrastructure to GitLab

[Thomas Huth]

On 09/07/2020 12.16, Gerd Hoffmann wrote:
>   Hi,
> 
>>> 2. wiki.qemu.org is a MediaWiki instance. Account creation is a hurdle
>>> to one-time or new contributors. It is unclear whether GitLab's wiki
>>> is expressive enough for a lossless conversion of the existing QEMU
>>> wiki. Any volunteers interested in evaluating the wiki migration would
>>> be appreciated.
>>
>> Yeah, this is a potentially big piece of work. We didn't finish this
>> in libvirt either. Looking at the libvirt mediawiki though, I decided
>> not todo a straight export/import of all content.
> 
> FYI: gitlab wiki is basically just a git repo with markdown pages +
> renderer + gui editor.  You can also update the wiki using git clone +
> edit + git commit + git push.

FWIW, seems like we could use the "pandoc" tool to convert Mediawiki
(our old Wiki) to Markdown (Gitlab wiki). I've done a quick test and
converted https://wiki.qemu.org/Contribute/MailingLists into
https://gitlab.com/huth/qemu/-/wikis/Contribute/MailingLists with some
few clicks.

But the longer I look at most Wiki pages, the more I think that we
should convert the important pages rather into a part of qemu-web
instead. I'll have a closer look and will suggest some patches when time
permits...

 Thomas

Re: [PATCH] Remove the CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE switch

[Thomas Huth]

On 09/07/2020 12.51, Stefan Hajnoczi wrote:
> On Thu, Jul 09, 2020 at 07:34:56AM +0200, Thomas Huth wrote:
>> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
>> index f0b66320e1..a4e6446ed9 100644
>> --- a/util/coroutine-ucontext.c
>> +++ b/util/coroutine-ucontext.c
>> @@ -237,19 +237,15 @@ Coroutine *qemu_coroutine_new(void)
>>  }
>>  
>>  #ifdef CONFIG_VALGRIND_H
>> -#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)
>>  /* Work around an unused variable in the valgrind.h macro... */
>>  #pragma GCC diagnostic push
>>  #pragma GCC diagnostic ignored "-Wunused-but-set-variable"
>> -#endif
> 
> What about !defined(__clang__)? Looks like this will break clang builds:
> 
>   warning: unknown warning option '-Wunused-but-set-variable'; did you mean 
> '-Wunused-const-variable'? [-Wunknown-warning-option]

Oh, I didn't hit this problem in the CI:

 https://gitlab.com/huth/qemu/-/jobs/629814877#L2287

... which version of Clang are you using? Anyway, I'll put the
!defined(__clang__) back here, thanks for reporting it!

 Thomas

Re: [PATCH RFC 3/5] s390x: prepare device memory address space

[Cornelia Huck]

On Wed,  8 Jul 2020 20:51:33 +0200
David Hildenbrand  wrote:

> Let's allocate the device memory information and setup the device
> memory address space. Expose the maximum ramsize via SCLP and the actual
> initial ramsize via diag260.
> 
> Signed-off-by: David Hildenbrand 
> ---
>  hw/s390x/s390-virtio-ccw.c | 43 ++
>  hw/s390x/sclp.c| 12 +++--
>  include/hw/s390x/s390-virtio-ccw.h |  3 +++
>  target/s390x/diag.c|  4 +--
>  4 files changed, 58 insertions(+), 4 deletions(-)

(...)

> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index c3b1e24b2c..6b33eb0efc 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -32,8 +32,8 @@ void handle_diag_260(CPUS390XState *env, uint64_t r1, 
> uint64_t r3, uintptr_t ra)
>  ram_addr_t addr, length;
>  uint64_t tmp;
>  
> -/* TODO: Unlock with new QEMU machine. */
> -if (false) {
> +/* Support for diag260 is glued to support for memory devices. */

I'm wondering why you need to do this... sure, the availability of a
new diagnose could be perceived as a guest-visible change, but does the
information presented change anything? Without memory devices, it will
just duplicate the information already reported via SCLP, IIUC?

> +if (!memory_devices_allowed()) {
>  s390_program_interrupt(env, PGM_OPERATION, ra);
>  return;
>  }

Re: Failure prints during format or mounting a usb storage device

[Gerd Hoffmann]

> Starting at line 1746 is the first CBW, it's for an Inquiry command.
> 
> Starting at line 1759 is the response, notice at line 1761 the MSD debug
> says "Data in 64/36", which is strange.

Not really.  First is the packet size, second is the (remaining) data
size.  Inquiry data is 36 bytes, and dwc2 uses 64 byte instead of 36
byte transfers.

> Then the MSD defers the packet, even though the full 36 bytes has
> already been received.

Yes, and this is the problem.  The condition checks whenever there is
room left in the usb packet.  But we should also check whenever there
is actually more data pending, so how about this:

if (p->actual_length < p->iov.size && s->mode == USB_MSDM_DATAIN) {
DPRINTF("Deferring packet %p [wait data-in]\n", p);

take care,
  Gerd

Re: [PATCH v7 02/47] block: Add chain helper functions

[Max Reitz]

On 08.07.20 19:20, Andrey Shinkevich wrote:
> On 25.06.2020 18:21, Max Reitz wrote:
>> Add some helper functions for skipping filters in a chain of block
>> nodes.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   include/block/block_int.h |  3 +++
>>   block.c   | 55 +++
>>   2 files changed, 58 insertions(+)
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index bb3457c5e8..5da793bfc3 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1382,6 +1382,9 @@ BdrvChild *bdrv_cow_child(BlockDriverState *bs);
>>   BdrvChild *bdrv_filter_child(BlockDriverState *bs);
>>   BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs);
>>   BdrvChild *bdrv_primary_child(BlockDriverState *bs);
>> +BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs);
>> +BlockDriverState *bdrv_skip_filters(BlockDriverState *bs);
>> +BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs);
>>     static inline BlockDriverState *child_bs(BdrvChild *child)
>>   {
>> diff --git a/block.c b/block.c
>> index 5a42ef49fd..0a0b855261 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -7008,3 +7008,58 @@ BdrvChild *bdrv_primary_child(BlockDriverState
>> *bs)
>>     return NULL;
>>   }
>> +
>> +static BlockDriverState *bdrv_do_skip_filters(BlockDriverState *bs,
>> +  bool
>> stop_on_explicit_filter)
>> +{
>> +    BdrvChild *c;
>> +
>> +    if (!bs) {
>> +    return NULL;
>> +    }
>> +
>> +    while (!(stop_on_explicit_filter && !bs->implicit)) {
>> +    c = bdrv_filter_child(bs);
>> +    if (!c) {
>> +    break;
>> +    }
>> +    bs = c->bs;
> 
> Could it be child_bs(bs) ?

Well, in a sense, but not really.  We need to check whether there is a
child before overwriting @bs (because @bs must stay a non-NULL pointer),
so we wouldn’t have fewer lines of code if we replaced “BdrvChild *c” by
“BlockDriverState *child_bs”, and then used bdrv_child() to set child_bs.

(And because we have to check whether @c is NULL anyway, there is no
real reason to use child_bs(c) instead of c->bs afterwards.)

>> +    }
> Reviewed-by: Andrey Shinkevich 

Thanks a lot for reviewing!



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] Remove the CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE switch

[Daniel P . Berrangé]

On Thu, Jul 09, 2020 at 07:34:56AM +0200, Thomas Huth wrote:
> GCC supports "#pragma GCC diagnostic" since version 4.6, and
> Clang seems to support it, too, since its early versions 3.x.
> That means that our minimum required compiler versions all support
> this pragma already and we can remove the test from configure and
> all the related #ifdefs in the code.
> 
> Signed-off-by: Thomas Huth 
> ---
>  configure | 29 -
>  include/ui/gtk.h  |  4 
>  include/ui/qemu-pixman.h  |  4 
>  scripts/decodetree.py | 12 
>  ui/gtk.c  |  4 
>  util/coroutine-ucontext.c |  4 
>  6 files changed, 4 insertions(+), 53 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH v2 2/2] GitLab Gating CI: initial set of jobs, documentation and scripts

[Erik Skultety]

On Wed, Jul 08, 2020 at 10:46:57PM -0400, Cleber Rosa wrote:
> This is a mapping of Peter's "remake-merge-builds" and
> "pull-buildtest" scripts, gone through some updates, adding some build
> option and removing others.
>
> The jobs currently cover the machines that the QEMU project owns, and that
> are setup and ready to run jobs:
>
>  - Ubuntu 18.04 on S390x
>  - Ubuntu 20.04 on aarch64
>
> During the development of this set of jobs, the GitLab CI was tested
> with many other architectures, including ppc64, s390x and aarch64,
> along with the other OSs (not included here):
>
>  - Fedora 30
>  - FreeBSD 12.1
>
> More information can be found in the documentation itself.
>
> Signed-off-by: Cleber Rosa 
> ---
>  .gitlab-ci.d/gating.yml| 146 +
>  .gitlab-ci.yml |   1 +
>  docs/devel/testing.rst | 147 +
>  scripts/ci/setup/build-environment.yml | 217 +
>  scripts/ci/setup/gitlab-runner.yml |  72 
>  scripts/ci/setup/inventory |   2 +
>  scripts/ci/setup/vars.yml  |  13 ++
>  7 files changed, 598 insertions(+)
>  create mode 100644 .gitlab-ci.d/gating.yml
>  create mode 100644 scripts/ci/setup/build-environment.yml
>  create mode 100644 scripts/ci/setup/gitlab-runner.yml
>  create mode 100644 scripts/ci/setup/inventory
>  create mode 100644 scripts/ci/setup/vars.yml
>
> diff --git a/.gitlab-ci.d/gating.yml b/.gitlab-ci.d/gating.yml
> new file mode 100644
> index 00..5562df5708
> --- /dev/null
> +++ b/.gitlab-ci.d/gating.yml
> @@ -0,0 +1,146 @@
> +variables:
> +  GIT_SUBMODULE_STRATEGY: recursive
> +
> +# All ubuntu-18.04 jobs should run successfully in an environment
> +# setup by the scripts/ci/setup/build-environment.yml task
> +# "Install basic packages to build QEMU on Ubuntu 18.04/20.04"
> +ubuntu-18.04-s390x-all-linux-static:
> + tags:
> + - ubuntu_18.04
> + - s390x
> + rules:
> + - if: '$CI_COMMIT_REF_NAME == "staging"'
> + script:
> + # --disable-libssh is needed because of 
> https://bugs.launchpad.net/qemu/+bug/1838763
> + # --disable-glusterfs is needed because there's no static version of those 
> libs in distro supplied packages
> + - ./configure --enable-debug --static --disable-system --disable-glusterfs 
> --disable-libssh
> + - make --output-sync -j`nproc`
> + - make --output-sync -j`nproc` check V=1
> + - make --output-sync -j`nproc` check-tcg V=1

I know this patch doesn't introduce a FreeBSD job, but later in the patch it's
clear you'd want to introduce them at some point, so:
'nproc' doesn't exist on FreeBSD, but `getconf _NPROCESSORS_ONLN` does, so you
may want to use it right from the start.

...

> +
> +CI
> +==
> +
> +QEMU has configurations enabled for a number of different CI services.
> +The most update information about them and their status can be found

s/update/up to date/

> +at::
> +
> +   https://wiki.qemu.org/Testing/CI
> +
> +Gating CI
> +--
> +
> +A Pull Requests will only to be merged if they successfully go through

s/A /
s/to be/be/

> +a different set of CI jobs.  GitLab's CI is the service/framework used

s/a different set/different sets/   (I may be wrong with this one)

...

> +
> +gitlab-runner setup and registration
> +
> +
> +The gitlab-runner agent needs to be installed on each machine that
> +will run jobs.  The association between a machine and a GitLab project
> +happens with a registration token.  To find the registration token for
> +your repository/project, navigate on GitLab's web UI to:
> +
> + * Settings (the gears like icon), then
> + * CI/CD, then
> + * Runners, and click on the "Expand" button, then
> + * Under "Set up a specific Runner manually", look for the value under
> +   "Use the following registration token during setup"
> +
> +Edit the ``scripts/ci/setup/vars.yml`` file, setting the
> +``gitlab_runner_registration_token`` variable to the value obtained
> +earlier.
> +
> +.. note:: gitlab-runner is not available from the standard location
> +  for all OS and architectures combinations.  For some systems,
> +  a custom build may be necessary.  Some builds are avaiable
> +  at https://cleber.fedorapeople.org/gitlab-runner/ and this
> +  URI may be used as a value on ``vars.yml``
> +
> +To run the playbook, execute::
> +
> +  cd scripts/ci/setup
> +  ansible-playbook -i inventory gitlab-runner.yml
> +
> +.. note:: there are currently limitations to gitlab-runner itself when
> +  setting up a service under FreeBSD systems.  You will need to
> +  perform steps 4 to 10 manually, as described at
> +  https://docs.gitlab.com/runner/install/freebsd.html

What kinds of limitations? Is it architecture constrained maybe? I'm asking
because we have all of the steps covered by an Ansible playbook, so I kinda got
triggered by the word "manually". Also, the document only mentions 9 steps
overall.

Re: [PATCH 2/2] util/qemu-sockets: make keep-alive enabled by default

[Denis V. Lunev]

On 7/9/20 11:29 AM, Daniel P. Berrangé wrote:
> On Wed, Jul 08, 2020 at 10:15:39PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Keep-alive won't hurt, let's try to enable it even if not requested by
>> user.
> Keep-alive intentionally breaks TCP connections earlier than normal
> in face of transient networking problems.
>
> The question is more about which type of pain is more desirable. A
> stall in the network connection (for a potentially very long time),
> or an intentionally broken socket.
>
> I'm not at all convinced it is a good idea to intentionally break
> /all/ QEMU sockets in the face of transient problems, even if the
> problems last for 2 hours or more. 
>
> I could see keep-alives being ok on some QEMU socket. For example
> VNC/SPICE clients, as there is no downside to proactively culling
> them as they can trivially reconnect. Migration too is quite
> reasonable to use keep alives, as you generally want migration to
> run to completion in a short amount of time, and aborting migration
> needs to be safe no matter what.
>
> Breaking chardevs or block devices or network devices that use
> QEMU sockets though will be disruptive. The only solution once
> those backends have a dead socket is going to be to kill QEMU
> and cold-boot the VM again.

nope, and this is exactly what we are trying to achive.

Let us assume that QEMU NBD is connected to the
outside world, f.e. to some HA service running in
other virtual machine. Once that far away VM is
becoming dead, it is re-started on some other host
with the same IP.

QEMU NBD has an ability to reconnect to this same
endpoint and this process is transient for the guest.

This is the workflow we are trying to improve.

Anyway, sitting over dead socket is somewhat
which is not productive. This is like NFS hard and
soft mounts. In hypervisor world using hard mounts
(defaults before the patch) leads to various non
detectable deadlocks, that is why we are proposing
soft with such defaults.

It should also be noted that this is more consistent
as we could face the problem if we perform write
to the dead socket OR we could hang forever, thus
the problem with the current state is still possible.
With new settings we would consistently observe
the problem.

Den

Re: [PATCH v10 02/34] qcow2: Convert qcow2_get_cluster_offset() into qcow2_get_host_offset()

[Max Reitz]

On 03.07.20 17:57, Alberto Garcia wrote:
> qcow2_get_cluster_offset() takes an (unaligned) guest offset and
> returns the (aligned) offset of the corresponding cluster in the qcow2
> image.
> 
> In practice none of the callers need to know where the cluster starts
> so this patch makes the function calculate and return the final host
> offset directly. The function is also renamed accordingly.
> 
> There is a pre-existing exception with compressed clusters: in this
> case the function returns the complete cluster descriptor (containing
> the offset and size of the compressed data). This does not change with
> this patch but it is now documented.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.h |  4 ++--
>  block/qcow2-cluster.c | 41 +++--
>  block/qcow2.c | 24 +++-
>  3 files changed, 32 insertions(+), 37 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 1/2] GitLab Gating CI: introduce pipeline-status contrib script

[Philippe Mathieu-Daudé]

On 7/9/20 10:55 AM, Erik Skultety wrote:
> On Wed, Jul 08, 2020 at 10:46:56PM -0400, Cleber Rosa wrote:
>> This script is intended to be used right after a push to a branch.
>>
>> By default, it will look for the pipeline associated with the commit
>> that is the HEAD of the *local* staging branch.  It can be used as a
>> one time check, or with the `--wait` option to wait until the pipeline
>> completes.
>>
>> If the pipeline is successful, then a merge of the staging branch into
>> the master branch should be the next step.
>>
>> Signed-off-by: Cleber Rosa 
>> ---
>>  scripts/ci/gitlab-pipeline-status | 156 ++
>>  1 file changed, 156 insertions(+)
>>  create mode 100755 scripts/ci/gitlab-pipeline-status
>>
>> diff --git a/scripts/ci/gitlab-pipeline-status 
>> b/scripts/ci/gitlab-pipeline-status
>> new file mode 100755
>> index 00..4a9de39872
>> --- /dev/null
>> +++ b/scripts/ci/gitlab-pipeline-status
>> @@ -0,0 +1,156 @@
>> +#!/usr/bin/env python3
>> +#
>> +# Copyright (c) 2019-2020 Red Hat, Inc.
>> +#
>> +# Author:
>> +#  Cleber Rosa 
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or
>> +# later.  See the COPYING file in the top-level directory.
>> +
>> +"""
>> +Checks the GitLab pipeline status for a given commit commit
> 
> s/commit$/(hash|sha|ID|)
> 
>> +"""
>> +
>> +# pylint: disable=C0103
>> +
>> +import argparse
>> +import http.client
>> +import json
>> +import os
>> +import subprocess
>> +import time
>> +import sys
>> +
>> +
>> +def get_local_staging_branch_commit():
>> +"""
>> +Returns the commit sha1 for the *local* branch named "staging"
>> +"""
>> +result = subprocess.run(['git', 'rev-parse', 'staging'],
> 
> If one day Peter decides that "staging" is not a cool name anymore and use a
> different name for the branch :) we should account for that and make it a
> variable, possibly even parametrize this function with it.

This script can be used by any fork, not only Peter.
So having a parameter (default to 'staging') is a requisite IMO.

>> +stdin=subprocess.DEVNULL,
>> +stdout=subprocess.PIPE,
>> +stderr=subprocess.DEVNULL,
>> +cwd=os.path.dirname(__file__),
>> +universal_newlines=True).stdout.strip()
>> +if result == 'staging':
>> +raise ValueError("There's no local staging branch")
> 
> "There's no local branch named 'staging'" would IMO be more descriptive, so as
> not to confuse it with staging in git.
> 
>> +if len(result) != 40:
>> +raise ValueError("Branch staging HEAD doesn't look like a sha1")
>> +return result
>> +
>> +
>> +def get_pipeline_status(project_id, commit_sha1):
>> +"""
>> +Returns the JSON content of the pipeline status API response
>> +"""
>> +url = '/api/v4/projects/{}/pipelines?sha={}'.format(project_id,
>> +commit_sha1)
>> +connection = http.client.HTTPSConnection('gitlab.com')
>> +connection.request('GET', url=url)
>> +response = connection.getresponse()
>> +if response.code != http.HTTPStatus.OK:
>> +raise ValueError("Failed to receive a successful response")
>> +json_response = json.loads(response.read())
> 
> a blank line separating the commentary block would slightly help readability
> 
>> +# afaict, there should one one pipeline for the same project + commit
> 
> s/one one/be only one/

'afaict' is not a word.

> 
>> +# if this assumption is false, we can add further filters to the
>> +# url, such as username, and order_by.
>> +if not json_response:
>> +raise ValueError("No pipeline found")
>> +return json_response[0]
>> +
>> +
>> +def wait_on_pipeline_success(timeout, interval,
>> + project_id, commit_sha):
>> +"""
>> +Waits for the pipeline to end up to the timeout given
> 
> "Waits for the pipeline to finish within the given timeout"
> 
>> +"""
>> +start = time.time()
>> +while True:
>> +if time.time() >= (start + timeout):
>> +print("Waiting on the pipeline success timed out")
> 
> s/success//
> (the pipeline doesn't always have to finish with success)
> 
>> +return False
>> +
>> +status = get_pipeline_status(project_id, commit_sha)
>> +if status['status'] == 'running':
>> +time.sleep(interval)
>> +print('running...')

If we want to automate the use of this script by a daemon, it would
be better to use the logging class. Then maybe 'running...' is for
the DEBUG level, Other print() calls can be updated to WARN/INFO
levels.

>> +continue
>> +
>> +if status['status'] == 'success':
>> +return True
>> +
>> +msg = "Pipeline ended unsuccessfully, check: %s" % status['web_url']
> 
> I think more common expression is "Pipeline failed"
> 

Re: [PULL 0/3] M68k next patches

[Peter Maydell]

On Mon, 6 Jul 2020 at 21:06, Laurent Vivier  wrote:
>
> The following changes since commit 64f0ad8ad8e13257e7c912df470d46784b55c3fd:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2020-07-02' 
> into staging (2020-07-02 15:54:09 +0100)
>
> are available in the Git repository at:
>
>   git://github.com/vivier/qemu-m68k.git tags/m68k-next-pull-request
>
> for you to fetch changes up to d159dd058c7dc48a9291fde92eaae52a9f26a4d1:
>
>   softfloat,m68k: disable floatx80_invalid_encoding() for m68k (2020-07-06 
> 21:41:52 +0200)
>
> 
> m68k pull-request 20200706
>
> disable floatx80_invalid_encoding() for m68k
> fix m68k_cpu_get_phys_page_debug()
>


Applied, thanks.

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

-- PMM

Re: [PATCH v2 7/7] virtio-scsi: use scsi_device_get

[Maxim Levitsky]

On Wed, 2020-05-27 at 16:50 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:51PM +0300, Maxim Levitsky wrote:
> > This will help us to avoid the scsi device disappearing
> > after we took a reference to it.
> > 
> > It doesn't by itself forbid case when we try to access
> > an unrealized device
> > 
> > Suggested-by: Stefan Hajnoczi 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  hw/scsi/virtio-scsi.c | 23 +++
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> I'm not very familiar with the SCSI emulation code, but this looks
> correct. My understanding of what this patch does:
> 
> This patch keeps SCSIDevice alive between scsi_device_find() and
> scsi_req_new(). Previously no SCSIDevice ref was taken so the device
> could have been freed before scsi_req_new() had a chance to take a ref.
Yep, I also verified now that this is what happens.

> 
> The TMF case is similar: the SCSIDevice ref must be held during
> virtio_scsi_do_tmf(). We don't need to worry about the async cancel
> notifiers because the request being canceled already holds a ref.
This code I understand less to be honest, but in the worst case,
the patch shoudn't make it worse.

Best regards,
Maxim Levitsky

Re: [PATCH] tests: improve performance of device-introspect-test

[Laurent Vivier]

On 09/07/2020 13:28, Daniel P. Berrangé wrote:
> Total execution time with "-m slow" and x86_64 QEMU, drops from 3
> minutes 15 seconds, down to 54 seconds.
> 
> Individual tests drop from 17-20 seconds, down to 3-4 seconds.
> 
> The cost of this change is that any QOM bugs resulting in the test
> failure will not be directly associated with the device that caused
> the failure. The test case is not frequently identifying such bugs
> though, and the cause is likely easily visible in the patch series
> that causes the failure. So overall the shorter running time is
> considered the more important factor.

You don't report the test to test_device_intro_none() and
test_device_intro_abstract(): is it intended ?

Thanks,
Laurent

Re: [PATCH v2 2/7] Implement drain_call_rcu and use it in hmp_device_del

[Maxim Levitsky]

On Thu, 2020-07-09 at 13:42 +0200, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > This allows to preserve the semantics of hmp_device_del,
> > that the device is deleted immediatly which was changed by previos
> > patch that delayed this to RCU callback
> > 
> > Suggested-by: Stefan Hajnoczi 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  include/qemu/rcu.h |  1 +
> >  qdev-monitor.c |  3 +++
> >  util/rcu.c | 33 +
> >  3 files changed, 37 insertions(+)
> > 
> > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> > index 570aa603eb..0e375ebe13 100644
> > --- a/include/qemu/rcu.h
> > +++ b/include/qemu/rcu.h
> > @@ -133,6 +133,7 @@ struct rcu_head {
> >  };
> >  
> >  extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> > +extern void drain_call_rcu(void);
> >  
> >  /* The operands of the minus operator must have the same type,
> >   * which must be the one that we specify in the cast.
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 56cee1483f..70877840a2 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, 
> > Error **errp)
> >  return;
> >  }
> >  dev = qdev_device_add(opts, _err);
> > +drain_call_rcu();
> > +
> >  if (!dev) {
> >  error_propagate(errp, local_err);
> >  qemu_opts_del(opts);
> > @@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp)
> >  }
> >  
> >  qdev_unplug(dev, errp);
> > +drain_call_rcu();
> >  }
> >  }
> >  
> 
> Subject claims "in hmp_device_del", code has it in qmp_device_add() and
> qmp_device_del().  Please advise.

I added it in both, because addition of a device can fail and trigger removal,
which can also be now delayed due to RCU.
Since both device_add and device_del aren't used often, the overhead won't
be a problem IMHO.

Best regards,
Maxim Levitsky

> 
> [...]

Re: Migrating custom qemu.org infrastructure to GitLab

[Thomas Huth]

On 09/07/2020 12.33, Paolo Bonzini wrote:
> On 09/07/20 12:22, Thomas Huth wrote:
>> FWIW, seems like we could use the "pandoc" tool to convert Mediawiki
>> (our old Wiki) to Markdown (Gitlab wiki). I've done a quick test and
>> converted https://wiki.qemu.org/Contribute/MailingLists into
>> https://gitlab.com/huth/qemu/-/wikis/Contribute/MailingLists with some
>> few clicks.
>>
>> But the longer I look at most Wiki pages, the more I think that we
>> should convert the important pages rather into a part of qemu-web
>> instead. I'll have a closer look and will suggest some patches when time
>> permits...
> 
> The wiki was cleaned up more or less at the same time as the
> qemu-web.git repo was created (actually as a prerequisite), it's
> actually not in a bad shape.

Agreed, there were definitely worse times in the past.

> We can certainly move some wiki pages to qemu-web, like we did for
> "report a bug" in the past and like Alex did recently for the
> Conservancy page.  But I think there aren't that many left, most of them
> are in the first category above and should be moved to docs/devel (for
> example https://wiki.qemu.org/Contribute/SubmitAPatch).

I was looking at pages like
https://wiki.qemu.org/Contribute/MailingLists and
https://wiki.qemu.org/License ... but yes, the first one should be
likely moved to docs/devel, too, and the second one is maybe redundant
anyway, since we have this information in the LICENSE file already (so
we could also link to
https://git.qemu.org/?p=qemu.git;a=blob_plain;f=LICENSE instead).

 Thomas

Re: [PULL 00/41] testing updates (vm, gitlab, misc build fixes)

[Peter Maydell]

On Thu, 9 Jul 2020 at 13:24, Philippe Mathieu-Daudé  wrote:
> libssh is bugged on Ubuntu 18.04.
> https://bugs.launchpad.net/qemu/+bug/1838763
>
> We need to use 'configure --disable-libssh' there.

Ah, thanks. I guess libssh recently got installed on that
box.

-- PMM

[PATCH 1/6] block/aio_task: allow start/wait task from any coroutine

[Denis V. Lunev]

From: Vladimir Sementsov-Ogievskiy 

Currently, aio task pool assumes that there is a main coroutine, which
creates tasks and wait for them. Let's remove the restriction by using
CoQueue. Code becomes clearer, interface more obvious.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Vladimir Sementsov-Ogievskiy 
CC: Denis Plotnikov 
---
 block/aio_task.c | 21 ++---
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/block/aio_task.c b/block/aio_task.c
index 88989fa248..cf62e5c58b 100644
--- a/block/aio_task.c
+++ b/block/aio_task.c
@@ -27,11 +27,10 @@
 #include "block/aio_task.h"
 
 struct AioTaskPool {
-Coroutine *main_co;
 int status;
 int max_busy_tasks;
 int busy_tasks;
-bool waiting;
+CoQueue waiters;
 };
 
 static void coroutine_fn aio_task_co(void *opaque)
@@ -52,31 +51,23 @@ static void coroutine_fn aio_task_co(void *opaque)
 
 g_free(task);
 
-if (pool->waiting) {
-pool->waiting = false;
-aio_co_wake(pool->main_co);
-}
+qemu_co_queue_restart_all(>waiters);
 }
 
 void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool)
 {
 assert(pool->busy_tasks > 0);
-assert(qemu_coroutine_self() == pool->main_co);
 
-pool->waiting = true;
-qemu_coroutine_yield();
+qemu_co_queue_wait(>waiters, NULL);
 
-assert(!pool->waiting);
 assert(pool->busy_tasks < pool->max_busy_tasks);
 }
 
 void coroutine_fn aio_task_pool_wait_slot(AioTaskPool *pool)
 {
-if (pool->busy_tasks < pool->max_busy_tasks) {
-return;
+while (pool->busy_tasks >= pool->max_busy_tasks) {
+aio_task_pool_wait_one(pool);
 }
-
-aio_task_pool_wait_one(pool);
 }
 
 void coroutine_fn aio_task_pool_wait_all(AioTaskPool *pool)
@@ -98,8 +89,8 @@ AioTaskPool *coroutine_fn aio_task_pool_new(int 
max_busy_tasks)
 {
 AioTaskPool *pool = g_new0(AioTaskPool, 1);
 
-pool->main_co = qemu_coroutine_self();
 pool->max_busy_tasks = max_busy_tasks;
+qemu_co_queue_init(>waiters);
 
 return pool;
 }
-- 
2.17.1

[PATCH v8 0/6] block: seriously improve savevm/loadvm performance

[Denis V. Lunev]

This series do standard basic things:
- it creates intermediate buffer for all writes from QEMU migration code
  to QCOW2 image,
- this buffer is sent to disk asynchronously, allowing several writes to
  run in parallel.

In general, migration code is fantastically inefficent (by observation),
buffers are not aligned and sent with arbitrary pieces, a lot of time
less than 100 bytes at a chunk, which results in read-modify-write
operations with non-cached operations. It should also be noted that all
operations are performed into unallocated image blocks, which also suffer
due to partial writes to such new clusters.

This patch series is an implementation of idea discussed in the RFC
posted by Denis Plotnikov
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg01925.html
Results with this series over NVME are better than original code
original rfcthis
cached:  1.79s  2.38s   1.27s
non-cached:  3.29s  1.31s   0.81s

Changes from v7:
- dropped lock from LoadVMState
- fixed assert in last patch
- dropped patch 1 as queued

Changes from v6:
- blk_load_vmstate kludges added (patchew problem fixed)

Changes from v5:
- loadvm optimizations added with Vladimir comments included

Changes from v4:
- added patch 4 with blk_save_vmstate() cleanup
- added R-By
- bdrv_flush_vmstate -> bdrv_finalize_vmstate
- fixed return code of bdrv_co_do_save_vmstate
- fixed typos in comments (Eric, thanks!)
- fixed patchew warnings

Changes from v3:
- rebased to master
- added patch 3 which removes aio_task_pool_wait_one()
- added R-By to patch 1
- patch 4 is rewritten via bdrv_run_co
- error path in blk_save_vmstate() is rewritten to call bdrv_flush_vmstate
  unconditionally
- added some comments
- fixes initialization in bdrv_co_vmstate_save_task_entry as suggested

Changes from v2:
- code moved from QCOW2 level to generic block level
- created bdrv_flush_vmstate helper to fix 022, 029 tests
- added recursive for bs->file in bdrv_co_flush_vmstate (fix 267)
- fixed blk_save_vmstate helper
- fixed coroutine wait as Vladimir suggested with waiting fixes from me

Changes from v1:
- patchew warning fixed
- fixed validation that only 1 waiter is allowed in patch 1

Signed-off-by: Denis V. Lunev 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Juan Quintela 
CC: "Dr. David Alan Gilbert" 
CC: Vladimir Sementsov-Ogievskiy 
CC: Denis Plotnikov 

[Bug 1886811] Re: systemd complains Failed to enqueue loopback interface start request: Operation not supported

[Bug Watch Updater]

** Changed in: qemu (Debian)
   Status: Unknown => Confirmed

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1886811

Title:
  systemd complains Failed to enqueue loopback interface start request:
  Operation not supported

Status in QEMU:
  In Progress
Status in qemu package in Debian:
  Confirmed

Bug description:
  This symptom seems similar to
  https://bugs.launchpad.net/qemu/+bug/1823790

  Host Linux: Debian 11 Bullseye (testing) on x84-64 architecture
  qemu version: latest git of git commit hash 
eb2c66b10efd2b914b56b20ae90655914310c925
  compiled with "./configure --static --disable-system" 

  Down stream bug report at 
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=964289
  Bug report (closed) to systemd: 
https://github.com/systemd/systemd/issues/16359

  systemd in armhf and armel (both little endian 32-bit) containers fail to 
start with
  Failed to enqueue loopback interface start request: Operation not supported

  How to reproduce on Debian (and probably Ubuntu):
  mmdebstrap --components="main contrib non-free" --architectures=armhf 
--variant=important bullseye /var/lib/machines/armhf-bullseye
  systemd-nspawn -D /var/lib/machines/armhf-bullseye -b

  When "armhf" architecture is replaced with "mips" (32-bit big endian) or 
"ppc64"
  (64-bit big endian), the container starts up fine.

  The same symptom is also observed with "powerpc" (32-bit big endian)
  architecture.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1886811/+subscriptions

Re: [PATCH] tests/qtest/fuzz: Add missing spaces in description

[Alexander Bulekov]

On 200709 1228, Philippe Mathieu-Daudé wrote:
> On 7/9/20 10:37 AM, Thomas Huth wrote:
> > There should be a space between "forking" and "for".
> > 
> > Signed-off-by: Thomas Huth 
> > ---
> >  tests/qtest/fuzz/virtio_scsi_fuzz.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/qtest/fuzz/virtio_scsi_fuzz.c 
> > b/tests/qtest/fuzz/virtio_scsi_fuzz.c
> > index 51dce491ab..3a9ea13736 100644
> > --- a/tests/qtest/fuzz/virtio_scsi_fuzz.c
> > +++ b/tests/qtest/fuzz/virtio_scsi_fuzz.c
> > @@ -191,7 +191,7 @@ static void register_virtio_scsi_fuzz_targets(void)
> >  {
> >  fuzz_add_qos_target(&(FuzzTarget){
> >  .name = "virtio-scsi-fuzz",
> > -.description = "Fuzz the virtio-scsi virtual queues, 
> > forking"
> > +.description = "Fuzz the virtio-scsi virtual queues, 
> > forking "
> >  "for each fuzz run",
> >  .pre_vm_init = _shm_init,
> >  .pre_fuzz = _scsi_pre_fuzz,
> > @@ -202,7 +202,7 @@ static void register_virtio_scsi_fuzz_targets(void)
> >  
> >  fuzz_add_qos_target(&(FuzzTarget){
> >  .name = "virtio-scsi-flags-fuzz",
> > -.description = "Fuzz the virtio-scsi virtual queues, 
> > forking"
> > +.description = "Fuzz the virtio-scsi virtual queues, 
> > forking "
> >  "for each fuzz run (also fuzzes the virtio flags)",
> >  .pre_vm_init = _shm_init,
> >  .pre_fuzz = _scsi_pre_fuzz,
> > 
> 
> Uh I thought we had fixed these already :/
> 
> Reviewed-by: Philippe Mathieu-Daudé 

Same..
Reviewed-by: Alexander Bulekov 

[PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset

[Alex Bennée]

Any write to a device might cause a re-arrangement of memory
triggering a TLB flush and potential re-size of the TLB invalidating
previous entries. This would cause users of qemu_plugin_get_hwaddr()
to see the warning:

  invalid use of qemu_plugin_get_hwaddr

because of the failed tlb_lookup which should always succeed. To
prevent this we save the IOTLB data in case it is later needed by a
plugin doing a lookup.

Signed-off-by: Alex Bennée 

---
v2
  - save the entry instead of re-running the tlb_fill.
v3
  - don't abuse TLS, use CPUState to store data
  - just use g_free_rcu() to avoid ugliness
  - verify addr matches before returning data
  - ws fix
---
 include/hw/core/cpu.h   |  4 +++
 include/qemu/typedefs.h |  1 +
 accel/tcg/cputlb.c  | 57 +++--
 3 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index b3f4b7931823..bedbf098dc57 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -417,7 +417,11 @@ struct CPUState {
 
 DECLARE_BITMAP(plugin_mask, QEMU_PLUGIN_EV_MAX);
 
+#ifdef CONFIG_PLUGIN
 GArray *plugin_mem_cbs;
+/* saved iotlb data from io_writex */
+SavedIOTLB *saved_iotlb;
+#endif
 
 /* TODO Move common fields from CPUArchState here. */
 int cpu_index;
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 15f5047bf1dc..427027a9707a 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -116,6 +116,7 @@ typedef struct QObject QObject;
 typedef struct QString QString;
 typedef struct RAMBlock RAMBlock;
 typedef struct Range Range;
+typedef struct SavedIOTLB SavedIOTLB;
 typedef struct SHPCDevice SHPCDevice;
 typedef struct SSIBus SSIBus;
 typedef struct VirtIODevice VirtIODevice;
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 1e815357c709..8636b66e036a 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1073,6 +1073,42 @@ static uint64_t io_readx(CPUArchState *env, 
CPUIOTLBEntry *iotlbentry,
 return val;
 }
 
+#ifdef CONFIG_PLUGIN
+
+typedef struct SavedIOTLB {
+struct rcu_head rcu;
+hwaddr addr;
+MemoryRegionSection *section;
+hwaddr mr_offset;
+} SavedIOTLB;
+
+/*
+ * Save a potentially trashed IOTLB entry for later lookup by plugin.
+ *
+ * We also need to track the thread storage address because the RCU
+ * cleanup that runs when we leave the critical region (the current
+ * execution) is actually in a different thread.
+ */
+static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection 
*section, hwaddr mr_offset)
+{
+SavedIOTLB *old, *new = g_new(SavedIOTLB, 1);
+new->addr = addr;
+new->section = section;
+new->mr_offset = mr_offset;
+old = atomic_rcu_read(>saved_iotlb);
+atomic_rcu_set(>saved_iotlb, new);
+if (old) {
+g_free_rcu(old, rcu);
+}
+}
+
+#else
+static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection 
*section, hwaddr mr_offset)
+{
+/* do nothing */
+}
+#endif
+
 static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
   int mmu_idx, uint64_t val, target_ulong addr,
   uintptr_t retaddr, MemOp op)
@@ -1092,6 +1128,12 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry 
*iotlbentry,
 }
 cpu->mem_io_pc = retaddr;
 
+/*
+ * The memory_region_dispatch may trigger a flush/resize
+ * so for plugins we save the iotlb_data just in case.
+ */
+save_iotlb_data(cpu, iotlbentry->addr, section, mr_offset);
+
 if (mr->global_locking && !qemu_mutex_iothread_locked()) {
 qemu_mutex_lock_iothread();
 locked = true;
@@ -1381,8 +1423,11 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
  * in the softmmu lookup code (or helper). We don't handle re-fills or
  * checking the victim table. This is purely informational.
  *
- * This should never fail as the memory access being instrumented
- * should have just filled the TLB.
+ * This almost never fails as the memory access being instrumented
+ * should have just filled the TLB. The one corner case is io_writex
+ * which can cause TLB flushes and potential resizing of the TLBs
+ * loosing the information we need. In those cases we need to recover
+ * data from a copy of the io_tlb entry.
  */
 
 bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
@@ -1406,6 +1451,14 @@ bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, 
int mmu_idx,
 data->v.ram.hostaddr = addr + tlbe->addend;
 }
 return true;
+} else {
+SavedIOTLB *saved = atomic_rcu_read(>saved_iotlb);
+if (saved && saved->addr == tlb_addr) {
+data->is_io = true;
+data->v.io.section = saved->section;
+data->v.io.offset = saved->mr_offset;
+return true;
+}
 }
 return false;
 }
-- 
2.20.1

[PATCH v1 06/13] plugins: add API to return a name for a IO device

[Alex Bennée]

This may well end up being anonymous but it should always be unique.

Signed-off-by: Alex Bennée 
[r-b provisional given change to g_intern_string]
Reviewed-by: Clement Deschamps 
Reviewed-by: Emilio G. Cota 

---
v3
  - return a non-freeable const g_intern_string()
  - checkpatch cleanups
---
 include/qemu/qemu-plugin.h |  6 ++
 plugins/api.c  | 20 
 2 files changed, 26 insertions(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index bab8b0d4b3af..c98c18d6b052 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -335,6 +335,12 @@ struct qemu_plugin_hwaddr 
*qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
 bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
 uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr 
*haddr);
 
+/*
+ * Returns a string representing the device. The string is valid for
+ * the lifetime of the plugin.
+ */
+const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);
+
 typedef void
 (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
  qemu_plugin_meminfo_t info, uint64_t vaddr,
diff --git a/plugins/api.c b/plugins/api.c
index bbdc5a4eb46d..4304e63f0cf8 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -303,6 +303,26 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct 
qemu_plugin_hwaddr *haddr
 return 0;
 }
 
+const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)
+{
+#ifdef CONFIG_SOFTMMU
+if (h && h->is_io) {
+MemoryRegionSection *mrs = h->v.io.section;
+if (!mrs->mr->name) {
+unsigned long maddr = 0x & (uintptr_t) mrs->mr;
+g_autofree char *temp = g_strdup_printf("anon%08lx", maddr);
+return g_intern_string(temp);
+} else {
+return g_intern_string(mrs->mr->name);
+}
+} else {
+return g_intern_string("RAM");
+}
+#else
+return g_intern_string("Invalid");
+#endif
+}
+
 /*
  * Queries to the number and potential maximum number of vCPUs there
  * will be. This helps the plugin dimension per-vcpu arrays.
-- 
2.20.1

[PATCH v1 08/13] plugins: expand the bb plugin to be thread safe and track per-cpu

[Alex Bennée]

While there isn't any easy way to make the inline counts thread safe
we can ensure the callback based ones are. While we are at it we can
reduce introduce a new option ("idle") to dump a report of the current
bb and insn count each time a vCPU enters the idle state.

Signed-off-by: Alex Bennée 
Cc: Dave Bort 

---
v2
  - fixup for non-inline linux-user case
  - minor cleanup and re-factor
---
 tests/plugin/bb.c | 96 ---
 1 file changed, 83 insertions(+), 13 deletions(-)

diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
index df19fd359df3..89c373e19cd8 100644
--- a/tests/plugin/bb.c
+++ b/tests/plugin/bb.c
@@ -16,24 +16,67 @@
 
 QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
 
-static uint64_t bb_count;
-static uint64_t insn_count;
+typedef struct {
+GMutex lock;
+int index;
+uint64_t bb_count;
+uint64_t insn_count;
+} CPUCount;
+
+/* Used by the inline & linux-user counts */
 static bool do_inline;
+static CPUCount inline_count;
+
+/* Dump running CPU total on idle? */
+static bool idle_report;
+static GPtrArray *counts;
+static int max_cpus;
+
+static void gen_one_cpu_report(CPUCount *count, GString *report)
+{
+if (count->bb_count) {
+g_string_append_printf(report, "CPU%d: "
+   "bb's: %" PRIu64", insns: %" PRIu64 "\n",
+   count->index,
+   count->bb_count, count->insn_count);
+}
+}
 
 static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
-g_autofree gchar *out = g_strdup_printf(
-"bb's: %" PRIu64", insns: %" PRIu64 "\n",
-bb_count, insn_count);
-qemu_plugin_outs(out);
+g_autoptr(GString) report = g_string_new("");
+
+if (do_inline || !max_cpus) {
+g_string_printf(report, "bb's: %" PRIu64", insns: %" PRIu64 "\n",
+inline_count.bb_count, inline_count.insn_count);
+} else {
+g_ptr_array_foreach(counts, (GFunc) gen_one_cpu_report, report);
+}
+qemu_plugin_outs(report->str);
+}
+
+static void vcpu_idle(qemu_plugin_id_t id, unsigned int cpu_index)
+{
+CPUCount *count = g_ptr_array_index(counts, cpu_index);
+g_autoptr(GString) report = g_string_new("");
+gen_one_cpu_report(count, report);
+
+if (report->len > 0) {
+g_string_prepend(report, "Idling ");
+qemu_plugin_outs(report->str);
+}
 }
 
 static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
 {
-unsigned long n_insns = (unsigned long)udata;
+CPUCount *count = max_cpus ?
+g_ptr_array_index(counts, cpu_index) : _count;
 
-insn_count += n_insns;
-bb_count++;
+unsigned long n_insns = (unsigned long)udata;
+g_mutex_lock(>lock);
+count->insn_count += n_insns;
+count->bb_count++;
+g_mutex_unlock(>lock);
 }
 
 static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
@@ -42,9 +85,9 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct 
qemu_plugin_tb *tb)
 
 if (do_inline) {
 qemu_plugin_register_vcpu_tb_exec_inline(tb, 
QEMU_PLUGIN_INLINE_ADD_U64,
- _count, 1);
+ _count.bb_count, 1);
 qemu_plugin_register_vcpu_tb_exec_inline(tb, 
QEMU_PLUGIN_INLINE_ADD_U64,
- _count, n_insns);
+ _count.insn_count, 
n_insns);
 } else {
 qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
  QEMU_PLUGIN_CB_NO_REGS,
@@ -56,8 +99,35 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t 
id,
const qemu_info_t *info,
int argc, char **argv)
 {
-if (argc && strcmp(argv[0], "inline") == 0) {
-do_inline = true;
+int i;
+
+for (i = 0; i < argc; i++) {
+char *opt = argv[i];
+if (g_strcmp0(opt, "inline") == 0) {
+do_inline = true;
+} else if (g_strcmp0(opt, "idle") == 0) {
+idle_report = true;
+} else {
+fprintf(stderr, "option parsing failed: %s\n", opt);
+return -1;
+}
+}
+
+if (info->system_emulation && !do_inline) {
+max_cpus = info->system.max_vcpus;
+counts = g_ptr_array_new();
+for (i = 0; i < max_cpus; i++) {
+CPUCount *count = g_new0(CPUCount, 1);
+g_mutex_init(>lock);
+count->index = i;
+g_ptr_array_add(counts, count);
+}
+} else if (!do_inline) {
+g_mutex_init(_count.lock);
+}
+
+if (idle_report) {
+qemu_plugin_register_vcpu_idle_cb(id, vcpu_idle);
 }
 
 qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
-- 
2.20.1

Updates on libcapstone?

[Philippe Mathieu-Daudé]

Hi Richard,

We are using libcapstone since almost 3 years for the arm/i386/ppc
'base' architectures.
The library is still optional in ./configure.
I wonder if we can make it a strong requisite, this way we could
get rid of disas/{arm.c,arm-a64.cc,i386.c,ppc.c} and the disas/libvixl
submodule, then having C++ compiler is not necessary anymore.

As of the release 4.0, the list of supported architectures is:

"ARM, ARM64 (ARMv8), Ethereum VM, M68K, Mips, PPC, Sparc, SystemZ,
TMS320C64X, M680X, XCore and X86 (including X86_64)."

m68k/mips/sparc/s390x are other candidates.

I remember you had a series related to capstone, but eventually there
was a problem so you postponed it until some patches were merged
upstream, do you remember?

Thanks for any update,

Phil.

Re: [PATCH v10 31/34] qcow2: Add the 'extended_l2' option and the QCOW2_INCOMPAT_EXTL2 bit

[Max Reitz]

On 03.07.20 17:58, Alberto Garcia wrote:
> Now that the implementation of subclusters is complete we can finally
> add the necessary options to create and read images with this feature,
> which we call "extended L2 entries".
> 
> Signed-off-by: Alberto Garcia 
> Reviewed-by: Eric Blake 
> Reviewed-by: Max Reitz 
> ---

(This requires quite a bit of a rebase, but nothing spectacularly
interesting, I think.)



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v10 34/34] iotests: Add tests for qcow2 images with extended L2 entries

[Max Reitz]

On 03.07.20 17:58, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia 
> ---
>  tests/qemu-iotests/271 | 901 +
>  tests/qemu-iotests/271.out | 724 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 1626 insertions(+)
>  create mode 100755 tests/qemu-iotests/271
>  create mode 100644 tests/qemu-iotests/271.out

[...]

> diff --git a/tests/qemu-iotests/271.out b/tests/qemu-iotests/271.out
> new file mode 100644
> index 00..07c1e7b46d
> --- /dev/null
> +++ b/tests/qemu-iotests/271.out
> @@ -0,0 +1,724 @@

[...]

> +### qemu-img amend ###
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> +qemu-img: Toggling extended L2 entries is not supported
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> +qemu-img: Toggling extended L2 entries is not supported

With Maxim’s patches, this code in patch 31 became unnecessary, so it
should now read “qemu-img: Invalid parameter 'extended_l2'\nThis option
is only supported for image creation”.

With that fixed:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] Fix MIPS add.s after 1ace099f2acb952eaaef0ba7725879949a7e4406

[Philippe Mathieu-Daudé]

Hi Aleksandar,

On 7/7/20 6:26 PM, Aleksandar Markovic wrote:
> On Fri, Jul 3, 2020 at 6:33 PM Alex Richardson
>  wrote:
>>
>> After merging latest QEMU upstream into our CHERI fork, I noticed that
>> some of the FPU tests in our MIPS baremetal testsuite
>> (https://github.com/CTSRD-CHERI/cheritest) started failing. It turns out
>> this commit accidentally changed add.s into a subtract.
>>
>> Signed-off-by: Alex Richardson 
>> ---
> 
> Applied to MIPS + TCG Continuous Benchmarking queue.

If you don't mind I'll include this patch for the mips pull request I
plan to send before hard freeze (on the list). I'm keeping your S-o-b.

> 
>>  target/mips/fpu_helper.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/mips/fpu_helper.c b/target/mips/fpu_helper.c
>> index 7a3a61cab3..56beda49d8 100644
>> --- a/target/mips/fpu_helper.c
>> +++ b/target/mips/fpu_helper.c
>> @@ -1221,7 +1221,7 @@ uint32_t helper_float_add_s(CPUMIPSState *env,
>>  {
>>  uint32_t wt2;
>>
>> -wt2 = float32_sub(fst0, fst1, >active_fpu.fp_status);
>> +wt2 = float32_add(fst0, fst1, >active_fpu.fp_status);
>>  update_fcr31(env, GETPC());
>>  return wt2;
>>  }
>> --
>> 2.27.0
>>
>>
> 

Re: [PATCH-for-5.1 2/2] fuzz: add missing header for rcu_enable_atfork

[Alexander Bulekov]

On 200709 0718, Thomas Huth wrote:
> On 08/07/2020 22.01, Alexander Bulekov wrote:
> > In 45222b9a90, I fixed a broken check for rcu_enable_atfork introduced
> > in d6919e4cb6. I added a call to rcu_enable_atfork after the
> > call to qemu_init in fuzz.c, but forgot to include the corresponding
> > header, breaking --enable-fuzzing --enable-werror builds.
> > 
> > Fixes: 45222b9a90 ("fuzz: fix broken qtest check at rcu_disable_atfork")
> > Signed-off-by: Alexander Bulekov 
> > ---
> >  tests/qtest/fuzz/fuzz.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
> > index a36d9038e0..0b66e43409 100644
> > --- a/tests/qtest/fuzz/fuzz.c
> > +++ b/tests/qtest/fuzz/fuzz.c
> > @@ -19,6 +19,7 @@
> >  #include "sysemu/runstate.h"
> >  #include "sysemu/sysemu.h"
> >  #include "qemu/main-loop.h"
> > +#include "qemu/rcu.h"
> >  #include "tests/qtest/libqtest.h"
> >  #include "tests/qtest/libqos/qgraph.h"
> >  #include "fuzz.h"
> 
> D'oh, mea culpa, I also apparently did not properly compile test that
> patch :-( I think we need a CI job that at least compile tests the
> fuzzing code - I can look into that once Alex Bennée's current testing
> pull request has been merged.

My bad - I should have done a clean build with a version of clang
that doesn't require me to -disable-werror

> Alexander, is there also a way to run a fuzzer just for some few
> minutes? E.g. a fuzzing test that finishes quickly, or an option to
> limit the time that a test is running? If so, we could also add that
> quick test to the CI pipeline, to make sure that the fuzzer code does
> not only compile, but is also able to run (at least a little bit).

Yes. I think the sequence could look something like:
CC=clang CXX=clang++ ../configure --enable-fuzzing --enable-sanitizers \
 --enable-werror
make i386-softmmu/fuzz
./i386-softmmu/qemu-fuzz-i386 --fuzz-target=i440fx-qtest-reboot-fuzz -runs=5000

This will run the i440fx fuzzer over 5000 inputs which should finish in
a second or so. I don't expect it to actually find any crashes in the
i440fx in such a short period, so, ideally, all errors would be
fuzzer-related.

Where can I get started with building out a CI job for this?

One aside: running this right now, QEMU exits and AddressSanitizer
complains about some leaks. There is a patch in Paolo's PR that should
fix this, but I was surprised that existing CI tests didn't catch it. Is
leak detection usually disabled in CI?

> For this patch here:
> Reviewed-by: Thomas Huth 

Thanks!
-Alex

Re: [PATCH-for-5.1 2/2] fuzz: add missing header for rcu_enable_atfork

[Thomas Huth]

On 09/07/2020 15.38, Alexander Bulekov wrote:
> On 200709 0718, Thomas Huth wrote:
>> On 08/07/2020 22.01, Alexander Bulekov wrote:
>>> In 45222b9a90, I fixed a broken check for rcu_enable_atfork introduced
>>> in d6919e4cb6. I added a call to rcu_enable_atfork after the
>>> call to qemu_init in fuzz.c, but forgot to include the corresponding
>>> header, breaking --enable-fuzzing --enable-werror builds.
>>>
>>> Fixes: 45222b9a90 ("fuzz: fix broken qtest check at rcu_disable_atfork")
>>> Signed-off-by: Alexander Bulekov 
>>> ---
>>>  tests/qtest/fuzz/fuzz.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
>>> index a36d9038e0..0b66e43409 100644
>>> --- a/tests/qtest/fuzz/fuzz.c
>>> +++ b/tests/qtest/fuzz/fuzz.c
>>> @@ -19,6 +19,7 @@
>>>  #include "sysemu/runstate.h"
>>>  #include "sysemu/sysemu.h"
>>>  #include "qemu/main-loop.h"
>>> +#include "qemu/rcu.h"
>>>  #include "tests/qtest/libqtest.h"
>>>  #include "tests/qtest/libqos/qgraph.h"
>>>  #include "fuzz.h"
>>
>> D'oh, mea culpa, I also apparently did not properly compile test that
>> patch :-( I think we need a CI job that at least compile tests the
>> fuzzing code - I can look into that once Alex Bennée's current testing
>> pull request has been merged.
> 
> My bad - I should have done a clean build with a version of clang
> that doesn't require me to -disable-werror
> 
>> Alexander, is there also a way to run a fuzzer just for some few
>> minutes? E.g. a fuzzing test that finishes quickly, or an option to
>> limit the time that a test is running? If so, we could also add that
>> quick test to the CI pipeline, to make sure that the fuzzer code does
>> not only compile, but is also able to run (at least a little bit).
> 
> Yes. I think the sequence could look something like:
> CC=clang CXX=clang++ ../configure --enable-fuzzing --enable-sanitizers \
>  --enable-werror
> make i386-softmmu/fuzz
> ./i386-softmmu/qemu-fuzz-i386 --fuzz-target=i440fx-qtest-reboot-fuzz 
> -runs=5000
> 
> This will run the i440fx fuzzer over 5000 inputs which should finish in
> a second or so. I don't expect it to actually find any crashes in the
> i440fx in such a short period, so, ideally, all errors would be
> fuzzer-related.
> 
> Where can I get started with building out a CI job for this?

I'd suggest to use gitlab, since we're currently focusing on that for
our CI. So get an account on gitlab, clone the qemu repository there
(https://gitlab.com/qemu-project/qemu) to your account, and then you
should almost be ready to go: Edit the .gitlab-ci.yml file in the
repository, and once you push your local branch to the gitlab server,
you should see the jobs running in the "CI / CD" section. (Not sure
anymore whether you have to enable the CI manually for your project,
though, but it should not be too hard to find that setting if that's the
case)

> One aside: running this right now, QEMU exits and AddressSanitizer
> complains about some leaks. There is a patch in Paolo's PR that should
> fix this, but I was surprised that existing CI tests didn't catch it. Is
> leak detection usually disabled in CI?

I'm not aware of any CI tests that is currently using leak detection ...
so it's certainly welcome if we get more test coverage here!

 Thomas

Re: [PATCH 2/2] hw/sd/sdcard: Do not allow invalid SD card sizes

[Philippe Mathieu-Daudé]

On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> Hi Philippe,
> 
> Just tried out your patch on latest master, and I noticed I couldn't
> apply it without getting this error:
> 
> $ git am ~/Downloads/patches/\[PATCH\ 2_2\]\ hw_sd_sdcard\:\ Do\ not\
> allow\ invalid\ SD\ card\ sizes\ -\ Philippe\ Mathieu-Daudé\
> \mailto:f4...@amsat.org>\>\ -\ 2020-07-07\ 1521.eml
> Applying: hw/sd/sdcard: Do not allow invalid SD card sizes
> error: patch failed: hw/sd/sd.c:2130
> error: hw/sd/sd.c: patch does not apply
> Patch failed at 0001 hw/sd/sdcard: Do not allow invalid SD card sizes
> Use 'git am --show-current-patch' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> 
> The first patch did go OK. Maybe this one just needs to be rebased, or I
> made a mistake.

Sorry it was not clear on the cover:

  Part 1 is already reviewed:
  https://www.mail-archive.com/qemu-devel@nongnu.org/msg718150.html
  Based-on: <20200630133912.9428-1-f4...@amsat.org>

This series is based on the "Part 1".

> So I manually copy & pasted the change into hw/sd/sd.c to test it.
> It looks like the check works, but my concern is that with this change,
> we will be getting this error on 'off-the-shelf' images as well.
> For example, the latest Raspbian image size also isn't a power of two:
> 
> $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> WARNING: Image format was not specified for
> '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> probing guessed raw.
>          Automatically detecting the format is dangerous for raw images,
> write operations on block 0 will be restricted.
>          Specify the 'raw' format explicitly to remove the restrictions.
> qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)
> 
> If we do decide that the change is needed, I would like to propose that
> we also give the user some instructions
> on how to fix it, maybe some 'dd' command?

On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
This is not in the default Darwin packages.
On Windows I have no clue.

> In my opinion that should
> also go in some of the documentation file(s),
> possibly also in the one for the OrangePi PC at
> docs/system/arm/orangepi.rst (I can also provide a patch for that if you
> wish).

Good idea, if you can send that patch that would a precious help,
and I'd include it with the other patches :)

Note that this was your orangepi-pc acceptance test that catched
this bug!
See https://travis-ci.org/github/philmd/qemu/jobs/705653532#L5672:

 CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), cr=50c5387d
 OF: fdt: Machine model: Xunlong Orange Pi PC
 Kernel command line: printk.time=0 console=ttyS0,115200
root=/dev/mmcblk0 rootwait rw panic=-1 noreboot
 sunxi-mmc 1c0f000.mmc: Linked as a consumer to regulator.2
 sunxi-mmc 1c0f000.mmc: Got CD GPIO
 sunxi-mmc 1c0f000.mmc: initialized, max. request size: 16384 KB
 mmc0: host does not support reading read-only switch, assuming write-enable
 mmc0: Problem switching card into high-speed mode!
 mmc0: new SD card at address 4567
 mmcblk0: mmc0:4567 QEMU! 60.0 MiB
 EXT4-fs (mmcblk0): mounting ext2 file system using the ext4 subsystem
 EXT4-fs (mmcblk0): mounted filesystem without journal. Opts: (null)
 VFS: Mounted root (ext2 filesystem) on device 179:0.
 EXT4-fs (mmcblk0): re-mounted. Opts: block_validity,barrier,user_xattr,acl
 Populating /dev using udev: udevd[204]: starting version 3.2.7
 udevadm settle failed
 done
 udevd[205]: worker [208]
/devices/platform/soc/1c0f000.mmc/mmc_host/mmc0/mmc0:4567/block/mmcblk0
is taking a long time
Runner error occurred: Timeout reached
Original status: ERROR

(I'll add that in the commit description too).

Thanks for your testing/review!

> Kind regards,
> 
> Niek
> 
> 
> On Tue, Jul 7, 2020 at 6:11 PM Philippe Mathieu-Daudé  > wrote:
> 
> On 7/7/20 6:06 PM, Peter Maydell wrote:
> > On Tue, 7 Jul 2020 at 17:04, Alistair Francis
> mailto:alistai...@gmail.com>> wrote:
> >>
> >> On Tue, Jul 7, 2020 at 6:22 AM Philippe Mathieu-Daudé
> mailto:f4...@amsat.org>> wrote:
> >>>
> >>> QEMU allows to create SD card with unrealistic sizes. This could
> work,
> >>> but some guests (at least Linux) consider sizes that are not a power
> >>> of 2 as a firmware bug and fix the card size to the next power of 2.
> >>>
> >>> Before CVE-2020-13253 fix, this would allow OOB read/write accesses
> >>> past the image size end.
> >>>
> >>> CVE-2020-13253 has been fixed as:
> >>>
> >>>     Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
> >>>     occurred and no data transfer is performed.
> >>>
> >>>     Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
> >>>  

[PATCH v1 02/13] docs/devel: add some notes on tcg-icount for developers

[Alex Bennée]

This attempts to bring together my understanding of the requirements
for icount behaviour into one reference document for our developer
notes.

Signed-off-by: Alex Bennée 
Reviewed-by: Richard Henderson 
Cc: Paolo Bonzini 
Cc: Pavel Dovgalyuk 
Cc: Peter Maydell 
Message-Id: <20200619135844.23307-1-alex.ben...@linaro.org>

---
v2
  - fix copyright date
  - it's -> its
  - drop mentioned of gen_io_end()
  - remove and correct original conjecture
v3
  - include link in index
v4
  - Grammar fixes from Peter
  - re-worded final section
---
 docs/devel/index.rst  |  1 +
 docs/devel/tcg-icount.rst | 97 +++
 2 files changed, 98 insertions(+)
 create mode 100644 docs/devel/tcg-icount.rst

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 4ecaea3643fb..ae6eac7c9c66 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -23,6 +23,7 @@ Contents:
decodetree
secure-coding-practices
tcg
+   tcg-icount
multi-thread-tcg
tcg-plugins
bitops
diff --git a/docs/devel/tcg-icount.rst b/docs/devel/tcg-icount.rst
new file mode 100644
index ..8d67b6c076a7
--- /dev/null
+++ b/docs/devel/tcg-icount.rst
@@ -0,0 +1,97 @@
+..
+   Copyright (c) 2020, Linaro Limited
+   Written by Alex Bennée
+
+
+
+TCG Instruction Counting
+
+
+TCG has long supported a feature known as icount which allows for
+instruction counting during execution. This should not be confused
+with cycle accurate emulation - QEMU does not attempt to emulate how
+long an instruction would take on real hardware. That is a job for
+other more detailed (and slower) tools that simulate the rest of a
+micro-architecture.
+
+This feature is only available for system emulation and is
+incompatible with multi-threaded TCG. It can be used to better align
+execution time with wall-clock time so a "slow" device doesn't run too
+fast on modern hardware. It can also provides for a degree of
+deterministic execution and is an essential part of the record/replay
+support in QEMU.
+
+Core Concepts
+=
+
+At its heart icount is simply a count of executed instructions which
+is stored in the TimersState of QEMU's timer sub-system. The number of
+executed instructions can then be used to calculate QEMU_CLOCK_VIRTUAL
+which represents the amount of elapsed time in the system since
+execution started. Depending on the icount mode this may either be a
+fixed number of ns per instruction or adjusted as execution continues
+to keep wall clock time and virtual time in sync.
+
+To be able to calculate the number of executed instructions the
+translator starts by allocating a budget of instructions to be
+executed. The budget of instructions is limited by how long it will be
+until the next timer will expire. We store this budget as part of a
+vCPU icount_decr field which shared with the machinery for handling
+cpu_exit(). The whole field is checked at the start of every
+translated block and will cause a return to the outer loop to deal
+with whatever caused the exit.
+
+In the case of icount, before the flag is checked we subtract the
+number of instructions the translation block would execute. If this
+would cause the instruction budget to go negative we exit the main
+loop and regenerate a new translation block with exactly the right
+number of instructions to take the budget to 0 meaning whatever timer
+was due to expire will expire exactly when we exit the main run loop.
+
+Dealing with MMIO
+-
+
+While we can adjust the instruction budget for known events like timer
+expiry we cannot do the same for MMIO. Every load/store we execute
+might potentially trigger an I/O event, at which point we will need an
+up to date and accurate reading of the icount number.
+
+To deal with this case, when an I/O access is made we:
+
+  - restore un-executed instructions to the icount budget
+  - re-compile a single [1]_ instruction block for the current PC
+  - exit the cpu loop and execute the re-compiled block
+
+The new block is created with the CF_LAST_IO compile flag which
+ensures the final instruction translation starts with a call to
+gen_io_start() so we don't enter a perpetual loop constantly
+recompiling a single instruction block. For translators using the
+common translator_loop this is done automatically.
+  
+.. [1] sometimes two instructions if dealing with delay slots  
+
+Other I/O operations
+
+
+MMIO isn't the only type of operation for which we might need a
+correct and accurate clock. IO port instructions and accesses to
+system registers are the common examples here. These instructions have
+to be handled by the individual translators which have the knowledge
+of which operations are I/O operations.
+
+When the translator is handling an instruction of this kind:
+
+* it must call gen_io_start() if icount is enabled, at some
+   point before the generation of the code which actually does
+   the 

[PATCH v1 00/13] misc rc0 fixes (docs, plugins, docker)

[Alex Bennée]

Hi,

These are some candidate patches for rc0 along with a few plugin
patches that haven't yet gotten review. The new functionality won't
get added to the PR but I'd like to get the cputlb fix in.

Alongside the plugin stuff there are some documentation updates which
are worth adding and some tweaks to the docker cache handling that
I only discovered after I sent the last PR.

Based on:

  Message-Id: <20200707070858.6622-1-alex.ben...@linaro.org>
  https://github.com/stsquad/qemu.git tags/pull-testing-and-misc-070720-1

The following need review:

 - configure: remove all dependencies on a (re)configure
 - tests/docker: fall back more gracefully when pull fails
 - tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi
 - target/sh4: revert to using cpu_lduw_code to decode gusa
 - plugins: expand the bb plugin to be thread safe and track per-cpu
 - cputlb: ensure we save the IOTLB data in case of reset

Alex Bennée (11):
  docs/devel: convert and update MTTCG design document
  docs/devel: add some notes on tcg-icount for developers
  cputlb: ensure we save the IOTLB data in case of reset
  hw/virtio/pci: include vdev name in registered PCI sections
  plugins: add API to return a name for a IO device
  plugins: new hwprofile plugin
  plugins: expand the bb plugin to be thread safe and track per-cpu
  target/sh4: revert to using cpu_lduw_code to decode gusa
  tests/plugins: add -Wno-unknown-warning-option to handle -Wpsabi
  tests/docker: fall back more gracefully when pull fails
  configure: remove all dependencies on a (re)configure

Jon Doron (1):
  docs: Add to gdbstub documentation the PhyMemMode

Max Filippov (1):
  tests/docker: update toolchain set in debian-xtensa-cross

 docs/devel/index.rst  |   2 +
 ...ti-thread-tcg.txt => multi-thread-tcg.rst} |  52 +--
 docs/devel/tcg-icount.rst |  97 ++
 docs/system/gdb.rst   |  20 ++
 configure |  15 +-
 include/hw/core/cpu.h |   4 +
 include/qemu/qemu-plugin.h|   6 +
 include/qemu/typedefs.h   |   1 +
 accel/tcg/cputlb.c|  57 +++-
 hw/virtio/virtio-pci.c|  22 +-
 plugins/api.c |  20 ++
 target/sh4/translate.c|   8 +-
 tests/plugin/bb.c |  96 +-
 tests/plugin/hwprofile.c  | 305 ++
 tests/docker/docker.py|  11 +-
 .../dockerfiles/debian-xtensa-cross.docker|   6 +-
 tests/plugin/Makefile |   3 +-
 tests/tcg/Makefile.target |  12 +-
 18 files changed, 673 insertions(+), 64 deletions(-)
 rename docs/devel/{multi-thread-tcg.txt => multi-thread-tcg.rst} (90%)
 create mode 100644 docs/devel/tcg-icount.rst
 create mode 100644 tests/plugin/hwprofile.c

-- 
2.20.1

[PATCH v1 01/13] docs/devel: convert and update MTTCG design document

[Alex Bennée]

Do a light conversion to .rst and clean-up some of the language at the
start now MTTCG has been merged for a while.

Signed-off-by: Alex Bennée 
Reviewed-by: Richard Henderson 
---
 docs/devel/index.rst  |  1 +
 ...ti-thread-tcg.txt => multi-thread-tcg.rst} | 52 ---
 2 files changed, 34 insertions(+), 19 deletions(-)
 rename docs/devel/{multi-thread-tcg.txt => multi-thread-tcg.rst} (90%)

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index bb8238c5d6de..4ecaea3643fb 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -23,6 +23,7 @@ Contents:
decodetree
secure-coding-practices
tcg
+   multi-thread-tcg
tcg-plugins
bitops
reset
diff --git a/docs/devel/multi-thread-tcg.txt b/docs/devel/multi-thread-tcg.rst
similarity index 90%
rename from docs/devel/multi-thread-tcg.txt
rename to docs/devel/multi-thread-tcg.rst
index 3c85ac0eab9b..42158b77c707 100644
--- a/docs/devel/multi-thread-tcg.txt
+++ b/docs/devel/multi-thread-tcg.rst
@@ -1,15 +1,17 @@
-Copyright (c) 2015-2016 Linaro Ltd.
+..
+  Copyright (c) 2015-2020 Linaro Ltd.
 
-This work is licensed under the terms of the GNU GPL, version 2 or
-later. See the COPYING file in the top-level directory.
+  This work is licensed under the terms of the GNU GPL, version 2 or
+  later. See the COPYING file in the top-level directory.
 
 Introduction
 
 
-This document outlines the design for multi-threaded TCG system-mode
-emulation. The current user-mode emulation mirrors the thread
-structure of the translated executable. Some of the work will be
-applicable to both system and linux-user emulation.
+This document outlines the design for multi-threaded TCG (a.k.a MTTCG)
+system-mode emulation. user-mode emulation has always mirrored the
+thread structure of the translated executable although some of the
+changes done for MTTCG system emulation have improved the stability of
+linux-user emulation.
 
 The original system-mode TCG implementation was single threaded and
 dealt with multiple CPUs with simple round-robin scheduling. This
@@ -21,9 +23,18 @@ vCPU Scheduling
 ===
 
 We introduce a new running mode where each vCPU will run on its own
-user-space thread. This will be enabled by default for all FE/BE
-combinations that have had the required work done to support this
-safely.
+user-space thread. This is enabled by default for all FE/BE
+combinations where the host memory model is able to accommodate the
+guest (TCG_GUEST_DEFAULT_MO & ~TCG_TARGET_DEFAULT_MO is zero) and the
+guest has had the required work done to support this safely
+(TARGET_SUPPORTS_MTTCG).
+
+System emulation will fall back to the original round robin approach
+if:
+
+* forced by --accel tcg,thread=single
+* enabling --icount mode
+* 64 bit guests on 32 bit hosts (TCG_OVERSIZED_GUEST)
 
 In the general case of running translated code there should be no
 inter-vCPU dependencies and all vCPUs should be able to run at full
@@ -61,7 +72,9 @@ have their block-to-block jumps patched.
 Global TCG State
 
 
-### User-mode emulation
+User-mode emulation
+~~~
+
 We need to protect the entire code generation cycle including any post
 generation patching of the translated code. This also implies a shared
 translation buffer which contains code running on all cores. Any
@@ -78,9 +91,11 @@ patching.
 
 Code generation is serialised with mmap_lock().
 
-### !User-mode emulation
+!User-mode emulation
+
+
 Each vCPU has its own TCG context and associated TCG region, thereby
-requiring no locking.
+requiring no locking during translation.
 
 Translation Blocks
 --
@@ -92,6 +107,7 @@ including:
 
   - debugging operations (breakpoint insertion/removal)
   - some CPU helper functions
+  - linux-user spawning it's first thread
 
 This is done with the async_safe_run_on_cpu() mechanism to ensure all
 vCPUs are quiescent when changes are being made to shared global
@@ -250,8 +266,10 @@ to enforce a particular ordering of memory operations from 
the point
 of view of external observers (e.g. another processor core). They can
 apply to any memory operations as well as just loads or stores.
 
-The Linux kernel has an excellent write-up on the various forms of
-memory barrier and the guarantees they can provide [1].
+The Linux kernel has an excellent `write-up
+`
+on the various forms of memory barrier and the guarantees they can
+provide.
 
 Barriers are often wrapped around synchronisation primitives to
 provide explicit memory ordering semantics. However they can be used
@@ -352,7 +370,3 @@ an exclusive lock which ensures all emulation is serialised.
 While the atomic helpers look good enough for now there may be a need
 to look at solutions that can more closely model the guest
 architectures semantics.
-
-==
-
-[1] 

[PATCH v1 03/13] docs: Add to gdbstub documentation the PhyMemMode

[Alex Bennée]

From: Jon Doron 

The PhyMemMode gdb extension command was missing from the gdb.rst
document.

Signed-off-by: Jon Doron 
Signed-off-by: Alex Bennée 
Message-Id: <20200601171609.1665397-1-ari...@gmail.com>
---
 docs/system/gdb.rst | 20 
 1 file changed, 20 insertions(+)

diff --git a/docs/system/gdb.rst b/docs/system/gdb.rst
index a40145fcf849..abda961e2b49 100644
--- a/docs/system/gdb.rst
+++ b/docs/system/gdb.rst
@@ -87,3 +87,23 @@ three commands you can query and set the single step 
behavior:
   (gdb) maintenance packet Qqemu.sstep=0x5
   sending: "qemu.sstep=0x5"
   received: "OK"
+
+
+Another feature that QEMU gdbstub provides is to toggle the memory GDB
+works with, by default GDB will show the current process memory respecting
+the virtual address translation.
+
+If you want to examine/change the physical memory you can set the gdbstub
+to work with the physical memory rather with the virtual one.
+
+The memory mode can be checked by sending the following command:
+
+``maintenance packet qqemu.PhyMemMode``
+This will return either 0 or 1, 1 indicates you are currently in the
+physical memory mode.
+
+``maintenance packet Qqemu.PhyMemMode:1``
+This will change the memory mode to physical memory.
+
+``maintenance packet Qqemu.PhyMemMode:0``
+This will change it back to normal memory mode.
-- 
2.20.1

Re: [qemu-web PATCH] new page: conservancy.md

[Thomas Huth]

On 07/07/2020 17.19, Paolo Bonzini wrote:
> On 07/07/20 16:51, Alex Bennée wrote:
>> +QEMU interacts with Conservancy through a Leadership Committee,
>> +currently composed of the following members:
>> +
>> +* Alex Bennée
>> +* Paolo Bonzini
>> +* Andreas Färber
>> +* Alexander Graf
>> +* Stefan Hajnoczi
>> +* Peter Maydell
>> +
>> +The committee votes via simple majority. There cannot be more than two
>> +members employed by the same company at once.
> 
> s/two members/one third of the members (currently two)/
> 
> s/company/entity/ (because Stefan and I are still employed by Red Hat,
> not IBM).
> 
>> +
>> +If you would like to raise an issue to the Leadership Committee,
>> +please email [qemu-devel@nongnu.org](mailto:qemu-devel@nongnu.org) and
>> +CC at least one of the members so they can bring the issue forward.
>> +For private emails you can send an email
> 
> Missing "to" here.
> 
>> +[q...@sfconservancy.org](mailto:q...@sfconservancy.org) which is a
>> +private list that all the leadership committee are subscribed to.
> 
> Missing comma before "which", or remove "which is" altogether after
> adding the comma.
> 
> I would also add a link to the first column of _includes/footer.html.

I've done the suggested modifications and pushed the page now. Please
double-check whether it looks as expected!

Paolo, could you please update the link on
https://wiki.qemu.org/Main_Page ? I do not have the access rights to
edit that page. Once that is done, I think we can delete the old page
https://wiki.qemu.org/Conservancy in the wiki (or turn it into a
redirect page instead?).

 Thanks,
  Thomas

Re: [PULL 00/41] testing updates (vm, gitlab, misc build fixes)

[Peter Maydell]

On Tue, 7 Jul 2020 at 08:09, Alex Bennée  wrote:
>
> There will be some docker failures until the official repository has
> seeded but local builds should continue to work.
>
> 
>
> The following changes since commit eb6490f544388dd24c0d054a96dd304bc7284450:
>
>   Merge remote-tracking branch 
> 'remotes/pmaydell/tags/pull-target-arm-20200703' into staging (2020-07-04 
> 16:08:41 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/stsquad/qemu.git tags/pull-testing-and-misc-070720-1
>
> for you to fetch changes up to 6a726e8ca0286e3ed69945abd447099f6f6a903c:
>
>   tests/qht-bench: Adjust threshold computation (2020-07-07 07:57:41 +0100)
>
> 
> Testing and build updates:
>
>   - tests/vm support for aarch64 VMs
>   - tests/tcg better cross-compiler detection
>   - update docker tooling to support registries
>   - gitlab build docker images and store in registry
>   - gitlab use docker images for builds
>   - a number of skipIf updates to support move
>   - linux-user MAP_FIXED_NOREPLACE fix
>   - qht-bench compiler tweaks
>   - configure fix for secret keyring
>   - tsan fiber annotation clean-up

freebsd failed:

perl: warning: Please check that your locale settings:
perl: warning: Falling back to the standard locale ("C").
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
con recv: Loading /boot/defaults/loader.conf
con recv: Loading /boot/device.hints
con recv: Loading /boot/loader.conf
con recv: Loading /boot/loader.conf.local
con recv: \/  ```` s` `.---...--.```
-/ +o   .--` /y:`  +.  yo`:.:o  `+-   y/
-/`   -o/  .-  ::/sy+:.  /
`--  / `:  :` `:
:`  /  /  .--.   --
  -.`:`  `:`  .--
`--. .---..  __      _ _
|  | |  _ \ / |  __ \  | |___ _ __ ___  ___ | |_)
| (___ | |  | | |  ___| '__/ _ \/ _ \|  _ < \___ \| |  | | | |   | | |
 __/  __/| |_) |) | |__| | | |   | | |||| |  |
 | |_|   |_|  \___|\___||/|_/|_/
..Welcome
to FreeBSD1. Boot Multi user [Enter]2. Boot Single user3. Escape to
loader prompt4. RebootOptions:/\/\5. Kernel: default/kernel (1 of 1)6.
Boot OptionsAutoboot
con send: 3
con recv:  in 10 seconds, hit [Enter] to boot or any other key to stop
con recv:
con recv: Exiting menu!
con recv: Type '?' for a list of commands, 'help' for more detailed help.
con recv: OK
con send: set console=comconsole
console: *** read timeout ***
console: waiting for: 'OK'
console: line buffer:

con recv:  set console=comconso

Failed to prepare guest environment
Traceback (most recent call last):
  File "/home/peter.maydell/qemu-freebsd/tests/vm/basevm.py", line 628, in main
return vm.build_image(args.image)
  File "/home/peter.maydell/qemu-freebsd/tests/vm/freebsd", line 163,
in build_image
self.console_boot_serial()
  File "/home/peter.maydell/qemu-freebsd/tests/vm/freebsd", line 76,
in console_boot_serial
self.console_wait_send("OK", "boot\n")
  File "/home/peter.maydell/qemu-freebsd/tests/vm/basevm.py", line
400, in console_wait_send
self.console_wait(wait)
  File "/home/peter.maydell/qemu-freebsd/tests/vm/basevm.py", line
340, in console_wait
chars = vm.console_socket.recv(1)
  File 
"/home/peter.maydell/qemu-freebsd/tests/vm/../../python/qemu/console_socket.py",
line 96, in recv
raise socket.timeout
socket.timeout


NetBSD failed:
con recv: postfix: rebuilding /etc/mail/aliases (missing /etc/mail/aliases.db)
con recv: Starting inetd.
con recv: Starting cron.
con recv: Thu Jul  9 10:40:07 UTC 2020
con recv: NetBSD/amd64 (localhost) (constty)
con recv: login:
con send: qemu
con recv:  Jul  9 10:40:09 localhost getty[756]: /dev/ttyE2: Device
not configured
con recv: Jul  9 10:40:09 localhost getty[703]: /dev/ttyE3: Device not
configured
con recv: Jul  9 10:40:09 localhost getty[753]: /dev/ttyE1: Device not
configured
con recv: qemu
con recv: Password:
con send: qemupass
con recv: Login incorrect or refused on this terminal.
console: *** read timeout ***
console: waiting for: 'localhost$'
console: line buffer:

con recv: login:

Failed to prepare guest environment
Traceback (most recent call last):
  File "/home/peter.maydell/qemu-netbsd/tests/vm/basevm.py", line 628, in main
return vm.build_image(args.image)
  File "/home/peter.maydell/qemu-netbsd/tests/vm/netbsd", line 174, in
build_image
self._config["guest_pass"])
  File 

Re: [PATCH 2/2] util/qemu-sockets: make keep-alive enabled by default

[Daniel P . Berrangé]

On Thu, Jul 09, 2020 at 11:49:17AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 09.07.2020 11:29, Daniel P. Berrangé wrote:
> > On Wed, Jul 08, 2020 at 10:15:39PM +0300, Vladimir Sementsov-Ogievskiy 
> > wrote:
> > > Keep-alive won't hurt, let's try to enable it even if not requested by
> > > user.
> > 
> > Keep-alive intentionally breaks TCP connections earlier than normal
> > in face of transient networking problems.
> > 
> > The question is more about which type of pain is more desirable. A
> > stall in the network connection (for a potentially very long time),
> > or an intentionally broken socket.
> > 
> > I'm not at all convinced it is a good idea to intentionally break
> > /all/ QEMU sockets in the face of transient problems, even if the
> > problems last for 2 hours or more.
> > 
> > I could see keep-alives being ok on some QEMU socket. For example
> > VNC/SPICE clients, as there is no downside to proactively culling
> > them as they can trivially reconnect. Migration too is quite
> > reasonable to use keep alives, as you generally want migration to
> > run to completion in a short amount of time, and aborting migration
> > needs to be safe no matter what.
> > 
> > Breaking chardevs or block devices or network devices that use
> > QEMU sockets though will be disruptive. The only solution once
> > those backends have a dead socket is going to be to kill QEMU
> > and cold-boot the VM again.
> > 
> 
> Reasonable, thanks for explanation.
> 
> We are mostly interested in keep-alive for migration and NBD connections.
> (NBD driver has ability to reconnect). What do you think about setting
> keep-alive (with some KEEPIDLE smaller than 2 hours) by default for
> migration and NBD (at least when NBD reconnect is enabled), would it be
> valid?

I think it should be reasonable to set by default for those particular
scenarios, as both are expecting failures and ready to take action when
they occur.

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: Questions about online resizing a lun passthrough disk with virtio-scsi

[Lin Ma]

On 2020-07-08 15:11, Paolo Bonzini wrote:

On 08/07/20 16:44, lma wrote:


Is the 'block_resize' mandatory to notify guest os after online 
resizing
a lun passed through disk? I'm curious it because I found there're 
couple

of ways can make guest os realize the disk capacity change.
e.g:
* run 'block_resize' via qmp to let virtio-scsi notify the frontend 
about

  capacity change.
* run 'rescan-scsi-bus.sh -s' inside guest.
* run 'sg_readcap --16 /dev/sda' inside guest.

I knew that the purpose of 'block_resize' is not only to notify guest 
os,
but also to update some internal structure's member, say 
bs->total_sectors.
What if I forgot to run 'block_resize', but run 'rescan-scsi-bus.sh 
-s'

in guest?


Request start and length are checked even for passthrough disks (see
scsi_disk_dma_command in hw/scsi/scsi-disk.c, called by
scsi_block_dma_command), but the maximum LBA is snooped from READ
CAPACITY commands (see scsi_read_complete in hw/scsi/scsi-generic.c).
So as long as rescan-scsi-bus.sh results in a READ CAPACITY command, it
should work.


Yeah, the rescan-scsi-bus.sh does result in a READ CAPACITY command.


It's not recommended however, because block_resize will report the
change to the guest directly with a CAPACITY HAS CHANGED unit attention
condition.


Got it, The 'block_resize' is the recommended or necessary step, Even 
for

passthrough disk online resizing.

Thanks for your information,
Lin

Re: [PATCH] ossaudio: fix out of bounds write

[Gerd Hoffmann]

> > diff --git a/audio/ossaudio.c b/audio/ossaudio.c
> > index f88d076ec2..a7dcaa31ad 100644
> > --- a/audio/ossaudio.c
> > +++ b/audio/ossaudio.c
> > @@ -691,6 +691,7 @@ static size_t oss_read(HWVoiceIn *hw, void *buf, size_t 
> > len)
> > len, dst);
> >  break;
> >  }
> > +break;
> >  }
> >  
> >  pos += nread;
> 
> ... now pos += -1, then the size returned misses the last byte.

No, it doesn't.  break leaves the while loop, not the if condition.
>From patch context it isn't obvious though, you need to look at the
source code ...

Patch queued.

thanks,
  Gerd

Re: Delete some Wiki pages (was: Migrating custom qemu.org infrastructure to GitLab)

[Paolo Bonzini]

On 09/07/20 15:10, Thomas Huth wrote:
> - https://wiki.qemu.org/Features/40p :
> - https://wiki.qemu.org/Features/PRePCleanup :
> - https://wiki.qemu.org/Features/BeBox
> - https://wiki.qemu.org/Features/DriveRefactoring
> - https://wiki.qemu.org/Features/LegacyRemoval
> - https://wiki.qemu.org/Features/Machines/Edison
> - https://wiki.qemu.org/Features/Tegra2

Can you make them either "Completed feature pages" or "Obsolete feature
pages"?  We can certainly drop the obsolete features if we migrate to
the GitLab wiki.

> - https://wiki.qemu.org/Features/Documentation/interop
>   that has been superseded by:
>   https://www.qemu.org/docs/master/interop/index.html
> 
> - https://wiki.qemu.org/Features/Documentation/specs
>   that has been superseded by:
>   https://www.qemu.org/docs/master/specs/index.html

Need to check if there's anything else left to do for those two manuals.
 Probably the rST conversion of QMP documentation.  I would leave them
around, they are linked from the main Features/Documentation page.

> - https://wiki.qemu.org/Features/LibvirtWiresharkDissector
>   seems to be a libvirt proposal - IMHO should not be in the QEMU wiki

Probably an internship idea.

> - https://wiki.qemu.org/Features/Version3.0
>   Old suggestions for QEMU version 3.0 ... we're close to 5.1 already

This is more like ideas for deprecation.

> - https://wiki.qemu.org/KeySigningParty2013
>   https://wiki.qemu.org/KeySigningParty2014
>   https://wiki.qemu.org/KeySigningParty2015
>   Only some few old information here, useless nowadays?
> 
> - https://wiki.qemu.org/Features/network_reentrant
>   Old ideas from 2013 ... I think vhost-net superseded this?

It's in [[Category:Obsolete feature pages]] for a reason. :)

> - https://wiki.qemu.org/Planning/Relicensing
>   I think this has been completed. The page looks very outdated now.

No, it's not completed and it's probably never going to be.

> - https://wiki.qemu.org/SecurityProcess
>   Should be replaced with a redirect

Done.

Paolo

[PATCH] softmmu/vl: Include "qemu/rcu.h" for rcu_disable_atfork()

[Philippe Mathieu-Daudé]

In commit 73c6e4013b we let vl.c use rcu_disable_atfork()
which is declared in "qemu/rcu.h", but forgot to include
this header. Fortunately has never been a problem since
vl.c includes "exec/memory.h" which includes "qemu/rcu.h".

Include the missing header now in case we split vl.c later.

Fixes: 73c6e4013b ("rcu: disable pthread_atfork callbacks ASAP")
Signed-off-by: Philippe Mathieu-Daudé 
---
 softmmu/vl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index ecbc18ba75..f243745c51 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -83,6 +83,7 @@
 #include "qemu/config-file.h"
 #include "qemu-options.h"
 #include "qemu/main-loop.h"
+#include "qemu/rcu.h"
 #ifdef CONFIG_VIRTFS
 #include "fsdev/qemu-fsdev.h"
 #endif
-- 
2.21.3

[PATCH v1 05/13] hw/virtio/pci: include vdev name in registered PCI sections

[Alex Bennée]

When viewing/debugging memory regions it is sometimes hard to figure
out which PCI device something belongs to. Make the names unique by
including the vdev name in the name string.

Signed-off-by: Alex Bennée 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Michael S. Tsirkin 

---
v2
  - swap ()'s for an extra -
---
 hw/virtio/virtio-pci.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 8554cf2a038e..215e680c71f4 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1406,7 +1406,8 @@ static void virtio_pci_device_write(void *opaque, hwaddr 
addr,
 }
 }
 
-static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
+static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy,
+   const char *vdev_name)
 {
 static const MemoryRegionOps common_ops = {
 .read = virtio_pci_common_read,
@@ -1453,36 +1454,41 @@ static void 
virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
 },
 .endianness = DEVICE_LITTLE_ENDIAN,
 };
+g_autoptr(GString) name = g_string_new(NULL);
 
-
+g_string_printf(name, "virtio-pci-common-%s", vdev_name);
 memory_region_init_io(>common.mr, OBJECT(proxy),
   _ops,
   proxy,
-  "virtio-pci-common",
+  name->str,
   proxy->common.size);
 
+g_string_printf(name, "virtio-pci-isr-%s", vdev_name);
 memory_region_init_io(>isr.mr, OBJECT(proxy),
   _ops,
   proxy,
-  "virtio-pci-isr",
+  name->str,
   proxy->isr.size);
 
+g_string_printf(name, "virtio-pci-device-%s", vdev_name);
 memory_region_init_io(>device.mr, OBJECT(proxy),
   _ops,
   virtio_bus_get_device(>bus),
-  "virtio-pci-device",
+  name->str,
   proxy->device.size);
 
+g_string_printf(name, "virtio-pci-notify-%s", vdev_name);
 memory_region_init_io(>notify.mr, OBJECT(proxy),
   _ops,
   virtio_bus_get_device(>bus),
-  "virtio-pci-notify",
+  name->str,
   proxy->notify.size);
 
+g_string_printf(name, "virtio-pci-notify-pio-%s", vdev_name);
 memory_region_init_io(>notify_pio.mr, OBJECT(proxy),
   _pio_ops,
   virtio_bus_get_device(>bus),
-  "virtio-pci-notify-pio",
+  name->str,
   proxy->notify_pio.size);
 }
 
@@ -1623,7 +1629,7 @@ static void virtio_pci_device_plugged(DeviceState *d, 
Error **errp)
 
 struct virtio_pci_cfg_cap *cfg_mask;
 
-virtio_pci_modern_regions_init(proxy);
+virtio_pci_modern_regions_init(proxy, vdev->name);
 
 virtio_pci_modern_mem_region_map(proxy, >common, );
 virtio_pci_modern_mem_region_map(proxy, >isr, );
-- 
2.20.1

QEMU | Pipeline #164743957 has failed for master | 48f22ad0

[GitLab via]

Your pipeline has failed.

Project: QEMU ( https://gitlab.com/qemu-project/qemu )
Branch: master ( https://gitlab.com/qemu-project/qemu/-/commits/master )

Commit: 48f22ad0 ( 
https://gitlab.com/qemu-project/qemu/-/commit/48f22ad04ead83e61b4b35871ec6f6109779b791
 )
Commit Message: Merge remote-tracking branch 'remotes/vivier/ta...
Commit Author: Peter Maydell ( https://gitlab.com/pm215 )

Pipeline #164743957 ( 
https://gitlab.com/qemu-project/qemu/-/pipelines/164743957 ) triggered by Alex 
Bennée ( https://gitlab.com/stsquad )
had 1 failed build.

Job #630887368 ( https://gitlab.com/qemu-project/qemu/-/jobs/630887368/raw )

Stage: test
Name: build-disabled
Trace: qemu-system-i386: falling back to tcg
Could not access KVM kernel module: No such file or directory
qemu-system-i386: -accel kvm: failed to initialize kvm: No such file or 
directory
qemu-system-i386: falling back to tcg
Could not access KVM kernel module: No such file or directory
qemu-system-i386: -accel kvm: failed to initialize kvm: No such file or 
directory
qemu-system-i386: falling back to tcg
  TESTcheck-qtest-i386: tests/qtest/device-introspect-test
  TESTcheck-qtest-i386: tests/qtest/machine-none-test
  TESTcheck-qtest-i386: tests/qtest/qmp-test
  TESTcheck-qtest-i386: tests/qtest/qmp-cmd-test
  TESTcheck-qtest-i386: tests/qtest/qom-test
  TESTcheck-qtest-i386: tests/qtest/test-hmp
  TESTcheck-qtest-i386: tests/qtest/qos-test
  TESTcheck-qtest-mips64: tests/qtest/endianness-test
  TESTcheck-qtest-mips64: tests/qtest/display-vga-test
  TESTcheck-qtest-mips64: tests/qtest/cdrom-test
  TESTcheck-qtest-mips64: tests/qtest/device-introspect-test
  TESTcheck-qtest-mips64: tests/qtest/machine-none-test
  TESTcheck-qtest-mips64: tests/qtest/qmp-test
  TESTcheck-qtest-mips64: tests/qtest/qmp-cmd-test
  TESTcheck-qtest-mips64: tests/qtest/qom-test
  TESTcheck-qtest-mips64: tests/qtest/test-hmp
  TESTcheck-qtest-mips64: tests/qtest/qos-test
  TESTcheck-qtest-ppc64: tests/qtest/machine-none-test
  TESTcheck-qtest-ppc64: tests/qtest/qmp-test
  TESTcheck-qtest-ppc64: tests/qtest/qmp-cmd-test
  TESTcheck-qtest-ppc64: tests/qtest/qom-test
section_end:1594294436:step_script
ERROR: Job failed: execution took longer than 1h0m0s seconds



-- 
You're receiving this email because of your account on gitlab.com.

Re: Questions about online resizing a lun passthrough disk with virtio-scsi

[Lin Ma]

On 2020-07-09 12:00, Paolo Bonzini wrote:

On 09/07/20 13:52, Lin Ma wrote:

It's not recommended however, because block_resize will report the
change to the guest directly with a CAPACITY HAS CHANGED unit 
attention

condition.


Got it, The 'block_resize' is the recommended or necessary step, Even 
for

passthrough disk online resizing.


If your target is able to report the unit attention itself, it is okay
to skip it.  AFAIK drivers/target/ doesn't, though.


Got you.
I happen to use the drivers/target/ :-)

Thank you very much,
Lin

Re: [PATCH v10 21/34] qcow2: Add subcluster support to qcow2_get_host_offset()

[Max Reitz]

On 03.07.20 17:58, Alberto Garcia wrote:
> The logic of this function remains pretty much the same, except that
> it uses count_contiguous_subclusters(), which combines the logic of
> count_contiguous_clusters() / count_contiguous_clusters_unallocated()
> and checks individual subclusters.
> 
> qcow2_cluster_to_subcluster_type() is not necessary as a separate
> function anymore so it's inlined into its caller.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.h |  38 ---
>  block/qcow2-cluster.c | 150 ++
>  2 files changed, 92 insertions(+), 96 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PULL 00/41] testing updates (vm, gitlab, misc build fixes)

[Philippe Mathieu-Daudé]

On 7/9/20 1:31 PM, Peter Maydell wrote:
> On Tue, 7 Jul 2020 at 08:09, Alex Bennée  wrote:
>>
>> There will be some docker failures until the official repository has
>> seeded but local builds should continue to work.
>>
>> 
>>
>> The following changes since commit eb6490f544388dd24c0d054a96dd304bc7284450:
>>
>>   Merge remote-tracking branch 
>> 'remotes/pmaydell/tags/pull-target-arm-20200703' into staging (2020-07-04 
>> 16:08:41 +0100)
>>
>> are available in the Git repository at:
>>
>>   https://github.com/stsquad/qemu.git tags/pull-testing-and-misc-070720-1
>>
>> for you to fetch changes up to 6a726e8ca0286e3ed69945abd447099f6f6a903c:
>>
>>   tests/qht-bench: Adjust threshold computation (2020-07-07 07:57:41 +0100)
>>
>> 
>> Testing and build updates:
>>
>>   - tests/vm support for aarch64 VMs
>>   - tests/tcg better cross-compiler detection
>>   - update docker tooling to support registries
>>   - gitlab build docker images and store in registry
>>   - gitlab use docker images for builds
>>   - a number of skipIf updates to support move
>>   - linux-user MAP_FIXED_NOREPLACE fix
>>   - qht-bench compiler tweaks
>>   - configure fix for secret keyring
>>   - tsan fiber annotation clean-up
[...]
> Also a compile failure on s390x, but since this isn't related
> to changes you made afaict I wonder if it's the result of
> a change in the build environment:
> /home/ubuntu/qemu/block/ssh.c: In function ‘check_host_key_knownhosts’:
> /home/ubuntu/qemu/block/ssh.c:281:28: error: storage size of ‘state’ isn’t 
> known
>  enum ssh_known_hosts_e state;
> ^
> /home/ubuntu/qemu/block/ssh.c:289:13: error: implicit declaration of
> function ‘ssh_session_is_known_server’ [-Werror=implicit-funct
> ion-declaration]
>  state = ssh_session_is_known_server(s->session);
>  ^~~
> [and other errors]

libssh is bugged on Ubuntu 18.04.
https://bugs.launchpad.net/qemu/+bug/1838763

We need to use 'configure --disable-libssh' there.

Re: [PATCH v5 08/20] microvm/acpi: add minimal acpi support

[Gerd Hoffmann]

  Hi,

> > +scope = aml_scope("\\");
> > +pkg = aml_package(4);
> > +aml_append(pkg, aml_int(5)); /* SLEEP_CONTROL_REG.SLP_TYP */
> 
> I'm not sure what does the comment refer to here.

It's the register field the value gets written to.
With full acpi this is PM1a_CNT.SLP_TYP, hw-reduced uses
SLEEP_CONTROL_REG.SLP_TYP instead.  This is cut from pc/q35
version + adapted for hw-reduced.

> Does this 5 match
> the value IO handler tests against?

Yes.  "5" for S5 state (aka poweroff).  Can add a #define.

> Below is from "7.3.4 System \_Sx states" right?

"7.4.2 \_Sx (System States)" here (ACPI 6.3), guess that is the same.

> > +AcpiFadtData pmfadt = {
> > +/*
> > + * minimum version for ACPI_FADT_F_HW_REDUCED_ACPI,
> > + * see acpi spec "4.1 Hardware-Reduced ACPI"
> 
> Spec version - I'm guessing ACPI spec 5.0.

6.3

> And I think here is where you refer to
>   Table 5-34 Fixed ACPI Description Table (FADT) Format

Table 5-33 FADT Format

> > + */
> > +.rev = 5,
> > +.minor_ver = 1,
> 
> So 5.1 I am guessing just copied from virt/arm?

Yes.

> > +.flags = ((1 << ACPI_FADT_F_HW_REDUCED_ACPI) |
> > +  (1 << ACPI_FADT_F_RESET_REG_SUP)),
> > +
> > +/* Table 5-33 FADT Format -- SLEEP_CONTROL_REG */
> 
> You need to use the earliest spec version that includes
> a specific feature - and document which one it is.

Phew.  Isn't it easier to just use table and field name then, so it is
easy to find in whatever version of the spec you have at hand?  Also how
can I figure the earliest spec version easily?

Sometimes the 6.3 spec documents which table version added specific
fields, sometimes not ...

Is the table version synced with the acpi spec version?  Does DSDT v2
mean the DSDT format was updated for ACPI 2.0 and hasn't changed since?

> But the main poit is AcpiFadtData actually has nothing to do with
> FADT format. It's an abstracted API 

The FADT is generated from AcpiFadtData.  There is a 1:1 relationship
between most AcpiFadtData fields and FADT table entries.  This isn't
what I would call "has nothing to do with" ...

> > +xsdt = tables_blob->len;
> > +build_xsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> > +
> > +/* RSDP is in FSEG memory, so allocate it separately */
> > +{
> > +AcpiRsdpData rsdp_data = {
> > +/* Table 5-27 RSDP Structure */
> 
> RSDP is since ACPI 2.0, table number there is different.

References to ACPI 2.0 are almost useless.  ACPI 5.0 is the oldest
version uefi.org offers for download.

Guess that underlines the point I made above that referencing specific
versions of the spec doesn't work very well ...

take care,
  Gerd

Re: [PATCH] cpu: Add starts_halted() method

[Greg Kurz]

On Thu, 9 Jul 2020 14:21:04 +0200
Philippe Mathieu-Daudé  wrote:

> On 7/9/20 12:55 PM, Greg Kurz wrote:
> > On Thu, 9 Jul 2020 12:18:06 +0200
> > Philippe Mathieu-Daudé  wrote:
> >

[...]

> >>>
> >>> FYI, PAPR requires all vCPUs to be "stopped" by default. It is up to the
> >>> guest to start them explicitly through an RTAS call. The hypervisor is
> >>> only responsible to start a single vCPU (see spapr_cpu_set_entry_state()
> >>> called from spapr_machine_reset()) to be able to boot the guest.
> >>>
> >>> So I'm not sure to see how that would depend on the accelerator...
> >>
> >> $ qemu-system-ppc64 -M pseries-5.0,accel=tcg -d in_asm
> >> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> >> cap-cfpc=workaround
> >> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> >> cap-sbbc=workaround
> >> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> >> cap-ibs=workaround
> >> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> >> cap-ccf-assist=on
> >> 
> >> IN:
> >> 0x0100:  48003f00  b0x4000
> >>
> >> 
> >> IN:
> >> 0x4000:  7c7f1b78  mr   r31, r3
> >> 0x4004:  7d6000a6  mfmsrr11
> >> 0x4008:  3980a000  li   r12, 0xa000
> >> 0x400c:  798c83c6  sldi r12, r12, 0x30
> >> 0x4010:  7d6b6378  or   r11, r11, r12
> >> 0x4014:  7d600164  mtmsrd   r11
> >> ...
> >>
> >> The vCPU doesn't seem stopped to me...
> >>
> >> Am I missing something?
> >>
> > 
> > Yeah this is the boot vCPU which is required to be started
> > by the platform as explained above, but if you had more
> > vCPUs the other ones would be stopped until the guest OS
> > asks us to start them.
> 
> Ah OK, so we are good :)
> 
> The machine simply has to set the 'start-powered-off' flag on
> all vCPUS except the 1st one.
> 

We only want the first vCPU to start when the platform is
fully configured, so I'd rather put 'start-powered-off' on
every body and explicitly power on the first one during
machine reset as we do now.