Re: [PATCH] char: don't fail when client is not connected

[Marc-André Lureau]

Hi

On Tue, Feb 2, 2021 at 11:33 AM Pavel Dovgalyuk 
wrote:

> On 02.02.2021 10:27, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Feb 2, 2021 at 11:18 AM Pavel Dovgalyuk
> > mailto:pavel.dovgal...@ispras.ru>> wrote:
> >
> > This patch checks that ioc is not null before
> > using it in tcp socket tcp_chr_add_watch function.
> >
> > Signed-off-by: Pavel Dovgalyuk  > >
> >
> >
> > Do you have a backtrace or a reproducer when this happens?
> > thanks
>
> Here is the backtrace:
>
> Thread 4 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x72506700 (LWP 64988)]
> object_get_class (obj=obj@entry=0x0) at ../qom/object.c:999
> 999 return obj->class;
> (gdb) bt
> #0  object_get_class (obj=obj@entry=0x0) at ../qom/object.c:999
> #1  0x55b70e26 in QIO_CHANNEL_GET_CLASS (obj=0x0) at
> /home/pasha/ispras/qemu-test/include/io/channel.h:29
> #2  qio_channel_create_watch (ioc=0x0, condition=(G_IO_OUT | G_IO_HUP))
> at ../io/channel.c:281
> #3  0x55c1bf9b in qemu_chr_fe_add_watch
>  (be=be@entry=0x56981648, cond=cond@entry=(G_IO_OUT | G_IO_HUP),
> func=func@entry=0x5597f170 ,
> user_data=user_data@entry=0x569815a0)
>  at /home/pasha/ispras/qemu-test/include/chardev/char.h:229
> #4  0x5597f042 in serial_xmit (s=s@entry=0x569815a0) at
> ../hw/char/serial.c:265
> #5  0x5597f437 in serial_ioport_write (opaque=0x569815a0,
> addr=, val=91, size=) at
> ../hw/char/serial.c:359
>

Thanks, I don't understand how this could happen.

serial_xmit:
   int rc = qemu_chr_fe_write(&s->chr, &s->tsr, 1);

if ((rc == 0 ||
 (rc == -1 && errno == EAGAIN)) &&
s->tsr_retry < MAX_XMIT_RETRY) {
assert(s->watch_tag == 0);
s->watch_tag =
qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
  serial_watch_cb, s);

The watch is added only if fe_write() returned 0 || -1 with EAGAIN.

But tcp_chr_write() should return -1 with EIO if the state is disconnected
(and ioc is NULL), or other errors on disconnect.

Can you provide a reproducer?

thanks


> >
> > ---
> >   chardev/char-socket.c |3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 213a4c8dd0..cef1d9438f 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -385,6 +385,9 @@ static ssize_t tcp_chr_recv(Chardev *chr, char
> > *buf, size_t len)
> >   static GSource *tcp_chr_add_watch(Chardev *chr, GIOCondition cond)
> >   {
> >   SocketChardev *s = SOCKET_CHARDEV(chr);
> > +if (!s->ioc) {
> > +return NULL;
> > +}
> >   return qio_channel_create_watch(s->ioc, cond);
> >   }
> >
> >
>
>

Re: ARM Snapshots Not Backwards-Compatible

[Philippe Mathieu-Daudé]

Cc'ing migration team and qemu-arm@ list.

On 2/3/21 5:01 AM, Aaron Lindsay wrote:
> Hello,
> 
> I'm attempting to restore an AArch64 snapshot taken on QEMU 4.1.0 on
> QEMU 5.2.0, using system mode. My previous impression, possibly from
> https://wiki.qemu.org/Features/Migration/Troubleshooting#Basics was that
> this ought to work:
> 
>> Note that QEMU supports migrating forward between QEMU versions
> 
> Note that I'm using qemu-system-aarch64 with -loadvm.
> 
> However, I've run into several issues I thought I should report. The
> first of them was that this commit changed the address of CBAR, which
> resulted in a mismatch of the register IDs in `cpu_post_load` in
> target/arm/machine.c:
> https://patchwork.kernel.org/project/qemu-devel/patch/20190927144249.2-2-peter.mayd...@linaro.org/
> 
> The second was that several system registers have changed which bits are
> allowed to be written in different circumstances, seemingly as a result
> of a combination of bugfixes and implementation of additional behavior.
> These hit errors detected in `write_list_to_cpustate` in
> target/arm/helper.c.
> 
> The third is that meanings of the bits in env->features (as defined by
> `enum arm_features` in target/arm/cpu.h) has shifted. For example,
> ARM_FEATURE_PXN, ARM_FEATURE_CRC, ARM_FEATURE_VFP, ARM_FEATURE_VFP3,
> ARM_FEATURE_VFP4 have all been removed and ARM_FEATURE_V8_1M has been
> added since 4.1.0. Heck, even I have added a field there in the past.
> Unfortunately, these additions/removals mean that when env->features is
> saved on one version and restored on another the bits can mean different
> things. Notably, the removal of the *VFP features means that a snapshot
> of a CPU reporting it supports ARM_FEATURE_VFP3 on 4.1.0 thinks it's now
> ARM_FEATURE_M on 5.2.0!
> 
> My guess, given the numerous issues and the additional complexity
> required to properly implement backwards-compatible snapshotting, is
> that this is not a primary goal. However, if it is a goal, what steps
> can/should we take to support it more thoroughly?
> 
> Thanks!
> 
> -Aaron
> 
> p.s. Now for an admission: the snapshots I'm testing with were
> originally taken with `-cpu max`. This was unintentional, and I
> understand if the response is that I can't expect `-cpu max` checkpoints
> to work across QEMU versions... but I also don't think that all of these
> issues can be blamed on that (notably CBAR and env->features).
> 

Re: [PATCH] char: don't fail when client is not connected

[Pavel Dovgalyuk]

On 03.02.2021 11:13, Marc-André Lureau wrote:

Hi

On Tue, Feb 2, 2021 at 11:33 AM Pavel Dovgalyuk 
mailto:pavel.dovgal...@ispras.ru>> wrote:


On 02.02.2021 10:27, Marc-André Lureau wrote:
 > Hi
 >
 > On Tue, Feb 2, 2021 at 11:18 AM Pavel Dovgalyuk
 > mailto:pavel.dovgal...@ispras.ru>
>> wrote:
 >
 >     This patch checks that ioc is not null before
 >     using it in tcp socket tcp_chr_add_watch function.
 >
 >     Signed-off-by: Pavel Dovgalyuk mailto:pavel.dovgal...@ispras.ru>
 >     >>
 >
 >
 > Do you have a backtrace or a reproducer when this happens?
 > thanks

Here is the backtrace:

Thread 4 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x72506700 (LWP 64988)]
object_get_class (obj=obj@entry=0x0) at ../qom/object.c:999
999         return obj->class;
(gdb) bt
#0  object_get_class (obj=obj@entry=0x0) at ../qom/object.c:999
#1  0x55b70e26 in QIO_CHANNEL_GET_CLASS (obj=0x0) at
/home/pasha/ispras/qemu-test/include/io/channel.h:29
#2  qio_channel_create_watch (ioc=0x0, condition=(G_IO_OUT | G_IO_HUP))
at ../io/channel.c:281
#3  0x55c1bf9b in qemu_chr_fe_add_watch
      (be=be@entry=0x56981648, cond=cond@entry=(G_IO_OUT |
G_IO_HUP),
func=func@entry=0x5597f170 ,
user_data=user_data@entry=0x569815a0)
      at /home/pasha/ispras/qemu-test/include/chardev/char.h:229
#4  0x5597f042 in serial_xmit (s=s@entry=0x569815a0) at
../hw/char/serial.c:265
#5  0x5597f437 in serial_ioport_write (opaque=0x569815a0,
addr=, val=91, size=) at
../hw/char/serial.c:359


Thanks, I don't understand how this could happen.

serial_xmit:
            int rc = qemu_chr_fe_write(&s->chr, &s->tsr, 1);

             if ((rc == 0 ||
                  (rc == -1 && errno == EAGAIN)) &&
                 s->tsr_retry < MAX_XMIT_RETRY) {
                 assert(s->watch_tag == 0);
                 s->watch_tag =
                     qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
                                           serial_watch_cb, s);

The watch is added only if fe_write() returned 0 || -1 with EAGAIN.

But tcp_chr_write() should return -1 with EIO if the state is 
disconnected (and ioc is NULL), or other errors on disconnect.


Can you provide a reproducer?



That was a record/replay scenario. I've used Fedora cloudinit images, 
that are used in acceptance tests:


qemu-system-x86_64 \
 -display none -vga none -machine pc -smp 1 -m 1024 \
 -monitor tcp:127.0.0.1:8081,server,nowait \
 -serial tcp:127.0.0.1:8082,server,nowait \
 -object filter-replay,id=replay,netdev=hub0port0 \
 -drive 
file=Fedora-Cloud-Base-31-1.9.x86_64.qcow2,snapshot,id=disk0,if=none \

 -drive driver=blkreplay,id=disk0-rr,if=none,image=disk0 \
 -device virtio-blk-pci,drive=disk0-rr,ioeventfd=false \
 -icount shift=1,rr=record,rrfile=replay.bin \
 -drive file=cloudinit.iso,snapshot,id=disk1,if=none \
 -drive driver=blkreplay,id=disk1-rr,if=none,image=disk1 \
 -device virtio-blk-pci,drive=disk1-rr,ioeventfd=false


The failure occurred on replay stage:

qemu-system-x86_64 \
 -display none -vga none -machine pc -smp 1 -m 1024 \
 -monitor tcp:127.0.0.1:8081,server,nowait \
 -serial tcp:127.0.0.1:8082,server,nowait \
 -object filter-replay,id=replay,netdev=hub0port0 \
 -drive 
file=Fedora-Cloud-Base-31-1.9.x86_64.qcow2,snapshot,id=disk0,if=none \

 -drive driver=blkreplay,id=disk0-rr,if=none,image=disk0 \
 -device virtio-blk-pci,drive=disk0-rr,ioeventfd=false \
 -icount shift=1,rr=replay,rrfile=replay.bin \
 -drive file=cloudinit.iso,snapshot,id=disk1,if=none \
 -drive driver=blkreplay,id=disk1-rr,if=none,image=disk1 \
 -device virtio-blk-pci,drive=disk1-rr,ioeventfd=false



thanks


 >
 >     ---
 >       chardev/char-socket.c |    3 +++
 >       1 file changed, 3 insertions(+)
 >
 >     diff --git a/chardev/char-socket.c b/chardev/char-socket.c
 >     index 213a4c8dd0..cef1d9438f 100644
 >     --- a/chardev/char-socket.c
 >     +++ b/chardev/char-socket.c
 >     @@ -385,6 +385,9 @@ static ssize_t tcp_chr_recv(Chardev *chr,
char
 >     *buf, size_t len)
 >       static GSource *tcp_chr_add_watch(Chardev *chr,
GIOCondition cond)
 >       {
 >           SocketChardev *s = SOCKET_CHARDEV(chr);
 >     +    if (!s->ioc) {
 >     +        return NULL;
 >     +    }
 >           return qio_channel_create_watch(s->ioc, cond);
 >       }
 >
 >

Re: [PATCH] MAINTAINERS: suggest myself as co-maintainer for Block Jobs

[Vladimir Sementsov-Ogievskiy]

01.02.2021 19:20, Vladimir Sementsov-Ogievskiy wrote:

01.02.2021 17:50, Kevin Wolf wrote:

Am 01.02.2021 um 12:03 hat Vladimir Sementsov-Ogievskiy geschrieben:

28.01.2021 18:28, John Snow wrote:

On 1/28/21 10:09 AM, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


I'm developing Qemu backup for several years, and finally new backup
architecture, including block-copy generic engine and backup-top filter
landed upstream, great thanks to reviewers and especially to
Max Reitz!

I also have plans of moving other block-jobs onto block-copy, so that
we finally have one generic block copying path, fast and well-formed.

So, now I suggest to bring all parts of backup architecture into
"Block Jobs" subsystem (actually, aio_task is shared with qcow2 and
qemu-co-shared-resource can be reused somewhere else, but I'd keep an
eye on them in context of block-jobs) and add myself as co-maintainer.

Signed-off-by: Vladimir Sementsov-Ogievskiy 


With pleasure:
Reviewed-by: Markus Armbruster 



Absolutely! Glad to see it.

Reviewed-by: John Snow 



[..]


Great!

Reviewed-by: Max Reitz 


Thanks!

Could someone pull it?


I've put it in my block branch (with s/suggest myself/Add Vladimir/ in
the subject line), but I don't know when I'll send the next pull
request. If someone else sends one first, feel free to include it with:

Acked-by: Kevin Wolf 


I don't have any signed PGP key for now, to send pull requests :\
Interesting, could I get one while sitting in Moscow?


If you're planning to send pull requests, should a git tree of yours be
added to the MAINTAINERS sections, too?



I didn't add it because of signed key absence. As it turned out, Denis Lunev 
(my boss) already has a signed key, so it's not a problem.



Unfortunately, Den doesn't have an access to his private key, so this variant 
doesn't work. So, probably someone could recognize me in a video call?

My key is here: 
http://keys.gnupg.net/pks/lookup?search=Vladimir+Sementsov-Ogievskiy&fingerprint=on&op=index
  (note that there is only one Vladimir Sementsov-Ogievskiy, so I can't imagine who can 
that be other than me :)

--
Best regards,
Vladimir

Re: [PATCH] char: don't fail when client is not connected

[Marc-André Lureau]

Hi

On Wed, Feb 3, 2021 at 12:22 PM Pavel Dovgalyuk 
wrote:

> On 03.02.2021 11:13, Marc-André Lureau wrote:
>
> > Can you provide a reproducer?
>
>
> That was a record/replay scenario. I've used Fedora cloudinit images,
> that are used in acceptance tests:
>
> qemu-system-x86_64 \
>   -display none -vga none -machine pc -smp 1 -m 1024 \
>   -monitor tcp:127.0.0.1:8081,server,nowait \
>   -serial tcp:127.0.0.1:8082,server,nowait \
>   -object filter-replay,id=replay,netdev=hub0port0 \
>   -drive
> file=Fedora-Cloud-Base-31-1.9.x86_64.qcow2,snapshot,id=disk0,if=none \
>   -drive driver=blkreplay,id=disk0-rr,if=none,image=disk0 \
>   -device virtio-blk-pci,drive=disk0-rr,ioeventfd=false \
>   -icount shift=1,rr=record,rrfile=replay.bin \
>   -drive file=cloudinit.iso,snapshot,id=disk1,if=none \
>   -drive driver=blkreplay,id=disk1-rr,if=none,image=disk1 \
>   -device virtio-blk-pci,drive=disk1-rr,ioeventfd=false
>
>
> The failure occurred on replay stage:
>
> qemu-system-x86_64 \
>   -display none -vga none -machine pc -smp 1 -m 1024 \
>   -monitor tcp:127.0.0.1:8081,server,nowait \
>   -serial tcp:127.0.0.1:8082,server,nowait \
>   -object filter-replay,id=replay,netdev=hub0port0 \
>   -drive
> file=Fedora-Cloud-Base-31-1.9.x86_64.qcow2,snapshot,id=disk0,if=none \
>   -drive driver=blkreplay,id=disk0-rr,if=none,image=disk0 \
>   -device virtio-blk-pci,drive=disk0-rr,ioeventfd=false \
>   -icount shift=1,rr=replay,rrfile=replay.bin \
>   -drive file=cloudinit.iso,snapshot,id=disk1,if=none \
>   -drive driver=blkreplay,id=disk1-rr,if=none,image=disk1 \
>   -device virtio-blk-pci,drive=disk1-rr,ioeventfd=false
>

Ah, that helps. qemu_chr_write() will return
replay_char_write_event_load(), bypassing the current state. If the
connected state during replay doesn't match the state during recording, it
is possible to reach the bad condition. (there are probably many such
corner-cases situations with replay..)

Can you update the commit message to explain the relation with replay? with
that:
Reviewed-by: Marc-André Lureau 

Daniel, could you review it too?
thanks

Re: [PATCH v8 13/13] s390: Recognize confidential-guest-support option

[Christian Borntraeger]

On 02.02.21 05:13, David Gibson wrote:
> At least some s390 cpu models support "Protected Virtualization" (PV),
> a mechanism to protect guests from eavesdropping by a compromised
> hypervisor.
> 
> This is similar in function to other mechanisms like AMD's SEV and
> POWER's PEF, which are controlled by the "confidential-guest-support"
> machine option.  s390 is a slightly special case, because we already
> supported PV, simply by using a CPU model with the required feature
> (S390_FEAT_UNPACK).
> 
> To integrate this with the option used by other platforms, we
> implement the following compromise:
> 
>  - When the confidential-guest-support option is set, s390 will
>recognize it, verify that the CPU can support PV (failing if not)
>and set virtio default options necessary for encrypted or protected
>guests, as on other platforms.  i.e. if confidential-guest-support
>is set, we will either create a guest capable of entering PV mode,
>or fail outright.
> 
>  - If confidential-guest-support is not set, guests might still be
>able to enter PV mode, if the CPU has the right model.  This may be
>a little surprising, but shouldn't actually be harmful.
> 
> To start a guest supporting Protected Virtualization using the new
> option use the command line arguments:
> -object s390-pv-guest,id=pv0 -machine confidential-guest-support=pv0
> 

This version seems to work fine.

Tested-by: Christian Borntraeger 
Reviewed-by: Christian Borntraeger 


> Signed-off-by: David Gibson 
> ---
>  docs/confidential-guest-support.txt |  3 ++
>  docs/system/s390x/protvirt.rst  | 19 ++---
>  hw/s390x/pv.c   | 62 +
>  hw/s390x/s390-virtio-ccw.c  |  3 ++
>  include/hw/s390x/pv.h   | 17 
>  5 files changed, 98 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/confidential-guest-support.txt 
> b/docs/confidential-guest-support.txt
> index 4da4c91bd3..71d07ba57a 100644
> --- a/docs/confidential-guest-support.txt
> +++ b/docs/confidential-guest-support.txt
> @@ -43,4 +43,7 @@ AMD Secure Encrypted Virtualization (SEV)
>  POWER Protected Execution Facility (PEF)
>  docs/papr-pef.txt
>  
> +s390x Protected Virtualization (PV)
> +docs/system/s390x/protvirt.rst
> +
>  Other mechanisms may be supported in future.
> diff --git a/docs/system/s390x/protvirt.rst b/docs/system/s390x/protvirt.rst
> index 712974ad87..0f481043d9 100644
> --- a/docs/system/s390x/protvirt.rst
> +++ b/docs/system/s390x/protvirt.rst
> @@ -22,15 +22,22 @@ If those requirements are met, the capability 
> `KVM_CAP_S390_PROTECTED`
>  will indicate that KVM can support PVMs on that LPAR.
>  
>  
> -QEMU Settings
> --
> +Running a Protected Virtual Machine
> +---
>  
> -To indicate to the VM that it can transition into protected mode, the
> +To run a PVM you will need to select a CPU model which includes the
>  `Unpack facility` (stfle bit 161 represented by the feature
> -`unpack`/`S390_FEAT_UNPACK`) needs to be part of the cpu model of
> -the VM.
> +`unpack`/`S390_FEAT_UNPACK`), and add these options to the command line::
> +
> +-object s390-pv-guest,id=pv0 \
> +-machine confidential-guest-support=pv0
> +
> +Adding these options will:
> +
> +* Ensure the `unpack` facility is available
> +* Enable the IOMMU by default for all I/O devices
> +* Initialize the PV mechanism
>  
> -All I/O devices need to use the IOMMU.
>  Passthrough (vfio) devices are currently not supported.
>  
>  Host huge page backings are not supported. However guests can use huge
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index ab3a2482aa..93eccfc05d 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -14,8 +14,11 @@
>  #include 
>  
>  #include "cpu.h"
> +#include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/kvm.h"
> +#include "qom/object_interfaces.h"
> +#include "exec/confidential-guest-support.h"
>  #include "hw/s390x/ipl.h"
>  #include "hw/s390x/pv.h"
>  
> @@ -111,3 +114,62 @@ void s390_pv_inject_reset_error(CPUState *cs)
>  /* Report that we are unable to enter protected mode */
>  env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
>  }
> +
> +#define TYPE_S390_PV_GUEST "s390-pv-guest"
> +OBJECT_DECLARE_SIMPLE_TYPE(S390PVGuest, S390_PV_GUEST)
> +
> +/**
> + * S390PVGuest:
> + *
> + * The S390PVGuest object is basically a dummy used to tell the
> + * confidential guest support system to use s390's PV mechanism.
> + *
> + * # $QEMU \
> + * -object s390-pv-guest,id=pv0 \
> + * -machine ...,confidential-guest-support=pv0
> + */
> +struct S390PVGuest {
> +ConfidentialGuestSupport parent_obj;
> +};
> +
> +typedef struct S390PVGuestClass S390PVGuestClass;
> +
> +struct S390PVGuestClass {
> +ConfidentialGuestSupportClass parent_class;
> +};
> +
> +int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> +{
> +if (!object_dynamic_cast(OBJECT(cgs), TYPE_S390_PV

Re: [PATCH v2 2/2] sev: update sev-inject-launch-secret to make gpa optional

[Dr. David Alan Gilbert]

* James Bottomley (j...@linux.ibm.com) wrote:
> On Tue, 2021-01-26 at 12:32 +, Dr. David Alan Gilbert wrote:
> > * James Bottomley (j...@linux.ibm.com) wrote:
> > > If the gpa isn't specified, it's value is extracted from the OVMF
> > > properties table located below the reset vector (and if this
> > > doesn't
> > > exist, an error is returned).  OVMF has defined the GUID for the
> > > SEV
> > > secret area as 4c2eb361-7d9b-4cc3-8081-127c90d3d294 and the format
> > > of
> > > the  is: | where both are uint32_t.  We extract
> > >  and use it as the gpa for the injection.
> > > 
> > > Note: it is expected that the injected secret will also be GUID
> > > described but since qemu can't interpret it, the format is left
> > > undefined here.
> > > 
> > > Signed-off-by: James Bottomley 
> > > 
> > > ---
> > > 
> > > v2: fix line length warning, add more comments about sev area
> > > ---
> > >  qapi/misc-target.json |  2 +-
> > >  target/i386/monitor.c | 27 ++-
> > >  2 files changed, 27 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> > > index 06ef8757f0..0c7491cd82 100644
> > > --- a/qapi/misc-target.json
> > > +++ b/qapi/misc-target.json
> > > @@ -216,7 +216,7 @@
> > >  #
> > >  ##
> > >  { 'command': 'sev-inject-launch-secret',
> > > -  'data': { 'packet-header': 'str', 'secret': 'str', 'gpa':
> > > 'uint64' },
> > > +  'data': { 'packet-header': 'str', 'secret': 'str', '*gpa':
> > > 'uint64' },
> > >'if': 'defined(TARGET_I386)' }
> > >  
> > >  ##
> > > diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> > > index 1bc91442b1..11bdb04155 100644
> > > --- a/target/i386/monitor.c
> > > +++ b/target/i386/monitor.c
> > > @@ -34,6 +34,7 @@
> > >  #include "sev_i386.h"
> > >  #include "qapi/qapi-commands-misc-target.h"
> > >  #include "qapi/qapi-commands-misc.h"
> > > +#include "hw/i386/pc.h"
> > >  
> > >  /* Perform linear address sign extension */
> > >  static hwaddr addr_canonical(CPUArchState *env, hwaddr addr)
> > > @@ -730,9 +731,33 @@ SevCapability
> > > *qmp_query_sev_capabilities(Error **errp)
> > >  return sev_get_capabilities(errp);
> > >  }
> > >  
> > > +#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294"
> > > +struct sev_secret_area {
> > > +uint32_t base;
> > > +uint32_t size;
> > > +};
> > > +
> > >  void qmp_sev_inject_launch_secret(const char *packet_hdr,
> > > -  const char *secret, uint64_t
> > > gpa,
> > > +  const char *secret,
> > > +  bool has_gpa, uint64_t gpa,
> > >Error **errp)
> > >  {
> > > +if (!has_gpa) {
> > > +uint8_t *data;
> > > +struct sev_secret_area *area;
> > > +
> > > +/*
> > > + * not checking length means that this area can't be
> > > versioned
> > > + * by length and would have to be replaced if updated
> > > + */
> > 
> > Can you just explain that a bit more?
> 
> It's referring back to the original concept that the reset vector
> length would tell you what version of the thing you were using.  So if
> you were looking for a property at offset 10 and the length came in as
> 8 the version was too early.  If it was 18 you had a later version and
> your property was present.
> 
> The current scheme uses guids which can be versioned by length if you
> think you'll add extra properties to them.  This one I don't think
> would ever get an extra property, so there's no point checking the
> length.  Not checking the length means if I'm wrong and we do need an
> extra property it will have to be attached to a new guid.
> 
> That's a bit confusing to add to the comment ... how about I just leave
> out the comment entirely?

Yes, I don't think it makes much sense unless you knew the history.

Dave

> > > +if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data,
> > > NULL)) {
> > > +error_setg(errp, "SEV: no secret area found in OVMF,"
> > > +   " gpa must be specified.");
> > > +return;
> > > +}
> > > +area = (struct sev_secret_area *)data;
> > > +gpa = area->base;
> > > +}
> > > +
> > >  sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
> > 
> > Other than me not understanding that comment, I think we're fine:
> 
> Thanks.
> 
> > Reviewed-by: Dr. David Alan Gilbert 
> > 
> > >  }
> > > -- 
> > > 2.26.2
> > > 
> > > 
> 
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 05/12] tools/virtiofsd: Replace the word 'whitelist'

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the words "whitelist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  tools/virtiofsd/passthrough_ll.c  |  6 +++---
>  tools/virtiofsd/passthrough_seccomp.c | 12 ++--
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c 
> b/tools/virtiofsd/passthrough_ll.c
> index 5fb36d94074..4bf86d44211 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -3132,7 +3132,7 @@ static void setup_mounts(const char *source)
>  }
>  
>  /*
> - * Only keep whitelisted capabilities that are needed for file system 
> operation
> + * Only keep capabilities in allowlist that are needed for file system 
> operation
>   * The (possibly NULL) modcaps_in string passed in is free'd before exit.
>   */
>  static void setup_capabilities(char *modcaps_in)
> @@ -3142,8 +3142,8 @@ static void setup_capabilities(char *modcaps_in)
>  capng_restore_state(&cap.saved);
>  
>  /*
> - * Whitelist file system-related capabilities that are needed for a file
> - * server to act like root.  Drop everything else like networking and
> + * Add to allowlist file system-related capabilities that are needed for 
> a
> + * file server to act like root.  Drop everything else like networking 
> and
>   * sysadmin capabilities.
>   *
>   * Exclusions:
> diff --git a/tools/virtiofsd/passthrough_seccomp.c 
> b/tools/virtiofsd/passthrough_seccomp.c
> index a60d7da4b4e..c8b1ebbe830 100644
> --- a/tools/virtiofsd/passthrough_seccomp.c
> +++ b/tools/virtiofsd/passthrough_seccomp.c
> @@ -21,7 +21,7 @@
>  #endif
>  #endif
>  
> -static const int syscall_whitelist[] = {
> +static const int syscall_allowlist[] = {
>  /* TODO ireg sem*() syscalls */
>  SCMP_SYS(brk),
>  SCMP_SYS(capget), /* For CAP_FSETID */
> @@ -115,12 +115,12 @@ static const int syscall_whitelist[] = {
>  };
>  
>  /* Syscalls used when --syslog is enabled */
> -static const int syscall_whitelist_syslog[] = {
> +static const int syscall_allowlist_syslog[] = {
>  SCMP_SYS(send),
>  SCMP_SYS(sendto),
>  };
>  
> -static void add_whitelist(scmp_filter_ctx ctx, const int syscalls[], size_t 
> len)
> +static void add_allowlist(scmp_filter_ctx ctx, const int syscalls[], size_t 
> len)
>  {
>  size_t i;
>  
> @@ -151,10 +151,10 @@ void setup_seccomp(bool enable_syslog)
>  exit(1);
>  }
>  
> -add_whitelist(ctx, syscall_whitelist, G_N_ELEMENTS(syscall_whitelist));
> +add_allowlist(ctx, syscall_allowlist, G_N_ELEMENTS(syscall_allowlist));
>  if (enable_syslog) {
> -add_whitelist(ctx, syscall_whitelist_syslog,
> -  G_N_ELEMENTS(syscall_whitelist_syslog));
> +add_allowlist(ctx, syscall_allowlist_syslog,
> +  G_N_ELEMENTS(syscall_allowlist_syslog));
>  }
>  
>  /* libvhost-user calls this for post-copy migration, we don't need it */
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 07/12] scripts/device-crash-test: Replace the word 'whitelist'

