Re: [PULL 00/11] capstone + disassembler patch queue
On 22/09/20 19:47, Richard Henderson wrote:
> The following changes since commit 834b9273d5cdab68180dc8c84d641aaa4344b057:
>
> Merge remote-tracking branch
> 'remotes/vivier2/tags/trivial-branch-for-5.2-pull-request' into staging
> (2020-09-22 15:42:23 +0100)
>
> are available in the Git repository at:
>
> https://github.com/rth7680/qemu.git tags/pull-cap-20200922
>
> for you to fetch changes up to fcfea6ced053045beb1dc8d22bdeaacc9c03d0b9:
>
> disas/capstone: Add skipdata hook for s390x (2020-09-22 08:59:28 -0700)
>
>
> Update capstone submodule from v3.0.5 to v5 ("next").
> Convert submodule build to meson.
> Enable capstone disassembly for s390x.
> Code cleanups in disas.c
>
>
> Richard Henderson (11):
> capstone: Convert Makefile bits to meson bits
> capstone: Update to upstream "next" branch
> capstone: Require version 4.0 from a system library
> disas: Move host asm annotations to tb_gen_code
> disas: Clean up CPUDebug initialization
> disas: Use qemu/bswap.h for bfd endian loads
> disas: Cleanup plugin_disas
> disas: Configure capstone for aarch64 host without libvixl
> disas: Split out capstone code to disas/capstone.c
> disas: Enable capstone disassembly for s390x
> disas/capstone: Add skipdata hook for s390x
>
> configure | 64 +
> Makefile | 16 --
> meson.build | 124 +++-
> include/disas/dis-asm.h | 104 +++
> include/disas/disas.h | 2 +-
> include/exec/log.h| 4 +-
> accel/tcg/translate-all.c | 24 +-
> disas.c | 707
> +++---
> disas/capstone.c | 326 +
> target/s390x/cpu.c| 4 +
> tcg/tcg.c | 4 +-
> capstone | 2 +-
> disas/meson.build | 1 +
> meson_options.txt | 4 +
> 14 files changed, 681 insertions(+), 705 deletions(-)
> create mode 100644 disas/capstone.c
>
I will pull this as well into my branch (as a kind of topic branch) to
avoid further conflicts.
Paolo
Re: [PATCH v8 3/7] usb/hcd-xhci: Split pci wrapper for xhci base model
Hi,
> Can you also provide any steps to test vmstate migration ?
Helper script below, run it with something like:
$script -m 4G -vga std \
-cdrom Fedora-Workstation-Live-x86_64-32-1.6.iso \
-device qemu-xhci -device usb-tablet
cut here ==
#!/bin/bash
# most recent release
src="/usr/local/bin/qemu-system-x86_64"
# master branch / devel branch
dst="/home/kraxel/projects/qemu/build/default/x86_64-softmmu/qemu-system-x86_64"
# time to wait before migration
sec=60
# vmstate storage
tmp="$(mktemp ${TMPDIR-/var/tmp}/vmstate-XX)"
trap "rm -f $tmp" EXIT
# figure machine type to use
machine=$($src -M help | awk '/default/ { print $1 }')
machine="${machine},vmport=off"
machine="${machine},accel=kvm"
echo "#"
echo "# vmsave (after $sec seconds)"
echo "#"
(
sleep $sec
echo "migrate_set_speed 100M"
echo "migrate exec:cat>$tmp"
echo "quit"
) |\
$src -nodefaults \
-monitor stdio \
-M $machine \
"$@"
echo "#"
echo "# vmload"
echo "#"
$dst -nodefaults \
-monitor stdio \
-incoming "exec:cat $tmp" \
-M $machine \
"$@"
Re: [PATCH v2 1/3] hw/smbios: support loading OEM strings values from a file
Hi Daniel,
On 09/23/20 12:41, Daniel P. Berrangé wrote:
> Some applications want to pass quite large values for the OEM strings
> entries. Rather than having huge strings on the command line, it would
> be better to load them from a file, as supported with -fw_cfg.
>
> This introduces the "path" parameter allowing for:
>
> $ echo -n "thisthing" > mydata.txt
> $ qemu-system-x86_64 \
> -smbios type=11,value=something \
> -smbios type=11,path=mydata.txt \
> -smbios type=11,value=somemore \
> ...other args...
>
> Now in the guest
>
> $ dmidecode -t 11
> Getting SMBIOS data from sysfs.
> SMBIOS 2.8 present.
>
> Handle 0x0E00, DMI type 11, 5 bytes
> OEM Strings
> String 1: something
> String 2: thisthing
> String 3: somemore
>
> Reviewed-by: Philippe Mathieu-Daudé
> Signed-off-by: Daniel P. Berrangé
> ---
> hw/smbios/smbios.c | 72 +-
> 1 file changed, 59 insertions(+), 13 deletions(-)
>
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 7cc950b41c..8450fad285 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -110,7 +110,7 @@ static struct {
>
> static struct {
> size_t nvalues;
> -const char **values;
> +char **values;
> } type11;
>
> static struct {
> @@ -314,6 +314,11 @@ static const QemuOptDesc qemu_smbios_type11_opts[] = {
> .type = QEMU_OPT_STRING,
> .help = "OEM string data",
> },
> +{
> +.name = "path",
> +.type = QEMU_OPT_STRING,
> +.help = "OEM string data from file",
> +},
> };
>
> static const QemuOptDesc qemu_smbios_type17_opts[] = {
> @@ -641,6 +646,8 @@ static void smbios_build_type_11_table(void)
>
> for (i = 0; i < type11.nvalues; i++) {
> SMBIOS_TABLE_SET_STR_LIST(11, type11.values[i]);
> +g_free(type11.values[i]);
> +type11.values[i] = NULL;
> }
>
> SMBIOS_BUILD_TABLE_POST;
> @@ -940,9 +947,8 @@ static void save_opt(const char **dest, QemuOpts *opts,
> const char *name)
>
>
> struct opt_list {
> -const char *name;
> size_t *ndest;
> -const char ***dest;
> +char ***dest;
> };
>
> static int save_opt_one(void *opaque,
> @@ -951,23 +957,61 @@ static int save_opt_one(void *opaque,
> {
> struct opt_list *opt = opaque;
>
> -if (!g_str_equal(name, opt->name)) {
> -return 0;
> +if (g_str_equal(name, "path")) {
> +g_autoptr(GByteArray) data = g_byte_array_new();
> +g_autofree char *buf = g_new(char, 4096);
> +ssize_t ret;
> +int fd = qemu_open(value, O_RDONLY);
This line now fails to compile, due to commit c490af57cb45 ("util:
introduce qemu_open and qemu_create with error reporting", 2020-09-16).
... I guess I could test the patch with qemu_open_old(), but that
wouldn't allow for a valid Tested-by.
Thanks,
Laszlo
> +if (fd < 0) {
> +error_setg(errp, "Unable to open %s: %s", value,
> strerror(errno));
> +return -1;
> +}
> +
> +while (1) {
> +ret = read(fd, buf, 4096);
> +if (ret == 0) {
> +break;
> +}
> +if (ret < 0) {
> +error_setg(errp, "Unable to read from %s: %s",
> + value, strerror(errno));
> +return -1;
> +}
> +if (memchr(buf, '\0', ret)) {
> +error_setg(errp, "NUL in OEM strings value in %s", value);
> +return -1;
> +}
> +g_byte_array_append(data, (guint8 *)buf, ret);
> +}
> +
> +close(fd);
> +
> +*opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1);
> +(*opt->dest)[*opt->ndest] = (char *)g_byte_array_free(data, FALSE);
> +(*opt->ndest)++;
> +data = NULL;
> + } else if (g_str_equal(name, "value")) {
> +*opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1);
> +(*opt->dest)[*opt->ndest] = g_strdup(value);
> +(*opt->ndest)++;
> +} else if (!g_str_equal(name, "type")) {
> +error_setg(errp, "Unexpected option %s", name);
> +return -1;
> }
>
> -*opt->dest = g_renew(const char *, *opt->dest, (*opt->ndest) + 1);
> -(*opt->dest)[*opt->ndest] = value;
> -(*opt->ndest)++;
> return 0;
> }
>
> -static void save_opt_list(size_t *ndest, const char ***dest,
> - QemuOpts *opts, const char *name)
> +static bool save_opt_list(size_t *ndest, char ***dest, QemuOpts *opts,
> + Error **errp)
> {
> struct opt_list opt = {
> -name, ndest, dest,
> +ndest, dest,
> };
> -qemu_opt_foreach(opts, save_opt_one, , NULL);
> +if (!qemu_opt_foreach(opts, save_opt_one, , errp)) {
> +return false;
> +}
> +return true;
> }
>
> void smbios_entry_add(QemuOpts *opts, Error **errp)
> @@ -1149,7 +1193,9 @@ void
Re: [PATCH] iotests: Remove 030 from the auto group
On 23/09/2020 20.18, Alberto Garcia wrote: > On Fri 04 Sep 2020 10:25:13 AM CEST, Kevin Wolf wrote: >>> Test 030 is still occasionally failing in the CI ... so for the >>> time being, let's disable it in the "auto" group. We can add it >>> back once it got more stable. >>> >>> Signed-off-by: Thomas Huth >> >> I would rather just disable this one test function as 030 is a pretty >> important one that tends to catch bugs. >> >>> I just saw the problem here: >>> https://cirrus-ci.com/task/5449330930745344?command=main#L6482 >>> and Peter hit it a couple of weeks ago: >>> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00136.html >> >> I wonder how this can still happen. The test should have more than >> enough time to complete now. Except if the throttling doesn't work as >> expected. >> >> I can't seem to reproduce this even if I add rather long delays. After >> 40 seconds, all jobs have moved either by 512k (which is STREAM_CHUNK) >> or not at all. > > I also don't understand how this can fail... I assume the test is not > running for that long in the cases when it fails, right? Hard to say ... the problem only occurs occasionally, and I've never seen it happen "live", only in the CI logs after the job has failed. I guess you'd have to print timestamps in the code and then submit a lot of jobs to the CI systems that are sensitive to this problem (e.g. Cirrus and Travis) to find out... Thomas
RE: [RFC PATCH v3 30/34] Hexagon (target/hexagon) TCG for instructions with multiple definitions
> > On 8/31/20 4:10 PM, Taylor Simpson wrote:
> > >
> > >
> > >> -Original Message-
> > >> From: Richard Henderson
> > >> Sent: Monday, August 31, 2020 1:20 PM
> > >> To: Taylor Simpson ; qemu-devel@nongnu.org
> > >> Cc: phi...@redhat.com; laur...@vivier.eu; riku.voi...@iki.fi;
> > >> aleksandar.m.m...@gmail.com; a...@rev.ng
> > >> Subject: Re: [RFC PATCH v3 30/34] Hexagon (target/hexagon) TCG for
> > >> instructions with multiple definitions
> > >>
> > Ho hum. Maybe I'm trying to overthink this too much before tackling the
> > ultimate goal of full parsing of the SHORTCODE.
> > Perhaps the only thing for the short term is to have the generator grep
> > genptr.c for "#define fGEN", to choose between the two alternatives:
> inline
> > generation or out-of-line helper generation.
>
> That's certainly doable. It will also be good to implement some of your other
> ideas
> - Have the generator expand the DECL/READ/WRITE/FREE macros will make
> the generated code more readable and we can specialize them for
> predicated vs non-predicated instructions which will make translation faster.
> - Generate the entire generate_ function instead of just the body will
> make the generated code more readable.
I've made these changes to the generator. I hope you like the results. As an
example, here is what we generate for the add instruction
DEF_TCG_FUNC(A2_add,
static void generate_A2_add(
CPUHexagonState *env,
DisasContext *ctx,
insn_t *insn,
packet_t *pkt)
{
TCGv RdV = tcg_temp_local_new();
const int RdN = insn->regno[0];
TCGv RsV = hex_gpr[insn->regno[1]];
TCGv RtV = hex_gpr[insn->regno[2]];
gen_helper_A2_add(RdV, cpu_env, RsV, RtV);
gen_log_reg_write(RdN, RdV);
ctx_log_reg_write(ctx, RdN);
tcg_temp_free(RdV);
})
And here is how the generated file gets used in genptr.c
#define DEF_TCG_FUNC(TAG, GENFN) \
GENFN
#include "tcg_funcs_generated.h"
#undef DEF_TCG_FUNC
/*
* Not all opcodes have generate_ functions, so initialize
* the table from the tcg_funcs_generated.h file.
*/
const semantic_insn_t opcode_genptr[XX_LAST_OPCODE] = {
#define DEF_TCG_FUNC(TAG, GENFN) \
[TAG] = generate_##TAG,
#include "tcg_funcs_generated.h"
#undef DEF_TCG_FUNC
};
I've also addressed several of the items from Richard's review, so I'll
resubmit the series once I figure out how to get "make check-tcg" working under
meson.
Thanks,
Taylor
RE: [RFC PATCH v3 34/34] Hexagon build infrastructure
> -Original Message- > From: Richard Henderson > Sent: Friday, August 28, 2020 9:20 PM > To: Taylor Simpson ; qemu-devel@nongnu.org > Cc: phi...@redhat.com; laur...@vivier.eu; riku.voi...@iki.fi; > aleksandar.m.m...@gmail.com; a...@rev.ng > Subject: Re: [RFC PATCH v3 34/34] Hexagon build infrastructure > > This will have to be updated for the meson conversion. > > I don't understand it all myself, and all of those generated files will need > special attention. > I've made the changes for meson, including converting target/hexagon/Makefile.objs to target/hexagon/meson.build, and I can build qemu-hexagon with mkdir build cd build configure --target-list=hexagon-linux-user make However, when I run "make check-tcg", nothing actually happens. BUILD TCG tests for hexagon-linux-user Generating qemu-version.h with a meson_exe.py custom command RUN TCG tests for hexagon-linux-user What am I missing? Has some other command replaced "make check-tcg"? Thanks, Taylor
Re: [PATCH 3/4] net/colo-compare.c: Add secondary old packet detection
On 9/23/20 2:47 PM, Zhang, Chen wrote:
-Original Message-
From: Li Zhijian
Sent: Tuesday, September 22, 2020 2:47 PM
To: Zhang, Chen ; Jason Wang
; qemu-dev
Cc: Zhang Chen
Subject: Re: [PATCH 3/4] net/colo-compare.c: Add secondary old packet
detection
On 9/18/20 5:22 PM, Zhang Chen wrote:
From: Zhang Chen
Detect queued secondary packet to sync VM state in time.
Signed-off-by: Zhang Chen
---
net/colo-compare.c | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c index
3b72309d08..f7271b976f 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -641,19 +641,26 @@ void colo_compare_unregister_notifier(Notifier
*notify)
static int colo_old_packet_check_one_conn(Connection *conn,
CompareState *s)
{
-GList *result = NULL;
-
-result = g_queue_find_custom(>primary_list,
- >compare_timeout,
- (GCompareFunc)colo_old_packet_check_one);
+if (!g_queue_is_empty(>primary_list)) {
Looks we don't need to check is_empty
Re-checked glib code, it just checked the queue rather than inside content.
Maybe check empty before that will benefit performance.
Yeah, you are right
Reviewed-by: Li Zhijian
Thank
Thanks
Zhang Chen
+if (g_queue_find_custom(>primary_list,
+>compare_timeout,
+(GCompareFunc)colo_old_packet_check_one))
+goto out;
+}
-if (result) {
-/* Do checkpoint will flush old packet */
-colo_compare_inconsistency_notify(s);
-return 0;
+if (!g_queue_is_empty(>secondary_list)) {
Ditto
Thanks
+if (g_queue_find_custom(>secondary_list,
+>compare_timeout,
+(GCompareFunc)colo_old_packet_check_one))
+goto out;
}
return 1;
+
+out:
+/* Do checkpoint will flush old packet */
+colo_compare_inconsistency_notify(s);
+return 0;
}
/*
[Bug 1896317] Re: ioapic: UndefinedBehaviorSanitizer starting qemu-system-i386
I cannot reproduce locally with 053a4177817... What could I have missed? It's kind of odd - For i386, ioapic_as should be set in ../softmmu/vl.c:4355 in pc_memory_init(). The failure triggered at qemu_init softmmu/vl.c:4458:5, which is later. However I don't see any place that ioapic_as can be cleared, yet. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1896317 Title: ioapic: UndefinedBehaviorSanitizer starting qemu-system-i386 Status in QEMU: New Bug description: As of commit 053a4177817: $ ./configure --enable-sanitizers --disable-kvm $ make qemu-system-i386 $ ./build/i386-softmmu/qemu-system-i386 include/exec/memory.h:688:12: runtime error: member access within null pointer of type 'AddressSpace' (aka 'struct AddressSpace') SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior include/exec/memory.h:688:12 in AddressSanitizer:DEADLYSIGNAL = ==249513==ERROR: AddressSanitizer: SEGV on unknown address 0x0020 (pc 0x55955d7f8c4f bp 0x7fff10f3cff0 sp 0x7fff10f3cf20 T0) ==249513==The signal is caused by a READ memory access. ==249513==Hint: address points to the zero page. #0 0x55955d7f8c4f in address_space_to_flatview include/exec/memory.h:688:12 #1 0x55955d8003d2 in address_space_translate include/exec/memory.h:2286:31 #2 0x55955d8315f3 in address_space_stl_internal memory_ldst.c.inc:312:10 #3 0x55955d831cd1 in address_space_stl_le memory_ldst.c.inc:353:5 #4 0x55955d7ef2e1 in stl_le_phys include/exec/memory_ldst_phys.h.inc:103:5 #5 0x55955d7ed299 in ioapic_service hw/intc/ioapic.c:138:17 #6 0x55955d7f0e30 in ioapic_set_irq hw/intc/ioapic.c:186:17 #7 0x55955e34b825 in qemu_set_irq hw/core/irq.c:45:5 #8 0x55955d0409e6 in gsi_handler hw/i386/x86.c:583:5 #9 0x55955e34b825 in qemu_set_irq hw/core/irq.c:45:5 #10 0x55955ca539c9 in hpet_handle_legacy_irq hw/timer/hpet.c:724:13 #11 0x55955e34b825 in qemu_set_irq hw/core/irq.c:45:5 #12 0x55955ce7a695 in pit_irq_timer_update hw/timer/i8254.c:264:5 #13 0x55955ce7a1d8 in pit_irq_control hw/timer/i8254.c:306:9 #14 0x55955e34b825 in qemu_set_irq hw/core/irq.c:45:5 #15 0x55955ca52276 in hpet_reset hw/timer/hpet.c:707:5 #16 0x55955e342e91 in device_transitional_reset hw/core/qdev.c:1114:9 #17 0x55955e345cfc in resettable_phase_hold hw/core/resettable.c:182:13 #18 0x55955e31c1e5 in bus_reset_child_foreach hw/core/bus.c:94:9 #19 0x55955e348a58 in resettable_child_foreach hw/core/resettable.c:96:9 #20 0x55955e34596f in resettable_phase_hold hw/core/resettable.c:173:5 #21 0x55955e344a72 in resettable_assert_reset hw/core/resettable.c:60:5 #22 0x55955e344919 in resettable_reset hw/core/resettable.c:45:5 #23 0x55955e3473e9 in resettable_cold_reset_fn hw/core/resettable.c:269:5 #24 0x55955e344898 in qemu_devices_reset hw/core/reset.c:69:9 #25 0x55955d05c5b0 in pc_machine_reset hw/i386/pc.c:1632:5 #26 0x55955d55ab84 in qemu_system_reset softmmu/vl.c:1403:9 #27 0x55955d56816d in qemu_init softmmu/vl.c:4458:5 #28 0x55955bc13609 in main softmmu/main.c:49:5 #29 0x7f3baad20041 in __libc_start_main (/lib64/libc.so.6+0x27041) #30 0x55955bb398ed in _start (build-sanitizer/qemu-system-i386+0x1b3d8ed) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV include/exec/memory.h:688:12 in address_space_to_flatview Comment and stl_le_phys() added in commit cb135f59b80: /* No matter whether IR is enabled, we translate * the IOAPIC message into a MSI one, and its * address space will decide whether we need a * translation. */ stl_le_phys(ioapic_as, info.addr, info.data); To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1896317/+subscriptions
Re: [PATCH v2 25/38] qapi/gen.py: add type hint annotations
On Wed, Sep 23, 2020 at 08:29:17PM -0400, John Snow wrote:
> On 9/23/20 7:51 PM, Cleber Rosa wrote:
> > On Tue, Sep 22, 2020 at 05:00:48PM -0400, John Snow wrote:
> > > Annotations do not change runtime behavior.
> > > This commit *only* adds annotations.
> > >
> > > Signed-off-by: John Snow
> > > ---
> > > scripts/qapi/gen.py | 102 +++-
> > > 1 file changed, 53 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> > > index cb2b2655c3..df8cf8271c 100644
> > > --- a/scripts/qapi/gen.py
> > > +++ b/scripts/qapi/gen.py
> > > @@ -17,7 +17,7 @@
> > > import errno
> > > import os
> > > import re
> > > -from typing import Optional
> > > +from typing import Dict, Generator, List, Optional, Tuple
> > > from .common import (
> > > c_fname,
> > > @@ -32,31 +32,31 @@
> > > QAPISchemaObjectType,
> > > QAPISchemaVisitor,
> > > )
> > > +from .source import QAPISourceInfo
> > > class QAPIGen:
> > > -
> > > -def __init__(self, fname):
> > > +def __init__(self, fname: Optional[str]):
> > > self.fname = fname
> > > self._preamble = ''
> > > self._body = ''
> > > -def preamble_add(self, text):
> > > +def preamble_add(self, text: str) -> None:
> > > self._preamble += text
> > > -def add(self, text):
> > > +def add(self, text: str) -> None:
> > > self._body += text
> > > -def get_content(self):
> > > +def get_content(self) -> str:
> > > return self._top() + self._preamble + self._body +
> > > self._bottom()
> > > -def _top(self):
> > > +def _top(self) -> str:
> > > return ''
> > > -def _bottom(self):
> > > +def _bottom(self) -> str:
> > > return ''
> > > -def write(self, output_dir):
> > > +def write(self, output_dir: str) -> None:
> > > # Include paths starting with ../ are used to reuse modules of
> > > the main
> > > # schema in specialised schemas. Don't overwrite the files that
> > > are
> > > # already generated for the main schema.
> > > @@ -81,7 +81,7 @@ def write(self, output_dir):
> > > f.close()
> > > -def _wrap_ifcond(ifcond, before, after):
> > > +def _wrap_ifcond(ifcond: List[str], before: str, after: str) -> str:
> > > if before == after:
> > > return after # suppress empty #if ... #endif
> > > @@ -121,40 +121,38 @@ def build_params(arg_type:
> > > Optional[QAPISchemaObjectType],
> > > class QAPIGenCCode(QAPIGen):
> > > -
> > > -def __init__(self, fname):
> > > +def __init__(self, fname: Optional[str]):
> > > super().__init__(fname)
> > > -self._start_if = None
> > > +self._start_if: Optional[Tuple[List[str], str, str]] = None
> > > -def start_if(self, ifcond):
> > > +def start_if(self, ifcond: List[str]) -> None:
> > > assert self._start_if is None
> > > self._start_if = (ifcond, self._body, self._preamble)
> > > -def end_if(self):
> > > +def end_if(self) -> None:
> > > assert self._start_if
> > > self._wrap_ifcond()
> > > self._start_if = None
> > > -def _wrap_ifcond(self):
> > > +def _wrap_ifcond(self) -> None:
> > > self._body = _wrap_ifcond(self._start_if[0],
> > > self._start_if[1], self._body)
> > > self._preamble = _wrap_ifcond(self._start_if[0],
> > > self._start_if[2], self._preamble)
> > > -def get_content(self):
> > > +def get_content(self) -> str:
> > > assert self._start_if is None
> > > return super().get_content()
> > > class QAPIGenC(QAPIGenCCode):
> > > -
> > > -def __init__(self, fname, blurb, pydoc):
> > > +def __init__(self, fname: str, blurb: str, pydoc: str):
> > > super().__init__(fname)
> > > self._blurb = blurb
> > > self._copyright = '\n * '.join(re.findall(r'^Copyright .*',
> > > pydoc,
> > > re.MULTILINE))
> > > -def _top(self):
> > > +def _top(self) -> str:
> > > return mcgen('''
> > > /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> > > @@ -170,7 +168,7 @@ def _top(self):
> > > ''',
> > >blurb=self._blurb, copyright=self._copyright)
> > > -def _bottom(self):
> > > +def _bottom(self) -> str:
> > > return mcgen('''
> > > /* Dummy declaration to prevent empty .o file */
> > > @@ -180,16 +178,16 @@ def _bottom(self):
> > > class QAPIGenH(QAPIGenC):
> > > -
> > > -def _top(self):
> > > +def _top(self) -> str:
> > > return super()._top() + guardstart(self.fname)
> > > -def _bottom(self):
> > > +def _bottom(self) -> str:
> > > return guardend(self.fname)
> > > @contextmanager
> > > -def ifcontext(ifcond, *args):
> > > +def ifcontext(ifcond:
Re: [PATCH v3 51/81] target/arm: Pass separate addend to {U, S}DOT helpers
On 2020/9/23 22:46, Richard Henderson wrote: On 9/23/20 3:01 AM, LIU Zhiwei wrote: On 2020/9/19 2:37, Richard Henderson wrote: For SVE, we potentially have a 4th argument coming from the movprfx instruction. Currently we do not optimize movprfx, so the problem is not visible. Hi Richard, I am a little confused. If it is not immediately preceded by a MOVPRFX instruction, the addend will still be used. Is it right? If movprfx is not used, then the addend register will be the same as the destination register. Get it. Could you see again the definition of HELPER(gvec_udot_idx_h) or the HELPER(gvec_sdot_idx_h)? I think it is wrong there, it code sequence is like this: d0 = a[i+0] //dot calculation d[i+0] += d0 Because when addend is the destination register, it has no reason to add destination register twice. Best Regards, Zhiwei r~
[PATCH 7/8] softfloat: Use x86_64 assembly for {add,sub}{192,256}
The compiler cannot chain more than two additions together.
Use inline assembly for 3 or 4 additions.
Signed-off-by: Richard Henderson
---
include/fpu/softfloat-macros.h | 18 --
fpu/softfloat.c| 28
2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
index 95d88d05b8..99fa124e56 100644
--- a/include/fpu/softfloat-macros.h
+++ b/include/fpu/softfloat-macros.h
@@ -436,6 +436,13 @@ static inline void
uint64_t *z2Ptr
)
{
+#ifdef __x86_64__
+asm("add %5, %2\n\t"
+"adc %4, %1\n\t"
+"adc %3, %0"
+: "="(*z0Ptr), "="(*z1Ptr), "="(*z2Ptr)
+: "rm"(b0), "rm"(b1), "rm"(b2), "0"(a0), "1"(a1), "2"(a2));
+#else
uint64_t z0, z1, z2;
int8_t carry0, carry1;
@@ -450,7 +457,7 @@ static inline void
*z2Ptr = z2;
*z1Ptr = z1;
*z0Ptr = z0;
-
+#endif
}
/*
@@ -494,6 +501,13 @@ static inline void
uint64_t *z2Ptr
)
{
+#ifdef __x86_64__
+asm("sub %5, %2\n\t"
+"sbb %4, %1\n\t"
+"sbb %3, %0"
+: "="(*z0Ptr), "="(*z1Ptr), "="(*z2Ptr)
+: "rm"(b0), "rm"(b1), "rm"(b2), "0"(a0), "1"(a1), "2"(a2));
+#else
uint64_t z0, z1, z2;
int8_t borrow0, borrow1;
@@ -508,7 +522,7 @@ static inline void
*z2Ptr = z2;
*z1Ptr = z1;
*z0Ptr = z0;
-
+#endif
}
/*
diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 5b714fbd82..d8e5d90fd7 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -7297,6 +7297,15 @@ static void shift256RightJamming(uint64_t p[4], int
count)
/* R = A - B */
static void sub256(uint64_t r[4], uint64_t a[4], uint64_t b[4])
{
+#if defined(__x86_64__)
+asm("sub %7, %3\n\t"
+"sbb %6, %2\n\t"
+"sbb %5, %1\n\t"
+"sbb %4, %0"
+: "="(r[0]), "="(r[1]), "="(r[2]), "="(r[3])
+: "rme"(b[0]), "rme"(b[1]), "rme"(b[2]), "rme"(b[3]),
+"0"(a[0]), "1"(a[1]), "2"(a[2]), "3"(a[3]));
+#else
bool borrow = false;
for (int i = 3; i >= 0; --i) {
@@ -7308,11 +7317,20 @@ static void sub256(uint64_t r[4], uint64_t a[4],
uint64_t b[4])
r[i] = a[i] - b[i];
}
}
+#endif
}
/* A = -A */
static void neg256(uint64_t a[4])
{
+#if defined(__x86_64__)
+asm("negq %3\n\t"
+"sbb %6, %2\n\t"
+"sbb %5, %1\n\t"
+"sbb %4, %0"
+: "="(a[0]), "="(a[1]), "="(a[2]), "+rm"(a[3])
+: "rme"(a[0]), "rme"(a[1]), "rme"(a[2]), "0"(0), "1"(0), "2"(0));
+#else
a[3] = -a[3];
if (likely(a[3])) {
goto not2;
@@ -7333,11 +7351,20 @@ static void neg256(uint64_t a[4])
a[1] = ~a[1];
not0:
a[0] = ~a[0];
+#endif
}
/* A += B */
static void add256(uint64_t a[4], uint64_t b[4])
{
+#if defined(__x86_64__)
+asm("add %7, %3\n\t"
+"adc %6, %2\n\t"
+"adc %5, %1\n\t"
+"adc %4, %0"
+: "+r"(a[0]), "+r"(a[1]), "+r"(a[2]), "+r"(a[3])
+: "rme"(b[0]), "rme"(b[1]), "rme"(b[2]), "rme"(b[3]));
+#else
bool carry = false;
for (int i = 3; i >= 0; --i) {
@@ -7350,6 +7377,7 @@ static void add256(uint64_t a[4], uint64_t b[4])
}
a[i] = t;
}
+#endif
}
float128 float128_muladd(float128 a_f, float128 b_f, float128 c_f,
--
2.25.1
[PATCH 6/8] softfloat: Implement float128_muladd
Signed-off-by: Richard Henderson
---
include/fpu/softfloat.h | 2 +
fpu/softfloat.c | 356 +++-
tests/fp/fp-test.c | 2 +-
tests/fp/wrap.c.inc | 12 ++
4 files changed, 370 insertions(+), 2 deletions(-)
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index 78ad5ca738..a38433deb4 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -1196,6 +1196,8 @@ float128 float128_sub(float128, float128, float_status
*status);
float128 float128_mul(float128, float128, float_status *status);
float128 float128_div(float128, float128, float_status *status);
float128 float128_rem(float128, float128, float_status *status);
+float128 float128_muladd(float128, float128, float128, int,
+ float_status *status);
float128 float128_sqrt(float128, float_status *status);
FloatRelation float128_compare(float128, float128, float_status *status);
FloatRelation float128_compare_quiet(float128, float128, float_status *status);
diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index e038434a07..5b714fbd82 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -512,11 +512,19 @@ static inline __attribute__((unused)) bool
is_qnan(FloatClass c)
typedef struct {
uint64_t frac;
-int32_t exp;
+int32_t exp;
FloatClass cls;
bool sign;
} FloatParts;
+/* Similar for float128. */
+typedef struct {
+uint64_t frac0, frac1;
+int32_t exp;
+FloatClass cls;
+bool sign;
+} FloatParts128;
+
#define DECOMPOSED_BINARY_POINT(64 - 2)
#define DECOMPOSED_IMPLICIT_BIT(1ull << DECOMPOSED_BINARY_POINT)
#define DECOMPOSED_OVERFLOW_BIT(DECOMPOSED_IMPLICIT_BIT << 1)
@@ -4574,6 +4582,46 @@ static void
}
+/*
+| Returns the parts of floating-point value `a'.
+**/
+
+static void float128_unpack(FloatParts128 *p, float128 a, float_status *status)
+{
+p->sign = extractFloat128Sign(a);
+p->exp = extractFloat128Exp(a);
+p->frac0 = extractFloat128Frac0(a);
+p->frac1 = extractFloat128Frac1(a);
+
+if (p->exp == 0) {
+if ((p->frac0 | p->frac1) == 0) {
+p->cls = float_class_zero;
+} else if (status->flush_inputs_to_zero) {
+float_raise(float_flag_input_denormal, status);
+p->cls = float_class_zero;
+p->frac0 = p->frac1 = 0;
+} else {
+normalizeFloat128Subnormal(p->frac0, p->frac1, >exp,
+ >frac0, >frac1);
+p->exp -= 0x3fff;
+p->cls = float_class_normal;
+}
+} else if (p->exp == 0x7fff) {
+if ((p->frac0 | p->frac1) == 0) {
+p->cls = float_class_inf;
+} else if (float128_is_signaling_nan(a, status)) {
+p->cls = float_class_snan;
+} else {
+p->cls = float_class_qnan;
+}
+} else {
+/* Add the implicit bit. */
+p->frac0 |= UINT64_C(0x0001);
+p->exp -= 0x3fff;
+p->cls = float_class_normal;
+}
+}
+
/*
| Packs the sign `zSign', the exponent `zExp', and the significand formed
| by the concatenation of `zSig0' and `zSig1' into a quadruple-precision
@@ -7205,6 +7253,312 @@ float128 float128_mul(float128 a, float128 b,
float_status *status)
}
+static void shortShift256Left(uint64_t p[4], unsigned count)
+{
+int negcount = -count & 63;
+
+if (count == 0) {
+return;
+}
+g_assert(count < 64);
+p[0] = (p[0] << count) | (p[1] >> negcount);
+p[1] = (p[1] << count) | (p[2] >> negcount);
+p[2] = (p[2] << count) | (p[3] >> negcount);
+p[3] = (p[3] << count);
+}
+
+static void shift256RightJamming(uint64_t p[4], int count)
+{
+uint64_t in = 0;
+
+g_assert(count >= 0);
+
+count = MIN(count, 256);
+for (; count >= 64; count -= 64) {
+in |= p[3];
+p[3] = p[2];
+p[2] = p[1];
+p[1] = p[0];
+p[0] = 0;
+}
+
+if (count) {
+int negcount = -count & 63;
+
+in |= p[3] << negcount;
+p[3] = (p[2] << negcount) | (p[3] >> count);
+p[2] = (p[1] << negcount) | (p[2] >> count);
+p[1] = (p[0] << negcount) | (p[1] >> count);
+p[0] = p[0] >> count;
+}
+p[3] |= (in != 0);
+}
+
+/* R = A - B */
+static void sub256(uint64_t r[4], uint64_t a[4], uint64_t b[4])
+{
+bool borrow = false;
+
+for (int i = 3; i >= 0; --i) {
+if (borrow) {
+borrow = a[i] <= b[i];
+r[i] = a[i] - b[i] - 1;
+} else {
+borrow = a[i] < b[i];
+r[i] = a[i] - b[i];
+}
+}
+}
+
+/* A = -A */
+static void neg256(uint64_t a[4])
+{
+a[3] = -a[3];
+if (likely(a[3])) {
+goto not2;
+}
+a[2] =
[PATCH 8/8] softfloat: Use aarch64 assembly for {add,sub}{192,256}
The compiler cannot chain more than two additions together.
Use inline assembly for 3 or 4 additions.
Signed-off-by: Richard Henderson
---
include/fpu/softfloat-macros.h | 14 ++
fpu/softfloat.c| 25 +
2 files changed, 39 insertions(+)
diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
index 99fa124e56..969a486fd2 100644
--- a/include/fpu/softfloat-macros.h
+++ b/include/fpu/softfloat-macros.h
@@ -442,6 +442,13 @@ static inline void
"adc %3, %0"
: "="(*z0Ptr), "="(*z1Ptr), "="(*z2Ptr)
: "rm"(b0), "rm"(b1), "rm"(b2), "0"(a0), "1"(a1), "2"(a2));
+#elif defined(__aarch64__)
+asm("adds %2, %x5, %x8\n\t"
+"adcs %1, %x4, %x7\n\t"
+"adc %0, %x3, %x6"
+: "="(*z0Ptr), "="(*z1Ptr), "="(*z2Ptr)
+: "rZ"(a0), "rZ"(a1), "rZ"(a2), "rZ"(b0), "rZ"(b1), "rZ"(b2)
+: "cc");
#else
uint64_t z0, z1, z2;
int8_t carry0, carry1;
@@ -507,6 +514,13 @@ static inline void
"sbb %3, %0"
: "="(*z0Ptr), "="(*z1Ptr), "="(*z2Ptr)
: "rm"(b0), "rm"(b1), "rm"(b2), "0"(a0), "1"(a1), "2"(a2));
+#elif defined(__aarch64__)
+asm("subs %2, %x5, %x8\n\t"
+"sbcs %1, %x4, %x7\n\t"
+"sbc %0, %x3, %x6"
+: "="(*z0Ptr), "="(*z1Ptr), "="(*z2Ptr)
+: "rZ"(a0), "rZ"(a1), "rZ"(a2), "rZ"(b0), "rZ"(b1), "rZ"(b2)
+: "cc");
#else
uint64_t z0, z1, z2;
int8_t borrow0, borrow1;
diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index d8e5d90fd7..1601095d60 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -7305,6 +7305,16 @@ static void sub256(uint64_t r[4], uint64_t a[4],
uint64_t b[4])
: "="(r[0]), "="(r[1]), "="(r[2]), "="(r[3])
: "rme"(b[0]), "rme"(b[1]), "rme"(b[2]), "rme"(b[3]),
"0"(a[0]), "1"(a[1]), "2"(a[2]), "3"(a[3]));
+#elif defined(__aarch64__)
+asm("subs %[r3], %x[a3], %x[b3]\n\t"
+"sbcs %[r2], %x[a2], %x[b2]\n\t"
+"sbcs %[r1], %x[a1], %x[b1]\n\t"
+"sbc %[r0], %x[a0], %x[b0]"
+: [r0] "="(r[0]), [r1] "="(r[1]),
+ [r2] "="(r[2]), [r3] "="(r[3])
+: [a0] "rZ"(a[0]), [a1] "rZ"(a[1]), [a2] "rZ"(a[2]), [a3] "rZ"(a[3]),
+ [b0] "rZ"(b[0]), [b1] "rZ"(b[1]), [b2] "rZ"(b[2]), [b3] "rZ"(b[3])
+: "cc");
#else
bool borrow = false;
@@ -7330,6 +7340,13 @@ static void neg256(uint64_t a[4])
"sbb %4, %0"
: "="(a[0]), "="(a[1]), "="(a[2]), "+rm"(a[3])
: "rme"(a[0]), "rme"(a[1]), "rme"(a[2]), "0"(0), "1"(0), "2"(0));
+#elif defined(__aarch64__)
+asm("negs %3, %3\n\t"
+"ngcs %2, %2\n\t"
+"ngcs %1, %1\n\t"
+"ngc %0, %0"
+: "+r"(a[0]), "+r"(a[1]), "+r"(a[2]), "+r"(a[3])
+: : "cc");
#else
a[3] = -a[3];
if (likely(a[3])) {
@@ -7364,6 +7381,14 @@ static void add256(uint64_t a[4], uint64_t b[4])
"adc %4, %0"
: "+r"(a[0]), "+r"(a[1]), "+r"(a[2]), "+r"(a[3])
: "rme"(b[0]), "rme"(b[1]), "rme"(b[2]), "rme"(b[3]));
+#elif defined(__aarch64__)
+asm("adds %3, %3, %x7\n\t"
+"adcs %2, %2, %x6\n\t"
+"adcs %1, %1, %x5\n\t"
+"adc %0, %0, %x4"
+: "+r"(a[0]), "+r"(a[1]), "+r"(a[2]), "+r"(a[3])
+: "rZ"(b[0]), "rZ"(b[1]), "rZ"(b[2]), "rZ"(b[3])
+: "cc");
#else
bool carry = false;
--
2.25.1
[PATCH 2/8] softfloat: Use int128.h for some operations
Use our Int128, which wraps the compiler's __int128_t,
instead of open-coding left shifts and arithmetic.
We'd need to extend Int128 to have unsigned operations
to replace more than these three.
Signed-off-by: Richard Henderson
---
include/fpu/softfloat-macros.h | 39 +-
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
index 57845f8af0..95d88d05b8 100644
--- a/include/fpu/softfloat-macros.h
+++ b/include/fpu/softfloat-macros.h
@@ -84,6 +84,7 @@ this code that are retained.
#include "fpu/softfloat-types.h"
#include "qemu/host-utils.h"
+#include "qemu/int128.h"
/*
| Shifts `a' right by the number of bits given in `count'. If any nonzero
@@ -352,13 +353,11 @@ static inline void shortShift128Left(uint64_t a0,
uint64_t a1, int count,
static inline void shift128Left(uint64_t a0, uint64_t a1, int count,
uint64_t *z0Ptr, uint64_t *z1Ptr)
{
-if (count < 64) {
-*z1Ptr = a1 << count;
-*z0Ptr = count == 0 ? a0 : (a0 << count) | (a1 >> (-count & 63));
-} else {
-*z1Ptr = 0;
-*z0Ptr = a1 << (count - 64);
-}
+Int128 a = int128_make128(a1, a0);
+Int128 z = int128_lshift(a, count);
+
+*z0Ptr = int128_gethi(z);
+*z1Ptr = int128_getlo(z);
}
/*
@@ -405,15 +404,15 @@ static inline void
**/
static inline void
- add128(
- uint64_t a0, uint64_t a1, uint64_t b0, uint64_t b1, uint64_t *z0Ptr,
uint64_t *z1Ptr )
+add128(uint64_t a0, uint64_t a1, uint64_t b0, uint64_t b1,
+ uint64_t *z0Ptr, uint64_t *z1Ptr)
{
-uint64_t z1;
-
-z1 = a1 + b1;
-*z1Ptr = z1;
-*z0Ptr = a0 + b0 + ( z1 < a1 );
+Int128 a = int128_make128(a1, a0);
+Int128 b = int128_make128(b1, b0);
+Int128 z = int128_add(a, b);
+*z0Ptr = int128_gethi(z);
+*z1Ptr = int128_getlo(z);
}
/*
@@ -463,13 +462,15 @@ static inline void
**/
static inline void
- sub128(
- uint64_t a0, uint64_t a1, uint64_t b0, uint64_t b1, uint64_t *z0Ptr,
uint64_t *z1Ptr )
+sub128(uint64_t a0, uint64_t a1, uint64_t b0, uint64_t b1,
+ uint64_t *z0Ptr, uint64_t *z1Ptr)
{
+Int128 a = int128_make128(a1, a0);
+Int128 b = int128_make128(b1, b0);
+Int128 z = int128_sub(a, b);
-*z1Ptr = a1 - b1;
-*z0Ptr = a0 - b0 - ( a1 < b1 );
-
+*z0Ptr = int128_gethi(z);
+*z1Ptr = int128_getlo(z);
}
/*
--
2.25.1
[PATCH 5/8] softfloat: Inline pick_nan_muladd into its caller
Because of FloatParts, there will only ever be one caller.
Inlining allows us to re-use abc_mask for the snan test.
Signed-off-by: Richard Henderson
---
fpu/softfloat.c | 75 +++--
1 file changed, 35 insertions(+), 40 deletions(-)
diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 3e625c47cd..e038434a07 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -929,45 +929,6 @@ static FloatParts pick_nan(FloatParts a, FloatParts b,
float_status *s)
return a;
}
-static FloatParts pick_nan_muladd(FloatParts a, FloatParts b, FloatParts c,
- bool inf_zero, float_status *s)
-{
-int which;
-
-if (is_snan(a.cls) || is_snan(b.cls) || is_snan(c.cls)) {
-s->float_exception_flags |= float_flag_invalid;
-}
-
-which = pickNaNMulAdd(a.cls, b.cls, c.cls, inf_zero, s);
-
-if (s->default_nan_mode) {
-/* Note that this check is after pickNaNMulAdd so that function
- * has an opportunity to set the Invalid flag.
- */
-which = 3;
-}
-
-switch (which) {
-case 0:
-break;
-case 1:
-a = b;
-break;
-case 2:
-a = c;
-break;
-case 3:
-return parts_default_nan(s);
-default:
-g_assert_not_reached();
-}
-
-if (is_snan(a.cls)) {
-return parts_silence_nan(a, s);
-}
-return a;
-}
-
/*
* Returns the result of adding or subtracting the values of the
* floating-point values `a' and `b'. The operation is performed
@@ -1366,7 +1327,41 @@ static FloatParts muladd_floats(FloatParts a, FloatParts
b, FloatParts c,
* off to the target-specific pick-a-NaN routine.
*/
if (unlikely(abc_mask & float_cmask_anynan)) {
-return pick_nan_muladd(a, b, c, inf_zero, s);
+int which;
+
+if (unlikely(abc_mask & float_cmask_snan)) {
+float_raise(float_flag_invalid, s);
+}
+
+which = pickNaNMulAdd(a.cls, b.cls, c.cls, inf_zero, s);
+
+if (s->default_nan_mode) {
+/*
+ * Note that this check is after pickNaNMulAdd so that function
+ * has an opportunity to set the Invalid flag for inf_zero.
+ */
+which = 3;
+}
+
+switch (which) {
+case 0:
+break;
+case 1:
+a = b;
+break;
+case 2:
+a = c;
+break;
+case 3:
+return parts_default_nan(s);
+default:
+g_assert_not_reached();
+}
+
+if (is_snan(a.cls)) {
+return parts_silence_nan(a, s);
+}
+return a;
}
if (unlikely(inf_zero)) {
--
2.25.1
[PATCH 1/8] softfloat: Use mulu64 for mul64To128
Via host-utils.h, we use a host widening multiply for
64-bit hosts, and a common subroutine for 32-bit hosts.
Signed-off-by: Richard Henderson
---
include/fpu/softfloat-macros.h | 24
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/include/fpu/softfloat-macros.h b/include/fpu/softfloat-macros.h
index a35ec2893a..57845f8af0 100644
--- a/include/fpu/softfloat-macros.h
+++ b/include/fpu/softfloat-macros.h
@@ -83,6 +83,7 @@ this code that are retained.
#define FPU_SOFTFLOAT_MACROS_H
#include "fpu/softfloat-types.h"
+#include "qemu/host-utils.h"
/*
| Shifts `a' right by the number of bits given in `count'. If any nonzero
@@ -515,27 +516,10 @@ static inline void
| `z0Ptr' and `z1Ptr'.
**/
-static inline void mul64To128( uint64_t a, uint64_t b, uint64_t *z0Ptr,
uint64_t *z1Ptr )
+static inline void
+mul64To128(uint64_t a, uint64_t b, uint64_t *z0Ptr, uint64_t *z1Ptr)
{
-uint32_t aHigh, aLow, bHigh, bLow;
-uint64_t z0, zMiddleA, zMiddleB, z1;
-
-aLow = a;
-aHigh = a>>32;
-bLow = b;
-bHigh = b>>32;
-z1 = ( (uint64_t) aLow ) * bLow;
-zMiddleA = ( (uint64_t) aLow ) * bHigh;
-zMiddleB = ( (uint64_t) aHigh ) * bLow;
-z0 = ( (uint64_t) aHigh ) * bHigh;
-zMiddleA += zMiddleB;
-z0 += ( ( (uint64_t) ( zMiddleA < zMiddleB ) )<<32 ) + ( zMiddleA>>32 );
-zMiddleA <<= 32;
-z1 += zMiddleA;
-z0 += ( z1 < zMiddleA );
-*z1Ptr = z1;
-*z0Ptr = z0;
-
+mulu64(z1Ptr, z0Ptr, a, b);
}
/*
--
2.25.1
[PATCH 3/8] softfloat: Tidy a * b + inf return
No reason to set values in 'a', when we already
have float_class_inf in 'c', and can flip that sign.
Signed-off-by: Richard Henderson
---
fpu/softfloat.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 67cfa0fd82..9db55d2b11 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -1380,9 +1380,8 @@ static FloatParts muladd_floats(FloatParts a, FloatParts
b, FloatParts c,
s->float_exception_flags |= float_flag_invalid;
return parts_default_nan(s);
} else {
-a.cls = float_class_inf;
-a.sign = c.sign ^ sign_flip;
-return a;
+c.sign ^= sign_flip;
+return c;
}
}
--
2.25.1
[PATCH 0/8] softfloat: Implement float128_muladd
Plus assorted cleanups, passes tests/fp/fp-test.
I will eventually fill in ppc and s390x assembly bits.
r~
Richard Henderson (8):
softfloat: Use mulu64 for mul64To128
softfloat: Use int128.h for some operations
softfloat: Tidy a * b + inf return
softfloat: Add float_cmask and constants
softfloat: Inline pick_nan_muladd into its caller
softfloat: Implement float128_muladd
softfloat: Use x86_64 assembly for {add,sub}{192,256}
softfloat: Use aarch64 assembly for {add,sub}{192,256}
include/fpu/softfloat-macros.h | 95 +++---
include/fpu/softfloat.h| 2 +
fpu/softfloat.c| 520 +
tests/fp/fp-test.c | 2 +-
tests/fp/wrap.c.inc| 12 +
5 files changed, 538 insertions(+), 93 deletions(-)
--
2.25.1
[PATCH 4/8] softfloat: Add float_cmask and constants
Testing more than one class at a time is better done with masks.
This reduces the static branch count.
Signed-off-by: Richard Henderson
---
fpu/softfloat.c | 31 ---
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 9db55d2b11..3e625c47cd 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -469,6 +469,20 @@ typedef enum __attribute__ ((__packed__)) {
float_class_snan,
} FloatClass;
+#define float_cmask(bit) (1u << (bit))
+
+enum {
+float_cmask_zero= float_cmask(float_class_zero),
+float_cmask_normal = float_cmask(float_class_normal),
+float_cmask_inf = float_cmask(float_class_inf),
+float_cmask_qnan= float_cmask(float_class_qnan),
+float_cmask_snan= float_cmask(float_class_snan),
+
+float_cmask_infzero = float_cmask_zero | float_cmask_inf,
+float_cmask_anynan = float_cmask_qnan | float_cmask_snan,
+};
+
+
/* Simple helpers for checking if, or what kind of, NaN we have */
static inline __attribute__((unused)) bool is_nan(FloatClass c)
{
@@ -1335,24 +1349,27 @@ bfloat16 QEMU_FLATTEN bfloat16_mul(bfloat16 a, bfloat16
b, float_status *status)
static FloatParts muladd_floats(FloatParts a, FloatParts b, FloatParts c,
int flags, float_status *s)
{
-bool inf_zero = ((1 << a.cls) | (1 << b.cls)) ==
-((1 << float_class_inf) | (1 << float_class_zero));
-bool p_sign;
+bool inf_zero, p_sign;
bool sign_flip = flags & float_muladd_negate_result;
FloatClass p_class;
uint64_t hi, lo;
int p_exp;
+int ab_mask, abc_mask;
+
+ab_mask = float_cmask(a.cls) | float_cmask(b.cls);
+abc_mask = float_cmask(c.cls) | ab_mask;
+inf_zero = ab_mask == float_cmask_infzero;
/* It is implementation-defined whether the cases of (0,inf,qnan)
* and (inf,0,qnan) raise InvalidOperation or not (and what QNaN
* they return if they do), so we have to hand this information
* off to the target-specific pick-a-NaN routine.
*/
-if (is_nan(a.cls) || is_nan(b.cls) || is_nan(c.cls)) {
+if (unlikely(abc_mask & float_cmask_anynan)) {
return pick_nan_muladd(a, b, c, inf_zero, s);
}
-if (inf_zero) {
+if (unlikely(inf_zero)) {
s->float_exception_flags |= float_flag_invalid;
return parts_default_nan(s);
}
@@ -1367,9 +1384,9 @@ static FloatParts muladd_floats(FloatParts a, FloatParts
b, FloatParts c,
p_sign ^= 1;
}
-if (a.cls == float_class_inf || b.cls == float_class_inf) {
+if (ab_mask & float_cmask_inf) {
p_class = float_class_inf;
-} else if (a.cls == float_class_zero || b.cls == float_class_zero) {
+} else if (ab_mask & float_cmask_zero) {
p_class = float_class_zero;
} else {
p_class = float_class_normal;
--
2.25.1
Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py
On 9/23/20 7:00 PM, Eric Blake wrote:
Tested-by: Eric Blake
There's enough grammar fixes, and the fact that John is working on
python cleanups, to make me wonder if we need a v9, or if I should just
stage it where it is with any other cleanups as followups. But I'm
liking the reduced maintenance burden once it is in, and don't want to
drag it out to the point that it needs more rebasing as other things
land first.
Here's what I've squashed in and temporarily pushed to my tree if you
want to double-check my rebase work:
https://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/master
diff --git a/docs/devel/block-coroutine-wrapper.rst
b/docs/devel/block-coroutine-wrapper.rst
index f7050bbc8fa6..d09fff2cc539 100644
--- a/docs/devel/block-coroutine-wrapper.rst
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -2,43 +2,43 @@
block-coroutine-wrapper
===
-A lot of functions in QEMJ block layer (see ``block/*``) can by called
-only in coroutine context. Such functions are normally marked by
+A lot of functions in QEMU block layer (see ``block/*``) can only be
+called in coroutine context. Such functions are normally marked by the
coroutine_fn specifier. Still, sometimes we need to call them from
-non-coroutine context, for this we need to start a coroutine, run the
+non-coroutine context; for this we need to start a coroutine, run the
needed function from it and wait for coroutine finish in
BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
-void* argument. So for each coroutine_fn function, which needs
+void* argument. So for each coroutine_fn function which needs a
non-coroutine interface, we should define a structure to pack the
parameters, define a separate function to unpack the parameters and
call the original function and finally define a new interface function
with same list of arguments as original one, which will pack the
parameters into a struct, create a coroutine, run it and wait in
-BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand, so
-we have a script to generate them.
+BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand,
+so we have a script to generate them.
Usage
=
-Assume we have defined ``coroutine_fn`` function
+Assume we have defined the ``coroutine_fn`` function
``bdrv_co_foo()`` and need a non-coroutine interface for it,
called ``bdrv_foo()``. In this case the script can help. To
trigger the generation:
-1. You need ``bdrv_foo`` declaration somewhere (for example in
- ``block/coroutines.h`` with ``generated_co_wrapper`` mark,
+1. You need ``bdrv_foo`` declaration somewhere (for example, in
+ ``block/coroutines.h``) with the ``generated_co_wrapper`` mark,
like this:
.. code-block:: c
-int generated_co_wrapper bdrv_foor();
+int generated_co_wrapper bdrv_foo();
2. You need to feed this declaration to block-coroutine-wrapper script.
- For this, add .h (or .c) file with the declaration to
+ For this, add the .h (or .c) file with the declaration to the
``input: files(...)`` list of ``block_gen_c`` target declaration in
``block/meson.build``
-You are done. On build, coroutine wrappers will be generated in
+You are done. During the build, coroutine wrappers will be generated in
``/block/block-gen.c``.
Links
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 04773ce076b3..cb0abe1e6988 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -31,3 +31,4 @@ Contents:
reset
s390-dasd-ipl
clocks
+ block-coroutine-wrapper
diff --git a/scripts/block-coroutine-wrapper.py
b/scripts/block-coroutine-wrapper.py
index d859c07a5f55..8c0a08d9b020 100755
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -2,7 +2,7 @@
"""Generate coroutine wrappers for block subsystem.
The program parses one or several concatenated c files from stdin,
-searches for functions with 'generated_co_wrapper' specifier
+searches for functions with the 'generated_co_wrapper' specifier
and generates corresponding wrappers on stdout.
Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
@@ -39,7 +39,7 @@ def prettify(code: str) -> str:
'BraceWrapping': {'AfterFunction': True},
'BreakBeforeBraces': 'Custom',
'SortIncludes': False,
-'MaxEmptyLinesToKeep': 2
+'MaxEmptyLinesToKeep': 2,
})
p = subprocess.run(['clang-format', f'-style={style}'],
check=True,
encoding='utf-8', input=code,
@@ -168,7 +168,7 @@ int {func.name}({ func.gen_list('{decl}') })
def gen_wrappers_file(input_code: str) -> str:
-res = gen_header()
+res = ''
for func in func_decl_iter(input_code):
res += '\n\n\n'
res += gen_wrapper(func)
@@ -181,6 +181,7 @@ if __name__ == '__main__':
exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
with open(sys.argv[1], 'w') as f_out:
+
Re: [PATCH v2 25/38] qapi/gen.py: add type hint annotations
On 9/23/20 7:51 PM, Cleber Rosa wrote:
On Tue, Sep 22, 2020 at 05:00:48PM -0400, John Snow wrote:
Annotations do not change runtime behavior.
This commit *only* adds annotations.
Signed-off-by: John Snow
---
scripts/qapi/gen.py | 102 +++-
1 file changed, 53 insertions(+), 49 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index cb2b2655c3..df8cf8271c 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -17,7 +17,7 @@
import errno
import os
import re
-from typing import Optional
+from typing import Dict, Generator, List, Optional, Tuple
from .common import (
c_fname,
@@ -32,31 +32,31 @@
QAPISchemaObjectType,
QAPISchemaVisitor,
)
+from .source import QAPISourceInfo
class QAPIGen:
-
-def __init__(self, fname):
+def __init__(self, fname: Optional[str]):
self.fname = fname
self._preamble = ''
self._body = ''
-def preamble_add(self, text):
+def preamble_add(self, text: str) -> None:
self._preamble += text
-def add(self, text):
+def add(self, text: str) -> None:
self._body += text
-def get_content(self):
+def get_content(self) -> str:
return self._top() + self._preamble + self._body + self._bottom()
-def _top(self):
+def _top(self) -> str:
return ''
-def _bottom(self):
+def _bottom(self) -> str:
return ''
-def write(self, output_dir):
+def write(self, output_dir: str) -> None:
# Include paths starting with ../ are used to reuse modules of the
main
# schema in specialised schemas. Don't overwrite the files that are
# already generated for the main schema.
@@ -81,7 +81,7 @@ def write(self, output_dir):
f.close()
-def _wrap_ifcond(ifcond, before, after):
+def _wrap_ifcond(ifcond: List[str], before: str, after: str) -> str:
if before == after:
return after # suppress empty #if ... #endif
@@ -121,40 +121,38 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
class QAPIGenCCode(QAPIGen):
-
-def __init__(self, fname):
+def __init__(self, fname: Optional[str]):
super().__init__(fname)
-self._start_if = None
+self._start_if: Optional[Tuple[List[str], str, str]] = None
-def start_if(self, ifcond):
+def start_if(self, ifcond: List[str]) -> None:
assert self._start_if is None
self._start_if = (ifcond, self._body, self._preamble)
-def end_if(self):
+def end_if(self) -> None:
assert self._start_if
self._wrap_ifcond()
self._start_if = None
-def _wrap_ifcond(self):
+def _wrap_ifcond(self) -> None:
self._body = _wrap_ifcond(self._start_if[0],
self._start_if[1], self._body)
self._preamble = _wrap_ifcond(self._start_if[0],
self._start_if[2], self._preamble)
-def get_content(self):
+def get_content(self) -> str:
assert self._start_if is None
return super().get_content()
class QAPIGenC(QAPIGenCCode):
-
-def __init__(self, fname, blurb, pydoc):
+def __init__(self, fname: str, blurb: str, pydoc: str):
super().__init__(fname)
self._blurb = blurb
self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
re.MULTILINE))
-def _top(self):
+def _top(self) -> str:
return mcgen('''
/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
@@ -170,7 +168,7 @@ def _top(self):
''',
blurb=self._blurb, copyright=self._copyright)
-def _bottom(self):
+def _bottom(self) -> str:
return mcgen('''
/* Dummy declaration to prevent empty .o file */
@@ -180,16 +178,16 @@ def _bottom(self):
class QAPIGenH(QAPIGenC):
-
-def _top(self):
+def _top(self) -> str:
return super()._top() + guardstart(self.fname)
-def _bottom(self):
+def _bottom(self) -> str:
return guardend(self.fname)
@contextmanager
-def ifcontext(ifcond, *args):
+def ifcontext(ifcond: List[str],
+ *args: QAPIGenCCode) -> Generator[None, None, None]:
IIUC, this could simply be "Iterator[None]" instead of
"Generator[None, None, None]".
Anyway,
Reviewed-by: Cleber Rosa
Oh, you're right! Let's do that instead.
Reference:
https://mypy.readthedocs.io/en/stable/kinds_of_types.html#generators
Eduardo, I am making this change and keeping your R-B.
--js
Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py
On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:
We have a very frequent pattern of creating coroutine from function
with several arguments:
+++ b/scripts/block-coroutine-wrapper.py
@@ -0,0 +1,187 @@
+#!/usr/bin/env python3
+"""Generate coroutine wrappers for block subsystem.
Looking at the generated file after patch 5 is applied,...
+
+def gen_header():
+copyright = re.sub('^.*Copyright', 'Copyright', __doc__, flags=re.DOTALL)
+copyright = re.sub('^(?=.)', ' * ', copyright.strip(), flags=re.MULTILINE)
+copyright = re.sub('^$', ' *', copyright, flags=re.MULTILINE)
+return f"""\
This generated comment...
+
+
+def gen_wrappers_file(input_code: str) -> str:
+res = gen_header()
...is getting inserted into the generated file...
+for func in func_decl_iter(input_code):
+res += '\n\n\n'
+res += gen_wrapper(func)
+
+return prettify(res) # prettify to wrap long lines
+
+
+if __name__ == '__main__':
+if len(sys.argv) < 3:
+exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
+
+with open(sys.argv[1], 'w') as f_out:
+for fname in sys.argv[2:]:
+with open(fname) as f_in:
+f_out.write(gen_wrappers_file(f_in.read()))
multiple times. You'll want to hoist the call to gen_header outside the
loop over fname in sys.argv[2:].
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py
On 9/15/20 3:02 PM, Vladimir Sementsov-Ogievskiy wrote:
15.09.2020 19:44, Vladimir Sementsov-Ogievskiy wrote:
We have a very frequent pattern of creating coroutine from function
with several arguments:
- create structure to pack parameters
- create _entry function to call original function taking parameters
from struct
- do different magic to handle completion: set ret to NOT_DONE or
EINPROGRESS or use separate bool field
- fill the struct and create coroutine from _entry function and this
struct as a parameter
- do coroutine enter and BDRV_POLL_WHILE loop
Let's reduce code duplication by generating coroutine wrappers.
This patch adds scripts/block-coroutine-wrapper.py together with some
friends, which will generate functions with declared prototypes marked
by 'generated_co_wrapper' specifier.
4. add header with generated_co_wrapper declaration into
COROUTINE_HEADERS list in Makefile
This phrase is out-of-date. I also see 4 steps here,...
Still, no function is now marked, this work is for the following
commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
docs/devel/block-coroutine-wrapper.rst | 54 +++
block/block-gen.h | 49 +++
include/block/block.h | 10 ++
block/meson.build | 8 ++
scripts/block-coroutine-wrapper.py | 187 +
5 files changed, 308 insertions(+)
create mode 100644 docs/devel/block-coroutine-wrapper.rst
create mode 100644 block/block-gen.h
create mode 100755 scripts/block-coroutine-wrapper.py
Also needed:
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 04773ce076..cb0abe1e69 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -31,3 +31,4 @@ Contents:
reset
s390-dasd-ipl
clocks
+ block-coroutine-wrapper
I've squashed that in.
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -0,0 +1,54 @@
+===
+block-coroutine-wrapper
+===
+
+A lot of functions in QEMJ block layer (see ``block/*``) can by called
QEMU
s/by called only/only be called/
+only in coroutine context. Such functions are normally marked by
by the
+coroutine_fn specifier. Still, sometimes we need to call them from
+non-coroutine context, for this we need to start a coroutine, run the
s/context,/context;/
+needed function from it and wait for coroutine finish in
in a
+BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
+void* argument. So for each coroutine_fn function, which needs
needs a
+non-coroutine interface, we should define a structure to pack the
+parameters, define a separate function to unpack the parameters and
+call the original function and finally define a new interface function
+with same list of arguments as original one, which will pack the
+parameters into a struct, create a coroutine, run it and wait in
+BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand, so
+we have a script to generate them.
+Usage
+=
+
+Assume we have defined ``coroutine_fn`` function
+``bdrv_co_foo()`` and need a non-coroutine interface for it,
+called ``bdrv_foo()``. In this case the script can help. To
+trigger the generation:
+
+1. You need ``bdrv_foo`` declaration somewhere (for example in
+ ``block/coroutines.h`` with ``generated_co_wrapper`` mark,
+ like this:
Missing a closing ).
+
+.. code-block:: c
+
+int generated_co_wrapper bdrv_foor();
s/foor/foo/
+
+2. You need to feed this declaration to block-coroutine-wrapper script.
to the block-
+ For this, add .h (or .c) file with the declaration to
+ ``input: files(...)`` list of ``block_gen_c`` target declaration in
+ ``block/meson.build``
+
+You are done. On build, coroutine wrappers will be generated in
s/On/During the/
+``/block/block-gen.c``.
...but 2 in the .rst. Presumably, the .rst steps belong in the commit
message as well.
+++ b/block/block-gen.h
+++ b/include/block/block.h
@@ -10,6 +10,16 @@
#include "block/blockjob.h"
#include "qemu/hbitmap.h"
+/*
+ * generated_co_wrapper
+ *
+ * Function specifier, which does nothing but marking functions to be
s/marking/mark/
+ * generated by scripts/block-coroutine-wrapper.py
+ *
+ * Read more in docs/devel/block-coroutine-wrapper.rst
+ */
+#define generated_co_wrapper
+
/* block.c */
typedef struct BlockDriver BlockDriver;
typedef struct BdrvChild BdrvChild;
diff --git a/block/meson.build b/block/meson.build
index a3e56b7cd1..88ad73583a 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -107,6 +107,14 @@ module_block_h = custom_target('module_block.h',
command: [module_block_py, '@OUTPUT0@', modsrc])
block_ss.add(module_block_h)
+wrapper_py = find_program('../scripts/block-coroutine-wrapper.py')
+block_gen_c = custom_target('block-gen.c',
+output: 'block-gen.c',
+input:
Re: [PATCH v2 27/38] qapi/gen.py: Remove unused parameter
On Tue, Sep 22, 2020 at 05:00:50PM -0400, John Snow wrote: > module_basename doesn't use the 'what' argument, so remove it. > > Signed-off-by: John Snow Reviewed-by: Cleber Rosa signature.asc Description: PGP signature
Re: [PATCH v2 26/38] qapi/gen.py: Enable checking with mypy
On Tue, Sep 22, 2020 at 05:00:49PM -0400, John Snow wrote: > Signed-off-by: John Snow > --- > scripts/qapi/mypy.ini | 5 - > 1 file changed, 5 deletions(-) > > diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini > index 43c8bd1973..dbfeda748c 100644 > --- a/scripts/qapi/mypy.ini > +++ b/scripts/qapi/mypy.ini > @@ -19,11 +19,6 @@ disallow_untyped_defs = False > disallow_incomplete_defs = False > check_untyped_defs = False > > -[mypy-qapi.gen] > -disallow_untyped_defs = False > -disallow_incomplete_defs = False > -check_untyped_defs = False > - > [mypy-qapi.introspect] > disallow_untyped_defs = False > disallow_incomplete_defs = False > -- > 2.26.2 > Like I mentioned before, I'd squash this together with the actual changes that enable this increased coverage. Anyway, Reviewed-by: Cleber Rosa Tested-by: Cleber Rosa signature.asc Description: PGP signature
Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module
On 9/23/20 7:08 PM, Cleber Rosa wrote:
On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
The edge case is that if the name is '', this expression returns a
string instead of a bool, which violates our declared type.
Signed-off-by: John Snow
---
scripts/qapi/gen.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 9898d513ae..cb2b2655c3 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb,
pydoc):
@staticmethod
def _is_user_module(name):
-return name and not name.startswith('./')
+return name is not None and not name.startswith('./')
Another possibility here that handles the empty string situation and
will never return anything but a bool:
return all([name, not name.startswith('./')])
Either way,
Reviewed-by: Cleber Rosa
I wound up changing this per ehabkost's review.
--js
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
On 9/23/20 6:36 PM, Cleber Rosa wrote:
On Tue, Sep 22, 2020 at 05:00:45PM -0400, John Snow wrote:
Annotations do not change runtime behavior.
This commit *only* adds annotations.
Signed-off-by: John Snow
---
scripts/qapi/mypy.ini | 5 -
scripts/qapi/source.py | 31 ++-
2 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 9da1dccef4..43c8bd1973 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -39,11 +39,6 @@ disallow_untyped_defs = False
disallow_incomplete_defs = False
check_untyped_defs = False
-[mypy-qapi.source]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-
This is what I meant in my comment in the previous patch. It looks
like a mix of commit grannurality styles. Not a blocker though.
Yep. Just how the chips fell. Some files were just very quick to cleanup
and I didn't have to refactor them much when I split things out, so the
enablements got rolled in.
I will, once reviews are in (and there is a commitment to merge), try to
squash things where it seems appropriate.
[mypy-qapi.types]
disallow_untyped_defs = False
disallow_incomplete_defs = False
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index e97b9a8e15..1cc6a5b82d 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -11,37 +11,42 @@
import copy
import sys
+from typing import List, Optional, TypeVar
class QAPISchemaPragma:
-def __init__(self):
+def __init__(self) -> None:
I don't follow the reason for typing this...
# Are documentation comments required?
self.doc_required = False
# Whitelist of commands allowed to return a non-dictionary
-self.returns_whitelist = []
+self.returns_whitelist: List[str] = []
# Whitelist of entities allowed to violate case conventions
-self.name_case_whitelist = []
+self.name_case_whitelist: List[str] = []
class QAPISourceInfo:
-def __init__(self, fname, line, parent):
+T = TypeVar('T', bound='QAPISourceInfo')
+
+def __init__(self: T, fname: str, line: int, parent: Optional[T]):
And not this... to tune my review approach, should I assume that this
series intends to add complete type hints or not?
This is a mypy quirk you've discovered that I've simply forgotten about.
When __init__ has *no* arguments, you need to annotate its return to
explain to mypy that you have fully typed that method. It's a sentinel
that says "Please type check this class!"
When __init__ has some arguments, you only need to type those arguments
and not the return type. The sentinel is not needed.
__init__ *never* returns anything, so I opted to omit this useless
annotation whenever it was possible to do so.
- Cleber.
Re: [PATCH v2 25/38] qapi/gen.py: add type hint annotations
On Tue, Sep 22, 2020 at 05:00:48PM -0400, John Snow wrote:
> Annotations do not change runtime behavior.
> This commit *only* adds annotations.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/gen.py | 102 +++-
> 1 file changed, 53 insertions(+), 49 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index cb2b2655c3..df8cf8271c 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -17,7 +17,7 @@
> import errno
> import os
> import re
> -from typing import Optional
> +from typing import Dict, Generator, List, Optional, Tuple
>
> from .common import (
> c_fname,
> @@ -32,31 +32,31 @@
> QAPISchemaObjectType,
> QAPISchemaVisitor,
> )
> +from .source import QAPISourceInfo
>
>
> class QAPIGen:
> -
> -def __init__(self, fname):
> +def __init__(self, fname: Optional[str]):
> self.fname = fname
> self._preamble = ''
> self._body = ''
>
> -def preamble_add(self, text):
> +def preamble_add(self, text: str) -> None:
> self._preamble += text
>
> -def add(self, text):
> +def add(self, text: str) -> None:
> self._body += text
>
> -def get_content(self):
> +def get_content(self) -> str:
> return self._top() + self._preamble + self._body + self._bottom()
>
> -def _top(self):
> +def _top(self) -> str:
> return ''
>
> -def _bottom(self):
> +def _bottom(self) -> str:
> return ''
>
> -def write(self, output_dir):
> +def write(self, output_dir: str) -> None:
> # Include paths starting with ../ are used to reuse modules of the
> main
> # schema in specialised schemas. Don't overwrite the files that are
> # already generated for the main schema.
> @@ -81,7 +81,7 @@ def write(self, output_dir):
> f.close()
>
>
> -def _wrap_ifcond(ifcond, before, after):
> +def _wrap_ifcond(ifcond: List[str], before: str, after: str) -> str:
> if before == after:
> return after # suppress empty #if ... #endif
>
> @@ -121,40 +121,38 @@ def build_params(arg_type:
> Optional[QAPISchemaObjectType],
>
>
> class QAPIGenCCode(QAPIGen):
> -
> -def __init__(self, fname):
> +def __init__(self, fname: Optional[str]):
> super().__init__(fname)
> -self._start_if = None
> +self._start_if: Optional[Tuple[List[str], str, str]] = None
>
> -def start_if(self, ifcond):
> +def start_if(self, ifcond: List[str]) -> None:
> assert self._start_if is None
> self._start_if = (ifcond, self._body, self._preamble)
>
> -def end_if(self):
> +def end_if(self) -> None:
> assert self._start_if
> self._wrap_ifcond()
> self._start_if = None
>
> -def _wrap_ifcond(self):
> +def _wrap_ifcond(self) -> None:
> self._body = _wrap_ifcond(self._start_if[0],
>self._start_if[1], self._body)
> self._preamble = _wrap_ifcond(self._start_if[0],
>self._start_if[2], self._preamble)
>
> -def get_content(self):
> +def get_content(self) -> str:
> assert self._start_if is None
> return super().get_content()
>
>
> class QAPIGenC(QAPIGenCCode):
> -
> -def __init__(self, fname, blurb, pydoc):
> +def __init__(self, fname: str, blurb: str, pydoc: str):
> super().__init__(fname)
> self._blurb = blurb
> self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
>re.MULTILINE))
>
> -def _top(self):
> +def _top(self) -> str:
> return mcgen('''
> /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>
> @@ -170,7 +168,7 @@ def _top(self):
> ''',
> blurb=self._blurb, copyright=self._copyright)
>
> -def _bottom(self):
> +def _bottom(self) -> str:
> return mcgen('''
>
> /* Dummy declaration to prevent empty .o file */
> @@ -180,16 +178,16 @@ def _bottom(self):
>
>
> class QAPIGenH(QAPIGenC):
> -
> -def _top(self):
> +def _top(self) -> str:
> return super()._top() + guardstart(self.fname)
>
> -def _bottom(self):
> +def _bottom(self) -> str:
> return guardend(self.fname)
>
>
> @contextmanager
> -def ifcontext(ifcond, *args):
> +def ifcontext(ifcond: List[str],
> + *args: QAPIGenCCode) -> Generator[None, None, None]:
IIUC, this could simply be "Iterator[None]" instead of
"Generator[None, None, None]".
Anyway,
Reviewed-by: Cleber Rosa
signature.asc
Description: PGP signature
Re: [PATCH v2 21/38] qapi/commands.py: enable checking with mypy
On 9/23/20 6:21 PM, Cleber Rosa wrote: On Tue, Sep 22, 2020 at 05:00:44PM -0400, John Snow wrote: Signed-off-by: John Snow --- scripts/qapi/mypy.ini | 5 - 1 file changed, 5 deletions(-) diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini index b668776d94..9da1dccef4 100644 --- a/scripts/qapi/mypy.ini +++ b/scripts/qapi/mypy.ini @@ -4,11 +4,6 @@ strict_optional = False disallow_untyped_calls = False python_version = 3.6 -[mypy-qapi.commands] -disallow_untyped_defs = False -disallow_incomplete_defs = False -check_untyped_defs = False - [mypy-qapi.doc] disallow_subclassing_any = False disallow_untyped_defs = False -- 2.26.2 IMO, this increase of strictness for "commands" would make more sense to be squashed together with the previous changes on "commands.py". Not only here, but for the other patches for the other modules too. Anyway, Reviewed-by: Cleber Rosa Admittedly, the only reason I *didn't* is because these patches have been reordered a *lot* and in some cases, it helped me to have distinct "This patch is last and enables the checks!" commits. (I am hedging my bets that more re-ordering is in my future.) I will squash the "Enable such-and-such" commits with whatever fixed the last error for final inclusion, but I might keep them separate for now just for my own convenience. Sorry for the volume. --js
Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module
On Wed, Sep 23, 2020 at 07:10:35PM -0400, Cleber Rosa wrote:
> On Wed, Sep 23, 2020 at 02:33:35PM -0400, Eduardo Habkost wrote:
> > On Wed, Sep 23, 2020 at 02:29:28PM -0400, John Snow wrote:
> > > On 9/23/20 11:17 AM, Eduardo Habkost wrote:
> > > > This changes behavior if name=='', and I guess this is OK, but
> > > > I'm not sure. I miss documentation on `visit_module()`,
> > > > `visit_include()`, and `_is_user_module()`. I don't know what
> > > > `name` means here, and what is a "user module".
> > > >
> > >
> > > Good spot, I missed that.
> > >
>
> Nice catch indeed.
>
> > > I can probably do: bool(name and not name.startswith('./'))
> > >
>
> In this case I like my previous suggestion even better. :)
>
> - Cleber.
Never mind me... I noticed that this actually gets called with None.
- Cleber.
signature.asc
Description: PGP signature
Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module
On Wed, Sep 23, 2020 at 07:08:50PM -0400, Cleber Rosa wrote:
> On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
> > The edge case is that if the name is '', this expression returns a
> > string instead of a bool, which violates our declared type.
> >
> > Signed-off-by: John Snow
> > ---
> > scripts/qapi/gen.py | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> > index 9898d513ae..cb2b2655c3 100644
> > --- a/scripts/qapi/gen.py
> > +++ b/scripts/qapi/gen.py
> > @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb,
> > builtin_blurb, pydoc):
> >
> > @staticmethod
> > def _is_user_module(name):
> > -return name and not name.startswith('./')
> > +return name is not None and not name.startswith('./')
>
> Another possibility here that handles the empty string situation and
> will never return anything but a bool:
>
> return all([name, not name.startswith('./')])
>
Never mind me... I noticed that this actually gets called with None.
- Cleber.
signature.asc
Description: PGP signature
Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module
On Wed, Sep 23, 2020 at 02:33:35PM -0400, Eduardo Habkost wrote:
> On Wed, Sep 23, 2020 at 02:29:28PM -0400, John Snow wrote:
> > On 9/23/20 11:17 AM, Eduardo Habkost wrote:
> > > This changes behavior if name=='', and I guess this is OK, but
> > > I'm not sure. I miss documentation on `visit_module()`,
> > > `visit_include()`, and `_is_user_module()`. I don't know what
> > > `name` means here, and what is a "user module".
> > >
> >
> > Good spot, I missed that.
> >
Nice catch indeed.
> > I can probably do: bool(name and not name.startswith('./'))
> >
In this case I like my previous suggestion even better. :)
- Cleber.
signature.asc
Description: PGP signature
Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module
On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
> The edge case is that if the name is '', this expression returns a
> string instead of a bool, which violates our declared type.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/gen.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 9898d513ae..cb2b2655c3 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb,
> builtin_blurb, pydoc):
>
> @staticmethod
> def _is_user_module(name):
> -return name and not name.startswith('./')
> +return name is not None and not name.startswith('./')
Another possibility here that handles the empty string situation and
will never return anything but a bool:
return all([name, not name.startswith('./')])
Either way,
Reviewed-by: Cleber Rosa
signature.asc
Description: PGP signature
Re: [PATCH v2 32/38] qapi/introspect.py: create a typed 'Node' data structure
On 9/23/20 2:41 PM, Eduardo Habkost wrote:
On Tue, Sep 22, 2020 at 05:00:55PM -0400, John Snow wrote:
Replacing the un-typed tuple, add a typed Node that we can add typed
metadata to.
Signed-off-by: John Snow
This is the most complex patch so far, and it's very hard to
understand what it does without type annotations.
Have you consider adding type annotations to the code before
patch 30/38 (even if using `object` in some parts), so we can
make this easier to review?
In case it's useful, below is an attempt to add type annotations
to the old code. It can be applied after patch 29/38. It reuses
portions of patch 33/38.
Signed-off-by: John Snow
Signed-off-by: Eduardo Habkost
---
scripts/qapi/introspect.py | 138 ++---
scripts/qapi/mypy.ini | 5 --
scripts/qapi/schema.py | 2 +-
3 files changed, 100 insertions(+), 45 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index b036fcf9ce7..4eaebdef58b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,6 +10,17 @@ This work is licensed under the terms of the GNU GPL,
version 2.
See the COPYING file in the top-level directory.
"""
+from typing import (
+Dict,
+Generic,
+List,
+NamedTuple,
+Optional,
+Sequence,
+TypeVar,
+Tuple
+)
+
from .common import (
c_name,
gen_endif,
@@ -17,25 +28,48 @@ from .common import (
mcgen,
)
from .gen import QAPISchemaMonolithicCVisitor
-from .schema import (QAPISchemaArrayType, QAPISchemaBuiltinType,
- QAPISchemaType)
-
-
-def _make_tree(obj, ifcond, features, extra=None):
+from .schema import (
+QAPISchema,
+QAPISchemaArrayType,
+QAPISchemaBuiltinType,
+QAPISchemaEntity,
+QAPISchemaEnumMember,
+QAPISchemaFeature,
+QAPISchemaObjectType,
+QAPISchemaObjectTypeMember,
+QAPISchemaType,
+QAPISchemaVariant,
+QAPISchemaVariants,
+)
+from .source import QAPISourceInfo
+
+T = TypeVar('T')
+# this should actually be: Union[str, list, dict, bool, 'AnnotatedNode']
+# but mypy doesn't support recursive types
+TreeNode = object
+TreeDict = Dict[str, TreeNode]
+Extra = Dict[str, object]
+AnnotatedNode = Tuple[T, Extra]
+
+def _make_tree(obj: TreeDict, ifcond: List[str],
Technically untrue! obj may be a SchemaInfo-like dict, or a string.
(And I will tell you why mypy didn't catch this!)
+ features: List[QAPISchemaFeature],
+ extra: Optional[Extra] = None) -> TreeNode:
if extra is None:
extra = {}
if ifcond:
extra['if'] = ifcond
if features:
-obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
+obj['features'] = ([(f.name, {'if': f.ifcond}) for f in features])
if extra:
return (obj, extra)
return obj
-def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
+def _tree_to_qlit(obj: TreeNode,
+ level: int = 0,
+ suppress_first_indent : bool = False) -> str:
-def indent(level):
+def indent(level: int) -> str:
return level * 4 * ' '
if isinstance(obj, tuple):
@@ -85,21 +119,20 @@ def _tree_to_qlit(obj, level=0,
suppress_first_indent=False):
return ret
-def to_c_string(string):
+def to_c_string(string: str) -> str:
return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
-
-def __init__(self, prefix, unmask):
+def __init__(self, prefix: str, unmask: bool):
super().__init__(
prefix, 'qapi-introspect',
' * QAPI/QMP schema introspection', __doc__)
self._unmask = unmask
-self._schema = None
-self._trees = []
-self._used_types = []
-self._name_map = {}
+self._schema: Optional[QAPISchema] = None
+self._trees: List[TreeNode] = []
+self._used_types: List[QAPISchemaType] = []
+self._name_map: Dict[str, str] = {}
self._genc.add(mcgen('''
#include "qemu/osdep.h"
#include "%(prefix)sqapi-introspect.h"
@@ -107,10 +140,10 @@ class
QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
''',
prefix=prefix))
-def visit_begin(self, schema):
+def visit_begin(self, schema: QAPISchema) -> None:
self._schema = schema
-def visit_end(self):
+def visit_end(self) -> None:
# visit the types that are actually used
for typ in self._used_types:
typ.visit(self)
@@ -132,18 +165,18 @@ const QLitObject %(c_name)s = %(c_string)s;
self._used_types = []
self._name_map = {}
-def visit_needed(self, entity):
+def visit_needed(self, entity: QAPISchemaEntity) -> bool:
# Ignore types on first pass; visit_end() will pick up used types
Re: [PATCH v2 23/38] qapi/source.py: delint with pylint
On Tue, Sep 22, 2020 at 05:00:46PM -0400, John Snow wrote: > Shush an error and leave a hint for future cleanups when we're allowed > to use Python 3.7+. > > Signed-off-by: John Snow Reviewed-by: Cleber Rosa Tested-by: Cleber Rosa signature.asc Description: PGP signature
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
On Tue, Sep 22, 2020 at 05:00:45PM -0400, John Snow wrote:
> Annotations do not change runtime behavior.
> This commit *only* adds annotations.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/mypy.ini | 5 -
> scripts/qapi/source.py | 31 ++-
> 2 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> index 9da1dccef4..43c8bd1973 100644
> --- a/scripts/qapi/mypy.ini
> +++ b/scripts/qapi/mypy.ini
> @@ -39,11 +39,6 @@ disallow_untyped_defs = False
> disallow_incomplete_defs = False
> check_untyped_defs = False
>
> -[mypy-qapi.source]
> -disallow_untyped_defs = False
> -disallow_incomplete_defs = False
> -check_untyped_defs = False
> -
This is what I meant in my comment in the previous patch. It looks
like a mix of commit grannurality styles. Not a blocker though.
> [mypy-qapi.types]
> disallow_untyped_defs = False
> disallow_incomplete_defs = False
> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> index e97b9a8e15..1cc6a5b82d 100644
> --- a/scripts/qapi/source.py
> +++ b/scripts/qapi/source.py
> @@ -11,37 +11,42 @@
>
> import copy
> import sys
> +from typing import List, Optional, TypeVar
>
>
> class QAPISchemaPragma:
> -def __init__(self):
> +def __init__(self) -> None:
I don't follow the reason for typing this...
> # Are documentation comments required?
> self.doc_required = False
> # Whitelist of commands allowed to return a non-dictionary
> -self.returns_whitelist = []
> +self.returns_whitelist: List[str] = []
> # Whitelist of entities allowed to violate case conventions
> -self.name_case_whitelist = []
> +self.name_case_whitelist: List[str] = []
>
>
> class QAPISourceInfo:
> -def __init__(self, fname, line, parent):
> +T = TypeVar('T', bound='QAPISourceInfo')
> +
> +def __init__(self: T, fname: str, line: int, parent: Optional[T]):
And not this... to tune my review approach, should I assume that this
series intends to add complete type hints or not?
- Cleber.
signature.asc
Description: PGP signature
Re: [PATCH v4 0/2] qemu/atomic.h: rename atomic_ to qatomic_
Patchew URL: https://patchew.org/QEMU/20200923151901.745277-1-phi...@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20200923151901.745277-1-phi...@redhat.com Subject: [PATCH v4 0/2] qemu/atomic.h: rename atomic_ to qatomic_ === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === From https://github.com/patchew-project/qemu - [tag update] patchew/20200923151901.745277-1-phi...@redhat.com -> patchew/20200923151901.745277-1-phi...@redhat.com Switched to a new branch 'test' 424f21a qemu/atomic.h: rename atomic_ to qatomic_ 90bd4fa qemu/atomic: Update coding style to make checkpatch.pl happier === OUTPUT BEGIN === 1/2 Checking commit 90bd4facd2bb (qemu/atomic: Update coding style to make checkpatch.pl happier) ERROR: Use of volatile is usually wrong, please add a comment #23: FILE: include/qemu/atomic.h:328: +#define atomic_read__nocheck(p) (*(__typeof__(*(p)) volatile *) (p)) ERROR: Use of volatile is usually wrong, please add a comment #24: FILE: include/qemu/atomic.h:329: +#define atomic_set__nocheck(p, i) ((*(__typeof__(*(p)) volatile *) (p)) = (i)) total: 2 errors, 0 warnings, 50 lines checked Patch 1/2 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 2/2 Checking commit 424f21a3295e (qemu/atomic.h: rename atomic_ to qatomic_) ERROR: Macros with multiple statements should be enclosed in a do - while loop #2801: FILE: include/qemu/atomic.h:152: +#define qatomic_rcu_read__nocheck(ptr, valptr) \ +__atomic_load(ptr, valptr, __ATOMIC_RELAXED);\ smp_read_barrier_depends(); ERROR: Use of volatile is usually wrong, please add a comment #2946: FILE: include/qemu/atomic.h:333: +#define qatomic_read__nocheck(p) (*(__typeof__(*(p)) volatile *) (p)) ERROR: Use of volatile is usually wrong, please add a comment #2947: FILE: include/qemu/atomic.h:334: +#define qatomic_set__nocheck(p, i) ((*(__typeof__(*(p)) volatile *) (p)) = (i)) ERROR: memory barrier without comment #3024: FILE: include/qemu/atomic.h:395: +#define qatomic_xchg(ptr, i)(smp_mb(), __sync_lock_test_and_set(ptr, i)) total: 4 errors, 0 warnings, 6269 lines checked Patch 2/2 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20200923151901.745277-1-phi...@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v2 21/38] qapi/commands.py: enable checking with mypy
On Tue, Sep 22, 2020 at 05:00:44PM -0400, John Snow wrote: > Signed-off-by: John Snow > --- > scripts/qapi/mypy.ini | 5 - > 1 file changed, 5 deletions(-) > > diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini > index b668776d94..9da1dccef4 100644 > --- a/scripts/qapi/mypy.ini > +++ b/scripts/qapi/mypy.ini > @@ -4,11 +4,6 @@ strict_optional = False > disallow_untyped_calls = False > python_version = 3.6 > > -[mypy-qapi.commands] > -disallow_untyped_defs = False > -disallow_incomplete_defs = False > -check_untyped_defs = False > - > [mypy-qapi.doc] > disallow_subclassing_any = False > disallow_untyped_defs = False > -- > 2.26.2 > IMO, this increase of strictness for "commands" would make more sense to be squashed together with the previous changes on "commands.py". Not only here, but for the other patches for the other modules too. Anyway, Reviewed-by: Cleber Rosa signature.asc Description: PGP signature
Re: [PATCH v2 20/38] qapi/commands.py: add notational type hints
On Tue, Sep 22, 2020 at 05:00:43PM -0400, John Snow wrote: > Signed-off-by: John Snow Reviewed-by: Cleber Rosa signature.asc Description: PGP signature
Re: [PATCH v2 36/38] qapi/visit.py: assert tag_member contains a QAPISchemaEnumType
On 9/23/20 3:15 PM, Eduardo Habkost wrote: On Tue, Sep 22, 2020 at 05:00:59PM -0400, John Snow wrote: Signed-off-by: John Snow This for making mypy happy, correct? An explanation in the commit message would be nice. Reviewed-by: Eduardo Habkost Yes, it's for mypy -- but it's a runtime visible change. Technically our type system isn't mature enough to express this constraint natively, so it's being carried around as developer knowledge. This formalizes that knowledge, albeit in a very crude way. I've amended the commit msg.
Re: [PATCH v2 35/38] qapi/types.py: remove one-letter variables
On 9/23/20 3:14 PM, Eduardo Habkost wrote: I'm not sure I like this weird expression, but I believe asking for a 120-patch cleanup series to be respun because of a tiny style issue would be counterproductive, so: Reviewed-by: Eduardo Habkost I was trying to reduce the indent level to accommodate the longer names, but python ternaries *are* pretty weird. It'd be nice to enforce always having a variants object instead (even if it's empty!) and then add __bool__ and __iter__ methods to QAPISchemaVariants such that you could always do: "if variants" or "for variant in variants" but we're not there just yet... should I just put it back the way it was, with the deep nesting? --js
Re: [PATCH] docs: Better mention of qemu-img amend limitations
On Wed, Sep 23, 2020 at 11:38 PM Eric Blake wrote: > > Missed during merge resolution of commit bc5ee6da71. > > Signed-off-by: Eric Blake > --- > docs/tools/qemu-img.rst | 4 > 1 file changed, 4 insertions(+) > > diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst > index c35bd6482203..2b5891b54db7 100644 > --- a/docs/tools/qemu-img.rst > +++ b/docs/tools/qemu-img.rst > @@ -265,6 +265,10 @@ Command description: >--force allows some unsafe operations. Currently for -f luks, it allows to >erase the last encryption key, and to overwrite an active encryption key. > > + The set of options that can be amended are dependent on the image > + format, but note that amending the backing chain relationship should > + instead be performed with ``qemu-img rebase``. Because of the backing format? > + > .. option:: bench [-c COUNT] [-d DEPTH] [-f FMT] > [--flush-interval=FLUSH_INTERVAL] [-i AIO] [-n] [--no-drain] [-o OFFSET] > [--pattern=PATTERN] [-q] [-s BUFFER_SIZE] [-S STEP_SIZE] [-t CACHE] [-w] [-U] > FILENAME > >Run a simple sequential I/O benchmark on the specified image. If ``-w`` is > -- > 2.28.0 > >
[Bug 1896096] Re: Git version: Build process is broken in block_curl.c.o
v2 patches appear to work fine in both test scenarios. Thanks again. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1896096 Title: Git version: Build process is broken in block_curl.c.o Status in QEMU: Invalid Bug description: Gcc version: 10.2.0 Glusterfs: 8.1 Libguestfs: 1.42 Configure options used: configure \ --prefix=/usr \ --sysconfdir=/etc \ --localstatedir=/var \ --libexecdir=/usr/lib/qemu \ --extra-ldflags="$LDFLAGS" \ --smbd=/usr/bin/smbd \ --enable-modules \ --enable-sdl \ --disable-werror \ --enable-slirp=system \ --enable-xfsctl \ --audio-drv-list="pa alsa sdl" Error log attached. Here is the beginning: /usr/bin/ld: /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../lib/Scrt1.o: in function `_start': (.text+0x24): undefined reference to `main' /usr/bin/ld: libblock-curl.a(block_curl.c.o): in function `curl_block_init': To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1896096/+subscriptions
Re: [PATCH v2 31/38] qapi/introspect.py: add _gen_features helper
On Wed, Sep 23, 2020 at 05:43:45PM -0400, John Snow wrote: > On 9/23/20 12:35 PM, Eduardo Habkost wrote: > > I believe these two lines above should be removed, as suggested > > in patch 30, but let's ignore that for now. > > > > Yup, headed there. > > > > -if features: > > > -obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in > > > features] > > I can't say I understand completely why moving these two lines > > outside _make_tree() is useful, but if it makes the cleanup work > > you did easier, I trust this is the right thing to do. The > > changes look correct. > > The basic premise is: > > Why pass information you want to add to obj['features'] to a function to > make that assignment, when you could just perform that assignment yourself? > > Otherwise, _make_tree, which accepts any arbitrary object (not just dicts!) > has to interrogate its arguments to make sure you gave it a dict when you > give it a features argument. Oh, I was not aware that obj could be not a dictionary. In this case, it makes lots of sense to move the magic outside the function. > > Type-wise, it's cleaner to perform this transformation when we KNOW we have > an object than it is to defer to a more abstracted function and > assert/downcast back to more specific types. -- Eduardo
Re: [PATCH v2 32/38] qapi/introspect.py: create a typed 'Node' data structure
On 9/23/20 2:41 PM, Eduardo Habkost wrote:
On Tue, Sep 22, 2020 at 05:00:55PM -0400, John Snow wrote:
Replacing the un-typed tuple, add a typed Node that we can add typed
metadata to.
Signed-off-by: John Snow
This is the most complex patch so far, and it's very hard to
understand what it does without type annotations.
Mmm, sorry about that.
Have you consider adding type annotations to the code before
patch 30/38 (even if using `object` in some parts), so we can
make this easier to review?
If you feel like it isn't just noise to learn types we're about to
destroy with something else, I'll try.
In case it's useful, below is an attempt to add type annotations
to the old code. It can be applied after patch 29/38. It reuses
portions of patch 33/38.
OK. End result is I will squash the Extra and Node types together, but I
need to figure out how to make it look nice for the review history.
Gimmie a few to make a mess and I'll try to put it back together afterward.
Signed-off-by: John Snow
Signed-off-by: Eduardo Habkost
---
scripts/qapi/introspect.py | 138 ++---
scripts/qapi/mypy.ini | 5 --
scripts/qapi/schema.py | 2 +-
3 files changed, 100 insertions(+), 45 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index b036fcf9ce7..4eaebdef58b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,6 +10,17 @@ This work is licensed under the terms of the GNU GPL,
version 2.
See the COPYING file in the top-level directory.
"""
+from typing import (
+Dict,
+Generic,
+List,
+NamedTuple,
+Optional,
+Sequence,
+TypeVar,
+Tuple
+)
+
from .common import (
c_name,
gen_endif,
@@ -17,25 +28,48 @@ from .common import (
mcgen,
)
from .gen import QAPISchemaMonolithicCVisitor
-from .schema import (QAPISchemaArrayType, QAPISchemaBuiltinType,
- QAPISchemaType)
-
-
-def _make_tree(obj, ifcond, features, extra=None):
+from .schema import (
+QAPISchema,
+QAPISchemaArrayType,
+QAPISchemaBuiltinType,
+QAPISchemaEntity,
+QAPISchemaEnumMember,
+QAPISchemaFeature,
+QAPISchemaObjectType,
+QAPISchemaObjectTypeMember,
+QAPISchemaType,
+QAPISchemaVariant,
+QAPISchemaVariants,
+)
+from .source import QAPISourceInfo
+
+T = TypeVar('T')
+# this should actually be: Union[str, list, dict, bool, 'AnnotatedNode']
+# but mypy doesn't support recursive types
+TreeNode = object
+TreeDict = Dict[str, TreeNode]
+Extra = Dict[str, object]
+AnnotatedNode = Tuple[T, Extra]
+
+def _make_tree(obj: TreeDict, ifcond: List[str],
+ features: List[QAPISchemaFeature],
+ extra: Optional[Extra] = None) -> TreeNode:
if extra is None:
extra = {}
if ifcond:
extra['if'] = ifcond
if features:
-obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
+obj['features'] = ([(f.name, {'if': f.ifcond}) for f in features])
if extra:
return (obj, extra)
return obj
-def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
+def _tree_to_qlit(obj: TreeNode,
+ level: int = 0,
+ suppress_first_indent : bool = False) -> str:
-def indent(level):
+def indent(level: int) -> str:
return level * 4 * ' '
if isinstance(obj, tuple):
@@ -85,21 +119,20 @@ def _tree_to_qlit(obj, level=0,
suppress_first_indent=False):
return ret
-def to_c_string(string):
+def to_c_string(string: str) -> str:
return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
-
-def __init__(self, prefix, unmask):
+def __init__(self, prefix: str, unmask: bool):
super().__init__(
prefix, 'qapi-introspect',
' * QAPI/QMP schema introspection', __doc__)
self._unmask = unmask
-self._schema = None
-self._trees = []
-self._used_types = []
-self._name_map = {}
+self._schema: Optional[QAPISchema] = None
+self._trees: List[TreeNode] = []
+self._used_types: List[QAPISchemaType] = []
+self._name_map: Dict[str, str] = {}
self._genc.add(mcgen('''
#include "qemu/osdep.h"
#include "%(prefix)sqapi-introspect.h"
@@ -107,10 +140,10 @@ class
QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
''',
prefix=prefix))
-def visit_begin(self, schema):
+def visit_begin(self, schema: QAPISchema) -> None:
self._schema = schema
-def visit_end(self):
+def visit_end(self) -> None:
# visit the types that are actually used
for typ in self._used_types:
typ.visit(self)
@@ -132,18 +165,18 @@ const QLitObject %(c_name)s = %(c_string)s;
self._used_types = []
Re: [PATCH v8 3/7] block: declare some coroutine functions in block/coroutines.h
On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote: We are going to keep coroutine-wrappers code (structure-packing parameters, BDRV_POLL wrapper functions) in separate auto-generated files. So, we'll need a header with declaration of original _co_ functions, for those which are static now. As well, we'll need declarations for wrapper functions. Do these declarations now, as a preparation step. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake --- +++ b/block/coroutines.h +int coroutine_fn bdrv_co_check(BlockDriverState *bs, + BdrvCheckResult *res, BdrvCheckMode fix); +int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp); + +int coroutine_fn +bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov, + bool is_write, BdrvRequestFlags flags); Inconsistent on whether the function name is in column 1 or after the return type. But we aren't consistent elsewhre, so I won't bother changing it. R-b still stands -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 31/38] qapi/introspect.py: add _gen_features helper
On 9/23/20 12:35 PM, Eduardo Habkost wrote: I believe these two lines above should be removed, as suggested in patch 30, but let's ignore that for now. Yup, headed there. -if features: -obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in features] I can't say I understand completely why moving these two lines outside _make_tree() is useful, but if it makes the cleanup work you did easier, I trust this is the right thing to do. The changes look correct. The basic premise is: Why pass information you want to add to obj['features'] to a function to make that assignment, when you could just perform that assignment yourself? Otherwise, _make_tree, which accepts any arbitrary object (not just dicts!) has to interrogate its arguments to make sure you gave it a dict when you give it a features argument. Type-wise, it's cleaner to perform this transformation when we KNOW we have an object than it is to defer to a more abstracted function and assert/downcast back to more specific types.
Re: [PATCH v2 30/38] qapi/introspect.py: Add a typed 'extra' structure
On 9/23/20 12:13 PM, Eduardo Habkost wrote:
On Tue, Sep 22, 2020 at 05:00:53PM -0400, John Snow wrote:
Typing arbitrarily shaped dicts with mypy is difficult prior to Python
3.8; using explicit structures is nicer.
Since we always define an Extra type now, the return type of _make_tree
simplifies and always returns the tuple.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 31 +++
1 file changed, 19 insertions(+), 12 deletions(-)
Here I'm confused by both the original code and the new code.
I will try to review as a refactoring of existing code, but I'll
have suggestions for follow ups:
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index b036fcf9ce..41ca8afc67 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,6 +10,8 @@
See the COPYING file in the top-level directory.
"""
+from typing import (NamedTuple, Optional, Sequence)
+
from .common import (
c_name,
gen_endif,
@@ -21,16 +23,21 @@
QAPISchemaType)
-def _make_tree(obj, ifcond, features, extra=None):
-if extra is None:
-extra = {}
-if ifcond:
-extra['if'] = ifcond
+class Extra(NamedTuple):
+"""
+Extra contains data that isn't intended for output by introspection.
+"""
+comment: Optional[str] = None
+ifcond: Sequence[str] = tuple()
+
+
+def _make_tree(obj, ifcond, features,
+ extra: Optional[Extra] = None):
+comment = extra.comment if extra else None
+extra = Extra(comment, ifcond)
Here we have one big difference: now `extra` is being recreated,
and all fields except `extra.comment` are being ignored. On the
original version, all fields in `extra` were being kept. This
makes the existence of the `extra` argument pointless.
Yup, oops.
If you are going through the trouble of changing the type of the
4rd argument to _make_tree(), this seems more obvious:
Yes, agree. I came up with something similar after talking to you this
morning.
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 41ca8afc672..c62af94c9ad 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -32,8 +32,7 @@ class Extra(NamedTuple):
def _make_tree(obj, ifcond, features,
- extra: Optional[Extra] = None):
-comment = extra.comment if extra else None
+ comment: Optional[str] = None):
extra = Extra(comment, ifcond)
if features:
obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in
features]
@@ -170,16 +169,16 @@ const QLitObject %(c_name)s = %(c_string)s;
return self._name(typ.name)
def _gen_tree(self, name, mtype, obj, ifcond, features):
-extra = None
+comment = None
if mtype not in ('command', 'event', 'builtin', 'array'):
if not self._unmask:
# Output a comment to make it easy to map masked names
# back to the source when reading the generated output.
-extra = Extra(comment=f'"{self._name(name)}" = {name}')
+comment = f'"{self._name(name)}" = {name}'
name = self._name(name)
obj['name'] = name
obj['meta-type'] = mtype
-self._trees.append(_make_tree(obj, ifcond, features, extra))
+self._trees.append(_make_tree(obj, ifcond, features, comment))
def _gen_member(self, member):
obj = {'name': member.name, 'type': self._use_type(member.type)}
I understand you're trying to just make minimal refactoring, and I
don't think this should block your cleanup series. So:
Reviewed-by: Eduardo Habkost
I appreciate the benefit-of-the-doubt, but I think this change is worth
making while we're here.
if features:
-obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
-if extra:
-return (obj, extra)
-return obj
+obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in features]
+return (obj, extra)
def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
@@ -40,8 +47,8 @@ def indent(level):
if isinstance(obj, tuple):
ifobj, extra = obj
-ifcond = extra.get('if')
-comment = extra.get('comment')
+ifcond = extra.ifcond
+comment = extra.comment
ret = ''
if comment:
ret += indent(level) + '/* %s */\n' % comment
@@ -168,7 +175,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
if not self._unmask:
# Output a comment to make it easy to map masked names
# back to the source when reading the generated output.
-extra = {'comment': '"%s" = %s' % (self._name(name), name)}
+extra = Extra(comment=f'"{self._name(name)}" = {name}')
name
Re: [PATCH v2 14/38] qapi/common.py: Convert comments into docstrings, and elaborate
On 9/23/20 3:38 PM, Cleber Rosa wrote:
On Tue, Sep 22, 2020 at 05:00:37PM -0400, John Snow wrote:
As docstrings, they'll show up in documentation and IDE help.
Signed-off-by: John Snow
---
scripts/qapi/common.py | 51 ++
1 file changed, 37 insertions(+), 14 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 0ce4a107e6..730283722a 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -20,10 +20,18 @@
_C_NAME_TRANS = str.maketrans('.-', '__')
-# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
-# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
-# ENUM24_Name -> ENUM24_NAME
def camel_to_upper(value: str) -> str:
+"""
+Converts CamelCase to CAMEL_CASE.
+
+Examples:
+ ENUMName -> ENUM_NAME
+ EnumName1 -> ENUM_NAME1
+ ENUM_NAME -> ENUM_NAME
+ ENUM_NAME1 -> ENUM_NAME1
+ ENUM_Name2 -> ENUM_NAME2
+ ENUM24_Name -> ENUM24_NAME
+"""
c_fun_str = c_name(value, False)
if value.isupper():
return c_fun_str
@@ -45,21 +53,33 @@ def camel_to_upper(value: str) -> str:
def c_enum_const(type_name: str,
const_name: str,
prefix: Optional[str] = None) -> str:
+"""
+Generate a C enumeration constant name.
+
+:param type_name: The name of the enumeration.
+:param const_name: The name of this constant.
+:param prefix: Optional, prefix that overrides the type_name.
+"""
if prefix is not None:
type_name = prefix
return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
-# Map @name to a valid C identifier.
-# If @protect, avoid returning certain ticklish identifiers (like
-# C keywords) by prepending 'q_'.
-#
-# Used for converting 'name' from a 'name':'type' qapi definition
-# into a generated struct member, as well as converting type names
-# into substrings of a generated C function name.
-# '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
-# protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
def c_name(name: str, protect: bool = True) -> str:
+"""
+Map `name` to a valid C identifier.
+
+Used for converting 'name' from a 'name':'type' qapi definition
+into a generated struct member, as well as converting type names
+into substrings of a generated C function name.
+
+'__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
+protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
+
+:param name: The name to map.
+:param protect: If true, avoid returning certain ticklish identifiers
+(like C keywords) by prepending ``q_``.
+"""
# ANSI X3J11/88-090, 3.1.1
c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
'default', 'do', 'double', 'else', 'enum', 'extern',
@@ -134,9 +154,12 @@ def decrease(self, amount: int = 4) -> int:
indent = Indentation()
-# Generate @code with @kwds interpolated.
-# Obey indent, and strip EATSPACE.
def cgen(code: str, **kwds: object) -> str:
+"""
+Generate `code` with `kwds` interpolated.
+
+Obey `indent`, and strip `EATSPACE`.
+"""
This probably won't help on IDEs (never checked any), but sphinx will
let you do:
"""
Generate `code` with `kwds` interpolated.
Obey `indent`, and strip :data:`EATSPACE`.
"""
I'm not sure that a maximum level of docstring "sphinxzation" is the
goal here, though.
Reviewed-by: Cleber Rosa
It isn't yet, but I intend to address that when I remove
missing-docstring from pylint exemptions. Do I need :data: if I set the
default role to 'any'?
I'll probably try to enable sphinx at that time (and put the docs in a
devel/python manual?) and worry about the formatting at that point.
--js
Re: [PATCH] load_elf: Remove unused address variables from callers
On Wed, 23 Sep 2020, BALATON Zoltan wrote:
On Tue, 7 Jul 2020, BALATON Zoltan wrote:
On Tue, 7 Jul 2020, Alistair Francis wrote:
On Sun, Jul 5, 2020 at 10:41 AM BALATON Zoltan wrote:
Several callers of load_elf() pass pointers for lowaddr and highaddr
parameters which are then not used for anything. This may stem from a
misunderstanding that load_elf need a value here but in fact it can
take NULL to ignore these values. Remove such unused variables and
pass NULL instead from callers that don't need these.
Signed-off-by: BALATON Zoltan
Reviewed-by: Alistair Francis
So this got a few review and acked by but since it touches multiple parts
who will actually send pull or merge it? I'd like to make sure this won't
miss the freeze deadline just because everybody thinks this should go in
via some other maintainer. What's the best way for this? Trivial
maintainers or Peter should handle such patches?
Ping? Could someone please queue this patch? It still seems to apply cleanly.
Forgot to include the link to the original patch:
http://patchwork.ozlabs.org/project/qemu-devel/patch/20200705174020.bdd01746...@zero.eik.bme.hu/
Regards,
BALATON Zoltan
---
hw/alpha/dp264.c | 8
hw/arm/armv7m.c| 4 +---
hw/cris/boot.c | 4 ++--
hw/microblaze/boot.c | 4 ++--
hw/mips/fuloong2e.c| 8
hw/moxie/moxiesim.c| 4 ++--
hw/nios2/boot.c| 4 ++--
hw/ppc/mac_newworld.c | 6 ++
hw/ppc/mac_oldworld.c | 6 ++
hw/ppc/ppc440_bamboo.c | 9 +++--
hw/ppc/sam460ex.c | 12 +---
hw/ppc/spapr.c | 11 ---
hw/ppc/virtex_ml507.c | 4 ++--
hw/riscv/boot.c| 8
hw/xtensa/sim.c| 3 +--
hw/xtensa/xtfpga.c | 3 +--
16 files changed, 41 insertions(+), 57 deletions(-)
diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index f7751b18f6..4d24518d1d 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -62,8 +62,8 @@ static void clipper_init(MachineState *machine)
qemu_irq rtc_irq;
long size, i;
char *palcode_filename;
-uint64_t palcode_entry, palcode_low, palcode_high;
-uint64_t kernel_entry, kernel_low, kernel_high;
+uint64_t palcode_entry;
+uint64_t kernel_entry, kernel_low;
unsigned int smp_cpus = machine->smp.cpus;
/* Create up to 4 cpus. */
@@ -113,7 +113,7 @@ static void clipper_init(MachineState *machine)
exit(1);
}
size = load_elf(palcode_filename, NULL, cpu_alpha_superpage_to_phys,
-NULL, _entry, _low, _high,
NULL,
+NULL, _entry, NULL, NULL, NULL,
0, EM_ALPHA, 0, 0);
if (size < 0) {
error_report("could not load palcode '%s'", palcode_filename);
@@ -132,7 +132,7 @@ static void clipper_init(MachineState *machine)
uint64_t param_offset;
size = load_elf(kernel_filename, NULL,
cpu_alpha_superpage_to_phys,
-NULL, _entry, _low, _high,
NULL,
+NULL, _entry, _low, NULL, NULL,
0, EM_ALPHA, 0, 0);
if (size < 0) {
error_report("could not load kernel '%s'", kernel_filename);
diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 3308211e9c..92f859d760 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -309,7 +309,6 @@ void armv7m_load_kernel(ARMCPU *cpu, const char
*kernel_filename, int mem_size)
{
int image_size;
uint64_t entry;
-uint64_t lowaddr;
int big_endian;
AddressSpace *as;
int asidx;
@@ -330,12 +329,11 @@ void armv7m_load_kernel(ARMCPU *cpu, const char
*kernel_filename, int mem_size)
if (kernel_filename) {
image_size = load_elf_as(kernel_filename, NULL, NULL, NULL,
- , , NULL,
+ , NULL, NULL,
NULL, big_endian, EM_ARM, 1, 0, as);
if (image_size < 0) {
image_size = load_image_targphys_as(kernel_filename, 0,
mem_size, as);
-lowaddr = 0;
}
if (image_size < 0) {
error_report("Could not load kernel '%s'", kernel_filename);
diff --git a/hw/cris/boot.c b/hw/cris/boot.c
index b8947bc660..aa8d2756d6 100644
--- a/hw/cris/boot.c
+++ b/hw/cris/boot.c
@@ -67,7 +67,7 @@ static uint64_t translate_kernel_address(void *opaque,
uint64_t addr)
void cris_load_image(CRISCPU *cpu, struct cris_load_info *li)
{
CPUCRISState *env = >env;
-uint64_t entry, high;
+uint64_t entry;
int kcmdline_len;
int image_size;
@@ -76,7 +76,7 @@ void cris_load_image(CRISCPU *cpu, struct
cris_load_info *li)
devboard SDK. */
image_size = load_elf(li->image_filename, NULL,
translate_kernel_address, NULL,
- , NULL, , NULL, 0, EM_CRIS, 0, 0);
+ , NULL, NULL, NULL, 0, EM_CRIS, 0, 0);
li->entry = entry;
Re: [PATCH] load_elf: Remove unused address variables from callers
On Tue, 7 Jul 2020, BALATON Zoltan wrote:
On Tue, 7 Jul 2020, Alistair Francis wrote:
On Sun, Jul 5, 2020 at 10:41 AM BALATON Zoltan wrote:
Several callers of load_elf() pass pointers for lowaddr and highaddr
parameters which are then not used for anything. This may stem from a
misunderstanding that load_elf need a value here but in fact it can
take NULL to ignore these values. Remove such unused variables and
pass NULL instead from callers that don't need these.
Signed-off-by: BALATON Zoltan
Reviewed-by: Alistair Francis
So this got a few review and acked by but since it touches multiple parts who
will actually send pull or merge it? I'd like to make sure this won't miss
the freeze deadline just because everybody thinks this should go in via some
other maintainer. What's the best way for this? Trivial maintainers or Peter
should handle such patches?
Ping? Could someone please queue this patch? It still seems to apply
cleanly.
Regards,
BALATON Zoltan
---
hw/alpha/dp264.c | 8
hw/arm/armv7m.c| 4 +---
hw/cris/boot.c | 4 ++--
hw/microblaze/boot.c | 4 ++--
hw/mips/fuloong2e.c| 8
hw/moxie/moxiesim.c| 4 ++--
hw/nios2/boot.c| 4 ++--
hw/ppc/mac_newworld.c | 6 ++
hw/ppc/mac_oldworld.c | 6 ++
hw/ppc/ppc440_bamboo.c | 9 +++--
hw/ppc/sam460ex.c | 12 +---
hw/ppc/spapr.c | 11 ---
hw/ppc/virtex_ml507.c | 4 ++--
hw/riscv/boot.c| 8
hw/xtensa/sim.c| 3 +--
hw/xtensa/xtfpga.c | 3 +--
16 files changed, 41 insertions(+), 57 deletions(-)
diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index f7751b18f6..4d24518d1d 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -62,8 +62,8 @@ static void clipper_init(MachineState *machine)
qemu_irq rtc_irq;
long size, i;
char *palcode_filename;
-uint64_t palcode_entry, palcode_low, palcode_high;
-uint64_t kernel_entry, kernel_low, kernel_high;
+uint64_t palcode_entry;
+uint64_t kernel_entry, kernel_low;
unsigned int smp_cpus = machine->smp.cpus;
/* Create up to 4 cpus. */
@@ -113,7 +113,7 @@ static void clipper_init(MachineState *machine)
exit(1);
}
size = load_elf(palcode_filename, NULL, cpu_alpha_superpage_to_phys,
-NULL, _entry, _low, _high,
NULL,
+NULL, _entry, NULL, NULL, NULL,
0, EM_ALPHA, 0, 0);
if (size < 0) {
error_report("could not load palcode '%s'", palcode_filename);
@@ -132,7 +132,7 @@ static void clipper_init(MachineState *machine)
uint64_t param_offset;
size = load_elf(kernel_filename, NULL,
cpu_alpha_superpage_to_phys,
-NULL, _entry, _low, _high,
NULL,
+NULL, _entry, _low, NULL, NULL,
0, EM_ALPHA, 0, 0);
if (size < 0) {
error_report("could not load kernel '%s'", kernel_filename);
diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 3308211e9c..92f859d760 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -309,7 +309,6 @@ void armv7m_load_kernel(ARMCPU *cpu, const char
*kernel_filename, int mem_size)
{
int image_size;
uint64_t entry;
-uint64_t lowaddr;
int big_endian;
AddressSpace *as;
int asidx;
@@ -330,12 +329,11 @@ void armv7m_load_kernel(ARMCPU *cpu, const char
*kernel_filename, int mem_size)
if (kernel_filename) {
image_size = load_elf_as(kernel_filename, NULL, NULL, NULL,
- , , NULL,
+ , NULL, NULL,
NULL, big_endian, EM_ARM, 1, 0, as);
if (image_size < 0) {
image_size = load_image_targphys_as(kernel_filename, 0,
mem_size, as);
-lowaddr = 0;
}
if (image_size < 0) {
error_report("Could not load kernel '%s'", kernel_filename);
diff --git a/hw/cris/boot.c b/hw/cris/boot.c
index b8947bc660..aa8d2756d6 100644
--- a/hw/cris/boot.c
+++ b/hw/cris/boot.c
@@ -67,7 +67,7 @@ static uint64_t translate_kernel_address(void *opaque,
uint64_t addr)
void cris_load_image(CRISCPU *cpu, struct cris_load_info *li)
{
CPUCRISState *env = >env;
-uint64_t entry, high;
+uint64_t entry;
int kcmdline_len;
int image_size;
@@ -76,7 +76,7 @@ void cris_load_image(CRISCPU *cpu, struct cris_load_info
*li)
devboard SDK. */
image_size = load_elf(li->image_filename, NULL,
translate_kernel_address, NULL,
- , NULL, , NULL, 0, EM_CRIS, 0, 0);
+ , NULL, NULL, NULL, 0, EM_CRIS, 0, 0);
li->entry = entry;
if (image_size < 0) {
/* Takes a kimage from the axis devboard SDK. */
diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 925e3f7c9d..8ad3c27f2c 100644
---
Re: [PATCH 14/14] qapi/doc.py: enable pylint checks
On Tue, Sep 22, 2020 at 05:18:02PM -0400, John Snow wrote: > Signed-off-by: John Snow Tested-by: Eduardo Habkost Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 13/14] qapi/doc.py: Assert type of object variant
On Tue, Sep 22, 2020 at 05:18:01PM -0400, John Snow wrote: > Objects may have variants, but those variants must themselves be > objects. This is difficult to express with our current type system and > hierarchy, so instead pepper in an assertion. > > Note: These assertions don't appear to be useful yet because schema.py > is not yet typed. Once it is, these assertions will matter. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 12/14] qapi/doc.py: Assert tag member is Enum type
On Tue, Sep 22, 2020 at 05:18:00PM -0400, John Snow wrote: > The type system can't quite express this constraint natively: members > can envelop any type -- but tag_members may only ever envelop an > enumerated type. > > For now, shrug and add an assertion. > > Note: These assertions don't appear to be useful yet because schema.py > is not yet typed. Once it is, these assertions will matter. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 11/14] qapi/doc.py: Don't use private attributes of QAPIGen property
On Tue, Sep 22, 2020 at 05:17:59PM -0400, John Snow wrote: > Use the new __bool__ method to do the same without exposing the private > attribute. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 10/14] qapi/gen.py: Add __bool__ dunder method to QAPIGen
On Tue, Sep 22, 2020 at 05:17:58PM -0400, John Snow wrote: > Falseish when there is no body or preamble; Trueish when there is. > Header and footer are excluded for now, because they are assumed to be > dynamic and always present. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 09/14] qapi/doc.py: Remove one-letter variables
On Tue, Sep 22, 2020 at 05:17:57PM -0400, John Snow wrote: > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 06/14] qapi/doc.py: Add type hint annotations
On Tue, Sep 22, 2020 at 05:17:54PM -0400, John Snow wrote: > Annotations do not change runtime behavior. > This commit *only* adds annotations. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH v4 1/2] qemu/atomic: Update coding style to make checkpatch.pl happier
will be also nice to squash the following on top for a complete clean
checkpatch version, as the original patch introduces at least 1 issue
Carlo
--- >8 ---
Subject: fixup! [PATCH 1/2] qemu/atomic.h: rename atomic_ to qatomic_
fixes:
ERROR: Macros with multiple statements should be enclosed in a do - while loop
+#define qatomic_rcu_read__nocheck(ptr, valptr) \
+__atomic_load(ptr, valptr, __ATOMIC_RELAXED);\
smp_read_barrier_depends();
false positive:
ERROR: memory barrier without comment
+#define qatomic_xchg(ptr, i)(smp_mb(), __sync_lock_test_and_set(ptr, i))
Signed-off-by: Carlo Marcelo Arenas Belón
---
include/qemu/atomic.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 87b85f9f6d..be47e083be 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -149,9 +149,10 @@
#define qatomic_rcu_read__nocheck(ptr, valptr) \
__atomic_load(ptr, valptr, __ATOMIC_CONSUME);
#else
-#define qatomic_rcu_read__nocheck(ptr, valptr) \
+#define qatomic_rcu_read__nocheck(ptr, valptr) do { \
__atomic_load(ptr, valptr, __ATOMIC_RELAXED);\
-smp_read_barrier_depends();
+smp_read_barrier_depends(); \
+} while (0)
#endif
#define qatomic_rcu_read(ptr) \
--
2.28.0.681.g6f77f65b4e
Re: [PATCH 05/14] qapi/doc.py: Assert no suffix given for enum members
On Tue, Sep 22, 2020 at 05:17:53PM -0400, John Snow wrote: > We don't do anything with this argument. If something shows up here, > something wrong has happened. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 04/14] qapi/doc.py: assert correct types in member_func callbacks
On Tue, Sep 22, 2020 at 05:17:52PM -0400, John Snow wrote: > These each take a specific subtype; assert that they got that correct > subtype. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 07/14] qapi/doc.py: enable mypy checks
On Tue, Sep 22, 2020 at 05:17:55PM -0400, John Snow wrote: > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 03/14] qapi/doc.py: Add assertion on section.member
On Tue, Sep 22, 2020 at 05:17:51PM -0400, John Snow wrote: > Similarly to other cases, we lack the power at the moment to express > that a specific member is constrained to a certain containing type. Add > an assertion before we use properties specific to that type. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 02/14] qapi/doc.py: avoid unnecessary keyword arguments
On Tue, Sep 22, 2020 at 05:17:50PM -0400, John Snow wrote: > Keyword Callables are hard to type in Python 3.6, avoid them if there's > no urgent need to use them. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 01/14] qapi/doc.py: stash long temporary locals in named locals
On Tue, Sep 22, 2020 at 05:17:49PM -0400, John Snow wrote: > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PULL v3 00/15] virtio,pc,acpi: fixes, tests
Peter, you said you see issues on some systems. I pushed a tag: git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream_test which drops some patches I suspect. If that helps, pls let me know. On Mon, Sep 21, 2020 at 08:31:35AM -0400, Michael S. Tsirkin wrote: > On Mon, Sep 21, 2020 at 07:44:42PM +0800, Li Qiang wrote: > > Michael S. Tsirkin 于2020年9月21日周一 下午7:30写道： > > > > > > The following changes since commit > > > 053a4177817db307ec854356e95b5b350800a216: > > > > > > Merge remote-tracking branch > > > 'remotes/philmd-gitlab/tags/fw_cfg-20200918' into staging (2020-09-18 > > > 16:34:26 +0100) > > > > > > are available in the Git repository at: > > > > > > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream > > > > > > for you to fetch changes up to acbdbd5db6dab68534702987a487360fd8ae02b7: > > > > > > virtio-iommu-pci: force virtio version 1 (2020-09-21 06:14:46 -0400) > > > > > > > > > virtio,pc,acpi: fixes, tests > > > > > > Fixes and tests all over the place. > > > Batch iommu updates for vdpa. > > > Removal of deprecated cpu hotplug commands. > > > > > > Signed-off-by: Michael S. Tsirkin > > > > > > > > > David Hildenbrand (1): > > > pc: fix auto_enable_numa_with_memhp/auto_enable_numa_with_memdev > > > for the 5.0 machine > > > > > > Dima Stepanov (7): > > > vhost: recheck dev state in the vhost_migration_log routine > > > vhost: check queue state in the vhost_dev_set_log routine > > > tests/qtest/vhost-user-test: prepare the tests for adding new dev > > > class > > > tests/qtest/libqos/virtio-blk: add support for vhost-user-blk > > > tests/qtest/vhost-user-test: add support for the vhost-user-blk > > > device > > > tests/qtest/vhost-user-test: add migrate_reconnect test > > > tests/qtest/vhost-user-test: enable the reconnect tests > > > > > > Eric Auger (2): > > > virtio-iommu: Check gtrees are non null before destroying them > > > virtio-iommu-pci: force virtio version 1 > > > > > > Igor Mammedov (1): > > > cphp: remove deprecated cpu-add command(s) > > > > > > Jason Wang (3): > > > linux headers: sync to 5.9-rc4 > > > vhost: switch to use IOTLB v2 format > > > vhost-vdpa: batch updating IOTLB mappings > > > > > > Li Qiang (1): > > > virtio-mem: detach the element from the virtqueue when error occurs > > > > Hello Michael, > > It seems you lost the virtio-pmem patch. > > > > https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg02639.html > > That's because I was never copied. Can you repost with all tags and Cc > me? > Subject can be PATCH repost. > > > Anyway, it can be queued in the next pr. > > > > Thanks, > > Li Qiang
[PATCH] docs: Better mention of qemu-img amend limitations
Missed during merge resolution of commit bc5ee6da71. Signed-off-by: Eric Blake --- docs/tools/qemu-img.rst | 4 1 file changed, 4 insertions(+) diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index c35bd6482203..2b5891b54db7 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -265,6 +265,10 @@ Command description: --force allows some unsafe operations. Currently for -f luks, it allows to erase the last encryption key, and to overwrite an active encryption key. + The set of options that can be amended are dependent on the image + format, but note that amending the backing chain relationship should + instead be performed with ``qemu-img rebase``. + .. option:: bench [-c COUNT] [-d DEPTH] [-f FMT] [--flush-interval=FLUSH_INTERVAL] [-i AIO] [-n] [--no-drain] [-o OFFSET] [--pattern=PATTERN] [-q] [-s BUFFER_SIZE] [-S STEP_SIZE] [-t CACHE] [-w] [-U] FILENAME Run a simple sequential I/O benchmark on the specified image. If ``-w`` is -- 2.28.0
Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table
On Tue, Sep 22, 2020 at 05:13:13PM -0400, John Snow wrote:
> This enforces a type signature against all of the top-level expression
> check routines without necessarily needing to create some
> overcomplicated class hierarchy for them.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/expr.py | 69 ++--
> 1 file changed, 35 insertions(+), 34 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 74b2681ef8..cfd342aa04 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -31,8 +31,11 @@
> structures and contextual semantic validation.
> """
>
> +from enum import Enum
> import re
> from typing import (
> +Callable,
> +Dict,
> Iterable,
> List,
> MutableMapping,
> @@ -494,6 +497,26 @@ def check_event(expr: Expression, info: QAPISourceInfo)
> -> None:
> check_type(args, info, "'data'", allow_dict=not boxed)
>
>
> +class ExpressionType(str, Enum):
> +INCLUDE = 'include'
> +ENUM = 'enum'
> +UNION = 'union'
> +ALTERNATE = 'alternate'
> +STRUCT = 'struct'
> +COMMAND = 'command'
> +EVENT = 'event'
> +
> +
> +_CHECK_FN: Dict[str, Callable[[Expression, QAPISourceInfo], None]] = {
> +'enum': check_enum,
> +'union': check_union,
> +'alternate': check_alternate,
> +'struct': check_struct,
> +'command': check_command,
> +'event': check_event,
> +}
> +
> +
> def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
> """
> Validate and normalize a list of parsed QAPI schema expressions. [RW]
> @@ -519,28 +542,20 @@ def check_exprs(exprs: List[_JSObject]) ->
> List[_JSObject]:
> assert tmp is None or isinstance(tmp, QAPIDoc)
> doc: Optional[QAPIDoc] = tmp
>
> -if 'include' in expr:
> -continue
> -
> -if 'enum' in expr:
> -meta = 'enum'
> -elif 'union' in expr:
> -meta = 'union'
> -elif 'alternate' in expr:
> -meta = 'alternate'
> -elif 'struct' in expr:
> -meta = 'struct'
> -elif 'command' in expr:
> -meta = 'command'
> -elif 'event' in expr:
> -meta = 'event'
> +for kind in ExpressionType:
> +if kind in expr:
> +meta = kind
I see lots of meta.value expressions below. Maybe setting
meta = kind.value
will make the code more readable?
I don't think this should block an obvious improvement to the
code, so:
Reviewed-by: Eduardo Habkost
> +break
> else:
> raise QAPISemError(info, "expression is missing metatype")
>
> +if meta == ExpressionType.INCLUDE:
> +continue
> +
> name = cast(str, expr[meta]) # asserted right below:
> -check_name_is_str(name, info, "'%s'" % meta)
> -info.set_defn(meta, name)
> -check_defn_name_str(name, info, meta)
> +check_name_is_str(name, info, "'%s'" % meta.value)
> +info.set_defn(meta.value, name)
> +check_defn_name_str(name, info, meta.value)
>
> if doc:
> if doc.symbol != name:
> @@ -551,22 +566,8 @@ def check_exprs(exprs: List[_JSObject]) ->
> List[_JSObject]:
> raise QAPISemError(info,
> "documentation comment required")
>
> -if meta == 'enum':
> -check_enum(expr, info)
> -elif meta == 'union':
> -check_union(expr, info)
> -elif meta == 'alternate':
> -check_alternate(expr, info)
> -elif meta == 'struct':
> -check_struct(expr, info)
> -elif meta == 'command':
> -check_command(expr, info)
> -elif meta == 'event':
> -check_event(expr, info)
> -else:
> -assert False, 'unexpected meta type'
> -
> -check_if(expr, info, meta)
> +_CHECK_FN[meta](expr, info)
> +check_if(expr, info, meta.value)
> check_features(expr.get('features'), info)
> check_flags(expr, info)
>
> --
> 2.26.2
>
--
Eduardo
Re: [PULL 00/13] Block patches
Patchew URL: https://patchew.org/QEMU/20200923161031.69474-1-stefa...@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20200923161031.69474-1-stefa...@redhat.com Subject: [PULL 00/13] Block patches === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === From https://github.com/patchew-project/qemu * [new tag] patchew/20200923161031.69474-1-stefa...@redhat.com -> patchew/20200923161031.69474-1-stefa...@redhat.com Switched to a new branch 'test' 4bf3742 qemu/atomic.h: rename atomic_ to qatomic_ 8cecbb3 tests: add test-fdmon-epoll 8df4df8 fdmon-poll: reset npfd when upgrading to fdmon-epoll 825386c gitmodules: add qemu.org vbootrom submodule 5cae6f8 gitmodules: switch to qemu.org meson mirror 98349d3 gitmodules: switch to qemu.org qboot mirror 5f9d298 docs/system: clarify deprecation schedule b431c3a virtio-crypto: don't modify elem->in/out_sg c1f9691 virtio-blk: undo destructive iov_discard_*() operations 4182d4f util/iov: add iov_discard_undo() a59a839 virtio: add vhost-user-fs-ccw device 0d23a8c libvhost-user: handle endianness as mandated by the spec 3167d81 MAINTAINERS: add Stefan Hajnoczi as block/nvme.c maintainer === OUTPUT BEGIN === 1/13 Checking commit 3167d818e62a (MAINTAINERS: add Stefan Hajnoczi as block/nvme.c maintainer) 2/13 Checking commit 0d23a8ccd2a9 (libvhost-user: handle endianness as mandated by the spec) WARNING: line over 80 characters #53: FILE: contrib/libvhost-user/libvhost-user.c:548: +vu_panic(dev, "virtio legacy devices aren't supported by libvhost-user"); total: 0 errors, 1 warnings, 217 lines checked Patch 2/13 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 3/13 Checking commit a59a839e1d76 (virtio: add vhost-user-fs-ccw device) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #28: new file mode 100644 total: 0 errors, 1 warnings, 82 lines checked Patch 3/13 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 4/13 Checking commit 4182d4f76678 (util/iov: add iov_discard_undo()) 5/13 Checking commit c1f96917a3f1 (virtio-blk: undo destructive iov_discard_*() operations) 6/13 Checking commit b431c3a4b415 (virtio-crypto: don't modify elem->in/out_sg) 7/13 Checking commit 5f9d298c71ad (docs/system: clarify deprecation schedule) 8/13 Checking commit 98349d316498 (gitmodules: switch to qemu.org qboot mirror) 9/13 Checking commit 5cae6f86bb22 (gitmodules: switch to qemu.org meson mirror) 10/13 Checking commit 825386c4f0f2 (gitmodules: add qemu.org vbootrom submodule) 11/13 Checking commit 8df4df890bf1 (fdmon-poll: reset npfd when upgrading to fdmon-epoll) 12/13 Checking commit 8cecbb39509f (tests: add test-fdmon-epoll) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #42: new file mode 100644 total: 0 errors, 1 warnings, 89 lines checked Patch 12/13 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 13/13 Checking commit 4bf37426ff03 (qemu/atomic.h: rename atomic_ to qatomic_) ERROR: Macros with multiple statements should be enclosed in a do - while loop #2800: FILE: include/qemu/atomic.h:152: +#define qatomic_rcu_read__nocheck(ptr, valptr) \ +__atomic_load(ptr, valptr, __ATOMIC_RELAXED);\ smp_read_barrier_depends(); ERROR: space required before that '*' (ctx:VxB) #2945: FILE: include/qemu/atomic.h:333: +#define qatomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p)) ^ ERROR: Use of volatile is usually wrong, please add a comment #2945: FILE: include/qemu/atomic.h:333: +#define qatomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p)) ERROR: space required before that '*' (ctx:VxB) #2946: FILE: include/qemu/atomic.h:334: +#define qatomic_set__nocheck(p, i) ((*(__typeof__(*(p)) volatile*) (p)) = (i)) ^ ERROR: Use of volatile is usually wrong, please add a comment #2946: FILE: include/qemu/atomic.h:334: +#define qatomic_set__nocheck(p, i) ((*(__typeof__(*(p)) volatile*) (p)) = (i)) ERROR: space required after that ',' (ctx:VxV) #2951: FILE: include/qemu/atomic.h:337: +#define qatomic_set(ptr, i) qatomic_set__nocheck(ptr,i) ^ ERROR: memory barrier without comment #3023: FILE: include/qemu/atomic.h:395: +#define qatomic_xchg(ptr, i)(smp_mb(), __sync_lock_test_and_set(ptr, i)) WARNING: Block comments use a
Re: [PATCH 15/16] qapi/expr.py: move related checks inside check_xxx functions
On Tue, Sep 22, 2020 at 05:13:12PM -0400, John Snow wrote:
> There's not a big obvious difference between the types of checks that
> happen in the main function versus the kind that happen in the
> functions. Now they're in one place for each of the main types.
>
> As part of the move, spell out the required and optional keywords so
> they're obvious at a glance.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/expr.py | 55 ++--
> 1 file changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 69ee9e3f18..74b2681ef8 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -333,6 +333,10 @@ def check_enum(expr: Expression, info: QAPISourceInfo)
> -> None:
> :param expr: `Expression` to validate.
> :param info: QAPI source file information.
> """
> +check_keys(expr, info, 'enum',
> + required=('enum', 'data'),
> + optional=('if', 'features', 'prefix'))
> +
> name = expr['enum']
> members = expr['data']
> prefix = expr.get('prefix')
> @@ -363,6 +367,11 @@ def check_struct(expr: Expression, info: QAPISourceInfo)
> -> None:
> :param expr: `Expression` to validate.
> :param info: QAPI source file information.
> """
> +check_keys(expr, info, 'struct',
> + required=('struct', 'data'),
> + optional=('base', 'if', 'features'))
> +normalize_members(expr['data'])
> +
> name = cast(str, expr['struct']) # Asserted in check_exprs
> members = expr['data']
>
> @@ -377,6 +386,13 @@ def check_union(expr: Expression, info: QAPISourceInfo)
> -> None:
> :param expr: `Expression` to validate.
> :param info: QAPI source file information.
> """
> +check_keys(expr, info, 'union',
> + required=('union', 'data'),
> + optional=('base', 'discriminator', 'if', 'features'))
> +
> +normalize_members(expr.get('base'))
> +normalize_members(expr['data'])
> +
> name = cast(str, expr['union']) # Asserted in check_exprs
> base = expr.get('base')
> discriminator = expr.get('discriminator')
> @@ -409,6 +425,11 @@ def check_alternate(expr: Expression, info:
> QAPISourceInfo) -> None:
> :param expr: Expression to validate.
> :param info: QAPI source file information.
> """
> +check_keys(expr, info, 'alternate',
> + required=('alternate', 'data'),
> + optional=('if', 'features'))
> +normalize_members(expr['data'])
> +
> members = expr['data']
>
> if not members:
> @@ -432,6 +453,13 @@ def check_command(expr: Expression, info:
> QAPISourceInfo) -> None:
> :param expr: `Expression` to validate.
> :param info: QAPI source file information.
> """
> +check_keys(expr, info, 'command',
> + required=['command'],
> + optional=('data', 'returns', 'boxed', 'if', 'features',
> + 'gen', 'success-response', 'allow-oob',
> + 'allow-preconfig'))
> +normalize_members(expr.get('data'))
> +
> args = expr.get('data')
> rets = expr.get('returns')
> boxed = expr.get('boxed', False)
> @@ -453,6 +481,11 @@ def check_event(expr: Expression, info: QAPISourceInfo)
> -> None:
> if: `Optional[Ifcond]`
> features: `Optional[Features]`
> """
> +normalize_members(expr.get('data'))
> +check_keys(expr, info, 'event',
> + required=['event'],
> + optional=('data', 'boxed', 'if', 'features'))
Why is the order reversed here? The other functions call
check_keys() before normalize_members().
> +
> args = expr.get('data')
> boxed = expr.get('boxed', False)
>
> @@ -519,38 +552,16 @@ def check_exprs(exprs: List[_JSObject]) ->
> List[_JSObject]:
> "documentation comment required")
>
> if meta == 'enum':
> -check_keys(expr, info, meta,
> - ['enum', 'data'], ['if', 'features', 'prefix'])
> check_enum(expr, info)
> elif meta == 'union':
> -check_keys(expr, info, meta,
> - ['union', 'data'],
> - ['base', 'discriminator', 'if', 'features'])
> -normalize_members(expr.get('base'))
> -normalize_members(expr['data'])
> check_union(expr, info)
> elif meta == 'alternate':
> -check_keys(expr, info, meta,
> - ['alternate', 'data'], ['if', 'features'])
> -normalize_members(expr['data'])
> check_alternate(expr, info)
> elif meta == 'struct':
> -check_keys(expr, info, meta,
> - ['struct', 'data'], ['base', 'if', 'features'])
> -normalize_members(expr['data'])
> check_struct(expr, info)
> elif meta == 'command':
Re: [PATCH v2 19/38] qapi/commands.py: Don't re-bind to variable of different type
On Tue, Sep 22, 2020 at 05:00:42PM -0400, John Snow wrote: > Mypy isn't a fan of rebinding a variable with a new data type. > It's easy enough to avoid. > > Signed-off-by: John Snow Reviewed-by: Cleber Rosa signature.asc Description: PGP signature
Re: [PATCH 14/16] qapi/expr.py: Use tuples instead of lists for static data
On Tue, Sep 22, 2020 at 05:13:11PM -0400, John Snow wrote: > It is -- maybe -- possibly -- three nanoseconds faster. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH v2 18/38] qapi/events.py: Move comments into docstrings
On Tue, Sep 22, 2020 at 05:00:41PM -0400, John Snow wrote: > Signed-off-by: John Snow Reviewed-by: Cleber Rosa signature.asc Description: PGP signature
Re: [PATCH 13/16] qapi/expr.py: Modify check_keys to accept any Iterable
On Tue, Sep 22, 2020 at 05:13:10PM -0400, John Snow wrote: > This is a very minor adjustment. > > a + b is list-specific behavior, but we can accept a wider variety of > types in a more pythonic fashion if we avoid that behavior. > > Typing it this way allows callers to use things like dict.keys() and > other iterables that are not their own discrete lists. > > Including it just as a statement of practice if nothing else: It's nice > to use the least-specific type possible as function input and use the > most-specific type for returns. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH v2 17/38] qapi/events.py: add type hint annotations
On Tue, Sep 22, 2020 at 05:00:40PM -0400, John Snow wrote: > Annotations do not change runtime behavior. > This commit *only* adds annotations. > > Signed-off-by: John Snow Reviewed-by: Cleber Rosa signature.asc Description: PGP signature
Re: [PATCH 12/16] qapi/expr.py: Add docstrings
On Tue, Sep 22, 2020 at 05:13:09PM -0400, John Snow wrote: > Signed-off-by: John Snow > --- [...] > @@ -160,6 +244,18 @@ def check_type(value: Optional[object], > source: str, > allow_array: bool = False, > allow_dict: Union[bool, str] = False) -> None: > +""" > +Check the QAPI type of `value`. [RW] > + > +Python types of `str` or `None` are always allowed. > + > +:param value: The value to check. > +:param info:QAPI Source file information. > +:param source: Human readable string describing "what" the value is. > +:param allow_array: Allow a `List[str]` of length 1, > +which indicates an Array type. > +:param allow_dict: Allow a dict, treated as an anonymous type. I was hoping the docstring would explain what happens when allow_dict is a string. -- Eduardo
Re: [PATCH 11/16] qapi/expr.py: enable pylint checks
On Tue, Sep 22, 2020 at 05:13:08PM -0400, John Snow wrote: > Signed-off-by: John Snow Tested-by: Eduardo Habkost Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH v2 16/38] qapi: establish mypy type-checking baseline
On Tue, Sep 22, 2020 at 05:00:39PM -0400, John Snow wrote: > Fix two very minor issues, and then establish a mypy type-checking > baseline. > > Like pylint, this should be run from the folder above: > > > mypy --config-file=qapi/mypy.ini qapi/ Like with pylint, let's at least document the version of mypy used. For example, with 0.740, this fails. With 0.782, it passes. - Cleber. signature.asc Description: PGP signature
Re: [PATCH v8 7/7] block/io: refactor save/load vmstate
On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:
Like for read/write in a previous commit, drop extra indirection layer,
generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate().
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Eric Blake
---
block/coroutines.h| 10 +++
include/block/block.h | 6 ++--
block/io.c| 67 ++-
3 files changed, 42 insertions(+), 41 deletions(-)
diff --git a/block/coroutines.h b/block/coroutines.h
int coroutine_fn
-bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
- bool is_read)
+bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
{
BlockDriver *drv = bs->drv;
int ret = -ENOTSUP;
+if (!drv) {
+return -ENOMEDIUM;
+}
+
bdrv_inc_in_flight(bs);
-if (!drv) {
-ret = -ENOMEDIUM;
-} else if (drv->bdrv_load_vmstate) {
-if (is_read) {
-ret = drv->bdrv_load_vmstate(bs, qiov, pos);
-} else {
-ret = drv->bdrv_save_vmstate(bs, qiov, pos);
-}
+if (drv->bdrv_load_vmstate) {
+ret = drv->bdrv_load_vmstate(bs, qiov, pos);
This one makes sense;
} else if (bs->file) {
-ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read);
+ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos);
}
bdrv_dec_in_flight(bs);
+
return ret;
}
-int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
- int64_t pos, int size)
+int coroutine_fn
+bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
{
-QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
-int ret;
+BlockDriver *drv = bs->drv;
+int ret = -ENOTSUP;
-ret = bdrv_writev_vmstate(bs, , pos);
-if (ret < 0) {
-return ret;
+if (!drv) {
+return -ENOMEDIUM;
}
-return size;
-}
+bdrv_inc_in_flight(bs);
-int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
-{
-return bdrv_rw_vmstate(bs, qiov, pos, false);
+if (drv->bdrv_load_vmstate) {
+ret = drv->bdrv_save_vmstate(bs, qiov, pos);
but this one looks awkward. It represents the pre-patch logic, but it
would be nicer to check for bdrv_save_vmstate. With that tweak, my R-b
still stands.
I had an interesting time applying this patch due to merge conflicts
with the new bdrv_primary_bs() changes that landed in the meantime.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 10/16] qapi/expr.py: Remove single-letter variable
On Tue, Sep 22, 2020 at 05:13:07PM -0400, John Snow wrote: > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 09/16] qapi/expr.py: rewrite check_if
On Tue, Sep 22, 2020 at 05:13:06PM -0400, John Snow wrote: > This is a only minor rewrite to address some minor style nits. Don't > compare against the empty list to check for the empty condition, and > move the normalization forward to unify the check on the now-normalized > structure. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 08/16] qapi/expr.py: add type hint annotations
On Tue, Sep 22, 2020 at 05:13:05PM -0400, John Snow wrote: > Annotations do not change runtime behavior. > This commit *only* adds annotations. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH v2 15/38] qapi/common.py: move build_params into gen.py
On Tue, Sep 22, 2020 at 05:00:38PM -0400, John Snow wrote: > Including it in common.py creates a circular import dependency; schema > relies on common, but common.build_params requires a type annotation > from schema. To type this properly, it needs to be moved outside the > cycle. > > Signed-off-by: John Snow Reviewed-by: Cleber Rosa signature.asc Description: PGP signature
Re: [PATCH 07/16] qapi/expr.py: Add casts in a few select cases
On Tue, Sep 22, 2020 at 05:13:04PM -0400, John Snow wrote: > Casts are instructions to the type checker only, they aren't "safe" and > should probably be avoided in general. In this case, when we perform > type checking on a nested structure, the type of each field does not > "stick". > > We don't need to assert that something is a str if we've already checked > that it is -- use a cast instead for these cases. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 06/16] qapi/expr.py: Check type of 'data' member
On Tue, Sep 22, 2020 at 05:13:03PM -0400, John Snow wrote: > Iterating over the members of data isn't going to work if it's not some > form of dict anyway, but for type safety, formalize it. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 05/16] qapi/expr.py: move string check upwards in check_type
On Tue, Sep 22, 2020 at 05:13:02PM -0400, John Snow wrote: > It's a simple case, shimmy the early return upwards. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote: > mypy isn't fond of allowing you to check for bool membership in a > collection of str elements. Guard this lookup for precisely when we were > given a name. > > Signed-off-by: John Snow > --- > scripts/qapi/expr.py | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index f6b55a87c1..67892502e9 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -166,7 +166,9 @@ def check_type(value, info, source, > raise QAPISemError(info, > "%s should be an object or type name" % source) > > -permit_upper = allow_dict in info.pragma.name_case_whitelist > +permit_upper = False > +if isinstance(allow_dict, str): > +permit_upper = allow_dict in info.pragma.name_case_whitelist Well, this keeps existing behavior, so: Reviewed-by: Eduardo Habkost But: what exactly is the meaning of allow_dict=False, allow_dict=True, and allow_dict being a string? > > # value is a dictionary, check that each member is okay > for (key, arg) in value.items(): > -- > 2.26.2 > -- Eduardo
Re: [PATCH 03/16] qapi/expr.py: constrain incoming expression types
On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
> mypy does not know the types of values stored in Dicts that masquerade
> as objects. Help the type checker out by constraining the type.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/expr.py | 25 ++---
> 1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 1872a8a3cc..f6b55a87c1 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -15,9 +15,17 @@
> # See the COPYING file in the top-level directory.
>
> import re
> +from typing import MutableMapping, Optional
>
> from .common import c_name
> from .error import QAPISemError
> +from .parser import QAPIDoc
> +from .source import QAPISourceInfo
> +
> +
> +# Expressions in their raw form are JSON-like structures with arbitrary
> forms.
> +# Minimally, their top-level form must be a mapping of strings to values.
> +Expression = MutableMapping[str, object]
>
>
> # Names must be letters, numbers, -, and _. They must start with letter,
> @@ -280,9 +288,20 @@ def check_event(expr, info):
>
> def check_exprs(exprs):
> for expr_elem in exprs:
> -expr = expr_elem['expr']
> -info = expr_elem['info']
> -doc = expr_elem.get('doc')
> +# Expression
> +assert isinstance(expr_elem['expr'], dict)
> +expr: Expression = expr_elem['expr']
> +for key in expr.keys():
> +assert isinstance(key, str)
mypy doesn't seem to require the key type asserts, on my testing.
> +
> +# QAPISourceInfo
> +assert isinstance(expr_elem['info'], QAPISourceInfo)
> +info: QAPISourceInfo = expr_elem['info']
> +
> +# Optional[QAPIDoc]
> +tmp = expr_elem.get('doc')
> +assert tmp is None or isinstance(tmp, QAPIDoc)
> +doc: Optional[QAPIDoc] = tmp
Do you need a separate variable here? This seems to work too:
doc = expr_elem.get('doc')
assert doc is None or isinstance(doc, QAPIDoc)
after the assert, mypy will infer the type of doc to be
Optional[QAPIDoc].
None of this should block a useful 120-patch cleanup series, so:
Reviewed-by: Eduardo Habkost
>
> if 'include' in expr:
> continue
> --
> 2.26.2
>
--
Eduardo
[PATCH 5/6] spapr_numa: consider user input when defining associativity
This patch puts all the pieces together to finally allow user
input when defining the NUMA topology of the spapr guest.
We have one more kernel restriction to handle in this patch:
the associativity array of node 0 must be filled with zeroes
[1]. The strategy below ensures that this will happen.
spapr_numa_define_associativity_domains() will read the distance
(already PAPRified) between the nodes from numa_state and determine
the appropriate NUMA level. The NUMA domains, processed in ascending
order, are going to be matched via NUMA levels, and the lowest
associativity domain value is assigned to that specific level for
both.
This will create an heuristic where the associativities of the first
nodes have higher priority and are re-used in new matches, instead of
overwriting them with a new associativity match. This is necessary
because neither QEMU, nor the pSeries kernel, supports multiple
associativity domains for each resource, meaning that we have to
decide which associativity relation is relevant.
Ultimately, all of this results in a best effort approximation for
the actual NUMA distances the user input in the command line. Given
the nature of how PAPR itself interprets NUMA distances versus the
expectations risen by how ACPI SLIT works, there might be better
algorithms but, in the end, it'll also result in another way to
approximate what the user really wanted.
To keep this commit message no longer than it already is, the next
patch will update the existing documentation in ppc-spapr-numa.rst
with more in depth details and design considerations/drawbacks.
[1]
https://lore.kernel.org/linuxppc-dev/5e8fbea3-8faf-0951-172a-b41a2138f...@gmail.com/
Signed-off-by: Daniel Henrique Barboza
---
hw/ppc/spapr_numa.c | 81 -
1 file changed, 80 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
index 688391278e..c84f77cda7 100644
--- a/hw/ppc/spapr_numa.c
+++ b/hw/ppc/spapr_numa.c
@@ -80,12 +80,79 @@ static void spapr_numa_PAPRify_distances(MachineState *ms)
}
}
+static uint8_t spapr_numa_get_NUMA_level(uint8_t distance)
+{
+uint8_t numa_level;
+
+switch (distance) {
+case 20:
+numa_level = 0x3;
+break;
+case 40:
+numa_level = 0x2;
+break;
+case 80:
+numa_level = 0x1;
+break;
+default:
+numa_level = 0;
+}
+
+return numa_level;
+}
+
+static void spapr_numa_define_associativity_domains(SpaprMachineState *spapr,
+MachineState *ms)
+{
+int src, dst;
+int nb_numa_nodes = ms->numa_state->num_nodes;
+NodeInfo *numa_info = ms->numa_state->nodes;
+
+for (src = 0; src < nb_numa_nodes; src++) {
+for (dst = src; dst < nb_numa_nodes; dst++) {
+/*
+ * This is how the associativity domain between A and B
+ * is calculated:
+ *
+ * - get the distance between them
+ * - get the correspondent NUMA level for this distance
+ * - the arrays were initialized with their own numa_ids,
+ * and we're calculating the distance in node_id ascending order,
+ * starting from node 0. This will have a cascade effect in the
+ * algorithm because the associativity domains that node 0 defines
+ * will be carried over to the other nodes, and node 1
+ * associativities will be carried over unless there's already a
+ * node 0 associativity assigned, and so on. This happens because
+ * we'll assign the lowest value of assoc_src and assoc_dst to be
+ * the associativity domain of both, for the given NUMA level.
+ *
+ * The PPC kernel expects the associativity domains of node 0 to
+ * be always 0, and this algorithm will grant that by default.
+ */
+uint8_t distance = numa_info[src].distance[dst];
+uint8_t n_level = spapr_numa_get_NUMA_level(distance);
+uint32_t assoc_src, assoc_dst;
+
+assoc_src = be32_to_cpu(spapr->numa_assoc_array[src][n_level]);
+assoc_dst = be32_to_cpu(spapr->numa_assoc_array[dst][n_level]);
+
+if (assoc_src < assoc_dst) {
+spapr->numa_assoc_array[dst][n_level] = cpu_to_be32(assoc_src);
+} else {
+spapr->numa_assoc_array[src][n_level] = cpu_to_be32(assoc_dst);
+}
+}
+}
+
+}
+
void spapr_numa_associativity_init(SpaprMachineState *spapr,
MachineState *machine)
{
SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
int nb_numa_nodes = machine->numa_state->num_nodes;
int i, j, max_nodes_with_gpus;
+bool using_legacy_numa = spapr_machine_using_legacy_numa(spapr);
/*
* For all associativity arrays: first position is the size,
@@ -99,6 +166,17 @@ void
Re: [PATCH v8 2/7] block/io: refactor coroutine wrappers
On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote: Most of our coroutine wrappers already follow this convention: We have 'coroutine_fn bdrv_co_()' as the core function, and a wrapper 'bdrv_()' which does parameters packing and call bdrv_run_co(). The only outsiders are the bdrv_prwv_co and bdrv_common_block_status_above wrappers. Let's refactor them to behave as the others, it simplifies further conversion of coroutine wrappers. This patch adds indirection layer, but it will be compensated by adds an further commit, which will drop bdrv_co_prwv together with is_write logic, to keep read and write path separate. keep the Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake R-b stands. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH 1/6] spapr: add spapr_machine_using_legacy_numa() helper
The changes to come to NUMA support are all guest visible. In
theory we could just create a new 5_1 class option flag to
avoid the changes to cascade to 5.1 and under. The reality is that
these changes are only relevant if the machine has more than one
NUMA node. There is no need to change guest behavior that has
been around for years needlesly.
This new helper will be used by the next patches to determine
whether we should retain the (soon to be) legacy NUMA behavior
in the pSeries machine. The new behavior will only be exposed
if:
- machine is pseries-5.2 and newer;
- more than one NUMA node is declared in NUMA state.
Reviewed-by: David Gibson
Signed-off-by: Daniel Henrique Barboza
---
hw/ppc/spapr.c | 12
include/hw/ppc/spapr.h | 2 ++
2 files changed, 14 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e813c7cfb9..c5d8910a74 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -294,6 +294,15 @@ static hwaddr spapr_node0_size(MachineState *machine)
return machine->ram_size;
}
+bool spapr_machine_using_legacy_numa(SpaprMachineState *spapr)
+{
+MachineState *machine = MACHINE(spapr);
+SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
+
+return smc->pre_5_2_numa_associativity ||
+ machine->numa_state->num_nodes <= 1;
+}
+
static void add_str(GString *s, const gchar *s1)
{
g_string_append_len(s, s1, strlen(s1) + 1);
@@ -4522,8 +4531,11 @@ DEFINE_SPAPR_MACHINE(5_2, "5.2", true);
*/
static void spapr_machine_5_1_class_options(MachineClass *mc)
{
+SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
+
spapr_machine_5_2_class_options(mc);
compat_props_add(mc->compat_props, hw_compat_5_1, hw_compat_5_1_len);
+smc->pre_5_2_numa_associativity = true;
}
DEFINE_SPAPR_MACHINE(5_1, "5.1", false);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 114e819969..d1aae03b97 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -143,6 +143,7 @@ struct SpaprMachineClass {
bool smp_threads_vsmt; /* set VSMT to smp_threads by default */
hwaddr rma_limit; /* clamp the RMA to this size */
bool pre_5_1_assoc_refpoints;
+bool pre_5_2_numa_associativity;
void (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
uint64_t *buid, hwaddr *pio,
@@ -860,6 +861,7 @@ int spapr_max_server_number(SpaprMachineState *spapr);
void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
uint64_t pte0, uint64_t pte1);
void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered);
+bool spapr_machine_using_legacy_numa(SpaprMachineState *spapr);
/* DRC callbacks. */
void spapr_core_release(DeviceState *dev);
--
2.26.2
Re: [PATCH v2 14/38] qapi/common.py: Convert comments into docstrings, and elaborate
On Tue, Sep 22, 2020 at 05:00:37PM -0400, John Snow wrote:
> As docstrings, they'll show up in documentation and IDE help.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/common.py | 51 ++
> 1 file changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 0ce4a107e6..730283722a 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -20,10 +20,18 @@
> _C_NAME_TRANS = str.maketrans('.-', '__')
>
>
> -# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
> -# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
> -# ENUM24_Name -> ENUM24_NAME
> def camel_to_upper(value: str) -> str:
> +"""
> +Converts CamelCase to CAMEL_CASE.
> +
> +Examples:
> + ENUMName -> ENUM_NAME
> + EnumName1 -> ENUM_NAME1
> + ENUM_NAME -> ENUM_NAME
> + ENUM_NAME1 -> ENUM_NAME1
> + ENUM_Name2 -> ENUM_NAME2
> + ENUM24_Name -> ENUM24_NAME
> +"""
> c_fun_str = c_name(value, False)
> if value.isupper():
> return c_fun_str
> @@ -45,21 +53,33 @@ def camel_to_upper(value: str) -> str:
> def c_enum_const(type_name: str,
> const_name: str,
> prefix: Optional[str] = None) -> str:
> +"""
> +Generate a C enumeration constant name.
> +
> +:param type_name: The name of the enumeration.
> +:param const_name: The name of this constant.
> +:param prefix: Optional, prefix that overrides the type_name.
> +"""
> if prefix is not None:
> type_name = prefix
> return camel_to_upper(type_name) + '_' + c_name(const_name,
> False).upper()
>
>
> -# Map @name to a valid C identifier.
> -# If @protect, avoid returning certain ticklish identifiers (like
> -# C keywords) by prepending 'q_'.
> -#
> -# Used for converting 'name' from a 'name':'type' qapi definition
> -# into a generated struct member, as well as converting type names
> -# into substrings of a generated C function name.
> -# '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
> -# protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
> def c_name(name: str, protect: bool = True) -> str:
> +"""
> +Map `name` to a valid C identifier.
> +
> +Used for converting 'name' from a 'name':'type' qapi definition
> +into a generated struct member, as well as converting type names
> +into substrings of a generated C function name.
> +
> +'__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
> +protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
> +
> +:param name: The name to map.
> +:param protect: If true, avoid returning certain ticklish identifiers
> +(like C keywords) by prepending ``q_``.
> +"""
> # ANSI X3J11/88-090, 3.1.1
> c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
> 'default', 'do', 'double', 'else', 'enum', 'extern',
> @@ -134,9 +154,12 @@ def decrease(self, amount: int = 4) -> int:
> indent = Indentation()
>
>
> -# Generate @code with @kwds interpolated.
> -# Obey indent, and strip EATSPACE.
> def cgen(code: str, **kwds: object) -> str:
> +"""
> +Generate `code` with `kwds` interpolated.
> +
> +Obey `indent`, and strip `EATSPACE`.
> +"""
This probably won't help on IDEs (never checked any), but sphinx will
let you do:
"""
Generate `code` with `kwds` interpolated.
Obey `indent`, and strip :data:`EATSPACE`.
"""
I'm not sure that a maximum level of docstring "sphinxzation" is the
goal here, though.
Reviewed-by: Cleber Rosa
signature.asc
Description: PGP signature
[PATCH 3/6] spapr_numa: translate regular NUMA distance to PAPR distance
QEMU allows the user to set NUMA distances in the command line.
For ACPI architectures like x86, this means that user input is
used to populate the SLIT table, and the guest perceives the
distances as the user chooses to.
PPC64 does not work that way. In the PAPR concept of NUMA,
associativity relations between the NUMA nodes are provided by
the device tree, and the guest kernel is free to calculate the
distances as it sees fit. Given how ACPI architectures works,
this puts the pSeries machine in a strange spot - users expect
to define NUMA distances like in the ACPI case, but QEMU does
not have control over it. To give pSeries users a similar
experience, we'll need to bring kernel specifics to QEMU
to approximate the NUMA distances.
The pSeries kernel works with the NUMA distance range 10,
20, 40, 80 and 160. The code starts at 10 (local distance) and
searches for a match in the first NUMA level between the
resources. If there is no match, the distance is doubled and
then it proceeds to try to match in the next NUMA level. Rinse
and repeat for MAX_DISTANCE_REF_POINTS levels.
This patch introduces a spapr_numa_PAPRify_distances() helper
that translates the user distances to kernel distance, which
we're going to use to determine the associativity domains for
the NUMA nodes.
Signed-off-by: Daniel Henrique Barboza
---
hw/ppc/spapr_numa.c | 44
1 file changed, 44 insertions(+)
diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
index 36aaa273ee..180800b2f3 100644
--- a/hw/ppc/spapr_numa.c
+++ b/hw/ppc/spapr_numa.c
@@ -37,6 +37,49 @@ static bool spapr_numa_is_symmetrical(MachineState *ms)
return true;
}
+/*
+ * This function will translate the user distances into
+ * what the kernel understand as possible values: 10
+ * (local distance), 20, 40, 80 and 160. Current heuristic
+ * is:
+ *
+ * - distances between 11 and 30 -> rounded to 20
+ * - distances between 31 and 60 -> rounded to 40
+ * - distances between 61 and 120 -> rounded to 80
+ * - everything above 120 -> 160
+ *
+ * This step can also be done in the same time as the NUMA
+ * associativity domains calculation, at the cost of extra
+ * complexity. We chose to keep it simpler.
+ *
+ * Note: this will overwrite the distance values in
+ * ms->numa_state->nodes.
+ */
+static void spapr_numa_PAPRify_distances(MachineState *ms)
+{
+int src, dst;
+int nb_numa_nodes = ms->numa_state->num_nodes;
+NodeInfo *numa_info = ms->numa_state->nodes;
+
+for (src = 0; src < nb_numa_nodes; src++) {
+for (dst = src; dst < nb_numa_nodes; dst++) {
+uint8_t distance = numa_info[src].distance[dst];
+uint8_t rounded_distance = 160;
+
+if (distance > 11 && distance < 30) {
+rounded_distance = 20;
+} else if (distance > 31 && distance < 60) {
+rounded_distance = 40;
+} else if (distance > 61 && distance < 120) {
+rounded_distance = 80;
+}
+
+numa_info[src].distance[dst] = rounded_distance;
+numa_info[dst].distance[src] = rounded_distance;
+}
+}
+}
+
void spapr_numa_associativity_init(SpaprMachineState *spapr,
MachineState *machine)
{
@@ -95,6 +138,7 @@ void spapr_numa_associativity_init(SpaprMachineState *spapr,
exit(1);
}
+spapr_numa_PAPRify_distances(machine);
}
void spapr_numa_write_associativity_dt(SpaprMachineState *spapr, void *fdt,
--
2.26.2
[PATCH 4/6] spapr_numa: change reference-points and maxdomain settings
This is the first guest visible change introduced in
spapr_numa.c. The previous settings of both reference-points
and maxdomains were too restrictive, but enough for the
existing associativity we're setting in the resources.
We'll change that in the following patches, populating the
associativity arrays based on user input. For those changes
to be effective, reference-points and maxdomains must be
more flexible. After this patch, we'll have 4 distinct
levels of NUMA (0x4, 0x3, 0x2, 0x1) and maxdomains will
allow for any type of configuration the user intends to
do - under the scope and limitations of PAPR itself, of
course.
Signed-off-by: Daniel Henrique Barboza
---
hw/ppc/spapr_numa.c | 27 ++-
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
index 180800b2f3..688391278e 100644
--- a/hw/ppc/spapr_numa.c
+++ b/hw/ppc/spapr_numa.c
@@ -222,21 +222,30 @@ int
spapr_numa_write_assoc_lookup_arrays(SpaprMachineState *spapr, void *fdt,
*/
void spapr_numa_write_rtas_dt(SpaprMachineState *spapr, void *fdt, int rtas)
{
+MachineState *ms = MACHINE(spapr);
SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
uint32_t refpoints[] = {
cpu_to_be32(0x4),
-cpu_to_be32(0x4),
+cpu_to_be32(0x3),
cpu_to_be32(0x2),
+cpu_to_be32(0x1),
};
uint32_t nr_refpoints = ARRAY_SIZE(refpoints);
-uint32_t maxdomain = cpu_to_be32(spapr->gpu_numa_id > 1 ? 1 : 0);
-uint32_t maxdomains[] = {
-cpu_to_be32(4),
-maxdomain,
-maxdomain,
-maxdomain,
-cpu_to_be32(spapr->gpu_numa_id),
-};
+uint32_t maxdomain = cpu_to_be32(ms->numa_state->num_nodes +
+ spapr->gpu_numa_id);
+uint32_t maxdomains[] = {0x4, maxdomain, maxdomain, maxdomain, maxdomain};
+
+if (spapr_machine_using_legacy_numa(spapr)) {
+refpoints[1] = cpu_to_be32(0x4);
+refpoints[2] = cpu_to_be32(0x2);
+nr_refpoints = 3;
+
+maxdomain = cpu_to_be32(spapr->gpu_numa_id > 1 ? 1 : 0);
+maxdomains[1] = maxdomain;
+maxdomains[2] = maxdomain;
+maxdomains[3] = maxdomain;
+maxdomains[4] = cpu_to_be32(spapr->gpu_numa_id);
+}
if (smc->pre_5_1_assoc_refpoints) {
nr_refpoints = 2;
--
2.26.2
[PATCH 0/6] pseries NUMA distance calculation
Hi, This series is a follow-up of the reworked pSeries NUMA code that is already merged upstream. It contains some of the patches that were presented in the first version of this work [1], some of them changed based on the reviews made back there. With this series, we're able to take user input into consideration when setting up the NUMA topology of the guest. It is still an approximation, but at least user input is not completely ignored. The changes will only be effective with pseries-5.2 and newer machines, and if more than one NUMA node is declared by the user. The idea is that we don't want to tamper with legacy guest behavior. Patch 6 has examples of how we are approximating NUMA distance via user input. The series was rebased using David's ppc-for-5.2 at 4cca31df828. Changes carried over from [1]: - patch 1 (former 4): same patch, added David's r-b - patch 2 (former 2): the check for asymetrical NUMA was moved to spapr code as requested in the review - patch 4 is a merge of former patches 5 and 6 - patch 5 (former 9): reworked - patch 6 (former 10): same patch Patch 3 is new in the series. [1] https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg03169.html Daniel Henrique Barboza (6): spapr: add spapr_machine_using_legacy_numa() helper spapr_numa: forbid asymmetrical NUMA setups spapr_numa: translate regular NUMA distance to PAPR distance spapr_numa: change reference-points and maxdomain settings spapr_numa: consider user input when defining associativity specs/ppc-spapr-numa: update with new NUMA support docs/specs/ppc-spapr-numa.rst | 213 ++ hw/ppc/spapr.c| 12 ++ hw/ppc/spapr_numa.c | 184 +++-- include/hw/ppc/spapr.h| 2 + 4 files changed, 402 insertions(+), 9 deletions(-) -- 2.26.2
[PATCH 6/6] specs/ppc-spapr-numa: update with new NUMA support
This update provides more in depth information about the
choices and drawbacks of the new NUMA support for the
spapr machine.
Signed-off-by: Daniel Henrique Barboza
---
docs/specs/ppc-spapr-numa.rst | 213 ++
1 file changed, 213 insertions(+)
diff --git a/docs/specs/ppc-spapr-numa.rst b/docs/specs/ppc-spapr-numa.rst
index e762038022..994bfb996f 100644
--- a/docs/specs/ppc-spapr-numa.rst
+++ b/docs/specs/ppc-spapr-numa.rst
@@ -189,3 +189,216 @@ QEMU up to 5.1, as follows:
This also means that user input in QEMU command line does not change the
NUMA distancing inside the guest for the pseries machine.
+
+New NUMA mechanics for pseries in QEMU 5.2
+==
+
+Starting in QEMU 5.2, the pseries machine now considers user input when
+setting NUMA topology of the guest. The following changes were made:
+
+* ibm,associativity-reference-points was changed to {0x4, 0x3, 0x2, 0x1},
allowing
+ for 4 distinct NUMA distance values based on the NUMA levels
+
+* ibm,max-associativity-domains was changed to support multiple associativity
+ domains in all NUMA levels. This is needed to ensure user flexibility
+
+* ibm,associativity for all resources now varies with user input
+
+These changes are only effective for pseries-5.2 and newer machines that are
+created with more than one NUMA node (disconsidering NUMA nodes created by
+the machine itself, e.g. NVLink 2 GPUs). The now legacy support has been
+around for such a long time, with users seeing NUMA distances 10 and 40
+(and 80 if using NVLink2 GPUs), and there is no need to disrupt the
+existing experience of those guests.
+
+To bring the user experience x86 users have when tuning up NUMA, we had
+to operate under the current pseries Linux kernel logic described in
+`How the pseries Linux guest calculates NUMA distances`_. The result
+is that we needed to translate NUMA distance user input to pseries
+Linux kernel input.
+
+Translating user distance to kernel distance
+
+
+User input for NUMA distance can vary from 10 to 254. We need to translate
+that to the values that the Linux kernel operates on (10, 20, 40, 80, 160).
+This is how it is being done:
+
+* user distance 11 to 30 will be interpreted as 20
+* user distance 31 to 60 will be interpreted as 40
+* user distance 61 to 120 will be interpreted as 80
+* user distance 121 and beyond will be interpreted as 160
+* user distance 10 stays 10
+
+The reasoning behind this aproximation is to avoid any round up to the local
+distance (10), keeping it exclusive to the 4th NUMA level (which is still
+exclusive to the node_id). All other ranges were chosen under the developer
+discretion of what would be (somewhat) sensible considering the user input.
+Any other strategy can be used here, but in the end the reality is that we'll
+have to accept that a large array of values will be translated to the same
+NUMA topology in the guest, e.g. this user input:
+
+::
+
+ 0 1 2
+ 0 10 31 120
+ 1 31 10 30
+ 2 120 30 10
+
+And this other user input:
+
+::
+
+ 0 1 2
+ 0 10 60 61
+ 1 60 10 11
+ 2 61 11 10
+
+Will both be translated to the same values internally:
+
+::
+
+ 0 1 2
+ 0 10 40 80
+ 1 40 10 20
+ 2 80 20 10
+
+Users are encouraged to use only the kernel values in the NUMA definition to
+avoid being taken by surprise with that the guest is actually seeing in the
+topology. There are enough potential surprises that are inherent to the
+associativity domain assignment process, discussed below.
+
+
+How associativity domains are assigned
+--
+
+LOPAPR allows more than one associativity array (or 'string') per allocated
+resource. This would be used to represent that the resource has multiple
+connections with the board, and then the operational system, when deciding
+NUMA distancing, should consider the associativity information that provides
+the shortest distance.
+
+The spapr implementation does not support multiple associativity arrays per
+resource, neither does the pseries Linux kernel. We'll have to represent the
+NUMA topology using one associativity per resource, which means that choices
+and compromises are going to be made.
+
+Consider the following NUMA topology entered by user input:
+
+::
+
+ 0 1 2 3
+ 0 10 20 20 40
+ 1 20 10 80 40
+ 2 20 80 10 20
+ 3 40 40 20 10
+
+Honoring just the relative distances of node 0 to every other node, one
possible
+value for all associativity arrays would be:
+
+* node 0: 0 B A 0
+* node 1: 0 0 A 1
+* node 2: 0 0 A 2
+* node 3: 0 B 0 3
+
+With the reference points {0x4, 0x3, 0x2, 0x1}, for node 0:
+
+* distance from 0 to 1 is 20 (no match at 0x4, will match at 0x3)
+* distance from 0 to 2 is 20 (no match at 0x4, will match at 0x3)
+* distance from 0 to 3 is 40 (no match at 0x4 and 0x3, will match
+ at 0x2)
+
+The distances related to
[PATCH 2/6] spapr_numa: forbid asymmetrical NUMA setups
The pSeries machine does not support asymmetrical NUMA
configurations. This doesn't make much of a different
since we're not using user input for pSeries NUMA setup,
but this will change in the next patches.
To avoid breaking existing setups, gate this change by
checking for legacy NUMA support.
Signed-off-by: Daniel Henrique Barboza
---
hw/ppc/spapr_numa.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
index 64fe567f5d..36aaa273ee 100644
--- a/hw/ppc/spapr_numa.c
+++ b/hw/ppc/spapr_numa.c
@@ -19,6 +19,24 @@
/* Moved from hw/ppc/spapr_pci_nvlink2.c */
#define SPAPR_GPU_NUMA_ID (cpu_to_be32(1))
+static bool spapr_numa_is_symmetrical(MachineState *ms)
+{
+int src, dst;
+int nb_numa_nodes = ms->numa_state->num_nodes;
+NodeInfo *numa_info = ms->numa_state->nodes;
+
+for (src = 0; src < nb_numa_nodes; src++) {
+for (dst = src; dst < nb_numa_nodes; dst++) {
+if (numa_info[src].distance[dst] !=
+numa_info[dst].distance[src]) {
+return false;
+}
+}
+}
+
+return true;
+}
+
void spapr_numa_associativity_init(SpaprMachineState *spapr,
MachineState *machine)
{
@@ -61,6 +79,22 @@ void spapr_numa_associativity_init(SpaprMachineState *spapr,
spapr->numa_assoc_array[i][MAX_DISTANCE_REF_POINTS] = cpu_to_be32(i);
}
+
+/*
+ * Legacy NUMA guests (pseries-5.1 and order, or guests with only
+ * 1 NUMA node) will not benefit from anything we're going to do
+ * after this point.
+ */
+if (spapr_machine_using_legacy_numa(spapr)) {
+return;
+}
+
+if (!spapr_numa_is_symmetrical(machine)) {
+error_report("Asymmetrical NUMA topologies aren't supported "
+ "in the pSeries machine");
+exit(1);
+}
+
}
void spapr_numa_write_associativity_dt(SpaprMachineState *spapr, void *fdt,
--
2.26.2
Re: [PATCH v2 13/38] qapi/common.py: add type hint annotations
On Tue, Sep 22, 2020 at 05:00:36PM -0400, John Snow wrote: > Annotations do not change runtime behavior. > This commit *only* adds annotations. > > Signed-off-by: John Snow Reviewed-by: Cleber Rosa signature.asc Description: PGP signature
Re: [PATCH 02/16] qapi/expr.py: Check for dict instead of OrderedDict
On Tue, Sep 22, 2020 at 05:12:59PM -0400, John Snow wrote: > OrderedDict is a subtype of dict, so we can check for a more general form. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
Re: [PATCH 01/16] qapi/expr.py: Remove 'info' argument from nested check_if_str
On Tue, Sep 22, 2020 at 05:12:58PM -0400, John Snow wrote: > The function can just use the argument from the scope above. Otherwise, > we get shadowed argument errors because the parameter name clashes with > the name of a variable already in-scope. > > Signed-off-by: John Snow Reviewed-by: Eduardo Habkost -- Eduardo
