Re: [PATCH v5 4/4] colo: Update Documentation for continuous replication
On Thu, 26 Sep 2019 17:27:35 + "Zhang, Chen" wrote: > > -Original Message- > > From: Lukas Straub > > Sent: Monday, September 16, 2019 3:20 AM > > To: qemu-devel > > Cc: Zhang, Chen ; Jason Wang > > ; Wen Congyang ; > > Xie Changlong ; kw...@redhat.com; > > mre...@redhat.com > > Subject: [PATCH v5 4/4] colo: Update Documentation for continuous > > replication > > > > Document the qemu command-line and qmp commands for continuous > > replication > > > > Signed-off-by: Lukas Straub > > --- > > docs/COLO-FT.txt | 212 +++-- > > docs/block-replication.txt | 28 +++-- > > 2 files changed, 173 insertions(+), 67 deletions(-) > > > > diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt index > > ad24680d13..e98c95ca2b 100644 > > --- a/docs/COLO-FT.txt > > +++ b/docs/COLO-FT.txt > > @@ -145,35 +145,65 @@ The diagram just shows the main qmp command, > > you can get the detail in test procedure. > > > > == Test procedure == > > -1. Startup qemu > > -Primary: > > -# qemu-system-x86_64 -accel kvm -m 2048 -smp 2 -qmp stdio -name > > primary \ > > - -device piix3-usb-uhci -vnc :7 \ > > - -device usb-tablet -netdev tap,id=hn0,vhost=off \ > > - -device virtio-net-pci,id=net-pci0,netdev=hn0 \ > > - -drive if=virtio,id=primary-disk0,driver=quorum,read-pattern=fifo,vote- > > threshold=1,\ > > - children.0.file.filename=1.raw,\ > > - children.0.driver=raw -S > > -Secondary: > > -# qemu-system-x86_64 -accel kvm -m 2048 -smp 2 -qmp stdio -name > > secondary \ > > - -device piix3-usb-uhci -vnc :7 \ > > - -device usb-tablet -netdev tap,id=hn0,vhost=off \ > > - -device virtio-net-pci,id=net-pci0,netdev=hn0 \ > > - -drive if=none,id=secondary-disk0,file.filename=1.raw,driver=raw,node- > > name=node0 \ > > - -drive if=virtio,id=active-disk0,driver=replication,mode=secondary,\ > > - file.driver=qcow2,top-id=active-disk0,\ > > - file.file.filename=/mnt/ramfs/active_disk.img,\ > > - file.backing.driver=qcow2,\ > > - file.backing.file.filename=/mnt/ramfs/hidden_disk.img,\ > > - file.backing.backing=secondary-disk0 \ > > - -incoming tcp:0: > > - > > -2. On Secondary VM's QEMU monitor, issue command > > +Note: Here we are running both instances on the same Machine for > > +testing, change the IP Addresses if you want to run it on two Hosts > > + > > +== Startup qemu == > > +1. Primary: > > +# imagefolder="/mnt/vms/colo-test" > > + > > +The disks for the primary and secondary need to be the same before > > +starting colo # cp --reflink=auto $imagefolder/primary.qcow2 > > +$imagefolder/primary-copy.qcow2 > > + > > +# qemu-system-x86_64 -enable-kvm -cpu qemu64,+kvmclock -m 512 -smp > > 1 -qmp stdio \ > > + -vnc :0 -device piix3-usb-uhci -device usb-tablet -name primary \ > > + -netdev tap,id=hn0,vhost=off,helper=/usr/lib/qemu/qemu-bridge-helper > > \ > > + -device rtl8139,id=e0,netdev=hn0 \ > > + -chardev socket,id=mirror0,host=127.0.0.1,port=9003,server,nowait \ > > + -chardev socket,id=compare1,host=127.0.0.1,port=9004,server,wait \ > > + -chardev socket,id=compare0,host=127.0.0.1,port=9001,server,nowait \ > > + -chardev socket,id=compare0-0,host=127.0.0.1,port=9001 \ > > + -chardev socket,id=compare_out,host=127.0.0.1,port=9005,server,nowait > > \ > > + -chardev socket,id=compare_out0,host=127.0.0.1,port=9005 \ > > + -object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0 \ > > + -object filter- > > redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out \ > > + -object filter- > > redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0 \ > > + -object iothread,id=iothread1 \ > > + -object > > +colo-compare,id=comp0,primary_in=compare0- > > 0,secondary_in=compare1,\ > > +outdev=compare_out0,iothread=iothread1 \ > > + -drive > > +if=ide,id=colo-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,\ > > +children.0.file.filename=$imagefolder/primary.qcow2,children.0.driver=q > > +cow2 -S > > + > > +2. Secondary: > > +# imagefolder="/mnt/vms/colo-test" > > + > > +# qemu-img create -f qcow2 $imagefolder/secondary-active.qcow2 10G > > + > > +# qemu-img create -f qcow2 $imagefolder/secondary-hidden.qcow2 10G > > + > > +# qemu-system-x86_64 -enable-kvm -cpu qemu64,+kvmclock -m 512 -smp > > 1 -qmp stdio \ > > + -vnc :1 -device piix3-usb-uhci -device usb-tablet -name secondary \ > > + -netdev tap,id=hn0,vhost=off,helper=/usr/lib/qemu/qemu-bridge-helper > > \ > > + -device rtl8139,id=e0,netdev=hn0 \ > > + -chardev socket,id=red0,host=127.0.0.1,port=9003,reconnect=1 \ > > + -chardev socket,id=red1,host=127.0.0.1,port=9004,reconnect=1 \ > > + -object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0 \ > > + -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1 \ > > + -object filter-rewriter,id=rew0,netdev=hn0,queue=all \ > > + -drive if=none,id=parent0,file.filename=$imagefolder/primary- > > copy.qcow2,driver=qcow2 \ > > + -drive > > +if=non
Re: [PATCH v5 2/4] tests/test-replication.c: Add test for ignoring requests after failover
On Thu, 26 Sep 2019 17:40:03 +
"Zhang, Chen" wrote:
> > -Original Message-
> > From: Lukas Straub
> > Sent: Monday, September 16, 2019 3:20 AM
> > To: qemu-devel
> > Cc: Zhang, Chen ; Jason Wang
> > ; Wen Congyang ;
> > Xie Changlong ; kw...@redhat.com;
> > mre...@redhat.com
> > Subject: [PATCH v5 2/4] tests/test-replication.c: Add test for ignoring
> > requests after failover
> >
> > This simulates the case that happens when we resume COLO after failover.
> >
>
> It looks change the title to "Add test for secondary node continuous backup"
> is better.
Did you mean "continuous replication"? Would "Add test for secondary node
continuing replication" be Ok?
> > Signed-off-by: Lukas Straub
> > ---
> > tests/test-replication.c | 52
> >
> > 1 file changed, 52 insertions(+)
> >
> > diff --git a/tests/test-replication.c b/tests/test-replication.c index
> > f085d1993a..5addfc2227 100644
> > --- a/tests/test-replication.c
> > +++ b/tests/test-replication.c
> > @@ -489,6 +489,56 @@ static void test_secondary_stop(void)
> > teardown_secondary();
> > }
> >
> > +static void test_secondary_failover_then_ignore_requests(void)
>
> Same as title, I think change to "test_secondary_continuous_backup" is more
> clear.
>
> Thanks
> Zhang Chen
>
> > +{
> > +BlockBackend *top_blk, *local_blk;
> > +Error *local_err = NULL;
> > +
> > +top_blk = start_secondary();
> > +replication_start_all(REPLICATION_MODE_SECONDARY, &local_err);
> > +g_assert(!local_err);
> > +
> > +/* write 0x22 to s_local_disk (IMG_SIZE / 2, IMG_SIZE) */
> > +local_blk = blk_by_name(S_LOCAL_DISK_ID);
> > +test_blk_write(local_blk, 0x22, IMG_SIZE / 2, IMG_SIZE / 2, false);
> > +
> > +/* replication will backup s_local_disk to s_hidden_disk */
> > +test_blk_read(top_blk, 0x11, IMG_SIZE / 2,
> > + IMG_SIZE / 2, 0, IMG_SIZE, false);
> > +
> > +/* write 0x33 to s_active_disk (0, IMG_SIZE / 2) */
> > +test_blk_write(top_blk, 0x33, 0, IMG_SIZE / 2, false);
> > +
> > +/* do failover (active commit) */
> > +replication_stop_all(true, &local_err);
> > +g_assert(!local_err);
> > +
> > +/* it should ignore all requests from now on */
> > +
> > +/* start after failover */
> > +replication_start_all(REPLICATION_MODE_PRIMARY, &local_err);
> > +g_assert(!local_err);
> > +
> > +/* checkpoint */
> > +replication_do_checkpoint_all(&local_err);
> > +g_assert(!local_err);
> > +
> > +/* stop */
> > +replication_stop_all(true, &local_err);
> > +g_assert(!local_err);
> > +
> > +/* read from s_local_disk (0, IMG_SIZE / 2) */
> > +test_blk_read(top_blk, 0x33, 0, IMG_SIZE / 2,
> > + 0, IMG_SIZE / 2, false);
> > +
> > +
> > +/* read from s_local_disk (IMG_SIZE / 2, IMG_SIZE) */
> > +test_blk_read(top_blk, 0x22, IMG_SIZE / 2,
> > + IMG_SIZE / 2, 0, IMG_SIZE, false);
> > +
> > +teardown_secondary();
> > +}
> > +
> > static void test_secondary_do_checkpoint(void)
> > {
> > BlockBackend *top_blk, *local_blk;
> > @@ -584,6 +634,8 @@ int main(int argc, char **argv)
> > g_test_add_func("/replication/secondary/write", test_secondary_write);
> > g_test_add_func("/replication/secondary/start", test_secondary_start);
> > g_test_add_func("/replication/secondary/stop", test_secondary_stop);
> > +
> > g_test_add_func("/replication/secondary/failover_then_ignore_requests",
> > +test_secondary_failover_then_ignore_requests);
> > g_test_add_func("/replication/secondary/do_checkpoint",
> > test_secondary_do_checkpoint);
> > g_test_add_func("/replication/secondary/get_error_all",
> > --
> > 2.20.1
>
Re: [PATCH v5 3/4] net/filter.c: Add Options to insert filters anywhere in the filter list
On Thu, 26 Sep 2019 17:02:58 +
"Zhang, Chen" wrote:
> > diff --git a/qemu-options.hx b/qemu-options.hx index
> > 08749a3391..23fa5a344e 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -4368,7 +4368,7 @@ applications, they can do this through this
> > parameter. Its format is a gnutls priority string as described at
> > @url{https://gnutls.org/manual/html_node/Priority-Strings.html}.
> >
> > -@item -object filter-
> > buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{
> > all|rx|tx}][,status=@var{on|off}]
> > +@item -object
> > +filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue
> > +=@var{all|rx|tx}][,status=@var{on|off}][,position=@var{head|tail|id= > +>}][,insert=@var{behind|before}]
> >
> > Interval @var{t} can't be 0, this filter batches the packet delivery: all
> > packets
> > arriving in a given interval on netdev @var{netdevid} are delayed @@ -
> > 4387,11 +4387,11 @@ queue @var{all|rx|tx} is an option that can be applied
> > to any netfilter.
> > @option{tx}: the filter is attached to the transmit queue of the netdev,
> > where it will receive packets sent by the netdev.
> >
> > -@item -object filter-
> > mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},queue
> > =@var{all|rx|tx}[,vnet_hdr_support]
> > +@item -object
> > +filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},
> > +queue=@var{all|rx|tx}[,vnet_hdr_support][,position=@var{head|tail|id=<
> > i
> > +d>}][,insert=@var{behind|before}]
> >
> > filter-mirror on netdev @var{netdevid},mirror net packet to
> > chardev@var{chardevid}, if it has the vnet_hdr_support flag, filter-mirror
> > will
> > mirror packet with vnet_hdr_len.
> >
>
> Please add description for the newly added parameter in each filter.
> After that:
> Reviewed-by: Zhang Chen
>
> Thanks
> Zhang Chen
Hi,
I will add a single description like its currently done with the "queue"
option, noting that
it applies to any netfilter. Is that Ok?
Regards,
Lukas Straub
>
> > -@item -object filter-
> > redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid},out
> > dev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support]
> > +@item -object
> > +filter-redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevi
> > +d},outdev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support][,p
> > os
> > +ition=@var{head|tail|id=}][,insert=@var{behind|before}]
> >
> > filter-redirector on netdev @var{netdevid},redirect filter's net packet to
> > chardev @var{chardevid},and redirect indev's packet to filter.if it has the
> > vnet_hdr_support flag, @@ -4400,7 +4400,7 @@ Create a filter-redirector
> > we need to differ outdev id from indev id, id can not be the same. we can
> > just use indev or outdev, but at least one of indev or outdev need to be
> > specified.
> >
> > -@item -object filter-
> > rewriter,id=@var{id},netdev=@var{netdevid},queue=@var{all|rx|tx},[vnet_
> > hdr_support]
> > +@item -object
> > +filter-rewriter,id=@var{id},netdev=@var{netdevid},queue=@var{all|rx|tx}
> > +,[vnet_hdr_support][,position=@var{head|tail|id=}][,insert=@var{beh
> > +ind|before}]
> >
> > Filter-rewriter is a part of COLO project.It will rewrite tcp packet to
> > secondary from primary to keep secondary tcp connection,and rewrite @@ -
> > 4413,7 +4413,7 @@ colo secondary:
> > -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
> > -object filter-rewriter,id=rew0,netdev=hn0,queue=all
> >
> > -@item -object filter-
> > dump,id=@var{id},netdev=@var{dev}[,file=@var{filename}][,maxlen=@var{
> > len}]
> > +@item -object
> > +filter-
> > dump,id=@var{id},netdev=@var{dev}[,file=@var{filename}][,maxlen=
> > +@var{len}][,position=@var{head|tail|id=}][,insert=@var{behind|befor
> > +e}]
> >
> > Dump the network traffic on netdev @var{dev} to the file specified by
> > @var{filename}. At most @var{len} bytes (64k by default) per packet are
> > stored.
> > --
> > 2.20.1
>
Re: [PATCH v2 01/20] target/mips: Clean up helper.c
"Aleksandar Markovic" writes: > OK, my case belongs to "used to work before" group. I used GMail > Android app to send this particular mail, and have been using that app > for several months without problema. I am going to find an alternative > way of sending mails to the list. Appreciated! [...]
Re: target/ppc: bug in optimised vsl/vsr implementation?
26.09.2019. 20.14, "Mark Cave-Ayland" је написао/ла: > > As part of the investigation into the DFP number issue reported at > https://bugs.launchpad.net/qemu/+bug/1841990 it appears that there may also be a bug > introduced by the new optimised vsl/vsr implementation: > > commit 4e6d0920e7547e6af4bbac5ffe9adfe6ea621822 > Author: Stefan Brankovic > Date: Mon Jul 15 16:22:48 2019 +0200 > > target/ppc: Optimize emulation of vsl and vsr instructions > > (sorry in advance if the format of this message looks odd, I have some problems with mail settings related to recent qemu-devel mailer settings changes; I will adjust my mail settings in next few days) Mark and Paul (and Stefan), Thanks for spotting this and pinpointing the culprit commit. I guess Stefan is going to respond soon, but, in the meantime, I took a look at the commit in question: https://github.com/qemu/qemu/commit/4e6d0920e7547e6af4bbac5ffe9adfe6ea621822 I don't have at the moment any dev/test environment handy, but I did manual inspection of the code, and here is what I found (in order of importance, perceived by me): 1. The code will not work correctly if the shift ammount (variable 'sh') is 0. This is because, in that case, one of succeeding invocations of TCG shift functions will be required to shift a 64-bit TCG variable by 64 bits, and the result of such TCG operation is undefined (shift amount must be 63 or less) - see https://github.com/qemu/qemu/blob/master/tcg/README. 2. Variable naming is better in the old helper than in the new translator. In that light, I would advise Stefan to change 'sh' to 'shift', and 'shifted' to 'carry'. 3. Lines tcg_gen_andi_i64(shifted, shifted, 0x7fULL); and tcg_gen_andi_i64(shifted, shifted, 0xfe00ULL); appear to be spurious (albait in a harmless way). Therefore, they should be deleted, or, alternatevely, a justification for them should be provided. 4. In the commit message, variable names were used without quotation mark, resulting in puzzling and unclear wording. 5. (a question for Mark) After all recent changes, does get_avr64(..., ..., true) always (for any endian configuration) return the "high" half of an Altivec register, and get_avr64(..., ..., false) the "low" one? Given all these circumstances, perhaps the most reasonable solution would be to revert the commit in question, and allow Stefan enough dev and test time to hopefully submit a new, better, version later on. Sincerely, Aleksandar
Re: [PATCH v25 00/22] Add RX archtecture support
Patchew URL:
https://patchew.org/QEMU/20190927062302.110144-1-ys...@users.sourceforge.jp/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20190927062302.110144-1-ys...@users.sourceforge.jp
Subject: [PATCH v25 00/22] Add RX archtecture support
=== 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 ===
Switched to a new branch 'test'
1a74ffc qapi/machine.json: Add RX cpu.
c173e68 BootLinuxConsoleTest: Test the RX-Virt machine
b4e9a7a Add rx-softmmu
5bdc07e hw/rx: Restrict the RX62N microcontroller to the RX62N CPU core
2496aa4 hw/rx: Honor -accel qtest
b97a84d hw/rx: RX Target hardware definition
f806e27 hw/char: RX62N serial communication interface (SCI)
bd59212 hw/timer: RX62N internal timer modules
cb22efa hw/intc: RX62N interrupt controller (ICUa)
ae43531 target/rx: Dump bytes for each insn during disassembly
26dd1a2 target/rx: Collect all bytes during disassembly
18aeea2 target/rx: Emit all disassembly in one prt()
17d9d21 target/rx: Use prt_ldmi for XCHG_mr disassembly
2882571 target/rx: Replace operand with prt_ldmi in disassembler
bf3b3f2 target/rx: Disassemble rx_index_addr into a string
73102ce target/rx: RX disassembler
90cb197 target/rx: CPU definition
5c00dbb target/rx: TCG helper
2b15272 target/rx: TCG translation
15f28f1 hw/registerfields.h: Add 8bit and 16bit register macros
fa571c7 qemu/bitops.h: Add extract8 and extract16
15bf923 MAINTAINERS: Add RX
=== OUTPUT BEGIN ===
1/22 Checking commit 15bf92348a56 (MAINTAINERS: Add RX)
2/22 Checking commit fa571c7ef6af (qemu/bitops.h: Add extract8 and extract16)
3/22 Checking commit 15f28f1299de (hw/registerfields.h: Add 8bit and 16bit
register macros)
Use of uninitialized value in concatenation (.) or string at
./scripts/checkpatch.pl line 2484.
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#27: FILE: include/hw/registerfields.h:25:
+#define REG8(reg, addr) \
+enum { A_ ## reg = (addr) }; \
+enum { R_ ## reg = (addr) };
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#31: FILE: include/hw/registerfields.h:29:
+#define REG16(reg, addr) \
+enum { A_ ## reg = (addr) }; \
+enum { R_ ## reg = (addr) / 2 };
total: 2 errors, 0 warnings, 56 lines checked
Patch 3/22 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/22 Checking commit 2b1527203652 (target/rx: TCG translation)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#20:
new file mode 100644
total: 0 errors, 1 warnings, 3065 lines checked
Patch 4/22 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/22 Checking commit 5c00dbb5e309 (target/rx: TCG helper)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#21:
new file mode 100644
total: 0 errors, 1 warnings, 650 lines checked
Patch 5/22 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/22 Checking commit 90cb19738c00 (target/rx: CPU definition)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#38:
new file mode 100644
total: 0 errors, 1 warnings, 588 lines checked
Patch 6/22 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/22 Checking commit 73102ce10f6a (target/rx: RX disassembler)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#38:
new file mode 100644
total: 0 errors, 1 warnings, 1497 lines checked
Patch 7/22 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
8/22 Checking commit bf3b3f236e0b (target/rx: Disassemble rx_index_addr into a
string)
9/22 Checking commit 2882571a240d (target/rx: Replace operand with prt_ldmi in
disassembler)
10/22 Checking commit 17d9d217fe49 (target/rx: Use prt_ldmi for XCHG_mr
disassembly)
11/22 Checking commit 18aeea285a8b (target/rx: Emit all disassembly in one
prt())
12/22 Checking commit 26dd1a2ced54 (target/rx: Collect all bytes during
disassembly)
13/22 Checking commit ae43531ca84d (target/rx: Dump bytes for each insn during
disassembly)
14/22 Checking commit cb22efa8a743 (hw/intc: RX62N interrupt controller (ICUa))
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#40:
new fil
[Bug 1845580] Re: issue with QEMU on Raspberry Pi failing to access CDROM
I compiled 4.1.0 on the Raspberry, everything seemed to compile. I have verified with qemu-system-i386 --version displays 4.1.0 Using the below basic command line: qemu-system-i386 -hda c.hda -cdrom FD12CD.iso -boot order=d now when I try to execute the qemu I get only below as it just seems to hang: WARNING: Image format was not specified for 'c.hda' and probing guessed raw. Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted. Specify the 'raw' format explicitly to remove the restrictions. VNC server running on ::1:5901 -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1845580 Title: issue with QEMU on Raspberry Pi failing to access CDROM Status in QEMU: New Bug description: I am trying to access the CDROM (iso) from QEMU using FreeDOS and I get an error when doing a directory for: i can boot from the iso but if i exit to access the files from the CDROM ISO i get the attached error. I believe there is an issue with the QEMU for the Raspberry Pi. I am using a Raspberry Pi3 with the latest full Raspbian Buster load To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1845580/+subscriptions
[PULL 02/27] qapi: Tighten QAPISchemaFOO.check() assertions
When we introduced the QAPISchema intermediate representation (commit
ac88219a6c7), we took a shortcut: we left check_exprs() & friends
alone instead of moving semantic checks into the
QAPISchemaFOO.check(). check_exprs() still checks and reports errors,
and the .check() assert check_exprs() did the job. There are a few
gaps, though.
QAPISchemaArrayType.check() neglects to assert the element type is not
an array. Add the assertion.
QAPISchemaObjectTypeVariants.check() neglects to assert the tag member
is not optional. Add the assertion.
It neglects to assert the tag member is not conditional. Add the
assertion.
It neglects to assert we actually have variants. Add the assertion.
It asserts the variants are object types, but neglects to assert they
don't have variants. Tighten the assertion.
QAPISchemaObjectTypeVariants.check_clash() has the same issue.
However, it can run only after .check(). Delete the assertion instead
of tightening it.
QAPISchemaAlternateType.check() neglects to assert the branch types
don't conflict. Fixing that isn't trivial, so add just a TODO comment
for now. It'll be resolved later in this series.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-2-arm...@redhat.com>
---
scripts/qapi/common.py | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index b00caacca3..155b87b825 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1362,6 +1362,7 @@ class QAPISchemaArrayType(QAPISchemaType):
QAPISchemaType.check(self, schema)
self.element_type = schema.lookup_type(self._element_type_name)
assert self.element_type
+assert not isinstance(self.element_type, QAPISchemaArrayType)
@property
def ifcond(self):
@@ -1606,6 +1607,8 @@ class QAPISchemaObjectTypeVariants(object):
self.tag_member = seen[c_name(self._tag_name)]
assert self._tag_name == self.tag_member.name
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+assert not self.tag_member.optional
+assert self.tag_member.ifcond == []
if self._tag_name:# flat union
# branches that are not explicitly covered get an empty type
cases = set([v.name for v in self.variants])
@@ -1615,20 +1618,21 @@ class QAPISchemaObjectTypeVariants(object):
m.ifcond)
v.set_owner(self.tag_member.owner)
self.variants.append(v)
+assert self.variants
for v in self.variants:
v.check(schema)
# Union names must match enum values; alternate names are
# checked separately. Use 'seen' to tell the two apart.
if seen:
assert v.name in self.tag_member.type.member_names()
-assert isinstance(v.type, QAPISchemaObjectType)
+assert (isinstance(v.type, QAPISchemaObjectType)
+and not v.type.variants)
v.type.check(schema)
def check_clash(self, info, seen):
for v in self.variants:
# Reset seen map for each variant, since qapi names from one
# branch do not affect another branch
-assert isinstance(v.type, QAPISchemaObjectType)
v.type.check_clash(info, dict(seen))
@@ -1659,6 +1663,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
seen = {}
for v in self.variants.variants:
v.check_clash(self.info, seen)
+# TODO check conflicting qtypes
if self.doc:
self.doc.connect_member(v)
if self.doc:
--
2.21.0
[PULL 01/27] qmp-dispatch: Use CommandNotFound error for disabled commands
From: Michal Privoznik
If a command is disabled an error is reported. But due to usage of
error_setg() the class of the error is GenericError which does not
help callers in distinguishing this case from a case where a qmp
command fails regularly due to other reasons.
We used to use class CommandDisabled until the great error
simplification (commit de253f1491 for QMP and commit 93b91c59db for
qemu-ga, both v1.2.0).
Use CommandNotFound error class, which is close enough.
Signed-off-by: Michal Privoznik
Message-Id:
Reviewed-by: Eric Blake
[Test update squashed in, commit message tweaked]
Signed-off-by: Markus Armbruster
---
qapi/qmp-dispatch.c | 5 +++--
tests/test-qga.c| 4 ++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 3037d353a4..bc264b3c9b 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -104,8 +104,9 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds,
QObject *request,
return NULL;
}
if (!cmd->enabled) {
-error_setg(errp, "The command %s has been disabled for this instance",
- command);
+error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+ "The command %s has been disabled for this instance",
+ command);
return NULL;
}
if (oob && !(cmd->options & QCO_ALLOW_OOB)) {
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 891aa3d322..1ca49bbced 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -668,7 +668,7 @@ static void test_qga_blacklist(gconstpointer data)
error = qdict_get_qdict(ret, "error");
class = qdict_get_try_str(error, "class");
desc = qdict_get_try_str(error, "desc");
-g_assert_cmpstr(class, ==, "GenericError");
+g_assert_cmpstr(class, ==, "CommandNotFound");
g_assert_nonnull(g_strstr_len(desc, -1, "has been disabled"));
qobject_unref(ret);
@@ -677,7 +677,7 @@ static void test_qga_blacklist(gconstpointer data)
error = qdict_get_qdict(ret, "error");
class = qdict_get_try_str(error, "class");
desc = qdict_get_try_str(error, "desc");
-g_assert_cmpstr(class, ==, "GenericError");
+g_assert_cmpstr(class, ==, "CommandNotFound");
g_assert_nonnull(g_strstr_len(desc, -1, "has been disabled"));
qobject_unref(ret);
--
2.21.0
[PULL 14/27] qapi: Make check_type()'s array case a bit more obvious
check_type() checks the array's contents, then peels off the array and falls through to the "not array" code without resetting allow_array and allow_dict to False. Works because the peeled value is a string, and allow_array and allow_dict aren't used then. Tidy up anyway: recurse instead, defaulting allow_array and allow_dict to False. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-14-arm...@redhat.com> --- scripts/qapi/common.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 111a4bbe55..870d8b0ecb 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -795,7 +795,8 @@ def check_type(value, info, source, raise QAPISemError(info, "%s: array type must contain single type name" % source) -value = value[0] +check_type(value[0], info, source, allow_metas=allow_metas) +return # Check if type name for value is okay if isinstance(value, str): -- 2.21.0
[PULL 10/27] qapi: Improve reporting of invalid name errors
Split check_name() into check_name_is_str() and check_name_str(), keep
check_name() as a wrapper.
Move add_name()'s call into its caller check_exprs(), and inline.
This permits delaying check_name_str() there, so its error message
gains an "in definition" line.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-10-arm...@redhat.com>
---
scripts/qapi/common.py | 20
tests/qapi-schema/bad-ident.err | 1 +
tests/qapi-schema/command-int.err| 1 +
tests/qapi-schema/redefined-builtin.err | 1 +
tests/qapi-schema/redefined-command.err | 1 +
tests/qapi-schema/redefined-event.err| 1 +
tests/qapi-schema/redefined-type.err | 1 +
tests/qapi-schema/reserved-command-q.err | 1 +
tests/qapi-schema/reserved-type-kind.err | 1 +
tests/qapi-schema/reserved-type-list.err | 1 +
10 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index c909821560..6f35cd131e 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -708,11 +708,22 @@ valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
def check_name(name, info, source,
allow_optional=False, enum_member=False, permit_upper=False):
+check_name_is_str(name, info, source)
+check_name_str(name, info, source,
+ allow_optional, enum_member, permit_upper)
+
+
+def check_name_is_str(name, info, source):
+if not isinstance(name, str):
+raise QAPISemError(info, "%s requires a string name" % source)
+
+
+def check_name_str(name, info, source,
+ allow_optional=False, enum_member=False,
+ permit_upper=False):
global valid_name
membername = name
-if not isinstance(name, str):
-raise QAPISemError(info, "%s requires a string name" % source)
if name.startswith('*'):
membername = name[1:]
if not allow_optional:
@@ -734,7 +745,6 @@ def check_name(name, info, source,
def add_name(name, info, meta):
global all_names
-check_name(name, info, "'%s'" % meta, permit_upper=True)
# FIXME should reject names that differ only in '_' vs. '.'
# vs. '-', because they're liable to clash in generated C.
if name in all_names:
@@ -1153,8 +1163,10 @@ def check_exprs(exprs):
raise QAPISemError(info, "expression is missing metatype")
normalize_if(expr)
name = expr[meta]
-add_name(name, info, meta)
+check_name_is_str(name, info, "'%s'" % meta)
info.set_defn(meta, name)
+check_name_str(name, info, "'%s'" % meta, permit_upper=True)
+add_name(name, info, meta)
if doc and doc.symbol != name:
raise QAPISemError(
info,
diff --git a/tests/qapi-schema/bad-ident.err b/tests/qapi-schema/bad-ident.err
index c4190602b5..6878889854 100644
--- a/tests/qapi-schema/bad-ident.err
+++ b/tests/qapi-schema/bad-ident.err
@@ -1 +1,2 @@
+tests/qapi-schema/bad-ident.json: In struct '*oops':
tests/qapi-schema/bad-ident.json:2: 'struct' does not allow optional name
'*oops'
diff --git a/tests/qapi-schema/command-int.err
b/tests/qapi-schema/command-int.err
index 0f9300679b..56b45bf656 100644
--- a/tests/qapi-schema/command-int.err
+++ b/tests/qapi-schema/command-int.err
@@ -1 +1,2 @@
+tests/qapi-schema/command-int.json: In command 'int':
tests/qapi-schema/command-int.json:2: built-in 'int' is already defined
diff --git a/tests/qapi-schema/redefined-builtin.err
b/tests/qapi-schema/redefined-builtin.err
index b2757225c4..67775fdb41 100644
--- a/tests/qapi-schema/redefined-builtin.err
+++ b/tests/qapi-schema/redefined-builtin.err
@@ -1 +1,2 @@
+tests/qapi-schema/redefined-builtin.json: In struct 'size':
tests/qapi-schema/redefined-builtin.json:2: built-in 'size' is already defined
diff --git a/tests/qapi-schema/redefined-command.err
b/tests/qapi-schema/redefined-command.err
index 82ae256e63..b77a05d354 100644
--- a/tests/qapi-schema/redefined-command.err
+++ b/tests/qapi-schema/redefined-command.err
@@ -1 +1,2 @@
+tests/qapi-schema/redefined-command.json: In command 'foo':
tests/qapi-schema/redefined-command.json:3: command 'foo' is already defined
diff --git a/tests/qapi-schema/redefined-event.err
b/tests/qapi-schema/redefined-event.err
index 35429cb481..fd02d38157 100644
--- a/tests/qapi-schema/redefined-event.err
+++ b/tests/qapi-schema/redefined-event.err
@@ -1 +1,2 @@
+tests/qapi-schema/redefined-event.json: In event 'EVENT_A':
tests/qapi-schema/redefined-event.json:3: event 'EVENT_A' is already defined
diff --git a/tests/qapi-schema/redefined-type.err
b/tests/qapi-schema/redefined-type.err
index 06ea78c478..89acc82c2d 100644
--- a/tests/qapi-schema/redefined-type.err
+++ b/tests/qapi-schema/redefined-type.err
@@ -1 +1,2 @@
+tests/qapi-schema/redefined-type.json: In enum 'foo':
tests/qapi-schema/redefined-type.json:3: struct 'foo' is already defined
diff --git a/tests/qa
[PULL 04/27] qapi: New QAPISourceInfo, replacing dict
We track source locations with a dict of the form
{'file': FNAME, 'line': LINENO, 'parent': PARENT}
where PARENT is None for the main file, and the include directive's
source location for included files.
This is serviceable enough, but the next commit will add information,
and that's going to come out cleaner if we turn this into a class. So
do that.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-4-arm...@redhat.com>
---
scripts/qapi/common.py | 69 +-
1 file changed, 41 insertions(+), 28 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index bfb3e8a493..5843f3eeb2 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -13,6 +13,7 @@
from __future__ import print_function
from contextlib import contextmanager
+import copy
import errno
import os
import re
@@ -53,34 +54,50 @@ struct_types = {}
union_types = {}
all_names = {}
+
#
# Parsing the schema into expressions
#
+class QAPISourceInfo(object):
+def __init__(self, fname, line, parent):
+self.fname = fname
+self.line = line
+self.parent = parent
-def error_path(parent):
-res = ''
-while parent:
-res = ('In file included from %s:%d:\n' % (parent['file'],
- parent['line'])) + res
-parent = parent['parent']
-return res
+def next_line(self):
+info = copy.copy(self)
+info.line += 1
+return info
+
+def loc(self):
+return '%s:%d' % (self.fname, self.line)
+
+def include_path(self):
+ret = ''
+parent = self.parent
+while parent:
+ret = 'In file included from %s:\n' % parent.loc() + ret
+parent = parent.parent
+return ret
+
+def __str__(self):
+return self.include_path() + self.loc()
class QAPIError(Exception):
-def __init__(self, fname, line, col, incl_info, msg):
+def __init__(self, info, col, msg):
Exception.__init__(self)
-self.fname = fname
-self.line = line
+self.info = info
self.col = col
-self.info = incl_info
self.msg = msg
def __str__(self):
-loc = '%s:%d' % (self.fname, self.line)
+loc = str(self.info)
if self.col is not None:
+assert self.info.line is not None
loc += ':%s' % self.col
-return error_path(self.info) + '%s: %s' % (loc, self.msg)
+return loc + ': ' + self.msg
class QAPIParseError(QAPIError):
@@ -91,14 +108,12 @@ class QAPIParseError(QAPIError):
col = (col + 7) % 8 + 1
else:
col += 1
-QAPIError.__init__(self, parser.fname, parser.line, col,
- parser.incl_info, msg)
+QAPIError.__init__(self, parser.info, col, msg)
class QAPISemError(QAPIError):
def __init__(self, info, msg):
-QAPIError.__init__(self, info['file'], info['line'], None,
- info['parent'], msg)
+QAPIError.__init__(self, info, None, msg)
class QAPIDoc(object):
@@ -382,12 +397,11 @@ class QAPISchemaParser(object):
def __init__(self, fp, previously_included=[], incl_info=None):
self.fname = fp.name
previously_included.append(os.path.abspath(fp.name))
-self.incl_info = incl_info
self.src = fp.read()
if self.src == '' or self.src[-1] != '\n':
self.src += '\n'
self.cursor = 0
-self.line = 1
+self.info = QAPISourceInfo(self.fname, 1, incl_info)
self.line_pos = 0
self.exprs = []
self.docs = []
@@ -395,8 +409,7 @@ class QAPISchemaParser(object):
cur_doc = None
while self.tok is not None:
-info = {'file': self.fname, 'line': self.line,
-'parent': self.incl_info}
+info = self.info
if self.tok == '#':
self.reject_expr_doc(cur_doc)
cur_doc = self.get_doc(info)
@@ -456,9 +469,9 @@ class QAPISchemaParser(object):
# catch inclusion cycle
inf = info
while inf:
-if incl_abs_fname == os.path.abspath(inf['file']):
+if incl_abs_fname == os.path.abspath(inf.fname):
raise QAPISemError(info, "Inclusion loop for %s" % include)
-inf = inf['parent']
+inf = inf.parent
# skip multiple include of the same file
if incl_abs_fname in previously_included:
@@ -552,7 +565,7 @@ class QAPISchemaParser(object):
if self.cursor == len(self.src):
self.tok = None
return
-self.line += 1
+self.info = self.info.next_line()
self.line_pos = self.cursor
elif not self.tok.isspace():
# Show up to next structural,
[PULL 03/27] qapi: Rename .owner to .defined_in
QAPISchemaMember.owner is the name of the defining entity. That's a
confusing name when an object type inherits members from a base type.
Rename it to .defined_in. Rename .set_owner() and ._pretty_owner() to
match.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-3-arm...@redhat.com>
---
scripts/qapi/common.py | 61 +-
1 file changed, 31 insertions(+), 30 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 155b87b825..bfb3e8a493 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1319,7 +1319,7 @@ class QAPISchemaEnumType(QAPISchemaType):
QAPISchemaType.__init__(self, name, info, doc, ifcond)
for m in members:
assert isinstance(m, QAPISchemaEnumMember)
-m.set_owner(name)
+m.set_defined_in(name)
assert prefix is None or isinstance(prefix, str)
self.members = members
self.prefix = prefix
@@ -1405,13 +1405,13 @@ class QAPISchemaObjectType(QAPISchemaType):
assert base is None or isinstance(base, str)
for m in local_members:
assert isinstance(m, QAPISchemaObjectTypeMember)
-m.set_owner(name)
+m.set_defined_in(name)
if variants is not None:
assert isinstance(variants, QAPISchemaObjectTypeVariants)
-variants.set_owner(name)
+variants.set_defined_in(name)
for f in features:
assert isinstance(f, QAPISchemaFeature)
-f.set_owner(name)
+f.set_defined_in(name)
self._base_name = base
self.base = None
self.local_members = local_members
@@ -1521,15 +1521,16 @@ class QAPISchemaMember(object):
assert isinstance(name, str)
self.name = name
self.ifcond = ifcond or []
-self.owner = None
+self.defined_in = None
-def set_owner(self, name):
-assert not self.owner
-self.owner = name
+def set_defined_in(self, name):
+assert not self.defined_in
+self.defined_in = name
def check_clash(self, info, seen):
cname = c_name(self.name)
-if cname.lower() != cname and self.owner not in name_case_whitelist:
+if (cname.lower() != cname
+and self.defined_in not in name_case_whitelist):
raise QAPISemError(info,
"%s should not use uppercase" % self.describe())
if cname in seen:
@@ -1537,27 +1538,27 @@ class QAPISchemaMember(object):
(self.describe(), seen[cname].describe()))
seen[cname] = self
-def _pretty_owner(self):
-owner = self.owner
-if owner.startswith('q_obj_'):
+def _pretty_defined_in(self):
+defined_in = self.defined_in
+if defined_in.startswith('q_obj_'):
# See QAPISchema._make_implicit_object_type() - reverse the
# mapping there to create a nice human-readable description
-owner = owner[6:]
-if owner.endswith('-arg'):
-return '(parameter of %s)' % owner[:-4]
-elif owner.endswith('-base'):
-return '(base of %s)' % owner[:-5]
+defined_in = defined_in[6:]
+if defined_in.endswith('-arg'):
+return '(parameter of %s)' % defined_in[:-4]
+elif defined_in.endswith('-base'):
+return '(base of %s)' % defined_in[:-5]
else:
-assert owner.endswith('-wrapper')
+assert defined_in.endswith('-wrapper')
# Unreachable and not implemented
assert False
-if owner.endswith('Kind'):
+if defined_in.endswith('Kind'):
# See QAPISchema._make_implicit_enum_type()
-return '(branch of %s)' % owner[:-4]
-return '(%s of %s)' % (self.role, owner)
+return '(branch of %s)' % defined_in[:-4]
+return '(%s of %s)' % (self.role, defined_in)
def describe(self):
-return "'%s' %s" % (self.name, self._pretty_owner())
+return "'%s' %s" % (self.name, self._pretty_defined_in())
class QAPISchemaEnumMember(QAPISchemaMember):
@@ -1578,7 +1579,7 @@ class QAPISchemaObjectTypeMember(QAPISchemaMember):
self.optional = optional
def check(self, schema):
-assert self.owner
+assert self.defined_in
self.type = schema.lookup_type(self._type_name)
assert self.type
@@ -1598,9 +1599,9 @@ class QAPISchemaObjectTypeVariants(object):
self.tag_member = tag_member
self.variants = variants
-def set_owner(self, name):
+def set_defined_in(self, name):
for v in self.variants:
-v.set_owner(name)
+v.set_defined_in(name)
def check(self, schema, seen):
if not self.tag_member:# flat union
@@ -1616,7 +161
[PULL 20/27] qapi: Improve reporting of invalid flags
Split check_flags() off check_keys() and have check_exprs() call it later, so its error messages gain an "in definition" line. Tweak the error messages. Checking values in a function named check_keys() is unclean anyway. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-20-arm...@redhat.com> --- scripts/qapi/common.py | 22 -- tests/qapi-schema/allow-preconfig-test.err | 3 ++- tests/qapi-schema/args-bad-boxed.err | 3 ++- tests/qapi-schema/oob-test.err | 3 ++- tests/qapi-schema/type-bypass-bad-gen.err | 3 ++- 5 files changed, 20 insertions(+), 14 deletions(-) diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 8f96974f85..4f67b73684 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -915,16 +915,17 @@ def check_keys(expr, info, meta, required, optional=[]): required = required + [meta] source = "%s '%s'" % (meta, name) check_known_keys(expr, info, source, required, optional) -for (key, value) in expr.items(): -if key in ['gen', 'success-response'] and value is not False: -raise QAPISemError(info, - "'%s' of %s '%s' should only use false value" - % (key, meta, name)) -if (key in ['boxed', 'allow-oob', 'allow-preconfig'] -and value is not True): -raise QAPISemError(info, - "'%s' of %s '%s' should only use true value" - % (key, meta, name)) + + +def check_flags(expr, info): +for key in ['gen', 'success-response']: +if key in expr and expr[key] is not False: +raise QAPISemError( +info, "flag '%s' may only use false value" % key) +for key in ['boxed', 'allow-oob', 'allow-preconfig']: +if key in expr and expr[key] is not True: +raise QAPISemError( +info, "flag '%s' may only use true value" % key) def normalize_enum(expr): @@ -1027,6 +1028,7 @@ def check_exprs(exprs): assert False, 'unexpected meta type' check_if(expr, info) +check_flags(expr, info) if doc: doc.check_expr(expr) diff --git a/tests/qapi-schema/allow-preconfig-test.err b/tests/qapi-schema/allow-preconfig-test.err index 700d583306..2a4e6ce663 100644 --- a/tests/qapi-schema/allow-preconfig-test.err +++ b/tests/qapi-schema/allow-preconfig-test.err @@ -1 +1,2 @@ -tests/qapi-schema/allow-preconfig-test.json:2: 'allow-preconfig' of command 'allow-preconfig-test' should only use true value +tests/qapi-schema/allow-preconfig-test.json: In command 'allow-preconfig-test': +tests/qapi-schema/allow-preconfig-test.json:2: flag 'allow-preconfig' may only use true value diff --git a/tests/qapi-schema/args-bad-boxed.err b/tests/qapi-schema/args-bad-boxed.err index ad0d417321..31d39038fc 100644 --- a/tests/qapi-schema/args-bad-boxed.err +++ b/tests/qapi-schema/args-bad-boxed.err @@ -1 +1,2 @@ -tests/qapi-schema/args-bad-boxed.json:2: 'boxed' of command 'foo' should only use true value +tests/qapi-schema/args-bad-boxed.json: In command 'foo': +tests/qapi-schema/args-bad-boxed.json:2: flag 'boxed' may only use true value diff --git a/tests/qapi-schema/oob-test.err b/tests/qapi-schema/oob-test.err index 35b60f7480..3c2ba6e0fd 100644 --- a/tests/qapi-schema/oob-test.err +++ b/tests/qapi-schema/oob-test.err @@ -1 +1,2 @@ -tests/qapi-schema/oob-test.json:2: 'allow-oob' of command 'oob-command-1' should only use true value +tests/qapi-schema/oob-test.json: In command 'oob-command-1': +tests/qapi-schema/oob-test.json:2: flag 'allow-oob' may only use true value diff --git a/tests/qapi-schema/type-bypass-bad-gen.err b/tests/qapi-schema/type-bypass-bad-gen.err index a83c3c655d..1077651896 100644 --- a/tests/qapi-schema/type-bypass-bad-gen.err +++ b/tests/qapi-schema/type-bypass-bad-gen.err @@ -1 +1,2 @@ -tests/qapi-schema/type-bypass-bad-gen.json:2: 'gen' of command 'foo' should only use false value +tests/qapi-schema/type-bypass-bad-gen.json: In command 'foo': +tests/qapi-schema/type-bypass-bad-gen.json:2: flag 'gen' may only use false value -- 2.21.0
[PULL 00/27] QAPI patches for 2019-09-28
The following changes since commit c6f5012ba5fa834cbd5274b1b8369e2c5d2f5933: Merge remote-tracking branch 'remotes/stsquad/tags/pull-testing-next-260919-1' into staging (2019-09-27 15:43:41 +0100) are available in the Git repository at: git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2019-09-28 for you to fetch changes up to c615550df306a7b16e75d21f65ee38898c756bac: qapi: Improve source file read error handling (2019-09-28 17:17:48 +0200) QAPI patches for 2019-09-28 Markus Armbruster (26): qapi: Tighten QAPISchemaFOO.check() assertions qapi: Rename .owner to .defined_in qapi: New QAPISourceInfo, replacing dict qapi: Prefix frontend errors with an "in definition" line qapi: Clean up member name case checking qapi: Change frontend error messages to start with lower case qapi: Improve reporting of member name clashes qapi: Reorder check_FOO() parameters for consistency qapi: Improve reporting of invalid name errors qapi: Use check_name_str() where it suffices qapi: Report invalid '*' prefix like any other invalid name qapi: Move check for reserved names out of add_name() qapi: Make check_type()'s array case a bit more obvious qapi: Plumb info to the QAPISchemaMember qapi: Inline check_name() into check_union() qapi: Move context-sensitive checking to the proper place qapi: Move context-free checking to the proper place qapi: Improve reporting of invalid 'if' errors qapi: Improve reporting of invalid flags qapi: Improve reporting of missing / unknown definition keys qapi: Avoid redundant definition references in error messages qapi: Improve reporting of invalid 'if' further qapi: Eliminate check_keys(), rename check_known_keys() qapi: Improve reporting of missing documentation comment qapi: Improve reporting of redefinition qapi: Improve source file read error handling Michal Privoznik (1): qmp-dispatch: Use CommandNotFound error for disabled commands qapi/qapi-schema.json |2 +- qapi/qmp-dispatch.c|5 +- tests/test-qga.c |4 +- scripts/qapi/common.py | 1052 ++-- scripts/qapi/events.py |2 +- tests/qapi-schema/allow-preconfig-test.err |3 +- tests/qapi-schema/alternate-any.err|3 +- tests/qapi-schema/alternate-array.err |3 +- tests/qapi-schema/alternate-base.err |3 +- tests/qapi-schema/alternate-branch-if-invalid.err |3 +- tests/qapi-schema/alternate-clash.err |3 +- .../qapi-schema/alternate-conflict-bool-string.err |3 +- tests/qapi-schema/alternate-conflict-dict.err |3 +- tests/qapi-schema/alternate-conflict-enum-bool.err |3 +- tests/qapi-schema/alternate-conflict-enum-int.err |3 +- .../qapi-schema/alternate-conflict-num-string.err |3 +- tests/qapi-schema/alternate-conflict-string.err|3 +- tests/qapi-schema/alternate-empty.err |3 +- tests/qapi-schema/alternate-invalid-dict.err |3 +- tests/qapi-schema/alternate-nested.err |3 +- tests/qapi-schema/alternate-unknown.err|3 +- tests/qapi-schema/args-alternate.err |3 +- tests/qapi-schema/args-any.err |3 +- tests/qapi-schema/args-array-empty.err |3 +- tests/qapi-schema/args-array-unknown.err |3 +- tests/qapi-schema/args-bad-boxed.err |3 +- tests/qapi-schema/args-boxed-anon.err |3 +- tests/qapi-schema/args-boxed-string.err|3 +- tests/qapi-schema/args-int.err |3 +- tests/qapi-schema/args-invalid.err |3 +- tests/qapi-schema/args-member-array-bad.err|3 +- tests/qapi-schema/args-member-case.err |3 +- tests/qapi-schema/args-member-case.json|2 +- tests/qapi-schema/args-member-unknown.err |3 +- tests/qapi-schema/args-name-clash.err |3 +- tests/qapi-schema/args-union.err |3 +- tests/qapi-schema/args-unknown.err |3 +- tests/qapi-schema/bad-base.err |3 +- tests/qapi-schema/bad-data.err |3 +- tests/qapi-schema/bad-ident.err|3 +- tests/qapi-schema/bad-if-empty-list.err|3 +- tests/qapi-schema/bad-if-empty.err |3 +- tests/qapi-schema/bad-if-list.err |3 +- tests/qapi-schema/bad-if.err |3 +- tests/qapi-schema/bad-type-bool.err|2 +- t
[PULL 16/27] qapi: Inline check_name() into check_union()
check_name() consists of check_name_is_str() and check_name_str(). check_union() relies on the latter to catch optional discriminators. The next commit will replace that by a more straightforward check. Inlining check_name() into check_union() now should make that easier to review. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-16-arm...@redhat.com> --- scripts/qapi/common.py | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 88945804dc..9acff01d3e 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -897,8 +897,10 @@ def check_union(expr, info): # The value of member 'discriminator' must name a non-optional # member of the base struct. -check_name(discriminator, info, - "discriminator of flat union '%s'" % name) +check_name_is_str(discriminator, info, + "discriminator of flat union '%s'" % name) +check_name_str(discriminator, info, + "discriminator of flat union '%s'" % name) discriminator_value = base_members.get(discriminator) if not discriminator_value: raise QAPISemError(info, -- 2.21.0
[PULL 19/27] qapi: Improve reporting of invalid 'if' errors
Move check_if() from check_keys() to check_exprs() and call it later, so its error messages gain an "in definition" line. Checking values in a function named check_keys() is unclean anyway. The original sin was commit 0545f6b887 "qapi: Better error messages for bad expressions", which checks the value of key 'name'. More sinning in commit 2cbf09925a "qapi: More rigorous checking for type safety bypass", commit c818408e44 "qapi: Implement boxed types for commands/events", and commit 967c885108 "qapi: add 'if' to top-level expressions". This commit does penance for the latter. The next commits will do penance for the others. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-19-arm...@redhat.com> --- scripts/qapi/common.py | 4 ++-- tests/qapi-schema/bad-if-empty-list.err | 1 + tests/qapi-schema/bad-if-empty.err | 1 + tests/qapi-schema/bad-if-list.err | 1 + tests/qapi-schema/bad-if.err| 1 + 5 files changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 2e1d8158d6..8f96974f85 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -925,8 +925,6 @@ def check_keys(expr, info, meta, required, optional=[]): raise QAPISemError(info, "'%s' of %s '%s' should only use true value" % (key, meta, name)) -if key == 'if': -check_if(expr, info) def normalize_enum(expr): @@ -1028,6 +1026,8 @@ def check_exprs(exprs): else: assert False, 'unexpected meta type' +check_if(expr, info) + if doc: doc.check_expr(expr) diff --git a/tests/qapi-schema/bad-if-empty-list.err b/tests/qapi-schema/bad-if-empty-list.err index 75fe6497bc..2218c9279b 100644 --- a/tests/qapi-schema/bad-if-empty-list.err +++ b/tests/qapi-schema/bad-if-empty-list.err @@ -1 +1,2 @@ +tests/qapi-schema/bad-if-empty-list.json: In struct 'TestIfStruct': tests/qapi-schema/bad-if-empty-list.json:2: 'if' condition [] is useless diff --git a/tests/qapi-schema/bad-if-empty.err b/tests/qapi-schema/bad-if-empty.err index 358bdc3e51..a3fdb3009d 100644 --- a/tests/qapi-schema/bad-if-empty.err +++ b/tests/qapi-schema/bad-if-empty.err @@ -1 +1,2 @@ +tests/qapi-schema/bad-if-empty.json: In struct 'TestIfStruct': tests/qapi-schema/bad-if-empty.json:2: 'if' condition '' makes no sense diff --git a/tests/qapi-schema/bad-if-list.err b/tests/qapi-schema/bad-if-list.err index 53af099083..e03bf0fc3a 100644 --- a/tests/qapi-schema/bad-if-list.err +++ b/tests/qapi-schema/bad-if-list.err @@ -1 +1,2 @@ +tests/qapi-schema/bad-if-list.json: In struct 'TestIfStruct': tests/qapi-schema/bad-if-list.json:2: 'if' condition ' ' makes no sense diff --git a/tests/qapi-schema/bad-if.err b/tests/qapi-schema/bad-if.err index c2e3f5f44c..190216c109 100644 --- a/tests/qapi-schema/bad-if.err +++ b/tests/qapi-schema/bad-if.err @@ -1 +1,2 @@ +tests/qapi-schema/bad-if.json: In struct 'TestIfStruct': tests/qapi-schema/bad-if.json:2: 'if' condition must be a string or a list of strings -- 2.21.0
[PULL 13/27] qapi: Move check for reserved names out of add_name()
The checks for reserved names are spread far and wide. Move one from
add_name() to new check_defn_name_str(). This is a first step towards
collecting them all in dedicated name checking functions next to
check_name().
While there, drop the quotes around the meta-type in
check_name_str()'s error messages: "'command' uses ... name 'NAME'"
becomes "command uses ... name 'NAME'".
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-13-arm...@redhat.com>
---
scripts/qapi/common.py | 16 ++--
tests/qapi-schema/bad-ident.err | 2 +-
tests/qapi-schema/reserved-command-q.err | 2 +-
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index a4cf41f13e..111a4bbe55 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -741,6 +741,13 @@ def check_name_str(name, info, source,
assert not membername.startswith('*')
+def check_defn_name_str(name, info, meta):
+check_name_str(name, info, meta, permit_upper=True)
+if name.endswith('Kind') or name.endswith('List'):
+raise QAPISemError(
+info, "%s '%s' should not end in '%s'" % (meta, name, name[-4:]))
+
+
def add_name(name, info, meta):
global all_names
# FIXME should reject names that differ only in '_' vs. '.'
@@ -748,9 +755,6 @@ def add_name(name, info, meta):
if name in all_names:
raise QAPISemError(info, "%s '%s' is already defined"
% (all_names[name], name))
-if name.endswith('Kind') or name.endswith('List'):
-raise QAPISemError(info, "%s '%s' should not end in '%s'"
- % (meta, name, name[-4:]))
all_names[name] = meta
@@ -1162,7 +1166,7 @@ def check_exprs(exprs):
name = expr[meta]
check_name_is_str(name, info, "'%s'" % meta)
info.set_defn(meta, name)
-check_name_str(name, info, "'%s'" % meta, permit_upper=True)
+check_defn_name_str(name, info, meta)
add_name(name, info, meta)
if doc and doc.symbol != name:
raise QAPISemError(
@@ -1889,13 +1893,13 @@ class QAPISchema(object):
def _make_implicit_enum_type(self, name, info, ifcond, values):
# See also QAPISchemaObjectTypeMember.describe()
-name = name + 'Kind' # Use namespace reserved by add_name()
+name = name + 'Kind'# reserved by check_defn_name_str()
self._def_entity(QAPISchemaEnumType(
name, info, None, ifcond, self._make_enum_members(values), None))
return name
def _make_array_type(self, element_type, info):
-name = element_type + 'List' # Use namespace reserved by add_name()
+name = element_type + 'List'# reserved by check_defn_name_str()
if not self.lookup_type(name):
self._def_entity(QAPISchemaArrayType(name, info, element_type))
return name
diff --git a/tests/qapi-schema/bad-ident.err b/tests/qapi-schema/bad-ident.err
index ddc96bd3a9..79d14758ce 100644
--- a/tests/qapi-schema/bad-ident.err
+++ b/tests/qapi-schema/bad-ident.err
@@ -1,2 +1,2 @@
tests/qapi-schema/bad-ident.json: In struct '*oops':
-tests/qapi-schema/bad-ident.json:2: 'struct' uses invalid name '*oops'
+tests/qapi-schema/bad-ident.json:2: struct uses invalid name '*oops'
diff --git a/tests/qapi-schema/reserved-command-q.err
b/tests/qapi-schema/reserved-command-q.err
index 0844e14b26..631cb5cdcc 100644
--- a/tests/qapi-schema/reserved-command-q.err
+++ b/tests/qapi-schema/reserved-command-q.err
@@ -1,2 +1,2 @@
tests/qapi-schema/reserved-command-q.json: In command 'q-unix':
-tests/qapi-schema/reserved-command-q.json:5: 'command' uses invalid name
'q-unix'
+tests/qapi-schema/reserved-command-q.json:5: command uses invalid name 'q-unix'
--
2.21.0
[PULL 11/27] qapi: Use check_name_str() where it suffices
Replace check_name() by check_name_str() where the name is known to be
a string.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-11-arm...@redhat.com>
---
scripts/qapi/common.py | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 6f35cd131e..d0d997f31c 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -816,8 +816,8 @@ def check_type(value, info, source,
# value is a dictionary, check that each member is okay
for (key, arg) in value.items():
-check_name(key, info, "member of %s" % source,
- allow_optional=True, permit_upper=permit_upper)
+check_name_str(key, info, "member of %s" % source,
+ allow_optional=True, permit_upper=permit_upper)
if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
raise QAPISemError(
info, "member of %s uses reserved name '%s'" % (source, key))
@@ -920,8 +920,7 @@ def check_union(expr, info):
raise QAPISemError(info, "union '%s' has no branches" % name)
for (key, value) in members.items():
-check_name(key, info, "member of union '%s'" % name)
-
+check_name_str(key, info, "member of union '%s'" % name)
check_known_keys(value, info,
"member '%s' of union '%s'" % (key, name),
['type'], ['if'])
@@ -951,7 +950,7 @@ def check_alternate(expr, info):
raise QAPISemError(info,
"alternate '%s' cannot have empty 'data'" % name)
for (key, value) in members.items():
-check_name(key, info, "member of alternate '%s'" % name)
+check_name_str(key, info, "member of alternate '%s'" % name)
check_known_keys(value, info,
"member '%s' of alternate '%s'" % (key, name),
['type'], ['if'])
--
2.21.0
[PULL 25/27] qapi: Improve reporting of missing documentation comment
Have check_exprs() check this later, so the error message gains an "in definition line". Tweak the error message. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-25-arm...@redhat.com> --- scripts/qapi/common.py| 18 -- tests/qapi-schema/doc-missing.err | 3 ++- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index fa354b3f1e..bd834270f8 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -933,10 +933,6 @@ def check_exprs(exprs): if 'include' in expr: continue -if not doc and doc_required: -raise QAPISemError(info, - "definition missing documentation comment") - if 'enum' in expr: meta = 'enum' elif 'union' in expr: @@ -957,9 +953,14 @@ def check_exprs(exprs): info.set_defn(meta, name) check_defn_name_str(name, info, meta) -if doc and doc.symbol != name: -raise QAPISemError( -info, "documentation comment is for '%s'" % doc.symbol) +if doc: +if doc.symbol != name: +raise QAPISemError( +info, "documentation comment is for '%s'" % doc.symbol) +doc.check_expr(expr) +elif doc_required: +raise QAPISemError(info, + "documentation comment required") if meta == 'enum': check_keys(expr, info, meta, @@ -1004,9 +1005,6 @@ def check_exprs(exprs): check_if(expr, info, meta) check_flags(expr, info) -if doc: -doc.check_expr(expr) - return exprs diff --git a/tests/qapi-schema/doc-missing.err b/tests/qapi-schema/doc-missing.err index 08c827931a..7fbf54ff65 100644 --- a/tests/qapi-schema/doc-missing.err +++ b/tests/qapi-schema/doc-missing.err @@ -1 +1,2 @@ -tests/qapi-schema/doc-missing.json:5: definition missing documentation comment +tests/qapi-schema/doc-missing.json: In command 'undocumented': +tests/qapi-schema/doc-missing.json:5: documentation comment required -- 2.21.0
[PULL 06/27] qapi: Clean up member name case checking
QAPISchemaMember.check_clash() checks for member names that map to the
same c_name(). Takes care of rejecting duplicate names.
It also checks a naming rule: no uppercase in member names. That's a
rather odd place to do it. Enforcing naming rules is
check_name_str()'s job.
qapi-code-gen.txt specifies the name case rule applies to the name as
it appears in the schema. check_clash() checks c_name(name) instead.
No difference, as c_name() leaves alone case, but unclean.
Move the name case check into check_name_str(), less the c_name().
New argument @permit_upper suppresses it. Pass permit_upper=True for
definitions (which are not members), and when the member's owner is
whitelisted with pragma name-case-whitelist.
Bonus: name-case-whitelist now applies to a union's inline base, too.
Update qapi/qapi-schema.json pragma to whitelist union CpuInfo instead
of CpuInfo's implicit base type's name q_obj_CpuInfo-base.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-6-arm...@redhat.com>
---
qapi/qapi-schema.json| 2 +-
scripts/qapi/common.py | 25 +---
tests/qapi-schema/args-member-case.err | 2 +-
tests/qapi-schema/args-member-case.json | 2 +-
tests/qapi-schema/enum-member-case.err | 2 +-
tests/qapi-schema/union-branch-case.err | 4 ++--
tests/qapi-schema/union-branch-case.json | 4 ++--
7 files changed, 22 insertions(+), 19 deletions(-)
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index 920b03b0aa..9751b11f8f 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -71,7 +71,7 @@
'QapiErrorClass', # all members, visible through errors
'UuidInfo', # UUID, visible through query-uuid
'X86CPURegister32', # all members, visible indirectly through
qom-get
-'q_obj_CpuInfo-base'# CPU, visible through query-cpu
+'CpuInfo' # CPU, visible through query-cpu
] } }
# Documentation generated with qapi-gen.py is in source order, with
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index f0e7d5ad34..ed4bff4479 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -704,8 +704,8 @@ valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
'[a-zA-Z][a-zA-Z0-9_-]*$')
-def check_name(info, source, name, allow_optional=False,
- enum_member=False):
+def check_name(info, source, name,
+ allow_optional=False, enum_member=False, permit_upper=False):
global valid_name
membername = name
@@ -725,11 +725,14 @@ def check_name(info, source, name, allow_optional=False,
if not valid_name.match(membername) or \
c_name(membername, False).startswith('q_'):
raise QAPISemError(info, "%s uses invalid name '%s'" % (source, name))
+if not permit_upper and name.lower() != name:
+raise QAPISemError(
+info, "%s uses uppercase in name '%s'" % (source, name))
def add_name(name, info, meta):
global all_names
-check_name(info, "'%s'" % meta, name)
+check_name(info, "'%s'" % meta, name, permit_upper=True)
# FIXME should reject names that differ only in '_' vs. '.'
# vs. '-', because they're liable to clash in generated C.
if name in all_names:
@@ -797,10 +800,12 @@ def check_type(info, source, value,
raise QAPISemError(info,
"%s should be an object or type name" % source)
+permit_upper = allow_dict in name_case_whitelist
+
# value is a dictionary, check that each member is okay
for (key, arg) in value.items():
check_name(info, "Member of %s" % source, key,
- allow_optional=True)
+ allow_optional=True, permit_upper=permit_upper)
if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
raise QAPISemError(info, "Member of %s uses reserved name '%s'"
% (source, key))
@@ -870,7 +875,7 @@ def check_union(expr, info):
else:
# The object must have a string or dictionary 'base'.
check_type(info, "'base' for union '%s'" % name,
- base, allow_dict=True,
+ base, allow_dict=name,
allow_metas=['struct'])
if not base:
raise QAPISemError(info, "Flat union '%s' must have a base"
@@ -982,13 +987,15 @@ def check_enum(expr, info):
raise QAPISemError(info,
"Enum '%s' requires a string for 'prefix'" % name)
+permit_upper = name in name_case_whitelist
+
for member in members:
check_known_keys(info, "member of enum '%s'" % name, member,
['name'], ['if'])
check_if(member, info)
normalize_if(member)
check_name(info, "Member of enum '%s'" % name, member['name'],
- enum_member=True)
+
[PULL 15/27] qapi: Plumb info to the QAPISchemaMember
Future commits will need info in the .check() methods of
QAPISchemaMember and its descendants. Get it there.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-15-arm...@redhat.com>
---
scripts/qapi/common.py | 76 +++---
scripts/qapi/events.py | 2 +-
2 files changed, 43 insertions(+), 35 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 870d8b0ecb..88945804dc 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1574,9 +1574,10 @@ class QAPISchemaMember(object):
""" Represents object members, enum members and features """
role = 'member'
-def __init__(self, name, ifcond=None):
+def __init__(self, name, info, ifcond=None):
assert isinstance(name, str)
self.name = name
+self.info = info
self.ifcond = ifcond or []
self.defined_in = None
@@ -1633,8 +1634,8 @@ class QAPISchemaFeature(QAPISchemaMember):
class QAPISchemaObjectTypeMember(QAPISchemaMember):
-def __init__(self, name, typ, optional, ifcond=None):
-QAPISchemaMember.__init__(self, name, ifcond)
+def __init__(self, name, info, typ, optional, ifcond=None):
+QAPISchemaMember.__init__(self, name, info, ifcond)
assert isinstance(typ, str)
assert isinstance(optional, bool)
self._type_name = typ
@@ -1648,7 +1649,7 @@ class QAPISchemaObjectTypeMember(QAPISchemaMember):
class QAPISchemaObjectTypeVariants(object):
-def __init__(self, tag_name, tag_member, variants):
+def __init__(self, tag_name, info, tag_member, variants):
# Flat unions pass tag_name but not tag_member.
# Simple unions and alternates pass tag_member but not tag_name.
# After check(), tag_member is always set, and tag_name remains
@@ -1659,6 +1660,7 @@ class QAPISchemaObjectTypeVariants(object):
for v in variants:
assert isinstance(v, QAPISchemaObjectTypeVariant)
self._tag_name = tag_name
+self.info = info
self.tag_member = tag_member
self.variants = variants
@@ -1678,8 +1680,8 @@ class QAPISchemaObjectTypeVariants(object):
cases = set([v.name for v in self.variants])
for m in self.tag_member.type.members:
if m.name not in cases:
-v = QAPISchemaObjectTypeVariant(m.name, 'q_empty',
-m.ifcond)
+v = QAPISchemaObjectTypeVariant(m.name, self.info,
+'q_empty', m.ifcond)
v.set_defined_in(self.tag_member.defined_in)
self.variants.append(v)
assert self.variants
@@ -1703,8 +1705,9 @@ class QAPISchemaObjectTypeVariants(object):
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
role = 'branch'
-def __init__(self, name, typ, ifcond=None):
-QAPISchemaObjectTypeMember.__init__(self, name, typ, False, ifcond)
+def __init__(self, name, info, typ, ifcond=None):
+QAPISchemaObjectTypeMember.__init__(self, name, info, typ,
+False, ifcond)
class QAPISchemaAlternateType(QAPISchemaType):
@@ -1880,23 +1883,26 @@ class QAPISchema(object):
qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
'qbool']
-qtype_values = self._make_enum_members([{'name': n} for n in qtypes])
+qtype_values = self._make_enum_members(
+[{'name': n} for n in qtypes], None)
self._def_entity(QAPISchemaEnumType('QType', None, None, None,
qtype_values, 'QTYPE'))
-def _make_features(self, features):
-return [QAPISchemaFeature(f['name'], f.get('if')) for f in features]
+def _make_features(self, features, info):
+return [QAPISchemaFeature(f['name'], info, f.get('if'))
+for f in features]
-def _make_enum_members(self, values):
-return [QAPISchemaEnumMember(v['name'], v.get('if'))
+def _make_enum_members(self, values, info):
+return [QAPISchemaEnumMember(v['name'], info, v.get('if'))
for v in values]
def _make_implicit_enum_type(self, name, info, ifcond, values):
# See also QAPISchemaObjectTypeMember.describe()
name = name + 'Kind'# reserved by check_defn_name_str()
self._def_entity(QAPISchemaEnumType(
-name, info, None, ifcond, self._make_enum_members(values), None))
+name, info, None, ifcond, self._make_enum_members(values, info),
+None))
return name
def _make_array_type(self, element_type, info):
@@ -1935,7 +1941,7 @@ class QAPISchema(object):
ifcond = expr.get('if')
self._def_entity(QAPISchemaEnumType(
name, info, doc, ifcond,
-self._make_enum_mem
[PULL 12/27] qapi: Report invalid '*' prefix like any other invalid name
The special "does not allow optional name" error is well meant, but
confusing in practice. Drop it.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-12-arm...@redhat.com>
---
scripts/qapi/common.py | 6 ++
tests/qapi-schema/bad-ident.err | 2 +-
tests/qapi-schema/flat-union-discriminator-bad-name.err | 2 +-
tests/qapi-schema/flat-union-discriminator-bad-name.json | 2 +-
tests/qapi-schema/union-optional-branch.err | 2 +-
5 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index d0d997f31c..a4cf41f13e 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -724,11 +724,8 @@ def check_name_str(name, info, source,
global valid_name
membername = name
-if name.startswith('*'):
+if allow_optional and name.startswith('*'):
membername = name[1:]
-if not allow_optional:
-raise QAPISemError(info, "%s does not allow optional name '%s'"
- % (source, name))
# Enum members can start with a digit, because the generated C
# code always prefixes it with the enum name
if enum_member and membername[0].isdigit():
@@ -741,6 +738,7 @@ def check_name_str(name, info, source,
if not permit_upper and name.lower() != name:
raise QAPISemError(
info, "%s uses uppercase in name '%s'" % (source, name))
+assert not membername.startswith('*')
def add_name(name, info, meta):
diff --git a/tests/qapi-schema/bad-ident.err b/tests/qapi-schema/bad-ident.err
index 6878889854..ddc96bd3a9 100644
--- a/tests/qapi-schema/bad-ident.err
+++ b/tests/qapi-schema/bad-ident.err
@@ -1,2 +1,2 @@
tests/qapi-schema/bad-ident.json: In struct '*oops':
-tests/qapi-schema/bad-ident.json:2: 'struct' does not allow optional name
'*oops'
+tests/qapi-schema/bad-ident.json:2: 'struct' uses invalid name '*oops'
diff --git a/tests/qapi-schema/flat-union-discriminator-bad-name.err
b/tests/qapi-schema/flat-union-discriminator-bad-name.err
index f7f64c5c1a..44e41883b1 100644
--- a/tests/qapi-schema/flat-union-discriminator-bad-name.err
+++ b/tests/qapi-schema/flat-union-discriminator-bad-name.err
@@ -1,2 +1,2 @@
tests/qapi-schema/flat-union-discriminator-bad-name.json: In union 'MyUnion':
-tests/qapi-schema/flat-union-discriminator-bad-name.json:7: discriminator of
flat union 'MyUnion' does not allow optional name '*switch'
+tests/qapi-schema/flat-union-discriminator-bad-name.json:7: discriminator of
flat union 'MyUnion' uses invalid name '*switch'
diff --git a/tests/qapi-schema/flat-union-discriminator-bad-name.json
b/tests/qapi-schema/flat-union-discriminator-bad-name.json
index 66376084fc..ea84b75cac 100644
--- a/tests/qapi-schema/flat-union-discriminator-bad-name.json
+++ b/tests/qapi-schema/flat-union-discriminator-bad-name.json
@@ -1,5 +1,5 @@
# discriminator '*switch' isn't a member of base, 'switch' is
-# reports "does not allow optional name", which is good enough
+# reports "uses invalid name", which is good enough
{ 'enum': 'Enum', 'data': [ 'one', 'two' ] }
{ 'struct': 'Base',
'data': { '*switch': 'Enum' } }
diff --git a/tests/qapi-schema/union-optional-branch.err
b/tests/qapi-schema/union-optional-branch.err
index a5677f74bc..8e9b18d7c6 100644
--- a/tests/qapi-schema/union-optional-branch.err
+++ b/tests/qapi-schema/union-optional-branch.err
@@ -1,2 +1,2 @@
tests/qapi-schema/union-optional-branch.json: In union 'Union':
-tests/qapi-schema/union-optional-branch.json:2: member of union 'Union' does
not allow optional name '*a'
+tests/qapi-schema/union-optional-branch.json:2: member of union 'Union' uses
invalid name '*a'
--
2.21.0
[PULL 18/27] qapi: Move context-free checking to the proper place
QAPISchemaCommand.check() and QAPISchemaEvent().check() check 'data'
is present when 'boxed': true. That's context-free. Move to
check_command() and check_event().
Tweak the error message while there.
check_exprs() & friends now check exactly what qapi-code-gen.txt calls
the second layer of syntax.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-18-arm...@redhat.com>
---
scripts/qapi/common.py | 16
tests/qapi-schema/event-boxed-empty.err | 2 +-
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index f22e84c4a8..2e1d8158d6 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -768,10 +768,12 @@ def check_type(value, info, source,
def check_command(expr, info):
name = expr['command']
+args = expr.get('data')
boxed = expr.get('boxed', False)
-check_type(expr.get('data'), info,
- "'data' for command '%s'" % name,
+if boxed and args is None:
+raise QAPISemError(info, "'boxed': true requires 'data'")
+check_type(args, info, "'data' for command '%s'" % name,
allow_dict=not boxed)
check_type(expr.get('returns'), info,
"'returns' for command '%s'" % name,
@@ -780,10 +782,12 @@ def check_command(expr, info):
def check_event(expr, info):
name = expr['event']
+args = expr.get('data')
boxed = expr.get('boxed', False)
-check_type(expr.get('data'), info,
- "'data' for event '%s'" % name,
+if boxed and args is None:
+raise QAPISemError(info, "'boxed': true requires 'data'")
+check_type(args, info, "'data' for event '%s'" % name,
allow_dict=not boxed)
@@ -1699,8 +1703,6 @@ class QAPISchemaCommand(QAPISchemaEntity):
self.info,
"command's 'data' can take %s only with 'boxed': true"
% self.arg_type.describe())
-elif self.boxed:
-raise QAPISemError(self.info, "use of 'boxed' requires 'data'")
if self._ret_type_name:
self.ret_type = schema.resolve_type(
self._ret_type_name, self.info, "command's 'returns'")
@@ -1748,8 +1750,6 @@ class QAPISchemaEvent(QAPISchemaEntity):
self.info,
"event's 'data' can take %s only with 'boxed': true"
% self.arg_type.describe())
-elif self.boxed:
-raise QAPISemError(self.info, "use of 'boxed' requires 'data'")
def visit(self, visitor):
QAPISchemaEntity.visit(self, visitor)
diff --git a/tests/qapi-schema/event-boxed-empty.err
b/tests/qapi-schema/event-boxed-empty.err
index 9c691b7d97..931c10b036 100644
--- a/tests/qapi-schema/event-boxed-empty.err
+++ b/tests/qapi-schema/event-boxed-empty.err
@@ -1,2 +1,2 @@
tests/qapi-schema/event-boxed-empty.json: In event 'FOO':
-tests/qapi-schema/event-boxed-empty.json:2: use of 'boxed' requires 'data'
+tests/qapi-schema/event-boxed-empty.json:2: 'boxed': true requires 'data'
--
2.21.0
[PULL 09/27] qapi: Reorder check_FOO() parameters for consistency
Most check_FOO() take the thing being checked as first argument.
check_name(), check_type(), check_known_keys() don't. Clean that up.
While there, drop a "Todo" comment that should have been dropped in
commit 87adbbffd4 "qapi: add a dictionary form for TYPE".
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-9-arm...@redhat.com>
---
scripts/qapi/common.py | 80 --
1 file changed, 39 insertions(+), 41 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 14d1e34c2c..c909821560 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -706,7 +706,7 @@ valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
'[a-zA-Z][a-zA-Z0-9_-]*$')
-def check_name(info, source, name,
+def check_name(name, info, source,
allow_optional=False, enum_member=False, permit_upper=False):
global valid_name
membername = name
@@ -734,7 +734,7 @@ def check_name(info, source, name,
def add_name(name, info, meta):
global all_names
-check_name(info, "'%s'" % meta, name, permit_upper=True)
+check_name(name, info, "'%s'" % meta, permit_upper=True)
# FIXME should reject names that differ only in '_' vs. '.'
# vs. '-', because they're liable to clash in generated C.
if name in all_names:
@@ -768,7 +768,7 @@ def check_if(expr, info):
check_if_str(ifcond, info)
-def check_type(info, source, value,
+def check_type(value, info, source,
allow_array=False, allow_dict=False, allow_metas=[]):
global all_names
@@ -806,19 +806,17 @@ def check_type(info, source, value,
# value is a dictionary, check that each member is okay
for (key, arg) in value.items():
-check_name(info, "member of %s" % source, key,
+check_name(key, info, "member of %s" % source,
allow_optional=True, permit_upper=permit_upper)
if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
raise QAPISemError(
info, "member of %s uses reserved name '%s'" % (source, key))
-# Todo: allow dictionaries to represent default values of
-# an optional argument.
-check_known_keys(info, "member '%s' of %s" % (key, source),
- arg, ['type'], ['if'])
+check_known_keys(arg, info, "member '%s' of %s" % (key, source),
+ ['type'], ['if'])
check_if(arg, info)
normalize_if(arg)
-check_type(info, "member '%s' of %s" % (key, source),
- arg['type'], allow_array=True,
+check_type(arg['type'], info, "member '%s' of %s" % (key, source),
+ allow_array=True,
allow_metas=['built-in', 'union', 'alternate', 'struct',
'enum'])
@@ -830,15 +828,15 @@ def check_command(expr, info):
args_meta = ['struct']
if boxed:
args_meta += ['union']
-check_type(info, "'data' for command '%s'" % name,
- expr.get('data'), allow_dict=not boxed,
- allow_metas=args_meta)
+check_type(expr.get('data'), info,
+ "'data' for command '%s'" % name,
+ allow_dict=not boxed, allow_metas=args_meta)
returns_meta = ['union', 'struct']
if name in returns_whitelist:
returns_meta += ['built-in', 'alternate', 'enum']
-check_type(info, "'returns' for command '%s'" % name,
- expr.get('returns'), allow_array=True,
- allow_metas=returns_meta)
+check_type(expr.get('returns'), info,
+ "'returns' for command '%s'" % name,
+ allow_array=True, allow_metas=returns_meta)
def check_event(expr, info):
@@ -848,9 +846,9 @@ def check_event(expr, info):
meta = ['struct']
if boxed:
meta += ['union']
-check_type(info, "'data' for event '%s'" % name,
- expr.get('data'), allow_dict=not boxed,
- allow_metas=meta)
+check_type(expr.get('data'), info,
+ "'data' for event '%s'" % name,
+ allow_dict=not boxed, allow_metas=meta)
def enum_get_names(expr):
@@ -876,9 +874,8 @@ def check_union(expr, info):
# Else, it's a flat union.
else:
# The object must have a string or dictionary 'base'.
-check_type(info, "'base' for union '%s'" % name,
- base, allow_dict=name,
- allow_metas=['struct'])
+check_type(base, info, "'base' for union '%s'" % name,
+ allow_dict=name, allow_metas=['struct'])
if not base:
raise QAPISemError(
info, "flat union '%s' must have a base" % name)
@@ -887,8 +884,8 @@ def check_union(expr, info):
# The value of member 'discriminator' must name a non-optional
# member of the base struct.
-check_name(info, "discriminator of flat un
[PULL 23/27] qapi: Improve reporting of invalid 'if' further
check_if()'s errors don't point to the offending part of the
expression. For instance:
tests/qapi-schema/alternate-branch-if-invalid.json:2: 'if' condition ' '
makes no sense
Other check_FOO() do, with the help of a @source argument. Make
check_if() do that, too. The example above improves to:
tests/qapi-schema/alternate-branch-if-invalid.json:2: 'if' condition ' ' of
'data' member 'branch' makes no sense
Signed-off-by: Markus Armbruster
Message-Id: <20190927134639.4284-23-arm...@redhat.com>
Reviewed-by: Eric Blake
---
scripts/qapi/common.py| 27 +++
.../alternate-branch-if-invalid.err | 2 +-
tests/qapi-schema/bad-if-empty-list.err | 2 +-
tests/qapi-schema/bad-if-empty.err| 2 +-
tests/qapi-schema/bad-if-list.err | 2 +-
tests/qapi-schema/bad-if.err | 2 +-
tests/qapi-schema/enum-if-invalid.err | 2 +-
tests/qapi-schema/features-if-invalid.err | 2 +-
.../qapi-schema/struct-member-if-invalid.err | 2 +-
tests/qapi-schema/union-branch-if-invalid.err | 2 +-
10 files changed, 25 insertions(+), 20 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 81c217cd60..4bc8c807aa 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -692,22 +692,27 @@ def check_defn_name_str(name, info, meta):
info, "%s name should not end in '%s'" % (meta, name[-4:]))
-def check_if(expr, info):
+def check_if(expr, info, source):
def check_if_str(ifcond, info):
if not isinstance(ifcond, str):
raise QAPISemError(
-info, "'if' condition must be a string or a list of strings")
+info,
+"'if' condition of %s must be a string or a list of strings"
+% source)
if ifcond.strip() == '':
-raise QAPISemError(info, "'if' condition '%s' makes no sense"
- % ifcond)
+raise QAPISemError(
+info,
+"'if' condition '%s' of %s makes no sense"
+% (ifcond, source))
ifcond = expr.get('if')
if ifcond is None:
return
if isinstance(ifcond, list):
if ifcond == []:
-raise QAPISemError(info, "'if' condition [] is useless")
+raise QAPISemError(
+info, "'if' condition [] of %s is useless" % source)
for elt in ifcond:
check_if_str(elt, info)
else:
@@ -752,7 +757,7 @@ def check_type(value, info, source,
if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
raise QAPISemError(info, "%s uses reserved name" % key_source)
check_known_keys(arg, info, key_source, ['type'], ['if'])
-check_if(arg, info)
+check_if(arg, info, key_source)
normalize_if(arg)
check_type(arg['type'], info, key_source, allow_array=True)
@@ -796,7 +801,7 @@ def check_union(expr, info):
source = "'data' member '%s'" % key
check_name_str(key, info, source)
check_known_keys(value, info, source, ['type'], ['if'])
-check_if(value, info)
+check_if(value, info, source)
normalize_if(value)
check_type(value['type'], info, source, allow_array=not base)
@@ -810,7 +815,7 @@ def check_alternate(expr, info):
source = "'data' member '%s'" % key
check_name_str(key, info, source)
check_known_keys(value, info, source, ['type'], ['if'])
-check_if(value, info)
+check_if(value, info, source)
normalize_if(value)
check_type(value['type'], info, source)
@@ -834,7 +839,7 @@ def check_enum(expr, info):
source = "%s '%s'" % (source, member['name'])
check_name_str(member['name'], info, source,
enum_member=True, permit_upper=permit_upper)
-check_if(member, info)
+check_if(member, info, source)
normalize_if(member)
@@ -856,7 +861,7 @@ def check_struct(expr, info):
check_name_is_str(f['name'], info, source)
source = "%s '%s'" % (source, f['name'])
check_name_str(f['name'], info, source)
-check_if(f, info)
+check_if(f, info, source)
normalize_if(f)
@@ -994,7 +999,7 @@ def check_exprs(exprs):
assert False, 'unexpected meta type'
normalize_if(expr)
-check_if(expr, info)
+check_if(expr, info, meta)
check_flags(expr, info)
if doc:
diff --git a/tests/qapi-schema/alternate-branch-if-invalid.err
b/tests/qapi-schema/alternate-branch-if-invalid.err
index 8684829aca..6c68e5a922 100644
--- a/tests/qapi-schema/alternate-branch-if-invalid.err
+++ b/tests/qapi-schema/alternate-branch-if-invalid.err
@@ -1,2 +1,2 @@
tests/qapi-schema/alternate-branch-if-invalid.json: In alternate 'Alt':
-tests/qapi-schema/alternate-branch-if-invalid.json
[PULL 27/27] qapi: Improve source file read error handling
qapi-gen.py crashes when it can't open the main schema file, and when
it can't read from any schema file. Lazy.
Change QAPISchema.__init__() to take a file name instead of a file
object. Move the open code from _include() to __init__(), so it's
used for the main schema file, too.
Move the read into the try for good measure, and rephrase the error
message.
Reporting open or read failure for the main schema file needs a
QAPISourceInfo representing "no source". Make QAPISourceInfo cope
with fname=None.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-27-arm...@redhat.com>
---
scripts/qapi/common.py| 46 +++
tests/qapi-schema/include-no-file.err | 2 +-
2 files changed, 27 insertions(+), 21 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index a74cd957d4..d6e00c80ea 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -53,7 +53,12 @@ class QAPISourceInfo(object):
return info
def loc(self):
-return '%s:%d' % (self.fname, self.line)
+if self.fname is None:
+return sys.argv[0]
+ret = self.fname
+if self.line is not None:
+ret += ':%d' % self.line
+return ret
def in_defn(self):
if self.defn_name:
@@ -383,14 +388,26 @@ class QAPIDoc(object):
class QAPISchemaParser(object):
-def __init__(self, fp, previously_included=[], incl_info=None):
-self.fname = fp.name
-previously_included.append(os.path.abspath(fp.name))
-self.src = fp.read()
+def __init__(self, fname, previously_included=[], incl_info=None):
+previously_included.append(os.path.abspath(fname))
+
+try:
+if sys.version_info[0] >= 3:
+fp = open(fname, 'r', encoding='utf-8')
+else:
+fp = open(fname, 'r')
+self.src = fp.read()
+except IOError as e:
+raise QAPISemError(incl_info or QAPISourceInfo(None, None, None),
+ "can't read %s file '%s': %s"
+ % ("include" if incl_info else "schema",
+ fname,
+ e.strerror))
+
if self.src == '' or self.src[-1] != '\n':
self.src += '\n'
self.cursor = 0
-self.info = QAPISourceInfo(self.fname, 1, incl_info)
+self.info = QAPISourceInfo(fname, 1, incl_info)
self.line_pos = 0
self.exprs = []
self.docs = []
@@ -414,7 +431,7 @@ class QAPISchemaParser(object):
if not isinstance(include, str):
raise QAPISemError(info,
"value of 'include' must be a string")
-incl_fname = os.path.join(os.path.dirname(self.fname),
+incl_fname = os.path.join(os.path.dirname(fname),
include)
self.exprs.append({'expr': {'include': incl_fname},
'info': info})
@@ -466,14 +483,7 @@ class QAPISchemaParser(object):
if incl_abs_fname in previously_included:
return None
-try:
-if sys.version_info[0] >= 3:
-fobj = open(incl_fname, 'r', encoding='utf-8')
-else:
-fobj = open(incl_fname, 'r')
-except IOError as e:
-raise QAPISemError(info, "%s: %s" % (e.strerror, incl_fname))
-return QAPISchemaParser(fobj, previously_included, info)
+return QAPISchemaParser(incl_fname, previously_included, info)
def _pragma(self, name, value, info):
global doc_required, returns_whitelist, name_case_whitelist
@@ -1734,11 +1744,7 @@ class QAPISchemaEvent(QAPISchemaEntity):
class QAPISchema(object):
def __init__(self, fname):
self.fname = fname
-if sys.version_info[0] >= 3:
-f = open(fname, 'r', encoding='utf-8')
-else:
-f = open(fname, 'r')
-parser = QAPISchemaParser(f)
+parser = QAPISchemaParser(fname)
exprs = check_exprs(parser.exprs)
self.docs = parser.docs
self._entity_list = []
diff --git a/tests/qapi-schema/include-no-file.err
b/tests/qapi-schema/include-no-file.err
index e42bcf4bc1..0a6c6bb4a9 100644
--- a/tests/qapi-schema/include-no-file.err
+++ b/tests/qapi-schema/include-no-file.err
@@ -1 +1 @@
-tests/qapi-schema/include-no-file.json:1: No such file or directory:
tests/qapi-schema/include-no-file-sub.json
+tests/qapi-schema/include-no-file.json:1: can't read include file
'tests/qapi-schema/include-no-file-sub.json': No such file or directory
--
2.21.0
[PULL 21/27] qapi: Improve reporting of missing / unknown definition keys
Have check_exprs() call check_keys() later, so its error messages gain
an "in definition" line.
Both check_keys() and check_name_is_str() check the definition's name
is a string. Since check_keys() now runs after check_name_is_str()
rather than before, its check is dead. Bury it. Checking values in
check_keys() is unclean anyway.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-21-arm...@redhat.com>
---
scripts/qapi/common.py | 40 -
tests/qapi-schema/alternate-base.err| 1 +
tests/qapi-schema/bad-type-bool.err | 2 +-
tests/qapi-schema/bad-type-dict.err | 2 +-
tests/qapi-schema/double-type.err | 1 +
tests/qapi-schema/enum-missing-data.err | 1 +
tests/qapi-schema/unknown-expr-key.err | 1 +
7 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 4f67b73684..42b9c2e36b 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -910,8 +910,6 @@ def check_known_keys(value, info, source, required,
optional):
def check_keys(expr, info, meta, required, optional=[]):
name = expr[meta]
-if not isinstance(name, str):
-raise QAPISemError(info, "'%s' key must have a string value" % meta)
required = required + [meta]
source = "%s '%s'" % (meta, name)
check_known_keys(expr, info, source, required, optional)
@@ -969,37 +967,18 @@ def check_exprs(exprs):
if 'enum' in expr:
meta = 'enum'
-check_keys(expr, info, 'enum', ['data'], ['if', 'prefix'])
-normalize_enum(expr)
elif 'union' in expr:
meta = 'union'
-check_keys(expr, info, 'union', ['data'],
- ['base', 'discriminator', 'if'])
-normalize_members(expr.get('base'))
-normalize_members(expr['data'])
elif 'alternate' in expr:
meta = 'alternate'
-check_keys(expr, info, 'alternate', ['data'], ['if'])
-normalize_members(expr['data'])
elif 'struct' in expr:
meta = 'struct'
-check_keys(expr, info, 'struct', ['data'],
- ['base', 'if', 'features'])
-normalize_members(expr['data'])
-normalize_features(expr.get('features'))
elif 'command' in expr:
meta = 'command'
-check_keys(expr, info, 'command', [],
- ['data', 'returns', 'gen', 'success-response',
-'boxed', 'allow-oob', 'allow-preconfig', 'if'])
-normalize_members(expr.get('data'))
elif 'event' in expr:
meta = 'event'
-check_keys(expr, info, 'event', [], ['data', 'boxed', 'if'])
-normalize_members(expr.get('data'))
else:
raise QAPISemError(info, "expression is missing metatype")
-normalize_if(expr)
name = expr[meta]
check_name_is_str(name, info, "'%s'" % meta)
@@ -1013,20 +992,39 @@ def check_exprs(exprs):
% (name, doc.symbol))
if meta == 'enum':
+check_keys(expr, info, 'enum', ['data'], ['if', 'prefix'])
+normalize_enum(expr)
check_enum(expr, info)
elif meta == 'union':
+check_keys(expr, info, 'union', ['data'],
+ ['base', 'discriminator', 'if'])
+normalize_members(expr.get('base'))
+normalize_members(expr['data'])
check_union(expr, info)
elif meta == 'alternate':
+check_keys(expr, info, 'alternate', ['data'], ['if'])
+normalize_members(expr['data'])
check_alternate(expr, info)
elif meta == 'struct':
+check_keys(expr, info, 'struct', ['data'],
+ ['base', 'if', 'features'])
+normalize_members(expr['data'])
+normalize_features(expr.get('features'))
check_struct(expr, info)
elif meta == 'command':
+check_keys(expr, info, 'command', [],
+ ['data', 'returns', 'gen', 'success-response',
+'boxed', 'allow-oob', 'allow-preconfig', 'if'])
+normalize_members(expr.get('data'))
check_command(expr, info)
elif meta == 'event':
+check_keys(expr, info, 'event', [], ['data', 'boxed', 'if'])
+normalize_members(expr.get('data'))
check_event(expr, info)
else:
assert False, 'unexpected meta type'
+normalize_if(expr)
check_if(expr, info)
check_flags(expr, info)
diff --git a/tests/qapi-schema/alternate-base.err
b/tests/qapi-schema/alternate-base.err
index 4c9158db02..6290665ac2 100644
--- a/tests/qapi-schema/alternate-base.err
+++ b/tests/qapi-schema/alternate-base.err
@@ -1,2 +1,3 @@
+tests/qapi-schema/alternate-base.json: In alternate 'Alt':
[PULL 08/27] qapi: Improve reporting of member name clashes
We report name clashes like this:
struct-base-clash.json: In struct 'Sub':
struct-base-clash.json:5: 'name' (member of Sub) collides with 'name'
(member of Base)
The "(member of Sub)" is redundant with "In struct 'Sub'". Comes from
QAPISchemaMember.describe(). Pass info to it, so it can detect the
redundancy and avoid it. Result:
struct-base-clash.json: In struct 'Sub':
struct-base-clash.json:5: member 'name' collides with member 'name' of type
'Base'
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-8-arm...@redhat.com>
---
scripts/qapi/common.py| 36 ---
tests/qapi-schema/alternate-clash.err | 2 +-
tests/qapi-schema/args-name-clash.err | 2 +-
tests/qapi-schema/enum-clash-member.err | 2 +-
tests/qapi-schema/features-duplicate-name.err | 2 +-
tests/qapi-schema/flat-union-bad-base.err | 2 +-
tests/qapi-schema/flat-union-clash-member.err | 2 +-
tests/qapi-schema/struct-base-clash-deep.err | 2 +-
tests/qapi-schema/struct-base-clash.err | 2 +-
tests/qapi-schema/union-clash-branches.err| 2 +-
10 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 3d73332487..14d1e34c2c 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1575,31 +1575,41 @@ class QAPISchemaMember(object):
def check_clash(self, info, seen):
cname = c_name(self.name)
if cname in seen:
-raise QAPISemError(info, "%s collides with %s" %
- (self.describe(), seen[cname].describe()))
+raise QAPISemError(
+info,
+"%s collides with %s"
+% (self.describe(info), seen[cname].describe(info)))
seen[cname] = self
-def _pretty_defined_in(self):
+def describe(self, info):
+role = self.role
defined_in = self.defined_in
+assert defined_in
+
if defined_in.startswith('q_obj_'):
# See QAPISchema._make_implicit_object_type() - reverse the
# mapping there to create a nice human-readable description
defined_in = defined_in[6:]
if defined_in.endswith('-arg'):
-return '(parameter of %s)' % defined_in[:-4]
+# Implicit type created for a command's dict 'data'
+assert role == 'member'
+role = 'parameter'
elif defined_in.endswith('-base'):
-return '(base of %s)' % defined_in[:-5]
+# Implicit type created for a flat union's dict 'base'
+role = 'base ' + role
else:
+# Implicit type created for a simple union's branch
assert defined_in.endswith('-wrapper')
# Unreachable and not implemented
assert False
-if defined_in.endswith('Kind'):
+elif defined_in.endswith('Kind'):
# See QAPISchema._make_implicit_enum_type()
-return '(branch of %s)' % defined_in[:-4]
-return '(%s of %s)' % (self.role, defined_in)
-
-def describe(self):
-return "'%s' %s" % (self.name, self._pretty_defined_in())
+# Implicit enum created for simple union's branches
+assert role == 'value'
+role = 'branch'
+elif defined_in != info.defn_name:
+return "%s '%s' of type '%s'" % (role, self.name, defined_in)
+return "%s '%s'" % (role, self.name)
class QAPISchemaEnumMember(QAPISchemaMember):
@@ -1871,7 +1881,7 @@ class QAPISchema(object):
for v in values]
def _make_implicit_enum_type(self, name, info, ifcond, values):
-# See also QAPISchemaObjectTypeMember._pretty_defined_in()
+# See also QAPISchemaObjectTypeMember.describe()
name = name + 'Kind' # Use namespace reserved by add_name()
self._def_entity(QAPISchemaEnumType(
name, info, None, ifcond, self._make_enum_members(values), None))
@@ -1887,7 +1897,7 @@ class QAPISchema(object):
role, members):
if not members:
return None
-# See also QAPISchemaObjectTypeMember._pretty_defined_in()
+# See also QAPISchemaObjectTypeMember.describe()
name = 'q_obj_%s-%s' % (name, role)
typ = self.lookup_entity(name, QAPISchemaObjectType)
if typ:
diff --git a/tests/qapi-schema/alternate-clash.err
b/tests/qapi-schema/alternate-clash.err
index 426ff6a7c4..73a52d69d1 100644
--- a/tests/qapi-schema/alternate-clash.err
+++ b/tests/qapi-schema/alternate-clash.err
@@ -1,2 +1,2 @@
tests/qapi-schema/alternate-clash.json: In alternate 'Alt1':
-tests/qapi-schema/alternate-clash.json:7: 'a_b' (branch of Alt1) collides with
'a-b' (branch of Alt1)
+tests/qapi-schema/alternate-clash.json:7: branch 'a_b' collides with branch
'a-b'
di
Re: [PATCH v25 00/22] Add RX archtecture support
Patchew URL:
https://patchew.org/QEMU/20190927062302.110144-1-ys...@users.sourceforge.jp/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20190927062302.110144-1-ys...@users.sourceforge.jp
Subject: [PATCH v25 00/22] Add RX archtecture support
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
dadff51 qapi/machine.json: Add RX cpu.
ebb6621 BootLinuxConsoleTest: Test the RX-Virt machine
f81a4dd Add rx-softmmu
91354fd hw/rx: Restrict the RX62N microcontroller to the RX62N CPU core
6703279 hw/rx: Honor -accel qtest
86d0724 hw/rx: RX Target hardware definition
f85dbca hw/char: RX62N serial communication interface (SCI)
0aac3dc hw/timer: RX62N internal timer modules
189c157 hw/intc: RX62N interrupt controller (ICUa)
f4e110a target/rx: Dump bytes for each insn during disassembly
68f9b3c target/rx: Collect all bytes during disassembly
c99e307 target/rx: Emit all disassembly in one prt()
afb3c6f target/rx: Use prt_ldmi for XCHG_mr disassembly
a2ff9b7 target/rx: Replace operand with prt_ldmi in disassembler
2753df2 target/rx: Disassemble rx_index_addr into a string
6c571d3 target/rx: RX disassembler
149fc98 target/rx: CPU definition
1c56e9d target/rx: TCG helper
33ec1bb target/rx: TCG translation
6dcb4ce hw/registerfields.h: Add 8bit and 16bit register macros
15439bc qemu/bitops.h: Add extract8 and extract16
6e3543e MAINTAINERS: Add RX
=== OUTPUT BEGIN ===
1/22 Checking commit 6e3543eb958d (MAINTAINERS: Add RX)
2/22 Checking commit 15439bc03d71 (qemu/bitops.h: Add extract8 and extract16)
3/22 Checking commit 6dcb4ce925f7 (hw/registerfields.h: Add 8bit and 16bit
register macros)
Use of uninitialized value in concatenation (.) or string at
./scripts/checkpatch.pl line 2484.
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#27: FILE: include/hw/registerfields.h:25:
+#define REG8(reg, addr) \
+enum { A_ ## reg = (addr) }; \
+enum { R_ ## reg = (addr) };
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#31: FILE: include/hw/registerfields.h:29:
+#define REG16(reg, addr) \
+enum { A_ ## reg = (addr) }; \
+enum { R_ ## reg = (addr) / 2 };
total: 2 errors, 0 warnings, 56 lines checked
Patch 3/22 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/22 Checking commit 33ec1bbabfae (target/rx: TCG translation)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#20:
new file mode 100644
total: 0 errors, 1 warnings, 3065 lines checked
Patch 4/22 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/22 Checking commit 1c56e9df0bda (target/rx: TCG helper)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#21:
new file mode 100644
total: 0 errors, 1 warnings, 650 lines checked
Patch 5/22 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/22 Checking commit 149fc989a0b3 (target/rx: CPU definition)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#38:
new file mode 100644
total: 0 errors, 1 warnings, 588 lines checked
Patch 6/22 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/22 Checking commit 6c571d3b6a2f (target/rx: RX disassembler)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#38:
new file mode 100644
total: 0 errors, 1 warnings, 1497 lines checked
Patch 7/22 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
8/22 Checking commit 2753df288343 (target/rx: Disassemble rx_index_addr into a
string)
9/22 Checking commit a2ff9b7af426 (target/rx: Replace operand with prt_ldmi in
disassembler)
10/22 Checking commit afb3c6f62e73 (target/rx: Use prt_ldmi for XCHG_mr
disassembly)
11/22 Checking commit c99e3071a323 (target/rx: Emit all disassembly in one
prt())
12/22 Checking commit 68f9b3cbfd59 (target/rx: Collect all bytes during
disassembly)
13/22 Checking commit f4e110a34345 (target/rx: Dump bytes for each insn during
disassembly)
14/22 Checking commit 189c15760991 (hw/intc: RX62N interrupt controller (ICUa))
WARNING: added, moved or deleted file
[PULL 26/27] qapi: Improve reporting of redefinition
Point to the previous definition, unless it's a built-in. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-26-arm...@redhat.com> --- scripts/qapi/common.py | 5 + tests/qapi-schema/redefined-command.err | 4 +++- tests/qapi-schema/redefined-event.err | 4 +++- tests/qapi-schema/redefined-type.err| 4 +++- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index bd834270f8..a74cd957d4 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -1759,6 +1759,11 @@ class QAPISchema(object): # because they're liable to clash in generated C. other_ent = self._entity_dict.get(ent.name) if other_ent: +if other_ent.info: +where = QAPIError(other_ent.info, None, "previous definition") +raise QAPISemError( +ent.info, +"'%s' is already defined\n%s" % (ent.name, where)) raise QAPISemError( ent.info, "%s is already defined" % other_ent.describe()) self._entity_dict[ent.name] = ent diff --git a/tests/qapi-schema/redefined-command.err b/tests/qapi-schema/redefined-command.err index b77a05d354..54e366bbf3 100644 --- a/tests/qapi-schema/redefined-command.err +++ b/tests/qapi-schema/redefined-command.err @@ -1,2 +1,4 @@ tests/qapi-schema/redefined-command.json: In command 'foo': -tests/qapi-schema/redefined-command.json:3: command 'foo' is already defined +tests/qapi-schema/redefined-command.json:3: 'foo' is already defined +tests/qapi-schema/redefined-command.json: In command 'foo': +tests/qapi-schema/redefined-command.json:2: previous definition diff --git a/tests/qapi-schema/redefined-event.err b/tests/qapi-schema/redefined-event.err index fd02d38157..606c6e4497 100644 --- a/tests/qapi-schema/redefined-event.err +++ b/tests/qapi-schema/redefined-event.err @@ -1,2 +1,4 @@ tests/qapi-schema/redefined-event.json: In event 'EVENT_A': -tests/qapi-schema/redefined-event.json:3: event 'EVENT_A' is already defined +tests/qapi-schema/redefined-event.json:3: 'EVENT_A' is already defined +tests/qapi-schema/redefined-event.json: In event 'EVENT_A': +tests/qapi-schema/redefined-event.json:2: previous definition diff --git a/tests/qapi-schema/redefined-type.err b/tests/qapi-schema/redefined-type.err index 39f51c14ea..77786f98ae 100644 --- a/tests/qapi-schema/redefined-type.err +++ b/tests/qapi-schema/redefined-type.err @@ -1,2 +1,4 @@ tests/qapi-schema/redefined-type.json: In enum 'foo': -tests/qapi-schema/redefined-type.json:3: struct type 'foo' is already defined +tests/qapi-schema/redefined-type.json:3: 'foo' is already defined +tests/qapi-schema/redefined-type.json: In struct 'foo': +tests/qapi-schema/redefined-type.json:2: previous definition -- 2.21.0
[PULL 24/27] qapi: Eliminate check_keys(), rename check_known_keys()
check_keys() has become a trivial wrapper for check_known_keys().
Eliminate it.
This makes its name available. Rename check_known_keys().
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-24-arm...@redhat.com>
---
scripts/qapi/common.py | 40 +---
1 file changed, 21 insertions(+), 19 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 4bc8c807aa..fa354b3f1e 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -756,7 +756,7 @@ def check_type(value, info, source,
allow_optional=True, permit_upper=permit_upper)
if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
raise QAPISemError(info, "%s uses reserved name" % key_source)
-check_known_keys(arg, info, key_source, ['type'], ['if'])
+check_keys(arg, info, key_source, ['type'], ['if'])
check_if(arg, info, key_source)
normalize_if(arg)
check_type(arg['type'], info, key_source, allow_array=True)
@@ -800,7 +800,7 @@ def check_union(expr, info):
for (key, value) in members.items():
source = "'data' member '%s'" % key
check_name_str(key, info, source)
-check_known_keys(value, info, source, ['type'], ['if'])
+check_keys(value, info, source, ['type'], ['if'])
check_if(value, info, source)
normalize_if(value)
check_type(value['type'], info, source, allow_array=not base)
@@ -814,7 +814,7 @@ def check_alternate(expr, info):
for (key, value) in members.items():
source = "'data' member '%s'" % key
check_name_str(key, info, source)
-check_known_keys(value, info, source, ['type'], ['if'])
+check_keys(value, info, source, ['type'], ['if'])
check_if(value, info, source)
normalize_if(value)
check_type(value['type'], info, source)
@@ -834,7 +834,7 @@ def check_enum(expr, info):
for member in members:
source = "'data' member"
-check_known_keys(member, info, source, ['name'], ['if'])
+check_keys(member, info, source, ['name'], ['if'])
check_name_is_str(member['name'], info, source)
source = "%s '%s'" % (source, member['name'])
check_name_str(member['name'], info, source,
@@ -857,7 +857,7 @@ def check_struct(expr, info):
for f in features:
source = "'features' member"
assert isinstance(f, dict)
-check_known_keys(f, info, source, ['name'], ['if'])
+check_keys(f, info, source, ['name'], ['if'])
check_name_is_str(f['name'], info, source)
source = "%s '%s'" % (source, f['name'])
check_name_str(f['name'], info, source)
@@ -865,7 +865,7 @@ def check_struct(expr, info):
normalize_if(f)
-def check_known_keys(value, info, source, required, optional):
+def check_keys(value, info, source, required, optional):
def pprint(elems):
return ', '.join("'" + e + "'" for e in sorted(elems))
@@ -887,10 +887,6 @@ def check_known_keys(value, info, source, required,
optional):
pprint(unknown), pprint(allowed)))
-def check_keys(expr, info, meta, required, optional=[]):
-check_known_keys(expr, info, meta, required + [meta], optional)
-
-
def check_flags(expr, info):
for key in ['gen', 'success-response']:
if key in expr and expr[key] is not False:
@@ -966,33 +962,39 @@ def check_exprs(exprs):
info, "documentation comment is for '%s'" % doc.symbol)
if meta == 'enum':
-check_keys(expr, info, 'enum', ['data'], ['if', 'prefix'])
+check_keys(expr, info, meta,
+ ['enum', 'data'], ['if', 'prefix'])
normalize_enum(expr)
check_enum(expr, info)
elif meta == 'union':
-check_keys(expr, info, 'union', ['data'],
+check_keys(expr, info, meta,
+ ['union', 'data'],
['base', 'discriminator', 'if'])
normalize_members(expr.get('base'))
normalize_members(expr['data'])
check_union(expr, info)
elif meta == 'alternate':
-check_keys(expr, info, 'alternate', ['data'], ['if'])
+check_keys(expr, info, meta,
+ ['alternate', 'data'], ['if'])
normalize_members(expr['data'])
check_alternate(expr, info)
elif meta == 'struct':
-check_keys(expr, info, 'struct', ['data'],
- ['base', 'if', 'features'])
+check_keys(expr, info, meta,
+ ['struct', 'data'], ['base', 'if', 'features'])
normalize_members(expr['data'])
normalize_features(expr.get('features'))
check_struct(expr, info)
elif meta == 'command':
-check_keys(expr, info, 'comman
[PATCH] configure: Remove s390 (31-bit mode) from the list of supported CPUs
On IBM Z, KVM in the kernel is only implemented for 64-bit mode, and with regards to TCG, we also only support 64-bit host CPUs (see the check at the beginning of tcg/s390/tcg-target.inc.c), so we should remove s390 (without "x", i.e. the old 31-bit mode CPUs) from the list of supported CPUs. Signed-off-by: Thomas Huth --- configure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure b/configure index 397bb476e1..a4488c6705 100755 --- a/configure +++ b/configure @@ -728,7 +728,7 @@ ARCH= # Normalise host CPU name and set ARCH. # Note that this case should only have supported host CPUs, not guests. case "$cpu" in - ppc|ppc64|s390|s390x|sparc64|x32|riscv32|riscv64) + ppc|ppc64|s390x|sparc64|x32|riscv32|riscv64) supported_cpu="yes" ;; ppc64le) -- 2.18.1
[PULL 05/27] qapi: Prefix frontend errors with an "in definition" line
We take pains to include the offending expression in error messages, e.g. tests/qapi-schema/alternate-any.json:2: alternate 'Alt' member 'one' cannot use type 'any' But not always: tests/qapi-schema/enum-if-invalid.json:2: 'if' condition must be a string or a list of strings Instead of improving them one by one, report the offending expression whenever it is known, like this: tests/qapi-schema/enum-if-invalid.json: In enum 'TestIfEnum': tests/qapi-schema/enum-if-invalid.json:2: 'if' condition must be a string or a list of strings Error messages that mention the offending expression become a bit redundant, e.g. tests/qapi-schema/alternate-any.json: In alternate 'Alt': tests/qapi-schema/alternate-any.json:2: alternate 'Alt' member 'one' cannot use type 'any' I'll take care of that later in this series. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-5-arm...@redhat.com> --- scripts/qapi/common.py| 15 ++- tests/qapi-schema/alternate-any.err | 1 + tests/qapi-schema/alternate-array.err | 1 + tests/qapi-schema/alternate-branch-if-invalid.err | 1 + tests/qapi-schema/alternate-clash.err | 1 + .../alternate-conflict-bool-string.err| 1 + tests/qapi-schema/alternate-conflict-dict.err | 1 + .../qapi-schema/alternate-conflict-enum-bool.err | 1 + tests/qapi-schema/alternate-conflict-enum-int.err | 1 + .../qapi-schema/alternate-conflict-num-string.err | 1 + tests/qapi-schema/alternate-conflict-string.err | 1 + tests/qapi-schema/alternate-empty.err | 1 + tests/qapi-schema/alternate-invalid-dict.err | 1 + tests/qapi-schema/alternate-nested.err| 1 + tests/qapi-schema/alternate-unknown.err | 1 + tests/qapi-schema/args-alternate.err | 1 + tests/qapi-schema/args-any.err| 1 + tests/qapi-schema/args-array-empty.err| 1 + tests/qapi-schema/args-array-unknown.err | 1 + tests/qapi-schema/args-boxed-anon.err | 1 + tests/qapi-schema/args-boxed-string.err | 1 + tests/qapi-schema/args-int.err| 1 + tests/qapi-schema/args-invalid.err| 1 + tests/qapi-schema/args-member-array-bad.err | 1 + tests/qapi-schema/args-member-case.err| 1 + tests/qapi-schema/args-member-unknown.err | 1 + tests/qapi-schema/args-name-clash.err | 1 + tests/qapi-schema/args-union.err | 1 + tests/qapi-schema/args-unknown.err| 1 + tests/qapi-schema/bad-base.err| 1 + tests/qapi-schema/bad-data.err| 1 + tests/qapi-schema/base-cycle-direct.err | 1 + tests/qapi-schema/base-cycle-indirect.err | 1 + tests/qapi-schema/doc-bad-symbol.err | 1 + tests/qapi-schema/enum-bad-member.err | 1 + tests/qapi-schema/enum-bad-name.err | 1 + tests/qapi-schema/enum-bad-prefix.err | 1 + tests/qapi-schema/enum-clash-member.err | 1 + tests/qapi-schema/enum-dict-member-unknown.err| 1 + tests/qapi-schema/enum-if-invalid.err | 1 + tests/qapi-schema/enum-member-case.err| 1 + tests/qapi-schema/enum-wrong-data.err | 1 + tests/qapi-schema/event-boxed-empty.err | 1 + tests/qapi-schema/event-member-invalid-dict.err | 1 + tests/qapi-schema/event-nest-struct.err | 1 + tests/qapi-schema/features-bad-type.err | 1 + tests/qapi-schema/features-duplicate-name.err | 1 + tests/qapi-schema/features-if-invalid.err | 1 + tests/qapi-schema/features-missing-name.err | 1 + tests/qapi-schema/features-name-bad-type.err | 1 + tests/qapi-schema/features-no-list.err| 1 + tests/qapi-schema/features-unknown-key.err| 1 + tests/qapi-schema/flat-union-array-branch.err | 1 + tests/qapi-schema/flat-union-bad-base.err | 1 + .../qapi-schema/flat-union-bad-discriminator.err | 1 + tests/qapi-schema/flat-union-base-any.err | 1 + tests/qapi-schema/flat-union-base-union.err | 1 + tests/qapi-schema/flat-union-clash-member.err | 1 + .../flat-union-discriminator-bad-name.err | 1 + tests/qapi-schema/flat-union-empty.err| 1 + .../flat-union-inline-invalid-dict.err| 1 + tests/qapi-schema/flat-union-inline.err | 1 + tests/qapi-schema/flat-union-int-branch.err | 1 + .../qapi-schema/flat-union-invalid-branch-key.err | 1 + .../flat-union-invalid-discriminator.err | 1 + .../flat-union-invalid-if-discriminator.err | 1 + tests/qapi-schema/flat-union-no-base.err | 1 + .../flat-union-optional-discriminator.err | 1 + .../flat-union-string-discriminator.err | 1 + .../nested-struc
[PULL 22/27] qapi: Avoid redundant definition references in error messages
Many error messages refer to the offending definition even though
they're preceded by an "in definition" line. Rephrase them.
Signed-off-by: Markus Armbruster
Message-Id: <20190927134639.4284-22-arm...@redhat.com>
Reviewed-by: Eric Blake
---
scripts/qapi/common.py| 129 +++---
tests/qapi-schema/alternate-array.err | 2 +-
tests/qapi-schema/alternate-base.err | 2 +-
tests/qapi-schema/alternate-empty.err | 2 +-
tests/qapi-schema/alternate-invalid-dict.err | 2 +-
tests/qapi-schema/args-array-empty.err| 2 +-
tests/qapi-schema/args-boxed-anon.err | 2 +-
tests/qapi-schema/args-invalid.err| 2 +-
tests/qapi-schema/args-member-array-bad.err | 2 +-
tests/qapi-schema/args-member-case.err| 2 +-
tests/qapi-schema/bad-data.err| 2 +-
tests/qapi-schema/bad-ident.err | 2 +-
tests/qapi-schema/doc-bad-symbol.err | 2 +-
tests/qapi-schema/double-type.err | 2 +-
tests/qapi-schema/enum-bad-member.err | 2 +-
tests/qapi-schema/enum-bad-name.err | 2 +-
tests/qapi-schema/enum-bad-prefix.err | 2 +-
.../qapi-schema/enum-dict-member-unknown.err | 2 +-
tests/qapi-schema/enum-member-case.err| 2 +-
tests/qapi-schema/enum-missing-data.err | 2 +-
tests/qapi-schema/enum-wrong-data.err | 2 +-
.../qapi-schema/event-member-invalid-dict.err | 2 +-
tests/qapi-schema/event-nest-struct.err | 2 +-
tests/qapi-schema/features-bad-type.err | 2 +-
tests/qapi-schema/features-missing-name.err | 2 +-
tests/qapi-schema/features-name-bad-type.err | 2 +-
tests/qapi-schema/features-no-list.err| 2 +-
tests/qapi-schema/features-unknown-key.err| 2 +-
tests/qapi-schema/flat-union-array-branch.err | 2 +-
.../flat-union-bad-discriminator.err | 2 +-
.../flat-union-inline-invalid-dict.err| 2 +-
tests/qapi-schema/flat-union-inline.err | 2 +-
tests/qapi-schema/flat-union-no-base.err | 2 +-
.../nested-struct-data-invalid-dict.err | 2 +-
tests/qapi-schema/nested-struct-data.err | 2 +-
tests/qapi-schema/reserved-command-q.err | 2 +-
tests/qapi-schema/reserved-enum-q.err | 2 +-
tests/qapi-schema/reserved-member-has.err | 2 +-
tests/qapi-schema/reserved-member-q.err | 2 +-
tests/qapi-schema/reserved-member-u.err | 2 +-
.../reserved-member-underscore.err| 2 +-
tests/qapi-schema/reserved-type-kind.err | 2 +-
tests/qapi-schema/reserved-type-list.err | 2 +-
tests/qapi-schema/returns-array-bad.err | 2 +-
tests/qapi-schema/returns-dict.err| 2 +-
tests/qapi-schema/struct-data-invalid.err | 2 +-
.../struct-member-invalid-dict.err| 2 +-
tests/qapi-schema/struct-member-invalid.err | 2 +-
.../union-base-no-discriminator.err | 2 +-
tests/qapi-schema/union-branch-case.err | 2 +-
.../qapi-schema/union-branch-invalid-dict.err | 2 +-
tests/qapi-schema/union-optional-branch.err | 2 +-
tests/qapi-schema/unknown-expr-key.err| 2 +-
53 files changed, 101 insertions(+), 132 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 42b9c2e36b..81c217cd60 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -657,13 +657,6 @@ valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
'[a-zA-Z][a-zA-Z0-9_-]*$')
-def check_name(name, info, source,
- allow_optional=False, enum_member=False, permit_upper=False):
-check_name_is_str(name, info, source)
-check_name_str(name, info, source,
- allow_optional, enum_member, permit_upper)
-
-
def check_name_is_str(name, info, source):
if not isinstance(name, str):
raise QAPISemError(info, "%s requires a string name" % source)
@@ -685,10 +678,10 @@ def check_name_str(name, info, source,
# and 'q_obj_*' implicit type names.
if not valid_name.match(membername) or \
c_name(membername, False).startswith('q_'):
-raise QAPISemError(info, "%s uses invalid name '%s'" % (source, name))
+raise QAPISemError(info, "%s has an invalid name" % source)
if not permit_upper and name.lower() != name:
raise QAPISemError(
-info, "%s uses uppercase in name '%s'" % (source, name))
+info, "%s uses uppercase in name" % source)
assert not membername.startswith('*')
@@ -696,7 +689,7 @@ def check_defn_name_str(name, info, meta):
check_name_str(name, info, meta, permit_upper=True)
if name.endswith('Kind') or name.endswith('List'):
raise QAPISemError(
-info, "%s '%s' should not end in '%s'" % (meta, name, name[-4:]))
+info, "%s name should not end in '%s'" % (meta, name[-4:]))
def check_if(expr, info):
@@ -753,42 +746,35 @@ def
[PULL 17/27] qapi: Move context-sensitive checking to the proper place
When we introduced the QAPISchema intermediate representation (commit
ac88219a6c7), we took a shortcut: we left check_exprs() & friends
alone instead of moving semantic checks into the
QAPISchemaFOO.check(). The .check() assert check_exprs() did its job.
Time to finish the conversion job. Move exactly the context-sensitive
checks to the .check(). They replace assertions there. Context-free
checks stay put.
Fixes the misleading optional tag error demonstrated by test
flat-union-optional-discriminator.
A few other error message improve.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20190927134639.4284-17-arm...@redhat.com>
---
scripts/qapi/common.py| 424 --
tests/qapi-schema/alternate-any.err | 2 +-
.../alternate-conflict-bool-string.err| 2 +-
tests/qapi-schema/alternate-conflict-dict.err | 2 +-
.../alternate-conflict-enum-bool.err | 2 +-
.../alternate-conflict-enum-int.err | 2 +-
.../alternate-conflict-num-string.err | 2 +-
.../qapi-schema/alternate-conflict-string.err | 2 +-
tests/qapi-schema/alternate-nested.err| 2 +-
tests/qapi-schema/alternate-unknown.err | 2 +-
tests/qapi-schema/args-alternate.err | 2 +-
tests/qapi-schema/args-any.err| 2 +-
tests/qapi-schema/args-array-unknown.err | 2 +-
tests/qapi-schema/args-boxed-string.err | 2 +-
tests/qapi-schema/args-int.err| 2 +-
tests/qapi-schema/args-member-unknown.err | 2 +-
tests/qapi-schema/args-union.err | 2 +-
tests/qapi-schema/args-unknown.err| 2 +-
tests/qapi-schema/bad-base.err| 2 +-
tests/qapi-schema/command-int.err | 2 +-
tests/qapi-schema/flat-union-base-any.err | 2 +-
tests/qapi-schema/flat-union-base-union.err | 2 +-
.../flat-union-discriminator-bad-name.err | 2 +-
.../flat-union-discriminator-bad-name.json| 1 -
tests/qapi-schema/flat-union-empty.err| 2 +-
tests/qapi-schema/flat-union-int-branch.err | 2 +-
.../flat-union-invalid-branch-key.err | 2 +-
.../flat-union-invalid-if-discriminator.err | 2 +-
.../flat-union-optional-discriminator.err | 2 +-
.../flat-union-optional-discriminator.json| 1 -
.../flat-union-string-discriminator.err | 2 +-
tests/qapi-schema/redefined-builtin.err | 2 +-
tests/qapi-schema/redefined-type.err | 2 +-
tests/qapi-schema/returns-alternate.err | 2 +-
tests/qapi-schema/returns-unknown.err | 2 +-
tests/qapi-schema/returns-whitelist.err | 2 +-
tests/qapi-schema/union-empty.err | 2 +-
tests/qapi-schema/union-invalid-base.err | 2 +-
tests/qapi-schema/union-unknown.err | 2 +-
tests/qapi-schema/union-unknown.json | 2 +-
40 files changed, 230 insertions(+), 270 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 9acff01d3e..f22e84c4a8 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -21,25 +21,6 @@ import string
import sys
from collections import OrderedDict
-builtin_types = {
-'null': 'QTYPE_QNULL',
-'str': 'QTYPE_QSTRING',
-'int': 'QTYPE_QNUM',
-'number': 'QTYPE_QNUM',
-'bool': 'QTYPE_QBOOL',
-'int8': 'QTYPE_QNUM',
-'int16':'QTYPE_QNUM',
-'int32':'QTYPE_QNUM',
-'int64':'QTYPE_QNUM',
-'uint8':'QTYPE_QNUM',
-'uint16': 'QTYPE_QNUM',
-'uint32': 'QTYPE_QNUM',
-'uint64': 'QTYPE_QNUM',
-'size': 'QTYPE_QNUM',
-'any': None, # any QType possible, actually
-'QType':'QTYPE_QSTRING',
-}
-
# Are documentation comments required?
doc_required = False
@@ -49,11 +30,6 @@ returns_whitelist = []
# Whitelist of entities allowed to violate case conventions
name_case_whitelist = []
-enum_types = {}
-struct_types = {}
-union_types = {}
-all_names = {}
-
#
# Parsing the schema into expressions
@@ -671,34 +647,9 @@ class QAPISchemaParser(object):
#
-# Semantic analysis of schema expressions
-# TODO fold into QAPISchema
-# TODO catching name collisions in generated code would be nice
+# Check (context-free) schema expression structure
#
-
-def find_base_members(base):
-if isinstance(base, dict):
-return base
-base_struct_define = struct_types.get(base)
-if not base_struct_define:
-return None
-return base_struct_define['data']
-
-
-# Return the qtype of an alternate branch, or None on error.
-def find_alternate_member_qtype(qapi_type):
-if qapi_type in builtin_types:
-return builtin_types[qapi_type]
-elif qapi_type in struct_types:
-return 'QTYPE_QDICT'
-elif qapi_type in enum_types:
-return 'QTYPE_QSTRING'
-elif qapi_type in union_types:
-return 'QTYPE_QDICT'
-return None
-
-
# Names must be lette
[PULL 07/27] qapi: Change frontend error messages to start with lower case
Starting error messages with a capital letter complicates things when text can get interpolated both at the beginning and in the middle of an error message. The next patch will do that. Switch to lower case to keep it simpler. For what it's worth, the GNU Coding Standards advise the message "should not begin with a capital letter when it follows a program name and/or file name, because that isn’t the beginning of a sentence. (The sentence conceptually starts at the beginning of the line.)" While there, avoid breaking lines containing multiple arguments in the middle of an argument. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-7-arm...@redhat.com> --- scripts/qapi/common.py| 192 ++ tests/qapi-schema/alternate-any.err | 2 +- tests/qapi-schema/alternate-array.err | 2 +- tests/qapi-schema/alternate-base.err | 2 +- .../alternate-conflict-bool-string.err| 2 +- tests/qapi-schema/alternate-conflict-dict.err | 2 +- .../alternate-conflict-enum-bool.err | 2 +- .../alternate-conflict-enum-int.err | 2 +- .../alternate-conflict-num-string.err | 2 +- .../qapi-schema/alternate-conflict-string.err | 2 +- tests/qapi-schema/alternate-empty.err | 2 +- tests/qapi-schema/alternate-invalid-dict.err | 2 +- tests/qapi-schema/alternate-nested.err| 2 +- tests/qapi-schema/alternate-unknown.err | 2 +- tests/qapi-schema/args-array-empty.err| 2 +- tests/qapi-schema/args-array-unknown.err | 2 +- tests/qapi-schema/args-member-array-bad.err | 2 +- tests/qapi-schema/args-member-case.err| 2 +- tests/qapi-schema/args-member-unknown.err | 2 +- tests/qapi-schema/bad-type-int.err| 2 +- tests/qapi-schema/base-cycle-direct.err | 2 +- tests/qapi-schema/base-cycle-indirect.err | 2 +- .../qapi-schema/doc-bad-alternate-member.err | 2 +- tests/qapi-schema/doc-bad-command-arg.err | 2 +- tests/qapi-schema/doc-bad-symbol.err | 2 +- tests/qapi-schema/doc-bad-union-member.err| 2 +- tests/qapi-schema/doc-before-include.err | 2 +- tests/qapi-schema/doc-before-pragma.err | 2 +- tests/qapi-schema/doc-duplicated-return.err | 2 +- tests/qapi-schema/doc-duplicated-since.err| 2 +- tests/qapi-schema/doc-empty-arg.err | 2 +- tests/qapi-schema/doc-empty-section.err | 2 +- tests/qapi-schema/doc-empty-symbol.err| 2 +- tests/qapi-schema/doc-invalid-end.err | 2 +- tests/qapi-schema/doc-invalid-end2.err| 2 +- tests/qapi-schema/doc-invalid-start.err | 2 +- tests/qapi-schema/doc-missing-colon.err | 2 +- tests/qapi-schema/doc-missing-expr.err| 2 +- tests/qapi-schema/doc-missing-space.err | 2 +- tests/qapi-schema/doc-missing.err | 2 +- tests/qapi-schema/doc-no-symbol.err | 2 +- tests/qapi-schema/double-type.err | 2 +- tests/qapi-schema/duplicate-key.err | 2 +- tests/qapi-schema/enum-bad-member.err | 2 +- tests/qapi-schema/enum-bad-name.err | 2 +- tests/qapi-schema/enum-bad-prefix.err | 2 +- .../qapi-schema/enum-dict-member-unknown.err | 2 +- tests/qapi-schema/enum-int-member.err | 2 +- tests/qapi-schema/enum-member-case.err| 2 +- tests/qapi-schema/enum-missing-data.err | 2 +- tests/qapi-schema/enum-wrong-data.err | 2 +- tests/qapi-schema/escape-outside-string.err | 2 +- tests/qapi-schema/event-boxed-empty.err | 2 +- .../qapi-schema/event-member-invalid-dict.err | 2 +- tests/qapi-schema/event-nest-struct.err | 2 +- tests/qapi-schema/features-bad-type.err | 2 +- tests/qapi-schema/features-missing-name.err | 2 +- tests/qapi-schema/features-name-bad-type.err | 2 +- tests/qapi-schema/features-no-list.err| 2 +- tests/qapi-schema/features-unknown-key.err| 2 +- tests/qapi-schema/flat-union-array-branch.err | 2 +- .../flat-union-bad-discriminator.err | 2 +- .../flat-union-discriminator-bad-name.err | 2 +- tests/qapi-schema/flat-union-empty.err| 2 +- .../flat-union-inline-invalid-dict.err| 2 +- tests/qapi-schema/flat-union-inline.err | 2 +- tests/qapi-schema/flat-union-int-branch.err | 2 +- .../flat-union-invalid-branch-key.err | 2 +- .../flat-union-invalid-discriminator.err | 2 +- .../flat-union-invalid-if-discriminator.err | 2 +- tests/qapi-schema/flat-union-no-base.err | 2 +- .../flat-union-optional-discriminator.err | 2 +- .../flat-union-string-discriminator.err | 2 +- tests/qapi-schema/funny-char.err | 2 +- tests/qapi-schema/funny-word.err | 2 +- tests/qapi-schema/ident-with-escape.err | 2 +- tests/qapi-sche
Re: Questions about the real mode in kvm/qemu
On 9/26/19 12:18 PM, Paolo Bonzini wrote: On 26/09/19 10:59, Maxim Levitsky wrote: If you mean to ask if there is a way to let guest access use no paging at all, that is access host physical addresses directly, then indeed there is no way, since regular non 'unrestricted guest' mode required both protected mode and paging, and 'unrestricted guest' requires EPT. Academically speaking it is of course possible to create paging tables that are 1:1... Not so academically, it's exactly what KVM does. However, indeed it would also be possible to switch out of EPT mode when CR0.PG=0. I'm not sure why it was done this way, maybe when the code was written it was simpler to use the identity map. Let's see if Avi is listening... :) I think it was just simpler for the people who implemented it at the time. Switching out of EPT would have been a better solution as it would no longer require allocating guest physical address space with the few warts that requires.
Re: target/ppc: bug in optimised vsl/vsr implementation?
28.09.2019. 19.45, "Aleksandar Markovic" је написао/ла: > > > 26.09.2019. 20.14, "Mark Cave-Ayland" је написао/ла: > > > > As part of the investigation into the DFP number issue reported at > > https://bugs.launchpad.net/qemu/+bug/1841990 it appears that there may also be a bug > > introduced by the new optimised vsl/vsr implementation: > > > > commit 4e6d0920e7547e6af4bbac5ffe9adfe6ea621822 > > Author: Stefan Brankovic > > Date: Mon Jul 15 16:22:48 2019 +0200 > > > > target/ppc: Optimize emulation of vsl and vsr instructions > > > > > > (sorry in advance if the format of this message looks odd, I have some problems with mail settings related to recent qemu-devel mailer settings changes; I will adjust my mail settings in next few days) > > Mark and Paul (and Stefan), > > Thanks for spotting this and pinpointing the culprit commit. I guess Stefan is going to respond soon, but, in the meantime, I took a look at the commit in question: > > https://github.com/qemu/qemu/commit/4e6d0920e7547e6af4bbac5ffe9adfe6ea621822 > > I don't have at the moment any dev/test environment handy, but I did manual inspection of the code, and here is what I found (in order of importance, perceived by me): > > 1. The code will not work correctly if the shift ammount (variable 'sh') is 0. This is because, in that case, one of succeeding invocations of TCG shift functions will be required to shift a 64-bit TCG variable by 64 bits, and the result of such TCG operation is undefined (shift amount must be 63 or less) - see https://github.com/qemu/qemu/blob/master/tcg/README. > > 2. Variable naming is better in the old helper than in the new translator. In that light, I would advise Stefan to change 'sh' to 'shift', and 'shifted' to 'carry'. > > 3. Lines > > tcg_gen_andi_i64(shifted, shifted, 0x7fULL); > > and > > tcg_gen_andi_i64(shifted, shifted, 0xfe00ULL); > > appear to be spurious (albait in a harmless way). Therefore, they should be deleted, or, alternatevely, a justification for them should be provided. > > 4. In the commit message, variable names were used without quotation mark, resulting in puzzling and unclear wording. > > 5. (a question for Mark) After all recent changes, does get_avr64(..., ..., true) always (for any endian configuration) return the "high" half of an Altivec register, and get_avr64(..., ..., false) the "low" one? > One more hint: variables 'avrA' and 'avrB' can be a single variable, since there is no moment during execution where both are needed; the same for 'tmp' and 'shifted'. Also, check on the hardware the behavior listed as 'undefined' for vsl/vsr in the docs - even though it is tehnically irrelevant, I am courious whether the old or the new (or none of them) solution match the hardware. > Given all these circumstances, perhaps the most reasonable solution would be to revert the commit in question, and allow Stefan enough dev and test time to hopefully submit a new, better, version later on. > > Sincerely, > Aleksandar >
RE: [PATCH V2] intel_iommu: TM field should not be in reserved bits
> -Original Message-
> From: Peter Xu
> Sent: Friday, September 27, 2019 5:32 PM
> To: Zhang, Qi1
> Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com;
> pbonz...@redhat.com; r...@twiddle.net; Qi, Yadong
> Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
>
> On Fri, Sep 27, 2019 at 08:03:21AM +, Zhang, Qi1 wrote:
> >
> >
> > > -Original Message-
> > > From: Peter Xu
> > > Sent: Friday, September 27, 2019 2:10 PM
> > > To: Zhang, Qi1
> > > Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com;
> > > pbonz...@redhat.com; r...@twiddle.net
> > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in
> > > reserved bits
> > >
> > > On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zh...@intel.com wrote:
> > > > From: "Zhang, Qi"
> > > >
> > > > When dt is supported, TM field should not be Reserved(0).
> > > >
> > > > Refer to VT-d Spec 9.8
> > > >
> > > > Signed-off-by: Zhang, Qi
> > > > Signed-off-by: Qi, Yadong
> > > > ---
> > > > hw/i386/intel_iommu.c | 12 ++--
> > > > hw/i386/intel_iommu_internal.h | 25 +++--
> > > > 2 files changed, 25 insertions(+), 12 deletions(-)
> > > > ---
> VTD_SPTE_PAGE_L2_RSVD_MASK(s-
> > > >aw_bits);
> > > > -vtd_paging_entry_rsvd_field[3] =
> > > >aw_bits);
> > > > +vtd_paging_entry_rsvd_field[5] =
> > > VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu-
> > > >dt_supported);
> > > > +vtd_paging_entry_rsvd_field[6] =
> > > VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu-
> > > >dt_supported);
> > > > +vtd_paging_entry_rsvd_field[7] =
> > > > + VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu-
> > > >dt_supported);
> > > > vtd_paging_entry_rsvd_field[8] =
> > > >VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
> > >
> > > Should this TM bit only affects leaves? Say, entry 1 (4K), 5 (2M), 6
> > > (1G).
> > > While this reminded me that I'm totally confused on why we have had
> > > entry 7, 8 after all... Are they really used?
> > Yes. TM bit only affects. To this array, index 1, 5,6,7 may be leaf. Will
> > update
> a new patchset for it.
>
> Could I ask why index 7 may be leaf?
Index 7 is PDPE 1G GB leaf.
>
> > >
> > > >
> > > > if (x86_iommu_ir_supported(x86_iommu)) { diff --git
> > > > a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> > > > index c1235a7063..01f1aa6c86 100644
> > > > --- a/hw/i386/intel_iommu_internal.h
> > > > +++ b/hw/i386/intel_iommu_internal.h
> > > > @@ -387,19 +387,31 @@ typedef union VTDInvDesc VTDInvDesc;
> > > > #define VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffe0fff8
> > > >
> > > > /* Rsvd field masks for spte */
> > > > -#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw) \
> > > > +#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw, dt_supported) \
> > > > +dt_supported? \
> > > > +(0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM |
> > > VTD_SL_TM))
> > > > +: \
> > > > (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> > >
> > > This seems strange too in that ~VTD_HAW_MASK(aw) probably covered
> > > bits
> > > 63-48 for aw==48 case so it should already cover VTD_SL_TM?
> > VTD_SL_IGN_COM 0xbff0ULL, TM field is cleared by ~
> > VTD_SL_IGN_COM
> > >
> > > Meanwhile when I'm reading the spec I see at least bits 61-52
> > > ignored rather than reserved.
> > Yes. Bit 61~52 is ignored. Such as the index 5 of this array is
> > 0xfff800800.
>
> Oops, my poor eye obviously didn't see that the "~" operator is applied over
> the whole (VTD_HAW_MASK(aw) | VTD_SL_IGN_COM)... :)
>
> Btw, you should only touch up the macros that are leaves here.
> Non-leaves should still keep that bit as reserved.
Yes. I will.
>
> Thanks,
>
> --
> Peter Xu
Re: [PATCH RESEND v4 1/2] x86/cpu: Add support for UMONITOR/UMWAIT/TPAUSE
On 9/28/2019 4:22 AM, Paolo Bonzini wrote:
On 18/09/19 09:23, Tao Xu wrote:
+} else if (function == 7 && index == 0 && reg == R_ECX) {
+if (enable_cpu_pm) {
+ret |= CPUID_7_0_ECX_WAITPKG;
+}
This should be the opposite; remove the bit if enable_cpu_pm is not set.
Paolo
Thanks, I will improve it.
[PATCH v5 1/2] x86/cpu: Add support for UMONITOR/UMWAIT/TPAUSE
UMONITOR, UMWAIT and TPAUSE are a set of user wait instructions.
This patch adds support for user wait instructions in KVM. Availability
of the user wait instructions is indicated by the presence of the CPUID
feature flag WAITPKG CPUID.0x07.0x0:ECX[5]. User wait instructions may
be executed at any privilege level, and use IA32_UMWAIT_CONTROL MSR to
set the maximum time.
The patch enable the umonitor, umwait and tpause features in KVM.
Because umwait and tpause can put a (psysical) CPU into a power saving
state, by default we dont't expose it to kvm and enable it only when
guest CPUID has it. And use QEMU command-line "-overcommit cpu-pm=on"
(enable_cpu_pm is enabled), a VM can use UMONITOR, UMWAIT and TPAUSE
instructions. If the instruction causes a delay, the amount of time
delayed is called here the physical delay. The physical delay is first
computed by determining the virtual delay (the time to delay relative to
the VM’s timestamp counter). Otherwise, UMONITOR, UMWAIT and TPAUSE cause
an invalid-opcode exception(#UD).
The release document ref below link:
https://software.intel.com/sites/default/files/\
managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf
Co-developed-by: Jingqi Liu
Signed-off-by: Jingqi Liu
Signed-off-by: Tao Xu
---
Changes in v5:
- Remove CPUID_7_0_ECX_WAITPKG if enable_cpu_pm is not set.
(Paolo)
---
target/i386/cpu.c | 3 ++-
target/i386/cpu.h | 1 +
target/i386/kvm.c | 6 ++
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 9e0bac31e8..15f888b13f 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1062,7 +1062,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] =
{
.type = CPUID_FEATURE_WORD,
.feat_names = {
NULL, "avx512vbmi", "umip", "pku",
-NULL /* ospke */, NULL, "avx512vbmi2", NULL,
+NULL /* ospke */, "waitpkg", "avx512vbmi2", NULL,
"gfni", "vaes", "vpclmulqdq", "avx512vnni",
"avx512bitalg", NULL, "avx512-vpopcntdq", NULL,
"la57", NULL, NULL, NULL,
@@ -5227,6 +5227,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error
**errp)
host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx,
&cpu->mwait.ecx, &cpu->mwait.edx);
env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
+env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG;
}
}
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 5f6e3a029a..33a0b8b365 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -673,6 +673,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
#define CPUID_7_0_ECX_UMIP (1U << 2)
#define CPUID_7_0_ECX_PKU (1U << 3)
#define CPUID_7_0_ECX_OSPKE(1U << 4)
+#define CPUID_7_0_ECX_WAITPKG (1U << 5) /* UMONITOR/UMWAIT/TPAUSE
Instructions */
#define CPUID_7_0_ECX_VBMI2(1U << 6) /* Additional VBMI Instrs */
#define CPUID_7_0_ECX_GFNI (1U << 8)
#define CPUID_7_0_ECX_VAES (1U << 9)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 92069099ab..ea9a87bfd8 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -400,6 +400,12 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s,
uint32_t function,
if (host_tsx_blacklisted()) {
ret &= ~(CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_HLE);
}
+} else if (function == 7 && index == 0 && reg == R_ECX) {
+if (enable_cpu_pm) {
+ret |= CPUID_7_0_ECX_WAITPKG;
+} else {
+ret &= ~CPUID_7_0_ECX_WAITPKG;
+}
} else if (function == 7 && index == 0 && reg == R_EDX) {
/*
* Linux v4.17-v4.20 incorrectly return ARCH_CAPABILITIES on SVM hosts.
--
2.20.1
[PATCH v5 0/2] x86: Enable user wait instructions
UMONITOR, UMWAIT and TPAUSE are a set of user wait instructions. UMONITOR arms address monitoring hardware using an address. A store to an address within the specified address range triggers the monitoring hardware to wake up the processor waiting in umwait. UMWAIT instructs the processor to enter an implementation-dependent optimized state while monitoring a range of addresses. The optimized state may be either a light-weight power/performance optimized state (c0.1 state) or an improved power/performance optimized state (c0.2 state). TPAUSE instructs the processor to enter an implementation-dependent optimized state c0.1 or c0.2 state and wake up when time-stamp counter reaches specified timeout. Availability of the user wait instructions is indicated by the presence of the CPUID feature flag WAITPKG CPUID.0x07.0x0:ECX[5]. The patches enable the umonitor, umwait and tpause features in KVM. Because umwait and tpause can put a (psysical) CPU into a power saving state, by default we dont't expose it in kvm and provide a capability to enable it. Use kvm capability to enable UMONITOR, UMWAIT and TPAUSE when QEMU use "-overcommit cpu-pm=on, a VM can use UMONITOR, UMWAIT and TPAUSE instructions. If the instruction causes a delay, the amount of time delayed is called here the physical delay. The physical delay is first computed by determining the virtual delay (the time to delay relative to the VM’s timestamp counter). Otherwise, UMONITOR, UMWAIT and TPAUSE cause an invalid-opcode exception(#UD). The release document ref below link: https://software.intel.com/sites/default/files/\ managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf Changelog: v5: Remove CPUID_7_0_ECX_WAITPKG if enable_cpu_pm is not set. (Paolo) v4: Set IA32_UMWAIT_CONTROL 32bits v3: Simplify the patches, expose user wait instructions when the guest has CPUID (Paolo) v2: Separated from the series https://www.mail-archive.com/qemu-devel@nongnu.org/msg549526.html Use kvm capability to enable UMONITOR, UMWAIT and TPAUSE when QEMU use "-overcommit cpu-pm=on" v1: Sent out with MOVDIRI/MOVDIR64B instructions patches Tao Xu (2): x86/cpu: Add support for UMONITOR/UMWAIT/TPAUSE target/i386: Add support for save/load IA32_UMWAIT_CONTROL MSR target/i386/cpu.c | 3 ++- target/i386/cpu.h | 3 +++ target/i386/kvm.c | 19 +++ target/i386/machine.c | 20 4 files changed, 44 insertions(+), 1 deletion(-) -- 2.20.1
[PATCH v5 2/2] target/i386: Add support for save/load IA32_UMWAIT_CONTROL MSR
UMWAIT and TPAUSE instructions use 32bits IA32_UMWAIT_CONTROL at MSR
index E1H to determines the maximum time in TSC-quanta that the processor
can reside in either C0.1 or C0.2.
This patch is to Add support for save/load IA32_UMWAIT_CONTROL MSR in
guest.
Co-developed-by: Jingqi Liu
Signed-off-by: Jingqi Liu
Signed-off-by: Tao Xu
---
No changes in v5.
Changes in v4:
Set IA32_UMWAIT_CONTROL 32bits
---
target/i386/cpu.h | 2 ++
target/i386/kvm.c | 13 +
target/i386/machine.c | 20
3 files changed, 35 insertions(+)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 33a0b8b365..bcd1cbbfc0 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -451,6 +451,7 @@ typedef enum X86Seg {
#define MSR_IA32_BNDCFGS0x0d90
#define MSR_IA32_XSS0x0da0
+#define MSR_IA32_UMWAIT_CONTROL 0xe1
#define XSTATE_FP_BIT 0
#define XSTATE_SSE_BIT 1
@@ -1393,6 +1394,7 @@ typedef struct CPUX86State {
uint16_t fpregs_format_vmstate;
uint64_t xss;
+uint32_t umwait;
TPRAccess tpr_access_type;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index ea9a87bfd8..8b715af8eb 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -95,6 +95,7 @@ static bool has_msr_hv_stimer;
static bool has_msr_hv_frequencies;
static bool has_msr_hv_reenlightenment;
static bool has_msr_xss;
+static bool has_msr_umwait;
static bool has_msr_spec_ctrl;
static bool has_msr_virt_ssbd;
static bool has_msr_smi_count;
@@ -1909,6 +1910,9 @@ static int kvm_get_supported_msrs(KVMState *s)
case MSR_IA32_XSS:
has_msr_xss = true;
break;
+case MSR_IA32_UMWAIT_CONTROL:
+has_msr_umwait = true;
+break;
case HV_X64_MSR_CRASH_CTL:
has_msr_hv_crash = true;
break;
@@ -2459,6 +2463,9 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
if (has_msr_xss) {
kvm_msr_entry_add(cpu, MSR_IA32_XSS, env->xss);
}
+if (has_msr_umwait) {
+kvm_msr_entry_add(cpu, MSR_IA32_UMWAIT_CONTROL, env->umwait);
+}
if (has_msr_spec_ctrl) {
kvm_msr_entry_add(cpu, MSR_IA32_SPEC_CTRL, env->spec_ctrl);
}
@@ -2863,6 +2870,9 @@ static int kvm_get_msrs(X86CPU *cpu)
if (has_msr_xss) {
kvm_msr_entry_add(cpu, MSR_IA32_XSS, 0);
}
+if (has_msr_umwait) {
+kvm_msr_entry_add(cpu, MSR_IA32_UMWAIT_CONTROL, 0);
+}
if (has_msr_spec_ctrl) {
kvm_msr_entry_add(cpu, MSR_IA32_SPEC_CTRL, 0);
}
@@ -3115,6 +3125,9 @@ static int kvm_get_msrs(X86CPU *cpu)
case MSR_IA32_XSS:
env->xss = msrs[i].data;
break;
+case MSR_IA32_UMWAIT_CONTROL:
+env->umwait = msrs[i].data;
+break;
default:
if (msrs[i].index >= MSR_MC0_CTL &&
msrs[i].index < MSR_MC0_CTL + (env->mcg_cap & 0xff) * 4) {
diff --git a/target/i386/machine.c b/target/i386/machine.c
index 2767b3096d..6481f846f6 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -943,6 +943,25 @@ static const VMStateDescription vmstate_xss = {
}
};
+static bool umwait_needed(void *opaque)
+{
+X86CPU *cpu = opaque;
+CPUX86State *env = &cpu->env;
+
+return env->umwait != 0;
+}
+
+static const VMStateDescription vmstate_umwait = {
+.name = "cpu/umwait",
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = umwait_needed,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(env.umwait, X86CPU),
+VMSTATE_END_OF_LIST()
+}
+};
+
#ifdef TARGET_X86_64
static bool pkru_needed(void *opaque)
{
@@ -1391,6 +1410,7 @@ VMStateDescription vmstate_x86_cpu = {
&vmstate_msr_hyperv_reenlightenment,
&vmstate_avx512,
&vmstate_xss,
+&vmstate_umwait,
&vmstate_tsc_khz,
&vmstate_msr_smi_count,
#ifdef TARGET_X86_64
--
2.20.1
Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
On Sun, Sep 29, 2019 at 01:11:12AM +, Zhang, Qi1 wrote: > > > > -Original Message- > > From: Peter Xu > > Sent: Friday, September 27, 2019 5:32 PM > > To: Zhang, Qi1 > > Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com; > > pbonz...@redhat.com; r...@twiddle.net; Qi, Yadong > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits > > > > On Fri, Sep 27, 2019 at 08:03:21AM +, Zhang, Qi1 wrote: > > > > > > > > > > -Original Message- > > > > From: Peter Xu > > > > Sent: Friday, September 27, 2019 2:10 PM > > > > To: Zhang, Qi1 > > > > Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com; > > > > pbonz...@redhat.com; r...@twiddle.net > > > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in > > > > reserved bits > > > > > > > > On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zh...@intel.com wrote: > > > > > From: "Zhang, Qi" > > > > > > > > > > When dt is supported, TM field should not be Reserved(0). > > > > > > > > > > Refer to VT-d Spec 9.8 > > > > > > > > > > Signed-off-by: Zhang, Qi > > > > > Signed-off-by: Qi, Yadong > > > > > --- > > > > > hw/i386/intel_iommu.c | 12 ++-- > > > > > hw/i386/intel_iommu_internal.h | 25 +++-- > > > > > 2 files changed, 25 insertions(+), 12 deletions(-) > > > > > --- > > VTD_SPTE_PAGE_L2_RSVD_MASK(s- > > > > >aw_bits); > > > > > -vtd_paging_entry_rsvd_field[3] = > > > > >aw_bits); > > > > > +vtd_paging_entry_rsvd_field[5] = > > > > VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu- > > > > >dt_supported); > > > > > +vtd_paging_entry_rsvd_field[6] = > > > > VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu- > > > > >dt_supported); > > > > > +vtd_paging_entry_rsvd_field[7] = > > > > > + VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu- > > > > >dt_supported); > > > > > vtd_paging_entry_rsvd_field[8] = > > > > >VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits); > > > > > > > > Should this TM bit only affects leaves? Say, entry 1 (4K), 5 (2M), 6 > > > > (1G). [1] > > > > While this reminded me that I'm totally confused on why we have had > > > > entry 7, 8 after all... Are they really used? > > > Yes. TM bit only affects. To this array, index 1, 5,6,7 may be leaf. Will > > > update > > a new patchset for it. > > > > Could I ask why index 7 may be leaf? > Index 7 is PDPE 1G GB leaf. I thought 1G was index 6. I've listed my understanding above [1]. Would you please double confirm? Thanks, -- Peter Xu
Re: [PATCH v18 1/6] hw/arm/virt: Introduce RAS platform version and RAS machine option
On 2019/9/27 22:02, Peter Maydell wrote:
> On Fri, 6 Sep 2019 at 09:33, Xiang Zheng wrote:
>>
>> From: Dongjiu Geng
>>
>> Support RAS Virtualization feature since version 4.2, disable it by
>> default in the old versions. Also add a machine option which allows user
>> to enable it explicitly.
>>
>> Signed-off-by: Dongjiu Geng
>> Signed-off-by: Xiang Zheng
>> ---
>> hw/arm/virt.c | 33 +
>> include/hw/arm/virt.h | 2 ++
>> 2 files changed, 35 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index d74538b021..e0451433c8 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1783,6 +1783,20 @@ static void virt_set_its(Object *obj, bool value,
>> Error **errp)
>> vms->its = value;
>> }
>>
>> +static bool virt_get_ras(Object *obj, Error **errp)
>> +{
>> +VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +return vms->ras;
>> +}
>> +
>> +static void virt_set_ras(Object *obj, bool value, Error **errp)
>> +{
>> +VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +vms->ras = value;
>> +}
>> +
>> static char *virt_get_gic_version(Object *obj, Error **errp)
>> {
>> VirtMachineState *vms = VIRT_MACHINE(obj);
>> @@ -2026,6 +2040,19 @@ static void virt_instance_init(Object *obj)
>> "Valid values are none and smmuv3",
>> NULL);
>>
>> +if (vmc->no_ras) {
>> +vms->ras = false;
>> +} else {
>> +/* Default disallows RAS instantiation */
>> +vms->ras = false;
>> +object_property_add_bool(obj, "ras", virt_get_ras,
>> + virt_set_ras, NULL);
>> +object_property_set_description(obj, "ras",
>> +"Set on/off to enable/disable "
>> +"RAS instantiation",
>> +NULL);
>> +}
>
> For a property which is disabled by default, you don't need
> to have a separate flag in the VirtMachineClass struct.
> Those are only needed for properties where we need the old machine
> types to have the property be 'off' but new machine types
> need to default to it be 'on'. Since vms->ras is false
> by default anyway, you can just have this part:
>
>> +/* Default disallows RAS instantiation */
>> +vms->ras = false;
>> +object_property_add_bool(obj, "ras", virt_get_ras,
>> + virt_set_ras, NULL);
>> +object_property_set_description(obj, "ras",
>> +"Set on/off to enable/disable "
>> +"RAS instantiation",
>> +NULL);
>
> Compare the 'vms->secure' flag and associated property
> for an example of this.
Thanks for pointing it out, I will remove the no_ras in the VirtMachineClass
struct.
>
>> vms->irqmap = a15irqmap;
>>
>> virt_flash_create(vms);
>> @@ -2058,8 +2085,14 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
>>
>> static void virt_machine_4_1_options(MachineClass *mc)
>> {
>> +VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
>> +
>> virt_machine_4_2_options(mc);
>> compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len);
>> +/* Disable memory recovery feature for 4.1 as RAS support was
>> + * introduced with 4.2.
>> + */
>> +vmc->no_ras = true;
>> }
>> DEFINE_VIRT_MACHINE(4, 1)
>
> thanks
> -- PMM
>
> .
>
--
Thanks,
Xiang
RE: [PATCH V2] intel_iommu: TM field should not be in reserved bits
> -Original Message-
> From: Peter Xu
> Sent: Sunday, September 29, 2019 10:02 AM
> To: Zhang, Qi1
> Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com;
> pbonz...@redhat.com; r...@twiddle.net; Qi, Yadong
> Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
>
> On Sun, Sep 29, 2019 at 01:11:12AM +, Zhang, Qi1 wrote:
> >
> >
> > > -Original Message-
> > > From: Peter Xu
> > > Sent: Friday, September 27, 2019 5:32 PM
> > > To: Zhang, Qi1
> > > Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com;
> > > pbonz...@redhat.com; r...@twiddle.net; Qi, Yadong
> > >
> > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in
> > > reserved bits
> > >
> > > On Fri, Sep 27, 2019 at 08:03:21AM +, Zhang, Qi1 wrote:
> > > >
> > > >
> > > > > -Original Message-
> > > > > From: Peter Xu
> > > > > Sent: Friday, September 27, 2019 2:10 PM
> > > > > To: Zhang, Qi1
> > > > > Cc: qemu-devel@nongnu.org; ehabk...@redhat.com;
> m...@redhat.com;
> > > > > pbonz...@redhat.com; r...@twiddle.net
> > > > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in
> > > > > reserved bits
> > > > >
> > > > > On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zh...@intel.com wrote:
> > > > > > From: "Zhang, Qi"
> > > > > >
> > > > > > When dt is supported, TM field should not be Reserved(0).
> > > > > >
> > > > > > Refer to VT-d Spec 9.8
> > > > > >
> > > > > > Signed-off-by: Zhang, Qi
> > > > > > Signed-off-by: Qi, Yadong
> > > > > > ---
> > > > > > hw/i386/intel_iommu.c | 12 ++--
> > > > > > hw/i386/intel_iommu_internal.h | 25 +++--
> > > > > > 2 files changed, 25 insertions(+), 12 deletions(-)
> > > > > > ---
> > > VTD_SPTE_PAGE_L2_RSVD_MASK(s-
> > > > > >aw_bits);
> > > > > > -vtd_paging_entry_rsvd_field[3] =
> > > > > >aw_bits);
> > > > > > +vtd_paging_entry_rsvd_field[5] =
> > > > > VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > > >dt_supported);
> > > > > > +vtd_paging_entry_rsvd_field[6] =
> > > > > VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > > >dt_supported);
> > > > > > +vtd_paging_entry_rsvd_field[7] =
> > > > > > + VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > > >dt_supported);
> > > > > > vtd_paging_entry_rsvd_field[8] =
> > > > > >VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
> > > > >
> > > > > Should this TM bit only affects leaves? Say, entry 1 (4K), 5 (2M), 6
> (1G).
>
> [1]
>
> > > > > While this reminded me that I'm totally confused on why we have
> > > > > had entry 7, 8 after all... Are they really used?
> > > > Yes. TM bit only affects. To this array, index 1, 5,6,7 may be
> > > > leaf. Will update
> > > a new patchset for it.
> > >
> > > Could I ask why index 7 may be leaf?
> > Index 7 is PDPE 1G GB leaf.
>
> I thought 1G was index 6. I've listed my understanding above [1].
> Would you please double confirm? Thanks,
Check the existing function. When level is 3 VTD_SL_PDP_LEVEL and the entry is
leaf, it is PDPE 1G leaf and the corresponding index of this array 7.
static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
{
if (slpte & VTD_SL_PT_PAGE_SIZE_MASK) {
/* Maybe large page */
return slpte & vtd_paging_entry_rsvd_field[level + 4];
} else {
return slpte & vtd_paging_entry_rsvd_field[level];
}
}
>
> --
> Peter Xu
Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
On Sun, Sep 29, 2019 at 10:02:20AM +0800, Peter Xu wrote: > On Sun, Sep 29, 2019 at 01:11:12AM +, Zhang, Qi1 wrote: > > > > > > > -Original Message- > > > From: Peter Xu > > > Sent: Friday, September 27, 2019 5:32 PM > > > To: Zhang, Qi1 > > > Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com; > > > pbonz...@redhat.com; r...@twiddle.net; Qi, Yadong > > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved > > > bits > > > > > > On Fri, Sep 27, 2019 at 08:03:21AM +, Zhang, Qi1 wrote: > > > > > > > > > > > > > -Original Message- > > > > > From: Peter Xu > > > > > Sent: Friday, September 27, 2019 2:10 PM > > > > > To: Zhang, Qi1 > > > > > Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com; > > > > > pbonz...@redhat.com; r...@twiddle.net > > > > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in > > > > > reserved bits > > > > > > > > > > On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zh...@intel.com wrote: > > > > > > From: "Zhang, Qi" > > > > > > > > > > > > When dt is supported, TM field should not be Reserved(0). > > > > > > > > > > > > Refer to VT-d Spec 9.8 > > > > > > > > > > > > Signed-off-by: Zhang, Qi > > > > > > Signed-off-by: Qi, Yadong > > > > > > --- > > > > > > hw/i386/intel_iommu.c | 12 ++-- > > > > > > hw/i386/intel_iommu_internal.h | 25 +++-- > > > > > > 2 files changed, 25 insertions(+), 12 deletions(-) > > > > > > --- > > > VTD_SPTE_PAGE_L2_RSVD_MASK(s- > > > > > >aw_bits); > > > > > > -vtd_paging_entry_rsvd_field[3] = > > > > > >aw_bits); > > > > > > +vtd_paging_entry_rsvd_field[5] = > > > > > VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu- > > > > > >dt_supported); > > > > > > +vtd_paging_entry_rsvd_field[6] = > > > > > VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu- > > > > > >dt_supported); > > > > > > +vtd_paging_entry_rsvd_field[7] = > > > > > > + VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu- > > > > > >dt_supported); > > > > > > vtd_paging_entry_rsvd_field[8] = > > > > > >VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits); > > > > > > > > > > Should this TM bit only affects leaves? Say, entry 1 (4K), 5 (2M), 6 > > > > > (1G). > > [1] > > > > > > While this reminded me that I'm totally confused on why we have had > > > > > entry 7, 8 after all... Are they really used? > > > > Yes. TM bit only affects. To this array, index 1, 5,6,7 may be leaf. > > > > Will update > > > a new patchset for it. > > > > > > Could I ask why index 7 may be leaf? > > Index 7 is PDPE 1G GB leaf. > > I thought 1G was index 6. I've listed my understanding above [1]. > Would you please double confirm? Thanks, Oh wait, You are right... Index 6 should be for 1G because index 5 seems to be unused as well. However then again we should drop 5 instead of 7? I think we can do this in two patches: Patch 1 to clean these up by only let vtd_paging_rsvd (we can rename it to shorter one like this if going to touch it) to keep reserved bits for non-huge pages. Then we define a new struct (e.g. vtd_paging_rsvd_huge) to only keep the huge page entries. The thing is that I see no point in keeping huge && non-huge in a single array (and I believe that's why it caused confusion so far...). That new one should be a size of 2 array. Meanwhile we need to fix vtd_slpte_nonzero_rsvd() too using the new arrays. Then in patch 2 we do the DT bit change. Does that look ok? Thanks, -- Peter Xu
Why on earth is this code giving me Segfaults?
Hi All,
I have a custom ISA that's based on MIPS. The LW and SW instructions' opcodes
are changed into 0x17(OPC_BGTZL) and 0x1F(OPC_SPECIAL3).
I have made the following changes in target/mips/translate.c:
@@ -29331,7 +29331,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext
*ctx)
decode_opc_special3(env, ctx);
}
#else
-decode_opc_special3(env, ctx);
+if (ctx->insn_flags & INSN_MYISA) {
+gen_st(ctx, OPC_SW, rt, rs, imm); /* OPC_MYISA_SW */
+} else {
+decode_opc_special3(env, ctx);
+}
#endif
break;
case OPC_REGIMM:
@@ -29589,6 +29603,8 @@ static void decode_opc(CPUMIPSState *env, DisasContext
*ctx)
}
/* OPC_BGTZC, OPC_BLTZC, OPC_BLTC */
gen_compute_compact_branch(ctx, op, rs, rt, imm << 2);
+} else if (ctx->insn_flags & INSN_MYISA) {
+gen_ld(ctx, OPC_LW, rt, rs, imm); /* OPC_MYISA_LW */
} else {
/* OPC_BGTZL */
gen_compute_branch(ctx, op, 4, rs, rt, imm << 2, 4);
I used gdbstub to singlestep my program, and I found that my sw instruction is
working fine, but lw gives me a segfault. I have been stuck on this for a long
while, since it looks like I only need to add that one line of gen_ld function.
I also tried debugging QEMU wtih gdb, but the segfault wasn't thrown
immediately after lw instruction like gdbstub does.
Does anyone have any advice?
Thanks,
Libo Zhou
Re: [PATCH 01/14] hw/arm/raspi: Use the IEC binary prefix definitions
El mié, 04-09-2019 a las 19:13 +0200, Philippe Mathieu-Daudé escribió:
> IEC binary prefixes ease code review: the unit is explicit.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/arm/raspi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 74c062d05e..615d755879 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -230,7 +230,7 @@ static void raspi2_machine_init(MachineClass *mc)
> mc->max_cpus = BCM283X_NCPUS;
> mc->min_cpus = BCM283X_NCPUS;
> mc->default_cpus = BCM283X_NCPUS;
> -mc->default_ram_size = 1024 * 1024 * 1024;
> +mc->default_ram_size = 1 * GiB;
> mc->ignore_memory_transaction_failures = true;
> };
> DEFINE_MACHINE("raspi2", raspi2_machine_init)
> @@ -252,7 +252,7 @@ static void raspi3_machine_init(MachineClass *mc)
> mc->max_cpus = BCM283X_NCPUS;
> mc->min_cpus = BCM283X_NCPUS;
> mc->default_cpus = BCM283X_NCPUS;
> -mc->default_ram_size = 1024 * 1024 * 1024;
> +mc->default_ram_size = 1 * GiB;
> }
> DEFINE_MACHINE("raspi3", raspi3_machine_init)
> #endif
Reviewed-by: Esteban Bosse