[Philippe Mathieu-Daudé]

On 2/2/21 9:58 PM, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "whitelist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  scripts/device-crash-test | 30 +++---
>  1 file changed, 15 insertions(+), 15 deletions(-)

Eduardo already sent a clever patch:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg778534.html

Re: [PATCH v2 1/2] pc: add parser for OVMF reset block

[Dr. David Alan Gilbert]

* James Bottomley (j...@linux.ibm.com) wrote:
> On Tue, 2021-01-26 at 12:22 +, Dr. David Alan Gilbert wrote:
> > * James Bottomley (j...@linux.ibm.com) wrote:
> > > OVMF is developing a mechanism for depositing a GUIDed table just
> > > below the known location of the reset vector.  The table goes
> > > backwards in memory so all entries are of the form
> > > 
> > > |len|
> > > 
> > > Where  is arbtrary size and type,  is a uint16_t and
> > > describes the entire length of the entry from the beginning of the
> > > data to the end of the guid.
> > > 
> > > The foot of the table is of this form and  for this case
> > > describes the entire size of the table.  The table foot GUID is
> > > defined by OVMF as 96b582de-1fb2-45f7-baea-a366c55a082d and if the
> > > table is present this GUID is just below the reset vector, 48 bytes
> > > before the end of the firmware file.
> > > 
> > > Add a parser for the ovmf reset block which takes a copy of the
> > > block,
> > > if the table foot guid is found, minus the footer and a function
> > > for
> > > later traversal to return the data area of any specified GUIDs.
> > > 
> > > Signed-off-by: James Bottomley 
> > > 
> > > ---
> > > 
> > > v2: fix brace warnings and return values
> > > ---
> > >  hw/i386/pc_sysfw.c   | 106
> > > +++
> > >  include/hw/i386/pc.h |   4 ++
> > >  2 files changed, 110 insertions(+)
> > > 
> > > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > > index 92e90ff013..436b78c587 100644
> > > --- a/hw/i386/pc_sysfw.c
> > > +++ b/hw/i386/pc_sysfw.c
> > > @@ -124,6 +124,107 @@ void
> > > pc_system_flash_cleanup_unused(PCMachineState *pcms)
> > >  }
> > >  }
> > >  
> > > +#define OVMF_TABLE_FOOTER_GUID "96b582de-1fb2-45f7-baea-
> > > a366c55a082d"
> > > +
> > > +static uint8_t *ovmf_table;
> > > +static int ovmf_table_len;
> > > +
> > > +static void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, int
> > > flash_size)
> > 
> > Maybe size_t for flash_size?
> 
> Heh, sure, who knows how big OVMF will get ...  but I get the point
> about an int overflow attack.

To be honest I was more style than actually worrying about overflow; I
like size_t for sizes.

> > > +{
> > > +uint8_t *ptr;
> > > +QemuUUID guid;
> > > +int tot_len;
> > > +
> > > +/* should only be called once */
> > > +if (ovmf_table) {
> > > +return;
> > > +}
> > > +
> > > +/*
> > > + * if this is OVMF there will be a table footer
> > > + * guid 48 bytes before the end of the flash file.  If it's
> > > + * not found, silently abort the flash parsing.
> > > + */
> > > +qemu_uuid_parse(OVMF_TABLE_FOOTER_GUID, &guid);
> > > +guid = qemu_uuid_bswap(guid); /* guids are LE */
> > > +ptr = flash_ptr + flash_size - 48;
> > 
> > I think since flash_size is coming from memory_region_size it's
> > probably rounded to a page size by now, but perhaps we should always
> > check we have enough space before we start moving pointers around
> 
> I think OVMF must be at least a page, so I can add that check.
> 
> > (Given that the OVMF binary might be provided by the guest owner, we
> > have to consider it might be a vector to attack the hypervisor).
> > 
> > > +if (!qemu_uuid_is_equal((QemuUUID *)ptr, &guid)) {
> > > +return;
> > > +}
> > > +
> > > +/* if found, just before is two byte table length */
> > > +ptr -= sizeof(uint16_t);
> > > +tot_len = le16_to_cpu(*(uint16_t *)ptr) - sizeof(guid) -
> > > sizeof(uint16_t);
> > > +
> > > +if (tot_len <= 0) {
> > > +return;
> > > +}
> > > +
> > > +ovmf_table = g_malloc(tot_len);
> > > +ovmf_table_len = tot_len;
> > > +
> > > +/*
> > > + * ptr is the foot of the table, so copy it all to the newly
> > > + * allocated ovmf_table and then set the ovmf_table pointer
> > > + * to the table foot
> > > + */
> > > +memcpy(ovmf_table, ptr - tot_len, tot_len);
> > > +ovmf_table += tot_len;
> > > +}
> > > +
> > > +bool pc_system_ovmf_table_find(const char *entry, uint8_t **data,
> > > +   int *data_len)
> > > +{
> > > +uint8_t *ptr = ovmf_table;
> > > +int tot_len = ovmf_table_len;
> > > +QemuUUID entry_guid;
> > > +
> > > +if (qemu_uuid_parse(entry, &entry_guid) < 0) {
> > > +return false;
> > > +}
> > > +
> > > +if (!ptr) {
> > > +return false;
> > > +}
> > > +
> > > +entry_guid = qemu_uuid_bswap(entry_guid); /* guids are LE */
> > > +while (tot_len > 0) {
> > > +int len;
> > > +QemuUUID *guid;
> > > +
> > > +/*
> > > + * The data structure is
> > > + *   arbitrary length data
> > > + *   2 byte length of entire entry
> > > + *   16 byte guid
> > > + */
> > > +guid = (QemuUUID *)(ptr - sizeof(QemuUUID));
> > > +len = le16_to_cpu(*(uint16_t *)(ptr - sizeof(QemuUUID) -
> > > +sizeof(ui

Re: [PATCH 11/12] tests/fp/fp-test: Replace the word 'blacklist'

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "blacklist"
> appropriately.
>
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
>
> Signed-off-by: Philippe Mathieu-Daudé 

Acked-by: Alex Bennée 

-- 
Alex Bennée

Re: [PULL 00/21] target-arm queue

[Philippe Mathieu-Daudé]

Hi Peter,

On 2/2/21 6:54 PM, Peter Maydell wrote:
> Mostly just bug fixes. The important one here is
>   hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register
> which fixes a buffer overrun that's a security issue if you're running
> KVM on Arm with kernel-irqchip=off (which hopefully nobody is doing in
> a security context, because kernel-irqchip=on is the default and the
> sensible choice for performance).

FYI Prasad mentioned a CVE was requested:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg778659.html

As you said it is an odd configuration, I am not sure it is worth
to wait for the CVE number to add it to the commit (which helps
downstream distributions tracking these).

[updating]

Just got detail from Prasad on IRC, it usually takes ~1 day to get
the CVE number assigned, so maybe worth postponing this until tomorrow.

Prasad, can you reply to this message ASAP once you get the number?

Thanks,

Phil.

> -- PMM
> 
> The following changes since commit cf7ca7d5b9faca13f1f8e3ea92cfb2f741eb0c0e:
> 
>   Merge remote-tracking branch 
> 'remotes/stefanha-gitlab/tags/tracing-pull-request' into staging (2021-02-01 
> 16:28:00 +)
> 
> are available in the Git repository at:
> 
>   https://git.linaro.org/people/pmaydell/qemu-arm.git 
> tags/pull-target-arm-20210202-1
> 
> for you to fetch changes up to 14657850c9cc10948551fbb884c30eb5a3a7370a:
> 
>   hw/arm: Display CPU type in machine description (2021-02-02 17:53:44 +)
> 
> 
> target-arm queue:
>  * hw/intc/arm_gic: Allow to use QTest without crashing
>  * hw/char/exynos4210_uart: Fix buffer size reporting with FIFO disabled
>  * hw/char/exynos4210_uart: Fix missing call to report ready for input
>  * hw/arm/smmuv3: Fix addr_mask for range-based invalidation
>  * hw/ssi/imx_spi: Fix various minor bugs
>  * hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register
>  * hw/arm: Add missing Kconfig dependencies
>  * hw/arm: Display CPU type in machine description
> 
> 
> Bin Meng (5):
>   hw/ssi: imx_spi: Use a macro for number of chip selects supported
>   hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()
>   hw/ssi: imx_spi: Round up the burst length to be multiple of 8
>   hw/ssi: imx_spi: Correct the burst length > 32 bit transfer logic
>   hw/ssi: imx_spi: Correct tx and rx fifo endianness
> 
> Iris Johnson (2):
>   hw/char/exynos4210_uart: Fix buffer size reporting with FIFO disabled
>   hw/char/exynos4210_uart: Fix missing call to report ready for input
> 
> Philippe Mathieu-Daudé (12):
>   hw/intc/arm_gic: Allow to use QTest without crashing
>   hw/ssi: imx_spi: Remove pointless variable initialization
>   hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value
>   hw/ssi: imx_spi: Rework imx_spi_read() to handle block disabled
>   hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled
>   hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register
>   hw/arm/stm32f405_soc: Add missing dependency on OR_IRQ
>   hw/arm/exynos4210: Add missing dependency on OR_IRQ
>   hw/arm/xlnx-versal: Versal SoC requires ZDMA
>   hw/arm/xlnx-versal: Versal SoC requires ZynqMP peripherals
>   hw/net/can: ZynqMP CAN device requires PTIMER
>   hw/arm: Display CPU type in machine description
> 
> Xuzhou Cheng (1):
>   hw/ssi: imx_spi: Disable chip selects when controller is disabled
> 
> Zenghui Yu (1):
>   hw/arm/smmuv3: Fix addr_mask for range-based invalidation
> 
>  include/hw/ssi/imx_spi.h  |   5 +-
>  hw/arm/digic_boards.c |   2 +-
>  hw/arm/microbit.c |   2 +-
>  hw/arm/netduino2.c|   2 +-
>  hw/arm/netduinoplus2.c|   2 +-
>  hw/arm/orangepi.c |   2 +-
>  hw/arm/smmuv3.c   |   4 +-
>  hw/arm/stellaris.c|   4 +-
>  hw/char/exynos4210_uart.c |   7 ++-
>  hw/intc/arm_gic.c |   5 +-
>  hw/ssi/imx_spi.c  | 153 
> +-
>  hw/Kconfig|   1 +
>  hw/arm/Kconfig|   5 ++
>  hw/dma/Kconfig|   3 +
>  hw/dma/meson.build|   2 +-
>  15 files changed, 130 insertions(+), 69 deletions(-)
> 

Re: vnc clipboard support

[Gerd Hoffmann]

  Hi,

> > Well, spice isn't that much better.  Has a short, fixed list too:
> >
> > VD_AGENT_CLIPBOARD_UTF8_TEXT

> There has been some attempts to support generic mime types in Spice clipboard:
> https://lists.freedesktop.org/archives/spice-devel/2018-May/043782.html
> 
> (If I am not mistaken, I think it was inspired on some earlier attempt
> I worked on I lost trace)
> 
> In any case, it seems there is just not enough interest for other
> things beside the existing Spice clipboard types.

I guess "text/plain; charset=utf-8" covers 95% of the use cases.

Personally I *very* rarely cut+paste anything beside plan text,
and in case it is formated text it typically is something like
markdown ;)

> Interoperability
> between OSes quickly becomes problematic for more advanced types, I am
> afraid. (and various OS-specific clipboard & dnd details may raise)

That too.

I'll guess I start with text only, but of course keep the door open to
add more later on.  So I need some way to specify the type(s).  Obvious
choices are fixed list (like vnc/spice) or mime.  The later is more
flexible and future-proof of course, but I'm not sure we'll actually
need that in practice ...

take care,
  Gerd

Re: [PATCH 01/12] ui: Replace the word 'whitelist'

[Gerd Hoffmann]

On Tue, Feb 02, 2021 at 09:58:13PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the words "whitelist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Gerd Hoffmann 

[PATCH] target/s390x/arch_dump: Fixes for the name field in the PT_NOTE section

[Thomas Huth]

According to the "ELF-64 Object File Format" specification:

"The first word in the entry, namesz, identifies the length, in
 bytes, of a name identifying the entry’s owner or originator. The name field
 contains a null-terminated string, with padding as necessary to ensure 8-
 byte alignment for the descriptor field. The length does not include the
 terminating null or the padding."

So we should not include the terminating NUL in the length field here.

Also there is a compiler warning with GCC 9.3 when compiling with
the -fsanitize=thread compiler flag:

 In function 'strncpy',
inlined from 's390x_write_elf64_notes' at ../target/s390x/arch_dump.c:219:9:
 /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
  '__builtin_strncpy' specified bound 8 equals destination size
  [-Werror=stringop-truncation]

Since the name should always be NUL-terminated, we can simply decrease
the size of the strncpy by one here to silence this warning. And while
we're at it, also add an assert() to make sure that the provided names
always fit the size field (which is fine for the current callers, the
function is called once with "CORE" and once with "LINUX" as a name).

Signed-off-by: Thomas Huth 
---
 The ELF-64 spec can be found here, for example:
 https://uclibc.org/docs/elf-64-gen.pdf

 Here's a CI run with the compiler warning:
 https://gitlab.com/huth/qemu/-/jobs/1003508341#L1248

 target/s390x/arch_dump.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
index 50fa0ae4b6..20c3a09707 100644
--- a/target/s390x/arch_dump.c
+++ b/target/s390x/arch_dump.c
@@ -212,11 +212,13 @@ static int s390x_write_elf64_notes(const char *note_name,
 int note_size;
 int ret = -1;
 
+assert(strlen(note_name) < sizeof(note.name));
+
 for (nf = funcs; nf->note_contents_func; nf++) {
 memset(¬e, 0, sizeof(note));
-note.hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
+note.hdr.n_namesz = cpu_to_be32(strlen(note_name));
 note.hdr.n_descsz = cpu_to_be32(nf->contents_size);
-strncpy(note.name, note_name, sizeof(note.name));
+strncpy(note.name, note_name, sizeof(note.name) - 1);
 (*nf->note_contents_func)(¬e, cpu, id);
 
 note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
-- 
2.27.0

Re: macOS (Big Sur, Apple Silicon) 'make check' fails in test-crypto-tlscredsx509

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:50:39PM +0100, Stefan Weil wrote:
> Am 02.02.21 um 21:31 schrieb Stefan Weil:
> 
> > The code uses NULL + offset constructs, so requires a fix.
> > 
> > https://gitlab.com/gnutls/libtasn1/-/merge_requests/71 fixes the unit
> > tests of libtasn1 for me, maybe also the test for QEMU which I still
> > have to check.
> > 
> 
> The QEMU test passes with the patch for libtasn1:

That's great, thanks for chasing this problem.


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: ARM Snapshots Not Backwards-Compatible

[Peter Maydell]

On Wed, 3 Feb 2021 at 04:01, Aaron Lindsay  wrote:
>
> Hello,
>
> I'm attempting to restore an AArch64 snapshot taken on QEMU 4.1.0 on
> QEMU 5.2.0, using system mode. My previous impression, possibly from
> https://wiki.qemu.org/Features/Migration/Troubleshooting#Basics was that
> this ought to work:
>
> > Note that QEMU supports migrating forward between QEMU versions
>
> Note that I'm using qemu-system-aarch64 with -loadvm.

You don't say what machine type and command line you're using. Strictly,
Strictly speaking, the intended guarantee only covers versioned
machines, eg "virt-4.1" on QEMU 4.1 to "virt-4.1" on QEMU 5.2.

> The third is that meanings of the bits in env->features (as defined by
> `enum arm_features` in target/arm/cpu.h) has shifted. For example,
> ARM_FEATURE_PXN, ARM_FEATURE_CRC, ARM_FEATURE_VFP, ARM_FEATURE_VFP3,
> ARM_FEATURE_VFP4 have all been removed and ARM_FEATURE_V8_1M has been
> added since 4.1.0. Heck, even I have added a field there in the past.
> Unfortunately, these additions/removals mean that when env->features is
> saved on one version and restored on another the bits can mean different
> things. Notably, the removal of the *VFP features means that a snapshot
> of a CPU reporting it supports ARM_FEATURE_VFP3 on 4.1.0 thinks it's now
> ARM_FEATURE_M on 5.2.0!

Ow. I didn't realize the env->features was in the migration state :-(
There is no reason for it to be, because it's a constant property
of the CPU. The easy fix is to replace
   VMSTATE_UINT64(env.features, ARMCPU),
in target/arm/machine.c with whatever the syntax is for "ignore
64 bits of data here". Then we'll ignore whatever is coming in
from the source, which we don't need, and we'll stop sending it
out if we're the destination.

thanks
-- PMM

Re: [PATCH 01/12] ui: Replace the word 'whitelist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:13PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the words "whitelist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  ui/console.c   | 2 +-
>  ui/vnc-auth-sasl.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/ui/console.c b/ui/console.c
> index d80ce7037c3..9e13bf9020f 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1661,7 +1661,7 @@ bool dpy_gfx_check_format(QemuConsole *con,
>  return false;
>  }
>  } else {
> -/* default is to whitelist native 32 bpp only */
> +/* default is to allow native 32 bpp only */
>  if (format != qemu_default_pixman_format(32, true)) {
>  return false;
>  }
> diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
> index f67111a3662..dde4b8d4144 100644
> --- a/ui/vnc-auth-sasl.c
> +++ b/ui/vnc-auth-sasl.c
> @@ -288,7 +288,7 @@ static int protocol_client_auth_sasl_step(VncState *vs, 
> uint8_t *data, size_t le
>  goto authreject;
>  }
>  
> -/* Check username whitelist ACL */
> +/* Check username allowlist ACL */

ACL expands to "access control list" so this original comment
was already redundant, and so is the replacement. Using
acronyms is bad practice, so I'd suggest we go for

  "Check the username access control list"

>  if (vnc_auth_sasl_check_access(vs) < 0) {
>  goto authreject;
>  }
> @@ -409,7 +409,7 @@ static int protocol_client_auth_sasl_start(VncState *vs, 
> uint8_t *data, size_t l
>  goto authreject;
>  }
>  
> -/* Check username whitelist ACL */
> +/* Check username allowlist ACL */

Likewise

>  if (vnc_auth_sasl_check_access(vs) < 0) {
>  goto authreject;
>  }

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 03/12] qga: Replace '--blacklist' command line option by '--denylist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:15PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "blacklist"
> appropriately.
> 
> Keep the --blacklist available for backward compatibility.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  docs/interop/qemu-ga.rst | 2 +-
>  qga/main.c   | 6 --
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/interop/qemu-ga.rst b/docs/interop/qemu-ga.rst
> index 9a590bf95cb..89596e646de 100644
> --- a/docs/interop/qemu-ga.rst
> +++ b/docs/interop/qemu-ga.rst
> @@ -79,7 +79,7 @@ Options
>  
>Daemonize after startup (detach from terminal).
>  
> -.. option:: -b, --blacklist=LIST
> +.. option:: -b, --denylist=LIST
>  
>Comma-separated list of RPCs to disable (no spaces, ``?`` to list
>available RPCs).
> diff --git a/qga/main.c b/qga/main.c
> index 249fe06e8e5..66177b9e93d 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -257,7 +257,8 @@ QEMU_COPYRIGHT "\n"
>  #ifdef _WIN32
>  "  -s, --service service commands: install, uninstall, vss-install, 
> vss-uninstall\n"
>  #endif
> -"  -b, --blacklist   comma-separated list of RPCs to disable (no spaces, 
> \"?\"\n"
> +"  --blacklist   backward compatible alias for --denylist (deprecated)\n"
> +"  -b, --denylistcomma-separated list of RPCs to disable (no spaces, 
> \"?\"\n"


"-b" is a bit odd as a short name now, but i guess that's not the end
of the world.

The deprecation should be documented though. Ideally we would report
a warning if the deprecated long arg was used too.

>  "to list available RPCs)\n"
>  "  -D, --dump-conf   dump a qemu-ga config file based on current config\n"
>  "options / command-line parameters to stdout\n"
> @@ -,7 +1112,8 @@ static void config_parse(GAConfig *config, int argc, 
> char **argv)
>  { "method", 1, NULL, 'm' },
>  { "path", 1, NULL, 'p' },
>  { "daemonize", 0, NULL, 'd' },
> -{ "blacklist", 1, NULL, 'b' },
> +{ "denylist", 1, NULL, 'b' },
> +{ "blacklist", 1, NULL, 'b' }, /* deprecated alias for 'denylist' */
>  #ifdef _WIN32
>  { "service", 1, NULL, 's' },
>  #endif
> -- 
> 2.26.2
> 
> 

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

[Bug 1914353] Re: QEMU: aarch64: :GIC: out-of-bounds access via interrupt ID

[P J P]

'CVE-2021-20221' assigned by Red Hat Inc.

** CVE added: https://cve.mitre.org/cgi-bin/cvename.cgi?name=2021-20221

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

Title:
  QEMU: aarch64: :GIC: out-of-bounds access via interrupt ID

Status in QEMU:
  New

Bug description:
  Via [qemu-security] list

  +-- On Sun, 31 Jan 2021, Philippe Mathieu-Daudé wrote --+
  | On 1/31/21 11:34 AM, Philippe Mathieu-Daudé wrote:
  | > Per the ARM Generic Interrupt Controller Architecture specification
  | > (document "ARM IHI 0048B.b (ID072613)"), the SGIINTID field is 4 bit,
  | > not 10:
  | >
  | >- Table 4-21 GICD_SGIR bit assignments
  | >
  | >The Interrupt ID of the SGI to forward to the specified CPU
  | >interfaces. The value of this field is the Interrupt ID, in
  | >the range 0-15, for example a value of 0b0011 specifies
  | >Interrupt ID 3.
  | >
  ...
  | > Correct the irq mask to fix an undefined behavior (which eventually
  | > lead to a heap-buffer-overflow, see [Buglink]):
  | >
  | >$ echo 'writel 0x8000f00 0xff4affb0' | qemu-system-aarch64 -M 
virt,accel=qtest -qtest stdio
  | >[I 1612088147.116987] OPENED
  | >  [R +0.278293] writel 0x8000f00 0xff4affb0
  | >  ../hw/intc/arm_gic.c:1498:13: runtime error: index 944 out of bounds for 
type 'uint8_t [16][8]'
  | >  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior 
../hw/intc/arm_gic.c:1498:13
  | >
  | > Cc: qemu-sta...@nongnu.org
  | > Fixes: 9ee6e8bb853 ("ARMv7 support.")
  | > Buglink: https://bugs.launchpad.net/qemu/+bug/1913916
  | > Buglink: https://bugs.launchpad.net/qemu/+bug/1913917
  ...

  On 210202 1221, Peter Maydell wrote:
  > In both cases the overrun is on the first writel to 0x8000f00,
  > but the fuzzer has for some reason not reported that but instead
  > blundered on until it happens to trigger some other issue that
  > resulted from the memory corruption it induced with the first write.
  >
  ...
  > On the CVE:
  >
  > Since this can affect systems using KVM, this is a security bug for
  > us. However, it only affects an uncommon configuration:
  > you are only vulnerable if you are using "kernel-irqchip=off"
  > (the default is 'on', and turning it off is an odd thing to do).
  >
  > thanks
  > -- PMM
  >

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

Re: [PATCH 04/12] qga: Replace the word 'blacklist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:16PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "blacklist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  qga/guest-agent-core.h |  2 +-
>  qga/commands-posix.c   | 14 +--
>  qga/commands-win32.c   | 10 
>  qga/main.c | 57 +-
>  4 files changed, 42 insertions(+), 41 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: [PULL 00/21] target-arm queue

[P J P]

+-- On Wed, 3 Feb 2021, Philippe Mathieu-Daudé wrote --+
| FYI Prasad mentioned a CVE was requested:
| https://www.mail-archive.com/qemu-devel@nongnu.org/msg778659.html
| 
| As you said it is an odd configuration, I am not sure it is worth
| to wait for the CVE number to add it to the commit (which helps
| downstream distributions tracking these).
| 
| [updating]
| 
| Just got detail from Prasad on IRC, it usually takes ~1 day to get
| the CVE number assigned, so maybe worth postponing this until tomorrow.
| 
| Prasad, can you reply to this message ASAP once you get the number?

'CVE-2021-20221' assigned by Red Hat Inc.
  -> https://bugs.launchpad.net/qemu/+bug/1914353/comments/3

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH 06/12] scripts/tracetool: Replace the word 'whitelist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:18PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the words "whitelist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  scripts/tracetool/__init__.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


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

[Bug 1912224] Re: qemu may freeze during drive-mirroring on fragmented FS

[Max Reitz]

Hi,

As I said on IRC, I’m not sure this additional block_status argument
would be good, because the hole offset needs to be reset when the file
is written to (at least on zero writes; if we additionally stored a data
offset, then that would need to be reset on all writes).  Technically,
mirror can do that, because all writes should go through it, but it
doesn’t seem the right place to cache it there.  Furthermore, depending
on how often writes occur, this cache may end up not doing much.

We could place it in file-posix instead (i.e., it would store the last
offset where SEEK_HOLE/DATA was invoked and the last offset that they
returned, so if a block_status request comes in in that range, it can be
answered without doing a SEEK_HOLE/DATA), but that might suffer from the
same problem of having to invalidate the cache too often.

Though OTOH, as I also admitted on IRC, perhaps we should just try and
see what happens.


As an afterthought, it might be cool to have file-posix use bitmaps to cache 
this status.  In the simplest case, we could have one bitmaps that tells 
whether the block status is known (0 = known, 1 = unknown); this bitmap is 
active, so that writes would automatically invalidate the affected blocks.  And 
then we have another bitmap that for the blocks of known status tells us 
whether they contain data or only zeroes.  This solution wouldn’t suffer from a 
complete cache invalidation on every write.

(Fine-tuning it, we could instead have both bitmaps be inactive, so that
file-posix itself needs to update them on writes, so that all writes
would give their respective blocks a known status, with data writes
making them contain data, and zero writes making them contain zeroes.)

(Perhaps we could consider offering this as a GSoC project?)

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

Title:
  qemu may freeze during drive-mirroring on fragmented FS

Status in QEMU:
  New

Bug description:
  
  We have odd behavior in operation where qemu freeze during long
  seconds, We started an thread about that issue here:
  https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg05623.html

  It happens at least during openstack nova snapshot (qemu blockdev-mirror)
  or live block migration(which include network copy of disk).

  After further troubleshoots, it seems related to FS fragmentation on
  host.

  reproducible at least on:
  Ubuntu 18.04.3/4.18.0-25-generic/qemu-4.0
  Ubuntu 16.04.6/5.10.6/qemu-5.2.0-rc2

  # Lets create a dedicated file system on a SSD/Nvme 60GB disk in my case:
  $sudo mkfs.ext4 /dev/sda3
  $sudo mount /dev/sda3 /mnt
  $df -h /mnt
  Filesystem  Size  Used Avail Use% Mounted on
  /dev/sda3 59G   53M   56G   1% /mnt

  #Create a fragmented disk on it using 2MB Chunks (about 30min):
  $sudo python3 create_fragged_disk.py /mnt 2
  Filling up FS by Creating chunks files in:  /mnt/chunks
  We are probably full as expected!!:  [Errno 28] No space left on device
  Creating fragged disk file:  /mnt/disk

  $ls -lhs 
  59G -rw-r--r-- 1 root root 59G Jan 15 14:08 /mnt/disk

  $ sudo e4defrag -c /mnt/disk
   Total/best extents 41971/30
   Average size per extent1466 KB
   Fragmentation score2
   [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag]
   This file (/mnt/disk) does not need defragmentation.
   Done.

  # the tool^^^ says it is not enough fragmented to be able to defrag.

  #Inject an image on fragmented disk
  sudo chown ubuntu /mnt/disk
  wget 
https://cloud-images.ubuntu.com/bionic/current/bionic-server-cloudimg-amd64.img
  qemu-img convert -O raw  bionic-server-cloudimg-amd64.img \
   bionic-server-cloudimg-amd64.img.raw
  dd conv=notrunc iflag=fullblock if=bionic-server-cloudimg-amd64.img.raw \
  of=/mnt/disk bs=1M
  virt-customize -a /mnt/disk --root-password password:

  # logon run console activity ex: ping -i 0.3 127.0.0.1
  $qemu-system-x86_64 -m 2G -enable-kvm  -nographic \
  -chardev socket,id=test,path=/tmp/qmp-monitor,server,nowait \
  -mon chardev=test,mode=control \
  -drive 
file=/mnt/disk,format=raw,if=none,id=drive-virtio-disk0,cache=none,discard\
  -device 
virtio-blk-pci,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on

  $sync
  $echo 3 | sudo tee -a /proc/sys/vm/drop_caches

  #start drive-mirror via qmp on another SSD/nvme partition
  nc -U /tmp/qmp-monitor
  {"execute":"qmp_capabilities"}
  
{"execute":"drive-mirror","arguments":{"device":"drive-virtio-disk0","target":"/home/ubuntu/mirror","sync":"full","format":"qcow2"}}
  ^^^ qemu console may start to freeze at this step.

  NOTE:
   - smaller chunk sz and bigger disk size the worst it is.
 In operation we also have issue on 400GB disk size with average 13MB/extent
   - Reproducible also on xfs

  

Re: [PATCH 07/12] scripts/device-crash-test: Replace the word 'whitelist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:19PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "whitelist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  scripts/device-crash-test | 30 +++---
>  1 file changed, 15 insertions(+), 15 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: [QEMU-SECURITY] [PATCH] hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register

[P J P]

On Tuesday, 2 February, 2021, 08:45:19 pm IST, Peter Maydell 
 wrote: 
>On the CVE:
>
>Since this can affect systems using KVM, this is a security bug for
>us. However, it only affects an uncommon configuration:
>you are only vulnerable if you are using "kernel-irqchip=off"
>(the default is 'on', and turning it off is an odd thing to do).
>

'CVE-2021-20221' assigned by Red Hat Inc.
  -> https://bugs.launchpad.net/qemu/+bug/1914353/comments/3

Thank you.
---
  -P J P
http://feedmug.com

Re: [PATCH v15 04/23] cpu: Move synchronize_from_tb() to tcg_ops

[Alex Bennée]

Claudio Fontana  writes:

> From: Eduardo Habkost 
>
> Signed-off-by: Eduardo Habkost 
>
> [claudio: wrapped target code in CONFIG_TCG]
> Signed-off-by: Claudio Fontana 
> ---
>  include/hw/core/cpu.h | 20 +++-
>  accel/tcg/cpu-exec.c  |  4 ++--
>  target/arm/cpu.c  |  4 +++-
>  target/avr/cpu.c  |  2 +-
>  target/hppa/cpu.c |  2 +-
>  target/i386/tcg/tcg-cpu.c |  2 +-
>  target/microblaze/cpu.c   |  2 +-
>  target/mips/cpu.c |  4 +++-
>  target/riscv/cpu.c|  2 +-
>  target/rx/cpu.c   |  2 +-
>  target/sh4/cpu.c  |  2 +-
>  target/sparc/cpu.c|  2 +-
>  target/tricore/cpu.c  |  2 +-
>  13 files changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index d0b17dcc4c..b9803885e5 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -86,6 +86,17 @@ typedef struct TcgCpuOperations {
>   * Called when the first CPU is realized.
>   */
>  void (*initialize)(void);
> +/**
> + * @synchronize_from_tb: Synchronize state from a TCG #TranslationBlock
> + *
> + * This is called when we abandon execution of a TB before
> + * starting it, and must set all parts of the CPU state which
> + * the previous TB in the chain may not have updated. This
> + * will need to do more. If this hook is not implemented then
> + * the default is to call @set_pc(tb->pc).
> + */
> +void (*synchronize_from_tb)(CPUState *cpu,
> +const struct TranslationBlock *tb);

Did you miss my comment last time or just not think it flowed better?

  ...TB in the chain may not have updated. By default when no hook is
  defined a call is made to @set_pc(tb->pc). If more state needs to be
  restored the front-end must provide a hook function and restore all the
  state there.

Either way:

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH 05/12] tools/virtiofsd: Replace the word 'whitelist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:17PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the words "whitelist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tools/virtiofsd/passthrough_ll.c  |  6 +++---
>  tools/virtiofsd/passthrough_seccomp.c | 12 ++--
>  2 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH v15 13/23] cpu: move adjust_watchpoint_address to tcg_ops

[Alex Bennée]

Claudio Fontana  writes:

> commit 40612000599e ("arm: Correctly handle watchpoints for BE32 CPUs")
>
> introduced this ARM-specific, TCG-specific hack to adjust the address,
> before checking it with cpu_check_watchpoint.
>
> Make adjust_watchpoint_address optional and move it to tcg_ops.
>
> Signed-off-by: Claudio Fontana 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

[PULL v2 00/21] target-arm queue

[Peter Maydell]

no changes to v1, except adding the CVE identifier to one of the commit
messages.

-- PMM

The following changes since commit cf7ca7d5b9faca13f1f8e3ea92cfb2f741eb0c0e:

  Merge remote-tracking branch 
'remotes/stefanha-gitlab/tags/tracing-pull-request' into staging (2021-02-01 
16:28:00 +)

are available in the Git repository at:

  https://git.linaro.org/people/pmaydell/qemu-arm.git 
tags/pull-target-arm-20210203

for you to fetch changes up to fd8f71b95da86f530aae3d02a14b0ccd9e024772:

  hw/arm: Display CPU type in machine description (2021-02-03 10:15:51 +)


target-arm queue:
 * hw/intc/arm_gic: Allow to use QTest without crashing
 * hw/char/exynos4210_uart: Fix buffer size reporting with FIFO disabled
 * hw/char/exynos4210_uart: Fix missing call to report ready for input
 * hw/arm/smmuv3: Fix addr_mask for range-based invalidation
 * hw/ssi/imx_spi: Fix various minor bugs
 * hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register
 * hw/arm: Add missing Kconfig dependencies
 * hw/arm: Display CPU type in machine description


Bin Meng (5):
  hw/ssi: imx_spi: Use a macro for number of chip selects supported
  hw/ssi: imx_spi: Remove imx_spi_update_irq() in imx_spi_reset()
  hw/ssi: imx_spi: Round up the burst length to be multiple of 8
  hw/ssi: imx_spi: Correct the burst length > 32 bit transfer logic
  hw/ssi: imx_spi: Correct tx and rx fifo endianness

Iris Johnson (2):
  hw/char/exynos4210_uart: Fix buffer size reporting with FIFO disabled
  hw/char/exynos4210_uart: Fix missing call to report ready for input

Philippe Mathieu-Daudé (12):
  hw/intc/arm_gic: Allow to use QTest without crashing
  hw/ssi: imx_spi: Remove pointless variable initialization
  hw/ssi: imx_spi: Rework imx_spi_reset() to keep CONREG register value
  hw/ssi: imx_spi: Rework imx_spi_read() to handle block disabled
  hw/ssi: imx_spi: Rework imx_spi_write() to handle block disabled
  hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register
  hw/arm/stm32f405_soc: Add missing dependency on OR_IRQ
  hw/arm/exynos4210: Add missing dependency on OR_IRQ
  hw/arm/xlnx-versal: Versal SoC requires ZDMA
  hw/arm/xlnx-versal: Versal SoC requires ZynqMP peripherals
  hw/net/can: ZynqMP CAN device requires PTIMER
  hw/arm: Display CPU type in machine description

Xuzhou Cheng (1):
  hw/ssi: imx_spi: Disable chip selects when controller is disabled

Zenghui Yu (1):
  hw/arm/smmuv3: Fix addr_mask for range-based invalidation

 include/hw/ssi/imx_spi.h  |   5 +-
 hw/arm/digic_boards.c |   2 +-
 hw/arm/microbit.c |   2 +-
 hw/arm/netduino2.c|   2 +-
 hw/arm/netduinoplus2.c|   2 +-
 hw/arm/orangepi.c |   2 +-
 hw/arm/smmuv3.c   |   4 +-
 hw/arm/stellaris.c|   4 +-
 hw/char/exynos4210_uart.c |   7 ++-
 hw/intc/arm_gic.c |   5 +-
 hw/ssi/imx_spi.c  | 153 +-
 hw/Kconfig|   1 +
 hw/arm/Kconfig|   5 ++
 hw/dma/Kconfig|   3 +
 hw/dma/meson.build|   2 +-
 15 files changed, 130 insertions(+), 69 deletions(-)

Re: [PATCH 02/12] qga: Rename config key 'blacklist' as 'denylist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:14PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "blacklist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  docs/interop/qemu-ga.rst   |  2 +-
>  qga/main.c | 15 +++
>  tests/test-qga.c   |  8 
>  tests/data/test-qga-config |  2 +-
>  4 files changed, 17 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé 

> 
> diff --git a/docs/interop/qemu-ga.rst b/docs/interop/qemu-ga.rst
> index 3063357bb5d..9a590bf95cb 100644
> --- a/docs/interop/qemu-ga.rst
> +++ b/docs/interop/qemu-ga.rst
> @@ -125,7 +125,7 @@ pidfilestring
>  fsfreeze-hook  string
>  statedir   string
>  verboseboolean
> -blacklist  string list
> +denylist   string list
>  =  ===
>  
>  See also
> diff --git a/qga/main.c b/qga/main.c
> index e7f8f3b1616..249fe06e8e5 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -962,6 +962,7 @@ static void config_load(GAConfig *config)
>  GError *gerr = NULL;
>  GKeyFile *keyfile;
>  g_autofree char *conf = g_strdup(g_getenv("QGA_CONF")) ?: 
> get_relocated_path(QGA_CONF_DEFAULT);
> +const gchar *denylist_key = "denylist";
>  
>  /* read system config */
>  keyfile = g_key_file_new();
> @@ -1008,10 +1009,16 @@ static void config_load(GAConfig *config)
>  config->retry_path =
>  g_key_file_get_boolean(keyfile, "general", "retry-path", &gerr);
>  }
> +
>  if (g_key_file_has_key(keyfile, "general", "blacklist", NULL)) {
> +g_warning("config using deprecated 'blacklist' key, now replaced"
> +  " by the 'denylist' key.");

We should document the config file option deprecation in the norma
place for deprecations.

> +denylist_key = "blacklist";
> +}

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 08/12] seccomp: Replace the word 'blacklist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:20PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "blacklist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  softmmu/qemu-seccomp.c | 16 
>  1 file changed, 8 insertions(+), 8 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 09/12] qemu-options: Replace the word 'blacklist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:21PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "blacklist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  qemu-options.hx | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index d0410f05125..75997ee2ea6 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4275,11 +4275,11 @@ DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \
>  "by the kernel, but typically no longer used by 
> modern\n" \
>  "C library implementations.\n" \
>  "use 'elevateprivileges' to allow or deny QEMU process 
> to elevate\n" \
> -"its privileges by blacklisting all set*uid|gid 
> system calls.\n" \
> +"its privileges by denylisting all set*uid|gid 
> system calls.\n" \

