[Qemu-devel] [PATCH v2 0/4] xen: don't save/restore the physmap on VM save/restore

Igor Druzhinin posted 4 patches 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1499183267-28623-1-git-send-email-igor.druzhinin@citrix.com
Test FreeBSD passed
Test checkpatch failed
Test docker failed
Test s390x failed
There is a newer version of this series
configure                     |  18 +++++++
hw/i386/xen/xen-hvm.c         | 105 ++++++++++++++++++++++++++---------------
hw/i386/xen/xen-mapcache.c    | 107 ++++++++++++++++++++++++++++++++++++++----
include/hw/xen/xen_common.h   |   8 ++++
include/sysemu/xen-mapcache.h |  11 ++++-
5 files changed, 201 insertions(+), 48 deletions(-)
[Qemu-devel] [PATCH v2 0/4] xen: don't save/restore the physmap on VM save/restore
Posted by Igor Druzhinin 6 years, 10 months ago
Saving/restoring the physmap to/from xenstore was introduced to
QEMU majorly in order to cover up the VRAM region restore issue.
The sequence of restore operations implies that we should know
the effective guest VRAM address *before* we have the VRAM region
restored (which happens later). Unfortunately, in Xen environment
VRAM memory does actually belong to a guest - not QEMU itself -
which means the position of this region is unknown beforehand and
can't be mapped into QEMU address space immediately.

Previously, recreating xenstore keys, holding the physmap, by the
toolstack helped to get this information in place at the right
moment ready to be consumed by QEMU to map the region properly.
But using xenstore for it has certain disadvantages: toolstack
needs to be aware of these keys and save/restore them accordingly;
accessing xenstore requires extra privileges which hinders QEMU
sandboxing.

The previous attempt to get rid of that was to remember all the
VRAM pointers during QEMU initialization phase and then update
them all at once when an actual foreign mapping is established.
Unfortunately, this approach worked only for VRAM and only for
a predefined set of devices - stdvga and cirrus. QXL and other
possible future devices using a moving emulated MMIO region
would be equally broken.

The new approach leverages xenforeignmemory_map2() call recently
introduced in libxenforeignmemory. It allows to create a dummy
anonymous mapping for QEMU during its initialization and change
it to a real one later during machine state restore.

---
Changed in v2:
* Patch 2: set dummy flag in a new flags field in struct MapCacheEntry
* Patch 3: change xen_remap_cache_entry name and signature
* Patch 3: gate ram_block_notify_* functions in xen_remap_bucket
* Patch 3: rewrite the logic of xen_replace_cache_entry_unlocked to
           reuse the existing entry instead of allocating a new one
* Patch 4: don't use xen_phys_offset_to_gaddr in non-compat mode

---
Igor Druzhinin (4):
  xen: move physmap saving into a separate function
  xen/mapcache: add an ability to create dummy mappings
  xen/mapcache: introduce xen_replace_cache_entry()
  xen: don't use xenstore to save/restore physmap anymore

 configure                     |  18 +++++++
 hw/i386/xen/xen-hvm.c         | 105 ++++++++++++++++++++++++++---------------
 hw/i386/xen/xen-mapcache.c    | 107 ++++++++++++++++++++++++++++++++++++++----
 include/hw/xen/xen_common.h   |   8 ++++
 include/sysemu/xen-mapcache.h |  11 ++++-
 5 files changed, 201 insertions(+), 48 deletions(-)

-- 
2.7.4


Re: [Qemu-devel] [PATCH v2 0/4] xen: don't save/restore the physmap on VM save/restore
Posted by no-reply@patchew.org 6 years, 10 months ago
Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 1499183267-28623-1-git-send-email-igor.druzhinin@citrix.com
Type: series
Subject: [Qemu-devel] [PATCH v2 0/4] xen: don't save/restore the physmap on VM save/restore

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c5f49bf xen: don't use xenstore to save/restore physmap anymore
da3fc1b xen/mapcache: introduce xen_replace_cache_entry()
e9069c8 xen/mapcache: add an ability to create dummy mappings
dfbd8c6 xen: move physmap saving into a separate function

=== OUTPUT BEGIN ===
Checking PATCH 1/4: xen: move physmap saving into a separate function...
Checking PATCH 2/4: xen/mapcache: add an ability to create dummy mappings...
ERROR: spaces required around that '|' (ctx:VxV)
#52: FILE: hw/i386/xen/xen-mapcache.c:185:
+                                           PROT_READ|PROT_WRITE,
                                                     ^

ERROR: spaces required around that '|' (ctx:VxV)
#64: FILE: hw/i386/xen/xen-mapcache.c:197:
+        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
                                                ^

ERROR: spaces required around that '|' (ctx:VxV)
#65: FILE: hw/i386/xen/xen-mapcache.c:198:
+                          MAP_ANON|MAP_SHARED, -1, 0);
                                   ^

total: 3 errors, 0 warnings, 82 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/4: xen/mapcache: introduce xen_replace_cache_entry()...
ERROR: spaces required around that '|' (ctx:VxV)
#80: FILE: hw/i386/xen/xen-mapcache.c:188:
+                                           PROT_READ|PROT_WRITE, 0,
                                                     ^

ERROR: spaces required around that '|' (ctx:VxV)
#93: FILE: hw/i386/xen/xen-mapcache.c:200:
+        vaddr_base = mmap(vaddr, size, PROT_READ|PROT_WRITE,
                                                 ^

WARNING: line over 80 characters
#162: FILE: hw/i386/xen/xen-mapcache.c:521:
+    while (entry && !(entry->paddr_index == address_index && entry->size == cache_size)) {

ERROR: line over 90 characters
#166: FILE: hw/i386/xen/xen-mapcache.c:525:
+        DPRINTF("Trying to update an entry for %lx that is not in the mapcache!\n", phys_addr);

WARNING: line over 80 characters
#173: FILE: hw/i386/xen/xen-mapcache.c:532:
+    xen_remap_bucket(entry, entry->vaddr_base, cache_size, address_index, false);

ERROR: space required before the open parenthesis '('
#174: FILE: hw/i386/xen/xen-mapcache.c:533:
+    if(!test_bits(address_offset >> XC_PAGE_SHIFT,

WARNING: line over 80 characters
#177: FILE: hw/i386/xen/xen-mapcache.c:536:
+        DPRINTF("Unable to update an entry for %lx in the mapcache!\n", phys_addr);

WARNING: line over 80 characters
#184: FILE: hw/i386/xen/xen-mapcache.c:543:
+uint8_t *xen_replace_cache_entry(hwaddr old_phys_addr, hwaddr new_phys_addr, hwaddr size)

total: 4 errors, 4 warnings, 192 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/4: xen: don't use xenstore to save/restore physmap anymore...
ERROR: space prohibited between function name and open parenthesis '('
#47: FILE: hw/i386/xen/xen-hvm.c:380:
+    physmap = g_malloc(sizeof (XenPhysmap));

total: 1 errors, 0 warnings, 98 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org