Re: VFIO Migration

[Gerd Hoffmann]

  Hi,

> > > The hardware interface together with the device state representation is 
> > > called
> > > a *device model*. Device models can be assigned URIs such as
> > > https://qemu.org/devices/e1000e to uniquely identify them.
> > 
> > Is that something that needs to be put together for every device where we
> > want to support migration? How do you create the URI?
> 
> Yes. If you are creating a custom device that no one else needs to
> emulate then you can simply pick a unique URL:
> 
>   https://vendor.com/my-dev
> 
> There doesn't need to be anything at the URL. It's just a unique string
> that no one else will use and therefore web URLs are handy because no
> one else will accidentally pick your string.

If this is just a string I think it would be better to use the reverse
domain name scheme (as used by virtio-serial too), i.e.

 - org.qemu.devices.e1000e
 - com.vendor.my-dev

take care,
  Gerd

Re: [PATCH for-5.2 3/4] qtest: escape device name in device-introspect-test

[Paolo Bonzini]

I will just drop autofree usage completely, also because valgrind showed
that GRegex does not support it and apparently is leaked.

Paolo

Il mer 4 nov 2020, 08:44 Thomas Huth  ha scritto:

> On 03/11/2020 16.14, Paolo Bonzini wrote:
> > device-introspect-test uses HMP, so it should escape the device name
> > properly.  Because of this, a few devices that had commas in their
> > names were escaping testing.
> >
> > Signed-off-by: Paolo Bonzini 
> > ---
> >  tests/qtest/device-introspect-test.c | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/qtest/device-introspect-test.c
> b/tests/qtest/device-introspect-test.c
> > index 9f22340ee5..f471b0e0dd 100644
> > --- a/tests/qtest/device-introspect-test.c
> > +++ b/tests/qtest/device-introspect-test.c
> > @@ -104,7 +104,9 @@ static QList *device_type_list(QTestState *qts, bool
> abstract)
> >  static void test_one_device(QTestState *qts, const char *type)
> >  {
> >  QDict *resp;
> > -char *help;
> > +g_autofree char *help;
> > +g_autofree GRegex *comma;
> > +g_autofree char *escaped;
> >
> >  g_test_message("Testing device '%s'", type);
> >
> > @@ -113,8 +115,9 @@ static void test_one_device(QTestState *qts, const
> char *type)
> > type);
> >  qobject_unref(resp);
> >
> > -help = qtest_hmp(qts, "device_add \"%s,help\"", type);
> > -g_free(help);
> > +comma = g_regex_new(",", 0, 0, NULL);
> > +escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0,
> NULL);
> > +help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);
> >  }
>
> Having "help =" as final statement now, this looks somewhat weird at a
> first
> glance (until you look at the g_autofree at the beginning of the function).
> Maybe it's better to drop the help variable completely and just do:
> g_free(gtest_hmp(...)) ?
>
>  Thomas
>
>

Re: [PATCH-for-5.2 2/3] gitlab-ci: Add a job to cover the --without-default-devices config

[Paolo Bonzini]

Il mer 4 nov 2020, 03:27 Stefano Stabellini  ha
scritto:

> FYI I tried to build the latest QEMU on Alpine Linux 3.12 ARM64 and I
> get:
>
>   ninja: unknown tool 'query'
>
> Even after rebuilding ninja master by hand. Any ideas? I don't know much
> about ninja.
>

Are you sure you have ninja installed and not its clone samurai (yes, I am
serious)? I have contributed query support to samurai but it hasn't made it
to a release yet.

What is the output of "ninja -t list"?

Paolo


>
> So I gave up on that and I spinned up a Debian Buster x86 container for
> this build. That one got past the "ninja: unknown tool 'query'" error.
> The build completed without problems to the end.
>
> Either way I can't reproduce the build error above.

Re: [PULL 12/12] hw/misc/sifive_u_otp: Add backend drive support

[Green Wan]

OK, let me do it. I'll send it soon.

On Tue, Nov 3, 2020 at 11:53 PM Alistair Francis  wrote:
>
> On Mon, Nov 2, 2020 at 9:49 AM Peter Maydell  wrote:
> >
> > On Fri, 23 Oct 2020 at 16:27, Alistair Francis  
> > wrote:
> > >
> > > From: Green Wan 
> > >
> > > Add '-drive' support to OTP device. Allow users to assign a raw file
> > > as OTP image.
> >
> > Hi; Coverity reports some issues with this code (CID 1435959,
> > CID 1435960, CID 1435961). They're all basically the same thing:
> > in read, write and reset functions this code calls blk_pread()
> > or blk_pwrite() but doesn't check the return value, so if the
> > underlying file operation fails then the guest will be
> > returned garbage data or have its write thrown away without
> > either the guest or the user being warned about that.
>
> Green Wan are you able to send a patch to check the error value?
>
> Alistair
>
> >
> > > @@ -54,6 +57,16 @@ static uint64_t sifive_u_otp_read(void *opaque, hwaddr 
> > > addr, unsigned int size)
> > >  if ((s->pce & SIFIVE_U_OTP_PCE_EN) &&
> > >  (s->pdstb & SIFIVE_U_OTP_PDSTB_EN) &&
> > >  (s->ptrim & SIFIVE_U_OTP_PTRIM_EN)) {
> > > +
> > > +/* read from backend */
> > > +if (s->blk) {
> > > +int32_t buf;
> > > +
> > > +blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
> > > +  SIFIVE_U_OTP_FUSE_WORD);
> > > +return buf;
> > > +}
> > > +
> > >  return s->fuse[s->pa & SIFIVE_U_OTP_PA_MASK];
> > >  } else {
> > >  return 0xff;
> > > @@ -145,6 +158,12 @@ static void sifive_u_otp_write(void *opaque, hwaddr 
> > > addr,
> > >  /* write bit data */
> > >  SET_FUSEARRAY_BIT(s->fuse, s->pa, s->paio, s->pdin);
> > >
> > > +/* write to backend */
> > > +if (s->blk) {
> > > +blk_pwrite(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD,
> > > +   &s->fuse[s->pa], SIFIVE_U_OTP_FUSE_WORD, 0);
> > > +}
> > > +
> > >  /* update written bit */
> > >  SET_FUSEARRAY_BIT(s->fuse_wo, s->pa, s->paio, 
> > > WRITTEN_BIT_ON);
> > >  }
> > > @@ -168,16 +187,48 @@ static const MemoryRegionOps sifive_u_otp_ops = {
> >
> > >  static void sifive_u_otp_reset(DeviceState *dev)
> > > @@ -191,6 +242,20 @@ static void sifive_u_otp_reset(DeviceState *dev)
> > >  s->fuse[SIFIVE_U_OTP_SERIAL_ADDR] = s->serial;
> > >  s->fuse[SIFIVE_U_OTP_SERIAL_ADDR + 1] = ~(s->serial);
> > >
> > > +if (s->blk) {
> > > +/* Put serial number to backend as well*/
> > > +uint32_t serial_data;
> > > +int index = SIFIVE_U_OTP_SERIAL_ADDR;
> > > +
> > > +serial_data = s->serial;
> > > +blk_pwrite(s->blk, index * SIFIVE_U_OTP_FUSE_WORD,
> > > +   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0);
> > > +
> > > +serial_data = ~(s->serial);
> > > +blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
> > > +   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0);
> > > +}
> > > +
> > >  /* Initialize write-once map */
> > >  memset(s->fuse_wo, 0x00, sizeof(s->fuse_wo));
> > >  }
> > > --
> > > 2.28.0
> >
> > thanks
> > -- PMM

Re: [PATCH v2] qapi, qemu-options: make all parsing visitors parse boolean options the same

[Markus Armbruster]

Paolo Bonzini  writes:

> On 03/11/20 17:25, Daniel P. Berrangé wrote:
>>> OptsVisitor, StringInputVisitor and the keyval visitor have
>>> three different ideas of how a human could write the value of
>>> a boolean option.  Pay homage to the backwards-compatibility
>>> gods and make the new common helper accept all four sets (on/off,
>>> true/false, y/n and yes/no), and case-insensitive at that.
>>>
>>> Since OptsVisitor is supposed to match qemu-options, adjust
>>> it as well.
>> FWIW, libvirt does not appear to use true/false or y/n, nor
>> ever use uppercase / mixed case.
>> 
>> IOW this level of back compat may well be overkill.
>> 
>> I'd particular suggest deprecating case-insensitivity, as
>> Yes, YES, yEs feel unlikely to be important or widely used.
>
> True; at least it's type-safe code unlike the short-form boolean option.
>  It only hurts in the odd case of a boolean option becoming on/off/auto
> or on/off/split.

Another argument for deprecating values other than "on" and "off".

> I didn't want to introduce deprecation at this point, because

Not wanting to rush deprecation this close to the release is a valid
point.  So is not wanting to rush in additional sugar :)

> consistency is better anyway even if we plan to later deprecate
> something.  For example, since there is a common parser now, introducing
> deprecation would be much easier.  It also lets us switch parsers even
> during the deprecation period (which is how I got into this mess).

On the other hand, we'll have more to deprecate.  Status quo:


  on/off  yes/no y/n  true/false  case-sensitive?
qemu_opt_get_bool() X no
keyval visitor  X no
string visitor  XXX   yes
opts visitorXXX   no

For once, there is no way to blame QemuOpts ;)

qemu_opt_get_bool() is everywhere in CLI and HMP.

The keyval visitor is used for QAPIfied human-friendly interfaces: block
stuff, -display, -audio.

The string input visitor is used for -object, -global, HMP object_add,
qom-set, migrate_set_parameter.

The opts visitor is used for -mon and its sugared forms (new in 5.2!),
-net, -netdev, -acpitable, -numa.

I'd very much prefer to deprecate the odd ones in string and opts
visitor, and not copy them elsewhere.

We can still factor out a common parsing function if we like: pass a
flag that makes it recognize deprecated forms.  A single flag should
suffice; it lets odd ones spread from string to opts visitor and vice
versa, which I find tolerable.

Re: [PATCH-for-5.2 2/3] gitlab-ci: Add a job to cover the --without-default-devices config

[Philippe Mathieu-Daudé]

On 11/4/20 7:21 AM, Thomas Huth wrote:
> On 03/11/2020 21.41, Philippe Mathieu-Daudé wrote:
>> On 11/3/20 7:43 PM, Thomas Huth wrote:
>>> On 03/11/2020 17.46, Philippe Mathieu-Daudé wrote:
> [...]
 diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
 index 3b15ae5c302..6ee098ec53c 100644
 --- a/.gitlab-ci.yml
 +++ b/.gitlab-ci.yml
 @@ -262,6 +262,17 @@ build-user-plugins:
  MAKE_CHECK_ARGS: check-tcg
timeout: 1h 30m
  
 +build-system-ubuntu-without-default-devices:
 +  <<: *native_build_job_definition
 +  variables:
 +IMAGE: ubuntu2004
 +CONFIGURE_ARGS: --without-default-devices --disable-user 
 --disable-xen --disable-tools --disable-docs
 +MAKE_CHECK_ARGS: check-build
>>>
>>> AFAIK "check-build" is pretty much a no-op since the convertion to meson ...
>>> could you maybe replace with a set of qtest targets that work, to make sure
>>> that we do not regress here? E.g.:
>>>
>>> MAKE_CHECK_ARGS: check-qtest-avr check-qtestcris check-qtest-m68k
>>> check-qtest-microblaze check-qtest-mipsel check-qtest-moxie ...
>>
>> qtests don't work with --without-default-devices, as we don't check
>> for (un-)available devices.
> 
> Sure, "make check-qtest" does not work, I know. But some targets work fine,
> e.g. "make check-qtest-avr", and that's what I've suggested.

Yes, I tested that successfully yesterday late.

> By testing
> those targets, we can make sure that the qtests don't regress any further
> with --without-default-devices.

Yes, but I'm being wary to use it with the sole AVR target, because
I don't want this target development to be limited by unrelated
technical debts (in case we add optional device on an AVR board).

> 
>> I'll try check-unit.
> 
> I think that does not have much benefit since it should be independent of
> the devices and is tested in the other pipelines already?

Ah, good point.

Regards,

Phil.

[PATCH-for-5.2 v2 0/4] ci: Move --without-default-devices job from Travis to GitLab

[Philippe Mathieu-Daudé]

We have a job covering the --without-default-devices configure
option on Travis-CI, but recommend the community to use GitLab,
so build failures are missed.

We need help to move the jobs to GitLab (we will keep the s390x
and ppc64 containerized jobs on Travis as there is no similar
offer on GitLab). Start with this single job.

Since v1:
- Fix Xen+9pfs config (Paolo)
- Run AVR qtests (Thomas)

Cornelia Huck (1):
  s390x: fix build for --without-default-devices

Philippe Mathieu-Daudé (3):
  hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen
  gitlab-ci: Add a job to cover the --without-default-devices config
  travis-ci: Remove the --without-default-devices job

 include/hw/s390x/s390-pci-vfio.h | 3 ++-
 .gitlab-ci.yml   | 7 +++
 .travis.yml  | 8 
 hw/9pfs/Kconfig  | 4 
 hw/9pfs/meson.build  | 2 +-
 hw/s390x/meson.build | 2 +-
 6 files changed, 11 insertions(+), 15 deletions(-)

-- 
2.26.2

[PATCH-for-5.2 v2 2/4] hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen

[Philippe Mathieu-Daudé]

Fixes './configure --without-default-devices --enable-xen' build:

  /usr/bin/ld: libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function 
`xen_be_register_common':
  hw/xen/xen-legacy-backend.c:754: undefined reference to `xen_9pfs_ops'
  /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x8): undefined 
reference to `local_ops'
  /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x20): undefined 
reference to `synth_ops'
  /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x38): undefined 