The original description is a bit wierd in how it reads/explains it, so
I think it needs bigger changes:

"use 'elevateprivileges' to allow or deny the QEMU process 
ability
"to elevate privileges using set*uid|gid system calls.\n" \

>  "The value 'children' will deny set*uid|gid system 
> calls for\n" \
>  "main QEMU process but will allow forks and execves 
> to run unprivileged\n" \
>  "use 'spawn' to avoid QEMU to spawn new threads or 
> processes by\n" \
> -" blacklisting *fork and execve\n" \
> +" denylisting *fork and execve\n" \

denylisting is a very strange term to use - its not really a word IMHO.
Better as

" preventing *fork and execve\n" \

or

" blocking *fork and execve\n" \



>  "use 'resourcecontrol' to disable process affinity and 
> schedular priority\n",
>  QEMU_ARCH_ALL)
>  SRST

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: ARM Snapshots Not Backwards-Compatible

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> Cc'ing migration team and qemu-arm@ list.

I'll have to leave the detail of that to the ARM peole; but from a
migration point of view I think we do want the 64 bit ARM migrations to
be stable now.  Please tie incompatible changes to machine types.

Dave

> On 2/3/21 5:01 AM, Aaron Lindsay wrote:
> > Hello,
> > 
> > I'm attempting to restore an AArch64 snapshot taken on QEMU 4.1.0 on
> > QEMU 5.2.0, using system mode. My previous impression, possibly from
> > https://wiki.qemu.org/Features/Migration/Troubleshooting#Basics was that
> > this ought to work:
> > 
> >> Note that QEMU supports migrating forward between QEMU versions
> > 
> > Note that I'm using qemu-system-aarch64 with -loadvm.
> > 
> > However, I've run into several issues I thought I should report. The
> > first of them was that this commit changed the address of CBAR, which
> > resulted in a mismatch of the register IDs in `cpu_post_load` in
> > target/arm/machine.c:
> > https://patchwork.kernel.org/project/qemu-devel/patch/20190927144249.2-2-peter.mayd...@linaro.org/
> > 
> > The second was that several system registers have changed which bits are
> > allowed to be written in different circumstances, seemingly as a result
> > of a combination of bugfixes and implementation of additional behavior.
> > These hit errors detected in `write_list_to_cpustate` in
> > target/arm/helper.c.
> > 
> > The third is that meanings of the bits in env->features (as defined by
> > `enum arm_features` in target/arm/cpu.h) has shifted. For example,
> > ARM_FEATURE_PXN, ARM_FEATURE_CRC, ARM_FEATURE_VFP, ARM_FEATURE_VFP3,
> > ARM_FEATURE_VFP4 have all been removed and ARM_FEATURE_V8_1M has been
> > added since 4.1.0. Heck, even I have added a field there in the past.
> > Unfortunately, these additions/removals mean that when env->features is
> > saved on one version and restored on another the bits can mean different
> > things. Notably, the removal of the *VFP features means that a snapshot
> > of a CPU reporting it supports ARM_FEATURE_VFP3 on 4.1.0 thinks it's now
> > ARM_FEATURE_M on 5.2.0!
> > 
> > My guess, given the numerous issues and the additional complexity
> > required to properly implement backwards-compatible snapshotting, is
> > that this is not a primary goal. However, if it is a goal, what steps
> > can/should we take to support it more thoroughly?
> > 
> > Thanks!
> > 
> > -Aaron
> > 
> > p.s. Now for an admission: the snapshots I'm testing with were
> > originally taken with `-cpu max`. This was unintentional, and I
> > understand if the response is that I can't expect `-cpu max` checkpoints
> > to work across QEMU versions... but I also don't think that all of these
> > issues can be blamed on that (notably CBAR and env->features).
> > 
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 1/1] docs: fix mistake in dirty bitmap feature description

[Denis V. Lunev]

On 2/3/21 2:08 AM, Eric Blake wrote:
> On 2/2/21 4:50 PM, Denis V. Lunev wrote:
>> On 2/3/21 1:15 AM, Eric Blake wrote:
>>> On 1/28/21 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
 28.01.2021 20:13, Denis V. Lunev wrote:
> Original specification says that l1 table size if 64 * l1_size, which
> is obviously wrong. The size of the l1 entry is 64 _bits_, not bytes.
> Thus 64 is to be replaces with 8 as specification says about bytes.
>
> There is also minor tweak, field name is renamed from l1 to l1_table,
> which matches with the later text.
>
> Signed-off-by: Denis V. Lunev 
> CC: Stefan Hajnoczi 
> CC: Vladimir Sementsov-Ogievskiy 
 Reviewed-by: Vladimir Sementsov-Ogievskiy 

>>> I saw the subject "dirty bitmap", and assumed it would go through my
>>> dirty bitmap tree.  In reality, it's unrelated to the dirty bitmap code.
>>>  Would an improved subject line help?
>> hmm. Actually this is about "how the dirty bitmaps are stored in the
>> Parallels Image format". The section is called "dirty bitmap feature".
>>
>> What I can propose? :)
>>
>> "docs: fix mistake in Parallels Image "dirty bitmap" feature description"
>>
>> Will this work for you?
> That feels a bit long; maybe just:
>
> docs: fix Parallels Image "dirty bitmap" section
>
>

looks great to me. Should I resend?

Den

