Re: [PATCH 2/2] hw/nvme: fix copy cmd for pi enabled namespaces
On Apr 20 12:04, Klaus Jensen wrote: > On Apr 20 12:03, Dmitry Tikhov wrote: > > Current implementation have two problems: > > First in the read part of copy command. Because there is no metadata > > mangling before nvme_dif_check invocation, reftag error is thrown for > > blocks of namespace that have not been previously written to. > > Yes, this is definitely a bug and the fix is good, thanks! > > > Second in the write part. Reftag in the protection information section > > of the source metadata should not be copied as is to the destination. > > Hmm, says who? > > > Source range start lba and destination range start lba could differ so > > recalculation of reftag is always needed. > > > > If PRACT is 0, we really should not touch the protection information. My > interpretation of the Copy command is that it is simply just screwed if > used with PRACT 0 and Type 1. PRACT bit is specifically to allow the > controller to generate appropriate PI in this case. > > On the other hand, I can totally see your interpretation as valid as > well. Let me ask some spec people about this, and I will get back to > you. Discussed this with the TP authors and, no, reftag should not be re-computed for PRACT 0, regardless of the PI type. signature.asc Description: PGP signature
virtio-scsi and block mirroring
Hello everyone, I’m looking at an issue where I do see guests freezing (Dl) process state during a block disk mirror from one storage to another storage (NFS) where the network stack of the guest can freeze for up to 10 seconds. Looking at the storage and IO I noticed good throughput ad low latency <3ms and I am having trouble to track down the source for the issue, as neither storage nor networking show issues. Interestingly when I do the same test with virtio-blk I do not really see the process freezes at the frequency or duration compared to virtio-scsi which seem to indicate a client side rather than storage side problem. The copy job is setup by openstack volume migration and translate into >From what I observed the issue is more noticeable when I see more fdatasync >calls during the copy but I haven’t been able to correlate that to the issue >100% yet % time seconds usecs/call callserrors syscall -- --- --- - - 28.51 20.6726548339 2479 ioctl 27.81 20.1627143379 596731 futex 22.02 15.964498 785 20335 poll 15.22 11.038403 150 73561 io_submit 4.173.023285 41 73540 lseek 1.200.868003 5158591 write 0.630.459030 11 42871 ppoll 0.220.159263 8 19314 recvmsg 0.160.115520 5 22526 read 0.040.029149 29149 1 restart_syscall 0.010.009252 28 330 sendmsg 0.000.0012211221 1 munmap 0.000.000458 2221 fcntl 0.000.000286 95 3 openat 0.000.000166 532 rt_sigprocmask 0.000.000103 1010 fdatasync 0.000.99 25 4 clone 0.000.81 712 mmap 0.000.77 19 4 close 0.000.76 612 mprotect 0.000.56 14 4 madvise 0.000.25 6 4 set_robust_list 0.000.23 6 4 prctl -- --- --- - - 100.00 72.50444241962631 total Does anyone have an idea how to better debug this issue ? Thanks Bjoern
Re: introducing vrc :)
On Tue, Apr 19, 2022 at 04:39:13PM +0200, Paolo Bonzini wrote: > Hi all, > > a while ago I looked at tools that could be used too build a call graph. > The simplest but most effective that I found was a small Perl program > (called "egypt", which is rot13 for "rtlcg" aka RTL call graph) that used > the GCC dumps to build the graph. Paolo, Do you have any plan to put it into some git repository? Thanks! -- Peter Xu
Re: [PULL 0/8] Block patches
On 4/20/22 05:40, Hanna Reitz wrote: The following changes since commit 1be5a765c08cee3a9587c8a8d3fc2ea247b13f9c: Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging (2022-04-19 18:22:16 -0700) are available in the Git repository at: https://gitlab.com/hreitz/qemu.git tags/pull-block-2022-04-20 for you to fetch changes up to 0423f75351ab83b844a31349218b0eadd830e07a: qcow2: Add errp to rebuild_refcount_structure() (2022-04-20 12:09:17 +0200) Block patches: - Some changes for qcow2's refcount repair algorithm to make it work for qcow2 images stored on block devices - Skip test cases that require zstd when support for it is missing - Some refactoring in the iotests' meson.build Applied, thanks. Please update the wiki changelog for 7.1 as appropriate. r~ Hanna Reitz (6): iotests.py: Add supports_qcow2_zstd_compression() iotests/065: Check for zstd support iotests/303: Check for zstd support qcow2: Improve refcount structure rebuilding iotests/108: Test new refcount rebuild algorithm qcow2: Add errp to rebuild_refcount_structure() Thomas Huth (2): tests/qemu-iotests/meson.build: Improve the indentation tests/qemu-iotests: Move the bash and sanitizer checks to meson.build block/qcow2-refcount.c | 353 +++-- tests/check-block.sh | 26 --- tests/qemu-iotests/065 | 24 ++- tests/qemu-iotests/108 | 259 +++- tests/qemu-iotests/108.out | 81 tests/qemu-iotests/303 | 4 +- tests/qemu-iotests/iotests.py | 20 ++ tests/qemu-iotests/meson.build | 73 --- 8 files changed, 673 insertions(+), 167 deletions(-)
Re: [PATCH 06/41] include: rename qemu-common.h qemu/copyright.h
On Wed, Apr 20, 2022 at 05:10:51PM +0100, Peter Maydell wrote: > On Wed, 20 Apr 2022 at 16:04, Daniel P. Berrangé wrote: > > > > On Wed, Apr 20, 2022 at 05:25:49PM +0400, marcandre.lur...@redhat.com wrote: > > > From: Marc-André Lureau > > > > Could use a commit message explaining why this is a good > > idea. > > > > I see it contains QEMU_COPYRIGHT macro, but it also then > > contains QEMU_HELP_BOTTOM which is about bug reporting > > not copyright. > > > > IMHO something like 'qemu-cli.h' could be a better match > > "-cli" is both too broad and also inaccurate, because we use > these macros in the GUI UI frontends too. It's "macros defining > strings for use in version/usage/help dialogs and output". > Maybe qemu/help-texts.h ? I guess I tend to still view qemu-system-XXX as cli, despite the possibility of a GUI, since you're passing 10's/100's of CLI parameters regardless. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 38/41] util: replace qemu_get_local_state_pathname()
On Wed, Apr 20, 2022 at 05:26:21PM +0400, marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau > > Simplify the function to only return the directory path. Callers are > adjusted to use the GLib function to build paths, g_build_filename(). > > Signed-off-by: Marc-André Lureau > --- > include/qemu/osdep.h | 9 +++-- > qga/main.c| 8 > scsi/qemu-pr-helper.c | 6 -- > tools/virtiofsd/fuse_virtio.c | 4 +++- > util/oslib-posix.c| 7 ++- > util/oslib-win32.c| 5 ++--- > 6 files changed, 18 insertions(+), 21 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 06/41] include: rename qemu-common.h qemu/copyright.h
On Wed, 20 Apr 2022 at 16:04, Daniel P. Berrangé wrote: > > On Wed, Apr 20, 2022 at 05:25:49PM +0400, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > Could use a commit message explaining why this is a good > idea. > > I see it contains QEMU_COPYRIGHT macro, but it also then > contains QEMU_HELP_BOTTOM which is about bug reporting > not copyright. > > IMHO something like 'qemu-cli.h' could be a better match "-cli" is both too broad and also inaccurate, because we use these macros in the GUI UI frontends too. It's "macros defining strings for use in version/usage/help dialogs and output". Maybe qemu/help-texts.h ? -- PMM
Re: [PATCH 22/41] include: move qemu_*_exec_dir() to cutils
On Wed, Apr 20, 2022 at 05:26:05PM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> The function is required by get_relocated_path(), which is used by
> qemu-ga and may be generally useful.
In patch 06 I suggested we have a qemu-cli.h, and perhaps this
qemu_*_exec_dir functionality is also relevant for qemu-cli.{c,h}
since it is specific to command line tool initialization.
>
> Signed-off-by: Marc-André Lureau
> ---
> include/qemu/cutils.h| 7 ++
> include/qemu/osdep.h | 8 --
> qemu-io.c| 1 +
> storage-daemon/qemu-storage-daemon.c | 1 +
> tests/qtest/fuzz/fuzz.c | 1 +
> util/cutils.c| 109 +++
> util/oslib-posix.c | 81
> util/oslib-win32.c | 36 -
> 8 files changed, 119 insertions(+), 125 deletions(-)
>
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 5c6572d44422..40e10e19a7ed 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -193,6 +193,13 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n);
> */
> int qemu_pstrcmp0(const char **str1, const char **str2);
>
> +/* Find program directory, and save it for later usage with
> + * qemu_get_exec_dir().
> + * Try OS specific API first, if not working, parse from argv0. */
> +void qemu_init_exec_dir(const char *argv0);
> +
> +/* Get the saved exec dir. */
> +const char *qemu_get_exec_dir(void);
>
> /**
> * get_relocated_path:
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index a87f1b7f32e6..9fd52d6a33a7 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -567,14 +567,6 @@ bool fips_get_state(void);
> */
> char *qemu_get_local_state_pathname(const char *relative_pathname);
>
> -/* Find program directory, and save it for later usage with
> - * qemu_get_exec_dir().
> - * Try OS specific API first, if not working, parse from argv0. */
> -void qemu_init_exec_dir(const char *argv0);
> -
> -/* Get the saved exec dir. */
> -const char *qemu_get_exec_dir(void);
> -
> /**
> * qemu_getauxval:
> * @type: the auxiliary vector key to lookup
> diff --git a/qemu-io.c b/qemu-io.c
> index 952a36643b0c..458fadd99a92 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -16,6 +16,7 @@
> #endif
>
> #include "qemu/copyright.h"
> +#include "qemu/cutils.h"
> #include "qapi/error.h"
> #include "qemu-io.h"
> #include "qemu/error-report.h"
> diff --git a/storage-daemon/qemu-storage-daemon.c
> b/storage-daemon/qemu-storage-daemon.c
> index a4415e8c995b..63b155013ed7 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -44,6 +44,7 @@
>
> #include "qemu/copyright.h"
> #include "qemu-version.h"
> +#include "qemu/cutils.h"
> #include "qemu/config-file.h"
> #include "qemu/error-report.h"
> #include "qemu/help_option.h"
> diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
> index 5f77c849837f..d3afd294db24 100644
> --- a/tests/qtest/fuzz/fuzz.c
> +++ b/tests/qtest/fuzz/fuzz.c
> @@ -15,6 +15,7 @@
>
> #include
>
> +#include "qemu/cutils.h"
> #include "qemu/datadir.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/qtest.h"
> diff --git a/util/cutils.c b/util/cutils.c
> index b2777210e7da..443927275fdb 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -931,6 +931,115 @@ static inline const char *next_component(const char
> *dir, int *p_len)
> return dir;
> }
>
> +static const char *exec_dir;
> +
> +void qemu_init_exec_dir(const char *argv0)
> +{
> +#ifdef G_OS_WIN32
> +char *p;
> +char buf[MAX_PATH];
> +DWORD len;
> +
> +if (exec_dir) {
> +return;
> +}
> +
> +len = GetModuleFileName(NULL, buf, sizeof(buf) - 1);
> +if (len == 0) {
> +return;
> +}
> +
> +buf[len] = 0;
> +p = buf + len - 1;
> +while (p != buf && *p != '\\') {
> +p--;
> +}
> +*p = 0;
> +if (access(buf, R_OK) == 0) {
> +exec_dir = g_strdup(buf);
> +} else {
> +exec_dir = CONFIG_BINDIR;
> +}
> +#else
> +char *p = NULL;
> +char buf[PATH_MAX];
> +
> +if (exec_dir) {
> +return;
> +}
> +
> +#if defined(__linux__)
> +{
> +int len;
> +len = readlink("/proc/self/exe", buf, sizeof(buf) - 1);
> +if (len > 0) {
> +buf[len] = 0;
> +p = buf;
> +}
> +}
> +#elif defined(__FreeBSD__) \
> + || (defined(__NetBSD__) && defined(KERN_PROC_PATHNAME))
> +{
> +#if defined(__FreeBSD__)
> +static int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1};
> +#else
> +static int mib[4] = {CTL_KERN, KERN_PROC_ARGS, -1,
> KERN_PROC_PATHNAME};
> +#endif
> +size_t len = sizeof(buf) - 1;
> +
> +*buf = '\0';
> +if (!sysctl(mib, ARRAY_SIZE(mib), buf, , NULL, 0) &&
> +*buf) {
> +
Re: [PATCH 24/41] include: move qdict_{crumple,flatten} declarations
On Wed, Apr 20, 2022 at 05:26:07PM +0400, marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau > > Move them where they belong, since the functions are implemented in > block-qdict.c. > > Signed-off-by: Marc-André Lureau > --- > include/block/qdict.h | 3 +++ > include/qapi/qmp/qdict.h | 3 --- > softmmu/vl.c | 1 + > tests/unit/check-qobject.c | 1 + > 4 files changed, 5 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 19/41] compiler.h: replace QEMU_NORETURN with G_NORETURN
On Wed, Apr 20, 2022 at 05:26:02PM +0400, marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau > > G_NORETURN was introduced in glib 2.68, fallback to G_GNUC_NORETURN in > glib-compat. > > Note that this attribute must be placed before the function declaration > (bringing a bit of consistency in qemu codebase usage). > > Signed-off-by: Marc-André Lureau > --- > accel/tcg/internal.h | 3 +-- > include/exec/exec-all.h | 20 +- > include/exec/helper-head.h | 2 +- > include/glib-compat.h| 4 > include/hw/core/cpu.h| 2 +- > include/hw/core/tcg-cpu-ops.h| 6 +++--- > include/hw/hw.h | 2 +- > include/qemu/compiler.h | 2 -- > include/qemu/osdep.h | 3 ++- > include/qemu/thread.h| 2 +- > include/tcg/tcg-ldst.h | 4 ++-- > include/tcg/tcg.h| 2 +- > linux-user/user-internals.h | 2 +- > scripts/cocci-macro-file.h | 2 +- > target/alpha/cpu.h | 10 - > target/arm/internals.h | 12 +-- > target/hppa/cpu.h| 2 +- > target/i386/tcg/helper-tcg.h | 24 ++--- > target/microblaze/cpu.h | 6 +++--- > target/mips/tcg/tcg-internal.h | 17 --- > target/nios2/cpu.h | 6 +++--- > target/openrisc/exception.h | 2 +- > target/ppc/cpu.h | 14 ++--- > target/ppc/internal.h| 6 +++--- > target/riscv/cpu.h | 10 - > target/s390x/s390x-internal.h| 6 +++--- > target/s390x/tcg/tcg_s390x.h | 12 +-- > target/sh4/cpu.h | 6 +++--- > target/sparc/cpu.h | 10 - > target/xtensa/cpu.h | 6 +++--- > accel/stubs/tcg-stub.c | 4 ++-- > bsd-user/signal.c| 3 ++- > hw/misc/mips_itu.c | 3 ++- > linux-user/signal.c | 3 ++- > monitor/hmp.c| 4 ++-- > qemu-img.c | 12 +++ > target/alpha/helper.c| 10 - > target/arm/pauth_helper.c| 4 ++-- > target/arm/tlb_helper.c | 7 --- > target/hexagon/op_helper.c | 9 > target/hppa/cpu.c| 8 +++ > target/hppa/op_helper.c | 4 ++-- > target/i386/tcg/bpt_helper.c | 2 +- > target/i386/tcg/excp_helper.c| 31 ++-- > target/i386/tcg/misc_helper.c| 6 +++--- > target/i386/tcg/sysemu/misc_helper.c | 7 --- > target/openrisc/exception.c | 2 +- > target/openrisc/exception_helper.c | 3 ++- > target/riscv/op_helper.c | 4 ++-- > target/rx/op_helper.c| 22 +++- > target/s390x/tcg/excp_helper.c | 22 +++- > target/sh4/op_helper.c | 5 +++-- > target/sparc/mmu_helper.c| 8 +++ > target/tricore/op_helper.c | 6 +++--- > tcg/tcg.c| 3 ++- > tests/fp/fp-bench.c | 3 ++- > tests/fp/fp-test.c | 3 ++- > scripts/checkpatch.pl| 2 +- > 58 files changed, 214 insertions(+), 191 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 06/41] include: rename qemu-common.h qemu/copyright.h
Hi
On Wed, Apr 20, 2022 at 7:17 PM Daniel P. Berrangé
wrote:
> On Wed, Apr 20, 2022 at 05:25:49PM +0400, marcandre.lur...@redhat.com
> wrote:
> > From: Marc-André Lureau
>
> Could use a commit message explaining why this is a good
> idea.
>
> I see it contains QEMU_COPYRIGHT macro, but it also then
> contains QEMU_HELP_BOTTOM which is about bug reporting
> not copyright.
>
> IMHO something like 'qemu-cli.h' could be a better match
>
That was Peter's suggestion:
https://patchew.org/QEMU/20220323155743.1585078-1-marcandre.lur...@redhat.com/20220323155743.1585078-33-marcandre.lur...@redhat.com/#CAFEAcA9kYweS2zMHjWDuV_y2AxKbgJ5UYNHLK3sASCLVD=y...@mail.gmail.com
I don't mind qemu-cli.h or elsewhere
Peter?
> >
> > Suggested-by: Peter Maydell
> > Signed-off-by: Marc-André Lureau
> > ---
> > include/{qemu-common.h => qemu/copyright.h} | 0
> > bsd-user/main.c | 2 +-
> > linux-user/main.c | 2 +-
> > qemu-img.c | 2 +-
> > qemu-io.c | 2 +-
> > qemu-nbd.c | 2 +-
> > qga/main.c | 2 +-
> > scsi/qemu-pr-helper.c | 2 +-
> > softmmu/vl.c| 2 +-
> > storage-daemon/qemu-storage-daemon.c| 2 +-
> > tools/virtiofsd/passthrough_ll.c| 2 +-
> > ui/cocoa.m | 2 +-
> > 12 files changed, 11 insertions(+), 11 deletions(-)
> > rename include/{qemu-common.h => qemu/copyright.h} (100%)
> >
> > diff --git a/include/qemu-common.h b/include/qemu/copyright.h
> > similarity index 100%
> > rename from include/qemu-common.h
> > rename to include/qemu/copyright.h
> > diff --git a/bsd-user/main.c b/bsd-user/main.c
> > index 88d347d05ebf..aaab3f278534 100644
> > --- a/bsd-user/main.c
> > +++ b/bsd-user/main.c
> > @@ -24,7 +24,7 @@
> > #include
> >
> > #include "qemu/osdep.h"
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qemu/units.h"
> > #include "qemu/accel.h"
> > #include "sysemu/tcg.h"
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index fbc9bcfd5f5f..744d216b1e8e 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -18,7 +18,7 @@
> > */
> >
> > #include "qemu/osdep.h"
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qemu/units.h"
> > #include "qemu/accel.h"
> > #include "sysemu/tcg.h"
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 116e05867558..a2b1d3653a1e 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -25,7 +25,7 @@
> > #include "qemu/osdep.h"
> > #include
> >
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qemu/qemu-progress.h"
> > #include "qemu-version.h"
> > #include "qapi/error.h"
> > diff --git a/qemu-io.c b/qemu-io.c
> > index eb8afc8b413b..952a36643b0c 100644
> > --- a/qemu-io.c
> > +++ b/qemu-io.c
> > @@ -15,7 +15,7 @@
> > #include
> > #endif
> >
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qapi/error.h"
> > #include "qemu-io.h"
> > #include "qemu/error-report.h"
> > diff --git a/qemu-nbd.c b/qemu-nbd.c
> > index 713e7557a9eb..f4d121c0c40e 100644
> > --- a/qemu-nbd.c
> > +++ b/qemu-nbd.c
> > @@ -21,7 +21,7 @@
> > #include
> > #include
> >
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qapi/error.h"
> > #include "qemu/cutils.h"
> > #include "sysemu/block-backend.h"
> > diff --git a/qga/main.c b/qga/main.c
> > index ac63d8e47802..8994f73e4735 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -18,7 +18,7 @@
> > #include
> > #include
> > #endif
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qapi/qmp/json-parser.h"
> > #include "qapi/qmp/qdict.h"
> > #include "qapi/qmp/qjson.h"
> > diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> > index f281daeced8d..e7549ffb3bc9 100644
> > --- a/scsi/qemu-pr-helper.c
> > +++ b/scsi/qemu-pr-helper.c
> > @@ -36,7 +36,7 @@
> > #include
> > #endif
> >
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qapi/error.h"
> > #include "qemu/cutils.h"
> > #include "qemu/main-loop.h"
> > diff --git a/softmmu/vl.c b/softmmu/vl.c
> > index 46aba6a039c4..b0bf16e16aaa 100644
> > --- a/softmmu/vl.c
> > +++ b/softmmu/vl.c
> > @@ -23,7 +23,7 @@
> > */
> >
> > #include "qemu/osdep.h"
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qemu/datadir.h"
> > #include "qemu/units.h"
> > #include "exec/cpu-common.h"
> > diff --git a/storage-daemon/qemu-storage-daemon.c
> b/storage-daemon/qemu-storage-daemon.c
> > index eb724072579a..a4415e8c995b 100644
> > --- a/storage-daemon/qemu-storage-daemon.c
> > +++ b/storage-daemon/qemu-storage-daemon.c
> > @@ -42,7 +42,7 @@
> > #include "qapi/qmp/qstring.h"
> > #include "qapi/qobject-input-visitor.h"
> >
> > -#include "qemu-common.h"
> >
Re: [PATCH 06/41] include: rename qemu-common.h qemu/copyright.h
On Wed, Apr 20, 2022 at 05:25:49PM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
Could use a commit message explaining why this is a good
idea.
I see it contains QEMU_COPYRIGHT macro, but it also then
contains QEMU_HELP_BOTTOM which is about bug reporting
not copyright.
IMHO something like 'qemu-cli.h' could be a better match
>
> Suggested-by: Peter Maydell
> Signed-off-by: Marc-André Lureau
> ---
> include/{qemu-common.h => qemu/copyright.h} | 0
> bsd-user/main.c | 2 +-
> linux-user/main.c | 2 +-
> qemu-img.c | 2 +-
> qemu-io.c | 2 +-
> qemu-nbd.c | 2 +-
> qga/main.c | 2 +-
> scsi/qemu-pr-helper.c | 2 +-
> softmmu/vl.c| 2 +-
> storage-daemon/qemu-storage-daemon.c| 2 +-
> tools/virtiofsd/passthrough_ll.c| 2 +-
> ui/cocoa.m | 2 +-
> 12 files changed, 11 insertions(+), 11 deletions(-)
> rename include/{qemu-common.h => qemu/copyright.h} (100%)
>
> diff --git a/include/qemu-common.h b/include/qemu/copyright.h
> similarity index 100%
> rename from include/qemu-common.h
> rename to include/qemu/copyright.h
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 88d347d05ebf..aaab3f278534 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -24,7 +24,7 @@
> #include
>
> #include "qemu/osdep.h"
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qemu/units.h"
> #include "qemu/accel.h"
> #include "sysemu/tcg.h"
> diff --git a/linux-user/main.c b/linux-user/main.c
> index fbc9bcfd5f5f..744d216b1e8e 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -18,7 +18,7 @@
> */
>
> #include "qemu/osdep.h"
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qemu/units.h"
> #include "qemu/accel.h"
> #include "sysemu/tcg.h"
> diff --git a/qemu-img.c b/qemu-img.c
> index 116e05867558..a2b1d3653a1e 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -25,7 +25,7 @@
> #include "qemu/osdep.h"
> #include
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qemu/qemu-progress.h"
> #include "qemu-version.h"
> #include "qapi/error.h"
> diff --git a/qemu-io.c b/qemu-io.c
> index eb8afc8b413b..952a36643b0c 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -15,7 +15,7 @@
> #include
> #endif
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qapi/error.h"
> #include "qemu-io.h"
> #include "qemu/error-report.h"
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 713e7557a9eb..f4d121c0c40e 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -21,7 +21,7 @@
> #include
> #include
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qapi/error.h"
> #include "qemu/cutils.h"
> #include "sysemu/block-backend.h"
> diff --git a/qga/main.c b/qga/main.c
> index ac63d8e47802..8994f73e4735 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -18,7 +18,7 @@
> #include
> #include
> #endif
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qapi/qmp/json-parser.h"
> #include "qapi/qmp/qdict.h"
> #include "qapi/qmp/qjson.h"
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index f281daeced8d..e7549ffb3bc9 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -36,7 +36,7 @@
> #include
> #endif
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qapi/error.h"
> #include "qemu/cutils.h"
> #include "qemu/main-loop.h"
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 46aba6a039c4..b0bf16e16aaa 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -23,7 +23,7 @@
> */
>
> #include "qemu/osdep.h"
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qemu/datadir.h"
> #include "qemu/units.h"
> #include "exec/cpu-common.h"
> diff --git a/storage-daemon/qemu-storage-daemon.c
> b/storage-daemon/qemu-storage-daemon.c
> index eb724072579a..a4415e8c995b 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -42,7 +42,7 @@
> #include "qapi/qmp/qstring.h"
> #include "qapi/qobject-input-visitor.h"
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qemu-version.h"
> #include "qemu/config-file.h"
> #include "qemu/error-report.h"
> diff --git a/tools/virtiofsd/passthrough_ll.c
> b/tools/virtiofsd/passthrough_ll.c
> index 028dacdd8f5a..8af28f5fb823 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -38,7 +38,7 @@
> #include "qemu/osdep.h"
> #include "qemu/timer.h"
> #include "qemu-version.h"
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "fuse_virtio.h"
> #include "fuse_log.h"
> #include "fuse_lowlevel.h"
> diff --git a/ui/cocoa.m b/ui/cocoa.m
Re: [PATCH 19/41] compiler.h: replace QEMU_NORETURN with G_NORETURN
On Wed, Apr 20, 2022 at 7:28 AM wrote: > From: Marc-André Lureau > > G_NORETURN was introduced in glib 2.68, fallback to G_GNUC_NORETURN in > glib-compat. > > Note that this attribute must be placed before the function declaration > (bringing a bit of consistency in qemu codebase usage). > > Signed-off-by: Marc-André Lureau > Reviewed-by: Warner Losh Most of this looks mechanical, but I only looked closely at the bsd-user changes. > --- > accel/tcg/internal.h | 3 +-- > include/exec/exec-all.h | 20 +- > include/exec/helper-head.h | 2 +- > include/glib-compat.h| 4 > include/hw/core/cpu.h| 2 +- > include/hw/core/tcg-cpu-ops.h| 6 +++--- > include/hw/hw.h | 2 +- > include/qemu/compiler.h | 2 -- > include/qemu/osdep.h | 3 ++- > include/qemu/thread.h| 2 +- > include/tcg/tcg-ldst.h | 4 ++-- > include/tcg/tcg.h| 2 +- > linux-user/user-internals.h | 2 +- > scripts/cocci-macro-file.h | 2 +- > target/alpha/cpu.h | 10 - > target/arm/internals.h | 12 +-- > target/hppa/cpu.h| 2 +- > target/i386/tcg/helper-tcg.h | 24 ++--- > target/microblaze/cpu.h | 6 +++--- > target/mips/tcg/tcg-internal.h | 17 --- > target/nios2/cpu.h | 6 +++--- > target/openrisc/exception.h | 2 +- > target/ppc/cpu.h | 14 ++--- > target/ppc/internal.h| 6 +++--- > target/riscv/cpu.h | 10 - > target/s390x/s390x-internal.h| 6 +++--- > target/s390x/tcg/tcg_s390x.h | 12 +-- > target/sh4/cpu.h | 6 +++--- > target/sparc/cpu.h | 10 - > target/xtensa/cpu.h | 6 +++--- > accel/stubs/tcg-stub.c | 4 ++-- > bsd-user/signal.c| 3 ++- > hw/misc/mips_itu.c | 3 ++- > linux-user/signal.c | 3 ++- > monitor/hmp.c| 4 ++-- > qemu-img.c | 12 +++ > target/alpha/helper.c| 10 - > target/arm/pauth_helper.c| 4 ++-- > target/arm/tlb_helper.c | 7 --- > target/hexagon/op_helper.c | 9 > target/hppa/cpu.c| 8 +++ > target/hppa/op_helper.c | 4 ++-- > target/i386/tcg/bpt_helper.c | 2 +- > target/i386/tcg/excp_helper.c| 31 ++-- > target/i386/tcg/misc_helper.c| 6 +++--- > target/i386/tcg/sysemu/misc_helper.c | 7 --- > target/openrisc/exception.c | 2 +- > target/openrisc/exception_helper.c | 3 ++- > target/riscv/op_helper.c | 4 ++-- > target/rx/op_helper.c| 22 +++- > target/s390x/tcg/excp_helper.c | 22 +++- > target/sh4/op_helper.c | 5 +++-- > target/sparc/mmu_helper.c| 8 +++ > target/tricore/op_helper.c | 6 +++--- > tcg/tcg.c| 3 ++- > tests/fp/fp-bench.c | 3 ++- > tests/fp/fp-test.c | 3 ++- > scripts/checkpatch.pl| 2 +- > 58 files changed, 214 insertions(+), 191 deletions(-) > > diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h > index 881bc1ede0b1..3092bfa96430 100644 > --- a/accel/tcg/internal.h > +++ b/accel/tcg/internal.h > @@ -14,8 +14,7 @@ > TranslationBlock *tb_gen_code(CPUState *cpu, target_ulong pc, >target_ulong cs_base, uint32_t flags, >int cflags); > - > -void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr); > +G_NORETURN void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr); > void page_init(void); > void tb_htable_init(void); > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index d2cb0981f405..311e5fb422a3 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -58,10 +58,10 @@ void restore_state_to_opc(CPUArchState *env, > TranslationBlock *tb, > */ > bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool > will_exit); > > -void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu); > -void QEMU_NORETURN cpu_loop_exit(CPUState *cpu); > -void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); > -void QEMU_NORETURN cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc); > +G_NORETURN void cpu_loop_exit_noexc(CPUState *cpu); > +G_NORETURN void cpu_loop_exit(CPUState *cpu); > +G_NORETURN void cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); > +G_NORETURN void cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc); > > /** > *
Re: [PATCH 06/41] include: rename qemu-common.h qemu/copyright.h
On Wed, Apr 20, 2022 at 7:26 AM wrote:
> From: Marc-André Lureau
>
> Suggested-by: Peter Maydell
> Signed-off-by: Marc-André Lureau
>
Reviewed-by: Warner Losh
> ---
> include/{qemu-common.h => qemu/copyright.h} | 0
> bsd-user/main.c | 2 +-
> linux-user/main.c | 2 +-
> qemu-img.c | 2 +-
> qemu-io.c | 2 +-
> qemu-nbd.c | 2 +-
> qga/main.c | 2 +-
> scsi/qemu-pr-helper.c | 2 +-
> softmmu/vl.c| 2 +-
> storage-daemon/qemu-storage-daemon.c| 2 +-
> tools/virtiofsd/passthrough_ll.c| 2 +-
> ui/cocoa.m | 2 +-
> 12 files changed, 11 insertions(+), 11 deletions(-)
> rename include/{qemu-common.h => qemu/copyright.h} (100%)
>
> diff --git a/include/qemu-common.h b/include/qemu/copyright.h
> similarity index 100%
> rename from include/qemu-common.h
> rename to include/qemu/copyright.h
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 88d347d05ebf..aaab3f278534 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -24,7 +24,7 @@
> #include
>
> #include "qemu/osdep.h"
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qemu/units.h"
> #include "qemu/accel.h"
> #include "sysemu/tcg.h"
> diff --git a/linux-user/main.c b/linux-user/main.c
> index fbc9bcfd5f5f..744d216b1e8e 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -18,7 +18,7 @@
> */
>
> #include "qemu/osdep.h"
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qemu/units.h"
> #include "qemu/accel.h"
> #include "sysemu/tcg.h"
> diff --git a/qemu-img.c b/qemu-img.c
> index 116e05867558..a2b1d3653a1e 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -25,7 +25,7 @@
> #include "qemu/osdep.h"
> #include
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qemu/qemu-progress.h"
> #include "qemu-version.h"
> #include "qapi/error.h"
> diff --git a/qemu-io.c b/qemu-io.c
> index eb8afc8b413b..952a36643b0c 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -15,7 +15,7 @@
> #include
> #endif
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qapi/error.h"
> #include "qemu-io.h"
> #include "qemu/error-report.h"
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 713e7557a9eb..f4d121c0c40e 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -21,7 +21,7 @@
> #include
> #include
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qapi/error.h"
> #include "qemu/cutils.h"
> #include "sysemu/block-backend.h"
> diff --git a/qga/main.c b/qga/main.c
> index ac63d8e47802..8994f73e4735 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -18,7 +18,7 @@
> #include
> #include
> #endif
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qapi/qmp/json-parser.h"
> #include "qapi/qmp/qdict.h"
> #include "qapi/qmp/qjson.h"
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index f281daeced8d..e7549ffb3bc9 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -36,7 +36,7 @@
> #include
> #endif
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qapi/error.h"
> #include "qemu/cutils.h"
> #include "qemu/main-loop.h"
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 46aba6a039c4..b0bf16e16aaa 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -23,7 +23,7 @@
> */
>
> #include "qemu/osdep.h"
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qemu/datadir.h"
> #include "qemu/units.h"
> #include "exec/cpu-common.h"
> diff --git a/storage-daemon/qemu-storage-daemon.c
> b/storage-daemon/qemu-storage-daemon.c
> index eb724072579a..a4415e8c995b 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -42,7 +42,7 @@
> #include "qapi/qmp/qstring.h"
> #include "qapi/qobject-input-visitor.h"
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qemu-version.h"
> #include "qemu/config-file.h"
> #include "qemu/error-report.h"
> diff --git a/tools/virtiofsd/passthrough_ll.c
> b/tools/virtiofsd/passthrough_ll.c
> index 028dacdd8f5a..8af28f5fb823 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -38,7 +38,7 @@
> #include "qemu/osdep.h"
> #include "qemu/timer.h"
> #include "qemu-version.h"
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "fuse_virtio.h"
> #include "fuse_log.h"
> #include "fuse_lowlevel.h"
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 839ae4f58a69..a2a74656fabf 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -27,7 +27,7 @@
> #import
> #include
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
> #include "qemu-main.h"
> #include "ui/clipboard.h"
> #include "ui/console.h"
> --
>
Re: [PATCH 03/26] nbd: remove incorrect coroutine_fn annotations
On 4/19/22 20:08, Eric Blake wrote: -void coroutine_fn nbd_co_establish_connection_cancel(NBDClientConnection *conn); +void nbd_co_establish_connection_cancel(NBDClientConnection *conn); Should we rename this function to drop_co_ while at it? Or perhaps rename it to nbd_cancel_co_establish_connection... Paolo
Re: introducing vrc :)
On Tue, Apr 19, 2022 at 04:39:13PM +0200, Paolo Bonzini wrote: > Hi all, > > a while ago I looked at tools that could be used too build a call graph. > The simplest but most effective that I found was a small Perl program > (called "egypt", which is rot13 for "rtlcg" aka RTL call graph) that used > the GCC dumps to build the graph. > > I have now rewritten it in Python and extended it with a lot of new > functionality: > > - consult compile_commands.json to find/build dumps automatically > > - virtual (manually created) nodes and edges > > - query call graph in addition to generating DOT file > > - interactive mode with readline + completion > > The name is unfortunately not rot13 anymore, it stands for visit RTL > callgraph. > > Here is an example (run vrc from the root build directory of QEMU): > > # load files > load libblock.fa.p/*.c.o > > # introduce virtual edges corresponding to function pointers > node BlockDriverState.bdrv_co_flush > edge bdrv_co_flush BlockDriverState.bdrv_co_flush > edge BlockDriverState.bdrv_co_flush blk_log_writes_co_do_file_flush > edge BlockDriverState.bdrv_co_flush preallocate_co_flush > edge BlockDriverState.bdrv_co_flush raw_co_invalidate_cache > edge BlockDriverState.bdrv_co_flush cbw_co_flush > edge BlockDriverState.bdrv_co_flush quorum_co_flush > edge BlockDriverState.bdrv_co_flush throttle_co_flush > edge BlockDriverState.bdrv_co_flush blkdebug_co_flush > edge BlockDriverState.bdrv_co_flush blkverify_co_flush > edge BlockDriverState.bdrv_co_flush bdrv_mirror_top_flush > # apply filter > only --callees bdrv_co_flush > # draw graph > dotty --files > > The filtering functionality is a bit rough in the presence of mutual > recursion, but hopefully this can be already useful to find the root calls > of bdrv_*, which are the places where the graph lock has to be taken for > read. Continuing the previous example: > > # apply another filter > reset > omit --callees bdrv_co_flush > keep bdrv_co_flush > # example of query > callers bdrv_co_flush > > already gives a reasonable answer (not entirely correct, but the actual > analysis must be done on all callbacks at once): > > qed_co_request -> bdrv_co_flush > qed_need_check_timer_entry -> bdrv_co_flush > blk_log_writes_co_log -> bdrv_co_flush > bdrv_co_flush_entry -> bdrv_co_flush > bdrv_co_flush -> bdrv_co_flush > blk_co_do_flush -> bdrv_co_flush > bdrv_driver_pwritev -> bdrv_co_flush > blk_co_flush -> bdrv_co_flush > bdrv_flush -> bdrv_co_flush > bdrv_co_do_pwrite_zeroes -> bdrv_co_flush > blk_aio_flush_entry -> bdrv_co_flush Cool, thanks for sharing. I will keep this in mind when I need to analyze call graphs. Stefan signature.asc Description: PGP signature
Re: [PATCH v7 00/12] hw/nvme: SR-IOV with Virtualization Enhancements
On Wed, Apr 20, 2022 at 02:12:58PM +0200, Klaus Jensen wrote: > On Apr 20 08:02, Michael S. Tsirkin wrote: > > On Fri, Mar 18, 2022 at 08:18:07PM +0100, Lukasz Maniak wrote: > > > Resubmitting v6 as v7 since Patchew got lost with my sophisticated CC of > > > all maintainers just for the cover letter. > > > > > > Changes since v5: > > > - Fixed PCI hotplug issue related to deleting VF twice > > > - Corrected error messages for SR-IOV parameters > > > - Rebased on master, patches for PCI got pulled into the tree > > > - Added Reviewed-by labels > > > > > > Lukasz Maniak (4): > > > hw/nvme: Add support for SR-IOV > > > hw/nvme: Add support for Primary Controller Capabilities > > > hw/nvme: Add support for Secondary Controller List > > > docs: Add documentation for SR-IOV and Virtualization Enhancements > > > > > > Łukasz Gieryk (8): > > > hw/nvme: Implement the Function Level Reset > > > hw/nvme: Make max_ioqpairs and msix_qsize configurable in runtime > > > hw/nvme: Remove reg_size variable and update BAR0 size calculation > > > hw/nvme: Calculate BAR attributes in a function > > > hw/nvme: Initialize capability structures for primary/secondary > > > controllers > > > hw/nvme: Add support for the Virtualization Management command > > > hw/nvme: Update the initalization place for the AER queue > > > hw/acpi: Make the PCI hot-plug aware of SR-IOV > > > > the right people to review and merge this would be storage/nvme > > maintainers. > > I did take a quick look though. > > > > Acked-by: Michael S. Tsirkin > > > > Was waiting for a review on the acpi stuff. Thanks! :) Thank you for checking and acking Michael :) Klaus, looking through the list of patches, we are still missing reviews for numbers 03, 08 and 09. Do you want me to update to v8 or wait for a review first? Thanks, Lukasz
Re: [PATCH 28/41] Use QEMU_SANITIZE_ADDRESS
On Wed, Apr 20, 2022 at 6:20 PM Thomas Huth wrote:
>
> On 20/04/2022 15.26, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > tests/qtest/fdc-test.c| 2 +-
> > util/coroutine-ucontext.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
> > index 4aa72f36431f..0b3c2c0d523f 100644
> > --- a/tests/qtest/fdc-test.c
> > +++ b/tests/qtest/fdc-test.c
> > @@ -550,7 +550,7 @@ static void fuzz_registers(void)
> >
> > static bool qtest_check_clang_sanitizer(void)
> > {
> > -#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
> > +#ifdef QEMU_SANITIZE_ADDRESS
> > return true;
> > #else
> > g_test_skip("QEMU not configured using --enable-sanitizers");
> > diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
> > index 904b375192ca..52725f5344fb 100644
> > --- a/util/coroutine-ucontext.c
> > +++ b/util/coroutine-ucontext.c
> > @@ -30,7 +30,7 @@
> > #include
> > #endif
> >
> > -#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
> > +#ifdef QEMU_SANITIZE_THREAD
>
> Shouldn't that be QEMU_SANITIZE_ADDRESS instead?
>
oops, thanks
Re: [PATCH 28/41] Use QEMU_SANITIZE_ADDRESS
On 20/04/2022 15.26, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
Signed-off-by: Marc-André Lureau
---
tests/qtest/fdc-test.c| 2 +-
util/coroutine-ucontext.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index 4aa72f36431f..0b3c2c0d523f 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -550,7 +550,7 @@ static void fuzz_registers(void)
static bool qtest_check_clang_sanitizer(void)
{
-#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+#ifdef QEMU_SANITIZE_ADDRESS
return true;
#else
g_test_skip("QEMU not configured using --enable-sanitizers");
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 904b375192ca..52725f5344fb 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -30,7 +30,7 @@
#include
#endif
-#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+#ifdef QEMU_SANITIZE_THREAD
Shouldn't that be QEMU_SANITIZE_ADDRESS instead?
Thomas
[PATCH 38/41] util: replace qemu_get_local_state_pathname()
From: Marc-André Lureau
Simplify the function to only return the directory path. Callers are
adjusted to use the GLib function to build paths, g_build_filename().
Signed-off-by: Marc-André Lureau
---
include/qemu/osdep.h | 9 +++--
qga/main.c| 8
scsi/qemu-pr-helper.c | 6 --
tools/virtiofsd/fuse_virtio.c | 4 +++-
util/oslib-posix.c| 7 ++-
util/oslib-win32.c| 5 ++---
6 files changed, 18 insertions(+), 21 deletions(-)
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index ccf10f05f806..220b44f710e5 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -556,16 +556,13 @@ void qemu_set_cloexec(int fd);
void fips_set_state(bool requested);
bool fips_get_state(void);
-/* Return a dynamically allocated pathname denoting a file or directory that is
- * appropriate for storing local state.
- *
- * @relative_pathname need not start with a directory separator; one will be
- * added automatically.
+/* Return a dynamically allocated directory path that is appropriate for
storing
+ * local state.
*
* The caller is responsible for releasing the value returned with g_free()
* after use.
*/
-char *qemu_get_local_state_pathname(const char *relative_pathname);
+char *qemu_get_local_state_dir(void);
/**
* qemu_getauxval:
diff --git a/qga/main.c b/qga/main.c
index b1dae0659d16..daff9bb024f3 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -129,12 +129,12 @@ static void stop_agent(GAState *s, bool requested);
static void
init_dfl_pathnames(void)
{
+g_autofree char *state = qemu_get_local_state_dir();
+
g_assert(dfl_pathnames.state_dir == NULL);
g_assert(dfl_pathnames.pidfile == NULL);
-dfl_pathnames.state_dir = qemu_get_local_state_pathname(
- QGA_STATE_RELATIVE_DIR);
-dfl_pathnames.pidfile = qemu_get_local_state_pathname(
- QGA_STATE_RELATIVE_DIR G_DIR_SEPARATOR_S "qemu-ga.pid");
+dfl_pathnames.state_dir = g_build_filename(state, QGA_STATE_RELATIVE_DIR,
NULL);
+dfl_pathnames.pidfile = g_build_filename(state, QGA_STATE_RELATIVE_DIR,
"qemu-ga.pid", NULL);
}
static void quit_handler(int sig)
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index e7549ffb3bc9..c77153392fcd 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -77,8 +77,10 @@ static int gid = -1;
static void compute_default_paths(void)
{
-socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
-pidfile = qemu_get_local_state_pathname("run/qemu-pr-helper.pid");
+g_autofree char *state = qemu_get_local_state_dir();
+
+socket_path = g_build_filename(state, "run", "qemu-pr-helper.sock", NULL);
+pidfile = g_build_filename(state, "run", "qemu-pr-helper.pid", NULL);
}
static void usage(const char *name)
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 60b96470c51a..a52eacf82e1e 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -901,10 +901,12 @@ static bool fv_socket_lock(struct fuse_session *se)
{
g_autofree gchar *sk_name = NULL;
g_autofree gchar *pidfile = NULL;
+g_autofree gchar *state = NULL;
g_autofree gchar *dir = NULL;
Error *local_err = NULL;
-dir = qemu_get_local_state_pathname("run/virtiofsd");
+state = qemu_get_local_state_dir();
+dir = g_build_filename(state, "run", "virtiofsd", NULL);
if (g_mkdir_with_parents(dir, S_IRWXU) < 0) {
fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s\n",
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index ed8a2558b14d..03d97741562c 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -297,12 +297,9 @@ int qemu_pipe(int pipefd[2])
}
char *
-qemu_get_local_state_pathname(const char *relative_pathname)
+qemu_get_local_state_dir(void)
{
-g_autofree char *dir = g_strdup_printf("%s/%s",
- CONFIG_QEMU_LOCALSTATEDIR,
- relative_pathname);
-return get_relocated_path(dir);
+return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
}
void qemu_set_tty_echo(int fd, bool echo)
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 41df0a289e28..9483c4c1d5de 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -235,7 +235,7 @@ int qemu_get_thread_id(void)
}
char *
-qemu_get_local_state_pathname(const char *relative_pathname)
+qemu_get_local_state_dir(void)
{
HRESULT result;
char base_path[MAX_PATH+1] = "";
@@ -247,8 +247,7 @@ qemu_get_local_state_pathname(const char *relative_pathname)
g_critical("CSIDL_COMMON_APPDATA unavailable: %ld", (long)result);
abort();
}
-return g_strdup_printf("%s" G_DIR_SEPARATOR_S "%s", base_path,
- relative_pathname);
+return g_strdup(base_path);
}
void qemu_set_tty_echo(int fd, bool echo)
--
2.35.1.693.g805e0a68082a
Re: [PATCH 07/26] block: add missing coroutine_fn annotations
On 4/19/22 20:50, Eric Blake wrote: +int coroutine_fn blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int bytes, +BdrvRequestFlags flags) Long line, worth rewrapping differently? The functions with_co_ in the name are obvious, the others might be worth a comment why it is okay. Or perhaps should be renamed to have _co_ in the name. Paolo
[PATCH 24/41] include: move qdict_{crumple,flatten} declarations
From: Marc-André Lureau Move them where they belong, since the functions are implemented in block-qdict.c. Signed-off-by: Marc-André Lureau --- include/block/qdict.h | 3 +++ include/qapi/qmp/qdict.h | 3 --- softmmu/vl.c | 1 + tests/unit/check-qobject.c | 1 + 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/block/qdict.h b/include/block/qdict.h index ced2acfb92a0..b4c28d96a9e5 100644 --- a/include/block/qdict.h +++ b/include/block/qdict.h @@ -12,6 +12,9 @@ #include "qapi/qmp/qdict.h" +QObject *qdict_crumple(const QDict *src, Error **errp); +void qdict_flatten(QDict *qdict); + void qdict_copy_default(QDict *dst, QDict *src, const char *key); void qdict_set_default_str(QDict *dst, const char *key, const char *val); diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h index 882d950bde89..82e90fc07229 100644 --- a/include/qapi/qmp/qdict.h +++ b/include/qapi/qmp/qdict.h @@ -68,7 +68,4 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key); QDict *qdict_clone_shallow(const QDict *src); -QObject *qdict_crumple(const QDict *src, Error **errp); -void qdict_flatten(QDict *qdict); - #endif /* QDICT_H */ diff --git a/softmmu/vl.c b/softmmu/vl.c index 8e3163a09233..b20d470b62a5 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -125,6 +125,7 @@ #include "qapi/qapi-visit-qom.h" #include "qapi/qapi-commands-ui.h" #include "qapi/qmp/qdict.h" +#include "block/qdict.h" #include "qapi/qmp/qerror.h" #include "sysemu/iothread.h" #include "qemu/guest-random.h" diff --git a/tests/unit/check-qobject.c b/tests/unit/check-qobject.c index 0ed094e55f3a..c5e850a10cb5 100644 --- a/tests/unit/check-qobject.c +++ b/tests/unit/check-qobject.c @@ -15,6 +15,7 @@ #include "qapi/qmp/qnull.h" #include "qapi/qmp/qnum.h" #include "qapi/qmp/qstring.h" +#include "block/qdict.h" #include -- 2.35.1.693.g805e0a68082a
[PATCH 22/41] include: move qemu_*_exec_dir() to cutils
From: Marc-André Lureau
The function is required by get_relocated_path(), which is used by
qemu-ga and may be generally useful.
Signed-off-by: Marc-André Lureau
---
include/qemu/cutils.h| 7 ++
include/qemu/osdep.h | 8 --
qemu-io.c| 1 +
storage-daemon/qemu-storage-daemon.c | 1 +
tests/qtest/fuzz/fuzz.c | 1 +
util/cutils.c| 109 +++
util/oslib-posix.c | 81
util/oslib-win32.c | 36 -
8 files changed, 119 insertions(+), 125 deletions(-)
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 5c6572d44422..40e10e19a7ed 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -193,6 +193,13 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n);
*/
int qemu_pstrcmp0(const char **str1, const char **str2);
+/* Find program directory, and save it for later usage with
+ * qemu_get_exec_dir().
+ * Try OS specific API first, if not working, parse from argv0. */
+void qemu_init_exec_dir(const char *argv0);
+
+/* Get the saved exec dir. */
+const char *qemu_get_exec_dir(void);
/**
* get_relocated_path:
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index a87f1b7f32e6..9fd52d6a33a7 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -567,14 +567,6 @@ bool fips_get_state(void);
*/
char *qemu_get_local_state_pathname(const char *relative_pathname);
-/* Find program directory, and save it for later usage with
- * qemu_get_exec_dir().
- * Try OS specific API first, if not working, parse from argv0. */
-void qemu_init_exec_dir(const char *argv0);
-
-/* Get the saved exec dir. */
-const char *qemu_get_exec_dir(void);
-
/**
* qemu_getauxval:
* @type: the auxiliary vector key to lookup
diff --git a/qemu-io.c b/qemu-io.c
index 952a36643b0c..458fadd99a92 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -16,6 +16,7 @@
#endif
#include "qemu/copyright.h"
+#include "qemu/cutils.h"
#include "qapi/error.h"
#include "qemu-io.h"
#include "qemu/error-report.h"
diff --git a/storage-daemon/qemu-storage-daemon.c
b/storage-daemon/qemu-storage-daemon.c
index a4415e8c995b..63b155013ed7 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -44,6 +44,7 @@
#include "qemu/copyright.h"
#include "qemu-version.h"
+#include "qemu/cutils.h"
#include "qemu/config-file.h"
#include "qemu/error-report.h"
#include "qemu/help_option.h"
diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
index 5f77c849837f..d3afd294db24 100644
--- a/tests/qtest/fuzz/fuzz.c
+++ b/tests/qtest/fuzz/fuzz.c
@@ -15,6 +15,7 @@
#include
+#include "qemu/cutils.h"
#include "qemu/datadir.h"
#include "sysemu/sysemu.h"
#include "sysemu/qtest.h"
diff --git a/util/cutils.c b/util/cutils.c
index b2777210e7da..443927275fdb 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -931,6 +931,115 @@ static inline const char *next_component(const char *dir,
int *p_len)
return dir;
}
+static const char *exec_dir;
+
+void qemu_init_exec_dir(const char *argv0)
+{
+#ifdef G_OS_WIN32
+char *p;
+char buf[MAX_PATH];
+DWORD len;
+
+if (exec_dir) {
+return;
+}
+
+len = GetModuleFileName(NULL, buf, sizeof(buf) - 1);
+if (len == 0) {
+return;
+}
+
+buf[len] = 0;
+p = buf + len - 1;
+while (p != buf && *p != '\\') {
+p--;
+}
+*p = 0;
+if (access(buf, R_OK) == 0) {
+exec_dir = g_strdup(buf);
+} else {
+exec_dir = CONFIG_BINDIR;
+}
+#else
+char *p = NULL;
+char buf[PATH_MAX];
+
+if (exec_dir) {
+return;
+}
+
+#if defined(__linux__)
+{
+int len;
+len = readlink("/proc/self/exe", buf, sizeof(buf) - 1);
+if (len > 0) {
+buf[len] = 0;
+p = buf;
+}
+}
+#elif defined(__FreeBSD__) \
+ || (defined(__NetBSD__) && defined(KERN_PROC_PATHNAME))
+{
+#if defined(__FreeBSD__)
+static int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1};
+#else
+static int mib[4] = {CTL_KERN, KERN_PROC_ARGS, -1, KERN_PROC_PATHNAME};
+#endif
+size_t len = sizeof(buf) - 1;
+
+*buf = '\0';
+if (!sysctl(mib, ARRAY_SIZE(mib), buf, , NULL, 0) &&
+*buf) {
+buf[sizeof(buf) - 1] = '\0';
+p = buf;
+}
+}
+#elif defined(__APPLE__)
+{
+char fpath[PATH_MAX];
+uint32_t len = sizeof(fpath);
+if (_NSGetExecutablePath(fpath, ) == 0) {
+p = realpath(fpath, buf);
+if (!p) {
+return;
+}
+}
+}
+#elif defined(__HAIKU__)
+{
+image_info ii;
+int32_t c = 0;
+
+*buf = '\0';
+while (get_next_image_info(0, , ) == B_OK) {
+if (ii.type == B_APP_IMAGE) {
+strncpy(buf, ii.name,
[PATCH 19/41] compiler.h: replace QEMU_NORETURN with G_NORETURN
From: Marc-André Lureau G_NORETURN was introduced in glib 2.68, fallback to G_GNUC_NORETURN in glib-compat. Note that this attribute must be placed before the function declaration (bringing a bit of consistency in qemu codebase usage). Signed-off-by: Marc-André Lureau --- accel/tcg/internal.h | 3 +-- include/exec/exec-all.h | 20 +- include/exec/helper-head.h | 2 +- include/glib-compat.h| 4 include/hw/core/cpu.h| 2 +- include/hw/core/tcg-cpu-ops.h| 6 +++--- include/hw/hw.h | 2 +- include/qemu/compiler.h | 2 -- include/qemu/osdep.h | 3 ++- include/qemu/thread.h| 2 +- include/tcg/tcg-ldst.h | 4 ++-- include/tcg/tcg.h| 2 +- linux-user/user-internals.h | 2 +- scripts/cocci-macro-file.h | 2 +- target/alpha/cpu.h | 10 - target/arm/internals.h | 12 +-- target/hppa/cpu.h| 2 +- target/i386/tcg/helper-tcg.h | 24 ++--- target/microblaze/cpu.h | 6 +++--- target/mips/tcg/tcg-internal.h | 17 --- target/nios2/cpu.h | 6 +++--- target/openrisc/exception.h | 2 +- target/ppc/cpu.h | 14 ++--- target/ppc/internal.h| 6 +++--- target/riscv/cpu.h | 10 - target/s390x/s390x-internal.h| 6 +++--- target/s390x/tcg/tcg_s390x.h | 12 +-- target/sh4/cpu.h | 6 +++--- target/sparc/cpu.h | 10 - target/xtensa/cpu.h | 6 +++--- accel/stubs/tcg-stub.c | 4 ++-- bsd-user/signal.c| 3 ++- hw/misc/mips_itu.c | 3 ++- linux-user/signal.c | 3 ++- monitor/hmp.c| 4 ++-- qemu-img.c | 12 +++ target/alpha/helper.c| 10 - target/arm/pauth_helper.c| 4 ++-- target/arm/tlb_helper.c | 7 --- target/hexagon/op_helper.c | 9 target/hppa/cpu.c| 8 +++ target/hppa/op_helper.c | 4 ++-- target/i386/tcg/bpt_helper.c | 2 +- target/i386/tcg/excp_helper.c| 31 ++-- target/i386/tcg/misc_helper.c| 6 +++--- target/i386/tcg/sysemu/misc_helper.c | 7 --- target/openrisc/exception.c | 2 +- target/openrisc/exception_helper.c | 3 ++- target/riscv/op_helper.c | 4 ++-- target/rx/op_helper.c| 22 +++- target/s390x/tcg/excp_helper.c | 22 +++- target/sh4/op_helper.c | 5 +++-- target/sparc/mmu_helper.c| 8 +++ target/tricore/op_helper.c | 6 +++--- tcg/tcg.c| 3 ++- tests/fp/fp-bench.c | 3 ++- tests/fp/fp-test.c | 3 ++- scripts/checkpatch.pl| 2 +- 58 files changed, 214 insertions(+), 191 deletions(-) diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h index 881bc1ede0b1..3092bfa96430 100644 --- a/accel/tcg/internal.h +++ b/accel/tcg/internal.h @@ -14,8 +14,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, target_ulong pc, target_ulong cs_base, uint32_t flags, int cflags); - -void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr); +G_NORETURN void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr); void page_init(void); void tb_htable_init(void); diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index d2cb0981f405..311e5fb422a3 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -58,10 +58,10 @@ void restore_state_to_opc(CPUArchState *env, TranslationBlock *tb, */ bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit); -void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu); -void QEMU_NORETURN cpu_loop_exit(CPUState *cpu); -void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); -void QEMU_NORETURN cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc); +G_NORETURN void cpu_loop_exit_noexc(CPUState *cpu); +G_NORETURN void cpu_loop_exit(CPUState *cpu); +G_NORETURN void cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); +G_NORETURN void cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc); /** * cpu_loop_exit_requested: @@ -669,9 +669,9 @@ bool handle_sigsegv_accerr_write(CPUState *cpu, sigset_t *old_set, * Use the TCGCPUOps hook to record cpu state, do guest operating system * specific things to raise SIGSEGV, and jump to the main cpu loop. */ -void QEMU_NORETURN cpu_loop_exit_sigsegv(CPUState *cpu, target_ulong addr, -
[PATCH 28/41] Use QEMU_SANITIZE_ADDRESS
From: Marc-André Lureau
Signed-off-by: Marc-André Lureau
---
tests/qtest/fdc-test.c| 2 +-
util/coroutine-ucontext.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index 4aa72f36431f..0b3c2c0d523f 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -550,7 +550,7 @@ static void fuzz_registers(void)
static bool qtest_check_clang_sanitizer(void)
{
-#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+#ifdef QEMU_SANITIZE_ADDRESS
return true;
#else
g_test_skip("QEMU not configured using --enable-sanitizers");
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 904b375192ca..52725f5344fb 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -30,7 +30,7 @@
#include
#endif
-#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+#ifdef QEMU_SANITIZE_THREAD
#ifdef CONFIG_ASAN_IFACE_FIBER
#define CONFIG_ASAN 1
#include
--
2.35.1.693.g805e0a68082a
[PATCH 06/41] include: rename qemu-common.h qemu/copyright.h
From: Marc-André Lureau
Suggested-by: Peter Maydell
Signed-off-by: Marc-André Lureau
---
include/{qemu-common.h => qemu/copyright.h} | 0
bsd-user/main.c | 2 +-
linux-user/main.c | 2 +-
qemu-img.c | 2 +-
qemu-io.c | 2 +-
qemu-nbd.c | 2 +-
qga/main.c | 2 +-
scsi/qemu-pr-helper.c | 2 +-
softmmu/vl.c| 2 +-
storage-daemon/qemu-storage-daemon.c| 2 +-
tools/virtiofsd/passthrough_ll.c| 2 +-
ui/cocoa.m | 2 +-
12 files changed, 11 insertions(+), 11 deletions(-)
rename include/{qemu-common.h => qemu/copyright.h} (100%)
diff --git a/include/qemu-common.h b/include/qemu/copyright.h
similarity index 100%
rename from include/qemu-common.h
rename to include/qemu/copyright.h
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 88d347d05ebf..aaab3f278534 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -24,7 +24,7 @@
#include
#include "qemu/osdep.h"
-#include "qemu-common.h"
+#include "qemu/copyright.h"
#include "qemu/units.h"
#include "qemu/accel.h"
#include "sysemu/tcg.h"
diff --git a/linux-user/main.c b/linux-user/main.c
index fbc9bcfd5f5f..744d216b1e8e 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -18,7 +18,7 @@
*/
#include "qemu/osdep.h"
-#include "qemu-common.h"
+#include "qemu/copyright.h"
#include "qemu/units.h"
#include "qemu/accel.h"
#include "sysemu/tcg.h"
diff --git a/qemu-img.c b/qemu-img.c
index 116e05867558..a2b1d3653a1e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -25,7 +25,7 @@
#include "qemu/osdep.h"
#include
-#include "qemu-common.h"
+#include "qemu/copyright.h"
#include "qemu/qemu-progress.h"
#include "qemu-version.h"
#include "qapi/error.h"
diff --git a/qemu-io.c b/qemu-io.c
index eb8afc8b413b..952a36643b0c 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -15,7 +15,7 @@
#include
#endif
-#include "qemu-common.h"
+#include "qemu/copyright.h"
#include "qapi/error.h"
#include "qemu-io.h"
#include "qemu/error-report.h"
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 713e7557a9eb..f4d121c0c40e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -21,7 +21,7 @@
#include
#include
-#include "qemu-common.h"
+#include "qemu/copyright.h"
#include "qapi/error.h"
#include "qemu/cutils.h"
#include "sysemu/block-backend.h"
diff --git a/qga/main.c b/qga/main.c
index ac63d8e47802..8994f73e4735 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -18,7 +18,7 @@
#include
#include
#endif
-#include "qemu-common.h"
+#include "qemu/copyright.h"
#include "qapi/qmp/json-parser.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qjson.h"
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index f281daeced8d..e7549ffb3bc9 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -36,7 +36,7 @@
#include
#endif
-#include "qemu-common.h"
+#include "qemu/copyright.h"
#include "qapi/error.h"
#include "qemu/cutils.h"
#include "qemu/main-loop.h"
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 46aba6a039c4..b0bf16e16aaa 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -23,7 +23,7 @@
*/
#include "qemu/osdep.h"
-#include "qemu-common.h"
+#include "qemu/copyright.h"
#include "qemu/datadir.h"
#include "qemu/units.h"
#include "exec/cpu-common.h"
diff --git a/storage-daemon/qemu-storage-daemon.c
b/storage-daemon/qemu-storage-daemon.c
index eb724072579a..a4415e8c995b 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -42,7 +42,7 @@
#include "qapi/qmp/qstring.h"
#include "qapi/qobject-input-visitor.h"
-#include "qemu-common.h"
+#include "qemu/copyright.h"
#include "qemu-version.h"
#include "qemu/config-file.h"
#include "qemu/error-report.h"
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 028dacdd8f5a..8af28f5fb823 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -38,7 +38,7 @@
#include "qemu/osdep.h"
#include "qemu/timer.h"
#include "qemu-version.h"
-#include "qemu-common.h"
+#include "qemu/copyright.h"
#include "fuse_virtio.h"
#include "fuse_log.h"
#include "fuse_lowlevel.h"
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 839ae4f58a69..a2a74656fabf 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -27,7 +27,7 @@
#import
#include
-#include "qemu-common.h"
+#include "qemu/copyright.h"
#include "qemu-main.h"
#include "ui/clipboard.h"
#include "ui/console.h"
--
2.35.1.693.g805e0a68082a
[PULL 7/8] iotests/108: Test new refcount rebuild algorithm
One clear problem with how qcow2's refcount structure rebuild algorithm
used to be before "qcow2: Improve refcount structure rebuilding" was
that it is prone to failure for qcow2 images on block devices: There is
generally unused space after the actual image, and if that exceeds what
one refblock covers, the old algorithm would invariably write the
reftable past the block device's end, which cannot work. The new
algorithm does not have this problem.
Test it with three tests:
(1) Create an image with more empty space at the end than what one
refblock covers, see whether rebuilding the refcount structures
results in a change in the image file length. (It should not.)
(2) Leave precisely enough space somewhere at the beginning of the image
for the new reftable (and the refblock for that place), see whether
the new algorithm puts the reftable there. (It should.)
(3) Test the original problem: Create (something like) a block device
with a fixed size, then create a qcow2 image in there, write some
data, and then have qemu-img check rebuild the refcount structures.
Before HEAD^, the reftable would have been written past the image
file end, i.e. outside of what the block device provides, which
cannot work. HEAD^ should have fixed that.
("Something like a block device" means a loop device if we can use
one ("sudo -n losetup" works), or a FUSE block export with
growable=false otherwise.)
Reviewed-by: Eric Blake
Signed-off-by: Hanna Reitz
Message-Id: <20220405134652.19278-3-hre...@redhat.com>
---
tests/qemu-iotests/108 | 259 -
tests/qemu-iotests/108.out | 81
2 files changed, 339 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
index 56339ab2c5..688d3ae8f6 100755
--- a/tests/qemu-iotests/108
+++ b/tests/qemu-iotests/108
@@ -30,13 +30,20 @@ status=1# failure is the default!
_cleanup()
{
- _cleanup_test_img
+_cleanup_test_img
+if [ -f "$TEST_DIR/qsd.pid" ]; then
+qsd_pid=$(cat "$TEST_DIR/qsd.pid")
+kill -KILL "$qsd_pid"
+fusermount -u "$TEST_DIR/fuse-export" &>/dev/null
+fi
+rm -f "$TEST_DIR/fuse-export"
}
trap "_cleanup; exit \$status" 0 1 2 3 15
# get standard environment, filters and checks
. ./common.rc
. ./common.filter
+. ./common.qemu
# This tests qcow2-specific low-level functionality
_supported_fmt qcow2
@@ -47,6 +54,22 @@ _supported_os Linux
# files
_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
+# This test either needs sudo -n losetup or FUSE exports to work
+if sudo -n losetup &>/dev/null; then
+loopdev=true
+else
+loopdev=false
+
+# QSD --export fuse will either yield "Parameter 'id' is missing"
+# or "Invalid parameter 'fuse'", depending on whether there is
+# FUSE support or not.
+error=$($QSD --export fuse 2>&1)
+if [[ $error = *"'fuse'"* ]]; then
+_notrun 'Passwordless sudo for losetup or FUSE support required, but' \
+'neither is available'
+fi
+fi
+
echo
echo '=== Repairing an image without any refcount table ==='
echo
@@ -138,6 +161,240 @@ _make_test_img 64M
poke_file "$TEST_IMG" $((0x10008)) "\xff\xff\xff\xff\xff\xff\x00\x00"
_check_test_img -r all
+echo
+echo '=== Check rebuilt reftable location ==='
+
+# In an earlier version of the refcount rebuild algorithm, the
+# reftable was generally placed at the image end (unless something was
+# allocated in the area covered by the refblock right before the image
+# file end, then we would try to place the reftable in that refblock).
+# This was later changed so the reftable would be placed in the
+# earliest possible location. Test this.
+
+echo
+echo '--- Does the image size increase? ---'
+echo
+
+# First test: Just create some image, write some data to it, and
+# resize it so there is free space at the end of the image (enough
+# that it spans at least one full refblock, which for cluster_size=512
+# images, spans 128k). With the old algorithm, the reftable would
+# have then been placed at the end of the image file, but with the new
+# one, it will be put in that free space.
+# We want to check whether the size of the image file increases due to
+# rebuilding the refcount structures (it should not).
+
+_make_test_img -o 'cluster_size=512' 1M
+# Write something
+$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+# Add free space
+file_len=$(stat -c '%s' "$TEST_IMG")
+truncate -s $((file_len + 256 * 1024)) "$TEST_IMG"
+
+# Corrupt the image by saying the image header was not allocated
+rt_offset=$(peek_file_be "$TEST_IMG" 48 8)
+rb_offset=$(peek_file_be "$TEST_IMG" $rt_offset 8)
+poke_file "$TEST_IMG" $rb_offset "\x00\x00"
+
+# Check whether rebuilding the refcount structures increases the image
+# file size
+file_len=$(stat -c '%s' "$TEST_IMG")
+echo
+# The only leaks there can be are the old refcount structures that are
+# leaked
[PULL 2/8] tests/qemu-iotests: Move the bash and sanitizer checks to meson.build
From: Thomas Huth
We want to get rid of check-block.sh in the long run, so let's move
the checks for the bash version and sanitizers from check-block.sh
into the meson.build file instead.
Signed-off-by: Thomas Huth
Message-Id: <20220223093840.2515281-4-th...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/check-block.sh | 26 --
tests/qemu-iotests/meson.build | 14 ++
2 files changed, 14 insertions(+), 26 deletions(-)
diff --git a/tests/check-block.sh b/tests/check-block.sh
index f59496396c..5de2c1ba0b 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -18,36 +18,10 @@ skip() {
exit 0
}
-# Disable tests with any sanitizer except for specific ones
-SANITIZE_FLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
-ALLOWED_SANITIZE_FLAGS="safe-stack cfi-icall"
-#Remove all occurrencies of allowed Sanitize flags
-for j in ${ALLOWED_SANITIZE_FLAGS}; do
-TMP_FLAGS=${SANITIZE_FLAGS}
-SANITIZE_FLAGS=""
-for i in ${TMP_FLAGS}; do
-if ! echo ${i} | grep -q "${j}" 2>/dev/null; then
-SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
-fi
-done
-done
-if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
-# Have a sanitize flag that is not allowed, stop
-skip "Sanitizers are enabled ==> Not running the qemu-iotests."
-fi
-
if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then
skip "No qemu-system binary available ==> Not running the qemu-iotests."
fi
-if ! command -v bash >/dev/null 2>&1 ; then
-skip "bash not available ==> Not running the qemu-iotests."
-fi
-
-if LANG=C bash --version | grep -q 'GNU bash, version [123]' ; then
-skip "bash version too old ==> Not running the qemu-iotests."
-fi
-
cd tests/qemu-iotests
# QEMU_CHECK_BLOCK_AUTO is used to disable some unstable sub-tests
diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index 92f09251a6..323a4acb6a 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -2,6 +2,20 @@ if not have_tools or targetos == 'windows' or
get_option('gprof')
subdir_done()
endif
+foreach cflag: config_host['QEMU_CFLAGS'].split()
+ if cflag.startswith('-fsanitize') and \
+ not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
+message('Sanitizers are enabled ==> Disabled the qemu-iotests.')
+subdir_done()
+ endif
+endforeach
+
+bash = find_program('bash', required: false, version: '>= 4.0')
+if not bash.found()
+ message('bash >= v4.0 not available ==> Disabled the qemu-iotests.')
+ subdir_done()
+endif
+
qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
qemu_iotests_env = {'PYTHON': python.full_path()}
qemu_iotests_formats = {
--
2.35.1
Re: [PATCH] hw/nvme: add new command abort case
On Apr 20 15:31, Dmitry Tikhov wrote: > On Wed, Apr 20, 2022 at 12:54:44, Klaus Jensen wrote: > > > > NVM Command Set Specification v1.0b, Section 5.2.3. It is exactly what > > you quoted above. > > > > I think you are interpreting > > > > "If a command is aborted as a result of the Reference Tag Check bit of > > the PRCHK field being set to '1', ..." > > > > as > > > >"If a command is aborted *because* the Reference Tag Check bit of the > >PRCHK field being set to '1', ...". > Yeah, i was interpreting it exactly this way. > > > > > But that is not what it is saying. IMO, the only meaningful > > interpretation is that "If the command is aborted *as a result of* the > > check being done *because* the bit is set, *then* return an error". > Ok, but return error in this context still means to return either > Invalid Protection Information or Invalid Field in Command, isn't it? > Or why would it specify > ...then that command should be aborted with a status code of Invalid > Protection Information, but may be aborted with a status code of > Invalid Field in Command > exactly this 2 status codes? > > > > > Your interpretation would break existing hosts that set the bit. > > I also opened NVM Express 1.4 "8.3.1.5 Control of Protection Information > Checking - PRCHK" and it says > For Type 3 protection, if bit 0 of the PRCHK field is set to ‘1’, then > the command should be aborted with status Invalid Protection > Information, but may be aborted with status Invalid Field in Command. > The controller may ignore the ILBRT and EILBRT fields when Type 3 > protection is used because the computed reference tag remains > unchanged. > I think it marks clear intent to abort cmd with "Invalid Protection > Information" or "Invalid Field in Command" status codes exactly in case > reftag check bit is set. Also isn't "may ignore the ILBRT and EILBRT > fields" means not to compare reftag with ILBRT/EILBRT? If it is not > compared then reftag check error can't be returned. What the heck. This is a pretty major difference between v1.4 and v1.4b. v1.4b does not include that wording (but it *is* present in v1.3d). You are absolutely right that this conveys the intent to abort the command. Looks like this was lost in the changes in that section between v1.4 and v1.4b. This explains the wording in v2.0 - the spec people realized they screwed up and now they have to accept both behaviors. > > But anyways, spec says that "should" and "may" indicates flexibility of > choice and not mandatory behavior. So if you think that current behavior > is right i don't insist. I'm not so sure now. Another question for the spec people... I'll get back to you. signature.asc Description: PGP signature
[PULL 5/8] iotests/303: Check for zstd support
303 runs two test cases, one of which requires zstd support.
Unfortunately, given that this is not a unittest-style test, we cannot
easily skip that single case, and instead can only skip the whole test.
(Alternatively, we could split this test into a zlib and a zstd part,
but that seems excessive, given that this test is not in auto and thus
likely only run by developers who have zstd support compiled in.)
Fixes: 677e0bae686e7c670a71d1f ("iotest 303: explicit compression type")
Signed-off-by: Hanna Reitz
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20220323105522.53660-4-hre...@redhat.com>
---
tests/qemu-iotests/303 | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
index 93aa5ce9b7..40e947f26c 100755
--- a/tests/qemu-iotests/303
+++ b/tests/qemu-iotests/303
@@ -21,10 +21,12 @@
import iotests
import subprocess
-from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io
+from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io, \
+verify_qcow2_zstd_compression
iotests.script_initialize(supported_fmts=['qcow2'],
unsupported_imgopts=['refcount_bits', 'compat'])
+verify_qcow2_zstd_compression()
disk = file_path('disk')
chunk = 1024 * 1024
--
2.35.1
[PULL 6/8] qcow2: Improve refcount structure rebuilding
When rebuilding the refcount structures (when qemu-img check -r found
errors with refcount = 0, but reference count > 0), the new refcount
table defaults to being put at the image file end[1]. There is no good
reason for that except that it means we will not have to rewrite any
refblocks we already wrote to disk.
Changing the code to rewrite those refblocks is not too difficult,
though, so let us do that. That is beneficial for images on block
devices, where we cannot really write beyond the end of the image file.
Use this opportunity to add extensive comments to the code, and refactor
it a bit, getting rid of the backwards-jumping goto.
[1] Unless there is something allocated in the area pointed to by the
last refblock, so we have to write that refblock. In that case, we
try to put the reftable in there.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1519071
Closes: https://gitlab.com/qemu-project/qemu/-/issues/941
Reviewed-by: Eric Blake
Signed-off-by: Hanna Reitz
Message-Id: <20220405134652.19278-2-hre...@redhat.com>
---
block/qcow2-refcount.c | 332 +
1 file changed, 235 insertions(+), 97 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b91499410c..c5669eaa51 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2438,111 +2438,140 @@ static int64_t alloc_clusters_imrt(BlockDriverState
*bs,
}
/*
- * Creates a new refcount structure based solely on the in-memory information
- * given through *refcount_table. All necessary allocations will be reflected
- * in that array.
+ * Helper function for rebuild_refcount_structure().
*
- * On success, the old refcount structure is leaked (it will be covered by the
- * new refcount structure).
+ * Scan the range of clusters [first_cluster, end_cluster) for allocated
+ * clusters and write all corresponding refblocks to disk. The refblock
+ * and allocation data is taken from the in-memory refcount table
+ * *refcount_table[] (of size *nb_clusters), which is basically one big
+ * (unlimited size) refblock for the whole image.
+ *
+ * For these refblocks, clusters are allocated using said in-memory
+ * refcount table. Care is taken that these allocations are reflected
+ * in the refblocks written to disk.
+ *
+ * The refblocks' offsets are written into a reftable, which is
+ * *on_disk_reftable_ptr[] (of size *on_disk_reftable_entries_ptr). If
+ * that reftable is of insufficient size, it will be resized to fit.
+ * This reftable is not written to disk.
+ *
+ * (If *on_disk_reftable_ptr is not NULL, the entries within are assumed
+ * to point to existing valid refblocks that do not need to be allocated
+ * again.)
+ *
+ * Return whether the on-disk reftable array was resized (true/false),
+ * or -errno on error.
*/
-static int rebuild_refcount_structure(BlockDriverState *bs,
- BdrvCheckResult *res,
- void **refcount_table,
- int64_t *nb_clusters)
+static int rebuild_refcounts_write_refblocks(
+BlockDriverState *bs, void **refcount_table, int64_t *nb_clusters,
+int64_t first_cluster, int64_t end_cluster,
+uint64_t **on_disk_reftable_ptr, uint32_t *on_disk_reftable_entries_ptr
+)
{
BDRVQcow2State *s = bs->opaque;
-int64_t first_free_cluster = 0, reftable_offset = -1, cluster = 0;
+int64_t cluster;
int64_t refblock_offset, refblock_start, refblock_index;
-uint32_t reftable_size = 0;
-uint64_t *on_disk_reftable = NULL;
+int64_t first_free_cluster = 0;
+uint64_t *on_disk_reftable = *on_disk_reftable_ptr;
+uint32_t on_disk_reftable_entries = *on_disk_reftable_entries_ptr;
void *on_disk_refblock;
-int ret = 0;
-struct {
-uint64_t reftable_offset;
-uint32_t reftable_clusters;
-} QEMU_PACKED reftable_offset_and_clusters;
-
-qcow2_cache_empty(bs, s->refcount_block_cache);
+bool reftable_grown = false;
+int ret;
-write_refblocks:
-for (; cluster < *nb_clusters; cluster++) {
+for (cluster = first_cluster; cluster < end_cluster; cluster++) {
+/* Check all clusters to find refblocks that contain non-zero entries
*/
if (!s->get_refcount(*refcount_table, cluster)) {
continue;
}
+/*
+ * This cluster is allocated, so we need to create a refblock
+ * for it. The data we will write to disk is just the
+ * respective slice from *refcount_table, so it will contain
+ * accurate refcounts for all clusters belonging to this
+ * refblock. After we have written it, we will therefore skip
+ * all remaining clusters in this refblock.
+ */
+
refblock_index = cluster >> s->refcount_block_bits;
refblock_start = refblock_index << s->refcount_block_bits;
-/* Don't allocate a cluster in a refblock already written to
[PULL 8/8] qcow2: Add errp to rebuild_refcount_structure()
Instead of fprint()-ing error messages in rebuild_refcount_structure()
and its rebuild_refcounts_write_refblocks() helper, pass them through an
Error object to qcow2_check_refcounts() (which will then print it).
Suggested-by: Eric Blake
Signed-off-by: Hanna Reitz
Message-Id: <20220405134652.19278-4-hre...@redhat.com>
Reviewed-by: Eric Blake
---
block/qcow2-refcount.c | 33 +++--
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c5669eaa51..ed0ecfaa89 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2465,7 +2465,8 @@ static int64_t alloc_clusters_imrt(BlockDriverState *bs,
static int rebuild_refcounts_write_refblocks(
BlockDriverState *bs, void **refcount_table, int64_t *nb_clusters,
int64_t first_cluster, int64_t end_cluster,
-uint64_t **on_disk_reftable_ptr, uint32_t *on_disk_reftable_entries_ptr
+uint64_t **on_disk_reftable_ptr, uint32_t
*on_disk_reftable_entries_ptr,
+Error **errp
)
{
BDRVQcow2State *s = bs->opaque;
@@ -2516,8 +2517,8 @@ static int rebuild_refcounts_write_refblocks(
nb_clusters,
_free_cluster);
if (refblock_offset < 0) {
-fprintf(stderr, "ERROR allocating refblock: %s\n",
-strerror(-refblock_offset));
+error_setg_errno(errp, -refblock_offset,
+ "ERROR allocating refblock");
return refblock_offset;
}
@@ -2539,6 +2540,7 @@ static int rebuild_refcounts_write_refblocks(
on_disk_reftable_entries *
REFTABLE_ENTRY_SIZE);
if (!on_disk_reftable) {
+error_setg(errp, "ERROR allocating reftable memory");
return -ENOMEM;
}
@@ -2562,7 +2564,7 @@ static int rebuild_refcounts_write_refblocks(
ret = qcow2_pre_write_overlap_check(bs, 0, refblock_offset,
s->cluster_size, false);
if (ret < 0) {
-fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret));
+error_setg_errno(errp, -ret, "ERROR writing refblock");
return ret;
}
@@ -2578,7 +2580,7 @@ static int rebuild_refcounts_write_refblocks(
ret = bdrv_pwrite(bs->file, refblock_offset, on_disk_refblock,
s->cluster_size);
if (ret < 0) {
-fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret));
+error_setg_errno(errp, -ret, "ERROR writing refblock");
return ret;
}
@@ -2601,7 +2603,8 @@ static int rebuild_refcounts_write_refblocks(
static int rebuild_refcount_structure(BlockDriverState *bs,
BdrvCheckResult *res,
void **refcount_table,
- int64_t *nb_clusters)
+ int64_t *nb_clusters,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
int64_t reftable_offset = -1;
@@ -2652,7 +2655,7 @@ static int rebuild_refcount_structure(BlockDriverState
*bs,
rebuild_refcounts_write_refblocks(bs, refcount_table, nb_clusters,
0, *nb_clusters,
_disk_reftable,
- _disk_reftable_entries);
+ _disk_reftable_entries, errp);
if (reftable_size_changed < 0) {
res->check_errors++;
ret = reftable_size_changed;
@@ -2676,8 +2679,8 @@ static int rebuild_refcount_structure(BlockDriverState
*bs,
refcount_table, nb_clusters,
_free_cluster);
if (reftable_offset < 0) {
-fprintf(stderr, "ERROR allocating reftable: %s\n",
-strerror(-reftable_offset));
+error_setg_errno(errp, -reftable_offset,
+ "ERROR allocating reftable");
res->check_errors++;
ret = reftable_offset;
goto fail;
@@ -2695,7 +2698,7 @@ static int rebuild_refcount_structure(BlockDriverState
*bs,
reftable_start_cluster,
reftable_end_cluster,
_disk_reftable,
- _disk_reftable_entries);
+ _disk_reftable_entries, errp);
if (reftable_size_changed < 0) {
res->check_errors++;
ret = reftable_size_changed;
@@ -2725,7
[PULL 1/8] tests/qemu-iotests/meson.build: Improve the indentation
From: Thomas Huth
By using subdir_done(), we can get rid of one level of indentation
in this file. This will make it easier to add more conditions to
skip the iotests in future patches.
Reviewed-by: Hanna Reitz
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Thomas Huth
Message-Id: <20220223093840.2515281-3-th...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/meson.build | 61 ++
1 file changed, 32 insertions(+), 29 deletions(-)
diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index 9747bb68a5..92f09251a6 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -1,30 +1,33 @@
-if have_tools and targetos != 'windows' and not get_option('gprof')
- qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
- qemu_iotests_env = {'PYTHON': python.full_path()}
- qemu_iotests_formats = {
-'qcow2': 'quick',
-'raw': 'slow',
-'qed': 'thorough',
-'vmdk': 'thorough',
-'vpc': 'thorough'
- }
-
- foreach k, v : emulators
-if k.startswith('qemu-system-')
- qemu_iotests_binaries += v
-endif
- endforeach
- foreach format, speed: qemu_iotests_formats
-if speed == 'quick'
- suites = 'block'
-else
- suites = ['block-' + speed, speed]
-endif
-test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'),
format],
- depends: qemu_iotests_binaries, env: qemu_iotests_env,
- protocol: 'tap',
- suite: suites,
- timeout: 0,
- is_parallel: false)
- endforeach
+if not have_tools or targetos == 'windows' or get_option('gprof')
+ subdir_done()
endif
+
+qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
+qemu_iotests_env = {'PYTHON': python.full_path()}
+qemu_iotests_formats = {
+ 'qcow2': 'quick',
+ 'raw': 'slow',
+ 'qed': 'thorough',
+ 'vmdk': 'thorough',
+ 'vpc': 'thorough'
+}
+
+foreach k, v : emulators
+ if k.startswith('qemu-system-')
+qemu_iotests_binaries += v
+ endif
+endforeach
+
+foreach format, speed: qemu_iotests_formats
+ if speed == 'quick'
+suites = 'block'
+ else
+suites = ['block-' + speed, speed]
+ endif
+ test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'),
format],
+ depends: qemu_iotests_binaries, env: qemu_iotests_env,
+ protocol: 'tap',
+ suite: suites,
+ timeout: 0,
+ is_parallel: false)
+endforeach
--
2.35.1
[PULL 3/8] iotests.py: Add supports_qcow2_zstd_compression()
Suggested-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Hanna Reitz
Message-Id: <20220323105522.53660-2-hre...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/iotests.py | 20
1 file changed, 20 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index fcec3e51e5..fe10a6cf05 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1471,6 +1471,26 @@ def verify_working_luks():
if not working:
notrun(reason)
+def supports_qcow2_zstd_compression() -> bool:
+img_file = f'{test_dir}/qcow2-zstd-test.qcow2'
+res = qemu_img('create', '-f', 'qcow2', '-o', 'compression_type=zstd',
+ img_file, '0',
+ check=False)
+try:
+os.remove(img_file)
+except OSError:
+pass
+
+if res.returncode == 1 and \
+"'compression-type' does not accept value 'zstd'" in res.stdout:
+return False
+else:
+return True
+
+def verify_qcow2_zstd_compression():
+if not supports_qcow2_zstd_compression():
+notrun('zstd compression not supported')
+
def qemu_pipe(*args: str) -> str:
"""
Run qemu with an option to print something and exit (e.g. a help option).
--
2.35.1
[PULL 4/8] iotests/065: Check for zstd support
Some test cases run in iotest 065 want to run with zstd compression just
for added coverage. Run them with zlib if there is no zstd support
compiled in.
Reported-by: Thomas Huth
Fixes: 12a936171d71f839dc907ff ("iotest 065: explicit compression type")
Signed-off-by: Hanna Reitz
Message-Id: <20220323105522.53660-3-hre...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/065 | 24 ++--
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index ba94e19349..b724c89c7c 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -24,7 +24,7 @@ import os
import re
import json
import iotests
-from iotests import qemu_img, qemu_img_info
+from iotests import qemu_img, qemu_img_info, supports_qcow2_zstd_compression
import unittest
test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -95,11 +95,17 @@ class TestQCow2(TestQemuImgInfo):
class TestQCow3NotLazy(TestQemuImgInfo):
'''Testing a qcow2 version 3 image with lazy refcounts disabled'''
-img_options = 'compat=1.1,lazy_refcounts=off,compression_type=zstd'
+if supports_qcow2_zstd_compression():
+compression_type = 'zstd'
+else:
+compression_type = 'zlib'
+
+img_options = 'compat=1.1,lazy_refcounts=off'
+img_options += f',compression_type={compression_type}'
json_compare = { 'compat': '1.1', 'lazy-refcounts': False,
'refcount-bits': 16, 'corrupt': False,
- 'compression-type': 'zstd', 'extended-l2': False }
-human_compare = [ 'compat: 1.1', 'compression type: zstd',
+ 'compression-type': compression_type, 'extended-l2':
False }
+human_compare = [ 'compat: 1.1', f'compression type: {compression_type}',
'lazy refcounts: false', 'refcount bits: 16',
'corrupt: false', 'extended l2: false' ]
@@ -126,11 +132,17 @@ class TestQCow3NotLazyQMP(TestQMP):
class TestQCow3LazyQMP(TestQMP):
'''Testing a qcow2 version 3 image with lazy refcounts enabled, opening
with lazy refcounts disabled'''
-img_options = 'compat=1.1,lazy_refcounts=on,compression_type=zstd'
+if supports_qcow2_zstd_compression():
+compression_type = 'zstd'
+else:
+compression_type = 'zlib'
+
+img_options = 'compat=1.1,lazy_refcounts=on'
+img_options += f',compression_type={compression_type}'
qemu_options = 'lazy-refcounts=off'
compare = { 'compat': '1.1', 'lazy-refcounts': True,
'refcount-bits': 16, 'corrupt': False,
-'compression-type': 'zstd', 'extended-l2': False }
+'compression-type': compression_type, 'extended-l2': False }
TestImageInfoSpecific = None
TestQemuImgInfo = None
--
2.35.1
[PULL 0/8] Block patches
The following changes since commit 1be5a765c08cee3a9587c8a8d3fc2ea247b13f9c: Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging (2022-04-19 18:22:16 -0700) are available in the Git repository at: https://gitlab.com/hreitz/qemu.git tags/pull-block-2022-04-20 for you to fetch changes up to 0423f75351ab83b844a31349218b0eadd830e07a: qcow2: Add errp to rebuild_refcount_structure() (2022-04-20 12:09:17 +0200) Block patches: - Some changes for qcow2's refcount repair algorithm to make it work for qcow2 images stored on block devices - Skip test cases that require zstd when support for it is missing - Some refactoring in the iotests' meson.build Hanna Reitz (6): iotests.py: Add supports_qcow2_zstd_compression() iotests/065: Check for zstd support iotests/303: Check for zstd support qcow2: Improve refcount structure rebuilding iotests/108: Test new refcount rebuild algorithm qcow2: Add errp to rebuild_refcount_structure() Thomas Huth (2): tests/qemu-iotests/meson.build: Improve the indentation tests/qemu-iotests: Move the bash and sanitizer checks to meson.build block/qcow2-refcount.c | 353 +++-- tests/check-block.sh | 26 --- tests/qemu-iotests/065 | 24 ++- tests/qemu-iotests/108 | 259 +++- tests/qemu-iotests/108.out | 81 tests/qemu-iotests/303 | 4 +- tests/qemu-iotests/iotests.py | 20 ++ tests/qemu-iotests/meson.build | 73 --- 8 files changed, 673 insertions(+), 167 deletions(-) -- 2.35.1
Re: [PATCH] hw/nvme: add new command abort case
On Wed, Apr 20, 2022 at 12:54:44, Klaus Jensen wrote: > > NVM Command Set Specification v1.0b, Section 5.2.3. It is exactly what > you quoted above. > > I think you are interpreting > > "If a command is aborted as a result of the Reference Tag Check bit of > the PRCHK field being set to '1', ..." > > as > >"If a command is aborted *because* the Reference Tag Check bit of the >PRCHK field being set to '1', ...". Yeah, i was interpreting it exactly this way. > > But that is not what it is saying. IMO, the only meaningful > interpretation is that "If the command is aborted *as a result of* the > check being done *because* the bit is set, *then* return an error". Ok, but return error in this context still means to return either Invalid Protection Information or Invalid Field in Command, isn't it? Or why would it specify ...then that command should be aborted with a status code of Invalid Protection Information, but may be aborted with a status code of Invalid Field in Command exactly this 2 status codes? > > Your interpretation would break existing hosts that set the bit. I also opened NVM Express 1.4 "8.3.1.5 Control of Protection Information Checking - PRCHK" and it says For Type 3 protection, if bit 0 of the PRCHK field is set to ‘1’, then the command should be aborted with status Invalid Protection Information, but may be aborted with status Invalid Field in Command. The controller may ignore the ILBRT and EILBRT fields when Type 3 protection is used because the computed reference tag remains unchanged. I think it marks clear intent to abort cmd with "Invalid Protection Information" or "Invalid Field in Command" status codes exactly in case reftag check bit is set. Also isn't "may ignore the ILBRT and EILBRT fields" means not to compare reftag with ILBRT/EILBRT? If it is not compared then reftag check error can't be returned. But anyways, spec says that "should" and "may" indicates flexibility of choice and not mandatory behavior. So if you think that current behavior is right i don't insist.
Re: [PATCH v7 12/12] hw/acpi: Make the PCI hot-plug aware of SR-IOV
On Fri, Mar 18, 2022 at 08:18:19PM +0100, Lukasz Maniak wrote: > From: Łukasz Gieryk > > PCI device capable of SR-IOV support is a new, still-experimental > feature with only a single working example of the Nvme device. > > This patch in an attempt to fix a double-free problem when a > SR-IOV-capable Nvme device is hot-unplugged. The problem and the > reproduction steps can be found in this thread: > > https://patchew.org/QEMU/20220217174504.1051716-1-lukasz.man...@linux.intel.com/20220217174504.1051716-14-lukasz.man...@linux.intel.com/ > > Details of the proposed solution are, for convenience, included below. > > 1) The current SR-IOV implementation assumes it’s the PhysicalFunction >that creates and deletes VirtualFunctions. > 2) It’s a design decision (the Nvme device at least) for the VFs to be >of the same class as PF. Effectively, they share the dc->hotpluggable >value. > 3) When a VF is created, it’s added as a child node to PF’s PCI bus >slot. > 4) Monitor/device_del triggers the ACPI mechanism. The implementation is >not aware of SR/IOV and ejects PF’s PCI slot, directly unrealizing all >hot-pluggable (!acpi_pcihp_pc_no_hotplug) children nodes. > 5) VFs are unrealized directly, and it doesn’t work well with (1). >SR/IOV structures are not updated, so when it’s PF’s turn to be >unrealized, it works on stale pointers to already-deleted VFs. > > Signed-off-by: Łukasz Gieryk Reviewed-by: Michael S. Tsirkin feel free to include when merging the rest of the patchset. > --- > hw/acpi/pcihp.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index 6351bd3424d..248839e1110 100644 > --- a/hw/acpi/pcihp.c > +++ b/hw/acpi/pcihp.c > @@ -192,8 +192,12 @@ static bool acpi_pcihp_pc_no_hotplug(AcpiPciHpState *s, > PCIDevice *dev) > * ACPI doesn't allow hotplug of bridge devices. Don't allow > * hot-unplug of bridge devices unless they were added by hotplug > * (and so, not described by acpi). > + * > + * Don't allow hot-unplug of SR-IOV Virtual Functions, as they > + * will be removed implicitly, when Physical Function is unplugged. > */ > -return (pc->is_bridge && !dev->qdev.hotplugged) || !dc->hotpluggable; > +return (pc->is_bridge && !dev->qdev.hotplugged) || !dc->hotpluggable || > + pci_is_vf(dev); > } > > static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned > slots) > -- > 2.25.1
Re: [PATCH v7 00/12] hw/nvme: SR-IOV with Virtualization Enhancements
On Fri, Mar 18, 2022 at 08:18:07PM +0100, Lukasz Maniak wrote: > Resubmitting v6 as v7 since Patchew got lost with my sophisticated CC of > all maintainers just for the cover letter. > > Changes since v5: > - Fixed PCI hotplug issue related to deleting VF twice > - Corrected error messages for SR-IOV parameters > - Rebased on master, patches for PCI got pulled into the tree > - Added Reviewed-by labels > > Lukasz Maniak (4): > hw/nvme: Add support for SR-IOV > hw/nvme: Add support for Primary Controller Capabilities > hw/nvme: Add support for Secondary Controller List > docs: Add documentation for SR-IOV and Virtualization Enhancements > > Łukasz Gieryk (8): > hw/nvme: Implement the Function Level Reset > hw/nvme: Make max_ioqpairs and msix_qsize configurable in runtime > hw/nvme: Remove reg_size variable and update BAR0 size calculation > hw/nvme: Calculate BAR attributes in a function > hw/nvme: Initialize capability structures for primary/secondary > controllers > hw/nvme: Add support for the Virtualization Management command > hw/nvme: Update the initalization place for the AER queue > hw/acpi: Make the PCI hot-plug aware of SR-IOV the right people to review and merge this would be storage/nvme maintainers. I did take a quick look though. Acked-by: Michael S. Tsirkin > docs/system/devices/nvme.rst | 82 + > hw/acpi/pcihp.c | 6 +- > hw/nvme/ctrl.c | 673 --- > hw/nvme/ns.c | 2 +- > hw/nvme/nvme.h | 55 ++- > hw/nvme/subsys.c | 75 +++- > hw/nvme/trace-events | 6 + > include/block/nvme.h | 65 > include/hw/pci/pci_ids.h | 1 + > 9 files changed, 909 insertions(+), 56 deletions(-) > > -- > 2.25.1
Re: [PATCH v7 00/12] hw/nvme: SR-IOV with Virtualization Enhancements
On Apr 20 08:02, Michael S. Tsirkin wrote: > On Fri, Mar 18, 2022 at 08:18:07PM +0100, Lukasz Maniak wrote: > > Resubmitting v6 as v7 since Patchew got lost with my sophisticated CC of > > all maintainers just for the cover letter. > > > > Changes since v5: > > - Fixed PCI hotplug issue related to deleting VF twice > > - Corrected error messages for SR-IOV parameters > > - Rebased on master, patches for PCI got pulled into the tree > > - Added Reviewed-by labels > > > > Lukasz Maniak (4): > > hw/nvme: Add support for SR-IOV > > hw/nvme: Add support for Primary Controller Capabilities > > hw/nvme: Add support for Secondary Controller List > > docs: Add documentation for SR-IOV and Virtualization Enhancements > > > > Łukasz Gieryk (8): > > hw/nvme: Implement the Function Level Reset > > hw/nvme: Make max_ioqpairs and msix_qsize configurable in runtime > > hw/nvme: Remove reg_size variable and update BAR0 size calculation > > hw/nvme: Calculate BAR attributes in a function > > hw/nvme: Initialize capability structures for primary/secondary > > controllers > > hw/nvme: Add support for the Virtualization Management command > > hw/nvme: Update the initalization place for the AER queue > > hw/acpi: Make the PCI hot-plug aware of SR-IOV > > the right people to review and merge this would be storage/nvme > maintainers. > I did take a quick look though. > > Acked-by: Michael S. Tsirkin > Was waiting for a review on the acpi stuff. Thanks! :) signature.asc Description: PGP signature
Re: [PATCH] hw/nvme: add new command abort case
On Apr 20 13:41, Dmitry Tikhov wrote: > On Wed, Apr 20, 2022 at 12:36:54, Klaus Jensen wrote: > > On Apr 20 12:13, Klaus Jensen wrote: > > > On Apr 20 11:20, Dmitry Tikhov wrote: > > > > NVMe command set specification for end-to-end data protection formatted > > > > namespace states: > > > > > > > > o If the Reference Tag Check bit of the PRCHK field is set to ‘1’ > > > > and > > > > the namespace is formatted for Type 3 protection, then the > > > > controller: > > > > ▪ should not compare the protection Information Reference Tag > > > > field to the computed reference tag; and > > > > ▪ may ignore the ILBRT and EILBRT fields. If a command is > > > > aborted as a result of the Reference Tag Check bit of the > > > > PRCHK field being set to ‘1’, then that command should be > > > > aborted with a status code of Invalid Protection > > > > Information, > > > > but may be aborted with a status code of Invalid Field in > > > > Command. > > > > > > > > Currently qemu compares reftag in the nvme_dif_prchk function whenever > > > > Reference Tag Check bit is set in the command. For type 3 namespaces > > > > however, caller of nvme_dif_prchk - nvme_dif_check does not increment > > > > reftag for each subsequent logical block. That way commands > > > > incorporating > > > > more than one logical block for type 3 formatted namespaces with reftag > > > > check bit set, always fail with End-to-end Reference Tag Check Error. > > > > Comply with spec by handling case of set Reference Tag Check > > > > bit in the type 3 formatted namespace. > > > > > > > > > > Note the "should" and "may" in your quote. What QEMU does right now is > > > compliant with v1.4. That is, the reftag must NOT be incremented > > > - it is the same for the first and all subsequent logical blocks. > > > > > > I'm a bit hesitant to follow v2.0 here, since we do not report v2.0 > > > compliance yet. I'm honestly also a bit perplexed as to how the NVMe TWG > > > ended up considering this backwards compatible. As far as I can tell > > > this breaks current hosts that do set the reference tag check bit, > > > provides a valid ILBRT/EILBRT and expects it to succeed. > > > > Ok, so reading the spec more closely... > > > > The Invalid Protection Information should not be set just because the > > reference tag check bit is set. It should be set *if* the controller > > chooses to check it and it fails. However, in v2.0, the controller is > > allowed to ignore it and not perform the check. > > > > So, just because the host sets the bit, that does not mean we fail the > > command. However, a difference is that a v2.0 compliant controller > > should return Invalid Protection Information or Invalid Field in Command > > instead of End-to-end Reference Tag Check Error if the check fails. > > Can you please link the spec you are referring to? NVM Command Set Specification v1.0b, Section 5.2.3. It is exactly what you quoted above. I think you are interpreting "If a command is aborted as a result of the Reference Tag Check bit of the PRCHK field being set to '1', ..." as "If a command is aborted *because* the Reference Tag Check bit of the PRCHK field being set to '1', ...". But that is not what it is saying. IMO, the only meaningful interpretation is that "If the command is aborted *as a result of* the check being done *because* the bit is set, *then* return an error". Your interpretation would break existing hosts that set the bit. signature.asc Description: PGP signature
Re: [PATCH v7 12/12] hw/acpi: Make the PCI hot-plug aware of SR-IOV
On Mon, Apr 04, 2022 at 11:41:46AM +0200, Łukasz Gieryk wrote: > On Thu, Mar 31, 2022 at 02:38:41PM +0200, Igor Mammedov wrote: > > it's unclear what's bing hotpluged and unplugged, it would be better if > > you included QEMU CLI and relevan qmp/monito commands to reproduce it. > > Qemu CLI: > - > -device pcie-root-port,slot=0,id=rp0 > -device nvme-subsys,id=subsys0 > -device > nvme,id=nvme0,bus=rp0,serial=deadbeef,subsys=subsys0,sriov_max_vfs=1,sriov_vq_flexible=2,sriov_vi_flexible=1 > > Guest OS: > - > sudo nvme virt-mgmt /dev/nvme0 -c 0 -r 1 -a 1 -n 0 > sudo nvme virt-mgmt /dev/nvme0 -c 0 -r 0 -a 1 -n 0 > echo 1 > /sys/bus/pci/devices/:01:00.0/reset > sleep 1 > echo 1 > /sys/bus/pci/devices/:01:00.0/sriov_numvfs > nvme virt-mgmt /dev/nvme0 -c 1 -r 1 -a 8 -n 1 > nvme virt-mgmt /dev/nvme0 -c 1 -r 0 -a 8 -n 2 > nvme virt-mgmt /dev/nvme0 -c 1 -r 0 -a 9 -n 0 > sleep 2 > echo 01:00.1 > /sys/bus/pci/drivers/nvme/bind > > Qemu monitor: > - > device_del nvme0 > Hi Igor, Do you need any more details on this? Best regards, Lukasz
Re: [PATCH] hw/nvme: add new command abort case
On Wed, Apr 20, 2022 at 12:36:54, Klaus Jensen wrote: > On Apr 20 12:13, Klaus Jensen wrote: > > On Apr 20 11:20, Dmitry Tikhov wrote: > > > NVMe command set specification for end-to-end data protection formatted > > > namespace states: > > > > > > o If the Reference Tag Check bit of the PRCHK field is set to ‘1’ and > > > the namespace is formatted for Type 3 protection, then the > > > controller: > > > ▪ should not compare the protection Information Reference Tag > > > field to the computed reference tag; and > > > ▪ may ignore the ILBRT and EILBRT fields. If a command is > > > aborted as a result of the Reference Tag Check bit of the > > > PRCHK field being set to ‘1’, then that command should be > > > aborted with a status code of Invalid Protection Information, > > > but may be aborted with a status code of Invalid Field in > > > Command. > > > > > > Currently qemu compares reftag in the nvme_dif_prchk function whenever > > > Reference Tag Check bit is set in the command. For type 3 namespaces > > > however, caller of nvme_dif_prchk - nvme_dif_check does not increment > > > reftag for each subsequent logical block. That way commands incorporating > > > more than one logical block for type 3 formatted namespaces with reftag > > > check bit set, always fail with End-to-end Reference Tag Check Error. > > > Comply with spec by handling case of set Reference Tag Check > > > bit in the type 3 formatted namespace. > > > > > > > Note the "should" and "may" in your quote. What QEMU does right now is > > compliant with v1.4. That is, the reftag must NOT be incremented > > - it is the same for the first and all subsequent logical blocks. > > > > I'm a bit hesitant to follow v2.0 here, since we do not report v2.0 > > compliance yet. I'm honestly also a bit perplexed as to how the NVMe TWG > > ended up considering this backwards compatible. As far as I can tell > > this breaks current hosts that do set the reference tag check bit, > > provides a valid ILBRT/EILBRT and expects it to succeed. > > Ok, so reading the spec more closely... > > The Invalid Protection Information should not be set just because the > reference tag check bit is set. It should be set *if* the controller > chooses to check it and it fails. However, in v2.0, the controller is > allowed to ignore it and not perform the check. > > So, just because the host sets the bit, that does not mean we fail the > command. However, a difference is that a v2.0 compliant controller > should return Invalid Protection Information or Invalid Field in Command > instead of End-to-end Reference Tag Check Error if the check fails. Can you please link the spec you are referring to?
Re: [PATCH] hw/nvme: add new command abort case
On Apr 20 12:13, Klaus Jensen wrote: > On Apr 20 11:20, Dmitry Tikhov wrote: > > NVMe command set specification for end-to-end data protection formatted > > namespace states: > > > > o If the Reference Tag Check bit of the PRCHK field is set to ‘1’ and > > the namespace is formatted for Type 3 protection, then the > > controller: > > ▪ should not compare the protection Information Reference Tag > > field to the computed reference tag; and > > ▪ may ignore the ILBRT and EILBRT fields. If a command is > > aborted as a result of the Reference Tag Check bit of the > > PRCHK field being set to ‘1’, then that command should be > > aborted with a status code of Invalid Protection Information, > > but may be aborted with a status code of Invalid Field in > > Command. > > > > Currently qemu compares reftag in the nvme_dif_prchk function whenever > > Reference Tag Check bit is set in the command. For type 3 namespaces > > however, caller of nvme_dif_prchk - nvme_dif_check does not increment > > reftag for each subsequent logical block. That way commands incorporating > > more than one logical block for type 3 formatted namespaces with reftag > > check bit set, always fail with End-to-end Reference Tag Check Error. > > Comply with spec by handling case of set Reference Tag Check > > bit in the type 3 formatted namespace. > > > > Note the "should" and "may" in your quote. What QEMU does right now is > compliant with v1.4. That is, the reftag must NOT be incremented > - it is the same for the first and all subsequent logical blocks. > > I'm a bit hesitant to follow v2.0 here, since we do not report v2.0 > compliance yet. I'm honestly also a bit perplexed as to how the NVMe TWG > ended up considering this backwards compatible. As far as I can tell > this breaks current hosts that do set the reference tag check bit, > provides a valid ILBRT/EILBRT and expects it to succeed. Ok, so reading the spec more closely... The Invalid Protection Information should not be set just because the reference tag check bit is set. It should be set *if* the controller chooses to check it and it fails. However, in v2.0, the controller is allowed to ignore it and not perform the check. So, just because the host sets the bit, that does not mean we fail the command. However, a difference is that a v2.0 compliant controller should return Invalid Protection Information or Invalid Field in Command instead of End-to-end Reference Tag Check Error if the check fails. signature.asc Description: PGP signature
Re: [PATCH] hw/nvme: add new command abort case
On Apr 20 11:20, Dmitry Tikhov wrote: > NVMe command set specification for end-to-end data protection formatted > namespace states: > > o If the Reference Tag Check bit of the PRCHK field is set to ‘1’ and > the namespace is formatted for Type 3 protection, then the > controller: > ▪ should not compare the protection Information Reference Tag > field to the computed reference tag; and > ▪ may ignore the ILBRT and EILBRT fields. If a command is > aborted as a result of the Reference Tag Check bit of the > PRCHK field being set to ‘1’, then that command should be > aborted with a status code of Invalid Protection Information, > but may be aborted with a status code of Invalid Field in > Command. > > Currently qemu compares reftag in the nvme_dif_prchk function whenever > Reference Tag Check bit is set in the command. For type 3 namespaces > however, caller of nvme_dif_prchk - nvme_dif_check does not increment > reftag for each subsequent logical block. That way commands incorporating > more than one logical block for type 3 formatted namespaces with reftag > check bit set, always fail with End-to-end Reference Tag Check Error. > Comply with spec by handling case of set Reference Tag Check > bit in the type 3 formatted namespace. > Note the "should" and "may" in your quote. What QEMU does right now is compliant with v1.4. That is, the reftag must NOT be incremented - it is the same for the first and all subsequent logical blocks. I'm a bit hesitant to follow v2.0 here, since we do not report v2.0 compliance yet. I'm honestly also a bit perplexed as to how the NVMe TWG ended up considering this backwards compatible. As far as I can tell this breaks current hosts that do set the reference tag check bit, provides a valid ILBRT/EILBRT and expects it to succeed. signature.asc Description: PGP signature
Re: [PATCH 2/2] hw/nvme: fix copy cmd for pi enabled namespaces
On Apr 20 12:03, Dmitry Tikhov wrote: > Current implementation have two problems: > First in the read part of copy command. Because there is no metadata > mangling before nvme_dif_check invocation, reftag error is thrown for > blocks of namespace that have not been previously written to. Yes, this is definitely a bug and the fix is good, thanks! > Second in the write part. Reftag in the protection information section > of the source metadata should not be copied as is to the destination. Hmm, says who? > Source range start lba and destination range start lba could differ so > recalculation of reftag is always needed. > If PRACT is 0, we really should not touch the protection information. My interpretation of the Copy command is that it is simply just screwed if used with PRACT 0 and Type 1. PRACT bit is specifically to allow the controller to generate appropriate PI in this case. On the other hand, I can totally see your interpretation as valid as well. Let me ask some spec people about this, and I will get back to you. signature.asc Description: PGP signature
[PATCH 0/2] Fix nvme copy command with pi metadata
Current implementation of copy command, for namespace with end-to-end data protection enabled, always returns data integrity field check errors. For example, issuing with nvme-cli: nvme copy --sdlba=25 --blocks=2,1,3 --slbs=1,37,50 --prinfow=5 --prinfor=5 --ref-tag=25 --expected-ref-tags=1,37,50 /dev/nvme0n1 Always returns End-to-end Reference Tag Check Error. To reproduce you may need to use upstream version of nvme-cli since there was a bug which prevented passing prinfow to a command, fixed in 2cf9825 commit. This patch set attempts to fix copy command for data protection enabled namespaces. Dmitry Tikhov (2): hw/nvme: refactor check of disabled dif hw/nvme: fix copy cmd for pi enabled namespaces hw/nvme/ctrl.c | 5 ++ hw/nvme/dif.c| 186 +-- hw/nvme/dif.h| 3 + hw/nvme/trace-events | 4 +- 4 files changed, 155 insertions(+), 43 deletions(-) -- 2.35.1
[PATCH 2/2] hw/nvme: fix copy cmd for pi enabled namespaces
Current implementation have two problems:
First in the read part of copy command. Because there is no metadata
mangling before nvme_dif_check invocation, reftag error is thrown for
blocks of namespace that have not been previously written to.
Second in the write part. Reftag in the protection information section
of the source metadata should not be copied as is to the destination.
Source range start lba and destination range start lba could differ so
recalculation of reftag is always needed.
Signed-off-by: Dmitry Tikhov
---
hw/nvme/ctrl.c | 5
hw/nvme/dif.c | 65 ++
hw/nvme/dif.h | 2 ++
3 files changed, 72 insertions(+)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 74540a03d5..cb493f4506 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -2787,6 +2787,10 @@ static void nvme_copy_in_completed_cb(void *opaque, int
ret)
size_t mlen = nvme_m2b(ns, nlb);
uint8_t *mbounce = iocb->bounce + nvme_l2b(ns, nlb);
+status = nvme_dif_mangle_mdata(ns, mbounce, mlen, reftag);
+if (status) {
+goto invalid;
+}
status = nvme_dif_check(ns, iocb->bounce, len, mbounce, mlen, prinfor,
slba, apptag, appmask, );
if (status) {
@@ -2805,6 +2809,7 @@ static void nvme_copy_in_completed_cb(void *opaque, int
ret)
nvme_dif_pract_generate_dif(ns, iocb->bounce, len, mbounce, mlen,
apptag, >reftag);
} else {
+nvme_dif_restore_reftag(ns, mbounce, mlen, iocb->reftag);
status = nvme_dif_check(ns, iocb->bounce, len, mbounce, mlen,
prinfow, iocb->slba, apptag, appmask,
>reftag);
diff --git a/hw/nvme/dif.c b/hw/nvme/dif.c
index 0f65687396..f29c5893e2 100644
--- a/hw/nvme/dif.c
+++ b/hw/nvme/dif.c
@@ -385,6 +385,71 @@ uint16_t nvme_dif_check(NvmeNamespace *ns, uint8_t *buf,
size_t len,
return NVME_SUCCESS;
}
+static void nvme_dif_restore_reftag_crc16(NvmeNamespace *ns, uint8_t *mbuf,
+ size_t mlen, uint64_t reftag,
+ int16_t pil)
+{
+uint8_t *mbufp, *end = mbuf + mlen;
+
+for (mbufp = mbuf; mbufp < end; mbufp += ns->lbaf.ms) {
+NvmeDifTuple *dif = (NvmeDifTuple *)(mbufp + pil);
+
+if (!nvme_dif_is_disabled_crc16(ns, dif)) {
+dif->g16.reftag = cpu_to_be32(reftag++);
+}
+
+}
+
+return;
+}
+
+static void nvme_dif_restore_reftag_crc64(NvmeNamespace *ns, uint8_t *mbuf,
+ size_t mlen, uint64_t reftag,
+ int16_t pil)
+{
+uint8_t *mbufp, *end = mbuf + mlen;
+
+for (mbufp = mbuf; mbufp < end; mbufp += ns->lbaf.ms) {
+NvmeDifTuple *dif = (NvmeDifTuple *)(mbufp + pil);
+
+if (!nvme_dif_is_disabled_crc64(ns, dif)) {
+dif->g64.sr[0] = reftag >> 40;
+dif->g64.sr[1] = reftag >> 32;
+dif->g64.sr[2] = reftag >> 24;
+dif->g64.sr[3] = reftag >> 16;
+dif->g64.sr[4] = reftag >> 8;
+dif->g64.sr[5] = reftag;
+
+reftag++;
+}
+
+}
+
+return;
+}
+
+void nvme_dif_restore_reftag(NvmeNamespace *ns, uint8_t *mbuf,
+ size_t mlen, uint64_t reftag)
+{
+int16_t pil = 0;
+
+if (!(ns->id_ns.dps & NVME_ID_NS_DPS_FIRST_EIGHT)) {
+pil = ns->lbaf.ms - nvme_pi_tuple_size(ns);
+}
+
+switch (ns->pif) {
+case NVME_PI_GUARD_16:
+nvme_dif_restore_reftag_crc16(ns, mbuf, mlen, reftag, pil);
+return;
+case NVME_PI_GUARD_64:
+nvme_dif_restore_reftag_crc64(ns, mbuf, mlen, reftag, pil);
+return;
+default:
+abort();
+}
+
+}
+
uint16_t nvme_dif_mangle_mdata(NvmeNamespace *ns, uint8_t *mbuf, size_t mlen,
uint64_t slba)
{
diff --git a/hw/nvme/dif.h b/hw/nvme/dif.h
index fe1e5828d7..3203837658 100644
--- a/hw/nvme/dif.h
+++ b/hw/nvme/dif.h
@@ -186,6 +186,8 @@ uint16_t nvme_dif_check(NvmeNamespace *ns, uint8_t *buf,
size_t len,
uint8_t *mbuf, size_t mlen, uint8_t prinfo,
uint64_t slba, uint16_t apptag,
uint16_t appmask, uint64_t *reftag);
+void nvme_dif_restore_reftag(NvmeNamespace *ns, uint8_t *mbuf,
+ size_t mlen, uint64_t reftag);
bool nvme_dif_is_disabled(NvmeNamespace *ns, NvmeDifTuple *dif);
uint16_t nvme_dif_rw(NvmeCtrl *n, NvmeRequest *req);
--
2.35.1
[PATCH 1/2] hw/nvme: refactor check of disabled dif
Move to a separate function code determining whether protection checking
in end-to-end data protection enabled namespace should be done.
Currently this code is used only in nvme_dif_prchk_crc16 and
nvme_dif_prchk_crc64 functions.
Signed-off-by: Dmitry Tikhov
---
hw/nvme/dif.c| 121 ---
hw/nvme/dif.h| 1 +
hw/nvme/trace-events | 4 +-
3 files changed, 83 insertions(+), 43 deletions(-)
diff --git a/hw/nvme/dif.c b/hw/nvme/dif.c
index 63c44c86ab..0f65687396 100644
--- a/hw/nvme/dif.c
+++ b/hw/nvme/dif.c
@@ -60,6 +60,75 @@ static uint64_t crc64_nvme(uint64_t crc, const unsigned char
*buffer,
return crc ^ (uint64_t)~0;
}
+static bool nvme_dif_is_disabled_crc16(NvmeNamespace *ns, NvmeDifTuple *dif)
+{
+switch (NVME_ID_NS_DPS_TYPE(ns->id_ns.dps)) {
+case NVME_ID_NS_DPS_TYPE_3:
+if (be32_to_cpu(dif->g16.reftag) != 0x) {
+break;
+}
+
+/* fallthrough */
+case NVME_ID_NS_DPS_TYPE_1:
+case NVME_ID_NS_DPS_TYPE_2:
+if (be16_to_cpu(dif->g16.apptag) != 0x) {
+break;
+}
+
+trace_pci_nvme_dif_is_disabled_crc16(be16_to_cpu(dif->g16.apptag),
+ be32_to_cpu(dif->g16.reftag));
+
+return true;
+}
+
+return false;
+}
+
+static bool nvme_dif_is_disabled_crc64(NvmeNamespace *ns, NvmeDifTuple *dif)
+{
+uint64_t r = 0;
+
+r |= (uint64_t)dif->g64.sr[0] << 40;
+r |= (uint64_t)dif->g64.sr[1] << 32;
+r |= (uint64_t)dif->g64.sr[2] << 24;
+r |= (uint64_t)dif->g64.sr[3] << 16;
+r |= (uint64_t)dif->g64.sr[4] << 8;
+r |= (uint64_t)dif->g64.sr[5];
+
+switch (NVME_ID_NS_DPS_TYPE(ns->id_ns.dps)) {
+case NVME_ID_NS_DPS_TYPE_3:
+if (r != 0x) {
+break;
+}
+
+/* fallthrough */
+case NVME_ID_NS_DPS_TYPE_1:
+case NVME_ID_NS_DPS_TYPE_2:
+if (be16_to_cpu(dif->g64.apptag) != 0x) {
+break;
+}
+
+trace_pci_nvme_dif_is_disabled_crc64(be16_to_cpu(dif->g16.apptag),
+ r);
+
+return true;
+}
+
+return false;
+}
+
+bool nvme_dif_is_disabled(NvmeNamespace *ns, NvmeDifTuple *dif)
+{
+switch (ns->pif) {
+case NVME_PI_GUARD_16:
+return nvme_dif_is_disabled_crc16(ns, dif);
+case NVME_PI_GUARD_64:
+return nvme_dif_is_disabled_crc64(ns, dif);
+default:
+abort();
+}
+}
+
static void nvme_dif_pract_generate_dif_crc16(NvmeNamespace *ns, uint8_t *buf,
size_t len, uint8_t *mbuf,
size_t mlen, uint16_t apptag,
@@ -155,22 +224,7 @@ static uint16_t nvme_dif_prchk_crc16(NvmeNamespace *ns,
NvmeDifTuple *dif,
uint8_t prinfo, uint16_t apptag,
uint16_t appmask, uint64_t reftag)
{
-switch (NVME_ID_NS_DPS_TYPE(ns->id_ns.dps)) {
-case NVME_ID_NS_DPS_TYPE_3:
-if (be32_to_cpu(dif->g16.reftag) != 0x) {
-break;
-}
-
-/* fallthrough */
-case NVME_ID_NS_DPS_TYPE_1:
-case NVME_ID_NS_DPS_TYPE_2:
-if (be16_to_cpu(dif->g16.apptag) != 0x) {
-break;
-}
-
-trace_pci_nvme_dif_prchk_disabled_crc16(be16_to_cpu(dif->g16.apptag),
-be32_to_cpu(dif->g16.reftag));
-
+if (nvme_dif_is_disabled_crc16(ns, dif)) {
return NVME_SUCCESS;
}
@@ -214,31 +268,7 @@ static uint16_t nvme_dif_prchk_crc64(NvmeNamespace *ns,
NvmeDifTuple *dif,
uint8_t prinfo, uint16_t apptag,
uint16_t appmask, uint64_t reftag)
{
-uint64_t r = 0;
-
-r |= (uint64_t)dif->g64.sr[0] << 40;
-r |= (uint64_t)dif->g64.sr[1] << 32;
-r |= (uint64_t)dif->g64.sr[2] << 24;
-r |= (uint64_t)dif->g64.sr[3] << 16;
-r |= (uint64_t)dif->g64.sr[4] << 8;
-r |= (uint64_t)dif->g64.sr[5];
-
-switch (NVME_ID_NS_DPS_TYPE(ns->id_ns.dps)) {
-case NVME_ID_NS_DPS_TYPE_3:
-if (r != 0x) {
-break;
-}
-
-/* fallthrough */
-case NVME_ID_NS_DPS_TYPE_1:
-case NVME_ID_NS_DPS_TYPE_2:
-if (be16_to_cpu(dif->g64.apptag) != 0x) {
-break;
-}
-
-trace_pci_nvme_dif_prchk_disabled_crc64(be16_to_cpu(dif->g16.apptag),
-r);
-
+if (nvme_dif_is_disabled_crc64(ns, dif)) {
return NVME_SUCCESS;
}
@@ -266,6 +296,15 @@ static uint16_t nvme_dif_prchk_crc64(NvmeNamespace *ns,
NvmeDifTuple *dif,
}
if (prinfo & NVME_PRINFO_PRCHK_REF) {
+uint64_t r = 0;
+
+r |= (uint64_t)dif->g64.sr[0] << 40;
+r |= (uint64_t)dif->g64.sr[1] << 32;
+r |= (uint64_t)dif->g64.sr[2] << 24;
+r |=
Re: [PATCH] Only advertise aio=io_uring if support is actually available
On Tue, Apr 19, 2022 at 07:19:31PM +0200, Dirk Müller wrote:
> This allows $qemu --help runtime configure checks for detecting
> the host support.
Note: detecting features by parsing --help output is something that
is explicitly a non-goal for QEMU. The only supported interface for
detecting features is QMP. We can't actively prevent people writing
code that parses --help, but if such parsing breaks on arrival of
new QEMU releases that is not considered a bug on the QEMU side.
That all said, the patch itself is OK, because for human targetted
interactive usage, it is desirable for --help to be representative
of what's available.
IOW, I'm just complaining about the commit message justification
here & warning against writing tools to use --help :-)
> Signed-off-by: Dirk Müller
> ---
> block/file-posix.c | 4
> qemu-nbd.c | 4
> qemu-options.hx| 6 +-
> 3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 39a3d6dbe6..aec4763862 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -544,7 +544,11 @@ static QemuOptsList raw_runtime_opts = {
> {
> .name = "aio",
> .type = QEMU_OPT_STRING,
> +#ifdef CONFIG_LINUX_IO_URING
> .help = "host AIO implementation (threads, native, io_uring)",
> +#else
> +.help = "host AIO implementation (threads, native)",
> +#endif
If we're going to conditionalize this, then we really ought to be
address it fully, because 'native' is also platform specific.
IOW, we would end up needing something more like this:
.help = "host AIO implementation (threads"
#if defined(WIN32) || defined(CONFIG_LINUX_AIO)
", native"
#endif
#if defined(CONFIG_LINUX_IO_URING)
", io_uring"
#else
"),"
admittedly pretty ugly
> },
> {
> .name = "aio-max-batch",
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 713e7557a9..4634a0fc42 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -147,7 +147,11 @@ static void usage(const char *name)
> " --cache=MODE set cache mode used to access the disk image,
> the\n"
> "valid options are: 'none', 'writeback'
> (default),\n"
> "'writethrough', 'directsync' and 'unsafe'\n"
> +#ifdef CONFIG_LINUX_IO_URING
> " --aio=MODEset AIO mode (native, io_uring or threads)\n"
> +#else
> +" --aio=MODEset AIO mode (native or threads)\n"
> +#endif
> " --discard=MODEset discard mode (ignore, unmap)\n"
> " --detect-zeroes=MODE set detect-zeroes mode (off, on, unmap)\n"
> " --image-opts treat FILE as a full set of image options\n"
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 34e9b32a5c..973125cfca 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1338,7 +1338,11 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
> "
> [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
> " [,snapshot=on|off][,rerror=ignore|stop|report]\n"
> " [,werror=ignore|stop|report|enospc][,id=name]\n"
> -" [,aio=threads|native|io_uring]\n"
> +" [,aio=threads|native"
> +#if defined(CONFIG_LINUX_IO_URING)
> +"|io_uring"
> +#endif
> +"]\n"
> " [,readonly=on|off][,copy-on-read=on|off]\n"
> " [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
> " [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
> --
> 2.35.3
>
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 1/2] python/machine.py: upgrade vm.command() method
On Tue, Apr 19, 2022 at 01:08:06PM -0400, John Snow wrote: > On Tue, Apr 19, 2022, 12:42 PM Daniel P. Berrangé > wrote: > > > On Fri, Apr 08, 2022 at 08:02:13PM +0300, Vladimir Sementsov-Ogievskiy > > wrote: > > > The method is not popular, we prefer use vm.qmp() and then check > > > success by hand.. But that's not optimal. To simplify movement to > > > vm.command() support same interface improvements like in vm.qmp() and > > > rename to shorter vm.cmd(). > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > --- > > > python/qemu/machine/machine.py | 16 --- > > > tests/qemu-iotests/256 | 34 > > > tests/qemu-iotests/257 | 36 +- > > > 3 files changed, 48 insertions(+), 38 deletions(-) > > > > > > diff --git a/python/qemu/machine/machine.py > > b/python/qemu/machine/machine.py > > > index 07ac5a710b..a3fb840b93 100644 > > > --- a/python/qemu/machine/machine.py > > > +++ b/python/qemu/machine/machine.py > > > @@ -648,14 +648,24 @@ def qmp(self, cmd: str, > > > self._quit_issued = True > > > return ret > > > > > > -def command(self, cmd: str, > > > -conv_keys: bool = True, > > > -**args: Any) -> QMPReturnValue: > > > +def cmd(self, cmd: str, > > > > FYI, the original 'command' name matches semantics of 'command' > > in the QEMUMonitorProtocol class. The QEMUMonitorProtocol > > class has a 'cmd' method that matches semantics of 'qmp' in > > QEMUMachine. > > > > I think it is desirable to have consistency between naming in > > the two classes. > > > > Broadly agree. > > > > The current use of both 'cmd' and 'command' in QEMUMonitorProtocol > > is horrible naming though. How is anyone supposed to remember which > > name raises the exception and which doesn't ? Urgh. > > > > Also agree. > > > > I agree with your desire to simplify things, but if anything I would > > suggest we change both QEMUMonitorProtocol and QEMUMachine to have a > > convention more like python's subprocess module: > > > > - 'cmd' runs without error checking, just returning the > >reply data as is > > > > - 'check_cmd' calls 'cmd' but raises an exception on error. > > > > Not sure I'm on board here, though. > > For what it's worth, in async qmp I added a single method "execute()", > mimicking the name of the field used over the wire. It uses semantics like > command() here, in that it raises an exception on error and returns only > the response data and not the entire QMP response. > > I'm in favor of, broadly, an approach wherein the default behavior raises > an exception and the caller must opt-in to squelch it; either via > try-except or a check=False argument. It should be a conscious decision. I'd be happy with a single 'cmd' method with 'check=False' to turn off exceptions on demand. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PATCH] hw/nvme: add new command abort case
NVMe command set specification for end-to-end data protection formatted
namespace states:
o If the Reference Tag Check bit of the PRCHK field is set to ‘1’ and
the namespace is formatted for Type 3 protection, then the
controller:
▪ should not compare the protection Information Reference Tag
field to the computed reference tag; and
▪ may ignore the ILBRT and EILBRT fields. If a command is
aborted as a result of the Reference Tag Check bit of the
PRCHK field being set to ‘1’, then that command should be
aborted with a status code of Invalid Protection Information,
but may be aborted with a status code of Invalid Field in
Command.
Currently qemu compares reftag in the nvme_dif_prchk function whenever
Reference Tag Check bit is set in the command. For type 3 namespaces
however, caller of nvme_dif_prchk - nvme_dif_check does not increment
reftag for each subsequent logical block. That way commands incorporating
more than one logical block for type 3 formatted namespaces with reftag
check bit set, always fail with End-to-end Reference Tag Check Error.
Comply with spec by handling case of set Reference Tag Check
bit in the type 3 formatted namespace.
Signed-off-by: Dmitry Tikhov
---
hw/nvme/dif.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/nvme/dif.c b/hw/nvme/dif.c
index 62d885f83e..63c44c86ab 100644
--- a/hw/nvme/dif.c
+++ b/hw/nvme/dif.c
@@ -26,6 +26,11 @@ uint16_t nvme_check_prinfo(NvmeNamespace *ns, uint8_t
prinfo, uint64_t slba,
return NVME_INVALID_PROT_INFO | NVME_DNR;
}
+if ((NVME_ID_NS_DPS_TYPE(ns->id_ns.dps) == NVME_ID_NS_DPS_TYPE_3) &&
+(prinfo & NVME_PRINFO_PRCHK_REF)) {
+return NVME_INVALID_PROT_INFO;
+}
+
return NVME_SUCCESS;
}
--
2.35.1
Re: [PATCH 4/5] hw/nvme: do not auto-generate uuid
On Apr 20 08:53, Christoph Hellwig wrote: > On Wed, Apr 20, 2022 at 07:51:32AM +0200, Klaus Jensen wrote: > > > So unlike the EUI, UUIDs are designed to be autogenerated even if the > > > current algorithm is completely broken. We'd just need to persist them. > > > Note that NVMe at least in theory requires providing at least on of > > > the unique identifiers, and the UUID is the only one designed to be > > > autogenerated in a distributed fashion. > > > > I understand, but it boils down to the fact that we do not have a > > general method of storing "metadata" like this persistently. > > > > But maybe it is time that we come up with something to do this. > > If we can't make the persistent uniqueue identifiers persistent and > unique, we should not provide them. While NVMe does require a > namespace to report at least one of the three identifies, the failure > mode for now having one is much more graceful than providing one that > is not unique or not persistent. Alright. I think we can do that. We can revert the eui64 defaulting as well. Thanks for your reviews/comments Christoph. signature.asc Description: PGP signature
Re: [PATCH 4/5] hw/nvme: do not auto-generate uuid
On Wed, Apr 20, 2022 at 07:51:32AM +0200, Klaus Jensen wrote: > > So unlike the EUI, UUIDs are designed to be autogenerated even if the > > current algorithm is completely broken. We'd just need to persist them. > > Note that NVMe at least in theory requires providing at least on of > > the unique identifiers, and the UUID is the only one designed to be > > autogenerated in a distributed fashion. > > I understand, but it boils down to the fact that we do not have a > general method of storing "metadata" like this persistently. > > But maybe it is time that we come up with something to do this. If we can't make the persistent uniqueue identifiers persistent and unique, we should not provide them. While NVMe does require a namespace to report at least one of the three identifies, the failure mode for now having one is much more graceful than providing one that is not unique or not persistent.
Re: [PATCH 2/5] hw/nvme: always set eui64
On Apr 20 07:48, Klaus Jensen wrote: > On Apr 20 07:30, Christoph Hellwig wrote: > > Also EUI64 values are based on a OUI, while NVME_EUI64_DEFAULT seems > > to have the OUI values cleared to all zero as far as I can tell. > > > > It really should be a u8 array, yes, but won't the integer approach > work? The "template" is byte swapped to big endian, or am I off here? > Nevermind. I see what you mean, I'll fix it up. signature.asc Description: PGP signature