reference to `proxy_ops'
  collect2: error: ld returned 1 exit status

Fixes: b2c00bce54c ("meson: convert hw/9pfs, cleanup")
Suggested-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
---
I'm not sure b2c00bce54c is the real culprit

Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Paul Durrant 
Cc: xen-de...@lists.xenproject.org
Cc: Greg Kurz 
Cc: Christian Schoenebeck 
---
 hw/9pfs/Kconfig | 4 
 hw/9pfs/meson.build | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/9pfs/Kconfig b/hw/9pfs/Kconfig
index d3ebd737301..3ae57496613 100644
--- a/hw/9pfs/Kconfig
+++ b/hw/9pfs/Kconfig
@@ -2,12 +2,8 @@ config FSDEV_9P
 bool
 depends on VIRTFS
 
-config 9PFS
-bool
-
 config VIRTIO_9P
 bool
 default y
 depends on VIRTFS && VIRTIO
 select FSDEV_9P
-select 9PFS
diff --git a/hw/9pfs/meson.build b/hw/9pfs/meson.build
index cc094262122..99be5d91196 100644
--- a/hw/9pfs/meson.build
+++ b/hw/9pfs/meson.build
@@ -15,6 +15,6 @@
   'coxattr.c',
 ))
 fs_ss.add(when: 'CONFIG_XEN', if_true: files('xen-9p-backend.c'))
-softmmu_ss.add_all(when: 'CONFIG_9PFS', if_true: fs_ss)
+softmmu_ss.add_all(when: 'CONFIG_FSDEV_9P', if_true: fs_ss)
 
 specific_ss.add(when: 'CONFIG_VIRTIO_9P', if_true: files('virtio-9p-device.c'))
-- 
2.26.2

[PATCH-for-5.2 v2 1/4] s390x: fix build for --without-default-devices

[Philippe Mathieu-Daudé]

From: Cornelia Huck 

s390-pci-vfio.c calls into the vfio code, so we need it to be
built conditionally on vfio (which implies CONFIG_LINUX).

Reported-by: Philippe Mathieu-Daudé 
Fixes: cd7498d07fbb ("s390x/pci: Add routine to get the vfio dma available 
count")
Signed-off-by: Cornelia Huck 
Message-Id: <20201103123237.718242-1-coh...@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/s390x/s390-pci-vfio.h | 3 ++-
 hw/s390x/meson.build | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/hw/s390x/s390-pci-vfio.h b/include/hw/s390x/s390-pci-vfio.h
index c7984905b3b..ff708aef500 100644
--- a/include/hw/s390x/s390-pci-vfio.h
+++ b/include/hw/s390x/s390-pci-vfio.h
@@ -13,8 +13,9 @@
 #define HW_S390_PCI_VFIO_H
 
 #include "hw/s390x/s390-pci-bus.h"
+#include CONFIG_DEVICES
 
-#ifdef CONFIG_LINUX
+#ifdef CONFIG_VFIO
 bool s390_pci_update_dma_avail(int fd, unsigned int *avail);
 S390PCIDMACount *s390_pci_start_dma_count(S390pciState *s,
   S390PCIBusDevice *pbdev);
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index f4663a83551..2a7818d94b9 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -27,7 +27,7 @@
 ))
 s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: 
files('s390-virtio-ccw.c'))
 s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
-s390x_ss.add(when: 'CONFIG_LINUX', if_true: files('s390-pci-vfio.c'))
+s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
 
 virtio_ss = ss.source_set()
 virtio_ss.add(files('virtio-ccw.c'))
-- 
2.26.2

[PATCH-for-5.2 v2 3/4] gitlab-ci: Add a job to cover the --without-default-devices config

[Philippe Mathieu-Daudé]

We test './configure --without-default-devices' since commit
20885b5b169 (".travis.yml: test that no-default-device builds
do not regress") in Travis-CI.

Since having a single CI to look at is easier, and GitLab-CI
is the preferred one, add the equivalent job there.

As smoke test, run the qtests on the AVR target. Since the
boards are simple SoC, there is not issue with unavailable
default devices there.

Signed-off-by: Philippe Mathieu-Daudé 
---
 .gitlab-ci.yml | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 3b15ae5c302..321cca2c216 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -262,6 +262,13 @@ build-user-plugins:
 MAKE_CHECK_ARGS: check-tcg
   timeout: 1h 30m
 
+build-system-ubuntu-without-default-devices:
+  <<: *native_build_job_definition
+  variables:
+IMAGE: ubuntu2004
+CONFIGURE_ARGS: --without-default-devices --disable-user --disable-tools 
--disable-docs
+MAKE_CHECK_ARGS: check-qtest-avr
+
 build-clang:
   <<: *native_build_job_definition
   variables:
-- 
2.26.2

Re: [PATCH-for-5.2 2/3] gitlab-ci: Add a job to cover the --without-default-devices config

[Philippe Mathieu-Daudé]

On 11/3/20 10:12 PM, Paolo Bonzini wrote:
> On 03/11/20 22:07, Philippe Mathieu-Daudé wrote:
>>> diff --git a/accel/Kconfig b/accel/Kconfig
>>> index 2ad94a3839..d24664d736 100644
>>> --- a/accel/Kconfig
>>> +++ b/accel/Kconfig
>>> @@ -7,3 +7,4 @@ config KVM
>>>   config XEN
>>>   bool
>>>   select FSDEV_9P if VIRTFS
>>> +    select 9PFS if VIRTFS
>> Without this line ^ it works! Thanks :*
> 
> Without which line?

Not adding "select 9PFS if VIRTFS" (keeping
the current "select FSDEV_9P if VIRTFS").

> 
> (Also if both work I prefer the other one).
> 
> Paolo
> 

[PATCH-for-5.2 v2 4/4] travis-ci: Remove the --without-default-devices job

[Philippe Mathieu-Daudé]

We replicated the --without-default-devices job on GitLab-CI
in the previous commit. We can now remove it from Travis-CI.

Reviewed-by: Thomas Huth 
Signed-off-by: Philippe Mathieu-Daudé 
---
 .travis.yml | 8 
 1 file changed, 8 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index a3d78171cab..15d92291358 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -224,14 +224,6 @@ jobs:
 - ${SRC_DIR}/scripts/travis/coverage-summary.sh
 
 
-# We manually include builds which we disable "make check" for
-- name: "GCC without-default-devices (softmmu)"
-  env:
-- CONFIG="--without-default-devices --disable-user"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
-- TEST_CMD=""
-
-
 # We don't need to exercise every backend with every front-end
 - name: "GCC trace log,simple,syslog (user)"
   env:
-- 
2.26.2

Re: [PATCH for-5.2 1/4] ivshmem-test: do not use short-form boolean option

[Markus Armbruster]

Paolo Bonzini  writes:

> This QemuOpts idiom will be deprecated, so get rid of it in the tests.
>
> Signed-off-by: Paolo Bonzini 
> ---
>  tests/qtest/ivshmem-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c
> index d5c8b9f128..dfa69424ed 100644
> --- a/tests/qtest/ivshmem-test.c
> +++ b/tests/qtest/ivshmem-test.c
> @@ -135,7 +135,7 @@ static void setup_vm_cmd(IVState *s, const char *cmd, 
> bool msix)
>  static void setup_vm(IVState *s)
>  {
>  char *cmd = g_strdup_printf("-object memory-backend-file"
> -",id=mb1,size=1M,share,mem-path=/dev/shm%s"
> +
> ",id=mb1,size=1M,share=on,mem-path=/dev/shm%s"
>  " -device ivshmem-plain,memdev=mb1", tmpshm);
>  
>  setup_vm_cmd(s, cmd, false);

Reviewed-by: Markus Armbruster 

Re: [PATCH-for-5.2 2/3] gitlab-ci: Add a job to cover the --without-default-devices config

[Thomas Huth]

On 04/11/2020 09.32, Philippe Mathieu-Daudé wrote:
> On 11/4/20 7:21 AM, Thomas Huth wrote:
>> On 03/11/2020 21.41, Philippe Mathieu-Daudé wrote:
>>> On 11/3/20 7:43 PM, Thomas Huth wrote:
 On 03/11/2020 17.46, Philippe Mathieu-Daudé wrote:
>> [...]
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 3b15ae5c302..6ee098ec53c 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -262,6 +262,17 @@ build-user-plugins:
>  MAKE_CHECK_ARGS: check-tcg
>timeout: 1h 30m
>  
> +build-system-ubuntu-without-default-devices:
> +  <<: *native_build_job_definition
> +  variables:
> +IMAGE: ubuntu2004
> +CONFIGURE_ARGS: --without-default-devices --disable-user 
> --disable-xen --disable-tools --disable-docs
> +MAKE_CHECK_ARGS: check-build

 AFAIK "check-build" is pretty much a no-op since the convertion to meson 
 ...
 could you maybe replace with a set of qtest targets that work, to make sure
 that we do not regress here? E.g.:

 MAKE_CHECK_ARGS: check-qtest-avr check-qtestcris check-qtest-m68k
 check-qtest-microblaze check-qtest-mipsel check-qtest-moxie ...
>>>
>>> qtests don't work with --without-default-devices, as we don't check
>>> for (un-)available devices.
>>
>> Sure, "make check-qtest" does not work, I know. But some targets work fine,
>> e.g. "make check-qtest-avr", and that's what I've suggested.
> 
> Yes, I tested that successfully yesterday late.
> 
>> By testing
>> those targets, we can make sure that the qtests don't regress any further
>> with --without-default-devices.
> 
> Yes, but I'm being wary to use it with the sole AVR target, because
> I don't want this target development to be limited by unrelated
> technical debts (in case we add optional device on an AVR board).

If you feel uncomfortable with check-qtest-avr, you could also use
check-qtest-m68k instead - I think that's fine for me (and hopefully also
Laurent).

 Thomas

Re: [PATCH-for-5.2 v2 3/4] gitlab-ci: Add a job to cover the --without-default-devices config

[Thomas Huth]

On 04/11/2020 09.43, Philippe Mathieu-Daudé wrote:
> We test './configure --without-default-devices' since commit
> 20885b5b169 (".travis.yml: test that no-default-device builds
> do not regress") in Travis-CI.
> 
> Since having a single CI to look at is easier, and GitLab-CI
> is the preferred one, add the equivalent job there.
> 
> As smoke test, run the qtests on the AVR target. Since the
> boards are simple SoC, there is not issue with unavailable
> default devices there.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  .gitlab-ci.yml | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 3b15ae5c302..321cca2c216 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -262,6 +262,13 @@ build-user-plugins:
>  MAKE_CHECK_ARGS: check-tcg
>timeout: 1h 30m
>  
> +build-system-ubuntu-without-default-devices:
> +  <<: *native_build_job_definition
> +  variables:
> +IMAGE: ubuntu2004
> +CONFIGURE_ARGS: --without-default-devices --disable-user --disable-tools 
> --disable-docs
> +MAKE_CHECK_ARGS: check-qtest-avr

As mentioned in my other mail, we can also use -m68k if you prefer that
instead of -avr.

>  build-clang:
><<: *native_build_job_definition
>variables:
> 

Reviewed-by: Thomas Huth 

[PATCH] hw/misc/sifive_u_otp: handling the fails of blk_pread and blk_pwrite

[Green Wan]

Fix code coverage issues by checking return value and handling fail case
of blk_pread() and blk_pwrite(). Return default value 0xff if read fails.

Signed-off-by: Green Wan 
---
 hw/misc/sifive_u_otp.c | 31 +++
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
index 60066375ab..4314727d0d 100644
--- a/hw/misc/sifive_u_otp.c
+++ b/hw/misc/sifive_u_otp.c
@@ -62,8 +62,13 @@ static uint64_t sifive_u_otp_read(void *opaque, hwaddr addr, 
unsigned int size)
 if (s->blk) {
 int32_t buf;
 
-blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
-  SIFIVE_U_OTP_FUSE_WORD);
+if (blk_pread(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD, &buf,
+  SIFIVE_U_OTP_FUSE_WORD) < 0) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "read error index<%d>\n", s->pa);
+return 0xff;
+}
+
 return buf;
 }
 
@@ -160,8 +165,12 @@ static void sifive_u_otp_write(void *opaque, hwaddr addr,
 
 /* write to backend */
 if (s->blk) {
-blk_pwrite(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD,
-   &s->fuse[s->pa], SIFIVE_U_OTP_FUSE_WORD, 0);
+if (blk_pwrite(s->blk, s->pa * SIFIVE_U_OTP_FUSE_WORD,
+   &s->fuse[s->pa], SIFIVE_U_OTP_FUSE_WORD,
+   0) < 0) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "write error index<%d>\n", s->pa);
+}
 }
 
 /* update written bit */
@@ -248,12 +257,18 @@ static void sifive_u_otp_reset(DeviceState *dev)
 int index = SIFIVE_U_OTP_SERIAL_ADDR;
 
 serial_data = s->serial;
-blk_pwrite(s->blk, index * SIFIVE_U_OTP_FUSE_WORD,
-   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0);
+if (blk_pwrite(s->blk, index * SIFIVE_U_OTP_FUSE_WORD,
+   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0) < 0) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "write error index<%d>\n", index);
+}
 
 serial_data = ~(s->serial);
-blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
-   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0);
+if (blk_pwrite(s->blk, (index + 1) * SIFIVE_U_OTP_FUSE_WORD,
+   &serial_data, SIFIVE_U_OTP_FUSE_WORD, 0) < 0) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "write error index<%d>\n", index + 1);
+}
 }
 
 /* Initialize write-once map */
-- 
2.17.1

Re: [RFC PATCH 0/6] eBPF RSS support for virtio-net

[Daniel P . Berrangé]

On Wed, Nov 04, 2020 at 10:07:52AM +0800, Jason Wang wrote:
> 
> On 2020/11/3 下午6:32, Yuri Benditovich wrote:
> > 
> > 
> > On Tue, Nov 3, 2020 at 11:02 AM Jason Wang  > > wrote:
> > 
> > 
> > On 2020/11/3 上午2:51, Andrew Melnychenko wrote:
> > > Basic idea is to use eBPF to calculate and steer packets in TAP.
> > > RSS(Receive Side Scaling) is used to distribute network packets
> > to guest virtqueues
> > > by calculating packet hash.
> > > eBPF RSS allows us to use RSS with vhost TAP.
> > >
> > > This set of patches introduces the usage of eBPF for packet steering
> > > and RSS hash calculation:
> > > * RSS(Receive Side Scaling) is used to distribute network packets to
> > > guest virtqueues by calculating packet hash
> > > * eBPF RSS suppose to be faster than already existing 'software'
> > > implementation in QEMU
> > > * Additionally adding support for the usage of RSS with vhost
> > >
> > > Supported kernels: 5.8+
> > >
> > > Implementation notes:
> > > Linux TAP TUNSETSTEERINGEBPF ioctl was used to set the eBPF program.
> > > Added eBPF support to qemu directly through a system call, see the
> > > bpf(2) for details.
> > > The eBPF program is part of the qemu and presented as an array
> > of bpf
> > > instructions.
> > > The program can be recompiled by provided Makefile.ebpf(need to
> > adjust
> > > 'linuxhdrs'),
> > > although it's not required to build QEMU with eBPF support.
> > > Added changes to virtio-net and vhost, primary eBPF RSS is used.
> > > 'Software' RSS used in the case of hash population and as a
> > fallback option.
> > > For vhost, the hash population feature is not reported to the guest.
> > >
> > > Please also see the documentation in PATCH 6/6.
> > >
> > > I am sending those patches as RFC to initiate the discussions
> > and get
> > > feedback on the following points:
> > > * Fallback when eBPF is not supported by the kernel
> > 
> > 
> > Yes, and it could also a lacking of CAP_BPF.
> > 
> > 
> > > * Live migration to the kernel that doesn't have eBPF support
> > 
> > 
> > Is there anything that we needs special treatment here?
> > 
> > Possible case: rss=on, vhost=on, source system with kernel 5.8
> > (everything works) -> dest. system 5.6 (bpf does not work), the adapter
> > functions, but all the steering does not use proper queues.
> 
> 
> Right, I think we need to disable vhost on dest.
> 
> 
> > 
> > 
> > 
> > > * Integration with current QEMU build
> > 
> > 
> > Yes, a question here:
> > 
> > 1) Any reason for not using libbpf, e.g it has been shipped with some
> > distros
> > 
> > 
> > We intentionally do not use libbpf, as it present only on some distros.
> > We can switch to libbpf, but this will disable bpf if libbpf is not
> > installed
> 
> 
> That's better I think.
> 
> 
> > 2) It would be better if we can avoid shipping bytecodes
> > 
> > 
> > 
> > This creates new dependencies: llvm + clang + ...
> > We would prefer byte code and ability to generate it if prerequisites
> > are installed.
> 
> 
> It's probably ok if we treat the bytecode as a kind of firmware.

That is explicitly *not* OK for inclusion in Fedora. They require that
BPF is compiled from source, and rejected my suggestion that it could
be considered a kind of firmware and thus have an exception from building
from source.

> But in the long run, it's still worthwhile consider the qemu source is used
> for development and llvm/clang should be a common requirement for generating
> eBPF bytecode for host.

So we need to do this right straight way before this merges.

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 18/30] hw/block/nvme: update nsid when registered

[Max Reitz]

On 27.10.20 11:49, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> If the user does not specify an nsid parameter on the nvme-ns device,
> nvme_register_namespace will find the first free namespace id and assign
> that.
> 
> This fix makes sure the assigned id is saved.
> 
> Signed-off-by: Klaus Jensen 
> Reviewed-by: Dmitry Fomichev 
> ---
>  hw/block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 5768a6804f41..2225b944f935 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -2578,7 +2578,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace 
> *ns, Error **errp)
>  for (int i = 1; i <= n->num_namespaces; i++) {
>  NvmeNamespace *ns = nvme_ns(n, i);
>  if (!ns) {
> -nsid = i;
> +nsid = ns->params.nsid = i;

Coverity reports that @ns is NULL here.  I think the problem is that we
want to access the *ns given to nvme_register_namespace() here, but it’s
shadowed by another @ns in this for () loop.

Max

>  break;
>  }
>  }
> 

[PATCH 1/2] MAINTAINERS: Cover exec-vary.c (variable page size)

[Philippe Mathieu-Daudé]

Add exec-vary.c to the 'Overall TCG CPUs' section.

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index dd16606bcdc..466898d3dbd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -117,6 +117,7 @@ R: Paolo Bonzini 
 S: Maintained
 F: softmmu/cpus.c
 F: cpus-common.c
+F: exec-vary.c
 F: accel/tcg/
 F: accel/stubs/tcg-stub.c
 F: scripts/decodetree.py
-- 
2.26.2

Re: [RFC PATCH 1/6] net: Added SetSteeringEBPF method for NetClientState.

[Yuri Benditovich]

On Wed, Nov 4, 2020 at 4:49 AM Jason Wang  wrote:

>
> On 2020/11/3 上午2:51, Andrew Melnychenko wrote:
> > From: Andrew 
> >
> > For now, that method supported only by Linux TAP.
> > Linux TAP uses TUNSETSTEERINGEBPF ioctl.
> > TUNSETSTEERINGBPF was added 3 years ago.
> > Qemu checks if it was defined before using.
> >
> > Signed-off-by: Andrew Melnychenko 
> > ---
> >   include/net/net.h |  2 ++
> >   net/tap-bsd.c |  5 +
> >   net/tap-linux.c   | 19 +++
> >   net/tap-solaris.c |  5 +
> >   net/tap-stub.c|  5 +
> >   net/tap.c |  9 +
> >   net/tap_int.h |  1 +
> >   7 files changed, 46 insertions(+)
> >
> > diff --git a/include/net/net.h b/include/net/net.h
> > index 897b2d7595..d8a41fb010 100644
> > --- a/include/net/net.h
> > +++ b/include/net/net.h
> > @@ -60,6 +60,7 @@ typedef int (SetVnetBE)(NetClientState *, bool);
> >   typedef struct SocketReadState SocketReadState;
> >   typedef void (SocketReadStateFinalize)(SocketReadState *rs);
> >   typedef void (NetAnnounce)(NetClientState *);
> > +typedef bool (SetSteeringEBPF)(NetClientState *, int);
> >
> >   typedef struct NetClientInfo {
> >   NetClientDriver type;
> > @@ -81,6 +82,7 @@ typedef struct NetClientInfo {
> >   SetVnetLE *set_vnet_le;
> >   SetVnetBE *set_vnet_be;
> >   NetAnnounce *announce;
> > +SetSteeringEBPF *set_steering_ebpf;
> >   } NetClientInfo;
> >
> >   struct NetClientState {
> > diff --git a/net/tap-bsd.c b/net/tap-bsd.c
> > index 77aaf674b1..4f64f31e98 100644
> > --- a/net/tap-bsd.c
> > +++ b/net/tap-bsd.c
> > @@ -259,3 +259,8 @@ int tap_fd_get_ifname(int fd, char *ifname)
> >   {
> >   return -1;
> >   }
> > +
> > +int tap_fd_set_steering_ebpf(int fd, int prog_fd)
> > +{
> > +return -1;
> > +}
> > diff --git a/net/tap-linux.c b/net/tap-linux.c
> > index b0635e9e32..196373019f 100644
> > --- a/net/tap-linux.c
> > +++ b/net/tap-linux.c
> > @@ -31,6 +31,7 @@
> >
> >   #include 
> >   #include 
> > +#include  /* TUNSETSTEERINGEBPF */
> >
> >   #include "qapi/error.h"
> >   #include "qemu/error-report.h"
> > @@ -316,3 +317,21 @@ int tap_fd_get_ifname(int fd, char *ifname)
> >   pstrcpy(ifname, sizeof(ifr.ifr_name), ifr.ifr_name);
> >   return 0;
> >   }
> > +
> > +int tap_fd_set_steering_ebpf(int fd, int prog_fd)
> > +{
> > +#ifdef TUNSETSTEERINGEBPF
>
>
> I'm not sure how much this can help.
>
> But looking at tap-linux.h, I wonder do we need to pull TUN/TAP uapi
> headers.
>
> Thanks
>

Agree, we just need to add this define to tap-linux.h


>
>
> > +if (ioctl(fd, TUNSETSTEERINGEBPF, (void *) &prog_fd) != 0) {
> > +error_report("Issue while setting TUNSETSTEERINGEBPF:"
> > +" %s with fd: %d, prog_fd: %d",
> > +strerror(errno), fd, prog_fd);
> > +
> > +   return -1;
> > +}
> > +
> > +return 0;
> > +#else
> > +error_report("TUNSETSTEERINGEBPF is not supported");
> > +return -1;
> > +#endif
> > +}
> > diff --git a/net/tap-solaris.c b/net/tap-solaris.c
> > index 0475a58207..d85224242b 100644
> > --- a/net/tap-solaris.c
> > +++ b/net/tap-solaris.c
> > @@ -255,3 +255,8 @@ int tap_fd_get_ifname(int fd, char *ifname)
> >   {
> >   return -1;
> >   }
> > +
> > +int tap_fd_set_steering_ebpf(int fd, int prog_fd)
> > +{
> > +return -1;
> > +}
> > diff --git a/net/tap-stub.c b/net/tap-stub.c
> > index de525a2e69..a0fa25804b 100644
> > --- a/net/tap-stub.c
> > +++ b/net/tap-stub.c
> > @@ -85,3 +85,8 @@ int tap_fd_get_ifname(int fd, char *ifname)
> >   {
> >   return -1;
> >   }
> > +
> > +int tap_fd_set_steering_ebpf(int fd, int prog_fd)
> > +{
> > +return -1;
> > +}
> > diff --git a/net/tap.c b/net/tap.c
> > index c46ff66184..81f50017bd 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -337,6 +337,14 @@ static void tap_poll(NetClientState *nc, bool
> enable)
> >   tap_write_poll(s, enable);
> >   }
> >
> > +static bool tap_set_steering_ebpf(NetClientState *nc, int prog_fd)
> > +{
> > +TAPState *s = DO_UPCAST(TAPState, nc, nc);
> > +assert(nc->info->type == NET_CLIENT_DRIVER_TAP);
> > +
> > +return tap_fd_set_steering_ebpf(s->fd, prog_fd) == 0;
> > +}
> > +
> >   int tap_get_fd(NetClientState *nc)
> >   {
> >   TAPState *s = DO_UPCAST(TAPState, nc, nc);
> > @@ -362,6 +370,7 @@ static NetClientInfo net_tap_info = {
> >   .set_vnet_hdr_len = tap_set_vnet_hdr_len,
> >   .set_vnet_le = tap_set_vnet_le,
> >   .set_vnet_be = tap_set_vnet_be,
> > +.set_steering_ebpf = tap_set_steering_ebpf,
> >   };
> >
> >   static TAPState *net_tap_fd_init(NetClientState *peer,
> > diff --git a/net/tap_int.h b/net/tap_int.h
> > index 225a49ea48..547f8a5a28 100644
> > --- a/net/tap_int.h
> > +++ b/net/tap_int.h
> > @@ -44,5 +44,6 @@ int tap_fd_set_vnet_be(int fd, int vnet_is_be);
> >   int tap_fd_enable(int fd);
> >   int tap_fd_disable(int fd);
> >   int tap_fd_get_ifname(int fd, char *ifname);
> > +int tap_fd_set_steering_ebpf(int fd, int prog_fd);

[PATCH 0/2] exec: Ensure variable page size is only used with TARGET_PAGE_BITS_VARY

[Philippe Mathieu-Daudé]

Simple patch while trying to figure out Fuloong-2E 16KB page size.
Better safe than sorry =)

Philippe Mathieu-Daudé (2):
  MAINTAINERS: Cover exec-vary.c (variable page size)
  exec: Ensure variable page size is only used with
TARGET_PAGE_BITS_VARY

 include/qemu-common.h | 4 +++-
 exec-vary.c   | 4 +++-
 MAINTAINERS   | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH 2/2] exec: Ensure variable page size is only used with TARGET_PAGE_BITS_VARY

[Philippe Mathieu-Daudé]

If TARGET_PAGE_BITS_VARY is not supported, machines should not
intent to modify the target page size.
As set_preferred_target_page_bits() is supposed to return 'false'
on failure (documented in "qemu-common.h"), return false to
indicate failure if this ever happens.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/qemu-common.h | 4 +++-
 exec-vary.c   | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index fda7dc6ca77..3ea616d4567 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -70,7 +70,9 @@ void cpu_exec_step_atomic(CPUState *cpu);
  * size may be smaller than any given CPU's preference).
  * Returns true on success, false on failure (which can only happen
  * if this is called after the system has already finalized its
- * choice of page size and the requested page size is smaller than that).
+ * choice of page size and the requested page size is smaller than
+ * that). Only target supporting variable page size should set a
+ * preferred target page size.
  */
 bool set_preferred_target_page_bits(int bits);
 
diff --git a/exec-vary.c b/exec-vary.c
index ff905f2a8fb..4b0b7f193af 100644
--- a/exec-vary.c
+++ b/exec-vary.c
@@ -86,8 +86,10 @@ bool set_preferred_target_page_bits(int bits)
 }
 init_target_page.bits = bits;
 }
-#endif
 return true;
+#else
+return false;
+#endif
 }
 
 void finalize_target_page_bits(void)
-- 
2.26.2

Re: [PATCH-for-5.2 v2 2/4] hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen

[Greg Kurz]

On Wed,  4 Nov 2020 09:43:25 +0100
Philippe Mathieu-Daudé  wrote:

> Fixes './configure --without-default-devices --enable-xen' build:
> 
>   /usr/bin/ld: libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function 
> `xen_be_register_common':
>   hw/xen/xen-legacy-backend.c:754: undefined reference to `xen_9pfs_ops'
>   /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x8): undefined 
> reference to `local_ops'
>   /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x20): 
> undefined reference to `synth_ops'
>   /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x38): 
> undefined reference to `proxy_ops'
>   collect2: error: ld returned 1 exit status
> 
> Fixes: b2c00bce54c ("meson: convert hw/9pfs, cleanup")
> Suggested-by: Paolo Bonzini 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> I'm not sure b2c00bce54c is the real culprit
> 

FWIW this commit introduced the 9PFS config which isn't used
anywhere. Backends depend on FSDEV_9P which itself depends
on VIRTFS. So I tend to think b2c00bce54c is the culprit
but _of couse_ I could be wrong :)

Anyway, this patch (+ patch 1) fix the build break mentioned
in the changelog so:

Acked-by: Greg Kurz 
Tested-by: Greg Kurz 

> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: xen-de...@lists.xenproject.org
> Cc: Greg Kurz 
> Cc: Christian Schoenebeck 
> ---
>  hw/9pfs/Kconfig | 4 
>  hw/9pfs/meson.build | 2 +-
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/hw/9pfs/Kconfig b/hw/9pfs/Kconfig
> index d3ebd737301..3ae57496613 100644
> --- a/hw/9pfs/Kconfig
> +++ b/hw/9pfs/Kconfig
> @@ -2,12 +2,8 @@ config FSDEV_9P
>  bool
>  depends on VIRTFS
>  
> -config 9PFS
> -bool
> -
>  config VIRTIO_9P
>  bool
>  default y
>  depends on VIRTFS && VIRTIO
>  select FSDEV_9P
> -select 9PFS
> diff --git a/hw/9pfs/meson.build b/hw/9pfs/meson.build
> index cc094262122..99be5d91196 100644
> --- a/hw/9pfs/meson.build
> +++ b/hw/9pfs/meson.build
> @@ -15,6 +15,6 @@
>'coxattr.c',
>  ))
>  fs_ss.add(when: 'CONFIG_XEN', if_true: files('xen-9p-backend.c'))
> -softmmu_ss.add_all(when: 'CONFIG_9PFS', if_true: fs_ss)
> +softmmu_ss.add_all(when: 'CONFIG_FSDEV_9P', if_true: fs_ss)
>  
>  specific_ss.add(when: 'CONFIG_VIRTIO_9P', if_true: 
> files('virtio-9p-device.c'))

Re: [PULL 12/30] hw/block/nvme: add support for scatter gather lists

[Max Reitz]

On 27.10.20 11:49, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> For now, support the Data Block, Segment and Last Segment descriptor
> types.
> 
> See NVM Express 1.3d, Section 4.4 ("Scatter Gather List (SGL)").
> 
> Signed-off-by: Klaus Jensen 
> Reviewed-by: Keith Busch 
> ---
>  include/block/nvme.h  |   6 +-
>  hw/block/nvme.c   | 329 ++
>  hw/block/trace-events |   4 +
>  3 files changed, 279 insertions(+), 60 deletions(-)

[...]

> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index c0f1f8ccd473..63d0a17bc5f0 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -413,13 +413,262 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t 
> prp1, uint64_t prp2,

[...]