Re: [PATCH 10/12] tests/qemu-iotests: Replace the words 'blacklist/whitelist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:22PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the words "blacklist"
> and "whitelist" appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/qemu-iotests/149 | 14 +++---
>  tests/qemu-iotests/149.out |  8 
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
> index 328fd05a4c9..b1d3f5fad67 100755
> --- a/tests/qemu-iotests/149
> +++ b/tests/qemu-iotests/149
> @@ -500,7 +500,7 @@ configs = [
>  
>  ]
>  
> -blacklist = [
> +denylist = [
>  # We don't have a cast-6 cipher impl for QEMU yet
>  "cast6-256-xts-plain64-sha1",
>  "cast6-128-xts-plain64-sha1",
> @@ -510,17 +510,17 @@ blacklist = [
>  "twofish-192-xts-plain64-sha1",
>  ]

"skiplist" better describes the purpose of this.

>  
> -whitelist = []
> +allowlist = []
>  if "LUKS_CONFIG" in os.environ:
> -whitelist = os.environ["LUKS_CONFIG"].split(",")
> +allowlist = os.environ["LUKS_CONFIG"].split(",")

And "filterlist"

>  
>  for config in configs:
> -if config.name in blacklist:
> -iotests.log("Skipping %s in blacklist" % config.name)
> +if config.name in denylist:
> +iotests.log("Skipping %s in denylist" % config.name)
>  continue
>  
> -if len(whitelist) > 0 and config.name not in whitelist:
> -iotests.log("Skipping %s not in whitelist" % config.name)
> +if len(allowlist) > 0 and config.name not in allowlist:
> +iotests.log("Skipping %s not in allowlist" % config.name)
>  continue
>  
>  test_once(config, qemu_img=False)

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 11/12] tests/fp/fp-test: Replace the word 'blacklist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:23PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "blacklist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/fp/fp-test.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH 12/12] hw/vfio/pci-quirks: Replace the word 'blacklist'

[Daniel P . Berrangé]

On Tue, Feb 02, 2021 at 09:58:24PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "blacklist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/vfio/pci.h|  2 +-
>  hw/vfio/pci-quirks.c | 14 +++---
>  hw/vfio/pci.c|  4 ++--
>  hw/vfio/trace-events |  2 +-
>  4 files changed, 11 insertions(+), 11 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: ARM Snapshots Not Backwards-Compatible

[Peter Maydell]

On Wed, 3 Feb 2021 at 10:28, Dr. David Alan Gilbert  wrote:
>
> * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> > Cc'ing migration team and qemu-arm@ list.
>
> I'll have to leave the detail of that to the ARM peole; but from a
> migration point of view I think we do want the 64 bit ARM migrations to
> be stable now.  Please tie incompatible changes to machine types.

That is the intention, but because there's no upstream testing
of migration compat, we never notice if we get it wrong.
What is x86 doing to keep cross-version migration working ?

thanks
-- PMM

Re: [PATCH 08/12] seccomp: Replace the word 'blacklist'

[Eduardo Otubo]

On 03/02/2021 - 10:15:55, Daniel P. Berrange wrote:
> On Tue, Feb 02, 2021 at 09:58:20PM +0100, Philippe Mathieu-Daudé wrote:
> > Follow the inclusive terminology from the "Conscious Language in your
> > Open Source Projects" guidelines [*] and replace the word "blacklist"
> > appropriately.
> > 
> > [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> > 
> > Signed-off-by: Philippe Mathieu-Daudé 
> > ---
> >  softmmu/qemu-seccomp.c | 16 
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé 

Acked-by: Eduardo Otubo 


signature.asc
Description: PGP signature

Re: [PATCH v8 07/13] confidential guest support: Introduce cgs "ready" flag

[Dr. David Alan Gilbert]

* David Gibson (da...@gibson.dropbear.id.au) wrote:
> The platform specific details of mechanisms for implementing
> confidential guest support may require setup at various points during
> initialization.  Thus, it's not really feasible to have a single cgs
> initialization hook, but instead each mechanism needs its own
> initialization calls in arch or machine specific code.
> 
> However, to make it harder to have a bug where a mechanism isn't
> properly initialized under some circumstances, we want to have a
> common place, late in boot, where we verify that cgs has been
> initialized if it was requested.
> 
> This patch introduces a ready flag to the ConfidentialGuestSupport
> base type to accomplish this, which we verify in
> qemu_machine_creation_done().
> 
> Signed-off-by: David Gibson 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  include/exec/confidential-guest-support.h | 24 +++
>  softmmu/vl.c  | 10 ++
>  target/i386/sev.c |  2 ++
>  3 files changed, 36 insertions(+)
> 
> diff --git a/include/exec/confidential-guest-support.h 
> b/include/exec/confidential-guest-support.h
> index 3db6380e63..5dcf602047 100644
> --- a/include/exec/confidential-guest-support.h
> +++ b/include/exec/confidential-guest-support.h
> @@ -27,6 +27,30 @@ OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, 
> CONFIDENTIAL_GUEST_SUPPORT)
>  
>  struct ConfidentialGuestSupport {
>  Object parent;
> +
> +/*
> + * ready: flag set by CGS initialization code once it's ready to
> + *start executing instructions in a potentially-secure
> + *guest
> + *
> + * The definition here is a bit fuzzy, because this is essentially
> + * part of a self-sanity-check, rather than a strict mechanism.
> + *
> + * It's not fasible to have a single point in the common machine
> + * init path to configure confidential guest support, because
> + * different mechanisms have different interdependencies requiring
> + * initialization in different places, often in arch or machine
> + * type specific code.  It's also usually not possible to check
> + * for invalid configurations until that initialization code.
> + * That means it would be very easy to have a bug allowing CGS
> + * init to be bypassed entirely in certain configurations.
> + *
> + * Silently ignoring a requested security feature would be bad, so
> + * to avoid that we check late in init that this 'ready' flag is
> + * set if CGS was requested.  If the CGS init hasn't happened, and
> + * so 'ready' is not set, we'll abort.
> + */
> +bool ready;
>  };
>  
>  typedef struct ConfidentialGuestSupportClass {
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 1b464e3474..1869ed54a9 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -101,6 +101,7 @@
>  #include "qemu/plugin.h"
>  #include "qemu/queue.h"
>  #include "sysemu/arch_init.h"
> +#include "exec/confidential-guest-support.h"
>  
>  #include "ui/qemu-spice.h"
>  #include "qapi/string-input-visitor.h"
> @@ -2497,6 +2498,8 @@ static void qemu_create_cli_devices(void)
>  
>  static void qemu_machine_creation_done(void)
>  {
> +MachineState *machine = MACHINE(qdev_get_machine());
> +
>  /* Did we create any drives that we failed to create a device for? */
>  drive_check_orphaned();
>  
> @@ -2516,6 +2519,13 @@ static void qemu_machine_creation_done(void)
>  
>  qdev_machine_creation_done();
>  
> +if (machine->cgs) {
> +/*
> + * Verify that Confidential Guest Support has actually been 
> initialized
> + */
> +assert(machine->cgs->ready);
> +}
> +
>  if (foreach_device_config(DEV_GDB, gdbserver_start) < 0) {
>  exit(1);
>  }
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 590cb31fa8..f9e9b5d8ae 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -737,6 +737,8 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
> **errp)
>  qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
>  qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
>  
> +cgs->ready = true;
> +
>  return 0;
>  err:
>  sev_guest = NULL;
> -- 
> 2.29.2
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: iotest failures in head [was: [PATCH v4 00/16] 64bit block-layer: part I]

[Peter Maydell]

On Tue, 2 Feb 2021 at 22:47, Peter Maydell  wrote:
>
> On Tue, 2 Feb 2021 at 17:09, Vladimir Sementsov-Ogievskiy
>  wrote:
> > Note that 30 is known to crash sometimes. Look at
> >
> > "[PATCH RFC 0/5] Fix accidental crash in iotest 30"
> >
> > https://patchew.org/QEMU/20201120161622.1537-1-vsement...@virtuozzo.com/
>
> It certainly seems to be the least reliable iotest in my experience.
> For example it just fell over on ppc64 on MST's latest pullreq merge:
>
> https://lore.kernel.org/qemu-devel/cafeaca8az6qtljp00fyqyuwtqk0tafyupjw0feeppmmvfou...@mail.gmail.com/

Just saw this same error again, on FreeBSD rather than OpenBSD
this time. Can we please just disable 030 if we can't fix it ?

thanks
-- PMM

Re: ARM Snapshots Not Backwards-Compatible

[Dr. David Alan Gilbert]

* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On Wed, 3 Feb 2021 at 10:28, Dr. David Alan Gilbert  
> wrote:
> >
> > * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> > > Cc'ing migration team and qemu-arm@ list.
> >
> > I'll have to leave the detail of that to the ARM peole; but from a
> > migration point of view I think we do want the 64 bit ARM migrations to
> > be stable now.  Please tie incompatible changes to machine types.
> 
> That is the intention, but because there's no upstream testing
> of migration compat, we never notice if we get it wrong.
> What is x86 doing to keep cross-version migration working ?

I know there used to be some of our team running Avocado tests for
compatibility regularly, I'm not sure of the current status.
It's something we also do regularly around when we do downstream
releases, so we tend to catch them then, although even on x86 that
often turns out to be a bit late.

Avocado can take a separate qemu path for source/destination.

Dave

> thanks
> -- PMM
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: ARM Snapshots Not Backwards-Compatible

[Peter Maydell]

On Wed, 3 Feb 2021 at 10:49, Dr. David Alan Gilbert  wrote:
>
> * Peter Maydell (peter.mayd...@linaro.org) wrote:
> > On Wed, 3 Feb 2021 at 10:28, Dr. David Alan Gilbert  
> > wrote:
> > >
> > > * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> > > > Cc'ing migration team and qemu-arm@ list.
> > >
> > > I'll have to leave the detail of that to the ARM peole; but from a
> > > migration point of view I think we do want the 64 bit ARM migrations to
> > > be stable now.  Please tie incompatible changes to machine types.
> >
> > That is the intention, but because there's no upstream testing
> > of migration compat, we never notice if we get it wrong.
> > What is x86 doing to keep cross-version migration working ?
>
> I know there used to be some of our team running Avocado tests for
> compatibility regularly, I'm not sure of the current status.
> It's something we also do regularly around when we do downstream
> releases, so we tend to catch them then, although even on x86 that
> often turns out to be a bit late.

So downstream testing only? I think that unless we either (a) start
doing migration-compat testing consistently upstream or (b) RedHat or
some other downstream start testing and reporting compat issues
to us for aarch64 as they do for x86-64, in practice we're just
not going to have working migration compat despite our best
intentions. (None of the issues Aaron raises were deliberate
compat breaks -- they're all "we made a change we didn't think
affected migration but it turns out that it does".)

thanks
-- PMM

Re: [PATCH 1/4] block/nbd: fix drain dead-lock because of nbd reconnect-delay

[Roman Kagan]

On Thu, Sep 03, 2020 at 10:02:58PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We pause reconnect process during drained section. So, if we have some
> requests, waiting for reconnect we should cancel them, otherwise they
> deadlock the drained section.
> 
> How to reproduce:
> 
> 1. Create an image:
>qemu-img create -f qcow2 xx 100M
> 
> 2. Start NBD server:
>qemu-nbd xx
> 
> 3. Start vm with second nbd disk on node2, like this:
> 
>   ./build/x86_64-softmmu/qemu-system-x86_64 -nodefaults -drive \
>  file=/work/images/cent7.qcow2 -drive \
>  
> driver=nbd,server.type=inet,server.host=192.168.100.5,server.port=10809,reconnect-delay=60
>  \
>  -vnc :0 -m 2G -enable-kvm -vga std
> 
> 4. Access the vm through vnc (or some other way?), and check that NBD
>drive works:
> 
>dd if=/dev/sdb of=/dev/null bs=1M count=10
> 
>- the command should succeed.
> 
> 5. Now, kill the nbd server, and run dd in the guest again:
> 
>dd if=/dev/sdb of=/dev/null bs=1M count=10
> 
> Now Qemu is trying to reconnect, and dd-generated requests are waiting
> for the connection (they will wait up to 60 seconds (see
> reconnect-delay option above) and than fail). But suddenly, vm may
> totally hang in the deadlock. You may need to increase reconnect-delay
> period to catch the dead-lock.
> 
> VM doesn't respond because drain dead-lock happens in cpu thread with
> global mutex taken. That's not good thing by itself and is not fixed
> by this commit (true way is using iothreads). Still this commit fixes
> drain dead-lock itself.
> 
> Note: probably, we can instead continue to reconnect during drained
> section. To achieve this, we may move negotiation to the connect thread
> to make it independent of bs aio context. But expanding drained section
> doesn't seem good anyway. So, let's now fix the bug the simplest way.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 9daf003bea..912ea27be7 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -242,6 +242,11 @@ static void coroutine_fn 
> nbd_client_co_drain_begin(BlockDriverState *bs)
>  }
>  
>  nbd_co_establish_connection_cancel(bs, false);
> +
> +if (s->state == NBD_CLIENT_CONNECTING_WAIT) {
> +s->state = NBD_CLIENT_CONNECTING_NOWAIT;
> +qemu_co_queue_restart_all(&s->free_sema);
> +}
>  }
>  
>  static void coroutine_fn nbd_client_co_drain_end(BlockDriverState *bs)

This basically defeats the whole purpose of reconnect: if the nbd client
is trying to reconnect, drain effectively cancels that and makes all
in-flight requests to complete with an error.

I'm not suggesting to revert this patch (it's now in the tree as commit
8c517de24a), because the deadlock is no better, but I'm afraid the only
real fix is to implement reconnect during the drain section.  I'm still
trying to get my head around it so no patch yet, but I just wanted to
bring this up in case anybody beats me to it.

Thanks,
Roman.

Re: ARM Snapshots Not Backwards-Compatible

[Dr. David Alan Gilbert]

* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On Wed, 3 Feb 2021 at 10:49, Dr. David Alan Gilbert  
> wrote:
> >
> > * Peter Maydell (peter.mayd...@linaro.org) wrote:
> > > On Wed, 3 Feb 2021 at 10:28, Dr. David Alan Gilbert  
> > > wrote:
> > > >
> > > > * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> > > > > Cc'ing migration team and qemu-arm@ list.
> > > >
> > > > I'll have to leave the detail of that to the ARM peole; but from a
> > > > migration point of view I think we do want the 64 bit ARM migrations to
> > > > be stable now.  Please tie incompatible changes to machine types.
> > >
> > > That is the intention, but because there's no upstream testing
> > > of migration compat, we never notice if we get it wrong.
> > > What is x86 doing to keep cross-version migration working ?
> >
> > I know there used to be some of our team running Avocado tests for
> > compatibility regularly, I'm not sure of the current status.
> > It's something we also do regularly around when we do downstream
> > releases, so we tend to catch them then, although even on x86 that
> > often turns out to be a bit late.
> 
> So downstream testing only?

I thought there used to be some regular avocado testing of upstream but
I'm not sure if it's all architectures and I'm not sure if it's still
happening; I haven't seen any migration issues from it for a while,
which makes me think it isn't.

> I think that unless we either (a) start
> doing migration-compat testing consistently upstream or (b) RedHat or
> some other downstream start testing and reporting compat issues
> to us for aarch64 as they do for x86-64, in practice we're just
> not going to have working migration compat despite our best
> intentions. (None of the issues Aaron raises were deliberate
> compat breaks -- they're all "we made a change we didn't think
> affected migration but it turns out that it does".)

I'd agree; we still hit this too often on x86 as well.

Dave

> thanks
> -- PMM
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PULL v2 00/21] target-arm queue

[Philippe Mathieu-Daudé]

On 2/3/21 11:17 AM, Peter Maydell wrote:
> no changes to v1, except adding the CVE identifier to one of the commit
> messages.

Thank you Peter for taking the time to do this change.

> 
> -- PMM
> 
> The following changes since commit cf7ca7d5b9faca13f1f8e3ea92cfb2f741eb0c0e:
> 
>   Merge remote-tracking branch 
> 'remotes/stefanha-gitlab/tags/tracing-pull-request' into staging (2021-02-01 
> 16:28:00 +)
> 
> are available in the Git repository at:
> 
>   https://git.linaro.org/people/pmaydell/qemu-arm.git 
> tags/pull-target-arm-20210203
> 
> for you to fetch changes up to fd8f71b95da86f530aae3d02a14b0ccd9e024772:
> 
>   hw/arm: Display CPU type in machine description (2021-02-03 10:15:51 +)
> 
> 
> target-arm queue:
>  * hw/intc/arm_gic: Allow to use QTest without crashing
>  * hw/char/exynos4210_uart: Fix buffer size reporting with FIFO disabled
>  * hw/char/exynos4210_uart: Fix missing call to report ready for input
>  * hw/arm/smmuv3: Fix addr_mask for range-based invalidation
>  * hw/ssi/imx_spi: Fix various minor bugs
>  * hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register
>  * hw/arm: Add missing Kconfig dependencies
>  * hw/arm: Display CPU type in machine description
> 
> 

Re: [PATCH 1/1] virtiofsd: Allow to build it without the tools

[Dr. David Alan Gilbert]

* Wainer dos Santos Moschetta (waine...@redhat.com) wrote:
> 
> On 2/2/21 2:55 AM, misono.tomoh...@fujitsu.com wrote:
> > > Subject: [PATCH 1/1] virtiofsd: Allow to build it without the tools
> > > 
> > > This changed the Meson build script to allow virtiofsd be built even
> > > though the tools build is disabled, thus honoring the --enable-virtiofsd
> > > option.
> > > 
> > > Signed-off-by: Wainer dos Santos Moschetta 
> > I misunderstood that virtiofsd builds somehow depends on tools build at 
> > that time.
> > Thanks for fixing. I did quick build test.
> Thanks for the review and test!
> 
> If not needed a v2 for this patch, please could the maintainer add to the
> commit message:
> 
>   Fixes: cece116c939d219070b250338439c2d16f94e3da (configure: add option for
> virtiofsd)

OK, I cna add that; I'll take it through virtiofs next time I need to do
a pull.

> - Wainer
> 
> > 
> > Reviewed-by: Misono Tomohiro 
> > 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[PATCH 0/6] Move remaining x86 Travis jobs to the gitlab-CI

[Thomas Huth]

Since Travis changed their policies, travis-ci.org will soon become
completely useless for the QEMU project. We should now really make sure
that we move the remaining tests as good as possible to the gitlab-CI
instead. Since the gitlab-CI has already quite a lot of jobs, I tried
to squeeze the missing bits as good as possible into the existing jobs
instead of adding a separate job for each and ever test that we had
in the Travis-CI - I hope that will help to avoid increasing the stress
on the CI system too much. 

Philippe Mathieu-Daudé (1):
  travis.yml: Move gprof/gcov test across to gitlab

Thomas Huth (5):
  travis.yml: Move the -fsanitize=undefined test to the gitlab-CI
  travis.yml: Move the --enable-modules test to the gitlab-CI
  travis.yml: Remove the --enable-debug jobs
  target/s390x/arch_dump: Fixes for the name field in the PT_NOTE
section
  travis.yml: Move the -fsanitize=thread compile-testing to the
gitlab-CI

 .gitlab-ci.yml |  27 -
 .travis.yml| 110 -
 MAINTAINERS|   2 +-
 scripts/{travis => ci}/coverage-summary.sh |   2 +-
 target/s390x/arch_dump.c   |   6 +-
 tests/docker/dockerfiles/ubuntu2004.docker |   2 +
 6 files changed, 33 insertions(+), 116 deletions(-)
 rename scripts/{travis => ci}/coverage-summary.sh (92%)

-- 
2.27.0

[PATCH 1/6] travis.yml: Move gprof/gcov test across to gitlab

[Thomas Huth]

From: Philippe Mathieu-Daudé 

Similarly to commit 8cdb2cef3f1, move the gprof/gcov test to GitLab.

The coverage-summary.sh script is not Travis-CI specific, make it
generic.

Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20201108204535.2319870-10-phi...@redhat.com>
[thuth: Add gcovr and bsdmainutils which are required for the
overage-summary.sh script to the ubuntu docker file]
Signed-off-by: Thomas Huth 
---
 .gitlab-ci.yml | 12 
 .travis.yml| 14 --
 MAINTAINERS|  2 +-
 scripts/{travis => ci}/coverage-summary.sh |  2 +-
 tests/docker/dockerfiles/ubuntu2004.docker |  2 ++
 5 files changed, 16 insertions(+), 16 deletions(-)
 rename scripts/{travis => ci}/coverage-summary.sh (92%)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 7c0db64710..8b97b512bb 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -468,6 +468,18 @@ check-deprecated:
 MAKE_CHECK_ARGS: check-tcg
   allow_failure: true
 
+# gprof/gcov are GCC features
+build-gprof-gcov:
+  <<: *native_build_job_definition
+  variables:
+IMAGE: ubuntu2004
+CONFIGURE_ARGS: --enable-gprof --enable-gcov
+MAKE_CHECK_ARGS: build-tcg
+TARGETS: aarch64-softmmu mips64-softmmu ppc64-softmmu
+ riscv64-softmmu s390x-softmmu x86_64-softmmu
+  after_script:
+- ${CI_PROJECT_DIR}/scripts/ci/coverage-summary.sh
+
 build-oss-fuzz:
   <<: *native_build_job_definition
   variables:
diff --git a/.travis.yml b/.travis.yml
index 5f1dea873e..76b69f6de1 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -166,20 +166,6 @@ jobs:
   compiler: clang
 
 
-# gprof/gcov are GCC features
-- name: "GCC gprof/gcov"
-  dist: bionic
-  addons:
-apt:
-  packages:
-- ninja-build
-  env:
-- CONFIG="--enable-gprof --enable-gcov --disable-libssh
-  --target-list=${MAIN_SOFTMMU_TARGETS}"
-  after_success:
-- ${SRC_DIR}/scripts/travis/coverage-summary.sh
-
-
 # Using newer GCC with sanitizers
 - name: "GCC9 with sanitizers (softmmu)"
   dist: bionic
diff --git a/MAINTAINERS b/MAINTAINERS
index bcd88668bc..f14a2e6eb5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3193,7 +3193,7 @@ R: Philippe Mathieu-Daudé 
 S: Maintained
 F: .github/lockdown.yml
 F: .travis.yml
-F: scripts/travis/
+F: scripts/ci/
 F: .shippable.yml
 F: tests/docker/
 F: tests/vm/
diff --git a/scripts/travis/coverage-summary.sh b/scripts/ci/coverage-summary.sh
similarity index 92%
rename from scripts/travis/coverage-summary.sh
rename to scripts/ci/coverage-summary.sh
index d7086cf9ca..8d9fb4de40 100755
--- a/scripts/travis/coverage-summary.sh
+++ b/scripts/ci/coverage-summary.sh
@@ -3,7 +3,7 @@
 # Author: Alex Bennée 
 #
 # Summerise the state of code coverage with gcovr and tweak the output
-# to be more sane on Travis hosts. As we expect to be executed on a
+# to be more sane on CI runner. As we expect to be executed on a
 # throw away CI instance we do spam temp files all over the shop. You
 # most likely don't want to execute this script but just call gcovr
 # directly. See also "make coverage-report"
diff --git a/tests/docker/dockerfiles/ubuntu2004.docker 
b/tests/docker/dockerfiles/ubuntu2004.docker
index 8519584d2b..9750016e51 100644
--- a/tests/docker/dockerfiles/ubuntu2004.docker
+++ b/tests/docker/dockerfiles/ubuntu2004.docker
@@ -1,8 +1,10 @@
 FROM ubuntu:20.04
 ENV PACKAGES flex bison \
+bsdmainutils \
 ccache \
 clang-10\
 gcc \
+gcovr \
 genisoimage \
 gettext \
 git \
-- 
2.27.0

[PATCH 3/6] travis.yml: Move the --enable-modules test to the gitlab-CI

[Thomas Huth]

Simply add the flag to an existing job, no need for yet another
job here.

Signed-off-by: Thomas Huth 
---
 .gitlab-ci.yml | 1 +
 .travis.yml| 6 --
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 41e11b41e4..4654798523 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -223,6 +223,7 @@ build-system-centos:
   variables:
 IMAGE: centos8
 CONFIGURE_ARGS: --disable-nettle --enable-gcrypt --enable-fdt=system
+--enable-modules
 TARGETS: ppc64-softmmu or1k-softmmu s390x-softmmu
   x86_64-softmmu rx-softmmu sh4-softmmu nios2-softmmu
 MAKE_CHECK_ARGS: check-build
diff --git a/.travis.yml b/.travis.yml
index d1e9016da5..45dd017420 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -132,12 +132,6 @@ jobs:
 - CONFIG="--enable-debug-tcg --disable-system"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug-tcg"
 
-# Module builds are mostly of interest to major distros
-- name: "GCC modules (main-softmmu)"
-  env:
-- CONFIG="--enable-modules --target-list=${MAIN_SOFTMMU_TARGETS}"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
-
 
 # Using newer GCC with sanitizers
 - name: "GCC9 with sanitizers (softmmu)"
-- 
2.27.0

[PATCH 5/6] target/s390x/arch_dump: Fixes for the name field in the PT_NOTE section

[Thomas Huth]

According to the "ELF-64 Object File Format" specification:

"The first word in the entry, namesz, identifies the length, in
 bytes, of a name identifying the entry’s owner or originator. The name field
 contains a null-terminated string, with padding as necessary to ensure 8-
 byte alignment for the descriptor field. The length does not include the
 terminating null or the padding."

So we should not include the terminating NUL in the length field here.

Also there is a compiler warning with GCC 9.3 when compiling with
the -fsanitize=thread compiler flag:

 In function 'strncpy',
inlined from 's390x_write_elf64_notes' at ../target/s390x/arch_dump.c:219:9:
 /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
  '__builtin_strncpy' specified bound 8 equals destination size
  [-Werror=stringop-truncation]

Since the name should always be NUL-terminated, we can simply decrease
the size of the strncpy by one here to silence this warning. And while
we're at it, also add an assert() to make sure that the provided names
always fit the size field (which is fine for the current callers, the
function is called once with "CORE" and once with "LINUX" as a name).

Signed-off-by: Thomas Huth 
---
 target/s390x/arch_dump.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
