[Qemu-devel] [Bug 1755912] Re: qemu-system-x86_64 crashed with SIGABRT when using option -vga qxl
This bug was fixed in the package qemu - 1:2.12+dfsg-3ubuntu3 --- qemu (1:2.12+dfsg-3ubuntu3) cosmic; urgency=medium * d/p/lp-1755912-qxl-fix-local-renderer-crash.patch: Fix an issue triggered by migrations with UI frontends or frequent guest resolution changes (LP: #1755912) -- Christian Ehrhardt Thu, 19 Jul 2018 08:26:52 +0200 ** Changed in: qemu (Ubuntu) Status: Triaged => Fix Released -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1755912 Title: qemu-system-x86_64 crashed with SIGABRT when using option -vga qxl Status in QEMU: Fix Committed Status in qemu package in Ubuntu: Fix Released Status in qemu source package in Bionic: New Bug description: When using qemu-system-x86_64 with the option -vga qxl, it crashes. The easiest way to crash it is by trying to change the guest's resolution. However, the system may randomly crash too, not happening only when changing resolution. Here is the terminal output of one of these random crashes: $ qemu-system-x86_64 -hda /dev/sdb -m 2048 -enable-kvm -cpu host -vga qxl -nodefaults -netdev user,id=hostnet0 -device virtio-net-pci,id=net0,netdev=hostnet0 WARNING: Image format was not specified for '/dev/sdb' 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. (process:21313): Spice-WARNING **: 16:01:45.759: display- channel.c:2431:display_channel_validate_surface: canvas address is 0x7f8eb948ab18 for 0 (and is NULL) (process:21313): Spice-WARNING **: 16:01:45.759: display-channel.c:2432:display_channel_validate_surface: failed on 0 (process:21313): Spice-CRITICAL **: 16:01:45.759: display-channel.c:2035:display_channel_update: condition `display_channel_validate_surface(display, surface_id)' failed Abortado (imagem do núcleo gravada) I was running QEMU as a normal user which is on the groups kvm and disk. Initially I supposed the problem was because I was running QEMU as root, but as a normal user this happens too. I have tested with guests with different Ubuntu version: 18.04, 17.10 and 16.04. It is happening with them all. ProblemType: Crash DistroRelease: Ubuntu 18.04 Package: qemu-system-x86 1:2.11+dfsg-1ubuntu4 ProcVersionSignature: Ubuntu 4.15.0-10.11-generic 4.15.3 Uname: Linux 4.15.0-10-generic x86_64 ApportVersion: 2.20.8-0ubuntu10 Architecture: amd64 CurrentDesktop: XFCE Date: Wed Mar 14 17:13:52 2018 ExecutablePath: /usr/bin/qemu-system-x86_64 InstallationDate: Installed on 2017-06-13 (273 days ago) InstallationMedia: Xubuntu 17.04 "Zesty Zapus" - Release amd64 (20170412) KvmCmdLine: COMMAND STAT EUID RUID PID PPID %CPU COMMAND MachineType: LENOVO 80UG ProcCmdline: qemu-system-x86_64 -hda /dev/sdb -smp cpus=2 -m 512 -enable-kvm -cpu host -vga qxl -nodefaults -netdev user,id=hostnet0 -device virtio-net-pci,id=net0,netdev=hostnet0 ProcKernelCmdLine: BOOT_IMAGE=/boot/vmlinuz-4.15.0-10-generic.efi.signed root=UUID=6b4ae5c0-c78c-49a6-a1ba-029192618a7a ro quiet Signal: 6 SourcePackage: qemu StacktraceTop: () at /usr/lib/x86_64-linux-gnu/libspice-server.so.1 () at /usr/lib/x86_64-linux-gnu/libspice-server.so.1 () at /usr/lib/x86_64-linux-gnu/libspice-server.so.1 () at /usr/lib/x86_64-linux-gnu/libspice-server.so.1 () at /usr/lib/x86_64-linux-gnu/libspice-server.so.1 Title: qemu-system-x86_64 crashed with SIGABRT UpgradeStatus: Upgraded to bionic on 2017-10-20 (145 days ago) UserGroups: adm bluetooth cdrom dialout dip disk kvm libvirt lpadmin netdev plugdev sambashare sudo dmi.bios.date: 07/10/2017 dmi.bios.vendor: LENOVO dmi.bios.version: 0XCN43WW dmi.board.asset.tag: NO Asset Tag dmi.board.name: Toronto 4A2 dmi.board.vendor: LENOVO dmi.board.version: SDK0J40679 WIN dmi.chassis.asset.tag: NO Asset Tag dmi.chassis.type: 10 dmi.chassis.vendor: LENOVO dmi.chassis.version: Lenovo ideapad 310-14ISK dmi.modalias: dmi:bvnLENOVO:bvr0XCN43WW:bd07/10/2017:svnLENOVO:pn80UG:pvrLenovoideapad310-14ISK:rvnLENOVO:rnToronto4A2:rvrSDK0J40679WIN:cvnLENOVO:ct10:cvrLenovoideapad310-14ISK: dmi.product.family: IDEAPAD dmi.product.name: 80UG dmi.product.version: Lenovo ideapad 310-14ISK dmi.sys.vendor: LENOVO To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1755912/+subscriptions
Re: [Qemu-devel] [PATCH v1 06/17] background snapshot: add helpers to manage a copy of ram block list
On Wed, Jul 18, 2018 at 06:41:49PM +0300, Denis Plotnikov wrote:
> The VM ramblock list may be changed during the snapshotting.
> We want to make sure that we have the same ramblock list as it
> was at the time of snapshot beginning.
> So, we create a copy of the list at the beginning of the snapshotting
> and use it during the process.
Could I ask why the list might change?
>
> Signed-off-by: Denis Plotnikov
> ---
> migration/ram.c | 51 +
> migration/ram.h | 6 ++
> 2 files changed, 57 insertions(+)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 021d583b9b..4399c31606 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1508,6 +1508,57 @@ static int ram_save_host_page(RAMState *rs,
> PageSearchStatus *pss,
> return pages;
> }
>
> +/* BackGround Snapshot Blocks */
> +static RamBlockList bgs_blocks;
> +
> +static RamBlockList *ram_bgs_block_list_get(void)
> +{
> +return &bgs_blocks;
> +}
> +
> +void ram_block_list_create(void)
> +{
> +RAMBlock *block = NULL;
> +RamBlockList *block_list = ram_bgs_block_list_get();
> +
> +qemu_mutex_lock_ramlist();
> +QLIST_FOREACH(block, &ram_list.blocks, next) {
Even if we want to copy a list of ramblocks, we possibly don't need
the ones that aren't migratable? Please refer to
RAMBLOCK_FOREACH_MIGRATABLE(). Then I think...
> +memory_region_ref(block->mr);
> +QLIST_INSERT_HEAD(block_list, block, bgs_next);
> +}
> +qemu_mutex_unlock_ramlist();
> +}
> +
> +void ram_block_list_destroy(void)
> +{
> +RAMBlock *next, *block;
> +RamBlockList *block_list = ram_bgs_block_list_get();
> +
> +QLIST_FOREACH_SAFE(block, block_list, bgs_next, next) {
> +QLIST_REMOVE(block, bgs_next);
> +memory_region_unref(block->mr);
> +}
> +}
> +
> +RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset)
> +{
> +RAMBlock *block = NULL;
> +RamBlockList *block_list = ram_bgs_block_list_get();
> +
> +QLIST_FOREACH(block, block_list, bgs_next) {
> +/* This case append when the block is not mapped. */
> +if (block->host == NULL) {
... things like this should not happen.
Thanks,
> +continue;
> +}
> +
> +if (address - block->host < block->max_length) {
> +*page_offset = (address - block->host) & TARGET_PAGE_MASK;
> +return block;
> +}
> +}
> +
> +return NULL;
> +}
> /**
> * ram_find_and_save_block: finds a dirty page and sends it to f
> *
> diff --git a/migration/ram.h b/migration/ram.h
> index 64d81e9f1d..385579cae9 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -31,6 +31,7 @@
>
> #include "qemu-common.h"
> #include "exec/cpu-common.h"
> +#include "exec/ramlist.h"
>
> extern MigrationStats ram_counters;
> extern XBZRLECacheStats xbzrle_counters;
> @@ -61,5 +62,10 @@ void ram_handle_compressed(void *host, uint8_t ch,
> uint64_t size);
> int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr);
> void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr);
> void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr, size_t
> nr);
> +/* For background snapshot */
> +void ram_block_list_create(void);
> +void ram_block_list_destroy(void);
> +
> +RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset);
>
> #endif
> --
> 2.17.0
>
>
--
Peter Xu
Re: [Qemu-devel] [PATCH v1 05/17] ram: extend the data structures for background snapshotting
On Wed, Jul 18, 2018 at 06:41:48PM +0300, Denis Plotnikov wrote:
> Signed-off-by: Denis Plotnikov
> ---
> include/exec/ram_addr.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 6cbc02aa0f..5b403d537d 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -36,6 +36,8 @@ struct RAMBlock {
> char idstr[256];
> /* RCU-enabled, writes protected by the ramlist lock */
> QLIST_ENTRY(RAMBlock) next;
> +/* blocks used for background snapshot */
> +QLIST_ENTRY(RAMBlock) bgs_next;
> QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
> int fd;
> size_t page_size;
> @@ -49,6 +51,11 @@ struct RAMBlock {
> unsigned long *unsentmap;
> /* bitmap of already received pages in postcopy */
> unsigned long *receivedmap;
> +/* The following 2 are for background snapshot */
> +/* Pages currently being copied */
> +unsigned long *touched_map;
> +/* Pages has been copied already */
> +unsigned long *copied_map;
> };
>
> static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
> --
> 2.17.0
>
>
IMHO these new fields can be put into the patches where they are
firstly used. Isolating them do not really help much for anything
AFAICT...
Regards,
--
Peter Xu
Re: [Qemu-devel] [PATCH v2] nvic: Change NVIC to support ARMv6-M
On 19.07.2018 19:25, Peter Maydell wrote: On 19 July 2018 at 13:16, Julia Suvorova wrote: The differences from ARMv7-M NVIC are: * ARMv6-M only supports up to 32 external interrupts (configurable feature already). The ICTR is reserved. * Active Bit Register is reserved. * ARMv6-M supports 4 priority levels against 256 in ARMv7-M. Signed-off-by: Julia Suvorova --- v2: * Added num_prio_bits field * AIRCR.PRIGROUP is set as RAZ/WI for Baseline Applied to target-arm.for-3.1, thanks. It seems like you applied the first version of this patch. Can you check this, please? Best regards, Julia Suvorova.
[Qemu-devel] [PATCH] secondary-vga: unregister vram on unplug.
From: "Remy Noel" When removing a secondary-vga device and then adding it back (or adding an other one), qemu aborts with: "RAMBlock ":00:02.0/vga.vram" already registered, abort!". It is caused by the vram staying registered, preventing vga replugging. Signed-off-by: Remy Noel --- hw/display/vga-pci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c index e9e62eac70..1c89571e46 100644 --- a/hw/display/vga-pci.c +++ b/hw/display/vga-pci.c @@ -288,6 +288,7 @@ static void pci_secondary_vga_exit(PCIDevice *dev) VGACommonState *s = &d->vga; graphic_console_close(s->con); +vmstate_unregister_ram(&s->vram, DEVICE(dev)); } static void pci_secondary_vga_init(Object *obj) -- 2.18.0
Re: [Qemu-devel] [Qemu-block] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
On Thu, 07/19 13:57, John Snow wrote: > Should we instead modify the test in this case to not attempt to take a > lock on a device we know cannot meaningfully store state, or is it your > preference to attempt to maintain such a list in the raw driver itself? > > I guess we never want QEMU to try to lock things like /dev/zero, but I > don't know if there are more such pseudo-devices we should never try to > lock and if so, what common property unifies them such that we don't > have to maintain a list. They are 0 sized anyway so I only expect them to be used in test cases like the one we're fixing. So this really doesn't matter. An exceptional one would be /dev/nullb* but that is not used in real world either. I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb* are missing), this patch is just trying to make writing test code easier. Fam
[Qemu-devel] [PATCH] vhost: check region type before casting
Check region type first before casting the memory region
to IOMMUMemoryRegion. Otherwise QEMU will abort with below
error message when casting non-IOMMU memory region:
vhost_iommu_region_add: Object 0x561f28bce4f0 is not an
instance of type qemu:iommu-memory-region
Fixes: cb1efcf462a2 ("iommu: Add IOMMU index argument to notifier APIs")
Cc: Peter Maydell
Signed-off-by: Tiwei Bie
---
hw/virtio/vhost.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index b129cb9ddd..d4cb5894a8 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -663,12 +663,14 @@ static void vhost_iommu_region_add(MemoryListener
*listener,
struct vhost_iommu *iommu;
Int128 end;
int iommu_idx;
-IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
+IOMMUMemoryRegion *iommu_mr;
if (!memory_region_is_iommu(section->mr)) {
return;
}
+iommu_mr = IOMMU_MEMORY_REGION(section->mr);
+
iommu = g_malloc0(sizeof(*iommu));
end = int128_add(int128_make64(section->offset_within_region),
section->size);
--
2.18.0
Re: [Qemu-devel] [PATCH] migration: fix potential overflow in multifd send
Peter Xu wrote: > I would guess it won't happen normally, but this should ease Coverity. > CID 1394385: Integer handling issues (OVERFLOW_BEFORE_WIDEN) Potentially overflowing expression "pages->used * 8192U" with type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "uint64_t" (64 bits, unsigned). > 854 transferred = pages->used * TARGET_PAGE_SIZE + p->packet_len; > > Fixes: CID 1394385 > CC: Juan Quintela > Signed-off-by: Peter Xu Reviewed-by: Juan Quintela a - I hate C promotion rules b - why gcc don't warn me c - it don't matter. If the size of the package is bigger than 4GB, we have other problems already. Thanks, Juan.
Re: [Qemu-devel] [PATCH v1 10/17] background snapshots: adapt the page queueing code for using page copies
On Wed, Jul 18, 2018 at 06:41:53PM +0300, Denis Plotnikov wrote:
> The background snapshot uses memeory page copying to seal the page
> memory content. The patch adapts the migration infrastructure to save
> copies of the pages.
Again, since previous page only defined some fields that are firstly
used in this patch, I think you can squash that into this one.
>
> Signed-off-by: Denis Plotnikov
> ---
> migration/migration.c | 2 +-
> migration/ram.c | 59 ---
> migration/ram.h | 3 ++-
> 3 files changed, 47 insertions(+), 17 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 87096d23ef..131d0904e4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1716,7 +1716,7 @@ static void migrate_handle_rp_req_pages(MigrationState
> *ms, const char* rbname,
> return;
> }
>
> -if (ram_save_queue_pages(rbname, start, len)) {
> +if (ram_save_queue_pages(NULL, rbname, start, len, NULL)) {
> mark_source_rp_bad(ms);
> }
> }
> diff --git a/migration/ram.c b/migration/ram.c
> index dc7dfe0726..3c4ccd85b4 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -976,7 +976,12 @@ static int ram_save_page(RAMState *rs, PageSearchStatus
> *pss, bool last_stage)
> RAMBlock *block = pss->block;
> ram_addr_t offset = pss->page << TARGET_PAGE_BITS;
>
> -p = block->host + offset;
> +if (pss->page_copy) {
> +p = pss->page_copy;
> +} else {
> +p = block->host + offset;
> +}
> +
> trace_ram_save_page(block->idstr, (uint64_t)offset, p);
>
> /* In doubt sent page as normal */
> @@ -1003,13 +1008,18 @@ static int ram_save_page(RAMState *rs,
> PageSearchStatus *pss, bool last_stage)
> } else {
> pages = save_zero_page(rs, block, offset, p);
> if (pages > 0) {
> -/* Must let xbzrle know, otherwise a previous (now 0'd) cached
> - * page would be stale
> - */
> -xbzrle_cache_zero_page(rs, current_addr);
> -ram_release_pages(block->idstr, offset, pages);
> +if (pss->page_copy) {
> +qemu_madvise(p, TARGET_PAGE_SIZE, MADV_DONTNEED);
> +} else {
> +/* Must let xbzrle know, otherwise a previous (now 0'd)
> cached
> + * page would be stale
> + */
> +xbzrle_cache_zero_page(rs, current_addr);
> +ram_release_pages(block->idstr, offset, pages);
> +}
> } else if (!rs->ram_bulk_stage &&
> - !migration_in_postcopy() && migrate_use_xbzrle()) {
> + !migration_in_postcopy() && migrate_use_xbzrle() &&
> + !migrate_background_snapshot()) {
> pages = save_xbzrle_page(rs, &p, current_addr, block,
> offset, last_stage);
> if (!last_stage) {
> @@ -1026,9 +1036,10 @@ static int ram_save_page(RAMState *rs,
> PageSearchStatus *pss, bool last_stage)
> ram_counters.transferred +=
> save_page_header(rs, rs->f, block, offset | RAM_SAVE_FLAG_PAGE);
> if (send_async) {
> -qemu_put_buffer_async(rs->f, p, TARGET_PAGE_SIZE,
> - migrate_release_ram() &
> - migration_in_postcopy());
> +bool may_free = migrate_background_snapshot() ||
> +(migrate_release_ram() &&
> + migration_in_postcopy());
> +qemu_put_buffer_async(rs->f, p, TARGET_PAGE_SIZE, may_free);
> } else {
> qemu_put_buffer(rs->f, p, TARGET_PAGE_SIZE);
> }
> @@ -1269,7 +1280,8 @@ static bool find_dirty_block(RAMState *rs,
> PageSearchStatus *pss, bool *again)
> * @rs: current RAM state
> * @offset: used to return the offset within the RAMBlock
> */
> -static RAMBlock *unqueue_page(RAMState *rs, ram_addr_t *offset)
> +static RAMBlock *unqueue_page(RAMState *rs, ram_addr_t *offset,
> + void **page_copy)
> {
> RAMBlock *block = NULL;
>
> @@ -1279,10 +1291,14 @@ static RAMBlock *unqueue_page(RAMState *rs,
> ram_addr_t *offset)
> QSIMPLEQ_FIRST(&rs->src_page_requests);
> block = entry->rb;
> *offset = entry->offset;
> +*page_copy = entry->page_copy;
>
> if (entry->len > TARGET_PAGE_SIZE) {
> entry->len -= TARGET_PAGE_SIZE;
> entry->offset += TARGET_PAGE_SIZE;
> +if (entry->page_copy) {
> +entry->page_copy += TARGET_PAGE_SIZE / sizeof(void *);
> +}
> } else {
> memory_region_unref(block->mr);
> QSIMPLEQ_REMOVE_HEAD(&rs->src_page_requests, next_req);
> @@ -1309,9 +1325,10 @@ static bool get_queued_page(RAMState *rs,
> PageSearchStatus *pss)
> RAMB
Re: [Qemu-devel] [PATCH v2 10/18] qjson: report an error if there are multiple results
Marc-André Lureau writes:
> qobject_from_jsonv() returns a single object. Let's make sure that
> during parsing we don't leak an intermediary object. Instead of
> returning the last object, set a parsing error.
>
> Also, the lexer/parser keeps consuming all the data. There might be an
> error set earlier. Let's keep it and not call json_parser_parse() with
> the remaining data. Eventually, we may teach the message parser to
> stop consuming the data.
Took me a while to figure out what you mean :)
qobject_from_jsonv() feeds a complete string to the JSON parser, with
json_message_parser_feed(). This actually feeds one character after the
other to the lexer, via json_lexer_feed_char().
Whenever a character completes a token, the lexer feeds that token to
the streamer via a callback that is always json_message_process_token().
The attentive reader may wonder what it does with trailing characters
that aren't a complete token. More on that below.
The streamer accumulates tokens, counting various parenthesis. When all
counts are zero or any is negative, the group is complete, and is fed to
the parser via another callback. That callback is parse_json() here.
There are more elsewhere.
The attentive reader may wonder what it does with trailing tokens that
aren't a complete group. More on that below.
The callbacks all pass the tokens to json_parser_parse() to do the
actual parsing. Returns the abstract syntax tree as QObject on success.
Note that the callback can be called any number of times.
In my opinion, this is over-engineered and under-thought. There's more
flexibility than we're using, and the associated complexity breeds bugs.
In particular, parse_json() is wrong:
s->result = json_parser_parse(tokens, s->ap, &s->err);
If the previous call succeeded, we leak its result. This is the leak
you mentioned.
If any previous call set an error, we pass &s->err pointing to non-null,
which is forbidden. If json_parser_parse() passed it to error_setg(),
this would crash. Since it passes it only to error_propagate(), all
errors but the first one are ignored. Latent crash bug.
If the last call succeeds and any previous call failed, the function
simultaneously succeeds and fails: we return both a result and set an
error.
Let's demonstrate these bugs before we fix them, by inserting the
appended patch before this one.
Are the other callbacks similarly buggy? Turns out they're okay:
* handle_qmp_command() consumes result and error on each call.
* process_event() does, too.
* qmp_response() treats errors as fatal, and asserts "no previous
response".
On trailing tokens that don't form a group: they get silently ignored,
as demonstrated by check-qjson test cases unterminated_array(),
unterminated_array_comma(), unterminated_dict(),
unterminated_dict_comma(), all marked BUG.
On trailing characters that don't form a token: they get silently
ignored, as demonstrated by check-qjson test cases
unterminated_string(), unterminated_sq_string(), unterminated_escape(),
all marked BUG, except when they aren't, as in test case
unterminated_literal().
The BUG marks are all gone at the end of this series. I guess you're
fixing all that :)
Note that these "trailing characters / tokens are silently ignored" bugs
*do* affect the other callbacks. Reproducer for handle_qmp_command():
$ echo -e '{ "execute": "qmp_capabilities" }\n{ "execute": "query-name"
}\n"unterminated' | socat UNIX:test-qmp STDIO
{"QMP": {"version": {"qemu": {"micro": 90, "minor": 12, "major": 2},
"package": "v3.0.0-rc1-20-g6a024cd461"}, "capabilities": ["oob"]}}
{"return": {}}
{"return": {}}
Note there's no error reported for the last line.
> Signed-off-by: Marc-André Lureau
> ---
> qobject/qjson.c | 16 +++-
> tests/check-qjson.c | 11 +++
> 2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/qobject/qjson.c b/qobject/qjson.c
> index ee04e61096..8a9d116150 100644
> --- a/qobject/qjson.c
> +++ b/qobject/qjson.c
> @@ -22,6 +22,7 @@
> #include "qapi/qmp/qlist.h"
> #include "qapi/qmp/qnum.h"
> #include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qerror.h"
> #include "qemu/unicode.h"
>
> typedef struct JSONParsingState
> @@ -36,7 +37,20 @@ static void parse_json(JSONMessageParser *parser, GQueue
> *tokens)
> {
> JSONParsingState *s = container_of(parser, JSONParsingState, parser);
>
> -s->result = json_parser_parse(tokens, s->ap, &s->err);
> +if (s->result || s->err) {
> +if (s->result) {
> +qobject_unref(s->result);
> +s->result = NULL;
> +if (!s->err) {
Conditional is necessary because a previous call to json_parser_parse()
could have set s->err already. Without it, error_setg() would fail the
assertion in error_setv() then. Subtle.
> +error_setg(&s->err, QERR_JSON_PARSING);
Hmm. Is this really "Invalid JSON syntax"? In a way, it is, because
RFC 7159 defines JSON-text as a single value.
Re: [Qemu-devel] [PULL for-3.0 0/1] Tracing patches
On 19 July 2018 at 17:25, Stefan Hajnoczi wrote: > The following changes since commit ea6abffa8a08d832feb759d359d5b935e3087cf7: > > Update version for v3.0.0-rc1 release (2018-07-17 18:15:19 +0100) > > are available in the Git repository at: > > git://github.com/stefanha/qemu.git tags/tracing-pull-request > > for you to fetch changes up to db817b8c500a60873eba80cbf047900ae5b32766: > > tracing: Use double-dash spelling for trace option (2018-07-19 13:09:04 > +0100) > > > Pull request > > Contains a fix to use double-dash consistently with tracing command-line > options in documentation and output. > > > > Yaowei Bai (1): > tracing: Use double-dash spelling for trace option > > docs/devel/tracing.txt | 6 +++--- > trace/control.h| 4 ++-- > trace/control.c| 4 ++-- > 3 files changed, 7 insertions(+), 7 deletions(-) > Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH] hw/intc/arm_gicv3_its: downgrade error_report to warn_report in kvm_arm_its_reset
On 20 July 2018 at 02:22, Jia He wrote: > Hi Peter。 Thanks for the comments > > On 7/19/2018 8:41 PM, Peter Maydell Wrote: >> On 19 July 2018 at 04:11, Jia He wrote: >>> In scripts/arch-run.bash of kvm-unit-tests, it will check the qemu >>> output log with: >>> if [ -z "$(echo "$errors" | grep -vi warning)" ]; then >>> >>> Thus without the warning prefix, all of the test fail. >>> >>> Since it is not unrecoverable error in kvm_arm_its_reset for >>> current implementation, downgrading the report from error to >>> warn makes sense. >> >> I think the counterargument would be that this should report >> an error, because you just asked the device to do something >> that it doesn't support (ie to do a clean reset). Since the >> device isn't going to behave correctly, the tests should fail, >> and the way to make them pass is to upgrade to a kernel that >> implements the device correctly (by implementing the necessary >> feature). >> >> But we could maybe move to warn_report() here -- I'm not >> sure what our rules are for what counts as an error and >> what counts as a warning. > I reviewed the other error_report in qemu. Some of them are followed > by exit(), but the rest are not. So it seems there is no definite > rules for error_report. > > IMO, the best resolution is to change the output grep search criteria. > But it is difficult for kvm-unit-tests to identify whether it is an > error without exit() or a warning. The fastest resolution is moving > error_report to warn_report. Well, as I say, this implementation of the ITS *should* fail tests -- it doesn't behave to spec. The fastest resolution is for you to upgrade your host kernel to one that has the bugfix :-) thanks -- PMM
Re: [Qemu-devel] [PATCH v2] nvic: Change NVIC to support ARMv6-M
On 20 July 2018 at 09:09, Julia Suvorova wrote: > On 19.07.2018 19:25, Peter Maydell wrote: >> >> On 19 July 2018 at 13:16, Julia Suvorova wrote: >>> >>> The differences from ARMv7-M NVIC are: >>>* ARMv6-M only supports up to 32 external interrupts >>> (configurable feature already). The ICTR is reserved. >>>* Active Bit Register is reserved. >>>* ARMv6-M supports 4 priority levels against 256 in ARMv7-M. >>> >>> Signed-off-by: Julia Suvorova >>> --- >>> v2: >>> * Added num_prio_bits field >>> * AIRCR.PRIGROUP is set as RAZ/WI for Baseline >> >> >> Applied to target-arm.for-3.1, thanks. > > > It seems like you applied the first version of this patch. > Can you check this, please? Oops, yes, you're right (v2 didn't get into the 'patches' db for some reason). Now fixed. thanks -- PMM
Re: [Qemu-devel] [PATCH] vhost: check region type before casting
On 20 July 2018 at 09:36, Tiwei Bie wrote:
> Check region type first before casting the memory region
> to IOMMUMemoryRegion. Otherwise QEMU will abort with below
> error message when casting non-IOMMU memory region:
>
> vhost_iommu_region_add: Object 0x561f28bce4f0 is not an
> instance of type qemu:iommu-memory-region
>
> Fixes: cb1efcf462a2 ("iommu: Add IOMMU index argument to notifier APIs")
> Cc: Peter Maydell
>
> Signed-off-by: Tiwei Bie
> ---
Reviewed-by: Peter Maydell
thanks
-- PMM
Re: [Qemu-devel] [PATCH v1 07/17] background snapshot: introduce page buffer
On Wed, Jul 18, 2018 at 06:41:50PM +0300, Denis Plotnikov wrote:
> To limit the amount of memory used by the background snapshot
> a memory limiter is used which called "page buffer".
> In fact, it's a memory page counter limiting the page allocation.
> Currently, the limit of pages is hardcoded but its setter is
> the matter of the future work.
>
> The background snapshot makes a copy of the page before writing it
> to the snapshot destination, but it doesn't use a preallocated buffer,
> because the background snapshot employs the migration infrastructure
> which, in turn, uses madvice to release the buffer.
> The advice issuing is hard to track, so here we rely on anonymous
> memory mapping and releasing it by the existing code.
>
> Signed-off-by: Denis Plotnikov
> ---
> migration/ram.c | 91 +
> migration/ram.h | 3 ++
> 2 files changed, 94 insertions(+)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 4399c31606..27d3403435 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -192,6 +192,16 @@ struct RAMSrcPageRequest {
> QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
> };
>
> +/* Page buffer used for background snapshot */
> +typedef struct RAMPageBuffer {
> +/* Page buffer capacity in host pages */
> +int capacity;
> +/* Current number of pages in the buffer */
> +int used;
> +/* Event to notify that buffer usage is under capacity */
> +QemuEvent used_decreased;
> +} RAMPageBuffer;
> +
> /* State of RAM for migration */
> struct RAMState {
> /* QEMUFile used for this migration */
> @@ -230,6 +240,11 @@ struct RAMState {
> /* Queue of outstanding page requests from the destination */
> QemuMutex src_page_req_mutex;
> QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
> +/* The following 2 are for background snapshot */
> +/* Buffer data to store copies of ram pages while async vm saving */
> +RAMPageBuffer page_buffer;
> +/* Event to notify that a page copying just has finished*/
> +QemuEvent page_copying_done;
> };
> typedef struct RAMState RAMState;
>
> @@ -1540,6 +1555,52 @@ void ram_block_list_destroy(void)
> }
> }
>
> +static void ram_page_buffer_decrease_used(void)
> +{
> +qemu_event_reset(&ram_state->page_buffer.used_decreased);
Could I ask why do we need to reset it before set?
> +atomic_dec(&ram_state->page_buffer.used);
> +qemu_event_set(&ram_state->page_buffer.used_decreased);
> +}
> +
> +static void ram_page_buffer_increase_used_wait(void)
> +{
> +int used, *used_ptr;
> +RAMState *rs = ram_state;
> +used_ptr = &rs->page_buffer.used;
> +do {
> +used = atomic_read(used_ptr);
> +if (rs->page_buffer.capacity > used) {
> +if (atomic_cmpxchg(used_ptr, used, used + 1) == used) {
> +return;
> +} else {
> +continue;
> +}
> +} else {
> +qemu_event_wait(&rs->page_buffer.used_decreased);
> +}
> +} while (true);
AFAIU this whole function tries to serialize the usage of
page_buffer.used. Have you ever tried to simply use e.g. a mutex to
protect it, and just implement the function in the simplest (but
complete) way first? I believe this is for performance's sake but I'm
not sure if it will run faster if the critical section of the mutex is
very small (here, only the update of page_buffer_used). It seems that
there are multiple ongoing works on the list that have tried to
implement their own data structures (possibly lockless), including
this one. For example, Guangrong has proposed a lockless fifo just in
~1.5 month:
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg00526.html
Lots of discussions there, and IIUC the conclusion is that we can
consider to just backport an existing implementation from the kernel:
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg08807.html
I'm just hoping that these "data structure" works might not block the
function itself from being merged. Same thing applies to this series.
I never meant that you should drop existing work and do it again using
mutex, but I just spoke this out in case it makes some sense to move
the work forward faster.
> +}
> +
> +void *ram_page_buffer_get(void)
> +{
> +void *page;
> +ram_page_buffer_increase_used_wait();
> +page = mmap(0, TARGET_PAGE_SIZE, PROT_READ | PROT_WRITE,
> +MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +
> +if (page == MAP_FAILED) {
> +ram_page_buffer_decrease_used();
> +page = NULL;
> +}
> +return page;
> +}
> +
> +int ram_page_buffer_free(void *buffer)
> +{
> +ram_page_buffer_decrease_used();
> +return qemu_madvise(buffer, TARGET_PAGE_SIZE, MADV_DONTNEED);
Just to confirm: is this really exactly the same as munmap()?
> +}
> +
> RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset)
> {
> RAMBlock *block = N
Re: [Qemu-devel] [PATCH v1 00/17] Background snapshots
On Wed, Jul 18, 2018 at 06:41:43PM +0300, Denis Plotnikov wrote: > The workflow to make a snapshot is the following: > 1. Pause the vm > 2. Make a snapshot of block devices using the scheme of your choice > 3. Turn on background-snapshot migration capability > 4. Start the migration using the destination (migration stream) of your > choice. >The migration will resume the vm execution by itself >when it has the devices' states saved and is ready to start ram writing >to the migration stream. > 5. Listen to the migration finish event > > The bakground snapshot works with support of KVM patch: > "x86: mmu: report failed memory access to the userspace" > (not applied to the mainstream, it's in the kvm mailing list) Hello, Denis, Do you mind to push your tree to an online repository in case to make review easier? Thanks, -- Peter Xu
Re: [Qemu-devel] [PATCH v1 15/17] kvm: add vCPU failed memeory access processing
On Wed, Jul 18, 2018 at 06:41:58PM +0300, Denis Plotnikov wrote:
> Is done with support of the KVM patch returning the faulting address.
>
> Signed-off-by: Denis Plotnikov
I feel like these two kvm-related patches can be put at the end of the
series as an extension to kvm support. E.g., without these kvm
patches one should still be able to do live snapshot with TCG. Then
even if your KVM patches are not settled (along with the header
update) there's still chance that your framework can be merged first.
> ---
> target/i386/kvm.c | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 3ac5302bc5..55b8860d1a 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -45,6 +45,8 @@
> #include "hw/pci/msi.h"
> #include "hw/pci/msix.h"
> #include "migration/blocker.h"
> +#include "migration/savevm.h"
> +#include "migration/ram.h"
> #include "exec/memattrs.h"
> #include "trace.h"
>
> @@ -3130,6 +3132,18 @@ static bool host_supports_vmx(void)
> return ecx & CPUID_EXT_VMX;
> }
>
> +static int kvm_handle_fail_mem_access(CPUState *cpu)
> +{
> +struct kvm_run *run = cpu->kvm_run;
> +int ret = ram_process_page_fault((void *)run->fail_mem_access.hva);
> +
> +if (ret >= 0) {
> +cpu_resume(cpu);
> +}
> +
> +return ret;
> +}
> +
> #define VMX_INVALID_GUEST_STATE 0x8021
>
> int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> @@ -3188,6 +3202,9 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run
> *run)
> ioapic_eoi_broadcast(run->eoi.vector);
> ret = 0;
> break;
> +case KVM_EXIT_FAIL_MEM_ACCESS:
> +ret = kvm_handle_fail_mem_access(cs);
> +break;
> default:
> fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
> ret = -1;
> --
> 2.17.0
>
>
Regards,
--
Peter Xu
Re: [Qemu-devel] [PULL 0/2] Net patches
On 20 July 2018 at 01:45, Jason Wang wrote: > The following changes since commit 9f2b67e1ca43c84ed37ebd027e7e77a0f2f8ef65: > > Merge remote-tracking branch > 'remotes/alistair/tags/pull-riscv-pull-20180719' into staging (2018-07-19 > 17:21:43 +0100) > > are available in the git repository at: > > https://github.com/jasowang/qemu.git tags/net-pull-request > > for you to fetch changes up to 323e7c117754e4d4ce6b4282d74ad01c99d67714: > > tap: fix memory leak on success to create a tap device (2018-07-20 08:30:49 > +0800) > > > > Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH v1 07/17] background snapshot: introduce page buffer
On 18/07/2018 17:41, Denis Plotnikov wrote:
> +static void ram_page_buffer_decrease_used(void)
> +{
> +qemu_event_reset(&ram_state->page_buffer.used_decreased);
> +atomic_dec(&ram_state->page_buffer.used);
> +qemu_event_set(&ram_state->page_buffer.used_decreased);
As Peter noted, only the qemu_event_set is needed here, while...
> +}
> +
> +static void ram_page_buffer_increase_used_wait(void)
> +{
> +int used, *used_ptr;
> +RAMState *rs = ram_state;
> +used_ptr = &rs->page_buffer.used;
> +do {
> +used = atomic_read(used_ptr);
> +if (rs->page_buffer.capacity > used) {
> +if (atomic_cmpxchg(used_ptr, used, used + 1) == used) {
> +return;
> +} else {
> +continue;
> +}
> +} else {
> +qemu_event_wait(&rs->page_buffer.used_decreased);
> +}
> +} while (true);
... the right idiom for using qemu_event_wait is this:
if test condition
reset
if test condition
wait
So something like
for(;;) {
used = atomic_read(used_ptr);
if (rs->page_buffer.capacity <= used) {
qemu_event_reset(&rs->page_buffer.used_decreased);
used = atomic_read(used_ptr);
if (rs->page_buffer.capacity <= used) {
qemu_event_wait(&rs->page_buffer.used_decreased);
continue;
}
}
if (atomic_cmpxchg(used_ptr, used, used + 1) == used) {
return;
}
}
Note that there must be a single thread doing the above. Otherwise, you
have to use mutex and condvar.
Paolo
Re: [Qemu-devel] [PATCH v2 10/18] qjson: report an error if there are multiple results
Hi
On Fri, Jul 20, 2018 at 10:49 AM, Markus Armbruster wrote:
> Marc-André Lureau writes:
>
>> qobject_from_jsonv() returns a single object. Let's make sure that
>> during parsing we don't leak an intermediary object. Instead of
>> returning the last object, set a parsing error.
>>
>> Also, the lexer/parser keeps consuming all the data. There might be an
>> error set earlier. Let's keep it and not call json_parser_parse() with
>> the remaining data. Eventually, we may teach the message parser to
>> stop consuming the data.
>
> Took me a while to figure out what you mean :)
>
> qobject_from_jsonv() feeds a complete string to the JSON parser, with
> json_message_parser_feed(). This actually feeds one character after the
> other to the lexer, via json_lexer_feed_char().
>
> Whenever a character completes a token, the lexer feeds that token to
> the streamer via a callback that is always json_message_process_token().
>
> The attentive reader may wonder what it does with trailing characters
> that aren't a complete token. More on that below.
>
> The streamer accumulates tokens, counting various parenthesis. When all
> counts are zero or any is negative, the group is complete, and is fed to
> the parser via another callback. That callback is parse_json() here.
> There are more elsewhere.
>
> The attentive reader may wonder what it does with trailing tokens that
> aren't a complete group. More on that below.
>
> The callbacks all pass the tokens to json_parser_parse() to do the
> actual parsing. Returns the abstract syntax tree as QObject on success.
>
> Note that the callback can be called any number of times.
>
> In my opinion, this is over-engineered and under-thought. There's more
> flexibility than we're using, and the associated complexity breeds bugs.
>
> In particular, parse_json() is wrong:
>
> s->result = json_parser_parse(tokens, s->ap, &s->err);
>
> If the previous call succeeded, we leak its result. This is the leak
> you mentioned.
>
> If any previous call set an error, we pass &s->err pointing to non-null,
> which is forbidden. If json_parser_parse() passed it to error_setg(),
> this would crash. Since it passes it only to error_propagate(), all
> errors but the first one are ignored. Latent crash bug.
>
> If the last call succeeds and any previous call failed, the function
> simultaneously succeeds and fails: we return both a result and set an
> error.
>
> Let's demonstrate these bugs before we fix them, by inserting the
> appended patch before this one.
>
> Are the other callbacks similarly buggy? Turns out they're okay:
>
> * handle_qmp_command() consumes result and error on each call.
>
> * process_event() does, too.
>
> * qmp_response() treats errors as fatal, and asserts "no previous
> response".
>
> On trailing tokens that don't form a group: they get silently ignored,
> as demonstrated by check-qjson test cases unterminated_array(),
> unterminated_array_comma(), unterminated_dict(),
> unterminated_dict_comma(), all marked BUG.
>
> On trailing characters that don't form a token: they get silently
> ignored, as demonstrated by check-qjson test cases
> unterminated_string(), unterminated_sq_string(), unterminated_escape(),
> all marked BUG, except when they aren't, as in test case
> unterminated_literal().
>
> The BUG marks are all gone at the end of this series. I guess you're
> fixing all that :)
>
> Note that these "trailing characters / tokens are silently ignored" bugs
> *do* affect the other callbacks. Reproducer for handle_qmp_command():
>
> $ echo -e '{ "execute": "qmp_capabilities" }\n{ "execute": "query-name"
> }\n"unterminated' | socat UNIX:test-qmp STDIO
> {"QMP": {"version": {"qemu": {"micro": 90, "minor": 12, "major": 2},
> "package": "v3.0.0-rc1-20-g6a024cd461"}, "capabilities": ["oob"]}}
> {"return": {}}
> {"return": {}}
>
> Note there's no error reported for the last line.
>
>> Signed-off-by: Marc-André Lureau
>> ---
>> qobject/qjson.c | 16 +++-
>> tests/check-qjson.c | 11 +++
>> 2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/qobject/qjson.c b/qobject/qjson.c
>> index ee04e61096..8a9d116150 100644
>> --- a/qobject/qjson.c
>> +++ b/qobject/qjson.c
>> @@ -22,6 +22,7 @@
>> #include "qapi/qmp/qlist.h"
>> #include "qapi/qmp/qnum.h"
>> #include "qapi/qmp/qstring.h"
>> +#include "qapi/qmp/qerror.h"
>> #include "qemu/unicode.h"
>>
>> typedef struct JSONParsingState
>> @@ -36,7 +37,20 @@ static void parse_json(JSONMessageParser *parser, GQueue
>> *tokens)
>> {
>> JSONParsingState *s = container_of(parser, JSONParsingState, parser);
>>
>> -s->result = json_parser_parse(tokens, s->ap, &s->err);
>> +if (s->result || s->err) {
>> +if (s->result) {
>> +qobject_unref(s->result);
>> +s->result = NULL;
>> +if (!s->err) {
>
> Conditional is necessary because a previous call to json_parser_parse()
> could have set s->err already. Without it, e
Re: [Qemu-devel] [PULL for-3.0 0/1] tcg-next pull
On 20 July 2018 at 05:03, Richard Henderson wrote: > Just one patch for rc2, fixing a bug with an aarch64 host > emulating sve instructions. > > > r~ > > > The following changes since commit e1ea55668ffe6ce558a063f3a9621b761738e1f2: > > Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' > into staging (2018-07-19 15:38:06 +0100) > > are available in the Git repository at: > > https://github.com/rth7680/qemu.git tags/pull-tcg-20180719 > > for you to fetch changes up to e65a5f227d77a5dbae7a7123c3ee915ee4bd80cf: > > tcg/aarch64: limit mul_vec size (2018-07-19 09:07:31 -0700) > > > Fix aarch64 host vector mul > > Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH v1 08/17] migration: add helpers to change VM memory protection rights
* Denis Plotnikov (dplotni...@virtuozzo.com) wrote:
> Signed-off-by: Denis Plotnikov
> ---
> migration/ram.c | 54 +
> migration/ram.h | 3 +++
> 2 files changed, 57 insertions(+)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 27d3403435..ce3dead932 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1555,6 +1555,60 @@ void ram_block_list_destroy(void)
> }
> }
>
> +static int mem_protect(void *addr, uint64_t length, int prot)
> +{
> +int ret = mprotect(addr, length, prot);
> +
> +if (ret < 0) {
> +error_report("%s: Can't change protection on ram block at %p",
> + __func__, addr);
> +}
> +
> +return ret;
> +}
> +
> +static int ram_set_ro(void *addr, uint64_t length)
> +{
> +return mem_protect(addr, length, PROT_READ);
> +}
> +
> +static int ram_set_rw(void *addr, uint64_t length)
> +{
> +return mem_protect(addr, length, PROT_READ | PROT_WRITE);
> +}
> +
> +int ram_block_list_set_readonly(void)
> +{
> +RAMBlock *block = NULL;
> +RamBlockList *block_list = ram_bgs_block_list_get();
> +int ret = 0;
> +
> +QLIST_FOREACH(block, block_list, bgs_next) {
> +ret = ram_set_ro(block->host, block->used_length);
> +if (ret) {
> +break;
> +}
> +}
> +
> +return ret;
> +}
> +
> +int ram_block_list_set_writable(void)
> +{
> +RAMBlock *block = NULL;
> +RamBlockList *block_list = ram_bgs_block_list_get();
> +int ret = 0;
> +
> +QLIST_FOREACH(block, block_list, bgs_next) {
> +ret = ram_set_rw(block->host, block->used_length);
> +if (ret) {
> +break;
> +}
> +}
Do you need to keep track of who was writeable previously
to avoid changing blocks that should be read-only,
Or are all RAMBlocks currently mapped rw?
Dave
> +return ret;
> +}
> +
> static void ram_page_buffer_decrease_used(void)
> {
> qemu_event_reset(&ram_state->page_buffer.used_decreased);
> diff --git a/migration/ram.h b/migration/ram.h
> index 7c802c1d7f..4c463597a5 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -71,4 +71,7 @@ RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t
> *page_offset);
> void *ram_page_buffer_get(void);
> int ram_page_buffer_free(void *buffer);
>
> +int ram_block_list_set_readonly(void);
> +int ram_block_list_set_writable(void);
> +
> #endif
> --
> 2.17.0
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v1 10/17] background snapshots: adapt the page queueing code for using page copies
On Wed, Jul 18, 2018 at 06:41:53PM +0300, Denis Plotnikov wrote:
[...]
> @@ -1003,13 +1008,18 @@ static int ram_save_page(RAMState *rs,
> PageSearchStatus *pss, bool last_stage)
> } else {
> pages = save_zero_page(rs, block, offset, p);
Now save_zero_page() is not called by ram_save_page(). I'd suggest
that you rebase to the latest master in your next post since the code
base looks old.
Thanks,
--
Peter Xu
Re: [Qemu-devel] [v23 1/2] virtio-crypto: Add virtio crypto device specification
On 2018/1/10 1:05, Halil Pasic wrote:
>
>
> On 12/30/2017 10:35 AM, Longpeng(Mike) wrote:
>> From: Gonglei
>>
>> The virtio crypto device is a virtual crypto device (ie. hardware
>> crypto accelerator card). Currently, the virtio crypto device provides
>> the following crypto services: CIPHER, MAC, HASH, and AEAD.
>>
>> In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
>>
>> VIRTIO-153
>>
>> Signed-off-by: Gonglei
>> Signed-off-by: Zhoujian
>> Signed-off-by: Longpeng(Mike)
>> ---
>> acknowledgements.tex |4 +
>> content.tex |2 +
>> virtio-crypto.tex| 1525
>> ++
>> 3 files changed, 1531 insertions(+)
>> create mode 100644 virtio-crypto.tex
>>
>> diff --git a/acknowledgements.tex b/acknowledgements.tex
>> index 2d893d9..cdde247 100644
>> --- a/acknowledgements.tex
>> +++ b/acknowledgements.tex
>> @@ -16,10 +16,13 @@ Daniel Kiper,Oracle \newline
>> Geoff Brown,Machine-to-Machine Intelligence (M2MI) Corporation
>> \newline
>> Gershon Janssen,Individual Member \newline
>> James Bottomley,Parallels IP Holdings GmbH \newline
>> +Jian Zhou, Huawei \newline
>> +Lei Gong, Huawei \newline
>> Luiz Capitulino,Red Hat \newline
>> Michael S. Tsirkin, Red Hat \newline
>> Paolo Bonzini, Red Hat \newline
>> Pawel Moll, ARM \newline
>> +Peng Long, Huawei \newline
>> Richard Sohn, Alcatel-Lucent \newline
>> Rusty Russell, IBM \newline
>> Sasha Levin,Oracle \newline
>> @@ -38,6 +41,7 @@ Brian Foley, ARM \newline
>> David Alan Gilbert, Red Hat \newline
>> Fam Zheng, Red Hat \newline
>> Gerd Hoffmann, Red Hat \newline
>> +Halil Pasic,IBM \newline
>> Jason Wang, Red Hat \newline
>> Laura Novich, Red Hat \newline
>> Patrick Durusau,Technical Advisory Board, OASIS \newline
>> diff --git a/content.tex b/content.tex
>> index c840588..d1d3b09 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -5819,6 +5819,8 @@ descriptor for the \field{sense_len}, \field{residual},
>> \field{status_qualifier}, \field{status}, \field{response} and
>> \field{sense} fields.
>>
>> +\input{virtio-crypto.tex}
>> +
>> \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>
>> Currently these device-independent feature bits defined:
>> diff --git a/virtio-crypto.tex b/virtio-crypto.tex
>> new file mode 100644
>> index 000..4bd5b51
>> --- /dev/null
>> +++ b/virtio-crypto.tex
>> @@ -0,0 +1,1525 @@
>> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
>> +
>> +The virtio crypto device is a virtual cryptography device as well as a
>> +virtual cryptographic accelerator. The virtio crypto device provides the
>> +following crypto services: CIPHER, MAC, HASH, and AEAD. Virtio crypto
>> +devices have a single control queue and at least one data queue. Crypto
>> +operation requests are placed into a data queue, and serviced by the
>> +device. Some crypto operation requests are only valid in the context of a
>> +session. The role of the control queue is facilitating control operation
>> +requests. Sessions management is realized with control operation
>> +requests.
>> +
>> +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
>> +
>> +20
>> +
>> +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device / Virtqueues}
>> +
>> +\begin{description}
>> +\item[0] dataq1
>> +\item[\ldots]
>> +\item[N-1] dataqN
>> +\item[N] controlq
>> +\end{description}
>> +
>> +N is set by \field{max_dataqueues}.
>> +
>> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature
>> bits}
>> +
>> +\begin{description}
>> +\item VIRTIO_CRYPTO_F_REVISION_1 (0) revision 1. Revision 1 has a specific
>> +request format and other enhancements (which result in some additional
>> +requirements).
>> +\item VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE (1) stateless mode requests are
>> +supported by the CIPHER service.
>> +\item VIRTIO_CRYPTO_F_HASH_STATELESS_MODE (2) stateless mode requests are
>> +supported by the HASH service.
>> +\item VIRTIO_CRYPTO_F_MAC_STATELESS_MODE (3) stateless mode requests are
>> +supported by the MAC service.
>> +\item VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE (4) stateless mode requests are
>> +supported by the AEAD service.
>> +\end{description}
>> +
>> +
>> +\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto
>> Device / Feature bits}
>> +
>> +Some crypto feature bits require other crypto feature bits
>> +(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature
>> Bits}):
>> +
>> +\begin{description}
>> +\item[VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE] Requires
>> VIRTIO_CRYPTO_F_REVISION_1.
>> +\item[VIRTIO_CRYPTO_F_HASH_STATELESS_MODE] Requires
>> VIRTIO_CRYPTO_F_REVISION_1.
>> +\item[VIRTIO_CRYPTO_F_MAC_STATELESS_MODE] Requires
>> VIRTIO_CRYPTO_F_REVISION_1.
>> +\item[VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE] Requires
>> VIRT
Re: [Qemu-devel] [RFC v3] qemu: Add virtio pmem device
> /*
> * virtio-balloon-pci: This extends VirtioPCIProxy.
> */
> diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
> new file mode 100644
> index 00..08c96d7e80
> --- /dev/null
> +++ b/hw/virtio/virtio-pmem.c
> @@ -0,0 +1,241 @@
> +/*
> + * Virtio pmem device
> + *
> + * Copyright (C) 2018 Red Hat, Inc.
> + * Copyright (C) 2018 Pankaj Gupta
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "qemu/error-report.h"
> +#include "hw/virtio/virtio-access.h"
> +#include "hw/virtio/virtio-pmem.h"
> +#include "hw/mem/memory-device.h"
> +#include "block/aio.h"
> +#include "block/thread-pool.h"
> +
> +typedef struct VirtIOPMEMresp {
> +int ret;
> +} VirtIOPMEMResp;
> +
> +typedef struct VirtIODeviceRequest {
> +VirtQueueElement elem;
> +int fd;
> +VirtIOPMEM *pmem;
> +VirtIOPMEMResp resp;
> +} VirtIODeviceRequest;
> +
> +static int worker_cb(void *opaque)
> +{
> +VirtIODeviceRequest *req = opaque;
> +int err = 0;
> +
> +/* flush raw backing image */
> +err = fsync(req->fd);
> +if (err != 0) {
> +err = errno;
> +}
> +req->resp.ret = err;
> >>>
> >>> Host question: are you returning the guest errno code to the host?
> >>
> >> No. I am returning error code from the host in-case of host fsync
> >> failure, otherwise returning zero.
> >
> > I think that's what Luiz meant. errno constants are not portable
> > between operating systems and architectures. Therefore they cannot be
> > used in external interfaces in software that expects to communicate with
> > other systems.
> >
> > It will be necessary to define specific constants for virtio-pmem
> > instead of passing errno from the host to guest.
> >
>
> In general, I wonder if we should report errors at all or rather *kill*
> the guest. That might sound harsh, but think about the following scenario:
>
> fsync() fails due to some block that cannot e.g. be written (e.g.
> network connection failed). What happens if our guest tries to
> read/write that mmaped block? (e.g. network connection failed).
>
> I assume we'll get a signal an get killed? So we are trying to optimize
> one special case (fsync()) although every read/write is prone to kill
> the guest. And as soon as the guest will try to access the block that
> made fsync fail, we will crash the guest either way.
>
> I assume the main problem is that we are trying to take a file (with all
> the errors that can happen during read/write/fsync) and make it look
> like memory (dax). On ordinary block access, we can forward errors, but
> not if it's memory (maybe using MCE, but it's complicated and
> architecture specific).
There are two points which you highlighted:
1] Memory hardware errors:
These type of errors will be notified by MCA. If mce is non-recoverable, KVM
gets
SIG_BUS when hardware detects such error and injects mce in guest vCPU. If
guest
does not recoverable it can decide to kill the user-space process.
Default option for mce is '1':
1: panic or SIGBUS on uncorrected errors, log corrected errors
2] read/write/fsync failure because of (network connection failure):
I assume you are talking about something like NFS mount where read/write/fsync
responsibility is taken care by NFS. This scenario can happen for any
application
accessing a network filesystem and return appropriate error or wait. Until
'fsync'
is not performed there is no guarantee ram data is backed. I think its
the responsibility of application to perform fsync after write operation or
a transaction.
>
> So I wonder if we should rather assume that our backend file is placed
> on some stable storage that cannot easily fail.
>
> (we might have the same problem with NVDIMM right now, at least the
> memory reading/writing part)
NVDIMM NFIT handles this handler and checks if any SPA falls in the range
of mce:address. It creates a list of bad blocks(corresponding to nd_region) and
handle
in function 'pmem_do_bvec' used by 'pmem_mem_request' & 'pmem_read_write'.
void nfit_mce_register(void)
{
mce_register_decode_chain(&nfit_mce_dec);
}
In 'fake DAX', we bypass NFIT ACPI and using virtio & nvdimm_bus way of
registering
memory region. By default it should kill the userspace process or at worst
cause guest reboot.
I am thinking how we can integrate the NFIT bad block handling with mce handler
approach
for fake DAX. I think we can do this. But I want inputs from NVDIMM guys?
Thanks,
Pankaj
>
> It's complicated and I am not a block level expert :)
Re: [Qemu-devel] [RFC v3] qemu: Add virtio pmem device
> > > > > +
> > > > > +typedef struct VirtIOPMEMresp {
> > > > > +int ret;
> > > > > +} VirtIOPMEMResp;
> > > > > +
> > > > > +typedef struct VirtIODeviceRequest {
> > > > > +VirtQueueElement elem;
> > > > > +int fd;
> > > > > +VirtIOPMEM *pmem;
> > > > > +VirtIOPMEMResp resp;
> > > > > +} VirtIODeviceRequest;
> > > > > +
> > > > > +static int worker_cb(void *opaque)
> > > > > +{
> > > > > +VirtIODeviceRequest *req = opaque;
> > > > > +int err = 0;
> > > > > +
> > > > > +/* flush raw backing image */
> > > > > +err = fsync(req->fd);
> > > > > +if (err != 0) {
> > > > > +err = errno;
> > > > > +}
> > > > > +req->resp.ret = err;
> > > >
> > > > Host question: are you returning the guest errno code to the host?
> > >
> > > No. I am returning error code from the host in-case of host fsync
> > > failure, otherwise returning zero.
> >
> > I think that's what Luiz meant. errno constants are not portable
> > between operating systems and architectures. Therefore they cannot be
> > used in external interfaces in software that expects to communicate with
> > other systems.
>
> Oh, thanks. Only saw this email now.
>
> > It will be necessary to define specific constants for virtio-pmem
> > instead of passing errno from the host to guest.
>
> Yes, defining your own constants work. But I think the only fsync()
> error that will make sense for the guest is EIO. The other errors
> only make sense for the host.
Agree.
Thanks,
Pankaj
[Qemu-devel] [PATCH] qstring: Fix integer overflow
From: l00425170
The incoming parameters "start" and "end" is int type in
qstring_from_substr(), but this function can be called by
qstring_from_str, which is size_t type in strlen(str).
It may result in coredump when called g_malloc later.
One scene to triger is to call hmp "into tlb", which may have
too long length of string.
Signed-off-by: l00425170
---
include/qapi/qmp/qstring.h | 2 +-
qobject/qstring.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index b3b3d44..3e83e3a 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -24,7 +24,7 @@ struct QString {
QString *qstring_new(void);
QString *qstring_from_str(const char *str);
-QString *qstring_from_substr(const char *str, int start, int end);
+QString *qstring_from_substr(const char *str, size_t start, size_t end);
size_t qstring_get_length(const QString *qstring);
const char *qstring_get_str(const QString *qstring);
const char *qstring_get_try_str(const QString *qstring);
diff --git a/qobject/qstring.c b/qobject/qstring.c
index afca54b..18b8eb8 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -37,7 +37,7 @@ size_t qstring_get_length(const QString *qstring)
*
* Return string reference
*/
-QString *qstring_from_substr(const char *str, int start, int end)
+QString *qstring_from_substr(const char *str, size_t start, size_t end)
{
QString *qstring;
--
1.8.3.1
Re: [Qemu-devel] [PATCH v4 12/20] intc/arm_gic: Implement virtualization extensions in gic_(deactivate|complete_irq)
On 18 July 2018 at 14:22, Luc Michel wrote:
>
>
> On 07/17/2018 03:32 PM, Peter Maydell wrote:
>> On 14 July 2018 at 18:15, Luc Michel wrote:
>>> Implement virtualization extensions in the gic_deactivate_irq() and
>>> gic_complete_irq() functions. When a guest tries to deactivat or end an
>>
>> "deactivate"
>>
>>> IRQ that does not exist in the LRs, the EOICount field of the virtual
>>> interface HCR register is incremented by one, and the request is
>>> ignored.
>>>
>>> Signed-off-by: Luc Michel
>>> ---
>>> hw/intc/arm_gic.c | 18 ++
>>> 1 file changed, 18 insertions(+)
>>>
>>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>>> index be9e2594d9..50cbbfbe24 100644
>>> --- a/hw/intc/arm_gic.c
>>> +++ b/hw/intc/arm_gic.c
>>> @@ -590,6 +590,15 @@ static void gic_deactivate_irq(GICState *s, int cpu,
>>> int irq, MemTxAttrs attrs)
>>> return;
>>> }
>>>
>>> +if (gic_is_vcpu(cpu) && !gic_virq_is_valid(s, irq, cpu)) {
>>> +/* This vIRQ does not have an LR entry which is either active or
>>> + * pending and active. Increment EOICount and ignore the write.
>>> + */
>>> +int rcpu = gic_get_vcpu_real_id(cpu);
>>> +s->h_hcr[rcpu] += 1 << R_GICH_HCR_EOICount_SHIFT;
>>> +return;
>>> +}
>>
>> It's a bit hard to tell from the amount of context in the diff,
>> but I think this check is being done too late in this function.
>> It needs to happen before we do any operations on the irq we're
>> passed (eg checking which group it is).
> For operations on the IRQ, yes. But there is also the test on the
> EOIMode bit in C_CTLR before that. Writing to C_DIR when EOIMode is 0 is
> unpredictable. I was thinking of keeping the same behaviour we had until
> then, which is to completely ignore the write.
Yes, that's a reasonable choice.
>>> +
>>> if (gic_cpu_ns_access(s, cpu, attrs) && !group) {
>>> DPRINTF("Non-secure DI for Group0 interrupt %d ignored\n", irq);
>>> return;
>>> @@ -604,6 +613,15 @@ static void gic_complete_irq(GICState *s, int cpu, int
>>> irq, MemTxAttrs attrs)
>>> int group;
>>>
>>> DPRINTF("EOI %d\n", irq);
>>> +if (gic_is_vcpu(cpu) && !gic_virq_is_valid(s, irq, cpu)) {
>>> +/* This vIRQ does not have an LR entry which is either active or
>>> + * pending and active. Increment EOICount and ignore the write.
>>> + */
>>> +int rcpu = gic_get_vcpu_real_id(cpu);
>>> +s->h_hcr[rcpu] += 1 << R_GICH_HCR_EOICount_SHIFT;
>>> +return;
>>> +}
>>
>> This isn't consistent with the deactivate code about whether we
>> do this check before the "irq >= s->num_irq" check or after.
>>
>> I think the correct answer is "before", because the number of
>> physical interrupts in the GIC shouldn't affect the valid
>> range of virtual interrupts.
>>
>> There is also an edge case here: if the VIRQ written to the
>> EOI or DIR registers is a special interrupt number (1020..1023),
>> then should we increment the EOI count or not? The GICv2 spec
>> is not entirely clear on this point, but it does say in the
>> GICH_HCR.EOICount docs that "EOIs that do not clear a bit in
>> the Active Priorities register GICH_APR do not cause an increment",
>> and the GICv3 spec is clear so my recommendation is that for
>> 1020..1023 we should ignore the write and not increment EOICount.
>>
>> That bit about "EOIs that do not clear a bit in GICH_APR do
>> not increment EOICount" also suggests that our logic for DIR
>> and EOI needs to be something like:
>>
>> if (vcpu) {
>> if (irq > 1020) {
>> return;
>> }
>> clear GICH_HCR bit;
>> if (no bit cleared) {
>> return;
>> }
>> if (!gic_virq_is_valid()) {
>> increment EOICount;
>> return;
>> }
>> clear active bit in LR;
>> }
>>
>> ?
> I agree for EOIR, but for DIR, it seems weird. A write to DIR never
> causes a bit to be cleared in GICH_APR. It can only change the state of
> the LR corresponding to the given vIRQ. So if we read the specification
> this way, a write to DIR should never cause a EOICount increment.
Yes, I think you're right and I was misreading the spec here,
at least where it regards DIR.
> However the part you quoted:
>
> "EOIs that do not clear a bit in the Active Priorities register GICH_APR
> do not cause an increment"
>
> seems to apply to EOIs only, not to interrupt deactivations.
>
> And in the DIR specification:
> "If the specified Interrupt does not exist in the List registers, the
> GICH_HCR.EOIcount field is incremented"
>
> So I would suggest that we apply your reasoning for EOIR. For DIR, I
> think something like this is sufficient:
>
>if (vcpu) {
>if (irq > 1020) {
>return;
>}
>if (!gic_virq_is_valid()) {
>increment EOICount;
>return;
>}
>clear active bit in LR;
>}
>
>
> What do you think?
Yes, I think this is right.
thanks
--
Re: [Qemu-devel] [PATCH v6 11/11] linux-user: Add availability control to some syscalls
> > From: Aleksandar Rikalo > > > > Signed-off-by: Aleksandar Markovic > > Signed-off-by: Stefan Markovic > > --- > > linux-user/strace.c | 14 +- > > linux-user/syscall.c | 25 + > > 2 files changed, 38 insertions(+), 1 deletion(-) If this patch gets a proper commit message (as described in my response): Reviewed-by: Aleksandar Markovic
[Qemu-devel] [PATCH 0/4] target/arm: Implement tailchaining for M profile cores
Tailchaining is an optimization in handling of exception return for M-profile cores: if we are about to pop the exception stack for an exception return, but there is a pending exception which is higher priority than the priority we are returning to, then instead of unstacking and then immediately taking the exception and stacking registers again, we can chain to the pending exception without unstacking and stacking. For v6M and v7M it is IMPDEF whether tailchaining happens for pending exceptions; for v8M this is architecturally required. Implement it in QEMU for all M-profile cores, since in practice v6M and v7M hardware implementations generally do have it. (We were already doing tailchaining for derived exceptions which happened during exception return, like the validity checks and stack access failures; these have always been required to be tailchained for all versions of the architecture.) The first few patches here do some minor cleanup and bug fixing that I noticed while working on this; patch 4 is the actual implementation, which turns out to be pretty trivial. thanks -- PMM Peter Maydell (4): target/arm: Improve exception-taken logging target/arm: Initialize exc_secure correctly in do_v7m_exception_exit() target/arm: Restore M-profile CONTROL.SPSEL before any tailchaining target/arm: Implement tailchaining for M profile cores target/arm/helper.c | 47 ++--- 1 file changed, 36 insertions(+), 11 deletions(-) -- 2.17.1
[Qemu-devel] [PATCH 2/4] target/arm: Initialize exc_secure correctly in do_v7m_exception_exit()
In do_v7m_exception_exit(), we use the exc_secure variable to track
whether the exception we're returning from is secure or non-secure.
Unfortunately the statement initializing this was accidentally
inside an "if (env->v7m.exception != ARMV7M_EXCP_NMI)" conditional,
which meant that we were using the wrong value for NMI handlers.
Move the initialization out to the right place.
Signed-off-by: Peter Maydell
---
target/arm/helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 3ec6114599b..efcadfc7fa9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7052,6 +7052,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
/* For all other purposes, treat ES as 0 (R_HXSR) */
excret &= ~R_V7M_EXCRET_ES_MASK;
}
+exc_secure = excret & R_V7M_EXCRET_ES_MASK;
}
if (env->v7m.exception != ARMV7M_EXCP_NMI) {
@@ -7062,7 +7063,6 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
* which security state's faultmask to clear. (v8M ARM ARM R_KBNF.)
*/
if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
-exc_secure = excret & R_V7M_EXCRET_ES_MASK;
if (armv7m_nvic_raw_execution_priority(env->nvic) >= 0) {
env->v7m.faultmask[exc_secure] = 0;
}
--
2.17.1
[Qemu-devel] [PATCH 3/4] target/arm: Restore M-profile CONTROL.SPSEL before any tailchaining
On exception return for M-profile, we must restore the CONTROL.SPSEL
bit from the EXCRET value before we do any kind of tailchaining,
including for the derived exceptions on integrity check failures.
Otherwise we will give the guest an incorrect EXCRET.SPSEL value on
exception entry for the tailchained exception.
Signed-off-by: Peter Maydell
---
target/arm/helper.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index efcadfc7fa9..ff947416c9d 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7131,6 +7131,16 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
}
}
+/*
+ * Set CONTROL.SPSEL from excret.SPSEL. Since we're still in
+ * Handler mode (and will be until we write the new XPSR.Interrupt
+ * field) this does not switch around the current stack pointer.
+ * We must do this before we do any kind of tailchaining, including
+ * for the derived exceptions on integrity check failures, or we will
+ * give the guest an incorrect EXCRET.SPSEL value on exception entry.
+ */
+write_v7m_control_spsel_for_secstate(env, return_to_sp_process,
exc_secure);
+
if (sfault) {
env->v7m.sfsr |= R_V7M_SFSR_INVER_MASK;
armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SECURE, false);
@@ -7152,12 +7162,6 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
return;
}
-/* Set CONTROL.SPSEL from excret.SPSEL. Since we're still in
- * Handler mode (and will be until we write the new XPSR.Interrupt
- * field) this does not switch around the current stack pointer.
- */
-write_v7m_control_spsel_for_secstate(env, return_to_sp_process,
exc_secure);
-
switch_v7m_security_state(env, return_to_secure);
{
--
2.17.1
[Qemu-devel] [PATCH 1/4] target/arm: Improve exception-taken logging
Improve the exception-taken logging by logging in
v7m_exception_taken() the exception we're going to take
and whether it is secure/nonsecure.
This requires us to move logging at many callsites from after the
call to before it, so that the logging appears in a sensible order.
(This will make tail-chaining produce more useful logs; for the
current callers of v7m_exception_taken() we know which exception
we're going to take, so custom log messages at the callsite sufficed;
for tail-chaining only v7m_exception_taken() knows the exception
number that we're going to tail-chain to.)
Signed-off-by: Peter Maydell
---
target/arm/helper.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6f5bb198c3b..3ec6114599b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6840,6 +6840,8 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr,
bool dotailchain,
bool push_failed = false;
armv7m_nvic_get_pending_irq_info(env->nvic, &exc, &targets_secure);
+qemu_log_mask(CPU_LOG_INT, "...taking pending %s exception %d\n",
+ targets_secure ? "secure" : "nonsecure", exc);
if (arm_feature(env, ARM_FEATURE_V8)) {
if (arm_feature(env, ARM_FEATURE_M_SECURITY) &&
@@ -6913,12 +6915,15 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t
lr, bool dotailchain,
* we might now want to take a different exception which
* targets a different security state, so try again from the top.
*/
+qemu_log_mask(CPU_LOG_INT,
+ "...derived exception on callee-saves register
stacking");
v7m_exception_taken(cpu, lr, true, true);
return;
}
if (!arm_v7m_load_vector(cpu, exc, targets_secure, &addr)) {
/* Vector load failed: derived exception */
+qemu_log_mask(CPU_LOG_INT, "...derived exception on vector table
load");
v7m_exception_taken(cpu, lr, true, true);
return;
}
@@ -7129,9 +7134,9 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
if (sfault) {
env->v7m.sfsr |= R_V7M_SFSR_INVER_MASK;
armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SECURE, false);
-v7m_exception_taken(cpu, excret, true, false);
qemu_log_mask(CPU_LOG_INT, "...taking SecureFault on existing "
"stackframe: failed EXC_RETURN.ES validity check\n");
+v7m_exception_taken(cpu, excret, true, false);
return;
}
@@ -7141,9 +7146,9 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
*/
env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, env->v7m.secure);
-v7m_exception_taken(cpu, excret, true, false);
qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on existing "
"stackframe: failed exception return integrity check\n");
+v7m_exception_taken(cpu, excret, true, false);
return;
}
@@ -7198,10 +7203,10 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
/* Take a SecureFault on the current stack */
env->v7m.sfsr |= R_V7M_SFSR_INVIS_MASK;
armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SECURE, false);
-v7m_exception_taken(cpu, excret, true, false);
qemu_log_mask(CPU_LOG_INT, "...taking SecureFault on existing "
"stackframe: failed exception return integrity "
"signature check\n");
+v7m_exception_taken(cpu, excret, true, false);
return;
}
@@ -7234,6 +7239,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
/* v7m_stack_read() pended a fault, so take it (as a tail
* chained exception on the same stack frame)
*/
+qemu_log_mask(CPU_LOG_INT, "...derived exception on unstacking\n");
v7m_exception_taken(cpu, excret, true, false);
return;
}
@@ -7270,10 +7276,10 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE,
env->v7m.secure);
env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
-v7m_exception_taken(cpu, excret, true, false);
qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on existing "
"stackframe: failed exception return integrity "
"check\n");
+v7m_exception_taken(cpu, excret, true, false);
return;
}
}
@@ -7309,9 +7315,9 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE, false);
env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
ignore_stackfaults = v7m_push_st
[Qemu-devel] [PATCH 4/4] target/arm: Implement tailchaining for M profile cores
Tailchaining is an optimization in handling of exception return
for M-profile cores: if we are about to pop the exception stack
for an exception return, but there is a pending exception which
is higher priority than the priority we are returning to, then
instead of unstacking and then immediately taking the exception
and stacking registers again, we can chain to the pending
exception without unstacking and stacking.
For v6M and v7M it is IMPDEF whether tailchaining happens for pending
exceptions; for v8M this is architecturally required. Implement it
in QEMU for all M-profile cores, since in practice v6M and v7M
hardware implementations generally do have it.
(We were already doing tailchaining for derived exceptions which
happened during exception return, like the validity checks and
stack access failures; these have always been required to be
tailchained for all versions of the architecture.)
Signed-off-by: Peter Maydell
---
target/arm/helper.c | 16
1 file changed, 16 insertions(+)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index ff947416c9d..c9e9f95051c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7162,6 +7162,22 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
return;
}
+/*
+ * Tailchaining: if there is currently a pending exception that
+ * is high enough priority to preempt execution at the level we're
+ * about to return to, then just directly take that exception now,
+ * avoiding an unstack-and-then-stack. Note that now we have
+ * deactivated the previous exception by calling armv7m_nvic_complete_irq()
+ * our current execution priority is already the execution priority we are
+ * returning to -- none of the state we would unstack or set based on
+ * the EXCRET value affects it.
+ */
+if (armv7m_nvic_can_take_pending_exception(env->nvic)) {
+qemu_log_mask(CPU_LOG_INT, "...tailchaining to pending exception\n");
+v7m_exception_taken(cpu, excret, true, false);
+return;
+}
+
switch_v7m_security_state(env, return_to_secure);
{
--
2.17.1
Re: [Qemu-devel] [PATCH] hw/arm/spitz: Move problematic nand_init() code to realize function
On 19 July 2018 at 14:15, Thomas Huth wrote: > nand_init() does not only create the NAND device, it also realizes > the device with qdev_init_nofail() already. So we must not call > nand_init() from an instance_init function like sl_nand_init(), > otherwise we get superfluous NAND devices in the QOM tree after > introspecting the 'sl-nand' device. So move the nand_init() to the > realize function of 'sl-nand' instead. > > Signed-off-by: Thomas Huth > --- > hw/arm/spitz.c | 15 +++ > 1 file changed, 11 insertions(+), 4 deletions(-) > Applied to target-arm.next, thanks. -- PMM
Re: [Qemu-devel] [PATCH for-3.0] po: Don't include comments with location
On 18 July 2018 at 07:10, Stefan Weil wrote: > Those comments change often when ui/gtk.c is changed and are not > really useful. > > Add also a new translation for German (still to be done for all other > languages). > > Signed-off-by: Stefan Weil > --- I think I agree with Philippe that we should split out the mechanical change from the significant one. thanks -- PMM
[Qemu-devel] [PATCH for-3.0 ?] tests/libqtest: Improve kill_qemu() assert
In kill_qemu() we have an assert that checks that the QEMU process
didn't dump core:
assert(!WCOREDUMP(wstatus));
Unfortunately the WCOREDUMP macro here means the resulting message
is not very easy to comprehend on at least some systems:
ahci-test: tests/libqtest.c:113: kill_qemu: Assertion `!(((__extension__
(((union { __typeof(wstatus) __in; int __i; }) { .__in = (wstatus) }).__i))) &
0x80)' failed.
and it doesn't identify what signal the process took.
Instead of using a raw assert, print the information in an
easier to understand way:
/i386/ahci/sanity: libqtest.c: kill_qemu() tried to terminate QEMU process but
it dumped core with signal 11
ahci-test: tests/libqtest.c:118: kill_qemu: Assertion `0' failed.
Aborted (core dumped)
(Of course, the really useful information would be why the QEMU
process dumped core in the first place, but we don't have that
by the time the test program has picked up the exit status.)
Signed-off-by: Peter Maydell
---
In particular, the travis test config that enables gprof
seems to (a) run into this every so often and (b) have the
really unhelpful assertion text quoted above:
https://travis-ci.org/qemu/qemu/jobs/406192798
Maybe for 3.0 since it's only test code.
tests/libqtest.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 098af6aec44..99341e1b47d 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -110,7 +110,13 @@ static void kill_qemu(QTestState *s)
pid = waitpid(s->qemu_pid, &wstatus, 0);
if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
-assert(!WCOREDUMP(wstatus));
+if (WCOREDUMP(wstatus)) {
+fprintf(stderr,
+"libqtest.c: kill_qemu() tried to terminate QEMU "
+"process but it dumped core with signal %d\n",
+WTERMSIG(wstatus));
+assert(0);
+}
}
}
}
--
2.17.1
[Qemu-devel] [kvm-unit-tests PATCH v2 0/4] arm: add GICv2 MMIO tests
I found this in one my branches: this is an updated version of what I sent end of 2016 [1]. I tried to address all comments that Drew and Eric had at the time. Please have a look whether this makes sense. Changelog v1..v2: - made many functions void - use symbolic name for first SPI being number 32 - add test runs with one and three vCPUs - use gic_version() directly - factor out test_byte_access() - drop redundant "filling priorities" test - dropped GICv3 test [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2016-November/022352.html Original cover letter: == The GIC spec mandates certain constraints on how to acccess the MMIO mapped registers, both in terms of which registers are available and also in terms of which bits within a register should be masked, for instance. Since we went through some lengths in the KVM emulation to implement this, it's about time to give this actually a test beyond what the kernel as a GIC user actually implements - for instance we ignore priorities in Linux. This series tries to attack some constraints, on a low-hanging-fruit base. It focusses on some generic registers and the PRIORITY and TARGETS registers of GICv2. GICv3 is not covered yet. This actually revealed genuine bugs in the KVM emulation in the past. KVM passes these tests now, but QEMU fails some UP and 3-way-SMP tests. Cheers, Andre. Andre Przywara (4): mark exit() and abort() as non-returning functions arm/arm64: GIC: basic GICv2 MMIO tests arm/arm64: GICv2: add GICD_IPRIORITYR testing arm/arm64: GICv2: add GICD_ITARGETSR testing arm/gic.c | 213 ++ arm/unittests.cfg | 18 + lib/arm/asm/gic.h | 5 ++ lib/arm/io.c | 1 + lib/libcflat.h| 7 +- lib/powerpc/io.c | 1 + lib/x86/io.c | 1 + 7 files changed, 243 insertions(+), 3 deletions(-) -- 2.14.4
[Qemu-devel] [kvm-unit-tests PATCH v2 2/4] arm/arm64: GIC: basic GICv2 MMIO tests
This adds an MMIO subtest to the GIC test.
It accesses some generic GICv2 registers and does some sanity tests,
like checking for some of them being read-only.
Signed-off-by: Andre Przywara
---
arm/gic.c | 91 +++
arm/unittests.cfg | 18 +++
lib/arm/asm/gic.h | 4 +++
3 files changed, 113 insertions(+)
diff --git a/arm/gic.c b/arm/gic.c
index 5dd958e..23cb9a4 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -3,6 +3,7 @@
*
* GICv2
* + test sending/receiving IPIs
+ * + MMIO access tests
* GICv3
* + test sending/receiving IPIs
*
@@ -303,6 +304,92 @@ static void run_active_clear_test(void)
report_prefix_pop();
}
+static bool test_ro_pattern_32(void *address, u32 pattern, u32 orig)
+{
+ u32 reg;
+
+ writel(pattern, address);
+ reg = readl(address);
+
+ if (reg != orig)
+ writel(orig, address);
+
+ return reg == orig;
+}
+
+static bool test_readonly_32(void *address, bool razwi)
+{
+ u32 orig, pattern;
+
+ orig = readl(address);
+ if (razwi && orig)
+ return false;
+
+ pattern = 0x;
+ if (orig != pattern) {
+ if (!test_ro_pattern_32(address, pattern, orig))
+ return false;
+ }
+
+ pattern = 0xa5a55a5a;
+ if (orig != pattern) {
+ if (!test_ro_pattern_32(address, pattern, orig))
+ return false;
+ }
+
+ pattern = 0;
+ if (orig != pattern) {
+ if (!test_ro_pattern_32(address, pattern, orig))
+ return false;
+ }
+
+ return true;
+}
+
+static void test_typer_v2(uint32_t reg)
+{
+ int nr_gic_cpus = ((reg >> 5) & 0x7) + 1;
+
+ report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus,
+ nr_gic_cpus);
+}
+
+static void gic_test_mmio(void)
+{
+ u32 reg;
+ int nr_irqs;
+ void *gic_dist_base, *idreg;
+
+ switch(gic_version()) {
+ case 0x2:
+ gic_dist_base = gicv2_dist_base();
+ idreg = gic_dist_base + GICD_ICPIDR2;
+ break;
+ case 0x3:
+ report_abort("GICv3 MMIO tests NYI");
+ default:
+ report_abort("GIC version %d not supported", gic_version());
+ }
+
+ reg = readl(gic_dist_base + GICD_TYPER);
+ nr_irqs = GICD_TYPER_IRQS(reg);
+ report_info("number of implemented SPIs: %d", nr_irqs - GIC_FIRST_SPI);
+
+ test_typer_v2(reg);
+
+ report_info("IIDR: 0x%08x", readl(gic_dist_base + GICD_IIDR));
+
+ report("GICD_TYPER is read-only",
+ test_readonly_32(gic_dist_base + GICD_TYPER, false));
+ report("GICD_IIDR is read-only",
+ test_readonly_32(gic_dist_base + GICD_IIDR, false));
+
+ reg = readl(idreg);
+ report("ICPIDR2 is read-only (0x%08x)",
+ test_readonly_32(idreg, false),
+ reg);
+}
+
int main(int argc, char **argv)
{
if (!gic_init()) {
@@ -330,6 +417,10 @@ int main(int argc, char **argv)
on_cpus(ipi_test, NULL);
} else if (strcmp(argv[1], "active") == 0) {
run_active_clear_test();
+ } else if (strcmp(argv[1], "mmio") == 0) {
+ report_prefix_push(argv[1]);
+ gic_test_mmio();
+ report_prefix_pop();
} else {
report_abort("Unknown subtest '%s'", argv[1]);
}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 44b98cf..7f3a321 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -86,6 +86,24 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
extra_params = -machine gic-version=2 -append 'ipi'
groups = gic
+[gicv2-mmio]
+file = gic.flat
+smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
+extra_params = -machine gic-version=2 -append 'mmio'
+groups = gic
+
+[gicv2-mmio-up]
+file = gic.flat
+smp = 1
+extra_params = -machine gic-version=2 -append 'mmio'
+groups = gic
+
+[gicv2-mmio-3p]
+file = gic.flat
+smp = $((($MAX_SMP < 3)?$MAX_SMP:3))
+extra_params = -machine gic-version=2 -append 'mmio'
+groups = gic
+
[gicv3-ipi]
file = gic.flat
smp = $MAX_SMP
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index 2eb4af8..a469645 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -6,10 +6,13 @@
#ifndef _ASMARM_GIC_H_
#define _ASMARM_GIC_H_
+#define GIC_NR_PRIVATE_IRQS32
+#define GIC_FIRST_SPI GIC_NR_PRIVATE_IRQS
/* Distributor registers */
#define GICD_CTLR 0x
#define GICD_TYPER 0x0004
+#define GICD_IIDR 0x0008
#define GICD_IGROUPR 0x0080
#define GICD_ISENABLER 0x0100
#define GICD_ISPENDR 0x0200
@@ -18,6 +21,7 @@
#define GICD_ICACTIVER 0x0380
#define GICD_IPRIORITYR0x0400
#define GICD_SGIR 0x0f00
+#define GIC
[Qemu-devel] [kvm-unit-tests PATCH v2 1/4] mark exit() and abort() as non-returning functions
exit() and abort() are functions that never return, and (at least)
GCC has an attribute to flag those functions accordingly. This allows
the compiler to do further optimizations and to omit various warnings
about uninitialized variables, for instance.
Since the actual "play-dead" function is in (inline) assembly, the
compiler does not recognize its fatal nature, so help it with the
__builtin_unreachable() hint.
Flag the prototypes of our fatal functions accordingly.
Signed-off-by: Andre Przywara
---
lib/arm/io.c | 1 +
lib/libcflat.h | 7 ---
lib/powerpc/io.c | 1 +
lib/x86/io.c | 1 +
4 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/lib/arm/io.c b/lib/arm/io.c
index 603456f..d2c1a07 100644
--- a/lib/arm/io.c
+++ b/lib/arm/io.c
@@ -83,4 +83,5 @@ void exit(int code)
{
chr_testdev_exit(code);
halt(code);
+ __builtin_unreachable();
}
diff --git a/lib/libcflat.h b/lib/libcflat.h
index cc56553..7529958 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -84,8 +84,8 @@ typedef u64 phys_addr_t;
extern void puts(const char *s);
extern int __getchar(void);
extern int getchar(void);
-extern void exit(int code);
-extern void abort(void);
+extern void exit(int code) __attribute__((noreturn));
+extern void abort(void) __attribute__((noreturn));
extern long atol(const char *ptr);
extern char *getenv(const char *name);
@@ -107,7 +107,8 @@ extern void report(const char *msg_fmt, bool pass, ...)
extern void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...)
__attribute__((format(printf, 1, 4)));
extern void report_abort(const char *msg_fmt, ...)
- __attribute__((format(printf, 1, 2)));
+ __attribute__((format(printf, 1, 2)))
+ __attribute__((noreturn));
extern void report_skip(const char *msg_fmt, ...)
__attribute__((format(printf, 1, 2)));
extern void report_info(const char *msg_fmt, ...)
diff --git a/lib/powerpc/io.c b/lib/powerpc/io.c
index 915e12e..217eb07 100644
--- a/lib/powerpc/io.c
+++ b/lib/powerpc/io.c
@@ -35,4 +35,5 @@ void exit(int code)
printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1);
rtas_power_off();
halt(code);
+ __builtin_unreachable();
}
diff --git a/lib/x86/io.c b/lib/x86/io.c
index 7e1c16d..057f579 100644
--- a/lib/x86/io.c
+++ b/lib/x86/io.c
@@ -82,6 +82,7 @@ void exit(int code)
#else
asm volatile("out %0, %1" : : "a"(code), "d"((short)0xf4));
#endif
+ __builtin_unreachable();
}
void __iomem *ioremap(phys_addr_t phys_addr, size_t size)
--
2.14.4
[Qemu-devel] [kvm-unit-tests PATCH v2 3/4] arm/arm64: GICv2: add GICD_IPRIORITYR testing
Some tests for the IPRIORITY registers. The significant number of bits
is IMPLEMENTATION DEFINED, but should be the same for every IRQ.
Also these registers must be byte-accessible.
Check that accesses beyond the implemented IRQ limit are actually
read-as-zero/write-ignore.
Signed-off-by: Andre Przywara
---
arm/gic.c | 79 +++
1 file changed, 79 insertions(+)
diff --git a/arm/gic.c b/arm/gic.c
index 23cb9a4..57a2995 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -354,6 +354,83 @@ static void test_typer_v2(uint32_t reg)
nr_gic_cpus);
}
+#define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff)
+#define REPLACE_BYTE(reg32, byte, new) (((reg32) & ~(0xff << ((byte) * 8))) |\
+ ((new) << ((byte) * 8)))
+
+/*
+ * Some registers are byte accessible, do a byte-wide read and write of known
+ * content to check for this.
+ * Apply a @mask to cater for special register properties.
+ * @pattern contains the value already in the register.
+ */
+static void test_byte_access(void *base_addr, u32 pattern, u32 mask)
+{
+ u32 reg = readb(base_addr + 1);
+
+ report("byte reads successful (0x%08x => 0x%02x)",
+ reg == (BYTE(pattern, 1) & (mask >> 8)),
+ pattern & mask, reg);
+
+ pattern = REPLACE_BYTE(pattern, 2, 0x1f);
+ writeb(BYTE(pattern, 2), base_addr + 2);
+ reg = readl(base_addr);
+ report("byte writes successful (0x%02x => 0x%08x)",
+ reg == (pattern & mask), BYTE(pattern, 2), reg);
+}
+
+static void test_priorities(int nr_irqs, void *priptr)
+{
+ u32 orig_prio, reg, pri_bits;
+ u32 pri_mask, pattern;
+ void *first_spi = priptr + GIC_FIRST_SPI;
+
+ orig_prio = readl(first_spi);
+ report_prefix_push("IPRIORITYR");
+
+ /*
+* Determine implemented number of priority bits by writing all 1's
+* and checking the number of cleared bits in the value read back.
+*/
+ writel(0x, first_spi);
+ pri_mask = readl(first_spi);
+
+ reg = ~pri_mask;
+ report("consistent priority masking (0x%08x)",
+ (((reg >> 16) == (reg & 0x)) &&
+ ((reg & 0xff) == ((reg >> 8) & 0xff))), pri_mask);
+
+ reg = reg & 0xff;
+ for (pri_bits = 8; reg & 1; reg >>= 1, pri_bits--)
+ ;
+ report("implements at least 4 priority bits (%d)",
+ pri_bits >= 4, pri_bits);
+
+ pattern = 0;
+ writel(pattern, first_spi);
+ report("clearing priorities", readl(first_spi) == pattern);
+
+ /* setting all priorities to their max valus was tested above */
+
+ report("accesses beyond limit RAZ/WI",
+ test_readonly_32(priptr + nr_irqs, true));
+
+ writel(pattern, priptr + nr_irqs - 4);
+ report("accessing last SPIs",
+ readl(priptr + nr_irqs - 4) == (pattern & pri_mask));
+
+ pattern = 0xff7fbf3f;
+ writel(pattern, first_spi);
+ report("priorities are preserved",
+ readl(first_spi) == (pattern & pri_mask));
+
+ /* The PRIORITY registers are byte accessible. */
+ test_byte_access(first_spi, pattern, pri_mask);
+
+ report_prefix_pop();
+ writel(orig_prio, first_spi);
+}
+
static void gic_test_mmio(void)
{
u32 reg;
@@ -388,6 +465,8 @@ static void gic_test_mmio(void)
report("ICPIDR2 is read-only (0x%08x)",
test_readonly_32(idreg, false),
reg);
+
+ test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
}
int main(int argc, char **argv)
--
2.14.4
[Qemu-devel] [kvm-unit-tests PATCH v2 4/4] arm/arm64: GICv2: add GICD_ITARGETSR testing
Some tests for the ITARGETS registers.
Bits corresponding to non-existent CPUs must be RAZ/WI.
These registers must be byte-accessible, also check that accesses beyond
the implemented IRQ limit are actually read-as-zero/write-ignore.
Signed-off-by: Andre Przywara
---
arm/gic.c | 43 +++
lib/arm/asm/gic.h | 1 +
2 files changed, 44 insertions(+)
diff --git a/arm/gic.c b/arm/gic.c
index 57a2995..ed5642e 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -431,6 +431,46 @@ static void test_priorities(int nr_irqs, void *priptr)
writel(orig_prio, first_spi);
}
+/* GICD_ITARGETSR is only used by GICv2. */
+static void test_targets(int nr_irqs)
+{
+ void *targetsptr = gicv2_dist_base() + GICD_ITARGETSR;
+ u32 orig_targets;
+ u32 cpu_mask;
+ u32 pattern, reg;
+
+ orig_targets = readl(targetsptr + GIC_FIRST_SPI);
+ report_prefix_push("ITARGETSR");
+
+ cpu_mask = (1 << nr_cpus) - 1;
+ cpu_mask |= cpu_mask << 8;
+ cpu_mask |= cpu_mask << 16;
+
+ /* Check that bits for non implemented CPUs are RAZ/WI. */
+ if (nr_cpus < 8) {
+ writel(0x, targetsptr + GIC_FIRST_SPI);
+ report("bits for %d non-existent CPUs masked",
+ !(readl(targetsptr + GIC_FIRST_SPI) & ~cpu_mask),
+ 8 - nr_cpus);
+ } else {
+ report_skip("CPU masking (all CPUs implemented)");
+ }
+
+ report("accesses beyond limit RAZ/WI",
+ test_readonly_32(targetsptr + nr_irqs, true));
+
+ pattern = 0x0103020f;
+ writel(pattern, targetsptr + GIC_FIRST_SPI);
+ reg = readl(targetsptr + GIC_FIRST_SPI);
+ report("register content preserved (%08x => %08x)",
+ reg == (pattern & cpu_mask), pattern & cpu_mask, reg);
+
+ /* The TARGETS registers are byte accessible. */
+ test_byte_access(targetsptr + GIC_FIRST_SPI, pattern, cpu_mask);
+
+ writel(orig_targets, targetsptr + GIC_FIRST_SPI);
+}
+
static void gic_test_mmio(void)
{
u32 reg;
@@ -467,6 +507,9 @@ static void gic_test_mmio(void)
reg);
test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
+
+ if (gic_version() == 2)
+ test_targets(nr_irqs);
}
int main(int argc, char **argv)
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index a469645..f6dfb90 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -20,6 +20,7 @@
#define GICD_ISACTIVER 0x0300
#define GICD_ICACTIVER 0x0380
#define GICD_IPRIORITYR0x0400
+#define GICD_ITARGETSR 0x0800
#define GICD_SGIR 0x0f00
#define GICD_ICPIDR2 0x0fe8
--
2.14.4
[Qemu-devel] [PATCH 2/2] RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available
The upcoming libseccomp release should have SCMP_ACT_KILL_PROCESS
action (https://github.com/seccomp/libseccomp/issues/96).
SCMP_ACT_KILL_PROCESS is preferable to immediately terminate the
offending process, rather than having the SIGSYS handler running.
Use SECCOMP_GET_ACTION_AVAIL to check availability of kernel support,
as libseccomp will fallback on SCMP_ACT_KILL otherwise, and we still
prefer SCMP_ACT_TRAP.
Signed-off-by: Marc-André Lureau
---
qemu-seccomp.c | 30 +-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index b117a92559..505887d5af 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -20,6 +20,7 @@
#include
#include
#include "sysemu/seccomp.h"
+#include
/* For some architectures (notably ARM) cacheflush is not supported until
* libseccomp 2.2.3, but configure enforces that we are using a more recent
@@ -107,12 +108,39 @@ static const struct QemuSeccompSyscall blacklist[] = {
{ SCMP_SYS(sched_get_priority_min), QEMU_SECCOMP_SET_RESOURCECTL },
};
+static inline int
+qemu_seccomp(unsigned int operation, unsigned int flags, void *args)
+{
+#ifdef __NR_seccomp
+return syscall(__NR_seccomp, operation, flags, args);
+#else
+return -1;
+#endif
+}
+
+static uint32_t qemu_seccomp_get_kill_action(void)
+{
+#if defined(SECCOMP_GET_ACTION_AVAIL) && defined(SCMP_ACT_KILL_PROCESS) && \
+defined(SECCOMP_RET_KILL_PROCESS)
+{
+uint32_t action = SECCOMP_RET_KILL_PROCESS;
+
+if (qemu_seccomp(SECCOMP_GET_ACTION_AVAIL, 0, &action) == 0) {
+return SCMP_ACT_KILL_PROCESS;
+}
+}
+#endif
+
+return SCMP_ACT_TRAP;
+}
+
static int seccomp_start(uint32_t seccomp_opts)
{
int rc = 0;
unsigned int i = 0;
scmp_filter_ctx ctx;
+uint32_t action = qemu_seccomp_get_kill_action();
ctx = seccomp_init(SCMP_ACT_ALLOW);
if (ctx == NULL) {
@@ -125,7 +153,7 @@ static int seccomp_start(uint32_t seccomp_opts)
continue;
}
-rc = seccomp_rule_add_array(ctx, SCMP_ACT_TRAP, blacklist[i].num,
+rc = seccomp_rule_add_array(ctx, action, blacklist[i].num,
blacklist[i].narg, blacklist[i].arg_cmp);
if (rc < 0) {
goto seccomp_return;
--
2.18.0.232.gb7bd9486b0
Re: [Qemu-devel] [kvm-unit-tests PATCH v2 0/4] arm: add GICv2 MMIO tests
On 20 July 2018 at 16:39, Andre Przywara wrote: > I found this in one my branches: this is an updated version of what I sent > end of 2016 [1]. I tried to address all comments that Drew and Eric had at > the time. > Please have a look whether this makes sense. > > Changelog v1..v2: > - made many functions void > - use symbolic name for first SPI being number 32 > - add test runs with one and three vCPUs > - use gic_version() directly > - factor out test_byte_access() > - drop redundant "filling priorities" test > - dropped GICv3 test > > [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2016-November/022352.html > > Original cover letter: > == > The GIC spec mandates certain constraints on how to acccess the MMIO > mapped registers, both in terms of which registers are available and also > in terms of which bits within a register should be masked, for instance. > Since we went through some lengths in the KVM emulation to implement this, > it's about time to give this actually a test beyond what the kernel as a > GIC user actually implements - for instance we ignore priorities in Linux. > > This series tries to attack some constraints, on a low-hanging-fruit base. > It focusses on some generic registers and the PRIORITY and TARGETS registers > of GICv2. GICv3 is not covered yet. > > This actually revealed genuine bugs in the KVM emulation in the past. KVM > passes these tests now, but QEMU fails some UP and 3-way-SMP tests. Would be interesting to see if we've fixed some of the QEMU emulation bugs, either already in master or in Luc's on-list patchset that adds support for emulation of the GICv2 virt extensions... thanks -- PMM
[Qemu-devel] [PATCH 1/2] seccomp: use SIGSYS signal instead of killing the thread
The seccomp action SCMP_ACT_KILL results in immediate termination of
the thread that made the bad system call. However, qemu being
multi-threaded, it keeps running. There is no easy way for parent
process / management layer (libvirt) to know about that situation.
Instead, the default SIGSYS handler when invoked with SCMP_ACT_TRAP
will terminate the program and core dump.
This may not be the most secure solution, but probably better than
just killing the offending thread. SCMP_ACT_KILL_PROCESS has been
added in Linux 4.14 to improve the situation, which I propose to use
by default if available in the next patch.
Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=1594456
Signed-off-by: Marc-André Lureau
---
qemu-seccomp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 9cd8eb9499..b117a92559 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -125,7 +125,7 @@ static int seccomp_start(uint32_t seccomp_opts)
continue;
}
-rc = seccomp_rule_add_array(ctx, SCMP_ACT_KILL, blacklist[i].num,
+rc = seccomp_rule_add_array(ctx, SCMP_ACT_TRAP, blacklist[i].num,
blacklist[i].narg, blacklist[i].arg_cmp);
if (rc < 0) {
goto seccomp_return;
--
2.18.0.232.gb7bd9486b0
[Qemu-devel] [PATCH 0/2] RFC: seccomp action, prefer KILL_PROCESS or TRAP
Hi, The seccomp action SCMP_ACT_KILL results in immediate termination of the thread that made the bad system call. However, qemu being multi-threaded, it keeps running. There is no easy way for parent process / management layer (libvirt) to know about that situation. Instead, the default SIGSYS handler when invoked with SCMP_ACT_TRAP will terminate the program and core dump. This may not be the most secure solution, but probably better than just killing the offending thread. SCMP_ACT_KILL_PROCESS has been added in Linux 4.14 to improve the situation, which I propose to use by default if available. Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1594456 Marc-André Lureau (2): seccomp: use SIGSYS signal instead of killing the thread RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available qemu-seccomp.c | 30 +- 1 file changed, 29 insertions(+), 1 deletion(-) -- 2.18.0.232.gb7bd9486b0
Re: [Qemu-devel] [PATCH for-3.0 ?] tests/libqtest: Improve kill_qemu() assert
On 07/20/2018 12:39 PM, Peter Maydell wrote:
> In kill_qemu() we have an assert that checks that the QEMU process
> didn't dump core:
> assert(!WCOREDUMP(wstatus));
>
> Unfortunately the WCOREDUMP macro here means the resulting message
> is not very easy to comprehend on at least some systems:
>
> ahci-test: tests/libqtest.c:113: kill_qemu: Assertion `!(((__extension__
> (((union { __typeof(wstatus) __in; int __i; }) { .__in = (wstatus) }).__i)))
> & 0x80)' failed.
>
> and it doesn't identify what signal the process took.
>
> Instead of using a raw assert, print the information in an
> easier to understand way:
>
> /i386/ahci/sanity: libqtest.c: kill_qemu() tried to terminate QEMU process
> but it dumped core with signal 11
Less cryptic, indeed.
> ahci-test: tests/libqtest.c:118: kill_qemu: Assertion `0' failed.
This file:line is not very relevant, ...
> Aborted (core dumped)
>
> (Of course, the really useful information would be why the QEMU
> process dumped core in the first place, but we don't have that
> by the time the test program has picked up the exit status.)
>
> Signed-off-by: Peter Maydell
> ---
> In particular, the travis test config that enables gprof
> seems to (a) run into this every so often and (b) have the
> really unhelpful assertion text quoted above:
> https://travis-ci.org/qemu/qemu/jobs/406192798
>
> Maybe for 3.0 since it's only test code.
>
> tests/libqtest.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 098af6aec44..99341e1b47d 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -110,7 +110,13 @@ static void kill_qemu(QTestState *s)
> pid = waitpid(s->qemu_pid, &wstatus, 0);
>
> if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
> -assert(!WCOREDUMP(wstatus));
> +if (WCOREDUMP(wstatus)) {
> +fprintf(stderr,
> +"libqtest.c: kill_qemu() tried to terminate QEMU "
> +"process but it dumped core with signal %d\n",
> +WTERMSIG(wstatus));
> +assert(0);
... what about directly using abort() here?
> +}
> }
> }
> }
>
Reviewed-by: Philippe Mathieu-Daudé
Re: [Qemu-devel] [PATCH for-3.0 ?] tests/libqtest: Improve kill_qemu() assert
On 20 July 2018 at 16:48, Philippe Mathieu-Daudé wrote:
> On 07/20/2018 12:39 PM, Peter Maydell wrote:
>> In kill_qemu() we have an assert that checks that the QEMU process
>> didn't dump core:
>> assert(!WCOREDUMP(wstatus));
>>
>> Unfortunately the WCOREDUMP macro here means the resulting message
>> is not very easy to comprehend on at least some systems:
>>
>> ahci-test: tests/libqtest.c:113: kill_qemu: Assertion `!(((__extension__
>> (((union { __typeof(wstatus) __in; int __i; }) { .__in = (wstatus) }).__i)))
>> & 0x80)' failed.
>>
>> and it doesn't identify what signal the process took.
>>
>> Instead of using a raw assert, print the information in an
>> easier to understand way:
>>
>> /i386/ahci/sanity: libqtest.c: kill_qemu() tried to terminate QEMU process
>> but it dumped core with signal 11
>
> Less cryptic, indeed.
>
>> ahci-test: tests/libqtest.c:118: kill_qemu: Assertion `0' failed.
>
> This file:line is not very relevant, ...
> ... what about directly using abort() here?
I did actually start with that, but decided I'd rather have
the file-and-line in there to direct people more quickly
to the immediate point where we asserted.
thanks
-- PMM
Re: [Qemu-devel] [PATCH 1/2] seccomp: use SIGSYS signal instead of killing the thread
On Fri, Jul 20, 2018 at 05:44:24PM +0200, Marc-André Lureau wrote:
> The seccomp action SCMP_ACT_KILL results in immediate termination of
> the thread that made the bad system call. However, qemu being
> multi-threaded, it keeps running. There is no easy way for parent
> process / management layer (libvirt) to know about that situation.
>
> Instead, the default SIGSYS handler when invoked with SCMP_ACT_TRAP
> will terminate the program and core dump.
>
> This may not be the most secure solution, but probably better than
> just killing the offending thread. SCMP_ACT_KILL_PROCESS has been
> added in Linux 4.14 to improve the situation, which I propose to use
> by default if available in the next patch.
Note that seccomp doesn't promise to protect against all types
of vulnerability in a program. It merely aims to stop the program
executing designated system calls.
Using SCMP_ACT_TRAP still prevents syscal execution to exactly the
same extent that SCMP_ACT_KILL does, so its security level is the
same.
What differs is that the userspace app has option to ignore the
syscall and carry on instead of being killed. A malicous attacker
would thus have option to try to influence other parts of QEMU
todo bad stuff, but if they already have control over the userspace
process to this extent, they can likely do such bad stuff even
before executing the syscalls
So I don't think there's any significant difference in security
protection here. Mostly the difference is just about what the
crash will look like. A full process crash (from the default
signal handler) looks better than a thread crash for the reasons
you've explained.
>
> Related to:
> https://bugzilla.redhat.com/show_bug.cgi?id=1594456
>
> Signed-off-by: Marc-André Lureau
> ---
> qemu-seccomp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 9cd8eb9499..b117a92559 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -125,7 +125,7 @@ static int seccomp_start(uint32_t seccomp_opts)
> continue;
> }
>
> -rc = seccomp_rule_add_array(ctx, SCMP_ACT_KILL, blacklist[i].num,
> +rc = seccomp_rule_add_array(ctx, SCMP_ACT_TRAP, blacklist[i].num,
> blacklist[i].narg, blacklist[i].arg_cmp);
> if (rc < 0) {
> goto seccomp_return;
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH for-3.0 ?] tests/libqtest: Improve kill_qemu() assert
On 07/20/2018 08:49 AM, Peter Maydell wrote:
> On 20 July 2018 at 16:48, Philippe Mathieu-Daudé wrote:
>> On 07/20/2018 12:39 PM, Peter Maydell wrote:
>>> In kill_qemu() we have an assert that checks that the QEMU process
>>> didn't dump core:
>>> assert(!WCOREDUMP(wstatus));
>>>
>>> Unfortunately the WCOREDUMP macro here means the resulting message
>>> is not very easy to comprehend on at least some systems:
>>>
>>> ahci-test: tests/libqtest.c:113: kill_qemu: Assertion `!(((__extension__
>>> (((union { __typeof(wstatus) __in; int __i; }) { .__in = (wstatus)
>>> }).__i))) & 0x80)' failed.
>>>
>>> and it doesn't identify what signal the process took.
>>>
>>> Instead of using a raw assert, print the information in an
>>> easier to understand way:
>>>
>>> /i386/ahci/sanity: libqtest.c: kill_qemu() tried to terminate QEMU process
>>> but it dumped core with signal 11
>>
>> Less cryptic, indeed.
>>
>>> ahci-test: tests/libqtest.c:118: kill_qemu: Assertion `0' failed.
>>
>> This file:line is not very relevant, ...
>
>> ... what about directly using abort() here?
>
> I did actually start with that, but decided I'd rather have
> the file-and-line in there to direct people more quickly
> to the immediate point where we asserted.
You already print the file, just include the line. Perhaps
fprintf(stderr,
"%s:%d: kill_qemu tried to terminate QEMU "
"process but it dumped core with signal %s\n",
__FILE__, __LINE__, strsignal(WTERMSIG(wstatus)));
abort();
Not that I expect the signal to ever be anything other than 11,
and that being one of the handful that are consistent across
pretty much all unix systems. But still.
r~
PS: The bike shed should be blue.
Re: [Qemu-devel] [PATCH v3 14/40] target/mips: Add emulation of misc nanoMIPS instructions (pool32axf)
> From: Richard Henderson
> Sent: Thursday, July 19, 2018 9:13 PM
>
> > case NM_POOL32A7:
> > +{
> > +switch ((ctx->opcode >> 3) & 0x07) {
> > +case NM_POOL32AXF:
> > +gen_pool32axf_nanomips_insn(env, ctx);
> > +break;
> > +}
> > +}
>
> Bad indentation of a block that need not exist anyway.
>
> Otherwise,
> Reviewed-by: Richard Henderson
Outer braces are unnecessary. The switch is missing the default case. This
switch statement is amended in one of subsequent patches, and at the end it
contains four cases, but no default case. The missing default should be fixed
in this patch.
Re: [Qemu-devel] [PATCH 1/4] target/arm: Improve exception-taken logging
On 07/20/2018 07:56 AM, Peter Maydell wrote: > Improve the exception-taken logging by logging in > v7m_exception_taken() the exception we're going to take > and whether it is secure/nonsecure. > > This requires us to move logging at many callsites from after the > call to before it, so that the logging appears in a sensible order. > > (This will make tail-chaining produce more useful logs; for the > current callers of v7m_exception_taken() we know which exception > we're going to take, so custom log messages at the callsite sufficed; > for tail-chaining only v7m_exception_taken() knows the exception > number that we're going to tail-chain to.) > > Signed-off-by: Peter Maydell > --- > target/arm/helper.c | 17 +++-- > 1 file changed, 11 insertions(+), 6 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH 2/4] target/arm: Initialize exc_secure correctly in do_v7m_exception_exit()
On 07/20/2018 07:56 AM, Peter Maydell wrote: > In do_v7m_exception_exit(), we use the exc_secure variable to track > whether the exception we're returning from is secure or non-secure. > Unfortunately the statement initializing this was accidentally > inside an "if (env->v7m.exception != ARMV7M_EXCP_NMI)" conditional, > which meant that we were using the wrong value for NMI handlers. > Move the initialization out to the right place. > > Signed-off-by: Peter Maydell > --- > target/arm/helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH 3/4] target/arm: Restore M-profile CONTROL.SPSEL before any tailchaining
On 07/20/2018 07:56 AM, Peter Maydell wrote: > On exception return for M-profile, we must restore the CONTROL.SPSEL > bit from the EXCRET value before we do any kind of tailchaining, > including for the derived exceptions on integrity check failures. > Otherwise we will give the guest an incorrect EXCRET.SPSEL value on > exception entry for the tailchained exception. > > Signed-off-by: Peter Maydell > --- > target/arm/helper.c | 16 ++-- > 1 file changed, 10 insertions(+), 6 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH 4/4] target/arm: Implement tailchaining for M profile cores
On 07/20/2018 07:56 AM, Peter Maydell wrote: > Tailchaining is an optimization in handling of exception return > for M-profile cores: if we are about to pop the exception stack > for an exception return, but there is a pending exception which > is higher priority than the priority we are returning to, then > instead of unstacking and then immediately taking the exception > and stacking registers again, we can chain to the pending > exception without unstacking and stacking. > > For v6M and v7M it is IMPDEF whether tailchaining happens for pending > exceptions; for v8M this is architecturally required. Implement it > in QEMU for all M-profile cores, since in practice v6M and v7M > hardware implementations generally do have it. > > (We were already doing tailchaining for derived exceptions which > happened during exception return, like the validity checks and > stack access failures; these have always been required to be > tailchained for all versions of the architecture.) > > Signed-off-by: Peter Maydell > --- > target/arm/helper.c | 16 > 1 file changed, 16 insertions(+) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH for-3.0 ?] tests/libqtest: Improve kill_qemu() assert
On 20 July 2018 at 17:14, Richard Henderson wrote: > You already print the file, just include the line. Perhaps > > fprintf(stderr, > "%s:%d: kill_qemu tried to terminate QEMU " > "process but it dumped core with signal %s\n", > __FILE__, __LINE__, strsignal(WTERMSIG(wstatus))); > abort(); I wasn't convinced that strsignal() would be available on all the host OSes we build on (we don't currently use it outside linux-user/), and I definitely didn't think that it merited a configure test for its presence just for a test error message :-) thanks -- PMM
Re: [Qemu-devel] [Qemu-block] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
On 07/19/2018 12:57 PM, John Snow wrote: Should we instead modify the test in this case to not attempt to take a lock on a device we know cannot meaningfully store state, or is it your preference to attempt to maintain such a list in the raw driver itself? I guess we never want QEMU to try to lock things like /dev/zero, but I don't know if there are more such pseudo-devices we should never try to lock and if so, what common property unifies them such that we don't have to maintain a list. One potential common property: /dev/zero and /dev/null are character devices, rather than block devices. Character devices in general don't make much sense for holding stateful images, so it may be as simple as gating our locking based on fstat() distinguishing which type of file we are accessing. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [Qemu-block] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
On 07/20/2018 03:24 AM, Fam Zheng wrote: On Thu, 07/19 13:57, John Snow wrote: Should we instead modify the test in this case to not attempt to take a lock on a device we know cannot meaningfully store state, or is it your preference to attempt to maintain such a list in the raw driver itself? I guess we never want QEMU to try to lock things like /dev/zero, but I don't know if there are more such pseudo-devices we should never try to lock and if so, what common property unifies them such that we don't have to maintain a list. They are 0 sized anyway so I only expect them to be used in test cases like the one we're fixing. So this really doesn't matter. An exceptional one would be /dev/nullb* but that is not used in real world either. I'm not familiar with /dev/nullb* - is that a typo? $ ll /dev/nullb* ls: cannot access '/dev/nullb*': No such file or directory I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb* are missing), this patch is just trying to make writing test code easier. /dev/urandom is also a character device, and also fits in my heuristic that we likely only care about locking of block devices. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-3.0 ?] tests/libqtest: Improve kill_qemu() assert
On 07/20/2018 09:25 AM, Peter Maydell wrote: > On 20 July 2018 at 17:14, Richard Henderson > wrote: >> You already print the file, just include the line. Perhaps >> >> fprintf(stderr, >> "%s:%d: kill_qemu tried to terminate QEMU " >> "process but it dumped core with signal %s\n", >> __FILE__, __LINE__, strsignal(WTERMSIG(wstatus))); >> abort(); > > I wasn't convinced that strsignal() would be available > on all the host OSes we build on (we don't currently use > it outside linux-user/), and I definitely didn't think that > it merited a configure test for its presence just for a > test error message :-) Hmm. It has been in _GNU_SOURCE since the dawn of time and in POSIX since 2008. For non-linux, I peeked at the OpenBSD man page, which says The strsignal() function first appeared in AT&T System V Release 4 UNIX and was reimplemented for NetBSD 1.0. That suggests all of the extant BSDs should have it. MinGW has had the function since 2008. What other hosts do we support? r~
Re: [Qemu-devel] [PATCH] vhost: check region type before casting
Hi, This series failed docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. Type: series Message-id: 20180720083644.1431-1-tiwei@intel.com Subject: [Qemu-devel] [PATCH] vhost: check region type before casting === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-mingw@fedora === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 fatal: unable to access 'https://github.com/patchew-project/qemu/': Encountered end of file error: Could not fetch 3c8cf5a9c21ff8782164d1def7f44bd888713384 Traceback (most recent call last): File "/usr/bin/patchew", line 442, in test_one git_clone_repo(clone, r["repo"], r["head"], logf) File "/usr/bin/patchew", line 48, in git_clone_repo stdout=logf, stderr=logf) File "/usr/lib64/python3.6/subprocess.py", line 291, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['git', 'remote', 'add', '-f', '--mirror=fetch', '3c8cf5a9c21ff8782164d1def7f44bd888713384', 'https://github.com/patchew-project/qemu']' returned non-zero exit status 1. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [Qemu-devel] [PATCH] monitor: print message when using 'help' with an unknown command
On 07/19/2018 11:39 AM, Collin Walling wrote: On 07/19/2018 12:31 PM, Markus Armbruster wrote: You neglected to cc: maintainers. Cc'ing them increases the odds your patch will be noticed and picked up. You can use scripts/get_maintainer.pl to find maintainers. You don't have to do anything for this patch; it got noticed anyway. David, this is yours :) Very true. Was a minor fix that I thought I'd just toss it out there and let anyone view it if they had the time. Will be more aware of who to CC next time around. Or automate it: https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer suggests: git config sendemail.cccmd 'scripts/get_maintainer.pl --nogit-fallback' -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-3.0 ?] tests/libqtest: Improve kill_qemu() assert
On 20 July 2018 at 17:36, Richard Henderson wrote: > On 07/20/2018 09:25 AM, Peter Maydell wrote: >> On 20 July 2018 at 17:14, Richard Henderson >> wrote: >>> You already print the file, just include the line. Perhaps >>> >>> fprintf(stderr, >>> "%s:%d: kill_qemu tried to terminate QEMU " >>> "process but it dumped core with signal %s\n", >>> __FILE__, __LINE__, strsignal(WTERMSIG(wstatus))); >>> abort(); >> >> I wasn't convinced that strsignal() would be available >> on all the host OSes we build on (we don't currently use >> it outside linux-user/), and I definitely didn't think that >> it merited a configure test for its presence just for a >> test error message :-) > > Hmm. It has been in _GNU_SOURCE since the dawn of time > and in POSIX since 2008. > > For non-linux, I peeked at the OpenBSD man page, which says > > The strsignal() function first appeared in AT&T System V > Release 4 UNIX and was reimplemented for NetBSD 1.0. > > That suggests all of the extant BSDs should have it. > > MinGW has had the function since 2008. > > What other hosts do we support? OSX, but that's I think OK as it inherits it from BSD. The configure script also has support for Solaris-variants and Haiku... thanks -- PMM
Re: [Qemu-devel] [PATCH for-3.0 ?] tests/libqtest: Improve kill_qemu() assert
On 07/20/2018 09:45 AM, Peter Maydell wrote: >> For non-linux, I peeked at the OpenBSD man page, which says >> >> The strsignal() function first appeared in AT&T System V >> Release 4 UNIX and was reimplemented for NetBSD 1.0. >> >> That suggests all of the extant BSDs should have it. >> >> MinGW has had the function since 2008. >> >> What other hosts do we support? > > OSX, but that's I think OK as it inherits it from BSD. > The configure script also has support for Solaris-variants > and Haiku... Solaris derives from SVR4, and so should have had it since the beginning of time; certainly OpenSolaris does: http://repo.or.cz/opensolaris.git/blob/HEAD:/usr/src/head/string.h#l94 Haiku has it as well: https://git.haiku-os.org/haiku/tree/headers/posix/string.h#n75 r~
Re: [Qemu-devel] [PATCH] monitor: print message when using 'help' with an unknown command
* Collin Walling (wall...@linux.ibm.com) wrote:
> On 07/19/2018 03:18 PM, Dr. David Alan Gilbert wrote:
> > * Collin Walling (wall...@linux.ibm.com) wrote:
> >> When typing 'help' followed by an unknown command, QEMU will
> >> not print anything to the command line to let the user know
> >> they typed a bad command. Let's fix this by printing a message
> >> to the monitor when this happens. For example:
> >>
> >> (qemu) help xyz
> >> unknown command: 'xyz'
> >>
> >> Reported-by: Stefan Zimmermann
> >> Signed-off-by: Collin Walling
> >> ---
> >> monitor.c | 5 -
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index 7af1f18..7942f9f 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -1034,9 +1034,12 @@ static void help_cmd_dump(Monitor *mon, const
> >> mon_cmd_t *cmds,
> >> } else {
> >> help_cmd_dump_one(mon, cmd, args, arg_index);
> >> }
> >> -break;
> >> +return;
> >> }
> >> }
> >> +
> >> +/* Entry not found */
> >> +monitor_printf(mon, "unknown command: '%s'\n", args[arg_index]);
> >
> > Thanks, that does suffer from a similar bug to the one you fixed a
> > few months back in 317c52cc6aa0d ('monitor: report entirety of hmp
> > command on error'):
> >
> > (qemu) help foo
> > unknown command: 'foo'
> > (qemu) help info foo
> > unknown command: 'foo'
>
> Yeah... my thinking was that "info" is a correct command, so let's instead
> only
> report to the user just the piece that was incorrect.
>
> If it makes better sense to include the whole "info foo" piece, it's certainly
> doable... whichever makes the most sense. Thoughts?
Yes, I'd prefer the full version; unless it makes it particularly
complicated.
> >
> > Dave
> > (And yes, please cc me, otherwise I can miss them)
> >
>
> Will do :)
Thanks,
Dave
> >> }
> >>
> >> static void help_cmd(Monitor *mon, const char *name)
> >> --
> >> 2.7.4
> >>
> >>
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> >
>
>
> --
> Respectfully,
> - Collin Walling
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [Qemu-block] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
On 07/20/2018 04:24 AM, Fam Zheng wrote: > On Thu, 07/19 13:57, John Snow wrote: >> Should we instead modify the test in this case to not attempt to take a >> lock on a device we know cannot meaningfully store state, or is it your >> preference to attempt to maintain such a list in the raw driver itself? >> >> I guess we never want QEMU to try to lock things like /dev/zero, but I >> don't know if there are more such pseudo-devices we should never try to >> lock and if so, what common property unifies them such that we don't >> have to maintain a list. > > They are 0 sized anyway so I only expect them to be used in test cases like > the > one we're fixing. So this really doesn't matter. An exceptional one would be > /dev/nullb* but that is not used in real world either. > I agree in general; I'm questioning somewhat if we ought to just patch the test instead of the driver, given that we can't really be consistent or accurate about when we decide not to take the lock in the driver; unless we use something like Eric's suggestion (we don't take locks on char devices, maybe?) > I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb* > are missing), this patch is just trying to make writing test code easier. > Yeah, it's the little quality-of-life band-aid that I'm wondering if we ought to stick in here. Granted, this fixes a test that's broken, so... Reviewed-by: John Snow I'll let Kevin decide if we ought to patch it nicer than this. --js > Fam >
Re: [Qemu-devel] [PATCH] block/file-posix: add bdrv_attach_aio_context callback for host dev and cdrom
I am seeing another issue pop up, in a different test. Even though it's
a different assertion, it might be related based on the call trace.
Stack trace of thread 276199:
#0 0x03ff8473e274 raise (libc.so.6)
#1 0x03ff847239a8 abort (libc.so.6)
#2 0x03ff847362ce __assert_fail_base (libc.so.6)
#3 0x03ff8473634c __assert_fail (libc.so.6)
#4 0x02aa30aba0c4 iov_memset (qemu-system-s390x)
#5 0x02aa30aba9a6 qemu_iovec_memset (qemu-system-s390x)
#6 0x02aa30a23e88 qemu_laio_process_completion (qemu-system-s390x)
#7 0x02aa30a23f68 qemu_laio_process_completions (qemu-system-s390x)
#8 0x02aa30a2418e qemu_laio_process_completions_and_submit
(qemu-system-s390x)
#9 0x02aa30a24220 qemu_laio_poll_cb (qemu-system-s390x)
#10 0x02aa30ab22c4 run_poll_handlers_once (qemu-system-s390x)
#11 0x02aa30ab2e78 aio_poll (qemu-system-s390x)
#12 0x02aa30a29f4e bdrv_do_drained_begin (qemu-system-s390x)
#13 0x02aa30a2a276 bdrv_drain (qemu-system-s390x)
#14 0x02aa309d45aa bdrv_set_aio_context (qemu-system-s390x)
#15 0x02aa3085acfe virtio_blk_data_plane_stop (qemu-system-s390x)
#16 0x02aa3096994c virtio_bus_stop_ioeventfd.part.1 (qemu-system-s390x)
#17 0x02aa3087d1d6 virtio_vmstate_change (qemu-system-s390x)
#18 0x02aa308e8a12 vm_state_notify (qemu-system-s390x)
#19 0x02aa3080ed54 do_vm_stop (qemu-system-s390x)
#20 0x02aa307bea04 main (qemu-system-s390x)
#21 0x03ff84723dd2 __libc_start_main (libc.so.6)
#22 0x02aa307c0414 _start (qemu-system-s390x)
The failing assertion is:
qemu-kvm: util/iov.c:78: iov_memset: Assertion `offset == 0' failed.
On 07/18/2018 05:12 PM, Nishanth Aravamudan wrote:
In ed6e2161 ("linux-aio: properly bubble up errors from initialzation"),
I only added a bdrv_attach_aio_context callback for the bdrv_file
driver. There are several other drivers that use the shared
aio_plug callback, though, and they will trip the assertion added to
aio_get_linux_aio because they did not call aio_setup_linux_aio first.
Add the appropriate callback definition to the affected driver
definitions.
Fixes: ed6e2161 ("linux-aio: properly bubble up errors from initialization")
Reported-by: Farhan Ali
Signed-off-by: Nishanth Aravamudan
Cc: Eric Blake
Cc: Kevin Wolf
Cc: John Snow
Cc: Max Reitz
Cc: Stefan Hajnoczi
Cc: Fam Zheng
Cc: Paolo Bonzini
Cc: qemu-bl...@nongnu.org
Cc: qemu-devel@nongnu.org
---
block/file-posix.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 60af4b3d51..ad299beb38 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3158,6 +3158,7 @@ static BlockDriver bdrv_host_device = {
.bdrv_refresh_limits = raw_refresh_limits,
.bdrv_io_plug = raw_aio_plug,
.bdrv_io_unplug = raw_aio_unplug,
+.bdrv_attach_aio_context = raw_aio_attach_aio_context,
.bdrv_co_truncate = raw_co_truncate,
.bdrv_getlength = raw_getlength,
@@ -3280,6 +3281,7 @@ static BlockDriver bdrv_host_cdrom = {
.bdrv_refresh_limits = raw_refresh_limits,
.bdrv_io_plug = raw_aio_plug,
.bdrv_io_unplug = raw_aio_unplug,
+.bdrv_attach_aio_context = raw_aio_attach_aio_context,
.bdrv_co_truncate= raw_co_truncate,
.bdrv_getlength = raw_getlength,
@@ -3410,6 +3412,7 @@ static BlockDriver bdrv_host_cdrom = {
.bdrv_refresh_limits = raw_refresh_limits,
.bdrv_io_plug = raw_aio_plug,
.bdrv_io_unplug = raw_aio_unplug,
+.bdrv_attach_aio_context = raw_aio_attach_aio_context,
.bdrv_co_truncate= raw_co_truncate,
.bdrv_getlength = raw_getlength,
Re: [Qemu-devel] [PATCH] block/file-posix: add bdrv_attach_aio_context callback for host dev and cdrom
On 20.07.2018 [15:11:14 -0400], Farhan Ali wrote: > I am seeing another issue pop up, in a different test. Even though it's a > different assertion, it might be related based on the call trace. Just to be clear, this does not happen if you revert the original patch (i.e., the one you bisected to before)? I'm digging into the code now. -Nish
Re: [Qemu-devel] [PATCH] monitor: print message when using 'help' with an unknown command
On 07/20/2018 12:40 PM, Eric Blake wrote: > On 07/19/2018 11:39 AM, Collin Walling wrote: >> On 07/19/2018 12:31 PM, Markus Armbruster wrote: >>> You neglected to cc: maintainers. Cc'ing them increases the odds your >>> patch will be noticed and picked up. You can use >>> scripts/get_maintainer.pl to find maintainers. You don't have to do >>> anything for this patch; it got noticed anyway. >>> >>> David, this is yours :) >> >> Very true. Was a minor fix that I thought I'd just toss it out there and >> let anyone view it if they had the time. Will be more aware of who to CC >> next time around. > > Or automate it: > https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer > suggests: > > git config sendemail.cccmd 'scripts/get_maintainer.pl --nogit-fallback' > This is very helpful, thank you. -- Respectfully, - Collin Walling
[Qemu-devel] [PATCH v2] monitor: print message when using 'help' with an unknown command
When typing 'help' followed by an unknown command, QEMU will
not print anything to the command line to let the user know
they typed a bad command. Let's fix this by printing a message
to the monitor when this happens. For example:
(qemu) help xyz
unknown command: 'xyz'
Reported-by: Stefan Zimmermann
Signed-off-by: Collin Walling
---
monitor.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/monitor.c b/monitor.c
index 7af1f18..deeb41c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1013,6 +1013,7 @@ static void help_cmd_dump(Monitor *mon, const mon_cmd_t
*cmds,
char **args, int nb_args, int arg_index)
{
const mon_cmd_t *cmd;
+size_t i;
/* No valid arg need to compare with, dump all in *cmds */
if (arg_index >= nb_args) {
@@ -1034,9 +1035,15 @@ static void help_cmd_dump(Monitor *mon, const mon_cmd_t
*cmds,
} else {
help_cmd_dump_one(mon, cmd, args, arg_index);
}
-break;
+return;
}
}
+
+/* Command not found */
+monitor_printf(mon, "unknown command: '");
+for (i = 0; i <= arg_index; i++) {
+monitor_printf(mon, "%s%s", args[i], i == arg_index ? "'\n" : " ");
+}
}
static void help_cmd(Monitor *mon, const char *name)
--
2.7.4
Re: [Qemu-devel] [PATCH] block/file-posix: add bdrv_attach_aio_context callback for host dev and cdrom
On 07/20/2018 03:32 PM, Nishanth Aravamudan wrote: On 20.07.2018 [15:11:14 -0400], Farhan Ali wrote: I am seeing another issue pop up, in a different test. Even though it's a different assertion, it might be related based on the call trace. Just to be clear, this does not happen if you revert the original patch (i.e., the one you bisected to before)? I'm digging into the code now. -Nish I had not seen this issue before. I just ran my regression tests with your fix and saw the failure in one of my test. The patch in itself fixes the original issue I reported. I am going to try and debug some more. Thanks Farhan
Re: [Qemu-devel] issue about numa configure
Hi,
On Thu, Jul 19, 2018 at 09:21:36AM +, linzhecheng wrote:
> Hi, all
> I found that qemu has a constraint in function numa_node_parse now:
> If (node->has_memdev != have_memdevs) {
> Error_setg(errp, "qemu: memdev option must be specified for either "
> "all or no nodes");
> Return;
> }
> This restricts us from being able to configure an empty numa node (without
> memory and cpus). But if I delete these codes, I can start a VM with cmdline:
> qemu-system-x86_64 --enable-kvm -m size=2G,slots=256,maxmem=300G -smp
> 2,maxcpus=4,sockets=4,cores=1,threads=1 -numa node,nodeid=0,cpus=0-1,mem=2048
> -numa node,nodeid=1,cpus=2-3 ...
> We can see only one numa node inside the VM(I have tested both linux and
> windows) after beginning.
> And if I hot-plug the dimm memory devices into the empty node, vm will
> present a new numa node inside and the new memory is online then.
> I'm wondering if you have any related issue before? Or can we remove this
> constraint?
> Looking forward to your answers, thanks.
The check is there because memory_region_allocate_system_memory()
doesn't know how to allocate memory correctly if only some nodes
use memdev.
I wouldn't remove the check completely, but just skip it if
"mem=0" is specified explicitly.
--
Eduardo
Re: [Qemu-devel] [PATCH for-3.1 v2] python: Use io.StringIO
On Wed, Jul 18, 2018 at 07:36:28PM -0300, Philippe Mathieu-Daudé wrote:
> Both Python 2.7 and 3 support the same io.StringIO to
> handle unicode strings.
>
> Python 2.6 requires special care, but since 7f2b55443a his
> support was removed. Stop caring, drop the ImportError check.
>
> Use the common form to use indistinctly Python 2.7 or 3.
>
> http://python-future.org/compatible_idioms.html#stringio
>
> This fixes running tests on the Fedora Docker image,
> which uses Python3 since 356dc290f:
>
> $ make docker-test-block@fedora
> [...]
> 045 [failed, exit status 1] - output mismatch (see 045.out.bad)
> --- /tmp/qemu-test/src/tests/qemu-iotests/045.out 2018-07-17
> 16:56:18.0 +
> +++ /tmp/qemu-test/build/tests/qemu-iotests/045.out.bad 2018-07-17
> 17:19:22.448409007 +
> @@ -1,5 +1,6 @@
> -...
> ---
> -Ran 11 tests
> -
> -OK
> +Traceback (most recent call last):
> + File "045", line 178, in
> +iotests.main(supported_fmts=['raw'])
> + File "/tmp/qemu-test/src/tests/qemu-iotests/iotests.py", line 682, in
> main
> +import StringIO
> +ModuleNotFoundError: No module named 'StringIO'
> 132 [failed, exit status 1] - output mismatch (see 132.out.bad)
> 152 [failed, exit status 1] - output mismatch (see 152.out.bad)
>
> Failures: 045 132 152
>
> Suggested-by: Eduardo Habkost
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> tests/docker/docker.py| 5 +
> tests/image-fuzzer/runner.py | 6 +++---
> tests/qemu-iotests/iotests.py | 5 +++--
> 3 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index 69e7130db7..9d53b868db 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -26,10 +26,7 @@ import tempfile
> import re
> import signal
> from tarfile import TarFile, TarInfo
> -try:
> -from StringIO import StringIO
> -except ImportError:
> -from io import StringIO
> +from io import StringIO
This one should be BytesIO, see:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg545627.html
> from shutil import copy, rmtree
> from pwd import getpwuid
> from datetime import datetime,timedelta
> diff --git a/tests/image-fuzzer/runner.py b/tests/image-fuzzer/runner.py
> index 95d84f38f3..4462d84f45 100755
> --- a/tests/image-fuzzer/runner.py
> +++ b/tests/image-fuzzer/runner.py
> @@ -28,7 +28,7 @@ import shutil
> from itertools import count
> import time
> import getopt
> -import StringIO
> +from io import StringIO
> import resource
>
> try:
> @@ -183,7 +183,7 @@ class TestEnv(object):
> MAX_BACKING_FILE_SIZE) * (1 << 20)
> cmd = self.qemu_img + ['create', '-f', backing_file_fmt,
> backing_file_name, str(backing_file_size)]
> -temp_log = StringIO.StringIO()
> +temp_log = StringIO()
> retcode = run_app(temp_log, cmd)
I wouldn't touch this until we're sure if temp_log needs to be a
binary file or a text file.
> if retcode == 0:
> temp_log.close()
> @@ -240,7 +240,7 @@ class TestEnv(object):
> "Backing file: %s\n" \
> % (self.seed, " ".join(current_cmd),
>self.current_dir, backing_file_name)
> -temp_log = StringIO.StringIO()
> +temp_log = StringIO()
> try:
> retcode = run_app(temp_log, current_cmd)
> except OSError as e:
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 4e67fbbe96..c95dd17190 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -679,13 +679,14 @@ def main(supported_fmts=[], supported_oses=['linux'],
> supported_cache_modes=[],
>
> # We need to filter out the time taken from the output so that
> qemu-iotest
> # can reliably diff the results against master output.
> -import StringIO
> +from io import StringIO
> +
This one looks correct.
> if debug:
> output = sys.stdout
> verbosity = 2
> sys.argv.remove('-d')
> else:
> -output = StringIO.StringIO()
> +output = StringIO()
>
> logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
>
> --
> 2.18.0
>
--
Eduardo
Re: [Qemu-devel] [PATCH v3 01/11] block/nbd-client: split channel errors from export errors
On 06/09/2018 10:32 AM, Vladimir Sementsov-Ogievskiy wrote: To implement nbd reconnect in further patches, we need to distinguish error codes, returned by nbd server, from channel errors, to reconnect only in the latter case. Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/nbd-client.c | 83 +++--- 1 file changed, 47 insertions(+), 36 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] block: Fix copy-on-read crash with partial final cluster
On 07/09/2018 04:32 AM, Kevin Wolf wrote: Am 06.07.2018 um 23:20 hat Eric Blake geschrieben: On 07/06/2018 11:45 AM, Kevin Wolf wrote: If the virtual disk size isn't aligned to full clusters, bdrv_co_do_copy_on_readv() may get pnum == 0 before having the full cluster completed, which will let it run into an assertion failure: qemu-io: block/io.c:1203: bdrv_co_do_copy_on_readv: Assertion `skip_bytes < pnum' failed. Check for EOF, assert that we read at least as much as the read request originally wanted to have (which is true at EOF because otherwise bdrv_check_byte_request() would already have returned an error) and return success early even though we couldn't copy the full cluster. +echo +echo '=== Partial final cluster ===' +echo + +_make_test_img 1024 +$QEMU_IO -f $IMGFMT -C -c 'read 0 1024' "$TEST_IMG" | _filter_qemu_io Here, $TEST_IMG has no backing file, and does not have the final cluster allocated; all we have to do is properly read all zeroes. And write them back, we test that it's allocated afterwards. Is it worth also explicitly testing reading of allocated data under COR I could add a second read of the same area, which would not only test reading of allocated data, but also test whether the allocating COR wrote the correct data (all zeros). Do you think that would be a worthwhile addition? It can't hurt (we've already proven that our iotests coverage is incomplete, and anything we can do to enhance it improves chances of it catching unintentional regressions). and/or the case of one image with another backing image (with same or differing partial cluster sizes), where COR actually has to write the partial cluster? However, the logic in the code appears to cover all of those, whether or not the testsuite does as well. Having a backing file is a different code path for non-zero data, but I think we already test normal write/write_zeroes extensively. For zero data, it doesn't make a difference whether it comes from the backing file or from not having a backing file (as far as the COR logic is concerned, anyway). I didn't want to use a backing file because the test is 'generic', but it already uses qcow2 in other cases, so if we really think this is important to have, I guess I can have another case with qcow2 over $IMGFMT and non-zero data. At this point, I'm not volunteering to write such test enhancements, and I don't think it's a show-stopper if 3.0 uses the test as currently in git. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 2/2] nbd/server: send more than one extent of base:allocation context
On 07/10/2018 09:33 AM, Vladimir Sementsov-Ogievskiy wrote:
04.07.2018 14:23, Vladimir Sementsov-Ogievskiy wrote:
This is necessary for efficient block-status export, for clients which
support it.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
+static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
+ uint64_t *bytes, NBDExtent *extents,
+ unsigned int *nb_extents)
{
- uint64_t remaining_bytes = bytes;
+ uint64_t remaining_bytes = *bytes;
+ NBDExtent *extent = extents, *extents_end = extents + *nb_extents;
+ bool first_extent = true;
+ assert(*nb_extents);
while (remaining_bytes) {
uint32_t flags;
int64_t num;
@@ -1860,21 +1871,40 @@ static int
blockstatus_to_extent_be(BlockDriverState *bs, uint64_t offset,
after bdrv_block_status_above, is there a guarantee that num > 0? Should
we add an assertion?
bdrv_block_status_above() asserts that drivers set *pnum to non-zero on
success (that is, a driver's .bdrv_co_block_status must make progress or
fail); however, a caller can still get *pnum == 0 on the corner case of
requesting status at or beyond the end-of-file boundary (where
bdrv_block_status_above() does not call into a driver's
.bdrv_co_block_status callback). Since the NBD code already validated
that the client's code does not exceed EOF, and the block layer
guaranteed that an in-range request made progress on success, you should
be fine with an assertion that num > 0.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [Qemu-block] [PATCH] block: Fix typos in comments (found by codespell)
On 07/12/2018 03:32 PM, John Snow wrote: +++ b/block/gluster.c @@ -1326,7 +1326,7 @@ static int qemu_gluster_has_zero_init(BlockDriverState *bs) * If @start is in a trailing hole or beyond EOF, return -ENXIO. * If we can't find out, return a negative errno other than -ENXIO. * - * (Shamefully copied from file-posix.c, only miniscule adaptions.) + * (Shamefully copied from file-posix.c, only minuscule adaptions.) Huh! Merriam-Webster lists miniscule as a "disputed" spelling of minuscule. Dictionary.com simply lists it as an acceptable alternative. Anyway, this is a change from something dubiously correct to something unambiguously correct, so it's fine. While at it, I would s/adaptions/adaptations/ -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 2/3] qemu-img: Use zero writes after source backing EOF
On 07/13/2018 06:14 AM, Max Reitz wrote:
Past the end of the source backing file, we memset() buf_old to zero, so
it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
then.
Signed-off-by: Max Reitz
---
qemu-img.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index dd684d8bf0..2552e7dad6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3403,6 +3403,8 @@ static int img_rebase(int argc, char **argv)
}
for (offset = 0; offset < size; offset += n) {
+bool buf_old_is_zero = false;
+
/* How many bytes can we handle with the next read? */
n = MIN(IO_BUF_SIZE, size - offset);
@@ -3423,6 +3425,7 @@ static int img_rebase(int argc, char **argv)
*/
if (offset >= old_backing_size) {
memset(buf_old, 0, n);
+buf_old_is_zero = true;
Do we still need to spend time on the memset(), or...
} else {
if (offset + n > old_backing_size) {
n = old_backing_size - offset;
@@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv)
if (compare_buffers(buf_old + written, buf_new + written,
n - written, &pnum))
{
-ret = blk_pwrite(blk, offset + written,
- buf_old + written, pnum, 0);
+if (buf_old_is_zero) {
+ret = blk_pwrite_zeroes(blk, offset + written, pnum,
0);
...are we able to guarantee that old_buf will not be used when
buf_old_is_zero?
+} else {
+ret = blk_pwrite(blk, offset + written,
+ buf_old + written, pnum, 0);
+}
if (ret < 0) {
error_report("Error while writing to COW image: %s",
strerror(-ret));
The series seems reasonable, but is post-3.0 material, so I haven't
reviewed it any closer than this question.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[Qemu-devel] [PATCH v2 for-3.0] po: Don't include comments with location
Those comments change often when ui/gtk.c is changed and are not really useful. Signed-off-by: Stefan Weil --- CC'ing all translators because of the new string which still needs translations. v2: Only automatically created content, no new German translation. Regards Stefan po/Makefile| 2 +- po/bg.po | 23 --- po/de_DE.po| 23 --- po/fr_FR.po| 23 --- po/hu.po | 23 --- po/it.po | 23 --- po/messages.po | 25 + po/tr.po | 23 --- po/zh_CN.po| 23 --- 9 files changed, 34 insertions(+), 154 deletions(-) diff --git a/po/Makefile b/po/Makefile index cc630363de..e47e262ee6 100644 --- a/po/Makefile +++ b/po/Makefile @@ -43,7 +43,7 @@ install: $(OBJS) $(PO_PATH)/messages.po: $(SRC_PATH)/ui/gtk.c $(call quiet-command, ( cd $(SRC_PATH) && \ - xgettext -o - --from-code=UTF-8 --foreign-user \ + xgettext -o - --from-code=UTF-8 --foreign-user --no-location \ --package-name=QEMU --package-version=$(VERSION) \ --msgid-bugs-address=qemu-devel@nongnu.org -k_ -C ui/gtk.c | \ sed -e s/CHARSET/UTF-8/) >$@,"GEN","$@") diff --git a/po/bg.po b/po/bg.po index 279d1b864a..3d8c353372 100644 --- a/po/bg.po +++ b/po/bg.po @@ -7,7 +7,7 @@ msgid "" msgstr "" "Project-Id-Version: QEMU 2.6.50\n" "Report-Msgid-Bugs-To: qemu-devel@nongnu.org\n" -"POT-Creation-Date: 2016-12-13 21:46+\n" +"POT-Creation-Date: 2018-07-18 07:56+0200\n" "PO-Revision-Date: 2016-06-09 15:54+0300\n" "Last-Translator: Alexander Shopov \n" "Language-Team: Bulgarian \n" @@ -17,74 +17,59 @@ msgstr "" "Content-Transfer-Encoding: 8bit\n" "Plural-Forms: nplurals=2; plural=(n != 1);\n" -#: ui/gtk.c:275 msgid " - Press Ctrl+Alt+G to release grab" msgstr " — натиснете Ctrl+Alt+G, за да освободите фокуса" -#: ui/gtk.c:279 msgid " [Paused]" msgstr " [пауза]" -#: ui/gtk.c:1922 msgid "_Pause" msgstr "_Пауза" -#: ui/gtk.c:1928 msgid "_Reset" msgstr "_Рестартиране" -#: ui/gtk.c:1931 msgid "Power _Down" msgstr "_Изключване" -#: ui/gtk.c:1937 msgid "_Quit" msgstr "_Спиране на програмата" -#: ui/gtk.c:2029 msgid "_Fullscreen" msgstr "На _цял екран" -#: ui/gtk.c:2032 msgid "_Copy" msgstr "_Копиране" -#: ui/gtk.c:2048 msgid "Zoom _In" msgstr "_Увеличаване" -#: ui/gtk.c:2055 msgid "Zoom _Out" msgstr "_Намаляване" -#: ui/gtk.c:2062 msgid "Best _Fit" msgstr "По_местване" -#: ui/gtk.c:2069 msgid "Zoom To _Fit" msgstr "Напас_ване" -#: ui/gtk.c:2075 msgid "Grab On _Hover" msgstr "Прихващане при посо_чване" -#: ui/gtk.c:2078 msgid "_Grab Input" msgstr "Прихващане на _фокуса" -#: ui/gtk.c:2107 msgid "Show _Tabs" msgstr "Подпро_зорци" -#: ui/gtk.c:2110 msgid "Detach Tab" msgstr "Към самостоятелен подпрозорец" -#: ui/gtk.c:2122 +msgid "Show Menubar" +msgstr "" + msgid "_Machine" msgstr "_Машина" -#: ui/gtk.c:2127 msgid "_View" msgstr "_Изглед" diff --git a/po/de_DE.po b/po/de_DE.po index de27fcf174..1411dc52f4 100644 --- a/po/de_DE.po +++ b/po/de_DE.po @@ -6,7 +6,7 @@ msgid "" msgstr "" "Project-Id-Version: QEMU 1.4.50\n" "Report-Msgid-Bugs-To: qemu-devel@nongnu.org\n" -"POT-Creation-Date: 2016-12-13 21:46+\n" +"POT-Creation-Date: 2018-07-18 07:56+0200\n" "PO-Revision-Date: 2012-02-28 16:00+0100\n" "Last-Translator: Kevin Wolf \n" "Language-Team: Deutsch \n" @@ -16,74 +16,59 @@ msgstr "" "Content-Transfer-Encoding: 8bit\n" "Plural-Forms: nplurals=2; plural=(n!=1);\n" -#: ui/gtk.c:275 msgid " - Press Ctrl+Alt+G to release grab" msgstr " - Strg+Alt+G drücken, um Eingabegeräte freizugeben" -#: ui/gtk.c:279 msgid " [Paused]" msgstr " [Angehalten]" -#: ui/gtk.c:1922 msgid "_Pause" msgstr "_Angehalten" -#: ui/gtk.c:1928 msgid "_Reset" msgstr "_Reset" -#: ui/gtk.c:1931 msgid "Power _Down" msgstr "_Herunterfahren" -#: ui/gtk.c:1937 msgid "_Quit" msgstr "_Beenden" -#: ui/gtk.c:2029 msgid "_Fullscreen" msgstr "_Vollbild" -#: ui/gtk.c:2032 msgid "_Copy" msgstr "_Kopieren" -#: ui/gtk.c:2048 msgid "Zoom _In" msgstr "_Heranzoomen" -#: ui/gtk.c:2055 msgid "Zoom _Out" msgstr "_Wegzoomen" -#: ui/gtk.c:2062 msgid "Best _Fit" msgstr "_Einpassen" -#: ui/gtk.c:2069 msgid "Zoom To _Fit" msgstr "Auf _Fenstergröße skalieren" -#: ui/gtk.c:2075 msgid "Grab On _Hover" msgstr "Tastatur _automatisch einfangen" -#: ui/gtk.c:2078 msgid "_Grab Input" msgstr "_Eingabegeräte einfangen" -#: ui/gtk.c:2107 msgid "Show _Tabs" msgstr "Reiter anzeigen" -#: ui/gtk.c:2110 msgid "Detach Tab" msgstr "Reiter abtrennen" -#: ui/gtk.c:2122 +msgid "Show Menubar" +msgstr "" + msgid "_Machine" msgstr "_Maschine" -#: ui/gtk.c:2127 msgid "_View" msgstr "_Ansicht" diff --git a/po/fr_FR.po b/po/fr_FR.po index 94f4a94f5c..25ad4c954a 100644 --- a/po/fr_FR.po +++ b/po/fr_FR.po @@ -6,7 +6,7 @@ msgid "" msgstr "" "Pr
[Qemu-devel] [PATCH 1/5] dev-mtp: add support for canceling transaction
The initiator can choose to cancel an ongoing request which
is specified by bRequest=0x64. If such a request arrives,
free up any pending state
Signed-off-by: Bandan Das
---
hw/usb/dev-mtp.c | 30 ++
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 1ded7ac9a3..c40b0de0bb 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -82,6 +82,7 @@ enum mtp_code {
FMT_ASSOCIATION= 0x3001,
/* event codes */
+EVT_CANCEL_TRANSACTION = 0x4001,
EVT_OBJ_ADDED = 0x4002,
EVT_OBJ_REMOVED= 0x4003,
EVT_OBJ_INFO_CHANGED = 0x4007,
@@ -1551,14 +1552,35 @@ static void usb_mtp_handle_control(USBDevice *dev,
USBPacket *p,
int length, uint8_t *data)
{
int ret;
+MTPState *s = USB_MTP(dev);
+uint16_t *event = (uint16_t *)data;
-ret = usb_desc_handle_control(dev, p, request, value, index, length, data);
-if (ret >= 0) {
-return;
+switch (request) {
+case ClassInterfaceOutRequest | 0x64:
+if (*event == EVT_CANCEL_TRANSACTION) {
+g_free(s->result);
+s->result = NULL;
+usb_mtp_data_free(s->data_in);
+s->data_in = NULL;
+if (s->write_pending) {
+g_free(s->dataset.filename);
+s->write_pending = false;
+}
+usb_mtp_data_free(s->data_out);
+s->data_out = NULL;
+} else {
+p->status = USB_RET_STALL;
+}
+break;
+default:
+ret = usb_desc_handle_control(dev, p, request,
+ value, index, length, data);
+if (ret >= 0) {
+return;
+}
}
trace_usb_mtp_stall(dev->addr, "unknown control request");
-p->status = USB_RET_STALL;
}
static void usb_mtp_cancel_packet(USBDevice *dev, USBPacket *p)
--
2.14.4
[Qemu-devel] [PATCH 4/5] dev-mtp: Add support for > 4GB file transfers
To support larger file transfers, rely on a short packet
to detect end of the data phase and rewrite d->length to
the size received
Signed-off-by: Bandan Das
---
hw/usb/dev-mtp.c | 31 +++
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index c8f6eb4e9e..2e3ea58da6 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -147,9 +147,12 @@ struct MTPData {
uint32_t trans;
uint64_t offset;
uint64_t length;
-uint32_t alloc;
+uint64_t alloc;
uint8_t *data;
bool first;
+/* Used for >4G file sizes */
+bool pending;
+uint64_t cached_length;
int fd;
};
@@ -1626,7 +1629,7 @@ static void usb_mtp_write_data(MTPState *s)
MTPObject *parent =
usb_mtp_object_lookup(s, s->dataset.parent_handle);
char *path = NULL;
-int rc = -1;
+uint64_t rc;
mode_t mask = 0644;
assert(d != NULL);
@@ -1643,7 +1646,7 @@ static void usb_mtp_write_data(MTPState *s)
d->fd = mkdir(path, mask);
goto free;
}
-if (s->dataset.size < d->length) {
+if ((s->dataset.size != 0x) && (s->dataset.size < d->length)) {
usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
0, 0, 0, 0);
goto done;
@@ -1754,13 +1757,27 @@ static void usb_mtp_get_data(MTPState *s, mtp_container
*container,
usb_mtp_realloc(d, total_len);
d->length += total_len;
d->offset = 0;
+d->cached_length = total_len;
d->first = false;
+d->pending = false;
+}
+
+if (d->pending) {
+usb_mtp_realloc(d, d->cached_length);
+d->length += d->cached_length;
+d->pending = false;
}
if (d->length - d->offset > data_len) {
dlen = data_len;
} else {
dlen = d->length - d->offset;
+/* Check for cached data for large files */
+if ((s->dataset.size == 0x) && (dlen < p->iov.size)) {
+usb_mtp_realloc(d, p->iov.size - dlen);
+d->length += p->iov.size - dlen;
+dlen = p->iov.size;
+}
}
switch (d->code) {
@@ -1780,12 +1797,18 @@ static void usb_mtp_get_data(MTPState *s, mtp_container
*container,
case CMD_SEND_OBJECT:
usb_packet_copy(p, d->data + d->offset, dlen);
d->offset += dlen;
-if (d->offset == d->length) {
+if ((p->iov.size % 64) || !p->iov.size) {
+assert((s->dataset.size == 0x) ||
+ (s->dataset.size == d->length));
+
usb_mtp_write_data(s);
usb_mtp_data_free(s->data_out);
s->data_out = NULL;
return;
}
+if (d->offset == d->length) {
+d->pending = true;
+}
break;
default:
p->status = USB_RET_STALL;
--
2.14.4
[Qemu-devel] [PATCH 3/5] dev-mtp: retry write for incomplete transfers
For large buffers, write may not copy the full buffer. For example,
on Linux, write imposes a limit of 0x7000. Note that this does
not fix >4G transfers but ~>2G files will transfer successfully.
Signed-off-by: Bandan Das
---
hw/usb/dev-mtp.c | 22 --
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 1b72603dc5..c8f6eb4e9e 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1602,6 +1602,24 @@ static void utf16_to_str(uint8_t len, uint16_t *arr,
char *name)
g_free(wstr);
}
+/* Wrapper around write, returns 0 on failure */
+static uint64_t write_retry(int fd, void *buf, uint64_t size)
+{
+uint64_t bytes_left = size, ret;
+
+while (bytes_left > 0) {
+ret = write(fd, buf, bytes_left);
+if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
+errno != EWOULDBLOCK)) {
+break;
+}
+bytes_left -= ret;
+buf += ret;
+}
+
+return size - bytes_left;
+}
+
static void usb_mtp_write_data(MTPState *s)
{
MTPData *d = s->data_out;
@@ -1644,8 +1662,8 @@ static void usb_mtp_write_data(MTPState *s)
goto success;
}
-rc = write(d->fd, d->data, s->dataset.size);
-if (rc == -1) {
+rc = write_retry(d->fd, d->data, s->dataset.size);
+if (!rc) {
usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
0, 0, 0, 0);
goto done;
--
2.14.4
[Qemu-devel] [PATCH 0/5] usb-mtp write fixes
Patch 1 adds support for canceling an ongoing transaction. 2,3 and 4 fix writes for large transfers. For > 4G file transfers, the logic has been modified to check for the end of the data phase by checking for a short packet. Patch 5 renames x-root to a more meaningful rootdir. Bandan Das (5): dev-mtp: add support for canceling transaction dev-mtp: fix buffer allocation for writing file contents dev-mtp: retry write for incomplete transfers dev-mtp: Add support for > 4GB file transfers dev-mtp: rename x-root to rootdir hw/usb/dev-mtp.c | 93 +++- 1 file changed, 79 insertions(+), 14 deletions(-) -- 2.14.4
[Qemu-devel] [PATCH 2/5] dev-mtp: fix buffer allocation for writing file contents
usb_mtp_realloc() was being incorrectly used when allocating
buffer for incoming data. Set d->length only after resizing
the buffer.
Signed-off-by: Bandan Das
---
hw/usb/dev-mtp.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index c40b0de0bb..1b72603dc5 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1721,6 +1721,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container
*container,
MTPData *d = s->data_out;
uint64_t dlen;
uint32_t data_len = p->iov.size;
+uint64_t total_len;
if (!d) {
usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, 0,
@@ -1729,10 +1730,11 @@ static void usb_mtp_get_data(MTPState *s, mtp_container
*container,
}
if (d->first) {
/* Total length of incoming data */
-d->length = cpu_to_le32(container->length) - sizeof(mtp_container);
+total_len = cpu_to_le32(container->length) - sizeof(mtp_container);
/* Length of data in this packet */
data_len -= sizeof(mtp_container);
-usb_mtp_realloc(d, d->length);
+usb_mtp_realloc(d, total_len);
+d->length += total_len;
d->offset = 0;
d->first = false;
}
--
2.14.4
[Qemu-devel] [PATCH 5/5] dev-mtp: rename x-root to rootdir
x-root was renamed as such owing to the experimental nature of the
property; the underlying filesystem semantics were undecided
Signed-off-by: Bandan Das
---
hw/usb/dev-mtp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 2e3ea58da6..3fdc4b0da1 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -2018,7 +2018,7 @@ static void usb_mtp_realize(USBDevice *dev, Error **errp)
QTAILQ_INIT(&s->objects);
if (s->desc == NULL) {
if (s->root == NULL) {
-error_setg(errp, "usb-mtp: x-root property must be configured");
+error_setg(errp, "usb-mtp: rootdir property must be configured");
return;
}
s->desc = strrchr(s->root, '/');
@@ -2047,7 +2047,7 @@ static const VMStateDescription vmstate_usb_mtp = {
};
static Property mtp_properties[] = {
-DEFINE_PROP_STRING("x-root", MTPState, root),
+DEFINE_PROP_STRING("rootdir", MTPState, root),
DEFINE_PROP_STRING("desc", MTPState, desc),
DEFINE_PROP_BOOL("readonly", MTPState, readonly, true),
DEFINE_PROP_END_OF_LIST(),
--
2.14.4
Re: [Qemu-devel] [Qemu-trivial] [PATCH v2 for-3.0] po: Don't include comments with location
On 07/20/2018 06:25 PM, Stefan Weil wrote: > Those comments change often when ui/gtk.c is changed and are not > really useful. > > Signed-off-by: Stefan Weil Thanks for splitting this. Reviewed-by: Philippe Mathieu-Daudé > --- > > CC'ing all translators because of the new string which still needs > translations. > > v2: Only automatically created content, no new German translation. > > Regards > Stefan > > po/Makefile| 2 +- > po/bg.po | 23 --- > po/de_DE.po| 23 --- > po/fr_FR.po| 23 --- > po/hu.po | 23 --- > po/it.po | 23 --- > po/messages.po | 25 + > po/tr.po | 23 --- > po/zh_CN.po| 23 --- > 9 files changed, 34 insertions(+), 154 deletions(-) > > diff --git a/po/Makefile b/po/Makefile > index cc630363de..e47e262ee6 100644 > --- a/po/Makefile > +++ b/po/Makefile > @@ -43,7 +43,7 @@ install: $(OBJS) > > $(PO_PATH)/messages.po: $(SRC_PATH)/ui/gtk.c > $(call quiet-command, ( cd $(SRC_PATH) && \ > - xgettext -o - --from-code=UTF-8 --foreign-user \ > + xgettext -o - --from-code=UTF-8 --foreign-user --no-location \ > --package-name=QEMU --package-version=$(VERSION) \ > --msgid-bugs-address=qemu-devel@nongnu.org -k_ -C ui/gtk.c | \ > sed -e s/CHARSET/UTF-8/) >$@,"GEN","$@") > diff --git a/po/bg.po b/po/bg.po > index 279d1b864a..3d8c353372 100644 > --- a/po/bg.po > +++ b/po/bg.po > @@ -7,7 +7,7 @@ msgid "" > msgstr "" > "Project-Id-Version: QEMU 2.6.50\n" > "Report-Msgid-Bugs-To: qemu-devel@nongnu.org\n" > -"POT-Creation-Date: 2016-12-13 21:46+\n" > +"POT-Creation-Date: 2018-07-18 07:56+0200\n" > "PO-Revision-Date: 2016-06-09 15:54+0300\n" > "Last-Translator: Alexander Shopov \n" > "Language-Team: Bulgarian \n" > @@ -17,74 +17,59 @@ msgstr "" > "Content-Transfer-Encoding: 8bit\n" > "Plural-Forms: nplurals=2; plural=(n != 1);\n" > > -#: ui/gtk.c:275 > msgid " - Press Ctrl+Alt+G to release grab" > msgstr " — натиснете Ctrl+Alt+G, за да освободите фокуса" > > -#: ui/gtk.c:279 > msgid " [Paused]" > msgstr " [пауза]" > > -#: ui/gtk.c:1922 > msgid "_Pause" > msgstr "_Пауза" > > -#: ui/gtk.c:1928 > msgid "_Reset" > msgstr "_Рестартиране" > > -#: ui/gtk.c:1931 > msgid "Power _Down" > msgstr "_Изключване" > > -#: ui/gtk.c:1937 > msgid "_Quit" > msgstr "_Спиране на програмата" > > -#: ui/gtk.c:2029 > msgid "_Fullscreen" > msgstr "На _цял екран" > > -#: ui/gtk.c:2032 > msgid "_Copy" > msgstr "_Копиране" > > -#: ui/gtk.c:2048 > msgid "Zoom _In" > msgstr "_Увеличаване" > > -#: ui/gtk.c:2055 > msgid "Zoom _Out" > msgstr "_Намаляване" > > -#: ui/gtk.c:2062 > msgid "Best _Fit" > msgstr "По_местване" > > -#: ui/gtk.c:2069 > msgid "Zoom To _Fit" > msgstr "Напас_ване" > > -#: ui/gtk.c:2075 > msgid "Grab On _Hover" > msgstr "Прихващане при посо_чване" > > -#: ui/gtk.c:2078 > msgid "_Grab Input" > msgstr "Прихващане на _фокуса" > > -#: ui/gtk.c:2107 > msgid "Show _Tabs" > msgstr "Подпро_зорци" > > -#: ui/gtk.c:2110 > msgid "Detach Tab" > msgstr "Към самостоятелен подпрозорец" > > -#: ui/gtk.c:2122 > +msgid "Show Menubar" > +msgstr "" > + > msgid "_Machine" > msgstr "_Машина" > > -#: ui/gtk.c:2127 > msgid "_View" > msgstr "_Изглед" > diff --git a/po/de_DE.po b/po/de_DE.po > index de27fcf174..1411dc52f4 100644 > --- a/po/de_DE.po > +++ b/po/de_DE.po > @@ -6,7 +6,7 @@ msgid "" > msgstr "" > "Project-Id-Version: QEMU 1.4.50\n" > "Report-Msgid-Bugs-To: qemu-devel@nongnu.org\n" > -"POT-Creation-Date: 2016-12-13 21:46+\n" > +"POT-Creation-Date: 2018-07-18 07:56+0200\n" > "PO-Revision-Date: 2012-02-28 16:00+0100\n" > "Last-Translator: Kevin Wolf \n" > "Language-Team: Deutsch \n" > @@ -16,74 +16,59 @@ msgstr "" > "Content-Transfer-Encoding: 8bit\n" > "Plural-Forms: nplurals=2; plural=(n!=1);\n" > > -#: ui/gtk.c:275 > msgid " - Press Ctrl+Alt+G to release grab" > msgstr " - Strg+Alt+G drücken, um Eingabegeräte freizugeben" > > -#: ui/gtk.c:279 > msgid " [Paused]" > msgstr " [Angehalten]" > > -#: ui/gtk.c:1922 > msgid "_Pause" > msgstr "_Angehalten" > > -#: ui/gtk.c:1928 > msgid "_Reset" > msgstr "_Reset" > > -#: ui/gtk.c:1931 > msgid "Power _Down" > msgstr "_Herunterfahren" > > -#: ui/gtk.c:1937 > msgid "_Quit" > msgstr "_Beenden" > > -#: ui/gtk.c:2029 > msgid "_Fullscreen" > msgstr "_Vollbild" > > -#: ui/gtk.c:2032 > msgid "_Copy" > msgstr "_Kopieren" > > -#: ui/gtk.c:2048 > msgid "Zoom _In" > msgstr "_Heranzoomen" > > -#: ui/gtk.c:2055 > msgid "Zoom _Out" > msgstr "_Wegzoomen" > > -#: ui/gtk.c:2062 > msgid "Best _Fit" > msgstr "_Einpassen" > > -#: ui/gtk.c:2069 > msgid "Zoom To _Fit" > msgstr "Auf _Fenstergröße skalieren" > > -#: ui/gtk.c:2075 > msgid "Grab On _Hover" > msgstr "Tastatur _automatisch einfangen" > > -#: u
[Qemu-devel] [PATCH 4/6] dirty-bitmaps: clean-up bitmaps loading and migration logic
On 06/26/2018 09:50 AM, Vladimir Sementsov-Ogievskiy wrote: > This patch aims to bring the following behavior: Just as a primer for anyone else reading this email (nobody) who might not understand the terminology (less than nobody), it might be helpful to remember that: -INACTIVATE occurs either as a starting state for a QEMU awaiting an incoming migration, or as a finishing state for a QEMU that has handed off control of its disks to a target QEMU during a migration. Despite being questionable grammar, it largely makes sense. -INVALIDATE occurs only on inactive images (it is a NOP on active images); and is used to obliterate any cache/state that may exist from a prior open call. This also removes the BDRV_O_INACTIVE flag, and therefore also "activates" an image. It is called in two cases, AFAICT: (1) To engage an image after a shared storage migration, by the destination QEMU (2) To re-engage a previously inactivated image after a failed migration, by the source QEMU And for those of you who already knew all of this, this is a chance for you to correct me if I was wrong. As further recap, bitmaps can be migrated generally in two ways: (1) For persistent bitmaps only: as part of a shared storage migration, they can be flushed to disk before the handoff. This does not require any special migration capabilities. (2) For either shared or non-shared storage migrations, bitmaps regardless of their persistence can be migrated using migration/block-dirty-bitmap.c. This feature has to be opted into. > > 1. We don't load bitmaps, when started in inactive mode. It's the case > of incoming migration. In this case we wait for bitmaps migration > through migration channel (if 'dirty-bitmaps' capability is enabled) or > for invalidation (to load bitmaps from the image). > OK, so performing qcow2_open while (flags & BDRV_O_INACTIVE) -- when we're awaiting an incoming migration, generally -- will forego attempting to load ANY bitmaps, under the pretense that: (1) We will need to have re-loaded them on invalidate anyway, or (2) We will be receiving them through the postcopy migration mechanisms. This sounds correct to me; they won't get set as IN_USE on disk and if anyone tries to query or address them prior to handoff, they won't exist, so they can't be misused, either. > 2. We don't remove persistent bitmaps on inactivation. Instead, we only > remove bitmaps after storing. This is the only way to restore bitmaps, > if we decided to resume source after [failed] migration with > 'dirty-bitmaps' capability enabled (which means, that bitmaps were not > stored). > So currently, when we inactivate we remove the in-memory bitmaps that we considered to be associated with the bitmap -- ONLY for persistent ones. bdrv_release_persistent_dirty_bitmaps() managed this. However, the current hook for this in bdrv_inactivate_recurse is a bit of a hack -- it just muses that the bitmaps "should already be stored by the format driver" -- and it's correct, they SHOULD be, but we can just move the hook to precisely when we store the bitmap. This is indeed more resilient. For any bitmaps theoretically left behind in this state, we can rest assured that we cannot write to an INACTIVE node anyway, so they won't be able to change on us. Either they'll get erased on close, or we'll re-use them on INVALIDATE. So now, in the shared storage + no-bitmap-migrate case: - We flush the bitmaps to disk anyway. The bitmaps are removed on-store. If we need to INVALIDATE and become active again, we just re-read them from disk. Any bitmaps we had that were NOT persistent never got removed, so we are fine. And in the migrate-bitmap case: You opt not to allow them to be flushed to disk, which means that not deleting them is definitely mandatory, in case of failure. The change that correlates to this bullet-point (delete only on store) is good regardless, but as of right now I'm confused as to why we can't flush bitmaps we want to transmit across the live migration channel to disk anyway. I guess you want to avoid a double-load in the case where we do a shared storage migration and a bitmap migration? The problem I have here is that this means that we simply ignore flushing to disk, so the bitmap remains IN_USE even when it isn't truly IN_USE, and that the method of coping with this means *ignoring* bitmaps that are IN_USE instead of erroring out and failing to load. I think that's dangerous, unless I'm missing something -- I want QEMU to *error* if it sees an IN_USE bitmap. I think it ought to, as it's not safe to modify only some of these bitmaps. I think it's very strange to NOT flush bitmaps to disk on INACTIVATE, and I think we MUST do so. > 3. We load bitmaps on open and any invalidation, it's ok for all cases: > - normal open "Normal" open in the !(BDRV_O_INACTIVE) sense, yes. > - migration target invalidation with dirty-bitmaps capability > (bitmaps are migrating through migration channel, the are not > sto
Re: [Qemu-devel] [Qemu-block] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
On Sat, Jul 21, 2018 at 12:35 AM Eric Blake wrote: > > On 07/20/2018 03:24 AM, Fam Zheng wrote: > I'm not familiar with /dev/nullb* - is that a typo? > > $ ll /dev/nullb* > ls: cannot access '/dev/nullb*': No such file or directory You probably have figured out already but just in case: it's kernel null_blk mod. Fam
[Qemu-devel] [PATCH] qga-win: add support for qmp_guest_fsfreeze_freeze_list
From: Chen Hanxiao
This patch add support for freeze specified fs.
The valid mountpoints list member are [1]:
The path of a mounted folder, for example, Y:\MountX\
A drive letter, for example, D:\
A volume GUID path of the form \\?\Volume{GUID}\,
where GUID identifies the volume
A UNC path that specifies a remote file share,
for example, \\Clusterx\Share1\
[1]
https://docs.microsoft.com/en-us/windows/desktop/api/vsbackup/nf-vsbackup-ivssbackupcomponents-addtosnapshotset
Cc: Michael Roth
Signed-off-by: Chen Hanxiao
---
qga/commands-win32.c| 21 ++
qga/main.c | 2 +-
qga/vss-win32.c | 5 ++--
qga/vss-win32.h | 3 +-
qga/vss-win32/requester.cpp | 56 +++--
qga/vss-win32/requester.h | 13 +++--
6 files changed, 72 insertions(+), 28 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 318d760a74..246f426999 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -776,6 +776,13 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
* The frozen state is limited for up to 10 seconds by VSS.
*/
int64_t qmp_guest_fsfreeze_freeze(Error **errp)
+{
+return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
+}
+
+int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
+ strList *mountpoints,
+ Error **errp)
{
int i;
Error *local_err = NULL;
@@ -790,7 +797,7 @@ int64_t qmp_guest_fsfreeze_freeze(Error **errp)
/* cannot risk guest agent blocking itself on a write in this state */
ga_set_frozen(ga_state);
-qga_vss_fsfreeze(&i, true, &local_err);
+qga_vss_fsfreeze(&i, true, mountpoints, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto error;
@@ -808,15 +815,6 @@ error:
return 0;
}
-int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
- strList *mountpoints,
- Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-
-return 0;
-}
-
/*
* Thaw local file systems using Volume Shadow-copy Service.
*/
@@ -829,7 +827,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
return 0;
}
-qga_vss_fsfreeze(&i, false, errp);
+qga_vss_fsfreeze(&i, false, NULL, errp);
ga_unset_frozen(ga_state);
return i;
@@ -1633,7 +1631,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
"guest-set-vcpus",
"guest-get-memory-blocks", "guest-set-memory-blocks",
"guest-get-memory-block-size",
-"guest-fsfreeze-freeze-list",
NULL};
char **p = (char **)list_unsupported;
diff --git a/qga/main.c b/qga/main.c
index 537cc0e162..2575ea206a 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -152,7 +152,7 @@ static void quit_handler(int sig)
WaitForSingleObject(hEventTimeout, 0);
CloseHandle(hEventTimeout);
}
-qga_vss_fsfreeze(&i, false, &err);
+qga_vss_fsfreeze(&i, false, NULL, &err);
if (err) {
g_debug("Error unfreezing filesystems prior to exiting: %s",
error_get_pretty(err));
diff --git a/qga/vss-win32.c b/qga/vss-win32.c
index a541f3ae01..f444a25a70 100644
--- a/qga/vss-win32.c
+++ b/qga/vss-win32.c
@@ -147,7 +147,8 @@ void ga_uninstall_vss_provider(void)
}
/* Call VSS requester and freeze/thaw filesystems and applications */
-void qga_vss_fsfreeze(int *nr_volume, bool freeze, Error **errp)
+void qga_vss_fsfreeze(int *nr_volume, bool freeze,
+ strList *mountpoints, Error **errp)
{
const char *func_name = freeze ? "requester_freeze" : "requester_thaw";
QGAVSSRequesterFunc func;
@@ -164,5 +165,5 @@ void qga_vss_fsfreeze(int *nr_volume, bool freeze, Error
**errp)
return;
}
-func(nr_volume, &errset);
+func(nr_volume, mountpoints, &errset);
}
diff --git a/qga/vss-win32.h b/qga/vss-win32.h
index 4f8e39aa5c..ce2abe5a72 100644
--- a/qga/vss-win32.h
+++ b/qga/vss-win32.h
@@ -22,6 +22,7 @@ bool vss_initialized(void);
int ga_install_vss_provider(void);
void ga_uninstall_vss_provider(void);
-void qga_vss_fsfreeze(int *nr_volume, bool freeze, Error **errp);
+void qga_vss_fsfreeze(int *nr_volume, bool freeze,
+ strList *mountpints, Error **errp);
#endif
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 3d9c9716c0..0bd170eddc 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -234,7 +234,7 @@ out:
}
}
-void requester_freeze(int *num_vols, ErrorSet *errset)
+void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset)
{
COMPointer pAsync;
HANDLE volume;
@@ -245,7 +245,9 @@ void requester_freeze(int *num_vols, ErrorSet *errset)
SECURITY_ATTRIBUTES sa;
WCHAR short_volume_name[64], *display_name = sho