> +if (*len == 0) {
> +/*
> + * All data has been mapped, but the SGL contains additional
> + * segments and/or descriptors. The controller might accept
> + * ignoring the rest of the SGL.
> + */
> +uint16_t sgls = le16_to_cpu(n->id_ctrl.sgls);
> +if (sgls & NVME_CTRL_SGLS_EXCESS_LENGTH) {

Hi,

Coverity reports that this condition is always false because
NVME_CTRL_SGLS_EXCESS_LENGTH is (1 << 18) and thus won’t fit in a uint16_t.

I think the problem is that n->id_ctrl.sgls is a uint32_t, so sgls
should be uint32_t and we should use le32_to_cpu().  (Which technically
is a second problem, namely to access a 32 bit LE field as a 16 bit
field.  That will give wrong results on big endian hosts.)

Max

Re: [qemu-web PATCH] Add virtio-fs in OSv overview blog post

[Thomas Huth]

On 04/11/2020 01.42, Fotis Xenakis wrote:
> This post briefly goes over the main points of virtio-fs and OSv, a
> unikernel running under QEMU/KVM and taking advantage of its virtio-fs
> implementation.
> 
> Feel free to review, I will be more than happy to address any comments.

Thanks, articles for the QEMU blog are always welcome! Some comments below...

> diff --git a/_posts/2020-11-04-osv-virtio-fs.md 
> b/_posts/2020-11-04-osv-virtio-fs.md
> new file mode 100644
> index 000..af5bb0d
> --- /dev/null
> +++ b/_posts/2020-11-04-osv-virtio-fs.md
> @@ -0,0 +1,123 @@
> +---
> +layout: post
> +title:  "Using virtio-fs on a unikernel"
> +author: Fotis Xenakis
> +date:   2020-11-04 02:00:00 +0200
> +categories: [storage, virtio-fs, unikernel, OSv]
> +---
> +
> +This article provides an overview of virtio-fs, a novel way for sharing the 
> host
> +file system with guests and OSv, a specialized, light-weight operating system
> +(unikernel) for the cloud, as well as how these two fit together.

I'd maybe add the links to the virtio-fs and OSv homepage in this paragraph
already, since they are mentioned here for the first time.
For links that you want to use multiple times, you can also declare them at
the end of the file, so that you only have to provide the URL one time. See
e.g. this article as an example:

https://gitlab.com/qemu-project/qemu-web/-/blob/master/_posts/2017-11-22-haxm-usage-windows.md#L101

> +## virtio-fs
> +
> +[Virtio-fs](https://virtio-fs.gitlab.io/) is a new host-guest shared 
> filesystem,
> +purpose-built for local file system semantics and performance. To that end, 
> it
> +takes full advantage of the host's and the guest's colocation on the same
> +physical machine, unlike network-based efforts, like virtio-9p.
> +
> +As the name suggests, virtio-fs builds on virtio for providing an efficient
> +transport: it is included in the (currently draft, to become v1.2) virtio
> +[specification](https://github.com/oasis-tcs/virtio-spec) as a new device. 
> The
> +protocol used by the device is a slightly extended version of
> +[FUSE](https://github.com/libfuse/libfuse), providing a solid foundation for
> +all file system operations native on Linux. Implementation-wise, on the QEMU
> +side, it takes the approach of splitting between the guest interface (handled
> +by QEMU) and the host file system interface (the device "backend"). The 
> latter
> +is handled by virtiofsd ("virtio-fs daemon"), running as a separate process,
> +utilizing the
> +[vhost-user](https://www.qemu.org/docs/master/interop/vhost-user.html) 
> protocol
> +to communicate with QEMU.
> +
> +One prominent performance feature of virtio-fs is the DAX (Direct Access)
> +window. What is that? It's a shared memory window between the host and the

I'd rather simply say "This is a shared ..." and scratch the "What is that?"
question.

> +guest, exposed as device memory (a PCI BAR) to the second. How is it used? 
> Upon

I'd rather drop the "How is it used?" question.

> +request, the host (QEMU) maps file contents to the window for the guest to
> +access directly. This bears performance gains due to taking VMEXITs out of 
> the
> +read/write data path and bypassing the guest page cache on Linux, while not
> +counting against the VM's memory (since it's just device memory, managed on 
> the
> +host).
> +
> +![virtio-fs DAX 
> architecture](https://gitlab.com/virtio-fs/virtio-fs.gitlab.io/-/raw/master/architecture.svg)
> +
> +Virtio-fs is under active development, with its community focussing on a 
> pair of
> +device implementation in QEMU and device driver in Linux. Both components are
> +already available upstream in their initial iterations, while upstreaming
> +continues further e.g. with DAX window support.
> +
> +## OSv
> +
> +[OSv](https://github.com/cloudius-systems/osv) is a
> +[unikernel](https://en.wikipedia.org/wiki/Unikernel) (framework). The two
> +defining characteristics of a unikernel are:
> +
> +- **Application-specialized**: a unikernel is an executable machine image,
> +  consisting of an application and supporting code (drivers, memory 
> management,
> +  runtime etc.) linked together, running in a single address space (typically
> +  in guest "kernel mode").
> +- **Library OS**: each unikernel only contains the functionality mandated by 
> its
> +  application in terms of non-application code, i.e. no unused drivers, or 
> even
> +  whole subsystems (e.g. networking, if the application doesn't use the
> +  network).
> +
> +OSv in particular strives for binary compatibility with Linux. What this 
> means

I'd rather say "This means that ..." instead.

> +is that applications built for Linux should run as OSv unikernels without
> +requiring modifications or even rebuilding, at least most of the time.

Sorry for asking ignorant questions: Does that mean that OSv has a dynamic
linker for Linux ELF binaries?

> Of
> +course, not the whole Linux ABI is supported, with system calls like `fork()`
> +and relatives missi

Re: [PATCH v2 00/11] qapi: static typing conversion, pt2

[Marc-André Lureau]

Hi

On Mon, Nov 2, 2020 at 7:41 PM John Snow  wrote:

> On 10/26/20 3:42 PM, John Snow wrote:
> > Hi, this series adds static type hints to the QAPI module.
> > This is part two, and covers introspect.py.
> >
> > Part 2: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2
> > Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6
> >
> > - Requires Python 3.6+
> > - Requires mypy 0.770 or newer (for type analysis only)
> > - Requires pylint 2.6.0 or newer (for lint checking only)
> >
> > Type hints are added in patches that add *only* type hints and change no
> > other behavior. Any necessary changes to behavior to accommodate typing
> > are split out into their own tiny patches.
> >
> > Every commit should pass with:
> >   - flake8 qapi/
> >   - pylint --rcfile=qapi/pylintrc qapi/
> >   - mypy --config-file=qapi/mypy.ini qapi/
> >
> > V2:
> >   - Dropped all R-B from previous series; enough has changed.
> >   - pt2 is now introspect.py, expr.py is pushed to pt3.
> >   - Reworked again to have less confusing (?) type names
> >   - Added an assertion to prevent future accidental breakage
> >
>
> Ping!
>
> Patches 1-3: Can be skipped; just enables sphinx to check the docstring
> syntax. Don't worry about these too much, they're just here for you to
> test with.
>

They are interesting, but the rebase version fails. And the error produced
is not exactly friendly:
Exception occurred:
  File "/usr/lib/python3.9/site-packages/sphinx/domains/c.py", line 3751,
in resolve_any_xref
return [('c:' + self.role_for_objtype(objtype), retnode)]
TypeError: can only concatenate str (not "NoneType") to str

Could you rebase and split off in a separate series?

Patch 4 adds some small changes, to support:
> Patch 5 adds the type hints.
> Patches 6-11 try to improve the readability of the types and the code.
>
> This was a challenging file to clean up, so I am sure there's lots of
> easy, low-hanging fruit in the review/feedback for me to improve.
>

Nothing obvious to me.

Python typing is fairly new to me, and I don't know the best practices. I
would just take what you did and improve later, if needed.

A wish item before we proceed with more python code cleanups is
documentation and/or automated tests.

Could you add a new make check-python and perhaps document what the new
code-style expectations?


> > John Snow (11):
> >[DO-NOT-MERGE] docs: replace single backtick (`) with double-backtick
> >  (``)
> >[DO-NOT-MERGE] docs/sphinx: change default role to "any"
> >[DO-NOT-MERGE] docs: enable sphinx-autodoc for scripts/qapi
> >qapi/introspect.py: add assertions and casts
> >qapi/introspect.py: add preliminary type hint annotations
> >qapi/introspect.py: add _gen_features helper
> >qapi/introspect.py: Unify return type of _make_tree()
> >qapi/introspect.py: replace 'extra' dict with 'comment' argument
> >qapi/introspect.py: create a typed 'Annotated' data strutcure
> >qapi/introspect.py: improve readability of _tree_to_qlit
> >qapi/introspect.py: Add docstring to _tree_to_qlit
> >
> >   docs/conf.py   |   6 +-
> >   docs/devel/build-system.rst| 120 +--
> >   docs/devel/index.rst   |   1 +
> >   docs/devel/migration.rst   |  59 +++---
> >   docs/devel/python/index.rst|   7 +
> >   docs/devel/python/qapi.commands.rst|   7 +
> >   docs/devel/python/qapi.common.rst  |   7 +
> >   docs/devel/python/qapi.error.rst   |   7 +
> >   docs/devel/python/qapi.events.rst  |   7 +
> >   docs/devel/python/qapi.expr.rst|   7 +
> >   docs/devel/python/qapi.gen.rst |   7 +
> >   docs/devel/python/qapi.introspect.rst  |   7 +
> >   docs/devel/python/qapi.main.rst|   7 +
> >   docs/devel/python/qapi.parser.rst  |   8 +
> >   docs/devel/python/qapi.rst |  26 +++
> >   docs/devel/python/qapi.schema.rst  |   7 +
> >   docs/devel/python/qapi.source.rst  |   7 +
> >   docs/devel/python/qapi.types.rst   |   7 +
> >   docs/devel/python/qapi.visit.rst   |   7 +
> >   docs/devel/tcg-plugins.rst |  14 +-
> >   docs/devel/testing.rst |   2 +-
> >   docs/interop/live-block-operations.rst |   4 +-
> >   docs/system/arm/cpu-features.rst   | 110 +-
> >   docs/system/arm/nuvoton.rst|   2 +-
> >   docs/system/s390x/protvirt.rst |  10 +-
> >   qapi/block-core.json   |   4 +-
> >   scripts/qapi/introspect.py | 277 +
> >   scripts/qapi/mypy.ini  |   5 -
> >   scripts/qapi/schema.py |   2 +-
> >   29 files changed, 487 insertions(+), 254 deletions(-)
> >   create mode 100644 docs/devel/python/index.rst
> >   create mode 100644 docs/devel/python/qapi.commands.rst
> >   create mode 100644 docs/devel/python/qapi.common.rst
> >   create mode 100644 docs/devel/python/qapi.error.rst
> >   create mode 1

Re: [PULL 18/30] hw/block/nvme: update nsid when registered

[Klaus Jensen]

On Nov  4 10:32, Max Reitz wrote:
> On 27.10.20 11:49, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > If the user does not specify an nsid parameter on the nvme-ns device,
> > nvme_register_namespace will find the first free namespace id and assign
> > that.
> > 
> > This fix makes sure the assigned id is saved.
> > 
> > Signed-off-by: Klaus Jensen 
> > Reviewed-by: Dmitry Fomichev 
> > ---
> >  hw/block/nvme.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 5768a6804f41..2225b944f935 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -2578,7 +2578,7 @@ int nvme_register_namespace(NvmeCtrl *n, 
> > NvmeNamespace *ns, Error **errp)
> >  for (int i = 1; i <= n->num_namespaces; i++) {
> >  NvmeNamespace *ns = nvme_ns(n, i);
> >  if (!ns) {
> > -nsid = i;
> > +nsid = ns->params.nsid = i;
> 
> Coverity reports that @ns is NULL here.  I think the problem is that we
> want to access the *ns given to nvme_register_namespace() here, but it’s
> shadowed by another @ns in this for () loop.
> 

Sure enough. Thanks!

I'll send a fix.


signature.asc
Description: PGP signature

Re: [PULL 48/48] hw/timer/armv7m_systick: Rewrite to use ptimers

[Andrew Jones]

On Tue, Oct 27, 2020 at 11:44:38AM +, Peter Maydell wrote:
> The armv7m systick timer is a 24-bit decrementing, wrap-on-zero,
> clear-on-write counter. Our current implementation has various
> bugs and dubious workarounds in it (for instance see
> https://bugs.launchpad.net/qemu/+bug/1872237).
> 
> We have an implementation of a simple decrementing counter
> and we put a lot of effort into making sure it handles the
> interesting corner cases (like "spend a cycle at 0 before
> reloading") -- ptimer.
> 
> Rewrite the systick timer to use a ptimer rather than
> a raw QEMU timer.
> 
> Unfortunately this is a migration compatibility break,
> which will affect all M-profile boards.
> 
> Among other bugs, this fixes
> https://bugs.launchpad.net/qemu/+bug/1872237 :
> now writes to SYST_CVR when the timer is enabled correctly
> do nothing; when the timer is enabled via SYST_CSR.ENABLE,
> the ptimer code will (because of POLICY_NO_IMMEDIATE_RELOAD)
> arrange that after one timer tick the counter is reloaded
> from SYST_RVR and then counts down from there, as the
> architecture requires.
> 
> Signed-off-by: Peter Maydell 
> Reviewed-by: Philippe Mathieu-Daudé 
> Message-id: 20201015151829.14656-3-peter.mayd...@linaro.org
> ---
>  include/hw/timer/armv7m_systick.h |   3 +-
>  hw/timer/armv7m_systick.c | 124 +-
>  2 files changed, 54 insertions(+), 73 deletions(-)
>

Do we also need something like the diff below now?


diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index fdf4464b9484..7d5d89e1acf9 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -291,6 +291,7 @@ config ZYNQ
 
 config ARM_V7M
 bool
+select PTIMER
 
 config ALLWINNER_A10
 bool


Thanks,
drew

Re: [PULL 15/30] hw/block/nvme: support multiple namespaces

[Max Reitz]

On 27.10.20 11:49, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> This adds support for multiple namespaces by introducing a new 'nvme-ns'
> device model. The nvme device creates a bus named from the device name
> ('id'). The nvme-ns devices then connect to this and registers
> themselves with the nvme device.
> 
> This changes how an nvme device is created. Example with two namespaces:
> 
>   -drive file=nvme0n1.img,if=none,id=disk1
>   -drive file=nvme0n2.img,if=none,id=disk2
>   -device nvme,serial=deadbeef,id=nvme0
>   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
>   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
> 
> The drive property is kept on the nvme device to keep the change
> backward compatible, but the property is now optional. Specifying a
> drive for the nvme device will always create the namespace with nsid 1.
> 
> Signed-off-by: Klaus Jensen 
> Reviewed-by: Keith Busch 
> Reviewed-by: Minwoo Im 
> ---
>  hw/block/nvme-ns.h|  74 +
>  hw/block/nvme.h   |  46 
>  hw/block/nvme-ns.c| 167 
>  hw/block/nvme.c   | 249 +++---
>  hw/block/meson.build  |   2 +-
>  hw/block/trace-events |   6 +-
>  6 files changed, 428 insertions(+), 116 deletions(-)
>  create mode 100644 hw/block/nvme-ns.h
>  create mode 100644 hw/block/nvme-ns.c

Hi,

Coverity reports that nvme_exit() still contains a

g_free(n->namespaces);

call, which becomes wrong with this commit (because @namespaces is now
an array).

(This leads to qemu crashing when deleting an NVMe device.)

Max

Re: [PULL 48/48] hw/timer/armv7m_systick: Rewrite to use ptimers

[Philippe Mathieu-Daudé]

On 11/4/20 11:03 AM, Andrew Jones wrote:
> On Tue, Oct 27, 2020 at 11:44:38AM +, Peter Maydell wrote:
>> The armv7m systick timer is a 24-bit decrementing, wrap-on-zero,
>> clear-on-write counter. Our current implementation has various
>> bugs and dubious workarounds in it (for instance see
>> https://bugs.launchpad.net/qemu/+bug/1872237).
>>
>> We have an implementation of a simple decrementing counter
>> and we put a lot of effort into making sure it handles the
>> interesting corner cases (like "spend a cycle at 0 before
>> reloading") -- ptimer.
>>
>> Rewrite the systick timer to use a ptimer rather than
>> a raw QEMU timer.
>>
>> Unfortunately this is a migration compatibility break,
>> which will affect all M-profile boards.
>>
>> Among other bugs, this fixes
>> https://bugs.launchpad.net/qemu/+bug/1872237 :
>> now writes to SYST_CVR when the timer is enabled correctly
>> do nothing; when the timer is enabled via SYST_CSR.ENABLE,
>> the ptimer code will (because of POLICY_NO_IMMEDIATE_RELOAD)
>> arrange that after one timer tick the counter is reloaded
>> from SYST_RVR and then counts down from there, as the
>> architecture requires.
>>
>> Signed-off-by: Peter Maydell 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Message-id: 20201015151829.14656-3-peter.mayd...@linaro.org
>> ---
>>  include/hw/timer/armv7m_systick.h |   3 +-
>>  hw/timer/armv7m_systick.c | 124 +-
>>  2 files changed, 54 insertions(+), 73 deletions(-)
>>
> 
> Do we also need something like the diff below now?
> 
> 
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index fdf4464b9484..7d5d89e1acf9 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -291,6 +291,7 @@ config ZYNQ
>  
>  config ARM_V7M
>  bool
> +select PTIMER

Oops yes indeed.

>  
>  config ALLWINNER_A10
>  bool
> 
> 
> Thanks,
> drew
> 
> 

Re: VFIO Migration

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefa...@redhat.com) wrote:
> On Tue, Nov 03, 2020 at 06:49:51PM +, Dr. David Alan Gilbert wrote:
> > * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > > On Tue, Nov 03, 2020 at 12:17:09PM +, Dr. David Alan Gilbert wrote:
> > > > * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > > > > Device Models
> > > > > -
> > > > > Devices have a *hardware interface* consisting of hardware registers,
> > > > > interrupts, and so on.
> > > > > 
> > > > > The hardware interface together with the device state representation 
> > > > > is called
> > > > > a *device model*. Device models can be assigned URIs such as
> > > > > https://qemu.org/devices/e1000e to uniquely identify them.
> > > > 
> > > > I think this is a unique identifier, not actually a URI; the https://
> > > > isn't needed since no one expects to ever connect to this.
> > > 
> > > Yes, it could be any unique string. If the URI idea is not popular we
> > > can use any similar scheme.
> > 
> > I'm OK with it being a URI; just drop the https.
> 
> Okay.
> 
> > > > > However, secondary aspects related to the physical port may affect 
> > > > > the device's
> > > > > hardware interface and need to be reflected in the device 
> > > > > configuration. The
> > > > > link speed may depend on the physical port and be reported through 
> > > > > the device's
> > > > > hardware interface. In that case a ``link-speed`` configuration 
> > > > > parameter is
> > > > > required to prevent unexpected changes to the link speed after 
> > > > > migration.
> > > > 
> > > > That's an interesting example; because depending on the device, it might
> > > > be:
> > > > a) Completely virtualised so that the guest *shouldn't* know what
> > > > the physical link speed is, precisely to allow the physical network on
> > > > the destination to be different.
> > > > 
> > > > b) Part of the migrated state
> > > > 
> > > > c) Something that's allowed to be reloaded after migration
> > > > 
> > > > d) Configurable
> > > > 
> > > > so I'm not sure whether it's a good example in this case or not.
> > > 
> > > Can you think of an example that has only one option?
> > > 
> > > I tried but couldn't. For example take a sound card. The guest is aware
> > > the device supports stereo playback (2 output channels), but which exact
> > > stereo host device is used doesn't matter, they are all suitable.
> > > 
> > > Now imagine migrating to a 7.1 surround-sound device. Similar options
> > > come into play:
> > > 
> > > a) Emulate stereo and mix it to 7.1 surround-sound on the physical
> > >device. The guest still sees the stereo device.
> > > 
> > > b) Refuse migration.
> > > 
> > > c) Indicate that the output has switched and let the guest reconfigure
> > >itself (e.g. a sound card with multiple outputs, where one of them is
> > >stereo and another is 7.1 surround sound).
> > > 
> > > Which option is desirable depends on the use case.
> > 
> > Yes, but I think it might be worth calling out these differences;  there
> > are explicitly cases where you don't want external changes to be visible
> > and other cases where you do; both are valid, but both need thinking
> > about. (Another one, GPU whether you have a monitor plugged in!)
> 
> Okay.
> 
> > > > Maybe what's needed is a stronger instruction to abstract external
> > > > device state so that it's not part of the configuration in most cases.
> > > 
> > > Do you want to propose something?
> > 
> > I think something like 'Some part of a devices state may be irrelevant
> > to a migration; for example on some NICs it might be preferable to hide
> > the physical characteristics of the link from the guest.'
> 
> Got it.
> 
> > > > > For example, if address filtering support was added to a network card 
> > > > > then
> > > > > device versions and the corresponding configurations may look like 
> > > > > this:
> > > > > * ``version=1`` - Behaves as if ``rx-filter-size=0``
> > > > > * ``version=2`` - ``rx-filter-size=32``
> > > > 
> > > > Note configuration parameters might have been added during the life of
> > > > the device; e.g. if the original card had no support for rx-filters, it
> > > > might not have a rx-filter-size parameter.
> > > 
> > > version=1 does not explicitly set rx-filter-size=0. When a new parameter
> > > is introduced it must have a default value that disables its effect on
> > > the hardware interface and/or device state representation. This is
> > > described in a bit more detail in the next section, maybe it should be
> > > reordered.
> > 
> > We've generally found the definition of devices tends in practice to be
> > done newer->older; i.e. you define the current machine, and then define
> > the next older machine setting the defaults that used to be true; then
> > define the older version behind that
> 
> That is not possible here because an older device implementation is
> unaware of new configuration parameters.
> 
> Looking at the example above, imagine 

Re: [PATCH] target/microblaze: Fix possible array out of bounds in mmu_write()

[Thomas Huth]

On 03/11/2020 08.46, AlexChen wrote:
> The size of env->mmu.regs is 3, but the range of 'rn' is [0, 5].
> To avoid data access out of bounds, only if 'rn' is less than 3, we
> can print env->mmu.regs[rn]. In other cases, we can print
> env->mmu.regs[MMU_R_TLBX].

... since env->mmu.regs[MMU_R_TLBX] is used in the other cases in this
function. Makes sense, indeed.

> Reported-by: Euler Robot 
> Signed-off-by: Alex Chen 
> ---
>  target/microblaze/mmu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c
> index 1dbbb271c4..917ad6d69e 100644
> --- a/target/microblaze/mmu.c
> +++ b/target/microblaze/mmu.c
> @@ -234,7 +234,8 @@ void mmu_write(CPUMBState *env, bool ext, uint32_t rn, 
> uint32_t v)
>  unsigned int i;
> 
>  qemu_log_mask(CPU_LOG_MMU,
> -  "%s rn=%d=%x old=%x\n", __func__, rn, v, 
> env->mmu.regs[rn]);
> +  "%s rn=%d=%x old=%x\n", __func__, rn, v,
> +  rn < 3 ? env->mmu.regs[rn] : env->mmu.regs[MMU_R_TLBX]);
> 
>  if (cpu->cfg.mmu < 2 || !cpu->cfg.mmu_tlb_access) {
>  qemu_log_mask(LOG_GUEST_ERROR, "MMU access on MMU-less system\n");
> 

Reviewed-by: Thomas Huth 

[PATCH 1/4] bsd-user: space required after semicolon

[shiliyang]

This patch fixes error style problems found by checkpatch.pl:
ERROR: space required after that ';'

Signed-off-by: Liyang Shi 

---
 bsd-user/elfload.c | 2 +-
 bsd-user/syscall.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index 9f4210af9a..25e625d86b 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -1227,7 +1227,7 @@ int load_elf_binary(struct linux_binprm *bprm, struct 
target_pt_regs *regs,
 end_data = 0;
 interp_ex.a_info = 0;

-for(i=0;i < elf_ex.e_phnum; i++) {
+for(i=0; i < elf_ex.e_phnum; i++) {
 if (elf_ppnt->p_type == PT_INTERP) {
 if ( elf_interpreter != NULL )
 {
diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
index 9b471b665c..b178d87de1 100644
--- a/bsd-user/syscall.c
+++ b/bsd-user/syscall.c
@@ -271,7 +271,7 @@ static abi_long lock_iovec(int type, struct iovec *vec, 
abi_ulong target_addr,
 target_vec = lock_user(VERIFY_READ, target_addr, count * sizeof(struct 
target_iovec), 1);
 if (!target_vec)
 return -TARGET_EFAULT;
-for(i = 0;i < count; i++) {
+for(i = 0; i < count; i++) {
 base = tswapl(target_vec[i].iov_base);
 vec[i].iov_len = tswapl(target_vec[i].iov_len);
 if (vec[i].iov_len != 0) {
@@ -297,7 +297,7 @@ static abi_long unlock_iovec(struct iovec *vec, abi_ulong 
target_addr,
 target_vec = lock_user(VERIFY_READ, target_addr, count * sizeof(struct 
target_iovec), 1);
 if (!target_vec)
 return -TARGET_EFAULT;
-for(i = 0;i < count; i++) {
+for(i = 0; i < count; i++) {
 if (target_vec[i].iov_base) {
 base = tswapl(target_vec[i].iov_base);
 unlock_user(vec[i].iov_base, base, copy ? vec[i].iov_len : 0);
-- 
2.29.1.59.gf9b6481aed

[PATCH 4/4] bsd-user: suspect code indent for conditional statements

[shiliyang]

This patch fixes error style problems found by checkpatch.pl:
ERROR: suspect code indent for conditional statements

Signed-off-by: Liyang Shi 

---
 bsd-user/elfload.c | 10 +-
 bsd-user/main.c|  4 ++--
 bsd-user/syscall.c |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index d5746f25e7..9f4210af9a 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -184,7 +184,7 @@ static inline void init_thread(struct target_pt_regs *regs, 
struct image_info *i
 memset(regs, 0, sizeof(*regs));
 regs->ARM_cpsr = 0x10;
 if (infop->entry & 1)
-  regs->ARM_cpsr |= CPSR_T;
+regs->ARM_cpsr |= CPSR_T;
 regs->ARM_pc = infop->entry & 0xfffe;
 regs->ARM_sp = infop->start_stack;
 /* FIXME - what to for failure of get_user()? */
@@ -784,7 +784,7 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, 
int envc,
 sp = sp &~ (abi_ulong)15;
 size = (DLINFO_ITEMS + 1) * 2;
 if (k_platform)
-  size += 2;
+size += 2;
 #ifdef DLINFO_ARCH_ITEMS
 size += DLINFO_ARCH_ITEMS * 2;
 #endif
@@ -871,7 +871,7 @@ static abi_ulong load_elf_interp(struct elfhdr 
*interp_elf_ex,
 malloc(sizeof(struct elf_phdr) * interp_elf_ex->e_phnum);

 if (!elf_phdata)
-  return ~((abi_ulong)0UL);
+return ~((abi_ulong)0UL);

 /*
  * If the size of this structure has changed, then punt, since
@@ -1267,7 +1267,7 @@ int load_elf_binary(struct linux_binprm *bprm, struct 
target_pt_regs *regs,

 if (strcmp(elf_interpreter,"/usr/lib/libc.so.1") == 0 ||
 strcmp(elf_interpreter,"/usr/lib/ld.so.1") == 0) {
-  ibcs2_interpreter = 1;
+ibcs2_interpreter = 1;
 }

 #if 0
@@ -1314,7 +1314,7 @@ int load_elf_binary(struct linux_binprm *bprm, struct 
target_pt_regs *regs,
 /* Now figure out which format our binary is */
 if ((N_MAGIC(interp_ex) != OMAGIC) && (N_MAGIC(interp_ex) != ZMAGIC) &&
 (N_MAGIC(interp_ex) != QMAGIC)) {
-  interpreter_type = INTERPRETER_ELF;
+interpreter_type = INTERPRETER_ELF;
 }

 if (interp_elf_ex.e_ident[0] != 0x7f ||
diff --git a/bsd-user/main.c b/bsd-user/main.c
index ac40d79bfa..d8a2011501 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -831,8 +831,8 @@ int main(int argc, char **argv)
 exit(1);
 }
 } else if (!strcmp(r, "B")) {
-   guest_base = strtol(argv[optind++], NULL, 0);
-   have_guest_base = true;
+guest_base = strtol(argv[optind++], NULL, 0);
+have_guest_base = true;
 } else if (!strcmp(r, "drop-ld-preload")) {
 (void) envlist_unsetenv(envlist, "LD_PRELOAD");
 } else if (!strcmp(r, "bsd")) {
diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
index d38ec7a162..9b471b665c 100644
--- a/bsd-user/syscall.c
+++ b/bsd-user/syscall.c
@@ -241,7 +241,7 @@ static abi_long do_freebsd_sysctl(abi_ulong namep, int32_t 
namelen, abi_ulong ol
 return -TARGET_EFAULT;
 holdlen = oldlen;
 for (p = hnamep, q = snamep, i = 0; i < namelen; p++, i++)
-   *q++ = tswap32(*p);
+*q++ = tswap32(*p);
 oidfmt(snamep, namelen, NULL, &kind);
 /* XXX swap hnewp */
 ret = get_errno(sysctl(snamep, namelen, holdp, &holdlen, hnewp, newlen));
-- 
2.29.1.59.gf9b6481aed

[PATCH] ssi: Fix bad printf format specifiers

[AlexChen]

We should use printf format specifier "%u" instead of "%d" for
argument of type "unsigned int".

Reported-by: Euler Robot 
Signed-off-by: Alex Chen 
---
 hw/ssi/imx_spi.c| 2 +-
 hw/ssi/xilinx_spi.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 7f703d8328..d8885ae454 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -53,7 +53,7 @@ static const char *imx_spi_reg_name(uint32_t reg)
 case ECSPI_MSGDATA:
 return  "ECSPI_MSGDATA";
 default:
-sprintf(unknown, "%d ?", reg);
+sprintf(unknown, "%u ?", reg);
 return unknown;
 }
 }
diff --git a/hw/ssi/xilinx_spi.c b/hw/ssi/xilinx_spi.c
index fec8817d94..49ff275593 100644
--- a/hw/ssi/xilinx_spi.c
+++ b/hw/ssi/xilinx_spi.c
@@ -142,7 +142,7 @@ static void xlx_spi_update_irq(XilinxSPI *s)
irq chain unless things really changed.  */
 if (pending != s->irqline) {
 s->irqline = pending;
-DB_PRINT("irq_change of state %d ISR:%x IER:%X\n",
+DB_PRINT("irq_change of state %u ISR:%x IER:%X\n",
 pending, s->regs[R_IPISR], s->regs[R_IPIER]);
 qemu_set_irq(s->irq, pending);
 }
-- 
2.19.1

[PATCH 2/4] bsd-user: do not use C99 // comments

[shiliyang]

This patch fixes error style problems found by checkpatch.pl:
ERROR: do not use C99 // comments

Signed-off-by: Liyang Shi 

---
 bsd-user/elfload.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index 25e625d86b..30d2d6b7a1 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -1010,7 +1010,7 @@ static const char *lookup_symbolxx(struct syminfo *s, 
target_ulong orig_addr)
 struct elf_sym *syms = s->disas_symtab.elf64;
 #endif

-// binary search
+/* binary search */
 struct elf_sym *sym;

 sym = bsearch(&orig_addr, syms, s->disas_num_syms, sizeof(*syms), symfind);
@@ -1092,7 +1092,7 @@ static void load_symbols(struct elfhdr *hdr, int fd)
 #ifdef BSWAP_NEEDED
 bswap_sym(syms + i);
 #endif
-// Throw away entries which we do not need.
+/* Throw away entries which we do not need. */
 if (syms[i].st_shndx == SHN_UNDEF ||
 syms[i].st_shndx >= SHN_LORESERVE ||
 ELF_ST_TYPE(syms[i].st_info) != STT_FUNC) {
-- 
2.29.1.59.gf9b6481aed

[PATCH for-5.2 1/3] hw/block/nvme: fix null ns in register namespace

[Klaus Jensen]

From: Klaus Jensen 

Fix dereference after NULL check.

Reported-by: Coverity (CID 1436128)
Fixes: b20804946bce ("hw/block/nvme: update nsid when registered")
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fa2cba744b57..080d782f1c2b 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2562,8 +2562,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace 
*ns, Error **errp)
 
 if (!nsid) {
 for (int i = 1; i <= n->num_namespaces; i++) {
-NvmeNamespace *ns = nvme_ns(n, i);
-if (!ns) {
+if (!nvme_ns(n, i)) {
 nsid = ns->params.nsid = i;
 break;
 }
-- 
2.29.1

[PATCH 3/4] bsd-user: "foo * bar" should be "foo *bar"

[shiliyang]

This patch fixes error style problems found by checkpatch.pl:
ERROR: "foo ** bar" should be "foo **bar".
ERROR: "foo * bar" should be "foo *bar"

Signed-off-by: Liyang Shi 

---
 bsd-user/bsdload.c |  6 +++---
 bsd-user/elfload.c | 22 +++---
 bsd-user/qemu.h| 14 +++---
 3 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/bsd-user/bsdload.c b/bsd-user/bsdload.c
index f38c4faacf..b2f352c041 100644
--- a/bsd-user/bsdload.c
+++ b/bsd-user/bsdload.c
@@ -20,7 +20,7 @@ abi_long memcpy_to_target(abi_ulong dest, const void *src,
 return 0;
 }

-static int count(char ** vec)
+static int count(char **vec)
 {
 int i;

@@ -125,8 +125,8 @@ abi_ulong loader_build_argptr(int envc, int argc, abi_ulong 
sp,
 return sp;
 }

-int loader_exec(const char * filename, char ** argv, char ** envp,
- struct target_pt_regs * regs, struct image_info *infop)
+int loader_exec(const char *filename, char **argv, char **envp,
+ struct target_pt_regs *regs, struct image_info *infop)
 {
 struct linux_binprm bprm;
 int retval;
diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index 32378af7b2..d5746f25e7 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -547,12 +547,12 @@ struct exec

 #define DLINFO_ITEMS 12

-static inline void memcpy_fromfs(void * to, const void * from, unsigned long n)
+static inline void memcpy_fromfs(void *to, const void *from, unsigned long n)
 {
 memcpy(to, from, n);
 }

-static int load_aout_interp(void * exptr, int interp_fd);
+static int load_aout_interp(void *exptr, int interp_fd);

 #ifdef BSWAP_NEEDED
 static void bswap_ehdr(struct elfhdr *ehdr)
@@ -613,7 +613,7 @@ static void bswap_sym(struct elf_sym *sym)
  * to be put directly into the top of new user memory.
  *
  */
-static abi_ulong copy_elf_strings(int argc,char ** argv, void **page,
+static abi_ulong copy_elf_strings(int argc,char **argv, void **page,
   abi_ulong p)
 {
 char *tmp, *tmp1, *pag = NULL;
@@ -756,7 +756,7 @@ static void padzero(abi_ulong elf_bss, abi_ulong last_bss)


 static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
-   struct elfhdr * exec,
+   struct elfhdr *exec,
abi_ulong load_addr,
abi_ulong load_bias,
abi_ulong interp_load_addr, int ibcs,
@@ -834,7 +834,7 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, 
int envc,
 }


-static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
+static abi_ulong load_elf_interp(struct elfhdr *interp_elf_ex,
  int interpreter_fd,
  abi_ulong *interp_load_addr)
 {
@@ -1143,8 +1143,8 @@ static void load_symbols(struct elfhdr *hdr, int fd)
 syminfos = s;
 }

-int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
-struct image_info * info)
+int load_elf_binary(struct linux_binprm *bprm, struct target_pt_regs *regs,
+struct image_info *info)
 {
 struct elfhdr elf_ex;
 struct elfhdr interp_elf_ex;
@@ -1155,11 +1155,11 @@ int load_elf_binary(struct linux_binprm * bprm, struct 
target_pt_regs * regs,
 unsigned int interpreter_type = INTERPRETER_NONE;
 unsigned char ibcs2_interpreter;
 int i;
-struct elf_phdr * elf_ppnt;
+struct elf_phdr *elf_ppnt;
 struct elf_phdr *elf_phdata;
 abi_ulong elf_bss, k, elf_brk;
 int retval;
-char * elf_interpreter;
+char *elf_interpreter;
 abi_ulong elf_entry, interp_load_addr = 0;
 abi_ulong start_code, end_code, start_data, end_data;
 abi_ulong reloc_func_desc = 0;
@@ -1334,7 +1334,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct 
target_pt_regs * regs,
and then start this sucker up */

 {
-char * passed_p;
+char *passed_p;

 if (interpreter_type == INTERPRETER_AOUT) {
 snprintf(passed_fileno, sizeof(passed_fileno), "%d", bprm->fd);
@@ -1553,7 +1553,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct 
target_pt_regs * regs,
 return 0;
 }

-static int load_aout_interp(void * exptr, int interp_fd)
+static int load_aout_interp(void *exptr, int interp_fd)
 {
 printf("a.out interpreter not yet supported\n");
 return(0);
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index f8bb1e5459..cbf42129e4 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -123,19 +123,19 @@ struct linux_binprm {
 int argc, envc;
 char **argv;
 char **envp;
-char * filename;/* Name of binary */
+char *filename;/* Name of binary */
 };

 void do_init_thread(struct target_pt_regs *regs, struct image_info *infop);
 abi_ulong loader_build_argptr(int envc, int argc, abi_ulong sp,
   abi_ulong stringp, int push_ptr

[PATCH] qtest: Fix bad printf format specifiers

[AlexChen]

We should use printf format specifier "%u" instead of "%d" for
argument of type "unsigned int".

Reported-by: Euler Robot 
Signed-off-by: Alex Chen 
---
 tests/qtest/arm-cpu-features.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index d20094d5a7..bc681a95d5 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -536,7 +536,7 @@ static void test_query_cpu_model_expansion_kvm(const void 
*data)
 if (kvm_supports_sve) {
 g_assert(vls != 0);
 max_vq = 64 - __builtin_clzll(vls);
-sprintf(max_name, "sve%d", max_vq * 128);
+sprintf(max_name, "sve%u", max_vq * 128);

 /* Enabling a supported length is of course fine. */
 assert_sve_vls(qts, "host", vls, "{ %s: true }", max_name);
@@ -556,7 +556,7 @@ static void test_query_cpu_model_expansion_kvm(const void 
*data)
  * unless all larger, supported vector lengths are also
  * disabled.
  */
-sprintf(name, "sve%d", vq * 128);
+sprintf(name, "sve%u", vq * 128);
 error = g_strdup_printf("cannot disable %s", name);
 assert_error(qts, "host", error,
  "{ %s: true, %s: false }",
@@ -569,7 +569,7 @@ static void test_query_cpu_model_expansion_kvm(const void 
*data)
  * we need at least one vector length enabled.
  */
 vq = __builtin_ffsll(vls);
-sprintf(name, "sve%d", vq * 128);
+sprintf(name, "sve%u", vq * 128);
 error = g_strdup_printf("cannot disable %s", name);
 assert_error(qts, "host", error, "{ %s: false }", name);
 g_free(error);
@@ -581,7 +581,7 @@ static void test_query_cpu_model_expansion_kvm(const void 
*data)
 }
 }
 if (vq <= SVE_MAX_VQ) {
-sprintf(name, "sve%d", vq * 128);
+sprintf(name, "sve%u", vq * 128);
 error = g_strdup_printf("cannot enable %s", name);
 assert_error(qts, "host", error, "{ %s: true }", name);
 g_free(error);
-- 
2.19.1

[PATCH for-5.2 0/3] hw/block/nvme: coverity fixes

[Klaus Jensen]

From: Klaus Jensen 

Fix three issues reported by coverity (CIDs 1436128, 1436129 and
1436131).

Klaus Jensen (3):
  hw/block/nvme: fix null ns in register namespace
  hw/block/nvme: fix uint16_t use of uint32_t sgls member
  hw/block/nvme: fix free of array-typed value

 hw/block/nvme.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

-- 
2.29.1

[PATCH for-5.2 2/3] hw/block/nvme: fix uint16_t use of uint32_t sgls member

[Klaus Jensen]

From: Klaus Jensen 

nvme_map_sgl_data erroneously uses the sgls member of NvmeIdNs as a
uint16_t.

Reported-by: Coverity (CID 1436129)
Fixes: cba0a8a344fe ("hw/block/nvme: add support for scatter gather lists")
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 080d782f1c2b..2bdc50eb6fce 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -452,7 +452,7 @@ static uint16_t nvme_map_sgl_data(NvmeCtrl *n, QEMUSGList 
*qsg,
  * segments and/or descriptors. The controller might accept
  * ignoring the rest of the SGL.
  */
-uint16_t sgls = le16_to_cpu(n->id_ctrl.sgls);
+uint32_t sgls = le32_to_cpu(n->id_ctrl.sgls);
 if (sgls & NVME_CTRL_SGLS_EXCESS_LENGTH) {
 break;
 }
-- 
2.29.1

Re: [PULL v2 00/38] pc,pci,vhost,virtio: fixes

[Peter Maydell]

On Wed, 4 Nov 2020 at 04:50, Michael S. Tsirkin  wrote:
>
> Sending v2 since v1 was borken on 32 bit.
>
> The following changes since commit c7a7a877b716cf14848f1fd5c754d293e2f8d852:
>
>   Merge remote-tracking branch 
> 'remotes/pmaydell/tags/pull-target-arm-20201102' into staging (2020-11-03 
> 10:38:05 +)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 5676edd6d984b0696d206faa230c8da15e271044:
>
>   vhost-user-blk-test: fix races by using fd passing (2020-11-03 16:39:06 
> -0500)
>
> 
> pc,pci,vhost,virtio: fixes
>
> Lots of fixes all over the place.
> virtio-mem and virtio-iommu patches are kind of fixes but
> it seems better to just make them behave sanely than
> try to educate users about the limitations ...
>
> Signed-off-by: Michael S. Tsirkin 

Fails 'make check' on ppc64be and s390x, ie the big-endian hosts:

**
ERROR:../../tests/qtest/libqos/virtio.c:228:qvirtio_wait_used_elem:
assertion failed: (g_get_monotonic_time() - start_time <= timeout_us)
ERROR qtest-i386/qos-test - Bail out!
ERROR:../../tests/qtest/libqos/virtio.c:228:qvirtio_wait_used_elem:
assertion failed: (g_get_monotonic_time() - start_time <= timeout_us)

thanks
-- PMM

[PATCH] hw/arm/Kconfig: ARM_V7M depends on PTIMER

[Andrew Jones]

commit 32bd322a0134 ("hw/timer/armv7m_systick: Rewrite to use ptimers")
changed armv7m_systick to build on ptimers. Make sure we have ptimers
in the build when building armv7m_systick.

Signed-off-by: Andrew Jones 
---
 hw/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index fdf4464b9484..7d5d89e1acf9 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -291,6 +291,7 @@ config ZYNQ
 
 config ARM_V7M
 bool
+select PTIMER
 
 config ALLWINNER_A10
 bool
-- 
2.26.2

Re: [PATCH] hw/arm/Kconfig: ARM_V7M depends on PTIMER

[Philippe Mathieu-Daudé]

On 11/4/20 11:33 AM, Andrew Jones wrote:
> commit 32bd322a0134 ("hw/timer/armv7m_systick: Rewrite to use ptimers")
> changed armv7m_systick to build on ptimers. Make sure we have ptimers
> in the build when building armv7m_systick.
> 
> Signed-off-by: Andrew Jones 
> ---
>  hw/arm/Kconfig | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé 

[PATCH for-5.2 3/3] hw/block/nvme: fix free of array-typed value

[Klaus Jensen]

From: Klaus Jensen 

Since 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces"), the
namespaces member of NvmeCtrl is no longer a dynamically allocated
array. Remove the free.

Fixes: 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces")
Reported-by: Coverity (CID 1436131)
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2bdc50eb6fce..01b657b1c5e2 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2799,7 +2799,6 @@ static void nvme_exit(PCIDevice *pci_dev)
 NvmeCtrl *n = NVME(pci_dev);
 
 nvme_clear_ctrl(n);
-g_free(n->namespaces);
 g_free(n->cq);
 g_free(n->sq);
 g_free(n->aer_reqs);
-- 
2.29.1

[Bug 1901981] Re: assert issue locates in hw/usb/dev-storage.c:248: usb_msd_send_status

[Gerd Hoffmann]

poc doens't run on fedora:
uhci: common.c:59: gva_to_gpa: Assertion `gfn != -1' failed.

Can you build qemu with DEBUG_MSD enabled (see hw/usb/dev-storage.c),
then attach both stderr log and stacktrace?

thanks.

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

Title:
  assert issue locates in hw/usb/dev-storage.c:248: usb_msd_send_status

Status in QEMU:
  New

Bug description:
  Hello,

  I found an assertion failure through hw/usb/dev-storage.c.

  This was found in latest version 5.1.0.

  

  qemu-system-x86_64: hw/usb/dev-storage.c:248: usb_msd_send_status: Assertion 
`s->csw.sig == cpu_to_le32(0x53425355)' failed.
  [1]29544 abort  sudo  -enable-kvm -boot c -m 2G -drive 
format=qcow2,file=./ubuntu.img -nic

  To reproduce the assertion failure, please run the QEMU with following
  command line.

  
  $ qemu-system-x86_64 -enable-kvm -boot c -m 2G -drive 
format=qcow2,file=./ubuntu.img -nic 
user,model=rtl8139,hostfwd=tcp:0.0.0.0:-:22 -device piix4-usb-uhci,id=uhci 
-device usb-storage,drive=mydrive -drive 
id=mydrive,file=null-co://,size=2M,format=raw,if=none

  The poc is attached.

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

Re: [PATCH] target/openrisc: Remove dead code attempting to check "is timer disabled"

[Peter Maydell]

On Wed, 4 Nov 2020 at 07:10, Stafford Horne  wrote:
>
> On Tue, Nov 03, 2020 at 11:46:54AM +, Peter Maydell wrote:
> > In the mtspr helper we attempt to check for "is the timer disabled"
> > with "if (env->ttmr & TIMER_NONE)".  This is wrong because TIMER_NONE
> > is zero and the condition is always false (Coverity complains about
> > the dead code.)
> >
> > The correct check would be to test whether the TTMR_M field in the
> > register is equal to TIMER_NONE instead.  However, the
> > cpu_openrisc_timer_update() function checks whether the timer is
> > enabled (it looks at cpu->env.is_counting, which is set to 0 via
> > cpu_openrisc_count_stop() when the TTMR_M field is set to
> > TIMER_NONE), so there's no need to check for "timer disabled" in the
> > target/openrisc code.  Instead, simply remove the dead code.
>
> Thanks for the patch!
>
> I think the check is needed, but it's coded wrong.  The ttmr bits 31,30
> are the timer mode.  If they are equal to zero (TIMER_NONE) then it means
> the timer is disabled and we can skip the timer update.

My analysis is in the commit message -- the timer_update() function
will just happily do nothing if the timer is disabled. It seemed
cleaner to me to let the timer function just do the right thing
rather than having both layers of the code try to figure out if
there's no action to take.

> The code should be something like ((env->ttmr >> 30) == TIMER_NONE). But I
> haven't tested it.

TIMER_NONE and the other TIMER_* fields are defined as (n << 30),
and the mask TTMR_M is (3 << 30), so "(env->ttmr & TTMR_M) == TIMER_NONE"
would be the condition to check if we wanted to do it here. (That
matches how the code earlier in the function does it.)

thanks
-- PMM

[PATCH] contrib/libvhost-user: Fix bad printf format specifiers

[AlexChen]

We should use printf format specifier "%u" instead of "%d" for
argument of type "unsigned int".

Reported-by: Euler Robot 
Signed-off-by: Alex Chen 
---
 contrib/libvhost-user/libvhost-user.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/contrib/libvhost-user/libvhost-user.c 
b/contrib/libvhost-user/libvhost-user.c
index bfec8a881a..5c73ffdd6b 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -701,7 +701,7 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
 return false;
 }

-DPRINT("Adding region: %d\n", dev->nregions);
+DPRINT("Adding region: %u\n", dev->nregions);
 DPRINT("guest_phys_addr: 0x%016"PRIx64"\n",
msg_region->guest_phys_addr);
 DPRINT("memory_size: 0x%016"PRIx64"\n",
@@ -848,7 +848,7 @@ vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg 
*vmsg)
 VhostUserMemory m = vmsg->payload.memory, *memory = &m;
 dev->nregions = memory->nregions;

-DPRINT("Nregions: %d\n", memory->nregions);
+DPRINT("Nregions: %u\n", memory->nregions);
 for (i = 0; i < dev->nregions; i++) {
 void *mmap_addr;
 VhostUserMemoryRegion *msg_region = &memory->regions[i];
@@ -938,7 +938,7 @@ vu_set_mem_table_exec(VuDev *dev, VhostUserMsg *vmsg)
 return vu_set_mem_table_exec_postcopy(dev, vmsg);
 }

-DPRINT("Nregions: %d\n", memory->nregions);
+DPRINT("Nregions: %u\n", memory->nregions);
 for (i = 0; i < dev->nregions; i++) {
 void *mmap_addr;
 VhostUserMemoryRegion *msg_region = &memory->regions[i];
@@ -1049,8 +1049,8 @@ vu_set_vring_num_exec(VuDev *dev, VhostUserMsg *vmsg)
 unsigned int index = vmsg->payload.state.index;
 unsigned int num = vmsg->payload.state.num;

-DPRINT("State.index: %d\n", index);
-DPRINT("State.num:   %d\n", num);
+DPRINT("State.index: %u\n", index);
+DPRINT("State.num:   %u\n", num);
 dev->vq[index].vring.num = num;

 return false;
@@ -1105,8 +1105,8 @@ vu_set_vring_base_exec(VuDev *dev, VhostUserMsg *vmsg)
 unsigned int index = vmsg->payload.state.index;
 unsigned int num = vmsg->payload.state.num;

-DPRINT("State.index: %d\n", index);
-DPRINT("State.num:   %d\n", num);
+DPRINT("State.index: %u\n", index);
+DPRINT("State.num:   %u\n", num);
 dev->vq[index].shadow_avail_idx = dev->vq[index].last_avail_idx = num;

 return false;
@@ -1117,7 +1117,7 @@ vu_get_vring_base_exec(VuDev *dev, VhostUserMsg *vmsg)
 {
 unsigned int index = vmsg->payload.state.index;

-DPRINT("State.index: %d\n", index);
+DPRINT("State.index: %u\n", index);
 vmsg->payload.state.num = dev->vq[index].last_avail_idx;
 vmsg->size = sizeof(vmsg->payload.state);

@@ -1478,8 +1478,8 @@ vu_set_vring_enable_exec(VuDev *dev, VhostUserMsg *vmsg)
 unsigned int index = vmsg->payload.state.index;
 unsigned int enable = vmsg->payload.state.num;

-DPRINT("State.index: %d\n", index);
-DPRINT("State.enable:   %d\n", enable);
+DPRINT("State.index: %u\n", index);
+DPRINT("State.enable:   %u\n", enable);

 if (index >= dev->max_queues) {
 vu_panic(dev, "Invalid vring_enable index: %u", index);
@@ -1728,7 +1728,7 @@ vu_handle_vring_kick(VuDev *dev, VhostUserMsg *vmsg)
 return false;
 }

-DPRINT("Got kick message: handler:%p idx:%d\n",
+DPRINT("Got kick message: handler:%p idx:%u\n",
dev->vq[index].handler, index);

 if (!dev->vq[index].started) {
@@ -1772,7 +1772,7 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
 DPRINT("Request: %s (%d)\n", vu_request_to_string(vmsg->request),
vmsg->request);
 DPRINT("Flags:   0x%x\n", vmsg->flags);
-DPRINT("Size:%d\n", vmsg->size);
+DPRINT("Size:%u\n", vmsg->size);

 if (vmsg->fd_num) {
 int i;
-- 
2.19.1

Re: [PULL 48/48] hw/timer/armv7m_systick: Rewrite to use ptimers

[Andrew Jones]

On Wed, Nov 04, 2020 at 11:11:53AM +0100, Philippe Mathieu-Daudé wrote:
> On 11/4/20 11:03 AM, Andrew Jones wrote:
> > On Tue, Oct 27, 2020 at 11:44:38AM +, Peter Maydell wrote:
> >> The armv7m systick timer is a 24-bit decrementing, wrap-on-zero,
> >> clear-on-write counter. Our current implementation has various
> >> bugs and dubious workarounds in it (for instance see
> >> https://bugs.launchpad.net/qemu/+bug/1872237).
> >>
> >> We have an implementation of a simple decrementing counter
> >> and we put a lot of effort into making sure it handles the
> >> interesting corner cases (like "spend a cycle at 0 before
> >> reloading") -- ptimer.
> >>
> >> Rewrite the systick timer to use a ptimer rather than
> >> a raw QEMU timer.
> >>
> >> Unfortunately this is a migration compatibility break,
> >> which will affect all M-profile boards.
> >>
> >> Among other bugs, this fixes
> >> https://bugs.launchpad.net/qemu/+bug/1872237 :
> >> now writes to SYST_CVR when the timer is enabled correctly
> >> do nothing; when the timer is enabled via SYST_CSR.ENABLE,
> >> the ptimer code will (because of POLICY_NO_IMMEDIATE_RELOAD)
> >> arrange that after one timer tick the counter is reloaded
> >> from SYST_RVR and then counts down from there, as the
> >> architecture requires.
> >>
> >> Signed-off-by: Peter Maydell 
> >> Reviewed-by: Philippe Mathieu-Daudé 
> >> Message-id: 20201015151829.14656-3-peter.mayd...@linaro.org
> >> ---
> >>  include/hw/timer/armv7m_systick.h |   3 +-
> >>  hw/timer/armv7m_systick.c | 124 +-
> >>  2 files changed, 54 insertions(+), 73 deletions(-)
> >>
> > 
> > Do we also need something like the diff below now?
> > 
> > 
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index fdf4464b9484..7d5d89e1acf9 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -291,6 +291,7 @@ config ZYNQ
> >  
> >  config ARM_V7M
> >  bool
> > +select PTIMER
> 
> Oops yes indeed.

OK, I'll post it.

Thanks,
drew

> 
> >  
> >  config ALLWINNER_A10
> >  bool
> > 
> > 
> > Thanks,
> > drew
> > 
> > 
> 
> 

Re: Ramping up Continuous Fuzzing of Virtual Devices in QEMU

[P J P]

+-- On Thu, 22 Oct 2020, Daniel P. Berrangé wrote --+
| On Thu, Oct 22, 2020 at 12:24:16PM -0400, Alexander Bulekov wrote:
| > > Once [2] lands upstream, we should see a significant uptick in oss-fuzz 
| > > reports, and I hope that we can develop a process to ensure these bugs 
| > > are properly dealt with. One option we have is to make the reports 
| > > public immediately and send notifications to qemu-devel. This is the 
| > > approach taken by some other projects on oss-fuzz, such as LLVM. Though 
| > > its not on oss-fuzz, bugs found by syzkaller in the kernel, are also 
| > > automatically sent to a public list. The question is:
| > > 
| > > What approach should we take for dealing with bugs found on oss-fuzz?
| 
| If we assume that a non-negligible number of fuzz bugs will be exploitable
| by a malicious guest OS to break out into the host, then I think it is
| likely undesirable to make them public immediately without at least a basic
| human triage step to catch possibly serious security issues.

* Maybe the proposed 'qemu-security' list can receive such issue reports.  It 
  is more close than qemu-devel.

  But it also depends on the quantum of traffic oss-fuzz generates. We don't 
  want to flood/overwhelm qemu-security list or any other list for that 
  matter.

* Human triage is required to know potential impact of an issue before it is 
  sent to a public list. It would not be good to send guest-to-host-escape
  type issues directly to a public list.

* Ideally preliminary human triage should be done on the fuzzers' side.  
  After it hits an issue, someone should have a look at it before sending an 
  email to a list OR maintainer(s).

  Ex. TCG issues are generally not considered for CVE. They need not go to a
  security list.



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

Re: Debugging with rr

[Peter Maydell]

On Wed, 4 Nov 2020 at 06:11, Sai Pavan Boddu  wrote:
> I tired debugging QEMU with rr version 4.1.0, on ubuntu 16.04. And I see 
> below errors, any suggestions on the issue would be helpful

rr version 4.1.0 is pretty old -- it was released in 2016.
QEMU does some complicated stuff that in the past rr
needed fixes to handle. Start by trying the most recent
rr release (or rr from git if you like). If that doesn't
work then the rr folks are pretty responsive to bug reports.

thanks
-- PMM

Re: [PATCH] qtest: Fix bad printf format specifiers

[Thomas Huth]

On 04/11/2020 11.23, AlexChen wrote:
> We should use printf format specifier "%u" instead of "%d" for
> argument of type "unsigned int".
> 
> Reported-by: Euler Robot 
> Signed-off-by: Alex Chen 
> ---
>  tests/qtest/arm-cpu-features.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index d20094d5a7..bc681a95d5 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -536,7 +536,7 @@ static void test_query_cpu_model_expansion_kvm(const void 
> *data)
>  if (kvm_supports_sve) {
>  g_assert(vls != 0);
>  max_vq = 64 - __builtin_clzll(vls);
> -sprintf(max_name, "sve%d", max_vq * 128);
> +sprintf(max_name, "sve%u", max_vq * 128);
> 
>  /* Enabling a supported length is of course fine. */
>  assert_sve_vls(qts, "host", vls, "{ %s: true }", max_name);
> @@ -556,7 +556,7 @@ static void test_query_cpu_model_expansion_kvm(const void 
> *data)
>   * unless all larger, supported vector lengths are also
>   * disabled.
>   */
> -sprintf(name, "sve%d", vq * 128);
> +sprintf(name, "sve%u", vq * 128);
>  error = g_strdup_printf("cannot disable %s", name);
>  assert_error(qts, "host", error,
>   "{ %s: true, %s: false }",
> @@ -569,7 +569,7 @@ static void test_query_cpu_model_expansion_kvm(const void 
> *data)
>   * we need at least one vector length enabled.
>   */
>  vq = __builtin_ffsll(vls);
> -sprintf(name, "sve%d", vq * 128);
> +sprintf(name, "sve%u", vq * 128);
>  error = g_strdup_printf("cannot disable %s", name);
>  assert_error(qts, "host", error, "{ %s: false }", name);
>  g_free(error);
> @@ -581,7 +581,7 @@ static void test_query_cpu_model_expansion_kvm(const void 
> *data)
>  }
>  }
>  if (vq <= SVE_MAX_VQ) {
> -sprintf(name, "sve%d", vq * 128);
> +sprintf(name, "sve%u", vq * 128);
>  error = g_strdup_printf("cannot enable %s", name);
>  assert_error(qts, "host", error, "{ %s: true }", name);
>  g_free(error);
> 

max_vq and vq are both "uint32_t" and not "unsigned int" ... so if you want
to fix this really really correctly, please use PRIu32 from inttypes.h instead.

 Thanks,
  Thomas

Re: [PATCH for-5.2 3/3] hw/block/nvme: fix free of array-typed value

[Philippe Mathieu-Daudé]

On 11/4/20 11:22 AM, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Since 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces"), the
> namespaces member of NvmeCtrl is no longer a dynamically allocated
> array. Remove the free.
> 
> Fixes: 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces")
> Reported-by: Coverity (CID 1436131)
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [QEMU] Question regarding user mode support for ARM syscalls

[Lukasz Majewski]

Hi Alistair,

> On Tue, Nov 3, 2020 at 8:55 AM Lukasz Majewski  wrote:
> >
> > Hi Alistair,
> >  
> > > On Tue, Nov 3, 2020 at 3:03 AM Lukasz Majewski 
> > > wrote:  
> > > >
> > > > Dear Qemu Community,  
> > >
> > > Hey Lukasz,
> > >
> > > + QEMU Dev Mailing list
> > > + Laurent
> > >  
> >
> > Thanks for reaching more people.
> >  
> > > >
> > > > I would like to ask you for some advice regarding the usage of
> > > > arm-linux-user/qemu-arm user space program simulation.
> > > >
> > > > Background:
> > > > ---
> > > >
> > > > I'm looking for a way to efficiently test y2038 in-glibc
> > > > solution for 32 bit architectures (e.g. ARM).
> > > >
> > > > For now I do use qemu-system-arm (part of Yocto/OE), which I'm
> > > > using to run Linux kernel 5.1, glibc under test and Y2038
> > > > tests. It works [1].
> > > >
> > > > Problem:
> > > > 
> > > >
> > > > I would like to test cross-compiled tests (which are built from
> > > > glibc sources) without the need to run the emulated system with
> > > > qemu-system-arm.
> > > >
> > > > I've come across the "QEMU user mode", which would execute the
> > > > cross-compiled test (with already cross-compiled glibc via -L
> > > > switch) and just return exit status code. This sounds
> > > > appealing.  
> > >
> > > As another advantage it is much, much faster at running the glibc
> > > tests.
> > >  
> >
> > +1
> >  
> > > >
> > > > As fair as I've read - QEMU user mode emulates ARM syscalls.
> > > >
> > > > During test execution (single qemu user mode process) I would
> > > > need to adjust date with clock_settime64 syscall and then
> > > > execute other syscalls if needed.
> > > >
> > > >
> > > > Please correct me if I'm wrong:
> > > > - It looks like qemu-arm doesn't have switch which would allow
> > > > it to set time offset (to e.g. year 2039 - something similar to
> > > >   qemu-system-arm -rtc=).
> > > >
> > > > - As of 5.1 qemu version there is no support for syscalls
> > > > supporting 64 bit time on 32 bit architectures (e.g.
> > > > clock_settime64 and friends from [2]).  
> > >
> > > There is some support in the current master, for example
> > > __NR_futex_time64 is supported.  
> >
> > I've just looked into v5.1.0 stable release. I will double check
> > this with -master branch.
> >  
> > >
> > > I started to add some support for RV32 once it was merged into
> > > glibc.  
> >
> > Ok.
> >  
> > >  
> > > >
> > > > For my example program [3] statically build for testing (it
> > > > works with qemu-system-arm):
> > > >
> > > > ~/work/qemu-arm-tests-program$
> > > > ../qemu-5.1.0-arm/arm-linux-user/qemu-arm -L
> > > > ~/work/yocto/y2038/build/tmp/armv7at2hf-neon-poky-linux-gnueabi/y2038-glibc/2.30+git999-r0/image/opt
> > > > -strace ./cst
> > > >
> > > > 17746 brk(NULL) = 0x00074000
> > > > 17746 brk(0x000748a8) = 0x000748a8
> > > > 17746 uname(0x40800370) = 0
> > > > 17746 readlink("/proc/self/exe",0x407ff488,4096) = 43
> > > > 17746 brk(0x000958a8) = 0x000958a8
> > > > 17746 brk(0x00096000) = 0x00096000
> > > > 17746 mprotect(0x0007,8192,PROT_READ) = 0
> > > > 17746statx(1,"",AT_EMPTY_PATH|AT_NO_AUTOMOUNT,STATX_BASIC_STATS,0x407ffd70)
> > > > = 0
> > > > 17746 Unknown syscall 404 --> is the syscall number of
> > > > clock_settime64  
> > >
> > > clock_settime64 is supported in master QEMU.  
> >
> > I will double check it - thanks for pointing this out.
> >  
> > >  
> > > >
> > > > 17746 dup(2) = 3
> > > > 17746 fcntl64(3,F_GETFL) = 2
> > > > 17746statx(3,"",AT_EMPTY_PATH|AT_NO_AUTOMOUNT,STATX_BASIC_STATS,0x407ff8e8)
> > > > = 0 ERR
> > > >
> > > > Questions:
> > > > --
> > > >
> > > > 1. Is there any plan to add support for emulating syscalls
> > > > supporting 64 bit time on 32 bit architectures [2]?  
> > >
> > > I would like to have RV32 supported, but it's a low priority for
> > > me.  
> >
> > Having syscalls supporting 64 bit time on 32 bit machines indicated
> > in [2] would be a very welcome for glibc testing.
> >  
> > > I
> > > expect it's something that will eventually happen though.  
> >
> > Ok.
> >  
> > >  
> > > >
> > > > 2. Provide QEMU user space switch to adjust its time (i.e. add
> > > > some offset to in-fly emulated time syscalls - like
> > > > clock_settime64) when it is started?  
> > >
> > > That I'm not sure about.  
> >
> > For me it would be enough to have:
> >
> > qemu-arm -rtc="2039-01-01" -L... ./ctx
> > So the emulated "time" would be after 32 bit time_t overflow when
> > QEMU user space emulation process starts (as long as it doesn't
> > touch the host machine time).
> >
> >
> > Another option (workaround) would be to run clock_settime64() with
> > time set to year 2038+ on the beginning of each glibc test. It
> > shall work as long as we don't change host time (and all time
> > changes would stay in the qemu user mode process).
> >  
> > > I assume just running date/clock_settime64
> > > from a script wouldn't work with the glibc test framework?  
> >
> > Could you elaborate on this use case/

Re: [PATCH] hw/9pfs: virtio-9p: Ensure config space is a multiple of 4 bytes

[Christian Schoenebeck]

On Mittwoch, 4. November 2020 08:44:44 CET Bin Meng wrote:
> Hi Michael,
> 
> On Tue, Nov 3, 2020 at 8:05 PM Michael S. Tsirkin  wrote:
> > On Tue, Nov 03, 2020 at 02:26:10PM +0800, Bin Meng wrote:
> > > Hi Michael,
> > > 
> > > On Fri, Oct 30, 2020 at 5:29 PM Michael S. Tsirkin  
wrote:
> > > > On Thu, Oct 29, 2020 at 04:25:41PM +0800, Bin Meng wrote:
> > > > > From: Bin Meng 
> > > > > 
> > > > > At present the virtio device config space access is handled by the
> > > > > virtio_config_readX() and virtio_config_writeX() APIs. They perform
> > > > > a sanity check on the result of address plus size against the config
> > > > > space size before the access occurs.
> > > > > 
> > > > > For unaligned access, the last converted naturally aligned access
> > > > > will fail the sanity check on 9pfs. For example, with a mount_tag
> > > > > `p9fs`, if guest software tries to read the mount_tag via a 4 byte
> > > > > read at the mount_tag offset which is not 4 byte aligned, the read
> > > > > result will be `p9\377\377`, which is wrong.
> > > > > 
> > > > > This changes the size of device config space to be a multiple of 4
> > > > > bytes so that correct result can be returned in all circumstances.
> > > > > 
> > > > > Signed-off-by: Bin Meng 
> > > > 
> > > > The patch is ok, but I'd like to clarify the commit log.
> > > 
> > > Thanks for the review.
> > > 
> > > > If I understand correctly, what happens is:
> > > > - tag is set to a value that is not a multiple of 4 bytes
> > > 
> > > It's not about the mount_tag value, but the length of the mount_tag is
> > > 4.
> > > 
> > > > - guest attempts to read the last 4 bytes of the tag
> > > 
> > > Yep. So the config space of a 9pfs looks like the following:
> > > 
> > > offset: 0x14, size: 2 bytes indicating the length of the following
> > > mount_tag offset: 0x16, size: value of (offset 0x14).
> > > 
> > > When a 4-byte mount_tag is given, guest software is subject to read 4
> > > bytes (value read from offset 0x14) at offset 0x16.
> > 
> > Well looking at Linux guest code:
> > 
> > 
> > static inline void __virtio_cread_many(struct virtio_device *vdev,
> > 
> >unsigned int offset,
> >void *buf, size_t count, size_t
> >bytes)
> > 
> > {
> > 
> > u32 old, gen = vdev->config->generation ?
> > 
> > vdev->config->generation(vdev) : 0;
> > 
> > int i;
> > 
> > might_sleep();
> > do {
> > 
> > old = gen;
> > 
> > for (i = 0; i < count; i++)
> > 
> > vdev->config->get(vdev, offset + bytes * i,
> > 
> >   buf + i * bytes, bytes);
> > 
> > gen = vdev->config->generation ?
> > 
> > vdev->config->generation(vdev) : 0;
> > 
> > } while (gen != old);
> > 
> > }
> > 
> > 
> > 
> > static inline void virtio_cread_bytes(struct virtio_device *vdev,
> > 
> >   unsigned int offset,
> >   void *buf, size_t len)
> > 
> > {
> > 
> > __virtio_cread_many(vdev, offset, buf, len, 1);
> > 
> > }
> > 
> > and:
> > virtio_cread_bytes(vdev, offsetof(struct virtio_9p_config, tag),
> > 
> >tag, tag_len);
> > 
> > So guest is doing multiple 1-byte reads.
> 
> Correct.
> 
> > Spec actually says:
> > For device configuration access, the driver MUST use 8-bit wide
> > accesses for 8-bit wide fields, 16-bit wide
> > 
> > and aligned accesses for 16-bit wide fields and 32-bit wide and
> > aligned accesses for 32-bit and 64-bit wide
> > 
> > fields. For 64-bit fields, the driver MAY access each of the high
> > and low 32-bit parts of the field independently.
> Yes.
> 
> > 9p was never standardized, but the linux header at least lists it as
> > follows:
> > 
> > struct virtio_9p_config {
> > 
> > /* length of the tag name */
> > __virtio16 tag_len;
> > /* non-NULL terminated tag name */
> > __u8 tag[0];
> > 
> > } __attribute__((packed));
> > 
> > In that sense tag is an 8 byte field.
> > 
> > So which guest reads tag using a 32 bit read, and why?
> 
> The obvious fix can be made to the guest which exposed this issue, but
> I was wondering whether we should enforce all virtio devices' config
> space size to be a multiple of 4 bytes which sounds more natural.
> 
> Regards,
> Bin

Personally I am not opposed for this to be addressed in qemu, but Michael 
should decide that.

But even if it would be addressed in qemu, I still think this would better be 
addressed on virtio side, not on virtio user side (9pfs, etc.), because that's 
a virtio implementation detai

Re: [PATCH for-5.2 1/3] hw/block/nvme: fix null ns in register namespace

[Max Reitz]

On 04.11.20 11:22, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Fix dereference after NULL check.
> 
> Reported-by: Coverity (CID 1436128)
> Fixes: b20804946bce ("hw/block/nvme: update nsid when registered")
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Max Reitz 

Re: [PATCH for-5.2 3/3] hw/block/nvme: fix free of array-typed value

[Max Reitz]

On 04.11.20 11:22, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Since 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces"), the
> namespaces member of NvmeCtrl is no longer a dynamically allocated
> array. Remove the free.
> 
> Fixes: 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces")
> Reported-by: Coverity (CID 1436131)
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 1 -
>  1 file changed, 1 deletion(-)

Thanks! :)

Reviewed-by: Max Reitz 

Re: [PATCH for-5.2 2/3] hw/block/nvme: fix uint16_t use of uint32_t sgls member

[Max Reitz]

On 04.11.20 11:22, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> nvme_map_sgl_data erroneously uses the sgls member of NvmeIdNs as a
> uint16_t.
> 
> Reported-by: Coverity (CID 1436129)
> Fixes: cba0a8a344fe ("hw/block/nvme: add support for scatter gather lists")
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz 

Re: [PATCH for-5.2 4/4] qemu-option: warn for short-form boolean options

[Igor Mammedov]

On Tue, 3 Nov 2020 22:41:40 +0100
Paolo Bonzini  wrote:

> On 03/11/20 22:22, Igor Mammedov wrote:
> > shall we also deprecate short forms for -cpu model,[feat|+feat|-feat]
> > and in the end allow only -device compatible form i.e. -cpu 
> > type,feat=[on|off]
> > 
> > that would let us drop custom
> >x86_cpu_parse_featurestr,
> >ppc_cpu_parse_featurestr,
> >sparc_cpu_parse_features
> > 
> > and a bunch of cpu_class_by_name, where almost each target does its
> > magic conversion of cpu_model to the type (which ranges from various
> > prefix/suffix shuffling to completely ignoring cpu_model and returning
> > a fixed cpu type)  
> 
> Yes please. :)  But I would prefer someone else to do the work because I 
> have quite some KVM backlog...
> 

I'll look into it.

> Paolo
> 
> 

Re: [PATCH for-5.2 3/3] hw/block/nvme: fix free of array-typed value

[Klaus Jensen]

On Nov  4 11:59, Max Reitz wrote:
> On 04.11.20 11:22, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Since 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces"), the
> > namespaces member of NvmeCtrl is no longer a dynamically allocated
> > array. Remove the free.
> > 
> > Fixes: 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces")
> > Reported-by: Coverity (CID 1436131)
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/block/nvme.c | 1 -
> >  1 file changed, 1 deletion(-)
> 
> Thanks! :)
> 
> Reviewed-by: Max Reitz 
> 

Will Peter pick up fixes like this directly so we don't have to go
through a pull request from nvme-next?

Did I correctly annotate with "for-5.2"? :)


signature.asc
Description: PGP signature

[Bug 1901981] Re: assert issue locates in hw/usb/dev-storage.c:248: usb_msd_send_status

[Gaoning Pan]

Sorry, my reproduced environment is as follows:
Host: ubuntu 18.04
Guest: ubuntu 18.04

Stderr log is as follows:
usb-msd: Reset
usb-msd: Command on LUN 0
usb-msd: Command tag 0x0 flags  len 0 data 0
[scsi.0 id=0] INQUIRY 0x00 0x00 0x00 0x01 0x00 - from-dev len=1
usb-msd: Deferring packet 0x6110002d2d40 [wait status]
usb-msd: Command status 0 tag 0x0, len 256
qemu-system-x86_64: hw/usb/dev-storage.c:248: usb_msd_send_status: Assertion 
`s->csw.sig == cpu_to_le32(0x53425355)' failed.
[1]643 abort  sudo  -enable-kvm -boot c -m 4G -drive 
format=qcow2,file=./ubuntu.img -nic


Backtrace is as follows:
#0  0x7f8b36a63f47 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/unix/sysv/linux/raise.c:51
#1  0x7f8b36a658b1 in __GI_abort () at abort.c:79
#2  0x7f8b36a5542a in __assert_fail_base (fmt=0x7f8b36bdca38 "%s%s%s:%u: 
%s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x55aef41e7440 
"s->csw.sig == cpu_to_le32(0x53425355)", file=file@entry=0x55aef41e7180 
"hw/usb/dev-storage.c", line=line@entry=248, 
function=function@entry=0x55aef41e7980 <__PRETTY_FUNCTION__.29124> 
"usb_msd_send_status") at assert.c:92
#3  0x7f8b36a554a2 in __GI___assert_fail 
(assertion=assertion@entry=0x55aef41e7440 "s->csw.sig == 
cpu_to_le32(0x53425355)", file=file@entry=0x55aef41e7180 
"hw/usb/dev-storage.c", line=line@entry=248, 
function=function@entry=0x55aef41e7980 <__PRETTY_FUNCTION__.29124> 
"usb_msd_send_status") at assert.c:101
#4  0x55aef32226d5 in usb_msd_send_status (s=0x62301d00, 
p=0x6110002e3500) at hw/usb/dev-storage.c:248
#5  0x55aef322804e in usb_msd_handle_data (dev=0x62301d00, 
p=0x6110002e3500) at hw/usb/dev-storage.c:525
#6  0x55aef30bc46a in usb_device_handle_data (dev=dev@entry=0x62301d00, 
p=p@entry=0x6110002e3500) at hw/usb/bus.c:179
#7  0x55aef30a0ab4 in usb_process_one (p=p@entry=0x6110002e3500) at 
hw/usb/core.c:387
#8  0x55aef30a9db0 in usb_handle_packet (dev=0x62301d00, 
p=p@entry=0x6110002e3500) at hw/usb/core.c:419
#9  0x55aef30fe890 in uhci_handle_td (s=s@entry=0x61f02a80, 
q=0x606c9200, q@entry=0x0, qh_addr=qh_addr@entry=0, 
td=td@entry=0x7ffd88f90620, td_addr=, 
int_mask=int_mask@entry=0x7ffd88f905a0) at hw/usb/hcd-uhci.c:899
#10 0x55aef3104c6f in uhci_process_frame (s=s@entry=0x61f02a80) at 
hw/usb/hcd-uhci.c:1075
#11 0x55aef31098e0 in uhci_frame_timer (opaque=0x61f02a80) at 
hw/usb/hcd-uhci.c:1174
#12 0x55aef3ae5f95 in timerlist_run_timers (timer_list=0x60b51be0) at 
util/qemu-timer.c:572
#13 0x55aef3ae619b in qemu_clock_run_timers (type=QEMU_CLOCK_VIRTUAL) at 
util/qemu-timer.c:586
#14 0x55aef3ae6922 in qemu_clock_run_all_timers () at util/qemu-timer.c:672
#15 0x55aef3aca63d in main_loop_wait (nonblocking=0) at util/main-loop.c:523
#16 0x55aef1f320f5 in qemu_main_loop () at 
/home/zjusvn/new-hyper/qemu-5.1.0/softmmu/vl.c:1676
#17 0x55aef397475c in main (argc=18, argv=0x7ffd88f90e98, 
envp=0x7ffd88f90f30) at /home/zjusvn/new-hyper/qemu-5.1.0/softmmu/main.c:49
#18 0x7f8b36a46b97 in __libc_start_main (main=0x55aef397471d , 
argc=18, argv=0x7ffd88f90e98, init=, fini=, 
rtld_fini=, stack_end=0x7ffd88f90e88) at ../csu/libc-start.c:310
#19 0x55aef1a3481a in _start ()

thanks.

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

Title:
  assert issue locates in hw/usb/dev-storage.c:248: usb_msd_send_status

Status in QEMU:
  New

Bug description:
  Hello,

  I found an assertion failure through hw/usb/dev-storage.c.

  This was found in latest version 5.1.0.

  

  qemu-system-x86_64: hw/usb/dev-storage.c:248: usb_msd_send_status: Assertion 
`s->csw.sig == cpu_to_le32(0x53425355)' failed.
  [1]29544 abort  sudo  -enable-kvm -boot c -m 2G -drive 
format=qcow2,file=./ubuntu.img -nic

  To reproduce the assertion failure, please run the QEMU with following
  command line.

  
  $ qemu-system-x86_64 -enable-kvm -boot c -m 2G -drive 
format=qcow2,file=./ubuntu.img -nic 
user,model=rtl8139,hostfwd=tcp:0.0.0.0:-:22 -device piix4-usb-uhci,id=uhci 
-device usb-storage,drive=mydrive -drive 
id=mydrive,file=null-co://,size=2M,format=raw,if=none

  The poc is attached.

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

Re: [RFC PATCH 5/6] virtio-net: Added eBPF RSS to virtio-net.

[Yuri Benditovich]

On Wed, Nov 4, 2020 at 5:09 AM Jason Wang  wrote:

>
> On 2020/11/3 上午2:51, Andrew Melnychenko wrote:
> > From: Andrew 
> >
> > When RSS is enabled the device tries to load the eBPF program
> > to select RX virtqueue in the TUN. If eBPF can be loaded
> > the RSS will function also with vhost (works with kernel 5.8 and later).
> > Software RSS is used as a fallback with vhost=off when eBPF can't be
> loaded
> > or when hash population requested by the guest.
> >
> > Signed-off-by: Yuri Benditovich 
> > Signed-off-by: Andrew Melnychenko 
> > ---
> >   hw/net/vhost_net.c |   2 +
> >   hw/net/virtio-net.c| 120 +++--
> >   include/hw/virtio/virtio-net.h |   4 ++
> >   net/vhost-vdpa.c   |   2 +
> >   4 files changed, 124 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index 24d555e764..16124f99c3 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -71,6 +71,8 @@ static const int user_feature_bits[] = {
> >   VIRTIO_NET_F_MTU,
> >   VIRTIO_F_IOMMU_PLATFORM,
> >   VIRTIO_F_RING_PACKED,
> > +VIRTIO_NET_F_RSS,
> > +VIRTIO_NET_F_HASH_REPORT,
> >
> >   /* This bit implies RARP isn't sent by QEMU out of band */
> >   VIRTIO_NET_F_GUEST_ANNOUNCE,
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 277289d56e..afcc3032ec 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -698,6 +698,19 @@ static void virtio_net_set_queues(VirtIONet *n)
> >
> >   static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
> >
> > +static uint64_t fix_ebpf_vhost_features(uint64_t features)
> > +{
> > +/* If vhost=on & CONFIG_EBPF doesn't set - disable RSS feature */
> > +uint64_t ret = features;
> > +#ifndef CONFIG_EBPF
> > +virtio_clear_feature(&ret, VIRTIO_NET_F_RSS);
> > +#endif
> > +/* for now, there is no solution for populating the hash from eBPF
> */
> > +virtio_clear_feature(&ret, VIRTIO_NET_F_HASH_REPORT);
>
>
> I think we probably need to to something reverse since RSS is under the
> control on qemu cli, disable features like this may break migration.
>
>
How by design we add new features to qemu in light of possible migration to
older qemu version when the destination
qemu does not support these features?


> We need disable vhost instead when:
>
> 1) eBPF is not supported but RSS is required from command line
>
> or
>
> 2) HASH_REPORT is required from command line
>
> > +
> > +return ret;
> > +}
> > +
> >   static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t
> features,
> >   Error **errp)
> >   {
> > @@ -732,9 +745,9 @@ static uint64_t virtio_net_get_features(VirtIODevice
> *vdev, uint64_t features,
> >   return features;
> >   }
> >
> > -virtio_clear_feature(&features, VIRTIO_NET_F_RSS);
> > -virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT);
> > -features = vhost_net_get_features(get_vhost_net(nc->peer),
> features);
> > +features = fix_ebpf_vhost_features(
> > +vhost_net_get_features(get_vhost_net(nc->peer), features));
> > +
> >   vdev->backend_features = features;
> >
> >   if (n->mtu_bypass_backend &&
> > @@ -1169,12 +1182,75 @@ static int virtio_net_handle_announce(VirtIONet
> *n, uint8_t cmd,
> >   }
> >   }
> >
> > +static void virtio_net_unload_epbf_rss(VirtIONet *n);
> > +
> >   static void virtio_net_disable_rss(VirtIONet *n)
> >   {
> >   if (n->rss_data.enabled) {
> >   trace_virtio_net_rss_disable();
> >   }
> >   n->rss_data.enabled = false;
> > +
> > +if (!n->rss_data.enabled_software_rss &&
> ebpf_rss_is_loaded(&n->ebpf_rss)) {
> > +virtio_net_unload_epbf_rss(n);
> > +}
> > +}
> > +
> > +static bool virtio_net_attach_steering_ebpf(NICState *nic, int prog_fd)
> > +{
> > +NetClientState *nc = qemu_get_peer(qemu_get_queue(nic), 0);
> > +if (nc == NULL || nc->info->set_steering_ebpf == NULL) {
> > +return false;
> > +}
> > +
> > +return nc->info->set_steering_ebpf(nc, prog_fd);
> > +}
> > +
> > +static void rss_data_to_rss_config(struct VirtioNetRssData *data,
> > +   struct EBPFRSSConfig *config)
> > +{
> > +config->redirect = data->redirect;
> > +config->populate_hash = data->populate_hash;
> > +config->hash_types = data->hash_types;
> > +config->indirections_len = data->indirections_len;
> > +config->default_queue = data->default_queue;
> > +}
> > +
> > +static bool virtio_net_load_epbf_rss(VirtIONet *n)
> > +{
> > +struct EBPFRSSConfig config = {};
> > +
> > +if (!n->rss_data.enabled) {
> > +if (ebpf_rss_is_loaded(&n->ebpf_rss)) {
> > +ebpf_rss_unload(&n->ebpf_rss);
> > +}
> > +return true;
> > +}
> > +
> > +if (!ebpf_rss_is_loaded(&n->ebpf_rss) &&
> !ebpf_rss_load(&n->ebpf_rss)) {
> > +return fal

Re: [PATCH 1/4] bsd-user: space required after semicolon

[Thomas Huth]

On 04/11/2020 11.20, shiliyang wrote:
> This patch fixes error style problems found by checkpatch.pl:
> ERROR: space required after that ';'
> 
> Signed-off-by: Liyang Shi 
> 
> ---
>  bsd-user/elfload.c | 2 +-
>  bsd-user/syscall.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index 9f4210af9a..25e625d86b 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -1227,7 +1227,7 @@ int load_elf_binary(struct linux_binprm *bprm, struct 
> target_pt_regs *regs,
>  end_data = 0;
>  interp_ex.a_info = 0;
> 
> -for(i=0;i < elf_ex.e_phnum; i++) {
> +for(i=0; i < elf_ex.e_phnum; i++) {

While you're at it, please also add white spaces around the "=" in that line.

 Thanks,
  Thomas

Re: VFIO Migration

[Christophe de Dinechin]

> On 3 Nov 2020, at 19:49, Dr. David Alan Gilbert  wrote:
> 
> * Stefan Hajnoczi (stefa...@redhat.com ) wrote:
>> On Tue, Nov 03, 2020 at 12:17:09PM +, Dr. David Alan Gilbert wrote:
>>> * Stefan Hajnoczi (stefa...@redhat.com) wrote:
 Device Models
 -
 Devices have a *hardware interface* consisting of hardware registers,
 interrupts, and so on.
 
 The hardware interface together with the device state representation is 
 called
 a *device model*. Device models can be assigned URIs such as
 https://qemu.org/devices/e1000e to uniquely identify them.
>>> 
>>> I think this is a unique identifier, not actually a URI; the https://
>>> isn't needed since no one expects to ever connect to this.
>> 
>> Yes, it could be any unique string. If the URI idea is not popular we
>> can use any similar scheme.
> 
> I'm OK with it being a URI; just drop the https.

I completely agree. https gives the wrong idea about what this represents. 
Unless you give it https semantics, by requiring a doc or a schema or whatever 
to be at the URL, but then you enter another universe of cans of worms.

Re: [PATCH for-5.2 3/3] hw/block/nvme: fix free of array-typed value

[Max Reitz]

On 04.11.20 12:04, Klaus Jensen wrote:
> On Nov  4 11:59, Max Reitz wrote:
>> On 04.11.20 11:22, Klaus Jensen wrote:
>>> From: Klaus Jensen 
>>>
>>> Since 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces"), the
>>> namespaces member of NvmeCtrl is no longer a dynamically allocated
>>> array. Remove the free.
>>>
>>> Fixes: 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces")
>>> Reported-by: Coverity (CID 1436131)
>>> Signed-off-by: Klaus Jensen 
>>> ---
>>>  hw/block/nvme.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>
>> Thanks! :)
>>
>> Reviewed-by: Max Reitz 
>>
> 
> Will Peter pick up fixes like this directly so we don't have to go
> through a pull request from nvme-next?

AFAIA, Peter only picks up build fixes.  Since the build wasn’t broken,
I think someone™ will have to send a pull request...

I understand you don’t necessarily want to be that someone, so I suppose
I might as well.

> Did I correctly annotate with "for-5.2"? :)

Yes!

Max

Re: VFIO Migration

[Stefan Hajnoczi]

On Tue, Nov 03, 2020 at 04:23:43PM +0100, Christophe de Dinechin wrote:
> On 2020-11-02 at 12:11 CET, Stefan Hajnoczi wrote...
> > There is discussion about VFIO migration in the "Re: Out-of-Process
> > Device Emulation session at KVM Forum 2020" thread. The current status
> > is that Kirti proposed a VFIO device region type for saving and loading
> > device state. There is currently no guidance on migrating between
> > different device versions or device implementations from different
> > vendors. This is known to be non-trivial and raised discussion about
> > whether it should really be handled by VFIO or centralized in QEMU.
> >
> > Below is a document that describes how to ensure migration compatibility
> > in VFIO. It does not require changes to the VFIO migration interface. It
> > can be used for both VFIO/mdev kernel devices and vfio-user devices.
> >
> > The idea is that the device state blob is opaque to the VMM but the same
> > level of migration compatibility that exists today is still available.
> >
> > I hope this will help us reach consensus and let us discuss specifics.
> >
> > If you followed the previous discussion, I changed the approach from
> > sending a magic constant in the device state blob to identifying device
> > models by URIs. Therefore the device state structure does not need to be
> > defined here - the critical information for ensuring device migration
> > compatibility is the device model and configuration defined below.
> >
> > Stefan
> > ---
> > VFIO Migration
> > ==
> > This document describes how to save and load VFIO device states. Saving a
> > device state produces a snapshot of a VFIO device's state that can be loaded
> > again at a later point in time to resume the device from the snapshot.
> >
> > The data representation of the device state is outside the scope of this
> > document.
> >
> > Overview
> > 
> > The purpose of device states is to save the device at a point in time and 
> > then
> > restore the device back to the saved state later. This is more challenging 
> > than
> > it first appears.
> >
> > The process of saving a device state and loading it later is called
> > *migration*. The state may be loaded by the same device that saved it or by 
> > a
> > new instance of the device, possibly running on a different computer.
> >
> > It must be possible to migrate to a newer implementation of the device
> > as well as to an older implementation of the device. This allows users
> > to upgrade and roll back their systems.
> >
> > Migration can fail if loading the device state is not possible. It should 
> > fail
> > early with a clear error message. It must not appear to complete but leave 
> > the
> > device inoperable due to a migration problem.
> >
> > The rest of this document describes how these requirements can be met.
> >
> > Device Models
> > -
> > Devices have a *hardware interface* consisting of hardware registers,
> > interrupts, and so on.
> >
> > The hardware interface together with the device state representation is 
> > called
> > a *device model*. Device models can be assigned URIs such as
> > https://qemu.org/devices/e1000e to uniquely identify them.
> 
> Like others, I think we should either
> 
> a) Give a relatively strong requirement regarding what is at the URL in
> question, e.g. docs, maybe even a machine-readable schema describing
> configuration and state for the device. Leaving the option "there can be
> nothing here" is IMO asking for trouble.
> 
> b) simply call that a unique ID, and then either drop the https: entirely or
> use something else, like pci:// or, to be more specific, vfio://
> 
> I'd favor option (b) for a different practical reason. URLs are subject to
> redirection and other mishaps. For example, using https:// begs the question
> whether
> https://qemu.org/devices/e1000e and
> https://www.qemu.org/devices/e1000e
> should be treated as the same device. I believe that your intent is that
> they shouldn't, but if the qemu web server redirects to www, and someone
> wants to copy-paste their web browser's URL bar to the command line, they'd
> get the wrong one.
> 
> 
> >
> > Multiple implementations of a device model may exist. They are they are
> 
> dup "they are"

Thanks, will fix.

> > interchangeable if they follow the same hardware interface and device
> > state representation.
> >
> > Multiple implementations of the same hardware interface may exist with
> > different device state representations, in which case the device models are 
> > not
> > interchangeable and must be assigned different URIs.
> >
> > Migration is only possible when the same device model is supported by the
> > *source* and the *destination* devices.
> >
> > Device Configuration
> > 
> 
> I find "device configuration" to be a bit confusing and ambiguous here.
> From the discussion, it appears that you are not talking about the active
> meaning of "configuration", as in "configuring" the device a

Re: [PATCH for-5.2 1/3] hw/block/nvme: fix null ns in register namespace

[Philippe Mathieu-Daudé]

On 11/4/20 11:22 AM, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Fix dereference after NULL check.
> 
> Reported-by: Coverity (CID 1436128)
> Fixes: b20804946bce ("hw/block/nvme: update nsid when registered")
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fa2cba744b57..080d782f1c2b 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -2562,8 +2562,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace 
> *ns, Error **errp)
>  
>  if (!nsid) {
>  for (int i = 1; i <= n->num_namespaces; i++) {
> -NvmeNamespace *ns = nvme_ns(n, i);
> -if (!ns) {
> +if (!nvme_ns(n, i)) {
>  nsid = ns->params.nsid = i;

Uh.

Reviewed-by: Philippe Mathieu-Daudé 

>  break;
>  }
> 

Re: VFIO Migration

[Stefan Hajnoczi]

On Tue, Nov 03, 2020 at 10:31:35AM -0700, Alex Williamson wrote:
> On Tue, 3 Nov 2020 15:33:56 +
> Daniel P. Berrangé  wrote:
> 
> > On Tue, Nov 03, 2020 at 04:23:43PM +0100, Christophe de Dinechin wrote:
> > > 
> > > On 2020-11-02 at 12:11 CET, Stefan Hajnoczi wrote...  
> > > > There is discussion about VFIO migration in the "Re: Out-of-Process
> > > > Device Emulation session at KVM Forum 2020" thread. The current status
> > > > is that Kirti proposed a VFIO device region type for saving and loading
> > > > device state. There is currently no guidance on migrating between
> > > > different device versions or device implementations from different
> > > > vendors. This is known to be non-trivial and raised discussion about
> > > > whether it should really be handled by VFIO or centralized in QEMU.
> > > >
> > > > Below is a document that describes how to ensure migration compatibility
> > > > in VFIO. It does not require changes to the VFIO migration interface. It
> > > > can be used for both VFIO/mdev kernel devices and vfio-user devices.
> > > >
> > > > The idea is that the device state blob is opaque to the VMM but the same
> > > > level of migration compatibility that exists today is still available.
> > > >
> > > > I hope this will help us reach consensus and let us discuss specifics.
> > > >
> > > > If you followed the previous discussion, I changed the approach from
> > > > sending a magic constant in the device state blob to identifying device
> > > > models by URIs. Therefore the device state structure does not need to be
> > > > defined here - the critical information for ensuring device migration
> > > > compatibility is the device model and configuration defined below.
> > > >
> > > > Stefan
> > > > ---
> > > > VFIO Migration
> > > > ==
> > > > This document describes how to save and load VFIO device states. Saving 
> > > > a
> > > > device state produces a snapshot of a VFIO device's state that can be 
> > > > loaded
> > > > again at a later point in time to resume the device from the snapshot.
> > > >
> > > > The data representation of the device state is outside the scope of this
> > > > document.
> > > >
> > > > Overview
> > > > 
> > > > The purpose of device states is to save the device at a point in time 
> > > > and then
> > > > restore the device back to the saved state later. This is more 
> > > > challenging than
> > > > it first appears.
> > > >
> > > > The process of saving a device state and loading it later is called
> > > > *migration*. The state may be loaded by the same device that saved it 
> > > > or by a
> > > > new instance of the device, possibly running on a different computer.
> > > >
> > > > It must be possible to migrate to a newer implementation of the device
> > > > as well as to an older implementation of the device. This allows users
> > > > to upgrade and roll back their systems.
> > > >
> > > > Migration can fail if loading the device state is not possible. It 
> > > > should fail
> > > > early with a clear error message. It must not appear to complete but 
> > > > leave the
> > > > device inoperable due to a migration problem.
> > > >
> > > > The rest of this document describes how these requirements can be met.
> > > >
> > > > Device Models
> > > > -
> > > > Devices have a *hardware interface* consisting of hardware registers,
> > > > interrupts, and so on.
> > > >
> > > > The hardware interface together with the device state representation is 
> > > > called
> > > > a *device model*. Device models can be assigned URIs such as
> > > > https://qemu.org/devices/e1000e to uniquely identify them.  
> > > 
> > > Like others, I think we should either
> > > 
> > > a) Give a relatively strong requirement regarding what is at the URL in
> > > question, e.g. docs, maybe even a machine-readable schema describing
> > > configuration and state for the device. Leaving the option "there can be
> > > nothing here" is IMO asking for trouble.
> > > 
> > > b) simply call that a unique ID, and then either drop the https: entirely 
> > > or
> > > use something else, like pci:// or, to be more specific, vfio://
> > > 
> > > I'd favor option (b) for a different practical reason. URLs are subject to
> > > redirection and other mishaps. For example, using https:// begs the 
> > > question
> > > whether
> > > https://qemu.org/devices/e1000e and
> > > https://www.qemu.org/devices/e1000e
> > > should be treated as the same device. I believe that your intent is that
> > > they shouldn't, but if the qemu web server redirects to www, and someone
> > > wants to copy-paste their web browser's URL bar to the command line, 
> > > they'd
> > > get the wrong one.  
> > 
> > That's not a real world problem IMHO, because neither of these URLs
> > ever need resolve to a real webpage, and thus not need to be cut +
> > paste from a browser.
> > 
> > They are simply expressing a resource identifier using a URI as a
> > convenient format. This is the same as an XML na

Re: [PATCH-for-5.2? 3/5] tests/acceptance: Skip incomplete virtio_version tests using '@skip'

[Thomas Huth]

On 02/11/2020 15.42, Philippe Mathieu-Daudé wrote:
> Prefer skipping incomplete tests with the "@skip" keyword,
> rather than commenting the code.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/acceptance/virtio_version.py | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/acceptance/virtio_version.py 
> b/tests/acceptance/virtio_version.py
> index 33593c29dd0..187bbfa1f42 100644
> --- a/tests/acceptance/virtio_version.py
> +++ b/tests/acceptance/virtio_version.py
> @@ -140,17 +140,20 @@ def check_all_variants(self, qemu_devtype, 
> virtio_devid):
>  self.assertIn('conventional-pci-device', trans_ifaces)
>  self.assertNotIn('pci-express-device', trans_ifaces)
>  
> +@skip("virtio-blk requires 'driver' parameter")

Shouldn't that be 'drive' instead of 'driver' ?

> +def test_conventional_devs_driver(self):
> +self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK)
> +
> +@skip("virtio-9p requires 'fsdev' parameter")
> +def test_conventional_devs_fsdev(self):
> +self.check_all_variants('virtio-9p-pci', VIRTIO_9P)
>  
>  def test_conventional_devs(self):
>  self.check_all_variants('virtio-net-pci', VIRTIO_NET)
> -# virtio-blk requires 'driver' parameter
> -#self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK)
>  self.check_all_variants('virtio-serial-pci', VIRTIO_CONSOLE)
>  self.check_all_variants('virtio-rng-pci', VIRTIO_RNG)
>  self.check_all_variants('virtio-balloon-pci', VIRTIO_BALLOON)
>  self.check_all_variants('virtio-scsi-pci', VIRTIO_SCSI)
> -# virtio-9p requires 'fsdev' parameter
> -#self.check_all_variants('virtio-9p-pci', VIRTIO_9P)

I think I'd prefer to keep the stuff commented ... otherwise it will show up
in the logs, giving the impression that you could run the tests somehow if
you just provided the right environment, which is just not possible right now.

 Thomas

Re: [PATCH for-5.2 0/3] hw/block/nvme: coverity fixes

[Max Reitz]

On 04.11.20 11:22, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Fix three issues reported by coverity (CIDs 1436128, 1436129 and
> 1436131).
> 
> Klaus Jensen (3):
>   hw/block/nvme: fix null ns in register namespace
>   hw/block/nvme: fix uint16_t use of uint32_t sgls member
>   hw/block/nvme: fix free of array-typed value
> 
>  hw/block/nvme.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)

Thanks again, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max

Re: [PATCH for-5.2 2/3] hw/block/nvme: fix uint16_t use of uint32_t sgls member

[Philippe Mathieu-Daudé]

On 11/4/20 11:22 AM, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> nvme_map_sgl_data erroneously uses the sgls member of NvmeIdNs as a
> uint16_t.
> 
> Reported-by: Coverity (CID 1436129)
> Fixes: cba0a8a344fe ("hw/block/nvme: add support for scatter gather lists")
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 080d782f1c2b..2bdc50eb6fce 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -452,7 +452,7 @@ static uint16_t nvme_map_sgl_data(NvmeCtrl *n, QEMUSGList 
> *qsg,
>   * segments and/or descriptors. The controller might accept
>   * ignoring the rest of the SGL.
>   */
> -uint16_t sgls = le16_to_cpu(n->id_ctrl.sgls);
> +uint32_t sgls = le32_to_cpu(n->id_ctrl.sgls);
>  if (sgls & NVME_CTRL_SGLS_EXCESS_LENGTH) {

I'm surprise the compiler doesn't warn here.

Reviewed-by: Philippe Mathieu-Daudé 

>  break;
>  }
> 

Re: [RFC PATCH 5/6] virtio-net: Added eBPF RSS to virtio-net.

[Daniel P . Berrangé]

On Wed, Nov 04, 2020 at 01:07:41PM +0200, Yuri Benditovich wrote:
> On Wed, Nov 4, 2020 at 5:09 AM Jason Wang  wrote:
> 
> >
> > On 2020/11/3 上午2:51, Andrew Melnychenko wrote:
> > > From: Andrew 
> > >
> > > When RSS is enabled the device tries to load the eBPF program
> > > to select RX virtqueue in the TUN. If eBPF can be loaded
> > > the RSS will function also with vhost (works with kernel 5.8 and later).
> > > Software RSS is used as a fallback with vhost=off when eBPF can't be
> > loaded
> > > or when hash population requested by the guest.
> > >
> > > Signed-off-by: Yuri Benditovich 
> > > Signed-off-by: Andrew Melnychenko 
> > > ---
> > >   hw/net/vhost_net.c |   2 +
> > >   hw/net/virtio-net.c| 120 +++--
> > >   include/hw/virtio/virtio-net.h |   4 ++
> > >   net/vhost-vdpa.c   |   2 +
> > >   4 files changed, 124 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index 24d555e764..16124f99c3 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -71,6 +71,8 @@ static const int user_feature_bits[] = {
> > >   VIRTIO_NET_F_MTU,
> > >   VIRTIO_F_IOMMU_PLATFORM,
> > >   VIRTIO_F_RING_PACKED,
> > > +VIRTIO_NET_F_RSS,
> > > +VIRTIO_NET_F_HASH_REPORT,
> > >
> > >   /* This bit implies RARP isn't sent by QEMU out of band */
> > >   VIRTIO_NET_F_GUEST_ANNOUNCE,
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 277289d56e..afcc3032ec 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -698,6 +698,19 @@ static void virtio_net_set_queues(VirtIONet *n)
> > >
> > >   static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
> > >
> > > +static uint64_t fix_ebpf_vhost_features(uint64_t features)
> > > +{
> > > +/* If vhost=on & CONFIG_EBPF doesn't set - disable RSS feature */
> > > +uint64_t ret = features;
> > > +#ifndef CONFIG_EBPF
> > > +virtio_clear_feature(&ret, VIRTIO_NET_F_RSS);
> > > +#endif
> > > +/* for now, there is no solution for populating the hash from eBPF
> > */
> > > +virtio_clear_feature(&ret, VIRTIO_NET_F_HASH_REPORT);
> >
> >
> > I think we probably need to to something reverse since RSS is under the
> > control on qemu cli, disable features like this may break migration.
> >
> >
> How by design we add new features to qemu in light of possible migration to
> older qemu version when the destination
> qemu does not support these features?

If the feature affects guest ABI, then we don't want to silently/
automatically turn on features that have a dependancy on kernel
features existing. They need to be an opt-in by mgmt app/admin.

IOW there needs to be an explicit property that is set to turn on use
of eBPF. If this property is set, then QEMU must use eBPF or fail
with an error. If it is unset, then QEMU must never use eBPF.

The mgmt app controlling QEMU will decide whether to use eBPF and
turn on the property, and will then know not to migrate it to a
host without eBPF support.


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

Re: [PATCH 1/4] bsd-user: space required after semicolon

[Philippe Mathieu-Daudé]

On 11/4/20 12:08 PM, Thomas Huth wrote:
> On 04/11/2020 11.20, shiliyang wrote:
>> This patch fixes error style problems found by checkpatch.pl:
>> ERROR: space required after that ';'
>>
>> Signed-off-by: Liyang Shi 
>>
>> ---
>>  bsd-user/elfload.c | 2 +-
>>  bsd-user/syscall.c | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
>> index 9f4210af9a..25e625d86b 100644
>> --- a/bsd-user/elfload.c
>> +++ b/bsd-user/elfload.c
>> @@ -1227,7 +1227,7 @@ int load_elf_binary(struct linux_binprm *bprm, struct 
>> target_pt_regs *regs,
>>  end_data = 0;
>>  interp_ex.a_info = 0;
>>
>> -for(i=0;i < elf_ex.e_phnum; i++) {
>> +for(i=0; i < elf_ex.e_phnum; i++) {
> 
> While you're at it, please also add white spaces around the "=" in that line.

IOW please run checkpatch after fixing a checkpatch issue ;)

Re: [PULL 0/6] Mips fixes patches

[Peter Maydell]

On Tue, 3 Nov 2020 at 17:33, Philippe Mathieu-Daudé  wrote:
>
> The following changes since commit 83851c7c60c90e9fb6a23ff48076387a77bc33cd:
>
>   Merge remote-tracking branch 'remotes/mdroth/tags/qga-pull-2020-10-27-v3-ta=
> g' into staging (2020-11-03 12:47:58 +)
>
> are available in the Git repository at:
>
>   https://gitlab.com/philmd/qemu.git tags/mips-fixes-20201103
>
> for you to fetch changes up to 8a805609d126ff2be9ad9ec118185dfc52633d6f:
>
>   target/mips: Add unaligned access support for MIPS64R6 and Loongson-3 (2020=
> -11-03 16:51:13 +0100)
>
> 
> MIPS patches queue
>
> - Removal of the 'r4k' machine (deprecated before 5.0)
> - Fix LGPL license text (Chetan Pant)
> - Support unaligned accesses on Loongson-3 (Huacai Chen)
> - Fix out-of-bound access in Loongson-3 embedded I/O interrupt
>   controller (Alex Chen)
>
> CI jobs results:
> . https://cirrus-ci.com/build/6324890389184512
> . https://gitlab.com/philmd/qemu/-/pipelines/211275262
> . https://travis-ci.org/github/philmd/qemu/builds/741188958


Applied, thanks.

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

-- PMM

Re: [PATCH-for-5.2? 3/5] tests/acceptance: Skip incomplete virtio_version tests using '@skip'

[Philippe Mathieu-Daudé]

On 11/4/20 12:13 PM, Thomas Huth wrote:
> On 02/11/2020 15.42, Philippe Mathieu-Daudé wrote:
>> Prefer skipping incomplete tests with the "@skip" keyword,
>> rather than commenting the code.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  tests/acceptance/virtio_version.py | 11 +++
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/acceptance/virtio_version.py 
>> b/tests/acceptance/virtio_version.py
>> index 33593c29dd0..187bbfa1f42 100644
>> --- a/tests/acceptance/virtio_version.py
>> +++ b/tests/acceptance/virtio_version.py
>> @@ -140,17 +140,20 @@ def check_all_variants(self, qemu_devtype, 
>> virtio_devid):
>>  self.assertIn('conventional-pci-device', trans_ifaces)
>>  self.assertNotIn('pci-express-device', trans_ifaces)
>>  
>> +@skip("virtio-blk requires 'driver' parameter")
> 
> Shouldn't that be 'drive' instead of 'driver' ?

No clue, this is the previous commented line.

> 
>> +def test_conventional_devs_driver(self):
>> +self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK)
>> +
>> +@skip("virtio-9p requires 'fsdev' parameter")
>> +def test_conventional_devs_fsdev(self):
>> +self.check_all_variants('virtio-9p-pci', VIRTIO_9P)
>>  
>>  def test_conventional_devs(self):
>>  self.check_all_variants('virtio-net-pci', VIRTIO_NET)
>> -# virtio-blk requires 'driver' parameter
>> -#self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK)
>>  self.check_all_variants('virtio-serial-pci', VIRTIO_CONSOLE)
>>  self.check_all_variants('virtio-rng-pci', VIRTIO_RNG)
>>  self.check_all_variants('virtio-balloon-pci', VIRTIO_BALLOON)
>>  self.check_all_variants('virtio-scsi-pci', VIRTIO_SCSI)
>> -# virtio-9p requires 'fsdev' parameter
>> -#self.check_all_variants('virtio-9p-pci', VIRTIO_9P)
> 
> I think I'd prefer to keep the stuff commented ... otherwise it will show up
> in the logs, giving the impression that you could run the tests somehow if
> you just provided the right environment, which is just not possible right now.

Well, we usually don't commit commented code like that.
If it is committed, it is important, then it has to show up in
the log. If you don't want it logged, why not remove it then?

Re: [PATCH-for-5.2 v2 2/4] hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen

[Paolo Bonzini]

On 04/11/20 09:43, Philippe Mathieu-Daudé wrote:

Fixes './configure --without-default-devices --enable-xen' build:

   /usr/bin/ld: libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function 
`xen_be_register_common':
   hw/xen/xen-legacy-backend.c:754: undefined reference to `xen_9pfs_ops'
   /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x8): undefined 
reference to `local_ops'
   /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x20): undefined 