index 50fa0ae4b6..20c3a09707 100644
--- a/target/s390x/arch_dump.c
+++ b/target/s390x/arch_dump.c
@@ -212,11 +212,13 @@ static int s390x_write_elf64_notes(const char *note_name,
 int note_size;
 int ret = -1;
 
+assert(strlen(note_name) < sizeof(note.name));
+
 for (nf = funcs; nf->note_contents_func; nf++) {
 memset(¬e, 0, sizeof(note));
-note.hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
+note.hdr.n_namesz = cpu_to_be32(strlen(note_name));
 note.hdr.n_descsz = cpu_to_be32(nf->contents_size);
-strncpy(note.name, note_name, sizeof(note.name));
+strncpy(note.name, note_name, sizeof(note.name) - 1);
 (*nf->note_contents_func)(¬e, cpu, id);
 
 note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
-- 
2.27.0

[PATCH 2/6] travis.yml: Move the -fsanitize=undefined test to the gitlab-CI

[Thomas Huth]

Add it to the existing Clang job and also add a job that covers the
linux-user code with this compiler flag. To make sure that the detected
problems are not simply ignored, let's also use "-fno-sanitize-recover=..."
now instead.

Signed-off-by: Thomas Huth 
---
 .gitlab-ci.yml | 13 +++--
 .travis.yml| 27 ---
 2 files changed, 11 insertions(+), 29 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 8b97b512bb..41e11b41e4 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -433,13 +433,22 @@ build-some-softmmu-plugins:
 TARGETS: xtensa-softmmu arm-softmmu aarch64-softmmu alpha-softmmu
 MAKE_CHECK_ARGS: check-tcg
 
-build-clang:
+clang-system:
   <<: *native_build_job_definition
   variables:
 IMAGE: fedora
 CONFIGURE_ARGS: --cc=clang --cxx=clang++
+  --extra-cflags=-fno-sanitize-recover=undefined
 TARGETS: alpha-softmmu arm-softmmu m68k-softmmu mips64-softmmu
-  ppc-softmmu s390x-softmmu arm-linux-user
+  ppc-softmmu s390x-softmmu
+MAKE_CHECK_ARGS: check-qtest check-block check-tcg
+
+clang-user:
+  <<: *native_build_job_definition
+  variables:
+IMAGE: fedora
+CONFIGURE_ARGS: --cc=clang --cxx=clang++ --disable-system
+  --extra-cflags=-fno-sanitize-recover=undefined
 MAKE_CHECK_ARGS: check
 
 # These targets are on the way out
diff --git a/.travis.yml b/.travis.yml
index 76b69f6de1..d1e9016da5 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -139,33 +139,6 @@ jobs:
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
 
 
-# Test with Clang for compile portability (Travis uses clang-5.0)
-- name: "Clang (user)"
-  env:
-- CONFIG="--disable-system --host-cc=clang --cxx=clang++"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-clang-default"
-  compiler: clang
-
-
-- name: "Clang (main-softmmu)"
-  env:
-- CONFIG="--target-list=${MAIN_SOFTMMU_TARGETS}
-  --host-cc=clang --cxx=clang++"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-clang-sanitize"
-  compiler: clang
-  before_script:
-- mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
-- ${SRC_DIR}/configure ${CONFIG} --extra-cflags="-fsanitize=undefined 
-Werror" || { cat config.log meson-logs/meson-log.txt && exit 1; }
-
-
-- name: "Clang (other-softmmu)"
-  env:
-- CONFIG="--disable-user --target-list-exclude=${MAIN_SOFTMMU_TARGETS}
-  --host-cc=clang --cxx=clang++"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-clang-default"
-  compiler: clang
-
-
 # Using newer GCC with sanitizers
 - name: "GCC9 with sanitizers (softmmu)"
   dist: bionic
-- 
2.27.0

[PATCH 4/6] travis.yml: Remove the --enable-debug jobs

[Thomas Huth]

We already have such jobs in the gitlab-CI ("build-some-softmmu" and
"build-user-plugins"), so we can simply drop these from the Travis-CI.

Signed-off-by: Thomas Huth 
---
 .travis.yml | 12 
 1 file changed, 12 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 45dd017420..b3fc72f561 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -119,18 +119,6 @@ after_script:
 
 jobs:
   include:
-# --enable-debug implies --enable-debug-tcg, also runs quite a bit slower
-- name: "GCC debug (main-softmmu)"
-  env:
-- CONFIG="--enable-debug --target-list=${MAIN_SOFTMMU_TARGETS}"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug"
-
-
-# TCG debug can be run just on its own and is mostly agnostic to 
user/softmmu distinctions
-- name: "GCC debug (user)"
-  env:
-- CONFIG="--enable-debug-tcg --disable-system"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug-tcg"
 
 
 # Using newer GCC with sanitizers
-- 
2.27.0

[PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()

[Stefan Hajnoczi]

Both lo_open() and lo_create() have similar code to open a file. Extract
a common lo_do_open() function from lo_open() that will be used by
lo_create() in a later commit.

Since lo_do_open() does not otherwise need fuse_req_t req, convert
lo_add_fd_mapping() to use struct lo_data *lo instead.

Signed-off-by: Stefan Hajnoczi 
---
 tools/virtiofsd/passthrough_ll.c | 73 
 1 file changed, 46 insertions(+), 27 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 5fb36d9407..e63cbd3fb7 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -459,17 +459,17 @@ static void lo_map_remove(struct lo_map *map, size_t key)
 }
 
 /* Assumes lo->mutex is held */
-static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
+static ssize_t lo_add_fd_mapping(struct lo_data *lo, int fd)
 {
 struct lo_map_elem *elem;
 
-elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
+elem = lo_map_alloc_elem(&lo->fd_map);
 if (!elem) {
 return -1;
 }
 
 elem->fd = fd;
-return elem - lo_data(req)->fd_map.elems;
+return elem - lo->fd_map.elems;
 }
 
 /* Assumes lo->mutex is held */
@@ -1651,6 +1651,38 @@ static void update_open_flags(int writeback, int 
allow_direct_io,
 }
 }
 
+static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
+  struct fuse_file_info *fi)
+{
+char buf[64];
+ssize_t fh;
+int fd;
+
+update_open_flags(lo->writeback, lo->allow_direct_io, fi);
+
+sprintf(buf, "%i", inode->fd);
+fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
+if (fd == -1) {
+return -errno;
+}
+
+pthread_mutex_lock(&lo->mutex);
+fh = lo_add_fd_mapping(lo, fd);
+pthread_mutex_unlock(&lo->mutex);
+if (fh == -1) {
+close(fd);
+return ENOMEM;
+}
+
+fi->fh = fh;
+if (lo->cache == CACHE_NONE) {
+fi->direct_io = 1;
+} else if (lo->cache == CACHE_ALWAYS) {
+fi->keep_cache = 1;
+}
+return 0;
+}
+
 static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
   mode_t mode, struct fuse_file_info *fi)
 {
@@ -1691,7 +1723,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, 
const char *name,
 ssize_t fh;
 
 pthread_mutex_lock(&lo->mutex);
-fh = lo_add_fd_mapping(req, fd);
+fh = lo_add_fd_mapping(lo, fd);
 pthread_mutex_unlock(&lo->mutex);
 if (fh == -1) {
 close(fd);
@@ -1892,38 +1924,25 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, 
int datasync,
 
 static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
 {
-int fd;
-ssize_t fh;
-char buf[64];
 struct lo_data *lo = lo_data(req);
+struct lo_inode *inode = lo_inode(req, ino);
+int err;
 
 fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
  fi->flags);
 
-update_open_flags(lo->writeback, lo->allow_direct_io, fi);
-
-sprintf(buf, "%i", lo_fd(req, ino));
-fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
-if (fd == -1) {
-return (void)fuse_reply_err(req, errno);
-}
-
-pthread_mutex_lock(&lo->mutex);
-fh = lo_add_fd_mapping(req, fd);
-pthread_mutex_unlock(&lo->mutex);
-if (fh == -1) {
-close(fd);
-fuse_reply_err(req, ENOMEM);
+if (!inode) {
+fuse_reply_err(req, EBADF);
 return;
 }
 
-fi->fh = fh;
-if (lo->cache == CACHE_NONE) {
-fi->direct_io = 1;
-} else if (lo->cache == CACHE_ALWAYS) {
-fi->keep_cache = 1;
+err = lo_do_open(lo, inode, fi);
+lo_inode_put(lo, &inode);
+if (err) {
+fuse_reply_err(req, err);
+} else {
+fuse_reply_open(req, fi);
 }
-fuse_reply_open(req, fi);
 }
 
 static void lo_release(fuse_req_t req, fuse_ino_t ino,
-- 
2.29.2

[PATCH v4 0/3] virtiofsd: prevent opening of special files (CVE-2020-35517)

[Stefan Hajnoczi]

v3:
 * Restructure lo_create() to handle externally-created files (we need
   to allocate an inode for them) [Greg]
 * Patch 1 & 2 refactor the code so that Patch 3 can implement the CVE fix
v3:
 * Protect lo_create() [Greg]
v2:
 * Add doc comment clarifying that symlinks are traversed client-side
   [Daniel]

A well-behaved FUSE client does not attempt to open special files with
FUSE_OPEN because they are handled on the client side (e.g. device nodes
are handled by client-side device drivers).

The check to prevent virtiofsd from opening special files is missing in
a few cases, most notably FUSE_OPEN. A malicious client can cause
virtiofsd to open a device node, potentially allowing the guest to
escape. This can be exploited by a modified guest device driver. It is
not exploitable from guest userspace since the guest kernel will handle
special files inside the guest instead of sending FUSE requests.

This patch series fixes this issue by introducing the lo_inode_open() function
to check the file type before opening it. This is a short-term solution because
it does not prevent a compromised virtiofsd process from opening device nodes
on the host.

This issue was diagnosed on public IRC and is therefore already known
and not embargoed.

Reported-by: Alex Xu 
Fixes: CVE-2020-35517

Stefan Hajnoczi (3):
  virtiofsd: extract lo_do_open() from lo_open()
  virtiofsd: optionally return inode pointer from lo_do_lookup()
  virtiofsd: prevent opening of special files (CVE-2020-35517)

 tools/virtiofsd/passthrough_ll.c | 221 ---
 1 file changed, 145 insertions(+), 76 deletions(-)

-- 
2.29.2

[PATCH 6/6] travis.yml: Move the -fsanitize=thread compile-testing to the gitlab-CI

[Thomas Huth]

It's only about compile-testing (there is too much noise when running
the tests), so let's simply add the -fsanitize=thread flag to a job that
only compiles the sources. The "build-gprof-gcov" seems to be a good
candidate.

Signed-off-by: Thomas Huth 
---
 .gitlab-ci.yml |  1 +
 .travis.yml| 51 --
 2 files changed, 1 insertion(+), 51 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 4654798523..e5c86e38c4 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -484,6 +484,7 @@ build-gprof-gcov:
   variables:
 IMAGE: ubuntu2004
 CONFIGURE_ARGS: --enable-gprof --enable-gcov
+--extra-cflags=-fsanitize=thread
 MAKE_CHECK_ARGS: build-tcg
 TARGETS: aarch64-softmmu mips64-softmmu ppc64-softmmu
  riscv64-softmmu s390x-softmmu x86_64-softmmu
diff --git a/.travis.yml b/.travis.yml
index b3fc72f561..18e62f282f 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -120,57 +120,6 @@ after_script:
 jobs:
   include:
 
-
-# Using newer GCC with sanitizers
-- name: "GCC9 with sanitizers (softmmu)"
-  dist: bionic
-  addons:
-apt:
-  update: true
-  sources:
-# PPAs for newer toolchains
-- ubuntu-toolchain-r-test
-  packages:
-# Extra toolchains
-- gcc-9
-- g++-9
-# Build dependencies
-- libaio-dev
-- libattr1-dev
-- libbrlapi-dev
-- libcap-ng-dev
-- libgnutls28-dev
-- libgtk-3-dev
-- libiscsi-dev
-- liblttng-ust-dev
-- libnfs-dev
-- libncurses5-dev
-- libnss3-dev
-- libpixman-1-dev
-- libpng-dev
-- librados-dev
-- libsdl2-dev
-- libsdl2-image-dev
-- libseccomp-dev
-- libspice-protocol-dev
-- libspice-server-dev
-- liburcu-dev
-- libusb-1.0-0-dev
-- libvte-2.91-dev
-- ninja-build
-- sparse
-- uuid-dev
-  language: generic
-  compiler: none
-  env:
-- COMPILER_NAME=gcc CXX=g++-9 CC=gcc-9
-- CONFIG="--cc=gcc-9 --cxx=g++-9 --disable-linux-user"
-- TEST_CMD=""
-  before_script:
-- mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
-- ${SRC_DIR}/configure ${CONFIG} --extra-cflags="-g3 -O0 
-fsanitize=thread" || { cat config.log meson-logs/meson-log.txt && exit 1; }
-
-
 - name: "[aarch64] GCC check-tcg"
   arch: arm64
   dist: focal
-- 
2.27.0

Re: [RFC PATCH 4/4] hw/intc: make gicv3_idreg() distinguish between gicv3/gicv4

[Leif Lindholm]

On Tue, Feb 02, 2021 at 10:31:16 +, Peter Maydell wrote:
> On Sun, 24 Jan 2021 at 02:53, Leif Lindholm  wrote:
> >
> > Make gicv3_idreg() able to return either gicv3 or gicv4 data.
> > Add a parameter to specify gic version.
> >
> > Signed-off-by: Leif Lindholm 
> > ---
> >  hw/intc/arm_gicv3_dist.c   |  2 +-
> >  hw/intc/arm_gicv3_redist.c |  2 +-
> >  hw/intc/gicv3_internal.h   | 12 ++--
> >  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> > -static inline uint32_t gicv3_idreg(int regoffset)
> > +static inline uint32_t gicv3_idreg(int regoffset, int revision)
> 
> I would prefer to pass in the GICv3State* and let the function
> look at s->revision.

Yeah, that'd be neater.

> >  {
> >  /* Return the value of the CoreSight ID register at the specified
> >   * offset from the first ID register (as found in the distributor
> > @@ -331,7 +331,15 @@ static inline uint32_t gicv3_idreg(int regoffset)
> >  static const uint8_t gicd_ids[] = {
> >  0x44, 0x00, 0x00, 0x00, 0x92, 0xB4, 0x3B, 0x00, 0x0D, 0xF0, 0x05, 
> > 0xB1
> >  };
> > -return gicd_ids[regoffset / 4];
> > +static const uint8_t gicdv4_ids[] = {
> > +0x44, 0x00, 0x00, 0x00, 0x92, 0xB4, 0x4B, 0x00, 0x0D, 0xF0, 0x05, 
> > 0xB1
> > +};
> > +
> > +if (revision == 3) {
> > +return gicd_ids[regoffset / 4];
> > +} else {
> > +return gicdv4_ids[regoffset / 4];
> > +}
> >  }
> 
> Updating the comment "These values indicate an ARM implementation of a GICv3"
> to add a note about what the new values are indicating would be nice.

Will do.

Regards,

Leif

[PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()

[Stefan Hajnoczi]

lo_do_lookup() finds an existing inode or allocates a new one. It
increments nlookup so that the inode stays alive until the client
releases it.

Existing callers don't need the struct lo_inode so the function doesn't
return it. Extend the function to optionally return the inode. The next
commit will need it.

Signed-off-by: Stefan Hajnoczi 
---
 tools/virtiofsd/passthrough_ll.c | 29 +
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index e63cbd3fb7..c87a1f3d72 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const 
char *pathname,
 }
 
 /*
- * Increments nlookup and caller must release refcount using
- * lo_inode_put(&parent).
+ * Increments nlookup on the inode on success. unref_inode_lolocked() must be
+ * called eventually to decrement nlookup again. If inodep is non-NULL, the
+ * inode pointer is stored and the caller must call lo_inode_put().
  */
 static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
-struct fuse_entry_param *e)
+struct fuse_entry_param *e,
+struct lo_inode **inodep)
 {
 int newfd;
 int res;
@@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, 
const char *name,
 struct lo_inode *inode = NULL;
 struct lo_inode *dir = lo_inode(req, parent);
 
+if (inodep) {
+*inodep = NULL;
+}
+
 /*
  * name_to_handle_at() and open_by_handle_at() can reach here with fuse
  * mount point in guest, but we don't have its inode info in the
@@ -913,7 +919,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, 
const char *name,
 pthread_mutex_unlock(&lo->mutex);
 }
 e->ino = inode->fuse_ino;
-lo_inode_put(lo, &inode);
+
+/* Transfer ownership of inode pointer to caller or drop it */
+if (inodep) {
+*inodep = inode;
+} else {
+lo_inode_put(lo, &inode);
+}
+
 lo_inode_put(lo, &dir);
 
 fuse_log(FUSE_LOG_DEBUG, "  %lli/%s -> %lli\n", (unsigned long long)parent,
@@ -948,7 +961,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, 
const char *name)
 return;
 }
 
-err = lo_do_lookup(req, parent, name, &e);
+err = lo_do_lookup(req, parent, name, &e, NULL);
 if (err) {
 fuse_reply_err(req, err);
 } else {
@@ -1056,7 +1069,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t 
parent,
 goto out;
 }
 
-saverr = lo_do_lookup(req, parent, name, &e);
+saverr = lo_do_lookup(req, parent, name, &e, NULL);
 if (saverr) {
 goto out;
 }
@@ -1534,7 +1547,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, 
size_t size,
 
 if (plus) {
 if (!is_dot_or_dotdot(name)) {
-err = lo_do_lookup(req, ino, name, &e);
+err = lo_do_lookup(req, ino, name, &e, NULL);
 if (err) {
 goto error;
 }
@@ -1732,7 +1745,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, 
const char *name,
 }
 
 fi->fh = fh;
-err = lo_do_lookup(req, parent, name, &e);
+err = lo_do_lookup(req, parent, name, &e, NULL);
 }
 if (lo->cache == CACHE_NONE) {
 fi->direct_io = 1;
-- 
2.29.2

[PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)

[Stefan Hajnoczi]

A well-behaved FUSE client does not attempt to open special files with
FUSE_OPEN because they are handled on the client side (e.g. device nodes
are handled by client-side device drivers).

The check to prevent virtiofsd from opening special files is missing in
a few cases, most notably FUSE_OPEN. A malicious client can cause
virtiofsd to open a device node, potentially allowing the guest to
escape. This can be exploited by a modified guest device driver. It is
not exploitable from guest userspace since the guest kernel will handle
special files inside the guest instead of sending FUSE requests.

This patch fixes this issue by introducing the lo_inode_open() function
to check the file type before opening it. This is a short-term solution
because it does not prevent a compromised virtiofsd process from opening
device nodes on the host.

Restructure lo_create() to try O_CREAT | O_EXCL first. Note that O_CREAT
| O_EXCL does not follow symlinks, so O_NOFOLLOW masking is not
necessary here. If the file exists and the user did not specify O_EXCL,
open it via lo_do_open().

Reported-by: Alex Xu 
Fixes: CVE-2020-35517
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Vivek Goyal 
Signed-off-by: Stefan Hajnoczi 
---
v3:
 * Restructure lo_create() to handle externally-created files (we need
   to allocate an inode for them) [Greg]
v3:
 * Protect lo_create() [Greg]
v2:
 * Add doc comment clarifying that symlinks are traversed client-side
   [Daniel]

This issue was diagnosed on public IRC and is therefore already known
and not embargoed.

A stronger fix, and the long-term solution, is for users to mount the
shared directory and any sub-mounts with nodev, as well as nosuid and
noexec. Unfortunately virtiofsd cannot do this automatically because
bind mounts added by the user after virtiofsd has launched would not be
detected. I suggest the following:

1. Modify libvirt and Kata Containers to explicitly set these mount
   options.
2. Then modify virtiofsd to check that the shared directory has the
   necessary options at startup. Refuse to start if the options are
   missing so that the user is aware of the security requirements.

As a bonus this also increases the likelihood that other host processes
besides virtiofsd will be protected by nosuid/noexec/nodev so that a
malicious guest cannot drop these files in place and then arrange for a
host process to come across them.

Additionally, user namespaces have been discussed. They seem like a
worthwhile addition as an unprivileged or privilege-separated mode
although there are limitations with respect to security xattrs and the
actual uid/gid stored on the host file system not corresponding to the
guest uid/gid.

Signed-off-by: Stefan Hajnoczi 
---
 tools/virtiofsd/passthrough_ll.c | 139 +++
 1 file changed, 88 insertions(+), 51 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index c87a1f3d72..b607ef0f7e 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -555,6 +555,38 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino)
 return fd;
 }
 
+/*
+ * Open a file descriptor for an inode. Returns -EBADF if the inode is not a
+ * regular file or a directory.
+ *
+ * Use this helper function instead of raw openat(2) to prevent security issues
+ * when a malicious client opens special files such as block device nodes.
+ * Symlink inodes are also rejected since symlinks must already have been
+ * traversed on the client side.
+ */
+static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
+ int open_flags)
+{
+g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
+int fd;
+
+if (!S_ISREG(inode->filetype) && !S_ISDIR(inode->filetype)) {
+return -EBADF;
+}
+
+/*
+ * The file is a symlink so O_NOFOLLOW must be ignored. We checked earlier
+ * that the inode is not a special file but if an external process races
+ * with us then symlinks are traversed here. It is not possible to escape
+ * the shared directory since it is mounted as "/" though.
+ */
+fd = openat(lo->proc_self_fd, fd_str, open_flags & ~O_NOFOLLOW);
+if (fd < 0) {
+return -errno;
+}
+return fd;
+}
+
 static void lo_init(void *userdata, struct fuse_conn_info *conn)
 {
 struct lo_data *lo = (struct lo_data *)userdata;
@@ -684,8 +716,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, 
struct stat *attr,
 if (fi) {
 truncfd = fd;
 } else {
-sprintf(procname, "%i", ifd);
-truncfd = openat(lo->proc_self_fd, procname, O_RDWR);
+truncfd = lo_inode_open(lo, inode, O_RDWR);
 if (truncfd < 0) {
 goto out_err;
 }
@@ -1664,19 +1695,24 @@ static void update_open_flags(int writeback, int 
allow_direct_io,
 }
 }
 
+/*
+ * Open a regular file, set up an fd mapping, and fill out the struct
+ * fuse_

Re: [PATCH 20/20] RFC: tests: add some virtio-gpu & vhost-user-gpu acceptance test

[Gerd Hoffmann]

On Tue, Feb 02, 2021 at 06:26:25PM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> This will check virtio/vhost-user-vga & virgl are correctly initialized
> by the Linux kernel on an egl-headless display.
> 
> There are many other things that could be checked, but that's a start. I
> also don't know yet how to nicely skip on incompatible host &
> configurations.

You can annotate tests like this:

   @avocado.skipUnless(os.path.exists(pick_default_vug_bin()), "no 
vhost-user-gpu")
> +def test_vhost_user_vga_virgl(self):

[ queued whole series + some other pending ui/vga bits, kicked CI, lets
  see how it is going.  I suspect I'll have to drop this patch though. ]

HTH,
  Gerd

Re: [PATCH v4 0/3] virtiofsd: prevent opening of special files (CVE-2020-35517)

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20210203113719.83633-1-stefa...@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210203113719.83633-1-stefa...@redhat.com
Subject: [PATCH v4 0/3] virtiofsd: prevent opening of special files 
(CVE-2020-35517)

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
9e44f0e virtiofsd: prevent opening of special files (CVE-2020-35517)
e3ddfae virtiofsd: optionally return inode pointer from lo_do_lookup()
a6c73fd virtiofsd: extract lo_do_open() from lo_open()

=== OUTPUT BEGIN ===
1/3 Checking commit a6c73fd0a630 (virtiofsd: extract lo_do_open() from 
lo_open())
ERROR: return of an errno should typically be -ve (return -ENOMEM)
#70: FILE: tools/virtiofsd/passthrough_ll.c:1674:
+return ENOMEM;

total: 1 errors, 0 warnings, 114 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/3 Checking commit e3ddfaebb90a (virtiofsd: optionally return inode pointer 
from lo_do_lookup())
3/3 Checking commit 9e44f0e0be3a (virtiofsd: prevent opening of special files 
(CVE-2020-35517))
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [RFC PATCH 0/4] hw/intc: enable GICv4 memory layout for GICv3 driver

[Leif Lindholm]

On Tue, Feb 02, 2021 at 10:39:22 +, Peter Maydell wrote:
> On Sun, 24 Jan 2021 at 02:53, Leif Lindholm  wrote:
> >
> > GICv4 sets aside 256K per redistributor configuration block, whereas GICv3
> > only uses 128K. However, some codebases (like TF-A, EDK2) will happily use
> > the GICv3 functionality only.
> >
> > This set aims at enabling these codebases to run, without actually enabling
> > full support for GICv4.
> >
> > This creates a ... problematic ... system, which will misbehave if you try
> > to use the virtual LPIs. But it does help with letting me use QEMU for
> > modelling a platform containing a GICv4, and share firmware images with
> > other prototyping platforms.
> 
> So, what's your aim for this series? I think we could reasonably
> take patches 2 and 4 upstream (they are changes we'll want for eventual
> v4 emulation support), but I don't really want 1 and 3.
> That would reduce the delta you're carrying locally, at least.

That would be much appreciated.

In a way, I wanted to test the waters a bit with regards to whether
gicv4 emulation could be introduced gradually, and whether doing so by
extending the existing gicv3 driver was the way to go.

Anyway, for now, I'll address your comments and send out a 2-part v2,
containing 2,4/4.

Best Regards,

Leif

> I suppose we should look at what changes QEMU needs for KVM in-kernel GICv4
> support at some point...
> 
> thanks
> -- PMM

Re: [PATCH v15 04/23] cpu: Move synchronize_from_tb() to tcg_ops

[Claudio Fontana]

On 2/3/21 11:11 AM, Alex Bennée wrote:
> 
> Claudio Fontana  writes:
> 
>> From: Eduardo Habkost 
>>
>> Signed-off-by: Eduardo Habkost 
>>
>> [claudio: wrapped target code in CONFIG_TCG]
>> Signed-off-by: Claudio Fontana 
>> ---
>>  include/hw/core/cpu.h | 20 +++-
>>  accel/tcg/cpu-exec.c  |  4 ++--
>>  target/arm/cpu.c  |  4 +++-
>>  target/avr/cpu.c  |  2 +-
>>  target/hppa/cpu.c |  2 +-
>>  target/i386/tcg/tcg-cpu.c |  2 +-
>>  target/microblaze/cpu.c   |  2 +-
>>  target/mips/cpu.c |  4 +++-
>>  target/riscv/cpu.c|  2 +-
>>  target/rx/cpu.c   |  2 +-
>>  target/sh4/cpu.c  |  2 +-
>>  target/sparc/cpu.c|  2 +-
>>  target/tricore/cpu.c  |  2 +-
>>  13 files changed, 28 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index d0b17dcc4c..b9803885e5 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -86,6 +86,17 @@ typedef struct TcgCpuOperations {
>>   * Called when the first CPU is realized.
>>   */
>>  void (*initialize)(void);
>> +/**
>> + * @synchronize_from_tb: Synchronize state from a TCG #TranslationBlock
>> + *
>> + * This is called when we abandon execution of a TB before
>> + * starting it, and must set all parts of the CPU state which
>> + * the previous TB in the chain may not have updated. This
>> + * will need to do more. If this hook is not implemented then
>> + * the default is to call @set_pc(tb->pc).
>> + */
>> +void (*synchronize_from_tb)(CPUState *cpu,
>> +const struct TranslationBlock *tb);
> 
> Did you miss my comment last time or just not think it flowed better?
> 
>   ...TB in the chain may not have updated. By default when no hook is
>   defined a call is made to @set_pc(tb->pc). If more state needs to be
>   restored the front-end must provide a hook function and restore all the
>   state there.
> 
> Either way:
> 
> Reviewed-by: Alex Bennée 
> 

Hi Alex, sorry for missing this change, can add it for the next respin,

and thanks for looking at this,

Ciao,

Claudio

[Bug 1912224] Re: qemu may freeze during drive-mirroring on fragmented FS

[Max Reitz]

By the way, as a side note: I see you’re using raw, but qcow2 tries to
avoid deferring the block-status call to file-posix (i.e., it tries to
avoid SEEK_HOLE/DATA calls), because in most cases it knows from its own
metadata which block are zero. So I would guess that with qcow2, the
problem would not occur. Do you have any data on that?

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

Title:
  qemu may freeze during drive-mirroring on fragmented FS

Status in QEMU:
  New

Bug description:
  
  We have odd behavior in operation where qemu freeze during long
  seconds, We started an thread about that issue here:
  https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg05623.html

  It happens at least during openstack nova snapshot (qemu blockdev-mirror)
  or live block migration(which include network copy of disk).

  After further troubleshoots, it seems related to FS fragmentation on
  host.

  reproducible at least on:
  Ubuntu 18.04.3/4.18.0-25-generic/qemu-4.0
  Ubuntu 16.04.6/5.10.6/qemu-5.2.0-rc2

  # Lets create a dedicated file system on a SSD/Nvme 60GB disk in my case:
  $sudo mkfs.ext4 /dev/sda3
  $sudo mount /dev/sda3 /mnt
  $df -h /mnt
  Filesystem  Size  Used Avail Use% Mounted on
  /dev/sda3 59G   53M   56G   1% /mnt

  #Create a fragmented disk on it using 2MB Chunks (about 30min):
  $sudo python3 create_fragged_disk.py /mnt 2
  Filling up FS by Creating chunks files in:  /mnt/chunks
  We are probably full as expected!!:  [Errno 28] No space left on device
  Creating fragged disk file:  /mnt/disk

  $ls -lhs 
  59G -rw-r--r-- 1 root root 59G Jan 15 14:08 /mnt/disk

  $ sudo e4defrag -c /mnt/disk
   Total/best extents 41971/30
   Average size per extent1466 KB
   Fragmentation score2
   [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag]
   This file (/mnt/disk) does not need defragmentation.
   Done.

  # the tool^^^ says it is not enough fragmented to be able to defrag.

  #Inject an image on fragmented disk
  sudo chown ubuntu /mnt/disk
  wget 
https://cloud-images.ubuntu.com/bionic/current/bionic-server-cloudimg-amd64.img
  qemu-img convert -O raw  bionic-server-cloudimg-amd64.img \
   bionic-server-cloudimg-amd64.img.raw
  dd conv=notrunc iflag=fullblock if=bionic-server-cloudimg-amd64.img.raw \
  of=/mnt/disk bs=1M
  virt-customize -a /mnt/disk --root-password password:

  # logon run console activity ex: ping -i 0.3 127.0.0.1
  $qemu-system-x86_64 -m 2G -enable-kvm  -nographic \
  -chardev socket,id=test,path=/tmp/qmp-monitor,server,nowait \
  -mon chardev=test,mode=control \
  -drive 
file=/mnt/disk,format=raw,if=none,id=drive-virtio-disk0,cache=none,discard\
  -device 
virtio-blk-pci,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on

  $sync
  $echo 3 | sudo tee -a /proc/sys/vm/drop_caches

  #start drive-mirror via qmp on another SSD/nvme partition
  nc -U /tmp/qmp-monitor
  {"execute":"qmp_capabilities"}
  
{"execute":"drive-mirror","arguments":{"device":"drive-virtio-disk0","target":"/home/ubuntu/mirror","sync":"full","format":"qcow2"}}
  ^^^ qemu console may start to freeze at this step.

  NOTE:
   - smaller chunk sz and bigger disk size the worst it is.
 In operation we also have issue on 400GB disk size with average 13MB/extent
   - Reproducible also on xfs

  
  Expected behavior:
  ---
  QEMU should remain steady, eventually only have decrease storage Performance
  or mirroring, because of fragmented fs.

  Observed behavior:
  ---
  Perf of mirroring is still quite good even on fragmented FS,
  but it breaks qemu.

  
  ##  create_fragged_disk.py 
  import sys
  import os
  import tempfile
  import glob
  import errno

  MNT_DIR = sys.argv[1]
  CHUNK_SZ_MB = int(sys.argv[2])
  CHUNKS_DIR = MNT_DIR + '/chunks'
  DISK_FILE = MNT_DIR + '/disk'

  if not os.path.exists(CHUNKS_DIR):
  os.makedirs(CHUNKS_DIR)

  with open("/dev/urandom", "rb") as f_rand:
   mb_rand=f_rand.read(1024 * 1024)

  print("Filling up FS by Creating chunks files in: ",CHUNKS_DIR)
  try:
  while True:
  tp = tempfile.NamedTemporaryFile(dir=CHUNKS_DIR,delete=False)
  for x in range(CHUNK_SZ_MB):
  tp.write(mb_rand)
  os.fsync(tp)
  tp.close()
  except Exception as ex:
  print("We are probably full as expected!!: ",ex)

  chunks = glob.glob(CHUNKS_DIR + '/*')

  print("Creating fragged disk file: ",DISK_FILE)
  with open(DISK_FILE, "w+b") as f_disk:
  for chunk in chunks:
  try:
  os.unlink(chunk)
  for x in range(CHUNK_SZ_MB):
  f_disk.write(mb_rand)
  os.fsync(f_disk)
  except IOError as ex:
  if ex.er

[Bug 1912777] Re: KVM_EXIT_MMIO has increased in Qemu4.0.0 when compared to Qemu 2.11.0

[ANIMESH KUMAR SINHA]

** Changed in: qemu
   Status: New => Opinion

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

Title:
  KVM_EXIT_MMIO has increased in Qemu4.0.0 when compared to Qemu 2.11.0

Status in QEMU:
  Opinion
Status in Ubuntu:
  New

Bug description:
  I was able to generate trace dump in Qemu for kvm_run_exit event in both QEMU 
2.11.0 and QEMU 4.0.0
  From the trace i noticed that the number of KVM_KXIT_MMIO calls has increased 
alot and is causing delay in testcase execution.

  I executed same testcase from Qemu 2.11 and Qemu4.
  Inside Virtual machine when using qemu 2.11 testcase got completed in 11 
seconds
  but the same testcase when executed on Qemu 4.0.0 got executed in 26 seconds.

  
  I did a bit of digging and extracted the kvm_run_exit to figure out whats 
going on.

  Please find 
  Stats from Qemu2.11:

  KVM_EXIT_UNKNOWN  : 0
  KVM_EXIT_EXCEPTION: 0
  KVM_EXIT_IO   : 182513
  KVM_EXIT_HYPERCALL: 0
  KVM_EXIT_DEBUG: 0
  KVM_EXIT_HLT  : 0
  KVM_EXIT_MMIO : 216701
  KVM_EXIT_IRQ_WINDOW_OPEN  : 0
  KVM_EXIT_SHUTDOWN : 0
  KVM_EXIT_FAIL_ENTRY   : 0
  KVM_EXIT_INTR : 0
  KVM_EXIT_SET_TPR  : 0
  KVM_EXIT_TPR_ACCESS   : 0
  KVM_EXIT_S390_SIEIC   : 0
  KVM_EXIT_S390_RESET   : 0
  KVM_EXIT_DCR  : 0
  KVM_EXIT_NMI  : 0
  KVM_EXIT_INTERNAL_ERROR   : 0
  KVM_EXIT_OSI  : 0
  KVM_EXIT_PAPR_HCALL   : 0
  KVM_EXIT_S390_UCONTROL: 0
  KVM_EXIT_WATCHDOG : 0
  KVM_EXIT_S390_TSCH: 0
  KVM_EXIT_EPR  : 0
  KVM_EXIT_SYSTEM_EVENT : 0
  KVM_EXIT_S390_STSI: 0
  KVM_EXIT_IOAPIC_EOI   : 0
  KVM_EXIT_HYPERV   : 0

  KVM_RUN_EXIT  : 399214  (Total in Qemu 2.11 for a
  testcase)

  
  Stats For Qemu 4.0.0:

  VM_EXIT_UNKNOWN   : 0 
   
  KVM_EXIT_EXCEPTION: 0 
   
  KVM_EXIT_IO   : 163729
   
  KVM_EXIT_HYPERCALL: 0 
   
  KVM_EXIT_DEBUG: 0 
   
  KVM_EXIT_HLT  : 0 
   
  KVM_EXIT_MMIO : 1094231   
   
  KVM_EXIT_IRQ_WINDOW_OPEN  : 46
   
  KVM_EXIT_SHUTDOWN : 0 
   
  KVM_EXIT_FAIL_ENTRY   : 0 
   
  KVM_EXIT_INTR : 0 
   
  KVM_EXIT_SET_TPR  : 0 
   
  KVM_EXIT_TPR_ACCESS   : 0 
   
  KVM_EXIT_S390_SIEIC   : 0 
   
  KVM_EXIT_S390_RESET   : 0 
   
  KVM_EXIT_DCR  : 0 
   
  KVM_EXIT_NMI  : 0 
   
  KVM_EXIT_INTERNAL_ERROR   : 0 
   
  KVM_EXIT_OSI  : 0 
  

Re: ARM Snapshots Not Backwards-Compatible

[Andrew Jones]

On Wed, Feb 03, 2021 at 10:52:59AM +, Peter Maydell wrote:
> On Wed, 3 Feb 2021 at 10:49, Dr. David Alan Gilbert  
> wrote:
> >
> > * Peter Maydell (peter.mayd...@linaro.org) wrote:
> > > On Wed, 3 Feb 2021 at 10:28, Dr. David Alan Gilbert  
> > > wrote:
> > > >
> > > > * Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> > > > > Cc'ing migration team and qemu-arm@ list.
> > > >
> > > > I'll have to leave the detail of that to the ARM peole; but from a
> > > > migration point of view I think we do want the 64 bit ARM migrations to
> > > > be stable now.  Please tie incompatible changes to machine types.
> > >
> > > That is the intention, but because there's no upstream testing
> > > of migration compat, we never notice if we get it wrong.
> > > What is x86 doing to keep cross-version migration working ?
> >
> > I know there used to be some of our team running Avocado tests for
> > compatibility regularly, I'm not sure of the current status.
> > It's something we also do regularly around when we do downstream
> > releases, so we tend to catch them then, although even on x86 that
> > often turns out to be a bit late.
> 
> So downstream testing only?

Not even downstream for the Arm architecture, at least not at Red Hat. The
support we have for Arm Virt is still limited to the use cases for which
it has been deployed. To this day that hasn't included migration[*].

> I think that unless we either (a) start
> doing migration-compat testing consistently upstream or

This is the best choice and it can certainly be an additional approach
regardless of what goes on downstream. I can look into this. A pointer
to the x86 tests would be a good start. It's pretty simple to do a
quick migration test between two versions of QEMU, but we need the
whole build two versions of QEMU stuff first, which I hope already
exists.

> (b) RedHat or
> some other downstream start testing and reporting compat issues
> to us for aarch64 as they do for x86-64,

Red Hat isn't currently allocating resources to this type of testing
for AArch64. And, even if it were to start, it would likely focus on
compat testing of RHEL arm-virt machine types, which aren't exactly the
same thing as the upstream arm-virt machine types. In fact, currently,
there are only two (virt-rhel8.2.0 and virt-rhel8.3.0).

Thanks,
drew

[*] Migration support hasn't been a real high priority for AArch64 KVM,
because it currently requires identical host CPU types and identical
host kernels to work. It'd be nice if the destination host kernel
could at least be a later version, but not even that is guaranteed
to work at this time.

Re: [PATCH 03/12] qga: Replace '--blacklist' command line option by '--denylist'

[BALATON Zoltan]

On Wed, 3 Feb 2021, Daniel P. Berrangé wrote:

On Tue, Feb 02, 2021 at 09:58:15PM +0100, Philippe Mathieu-Daudé wrote:

Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.

Keep the --blacklist available for backward compatibility.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Signed-off-by: Philippe Mathieu-Daudé 
---
 docs/interop/qemu-ga.rst | 2 +-
 qga/main.c   | 6 --
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/docs/interop/qemu-ga.rst b/docs/interop/qemu-ga.rst
index 9a590bf95cb..89596e646de 100644
--- a/docs/interop/qemu-ga.rst
+++ b/docs/interop/qemu-ga.rst
@@ -79,7 +79,7 @@ Options

   Daemonize after startup (detach from terminal).

-.. option:: -b, --blacklist=LIST
+.. option:: -b, --denylist=LIST

   Comma-separated list of RPCs to disable (no spaces, ``?`` to list
   available RPCs).
diff --git a/qga/main.c b/qga/main.c
index 249fe06e8e5..66177b9e93d 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -257,7 +257,8 @@ QEMU_COPYRIGHT "\n"
 #ifdef _WIN32
 "  -s, --service service commands: install, uninstall, vss-install, 
vss-uninstall\n"
 #endif
-"  -b, --blacklist   comma-separated list of RPCs to disable (no spaces, 
\"?\"\n"
+"  --blacklist   backward compatible alias for --denylist (deprecated)\n"
+"  -b, --denylistcomma-separated list of RPCs to disable (no spaces, 
\"?\"\n"



"-b" is a bit odd as a short name now, but i guess that's not the end
of the world.


Maybe -b, --block  or --block-rpc? Not the best but at least preserves 
consistency with the short option.


Regards,
BALATON Zoltan


The deprecation should be documented though. Ideally we would report
a warning if the deprecated long arg was used too.


 "to list available RPCs)\n"
 "  -D, --dump-conf   dump a qemu-ga config file based on current config\n"
 "options / command-line parameters to stdout\n"
@@ -,7 +1112,8 @@ static void config_parse(GAConfig *config, int argc, char 
**argv)
 { "method", 1, NULL, 'm' },
 { "path", 1, NULL, 'p' },
 { "daemonize", 0, NULL, 'd' },
-{ "blacklist", 1, NULL, 'b' },
+{ "denylist", 1, NULL, 'b' },
+{ "blacklist", 1, NULL, 'b' }, /* deprecated alias for 'denylist' */
 #ifdef _WIN32
 { "service", 1, NULL, 's' },
 #endif
--
2.26.2




Regards,
Daniel

Re: [PULL 0/3] Machine queue, 2021-02-02

[Peter Maydell]

On Tue, 2 Feb 2021 at 19:31, Eduardo Habkost  wrote:
>
> I still have a huge list of patches to review, but I don't keep
> this series stuck in the queue while I try to catch up.
>
> The following changes since commit 74208cd252c5da9d867270a178799abd802b9338:
>
>   Merge remote-tracking branch 
> 'remotes/berrange-gitlab/tags/misc-fixes-pull-request' into staging 
> (2021-01-29 19:51:25 +)
>
> are available in the Git repository at:
>
>   https://gitlab.com/ehabkost/qemu.git tags/machine-next-pull-request
>
> for you to fetch changes up to dbd730e8598701e11b2fb7aee1704f4ec1787e86:
>
>   nvdimm: check -object memory-backend-file, readonly=on option (2021-02-01 
> 17:07:34 -0500)
>
> 
> Machine queue, 2021-02-02
>
> Feature:
> * nvdimm: read-only file support (Stefan Hajnoczi)


Applied, thanks.

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

-- PMM

[PATCH 1/2] migration: dirty-bitmap: Convert alias map inner members to a struct

[Peter Krempa]

Currently the alias mapping hash stores just strings of the target
objects internally. In further patches we'll be adding another member
which will need to be stored in the map so convert the members to a
struct.

Signed-off-by: Peter Krempa 
---
 migration/block-dirty-bitmap.c | 37 --
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index c61d382be8..b0403dd00c 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -169,6 +169,18 @@ typedef struct DBMState {

 static DBMState dbm_state;

+typedef struct AliasMapInnerBitmap {
+char *string;
+} AliasMapInnerBitmap;
+
+static void free_alias_map_inner_bitmap(void *amin_ptr)
+{
+AliasMapInnerBitmap *amin = amin_ptr;
+
+g_free(amin->string);
+g_free(amin);
+}
+
 /* For hash tables that map node/bitmap names to aliases */
 typedef struct AliasMapInnerNode {
 char *string;
@@ -264,7 +276,7 @@ static GHashTable *construct_alias_map(const 
BitmapMigrationNodeAliasList *bbm,
 }

 bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
-g_free, g_free);
+g_free, 
free_alias_map_inner_bitmap);

 amin = g_new(AliasMapInnerNode, 1);
 *amin = (AliasMapInnerNode){
@@ -277,6 +289,7 @@ static GHashTable *construct_alias_map(const 
BitmapMigrationNodeAliasList *bbm,
 for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
 const BitmapMigrationBitmapAlias *bmba = bmbal->value;
 const char *bmap_map_from, *bmap_map_to;
+AliasMapInnerBitmap *bmap_inner;

 if (strlen(bmba->alias) > UINT8_MAX) {
 error_setg(errp,
@@ -311,8 +324,11 @@ static GHashTable *construct_alias_map(const 
BitmapMigrationNodeAliasList *bbm,
 }
 }

+bmap_inner = g_new0(AliasMapInnerBitmap, 1);
+bmap_inner->string = g_strdup(bmap_map_to);
+
 g_hash_table_insert(bitmaps_map,
-g_strdup(bmap_map_from), 
g_strdup(bmap_map_to));
+g_strdup(bmap_map_from), bmap_inner);
 }
 }

@@ -538,11 +554,16 @@ static int add_bitmaps_to_list(DBMSaveState *s, 
BlockDriverState *bs,
 }

 if (bitmap_aliases) {
-bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
-if (!bitmap_alias) {
+AliasMapInnerBitmap *bmap_inner;
+
+bmap_inner = g_hash_table_lookup(bitmap_aliases, bitmap_name);
+
+if (!bmap_inner) {
 /* Skip bitmaps with no alias */
 continue;
 }
+
+bitmap_alias = bmap_inner->string;
 } else {
 if (strlen(bitmap_name) > UINT8_MAX) {
 error_report("Cannot migrate bitmap '%s' on node '%s': "
@@ -1074,14 +1095,18 @@ static int dirty_bitmap_load_header(QEMUFile *f, 
DBMLoadState *s,

 bitmap_name = s->bitmap_alias;
 if (!s->cancelled && bitmap_alias_map) {
-bitmap_name = g_hash_table_lookup(bitmap_alias_map,
+AliasMapInnerBitmap *bmap_inner;
+
+bmap_inner = g_hash_table_lookup(bitmap_alias_map,
   s->bitmap_alias);
-if (!bitmap_name) {
+if (!bmap_inner) {
 error_report("Error: Unknown bitmap alias '%s' on node "
  "'%s' (alias '%s')", s->bitmap_alias,
  s->bs->node_name, s->node_alias);
 cancel_incoming_locked(s);
 }
+
+bitmap_name = bmap_inner->string;
 }

 if (!s->cancelled) {
-- 
2.29.2

[PATCH 0/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Peter Krempa]

See 2/2 for explanation.

Please let me know if I need to add any tests for this.

Peter Krempa (2):
  migration: dirty-bitmap: Convert alias map inner members to a struct
  migration: dirty-bitmap: Allow control of bitmap persistence on
destination

 migration/block-dirty-bitmap.c | 67 ++
 qapi/migration.json|  7 +++-
 2 files changed, 66 insertions(+), 8 deletions(-)

-- 
2.29.2

[PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Peter Krempa]

Bitmap's source persistence is transported over the migration stream and
the destination mirrors it. In some cases the destination might want to
persist bitmaps which are not persistent on the source (e.g. the result
of merge of bitmaps from a number of layers on the source when migrating
into a squashed image) but currently it would need to create another set
of persistent bitmaps and merge them.

This adds 'dest-persistent' optional property to
'BitmapMigrationBitmapAlias' which when present overrides the bitmap
presence state from the source.

Signed-off-by: Peter Krempa 
---
 migration/block-dirty-bitmap.c | 30 +-
 qapi/migration.json|  7 ++-
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index b0403dd00c..1876c94c45 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -149,6 +149,9 @@ typedef struct DBMLoadState {

 bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */

+bool has_dest_persistent;
+bool dest_persistent;
+
 /*
  * cancelled
  * Incoming migration is cancelled for some reason. That means that we
@@ -171,6 +174,10 @@ static DBMState dbm_state;

 typedef struct AliasMapInnerBitmap {
 char *string;
+
+/* for destination's properties setting bitmap state */
+bool has_dest_persistent;
+bool dest_persistent;
 } AliasMapInnerBitmap;

 static void free_alias_map_inner_bitmap(void *amin_ptr)
@@ -289,6 +296,8 @@ static GHashTable *construct_alias_map(const 
BitmapMigrationNodeAliasList *bbm,
 for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
 const BitmapMigrationBitmapAlias *bmba = bmbal->value;
 const char *bmap_map_from, *bmap_map_to;
+bool bmap_has_dest_persistent = false;
+bool bmap_dest_persistent = false;
 AliasMapInnerBitmap *bmap_inner;

 if (strlen(bmba->alias) > UINT8_MAX) {
@@ -317,6 +326,9 @@ static GHashTable *construct_alias_map(const 
BitmapMigrationNodeAliasList *bbm,
 bmap_map_from = bmba->alias;
 bmap_map_to = bmba->name;

+bmap_has_dest_persistent = bmba->has_dest_persistent;
+bmap_dest_persistent = bmba->dest_persistent;
+
 if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
 error_setg(errp, "The bitmap alias '%s'/'%s' is used 
twice",
bmna->alias, bmba->alias);
@@ -326,6 +338,8 @@ static GHashTable *construct_alias_map(const 
BitmapMigrationNodeAliasList *bbm,

 bmap_inner = g_new0(AliasMapInnerBitmap, 1);
 bmap_inner->string = g_strdup(bmap_map_to);
+bmap_inner->has_dest_persistent = bmap_has_dest_persistent;
+bmap_inner->dest_persistent = bmap_dest_persistent;

 g_hash_table_insert(bitmaps_map,
 g_strdup(bmap_map_from), bmap_inner);
@@ -798,6 +812,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, 
DBMLoadState *s)
 uint32_t granularity = qemu_get_be32(f);
 uint8_t flags = qemu_get_byte(f);
 LoadBitmapState *b;
+bool persistent;

 if (s->cancelled) {
 return 0;
@@ -822,7 +837,13 @@ static int dirty_bitmap_load_start(QEMUFile *f, 
DBMLoadState *s)
 return -EINVAL;
 }

-if (flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT) {
+if (s->has_dest_persistent) {
+persistent = s->dest_persistent;
+} else {
+persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+}
+
+if (persistent) {
 bdrv_dirty_bitmap_set_persistence(s->bitmap, true);
 }

@@ -1087,6 +1108,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, 
DBMLoadState *s,

 if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
 const char *bitmap_name;
+bool bitmap_has_dest_persistent = false;
+bool bitmap_dest_persistent = false;

 if (!qemu_get_counted_string(f, s->bitmap_alias)) {
 error_report("Unable to read bitmap alias string");
@@ -1107,12 +1130,17 @@ static int dirty_bitmap_load_header(QEMUFile *f, 
DBMLoadState *s,
 }

 bitmap_name = bmap_inner->string;
+bitmap_has_dest_persistent = bmap_inner->has_dest_persistent;
+bitmap_dest_persistent = bmap_inner->dest_persistent;
 }

 if (!s->cancelled) {
 g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
 s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);

+s->has_dest_persistent = bitmap_has_dest_persistent;
+s->dest_persistent = bitmap_dest_persistent;
+
 /*
  * bitmap may be NULL here, it wouldn't be an error if it is the
  * first occurrence of the bitmap
diff --git a/qapi/migration.json b/qapi/migration.json
index d1d9632c2a..32e64dbce6 100644

Re: [PATCH 1/4] block/nbd: fix drain dead-lock because of nbd reconnect-delay

[Vladimir Sementsov-Ogievskiy]

03.02.2021 13:53, Roman Kagan wrote:

On Thu, Sep 03, 2020 at 10:02:58PM +0300, Vladimir Sementsov-Ogievskiy wrote:

We pause reconnect process during drained section. So, if we have some
requests, waiting for reconnect we should cancel them, otherwise they
deadlock the drained section.

How to reproduce:

1. Create an image:
qemu-img create -f qcow2 xx 100M

2. Start NBD server:
qemu-nbd xx

3. Start vm with second nbd disk on node2, like this:

   ./build/x86_64-softmmu/qemu-system-x86_64 -nodefaults -drive \
  file=/work/images/cent7.qcow2 -drive \
  
driver=nbd,server.type=inet,server.host=192.168.100.5,server.port=10809,reconnect-delay=60
 \
  -vnc :0 -m 2G -enable-kvm -vga std

4. Access the vm through vnc (or some other way?), and check that NBD
drive works:

dd if=/dev/sdb of=/dev/null bs=1M count=10

- the command should succeed.

5. Now, kill the nbd server, and run dd in the guest again:

dd if=/dev/sdb of=/dev/null bs=1M count=10

Now Qemu is trying to reconnect, and dd-generated requests are waiting
for the connection (they will wait up to 60 seconds (see
reconnect-delay option above) and than fail). But suddenly, vm may
totally hang in the deadlock. You may need to increase reconnect-delay
period to catch the dead-lock.

VM doesn't respond because drain dead-lock happens in cpu thread with
global mutex taken. That's not good thing by itself and is not fixed
by this commit (true way is using iothreads). Still this commit fixes
drain dead-lock itself.

Note: probably, we can instead continue to reconnect during drained
section. To achieve this, we may move negotiation to the connect thread
to make it independent of bs aio context. But expanding drained section
doesn't seem good anyway. So, let's now fix the bug the simplest way.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/nbd.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index 9daf003bea..912ea27be7 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -242,6 +242,11 @@ static void coroutine_fn 
nbd_client_co_drain_begin(BlockDriverState *bs)
  }
  
  nbd_co_establish_connection_cancel(bs, false);

+
+if (s->state == NBD_CLIENT_CONNECTING_WAIT) {
+s->state = NBD_CLIENT_CONNECTING_NOWAIT;
+qemu_co_queue_restart_all(&s->free_sema);
+}
  }
  
  static void coroutine_fn nbd_client_co_drain_end(BlockDriverState *bs)


This basically defeats the whole purpose of reconnect: if the nbd client
is trying to reconnect, drain effectively cancels that and makes all
in-flight requests to complete with an error.

I'm not suggesting to revert this patch (it's now in the tree as commit
8c517de24a), because the deadlock is no better, but I'm afraid the only
real fix is to implement reconnect during the drain section.  I'm still
trying to get my head around it so no patch yet, but I just wanted to
bring this up in case anybody beats me to it.




What do you mean by "reconnect during drained section"? Trying to establish the 
connection? Or keeping in-flight requests instead of cancelling them? We can't keep 
in-flight requests during drained section, as it's the whole sense of drained-section: no 
in-flight requests. So we'll have to wait for them at drain_begin (waiting up to 
reconnect-delay, which doesn't seem good and triggers dead-lock for non-iothread 
environment) or just cancel them..

Do you argue that waiting on drained-begin is somehow better than cancelling?

Or, the problem is that after drained section (assume it was short) we are in 
NBD_CLIENT_CONNECTING_NOWAIT ?  Fixing it should be simple enough, we just need 
to check the time at drained_end and if the delay is not expired enable 
NBD_CLIENT_CONNECTING_WAIT agaub,,


--
Best regards,
Vladimir

Re: [PATCH v15 14/23] cpu: move debug_check_watchpoint to tcg_ops

[Alex Bennée]

Claudio Fontana  writes:

> commit 568496c0c0f1 ("cpu: Add callback to check architectural") and
> commit 3826121d9298 ("target-arm: Implement checking of fired")
> introduced an ARM-specific hack for cpu_check_watchpoint.
>
> Make debug_check_watchpoint optional, and move it to tcg_ops.
>
> Signed-off-by: Claudio Fontana 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

[PATCH v3 0/2] pci: add romsize property

[Paolo Bonzini]

This property can be useful for distros to set up known-good ROM sizes for
migration purposes.  The VM will fail to start if the ROM is too large,
and migration compatibility will not be broken if the ROM is too small.

v1->v2: fix overflow issues in nearby code [Laszlo]

v2->v3: consistently use %u in error messages [David]

Paolo

[PATCH v3 2/2] pci: add romsize property

[Paolo Bonzini]

This property can be useful for distros to set up known-good ROM sizes for
migration purposes.  The VM will fail to start if the ROM is too large,
and migration compatibility will not be broken if the ROM is too small.

Note that even though romsize is a uint32_t, it has to be between 1
(because empty ROM files are not accepted, and romsize must be greater
than the file) and 2^31 (because values above are not powers of two and
are rejected).

Signed-off-by: Paolo Bonzini 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Peter Xu 
Message-Id: <20201218182736.1634344-1-pbonz...@redhat.com>
Signed-off-by: Paolo Bonzini 
---
 hw/pci/pci.c | 19 +--
 hw/xen/xen_pt_load_rom.c | 14 --
 include/hw/pci/pci.h |  1 +
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 58560c044d..a9ebef8a35 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -69,6 +69,7 @@ static void pcibus_reset(BusState *qbus);
 static Property pci_props[] = {
 DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
 DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
+DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, -1),
 DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
 DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
 QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
@@ -2084,6 +2085,11 @@ static void pci_qdev_realize(DeviceState *qdev, Error 
**errp)
 bool is_default_rom;
 uint16_t class_id;
 
+if (pci_dev->romsize != -1 && !is_power_of_2(pci_dev->romsize)) {
+error_setg(errp, "ROM size %u is not a power of two", 
pci_dev->romsize);
+return;
+}
+
 /* initialize cap_present for pci_is_express() and pci_config_size(),
  * Note that hybrid PCIs are not set automatically and need to manage
  * QEMU_PCI_CAP_EXPRESS manually */
@@ -2349,7 +2355,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool 
is_default_rom,
 g_free(path);
 return;
 }
-size = pow2ceil(size);
+if (pdev->romsize != -1) {
+if (size > pdev->romsize) {
+error_setg(errp, "romfile \"%s\" (%u bytes) is too large for ROM 
size %u",
+   pdev->romfile, (uint32_t)size, pdev->romsize);
+g_free(path);
+return;
+}
+} else {
+pdev->romsize = pow2ceil(size);
+}
 
 vmsd = qdev_get_vmsd(DEVICE(pdev));
 
@@ -2359,7 +2374,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool 
is_default_rom,
 snprintf(name, sizeof(name), "%s.rom", 
object_get_typename(OBJECT(pdev)));
 }
 pdev->has_rom = true;
-memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, size, &error_fatal);
+memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, pdev->romsize, 
&error_fatal);
 ptr = memory_region_get_ram_ptr(&pdev->rom);
 if (load_image_size(path, ptr, size) < 0) {
 error_setg(errp, "failed to load romfile \"%s\"", pdev->romfile);
diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c
index a50a80837e..03422a8a71 100644
--- a/hw/xen/xen_pt_load_rom.c
+++ b/hw/xen/xen_pt_load_rom.c
@@ -53,10 +53,20 @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev,
 }
 fseek(fp, 0, SEEK_SET);
 
+if (dev->romsize != -1) {
+if (st.st_size > dev->romsize) {
+error_report("ROM BAR \"%s\" (%ld bytes) is too large for ROM size 
%u",
+ rom_file, (long) st.st_size, dev->romsize);
+goto close_rom;
+}
+} else {
+dev->romsize = st.st_size;
+}
+
 snprintf(name, sizeof(name), "%s.rom", object_get_typename(owner));
-memory_region_init_ram(&dev->rom, owner, name, st.st_size, &error_abort);
+memory_region_init_ram(&dev->rom, owner, name, dev->romsize, &error_abort);
 ptr = memory_region_get_ram_ptr(&dev->rom);
-memset(ptr, 0xff, st.st_size);
+memset(ptr, 0xff, dev->romsize);
 
 if (!fread(ptr, 1, st.st_size, fp)) {
 error_report("pci-assign: Cannot read from host %s", rom_file);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 66db08462f..1bc231480f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -344,6 +344,7 @@ struct PCIDevice {
 
 /* Location of option rom */
 char *romfile;
+uint32_t romsize;
 bool has_rom;
 MemoryRegion rom;
 uint32_t rom_bar;
-- 
2.29.2

[PATCH v3 1/2] pci: reject too large ROMs

[Paolo Bonzini]

get_image_size() returns an int64_t, which pci_add_option_rom() assigns
to an "int" without any range checking.  A 32-bit BAR could be up to
2 GiB in size, so reject anything above it.  In order to accomodate
a rounded-up size of 2 GiB, change pci_patch_ids's size argument
to unsigned.

Reviewed-by: Peter Xu 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Laszlo Ersek 
Signed-off-by: Paolo Bonzini 
---
 hw/pci/pci.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 512e9042ff..58560c044d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/datadir.h"
+#include "qemu/units.h"
 #include "hw/irq.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bridge.h"
@@ -2234,7 +2235,7 @@ static uint8_t pci_find_capability_at_offset(PCIDevice 
*pdev, uint8_t offset)
 
 /* Patch the PCI vendor and device ids in a PCI rom image if necessary.
This is needed for an option rom which is used for more than one device. */
-static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, int size)
+static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size)
 {
 uint16_t vendor_id;
 uint16_t device_id;
@@ -2292,7 +2293,7 @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, 
int size)
 static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
Error **errp)
 {
-int size;
+int64_t size;
 char *path;
 void *ptr;
 char name[32];
@@ -2342,6 +2343,11 @@ static void pci_add_option_rom(PCIDevice *pdev, bool 
is_default_rom,
 error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
 g_free(path);
 return;
+} else if (size > 2 * GiB) {
+error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 GiB)",
+   pdev->romfile);
+g_free(path);
+return;
 }
 size = pow2ceil(size);
 
-- 
2.29.2

Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Vladimir Sementsov-Ogievskiy]

03.02.2021 16:00, Peter Krempa wrote:

Bitmap's source persistence is transported over the migration stream and
the destination mirrors it. In some cases the destination might want to
persist bitmaps which are not persistent on the source (e.g. the result
of merge of bitmaps from a number of layers on the source when migrating
into a squashed image)


Why not make merge target on source be persistent itself? Then it will be 
persistent on migration destination.


but currently it would need to create another set
of persistent bitmaps and merge them.

This adds 'dest-persistent' optional property to
'BitmapMigrationBitmapAlias' which when present overrides the bitmap
presence state from the source.


It's seems simpler to make a separate qmp command 
block-dirty-bitmap-make-persistent.. Didn't you consider this way?



--
Best regards,
Vladimir

[PULL v4 00/36] Misc patches (buildsys, i386, fuzzing) for 2021-01-29

[Paolo Bonzini]

The following changes since commit cf7ca7d5b9faca13f1f8e3ea92cfb2f741eb0c0e:

  Merge remote-tracking branch 
'remotes/stefanha-gitlab/tags/tracing-pull-request' into staging (2021-02-01 
16:28:00 +)

are available in the Git repository at:

  https://gitlab.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to 4e1cd7b1d59727ae471bae95db4002739eda085b:

  pc-bios/descriptors: fix paths in json files (2021-02-03 14:20:21 +0100)


* Fuzzing improvements (Qiuhao, Alexander)
* i386: Fix BMI decoding for instructions with the 0x66 prefix (David)
* initial attempt at fixing event_notifier emulation (Maxim)
* i386: PKS emulation, fix for "qemu-system-i386 -cpu host" (myself)
* meson: RBD test fixes (myself)
* meson: TCI warnings (Philippe)
* Leaner build for --disable-guest-agent, --disable-system and
  --disable-tools (Philippe, Stefan)
* --enable-tcg-interpreter fix (Richard)
* i386: SVM feature bits (Wei)
* HVF bugfix (Alex)
* KVM bugfix (Thomas)



v3->v4: dropped slirp update

Alexander Bulekov (7):
  fuzz: ignore address_space_map is_write flag
  fuzz: refine the ide/ahci fuzzer configs
  docs/fuzz: fix pre-meson path
  fuzz: log the arguments used to initialize QEMU
  fuzz: enable dynamic args for generic-fuzz configs
  docs/fuzz: add some information about OSS-Fuzz
  fuzz: add virtio-9p configurations for fuzzing

Alexander Graf (1):
  hvf: Fetch cr4 before evaluating CPUID(1)

David Greenaway (1):
  target/i386: Fix decoding of certain BMI instructions

Igor Mammedov (1):
  machine: add missing doc for memory-backend option

Maxim Levitsky (2):
  virtio-scsi: don't uninitialize queues that we didn't initialize
  event_notifier: handle initialization failure better

Paolo Bonzini (4):
  target/i386: do not set LM for 32-bit emulation "-cpu host/max"
  meson: accept either shared or static libraries if --disable-static
  meson: honor --enable-rbd if cc.links test fails
  target/i86: implement PKS

Pavel Dovgalyuk (1):
  replay: fix replay of the interrupts

Philippe Mathieu-Daudé (13):
  configure: Improve TCI feature description
  meson: Explicit TCG backend used
  meson: Warn when TCI is selected but TCG backend is available
  tests/meson: Only build softfloat objects if TCG is selected
  pc-bios/meson: Only install EDK2 blob firmwares with system emulation
  meson: Restrict block subsystem processing
  meson: Merge trace_events_subdirs array
  meson: Restrict some trace event directories to user/system emulation
  meson: Restrict emulation code
  qapi/meson: Restrict qdev code to system-mode emulation
  qapi/meson: Remove QMP from user-mode emulation
  qapi/meson: Restrict system-mode specific modules
  qapi/meson: Restrict UI module to system emulation and tools

Qiuhao Li (1):
  fuzz: fix wrong index in clear_bits

Richard Henderson (1):
  configure: Fix --enable-tcg-interpreter

Sergei Trofimovich (1):
  pc-bios/descriptors: fix paths in json files

Stefan Reiter (1):
  docs: don't install corresponding man page if guest agent is disabled

Thomas Huth (1):
  accel/kvm/kvm-all: Fix wrong return code handling in dirty log code

Wei Huang (1):
  x86/cpu: Populate SVM CPUID feature bits

 MAINTAINERS  |   1 +
 accel/kvm/kvm-all.c  |  21 +--
 accel/tcg/tcg-cpus-icount.c  |   8 +-
 backends/hostmem.c   |  10 ++
 configure|   7 +-
 docs/devel/build-system.rst  |   2 +-
 docs/devel/fuzzing.rst   |  35 -
 docs/meson.build |   6 +-
 hw/scsi/virtio-scsi-dataplane.c  |   8 +-
 include/exec/memory.h|   8 +-
 include/exec/memory_ldst_cached.h.inc|   6 +-
 include/qemu/event_notifier.h|   1 +
 memory_ldst.c.inc|   8 +-
 meson.build  | 214 +--
 meson_options.txt|   2 +-
 pc-bios/descriptors/meson.build  |   2 +-
 pc-bios/meson.build  |   1 +
 qapi/meson.build |  34 +++--
 qemu-options.hx  |  26 +++-
 scripts/oss-fuzz/minimize_qtest_trace.py |   2 +-
 softmmu/memory.c |   5 +-
 softmmu/physmem.c|   4 +-
 stubs/meson.build|   2 +
 stubs/qdev.c |  23 
 target/i386/cpu.c|  15 ++-
 target/i386/cpu.h|  29 +++--
 target/i386/helper.c |   3 +
 target/i386/hvf/hvf.c|   4 +
 target/i386/machine.c|  24 +++-
 target/i386/tcg/excp_helper.c  

Re: [PULL 00/20] NBD patches for 2021-02-02

[Eric Blake]

On 2/2/21 5:12 PM, no-re...@patchew.org wrote:
> Patchew URL: 
> https://patchew.org/QEMU/20210202224529.642055-1-ebl...@redhat.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 

> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #73: 
> new file mode 100644
> 
> ERROR: trailing whitespace
> #82: FILE: block/io.c.rej:5:

Ouch. That file should not be there. I will send a v2 pull request ASAP.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Peter Krempa]

On Wed, Feb 03, 2021 at 16:23:21 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 03.02.2021 16:00, Peter Krempa wrote:
> > Bitmap's source persistence is transported over the migration stream and
> > the destination mirrors it. In some cases the destination might want to
> > persist bitmaps which are not persistent on the source (e.g. the result
> > of merge of bitmaps from a number of layers on the source when migrating
> > into a squashed image)
> 
> Why not make merge target on source be persistent itself? Then it will be 
> persistent on migration destination.

Because they are temporary on the source. I don't want to make it
persistent in case of a failure so that it doesn't get written to the
disk e.g. in case of VM shutdown.

> 
> > but currently it would need to create another set
> > of persistent bitmaps and merge them.
> > 
> > This adds 'dest-persistent' optional property to
> > 'BitmapMigrationBitmapAlias' which when present overrides the bitmap
> > presence state from the source.
> 
> It's seems simpler to make a separate qmp command 
> block-dirty-bitmap-make-persistent.. Didn't you consider this way?

I'm not sure how the internals work entirely. In my case it's way
simpler to do this setup when generating the mapping which I need to do
anyways rather than calling separate commands.

Re: [PULL 00/20] NBD patches for 2021-02-02

[Eric Blake]

On 2/2/21 4:45 PM, Eric Blake wrote:
> The following changes since commit 77f3804ab7ed94b471a14acb260e5aeacf26193f:
> 
>   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging 
> (2021-02-02 16:47:51 +)
> 
> are available in the Git repository at:
> 
>   https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2021-02-02
> 
> for you to fetch changes up to bb1b0015dfc77bd8b82d8be806f8822d19e749b8:
> 
>   nbd: make nbd_read* return -EIO on error (2021-02-02 16:30:50 -0600)
> 
> 
> nbd patches for 2021-02-02
> 
> - more cleanup from iotest python conversion
> - progress towards consistent use of signed 64-bit types through block layer
> - fix some crashes related to NBD reconnect
> 

NACK, v2 coming up due to a rebase flaw in this spin.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 20/20] RFC: tests: add some virtio-gpu & vhost-user-gpu acceptance test

[Marc-André Lureau]

Hi

On Wed, Feb 3, 2021 at 3:44 PM Gerd Hoffmann  wrote:
>
> On Tue, Feb 02, 2021 at 06:26:25PM +0400, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> >
> > This will check virtio/vhost-user-vga & virgl are correctly initialized
> > by the Linux kernel on an egl-headless display.
> >
> > There are many other things that could be checked, but that's a start. I
> > also don't know yet how to nicely skip on incompatible host &
> > configurations.
>
> You can annotate tests like this:
>
>@avocado.skipUnless(os.path.exists(pick_default_vug_bin()), "no 
> vhost-user-gpu")
> > +def test_vhost_user_vga_virgl(self):

Ah interesting, thanks!

>
> [ queued whole series + some other pending ui/vga bits, kicked CI, lets
>   see how it is going.  I suspect I'll have to drop this patch though. ]
>

Yes, Cleber was going to take a look, and perhaps send another avocado
series I could take some inspiration from.

thanks!



-- 
Marc-André Lureau

Re: [PATCH v15 23/23] accel-cpu: make cpu_realizefn return a bool

[Philippe Mathieu-Daudé]

On 2/1/21 11:09 AM, Claudio Fontana wrote:
> overall, all devices' realize functions take an Error **errp, but return void.
> 
> hw/core/qdev.c code, which realizes devices, therefore does:
> 
> local_err = NULL;
> dc->realize(dev, &local_err);
> if (local_err != NULL) {
> goto fail;
> }
> 
> However, we can improve at least accel_cpu to return a meaningful bool value.
> 
> Signed-off-by: Claudio Fontana 
> ---
>  include/hw/core/accel-cpu.h |  2 +-
>  include/qemu/accel.h|  2 +-
>  target/i386/host-cpu.h  |  2 +-
>  accel/accel-common.c|  6 +++---
>  cpu.c   |  5 +++--
>  target/i386/host-cpu.c  | 25 ++---
>  target/i386/kvm/kvm-cpu.c   |  4 ++--
>  target/i386/tcg/tcg-cpu.c   |  6 --
>  8 files changed, 29 insertions(+), 23 deletions(-)
...

> diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
> index 9cfe56ce41..4ea9e354ea 100644
> --- a/target/i386/host-cpu.c
> +++ b/target/i386/host-cpu.c
> @@ -50,7 +50,7 @@ static void host_cpu_enable_cpu_pm(X86CPU *cpu)
>  env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
>  }
>  
> -static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu, Error **errp)
> +static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu)
>  {
>  uint32_t host_phys_bits = host_cpu_phys_bits();
>  uint32_t phys_bits = cpu->phys_bits;
> @@ -77,18 +77,10 @@ static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu, 
> Error **errp)
>  }
>  }
>  
> -if (phys_bits &&
> -(phys_bits > TARGET_PHYS_ADDR_SPACE_BITS ||
> - phys_bits < 32)) {
> -error_setg(errp, "phys-bits should be between 32 and %u "
> -   " (but is %u)",
> -   TARGET_PHYS_ADDR_SPACE_BITS, phys_bits);
> -}
> -
>  return phys_bits;
>  }
>  
> -void host_cpu_realizefn(CPUState *cs, Error **errp)
> +bool host_cpu_realizefn(CPUState *cs, Error **errp)
>  {
>  X86CPU *cpu = X86_CPU(cs);
>  CPUX86State *env = &cpu->env;
> @@ -97,8 +89,19 @@ void host_cpu_realizefn(CPUState *cs, Error **errp)
>  host_cpu_enable_cpu_pm(cpu);
>  }
>  if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
> -cpu->phys_bits = host_cpu_adjust_phys_bits(cpu, errp);
> +uint32_t phys_bits = host_cpu_adjust_phys_bits(cpu);
> +
> +if (phys_bits &&
> +(phys_bits > TARGET_PHYS_ADDR_SPACE_BITS ||
> + phys_bits < 32)) {
> +error_setg(errp, "phys-bits should be between 32 and %u "
> +   " (but is %u)",
> +   TARGET_PHYS_ADDR_SPACE_BITS, phys_bits);

Please this change in a preliminary patch (preferably), or comment it
in the commit description.
Either ways:
Reviewed-by: Philippe Mathieu-Daudé 

> +return false;
> +}
> +cpu->phys_bits = phys_bits;
>  }
> +return true;
>  }