reference to `synth_ops'
   /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x38): undefined 
reference to `proxy_ops'
   collect2: error: ld returned 1 exit status

Fixes: b2c00bce54c ("meson: convert hw/9pfs, cleanup")
Suggested-by: Paolo Bonzini
Signed-off-by: Philippe Mathieu-Daudé
---
I'm not sure b2c00bce54c is the real culprit


I think it is, probably a wrong conflict resolution.

Paolo

Re: [PATCH v2] qapi, qemu-options: make all parsing visitors parse boolean options the same

[Paolo Bonzini]

On 04/11/20 09:29, Markus Armbruster wrote:

  It only hurts in the odd case of a boolean option becoming on/off/auto
or on/off/split.

Another argument for deprecating values other than "on" and "off".


Unfortunately I'm fairly sure that I've seen yes/no in use.  I can buy 
insta-removal (not deprecation) of case-insensitivity.


Paolo

Re: [PATCH v2] qapi, qemu-options: make all parsing visitors parse boolean options the same

[Paolo Bonzini]

On 04/11/20 08:35, Markus Armbruster wrote:

+   "boolean (on/off, yes/no, true/false, y/n)");

Recommend to have the error message only mention the preferred form.  I
like the laconic "'on' or 'off'".  It's really all the user needs to
know.