[Bug 1914117] Re: Short files returned via FTP on Qemu with various architectures and OSes

[Chris Pinnock]

I've spent some more time on this.
I've tcpdump'ed the connection whilst doing the download (via both HTTP & FTP).

In the last data packet, the last byte that is missing on the filesystem
is in the packet, but the packet has the urgent bit set with the urgent
pointer the same as the length of the packet.

I'm not sure but this might cause the client app to discard part of the packet?
Unclear.

Also I've build Qemu 4.2.1 on MacOS X/Big Sur - I'm seeing the same issue on 
FreeBSD/amd64.
This bug might be related:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=237441


** Bug watch added: bugs.freebsd.org/bugzilla/ #237441
   https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=237441

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

Title:
  Short files returned via FTP on Qemu with various architectures and
  OSes

Status in QEMU:
  New

Bug description:
  
  Qemu 5.2 on Mac OS X Big Sur.

  I originally thought that it might be caused by the home-brew version of 
Qemu, but this evening I have removed the brew edition and compiled from 
scratch (using Ninja & Xcode compiler). 
  Still getting the same problem,.

  On the following architectures: 
  arm64, amd64 and sometimes i386 running NetBSD host OS; 
  i386 running OpenBSD host OS:

  I have seen a consistent problem with FTP returning short files. The
  file will be a couple of bytes too short. I do not believe this is a
  problem with the OS. Downloading the perl source code from CPAN does
  not work properly, nor does downloading bind from isc. I've tried this
  on different architectures as above.

  (Qemu 4.2 on Ubuntu/x86_64 with NetBSD/i386 seems to function fine. My
  gut feel is there is something not right on the Mac OS version of Qemu
  or a bug in 5.2 - obviously in the network layer somewhere. If you
  have anything you want me to try, please let me know - happy to help
  get a resolution.)

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

Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Peter Krempa]