I went for "boolean (on/off) value".



Avoiding the full_name() on success isn't hard:

   if (!qapi_bool_parse(name, str, obj, NULL)) {
   error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
  full_name(qiv, name), "'on' or 'off'");
   return false;
   }
   return true;


Can't refuse.



 case QEMU_OPT_BOOL:
-return parse_option_bool(opt->name, opt->str, &opt->value.boolean,
- errp);
+return qapi_bool_parse(opt->name, opt->str, &opt->value.boolean, errp);


Please break the line the same way as before.


Why? It's not even 80 characters.

Paolo

Re: [PATCH v2] qapi, qemu-options: make all parsing visitors parse boolean options the same

[Daniel P . Berrangé]

On Wed, Nov 04, 2020 at 12:31:40PM +0100, Paolo Bonzini wrote:
> On 04/11/20 09:29, Markus Armbruster wrote:
> > >   It only hurts in the odd case of a boolean option becoming on/off/auto
> > > or on/off/split.
> > Another argument for deprecating values other than "on" and "off".
> 
> Unfortunately I'm fairly sure that I've seen yes/no in use.  I can buy
> insta-removal (not deprecation) of case-insensitivity.

Seems a couple of example usages are my fault as I documented them :-(

docs/system/vnc-security.rst: -object 
tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \
docs/system/vnc-security.rst: -object 
tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \
docs/system/vnc-security.rst: -object 
tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \
include/authz/listfile.h: *   
filename=/etc/qemu/myvm-vnc.acl,refresh=yes
qemu-options.hx:``-object 
authz-listfile,id=id,filename=path,refresh=yes|no``
qemu-options.hx: -object 
authz-simple,id=auth0,filename=/etc/qemu/vnc-sasl.acl,refresh=yes \\

We should fix thos docs in QEMU at least, and unfortunately it seems I
missed that libvirt did use  verify-peer=yes


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] target/openrisc: Remove dead code attempting to check "is timer disabled"