On Wed, Feb 03, 2021 at 14:27:49 +0100, Peter Krempa wrote:
> On Wed, Feb 03, 2021 at 16:23:21 +0300, Vladimir Sementsov-Ogievskiy wrote:
> > 03.02.2021 16:00, Peter Krempa wrote:
> > > Bitmap's source persistence is transported over the migration stream and
> > > the destination mirrors it. In some cases the destination might want to
> > > persist bitmaps which are not persistent on the source (e.g. the result
> > > of merge of bitmaps from a number of layers on the source when migrating
> > > into a squashed image)
> > 
> > Why not make merge target on source be persistent itself? Then it will be 
> > persistent on migration destination.
> 
> Because they are temporary on the source. I don't want to make it
> persistent in case of a failure so that it doesn't get written to the
> disk e.g. in case of VM shutdown.

To be a bit more specific, I don't want the bitmaps to stay in the
image, that means that I'd have to also delete them on the source after
a successfull migration before qemu is terminated, which might not even
be possible since source deactivates storage after migration.

So making them persistent on source is impossible.

> 
> > 
> > > but currently it would need to create another set
> > > of persistent bitmaps and merge them.
> > > 
> > > This adds 'dest-persistent' optional property to
> > > 'BitmapMigrationBitmapAlias' which when present overrides the bitmap
> > > presence state from the source.
> > 
> > It's seems simpler to make a separate qmp command 
> > block-dirty-bitmap-make-persistent.. Didn't you consider this way?
> 
> I'm not sure how the internals work entirely. In my case it's way
> simpler to do this setup when generating the mapping which I need to do
> anyways rather than calling separate commands.

Similarly here, after a successful migration I'd have to go and make all
the bitmaps persistent, which is an extra step, and also a vector for
possible failures after migration which also doesn't seem appealing.

Re: [PATCH v4 02/14] qapi/introspect.py: use _make_tree for features nodes

[Markus Armbruster]

John Snow  writes:

> At present, we open-code this in _make_tree itself; but if the structure
> of the tree changes, this is brittle. Use an explicit recursive call to
> _make_tree when appropriate to help keep the interior node typing
> consistent.
>
> Signed-off-by: John Snow 
> ---
>  scripts/qapi/introspect.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 43ab4be1f77..3295a15c98e 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -30,7 +30,9 @@ def _make_tree(obj, ifcond, features, extra=None):
>  if ifcond:
>  extra['if'] = ifcond
>  if features:
> -obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
> +obj['features'] = [
> +_make_tree(f.name, f.ifcond, None) for f in features
> +]
>  if extra:
>  return (obj, extra)
>  return obj

The commit message made me expect a patch that improves the code without
changing its behavior.  This is not the case.  We go from

obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]

to

obj['features'] = [_make_tree(f.name, f.ifcond) for f in features]

where

_make_tree(f.name, f.ifcond)
= (f.name, {'if': f.ifcond})   if f.ifcond
= f.name   else

Differs when not f.ifcond.  Feels like an improvement.  However, the
commit message did not prepare me for it.

[PATCH 1/2] hw/ppc: e500: Use a macro for the platform clock frequency

[Bin Meng]

From: Bin Meng 

At present the platform clock frequency is using a magic number.
Convert it to a macro and use it everywhere.

Signed-off-by: Bin Meng 
---

 hw/ppc/e500.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index c64b5d0..672ccd5 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -74,6 +74,8 @@
 #define MPC8544_I2C_IRQ43
 #define RTC_REGS_OFFSET0x68
 
+#define PLATFORM_CLK_FREQ  4
+
 struct boot_info
 {
 uint32_t dt_base;
@@ -320,8 +322,8 @@ static int ppce500_load_device_tree(PPCE500MachineState 
*pms,
 int fdt_size;
 void *fdt;
 uint8_t hypercall[16];
-uint32_t clock_freq = 4;
-uint32_t tb_freq = 4;
+uint32_t clock_freq = PLATFORM_CLK_FREQ;
+uint32_t tb_freq = PLATFORM_CLK_FREQ;
 int i;
 char compatible_sb[] = "fsl,mpc8544-immr\0simple-bus";
 char *soc;
@@ -890,7 +892,7 @@ void ppce500_init(MachineState *machine)
 env->spr_cb[SPR_BOOKE_PIR].default_value = cs->cpu_index = i;
 env->mpic_iack = pmc->ccsrbar_base + MPC8544_MPIC_REGS_OFFSET + 0xa0;
 
-ppc_booke_timers_init(cpu, 4, PPC_TIMER_E500);
+ppc_booke_timers_init(cpu, PLATFORM_CLK_FREQ, PPC_TIMER_E500);
 
 /* Register reset handler */
 if (!i) {
-- 
2.7.4

[Bug 1813406] Re: qemu-img convert malfunction on macOS

[Thomas Huth]

The QEMU project is currently considering to move its bug tracking to another 
system. For this we need to know which bugs are still valid and which could be 
closed already. Thus we are setting older bugs to "Incomplete" now.
If you still think this bug report here is valid, then please switch the state 
back to "New" within the next 60 days, otherwise this report will be marked as 
"Expired". Or mark it as "Fix Released" if the problem has been solved with a 
newer version of QEMU already. Thank you and sorry for the inconvenience.

** Changed in: qemu
   Status: New => Incomplete

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

Title:
  qemu-img convert malfunction on macOS

Status in QEMU:
  Incomplete

Bug description:
  On macOS 10.13.6, `qemu-img convert` failed to convert a qcow2 into a
  new qcow2 (for the purpose of shrinking the image).

  A 50GB (3.7GB allocated) qcow2 image was used as source. The qemu-img
  convert output was a 3.4MB file.

  qemu-img from HomeBrew were tested. Both 2.11.1 and 3.1.0_1 failed to
  convert a qcow2 image.

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

[PATCH 2/2] hw/ppc: e500: Fill in correct for the serial nodes

[Bin Meng]

From: Bin Meng 

At present the  property of the serial node is
populated with value zero. U-Boot's ns16550 driver is not happy
about this, so let's fill in a meaningful value.

Signed-off-by: Bin Meng 
---

 hw/ppc/e500.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 672ccd5..e4677af 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -126,7 +126,7 @@ static void dt_serial_create(void *fdt, unsigned long long 
offset,
 qemu_fdt_setprop_string(fdt, ser, "compatible", "ns16550");
 qemu_fdt_setprop_cells(fdt, ser, "reg", offset, 0x100);
 qemu_fdt_setprop_cell(fdt, ser, "cell-index", idx);
-qemu_fdt_setprop_cell(fdt, ser, "clock-frequency", 0);
+qemu_fdt_setprop_cell(fdt, ser, "clock-frequency", PLATFORM_CLK_FREQ);
 qemu_fdt_setprop_cells(fdt, ser, "interrupts", 42, 2);
 qemu_fdt_setprop_phandle(fdt, ser, "interrupt-parent", mpic);
 qemu_fdt_setprop_string(fdt, "/aliases", alias, ser);
-- 
2.7.4

[Bug 1813940] Re: kvm_mem_ioeventfd_add: error adding ioeventfd: No space left on device

[Thomas Huth]

That patch has been included here:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=4f8260248c68e4599a5
Thus closing this ticket now.

** Changed in: qemu
   Status: Confirmed => Fix Released

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

Title:
  kvm_mem_ioeventfd_add: error adding ioeventfd: No space left on device

Status in QEMU:
  Fix Released

Bug description:
  Latest QEMU master fails to run with too many MMIO devices specified.

  After patch 3ac7d43a6fb [1] QEMU just prints an error message and exits.
  > kvm_mem_ioeventfd_add: error adding ioeventfd: No space left on device

  This is reproducible e.g. with the following setup:

  qemu-3.1.50-dirty \
  -machine pc-i440fx-2.7,accel=kvm \
  -cpu host -m 4096 \
  -smp 2,sockets=2,cores=1,threads=1 \
  -drive file=freebsd_vm_1.qcow2,format=qcow2,if=none,id=bootdr \
  -device ide-hd,drive=bootdr,bootindex=0 \
  -device virtio-scsi-pci,id=vc0 \
  -device virtio-scsi-pci,id=vc1 \
  -device virtio-scsi-pci,id=vc2 \
  -device virtio-scsi-pci,id=vc3 \

  Running with just 3 Virtio-SCSI controllers seems to work fine, adding
  more than that causes the error above. Note that this is not Virtio-
  SCSI specific. I've also reproduced this without any Virtio devices
  whatsoever.

  strace shows the following ioctl chain over and over:

  145787 ioctl(11, KVM_UNREGISTER_COALESCED_MMIO, 0x7f60a4985410) = 0
  145787 ioctl(11, KVM_UNREGISTER_COALESCED_MMIO, 0x7f60a4985410) = 0
  145787 ioctl(11, KVM_REGISTER_COALESCED_MMIO, 0x7f60a49853b0) = 0
  145787 ioctl(11, KVM_REGISTER_COALESCED_MMIO, 0x7f60a49853b0) = -1 ENOSPC (No 
space left on device)
  145787 ioctl(11, KVM_REGISTER_COALESCED_MMIO, 0x7f60a49853b0) = -1 ENOSPC (No 
space left on device)
  145787 ioctl(11, KVM_REGISTER_COALESCED_MMIO, 0x7f60a49853b0) = -1 ENOSPC (No 
space left on device)
  145787 ioctl(11, KVM_REGISTER_COALESCED_MMIO, 0x7f60a49853b0) = -1 ENOSPC (No 
space left on device)
  145787 ioctl(11, KVM_REGISTER_COALESCED_MMIO, 0x7f60a49853b0) = -1 ENOSPC (No 
space left on device)
  145787 ioctl(11, KVM_REGISTER_COALESCED_MMIO, 0x7f60a49853b0) = -1 ENOSPC (No 
space left on device)
  145787 ioctl(11, KVM_REGISTER_COALESCED_MMIO, 0x7f60a49853b0) = -1 ENOSPC (No 
space left on device)
  145787 ioctl(11, KVM_REGISTER_COALESCED_MMIO, 0x7f60a49853b0) = -1 ENOSPC (No 
space left on device)

  Which suggests there's some kind of MMIO region leak.

  [1]
  commit 3ac7d43a6fbb5d4a3d01fc9a055c218030af3727
  Author: Paolo Bonzini 
  AuthorDate: Wed Nov 28 17:28:45 2018 +0100
  Commit: Paolo Bonzini 
  CommitDate: Fri Jan 11 13:57:24 2019 +0100

  memory: update coalesced_range on transaction_commit

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

Re: [PATCH v3 1/2] pci: reject too large ROMs

[David Edmondson]

On Wednesday, 2021-02-03 at 14:18:27 +01, Paolo Bonzini wrote:

> get_image_size() returns an int64_t, which pci_add_option_rom() assigns
> to an "int" without any range checking.  A 32-bit BAR could be up to
> 2 GiB in size, so reject anything above it.  In order to accomodate
> a rounded-up size of 2 GiB, change pci_patch_ids's size argument
> to unsigned.
>
> Reviewed-by: Peter Xu 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Laszlo Ersek 
> Signed-off-by: Paolo Bonzini 

Reviewed-by: David Edmondson 

> ---
>  hw/pci/pci.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 512e9042ff..58560c044d 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -25,6 +25,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qemu/datadir.h"
> +#include "qemu/units.h"
>  #include "hw/irq.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bridge.h"
> @@ -2234,7 +2235,7 @@ static uint8_t pci_find_capability_at_offset(PCIDevice 
> *pdev, uint8_t offset)
>  
>  /* Patch the PCI vendor and device ids in a PCI rom image if necessary.
> This is needed for an option rom which is used for more than one device. 
> */
> -static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, int size)
> +static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size)
>  {
>  uint16_t vendor_id;
>  uint16_t device_id;
> @@ -2292,7 +2293,7 @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t 
> *ptr, int size)
>  static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
> Error **errp)
>  {
> -int size;
> +int64_t size;
>  char *path;
>  void *ptr;
>  char name[32];
> @@ -2342,6 +2343,11 @@ static void pci_add_option_rom(PCIDevice *pdev, bool 
> is_default_rom,
>  error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
>  g_free(path);
>  return;
> +} else if (size > 2 * GiB) {
> +error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 
> GiB)",
> +   pdev->romfile);
> +g_free(path);
> +return;
>  }
>  size = pow2ceil(size);
>  
> -- 
> 2.29.2

dme.
-- 
I used to worry, thought I was goin' mad in a hurry.

Re: [PULL v2 00/21] target-arm queue

[Peter Maydell]

On Wed, 3 Feb 2021 at 10:17, Peter Maydell  wrote:
>
> no changes to v1, except adding the CVE identifier to one of the commit
> messages.
>
> -- PMM
>
> The following changes since commit cf7ca7d5b9faca13f1f8e3ea92cfb2f741eb0c0e:
>
>   Merge remote-tracking branch 
> 'remotes/stefanha-gitlab/tags/tracing-pull-request' into staging (2021-02-01 
> 16:28:00 +)
>
> are available in the Git repository at:
>
>   https://git.linaro.org/people/pmaydell/qemu-arm.git 
> tags/pull-target-arm-20210203
>
> for you to fetch changes up to fd8f71b95da86f530aae3d02a14b0ccd9e024772:
>
>   hw/arm: Display CPU type in machine description (2021-02-03 10:15:51 +)
>
> 
> target-arm queue:
>  * hw/intc/arm_gic: Allow to use QTest without crashing
>  * hw/char/exynos4210_uart: Fix buffer size reporting with FIFO disabled
>  * hw/char/exynos4210_uart: Fix missing call to report ready for input
>  * hw/arm/smmuv3: Fix addr_mask for range-based invalidation
>  * hw/ssi/imx_spi: Fix various minor bugs
>  * hw/intc/arm_gic: Fix interrupt ID in GICD_SGIR register
>  * hw/arm: Add missing Kconfig dependencies
>  * hw/arm: Display CPU type in machine description
>
> 


Applied, thanks.

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

-- PMM

Re: [PATCH v4 04/14] qapi/introspect.py: guard against ifcond/comment misuse

[Markus Armbruster]

John Snow  writes:

> _tree_to_qlit is called recursively on dict values alone; at such a
> point in generating output it is too late to apply an ifcond. Similarly,
> comments do not necessarily have a "tidy" place they can be printed in
> such a circumstance.
>
> Forbid this usage.
>
> Signed-off-by: John Snow 
> ---
>  scripts/qapi/introspect.py | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 4749f65ea3c..ccdf4f1c0d0 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -43,6 +43,12 @@ def indent(level):
>  ifobj, extra = obj
>  ifcond = extra.get('if')
>  comment = extra.get('comment')
> +
> +# NB: _tree_to_qlit is called recursively on the values of a 
> key:value
> +# pair; those values can't be decorated with comments or 
> conditionals.
> +msg = "dict values cannot have attached comments or if-conditionals."
> +assert not suppress_first_indent, msg
> +
>  ret = ''
>  if comment:
>  ret += indent(level) + '/* %s */\n' % comment

This uses @suppress_first_indent as a proxy for "@obj is a value in a
dict".  Works, because we pass suppress_first_indent=True exactly
there.  Took me a minute to see, though.

Do you need this assertion to help mypy over the hump?

Perhaps we'd be better off with two functions, one that takes possibly
annotated @obj, and one that takes only plain @obj.  "Yes, but not now"
woule be one acceptable answer to that.

Re: [PATCH 2/2] hw/ppc: e500: Fill in correct for the serial nodes

[Philippe Mathieu-Daudé]

On 2/3/21 3:01 PM, Bin Meng wrote:
> From: Bin Meng 
> 
> At present the  property of the serial node is
> populated with value zero. U-Boot's ns16550 driver is not happy
> about this, so let's fill in a meaningful value.
> 
> Signed-off-by: Bin Meng 
> ---
> 
>  hw/ppc/e500.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 2/2] pci: add romsize property

[David Edmondson]

On Wednesday, 2021-02-03 at 14:18:28 +01, Paolo Bonzini wrote:

> This property can be useful for distros to set up known-good ROM sizes for
> migration purposes.  The VM will fail to start if the ROM is too large,
> and migration compatibility will not be broken if the ROM is too small.
>
> Note that even though romsize is a uint32_t, it has to be between 1
> (because empty ROM files are not accepted, and romsize must be greater
> than the file) and 2^31 (because values above are not powers of two and
> are rejected).
>
> Signed-off-by: Paolo Bonzini 
> Reviewed-by: Dr. David Alan Gilbert 
> Reviewed-by: Peter Xu 
> Message-Id: <20201218182736.1634344-1-pbonz...@redhat.com>
> Signed-off-by: Paolo Bonzini 

Reviewed-by: David Edmondson 

> ---
>  hw/pci/pci.c | 19 +--
>  hw/xen/xen_pt_load_rom.c | 14 --
>  include/hw/pci/pci.h |  1 +
>  3 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 58560c044d..a9ebef8a35 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -69,6 +69,7 @@ static void pcibus_reset(BusState *qbus);
>  static Property pci_props[] = {
>  DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>  DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> +DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, -1),
>  DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
>  DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
>  QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
> @@ -2084,6 +2085,11 @@ static void pci_qdev_realize(DeviceState *qdev, Error 
> **errp)
>  bool is_default_rom;
>  uint16_t class_id;
>  
> +if (pci_dev->romsize != -1 && !is_power_of_2(pci_dev->romsize)) {
> +error_setg(errp, "ROM size %u is not a power of two", 
> pci_dev->romsize);
> +return;
> +}
> +
>  /* initialize cap_present for pci_is_express() and pci_config_size(),
>   * Note that hybrid PCIs are not set automatically and need to manage
>   * QEMU_PCI_CAP_EXPRESS manually */
> @@ -2349,7 +2355,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool 
> is_default_rom,
>  g_free(path);
>  return;
>  }
> -size = pow2ceil(size);
> +if (pdev->romsize != -1) {
> +if (size > pdev->romsize) {
> +error_setg(errp, "romfile \"%s\" (%u bytes) is too large for ROM 
> size %u",
> +   pdev->romfile, (uint32_t)size, pdev->romsize);
> +g_free(path);
> +return;
> +}
> +} else {
> +pdev->romsize = pow2ceil(size);
> +}
>  
>  vmsd = qdev_get_vmsd(DEVICE(pdev));
>  
> @@ -2359,7 +2374,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool 
> is_default_rom,
>  snprintf(name, sizeof(name), "%s.rom", 
> object_get_typename(OBJECT(pdev)));
>  }
>  pdev->has_rom = true;
> -memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, size, 
> &error_fatal);
> +memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, pdev->romsize, 
> &error_fatal);
>  ptr = memory_region_get_ram_ptr(&pdev->rom);
>  if (load_image_size(path, ptr, size) < 0) {
>  error_setg(errp, "failed to load romfile \"%s\"", pdev->romfile);
> diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c
> index a50a80837e..03422a8a71 100644
> --- a/hw/xen/xen_pt_load_rom.c
> +++ b/hw/xen/xen_pt_load_rom.c
> @@ -53,10 +53,20 @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev,
>  }
>  fseek(fp, 0, SEEK_SET);
>  
> +if (dev->romsize != -1) {
> +if (st.st_size > dev->romsize) {
> +error_report("ROM BAR \"%s\" (%ld bytes) is too large for ROM 
> size %u",
> + rom_file, (long) st.st_size, dev->romsize);
> +goto close_rom;
> +}
> +} else {
> +dev->romsize = st.st_size;
> +}
> +
>  snprintf(name, sizeof(name), "%s.rom", object_get_typename(owner));
> -memory_region_init_ram(&dev->rom, owner, name, st.st_size, &error_abort);
> +memory_region_init_ram(&dev->rom, owner, name, dev->romsize, 
> &error_abort);
>  ptr = memory_region_get_ram_ptr(&dev->rom);
> -memset(ptr, 0xff, st.st_size);
> +memset(ptr, 0xff, dev->romsize);
>  
>  if (!fread(ptr, 1, st.st_size, fp)) {
>  error_report("pci-assign: Cannot read from host %s", rom_file);
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 66db08462f..1bc231480f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -344,6 +344,7 @@ struct PCIDevice {
>  
>  /* Location of option rom */
>  char *romfile;
> +uint32_t romsize;
>  bool has_rom;
>  MemoryRegion rom;
>  uint32_t rom_bar;
> -- 
> 2.29.2

dme.
-- 
I'm not the reason you're looking for redemption.

Re: [PATCH 1/2] hw/ppc: e500: Use a macro for the platform clock frequency

[Philippe Mathieu-Daudé]

On 2/3/21 3:01 PM, Bin Meng wrote:
> From: Bin Meng 
> 
> At present the platform clock frequency is using a magic number.
> Convert it to a macro and use it everywhere.
> 
> Signed-off-by: Bin Meng 
> ---
> 
>  hw/ppc/e500.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index c64b5d0..672ccd5 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -74,6 +74,8 @@
>  #define MPC8544_I2C_IRQ43
>  #define RTC_REGS_OFFSET0x68
>  
> +#define PLATFORM_CLK_FREQ  4

Consider 1/ using unit in variable name and 2/ decompose:

#define PLATFORM_CLK_FREQ_HZ  (400 * 1000 * 1000)

Regardless:
Reviewed-by: Philippe Mathieu-Daudé 

[Bug 1810603] Re: QEMU QCow Images grow dramatically

[Thomas Huth]

The QEMU project is currently considering to move its bug tracking to another 
system. For this we need to know which bugs are still valid and which could be 
closed already. Thus we are setting older bugs to "Incomplete" now.
If you still think this bug report here is valid, then please switch the state 
back to "New" within the next 60 days, otherwise this report will be marked as 
"Expired". Or mark it as "Fix Released" if the problem has been solved with a 
newer version of QEMU already. Thank you and sorry for the inconvenience.

** Changed in: qemu
   Status: New => Incomplete

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

Title:
  QEMU QCow Images grow dramatically

Status in QEMU:
  Incomplete

Bug description:
  I've recently migrated our VM infrastructure (~200 guest on 15 hosts)
  from vbox to Qemu (using KVM / libvirt). We have a master image (QEMU
  QCow v3) from which we spawn multiple instances (linked clones). All
  guests are being revert once per hour for security reasons.

  About 2 weeks after we successfully migrated to Qemu, we noticed that
  almost all disks went full across all 15 hosts. Our investigation
  showed that the initial qcow disk images blow up from a few gigabytes
  to 100GB and more. This should not happen, as we revert all VMs back
  to the initial snapshot once per hour and hence all changes that have
  been made to disks must be reverted too.

  We did an addition test with 24 hour time frame with which we could
  reproduce this bug as documented below.

  Initial disk image size (created on Jan 04):
  -rw-r--r-- 1 root root 7.1G Jan  4 15:59 W10-TS01-0.img
  -rw-r--r-- 1 root root 7.3G Jan  4 15:59 W10-TS02-0.img
  -rw-r--r-- 1 root root 7.4G Jan  4 15:59 W10-TS03-0.img
  -rw-r--r-- 1 root root 8.3G Jan  4 16:02 W10-CLIENT01-0.img
  -rw-r--r-- 1 root root 8.6G Jan  4 16:05 W10-CLIENT02-0.img
  -rw-r--r-- 1 root root 8.0G Jan  4 16:05 W10-CLIENT03-0.img
  -rw-r--r-- 1 root root 8.3G Jan  4 16:08 W10-CLIENT04-0.img
  -rw-r--r-- 1 root root 8.1G Jan  4 16:12 W10-CLIENT05-0.img
  -rw-r--r-- 1 root root 8.0G Jan  4 16:12 W10-CLIENT06-0.img
  -rw-r--r-- 1 root root 8.1G Jan  4 16:16 W10-CLIENT07-0.img
  -rw-r--r-- 1 root root 7.6G Jan  4 16:16 W10-CLIENT08-0.img
  -rw-r--r-- 1 root root 7.6G Jan  4 16:19 W10-CLIENT09-0.img
  -rw-r--r-- 1 root root 7.5G Jan  4 16:21 W10-ROUTER-0.img
  -rw-r--r-- 1 root root  18G Jan  4 16:25 W10-MASTER-IMG.qcow2

  Disk image size after 24 hours (printed on Jan 05):
  -rw-r--r-- 1 root root  13G Jan  5 15:07 W10-TS01-0.img
  -rw-r--r-- 1 root root 8.9G Jan  5 14:20 W10-TS02-0.img
  -rw-r--r-- 1 root root 9.0G Jan  5 15:07 W10-TS03-0.img
  -rw-r--r-- 1 root root  10G Jan  5 15:08 W10-CLIENT01-0.img
  -rw-r--r-- 1 root root  11G Jan  5 15:08 W10-CLIENT02-0.img
  -rw-r--r-- 1 root root  11G Jan  5 15:08 W10-CLIENT03-0.img
  -rw-r--r-- 1 root root  11G Jan  5 15:08 W10-CLIENT04-0.img
  -rw-r--r-- 1 root root  19G Jan  5 15:07 W10-CLIENT05-0.img
  -rw-r--r-- 1 root root  14G Jan  5 15:08 W10-CLIENT06-0.img
  -rw-r--r-- 1 root root 9.7G Jan  5 15:07 W10-CLIENT07-0.img
  -rw-r--r-- 1 root root  35G Jan  5 15:08 W10-CLIENT08-0.img
  -rw-r--r-- 1 root root 9.2G Jan  5 15:07 W10-CLIENT09-0.img
  -rw-r--r-- 1 root root  41G Jan  5 15:08 W10-ROUTER-0.img
  -rw-r--r-- 1 root root  18G Jan  4 16:25 W10-MASTER-IMG.qcow2

  You can reproduce this bug as follow:
  1) create an initial disk image
  2) create a linked clone
  3) create a snapshot of the linked clone
  4) revert the snapshot every X minutes / hours

  Due the described behavior / bug, our VM farm is completely down at
  the moment (as we run out of disk space on all host systems). A quick
  fix for this bug would be much appreciated.

  Host OS: Ubuntu 18.04.01 LTS
  Kernel: 4.15.0-43-generic
  Qemu: 3.1.0
  libvirt: 4.10.0
  Guest OS: Windows 10 64bit

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

Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Vladimir Sementsov-Ogievskiy]

03.02.2021 16:39, Peter Krempa wrote:

On Wed, Feb 03, 2021 at 14:27:49 +0100, Peter Krempa wrote:

On Wed, Feb 03, 2021 at 16:23:21 +0300, Vladimir Sementsov-Ogievskiy wrote:

03.02.2021 16:00, Peter Krempa wrote:

Bitmap's source persistence is transported over the migration stream and
the destination mirrors it. In some cases the destination might want to
persist bitmaps which are not persistent on the source (e.g. the result
of merge of bitmaps from a number of layers on the source when migrating
into a squashed image)


Why not make merge target on source be persistent itself? Then it will be 
persistent on migration destination.


Because they are temporary on the source. I don't want to make it
persistent in case of a failure so that it doesn't get written to the
disk e.g. in case of VM shutdown.


To be a bit more specific, I don't want the bitmaps to stay in the
image, that means that I'd have to also delete them on the source after
a successfull migration before qemu is terminated, which might not even
be possible since source deactivates storage after migration.

So making them persistent on source is impossible.


Actually on success path, persistent bitmaps are not stored on source.

Normally persistent bitmaps are stored on image inactivation. But bitmaps
involved into migration are an exclusion, they are not stored (otherwise,
stoing will influence downtime of migration). And of-course, we can't
store bitmaps after disks inactivation.

So, on success path, the only way to store bitmaps on source is to do
qmp 'cont' command on source after migration..

I'm not so sure about error path. Of course, if something breaks between
merge target creation and migration start, bitmaps will be stored.

So, I agree that in general it's bad idea to make temporary bitmap 'persistent'.








but currently it would need to create another set
of persistent bitmaps and merge them.

This adds 'dest-persistent' optional property to
'BitmapMigrationBitmapAlias' which when present overrides the bitmap
presence state from the source.


It's seems simpler to make a separate qmp command 
block-dirty-bitmap-make-persistent.. Didn't you consider this way?


I'm not sure how the internals work entirely. In my case it's way
simpler to do this setup when generating the mapping which I need to do
anyways rather than calling separate commands.


Similarly here, after a successful migration I'd have to go and make all
the bitmaps persistent, which is an extra step, and also a vector for
possible failures after migration which also doesn't seem appealing.



OK, that's reasonable, thanks for explanation


--
Best regards,
Vladimir

Re: [PULL 0/9] pc,virtio: fixes, features

[Peter Maydell]

On Tue, 2 Feb 2021 at 22:57, Michael S. Tsirkin  wrote:
> I added a fixup on top, and pushed.

Can you squash fixes into the correct patches in the pullreq, please?
Otherwise it breaks bisection.

thanks
-- PMM