[Stafford Horne]

On Wed, Nov 04, 2020 at 10:37:17AM +, Peter Maydell wrote:
> On Wed, 4 Nov 2020 at 07:10, Stafford Horne  wrote:
> >
> > On Tue, Nov 03, 2020 at 11:46:54AM +, Peter Maydell wrote:
> > > In the mtspr helper we attempt to check for "is the timer disabled"
> > > with "if (env->ttmr & TIMER_NONE)".  This is wrong because TIMER_NONE
> > > is zero and the condition is always false (Coverity complains about
> > > the dead code.)
> > >
> > > The correct check would be to test whether the TTMR_M field in the
> > > register is equal to TIMER_NONE instead.  However, the
> > > cpu_openrisc_timer_update() function checks whether the timer is
> > > enabled (it looks at cpu->env.is_counting, which is set to 0 via
> > > cpu_openrisc_count_stop() when the TTMR_M field is set to
> > > TIMER_NONE), so there's no need to check for "timer disabled" in the
> > > target/openrisc code.  Instead, simply remove the dead code.
> >
> > Thanks for the patch!
> >
> > I think the check is needed, but it's coded wrong.  The ttmr bits 31,30
> > are the timer mode.  If they are equal to zero (TIMER_NONE) then it means
> > the timer is disabled and we can skip the timer update.
> 
> My analysis is in the commit message -- the timer_update() function
> will just happily do nothing if the timer is disabled. It seemed
> cleaner to me to let the timer function just do the right thing
> rather than having both layers of the code try to figure out if
> there's no action to take.

If that's the case, then definitely remove it.  Sorry, I was just going off the
patch and didn't look into the code.

> > The code should be something like ((env->ttmr >> 30) == TIMER_NONE). But I
> > haven't tested it.
> 
> TIMER_NONE and the other TIMER_* fields are defined as (n << 30),
> and the mask TTMR_M is (3 << 30), so "(env->ttmr & TTMR_M) == TIMER_NONE"
> would be the condition to check if we wanted to do it here. (That
> matches how the code earlier in the function does it.)

Yeah, that is want I would want to do.  As I couldn't remember if there was a
mask variable available I just came up with the shift alternative.  Sorry, I was
in a bit of a hurry.

As for the patch.

Acked-by: Stafford Horne 

Re: [PATCH-for-5.2? 3/5] tests/acceptance: Skip incomplete virtio_version tests using '@skip'

[Thomas Huth]

On 04/11/2020 12.27, Philippe Mathieu-Daudé wrote:
> On 11/4/20 12:13 PM, Thomas Huth wrote:
>> On 02/11/2020 15.42, Philippe Mathieu-Daudé wrote:
>>> Prefer skipping incomplete tests with the "@skip" keyword,
>>> rather than commenting the code.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé 
>>> ---
>>>  tests/acceptance/virtio_version.py | 11 +++
>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/acceptance/virtio_version.py 
>>> b/tests/acceptance/virtio_version.py
>>> index 33593c29dd0..187bbfa1f42 100644
>>> --- a/tests/acceptance/virtio_version.py
>>> +++ b/tests/acceptance/virtio_version.py
>>> @@ -140,17 +140,20 @@ def check_all_variants(self, qemu_devtype, 
>>> virtio_devid):
>>>  self.assertIn('conventional-pci-device', trans_ifaces)
>>>  self.assertNotIn('pci-express-device', trans_ifaces)
>>>  
>>> +@skip("virtio-blk requires 'driver' parameter")
>>
>> Shouldn't that be 'drive' instead of 'driver' ?
> 
> No clue, this is the previous commented line.
> 
>>
>>> +def test_conventional_devs_driver(self):
>>> +self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK)
>>> +
>>> +@skip("virtio-9p requires 'fsdev' parameter")
>>> +def test_conventional_devs_fsdev(self):
>>> +self.check_all_variants('virtio-9p-pci', VIRTIO_9P)
>>>  
>>>  def test_conventional_devs(self):
>>>  self.check_all_variants('virtio-net-pci', VIRTIO_NET)
>>> -# virtio-blk requires 'driver' parameter
>>> -#self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK)
>>>  self.check_all_variants('virtio-serial-pci', VIRTIO_CONSOLE)
>>>  self.check_all_variants('virtio-rng-pci', VIRTIO_RNG)
>>>  self.check_all_variants('virtio-balloon-pci', VIRTIO_BALLOON)
>>>  self.check_all_variants('virtio-scsi-pci', VIRTIO_SCSI)
>>> -# virtio-9p requires 'fsdev' parameter
>>> -#self.check_all_variants('virtio-9p-pci', VIRTIO_9P)
>>
>> I think I'd prefer to keep the stuff commented ... otherwise it will show up
>> in the logs, giving the impression that you could run the tests somehow if
>> you just provided the right environment, which is just not possible right 
>> now.
> 
> Well, we usually don't commit commented code like that.
> If it is committed, it is important, then it has to show up in
> the log. If you don't want it logged, why not remove it then?

Then I'd rather remove it. Or leave a comment saying "we cannot exercise
virtio-9p-pci and virtio-blk-pci here (yet) since they need additional
parameters to be set".

 Thomas

Re: [PATCH-for-5.2 v2 3/4] gitlab-ci: Add a job to cover the --without-default-devices config

[Philippe Mathieu-Daudé]

On 11/4/20 10:17 AM, Thomas Huth wrote:
> On 04/11/2020 09.43, Philippe Mathieu-Daudé wrote:
>> We test './configure --without-default-devices' since commit
>> 20885b5b169 (".travis.yml: test that no-default-device builds
>> do not regress") in Travis-CI.
>>
>> Since having a single CI to look at is easier, and GitLab-CI
>> is the preferred one, add the equivalent job there.
>>
>> As smoke test, run the qtests on the AVR target. Since the
>> boards are simple SoC, there is not issue with unavailable
>> default devices there.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  .gitlab-ci.yml | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
>> index 3b15ae5c302..321cca2c216 100644
>> --- a/.gitlab-ci.yml
>> +++ b/.gitlab-ci.yml
>> @@ -262,6 +262,13 @@ build-user-plugins:
>>  MAKE_CHECK_ARGS: check-tcg
>>timeout: 1h 30m
>>  
>> +build-system-ubuntu-without-default-devices:
>> +  <<: *native_build_job_definition
>> +  variables:
>> +IMAGE: ubuntu2004
>> +CONFIGURE_ARGS: --without-default-devices --disable-user 
>> --disable-tools --disable-docs
>> +MAKE_CHECK_ARGS: check-qtest-avr
> 
> As mentioned in my other mail, we can also use -m68k if you prefer that
> instead of -avr.

I guess I send this series before reading it.
Let's use both, to support each others =)

> 
>>  build-clang:
>><<: *native_build_job_definition
>>variables:
>>
> 
> Reviewed-by: Thomas Huth 

Thanks!

Re: [RFC PATCH 0/6] eBPF RSS support for virtio-net

[Yuri Benditovich]

On Wed, Nov 4, 2020 at 4:08 AM Jason Wang  wrote:

>
> On 2020/11/3 下午6:32, Yuri Benditovich wrote:
> >
> >
> > On Tue, Nov 3, 2020 at 11:02 AM Jason Wang  > > wrote:
> >
> >
> > On 2020/11/3 上午2:51, Andrew Melnychenko wrote:
> > > Basic idea is to use eBPF to calculate and steer packets in TAP.
> > > RSS(Receive Side Scaling) is used to distribute network packets
> > to guest virtqueues
> > > by calculating packet hash.
> > > eBPF RSS allows us to use RSS with vhost TAP.
> > >
> > > This set of patches introduces the usage of eBPF for packet
> steering
> > > and RSS hash calculation:
> > > * RSS(Receive Side Scaling) is used to distribute network packets
> to
> > > guest virtqueues by calculating packet hash
> > > * eBPF RSS suppose to be faster than already existing 'software'
> > > implementation in QEMU
> > > * Additionally adding support for the usage of RSS with vhost
> > >
> > > Supported kernels: 5.8+
> > >
> > > Implementation notes:
> > > Linux TAP TUNSETSTEERINGEBPF ioctl was used to set the eBPF
> program.
> > > Added eBPF support to qemu directly through a system call, see the
> > > bpf(2) for details.
> > > The eBPF program is part of the qemu and presented as an array
> > of bpf
> > > instructions.
> > > The program can be recompiled by provided Makefile.ebpf(need to
> > adjust
> > > 'linuxhdrs'),
> > > although it's not required to build QEMU with eBPF support.
> > > Added changes to virtio-net and vhost, primary eBPF RSS is used.
> > > 'Software' RSS used in the case of hash population and as a
> > fallback option.
> > > For vhost, the hash population feature is not reported to the
> guest.
> > >
> > > Please also see the documentation in PATCH 6/6.
> > >
> > > I am sending those patches as RFC to initiate the discussions
> > and get
> > > feedback on the following points:
> > > * Fallback when eBPF is not supported by the kernel
> >
> >
> > Yes, and it could also a lacking of CAP_BPF.
> >
> >
> > > * Live migration to the kernel that doesn't have eBPF support
> >
> >
> > Is there anything that we needs special treatment here?
> >
> > Possible case: rss=on, vhost=on, source system with kernel 5.8
> > (everything works) -> dest. system 5.6 (bpf does not work), the
> > adapter functions, but all the steering does not use proper queues.
>
>
> Right, I think we need to disable vhost on dest.
>
>
Is this acceptable to disable vhost at time of migration?


> >
> >
> >
> > > * Integration with current QEMU build
> >
> >
> > Yes, a question here:
> >
> > 1) Any reason for not using libbpf, e.g it has been shipped with some
> > distros
> >
> >
> > We intentionally do not use libbpf, as it present only on some distros.
> > We can switch to libbpf, but this will disable bpf if libbpf is not
> > installed
>
>
> That's better I think.
>

We think the preferred way is to have an eBPF code built-in in QEMU (not
distribute it as a separate file).

Our initial idea was to not use the libbpf because it:
1. Does not create additional dependency during build time and during
run-time
2. Gives us smaller footprint of loadable eBPF blob inside qemu
3. Do not add too much code to QEMU

We can switch to libbpf, in this case:
1. Presence of dynamic library is not guaranteed on the target system
2. Static library is large
3. libbpf uses eBPF ELF which is significantly bigger than just the array
or instructions (May be we succeed to reduce the ELF to some suitable size
and still have it built-in)

Please let us know whether you still think libbpf is better and why.

Thanks


>
> > 2) It would be better if we can avoid shipping bytecodes
> >
> >
> >
> > This creates new dependencies: llvm + clang + ...
> > We would prefer byte code and ability to generate it if prerequisites
> > are installed.
>
>
> It's probably ok if we treat the bytecode as a kind of firmware.
>
> But in the long run, it's still worthwhile consider the qemu source is
> used for development and llvm/clang should be a common requirement for
> generating eBPF bytecode for host.
>
>
> >
> >
> > > * Additional usage for eBPF for packet filtering
> >
> >
> > Another interesting topics in to implement mac/vlan filters. And
> > in the
> > future, I plan to add mac based steering. All of these could be
> > done via
> > eBPF.
> >
> >
> > No problem, we can cooperate if needed
> >
> >
> > >
> > > Know issues:
> > > * hash population not supported by eBPF RSS: 'software' RSS used
> >
> >
> > Is this because there's not way to write to vnet header in
> > STERRING BPF?
> >
> > Yes. We plan to submit changes for kernel to cooperate with BPF and
> > populate the hash, this work is in progress
>
>
> That would require a new type of eBPF program and may need some work on
> verifier.
>
>
May be need to allow loa

[PATCH] qapi, qemu-options: make all parsing visitors parse boolean options the same

[Paolo Bonzini]

OptsVisitor, StringInputVisitor and the keyval visitor have
three different ideas of how a human could write the value of
a boolean option.  Pay homage to the backwards-compatibility
gods and make the new common helper accept all four sets (on/off,
true/false, y/n and yes/no), but remove case-insensitivity.

Since OptsVisitor is supposed to match qemu-options, adjust
it as well.

Signed-off-by: Paolo Bonzini 
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Markus Armbruster 
Message-Id: <20201103161339.447118-1-pbonz...@redhat.com>
Signed-off-by: Paolo Bonzini 
---
 include/qapi/util.h   |  2 ++
 qapi/opts-visitor.c   | 14 +-
 qapi/qapi-util.c  | 23 +++
 qapi/qobject-input-visitor.c  |  6 +-
 qapi/string-input-visitor.c   | 17 +
 tests/qemu-iotests/051.out|  6 +++---
 tests/qemu-iotests/051.pc.out |  6 +++---
 tests/qemu-iotests/133.out|  8 
 tests/qemu-iotests/137.out|  4 ++--
 util/qemu-option.c| 20 ++--
 10 files changed, 42 insertions(+), 64 deletions(-)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index bc312e90aa..6178e98e97 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -19,6 +19,8 @@ typedef struct QEnumLookup {
 const char *qapi_enum_lookup(const QEnumLookup *lookup, int val);
 int qapi_enum_parse(const QEnumLookup *lookup, const char *buf,
 int def, Error **errp);
+bool qapi_bool_parse(const char *name, const char *value, bool *obj,
+ Error **errp);
 
 int parse_qapi_name(const char *name, bool complete);
 
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 7781c23a42..587f31baf6 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -368,7 +368,6 @@ opts_type_str(Visitor *v, const char *name, char **obj, 
Error **errp)
 }
 
 
-/* mimics qemu-option.c::parse_option_bool() */
 static bool
 opts_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
 {
@@ -379,19 +378,8 @@ opts_type_bool(Visitor *v, const char *name, bool *obj, 
Error **errp)
 if (!opt) {
 return false;
 }
-
 if (opt->str) {
-if (strcmp(opt->str, "on") == 0 ||
-strcmp(opt->str, "yes") == 0 ||
-strcmp(opt->str, "y") == 0) {
-*obj = true;
-} else if (strcmp(opt->str, "off") == 0 ||
-strcmp(opt->str, "no") == 0 ||
-strcmp(opt->str, "n") == 0) {
-*obj = false;
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
-   "on|yes|y|off|no|n");
+if (!qapi_bool_parse(opt->name, opt->str, obj, errp)) {
 return false;
 }
 } else {
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 29a6c98b53..dfc0bde497 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/ctype.h"
+#include "qapi/qmp/qerror.h"
 
 const char *qapi_enum_lookup(const QEnumLookup *lookup, int val)
 {
@@ -40,6 +41,28 @@ int qapi_enum_parse(const QEnumLookup *lookup, const char 
*buf,
 return def;
 }
 
+bool qapi_bool_parse(const char *name, const char *value, bool *obj, Error 
**errp)
+{
+if (g_str_equal(value, "on") ||
+g_str_equal(value, "yes") ||
+g_str_equal(value, "true") ||
+g_str_equal(value, "y")) {
+*obj = true;
+return true;
+}
+if (g_str_equal(value, "off") ||
+g_str_equal(value, "no") ||
+g_str_equal(value, "false") ||
+g_str_equal(value, "n")) {
+*obj = false;
+return true;
+}
+
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
+   "boolean (on/off)");
+return false;
+}
+
 /*
  * Parse a valid QAPI name from @str.
  * A valid name consists of letters, digits, hyphen and underscore.
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 7b184b50a7..23843b242e 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -512,11 +512,7 @@ static bool qobject_input_type_bool_keyval(Visitor *v, 
const char *name,
 return false;
 }
 
-if (!strcmp(str, "on")) {
-*obj = true;
-} else if (!strcmp(str, "off")) {
-*obj = false;
-} else {
+if (!qapi_bool_parse(name, str, obj, NULL)) {
 error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
full_name(qiv, name), "'on' or 'off'");
 return false;
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 6e53396ea3..197139c1c0 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -332,22 +332,7 @@ static bool parse_type_bool(Visitor *v, const char *name, 
bool *obj,
 StringInputVisitor *siv = to_siv(v);
 
 assert(siv->lm == LM_NONE);
-if (!strcasecmp(siv->string, "on") ||
-!strcasecmp(siv->string, "yes") ||
-!strcasecmp(siv->string, "true"

[PATCH-for-5.2 v3 1/4] s390x: fix build for --without-default-devices

[Philippe Mathieu-Daudé]

From: Cornelia Huck 

s390-pci-vfio.c calls into the vfio code, so we need it to be
built conditionally on vfio (which implies CONFIG_LINUX).

Reported-by: Philippe Mathieu-Daudé 
Fixes: cd7498d07fbb ("s390x/pci: Add routine to get the vfio dma available 
count")
Signed-off-by: Cornelia Huck 
Message-Id: <20201103123237.718242-1-coh...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Acked-by: Greg Kurz 
Tested-by: Greg Kurz 
Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/s390x/s390-pci-vfio.h | 3 ++-
 hw/s390x/meson.build | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/hw/s390x/s390-pci-vfio.h b/include/hw/s390x/s390-pci-vfio.h
index c7984905b3b..ff708aef500 100644
--- a/include/hw/s390x/s390-pci-vfio.h
+++ b/include/hw/s390x/s390-pci-vfio.h
@@ -13,8 +13,9 @@
 #define HW_S390_PCI_VFIO_H
 
 #include "hw/s390x/s390-pci-bus.h"
+#include CONFIG_DEVICES
 
-#ifdef CONFIG_LINUX
+#ifdef CONFIG_VFIO
 bool s390_pci_update_dma_avail(int fd, unsigned int *avail);
 S390PCIDMACount *s390_pci_start_dma_count(S390pciState *s,
   S390PCIBusDevice *pbdev);
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index f4663a83551..2a7818d94b9 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -27,7 +27,7 @@
 ))
 s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: 
files('s390-virtio-ccw.c'))
 s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
-s390x_ss.add(when: 'CONFIG_LINUX', if_true: files('s390-pci-vfio.c'))
+s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
 
 virtio_ss = ss.source_set()
 virtio_ss.add(files('virtio-ccw.c'))
-- 
2.26.2

[PATCH-for-5.2 v3 2/4] hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen

[Philippe Mathieu-Daudé]

Commit b2c00bce54c ("meson: convert hw/9pfs, cleanup") introduced
CONFIG_9PFS (probably a wrong conflict resolution). This config is
not used anywhere. Backends depend on CONFIG_FSDEV_9P which itself
depends on CONFIG_VIRTFS.

Remove the invalid CONFIG_9PFS and use CONFIG_FSDEV_9P instead, to
fix the './configure --without-default-devices --enable-xen' build:

  /usr/bin/ld: libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function 
`xen_be_register_common':
  hw/xen/xen-legacy-backend.c:754: undefined reference to `xen_9pfs_ops'
  /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x8): undefined 
reference to `local_ops'
  /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x20): undefined 
reference to `synth_ops'
  /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x38): undefined 
reference to `proxy_ops'
  collect2: error: ld returned 1 exit status

Fixes: b2c00bce54c ("meson: convert hw/9pfs, cleanup")
Suggested-by: Paolo Bonzini 
Acked-by: Greg Kurz 
Tested-by: Greg Kurz 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Reworded description (Greg)

Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Paul Durrant 
Cc: xen-de...@lists.xenproject.org
Cc: Greg Kurz 
Cc: Christian Schoenebeck 
---
 hw/9pfs/Kconfig | 4 
 hw/9pfs/meson.build | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/9pfs/Kconfig b/hw/9pfs/Kconfig
index d3ebd737301..3ae57496613 100644
--- a/hw/9pfs/Kconfig
+++ b/hw/9pfs/Kconfig
@@ -2,12 +2,8 @@ config FSDEV_9P
 bool
 depends on VIRTFS
 
-config 9PFS
-bool
-
 config VIRTIO_9P
 bool
 default y
 depends on VIRTFS && VIRTIO
 select FSDEV_9P
-select 9PFS
diff --git a/hw/9pfs/meson.build b/hw/9pfs/meson.build
index cc094262122..99be5d91196 100644
--- a/hw/9pfs/meson.build
+++ b/hw/9pfs/meson.build
@@ -15,6 +15,6 @@
   'coxattr.c',
 ))
 fs_ss.add(when: 'CONFIG_XEN', if_true: files('xen-9p-backend.c'))
-softmmu_ss.add_all(when: 'CONFIG_9PFS', if_true: fs_ss)
+softmmu_ss.add_all(when: 'CONFIG_FSDEV_9P', if_true: fs_ss)
 
 specific_ss.add(when: 'CONFIG_VIRTIO_9P', if_true: files('virtio-9p-device.c'))
-- 
2.26.2

[PATCH-for-5.2 v3 0/4] ci: Move --without-default-devices job from Travis to GitLab

[Philippe Mathieu-Daudé]

We have a job covering the --without-default-devices configure
option on Travis-CI, but recommend the community to use GitLab,
so build failures are missed.

We need help to move the jobs to GitLab (we will keep the s390x
and ppc64 containerized jobs on Travis as there is no similar
offer on GitLab). Start with this single job.

Since v2:
- Run m68k qtests (Thomas)
- Reworded hw/9pfs description (Greg)
- Added tags

Since v1:
- Fix Xen+9pfs config (Paolo)
- Run AVR qtests (Thomas)

Cornelia Huck (1):
  s390x: fix build for --without-default-devices

Philippe Mathieu-Daudé (3):
  hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen
  gitlab-ci: Add a job to cover the --without-default-devices config
  travis-ci: Remove the --without-default-devices job

 include/hw/s390x/s390-pci-vfio.h | 3 ++-
 .gitlab-ci.yml   | 7 +++
 .travis.yml  | 8 
 hw/9pfs/Kconfig  | 4 
 hw/9pfs/meson.build  | 2 +-
 hw/s390x/meson.build | 2 +-
 6 files changed, 11 insertions(+), 15 deletions(-)

-- 
2.26.2

[PATCH-for-5.2 v3 3/4] gitlab-ci: Add a job to cover the --without-default-devices config

[Philippe Mathieu-Daudé]

We test './configure --without-default-devices' since commit
20885b5b169 (".travis.yml: test that no-default-device builds
do not regress") in Travis-CI.

Since having a single CI to look at is easier, and GitLab-CI
is the preferred one, add the equivalent job there.

As smoke test, run the qtests on the AVR target. Since the
boards are simple SoC, there is not issue with unavailable
default devices there.
Also include the m68k target which works fine.

Reviewed-by: Thomas Huth 
Signed-off-by: Philippe Mathieu-Daudé 
---
 .gitlab-ci.yml | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 3b15ae5c302..e4eba96ff34 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -262,6 +262,13 @@ build-user-plugins:
 MAKE_CHECK_ARGS: check-tcg
   timeout: 1h 30m
 
+build-system-ubuntu-without-default-devices:
+  <<: *native_build_job_definition
+  variables:
+IMAGE: ubuntu2004
+CONFIGURE_ARGS: --without-default-devices --disable-user --disable-tools 
--disable-docs
+MAKE_CHECK_ARGS: check-qtest-avr check-qtest-m68k
+
 build-clang:
   <<: *native_build_job_definition
   variables:
-- 
2.26.2

[PATCH-for-5.2 v3 4/4] travis-ci: Remove the --without-default-devices job

[Philippe Mathieu-Daudé]

We replicated the --without-default-devices job on GitLab-CI
in the previous commit. We can now remove it from Travis-CI.

Reviewed-by: Thomas Huth 
Signed-off-by: Philippe Mathieu-Daudé 
---
 .travis.yml | 8 
 1 file changed, 8 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index a3d78171cab..15d92291358 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -224,14 +224,6 @@ jobs:
 - ${SRC_DIR}/scripts/travis/coverage-summary.sh
 
 
-# We manually include builds which we disable "make check" for
-- name: "GCC without-default-devices (softmmu)"
-  env:
-- CONFIG="--without-default-devices --disable-user"
-- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
-- TEST_CMD=""
-
-
 # We don't need to exercise every backend with every front-end
 - name: "GCC trace log,simple,syslog (user)"
   env:
-- 
2.26.2

Re: [RFC PATCH 0/6] eBPF RSS support for virtio-net

[Daniel P . Berrangé]

On Wed, Nov 04, 2020 at 01:49:05PM +0200, Yuri Benditovich wrote:
> On Wed, Nov 4, 2020 at 4:08 AM Jason Wang  wrote:
> 
> >
> > On 2020/11/3 下午6:32, Yuri Benditovich wrote:
> > >
> > >
> > > On Tue, Nov 3, 2020 at 11:02 AM Jason Wang  > > > wrote:
> > >
> > >
> > > On 2020/11/3 上午2:51, Andrew Melnychenko wrote:
> > > > Basic idea is to use eBPF to calculate and steer packets in TAP.
> > > > RSS(Receive Side Scaling) is used to distribute network packets
> > > to guest virtqueues
> > > > by calculating packet hash.
> > > > eBPF RSS allows us to use RSS with vhost TAP.
> > > >
> > > > This set of patches introduces the usage of eBPF for packet
> > steering
> > > > and RSS hash calculation:
> > > > * RSS(Receive Side Scaling) is used to distribute network packets
> > to
> > > > guest virtqueues by calculating packet hash
> > > > * eBPF RSS suppose to be faster than already existing 'software'
> > > > implementation in QEMU
> > > > * Additionally adding support for the usage of RSS with vhost
> > > >
> > > > Supported kernels: 5.8+
> > > >
> > > > Implementation notes:
> > > > Linux TAP TUNSETSTEERINGEBPF ioctl was used to set the eBPF
> > program.
> > > > Added eBPF support to qemu directly through a system call, see the
> > > > bpf(2) for details.
> > > > The eBPF program is part of the qemu and presented as an array
> > > of bpf
> > > > instructions.
> > > > The program can be recompiled by provided Makefile.ebpf(need to
> > > adjust
> > > > 'linuxhdrs'),
> > > > although it's not required to build QEMU with eBPF support.
> > > > Added changes to virtio-net and vhost, primary eBPF RSS is used.
> > > > 'Software' RSS used in the case of hash population and as a
> > > fallback option.
> > > > For vhost, the hash population feature is not reported to the
> > guest.
> > > >
> > > > Please also see the documentation in PATCH 6/6.
> > > >
> > > > I am sending those patches as RFC to initiate the discussions
> > > and get
> > > > feedback on the following points:
> > > > * Fallback when eBPF is not supported by the kernel
> > >
> > >
> > > Yes, and it could also a lacking of CAP_BPF.
> > >
> > >
> > > > * Live migration to the kernel that doesn't have eBPF support
> > >
> > >
> > > Is there anything that we needs special treatment here?
> > >
> > > Possible case: rss=on, vhost=on, source system with kernel 5.8
> > > (everything works) -> dest. system 5.6 (bpf does not work), the
> > > adapter functions, but all the steering does not use proper queues.
> >
> >
> > Right, I think we need to disable vhost on dest.
> >
> >
> Is this acceptable to disable vhost at time of migration?
> 
> 
> > >
> > >
> > >
> > > > * Integration with current QEMU build
> > >
> > >
> > > Yes, a question here:
> > >
> > > 1) Any reason for not using libbpf, e.g it has been shipped with some
> > > distros
> > >
> > >
> > > We intentionally do not use libbpf, as it present only on some distros.
> > > We can switch to libbpf, but this will disable bpf if libbpf is not
> > > installed
> >
> >
> > That's better I think.
> >
> 
> We think the preferred way is to have an eBPF code built-in in QEMU (not
> distribute it as a separate file).
> 
> Our initial idea was to not use the libbpf because it:
> 1. Does not create additional dependency during build time and during
> run-time
> 2. Gives us smaller footprint of loadable eBPF blob inside qemu
> 3. Do not add too much code to QEMU
> 
> We can switch to libbpf, in this case:
> 1. Presence of dynamic library is not guaranteed on the target system

Again if a distro or users wants to use this feature in
QEMU they should be expected build the library.

> 2. Static library is large

QEMU doesn't support static linking for system emulators.  It may
happen to work at times but there's no expectations in this respect.

> 3. libbpf uses eBPF ELF which is significantly bigger than just the array
> or instructions (May be we succeed to reduce the ELF to some suitable size
> and still have it built-in)
> 
> Please let us know whether you still think libbpf is better and why.

It looks like both CLang and GCC compilers for BPF are moving towards
a world where they use BTF to get compile once, run everywhere portability
for the compiled bytecode. IIUC the libbpf is what is responsible for
processing the BTF data when loading it into the running kernel. This
all looks like a good thing in general. 

If we introduce BPF to QEMU without using libbpf, and then later decide
we absolutely need libbpf features, it creates an upgrade back compat
issue for existing deployments. It is better to use libbpf right from
the start, so we're set up to take full advantage of what it offers
long term.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt

[PULL 4/8] dev-serial: add trace-events for baud rate and data parameters

[Gerd Hoffmann]

From: Mark Cave-Ayland 

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Samuel Thibault 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20201027150456.24606-5-mark.cave-ayl...@ilande.co.uk
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/dev-serial.c | 3 +++
 hw/usb/trace-events | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index abc316c7bf1f..badf8785db46 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -307,6 +307,7 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 }
 
 s->params.speed = (4800 / 2) / (8 * divisor + subdivisor8);
+trace_usb_serial_set_baud(bus->busnr, dev->addr, s->params.speed);
 qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
 break;
 }
@@ -340,6 +341,8 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 goto fail;
 }
 
+trace_usb_serial_set_data(bus->busnr, dev->addr, s->params.parity,
+  s->params.data_bits, s->params.stop_bits);
 qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
 /* TODO: TX ON/OFF */
 break;
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index 89f1c351e339..98ee1c54627d 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -328,3 +328,5 @@ usb_serial_unsupported_parity(int bus, int addr, int value) 
"dev %d:%u unsupport
 usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%u 
unsupported stop bits %d"
 usb_serial_unsupported_control(int bus, int addr, int request, int value) "dev 
%d:%u got unsupported/bogus control 0x%x, value 0x%x"
 usb_serial_bad_token(int bus, int addr) "dev %d:%u bad token"
+usb_serial_set_baud(int bus, int addr, int baud) "dev %d:%u baud rate %d"
+usb_serial_set_data(int bus, int addr, int parity, int data, int stop) "dev 
%d:%u parity %c, data bits %d, stop bits %d"
-- 
2.27.0

[PULL 2/8] dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments

[Gerd Hoffmann]

From: Mark Cave-Ayland 

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Samuel Thibault 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20201027150456.24606-3-mark.cave-ayl...@ilande.co.uk
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/dev-serial.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 7a5fa3770e7f..77ce89d38b27 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -204,7 +204,7 @@ static void usb_serial_reset(USBSerialState *s)
 
 static void usb_serial_handle_reset(USBDevice *dev)
 {
-USBSerialState *s = (USBSerialState *)dev;
+USBSerialState *s = USB_SERIAL(dev);
 
 DPRINTF("Reset\n");
 
@@ -243,7 +243,7 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
   int request, int value, int index,
   int length, uint8_t *data)
 {
-USBSerialState *s = (USBSerialState *)dev;
+USBSerialState *s = USB_SERIAL(dev);
 int ret;
 
 DPRINTF("got control %x, value %x\n", request, value);
@@ -430,7 +430,7 @@ static void usb_serial_token_in(USBSerialState *s, 
USBPacket *p)
 
 static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
 {
-USBSerialState *s = (USBSerialState *)dev;
+USBSerialState *s = USB_SERIAL(dev);
 uint8_t devep = p->ep->nr;
 struct iovec *iov;
 int i;
-- 
2.27.0

[PULL 0/8] Usb 20201104 patches

[Gerd Hoffmann]

The following changes since commit 3d6e32347a3b57dac7f469a07c5f520e69bd070a:

  Update version for v5.2.0-rc0 release (2020-11-03 21:11:57 +)

are available in the Git repository at:

  git://git.kraxel.org/qemu tags/usb-20201104-pull-request

for you to fetch changes up to 963a7bed570ce12604a48755c78244a2b6e179b3:

  dev-serial: store flow control and xon/xoff characters (2020-11-04 07:22:37 
+0100)


usb: bugfixes for usb-serial



Mark Cave-Ayland (8):
  dev-serial: style changes to improve readability and checkpatch fixes
  dev-serial: use USB_SERIAL QOM macro for USBSerialState assignments
  dev-serial: convert from DPRINTF to trace-events
  dev-serial: add trace-events for baud rate and data parameters
  dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent
macros from usb.h
  dev-serial: add always-plugged property to ensure USB device is always
attached
  dev-serial: add support for setting data_bits in QEMUSerialSetParams
  dev-serial: store flow control and xon/xoff characters

 hw/usb/dev-serial.c | 334 +++-
 hw/usb/trace-events |  13 ++
 2 files changed, 216 insertions(+), 131 deletions(-)

-- 
2.27.0

[PULL 8/8] dev-serial: store flow control and xon/xoff characters

[Gerd Hoffmann]

From: Mark Cave-Ayland 

Note that whilst the device does not do anything with these values, they are
logged with trace events and stored to allow future implementation.

The default flow control is set to none at reset as documented in the Linux
ftdi_sio.h header file.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Samuel Thibault 
Message-id: 20201027150456.24606-9-mark.cave-ayl...@ilande.co.uk
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/dev-serial.c | 38 +++---
 hw/usb/trace-events |  2 ++
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index e42ce362956b..19e1933f0496 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -52,6 +52,7 @@
 
 /* SET_FLOW_CTRL */
 
+#define FTDI_NO_HS 0
 #define FTDI_RTS_CTS_HS1
 #define FTDI_DTR_DSR_HS2
 #define FTDI_XON_XOFF_HS   4
@@ -98,6 +99,9 @@ struct USBSerialState {
 uint8_t error_chr;
 uint8_t event_trigger;
 bool always_plugged;
+uint8_t flow_control;
+uint8_t xon;
+uint8_t xoff;
 QEMUSerialSetParams params;
 int latency;/* ms */
 CharBackend cs;
@@ -181,14 +185,36 @@ static const USBDesc desc_braille = {
 .str  = desc_strings,
 };
 
+static void usb_serial_set_flow_control(USBSerialState *s,
+uint8_t flow_control)
+{
+USBDevice *dev = USB_DEVICE(s);
+USBBus *bus = usb_bus_from_device(dev);
+
+/* TODO: ioctl */
+s->flow_control = flow_control;
+trace_usb_serial_set_flow_control(bus->busnr, dev->addr, flow_control);
+}
+
+static void usb_serial_set_xonxoff(USBSerialState *s, int xonxoff)
+{
+USBDevice *dev = USB_DEVICE(s);
+USBBus *bus = usb_bus_from_device(dev);
+
+s->xon = xonxoff & 0xff;
+s->xoff = (xonxoff >> 8) & 0xff;
+
+trace_usb_serial_set_xonxoff(bus->busnr, dev->addr, s->xon, s->xoff);
+}
+
 static void usb_serial_reset(USBSerialState *s)
 {
-/* TODO: Set flow control to none */
 s->event_chr = 0x0d;
 s->event_trigger = 0;
 s->recv_ptr = 0;
 s->recv_used = 0;
 /* TODO: purge in char driver */
+usb_serial_set_flow_control(s, FTDI_NO_HS);
 }
 
 static void usb_serial_handle_reset(USBDevice *dev)
@@ -285,9 +311,15 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_TIOCM, &flags);
 break;
 }
-case VendorDeviceOutRequest | FTDI_SET_FLOW_CTRL:
-/* TODO: ioctl */
+case VendorDeviceOutRequest | FTDI_SET_FLOW_CTRL: {
+uint8_t flow_control = index >> 8;
+
+usb_serial_set_flow_control(s, flow_control);
+if (flow_control & FTDI_XON_XOFF_HS) {
+usb_serial_set_xonxoff(s, value);
+}
 break;
+}
 case VendorDeviceOutRequest | FTDI_SET_BAUD: {
 static const int subdivisors8[8] = { 0, 4, 2, 1, 3, 5, 6, 7 };
 int subdivisor8 = subdivisors8[((value & 0xc000) >> 14)
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index 109da521cf4d..a3292d46248f 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -331,3 +331,5 @@ usb_serial_unsupported_data_bits(int bus, int addr, int 
value) "dev %d:%u unsupp
 usb_serial_bad_token(int bus, int addr) "dev %d:%u bad token"
 usb_serial_set_baud(int bus, int addr, int baud) "dev %d:%u baud rate %d"
 usb_serial_set_data(int bus, int addr, int parity, int data, int stop) "dev 
%d:%u parity %c, data bits %d, stop bits %d"
+usb_serial_set_flow_control(int bus, int addr, int index) "dev %d:%u flow 
control %d"
+usb_serial_set_xonxoff(int bus, int addr, uint8_t xon, uint8_t xoff) "dev 
%d:%u xon 0x%x xoff 0x%x"
-- 
2.27.0

[PULL 3/8] dev-serial: convert from DPRINTF to trace-events

[Gerd Hoffmann]

From: Mark Cave-Ayland 

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Samuel Thibault 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20201027150456.24606-4-mark.cave-ayl...@ilande.co.uk
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/dev-serial.c | 28 ++--
 hw/usb/trace-events |  8 
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 77ce89d38b27..abc316c7bf1f 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -20,15 +20,8 @@
 #include "chardev/char-serial.h"
 #include "chardev/char-fe.h"
 #include "qom/object.h"
+#include "trace.h"
 
-//#define DEBUG_Serial
-
-#ifdef DEBUG_Serial
-#define DPRINTF(fmt, ...) \
-do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) do {} while(0)
-#endif
 
 #define RECV_BUF (512 - (2 * 8))
 
@@ -205,8 +198,9 @@ static void usb_serial_reset(USBSerialState *s)
 static void usb_serial_handle_reset(USBDevice *dev)
 {
 USBSerialState *s = USB_SERIAL(dev);
+USBBus *bus = usb_bus_from_device(dev);
 
-DPRINTF("Reset\n");
+trace_usb_serial_reset(bus->busnr, dev->addr);
 
 usb_serial_reset(s);
 /* TODO: Reset char device, send BREAK? */
@@ -244,9 +238,11 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
   int length, uint8_t *data)
 {
 USBSerialState *s = USB_SERIAL(dev);
+USBBus *bus = usb_bus_from_device(dev);
 int ret;
 
-DPRINTF("got control %x, value %x\n", request, value);
+trace_usb_serial_handle_control(bus->busnr, dev->addr, request, value);
+
 ret = usb_desc_handle_control(dev, p, request, value, index, length, data);
 if (ret >= 0) {
 return;
@@ -326,7 +322,8 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 s->params.parity = 'E';
 break;
 default:
-DPRINTF("unsupported parity %d\n", value & FTDI_PARITY);
+trace_usb_serial_unsupported_parity(bus->busnr, dev->addr,
+value & FTDI_PARITY);
 goto fail;
 }
 
@@ -338,7 +335,8 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 s->params.stop_bits = 2;
 break;
 default:
-DPRINTF("unsupported stop bits %d\n", value & FTDI_STOP);
+trace_usb_serial_unsupported_stopbits(bus->busnr, dev->addr,
+  value & FTDI_STOP);
 goto fail;
 }
 
@@ -367,7 +365,8 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 break;
 default:
 fail:
-DPRINTF("got unsupported/bogus control %x, value %x\n", request, 
value);
+trace_usb_serial_unsupported_control(bus->busnr, dev->addr, request,
+ value);
 p->status = USB_RET_STALL;
 break;
 }
@@ -431,6 +430,7 @@ static void usb_serial_token_in(USBSerialState *s, 
USBPacket *p)
 static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
 {
 USBSerialState *s = USB_SERIAL(dev);
+USBBus *bus = usb_bus_from_device(dev);
 uint8_t devep = p->ep->nr;
 struct iovec *iov;
 int i;
@@ -459,7 +459,7 @@ static void usb_serial_handle_data(USBDevice *dev, 
USBPacket *p)
 break;
 
 default:
-DPRINTF("Bad token\n");
+trace_usb_serial_bad_token(bus->busnr, dev->addr);
 fail:
 p->status = USB_RET_STALL;
 break;
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index 72e4298780b7..89f1c351e339 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -320,3 +320,11 @@ usb_host_parse_interface(int bus, int addr, int num, int 
alt, int active) "dev %
 usb_host_parse_endpoint(int bus, int addr, int ep, const char *dir, const char 
*type, int active) "dev %d:%d, ep %d, %s, %s, active %d"
 usb_host_parse_error(int bus, int addr, const char *errmsg) "dev %d:%d, msg %s"
 usb_host_remote_wakeup_removed(int bus, int addr) "dev %d:%d"
+
+# dev-serial.c
+usb_serial_reset(int bus, int addr) "dev %d:%u reset"
+usb_serial_handle_control(int bus, int addr, int request, int value) "dev 
%d:%u got control 0x%x, value 0x%x"
+usb_serial_unsupported_parity(int bus, int addr, int value) "dev %d:%u 
unsupported parity %d"
+usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%u 
unsupported stop bits %d"
+usb_serial_unsupported_control(int bus, int addr, int request, int value) "dev 
%d:%u got unsupported/bogus control 0x%x, value 0x%x"
+usb_serial_bad_token(int bus, int addr) "dev %d:%u bad token"
-- 
2.27.0

[PULL 5/8] dev-serial: replace DeviceOutVendor/DeviceInVendor with equivalent macros from usb.h

[Gerd Hoffmann]

From: Mark Cave-Ayland 

The DeviceOutVendor and DeviceInVendor macros can be replaced with their
equivalent VendorDeviceOutRequest and VendorDeviceRequest macros from usb.h.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Samuel Thibault 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20201027150456.24606-6-mark.cave-ayl...@ilande.co.uk
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/dev-serial.c | 25 ++---
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index badf8785db46..92c35615eb13 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -37,11 +37,6 @@
 #define FTDI_SET_LATENCY   9
 #define FTDI_GET_LATENCY   10
 
-#define DeviceOutVendor \
-   ((USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
-#define DeviceInVendor \
-   ((USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
-
 /* RESET */
 
 #define FTDI_RESET_SIO 0
@@ -253,7 +248,7 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 break;
 
 /* Class specific requests.  */
-case DeviceOutVendor | FTDI_RESET:
+case VendorDeviceOutRequest | FTDI_RESET:
 switch (value) {
 case FTDI_RESET_SIO:
 usb_serial_reset(s);
@@ -268,7 +263,7 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 break;
 }
 break;
-case DeviceOutVendor | FTDI_SET_MDM_CTRL:
+case VendorDeviceOutRequest | FTDI_SET_MDM_CTRL:
 {
 static int flags;
 qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_GET_TIOCM, &flags);
@@ -289,10 +284,10 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_TIOCM, &flags);
 break;
 }
-case DeviceOutVendor | FTDI_SET_FLOW_CTRL:
+case VendorDeviceOutRequest | FTDI_SET_FLOW_CTRL:
 /* TODO: ioctl */
 break;
-case DeviceOutVendor | FTDI_SET_BAUD: {
+case VendorDeviceOutRequest | FTDI_SET_BAUD: {
 static const int subdivisors8[8] = { 0, 4, 2, 1, 3, 5, 6, 7 };
 int subdivisor8 = subdivisors8[((value & 0xc000) >> 14)
  | ((index & 1) << 2)];
@@ -311,7 +306,7 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
 break;
 }
-case DeviceOutVendor | FTDI_SET_DATA:
+case VendorDeviceOutRequest | FTDI_SET_DATA:
 switch (value & FTDI_PARITY) {
 case 0:
 s->params.parity = 'N';
@@ -346,23 +341,23 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 qemu_chr_fe_ioctl(&s->cs, CHR_IOCTL_SERIAL_SET_PARAMS, &s->params);
 /* TODO: TX ON/OFF */
 break;
-case DeviceInVendor | FTDI_GET_MDM_ST:
+case VendorDeviceRequest | FTDI_GET_MDM_ST:
 data[0] = usb_get_modem_lines(s) | 1;
 data[1] = FTDI_THRE | FTDI_TEMT;
 p->actual_length = 2;
 break;
-case DeviceOutVendor | FTDI_SET_EVENT_CHR:
+case VendorDeviceOutRequest | FTDI_SET_EVENT_CHR:
 /* TODO: handle it */
 s->event_chr = value;
 break;
-case DeviceOutVendor | FTDI_SET_ERROR_CHR:
+case VendorDeviceOutRequest | FTDI_SET_ERROR_CHR:
 /* TODO: handle it */
 s->error_chr = value;
 break;
-case DeviceOutVendor | FTDI_SET_LATENCY:
+case VendorDeviceOutRequest | FTDI_SET_LATENCY:
 s->latency = value;
 break;
-case DeviceInVendor | FTDI_GET_LATENCY:
+case VendorDeviceRequest | FTDI_GET_LATENCY:
 data[0] = s->latency;
 p->actual_length = 1;
 break;
-- 
2.27.0

[PULL 1/8] dev-serial: style changes to improve readability and checkpatch fixes

[Gerd Hoffmann]

From: Mark Cave-Ayland 

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Samuel Thibault 
Message-id: 20201027150456.24606-2-mark.cave-ayl...@ilande.co.uk
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/dev-serial.c | 228 
 1 file changed, 125 insertions(+), 103 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index b1622b7c7f94..7a5fa3770e7f 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -33,72 +33,75 @@ do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while 
(0)
 #define RECV_BUF (512 - (2 * 8))
 
 /* Commands */
-#define FTDI_RESET 0
-#define FTDI_SET_MDM_CTRL  1
-#define FTDI_SET_FLOW_CTRL 2
-#define FTDI_SET_BAUD  3
-#define FTDI_SET_DATA  4
-#define FTDI_GET_MDM_ST5
-#define FTDI_SET_EVENT_CHR 6
-#define FTDI_SET_ERROR_CHR 7
-#define FTDI_SET_LATENCY   9
-#define FTDI_GET_LATENCY   10
+#define FTDI_RESET 0
+#define FTDI_SET_MDM_CTRL  1
+#define FTDI_SET_FLOW_CTRL 2
+#define FTDI_SET_BAUD  3
+#define FTDI_SET_DATA  4
+#define FTDI_GET_MDM_ST5
+#define FTDI_SET_EVENT_CHR 6
+#define FTDI_SET_ERROR_CHR 7
+#define FTDI_SET_LATENCY   9
+#define FTDI_GET_LATENCY   10
 
-#define DeviceOutVendor
((USB_DIR_OUT|USB_TYPE_VENDOR|USB_RECIP_DEVICE)<<8)
-#define DeviceInVendor ((USB_DIR_IN |USB_TYPE_VENDOR|USB_RECIP_DEVICE)<<8)
+#define DeviceOutVendor \
+   ((USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
+#define DeviceInVendor \
+   ((USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE) << 8)
 
 /* RESET */
 
-#define FTDI_RESET_SIO 0
-#define FTDI_RESET_RX  1
-#define FTDI_RESET_TX  2
+#define FTDI_RESET_SIO 0
+#define FTDI_RESET_RX  1
+#define FTDI_RESET_TX  2
 
 /* SET_MDM_CTRL */
 
-#define FTDI_DTR   1
-#define FTDI_SET_DTR   (FTDI_DTR << 8)
-#define FTDI_RTS   2
-#define FTDI_SET_RTS   (FTDI_RTS << 8)
+#define FTDI_DTR   1
+#define FTDI_SET_DTR   (FTDI_DTR << 8)
+#define FTDI_RTS   2
+#define FTDI_SET_RTS   (FTDI_RTS << 8)
 
 /* SET_FLOW_CTRL */
 
-#define FTDI_RTS_CTS_HS1
-#define FTDI_DTR_DSR_HS2
-#define FTDI_XON_XOFF_HS   4
+#define FTDI_RTS_CTS_HS1
+#define FTDI_DTR_DSR_HS2
+#define FTDI_XON_XOFF_HS   4
 
 /* SET_DATA */
 
-#define FTDI_PARITY(0x7 << 8)
-#define FTDI_ODD   (0x1 << 8)
-#define FTDI_EVEN  (0x2 << 8)
-#define FTDI_MARK  (0x3 << 8)
-#define FTDI_SPACE (0x4 << 8)
+#define FTDI_PARITY(0x7 << 8)
+#define FTDI_ODD   (0x1 << 8)
+#define FTDI_EVEN  (0x2 << 8)
+#define FTDI_MARK  (0x3 << 8)
+#define FTDI_SPACE (0x4 << 8)
 
-#define FTDI_STOP  (0x3 << 11)
-#define FTDI_STOP1 (0x0 << 11)
-#define FTDI_STOP15(0x1 << 11)
-#define FTDI_STOP2 (0x2 << 11)
+#define FTDI_STOP  (0x3 << 11)
+#define FTDI_STOP1 (0x0 << 11)
+#define FTDI_STOP15(0x1 << 11)
+#define FTDI_STOP2 (0x2 << 11)
 
 /* GET_MDM_ST */
 /* TODO: should be sent every 40ms */
-#define FTDI_CTS  (1<<4)// CTS line status
-#define FTDI_DSR  (1<<5)// DSR line status
-#define FTDI_RI   (1<<6)// RI line status
-#define FTDI_RLSD (1<<7)// Receive Line Signal Detect
+#define FTDI_CTS   (1 << 4)/* CTS line status */
+#define FTDI_DSR   (1 << 5)/* DSR line status */
+#define FTDI_RI(1 << 6)/* RI line status */
+#define FTDI_RLSD  (1 << 7)/* Receive Line Signal Detect */
 
 /* Status */
 
-#define FTDI_DR   (1<<0)// Data Ready
-#define FTDI_OE   (1<<1)// Overrun Err
-#define FTDI_PE   (1<<2)// Parity Err
-#define FTDI_FE   (1<<3)// Framing Err
-#define FTDI_BI   (1<<4)// Break Interrupt
-#define FTDI_THRE (1<<5)// Transmitter Holding Register
-#define FTDI_TEMT (1<<6)// Transmitter Empty
-#define FTDI_FIFO (1<<7)// Error in FIFO
+#define FTDI_DR(1 << 0)/* Data Ready */
+#define FTDI_OE(1 << 1)/* Overrun Err */
+#define FTDI_PE(1 << 2)/* Parity Err */
+#define FTDI_FE(1 << 3)/* Framing Err */
+#define FTDI_BI(1 << 4)/* Break Interrupt */
+#define FTDI_THRE  (1 << 5)/* Transmitter Holding Register */
+#define FTDI_TEMT  (1 << 6)/* Transmitter Empty */
+#define FTDI_FIFO  (1 << 7)/* Error in FIFO */
 
 struct USBSerialState {
 USBDevice dev;
+
 USBEndpoint *intr;
 uint8_t recv_buf[RECV_BUF];
 uint16_t recv_ptr;
@@ -216,29 +219,34 @@ static uint8_t usb_get_modem_lines(USBSerialState *s)
 
 if (qemu_chr_fe_ioctl(&s->cs,
   CHR_IOCTL_SERIAL_GET_TIOCM, &flags) == -ENOTSUP) {
-return FTDI_CTS|FTDI_DSR|FTDI_RLSD;
+return FTDI_CTS | FTDI_DSR | FTDI_RLSD;
 }
 
 ret = 0;
-if (flags & CHR_TIOCM_CTS)
+if (flags & CHR_TIOCM_CTS) {
 ret |= FTDI_CTS;
-if (flags & CHR_TIOCM_DSR)
+}
+if (flags & CHR_TIOCM_DSR) {
 ret |= FTDI_DSR;
-if (flags & 

[PULL 7/8] dev-serial: add support for setting data_bits in QEMUSerialSetParams

[Gerd Hoffmann]

From: Mark Cave-Ayland 

Also implement the behaviour reported in Linux's ftdi_sio.c whereby if an 
invalid
data_bits value is provided then the hardware defaults to using 8.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Samuel Thibault 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20201027150456.24606-8-mark.cave-ayl...@ilande.co.uk
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/dev-serial.c | 17 +
 hw/usb/trace-events |  1 +
 2 files changed, 18 insertions(+)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index b9e308dca198..e42ce362956b 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -308,6 +308,23 @@ static void usb_serial_handle_control(USBDevice *dev, 
USBPacket *p,
 break;
 }
 case VendorDeviceOutRequest | FTDI_SET_DATA:
+switch (value & 0xff) {
+case 7:
+s->params.data_bits = 7;
+break;
+case 8:
+s->params.data_bits = 8;
+break;
+default:
+/*
+ * According to a comment in Linux's ftdi_sio.c original FTDI
+ * chips fall back to 8 data bits for unsupported data_bits
+ */
+trace_usb_serial_unsupported_data_bits(bus->busnr, dev->addr,
+   value & 0xff);
+s->params.data_bits = 8;
+}
+
 switch (value & FTDI_PARITY) {
 case 0:
 s->params.parity = 'N';
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index 98ee1c54627d..109da521cf4d 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -327,6 +327,7 @@ usb_serial_handle_control(int bus, int addr, int request, 
int value) "dev %d:%u
 usb_serial_unsupported_parity(int bus, int addr, int value) "dev %d:%u 
unsupported parity %d"
 usb_serial_unsupported_stopbits(int bus, int addr, int value) "dev %d:%u 
unsupported stop bits %d"
 usb_serial_unsupported_control(int bus, int addr, int request, int value) "dev 
%d:%u got unsupported/bogus control 0x%x, value 0x%x"
+usb_serial_unsupported_data_bits(int bus, int addr, int value) "dev %d:%u 
unsupported data bits %d, falling back to 8"
 usb_serial_bad_token(int bus, int addr) "dev %d:%u bad token"
 usb_serial_set_baud(int bus, int addr, int baud) "dev %d:%u baud rate %d"
 usb_serial_set_data(int bus, int addr, int parity, int data, int stop) "dev 
%d:%u parity %c, data bits %d, stop bits %d"
-- 
2.27.0

Re: [PATCH for-5.2 2/4] qemu-option: move help handling to get_opt_name_value

[Markus Armbruster]

Paolo Bonzini  writes:

> Right now, help options are parsed normally and then checked
> specially in opt_validate---but only if coming from
> qemu_opts_parse or qemu_opts_parse_noisily, not if coming
> from qemu_opt_set.
>
> Instead, move the check from opt_validate to the common workhorses
> of qemu_opts_parse and qemu_opts_parse_noisily, opts_do_parse and
> get_opt_name_value.
>
> This will come in handy in a subsequent patch, which will
> raise a warning for "-object memory-backend-ram,share"
> ("flag" option with no =on/=off part) but not for
> "-object memory-backend-ram,help".
>
> Signed-off-by: Paolo Bonzini 

I'm afraid this fails my smoke test:

$ o=`sed -n '/HAS_ARG,/s/DEF("\([^"]*\)".*/\1/p' qemu-options.hx`
$ for i in $o; do echo "= $i"; upstream-qemu -$i help -version; done 2>&1 | 
egrep -v 'QEMU emulator|Copyright'

Many output differences.  False positives due to help printing lists in
random order.  Arbitrarily picked true positive:

$ upstream-qemu -msg help
msg options:
  guest-name= - Prepends guest name for error messages but 
only if -name guest is set otherwise option is ignored

  timestamp=
$ echo $?
1

regresses to silent failure.

[PULL 6/8] dev-serial: add always-plugged property to ensure USB device is always attached

[Gerd Hoffmann]

From: Mark Cave-Ayland 

Some operating systems will generate a new device ID when a USB device is 
unplugged
and then replugged into the USB. If this is done whilst switching between 
multiple
applications over a virtual serial port, the change of device ID requires going
back into the OS/application to locate the new device accordingly.

Add a new always-plugged property that if specified will ensure that the device
always remains attached to the USB regardless of the state of the backend
chardev.

Signed-off-by: Mark Cave-Ayland 
Reviewed-by: Samuel Thibault 
Message-id: 20201027150456.24606-7-mark.cave-ayl...@ilande.co.uk
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/dev-serial.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 92c35615eb13..b9e308dca198 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -97,6 +97,7 @@ struct USBSerialState {
 uint8_t event_chr;
 uint8_t error_chr;
 uint8_t event_trigger;
+bool always_plugged;
 QEMUSerialSetParams params;
 int latency;/* ms */
 CharBackend cs;
@@ -516,12 +517,12 @@ static void usb_serial_event(void *opaque, QEMUChrEvent 
event)
 s->event_trigger |= FTDI_BI;
 break;
 case CHR_EVENT_OPENED:
-if (!s->dev.attached) {
+if (!s->always_plugged && !s->dev.attached) {
 usb_device_attach(&s->dev, &error_abort);
 }
 break;
 case CHR_EVENT_CLOSED:
-if (s->dev.attached) {
+if (!s->always_plugged && s->dev.attached) {
 usb_device_detach(&s->dev);
 }
 break;
@@ -556,7 +557,8 @@ static void usb_serial_realize(USBDevice *dev, Error **errp)
  usb_serial_event, NULL, s, NULL, true);
 usb_serial_handle_reset(dev);
 
-if (qemu_chr_fe_backend_open(&s->cs) && !dev->attached) {
+if ((s->always_plugged || qemu_chr_fe_backend_open(&s->cs)) &&
+!dev->attached) {
 usb_device_attach(dev, &error_abort);
 }
 s->intr = usb_ep_get(dev, USB_TOKEN_IN, 1);
@@ -584,6 +586,7 @@ static const VMStateDescription vmstate_usb_serial = {
 
 static Property serial_properties[] = {
 DEFINE_PROP_CHR("chardev", USBSerialState, cs),
+DEFINE_PROP_BOOL("always-plugged", USBSerialState, always_plugged, false),
 DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.27.0